From 8bc3337fdcbdd04dac3c06f96a9af399979fc147 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 17 Jul 2026 17:06:08 +0000 Subject: [PATCH] =?UTF-8?q?fix(audit):=207=20correctness=20bugs=20found=20?= =?UTF-8?q?by=20adversarial=20review=20of=20the=20v0.19=E2=80=93v0.21=20co?= =?UTF-8?q?de?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial review (13 reviewers → per-finding refutation) of everything shipped this session confirmed 7 real, reachable bugs and rejected the rest. Fixes: - deja: DEJA_FLOOR 0.55 → 0.39 — the old floor exceeded the ~0.42 score() ceiling for a repo-scoped summary, so the déjà-vu advisory could NEVER fire (HIGH; core feature was a silent no-op). Adds an end-to-end recordSessionSummary→dejaAdvisory regression test. - harden: resolve the git hooks dir via `git rev-parse --git-path hooks` — .git is a FILE in a linked worktree/submodule, so the hardcoded .git/hooks threw ENOTDIR (MEDIUM). - ledger_sync: only trust the up-to-date short-circuit when the fetch found the ref on THIS remote — a shared ref name left over from another remote could skip the push (MEDIUM). - cortex_hook_main: preflight halt metric read a non-existent result.gate?.halted → always "pass"; now reads assumption.shouldAsk (MEDIUM). - verify: changedFiles now mirrors added's --cached fallback so the structural lenses don't go silent on a mismatched evidence base (LOW). - radar: match dep names most-specific-first so lodash.debounce isn't swallowed by lodash's member prefix (LOW). - commit_gate: track hunk state so an added line beginning with `++ ` isn't misread as a `+++ ` diff header (LOW). Full suite 887 pass / 0 fail / 2 skip; biome/tsc/docs check green. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Ebv4QDTqzPzgApBW2KUsU --- CHANGELOG.md | 29 +++++++++++++++++++++++++++++ src/commit_gate.js | 18 ++++++++++++++++-- src/cortex_hook_main.js | 6 +++++- src/deja.js | 11 ++++++++--- src/harden.js | 20 ++++++++++++++++++-- src/ledger_sync.js | 11 +++++++++-- src/radar.js | 7 ++++++- src/verify.js | 11 ++++++++++- test/deja.test.js | 22 ++++++++++++++++++++++ 9 files changed, 123 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cdcb54f..e85ec68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,35 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed + +- **`forge deja` could never fire.** `DEJA_FLOOR` was 0.55, above the ~0.42 ceiling + `retrieve()` can produce for a repo-scoped `summary` claim (the σ term is < 0.73 and the + 0.6 repo scope-weight caps it), so the anti-repetition advisory was a permanent silent + no-op. Recalibrated to 0.39 — inside the real band (unrelated ≈0.34, identical ≈0.42) — + with an end-to-end test that drives `recordSessionSummary` → `dejaAdvisory` so the floor + can't drift out of range again. +- **`forge harden` crashed in a linked worktree/submodule.** `.git` is a _file_ there, not + a directory, so `mkdirSync('.git/hooks')` threw `ENOTDIR` and aborted the whole command. + The hooks dir is now resolved via `git rev-parse --git-path hooks`. +- **`forge ledger sync` could silently skip a push to a new/pruned remote.** The idempotence + check compared against a _stale local_ `refs//ledger` left from a prior remote; a + remote lacking the ref was reported `upToDate` without ever receiving the ledger. It now + only trusts the short-circuit when the fetch actually found the ref on this remote. +- **`forge verify --deep` evidence base could diverge.** `added` fell back to `--cached` + but `changedFiles` did not, so the structural lenses (impact/docsdrift) went silent on a + base whose worktree matches HEAD while the index differs. `changedFiles` now mirrors the + same fallback. +- **Cortex halt metrics were always "pass".** The preflight hook read `result.gate?.halted`, + a field `substrateCheck()` never returns, so the cost dashboard's halt-rate was + permanently zero. It now reads the real signal (`assumption.shouldAsk`). +- **`forge radar` usage misattribution.** A dep whose name is a `name.`-prefix of another + (`lodash` vs `lodash.debounce`) could steal the other's import count via a first-match + break in alphabetical order; names are now matched most-specific-first. +- **`forge precommit` diff-header parse.** An added content line beginning with `++ ` + rendered as `+++ ` and was misread as a file header (dropping that line, mis-attributing + the rest); the parser now tracks hunk state so headers are only matched before the `@@`. + ## [0.21.0] - 2026-07-17 ### Added diff --git a/src/commit_gate.js b/src/commit_gate.js index 7f50d23..bde5d0c 100644 --- a/src/commit_gate.js +++ b/src/commit_gate.js @@ -80,13 +80,27 @@ export function stagedAddedLines(root) { const raw = gitRaw(root, ["diff", "--cached", "--unified=0", "--no-color"]); const byFile = new Map(); let file = null; + // A `+++ ` line is the file header ONLY between `diff --git` and the first `@@`; once + // inside a hunk, a `+`-prefixed line is content — including added content that itself + // begins with `++ ` (which renders as `+++ ` in the diff and would otherwise be + // misread as a header, dropping that line and mis-attributing the ones after it). + let inHunk = false; for (const line of raw.split("\n")) { - if (line.startsWith("+++ ")) { + if (line.startsWith("diff --git")) { + file = null; + inHunk = false; + continue; + } + if (!inHunk && line.startsWith("+++ ")) { const p = line.slice(4).trim(); file = p === "/dev/null" ? null : p.replace(/^"?b\//, "").replace(/"$/, ""); continue; } - if (!file || !line.startsWith("+") || line.startsWith("+++")) continue; + if (line.startsWith("@@")) { + inHunk = true; + continue; + } + if (!inHunk || !file || !line.startsWith("+")) continue; if (!byFile.has(file)) byFile.set(file, []); byFile.get(file).push(line.slice(1)); } diff --git a/src/cortex_hook_main.js b/src/cortex_hook_main.js index b4f0ce4..92fc3f8 100644 --- a/src/cortex_hook_main.js +++ b/src/cortex_hook_main.js @@ -157,9 +157,13 @@ async function main() { // blocking the hook. A failing write is silently swallowed. try { const { record } = await import("./metrics.js"); + // substrateCheck() has no `gate` key — the halt signal is the assumption gate + // (same source substrate.js recordGate uses: preflight.assumption.shouldAsk). + // Reading the missing field recorded every prompt as "pass", so the cost + // dashboard's halt-rate was permanently zero. record(root, { stage: "gate", - outcome: result.gate?.halted ? "halt" : "pass", + outcome: result.assumption?.shouldAsk ? "halt" : "pass", }); if (result.route?.key) { record(root, { stage: "route", tier: result.route.tier }); diff --git a/src/deja.js b/src/deja.js index 3bccd3e..61f17cc 100644 --- a/src/deja.js +++ b/src/deja.js @@ -26,9 +26,14 @@ import { epochDay, gitAuthor } from "./util.js"; * edges, fingerprints) are not "have I done this task before" memory. */ export const DEJA_KINDS = ["summary", "lesson", "diagnosis"]; -/** Retrieval score below which a hit is noise, not a déjà vu — calibrated in tests. - * The advisory stays silent under this floor so an unrelated task says nothing. */ -export const DEJA_FLOOR = 0.55; +/** Retrieval score below which a hit is noise, not a déjà vu — calibrated against the + * REAL range of retrieve() for repo-scoped `summary` claims. score() = σ(a·rel+b·rec+g·val) + * × SCOPE_WEIGHT.repo (ledger.js): with a+b+g=1 the σ term is < 0.7311 and the 0.6 repo + * weight caps a same-day identical-task hit at ≈0.42, while an unrelated task sits ≈0.34. + * The floor must live in that band — 0.55 (the old value) exceeded the ceiling, so the + * advisory could NEVER fire. 0.39 clears the noise floor with margin and still catches a + * strong match. A test drives the full path so this stays inside the achievable range. */ +export const DEJA_FLOOR = 0.39; // Same test-command grammar cortex_hook.js keys its S1 signal on — a passing run here // is exactly what "this session's work was verified" means. Kept local (one small diff --git a/src/harden.js b/src/harden.js index e2931fe..bcb08c4 100644 --- a/src/harden.js +++ b/src/harden.js @@ -7,11 +7,27 @@ // rule protects a user-authored pre-commit hook: ours lands beside it as // `pre-commit.` instead of overwriting it. +import { execFileSync } from "node:child_process"; import { chmodSync, existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; -import { join } from "node:path"; +import { isAbsolute, join } from "node:path"; import { BRAND } from "./brand.js"; import { hasBin as have } from "./util.js"; +/** Resolve the real git hooks directory. In a linked worktree or submodule `.git` is a + * FILE (a `gitdir:` pointer), so the naive `/.git/hooks` throws ENOTDIR — ask git + * for the actual path instead, falling back to the classic layout when git is absent. */ +function gitHooksDir(targetRoot) { + try { + const p = execFileSync("git", ["rev-parse", "--git-path", "hooks"], { + cwd: targetRoot, + encoding: "utf8", + }).trim(); + return p && (isAbsolute(p) ? p : join(targetRoot, p)); + } catch { + return join(targetRoot, ".git", "hooks"); + } +} + // The sandbox config to merge into settings — deny the credential dirs an agent should never read. const SANDBOX = { sandbox: { enabled: true, allowUnsandboxedCommands: false }, @@ -53,7 +69,7 @@ export function harden({ targetRoot = process.cwd() } = {}) { report.gitleaks = have("gitleaks") ? "installed — the pre-commit hook runs it first" : "not installed — hook falls back to the built-in secret scan (`brew install gitleaks` for deeper coverage)"; - const hooks = join(targetRoot, ".git", "hooks"); + const hooks = gitHooksDir(targetRoot); mkdirSync(hooks, { recursive: true }); const hookPath = join(hooks, "pre-commit"); let existing = null; diff --git a/src/ledger_sync.js b/src/ledger_sync.js index 96b435a..477d827 100644 --- a/src/ledger_sync.js +++ b/src/ledger_sync.js @@ -210,10 +210,16 @@ export function syncRef( } // PULL: fetch the remote ledger ref. A missing ref is a first sync, not an error; - // anything else (no network, auth) fails open with an honest reason. + // anything else (no network, auth) fails open with an honest reason. The ref name is a + // single fixed name shared across remotes, so a local ref left over from syncing a + // DIFFERENT remote must never be mistaken for THIS remote's state — track whether the + // fetch actually found the ref on this remote, and only trust the idempotence + // short-circuit when it did (else a new/pruned remote is silently never pushed to). + let remoteHasRef = false; let pulled = { claims: 0, records: 0 }; try { run(["fetch", remote, `+${ref}:${ref}`], { cwd: root }); + remoteHasRef = true; } catch (e) { const msg = String(e?.stderr || e?.message || ""); if (!/couldn't find remote ref|not our ref|no matching|does not exist/i.test(msg)) @@ -254,7 +260,7 @@ export function syncRef( try { parentTree = run(["rev-parse", `${ref}^{tree}`], { cwd: root }); } catch {} - if (parentTree === tree) + if (remoteHasRef && parentTree === tree) return { ok: true, ...base, @@ -305,6 +311,7 @@ export function syncRef( retries++; try { run(["fetch", remote, `+${ref}:${ref}`], { cwd: root }); + remoteHasRef = true; // a race means the remote now has the ref } catch { return { ok: false, diff --git a/src/radar.js b/src/radar.js index 75035ef..ca2c2a9 100644 --- a/src/radar.js +++ b/src/radar.js @@ -362,10 +362,15 @@ export function usageFromAtlas(root, names) { return counts; } const edges = Array.isArray(atlas?.edges) ? atlas.edges : []; + // Most-specific name wins: test longer names first so `lodash.debounce` (a real, + // distinct package) is credited its own import rather than being swallowed by the + // `lodash.` member-prefix of the shorter `lodash`, which alphabetical manifest order + // would otherwise test first. + const ordered = [...names].sort((a, b) => b.length - a.length); for (const e of edges) { if (e?.kind !== "imports") continue; const target = String(e.target ?? ""); - for (const n of names) { + for (const n of ordered) { if (target === n || target.startsWith(`${n}/`) || target.startsWith(`${n}.`)) { counts[n]++; break; diff --git a/src/verify.js b/src/verify.js index de7c0d0..4faede1 100644 --- a/src/verify.js +++ b/src/verify.js @@ -93,7 +93,16 @@ export function verify({ targetRoot = process.cwd(), base = "HEAD" } = {}) { .filter((l) => l.startsWith("+") && !l.startsWith("+++")) .map((l) => l.slice(1)) .join("\n"); - const changedFiles = git(["diff", "--name-only", base], targetRoot).split("\n").filter(Boolean); + // Mirror the diff's --cached fallback so the file list is derived from the SAME diff + // that produced `added` — otherwise a base whose worktree matches HEAD but whose index + // differs yields `added` from --cached while changedFiles stays empty, silently + // weakening the structural lenses (impact/docsdrift) that key off changedFiles. + const changedFiles = ( + git(["diff", "--name-only", base], targetRoot) || + git(["diff", "--name-only", "--cached"], targetRoot) + ) + .split("\n") + .filter(Boolean); // Verify runs AFTER edits — a cached, stale atlas would miss newly-added-but-undefined symbols // (false negatives) or flag just-defined ones (false positives). Rebuild when stale; the diff --git a/test/deja.test.js b/test/deja.test.js index bb2a245..e956bd3 100644 --- a/test/deja.test.js +++ b/test/deja.test.js @@ -131,6 +131,28 @@ test("recordSessionSummary mints a retrievable summary; passing tests make it ve assert.equal(hits[0].claim.id, r.id, "the fresh summary is retrievable next session"); }); +test("dejaAdvisory actually fires for a repeated task (DEJA_FLOOR is inside the real range)", () => { + // Regression guard: DEJA_FLOOR must sit below the achievable score() ceiling for a + // repo-scoped summary (~0.42), or the whole anti-repetition feature is a silent no-op. + const root = fixture(); + recordSessionSummary( + root, + "sess-oauth", + [ + { + type: "prompt", + text: "add oauth login flow with pkce to the auth module", + }, + { type: "edit", file: "src/auth.js" }, + ], + 200, + ); + const hit = dejaAdvisory(root, "add oauth login flow with pkce to the auth module", 200); + assert.ok(hit.includes("déjà vu"), "a repeated task surfaces the advisory"); + const miss = dejaAdvisory(root, "optimize the image resizing pipeline for thumbnails", 200); + assert.equal(miss, "", "an unrelated task stays silent (below the noise floor)"); +}); + test("recordSessionSummary is best-effort and returns cleanly on an empty session", () => { const root = fixture(); const r = recordSessionSummary(root, "sess-empty", [], 200);