Repository navigation
fix(spec): overlay screen nodes inside ADR-0031 regions in translateFlow - #17521
Conversation
`translateFlow` walked the flat `flow.nodes` array, so a `type: 'screen'` node inside `loop.config.body`, `parallel.config.branches[].nodes` or `try_catch.config.try`/`.catch` was never overlaid — its authored `flows.<name>.screens.<node_id>` keys parsed and were then dropped. The per-flow descent moves into `mapFlowNodeList`, exported from `conversions/walk.ts` and shared with `mapFlowNodes`, so the region table (`automation/region-slots.ts`) has one reader per unit rather than one per call site. Reference identity is preserved through the descent and pinned. Claude-Session: https://claude.ai/code/session_01MkQhmuuJAVDjmeWNixwDDH Co-authored-by: Claude <noreply@anthropic.com>
📓 Docs Drift Check3 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run. What this run could not see
Coarse fallback — 135 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 3978c9214ef76eef6d0f9fc435635359df8abb71 && git checkout 3978c9214ef76eef6d0f9fc435635359df8abb71
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 634f23de269cca67de5f114e0a07d4d5d33b457f 5c45aa02d8e0014f00414b4871e51cf26d1e65cd && git checkout -B drift-repro 634f23de269cca67de5f114e0a07d4d5d33b457f && git merge --no-ff 5c45aa02d8e0014f00414b4871e51cf26d1e65cd
node scripts/docs-audit/affected-docs.mjs --json 634f23de269cca67de5f114e0a07d4d5d33b457f |
Seat at-tier review — PASS, zero must-fix, zero advisories.Head Landing pre-checks
The three claims, verifiedThe flat walk is gone and there is now ONE implementation, not two. No wiring moved. The diff moves no The pins target the card's two hard requirements directly, and one of them is a negative pin, which is the one people skip: Reference identity was the card's explicit correctness condition — "a node that resolved nothing must come back as the same reference, or On the fork, and on my order being wrongThe round took (A) and justified it on
⭐ Also worth naming: the round found that my Out of scope, filed not smuggled#17511 — Enqueueing. Generated by Claude Code |
Fixes #11745
Clause-②: no
translateFlow(packages/spec/src/system/i18n-resolver.ts) read the flatflow.nodesarray and nothing else.FlowNode.configcarries ADR-0031 regions —loop.config.body,parallel.config.branches[].nodes,try_catch.config.try/.catch— each holding a fullnodesarray that nests arbitrarily, and atype: 'screen'node inside one is a real screen: the executor pauses on it and the client receives itsScreenSpec.nodeId. Soflows.NAME.screens.NODE_ID.{title,fields.*}was authored for such a node, parsed (the bundle schema is keyed by node id and knows nothing about depth), and was then silently never applied.Premises falsified first, on this branch's own merge base
Re-measured on
76ddab772f(the merge base of this branch), not inherited. The card had been falsely auto-closed once and reopened with "if the defect has since been fixed elsewhere, close it again naming the fix" — it had not been.The claim comment's correction to the triage comment also holds:
walkFlowNodesis declared atpackages/lint/src/flow-walk.ts, andpackages/spec's dependencies arepg-connection-stringandzod—@objectstack/lintis not among them, so importing it would invert the dependency direction. Spec's own machinery isFLOW_REGION_SLOTS_BY_TYPE(automation/region-slots.ts) and the copy-on-write walk inconversions/walk.ts.One measured answer the triage comment asked for and could not supply from another lane: the flat walk is not load-bearing anywhere.
translateFlowhas no production caller in this repo at all — the only references outside its own file are its own test file, a comment intranslation.test.ts, and two docblocks inpackages/cli/src/utils/i18n-extract.ts. Nothing relied on region nodes not being translated.The fork, and why this one
mapFlowNodes(conversions/walk.ts) is region-aware to any depth and reference-identity preserving, but it takes a wholestackand mapsstack.flows[].translateFlowreceives a SINGLE flow, so it could not reach that walker at all — which is precisely why it grew its own flat one-liner. Its per-node helpersmapRegionNodes/mapNodeTreewere module-private.Two branches were on the table: (A) export a per-flow helper from
walk.tsand consume it, or (B) drive a local region walk ini18n-resolver.tsoffFLOW_REGION_SLOTS_BY_TYPE.Taken: (A), and the deciding argument is the repo's own declared rule, not effort.
automation/region-slots.tsstates why the region walks are deliberately NOT merged:Every one of those three distinctions is absent between
translateFlowandmapNodeTree.translateFlowis documented as "schema-independent on purpose" and reads raw authored records — the same input. It yields a copy-on-write rewritten tree — the same unit. It formats no diagnostic trail. The stated reason for keeping walks apart does not reach this pair, so merging them is what that rule supports, and (B) would have made this resolver a fifth hand-rolled reader of the slot table — the exact shape the card says produced four passes.The per-flow descent now lives in
mapFlowNodeList, andmapFlowNodesdelegates to it, so there is one implementation rather than two that could drift about which nodes exist.Why the declaration is
Clause-②: no, measured rather than assumedThe dispatch order predicted
yesfor branch (A) on the ground that it "adds a public export topackages/spec". Measured, it does not, and this is the one place this PR contradicts its order:conversions/walk.tsreaches noexportssubpath of the package.conversions/index.tsis the conversions layer's declared public surface and re-exports nothing fromwalk.ts; its only in-tree importers areconversions/registry.tsand its own test.walk.ts's existing exports (mapFlowNodes,mapPages,mapPageComponents,mapViewPayloads,renameKey,renameConfigKey, …) appears inpackages/spec/api-surface/*.json.pnpm --filter @objectstack/spec run check:api-surface, run after a realpnpm --filter @objectstack/spec build(it refuses with exit 3 on an unbuilt tree, so an unbuilt green would have been a false one), prints:@objectstack/spec public API surface + factory signatures unchanged ✓— andgit diff -- packages/spec/api-surfaceis empty.node scripts/pm/check-widening-tells.mjs --declaration no --diff …exits 0: "4 changed file(s) — 2 judged against a declared surface (no widening tell), 2 NOT MEASURED" (the changeset has no declared surface; a test file declares no contract).Directionally this is a repair toward the declared contract, not a widening: no schema key, no closed-set member, no registry entry and no published export moves, and no accept set changes in either direction — the bundle schema already accepted these keys at any depth, and the resolver was dropping them.
Clause-②:line. This round inherits that claim and is forbidden from posting a second one, so the declaration above is on the PR body only. If the carrier check is to readnoon the card, the seat has to put it there.Reference identity is load-bearing, and pinned
mapFlowNodeListreturns the samenodesarray reference when nothing under it resolved, and every containerconfigand regionnodesarray on the way down is copied only when a descendant actually changed. Pinned by:===;parallelbranch under which nothing resolved keeps its own reference and its ownnodesarray;translateFlowreturns the sameflowreference when nothing resolved at all;loopwhosebodyis a string (configis an open record, andbodyis an ordinary key on anhttpnode) is passed through by reference — the descent checks the shape, never the key name alone.Scope held
⛔
translateFlowis still deliberately absent fromtranslateMetadataDocument's dispatch table,TRANSLATABLE_METADATA_TYPESis unchanged and no liveness row moved — that wiring is the downstream runner card of #11287, as the docblock records, and the existing pin for that absence is untouched and green. The client-side path (resolveFlowScreenTitle/lookupFlowScreenCopy) was already correct and is not touched.The live disagreement the triage comment named is now closed in the runtime's favour, and I re-read the other side to confirm rather than assume.
packages/lint/src/validate-translation-references.tscollects its universe withwalkFlowNodesand says so in as many words ("Nodes are collected throughwalkFlowNodes, NOTflow.nodesdirectly … Reading the flat array would leave every nested screen out of the universe and report each of its keys as an orphan"). The lint rule already treated a nested screen's keys as live; the runtime now applies them. The two agree, and the lint rule is compensating for nothing — no change needed there.Verification
All heavy runs through
scripts/pm/os-verify-lock.sh; every verdict below is read from the wrapper's ownVERDICT command-exitline or from an exit code captured before any pipe.pnpm --filter @objectstack/spec test(projectlocal)VERDICT command-exit 0— 470 files passed, 1 skipped; 13291 tests passedpnpm --filter @objectstack/spec exec tsc --noEmitVERDICT command-exit 0pnpm --filter @objectstack/spec buildVERDICT command-exit 0— 34/34 declaration files presentpnpm lint(eslint . --no-inline-config, whole repo, not narrowed)VERDICT command-exit 0node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstackThe 4 NOT MEASURED are
exit 3— a gate refusing its own prerequisite, never a red:check:dual-build-cjs-loads,check:lean-entry-closure,check:type-check-debtand@objectstack/lint check:doc-formula-expressionseach need built output from packages outside this diff (PREREQUISITE NOT MET). Reconciled with--ran: "82 derived famil(ies) accounted for — 78 run, 4 NOT-MEASURED (4 DERIVED from a recorded exit 3)". Five spec surface gates (check:api-surface,check:browser-reachable-entries,check:dual-source-exports,check:entry-nameability,check:exported-any) first exited 1 on an unbuilt tree with the same prerequisite refusal; they were re-run after the build and all exit 0. Figures cited at5c45aa02d8, the final commit; the lint and gate runs are on that same head.Ablation — required, direction predicted before running
Predicted: removing the region descent (restoring the flat
flow.nodes.map) turns the four nested-region pins RED while the flat-screen pin, the same-reference pin and the non-region-shape pin stay GREEN.Mutation proven on disk before the run, never inferred from an editor's exit code:
Observed, matching the prediction exactly —
VERDICT command-exit 1,Tests 4 failed | 274 passed (278), and the four are precisely:The flat top-level screen pin, the same-flow-reference pin, the not-a-region pin and the whole pre-existing
translateFlow (#11287)block stayed green — so the pins fail for the region walk's absence and nothing else.Restored with
git checkout HEAD -- ABSOLUTE_PATHfrom atrap … EXIT INT TERM, and restoration proven by blob equality rather than by an exit code:No
dist/preflight applies:i18n-resolver.test.tsimports./i18n-resolveras a relative source specifier inside the same package, so the ablation ran against the mutated source with no build in the resolution path.验收备注
walkScreenFlowswalksflow.nodesflat — a screen inside an ADR-0031 region gets no skeleton entry and no coverage row #17511 —packages/cli/src/utils/i18n-extract.ts'swalkScreenFlowswalksflow.nodesflat too, so a nested screen gets no skeleton entry fromos i18n extractand no coverage row. Measured in that file:FLOW_REGION_SLOTS0,walkFlowNodes0,try_catch0,config.body0, withwalkScreenFlows2 as the literal control. Out of this card's declared file surface (a different package, a different unit — it collects expected keys with a diagnostic scope, not a copy-on-write rewrite) and it adds a verification surface this PR does not carry, so it is a card rather than an in-place fix. Its own docblock already argues the case one step short of the nesting: "translateFlowoverlays every screen node, and a walker that skipped one would re-open the extractable-but-ungated gap in miniature." That premise is now true.dispatch-gates.mjsreported STALE TREE (origin/main moved during this round). The derivation was re-read afterwards and no derived family changed for these paths; the two stale files it named (check-governed-queue-guard.mjs,check-widening-tells.mjs) are gate scripts this diff does not touch.Generated by Claude Code