From e483cef70dd5c19d01dabdcf3afcdfd03eaf91bb Mon Sep 17 00:00:00 2001 From: liolevx <312117550+liolevx@users.noreply.github.com> Date: Fri, 4 Sep 2026 14:51:51 -0400 Subject: [PATCH 1/8] fix: bind verification freshness to full changes --- src/adapters/vcs.ts | 63 +++++++++++++++++++++++- src/orchestration/guided.ts | 28 ++++++++--- tests/orchestration-learning-vcs.test.ts | 23 ++++++++- 3 files changed, 105 insertions(+), 9 deletions(-) diff --git a/src/adapters/vcs.ts b/src/adapters/vcs.ts index 5c84405..56964a4 100644 --- a/src/adapters/vcs.ts +++ b/src/adapters/vcs.ts @@ -1,4 +1,6 @@ -import { lstat, mkdir, readFile, realpath } from "node:fs/promises"; +import { createHash } from "node:crypto"; +import { createReadStream } from "node:fs"; +import { lstat, mkdir, readFile, readlink, realpath } from "node:fs/promises"; import path from "node:path"; import { runProcess } from "./process.js"; import { prepareStateRoot } from "../state/local.js"; @@ -18,6 +20,13 @@ export interface RepositoryBaseline { branch: string; } +export interface ChangeIdentity { + schemaVersion: 1; + baselineRevision: string; + changedPaths: string[]; + changeId: string; +} + function taskSlug(task: string): string { const slug = task .toLowerCase() @@ -70,6 +79,58 @@ export async function revisionInCurrentHistory(root: string, revision: string): return result.exitCode === 0; } +export async function identifyChange( + root: string, + baselineRevision: string, + changedPaths: string[], +): Promise { + const normalizedPaths = [...new Set(changedPaths.map((value) => value.replaceAll("\\", "/")))].sort(); + const hash = createHash("sha256"); + hash.update("noxroot-change-identity-v1\0"); + hash.update(baselineRevision); + + for (const relative of normalizedPaths) { + const absolute = resolveWithin(root, relative); + hash.update("\0path\0"); + hash.update(relative); + let entry; + try { + entry = await lstat(absolute); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + hash.update("\0deleted"); + continue; + } + throw error; + } + + hash.update(`\0mode:${entry.mode.toString(8)}\0size:${entry.size}`); + if (entry.isSymbolicLink()) { + hash.update("\0symlink\0"); + hash.update(await readlink(absolute)); + } else if (entry.isFile()) { + hash.update("\0file\0"); + await new Promise((resolve, reject) => { + const stream = createReadStream(absolute); + stream.on("data", (chunk: Buffer) => hash.update(chunk)); + stream.on("error", reject); + stream.on("end", resolve); + }); + } else if (entry.isDirectory()) { + hash.update("\0directory"); + } else { + hash.update("\0other"); + } + } + + return { + schemaVersion: 1, + baselineRevision, + changedPaths: normalizedPaths, + changeId: hash.digest("hex"), + }; +} + function matchesSensitivePath(relative: string, patterns: string[]): boolean { const normalized = relative.replaceAll("\\", "/"); return patterns.some((value) => { diff --git a/src/orchestration/guided.ts b/src/orchestration/guided.ts index 70c25a7..0489b3c 100644 --- a/src/orchestration/guided.ts +++ b/src/orchestration/guided.ts @@ -3,7 +3,12 @@ import { readFile } from "node:fs/promises"; import path from "node:path"; import type { AgentAdapter, AgentResult, ReviewerResponse } from "../adapters/agents.js"; import { parseReviewerResponse } from "../adapters/agents.js"; -import { captureRepositoryBaseline, diffFromRevision } from "../adapters/vcs.js"; +import { + captureRepositoryBaseline, + diffFromRevision, + identifyChange, + type ChangeIdentity, +} from "../adapters/vcs.js"; import type { ContextPackage, VerificationCommand, VerificationResult } from "../model.js"; import { cliCommand } from "../invocation.js"; import { resolveWithin } from "../security/paths.js"; @@ -24,6 +29,8 @@ export interface GuidedRunRecord extends RunRecord { startedAt: string; finishedAt?: string; changedPaths?: string[]; + changeIdentity?: ChangeIdentity; + /** @deprecated Read-only compatibility with 0.1 task records. */ diffHash?: string; reviewerPackage?: unknown; learningCandidates?: ReviewerResponse["learningCandidates"]; @@ -88,15 +95,21 @@ export async function inspectGuidedContinuation( sensitivePaths: string[] = [], ): Promise { const changedPaths = await changedFiles(root, record.baseline.revision); - const currentDiff = await diffFromRevision(root, record.baseline.revision, sensitivePaths); - const currentDiffHash = createHash("sha256").update(currentDiff).digest("hex"); + const currentIdentity = await identifyChange(root, record.baseline.revision, changedPaths); const latestChecks = record.verification.at(-1) ?? []; let status: ContinuationVerificationStatus; let summary: string; - if (!record.diffHash) { + if (!record.changeIdentity && !record.diffHash) { status = "not-run"; summary = "Not run for the current diff."; - } else if (record.diffHash !== currentDiffHash) { + } else if ( + record.changeIdentity + ? record.changeIdentity.changeId !== currentIdentity.changeId + : record.diffHash !== + createHash("sha256") + .update(await diffFromRevision(root, record.baseline.revision, sensitivePaths)) + .digest("hex") + ) { status = "stale"; summary = "Stale because the diff changed afterward."; } else if ( @@ -231,7 +244,7 @@ export async function finishGuidedRun(input: { record.baseline.revision, input.sensitivePaths ?? [], ); - const diffHash = createHash("sha256").update(diff).digest("hex"); + const changeIdentity = await identifyChange(input.root, record.baseline.revision, changedPaths); const reviewAssessment = assessReviewNeed(changedPaths, diff, record.task); const commands = selectVerification(record.trustedVerificationPolicy, changedPaths); const checks = await executeVerification(input.root, commands, { @@ -240,7 +253,8 @@ export async function finishGuidedRun(input: { const next: GuidedRunRecord = { ...record, changedPaths, - diffHash, + changeIdentity, + diffHash: undefined, reviewAssessment, verification: [...record.verification, checks], verificationGaps: [], diff --git a/tests/orchestration-learning-vcs.test.ts b/tests/orchestration-learning-vcs.test.ts index 0df90ec..71119f5 100644 --- a/tests/orchestration-learning-vcs.test.ts +++ b/tests/orchestration-learning-vcs.test.ts @@ -4,7 +4,7 @@ import { mkdir, readFile, rm, writeFile } from "node:fs/promises"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import type { AgentAdapter, AgentRequest, AgentResult } from "../src/adapters/agents.js"; -import { boundedDiff, prepareIsolatedWorktree } from "../src/adapters/vcs.js"; +import { boundedDiff, identifyChange, prepareIsolatedWorktree } from "../src/adapters/vcs.js"; import { applyLearning, proposeLearnings } from "../src/knowledge/learn.js"; import type { ContextPackage, VerificationResult } from "../src/model.js"; import { orchestrateRun, type RunRecord } from "../src/orchestration/run.js"; @@ -282,6 +282,27 @@ describe("orchestration, worktree isolation, and controlled learning", () => { expect(diff).not.toContain('"user":"new"'); }); + it("fingerprints the complete change even when bounded reviewer evidence keeps the same tail", async () => { + const root = await temporaryDirectory(); + cleanup.push(() => rm(root, { recursive: true, force: true })); + await exec("git", ["init"], { cwd: root }); + await exec("git", ["config", "user.email", "fixture@example.invalid"], { cwd: root }); + await exec("git", ["config", "user.name", "Fixture"], { cwd: root }); + await writeFile(path.join(root, "large.txt"), `${"baseline\n".repeat(20_000)}stable tail\n`); + await exec("git", ["add", "large.txt"], { cwd: root }); + await exec("git", ["commit", "-m", "initial"], { cwd: root }); + const revision = (await exec("git", ["rev-parse", "HEAD"], { cwd: root })).stdout.trim(); + + await writeFile(path.join(root, "large.txt"), `${"first\n".repeat(20_000)}stable tail\n`); + const first = await identifyChange(root, revision, ["large.txt"]); + await writeFile(path.join(root, "large.txt"), `${"second\n".repeat(20_000)}stable tail\n`); + const second = await identifyChange(root, revision, ["large.txt"]); + + expect(first.changeId).not.toBe(second.changeId); + expect(first.changedPaths).toEqual(["large.txt"]); + expect(first).not.toHaveProperty("content"); + }); + it("does not turn a one-off verification gap into project knowledge", async () => { const root = await temporaryDirectory(); cleanup.push(() => rm(root, { recursive: true, force: true })); From fead7c3bd5fd4684550e169b3ad9425b5f424c23 Mon Sep 17 00:00:00 2001 From: liolevx <312117550+liolevx@users.noreply.github.com> Date: Fri, 4 Sep 2026 14:55:32 -0400 Subject: [PATCH 2/8] fix: expose verification scope gaps --- src/adapters/vcs.ts | 9 ++++++-- src/core/intent.ts | 8 ++++++- src/orchestration/guided.ts | 34 ++++++++++++++++++++++++++---- src/verification/index.ts | 21 +++++++++++++++--- tests/process-verification.test.ts | 13 ++++++++++++ tests/task-intent.test.ts | 24 +++++++++++++++++++++ 6 files changed, 99 insertions(+), 10 deletions(-) create mode 100644 tests/task-intent.test.ts diff --git a/src/adapters/vcs.ts b/src/adapters/vcs.ts index 56964a4..e49ba35 100644 --- a/src/adapters/vcs.ts +++ b/src/adapters/vcs.ts @@ -84,7 +84,9 @@ export async function identifyChange( baselineRevision: string, changedPaths: string[], ): Promise { - const normalizedPaths = [...new Set(changedPaths.map((value) => value.replaceAll("\\", "/")))].sort(); + const normalizedPaths = [ + ...new Set(changedPaths.map((value) => value.replaceAll("\\", "/"))), + ].sort(); const hash = createHash("sha256"); hash.update("noxroot-change-identity-v1\0"); hash.update(baselineRevision); @@ -112,7 +114,7 @@ export async function identifyChange( hash.update("\0file\0"); await new Promise((resolve, reject) => { const stream = createReadStream(absolute); - stream.on("data", (chunk: Buffer) => hash.update(chunk)); + stream.on("data", (chunk) => hash.update(chunk)); stream.on("error", reject); stream.on("end", resolve); }); @@ -149,6 +151,7 @@ export async function diffFromRevision( root: string, revision: string, sensitivePaths: string[] = [], + excludedPaths: string[] = [], ): Promise { const tracked = await git(root, ["diff", "--name-only", "-z", revision, "--"], 100_000); if (tracked.exitCode !== 0) return `Diff unavailable: ${tracked.stderr.trim()}`; @@ -175,6 +178,7 @@ export async function diffFromRevision( "--", ".", ...protectedTracked.map((entry) => `:(top,exclude,literal)${entry.path}`), + ...excludedPaths.map((entry) => `:(top,exclude,literal)${entry}`), ], 100_000, ); @@ -191,6 +195,7 @@ export async function diffFromRevision( remaining -= Buffer.byteLength(bounded); } for (const relative of untracked.stdout.split("\0").filter(Boolean).sort()) { + if (excludedPaths.includes(relative)) continue; if (remaining <= 0) break; const absolute = resolveWithin(root, relative); const file = await lstat(absolute); diff --git a/src/core/intent.ts b/src/core/intent.ts index e68683a..23722ae 100644 --- a/src/core/intent.ts +++ b/src/core/intent.ts @@ -1,12 +1,18 @@ import type { TaskIntent } from "../model.js"; -const EXCLUSION = /\b(?:do not|don't|must not|never|without|except(?:ing)?|exclude|avoid)\b/i; +const EXCLUSION = + /\b(?:do not|don't|must not|never|without|except(?:ing)?|exclude|avoid(?:ing)?)\b/i; const ACCEPTANCE = /\b(?:acceptance|must|should|when|so that|ensure|verify)\b/i; const AUTHORITY = /\b(push|merge|deploy|publish|release)\b/gi; function clauses(task: string): string[] { return task .split(/(?:\r?\n|[.;](?:\s|$))/) + .flatMap((value) => + value.split( + /\s+(?:but|while)\s+|\s+and\s+(?=(?:do not|don't|must not|never|avoid|exclude)\b)/i, + ), + ) .flatMap((value) => { const trimmed = value.trim(); const without = /\bwithout\b/i.exec(trimmed); diff --git a/src/orchestration/guided.ts b/src/orchestration/guided.ts index 0489b3c..c6b6b75 100644 --- a/src/orchestration/guided.ts +++ b/src/orchestration/guided.ts @@ -12,7 +12,12 @@ import { import type { ContextPackage, VerificationCommand, VerificationResult } from "../model.js"; import { cliCommand } from "../invocation.js"; import { resolveWithin } from "../security/paths.js"; -import { changedFiles, executeVerification, selectVerification } from "../verification/index.js"; +import { + changedFiles, + executeVerification, + selectVerification, + unmatchedVerificationPaths, +} from "../verification/index.js"; import type { EffectiveAutonomy } from "./autonomy.js"; import type { RunRecord } from "./run.js"; import { assessReviewNeed, type ReviewAssessment } from "./review.js"; @@ -29,6 +34,8 @@ export interface GuidedRunRecord extends RunRecord { startedAt: string; finishedAt?: string; changedPaths?: string[]; + unmatchedChangedPaths?: string[]; + reviewEvidencePath?: string; changeIdentity?: ChangeIdentity; /** @deprecated Read-only compatibility with 0.1 task records. */ diffHash?: string; @@ -94,7 +101,9 @@ export async function inspectGuidedContinuation( record: GuidedRunRecord, sensitivePaths: string[] = [], ): Promise { - const changedPaths = await changedFiles(root, record.baseline.revision); + const changedPaths = (await changedFiles(root, record.baseline.revision)).filter( + (changedPath) => changedPath !== record.reviewEvidencePath, + ); const currentIdentity = await identifyChange(root, record.baseline.revision, changedPaths); const latestChecks = record.verification.at(-1) ?? []; let status: ContinuationVerificationStatus; @@ -237,24 +246,35 @@ export async function finishGuidedRun(input: { if (policyHash(record.trustedVerificationPolicy) !== record.verificationPolicyHash) { throw new Error("The recorded verification policy snapshot is invalid."); } - const changedPaths = await changedFiles(input.root, record.baseline.revision); + const reviewEvidencePath = input.reviewFile + ? path.relative(input.root, resolveWithin(input.root, input.reviewFile)).replaceAll("\\", "/") + : undefined; + const changedPaths = (await changedFiles(input.root, record.baseline.revision)).filter( + (changedPath) => changedPath !== reviewEvidencePath, + ); for (const changedPath of changedPaths) resolveWithin(input.root, changedPath); const diff = await diffFromRevision( input.root, record.baseline.revision, input.sensitivePaths ?? [], + reviewEvidencePath ? [reviewEvidencePath] : [], ); const changeIdentity = await identifyChange(input.root, record.baseline.revision, changedPaths); const reviewAssessment = assessReviewNeed(changedPaths, diff, record.task); const commands = selectVerification(record.trustedVerificationPolicy, changedPaths); + const unmatchedChangedPaths = unmatchedVerificationPaths( + record.trustedVerificationPolicy, + changedPaths, + ); const checks = await executeVerification(input.root, commands, { ...(input.signal === undefined ? {} : { signal: input.signal }), }); const next: GuidedRunRecord = { ...record, changedPaths, + unmatchedChangedPaths, + ...(reviewEvidencePath === undefined ? {} : { reviewEvidencePath }), changeIdentity, - diffHash: undefined, reviewAssessment, verification: [...record.verification, checks], verificationGaps: [], @@ -272,6 +292,12 @@ export async function finishGuidedRun(input: { next.handoff = guidedHandoff(next, checks); return next; } + if (unmatchedChangedPaths.length > 0) { + next.status = "incomplete"; + next.verificationGaps = [`No approved check applies to: ${unmatchedChangedPaths.join(", ")}.`]; + next.handoff = guidedHandoff(next, checks); + return next; + } if (checks.some((result) => result.status === "unavailable")) { next.status = "incomplete"; next.verificationGaps = checks diff --git a/src/verification/index.ts b/src/verification/index.ts index 63426a5..4209efb 100644 --- a/src/verification/index.ts +++ b/src/verification/index.ts @@ -5,10 +5,13 @@ import { scanRepository } from "../detection/scan.js"; import type { VerificationCommand, VerificationResult } from "../model.js"; import { runProcess, type ProcessRequest } from "../adapters/process.js"; -function matches(pattern: string, changedPath: string): boolean { +export function matchesVerificationPath(pattern: string, changedPath: string): boolean { const normalized = changedPath.replaceAll("\\", "/"); if (pattern === "**/*" || pattern === "**") return true; - if (pattern.endsWith("/**")) return normalized.startsWith(pattern.slice(0, -3)); + if (pattern.endsWith("/**")) { + const directory = pattern.slice(0, -3).replace(/\/$/, ""); + return normalized === directory || normalized.startsWith(`${directory}/`); + } if (pattern.startsWith("**/*.")) return normalized.endsWith(pattern.slice(4)); if (!pattern.includes("*")) return normalized === pattern; const escaped = pattern @@ -26,11 +29,23 @@ export function selectVerification( if (changedPaths.length === 0) return []; return commands.filter((command) => command.appliesTo.some((pattern) => - changedPaths.some((changedPath) => matches(pattern, changedPath)), + changedPaths.some((changedPath) => matchesVerificationPath(pattern, changedPath)), ), ); } +export function unmatchedVerificationPaths( + commands: VerificationCommand[], + changedPaths: string[], +): string[] { + return changedPaths.filter( + (changedPath) => + !commands.some((command) => + command.appliesTo.some((pattern) => matchesVerificationPath(pattern, changedPath)), + ), + ); +} + export async function planVerification( root: string, changedPaths: string[] = [], diff --git a/tests/process-verification.test.ts b/tests/process-verification.test.ts index edf9439..c15dca2 100644 --- a/tests/process-verification.test.ts +++ b/tests/process-verification.test.ts @@ -242,4 +242,17 @@ commands: ).toEqual(["trusted-source"]); expect(selectVerification(trustedSnapshot, ["docs/readme.md"])).toEqual([]); }); + + it("matches verification directories on path boundaries", () => { + const command = { + id: "source", + executable: "node", + args: ["--test"], + cwd: ".", + timeoutMs: 1_000, + appliesTo: ["src/**"], + }; + expect(selectVerification([command], ["src/index.ts"])).toEqual([command]); + expect(selectVerification([command], ["src-other/index.ts"])).toEqual([]); + }); }); diff --git a/tests/task-intent.test.ts b/tests/task-intent.test.ts new file mode 100644 index 0000000..e570b89 --- /dev/null +++ b/tests/task-intent.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from "vitest"; +import { parseTaskIntent } from "../src/core/intent.js"; + +describe("task intent", () => { + it.each([ + [ + "Fix the parser but do not change the public API", + "Fix the parser", + "do not change the public API", + ], + ["Fix the parser and do not publish a release", "Fix the parser", "do not publish a release"], + ["Improve the CLI while avoiding new commands", "Improve the CLI", "avoiding new commands"], + ])("separates a required outcome from an inline exclusion: %s", (task, outcome, exclusion) => { + const intent = parseTaskIntent(task); + expect(intent.requiredOutcomes).toEqual([outcome]); + expect(intent.explicitExclusions).toEqual([exclusion]); + }); + + it("does not split an ordinary positive conjunction", () => { + const intent = parseTaskIntent("Fix parsing and add a regression test"); + expect(intent.requiredOutcomes).toEqual(["Fix parsing and add a regression test"]); + expect(intent.explicitExclusions).toEqual([]); + }); +}); From 6aa4c6b834ab03e46ade18330c7fb3cf9f82418c Mon Sep 17 00:00:00 2001 From: liolevx <312117550+liolevx@users.noreply.github.com> Date: Fri, 4 Sep 2026 14:59:36 -0400 Subject: [PATCH 3/8] fix: bind reviews to tasks and changes --- .noxroot/skills/independent-review/SKILL.md | 3 ++ docs/adapters.md | 16 +++++----- docs/architecture.md | 6 ++-- docs/commands.md | 11 ++++--- docs/security.md | 5 +-- src/adapters/agents.ts | 24 +++++++++++++-- src/core/proposals.ts | 2 +- src/orchestration/guided.ts | 11 ++++++- tests/agent-review.test.ts | 19 ++++++++++++ tests/autonomy-guided.test.ts | 34 ++++++++++++++++++++- tests/orchestration-learning-vcs.test.ts | 3 ++ 11 files changed, 112 insertions(+), 22 deletions(-) diff --git a/.noxroot/skills/independent-review/SKILL.md b/.noxroot/skills/independent-review/SKILL.md index e559681..beaefb2 100644 --- a/.noxroot/skills/independent-review/SKILL.md +++ b/.noxroot/skills/independent-review/SKILL.md @@ -16,6 +16,9 @@ For automated mode, emit exactly one JSON object and no prose: ```json { + "schemaVersion": 2, + "taskId": "copy from task package", + "changeId": "copy from task package", "decision": "approved|changes-requested|blocked", "summary": "factual summary", "findings": [ diff --git a/docs/adapters.md b/docs/adapters.md index c977ece..8a8fcf3 100644 --- a/docs/adapters.md +++ b/docs/adapters.md @@ -23,13 +23,15 @@ The executable must understand this protocol. A vendor CLI name alone does not m use documented arguments or a wrapper that translates the task package. Install and authenticate that tool separately. Noxroot does not supply provider accounts, credentials, or model access. -Roles are `worker`, `repair`, and `reviewer`. An automated reviewer must write exactly one JSON -object to standard output with `decision`, `summary`, `findings`, and `learningCandidates`. Findings -require severity, evidence, and required outcome; optional paths are repository-relative. Prose, -additional text, missing fields, unknown fields, truncated output, nonzero exit, and a decision -printed only on standard error all block approval. Diagnostics remain separate on standard error. -Every invocation is a fresh process. Noxroot does not use a shell, interpolate repository text into -arguments, bypass permissions, or promise undocumented vendor flags. +Roles are `worker`, `repair`, and `reviewer`. An automated reviewer must write exactly one version 2 +JSON object to standard output. It must copy `taskId` and `changeId` from the reviewer package, then +provide `decision`, `summary`, `findings`, and `learningCandidates`. This binding prevents an older +or unrelated review from approving the current change. Findings require severity, evidence, and +required outcome; optional paths are repository-relative. Prose, additional text, missing fields, +unknown fields, a mismatched id, truncated output, nonzero exit, and a decision printed only on +standard error all block approval. Diagnostics remain separate on standard error. Every invocation +is a fresh process. Noxroot does not use a shell, interpolate repository text into arguments, bypass +permissions, or promise undocumented vendor flags. Before delegated implementation, preflight resolves the configured executable, validates literal arguments, checks repository write access and a committed Git baseline, and confirms executables for diff --git a/docs/architecture.md b/docs/architecture.md index cdfd5b7..4c521a0 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -49,8 +49,10 @@ evidence belongs in `.noxroot/knowledge/`. Guided orchestration is a two-command lifecycle. Start persists repository identity, clean revision, bounded context, effective autonomy, and a hash of the approved verification policy. Finish derives -the real diff and affected checks from that snapshot, then emits a portable reviewer package or a -strict decision. Local state is never treated as application runtime state. +the real changed paths and approved checks from that snapshot. A full-content change id establishes +freshness without retaining file contents; separately bounded and redacted diff evidence supports +display and review. Reviewer decisions must repeat the package's task and change ids. Local state is +never treated as application runtime state. Completed and approved local records are pruned by age and count after a run finishes. Running, incomplete, failed, blocked, review-pending, and malformed recovery evidence is never removed by diff --git a/docs/commands.md b/docs/commands.md index f771155..71f07f0 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -160,11 +160,12 @@ eligible task is active; multiple tasks require an explicit id. Finish validates and the policy snapshot, computes the actual diff, and runs matching approved checks. Routine checked changes become `completed` without a reviewer. User-facing, security-sensitive, and unusually broad diffs produce a review package and may become `review-pending`. Only a schema-valid -reviewer can produce `approved`. No matching or available check becomes `incomplete`: local handoff -can continue, but approval cannot. Finish also reports a deterministic documentation/learning -assessment without a new model call. When no deterministic documentation signal exists, -documentation is reported as `not-assessed`; an empty deterministic learning assessment is reported -as `no-candidate`, not as proof that no documentation could help. +reviewer response bound to the package's task and full-change ids can produce `approved`. A valid +response for an older or unrelated change is rejected. No matching or available check becomes +`incomplete`: local handoff can continue, but approval cannot. Finish also reports a deterministic +documentation/learning assessment without a new model call. When no deterministic documentation +signal exists, documentation is reported as `not-assessed`; an empty deterministic learning +assessment is reported as `no-candidate`, not as proof that no documentation could help. ## `learn` diff --git a/docs/security.md b/docs/security.md index e220605..1ab4103 100644 --- a/docs/security.md +++ b/docs/security.md @@ -31,8 +31,9 @@ Guided start requires a clean committed baseline. Finish validates repository id policy snapshot, derives actual changed paths, includes bounded tracked and new-file diff evidence, and treats zero matching checks or unavailable executables as blockers. Diff evidence records the path but omits contents for suspected secrets, configured sensitive paths, and symlinks; the same -redaction applies to connected-agent reviewer packages. Reviewer files are resolved inside the -repository and must satisfy the same strict JSON contract as command reviewers. +redaction applies to connected-agent reviewer packages. Freshness uses a separate full-change hash; +file contents are streamed into it, not retained in task state. Reviewer files are resolved inside +the repository and must satisfy the same strict bound JSON contract as command reviewers. Negative guarantees are release blockers. A newly discovered path to a preview write, child command, agent call, network attempt, secret disclosure, or path escape requires a regression test before diff --git a/src/adapters/agents.ts b/src/adapters/agents.ts index 851decd..d74a97b 100644 --- a/src/adapters/agents.ts +++ b/src/adapters/agents.ts @@ -28,6 +28,9 @@ export interface AgentResult { export const reviewerResponseSchema = z .object({ + schemaVersion: z.literal(2), + taskId: z.string().trim().min(1).max(200), + changeId: z.string().regex(/^[a-f0-9]{64}$/), decision: z.enum(["approved", "changes-requested", "blocked"]), summary: z.string().trim().min(1).max(2_000), findings: z @@ -95,11 +98,21 @@ export class ManualAgentAdapter implements AgentAdapter { } } -export function parseReviewerResponse(output: string): ReviewerResponse | undefined { +export function parseReviewerResponse( + output: string, + expected?: { taskId: string; changeId: string }, +): ReviewerResponse | undefined { try { const decoded: unknown = JSON.parse(output); const parsed = reviewerResponseSchema.safeParse(decoded); - return parsed.success ? parsed.data : undefined; + if (!parsed.success) return undefined; + if ( + expected && + (parsed.data.taskId !== expected.taskId || parsed.data.changeId !== expected.changeId) + ) { + return undefined; + } + return parsed.data; } catch { return undefined; } @@ -171,9 +184,14 @@ export class CommandAgentAdapter implements AgentAdapter { exitCode: evidence.exitCode, }; if (request.role === "reviewer") { + const candidate = request.package as { taskId?: unknown; changeId?: unknown }; + const expected = + typeof candidate.taskId === "string" && typeof candidate.changeId === "string" + ? { taskId: candidate.taskId, changeId: candidate.changeId } + : undefined; const review = evidence.exitCode === 0 && !evidence.outputTruncated - ? parseReviewerResponse(evidence.stdout) + ? parseReviewerResponse(evidence.stdout, expected) : undefined; if (review) { result.review = review; diff --git a/src/core/proposals.ts b/src/core/proposals.ts index e86b026..f6e06d1 100644 --- a/src/core/proposals.ts +++ b/src/core/proposals.ts @@ -75,7 +75,7 @@ Inspect the diff independently of worker rationale. Check acceptance criteria, c For automated mode, emit exactly one JSON object and no prose: \`\`\`json -{"decision":"approved|changes-requested|blocked","summary":"factual summary","findings":[{"severity":"critical|high|medium|low","path":"optional/path","evidence":"specific evidence","requiredOutcome":"required result"}],"learningCandidates":[]} +{"schemaVersion":2,"taskId":"copy from task package","changeId":"copy from task package","decision":"approved|changes-requested|blocked","summary":"factual summary","findings":[{"severity":"critical|high|medium|low","path":"optional/path","evidence":"specific evidence","requiredOutcome":"required result"}],"learningCandidates":[]} \`\`\` `; diff --git a/src/orchestration/guided.ts b/src/orchestration/guided.ts index c6b6b75..c586869 100644 --- a/src/orchestration/guided.ts +++ b/src/orchestration/guided.ts @@ -325,6 +325,9 @@ export async function finishGuidedRun(input: { } const reviewerPackage = { + schemaVersion: 2, + taskId: record.id, + changeId: changeIdentity.changeId, task: record.task, context: record.context, changedPaths, @@ -332,6 +335,9 @@ export async function finishGuidedRun(input: { verification: checks, reviewAssessment, responseContract: { + schemaVersion: 2, + taskId: record.id, + changeId: changeIdentity.changeId, decision: "approved | changes-requested | blocked", summary: "short factual summary", findings: ["severity, optional path, evidence, requiredOutcome"], @@ -342,7 +348,10 @@ export async function finishGuidedRun(input: { let reviewResult: AgentResult | undefined; if (input.reviewFile) { const source = await readFile(resolveWithin(input.root, input.reviewFile), "utf8"); - const review = parseReviewerResponse(source); + const review = parseReviewerResponse(source, { + taskId: record.id, + changeId: changeIdentity.changeId, + }); reviewResult = review ? { invoked: false, diff --git a/tests/agent-review.test.ts b/tests/agent-review.test.ts index 7387291..f4d3904 100644 --- a/tests/agent-review.test.ts +++ b/tests/agent-review.test.ts @@ -8,6 +8,9 @@ import type { ProcessEvidence } from "../src/model.js"; function response(decision: ReviewerResponse["decision"]): ReviewerResponse { return { + schemaVersion: 2, + taskId: "task-fixture", + changeId: "a".repeat(64), decision, summary: `${decision} from deterministic fixture`, findings: @@ -66,6 +69,22 @@ describe("strict reviewer protocol", () => { expect(parseReviewerResponse(output)).toBeUndefined(); }); + it("rejects a valid decision bound to another task or change", () => { + const approved = response("approved"); + expect( + parseReviewerResponse(JSON.stringify(approved), { + taskId: "another-task", + changeId: approved.changeId, + }), + ).toBeUndefined(); + expect( + parseReviewerResponse(JSON.stringify(approved), { + taskId: approved.taskId, + changeId: "b".repeat(64), + }), + ).toBeUndefined(); + }); + it("parses only stdout and blocks invalid output even when stderr says approved", async () => { const adapter = new CommandAgentAdapter( "fixture", diff --git a/tests/autonomy-guided.test.ts b/tests/autonomy-guided.test.ts index d51840c..e57f9d6 100644 --- a/tests/autonomy-guided.test.ts +++ b/tests/autonomy-guided.test.ts @@ -179,7 +179,12 @@ commands: const pending = await cli(["finish", "--json", "--root", root]); const pendingValue = JSON.parse(pending.stdout) as { - record: { status: string; calls: unknown[] }; + record: { + id: string; + status: string; + calls: unknown[]; + changeIdentity: { changeId: string }; + }; completion: { documentation: { status: string }; learning: { status: string } }; }; expect(pendingValue.record.status).toBe("completed"); @@ -191,6 +196,9 @@ commands: await writeFile( reviewPath, JSON.stringify({ + schemaVersion: 2, + taskId: pendingValue.record.id, + changeId: pendingValue.record.changeIdentity.changeId, decision: "approved", summary: "The actual diff and affected check passed.", findings: [], @@ -561,6 +569,30 @@ agents: {default: manual, adapters: {manual: {type: manual}}} await writeFile( path.join(root, reviewPath), JSON.stringify({ + schemaVersion: 2, + taskId: pending.id, + changeId: "0".repeat(64), + decision: "approved", + summary: "This approval belongs to a different change.", + findings: [], + learningCandidates: [], + }), + ); + const mismatched = await finishGuidedRun({ + root, + record: pending, + adapter: new ManualAgentAdapter(), + reviewAuthorized: false, + reviewFile: reviewPath, + }); + expect(mismatched.status).toBe("blocked"); + + await writeFile( + path.join(root, reviewPath), + JSON.stringify({ + schemaVersion: 2, + taskId: pending.id, + changeId: pending.changeIdentity!.changeId, decision: "approved", summary: "The bounded change and check evidence are acceptable.", findings: [], diff --git a/tests/orchestration-learning-vcs.test.ts b/tests/orchestration-learning-vcs.test.ts index 71119f5..a976d60 100644 --- a/tests/orchestration-learning-vcs.test.ts +++ b/tests/orchestration-learning-vcs.test.ts @@ -342,6 +342,9 @@ describe("orchestration, worktree isolation, and controlled learning", () => { exitCode: 0, reviewDecision: "approved", review: { + schemaVersion: 2, + taskId: "task-structured", + changeId: "a".repeat(64), decision: "approved", summary: "review complete", findings: [], From 46e5fcae980d902aad7a5f94048d73949305ee4e Mon Sep 17 00:00:00 2001 From: liolevx <312117550+liolevx@users.noreply.github.com> Date: Fri, 4 Sep 2026 15:03:53 -0400 Subject: [PATCH 4/8] fix: admit learning only from current approvals --- docs/architecture.md | 19 +++---- docs/commands.md | 10 ++-- src/knowledge/learn.ts | 37 ++++++++++++-- src/orchestration/guided.ts | 7 ++- tests/cli.test.ts | 2 +- tests/learning-bounds.test.ts | 25 ++++++++- tests/orchestration-learning-vcs.test.ts | 64 ++++++++++++++++++++++-- 7 files changed, 141 insertions(+), 23 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 4c521a0..9eb7e90 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -74,15 +74,16 @@ state, memory, and user data are not Noxroot project knowledge. The MVP uses gen detection, approved native tests/evals, and the command-adapter protocol; framework-specific semantic modules are deferred. -Controlled learning consumes deterministic verification evidence or already parsed structured -reviewer candidates. Deterministic signatures deduplicate Noxroot-owned knowledge; first creation -also updates the index. Every proposed entry names its confirmation date and source task. Per-file -and total corpus bounds prevent accumulated Markdown from silently consuming future context. -Learning writes are capped at 1,000,000 bytes across Markdown files, including nested files and -index growth. The limit is rechecked when a proposal is applied. Symbolic-link destinations are -refused. A full destination requires deliberate consolidation before another write. Canonical -`.noxroot/skills/*/SKILL.md` files are short, standards-compatible procedures selected through -ordinary routing, not a new skill runtime or vendor-specific tree. +Controlled learning consumes the current approved review's structured candidates. It rejects legacy, +failed, superseded, or subsequently edited task evidence. Deterministic signatures deduplicate +Noxroot-owned knowledge; first creation also updates the index. Every proposed entry names its +confirmation date and source task. Per-file and total corpus bounds prevent accumulated Markdown +from silently consuming future context. Learning writes are capped at 1,000,000 bytes across +Markdown files, including nested files and index growth. The limit is rechecked when a proposal is +applied. Symbolic-link destinations are refused. A full destination requires deliberate +consolidation before another write. Canonical `.noxroot/skills/*/SKILL.md` files are short, +standards-compatible procedures selected through ordinary routing, not a new skill runtime or +vendor-specific tree. Trust boundaries are described in [security.md](security.md). Public behavior belongs in tests before it is claimed in the README. diff --git a/docs/commands.md b/docs/commands.md index 71f07f0..ca68bde 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -174,7 +174,9 @@ assessment is reported as `no-candidate`, not as proof that no documentation cou become project knowledge merely because it occurred once. Proposals show evidence, expected value, duplication/conflict results, content, and whether an executable guardrail is better. `--apply` requires confirmation; the first learnings file and index link are written in the same operation. -Raw prose, task text, sessions, user data, secrets, and external human docs are not converted into -knowledge. New entries carry a confirmation date and source task id. Noxroot refuses another entry -when the destination would exceed `context.documentWarningBytes`; existing knowledge must then be -consolidated or superseded deliberately. +Candidates are eligible only after an approved review, and only while the complete change still +matches that approval. Earlier reviewer calls and candidates from failed or superseded attempts are +not reused. Raw prose, task text, sessions, user data, secrets, and external human docs are not +converted into knowledge. New entries carry a confirmation date and source task id. Noxroot refuses +another entry when the destination would exceed `context.documentWarningBytes`; existing knowledge +must then be consolidated or superseded deliberately. diff --git a/src/knowledge/learn.ts b/src/knowledge/learn.ts index 4ff046b..be8aed2 100644 --- a/src/knowledge/learn.ts +++ b/src/knowledge/learn.ts @@ -2,9 +2,11 @@ import { createHash } from "node:crypto"; import { lstat, mkdir, readFile, readdir, rename, stat, writeFile } from "node:fs/promises"; import path from "node:path"; import type { ReviewerResponse } from "../adapters/agents.js"; +import { identifyChange, type ChangeIdentity } from "../adapters/vcs.js"; import { loadConfig } from "../config/load.js"; import type { RunRecord } from "../orchestration/run.js"; import { resolveWithin } from "../security/paths.js"; +import { changedFiles } from "../verification/index.js"; export type LearningKind = "knowledge" | "decision" | "procedure" | "verification"; @@ -146,10 +148,29 @@ function structuredCandidates(run: RunRecord): ReviewerResponse["learningCandida learningCandidates?: ReviewerResponse["learningCandidates"]; } ).learningCandidates; - return [ - ...(direct ?? []), - ...run.calls.flatMap((call) => call.result.review?.learningCandidates ?? []), - ]; + return direct ?? []; +} + +async function currentApprovedChange(root: string, run: RunRecord): Promise { + const guided = run as RunRecord & { + mode?: unknown; + baseline?: { revision?: unknown }; + changeIdentity?: ChangeIdentity; + reviewEvidencePath?: string; + }; + if ( + run.status !== "approved" || + guided.mode !== "guided" || + typeof guided.baseline?.revision !== "string" || + !guided.changeIdentity + ) { + return false; + } + const changedPaths = (await changedFiles(root, guided.baseline.revision)).filter( + (changedPath) => changedPath !== guided.reviewEvidencePath, + ); + const current = await identifyChange(root, guided.baseline.revision, changedPaths); + return current.changeId === guided.changeIdentity.changeId; } function fromReviewer( @@ -190,6 +211,14 @@ ${candidate.content.trim()} } export async function proposeLearnings(root: string, run: RunRecord): Promise { + if (!(await currentApprovedChange(root, run))) { + return { + taskId: run.id, + proposals: [], + rejected: [], + message: "Learning requires an approved review of the current unchanged diff", + }; + } const candidates = structuredCandidates(run) .map(fromReviewer) .filter((candidate): candidate is Candidate => candidate !== undefined); diff --git a/src/orchestration/guided.ts b/src/orchestration/guided.ts index c586869..bcd9438 100644 --- a/src/orchestration/guided.ts +++ b/src/orchestration/guided.ts @@ -269,8 +269,13 @@ export async function finishGuidedRun(input: { const checks = await executeVerification(input.root, commands, { ...(input.signal === undefined ? {} : { signal: input.signal }), }); + const freshRecord = { ...record }; + delete freshRecord.learningCandidates; + delete freshRecord.reviewerPackage; + delete freshRecord.reviewDecision; + delete freshRecord.finishedAt; const next: GuidedRunRecord = { - ...record, + ...freshRecord, changedPaths, unmatchedChangedPaths, ...(reviewEvidencePath === undefined ? {} : { reviewEvidencePath }), diff --git a/tests/cli.test.ts b/tests/cli.test.ts index fe564fa..e052990 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -329,7 +329,7 @@ describe("CLI contracts", () => { expect(JSON.parse(machine.stdout)).toMatchObject({ taskId: "completed-task", proposals: [], - message: "No durable learning identified", + message: "Learning requires an approved review of the current unchanged diff", }); }); }); diff --git a/tests/learning-bounds.test.ts b/tests/learning-bounds.test.ts index e564f73..6e75277 100644 --- a/tests/learning-bounds.test.ts +++ b/tests/learning-bounds.test.ts @@ -1,11 +1,15 @@ +import { execFile } from "node:child_process"; import { mkdir, readFile, rm, symlink, writeFile } from "node:fs/promises"; import path from "node:path"; +import { promisify } from "node:util"; import { afterEach, describe, expect, it } from "vitest"; +import { identifyChange } from "../src/adapters/vcs.js"; import { applyLearning, proposeLearnings, type LearningProposal } from "../src/knowledge/learn.js"; import type { RunRecord } from "../src/orchestration/run.js"; import { temporaryDirectory } from "./helpers.js"; const roots: string[] = []; +const exec = promisify(execFile); afterEach(async () => Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))), ); @@ -19,6 +23,25 @@ async function fullCorpus() { return root; } +async function approvedCurrentChange(root: string, run: T) { + await exec("git", ["init"], { cwd: root }); + await exec("git", ["config", "user.email", "fixture@example.invalid"], { cwd: root }); + await exec("git", ["config", "user.name", "Fixture"], { cwd: root }); + await writeFile(path.join(root, ".learning-evidence"), "baseline\n"); + await exec("git", ["add", "."], { cwd: root }); + await exec("git", ["commit", "-m", "learning baseline"], { cwd: root }); + const revision = (await exec("git", ["rev-parse", "HEAD"], { cwd: root })).stdout.trim(); + await writeFile(path.join(root, ".learning-evidence"), "validated change\n"); + const changedPaths = [".learning-evidence"]; + return { + ...run, + mode: "guided" as const, + baseline: { revision, status: "" }, + changedPaths, + changeIdentity: await identifyChange(root, revision, changedPaths), + }; +} + const proposal: LearningProposal = { id: "knowledge-corpus", signature: "corpus-bound", @@ -82,7 +105,7 @@ describe("durable knowledge corpus bounds", () => { }, ], }; - const result = await proposeLearnings(root, run); + const result = await proposeLearnings(root, await approvedCurrentChange(root, run)); expect(result.proposals).toEqual([]); expect(result.rejected[0]?.reason).toContain("corpus"); }); diff --git a/tests/orchestration-learning-vcs.test.ts b/tests/orchestration-learning-vcs.test.ts index a976d60..60e7e3a 100644 --- a/tests/orchestration-learning-vcs.test.ts +++ b/tests/orchestration-learning-vcs.test.ts @@ -14,6 +14,26 @@ const exec = promisify(execFile); const cleanup: Array<() => Promise> = []; afterEach(async () => Promise.all(cleanup.splice(0).map((operation) => operation()))); +async function approvedCurrentChange(root: string, run: T) { + await exec("git", ["init"], { cwd: root }); + await exec("git", ["config", "user.email", "fixture@example.invalid"], { cwd: root }); + await exec("git", ["config", "user.name", "Fixture"], { cwd: root }); + const evidencePath = path.join(root, ".learning-evidence"); + await writeFile(evidencePath, "baseline\n"); + await exec("git", ["add", "."], { cwd: root }); + await exec("git", ["commit", "-m", "learning baseline"], { cwd: root }); + const revision = (await exec("git", ["rev-parse", "HEAD"], { cwd: root })).stdout.trim(); + await writeFile(evidencePath, "validated change\n"); + const changedPaths = [".learning-evidence"]; + return { + ...run, + mode: "guided" as const, + baseline: { revision, status: "" }, + changedPaths, + changeIdentity: await identifyChange(root, revision, changedPaths), + }; +} + const context: ContextPackage = { task: "change greeting", interpretation: "bounded greeting change", @@ -317,7 +337,9 @@ describe("orchestration, worktree isolation, and controlled learning", () => { }; const result = await proposeLearnings(root, run); expect(result.proposals).toEqual([]); - expect(result.message).toBe("No durable learning identified"); + expect(result.message).toBe( + "Learning requires an approved review of the current unchanged diff", + ); await expect(readFile(path.join(root, ".noxroot", "knowledge"))).rejects.toThrow(); }); @@ -379,7 +401,13 @@ describe("orchestration, worktree isolation, and controlled learning", () => { }, ], }; - const result = await proposeLearnings(root, run); + const result = await proposeLearnings( + root, + await approvedCurrentChange(root, { + ...run, + learningCandidates: run.calls[0]!.result.review!.learningCandidates, + }), + ); expect(result.proposals).toHaveLength(1); expect(result.proposals[0]).toMatchObject({ kind: "decision", @@ -397,6 +425,36 @@ describe("orchestration, worktree isolation, and controlled learning", () => { ]); }); + it("rejects learning candidates after the approved change is edited", async () => { + const root = await temporaryDirectory(); + cleanup.push(() => rm(root, { recursive: true, force: true })); + const run = await approvedCurrentChange(root, { + id: "task-stale-learning", + task: "validated task", + status: "approved" as const, + calls: [], + verification: [], + verificationGaps: [], + handoff: "", + learningCandidates: [ + { + kind: "knowledge" as const, + destination: ".noxroot/knowledge/learnings.md", + evidence: ["validated evidence"], + expectedValue: "Preserve a stable rule.", + content: "This candidate belongs only to the reviewed change.", + whyNotExecutable: "It records rationale.", + }, + ], + }); + await writeFile(path.join(root, ".learning-evidence"), "edited after approval\n"); + + const result = await proposeLearnings(root, run); + + expect(result.proposals).toEqual([]); + expect(result.message).toContain("current unchanged diff"); + }); + it("stops learning growth when the destination needs consolidation", async () => { const root = await temporaryDirectory(); cleanup.push(() => rm(root, { recursive: true, force: true })); @@ -430,7 +488,7 @@ describe("orchestration, worktree isolation, and controlled learning", () => { ], }; - const result = await proposeLearnings(root, run); + const result = await proposeLearnings(root, await approvedCurrentChange(root, run)); expect(result.proposals).toEqual([]); expect(result.rejected).toEqual([ From 2eb42e82a17ec3ebd8f1ba9d2e82fb955fe2ab1a Mon Sep 17 00:00:00 2001 From: liolevx <312117550+liolevx@users.noreply.github.com> Date: Fri, 4 Sep 2026 15:07:53 -0400 Subject: [PATCH 5/8] test: update live review contract fixture --- tests/acceptance/p1-p2-live.mjs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/acceptance/p1-p2-live.mjs b/tests/acceptance/p1-p2-live.mjs index 38a332d..4ef743b 100644 --- a/tests/acceptance/p1-p2-live.mjs +++ b/tests/acceptance/p1-p2-live.mjs @@ -160,6 +160,9 @@ if (mode === "prepare") { state.firstDiff = git(state.app, ["diff"]); // Scripted review input validates plumbing, not an independent model's discovery of the lesson. await save(path.join(state.app, ".noxroot/local/fixture-review.json"), { + schemaVersion: 2, + taskId: state.firstTask, + changeId: finished.record.changeIdentity.changeId, decision: "approved", summary: "Scripted acceptance review: exact millisecond delays and invalid-attempt regression passed.", From c891d76fb5506dbd50fcba90ddbe7f2d34f73ddc Mon Sep 17 00:00:00 2001 From: liolevx <312117550+liolevx@users.noreply.github.com> Date: Fri, 4 Sep 2026 15:09:25 -0400 Subject: [PATCH 6/8] fix: fail closed on incomplete change metadata --- src/adapters/vcs.ts | 13 +++++++++++-- src/knowledge/learn.ts | 6 +++--- src/orchestration/guided.ts | 12 ++++++------ src/verification/index.ts | 18 ++++++++++++++---- 4 files changed, 34 insertions(+), 15 deletions(-) diff --git a/src/adapters/vcs.ts b/src/adapters/vcs.ts index e49ba35..b81a6f6 100644 --- a/src/adapters/vcs.ts +++ b/src/adapters/vcs.ts @@ -90,6 +90,16 @@ export async function identifyChange( const hash = createHash("sha256"); hash.update("noxroot-change-identity-v1\0"); hash.update(baselineRevision); + const trackedMetadata = await git( + root, + ["diff", "--raw", "--no-abbrev", "-z", baselineRevision, "--"], + 1_000_000, + ); + if (trackedMetadata.exitCode !== 0 || trackedMetadata.outputTruncated) { + throw new Error("Complete Git change metadata could not be captured safely."); + } + hash.update("\0git-raw\0"); + hash.update(trackedMetadata.stdout); for (const relative of normalizedPaths) { const absolute = resolveWithin(root, relative); @@ -106,12 +116,11 @@ export async function identifyChange( throw error; } - hash.update(`\0mode:${entry.mode.toString(8)}\0size:${entry.size}`); if (entry.isSymbolicLink()) { hash.update("\0symlink\0"); hash.update(await readlink(absolute)); } else if (entry.isFile()) { - hash.update("\0file\0"); + hash.update(`\0file\0size:${entry.size}\0`); await new Promise((resolve, reject) => { const stream = createReadStream(absolute); stream.on("data", (chunk) => hash.update(chunk)); diff --git a/src/knowledge/learn.ts b/src/knowledge/learn.ts index be8aed2..545cb4f 100644 --- a/src/knowledge/learn.ts +++ b/src/knowledge/learn.ts @@ -166,9 +166,9 @@ async function currentApprovedChange(root: string, run: RunRecord): Promise changedPath !== guided.reviewEvidencePath, - ); + const changedPaths = ( + await changedFiles(root, guided.baseline.revision, { strict: true }) + ).filter((changedPath) => changedPath !== guided.reviewEvidencePath); const current = await identifyChange(root, guided.baseline.revision, changedPaths); return current.changeId === guided.changeIdentity.changeId; } diff --git a/src/orchestration/guided.ts b/src/orchestration/guided.ts index bcd9438..e5ecc13 100644 --- a/src/orchestration/guided.ts +++ b/src/orchestration/guided.ts @@ -101,9 +101,9 @@ export async function inspectGuidedContinuation( record: GuidedRunRecord, sensitivePaths: string[] = [], ): Promise { - const changedPaths = (await changedFiles(root, record.baseline.revision)).filter( - (changedPath) => changedPath !== record.reviewEvidencePath, - ); + const changedPaths = ( + await changedFiles(root, record.baseline.revision, { strict: true }) + ).filter((changedPath) => changedPath !== record.reviewEvidencePath); const currentIdentity = await identifyChange(root, record.baseline.revision, changedPaths); const latestChecks = record.verification.at(-1) ?? []; let status: ContinuationVerificationStatus; @@ -249,9 +249,9 @@ export async function finishGuidedRun(input: { const reviewEvidencePath = input.reviewFile ? path.relative(input.root, resolveWithin(input.root, input.reviewFile)).replaceAll("\\", "/") : undefined; - const changedPaths = (await changedFiles(input.root, record.baseline.revision)).filter( - (changedPath) => changedPath !== reviewEvidencePath, - ); + const changedPaths = ( + await changedFiles(input.root, record.baseline.revision, { strict: true }) + ).filter((changedPath) => changedPath !== reviewEvidencePath); for (const changedPath of changedPaths) resolveWithin(input.root, changedPath); const diff = await diffFromRevision( input.root, diff --git a/src/verification/index.ts b/src/verification/index.ts index 4209efb..4d8e7bf 100644 --- a/src/verification/index.ts +++ b/src/verification/index.ts @@ -128,7 +128,11 @@ export async function executeVerification( return results; } -export async function changedFiles(root: string, baseRevision?: string): Promise { +export async function changedFiles( + root: string, + baseRevision?: string, + options: { strict?: boolean } = {}, +): Promise { try { const result = await runProcess({ executable: "git", @@ -138,7 +142,10 @@ export async function changedFiles(root: string, baseRevision?: string): Promise timeoutMs: 10_000, outputLimitBytes: 1_000_000, }); - if (result.exitCode !== 0) return []; + if (result.exitCode !== 0 || result.outputTruncated) { + if (options.strict) throw new Error("Complete changed-path metadata could not be captured."); + return []; + } const parts = result.stdout.split("\0").filter(Boolean); const files: string[] = []; for (let index = 0; index < parts.length; index += 1) { @@ -157,12 +164,15 @@ export async function changedFiles(root: string, baseRevision?: string): Promise timeoutMs: 10_000, outputLimitBytes: 1_000_000, }); - if (committed.exitCode === 0) { + if (committed.exitCode === 0 && !committed.outputTruncated) { files.push(...committed.stdout.split("\0").filter(Boolean)); + } else if (options.strict) { + throw new Error("Complete committed-path metadata could not be captured."); } } return [...new Set(files.map((file) => file.replaceAll("\\", "/")))].sort(); - } catch { + } catch (error) { + if (options.strict) throw error; return []; } } From 48d883e6106396c347cd0a69bab2db64c7e541bc Mon Sep 17 00:00:00 2001 From: liolevx <312117550+liolevx@users.noreply.github.com> Date: Fri, 4 Sep 2026 15:21:36 -0400 Subject: [PATCH 7/8] fix: preserve mixed task constraints --- src/core/intent.ts | 24 +++++++++++++++++++----- tests/task-intent.test.ts | 15 +++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/core/intent.ts b/src/core/intent.ts index 23722ae..9670077 100644 --- a/src/core/intent.ts +++ b/src/core/intent.ts @@ -4,15 +4,29 @@ const EXCLUSION = /\b(?:do not|don't|must not|never|without|except(?:ing)?|exclude|avoid(?:ing)?)\b/i; const ACCEPTANCE = /\b(?:acceptance|must|should|when|so that|ensure|verify)\b/i; const AUTHORITY = /\b(push|merge|deploy|publish|release)\b/gi; +const POSITIVE_ACTION = + /^(?:add|build|change|create|document|ensure|fix|implement|improve|preserve|refactor|remove|run|support|test|update|write)\b/i; + +function splitConjunctions(value: string): string[] { + const parts = value.split(/\s+and\s+/i); + if (parts.length === 1) return parts; + const result: string[] = []; + for (const part of parts) { + const previous = result.at(-1); + const boundary = + EXCLUSION.test(part) || + (previous !== undefined && EXCLUSION.test(previous) && POSITIVE_ACTION.test(part)); + if (boundary || previous === undefined) result.push(part); + else result[result.length - 1] = `${previous} and ${part}`; + } + return result; +} function clauses(task: string): string[] { return task .split(/(?:\r?\n|[.;](?:\s|$))/) - .flatMap((value) => - value.split( - /\s+(?:but|while)\s+|\s+and\s+(?=(?:do not|don't|must not|never|avoid|exclude)\b)/i, - ), - ) + .flatMap((value) => value.split(/\s+(?:but|while)\s+/i)) + .flatMap(splitConjunctions) .flatMap((value) => { const trimmed = value.trim(); const without = /\bwithout\b/i.exec(trimmed); diff --git a/tests/task-intent.test.ts b/tests/task-intent.test.ts index e570b89..fa0c160 100644 --- a/tests/task-intent.test.ts +++ b/tests/task-intent.test.ts @@ -21,4 +21,19 @@ describe("task intent", () => { expect(intent.requiredOutcomes).toEqual(["Fix parsing and add a regression test"]); expect(intent.explicitExclusions).toEqual([]); }); + + it.each([ + ["Do not deploy and fix the parser", "fix the parser", "Do not deploy", ""], + ["Fix parser but do not deploy and add tests", "Fix parser", "do not deploy", "add tests"], + ])("preserves positive work after a negative clause: %s", (task, first, exclusion, second) => { + const intent = parseTaskIntent(task); + expect(intent.requiredOutcomes).toEqual(second ? [first, second] : [first]); + expect(intent.explicitExclusions).toEqual([exclusion]); + }); + + it("keeps a negative object list together", () => { + const intent = parseTaskIntent("Fix the parser but do not change the API and documentation"); + expect(intent.requiredOutcomes).toEqual(["Fix the parser"]); + expect(intent.explicitExclusions).toEqual(["do not change the API and documentation"]); + }); }); From 1c4c6d68c1fe3b622c4350d33e9e552ee8f84fe7 Mon Sep 17 00:00:00 2001 From: liolevx <312117550+liolevx@users.noreply.github.com> Date: Fri, 4 Sep 2026 15:21:48 -0400 Subject: [PATCH 8/8] fix: close evidence race windows --- docs/architecture.md | 5 + docs/commands.md | 14 +- docs/security.md | 4 +- src/adapters/agents.ts | 2 +- src/adapters/vcs.ts | 24 +- src/cli.ts | 32 ++- src/knowledge/learn.ts | 5 +- src/orchestration/guided.ts | 98 ++++++- src/orchestration/run.ts | 222 +++++++++++++-- src/state/local.ts | 5 +- src/verification/index.ts | 5 +- tests/autonomy-guided.test.ts | 159 ++++++++++- tests/orchestration-learning-vcs.test.ts | 336 ++++++++++++++++++++++- tests/state-location.test.ts | 14 +- 14 files changed, 887 insertions(+), 38 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 9eb7e90..8e325ba 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -54,6 +54,11 @@ freshness without retaining file contents; separately bounded and redacted diff display and review. Reviewer decisions must repeat the package's task and change ids. Local state is never treated as application runtime state. +Changed-path and Git metadata capture fails closed when bounded process output cannot represent the +complete change. Unsupported changed directory or submodule surfaces cannot complete. Identity is +checked again after approved commands and after review so a concurrently changing repository cannot +inherit earlier evidence. The same task/change binding applies to guided and delegated reviewers. + Completed and approved local records are pruned by age and count after a run finishes. Running, incomplete, failed, blocked, review-pending, and malformed recovery evidence is never removed by automatic retention. diff --git a/docs/commands.md b/docs/commands.md index ca68bde..cbb7de0 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -167,6 +167,11 @@ documentation/learning assessment without a new model call. When no deterministi signal exists, documentation is reported as `not-assessed`; an empty deterministic learning assessment is reported as `no-candidate`, not as proof that no documentation could help. +External review evidence must be an untracked regular JSON file under `.noxroot/local/`. Linked, +tracked, oversized, mismatched, and malformed evidence is rejected; rejected contents are not kept +in task state. If a check or reviewer changes the repository, prior verification and review evidence +becomes stale and `finish` must be run again. + ## `learn` `learn --task ID` accepts structured reviewer candidates of kind `knowledge`, `decision`, @@ -176,7 +181,8 @@ duplication/conflict results, content, and whether an executable guardrail is be requires confirmation; the first learnings file and index link are written in the same operation. Candidates are eligible only after an approved review, and only while the complete change still matches that approval. Earlier reviewer calls and candidates from failed or superseded attempts are -not reused. Raw prose, task text, sessions, user data, secrets, and external human docs are not -converted into knowledge. New entries carry a confirmation date and source task id. Noxroot refuses -another entry when the destination would exceed `context.documentWarningBytes`; existing knowledge -must then be consolidated or superseded deliberately. +not reused. Freshness is checked again after application confirmation. Raw prose, task text, +sessions, user data, secrets, and external human docs are not converted into knowledge. New entries +carry a confirmation date and source task id. Noxroot refuses another entry when the destination +would exceed `context.documentWarningBytes`; existing knowledge must then be consolidated or +superseded deliberately. diff --git a/docs/security.md b/docs/security.md index 1ab4103..7f61900 100644 --- a/docs/security.md +++ b/docs/security.md @@ -33,7 +33,9 @@ and treats zero matching checks or unavailable executables as blockers. Diff evi path but omits contents for suspected secrets, configured sensitive paths, and symlinks; the same redaction applies to connected-agent reviewer packages. Freshness uses a separate full-change hash; file contents are streamed into it, not retained in task state. Reviewer files are resolved inside -the repository and must satisfy the same strict bound JSON contract as command reviewers. +the dedicated untracked `.noxroot/local/` directory without following links and must satisfy the +same strict bound JSON contract as command reviewers. Invalid reviewer-file contents are discarded +rather than persisted as diagnostics. Negative guarantees are release blockers. A newly discovered path to a preview write, child command, agent call, network attempt, secret disclosure, or path escape requires a regression test before diff --git a/src/adapters/agents.ts b/src/adapters/agents.ts index d74a97b..1fc6682 100644 --- a/src/adapters/agents.ts +++ b/src/adapters/agents.ts @@ -190,7 +190,7 @@ export class CommandAgentAdapter implements AgentAdapter { ? { taskId: candidate.taskId, changeId: candidate.changeId } : undefined; const review = - evidence.exitCode === 0 && !evidence.outputTruncated + evidence.exitCode === 0 && !evidence.outputTruncated && expected ? parseReviewerResponse(evidence.stdout, expected) : undefined; if (review) { diff --git a/src/adapters/vcs.ts b/src/adapters/vcs.ts index b81a6f6..da43de0 100644 --- a/src/adapters/vcs.ts +++ b/src/adapters/vcs.ts @@ -79,6 +79,11 @@ export async function revisionInCurrentHistory(root: string, revision: string): return result.exitCode === 0; } +export async function isTrackedPath(root: string, relativePath: string): Promise { + const result = await git(root, ["ls-files", "--error-unmatch", "--", relativePath]); + return result.exitCode === 0; +} + export async function identifyChange( root: string, baselineRevision: string, @@ -98,6 +103,15 @@ export async function identifyChange( if (trackedMetadata.exitCode !== 0 || trackedMetadata.outputTruncated) { throw new Error("Complete Git change metadata could not be captured safely."); } + const untrackedMetadata = await git( + root, + ["ls-files", "--others", "--exclude-standard", "-z"], + 1_000_000, + ); + if (untrackedMetadata.exitCode !== 0 || untrackedMetadata.outputTruncated) { + throw new Error("Complete untracked change metadata could not be captured safely."); + } + const untrackedPaths = new Set(untrackedMetadata.stdout.split("\0").filter(Boolean)); hash.update("\0git-raw\0"); hash.update(trackedMetadata.stdout); @@ -121,6 +135,9 @@ export async function identifyChange( hash.update(await readlink(absolute)); } else if (entry.isFile()) { hash.update(`\0file\0size:${entry.size}\0`); + if (untrackedPaths.has(relative)) { + hash.update(entry.mode & 0o111 ? "executable\0" : "not-executable\0"); + } await new Promise((resolve, reject) => { const stream = createReadStream(absolute); stream.on("data", (chunk) => hash.update(chunk)); @@ -128,7 +145,9 @@ export async function identifyChange( stream.on("end", resolve); }); } else if (entry.isDirectory()) { - hash.update("\0directory"); + throw new Error( + `Complete change identity does not support changed directory or submodule path: ${relative}`, + ); } else { hash.update("\0other"); } @@ -208,7 +227,8 @@ export async function diffFromRevision( if (remaining <= 0) break; const absolute = resolveWithin(root, relative); const file = await lstat(absolute); - const header = `\ndiff --git a/${relative} b/${relative}\nnew file mode ${file.isSymbolicLink() ? "120000" : "100644"}\n--- /dev/null\n+++ b/${relative}\n`; + const mode = file.isSymbolicLink() ? "120000" : file.mode & 0o111 ? "100755" : "100644"; + const header = `\ndiff --git a/${relative} b/${relative}\nnew file mode ${mode}\n--- /dev/null\n+++ b/${relative}\n`; let body: string; if (isSuspectedSecret(relative) || matchesSensitivePath(relative, sensitivePaths)) { body = `Content omitted for sensitive path ${relative}.\n`; diff --git a/src/cli.ts b/src/cli.ts index 7aa489c..e97b9ef 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -12,6 +12,7 @@ import { configuredAgent, ManualAgentAdapter } from "./adapters/agents.js"; import { boundedDiff, captureRepositoryBaseline, + identifyChange, prepareIsolatedWorktree, revisionInCurrentHistory, } from "./adapters/vcs.js"; @@ -61,6 +62,7 @@ import { executeVerification, planVerification, selectVerification, + unmatchedVerificationPaths, } from "./verification/index.js"; const DESCRIPTION = @@ -1006,6 +1008,19 @@ export function createProgram(customIo?: Partial): Command { }); }, diff: () => boundedDiff(worktree, config?.sensitivePaths ?? []), + changeId: async () => { + const actualChanged = await changedFiles(worktree.path, worktree.baseRevision, { + strict: true, + }); + return (await identifyChange(worktree.path, worktree.baseRevision, actualChanged)) + .changeId; + }, + unmatchedPaths: async () => { + const actualChanged = await changedFiles(worktree.path, worktree.baseRevision, { + strict: true, + }); + return unmatchedVerificationPaths(checks, actualChanged); + }, }, ); const recordPath = await writeRunRecord(root, id, record); @@ -1138,9 +1153,24 @@ export function createProgram(customIo?: Partial): Command { io.stderr("Learning application cancelled; durable knowledge was not changed.\n"); return; } + const refreshed = await proposeLearnings(common.root, record); + const refreshedBySignature = new Map( + refreshed.proposals + .filter( + (proposal) => proposal.duplication === "not-found" && proposal.conflict === "none", + ) + .map((proposal) => [proposal.signature, proposal]), + ); + if (applicable.some((proposal) => !refreshedBySignature.has(proposal.signature))) { + throw new Error( + "Learning stopped because the approved change or its eligible proposals changed; review it again.", + ); + } const applied: string[] = []; for (const proposal of applicable) { - applied.push(...(await applyLearning(common.root, proposal))); + applied.push( + ...(await applyLearning(common.root, refreshedBySignature.get(proposal.signature)!)), + ); } if (common.json) writeJson(io, { ...result, applied }); else io.stdout(`Applied ${applicable.length} proposal(s).\n`); diff --git a/src/knowledge/learn.ts b/src/knowledge/learn.ts index 545cb4f..015357a 100644 --- a/src/knowledge/learn.ts +++ b/src/knowledge/learn.ts @@ -2,7 +2,7 @@ import { createHash } from "node:crypto"; import { lstat, mkdir, readFile, readdir, rename, stat, writeFile } from "node:fs/promises"; import path from "node:path"; import type { ReviewerResponse } from "../adapters/agents.js"; -import { identifyChange, type ChangeIdentity } from "../adapters/vcs.js"; +import { identifyChange, isTrackedPath, type ChangeIdentity } from "../adapters/vcs.js"; import { loadConfig } from "../config/load.js"; import type { RunRecord } from "../orchestration/run.js"; import { resolveWithin } from "../security/paths.js"; @@ -166,6 +166,9 @@ async function currentApprovedChange(root: string, run: RunRecord): Promise changedPath !== guided.reviewEvidencePath); diff --git a/src/orchestration/guided.ts b/src/orchestration/guided.ts index e5ecc13..2426ccd 100644 --- a/src/orchestration/guided.ts +++ b/src/orchestration/guided.ts @@ -1,5 +1,6 @@ import { createHash } from "node:crypto"; -import { readFile } from "node:fs/promises"; +import { constants } from "node:fs"; +import { open, realpath } from "node:fs/promises"; import path from "node:path"; import type { AgentAdapter, AgentResult, ReviewerResponse } from "../adapters/agents.js"; import { parseReviewerResponse } from "../adapters/agents.js"; @@ -7,11 +8,12 @@ import { captureRepositoryBaseline, diffFromRevision, identifyChange, + isTrackedPath, type ChangeIdentity, } from "../adapters/vcs.js"; import type { ContextPackage, VerificationCommand, VerificationResult } from "../model.js"; import { cliCommand } from "../invocation.js"; -import { resolveWithin } from "../security/paths.js"; +import { isWithin, resolveWithin } from "../security/paths.js"; import { changedFiles, executeVerification, @@ -67,6 +69,26 @@ function samePath(left: string, right: string): boolean { return normalize(left) === normalize(right); } +async function readReviewerEvidence(root: string, relativePath: string): Promise { + const target = resolveWithin(root, relativePath); + const canonical = await realpath(target); + if (!isWithin(await realpath(root), canonical) || !samePath(target, canonical)) { + throw new Error("Reviewer evidence must not use a symbolic link or leave the repository."); + } + const handle = await open(target, constants.O_RDONLY | (constants.O_NOFOLLOW ?? 0)); + try { + const metadata = await handle.stat(); + if (!metadata.isFile() || metadata.size > 1_000_000) { + throw new Error( + "Reviewer evidence must be a regular JSON file no larger than 1000000 bytes.", + ); + } + return await handle.readFile("utf8"); + } finally { + await handle.close(); + } +} + function continuationNextAction( record: GuidedRunRecord, changedPaths: string[], @@ -249,17 +271,48 @@ export async function finishGuidedRun(input: { const reviewEvidencePath = input.reviewFile ? path.relative(input.root, resolveWithin(input.root, input.reviewFile)).replaceAll("\\", "/") : undefined; + if ( + reviewEvidencePath && + (!reviewEvidencePath.toLowerCase().startsWith(".noxroot/local/") || + !reviewEvidencePath.toLowerCase().endsWith(".json")) + ) { + throw new Error("Reviewer evidence must be an untracked JSON file under .noxroot/local/."); + } + if (reviewEvidencePath && (await isTrackedPath(input.root, reviewEvidencePath))) { + throw new Error("Reviewer evidence must not be a tracked repository file."); + } const changedPaths = ( await changedFiles(input.root, record.baseline.revision, { strict: true }) ).filter((changedPath) => changedPath !== reviewEvidencePath); for (const changedPath of changedPaths) resolveWithin(input.root, changedPath); + const changeIdentity = await identifyChange(input.root, record.baseline.revision, changedPaths); const diff = await diffFromRevision( input.root, record.baseline.revision, input.sensitivePaths ?? [], reviewEvidencePath ? [reviewEvidencePath] : [], ); - const changeIdentity = await identifyChange(input.root, record.baseline.revision, changedPaths); + const changedAfterDiff = ( + await changedFiles(input.root, record.baseline.revision, { strict: true }) + ).filter((changedPath) => changedPath !== reviewEvidencePath); + const identityAfterDiff = await identifyChange( + input.root, + record.baseline.revision, + changedAfterDiff, + ); + if (identityAfterDiff.changeId !== changeIdentity.changeId) { + const stale: GuidedRunRecord = { + ...record, + status: "incomplete", + changedPaths, + changeIdentity, + verification: [...record.verification, []], + verificationGaps: ["The repository changed while review evidence was captured."], + handoff: "", + }; + stale.handoff = guidedHandoff(stale, []); + return stale; + } const reviewAssessment = assessReviewNeed(changedPaths, diff, record.task); const commands = selectVerification(record.trustedVerificationPolicy, changedPaths); const unmatchedChangedPaths = unmatchedVerificationPaths( @@ -285,6 +338,23 @@ export async function finishGuidedRun(input: { verificationGaps: [], }; + const changedAfterChecks = ( + await changedFiles(input.root, record.baseline.revision, { strict: true }) + ).filter((changedPath) => changedPath !== reviewEvidencePath); + const identityAfterChecks = await identifyChange( + input.root, + record.baseline.revision, + changedAfterChecks, + ); + if (identityAfterChecks.changeId !== changeIdentity.changeId) { + next.status = "incomplete"; + next.verificationGaps = [ + "The repository changed while approved checks ran; their evidence is stale.", + ]; + next.handoff = guidedHandoff(next, checks); + return next; + } + if (changedPaths.length === 0) { next.status = "blocked"; next.verificationGaps = ["No repository change was detected from the recorded baseline."]; @@ -352,7 +422,7 @@ export async function finishGuidedRun(input: { next.reviewerPackage = reviewerPackage; let reviewResult: AgentResult | undefined; if (input.reviewFile) { - const source = await readFile(resolveWithin(input.root, input.reviewFile), "utf8"); + const source = await readReviewerEvidence(input.root, input.reviewFile); const review = parseReviewerResponse(source, { taskId: record.id, changeId: changeIdentity.changeId, @@ -371,7 +441,8 @@ export async function finishGuidedRun(input: { invoked: false, status: "failed", summary: "External reviewer file was not one schema-valid JSON response.", - output: source, + output: "", + diagnostics: "Rejected reviewer evidence was not retained.", exitCode: null, reviewDecision: "blocked", }; @@ -395,6 +466,23 @@ export async function finishGuidedRun(input: { } next.calls = [...record.calls, { role: "reviewer", result: reviewResult }]; const review = reviewResult.review; + const changedAfterReview = ( + await changedFiles(input.root, record.baseline.revision, { strict: true }) + ).filter((changedPath) => changedPath !== reviewEvidencePath); + const identityAfterReview = await identifyChange( + input.root, + record.baseline.revision, + changedAfterReview, + ); + if (identityAfterReview.changeId !== changeIdentity.changeId) { + next.status = "blocked"; + next.verificationGaps = [ + "The repository changed during review; verification and the reviewer decision are stale.", + ]; + next.learningCandidates = []; + next.handoff = guidedHandoff(next, checks); + return next; + } next.reviewDecision = reviewResult.reviewDecision; next.learningCandidates = review?.learningCandidates ?? []; next.status = diff --git a/src/orchestration/run.ts b/src/orchestration/run.ts index a026225..03b4736 100644 --- a/src/orchestration/run.ts +++ b/src/orchestration/run.ts @@ -47,6 +47,26 @@ export interface OrchestrationRequest { export interface OrchestrationDependencies { verify: () => Promise; diff: () => Promise; + changeId: () => Promise; + unmatchedPaths: () => Promise; +} + +async function stableReviewEvidence( + dependencies: OrchestrationDependencies, +): Promise<{ diff: string; changeId: string } | undefined> { + const before = await dependencies.changeId(); + const diff = await dependencies.diff(); + return (await dependencies.changeId()) === before ? { diff, changeId: before } : undefined; +} + +async function stableVerificationCoverage( + dependencies: OrchestrationDependencies, +): Promise<{ changeId: string; unmatchedPaths: string[] } | undefined> { + const before = await dependencies.changeId(); + const unmatchedPaths = await dependencies.unmatchedPaths(); + return (await dependencies.changeId()) === before + ? { changeId: before, unmatchedPaths } + : undefined; } function passed(results: VerificationResult[]): boolean { @@ -153,8 +173,48 @@ export async function orchestrateRun( return { ...partial, handoff: handoff(partial) }; } + let coverage = await stableVerificationCoverage(dependencies); + if (!coverage) { + verificationGaps.push("The repository changed while verification coverage was inspected."); + } else if (coverage.unmatchedPaths.length > 0) { + verificationGaps.push(`No approved check applies to: ${coverage.unmatchedPaths.join(", ")}.`); + } + if (verificationGaps.length > 0) { + const partial: Omit = { + id: request.id, + task: request.task, + status: "incomplete", + ...(request.branch === undefined ? {} : { branch: request.branch }), + worktree: request.cwd, + calls, + verification, + verificationGaps, + }; + return { ...partial, handoff: handoff(partial) }; + } + let verificationChangeId = coverage!.changeId; let checkResults = await dependencies.verify(); + let verificationStable = + verificationChangeId !== undefined && (await dependencies.changeId()) === verificationChangeId; verification.push(checkResults); + if (!verificationStable) { + verificationGaps.push( + verificationChangeId + ? "The repository changed while approved checks ran; their evidence is stale." + : "A complete change id was unavailable for verification.", + ); + const partial: Omit = { + id: request.id, + task: request.task, + status: "incomplete", + ...(request.branch === undefined ? {} : { branch: request.branch }), + worktree: request.cwd, + calls, + verification, + verificationGaps, + }; + return { ...partial, handoff: handoff(partial) }; + } if (checkResults.length === 0) { verificationGaps.push("No approved deterministic checks matched the change."); const partial: Omit = { @@ -205,8 +265,44 @@ export async function orchestrateRun( }); calls.push({ role: "repair", result: repair }); if (repair.status !== "completed") break; + coverage = await stableVerificationCoverage(dependencies); + if (!coverage || coverage.unmatchedPaths.length > 0) { + verificationStable = false; + verificationGaps.push( + coverage + ? `No approved check applies to: ${coverage.unmatchedPaths.join(", ")}.` + : "The repository changed while verification coverage was inspected.", + ); + break; + } + verificationChangeId = coverage.changeId; checkResults = await dependencies.verify(); + verificationStable = + verificationChangeId !== undefined && + (await dependencies.changeId()) === verificationChangeId; verification.push(checkResults); + if (!verificationStable) { + verificationGaps.push( + verificationChangeId + ? "The repository changed while approved checks ran; their evidence is stale." + : "A complete change id was unavailable for verification.", + ); + break; + } + } + + if (!verificationStable) { + const partial: Omit = { + id: request.id, + task: request.task, + status: "incomplete", + ...(request.branch === undefined ? {} : { branch: request.branch }), + worktree: request.cwd, + calls, + verification, + verificationGaps, + }; + return { ...partial, handoff: handoff(partial) }; } if (!passed(checkResults)) { @@ -223,7 +319,24 @@ export async function orchestrateRun( return { ...partial, handoff: handoff(partial) }; } - const reviewDiff = await dependencies.diff(); + const initialReviewEvidence = await stableReviewEvidence(dependencies); + if (!initialReviewEvidence || initialReviewEvidence.changeId !== verificationChangeId) { + const partial: Omit = { + id: request.id, + task: request.task, + status: "incomplete", + ...(request.branch === undefined ? {} : { branch: request.branch }), + worktree: request.cwd, + calls, + verification, + verificationGaps: [ + ...verificationGaps, + "The repository changed while review evidence was captured.", + ], + }; + return { ...partial, handoff: handoff(partial) }; + } + const reviewDiff = initialReviewEvidence.diff; const reviewAssessment = assessReviewNeed([], reviewDiff, request.task); if (!reviewAssessment.required) { const partial: Omit = { @@ -270,9 +383,13 @@ export async function orchestrateRun( return { ...partial, handoff: handoff(partial) }; } + let reviewChangeId: string | undefined = initialReviewEvidence.changeId; let review = await request.adapter.invoke({ role: "reviewer", package: { + schemaVersion: 2, + taskId: request.id, + changeId: reviewChangeId, original: request.context, diff: reviewDiff, reviewAssessment, @@ -304,29 +421,102 @@ export async function orchestrateRun( }); calls.push({ role: "repair", result: repair }); if (repair.status === "completed") { - checkResults = await dependencies.verify(); - verification.push(checkResults); + coverage = await stableVerificationCoverage(dependencies); + if (!coverage || coverage.unmatchedPaths.length > 0) { + verificationStable = false; + verificationGaps.push( + coverage + ? `No approved check applies to: ${coverage.unmatchedPaths.join(", ")}.` + : "The repository changed while verification coverage was inspected.", + ); + } else { + verificationChangeId = coverage.changeId; + checkResults = await dependencies.verify(); + verificationStable = (await dependencies.changeId()) === verificationChangeId; + verification.push(checkResults); + } if ( + verificationStable && passed(checkResults) && calls.filter((call) => call.role === "reviewer").length < request.budgets.reviewerCalls ) { - review = await request.adapter.invoke({ - role: "reviewer", - package: { - original: request.context, - diff: await dependencies.diff(), - verification: checkResults, - priorFindings: review.output, - }, - cwd: request.cwd, - repositoryRoot: request.repositoryRoot, - ...(request.signal === undefined ? {} : { signal: request.signal }), - }); - calls.push({ role: "reviewer", result: review }); + const repairedReviewEvidence = await stableReviewEvidence(dependencies); + reviewChangeId = + repairedReviewEvidence?.changeId === verificationChangeId + ? repairedReviewEvidence.changeId + : undefined; + if (repairedReviewEvidence?.changeId === verificationChangeId) { + review = await request.adapter.invoke({ + role: "reviewer", + package: { + schemaVersion: 2, + taskId: request.id, + changeId: reviewChangeId, + original: request.context, + diff: repairedReviewEvidence.diff, + verification: checkResults, + priorFindings: review.output, + }, + cwd: request.cwd, + repositoryRoot: request.repositoryRoot, + ...(request.signal === undefined ? {} : { signal: request.signal }), + }); + calls.push({ role: "reviewer", result: review }); + } } } } + if (!verificationStable) { + const partial: Omit = { + id: request.id, + task: request.task, + status: "incomplete", + ...(request.branch === undefined ? {} : { branch: request.branch }), + worktree: request.cwd, + calls, + verification, + verificationGaps: [ + ...verificationGaps, + "The repository changed while approved checks ran; their evidence is stale.", + ], + }; + return { ...partial, handoff: handoff(partial) }; + } + + if (!reviewChangeId) { + const partial: Omit = { + id: request.id, + task: request.task, + status: "incomplete", + ...(request.branch === undefined ? {} : { branch: request.branch }), + worktree: request.cwd, + calls, + verification, + verificationGaps: [ + ...verificationGaps, + "The repository changed while review evidence was captured.", + ], + }; + return { ...partial, handoff: handoff(partial) }; + } + if ((await dependencies.changeId()) !== reviewChangeId) { + const partial: Omit = { + id: request.id, + task: request.task, + status: "blocked", + ...(request.branch === undefined ? {} : { branch: request.branch }), + worktree: request.cwd, + calls, + verification, + verificationGaps: [ + ...verificationGaps, + "The repository changed during review; verification and the reviewer decision are stale.", + ], + }; + return { ...partial, handoff: handoff(partial) }; + } + const status: RunRecord["status"] = review.status !== "completed" ? "failed" diff --git a/src/state/local.ts b/src/state/local.ts index 346e06d..2370442 100644 --- a/src/state/local.ts +++ b/src/state/local.ts @@ -46,7 +46,10 @@ export async function localStateRoot(root: string): Promise { if (!(await pathType(path.join(root, ".git")))) return legacy; const local = await setupDestination(root, ".noxroot/local"); if (await pathType(legacy)) { - if (await pathType(local)) { + // Review evidence is intentionally placed under .noxroot/local, including in + // repositories whose older task records still live under .git/noxroot. Only + // a runs entry makes the local directory a competing task-state store. + if (await pathType(path.join(local, "runs"))) { throw new TaskStateError( "Two task-state directories exist. Stop and reconcile the existing records before retrying; neither directory was changed.", ); diff --git a/src/verification/index.ts b/src/verification/index.ts index 4d8e7bf..e7c574a 100644 --- a/src/verification/index.ts +++ b/src/verification/index.ts @@ -153,7 +153,10 @@ export async function changedFiles( if (!entry || entry.length < 4) continue; const status = entry.slice(0, 2); files.push(entry.slice(3).replaceAll("\\", "/")); - if ((status.includes("R") || status.includes("C")) && parts[index + 1]) index += 1; + if ((status.includes("R") || status.includes("C")) && parts[index + 1]) { + files.push(parts[index + 1]!.replaceAll("\\", "/")); + index += 1; + } } if (baseRevision) { const committed = await runProcess({ diff --git a/tests/autonomy-guided.test.ts b/tests/autonomy-guided.test.ts index e57f9d6..dbf5531 100644 --- a/tests/autonomy-guided.test.ts +++ b/tests/autonomy-guided.test.ts @@ -4,7 +4,11 @@ import { CommanderError } from "commander"; import { afterEach, describe, expect, it } from "vitest"; import { effectiveAutonomy } from "../src/orchestration/autonomy.js"; import { finishGuidedRun, startGuidedRun } from "../src/orchestration/guided.js"; -import { ManualAgentAdapter } from "../src/adapters/agents.js"; +import { + ManualAgentAdapter, + type AgentAdapter, + type AgentRequest, +} from "../src/adapters/agents.js"; import { runProcess } from "../src/adapters/process.js"; import { temporaryDirectory } from "./helpers.js"; import { createProgram } from "../src/cli.js"; @@ -565,7 +569,17 @@ agents: {default: manual, adapters: {manual: {type: manual}}} expect(pending.reviewerPackage).toBeUndefined(); expect(pending.verification.at(-1)?.[0]?.status).toBe("passed"); - const reviewPath = "review.json"; + const reviewPath = ".noxroot/local/review.json"; + await mkdir(path.join(root, ".noxroot", "local"), { recursive: true }); + await expect( + finishGuidedRun({ + root, + record: pending, + adapter: new ManualAgentAdapter(), + reviewAuthorized: false, + reviewFile: "src/value.ts", + }), + ).rejects.toThrow("untracked JSON file under .noxroot/local"); await writeFile( path.join(root, reviewPath), JSON.stringify({ @@ -586,6 +600,8 @@ agents: {default: manual, adapters: {manual: {type: manual}}} reviewFile: reviewPath, }); expect(mismatched.status).toBe("blocked"); + expect(mismatched.calls.at(-1)?.result.output).toBe(""); + expect(mismatched.calls.at(-1)?.result.diagnostics).toContain("not retained"); await writeFile( path.join(root, reviewPath), @@ -648,6 +664,108 @@ agents: {default: manual, adapters: {manual: {type: manual}}} expect(JSON.stringify(finished.reviewerPackage)).toContain("