Repository navigation
fix(#5236): keep init and agent-skills working for an UNKNOWN runtime id - #5251
gremlinops wants to merge 12 commits into
Conversation
trek-e
left a comment
There was a problem hiding this comment.
Automated maintainer review of head 287777d8ba. Zero-tolerance policy: every finding at any severity must be resolved before approval.
Review - fix(#5236): keep init and agent-skills working for an UNKNOWN runtime id
Head 287777d, base next, branch fix/5236-init-unknown-runtime, 3 commits (test, fix, changeset backfill), not draft, Fixes #5236.
Summary
Root cause is correctly addressed: checkAgentsInstalled (src/agent-install-check.cts:195-220) now catches ONLY UnknownRuntimeError from getAgentsDir and returns agents_installed:null + reason 'unknown_runtime'; RetiredRuntimeError and other errors still propagate. buildAgentSkillsBlock (src/init.cts ~4240) resolves getGlobalSkillsBase only after the empty-paths early returns, catches only UnknownRuntimeError and skips global: entries with a warning. The path accessors and install refusal are unchanged. Not fail-open to a safe known: null is a distinct tri-state, not false/claude values; planning snapshot maps null to SCOPE.UNREADABLE so W010 stays silent. CI matrix not executed (fork, action_required) - orchestrator's call.
Acceptance checklist (issue #5236, "What did you expect" / proposed fix)
- checkAgentsInstalled does not throw for unknown id; reports not checkable (agents_installed:null + reason) - agent-install-check.cts:205-220, tests agent-install-check.test.cjs
- buildAgentSkillsBlock resolves skills base only when it has skill paths - init.cts, tests/agent-skills.test.cjs
- Refusal in install code unchanged - tests/install-runtime-artifacts.test.cjs pins refuse + writes nothing
- Every init command succeeds under unknown id (tests/init.test.cjs, 17 tests total; PR reports 11 fail on next)
- KNOWN runtimes unchanged (boolean, no reason key; property test)
Failing-first: two-commit TDD (test then fix) - verified by commit ordering; I did not execute tests (forbidden locally).
Findings
- Minor | judgment | CONTEXT.md:327 (Agent Install Check Module) - not updated. The result gains a third state (agents_installed:null, reason from frozen AGENTS_INSTALLED_REASON) plus new init/docs-init/validate-agents fields; PR body defers placement to maintainers ("for the maintainers to place in CONTEXT.md if wanted"). Glossary is a PR gate and no-deferral applies. Fix: add the null/reason state, AGENTS_INSTALLED_REASON enum (3-change lock convention) and agents_installed_reason/agents_found_reason to that entry.
- Minor | judgment | docs/adr/5057-* - PR states the caller-side catch is "an extension, not ADR text" (ADR-5057 (5) / Phase 10 amendment). Behavior contract extended without recording it. Fix: add a short amendment to ADR-5057 naming the reporting-path exception.
- Nit | judgment | gsd-core/workflows/new-project.md:46, new-milestone.md:314 - warn only on agents_installed === false; null silently skips (PR admits "not tested"). Defensible (cannot check), but untested model behavior; consider one sentence stating null means not checkable.
- Nit | judgment | .changeset/rapid-pumas-roam.md - body lacks the conventional trailing (#5236) backlink (optional per CLAUDE.md, not blocking by itself). Type Fixed, pr:5251 correct.
Test quality
Behavioral, no source-grep; fast-check property with pinned seed over registered/retired/near-miss/arbitrary ids with branch-coverage assertion (non-vacuous); reason enum key-lock test; control test for accessor refusal; GSD_AGENTS_DIR path covered. No wall-clock. Boundary (limit-1/limit/limit+1) not applicable (no numeric limit).
Security
Only UnknownRuntimeError caught (instanceof); retired/other errors rethrown. No new fs/argv/subprocess paths. agents_dir '' consumers: cmdAgentSkills (init.cts:4428) guards length>0, so no read from cwd-relative path. Warning embeds runtime id in stderr text only (already embedded in the pre-existing warning). No injection surface added.
Blast radius (codegraph)
checkAgentsInstalled: 5 callers (src/init.cts withProjectRoot + cmdAgentSkills, docs.cts, planning-snapshot.cts, verify.cts cmdValidateAgents); tested via tests/agent-install-check.test.cjs. Return type widened boolean -> boolean|null: only consumer branching is health-diagnostic-rules/agent-install.cts (UNREADABLE checked first). JSON output additive only for unknown ids. buildAgentSkillsBlock exported (init.cts:4866) behavior-only change. memtrace unavailable (quota); codegraph used.
Co-change gaps
CONTEXT.md glossary (Finding 1); ADR-5057 note (Finding 2). docs/how-to/control-the-reported-host-runtime.md updated. No references/ or workflows/ files added: INVENTORY not needed. No shipped workflow changed: no Emitted-Drift-Ack-Growth needed. Runtime-resolver surface parity: no new env var/resolver.
PR format
Branch fix/5236-init-unknown-runtime ok; conventional commits ok; Fixes #5236 ok; template followed; linked issue labeled confirmed-bug.
Behind-by / rebase
mergeStateStatus BEHIND, 1 commit behind origin/next (83aba44 -> ed0f3fd chore(#4036) CI timeout budget history #5253, touches only tests/ci-timeout-budget-history.jsonl). No overlap; rebases cleanly. Update-branch on a fork re-queues CI approval.
Verdict
Minor (no blockers/majors). Substance sound; zero-tolerance means Findings 1-4 block approval until addressed. Changes requested (minor).
Memtrace Evidence
Memtrace quota was exhausted for this review run, so the graph-backed checks were NOT executed; this is stated rather than reported as passed:
get_impact: not run (quota). Blast radius above comes from codegraph impact/callers where present.get_symbol_context: not run (quota). Callers/callees were read via codegraph where present.recall_decision: not run (quota). The recorded-decision gate was NOT evaluated; this is not a finding that no decision governs the change.find_code_review_issues: not run (quota). Deterministic AST/YAML/cross-module pass did not execute; every finding above is judgment-sourced unless marked deterministic.
287777d to
ef9b626
Compare
|
Addressed in 0b815f5, 42f5fe6 and ef9b626, rebased on next (ed0f3fd):
|
trek-e
left a comment
There was a problem hiding this comment.
Review of head ef9b62626324 — changes requested (2 Nits)
Summary. checkAgentsInstalled and buildAgentSkillsBlock now catch UnknownRuntimeError at those two caller sites only, so init and agent-skills keep working for an unknown runtime id while the accessors and install keep refusing.
All four prior findings are resolved on this head [verified on disk]:
CONTEXT.md:327(Agent Install Check Module) now records the could-not-run state:agents_installed: null, the frozenAGENTS_INSTALLED_REASONand its lock convention,agents_installed_reason/agents_found_reason, and theSCOPE.UNREADABLEmapping.- The ADR-5057 dated amendment states that the section-5 rule is unchanged, the accessors and install keep the refusal, and only two named callers catch
UnknownRuntimeError. It does not contradict the ADR's invariant. new-project.md:72andnew-milestone.md:328carry the null-branch sentence. Commit42f5fe60a2, the commit that grows both files, carries twoEmitted-Drift-Ack-Growthtrailers;git interpret-trailers --parsereturns both, each on a single line with a bare-filename key, matching ADR-3942 anddocs/how-to/acknowledge-emitted-drift.md..changeset/rapid-pumas-roam.mdis typeFixed,pr: 5251, bold lead, trailing(#5236).
Classification and gates. Fixes #5236, bug-fix track, conventional title, template followed, branch fix/5236-init-unknown-runtime. No references/ or workflows/ files added, so no INVENTORY change.
Root cause. The reporting read run on every init, and the skills base was resolved before the early returns, both propagating the accessors' refusal. The fix catches it only at those callers (instanceof UnknownRuntimeError; RetiredRuntimeError and everything else still propagate). The accessors and install are untouched. [verified]
Fail-safe. null is a distinct third state. planning-snapshot maps it to UNREADABLE (the same silent W010 as a scan throw), and the one other boolean consumer checks UNREADABLE first. [verified]
Security. An unknown id is refused before any path is built, so it never reaches a path. It is echoed only into the JSON agent_runtime and a stderr warning, the same exposure as the existing warning. No new subprocess, fs or argv path in the source diff.
Tests. Two-commit TDD ordering (test dce19abd7, then fix 7cc3fb1fe) establishes fail-first by order; I did not execute them (local node --test is not permitted). Behavioral, no source-grep, no wall-clock, fast-check with a pinned seed and a branch-count non-vacuity assertion, retired and known-runtime controls, install/uninstall refusal asserted.
Findings
Nit 1 — the null-branch prose has no test [judgment]
gsd-core/workflows/new-project.md:72 and new-milestone.md:328: the new agents_installed: null sentence is untested (the PR body says so). If a content-contract test exists for these workflows, add a row; otherwise state in the PR that none exists.
Nit 2 — "continue with Steps 6-7" on an unknown host [judgment, unverified behavior]
Same lines: null tells the model to continue into the steps that spawn named subagents (gsd-project-researcher and others). On an unknown host those agents may not exist. The previous behavior was exit 1, so this is a policy choice the PR makes. Please confirm it is intended.
CI. On this head the fork matrix has not executed: the rollup carries only policy checks (validate-target, template format, validate-title, issue link, GitGuardian, Socket), all SUCCESS, and nine runs are waiting for maintainer approval. That is not a code defect. A PR whose tests never ran is not approvable, so I am releasing the queued runs now and will check the result before any further verdict.
CodeGraph Evidence
Memtrace unavailable: monthly quota exhausted, so get_impact, get_symbol_context, recall_decision and find_code_review_issues were not run. The recorded-decision check was not evaluated. The index is of the main checkout, not this PR head, so it is a lower bound.
codegraph_impact checkAgentsInstalled: 35 symbols.src/agent-install-check.cts:195;buildAgentInstallFieldandbuildPlanningSnapshotinsrc/planning-snapshot.cts;cmdDocsInitinsrc/docs.cts;cmdValidateAgentsinsrc/verify.cts; and 29cmdInit*entry points insrc/init.cts(includingwithProjectRoot,cmdAgentSkills).codegraph_callers buildAgentSkillsBlock:cmdAgentSkills(src/init.cts:4379) andtests/runtime-homes-descriptor-drive.test.cjs.- Finding: the return type widens
booleantoboolean | null, which reaches everyinitcommand. Thenullmapping is therefore the load-bearing contract, and it is covered by the tests above.
Verdict
Changes requested on the two Nits only. Blocker 0, Major 0, Minor 0, Nit 2.
trek-e
left a comment
There was a problem hiding this comment.
Follow-up on head ef9b62626324 — changes requested (1 Major from the CI run, plus the 2 Nits above)
My earlier review on this head reported two Nits and no Blocker or Major. At that point the fork matrix had not executed, so that review could not see this. The runs have now been released and completed. This is a defect in the diff, not base drift.
Major 1 — the compact-content baseline was not refreshed [verified from the CI log]
test (ubuntu-latest, 24, shard 1/3) fails, and Required tests fails with it. The single failing test is tests/benchmark-compact-content.test.cjs:325 ("--check against the real, committed, non-drifted baseline exits 0"). It expects the output to match /up to date/ and got:
DRIFT: split "new-project": off 14286 -> 14326 (+40), on 12342 -> 12382 (+40), reduction 13.61% -> 13.57% (-0.04pp)
DRIFT: aggregate: off 110980 -> 111020, on 93501 -> 93541, reduction 15.75% -> 15.74%
Attribution: the drift is on the new-project split by exactly +40 tokens, and this PR edits gsd-core/workflows/new-project.md (the null-branch sentence). The PR changes new-project.md and new-milestone.md but does not touch tests/fixtures/compact-content-benchmark-baseline.json. next is clean on this test (its Tests run passed on the latest merge commit 5d987ccf6d), so this is not base drift.
Fix: run node scripts/benchmark-compact-content.cjs --write and commit the refreshed baseline in this PR, as a test(#5236) commit (a stale-test correction carries its own commit). Re-check that new-milestone shows no drift after the write.
Open from the earlier review (unchanged): Nit 1 (the null-branch prose has no test) and Nit 2 (confirm that "continue with Steps 6-7" is intended for an unknown host).
Branch state. BEHIND next by one commit (5d987ccf6d). Rebasing or merging next is a CI condition, not a finding, but the baseline refresh should be done on the refreshed base so the token counts are final.
CI at this head (live; head ef9b6262632438a39dc8056317a6e27c0602ed43): the only non-green rows are test (ubuntu-latest, 24, shard 1/3) = FAILURE and Required tests = FAILURE; the single failing test is the one above. Other shards and the remaining rows were not failing.
CodeGraph Evidence
Memtrace unavailable: monthly quota exhausted, so get_impact, get_symbol_context, recall_decision and find_code_review_issues were not run. The recorded-decision check was not evaluated. The index is of the main checkout, not this PR head.
codegraph_impact checkAgentsInstalled(from the earlier review on this head): 35 symbols, reaching 29cmdInit*entry points plusdocs,planning-snapshotandverify.codegraph_callers buildAgentSkillsBlock:cmdAgentSkills(src/init.cts:4379) andtests/runtime-homes-descriptor-drive.test.cjs.- Finding: the change itself is confined to those callers. The failing test is not a code-path defect but a stale generated baseline for the workflow files this PR grows.
Verdict
Changes requested. Blocker 0, Major 1, Minor 0, Nit 2.
trek-e
left a comment
There was a problem hiding this comment.
Review of head ef9b62626324 — changes requested
Summary. This is the full pass on the same head; my two earlier reviews (the first with two Nits, the follow-up with the baseline Major) are not edited. It keeps the baseline Major, raises the earlier "confirm it is intended" Nit to a Major after reading the workflow it affects, and adds the findings from the Skill and isolated passes. Blocker 0, Major 2, Minor 7, Nit 4.
Classification and gates. #5236 carries bug + confirmed-bug; bug-fix track (RULESET.CONTRIB.CLASSIFY.fix=requires confirmed-bug before implementation (legacy 'confirmed' label is back-compat only for duplicate-sweep exemption, not a valid implementation gate)). Fixes #5236, conventional title, not draft, no competing PR. Changeset .changeset/rapid-pumas-roam.md: type Fixed, pr: 5251, bold lead, (#5236); docs-exempt. Emitted-Drift-Ack-Growth trailers for the two workflow files are present on 42f5fe60a2 (single line, bare filename). CONTEXT.md and ADR-5057 updated.
Acceptance (issue #5236 Brief)
- "Each of init new-project, plan-phase, execute-phase, quick, progress exits 0 under an unregistered runtime id" — Met by mechanism (
withProjectRootconsumes the null result). - "agent-skills exits 0 with empty output when config.runtime is unregistered and no agent_skills is set" — Met.
- "agents_installed is null with a reason for an unregistered id; unchanged for registered ids" — Met (
unknown_runtime; known ids keep a boolean and no reason key). - "Consumers of the check result handle the not-checkable case without crashing" — Partial:
docs,verifyand the planning snapshot handle it; the two workflow consumers do not (Major 2). - "Installing under an unregistered id still refuses with UnknownRuntimeError" — Met (test-only, no install code changed).
- "Regression test covers all of the above" — Partial: no test covers the workflow null branch, and the stale baseline fails a test in CI (Major 1).
Findings
Major 1 — the compact-content baseline was not refreshed; CI is red [verified from the CI log, carried] tests/benchmark-compact-content.test.cjs:325 fails (DRIFT: split "new-project": off 14286 -> 14326 (+40) and the aggregate), failing test (ubuntu-latest, 24, shard 1/3) and Required tests. This PR grows gsd-core/workflows/new-project.md by one sentence and does not touch tests/fixtures/compact-content-benchmark-baseline.json; next is clean on this test. Fix: after rebasing, node scripts/benchmark-compact-content.cjs --write and commit it as a test(#5236) commit; re-check that new-milestone shows no drift.
Major 2 — on null the workflows send an unknown host into subagent spawns it may not have [verified by reading; promoted from the earlier Nit] gsd-core/workflows/new-project.md:72: "If agents_installed is null: GSD could not check agents for this runtime (agents_installed_reason). Do not show the warning above; continue with Steps 6–7." and new-milestone.md:328: "continue with the parallel research spawn step". Steps 6-8 spawn named subagents (gsd-project-researcher, gsd-research-synthesizer at :756, gsd-roadmapper at :1010/:1098); the only host that reaches null is one whose runtime id GSD does not know, exactly the class most likely to lack those agent types, and null means "could not check", not "present". If the spawn is rejected, new-project.md:766 says to "surface the error and stop" after researcher spend. The false branch (:63-68) already documents the "agent type not found" failure and the inline fallback; null gets neither a fallback nor an instruction to try-then-degrade. Fix: treat null as unverified — attempt the first spawn and take the inline-roadmap path on an agent-type-not-found rejection (or go inline outright), in both files, and cover the prose with a content-contract test.
Minor 1 — the parse lists and a sibling branch were not reconciled [verified] new-project.md:42 and new-milestone.md:312 list the fields to parse but omit agents_installed_reason, which the new paragraph tells the agent to consult; new-project.md:42 also lists required_agents, required_agents_installed, missing_required_agents, agent_skill_payloads_available, agent_skill_payload_agents, none of which init new-project emits (adversarial probe), so the required_agents_installed is false condition at :64 is dead and the PR adds a sibling branch to the same block without reconciling it.
Minor 2 — a behavior change is undisclosed [adversarial pass, verified by reading src/init.cts ~4227-4251; not probed by me] getGlobalSkillsBase(runtime) moved behind the !config.agent_skills / skillPaths.length === 0 early returns, so a retired runtime id is now refused by agent-skills only when skill paths exist; before, it surfaced even with none configured. The ADR amendment says RetiredRuntimeError "still propagates", true only conditionally; neither the changeset nor a test says so.
Minor 3 — narrowing by class where a code exists [verified from the test text; Standards and adversarial passes agree] src/agent-install-check.cts (~208) and src/init.cts (~4246) use instanceof UnknownRuntimeError, and the class already carries code = 'GSD_UNKNOWN_RUNTIME' (src/runtime-name-policy.cts:93). The tests contradict the implementation: tests/init.test.cjs matches the message ("The CLI gives this error no typed code, so match the class's own message") and tests/install-runtime-artifacts.test.cjs falls back to name plus message because a second copy of the class can exist. If a duplicate copy ever throws, the catch rethrows and init exits 1 again with no failing test on the gsd-tools path. Narrow on code.
Minor 4 — message-text assertions and process cost [verified from the diff] The "no init workflow refuses…" test passes when the refusal string is merely absent, so it passes vacuously if the command fails another way; assert exit 0 and parsed JSON per subcommand. It also spawns INIT_SUBCOMMANDS.length × 2 CLI processes.
Minor 5 — the property test restates the implementation [judgment] tests/agent-install-check.test.cjs "property:" uses assertKnownRuntime as its oracle and pins seed: 1729, so it cannot fail independently; the seen branch counters are good. No limit-1/limit/limit+1 rows (empty versus many skillPaths for buildAgentSkillsBlock).
Minor 6 — three emitters hand-copy the conditional spread [verified] src/init.cts:381, src/docs.cts:288 and src/verify.cts (agents_found_reason) each forward the reason by hand; no parity test ties them together (Generative Fix Divergence). src/health-diagnostic-rules/agent-install.cts:57 still tests truthiness and is safe only because the UNREADABLE guard comes first; the buildAgentInstallField catch fallback still sets agents_installed: false, so a throw and a null diverge.
Minor 7 — no reference entry for the new keys [verified] docs/how-to/control-the-reported-host-runtime.md covers init only; agents_found_reason (validate agents), the docs-init field and the new agent-skills warning have no reference entry, and the three key names (agents_installed_reason, agents_found_reason, the warning) are not in the Brief.
Nit 1 [judgment, carried] The null-branch prose has no test (see Major 2).
Nit 2 [judgment] agents_dir: '' and missing_agents: [] read as "nothing is missing" when the check did not run; prose and docs must caveat it, and reason? is meaningful only when agents_installed === null (a discriminated union would encode that).
Nit 3 [judgment] buildAgentSkillsBlock carries a runtimeUnknown flag beside globalSkillsBase === null, so null now means both "runtime has no skills dir" and "runtime unknown".
Nit 4 [security pass, 3/10, not a vulnerability] buildAgentSkillsBlock previously threw for an unknown runtime in config; it now still emits <agent_skills> for non-global: entries and skips only bare global:<name> ones; the namespaced global:ns/... branch was not read.
Considered, not findings: a retired id still throws from the catch (separate Error subclass, rethrown), __proto__ and constructor ids take the unknown branch (hasOwnProperty) with no path join, agents_dir '' only reaches fs after the length check, and the health rule returns no W010 for UNREADABLE; the compiled-head probes (init new-project, new-milestone, plan-phase, quick, progress, validate agents, docs-init, validate health, agent-skills) all exit 0 with agents_installed: null and unknown_runtime, and the claude runtime keeps a boolean with no reason key.
Passes run
| Pass | Result |
|---|---|
/diagnose |
run; the compiled-head probes above and the workflow prose that consumes null |
/root-cause-analysis |
run; catching UnknownRuntimeError at the two report-read callers is the right source fix, but null gives the workflow consumers no contract |
/qa-test-architect |
run; Minors 4 and 5, Nit 1, Major 1 |
/skills-from-the-artificer (bugfix-review) |
run; block below |
/code-review |
run as two parallel sub-agents: Standards (Minors 3, 4, 5, 6, 7) and Spec (acceptance consumers partial, Major 2) |
/security-review |
run: identification sub-agent found no issue at 80% (Nit 4 only) |
| Isolated adversarial pass | run by a fresh agent with no prior context; found Major 2 and Minors 1-3 independently |
Not run (Memtrace is unavailable): the Deterministic Review Pass (detect_changes, find_cross_module_issues, find_code_review_issues, find_yaml_rule_matches, review_github_pr), preflight_check, risk tiering (find_central_symbols, find_bridge_symbols), the recorded-decision check (recall_decision, governing_rules) and the co-change gate. None is reported clean. I did not execute any test (local node --test is not permitted).
Laws that apply
Subject: PR #5251 makes checkAgentsInstalled return null plus a reason for an unknown runtime id and adds null branches to the new-project and new-milestone workflows.
- postels-law — fired: the check is made lenient at the edge (an unknown id no longer throws), but the leniency is propagated as
nullto consumers that must each decide what "could not check" means; the workflow decides "proceed". Action: Major 2. - hyrums-law — fired:
agents_installedwas a boolean, is nowboolean | nullacross 29 callers, and the retired-id refusal moved behind an early return; scripts reading the field or relying on the refusal see a different contract. Action: Minors 2 and 6. - leaky-abstractions — fired:
instanceofleaks the module-copy boundary into the catch (a second copy of the class defeats it) whilecodeis the stable seam. Action: Minor 3. - kerckhoffs-principle — cleared: the retired-id refusal is by type and unchanged in effect for the install path.
Considered and cleared: knuths-optimization-principle (no optimization), kernighans-law (small catches).
CI. Red on this head: test (ubuntu-latest, 24, shard 1/3) and Required tests fail on tests/benchmark-compact-content.test.cjs:325 (Major 1). The branch is BEHIND next by one commit (a rebase is a CI condition; refresh the baseline on the rebased base so the counts are final).
CodeGraph Evidence
Memtrace unavailable: monthly quota exhausted and the server disconnected, so get_impact, get_symbol_context, recall_decision and find_code_review_issues were not run, and the recorded-decision check was not evaluated.
codegraph_impact checkAgentsInstalled(earlier pass on this head): 35 symbols, reaching 29cmdInit*entry points plusdocs,planning-snapshotandverify;codegraph_callers buildAgentSkillsBlock:cmdAgentSkills(src/init.cts:4379) andtests/runtime-homes-descriptor-drive.test.cjs. The index is of the main checkout, so it is a lower bound.- Finding: the code change is confined to those callers; the risk is the two shipped workflows that consume
null(Major 2) and the stale baseline (Major 1).
Verdict
Changes requested. Blocker 0, Major 2, Minor 7, Nit 4.
ef9b626 to
5b4838e
Compare
|
Thanks. Addressed on a branch rebased onto Major 1: Major 2: Minor 1: both parse lists now include Minor 2: disclosed in the changeset and the ADR-5057 amendment. Tests pin that Minor 3: Minor 4: the init test now requires each workflow to exit as it does for claude, with the same first error line when both fail. On success the output must parse as JSON, with Minor 5: KNOWN, retired and near-miss ids are checked exhaustively. fast-check covers arbitrary strings, with the seed pinned per CONTRIBUTING. agent-skills has zero/one/many skill-path rows that check Minor 6: the snapshot's catch now reports Minor 7: Nit 1: covered by the content-contract test above. Also found, not changed: Local verification (Node 24, macOS): |
…time id New regression tests, failing on next: every init workflow, validate agents, docs-init and agent-skills must not refuse an UNKNOWN runtime id; the agents check reports agents_installed null with reason 'unknown_runtime'; KNOWN runtimes keep a boolean and no reason key; the planning snapshot is UNREADABLE and W010 stays silent; install and uninstall still refuse an UNKNOWN id and write nothing.
… runtime id The path accessors refuse an UNKNOWN runtime id. The Agent Install Check Module's checkAgentsInstalled calls them, and it runs on every init command, validate agents, docs-init and the planning snapshot, so an EoS host integration whose runtime GSD does not know could not run any of them. buildAgentSkillsBlock resolved the global skills dir before its early returns, so agent-skills failed the same way. checkAgentsInstalled now catches only UnknownRuntimeError and reports that the check could not run: agents_installed null, no agents listed as installed or missing, no agents dir, and reason 'unknown_runtime' (frozen AGENTS_INSTALLED_REASON). init and docs-init add agents_installed_reason, and validate agents adds agents_found_reason, only when a reason is set, so output for KNOWN runtimes is unchanged. The planning snapshot maps agents_installed null to SCOPE.UNREADABLE, so W010 stays silent. buildAgentSkillsBlock resolves the skills base only when it has skill paths; for an UNKNOWN id, global: entries are skipped with their own warning and project-relative skills still resolve. A retired runtime id keeps its refusal, GSD_AGENTS_DIR still names the agents dir first, and install and uninstall still refuse.
… ADR-5057 The Agent Install Check Module entry now names the third result state (agents_installed null with a reason from AGENTS_INSTALLED_REASON), the agents_installed_reason and agents_found_reason fields, and the UNREADABLE mapping in the Planning Snapshot Module. ADR-5057 gets a dated amendment: a reporting path may catch the accessors' UnknownRuntimeError as a caller, while the accessors and install code keep the refusal.
…flows Both workflows warned only when agents_installed is false, so null skipped the warning without saying why. One sentence each now states that GSD could not check agents for the runtime and that the workflow continues normally. Emitted-Drift-Ack-Growth: new-project.md — the agents_installed null branch and the agents_installed_reason parse key Emitted-Drift-Ack-Growth: new-milestone.md — the agents_installed null branch and the agents_installed_reason parse key
New and tightened regression tests, failing where the behavior changes: a scan that throws reports agents_installed null in the planning snapshot; the W010 rule stays silent for null outside UNREADABLE; both workflows parse agents_installed_reason and, for null, probe one named spawn before the inline fallback (keeping Step 7 in new-project); a refusal from a second copy of the error class, matched by its code, is still reported by checkAgentsInstalled and skips global: skills in buildAgentSkillsBlock. Every init workflow must exit as for a KNOWN runtime (claude) and report null with the reason; the KNOWN-runtime control covers init, docs-init and validate agents; KNOWN, retired and near-miss ids are checked exhaustively and fast-check covers arbitrary strings; agent-skills gains zero/one/many skill-path rows, a namespaced global: row and the retired-id behavior of buildAgentSkillsBlock and cmdAgentSkills.
checkAgentsInstalled and buildAgentSkillsBlock matched the refusal with instanceof UnknownRuntimeError, which a second copy of the class would not satisfy. They now compare the error's code, which every copy carries.
…lent The planning snapshot's catch branch reported agents_installed false while an UNKNOWN runtime id reports null; both mean the check could not run, so the catch now reports null too. The W010 rule treats anything but false as nothing to report, so it no longer relies on the UNREADABLE guard above it.
…on null null means GSD could not check the agents, not that they are present. Both workflows told the model to continue into the named-subagent spawns, which an UNKNOWN host may not have. They now spawn the first named subagent alone (the first researcher, or gsd-roadmapper when research is skipped). If it fails with "agent type not found", new-project skips the rest of Step 6, keeps Step 7 (Define Requirements) and writes the roadmap inline instead of spawning gsd-roadmapper; new-milestone skips the research spawn step and writes the roadmap inline. Otherwise the remaining subagents are spawned as normal. Both parse lists now include agents_installed_reason.
…behavior docs/CLI-TOOLS.md describes the agents check in the init bundles (except init list-workspaces), docs-init and validate agents, including the null result, the reason keys, the GSD_AGENTS_DIR exception and what the empty lists do not mean, and the agent-skills warnings for an UNKNOWN runtime id. docs/COMMANDS.md adds agents_found_reason to the validate agents entry. The changeset and the ADR-5057 amendment disclose that buildAgentSkillsBlock no longer throws for a retired runtime id with no skill paths, while the agent-skills command still refuses it; CONTEXT.md and the ADR name the error code the catch narrows on.
The new-project workflow grew with the agents_installed null branch, so the new-project split and the aggregate drifted. Regenerated with `node scripts/benchmark-compact-content.cjs --write`. new-milestone is not a benchmark split, so it has no row to drift.
5b4838e to
88aa160
Compare
Fix PR
Linked Issue
Fixes #5236
What was broken
With an UNKNOWN runtime id, every
initcommand,validate agents, anddocs-initexited 1 withUnknownRuntimeError, and so didagent-skills <agent>. The Agent Install Check Module'scheckAgentsInstalledreadsgetDirName/getGlobalConfigDir, which refuse an UNKNOWN id throughassertKnownRuntime, and the Init Command Module'swithProjectRootcalls it for everyinitcommand.buildAgentSkillsBlockresolvedgetGlobalSkillsBasebefore its early returns.What this fix does
checkAgentsInstalledcatches onlyUnknownRuntimeErrorand reports that the check could not run:agents_installed: null,reason: 'unknown_runtime'(frozenAGENTS_INSTALLED_REASON),agents_dir: '', and no agents listed as installed or missing. The reason is the discriminator: the empty lists do not mean nothing is missing. A retired runtime id keeps itsRetiredRuntimeError, andGSD_AGENTS_DIRstill names the agents dir first.initanddocs-initaddagents_installed_reason, andvalidate agentsaddsagents_found_reason, only when a reason is set. Output for KNOWN runtimes is unchanged.buildAgentInstallField) mapsagents_installed: nulltoSCOPE.UNREADABLE, so W010 stays silent, as it does when the scan throws.buildAgentSkillsBlockresolves the skills base only when it has skill paths. For an UNKNOWN id,global:entries are skipped with their own warning; project-relative skills still resolve.new-projectandnew-milestonewarn whenagents_installedisfalse; whether a model treatsnullthe same way is not tested.resolveInstallPlan(Runtime Config Adapter Registry) throwsTypeError: Unknown runtime for install plan, notUnknownRuntimeError, onnextbefore and after this PR. The issue puts changing the install refusal out of scope, so the test pins "refuses and writes nothing" and also acceptsUnknownRuntimeError.Root cause
ADR-5057 (5) makes the path accessors refuse an UNKNOWN id.
checkAgentsInstalledis a report read on everyinit, but it called those accessors with the reported runtime and passed the refusal on, and nothing between them handled it.Testing
How I verified the fix
test(#5236)adds 17 tests; againstnext, 11 fail and the 6 that pass pin behavior this fix keeps (a KNOWN runtime keeps a boolean and no reason key; install and uninstall still refuse;GSD_AGENTS_DIRtwice; the accessor refusal; retired ids).fix(#5236)makes all 17 pass.config.runtimeset: 3 before, 0 after.nextbuild for claude and codex: identical output across 321 invocations (the census set minus commands that mutate state or print the clock).npm test(Node 24, macOS): no new failures. 7 tests fail locally onnextand on this branch alike, from local git config (commit.gpgsign,core.hooksPath) and the macOS/var→/private/vartemp path; they pass withGIT_CONFIG_GLOBAL=/dev/nulland a realpathTMPDIR.npm run lint:cipasses.Regression test added?
Platforms tested
Runtimes tested
example-host, and an EoS host integration's id through the install marker); claude and codex output compared byte for byteChecklist
Fixes #NNN— PR will be auto-closed if missingconfirmed-buglabelnpm test).changeset/fragment added if this is a user-facing fix (npm run changeset -- --type Fixed --pr <NNN> --body "...") — orno-changeloglabel appliedStandards
Followed
CONTEXT.md(Runtime Descriptor Owner Module, Agent Install Check Module, Init Command Module, Planning Snapshot Module, Planning Scope Module) and ADR-5057 (5). The accessors still refuse an UNKNOWN id. The catch in the Agent Install Check Module andbuildAgentSkillsBlockis new caller-side behavior. It follows the Phase 10 amendment's reasoning forhostBehaviorsFor(a throw would crash a session rather than skip a behavior), but that amendment does not cover it: this is an extension, not ADR text.New concept, for the maintainers to place in
CONTEXT.mdif wanted: the Agent Install Check Module's result can be "check could not run" (agents_installed: nullwith a reason fromAGENTS_INSTALLED_REASON), beside the boolean scan result. It follows the in-band reason pattern of Resolution Provenance and the reason-enum lock convention (enum, emitting site, a test lockingObject.keys(...).sort()).Breaking changes
agents_installed(andagents_foundinvalidate agents) isnullfor an UNKNOWN runtime id; before, those commands exited 1. KNOWN runtimes are unchanged.