9 Commits

Author	SHA1	Message	Date
Dennis Alexis Valin Dittrich	ad1477d659	enhance(#4154): validate configured entrypoints before reporting install success (#4249) ... * test(260903-m7p): expose configured-entrypoint validation gap * enhance(260903-m7p): validate configured entrypoints before success * test(260903-m7p): require pre-success entrypoint validation * enhance(260903-m7p): gate install success on entrypoints * test(260903-m7p): cover configured entrypoints across runtimes * enhance(260903-m7p): cover emitted runtime entrypoints * fix(260903-m7p): sandbox HOME in finishInstall test and fix changeset pr number - finishInstall(...'cline'...) calls writeNonClaudeDefaults(runtime) in-process before the new configured-entrypoint assertion throws. Without a HOME + config-location-env sandbox that write resolved through the ambient environment and landed in the developer's live ~/.gsd (confirmed absent on origin/next baseline, present only on this branch — full-suite HERMETICITY WARNING). Sandbox HOME/USERPROFILE and scrub config-location env for the duration of the test, matching the existing in-process finishInstall/ install() pattern in tests/install.test.cjs (#2665). - .changeset/quick-wasps-sing.md: pr: 0 is a never-backfilled placeholder (CONTRIBUTING.md) that fails changeset-lint's invalid_pr check; set to the fork PR number until the upstream PR number is known. * fix(260903-m7p): repair cross-platform and pre-existing shape fallout - tests/configured-entrypoint-validation.test.cjs: the win32 branch of ensureCodexHooksJsonSessionStart writes a .cmd shim under <codexRoot>/hooks/; create that dir in the test (the real installer only calls this once hooks/gsd-check-update.js already exists) and assert the platform-common entrypoint shape instead of a fixed non-Windows array, since win32 legitimately emits two entries (cmd shim + script). - tests/install.test.cjs: finishInstall's shared settings-json return now carries configuredEntrypoints/rollbackInstallerMigrations for every runtime on that path (trae included, not just Claude/Cursor/Windsurf); update the trae install() exact-shape assertion to match. * fix(260903-m7p): keep .sh interpreter tracking consistent with unresolved bash configuredEntrypointsForHook's shell branch dropped interpreterCandidates entirely when resolveBashExecutable returned null, unlike the sibling portableHooks runner entry a few lines below (which correctly falls back to the literal 'bash' token). Found via agy adversarial review; verified unreachable through the current call graph (buildHookCommand's own resolveBashRunner==null gate already short-circuits before recordConfiguredHookCommand runs), so this is a defensive consistency fix, not a live-bug patch — kept for the next caller that does not share that gate. * chore(260903-m7p): backfill changeset pr number to the opened upstream PR .changeset/quick-wasps-sing.md carried the fork PR number (16) as a placeholder until the upstream PR existed; open-gsd/gsd-core#4249 is now open, so record its real number per CONTRIBUTING.md's changeset pr-field convention. * fix(#4154): track already-registered hooks for entrypoint validation on update applySettingsJsonHooks registers each guard hook only if absent, so a hook already present from a prior install keeps its stale on-disk command. The new entrypoint tracker always records the freshly-computed command for it, which never matches what is actually persisted, so the exact-string filter in finishInstall silently dropped it from validation — the Blocker case this feature exists to catch (an already-installed entrypoint going stale between installs) was exactly the case it never validated. Match on the managed script's basename instead, which the persisted command carries either way, so an already-registered hook stays in the validated set. Regression test forces this path by mutating a freshly-installed hook's persisted command before a second install. * fix(#4154): distinguish an unreadable script from a missing one validateConfiguredEntrypoints folded an EACCES statSync failure into the same 'missing' reason as ENOENT, misreporting a real permission problem as an absent file. Check the error code and report 'unreadable' instead. * docs(#4154): document entrypoint validation's rollback and PATH scope CONTEXT.md's Runtime Hooks Surface Module / Installer Module entries had no mention of ConfiguredEntrypoint/validateConfiguredEntrypoints, despite bin/install.js x CONTEXT.md being this repo's strongest co-change pairing. The update-gsd.md how-to overstated what a validation failure undoes: for Codex/Cursor/Windsurf/Kimi, their own writer already persisted hooks.json/ config.toml inside install() before the aggregate validation call runs, so there is no rollback path for that write regardless of "where available" phrasing. Also note that interpreter resolution checks the installer's own PATH, not necessarily the PATH a hook fires under later (#2979 launchers). * chore(#4154): point changeset pr field at the fork PR while CI runs there Mirrors the branch's own prior backfill commit: pr: matches whichever PR number changeset-lint is currently validating against (fork PR #16 during the fork-first CI/review loop), flipped back to the upstream PR number right before the final push to open-gsd/gsd-core. * fix(#4249): address adversarial-review findings in entrypoint validation An internal adversarial review (agy/gemini-3.8-flash-high) of the whole PR found several real gaps beyond the human reviewer's Blocker, verified against source before fixing: - Codex's install() result bound rollbackInstallerMigrations to the narrow installer-migrations-only rollback instead of restoreCodexSnapshot (#3245), the full pre-install snapshot/restore Codex already owns for exactly this case — a validation failure discovered outside install() reverted nothing of the config.toml/hooks.json that call had already written. - The register-only-if-absent basename match from the prior fix used a bare substring, which an unrelated user command mentioning the same filename could false-positive into GSD's validated set — anchored on the `/hooks/<basename>` path segment instead. - nodeCandidates checked raw process.execPath (always true — we're running in that process) instead of normalizeNodePath's stable version-manager alias, the same one buildNodeRunnerChainToken bakes as its first choice — a false green regardless of whether that alias itself still resolves. - An entry with no interpreterCandidates (Cline's PreToolUse hook, or a Windows-Claude .sh hook invoked without a bash runner) runs via its own shebang; validateConfiguredEntrypoints checked only file-type, never the execute bit. Cline's writer also never reported an entrypoint at all. - Duplicate (configPath, scriptPath) entries (e.g. Kimi's context-monitor hook registered across several events) were validated once per duplicate. Each fix is covered by a new or extended test; the Codex one required inlining runCodexInstall's env sandboxing so the rollback closure — which re-resolves the $HOME-relative skills root live — runs before the sandbox is torn down, matching how installAllRuntimes' real aggregate gate calls it. * docs(#4249): document the round-2 entrypoint-validation fixes Runtime Hooks Surface Module and Installer Module entries now name ConfiguredEntrypoint's not-executable reason, the normalizeNodePath alignment, Cline's tracked hook, and which install() result the finishInstall/installAllRuntimes rollback path actually reverts per runtime (Codex's full snapshot vs. the others' narrow migrations-only rollback). * chore(#4249): point changeset pr field at the upstream PR now that fork CI is green * fix(#4249): address agy adversarial-review findings - validateConfiguredEntrypoints: statSync alone never detects a chmod-000 script (it only needs parent-dir search permission), so an interpreter-invoked entry with an unreadable script passed validation. Add an explicit R_OK check for the interpreterCandidates branch only — the candidate-less/shebang branch already has its own X_OK gate. - docs/how-to/update-gsd.md: the blanket "does not revert" claim was false for Codex, which reverts config.toml/hooks.json via its full pre-install snapshot; qualify it per runtime. - tests/codex-config.test.cjs: the #4249 rollback regression test asserted skills/ and VERSION were reverted but never asserted config.toml/hooks.json were too, despite the test's own stated intent. - CONTEXT.md: qualify which interpreterCandidates entries get normalizeNodePath'd (Node hooks only, not .sh/bash) and note Codex's Windows .cmd shim as a third candidate-less case that relies on extension dispatch, not a shebang. * fix(#4249): validate Cline's PATH-dependent interpreter, not just its execute bit Cline's hook is a hybrid: it self-executes via '#!/usr/bin/env node', so it needs the execute bit (like any shebang-invoked entry), but its interpreter is looked up on PATH by 'env' at hook-fire time (unlike every other GSD JS hook, which bakes an absolute node path specifically to avoid that dependency). The candidate-less/interpreterCandidates fork treated these as mutually exclusive, so Cline's entry silently skipped interpreter resolution entirely — a completely missing 'node' on PATH would still validate successfully. Add an orthogonal selfExecutable flag so both checks run for entries that need them. (CodeRabbit finding on the fork rehearsal PR.) * fix(#4249): address second-round adversarial review findings (opus + agy) - validateConfiguredEntrypoints: R_OK now runs for every scriptOk entry, not just interpreterCandidates ones — a self-executable shebang script is still opened and read by its kernel-invoked interpreter, so X_OK alone never proved it was readable. - selfExecutable is now the sole, explicit source of truth for the execute-bit check (every producer that needs it sets the flag) instead of being partly inferred from an absent interpreterCandidates, which Cline's hybrid entry also carries. - The execute-bit check now skips explicitly on win32 (matching resolveExecutableBinary's own carve-out) instead of relying on Node's accessSync(X_OK)-as-F_OK no-op, which only protects a real Windows machine and not a test that simulates win32 on a POSIX runner. - bin/install.js: fixed a stale comment claiming no runtime's install()-time writes have a rollback path — Codex's does (restoreCodexSnapshot) — and added the omitted Cline to both that comment and CONTEXT.md's equivalent lists. - CONTEXT.md: fixed the Cline description left stale by the previous commit's selfExecutable addition, and rewrote the validation-mechanism paragraph for clarity (writing-for-agents pass). - docs/how-to/update-gsd.md: split an overloaded 4-clause sentence. - Removed a fault-injection integration test that could not reliably exercise the real installAllRuntimes -> finalize -> rollback wiring without fighting the installer's own pre-registration existence guards; the constituent pieces remain covered individually. * fix(#4249): pin platform in X_OK-testing entries so they're deterministic cross-CI-runner X_OK is a POSIX-only concept, skipped entirely when an entry's platform is win32 (matching production). Two test entries omitted platform, defaulting to process.platform — on an actual windows-latest CI runner that silently skipped the very check they were meant to exercise, turning 'not-executable' into a false pass. Pin platform: 'linux' so these are deterministic regardless of which OS runs the suite. * fix(#4249): classify EPERM the same as EACCES in statSync error handling Windows raises EPERM (not EACCES) for a parent directory that couldn't be traversed into — was falling through to 'missing', misreporting a genuine permission problem as a nonexistent path. * docs(#4249): address final CodeRabbit doc-completeness findings - CONTEXT.md: install()'s documented result shape omitted configuredEntrypoints; the ConfiguredEntrypoint shape omitted selfExecutable. - docs/how-to/update-gsd.md: the failure-mode sentence omitted unreadable and lacks-execute-permission, which the installer also rejects. * fix(#4249): stop double-validating every configured entrypoint on install/update installAllRuntimes' finalize() already runs assertConfiguredEntrypoints once over the aggregate set; finishInstall then re-ran the identical check per runtime in the printSummaries loop right after, so every entrypoint paid its statSync/accessSync/interpreter-resolution cost twice on every install and update. Add entrypointsAlreadyValidated to skip the redundant pass specifically on that path, while leaving the check intact for any caller that invokes finishInstall directly. * chore(#4154): point changeset pr field at rehearsal fork PR while CI runs there * perf(#4249): memoize interpreter candidate resolution across entrypoints resolveExecutableBinary walked PATH once per (entry, candidate) pair; a typical install has a dozen-plus entries sharing the same few candidate lists (process.execPath for JS hooks, bash for shell hooks). Cache by (platform, candidate) so each distinct pair resolves once per validation call instead of once per entry. * chore(#4249): point changeset pr field at the rebased rehearsal fork PR * fix(#4249): drop entrypoint tracking from the now-dead Codex event writer #2586 (landed on next after this branch forked) removed install.js's CODEX_EXTENDED_HOOK_EVENTS registration loop, so ensureCodexHooksJsonEvent no longer runs during install or update. The ConfiguredEntrypoint records this branch added inside it were therefore unreachable and untested. Restore the function to its upstream shape; the entrypoints it used to report were never collected by any caller. * refactor(#4249): drop the revalidation bypass flag and the candidate cache Both were this PR's own micro-optimisations over a set of roughly a dozen entries. `entrypointsAlreadyValidated` let a caller turn the finishInstall gate off to save one statSync/accessSync pass; `resolvedCandidateCache` memoised resolveExecutableBinary across entries that are already deduped by (configPath, scriptPath). Neither is measurable, and the flag was the only way to reach finishInstall with validation disabled. finishInstall now always validates what it is given. * chore(#4249): point the changeset pr field back at the upstream PR * refactor(#4249): track settings.json entrypoints without the hooksSurface gate The install-surface writer only tracked configured entrypoints when the runtime's descriptor also declared `hooksSurface: 'settings-json'`. Nothing asserts that axis agrees with `installSurface`, so a descriptor that broke the coupling would silently pass `configuredEntrypoints: undefined` and drop that runtime out of the validation this PR adds — reintroducing the exact 'reports Done! over a broken entrypoint' failure #4154 exists to close. Remove the dependence rather than test it: everything recorded on this path lands in settings.json by construction, and the registered-command filter already discards entries no persisted hook references. * chore(#4249): put the changeset body in the documented two-part format CONTRIBUTING.md and .changeset/README.md both show `**<bold change>** — <symptom-led explanation>.`; the fragment was a single unbolded sentence. * chore(#4249): point the changeset pr field at the rehearsal fork PR while CI runs there * fix(#4249): restore the whole manifest-tracked GSD file set on Codex rollback #3245's snapshot covers config.toml, hooks.json, skills/gsd-*, agents/gsd-* and gsd-core/VERSION. The install overwrites every other GSD-owned file too — hooks/, gsd-core/CHANGELOG.md, scripts/, gsd-core/.gsd-runtime, the manifest itself — before the entrypoint-validation gate runs, so a validation failure left the new payload sitting on top of the restored old config. Snapshot the file set the PREVIOUS install's gsd-file-manifest.json claims, before runInstallerMigrations so the bytes are the true pre-install state, and restore it from both Codex rollback closures ahead of the per-surface restores. Files only the failed install introduced are removed, read from the manifest now on disk. The manifest is already the authoritative record of what GSD owns, so no second hand-written list can drift out of sync, and user-owned files are never snapshotted or removed. Every path is confined through resolveInstallRelativePath, so a hand-edited manifest cannot turn rollback into an arbitrary-path write. Non-Codex runtimes are unaffected: the snapshot is gated on the same tomlConfigInstall + non-minimal condition as #3245's. * fix(#4249): keep the managed-file snapshot honest in minimal mode and on a bad manifest Two follow-on defects in the previous commit's snapshot: - The capture was gated on `!isMinimalMode`, copied from #3245. A core/ --minimal Codex install still writes gsd-core/, hooks/, scripts/ and the manifest, and restoreCodexSnapshot is reachable in that mode (#2695), so the snapshot came back empty while the rollback still ran — and its removal pass would have deleted every file the new manifest lists. Gate on tomlConfigInstall alone, matching where the rollback actually reaches. - An unreadable or unparseable prior manifest was caught alongside ENOENT and treated as a fresh install. That is the same empty-snapshot state, so a failed update over a real install with a corrupt manifest could delete its prior payload. Track whether the pre-install GSD-owned set is KNOWN: ENOENT means known-empty; any other read error or a parse failure means unknown, and the restore closure returns without touching anything, degrading to #3245's narrower rollback. Deliberately not fatal — a corrupt manifest has to stay repairable by reinstalling over it. Both paths are covered by red-checked regression tests. * fix(#4249): snapshot Codex skills, agents and VERSION in minimal mode too commit removed from the manifest snapshot. restoreCodexSnapshot is reachable for a core/--minimal install (#2695), and its pass-2 sweeps remove every gsd-* skill dir and gsd-* agent file the snapshot does not claim — so with an empty minimal-mode snapshot a rollback deleted the whole skills/agents surface with nothing to restore it from. Codex resolves skills to $HOME/.agents/skills via the ADR-1239 skills-kind home override, so this is also the reason manifest `skills/` keys do not resolve under configDir: that surface belongs to this snapshot, not to the manifest-driven one. Gate on tomlConfigInstall alone. _codexPreConfigRollback stays null in minimal mode — doing nothing on an early failure is the non-destructive side. Covered by a red-checked regression test that plants bytes in an alternate-home skill file, reinstalls under the core profile marker, and asserts the rollback restores it. * fix(#4249): never remove on rollback unless a prior manifest proves what predates the install Three defects in the manifest-driven Codex rollback, all in its removal half: - ENOENT marked the snapshot usable, arming the removal pass on a FIRST install. GSD may have overwritten a user's file at a manifest-tracked path there, and no prior manifest records the difference — so rollback deleted it where before it merely left it overwritten. Absent, unreadable and malformed manifests now all leave the prior set UNKNOWN and skip removal entirely. - Membership was tested against the map of files whose pre-install read SUCCEEDED, so a tracked file that existed but was unreadable read as introduced-by-this-install and was removed. Track the prior manifest's paths in their own Set and test against that. - The unreachable "delete the manifest when there was no prior one" branch is gone: usable now implies a parsed prior manifest. Also adds the end-to-end test the aggregate gate was missing — the four Codex rollback tests drove the closure directly, proving the restore but not the wiring. installAllRuntimes(['codex','cline']) under an emptied PATH makes Cline's `env node` entry fail validation for real, and asserts Codex's payload comes back. Test preamble (HOME/USERPROFILE sandbox + config-env scrub) is now one helper instead of six copies. Both new tests are red-checked. * test(#4249): use unlinkSync, not rmSync, to drop the manifest in a test lint:ci's raw-fs.rmSync rule points tests at helpers.cleanup for its Windows-EBUSY retry budget. That budget is for directory trees; this removes a single file, which unlinkSync says more precisely and the rule does not flag. * chore(#4249): point the changeset pr field back at the upstream PR * fix(#4249): use an unambiguous dedup key and surface partial-restore failures trek-e's 2026-09-08 adversarial pass flagged two findings in the new entrypoint-validation/rollback code: - assertConfiguredEntrypoints' dedup key already used a raw NUL separator (introduced in ceebb65f2d), but git/Read render NUL as a space, so the key looked like a plain-space join to every reviewer that read the diff. Replace it with JSON.stringify([configPath, scriptPath]) so the separator is visible and unambiguous. - restoreManagedFileSnapshot's per-file restore catch block claimed to 'surface the original error' but only swallowed it, matching (and widening) the pre-existing #3245 restoreCodexSnapshot pattern. Add an actual console.warn using the existing best-effort-warning convention, scoped to just this PR's new function. * fix(#4249): treat a files-less prior manifest as unknown, not known-empty agy's gemini-3.8-flash-high adversarial pass (round 5) found and I reproduced empirically: a structurally-valid manifest missing the files key (e.g. {"version":1}) parses without throwing, so Object.keys(undefined || {}) silently read as 'zero files predate this install' instead of the UNKNOWN state the malformed-manifest guard exists to produce. Rollback's removal pass then deleted every GSD-owned file the failed install's own manifest listed, including ones that predated it — the exact data loss the #4249 CodeRabbit malformed-manifest fix was supposed to prevent, reachable through a JSON.parse success instead of a failure. Route the shapeless case into the same catch-all UNKNOWN path via an explicit shape check. Regression test reproduces the deletion before the fix and confirms the file survives after it. Also extend restoreManagedFileSnapshot's removal-pass rmSync and final manifest-rewrite catches with the same real console.warn trek-e's round-4 review asked for on the per-file restore catch — same rollback function, same operator-facing-signal gap. * docs(#4249): correct which runtimes actually leave a written config on rollback agy's completeness audit (round 5, holistic pass) caught this new paragraph claiming 'for every other runtime, the configuration file(s) already written during that update are left in place' — false for Claude Code and other settings.json-based runtimes, whose write never happens on failure (assertConfiguredEntrypoints runs before finishInstall's writeSettings). Only Cursor/Windsurf/Kimi/Cline actually match that description, since they persist their config file inside install() ahead of the gate. Split the one sentence into the three actual outcomes; matches the PR body's own accurate Before/After wording, which this doc addition had drifted from. * fix(#4249): clean up doc/comment mismatches and dead fields from opus review Opus critical-code-reviewer + ponytail-review pass on the final diff: - assertConfiguredEntrypoints carried finishInstall's old docblock ("Apply statusline config, then print completion message") from before this function was inserted between comment and callee. finishInstall already has its own accurate #4249 comment, so the stale docblock is removed rather than moved. - checked: number on ConfiguredEntrypointValidationResult and error.configuredEntrypointValidation on the thrown error: the first had zero consumers anywhere in the repo, including its own defining file, and is removed. The second matches an existing repo convention (bin/install.js's installerMigrationRollbackFailures, #4249 predates this PR) of attaching structured diagnostic context to a re-thrown Error even before a consumer exists, so it's kept. - finishInstall's own assertConfiguredEntrypoints call is a redundant backstop on the real production path (installAllRuntimes's aggregate call already validates the superset first), but its comment read as though this call alone provided the before-the-write guarantee. Clarified rather than removed — it's the only gate for a caller that invokes finishInstall directly. * chore(#4249): split the manifest-driven rollback engine out into #4544 Issue #4154 asked the installer to consume a validation failure "through the existing rollback mechanism, without a second transaction mechanism". The manifest-driven rollback widening added during review (capture every path the prior gsd-file-manifest.json claims, restore those bytes, remove what only the failed install introduced) is that second mechanism on a plain reading. It is a real fix for a #3245-era gap, but an independent one, so it moves to its own bug report and PR. Removed here: - bin/install.js: the pre-install managed-file capture block and restoreManagedFileSnapshot, plus its call sites in _codexPreConfigRollback and restoreCodexSnapshot (99 lines). - tests/configured-entrypoint-validation.test.cjs: the five tests that exercise the manifest engine. - CONTEXT.md and docs/how-to/update-gsd.md: the sentences describing the widened restore. update-gsd.md again documents the #3245 surfaces only. Kept, because it is #4154's own scope: - the entrypoint-validation gate itself; - Codex's install() result binding rollbackInstallerMigrations to restoreCodexSnapshot (config.toml, hooks.json, skills/gsd-*, agents/gsd-*, gsd-core/VERSION); - the !isMinimalMode gate removal on that snapshot. Binding the closure to the result made it reachable for a core/--minimal install, where its pass-2 sweeps delete every gsd-* skill dir and agent file the snapshot does not claim; an empty minimal-mode snapshot therefore deleted the whole surface with nothing to restore. The surviving aggregate-failure test now asserts on config.toml, a surface the #3245 snapshot owns, instead of gsd-core/CHANGELOG.md, which only the manifest engine restored. Refs #4544 * test(#4249): cover configured entrypoints through the packed install path #4154's scope lists install smoke coverage alongside the installer gate — "assert representative configured entrypoints resolve for supported runtime profiles". The gate itself (assertConfiguredEntrypoints / validateConfiguredEntrypoints) is unit-covered by in-process install() calls; nothing proved the property survives npm pack -> npm install -g -> install.js. Add Cycle 4 to runSmoke. For each of claude and codex — the two distinct config surfaces GSD writes launch paths into (settings.json, and hooks.json + config.toml) — run the tarball-installed installer into a throwaway HOME, then re-read that runtime's own written config and return the new ENTRYPOINT_UNRESOLVED code when a script path it names does not resolve to a file. install-smoke.yml already asserts .code == "ok" on the CLI, so the check becomes a release gate on every matrix host without workflow changes. The scan re-derives paths from the written config instead of reusing the installer's own entrypoint list, and test I shows why that matters: a registration the installer never touched during a run is invisible to the in-process gate, so the install exits 0 and only reading the config back off disk catches the dangling launch path. * ci(#4249): pack a publish-shaped tarball in the install smoke lane `npm pack` runs prepack/prepare (build:lib); only prepublishOnly runs build:hooks. hooks/dist is gitignored, so the tarball install-smoke.yml packs after `npm ci` carries no hook scripts at all — the lane has been smoking a package that differs from the published one in exactly the artifacts the lifecycle smoke is supposed to launch. That went unnoticed because the lane's init runs `--local`, which registers no statusline and therefore registers no hook whose target is missing. A `--global` install on the same tarball exits 1 on #4249's own gate (`gsd-statusline.js (missing)`), which is what the new configured-entrypoint cycle performs, so without this step the cycle would report INIT_FAILED instead of checking anything. Build hooks before packing so the smoked tarball matches prepublishOnly. The CLI now reports 16 configured entrypoints for claude and 1 for codex instead of zero. * fix(#4249): scope Codex's full snapshot restore to entrypoint failures Binding Codex's result to `restoreCodexSnapshot` made ANY finalize-stage exception un-install a Codex install that had already succeeded and already printed its own "Done!" summary — `rollbackFinalizedInstallerMigrations` wraps the whole `finalize()` body, not just the aggregate `assertConfiguredEntrypoints` call. Nothing documents that. `docs/installer-migrations.md#phase-4-installupdate-integration` scopes finalize-stage rollback to installer *migrations* ("the executor uses the journal to restore modified paths"), and this PR's own operator-facing paragraph in `docs/how-to/update-gsd.md` scopes the Codex config.toml/hooks.json/skills/ agents/VERSION revert to entrypoint-validation failures specifically ("If a script is missing, unreadable, ... For Codex, this reverts ..."). The wide behaviour is also incoherent as a transaction abort: the same doc says Cursor, Windsurf, Kimi and Cline keep the config they wrote inside install(). Concretely: `installAllRuntimes(['codex', 'kilo'])` where Kilo's finishInstall hits EACCES writing kilo.json rolled Codex's config.toml back to its pre-install bytes — on an update, silently downgrading a working Codex install to the previous version while the user had just been told it was Done. Select the rollback by error kind instead. `assertConfiguredEntrypoints` already tags its error with `configuredEntrypointValidation`, so the full snapshot restore runs for that error (and anything downstream of it, including finishInstall's per-runtime backstop) and the installer-migrations-only closure runs for everything else. The codex result now also exposes that narrow closure as `rollbackInstallerMigrationsOnly`; `rollbackInstallerMigrations` keeps meaning the full restore, so the direct-call contract asserted by tests/codex-config.test.cjs is unchanged. Adds a regression test that installs codex+kilo together, injects EACCES on the Kilo permission write by monkeypatching node:fs (restored in a finally — never chmod 0o000, which root bypasses in CI), and asserts Codex's config.toml keeps the bytes the successful install wrote. Verified red against the pre-fix unconditional path. Cline cannot host this test: its plan is writesSharedSettings:false + finishPermissionWriter:null, so its finishInstall performs no write and has no non-entrypoint failure path. Kilo's configureKiloPermissions runs unconditionally (unlike OpenCode's, it is not GSD_TEST_MODE-gated) and ends in an unguarded fs.writeFileSync. * docs(#4249): sync CONTEXT.md's rollback description with the round-6 narrowing CONTEXT.md still described Codex's rollback as an unconditional bind to restoreCodexSnapshot after ff13adc00 scoped it to entrypoint- validation failures via rollbackInstallerMigrationsOnly and the configuredEntrypointValidation error tag. Caught during the round-6 PR body pass. * fix(#4249): stop rollbackInstallerMigrations meaning its own opposite Codex's install() result bound `rollbackInstallerMigrations` to restoreCodexSnapshot (the FULL pre-install snapshot restore) and put the actual installer-migrations-only closure behind `rollbackInstallerMigrationsOnly` — so for one runtime the unsuffixed name meant the opposite of what it says, and CONTEXT.md had to concede as much in prose. Invert it: `rollbackInstallerMigrations` is the narrow closure for every runtime, matching both its name and the meaning it already has on next, and the snapshot restore gets its own Codex-only field, `rollbackPreInstallSnapshot`. The selection in rollbackFinalizedInstallerMigrations collapses to one line and no longer needs a fallback chain. Also in this commit, all against the same rollback path: - Correct the rollbackFinalizedInstallerMigrations comment. It read as if the round-6 narrowing prevented any sibling-triggered revert of a Codex install the user has already seen "Done!" for. It does not, and is not meant to: `wide` is true for ANY entrypoint-validation error from ANY runtime, because the aggregate gate is all-or-nothing — an invalid Cline entrypoint reverts Codex's snapshot, which tests/configured-entrypoint-validation.test.cjs's 'an aggregate entrypoint validation failure rolls the Codex install back (#4249)' asserts directly. The discriminator is the error's KIND, not which runtime owns the failing path. Comment and CONTEXT.md now say that. - Name the runtime in the "Configured entrypoint validation failed" error. ConfiguredEntrypointInvalid already carries `runtime`; the message threw it away, leaving an operator of a multi-runtime install unable to tell whose entrypoint broke — which matters precisely because the failure can revert a runtime that was itself fine. - Set `configuredEntrypoints: []` explicitly on the copilot-instructions early return. Every other branch states the key; this one relied on installAllRuntimes' `(result.configuredEntrypoints || [])` defence. `[]` is correct, not a workaround: every Copilot hook is an inline printf one-liner (GSD_COPILOT_*_HOOK_BASH/PWSH), so there is no GSD-managed script or interpreter to resolve. No behaviour change beyond the error-message text. * docs(#4249): narrow the smoke scan's config-surface claim to what it checks RUNTIME_CONFIG_FILES claimed every GSD-managed executable a runtime is told to launch is registered in one of settings.json / hooks.json / config.toml, and that nothing else in a config dir is runtime configuration. Both halves are false as stated. Cline registers its hook at .clinerules/hooks/PreToolUse — a subdirectory, and not one of those names (writeClineArtifacts, src/runtime-hooks-surface.cts). Kimi's native [[hooks]] config.toml lives under resolveKimiHooksTomlDir() (~/.kimi), a directory separate from Kimi's own GSD configDir — the same gap installer-migration 007 already documents as structurally unreachable. The scan is in fact correct for what it runs against: entrypointRuntimes defaults to claude + codex, whose launch paths do all live in those three top-level files. Restate the docstring at that scope, name the two known out-of-scope surfaces, and warn that adding either runtime to entrypointRuntimes without teaching scanConfiguredEntrypoints about its surface yields a scan that finds zero entrypoints and proves nothing. The entrypointRuntimes default comment carried the same overgeneralization ("every other runtime reuses one of them") and is corrected with it. Documentation only; no code change. * fix(#4249): complete configuredEntrypoints/rollback shape on unparseable settings.local.json An internal adversarial review (agy/gemini-3.8-flash-medium, round 8) found that install()'s settings-json early return for an unparseable settings.local.json omitted configuredEntrypoints and rollbackInstallerMigrations from its result, unlike every other branch. rollbackFinalizedInstallerMigrations reads result.rollbackInstallerMigrations unconditionally, so this branch silently dropped its own installer-migration rollback on a later finalize-stage failure. Completed the return shape: configuredEntrypoints: [] (matching Copilot's equally-early no-entrypoints-yet return) and rollbackInstallerMigrations (already in closure scope). Red-then-green regression test added. * test(#4249): ensure hooks/dist before packing in release-tarball-smoke.install.test.cjs Same internal adversarial review (round 8): this suite's before() packed the tarball directly, without the ensureHooksDist() guard every sibling install-test suite (install.test.cjs, install-minimal-hooks.test.cjs, mcp-catalog-parity.install.test.cjs) already uses. On a clean tree, or run in isolation ahead of a suite that builds hooks/dist itself, this suite's pack would ship a tarball with no hook scripts and fail closed on SMOKE.INIT_FAILED instead of testing anything. * fix(#4249): refresh stale test-timings weight for the codex-config split next's own consolidation split (#4139/#4540) moved tests/codex-config.test.cjs's heavy install()-pipeline blocks into tests/codex-config-hooks.test.cjs, but the CI shard packer's weight table (tests/test-timings.json) was never updated: codex-config.test.cjs still carried its pre-split weight (127783ms, ~18x the suite mean), and codex-config-hooks.test.cjs — which now holds the #3245 block this PR extends with its own #4249 install()-pipeline test — had no entry at all, so the packer would silently underestimate it at the table's median weight (roughly a 9x underestimate against its real cost). trek-e's most recent review flagged a Windows shard timeout in-flight on codex-config.test.cjs, plausibly aggravated by this PR's own addition to that file before the rebase moved it. Re-measured both files locally (node --test --test-reporter=tap, max of 3 runs, matching the table's own max-across-streams methodology) and patched just these two entries — not a full regeneration, which would need real multi-lane CI data this session doesn't have access to. * fix(#4249): register configured-entrypoint-validation tests in the conformance-tier lists next's platform-conformance-tier classifier (#4591/#4598) landed after this branch's last rebase, so tests/configured-entrypoint-validation.test.cjs and tests/codex-config-hooks.test.cjs were never classified, failing lint:ci's gen-platform-conformance-tier --check and both the Linux and macOS conformance suites. * fix(#4249): drop codex-config.test.cjs from the #4733 pinned isolated-set expectation next's #4733 (landed after this branch's last rebase) replaced the static ISOLATED_HEAVY_FILES set with a threshold derived live from tests/test-timings.json, and pins the current derived result in EXPECTED_ISOLATED_UNIT_FILES for regression coverage. That pinned list still named codex-config.test.cjs, whose own weight this PR already dropped from 127783ms to 189ms (after splitting its heavy install()-pipeline blocks into codex-config-hooks.test.cjs) — well under #4733's derived 120000ms bar. The live-computed set correctly no longer includes it; the pinned expectation is updated to match. * fix(#4249): name the rollback consequence in the entrypoint-validation error, and prove Cline's file survives it trek-e's review flagged two Major gaps: the thrown error read identically regardless of which of three real outcomes a runtime hit (nothing persisted / snapshot reverted / config left broken on disk), and no test proved the disclosed "left on disk, unreverted" case for Cursor/Windsurf/ Kimi/Cline — only Codex's revert path was ever asserted. assertConfiguredEntrypoints now tags each invalid entry with its actual consequence, mirrored from docs/how-to/update-gsd.md's existing rollback-matrix disclosure. A new test drives the same aggregate failure through Cline (whose own entrypoint is the one that fails) and asserts its hook file is still on disk afterward. * fix(#4249): close 4 gaps antigravity's adversarial review found in the entrypoint-validation PR One review pass (gemini-3.8-flash-high via the antigravity review lane) against this PR's full diff against next, findings independently verified against source before fixing: - Copilot's install() return object was the only one of 6 runtime branches missing rollbackInstallerMigrations — reachable now that this PR's own aggregate gate runs rollback across every result on any runtime's entrypoint failure, not just Copilot's own. - buildHookCommand's unresolved-bash early return skipped track() entirely, so a win32 install with no Git Bash silently produced an unregistered .sh hook instead of the 'unresolved-interpreter' validation failure configuredEntrypointsForHook's own comment said it would. - release-tarball-smoke.cjs reported a Cycle 4 install failure under SMOKE.INIT_FAILED (Cycle 1's code) instead of the already-existing SMOKE.INSTALL_FAILED. - SCRIPT_PATH_RE excluded whitespace to avoid swallowing a shell command's trailing args, which also truncated any configDir containing a space (e.g. a real "/Users/John Doe/.claude"), silently zeroing the scan. Anchored the match on the already-known configDir prefix instead of a generic absolute-path guess: removes the ambiguity outright rather than patching the character class, and stays a raw-text scan on purpose (it catches a writer that emits a path without registering it — a JSON.parse of the expected schema would miss exactly that case). One suggested finding (test-timings.json "missing" the new test file) was verified false — that table only holds measured CI timings, populated after a file's first real run — and one Ponytail suggestion (a JSON.stringify dedup key) was rejected as it would reintroduce a real, if narrow, key-collision risk for no benefit. * fix(#4249): fix fork CI red from a stale changeset pr field and an unquoted docs/ comment changeset-lint requires pr: to match the PR it runs on (16 on the fork, not the eventual upstream number) — rehearsal-branch convention already established earlier in this PR's history. lint-docs-guard-registration's quote-pairing heuristic doesn't require the docs/ path itself to be quoted — it flags a file once ANY quote-delimited span containing "docs/" appears anywhere in it, alongside any real fs read call. A comment ending "...update-gsd.md's rollback-matrix paragraph" supplied the closing quote character (the possessive apostrophe) the heuristic paired with an unrelated single-quoted string earlier in the file. Reworded to avoid the unquoted apostrophe next to the path. * chore(#4249): point the changeset pr field back at the upstream PR Fork rehearsal (PR #16) is green; the real target for this changeset is upstream PR #4249. --------- Co-authored-by: Test <test@test.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-09-16 04:23:53 -04:00
0xdhx	aeac47b95a	fix(#4465): bound /gsd:undo commit selection to the phase directory and HEAD (#4472) ... * fix(#4465): bound /gsd:undo commit selection to the phase directory and HEAD `--phase` documented a primary path reading `.planning/.phase-manifest.json`, but nothing in the repository writes that file, so the documented fallback was the only reachable path: git log --oneline --no-merges --all | grep -E "\(0*${TARGET_PHASE}(-[0-9]+)?\):" | head -50 That selection has no milestone bound and no reachability bound, and it feeds `git revert --no-commit`. On a project that reuses a phase number it stages deletion of a previous milestone's files, under a confirmation gate that displays only `{hash} — {message}` — the one field that carries no milestone discriminator. Port the #3995 anchor already live in code-review.md: resolve the phase's own directory via `find-phase` (which resolves through planningDir, so it is workstream-correct), take PHASE_START as the first commit adding anything under it, and select over `PHASE_START^..HEAD`. Drop `--all`. Fail closed when no anchor resolves rather than widening to a repository-wide search, and report truncation instead of silently capping at 50. Also resolve the dead manifest read rather than leaving documented-but- unreachable behaviour, and hoist dependency_check onto the workstream-resolved planning root — it read a hardcoded `.planning/ROADMAP.md` and `.planning/phases/`, which are the wrong tree under an active workstream. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B58zMdMYc3n16mfdqMHMqv * fix(#4465): close three defects the pre-create adversarial review found Root-commit off-by-one: `${PHASE_START}..HEAD` EXCLUDES PHASE_START, so when the phase's first commit is the repository root the selection silently dropped it and the undo refused legitimate work. The root branch now selects over `HEAD`. Empty-selection exit status: `grep` exits 1 on no match, and the removed `| head -50` had been masking that rc. Both selection pipelines now end in `|| true` so an empty selection reaches the workflow's own Empty check instead of aborting the block. Truncation stop was documented for MODE=phase only; MODE=plan could still cap silently. Both modes now carry it. Also documents two residuals the review surfaced rather than leaving them implicit: a revision range is ancestry and not chronology, so a pre-phase side branch merged in after PHASE_START stays selectable; and the `--diff-filter=A` anchor does not follow renames, so an archived phase directory under-selects (reverts too little or refuses, never too much). Tests: 12 assertions, negative-controlled. A-H and K-L are RED against the pre-fix workflow; J is RED against the intermediate revision that carried the off-by-one; I is green in both directions by design. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B58zMdMYc3n16mfdqMHMqv * chore(#4465): backfill the changeset fragment's pr field The fragment could not carry `pr:` before the PR existed; both `scripts/changeset/lint.cjs` and `scripts/lint-docs-required.cjs` require it and reported `missing_pr` until now. Both are green with it filled in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B58zMdMYc3n16mfdqMHMqv * chore(#4465): acknowledge the deliberate growth of undo.md The emitted-attribution gate flags undo.md growing 11881 -> 17435 bytes. The growth is the fix: a one-line `git log --all | grep` selection is replaced by an anchored, HEAD-bounded selection for BOTH modes, each with its own fail-closed branch and truncation stop, plus three residuals documented next to the code they qualify rather than left implicit. A workflow document is the executable contract, so the residuals belong in it. Emitted-Drift-Ack-Growth: undo.md — replaces a one-line unbounded commit-subject grep with an anchored HEAD-bounded selection in both --phase and --plan, each with a fail-closed branch and a truncation stop, plus three residuals documented in-workflow (#4465) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B58zMdMYc3n16mfdqMHMqv * test(#4465): execute undo.md's selection fences against a git fixture The shipped regression test matched substrings in undo.md's bash fences. A substring cannot tell a live invocation from a dead one, and this is the boundary/reachability defect class this repo's own conventions want driven with limit-1/limit/limit+1 execution proof. So the fences the runtime runs — sliced out of undo.md by content anchor, never by position — are now replayed with `bash -c` inside createTempGitProject fixtures against the real `gsd-tools.cjs`, the same fence-execution shape #2308 and #2352 use. Ten executed cases: the fixture's own negative control (the retired `--all` grep over-selects the archived milestone and a dead branch), `--phase` and `--plan` selecting only the current milestone's HEAD-reachable instance, the single-milestone selection unchanged, limit-1 (a matching pre-phase commit excluded, PHASE_START itself included), the root-commit branch selecting over `HEAD`, fail-closed on an absent phase in both --phase and --plan (empty PHASE_DIR and UNDO_RANGE), the active workstream's phase directory winning over the root's, and dependency_check's `planning inspect --pick generated_from.planning_root --raw` resolving the workstream root, the project root, and the `.planning` fallback. Skipped on win32 with the #2352 precedent's reason: the fences are POSIX bash driven through `bash -c`; the shape half still runs there. Negative-controlled against the pre-fix undo.md (upstream/next): 11 of 12 shape assertions red, and the executed block fails at fence extraction. Shape test L now pins the >50 refusal message in both modes, not only its heading — the cross-AI round review removed the paragraphs under intact headings and L stayed green; it now fails on that mutation. * fix(#4465): refuse an archived-milestone PHASE_DIR in both undo modes `find-phase` searches the live `phases/` directory first, then every `milestones/v<X.Y>-phases/` directory in ascending version order, and its ambiguity check is scoped to one directory: the `matches.length > 1` test sits inside `cmdFindPhase`'s per-`searchDir` loop (`src/phase.cts`). A phase number that is not live therefore resolves silently to the OLDEST archived milestone carrying one, with no warning. Anchoring there is wrong in both directions at once. The oldest commit adding that path is the archival move, so the phase's real work predates the window and falls outside it, while the window runs forward from that archival through every later milestone -- where the subject grep matches THEIR same-numbered phase. Driven on a two-archived-milestone fixture, `--phase 03` selected v2.0's `feat(03-01): add search index` and `docs(03-01): v2.0 phase plan` and excluded v1.0's own `feat(03-01): implement auth endpoint`. On `git revert --no-commit` that is the cross-milestone contamination this PR exists to close, recurring. Both modes now blank PHASE_DIR on an archived resolution and refuse with their own message, so the existing fail-closed rule stays load-bearing even if the refusal's prose is not honored. The "archived or renamed phase directory under-selects" residual claimed this failure could revert "too little or refuse, never too much". That was wrong in the direction that matters; it is rewritten to cover renames only, and the archival case is recorded as refused rather than disclosed. Tests: a two-archived-milestone fixture, a negative control asserting the unguarded window selects v2.0's two commits and none of v1.0's, refusals in both --phase and --plan, and an inertness check on a live resolution. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * docs(#4465): close the three Minor review items on undo.md and its test Purpose line: it still advertised rolling back "using the phase manifest", the exact `.planning/.phase-manifest.json` mechanism this PR removes. Test B already reads the whole file, so scope is not why it missed this -- B greps the hyphenated `phase-manifest` token, the filename, while the purpose line named the same dead mechanism in prose. Test N pins the <purpose> block itself, which is spelling-independent; widening B's pattern to /manifest/i instead would fire on any future sentence that merely mentions one. Merge-commit anchors: `git log --diff-filter=A -- "${PHASE_DIR}"` does not walk merge diffs by default. A directory added on a side branch is still found -- the side-branch commit that added it is in history -- so the uncovered case is narrower than "introduced via a merge": it is a directory first appearing in the merge RESOLUTION. The information is not absent from history, only unrequested: `git log -m` prints that add once per parent. Recorded as a residual rather than fixed -- the failure is a refusal, and the evil-merge fixture costs more than a safe-direction branch is worth. allow-test-rule marker: suppression is site-scoped, and CONTRIBUTING.md pins the window at MAX_MARKER_LOOKAHEAD_LINES = 8 with only blanks and comments between. The file-header marker sat 43 lines above the first `readFileSync` with requires and a function definition in between, so it was inert for both read sites. Moved to each site. The markers are belt-and-braces today, and NOT because the rule ignores `RegExp.test` -- it handles `regex.test(tracked)` explicitly (no-source-grep.cjs:239, :597-605). Neither read is tracked at all: `looksLikeSourcePath` (:378-390) admits only .cjs/.cts/.js/.mjs/.mts/.ts and UNDO_PATH is a .md, and the second site's reader is `readFileNormalized`, which the rule does not recognise as `readFileSync`. They become load-bearing if either scope widens. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * chore(#4465): record the archived-milestone refusal in the changeset The fragment described the PHASE_START bound and the fail-closed rule but not the archived-resolution refusal added this round, which is a user-visible behaviour change: `--phase N` on a number that is no longer live now stops rather than anchoring on an archived directory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * docs(#4465): disclose the same-number-and-slug residual in undo.md The round's own cross-model body audit found this stated in the PR description and nowhere in the deployed workflow -- which is the half that survives merge, and the half this PR's whole argument says residuals belong in. The anchor is the CURRENT path and `--diff-filter=A` does not follow renames, so a later milestone that re-creates the same literal directory (`03-auth` again, not merely phase `03` again) makes the oldest add at that path the previous occupant's. The archived-milestone refusal added this round structurally cannot reach it: `find-phase` returns the LIVE directory, so nothing is under `milestones/` to refuse. Driven -- v1.0 and v2.0 both using `.planning/phases/03-auth`, v1.0 archived in between: PHASE_DIR resolves live, the guard correctly does not fire, the anchor is `docs(03-01): v1 plan`, and the selection returns all four v1+v2 phase-03 commits. `code-review.md` carries the same residual on the same anchor, where it is read-only; on `git revert --no-commit` it is not, so the note points at `/gsd:undo --last N`. Documented rather than fixed: following renames across a re-created path needs a phase identity that a directory name does not carry, which is the same wall residual 1 hits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * fix(#4465): refuse a LIVE phase directory whose name is also archived Self-found this round, from the cross-model audit of the previous commit: that commit disclosed same-number-and-slug reuse as a residual and asserted closing it needed a phase identity a directory name does not carry. The audit refuted the second half by producing a fix, and it is cheap and safe-direction, so the case is now refused rather than documented. `--diff-filter=A` does not follow renames, so a later milestone that re-creates the same literal directory (`03-auth` again, not merely phase `03` again) anchors on the EARLIER occupant's add commit and the window opens there. The archived refusal added earlier in this round structurally cannot reach it: `find-phase` returns the LIVE directory, so nothing is under `milestones/` to refuse. Driven before the guard -- v1.0 and v2.0 both at `.planning/phases/03-auth`, v1.0 archived in between: PHASE_DIR resolves live, the archive guard correctly stays inert, the anchor is `docs(03-01): v1 plan`, and all four v1+v2 phase-03 commits are selected. That is a previous milestone's work staged for `git revert --no-commit`. The guard needs no identity reconstruction: the same basename present under an archived `milestones/v*-phases/` means the path has been used before, so the anchor is untrustworthy and both modes refuse with their own message. It fails toward refusing a legitimate undo of the reusing milestone, which `--last N` covers; the alternative is reverting the earlier one's commits. Tests: a reused-slug fixture, a negative control asserting the unguarded window reaches back into v1.0 (all four commits), refusals in both modes, and an inertness check on a distinct slug. All three new checks red against the pre-fix fence. Also narrows the changeset, which still claimed selection "no longer" reaches a previous milestone -- true of the archived route, not of this one until now -- and corrects the concurrent-workstreams residual, which claimed the window removed the previous-milestone class "entirely" while this case remained open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * fix(#4465): harden the reused-path refusal against its own false positives The cross-model audit of the previous commit refuted four of its claims. All four were right; this closes them. A REAL BUG: the refusal message interpolated `${PHASE_DIR}` after the fence had already blanked it, so it would have rendered "Phase 03 resolves to , but that directory name is also archived at ...". The live path is now preserved in `PHASE_DIR_LIVE` before blanking, and a test pins that it survives. THREE FALSE-REFUSAL ROUTES, each of which could block a legitimate undo: - `[ -e ]` accepted a regular FILE where an archived phase directory would sit. The evidence the refusal claims is "an earlier milestone used this path", and only a directory is that. Now `[ -d ]`. - The glob `v*-phases` accepted milestone directory names `cmdFindPhase` itself rejects -- its filter is /^v\d+.*-phases$/, so `vnondigit-phases` is not a milestone it would ever resolve. Refusing over one is refusing on evidence the producer discards. Now `v[0-9]*-phases`, in the collision check and in the archived-resolution guard alike. - `${PHASE_DIR%/phases/*}` silently left the path unchanged when it carried no `/phases/` segment, so the scan ran against the wrong root and read as clean. The strip must now have fired. Outside today's producer contract either way -- cmdFindPhase's live output always carries `/phases/` -- so this hardens a claim rather than fixing a reachable defect, and is stated as such. The commit message and the changeset both asserted the guard proves the name is "archived"; before this commit it proved only that some entry existed. Both are narrowed to what the fence now actually establishes, and the changeset headline no longer claims more than the anchor plus the two refusals deliver -- residual 2 (a range is ancestry, not chronology) is untouched by either. Tests: a file-not-directory twin, a malformed `vnondigit-phases` twin, and the preserved live path. All three red against the pre-hardening fence, along with shape test M. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * docs(#4465): disclose what the reused-path scan does not survive A fifth audit pass refuted three claims in the previous commit. Two are wording, one is a real fail-open; none is fixed by more shell, so all three are stated. The wording: `v[0-9]*-phases` was described as MIRRORING cmdFindPhase's /^v\d+.*-phases$/. It is not equivalent -- the glob's `*` matches a newline where the regex's `.` does not, so a directory named `v6<newline>-phases` is accepted here and rejected there. It tracks the filter closely enough to reject the malformed siblings that motivated it; it does not mirror it, and the comment no longer says so. The changeset likewise said the twin is "an archived phase directory", where the check establishes only that a directory of that name exists under an archived milestone -- narrowed to that. The fail-open: this collision check is the only fence in undo.md that relies on pathname expansion (every other one uses `case`, which `set -f` does not affect). Under a runtime with globbing disabled the scan is skipped silently and a genuine collision passes; under `shopt -s failglob` a NON-match aborts the fence. Both sit outside the shell state this workflow assumes throughout, and defending only this fence while the rest of the file assumes defaults would be inconsistent -- so it is a documented residual rather than a hardened one. Also disclosed: the check is conservative at two edges. `[ -d ]` follows symlinks, and an empty directory of the right name counts, so either can refuse an undo a stricter ownership test would allow. That direction is the intended one -- refusing too often costs a `--last N`, refusing too rarely reverts another milestone's work. No test is added. Asserting behaviour under `set -f` would pin a shell state the workflow does not otherwise support, and the two conservative edges are the documented intent rather than defects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * chore(#4465): register the new regression test in the conformance-tier list Round 3's only blocker. The platform-conformance-tier classifier, its generated registry and the test that asserts the registry is fresh all landed together on `next` in `bcd99696d` (#4591 / #4598) on 2026-09-10 08:36 -0400 -- a day after this branch's last push, so the gate did not exist when the branch was last green. This PR adds a unit-suite test file, the classifier's content scan selects it, and a list regenerated on `next` without it is therefore stale the moment the two meet. (The review cites `0b928fe28c` for that landing. That commit is #4592, four hours later the same day, and `git diff-tree` shows it touched only `scripts/ci-test-scope.cjs`, `scripts/gen-platform-conformance-tier.cjs` and those two files' tests -- not the generated registry at all. The date and the diagnosis hold either way.) Regenerated with `node scripts/gen-platform-conformance-tier.cjs --write` on the rebased tree: one line, 267 -> 268 entries. The `--target macos` list is unaffected -- it writes a different file, `macos-conformance-tier.generated.cjs`, and its classifier does not select this test (198, already matching) -- so `lint:generated-sync`'s second conformance-tier link needed nothing. **Two different baselines, stated so the counts are not read as one.** CI's failure on the prior head reads `546 !== 547`, and this commit's diff reads 267 -> 268. Both are correct and they are not the same tree: at the merge commit `54b0b197f` the committed list held 546 entries, so the classifier wanted 547. `4d65c248e` (#4641, "narrow the tier to 28.5%") then landed on `next` at 2026-09-11 17:00 -0400 -- after that CI run started at 13:26Z -- and collapsed the list to 266, with `a2331c01f` taking it to 267. Hence 268 here. The two 547-entry lists are not the same file set: upstream's includes `tests/execute-phase-decimal-arithmetic.test.cjs`. Order matters and is worth stating: the registry is derived from the `tests/` tree, and the base range removed 282 entries from it and added 3 (`git diff --numstat` reports `3 282`; the familiar 279 is the net shrinkage, not the count of entries changed). Regenerating before the rebase would have produced a 547-entry list against a base carrying 267, and a three-way `git merge-file` control over that pair does conflict. Rebase first, regenerate second. This clears both reds on the prior head, not one. `lint-tests` is the one the review named; `test (ubuntu-latest, 24, shard 3/3)` is the same staleness seen through `tests/platform-conformance-tier.test.cjs:278`, which asserts the committed list is fresh. It was the only failure in 1267 tests on that shard, and it completed at 13:38:33Z -- five minutes after the review was submitted at 13:33:54Z, which is why the review recorded the ubuntu matrix as green. Control: removing the added line reds `real tests/ tree classification matches the committed list` (1 of 55, `267 !== 268`); restoring it gives 55/55. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R4EJcuDjaaF1QeeT8AxYbj * test(#4465): pin what find-phase returns for the workstream archive layout Review round 4 found the archived-phase handling blind to the second archive layout, milestones/ws-<name>-<date>/phases/, that `workstream complete` writes. The suite had no fixture for it and its comments described one archive reader where there are two. - Names both readers in the resolver-contract comment: find-phase routes to cmdFindPhase, which admits only /^v\d+.*-phases$/ under milestones/, while the phase locator (listArchiveVersionDirs) also enumerates ws-*. - Adds a ws-* fixture built the way `workstream complete` builds it (the whole workstream dir moved into milestones/ws-feat-<date>/). - Negative control: a re-created workstream with the same phase slug, run without the collision guard, selects the archived generation's commits. - Pins the actual find-phase contract: a phase living only in a ws-* archive resolves to nothing, and nothing is selected. - Pins that an ordinary deleted plan file inside a live phase is not treated as a previous occupant. runFences gains an explicit env override so a test can activate a workstream after the leak scrub. Every test here passes on the pre-fix workflow; the assertions that need the fix land with it in the next commit. * fix(#4465): refuse a re-created phase path by its history, not an archive-layout glob Review round 4: the archived-phase handling encoded the archive layout as a literal `v[0-9]*-phases` glob, while the phase locator owns two layouts. The second, milestones/ws-<name>-<date>/phases/, is what `workstream complete` writes. Driven on the real fences: a workstream `feat` completed into ws-feat-<date>/ and then re-created with the same 03-auth slug resolves to the live path, the collision glob finds no v*-phases twin, the anchor opens on the archived generation's first commit, and --phase 03 selects that generation's commits. The collision check now asks git whether this exact path went EMPTY somewhere in HEAD's history and came back: `git log -m --no-renames --diff-filter=D` over PHASE_DIR, then `git ls-tree -d` at each deleting commit to tell a vacated directory from an ordinary deleted plan file. That answers for every way a path can be vacated -- the flat archive, the ws-* archive, and a phase removed and re-added under the same slug, which no layout glob could see -- without a second reader of the layout to keep in sync. The refusal message now names the commit that vacated the path. The archived-resolution refusal names both layouts. find-phase (cmdFindPhase) searches only the flat archives today, so a ws-*-only phase resolves to nothing and fails closed on the not-found rule; the ws-* arm keeps the refusal correct if find-phase is ever taught the locator's second layout, and a stubbed-resolver test pins it for both modes. The retired glob's shell-state residual (pathname expansion under set -f / failglob) is gone with it. Its replacement residual is documented in-workflow: the history check misses a single commit that both moves the directory away and re-creates it, and over-refuses when a side branch emptied it and the merge kept it. * fix(#4465): run undo's path-scoped git calls from the project root find-phase answers against Found by this round's pre-push review. find-phase prints PHASE_DIR relative to the PROJECT ROOT -- gsd-tools resolves the root before it dispatches -- while the workflow's shell stays wherever the user invoked /gsd:undo. A git pathspec is read relative to git's cwd, so from a subdirectory every PHASE_DIR-scoped call looked in the wrong place: - the anchor (`git log --diff-filter=A`) came back empty, so a legitimate --phase or --plan undo was refused. Fail-closed, but a refusal of valid work, and it dates from round 1; - the collision check's `git log --diff-filter=D` came back empty, so a re-created path was never recognised. The anchor failing too is the only reason this did not over-select. Both modes now take PROJECT_ROOT from `planning inspect --pick generated_from.cwd` -- the directory find-phase itself resolved against -- and run the three path-scoped calls as `git -C "${PROJECT_ROOT:-.}"`. The project root, not `git rev-parse --show-toplevel`: a project need not sit at the top of its repository, and a control test pins that choice. Selection, revert and rev-parse calls are SHA-only and unchanged. Tests: every behavioural test ran from the fixture root, where the two readings coincide. New ones run the fences from sub/dir -- normal selection in both modes, refusal of a re-created path for both archive layouts in both modes, and a dropped plan file NOT refused, which is how a mis-rooted ls-tree would fail (it lists nothing, and nothing reads as "vacated"). Shape test O pins every PHASE_DIR-scoped git call to the root. The two tests renamed in the previous commit are relabelled as false-positive controls: they pass with the collision check deleted, and say so. Reversion controls, all driven: the pre-change undo.md fails 7 tests; one mode's ls-tree mis-rooted fails 3 in either mode; PROJECT_ROOT from --show-toplevel fails 2. * fix(#4465): refuse a phase planned in another repository, and never widen a foreign anchor Found by this round's second pre-push review, against the previous commit. Running the anchor from the project root is right while the project root and the caller share a repository. In a `sub_repos` project they do not: .planning/ lives in a parent repository and the code in child ones, and from a child findProjectRoot returns the parent. The anchor then came from the PARENT's history -- a commit the child does not hold. `git rev-parse "${PHASE_START}^"` failed in the child, the root-commit arm read that as "no parent", set UNDO_RANGE=HEAD, and selection ran over the child's whole history. Driven: both child commits selected, including one older than the phase. Before the previous commit that case resolved no anchor and failed closed; the previous commit made it destructive. Two layers, both modes: - a same-repository refusal in the guard fence: the caller's git directory and PROJECT_ROOT's, each physically resolved, must be the same one (per-worktree, so a linked worktree compares correctly). A different one sets PHASE_DIR_FOREIGN, blanks PHASE_DIR, and the workflow stops with a --last N pointer; - in the anchor: a PHASE_START that is not a commit this repository holds is blanked before the root-commit arm. That ambiguity -- "no parent" read as "root commit" when it can also mean "not a commit here" -- dates from round 1; the previous commit made it reachable. Shape test O now also pins that PROJECT_ROOT is assigned only from planning inspect: a rooted call is only as good as the root it is handed, and a later reassignment passed every spelling check (review, claim 8). Shape test P pins both layers in both modes. Correction to the previous commit's message: it said an empty anchor was the only reason a missed collision could not over-select before that commit. The review drove a counterexample -- a tracked `.planning` path coinciding with the caller's subdirectory resolves a wrong, non-empty anchor -- so that sentence overstated it. Reversion controls, driven: the previous commit's undo.md fails 3 (P, the sub_repos refusal, defense in depth); the refusal made inert fails 2 (P and the refusal; the anchor check alone still keeps the range empty); the anchor check made inert fails 2 (P and defense in depth; the refusal alone still refuses). A verbatim negative control shows the pre-hardening root-commit arm widening the parent's anchor to all of HEAD. * fix(#4465): compare the repository, not the worktree, and anchor only inside HEAD's history Found by this round's third pre-push review, against the previous commit. Its repository gate compared per-worktree git directories. gsd-tools maps a linked worktree with no .planning/ of its own to the MAIN worktree (resolveMainWorktreeCwd), so from such a worktree PROJECT_ROOT is the main checkout: one repository, one object database, two per-worktree git dirs -- and the gate refused a legitimate undo. It now compares the COMMON git directory, physically resolved, which is the repository's identity; a sub_repos child is still a different one and is still refused. Driving that case showed the anchor check was too weak for it. The anchor comes from the main worktree's branch history, and nothing guaranteed the linked HEAD contains it: a linked branch that left main before the phase started would get a window bounded by a commit outside its own history. The check is now "PHASE_START is an ancestor of HEAD" (`git merge-base --is-ancestor`), which subsumes the previous "is a commit here" test -- a missing object is not an ancestor either -- and changes nothing in the ordinary case, where the anchor is read from HEAD's own log. Tests: a linked-worktree fixture (the linked checkout has no .planning/, which is what makes gsd-tools map it to main; the test asserts the mapping happened) is allowed in both modes and selects the phase; the same shape branched before the phase resolves no range. Shape test P pins the common-dir comparison, forbids --absolute-git-dir, and pins the ancestry check. Reversion controls, driven: the previous commit's undo.md fails 3 (P and both linked-worktree tests); per-worktree git dirs restored fails the same 3; the ancestry check replaced by the previous cat-file test fails 2 (P and the branched-before test, which then selects a linked commit whose history never held the phase); no anchor check at all fails 3 (P, defense in depth, the branched-before test). --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-09-15 01:41:35 -04:00
Tom Boucher	ca8d9d4459	fix(#4429): stop a large commit_types config blocking or bypassing the gate (#4723) ... * test(#4429): regression coverage for three defects in the commit hook Failing-first coverage. Every conforming-subject row is red against the unfixed hook, and each defect gets an explicit CONTROL row that reconstructs the pre-fix form and asserts the defect reproduces -- without those, the passing rows would pass with or without the fix. 1. SIGPIPE (the reported defect). The pre-fix first-line extraction used a `head -1` pipeline; once CONFIG_OUT exceeds the 64 KiB pipe buffer printf is killed and `set -euo pipefail` aborts the hook. That fix is already on next -- it landed incidentally in #4537, whose message never mentions #4429 -- and nothing in the tree would notice its removal. 2. regcomp. The commit-type alternation grew with the CONFIGURED list and exceeded bash's 64 KiB compiled-pattern cap. Boundary rows pin the cliff at 6051/6052, with controls on BOTH sides so limit-1 is not vacuous. 3. Ambient subprocess statuses (found by this change's security review). Defects 1 and 2 cannot be separated: each configured type adds len+1 bytes to CONFIG_OUT and len+1 to the alternation, so the smallest payload that overflows the pipe (N=6059) already puts the alternation past the ceiling. The SIGPIPE control accepts either SIGPIPE (141, Linux) or a reported write error (macOS bash 3.2's builtin printf, exit 1). Asserting only the message would go red on every CI lane, since the remote matrix is Linux-only. Named to bucket with gsd-validate-commit-crash-policy.test.cjs, which covers this same hook: lint-test-file-count derives a test's owning module from its filename prefix, and `validate-commit-*` collided with the `validate` module, already at its 2-file cap. Harness note, learned from three vacuous control runs: hooks/lib/git-cmd.js requires ../gsd-core/bin/lib/token-scanner.cjs relative to the hooks dir's parent, so a copy in a bare tmpdir fails open and returns 0 for any input. The layout symlinks gsd-core beside the copy, and every row that can prove it asserts the run was substantive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4429): bound the commit-type regex and isolate subprocess statuses Two fixes in the same file, both of the same shape: a value computed for one purpose was being read as authority about something else. 1. The commit-type alternation could not be compiled. COMMIT_TYPE_ALT joined every CONFIGURED type into one regex, so the pattern grew without bound. bash caps a compiled pattern at 64 KiB. Bisected on bash 3.2.57 (this repo's macOS target): a 65504-byte alternation compiles, 65515 fails. `[[ =~ ]]` returns 2 on a compile failure, and `if !` cannot tell that from "the subject does not conform" -- so the hook blocked a valid `feat(auth): ...` with CONVENTIONAL_COMMITS_VIOLATION while printing `feat` in its own valid_types. Match the shape with a fixed-size pattern, capture the type, then test membership against the COMMIT_TYPES array. The character class is exactly the `^[a-z][a-z0-9-]*$` safe-token filter the config loader already applies, so it captures every type that can legally reach COMMIT_TYPES and no token that cannot. Review verified equivalence over 46 handcrafted plus 6000 randomized adversarial subjects against a type list containing prefix-overlapping, digit-bearing and trailing-hyphen types: zero divergences. The loop adds no subprocess and no pipe, which is the hazard class #4429 is about. COMMIT_TYPE_ALT is now unused and removed. types pre-fix `feat(auth): ...` fixed 10 accept accept 6051 accept accept 6052 BLOCK accept 20000 BLOCK accept 2. Subprocess statuses were inherited from the environment. Each status is captured as `... || VAR=$?`, which assigns ONLY on the failure branch; on success the variable kept whatever it already held, and `${VAR:-0}` defaults only when unset or empty. So an EXPORTED CONFIG_STATUS, CMD_STATUS or CLASSIFY_STATUS -- from a CI wrapper, a .envrc, or another hook -- survived into the success path and was read as "the subprocess failed". Since the hook fails OPEN on a genuine subprocess failure by design (#3838), the result was a silent bypass. Measured: `CLASSIFY_STATUS=3 git commit -m "nope: bad"` printed "validator disabled for this call" and exited 0. The three are now initialised before use. The fail-open path is unchanged and verified byte-identical to origin/next with a failing node. hooks/dist/ is gitignored and rebuilt from hooks/ by scripts/build-hooks.js, so there is no second copy to sync. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4429): register the new suite with the conformance manifests Both conformance-tier manifests embed the test-file list, so adding a test file makes them stale. Regenerated with their own generators: node scripts/gen-platform-conformance-tier.cjs --write node scripts/gen-platform-conformance-tier.cjs --target macos --write Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#4429): pin both fail-open-prone controls to their named cause Two rows in the ambient-status block asserted `status === 0`, which the hook also returns when the harness layout is broken -- so either row could have passed for entirely the wrong reason. This is the same vacuity trap the rest of the suite already guards, applied inconsistently to the rows added last. Measured, rather than reasoned about: genuine ambient bypass (pre-fix hook, CLASSIFY_STATUS=3) rc=0, no CLASSIFIER_THREW orphaned layout (no gsd-core symlink) rc=0, CLASSIFIER_THREW genuine fail-open (node shim exits 3) rc=0, no CLASSIFIER_THREW So assertSubstantive separates the intended cause from the harness failure in both rows, and each now pins its pass to the cause it names. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#4429): stop asserting a macOS-only regex cap on every platform First verification run was RED: 45636/45638 passed, both failures in this new suite on linux-node24. Cause is mine -- I measured the compiled-pattern ceiling on macOS and encoded it as a cross-platform expectation. Measured in the tester image itself: engine 6051 6052 20000 bash 3.2.57 / BSD libc (macOS) compiles rc 2 rc 2 bash 5.2.15 / glibc (Linux) compiles compiles compiles (228943 B) glibc has no reachable cap, so the regcomp defect cannot occur there and the control asserting a block at 6052 was red for a behaviour the platform cannot produce. The control now calibrates at runtime: it runs the pre-fix form and, when this engine compiled the alternation, it SKIPS with a message naming the reason rather than asserting. Skipped out loud, never silently passed -- a green row there would read as "the defect is covered" on a platform where it cannot occur. Both branches verified: the capped branch asserts (macOS 17/17, zero skipped), and the uncapped branch was exercised by forcing the payload to a size that always compiles, producing a skip and not a failure. Consequence stated rather than hidden: the remote matrix is Linux-only, so this one control is skipped in CI and really runs only on a macOS workstation. The rows that run everywhere are the ones carrying the regression weight -- the shipped hook accepting a conforming commit at every payload size, the gate still blocking unknown types, the SIGPIPE control, and all seven ambient-status rows. Note this also narrows the coupling claim: SIGPIPE and regcomp are coupled only on a capped engine. On glibc the SIGPIPE defect is directly testable without the regcomp fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4429): scope the regex-cap claim to the platform it applies to The changeset told users the validator "built a regular expression bigger than bash can compile" past ~6,000 configured types. That is false on Linux: glibc compiled a 228,943-byte alternation without complaint, so a Linux reader would have been misled about their own exposure. These are user-facing release notes, so the claim is now scoped to macOS (bash 3.2 / BSD libc) and says explicitly that glibc was never affected by this half. The hook's own comment led with the same overstatement -- "bash caps a compiled pattern at 64 KiB" -- before qualifying it. Reworded so the first clause states what is actually true: the limit is a property of the platform's regex engine. Text only; no behaviour change. Suite 17/17, eslint and lint:ci clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4429): backfill changeset PR number (#4723) * chore(#4429): backfill changeset PR number (#4723) --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-14 05:43:59 -04:00
Tom Boucher	c0b2a05d2f	fix(#4594): one canonical dispatch-identity owner — the emitted format and the parser that reads it back (#4693) ... * fix(#4594): give dispatch identity one owner for the emitted format and its parser The isolation guards decided whether a run-scoped sentinel applied to a dispatch by regex-scraping model-authored prose. The scrape returned values in a different namespace from the ones the sentinel records, so the comparison could never succeed: sentinel { phase: "03", plan: "03-02-hardening" } <- $PHASE_NUMBER / $plan_id prose "Execute plan 02 of phase 03-auth." scraped { phase: "03-auth.", plan: "02" } <- greedy (\S+), both wrong #4594 reports only the phase half. Measured against a real phase-plan-index run, plans[].id is phase-prefixed, plan-numbered AND slugged, while the prose carries a bare in-phase plan number — so the plan field mismatches too, and the Claude path is dead rather than latent. A fresh sentinel was therefore discarded on every executor dispatch and every legitimate ISOLATION=none degrade was denied, leaving the work unrun. hooks/lib/dispatch-identity.js is now the single owner of both halves. The two prompt-body producers emit a canonical marker carrying the same shell values the sentinel records, so producer and consumer agree by construction. The prose frame stays as a fallback, bounded by the phase-token grammar ADR-2121 owns and deliberately reporting no plan — an absent identifier means "cannot compare" and is safe; a wrong one is a false mismatch and is not. The prose sentence itself is byte-identical: the executor agent reads it too, so the marker is purely additive (Hyrum's Law). An inapplicable sentinel is now named in the guards' deny reason instead of being dropped silently — the silence is why this survived three producers and two consumers unnoticed. Interpolated values come from a sentinel file and from prompt text, so both are length-bounded and stripped of control characters. ADR-4630 locks the seam and maps the epic's three phases. Refs #4630 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4594): resolve eight review findings across the dispatch-identity seam Three orthogonal review engines ran on 43418af144 — the code-review skill's Standards and Spec axes, and an isolated adversarial security pass — plus a self-review of the committed diff. Every finding is fixed here; none deferred. F1 (major, reproduced). A keyless or unknown-key-only marker — the literal "[gsd:dispatch]" or "[gsd:dispatch run=..]" — matched the marker grammar and returned source:'marker' with both fields null, suppressing the prose fallback entirely. Any prompt text containing that literal silently disabled identity narrowing, so a fresh sentinel applied to a dispatch it was never scoped to, defeating #3045 SECURITY F2. Prompt text is attacker-influenceable. A marker that yields neither recognized key is no longer a marker: the scan continues to later markers, then later texts, then prose. Forward-compatible tolerance of unknown keys is unchanged. F2/F3 (major). The first cut duplicated sanitizeForReason, describeSentinelDiscard and REASON_INTERPOLATION_MAX_LEN byte-for-byte across both guards — the exact defect class this epic exists to delete, and with no cold-load justification, since both hooks already require hooks/lib/. They now live in hooks/lib/isolation-deny-reason.js, and buildSentinelDiscard lives in isolation-sentinel.js beside the comparison it mirrors, returning the nested {sentinel:{phase,plan}, dispatch:{phase,plan}} shape instead of a bespoke four-field bag that renamed the pairs already flowing through the seam. F4 (hard violation). The visibility test asserted on the deny reason's prose. CONTRIBUTING.md prohibits raw text matching on hook output, which is why every deny carries a stable reason_code. The discard is now a structured sentinel_discarded field on each hook's stdout JSON, and the test asserts that; the sentence stays for the operator but is no longer the contract. F5 (hard violation). The 64-character truncation limit had no boundary coverage. 63/64/65 are now exercised against the single consolidated helper. F6 (minor). sanitizeForReason stripped C0/C1 controls but not U+2028/U+2029 or the bidi overrides, so a crafted value could still reflow or reverse the message. Both classes are stripped, with a test each. F7 (major). The producer/template parity test was vacuous — it rendered a marker and re-parsed its own output, and would have passed with both templates deleted. It now reads the two workflow templates, extracts each marker line, substitutes the measured values and asserts the owner's parser returns them. Proven red by deleting one template's marker line before being proven green. F8 (doc). ADR-4630 and the design notes claimed the marker is guaranteed on the orchestrator-worktree path because that prompt is built in shell. It is not: executor-isolation-dispatch.md:131 says plainly that those are template placeholders, not shell variables, so {plan_id} is model-substituted there too. A false guarantee in a design lock is worse than a stated limit. Both documents now say the marker is model-substituted on both paths and that the prose fallback is the real floor everywhere. The "3 workflow templates" count was also wrong — 3 prose sites across 2 files, 2 of which carry the marker. Refs #4630 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4594): refresh the compact-content baseline and acknowledge execute-phase.md growth Refs #4630. The dispatch-identity marker and its substitution note grew gsd-core/workflows/execute-phase.md by 525 bytes (91846 -> 92371), which drifts two real-tree guards that lint:ci does not run: - tests/benchmark-compact-content.test.cjs asserts the committed baseline is "up to date"; the split for execute-phase.md moved off 25827 -> 25952 and on 23576 -> 23701, taking its compaction reduction 8.72% -> 8.67%. Baseline regenerated with scripts/benchmark-compact-content.cjs --write. - tests/emitted-attribution.test.cjs requires a growth acknowledgment trailer for any emitted file that grows, keyed on the bare filename. Added below. The growth is two additions and no rewrites: the [gsd:dispatch ...] marker line inside the Agent() prompt's <objective>, and the note telling the orchestrator to substitute {plan_id} with the plan's id verbatim. Both are load-bearing -- the marker is what lets a guard hook match a dispatch to the sentinel the per-plan gate wrote, and without the note the orchestrator has no instruction telling it the value must not be paraphrased. Emitted-Drift-Ack-Growth: execute-phase.md — adds the canonical [gsd:dispatch] identity marker and its {plan_id} substitution note, which the isolation guards compare verbatim against the run-scoped sentinel (#4594) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4594): set changeset fragment pr to 4693 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-13 15:36:08 -04:00
sim	9c20b7b40a	fix(#4652): use the validated path, collapse the duplication, correct two false claims ... Seven findings from the two-axis review, all fixed in place. THE ONE THAT MATTERS: cmdTodoComplete validated sourcePath and targetPath and then ran every fs call against the RAW strings — existsSync, statSync, readFileSync, platformWriteSync, unlinkSync, and the dry-run path payload — never sourceCheck.resolved / targetCheck.resolved. That is the exact "validate one path, use another" shape ADR-4650 names as the defect this epic exists to prevent, and it is the same bug this phase had just fixed in check-command-router. Committed inside the fix for it. All I/O now uses the resolved paths; user-facing messages still echo the raw filename, never a resolved absolute path. A VACUOUS TEST, and the false doc claim it was propping up. The test "[RED #4327] an absolute path outside the project is rejected" would have passed with ZERO containment logic: path.join(pendingDir, '/abs/outside/x') yields <pendingDir>/abs/outside/x — Node does not let a later absolute segment escape — so the name is FOLDED under the root, passes containment, and simply 404s. The test only ever observed "Todo not found". It now asserts what is actually true and actually valuable: an absolute name is neutralized, and the real outside file is not read, not moved, and still present afterward. docs/CLI-TOOLS.md claimed such a path "is rejected as a usage error", which was false; it now describes the fold-under-root behavior. Traversal and embedded separators ARE rejected, and those claims stand. DUPLICATION THIS EPIC EXISTS TO REMOVE. resolvePath already did isAbsolute-or-join + validatePath + reject; cmdGapAnalysisPlanPost and cmdCheckPredicate each re-inlined the identical triplet in the same file. Both now call resolvePath. Cost, stated rather than hidden: its generic message replaces the two sites' distinct "phase-dir escapes…" wording. The message still names the offending input, and one predicate with one message is the point. SYMLINK COVERAGE was required by #4652's "Done when" and was missing. Added for both the todos root and --phase-dir, skipping cleanly on EPERM so the Windows lanes do not fail where unprivileged symlink creation is disallowed. Both fast-check properties were UNSEEDED. Seeded now. The changeset named "check decision-coverage-plan" as a boundary; that is a caller of the shared resolvePath, which the body never mentioned. Corrected. DISCLOSED, not hidden: ctx.phaseDir is now always the resolved ABSOLUTE path, so ${PHASE_DIR} interpolation and the "not found in <targetDir>" message show an absolute value where a relative --phase-dir previously produced a relative one. That is an observable output change. A test pins it and docs/reference/gate-predicates.md states it. Also regenerated scripts/lib/platform-conformance-tier.generated.cjs and its macos twin — the new tests changed check-predicate.test.cjs's tier classification. Caught by npm run lint:ci locally rather than by a bench run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-12 09:43:50 -04:00
Tom Boucher	a2331c01f1	fix(#4568): widen the phase-number regex to accept N-segment ids at 6 shell/markdown sites (#4646) ... * test(#4568): pin the N-segment phase-grammar defect across all 6 shell/markdown sites Manually traced against the current tree: the validating regex at code-review.md rejects a 3-segment id (23.1.2), and execute-plan.md's extraction truncates a 23.1.2-01-PLAN.md filename down to 1.2-01. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4568): widen the phase-number regex to accept N-segment ids at all 6 shell/markdown sites Widens `?` to `*` on the dotted-segment group at all 6 sites (byte-identical behavior for 1- and 2-segment ids, character class unchanged): code-review.md, code-review-fix.md, gsd-code-fixer.md, gsd-code-fixer.compact.md (validating sites, plus their comment/error-message text), execute-plan.md's plan-filename extraction, and plan-phase.md's --research-phase flag capture. Also disambiguates the nsegment-phase-grammar test's plan-phase.md anchor, which was matching an unrelated earlier `--research-phase` occurrence (line 77's generic-value capture) instead of the targeted site (line 131). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4634): extend lint-phase-id-drift to ban the single-segment phase regex in workflows/ and agents/ Adds findSingleSegmentPhaseRegexDrift, banning the bounded `[0-9]+(\.[0-9]+)?` shape (and its \d/doubled-backslash near-variants) on any phase-carrying line across gsd-core/workflows/**/*.md, gsd-core/references/**/*.md, and the newly-scanned agents/**/*.md, sanctioned the same way as the existing shell-arith rule. Wired into scanAll; confirmed zero violations against the real tree post-#4568 fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4568): add Fixed changeset Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: regenerate conformance-tier manifests for the new test file The emitted-attribution gate also flags 4 files growing: code-review-fix.md (+21 bytes), code-review.md (+21 bytes), gsd-code-fixer.compact.md (+9 bytes), gsd-code-fixer.md (+6 bytes). The growth is the fix itself: each site's validation regex widened from a bounded single-optional-dotted-segment shape to the unbounded form, and the accompanying comment/error-message text grew by a few characters to mention the new 3-segment example. Emitted-Drift-Ack-Growth: code-review-fix.md — widens the phase-number validation regex from a bounded single-dotted-segment shape to accept N-segment ids, and adds a 3-segment example to the comment/error text (#4568) Emitted-Drift-Ack-Growth: code-review.md — widens the phase-number validation regex from a bounded single-dotted-segment shape to accept N-segment ids, and adds a 3-segment example to the comment/error text (#4568) Emitted-Drift-Ack-Growth: gsd-code-fixer.compact.md — widens the padded_phase validation regex from a bounded single-dotted-segment shape to accept N-segment ids, and adds a 3-segment example to the error text (#4568) Emitted-Drift-Ack-Growth: gsd-code-fixer.md — widens the padded_phase validation regex from a bounded single-dotted-segment shape to accept N-segment ids, and adds a 3-segment example to the comment/error text (#4568) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4568): backfill changeset pr number to 4646 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-11 17:04:28 -04:00
Tom Boucher	4d65c248e5	fix(#4641): make test-conformance the sole Windows selector and narrow the tier to 28.5% (#4643) ... * test(#4641): failing-first tests for the tier ceiling and a single Windows selector Tests only, committed ahead of the implementation so the RED run is real. - tests/platform-conformance-tier.test.cjs: tier-size ceiling asserted as a ratio against a live denominator (Windows 33%, macOS 25%); per-helper negative cases proving seam calls and path-call-plus-slash-literal are not platform signals; positive pins that genuine platform content, seam-bypassing spawns, chmod and symlink still classify in; macOS signal set and generated list unchanged. - tests/ci-full-lane-sharding.test.cjs: the test job has zero windows-latest rows and test-conformance still has 3 windows + 1 macOS. - tests/ci-test-scope.test.cjs: windows_tests is absent rather than empty, a non-tier test file no longer forces full_matrix, a RULE-pulled windows-hint test does, and resolveSelection rejects the retired windows scope. Refs #4589, #4591, #4592, #4593, #4603 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): delete the second Windows selector and narrow the conformance tier Epic #4589's goal — the OS-agnostic bulk on Linux, a small explicitly-scoped conformance tier on real Windows/macOS — was not met. Measured on PR #4640 (run 34618834118): 7 non-Linux jobs, a 546/930 (58.7%) "tier", and 5 of 7 changed test files running on a real Windows runner twice. Two selectors, only one in the epic's scope. The test job's three scope:windows shards predate the epic (#494, sharded #3057) and gate on product_changed, not full_matrix, so they fire on every product PR whatever Phase 3's classifier decides. They are deleted; test-conformance becomes the sole Windows selector, as it already was for macOS. Non-Linux jobs 7 -> 4. Gating the lane instead was rejected as provably redundant: for a test file reachesConformanceTierOrSeam is literally CONFORMANCE_TIER_FILES.includes(file), and that same predicate sets full_matrix, which turns test-conformance on. Every file a gated lane would run is already covered in the same run. The lane's one non-redundant residue -- RULE-pulled tests matched by the isWindowsHint filename heuristic -- is ported into reachesConformanceTierOrSeam so it sets full_matrix instead of feeding a parallel lane. Two detectors matched the repo's own test idiom rather than any platform signal and carried 226 of the tier's sole-signal membership against 41 for the other eight: process-seam-subprocess (335 files, 118 unique) matches the tests/helpers.cjs entry points nearly every CLI test uses, and going through the seam is the opposite of a platform signal since shell-command-projection takes platform as an injected parameter; hardcoded-path-vs-path-call (328, 108) needs only a path call anywhere plus a slash literal anywhere, and that class is already enforced by ADR-1703's Linux-runnable ESLint rules. Both are removed. Tier 546 -> 254 (27.3%). src/ reachability is unchanged at 28 files, measured. Adds the size gate Phase 2 never had, as a ratio against a live denominator so it cannot stop binding as the suite grows. 292 files leave real-OS Windows execution. The drop-out set was audited: 14 have a platform-suggestive filename and all 14 are static source-text analyses or seam-mediated CLI tests. raw-child-process was investigated as a suspected false negative and left unchanged -- relaxing it adds 13 files, all false positives. macOS is untouched: MACOS_CATEGORIES is a separate array and the regenerated macos-conformance-tier.generated.cjs is byte-identical at 196 files. Fixes #4641 Refs #4589, #4591, #4592, #4593, #4603 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): register the new ADR path in the docs-guard exempt baseline tests/ci-test-scope.test.cjs references docs/adr/4641-windows-selector-consolidation.md in a comment justifying the retired windows scope; lint-docs-guard-registration tracks that reference set, so the baseline needs the new path. Verified the exemption still holds: the path is prose, not a filesystem read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): make the escalation tier-backed and drop every hardcoded count Three follow-ups from measuring the first pass rather than trusting it. The windows-hint escalation now requires tier membership as well as the filename hint. Setting full_matrix runs test-conformance, which runs only the tier; escalating on a test that is NOT in the tier costs four jobs and still never runs that test on Windows. Measured over the 16 RULES entries the narrowed predicate fires on exactly the same rules today, so this is correct-by-construction rather than a behavior change. The broader variant -- escalate on any tier member a rule pulls in, ignoring the hint -- was measured at 14/16 rules and rejected as over-broad. Removes the hardcoded counts. A hardcoded macOS tier length of 196 broke as soon as the rebase pulled in one new test file from #4253, which is the whole argument against them: the ceilings are ratios against a live denominator, the committed lists are pinned by comparison against a fresh classification of the live tree, and the three named probe files now assert on their SIGNAL rather than on membership in a literal list -- asserting by filename is the exact error this PR fixes in the classifier. Regenerates both lists against the rebased tree. Same-tree figures are now 547 -> 255 of 931 eligible (58.8% -> 27.4%), 292 entries removed and none added; macOS is unchanged at 197 with a zero-line diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): restore real-shell-spawn coverage and repair assertions the narrowing broke An isolated adversarial review found a real false negative. Removing the blanket process-seam-subprocess detector also removed the only coverage for tests that spawn a REAL shell: tests/helpers/process-seam.cjs's runHook spawns options.interpreter via real spawnSync, so runHook('-c', [script], { interpreter: 'bash' }) runs a real bash binary executing a shell script extracted from workflow markdown. The seam argument holds for src/shell-command-projection.cts, which takes platform as an injected parameter; it does NOT hold for the test helpers, which spawn real binaries. Conflating the two is what made the blanket detector look purely noisy -- it was 99% noise wrapping a real signal. Adds a narrow shell-interpreter-spawn category keyed on a real interpreter option. Measured 2026-09-11: 33 files match, 9 were outside the tier and are added back, taking it 255 -> 264 of 931 (27.4% -> 28.4%), still under the 33% ceiling. All 9 confirmed by reading the matching source line, zero comment or fixture matches. runGit-alone and non-node-spawnSeam alternatives were measured and rejected -- each adds 9 files but misses the counterexample entirely. Fixes a real bug the suite caught: jobs.test is ubuntu-only now that its scope:windows rows are gone, so it must wire GSD_STRICT_LIVE_CONFIG_GUARD strictly rather than carrying the Windows report-only carve-out. The carve-out now lives solely on test-conformance, whose matrix does include windows. Repairs seven pre-existing assertions the category removal invalidated, preserving each case's purpose rather than deleting coverage, and converts the last hardcoded tier bounds to live-derived ratios -- including the macOS sanity range that was still a magic [100, 350]. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): keep the confinement test on a real OS via a documented allowlist A security review found tests/external-descriptor-confinement.test.cjs had dropped out of the Windows tier. It must stay in, and no content signal can express why: it exercises isPathConfined (src/external-descriptor-trust.cts), which uses the AMBIENT path module -- path.resolve(root, target) and path.sep -- with no injection. Its win32 semantics (drive letters, UNC, separator) are only reachable by actually running on Windows, and it is a security-relevant write-confinement gate. A content classifier cannot see 'this module reads the ambient path module', so no regex belongs here. Adds ALWAYS_REAL_OS, a Map of path -> recorded reason, unioned into the Windows tier only. A Map rather than a list so an entry without a reason is impossible by construction, and tests assert every entry names a file that exists on disk so a stale entry fails loudly instead of rotting. This is the centrally- enumerated single source of truth epic #4589 Phase 2 asked for and ADR-1703's portability-vocab.cjs already models -- deliberately not a heuristic. Windows tier 264 -> 265 of 931 (28.5%), still under the 33% ceiling. macOS is untouched and byte-identical: the win32 concern does not apply to a POSIX runner, and a test asserts the allowlist does not leak into that tier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): inject the path impl into isPathConfined and correct the ADR count Two review findings, both fixed rather than dispositioned. A security review found tests/external-descriptor-confinement.test.cjs had left real-OS execution. The allowlist pinned it back, but that only restored INCIDENTAL coverage: isPathConfined used the ambient path module, and its test carried POSIX-only literals, so a win32 confinement escape was unverified on every platform including Windows. isPathConfined now takes an optional third parameter carrying the path implementation, defaulting to the ambient module. Blast radius is CRITICAL -- 53 affected symbols across 19 files -- so the change is purely additive and every existing two-argument caller is byte-identical. Tests now inject path.win32 and path.posix, covering a different drive letter, a cross-drive absolute, backslash and forward-slash traversal, UNC, and the startsWith prefix-boundary bug (.gsdEVIL against root .gsd) on both separators. Proved load-bearing: dropping the + p.sep from the prefix check fails exactly the two boundary cases and nothing else. Callers' suites 149/149. The spec review caught an off-by-one: the ADR narrated a 264-file tier while the committed list holds 265. The ADR now records the full chain 547 -> 255 -> 264 -> 265 (28.5%). Also corrects a stale comment in scripts/docs-guard-registry.cjs that narrated classify() as zeroing windows_tests, a key this change removes -- kept as historical narration but labelled as such. Refs #4641 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#131): make the unwritable-HOME test actually test something Found by sweeping for the root-bypass class after fixing commit-files-deletion. This one is the silent variant, and it was broken twice over. First, the condition: the test made a fake HOME unwritable with chmod 0o500. The gsd-test Docker bench runs as root, root bypasses mode bits, so HOME stayed writable and the hostile condition never existed. Replaced with a HOME whose PARENT is a regular file, so every write under it fails ENOTDIR at the VFS layer for every uid -- no permission check is involved at all. Second, and more fundamental: the probe was npm --version, which on npm 11.19.0 performs zero filesystem I/O against HOME. Proven rather than assumed -- neutralizing runNpm()'s isolation turned the sibling test red while this one stayed green, so its assertion could never detect the regression it guards, on any uid, with or without the condition fix. npm config get cache was tried next and proved vacuous the same way (it only string-resolves the path). The probe is now npm cache verify, which really does mkdir _cacache under HOME. Re-proved load-bearing after the change: with isolation neutralized the test now fails with ENOTDIR on <blocker>/home/.npm/_cacache. tests/helpers.cjs was restored and verified diff-clean; suite 13/13. Refs #4641 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): correct the net drop-out figure in ADR-4641 The Consequences section still said 292 files leave real-OS Windows execution. That was the count before the narrow shell-interpreter-spawn replacement restored 9 and ALWAYS_REAL_OS pinned 1. Net is 282. Also names both real-binary categories rather than only raw-child-process, and clarifies that the 14-file filename audit was against the 292 initially dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): record the rejected concentration ceiling and its measurement Applying Goodhart's own question to the new ceiling -- how would you make this metric look good without improving what it represents -- surfaces a real weakness: a ratio can be satisfied by inflating the denominator, so adding OS-agnostic tests loosens it without narrowing the tier. The obvious companion gate was a sole-signal concentration ceiling, since the original defect was one detector carrying half the tier. Measured and rejected: peak concentration post-fix is raw-child-process at 53/265 = 20.0%, against the historic offenders at 21.6% and 19.8%. Any threshold above 20% misses the original defect; any threshold below it fails on a legitimate category. The discriminator is whether a signal is platform-meaningful, which no threshold encodes. Weakness disclosed rather than covered by a gate that does not bind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4641): add the changeset fragment for the confinement-check change changeset-lint failed on PR #4643: the PR touches user-facing paths and carried no fragment. The earlier no-changeset call matched #4604's CI-only precedent and was correct then; it was not revisited once the PR grew a src/ change, which is my miss. The fragment describes the real user-visible improvement: the external-descriptor write-confinement check's Windows semantics are now verified deterministically rather than only when the suite happened to run on Windows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): correct the tier count in TESTING-SUITES.md Said the tier narrowed from 546 to 254. The final committed list is 265 of 931 eligible (58.8% -> 28.5%) after the shell-interpreter-spawn replacement restored 9 files and ALWAYS_REAL_OS pinned 1. Same error class the spec review caught in the ADR, in a live reference page rather than a dated record, so it states the current truth rather than carrying an amendment note. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): record the measured aggregate from real CI job lists Epic #4589's closeout asserted its reduction from a static count; #4641's acceptance criterion asks for a figure read off a real run. Recorded here: test.yml job count 21 -> 15 and non-Linux 7 -> 4, comparing PR #4640's run against this PR's own. Against the true pre-epic baseline of 9, that is 9 -> 4. Also states the caveat that a PR's total CHECK count is not a clean before/after comparison, since many gates are path-scoped and this change touches a broader path set -- the like-for-like figure is the test.yml job count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): compare job totals the same way on both sides The measured-aggregate table put #4640's COMPLETED run total (21) against this run's count at matrix-expansion time (15). Those are not the same measurement: the completed total includes the post-test Coverage gate and baseline-publisher jobs. Counted identically, it is 21 -> 17. The load-bearing figure, non-Linux jobs 7 -> 4, was correct and is unchanged. Called out in the table rather than silently corrected -- comparing two differently-derived numbers is exactly the error class this ADR is about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): record measured conformance wall-clock and date the stale counterfactual Adds the per-job durations from both runs. The honest read is that this is a correctness win more than a speed one: file count fell 52% but wall-clock only 9-29%, because what was removed were the cheap static tests and what remains is concentrated in expensive spawn-heavy work. Stated explicitly so nobody expects a future narrowing to buy time proportional to file count. The load-bearing figure is windows shard 3/3: 40m24s against a 45-minute cap on the 547-file tier -- 90% of the cliff #869 and #3057 were both filed about -- pulled back to 31m27s. macOS moved the wrong way (17m48s -> 21m02s) while its tier was UNCHANGED at 197 files, which fixes that as runner variance and is noted as a caution against reading a single duration as signal. Also dates the symlink-keyword counterfactual, which cited a 254-file tier from before the replacement category and allowlist took it to its final 265. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): re-measure against the rebased tree and disclose the allowlist's zero next gained #4644 mid-flight, so every absolute count shifted. Re-measured on the tree this actually ships against (932 eligible): 548 -> 257 by detector removal, 257 -> 266 once shell-interpreter-spawn restores 9. Net 282 removed, 9 restored. macOS 198, unchanged by this PR. The percentages did not move across three rebases (58.8% -> 28.5%), which is the whole argument for expressing the ceilings as ratios rather than counts -- noted in the ADR since it is now evidence rather than assertion. Also discloses that ALWAYS_REAL_OS now contributes ZERO files: this PR's own win32 test cases introduced the literal win32 into the pinned file, so it classifies in on content via win32-darwin-literal. The entry stays and the reason is written down, because the file's real-OS need is a property of the code under test (isPathConfined reads the ambient path module), not of the test's text -- the text that currently saves it is incidental and could be refactored away silently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-11 17:00:11 -04:00
Tom Boucher	db4d8a9bae	fix(#4619): execute-phase computes decimal/N-segment phase numbers without breaking shell arithmetic (#4644) ... * fix(#4619): execute-phase computes decimal/N-segment phase numbers without breaking shell arithmetic $((10#${PHASE_NUMBER})) is a hard bash/zsh syntax error when PHASE_NUMBER is decimal (01.1, from an inserted phase) or N-segment (23.1.2) — neither is valid shell-arithmetic syntax at all, and the failed expansion aborts the rest of the snippet in a non-interactive shell. safe_resume_gate runs unconditionally before trusting STATE.md or dispatching any executor, so execute-phase failed at its own gate before the first executor on any decimal phase, regardless of workflow.tdd_mode. Regression from #4194. Fixes all 4 sites: safe_resume_gate and the TDD gate in workflows/execute-phase.md, the completion-signal spot-check fallback in workflows/execute-phase/steps/completion-reconciliation.md, and the executor gate validation example in references/tdd.md. Each now zero-strips only the leading integer segment into a *_INT variable (via %%.* / # parameter expansion — always valid shell syntax regardless of what follows) and keeps the remainder as an escaped-dot string for the anchored commit- scope regex, exactly as issue #4619 verified in both bash and zsh. A plain integer phase (12, 01) computes byte-identically to before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4619): pin the decimal/N-segment fix and characterize the pre-fix bug Behavioral coverage via real bash execution: the old $((10#01.1)) form throws (characterizes the bug, matching the issue's own reproduction); the new form resolves 01.1 -> 1\.1 and 23.1.2 -> 23\.1\.2, unchanged for plain integers (12 -> 12, 01 -> 1); the resulting anchored ERE matches feat(01.1-03):/test(1.1-3): and correctly rejects feat(01-03):, feat(01.2-03):, feat(011-03):, feat(12-03): for a decimal phase — mirroring issue #4619's own verified table exactly. Updates safe-resume-gate-anchoring.test.cjs's 4 existing source-text assertions (one per site) to the new fixed text. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4634): refine the shell-arith drift detector to distinguish safe from unsafe arithmetic With #4619's fix in place, the guard's original "ban $((10#... outright, match any occurrence" was too blunt: it flagged a comment merely mentioning the pattern in prose, the now-safe $((10#$PHASE_INT)) arithmetic on an already-%%.*-stripped integer, and the always-safe plan-id arithmetic (plan ids are plain integers, never decimal). Refines the detector to skip full-line comments and to only flag a captured variable/placeholder name that contains "phase" and does NOT end in _INT/_int — the naming convention the #4619 fix establishes at all four sites for "already reduced to a safe integer." A plan-id variable was never phase-number arithmetic in the first place and is excluded on the same basis. This closes epic #4634's D6 ("lint-phase-id-drift... passes with no new exemptions") and D7 ("a decimal and N-segment phase id survive an end-to-end execute-phase selection without error") for real — the guard now reports zero violations across all five .cts/.md rules. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: regenerate conformance-tier manifests for the new test file Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4619): cover the plain-padded-integer near-miss matrix too Review found the anchored-ERE near-miss coverage only exercised the decimal case (PHASE_NUMBER=01.1); issue #4619's own worked table also verifies the plain padded-integer case (01 -> PHASE_N=1) against its own near-miss set (matches 01-03, rejects 01.1-03/011-03/12-03). Adds the missing assertion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4619): add Fixed changeset Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4619): correct JS backslash-escaping in safe-resume-gate anchoring test The test's string-literal assertions for the PHASE_FRAC//./\\.} pattern wrote only 2 backslash characters in JS source, which single-quoted-string parsing collapses to 1 real backslash at runtime -- but the workflow/reference files actually contain 2 raw backslash bytes at that position (needed so bash's ${var//pattern/replacement} produces the correct single-backslash output). Write 4 backslash characters in the JS source at all 4 occurrences so the runtime string matches the files' real bytes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4619): refresh the committed compact-content benchmark baseline The new PHASE_INT/PHASE_FRAC arithmetic lines added to gsd-core/workflows/execute-phase.md shifted its committed compaction-ratio baseline. Regenerate via `node scripts/benchmark-compact-content.cjs --write`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4619): note the safe_resume_gate arithmetic growth in the test header The emitted-attribution gate flags execute-phase.md growing 91253 -> 91846 bytes (593 bytes). The growth is the fix: the safe_resume_gate and TDD RED block now derive PHASE_INT/PHASE_FRAC before computing PHASE_N, so a decimal/N-segment phase number (e.g. 01.1, 2.3.1) zero-strips its leading integer segment via base-10 arithmetic instead of forcing the whole value through $((10#...)) and hitting a hard shell syntax error on the first dot. A blank line previously separated the Emitted-Drift-Ack-Growth trailer from the Co-Authored-By trailer below it, which splits git's trailer-block detection: only the last contiguous non-blank run of Key: Value lines at the end of a commit message is recognized as trailers, so the growth ack was silently read as ordinary body text and the differential-attribution gate failed with the growth unacknowledged. Joining the two trailers into one contiguous block fixes it. Emitted-Drift-Ack-Growth: execute-phase.md — adds PHASE_INT/PHASE_FRAC derivation to the safe_resume_gate and TDD RED commit-scope grep so a decimal/N-segment phase number zero-strips its leading integer segment via base-10 arithmetic instead of failing on a non-numeric value (#4619) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4208): replace chmod-based restore-failure injection with a root-proof git shim `tests/commit-files-deletion.test.cjs`'s two restore-failure tests simulated an unwritable index via a `post-index-change` hook running `chmod a-w` on the git dir. That relies on the OS enforcing the *owner's own* permission bits against itself, which uid 0 (a routine identity inside this repo's Docker-based gsd-test benches) does not: every DAC check short-circuits true for root, so the write the chmod meant to block silently succeeds, the restore comes back clean, and the disclosure/rollback behavior under test never actually gets exercised. This is CLAUDE.md's own named anti-pattern for I/O-failure injection ("Cross-platform test IO-failure injection" — chmod tricks fail under root Docker/CI). It is confirmed as the actual root cause here, not a production defect: `src/commands.cts`'s `restoreRemovedEntries`/rollback-disclosure logic (added by #4253, merged just before this run) was hand-traced and manually reproduced end to end on an unprivileged workstation against a freshly built `gsd-core/bin/lib/commands.cjs`, and it already produces exactly the `staging_failed` + "could not be restored" / "could NOT be restored during rollback" results both tests assert. The other `post-index-change`-based tests in this file (a `sleep` to force a timeout; a real `update-index` to flip a restored entry's mode) are unaffected because neither depends on a permission check — consistent with only the two chmod-based tests failing on the real remote run. Replaces the chmod fixture with a fake `git` placed ahead of the real one on PATH that fails only `update-index --add --cacheinfo` — the one call the restore makes — unconditionally, regardless of privilege level. Every other git invocation execs straight through to the real binary, so the rest of each scenario (`rm --cached`, the restore's own `ls-files` verification, etc.) is exercised exactly as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4619): backfill changeset pr number to 4644 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4619): feed the bash fixture script via stdin, not argv, to fix Windows CI Passing the script as a `-c "<script>"` argv element made it subject to Windows' CreateProcess command-line argument encoding, which silently dropped the escaped-dot backslashes before bash ever saw them (observed on PR #4644's windows-latest CI shard: `1\.1` came back as `1.1`). Feeding the same script via stdin instead removes argv entirely from the transport, so there is nothing for Windows to re-encode. POSIX behavior is unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-11 15:47:14 -04:00
Tom Boucher	bcd99696d3	chore(#4591): add platform-conformance-tier classifier + gate CI on it (#4598)	2026-09-10 08:36:13 -04:00