diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 82a4b98..bb4c9f7 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -11,7 +11,7 @@ { "name": "paacm", "description": "PAACM: the Verified Single-Writer Loop for agentic coding. One writer agent, read-only explorer and reviewer subagents, tests protected after approval, and completion gated on executed checks.", - "version": "3.5.0", + "version": "3.5.1", "source": "./plugins/paacm" } ] diff --git a/.cursor-plugin/marketplace.json b/.cursor-plugin/marketplace.json index 7c6c775..faa1ef3 100644 --- a/.cursor-plugin/marketplace.json +++ b/.cursor-plugin/marketplace.json @@ -11,7 +11,7 @@ { "name": "paacm", "description": "PAACM: the Verified Single-Writer Loop for agentic coding. One writer agent, read-only explorer and reviewer subagents, tests protected after approval, and completion gated on executed checks.", - "version": "3.5.0", + "version": "3.5.1", "source": "plugins/paacm" } ] diff --git a/CHANGELOG.md b/CHANGELOG.md index a2affe7..211d4cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,23 @@ # Changelog +## 3.5.1 — Fixes from a team's upgrade + +Reviewing a team project's upgrade to 3.5.0 found these problems in the runtime. Each fix is covered by a test in `test/review-fixes.test.mjs` that failed before it. + +- **An error no longer lets the agent stop unverified.** An invalid `.agent/paacm.json` crashed the stop hook, which is not configured to fail closed, so the turn ended without the gate; 3.4.1 blocked. It is now reported like the configuration errors the gate itself finds. An error while judging one project (the session's own, another worktree, a checkout below a non-git session folder) no longer discards the other projects' results: it blocks that stop once and names the error. On the continuation the stop is let through, as a hook error the IDE shows. +- **A gate log that cannot be written no longer loses the result.** A full disk, or a directory named `gate.log`, made `paacm gate` crash after it had recorded the result. It now warns and keeps the result and the `paacm gate: exit` line. An untracked file that cannot be inspected, such as a symbolic link loop, no longer breaks the gate either. +- **A missing script is not a missing tool.** Exit 127 for a command named by a path inside the project (`./scripts/build.sh`), which a change may have removed or renamed, is an ordinary failure again; only the failing command's own message counts. The verify instructions say so. A program that is not installed, also one named by a path outside the project, is still a missing tool. +- **The protect hook fails closed when git cannot run.** With git not on the hook's `PATH`, or refusing the folder (`safe.directory`), an edit in a sibling worktree was taken to be outside every project and allowed. It is now refused, with git's own message and the folder it failed in. +- **No second gate run and no repeated nudge for an unchanged working tree.** A stop that answers the finish nudge reuses a passing gate for the same working tree however old it is (a failing one is run again); before, a tree with uncommitted changes ran the whole gate again once 15 seconds had passed. A nudge is given once for a working tree and phase, not once every 15 seconds, so an agent waiting for the human is not nudged at every turn. The working-tree fingerprint now includes the size and modification time of untracked files. +- **The review input keeps secrets out.** Untracked files named like secrets (`.env`, `.env.*`, `*.pem`, `*.key`, `*.p12`, `*.pfx`, SSH private keys) are listed by name, anywhere in the repository; their content no longer goes to the reviewers. +- **A test added after a blocked task's approval does not hold other worktrees.** Only files that existed when the tests were approved count. A change to them that arrived from the base branch still holds every worktree that has it, as in 3.5.0, because git cannot tell a reviewed change from one the writer made a remote-tracking ref for. The troubleshooting guide says how to settle it. +- **A task state that cannot be read is reported.** When `task start` or `approve-tests` checks for blocked tasks, an unreadable `state.json` or tasks folder is skipped with a warning instead of silently. +- **The shell guard checks long option lists in linear time.** A git command with many `-c` options made the hook-skipping pattern backtrack exponentially; about 40 of them exceeded the hook's timeout, and the command was refused. + +Not changed: when the session folder is not a git repository, the stop gate still runs the checks, and the post-edit hook the `postEdit` commands, of each checkout below it that the session edited. Making that opt-in is a change to the method, for a later release. The upgrade does not remove the session baselines that 3.4.x left in the temporary folder (`paacm-sessions` in the system's temporary directory); delete them when no 3.4.x session runs on the machine. + +**Upgrading.** `paacm upgrade` to 3.5.1; nothing to configure. + ## 3.5.0 — Finishing, worktrees and shared machines Running 3.4.1 on a team project (several people on one server, linked worktrees, headless agent runs) showed where the runtime enforced less than the method asks. Each fix is covered by tests that failed before it ([ADR 0021](docs/method/adr/0021-finishing-worktrees-and-shared-machines.md)). diff --git a/README.md b/README.md index b1f3b91..9a1a802 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,7 @@ The [user guide](docs/guide/README.md) covers setup, writing requests, supportin ## Status and limits -PAACM 3.5.0 is a complete implementation of the specification for Claude Code and Cursor, including project extensions for adapting it to a repository's own conventions ([guide](docs/guide/extending.md)). Markdown instructions are guidance; the enforcement comes from the hooks, the IDE's permissions and its sandbox. Use the power layer or a plugin when enforcement matters. The method's evidence comes from published studies and vendor guidance, not from your code: [evaluate it](docs/guide/evaluation.md) on tasks from your own history before relying on a claim that it helps. +PAACM 3.5.1 is a complete implementation of the specification for Claude Code and Cursor, including project extensions for adapting it to a repository's own conventions ([guide](docs/guide/extending.md)). Markdown instructions are guidance; the enforcement comes from the hooks, the IDE's permissions and its sandbox. Use the power layer or a plugin when enforcement matters. The method's evidence comes from published studies and vendor guidance, not from your code: [evaluate it](docs/guide/evaluation.md) on tasks from your own history before relying on a claim that it helps. Upgrading from PAACM 2.0.0: see [upgrading-from-2.md](docs/guide/upgrading-from-2.md). diff --git a/docs/guide/setup.md b/docs/guide/setup.md index 9c1645b..2fd25f2 100644 --- a/docs/guide/setup.md +++ b/docs/guide/setup.md @@ -150,7 +150,7 @@ Run the same manifest in your CI, so that every pull request passes the same com With the plugin, the repository has no copy of the runtime; pin the kit's version instead: ```yaml -- run: npx github:Hyper-AI-Lab/hyper-agents#v3.5.0 check --tier full +- run: npx github:Hyper-AI-Lab/hyper-agents#v3.5.1 check --tier full ``` Add `--quiet` to show only the check lines and the end of a failing check's output. Held-out acceptance tests for high-stakes work ([phase 7](../method/ALGORITHM.md#phase-7-verify)) also run in CI, outside the agent's workspace. diff --git a/docs/guide/troubleshooting.md b/docs/guide/troubleshooting.md index 4979637..402eb7f 100644 --- a/docs/guide/troubleshooting.md +++ b/docs/guide/troubleshooting.md @@ -53,12 +53,18 @@ It closes only a finished task, and the message says what is missing: ### "The gate is green, but task … is still …" -The stop hook nudges once when the agent tries to stop with an open task whose gate passes, and once when a STANDARD or DEEP task changed files without recording its phases. If the agent is waiting for your answer, it says so and stops again; the second stop is always let through. Claude Code shows a nudge as stop hook feedback, not as a hook error; only a failing gate is reported as a blocking error. +The stop hook nudges once when the agent tries to stop with an open task whose gate passes, and once when a STANDARD or DEEP task changed files without recording its phases. If the agent is waiting for your answer, it says so and stops again; the second stop is always let through, and the nudge is not repeated while the working tree and the phase stay the same, however long you take to answer. Claude Code shows a nudge as stop hook feedback, not as a hook error; only a failing gate is reported as a blocking error. ### A check fails with exit code 127 The check could not find a command (on Windows, cmd says the command "is not recognized as an internal or external command"): the tool is not installed where the agent runs, or not on the `PATH` of the IDE. The gate prints a hint, and the agent stops with `BLOCKED: trigger 2` instead of changing code. Install the tool and say "continue", or take the check out of the tier the task needs. `doctor` lists the check programs it cannot find. +A command named by a path, such as `./scripts/build.sh`, is a file of the project, not a tool: when the shell cannot find it, a change probably removed or renamed it, and the gate reports an ordinary failure for the agent to fix. + +### "task … (in the worktree at …) is blocked, and these files frozen by its approved tests changed since" + +Another worktree of the same repository has a blocked task with approved tests, and one of the files those tests froze (a test, `.agent/checks.conf` or `.agent/paacm.json`) changed in a commit this worktree contains, for example through a merge of the base branch. PAACM does not let a new task start or approve tests over it, because that would let a frozen-file change through without you. The task's owner resumes it in that worktree (`paacm task resume `), settles the change with you (`reopen-tests`, then `approve-tests`), and finishes or blocks it again. A test file added after the approval does not count. Do not leave blocked tasks behind on a shared machine. + ### "PAACM could not record the session baseline" The session start could not write to `paacm-sessions/` in the worktree's git directory, for example because the clone belongs to another account. The session works normally; only the stop hook's list of files changed without a task falls back to every uncommitted file. Give each person their own clone ([shared machines](setup.md#shared-machines)). diff --git a/docs/method/ALGORITHM.md b/docs/method/ALGORITHM.md index 02b0664..39c095f 100644 --- a/docs/method/ALGORITHM.md +++ b/docs/method/ALGORITHM.md @@ -217,7 +217,7 @@ Each phase lists its entry condition, the steps, the artifact it produces and it - Optionally, run best-of-N: two to four worktree-isolated attempts, filtered by the regression and acceptance tests, then selected by the reviewer. The selector MUST be independent of the tests the writer saw. 5. When the `claude-security` plugin is installed, high-stakes work also runs its scan of the changes at high effort; confirmed findings are fixed within the plan or end in `BLOCKED:`. 6. A project MAY configure a mutation check in the `full` tier. When it fails, the writer MUST NOT edit the frozen tests; it reports the surviving mutants and stops with `BLOCKED:` (trigger 4) for the human to decide on more tests. - 7. The gate saves its full output in the task folder (`gate.log`), so the writer can keep its console output short (`paacm gate --quiet`) and the reviewer can read the evidence. A check that cannot find a command (exit 127) is reported as a missing tool: an environment problem for the human (trigger 2), not a reason to change code. + 7. The gate saves its full output in the task folder (`gate.log`), so the writer can keep its console output short (`paacm gate --quiet`) and the reviewer can read the evidence. A check that cannot find a program (exit 127) is reported as a missing tool: an environment problem for the human (trigger 2), not a reason to change code. A command named by a path in the project is an ordinary failure, since the change may have removed or renamed it. 8. The gate's configuration warnings say whether a change to a protected file arrived through a merge of commits a remote-tracking branch has (such as the base branch). That is context, not an exemption: the report accounts for every warning, saying who approved the change or, for a merged one, where it was reviewed. - **Exit:** the gate is green at the route's tier and the diff audit is clean. - **Rule:** after **three consecutive gate failures on the same check** (the retry cap), the writer MUST stop and escalate (trigger 5) instead of trying again. diff --git a/docs/project/requirements.md b/docs/project/requirements.md index 5239b0f..974ba5d 100644 --- a/docs/project/requirements.md +++ b/docs/project/requirements.md @@ -188,3 +188,9 @@ Evidence (`npm test`, 160 tests): - A fresh-context review of the release found twelve problems, eleven of them reproduced, and four plausible ones; each fix has a test that failed before it: the stop hook keeping the gate result it ran itself; moving into a worktree or sub-folder never releasing the session's failing gate; checkouts below a non-git session folder protected; a locally merged branch staying the task's own in the audit; no new task over a blocked task's changed frozen files; a review diff that survives untracked nested repositories, keeps skip-worktree files out, does not rehash every file, splits type changes and submodules correctly and cannot overflow on backtick runs; FAST next steps through phase 7; missing tools ending in `BLOCKED:` trigger 2; more ways to skip git hooks; doctor's program reader; every lefthook configuration file; a gate log bounded in memory. - An independent audit of the branch found seven more problems, all reproduced, and one plausible; each fix has a test that failed before it: a new worktree starting a task and approving tests over a blocked task's changed frozen files; a remote-tracking ref the writer made turning its own configuration change into one "reviewed there" (a merged change is now only marked `through a merge`, and `git update-ref` asks); `git commit -n` before `&&`, `;` or a redirect, a quoted `core.hooksPath` and `HUSKY='0'` passing the shell guard; with the agent's current directory in one checkout, a sibling checkout below a non-git session folder left unprotected and its failing gate not holding the stop; a nested `.agent/paacm.json` setting up a second set of protect rules; a symbolic link in the task folder hiding the evidence list; `docker build --push` allowed. The plausible one, private file modes on Windows, is documented: the modes apply on Linux and macOS. - Two Linux accounts sharing one temporary folder (run as two unprivileged uids): with 3.4.1 the second account's session start exits 1 with `EACCES` and emits no context; with 3.5.0 both get their context and each baseline folder is private to its owner. + +## Release 3.5.1 + +Release 3.5.1 fixes what a review of a team project's upgrade to 3.5.0 found in the runtime (R3, R4, R12): an error in the stop hook (an invalid configuration, one project among several) no longer lets the agent stop unverified, a gate log that cannot be written keeps the result, a missing script is an ordinary failure while a missing program stays a missing tool, the protect hook fails closed when git cannot run, an unchanged working tree is neither gated twice nor nudged at every turn, untracked files named like secrets stay out of the review input, a test added after a blocked task's approval does not hold other worktrees, an unreadable task state is reported, and the shell guard's hook-skipping pattern runs in linear time. + +Evidence (`npm test`, 179 tests): `test/review-fixes.test.mjs`, nineteen tests. Twelve failed on 3.5.0; one (a program that is not installed stays a missing tool) guards the old behaviour that the missing-script fix keeps. A fresh-context review of the first fixes found five more problems, four of them reproduced, and the last five tests failed before their fixes: an error on a continuation released the stop without a trace (now a hook error), a symbolic link loop broke the fingerprint, a path outside the project or an unrelated earlier message turned a missing program into an ordinary failure, secret-named files outside a sub-folder project went into the review input, and the git error behind the protect hook's refusal was dropped. A second review found that a continuation also reused a failing gate however old it was; the last test failed before that fix. diff --git a/package.json b/package.json index 7c5da60..315bc87 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "paacm", - "version": "3.5.0", + "version": "3.5.1", "description": "PAACM: the Verified Single-Writer Loop for agentic coding in Claude Code and Cursor. Installer, check runner, hooks and evaluation harness.", "type": "module", "license": "Apache-2.0", diff --git a/plugins/paacm/.claude-plugin/plugin.json b/plugins/paacm/.claude-plugin/plugin.json index fb241e9..b3fb971 100644 --- a/plugins/paacm/.claude-plugin/plugin.json +++ b/plugins/paacm/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "paacm", - "version": "3.5.0", + "version": "3.5.1", "description": "PAACM: the Verified Single-Writer Loop for agentic coding. One writer agent, read-only explorer and reviewer subagents, tests protected after approval, and completion gated on executed checks.", "author": { "name": "Hyper AI Lab", diff --git a/plugins/paacm/.cursor-plugin/plugin.json b/plugins/paacm/.cursor-plugin/plugin.json index 2f64129..66c7ba7 100644 --- a/plugins/paacm/.cursor-plugin/plugin.json +++ b/plugins/paacm/.cursor-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "paacm", - "version": "3.5.0", + "version": "3.5.1", "description": "PAACM: the Verified Single-Writer Loop for agentic coding. One writer agent, read-only explorer and reviewer subagents, tests protected after approval, and completion gated on executed checks.", "author": { "name": "Hyper AI Lab", diff --git a/plugins/paacm/skills/paacm-verify/SKILL.md b/plugins/paacm/skills/paacm-verify/SKILL.md index 60cd0b4..65b7d0a 100644 --- a/plugins/paacm/skills/paacm-verify/SKILL.md +++ b/plugins/paacm/skills/paacm-verify/SKILL.md @@ -14,7 +14,7 @@ If `.agent/extensions/07-verify.md` exists, read it first and follow it together 1. **Run the gate:** `paacm gate --quiet`, on its own (no pipe, no `;`). It audits protected paths and runs the check tier for the current task's route; `--quiet` prints the check lines and a failing check's last lines, and with a task the full output is saved as `gate.log` in the task folder. Without a current task it runs the `quick` tier. Add `--tier full` before delivering STANDARD work, and `--tier security` for security-relevant changes. 2. **Read the exit code:** - **0:** passed. - - **1:** a check failed. Fix the cause and run the gate again. If the check exited 127 (on Windows, cmd's "is not recognized" message), a command was not found: that is the environment, not the code. Record the check as not run and stop with `BLOCKED:` trigger 2 to ask the human for the tool. + - **1:** a check failed. Fix the cause and run the gate again. If the check exited 127 (on Windows, cmd's "is not recognized" message), a command was not found. When it is a program that is not installed, that is the environment, not the code: record the check as not run and stop with `BLOCKED:` trigger 2 to ask the human for the tool. When it is a file of the project (a path such as `./scripts/build.sh`) that the change removed or renamed, fix it like any failure. - **3:** a tier has no checks. Record it as not run and tell the human it needs configuring. - **4:** a protected path changed. Undo the change, or escalate if the change is needed. 3. **After three consecutive failures of the same check, stop and escalate** (trigger 5) instead of trying again. diff --git a/plugins/paacm/skills/paacm/references/07-verify.md b/plugins/paacm/skills/paacm/references/07-verify.md index 1276cdd..5e041f7 100644 --- a/plugins/paacm/skills/paacm/references/07-verify.md +++ b/plugins/paacm/skills/paacm/references/07-verify.md @@ -20,7 +20,9 @@ When the gate passes, it prints the next phase: review (phase 8) for STANDARD and DEEP, deliver (phase 9) for FAST, after `paacm task phase verifying` if you have not recorded it. A passing gate at a lower tier than the route's does not count for `paacm task done`. - **A missing tool.** A check that fails with exit 127 (on Windows, cmd's "is not recognized" message) could not find a command, and the gate says so. That is the environment, not the code: do not change code to get around it. The gate cannot pass without the tool, so list the check under CHECKS NOT RUN and stop with `BLOCKED:` trigger 2 (`paacm task block --trigger 2 --reason ""`) to ask the human to install it. + **A missing tool.** A check that fails with exit 127 (on Windows, cmd's "is not recognized" message) could not find a program, and the gate says so. That is the environment, not the code: do not change code to get around it. The gate cannot pass without the tool, so list the check under CHECKS NOT RUN and stop with `BLOCKED:` trigger 2 (`paacm task block --trigger 2 --reason ""`) to ask the human to install it. + + A command named by a path in the project (such as `./scripts/build.sh`) is not a tool: when the shell cannot find it, the change probably removed or renamed it, and the gate reports an ordinary failure. Fix it like any other. 3. **STANDARD work:** also run `paacm gate --tier full` before delivery. If the `full` tier is not configured, record that under CHECKS NOT RUN. 4. **Security-relevant diffs** (input handling, authentication, permissions, secrets, cryptography, dependency changes): also run `paacm gate --tier security`. Confirm that every new dependency exists in its registry under the exact name you used. diff --git a/plugins/paacm/skills/paacm/scripts/hooks/stop-gate.mjs b/plugins/paacm/skills/paacm/scripts/hooks/stop-gate.mjs index b48129a..8c126c0 100644 --- a/plugins/paacm/skills/paacm/scripts/hooks/stop-gate.mjs +++ b/plugins/paacm/skills/paacm/scripts/hooks/stop-gate.mjs @@ -23,7 +23,6 @@ import { changedThisSession, editedCheckouts, sessionIdFrom } from '../lib/sessi import { currentTask, writeState } from '../lib/task.mjs' const GATED = new Set(['implementing', 'verifying', 'reviewing']) -const NUDGE_DEDUP_MS = 15_000 const isTaskFile = (p) => p === '.agent/tasks' || p.startsWith('.agent/tasks/') const ALLOW = { kind: 'allow' } const block = (text) => ({ kind: 'block', text }) @@ -59,6 +58,15 @@ runHook(async () => { const lastMessage = typeof input.last_assistant_message === 'string' ? input.last_assistant_message : '' if (/^\s*BLOCKED:/m.test(lastMessage)) return respond(ALLOW) + // A gate that cannot run (a tier without checks, an invalid configuration): + // the agent cannot fix this by changing code, so it is told once. + const unverifiable = (log) => + continuation + ? ALLOW + : guide( + `PAACM gate could not verify this task:\n${log}\n\nYou cannot fix this by changing code. List the missing checks under CHECKS NOT RUN in the report, and ask the human to configure .agent/checks.conf (or fix .agent/paacm.json). Use \`paacm task block --trigger 2 --reason ""\` if the task cannot be verified without it.`, + ) + // The decision for one project. `own` is the project the agent works in; only // there are uncommitted files without a task worth a nudge. async function decide(projectDir, own) { @@ -77,17 +85,19 @@ runHook(async () => { } if (task.status === 'blocked' || task.status === 'done') return ALLOW - // A nudge is given once: never on a continuation, and not again when the same - // stop arrives twice (Cursor also runs the Claude Code hooks it imports). The - // state is read again before writing: a gate this hook just ran has recorded - // its result there, which the task read above does not have. + // A nudge is given once: never on a continuation, and not again for the same + // working tree and phase, however much later the next stop comes (an agent + // waiting for the human stops at every turn; Cursor also runs the Claude Code + // hooks it imports). The state is read again before writing: a gate this hook + // just ran has recorded its result there, which the task read above does not + // have. const nudge = async (kind, message) => { if (continuation) return ALLOW const fingerprint = worktreeFingerprint(projectDir) const current = (await currentTask(projectDir)) ?? task const last = current.lastNudge - if (last && last.kind === kind && last.fingerprint === fingerprint && Date.now() - Date.parse(last.at) < NUDGE_DEDUP_MS) return ALLOW - current.lastNudge = { kind, fingerprint, at: new Date().toISOString() } + if (last && last.kind === kind && last.fingerprint === fingerprint && last.status === current.status) return ALLOW + current.lastNudge = { kind, fingerprint, status: current.status, at: new Date().toISOString() } await writeState(projectDir, current) return guide(message) } @@ -111,13 +121,24 @@ runHook(async () => { const finish = () => nudge('finish', `PAACM: the gate is green, but task ${task.id} is still ${task.status}. If the work is complete, finish it: ${finishSteps(projectDir, task)}. ${WAITING}`) - const config = await loadConfig(projectDir) + let config + try { + config = await loadConfig(projectDir) + } catch (error) { + return unverifiable(`CONFIG ERROR: ${error.message}`) + } const required = tierForRoute(config, task) const g = task.lastGate const dirty = uncommittedPaths(projectDir).some((p) => !isTaskFile(p)) if (g && g.passed && g.head === head(projectDir) && !dirty && !g.dirty && tierAtLeast(g.tier, required)) return finish() - const duplicate = recentGate(projectDir, task) + // A result from a moment ago is the same stop delivered twice. On a + // continuation the agent only answered the previous stop, so a passing result + // for the same working tree also counts however old it is; a failing one is + // run again, since the agent may have fixed what the fingerprint cannot see + // (an install, a service). + const lasting = continuation ? recentGate(projectDir, task, Infinity) : null + const duplicate = recentGate(projectDir, task) ?? (lasting && lasting.passed ? lasting : null) if (duplicate && tierAtLeast(duplicate.tier, required)) { if (duplicate.passed) return finish() if (duplicate.missingTool) return block(missingToolMessage(duplicate.failed, ' See the previous gate output.')) @@ -129,16 +150,11 @@ runHook(async () => { if (result.code === GATE.PASSED) return finish() const log = lines.slice(-12).join('\n') - if (result.code === GATE.UNCONFIGURED || result.code === GATE.CONFIG) { - if (continuation) return ALLOW - return guide( - `PAACM gate could not verify this task:\n${log}\n\nYou cannot fix this by changing code. List the missing checks under CHECKS NOT RUN in the report, and ask the human to configure .agent/checks.conf (or fix .agent/paacm.json). Use \`paacm task block --trigger 2 --reason ""\` if the task cannot be verified without it.`, - ) - } + if (result.code === GATE.UNCONFIGURED || result.code === GATE.CONFIG) return unverifiable(log) const last = result.results?.length ? result.results[result.results.length - 1] : null const tail = last ? (last.outputTail || '').trim().split('\n').slice(-30).join('\n') : '' - if (last && isMissingTool(last)) return block(missingToolMessage(last.label, ` The check exited ${last.exitCode}.\n\n${log}${tail ? `\n\nLast output of the check:\n${tail}` : ''}`)) + if (last && isMissingTool(last, projectDir)) return block(missingToolMessage(last.label, ` The check exited ${last.exitCode}.\n\n${log}${tail ? `\n\nLast output of the check:\n${tail}` : ''}`)) if (result.gateFailures >= result.retryCap) { return block( `PAACM gate failed ${result.gateFailures} times in a row; the retry cap (${result.retryCap}) is reached. Do not try another fix. Run \`paacm task block --trigger 5 --reason ""\`, write REPORT.md, and end your turn with a BLOCKED: line.\n\n${log}`, @@ -149,10 +165,30 @@ runHook(async () => { ) } - const outcomes = [await decide(primary, true)] - if (anchor !== primary) outcomes.push(await decide(anchor, false)) + // One project's error never hides another project's result: it blocks the stop + // once with the error, so the agent cannot finish as if that project were + // verified, and the other projects are still judged. On the continuation the + // stop is let through, so the agent is never held in a loop, but the error is + // reported as a hook error (standard error, exit 1), which the IDE shows the + // human. + async function judge(projectDir, own) { + try { + return await decide(projectDir, own) + } catch (error) { + const text = `PAACM stop gate could not check ${projectDir}: ${error && error.message ? error.message : String(error)}` + if (continuation) { + process.stderr.write(`${text}\n`) + process.exitCode = 1 + return ALLOW + } + return block(`${text}\n\nDo not finish as if this project were verified: run \`paacm gate\` there, and report the error to the human.`) + } + } + + const outcomes = [await judge(primary, true)] + if (anchor !== primary) outcomes.push(await judge(anchor, false)) if (worktreeRoot(anchor) === null) { - for (const dir of await editedCheckouts(anchor, sessionId)) if (dir !== primary) outcomes.push(await decide(dir, false)) + for (const dir of await editedCheckouts(anchor, sessionId)) if (dir !== primary) outcomes.push(await judge(dir, false)) } respond(outcomes.find((o) => o.kind === 'block') ?? outcomes.find((o) => o.kind === 'guide') ?? ALLOW) }) diff --git a/plugins/paacm/skills/paacm/scripts/lib/gate.mjs b/plugins/paacm/skills/paacm/scripts/lib/gate.mjs index ec37891..4b7545e 100644 --- a/plugins/paacm/skills/paacm/scripts/lib/gate.mjs +++ b/plugins/paacm/skills/paacm/scripts/lib/gate.mjs @@ -3,6 +3,7 @@ // task state. Exit codes: 0 passed, 1 a check failed, 2 invalid configuration, // 3 a required tier has no checks, 4 a protected test file or the gate's own // configuration changed after the tests were approved. +import { lstatSync } from 'node:fs' import path from 'node:path' import { auditTask } from './audit.mjs' import { MANIFEST_PATH, missingTiers, parseManifest } from './checks.mjs' @@ -22,10 +23,25 @@ export async function loadChecks(projectDir) { } // Identifies the exact state of the working tree: HEAD plus every uncommitted -// change (tracked diffs and untracked files). Task files are excluded. +// change (tracked diffs, and untracked files with their size and modification +// time, as git's index records them, so a changed untracked file counts without +// reading it; a symbolic link by its own entry, as git tracks it). A file that +// cannot be inspected counts by its error code, so this never throws. Task files +// are excluded. export function worktreeFingerprint(projectDir) { const tracked = git(projectDir, ['diff', '--no-color', 'HEAD', '--', '.', ':(exclude).agent/tasks'], { allowFail: true }).stdout - const untracked = git(projectDir, ['ls-files', '--others', '--exclude-standard', '-z'], { allowFail: true }).stdout + const untracked = git(projectDir, ['ls-files', '--others', '--exclude-standard', '-z'], { allowFail: true }) + .stdout.split('\0') + .filter((p) => p !== '' && !p.startsWith('.agent/tasks/')) + .map((p) => { + try { + const s = lstatSync(path.join(projectDir, ...p.split('/')), { throwIfNoEntry: false }) + return `${p}\t${s ? `${s.size}\t${s.mtimeMs}` : '-'}` + } catch (error) { + return `${p}\t!${error.code ?? 'error'}` + } + }) + .join('\0') return sha256(`${head(projectDir) ?? ''}\0${tracked}\0${untracked}`) } @@ -82,12 +98,19 @@ export async function runGate(projectDir, { tier, log = () => {}, sink } = {}) { keep(text) if (userSink) userSink(text) } + // The log is evidence, not the result: a log that cannot be written (a full + // disk, a directory in its place) is reported, and the gate's result stands. const saveLog = async () => { if (!state) return - log(`Full output: ${TASKS_DIR}/${state.id}/gate.log`) + const rel = `${TASKS_DIR}/${state.id}/gate.log` + log(`Full output: ${rel}`) const cut = dropped || transcript.length > LOG_LIMIT const text = cut ? `[… earlier output dropped; the last ${LOG_LIMIT} characters follow …]\n${transcript.slice(-LOG_LIMIT)}` : transcript - await writeFileAtomic(path.join(taskDir(projectDir, state.id), 'gate.log'), text) + try { + await writeFileAtomic(path.join(taskDir(projectDir, state.id), 'gate.log'), text) + } catch (error) { + log(`WARN could not save ${rel}, so the full output is lost: ${error.message}`) + } } const requiredTier = tierForRoute(config, state) const effectiveTier = tier ?? requiredTier @@ -132,7 +155,7 @@ export async function runGate(projectDir, { tier, log = () => {}, sink } = {}) { } const run = await runChecks(projectDir, checks, effectiveTier, { timeoutSec: config.checks.timeoutSec, sink, log }) - const failures = await record(run.passed, run.failed, run.results.some((r) => r.exitCode !== 0 && isMissingTool(r))) + const failures = await record(run.passed, run.failed, run.results.some((r) => r.exitCode !== 0 && isMissingTool(r, projectDir))) if (!run.passed && failures !== null && failures >= config.gate.retryCap) { log(`RETRY CAP REACHED: ${failures} consecutive failures. Stop trying; run paacm task block --trigger 5 --reason "" and end your turn with BLOCKED.`) } diff --git a/plugins/paacm/skills/paacm/scripts/lib/git.mjs b/plugins/paacm/skills/paacm/scripts/lib/git.mjs index 71dc3fe..556c280 100644 --- a/plugins/paacm/skills/paacm/scripts/lib/git.mjs +++ b/plugins/paacm/skills/paacm/scripts/lib/git.mjs @@ -52,6 +52,17 @@ export function commitExists(cwd, sha) { return git(cwd, ['cat-file', '-e', `${sha}^{commit}`], { allowFail: true }).ok } +// Whether a repository-relative POSIX path existed in a commit. +export function existsAt(cwd, sha, rel) { + return git(cwd, ['cat-file', '-e', `${sha}:${rel}`], { allowFail: true }).ok +} + +// Untracked files whose names say they hold keys or secrets: the names the IDE +// settings keep agents from reading (.env, .env.*, *.pem, *.key), certificate +// stores and SSH private keys. Their content never goes into a review. +const SECRET_NAMES = [/^\.env$/, /^\.env\./, /\.(?:pem|key|p12|pfx)$/i, /^id_(?:rsa|dsa|ecdsa|ed25519)(?:[._-].*)?$/] +export const looksSecret = (rel) => SECRET_NAMES.some((re) => re.test(path.posix.basename(rel))) + // Paths (POSIX, repository-relative) with uncommitted changes, including untracked // files. Renames report both the old and the new path. export function uncommittedPaths(cwd) { @@ -107,8 +118,9 @@ export function ownChangesSince(cwd, base) { // The change from `base` to the working tree, committed, uncommitted and // untracked (unless git ignores the file): `files` ({ path, added, deleted, -// binary }, in diff order), the `diff` itself, and `skipped`, the paths git could -// not add (with its message). It is computed in a temporary index in the git +// binary }, in diff order), the `diff` itself, `skipped`, the paths git could +// not add (with its message), and `withheld`, untracked files left out because +// their names say they hold secrets (`looksSecret`). It is computed in a temporary index in the git // directory, started from a copy of the real index (so unchanged files are not // read again, and files git is told to skip stay skipped); the real index is // never touched. Without a base it compares with the empty tree. Throws when git @@ -124,6 +136,13 @@ export function workingTreeDiff(cwd, base) { const added = git(cwd, ['add', '-A', '--ignore-errors'], { env, allowFail: true }) if (added.status !== 0 && added.status !== 1) throw new Error(`git add failed: ${(added.stderr || '').trim()}`) const skipped = [...(added.stderr || '').matchAll(/^error: (.+)$/gm)].map((m) => m[1].trim()) + // The whole repository's untracked files, by repository-relative path, as the + // diff names them, whatever folder of the worktree the project is. + const top = topLevel(cwd) ?? cwd + const withheld = git(top, ['ls-files', '--others', '--exclude-standard', '-z']) + .stdout.split('\0') + .filter((p) => p !== '' && looksSecret(p)) + if (withheld.length) git(top, ['update-index', '--force-remove', '-z', '--stdin'], { env, input: `${withheld.join('\0')}\0` }) const from = base ?? git(cwd, ['hash-object', '-t', 'tree', '--stdin'], { input: '' }).stdout.trim() // Fixed prefixes and unescaped non-ASCII names, so each section's header names // its file whatever the user's diff settings; submodules as commit lines. @@ -137,7 +156,7 @@ export function workingTreeDiff(cwd, base) { const binary = added === '-' return { path: rest.join('\t'), added: binary ? 0 : Number(added), deleted: binary ? 0 : Number(deleted), binary } }) - return { files, diff: run(['--no-color']), skipped } + return { files, diff: run(['--no-color']), skipped, withheld } } finally { rmSync(index, { force: true }) rmSync(`${index}.lock`, { force: true }) diff --git a/plugins/paacm/skills/paacm/scripts/lib/hookio.mjs b/plugins/paacm/skills/paacm/scripts/lib/hookio.mjs index be52d93..baefd33 100644 --- a/plugins/paacm/skills/paacm/scripts/lib/hookio.mjs +++ b/plugins/paacm/skills/paacm/scripts/lib/hookio.mjs @@ -101,6 +101,9 @@ export function toProjectPath(projectDir, filePath) { // in it, by the project at the same place in that worktree; // - when the session's folder (`anchor`) is not in a git repository, a file in // any checkout below it by that checkout's root. +// It throws when git cannot say whether another worktree belongs to the same +// repository, so that the protect hook, which fails closed, refuses the edit +// instead of treating the file as outside every project. export function editLocation(projectDir, filePath, anchor = projectDir) { if (typeof filePath !== 'string' || filePath === '') return null const abs = realPath(path.resolve(projectDir, filePath)) @@ -113,7 +116,7 @@ export function editLocation(projectDir, filePath, anchor = projectDir) { } if (fileTree !== null && projectTree !== null) { if (fileTree === projectTree) return at(project) ?? at(fileTree) - if (sameRepository(fileTree, projectTree)) return at(path.join(fileTree, path.relative(projectTree, project))) ?? at(fileTree) + if (sameRepository(fileTree, projectTree, { strict: true })) return at(path.join(fileTree, path.relative(projectTree, project))) ?? at(fileTree) } if (fileTree !== null && worktreeRoot(path.resolve(anchor)) === null) return at(fileTree) return at(project) diff --git a/plugins/paacm/skills/paacm/scripts/lib/project.mjs b/plugins/paacm/skills/paacm/scripts/lib/project.mjs index 3bc2959..2180c7c 100644 --- a/plugins/paacm/skills/paacm/scripts/lib/project.mjs +++ b/plugins/paacm/skills/paacm/scripts/lib/project.mjs @@ -65,17 +65,27 @@ function realOrResolved(p) { } } -// The repository's shared git directory, as a real path, or null outside git. +// The repository's shared git directory, as a real path (`dir`), or git's error +// message (`error`) when git cannot say. function commonDir(dir) { const r = git(dir, ['rev-parse', '--git-common-dir'], { allowFail: true }) - if (!r.ok) return null + if (!r.ok) return { dir: null, error: (r.stderr || '').trim() || `exit ${r.status}` } const real = realOrResolved(path.resolve(dir, r.stdout.trim())) - return process.platform === 'win32' || process.platform === 'darwin' ? real.toLowerCase() : real + return { dir: process.platform === 'win32' || process.platform === 'darwin' ? real.toLowerCase() : real, error: null } } -// Whether two directories belong to worktrees of the same repository. -export function sameRepository(a, b) { +// Whether two directories belong to worktrees of the same repository. When git +// cannot answer for a directory that is a worktree on disk (git not on the PATH, +// or refusing the folder as unsafe), the answer is unknown: `strict` throws then, +// for a caller that must not guess, and otherwise the answer is no. +export function sameRepository(a, b, { strict = false } = {}) { if (realOrResolved(a) === realOrResolved(b)) return true const ca = commonDir(a) - return ca !== null && ca === commonDir(b) + const cb = commonDir(b) + if (ca.dir !== null && cb.dir !== null) return ca.dir === cb.dir + if (strict && worktreeRoot(a) !== null && worktreeRoot(b) !== null) { + const [failed, why] = ca.dir === null ? [a, ca.error] : [b, cb.error] + throw new Error(`git could not tell whether ${a} and ${b} belong to the same repository: git rev-parse --git-common-dir failed in ${failed}: ${why}. Check that git runs here and accepts these folders (safe.directory).`) + } + return false } diff --git a/plugins/paacm/skills/paacm/scripts/lib/runner.mjs b/plugins/paacm/skills/paacm/scripts/lib/runner.mjs index da55e29..f5b4740 100644 --- a/plugins/paacm/skills/paacm/scripts/lib/runner.mjs +++ b/plugins/paacm/skills/paacm/scripts/lib/runner.mjs @@ -4,6 +4,7 @@ // a bounded tail is kept for reports. A check that exceeds the timeout is killed // together with its child processes. import { spawn, spawnSync } from 'node:child_process' +import path from 'node:path' import { tiersFor } from './checks.mjs' const TAIL_BYTES = 16 * 1024 @@ -64,15 +65,39 @@ export function runOne(projectDir, check, { timeoutSec, sink } = {}) { }) } -// Whether a check failed because its shell could not find a command: exit 127 +// The last command a shell said it could not find in the check's output, which +// is the failing one: dash ("sh: 1: x: not found"), bash ("x: command not found", +// "x: No such file or directory"), zsh ("command not found: x") and cmd ("'x' is +// not recognized"). Null when no such message is found. +function lastNotFound(output) { + let last = null + const patterns = [ + /([^\s:'"`]+): (?:command )?not found\b/g, + /([^\s:'"`]+): No such file or directory\b/g, + /(?:command not found|no such file or directory): ([^\s'"`]+)/gi, + /'([^']+)' is not recognized as an internal or external command/g, + ] + for (const re of patterns) for (const m of output.matchAll(re)) if (!last || m.index > last.index) last = { index: m.index, name: m[1] } + return last ? last.name : null +} + +// Whether a check failed because its shell could not find a program: exit 127 // from a POSIX shell (also reported when the shell itself cannot start). On // Windows, `cmd /c` exits 1 and says the command "is not recognized" (9009 is // what a batch file sees); cmd prints that in the system's language, and only -// the English message is recognized. -export function isMissingTool(result) { - if (result.exitCode === 127) return true - if (process.platform !== 'win32') return false - return result.exitCode === 9009 || (result.exitCode === 1 && /is not recognized as an internal or external command/.test(result.outputTail ?? '')) +// the English message is recognized. A command named by a path inside the +// project (`./build.sh`, `scripts/x`) is a file of the project, not a tool to +// install: a change may have removed or renamed it, so that failure is an +// ordinary one. A path outside the project (`/opt/tools/bin/x`) is a tool. +export function isMissingTool(result, projectDir) { + const output = result.outputTail ?? '' + let missing = result.exitCode === 127 + if (!missing && process.platform === 'win32') missing = result.exitCode === 9009 || (result.exitCode === 1 && /is not recognized as an internal or external command/.test(output)) + if (!missing) return false + const name = lastNotFound(output) + if (!name || !/[\\/]/.test(name) || !projectDir) return true + const rel = path.relative(path.resolve(projectDir), path.resolve(projectDir, name)) + return rel === '' || rel === '..' || rel.startsWith(`..${path.sep}`) || path.isAbsolute(rel) } // Runs every check of the mode's tiers in manifest order and stops at the first @@ -90,7 +115,7 @@ export async function runChecks(projectDir, checks, mode, { timeoutSec, sink, lo log(`PASS [${check.tier}] ${check.label} (${secs}s)`) } else { log(`FAIL [${check.tier}] ${check.label} exit ${r.exitCode}${r.timedOut ? ` (timed out after ${timeoutSec}s)` : ''} (${secs}s)`) - if (isMissingTool(r)) { + if (isMissingTool(r, projectDir)) { log(`HINT [${check.tier}] ${check.label}: a command was not found (exit ${r.exitCode}). This is the environment, not the code: do not change code to get around it. Without the tool the gate cannot pass, so list the check under CHECKS NOT RUN and stop with BLOCKED: trigger 2 (paacm task block --trigger 2) to ask the human to install it.`) } break diff --git a/plugins/paacm/skills/paacm/scripts/lib/shell-rules.mjs b/plugins/paacm/skills/paacm/scripts/lib/shell-rules.mjs index fdc0fb5..982425a 100644 --- a/plugins/paacm/skills/paacm/scripts/lib/shell-rules.mjs +++ b/plugins/paacm/skills/paacm/scripts/lib/shell-rules.mjs @@ -67,8 +67,10 @@ export const SHELL_RULES = [ [ // --no-verify, or an abbreviation git accepts, on any git command String.raw`\bgit\b[^\n;&|]*\s--no-veri[a-z]*\b`, - // git commit -n (alone or in a cluster of short flags), after any global options - String.raw`\bgit(?:\s+(?:-[Cc]\s+\S+|-{1,2}[A-Za-z][\w-]*(?:=\S+)?))*\s+commit\b[^\n;&|]*\s-[a-z]*n[a-z]*(?=[\s;&|<>()'"]|$)`, + // git commit -n (alone or in a cluster of short flags), after any global options. + // A bare -c or -C takes the next word; the other branch never reads them, so + // each option has one reading and the check stays linear. + String.raw`\bgit(?:\s+(?:-[Cc]\s+\S+|-{1,2}(?![Cc](?:\s|$))[A-Za-z][\w-]*(?:=\S+)?))*\s+commit\b[^\n;&|]*\s-[a-z]*n[a-z]*(?=[\s;&|<>()'"]|$)`, // another hooks directory, given on the command line or in the configuration String.raw`\bgit\b[^\n;&|]*\s-c\s*['"]?core\.hookspath\b`, String.raw`\bgit\b[^\n;&|]*\bconfig\b[^\n;&|]*[\s'"]core\.hookspath\b`, diff --git a/plugins/paacm/skills/paacm/scripts/lib/task.mjs b/plugins/paacm/skills/paacm/scripts/lib/task.mjs index 7473873..fbed73b 100644 --- a/plugins/paacm/skills/paacm/scripts/lib/task.mjs +++ b/plugins/paacm/skills/paacm/scripts/lib/task.mjs @@ -4,7 +4,7 @@ import { realpathSync } from 'node:fs' import { appendFile, mkdir, readdir, rm } from 'node:fs/promises' import path from 'node:path' import { readJsonOrNull, readTextOrNull, stableJson, writeFileAtomic } from './fs.mjs' -import { GIT_BUFFER, generatedPaths, git, head, shortSha, uncommittedPaths, workingTreeDiff } from './git.mjs' +import { existsAt, GIT_BUFFER, generatedPaths, git, head, shortSha, uncommittedPaths, workingTreeDiff } from './git.mjs' import { auditTask } from './audit.mjs' import { isProtectedTest, loadConfig, tierAtLeast, tierForRoute } from './config.mjs' import { readExtension, sectionOf } from './extensions.mjs' @@ -70,18 +70,28 @@ export async function appendProgress(projectDir, id, text) { await appendFile(path.join(taskDir(projectDir, id), 'progress.md'), line) } -// The most recently updated task of the worktree, or null. +// The most recently updated task of the worktree, or null. A tasks folder or a +// task state that cannot be read is skipped with a warning on standard error, so +// the human learns that a blocked task there was not checked. async function latestTask(projectDir) { + const warn = (what, error) => process.stderr.write(`paacm: warning: could not read ${what}, so a blocked task there is not checked: ${error.message}\n`) let latest = null let entries = [] try { entries = await readdir(tasksDir(projectDir), { withFileTypes: true }) - } catch { + } catch (error) { + if (error.code !== 'ENOENT' && error.code !== 'ENOTDIR') warn(tasksDir(projectDir), error) return null } for (const entry of entries) { if (!entry.isDirectory()) continue - const state = await readJsonOrNull(path.join(tasksDir(projectDir), entry.name, 'state.json')).catch(() => null) + const file = path.join(tasksDir(projectDir), entry.name, 'state.json') + let state = null + try { + state = await readJsonOrNull(file) + } catch (error) { + warn(file, error) + } if (state && state.updatedAt && (!latest || state.updatedAt > latest.updatedAt)) latest = state } return latest @@ -113,14 +123,19 @@ function otherWorktrees(projectDir) { // the change is settled in the blocked task. The tasks checked are the latest one // of this worktree (with `own`) and the latest one of each other worktree whose // approved test commit this HEAD contains: the refusal follows the commit, not -// one .agent/tasks folder. +// one .agent/tasks folder. For another worktree's task only a file that existed +// at the approval counts: a test added later was never frozen by it. A change +// that arrived from the base branch still counts, since git cannot tell a +// reviewed change from one the writer made a remote-tracking ref for; the +// refusal names the blocked task and its worktree, where the human settles it. async function refuseOverBlockedTask(projectDir, config, { own, before }) { const dirs = [...(own ? [projectDir] : []), ...otherWorktrees(projectDir)] for (const dir of dirs) { const latest = await latestTask(dir) if (!latest || latest.status !== 'blocked' || !latest.testsApprovedAt) continue if (dir !== projectDir && !git(projectDir, ['merge-base', '--is-ancestor', latest.testsApprovedAt, 'HEAD'], { allowFail: true }).ok) continue - const changed = auditTask(projectDir, config, latest).violations.map((v) => v.path) + let changed = auditTask(projectDir, config, latest).violations.map((v) => v.path) + if (dir !== projectDir) changed = changed.filter((p) => existsAt(projectDir, latest.testsApprovedAt, p)) if (!changed.length) continue const where = dir === projectDir ? '' : ` (in the worktree at ${dir})` throw new TaskError( @@ -508,6 +523,9 @@ export async function writeReviewInput(projectDir, config) { '', entries.length ? entries.map((e) => `- ${e.path} (${stats(e)})`).join('\n') : '(none)', ...(change.skipped.length ? ['', 'These paths could not be added to the review, so their changes are not in it:', ...change.skipped.map((m) => `- ${m}`)] : []), + ...(change.withheld.length + ? ['', 'These untracked files look like keys or secrets, so their content is not in this review. Add them to .gitignore, or commit them if they belong to the change:', ...change.withheld.map((p) => `- ${p}`)] + : []), '', ...(checklist ? ['## Project reviewer checklist', '', 'Check each item against the diff as well, and report a violated item as a finding.', '', checklist, ''] : []), ...(separate.length diff --git a/src/content/kit.json b/src/content/kit.json index cc2c793..be15371 100644 --- a/src/content/kit.json +++ b/src/content/kit.json @@ -1,6 +1,6 @@ { "name": "paacm", - "version": "3.5.0", + "version": "3.5.1", "description": "PAACM: the Verified Single-Writer Loop for agentic coding. One writer agent, read-only explorer and reviewer subagents, tests protected after approval, and completion gated on executed checks.", "author": { "name": "Hyper AI Lab", diff --git a/src/content/skills/paacm-verify/SKILL.md b/src/content/skills/paacm-verify/SKILL.md index 60cd0b4..65b7d0a 100644 --- a/src/content/skills/paacm-verify/SKILL.md +++ b/src/content/skills/paacm-verify/SKILL.md @@ -14,7 +14,7 @@ If `.agent/extensions/07-verify.md` exists, read it first and follow it together 1. **Run the gate:** `paacm gate --quiet`, on its own (no pipe, no `;`). It audits protected paths and runs the check tier for the current task's route; `--quiet` prints the check lines and a failing check's last lines, and with a task the full output is saved as `gate.log` in the task folder. Without a current task it runs the `quick` tier. Add `--tier full` before delivering STANDARD work, and `--tier security` for security-relevant changes. 2. **Read the exit code:** - **0:** passed. - - **1:** a check failed. Fix the cause and run the gate again. If the check exited 127 (on Windows, cmd's "is not recognized" message), a command was not found: that is the environment, not the code. Record the check as not run and stop with `BLOCKED:` trigger 2 to ask the human for the tool. + - **1:** a check failed. Fix the cause and run the gate again. If the check exited 127 (on Windows, cmd's "is not recognized" message), a command was not found. When it is a program that is not installed, that is the environment, not the code: record the check as not run and stop with `BLOCKED:` trigger 2 to ask the human for the tool. When it is a file of the project (a path such as `./scripts/build.sh`) that the change removed or renamed, fix it like any failure. - **3:** a tier has no checks. Record it as not run and tell the human it needs configuring. - **4:** a protected path changed. Undo the change, or escalate if the change is needed. 3. **After three consecutive failures of the same check, stop and escalate** (trigger 5) instead of trying again. diff --git a/src/content/skills/paacm/references/07-verify.md b/src/content/skills/paacm/references/07-verify.md index 1276cdd..5e041f7 100644 --- a/src/content/skills/paacm/references/07-verify.md +++ b/src/content/skills/paacm/references/07-verify.md @@ -20,7 +20,9 @@ When the gate passes, it prints the next phase: review (phase 8) for STANDARD and DEEP, deliver (phase 9) for FAST, after `paacm task phase verifying` if you have not recorded it. A passing gate at a lower tier than the route's does not count for `paacm task done`. - **A missing tool.** A check that fails with exit 127 (on Windows, cmd's "is not recognized" message) could not find a command, and the gate says so. That is the environment, not the code: do not change code to get around it. The gate cannot pass without the tool, so list the check under CHECKS NOT RUN and stop with `BLOCKED:` trigger 2 (`paacm task block --trigger 2 --reason ""`) to ask the human to install it. + **A missing tool.** A check that fails with exit 127 (on Windows, cmd's "is not recognized" message) could not find a program, and the gate says so. That is the environment, not the code: do not change code to get around it. The gate cannot pass without the tool, so list the check under CHECKS NOT RUN and stop with `BLOCKED:` trigger 2 (`paacm task block --trigger 2 --reason ""`) to ask the human to install it. + + A command named by a path in the project (such as `./scripts/build.sh`) is not a tool: when the shell cannot find it, the change probably removed or renamed it, and the gate reports an ordinary failure. Fix it like any other. 3. **STANDARD work:** also run `paacm gate --tier full` before delivery. If the `full` tier is not configured, record that under CHECKS NOT RUN. 4. **Security-relevant diffs** (input handling, authentication, permissions, secrets, cryptography, dependency changes): also run `paacm gate --tier security`. Confirm that every new dependency exists in its registry under the exact name you used. diff --git a/src/runtime/hooks/stop-gate.mjs b/src/runtime/hooks/stop-gate.mjs index b48129a..8c126c0 100644 --- a/src/runtime/hooks/stop-gate.mjs +++ b/src/runtime/hooks/stop-gate.mjs @@ -23,7 +23,6 @@ import { changedThisSession, editedCheckouts, sessionIdFrom } from '../lib/sessi import { currentTask, writeState } from '../lib/task.mjs' const GATED = new Set(['implementing', 'verifying', 'reviewing']) -const NUDGE_DEDUP_MS = 15_000 const isTaskFile = (p) => p === '.agent/tasks' || p.startsWith('.agent/tasks/') const ALLOW = { kind: 'allow' } const block = (text) => ({ kind: 'block', text }) @@ -59,6 +58,15 @@ runHook(async () => { const lastMessage = typeof input.last_assistant_message === 'string' ? input.last_assistant_message : '' if (/^\s*BLOCKED:/m.test(lastMessage)) return respond(ALLOW) + // A gate that cannot run (a tier without checks, an invalid configuration): + // the agent cannot fix this by changing code, so it is told once. + const unverifiable = (log) => + continuation + ? ALLOW + : guide( + `PAACM gate could not verify this task:\n${log}\n\nYou cannot fix this by changing code. List the missing checks under CHECKS NOT RUN in the report, and ask the human to configure .agent/checks.conf (or fix .agent/paacm.json). Use \`paacm task block --trigger 2 --reason ""\` if the task cannot be verified without it.`, + ) + // The decision for one project. `own` is the project the agent works in; only // there are uncommitted files without a task worth a nudge. async function decide(projectDir, own) { @@ -77,17 +85,19 @@ runHook(async () => { } if (task.status === 'blocked' || task.status === 'done') return ALLOW - // A nudge is given once: never on a continuation, and not again when the same - // stop arrives twice (Cursor also runs the Claude Code hooks it imports). The - // state is read again before writing: a gate this hook just ran has recorded - // its result there, which the task read above does not have. + // A nudge is given once: never on a continuation, and not again for the same + // working tree and phase, however much later the next stop comes (an agent + // waiting for the human stops at every turn; Cursor also runs the Claude Code + // hooks it imports). The state is read again before writing: a gate this hook + // just ran has recorded its result there, which the task read above does not + // have. const nudge = async (kind, message) => { if (continuation) return ALLOW const fingerprint = worktreeFingerprint(projectDir) const current = (await currentTask(projectDir)) ?? task const last = current.lastNudge - if (last && last.kind === kind && last.fingerprint === fingerprint && Date.now() - Date.parse(last.at) < NUDGE_DEDUP_MS) return ALLOW - current.lastNudge = { kind, fingerprint, at: new Date().toISOString() } + if (last && last.kind === kind && last.fingerprint === fingerprint && last.status === current.status) return ALLOW + current.lastNudge = { kind, fingerprint, status: current.status, at: new Date().toISOString() } await writeState(projectDir, current) return guide(message) } @@ -111,13 +121,24 @@ runHook(async () => { const finish = () => nudge('finish', `PAACM: the gate is green, but task ${task.id} is still ${task.status}. If the work is complete, finish it: ${finishSteps(projectDir, task)}. ${WAITING}`) - const config = await loadConfig(projectDir) + let config + try { + config = await loadConfig(projectDir) + } catch (error) { + return unverifiable(`CONFIG ERROR: ${error.message}`) + } const required = tierForRoute(config, task) const g = task.lastGate const dirty = uncommittedPaths(projectDir).some((p) => !isTaskFile(p)) if (g && g.passed && g.head === head(projectDir) && !dirty && !g.dirty && tierAtLeast(g.tier, required)) return finish() - const duplicate = recentGate(projectDir, task) + // A result from a moment ago is the same stop delivered twice. On a + // continuation the agent only answered the previous stop, so a passing result + // for the same working tree also counts however old it is; a failing one is + // run again, since the agent may have fixed what the fingerprint cannot see + // (an install, a service). + const lasting = continuation ? recentGate(projectDir, task, Infinity) : null + const duplicate = recentGate(projectDir, task) ?? (lasting && lasting.passed ? lasting : null) if (duplicate && tierAtLeast(duplicate.tier, required)) { if (duplicate.passed) return finish() if (duplicate.missingTool) return block(missingToolMessage(duplicate.failed, ' See the previous gate output.')) @@ -129,16 +150,11 @@ runHook(async () => { if (result.code === GATE.PASSED) return finish() const log = lines.slice(-12).join('\n') - if (result.code === GATE.UNCONFIGURED || result.code === GATE.CONFIG) { - if (continuation) return ALLOW - return guide( - `PAACM gate could not verify this task:\n${log}\n\nYou cannot fix this by changing code. List the missing checks under CHECKS NOT RUN in the report, and ask the human to configure .agent/checks.conf (or fix .agent/paacm.json). Use \`paacm task block --trigger 2 --reason ""\` if the task cannot be verified without it.`, - ) - } + if (result.code === GATE.UNCONFIGURED || result.code === GATE.CONFIG) return unverifiable(log) const last = result.results?.length ? result.results[result.results.length - 1] : null const tail = last ? (last.outputTail || '').trim().split('\n').slice(-30).join('\n') : '' - if (last && isMissingTool(last)) return block(missingToolMessage(last.label, ` The check exited ${last.exitCode}.\n\n${log}${tail ? `\n\nLast output of the check:\n${tail}` : ''}`)) + if (last && isMissingTool(last, projectDir)) return block(missingToolMessage(last.label, ` The check exited ${last.exitCode}.\n\n${log}${tail ? `\n\nLast output of the check:\n${tail}` : ''}`)) if (result.gateFailures >= result.retryCap) { return block( `PAACM gate failed ${result.gateFailures} times in a row; the retry cap (${result.retryCap}) is reached. Do not try another fix. Run \`paacm task block --trigger 5 --reason ""\`, write REPORT.md, and end your turn with a BLOCKED: line.\n\n${log}`, @@ -149,10 +165,30 @@ runHook(async () => { ) } - const outcomes = [await decide(primary, true)] - if (anchor !== primary) outcomes.push(await decide(anchor, false)) + // One project's error never hides another project's result: it blocks the stop + // once with the error, so the agent cannot finish as if that project were + // verified, and the other projects are still judged. On the continuation the + // stop is let through, so the agent is never held in a loop, but the error is + // reported as a hook error (standard error, exit 1), which the IDE shows the + // human. + async function judge(projectDir, own) { + try { + return await decide(projectDir, own) + } catch (error) { + const text = `PAACM stop gate could not check ${projectDir}: ${error && error.message ? error.message : String(error)}` + if (continuation) { + process.stderr.write(`${text}\n`) + process.exitCode = 1 + return ALLOW + } + return block(`${text}\n\nDo not finish as if this project were verified: run \`paacm gate\` there, and report the error to the human.`) + } + } + + const outcomes = [await judge(primary, true)] + if (anchor !== primary) outcomes.push(await judge(anchor, false)) if (worktreeRoot(anchor) === null) { - for (const dir of await editedCheckouts(anchor, sessionId)) if (dir !== primary) outcomes.push(await decide(dir, false)) + for (const dir of await editedCheckouts(anchor, sessionId)) if (dir !== primary) outcomes.push(await judge(dir, false)) } respond(outcomes.find((o) => o.kind === 'block') ?? outcomes.find((o) => o.kind === 'guide') ?? ALLOW) }) diff --git a/src/runtime/lib/gate.mjs b/src/runtime/lib/gate.mjs index ec37891..4b7545e 100644 --- a/src/runtime/lib/gate.mjs +++ b/src/runtime/lib/gate.mjs @@ -3,6 +3,7 @@ // task state. Exit codes: 0 passed, 1 a check failed, 2 invalid configuration, // 3 a required tier has no checks, 4 a protected test file or the gate's own // configuration changed after the tests were approved. +import { lstatSync } from 'node:fs' import path from 'node:path' import { auditTask } from './audit.mjs' import { MANIFEST_PATH, missingTiers, parseManifest } from './checks.mjs' @@ -22,10 +23,25 @@ export async function loadChecks(projectDir) { } // Identifies the exact state of the working tree: HEAD plus every uncommitted -// change (tracked diffs and untracked files). Task files are excluded. +// change (tracked diffs, and untracked files with their size and modification +// time, as git's index records them, so a changed untracked file counts without +// reading it; a symbolic link by its own entry, as git tracks it). A file that +// cannot be inspected counts by its error code, so this never throws. Task files +// are excluded. export function worktreeFingerprint(projectDir) { const tracked = git(projectDir, ['diff', '--no-color', 'HEAD', '--', '.', ':(exclude).agent/tasks'], { allowFail: true }).stdout - const untracked = git(projectDir, ['ls-files', '--others', '--exclude-standard', '-z'], { allowFail: true }).stdout + const untracked = git(projectDir, ['ls-files', '--others', '--exclude-standard', '-z'], { allowFail: true }) + .stdout.split('\0') + .filter((p) => p !== '' && !p.startsWith('.agent/tasks/')) + .map((p) => { + try { + const s = lstatSync(path.join(projectDir, ...p.split('/')), { throwIfNoEntry: false }) + return `${p}\t${s ? `${s.size}\t${s.mtimeMs}` : '-'}` + } catch (error) { + return `${p}\t!${error.code ?? 'error'}` + } + }) + .join('\0') return sha256(`${head(projectDir) ?? ''}\0${tracked}\0${untracked}`) } @@ -82,12 +98,19 @@ export async function runGate(projectDir, { tier, log = () => {}, sink } = {}) { keep(text) if (userSink) userSink(text) } + // The log is evidence, not the result: a log that cannot be written (a full + // disk, a directory in its place) is reported, and the gate's result stands. const saveLog = async () => { if (!state) return - log(`Full output: ${TASKS_DIR}/${state.id}/gate.log`) + const rel = `${TASKS_DIR}/${state.id}/gate.log` + log(`Full output: ${rel}`) const cut = dropped || transcript.length > LOG_LIMIT const text = cut ? `[… earlier output dropped; the last ${LOG_LIMIT} characters follow …]\n${transcript.slice(-LOG_LIMIT)}` : transcript - await writeFileAtomic(path.join(taskDir(projectDir, state.id), 'gate.log'), text) + try { + await writeFileAtomic(path.join(taskDir(projectDir, state.id), 'gate.log'), text) + } catch (error) { + log(`WARN could not save ${rel}, so the full output is lost: ${error.message}`) + } } const requiredTier = tierForRoute(config, state) const effectiveTier = tier ?? requiredTier @@ -132,7 +155,7 @@ export async function runGate(projectDir, { tier, log = () => {}, sink } = {}) { } const run = await runChecks(projectDir, checks, effectiveTier, { timeoutSec: config.checks.timeoutSec, sink, log }) - const failures = await record(run.passed, run.failed, run.results.some((r) => r.exitCode !== 0 && isMissingTool(r))) + const failures = await record(run.passed, run.failed, run.results.some((r) => r.exitCode !== 0 && isMissingTool(r, projectDir))) if (!run.passed && failures !== null && failures >= config.gate.retryCap) { log(`RETRY CAP REACHED: ${failures} consecutive failures. Stop trying; run paacm task block --trigger 5 --reason "" and end your turn with BLOCKED.`) } diff --git a/src/runtime/lib/git.mjs b/src/runtime/lib/git.mjs index 71dc3fe..556c280 100644 --- a/src/runtime/lib/git.mjs +++ b/src/runtime/lib/git.mjs @@ -52,6 +52,17 @@ export function commitExists(cwd, sha) { return git(cwd, ['cat-file', '-e', `${sha}^{commit}`], { allowFail: true }).ok } +// Whether a repository-relative POSIX path existed in a commit. +export function existsAt(cwd, sha, rel) { + return git(cwd, ['cat-file', '-e', `${sha}:${rel}`], { allowFail: true }).ok +} + +// Untracked files whose names say they hold keys or secrets: the names the IDE +// settings keep agents from reading (.env, .env.*, *.pem, *.key), certificate +// stores and SSH private keys. Their content never goes into a review. +const SECRET_NAMES = [/^\.env$/, /^\.env\./, /\.(?:pem|key|p12|pfx)$/i, /^id_(?:rsa|dsa|ecdsa|ed25519)(?:[._-].*)?$/] +export const looksSecret = (rel) => SECRET_NAMES.some((re) => re.test(path.posix.basename(rel))) + // Paths (POSIX, repository-relative) with uncommitted changes, including untracked // files. Renames report both the old and the new path. export function uncommittedPaths(cwd) { @@ -107,8 +118,9 @@ export function ownChangesSince(cwd, base) { // The change from `base` to the working tree, committed, uncommitted and // untracked (unless git ignores the file): `files` ({ path, added, deleted, -// binary }, in diff order), the `diff` itself, and `skipped`, the paths git could -// not add (with its message). It is computed in a temporary index in the git +// binary }, in diff order), the `diff` itself, `skipped`, the paths git could +// not add (with its message), and `withheld`, untracked files left out because +// their names say they hold secrets (`looksSecret`). It is computed in a temporary index in the git // directory, started from a copy of the real index (so unchanged files are not // read again, and files git is told to skip stay skipped); the real index is // never touched. Without a base it compares with the empty tree. Throws when git @@ -124,6 +136,13 @@ export function workingTreeDiff(cwd, base) { const added = git(cwd, ['add', '-A', '--ignore-errors'], { env, allowFail: true }) if (added.status !== 0 && added.status !== 1) throw new Error(`git add failed: ${(added.stderr || '').trim()}`) const skipped = [...(added.stderr || '').matchAll(/^error: (.+)$/gm)].map((m) => m[1].trim()) + // The whole repository's untracked files, by repository-relative path, as the + // diff names them, whatever folder of the worktree the project is. + const top = topLevel(cwd) ?? cwd + const withheld = git(top, ['ls-files', '--others', '--exclude-standard', '-z']) + .stdout.split('\0') + .filter((p) => p !== '' && looksSecret(p)) + if (withheld.length) git(top, ['update-index', '--force-remove', '-z', '--stdin'], { env, input: `${withheld.join('\0')}\0` }) const from = base ?? git(cwd, ['hash-object', '-t', 'tree', '--stdin'], { input: '' }).stdout.trim() // Fixed prefixes and unescaped non-ASCII names, so each section's header names // its file whatever the user's diff settings; submodules as commit lines. @@ -137,7 +156,7 @@ export function workingTreeDiff(cwd, base) { const binary = added === '-' return { path: rest.join('\t'), added: binary ? 0 : Number(added), deleted: binary ? 0 : Number(deleted), binary } }) - return { files, diff: run(['--no-color']), skipped } + return { files, diff: run(['--no-color']), skipped, withheld } } finally { rmSync(index, { force: true }) rmSync(`${index}.lock`, { force: true }) diff --git a/src/runtime/lib/hookio.mjs b/src/runtime/lib/hookio.mjs index be52d93..baefd33 100644 --- a/src/runtime/lib/hookio.mjs +++ b/src/runtime/lib/hookio.mjs @@ -101,6 +101,9 @@ export function toProjectPath(projectDir, filePath) { // in it, by the project at the same place in that worktree; // - when the session's folder (`anchor`) is not in a git repository, a file in // any checkout below it by that checkout's root. +// It throws when git cannot say whether another worktree belongs to the same +// repository, so that the protect hook, which fails closed, refuses the edit +// instead of treating the file as outside every project. export function editLocation(projectDir, filePath, anchor = projectDir) { if (typeof filePath !== 'string' || filePath === '') return null const abs = realPath(path.resolve(projectDir, filePath)) @@ -113,7 +116,7 @@ export function editLocation(projectDir, filePath, anchor = projectDir) { } if (fileTree !== null && projectTree !== null) { if (fileTree === projectTree) return at(project) ?? at(fileTree) - if (sameRepository(fileTree, projectTree)) return at(path.join(fileTree, path.relative(projectTree, project))) ?? at(fileTree) + if (sameRepository(fileTree, projectTree, { strict: true })) return at(path.join(fileTree, path.relative(projectTree, project))) ?? at(fileTree) } if (fileTree !== null && worktreeRoot(path.resolve(anchor)) === null) return at(fileTree) return at(project) diff --git a/src/runtime/lib/project.mjs b/src/runtime/lib/project.mjs index 3bc2959..2180c7c 100644 --- a/src/runtime/lib/project.mjs +++ b/src/runtime/lib/project.mjs @@ -65,17 +65,27 @@ function realOrResolved(p) { } } -// The repository's shared git directory, as a real path, or null outside git. +// The repository's shared git directory, as a real path (`dir`), or git's error +// message (`error`) when git cannot say. function commonDir(dir) { const r = git(dir, ['rev-parse', '--git-common-dir'], { allowFail: true }) - if (!r.ok) return null + if (!r.ok) return { dir: null, error: (r.stderr || '').trim() || `exit ${r.status}` } const real = realOrResolved(path.resolve(dir, r.stdout.trim())) - return process.platform === 'win32' || process.platform === 'darwin' ? real.toLowerCase() : real + return { dir: process.platform === 'win32' || process.platform === 'darwin' ? real.toLowerCase() : real, error: null } } -// Whether two directories belong to worktrees of the same repository. -export function sameRepository(a, b) { +// Whether two directories belong to worktrees of the same repository. When git +// cannot answer for a directory that is a worktree on disk (git not on the PATH, +// or refusing the folder as unsafe), the answer is unknown: `strict` throws then, +// for a caller that must not guess, and otherwise the answer is no. +export function sameRepository(a, b, { strict = false } = {}) { if (realOrResolved(a) === realOrResolved(b)) return true const ca = commonDir(a) - return ca !== null && ca === commonDir(b) + const cb = commonDir(b) + if (ca.dir !== null && cb.dir !== null) return ca.dir === cb.dir + if (strict && worktreeRoot(a) !== null && worktreeRoot(b) !== null) { + const [failed, why] = ca.dir === null ? [a, ca.error] : [b, cb.error] + throw new Error(`git could not tell whether ${a} and ${b} belong to the same repository: git rev-parse --git-common-dir failed in ${failed}: ${why}. Check that git runs here and accepts these folders (safe.directory).`) + } + return false } diff --git a/src/runtime/lib/runner.mjs b/src/runtime/lib/runner.mjs index da55e29..f5b4740 100644 --- a/src/runtime/lib/runner.mjs +++ b/src/runtime/lib/runner.mjs @@ -4,6 +4,7 @@ // a bounded tail is kept for reports. A check that exceeds the timeout is killed // together with its child processes. import { spawn, spawnSync } from 'node:child_process' +import path from 'node:path' import { tiersFor } from './checks.mjs' const TAIL_BYTES = 16 * 1024 @@ -64,15 +65,39 @@ export function runOne(projectDir, check, { timeoutSec, sink } = {}) { }) } -// Whether a check failed because its shell could not find a command: exit 127 +// The last command a shell said it could not find in the check's output, which +// is the failing one: dash ("sh: 1: x: not found"), bash ("x: command not found", +// "x: No such file or directory"), zsh ("command not found: x") and cmd ("'x' is +// not recognized"). Null when no such message is found. +function lastNotFound(output) { + let last = null + const patterns = [ + /([^\s:'"`]+): (?:command )?not found\b/g, + /([^\s:'"`]+): No such file or directory\b/g, + /(?:command not found|no such file or directory): ([^\s'"`]+)/gi, + /'([^']+)' is not recognized as an internal or external command/g, + ] + for (const re of patterns) for (const m of output.matchAll(re)) if (!last || m.index > last.index) last = { index: m.index, name: m[1] } + return last ? last.name : null +} + +// Whether a check failed because its shell could not find a program: exit 127 // from a POSIX shell (also reported when the shell itself cannot start). On // Windows, `cmd /c` exits 1 and says the command "is not recognized" (9009 is // what a batch file sees); cmd prints that in the system's language, and only -// the English message is recognized. -export function isMissingTool(result) { - if (result.exitCode === 127) return true - if (process.platform !== 'win32') return false - return result.exitCode === 9009 || (result.exitCode === 1 && /is not recognized as an internal or external command/.test(result.outputTail ?? '')) +// the English message is recognized. A command named by a path inside the +// project (`./build.sh`, `scripts/x`) is a file of the project, not a tool to +// install: a change may have removed or renamed it, so that failure is an +// ordinary one. A path outside the project (`/opt/tools/bin/x`) is a tool. +export function isMissingTool(result, projectDir) { + const output = result.outputTail ?? '' + let missing = result.exitCode === 127 + if (!missing && process.platform === 'win32') missing = result.exitCode === 9009 || (result.exitCode === 1 && /is not recognized as an internal or external command/.test(output)) + if (!missing) return false + const name = lastNotFound(output) + if (!name || !/[\\/]/.test(name) || !projectDir) return true + const rel = path.relative(path.resolve(projectDir), path.resolve(projectDir, name)) + return rel === '' || rel === '..' || rel.startsWith(`..${path.sep}`) || path.isAbsolute(rel) } // Runs every check of the mode's tiers in manifest order and stops at the first @@ -90,7 +115,7 @@ export async function runChecks(projectDir, checks, mode, { timeoutSec, sink, lo log(`PASS [${check.tier}] ${check.label} (${secs}s)`) } else { log(`FAIL [${check.tier}] ${check.label} exit ${r.exitCode}${r.timedOut ? ` (timed out after ${timeoutSec}s)` : ''} (${secs}s)`) - if (isMissingTool(r)) { + if (isMissingTool(r, projectDir)) { log(`HINT [${check.tier}] ${check.label}: a command was not found (exit ${r.exitCode}). This is the environment, not the code: do not change code to get around it. Without the tool the gate cannot pass, so list the check under CHECKS NOT RUN and stop with BLOCKED: trigger 2 (paacm task block --trigger 2) to ask the human to install it.`) } break diff --git a/src/runtime/lib/shell-rules.mjs b/src/runtime/lib/shell-rules.mjs index fdc0fb5..982425a 100644 --- a/src/runtime/lib/shell-rules.mjs +++ b/src/runtime/lib/shell-rules.mjs @@ -67,8 +67,10 @@ export const SHELL_RULES = [ [ // --no-verify, or an abbreviation git accepts, on any git command String.raw`\bgit\b[^\n;&|]*\s--no-veri[a-z]*\b`, - // git commit -n (alone or in a cluster of short flags), after any global options - String.raw`\bgit(?:\s+(?:-[Cc]\s+\S+|-{1,2}[A-Za-z][\w-]*(?:=\S+)?))*\s+commit\b[^\n;&|]*\s-[a-z]*n[a-z]*(?=[\s;&|<>()'"]|$)`, + // git commit -n (alone or in a cluster of short flags), after any global options. + // A bare -c or -C takes the next word; the other branch never reads them, so + // each option has one reading and the check stays linear. + String.raw`\bgit(?:\s+(?:-[Cc]\s+\S+|-{1,2}(?![Cc](?:\s|$))[A-Za-z][\w-]*(?:=\S+)?))*\s+commit\b[^\n;&|]*\s-[a-z]*n[a-z]*(?=[\s;&|<>()'"]|$)`, // another hooks directory, given on the command line or in the configuration String.raw`\bgit\b[^\n;&|]*\s-c\s*['"]?core\.hookspath\b`, String.raw`\bgit\b[^\n;&|]*\bconfig\b[^\n;&|]*[\s'"]core\.hookspath\b`, diff --git a/src/runtime/lib/task.mjs b/src/runtime/lib/task.mjs index 7473873..fbed73b 100644 --- a/src/runtime/lib/task.mjs +++ b/src/runtime/lib/task.mjs @@ -4,7 +4,7 @@ import { realpathSync } from 'node:fs' import { appendFile, mkdir, readdir, rm } from 'node:fs/promises' import path from 'node:path' import { readJsonOrNull, readTextOrNull, stableJson, writeFileAtomic } from './fs.mjs' -import { GIT_BUFFER, generatedPaths, git, head, shortSha, uncommittedPaths, workingTreeDiff } from './git.mjs' +import { existsAt, GIT_BUFFER, generatedPaths, git, head, shortSha, uncommittedPaths, workingTreeDiff } from './git.mjs' import { auditTask } from './audit.mjs' import { isProtectedTest, loadConfig, tierAtLeast, tierForRoute } from './config.mjs' import { readExtension, sectionOf } from './extensions.mjs' @@ -70,18 +70,28 @@ export async function appendProgress(projectDir, id, text) { await appendFile(path.join(taskDir(projectDir, id), 'progress.md'), line) } -// The most recently updated task of the worktree, or null. +// The most recently updated task of the worktree, or null. A tasks folder or a +// task state that cannot be read is skipped with a warning on standard error, so +// the human learns that a blocked task there was not checked. async function latestTask(projectDir) { + const warn = (what, error) => process.stderr.write(`paacm: warning: could not read ${what}, so a blocked task there is not checked: ${error.message}\n`) let latest = null let entries = [] try { entries = await readdir(tasksDir(projectDir), { withFileTypes: true }) - } catch { + } catch (error) { + if (error.code !== 'ENOENT' && error.code !== 'ENOTDIR') warn(tasksDir(projectDir), error) return null } for (const entry of entries) { if (!entry.isDirectory()) continue - const state = await readJsonOrNull(path.join(tasksDir(projectDir), entry.name, 'state.json')).catch(() => null) + const file = path.join(tasksDir(projectDir), entry.name, 'state.json') + let state = null + try { + state = await readJsonOrNull(file) + } catch (error) { + warn(file, error) + } if (state && state.updatedAt && (!latest || state.updatedAt > latest.updatedAt)) latest = state } return latest @@ -113,14 +123,19 @@ function otherWorktrees(projectDir) { // the change is settled in the blocked task. The tasks checked are the latest one // of this worktree (with `own`) and the latest one of each other worktree whose // approved test commit this HEAD contains: the refusal follows the commit, not -// one .agent/tasks folder. +// one .agent/tasks folder. For another worktree's task only a file that existed +// at the approval counts: a test added later was never frozen by it. A change +// that arrived from the base branch still counts, since git cannot tell a +// reviewed change from one the writer made a remote-tracking ref for; the +// refusal names the blocked task and its worktree, where the human settles it. async function refuseOverBlockedTask(projectDir, config, { own, before }) { const dirs = [...(own ? [projectDir] : []), ...otherWorktrees(projectDir)] for (const dir of dirs) { const latest = await latestTask(dir) if (!latest || latest.status !== 'blocked' || !latest.testsApprovedAt) continue if (dir !== projectDir && !git(projectDir, ['merge-base', '--is-ancestor', latest.testsApprovedAt, 'HEAD'], { allowFail: true }).ok) continue - const changed = auditTask(projectDir, config, latest).violations.map((v) => v.path) + let changed = auditTask(projectDir, config, latest).violations.map((v) => v.path) + if (dir !== projectDir) changed = changed.filter((p) => existsAt(projectDir, latest.testsApprovedAt, p)) if (!changed.length) continue const where = dir === projectDir ? '' : ` (in the worktree at ${dir})` throw new TaskError( @@ -508,6 +523,9 @@ export async function writeReviewInput(projectDir, config) { '', entries.length ? entries.map((e) => `- ${e.path} (${stats(e)})`).join('\n') : '(none)', ...(change.skipped.length ? ['', 'These paths could not be added to the review, so their changes are not in it:', ...change.skipped.map((m) => `- ${m}`)] : []), + ...(change.withheld.length + ? ['', 'These untracked files look like keys or secrets, so their content is not in this review. Add them to .gitignore, or commit them if they belong to the change:', ...change.withheld.map((p) => `- ${p}`)] + : []), '', ...(checklist ? ['## Project reviewer checklist', '', 'Check each item against the diff as well, and report a violated item as a finding.', '', checklist, ''] : []), ...(separate.length diff --git a/test/review-fixes.test.mjs b/test/review-fixes.test.mjs new file mode 100644 index 0000000..8fa4573 --- /dev/null +++ b/test/review-fixes.test.mjs @@ -0,0 +1,266 @@ +// Release 3.5.1: fixes for what the reviewers of a project's upgrade to 3.5.0 +// found in the runtime. Each test failed before its fix. +import assert from 'node:assert/strict' +import { mkdir, readFile, symlink } from 'node:fs/promises' +import path from 'node:path' +import { performance } from 'node:perf_hooks' +import { test } from 'node:test' +import { worktreeFingerprint } from '../src/runtime/lib/gate.mjs' +import { isMissingTool } from '../src/runtime/lib/runner.mjs' +import { evaluateCommand } from '../src/runtime/lib/shell-rules.mjs' +import { commitAll, git, hook, read, readOrNull, repo, runtime, stopGuidance, tempDir, writeFiles } from './helpers.mjs' + +const pass = 'node -e "process.exit(0)"' +const fail = 'node -e "process.exit(1)"' +const files = { '.agent/checks.conf': `quick|unit|${pass}\n`, 'src/a.js': 'a\n', 'test/a.test.js': 'ok\n' } +const posixOnly = { skip: process.platform === 'win32' ? 'POSIX shell messages and PATH' : false } + +function approvedTask(dir, slug, route = 'STANDARD') { + assert.equal(runtime(dir, 'task', 'start', '--slug', slug, '--route', route, '--summary', 'x').code, 0) + assert.equal(runtime(dir, 'task', 'approve-tests').code, 0) + assert.equal(runtime(dir, 'task', 'phase', 'implementing').code, 0) +} +const currentId = async (dir) => (await read(dir, '.agent/tasks/CURRENT')).trim() +const stateOf = async (dir) => JSON.parse(await read(dir, `.agent/tasks/${await currentId(dir)}/state.json`)) +async function editState(dir, change) { + const state = await stateOf(dir) + change(state) + await writeFiles(dir, { [`.agent/tasks/${state.id}/state.json`]: `${JSON.stringify(state, null, 2)}\n` }) +} +const minuteAgo = () => new Date(Date.now() - 60_000).toISOString() +const stop = (dir, extra = {}) => hook('stop-gate', { hook_event_name: 'Stop', stop_hook_active: false, ...extra }, { projectDir: dir }) + +test('stop: an invalid .agent/paacm.json is reported as a gate that cannot verify the task, not a hook error', async (t) => { + const dir = await repo(t, { files }) + approvedTask(dir, 'cfg') + await writeFiles(dir, { '.agent/paacm.json': '{ "gate": { "tier": "fast" } }\n' }) + const r = stop(dir) + assert.equal(r.code, 0, r.out) + assert.match(stopGuidance(r), /could not verify this task[\s\S]*\.agent\/paacm\.json/) +}) + +test("stop: one project's error does not release another project's failing gate", async (t) => { + const dir = await repo(t, { files }) + approvedTask(dir, 'main') + await writeFiles(dir, { '.agent/paacm.json': '{ "gate": { "tier": "fast" } }\n' }) + const sibling = path.join(tempDir(t), 'wt') + git(dir, 'worktree', 'add', '-q', '-b', 's', sibling) + await writeFiles(sibling, { '.agent/checks.conf': `quick|unit|${fail}\n` }) + commitAll(sibling, 'a failing check') + approvedTask(sibling, 'sibling') + const r = stop(dir, { cwd: sibling }) + assert.equal(r.json?.decision, 'block', r.out) + assert.match(r.json.reason, /gate failed/) +}) + +test('gate: a gate.log that cannot be written leaves the result and the exit code intact', async (t) => { + const dir = await repo(t, { files }) + approvedTask(dir, 'log') + await mkdir(path.join(dir, '.agent', 'tasks', await currentId(dir), 'gate.log'), { recursive: true }) + const r = runtime(dir, 'gate') + assert.equal(r.code, 0, r.out) + assert.match(r.out, /WARN could not save .*gate\.log/) + assert.match(r.stdout, /paacm gate: exit 0/) + assert.equal((await stateOf(dir)).lastGate.passed, true) +}) + +test('gate: a script the shell cannot find in the repository is an ordinary failure, not a missing tool', posixOnly, async (t) => { + const dir = await repo(t, { files: { ...files, '.agent/checks.conf': 'quick|unit|./scripts/missing.sh\n' } }) + approvedTask(dir, 'path') + const r = runtime(dir, 'gate') + assert.equal(r.code, 1, r.out) + assert.doesNotMatch(r.out, /This is the environment, not the code/) + assert.equal((await stateOf(dir)).lastGate.missingTool, undefined) +}) + +test('gate: a program that is not installed is still reported as a missing tool', async (t) => { + const dir = await repo(t, { files: { ...files, '.agent/checks.conf': 'quick|unit|paacm-no-such-program-3-5-1\n' } }) + approvedTask(dir, 'prog') + const r = runtime(dir, 'gate') + assert.equal(r.code, 1, r.out) + assert.match(r.out, /a command was not found/) + assert.equal((await stateOf(dir)).lastGate.missingTool, true) +}) + +test('protect: when git cannot run, an edit in a sibling worktree is refused, not allowed', posixOnly, async (t) => { + const dir = await repo(t, { files }) + const sibling = path.join(tempDir(t), 'wt') + git(dir, 'worktree', 'add', '-q', '-b', 's', sibling) + approvedTask(sibling, 'sibling') + const r = hook('protect', { hook_event_name: 'PreToolUse', tool_name: 'Edit', tool_input: { file_path: path.join(sibling, 'test', 'a.test.js') } }, { projectDir: dir, env: { PATH: tempDir(t) } }) + assert.notEqual(r.code, 0, `a hook error, which the fail-closed hook turns into a refusal: ${r.out}`) + assert.match(r.stderr, /git could not tell/) +}) + +test('stop: a continuation reuses a passing gate for the same uncommitted tree, however old it is', async (t) => { + const counter = path.join(tempDir(t), 'runs.txt').split(path.sep).join('/') + const counted = `node -e "require('fs').appendFileSync('${counter}', 'x')"` + const dir = await repo(t, { files: { ...files, '.agent/checks.conf': `quick|unit|${counted}\n` } }) + approvedTask(dir, 'again') + await writeFiles(dir, { 'src/a.js': 'changed\n' }) + assert.equal(runtime(dir, 'gate').code, 0) + const runs = async () => (await readFile(counter, 'utf8')).length + assert.equal(await runs(), 1) + await editState(dir, (s) => { + s.lastGate.at = minuteAgo() + }) + const r = stop(dir, { stop_hook_active: true }) + assert.equal(r.code, 0, r.out) + assert.equal(await runs(), 1, 'the gate did not run again') +}) + +test("gate: the working tree's fingerprint changes when an untracked file's content changes", async (t) => { + const dir = await repo(t, { files }) + await writeFiles(dir, { 'notes.txt': 'a\n' }) + const before = worktreeFingerprint(dir) + await writeFiles(dir, { 'notes.txt': 'a longer note\n' }) + assert.notEqual(worktreeFingerprint(dir), before) +}) + +test('stop: the finish nudge is given once for the same state, not again after a pause', async (t) => { + const dir = await repo(t, { files }) + approvedTask(dir, 'nudge') + runtime(dir, 'task', 'phase', 'reviewing') + assert.equal(runtime(dir, 'gate').code, 0) + assert.match(stopGuidance(stop(dir)), /the gate is green/) + await editState(dir, (s) => { + s.lastNudge.at = minuteAgo() + }) + const again = stop(dir) + assert.equal(again.code, 0, again.out) + assert.equal(again.json, null, 'no second nudge for the same state') +}) + +test('shell guard: global options before a git command are checked in linear time', () => { + const started = performance.now() + evaluateCommand(`git${' -c'.repeat(36)} status`) + const ms = performance.now() - started + assert.ok(ms < 250, `took ${ms.toFixed(0)} ms`) + assert.equal(evaluateCommand(`git${' -c x=1'.repeat(30)} commit -n -m x`)?.id, 'skip-git-hooks') + for (const command of ['git commit -n -m x', 'git -c core.x=1 commit -an', 'git -C /repo commit -nm msg', 'git --no-pager commit -n']) { + assert.equal(evaluateCommand(command)?.id, 'skip-git-hooks', command) + } + for (const command of ['git commit -m x', 'git -C /r log -n 5']) assert.notEqual(evaluateCommand(command)?.id, 'skip-git-hooks', command) +}) + +test('a corrupt state.json in another worktree is reported, not silently skipped', async (t) => { + const dir = await repo(t, { files }) + const sibling = path.join(tempDir(t), 'wt') + git(dir, 'worktree', 'add', '-q', '-b', 's', sibling) + approvedTask(sibling, 'broken') + await writeFiles(sibling, { [`.agent/tasks/${await currentId(sibling)}/state.json`]: '{\n' }) + const r = runtime(dir, 'task', 'start', '--slug', 'next', '--route', 'FAST', '--summary', 'x') + assert.equal(r.code, 0, r.out) + assert.match(r.stderr, /could not read [^\n]*state\.json/) +}) + +test('review input withholds the content of untracked files that look like keys or secrets', async (t) => { + const dir = await repo(t, { files }) + approvedTask(dir, 'secrets') + await writeFiles(dir, { + 'notes.txt': 'plain-note-marker\n', + id_ed25519: 'ssh-key-marker\n', + 'config/.env': 'env-marker\n', + 'certs/server.pem': 'pem-marker\n', + 'certs/server.key': 'key-marker\n', + }) + const r = runtime(dir, 'task', 'review-input') + assert.equal(r.code, 0, r.out) + const id = await currentId(dir) + let text = await read(dir, `.agent/tasks/${id}/review-input.md`) + for (const n of ['001', '002', '003', '004', '005']) text += (await readOrNull(dir, `.agent/tasks/${id}/review-diffs/${n}.diff`)) ?? '' + assert.match(text, /plain-note-marker/) + for (const marker of ['ssh-key-marker', 'env-marker', 'pem-marker', 'key-marker']) assert.doesNotMatch(text, new RegExp(marker)) + for (const name of ['id_ed25519', 'config/.env', 'certs/server.pem', 'certs/server.key']) assert.ok(text.includes(name), `${name} is listed by name`) +}) + +test('a blocked task in another worktree is not held against a test file added after its approval', async (t) => { + const dir = await repo(t, { files }) + approvedTask(dir, 'held') + assert.equal(runtime(dir, 'task', 'block', '--trigger', '2', '--reason', 'a tool is missing').code, 0) + const base = git(dir, 'rev-parse', 'HEAD').trim() + + const added = path.join(tempDir(t), 'added') + git(dir, 'worktree', 'add', '-q', '-b', 'added', added, base) + await writeFiles(added, { 'test/b.test.js': 'a new test\n' }) + commitAll(added, 'a new test') + const started = runtime(added, 'task', 'start', '--slug', 'added', '--route', 'FAST', '--summary', 'x') + assert.equal(started.code, 0, started.out) + + const edited = path.join(tempDir(t), 'edited') + git(dir, 'worktree', 'add', '-q', '-b', 'edited', edited, base) + await writeFiles(edited, { 'test/a.test.js': 'weakened\n', 'test/c.test.js': 'another new test\n' }) + commitAll(edited, 'edit the approved test and add one') + const refused = runtime(edited, 'task', 'start', '--slug', 'edited', '--route', 'FAST', '--summary', 'x') + assert.equal(refused.code, 1, refused.out) + assert.match(refused.stderr, /\(in the worktree at [^)]+\) is blocked[^\n]*changed since: test\/a\.test\.js\./) +}) + +// Found by a fresh-context review of the 3.5.1 fixes themselves. + +test('stop: an error on a continuation lets the stop through, but as a hook error the human sees', async (t) => { + const dir = await repo(t, { files: { ...files, '.agent/checks.conf': `quick|unit|${fail}\n` } }) + approvedTask(dir, 'loop') + assert.equal(stop(dir).json?.decision, 'block') + await writeFiles(dir, { [`.agent/tasks/${await currentId(dir)}/state.json`]: '{\n' }) + const r = stop(dir, { stop_hook_active: true }) + assert.equal(r.code, 1, r.out) + assert.match(r.stderr, /PAACM stop gate could not check/) +}) + +test('gate: an untracked symbolic link loop neither breaks the fingerprint nor loses the result', { skip: process.platform === 'win32' ? 'symbolic links need privileges on Windows' : false }, async (t) => { + const dir = await repo(t, { files }) + approvedTask(dir, 'link') + await symlink('loop', path.join(dir, 'loop')) + assert.doesNotThrow(() => worktreeFingerprint(dir)) + const r = runtime(dir, 'gate') + assert.equal(r.code, 0, r.out) + assert.equal((await stateOf(dir)).lastGate.passed, true) +}) + +test('gate: a missing program named by a path outside the project, or after an unrelated message, is a missing tool', () => { + const project = path.resolve('/srv/project') + const result = (outputTail) => ({ exitCode: 127, outputTail }) + assert.equal(isMissingTool(result('/bin/sh: 1: /opt/tools/bin/golangci-lint: not found\n'), project), true) + assert.equal(isMissingTool(result('cat: config/local.json: No such file or directory\n/bin/sh: 1: paacm-no-such-linter: not found\n'), project), true) + assert.equal(isMissingTool(result('/bin/sh: 1: ./scripts/missing.sh: not found\n'), project), false) + assert.equal(isMissingTool(result('sh: scripts/missing.sh: No such file or directory\n'), project), false) +}) + +test('review input withholds secret-named untracked files anywhere in the repository when the project is a sub-folder', async (t) => { + const dir = await repo(t, { files: { 'pkg/.agent/paacm.json': '{}\n', 'pkg/.agent/checks.conf': `quick|unit|${pass}\n`, 'pkg/src/a.js': 'a\n' } }) + const pkg = path.join(dir, 'pkg') + approvedTask(pkg, 'sub') + await writeFiles(dir, { '.env': 'root-secret-marker\n', 'other/id_rsa': 'other-key-marker\n', 'pkg/.env': 'pkg-secret-marker\n' }) + const r = runtime(pkg, 'task', 'review-input') + assert.equal(r.code, 0, r.out) + const text = await read(pkg, `.agent/tasks/${await currentId(pkg)}/review-input.md`) + for (const marker of ['root-secret-marker', 'other-key-marker', 'pkg-secret-marker']) assert.doesNotMatch(text, new RegExp(marker)) + for (const name of ['- .env', '- other/id_rsa', '- pkg/.env']) assert.ok(text.includes(name), `${name} is listed`) +}) + +test("protect: the refusal when git cannot run names the folder and git's own message", posixOnly, async (t) => { + const dir = await repo(t, { files }) + const sibling = path.join(tempDir(t), 'wt') + git(dir, 'worktree', 'add', '-q', '-b', 's', sibling) + const r = hook('protect', { hook_event_name: 'PreToolUse', tool_name: 'Edit', tool_input: { file_path: path.join(sibling, 'src', 'a.js') } }, { projectDir: dir, env: { PATH: tempDir(t) } }) + assert.notEqual(r.code, 0, r.out) + assert.match(r.stderr, /failed in [^:]+: [^\n]*ENOENT/) +}) + +test('stop: a continuation runs a failing gate again instead of reusing the old failure', async (t) => { + const flag = path.join(tempDir(t), 'ready').split(path.sep).join('/') + const check = `node -e "process.exit(require('fs').existsSync('${flag}') ? 0 : 1)"` + const dir = await repo(t, { files: { ...files, '.agent/checks.conf': `quick|unit|${check}\n` } }) + approvedTask(dir, 'flaky') + await writeFiles(dir, { 'src/a.js': 'changed\n' }) + assert.equal(stop(dir).json?.decision, 'block', 'the gate fails while the flag is missing') + await writeFiles(path.dirname(flag), { ready: 'yes\n' }) + await editState(dir, (s) => { + s.lastGate.at = minuteAgo() + }) + const r = stop(dir, { stop_hook_active: true }) + assert.equal(r.code, 0, r.out) + assert.equal(r.json?.decision, undefined, `the gate ran again and passed: ${r.out}`) + assert.equal((await stateOf(dir)).lastGate.passed, true) +})