diff --git a/CHANGELOG.md b/CHANGELOG.md index e2390dc02..0e549ecaf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,37 @@ # Changelog +## [1.87.3.0] - 2026-09-15 + +**Changed code needs another pass.** +**Review freshness now checks both ends.** + +A green review now stays attached to the code that was there when the pass began. `/review` and `/ship` capture that content before reading it, then compare again when the pass finishes. If fixes changed the files, another pass has to review those fixes before the dashboard can call the result CURRENT. Older log-only records remain visible, but cannot stand in for a completed code review. + +A clean result requires a completed pass on unchanged content with no unresolved findings. A stopped or nonconverged run cannot clear the code-review row just because its commit has not moved. + +### The three numbers that matter + +Source: scenarios in `test/review-start-evidence.test.ts`, checked against released v1.87.0.0 and this version. Run `bun test test/review-start-evidence.test.ts` to verify current behavior. These are false CURRENT grades across five deterministic cases, not production incident estimates. + +| Cases incorrectly graded CURRENT | Before | After | Δ | +|---|---:|---:|---:| +| Mid-review edits: tracked and untracked source | 2 | 0 | -2 | +| Log-only result without a captured review start | 1 | 0 | -1 | +| Codex advisory findings: none or only some resolved | 2 | 0 | -2 | + +The old stamp could certify a review after its own fixes changed the tree. Those cases now stay STALE or UNVERIFIED, including an advisory-only Codex pass that leaves findings unresolved. + +### What this means for developers + +You can distinguish a completed, unchanged pass from a run that still needs attention before merging. Plan reviews keep their existing rules; they assess the plan rather than the checked-out source. Completion is still reviewer-reported, not proof that an LLM read every file. Run `/review` again after fixes and use the new result when preparing to ship. + +### Itemized changes + +#### Fixed + +- **Review freshness no longer certifies unreviewed fixes.** `/review` and `/ship` bind each diff pass to the content captured before it starts. Edits during review, incomplete passes, and older log-only records stay stale or unverified in the readiness dashboard and `/land-and-deploy`, even when HEAD has not moved. Plan-review evidence keeps its existing freshness rules. +- **Codex readiness keeps unresolved findings visible.** Passing the critical-findings gate does not make a review CURRENT while advisory findings remain open. The gate's severity policy is unchanged. + ## [1.87.2.0] - 2026-09-15 **Headless commands stop closing your logged-in browser.** diff --git a/README.md b/README.md index c62770881..0dfc52212 100644 --- a/README.md +++ b/README.md @@ -285,6 +285,8 @@ Beyond the slash-command skills, gstack ships standalone CLIs for workflows that | `gstack-verify-gate` | **Verification stop hook (opt-in)** — blocks a Claude Code turn from ending until the project's declared verify command passes (after 3 blocked re-entries it yields with a loud still-RED warning instead of looping forever). Declare it on one line in CLAUDE.md: ``. Hooks bypass the permission system, so a declared command never runs until you trust it once per repo (`gstack-verify-gate --trust`); editing the command invalidates trust until re-granted, and every grant is audit-logged. `./setup` never registers it for you — opt in with `gstack-settings-hook add-event --event Stop --command ~/.claude/skills/gstack/bin/gstack-verify-gate --source verify-gate`, remove with `gstack-settings-hook remove-source --source verify-gate`. | | `gstack-memorable` | **Memorable recall bridge (opt-in, third party, Claude Code only)** — connects Claude Code to the external [Memorable](https://memorable.sh) CLI *through gstack* instead of the vendor's own installer, so the hook gets gstack's guarantees: an explicit consent key (`memorable_recall`, off by default, listed by `gstack-egress grants`), a fail-closed egress receipt for every prompt handed over (`gstack-egress list --sink memorable-recall`), a HIGH-tier secret pre-scan, a trust envelope and 8 KiB cap on whatever comes back, an allowlisted environment and process-group containment for the vendor process, and clean removal. `enable` registers the hook at the stable install with a 5 s timeout and never runs the vendor's own consent command; `disable` revokes the gate first and removes the entry by identity even after Claude Code strips the tag; `status` is read-only. gstack never installs Memorable, and what its binary sends is the vendor's claim, not gstack's. Not available on Windows yet. [Full guide](docs/memorable-workflow-memory.md). | | `gstack-wtree` | **Working-tree fingerprint** — prints a content hash of what's actually on disk (temp index seeded from the stat cache, ~40x cheaper than a full re-hash; untracked source counts, gitignored scratch doesn't). Identical content fingerprints identically through commits, rebases, amends, and squashes — it's what binds reviews and test evidence to content instead of commit SHAs. | +| `gstack-review-log` | **Review-pass receipts** — `--start ` captures the working-tree fingerprint before a diff review; `'' --finish ` consumes that single-use, repository/branch/skill-scoped receipt. Binding requires matching start/end content and reviewer-reported `completed:true` and `converged:true`; it is not independent proof that a model read the source. | +| `gstack-review-read` | **Review freshness** — emits review records with computed `review_freshness.status` and `reason`: CURRENT, STALE, or UNVERIFIED for diff reviews. `/ship` and `/land-and-deploy` use the same grade; a matching commit alone never certifies a diff review. [Dashboard rules](docs/skills.md#review-readiness-dashboard). | | `gstack-evidence` | **Verification-evidence ledger** — `run --label -- ` transparently wraps any test command (the child's exit code always passes through) and records what ran against which working-tree fingerprint; `check` grades each label FRESH/STALE/MISSING with `--expect-cmd`, `--max-age`, and `--allow-paths` binding. /ship and /land-and-deploy cite fresh evidence instead of re-running suites. Per-run logs are 0600, capped at 2MB, pruned after 30 days; the ledger and logs stay machine-local by design. | | `gstack-issue-guard` | **Tracker-text trust envelope** — fetches GitHub issue/PR text (`issue `, `pr-body`, `pr-comments`, or `--stdin`) and wraps it in a labeled envelope so agents treat it as data: injection-shaped lines get labeled even through fullwidth and invisible-character evasion, and forged envelope banners are defused. Every tracker-text ingress in gstack routes through it, enforced by a CI scanner. | | `gstack-ios-qa-daemon` | **iOS QA daemon** — Mac-side broker between an agent and a connected iPhone over USB CoreDevice. Loopback by default; `--tailnet` opens a Tailscale-facing listener with identity-gated capability tiers. Single-instance via flock on `~/.gstack/ios-qa-daemon.pid`. See [docs/howto-ios-testing-with-gstack.md](docs/howto-ios-testing-with-gstack.md). | diff --git a/VERSION b/VERSION index 30478137d..01aa5a296 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.87.2.0 +1.87.3.0 diff --git a/agents-digest/gstack-AGENTS.md b/agents-digest/gstack-AGENTS.md index f81c28aca..98f0816ff 100644 --- a/agents-digest/gstack-AGENTS.md +++ b/agents-digest/gstack-AGENTS.md @@ -1,4 +1,4 @@ -# gstack digest v1.87.2.0 — regenerate/re-copy after upgrading gstack +# gstack digest v1.87.3.0 — regenerate/re-copy after upgrading gstack Behavioral rules from gstack (https://github.com/garrytan/gstack), compressed for agent hosts without a full skill install. The full skills add workflows, diff --git a/bin/gstack-review-log b/bin/gstack-review-log index 4448a5318..5a5b29d12 100755 --- a/bin/gstack-review-log +++ b/bin/gstack-review-log @@ -2,27 +2,31 @@ # gstack-review-log — atomically log a review result # Usage: gstack-review-log '{"skill":"...","timestamp":"...","status":"..."}' # -# Binding fields (content-addressed staleness): every appended record is -# stamped with commit_full, tree, dirty (informational) and wtree (the GATING -# working-tree fingerprint from bin/gstack-wtree). These are computed -# AUTHORITATIVELY here — caller-supplied values for the four keys are ignored, -# so a stale rendered template (or a forged field) cannot bind a record to -# content it wasn't made on. All other caller fields pass through untouched. -# Outside a git repo the fields are simply omitted (legacy consumers fall back -# to their heuristics). -# -# Known limitation: binding happens at LOG time, not review-START time — edits -# made between finishing a review and logging it (including fixes the review -# itself applied) are certified by the stamped fingerprint. gstack-evidence -# closes this window for test runs (before/after capture); review flows log -# immediately after reviewing, which keeps the window small but nonzero. +# Before reading a diff: gstack-review-log --start review +# After that pass: gstack-review-log '{...,"completed":true,"converged":true}' --finish TOKEN +# Diff reviews get wtree only from a consumed, matching start capture on an +# unchanged tree. Completion/convergence are reviewer-reported, not proof that +# a model read the code. Caller-supplied binding fields are always discarded. +# Plan-tier rows retain their legacy binding behavior. set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" eval "$("$SCRIPT_DIR/gstack-slug" 2>/dev/null)" GSTACK_HOME="${GSTACK_HOME:-$HOME/.gstack}" mkdir -p "$GSTACK_HOME/projects/$SLUG" -INPUT="$1" +INPUT="${1:-}" +export GSTACK_REVIEW_DIR="$GSTACK_HOME/projects/$SLUG" +export GSTACK_REVIEW_REPO="$(git rev-parse --show-toplevel 2>/dev/null || true)" +export GSTACK_REVIEW_BRANCH="$(git symbolic-ref --short HEAD 2>/dev/null || git rev-parse HEAD 2>/dev/null || true)" +export GSTACK_REVIEW_LIB="$SCRIPT_DIR/../lib/review-evidence.ts" +case "$(uname -s)" in + MINGW*|MSYS*|CYGWIN*) + if command -v cygpath >/dev/null 2>&1; then + GSTACK_REVIEW_LIB="$(cygpath -m "$GSTACK_REVIEW_LIB")" + GSTACK_REVIEW_DIR="$(cygpath -m "$GSTACK_REVIEW_DIR")" + fi + ;; +esac # Compute binding fields (best-effort; empty outside a git repo). COMMIT_FULL=$(git rev-parse HEAD 2>/dev/null || true) @@ -39,17 +43,26 @@ if [ -n "$COMMIT_FULL" ]; then fi fi +export GSTACK_STAMP_COMMIT_FULL="$COMMIT_FULL" GSTACK_STAMP_TREE="$TREE" GSTACK_STAMP_WTREE="$WTREE" GSTACK_STAMP_DIRTY="$DIRTY" +if [ "$INPUT" = --start ]; then + GSTACK_REVIEW_SKILL="${2:-}" bun -e ' + const { captureReviewStart } = await import(process.env.GSTACK_REVIEW_LIB); + console.log(captureReviewStart(process.env.GSTACK_REVIEW_SKILL)); + ' + exit $? +fi +if [ "$#" -ne 1 ] && { [ "$#" -ne 3 ] || [ "${2:-}" != --finish ]; }; then + echo 'Usage: gstack-review-log JSON [--finish TOKEN] | --start SKILL' >&2 + exit 1 +fi + # Validate (reject malformed or injection attempts) AND stamp in one pass. # Caller values for the binding keys are dropped before stamping. -STAMPED=$(printf '%s' "$INPUT" | GSTACK_STAMP_COMMIT_FULL="$COMMIT_FULL" GSTACK_STAMP_TREE="$TREE" GSTACK_STAMP_WTREE="$WTREE" GSTACK_STAMP_DIRTY="$DIRTY" bun -e " +STAMPED=$(printf '%s' "$INPUT" | GSTACK_REVIEW_TOKEN="${3:-}" bun -e " +const { bindReview } = await import(process.env.GSTACK_REVIEW_LIB); const rec = JSON.parse(await Bun.stdin.text()); -for (const k of ['commit_full', 'tree', 'wtree', 'dirty']) delete rec[k]; -const env = process.env; -if (env.GSTACK_STAMP_COMMIT_FULL) rec.commit_full = env.GSTACK_STAMP_COMMIT_FULL; -if (env.GSTACK_STAMP_TREE) rec.tree = env.GSTACK_STAMP_TREE; -if (env.GSTACK_STAMP_WTREE) rec.wtree = env.GSTACK_STAMP_WTREE; -if (env.GSTACK_STAMP_DIRTY) rec.dirty = env.GSTACK_STAMP_DIRTY === 'true'; -console.log(JSON.stringify(rec)); +if (!rec || Array.isArray(rec) || typeof rec !== 'object') throw new Error('expected object'); +console.log(JSON.stringify(bindReview(rec, process.env.GSTACK_REVIEW_TOKEN))); " 2>/dev/null) || { # Not valid JSON — refuse to append echo "gstack-review-log: invalid JSON, skipping" >&2 diff --git a/bin/gstack-review-read b/bin/gstack-review-read index 8400240ee..16139f1d8 100755 --- a/bin/gstack-review-read +++ b/bin/gstack-review-read @@ -2,23 +2,42 @@ # gstack-review-read — read review log and config for dashboard # Usage: gstack-review-read # -# Emits, in order: the raw reviews JSONL, ---CONFIG--- (skip_eng_review), +# Emits, in order: reviews JSONL with computed review_freshness, ---CONFIG--- (skip_eng_review), # ---HEAD--- (short sha), ---WTREE--- (current working-tree fingerprint from # bin/gstack-wtree, or "unknown"), ---TREE--- (HEAD tree, informational) and # ---DIRTY--- (tracked-file dirty flag). Consumers grade diff-scoped review -# rows CURRENT when a record's `wtree` equals ---WTREE---; everything needed -# for that rule ships in this one output so graders run no extra commands. +# rows using review_freshness; legacy or uncaptured diff evidence cannot fall +# back to HEAD equality. Plan rows are unchanged. set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +case "$(uname -s)" in + MINGW*|MSYS*|CYGWIN*) command -v cygpath >/dev/null 2>&1 && SCRIPT_DIR="$(cygpath -m "$SCRIPT_DIR")" ;; +esac eval "$("$SCRIPT_DIR/gstack-slug" 2>/dev/null)" GSTACK_HOME="${GSTACK_HOME:-$HOME/.gstack}" -cat "$GSTACK_HOME/projects/$SLUG/$BRANCH-reviews.jsonl" 2>/dev/null || echo "NO_REVIEWS" +WTREE=$("$SCRIPT_DIR/gstack-wtree" 2>/dev/null || echo "unknown") +if [ -f "$GSTACK_HOME/projects/$SLUG/$BRANCH-reviews.jsonl" ]; then + GSTACK_REVIEW_LIB="$SCRIPT_DIR/../lib/review-evidence.ts" GSTACK_REVIEW_WTREE="$WTREE" bun -e ' + const { reviewFreshness } = await import(process.env.GSTACK_REVIEW_LIB); + for (const line of (await Bun.stdin.text()).split("\n").filter(Boolean)) { + try { + const rec = JSON.parse(line); + delete rec.review_freshness; + const freshness = reviewFreshness(rec, process.env.GSTACK_REVIEW_WTREE); + if (freshness) rec.review_freshness = freshness; + console.log(JSON.stringify(rec)); + } catch { console.error("gstack-review-read: skipping malformed review row"); } + } + ' < "$GSTACK_HOME/projects/$SLUG/$BRANCH-reviews.jsonl" +else + echo "NO_REVIEWS" +fi echo "---CONFIG---" "$SCRIPT_DIR/gstack-config" get skip_eng_review 2>/dev/null || echo "false" echo "---HEAD---" git rev-parse --short HEAD 2>/dev/null || echo "unknown" echo "---WTREE---" -"$SCRIPT_DIR/gstack-wtree" 2>/dev/null || echo "unknown" +echo "$WTREE" echo "---TREE---" git rev-parse 'HEAD^{tree}' 2>/dev/null || echo "unknown" echo "---DIRTY---" diff --git a/codex/sections/review-mode.md b/codex/sections/review-mode.md index 51cf6fde2..84dc1f617 100644 --- a/codex/sections/review-mode.md +++ b/codex/sections/review-mode.md @@ -32,6 +32,8 @@ TMPERR=$(mktemp "$TMP_ROOT/codex-err-XXXXXX") 2. Run the review. No prompt argument — scope comes from `--base` (or `--commit ` when reviewing a single commit, or `--uncommitted` for the working tree). +Use only one command path below. Remember its printed start token as CODEX_REVIEW_START before the review reads or receives the diff. Capture a new token only before a genuine rerun, never just to log fixes. + **Sandbox is pinned read-only via config override.** Top-level `codex review` has no `-s`/`--sandbox` flag (verified on 0.147.0: `codex review --help` lists none), so the read-only sandbox is set with `-c 'sandbox_mode="read-only"'` — the same form the @@ -42,6 +44,7 @@ contradicting this skill's read-only contract (#2496, #2524): ```bash _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } cd "$_REPO_ROOT" +~/.claude/skills/gstack/bin/gstack-review-log --start codex-review # The 330s wrapper sits BELOW the 360s Bash gate so the wrapper fires FIRST # and a stall surfaces as a diagnosable exit 124 with an explicit message, # never as a silent harness kill that downstream reads as "no findings". @@ -76,6 +79,7 @@ adversarial: ```bash _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } cd "$_REPO_ROOT" +~/.claude/skills/gstack/bin/gstack-review-log --start codex-review _USER_INSTRUCTIONS="" _PROMPT_FILE=$(mktemp "$TMP_ROOT/codex-prompt-XXXXXX") { @@ -191,13 +195,14 @@ CROSS-MODEL ANALYSIS: 7. Persist the review result: ```bash -~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"codex-review","timestamp":"TIMESTAMP","status":"STATUS","gate":"GATE","findings":N,"findings_fixed":N,"commit":"'"$(git rev-parse --short HEAD)"'"}' +~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"codex-review","timestamp":"TIMESTAMP","status":"STATUS","gate":"GATE","findings":N,"findings_fixed":N,"commit":"'"$(git rev-parse --short HEAD)"'","completed":COMPLETED,"converged":CONVERGED}' --finish CODEX_REVIEW_START ``` Substitute: TIMESTAMP (ISO 8601), STATUS ("clean" if PASS, "issues_found" if FAIL), GATE ("pass" or "fail" — fail-closed verdicts log as "fail"), findings (count of [P0] + [P1] + [P2] markers; 0 for fail-closed runs, which reviewed nothing), findings_fixed (count of findings that were addressed/fixed before shipping). +CODEX_REVIEW_START is the original token from the command path that ran. COMPLETED is true only when the review completed with coverage of the branch diff and current working-tree changes; timeout, failure, refusal, or missing coverage is false. A limited `--commit`/`--uncommitted` review, or a committed-only custom prompt that omitted dirty/untracked source, does not establish whole-branch coverage: log completed false and explain the limitation. CONVERGED is true only for a completed pass with zero edits. Fixes stay stale until a genuine rerun reads the updated diff with a new start token. These evidence fields do not change the gate verdict above. 8. Clean up temp files: ```bash diff --git a/codex/sections/review-mode.md.tmpl b/codex/sections/review-mode.md.tmpl index 7ef603dd3..7f1e92d13 100644 --- a/codex/sections/review-mode.md.tmpl +++ b/codex/sections/review-mode.md.tmpl @@ -30,6 +30,8 @@ TMPERR=$(mktemp "$TMP_ROOT/codex-err-XXXXXX") 2. Run the review. No prompt argument — scope comes from `--base` (or `--commit ` when reviewing a single commit, or `--uncommitted` for the working tree). +Use only one command path below. Remember its printed start token as CODEX_REVIEW_START before the review reads or receives the diff. Capture a new token only before a genuine rerun, never just to log fixes. + **Sandbox is pinned read-only via config override.** Top-level `codex review` has no `-s`/`--sandbox` flag (verified on 0.147.0: `codex review --help` lists none), so the read-only sandbox is set with `-c 'sandbox_mode="read-only"'` — the same form the @@ -40,6 +42,7 @@ contradicting this skill's read-only contract (#2496, #2524): ```bash _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } cd "$_REPO_ROOT" +~/.claude/skills/gstack/bin/gstack-review-log --start codex-review # The 330s wrapper sits BELOW the 360s Bash gate so the wrapper fires FIRST # and a stall surfaces as a diagnosable exit 124 with an explicit message, # never as a silent harness kill that downstream reads as "no findings". @@ -74,6 +77,7 @@ adversarial: ```bash _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } cd "$_REPO_ROOT" +~/.claude/skills/gstack/bin/gstack-review-log --start codex-review _USER_INSTRUCTIONS="" _PROMPT_FILE=$(mktemp "$TMP_ROOT/codex-prompt-XXXXXX") { @@ -189,13 +193,14 @@ CROSS-MODEL ANALYSIS: 7. Persist the review result: ```bash -~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"codex-review","timestamp":"TIMESTAMP","status":"STATUS","gate":"GATE","findings":N,"findings_fixed":N,"commit":"'"$(git rev-parse --short HEAD)"'"}' +~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"codex-review","timestamp":"TIMESTAMP","status":"STATUS","gate":"GATE","findings":N,"findings_fixed":N,"commit":"'"$(git rev-parse --short HEAD)"'","completed":COMPLETED,"converged":CONVERGED}' --finish CODEX_REVIEW_START ``` Substitute: TIMESTAMP (ISO 8601), STATUS ("clean" if PASS, "issues_found" if FAIL), GATE ("pass" or "fail" — fail-closed verdicts log as "fail"), findings (count of [P0] + [P1] + [P2] markers; 0 for fail-closed runs, which reviewed nothing), findings_fixed (count of findings that were addressed/fixed before shipping). +CODEX_REVIEW_START is the original token from the command path that ran. COMPLETED is true only when the review completed with coverage of the branch diff and current working-tree changes; timeout, failure, refusal, or missing coverage is false. A limited `--commit`/`--uncommitted` review, or a committed-only custom prompt that omitted dirty/untracked source, does not establish whole-branch coverage: log completed false and explain the limitation. CONVERGED is true only for a completed pass with zero edits. Fixes stay stale until a genuine rerun reads the updated diff with a new start token. These evidence fields do not change the gate verdict above. 8. Clean up temp files: ```bash diff --git a/devex-review/SKILL.md b/devex-review/SKILL.md index 4eaf9af64..082a3f45c 100644 --- a/devex-review/SKILL.md +++ b/devex-review/SKILL.md @@ -952,17 +952,17 @@ Display: - **Outside Voice (default-on):** Independent plan review through the host-selected provider after /plan-ceo-review and /plan-eng-review. The codex_reviews switch disables the entire extra step. Provider failure uses the existing native fallback and reports missing outside coverage. Never gates shipping. **Verdict logic:** -- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean" (or \`skip_eng_review\` is \`true\`) +- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean"; diff review must also grade CURRENT below (or \`skip_eng_review\` is \`true\`) - **NOT CLEARED**: Eng Review missing, stale (>7 days), or has open issues - CEO, Design, and outside reviews are shown for context but never block shipping - If \`skip_eng_review\` config is \`true\`, Eng Review shows "SKIPPED (global)" and verdict is CLEARED -**Staleness detection:** After displaying the dashboard, check if any existing reviews may be stale: -- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-review\`, \`codex-review\`, ship-stage entries).** Parse the \`---WTREE---\` and \`---DIRTY---\` sections from the bash output. If an entry has a \`wtree\` field AND it equals the current \`---WTREE---\` value, the review is CURRENT — identical content, regardless of commit count, rebase, amend, or whether it was committed yet (wtree equality alone proves identical content; that is the keystone property). Skip the commit-count heuristic for that entry and show no staleness note. -- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If such an entry carries a \`plan_sha256\` field, you MAY compare it against the current plan file's sha256 and note "plan changed since review" on mismatch. -- Fallback (no \`wtree\` on the entry, or wtree mismatch): parse the \`---HEAD---\` section to get the current HEAD commit hash. For each review entry that has a \`commit\` field: compare it against the current HEAD. If different, count elapsed commits: \`git rev-list --count STORED_COMMIT..HEAD\`. If that command FAILS (the stored commit was rebased away), grade UNKNOWN and treat as stale — do not error. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" -- For entries without a \`commit\` field (legacy entries): display "Note: {skill} review from {date} has no commit tracking — consider re-running for accurate staleness detection" -- If all reviews grade CURRENT (wtree match or HEAD match), do not display any staleness notes +**Staleness detection:** Grade before deciding CLEARED: +- Ship telemetry reports metrics, not review coverage; it never satisfies a review row. +- **Content-first rule (diff-scoped rows only: `review`, `adversarial-review`, `codex-review`, ship-stage entries, `design-review-lite`).** Use the helper's computed `review_freshness.status` and show its `reason`. CURRENT requires a completed clean pass with captured start/end wtree equal to the current `---WTREE---`. STALE or UNVERIFIED never clears Eng Review. Missing `review_freshness` is UNVERIFIED, including legacy log-only rows. Never fall back to HEAD equality or commit distance for diff evidence, even at 0 commits. Show recorded cycles, completed/converged state, and missing per-source/phase coverage; unknown is not a pass. +- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review, codex-plan-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If an entry carries `plan_sha256`, you MAY compare it with the plan file and note "plan changed since review" on mismatch. +- Plan-tier fallback only: parse `---HEAD---`. For entries with a different `commit`, count elapsed commits: `git rev-list --count STORED_COMMIT..HEAD`. If that command FAILS, grade UNKNOWN and treat as stale. Display: "Note: {skill} review from {date} may be stale — {N} commits since review". Missing commit tracking retains the legacy note to consider re-running. +- If all reviews grade CURRENT, do not display staleness notes ## Plan File Review Report diff --git a/docs/PROJECT_STRUCTURE.md b/docs/PROJECT_STRUCTURE.md index f60acb04d..4d374b7d3 100644 --- a/docs/PROJECT_STRUCTURE.md +++ b/docs/PROJECT_STRUCTURE.md @@ -81,7 +81,7 @@ gstack/ │ └── dist/ # Compiled binary ├── agents-digest/ # Committed 2KB instruction-tier rules digest (gstack-AGENTS.md) for rules-reading hosts ├── extension/ # Chrome extension (side panel + activity feed + CSS inspector) -├── lib/ # Shared libraries (aside-render.ts = local-HTML rendering, Aside first, engine fallback; design-catalog.ts = the typed design anti-pattern catalog every design skill renders from; design-detect-contract.ts = detector sentinel vocabulary; design-md.ts = open DESIGN.md reader/writer; dom-dump-script.ts + generated dom-dump.js = rendered-DOM dump for the detector; frontend-scope.ts; claude-bin.ts, error-handling.ts, worktree.ts, egress-receipt.ts, context-bill.ts, redact-engine.ts, tracker-guard.ts, version-source.ts, code-intelligence/) +├── lib/ # Shared libraries (aside-render.ts = local-HTML rendering, Aside first, engine fallback; design-catalog.ts = the typed design anti-pattern catalog every design skill renders from; design-detect-contract.ts = detector sentinel vocabulary; design-md.ts = open DESIGN.md reader/writer; dom-dump-script.ts + generated dom-dump.js = rendered-DOM dump for the detector; review-evidence.ts = review-start receipt binding and computed freshness; frontend-scope.ts; claude-bin.ts, error-handling.ts, worktree.ts, egress-receipt.ts, context-bill.ts, redact-engine.ts, tracker-guard.ts, version-source.ts, code-intelligence/) │ └── diagram-render/ # Vendored mermaid + excalidraw runtimes, built into one offline bundle the renderer loads ├── patches/ # bun `patchedDependencies` patches (playwright-core windowsHide) ├── docs/designs/ # Design documents (incl. IMPECCABLE_INTEROP.md = the design detector / catalog / open DESIGN.md record, and fork-port-residual-2026-09/ evaluation evidence) diff --git a/docs/skills.md b/docs/skills.md index 09c2a2d2c..168e2673f 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -252,6 +252,12 @@ Every review (CEO, Eng, Design) logs its result. At the end of each review, you Eng Review is the only required gate (disable with `gstack-config set skip_eng_review true`). CEO and Design are informational — recommended for product and UI changes respectively. +Diff reviews use the `review_freshness` grade computed by `gstack-review-read`, shared by `/ship` and `/land-and-deploy`. CURRENT requires a clean, reviewer-reported completed and converged pass whose captured start and finish fingerprints match the current working-tree content. Tracked edits and non-ignored untracked source both count; an identical commit hash or zero commits since review is not a fallback. + +A captured pass with different start/end content grades STALE, as does a previously verified pass whose fingerprint no longer matches. Missing or reused start receipts, legacy log-only records, incomplete or nonconverged passes, and unresolved findings cannot grade CURRENT; missing evidence grades UNVERIFIED. Ship telemetry is not a review pass. After fixes, run a genuine full re-review with a new start receipt rather than capturing one only to log the result. Completion remains reviewer-reported, not independent proof that a model read the source. + +Plan-file reviews retain their existing seven-day freshness handling and optional plan-hash comparison; repository-content rules do not apply to them. A diff review must grade CURRENT before it can clear Eng Review, in addition to the dashboard's existing age and clean-status requirements. + ### Plan-to-QA flow When `/plan-eng-review` finishes the test review section, it writes a test plan artifact to `~/.gstack/projects/`. When you later run `/qa`, it picks up that test plan automatically — your engineering review feeds directly into QA testing with no manual copy-paste. @@ -1073,6 +1079,8 @@ independent of model selection. **Review** — run `codex review` against the current diff. Codex reads every changed file, classifies findings by severity (P1 critical, P2 high, P3 medium), and returns a PASS/FAIL verdict. Any P1 finding = FAIL. The review is fully independent — Codex doesn't see Claude's review. +A severity-gate PASS is separate from [review freshness](#review-readiness-dashboard): unresolved recorded findings (`findings > findings_fixed`, with missing `findings_fixed` treated as zero) prevent CURRENT even when the gate passes. This does not change the severity gate. Fixes still require a new completed, unchanged review pass before the fixed tree can grade CURRENT. + **Challenge** — adversarial mode. Codex actively tries to break your code. It looks for edge cases, race conditions, security holes, and assumptions that would fail under load. Uses maximum reasoning effort (`xhigh`). Think of it as a penetration test for your logic. **Consult** — open conversation with session continuity. Ask Codex anything about the codebase. Follow-up questions reuse the same session, so context carries over. Great for "am I thinking about this correctly?" moments. diff --git a/land-and-deploy/sections/readiness-gate.md b/land-and-deploy/sections/readiness-gate.md index 7054accfd..7e7a137dc 100644 --- a/land-and-deploy/sections/readiness-gate.md +++ b/land-and-deploy/sections/readiness-gate.md @@ -22,20 +22,27 @@ codex-plan-review): 1. Find the most recent entry within the last 7 days. 2. **Content-first rule (diff-scoped rows only: `review`, `adversarial-review`, - `codex-review`, ship-stage entries).** If the entry has a `wtree` field AND it - equals the `---WTREE---` section of the output → **CURRENT**, full stop. - Identical working-tree content, regardless of commit count, rebase, amend, or - whether it was committed yet (wtree equality alone proves identical content) — - skip steps 3-4 for this entry. Never apply the wtree rule to plan-tier rows (plan-eng-review, - plan-ceo-review, plan-design-review): those grade a plan file, not the repo - tree — they keep the 7-day logic and the commit heuristic below. + `codex-review`, ship-stage entries, `design-review-lite`).** Use the helper's + computed `review_freshness.status` and show its `reason`. Only **CURRENT** + certifies a completed clean pass whose start/end `wtree` still matches + `---WTREE---`, regardless of commit count, rebase, or amend. + **STALE** or **UNVERIFIED** (including legacy log-only rows, missing start + captures, incomplete/nonconverged passes, and unresolved findings) is a red + review warning. If `review_freshness` is missing, grade UNVERIFIED. + Never fall back to commit distance for a diff row, even at HEAD/0 commits; + skip steps 3-4 for ALL diff rows. Show `cycles`, `completed`, `converged`, and + per-source/phase missing coverage when present; an unknown value is not a pass. + Ship telemetry reports metrics, not review coverage; it never satisfies a review row. + Plan-tier rows (plan-eng-review, plan-ceo-review, plan-design-review, + codex-plan-review) grade a plan file — they retain the 7-day logic and commit + heuristic below. Never use repo fingerprints to certify a plan. 3. Extract its `commit` field. 4. Compare against current HEAD: `git rev-list --count STORED_COMMIT..HEAD`. **If this command fails** (the stored commit was rebased away and is unreachable) → grade **UNKNOWN** and treat as STALE. Do not error out of the readiness check. -**Staleness rules (fallback path):** +**Staleness rules (plan-tier fallback only):** - 0 commits since review → CURRENT - 1-3 commits since review → RECENT (yellow if those commits touch code, not just docs) - 4+ commits since review → STALE (red — review may not reflect current code) @@ -49,8 +56,7 @@ git log --oneline STORED_COMMIT..HEAD If any commits after the review contain words like "fix", "refactor", "rewrite", "overhaul", or touch more than 5 files — flag as **STALE (significant changes since review)**. The review was done on different code than what's about to merge. -(Skip this check for entries already graded CURRENT by the content-first rule — -same content is same content.) +(Diff rows already have their computed grade; commit history cannot upgrade it.) **Also check for adversarial review (`codex-review`).** If codex-review has been run and is CURRENT, mention it in the readiness report as an extra confidence signal. @@ -58,8 +64,8 @@ If not run, note as informational (not a blocker): "No adversarial review on rec ### 3.5a-bis: Inline review offer -**We are extra careful about deploys.** If engineering review is STALE (4+ commits since) -or NOT RUN, offer to run a quick review inline before proceeding. +**We are extra careful about deploys.** If engineering review is STALE, UNVERIFIED, +UNKNOWN, or NOT RUN, offer to run a quick review inline before proceeding. Use AskUserQuestion: - **Re-ground:** "I noticed {the code review is stale / no code review has been run} on this branch. Since this code is about to go to production, I'd like to do a quick safety check on the diff before we merge. This is one of the ways I make sure nothing ships that shouldn't." diff --git a/land-and-deploy/sections/readiness-gate.md.tmpl b/land-and-deploy/sections/readiness-gate.md.tmpl index 694628096..2b39c9864 100644 --- a/land-and-deploy/sections/readiness-gate.md.tmpl +++ b/land-and-deploy/sections/readiness-gate.md.tmpl @@ -20,20 +20,27 @@ codex-plan-review): 1. Find the most recent entry within the last 7 days. 2. **Content-first rule (diff-scoped rows only: `review`, `adversarial-review`, - `codex-review`, ship-stage entries).** If the entry has a `wtree` field AND it - equals the `---WTREE---` section of the output → **CURRENT**, full stop. - Identical working-tree content, regardless of commit count, rebase, amend, or - whether it was committed yet (wtree equality alone proves identical content) — - skip steps 3-4 for this entry. Never apply the wtree rule to plan-tier rows (plan-eng-review, - plan-ceo-review, plan-design-review): those grade a plan file, not the repo - tree — they keep the 7-day logic and the commit heuristic below. + `codex-review`, ship-stage entries, `design-review-lite`).** Use the helper's + computed `review_freshness.status` and show its `reason`. Only **CURRENT** + certifies a completed clean pass whose start/end `wtree` still matches + `---WTREE---`, regardless of commit count, rebase, or amend. + **STALE** or **UNVERIFIED** (including legacy log-only rows, missing start + captures, incomplete/nonconverged passes, and unresolved findings) is a red + review warning. If `review_freshness` is missing, grade UNVERIFIED. + Never fall back to commit distance for a diff row, even at HEAD/0 commits; + skip steps 3-4 for ALL diff rows. Show `cycles`, `completed`, `converged`, and + per-source/phase missing coverage when present; an unknown value is not a pass. + Ship telemetry reports metrics, not review coverage; it never satisfies a review row. + Plan-tier rows (plan-eng-review, plan-ceo-review, plan-design-review, + codex-plan-review) grade a plan file — they retain the 7-day logic and commit + heuristic below. Never use repo fingerprints to certify a plan. 3. Extract its `commit` field. 4. Compare against current HEAD: `git rev-list --count STORED_COMMIT..HEAD`. **If this command fails** (the stored commit was rebased away and is unreachable) → grade **UNKNOWN** and treat as STALE. Do not error out of the readiness check. -**Staleness rules (fallback path):** +**Staleness rules (plan-tier fallback only):** - 0 commits since review → CURRENT - 1-3 commits since review → RECENT (yellow if those commits touch code, not just docs) - 4+ commits since review → STALE (red — review may not reflect current code) @@ -47,8 +54,7 @@ git log --oneline STORED_COMMIT..HEAD If any commits after the review contain words like "fix", "refactor", "rewrite", "overhaul", or touch more than 5 files — flag as **STALE (significant changes since review)**. The review was done on different code than what's about to merge. -(Skip this check for entries already graded CURRENT by the content-first rule — -same content is same content.) +(Diff rows already have their computed grade; commit history cannot upgrade it.) **Also check for adversarial review (`codex-review`).** If codex-review has been run and is CURRENT, mention it in the readiness report as an extra confidence signal. @@ -56,8 +62,8 @@ If not run, note as informational (not a blocker): "No adversarial review on rec ### 3.5a-bis: Inline review offer -**We are extra careful about deploys.** If engineering review is STALE (4+ commits since) -or NOT RUN, offer to run a quick review inline before proceeding. +**We are extra careful about deploys.** If engineering review is STALE, UNVERIFIED, +UNKNOWN, or NOT RUN, offer to run a quick review inline before proceeding. Use AskUserQuestion: - **Re-ground:** "I noticed {the code review is stale / no code review has been run} on this branch. Since this code is about to go to production, I'd like to do a quick safety check on the diff before we merge. This is one of the ways I make sure nothing ships that shouldn't." diff --git a/lib/review-evidence.ts b/lib/review-evidence.ts new file mode 100644 index 000000000..1f4d8a34e --- /dev/null +++ b/lib/review-evidence.ts @@ -0,0 +1,69 @@ +import { mkdirSync, readFileSync, unlinkSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; + +const DIFF_REVIEWS = new Set(['review', 'adversarial-review', 'codex-review', 'design-review-lite', 'ship']); + +export function captureReviewStart(skill: string, env = process.env): string { + if (!DIFF_REVIEWS.has(skill) || !env.GSTACK_STAMP_WTREE || !env.GSTACK_REVIEW_REPO) { + throw new Error('cannot capture a diff review without a working-tree fingerprint'); + } + const dir = join(env.GSTACK_REVIEW_DIR!, '.review-starts'); + mkdirSync(dir, { recursive: true, mode: 0o700 }); + const token = crypto.randomUUID(); + writeFileSync(join(dir, `${token}.json`), JSON.stringify({ + skill, repo: env.GSTACK_REVIEW_REPO, branch: env.GSTACK_REVIEW_BRANCH, + wtree: env.GSTACK_STAMP_WTREE, started_at: new Date().toISOString(), + }), { mode: 0o600, flag: 'wx' }); + return token; +} + +export function bindReview(rec: Record, token: string, env = process.env): Record { + for (const key of ['commit_full', 'tree', 'wtree', 'dirty', 'review_binding', 'review_freshness']) delete rec[key]; + if (env.GSTACK_STAMP_COMMIT_FULL) rec.commit_full = env.GSTACK_STAMP_COMMIT_FULL; + if (env.GSTACK_STAMP_TREE) rec.tree = env.GSTACK_STAMP_TREE; + if (env.GSTACK_STAMP_DIRTY) rec.dirty = env.GSTACK_STAMP_DIRTY === 'true'; + if (!DIFF_REVIEWS.has(rec.skill)) { + if (env.GSTACK_STAMP_WTREE) rec.wtree = env.GSTACK_STAMP_WTREE; + return rec; + } + + let start; + if (/^[0-9a-f-]{36}$/.test(token)) { + const file = join(env.GSTACK_REVIEW_DIR!, '.review-starts', `${token}.json`); + try { + const saved = readFileSync(file, 'utf8'); + unlinkSync(file); + const parsed = JSON.parse(saved); + if (parsed.skill === rec.skill && parsed.repo === env.GSTACK_REVIEW_REPO && + parsed.branch === env.GSTACK_REVIEW_BRANCH && parsed.wtree) start = parsed; + } catch (error: any) { + if (error.code !== 'ENOENT') console.error(`gstack-review-log: cannot consume review start: ${error.message}`); + } + } + const end = env.GSTACK_STAMP_WTREE; + const state = !start || !end ? 'uncaptured' + : start.wtree !== end ? 'changed' + : rec.completed !== true || rec.converged !== true ? 'incomplete' : 'verified'; + rec.review_binding = { state, start_wtree: start?.wtree, end_wtree: end, started_at: start?.started_at }; + if (state === 'verified') rec.wtree = end; + return rec; +} + +export function reviewFreshness(rec: Record, currentWtree: string): { status: string; reason: string } | undefined { + if (!DIFF_REVIEWS.has(rec.skill)) return; + if (rec.skill === 'ship') return { status: 'UNVERIFIED', reason: 'ship telemetry is not a review pass' }; + const binding = rec.review_binding; + if (binding?.state === 'changed') return { status: 'STALE', reason: 'content changed during review' }; + if (binding?.state !== 'verified' || rec.completed !== true || rec.converged !== true || + !rec.wtree || binding.start_wtree !== rec.wtree || binding.end_wtree !== rec.wtree) { + return { status: 'UNVERIFIED', reason: 'missing start capture or incomplete/nonconverged pass' }; + } + if (!currentWtree || currentWtree === 'unknown' || rec.wtree !== currentWtree) { + return { status: 'STALE', reason: 'working-tree content differs from reviewed content' }; + } + if (rec.status !== 'clean' || rec.issues_found > 0 || rec.critical > 0 || + (rec.skill === 'codex-review' && rec.findings > (rec.findings_fixed ?? 0))) { + return { status: 'UNVERIFIED', reason: 'review has unresolved findings or did not finish clean' }; + } + return { status: 'CURRENT', reason: 'completed clean pass on unchanged content' }; +} diff --git a/package.json b/package.json index 9de176de5..cdd3a7883 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gstack", - "version": "1.87.2", + "version": "1.87.3", "description": "Garry's Stack — Claude Code skills + fast headless browser. One repo, one install, entire AI engineering workflow.", "license": "MIT", "type": "module", diff --git a/plan-ceo-review/sections/review-sections.md b/plan-ceo-review/sections/review-sections.md index fedab95cf..228b518ba 100644 --- a/plan-ceo-review/sections/review-sections.md +++ b/plan-ceo-review/sections/review-sections.md @@ -834,17 +834,17 @@ Display: - **Outside Voice (default-on):** Independent plan review through the host-selected provider after /plan-ceo-review and /plan-eng-review. The codex_reviews switch disables the entire extra step. Provider failure uses the existing native fallback and reports missing outside coverage. Never gates shipping. **Verdict logic:** -- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean" (or \`skip_eng_review\` is \`true\`) +- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean"; diff review must also grade CURRENT below (or \`skip_eng_review\` is \`true\`) - **NOT CLEARED**: Eng Review missing, stale (>7 days), or has open issues - CEO, Design, and outside reviews are shown for context but never block shipping - If \`skip_eng_review\` config is \`true\`, Eng Review shows "SKIPPED (global)" and verdict is CLEARED -**Staleness detection:** After displaying the dashboard, check if any existing reviews may be stale: -- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-review\`, \`codex-review\`, ship-stage entries).** Parse the \`---WTREE---\` and \`---DIRTY---\` sections from the bash output. If an entry has a \`wtree\` field AND it equals the current \`---WTREE---\` value, the review is CURRENT — identical content, regardless of commit count, rebase, amend, or whether it was committed yet (wtree equality alone proves identical content; that is the keystone property). Skip the commit-count heuristic for that entry and show no staleness note. -- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If such an entry carries a \`plan_sha256\` field, you MAY compare it against the current plan file's sha256 and note "plan changed since review" on mismatch. -- Fallback (no \`wtree\` on the entry, or wtree mismatch): parse the \`---HEAD---\` section to get the current HEAD commit hash. For each review entry that has a \`commit\` field: compare it against the current HEAD. If different, count elapsed commits: \`git rev-list --count STORED_COMMIT..HEAD\`. If that command FAILS (the stored commit was rebased away), grade UNKNOWN and treat as stale — do not error. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" -- For entries without a \`commit\` field (legacy entries): display "Note: {skill} review from {date} has no commit tracking — consider re-running for accurate staleness detection" -- If all reviews grade CURRENT (wtree match or HEAD match), do not display any staleness notes +**Staleness detection:** Grade before deciding CLEARED: +- Ship telemetry reports metrics, not review coverage; it never satisfies a review row. +- **Content-first rule (diff-scoped rows only: `review`, `adversarial-review`, `codex-review`, ship-stage entries, `design-review-lite`).** Use the helper's computed `review_freshness.status` and show its `reason`. CURRENT requires a completed clean pass with captured start/end wtree equal to the current `---WTREE---`. STALE or UNVERIFIED never clears Eng Review. Missing `review_freshness` is UNVERIFIED, including legacy log-only rows. Never fall back to HEAD equality or commit distance for diff evidence, even at 0 commits. Show recorded cycles, completed/converged state, and missing per-source/phase coverage; unknown is not a pass. +- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review, codex-plan-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If an entry carries `plan_sha256`, you MAY compare it with the plan file and note "plan changed since review" on mismatch. +- Plan-tier fallback only: parse `---HEAD---`. For entries with a different `commit`, count elapsed commits: `git rev-list --count STORED_COMMIT..HEAD`. If that command FAILS, grade UNKNOWN and treat as stale. Display: "Note: {skill} review from {date} may be stale — {N} commits since review". Missing commit tracking retains the legacy note to consider re-running. +- If all reviews grade CURRENT, do not display staleness notes ## Plan File Review Report diff --git a/plan-design-review/sections/review-sections.md b/plan-design-review/sections/review-sections.md index a08446641..4c938c47d 100644 --- a/plan-design-review/sections/review-sections.md +++ b/plan-design-review/sections/review-sections.md @@ -477,17 +477,17 @@ Display: - **Outside Voice (default-on):** Independent plan review through the host-selected provider after /plan-ceo-review and /plan-eng-review. The codex_reviews switch disables the entire extra step. Provider failure uses the existing native fallback and reports missing outside coverage. Never gates shipping. **Verdict logic:** -- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean" (or \`skip_eng_review\` is \`true\`) +- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean"; diff review must also grade CURRENT below (or \`skip_eng_review\` is \`true\`) - **NOT CLEARED**: Eng Review missing, stale (>7 days), or has open issues - CEO, Design, and outside reviews are shown for context but never block shipping - If \`skip_eng_review\` config is \`true\`, Eng Review shows "SKIPPED (global)" and verdict is CLEARED -**Staleness detection:** After displaying the dashboard, check if any existing reviews may be stale: -- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-review\`, \`codex-review\`, ship-stage entries).** Parse the \`---WTREE---\` and \`---DIRTY---\` sections from the bash output. If an entry has a \`wtree\` field AND it equals the current \`---WTREE---\` value, the review is CURRENT — identical content, regardless of commit count, rebase, amend, or whether it was committed yet (wtree equality alone proves identical content; that is the keystone property). Skip the commit-count heuristic for that entry and show no staleness note. -- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If such an entry carries a \`plan_sha256\` field, you MAY compare it against the current plan file's sha256 and note "plan changed since review" on mismatch. -- Fallback (no \`wtree\` on the entry, or wtree mismatch): parse the \`---HEAD---\` section to get the current HEAD commit hash. For each review entry that has a \`commit\` field: compare it against the current HEAD. If different, count elapsed commits: \`git rev-list --count STORED_COMMIT..HEAD\`. If that command FAILS (the stored commit was rebased away), grade UNKNOWN and treat as stale — do not error. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" -- For entries without a \`commit\` field (legacy entries): display "Note: {skill} review from {date} has no commit tracking — consider re-running for accurate staleness detection" -- If all reviews grade CURRENT (wtree match or HEAD match), do not display any staleness notes +**Staleness detection:** Grade before deciding CLEARED: +- Ship telemetry reports metrics, not review coverage; it never satisfies a review row. +- **Content-first rule (diff-scoped rows only: `review`, `adversarial-review`, `codex-review`, ship-stage entries, `design-review-lite`).** Use the helper's computed `review_freshness.status` and show its `reason`. CURRENT requires a completed clean pass with captured start/end wtree equal to the current `---WTREE---`. STALE or UNVERIFIED never clears Eng Review. Missing `review_freshness` is UNVERIFIED, including legacy log-only rows. Never fall back to HEAD equality or commit distance for diff evidence, even at 0 commits. Show recorded cycles, completed/converged state, and missing per-source/phase coverage; unknown is not a pass. +- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review, codex-plan-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If an entry carries `plan_sha256`, you MAY compare it with the plan file and note "plan changed since review" on mismatch. +- Plan-tier fallback only: parse `---HEAD---`. For entries with a different `commit`, count elapsed commits: `git rev-list --count STORED_COMMIT..HEAD`. If that command FAILS, grade UNKNOWN and treat as stale. Display: "Note: {skill} review from {date} may be stale — {N} commits since review". Missing commit tracking retains the legacy note to consider re-running. +- If all reviews grade CURRENT, do not display staleness notes ## Plan File Review Report diff --git a/plan-devex-review/sections/review-sections.md b/plan-devex-review/sections/review-sections.md index cc71ae953..7be194580 100644 --- a/plan-devex-review/sections/review-sections.md +++ b/plan-devex-review/sections/review-sections.md @@ -721,17 +721,17 @@ Display: - **Outside Voice (default-on):** Independent plan review through the host-selected provider after /plan-ceo-review and /plan-eng-review. The codex_reviews switch disables the entire extra step. Provider failure uses the existing native fallback and reports missing outside coverage. Never gates shipping. **Verdict logic:** -- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean" (or \`skip_eng_review\` is \`true\`) +- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean"; diff review must also grade CURRENT below (or \`skip_eng_review\` is \`true\`) - **NOT CLEARED**: Eng Review missing, stale (>7 days), or has open issues - CEO, Design, and outside reviews are shown for context but never block shipping - If \`skip_eng_review\` config is \`true\`, Eng Review shows "SKIPPED (global)" and verdict is CLEARED -**Staleness detection:** After displaying the dashboard, check if any existing reviews may be stale: -- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-review\`, \`codex-review\`, ship-stage entries).** Parse the \`---WTREE---\` and \`---DIRTY---\` sections from the bash output. If an entry has a \`wtree\` field AND it equals the current \`---WTREE---\` value, the review is CURRENT — identical content, regardless of commit count, rebase, amend, or whether it was committed yet (wtree equality alone proves identical content; that is the keystone property). Skip the commit-count heuristic for that entry and show no staleness note. -- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If such an entry carries a \`plan_sha256\` field, you MAY compare it against the current plan file's sha256 and note "plan changed since review" on mismatch. -- Fallback (no \`wtree\` on the entry, or wtree mismatch): parse the \`---HEAD---\` section to get the current HEAD commit hash. For each review entry that has a \`commit\` field: compare it against the current HEAD. If different, count elapsed commits: \`git rev-list --count STORED_COMMIT..HEAD\`. If that command FAILS (the stored commit was rebased away), grade UNKNOWN and treat as stale — do not error. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" -- For entries without a \`commit\` field (legacy entries): display "Note: {skill} review from {date} has no commit tracking — consider re-running for accurate staleness detection" -- If all reviews grade CURRENT (wtree match or HEAD match), do not display any staleness notes +**Staleness detection:** Grade before deciding CLEARED: +- Ship telemetry reports metrics, not review coverage; it never satisfies a review row. +- **Content-first rule (diff-scoped rows only: `review`, `adversarial-review`, `codex-review`, ship-stage entries, `design-review-lite`).** Use the helper's computed `review_freshness.status` and show its `reason`. CURRENT requires a completed clean pass with captured start/end wtree equal to the current `---WTREE---`. STALE or UNVERIFIED never clears Eng Review. Missing `review_freshness` is UNVERIFIED, including legacy log-only rows. Never fall back to HEAD equality or commit distance for diff evidence, even at 0 commits. Show recorded cycles, completed/converged state, and missing per-source/phase coverage; unknown is not a pass. +- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review, codex-plan-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If an entry carries `plan_sha256`, you MAY compare it with the plan file and note "plan changed since review" on mismatch. +- Plan-tier fallback only: parse `---HEAD---`. For entries with a different `commit`, count elapsed commits: `git rev-list --count STORED_COMMIT..HEAD`. If that command FAILS, grade UNKNOWN and treat as stale. Display: "Note: {skill} review from {date} may be stale — {N} commits since review". Missing commit tracking retains the legacy note to consider re-running. +- If all reviews grade CURRENT, do not display staleness notes ## Plan File Review Report diff --git a/plan-eng-review/sections/review-sections.md b/plan-eng-review/sections/review-sections.md index 03ff7cfa8..4a12d9b3d 100644 --- a/plan-eng-review/sections/review-sections.md +++ b/plan-eng-review/sections/review-sections.md @@ -825,17 +825,17 @@ Display: - **Outside Voice (default-on):** Independent plan review through the host-selected provider after /plan-ceo-review and /plan-eng-review. The codex_reviews switch disables the entire extra step. Provider failure uses the existing native fallback and reports missing outside coverage. Never gates shipping. **Verdict logic:** -- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean" (or \`skip_eng_review\` is \`true\`) +- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean"; diff review must also grade CURRENT below (or \`skip_eng_review\` is \`true\`) - **NOT CLEARED**: Eng Review missing, stale (>7 days), or has open issues - CEO, Design, and outside reviews are shown for context but never block shipping - If \`skip_eng_review\` config is \`true\`, Eng Review shows "SKIPPED (global)" and verdict is CLEARED -**Staleness detection:** After displaying the dashboard, check if any existing reviews may be stale: -- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-review\`, \`codex-review\`, ship-stage entries).** Parse the \`---WTREE---\` and \`---DIRTY---\` sections from the bash output. If an entry has a \`wtree\` field AND it equals the current \`---WTREE---\` value, the review is CURRENT — identical content, regardless of commit count, rebase, amend, or whether it was committed yet (wtree equality alone proves identical content; that is the keystone property). Skip the commit-count heuristic for that entry and show no staleness note. -- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If such an entry carries a \`plan_sha256\` field, you MAY compare it against the current plan file's sha256 and note "plan changed since review" on mismatch. -- Fallback (no \`wtree\` on the entry, or wtree mismatch): parse the \`---HEAD---\` section to get the current HEAD commit hash. For each review entry that has a \`commit\` field: compare it against the current HEAD. If different, count elapsed commits: \`git rev-list --count STORED_COMMIT..HEAD\`. If that command FAILS (the stored commit was rebased away), grade UNKNOWN and treat as stale — do not error. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" -- For entries without a \`commit\` field (legacy entries): display "Note: {skill} review from {date} has no commit tracking — consider re-running for accurate staleness detection" -- If all reviews grade CURRENT (wtree match or HEAD match), do not display any staleness notes +**Staleness detection:** Grade before deciding CLEARED: +- Ship telemetry reports metrics, not review coverage; it never satisfies a review row. +- **Content-first rule (diff-scoped rows only: `review`, `adversarial-review`, `codex-review`, ship-stage entries, `design-review-lite`).** Use the helper's computed `review_freshness.status` and show its `reason`. CURRENT requires a completed clean pass with captured start/end wtree equal to the current `---WTREE---`. STALE or UNVERIFIED never clears Eng Review. Missing `review_freshness` is UNVERIFIED, including legacy log-only rows. Never fall back to HEAD equality or commit distance for diff evidence, even at 0 commits. Show recorded cycles, completed/converged state, and missing per-source/phase coverage; unknown is not a pass. +- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review, codex-plan-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If an entry carries `plan_sha256`, you MAY compare it with the plan file and note "plan changed since review" on mismatch. +- Plan-tier fallback only: parse `---HEAD---`. For entries with a different `commit`, count elapsed commits: `git rev-list --count STORED_COMMIT..HEAD`. If that command FAILS, grade UNKNOWN and treat as stale. Display: "Note: {skill} review from {date} may be stale — {N} commits since review". Missing commit tracking retains the legacy note to consider re-running. +- If all reviews grade CURRENT, do not display staleness notes ## Plan File Review Report diff --git a/review/SKILL.md b/review/SKILL.md index b89bd3d17..8d46f64c5 100644 --- a/review/SKILL.md +++ b/review/SKILL.md @@ -563,10 +563,12 @@ Compute the merge base, then diff the working tree against that point: ```bash DIFF_BASE=$(git merge-base origin/ HEAD) +~/.claude/skills/gstack/bin/gstack-review-log --start review git diff "$DIFF_BASE" ``` This includes both committed and uncommitted changes while excluding commits that landed on the base branch after this branch was created. +Remember the printed start token as REVIEW_START for this pass. Capture it before reading the diff, never at log time. On each full re-review, capture a new token. Read any non-ignored untracked source files too (`git ls-files --others --exclude-standard`); the fingerprint includes them. ## Step 3.4: Workspace-aware queue status (advisory) @@ -924,10 +926,12 @@ If no documentation files exist, skip this step silently. After all review passes complete, persist the final `/review` outcome so `/ship` can recognize that Eng Review was run on this branch. +Follow the completion/retry and detailed record-field rules in the adversarial section before persisting. + Run: ```bash -~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"quality_score":SCORE,"specialists":SPECIALISTS_JSON,"findings":FINDINGS_JSON,"commit":"COMMIT"}' +~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"quality_score":SCORE,"specialists":SPECIALISTS_JSON,"findings":FINDINGS_JSON,"commit":"COMMIT","completed":COMPLETED,"converged":CONVERGED,"cycles":CYCLES}' --finish REVIEW_START ``` Substitute: @@ -937,8 +941,6 @@ Substitute: - `critical` = remaining unresolved critical findings - `informational` = remaining unresolved informational findings - `quality_score` = the PR Quality Score computed in Step 4.6 (e.g., 7.5). If specialists were skipped (small diff), use `10.0` -- `specialists` = the per-specialist stats object compiled in Step 4.6. Each specialist that was considered gets an entry: `{"dispatched":true/false,"findings":N,"critical":N,"informational":N}` if dispatched, or `{"dispatched":false,"reason":"scope|gated"}` if skipped. Include Design specialist. Example: `{"testing":{"dispatched":true,"findings":2,"critical":0,"informational":2},"security":{"dispatched":false,"reason":"scope"}}` -- `findings` = array of per-finding records from Step 5. For each finding (from critical pass and specialists), include: `{"fingerprint":"path:line:category","severity":"CRITICAL|INFORMATIONAL","action":"ACTION"}`. ACTION is `"auto-fixed"` (Step 5b), `"fixed"` (user approved in Step 5d), or `"skipped"` (user chose Skip in Step 5c). Suppressed findings from Step 5.0 are NOT included (they were already recorded in a prior review entry). - `COMMIT` = output of `git rev-parse --short HEAD` ## Capture Learnings diff --git a/review/SKILL.md.tmpl b/review/SKILL.md.tmpl index b3834be92..7b266e9fc 100644 --- a/review/SKILL.md.tmpl +++ b/review/SKILL.md.tmpl @@ -80,10 +80,12 @@ Compute the merge base, then diff the working tree against that point: ```bash DIFF_BASE=$(git merge-base origin/ HEAD) +~/.claude/skills/gstack/bin/gstack-review-log --start review git diff "$DIFF_BASE" ``` This includes both committed and uncommitted changes while excluding commits that landed on the base branch after this branch was created. +Remember the printed start token as REVIEW_START for this pass. Capture it before reading the diff, never at log time. On each full re-review, capture a new token. Read any non-ignored untracked source files too (`git ls-files --others --exclude-standard`); the fingerprint includes them. ## Step 3.4: Workspace-aware queue status (advisory) @@ -284,10 +286,12 @@ If no documentation files exist, skip this step silently. After all review passes complete, persist the final `/review` outcome so `/ship` can recognize that Eng Review was run on this branch. +Follow the completion/retry and detailed record-field rules in the adversarial section before persisting. + Run: ```bash -~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"quality_score":SCORE,"specialists":SPECIALISTS_JSON,"findings":FINDINGS_JSON,"commit":"COMMIT"}' +~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"quality_score":SCORE,"specialists":SPECIALISTS_JSON,"findings":FINDINGS_JSON,"commit":"COMMIT","completed":COMPLETED,"converged":CONVERGED,"cycles":CYCLES}' --finish REVIEW_START ``` Substitute: @@ -297,8 +301,6 @@ Substitute: - `critical` = remaining unresolved critical findings - `informational` = remaining unresolved informational findings - `quality_score` = the PR Quality Score computed in Step 4.6 (e.g., 7.5). If specialists were skipped (small diff), use `10.0` -- `specialists` = the per-specialist stats object compiled in Step 4.6. Each specialist that was considered gets an entry: `{"dispatched":true/false,"findings":N,"critical":N,"informational":N}` if dispatched, or `{"dispatched":false,"reason":"scope|gated"}` if skipped. Include Design specialist. Example: `{"testing":{"dispatched":true,"findings":2,"critical":0,"informational":2},"security":{"dispatched":false,"reason":"scope"}}` -- `findings` = array of per-finding records from Step 5. For each finding (from critical pass and specialists), include: `{"fingerprint":"path:line:category","severity":"CRITICAL|INFORMATIONAL","action":"ACTION"}`. ACTION is `"auto-fixed"` (Step 5b), `"fixed"` (user approved in Step 5d), or `"skipped"` (user chose Skip in Step 5c). Suppressed findings from Step 5.0 are NOT included (they were already recorded in a prior review entry). - `COMMIT` = output of `git rev-parse --short HEAD` {{LEARNINGS_LOG}} diff --git a/review/sections/adversarial.md b/review/sections/adversarial.md index 6eb7aff17..f14c43b1f 100644 --- a/review/sections/adversarial.md +++ b/review/sections/adversarial.md @@ -70,6 +70,8 @@ Claude only. ### Claude adversarial subagent (always runs) +Before dispatch, run `~/.claude/skills/gstack/bin/gstack-review-log --start adversarial-review` and remember the token for this native pass. Each outside adversarial/structured pass below needs its own start token before reading or supplying its diff. Capture a fresh token on each actual rerun, never while logging. Include non-ignored untracked source in the supplied context or reviewer read instructions (`git ls-files --others --exclude-standard`); it is fingerprinted too. + Dispatch via the Agent tool with `run_in_background: false` (subagents default to background since Claude Code v2.1.198; the adversarial findings must land before the review concludes). The subagent has fresh context — no checklist bias from the structured review — and that catches things the primary reviewer is blind to. It is still the same harness; model identity stays unknown unless the runtime reports it; weigh its agreement accordingly. Subagent prompt: @@ -217,8 +219,9 @@ If `DIFF_TOTAL < 200`: skip this section silently. The Claude + Codex adversaria After all passes complete, persist: ```bash -~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"adversarial-review","timestamp":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'","status":"STATUS","source":"SOURCE","host":"claude","outside_provider":"codex","outside_status":"OUTSIDE_STATUS","phase":"PHASE","tier":"always","gate":"GATE","commit":"'"$(git rev-parse --short HEAD)"'"}' +~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"adversarial-review","timestamp":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'","status":"STATUS","source":"SOURCE","host":"claude","outside_provider":"codex","outside_status":"OUTSIDE_STATUS","phase":"PHASE","tier":"always","gate":"GATE","commit":"'"$(git rev-parse --short HEAD)"'","completed":COMPLETED,"converged":CONVERGED}' --finish PASS_START ``` +PASS_START is this source/phase's original start token. COMPLETED is true only for a completed response (false for timeout, failure, refusal, or missing coverage). CONVERGED is true only if the completed pass made no edits. Each token is consumed once; a fixing pass cannot certify the fixed tree without a fresh full pass. Missing/disabled passes have no token: omit `--finish` and log completed/converged false. Log each source/phase separately so a clean native response cannot hide missing outside coverage. Substitute: PHASE = "adversarial" or "structured" for the corresponding pass. STATUS = "clean" only for a completed pass with no findings, "issues_found" if any pass found issues. SOURCE = the completed outside provider for its record; use a separate in-host record for the native subagent. GATE = the Codex structured review gate result ("pass"/"fail"), "skipped" if diff < 200, or "informational" if Codex was unavailable. If all passes failed, persist status "unavailable" with outside_status "unavailable"; never persist "clean". Record the adversarial and structured phases separately if their coverage differs. --- @@ -243,3 +246,12 @@ ADVERSARIAL REVIEW SYNTHESIS (always-on, N lines): High-confidence findings (agreed on by multiple sources) should be prioritized for fixes. --- + +### Before persisting Eng Review (Step 5.8) + +If this pass applied any fixes (including adversarial fixes), repeat Steps 3–5.7 against the updated diff with a new REVIEW_START. A pass converges only when it completes without edits. Allow at most 3 fix cycles; if the third still applies fixes, persist `converged:false` and stop with the remaining findings. Do not capture a new token just to log the fixed tree. + +For the Step 5.8 record, REVIEW_START is the token captured before this pass's Step 3 diff read. COMPLETED is true only if the checklist and dispatched specialists completed; missing coverage is false, never clean. CONVERGED is true only for a completed pass with zero edits. CYCLES counts fix cycles (0 for a first-pass completion). Preserve unavailable specialist/provider coverage in the summary; completion of one source does not imply completion of another. + +- `specialists` = the per-specialist stats object compiled in Step 4.6. Each specialist that was considered gets an entry: `{"dispatched":true/false,"findings":N,"critical":N,"informational":N}` if dispatched, or `{"dispatched":false,"reason":"scope|gated"}` if skipped. Include Design specialist. Example: `{"testing":{"dispatched":true,"findings":2,"critical":0,"informational":2},"security":{"dispatched":false,"reason":"scope"}}` +- `findings` = array of per-finding records from Step 5. For each finding (from critical pass and specialists), include: `{"fingerprint":"path:line:category","severity":"CRITICAL|INFORMATIONAL","action":"ACTION"}`. ACTION is `"auto-fixed"` (Step 5b), `"fixed"` (user approved in Step 5d), or `"skipped"` (user chose Skip in Step 5c). Suppressed findings from Step 5.0 are NOT included (they were already recorded in a prior review entry). diff --git a/review/sections/adversarial.md.tmpl b/review/sections/adversarial.md.tmpl index dd691cd57..ad7f054b9 100644 --- a/review/sections/adversarial.md.tmpl +++ b/review/sections/adversarial.md.tmpl @@ -1 +1,10 @@ {{ADVERSARIAL_STEP}} + +### Before persisting Eng Review (Step 5.8) + +If this pass applied any fixes (including adversarial fixes), repeat Steps 3–5.7 against the updated diff with a new REVIEW_START. A pass converges only when it completes without edits. Allow at most 3 fix cycles; if the third still applies fixes, persist `converged:false` and stop with the remaining findings. Do not capture a new token just to log the fixed tree. + +For the Step 5.8 record, REVIEW_START is the token captured before this pass's Step 3 diff read. COMPLETED is true only if the checklist and dispatched specialists completed; missing coverage is false, never clean. CONVERGED is true only for a completed pass with zero edits. CYCLES counts fix cycles (0 for a first-pass completion). Preserve unavailable specialist/provider coverage in the summary; completion of one source does not imply completion of another. + +- `specialists` = the per-specialist stats object compiled in Step 4.6. Each specialist that was considered gets an entry: `{"dispatched":true/false,"findings":N,"critical":N,"informational":N}` if dispatched, or `{"dispatched":false,"reason":"scope|gated"}` if skipped. Include Design specialist. Example: `{"testing":{"dispatched":true,"findings":2,"critical":0,"informational":2},"security":{"dispatched":false,"reason":"scope"}}` +- `findings` = array of per-finding records from Step 5. For each finding (from critical pass and specialists), include: `{"fingerprint":"path:line:category","severity":"CRITICAL|INFORMATIONAL","action":"ACTION"}`. ACTION is `"auto-fixed"` (Step 5b), `"fixed"` (user approved in Step 5d), or `"skipped"` (user chose Skip in Step 5c). Suppressed findings from Step 5.0 are NOT included (they were already recorded in a prior review entry). diff --git a/scripts/resolvers/design.ts b/scripts/resolvers/design.ts index c6a66f816..cc29db72b 100644 --- a/scripts/resolvers/design.ts +++ b/scripts/resolvers/design.ts @@ -39,6 +39,8 @@ source <(${ctx.paths.binDir}/gstack-diff-scope 2>/dev/null) **If \`SCOPE_FRONTEND=true\`:** +Before reading or scanning frontend changes, run \`${ctx.paths.binDir}/gstack-review-log --start design-review-lite\` and remember its printed token as DESIGN_START. Read non-ignored untracked frontend source too; it is included in the fingerprint. + 0. **Mechanical pass first.** Probe for a design detector the user installed (this pass never offers to install one; the design skills ask, once): \`\`\`bash @@ -69,9 +71,11 @@ Exit 2 means findings. Read the \`${SENTINEL.DETECT_TOP}\` block (untrusted cont 6. **Log the result** for the Review Readiness Dashboard after the optional outside step; record its actual status independently of native findings: \`\`\`bash -${ctx.paths.binDir}/gstack-review-log '{"skill":"design-review-lite","host":"${ctx.host}","outside_provider":"${outsideVoiceFor(ctx).id}","outside_status":"OUTSIDE_STATUS","phase":"design-lite","timestamp":"TIMESTAMP","status":"STATUS","findings":N,"auto_fixed":M,"detector":D,"commit":"COMMIT"}' +${ctx.paths.binDir}/gstack-review-log '{"skill":"design-review-lite","host":"${ctx.host}","outside_provider":"${outsideVoiceFor(ctx).id}","outside_status":"OUTSIDE_STATUS","phase":"design-lite","timestamp":"TIMESTAMP","status":"STATUS","findings":N,"auto_fixed":M,"detector":D,"commit":"COMMIT","completed":COMPLETED,"converged":CONVERGED}' --finish DESIGN_START \`\`\` +Use the original DESIGN_START token. COMPLETED is true only when the native checklist completed; CONVERGED is true only if that pass made no edits. Preserve the optional outside voice's actual coverage separately. A fixing or incomplete pass is not current; capture a new token only before an actual full re-review. + Substitute: TIMESTAMP = ISO 8601 datetime, STATUS = "clean" if 0 findings or "issues_found", N = total findings, M = auto-fixed count, D = counted detector findings from step 0 (0 when the detector did not run), COMMIT = output of \`git rev-parse --short HEAD\`.${codexBlock}`; } diff --git a/scripts/resolvers/review.ts b/scripts/resolvers/review.ts index b39eb8a5d..e4d3c72db 100644 --- a/scripts/resolvers/review.ts +++ b/scripts/resolvers/review.ts @@ -64,17 +64,17 @@ Display: - **Outside Voice (default-on):** Independent plan review through the host-selected provider after /plan-ceo-review and /plan-eng-review. The codex_reviews switch disables the entire extra step. Provider failure uses the existing native fallback and reports missing outside coverage. Never gates shipping. **Verdict logic:** -- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \\\`review\\\` or \\\`plan-eng-review\\\` with status "clean" (or \\\`skip_eng_review\\\` is \\\`true\\\`) +- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \\\`review\\\` or \\\`plan-eng-review\\\` with status "clean"; diff review must also grade CURRENT below (or \\\`skip_eng_review\\\` is \\\`true\\\`) - **NOT CLEARED**: Eng Review missing, stale (>7 days), or has open issues - CEO, Design, and outside reviews are shown for context but never block shipping - If \\\`skip_eng_review\\\` config is \\\`true\\\`, Eng Review shows "SKIPPED (global)" and verdict is CLEARED -**Staleness detection:** After displaying the dashboard, check if any existing reviews may be stale: -- **Content-first rule (diff-scoped rows only: \\\`review\\\`, \\\`adversarial-review\\\`, \\\`codex-review\\\`, ship-stage entries).** Parse the \\\`---WTREE---\\\` and \\\`---DIRTY---\\\` sections from the bash output. If an entry has a \\\`wtree\\\` field AND it equals the current \\\`---WTREE---\\\` value, the review is CURRENT — identical content, regardless of commit count, rebase, amend, or whether it was committed yet (wtree equality alone proves identical content; that is the keystone property). Skip the commit-count heuristic for that entry and show no staleness note. -- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If such an entry carries a \\\`plan_sha256\\\` field, you MAY compare it against the current plan file's sha256 and note "plan changed since review" on mismatch. -- Fallback (no \\\`wtree\\\` on the entry, or wtree mismatch): parse the \\\`---HEAD---\\\` section to get the current HEAD commit hash. For each review entry that has a \\\`commit\\\` field: compare it against the current HEAD. If different, count elapsed commits: \\\`git rev-list --count STORED_COMMIT..HEAD\\\`. If that command FAILS (the stored commit was rebased away), grade UNKNOWN and treat as stale — do not error. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" -- For entries without a \\\`commit\\\` field (legacy entries): display "Note: {skill} review from {date} has no commit tracking — consider re-running for accurate staleness detection" -- If all reviews grade CURRENT (wtree match or HEAD match), do not display any staleness notes`; +**Staleness detection:** Grade before deciding CLEARED: +- Ship telemetry reports metrics, not review coverage; it never satisfies a review row. +- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-review\`, \`codex-review\`, ship-stage entries, \`design-review-lite\`).** Use the helper's computed \`review_freshness.status\` and show its \`reason\`. CURRENT requires a completed clean pass with captured start/end wtree equal to the current \`---WTREE---\`. STALE or UNVERIFIED never clears Eng Review. Missing \`review_freshness\` is UNVERIFIED, including legacy log-only rows. Never fall back to HEAD equality or commit distance for diff evidence, even at 0 commits. Show recorded cycles, completed/converged state, and missing per-source/phase coverage; unknown is not a pass. +- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review, codex-plan-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If an entry carries \`plan_sha256\`, you MAY compare it with the plan file and note "plan changed since review" on mismatch. +- Plan-tier fallback only: parse \`---HEAD---\`. For entries with a different \`commit\`, count elapsed commits: \`git rev-list --count STORED_COMMIT..HEAD\`. If that command FAILS, grade UNKNOWN and treat as stale. Display: "Note: {skill} review from {date} may be stale — {N} commits since review". Missing commit tracking retains the legacy note to consider re-running. +- If all reviews grade CURRENT, do not display staleness notes`; } export function generatePlanFileReviewReport(_ctx: TemplateContext): string { @@ -514,6 +514,8 @@ ${outsideVoiceFor(ctx).nativeLabel} only. ### ${outsideVoiceFor(ctx).nativeLabel} adversarial subagent (always runs) +Before dispatch, run \`~/.claude/skills/gstack/bin/gstack-review-log --start adversarial-review\` and remember the token for this native pass. Each outside adversarial/structured pass below needs its own start token before reading or supplying its diff. Capture a fresh token on each actual rerun, never while logging. Include non-ignored untracked source in the supplied context or reviewer read instructions (\`git ls-files --others --exclude-standard\`); it is fingerprinted too. + Dispatch via the Agent tool with \`run_in_background: false\` (subagents default to background since ${CC_BACKGROUND_DEFAULT_SINCE}; the adversarial findings must land before the review concludes). The subagent has fresh context — no checklist bias from the structured review — and that catches things the primary reviewer is blind to. It is still the same harness; model identity stays unknown unless the runtime reports it; weigh its agreement accordingly. Subagent prompt: @@ -589,8 +591,9 @@ If \`DIFF_TOTAL < 200\`: skip this section silently. The ${outsideVoiceFor(ctx). After all passes complete, persist: \`\`\`bash -~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"adversarial-review","timestamp":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'","status":"STATUS","source":"SOURCE","host":"${ctx.host}","outside_provider":"${outsideVoiceFor(ctx).id}","outside_status":"OUTSIDE_STATUS","phase":"PHASE","tier":"always","gate":"GATE","commit":"'"$(git rev-parse --short HEAD)"'"}' +~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"adversarial-review","timestamp":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'","status":"STATUS","source":"SOURCE","host":"${ctx.host}","outside_provider":"${outsideVoiceFor(ctx).id}","outside_status":"OUTSIDE_STATUS","phase":"PHASE","tier":"always","gate":"GATE","commit":"'"$(git rev-parse --short HEAD)"'","completed":COMPLETED,"converged":CONVERGED}' --finish PASS_START \`\`\` +PASS_START is this source/phase's original start token. COMPLETED is true only for a completed response (false for timeout, failure, refusal, or missing coverage). CONVERGED is true only if the completed pass made no edits. Each token is consumed once; a fixing pass cannot certify the fixed tree without a fresh full pass. Missing/disabled passes have no token: omit \`--finish\` and log completed/converged false. Log each source/phase separately so a clean native response cannot hide missing outside coverage. Substitute: PHASE = "adversarial" or "structured" for the corresponding pass. STATUS = "clean" only for a completed pass with no findings, "issues_found" if any pass found issues. SOURCE = the completed outside provider for its record; use a separate in-host record for the native subagent. GATE = the ${outsideVoiceFor(ctx).label} structured review gate result ("pass"/"fail"), "skipped" if diff < 200, or "informational" if ${outsideVoiceFor(ctx).label} was unavailable. If all passes failed, persist status "unavailable" with outside_status "unavailable"; never persist "clean". Record the adversarial and structured phases separately if their coverage differs. --- diff --git a/ship/SKILL.md b/ship/SKILL.md index 2acf0b963..b12db06b2 100644 --- a/ship/SKILL.md +++ b/ship/SKILL.md @@ -620,17 +620,17 @@ Display: - **Outside Voice (default-on):** Independent plan review through the host-selected provider after /plan-ceo-review and /plan-eng-review. The codex_reviews switch disables the entire extra step. Provider failure uses the existing native fallback and reports missing outside coverage. Never gates shipping. **Verdict logic:** -- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean" (or \`skip_eng_review\` is \`true\`) +- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean"; diff review must also grade CURRENT below (or \`skip_eng_review\` is \`true\`) - **NOT CLEARED**: Eng Review missing, stale (>7 days), or has open issues - CEO, Design, and outside reviews are shown for context but never block shipping - If \`skip_eng_review\` config is \`true\`, Eng Review shows "SKIPPED (global)" and verdict is CLEARED -**Staleness detection:** After displaying the dashboard, check if any existing reviews may be stale: -- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-review\`, \`codex-review\`, ship-stage entries).** Parse the \`---WTREE---\` and \`---DIRTY---\` sections from the bash output. If an entry has a \`wtree\` field AND it equals the current \`---WTREE---\` value, the review is CURRENT — identical content, regardless of commit count, rebase, amend, or whether it was committed yet (wtree equality alone proves identical content; that is the keystone property). Skip the commit-count heuristic for that entry and show no staleness note. -- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If such an entry carries a \`plan_sha256\` field, you MAY compare it against the current plan file's sha256 and note "plan changed since review" on mismatch. -- Fallback (no \`wtree\` on the entry, or wtree mismatch): parse the \`---HEAD---\` section to get the current HEAD commit hash. For each review entry that has a \`commit\` field: compare it against the current HEAD. If different, count elapsed commits: \`git rev-list --count STORED_COMMIT..HEAD\`. If that command FAILS (the stored commit was rebased away), grade UNKNOWN and treat as stale — do not error. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" -- For entries without a \`commit\` field (legacy entries): display "Note: {skill} review from {date} has no commit tracking — consider re-running for accurate staleness detection" -- If all reviews grade CURRENT (wtree match or HEAD match), do not display any staleness notes +**Staleness detection:** Grade before deciding CLEARED: +- Ship telemetry reports metrics, not review coverage; it never satisfies a review row. +- **Content-first rule (diff-scoped rows only: `review`, `adversarial-review`, `codex-review`, ship-stage entries, `design-review-lite`).** Use the helper's computed `review_freshness.status` and show its `reason`. CURRENT requires a completed clean pass with captured start/end wtree equal to the current `---WTREE---`. STALE or UNVERIFIED never clears Eng Review. Missing `review_freshness` is UNVERIFIED, including legacy log-only rows. Never fall back to HEAD equality or commit distance for diff evidence, even at 0 commits. Show recorded cycles, completed/converged state, and missing per-source/phase coverage; unknown is not a pass. +- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review, codex-plan-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If an entry carries `plan_sha256`, you MAY compare it with the plan file and note "plan changed since review" on mismatch. +- Plan-tier fallback only: parse `---HEAD---`. For entries with a different `commit`, count elapsed commits: `git rev-list --count STORED_COMMIT..HEAD`. If that command FAILS, grade UNKNOWN and treat as stale. Display: "Note: {skill} review from {date} may be stale — {N} commits since review". Missing commit tracking retains the legacy note to consider re-running. +- If all reviews grade CURRENT, do not display staleness notes If the Eng Review is NOT "CLEAR": diff --git a/ship/sections/adversarial.md b/ship/sections/adversarial.md index 4c16dab4b..8727bdbc1 100644 --- a/ship/sections/adversarial.md +++ b/ship/sections/adversarial.md @@ -70,6 +70,8 @@ Claude only. ### Claude adversarial subagent (always runs) +Before dispatch, run `~/.claude/skills/gstack/bin/gstack-review-log --start adversarial-review` and remember the token for this native pass. Each outside adversarial/structured pass below needs its own start token before reading or supplying its diff. Capture a fresh token on each actual rerun, never while logging. Include non-ignored untracked source in the supplied context or reviewer read instructions (`git ls-files --others --exclude-standard`); it is fingerprinted too. + Dispatch via the Agent tool with `run_in_background: false` (subagents default to background since Claude Code v2.1.198; the adversarial findings must land before the review concludes). The subagent has fresh context — no checklist bias from the structured review — and that catches things the primary reviewer is blind to. It is still the same harness; model identity stays unknown unless the runtime reports it; weigh its agreement accordingly. Subagent prompt: @@ -217,8 +219,9 @@ If `DIFF_TOTAL < 200`: skip this section silently. The Claude + Codex adversaria After all passes complete, persist: ```bash -~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"adversarial-review","timestamp":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'","status":"STATUS","source":"SOURCE","host":"claude","outside_provider":"codex","outside_status":"OUTSIDE_STATUS","phase":"PHASE","tier":"always","gate":"GATE","commit":"'"$(git rev-parse --short HEAD)"'"}' +~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"adversarial-review","timestamp":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'","status":"STATUS","source":"SOURCE","host":"claude","outside_provider":"codex","outside_status":"OUTSIDE_STATUS","phase":"PHASE","tier":"always","gate":"GATE","commit":"'"$(git rev-parse --short HEAD)"'","completed":COMPLETED,"converged":CONVERGED}' --finish PASS_START ``` +PASS_START is this source/phase's original start token. COMPLETED is true only for a completed response (false for timeout, failure, refusal, or missing coverage). CONVERGED is true only if the completed pass made no edits. Each token is consumed once; a fixing pass cannot certify the fixed tree without a fresh full pass. Missing/disabled passes have no token: omit `--finish` and log completed/converged false. Log each source/phase separately so a clean native response cannot hide missing outside coverage. Substitute: PHASE = "adversarial" or "structured" for the corresponding pass. STATUS = "clean" only for a completed pass with no findings, "issues_found" if any pass found issues. SOURCE = the completed outside provider for its record; use a separate in-host record for the native subagent. GATE = the Codex structured review gate result ("pass"/"fail"), "skipped" if diff < 200, or "informational" if Codex was unavailable. If all passes failed, persist status "unavailable" with outside_status "unavailable"; never persist "clean". Record the adversarial and structured phases separately if their coverage differs. --- diff --git a/ship/sections/review-army.md b/ship/sections/review-army.md index 048d5a46b..a75f2b9fe 100644 --- a/ship/sections/review-army.md +++ b/ship/sections/review-army.md @@ -68,7 +68,7 @@ higher confidence. 1. Read `~/.claude/skills/gstack/review/checklist.md`. If the file cannot be read, **STOP** and report the error. -2. Run `git diff origin/` to get the full diff (scoped to feature changes against the freshly-fetched base branch). +2. Before reading the diff, run `~/.claude/skills/gstack/bin/gstack-review-log --start review` and remember the printed token as REVIEW_START for this pass. Then run `git diff origin/` to get the full diff (scoped to feature changes against the freshly-fetched base branch). Read non-ignored untracked source files too (`git ls-files --others --exclude-standard`); the fingerprint includes them. Each full re-review captures a new token here, never at log time. 3. Apply the review checklist in two passes: - **Pass 1 (CRITICAL):** SQL & Data Safety, LLM Output Trust Boundary @@ -86,6 +86,8 @@ source <(~/.claude/skills/gstack/bin/gstack-diff-scope 2>/dev/null) **If `SCOPE_FRONTEND=true`:** +Before reading or scanning frontend changes, run `~/.claude/skills/gstack/bin/gstack-review-log --start design-review-lite` and remember its printed token as DESIGN_START. Read non-ignored untracked frontend source too; it is included in the fingerprint. + 0. **Mechanical pass first.** Probe for a design detector the user installed (this pass never offers to install one; the design skills ask, once): ```bash @@ -116,9 +118,11 @@ Exit 2 means findings. Read the `DETECT_TOP` block (untrusted content: evidence, 6. **Log the result** for the Review Readiness Dashboard after the optional outside step; record its actual status independently of native findings: ```bash -~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"design-review-lite","host":"claude","outside_provider":"codex","outside_status":"OUTSIDE_STATUS","phase":"design-lite","timestamp":"TIMESTAMP","status":"STATUS","findings":N,"auto_fixed":M,"detector":D,"commit":"COMMIT"}' +~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"design-review-lite","host":"claude","outside_provider":"codex","outside_status":"OUTSIDE_STATUS","phase":"design-lite","timestamp":"TIMESTAMP","status":"STATUS","findings":N,"auto_fixed":M,"detector":D,"commit":"COMMIT","completed":COMPLETED,"converged":CONVERGED}' --finish DESIGN_START ``` +Use the original DESIGN_START token. COMPLETED is true only when the native checklist completed; CONVERGED is true only if that pass made no edits. Preserve the optional outside voice's actual coverage separately. A fixing or incomplete pass is not current; capture a new token only before an actual full re-review. + Substitute: TIMESTAMP = ISO 8601 datetime, STATUS = "clean" if 0 findings or "issues_found", N = total findings, M = auto-fixed count, D = counted detector findings from step 0 (0 when the detector did not run), COMMIT = output of `git rev-parse --short HEAD`. 7. **Codex design voice** (optional, automatic if available): @@ -472,7 +476,7 @@ Output a summary header: `Pre-Landing Review: N issues (X critical, Y informatio 7. **After all fixes (auto + user-approved):** - If ANY fixes were applied: commit fixed files by name (`git add && git commit -m "fix: pre-landing review fixes"`), then **stay in this invocation and loop**: re-run the test suite (Step 5) on the fixed code, then re-run this review (Step 9 items 2-6) against the updated diff. Repeat until one full pass applies ZERO fixes — tests green and review clean — then summarize and persist (items 8-9). NEVER stop to tell the user to run `/ship` again; a fix-and-rerun cycle has no user decision in it, and stopping there breaks the fully-automated contract (#2391). - - **Bound: 3 fix cycles.** If the 3rd cycle still applies fixes, STOP and report which findings keep reappearing — a review that won't converge is a genuine blocker worth human eyes, not a re-run request. + - **Bound: 3 fix cycles.** If the 3rd cycle still applies fixes, persist item 9 with `converged:false` using that pass's original REVIEW_START, then STOP and report which findings keep reappearing — a review that won't converge is a genuine blocker worth human eyes, not a re-run request. - If no fixes applied (all ASK items skipped, or no issues found): summarize and persist (items 8-9). 8. Output summary: `Pre-Landing Review: N issues — M auto-fixed, K asked (J fixed, L skipped)` @@ -481,10 +485,11 @@ Output a summary header: `Pre-Landing Review: N issues (X critical, Y informatio 9. Persist the review result to the review log: ```bash -~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"quality_score":SCORE,"specialists":SPECIALISTS_JSON,"findings":FINDINGS_JSON,"commit":"'"$(git rev-parse --short HEAD)"'","via":"ship"}' +~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"quality_score":SCORE,"specialists":SPECIALISTS_JSON,"findings":FINDINGS_JSON,"commit":"'"$(git rev-parse --short HEAD)"'","via":"ship","completed":COMPLETED,"converged":CONVERGED,"cycles":CYCLES}' --finish REVIEW_START ``` Substitute TIMESTAMP (ISO 8601), STATUS ("clean" if no issues, "issues_found" otherwise), -and N values from the summary counts above. The `via:"ship"` distinguishes from standalone `/review` runs. +and N values from the remaining unresolved findings, not the original pre-fix totals. The `via:"ship"` distinguishes from standalone `/review` runs. +- `REVIEW_START` = the token captured in item 2 before this pass read the diff. `COMPLETED` = true only if the checklist and dispatched specialists completed; missing coverage is false, never clean. `CONVERGED` = true only for a completed pass that applied zero fixes. `CYCLES` = fix cycles performed (0 for a first-pass completion). Never recapture at persistence to certify fixes that have not been reviewed. - `quality_score` = the PR Quality Score computed in Step 9.2 (e.g., 7.5). If specialists were skipped (small diff), use `10.0` - `specialists` = the per-specialist stats object compiled in Step 9.2. Each specialist that was considered gets an entry: `{"dispatched":true/false,"findings":N,"critical":N,"informational":N}` if dispatched, or `{"dispatched":false,"reason":"scope|gated"}` if skipped. Example: `{"testing":{"dispatched":true,"findings":2,"critical":0,"informational":2},"security":{"dispatched":false,"reason":"scope"}}` - `findings` = array of per-finding records. For each finding (from checklist pass and specialists), include: `{"fingerprint":"path:line:category","severity":"CRITICAL|INFORMATIONAL","action":"ACTION"}`. ACTION is `"auto-fixed"`, `"fixed"` (user approved), or `"skipped"` (user chose Skip). diff --git a/ship/sections/review-army.md.tmpl b/ship/sections/review-army.md.tmpl index 843e1ba9a..b2d6127c1 100644 --- a/ship/sections/review-army.md.tmpl +++ b/ship/sections/review-army.md.tmpl @@ -6,7 +6,7 @@ Review structural issues tests don't catch. Order: calibrate, checklist, design, 1. Read `~/.claude/skills/gstack/review/checklist.md`. If the file cannot be read, **STOP** and report the error. -2. Run `git diff origin/` to get the full diff (scoped to feature changes against the freshly-fetched base branch). +2. Before reading the diff, run `~/.claude/skills/gstack/bin/gstack-review-log --start review` and remember the printed token as REVIEW_START for this pass. Then run `git diff origin/` to get the full diff (scoped to feature changes against the freshly-fetched base branch). Read non-ignored untracked source files too (`git ls-files --others --exclude-standard`); the fingerprint includes them. Each full re-review captures a new token here, never at log time. 3. Apply the review checklist in two passes: - **Pass 1 (CRITICAL):** SQL & Data Safety, LLM Output Trust Boundary @@ -36,7 +36,7 @@ Review structural issues tests don't catch. Order: calibrate, checklist, design, 7. **After all fixes (auto + user-approved):** - If ANY fixes were applied: commit fixed files by name (`git add && git commit -m "fix: pre-landing review fixes"`), then **stay in this invocation and loop**: re-run the test suite (Step 5) on the fixed code, then re-run this review (Step 9 items 2-6) against the updated diff. Repeat until one full pass applies ZERO fixes — tests green and review clean — then summarize and persist (items 8-9). NEVER stop to tell the user to run `/ship` again; a fix-and-rerun cycle has no user decision in it, and stopping there breaks the fully-automated contract (#2391). - - **Bound: 3 fix cycles.** If the 3rd cycle still applies fixes, STOP and report which findings keep reappearing — a review that won't converge is a genuine blocker worth human eyes, not a re-run request. + - **Bound: 3 fix cycles.** If the 3rd cycle still applies fixes, persist item 9 with `converged:false` using that pass's original REVIEW_START, then STOP and report which findings keep reappearing — a review that won't converge is a genuine blocker worth human eyes, not a re-run request. - If no fixes applied (all ASK items skipped, or no issues found): summarize and persist (items 8-9). 8. Output summary: `Pre-Landing Review: N issues — M auto-fixed, K asked (J fixed, L skipped)` @@ -45,10 +45,11 @@ Review structural issues tests don't catch. Order: calibrate, checklist, design, 9. Persist the review result to the review log: ```bash -~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"quality_score":SCORE,"specialists":SPECIALISTS_JSON,"findings":FINDINGS_JSON,"commit":"'"$(git rev-parse --short HEAD)"'","via":"ship"}' +~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"quality_score":SCORE,"specialists":SPECIALISTS_JSON,"findings":FINDINGS_JSON,"commit":"'"$(git rev-parse --short HEAD)"'","via":"ship","completed":COMPLETED,"converged":CONVERGED,"cycles":CYCLES}' --finish REVIEW_START ``` Substitute TIMESTAMP (ISO 8601), STATUS ("clean" if no issues, "issues_found" otherwise), -and N values from the summary counts above. The `via:"ship"` distinguishes from standalone `/review` runs. +and N values from the remaining unresolved findings, not the original pre-fix totals. The `via:"ship"` distinguishes from standalone `/review` runs. +- `REVIEW_START` = the token captured in item 2 before this pass read the diff. `COMPLETED` = true only if the checklist and dispatched specialists completed; missing coverage is false, never clean. `CONVERGED` = true only for a completed pass that applied zero fixes. `CYCLES` = fix cycles performed (0 for a first-pass completion). Never recapture at persistence to certify fixes that have not been reviewed. - `quality_score` = the PR Quality Score computed in Step 9.2 (e.g., 7.5). If specialists were skipped (small diff), use `10.0` - `specialists` = the per-specialist stats object compiled in Step 9.2. Each specialist that was considered gets an entry: `{"dispatched":true/false,"findings":N,"critical":N,"informational":N}` if dispatched, or `{"dispatched":false,"reason":"scope|gated"}` if skipped. Example: `{"testing":{"dispatched":true,"findings":2,"critical":0,"informational":2},"security":{"dispatched":false,"reason":"scope"}}` - `findings` = array of per-finding records. For each finding (from checklist pass and specialists), include: `{"fingerprint":"path:line:category","severity":"CRITICAL|INFORMATIONAL","action":"ACTION"}`. ACTION is `"auto-fixed"`, `"fixed"` (user approved), or `"skipped"` (user chose Skip). diff --git a/test/binding-template-drift.test.ts b/test/binding-template-drift.test.ts index 834259439..711f8b97b 100644 --- a/test/binding-template-drift.test.ts +++ b/test/binding-template-drift.test.ts @@ -61,6 +61,45 @@ describe('content-binding template drift', () => { expect(rendered('land-and-deploy/sections/readiness-gate.md')).toMatch(rowList); }); + test('both grading surfaces reject missing capture instead of falling back to HEAD', () => { + for (const file of ['ship/SKILL.md', 'land-and-deploy/sections/readiness-gate.md']) { + const text = rendered(file); + expect(text).toContain('review_freshness'); + expect(text).toContain('UNVERIFIED'); + expect(text).toContain('Never fall back'); + expect(text).toContain('0 commits'); + expect(text.toLowerCase()).toContain('plan-tier'); + } + }); + + test('diff callers capture before reading and consume the original token', () => { + const review = rendered('review/SKILL.md'); + expect(review).toContain('gstack-review-log --start review\ngit diff "$DIFF_BASE"'); + expect(review).toContain('--finish REVIEW_START'); + expect(review).toContain('"completed":COMPLETED,"converged":CONVERGED,"cycles":CYCLES'); + const army = rendered('ship/sections/review-army.md'); + expect(army.indexOf('gstack-review-log --start review')).toBeLessThan(army.indexOf('run `git diff origin/`')); + expect(army).toContain('--finish REVIEW_START'); + expect(army).toContain('persist item 9 with `converged:false`'); + expect(army).toContain('--start design-review-lite'); + expect(army).toContain('--finish DESIGN_START'); + const codex = rendered('codex/sections/review-mode.md'); + const starts = [...codex.matchAll(/gstack-review-log --start codex-review/g)]; + expect(starts).toHaveLength(2); + expect(starts[0].index).toBeLessThan(codex.indexOf('_gstack_codex_timeout_wrapper 330 codex review')); + expect(starts[1].index).toBeLessThan(codex.indexOf('git diff "...HEAD"')); + expect(codex).toContain('--finish CODEX_REVIEW_START'); + expect(codex).toContain('"completed":COMPLETED,"converged":CONVERGED'); + expect(codex).toContain('Fixes stay stale until a genuine rerun'); + for (const skill of ['ship', 'review']) { + const adversarial = rendered(`${skill}/sections/adversarial.md`); + expect(adversarial).toContain('--start adversarial-review'); + expect(adversarial).toContain('--finish PASS_START'); + expect(adversarial).toContain('Each outside adversarial/structured pass'); + expect(adversarial).toContain('Each token is consumed once'); + } + }); + test('release-body write side carries the banner tripwire (and it actually fires)', () => { const body = rendered('document-release/sections/release-body.md'); expect(body).toContain('grep -c "UNTRUSTED TRACKER CONTENT" "/body.md"'); diff --git a/test/fixtures/golden/claude-ship-SKILL.md b/test/fixtures/golden/claude-ship-SKILL.md index 2acf0b963..b12db06b2 100644 --- a/test/fixtures/golden/claude-ship-SKILL.md +++ b/test/fixtures/golden/claude-ship-SKILL.md @@ -620,17 +620,17 @@ Display: - **Outside Voice (default-on):** Independent plan review through the host-selected provider after /plan-ceo-review and /plan-eng-review. The codex_reviews switch disables the entire extra step. Provider failure uses the existing native fallback and reports missing outside coverage. Never gates shipping. **Verdict logic:** -- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean" (or \`skip_eng_review\` is \`true\`) +- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean"; diff review must also grade CURRENT below (or \`skip_eng_review\` is \`true\`) - **NOT CLEARED**: Eng Review missing, stale (>7 days), or has open issues - CEO, Design, and outside reviews are shown for context but never block shipping - If \`skip_eng_review\` config is \`true\`, Eng Review shows "SKIPPED (global)" and verdict is CLEARED -**Staleness detection:** After displaying the dashboard, check if any existing reviews may be stale: -- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-review\`, \`codex-review\`, ship-stage entries).** Parse the \`---WTREE---\` and \`---DIRTY---\` sections from the bash output. If an entry has a \`wtree\` field AND it equals the current \`---WTREE---\` value, the review is CURRENT — identical content, regardless of commit count, rebase, amend, or whether it was committed yet (wtree equality alone proves identical content; that is the keystone property). Skip the commit-count heuristic for that entry and show no staleness note. -- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If such an entry carries a \`plan_sha256\` field, you MAY compare it against the current plan file's sha256 and note "plan changed since review" on mismatch. -- Fallback (no \`wtree\` on the entry, or wtree mismatch): parse the \`---HEAD---\` section to get the current HEAD commit hash. For each review entry that has a \`commit\` field: compare it against the current HEAD. If different, count elapsed commits: \`git rev-list --count STORED_COMMIT..HEAD\`. If that command FAILS (the stored commit was rebased away), grade UNKNOWN and treat as stale — do not error. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" -- For entries without a \`commit\` field (legacy entries): display "Note: {skill} review from {date} has no commit tracking — consider re-running for accurate staleness detection" -- If all reviews grade CURRENT (wtree match or HEAD match), do not display any staleness notes +**Staleness detection:** Grade before deciding CLEARED: +- Ship telemetry reports metrics, not review coverage; it never satisfies a review row. +- **Content-first rule (diff-scoped rows only: `review`, `adversarial-review`, `codex-review`, ship-stage entries, `design-review-lite`).** Use the helper's computed `review_freshness.status` and show its `reason`. CURRENT requires a completed clean pass with captured start/end wtree equal to the current `---WTREE---`. STALE or UNVERIFIED never clears Eng Review. Missing `review_freshness` is UNVERIFIED, including legacy log-only rows. Never fall back to HEAD equality or commit distance for diff evidence, even at 0 commits. Show recorded cycles, completed/converged state, and missing per-source/phase coverage; unknown is not a pass. +- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review, codex-plan-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If an entry carries `plan_sha256`, you MAY compare it with the plan file and note "plan changed since review" on mismatch. +- Plan-tier fallback only: parse `---HEAD---`. For entries with a different `commit`, count elapsed commits: `git rev-list --count STORED_COMMIT..HEAD`. If that command FAILS, grade UNKNOWN and treat as stale. Display: "Note: {skill} review from {date} may be stale — {N} commits since review". Missing commit tracking retains the legacy note to consider re-running. +- If all reviews grade CURRENT, do not display staleness notes If the Eng Review is NOT "CLEAR": diff --git a/test/fixtures/golden/codex-ship-SKILL.md b/test/fixtures/golden/codex-ship-SKILL.md index c23f6b547..40e07613f 100644 --- a/test/fixtures/golden/codex-ship-SKILL.md +++ b/test/fixtures/golden/codex-ship-SKILL.md @@ -613,17 +613,17 @@ Display: - **Outside Voice (default-on):** Independent plan review through the host-selected provider after /plan-ceo-review and /plan-eng-review. The codex_reviews switch disables the entire extra step. Provider failure uses the existing native fallback and reports missing outside coverage. Never gates shipping. **Verdict logic:** -- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean" (or \`skip_eng_review\` is \`true\`) +- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean"; diff review must also grade CURRENT below (or \`skip_eng_review\` is \`true\`) - **NOT CLEARED**: Eng Review missing, stale (>7 days), or has open issues - CEO, Design, and outside reviews are shown for context but never block shipping - If \`skip_eng_review\` config is \`true\`, Eng Review shows "SKIPPED (global)" and verdict is CLEARED -**Staleness detection:** After displaying the dashboard, check if any existing reviews may be stale: -- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-review\`, \`codex-review\`, ship-stage entries).** Parse the \`---WTREE---\` and \`---DIRTY---\` sections from the bash output. If an entry has a \`wtree\` field AND it equals the current \`---WTREE---\` value, the review is CURRENT — identical content, regardless of commit count, rebase, amend, or whether it was committed yet (wtree equality alone proves identical content; that is the keystone property). Skip the commit-count heuristic for that entry and show no staleness note. -- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If such an entry carries a \`plan_sha256\` field, you MAY compare it against the current plan file's sha256 and note "plan changed since review" on mismatch. -- Fallback (no \`wtree\` on the entry, or wtree mismatch): parse the \`---HEAD---\` section to get the current HEAD commit hash. For each review entry that has a \`commit\` field: compare it against the current HEAD. If different, count elapsed commits: \`git rev-list --count STORED_COMMIT..HEAD\`. If that command FAILS (the stored commit was rebased away), grade UNKNOWN and treat as stale — do not error. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" -- For entries without a \`commit\` field (legacy entries): display "Note: {skill} review from {date} has no commit tracking — consider re-running for accurate staleness detection" -- If all reviews grade CURRENT (wtree match or HEAD match), do not display any staleness notes +**Staleness detection:** Grade before deciding CLEARED: +- Ship telemetry reports metrics, not review coverage; it never satisfies a review row. +- **Content-first rule (diff-scoped rows only: `review`, `adversarial-review`, `codex-review`, ship-stage entries, `design-review-lite`).** Use the helper's computed `review_freshness.status` and show its `reason`. CURRENT requires a completed clean pass with captured start/end wtree equal to the current `---WTREE---`. STALE or UNVERIFIED never clears Eng Review. Missing `review_freshness` is UNVERIFIED, including legacy log-only rows. Never fall back to HEAD equality or commit distance for diff evidence, even at 0 commits. Show recorded cycles, completed/converged state, and missing per-source/phase coverage; unknown is not a pass. +- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review, codex-plan-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If an entry carries `plan_sha256`, you MAY compare it with the plan file and note "plan changed since review" on mismatch. +- Plan-tier fallback only: parse `---HEAD---`. For entries with a different `commit`, count elapsed commits: `git rev-list --count STORED_COMMIT..HEAD`. If that command FAILS, grade UNKNOWN and treat as stale. Display: "Note: {skill} review from {date} may be stale — {N} commits since review". Missing commit tracking retains the legacy note to consider re-running. +- If all reviews grade CURRENT, do not display staleness notes If the Eng Review is NOT "CLEAR": @@ -1734,7 +1734,7 @@ higher confidence. 1. Read `$GSTACK_ROOT/review/checklist.md`. If the file cannot be read, **STOP** and report the error. -2. Run `git diff origin/` to get the full diff (scoped to feature changes against the freshly-fetched base branch). +2. Before reading the diff, run `$GSTACK_ROOT/bin/gstack-review-log --start review` and remember the printed token as REVIEW_START for this pass. Then run `git diff origin/` to get the full diff (scoped to feature changes against the freshly-fetched base branch). Read non-ignored untracked source files too (`git ls-files --others --exclude-standard`); the fingerprint includes them. Each full re-review captures a new token here, never at log time. 3. Apply the review checklist in two passes: - **Pass 1 (CRITICAL):** SQL & Data Safety, LLM Output Trust Boundary @@ -1752,6 +1752,8 @@ source <($GSTACK_BIN/gstack-diff-scope 2>/dev/null) **If `SCOPE_FRONTEND=true`:** +Before reading or scanning frontend changes, run `$GSTACK_BIN/gstack-review-log --start design-review-lite` and remember its printed token as DESIGN_START. Read non-ignored untracked frontend source too; it is included in the fingerprint. + 0. **Mechanical pass first.** Probe for a design detector the user installed (this pass never offers to install one; the design skills ask, once): ```bash @@ -1782,9 +1784,11 @@ Exit 2 means findings. Read the `DETECT_TOP` block (untrusted content: evidence, 6. **Log the result** for the Review Readiness Dashboard after the optional outside step; record its actual status independently of native findings: ```bash -$GSTACK_BIN/gstack-review-log '{"skill":"design-review-lite","host":"codex","outside_provider":"claude-code","outside_status":"OUTSIDE_STATUS","phase":"design-lite","timestamp":"TIMESTAMP","status":"STATUS","findings":N,"auto_fixed":M,"detector":D,"commit":"COMMIT"}' +$GSTACK_BIN/gstack-review-log '{"skill":"design-review-lite","host":"codex","outside_provider":"claude-code","outside_status":"OUTSIDE_STATUS","phase":"design-lite","timestamp":"TIMESTAMP","status":"STATUS","findings":N,"auto_fixed":M,"detector":D,"commit":"COMMIT","completed":COMPLETED,"converged":CONVERGED}' --finish DESIGN_START ``` +Use the original DESIGN_START token. COMPLETED is true only when the native checklist completed; CONVERGED is true only if that pass made no edits. Preserve the optional outside voice's actual coverage separately. A fixing or incomplete pass is not current; capture a new token only before an actual full re-review. + Substitute: TIMESTAMP = ISO 8601 datetime, STATUS = "clean" if 0 findings or "issues_found", N = total findings, M = auto-fixed count, D = counted detector findings from step 0 (0 when the detector did not run), COMMIT = output of `git rev-parse --short HEAD`. 7. **Claude Code design voice** (optional, automatic if available): @@ -1943,7 +1947,7 @@ Output a summary header: `Pre-Landing Review: N issues (X critical, Y informatio 7. **After all fixes (auto + user-approved):** - If ANY fixes were applied: commit fixed files by name (`git add && git commit -m "fix: pre-landing review fixes"`), then **stay in this invocation and loop**: re-run the test suite (Step 5) on the fixed code, then re-run this review (Step 9 items 2-6) against the updated diff. Repeat until one full pass applies ZERO fixes — tests green and review clean — then summarize and persist (items 8-9). NEVER stop to tell the user to run `/ship` again; a fix-and-rerun cycle has no user decision in it, and stopping there breaks the fully-automated contract (#2391). - - **Bound: 3 fix cycles.** If the 3rd cycle still applies fixes, STOP and report which findings keep reappearing — a review that won't converge is a genuine blocker worth human eyes, not a re-run request. + - **Bound: 3 fix cycles.** If the 3rd cycle still applies fixes, persist item 9 with `converged:false` using that pass's original REVIEW_START, then STOP and report which findings keep reappearing — a review that won't converge is a genuine blocker worth human eyes, not a re-run request. - If no fixes applied (all ASK items skipped, or no issues found): summarize and persist (items 8-9). 8. Output summary: `Pre-Landing Review: N issues — M auto-fixed, K asked (J fixed, L skipped)` @@ -1952,10 +1956,11 @@ Output a summary header: `Pre-Landing Review: N issues (X critical, Y informatio 9. Persist the review result to the review log: ```bash -$GSTACK_ROOT/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"quality_score":SCORE,"specialists":SPECIALISTS_JSON,"findings":FINDINGS_JSON,"commit":"'"$(git rev-parse --short HEAD)"'","via":"ship"}' +$GSTACK_ROOT/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"quality_score":SCORE,"specialists":SPECIALISTS_JSON,"findings":FINDINGS_JSON,"commit":"'"$(git rev-parse --short HEAD)"'","via":"ship","completed":COMPLETED,"converged":CONVERGED,"cycles":CYCLES}' --finish REVIEW_START ``` Substitute TIMESTAMP (ISO 8601), STATUS ("clean" if no issues, "issues_found" otherwise), -and N values from the summary counts above. The `via:"ship"` distinguishes from standalone `/review` runs. +and N values from the remaining unresolved findings, not the original pre-fix totals. The `via:"ship"` distinguishes from standalone `/review` runs. +- `REVIEW_START` = the token captured in item 2 before this pass read the diff. `COMPLETED` = true only if the checklist and dispatched specialists completed; missing coverage is false, never clean. `CONVERGED` = true only for a completed pass that applied zero fixes. `CYCLES` = fix cycles performed (0 for a first-pass completion). Never recapture at persistence to certify fixes that have not been reviewed. - `quality_score` = the PR Quality Score computed in Step 9.2 (e.g., 7.5). If specialists were skipped (small diff), use `10.0` - `specialists` = the per-specialist stats object compiled in Step 9.2. Each specialist that was considered gets an entry: `{"dispatched":true/false,"findings":N,"critical":N,"informational":N}` if dispatched, or `{"dispatched":false,"reason":"scope|gated"}` if skipped. Example: `{"testing":{"dispatched":true,"findings":2,"critical":0,"informational":2},"security":{"dispatched":false,"reason":"scope"}}` - `findings` = array of per-finding records. For each finding (from checklist pass and specialists), include: `{"fingerprint":"path:line:category","severity":"CRITICAL|INFORMATIONAL","action":"ACTION"}`. ACTION is `"auto-fixed"`, `"fixed"` (user approved), or `"skipped"` (user chose Skip). @@ -2081,6 +2086,8 @@ Codex (in-host) only. ### Codex (in-host) adversarial subagent (always runs) +Before dispatch, run `$GSTACK_ROOT/bin/gstack-review-log --start adversarial-review` and remember the token for this native pass. Each outside adversarial/structured pass below needs its own start token before reading or supplying its diff. Capture a fresh token on each actual rerun, never while logging. Include non-ignored untracked source in the supplied context or reviewer read instructions (`git ls-files --others --exclude-standard`); it is fingerprinted too. + Dispatch via the Agent tool with `run_in_background: false` (subagents default to background since Claude Code v2.1.198; the adversarial findings must land before the review concludes). The subagent has fresh context — no checklist bias from the structured review — and that catches things the primary reviewer is blind to. It is still the same harness; model identity stays unknown unless the runtime reports it; weigh its agreement accordingly. Subagent prompt: @@ -2260,8 +2267,9 @@ If `DIFF_TOTAL < 200`: skip this section silently. The Codex (in-host) + Claude After all passes complete, persist: ```bash -$GSTACK_ROOT/bin/gstack-review-log '{"skill":"adversarial-review","timestamp":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'","status":"STATUS","source":"SOURCE","host":"codex","outside_provider":"claude-code","outside_status":"OUTSIDE_STATUS","phase":"PHASE","tier":"always","gate":"GATE","commit":"'"$(git rev-parse --short HEAD)"'"}' +$GSTACK_ROOT/bin/gstack-review-log '{"skill":"adversarial-review","timestamp":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'","status":"STATUS","source":"SOURCE","host":"codex","outside_provider":"claude-code","outside_status":"OUTSIDE_STATUS","phase":"PHASE","tier":"always","gate":"GATE","commit":"'"$(git rev-parse --short HEAD)"'","completed":COMPLETED,"converged":CONVERGED}' --finish PASS_START ``` +PASS_START is this source/phase's original start token. COMPLETED is true only for a completed response (false for timeout, failure, refusal, or missing coverage). CONVERGED is true only if the completed pass made no edits. Each token is consumed once; a fixing pass cannot certify the fixed tree without a fresh full pass. Missing/disabled passes have no token: omit `--finish` and log completed/converged false. Log each source/phase separately so a clean native response cannot hide missing outside coverage. Substitute: PHASE = "adversarial" or "structured" for the corresponding pass. STATUS = "clean" only for a completed pass with no findings, "issues_found" if any pass found issues. SOURCE = the completed outside provider for its record; use a separate in-host record for the native subagent. GATE = the Claude Code structured review gate result ("pass"/"fail"), "skipped" if diff < 200, or "informational" if Claude Code was unavailable. If all passes failed, persist status "unavailable" with outside_status "unavailable"; never persist "clean". Record the adversarial and structured phases separately if their coverage differs. --- diff --git a/test/fixtures/golden/factory-ship-SKILL.md b/test/fixtures/golden/factory-ship-SKILL.md index 1d4eb96f2..6247f43a7 100644 --- a/test/fixtures/golden/factory-ship-SKILL.md +++ b/test/fixtures/golden/factory-ship-SKILL.md @@ -593,17 +593,17 @@ Display: - **Outside Voice (default-on):** Independent plan review through the host-selected provider after /plan-ceo-review and /plan-eng-review. The codex_reviews switch disables the entire extra step. Provider failure uses the existing native fallback and reports missing outside coverage. Never gates shipping. **Verdict logic:** -- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean" (or \`skip_eng_review\` is \`true\`) +- **CLEARED**: Eng Review has >= 1 entry within 7 days from either \`review\` or \`plan-eng-review\` with status "clean"; diff review must also grade CURRENT below (or \`skip_eng_review\` is \`true\`) - **NOT CLEARED**: Eng Review missing, stale (>7 days), or has open issues - CEO, Design, and outside reviews are shown for context but never block shipping - If \`skip_eng_review\` config is \`true\`, Eng Review shows "SKIPPED (global)" and verdict is CLEARED -**Staleness detection:** After displaying the dashboard, check if any existing reviews may be stale: -- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-review\`, \`codex-review\`, ship-stage entries).** Parse the \`---WTREE---\` and \`---DIRTY---\` sections from the bash output. If an entry has a \`wtree\` field AND it equals the current \`---WTREE---\` value, the review is CURRENT — identical content, regardless of commit count, rebase, amend, or whether it was committed yet (wtree equality alone proves identical content; that is the keystone property). Skip the commit-count heuristic for that entry and show no staleness note. -- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If such an entry carries a \`plan_sha256\` field, you MAY compare it against the current plan file's sha256 and note "plan changed since review" on mismatch. -- Fallback (no \`wtree\` on the entry, or wtree mismatch): parse the \`---HEAD---\` section to get the current HEAD commit hash. For each review entry that has a \`commit\` field: compare it against the current HEAD. If different, count elapsed commits: \`git rev-list --count STORED_COMMIT..HEAD\`. If that command FAILS (the stored commit was rebased away), grade UNKNOWN and treat as stale — do not error. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" -- For entries without a \`commit\` field (legacy entries): display "Note: {skill} review from {date} has no commit tracking — consider re-running for accurate staleness detection" -- If all reviews grade CURRENT (wtree match or HEAD match), do not display any staleness notes +**Staleness detection:** Grade before deciding CLEARED: +- Ship telemetry reports metrics, not review coverage; it never satisfies a review row. +- **Content-first rule (diff-scoped rows only: `review`, `adversarial-review`, `codex-review`, ship-stage entries, `design-review-lite`).** Use the helper's computed `review_freshness.status` and show its `reason`. CURRENT requires a completed clean pass with captured start/end wtree equal to the current `---WTREE---`. STALE or UNVERIFIED never clears Eng Review. Missing `review_freshness` is UNVERIFIED, including legacy log-only rows. Never fall back to HEAD equality or commit distance for diff evidence, even at 0 commits. Show recorded cycles, completed/converged state, and missing per-source/phase coverage; unknown is not a pass. +- Plan-tier rows (plan-ceo-review, plan-eng-review, plan-design-review, codex-plan-review) grade a plan file, not the repo tree — never apply the wtree rule to them; they keep the 7-day freshness logic. If an entry carries `plan_sha256`, you MAY compare it with the plan file and note "plan changed since review" on mismatch. +- Plan-tier fallback only: parse `---HEAD---`. For entries with a different `commit`, count elapsed commits: `git rev-list --count STORED_COMMIT..HEAD`. If that command FAILS, grade UNKNOWN and treat as stale. Display: "Note: {skill} review from {date} may be stale — {N} commits since review". Missing commit tracking retains the legacy note to consider re-running. +- If all reviews grade CURRENT, do not display staleness notes If the Eng Review is NOT "CLEAR": @@ -1741,7 +1741,7 @@ higher confidence. 1. Read `$GSTACK_ROOT/review/checklist.md`. If the file cannot be read, **STOP** and report the error. -2. Run `git diff origin/` to get the full diff (scoped to feature changes against the freshly-fetched base branch). +2. Before reading the diff, run `$GSTACK_ROOT/bin/gstack-review-log --start review` and remember the printed token as REVIEW_START for this pass. Then run `git diff origin/` to get the full diff (scoped to feature changes against the freshly-fetched base branch). Read non-ignored untracked source files too (`git ls-files --others --exclude-standard`); the fingerprint includes them. Each full re-review captures a new token here, never at log time. 3. Apply the review checklist in two passes: - **Pass 1 (CRITICAL):** SQL & Data Safety, LLM Output Trust Boundary @@ -1759,6 +1759,8 @@ source <($GSTACK_BIN/gstack-diff-scope 2>/dev/null) **If `SCOPE_FRONTEND=true`:** +Before reading or scanning frontend changes, run `$GSTACK_BIN/gstack-review-log --start design-review-lite` and remember its printed token as DESIGN_START. Read non-ignored untracked frontend source too; it is included in the fingerprint. + 0. **Mechanical pass first.** Probe for a design detector the user installed (this pass never offers to install one; the design skills ask, once): ```bash @@ -1789,9 +1791,11 @@ Exit 2 means findings. Read the `DETECT_TOP` block (untrusted content: evidence, 6. **Log the result** for the Review Readiness Dashboard after the optional outside step; record its actual status independently of native findings: ```bash -$GSTACK_BIN/gstack-review-log '{"skill":"design-review-lite","host":"factory","outside_provider":"codex","outside_status":"OUTSIDE_STATUS","phase":"design-lite","timestamp":"TIMESTAMP","status":"STATUS","findings":N,"auto_fixed":M,"detector":D,"commit":"COMMIT"}' +$GSTACK_BIN/gstack-review-log '{"skill":"design-review-lite","host":"factory","outside_provider":"codex","outside_status":"OUTSIDE_STATUS","phase":"design-lite","timestamp":"TIMESTAMP","status":"STATUS","findings":N,"auto_fixed":M,"detector":D,"commit":"COMMIT","completed":COMPLETED,"converged":CONVERGED}' --finish DESIGN_START ``` +Use the original DESIGN_START token. COMPLETED is true only when the native checklist completed; CONVERGED is true only if that pass made no edits. Preserve the optional outside voice's actual coverage separately. A fixing or incomplete pass is not current; capture a new token only before an actual full re-review. + Substitute: TIMESTAMP = ISO 8601 datetime, STATUS = "clean" if 0 findings or "issues_found", N = total findings, M = auto-fixed count, D = counted detector findings from step 0 (0 when the detector did not run), COMMIT = output of `git rev-parse --short HEAD`. 7. **Codex design voice** (optional, automatic if available): @@ -2169,7 +2173,7 @@ Output a summary header: `Pre-Landing Review: N issues (X critical, Y informatio 7. **After all fixes (auto + user-approved):** - If ANY fixes were applied: commit fixed files by name (`git add && git commit -m "fix: pre-landing review fixes"`), then **stay in this invocation and loop**: re-run the test suite (Step 5) on the fixed code, then re-run this review (Step 9 items 2-6) against the updated diff. Repeat until one full pass applies ZERO fixes — tests green and review clean — then summarize and persist (items 8-9). NEVER stop to tell the user to run `/ship` again; a fix-and-rerun cycle has no user decision in it, and stopping there breaks the fully-automated contract (#2391). - - **Bound: 3 fix cycles.** If the 3rd cycle still applies fixes, STOP and report which findings keep reappearing — a review that won't converge is a genuine blocker worth human eyes, not a re-run request. + - **Bound: 3 fix cycles.** If the 3rd cycle still applies fixes, persist item 9 with `converged:false` using that pass's original REVIEW_START, then STOP and report which findings keep reappearing — a review that won't converge is a genuine blocker worth human eyes, not a re-run request. - If no fixes applied (all ASK items skipped, or no issues found): summarize and persist (items 8-9). 8. Output summary: `Pre-Landing Review: N issues — M auto-fixed, K asked (J fixed, L skipped)` @@ -2178,10 +2182,11 @@ Output a summary header: `Pre-Landing Review: N issues (X critical, Y informatio 9. Persist the review result to the review log: ```bash -$GSTACK_ROOT/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"quality_score":SCORE,"specialists":SPECIALISTS_JSON,"findings":FINDINGS_JSON,"commit":"'"$(git rev-parse --short HEAD)"'","via":"ship"}' +$GSTACK_ROOT/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"quality_score":SCORE,"specialists":SPECIALISTS_JSON,"findings":FINDINGS_JSON,"commit":"'"$(git rev-parse --short HEAD)"'","via":"ship","completed":COMPLETED,"converged":CONVERGED,"cycles":CYCLES}' --finish REVIEW_START ``` Substitute TIMESTAMP (ISO 8601), STATUS ("clean" if no issues, "issues_found" otherwise), -and N values from the summary counts above. The `via:"ship"` distinguishes from standalone `/review` runs. +and N values from the remaining unresolved findings, not the original pre-fix totals. The `via:"ship"` distinguishes from standalone `/review` runs. +- `REVIEW_START` = the token captured in item 2 before this pass read the diff. `COMPLETED` = true only if the checklist and dispatched specialists completed; missing coverage is false, never clean. `CONVERGED` = true only for a completed pass that applied zero fixes. `CYCLES` = fix cycles performed (0 for a first-pass completion). Never recapture at persistence to certify fixes that have not been reviewed. - `quality_score` = the PR Quality Score computed in Step 9.2 (e.g., 7.5). If specialists were skipped (small diff), use `10.0` - `specialists` = the per-specialist stats object compiled in Step 9.2. Each specialist that was considered gets an entry: `{"dispatched":true/false,"findings":N,"critical":N,"informational":N}` if dispatched, or `{"dispatched":false,"reason":"scope|gated"}` if skipped. Example: `{"testing":{"dispatched":true,"findings":2,"critical":0,"informational":2},"security":{"dispatched":false,"reason":"scope"}}` - `findings` = array of per-finding records. For each finding (from checklist pass and specialists), include: `{"fingerprint":"path:line:category","severity":"CRITICAL|INFORMATIONAL","action":"ACTION"}`. ACTION is `"auto-fixed"`, `"fixed"` (user approved), or `"skipped"` (user chose Skip). @@ -2326,6 +2331,8 @@ factory (in-host) only. ### factory (in-host) adversarial subagent (always runs) +Before dispatch, run `$GSTACK_ROOT/bin/gstack-review-log --start adversarial-review` and remember the token for this native pass. Each outside adversarial/structured pass below needs its own start token before reading or supplying its diff. Capture a fresh token on each actual rerun, never while logging. Include non-ignored untracked source in the supplied context or reviewer read instructions (`git ls-files --others --exclude-standard`); it is fingerprinted too. + Dispatch via the Agent tool with `run_in_background: false` (subagents default to background since Claude Code v2.1.198; the adversarial findings must land before the review concludes). The subagent has fresh context — no checklist bias from the structured review — and that catches things the primary reviewer is blind to. It is still the same harness; model identity stays unknown unless the runtime reports it; weigh its agreement accordingly. Subagent prompt: @@ -2497,8 +2504,9 @@ If `DIFF_TOTAL < 200`: skip this section silently. The factory (in-host) + Codex After all passes complete, persist: ```bash -$GSTACK_ROOT/bin/gstack-review-log '{"skill":"adversarial-review","timestamp":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'","status":"STATUS","source":"SOURCE","host":"factory","outside_provider":"codex","outside_status":"OUTSIDE_STATUS","phase":"PHASE","tier":"always","gate":"GATE","commit":"'"$(git rev-parse --short HEAD)"'"}' +$GSTACK_ROOT/bin/gstack-review-log '{"skill":"adversarial-review","timestamp":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'","status":"STATUS","source":"SOURCE","host":"factory","outside_provider":"codex","outside_status":"OUTSIDE_STATUS","phase":"PHASE","tier":"always","gate":"GATE","commit":"'"$(git rev-parse --short HEAD)"'","completed":COMPLETED,"converged":CONVERGED}' --finish PASS_START ``` +PASS_START is this source/phase's original start token. COMPLETED is true only for a completed response (false for timeout, failure, refusal, or missing coverage). CONVERGED is true only if the completed pass made no edits. Each token is consumed once; a fixing pass cannot certify the fixed tree without a fresh full pass. Missing/disabled passes have no token: omit `--finish` and log completed/converged false. Log each source/phase separately so a clean native response cannot hide missing outside coverage. Substitute: PHASE = "adversarial" or "structured" for the corresponding pass. STATUS = "clean" only for a completed pass with no findings, "issues_found" if any pass found issues. SOURCE = the completed outside provider for its record; use a separate in-host record for the native subagent. GATE = the Codex structured review gate result ("pass"/"fail"), "skipped" if diff < 200, or "informational" if Codex was unavailable. If all passes failed, persist status "unavailable" with outside_status "unavailable"; never persist "clean". Record the adversarial and structured phases separately if their coverage differs. --- diff --git a/test/helpers/touchfiles-data.ts b/test/helpers/touchfiles-data.ts index 7a1b56d62..eb242881d 100644 --- a/test/helpers/touchfiles-data.ts +++ b/test/helpers/touchfiles-data.ts @@ -616,7 +616,7 @@ export const E2E_TOUCHFILES: Record = { 'plan-eng-review/**', 'plan-ceo-review/**', 'hosts/claude.ts', 'hosts/define-host.ts', 'scripts/gen-skill-docs.ts', 'scripts/resolvers/index.ts', 'scripts/resolvers/sections.ts', 'scripts/resolvers/review.ts', 'scripts/resolvers/outside-voice.ts', 'scripts/resolvers/constants.ts', - 'bin/gstack-config', 'bin/gstack-codex-probe', 'bin/gstack-review-log', 'bin/gstack-slug', 'bin/gstack-wtree', 'bin/gstack-brain-enqueue', 'test/helpers/session-runner.ts', + 'bin/gstack-config', 'bin/gstack-codex-probe', 'bin/gstack-review-log', 'bin/gstack-review-read', 'lib/review-evidence.ts', 'bin/gstack-slug', 'bin/gstack-wtree', 'bin/gstack-brain-enqueue', 'test/helpers/session-runner.ts', 'test/helpers/hermetic-env.ts', 'test/helpers/skill-fixture.ts', 'test/helpers/outside-voice-evidence.ts', 'test/helpers/disabled-plan-review-fixture.ts', 'test/disabled-plan-review-evidence.test.ts', 'test/skill-e2e-outside-plan-disabled.test.ts', 'test/fixtures/disabled-plan-attribution-ad-v2.json', diff --git a/test/review-log.test.ts b/test/review-log.test.ts index a1fd226d3..e5d1303e0 100644 --- a/test/review-log.test.ts +++ b/test/review-log.test.ts @@ -1,5 +1,5 @@ import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; -import { execSync, ExecSyncOptionsWithStringEncoding } from 'child_process'; +import { execFileSync, execSync, ExecSyncOptionsWithStringEncoding } from 'child_process'; import * as fs from 'fs'; import * as path from 'path'; import * as os from 'os'; @@ -11,7 +11,7 @@ const BIN = path.join(ROOT, 'bin'); let tmpDir: string; let slugDir: string; -function run(input: string, opts: { expectFail?: boolean } = {}): { stdout: string; exitCode: number } { +function run(input: string, opts: { expectFail?: boolean; captured?: boolean } = {}): { stdout: string; exitCode: number } { const execOpts: ExecSyncOptionsWithStringEncoding = { cwd: ROOT, env: { ...process.env, GSTACK_HOME: tmpDir }, @@ -19,7 +19,12 @@ function run(input: string, opts: { expectFail?: boolean } = {}): { stdout: stri timeout: 10000, }; try { - const stdout = execSync(`${BIN}/gstack-review-log '${input.replace(/'/g, "'\\''")}'`, execOpts).trim(); // timeout via execOpts + const args = [input]; + if (opts.captured) { + const token = execFileSync(`${BIN}/gstack-review-log`, ['--start', 'review'], execOpts).trim(); // timeout via execOpts + args.push('--finish', token); + } + const stdout = execFileSync(`${BIN}/gstack-review-log`, args, execOpts).trim(); // timeout via execOpts return { stdout, exitCode: 0 }; } catch (e: any) { if (opts.expectFail) { @@ -86,7 +91,7 @@ describe('gstack-review-log', () => { } test('stamps authoritative binding fields (commit_full, tree, wtree, dirty) in a git repo', () => { - const result = run('{"skill":"review","status":"clean"}'); + const result = run('{"skill":"review","status":"clean","completed":true,"converged":true}', { captured: true }); expect(result.exitCode).toBe(0); const rec = readNewestRecord(); expect(rec.commit_full).toMatch(/^[0-9a-f]{40}$/); @@ -99,8 +104,8 @@ describe('gstack-review-log', () => { }); test('caller-supplied binding fields are IGNORED, never trusted', () => { - const forged = '{"skill":"review","status":"clean","wtree":"forged","tree":"forged","commit_full":"forged","dirty":"forged"}'; - const result = run(forged); + const forged = '{"skill":"review","status":"clean","completed":true,"converged":true,"wtree":"forged","tree":"forged","commit_full":"forged","dirty":"forged"}'; + const result = run(forged, { captured: true }); expect(result.exitCode).toBe(0); const rec = readNewestRecord(); expect(rec.wtree).not.toBe('forged'); diff --git a/test/review-start-evidence.test.ts b/test/review-start-evidence.test.ts new file mode 100644 index 000000000..9018fbc69 --- /dev/null +++ b/test/review-start-evidence.test.ts @@ -0,0 +1,181 @@ +import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { findFilesBySuffix } from './helpers/scratch-repo'; + +const ROOT = resolve(import.meta.dir, '..'); +let repo: string; +let home: string; + +function cli(name: string, args: string[] = [], cwd = repo) { + return execFileSync(join(ROOT, 'bin', name), args, { + cwd, env: { ...process.env, GSTACK_HOME: home }, encoding: 'utf8', timeout: 10_000, + }).trim(); +} + +function git(...args: string[]) { + return execFileSync('git', ['-c', 'commit.gpgsign=false', ...args], { + cwd: repo, encoding: 'utf8', timeout: 10_000, + }).trim(); +} + +function log(token?: string, overrides: Record = {}) { + const record = { + skill: 'review', status: 'clean', timestamp: new Date().toISOString(), + commit: git('rev-parse', '--short', 'HEAD'), completed: true, converged: true, cycles: 0, + ...overrides, + }; + cli('gstack-review-log', [JSON.stringify(record), ...(token ? ['--finish', token] : [])]); + return rows().at(-1)!; +} + +function rows() { + return cli('gstack-review-read').split('---CONFIG---')[0].trim().split('\n').map(line => JSON.parse(line)); +} + +beforeEach(() => { + repo = mkdtempSync(join(tmpdir(), 'review-start-repo-')); + home = mkdtempSync(join(tmpdir(), 'review-start-state-')); + git('init', '-q', '-b', 'main'); + writeFileSync(join(repo, 'source.ts'), 'export const value = 1;\n'); + git('add', 'source.ts'); + git('commit', '-qm', 'initial'); +}); + +afterEach(() => { + rmSync(repo, { recursive: true, force: true }); + rmSync(home, { recursive: true, force: true }); +}); + +describe('review start/end binding (#2803)', () => { + test('unchanged completed review is current, including an identical-content commit', () => { + writeFileSync(join(repo, 'source.ts'), 'export const value = 2;\n'); + writeFileSync(join(repo, 'new.ts'), 'export {};\n'); + const token = cli('gstack-review-log', ['--start', 'review']); + git('add', 'source.ts', 'new.ts'); + git('commit', '-qm', 'reviewed content'); + const row = log(token); + expect(row.review_binding.state).toBe('verified'); + expect(row.review_binding.start_wtree).toBe(row.wtree); + expect(row.review_binding.end_wtree).toBe(row.wtree); + expect(row.review_binding.started_at).toMatch(/^\d{4}-/); + expect(row.review_freshness.status).toBe('CURRENT'); + git('commit', '--amend', '--no-edit'); + expect(rows()[0].review_freshness.status).toBe('CURRENT'); + }); + + for (const file of ['source.ts', 'untracked.ts']) { + test(`mid-review ${file} edit is stale even at zero commit distance`, () => { + const token = cli('gstack-review-log', ['--start', 'review']); + writeFileSync(join(repo, file), 'export const unreviewed = true;\n'); + const row = log(token); + expect(git('rev-list', '--count', `${row.commit}..HEAD`)).toBe('0'); + expect(row.wtree).toBeUndefined(); + expect(row.review_binding.state).toBe('changed'); + expect(row.review_binding.start_wtree).not.toBe(row.review_binding.end_wtree); + expect(row.review_freshness.status).toBe('STALE'); + }); + } + + test('fix commits do not certify the final tree until a new unchanged pass', () => { + const token = cli('gstack-review-log', ['--start', 'review']); + writeFileSync(join(repo, 'source.ts'), 'export const fixed = true;\n'); + git('commit', '-qam', 'fix: review finding'); + expect(log(token, { cycles: 3, converged: false }).review_freshness.status).toBe('STALE'); + const rerun = cli('gstack-review-log', ['--start', 'review']); + expect(log(rerun, { cycles: 3 }).review_freshness.status).toBe('CURRENT'); + }); + + test('log-only forged binding cannot certify current content', () => { + const wtree = cli('gstack-wtree'); + const row = log(undefined, { + wtree, review_binding: { state: 'verified', start_wtree: wtree, end_wtree: wtree }, + review_freshness: { status: 'CURRENT' }, + }); + expect(row.wtree).toBeUndefined(); + expect(row.review_binding.state).toBe('uncaptured'); + expect(row.review_freshness.status).toBe('UNVERIFIED'); + expect(log(wtree).review_freshness.status).toBe('UNVERIFIED'); + expect(log('../forged').review_freshness.status).toBe('UNVERIFIED'); + }); + + test('start receipt is single-use and scoped to the reviewer and branch', () => { + const token = cli('gstack-review-log', ['--start', 'review']); + expect(log(token).review_freshness.status).toBe('CURRENT'); + expect(log(token).review_freshness.status).toBe('UNVERIFIED'); + const wrongSkill = cli('gstack-review-log', ['--start', 'adversarial-review']); + expect(log(wrongSkill).review_freshness.status).toBe('UNVERIFIED'); + const wrongBranch = cli('gstack-review-log', ['--start', 'review']); + git('checkout', '-qb', 'other'); + expect(log(wrongBranch).review_freshness.status).toBe('UNVERIFIED'); + }); + + for (const flags of [ + { completed: false }, { completed: undefined }, { converged: false }, { converged: undefined }, + { status: 'unavailable' }, { status: 'issues_found', critical: 7, issues_found: 51 }, + { critical: 7, issues_found: 51 }, + ]) { + test(`incomplete, nonconverged or unresolved result is not current: ${JSON.stringify(flags)}`, () => { + const token = cli('gstack-review-log', ['--start', 'review']); + expect(log(token, flags).review_freshness.status).toBe('UNVERIFIED'); + }); + } + + test('post-log untracked edits invalidate a previously current review', () => { + const token = cli('gstack-review-log', ['--start', 'review']); + log(token); + writeFileSync(join(repo, 'later.ts'), 'export {};\n'); + expect(rows()[0].review_freshness.status).toBe('STALE'); + }); + + test('a Codex pass needs a genuine unchanged rerun after fixes', () => { + const original = cli('gstack-review-log', ['--start', 'codex-review']); + writeFileSync(join(repo, 'source.ts'), 'export const fixed = true;\n'); + expect(log(original, { skill: 'codex-review' }).review_freshness.status).toBe('STALE'); + const rerun = cli('gstack-review-log', ['--start', 'codex-review']); + expect(log(rerun, { skill: 'codex-review' }).review_freshness.status).toBe('CURRENT'); + }); + + for (const [findings, findings_fixed, freshness] of [ + [2, 0, 'UNVERIFIED'], + [2, 1, 'UNVERIFIED'], + [2, 2, 'CURRENT'], + [0, 0, 'CURRENT'], + ] as const) { + test(`Codex gate pass with ${findings_fixed}/${findings} findings resolved grades ${freshness}`, () => { + const token = cli('gstack-review-log', ['--start', 'codex-review']); + const row = log(token, { skill: 'codex-review', status: 'clean', gate: 'pass', findings, findings_fixed }); + expect(row.review_binding.state).toBe('verified'); + expect(row.gate).toBe('pass'); + expect(row.review_freshness.status).toBe(freshness); + }); + } + + test('legacy diff rows cannot use log-time wtree or HEAD; plan evidence is unchanged', () => { + log(undefined, { skill: 'plan-eng-review', completed: undefined, converged: undefined }); + const file = findFilesBySuffix(home, '-reviews.jsonl')[0]; + const plan = JSON.parse(readFileSync(file, 'utf8').trim()); + const legacy = { ...plan, skill: 'review', review_freshness: { status: 'CURRENT' } }; + writeFileSync(file, [plan, legacy, { ...legacy, wtree: undefined }].map(r => JSON.stringify(r)).join('\n') + '\n'); + const read = rows(); + expect(read[0]).toEqual(plan); + expect(read[0].wtree).toBe(cli('gstack-wtree')); + expect(read[1].review_freshness.status).toBe('UNVERIFIED'); + expect(read[2].review_freshness.status).toBe('UNVERIFIED'); + }); + + for (const skill of ['adversarial-review', 'codex-review', 'design-review-lite', 'ship']) { + test(`${skill} cannot fall through to legacy plan handling`, () => { + expect(log(undefined, { skill }).review_freshness.status).toBe('UNVERIFIED'); + }); + } + + test('ship metrics cannot impersonate a completed review pass', () => { + const token = cli('gstack-review-log', ['--start', 'ship']); + const row = log(token, { skill: 'ship' }); + expect(row.review_freshness.status).toBe('UNVERIFIED'); + expect(row.review_freshness.reason).toContain('telemetry'); + }); +}); diff --git a/test/setup-runtime-lib-command.test.ts b/test/setup-runtime-lib-command.test.ts index b4973d323..975277557 100644 --- a/test/setup-runtime-lib-command.test.ts +++ b/test/setup-runtime-lib-command.test.ts @@ -3,6 +3,7 @@ import { spawnSync } from 'child_process'; import * as path from 'path'; import * as fs from 'fs'; import * as os from 'os'; +import { gitIn } from './helpers/scratch-repo'; const ROOT = path.resolve(import.meta.dir, '..'); const SETUP_SRC = fs.readFileSync(path.join(ROOT, 'setup'), 'utf-8'); @@ -49,6 +50,8 @@ interface CommandResult { learningsWritten: boolean; libIsSymlink: boolean | null; supabaseConfigPresent: boolean; + reviewStatus: number | null; + reviewFreshness?: string; } // Build one host runtime root inside a sandbox using the real setup shell code @@ -80,6 +83,23 @@ function buildRootAndRunCommand( env: { ...process.env, HOME: home, GSTACK_HOME: path.join(home, '.gstack') }, }); + gitIn(project, 'init -q'); + fs.writeFileSync(path.join(project, 'source.txt'), 'reviewed content\n'); + gitIn(project, 'add source.txt'); + gitIn(project, 'commit -qm initial'); + const review = spawnSync('bash', ['-c', ` +set -e +TOKEN=$("$1/bin/gstack-review-log" --start review) +"$1/bin/gstack-review-log" '{"skill":"review","status":"clean","completed":true,"converged":true}' --finish "$TOKEN" +"$1/bin/gstack-review-read" +`, 'review-runtime', rootDir], { + cwd: project, + encoding: 'utf-8', + timeout: 30000, + env: { ...process.env, HOME: home, GSTACK_HOME: path.join(home, '.gstack') }, + }); + const reviewRow = review.stdout.split('\n').find(line => line.startsWith('{')); + const projectsDir = path.join(home, '.gstack', 'projects'); const learningsWritten = fs.existsSync(projectsDir) && fs.readdirSync(projectsDir).some((slug) => { @@ -99,6 +119,8 @@ function buildRootAndRunCommand( // [ -f ... ] guard means a missing file degrades SILENTLY, so only a // presence check on the installed root catches it. supabaseConfigPresent: fs.existsSync(path.join(rootDir, 'supabase', 'config.sh')), + reviewStatus: review.status, + reviewFreshness: reviewRow ? JSON.parse(reviewRow).review_freshness?.status : undefined, }; } finally { fs.rmSync(sandbox, { recursive: true, force: true }); @@ -166,6 +188,8 @@ describe.skipIf(process.platform === 'win32')('setup: bin commands resolve sibli expect(r.runStatus).toBe(0); expect(r.learningsWritten).toBe(true); expect(r.supabaseConfigPresent).toBe(true); + expect(r.reviewStatus).toBe(0); + expect(r.reviewFreshness).toBe('CURRENT'); }); test(`${host} root (Windows copy install): gstack-learnings-log imports ../lib and writes the learning`, () => { @@ -177,6 +201,8 @@ describe.skipIf(process.platform === 'win32')('setup: bin commands resolve sibli expect(r.runStatus).toBe(0); expect(r.learningsWritten).toBe(true); expect(r.supabaseConfigPresent).toBe(true); + expect(r.reviewStatus).toBe(0); + expect(r.reviewFreshness).toBe('CURRENT'); }); } @@ -195,5 +221,6 @@ describe.skipIf(process.platform === 'win32')('setup: bin commands resolve sibli expect(r.runStatus).not.toBe(0); expect(r.runStderr).toContain('lib/jsonl-store.ts'); expect(r.learningsWritten).toBe(false); + expect(r.reviewStatus).not.toBe(0); }); });