From 382bf7c4231a59b877fc627e782632b8d4521ba4 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 25 Aug 2026 19:54:30 -0400 Subject: [PATCH] fix(#3706): deliver the resolved reasoning effort to OpenCode subagents (#3867) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#3706): failing-first coverage for OpenCode variant emission and frontmatter escaping * fix(#3706): emit the resolved reasoning effort as OpenCode's variant key `query resolve-execution` resolved an effort level for every agent, but the OpenCode bake wrote only `model:` — the effort never reached the generated agent, so subagents ran at whatever the runtime defaulted the model to. This is the effort-side twin of the model-side defect fixed in #3705. The key is written only when an `effort` block is actually configured. `resolveInstallTimeEffort` always returns a level (the catalog default is `high`), so gating on its return value would stamp `variant: high` into every existing OpenCode install — and OpenCode resolves a variant name against a `variants` map in the user's `opencode.jsonc`, so a value nobody declared is not a safe default. Gating on `readGsdEffectiveEffortConfig` keeps installs that never asked for effort routing byte-identical. Kilo does not receive the key: `EFFORT_ARGV` declares surfaces for claude, opencode and codex and has no kilo entry. This is deliberately asymmetric with the model side, where #2794 J8 requires the two runtimes to resolve alike. Both frontmatter sinks now route through `frontmatterScalar`, which quotes and escapes any value that is not a plain scalar. The raw interpolation predates this change, but it was already shown by execution during the #3705 security review to let a config value containing a newline inject additional top-level keys (`tools:`, `permission:`) into a generated agent file. This change adds a second write to that sink, so it is closed here rather than doubled. * fix(#3706): quote frontmatter values YAML would not read back verbatim Self-review of the predicate added in the previous commit. Treating /^[A-Za-z0-9._:/@+-]+$/ as 'safe to emit bare' answers the wrong question: a value can match it and still not round-trip. - A leading '@' is a YAML *reserved* indicator and may not open a plain scalar at all, so a scoped ID like '@org/model' emitted bare is a parse error, not an ambiguity — the whole agent file becomes unreadable. - 'no' / 'y' / 'off' / 'null' resolve to booleans and null, so a variant with one of those names would match no entry in the user's variants map. - '12:30' resolves to 750 under YAML 1.1 sexagesimal, and ':' is legal mid-identifier here, so the form is reachable rather than contrived. Real model IDs pass every clause and stay bare, so already-generated files remain byte-identical. * fix(#3706): route variant through the declared effort seam and cover the live path Addresses six findings from the isolated review, all confirmed by execution. The tests were the serious one: they required `../bin/install.js` while the fix landed in src/, which compiles to gsd-core/bin/lib/. They exercised a different copy of the converter than the one the bake actually uses, so the whole suite was green-by-construction against unchanged code and the remote run failed all 13. Every case now runs against BOTH copies from one table, which doubles as the parity assertion the generative-fix note in runtime-artifact-conversion.cts asks for, and bin/install.js carries the mirrored change. Emission no longer hand-rolls the value. It goes through `renderEffortArgv`, the declared OpenCode effort seam (EFFORT_ARGV.opencode: its own supported set and clamp). That is what rejects a level that is not a wire value — above all `inherit`, which per #3533 (10d) means "omit the key and follow the host default" and was previously written literally, naming a variant that cannot resolve. Reachable two ways, both now pinned: an agent_overrides entry and a routing_tier_defaults entry. A bare effort.default does NOT reach a tiered agent (the #3531 tier ladder answers first), so a test written against `default` alone asserts nothing — that is pinned too. The plain-scalar decision moved into frontmatter.cts beside `scalarNeedsDoubleQuoting` rather than sitting next to it as a second, weaker predicate. `agentScalarNeedsDoubleQuoting` is a documented superset: it adds a trailing `:` (read as a nested mapping key, which fails the whole frontmatter), boolean/null words, and numeric-looking values including YAML 1.1 sexagesimal. Docs now state the cascade plainly: the gate is on effort being configured at all, not on the individual agent being named, so every generated OpenCode agent gets a variant line once any effort block exists. * test(#3706): assert the two frontmatterScalar copies cannot diverge A hand-picked adversarial corpus plus a fast-check property over YAML-significant strings, both run against bin/install.js and the live src copy. Verified the property can actually fail: mutating one copy's quoting rule is killed well inside the run budget. * fix(#3706): close the review findings — predicate, seam, and dead mirror Third review round; every item below was confirmed by execution. The scalar predicate was wrong in two families, both found by a round-trip property test rather than by reading. Basing it on scalarNeedsDoubleQuoting dropped the "first character must be alphanumeric" clause, so `~`, `.inf`, `.nan`, `+1`, `-0` and `.5` went out bare and came back as null/floats/ints; and that base predicate only inspects the FIRST character, so an embedded `: ` (a nested mapping, i.e. a parse error) or ` #` (a comment, i.e. silent truncation) also passed. Dates round out the set: `2026-08-25` opens alphanumeric, survives every other clause, and YAML resolves it to a Date. The property now asserts the contract directly over generated values instead of trusting an enumerated character list. The bin/install.js mirror is gone. Its premise was false — install.js already requires bin/lib at :65 — and it was unreachable besides: install.js's convertClaudeToOpencodeFrontmatter has no `isAgent: true` call site, because its agents path resolves converters from the compiled module. It was a third copy of the YAML rules serving a test rather than a caller, so the file is back to origin/next and the tests target the live copy only. Effort clamping moved to `clampEffortForHost`, which renderEffortArgv now delegates to. The layout was calling renderEffortArgv with a hardcoded 'argv' to borrow its clamp, which read as if the frontmatter key were gated on the invocation-time axis. It is not: claude declares effortSurface "argv" and independently bakes an effort: key. One capability table, one clamp, two channels that no longer pretend to be each other. Also corrects an earlier claim of mine: adding EFFORT_RENDERING.opencode would NOT have made `effort sync` write the wrong key, because it guards on the runtime name before it ever renders. The seam choice stands on other grounds. `effort sync` still skips OpenCode, but its stated reason claimed OpenCode "does not use effort: frontmatter", which this change makes false — so the message now says what is actually true. * docs(#3706): restate the changeset around the round-trip contract * fix(#3706): restore the changeset fragment belonging to #3809 An earlier commit in this branch picked the first file in .changeset/ by glob order instead of the fragment created for this issue, and overwrote agile-geese-squeak.md (PR 3815 / #3809) with this change's body. Restored verbatim from origin/next; this change's text now lives in its own patient-cranes-parade.md, where it was created. * feat(#3706): maintain the OpenCode variant key from effort sync Install bakes the resolved effort into OpenCode agent frontmatter as `variant:`, so `effort sync` has to maintain it or a config change only takes effect on reinstall — and its skip message claimed OpenCode does not use frontmatter effort at all, which this issue made false. cmdEffortSyncOpencode mirrors the codex branch: resolve per agent, clamp through the declared OpenCode capability, then write, strip, or skip. A null target means the key must not exist, which covers both "no effort configured" and "resolved to inherit or to an unsupported level" — the same states under which install writes nothing, so sync and install agree by construction. The frontmatter line-editors are key-parameterised rather than copied: setEffortFrontmatter / removeEffortFrontmatter are now thin wrappers over the same internals the variant path uses, and a test pins that the claude `effort:` behavior did not move. The child-process test harness fixes both HOME and USERPROFILE, so the hermetic-config assertions cannot pass vacuously on Windows. * fix(#3706): scope the frontmatter line editors to the matched block Found by the security review of the sync path, reported as correctness rather than vulnerability, and reproduced against pre-fix code before being fixed. Both editors matched the frontmatter with a regex that can match a block after a preamble, then derived the EOL and the opening-fence length from the START OF THE FILE. On a CRLF document with a preamble those disagree, the offsets shift by one byte, and the reassembled document comes back with a mangled fence (`---\rname: x`). Both now take the EOL from the matched block. `setFrontmatterKeyLine` additionally did a whole-file `/m` replace when the key already existed, gated only on the key being present in the frontmatter body — so a preamble line starting with the same key was rewritten instead of the frontmatter one. It now replaces inside the frontmatter span only, which is the hazard `removeFrontmatterKeyLine` already documented and guarded against. Neither is reachable from an install-written `gsd-*.md` (those begin at byte 0 with `---`), and both predate this change — but the editors are in this diff because #3706 key-parameterised them, so they are fixed here rather than left for the next caller to trip over. Three regression tests, each confirmed to fail against the pre-fix build. * fix(#3706): treat a present-but-empty key as present, and pin the real seam Fourth review round. The MAJOR one: both sync branches read the current value with `(.+?)`, which needs at least one character, so a key present with an EMPTY value read as "key absent". When the target was also null the code concluded "already correct" and skipped — leaving the key in the file, where it reads back as YAML `null`: exactly the unresolvable-variant state this change exists to prevent. Whitespace decided whether it fired, since `variant: ` matched and `variant:` did not. Presence and value are now separate questions at both the opencode and the claude branch. The OpenCode writer now follows the codex branch rather than the claude one: tmp file plus retryRenameSync with orphan cleanup, and a write failure skips that agent and is reported instead of aborting the sweep. Same granularity, same transient-Windows-lock exposure, so the hardened sibling was the right precedent. Also: the generic line-editors escape their interpolated key, the JSDoc stranded by the clampEffortForHost extraction is back on renderEffortArgv, and a cast that declared a nullable function as non-nullable is corrected. Tests close the gaps the review listed — empty value (both spellings), CRLF round-trip through write and strip, the symlink guard, a body line starting `variant:`, a file with no frontmatter, and the YAML classes that actually broke the predicate. The new layout-seam test drives the real stage() path and was verified to FAIL when `variant` is removed from the converter call; a seam test that survives cutting the seam is worse than none. * fix(#3706): clear the round-five review findings No blockers or majors this round; the repo's review gate is zero-tolerance, so the minors are cleared too. A duplicated key was only half-stripped: the strip regex had no `g` flag, so a frontmatter carrying the key twice lost one occurrence, reported success, and left the "a null target means the key must not exist" invariant false on disk — converging only on a second run. Such a document is already invalid YAML, so this is robustness rather than a live corruption path, but a successful sync has to leave the invariant true. A run in which every write failed still summarised as `ok`, so a caller could not tell "nothing to do" from "everything failed". The OpenCode branch now reports `failed` when any write failed. The write-failure path was also the newest code in the change with no coverage at all; it now has a test that injects the failure by monkeypatching the write, per CLAUDE.md §4, rather than by chmod — mode bits do not bite under root in CI. `CodexEffortSyncWriteFailure` is renamed `EffortSyncWriteFailure` now that two branches share it. Removed a guard on the claude concrete path that was provably unreachable — no member of EFFORT_SET renders null there, so it read as protection that did not exist. The claude inherit path's presence check is load-bearing and untouched. Three stale statements corrected: the OpenCode result shape matches codex's, not claude's, now that it emits write_failures; the `thread()` test helper now calls `clampEffortForHost` so it genuinely mirrors the layout instead of merely claiming to; and a test helper restored `USERPROFILE` by assignment, writing the literal string "undefined" into the environment on POSIX — it deletes now. * fix(#3706): converge the set path, degrade on unreadable files, preserve mode Rounds five and six of review. No blockers or majors; the review gate is zero-tolerance, so the minors are cleared too. `setFrontmatterKeyLine` was the mirror of a defect already fixed in its sibling: `remove` was made global, `set` was not, so on a frontmatter carrying the key twice it rewrote the first and left a stale second. Last-wins YAML readers honour the stale value while the sync's own first-occurrence read reports "in sync" — permanently non-converging. It now collapses to exactly one occurrence, in the position of the first, so ordinary single-occurrence documents stay byte-identical (verified across seven shapes before and after). An unreadable agent file used to throw and abort the entire sweep, while a failed WRITE in the same loop degraded into a report. The OpenCode branch now reports read failures alongside write failures; the claude branch degrades to a skip without a new result field, because its shape is long-standing and widely consumed and one bad file aborting the sweep is the actual defect. The tmp+rename publish dropped the original file's mode — a plain writeFileSync preserves it, a rename does not — so a 0600 agent came back 0644. Both the OpenCode and the codex branch now carry the original's permission bits across the publish, masked with 0o7777: the raw stat mode includes the file-type bits, and POSIX leaves those unspecified for chmod. Linux is the only OS the remote matrix runs, so relying on Darwin's tolerance would have been untestable here. Also documents the `from` contract on EffortSyncChange (null means the key was absent, '' means present with an empty value — a distinction earlier rounds introduced and then collapsed in the output), adds OpenCode to the docs paragraph enumerating where the key is omitted under inherit, and records in a comment that the 'failed' summary reaches only raw mode and does not change the exit code, which is a CLI-contract change affecting all three branches and is deliberately not made here. * fix(#3706): guard the codex read, close the tmp permission window, rename the failure type Round seven, plus one thing I found myself. `cmdEffortSyncCodex` still had an unguarded `fs.readFileSync` — a read fault on one agent exited 1 and aborted the whole sweep. The claude and opencode branches were both guarded earlier this round and codex was missed, with the unguarded read sitting ten lines above the chmod block the previous commit did edit. It now reports read failures the way the OpenCode branch does, and a read failure flips its summary to `failed` — which write failures did not do there either, so both are corrected for consistency. The tmp file was created at the default mode and only tightened afterwards, so a 0600 agent's contents sat in a 0644 file for the length of the publish. I measured the window rather than assuming it, then closed it by passing the mode at creation. The chmod after the write is deliberately RETAINED and commented: the `mode` option only applies when the file is actually created, so a leftover tmp from an earlier crashed run would be truncated and reused at its old mode, and the chmod is what corrects that. `EffortSyncWriteFailure` is renamed `EffortSyncFileFailure` — it was typing a `read_failures` array, the same naming-lie the `Codex…` prefix had last round. Also pins the codex mode preservation with a test. It only writes on a path that genuinely rewrites the file, so the fixture is an Anthropic-flavoured model pin the sync strips, and the test asserts the content changed before checking the mode — otherwise it would pass on a sync that did nothing. * fix(#3706): guard the claude writes and share one escaping rule The security sign-off caught a comment of mine that was factually wrong: the new claude read guard said the failure is folded in "like the write path in this same loop does", and there was no write guard in that loop. Rather than correct the sentence, both claude write sites are now guarded the way the read is — a failed file is skipped, the sweep continues, and the raw summary token flips to `failed`. The JSON shape stays frozen deliberately, because it is long-standing and widely consumed; the token is the channel that can carry the signal without a compatibility risk, which is the reviewer's own suggestion. That makes all three branches consistent: reads and writes guarded everywhere, per-file failures degrade instead of aborting, and every branch reports `failed` rather than `ok` when something did not sync. `setFrontmatterKeyLine` interpolated its value raw while the install-side writer quoted through the shared helpers — two writers of the same frontmatter key disagreeing on escaping, the divergence class this repo requires closed. They now share one rule. Verified no churn: all six effort levels are plain scalars and emit byte-identically, with claude's documented minimal-to-low clamp the only difference in the table, exactly as before. * fix(#3706): publish claude agent writes atomically too Both reviewers found this independently, and it is data loss rather than a reporting gap. The claude branch wrote in place, so `fs.writeFileSync`'s O_TRUNC meant a post-open fault left the agent file truncated or half-written: an injected ENOSPC produced an empty file, and under `ulimit -f` a 60000-byte agent came back as 512 bytes of wrong content. The guard added earlier this round then counted that destroyed file as `skipped`, which in JSON mode is indistinguishable from "already in sync" — so a caller would have read the sweep as clean while an agent on disk was corrupt. It now publishes the way the codex and opencode branches already do: write to a tmp file created at the original's masked mode, chmod, then retryRenameSync, with the tmp unlinked and the agent skipped on any failure. The corrupting case is gone rather than merely reported, which matters because this branch deliberately takes no new result key. I had claimed all three branches were consistent after the previous commit. That was true for degradation and reporting and not for atomicity; the reviewer caught the overclaim. It is true now. Also sorts the claude file list, which the other two branches already did — readdir order is platform-dependent, so leaving it unsorted made the reported `changes` ordering differ across machines for identical inputs. * chore(#3706): backfill the changeset PR number pr:0 placeholder replaced with the real PR now that gh api returned it. * test(#3706): kill the frontmatter mutants this change introduced CI's Stryker frontmatter shard scored 60.58 against a break floor of 62. The cause is documented in the lane's own config, from #1882: this PR added a multi-clause predicate to frontmatter.cts and exported the escaper, but the tests constraining them live in tests/runtime-converters.test.cjs, which that shard does not run — so every mutant in the new code was uncovered there even though the behaviour is tested elsewhere. The fix is assertions that kill real mutants, per the repo's own instruction, not a lowered floor and not a Stryker disable: scripts/mutation-matrix.cjs is untouched. Each clause of agentScalarNeedsDoubleQuoting now has a true case AND a near-miss that must answer the opposite way, so flipping the clause fails a specific named test — alnum-first against `a-b`, trailing `:` against `foo:bar`, embedded `: ` against `a:b`, embedded ` #` against `a#b`, the word list against `yes1`/`nullish`, the numeric forms against `1a`/`0xzz`, the timestamp against `2026-08-25x`, plus the case-insensitive spellings that pin the `i` flag. escapeDoubleQuoted is pinned on exact output, including a case constructed so that escaping in the wrong ORDER yields a different string. Two of my expectations were wrong and are asserted as the code actually behaves: `12:99` is NOT quoted, because the sexagesimal alternative never range-checks minutes and so does not match — which is right, since YAML would not read it as sexagesimal either; and `20260825` is quoted by the numeric clause rather than the timestamp one, being a bare integer. * chore(#3706): ratchet the frontmatter mutation floor to 65 The lane measured 66.67 on PR 3867 after the mutant-killing unit tests landed — above its pre-change 63.35 baseline, not merely recovered. Step 3 of this file's own HOW TO UPDATE procedure says to set minScore = floor(measured) - 1 in the same diff, so 62 becomes 65 and the improvement is locked in rather than left free to slide back. The ledger of measured scores now records the new measurement, why the shard broke in the first place (logic added to frontmatter.cts whose only tests lived in a file this lane does not run — the same trap the #1882 note describes), and one discrepancy: step 3 also says to update "the matching RATCHET_BASELINE entry", but no such declaration exists in this file. The name appears only in that comment, so minScore and the ledger are all there is to update. * fix(#3706): update RATCHET_BASELINE alongside the raised floor The ratchet test caught the previous commit: it raised COVERED['frontmatter'] .minScore to 65 without updating the baseline that mirrors it, which is exactly the mismatch that guard exists to make visible in review. I had claimed RATCHET_BASELINE did not exist. It does — in tests/mutation-matrix-ratchet.test.cjs, not in scripts/mutation-matrix.cjs, which is the only file I searched before concluding it was a stale reference. The ledger comment is corrected to say where it lives and to record that the guard caught the error rather than leaving my wrong claim on the record. * docs(#3706): put the mutation ledger entries back under their own dates The 2026-08-25 measurement was spliced into the middle of the 2026-06-14 list, so adr-parser, config-schema, active-workstream-store and core-utils ended up sitting under the wrong heading and misattributing their measurement dates. That ledger is what a future change reads to calibrate a floor, so a wrong date there is not cosmetic. Each measurement is now under the date it was taken. Also drops the first-person account of my own mistake from the entry — the factual half (where RATCHET_BASELINE lives, and that it is updated in the same diff) is what a reader needs; the confession is not. --------- Co-authored-by: sim --- .changeset/patient-cranes-parade.md | 5 + docs/CONFIGURATION.md | 13 +- scripts/mutation-matrix.cjs | 16 +- src/commands.cts | 506 +++++++- src/frontmatter.cts | 53 + src/model-catalog.cts | 38 +- src/runtime-artifact-conversion.cts | 43 +- src/runtime-artifact-layout.cts | 37 +- tests/effort-sync-installed-runtime.test.cjs | 1186 ++++++++++++++++++ tests/frontmatter.unit.test.cjs | 235 ++++ tests/mutation-matrix-ratchet.test.cjs | 2 +- tests/runtime-converters.test.cjs | 371 ++++++ 12 files changed, 2449 insertions(+), 56 deletions(-) create mode 100644 .changeset/patient-cranes-parade.md diff --git a/.changeset/patient-cranes-parade.md b/.changeset/patient-cranes-parade.md new file mode 100644 index 000000000..5db274f71 --- /dev/null +++ b/.changeset/patient-cranes-parade.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3867 +--- +**OpenCode subagents now carry the reasoning effort GSD resolved for them** — `query resolve-execution` reported an effort level that never reached the generated OpenCode agent, so every subagent ran at whatever the runtime defaulted the model to, silently ignoring `effort` config. The bake now emits a `variant` key alongside `model`, and `effort sync` maintains it, so changing effort config no longer needs a reinstall. The key is written only when effort is actually configured; `inherit` and any level OpenCode does not accept omit it rather than naming a variant that cannot resolve. Config-supplied `model` and `variant` values are also quoted whenever YAML would not read them back verbatim — previously a value containing a newline could inject extra top-level keys into a generated agent file, and values like `no`, `12:30`, `@org/model` or a bare date were silently retyped or truncated. (#3706) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index fef50dc8f..ad7441299 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1560,7 +1560,13 @@ Control the reasoning effort of agent invocations with a single config. The univ minimal < low < medium < high < xhigh < max ``` -Effort is rendered per-runtime: `output_config.effort` for Claude (Claude Code subagent `effort` frontmatter / `CLAUDE_CODE_EFFORT_LEVEL` env), `model_reasoning_effort` for Codex (Responses API `reasoning.effort`). +Effort is rendered per-runtime: `output_config.effort` for Claude (Claude Code subagent `effort` frontmatter / `CLAUDE_CODE_EFFORT_LEVEL` env), `model_reasoning_effort` for Codex (Responses API `reasoning.effort`), and `variant` for OpenCode (agent frontmatter). + +**OpenCode `variant` is opt-in ([#3706](https://github.com/open-gsd/gsd-core/issues/3706)).** OpenCode resolves a `variant` name against the variants available for the agent's model — the built-in sets are provider-specific (Anthropic ships `high` and `max`; OpenAI the full ladder) and you can define your own in `opencode.jsonc`. GSD writes the key only when an `effort` block is actually configured; with no `effort` config the generated agent carries no `variant` line and OpenCode applies its own default. + +Note that the gate is on effort being configured **at all**, not on the individual agent being named. Once any `effort` block exists, the usual cascade resolves a level for *every* agent — an `agent_overrides` entry for one agent still leaves the others resolving through `routing_tier_defaults` and the tier ladder — so every generated OpenCode agent gets a `variant` line, not only the one you named. Two levels are never written: `inherit` (which means "follow the host default", so the key is omitted) and any level outside OpenCode's supported set. Runtimes with no declared effort surface — Kilo among them — never receive the key at all. + +`effort sync` maintains the key too, so changing `effort` config does not require a reinstall: it writes the newly resolved `variant` into each installed OpenCode agent, and removes the key when the agent resolves to `inherit` or to a level OpenCode does not accept — the same states under which install writes nothing. **Cross-provider clamping:** `minimal` is Anthropic-unsupported — it clamps to `low` on Claude. @@ -1658,8 +1664,9 @@ The model-catalog's `reasoning_effort` per-tier hint is a legacy field kept for Valid effort values: `minimal`, `low`, `medium`, `high`, `xhigh`, `max`, and `inherit` ([#3533](https://github.com/open-gsd/gsd-core/issues/3533)). `inherit` means "follow the session/host default" — it is a declarable choice, not a level: -at install time the agent's `effort:` frontmatter key (claude) or `model_reasoning_effort` -pin (Codex `.toml`) is **omitted** for an agent resolving to `inherit`; `effort sync` treats +at install time the agent's `effort:` frontmatter key (claude), `model_reasoning_effort` +pin (Codex `.toml`), or `variant:` frontmatter key (OpenCode) is **omitted** for an agent +resolving to `inherit`; `effort sync` treats an absent key as the correct in-sync state and strips a present one; no runtime ever receives the literal. An explicit `inherit` also never escalates on failed attempts — your choice outranks the automatic ladder. diff --git a/scripts/mutation-matrix.cjs b/scripts/mutation-matrix.cjs index bc3af2802..10dab79f4 100644 --- a/scripts/mutation-matrix.cjs +++ b/scripts/mutation-matrix.cjs @@ -114,12 +114,24 @@ const TARGET_MUTATION_SCORE = 80; // Measured CI scores 2026-06-14 (issue #1187, timeout-free — source of truth): // context-utilization 92.31% → floor 80 (target already met) // prompt-budget 68.33% → floor 66 (local was 99.6% — TIMEOUT INFLATION; CI is the truth) -// frontmatter 63.35% → floor 62 +// frontmatter 63.35% → floor 62 (SUPERSEDED — see 2026-08-25 below) // adr-parser 69.30% → floor 68 // config-schema 54.55% → floor 52 (local was 69.7% — TIMEOUT INFLATION; CI is the truth) // active-workstream-store 81.91% → floor 80 // core-utils 77.52% → floor 75 // +// Measured CI score 2026-08-25 (#3706, PR 3867): +// frontmatter 66.67% → floor 65 +// #3706 added agentScalarNeedsDoubleQuoting to frontmatter.cts and exported +// escapeDoubleQuoted, but the tests constraining them lived in +// tests/runtime-converters.test.cjs, which this lane does NOT run — the same trap the +// #1882 note on the frontmatter entry describes. The shard fell to 60.58 and broke the +// floor. Direct unit tests for both were added to tests/frontmatter.unit.test.cjs, each +// clause paired with a near-miss that must answer the opposite way, which took the module +// above its pre-change score. Floor ratcheted per the HOW TO UPDATE formula above, and +// RATCHET_BASELINE — which lives in tests/mutation-matrix-ratchet.test.cjs, not here — is +// updated in the same diff as that procedure requires. +// // LESSON: floors MUST be calibrated from CI mutation runs (CI runs with // timeout≈0, deterministic). Local runs count timeouts as kills and // inflate scores significantly (prompt-budget: 99.6% local vs 68.3% CI; @@ -169,7 +181,7 @@ const COVERED = { // no test in the shard, so the module's score drops even though the behaviour is tested. 'tests/unusable-input.test.cjs', ], - minScore: 62, + minScore: 65, }, 'adr-parser': { cjs: 'gsd-core/bin/lib/adr-parser.cjs', diff --git a/src/commands.cts b/src/commands.cts index b2055ca7a..a41ac1119 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -10,6 +10,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { normalizeEol } from './text-lines.cjs'; import { execGit, platformWriteSync, platformReadSync, platformEnsureDir, isSpawnTimeout, retryRenameSync } from './shell-command-projection.cjs'; +import { escapeRegex } from './pattern.cjs'; import { requireSafePath, sanitizeForDisplay } from './security.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); @@ -51,7 +52,7 @@ import planningWorkspace = require('./planning-workspace.cjs'); const { planningDir, planningPaths } = planningWorkspace; // eslint-disable-next-line @typescript-eslint/no-require-imports import frontmatter = require('./frontmatter.cjs'); -const { extractFrontmatter } = frontmatter; +const { extractFrontmatter, agentScalarNeedsDoubleQuoting, escapeDoubleQuoted } = frontmatter; // eslint-disable-next-line @typescript-eslint/no-require-imports import modelProfiles = require('./model-profiles.cjs'); const { MODEL_PROFILES, VALID_PHASE_TYPES } = modelProfiles; @@ -106,6 +107,9 @@ interface CommitToSubrepoRepoResult { interface EffortSyncChange { agent: string; + // #3533 (10d) / #3706: from === null means the key was ABSENT; from === '' + // means the key was PRESENT with an empty value. Collapsing both to null + // would make "no key" and "empty key" indistinguishable in sync output. from: string | null; // #3533 (10d): to === null is the typed IR for omission (inherit strips the key). to: string | null; @@ -738,45 +742,113 @@ function effortSurfaceForHost(cwd: string, host: string): string { } /** - * #488 — Replace or inject the `effort:` value in YAML frontmatter. + * #488 — Replace or inject the `:` value in YAML frontmatter. * Unlike injectEffortFrontmatter (install.js), this overwrites an existing value. + * #3706: key-parameterised so the same line-editor serves both claude's + * `effort:` and OpenCode's `variant:`. #3706: all offsets (eol, openLen, + * closingStart) are derived from the MATCHED BLOCK, not the start of the + * file, and the existing-key replace is scoped to the frontmatter span only. */ -function setEffortFrontmatter(content: string, effortValue: string): string { - const eol = /^---\r\n/.test(content) ? '\r\n' : '\n'; +function setFrontmatterKeyLine(content: string, key: string, value: string): string { const fmRe = /^---\r?\n([\s\S]*?)^---\r?$/m; const match = fmRe.exec(content); if (!match) return content; const fmBody = match[1]; - if (/^effort:/m.test(fmBody)) { - return content.replace(/^(effort:)[ \t]*.*$/m, `$1 ${effortValue}`); - } + // Both writers of these frontmatter keys — this sync path and the + // install-side `frontmatterScalar` in runtime-artifact-conversion.cts — + // now share one escaping rule: quote via `agentScalarNeedsDoubleQuoting` + + // `escapeDoubleQuoted` (both from frontmatter.cts) rather than each + // interpolating `value` raw/differently. + const renderedValue = agentScalarNeedsDoubleQuoting(value) ? `"${escapeDoubleQuoted(value)}"` : value; + // EOL comes from the MATCHED BLOCK, not the start of the file. With a + // preamble the two can disagree, and on a CRLF document that misaligns every + // offset below by one byte and mangles the opening fence. + const eol = /^---\r\n/.test(match[0]) ? '\r\n' : '\n'; const openLen = 3 + eol.length; - const closingStart = match.index + openLen + fmBody.length; - return content.slice(0, closingStart) + `effort: ${effortValue}${eol}` + content.slice(closingStart); + const bodyStart = match.index + openLen; + const closingStart = bodyStart + fmBody.length; + // #3706: key is now generic (not just the literal 'effort'/'variant' + // callers happen to pass today) — escape it before interpolating into the + // RegExp so a future caller can't have its key metacharacters reinterpreted. + const keyLineRe = new RegExp(`^(${escapeRegex(key)}:)[ \\t]*.*$`, 'm'); + if (keyLineRe.test(fmBody)) { + // #3706: a duplicated `:` line is already invalid YAML, but a + // non-first-wins reader (last-wins) would otherwise honour a stale + // second occurrence left behind by a naive single-hit replace, while + // this function's own single-hit read reports "in sync" — a + // permanently non-converging state. Use a GLOBAL replace with a + // first-hit flag so every occurrence collapses to exactly one, IN THE + // POSITION of the first occurrence (never delete-then-append, which + // would move the key to the end of the frontmatter and churn every + // already-generated single-occurrence file). + const escaped = escapeRegex(key); + let seen = false; + const newBody = fmBody.replace( + new RegExp(`^${escaped}:[ \\t]*.*(\\r?\\n?)`, 'gm'), + (_m, nl) => { + if (!seen) { + seen = true; + return `${key}: ${renderedValue}${nl}`; + } + return ''; + }, + ); + // Replace INSIDE the frontmatter span only: a whole-file /m replace would + // rewrite an earlier preamble line that happens to start with this key. + return content.slice(0, bodyStart) + newBody + content.slice(closingStart); + } + return content.slice(0, closingStart) + `${key}: ${renderedValue}${eol}` + content.slice(closingStart); } /** - * #3533 (10d) — remove exactly the frontmatter `effort:` line (and its line + * #3533 (10d) — remove exactly the frontmatter `:` line (and its line * ending) so an agent configured for `inherit` carries NO key. Mirrors the * codex-agent-toml strip discipline: targeted line removal, EOL-aware, every * other byte (comments, sibling keys, the body) untouched. + * #3706: key-parameterised so the same line-editor serves both claude's + * `effort:` and OpenCode's `variant:`. #3706: openLen is derived from the + * MATCHED BLOCK, not the start of the file — a preamble on a CRLF document + * would otherwise misalign every offset below. */ -function removeEffortFrontmatter(content: string): string { +function removeFrontmatterKeyLine(content: string, key: string): string { // Scoped to the FIRST frontmatter block (not a whole-file /m match): a - // preamble or body line starting with `effort:` (a fenced config example, + // preamble or body line starting with `:` (a fenced config example, // a thematic-break flanked fragment) must never be the line removed. const fmRe = /^---\r?\n([\s\S]*?)^---\r?$/m; const match = fmRe.exec(content); if (!match) return content; const fmBody = match[1]; - const lineRe = /^effort:[ \t]*.*\r?\n?/m; + // #3706: same generic-key escape as setFrontmatterKeyLine above. + const lineRe = new RegExp(`^${escapeRegex(key)}:[ \\t]*.*\\r?\\n?`, 'm'); if (!lineRe.test(fmBody)) return content; - const strippedFm = fmBody.replace(lineRe, ''); - const openLen = 3 + (/^---\r\n/.test(content) ? 2 : 1); + // A duplicate `:` mapping key is already invalid YAML (a document with + // two `effort:`/`variant:` lines does not parse), so this is robustness + // against a malformed document, not a live corruption path. Still, "a null + // target means the key must not exist" is an invariant this function must + // leave true on disk — a non-global replace here would strip only the + // FIRST occurrence and require a second run to converge. Use a fresh + // global RegExp for the strip so every occurrence in the frontmatter body + // is removed in one pass. + const stripAllRe = new RegExp(`^${escapeRegex(key)}:[ \\t]*.*\\r?\\n?`, 'gm'); + const strippedFm = fmBody.replace(stripAllRe, ''); + // Same rule as setFrontmatterKeyLine: the EOL must come from the matched + // block, not the start of the file, or a preambled CRLF document misaligns. + const eol = /^---\r\n/.test(match[0]) ? '\r\n' : '\n'; + const openLen = 3 + eol.length; const closingStart = match.index + openLen + fmBody.length; return content.slice(0, match.index + openLen) + strippedFm + content.slice(closingStart); } +/** #488 — Replace or inject the `effort:` value in YAML frontmatter. */ +function setEffortFrontmatter(content: string, effortValue: string): string { + return setFrontmatterKeyLine(content, 'effort', effortValue); +} + +/** #3533 (10d) — remove exactly the frontmatter `effort:` line (and its line ending). */ +function removeEffortFrontmatter(content: string): string { + return removeFrontmatterKeyLine(content, 'effort'); +} + /** * #488 — Re-sync effort: frontmatter in all installed gsd-*.md agent files to * match the current effort config, without requiring a full reinstall. @@ -803,6 +875,15 @@ function cmdEffortSync(cwd: string, raw: boolean, opts?: { dryRun?: boolean; con return; } + // #3706: install now bakes OpenCode's resolved effort into agent + // frontmatter under the `variant:` key (not `effort:`), so OpenCode gets + // its own sync path — mirroring the codex branch above — rather than + // falling into the generic "does not use effort: frontmatter" skip. + if (runtime === 'opencode') { + cmdEffortSyncOpencode(cwd, raw, dryRun, opts.configDir); + return; + } + if (runtime !== 'claude') { output({ synced: 0, skipped: 0, changes: [], dry_run: dryRun, reason: `runtime '${runtime}' does not use effort: frontmatter` }, raw, ''); return; @@ -816,8 +897,8 @@ function cmdEffortSync(cwd: string, raw: boolean, opts?: { dryRun?: boolean; con // package-root bin/install.js, which the installer never copies into a runtime home). // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/unbound-method const { readGsdEffectiveEffortConfig, resolveInstallTimeEffort } = require('./install-effort-resolver.cjs') as { - readGsdEffectiveEffortConfig(cwd: string): Record; - resolveInstallTimeEffort(cfg: Record, agentName: string): string; + readGsdEffectiveEffortConfig(cwd: string): Record | null; + resolveInstallTimeEffort(cfg: Record | null, agentName: string): string; }; const effortCfg = readGsdEffectiveEffortConfig(cwd); @@ -832,15 +913,47 @@ function cmdEffortSync(cwd: string, raw: boolean, opts?: { dryRun?: boolean; con const files = fs.readdirSync(agentsDir).filter(f => { if (!f.startsWith('gsd-') || !f.endsWith('.md')) return false; try { return fs.lstatSync(path.join(agentsDir, f)).isFile(); } catch { return false; } - }); + }).sort(); // #3706: sorted like the codex and + // opencode branches — readdir order is platform-dependent, so leaving it unsorted makes the + // reported `changes` ordering differ across machines for identical inputs. + const changes: EffortSyncChange[] = []; let synced = 0; let skipped = 0; + // Local-only counter: reads AND writes are both guarded in this loop (an + // unreadable or unwritable agent file must not abort the whole sweep), but + // this result shape (`{synced, skipped, changes, dry_run, agents_dir}`) is + // long-standing and widely consumed, so it deliberately gains NO new key + // (no `read_failures`/`write_failures`, unlike the codex/opencode branches + // below). Instead every per-file failure — read or write — is folded into + // `skipped` and rides the raw-mode summary token below — `output()`'s + // third argument is never merged into the emitted JSON object (see io.cts + // `output()`: it is only read when `raw === true`, entirely replacing the + // JSON payload), so flipping it to `'failed'` costs nothing in the wire + // shape while still surfacing the failure to a raw-mode caller. The three + // branches differ on that reporting shape, but are now also consistent in + // HOW they publish: every write below goes through the same tmp-file + + // chmod + retryRenameSync atomic-publish sequence used by + // cmdEffortSyncCodex and cmdEffortSyncOpencode, so a fault mid-write can + // never leave an agent file truncated or empty. + let fileFailureCount = 0; for (const file of files) { const agentName = file.replace(/\.md$/, ''); const filePath = path.join(agentsDir, file); - const content = fs.readFileSync(filePath, 'utf8'); + let content: string; + try { + content = fs.readFileSync(filePath, 'utf8'); + } catch { + // An unreadable agent file must not abort the whole sweep. Deliberately + // NOT adding a new field here: this result shape (`{synced, skipped, + // changes, dry_run, agents_dir}`) is long-standing and widely consumed, + // so the failure is folded into `skipped` only, with no + // read_failures/write_failures list — see `fileFailureCount` above. + skipped++; + fileFailureCount++; + continue; + } // Resolve using install-time logic: home defaults merged with project config. const universalEffort = resolveInstallTimeEffort(effortCfg, agentName); @@ -850,16 +963,66 @@ function cmdEffortSync(cwd: string, raw: boolean, opts?: { dryRun?: boolean; con // drift and the sync re-added a hand-stripped key on every apply. A // present key under inherit is stripped, reported as {from, to: null}. if (universalEffort === 'inherit') { - // eslint-disable-next-line local/no-unbounded-quantifier -- same lazy `*?` bounded by the `^---$/m` closing anchor as the concrete-path fmMatch below; duplicated here so the inherit branch validates against the same frontmatter span the strip targets const fmMatchInherit = /^---\r?\n([\s\S]*?)^---\r?$/m.exec(content); if (!fmMatchInherit) { skipped++; continue; } + // Presence and value are distinct questions: `effort:` with an EMPTY + // value is a key that IS present but whose captured value is null (the + // `(.+?)` group requires at least one char). Deciding "already correct" + // from a null value alone is wrong here — it would leave an + // unresolvable `effort: null` key on disk forever. Test presence with + // its own regex, and only compare values once presence is known. + const effortPresentInherit = /^effort:/m.test(fmMatchInherit[1]); + if (!effortPresentInherit) { skipped++; continue; } const effortMatchInherit = /^effort:[ \t]*(.+?)[ \t]*$/m.exec(fmMatchInherit[1]); - if (!effortMatchInherit) { skipped++; continue; } - changes.push({ agent: agentName, from: effortMatchInherit[1], to: null }); - synced++; + // `effortPresentInherit` is guaranteed true here (checked above), so a + // failed value match means the key is present with an EMPTY value — + // report `''`, not `null`, so "present-but-empty" is never conflated + // with "absent" in the sync output. if (!dryRun) { - fs.writeFileSync(filePath, removeEffortFrontmatter(content)); + // Atomic publish AND mode preservation, same discipline as + // cmdEffortSyncCodex/cmdEffortSyncOpencode: write to a sibling tmp + // file, chmod it to match filePath's existing (masked) mode, then + // retryRenameSync it over the target so filePath is either the old + // bytes or the new ones, never half-written and never dropped to a + // default mode. On any failure the tmp file is unlinked (best-effort) + // and the write is reported (folded into `skipped`/`fileFailureCount`, + // no new field), not thrown, so the remaining agents still get + // processed. ONE failure path for this site — no nested try/catch. + const tmpPathInherit = `${filePath}.tmp.${process.pid}`; + // Stat filePath BEFORE the write so its mode can be passed at + // CREATION time — a plain `writeFileSync(tmpPath, data)` creates the + // tmp file at the default `0666 & ~umask` even when filePath is more + // restrictive. Best-effort only: a stat failure must not abort the + // sync, since the content write is what matters, not the mode. + let originalModeInherit: number | undefined; + try { + originalModeInherit = fs.statSync(filePath).mode & 0o7777; + } catch { /* non-fatal: fall back to writing without an explicit mode */ } + try { + fs.writeFileSync( + tmpPathInherit, + removeEffortFrontmatter(content), + originalModeInherit !== undefined ? { mode: originalModeInherit } : undefined, + ); + // Not redundant with the `mode` option above: `mode` only applies + // when the file is actually created (O_CREAT). A leftover tmp file + // from an earlier crashed run would be reused (truncated) at its + // OLD mode instead, and this chmod is what corrects that case. + // Best-effort only: a chmod failure must not abort the sync, since + // the content write is what matters, not the mode. + try { + if (originalModeInherit !== undefined) fs.chmodSync(tmpPathInherit, originalModeInherit); + } catch { /* non-fatal: proceed with default tmp-file mode */ } + retryRenameSync(tmpPathInherit, filePath); + } catch { + try { fs.unlinkSync(tmpPathInherit); } catch { /* already gone or never created */ } + skipped++; + fileFailureCount++; + continue; + } } + changes.push({ agent: agentName, from: effortMatchInherit ? effortMatchInherit[1] : '', to: null }); + synced++; continue; } @@ -868,24 +1031,79 @@ function cmdEffortSync(cwd: string, raw: boolean, opts?: { dryRun?: boolean; con const rendered = renderEffortForRuntime(runtime, universalEffort); const newEffortValue = rendered.value as string; - // eslint-disable-next-line local/no-unbounded-quantifier -- lazy `*?` bounded by the `^---$/m` closing anchor, no nested quantifier, measured linear to 5MB (no-closing-marker adversarial input) const fmMatch = /^---\r?\n([\s\S]*?)^---\r?$/m.exec(content); if (!fmMatch) { skipped++; continue; } + // Presence and value are distinct questions here too: `currentEffort` + // reads null both when the key is ABSENT and when it is present with an + // EMPTY value. `effortPresent` disambiguates those two for the reported + // `from` below (never `null` when the key is present but empty) — but it + // has no bearing on the skip check that follows: `newEffortValue` is + // never null on this path (guarded above), so an absent key already + // yields `currentEffort === null !== newEffortValue` without consulting + // presence separately. + const effortPresent = /^effort:/m.test(fmMatch[1]); const effortMatch = /^effort:[ \t]*(.+?)[ \t]*$/m.exec(fmMatch[1]); - const currentEffort = effortMatch ? effortMatch[1] : null; + // `null` (key absent) and `''` (key present, value empty) are distinct + // states `effortPresent` deliberately disambiguates — collapsing both to + // `null` here would make the reported `from` lie about which case fired. + const currentEffort = effortPresent ? (effortMatch ? effortMatch[1] : '') : null; if (currentEffort === newEffortValue) { skipped++; continue; } + if (!dryRun) { + // Atomic publish AND mode preservation, same discipline as + // cmdEffortSyncCodex/cmdEffortSyncOpencode: write to a sibling tmp + // file, chmod it to match filePath's existing (masked) mode, then + // retryRenameSync it over the target so filePath is either the old + // bytes or the new ones, never half-written and never dropped to a + // default mode. On any failure the tmp file is unlinked (best-effort) + // and the write is reported (folded into `skipped`/`fileFailureCount`, + // no new field), not thrown, so the remaining agents still get + // processed. ONE failure path for this site — no nested try/catch. + const tmpPathSet = `${filePath}.tmp.${process.pid}`; + // Stat filePath BEFORE the write so its mode can be passed at CREATION + // time — a plain `writeFileSync(tmpPath, data)` creates the tmp file + // at the default `0666 & ~umask` even when filePath is more + // restrictive. Best-effort only: a stat failure must not abort the + // sync, since the content write is what matters, not the mode. + let originalModeSet: number | undefined; + try { + originalModeSet = fs.statSync(filePath).mode & 0o7777; + } catch { /* non-fatal: fall back to writing without an explicit mode */ } + try { + fs.writeFileSync( + tmpPathSet, + setEffortFrontmatter(content, newEffortValue), + originalModeSet !== undefined ? { mode: originalModeSet } : undefined, + ); + // Not redundant with the `mode` option above: `mode` only applies + // when the file is actually created (O_CREAT). A leftover tmp file + // from an earlier crashed run would be reused (truncated) at its OLD + // mode instead, and this chmod is what corrects that case. + // Best-effort only: a chmod failure must not abort the sync, since + // the content write is what matters, not the mode. + try { + if (originalModeSet !== undefined) fs.chmodSync(tmpPathSet, originalModeSet); + } catch { /* non-fatal: proceed with default tmp-file mode */ } + retryRenameSync(tmpPathSet, filePath); + } catch { + try { fs.unlinkSync(tmpPathSet); } catch { /* already gone or never created */ } + skipped++; + fileFailureCount++; + continue; + } + } + changes.push({ agent: agentName, from: currentEffort, to: newEffortValue }); synced++; - - if (!dryRun) { - fs.writeFileSync(filePath, setEffortFrontmatter(content, newEffortValue)); - } } - output({ synced, skipped, changes, dry_run: dryRun, agents_dir: agentsDir }, raw, synced > 0 ? 'changed' : 'ok'); + output( + { synced, skipped, changes, dry_run: dryRun, agents_dir: agentsDir }, + raw, + fileFailureCount > 0 ? 'failed' : synced > 0 ? 'changed' : 'ok', + ); } /** One `{agent, field, from}` strip reported by {@link cmdEffortSyncCodex} — `to` is always omission (`null`). */ @@ -903,8 +1121,8 @@ interface CodexEffortSyncRefusal { reason: string; } -/** A write that failed mid-sync (fs fault), reported so the remaining agents still get processed. */ -interface CodexEffortSyncWriteFailure { +/** A read or write that failed mid-sync (fs fault), reported so the remaining agents still get processed. */ +interface EffortSyncFileFailure { agent: string; file: string; error: string; @@ -919,8 +1137,9 @@ interface CodexEffortSyncWriteFailure { * document is refused and reported, never partially rewritten (40-design.md * "Reconciliation" — parseCodexAgentToml is the STRICT half of the reader/ * writer split). Result shape is additive over the claude branch's - * `{synced, skipped, changes, dry_run, agents_dir}` — `refused` and - * `write_failures` are new fields, never a reshape of the existing ones. + * `{synced, skipped, changes, dry_run, agents_dir}` — `refused`, + * `write_failures`, and `read_failures` are new fields, never a reshape of + * the existing ones. */ function cmdEffortSyncCodex(raw: boolean, dryRun: boolean, configDir?: string): void { // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/unbound-method @@ -944,14 +1163,26 @@ function cmdEffortSyncCodex(raw: boolean, dryRun: boolean, configDir?: string): const changes: CodexEffortSyncChange[] = []; const refused: CodexEffortSyncRefusal[] = []; - const writeFailures: CodexEffortSyncWriteFailure[] = []; + const writeFailures: EffortSyncFileFailure[] = []; + const readFailures: EffortSyncFileFailure[] = []; let synced = 0; let skipped = 0; for (const file of files) { const agentName = file.replace(/\.toml$/, ''); const filePath = path.join(agentsDir, file); - const content = fs.readFileSync(filePath, 'utf8'); + let content: string; + try { + content = fs.readFileSync(filePath, 'utf8'); + } catch (err) { + // An unreadable agent file must not abort the whole sweep — mirrors the + // opencode branch's own read guard, reported under its own + // `read_failures` key so a caller can tell "never read" apart from + // "read but write failed". + skipped++; + readFailures.push({ agent: agentName, file: filePath, error: err instanceof Error ? err.message : String(err) }); + continue; + } const parsed = parseCodexAgentToml(content); if (!parsed.ok) { @@ -998,8 +1229,29 @@ function cmdEffortSyncCodex(raw: boolean, dryRun: boolean, configDir?: string): // bare fs.renameSync) carries the transient-Windows-lock retry per // DEFECT.WINDOWS-FS-OPS. const tmpPath = `${filePath}.tmp.${process.pid}`; + // Stat filePath BEFORE the write so the original mode is available to + // pass at creation time, not just at chmod time afterward — otherwise + // the tmp file is briefly created at the default `0666 & ~umask` + // (world-readable under a typical 022 umask) even when filePath is + // e.g. 0600, exposing its contents for the window between creation and + // chmod. Best-effort: a stat failure must not abort the sync, since the + // content write is what matters, not the mode. + let originalMode: number | undefined; try { - fs.writeFileSync(tmpPath, renderCodexAgentToml(doc)); + originalMode = fs.statSync(filePath).mode & 0o7777; + } catch { /* non-fatal: fall back to writing without an explicit mode */ } + try { + fs.writeFileSync(tmpPath, renderCodexAgentToml(doc), originalMode !== undefined ? { mode: originalMode } : undefined); + // Not redundant with the `mode` option above: `mode` only applies + // when the file is actually created (O_CREAT). A leftover tmp file + // from an earlier crashed run would be reused (truncated) at its OLD + // mode instead, and this chmod is what corrects that case. Mask off + // the file-type bits fs.statSync().mode carries (POSIX leaves + // chmod's handling of those unspecified); best-effort only, since + // the content write is what matters, not the mode. + try { + if (originalMode !== undefined) fs.chmodSync(tmpPath, originalMode); + } catch { /* non-fatal: proceed with default tmp-file mode */ } retryRenameSync(tmpPath, filePath); } catch (err) { // Reported, not thrown — the remaining agents still get processed. @@ -1016,9 +1268,183 @@ function cmdEffortSyncCodex(raw: boolean, dryRun: boolean, configDir?: string): } output( - { synced, skipped, changes, dry_run: dryRun, agents_dir: agentsDir, refused, write_failures: writeFailures }, + { synced, skipped, changes, dry_run: dryRun, agents_dir: agentsDir, refused, write_failures: writeFailures, read_failures: readFailures }, raw, - synced > 0 ? 'changed' : 'ok', + writeFailures.length > 0 || readFailures.length > 0 ? 'failed' : synced > 0 ? 'changed' : 'ok', + ); +} + +/** + * #3706 — the OpenCode branch of `cmdEffortSync`. Maintains the `variant:` + * frontmatter key install now bakes into every `~/.config/opencode/agents/ + * gsd-*.md` (or configDir-relative equivalent), mirroring exactly what + * install writes: a resolved universal effort clamped through + * `clampEffortForHost('opencode', ...)`. Null means the key must be ABSENT — + * #3533 (10d): an absent key is the correct state under `inherit`, and a + * level OpenCode does not accept must never be written, so both collapse to + * the same `target: null` and the same removal path. Result shape is + * additive over the claude branch, matching the CODEX branch's + * `{synced, skipped, changes, dry_run, agents_dir, write_failures}`. + */ +function cmdEffortSyncOpencode(cwd: string, raw: boolean, dryRun: boolean, configDir?: string): void { + // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/unbound-method + const { getGlobalConfigDir } = require('./runtime-homes.cjs') as { getGlobalConfigDir(runtime: string, explicitDir?: string | null): string }; + const agentsDir = path.join(configDir || getGlobalConfigDir('opencode'), 'agents'); + + if (!fs.existsSync(agentsDir)) { + output({ synced: 0, skipped: 0, changes: [], dry_run: dryRun, agents_dir: agentsDir, reason: 'agents directory not found' }, raw, ''); + return; + } + + // Skip symlinks — matches the claude branch's existing guard (only write + // regular files, never follow a symlink into clobbering its target). + const files = fs + .readdirSync(agentsDir) + .filter(f => { + if (!f.startsWith('gsd-') || !f.endsWith('.md')) return false; + try { return fs.lstatSync(path.join(agentsDir, f)).isFile(); } catch { return false; } + }) + .sort(); + + // Use install-time resolvers: they merge ~/.gsd/defaults.json with project + // config, matching the exact logic used when agents were originally + // installed. Resolved once, outside the loop, like the claude branch. + // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/unbound-method + const { readGsdEffectiveEffortConfig, resolveInstallTimeEffort } = require('./install-effort-resolver.cjs') as { + readGsdEffectiveEffortConfig(cwd: string): Record | null; + resolveInstallTimeEffort(cfg: Record | null, agentName: string): string; + }; + const effortCfg = readGsdEffectiveEffortConfig(cwd); + + // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/unbound-method + const { clampEffortForHost } = require('./model-catalog.cjs') as { clampEffortForHost(host: string, effort: string): string | null }; + + const changes: EffortSyncChange[] = []; + const writeFailures: EffortSyncFileFailure[] = []; + const readFailures: EffortSyncFileFailure[] = []; + let synced = 0; + let skipped = 0; + + for (const file of files) { + const agentName = file.replace(/\.md$/, ''); + const filePath = path.join(agentsDir, file); + let content: string; + try { + content = fs.readFileSync(filePath, 'utf8'); + } catch (err) { + // An unreadable agent file must not abort the whole sweep — degrade + // like the write path below does, and report it under its own + // `read_failures` key so a caller can tell "never read" apart from + // "read but write failed". + skipped++; + readFailures.push({ agent: agentName, file: filePath, error: err instanceof Error ? err.message : String(err) }); + continue; + } + + // `target === null` covers both "no effort configured" (inherit) and "a + // level OpenCode does not accept" — both must produce NO `variant:` key, + // exactly what install writes. + const universal = effortCfg ? resolveInstallTimeEffort(effortCfg, agentName) : null; + const target = universal ? clampEffortForHost('opencode', universal) : null; + + const fmMatch = /^---\r?\n([\s\S]*?)^---\r?$/m.exec(content); + if (!fmMatch) { skipped++; continue; } + + // Presence and value are distinct questions: `variant:` with an EMPTY + // value is a key that IS present but whose captured value is null (the + // `(.+?)` group requires at least one char). Deciding "already correct" + // from a null-vs-null comparison alone is wrong when target is also + // null — it would leave an unresolvable `variant: null` key on disk + // forever. Test presence with its own regex, and only compare values + // once presence is known. + const variantPresent = /^variant:/m.test(fmMatch[1]); + const variantMatch = /^variant:[ \t]*(.+?)[ \t]*$/m.exec(fmMatch[1]); + // `null` (key absent) and `''` (key present, value empty) are distinct + // states this code deliberately tracks via `variantPresent` above — a + // reported `from` that collapses both to `null` would make "no key" and + // "empty key" indistinguishable in the sync output, even though only one + // of them actually has a `variant:` line to remove. + const currentVariant = variantPresent ? (variantMatch ? variantMatch[1] : '') : null; + + if (target === null) { + if (!variantPresent) { skipped++; continue; } + } else if (variantPresent && currentVariant === target) { + skipped++; + continue; + } + + changes.push({ agent: agentName, from: currentVariant, to: target }); + synced++; + + if (!dryRun) { + // Atomic publish AND mode preservation, same discipline as + // cmdEffortSyncCodex above: write to a sibling tmp file, chmod it to + // match filePath's existing (masked) mode, then retryRenameSync it over + // the target so filePath is either the old bytes or the new ones, never + // half-written and never dropped to a default mode. On failure the + // write is reported, not thrown, so the remaining agents still get + // processed. + const tmpPath = `${filePath}.tmp.${process.pid}`; + // Stat filePath BEFORE the write so its mode can be passed at CREATION + // time — a plain `writeFileSync(tmpPath, data)` creates the tmp file at + // the default `0666 & ~umask` (world-readable under a typical 022 + // umask) even when filePath is e.g. 0600, exposing its contents for + // the window between creation and the chmod below. Mask off the + // file-type bits (e.g. S_IFREG 0o100000) that fs.statSync().mode + // carries alongside the permission bits — POSIX leaves chmod's + // handling of those bits unspecified, and the remote matrix runs Linux + // only (Darwin tolerating the full mode is not evidence it is safe + // there). Best-effort only: a stat failure must not abort the sync, + // since the content write is what matters, not the mode. + let originalMode: number | undefined; + try { + originalMode = fs.statSync(filePath).mode & 0o7777; + } catch { /* non-fatal: fall back to writing without an explicit mode */ } + try { + fs.writeFileSync( + tmpPath, + target === null ? removeFrontmatterKeyLine(content, 'variant') : setFrontmatterKeyLine(content, 'variant', target), + originalMode !== undefined ? { mode: originalMode } : undefined, + ); + // Not redundant with the `mode` option above: `mode` only applies + // when the file is actually created (O_CREAT). A leftover tmp file + // from an earlier crashed run would be reused (truncated) at its OLD + // mode instead, and this chmod is what corrects that case. + // Best-effort only: a chmod failure must not abort the sync, since + // the content write is what matters, not the mode. + try { + if (originalMode !== undefined) fs.chmodSync(tmpPath, originalMode); + } catch { /* non-fatal: proceed with default tmp-file mode */ } + retryRenameSync(tmpPath, filePath); + } catch (err) { + try { fs.unlinkSync(tmpPath); } catch { /* already gone or never created */ } + changes.pop(); + synced--; + skipped++; + writeFailures.push({ agent: agentName, file: filePath, error: err instanceof Error ? err.message : String(err) }); + continue; + } + } + } + + // Any failure — a write OR a read — must not report 'ok' or 'changed': + // either would hide that at least one agent's on-disk state is now unknown + // (unread) or unchanged despite being reported as a pending change (write + // failed after being pushed onto `changes`/`synced`). `write_failures` and + // `read_failures` take priority over the synced-count-derived summary below, + // even when other agents in the same run succeeded. + // + // Known limitation, deliberately not fixed here: `output()` only honors its + // third argument when `raw === true`, and this command's process always + // exits 0 regardless of the summary string — so `if gsd-tools effort sync; + // then` reads success in a shell even on a run where every write failed. + // Making the exit code reflect failure would be a CLI-contract change + // affecting all three cmdEffortSync* branches (claude, codex, opencode) and + // is out of scope for this fix. + output( + { synced, skipped, changes, dry_run: dryRun, agents_dir: agentsDir, write_failures: writeFailures, read_failures: readFailures }, + raw, + writeFailures.length > 0 || readFailures.length > 0 ? 'failed' : synced > 0 ? 'changed' : 'ok', ); } diff --git a/src/frontmatter.cts b/src/frontmatter.cts index 95f0696aa..6ae7785b2 100644 --- a/src/frontmatter.cts +++ b/src/frontmatter.cts @@ -378,6 +378,54 @@ function scalarNeedsDoubleQuoting(s: string): boolean { return false; } +/** + * #3706 — Does this value need double-quoting when written as an AGENT + * frontmatter scalar (`model:`, `variant:`)? + * + * Deliberately a superset of `scalarNeedsDoubleQuoting` rather than a second, + * competing predicate: that one answers "can this open a plain scalar safely", + * which is necessary but not sufficient for a value that must ROUND-TRIP as the + * exact string it went in as. Keeping both here is the point — they are two + * answers to one question and drift the moment they live apart. + * + * The extra clauses, each an observed mis-parse rather than a precaution: + * + * - a non-alphanumeric first character. `scalarNeedsDoubleQuoting` rejects the + * indicators that cannot OPEN a scalar, but YAML still resolves plenty of + * values that open legally: `~` and `.inf`/`.nan` become null and floats, + * and a leading sign or dot (`+1`, `-0`, `.5`) becomes a number. Requiring + * alphanumeric-first covers that whole family at once, and costs nothing: + * every real model ID and effort level starts alphanumeric. + * - a trailing `:` — `model: foo:` is read as a nested mapping key and fails + * the whole frontmatter with "bad indentation of a mapping entry". + * - a boolean/null word — YAML 1.1 readers resolve `no`/`y`/`off`/`null` to + * non-strings, so a variant named `no` arrives as `false`. + * - a numeric-looking value, including the YAML 1.1 sexagesimal form: `12:30` + * resolves to the integer 750, and `:` is legal mid-identifier here. + * - a date. `2026-08-25` starts alphanumeric and survives every clause above, + * yet YAML resolves it to a Date object rather than a string. + */ +const YAML_WORD_SCALAR_RE = /^(?:y|n|yes|no|true|false|on|off|null)$/i; +const YAML_NUMERIC_RE = /^(?:\d[\d_]*(?:\.[\d_]*)?(?:[eE][-+]?\d+)?|0[xXbBoO][0-9a-fA-F_]+|\d[\d_]*(?::[0-5]?\d)+(?:\.[\d_]*)?)$/; +// YAML 1.1 timestamp: a bare ymd, optionally followed by a time part. +const YAML_TIMESTAMP_RE = /^\d{4}-\d{1,2}-\d{1,2}(?:[Tt ].*)?$/; + +function agentScalarNeedsDoubleQuoting(s: string): boolean { + if (scalarNeedsDoubleQuoting(s)) return true; + if (!/^[A-Za-z0-9]/.test(s)) return true; + // A plain scalar ENDS at `: ` or ` #` wherever they appear — the base + // predicate only inspects the first character, because its question is + // whether the scalar can legally open. `a: b` makes the line a nested + // mapping (a parse error at this indent) and `a #b` silently truncates to + // `a`. Both were found by the round-trip property, not by inspection. + if (/:\s/.test(s) || /\s#/.test(s)) return true; + if (s.endsWith(':')) return true; + if (YAML_WORD_SCALAR_RE.test(s)) return true; + if (YAML_NUMERIC_RE.test(s)) return true; + if (YAML_TIMESTAMP_RE.test(s)) return true; + return false; +} + function reconstructFrontmatter(obj: Frontmatter): string { const lines: string[] = []; // #3257: read the full-line-comment channel (set by parseYamlRegion when comments @@ -938,6 +986,11 @@ function cmdFrontmatterValidate(cwd: string, filePath: string, schemaName: strin } export = { + // #3706: shared with the agent-frontmatter writers so a config-supplied + // `model:`/`variant:` value cannot break out of its scalar. Previously private + // here while those writers interpolated raw — one escaper, three call sites. + escapeDoubleQuoted, + agentScalarNeedsDoubleQuoting, extractFrontmatter, UNTERMINATED_KEY_THRESHOLD, // Additive alias (#644 prohibition-probe schema contract): the probe round-trip seam reads a diff --git a/src/model-catalog.cts b/src/model-catalog.cts index a6c598002..de56c8e2b 100644 --- a/src/model-catalog.cts +++ b/src/model-catalog.cts @@ -351,6 +351,33 @@ export interface RenderedEffortArgv { host: string; } +/** + * Clamp a universal effort level to what a host actually accepts, or null. + * + * #3706 — extracted from `renderEffortArgv` so the two effort CHANNELS can share + * one capability table without one pretending to be the other. `EFFORT_ARGV[host]` + * describes what levels the host understands (`supported` + `clamp`); whether that + * reaches the host as a CLI flag or as a baked frontmatter key is the caller's + * business. The install-time OpenCode `variant:` writer needs the former without + * the latter, and hardcoding `'argv'` at that call site to borrow this logic read + * as if the frontmatter key were gated on the invocation-time axis. It is not — + * claude declares `effortSurface: "argv"` and independently bakes an `effort:` key. + * + * Returns null for a level the host does not accept, which every caller treats as + * "emit nothing". `inherit` lands here too: per #3533 (10d) it is not a wire level + * on any runtime, so it is in no `supported` set and must never be written out. + */ +export function clampEffortForHost(host: string, universalEffort: string): string | null { + // Own-property lookup only: a plain `EFFORT_ARGV[host]` resolves `__proto__` + // and friends to inherited members that carry no clamp/render. + if (typeof host !== 'string' || !Object.prototype.hasOwnProperty.call(EFFORT_ARGV, host)) return null; + const spec = EFFORT_ARGV[host]; + if (!spec || typeof spec.clamp !== 'function') return null; + if (typeof universalEffort !== 'string' || universalEffort.length === 0) return null; + const clamped = spec.clamp(universalEffort); + return spec.supported.has(clamped) ? clamped : null; +} + /** * Render the invocation-time effort argument for a host. * @@ -369,12 +396,13 @@ export function renderEffortArgv( // (and `constructor`/`toString`) to inherited members, which are truthy but // carry no `clamp`/`render` — a hostile host id would throw instead of // degrading. The host id reaches here from a descriptor, i.e. untrusted JSON. - if (typeof host !== 'string' || !Object.prototype.hasOwnProperty.call(EFFORT_ARGV, host)) return empty; + // #3706: the clamp/supported half lives in clampEffortForHost so the + // install-time channel can reuse it; this function keeps the axis gate and + // the argv rendering. One table, one clamp, two channels. + const clamped = clampEffortForHost(host, universalEffort); + if (clamped === null) return empty; const spec = EFFORT_ARGV[host]; - if (!spec || typeof spec.clamp !== 'function' || typeof spec.render !== 'function') return empty; - if (typeof universalEffort !== 'string' || universalEffort.length === 0) return empty; - const clamped = spec.clamp(universalEffort); - if (!spec.supported.has(clamped)) return empty; + if (typeof spec.render !== 'function') return empty; return { argv: spec.render(clamped), value: clamped, host }; } diff --git a/src/runtime-artifact-conversion.cts b/src/runtime-artifact-conversion.cts index f3e412583..32f8d1115 100644 --- a/src/runtime-artifact-conversion.cts +++ b/src/runtime-artifact-conversion.cts @@ -35,6 +35,7 @@ const { getDirName } = runtimeNamePolicy; import capabilityRegistry = require('./capability-registry.cjs'); import hostIntegration = require('./host-integration.cjs'); import { posixNormalize } from './shell-command-projection.cjs'; +import frontmatterModule = require('./frontmatter.cjs'); import { escapeRegex as escapeRegExp } from './pattern.cjs'; import { scanFencedBlocks } from './markdown-sectionizer.cjs'; // #2870: install-scope.cts is a leaf-tier sibling (imports only @@ -1828,7 +1829,27 @@ function neutralizeAgentReferences(content, instructionFile) { return c; } -function convertClaudeToOpencodeFrontmatter(content, { isAgent = false, modelOverride = null } = {}) { +/** + * Render one frontmatter `key: value` line whose value came from user config. + * + * #3706: both `model:` and `variant:` interpolate a value read from + * `.planning/config.json` / `~/.gsd/defaults.json`. Raw interpolation lets a value + * containing a newline inject additional TOP-LEVEL frontmatter keys into the + * generated agent file — proven by execution during the #3705 security review + * (`"sonnet\ntools: [\"*\"]\npermission: bypass"` produced two extra keys). + * + * That sink predates #3706, but this change adds a SECOND write to it, so it is + * closed here rather than doubled. Both the decision and the escaping live in + * frontmatter.cts so there is exactly one set of YAML scalar rules; a value that + * round-trips bare is still emitted bare, so generated files do not churn. + */ +function frontmatterScalar(key: string, value: string): string { + return frontmatterModule.agentScalarNeedsDoubleQuoting(value) + ? `${key} "${frontmatterModule.escapeDoubleQuoted(value)}"` + : `${key} ${value}`; +} + +function convertClaudeToOpencodeFrontmatter(content, { isAgent = false, modelOverride = null, variant = null } = {}) { // Replace tool name references in content (applies to all files) let convertedContent = content; convertedContent = convertedContent.replace(/\bAskUserQuestion\b/g, 'question'); @@ -1967,7 +1988,23 @@ function convertClaudeToOpencodeFrontmatter(content, { isAgent = false, modelOve // respected on OpenCode (which uses static agent frontmatter, not inline // Task() model parameters). See #2256. if (modelOverride) { - newLines.push(['model:', modelOverride].join(' ')); + newLines.push(frontmatterScalar('model:', modelOverride)); + } + // #3706: deliver the RESOLVED reasoning effort to the spawned subagent. + // `query resolve-model` reported an effort that never reached OpenCode, so + // every subagent ran at whatever opencode.jsonc defaults the model to — for + // a custom provider commonly the most expensive setting. + // + // OpenCode ONLY: `EFFORT_ARGV` declares surfaces for claude/opencode/codex + // and NO kilo entry, so the Kilo converter below deliberately does not emit + // this. Unlike the model side — where #2794 J8 requires kilo and opencode to + // resolve identically — there is no kilo effort surface to render into, and + // inventing one would emit a key that runtime never documented. + // + // Omitted entirely when absent, per #1156's rule for `model: inherit`: + // never an empty or sentinel value, let the runtime use its own default. + if (variant) { + newLines.push(frontmatterScalar('variant:', variant)); } } @@ -2155,7 +2192,7 @@ function convertClaudeToKiloFrontmatter(content, { isAgent = false, modelOverrid // model emission exactly (#2093 UPGRADE 2 / ADR-1239; Kilo is an OpenCode // fork with the same static-frontmatter model constraint). See #2256. if (modelOverride) { - newLines.push(['model:', modelOverride].join(' ')); + newLines.push(frontmatterScalar('model:', modelOverride)); } newLines.push(...buildKiloAgentPermissionBlock(agentTools)); } diff --git a/src/runtime-artifact-layout.cts b/src/runtime-artifact-layout.cts index c033463cf..99f5a353f 100644 --- a/src/runtime-artifact-layout.cts +++ b/src/runtime-artifact-layout.cts @@ -42,6 +42,10 @@ const conversionExports = runtimeArtifactConversion as Record & // rather than re-deriving the chain per runtime. // eslint-disable-next-line @typescript-eslint/no-require-imports import installModelOverrideResolver = require('./install-model-override-resolver.cjs'); +// eslint-disable-next-line @typescript-eslint/no-require-imports -- export= CommonJS module, same as the sibling resolver import above +import installEffortResolver = require('./install-effort-resolver.cjs'); +// eslint-disable-next-line @typescript-eslint/no-require-imports -- export= CommonJS module, same as the sibling resolver import above +import modelCatalog = require('./model-catalog.cjs'); import { posixNormalize } from './shell-command-projection.cjs'; // #2870: `isGlobalScope` centralizes the `scope === 'global'` boolean // projection both kind-builder closures below need at the converters' @@ -432,7 +436,7 @@ function convertedAgentsKind( // boolean `stageAgentsForRuntimeWithConverter`'s positional API // requires — see its doc comment in install-scope.cts. const rawConverter = _resolveNamedConverter(converterName, 'agents') as - (content: string, arg2?: boolean | { isAgent?: boolean; modelOverride?: string | null }) => string; + (content: string, arg2?: boolean | { isAgent?: boolean; modelOverride?: string | null; variant?: string | null }) => string; // #2875 Part 2 (J5-J8): kilo/opencode agent converters take an options // bag (`{isAgent, modelOverride}`), not the `isGlobal` boolean every @@ -449,11 +453,40 @@ function convertedAgentsKind( const overrideTargetDir = agentCtx?.targetDir ?? configDir; const modelOverrides = installModelOverrideResolver.readGsdEffectiveModelOverrides(overrideTargetDir); const runtimeResolver = installModelOverrideResolver.readGsdRuntimeProfileResolver(overrideTargetDir); + // #3706: the resolved reasoning effort, threaded exactly as the model is — + // config read ONCE per stage(), per-agent value resolved per file. + // + // Gated on the effort config being PRESENT, not on a value coming back. + // `resolveInstallTimeEffort` always returns something (measured: 'high' + // even with no project config and no effort block), so "skip when + // resolution yields no value" has no trigger and would stamp `variant:` + // into every generated agent file for every existing install. OpenCode's + // built-in variant sets are provider-specific upstream (Anthropic ships + // only `high`/`max`), so a level GSD resolved is not guaranteed to name a + // variant the user's provider actually has — emitting one unasked-for is + // the risk this gate avoids. #1156's rule for `model: inherit` is the + // precedent: do not emit a key the runtime may not understand. + // + // OpenCode only; the kilo converter ignores the field (no EFFORT_ARGV.kilo). + const effortConfig = converterName === 'convertClaudeToOpencodeFrontmatter' + ? installEffortResolver.readGsdEffectiveEffortConfig(overrideTargetDir) + : null; converter = (content, _isGlobal, meta) => { const modelOverride = meta ? installModelOverrideResolver.resolveAgentModelOverride(meta.agentName, modelOverrides, runtimeResolver) : null; - return rawConverter(content, { isAgent: true, modelOverride }); + // The universal level is NOT emitted raw. `clampEffortForHost` is the + // declared OpenCode effort capability (EFFORT_ARGV.opencode: its own + // `supported` set + `clamp`), and it is what rejects a level that is + // not a wire value — most importantly `inherit`, which per #3533 (10d) + // means "omit the key and follow the host default" and must never be + // written literally. Anything unsupported clamps to null, which omits + // the key rather than inventing one. + const universal = effortConfig && meta + ? installEffortResolver.resolveInstallTimeEffort(effortConfig, meta.agentName) + : null; + const variant = universal ? modelCatalog.clampEffortForHost('opencode', universal) : null; + return rawConverter(content, { isAgent: true, modelOverride, variant }); }; } else { // isGlobal is threaded so scope-aware agent converters (copilot, antigravity) diff --git a/tests/effort-sync-installed-runtime.test.cjs b/tests/effort-sync-installed-runtime.test.cjs index 3bf710c00..88dc88795 100644 --- a/tests/effort-sync-installed-runtime.test.cjs +++ b/tests/effort-sync-installed-runtime.test.cjs @@ -21,11 +21,18 @@ const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); +const os = require('node:os'); const { runNode } = require('./helpers/process-seam.cjs'); const { runMinimalInstall } = require('./helpers/install-shared.cjs'); const { cleanup } = require('./helpers.cjs'); +// Absolute path to the built module, spawned in a child process below so the +// JSON `cmdEffortSync` writes straight to fd 1 (via io.cjs's writeAllSync) can +// actually be captured — overriding process.stdout.write in-process does not +// intercept that write. +const COMMANDS_CJS = path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'commands.cjs'); + describe('#2071: effort sync runs in an installed runtime (no package-root bin/install.js)', () => { test('effort sync does not crash reaching for the un-shipped bin/install.js', () => { if (process.platform === 'win32') return; // install layout is POSIX-path-shaped @@ -65,3 +72,1182 @@ describe('#2071: effort sync runs in an installed runtime (no package-root bin/i } }); }); + +// #3706 — OpenCode's `effort sync` path (cmdEffortSyncOpencode) maintains the +// `variant:` frontmatter key install bakes into `~/.config/opencode/agents/ +// gsd-*.md`, mirroring the pre-existing claude/`effort:` and codex branches. +// Each case is run against a fresh sandbox: a project `cwd` (holding +// `.planning/config.json`), a `configDir` (holding `agents/`), and an +// isolated `home` (HOME env, so `~/.gsd/defaults.json` can never leak in from +// the real developer machine) — cmdEffortSync merges home defaults with the +// project config, so a hermetic test must control both sources. +describe('#3706: effort sync maintains OpenCode variant: frontmatter', () => { + function makeSandbox() { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-effort-sync-opencode-')); + const cwd = path.join(root, 'project'); + const configDir = path.join(root, 'runtime-home'); + const agentsDir = path.join(configDir, 'agents'); + const home = path.join(root, 'home'); + fs.mkdirSync(cwd, { recursive: true }); + fs.mkdirSync(agentsDir, { recursive: true }); + fs.mkdirSync(home, { recursive: true }); + return { root, cwd, configDir, agentsDir, home }; + } + + function writeProjectEffortConfig(cwd, effort) { + fs.mkdirSync(path.join(cwd, '.planning'), { recursive: true }); + fs.writeFileSync( + path.join(cwd, '.planning', 'config.json'), + JSON.stringify(effort === undefined ? {} : { effort }), + ); + } + + function writeAgent(agentsDir, name, lines) { + const filePath = path.join(agentsDir, name); + fs.writeFileSync(filePath, lines.join('\n')); + return filePath; + } + + /** + * Invokes `cmdEffortSync(cwd, false, opts)` in a CHILD process (required + * per module — see COMMANDS_CJS comment above) and parses the JSON result + * `output()` writes to fd 1. Callers that only need the on-disk effect of + * the sync (most cases below) still go through this so the harness stays + * single-shaped, but only cases 2 and 6 actually assert on the returned + * `result`. + */ + function runEffortSync({ cwd, home, configDir, runtime, dryRun }) { + const opts = { dryRun, configDir, runtime }; + const script = [ + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, false, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + const jsonStart = spawned.stdout.indexOf('{'); + assert.notStrictEqual(jsonStart, -1, `expected JSON output on stdout, got:\n${spawned.stdout}`); + return JSON.parse(spawned.stdout.slice(jsonStart)); + } + + test('writes the resolved variant into an agent that has none', () => { + // Protects: cmdEffortSyncOpencode's happy-path write — a fresh agent + // with no `variant:` key gets the resolved effort injected verbatim. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.match(content, /^variant: xhigh$/m); + } finally { + cleanup(root); + } + }); + + test('a synced agent keeps its original file mode', () => { + // The tmp-file + rename publish does not preserve mode the way an + // in-place writeFileSync would — cmdEffortSyncOpencode stat+chmods the + // tmp file before the rename specifically to compensate. Mode bits are + // not meaningful on Windows (no POSIX permission model), so skip there. + if (process.platform === 'win32') return; + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + fs.chmodSync(filePath, 0o600); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.match(content, /^variant: xhigh$/m, 'the sync must have actually rewritten the file'); + assert.strictEqual( + fs.statSync(filePath).mode & 0o777, 0o600, + 'the published file must keep the original file mode', + ); + } finally { + cleanup(root); + } + }); + + test('the temp file is never more permissive than the agent it replaces', () => { + // A plain `writeFileSync(tmpPath, data)` creates the tmp file at the + // default `0666 & ~umask`, then only tightens it afterward via chmod — + // for a 0600 agent that briefly leaves its contents world-readable + // inside agents/. Assert the mode is correct at CREATION, not just + // after the later chmod, by monkeypatching fs.writeFileSync in the + // child process to record the on-disk mode immediately after the real + // write runs. Mode bits are not meaningful on Windows, so skip there. + if (process.platform === 'win32') return; + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + fs.chmodSync(filePath, 0o600); + + const opts = { dryRun: false, configDir, runtime: 'opencode' }; + const script = [ + "const fs = require('node:fs');", + 'const captures = [];', + 'const originalWriteFileSync = fs.writeFileSync;', + 'fs.writeFileSync = function (targetPath, data, options) {', + ' const result = originalWriteFileSync.call(fs, targetPath, data, options);', + ' if (typeof targetPath === "string" && /\\.tmp\\.\\d+$/.test(targetPath)) {', + ' captures.push({ options: options || null, modeAfterWrite: fs.statSync(targetPath).mode & 0o777 });', + ' }', + ' return result;', + '};', + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, false, ${JSON.stringify(opts)});`, + 'process.stdout.write("###CAPTURE_START###" + JSON.stringify(captures) + "###CAPTURE_END###");', + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `capture child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + const match = /###CAPTURE_START###([\s\S]*?)###CAPTURE_END###/.exec(spawned.stdout); + assert.ok(match, `expected capture marker on stdout, got:\n${spawned.stdout}`); + const captures = JSON.parse(match[1]); + assert.strictEqual(captures.length, 1, `expected exactly one tmp-file write, got:\n${JSON.stringify(captures)}`); + + // The tmp file's mode immediately after creation must already match + // the target agent's 0600, not the default 0644 a bare writeFileSync + // would produce. + assert.strictEqual( + captures[0].modeAfterWrite, 0o600, + 'the tmp file must never be created more permissive than the agent it replaces', + ); + + const content = fs.readFileSync(filePath, 'utf8'); + assert.match(content, /^variant: xhigh$/m, 'the sync must have actually rewritten the file'); + assert.strictEqual(fs.statSync(filePath).mode & 0o777, 0o600, 'the published file must keep the original file mode'); + } finally { + cleanup(root); + } + }); + + test('reports the change without writing under dry run', () => { + // Protects: dry-run reports the pending change but leaves the file + // byte-identical — no write happens until dryRun is explicitly false. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const before = fs.readFileSync(filePath); + const result = runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: true }); + const after = fs.readFileSync(filePath); + assert.ok(before.equals(after), 'dry run must not touch the file on disk'); + assert.strictEqual(result.synced, 1); + assert.deepStrictEqual(result.changes[0], { agent: 'gsd-executor', from: null, to: 'xhigh' }); + } finally { + cleanup(root); + } + }); + + test('an already-correct agent is skipped, not rewritten', () => { + // Protects: an agent already carrying the resolved variant is reported + // skipped and its bytes are left completely untouched (no rewrite churn). + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'high' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', 'variant: high', '---', '', 'Body.', + ]); + const before = fs.readFileSync(filePath); + const result = runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const after = fs.readFileSync(filePath); + assert.ok(before.equals(after), 'an in-sync agent must not be rewritten'); + assert.strictEqual(result.skipped, 1); + assert.strictEqual(result.synced, 0); + } finally { + cleanup(root); + } + }); + + test('inherit strips the key rather than writing it literally', () => { + // #3533 (10d) — inherit means the key must not exist; writing + // `variant: inherit` would name a variant that cannot resolve. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'inherit' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', 'variant: high', '---', '', 'Body.', + ]); + const result = runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.doesNotMatch(content, /^variant:/m); + assert.match(content, /^name: gsd-executor$/m); + assert.match(content, /^description: x$/m); + assert.match(content, /^mode: subagent$/m); + assert.deepStrictEqual(result.changes[0], { agent: 'gsd-executor', from: 'high', to: null }); + } finally { + cleanup(root); + } + }); + + test('no effort config at all strips a stale key', () => { + // Matches what install bakes with no config: an agent that carries a + // stale `variant:` key from a prior sync must lose it, not keep it. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, undefined); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', 'variant: high', '---', '', 'Body.', + ]); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.doesNotMatch(content, /^variant:/m); + } finally { + cleanup(root); + } + }); + + test('a missing agents directory is reported, not thrown', () => { + // Protects: an install with no agents/ subdir under configDir must + // report the condition structurally, never throw out of cmdEffortSync. + const { root, cwd, configDir, home } = makeSandbox(); + try { + cleanup(path.join(configDir, 'agents')); + const result = runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + assert.strictEqual(result.reason, 'agents directory not found'); + } finally { + cleanup(root); + } + }); + + test('files that are not gsd-*.md are ignored', () => { + // Protects: the gsd-*.md filter — a non-gsd file sitting in agents/ must + // never be synced or skipped, i.e. never even enter the loop. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + writeAgent(agentsDir, 'not-gsd.md', [ + '---', 'name: not-gsd', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const result = runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + assert.strictEqual(result.synced, 0); + assert.strictEqual(result.skipped, 0); + } finally { + cleanup(root); + } + }); + + test('the document body is untouched', () => { + // Protects: the frontmatter line-editors only ever touch the frontmatter + // span — a body containing a colon, a `#`, and a `---` rule must survive + // byte-identical past the closing frontmatter delimiter. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'max' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', '---', '', + 'Body with: colons #hash and a --- rule.', '', 'more.', + ]); + const before = fs.readFileSync(filePath, 'utf8'); + const beforeBody = before.slice(before.indexOf('---', 3) + 3); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const after = fs.readFileSync(filePath, 'utf8'); + const afterBody = after.slice(after.indexOf('---', 3) + 3); + assert.strictEqual(afterBody, beforeBody); + assert.match(after, /^variant: max$/m); + } finally { + cleanup(root); + } + }); + + test('a key present with an empty value is removed, not mistaken for absent', () => { + // The value regex's `(.+?)` requires at least one character, so a bare + // `variant:` line (zero-width value) used to read as "key absent" rather + // than "key present, value empty" — presence and value are now distinct + // questions, and a stale empty-valued key must still be stripped when no + // effort config resolves one. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, undefined); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', 'variant:', '---', '', 'Body.', + ]); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.doesNotMatch(content, /^variant:/m); + } finally { + cleanup(root); + } + }); + + test('a key present with a whitespace-only value is removed too (the spelling that always worked)', () => { + // Trailing whitespace after the colon always matched the old regex (it is + // not zero-width), so this spelling never exhibited the absent-vs-empty + // bug above — pinned as the control that makes the bug look like a + // whitespace lottery rather than a real presence/value distinction. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, undefined); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', 'variant: ', '---', '', 'Body.', + ]); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.doesNotMatch(content, /^variant:/m); + } finally { + cleanup(root); + } + }); + + test('CRLF agents keep their line endings through a write and a strip', () => { + // Key-parameterising the frontmatter editors (claude's effort: vs + // opencode's variant:) is exactly the kind of change that would regress + // CRLF handling if the line ending were baked in per-key rather than + // detected from the matched frontmatter block. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = path.join(agentsDir, 'gsd-executor.md'); + const beforeWrite = [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ].join('\r\n'); + fs.writeFileSync(filePath, beforeWrite); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const afterWrite = fs.readFileSync(filePath, 'utf8'); + assert.match(afterWrite, /^variant: xhigh\r$/m); + assert.doesNotMatch(afterWrite, /(? { + // A symlinked gsd-*.md must never have its TARGET file rewritten — the + // sync must skip it structurally (readdir's Dirent reports a symlink + // entry as not-a-file), not follow it and edit whatever it points to. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + const targetPath = path.join(root, 'outside-target.md'); + const targetContent = [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ].join('\n'); + fs.writeFileSync(targetPath, targetContent); + const linkPath = path.join(agentsDir, 'gsd-executor.md'); + try { + fs.symlinkSync(targetPath, linkPath); + } catch { + return; // platform cannot create symlinks (e.g. unprivileged Windows) — skip + } + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const after = fs.readFileSync(targetPath, 'utf8'); + assert.strictEqual(after, targetContent, 'the symlink target must be left untouched'); + } finally { + cleanup(root); + } + }); + + test('a body line starting with variant: is never the line edited', () => { + // Scoping-by-content, not a whole-file /m replace: a body line that + // happens to start with `variant:` must survive untouched while the + // frontmatter gains its own key. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', + 'variant: in-the-body', '', 'more.', + ]); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.ok(content.includes('variant: in-the-body'), 'the body line must survive untouched'); + const fmEnd = content.indexOf('\n---', 4); + assert.match(content.slice(0, fmEnd), /^variant: xhigh$/m, 'the frontmatter must gain its own variant: line'); + } finally { + cleanup(root); + } + }); + + test('a file with no frontmatter at all is skipped, not corrupted', () => { + // No `---` fences at all: the sync must leave the file byte-identical + // rather than guessing where a frontmatter block would go. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + 'Just a body, no frontmatter fences at all.', + ]); + const before = fs.readFileSync(filePath); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const after = fs.readFileSync(filePath); + assert.ok(before.equals(after), 'a file with no frontmatter must be byte-identical afterward'); + } finally { + cleanup(root); + } + }); + + test('the claude path still writes effort:, not variant:', () => { + // The frontmatter line-editors were key-parameterised for #3706; this is + // the control that the pre-existing claude behavior did not move. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + runEffortSync({ cwd, home, configDir, runtime: 'claude', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.match(content, /^effort: xhigh$/m); + assert.doesNotMatch(content, /^variant:/m); + } finally { + cleanup(root); + } + }); + + test('a claude read failure reports failed without changing the result shape', () => { + // Protects: the claude branch of cmdEffortSync deliberately does NOT gain + // a `read_failures` key on a read failure — that result shape + // (`{synced, skipped, changes, dry_run, agents_dir}`) is long-standing + // and widely consumed. The failure is surfaced only through output()'s + // third argument (the raw-mode summary token), which is never merged + // into the JSON object. Asserts both halves: the raw token is 'failed', + // and the JSON result's key set is exactly the historic five, with no + // read_failures/write_failures key added. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-unreadable': 'xhigh' } }); + writeAgent(agentsDir, 'gsd-unreadable.md', [ + '---', 'name: gsd-unreadable', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + + const opts = { dryRun: false, configDir, runtime: 'claude' }; + const makeScript = raw => [ + `const fs = require('node:fs');`, + `const originalReadFileSync = fs.readFileSync;`, + `fs.readFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-unreadable.md')) {`, + ` throw new Error('injected read failure');`, + ` }`, + ` return originalReadFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, ${raw}, ${JSON.stringify(opts)});`, + ].join('\n'); + + const jsonRun = runNode(['-e', makeScript(false)], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + jsonRun.exitCode, 0, + `cmdEffortSync child process failed (outcome=${jsonRun.outcome}):\nstdout: ${jsonRun.stdout}\nstderr: ${jsonRun.stderr}`, + ); + const jsonStart = jsonRun.stdout.indexOf('{'); + assert.notStrictEqual(jsonStart, -1, `expected JSON output on stdout, got:\n${jsonRun.stdout}`); + const result = JSON.parse(jsonRun.stdout.slice(jsonStart)); + assert.strictEqual(result.skipped, 1); + assert.deepStrictEqual( + Object.keys(result).sort(), + ['agents_dir', 'changes', 'dry_run', 'skipped', 'synced'], + 'the claude result shape must gain no read_failures/write_failures key', + ); + + const rawRun = runNode(['-e', makeScript(true)], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + rawRun.exitCode, 0, + `cmdEffortSync child process failed (outcome=${rawRun.outcome}):\nstdout: ${rawRun.stdout}\nstderr: ${rawRun.stderr}`, + ); + assert.strictEqual(rawRun.stdout.trim(), 'failed', `expected the raw summary token 'failed', got:\n${rawRun.stdout}`); + } finally { + cleanup(root); + } + }); + + test('a claude write failure is reported and does not abort the sweep', () => { + // Protects: the claude branch's fs.writeFileSync (both the inherit/strip + // call site and the concrete/set call site) used to sit outside any try, + // so a single unwritable agent file threw and aborted the entire sweep. + // Injected deterministically by monkeypatching fs.writeFileSync inside + // the child script for exactly ONE agent's path — per this repo's + // CLAUDE.md §4, chmod-based injection is forbidden (root bypasses mode + // bits under Docker/CI and it leaks resources). + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-broken': 'xhigh', 'gsd-executor': 'xhigh' } }); + const brokenPath = writeAgent(agentsDir, 'gsd-broken.md', [ + '---', 'name: gsd-broken', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const okPath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const brokenBefore = fs.readFileSync(brokenPath); + + const opts = { dryRun: false, configDir, runtime: 'claude' }; + const makeScript = raw => [ + `const fs = require('node:fs');`, + `const originalWriteFileSync = fs.writeFileSync;`, + `fs.writeFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-broken.md')) {`, + ` throw new Error('injected write failure');`, + ` }`, + ` return originalWriteFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, ${raw}, ${JSON.stringify(opts)});`, + ].join('\n'); + + const jsonRun = runNode(['-e', makeScript(false)], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + jsonRun.exitCode, 0, + `cmdEffortSync child process failed (outcome=${jsonRun.outcome}):\nstdout: ${jsonRun.stdout}\nstderr: ${jsonRun.stderr}`, + ); + const jsonStart = jsonRun.stdout.indexOf('{'); + assert.notStrictEqual(jsonStart, -1, `expected JSON output on stdout, got:\n${jsonRun.stdout}`); + const result = JSON.parse(jsonRun.stdout.slice(jsonStart)); + assert.strictEqual(result.skipped, 1); + assert.deepStrictEqual( + Object.keys(result).sort(), + ['agents_dir', 'changes', 'dry_run', 'skipped', 'synced'], + 'the claude result shape must gain no read_failures/write_failures key', + ); + + const brokenAfter = fs.readFileSync(brokenPath); + assert.ok(brokenBefore.equals(brokenAfter), 'the failing agent file must be byte-unchanged'); + + const okContent = fs.readFileSync(okPath, 'utf8'); + assert.match(okContent, /^effort: xhigh$/m, 'the sweep must continue past the failing agent'); + + const rawRun = runNode(['-e', makeScript(true)], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + rawRun.exitCode, 0, + `cmdEffortSync child process failed (outcome=${rawRun.outcome}):\nstdout: ${rawRun.stdout}\nstderr: ${rawRun.stderr}`, + ); + assert.strictEqual(rawRun.stdout.trim(), 'failed', `expected the raw summary token 'failed', got:\n${rawRun.stdout}`); + } finally { + cleanup(root); + } + }); + + test('a claude agent survives a fault mid-publish', () => { + // Protects: the claude branch now publishes atomically (tmp-write + + // chmod + retryRenameSync), same discipline as cmdEffortSyncOpencode. An + // ENOSPC-mid-write injected on the TMP path must never truncate or empty + // the real agent file — pre-fix, an in-place `writeFileSync(filePath, + // ...)` truncates via O_TRUNC before the fault, leaving the file empty. + // Injected deterministically by monkeypatching fs.writeFileSync inside + // the child script for exactly the `.tmp.` path — per this repo's + // CLAUDE.md §4, chmod-based injection is forbidden (root bypasses mode + // bits under Docker/CI and it leaks resources). + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const before = fs.readFileSync(filePath); + + const opts = { dryRun: false, configDir, runtime: 'claude' }; + const makeScript = raw => [ + `const fs = require('node:fs');`, + `const originalWriteFileSync = fs.writeFileSync;`, + // cmdEffortSync's claude branch writes to `.tmp.` + // before renaming over the real path — throw only for that tmp + // write, simulating an ENOSPC fault after the original file has + // already been truncated by a naive in-place write (which this + // atomic-publish path no longer performs). + `fs.writeFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-executor.md.tmp.')) {`, + ` throw new Error('injected ENOSPC mid-write');`, + ` }`, + ` return originalWriteFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, ${raw}, ${JSON.stringify(opts)});`, + ].join('\n'); + + const jsonRun = runNode(['-e', makeScript(false)], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + jsonRun.exitCode, 0, + `cmdEffortSync child process failed (outcome=${jsonRun.outcome}):\nstdout: ${jsonRun.stdout}\nstderr: ${jsonRun.stderr}`, + ); + + const after = fs.readFileSync(filePath); + assert.ok(before.equals(after), 'the original agent file must be byte-identical after a fault mid-publish'); + + const leftoverTmp = fs.readdirSync(agentsDir).filter(f => f.includes('.tmp.')); + assert.deepStrictEqual(leftoverTmp, [], `no orphan tmp file must remain in agents_dir, found: ${leftoverTmp.join(', ')}`); + + const rawRun = runNode(['-e', makeScript(true)], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + rawRun.exitCode, 0, + `cmdEffortSync child process failed (outcome=${rawRun.outcome}):\nstdout: ${rawRun.stdout}\nstderr: ${rawRun.stderr}`, + ); + assert.strictEqual(rawRun.stdout.trim(), 'failed', `expected the raw summary token 'failed', got:\n${rawRun.stdout}`); + } finally { + cleanup(root); + } + }); + + test('a claude sync preserves the agent file mode', () => { + // The tmp-file + rename publish does not preserve mode the way an + // in-place writeFileSync would — cmdEffortSync's claude branch + // stat+chmods the tmp file before the rename specifically to compensate, + // mirroring cmdEffortSyncOpencode. Mode bits are not meaningful on + // Windows (no POSIX permission model), so skip there. + if (process.platform === 'win32') return; + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + fs.chmodSync(filePath, 0o600); + runEffortSync({ cwd, home, configDir, runtime: 'claude', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.match(content, /^effort: xhigh$/m, 'the sync must have actually rewritten the file'); + assert.strictEqual( + fs.statSync(filePath).mode & 0o777, 0o600, + 'the published file must keep the original file mode', + ); + } finally { + cleanup(root); + } + }); + + test('effort values are written unquoted', () => { + // Control for the shared-escaping fix: setFrontmatterKeyLine now routes + // through the same agentScalarNeedsDoubleQuoting/escapeDoubleQuoted + // helpers the install-side frontmatterScalar writer uses, but every + // effort level the sync actually writes today is a plain scalar — the + // output must stay byte-identical (unquoted) across runtimes/levels. + for (const level of ['low', 'xhigh']) { + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': level } }); + const claudePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + runEffortSync({ cwd, home, configDir, runtime: 'claude', dryRun: false }); + const claudeContent = fs.readFileSync(claudePath, 'utf8'); + assert.match(claudeContent, new RegExp(`^effort: ${level}$`, 'm')); + assert.doesNotMatch(claudeContent, /^effort: "/m); + } finally { + cleanup(root); + } + + const { root: root2, cwd: cwd2, configDir: configDir2, agentsDir: agentsDir2, home: home2 } = makeSandbox(); + try { + writeProjectEffortConfig(cwd2, { agent_overrides: { 'gsd-executor': level } }); + const opencodePath = writeAgent(agentsDir2, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + runEffortSync({ cwd: cwd2, home: home2, configDir: configDir2, runtime: 'opencode', dryRun: false }); + const opencodeContent = fs.readFileSync(opencodePath, 'utf8'); + assert.match(opencodeContent, new RegExp(`^variant: ${level}$`, 'm')); + assert.doesNotMatch(opencodeContent, /^variant: "/m); + } finally { + cleanup(root2); + } + } + }); + + test('a write failure on one agent is reported and does not stop the sweep', () => { + // Protects: cmdEffortSyncOpencode's atomic tmp-write + rename can fail + // mid-sync (fs fault). The failure must be reported per-agent in + // write_failures, the remaining agents must still be processed, the + // failing agent's file must be byte-unchanged, and cmdEffortSync must + // never throw. Injected deterministically by monkeypatching + // fs.writeFileSync inside the child script — per this repo's CLAUDE.md + // §4, chmod-based injection is forbidden (root bypasses mode bits under + // Docker/CI and it leaks resources). + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-broken': 'xhigh', 'gsd-executor': 'xhigh' } }); + const brokenPath = writeAgent(agentsDir, 'gsd-broken.md', [ + '---', 'name: gsd-broken', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const okPath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const brokenBefore = fs.readFileSync(brokenPath); + + const opts = { dryRun: false, configDir, runtime: 'opencode' }; + const script = [ + `const fs = require('node:fs');`, + `const originalWriteFileSync = fs.writeFileSync;`, + // The tmp-write + rename publish (cmdEffortSyncOpencode) writes to + // `.tmp.` before renaming over the real path — throw + // only for that one agent's tmp file, so every other write (and any + // fs machinery the require chain itself performs) passes through + // unmodified. + `fs.writeFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-broken.md.tmp.')) {`, + ` throw new Error('injected write failure');`, + ` }`, + ` return originalWriteFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, false, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + const jsonStart = spawned.stdout.indexOf('{'); + assert.notStrictEqual(jsonStart, -1, `expected JSON output on stdout, got:\n${spawned.stdout}`); + const result = JSON.parse(spawned.stdout.slice(jsonStart)); + + assert.strictEqual(result.write_failures.length, 1); + assert.strictEqual(result.write_failures[0].agent, 'gsd-broken'); + + const brokenAfter = fs.readFileSync(brokenPath); + assert.ok(brokenBefore.equals(brokenAfter), 'the failing agent file must be byte-unchanged'); + + const okContent = fs.readFileSync(okPath, 'utf8'); + assert.match(okContent, /^variant: xhigh$/m, 'the sweep must continue past the failing agent'); + } finally { + cleanup(root); + } + }); + + test('an unreadable agent file is reported and does not abort the sweep', () => { + // Protects: cmdEffortSyncOpencode's fs.readFileSync used to sit outside + // any try, so a single unreadable agent file threw and aborted the + // entire sweep, unlike the write path in the same loop which degrades + // into write_failures. Injected deterministically by monkeypatching + // fs.readFileSync inside the child script — per this repo's CLAUDE.md + // §4, chmod-based injection is forbidden (root bypasses mode bits under + // Docker/CI and it leaks resources). + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-unreadable': 'xhigh', 'gsd-executor': 'xhigh' } }); + writeAgent(agentsDir, 'gsd-unreadable.md', [ + '---', 'name: gsd-unreadable', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const okPath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + + const opts = { dryRun: false, configDir, runtime: 'opencode' }; + const script = [ + `const fs = require('node:fs');`, + `const originalReadFileSync = fs.readFileSync;`, + `fs.readFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-unreadable.md')) {`, + ` throw new Error('injected read failure');`, + ` }`, + ` return originalReadFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, false, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + const jsonStart = spawned.stdout.indexOf('{'); + assert.notStrictEqual(jsonStart, -1, `expected JSON output on stdout, got:\n${spawned.stdout}`); + const result = JSON.parse(spawned.stdout.slice(jsonStart)); + + assert.strictEqual(result.read_failures.length, 1); + assert.strictEqual(result.read_failures[0].agent, 'gsd-unreadable'); + + const okContent = fs.readFileSync(okPath, 'utf8'); + assert.match(okContent, /^variant: xhigh$/m, 'the sweep must continue past the unreadable agent'); + } finally { + cleanup(root); + } + }); + + test('the summary reports failed when an opencode write could not complete', () => { + // Protects: the raw-mode summary TOKEN cmdEffortSync passes as output()'s + // third argument (never merged into the JSON object — io.cts `output()` + // only reads it when `raw === true`, entirely replacing the JSON + // payload) must flip to 'failed' when a write_failures entry exists, even + // though the JSON result itself carries no such flag. Captured by + // running the child with `raw: true` and asserting on the literal stdout + // text `output()` writes for that mode, not on parsed JSON. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-broken': 'xhigh' } }); + writeAgent(agentsDir, 'gsd-broken.md', [ + '---', 'name: gsd-broken', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + + const opts = { dryRun: false, configDir, runtime: 'opencode' }; + const script = [ + `const fs = require('node:fs');`, + `const originalWriteFileSync = fs.writeFileSync;`, + `fs.writeFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-broken.md.tmp.')) {`, + ` throw new Error('injected write failure');`, + ` }`, + ` return originalWriteFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, true, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + assert.strictEqual(spawned.stdout.trim(), 'failed', `expected the raw summary token 'failed', got:\n${spawned.stdout}`); + } finally { + cleanup(root); + } + }); +}); + +// ADR-2313 D7 (#3243) — the Codex branch (cmdEffortSyncCodex) strips a stale +// Anthropic-flavored `model` pin (and its coupled `model_reasoning_effort`) +// from every installed `~/.codex/agents/.toml`, publishing via the same +// tmp-file + chmod + retryRenameSync discipline as the OpenCode branch above. +describe('#3243: effort sync strips Anthropic-flavored model pins from Codex agents', () => { + function makeSandbox() { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-effort-sync-codex-')); + const cwd = path.join(root, 'project'); + const configDir = path.join(root, 'runtime-home'); + const agentsDir = path.join(configDir, 'agents'); + const home = path.join(root, 'home'); + fs.mkdirSync(cwd, { recursive: true }); + fs.mkdirSync(agentsDir, { recursive: true }); + fs.mkdirSync(home, { recursive: true }); + return { root, cwd, configDir, agentsDir, home }; + } + + function writeAgent(agentsDir, name, lines) { + const filePath = path.join(agentsDir, name); + fs.writeFileSync(filePath, lines.join('\n')); + return filePath; + } + + test('a synced codex agent keeps its original file mode', () => { + // The tmp-file + rename publish does not preserve mode the way an + // in-place writeFileSync would — cmdEffortSyncCodex stat+chmods the tmp + // file before the rename specifically to compensate, same discipline as + // the OpenCode branch. Mode bits are not meaningful on Windows, so skip + // there. The reachable rewrite path is the Anthropic-model-strip: a + // `.toml` agent carrying an Anthropic-flavored `model` value. + if (process.platform === 'win32') return; + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + const before = ['model = "claude-sonnet-4"', 'model_reasoning_effort = "high"', ''].join('\n'); + const filePath = writeAgent(agentsDir, 'gsd-executor.toml', [before]); + fs.chmodSync(filePath, 0o600); + + const opts = { dryRun: false, configDir, runtime: 'codex' }; + const script = [ + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, false, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + + const after = fs.readFileSync(filePath, 'utf8'); + assert.notStrictEqual(after, before, 'the sync must have actually rewritten the file'); + assert.doesNotMatch(after, /claude/i, 'the Anthropic-flavored model pin must have been stripped'); + assert.strictEqual( + fs.statSync(filePath).mode & 0o777, 0o600, + 'the published file must keep the original file mode', + ); + } finally { + cleanup(root); + } + }); + + test('an unreadable codex agent is reported and does not abort the sweep', () => { + // Protects: cmdEffortSyncCodex's fs.readFileSync used to sit outside any + // try, so a single unreadable agent file would throw and abort the + // entire sweep instead of degrading into read_failures like the write + // path in the same loop. Injected deterministically by monkeypatching + // fs.readFileSync inside the child script — per this repo's CLAUDE.md + // §4, chmod-based injection is forbidden (root bypasses mode bits under + // Docker/CI and it leaks resources). The Anthropic-flavored `model` pin + // is the reachable codex rewrite path (verified by execution above), so + // the sibling agent uses that fixture to prove it still gets rewritten. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeAgent(agentsDir, 'gsd-unreadable.toml', [ + 'model = "claude-sonnet-4"', 'model_reasoning_effort = "high"', '', + ]); + const okPath = writeAgent(agentsDir, 'gsd-ok.toml', [ + 'model = "claude-sonnet-4"', 'model_reasoning_effort = "high"', '', + ]); + const okBefore = fs.readFileSync(okPath, 'utf8'); + + const opts = { dryRun: false, configDir, runtime: 'codex' }; + const script = [ + `const fs = require('node:fs');`, + `const originalReadFileSync = fs.readFileSync;`, + `fs.readFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-unreadable.toml')) {`, + ` throw new Error('injected read failure');`, + ` }`, + ` return originalReadFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, false, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + const jsonStart = spawned.stdout.indexOf('{'); + assert.notStrictEqual(jsonStart, -1, `expected JSON output on stdout, got:\n${spawned.stdout}`); + const result = JSON.parse(spawned.stdout.slice(jsonStart)); + + assert.strictEqual(result.read_failures.length, 1); + assert.strictEqual(result.read_failures[0].agent, 'gsd-unreadable'); + + const okAfter = fs.readFileSync(okPath, 'utf8'); + assert.notStrictEqual(okAfter, okBefore, 'the sweep must continue past the unreadable agent and still rewrite the sibling'); + assert.doesNotMatch(okAfter, /claude/i, 'the sibling agent must have had its Anthropic-flavored model pin stripped'); + } finally { + cleanup(root); + } + }); + + test('the summary reports failed when a codex sync could not complete', () => { + // Protects: same raw-mode summary TOKEN contract as the opencode case + // above — output()'s third argument is never merged into the JSON + // object, so a raw-mode caller can only see the failure via this token. + // Captured the same way: run with `raw: true` and assert on the literal + // stdout text, not on parsed JSON. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeAgent(agentsDir, 'gsd-unreadable.toml', [ + 'model = "claude-sonnet-4"', 'model_reasoning_effort = "high"', '', + ]); + + const opts = { dryRun: false, configDir, runtime: 'codex' }; + const script = [ + `const fs = require('node:fs');`, + `const originalReadFileSync = fs.readFileSync;`, + `fs.readFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-unreadable.toml')) {`, + ` throw new Error('injected read failure');`, + ` }`, + ` return originalReadFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, true, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + assert.strictEqual(spawned.stdout.trim(), 'failed', `expected the raw summary token 'failed', got:\n${spawned.stdout}`); + } finally { + cleanup(root); + } + }); +}); + +// #3706 — setFrontmatterKeyLine / removeFrontmatterKeyLine are not exported +// directly; they are reached through the exported cmdEffortSync (claude +// runtime, `effort:` key), the only public entry point that exercises them. +describe('#3706: frontmatter line editors are scoped to the matched block', () => { + function makeSandbox() { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-effort-sync-fm-scope-')); + const cwd = path.join(root, 'project'); + const configDir = path.join(root, 'runtime-home'); + const agentsDir = path.join(configDir, 'agents'); + const home = path.join(root, 'home'); + fs.mkdirSync(cwd, { recursive: true }); + fs.mkdirSync(agentsDir, { recursive: true }); + fs.mkdirSync(home, { recursive: true }); + return { root, cwd, configDir, agentsDir, home }; + } + + function writeProjectEffortConfig(cwd, override) { + fs.mkdirSync(path.join(cwd, '.planning'), { recursive: true }); + fs.writeFileSync( + path.join(cwd, '.planning', 'config.json'), + JSON.stringify({ effort: { agent_overrides: { 'gsd-executor': override } } }), + ); + } + + function runEffortSync({ cwd, home, configDir }) { + const opts = { dryRun: false, configDir, runtime: 'claude' }; + const script = [ + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, false, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + } + + test('a CRLF document with a preamble keeps its opening fence intact', () => { + // Pre-fix: openLen was derived from `/^---\r\n/.test(content)` (start of + // file, which here is "Preamble line", not CRLF) rather than from the + // matched frontmatter block, so the CRLF fence misaligned by one byte. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, 'inherit'); + const filePath = path.join(agentsDir, 'gsd-executor.md'); + const before = 'Preamble line\r\n\r\n---\r\nname: x\r\neffort: high\r\n---\r\n\r\nBody.\r\n'; + fs.writeFileSync(filePath, before); + runEffortSync({ cwd, home, configDir }); + const after = fs.readFileSync(filePath, 'utf8'); + assert.strictEqual(after, 'Preamble line\r\n\r\n---\r\nname: x\r\n---\r\n\r\nBody.\r\n'); + assert.ok(after.includes('---\r\nname: x\r\n'), 'the CRLF opening fence must survive intact'); + assert.ok(after.startsWith('Preamble line\r\n\r\n'), 'preamble must survive untouched'); + assert.ok(after.endsWith('\r\n\r\nBody.\r\n'), 'body must survive untouched'); + assert.doesNotMatch(after, /^-{1,2}\r?\n/m, 'no truncated/stray fence'); + } finally { + cleanup(root); + } + }); + + test('a preamble line starting with the key is not the line rewritten', () => { + // Pre-fix: `content.replace(keyLineRe, ...)` was a whole-file /m replace + // gated only on the key being present in fmBody, so the FIRST matching + // line in the whole file (the preamble) was rewritten instead of the + // frontmatter line. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, 'xhigh'); + const filePath = path.join(agentsDir, 'gsd-executor.md'); + const before = 'effort: not-the-frontmatter\n\n---\nname: x\neffort: high\n---\n\nBody.\n'; + fs.writeFileSync(filePath, before); + runEffortSync({ cwd, home, configDir }); + const after = fs.readFileSync(filePath, 'utf8'); + assert.strictEqual(after, 'effort: not-the-frontmatter\n\n---\nname: x\neffort: xhigh\n---\n\nBody.\n'); + // Bytes-based scoping (not a whole-file regex): the preamble line must + // precede the first `---` fence, and only the line AFTER that fence may + // read `effort: xhigh`. + const firstFence = after.indexOf('---'); + assert.ok(after.slice(0, firstFence).includes('effort: not-the-frontmatter'), 'preamble line must be untouched'); + assert.ok(after.slice(firstFence).includes('effort: xhigh'), 'frontmatter line must carry the new value'); + } finally { + cleanup(root); + } + }); + + test('a document whose frontmatter starts at byte 0 is byte-identical to before', () => { + // No-regression control: a normal install-written agent (frontmatter at + // byte 0, LF) must see only the one targeted line change, nothing else. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, 'xhigh'); + const filePath = path.join(agentsDir, 'gsd-executor.md'); + const before = ['---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.'].join('\n'); + fs.writeFileSync(filePath, before); + runEffortSync({ cwd, home, configDir }); + const afterSet = fs.readFileSync(filePath, 'utf8'); + assert.strictEqual( + afterSet, + ['---', 'name: gsd-executor', 'description: x', 'mode: subagent', 'effort: xhigh', '---', '', 'Body.'].join('\n'), + ); + + writeProjectEffortConfig(cwd, 'inherit'); + const beforeRemove = ['---', 'name: gsd-executor', 'description: x', 'mode: subagent', 'effort: high', '---', '', 'Body.'].join('\n'); + fs.writeFileSync(filePath, beforeRemove); + runEffortSync({ cwd, home, configDir }); + const afterRemove = fs.readFileSync(filePath, 'utf8'); + assert.strictEqual( + afterRemove, + ['---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.'].join('\n'), + ); + } finally { + cleanup(root); + } + }); + + test('a duplicated key converges to one occurrence on a single set', () => { + // Protects: setFrontmatterKeyLine used to rewrite only the FIRST + // occurrence of a duplicated key, leaving a stale second occurrence in + // place. A last-wins YAML reader would then honour the stale value while + // this function's own first-occurrence read reports "in sync" — a + // permanently non-converging state. One run must collapse a duplicated + // key to exactly one occurrence, carrying the new value, in the position + // of the FIRST occurrence. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, 'xhigh'); + const filePath = path.join(agentsDir, 'gsd-executor.md'); + const before = [ + '---', 'name: gsd-executor', 'effort: low', 'description: x', 'effort: high', '---', '', 'Body.', + ].join('\n'); + fs.writeFileSync(filePath, before); + runEffortSync({ cwd, home, configDir }); + const after = fs.readFileSync(filePath, 'utf8'); + assert.strictEqual( + after, + ['---', 'name: gsd-executor', 'effort: xhigh', 'description: x', '---', '', 'Body.'].join('\n'), + 'exactly one effort: line must remain, at the position of the first occurrence, carrying the resolved value', + ); + const occurrences = after.match(/^effort:/gm) || []; + assert.strictEqual(occurrences.length, 1, 'exactly one effort: line must remain'); + } finally { + cleanup(root); + } + }); +}); diff --git a/tests/frontmatter.unit.test.cjs b/tests/frontmatter.unit.test.cjs index 54e7d82dc..023674dc8 100644 --- a/tests/frontmatter.unit.test.cjs +++ b/tests/frontmatter.unit.test.cjs @@ -25,6 +25,8 @@ const { noOpObjectListSetError, parseMustHavesBlock, FRONTMATTER_SCHEMAS, + agentScalarNeedsDoubleQuoting, + escapeDoubleQuoted, } = require('../gsd-core/bin/lib/frontmatter.cjs'); // ─── extractFrontmatter ─────────────────────────────────────────────────────── @@ -1512,3 +1514,236 @@ describe('stripFrontmatter', () => { assert.strictEqual(stripFrontmatter(''), ''); }); }); + +// ─── agentScalarNeedsDoubleQuoting (#3706) ──────────────────────────────────── +// +// Mutant-killing discipline: every boolean clause gets a true case AND a +// near-miss false case one edit away, so flipping any operator/character +// class in the source changes at least one assertion here. + +describe('agentScalarNeedsDoubleQuoting: delegates to scalarNeedsDoubleQuoting', () => { + test('empty string needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting(''), true); + }); + test('embedded double quote needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a"b'), true); + }); + test('embedded backslash needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a\\b'), true); + }); + test('embedded control char needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a\u0001b'), true); + }); + test('leading whitespace needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting(' abc'), true); + }); + test('trailing whitespace needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('abc '), true); + }); + for (const c of ['#', '&', '*', '!', '|', '>', '%', '@', '`', '[', ']', '{', '}', ',', "'"]) { + test(`leading indicator "${c}" needs quoting`, () => { + assert.equal(agentScalarNeedsDoubleQuoting(`${c}foo`), true); + }); + } + test('near-miss: plain word does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('plainword'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: first character must be alphanumeric', () => { + for (const v of ['~', '.inf', '.nan', '+1', '-1', '-0', '.5']) { + test(`non-alphanumeric first char "${v}" needs quoting`, () => { + assert.equal(agentScalarNeedsDoubleQuoting(v), true); + }); + } + test('near-miss: alphanumeric-first with internal hyphen does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a-b'), false); + }); + test('near-miss: digit-first non-numeric word does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('0abc'), false); + }); + test('near-miss: alphanumeric-first with trailing hyphen does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('x-'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: trailing colon', () => { + test('a bare trailing colon needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('foo:'), true); + }); + test('near-miss: colon followed by more text does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('foo:bar'), false); + }); + test('near-miss: single-char key:value shape does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a:b'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: embedded ": " (colon + whitespace)', () => { + test('colon followed by space needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a: b'), true); + }); + test('colon followed by tab needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a:\tb'), true); + }); + test('near-miss: colon with no following whitespace does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a:b'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: embedded " #" (whitespace + hash)', () => { + test('space followed by hash needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a #b'), true); + }); + test('tab followed by hash needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a\t#b'), true); + }); + test('near-miss: hash with no preceding whitespace does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a#b'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: boolean/null words (case-insensitive)', () => { + for (const v of ['y', 'n', 'yes', 'no', 'true', 'false', 'on', 'off', 'null']) { + test(`lowercase word "${v}" needs quoting`, () => { + assert.equal(agentScalarNeedsDoubleQuoting(v), true); + }); + } + for (const v of ['YES', 'No', 'TRUE', 'Null']) { + test(`mixed-case word "${v}" needs quoting (case-insensitivity)`, () => { + assert.equal(agentScalarNeedsDoubleQuoting(v), true); + }); + } + test('near-miss: "yes1" is not an exact word match', () => { + assert.equal(agentScalarNeedsDoubleQuoting('yes1'), false); + }); + test('near-miss: "nope" is not an exact word match', () => { + assert.equal(agentScalarNeedsDoubleQuoting('nope'), false); + }); + test('near-miss: "nullish" is not an exact word match', () => { + assert.equal(agentScalarNeedsDoubleQuoting('nullish'), false); + }); + test('near-miss: "onward" is not an exact word match', () => { + assert.equal(agentScalarNeedsDoubleQuoting('onward'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: numeric-looking values', () => { + for (const v of [ + '1', '123', '1.5', '1.', '1e5', '1E5', '1e+5', '1e-5', '1_000', + '0x1F', '0b101', '0o17', '0X1f', '12:30', '1:2:3', '12:30.5', + ]) { + test(`numeric form "${v}" needs quoting`, () => { + assert.equal(agentScalarNeedsDoubleQuoting(v), true); + }); + } + test('near-miss: "1a" (digit then letter) does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('1a'), false); + }); + test('near-miss: "a1" (letter then digit) does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a1'), false); + }); + test('near-miss: "x1e5" (non-digit first char) does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('x1e5'), false); + }); + test('near-miss: "0xzz" (invalid hex digits) does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('0xzz'), false); + }); + // Verified against the real implementation: the sexagesimal group only + // consumes a leading [0-5]? then one mandatory digit per ":" segment, so + // "12:99" cannot fully match YAML_NUMERIC_RE (the second "9" is left over) + // and no other clause fires either. The code does NOT reject an + // out-of-range (60-99) minute-like group — pinned here as actual behavior, + // not the originally assumed "false because minutes must be 0-59". + test('"12:99" does not need quoting (sexagesimal regex cannot consume the trailing digit)', () => { + assert.equal(agentScalarNeedsDoubleQuoting('12:99'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: YAML timestamp', () => { + for (const v of [ + '2026-08-25', '2026-8-5', '2026-08-25T10:00:00Z', '2026-08-25 10:00:00', + ]) { + test(`timestamp form "${v}" needs quoting`, () => { + assert.equal(agentScalarNeedsDoubleQuoting(v), true); + }); + } + test('near-miss: "2026-08" (missing day) does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('2026-08'), false); + }); + // Verified against the real implementation: this string never reaches the + // timestamp clause at all — the pure-digit numeric clause (YAML_NUMERIC_RE's + // first alternative matches any \d[\d_]* string) fires first and returns + // true. So "20260825" IS true, but for a different reason than "looks like + // a date"; it does not exercise YAML_TIMESTAMP_RE. + test('"20260825" (no hyphens) needs quoting via the numeric clause, not the timestamp clause', () => { + assert.equal(agentScalarNeedsDoubleQuoting('20260825'), true); + }); + // This is the case that actually pins YAML_TIMESTAMP_RE's trailing + // `(?:[Tt ].*)?$` anchor: the numeric clause cannot match (hyphens present, + // no colon), so only the timestamp regex is left to decide, and it rejects + // trailing text that isn't introduced by "T"/"t"/" ". + test('"2026-08-25x" (trailing junk not preceded by T/t/space) does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('2026-08-25x'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: real-world values that must stay unquoted', () => { + for (const v of [ + 'sonnet', 'synthetic/hf:zai-org/GLM-5.2', 'gpt-5.6-luna', 'claude-opus-5', + 'high', 'xhigh', 'minimal', 'a.b', 'GLM-5.2', + ]) { + test(`"${v}" does not need quoting`, () => { + assert.equal(agentScalarNeedsDoubleQuoting(v), false); + }); + } +}); + +// ─── escapeDoubleQuoted (#1779 / #3497) ──────────────────────────────────────── + +describe('escapeDoubleQuoted: exact output strings', () => { + test('backslash is escaped before the quote it precedes (ordering matters)', () => { + // Input: a, \, ", b. If the quote were escaped BEFORE the backslash, the + // backslash added in front of the quote would then itself get doubled by + // a subsequent backslash pass, producing a different (wrong) string. The + // real order (backslash first, then quote) yields exactly 3 backslashes + // followed by the quote. + const input = 'a' + '\\' + '"' + 'b'; + const expected = 'a' + '\\'.repeat(3) + '"' + 'b'; + assert.equal(escapeDoubleQuoted(input), expected); + }); + + test('double quote alone', () => { + assert.equal(escapeDoubleQuoted('"'), '\\"'); + }); + + test('newline alone', () => { + assert.equal(escapeDoubleQuoted('\n'), '\\n'); + }); + + test('tab alone', () => { + assert.equal(escapeDoubleQuoted('\t'), '\\t'); + }); + + test('carriage return alone', () => { + assert.equal(escapeDoubleQuoted('\r'), '\\r'); + }); + + test('a C0 control char (0x01) becomes lowercase zero-padded \\xHH', () => { + assert.equal(escapeDoubleQuoted('\u0001'), '\\x01'); + }); + + test('DEL (0x7f) becomes \\x7f', () => { + assert.equal(escapeDoubleQuoted('\u007f'), '\\x7f'); + }); + + test('plain string with no specials is returned unchanged', () => { + assert.equal(escapeDoubleQuoted('plain'), 'plain'); + }); + + test('combined input exercising every escape in one pass', () => { + const input = 'a\\b"c\nd\te\rf\u0001g\u007fh'; + const expected = 'a\\\\b\\"c\\nd\\te\\rf\\x01g\\x7fh'; + assert.equal(escapeDoubleQuoted(input), expected); + }); +}); diff --git a/tests/mutation-matrix-ratchet.test.cjs b/tests/mutation-matrix-ratchet.test.cjs index bba8a90e5..51960461e 100644 --- a/tests/mutation-matrix-ratchet.test.cjs +++ b/tests/mutation-matrix-ratchet.test.cjs @@ -194,7 +194,7 @@ const RATCHET_BASELINE = { 'context-utilization': 80, 'context-composer': 66, // #2929: extracted from prompt-budget; same floor as prompt-budget 'prompt-budget': 66, // CI 68.33% 2026-06-14; was 90 (timeout-inflated local) - 'frontmatter': 62, + 'frontmatter': 65, // #3706: raised from 62; measured 66.67 on PR 3867 'adr-parser': 68, 'config-schema': 52, // CI 54.55% 2026-06-14; was 68 (timeout-inflated local) 'active-workstream-store': 80, diff --git a/tests/runtime-converters.test.cjs b/tests/runtime-converters.test.cjs index aa216ab5d..5905f8471 100644 --- a/tests/runtime-converters.test.cjs +++ b/tests/runtime-converters.test.cjs @@ -2208,3 +2208,374 @@ describe('bug-2876: skill+agent converters emit YAML-quoted description', () => }); }); } + +// ─── #3706: OpenCode subagent effort (variant) + frontmatter escaping ──────── +// +// `query resolve-execution` reported an effort (high/xhigh) that never reached +// OpenCode: the bake wrote `model:` and nothing else, so every subagent ran at +// whatever opencode.jsonc defaults the model to. The effort-side twin of #3705. +// +// bin/install.js has its own copy of these converters, but no `isAgent: true` +// call site: its agents install path runs through the compiled bin/lib module, +// not through its own copy. The live copy in src/runtime-artifact-conversion.cts +// (compiled to gsd-core/bin/lib/runtime-artifact-conversion.cjs) is therefore +// the one the bake actually uses, and the one under test here. +const liveConversion = require('../gsd-core/bin/lib/runtime-artifact-conversion.cjs'); + +const OPENCODE_CONVERTERS = [['src (live bake)', liveConversion.convertClaudeToOpencodeFrontmatter]]; +const KILO_CONVERTERS = [['src (live bake)', liveConversion.convertClaudeToKiloFrontmatter]]; + +describe('#3706: OpenCode agent variant (resolved effort)', () => { + const AGENT = ['---', 'name: gsd-executor', 'description: x', 'model: sonnet', '---', '', 'Body.'].join('\n'); + const hasKey = (out, key) => new RegExp(`^${key}:`, 'm').test(out); + + for (const [label, convert] of OPENCODE_CONVERTERS) { + test(`[${label}] emits variant alongside model`, () => { + const out = convert(AGENT, { + isAgent: true, modelOverride: 'synthetic/hf:zai-org/GLM-5.2', variant: 'high', + }); + assert.match(out, /^model: synthetic\/hf:zai-org\/GLM-5\.2$/m); + assert.match(out, /^variant: high$/m); + }); + + test(`[${label}] carries whatever level was resolved, not a fixed one`, () => { + assert.match(convert(AGENT, { isAgent: true, variant: 'xhigh' }), /^variant: xhigh$/m); + }); + + test(`[${label}] omits the key entirely when no variant is supplied`, () => { + // The control that keeps every existing install byte-identical. + const out = convert(AGENT, { isAgent: true, modelOverride: 'M' }); + assert.ok(!hasKey(out, 'variant'), `expected no variant key, got:\n${out}`); + assert.match(out, /^model: M$/m, 'the model side is unaffected'); + }); + + test(`[${label}] commands get neither model nor variant`, () => { + const out = convert(AGENT, { isAgent: false, modelOverride: 'M', variant: 'high' }); + assert.ok(!hasKey(out, 'variant')); + assert.ok(!hasKey(out, 'model')); + }); + + test(`[${label}] adding a variant perturbs no other line`, () => { + const withV = convert(AGENT, { isAgent: true, modelOverride: 'M', variant: 'high' }); + const without = convert(AGENT, { isAgent: true, modelOverride: 'M' }); + assert.match(withV, /^mode: subagent$/m); + assert.deepEqual( + withV.split('\n').filter((l) => !/^variant:/.test(l)), + without.split('\n'), + ); + }); + } + + for (const [label, convert] of KILO_CONVERTERS) { + test(`[${label}] Kilo does NOT emit variant even when one is passed`, () => { + // The asymmetry, pinned. EFFORT_ARGV declares surfaces for claude/opencode/ + // codex and has no kilo entry — unlike the model side, where #2794 J8 + // requires the two runtimes to resolve identically. + const out = convert(AGENT, { isAgent: true, modelOverride: 'M', variant: 'high' }); + assert.ok(!hasKey(out, 'variant'), `kilo has no declared effort surface; got:\n${out}`); + assert.match(out, /^model: M$/m, 'but the model override still applies'); + }); + } +}); + +// ─── #3706: config-supplied values cannot inject frontmatter keys ──────────── +// +// `model:` and now `variant:` both interpolate values read from +// .planning/config.json / ~/.gsd/defaults.json. Raw interpolation let a value +// containing a newline inject additional TOP-LEVEL keys — proven by execution +// during the #3705 security review. The sink predates #3706; this change adds a +// SECOND write to it, so it is closed here rather than doubled. +describe('#3706: frontmatter values are escaped, not interpolated raw', () => { + const AGENT = ['---', 'name: gsd-executor', 'description: x', '---', '', 'Body.'].join('\n'); + + for (const [label, convert] of OPENCODE_CONVERTERS) { + test(`[${label}] a newline-bearing model value cannot add top-level keys`, () => { + const out = convert(AGENT, { + isAgent: true, modelOverride: 'sonnet\ntools: ["*"]\npermission: bypass', + }); + assert.ok(!/^tools:/m.test(out), `injected a tools key:\n${out}`); + assert.ok(!/^permission: bypass$/m.test(out), `injected a permission key:\n${out}`); + assert.match(out, /^model: "/m, 'the value is quoted'); + }); + + test(`[${label}] a newline-bearing variant value cannot add top-level keys`, () => { + const out = convert(AGENT, { isAgent: true, variant: 'high\nevil: yes' }); + assert.ok(!/^evil:/m.test(out), `injected a key:\n${out}`); + assert.match(out, /^variant: "/m); + }); + } + + for (const [label, convert] of KILO_CONVERTERS) { + test(`[${label}] Kilo model values are escaped too — the same sink exists there`, () => { + const out = convert(AGENT, { isAgent: true, modelOverride: 'sonnet\ntools: ["*"]' }); + assert.ok(!/^tools:/m.test(out), `injected a tools key:\n${out}`); + }); + } +}); + +// ─── #3706: values that look plain but do not round-trip ──────────────────── +// +// Matching /^[A-Za-z0-9._:\/@+-]+$/ is not the same question as "does YAML read +// this back as the exact string that went in". Each row below is wrong when +// emitted bare; the last is the control that keeps real installs unchanged. +describe('#3706: YAML-ambiguous scalars are quoted, not emitted bare', () => { + const AGENT = ['---', 'name: gsd-executor', 'description: x', '---', '', 'Body.'].join('\n'); + + const QUOTED = [ + // A PARSE ERROR bare, not merely ambiguous: '@' is a YAML reserved + // indicator and may not open a plain scalar, so the whole agent file + // becomes unreadable. + '@org/model', + // A trailing ':' reads as a nested mapping key — "bad indentation of a + // mapping entry" kills the frontmatter. + 'foo:', + // YAML 1.1 resolves these to booleans/null, so a variant named 'no' + // arrives as `false` and matches no entry in the user's variants map. + 'no', 'yes', 'true', 'off', 'y', 'n', 'null', + // '12:30' resolves to the integer 750; ':' is legal mid-identifier here, + // so the form is reachable rather than contrived. + '12:30', '0755', '1.5', '0x1f', + // #3706 hardening: '(.+?)' in the value regex needs one character before + // this fix, so an empty/whitespace-only value read as "key absent" rather + // than "key present, value empty" — these are the YAML-ambiguous classes + // that predicate missed once presence and value became distinct questions. + '~', '.inf', '.Inf', '.nan', '+1', '-1', '-0', '+1.5', '.5', '0x1F', + '2026-08-25', '2026-08-25T10:00:00Z', 'a: b', 'a #b', + ]; + // The whole point of the predicate: no churn for anybody's existing files. + const BARE = ['sonnet', 'synthetic/hf:zai-org/GLM-5.2', 'gpt-5.6-luna', 'claude-opus-5', 'x-', 'a.b', 'GLM-5.2']; + + for (const [label, convert] of OPENCODE_CONVERTERS) { + const modelLine = (v) => + (convert(AGENT, { isAgent: true, modelOverride: v }) + .split('\n').find((l) => l.startsWith('model:')) ?? ''); + + for (const v of QUOTED) { + test(`[${label}] quotes ${JSON.stringify(v)}`, () => { + assert.equal(modelLine(v), `model: "${v}"`); + }); + } + for (const v of BARE) { + test(`[${label}] leaves ${JSON.stringify(v)} bare`, () => { + assert.equal(modelLine(v), `model: ${v}`); + }); + } + test(`[${label}] the same predicate governs variant, not just model`, () => { + assert.match(convert(AGENT, { isAgent: true, variant: 'no' }), /^variant: "no"$/m); + }); + } +}); + +// ─── #3706: the threading seam — where the bug actually was ───────────────── +// +// Every test above exercises a converter directly. #3706 broke one layer up, in +// runtime-artifact-layout's convertedAgentsKind: the converter had no variant to +// write because nothing resolved or passed one. A converter-only suite goes green +// while the value still never arrives, so the resolve→render chain is pinned here. +describe('#3706: install-time effort resolves and renders for OpenCode', () => { + const effortResolver = require('../gsd-core/bin/lib/install-effort-resolver.cjs'); + const catalog = require('../gsd-core/bin/lib/model-catalog.cjs'); + + // Exactly the composition the layout performs per agent. + const thread = (effortCfg, agentName) => { + const universal = effortCfg ? effortResolver.resolveInstallTimeEffort(effortCfg, agentName) : null; + return universal ? catalog.clampEffortForHost('opencode', universal) : null; + }; + + test('no effort config at all yields no variant', () => { + // The gate. resolveInstallTimeEffort ALWAYS returns a level ('high' from the + // catalog default) even for a null config, so gating on its return value + // would stamp `variant:` into every existing OpenCode install. + assert.equal(effortResolver.resolveInstallTimeEffort(null, 'gsd-executor'), 'high'); + assert.equal(thread(null, 'gsd-executor'), null); + }); + + test('a configured agent override reaches the emitted value', () => { + assert.equal(thread({ agent_overrides: { 'gsd-executor': 'xhigh' } }, 'gsd-executor'), 'xhigh'); + }); + + test('a configured default reaches an agent with no catalog tier', () => { + // effort.default is only consulted for agents the tier ladder does not + // answer for; see the tiered-agent test below for the other half. + assert.equal(thread({ default: 'low' }, 'not-a-catalog-agent'), 'low'); + }); + + test("'inherit' is never emitted as a literal", () => { + // #3533 (10d): inherit means "omit the key and follow the host default" and + // is not a wire level on ANY runtime. It IS a member of EFFORT_SET, so the + // resolver returns it verbatim — the declared render seam is what drops it. + // Writing `variant: inherit` would name a variant that cannot resolve. + for (const cfg of [ + { agent_overrides: { 'gsd-executor': 'inherit' } }, + { routing_tier_defaults: { light: 'inherit', standard: 'inherit', heavy: 'inherit' } }, + ]) { + assert.equal(effortResolver.resolveInstallTimeEffort(cfg, 'gsd-executor'), 'inherit'); + assert.equal(thread(cfg, 'gsd-executor'), null, `should omit for ${JSON.stringify(cfg)}`); + } + }); + + test('a bare effort.default does NOT reach a tiered agent', () => { + // Not obvious, and it is why the `inherit` case above goes through + // agent_overrides instead: for an agent WITH a catalog tier the manifest + // tier ladder (#3531) answers before effort.default is ever consulted, so + // `default: 'inherit'` leaves a tiered agent at its tier value. Pinned + // because a test written against `default` alone would assert nothing. + assert.equal(effortResolver.resolveInstallTimeEffort({ default: 'inherit' }, 'gsd-executor'), 'high'); + assert.equal(thread({ default: 'inherit' }, 'gsd-executor'), 'high'); + }); + + test('a level OpenCode does not accept is dropped, not emitted', () => { + // 'ultra' is a real EFFORT_SET member that OpenCode's declared supported set + // does not contain. Omitting beats naming a variant that cannot resolve. + assert.equal(catalog.renderEffortArgv('opencode', 'ultra', 'argv').value, null); + }); + + test('the full universal ladder OpenCode does accept passes through', () => { + for (const lvl of ['minimal', 'low', 'medium', 'high', 'xhigh', 'max']) { + assert.equal(catalog.renderEffortArgv('opencode', lvl, 'argv').value, lvl); + } + }); + + test('an empty model override omits the key rather than writing an empty value', () => { + // Found by the property below, which originally asserted a round-trip for + // every generated string and failed on "". An empty override is falsy, so + // no `model:` line is written at all — that is the #2256/J7 contract (omit + // the key, never `model: ""`), so the property is scoped to non-empty + // values and the empty case is pinned here instead of being generated away. + const out = liveConversion.convertClaudeToOpencodeFrontmatter( + ['---', 'name: x', 'description: y', '---', '', 'Body.'].join('\n'), + { isAgent: true, modelOverride: '' }, + ); + assert.ok(!/^model:/m.test(out), `expected no model key, got:\n${out}`); + }); + + test('every emitted model line parses back to the exact value', () => { + // The real contract, stated directly: whatever a user puts in config, the + // generated frontmatter must read back as that same string. A per-character + // rule list is only a means to this end, and this is what catches the case + // nobody thought to enumerate. + const fc = require('fast-check'); + const yaml = require('js-yaml'); + const chars = ':@-_./"\\ \n\t#&*!|>%`{}[],?~+abcXY019'; + fc.assert( + fc.property( + fc.string({ unit: fc.constantFrom(...chars.split('')), minLength: 1 }), + (value) => { + const out = liveConversion.convertClaudeToOpencodeFrontmatter( + ['---', 'name: x', 'description: y', '---', '', 'Body.'].join('\n'), + { isAgent: true, modelOverride: value }, + ); + const fmBody = /^---\r?\n([\s\S]*?)^---\r?$/m.exec(out)[1]; + return yaml.load(fmBody).model === value; + }, + ), + { numRuns: 2000 }, + ); + }); +}); + +// ─── #3706: the layout seam actually threads the variant ───────────────────── +// +// Every test above calls a converter directly. Direct-converter tests all +// still pass if a future edit drops `variant` from the `rawConverter(...)` +// call inside runtime-artifact-layout.cts's convertedAgentsKind, or flips its +// `converterName ===` gate — this drives the REAL staging path +// (resolveRuntimeArtifactLayout -> the OpenCode 'agents' kind's stage()) so +// that regression cannot hide behind converter-only coverage. +describe('#3706: the layout seam actually threads the variant', () => { + const fs = require('node:fs'); + const os = require('node:os'); + const path = require('node:path'); + const { cleanup } = require('./helpers.cjs'); + const { resolveRuntimeArtifactLayout } = require('../gsd-core/bin/lib/runtime-artifact-layout.cjs'); + + // Fixture layout mirrors the .gsd-source marker convention + // (findAgentsSourceRoot): the marker at /.gsd-source points to + // /commands/gsd, and agents/ is resolved as its sibling. + function makeConfigDirFixture(agentContent) { + const configDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3706-seam-configdir-')); + const commandsDir = path.join(configDir, 'commands', 'gsd'); + const agentsDir = path.join(configDir, 'agents'); + fs.mkdirSync(commandsDir, { recursive: true }); + fs.mkdirSync(agentsDir, { recursive: true }); + fs.writeFileSync(path.join(configDir, '.gsd-source'), commandsDir + '\n', 'utf8'); + fs.writeFileSync(path.join(agentsDir, 'gsd-executor.md'), agentContent, 'utf8'); + return configDir; + } + + function makeProjectRootWithEffort(effort) { + const projectRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3706-seam-project-')); + fs.mkdirSync(path.join(projectRoot, '.planning'), { recursive: true }); + fs.writeFileSync( + path.join(projectRoot, '.planning', 'config.json'), + JSON.stringify({ effort }), + 'utf8', + ); + return projectRoot; + } + + // resolveInstallTimeEffort also merges ~/.gsd/defaults.json (os.homedir()), + // so HOME/USERPROFILE are pinned to an empty temp dir for the duration of + // each stage() call to keep the assertion hermetic against the real + // developer machine's home directory. + function stageWithFakeHome(agentKind, resolvedProfile, agentCtx) { + const fakeHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3706-seam-home-')); + const realHome = process.env.HOME; + const realUserProfile = process.env.USERPROFILE; + process.env.HOME = fakeHome; + process.env.USERPROFILE = fakeHome; + try { + return agentKind.stage(resolvedProfile, agentCtx); + } finally { + // On POSIX, USERPROFILE (and potentially HOME) is unset before this + // helper runs — `process.env.X = undefined` would coerce to the + // literal string "undefined" and leak that into the environment for + // the rest of the test process. Restore by deletion when the saved + // value was genuinely absent. + if (realHome === undefined) delete process.env.HOME; + else process.env.HOME = realHome; + if (realUserProfile === undefined) delete process.env.USERPROFILE; + else process.env.USERPROFILE = realUserProfile; + cleanup(fakeHome); + } + } + + const AGENT_SOURCE = ['---', 'name: gsd-executor', 'description: x', '---', '', 'Body.'].join('\n'); + const resolvedProfile = { name: 'full', skills: '*', agents: new Set() }; + + test('a configured effort reaches the real OpenCode agents kind staging output', () => { + const configDir = makeConfigDirFixture(AGENT_SOURCE); + const projectRoot = makeProjectRootWithEffort({ agent_overrides: { 'gsd-executor': 'xhigh' } }); + try { + const layout = resolveRuntimeArtifactLayout('opencode', configDir, 'global'); + const agentKind = layout.kinds.find((k) => k.kind === 'agents'); + assert.ok(agentKind, 'opencode global layout must include an agents kind'); + + const agentCtx = { runtime: 'opencode', pathPrefix: '', attribution: null, targetDir: projectRoot }; + const stagedDir = stageWithFakeHome(agentKind, resolvedProfile, agentCtx); + const stagedContent = fs.readFileSync(path.join(stagedDir, 'gsd-executor.md'), 'utf8'); + assert.match(stagedContent, /^variant: xhigh$/m, `expected variant: xhigh in staged output:\n${stagedContent}`); + } finally { + cleanup(configDir); + cleanup(projectRoot); + } + }); + + test('no effort config at all reaches the real staging output with no variant key', () => { + const configDir = makeConfigDirFixture(AGENT_SOURCE); + const noConfigRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3706-seam-noconfig-')); + try { + const layout = resolveRuntimeArtifactLayout('opencode', configDir, 'global'); + const agentKind = layout.kinds.find((k) => k.kind === 'agents'); + + const agentCtx = { runtime: 'opencode', pathPrefix: '', attribution: null, targetDir: noConfigRoot }; + const stagedDir = stageWithFakeHome(agentKind, resolvedProfile, agentCtx); + const stagedContent = fs.readFileSync(path.join(stagedDir, 'gsd-executor.md'), 'utf8'); + assert.doesNotMatch(stagedContent, /^variant:/m, `expected no variant key in staged output:\n${stagedContent}`); + } finally { + cleanup(configDir); + cleanup(noConfigRoot); + } + }); +}); +