[malibio]

Closes #3451

Do not merge yet

The check fails on current main, as the issue says it will until its dependencies land. Two are still open, and they account for every difference it reports today:

• Make tool an abstract base with a tool-native subtype and drop source #3450 (tool as an abstract base with tool-native): the reference lists tool as abstract and tool-native as a core subtype with handler; the code has a concrete tool with handler and source.
• Seed built-ins from fixed-UUID tables with Markdown bodies, order agent guidance, and link skills to their schemas #3418 (skills linked to their schemas): the skill schema still declares node_types, which the reference replaced with the applies_to relationship.

Because the check runs in the merge gate, this PR's own gate run fails until both have landed, and merging it earlier would fail every other PR's gate too.

What it does

• packages/core/examples/dump_core_schemas.rs prints the CoreNodeType registry (id and kind) and get_core_schemas() as JSON.
• scripts/check-node-types-doc.ts (bun run node-types:check) reads node-types.md from the docs repository and compares:
  • the registry table: each concrete type, abstract base and core subtype is listed under its kind, and nothing else is;
  • one sheet per type, and each sheet's field table against the type's own schema: the same fields, each with its storage key (<type>.<field>), type, array element type, enum values in order, and extensible or closed (every enum row says one).
  • A row it can't read, a field table with no seeded schema behind it, and a seeded schema the registry doesn't list are reported, not skipped.
• Where the sheet is read from. test:changed's Rust tier and a plain run read the docs working tree, so a type and its sheet are developed together. The merge gate fetches the docs remote, resolves its main to a commit before it takes the machine slot, and the stage compares with that commit (--at <commit>). Edits in progress in the docs checkout, an unpushed commit, a stale checkout or a sheet pushed mid-run don't change the verdict.
• Finding the docs. From git's common directory (the primary checkout's sibling), so it resolves from .claude/worktrees/_gate; NODESPACE_DOCS_DIR overrides it.
• When it can't run. No docs checkout: the gate and test:changed print that the stage was skipped. A docs checkout that can't give the published sheet (no origin/main, not a git root, no reference on main): the gate exits as a machine fault, which ejects nobody. A failed fetch is printed and named in the stage label.
• changesGate in the merge queue counts the checker as a gate script, since it decides that exit.

Landing a type change

Push the sheet to the docs repository just before bun run merge. Until the PR lands, a merge round without it fails the check (a bisection retest included); the failure text tells a PR that changes no type to change nothing and re-queue. This is written into the node type sequence and CLAUDE.md. Making the verdict depend only on the stack (so such rounds warn instead of failing) is a queue design question left open.

Docs (on nodespace-docs main)

node-types.md rows the check couldn't read are rewritten (project priority values, one row per play suspension field, a tool-native field table, closed on two enums), and the reference, the node type sequence and Creating Node Types describe the check and the landing order.

Verification

• bun run test:scripts: fixture tests for the comparison (field gained, lost, renamed; type, element type, enum values and order, extensible; storage key; unreadable rows; type list), and tests against real temporary repositories for the docs location (primary checkout and a gate worktree) and the published read (uncommitted edit, unpushed commit, checkout behind its remote, failed fetch, each unreadable case).
• Against the real reference, working tree and published: 7 differences, all from the two issues above. Against a copy of the reference edited to describe today's tool and skill, it passes for all 24 types.
• bun run test:changed (every tier) passes with NODESPACE_DOCS_DIR pointed at that copy. Without the override it stops at node-types:check, as expected.
• Not run: the merge gate itself. Its preflight and stage wiring are pinned by tests but have not executed in a real merge round.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3494

Review Type: Initial Review (no prior reviews or comments on the PR; whole diff origin/main...5f060b53 reviewed)

Summary

The checker itself is solid. The parser reads every table in the real node-types.md correctly, unreadable rows are reported rather than skipped, and the fixture tests assert exact problem lists. The one thing I would not let into the merge queue as it stands is the gate wiring: the gate's verdict depends on the working tree of a second repository on whichever machine holds the queue lock, and a mismatch there is blamed on the queued PRs.

What I ran:

• bun test scripts/check-node-types-doc.test.ts scripts/gate-scope.test.ts: 53 pass.
• bun run quality:scripts:check, cargo clippy -p nodespace-core --example dump_core_schemas -- -D warnings, cargo fmt --check: clean.
• bun run scripts/check-code-references.ts: doc-path references 0 (baseline 0).
• bun run node-types:check against the real reference: exit 1 with 7 differences.
• Are the 7 differences real? Yes. I rewrote the dump to the documented design (tool abstract, tool-native registered with handler, no tool.source, no skill.node_types) and the real reference then produced 0 problems across all 24 types. None of the 7 is a parser artifact, and the PR description's explanation holds. Both dependency issues (Make tool an abstract base with a tool-native subtype and drop source #3450, Seed built-ins from fixed-UUID tables with Markdown bodies, order agent guidance, and link skills to their schemas #3418) are still open.
• Mutation sweep against the real reference: starting from that aligned dump, I applied 233 single-field code changes (rename, remove and retype every field; add, drop and reorder every enum's values; flip extensible; change every array's element type; add a field to every schema). 231 were reported. The 2 misses are finding 3.

Requirements Check

Acceptance criterion	Status	Notes
Fails when a core schema gains, loses or renames a field and node-types.md doesn't (fixture tests)	✅	Three tests assert the exact problem list with toEqual, plus the primitive-gains-a-field case. Confirmed on the real reference by the sweep above.
Passes on main plus docs once the dependencies have landed	❌ not yet	Exits 1 today with 7 differences, all from #3450 and #3418 (both open). It passes against a dump shaped like the documented design, so it should pass once they land. Until then this PR cannot merge.
Finds the docs repo from inside the merge gate's worktree (test)	✅	The test builds a real git worktree at .claude/worktrees/_gate and resolves from it and from a subdirectory. It would fail for a checkout-relative ../nodespace-docs.
Runs in the merge gate; a missing docs checkout skips with a warning	✅ with a caveat	Wired into test-gate.ts and test-changed.ts. The warning is written, but the gate never shows it (finding 2).
bun run test:scripts covers it; quality:fix clean	✅	26 tests; the non-mutating script and Rust lint checks are clean. I did not run quality:fix itself (it rewrites files).

Code Review Findings

🔴 Critical

None.

🟡 Important

1. The gate compares against the docs working tree on the lock-holder's machine, and a mismatch ejects the queued PRs
📁 scripts/check-node-types-doc.ts:425-451, scripts/gate-stage.ts:77-85, scripts/test-gate.ts:298-300

• What is wrong. The check does readFileSync on whatever is on disk in the sibling docs checkout: any branch, uncommitted edits, pulled or not. It exits 1 on a difference. gateVerdict in scripts/merge-queue.ts:380-384 maps exit 1 to failed, so the queue bisects the batch and ejects PRs. Every other gate input is the stacked tree under test; this one is mutable state outside it.
• Failure scenario A (uncommitted edits). A session is partway through a type change and has edited node-types.md, as the node type sequence requires ("docs, in the same change"). A merge round for unrelated PRs runs on that machine. The check fails, the queue bisects, and each unrelated PR is ejected with a failure comment.
• Failure scenario B (the ordering window). A type-change PR X can only pass if the docs already describe the new shape. If the docs commit is pushed first, every round that does not contain X fails on any machine that pulled. If it is not pushed, X passes only when the author's machine runs the round; "whichever bun run merge takes the lock" may be a teammate's, where X fails and is ejected. The PR description notes this for today ("merging it earlier would fail every other PR's gate too"), but the same window reopens on every future node-type change.
• Failure scenario C (stale checkout). A checkout behind its remote passes a PR that disagrees with current docs. The PR's Note acknowledges this.
• Recommendation. Decide this before the check gates merges. Options, cheapest first:
  • (a) In merge mode, read a committed ref instead of the working tree (git show origin/main:components/node-types.md after a fetch); keep working-tree reads for test:changed and manual runs. This removes A and C.
  • (b) When the check fails and the stack touches none of the check's inputs (packages/core/src/models/, packages/nodespace-types/src/core_type.rs, the example, the script), exit GATE_INFRA_EXIT so the queue backs off and ejects nobody, as changesGate already does for its own case. This removes the collateral ejections in B.
  • (c) At minimum, print the docs commit and dirty state in the failure message, and write the landing order (docs pushed and pulled, then bun run merge) into the node type sequence.
• Principle. Gate determinism. gate-stage.ts:28-34 already states the rule: "The merge queue must never blame a PR for its machine."

2. A skipped check shows as a green tick in the gate
📁 scripts/check-node-types-doc.ts:431-434, scripts/gate-stage.ts:231-234

• What is wrong. runStage sends stage output to a log file and prints only ✓ <label> Ns on exit 0. The skip path exits 0, so the warning lands in a log nobody opens.
• Failure scenario. A machine whose docs checkout is absent, or cloned under another directory name, takes the queue lock. It reports ✓ node-types:check for every round while checking nothing. Whether the check runs at all depends on which machine holds the lock, and nothing visible says so.
• Recommendation. Keep the skip (the issue asks for it) but make it visible: have the merge gate resolve the docs location itself and print one line on its own output when it will skip, or give StageSpec a way to surface a line from a passing stage. Printing the compared docs commit on success would also cover finding 1(c).
• Principle. A check that can pass without running is indistinguishable from one that passed; fail loud or skip loud.

🟢 Suggestions

3. extensible / closed is compared only when the sheet says it
📁 scripts/check-node-types-doc.ts:268, 371-375

• The two misses in the sweep: flipping extensible on query.generated_by or play.suspended_reason passes, because their rows say neither word.
• Recommendation. Report an enum row that says neither, and add closed to those two rows of the reference. Then the comparison is unconditional.
• Principle. A comparison that is optional per row is one a new row can opt out of by accident.

4. A type with no schema in the dump is skipped whatever its sheet says, and a schema with no registry entry is never looked at
📁 scripts/check-node-types-doc.ts:327-335

• Verified on the real reference:
  • dropping the person schema from the dump gives 0 problems although the sheet lists three fields;
  • a field table added under ### \schema`` gives 0 problems;
  • an extra seeded schema with no CoreNodeType gives 0 problems.
• The first and third are backstopped today by the Rust test the_seeded_schemas_are_exactly_the_registry_minus_the_meta_type (packages/core/src/models/core_schemas.rs:1911), which is why this is not Important.
• Recommendation. When schema === undefined, report any sheet.fields; and report dump schemas whose id is not in dump.types. Two short loops, and the script no longer leans on a test elsewhere. A type whose kind is none of the three KIND_ROWS keys is skipped by the registry comparison in the same way.

5. Nit: the dump carries data nothing reads
📁 packages/core/examples/dump_core_schemas.rs:20-23, scripts/check-node-types-doc.ts:39-43

• category is printed and never typed or read; parent is typed and never read. Either drop them (YAGNI) or use them: category against the section a sheet sits in (§3 primitive, §4 flat, §5 structured) would be a cheap extra comparison.

6. Nit: in test:changed, the first cold compile of nodespace-core now happens inside this stage's 20-minute budget
📁 scripts/test-changed.ts:69-73

• types:check builds only nodespace-types, so in a fresh worktree this stage is the first to compile nodespace-core. The Rust tier just below is raised to 60 minutes for exactly that cold compile. I did not measure a cold build, so this may never bite; passing the same override here would make the two consistent.

Things I checked that are fine

• Parser against the real reference: escaped pipes (the table row), multi-field rows with <type>.*, the three enum-value locations, the Values / default header, the content envelope rows, the parenthetical in the registry row, and the table-less ai-chat-acp lead.
• Structural edits to the reference fail loudly: a dropped bold lead, a renamed Storage header, a short row, a duplicate row and a fenced example table each produce a problem.
• CLAUDE.md compliance: TypeScript tooling, DOM-free bun:test tests, no lint suppression, no issue numbers or docs-path citations in comments, CLAUDE.md updated in the same change.
• Scope classification: the script reaches the Rust tier; the Rust inputs are already under RUST_DIRS; package.json forces every tier.
• Security and performance: nothing to report. The stage costs under a second on a warm build.

Recommendation

COMMENT

• The implementation is good and I found no defect in the parser or the comparison that needs a change.
• It is not mergeable today regardless: acceptance criterion 2 waits on Make tool an abstract base with a tool-native subtype and drop source #3450 and Seed built-ins from fixed-UUID tables with Markdown bodies, order agent guidance, and link skills to their schemas #3418, and landing earlier would fail every gate.
• Before it does land, finding 1 needs a decision (fix, or an explicit acceptance of the trade-off with the landing order written down), and finding 2 should be fixed with it. Findings 3 and 4 are small hardening changes worth folding in.

[malibio]

Automated pragmatic-code-review: COMMENT. Full findings in the review comment above.

[malibio]

Address review (commit 228f132)

Addressed (6 of 6 findings; one part of finding 1 not done)

1. 🟡 Gate read the docs working tree. The merge gate now runs node-types:check --published: it fetches the docs remote and reads origin/main:components/node-types.md, so uncommitted edits, an unpushed commit or a stale checkout on the gate machine no longer affect the verdict (scenarios A and C). If the fetch fails it reads main as last fetched and says so. test:changed and a plain run still read the working tree. The output names the docs commit compared. Tests use real remotes and clones: uncommitted edit, unpushed commit, checkout behind its remote, failed fetch.
2. 🟡 A skip showed as a pass. The gate and test:changed ask stageSkipReason() first and print ⚠ … SKIPPED: no docs checkout at … on their own output instead of running the stage. A docs directory that exists but has no reference is now an error, not a skip.
3. 🟢 extensible / closed optional per row. An enum row that says neither is reported. The two rows in the reference (query.generated_by, play.suspended_reason) now say closed.
4. 🟢 No-schema and no-registry cases. A field table on the sheet of a type with no seeded schema, and a seeded schema the registry doesn't list, are both reported, with tests.
5. 🟢 Unused dump data. category and parent are removed from the dump and its type.
6. 🟢 Cold-compile timeout. test:changed gives the stage 60 minutes, like the Rust tier.

Also: the gate now imports the checker, so gate-scope.ts lists it as gate machinery (a change to it runs every tier).

Not done: finding 1, option (b) (exit as a machine fault when the stack touches no type). A stage failure always exits the gate with 1 today, so this needs a new path through the gate's run() and a decision on what the queue does while main and the published sheet disagree (back off and retry until someone fixes one side). That is a change to the queue's behaviour, not to this check. What remains is scenario B: between pushing a sheet and merging its PR, a merge round that doesn't contain the PR fails the check. The landing order is written into the node type sequence, the reference and CLAUDE.md (push the sheet just before bun run merge), and an ejected PR is re-queued with bun run merge. This one is the maintainer's call.

Docs (nodespace-docs c958d8d): the two enum rows, how the gate reads the sheet, and the landing order.

Tests: bun run test:changed passes on every tier with NODESPACE_DOCS_DIR pointed at a copy of the reference that describes today's code. node-types:check --published against the real docs remote reports the same 7 differences as before (all #3450 and #3418).

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: The fix for finding 1 adds a second read path (fetch, read a ref) and changes how the gate and test:changed invoke and skip the stage. That is new gate logic, not a mechanical edit.

[malibio]

Pragmatic Code Review: PR #3494

Review Type: Re-Review (commit 228f132f, diff 5f060b53..228f132f, plus nodespace-docs c958d8d)

Previous Review Summary

	Count
Previous recommendations	6
Addressed	5 in full (findings 2, 3, 4, 5, 6), 1 in part (finding 1: scenarios A and C)
Not addressed	finding 1 option (b), declined by the author; judged in finding 1 below
New issues	3 (1 Important, 2 Suggestions)

The response's claims hold in the code. What I ran:

• bun test scripts/check-node-types-doc.test.ts scripts/gate-scope.test.ts: 62 pass. bun run quality:scripts:check: clean. rustfmt --check on the example: clean.
• bun run node-types:check and --published: both exit 1 with the same 7 differences, all from Make tool an abstract base with a tool-native subtype and drop source #3450 and Seed built-ins from fixed-UUID tables with Markdown bodies, order agent guidance, and link skills to their schemas #3418 (both still open). The published run fetched and read a newer docs commit than the local checkout's HEAD, so the fetch does what it says.
• Mutation sweep on a scratch copy of the checker (12 mutations, each run against the test file): all 12 fail at least one test. That covers the three stricter rules (enum row saying neither word, field table with no seeded schema, schema with no registry entry), the extensible mismatch, the new enum, closed: prefix, reading HEAD or the working tree instead of origin/main, dropping the fetch, dropping the "as last fetched" note, turning the fetch failure into an error, and both skip/error boundaries.
• readDoc against real clones (temporary repositories): results are in finding 2.

Verified as fixed:

• Finding 1, scenarios A and C. The gate stage is node-types:check --published; uncommitted edits, an unpushed commit, a checkout on another branch, a shallow clone and a checkout behind its remote all read the remote's main.
• Finding 2. test-gate.ts:301-303 and test-changed.ts:73-75 print ⚠ … SKIPPED on the gate's own stderr, which merge-pr.ts passes through. Once stageSkipReason() returns null the stage cannot skip: a docs directory with no reference is an error.
• Findings 3 and 4. Implemented as described; the two reference rows say closed.
• Findings 5 and 6. category and parent are gone from the dump and its type; test:changed gives the stage 60 minutes.
• Gate machinery listing. GATE_FILES (gate-scope.ts) and changesGate (merge-queue.ts:387) answer different questions and already differ: test-changed.ts and merge-pr.ts are in the first and not the second. changesGate only matters when the gate exits 3 or is killed, and the checker cannot produce either: a broken import or a failing stage exits 1 and is bisected to the PR. Nothing to change.

Requirements Check

Acceptance criterion	Status	Notes
Fails when a core schema gains, loses or renames a field and node-types.md doesn't (fixture tests)	✅	Unchanged tests, exact problem lists.
Passes on main plus docs once the dependencies have landed	❌ not yet	7 differences, all from #3450 and #3418, both open. The PR cannot merge until they land.
Finds the docs repo from inside the merge gate's worktree (test)	✅	Unchanged.
Runs in the merge gate; a missing docs checkout skips with a warning	✅	The warning is now on the gate's output.
bun run test:scripts covers it; quality:fix clean	✅	35 tests in the checker's file. Script lint and typecheck clean; I did not run quality:fix itself (it rewrites files).

Code Review Findings

🔴 Critical

None.

🟡 Important

1. The declined part: the landing order as written does not cover the cases that eject a PR, and the check breaks the queue's bisection
📁 scripts/check-node-types-doc.ts:462-463, 496-502; scripts/test-gate.ts:301-302; scripts/merge-queue.ts:110-112; docs development/node-type-sequence.md:39, components/node-types.md:8; CLAUDE.md:127

Verdict on declining option (b): acceptable, and (b) as I wrote it should not be built. An infra exit ends the round with nobody blamed (merge-pr.ts:553-556), and the next round restacks the same queue. When a batch fails for a real reason and its retested prefix exits as infra, that repeats without progress until the waiters time out. The author is right that this is queue design, not a change to this check.

What is not acceptable is leaving the remainder as "push the sheet just before bun run merge". Following that order exactly still ejects innocent PRs in three cases the docs don't mention:

• A round already running. The stage runs after the Rust test build and the daemon build, and fetches then. A sheet pushed while a round is in flight makes that round fail, although it was stacked before the push.
• Bisection. bisectBatch retests a prefix. Queue [A, X, C], where X is the type change with its sheet published and C has a failing test: the batch fails on C, the retest of [A] fails the node-types check because X is not in it, and A is ejected. A touched nothing. Any failing batch with a PR queued ahead of the type change does this.
• X itself is ejected (a conflict, its own test failure) or its bun run merge is stopped. The published sheet stays ahead of main, so every later round fails and ejects one PR per bisection until the sheet is reverted or X is back in the queue. The docs give the author no instruction for this.

In each case the ejected PR's comment ends with "Fix whichever side is wrong". A session acting on that for a PR that changed no type has two moves, both harmful: edit the published sheet back (which then fails X) or change the code.

No wrong code lands in any of these: the cost is ejections, wasted gate runs and a misleading instruction. That is why this is Important and not Critical.

Recommendation, for this PR (no queue change):

• (a) Add one sentence to the --published failure text: a PR that changes no core type did not cause the mismatch, the published sheet is ahead of main for a type change that is landing, change neither side and re-run bun run merge.
• (b) Complete the landing order in the three places it is written: a round already running can fail, and an author whose PR is ejected or whose merge is stopped re-queues at once or reverts the sheet push.
• (c) Resolve the published commit when the gate starts, not when the stage runs (see finding 2). A round stacked before the push then compares with the sheet as it was, which closes the first case.

For the maintainer to decide separately: making the verdict a function of the stack. One way that fits an existing pattern is to commit the dump as a generated file with a drift check, as types:check does for the TypeScript. "Does this stack change a core type" is then a git diff on one file, and a mismatch on a stack that doesn't is a printed warning. That keeps bisection sound and needs no new queue state.

Principle. A gate stage's result should be attributable to the stack under test; the queue's bisection assumes it.

2. A published sheet that cannot be read is reported as the PR's failure, and a degraded read passes silently
📁 scripts/check-node-types-doc.ts:453-470, 480-483; scripts/test-gate.ts:301-303

readDoc returns error for every unreadable case, the script exits 1, and gateVerdict maps 1 to failed. Measured against temporary clones:

Docs checkout on the gate machine	Result
Remote not named origin	fatal: invalid object name 'origin/main', exit 1
Remote's default branch is not main	same, exit 1
Single-branch clone of another branch	same, exit 1
NODESPACE_DOCS_DIR naming a plain copy (the setup the PR description used for test:changed)	not a git repository, exit 1
A plain copy inside another docs clone	reads the enclosing clone's origin/main and names the copy as the source
Fetch fails, remote has moved	reads the old sheet, notes it in the stage log only
Shallow clone, or checkout on another branch	correct

• Failure scenario. A machine with any of the first four takes the queue lock. Every round fails at this stage and the queue bisects and ejects PRs for a property of that machine's docs clone. This is the case gate-stage.ts:28-34 reserves GATE_INFRA_EXIT for; test-gate.ts:247-251 already does it for a missing nextest.
• Second scenario. git fetch has no bound of its own. A stalled or prompting fetch runs to the stage's 20-minute timeout holding the machine slot, and a timed-out stage also exits 1.
• Third scenario. When the fetch fails, the round is judged against the last-fetched sheet and the gate prints ✓. If the author has just pushed the sheet, their PR is ejected for the machine's network; the note is in the tail then, but a passing round shows nothing.
• No false pass found. Every unreadable case fails, and a stale read can only compare against an older published sheet.
• Recommendation. Do the published read's setup in the gate process, where the skip decision already is: fetch with GIT_TERMINAL_PROMPT=0 and a short bound, git rev-parse --verify origin/main^{commit}, and check git rev-parse --show-toplevel equals the docs directory. Unreadable: process.exit(GATE_INFRA_EXIT) beside the nextest check. Fetch failed: one ⚠ line on the gate's output. Pass the resolved SHA to the stage and read git show <sha>:components/node-types.md, which also makes the commit printed the commit read (today they are two separate reads of the ref).
• Principle. "The merge queue must never blame a PR for its machine."

🟢 Suggestions

3. An enum value named closed or extensible counts as the keyword
📁 scripts/check-node-types-doc.ts:274

• The keyword test runs over the whole Type cell, backticked values included. Measured: a row enum open, closed`` passes against a closed schema although it says neither word, and against an extensible schema it reports "closed in the sheet".
• No shipped enum has such a value today, so nothing is wrong on the real reference.
• Recommendation. Strip backticked spans from the cell before testing for the two words, and add the fixture.

4. Nothing pins which form each runner uses, or the skip line
📁 scripts/test-gate.ts:301-303, scripts/test-changed.ts:73-75

• Switching the merge gate back to TIERS.nodeTypesCheck (the working-tree read), or dropping the SKIPPED line, passes every test. Those two lines are the fixes for findings 1 and 2.
• scripts/gen-types.test.ts:247-254 already pins types:check this way: the stage's command, and that both runners contain run(TIERS.typesCheck).
• Recommendation. The same three assertions for this stage: the published stage's command ends --published, test-gate.ts runs it, test-changed.ts runs the working-tree one.

Recommendation

COMMENT

• The new commit does what the response says, and its tests hold under mutation. I found no defect in the stricter rules or the skip handling.
• It still cannot merge: acceptance criterion 2 waits on Make tool an abstract base with a tool-native subtype and drop source #3450 and Seed built-ins from fixed-UUID tables with Markdown bodies, order agent guidance, and link skills to their schemas #3418.
• Declining option (b) is the right call. Before this lands, fold in finding 1 (a) and (b), which are a sentence of output and a paragraph of docs, and finding 2, which is one preflight and gives 1 (c) for free. The structural change in finding 1 is the maintainer's decision and need not hold this PR.

[malibio]

Automated pragmatic-code-review (re-review): COMMENT. Full findings in the review comment above.

[malibio]

Address re-review (commit 3439b3c)

Addressed: all 4 findings

1. 🟡 The landing order didn't cover the cases that eject a PR.
   • (a) In the gate, the failure text now ends: a merge that changes no core type did not cause the difference; change neither side and re-run bun run merge once the type change has merged.
   • (b) The node type sequence, the reference and CLAUDE.md now say that a round without the PR fails (a bisection retest included), and that an author whose PR is ejected or whose merge is stopped re-queues at once or reverts the sheet push (nodespace-docs 24702f7).
   • (c) The gate resolves the published commit once, before it takes the machine slot, and the stage reads that commit (node-types:check --at <commit>). A sheet pushed while a round runs no longer changes what that round is compared with. Test: a commit resolved earlier is still what is read after a later push.
2. 🟡 An unreadable published sheet was the PR's failure. resolvePublished runs in the gate process, beside the nextest check. No origin/main, a directory that isn't the root of a git checkout, a main with no reference, or a missing NODESPACE_DOCS_DIR all exit with GATE_INFRA_EXIT. The fetch runs with GIT_TERMINAL_PROMPT=0 and a 60-second bound. A failed fetch prints one ⚠ line on the gate's output. The commit printed is the commit read. Tests cover each unreadable case with real clones.
3. 🟢 An enum value named closed or extensible. The two words are read outside backticks; fixture added.
4. 🟢 Nothing pinned which form each runner uses. Tests assert the stage commands, that test-gate.ts runs nodeTypesCheckAt(publishedSheet.commit) and not the working-tree stage, that test-changed.ts runs the working-tree stage, that both print SKIPPED:, and that the gate exits GATE_INFRA_EXIT on an unreadable checkout.

Left for the maintainer (the reviewer agreed it need not hold this PR): making the verdict a function of the stack, for example by committing the dump as a generated file with a drift check, so that a mismatch on a stack that changes no type is a warning.

Tests: bun run test:changed passes on every tier (with NODESPACE_DOCS_DIR at a copy of the reference describing today's code); scripts tier 1296 pass. node-types:check --published against the real docs remote: the same 7 differences, all from #3450 and #3418.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: The gate has a new preflight with a machine-fault exit, and the stage's read path changed from a ref to a commit passed in by the gate. Both are gate logic.

[malibio]

Pragmatic Code Review: PR #3494

Review Type: Re-Review (commit 3439b3cb, diff 228f132f..3439b3cb, plus the landing-order text in nodespace-docs)

Previous Review Summary

	Count
Previous recommendations (second review)	4 (2 Important, 2 Suggestions)
Addressed	4
Not addressed	0
New issues	3 (1 Important, 2 Suggestions)

The response's claims hold in the code. What I ran:

• bun test scripts/check-node-types-doc.test.ts scripts/gate-scope.test.ts: 68 pass. bun run quality:scripts:check: exit 0.
• bun run node-types:check, --published and --at 8e133ab: each exits 1 with the same 7 differences, all from Make tool an abstract base with a tool-native subtype and drop source #3450 and Seed built-ins from fixed-UUID tables with Markdown bodies, order agent guidance, and link skills to their schemas #3418. No row of the real reference is newly reported, so the backtick-stripping does not misread any shipped extensible / closed cell.
• Mutation sweep on a scratch copy of scripts/ (18 mutations, each run against the checker's test file): 14 killed, 4 survived. Killed: the backtick-stripping, both halves of the root-of-checkout test, the cat-file check, the fetch, fetchFailed, the ref resolved, reading the passed commit, the explicit-directory case, the missing origin/main case, the gate's GATE_INFRA_EXIT, the gate's stage call, the stage command, and the SKIPPED line. Of the survivors, one is an equivalent mutation and three are in finding 2 and the notes below it.
• Bun.spawnSync with timeout (Bun 1.4.2, scratch probe): a child is killed at the bound and the call returns then, even when a grandchild still holds the pipes. The result has exitCode: null, so docsGit reports ok: false and a cut-off fetch takes the "fetch failed" path. The bound is real for git, which does not ignore SIGTERM.

Verified as fixed:

• Finding 1 (a): the hint is printed for --at and --published runs, at the end of the stage output, so it lands in an ejected PR's comment.
• Finding 1 (b): development/node-type-sequence.md:39-41, components/node-types.md:8 and CLAUDE.md:127 cover a round without the PR, a bisection retest, an ejected or stopped PR, and a PR that changes no type. They match what the code does: each gate run (a retest included) resolves the commit again.
• Finding 1 (c) and finding 2: the preflight sits after the if (!merge) exit(0) return and before acquireGateLock, so a push never runs it and it never holds the machine slot. Every unreadable branch reaches process.exit(GATE_INFRA_EXIT). I found no path where a property of the docs checkout exits the gate with 1, and no silent pass in the gate itself.
• Exit 3 and the queue: gateVerdict maps it to infra, the round ends with nobody ejected, and the waiters back off. When the stack changes test-gate.ts or gate-*.ts it maps to failed, as for the disk and nextest preflights. That applies to this PR too, since it changes both files: a lock holder with an unreadable docs checkout would eject it. This is the existing rule, not a defect.
• Findings 3 and 4: implemented as described.

Requirements Check

Acceptance criterion	Status	Notes
Fails when a core schema gains, loses or renames a field and node-types.md doesn't (fixture tests)	✅	Unchanged.
Passes on main plus docs once the dependencies have landed	❌ not yet	7 differences, all from #3450 and #3418, both open. The PR must not be queued before they land.
Finds the docs repo from inside the merge gate's worktree (test)	✅	Unchanged.
Runs in the merge gate; a missing docs checkout skips with a warning	✅	The warning is printed by the gate's preflight.
bun run test:scripts covers it; quality:fix clean	✅	41 tests in the checker's file. Script lint and typecheck clean; I did not run quality:fix itself (it rewrites files).

Code Review Findings

🔴 Critical

None.

🟡 Important

1. The checker now decides a machine-fault exit, and the queue does not count it as a gate script
📁 scripts/merge-queue.ts:386-389 (changesGate), scripts/test-gate.ts:267-271, scripts/check-node-types-doc.ts:480-499

• What is wrong. gateVerdict treats exit 3 as the stack's failure when the stack changes the scripts that decide it; its comment gives the reason: "otherwise a PR that breaks the gate would stall the queue forever without being bisected out". changesGate matches scripts/(test-gate|gate-*).ts only. Before this commit the checker could not produce exit 3 (the second review relied on that). Now resolvePublished, skipReason and resolveDocsDir in check-node-types-doc.ts decide the gate's exit 3. Measured: gateVerdict(3, changesGate(["scripts/check-node-types-doc.ts"])) is infra.
• Failure scenario. A later PR edits the checker and breaks the published read on every machine: DOC_FILE renamed ahead of the docs move, PUBLISHED_REF changed, or a bug in the root-of-checkout test. Every round containing it exits 3 in the preflight. Nobody is ejected, the PR stays queued, and each following round restacks it and exits 3 again. Every queued PR waits until its bun run merge gives up after three hours. The message names the docs checkout, so the reader looks at their machine and not at the PR.
• Recommendation. Add check-node-types-doc to the pattern in changesGate and one line to its test in scripts/merge-queue.test.ts. Likelihood is low and the fix is one line; no further review needed for it.
• Principle. The invariant gateVerdict already states: a script that decides the infra exit is the stack's to own.

🟢 Suggestions

2. The stage can still exit 0 without comparing, and nothing tests that --at is used
📁 scripts/check-node-types-doc.ts:522-554

• Measured: node-types:check --at with no value compares with the working tree. --at "" runs git show :components/node-types.md, which is the docs index, and prints "at in". With --at <sha>, a docs directory that has gone since the preflight takes the skip branch and exits 0, which the gate prints as ✓.
• None of these is reachable from the gate as written: it always passes a full sha, and the directory would have to vanish mid-run. But the mutation that makes the main block ignore --at passes every test, and it turns the stage back into a working-tree read, the thing findings 1 and 2 of the first two reviews removed.
• Recommendation. Move the argument handling into an exported function that returns the commit or an error (--at without a 7 to 40 character hex value is an error), treat skip as an error when a commit was given, and test it.

3. A failed fetch is not visible where a PR's ejection is explained
📁 scripts/test-gate.ts:273-275, 325

• The ⚠ line is printed before the machine slot is taken. The stage is told only the commit, so its failure text does not say the sheet may be stale, and the ejection comment is the last 40 lines of gate output.
• Failure scenario. The author pushes the sheet and queues the PR. The lock holder's docs fetch fails (expired credentials now fail at once, with prompts off). The PR is compared with the old sheet and ejected, with a comment that reads as a real disagreement.
• Recommendation. Put it in the stage label when fetchFailed is set (for example "published node-types.md, as last fetched: the docs fetch failed"). run() repeats the label in its last line, so it reaches the comment.

Notes, no action needed:

• The other two surviving mutations (removing timeout and GIT_TERMINAL_PROMPT, and removing the gate's fetch-failed warning) have no test. Both are hard to test without a stalling remote, and I would not add one.
• Nit: CLAUDE.md:127 says to re-queue an innocent PR "unchanged"; the sequence and the failure text add "once the type change has landed". Re-queued earlier, it fails again unless the type change is in the same batch.

Recommendation

COMMENT

• The commit addresses all four findings of the second review, and its tests hold under mutation. The preflight is in the right place and maps every docs-checkout problem to the machine-fault exit.
• Fold in finding 1 (one line and one test line). Findings 2 and 3 are optional hardening. None of them needs another review round.
• The PR still cannot merge: acceptance criterion 2 waits on Make tool an abstract base with a tool-native subtype and drop source #3450 and Seed built-ins from fixed-UUID tables with Markdown bodies, order agent guidance, and link skills to their schemas #3418.

[malibio]

Automated pragmatic-code-review (re-review 2): COMMENT. Full findings in the review comment above.

[malibio]

Address re-review 2

Addressed: all 3 findings and the nit

1. 🟡 The queue didn't count the checker as a gate script. changesGate now matches scripts/check-node-types-doc.ts, so a stack that changes the checker owns a machine-fault exit the checker decides, and is bisected out instead of stalling the queue. Test line added (the checker's own test file is not a gate script).
2. 🟢 The stage could exit 0 without comparing. Argument handling is parseTarget, exported and tested: --at without a 7 to 40 character hex commit is an error, never a read of another tree. With a commit given, a missing docs checkout is an error, not a skip. A test pins that the script reads its arguments through parseTarget.
3. 🟢 A failed fetch wasn't visible in an ejection comment. nodeTypesCheckAt(commit, fetchFailed) puts "the docs fetch failed: compared as last fetched" in the stage label, which the gate repeats in its last line on failure.
4. Nit: CLAUDE.md now says to re-queue an innocent PR once the type change has landed, as the sequence and the failure text do.

Skipped: nothing. The two surviving mutations the reviewer listed as untestable without a stalling remote (the fetch bound and the gate's fetch-failed warning) are left without a test, as the review recommends.

Tests: bun run test:changed passes on every tier (NODESPACE_DOCS_DIR at a copy of the reference describing today's code); scripts tier 1299 pass.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: One regex alternative with its test, an argument parser with its tests, a label string and a sentence of prose. The reviewer stated that none of these needs another round.

Merge status

Not mergeable yet. The check reports 7 differences between the published node-types.md and main, all from #3450 (tool / tool-native) and #3418 (skill.node_types), which are both open. This PR's own gate run fails on them, so it is queued only after both have landed.