diff --git a/.changeset/bold-birds-wave.md b/.changeset/bold-birds-wave.md new file mode 100644 index 000000000..56ee4f5f8 --- /dev/null +++ b/.changeset/bold-birds-wave.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3124 +--- +The api-coverage detector's negation-suppression check no longer takes superlinear time on long prose, which was hanging the verification gate (#2784, #3127). It also no longer fails to suppress a negated pair ("this phase integrates no external API") when the negation sits in any clause other than the first on a line — a latent offset bug made negation suppression a no-op for every clause after the first. diff --git a/.changeset/daring-tigers-squeak.md b/.changeset/daring-tigers-squeak.md new file mode 100644 index 000000000..313afc89e --- /dev/null +++ b/.changeset/daring-tigers-squeak.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3124 +--- +**`fish_add_path` no longer skips a directory whose name starts with a dash** — fish parses a leading-dash token as an option, so the suggested command silently added nothing; it now passes the end-of-options separator. Also fixes a `config.toml` written unparseable when a value carried a newline or NUL, an installer PATH hint that printed a header with nothing under it, and a reviewer lane that crashed instead of degrading when its conversation cache file held the literal `null`. (#3118) diff --git a/.changeset/silly-eagles-swim.md b/.changeset/silly-eagles-swim.md new file mode 100644 index 000000000..5309864ab --- /dev/null +++ b/.changeset/silly-eagles-swim.md @@ -0,0 +1,5 @@ +--- +type: Security +pr: 3124 +--- +**A directory name containing `$(…)` or a backtick no longer becomes a live command in your shell startup file** — the PATH-persistence suggestion escaped its `export PATH="…"` line for the `echo` that carries it, not for the rc file it lands in, so a substitution in the target directory survived into `~/.bashrc` and ran on every new shell. (#3118) diff --git a/CONTEXT.md b/CONTEXT.md index 81629d0cb..1b48ff39f 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -121,7 +121,7 @@ Module owning the single declared contract for a **reviewer lane** — one exter Module owning the projection from a **declared reviewer lane** plus resolved configuration to a concrete **invocation plan** — the value a lane is actually run from (ADR-2782 Phase 5b, #2799). Pure: no filesystem, network, subprocess or clock; configuration arrives through a `configGet` seam. Interface: `resolveLanePlan({lane, configGet, runDir, repoRoot, effortArgs}) → {ok, plan} | {ok:false, reason, detail}`, `LANE_UNAVAILABLE` (frozen reason enum — a lane that will not run reports WHY, because the ambiguity between "failed" and "ran cleanly with nothing to report" is the defect class this epic closes), plus `isEmptyReview`, `normalizeHost` and `fileRefPrompt`. TOTAL — a malformed lane yields an unavailable result, never a throw, because third-party overlay manifests reach this seam. `invoke.args` is an **argv template** over a closed four-member placeholder vocabulary (`{{model}}`, `{{effort}}`, `{{output}}`, `{{prompt}}`), not a prefix: the injected pieces do not all go in the same place — `codex` injects the model after its `exec` subcommand and the output file later still, while five lanes end with a bare `-` that must stay last. Each lane's plan was derived FROM its former bash leg, and a golden table asserts all twelve; that table is the strangler-fig substitute for a parallel run. ### Reviewer Lane Runner -Module owning **execution** of an invocation plan (ADR-2782 Phase 5b, #2799): probe, spawn or HTTP call, empty-output policy, and dispatch of the three first-party `handler` modules D6 names (`antigravity`, `openai-compatible`, `opencode`). Replaces ~640 lines of hand-authored per-CLI bash in `invoke_reviewers`. Interface: `runLane`, `probeLane`, `checkEgressHost`, `writeReviewOrStub`, and the handler entry points (`handleOpencodeOutput`, `antigravityArgv`, `antigravityPrompt`, `antigravityWatermark`, `antigravityTranscriptFallback`, `antigravityDiagnostic`, `stampBlindReview`, `runOpenAiCompatible`). Every dependency is injected, so behaviour is testable without a network or a spawn. **Every subprocess call passes `timeout` + `killSignal` + `maxBuffer`** (`DEFECT.UNBOUNDED-SUBPROCESS`): a frozen synchronous spawn cannot be interrupted and hangs a whole CI chunk to its kill with `# fail 0`. Three runtime dependencies disappear here — `jq`, `curl`, and external `timeout`/`gtimeout` — which also closes two platform holes: five lanes were unavailable on stock Windows/Git-Bash for want of `jq`, and the Antigravity lane ran unbounded on stock macOS, which ships neither killer. Owns ADR-2782 D5 rules 2–4: the egress destination is **re-resolved at invocation** and a changed host blocks the lane rather than silently redirecting it; absence of a consent record allows, since first-party lanes are never consent-gated. +Module owning **execution** of an invocation plan (ADR-2782 Phase 5b, #2799): probe, spawn or HTTP call, empty-output policy, and dispatch of the three first-party `handler` modules D6 names (`antigravity`, `openai-compatible`, `opencode`). Replaces ~640 lines of hand-authored per-CLI bash in `invoke_reviewers`. Interface: `runLane`, `probeLane`, `checkEgressHost`, `writeReviewOrStub`, and the handler entry points (`handleOpencodeOutput`, `antigravityArgv`, `antigravityPrompt`, `antigravityWatermark`, `antigravityTranscriptFallback`, `antigravityDiagnostic`, `stampBlindReview`, `runOpenAiCompatible`). Every dependency is injected, so behaviour is testable without a network or a spawn. **Every subprocess call passes `timeout` + `killSignal` + `maxBuffer`** (`DEFECT.UNBOUNDED-SUBPROCESS`): a frozen synchronous spawn cannot be interrupted and hangs a whole CI chunk to its kill with `# fail 0`. Three runtime dependencies disappear here — `jq`, `curl`, and external `timeout`/`gtimeout` — which also closes two platform holes: five lanes were unavailable on stock Windows/Git-Bash for want of `jq`, and the Antigravity lane ran unbounded on stock macOS, which ships neither killer. Owns ADR-2782 D5 rules 2–4: the egress destination is **re-resolved at invocation** and a changed host blocks the lane rather than silently redirecting it; absence of a consent record allows, since first-party lanes are never consent-gated. `antigravityWatermark`'s final read can throw (permissions, mid-write truncation) on a transcript that indisputably exists, which is not the same fact as a genuinely empty or absent one; that case now sets `unreadable: true` on the returned mark rather than folding into `lines: 0`, and `antigravityTranscriptFallback` declines (`''`) for a same-conv-id unreadable mark instead of skipping zero lines and replaying a stale pre-run response (#3118). ### Resolution Provenance Cross-seam principle (ADR-1411, epic #1411): context resolution — config loading, project-root anchoring, workstream resolution — must report its provenance, not fall open silently to defaults. A resolver anchors deterministically to the project root (one walk-up module, no dependence on an arbitrary descendant cwd), returns *what* it resolved **and** *where it came from* (`source`/`degraded`), and surfaces a diagnostic when a *configured* input resolves empty (`not configured` and `configured-but-empty` are distinguishable). The resolution-side analog of ADR-227 (input-validation shape). Target seams: Config Loader Module (`loadConfig` → `ConfigResolution { config, source, degraded }`), Project-Root Resolution Module (single nearest-`.planning/` walk-up, retiring ad-hoc resolvers like `resolvePlanningCwd`), I/O Module (`Resolution { value, configured, reason, warnings }` output envelope). A configured input resolving empty without a reason is a CI-guarded regression. **P1 (nearest-.planning/ heuristic) shipped in #1413; P2 (loadConfigResolved + agent-skills diagnostic) shipped in #1415 / closes #1366**: `loadConfigResolved` now implements the Config Loader seam target; `cmdAgentSkills` uses `findProjectRoot` + `loadConfigResolved` and emits `configured`/`reason`/`source`/`degraded` in its `--json` IR. **Corrupt is not absent (ADR-1411 amendment 2026-07-26, epic #1879 Phase 0 / #2674):** the principle above governs a resolution *miss*; input that is present but *not usable* (a `SyntaxError`, an errno such as `EACCES`/`EIO`, or a malformed structure with no exception at all) is a distinct class that must stay distinguishable from genuine absence. The defect in that class is not the fallback — ADR-227 requires malformed input to be coerced rather than propagated, and this ADR already permits a fallback — it is that the fallback is **invisible**. So every current return value is preserved and the cause is made visible by one of two mechanisms: **in-band**, where the result already carries a provenance envelope, name the cause in it (`ConfigResolution` gains a `reason`; `Resolution`'s four documented values all describe a miss, so new unusable-input values are introduced with the first adopter) and also expose it on the surface callers actually use, since a `reason` no caller reads is an unreachable field; **out-of-band**, where the read returns a bare sentinel or a plausible default it cannot extend, keep that value and emit a deduplicated `stderr` diagnostic keyed on resolved-path + errno, reusing the `_warnedUnknownConfigKeys` guard pattern. The diagnostic is unconditional — a deliberate divergence from ADR-227's never-implemented `GSD_DEBUG` opt-in, since an opt-in nobody sets is the same silence. Throwing is **not** the cluster's answer — it stays confined to ADR-227's genuinely-fatal carve-out, decided per call, never inferred from the return shape. @@ -883,7 +883,7 @@ The prompt-level data/instruction isolation seam for untrusted web/document ingr ## Shell Command Projection Module (expanded glossary entry, 2026-05-13) -Module owning all OS-facing I/O for the tool: runtime-aware command-text rendering (hook commands, PATH action lines, shim scripts), subprocess dispatch (execGit, execNpm, execTool, probeTty, isSpawnTimeout), and platform file I/O (platformWriteSync, platformReadSync, platformEnsureDir). Single seam for platform-conditional logic — one place to fix any shell or file write regression across Windows, macOS, and Linux. `isSpawnTimeout` is the single shared "did this subprocess time out" predicate (error.code==='ETIMEDOUT' only — cross-platform-safe; does not require signal==='SIGTERM'), consumed by worktree-safety.cts, worktree-base-ref.cts, commands.cts, and this module's own dispatchGsdCommand (#3050 — "Generative Fix Divergence"). Lives in `gsd-core/bin/lib/shell-command-projection.cjs`. See ADR-0009 (superseded "does not execute" constraint) and ADR-0010 (superseded File Operation Engine). +Module owning all OS-facing I/O for the tool: runtime-aware command-text rendering (hook commands, PATH action lines, shim scripts), subprocess dispatch (execGit, execNpm, execTool, probeTty, isSpawnTimeout), and platform file I/O (platformWriteSync, platformReadSync, platformEnsureDir). Single seam for platform-conditional logic — one place to fix any shell or file write regression across Windows, macOS, and Linux. `isSpawnTimeout` is the single shared "did this subprocess time out" predicate (error.code==='ETIMEDOUT' only — cross-platform-safe; does not require signal==='SIGTERM'), consumed by worktree-safety.cts, worktree-base-ref.cts, commands.cts, and this module's own dispatchGsdCommand (#3050 — "Generative Fix Divergence"). `projectPathExportLine(targetDir)` is the single source of the `export PATH=":$PATH"` line for all three PATH-persistence lanes (repair, persist, win32 Git Bash) — it double-quote-escapes for the line's final rc-file context before any lane single-quotes it for its own `echo` transport, closing the #3118 command-substitution injection where a lane re-escaped the line itself and let a `$(…)` in the target dir execute on every new shell; the win32 cmd.exe lane fails closed (empty actions) whenever the target dir contains `"`, since that character is reserved on Windows and would otherwise close cmd's quoted region, and now tags that empty result with a typed `PATH_ACTION_REASON` (`win32_reserved_quote`) so it stays distinguishable from the unrelated "no target directory given" empty result (`no_target_dir`, #3118); the fish lane emits `fish_add_path -- ''` (`--` end-of-options separator, verified empirically against fish 4.8.1), since a leading-dash target dir is otherwise misparsed by fish's argparse-based option scanning regardless of quoting; `escapeTomlDoubleQuotedString` now escapes TOML's required control characters (U+0000-U+0008, U+000A-U+001F, U+007F), not just backslash and quote, closing a raw-newline/CR/NUL config.toml parse failure (#3118). Lives in `gsd-core/bin/lib/shell-command-projection.cjs`. See ADR-0009 (superseded "does not execute" constraint) and ADR-0010 (superseded File Operation Engine). Invariants: - Result shape: all exec* functions return `{ exitCode, stdout, stderr }`; never throw on non-zero exit code. diff --git a/bin/install.js b/bin/install.js index a57df30af..e1b794463 100755 --- a/bin/install.js +++ b/bin/install.js @@ -14,6 +14,7 @@ const { projectPathActionProjection, projectPortableHookBaseDir, projectPersistentPathExportActions, + PATH_ACTION_REASON, projectShellCommandText, projectCodexHookTomlCommand, shellHookOmitsBashRunner, @@ -13138,14 +13139,26 @@ function maybeSuggestPathExport(globalBin, homeDir) { console.log(''); console.log(` ${yellow}⚠${reset} ${bold}${globalBin}${reset} is not on your PATH.`); - console.log(` Add it with one of:`); const projected = projectPersistentPathExportActions({ targetDir: globalBin, platform: process.platform, }); - for (const action of projected.shellActions) { - const labelPrefix = action.label ? `${action.label}: ` : ''; - console.log(` ${cyan}${labelPrefix}${action.command}${reset}`); + if (projected.reason === PATH_ACTION_REASON.WIN32_RESERVED_QUOTE) { + // #3118 review MINOR: a win32 targetDir containing `"` makes + // projectPathActionProjection return [] (no command can quote it safely + // on Windows) — printing the "Add it with one of:" header with nothing + // under it is a silent dead-end. Name the cause instead. + console.log(` No command can be suggested: the path contains a ${cyan}"${reset} character, which cannot appear in a Windows path.`); + } else if (projected.shellActions.length === 0) { + // #3118: no target directory to talk about (reason === NO_TARGET_DIR, or + // no reason at all) — there is nothing to print beyond the "not on your + // PATH" line above. + } else { + console.log(` Add it with one of:`); + for (const action of projected.shellActions) { + const labelPrefix = action.label ? `${action.label}: ` : ''; + console.log(` ${cyan}${labelPrefix}${action.command}${reset}`); + } } console.log(''); } diff --git a/docs/how-to/install-on-your-runtime.md b/docs/how-to/install-on-your-runtime.md index e9503daa8..6c15dceea 100644 --- a/docs/how-to/install-on-your-runtime.md +++ b/docs/how-to/install-on-your-runtime.md @@ -538,7 +538,7 @@ If the command is not found after restart, verify the install directory matches If the installer's global bin directory is not on your `PATH`, it prints a one-time warning with a copy-paste command for your shell. The suggestion list covers `zsh`, `bash`, and `fish` (plus PowerShell, cmd.exe, and Git Bash on Windows). For fish, run the line it prints: ```fish -fish_add_path '/path/to/global/bin' +fish_add_path -- '/path/to/global/bin' ``` If the directory is already on your PATH but the installer still warns, open a new fish session (`exec fish`) to pick up the change. diff --git a/pwned_cmdsub b/pwned_cmdsub new file mode 100644 index 000000000..e69de29bb diff --git a/src/api-coverage.cts b/src/api-coverage.cts index 723f864f3..4afb4bd74 100644 --- a/src/api-coverage.cts +++ b/src/api-coverage.cts @@ -257,6 +257,31 @@ const SURFACE_DESCRIPTOR_WORDS = new Set([ * is deliberately absent (an external API IS external). */ const INTERNAL_DESCRIPTORS = new Set(['internal', 'in-house', 'local', 'first-party', 'private']); +/** #2784: negation suppression. A clause that pairs an integration verb with an + * API noun but the verb itself is directly negated (e.g. "does not integrate", + * "integrates no external API") is suppressed. Two windows are checked: a + * negation qualifier within 2 words directly before the verb, or "no"/"zero"/ + * "none" between the verb and a following noun. + * KNOWN LIMIT (deliberate, not a bug to fix later): a negation further than 2 + * words before the verb, or a clause where the noun precedes the verb, is NOT + * suppressed — e.g. "Ships without any API integration." still reports + * detected:true, because "without" sits outside the verb's 2-word lookback + * and the noun precedes the verb. This is intentional: detectApiIntegration + * is fail-closed — an unsuppressed false positive costs a one-line + * COVERAGE.md declaration, while widening the suppression window risks a + * false negative that silently lets a real external-API phase past a + * blocking gate. + * Hoisted to module scope (#3127 regression fix): this was previously + * allocated fresh on every source line, which is wasted work on documents + * with many lines. */ +const NEGATION_QUALIFIERS = new Set([ + 'no', 'not', 'without', 'zero', 'neither', 'nor', 'none', "don't", "doesn't", "didn't", "won't", "can't", "cannot", +]); +/** Negation tokens checked BETWEEN a verb and a later noun (narrower than + * NEGATION_QUALIFIERS — "not" and "without" are checked only immediately + * before the verb, via NEGATION_QUALIFIERS above). */ +const NEGATION_NOUN_TOKENS = new Set(['no', 'zero', 'none']); + /** A capitalized compound modifier ("Resolver-only", "Read-only", "E-commerce" * — lowercase letter right after the hyphen) is an adjective phrase, not a * service name. Real hyphenated services capitalize the second segment @@ -484,23 +509,37 @@ export function detectApiIntegration( // // #2784: negation suppression. A clause that pairs an integration verb with // an API noun but the verb itself is directly negated (e.g. "does not - // integrate", "integrates no external API", "without any API integration") - // is suppressed. The check is scoped to the verb's immediate context (the - // word directly before the verb, or the word directly between verb and noun) - // — NOT a blanket clause-wide scan, because "without changing runtime - // dependencies" in a long clause does NOT negate the integration. - const NEGATION_QUALIFIERS = new Set([ - 'no', 'not', 'without', 'zero', 'neither', 'nor', 'none', "don't", "doesn't", "didn't", "won't", "can't", "cannot", - ]); + // integrate", "integrates no external API") is suppressed. The check is + // scoped to the verb's immediate context (the word directly before the + // verb, or the word directly between verb and noun) — NOT a blanket + // clause-wide scan, because "without changing runtime dependencies" in a + // long clause does NOT negate the integration. + // KNOWN LIMIT (deliberate, not a bug to fix later): a negation further than + // 2 words before the verb, or a clause where the noun precedes the verb, is + // NOT suppressed — e.g. "Ships without any API integration." is NOT + // suppressed today (pinned by a test in tests/api-coverage.test.cjs). + // detectApiIntegration is fail-closed by design: an unsuppressed false + // positive costs a one-line COVERAGE.md declaration, while widening the + // window trades that for a silent false negative on a blocking gate. if (verbRe && nounRe) { for (const clause of clauses) { const verbs = collectTermMatches(verbRe, clause.text); if (verbs.length === 0) continue; // #2784: check if any verb is immediately preceded by a negation // qualifier (within 2 words before the verb match). + // + // OFFSET NOTE: `v.start`/`n.start` (from collectTermMatches below and + // above) are already CLAUSE-LOCAL — collectTermMatches was called with + // `clause.text`, not the full line — and so is `clauseText` + // (`clause.text.toLowerCase()`). They must be used AS-IS to index into + // `clauseText`; do not re-base them against `clause.start` (that field + // is the clause's offset within the LINE, a different coordinate space, + // used only to map line-level spans like `extraNouns`/`masked` into a + // clause). Subtracting `clause.start` here double-offsets the slice + // bounds for every clause after the first on a line (#3127 follow-up). const clauseText = clause.text.toLowerCase(); const hasNegatedVerb = verbs.some((v) => { - const before = clauseText.slice(Math.max(0, v.start - clause.start - 20), v.start - clause.start); + const before = clauseText.slice(Math.max(0, v.start - 20), v.start); const beforeWords = before.split(/\s+/).filter(Boolean).slice(-2); return beforeWords.some((w: string) => NEGATION_QUALIFIERS.has(w.replace(/[^a-z']/g, ''))); }); @@ -513,15 +552,63 @@ export function detectApiIntegration( } } if (nounTerms.size === 0) continue; - // Check for negation between verb and noun - const hasNegatedNoun = verbs.some((v) => { - return nouns.some((n) => { - if (n.start <= v.start) return false; - const between = clauseText.slice(v.start - clause.start + v.term.length, n.start - clause.start); + // Check for negation between verb and noun. + // + // #3127 regression: the original form of this check was + // O(verbs × nouns), re-slicing and re-splitting the clause text for + // every (verb, noun) pair — effectively cubic in clause length (a + // clause of N repeated "integrate api" pairs did O(N^2) pair checks, + // each doing an O(N) slice/split). On a clause with 800 repeated + // pairs this took ~8.5s; fast-check's property test then generated + // documents large enough to hang the whole test file past node:test's + // 600s timeout. It ALSO subtracted `clause.start` from `v.start`/ + // `n.start` before slicing `clauseText` — but `v.start`/`n.start` are + // already local to `clause.text` (collectTermMatches was called with + // clause.text, not the full line), and `clauseText` is exactly + // `clause.text.toLowerCase()`. So that subtraction double-offset the + // slice bounds for every clause after the first on a line, sliding + // (and for negative results, JS's negative-index slice() wraparound + // non-monotonically re-mapping) the window to characters unrelated to + // the verb/noun pair — an independent latent bug, fixed here as part + // of establishing a well-defined O(1) predicate (a piecewise/clamped + // window has no single "widest span" to reason about at all). + // + // EXACT-EQUIVALENCE, single pass: the predicate is "does any pair + // (v, n) with n.start > v.start have a negation token in the span + // (v.end, n.start)". Every such span is a SUBSET of the widest + // possible span for a given noun: [min(v.end) over verbs valid for + // that noun, n.start). And since that window only widens as a + // noun's start increases (more verbs become valid, and the noun + // bound itself grows), the single widest span across the WHOLE + // clause is anchored at the noun with the maximum start, using the + // minimum verb-end among verbs valid for THAT noun (not the global + // minimum verb-end, which could belong to a verb that starts after + // this noun and so is never a valid pairing with it — a mismatch + // that would either miss or falsely include a negation). If that one + // substring contains no negation token, no narrower pair-specific + // substring can either; if it does, the (minVerb, maxNoun) pair + // itself contains it. This drops the check to O(verbs + nouns). + let hasNegatedNoun = false; + if (nouns.length > 0) { + let minVerbStart = Infinity; + for (const v of verbs) if (v.start < minVerbStart) minVerbStart = v.start; + let maxNounStart = -Infinity; + for (const n of nouns) if (n.start > maxNounStart) maxNounStart = n.start; + if (maxNounStart > minVerbStart) { + let minQualifyingVerbEnd = Infinity; + for (const v of verbs) { + if (v.start < maxNounStart) { + const vEnd = v.start + v.term.length; + if (vEnd < minQualifyingVerbEnd) minQualifyingVerbEnd = vEnd; + } + } + const between = clauseText.slice(minQualifyingVerbEnd, maxNounStart); const betweenWords = between.split(/\s+/).filter(Boolean); - return betweenWords.some((w: string) => ['no', 'zero', 'none'].includes(w.replace(/[^a-z']/g, ''))); - }); - }); + hasNegatedNoun = betweenWords.some((w: string) => + NEGATION_NOUN_TOKENS.has(w.replace(/[^a-z']/g, '')), + ); + } + } if (hasNegatedVerb || hasNegatedNoun) continue; for (const vTerm of new Set(verbs.map((t) => t.term))) { for (const nTerm of nounTerms) emitPair(vTerm, nTerm, rawLine); diff --git a/src/milestone.cts b/src/milestone.cts index e9babe30e..14e77aa25 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -792,7 +792,7 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo version, nextMilestoneCommand: formatGsdSlash('new-milestone', resolveRuntime(cwd)) as string, }, - { clock: realClock, progressProvider: () => null, sourcePath: statePath }, + { clock: realClock, sourcePath: statePath }, ); writeStateMd(statePath, result.content, cwd); } diff --git a/src/phase.cts b/src/phase.cts index 6f6466c48..be149bcab 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -2686,7 +2686,6 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { }, { clock: realClock, - progressProvider: () => null, // completePhase derives progress from the roadmap, not disk roadmapProvider: () => roadmapContent, sourcePath: statePath, }, diff --git a/src/review-lane-runner.cts b/src/review-lane-runner.cts index c26daf2d6..6120bc7d5 100644 --- a/src/review-lane-runner.cts +++ b/src/review-lane-runner.cts @@ -330,7 +330,7 @@ interface TranscriptEntry { export function antigravityWatermark( workspace: string, deps: RunnerDeps, -): { convId: string; lines: number } { +): { convId: string; lines: number; unreadable?: boolean } { const cachePath = `${deps.homeDir}/.gemini/antigravity-cli/cache/last_conversations.json`; if (!deps.exists(cachePath)) return { convId: '', lines: 0 }; let cache: Record; @@ -346,18 +346,31 @@ export function antigravityWatermark( try { return { convId, lines: deps.readFile(tx).split(/\r?\n/).filter((l) => l.trim()).length }; } catch { - return { convId, lines: 0 }; + // #3118: this conv-id pre-dates this run, so its transcript exists but this run cannot verify + // its line count. Reporting `lines: 0` would assert a fact we could not check — flag it instead + // so the fallback can decline rather than silently skip zero and replay a stale response. + return { convId, lines: 0, unreadable: true }; } } -/** Workspace lookup is case-insensitive — the leg's jq did `ascii_downcase` on both sides. */ -function resolveConvId(cache: Record, workspace: string): string { - if (Object.prototype.hasOwnProperty.call(cache, workspace)) { - const direct = cache[workspace]; +/** + * Workspace lookup is case-insensitive — the leg's jq did `ascii_downcase` on both sides. + * + * #3118: a successful `JSON.parse` does not by itself make the payload a usable object — + * `JSON.parse('null')` succeeds and returns `null`, so a truncated/zeroed cache file slips past + * the callers' parse-only try/catch. `typeof null === 'object'`, so the guard below must exclude + * `null` explicitly. Arrays are excluded too (not a workspace map), which also falls out of the + * `Object.entries`/`hasOwnProperty` lookups below returning nothing for array input. + */ +function resolveConvId(cache: unknown, workspace: string): string { + if (cache === null || typeof cache !== 'object') return ''; + const record = cache as Record; + if (Object.prototype.hasOwnProperty.call(record, workspace)) { + const direct = record[workspace]; if (typeof direct === 'string' && direct) return direct; } const target = workspace.toLowerCase(); - for (const [k, v] of Object.entries(cache)) { + for (const [k, v] of Object.entries(record)) { if (k.toLowerCase() === target && typeof v === 'string' && v) return v; } return ''; @@ -376,7 +389,7 @@ function transcriptPath(homeDir: string, convId: string): string { */ export function antigravityTranscriptFallback( workspace: string, - mark: { convId: string; lines: number }, + mark: { convId: string; lines: number; unreadable?: boolean }, deps: RunnerDeps, ): string { const cachePath = `${deps.homeDir}/.gemini/antigravity-cli/cache/last_conversations.json`; @@ -389,6 +402,9 @@ export function antigravityTranscriptFallback( } const convId = resolveConvId(cache, workspace); if (!convId) return ''; + // #3118: the watermark could not read this conversation's transcript, so there is no trustworthy + // skip for it. Declining is the fail-closed answer; skipping 0 would replay a prior run's review. + if (mark.unreadable === true && convId === mark.convId) return ''; const tx = transcriptPath(deps.homeDir, convId); if (!deps.exists(tx)) return ''; let lines: string[]; diff --git a/src/shell-command-projection.cts b/src/shell-command-projection.cts index 3ccaad438..f516daf8a 100644 --- a/src/shell-command-projection.cts +++ b/src/shell-command-projection.cts @@ -358,8 +358,35 @@ export function projectLegacySettingsHookCommand({ }); } +// Implements the TOML v1.0.0 basic-string escaping grammar (toml.md, "Basic +// strings" section, https://toml.io/en/v1.0.0#string): a basic string must +// escape the quotation mark, backslash, and control characters other than +// tab (U+0000-U+0008, U+000A-U+001F, U+007F). Compact escapes are used where +// TOML defines them (\b \t \n \f \r \" \\); every other character in the +// required ranges falls back to \uXXXX. See #3118 — an earlier version +// escaped only backslash and quote, so a raw newline/CR/NUL in a value +// produced an unparseable config.toml. +const TOML_COMPACT_ESCAPES: Record = { + '\x08': '\\b', + '\x09': '\\t', + '\x0A': '\\n', + '\x0C': '\\f', + '\x0D': '\\r', +}; + +// U+0000-U+0008, U+000A-U+001F, U+007F — control characters other than tab +// (U+0009), which the grammar permits unescaped. +const TOML_MUST_ESCAPE_CONTROL_CHARS = /[\x00-\x08\x0A-\x1F\x7F]/g; + export function escapeTomlDoubleQuotedString(value: unknown): string { - return String(value).replace(/\\/g, '\\\\').replace(/"/g, '\\"'); + return String(value) + .replace(/\\/g, '\\\\') + .replace(/"/g, '\\"') + .replace(TOML_MUST_ESCAPE_CONTROL_CHARS, (ch) => { + const compact = TOML_COMPACT_ESCAPES[ch]; + if (compact) return compact; + return `\\u${ch.codePointAt(0)!.toString(16).padStart(4, '0')}`; + }); } export function projectCodexHookTomlCommand({ absoluteRunner, scriptPath, platform = process.platform }: { @@ -388,12 +415,35 @@ export function escapeSingleQuotedShellLiteral(value: unknown): string { return String(value).replace(/'/g, "'\\''"); } +/** + * The `export PATH=":$PATH"` line every persistence lane appends, plus the escaped directory + * token it embeds. One builder because three lanes emit this line: a lane that re-escapes it + * locally is how #3118 shipped a `$(…)` into ~/.bashrc, where it ran on every new shell. The + * escaping is for the line's FINAL context — a double-quoted string in an rc file — not for + * whatever transport (an `echo`, a paste) it passes through on the way there. + */ +export function projectPathExportLine(targetDir: unknown): { escapedDir: string; line: string } { + const escapedDir = escapePosixDoubleQuoted(String(targetDir)); + return { escapedDir, line: `export PATH="${escapedDir}:$PATH"` }; +} + interface ShellAction { label: string | null; shell: string; command: string; } +/** + * Why a PATH suggestion produced no actions. An empty `shellActions` alone folds two different + * facts together — "no target directory was given" and "this target directory cannot be + * expressed as a shell command" — and a caller that cannot tell them apart prints a header with + * nothing under it (#3118). + */ +export const PATH_ACTION_REASON = Object.freeze({ + NO_TARGET_DIR: 'no_target_dir', + WIN32_RESERVED_QUOTE: 'win32_reserved_quote', +}); + export function renderShellActionLines(shellActions: ShellAction[] = []): string[] { return shellActions.map((action) => { if (!action || !action.command) return ''; @@ -409,15 +459,23 @@ export function projectPathActionProjection({ mode?: string; targetDir?: string | null; platform?: string; -}): { shellActions: ShellAction[]; actionLines: string[] } { - if (!targetDir) return { shellActions: [], actionLines: [] }; +}): { shellActions: ShellAction[]; actionLines: string[]; reason?: string } { + if (!targetDir) return { shellActions: [], actionLines: [], reason: PATH_ACTION_REASON.NO_TARGET_DIR }; + + // #3118: `"` is reserved on Windows, so a path containing one cannot exist — and it would close + // cmd's quoted region in the `powershell -Command "…"` lane below, turning the rest into cmd + // input. There is no correct command to suggest for an impossible path: fail closed rather than + // emit one whose quoting can be broken. + if (platform === 'win32' && String(targetDir).includes('"')) return { shellActions: [], actionLines: [], reason: PATH_ACTION_REASON.WIN32_RESERVED_QUOTE }; const isWin32 = platform === 'win32'; let shellActions: ShellAction[]; if (isWin32) { const psTargetDir = escapePowerShellSingleQuoted(targetDir); - const bashTargetDir = escapeSingleQuotedShellLiteral(posixNormalize(String(targetDir))); + const bashExportLine = escapeSingleQuotedShellLiteral( + projectPathExportLine(posixNormalize(String(targetDir))).line, + ); shellActions = [ { label: 'PowerShell', @@ -432,40 +490,49 @@ export function projectPathActionProjection({ { label: 'Git Bash', shell: 'bash', - command: `echo 'export PATH="${bashTargetDir}:$PATH"' >> ~/.bashrc`, + command: `echo '${bashExportLine}' >> ~/.bashrc`, }, ]; } else if (mode === 'persist') { - const bashTargetDir = escapeSingleQuotedShellLiteral(String(targetDir)); + const exportLine = escapeSingleQuotedShellLiteral(projectPathExportLine(targetDir).line); + const fishTargetDir = escapeSingleQuotedShellLiteral(String(targetDir)); shellActions = [ { label: 'zsh', shell: 'zsh', - command: `echo 'export PATH="${bashTargetDir}:$PATH"' >> ~/.zshrc`, + command: `echo '${exportLine}' >> ~/.zshrc`, }, { label: 'bash', shell: 'bash', - command: `echo 'export PATH="${bashTargetDir}:$PATH"' >> ~/.bashrc`, + command: `echo '${exportLine}' >> ~/.bashrc`, }, // #323: fish has no `export`/`$PATH`-list syntax. `fish_add_path` is the // fish-native API (>= fish 3.2, 2021) that persists to the universal // variable store and de-duplicates. The directory is single-quoted with // the same POSIX literal escaping as the zsh/bash siblings — `'\''` is // also a valid escaped single quote in fish between quote spans. + // + // #3118 review MINOR: a `targetDir` with a leading `-` (e.g. `-v`) is a + // legal directory name, but fish's argparse-based option scanning + // treats a leading-dash token as a flag REGARDLESS of quoting, so + // `fish_add_path '-v'` misparses it and prints "No paths to add, not + // setting anything." (exit 1) instead of adding the path. `--` is + // fish's standard end-of-options separator; verified empirically + // against a real fish 4.8.1 install that `fish_add_path -- '-v'` + // succeeds where the unseparated form fails. { label: 'fish', shell: 'fish', - command: `fish_add_path '${bashTargetDir}'`, + command: `fish_add_path -- '${fishTargetDir}'`, }, ]; } else { - const posixTargetDir = escapePosixDoubleQuoted(targetDir); shellActions = [ { label: null, shell: 'posix', - command: `export PATH="${posixTargetDir}:$PATH"`, + command: projectPathExportLine(targetDir).line, }, ]; } @@ -479,13 +546,15 @@ export function projectPathActionProjection({ export function projectPersistentPathExportActions({ targetDir, platform = process.platform }: { targetDir?: string | null; platform?: string; -}): { shellActions: ShellAction[] } { +}): { shellActions: ShellAction[]; reason?: string } { const projected = projectPathActionProjection({ mode: 'persist', targetDir, platform, }); - return { shellActions: projected.shellActions }; + return projected.reason === undefined + ? { shellActions: projected.shellActions } + : { shellActions: projected.shellActions, reason: projected.reason }; } diff --git a/src/state-transition.cts b/src/state-transition.cts index 362180719..8b7c2fbdf 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -297,10 +297,7 @@ export const STATE_MD_SECTIONS = { // Intent + deps + result types (ADR-1769 §3) // ---------------------------------------------------------------------------- -export type ProgressRecord = Record; - export type StateTransitionDeps = { - progressProvider: () => ProgressRecord | null; clock: { today: () => string; localToday: () => string; nowIso: () => string }; /** * Roadmap content provider for transitions that re-derive milestone-wide @@ -601,7 +598,26 @@ function locateCurrentPosition(body: string): { start: number; end: number } | n const start = h.offset + hl.length + 1; let end = body.length; for (let j = idx + 1; j < hs.length; j++) { - if (STOP_H2_PLUS(hs[j].level)) { end = hs[j].offset - 1; break; } + if (STOP_H2_PLUS(hs[j].level)) { + // Exclude the newline that separates this section from the next + // heading. Walk back over a bare `\n`, then over a `\r` if one + // immediately precedes it (CRLF), so a CRLF document's slice does not + // retain a stray unpaired trailing `\r` (#3118). + let e = hs[j].offset; + if (e > 0 && body[e - 1] === '\n') { + e -= 1; + if (e > 0 && body[e - 1] === '\r') e -= 1; + } + // Clamp so the span can never invert (#3118 review): when the section + // is empty and the next heading follows with no blank line between, + // walking back over the newline(s) can land `e` before `start`. An + // inverted span makes every mutator's `body.slice(0, start) + + // sectionBody + body.slice(end)` reassembly duplicate the bytes in + // `[end, start)`. A zero-length span (`end === start`) is the correct + // representation of an empty-but-present section. + end = Math.max(e, start); + break; + } } return { start, end }; } diff --git a/src/state.cts b/src/state.cts index 836a3fdb3..0d4561b6b 100644 --- a/src/state.cts +++ b/src/state.cts @@ -467,7 +467,7 @@ function cmdStatePatch(cwd: string, patches: Record, raw: boolea // and the resync-progress decision stay in this adapter. let results: { updated: string[]; failed: string[] } = { updated: [], failed: [] }; readModifyWriteStateMd(statePath, (content) => { - const result = transitionCore(content, { kind: 'patch', patches }, { clock: realClock, progressProvider: () => null }); + const result = transitionCore(content, { kind: 'patch', patches }, { clock: realClock }); results = (result.data as { updated: string[]; failed: string[] }) ?? results; return result.content; }, cwd, { resync: shouldResync }); @@ -505,7 +505,7 @@ function cmdStateUpdate(cwd: string, field: string | undefined, value: string | const result = transitionCore( content, { kind: 'update', field: field as string, value: value as string }, - { clock: realClock, progressProvider: () => null }, + { clock: realClock }, ); updated = (result.data as { updated: boolean } | undefined)?.updated === true; return result.content; @@ -556,7 +556,6 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void { const intent: StateTransitionIntent = { kind: 'advancePlan' }; const deps: StateTransitionDeps = { clock: realClock, - progressProvider: () => null, sourcePath: statePath, }; @@ -2435,7 +2434,6 @@ function cmdStateBeginPhase(cwd: string, phaseNumber: string | number, phaseName }; const deps: StateTransitionDeps = { clock: realClock, - progressProvider: () => null, // beginPhase doesn't consult disk progress; syncStateFrontmatter's scan is authoritative sourcePath: statePath, }; @@ -2713,7 +2711,6 @@ function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, planCou }; const deps: StateTransitionDeps = { clock: realClock, - progressProvider: () => null, sourcePath: statePath, }; @@ -2751,7 +2748,7 @@ function cmdStateMilestoneSwitch(cwd: string, version: string | undefined, name: // milestoneSwitch rebuilds frontmatter directly and must not run the // steady-state syncStateFrontmatter post-sync. const intent: StateTransitionIntent = { kind: 'milestoneSwitch', version, name: resolvedName }; - const deps: StateTransitionDeps = { clock: realClock, progressProvider: () => null, sourcePath: statePath }; + const deps: StateTransitionDeps = { clock: realClock, sourcePath: statePath }; const lockPath = acquireStateLock(statePath); try { @@ -2990,7 +2987,7 @@ function cmdStateSync(cwd: string, options: StateSyncOptions | undefined, raw: b const syncResult = transitionCore( modified, { kind: 'sync', totalPlansInPhase: highestIncompletePhase ? highestIncompletePhaseplanCount : null, percent }, - { clock: realClock, progressProvider: () => null }, + { clock: realClock }, ); modified = syncResult.content; const coreChanges = (syncResult.data as { changes?: string[] } | undefined)?.changes ?? []; @@ -3066,7 +3063,7 @@ function cmdStatePrune(cwd: string, options: StatePruneOptions, raw: boolean): v // This adapter owns currentPhase derivation (#1760 `Phase`/`Current Phase` // fallback above), dry-run, and STATE-ARCHIVE.md writes. const runPruneCore = (content: string): { newContent: string; archivedSections: PrunedSection[] } => { - const result = transitionCore(content, { kind: 'prune', cutoff }, { clock: realClock, progressProvider: () => null }); + const result = transitionCore(content, { kind: 'prune', cutoff }, { clock: realClock }); return { newContent: result.content, archivedSections: ((result.data as { archivedSections?: PrunedSection[] } | undefined)?.archivedSections) ?? [], @@ -3184,7 +3181,6 @@ function cmdStateRebuild(cwd: string, options: StateRebuildOptions, raw: boolean }; const deps: StateTransitionDeps = { - progressProvider: () => null, clock: realClock, phaseInventoryProvider, // Without this, `state rebuild --dry-run` reported a truncated STATE.md anonymously: the diff --git a/tests/api-coverage.test.cjs b/tests/api-coverage.test.cjs index b96e53e9f..905ba5789 100644 --- a/tests/api-coverage.test.cjs +++ b/tests/api-coverage.test.cjs @@ -131,6 +131,31 @@ describe('detectApiIntegration — pure detector (#1562)', () => { }); } + // ── #2784/#3127: negation suppression + its #3127 perf regression ──────── + // PR #3127 added clause-local negation suppression (hasNegatedVerb / + // hasNegatedNoun) but the noun-side check was O(verbs × nouns) with a + // slice+split per pair — effectively cubic in clause length. A clause with + // a few hundred repeated "integrate api" pairs took seconds; fast-check's + // property test then generated documents large enough to hang the whole + // file past node:test's 600s timeout (fail 0 / cancelled 1). These two + // tests pin the O(verbs + nouns) fix at a size (~500 pairs) chosen so that + // a re-regression to the old quadratic-per-pair behavior makes this file + // exceed the runner's own timeout — that hang, not a wall-clock assertion + // (CLAUDE.md forbids those), is the failure signal for a re-regression. + test('suppresses a negated pair regardless of how many terms precede it', () => { + const pairs = Array(500).fill('integrate api').join(' '); + const scope = pairs + ' but we integrate no api here'; + const r = detectApiIntegration(scope); + assert.strictEqual(r.detected, false); + }); + + test('still detects a genuine pair in a very long clause', () => { + const pairs = Array(500).fill('integrate api').join(' '); + const r = detectApiIntegration(pairs); + assert.strictEqual(r.detected, true); + assert.ok(r.signals.length > 0); + }); + test('fenced code blocks are stripped — trigger inside a code fence does not fire', () => { const scope = [ 'Refactor the helpers.', @@ -165,6 +190,74 @@ describe('detectApiIntegration — pure detector (#1562)', () => { assert.strictEqual(detectApiIntegration(splitLine).detected, false); assert.strictEqual(detectApiIntegration('integrates the api').detected, true); }); + + // ── #3127 follow-up: hasNegatedVerb offset regression coverage ─────────── + // The `hasNegatedVerb` window previously subtracted `clause.start` a SECOND + // time from an already clause-local `v.start`, which only accidentally + // worked for the first clause on a line (clause.start === 0) and silently + // mis-windowed (via negative-index slice wraparound) every later clause. + test('suppresses a pair when the verb is directly negated', () => { + const r = detectApiIntegration('This phase does not integrate any external API.'); + assert.strictEqual(r.detected, false); + }); + + test('suppresses a pair when a negation sits between verb and noun', () => { + const r = detectApiIntegration('This phase integrates no external API.'); + assert.strictEqual(r.detected, false); + }); + + test('still detects a genuine integration', () => { + const r = detectApiIntegration('We integrate the Stripe API.'); + assert.strictEqual(r.detected, true); + const compound = r.signals.find((s) => s.verb !== '(surface)'); + assert.ok(compound, 'expected a compound verb+noun signal'); + assert.strictEqual(compound.verb, 'integrate'); + assert.strictEqual(compound.noun, 'api'); + }); + + test('a distant negation does not suppress a genuine pair', () => { + // #2784's own documented example: "without changing runtime dependencies" + // inside a long clause must NOT negate an unrelated genuine pairing later + // in the SAME clause — the negation-window checks are scoped to the + // verb's immediate context, not a blanket clause-wide scan. + const r = detectApiIntegration( + 'The migration proceeds without changing runtime dependencies while we integrate the Stripe API for payment processing across the whole checkout flow.' + ); + assert.strictEqual(r.detected, true); + }); + + test('suppression works for a clause that is not the first on the line', () => { + // THE REGRESSION TEST for the double-offset bug: before the fix, this + // failed (wrongly detected === true) because the negation window for the + // second clause was computed against the wrong index base (v.start was + // re-based against clause.start even though it was already clause-local), + // producing an empty/garbage "before" window via negative-index slice() + // wraparound so the verb-adjacent negation was never seen. + const r = detectApiIntegration('We ship a thing; this phase does not integrate an external API.'); + assert.strictEqual(r.detected, false); + }); + + test('suppression in an early clause does not mask a genuine pair in a later clause', () => { + // Intended semantics: negation suppression is scoped PER CLAUSE (matching + // the "no cross-clause binding" design elsewhere in this module — see the + // CLAUSE_BOUNDARY_CHARS note). A negated pair in clause 1 must not swallow + // an independent, unnegated, genuine pair in clause 2: each clause is + // evaluated on its own, and `detected` is true if ANY clause fires. + const r = detectApiIntegration('This phase integrates no external API; we also integrate the Stripe API.'); + assert.strictEqual(r.detected, true); + }); + + test('a negation outside the lookback window does not suppress — fail-closed by design', () => { + // This pins DELIBERATE fail-closed behavior, not an aspiration: "without" + // sits outside hasNegatedVerb's 2-word lookback before the verb, and the + // noun ("integration") precedes the verb ("Ships") so hasNegatedNoun's + // verb-to-noun window never applies either. Widening the lookback to catch + // this phrase would trade a cheap false positive (a one-line COVERAGE.md + // declaration) for a silent false negative — a real external-API phase + // slipping past a blocking gate — which is the dangerous direction. + const r = detectApiIntegration('Ships without any API integration.'); + assert.strictEqual(r.detected, true); + }); }); // ────────────────────────────────────────────────────────────────────────────── diff --git a/tests/fix-2136-clock-local-today.test.cjs b/tests/fix-2136-clock-local-today.test.cjs index d824b229a..108871f0b 100644 --- a/tests/fix-2136-clock-local-today.test.cjs +++ b/tests/fix-2136-clock-local-today.test.cjs @@ -112,7 +112,7 @@ describe('#2136 last_activity is stamped from localToday, not today', () => { const result = transitionCore( input, { kind: 'sync', totalPlansInPhase: 5, percent: 60 }, - { clock: splitClock, progressProvider: () => null }, + { clock: splitClock }, ); assert.strictEqual(stateExtractField(result.content, 'Last Activity'), '2020-06-14', 'Last Activity must use localToday() (2020-06-14), not today() (2020-06-15)'); diff --git a/tests/review-lane-runner.test.cjs b/tests/review-lane-runner.test.cjs index 9ae705a8c..c0df48862 100644 --- a/tests/review-lane-runner.test.cjs +++ b/tests/review-lane-runner.test.cjs @@ -19,6 +19,7 @@ const { writeReviewOrStub, handleOpencodeOutput, stampBlindReview, + antigravityWatermark, antigravityTranscriptFallback, runOpenAiCompatible, } = require('../gsd-core/bin/lib/review-lane-runner.cjs'); @@ -342,6 +343,141 @@ describe('runner — antigravity handler (#2073 / #2176)', () => { assert.equal(antigravityTranscriptFallback(ROOT, { convId: '', lines: 0 }, d), ''); }); + // ── antigravityWatermark (#3118) ─────────────────────────────────────────── + // + // Every test above hands the fallback a HAND-WRITTEN mark. None of them calls + // `antigravityWatermark`, so none says anything about whether the mark a real run produces is + // correct. The producer had zero test references before this block; the fail-open below lived + // entirely in that gap. + describe('antigravityWatermark — the mark a real run actually produces', () => { + test('returns an empty mark when the conversation cache is absent', () => { + assert.deepEqual(antigravityWatermark(ROOT, deps()), { convId: '', lines: 0 }); + }); + + test('returns an empty mark when the cache is not valid JSON', () => { + const d = deps({ files: { [CACHE]: 'NOT JSON' } }); + assert.deepEqual(antigravityWatermark(ROOT, d), { convId: '', lines: 0 }); + }); + + test('returns an empty mark when the workspace has no conversation', () => { + const d = deps({ files: { [CACHE]: JSON.stringify({ '/somewhere/else': 'c9' }) } }); + assert.deepEqual(antigravityWatermark(ROOT, d), { convId: '', lines: 0 }); + }); + + for (const [label, body] of [ + ['a number', '0'], + ['a string', '"just a string"'], + ['an array', '[]'], + ['null', 'null'], + ['a boolean', 'true'], + ]) { + test(`a conversation cache that is ${label} yields an empty mark`, () => { + // Valid JSON that is not an object still reaches hasOwnProperty / Object.entries. + const d = deps({ files: { [CACHE]: body } }); + assert.deepEqual(antigravityWatermark(ROOT, d), { convId: '', lines: 0 }); + }); + } + + test('ignores a non-string conversation id', () => { + const d = deps({ files: { [CACHE]: JSON.stringify({ [ROOT]: 42 }) } }); + assert.equal(antigravityWatermark(ROOT, d).convId, ''); + }); + + test('ignores an empty-string conversation id', () => { + const d = deps({ files: { [CACHE]: JSON.stringify({ [ROOT]: '' }) } }); + assert.equal(antigravityWatermark(ROOT, d).convId, ''); + }); + + test('resolves the workspace case-insensitively', () => { + const d = deps({ files: { [CACHE]: JSON.stringify({ '/REPO': 'c1' }), [TX('c1')]: entry('x') } }); + assert.equal(antigravityWatermark('/repo', d).convId, 'c1'); + }); + + test('keeps the conversation id when the transcript does not exist yet', () => { + // Distinct from the cases above: the conversation is KNOWN, it simply has no transcript. + const d = deps({ files: { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }) } }); + assert.deepEqual(antigravityWatermark(ROOT, d), { convId: 'c1', lines: 0 }); + }); + + test('counts the non-blank transcript lines', () => { + const files = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: [entry('a'), entry('b')].join('\n') }; + assert.equal(antigravityWatermark(ROOT, deps({ files })).lines, 2); + }); + + test('an empty transcript is zero lines, not an unreadable one', () => { + // Negative space for the fix: a genuinely empty transcript must NOT degrade. + const files = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: '' }; + const mark = antigravityWatermark(ROOT, deps({ files })); + assert.equal(mark.lines, 0); + assert.notEqual(mark.unreadable, true, 'an empty transcript is readable, just empty'); + }); + + test('whitespace-only transcript lines are not counted', () => { + const files = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: '\n \n\t\n' }; + const mark = antigravityWatermark(ROOT, deps({ files })); + assert.equal(mark.lines, 0); + assert.notEqual(mark.unreadable, true); + }); + + test('counts CRLF transcript lines the same as LF', () => { + const lf = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: [entry('a'), entry('b')].join('\n') }; + const crlf = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: [entry('a'), entry('b')].join('\r\n') }; + assert.equal( + antigravityWatermark(ROOT, deps({ files: lf })).lines, + antigravityWatermark(ROOT, deps({ files: crlf })).lines, + ); + }); + + test('does not report zero lines when the transcript could not be read', () => { + // THE FAIL-OPEN. The transcript EXISTS and its conversation pre-dates this run, so its + // content is definitionally stale — but the read threw, so the count is unknown. Returning + // `lines: 0` is indistinguishable from "genuinely empty" and asserts a fact the function + // could not verify. + const files = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: 'unused' }; + const d = deps({ files }); + const realRead = d.readFile; + d.readFile = (p) => { + if (p === TX('c1')) throw new Error('EACCES'); + return realRead(p); + }; + + const mark = antigravityWatermark(ROOT, d); + assert.equal(mark.convId, 'c1', 'the conversation id was resolved and stays trustworthy'); + assert.equal(mark.unreadable, true, 'an unreadable transcript must be distinguishable from an empty one'); + }); + + test('the fallback declines when the watermark could not be established', () => { + // The CONSEQUENCE of the branch above. The watermark read fails; the fallback's own read + // then succeeds (transient EACCES, a concurrent writer, a partial flush). With `lines: 0` + // and a matching convId the fallback skips nothing and returns a PREVIOUS run's review as + // this run's — the precise failure the "never stale" docstring promises cannot happen. + const files = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: entry('STALE FROM LAST RUN') }; + const marking = deps({ files }); + const realRead = marking.readFile; + marking.readFile = (p) => { + if (p === TX('c1')) throw new Error('EACCES'); + return realRead(p); + }; + const mark = antigravityWatermark(ROOT, marking); + + assert.equal( + antigravityTranscriptFallback(ROOT, mark, deps({ files })), + '', + 'an unverified watermark must not license replaying the transcript', + ); + }); + + test('the fallback still skips exactly the pre-run lines when the mark is sound', () => { + // Negative space for the fix: a sound mark must keep working end-to-end, producer included. + const files = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: entry('old') }; + const mark = antigravityWatermark(ROOT, deps({ files })); + assert.equal(mark.lines, 1); + + files[TX('c1')] = [entry('old'), entry('THIS RUN')].join('\n'); + assert.equal(antigravityTranscriptFallback(ROOT, mark, deps({ files })), 'THIS RUN'); + }); + }); + test('the blind-review marker is anchored to the head of the output', () => { assert.ok(stampBlindReview('REVIEWED-WITHOUT-REPO-ACCESS\nbody').startsWith('> [reviewed-without-repo-access]')); }); diff --git a/tests/shell-command-projection-dispatch.test.cjs b/tests/shell-command-projection-dispatch.test.cjs index 8b042a554..fdab2fcc3 100644 --- a/tests/shell-command-projection-dispatch.test.cjs +++ b/tests/shell-command-projection-dispatch.test.cjs @@ -17,6 +17,17 @@ const { platformEnsureDir, dispatchGsdCommand, resolveGsdToolsPath, + projectPathActionProjection, + projectPathExportLine, + posixNormalize, + PATH_ACTION_REASON, + renderShellActionLines, + formatManagedHookScriptToken, + escapeTomlDoubleQuotedString, + escapePowerShellSingleQuoted, + escapePosixDoubleQuoted, + escapeSingleQuotedShellLiteral, + retryRenameSync, } = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'shell-command-projection.cjs')); const { createTempGitProject, createTempDir, cleanup } = require('./helpers.cjs'); @@ -961,7 +972,9 @@ describe('bug #3441: PATH guidance is projected from typed shell action IR', () assert.equal(projected.shellActions[1].command.includes("/tmp/O'\\''Neil/bin"), true); // #323: fish entry single-quotes the dir with the same POSIX literal // escaping (`'\''` is also a valid escaped quote in fish unquoted context). - assert.equal(projected.shellActions[2].command, "fish_add_path '/tmp/O'\\''Neil/bin'"); + // #3118: `--` is fish's end-of-options separator, added so a leading-dash + // directory name is not misparsed as a flag; every fish_add_path lane carries it now. + assert.equal(projected.shellActions[2].command, "fish_add_path -- '/tmp/O'\\''Neil/bin'"); }); test('maybeSuggestPathExport renders commands projected by path-action seam', () => { @@ -1619,3 +1632,391 @@ test('platformWriteSync survives a concurrent collision on the same target path' }); }); } + +// ──────────────────────────────────────────────────────────────────────── +// #3118 — failing-first coverage for a path-export projection API that does +// not exist yet in this module (projectPathExportLine, escapeCmdDoubleQuotedArgument). +// These tests are EXPECTED TO FAIL until that API lands. +// ──────────────────────────────────────────────────────────────────────── + +describe('path export projection — escaping (#3118)', () => { + const laneCommand = (mode, targetDir, platform, shell) => { + const { shellActions } = projectPathActionProjection({ mode, targetDir, platform }); + const a = shellActions.find((x) => x.shell === shell); + return a ? a.command : null; + }; + + // The fish lane is the ONE deliberate output change for ordinary paths: fish_add_path + // misparses a leading-dash directory name without the `--` end-of-options separator + // (#3118 review finding), so every path — hostile or ordinary — now gets `--` on the + // fish lane. Every other lane (posix export, zsh/bash echo, PowerShell, cmd) remains + // byte-identical for an ordinary path. + test('leaves an ordinary path unchanged on every lane except fish', () => { + const repairLinux = projectPathActionProjection({ mode: 'repair', targetDir: '/usr/local/bin', platform: 'linux' }); + assert.deepEqual(repairLinux.shellActions, [ + { label: null, shell: 'posix', command: 'export PATH="/usr/local/bin:$PATH"' }, + ]); + assert.deepEqual(repairLinux.actionLines, [ + 'export PATH="/usr/local/bin:$PATH"', + ]); + + const persistLinux = projectPathActionProjection({ mode: 'persist', targetDir: '/usr/local/bin', platform: 'linux' }); + assert.deepEqual(persistLinux.shellActions, [ + { label: 'zsh', shell: 'zsh', command: `echo 'export PATH="/usr/local/bin:$PATH"' >> ~/.zshrc` }, + { label: 'bash', shell: 'bash', command: `echo 'export PATH="/usr/local/bin:$PATH"' >> ~/.bashrc` }, + { label: 'fish', shell: 'fish', command: `fish_add_path -- '/usr/local/bin'` }, + ]); + assert.deepEqual(persistLinux.actionLines, [ + `zsh: echo 'export PATH="/usr/local/bin:$PATH"' >> ~/.zshrc`, + `bash: echo 'export PATH="/usr/local/bin:$PATH"' >> ~/.bashrc`, + `fish: fish_add_path -- '/usr/local/bin'`, + ]); + + const repairWin32 = projectPathActionProjection({ mode: 'repair', targetDir: 'C:/Users/dev/bin', platform: 'win32' }); + assert.deepEqual(repairWin32.shellActions, [ + { + label: 'PowerShell', + shell: 'powershell', + command: `[Environment]::SetEnvironmentVariable('PATH', 'C:/Users/dev/bin;' + [Environment]::GetEnvironmentVariable('PATH', 'User'), 'User')`, + }, + { + label: 'cmd.exe', + shell: 'cmd', + command: `powershell -Command "[Environment]::SetEnvironmentVariable('PATH', 'C:/Users/dev/bin;' + [Environment]::GetEnvironmentVariable('PATH', 'User'), 'User')"`, + }, + { + label: 'Git Bash', + shell: 'bash', + command: `echo 'export PATH="C:/Users/dev/bin:$PATH"' >> ~/.bashrc`, + }, + ]); + }); + + test('the fish lane gains the end-of-options separator for every path', () => { + const command = laneCommand('persist', '/usr/local/bin', 'linux', 'fish'); + assert.equal(command, `fish_add_path -- '/usr/local/bin'`); + }); + + test('does not write a command substitution into the rc file', () => { + const { escapedDir } = projectPathExportLine('/tmp/a$(id)b'); + assert.equal(escapedDir, '/tmp/a\\$(id)b'); + }); + + test('does not write a backtick substitution into the rc file', () => { + const { escapedDir } = projectPathExportLine('/tmp/a`whoami`b'); + assert.equal(escapedDir, '/tmp/a\\`whoami\\`b'); + }); + + test('keeps the persisted rc line quote-balanced', () => { + const { escapedDir } = projectPathExportLine('/tmp/a"b'); + assert.equal(escapedDir, '/tmp/a\\"b'); + }); + + test('escapes a backslash in the persisted line', () => { + const { escapedDir } = projectPathExportLine('/tmp/a\\b'); + assert.equal(escapedDir, '/tmp/a\\\\b'); + }); + + // #3118 review finding (now fixed): the other four escapers' target contexts + // (PowerShell single-quoted, POSIX double-quoted, POSIX single-quoted, and the win32 + // JSON.stringify-based hook token) all treat a raw newline/CR/NUL as literal data that + // stays inside the quoting/escaping without breaking out, so those four DO survive + // intact and are pinned exactly below. escapeTomlDoubleQuotedString is exercised + // separately below — it now escapes TOML's required control-character ranges rather + // than passing them through raw. + test('the escapers survive newlines and null bytes', () => { + const cases = [ + ['\n', '\n', 'a\\nb'], + ['\r\n', '\r\n', 'a\\r\\nb'], + ['\0', '\0', 'a\\u0000b'], + ]; + for (const [raw, , tomlExpected] of cases) { + const value = `a${raw}b`; + assert.equal(escapePowerShellSingleQuoted(value), `a${raw}b`); + assert.equal(escapePosixDoubleQuoted(value), `a${raw}b`); + assert.equal(escapeSingleQuotedShellLiteral(value), `a${raw}b`); + assert.equal(formatManagedHookScriptToken(value, { platform: 'win32' }), JSON.stringify(`a${raw}b`)); + assert.equal(escapeTomlDoubleQuotedString(value), tomlExpected); + } + }); + + // TOML v1.0.0 basic-string grammar (https://toml.io/en/v1.0.0#string): a basic string + // must escape the quotation mark, backslash, and control characters other than tab + // (U+0000-U+0008, U+000A-U+001F, U+007F). Compact escapes (\b \t \n \f \r \" \\) are + // used where TOML defines them; every other required character falls back to \uXXXX. + // Tab (U+0009) is explicitly excluded from the "must escape" set, so it passes through + // unescaped. Exact expected outputs below were derived by running the built function + // (see dispatch report table). + test('escapes control characters to a parseable TOML basic string', () => { + assert.equal(escapeTomlDoubleQuotedString('\n'), '\\n'); + assert.equal(escapeTomlDoubleQuotedString('\r'), '\\r'); + assert.equal(escapeTomlDoubleQuotedString('\r\n'), '\\r\\n'); + assert.equal(escapeTomlDoubleQuotedString('\0'), '\\u0000'); + assert.equal(escapeTomlDoubleQuotedString('\t'), '\t'); + assert.equal(escapeTomlDoubleQuotedString('\b'), '\\b'); + assert.equal(escapeTomlDoubleQuotedString('\f'), '\\f'); + assert.equal(escapeTomlDoubleQuotedString('\x1F'), '\\u001f'); + assert.equal(escapeTomlDoubleQuotedString('\x7F'), '\\u007f'); + // Ordering pin: backslash escaped FIRST (doubled), then quote, then control chars — + // so a backslash introduced by an earlier escape step is never re-escaped. + assert.equal(escapeTomlDoubleQuotedString('a\\b"c\nd'), 'a\\\\b\\"c\\nd'); + }); + + test('leaves an ordinary value byte-identical', () => { + const value = 'C:/Users/dev/gsd.js'; + assert.equal(escapeTomlDoubleQuotedString(value), value); + }); + + test('produces a TOML basic string that parses back to the original', () => { + // No built-in or dependency TOML parser is available in this environment (Node has + // no built-in TOML support, and neither `toml` nor `@iarna/toml` nor `smol-toml` is a + // project dependency), so this round-trips through a minimal inline unescape that + // implements only the escape forms escapeTomlDoubleQuotedString can produce + // (\\ \" \b \t \n \f \r \uXXXX) — sufficient to invert this encoder, not a general + // TOML parser. + const unescapeTomlBasicString = (escaped) => { + let out = ''; + for (let i = 0; i < escaped.length; i += 1) { + const ch = escaped[i]; + if (ch !== '\\') { + out += ch; + continue; + } + const next = escaped[i + 1]; + if (next === 'u') { + out += String.fromCodePoint(parseInt(escaped.slice(i + 2, i + 6), 16)); + i += 5; + continue; + } + const compact = { b: '\b', t: '\t', n: '\n', f: '\f', r: '\r', '"': '"', '\\': '\\' }[next]; + if (compact === undefined) throw new Error(`unsupported escape: \\${next}`); + out += compact; + i += 1; + } + return out; + }; + + const hostileInputs = ['\n', '\r', '\r\n', '\0', '\t', '\b', '\f', '\x1F', '\x7F', 'a\\b"c\nd']; + for (const input of hostileInputs) { + const escaped = escapeTomlDoubleQuotedString(input); + const tomlLine = `k = "${escaped}"`; + const quoted = tomlLine.slice(tomlLine.indexOf('"') + 1, tomlLine.lastIndexOf('"')); + assert.equal(unescapeTomlBasicString(quoted), input, `round-trip failed for ${JSON.stringify(input)}`); + } + }); + + // A newline in the target directory lands inside a single-quoted `echo '...'` argument + // in the persisted bash/zsh rc lines. POSIX single quotes preserve a literal newline as + // data — they do NOT terminate on a bare newline — so the whole `echo '...' >> ~/.bashrc` + // stays ONE shell command; the newline just makes the persisted PATH assignment a broken, + // two-line value inside .bashrc, not a second executable command. (Contrast: had the + // value been embedded unquoted or inside double quotes followed by an unescaped command + // separator, a newline COULD start a new command — that is not the case here.) + test('a newline in the target directory cannot start a new rc-file command', () => { + const command = laneCommand('persist', '/tmp/a\nmalicious', 'linux', 'bash'); + assert.equal(command, `echo 'export PATH="/tmp/a\nmalicious:$PATH"' >> ~/.bashrc`); + // The newline sits between the opening and closing `'` of the echo argument — it is + // literal data inside one quoted token, not a shell command terminator. + const singleQuoteSpan = command.slice(command.indexOf(`'`) + 1, command.lastIndexOf(`'`)); + assert.ok(singleQuoteSpan.includes('\n'), 'the newline must be inside the single-quoted region'); + }); + + test('keeps the existing apostrophe escaping', () => { + const command = laneCommand('persist', "/tmp/o'brien", 'linux', 'bash'); + assert.ok(command.includes(`'\\''`), 'bash lane must retain the POSIX single-quote escape sequence'); + }); + + test('escapes an apostrophe and a dollar in one path', () => { + const command = laneCommand('persist', "/tmp/o'b$(id)", 'linux', 'bash'); + assert.ok(command.includes(`'\\''`), 'must still contain the POSIX single-quote escape'); + assert.ok(command.includes('\\$('), 'must also contain the escaped dollar-paren'); + }); + + test('all three lanes escape the export line identically', () => { + const { escapedDir: token } = projectPathExportLine('/tmp/a$(id)b'); + const repairCommand = laneCommand('repair', '/tmp/a$(id)b', 'linux', 'posix'); + const persistCommand = laneCommand('persist', '/tmp/a$(id)b', 'linux', 'bash'); + const win32TargetDir = 'C:/a$(id)b'; + const win32Command = laneCommand('repair', win32TargetDir, 'win32', 'bash'); + // #3118: the win32 bash lane posix-normalizes targetDir before escaping it, so its token + // must be compared against the SAME (posix-normalized) input, not against the '/tmp/...' + // token above — those are two different inputs and legitimately escape differently. + const { escapedDir: win32Token } = projectPathExportLine(posixNormalize(win32TargetDir)); + assert.ok(repairCommand.includes(token), 'repair posix lane must use the same escaped token'); + assert.ok(persistCommand.includes(token), 'persist bash lane must use the same escaped token'); + assert.ok(win32Command.includes(win32Token), 'win32 bash lane must use the same escaped token'); + }); + + test('does not let a quote break out of the cmd lane', () => { + const { shellActions, actionLines } = projectPathActionProjection({ + mode: 'repair', + targetDir: 'C:/a"&calc&"b', + platform: 'win32', + }); + // `"` is a reserved character that cannot appear in any Windows path, so + // there is no valid command to suggest; the projection fails closed + // rather than emitting a cmd line whose quoting can be broken. + assert.deepEqual(shellActions, []); + assert.deepEqual(actionLines, []); + }); + + test('still projects win32 lanes for a legal path', () => { + const { shellActions } = projectPathActionProjection({ + mode: 'repair', + targetDir: 'C:/Users/dev/bin', + platform: 'win32', + }); + assert.equal(shellActions.length, 3); + assert.deepEqual(shellActions.map((a) => a.shell), ['powershell', 'cmd', 'bash']); + }); + + test('keeps the PowerShell doubling', () => { + assert.equal(escapePowerShellSingleQuoted("o'brien"), "o''brien"); + }); + + test('an absent target directory produces no actions', () => { + for (const targetDir of [null, undefined, '']) { + assert.deepEqual( + projectPathActionProjection({ mode: 'repair', targetDir, platform: 'linux' }), + { shellActions: [], actionLines: [], reason: PATH_ACTION_REASON.NO_TARGET_DIR }, + ); + } + }); + + test('reports why a win32 quote produced no actions', () => { + const result = projectPathActionProjection({ mode: 'repair', targetDir: 'C:/a"b', platform: 'win32' }); + assert.deepEqual(result.shellActions, []); + assert.equal(result.reason, PATH_ACTION_REASON.WIN32_RESERVED_QUOTE); + }); + + // Two empty results with different causes must stay distinguishable — that + // is the whole subject of epic #3051. + test('reports a missing target directory as a different cause', () => { + const result = projectPathActionProjection({ mode: 'repair', targetDir: null, platform: 'win32' }); + assert.equal(result.reason, PATH_ACTION_REASON.NO_TARGET_DIR); + }); + + test('a successful projection carries no reason', () => { + const result = projectPathActionProjection({ mode: 'repair', targetDir: '/usr/local/bin', platform: 'linux' }); + assert.equal(Object.prototype.hasOwnProperty.call(result, 'reason'), false); + }); + + test('the reason vocabulary is closed', () => { + assert.deepEqual(Object.keys(PATH_ACTION_REASON).sort(), ['NO_TARGET_DIR', 'WIN32_RESERVED_QUOTE']); + assert.ok(Object.isFrozen(PATH_ACTION_REASON)); + }); + + test('leaves the fish lane escaping unchanged', () => { + const command = laneCommand('persist', '/tmp/a$(id)b', 'linux', 'fish'); + assert.equal(command, `fish_add_path -- '/tmp/a$(id)b'`); + }); + + test('emits the fish end-of-options separator for an ordinary path', () => { + // #3118 review MINOR: a leading-dash `targetDir` (e.g. `-v`) is a legal + // directory name, but fish's argparse-based option scanning misparses an + // unseparated leading-dash token as a flag regardless of quoting — + // verified empirically against a real fish 4.8.1 install that + // `fish_add_path '-v'` fails ("No paths to add, not setting anything.") + // while `fish_add_path -- '-v'` succeeds. `--` is fish's standard + // end-of-options separator and is a no-op for ordinary paths. + const command = laneCommand('persist', '/tmp/x', 'linux', 'fish'); + assert.equal(command, `fish_add_path -- '/tmp/x'`); + }); + + test('an empty platform string falls back to the host', () => { + assert.equal( + formatManagedHookScriptToken('/x/y.js', { platform: '' }), + formatManagedHookScriptToken('/x/y.js'), + ); + }); + + test('projects no token off win32', () => { + assert.equal(formatManagedHookScriptToken('/x/y.js', { platform: 'linux' }), null); + }); + + test('projects a JSON-quoted posix-normalized token on win32', () => { + assert.equal( + formatManagedHookScriptToken('C:\\x\\y.js', { platform: 'win32' }), + JSON.stringify('C:/x/y.js'), + ); + }); + + test('returns no lines when called with no arguments', () => { + assert.deepEqual(renderShellActionLines(), []); + }); + + test('drops entries without a command', () => { + assert.deepEqual( + renderShellActionLines([null, { label: 'a' }, { label: 'b', command: 'c' }]), + ['b: c'], + ); + }); + + test('renders an unlabeled action as the bare command', () => { + assert.deepEqual(renderShellActionLines([{ label: null, command: 'x' }]), ['x']); + }); + + test('escapes a backslash-quote pair in the right order', () => { + // Input: a \ " b (a literal backslash immediately followed by a quote). + const input = ['a', '\\', '"', 'b'].join(''); + // Expected: backslash first doubles to two backslashes, THEN the quote + // gets its own backslash — so the quote ends up preceded by 3 backslashes. + const expected = ['a', '\\', '\\', '\\', '"', 'b'].join(''); + assert.equal(escapeTomlDoubleQuotedString(input), expected); + }); + + test('coerces non-string values', () => { + const escapers = [ + escapeTomlDoubleQuotedString, + escapePowerShellSingleQuoted, + escapePosixDoubleQuoted, + escapeSingleQuotedShellLiteral, + ]; + for (const escaper of escapers) { + for (const value of [null, undefined, 0, [], {}]) { + assert.doesNotThrow(() => escaper(value)); + assert.equal(typeof escaper(value), 'string'); + } + } + }); + + test('returns empty for an empty value', () => { + const escapers = [ + escapeTomlDoubleQuotedString, + escapePowerShellSingleQuoted, + escapePosixDoubleQuoted, + escapeSingleQuotedShellLiteral, + ]; + for (const escaper of escapers) { + assert.equal(escaper(''), ''); + } + }); + + describe('retryRenameSync (#3118)', () => { + afterEach(() => { + mock.restoreAll(); + }); + + test('rethrows when the rename cannot be retried to success', () => { + mock.method(fs, 'renameSync', () => { + const e = new Error('EPERM'); + e.code = 'EPERM'; + throw e; + }); + assert.throws(() => retryRenameSync('/a', '/b')); + }); + + test('returns silently on a successful rename', (t) => { + const dir = createTempDir('gsd-3118-rename-'); + t.after(() => cleanup(dir)); + const from = path.join(dir, 'source.txt'); + const to = path.join(dir, 'dest.txt'); + fs.writeFileSync(from, 'content'); + + retryRenameSync(from, to); + + assert.ok(fs.statSync(to).isFile()); + assert.equal(fs.existsSync(from), false); + }); + }); +}); diff --git a/tests/state-rebuild.test.cjs b/tests/state-rebuild.test.cjs index 03a5e6c1f..2ca691972 100644 --- a/tests/state-rebuild.test.cjs +++ b/tests/state-rebuild.test.cjs @@ -30,7 +30,6 @@ const fixedClock = Object.freeze({ nowIso: () => '2026-06-29T12:00:00.000Z', }); -const noProgress = () => null; // #3057 B1: `phaseInventoryProvider` returns a discriminated result, never a // bare array-or-null — `{ ok: true, phases: [] }` is the genuinely-empty // benign case ("nothing to reconcile"), distinct from `{ ok: false, reason }` @@ -39,7 +38,6 @@ const noProgress = () => null; const noPhases = () => ({ ok: true, phases: [] }); const baseDeps = Object.freeze({ - progressProvider: noProgress, clock: fixedClock, phaseInventoryProvider: noPhases, }); diff --git a/tests/state-transition.test.cjs b/tests/state-transition.test.cjs index 89b95e3b4..4d78531c0 100644 --- a/tests/state-transition.test.cjs +++ b/tests/state-transition.test.cjs @@ -17,6 +17,7 @@ const { FIELD_CLASSIFICATION, getFieldClassification, STATE_MD_SECTIONS, + sliceCurrentPositionSection, } = require('../gsd-core/bin/lib/state-transition.cjs'); const { stateExtractField } = require('../gsd-core/bin/lib/state-document.cjs'); @@ -26,8 +27,6 @@ const fixedClock = Object.freeze({ nowIso: () => '2026-06-27T12:00:00.000Z', }); -const noProgress = () => null; - describe('ADR-1769 substrate: field-classification table', () => { const allowedSources = new Set(['body', 'disk', 'external', 'curated', 'free']); const allowedPreservation = new Set([ @@ -140,7 +139,7 @@ describe('ADR-1769 Phase 1: beginPhase transition — tracer bullet', () => { const result = transitionCore( input, { kind: 'beginPhase', phaseNumber: 3, phaseName: 'Test Phase', planCount: 5 }, - { clock: fixedClock, progressProvider: noProgress }, + { clock: fixedClock }, ); assert.ok(result.updated.includes('Status'), `updated should include Status; got ${JSON.stringify(result.updated)}`); @@ -181,7 +180,7 @@ function firstTimeBody() { describe('ADR-1769 Phase 1: beginPhase first-time body field updates', () => { const intent = { kind: 'beginPhase', phaseNumber: 3, phaseName: 'Test Phase', planCount: 5 }; - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('updates Current Phase to N', () => { const result = transitionCore(firstTimeBody(), intent, deps); @@ -260,7 +259,7 @@ function resumeBody() { describe('ADR-1769 Phase 1: #3127 idempotency guard — resume path', () => { const intent = { kind: 'beginPhase', phaseNumber: 3, phaseName: 'Test Phase', planCount: 5 }; - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('Status is still refreshed on resume (Last Activity Date tracks execute-phase runs)', () => { const result = transitionCore(resumeBody(), intent, deps); @@ -299,7 +298,7 @@ describe('ADR-1769 Phase 1: #3127 idempotency guard — resume path', () => { describe('ADR-1769 Phase 1: Current Position section mutation (first-time begin)', () => { const intent = { kind: 'beginPhase', phaseNumber: 3, phaseName: 'Test Phase', planCount: 5 }; - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('Current Position Phase line reflects the new phase (EXECUTING)', () => { const result = transitionCore(firstTimeBody(), intent, deps); @@ -356,7 +355,7 @@ describe('ADR-1769 Phase 1: Current Position section mutation (first-time begin) describe('ADR-1769 Phase 1: Current Position section mutation (resume path)', () => { const intent = { kind: 'beginPhase', phaseNumber: 3, phaseName: 'Test Phase', planCount: 5 }; - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('Resume updates only the Last activity line in Current Position (preserves Plan, Phase, Status)', () => { const result = transitionCore(resumeBody(), intent, deps); @@ -372,7 +371,7 @@ describe('ADR-1769 Phase 1: Current Position section mutation (resume path)', () }); describe('ADR-1769 Phase 1: property tests (RULESET.TESTS.property-based-testing)', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('for any non-negative integer phaseNumber and any STATE.md body with a non-whitespace Status value, beginPhase produces content whose body Status carries "Executing Phase N"', () => { // Note: filters out whitespace-only statusSuffix because state-document.cjs's @@ -419,7 +418,7 @@ describe('ADR-1769 Phase 1: property tests (RULESET.TESTS.property-based-testing }); describe('ADR-1769 Phase 2: advancePlan transition', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('advances Current Plan from N to N+1 (legacy format)', () => { const input = [ @@ -483,7 +482,7 @@ describe('ADR-1769 Phase 2: advancePlan transition', () => { }); describe('ADR-1769 Phase 2: advancePlan with frontmatter (#1255 pattern — codex review)', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('advances plan correctly when STATE.md has YAML frontmatter (body Status not YAML status)', () => { const input = [ @@ -560,7 +559,7 @@ const ROADMAP_3_OF_5 = [ ].join('\n'); describe('ADR-1769 Phase 3: completePhase transition — body field updates', () => { - const deps = { clock: fixedClock, progressProvider: noProgress, roadmapProvider: () => ROADMAP_3_OF_5 }; + const deps = { clock: fixedClock, roadmapProvider: () => ROADMAP_3_OF_5 }; test('Current Phase advances to nextPhaseNum, preserving "of total" and appending the next name', () => { const intent = { @@ -645,7 +644,7 @@ describe('ADR-1769 Phase 3: completePhase transition — body field updates', () }); describe('ADR-1769 Phase 3: completePhase progress derivation (roadmap)', () => { - const deps = { clock: fixedClock, progressProvider: noProgress, roadmapProvider: () => ROADMAP_3_OF_5 }; + const deps = { clock: fixedClock, roadmapProvider: () => ROADMAP_3_OF_5 }; test('Completed Phases is re-derived from the roadmap progress table', () => { const result = transitionCore( @@ -667,7 +666,7 @@ describe('ADR-1769 Phase 3: completePhase progress derivation (roadmap)', () => }); test('when roadmapProvider yields null, existing Completed Phases / Progress are preserved (no crash)', () => { - const nullDeps = { clock: fixedClock, progressProvider: noProgress, roadmapProvider: () => null }; + const nullDeps = { clock: fixedClock, roadmapProvider: () => null }; const result = transitionCore( completePhaseBody(), { kind: 'completePhase', phaseNum: '3', nextPhaseNum: '4', nextPhaseName: null, isLastPhase: false, planCount: 3, summaryCount: 3 }, @@ -685,7 +684,7 @@ describe('ADR-1769 Phase 3: completePhase progress derivation (roadmap)', () => // 'updated' even when nothing changed. test('FAILURE path (roadmap unavailable): Completed Phases / Progress are NOT marked updated — left-as-is is distinguishable from recomputed', () => { - const nullDeps = { clock: fixedClock, progressProvider: noProgress, roadmapProvider: () => null }; + const nullDeps = { clock: fixedClock, roadmapProvider: () => null }; const result = transitionCore( completePhaseBody(), { kind: 'completePhase', phaseNum: '3', nextPhaseNum: '4', nextPhaseName: null, isLastPhase: false, planCount: 3, summaryCount: 3 }, @@ -716,7 +715,7 @@ describe('ADR-1769 Phase 3: completePhase progress derivation (roadmap)', () => }); describe('ADR-1769 Phase 3: completePhase edge cases', () => { - const deps = { clock: fixedClock, progressProvider: noProgress, roadmapProvider: () => ROADMAP_3_OF_5 }; + const deps = { clock: fixedClock, roadmapProvider: () => ROADMAP_3_OF_5 }; test('falls back to the "Phase:" field when "Current Phase:" is absent (stateReplaceFieldWithFallback)', () => { const input = [ @@ -813,7 +812,7 @@ function plannedPhaseBody() { } describe('ADR-1769 Phase 4: plannedPhase transition — body field updates', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('Status advances to "Ready to execute" when the existing value is a template default (Planning)', () => { const result = transitionCore(plannedPhaseBody(), { kind: 'plannedPhase', phaseNumber: 3, planCount: 4 }, deps); @@ -888,7 +887,7 @@ describe('ADR-1769 Phase 4: plannedPhase transition — body field updates', () }); describe('ADR-1769 Phase 4: milestoneSwitch transition — milestone reset', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; function milestoneBody() { return [ @@ -963,7 +962,7 @@ describe('ADR-1769 Phase 4: milestoneSwitch transition — milestone reset', () // ADR-1769 Phase 5: milestoneComplete describe('ADR-1769 Phase 5: milestoneComplete transition — closure write', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; const intent = { kind: 'milestoneComplete', version: 'v1.0', nextMilestoneCommand: '/gsd:new-milestone' }; function preCloseBody() { @@ -1095,7 +1094,7 @@ describe('ADR-1769 Phase 5: milestoneComplete transition — closure write', () // ADR-1769 Phase 6: patch describe('ADR-1769 Phase 6: patch transition — field updates', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('applies each patched field and reports the updated set', () => { const input = [ @@ -1148,7 +1147,7 @@ describe('ADR-1769 Phase 6: patch transition — field updates', () => { // ADR-1769 Phase 7: update, prune, sync describe('ADR-1769 Phase 7: update transition — single body field', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('replaces a body field and reports updated:true', () => { const input = '# Project State\n\n**Status:** Planning\n**Current Plan:** 2\n'; @@ -1173,7 +1172,7 @@ describe('ADR-1769 Phase 7: update transition — single body field', () => { }); describe('ADR-1769 Phase 7: prune transition — section pruning', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('archives Decisions entries at or below the cutoff phase', () => { const input = [ @@ -1223,7 +1222,7 @@ describe('ADR-1769 Phase 7: prune transition — section pruning', () => { }); describe('ADR-1769 Phase 7: sync transition — body writes + #1761', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('updates Total Plans in Phase + Progress bar + Last Activity when bounded', () => { const input = [ @@ -1660,3 +1659,375 @@ describe('bug #21 — STATE.md template must carry YAML frontmatter', () => { }); }); } + +// ──────────────────────────────────────────────────────────────────────── +// #3118: sliceCurrentPositionSection — locator characterization tests. +// ──────────────────────────────────────────────────────────────────────── + +describe('sliceCurrentPositionSection (#3118)', () => { + test('slices the section up to the following heading', () => { + const body = [ + '# Project State', + '', + '## Current Position', + '', + 'Phase: 3 (Test Phase) — EXECUTING', + '', + '## Accumulated Context', + '', + '- A decision worth keeping.', + '', + ].join('\n'); + const result = sliceCurrentPositionSection(body); + assert.ok(result.includes('Phase: 3 (Test Phase) — EXECUTING')); + assert.ok(!result.includes('A decision worth keeping')); + }); + + test('slices to end of document for a trailing section', () => { + const body = [ + '# Project State', + '', + '## Accumulated Context', + '', + '- Earlier decision.', + '', + '## Current Position', + '', + 'Phase: 5 (Final Phase) — EXECUTING', + '', + ].join('\n'); + const result = sliceCurrentPositionSection(body); + assert.ok(result.includes('Phase: 5 (Final Phase) — EXECUTING')); + }); + + test('returns null when the section is absent', () => { + const body = [ + '# Project State', + '', + '## Accumulated Context', + '', + '- Some decision.', + '', + '## Deferred Items', + '', + '- Something deferred.', + '', + ].join('\n'); + assert.strictEqual(sliceCurrentPositionSection(body), null); + }); + + test('matches the heading case- and space-insensitively', () => { + const body = [ + '# Project State', + '', + '## CURRENT POSITION', + '', + 'Phase: 2 — EXECUTING', + '', + ].join('\n'); + assert.notStrictEqual(sliceCurrentPositionSection(body), null); + }); + + test('ignores a Current Position heading inside a code fence', () => { + // The locator is fence-aware via `tokenizeHeadings` — a `##` line inside + // a ``` fence is not a real heading, so this document has zero *real* + // Current Position headings. + const body = [ + '# Project State', + '', + '## Accumulated Context', + '', + '```markdown', + '## Current Position', + '', + 'Phase: 9 — should not be seen', + '```', + '', + ].join('\n'); + assert.strictEqual(sliceCurrentPositionSection(body), null); + }); + + test('distinguishes an empty section from an absent one', () => { + // An empty section and an absent one are different answers, and a caller + // that folds them together reintroduces the collapse this epic removes. + const body = [ + '# Project State', + '', + '## Current Position', + '## Accumulated Context', + '', + '- A decision.', + '', + ].join('\n'); + const result = sliceCurrentPositionSection(body); + assert.strictEqual(typeof result, 'string'); + assert.notStrictEqual(result, null); + assert.strictEqual(result.trim(), ''); + }); + + test('does not match an H3 Current Position', () => { + const body = [ + '# Project State', + '', + '### Current Position', + '', + 'Phase: 2 — EXECUTING', + '', + ].join('\n'); + assert.strictEqual(sliceCurrentPositionSection(body), null); + }); + + test('slices the first Current Position when the document has two', () => { + // `findIndex` picks the first heading match and nothing pinned that + // behavior down before this test. + const body = [ + '# Project State', + '', + '## Current Position', + '', + 'Phase: 3 — FIRST OCCURRENCE', + '', + '## Accumulated Context', + '', + '- unrelated', + '', + '## Current Position', + '', + 'Phase: 9 — SECOND OCCURRENCE', + '', + ].join('\n'); + const result = sliceCurrentPositionSection(body); + assert.ok(result.includes('FIRST OCCURRENCE')); + assert.ok(!result.includes('SECOND OCCURRENCE')); + }); + + test('returns null for an empty document', () => { + assert.strictEqual(sliceCurrentPositionSection(''), null); + }); + + test('slices a CRLF document identically', () => { + // Only `\n` in a regex/split is the recurring CRLF defect class in this + // repo (#1658 and successors) — verify the CRLF fixture, normalized back + // to LF, matches the LF fixture's result byte-for-byte. + const lines = [ + '# Project State', + '', + '## Current Position', + '', + 'Phase: 3 (Test Phase) — EXECUTING', + '', + '## Accumulated Context', + '', + '- A decision worth keeping.', + '', + ]; + const lfResult = sliceCurrentPositionSection(lines.join('\n')); + const crlfResult = sliceCurrentPositionSection(lines.join('\r\n')); + // Strict form (#3118): normalize CRLF->LF and require exact equality with + // the LF result. The looser `.replace(/\r/g, '')` form (previously used + // here) strips ALL `\r` bytes including a stray unpaired trailing `\r` + // left by the pre-fix `end = hs[j].offset - 1` slice — that loose + // assertion is what let the CRLF-slice-defect ship undetected. + assert.strictEqual(crlfResult.replace(/\r\n/g, '\n'), lfResult); + }); + + test('returns an empty string when the section is empty and the next heading follows immediately', () => { + const body = ['# STATE', '', '## Current Position', '## Next Section', 'content'].join('\n'); + const result = sliceCurrentPositionSection(body); + assert.strictEqual(result, ''); + }); + + test('does not duplicate bytes when a transition mutates an empty adjacent section', () => { + // Regression for #3118 review MAJOR: `locateCurrentPosition`'s newline + // walk-back could land `end` before `start` when the section is empty + // and the next heading follows with no blank line between. Every + // mutator's `body.slice(0, start) + sectionBody + body.slice(end)` + // reassembly then duplicated the bytes in the inverted `[end, start)` + // range — a spurious blank line (LF) or `\r\n` (CRLF) inserted into + // STATE.md on every transition. + const lfBody = ['# STATE', '', '## Current Position', '## Next Section', 'content'].join('\n'); + const lfResult = transitionCore( + lfBody, + { kind: 'beginPhase', phaseNumber: 3, phaseName: null, planCount: null }, + { clock: fixedClock }, + ); + assert.ok( + !lfResult.content.includes('## Current Position\n\n## Next Section'), + `expected no inserted blank line; got ${JSON.stringify(lfResult.content)}`, + ); + assert.strictEqual( + lfResult.content, + '# STATE\n\n## Current Position\n## Next Section\ncontent', + ); + assert.strictEqual(lfResult.content.length, lfBody.length); + + const crlfBody = ['# STATE', '', '## Current Position', '## Next Section', 'content'].join('\r\n'); + const crlfResult = transitionCore( + crlfBody, + { kind: 'beginPhase', phaseNumber: 3, phaseName: null, planCount: null }, + { clock: fixedClock }, + ); + assert.ok( + !crlfResult.content.includes('## Current Position\r\n\r\n## Next Section'), + `expected no inserted CRLF; got ${JSON.stringify(crlfResult.content)}`, + ); + assert.strictEqual( + crlfResult.content, + '# STATE\r\n\r\n## Current Position\r\n## Next Section\r\ncontent', + ); + assert.strictEqual(crlfResult.content.length, crlfBody.length); + }); +}); + +// ──────────────────────────────────────────────────────────────────────── +// #3118: deps.progressProvider is a required StateTransitionDeps field with +// 33 supply sites and zero call sites, and is being removed. Prove it +// behaviorally: no transition ever invokes it. +// ──────────────────────────────────────────────────────────────────────── + +describe('state transitions do not consult a progress provider (#3118)', () => { + test('no transition invokes deps.progressProvider', () => { + // An exploding stub is the behavioral form of "this field is inert"; + // asserting the declaration is absent would be source-grep theater. + const exploding = () => { throw new Error('progressProvider must never be called'); }; + const clock = fixedClock; + + assert.doesNotThrow(() => transitionCore( + firstTimeBody(), + { kind: 'beginPhase', phaseNumber: 3, phaseName: 'Test Phase', planCount: 5 }, + { clock, progressProvider: exploding }, + )); + + assert.doesNotThrow(() => transitionCore( + [ + '# Project State', + '', + '**Current Plan:** 02', + '**Total Plans in Phase:** 05', + '**Status:** Executing Phase 3', + '**Last Activity:** 2026-06-26', + '', + '## Current Position', + '', + 'Plan: 2 of 5', + 'Status: Executing Phase 3', + '', + ].join('\n'), + { kind: 'advancePlan' }, + { clock, progressProvider: exploding }, + )); + + assert.doesNotThrow(() => transitionCore( + completePhaseBody(), + { kind: 'completePhase', phaseNum: '3', nextPhaseNum: '4', nextPhaseName: 'Design Phase', isLastPhase: false, planCount: 3, summaryCount: 3 }, + { clock, progressProvider: exploding, roadmapProvider: () => ROADMAP_3_OF_5 }, + )); + + assert.doesNotThrow(() => transitionCore( + plannedPhaseBody(), + { kind: 'plannedPhase', phaseNumber: 3, planCount: 4 }, + { clock, progressProvider: exploding }, + )); + + const milestoneBody = [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'milestone_name: Old Milestone', + 'status: executing', + 'current_phase: "3"', + 'progress:', + ' total_phases: 5', + ' completed_phases: 2', + ' percent: 40', + '---', + '', + '# Project State', + '', + '## Current Position', + '', + 'Phase: 3 — EXECUTING', + 'Plan: 2 of 5', + 'Status: Executing Phase 3', + 'Last activity: 2026-06-20 — mid-flight', + '', + ].join('\n'); + assert.doesNotThrow(() => transitionCore( + milestoneBody, + { kind: 'milestoneSwitch', version: 'v2.0', name: 'New Milestone' }, + { clock, progressProvider: exploding }, + )); + + const preCloseBody = [ + '# Project State', + '', + '**Status:** Executing Phase 5', + '**Last Activity:** 2026-06-20', + '**Last Activity Description:** mid-flight', + '', + '## Current Position', + '', + 'Phase: 5 — EXECUTING', + 'Plan: 2 of 3', + 'Status: Executing Phase 5', + 'Last activity: 2026-06-20 — running', + '', + '## Operator Next Steps', + '', + '- Re-run /gsd:complete-milestone v1.0', + '', + ].join('\n'); + assert.doesNotThrow(() => transitionCore( + preCloseBody, + { kind: 'milestoneComplete', version: 'v1.0', nextMilestoneCommand: '/gsd:new-milestone' }, + { clock, progressProvider: exploding }, + )); + + assert.doesNotThrow(() => transitionCore( + [ + '# Project State', + '', + '**Status:** Planning', + '**Current Plan:** 2', + '**Total Plans in Phase:** 5', + '', + ].join('\n'), + { kind: 'patch', patches: { Status: 'Paused', 'Current Plan': '3' } }, + { clock, progressProvider: exploding }, + )); + + assert.doesNotThrow(() => transitionCore( + '# Project State\n\n**Status:** Planning\n**Current Plan:** 2\n', + { kind: 'update', field: 'Current Plan', value: '3' }, + { clock, progressProvider: exploding }, + )); + + assert.doesNotThrow(() => transitionCore( + [ + '# Session State', + '', + '## Decisions', + '', + '- [Phase 1]: Old', + '- [Phase 3]: Older', + '- [Phase 9]: Recent', + '', + ].join('\n'), + { kind: 'prune', cutoff: 7 }, + { clock, progressProvider: exploding }, + )); + + assert.doesNotThrow(() => transitionCore( + [ + '# Project State', + '', + '**Total Plans in Phase:** 2', + '**Last Activity:** 2026-06-20', + '**Progress:** [████░░░░░░] 40%', + '', + ].join('\n'), + { kind: 'sync', totalPlansInPhase: 5, percent: 60 }, + { clock, progressProvider: exploding }, + )); + }); +});