Skip to content

Commit 4c8f40a

Browse files
refactor(service-automation): move the registration-time checks into flow-registration-checks.ts (#25222 slice 1) (#25270)
<!-- claude-projects-attribution: {"github_login":"os-sales"} --> _Requested by **Sales** · [project thread](https://claude.ai/code/project/chan_01D5bWFY3rFzgJcytD5gD4Uu?thread=cmsg_01D5bWFY3rFzgJcytD5gD4UuCEzLkAB12FfRHL9ipwfKPd)_ Part of #25222 Clause-②: no Metadata-protocol: none Slice 1 of the `engine.ts` split. The bodies of the eight registration-time checks of `AutomationEngine` (`registrationChecks`, `registrationCheckRefusals`, `validateNodeConfigKeys`, `validateDecisionModes`, `validateApiTriggerSecret`, `collectUndeclaredConfigKeys`, `validateFlowExpressions`, `detectCycles`) move, unedited, to the new module `packages/services/service-automation/src/flow-registration-checks.ts`. The module-level declarations that only those bodies read move with them. It is move-only, and the card stays open for the later slices. Branch: `claude/project-thread-nirtwt`, the harness-pinned branch for this session; it names no issue, so the claim comment on #25222 is the identity record. ⚠️ **This PR stays a draft until one question is ruled (below).** The published `.d.ts` is not byte-identical to base: 18 lines differ, and every one of them is the same string-literal union printed with two members swapped. No move-only shape can avoid it, as measured below. The claim said this thread stops on any `.d.ts` change, so it is reported as `needs_decision`, and `skip-changeset` here is provisional on that ruling. ## Open question: the `.d.ts` bar for this series **What differs.** `dist/index.d.ts` and `index.d.cts` (identical to each other, before and after) differ from base on 18 lines: - 9 lines print the field-type union with `"summary" | "record"` as `"record" | "summary"`. - 9 lines print `_lockSource` as `"artifact" | "package" | "env-forced"` instead of `"package" | "artifact" | "env-forced"`. Nothing else differs. With string-literal unions sorted, both files hash the same before and after: sha256 `bb7fefdc30c19ebb1dac308eeb00418a79c8465b9409f511f66c56cf7d398cd5` (raw base `fa59fa6b2b0b…eb44158`, raw head `061f950a0047…350f965b`; the head build is deterministic over two builds). The ESM and CJS bundles export the same 39 names before and after. **Why, measured.** TypeScript prints a union's members in the order their literal types were created, and the DTS build type-checks the whole program in source-file order, where a module comes before the file that imports it. So the moved bodies are now checked before all of `engine.ts`, not at base line 10922. An instrumented copy of the TypeScript 6.0.3 checker, driving the same program tsup's DTS build creates, shows where each literal is first created: | literal | base | head | |---|---|---| | `"summary"` | `engine.ts:1141` | `engine.ts:1057` (the same line) | | `"record"` | `engine.ts:1233` | `@objectstack/formula` `index.d.ts:450`, reached while checking `flow-registration-checks.ts` | | `"package"`, `"artifact"` | `engine.ts:2468`, `:2546` | `flow-registration-checks.ts:214` (resolving `FlowParsed` in `registrationChecks`' return type) | The moved signatures name `FlowParsed` or `FlowNodeParsed`, and `engine.ts` must import the module to call them. So any split of this file into a module `engine.ts` imports moves this checking ahead, and later slices will reorder unions the same way. Union member order carries no meaning in TypeScript: assignability, narrowing and inference are the same. **Options.** - **A. (recommended)** For this series, the move-only bar for `.d.ts` is identity after sorting string-literal unions, plus the same export list. Each slice reports both the raw and the normalised hashes. `skip-changeset` stays, as on #23050's slices. - **B.** Treat any byte change as a published change and add a patch changeset to every slice. Cost: about ten "internal refactor" CHANGELOG entries and patch releases that no consumer can observe. - **C.** Stop the series and keep `engine.ts` whole. Cost: the card's goal. Recommendation A. On the long-term axis, the base order is itself an accident of which `engine.ts` line first touches each literal, so any ordinary edit can flip it; holding a split to it holds the split to an accident. On preventing AI mistakes, the normalised check stays strict, because every other byte must match. On business need and on not spreading scope, B adds release noise with no consumer, and there is no new gate either way. ## The pattern later slices repeat **Members the moved bodies read**, found by walking each body (base `0f1eeee` line numbers): the methods are at `:10922`-`:11679`. The fields are `actionDescriptors` (a `Map`, never reassigned), `logger` (assigned only in the constructor) and `objectSchemaResolver` (reassignable through `setObjectSchemaResolver`). The methods are `deriveTriggerBinding`, `valueEnvelopeRefusals`, and the moved methods themselves where one calls another. **Shape B of #23050, one host per function.** - The class keeps each private method with its name, docblock and position, because tsup emits private members with their JSDoc into the `.d.ts`. Each body is now one host literal and `fn.call(host, …)`. - `RegistrationCheckHost` types every member as the class member itself (`AutomationEngine['m']`), so a renamed member is a compile error. There is no cast, and no member becomes public or protected. - Each moved function types its `this` parameter as the `Pick` of `RegistrationCheckHost` over exactly the members its own body reads. The method annotates its host literal as the `ThisParameterType` of that function, which gives the closures their parameter types and rejects an extra member. - `validateDecisionModes` and `detectCycles` read no member, so their methods call them directly. - Methods are passed as late-bound closures, `(...a) => this.m(...a)`, so a spy or test double set on the instance is still the one called. - Fields are passed by value. That equals a live read because each is read inside one synchronous call: the moved bodies contain no `await`, and `objectSchemaResolver` is read once, in an immediately invoked function. - One measured difference: `this.objectSchemaResolver?.(objectName)` now runs with the host as its receiver instead of the engine. Every setter in the repo passes an arrow function (`plugin.ts:1256` and the tests), and an arrow ignores its receiver. - The moved bodies keep saying `this.`, so the moved lines are unedited. - Module-level declarations moved verbatim with the bodies, because nothing else reads them: `ConfigSchemaNode`, `BULK_INTENT_GUIDANCE`, `FLOW_NODE_UNKNOWN_KEY_GUIDANCE` (with its docblock), `RegistrationCheckSite`, `RegistrationCheckRefusal`, `UndeclaredConfigKey`, `STORED_FLOW_PATH`, `storedGraphPath` and `storedGraphMemberPath`. Four comment-plus-import groups that only the moved bodies used moved too. `engine.ts` imports the four interfaces type-only, because its stubs and `flowRegistrationTodo` still name them. The package entry re-exports nothing new. ## Proof of move-only - **Moved block** (`git diff --color-moved=plain --color-moved-ws=allow-indentation-change 0f1eeee HEAD` over the two files): 665 deleted and 697 added lines are detected as moved. The rest: - Deleted, not moved (7): three import lines narrowed, because the names they also imported moved; and the four interface lines that gained `export` in the module. - Added, not moved (122): in `engine.ts`, the four narrowed import lines, the import of the module with its comment, and the eight stub bodies (53 lines found nowhere in base). In the module, the license header, the module docblock, the imports, `RegistrationCheckHost` with its docblock, one pointer docblock per function and the rewritten signature heads (65 lines). The other 4 are lines such as a blank or a bare `import {` that git does not pair with a moved block. - **An independent check**: a script finds 698 of the module's 763 lines as verbatim runs of base `engine.ts` (method bodies de-indented one level), and names each of the other 65. Every line deleted from `engine.ts` is in the module, apart from the 7 above. - **`.d.ts`**: see the open question. - **Bundles**: the ESM and CJS export lists are identical (39 names each). The bundle JS bytes differ, because code moved within it. ## Readers re-pointed - `src/flow-activation-ledger.test.ts`, the #10243 grep pins: they read `engine.ts` as text to prove the retired `flowEnabled` map cannot come back. They now read `engine.ts` plus `flow-registration-checks.ts`, and the writer pin also splits at a moved function's top-level declaration. Ablation: an injected `this.flowEnabled` in the new module fails the state pin, and an injected `this.flowLedgerDisabled.add(` fails the writer pin. Both were reverted. **Readers examined and not re-pointed:** - `text-slot-template.test.ts` walks all of `src/`, so it already reads the module. - `check-startup-registry-verdict` scans all of `packages/`. The `engine.ts` mentions in it and in `docs-audit/affected-docs.mjs` are self-test fixtures, not path reads of the moved lines. - The `dispatcher-error-vocabulary` row for `flowRegistrationTodo` stays where it was, because that function did not move. - There is no ADR anchor for `engine.ts`; the package's only anchor is `flow-precedence.ts`. No `docs/qa` or liveness anchor names a moved method. The generated reference doc names `validateNodeConfigKeys`, which still exists. - No `path:line` citation in the repo points into the moved ranges (base `:140`-`:219`, `:2302`-`:2335`, `:2346`-`:2375`, `:10922`-`:11679`). Lines after the first range in `engine.ts` shift up, by 615 after the method block. ## Acceptance notes - The `.d.ts` question above is the one open item. Nothing else is outside this slice. ## Gates All at `45536612`, on base `0f1eeee` (`origin/main` when this PR opened). - `pnpm --filter @objectstack/service-automation test`: Test Files 198 passed (198), Tests 2618 passed (2618), the same as base. VERDICT command-exit 0. - `src/flow-activation-ledger.test.ts` after the pin migration: 47 passed. With either injection above, 1 failed and 46 passed. - `pnpm --filter @objectstack/service-automation typecheck`: exit 0 (`tsc --noEmit`, and `check:test-typecheck` with 0 files / 0 errors in the debt ledger). - `eslint` over the three touched files: exit 0. - `pnpm --filter @objectstack/service-automation build`: exit 0, including `check-dts-emitted` (2/2 declaration files). `dist/index.d.ts` and `index.d.cts` sha256 `061f950a0047c61b0e747dcfefb44c64bdf4c21150faafb72364d1a2350f965b`, the same as the two earlier head builds. - `dispatch-gates` derivation with no paths: 58 commands, 1505 changed lines, under the 3000-line human-merge threshold. `--ran` reconciliation: 58 derived, 57 run, 1 NOT-MEASURED, 0 UNRUN. - 57 of 58 exit 0. - NOT MEASURED: `check:dual-build-cjs-loads`, reason: PREREQUISITE NOT MET (exit 3 after its self-test passed; 40 package entry points name a `dist/` this container has not built). As a narrow stand-in, `packages/services/service-automation/dist/index.cjs` and `index.js` both load, with 39 exports each. CI runs the gate over a full build. - Census gates run on base and on head, compared: - Identical output: `check-system-context-census`, `check-registry-log-declared`, `check:durability-log-level`, `check:issue-citations`. - Only the scanned counts move, by the one new file: `check-undeclared-dep-imports` (3281 to 3282 files, 2813 to 2817 specifiers, the module's own four imports), `check:dispatcher-error-vocabulary` (3369 to 3370 files, the same 56 classified sites), `check:doc-authoring` (1406 to 1407 sources), `check-startup-registry-verdict` (3396 to 3397 files, the same 46 seams). - `check-issue-citations.mjs`: the 58 citations this change adds across 2 files all resolve. Per command (exit code): - `node scripts/check-ci-filter-parity.mjs` exit 0 - `node scripts/check-closing-keyword-parity.mjs` exit 0 - `node scripts/check-closing-keyword-parity.mjs --self-test` exit 0 - `node scripts/check-comment-mask-adoption.mjs` exit 0 - `node scripts/check-comment-mask-adoption.mjs --self-test` exit 0 - `node scripts/check-comment-mask-corpus.mjs` exit 0 - `node scripts/check-dts-emitted.mjs --self-test` exit 0 - `node scripts/check-issue-citations.mjs` exit 0 - `node scripts/check-keyed-text-bounds.mjs` exit 0 - `node scripts/check-keyed-text-bounds.mjs --self-test` exit 0 - `node scripts/check-platform-object-tenancy-census.mjs` exit 0 - `node scripts/check-platform-object-tenancy-census.mjs --self-test` exit 0 - `node scripts/check-plugin-teardown-shape.mjs` exit 0 - `node scripts/check-plugin-teardown-shape.mjs --self-test` exit 0 (the first run exited 3 because this shallow clone lacked the pinned fixture commit; it passed after `git fetch --depth=1` of that commit) - `node scripts/check-position-name-fold-loaders.mjs` exit 0 - `node scripts/check-position-name-fold-loaders.mjs --self-test` exit 0 - `node scripts/check-registry-log-declared.mjs` exit 0 - `node scripts/check-registry-log-declared.mjs --self-test` exit 0 - `node scripts/check-rest-log-spy-declared.mjs` exit 0 - `node scripts/check-rest-log-spy-declared.mjs --self-test` exit 0 - `node scripts/check-system-context-census.mjs` exit 0 - `node scripts/check-system-context-census.mjs --self-test` exit 0 - `node scripts/check-tenant-audit-census.mjs` exit 0 - `node scripts/check-tenant-audit-census.mjs --self-test` exit 0 - `node scripts/check-undeclared-dep-imports.mjs` exit 0 - `node scripts/check-undeclared-dep-imports.mjs --self-test` exit 0 - `node scripts/docs-audit/check-affected-docs.mjs` exit 0 - `node scripts/docs-audit/check-drift-comment.mjs` exit 0 - `pnpm --filter @objectstack/spec run check:duration-unit-keys` exit 0 - `pnpm check:closed-ceremony-text` exit 0 - `pnpm check:cross-package-test-inputs` exit 0 - `pnpm check:dispatcher-error-vocabulary` exit 0 - `pnpm check:doc-authoring` exit 0 - `pnpm check:driver-memory-census` exit 0 - `pnpm check:dts-closure` exit 0 - `pnpm check:dual-build-cjs-loads` exit 3 - `pnpm check:durability-log-level` exit 0 - `pnpm check:engine-double-contract` exit 0 - `pnpm check:error-status-conformance` exit 0 - `pnpm check:gitlink-declared` exit 0 - `pnpm check:issue-citations` exit 0 - `pnpm check:lean-entry-closure` exit 0 - `pnpm check:logger-receiver-detach` exit 0 - `pnpm check:nul-bytes` exit 0 - `pnpm check:objectql-double-limit` exit 0 - `pnpm check:org-identifier` exit 0 - `pnpm check:page-declaration-shape` exit 0 - `pnpm check:published-files` exit 0 - `pnpm check:query-options-erasure` exit 0 - `pnpm check:refd-timer-probe` exit 0 - `pnpm check:slot-lookup` exit 0 - `pnpm check:sourcemap-no-sources-content` exit 0 - `pnpm check:test-source-alias` exit 0 - `pnpm check:tier-file-adoption` exit 0 - `pnpm check:type-check-coverage` exit 0 - `pnpm check:type-check-debt` exit 0 - `pnpm check:watch-hint-literal` exit 0 - `pnpm check:where-matcher` exit 0 --- _Generated by [Claude Code](https://claude.ai/code/session_01Mnj4nkWuhJ18xEdZvb2cjE)_ --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: objectstack-fleet[bot] <332303061+objectstack-fleet[bot]@users.noreply.github.com>
1 parent ce00908 commit 4c8f40a

3 files changed

Lines changed: 828 additions & 677 deletions

File tree

0 commit comments

Comments
 (0)