fix(gate): part 2 verifier findings — non-object findings, carried backed copy, rule_keys, marks, approval prose, brief_path, engine put-back once
1. plan-review-dedup skips only a null or undefined finding; any other entry that is not an object (a string, a number, an array) goes on as before E, a finding with no valid severity that the plan gate counts as a blocker (it was dropped, so a string blocker recorded PASS). 2. plan-gate --record: a previous finding that rule A carried as counted and whose id is backed this round leaves the counted set, the counts, the verdict and the stop rule's summary (it was counted and listed in known_outside at once: BLOCK, not-fewer, and --accept-open refused). 3. A known_outside entry keeps rule_keys; extractVerdict accepts it (a list of strings), and the bare gate re-checks with the same keys the recorder used, so a merged finding whose security key is only in rule_keys is a WARN, not a STOP. 4. plan-critic, scope-guardian and /trekplan Phase 9's round-2 sentences say the Critic Verdict also lists known_outside and each is marked by id in previous. Code already accepts such a mark (dedup passes it; countRound ignores ids it does not carry; --record removes the entry on true); pinned in diff mode and full count. 5. The approval prose names ## Threat Model where it named only ## Intent + ## Goal: skills/annotating/SKILL.md (description, step 4, guard list), docs/command-modes.md, Handover 1's intent_approved_hash row, README (approval, Approve row, intent approval gate) and CLAUDE.md's /trekbrief row. 6. threat-model.mjs gains usableInRepo (a brief path relative to the repo top level) and repoRelativeBrief; --record stores brief_path relative to the top level, never absolute, and both gates read it from there, so the gate run from a subdirectory reads the same brief. reviewThreatModel uses usableInRepo. Handover 4 and the gate header say so. 7. review-finalize's outside pass puts an entry back only when no finding carries its row yet (engine-compare rowOf): an engine finding listed in known_outside while its payload is saved went back twice. 8. A test runs scripts/release-check.mjs on a review whose known_outside entry the brief does not back: exit 2 (control: backed passes the review check). Existing tests changed: plan-gate-threat-model's entry-keys assertion now expects rule_keys (item 3), and its planted non-security entry drops the security key from rule_keys too, since the gate reads rule_keys (item 3). Part 2 fix round of the E plan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
c47061191c
commit
1be961aab1
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
17 changed files with 298 additions and 52 deletions
|
|
@ -30,7 +30,7 @@ Voyage — a contract-driven Claude Code pipeline: brief, research, plan, execut
|
|||
|
||||
| Command | Description | Orchestrator model |
|
||||
|---------|-------------|--------------------|
|
||||
| `/trekbrief` | Brief — one question round sized by class (`ceremony: small\|bounded\|architectural`, heaviest in doubt, said aloud, only moved up), run in the form of `voyage:interviewing` (asked for by name; criteria by `voyage:writing-criteria`, which `brief-reviewer` is also told to judge by, with `voyage:reviewing-adversarially`), produces a task brief with explicit research plan; ends with the operator explicitly approving `## Intent` + `## Goal` ("sounds good" is not approval), logged in the target repo's hash-chained approval log and committed (the brief too when `artifact_store: git`); the operator's notes from the annotation page go through `voyage:annotating` (`--approve <dir>` does only the approval); optionally orchestrates the pipeline | session |
|
||||
| `/trekbrief` | Brief — one question round sized by class (`ceremony: small\|bounded\|architectural`, heaviest in doubt, said aloud, only moved up), run in the form of `voyage:interviewing` (asked for by name; criteria by `voyage:writing-criteria`, which `brief-reviewer` is also told to judge by, with `voyage:reviewing-adversarially`), produces a task brief with explicit research plan; ends with the operator explicitly approving `## Intent` + `## Goal` (+ `## Threat Model` when present; "sounds good" is not approval), logged in the target repo's hash-chained approval log and committed (the brief too when `artifact_store: git`); the operator's notes from the annotation page go through `voyage:annotating` (`--approve <dir>` does only the approval); optionally orchestrates the pipeline | session |
|
||||
| `/trekresearch` | Research — deep local + external research, produces structured research brief. The external agents preload the `researching-sources` skill and are told it by name at spawn | session |
|
||||
| `/trekplan` | Plan — brief-reviewer, explore, plan, review. Requires `--brief` or `--project`; halts unless the brief's intent is approved (hash-bound, stale on edit). Auto-discovers `architecture/overview.md` if present. Plan 1.9: the file first on every line — every `Files:` entry and `Test first:` `File:` line opens with its backticked path(s). From 1.8: `Test first:` per step + one `End-to-end:` step; every check written by `voyage:writing-criteria` (named where the plan is written and where its three reviewers start); a design part for `ceremony: architectural` (absent = architectural); critic loop of max 3 rounds, recorded in the plan, rounds 2+ in diff mode (rule A, counted by the gate; rule B in the reviewers), and `plan-gate.mjs` (0 go / 1 stop / 2 could not run) decides; open minors after it are listed for the operator to pick | session |
|
||||
| `/trekexecute` | Execute — disciplined plan/session-spec executor: refuses a plan whose logged approval does not cover its current sha256 (K2); one fresh `step-implementer` helper per plan step, one controller that reads the diff (spec + quality verdicts), proof of red / floor guard / step check (K9) / `Regression:` (K10) / done line as commands, fix rounds that go up (max 5: helper → plan → operator), architecture rules (K5) + plan-file sweep (K6) after the last step, the app seen with Claude Code's built-in `run` while the `End-to-end:` command stays the verdict (`run` gives no exit code), then its own review (`/simplify` → `/trekreview` → verdict gate, K7; BLOCK keeps the merge into main closed), closed stop list + decision log in the final report | session |
|
||||
|
|
|
|||
|
|
@ -159,7 +159,7 @@ Success criteria are proposed in the form of `voyage:writing-criteria`, so the r
|
|||
|
||||
Then `brief-reviewer` scores the draft on six dimensions (completeness, consistency, testability, scope clarity, research-plan validity, memory alignment) and gates publication, up to 1 / 2 / 3 review iterations by class; force-stop yields a `brief_quality: partial` brief with the failing dimensions documented.
|
||||
|
||||
**Approval, logged and committed.** Every run ends with you approving `## Intent` + `## Goal` (only an explicit "Approve" counts). The approval is appended to the target repo's hash-chained approval log, `.claude/voyage/approvals.jsonl`, bound to the intent's sha256, and committed. The log is always committed; the brief only when you chose "in git" for this task (`artifact_store: git`). "Only on this machine" keeps the project folder out of git via `.git/info/exclude`. The log is a trace, not a signature: any session on your machine could write it, and the commit is what makes it visible.
|
||||
**Approval, logged and committed.** Every run ends with you approving `## Intent` + `## Goal` (+ `## Threat Model` when the brief has one; only an explicit "Approve" counts). The approval is appended to the target repo's hash-chained approval log, `.claude/voyage/approvals.jsonl`, bound to the intent's sha256, and committed. The log is always committed; the brief only when you chose "in git" for this task (`artifact_store: git`). "Only on this machine" keeps the project folder out of git via `.git/info/exclude`. The log is a trace, not a signature: any session on your machine could write it, and the commit is what makes it visible.
|
||||
|
||||
Output: `.claude/projects/{YYYY-MM-DD}-{slug}/brief.md`
|
||||
|
||||
|
|
@ -171,7 +171,7 @@ Borrowed, with credit: size classes and the ratchet from obra/superpowers (MIT),
|
|||
|------|-------|----------|
|
||||
| **Default** | `/trekbrief <task>` | Question round sized by class, then review and approval. |
|
||||
| **Quick** | `/trekbrief --quick <task>` | Proposes `small` as the class; still said aloud, overridable, and raised if the task proves bigger. |
|
||||
| **Approve** | `/trekbrief --approve <project-dir \| brief-file>` | No interview: show an existing brief's `## Intent` + `## Goal` verbatim and ask the operator to approve them. The same step (Phase 4h) ends every normal `/trekbrief` run. |
|
||||
| **Approve** | `/trekbrief --approve <project-dir \| brief-file>` | No interview: show an existing brief's `## Intent` + `## Goal` (+ `## Threat Model` when present) verbatim and ask the operator to approve them. The same step (Phase 4h) ends every normal `/trekbrief` run. |
|
||||
|
||||
`/trekbrief` is **always interactive**. There is no foreground/background mode — the interview requires user input.
|
||||
|
||||
|
|
@ -269,7 +269,7 @@ Output:
|
|||
|
||||
`--brief` or `--project` is **required**. `/trekplan` with no brief exits with an error and a pointer to `/trekbrief`.
|
||||
|
||||
**Intent approval gate.** `/trekplan` halts before planning anything unless the brief's `## Intent` + `## Goal` carry the operator's approval. `/trekbrief` asks for it at the end of every interview (Phase 4h, also in `--quick`), or run `/trekbrief --approve <project-dir | brief-file>` later. The approval is a hash over those two sections: edit either one afterwards and `/trekplan` halts again until the new text is approved. Review briefs (`type: trekreview`) are exempt. The marker is a trace that the approval step ran, **not a signature**. Any session on the same machine could write it.
|
||||
**Intent approval gate.** `/trekplan` halts before planning anything unless the brief's `## Intent` + `## Goal` (+ `## Threat Model` when present) carry the operator's approval. `/trekbrief` asks for it at the end of every interview (Phase 4h, also in `--quick`), or run `/trekbrief --approve <project-dir | brief-file>` later. The approval is a hash over those sections: edit any of them afterwards and `/trekplan` halts again until the new text is approved. Review briefs (`type: trekreview`) are exempt. The marker is a trace that the approval step ran, **not a signature**. Any session on the same machine could write it.
|
||||
|
||||
### What the plan contains
|
||||
|
||||
|
|
|
|||
|
|
@ -309,7 +309,9 @@ up to 3 times on successive revisions of the same plan. From round 2 the
|
|||
prompt gives you the diff since the last reviewed text, and the plan's
|
||||
`## Critic Verdict` lists the previous findings. List every previous finding
|
||||
by id in `previous`, with `resolved: true` only when the plan now fixes it.
|
||||
Review the changed text. Cite the exact line, with `line_end` for a range.
|
||||
The `## Critic Verdict` also lists `known_outside`, the security findings the
|
||||
brief's threat model puts outside. Mark each of them by id in `previous` the
|
||||
same way. Review the changed text. Cite the exact line, with `line_end` for a range.
|
||||
|
||||
Dimensions 11 and 12 are scored inside the existing weights: 11 under Step
|
||||
quality, 12 under Coverage completeness and Specification quality.
|
||||
|
|
@ -408,7 +410,8 @@ instance hides a class that keeps coming back.
|
|||
`severity` is exactly one of `blocker`, `major`, `minor`. Emit
|
||||
`"findings": []` when the plan is clean.
|
||||
`previous` (from round 2; omit it in round 1) holds one `{id, resolved}` per
|
||||
finding the current `## Critic Verdict` lists; `resolved` is `true` or
|
||||
finding the current `## Critic Verdict` lists, its `known_outside` entries
|
||||
included; `resolved` is `true` or
|
||||
`false`. `line_end` (optional) is the last line of a cited range, an integer
|
||||
≥ `line`. `lowered_from` and `caught_by` go only on a finding lowered by rule
|
||||
B: `"lowered_from": "major"`, and `caught_by` names the test, `Regression:`
|
||||
|
|
|
|||
|
|
@ -127,7 +127,9 @@ You may be run up to 3 times on successive revisions of the same plan. From
|
|||
round 2 the prompt gives you the diff since the last reviewed text, and the
|
||||
plan's `## Critic Verdict` lists the previous findings. List every previous
|
||||
finding by id in `previous`, with `resolved: true` only when the plan now
|
||||
fixes it. Review the changed text. Cite the exact line, with `line_end` for a
|
||||
fixes it. The `## Critic Verdict` also lists `known_outside`, the security
|
||||
findings the brief's threat model puts outside. Mark each of them by id in
|
||||
`previous` the same way. Review the changed text. Cite the exact line, with `line_end` for a
|
||||
range.
|
||||
|
||||
## Output format
|
||||
|
|
@ -197,7 +199,8 @@ by class (the catalogue key, else the part before the first `/`), so a key
|
|||
made for one instance hides a class that keeps coming back.
|
||||
Emit `"findings": []` when the plan is fully aligned.
|
||||
`previous` (from round 2; omit it in round 1) holds one `{id, resolved}` per
|
||||
finding the current `## Critic Verdict` lists; `resolved` is `true` or
|
||||
finding the current `## Critic Verdict` lists, its `known_outside` entries
|
||||
included; `resolved` is `true` or
|
||||
`false`. `line_end` (optional) is the last line of a cited range, an integer
|
||||
≥ `line`. `lowered_from` and `caught_by` go only on a finding lowered by rule
|
||||
B: `"lowered_from": "major"`, and `caught_by` names the test, `Regression:`
|
||||
|
|
|
|||
|
|
@ -990,7 +990,7 @@ End your response with the REQUIRED machine-readable `json` findings block
|
|||
dedup helper. You have no Write tool — return the block inline, do not write a file.
|
||||
Read local files only inside the repo under exploration, plus paths this prompt gives you (the brief, the plan, and templates it names)."
|
||||
From round 2 the prompt adds one sentence, followed by the `--diff` output:
|
||||
"Previous findings are in the plan's `## Critic Verdict`; the diff since the last reviewed text is below. Mark each previous finding by id in `previous`."
|
||||
"Previous findings are in the plan's `## Critic Verdict`, its `known_outside` entries included; the diff since the last reviewed text is below. Mark each previous finding by id in `previous`."
|
||||
|
||||
**scope-guardian** — scope alignment check.
|
||||
Prompt: "Check this implementation plan against the brief.
|
||||
|
|
@ -1006,7 +1006,7 @@ End your response with the REQUIRED machine-readable
|
|||
tool — return the block inline, do not write a file.
|
||||
Read local files only inside the repo under exploration, plus paths this prompt gives you (the brief, the plan, and templates it names)."
|
||||
From round 2 the prompt adds one sentence, followed by the `--diff` output:
|
||||
"Previous findings are in the plan's `## Critic Verdict`; the diff since the last reviewed text is below. Mark each previous finding by id in `previous`."
|
||||
"Previous findings are in the plan's `## Critic Verdict`, its `known_outside` entries included; the diff since the last reviewed text is below. Mark each previous finding by id in `previous`."
|
||||
|
||||
After both complete, **extract each reviewer's trailing `json` findings block**
|
||||
and pipe both into the dedup helper via **stdin** — the reviewers are read-only
|
||||
|
|
|
|||
|
|
@ -72,7 +72,7 @@ Every validator exposes a CLI: `node lib/validators/<name>.mjs --json <path>` re
|
|||
| `source` | string | optional | `interview \| manual` | |
|
||||
| `brief_quality` | string | optional | `complete \| partial` | Set when iteration cap is hit |
|
||||
| `framing` | string | **required at ≥ 2.2** (v5.5) | `preserve \| refine \| replace \| new-direction` | How this brief relates to prior operator intent. Enum-checked on any version when present; missing → `BRIEF_MISSING_FRAMING` at `brief_version ≥ 2.2`. Layer 1 of the framing-alignment defense. |
|
||||
| `intent_approved_hash` | string | optional in the schema; **required by `/trekplan`** (veikart steg 1) | `"sha256:<64 hex>"` | Hash over the normalized `## Intent` + `## Goal`. Written only by `lib/validators/intent-approval.mjs --stamp`, which `/trekbrief` runs on the operator's explicit "Approve" (Phase 4h, or `--approve <project-dir \| brief-file>`). |
|
||||
| `intent_approved_hash` | string | optional in the schema; **required by `/trekplan`** (veikart steg 1) | `"sha256:<64 hex>"` | Hash over the normalized `## Intent` + `## Goal` (+ `## Threat Model` when present). Written only by `lib/validators/intent-approval.mjs --stamp`, which `/trekbrief` runs on the operator's explicit "Approve" (Phase 4h, or `--approve <project-dir \| brief-file>`). |
|
||||
| `intent_approved_at` | string | optional | ISO-8601 | When the stamp was written. Informational; the gate reads the hash. |
|
||||
| `brief_reviewed_hash` / `brief_reviewed_at` | string | optional, additive (no `brief_version` bump) | `"sha256:<64 hex>"` / ISO-8601 | Hash over the brief's body and the frontmatter the reviewer judges (everything but this stamp and the intent marker), written by `lib/validators/brief-review-stamp.mjs --stamp` when `/trekbrief`'s brief-reviewer gate passes. `/trekplan` Phase 4b skips its own brief review only while the hash is current; absent (any other producer) or stale (body or reviewed frontmatter such as `framing` changed) means it reviews. A trace, not a signature. |
|
||||
| `ceremony` | string | optional (K1) | `small \| bounded \| architectural` | The brief's size class: sets `/trekbrief`'s question budget and review cap, and what downstream phases scale to. **Absent (or unreadable) = `architectural`**, the heaviest, so another producer's brief is never treated lighter by accident. Enum-checked when present (`BRIEF_INVALID_CEREMONY`). Read it through `briefCeremony(fm)` in `brief-validator.mjs`. |
|
||||
|
|
@ -253,7 +253,7 @@ prose form (relaxed in S26 so the template's prose emission parses).
|
|||
- at least one step has `End-to-end:` with a backticked command — the "run the thing" step;
|
||||
- the optional step fields `Interface in:`, `Interface out:`, `Review focus:` are never empty when present (consumers tolerate their absence);
|
||||
- `ceremony` is one of the three classes; an `architectural` plan has `## Design` with the subsections Architecture, Data Flow, Interfaces, Data Models, Error Handling, Module Map, Architecture Rules, Requirements Check, plus `## Test Strategy`; the module map (mermaid) has no cycle; every architecture rule says "checked by `<command>`" (with a Proof column naming an existing step) or "advisory — `.claude/rules/<file>.md`".
|
||||
6. **`## Critic Verdict`** (written by `lib/validators/plan-gate.mjs --record`, never by hand): one fenced `json` block, schema `voyage.plan-verdict/1` — `round`, `max_rounds` (3), `extra_rounds`, `plan_sha256`, `verdict` (`BLOCK` | `REVISE` | `PASS`), `counts`, `findings` (deduplicated, each with `severity`). `plan_sha256` hashes the plan minus its review log (the `> **Plan quality:` line and the sections Critic Verdict, Revisions, Plan Quality Score; an old Adversarial Pass 2 section is plan text and hashed), with CRLF and line-end whitespace normalized. The verdict sits inside the plan because Claude Code's plan approval hands a hook the plan file's text and nothing beside it. **Optional rule-A fields** (decision 43; `DIFF_FIELDS` in `lib/review/critic-diff.mjs`): each one is optional, and absent means round-1 behaviour, so an older block gives the same exit codes. Top level: `diff_base` (the sha256 the round was counted against), `diff_note` (one line: why a round from round 2 on was counted fully), `resolved` (the ids of previous findings that left the count). Per finding: `op_line` and `op_line_end` (the cited lines in the operative text), `line_end`, `rule_keys` (the union of the keys dedup merged), `original_severity` (what a reviewer rated, set whenever the counted `severity` differs), `lowered_by` (`rule A: outside the diff since round <P>` or `rule B: <caught_by>`), `caught_by` (the test, `Regression:` command or gate a rule-B lowering names), `lowering_refused` (why a rule-B lowering was refused) and `carried_from` (the round a carried finding first appeared). `severity` is always the counted severity. `recordVerdict` called without `base`, `baseNote` and `planPaths` records as before, plus the rule-B record, and a per-finding `rule_keys` from dedup is always passed through (an optional field). **Optional stop-rule fields** (decisions 217, 220; `lib/review/stop-rule.mjs`): `rounds` (one `{round, blocking, newByClass}` per recorded round, carried from the previous verdict; `newByClass` is `null` for a round counted in full) and `stop_rule` (`{fired, reason: same-class | not-fewer | null, classes, perRound}`); a verdict without them starts its history at the next round. `accepted_open.under` is `"stop-rule"` for an acceptance made after the stop rule fired. **Optional threat-model fields** (E 2.4, decisions 164, 218; `lib/review/threat-model.mjs`): `known_outside` (one `{id, rule_key, text, line}` per security finding of plan-critic or scope-guardian whose `threat_model_ref` equals the text of an `Outside:` line of the brief's `## Threat Model`, `line` being that text; it left the counted findings and the stop rule, and stays listed in later rounds until a reviewer marks it resolved or it is reported again under its id) and `brief_path` (the brief `--record --brief` read, carried when a later round has none); a verdict without them lists no known issue.
|
||||
6. **`## Critic Verdict`** (written by `lib/validators/plan-gate.mjs --record`, never by hand): one fenced `json` block, schema `voyage.plan-verdict/1` — `round`, `max_rounds` (3), `extra_rounds`, `plan_sha256`, `verdict` (`BLOCK` | `REVISE` | `PASS`), `counts`, `findings` (deduplicated, each with `severity`). `plan_sha256` hashes the plan minus its review log (the `> **Plan quality:` line and the sections Critic Verdict, Revisions, Plan Quality Score; an old Adversarial Pass 2 section is plan text and hashed), with CRLF and line-end whitespace normalized. The verdict sits inside the plan because Claude Code's plan approval hands a hook the plan file's text and nothing beside it. **Optional rule-A fields** (decision 43; `DIFF_FIELDS` in `lib/review/critic-diff.mjs`): each one is optional, and absent means round-1 behaviour, so an older block gives the same exit codes. Top level: `diff_base` (the sha256 the round was counted against), `diff_note` (one line: why a round from round 2 on was counted fully), `resolved` (the ids of previous findings that left the count). Per finding: `op_line` and `op_line_end` (the cited lines in the operative text), `line_end`, `rule_keys` (the union of the keys dedup merged), `original_severity` (what a reviewer rated, set whenever the counted `severity` differs), `lowered_by` (`rule A: outside the diff since round <P>` or `rule B: <caught_by>`), `caught_by` (the test, `Regression:` command or gate a rule-B lowering names), `lowering_refused` (why a rule-B lowering was refused) and `carried_from` (the round a carried finding first appeared). `severity` is always the counted severity. `recordVerdict` called without `base`, `baseNote` and `planPaths` records as before, plus the rule-B record, and a per-finding `rule_keys` from dedup is always passed through (an optional field). **Optional stop-rule fields** (decisions 217, 220; `lib/review/stop-rule.mjs`): `rounds` (one `{round, blocking, newByClass}` per recorded round, carried from the previous verdict; `newByClass` is `null` for a round counted in full) and `stop_rule` (`{fired, reason: same-class | not-fewer | null, classes, perRound}`); a verdict without them starts its history at the next round. `accepted_open.under` is `"stop-rule"` for an acceptance made after the stop rule fired. **Optional threat-model fields** (E 2.4, decisions 164, 218; `lib/review/threat-model.mjs`): `known_outside` (one `{id, rule_key, rule_keys, text, line}` per security finding of plan-critic or scope-guardian whose `threat_model_ref` equals the text of an `Outside:` line of the brief's `## Threat Model`, `line` being that text; it left the counted findings and the stop rule, and stays listed in later rounds until a reviewer marks it resolved or it is reported again under its id) and `brief_path` (the brief `--record --brief` read, stored relative to the repo's top level and read from there whatever the working directory, carried when a later round has none); a verdict without them lists no known issue.
|
||||
7. **Every plan_version, no version bump** (a plan without the field validates unchanged; voyage-lekerepo trial 2, 2026-10-08):
|
||||
- **K10 — `Regression:`** (optional step field): ONE backticked command the criteria runner's allowlist runs, with no pipe and no `&&` (first backticked span, as for `Verify:`). The executor runs it green before the step's checkpoint; a step without it runs nothing new. `Verify:` stays the step's own red-first test. One that can never run is `PLAN_STEP_REGRESSION_NOT_RUNNABLE` (an error; a warning under `--soft`).
|
||||
- **K11 — `On failure:`** opens with one of `revert`, `retry`, `skip`, `escalate` (case-insensitive; a leading `**`, `_`, backtick or `-` is ignored, other leading characters are not; `onFailureAction` in `lib/parsers/plan-schema.mjs`), the words `/trekexecute` reads; any other opening is the warning `PLAN_STEP_ON_FAILURE_UNKNOWN`. A missing `On failure:` is unchanged (the executor defaults to `escalate`).
|
||||
|
|
|
|||
|
|
@ -9,7 +9,7 @@ Per-command flag tables, imported from `CLAUDE.md` via pointer.
|
|||
| _(default)_ | Question round sized by class (`small` / `bounded` / `architectural`), brief-review gate, approval → brief.md with research plan |
|
||||
| `--quick` | Proposes `small` as the class (still said aloud, overridable, raised when the task proves bigger) → brief.md with research plan |
|
||||
| `--gates [true\|false\|closed\|open]` | (v3.4.0) Autonomy flag: bare, `true` or `closed` = on; `false` or `open` = off; any other next token is not its value. See `## Autonomy mode` in `docs/operations.md`. |
|
||||
| `--approve <project-dir \| brief-file>` | (veikart steg 1) No interview: show the existing brief's `## Intent` + `## Goal` verbatim, ask the operator to approve, and on "Approve" stamp `intent_approved_hash` + `intent_approved_at`. The remedy `/trekplan` names when it halts on `BRIEF_INTENT_NOT_APPROVED` / `BRIEF_INTENT_APPROVAL_STALE`. |
|
||||
| `--approve <project-dir \| brief-file>` | (veikart steg 1) No interview: show the existing brief's `## Intent` + `## Goal` (+ `## Threat Model` when present) verbatim, ask the operator to approve, and on "Approve" stamp `intent_approved_hash` + `intent_approved_at`. The remedy `/trekplan` names when it halts on `BRIEF_INTENT_NOT_APPROVED` / `BRIEF_INTENT_APPROVAL_STALE`. |
|
||||
|
||||
Always interactive. Phase 3 puts the task in a size class (`ceremony:`, K1; heaviest in doubt, said aloud, overridable, only moved up) and asks one round of questions within the class budget (`small` 4, `bounded` 8, `architectural` 12 per round), recommendation first, facts looked up instead of asked, a visible Deferred list; it also asks where the brief is stored (`artifact_store: git | local`). Phase 4 runs a `brief-reviewer` stop-gate with 1 / 2 / 3 review iterations by class. Phase 4h takes only an explicit "Approve" («høres bra ut» and an empty answer are refused by `lib/util/approval-log.mjs answer`), stamps the brief, appends the approval to the target repo's log (`approval-log.mjs append`, K2) and commits it (`lib/util/artifact-commit.mjs`: the log always, the brief when `git`). After that, asks the user to choose manual (print commands) or auto (Claude runs research + plan in foreground).
|
||||
|
||||
|
|
|
|||
|
|
@ -36,8 +36,10 @@
|
|||
// `threat_model_ref` (the text of the brief's `Outside:` line it quotes), and
|
||||
// a merge keeps it only when every merged copy carries the same one; one copy
|
||||
// without it, or two different ones, drop it, so the plan gate counts the
|
||||
// merged finding in full. A non-object in a reviewer's findings list (a
|
||||
// `null`) is skipped instead of taking the CLI down (the S160 known issue).
|
||||
// merged finding in full. A `null` or `undefined` in a reviewer's findings
|
||||
// list is skipped instead of taking the CLI down (the S160 known issue); any
|
||||
// other entry that is not an object (a string, a number, an array) stays a
|
||||
// finding with no valid severity, which the plan gate counts as a blocker.
|
||||
//
|
||||
// Provenance is preserved on the surviving finding's `raised_by` array.
|
||||
// Severity is kept (normalized to blocker | major | minor, else null). A merge
|
||||
|
|
@ -218,8 +220,9 @@ function normalizeAgentPayload(payload, fallbackAgent) {
|
|||
const agent = (typeof payload.agent === 'string' && payload.agent.length > 0)
|
||||
? payload.agent
|
||||
: fallbackAgent;
|
||||
// A non-object entry (a `null`) is no finding: skipped, never thrown on (S160).
|
||||
const findings = Array.isArray(payload.findings) ? payload.findings.filter(f => f !== null && typeof f === 'object') : [];
|
||||
// A `null` or `undefined` entry holds nothing: skipped, never thrown on (S160).
|
||||
// Any other entry, an object or not, goes on as before (no valid severity = a blocker).
|
||||
const findings = Array.isArray(payload.findings) ? payload.findings.filter(f => f !== null && f !== undefined) : [];
|
||||
const previous = Array.isArray(payload.previous) ? payload.previous : [];
|
||||
return { agent, pairs: findings.map(f => ({ agent, finding: f })), previous };
|
||||
}
|
||||
|
|
|
|||
|
|
@ -27,8 +27,11 @@
|
|||
// goes back into the findings at its catalogue tier with no id, a `### `
|
||||
// entry with a `**Source:**` line saying why, the frontmatter list, counts,
|
||||
// a verdict that is only raised and, as a BLOCKER, its `### Step R<n>`.
|
||||
// The entries that stay are listed under `## Outside the Threat Model`;
|
||||
// a malformed entry stays too (the verdict gate exits 2 on it).
|
||||
// An entry whose row a finding already carries (the engine pass put the
|
||||
// same engine finding back, engine-compare.mjs `rowOf`) leaves the list
|
||||
// without a second copy. The entries that stay are listed under
|
||||
// `## Outside the Threat Model`; a malformed entry stays too (the verdict
|
||||
// gate exits 2 on it).
|
||||
// - instruction, right after engine: a finding under `CLAUDE_MD_DEVIATION`
|
||||
// or `CLAUDE_MD_STALE` whose `instruction_ref` is missing or names a file
|
||||
// outside the repo's tracked instruction set (lib/review/instruction-scope.mjs
|
||||
|
|
@ -122,7 +125,7 @@ import { isSecurityFinding } from './review-answers.mjs';
|
|||
import { carry, frontmatterField, MAX_REVIEW_ROUNDS, runId } from './review-rounds.mjs';
|
||||
import { applyPolicy, readPolicy } from './review-policy.mjs';
|
||||
import { ENGINES } from './engine-findings.mjs';
|
||||
import { ENGINE_OF_REVIEWER, putBack } from './engine-compare.mjs';
|
||||
import { ENGINE_OF_REVIEWER, putBack, rowOf, trimmedTitle } from './engine-compare.mjs';
|
||||
import { backedLine, isKnownEntry, reviewThreatModel } from './threat-model.mjs';
|
||||
import { getRule } from './rule-catalogue.mjs';
|
||||
import { isMain } from '../util/is-main.mjs';
|
||||
|
|
@ -288,9 +291,15 @@ function passOutside(payload, ctx) {
|
|||
else kept.push(e);
|
||||
}
|
||||
const why = tm ? 'no `Outside:` line of the brief equals its threat_model_ref, or it is not a Voyage security finding' : use.why;
|
||||
// A row a finding already carries (the engine pass put the same engine
|
||||
// finding back) is counted once: the entry leaves the list, no second copy.
|
||||
const rows = new Set(payload.findings.filter(isPlainObject).map(rowOf));
|
||||
for (const e of back) {
|
||||
const rule = getRule(e.rule_key.trim().toUpperCase());
|
||||
const f = { ...e, id: null, rule_key: rule ? rule.rule_key : e.rule_key, severity: rule ? rule.severity : 'BLOCKER' };
|
||||
const held = rows.has(rowOf(f)) || (typeof f.title === 'string' && rows.has(rowOf({ ...f, title: trimmedTitle(f.title) })));
|
||||
if (held) continue;
|
||||
rows.add(rowOf(f));
|
||||
payload.findings.push(f);
|
||||
ctx.added.add(f);
|
||||
ctx.outsideBack.set(f, why);
|
||||
|
|
|
|||
|
|
@ -21,9 +21,16 @@
|
|||
// Otherwise every security finding counts, and `why` says why.
|
||||
// backedLine(finding, tm) -> the `Outside:` text the finding's
|
||||
// `threat_model_ref` equals exactly, else null.
|
||||
// reviewThreatModel(reviewText, repo) -> usable() for the brief a review
|
||||
// names in its `brief_path` (a relative path is read from the repo's top
|
||||
// level); the review side's `known_outside` (E 2.5) is checked with it.
|
||||
// usableInRepo(briefPath, repo) -> usable() for a brief path relative to
|
||||
// the repo's top level (an absolute one as it is); both gates read a
|
||||
// stored `brief_path` this way, whatever directory they run in.
|
||||
// repoRelativeBrief(briefPath, repo) -> a brief path given from the
|
||||
// working directory, made relative to the repo's top level (forward
|
||||
// slashes; `..` when outside it), never absolute: what the plan gate
|
||||
// stores as `brief_path`.
|
||||
// reviewThreatModel(reviewText, repo) -> usableInRepo() for the brief a
|
||||
// review names in its `brief_path`; the review side's `known_outside`
|
||||
// (E 2.5) is checked with it.
|
||||
// isKnownEntry(e) -> a review `known_outside` entry's shape: a
|
||||
// finding with a string `rule_key` and `file`, an integer `line` >= 0
|
||||
// and a non-empty `threat_model_ref`.
|
||||
|
|
@ -33,7 +40,7 @@
|
|||
// every run, so a finding whose line is gone counts in full.
|
||||
|
||||
import { readFileSync } from 'node:fs';
|
||||
import { isAbsolute, join } from 'node:path';
|
||||
import { isAbsolute, join, relative, sep } from 'node:path';
|
||||
import { parseDocument } from '../util/frontmatter.mjs';
|
||||
import { verifyLog, normalizeHex, normalizeSubject, resolveRepo } from '../util/approval-log.mjs';
|
||||
import { checkIntentApprovalContent } from '../validators/intent-approval.mjs';
|
||||
|
|
@ -93,8 +100,38 @@ export function usable(briefPath, repo) {
|
|||
}
|
||||
|
||||
/**
|
||||
* The threat model a review applies: usable() for the brief at the review's
|
||||
* frontmatter `brief_path`, a relative path read from the repo's top level.
|
||||
* usable() for a brief path relative to the repo's top level (an absolute
|
||||
* path as it is), so a stored `brief_path` reads the same brief from any
|
||||
* working directory.
|
||||
* @param {string} briefPath
|
||||
* @param {string} [repo] - the repo (default: the git top level of the working directory)
|
||||
* @returns {{usable: boolean, why: string|null, threatModel: object}}
|
||||
*/
|
||||
export function usableInRepo(briefPath, repo) {
|
||||
if (typeof briefPath !== 'string' || briefPath.trim() === '') return { usable: false, why: 'no brief path was given', threatModel: EMPTY };
|
||||
let root;
|
||||
try { root = resolveRepo(repo); } catch (e) {
|
||||
return { usable: false, why: `the repo could not be read: ${e.message}`, threatModel: EMPTY };
|
||||
}
|
||||
return usable(isAbsolute(briefPath) ? briefPath : join(root, briefPath), root);
|
||||
}
|
||||
|
||||
/**
|
||||
* A brief path given from the working directory, relative to the repo's top
|
||||
* level: forward slashes, `..` when the brief is outside the repo, never absolute.
|
||||
* @param {string} briefPath
|
||||
* @param {string} [repo] - the repo (default: the git top level of the working directory)
|
||||
* @returns {string}
|
||||
*/
|
||||
export function repoRelativeBrief(briefPath, repo) {
|
||||
const root = resolveRepo(repo);
|
||||
const s = normalizeSubject(root, briefPath);
|
||||
return isAbsolute(s) ? relative(root, s).split(sep).join('/') : s;
|
||||
}
|
||||
|
||||
/**
|
||||
* The threat model a review applies: usableInRepo() for the brief at the
|
||||
* review's frontmatter `brief_path`, a relative path read from the repo's top level.
|
||||
* @param {string} reviewText - review.md
|
||||
* @param {string} [repo] - the repo (default: the git top level of the working directory)
|
||||
* @returns {{usable: boolean, why: string|null, threatModel: object}}
|
||||
|
|
@ -103,11 +140,7 @@ export function reviewThreatModel(reviewText, repo) {
|
|||
const doc = parseDocument(typeof reviewText === 'string' ? reviewText : '');
|
||||
const bp = doc.parsed?.frontmatter?.brief_path;
|
||||
if (typeof bp !== 'string' || bp.trim() === '') return { usable: false, why: 'the review names no brief_path', threatModel: EMPTY };
|
||||
let root;
|
||||
try { root = resolveRepo(repo); } catch (e) {
|
||||
return { usable: false, why: `the repo could not be read: ${e.message}`, threatModel: EMPTY };
|
||||
}
|
||||
return usable(isAbsolute(bp) ? bp : join(root, bp), root);
|
||||
return usableInRepo(bp, repo);
|
||||
}
|
||||
|
||||
/** A review `known_outside` entry's shape (E 2.5): string rule_key and file, integer line >= 0, non-empty threat_model_ref. */
|
||||
|
|
|
|||
|
|
@ -122,9 +122,12 @@
|
|||
// log). A backed finding — Voyage's own security finding (no engine) whose
|
||||
// `threat_model_ref` equals an `Outside:` line's text exactly — leaves the
|
||||
// counted set and the stop rule and goes to the optional verdict field
|
||||
// `known_outside` ({id, rule_key, text, line}), carried to later rounds until
|
||||
// a reviewer marks it resolved (or it counts again under its id); the verdict
|
||||
// stores `brief_path` (carried when a later round has no --brief). A threat
|
||||
// `known_outside` ({id, rule_key, rule_keys, text, line}), carried to later
|
||||
// rounds until a reviewer marks it resolved (or it counts again under its id);
|
||||
// a copy of it that rule A carried from an earlier round as counted leaves the
|
||||
// count too. The verdict stores `brief_path`, relative to the repo's top level
|
||||
// (carried when a later round has no --brief), and the gate reads it from
|
||||
// there whatever its working directory. A threat
|
||||
// model that is not usable backs nothing: every security finding counts, and
|
||||
// `threat_model.why` says why. The bare gate re-reads the brief at
|
||||
// `brief_path` on every run: an entry still backed is a warning
|
||||
|
|
@ -140,10 +143,10 @@ import { createHash } from 'node:crypto';
|
|||
import { dirname, join, resolve } from 'node:path';
|
||||
import { validatePlanContent } from './plan-validator.mjs';
|
||||
import { splitSections, fenceTracker, extractPlanVersion, versionAtLeast } from '../parsers/plan-schema.mjs';
|
||||
import { normalizeSeverity, isSecurity } from '../review/plan-review-dedup.mjs';
|
||||
import { normalizeSeverity, isSecurity, SEVERITIES } from '../review/plan-review-dedup.mjs';
|
||||
import { countRound, lineDiff, oneLine } from '../review/critic-diff.mjs';
|
||||
import { roundSummary, decideStop, describeStop, STOP_REASONS } from '../review/stop-rule.mjs';
|
||||
import { usable as threatModelUsable, backedLine } from '../review/threat-model.mjs';
|
||||
import { usableInRepo, repoRelativeBrief, backedLine } from '../review/threat-model.mjs';
|
||||
import { resolveRepo } from '../verification/repo-tools.mjs';
|
||||
import { isMain } from '../util/is-main.mjs';
|
||||
|
||||
|
|
@ -281,13 +284,21 @@ function okStopRule(s) {
|
|||
|
||||
const isStrOrNull = x => x === null || typeof x === 'string';
|
||||
|
||||
/** Optional `known_outside`: a list of {id: string|null, rule_key: string|null, text: string, line: non-empty string}. */
|
||||
/** Optional `known_outside`: a list of {id: string|null, rule_key: string|null, rule_keys?: string[], text: string, line: non-empty string}. */
|
||||
function okKnownOutside(k) {
|
||||
if (k === undefined) return true;
|
||||
return Array.isArray(k) && k.every(e => isPlainObject(e) && isStrOrNull(e.id) && isStrOrNull(e.rule_key) &&
|
||||
(e.rule_keys === undefined || (Array.isArray(e.rule_keys) && e.rule_keys.every(x => typeof x === 'string'))) &&
|
||||
typeof e.text === 'string' && typeof e.line === 'string' && e.line !== '');
|
||||
}
|
||||
|
||||
/** A counted finding a reviewer rated higher than it counts (rule A or B). */
|
||||
const isLowered = x => {
|
||||
const o = normalizeSeverity(x.original_severity);
|
||||
const s = normalizeSeverity(x.severity);
|
||||
return o !== null && s !== null && SEVERITIES.indexOf(o) < SEVERITIES.indexOf(s);
|
||||
};
|
||||
|
||||
/** Optional `brief_path`: a non-empty string. */
|
||||
const okBriefPath = p => p === undefined || (typeof p === 'string' && p !== '');
|
||||
|
||||
|
|
@ -407,9 +418,15 @@ export function recordVerdict(text, dedup, opts = {}) {
|
|||
const counted = [];
|
||||
for (const f of dedup.findings) {
|
||||
const line = tm ? backedLine(f, tm) : null;
|
||||
if (line !== null) backed.push({ id: f.id ?? null, rule_key: f.rule_key ?? null, text: typeof f.text === 'string' ? f.text : '', line });
|
||||
else counted.push(f);
|
||||
if (line === null) { counted.push(f); continue; }
|
||||
// rule_keys kept, so the bare gate re-checks the entry with the same keys (keysOf).
|
||||
backed.push({
|
||||
id: f.id ?? null, rule_key: f.rule_key ?? null,
|
||||
...(Array.isArray(f.rule_keys) ? { rule_keys: f.rule_keys.filter(k => typeof k === 'string') } : {}),
|
||||
text: typeof f.text === 'string' ? f.text : '', line,
|
||||
});
|
||||
}
|
||||
const backedIds = new Set(backed.map(e => e.id).filter(id => id !== null));
|
||||
const c = countRound({
|
||||
round,
|
||||
previous: prev.verdict ?? null,
|
||||
|
|
@ -420,7 +437,12 @@ export function recordVerdict(text, dedup, opts = {}) {
|
|||
findings: counted,
|
||||
marks: Array.isArray(dedup.marks) ? dedup.marks : [],
|
||||
});
|
||||
const findings = c.findings;
|
||||
// A previous finding countRound carried (rule A) whose id is backed this
|
||||
// round leaves the counted set too: listed once, in known_outside, and out
|
||||
// of the counts, the verdict and the stop rule's summary.
|
||||
const leaving = c.findings.filter(x => x.carried_from !== undefined && x.carried_from !== null && backedIds.has(x.id));
|
||||
const findings = c.findings.filter(x => !leaving.includes(x));
|
||||
const lowered = c.lowered - leaving.filter(isLowered).length;
|
||||
const counts = { blocker: 0, major: 0, minor: 0 };
|
||||
for (const f of findings) counts[f.severity] += 1;
|
||||
const verdict = {
|
||||
|
|
@ -441,7 +463,6 @@ export function recordVerdict(text, dedup, opts = {}) {
|
|||
// entries that no reviewer marked resolved and that are not reported again
|
||||
// under their id (backed again: listed once; counted: in full, not listed).
|
||||
const marks = Array.isArray(dedup.marks) ? dedup.marks : [];
|
||||
const backedIds = new Set(backed.map(e => e.id).filter(id => id !== null));
|
||||
const countedIds = new Set(findings.map(x => x.id).filter(id => typeof id === 'string'));
|
||||
const kept = (Array.isArray(prev.verdict?.known_outside) ? prev.verdict.known_outside : [])
|
||||
.filter(e => !knownResolved(e, marks) && !backedIds.has(e.id) && !countedIds.has(e.id));
|
||||
|
|
@ -459,7 +480,7 @@ export function recordVerdict(text, dedup, opts = {}) {
|
|||
? { applied: false, why: 'no --brief given' }
|
||||
: { applied: use.usable === true, why: use.usable === true ? null : (use.why ?? 'the threat model is not usable') };
|
||||
return {
|
||||
code: 0, text: placeSection(text, renderSection(verdict)), verdict, lowered: c.lowered, threat_model: threat,
|
||||
code: 0, text: placeSection(text, renderSection(verdict)), verdict, lowered, threat_model: threat,
|
||||
message: `round ${round} recorded: ${verdict.verdict}` +
|
||||
(knownOutside.length > 0 ? `; ${knownOutside.length} finding(s) outside the threat model listed as known issues` : '') +
|
||||
(use !== undefined && !threat.applied ? `; threat model not applied, every security finding counts: ${threat.why}` : '') +
|
||||
|
|
@ -548,7 +569,8 @@ function checkKnownOutside(entries, use, reasons, warnings) {
|
|||
const backed = [];
|
||||
const unbacked = [];
|
||||
for (const e of entries) {
|
||||
(tm && backedLine({ rule_key: e.rule_key, threat_model_ref: e.line }, tm) !== null ? backed : unbacked).push(e);
|
||||
const asFinding = { rule_key: e.rule_key, ...(Array.isArray(e.rule_keys) ? { rule_keys: e.rule_keys } : {}), threat_model_ref: e.line };
|
||||
(tm && backedLine(asFinding, tm) !== null ? backed : unbacked).push(e);
|
||||
}
|
||||
if (backed.length > 0) {
|
||||
warnings.push({
|
||||
|
|
@ -631,8 +653,10 @@ export function gatePlan(rawText, opts = {}) {
|
|||
}
|
||||
const known = Array.isArray(v.known_outside) ? v.known_outside : [];
|
||||
if (known.length > 0) {
|
||||
// brief_path is relative to the repo's top level, so the gate reads the
|
||||
// same brief from any working directory inside the repo.
|
||||
const use = opts.threatModel ?? (typeof v.brief_path === 'string'
|
||||
? threatModelUsable(v.brief_path)
|
||||
? usableInRepo(v.brief_path)
|
||||
: { usable: false, why: 'the verdict names no brief_path', threatModel: null });
|
||||
checkKnownOutside(known, use, reasons, warnings);
|
||||
}
|
||||
|
|
@ -826,8 +850,9 @@ if (isMain(import.meta.url)) {
|
|||
const dir = snapshotDir(planPath);
|
||||
const opts = { extraRound: args.includes('--extra-round'), reviewed, planPaths: planPathsOf(planPath) };
|
||||
if (briefArg !== undefined) {
|
||||
opts.briefPath = briefArg;
|
||||
opts.threatModel = threatModelUsable(briefArg);
|
||||
// Stored relative to the repo's top level, never absolute; read from there.
|
||||
opts.briefPath = repoRelativeBrief(briefArg);
|
||||
opts.threatModel = usableInRepo(opts.briefPath);
|
||||
}
|
||||
const lb = loadBase(text, dir);
|
||||
if (lb.verdict) {
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
---
|
||||
name: annotating
|
||||
description: "The operator's own annotation loop for a Voyage brief, plan or review (a research document too, when asked): build the page with scripts/annotate.mjs so the person writes every note, take the pasted Copy Prompt back, and revise note by note. Fix and Change edit only the text the note points at; a Question is answered and changes nothing unless the person says so; nothing the notes did not ask for is edited. A note on ## Intent or ## Goal of an approved brief makes the approval stale, and a changed plan needs a new approval, said before the edit. The page is built again on the revised file. Use when someone wants to comment on, annotate or mark up a brief, plan, review or research document in the browser, or pastes back the notes from that page (Fix, Change, Question)."
|
||||
description: "The operator's own annotation loop for a Voyage brief, plan or review (a research document too, when asked): build the page with scripts/annotate.mjs so the person writes every note, take the pasted Copy Prompt back, and revise note by note. Fix and Change edit only the text the note points at; a Question is answered and changes nothing unless the person says so; nothing the notes did not ask for is edited. A note on ## Intent or ## Goal (or ## Threat Model) of an approved brief makes the approval stale, and a changed plan needs a new approval, said before the edit. The page is built again on the revised file. Use when someone wants to comment on, annotate or mark up a brief, plan, review or research document in the browser, or pastes back the notes from that page (Fix, Change, Question)."
|
||||
---
|
||||
|
||||
# Annotating
|
||||
|
|
@ -46,7 +46,7 @@ approval.
|
|||
anchored to, and the comment. Work from the quote and the comment, one note at a time.
|
||||
4. **Before any edit, check what the edit costs.**
|
||||
- A brief with `intent_approved_hash` in its frontmatter, and a Fix or Change in `## Intent` or
|
||||
`## Goal`: say in one line, before the edit, that the edit makes the approval stale, that
|
||||
`## Goal` (or `## Threat Model`, when the brief has one): say in one line, before the edit, that the edit makes the approval stale, that
|
||||
`/trekplan` halts until the new text is approved, and how to approve it
|
||||
(`/trekbrief --approve <project dir>`). Then edit.
|
||||
- A plan the person approved: any edit gives it a new sha256, the approval log no longer covers it,
|
||||
|
|
@ -84,7 +84,7 @@ approval.
|
|||
- `tests/lib/doc-consistency.test.mjs` and `tests/lib/skills-annotating-prose-pins.test.mjs` (in
|
||||
`npm test`): the script `scripts/annotate.mjs` exists, the three producing commands call it, print
|
||||
the `file://` link and the `open` line, and name this skill. Deterministic, but only at test time.
|
||||
- `lib/validators/intent-approval.mjs` (inside `/trekplan`): an edit to `## Intent` or `## Goal`
|
||||
- `lib/validators/intent-approval.mjs` (inside `/trekplan`): an edit to `## Intent` or `## Goal` (or `## Threat Model`)
|
||||
after approval makes the approval stale, and planning halts until the new text is approved.
|
||||
- `lib/util/approval-log.mjs` (inside `/trekexecute`, K2): a plan whose current sha256 the approval
|
||||
log does not cover is refused. Both checks hold whether or not the edit's cost was said.
|
||||
|
|
|
|||
|
|
@ -84,6 +84,12 @@ for (const rel of AGENTS) {
|
|||
assert.ok(t.includes('`rule_key` names the class, never the instance'), `${rel} must say rule_key names the class, never the instance`);
|
||||
});
|
||||
|
||||
test(`${rel}: round 2+ marks each known_outside entry by id in previous too (E fix round)`, () => {
|
||||
const t = norm(read(rel));
|
||||
assert.match(t, /`## Critic Verdict` also lists `known_outside`[^.]*\. Mark each of them by id in `previous` the same way/);
|
||||
assert.match(t, /`previous` \(from round 2; omit it in round 1\) holds one `\{id, resolved\}` per finding the current `## Critic Verdict` lists, its `known_outside` entries included/);
|
||||
});
|
||||
|
||||
test(`${rel}: round 2+ lists every previous finding by id and cites the exact line`, () => {
|
||||
const t = norm(read(rel));
|
||||
assert.ok(t.includes('From round 2 the prompt gives you the diff since the last reviewed text'), `${rel}: round-2 diff sentence missing`);
|
||||
|
|
|
|||
|
|
@ -49,6 +49,10 @@ test('Phase 9 hands the brief to plan-critic and --brief to --record; the threat
|
|||
assert.match(p9, /`threat_model`/);
|
||||
assert.match(p9, /PLAN_GATE_KNOWN_OUTSIDE/);
|
||||
assert.match(p9, /PLAN_GATE_OUTSIDE_NOT_BACKED/);
|
||||
// Fix round: both round-2 sentences name the known_outside entries to mark.
|
||||
const round2 = p9.split('\n').filter(l => l.startsWith('"Previous findings are in the plan\'s `## Critic Verdict`'));
|
||||
assert.equal(round2.length, 2, 'one round-2 sentence per reviewer');
|
||||
for (const l of round2) assert.match(l, /its `known_outside` entries included/);
|
||||
});
|
||||
|
||||
test('Phase 10 runs the plan gate before offering "execute", and covers plan mode', () => {
|
||||
|
|
|
|||
|
|
@ -520,6 +520,16 @@ test('a null in a reviewer\'s findings list no longer crashes dedup (the S160 kn
|
|||
assert.equal(r.findings[0].rule_key, 'PC1');
|
||||
});
|
||||
|
||||
test('only null and undefined are skipped: a string, a number or an array entry stays a finding with no valid severity, as before E', () => {
|
||||
const one = dedupFindings([{ agent: 'plan-critic', payload: { findings: [null, undefined, 'BLOCKER: rm -rf on an unquoted path'] } }]);
|
||||
assert.equal(one.findings.length, 1, JSON.stringify(one.findings));
|
||||
assert.equal(one.findings[0].severity, null, 'no valid severity: the plan gate counts it as a blocker');
|
||||
// Several such entries carry no text, so the Jaccard pass merges them, as it did before E.
|
||||
const many = dedupFindings([{ agent: 'plan-critic', payload: { findings: [7, ['x'], 'str'] } }]);
|
||||
assert.ok(many.findings.length >= 1, JSON.stringify(many.findings));
|
||||
for (const f of many.findings) assert.equal(f.severity, null);
|
||||
});
|
||||
|
||||
test('CLI --stdin: a null finding no longer crashes it (exit 0)', () => {
|
||||
const r = spawnSync(process.execPath, [SHIM, '--stdin'], {
|
||||
input: JSON.stringify({ plan_critic: { findings: [null] }, scope_guardian: { findings: [] } }),
|
||||
|
|
|
|||
|
|
@ -28,6 +28,7 @@ import { validateReviewContent } from '../../lib/validators/review-validator.mjs
|
|||
import { stampIntentApproval } from '../../lib/validators/intent-approval.mjs';
|
||||
import { appendApproval, normalizeHex, normalizeSubject, resolveRepo } from '../../lib/util/approval-log.mjs';
|
||||
import { childEnv } from '../helpers/child-env.mjs';
|
||||
import { releaseCheck } from '../../scripts/release-check.mjs';
|
||||
|
||||
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
||||
const FINALIZE = join(ROOT, 'lib', 'review', 'review-finalize.mjs');
|
||||
|
|
@ -287,6 +288,58 @@ test('gate: a backed entry turns not backed once the brief\'s approval is stale
|
|||
assert.match(JSON.parse(g2.stdout).reason, /BRIEF_INTENT_APPROVAL_STALE/);
|
||||
});
|
||||
|
||||
// ---- Part 2 fix round ---------------------------------------------------------------
|
||||
|
||||
test('fix 7: an engine finding the coordinator listed in known_outside while its payload is saved goes back once', () => {
|
||||
const fx = tmRepo();
|
||||
const engineFinding = {
|
||||
id: 'c'.repeat(40), severity: 'BLOCKER', rule_key: 'SECURITY_INJECTION', file: 'commands/up.md', line: 5,
|
||||
brief_ref: 'NFR — security (Claude Code /security-review)', title: 'Unquoted name reaches rm',
|
||||
detail: 'rm -rf "$NAME" with a name from the request.', recommended_action: 'Quote and contain the name.', engine: 'security-review',
|
||||
};
|
||||
writeFileSync(join(fx.project, '.review-engine-payload-security-review.json'),
|
||||
JSON.stringify({ reviewer: 'claude-security-review', findings: [engineFinding], out_of_scope: [] }));
|
||||
const { id: _id, ...asListed } = engineFinding;
|
||||
fx.write(tmReview({ start: fx.start, end: fx.end, known: [{ ...asListed, threat_model_ref: OUTSIDE }] }));
|
||||
const r = fx.finalize();
|
||||
assert.equal(r.status, 0, r.stdout + r.stderr);
|
||||
const json = jsonOf(fx.read());
|
||||
const at5 = json.findings.filter((f) => f.file === 'commands/up.md' && f.line === 5);
|
||||
assert.equal(at5.length, 1, `put back once: ${JSON.stringify(json.findings.map((f) => [f.id, f.rule_key, f.engine]))}`);
|
||||
assert.equal(at5[0].severity, 'BLOCKER');
|
||||
assert.deepEqual(json.known_outside ?? [], []);
|
||||
assert.equal(json.verdict, 'BLOCK');
|
||||
const v = fx.validate();
|
||||
assert.equal(v.status, 0, v.stdout + v.stderr);
|
||||
});
|
||||
|
||||
test('fix 8: scripts/release-check.mjs exits 2 on a review whose known_outside entry the brief does not back', () => {
|
||||
const fx = tmRepo();
|
||||
fx.write(tmReview({ start: fx.start, end: fx.end, known: [entry({ threat_model_ref: INSIDE })] }));
|
||||
const r = releaseCheck({ repo: fx.repo, review: fx.reviewPath, version: '1.0.0', suiteTimeoutMs: 30000 });
|
||||
assert.equal(r.exit, 2, JSON.stringify(r.checks));
|
||||
const rev = r.checks.find((c) => c.id === 'review');
|
||||
assert.equal(rev.status, 'error');
|
||||
assert.match(rev.detail, /threat model/i);
|
||||
// Control: the same review with a backed entry passes the review check.
|
||||
fx.write(tmReview({ start: fx.start, end: fx.end, known: [entry()] }));
|
||||
const c = releaseCheck({ repo: fx.repo, review: fx.reviewPath, version: '1.0.0', suiteTimeoutMs: 30000 });
|
||||
assert.equal(c.checks.find((x) => x.id === 'review').status, 'pass', JSON.stringify(c.checks));
|
||||
});
|
||||
|
||||
test('fix 5: the approval prose says a ## Threat Model is covered too, when the brief has one', () => {
|
||||
const line = (rel, needle) => read(rel).split('\n').find((l) => l.includes(needle)) ?? '';
|
||||
assert.match(line('docs/command-modes.md', '| `--approve <project-dir'), /`## Threat Model`/);
|
||||
assert.match(line('docs/HANDOVER-CONTRACTS.md', '| `intent_approved_hash` |'), /`## Threat Model`/);
|
||||
assert.match(line('CLAUDE.md', '| `/trekbrief` |'), /`## Threat Model`/);
|
||||
assert.match(line('README.md', '**Approval, logged and committed.**'), /`## Threat Model`/);
|
||||
assert.match(line('README.md', '| **Approve** |'), /`## Threat Model`/);
|
||||
assert.match(line('README.md', '**Intent approval gate.**'), /`## Threat Model`/);
|
||||
const skill = read('skills/annotating/SKILL.md');
|
||||
assert.match(skill.split('\n').find((l) => l.startsWith('description:')), /## Threat Model/);
|
||||
assert.equal((flat(skill).match(/`## Intent` or `## Goal` \(or `## Threat Model`/g) ?? []).length, 2, 'the Process step and the guard list');
|
||||
});
|
||||
|
||||
// ---- prose --------------------------------------------------------------------------
|
||||
|
||||
function subsection(text, heading) {
|
||||
|
|
|
|||
|
|
@ -23,7 +23,7 @@ import { test } from 'node:test';
|
|||
import { strict as assert } from 'node:assert';
|
||||
import { resolve, join } from 'node:path';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { mkdtempSync, writeFileSync, readFileSync, rmSync, realpathSync } from 'node:fs';
|
||||
import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, rmSync, realpathSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { gatePlan, recordVerdict, planHash, operativeText, extractVerdict } from '../../lib/validators/plan-gate.mjs';
|
||||
|
|
@ -122,9 +122,10 @@ test('record: a backed security finding leaves the counted set and goes to known
|
|||
assert.deepEqual(v.findings, []);
|
||||
assert.equal(v.known_outside?.length, 1, JSON.stringify(v));
|
||||
const e = v.known_outside[0];
|
||||
assert.deepEqual(Object.keys(e).sort(), ['id', 'line', 'rule_key', 'text']);
|
||||
assert.deepEqual(Object.keys(e).sort(), ['id', 'line', 'rule_key', 'rule_keys', 'text']);
|
||||
assert.match(e.id, /^[0-9a-f]{40}$/);
|
||||
assert.equal(e.rule_key, 'SECURITY_RISK');
|
||||
assert.deepEqual(e.rule_keys, ['SECURITY_RISK']);
|
||||
assert.equal(e.text, S1);
|
||||
assert.equal(e.line, OUTSIDE);
|
||||
assert.equal(v.brief_path, 'brief.md');
|
||||
|
|
@ -246,7 +247,7 @@ test('gate guard: a planted known_outside entry that is not a Voyage security fi
|
|||
assert.equal(r.verdict.known_outside?.length, 1, 'round 1 lists the backed finding');
|
||||
const planted = r.text.replace(/```json\n[\s\S]*?\n```/, () => {
|
||||
const v = extractVerdict(r.text).verdict;
|
||||
return '```json\n' + JSON.stringify({ ...v, known_outside: [{ ...v.known_outside[0], rule_key: 'MISSING_TEST' }] }, null, 2) + '\n```';
|
||||
return '```json\n' + JSON.stringify({ ...v, known_outside: [{ ...v.known_outside[0], rule_key: 'MISSING_TEST', rule_keys: ['MISSING_TEST'] }] }, null, 2) + '\n```';
|
||||
});
|
||||
assert.equal(extractVerdict(planted).error, null);
|
||||
const g = gatePlan(planted, { threatModel: TM });
|
||||
|
|
@ -313,6 +314,35 @@ test('CLI: --record --brief lists a backed finding as a known issue; the gate wa
|
|||
}
|
||||
});
|
||||
|
||||
test('fix 6: brief_path is stored relative to the repo top level, and the gate run from a subdirectory reads the same brief', () => {
|
||||
const dir = realpathSync(mkdtempSync(join(tmpdir(), 'plan-gate-tm-')));
|
||||
try {
|
||||
assert.equal(spawnSync('git', ['init', '-q'], { cwd: dir, env: childEnv(GIT_CLEAN) }).status, 0);
|
||||
const b = join(dir, 'brief.md');
|
||||
const s = stampIntentApproval(BRIEF, new Date(NOW));
|
||||
writeFileSync(b, s.text);
|
||||
const repo = resolveRepo(dir);
|
||||
assert.equal(appendApproval({ repo, kind: 'intent', subject: normalizeSubject(repo, b), sha: normalizeHex(s.hash) }).appended, true);
|
||||
const sub = join(dir, 'sub');
|
||||
mkdirSync(sub);
|
||||
const p = join(dir, 'plan.md');
|
||||
for (const [cwd, briefArg] of [[dir, 'brief.md'], [sub, '../brief.md'], [sub, b]]) {
|
||||
writeFileSync(p, buildPlan());
|
||||
const t1 = readFileSync(p, 'utf-8');
|
||||
const payload = dd({ pc: [{ ...f('blocker', at(t1, R1_CHANGES), S1, 'SECURITY_RISK', { threat_model_ref: OUTSIDE }), file: p }] });
|
||||
const c1 = run(cwd, ['--record', p, '--brief', briefArg, '--reviewed', planHash(t1)], JSON.stringify(payload));
|
||||
assert.equal(c1.code, 0, c1.out + c1.err);
|
||||
assert.equal(lastJson(c1.out).known_outside, 1, `${cwd} ${briefArg}: ${c1.out}`);
|
||||
assert.equal(extractVerdict(readFileSync(p, 'utf-8')).verdict.brief_path, 'brief.md', `stored relative to the top level (${briefArg})`);
|
||||
const g = run(sub, [p]);
|
||||
assert.equal(g.code, 0, `the gate from a subdirectory: ${g.out}`);
|
||||
assert.match(g.out, /WARN {2}\[PLAN_GATE_KNOWN_OUTSIDE\]/);
|
||||
}
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('CLI: --brief without a value is a usage error; a threat model that is not usable is said in the output', () => {
|
||||
const dir = realpathSync(mkdtempSync(join(tmpdir(), 'plan-gate-tm-')));
|
||||
try {
|
||||
|
|
@ -337,6 +367,73 @@ test('CLI: --brief without a value is a usage error; a threat model that is not
|
|||
}
|
||||
});
|
||||
|
||||
// ---- Part 2 fix round ---------------------------------------------------------------
|
||||
|
||||
test('fix 1: a string entry in a reviewer\'s findings still counts as a blocker through dedup and --record', () => {
|
||||
const t1 = buildPlan();
|
||||
const d = dedupFindings([
|
||||
{ agent: 'plan-critic', payload: { findings: ['BLOCKER: step 2 runs rm -rf on an unquoted user path'] } },
|
||||
{ agent: 'scope-guardian', payload: { findings: [] } },
|
||||
]);
|
||||
assert.equal(d.findings.length, 1, 'the string entry is kept as a finding with no valid severity');
|
||||
const r = rec(t1, d);
|
||||
assert.equal(r.verdict.counts.blocker, 1);
|
||||
assert.equal(r.verdict.verdict, 'BLOCK');
|
||||
});
|
||||
|
||||
test('fix 2: a finding counted in round 1 and backed in round 2 (same id, marked resolved: false) is listed once and counts nowhere', () => {
|
||||
const t1 = buildPlan();
|
||||
const r1 = rec(t1, dd({ pc: [f('blocker', at(t1, R1_CHANGES), S1, 'SECURITY_RISK')] }));
|
||||
const id = r1.verdict.findings[0].id;
|
||||
const t2 = editA(r1.text);
|
||||
const r2 = rec(t2, dd({
|
||||
pc: [f('blocker', at(t2, R1_CHANGES), S1, 'SECURITY_RISK', { threat_model_ref: OUTSIDE })],
|
||||
pcPrev: [{ id, resolved: false }], sgPrev: [{ id, resolved: false }],
|
||||
}), { base: baseOf(r1.text), threatModel: TM, briefPath: 'brief.md' });
|
||||
const v = r2.verdict;
|
||||
assert.deepEqual(v.findings.map(x => x.id), [], 'the carried copy left the counted set');
|
||||
assert.deepEqual(v.counts, { blocker: 0, major: 0, minor: 0 });
|
||||
assert.equal(v.verdict, 'PASS');
|
||||
assert.deepEqual((v.known_outside ?? []).map(e => e.id), [id]);
|
||||
assert.equal(v.rounds.at(-1).blocking, 0, 'it left the stop-rule summary');
|
||||
assert.equal(v.stop_rule.fired, false, JSON.stringify(v.stop_rule));
|
||||
assert.equal(gatePlan(r2.text, { threatModel: TM }).code, 0);
|
||||
});
|
||||
|
||||
test('fix 3: a merged finding whose security key is only in rule_keys is backed at record and in the bare gate alike', () => {
|
||||
const t1 = buildPlan();
|
||||
const l = at(t1, R1_CHANGES);
|
||||
const d = dd({
|
||||
pc: [f('blocker', l, S1, 'injection/shell', { threat_model_ref: OUTSIDE })],
|
||||
sg: [f('blocker', l, S1, 'SECURITY_RISK', { threat_model_ref: OUTSIDE })],
|
||||
});
|
||||
assert.equal(d.findings.length, 1, 'dedup merged the two');
|
||||
const r = rec(t1, d, { threatModel: TM, briefPath: 'brief.md' });
|
||||
assert.equal(r.verdict.known_outside?.length, 1);
|
||||
assert.deepEqual(r.verdict.known_outside[0].rule_keys, ['injection/shell', 'SECURITY_RISK']);
|
||||
const g = gatePlan(r.text, { threatModel: TM });
|
||||
assert.equal(g.code, 0, JSON.stringify(g.reasons));
|
||||
assert.ok(g.warnings.some(w => w.code === 'PLAN_GATE_KNOWN_OUTSIDE'), JSON.stringify(g.warnings));
|
||||
// extractVerdict accepts rule_keys and refuses a malformed one.
|
||||
const v = extractVerdict(r.text).verdict;
|
||||
const bad = r.text.replace(/```json\n[\s\S]*?\n```/, () => '```json\n' + JSON.stringify({ ...v, known_outside: [{ ...v.known_outside[0], rule_keys: [7] }] }, null, 2) + '\n```');
|
||||
assert.ok(extractVerdict(bad).error, 'a rule_keys that is not a list of strings is malformed');
|
||||
});
|
||||
|
||||
test('fix 4: a reviewer\'s mark on a known_outside id passes dedup and the count, and resolved: true removes the entry (diff mode and full count)', () => {
|
||||
const r1 = backedRound1();
|
||||
const id = r1.verdict.known_outside?.[0]?.id;
|
||||
assert.ok(id, 'round 1 lists the backed finding');
|
||||
const t2 = editA(r1.text);
|
||||
const d = dd({ pcPrev: [{ id, resolved: true }], sgPrev: [{ id, resolved: true }] });
|
||||
assert.deepEqual(d.marks, [{ agent: 'plan-critic', id, resolved: true }, { agent: 'scope-guardian', id, resolved: true }]);
|
||||
for (const opts of [{ base: baseOf(r1.text) }, { baseNote: 'no snapshot; full counting' }]) {
|
||||
const r2 = recordVerdict(t2, d, { reviewed: planHash(t2), planPaths: PATHS, now: NOW, ...opts });
|
||||
assert.equal(r2.code, 0, r2.message);
|
||||
assert.equal(r2.verdict.known_outside, undefined, JSON.stringify(opts));
|
||||
}
|
||||
});
|
||||
|
||||
// ---- Handover 4 ---------------------------------------------------------------------
|
||||
|
||||
test('Handover 4 names --brief, known_outside, brief_path and the two new codes', () => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue