Skip to content

fix(spec): overlay screen nodes inside ADR-0031 regions in translateFlow - #17521

Merged
os-bill merged 1 commit into
mainfrom
claude/issue-11745-translate-flow-regions
Sep 10, 2026
Merged

os-bill merged 1 commit into
mainfrom
claude/issue-11745-translate-flow-regions

Conversation

@os-bill

@os-bill os-bill commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Fixes #11745

Clause-②: no

translateFlow (packages/spec/src/system/i18n-resolver.ts) read the flat flow.nodes array and nothing else. FlowNode.config carries ADR-0031 regions — loop.config.body, parallel.config.branches[].nodes, try_catch.config.try / .catch — each holding a full nodes array that nests arbitrarily, and a type: 'screen' node inside one is a real screen: the executor pauses on it and the client receives its ScreenSpec.nodeId. So flows.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.

packages/spec/src/system/i18n-resolver.ts
  translateFlow  → const nodes = Array.isArray(flow.nodes) ? flow.nodes.map(…)   ← still flat
  walkFlowNodes      references in this file : 0
  FLOW_REGION_SLOTS  references in this file : 0
  mapFlowNodes       references in this file : 0
  LIT CONTROL 'translateScreenNode'          : 3     ← the zeros are absences, not a bad path

The claim comment's correction to the triage comment also holds: walkFlowNodes is declared at packages/lint/src/flow-walk.ts, and packages/spec's dependencies are pg-connection-string and zod — @objectstack/lint is not among them, so importing it would invert the dependency direction. Spec's own machinery is FLOW_REGION_SLOTS_BY_TYPE (automation/region-slots.ts) and the copy-on-write walk in conversions/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. translateFlow has no production caller in this repo at all — the only references outside its own file are its own test file, a comment in translation.test.ts, and two docblocks in packages/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 whole stack and maps stack.flows[]. translateFlow receives 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 helpers mapRegionNodes / mapNodeTree were module-private.

Two branches were on the table: (A) export a per-flow helper from walk.ts and consume it, or (B) drive a local region walk in i18n-resolver.ts off FLOW_REGION_SLOTS_BY_TYPE.

Taken: (A), and the deciding argument is the repo's own declared rule, not effort. automation/region-slots.ts states why the region walks are deliberately NOT merged:

The walks are deliberately NOT merged: they take different inputs (parsed FlowNodeParsed vs raw authored records), yield different units (a graph, a node, a rewritten tree), and one of them formats human diagnostic trails, which is consumer logic rather than protocol (Prime Directive #2). Only the fact they all need — this table — is shared.

Every one of those three distinctions is absent between translateFlow and mapNodeTree. translateFlow is 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, and mapFlowNodes delegates 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 assumed

The dispatch order predicted yes for branch (A) on the ground that it "adds a public export to packages/spec". Measured, it does not, and this is the one place this PR contradicts its order:

  • conversions/walk.ts reaches no exports subpath of the package. conversions/index.ts is the conversions layer's declared public surface and re-exports nothing from walk.ts; its only in-tree importers are conversions/registry.ts and its own test.
  • None of walk.ts's existing exports (mapFlowNodes, mapPages, mapPageComponents, mapViewPayloads, renameKey, renameConfigKey, …) appears in packages/spec/api-surface/*.json.
  • pnpm --filter @objectstack/spec run check:api-surface, run after a real pnpm --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 ✓ — and git diff -- packages/spec/api-surface is 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.

⚠️ One carrier this PR cannot satisfy, for the seat. The clause-② declaration limb is read from the card's claim comment, and the claim comment on #11745 carries no 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 read no on the card, the seat has to put it there.

Reference identity is load-bearing, and pinned

mapFlowNodeList returns the same nodes array reference when nothing under it resolved, and every container config and region nodes array on the way down is copied only when a descendant actually changed. Pinned by:

  • an untranslated nested screen and a non-screen node inside a region both come back ===;
  • a parallel branch under which nothing resolved keeps its own reference and its own nodes array;
  • translateFlow returns the same flow reference when nothing resolved at all;
  • a loop whose body is a string (config is an open record, and body is an ordinary key on an http node) is passed through by reference — the descent checks the shape, never the key name alone.

Scope held

⛔ translateFlow is still deliberately absent from translateMetadataDocument's dispatch table, TRANSLATABLE_METADATA_TYPES is 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.ts collects its universe with walkFlowNodes and says so in as many words ("Nodes are collected through walkFlowNodes, NOT flow.nodes directly … 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 own VERDICT command-exit line or from an exit code captured before any pipe.

run verdict
pnpm --filter @objectstack/spec test (project local) VERDICT command-exit 0 — 470 files passed, 1 skipped; 13291 tests passed
pnpm --filter @objectstack/spec exec tsc --noEmit VERDICT command-exit 0
pnpm --filter @objectstack/spec build VERDICT command-exit 0 — 34/34 declaration files present
pnpm lint (eslint . --no-inline-config, whole repo, not narrowed) VERDICT command-exit 0
gate families, node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack 82 derived, 78 run green, 4 NOT MEASURED

The 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-debt and @objectstack/lint check:doc-formula-expressions each 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 at 5c45aa02d8, 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:

injected 'flow.nodes.map((node) => {' : 1
removed  'mapFlowNodeList('           : 0
mutated blob: 1da02c8d1f4c873c3bf432271c395666faa60d64
 packages/spec/src/system/i18n-resolver.ts | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Observed, matching the prediction exactly — VERDICT command-exit 1, Tests 4 failed | 274 passed (278), and the four are precisely:

× overlays a screen in EVERY region slot — loop.body, parallel.branches[], try_catch.try and .catch
× reaches ARBITRARY depth — a screen inside a loop inside a try_catch
× overlays per-FIELD copy on a nested screen, not just its title
× preserves REFERENCE IDENTITY for everything that resolved nothing

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_PATH from a trap … EXIT INT TERM, and restoration proven by blob equality rather than by an exit code:

HEAD blob   : edd8dfb635880fab50d6dba427b8743e96afdae0
on-disk blob: edd8dfb635880fab50d6dba427b8743e96afdae0
git diff HEAD --stat : (empty)

No dist/ preflight applies: i18n-resolver.test.ts imports ./i18n-resolver as a relative source specifier inside the same package, so the ablation ran against the mutated source with no build in the resolution path.

验收备注

  • Filed as i18n-extract: walkScreenFlows walks flow.nodes flat — a screen inside an ADR-0031 region gets no skeleton entry and no coverage row #17511 — packages/cli/src/utils/i18n-extract.ts's walkScreenFlows walks flow.nodes flat too, so a nested screen gets no skeleton entry from os i18n extract and no coverage row. Measured in that file: FLOW_REGION_SLOTS 0, walkFlowNodes 0, try_catch 0, config.body 0, with walkScreenFlows 2 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: "translateFlow overlays 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.mjs reported 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

`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>
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation protocol:system tests tooling labels Sep 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

3 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
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 135 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 634f23de269cca67de5f114e0a07d4d5d33b457f → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 3978c9214ef76eef6d0f9fc435635359df8abb71 — the merge of head 5c45aa02d8e0014f00414b4871e51cf26d1e65cd into base 634f23de269cca67de5f114e0a07d4d5d33b457f, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# 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

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

os-bill commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Seat at-tier review — PASS, zero must-fix, zero advisories.

Head 5c45aa02d8e0014f00414b4871e51cf26d1e65cd, merge-base 76ddab772f14. Readings 2026-09-10T19:17–19:19Z.

Landing pre-checks

# check reading
① at-tier verdict PASS (this comment). Clause ② is no, measured — see below
② check-clause2-carriers --pair 17521 exit 0
③ check-governed-merges --test, final 4 paths exit 0; lit control AGENTS.md ⇒ exit 3
④ checks on this head 34 distinct names, 0 pending, 0 failing

The three claims, verified

The flat walk is gone and there is now ONE implementation, not two. mapFlowNodeList is exported from conversions/walk.ts, and mapFlowNodes delegates to it rather than keeping a parallel copy. translateFlow imports it and calls it; the only surviving flow.nodes.map( in the file is inside a docblock describing the old behaviour. That is the difference between fixing the fourth site and closing the class — which is what this card was filed about.

No wiring moved. The diff moves no TRANSLATABLE_METADATA_TYPES entry, no dispatch-table registration and no liveness row. translateFlow stays deliberately out of translateMetadataDocument's dispatch, where #11287's downstream card left it.

The pins target the card's two hard requirements directly, and one of them is a negative pin, which is the one people skip:

every region slot — loop.body, parallel.branches[], try_catch.try and .catch
ARBITRARY depth — a screen inside a loop inside a try_catch
per-FIELD copy on a nested screen, not just its title
the FLAT top-level screen still overlays — "the reach grew, it did not move"
preserves REFERENCE IDENTITY for everything that resolved nothing
returns the SAME flow reference when nothing resolved, regions included
leaves a region-shaped value that is NOT a region alone (`config` is an open record)

Reference identity was the card's explicit correctness condition — "a node that resolved nothing must come back as the same reference, or nodesChanged starts reporting untouched documents as changed" — and it is pinned at both granularities, node and flow.

On the fork, and on my order being wrong

The round took (A) and justified it on region-slots.ts's own stated rule rather than on effort: that file keeps region walks separate when they take different inputs, yield different units, or one formats a diagnostic trail — and none of the three separates translateFlow from mapNodeTree. So (B) would have made this resolver the fifth hand-rolled reader of the slot table. That is reasoning from the repo's declared rule, which is what the fork asked for.

⚠️ And it correctly declared Clause-②: no against my dispatch order, which asserted that fork (A) adds a public export and therefore fires clause ②. My order was wrong: conversions is not an exports subpath, the api-surface/export-origins diff is empty, and the pre-existing sibling mapFlowNodes is itself absent from api-surface (lit control: ConversionNotice reads 2 in api-surface/root.json, so the zero discriminates). An export keyword on a module no barrel re-exports is not a published export. Full correction is on card #11745.

⭐ Also worth naming: the round found that my Claim: comment was malformed — a bold **Claim:** first line, which left the declaration limb with no reading at all — and correctly refused to repair it itself, since a round may post no second claim and the gate forbids filling that line in on the claiming seat's behalf. Reported to the seat, fixed by the seat, --pair now exits 0. Recorded as a recurring template defect at #17520.

Out of scope, filed not smuggled

#17511 — packages/cli's i18n-extract walkScreenFlows walks flow.nodes flat too, so os i18n extract emits no skeleton entry for a screen inside a region. That is the same defect one layer over, in a different package, and it was filed rather than folded into this diff.

Enqueueing.


Generated by Claude Code

@os-bill
os-bill marked this pull request as ready for review September 10, 2026 19:18
@os-bill
os-bill added this pull request to the merge queue Sep 10, 2026
Merged via the queue into main with commit bbca441 Sep 10, 2026
36 checks passed
@os-bill
os-bill deleted the claude/issue-11745-translate-flow-regions branch September 10, 2026 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation protocol:system size/m tests tooling

Projects

None yet

1 participant