Repository navigation
Conversation
k0stik
force-pushed
the
chore/SOF-8032
branch
2 times, most recently
from
August 20, 2026 16:19
9864917 to
a02d972
Compare
…in generic [release]
ide was the last package in the stack still built with babel from plain JS,
with its public types supplied by a hand-written, never-typechecked
compute.d.ts. ComputedEntityMixin declared `readonly compute: unknown`,
which collides with every consumer's generated schema mixin also declaring
a real `compute` (TS2320 requires identical types across multiple extends) -
forcing wode's Workflow.ts and jode's Job.ts to redeclare `compute` with a
`// TODO: fix ComputedEntityMixin and remove this`, and wode's Subworkflow.ts
to `Omit<ComputedEntityMixin, "compute">` with no explanation.
- Convert src/js/*.js -> *.ts, delete the hand-written compute.d.ts, add
tsconfig.json/tsconfig-transpile.json (matching code/made/wode/jode),
transpile switches from babel to `tsc -p tsconfig-transpile.json`.
- ComputedEntityMixin<C> is now generic over the compute payload (defaults
to esse's ComputeArgumentsSchema | undefined). Dependents (clusterFqdn,
clusterJid, timeLimit, computeQueue, errors) derive from C via indexed
access instead of being hardcoded `unknown`. `compute`'s own optionality
toggles on whether `undefined extends C` (`ComputeField<C>`): confirmed via
wode's Workflow.ts/Subworkflow.ts that TS2320 treats an optional property
(`compute?: X`, what esse's WorkflowSchema/SubworkflowMixinSchema actually
declare) and a required property typed `X | undefined` (what a naive
`compute: C` gives once C includes undefined) as *not* identical, so a
fixed modifier can never match both those schemas and esse's JobSchema
(compute required, no undefined) at once - only the conditional does.
- Fixed a latent runtime bug found while auditing: computedEntityMixin's
`compute` was getter-only. Applied after a generated schema mixin (as in
jode's Job.ts and wode's Subworkflow.ts), its getter-only descriptor
silently overwrote the schema mixin's get+set pair, so `job.compute = x`
threw `TypeError: Cannot set property compute which has only a getter`
even though the merged interface claimed compute was writable. `compute`
now has a real setter, verified with a descriptor probe before and after
plus a new regression test.
- bucket/filesRootDir guard against absent compute.cluster.fqdn (real per
the schema - both stay non-throwing, returning "").
- getApproximateCharge now throws a clear error when compute.timeLimit is
missing instead of silently producing a 0 charge (would otherwise let a
job through business_object.ts's balance check for free).
- Dropped @babel/* (7 packages) from dependencies - only moment/pluralize
are real runtime deps; added @mat3ra/tsconfig, typescript, ts-node,
@types/{node,mocha,pluralize}. Deleted .babelrc/.mocharc.json (ts-node
registers directly in the test script now, matching jode).
- Collapsed enums.ts's RMSNotificationsHandler class into a single
getNotificationValue() function: it was never exported, had exactly one
call site (always "PBS"), and the "adjust to make modular ... SLURM etc."
TODO it existed for was never implemented (no SLURM config object
anywhere) - a premature abstraction for a fixed 5-string computation.
EMAIL_NOTIFICATIONS' runtime output is unchanged.
Not run through eslint - same situation as jode: @exabyte-io/eslint-config
uses @babel/eslint-parser, which cannot parse the converted .ts files
(`npm run lint`/`lint:fix` now error standalone), and ide's own CI already
passes `skip-eslint: true` for this reason. The pre-commit hook itself is
unaffected: it runs lint-staged, whose src/js/**/*.js and tests/js/**/*.js
globs now simply match zero files and skip gracefully (verified). Wiring
up @typescript-eslint against the pinned eslint-config-airbnb chain is a
separate, larger change (same conclusion reached for jode).
Verified: `tsc --noEmit` clean, `npm test` 8/8 passing (incl. 2 new
regression tests for the setter fix and the timeLimit guard), `npm run
transpile` emits dist/js/*.d.ts including index.d.ts, and a runtime smoke
test against the compiled dist/js output (not just ts-node).
…ue object [release]
Was Array<{label, value}>, purely to support the array .find()-by-label
lookup that getNotificationValue() (née RMSNotificationsHandler) did
internally - now redundant now that lookup is a direct property access.
EMAIL_NOTIFICATIONS' runtime output is unchanged (verified).
The one real external consumer of the array shape is web-app's Cypress
e2e helper (ComputeFormWidget.ts, `.find(o => o.label === optionName)`) -
updated in a paired web-app change/commit to do a plain property lookup
instead, landed together with web-app's ide repin so nothing breaks
in between.
…ke our generated mixins [release] computedEntityMixin took `prototype: object` and returned `void`, with its own hand-rolled ComputedEntityHost<C> host-contract type (a `prop(name: string, default?): T` signature unrelated to the real one) - unlike every generated `*SchemaMixin` and hand-written mixin in `code`/`wode`/`jode`, which take `item: InMemoryEntity` and return `asserts item is T & Mixin`. - computedEntityMixin<C, S, T>(item: InMemoryEntity): asserts item is T & ComputedEntityMixin<C>. S is the extra host schema (owner/isExternal/ createdAt/workDir - not part of esse's bare job/workflow schemas, so any concrete schema missing them still satisfies S's constraint since they're all optional) layered on top of InMemoryEntity<S> itself, matching how a generated mixin's own `properties: InMemoryEntity<X> & X` + @ts-expect-error idiom works. - Dropped every `this.prop("compute.foo.bar", default)` dotted-path call. InMemoryEntity.prop's real signature is `prop<K extends keyof S>(name: K, ...)` - a single top-level key, not a dotted path - so passing "compute.cluster.jid" never actually typechecked against it (the previous ComputedEntityHost<C> silently allowed it via a fictional loose `prop(name: string)` signature that doesn't match the real one). Getters now read `this.compute?.cluster?.jid` etc. through the (now real) `compute` getter instead - verified identical at runtime against the compiled dist/js output, not just ts-node. - `set compute`/`setCompute` write directly to `this._json.compute` instead of calling `this.setProp("compute", value)`: setProp's signature (`value: S[typeof name]`) isn't call-site generic the way `prop`'s `<K extends keyof S>` is, so it resolves to the full S[keyof S] union rather than the one field's type when S is itself a still-abstract generic parameter (a real gap in code's own types, not something to work around with a cast here) - same fix and same reasoning as jode's Job.setMaterial. - owner/createdAt/workDir (bucket, filesRootDir, getApproximateCharge) now read via `this.prop("owner")` etc. instead of `this.owner`/`this.createdAt` /`this.workDir` direct property access - InMemoryEntity<S> only exposes S's keys through prop()/setProp(), not as direct instance properties, unless an actual mixin adds real getters for them. Verified: tsc clean, npm test 8/8, and a runtime check against the compiled dist/js output for every rewritten getter (clusterJid/clusterFqdn/ clusterFqdnShort/timeLimit/computeQueue/computePPN/computeNodes/errors/ bucket/filesRootDir) plus the compute setter and getApproximateCharge - all identical to pre-rewrite behavior. Also cross-checked by temporarily copying the rebuilt dist/js into wode's and jode's node_modules and running their own tsc+test suites against it (both green) before restoring their real installed copies via npm ci.
…d host fields [release] filesRootDir has zero consumers anywhere in the stack - web-app forked its own safer rewrite (Job.getFilesRootDirClient, taking owner as a param instead of reading this.owner, and using match?.[1] instead of a bare [1]) instead of using this getter, and nothing else calls it (verified via `grep -rn "\.filesRootDir\b"` across every reference/* package and web-app, plus ide's own tests). Dropped createdAt/workDir/owner.slug/owner.isPersonal/this.id from ComputedEntityHostSchema/the implementation along with it - they existed solely to support filesRootDir. owner.serviceLevel.nameBasedModifier stays (getApproximateCharge still reads it). Verified: tsc clean, npm test 8/8, and cross-checked by copying the rebuilt dist/js into wode's and jode's node_modules and running their own tsc+test suites against it (both green) before restoring their real installed copies via npm ci.
…eCharge [release]
getApproximateCharge read this.prop("owner")?.serviceLevel?.nameBasedModifier
directly - owner/serviceLevel with that shape are 100% web-app structures
(CoreAccount + ServiceLevel), not something esse's bare job/workflow schema
or ide itself has any business knowing about. The only real caller
(business_object.ts's ensureSufficientBalance) already loads the owner
account right there to check its balance, so it can compute the rate
modifier itself.
getApproximateCharge now takes `rateModifier` as part of its existing
`settings` object (defaults to 1, same as before) instead of reaching for
`this.prop("owner")`. owner/serviceLevel dropped from
ComputedEntityHostSchema entirely - nothing else in the mixin used them
(filesRootDir, the other consumer, was already removed).
web-app's own call site needs a paired follow-up (pass
`owner.serviceLevel?.nameBasedModifier` as `settings.rateModifier`) -
not done here, same as the ide.d.ts-shim/entityTypes.ts cleanup already
deferred earlier in this branch's history.
Verified: tsc clean, npm test 9/9 (added a regression test for
rateModifier), and cross-checked by copying the rebuilt dist/js into
wode's and jode's node_modules and running their own tsc+test suites
against it (both green) before restoring their real installed copies
via npm ci.
…n + infrastructureMixin [release] Audited every member against real usage across web-app, wode, jode, and the designer packages (wove/workflow-designer/job-designer/jove): compute/ setCompute/unsetCompute are genuinely shared (workflow-designer's own Subworkflow.tsx reads/writes subworkflow.compute directly), but clusterJid/clusterFqdn/clusterFqdnShort/timeLimit/computeQueue/computePPN/ computeNodes/computeNodesAndPPN/errors/getApproximateCharge/isExternalJob/ bucket/warnings/hasWarnings have zero non-Job callers anywhere in the stack - wode itself never touches any of them despite merging the type. - computedEntityMixin keeps only what's derived from `compute` itself: compute/setCompute/unsetCompute plus the compute-derived readouts (cluster*, timeLimit, computeQueue, computePPN, computeNodes, computeNodesAndPPN, errors). Makes sense for any entity carrying a compute config - Job, Workflow, or Subworkflow. - New infrastructureMixin (src/js/infrastructure.ts) holds what only makes sense for a job actually running somewhere: getApproximateCharge (billing), bucket/isExternalJob (cloud storage), warnings/hasWarnings. Requires computedEntityMixin already applied to the same host (reads clusterFqdn/ timeLimit/computeQueue/computePPN from it). - Dropped timePrediction entirely - hardcoded `return 0`, zero consumers anywhere, even in ide's own tests. - wode's Workflow.ts/Subworkflow.ts and jode's Job.ts need ZERO changes - verified by cross-checking their own tsc+test suites against the split dist/js before this commit. Neither ever referenced anything now in infrastructureMixin. Only web-app's own imports/jobs/job.ts (which calls job.bucket/job.getApproximateCharge/overrides warnings) needs infrastructureMixin added - separate follow-up commit there. Verified: tsc clean, npm test 9/9, a runtime check against the compiled dist/js output for both mixins together (clusterFqdn -> bucket -> getApproximateCharge -> isExternalJob -> warnings, all identical to pre-split behavior), and cross-checked wode's and jode's own tsc+test suites against the rebuilt dist before this commit.
… [release] Both were being silently shadowed in web-app's own imports/jobs/job.ts: computedEntityMixin(Job.prototype) runs after the class body, and Object.defineProperties always overwrites, so it was already clobbering job.ts's own get bucket()/get warnings() (verified empirically) - meaning job.warnings has ALWAYS returned [] in production and the hasBeenActiveMoreThanOnce message never actually surfaced. Rather than reorder application to make the mixin's own no-op warnings/isExternal-aware bucket win, dropping them here lets the host's own override take effect at all, and lets isExternal-based job bucketing be retired outright. - warnings/hasWarnings removed entirely - InfrastructureMixin now only has getApproximateCharge and bucket. web-app's own job.ts's warnings override (real behavior, no longer shadowed) is a separate follow-up commit there. - isExternalJob removed; bucket no longer branches on it - just clusterFqdn-derived, always. getExternalBucket/ExternalBucket (default.ts) dropped too - existed solely for the removed branch, zero consumers anywhere in the stack (verified). - wode/jode need zero changes (re-verified) - neither used warnings/ isExternalJob/bucket. Verified: tsc clean, npm test 9/9, runtime check against compiled dist/js confirming isExternalJob/warnings are gone and bucket still resolves correctly, plus wode's and jode's own tsc+test suites cross-checked against the rebuilt dist before this commit.
…n, not unconditional [release] Spotted by inspection: timeLimit/computeQueue were typed `Compute<C>["timeLimit"] | undefined` / `Compute<C>["queue"] | undefined` unconditionally, regardless of whether `compute` itself might be absent - even though esse's ComputeArgumentsSchema requires queue/nodes/ppn/ timeLimit *within* compute (required: ["queue","nodes","ppn","timeLimit"]). So for Job (compute required, no undefined in C), job.timeLimit was typed `string | undefined` even though it can only actually be undefined if compute itself is missing - which the type already promises won't happen. clusterJid/clusterFqdn didn't have this bug: `cluster` genuinely is optional *within* compute (not in that required list), so `Cluster<C>["jid"]`/`["fqdn"]` already carry `| undefined` correctly from the schema itself, no manual suffix needed. computePPN/computeNodes/errors don't have it either - their getters have `?? default` fallbacks (`this.compute?.ppn ?? 1`, `?? []`), so they're genuinely never undefined regardless of whether compute exists. - New `RequiredWithCompute<C, V>` - same `undefined extends C` conditional `ComputeField<C>` already uses, applied one level down: `V` when compute is guaranteed present, `V | undefined` when it might not be. - timeLimit/computeQueue now use it instead of a bare `| undefined` suffix. Verified: tsc clean, npm test 10/10 (added a compile-time regression test: required-compute's timeLimit assigns to plain `string` cleanly, optional-compute's needs @ts-expect-error against the same assignment), and cross-checked wode's and jode's own tsc+test suites against the rebuilt dist before this commit (Workflow/Subworkflow's timeLimit correctly stays `string | undefined`; Job's is now plain `string`).
…elease] The host app's Queue class mixed client-safe schema-derived getters with a getETA() that queried its jobs collection directly, which meant every consumer of Queue (including client-side rendering) pulled in a Mongo/Meteor dependency. Move the whole class here instead, with queueSettings and jobs passed to the constructor rather than reached for via a global - keeps this class free of any host-app-specific or Mongo/Meteor dependency, safe to construct on both client and server. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
getETA had exactly one caller in the host app, which now implements it itself as a private use-case method operating on the jobs collection it already has direct access to. Queue no longer needs a jobs finder at all, simplifying it back down to just queueSettings DI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| // Note: has more keys than esse's `job/compute.json#properties/queue` union (GPOF/GP2OF/GP4OF/ | ||
| // GPSF/GP2SF/GP4SF are not in the schema), so this stays a plain string index rather than typed | ||
| // against that union. | ||
| export const QUEUE_DISPLAY: Record<string, string> = { |
Member
There was a problem hiding this comment.
- We have removed GPOF and GP2OF from production
- We should probably manage QUEUE_TYPES via ESSE and/or standata instead of here directly
…[release] Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
QUEUE_TYPES is now an alias of esse's generated QueueNameEnum instead of a hand-kept copy, so the queue names have one source of truth. QUEUE_DISPLAY is typed Record<QueueNameEnum, string>, so adding a queue in esse fails to compile until it gets a label here; the pre-2020 "OF+"/"SF+" spellings (renamed to OFplus/SFplus in 2020-10) move to a separate legacy map. Esse's queue field is now the enum rather than a literal union, so the default compute config and tests use enum members instead of bare strings. Repin @mat3ra/esse to the WIP release that adds QueueNameEnum (wip-eed5826). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…gacy-peer-deps [release] The esse repin regenerated package-lock.json with --legacy-peer-deps, which dropped eslint-import-resolver-meteor and eslint-plugin-react-hooks, so CI's `npm ci` rejected the lockfile as out of sync and the WIP release never published. Regenerate without the flag. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…in generic [release]
ide was the last package in the stack still built with babel from plain JS, with its public types supplied by a hand-written, never-typechecked compute.d.ts. ComputedEntityMixin declared
readonly compute: unknown, which collides with every consumer's generated schema mixin also declaring a realcompute(TS2320 requires identical types across multiple extends) - forcing wode's Workflow.ts and jode's Job.ts to redeclarecomputewith a// TODO: fix ComputedEntityMixin and remove this, and wode's Subworkflow.ts toOmit<ComputedEntityMixin, "compute">with no explanation.tsc -p tsconfig-transpile.json.unknown.computewas getter-only. Applied after a generated schema mixin (as in jode's Job.ts and wode's Subworkflow.ts), its getter-only descriptor silently overwrote the schema mixin's get+set pair, sojob.compute = xthrewTypeError: Cannot set property compute which has only a gettereven though the merged interface claimed compute was writable.computenow has a real setter, verified with a descriptor probe before and after plus a new regression test.Not run through eslint - same situation as jode: @exabyte-io/eslint-config uses @babel/eslint-parser, which cannot parse the converted .ts files (
npm run lint/lint:fixnow error standalone), and ide's own CI already passesskip-eslint: truefor this reason. The pre-commit hook itself is unaffected: it runs lint-staged, whose src/js//*.js and tests/js//*.js globs now simply match zero files and skip gracefully (verified). Wiring up @typescript-eslint against the pinned eslint-config-airbnb chain is a separate, larger change (same conclusion reached for jode).Verified:
tsc --noEmitclean,npm test8/8 passing (incl. 2 new regression tests for the setter fix and the timeLimit guard),npm run transpileemits dist/js/*.d.ts including index.d.ts, and a runtime smoke test against the compiled dist/js output (not just ts-node).