From 8e4a4f7c3d46c48181a5a3afc0b0973895e923a5 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sat, 15 Aug 2026 23:17:02 -0700 Subject: [PATCH] feat(review): content-addressed staleness via working-tree fingerprint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review records now bind to the content they were made on. bin/gstack-review-log stamps every appended record with commit_full, tree, dirty (informational) and wtree — a working-tree fingerprint from the new bin/gstack-wtree (temp index seeded from HEAD + git add -A + write-tree). The binding fields are computed authoritatively; caller-supplied values for those keys are ignored, so a stale rendered template or a forged field can't bind a record to content it wasn't made on. Why a working-tree fingerprint instead of HEAD^{tree}: committing identical content doesn't change it (a record made on a dirty tree stays valid after the same content is committed), untracked new source files DO change it (new code can't hide from freshness), and gitignored scratch stays out. Rebase, amend and squash with identical content grade CURRENT instead of stale. Grading: the dashboard (scripts/resolvers/review.ts) and /land-and-deploy Step 3.5a apply a content-first rule to diff-scoped review rows — wtree match with both sides clean is CURRENT, full stop. Plan-tier reviews grade a plan file, not the repo tree, so they keep the 7-day logic (optional plan_sha256 caller field noted). The rev-list fallback no longer errors when the stored commit was rebased away: it grades UNKNOWN and treats it as stale. bin/gstack-review-read emits ---WTREE---/---TREE---/---DIRTY--- so graders consume one tool output. Old records without wtree fall back to the existing heuristics; no migration. Co-Authored-By: Claude Fable 5 --- bin/gstack-review-log | 43 +++++- bin/gstack-review-read | 13 ++ bin/gstack-wtree | 27 ++++ devex-review/SKILL.md | 7 +- land-and-deploy/SKILL.md | 20 ++- land-and-deploy/SKILL.md.tmpl | 20 ++- plan-ceo-review/sections/review-sections.md | 7 +- .../sections/review-sections.md | 7 +- plan-devex-review/sections/review-sections.md | 7 +- plan-eng-review/sections/review-sections.md | 7 +- scripts/resolvers/review.ts | 7 +- ship/SKILL.md | 7 +- test/review-log.test.ts | 135 ++++++++++++++++++ 13 files changed, 276 insertions(+), 31 deletions(-) create mode 100755 bin/gstack-wtree diff --git a/bin/gstack-review-log b/bin/gstack-review-log index fba2ee7d9..5459d2ae2 100755 --- a/bin/gstack-review-log +++ b/bin/gstack-review-log @@ -1,21 +1,56 @@ #!/usr/bin/env bash # 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). 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" -# Validate: input must be parseable JSON (reject malformed or injection attempts) INPUT="$1" -if ! printf '%s' "$INPUT" | bun -e "JSON.parse(await Bun.stdin.text())" 2>/dev/null; then + +# Compute binding fields (best-effort; empty outside a git repo). +COMMIT_FULL=$(git rev-parse HEAD 2>/dev/null || true) +TREE="" +WTREE="" +DIRTY="" +if [ -n "$COMMIT_FULL" ]; then + TREE=$(git rev-parse 'HEAD^{tree}' 2>/dev/null || true) + WTREE=$("$SCRIPT_DIR/gstack-wtree" 2>/dev/null || true) + if [ -n "$(git status --porcelain -uno 2>/dev/null | head -1)" ]; then + DIRTY="true" + else + DIRTY="false" + fi +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 " +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)); +" 2>/dev/null) || { # Not valid JSON — refuse to append echo "gstack-review-log: invalid JSON, skipping" >&2 exit 1 -fi +} -echo "$INPUT" >> "$GSTACK_HOME/projects/$SLUG/$BRANCH-reviews.jsonl" +echo "$STAMPED" >> "$GSTACK_HOME/projects/$SLUG/$BRANCH-reviews.jsonl" # gbrain-sync: enqueue for cross-machine sync (no-op if sync is off). "$SCRIPT_DIR/gstack-brain-enqueue" "projects/$SLUG/$BRANCH-reviews.jsonl" 2>/dev/null & diff --git a/bin/gstack-review-read b/bin/gstack-review-read index ccf1d70f6..8400240ee 100755 --- a/bin/gstack-review-read +++ b/bin/gstack-review-read @@ -1,6 +1,13 @@ #!/usr/bin/env bash # 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), +# ---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. set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" eval "$("$SCRIPT_DIR/gstack-slug" 2>/dev/null)" @@ -10,3 +17,9 @@ 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 "---TREE---" +git rev-parse 'HEAD^{tree}' 2>/dev/null || echo "unknown" +echo "---DIRTY---" +if [ -n "$(git status --porcelain -uno 2>/dev/null | head -1)" ]; then echo "true"; else echo "false"; fi diff --git a/bin/gstack-wtree b/bin/gstack-wtree new file mode 100755 index 000000000..0375c7154 --- /dev/null +++ b/bin/gstack-wtree @@ -0,0 +1,27 @@ +#!/usr/bin/env bash +# gstack-wtree — print a working-tree CONTENT fingerprint (a git tree hash). +# +# Builds a temp index seeded from HEAD, stages the full working tree into it +# (`git add -A`, so .gitignore'd scratch stays out and UNTRACKED source is +# included), and prints `git write-tree` of that index. Properties that make +# this the right staleness fingerprint, vs `git rev-parse HEAD^{tree}`: +# +# - Committing identical content does NOT change the fingerprint, so a +# record made on a dirty tree stays valid after the exact same content is +# committed (the /ship Step 5 -> Step 16 case). +# - Untracked new source files DO change the fingerprint, so "tests passed" +# can't stay FRESH after a new file appears. +# - Rebase/amend/squash that preserve content do not change it. +# +# The real repo index is never touched. Staged blobs land in the object store +# as unreachable objects and get gc'd like stash churn. Exit 1 outside a git +# repo or in a repo with no commits — callers treat that as "no fingerprint". +set -euo pipefail + +TOP=$(git rev-parse --show-toplevel 2>/dev/null) || exit 1 +TMPIDX=$(mktemp "${TMPDIR:-/tmp}/gstack-wtree-XXXXXX") +trap 'rm -f "$TMPIDX"' EXIT +export GIT_INDEX_FILE="$TMPIDX" +git -C "$TOP" read-tree HEAD 2>/dev/null || exit 1 +git -C "$TOP" add -A 2>/dev/null || exit 1 +git -C "$TOP" write-tree 2>/dev/null diff --git a/devex-review/SKILL.md b/devex-review/SKILL.md index 1fe9f52d5..b00342ff9 100644 --- a/devex-review/SKILL.md +++ b/devex-review/SKILL.md @@ -1177,10 +1177,11 @@ Display: - 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: -- Parse the \`---HEAD---\` section from the bash output 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\`. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" +- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-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 AND the entry's \`dirty\` is false AND \`---DIRTY---\` is false, the review is CURRENT — identical content, regardless of commit count, rebase, or amend. 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, wtree mismatch, or either side dirty): 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 match the current HEAD, do not display any staleness notes +- If all reviews grade CURRENT (wtree match or HEAD match), do not display any staleness notes ## Plan File Review Report diff --git a/land-and-deploy/SKILL.md b/land-and-deploy/SKILL.md index 0e3d23509..444c07261 100644 --- a/land-and-deploy/SKILL.md +++ b/land-and-deploy/SKILL.md @@ -1298,13 +1298,25 @@ plan-design-review, design-review-lite, codex-review, review, adversarial-review codex-plan-review): 1. Find the most recent entry within the last 7 days. -2. Extract its `commit` field. -3. Compare against current HEAD: `git rev-list --count STORED_COMMIT..HEAD` +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 AND the entry's `dirty` is false + AND `---DIRTY---` is false → **CURRENT**, full stop. Identical working-tree + content, regardless of commit count, rebase, or amend — 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. +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:** +**Staleness rules (fallback path):** - 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) +- rev-list failed → UNKNOWN (treat as STALE) - No review found → NOT RUN **Critical check:** Look at what changed AFTER the last review. Run: @@ -1314,6 +1326,8 @@ 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.) **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. diff --git a/land-and-deploy/SKILL.md.tmpl b/land-and-deploy/SKILL.md.tmpl index 0c8e17261..6a3f479ab 100644 --- a/land-and-deploy/SKILL.md.tmpl +++ b/land-and-deploy/SKILL.md.tmpl @@ -394,13 +394,25 @@ plan-design-review, design-review-lite, codex-review, review, adversarial-review codex-plan-review): 1. Find the most recent entry within the last 7 days. -2. Extract its `commit` field. -3. Compare against current HEAD: `git rev-list --count STORED_COMMIT..HEAD` +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 AND the entry's `dirty` is false + AND `---DIRTY---` is false → **CURRENT**, full stop. Identical working-tree + content, regardless of commit count, rebase, or amend — 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. +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:** +**Staleness rules (fallback path):** - 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) +- rev-list failed → UNKNOWN (treat as STALE) - No review found → NOT RUN **Critical check:** Look at what changed AFTER the last review. Run: @@ -410,6 +422,8 @@ 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.) **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. diff --git a/plan-ceo-review/sections/review-sections.md b/plan-ceo-review/sections/review-sections.md index 71bae4d93..ba521387d 100644 --- a/plan-ceo-review/sections/review-sections.md +++ b/plan-ceo-review/sections/review-sections.md @@ -669,10 +669,11 @@ Display: - 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: -- Parse the \`---HEAD---\` section from the bash output 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\`. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" +- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-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 AND the entry's \`dirty\` is false AND \`---DIRTY---\` is false, the review is CURRENT — identical content, regardless of commit count, rebase, or amend. 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, wtree mismatch, or either side dirty): 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 match the current HEAD, do not display any staleness notes +- If all reviews grade CURRENT (wtree match or HEAD match), do not display any 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 fde4b79f9..690fe37a5 100644 --- a/plan-design-review/sections/review-sections.md +++ b/plan-design-review/sections/review-sections.md @@ -405,10 +405,11 @@ Display: - 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: -- Parse the \`---HEAD---\` section from the bash output 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\`. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" +- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-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 AND the entry's \`dirty\` is false AND \`---DIRTY---\` is false, the review is CURRENT — identical content, regardless of commit count, rebase, or amend. 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, wtree mismatch, or either side dirty): 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 match the current HEAD, do not display any staleness notes +- If all reviews grade CURRENT (wtree match or HEAD match), do not display any 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 e4ce30a95..7bcfdf6d4 100644 --- a/plan-devex-review/sections/review-sections.md +++ b/plan-devex-review/sections/review-sections.md @@ -643,10 +643,11 @@ Display: - 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: -- Parse the \`---HEAD---\` section from the bash output 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\`. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" +- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-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 AND the entry's \`dirty\` is false AND \`---DIRTY---\` is false, the review is CURRENT — identical content, regardless of commit count, rebase, or amend. 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, wtree mismatch, or either side dirty): 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 match the current HEAD, do not display any staleness notes +- If all reviews grade CURRENT (wtree match or HEAD match), do not display any 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 caae69a7e..57ad2f984 100644 --- a/plan-eng-review/sections/review-sections.md +++ b/plan-eng-review/sections/review-sections.md @@ -728,10 +728,11 @@ Display: - 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: -- Parse the \`---HEAD---\` section from the bash output 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\`. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" +- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-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 AND the entry's \`dirty\` is false AND \`---DIRTY---\` is false, the review is CURRENT — identical content, regardless of commit count, rebase, or amend. 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, wtree mismatch, or either side dirty): 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 match the current HEAD, do not display any staleness notes +- If all reviews grade CURRENT (wtree match or HEAD match), do not display any staleness notes ## Plan File Review Report diff --git a/scripts/resolvers/review.ts b/scripts/resolvers/review.ts index aea233dbe..dc1f7306b 100644 --- a/scripts/resolvers/review.ts +++ b/scripts/resolvers/review.ts @@ -67,10 +67,11 @@ Display: - 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: -- Parse the \\\`---HEAD---\\\` section from the bash output 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\\\`. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" +- **Content-first rule (diff-scoped rows only: \\\`review\\\`, \\\`adversarial-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 AND the entry's \\\`dirty\\\` is false AND \\\`---DIRTY---\\\` is false, the review is CURRENT — identical content, regardless of commit count, rebase, or amend. 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, wtree mismatch, or either side dirty): 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 match the current HEAD, do not display any staleness notes`; +- If all reviews grade CURRENT (wtree match or HEAD match), do not display any staleness notes`; } export function generatePlanFileReviewReport(_ctx: TemplateContext): string { diff --git a/ship/SKILL.md b/ship/SKILL.md index 1446dd74b..94d4c714f 100644 --- a/ship/SKILL.md +++ b/ship/SKILL.md @@ -993,10 +993,11 @@ Display: - 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: -- Parse the \`---HEAD---\` section from the bash output 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\`. Display: "Note: {skill} review from {date} may be stale — {N} commits since review" +- **Content-first rule (diff-scoped rows only: \`review\`, \`adversarial-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 AND the entry's \`dirty\` is false AND \`---DIRTY---\` is false, the review is CURRENT — identical content, regardless of commit count, rebase, or amend. 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, wtree mismatch, or either side dirty): 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 match the current HEAD, do not display any staleness notes +- If all reviews grade CURRENT (wtree match or HEAD match), do not display any staleness notes If the Eng Review is NOT "CLEAR": diff --git a/test/review-log.test.ts b/test/review-log.test.ts index f418fa298..f42b16ab3 100644 --- a/test/review-log.test.ts +++ b/test/review-log.test.ts @@ -74,4 +74,139 @@ describe('gstack-review-log', () => { } } }); + + function readNewestRecord(): any { + const projectDirs = fs.readdirSync(slugDir); + const projectDir = path.join(slugDir, projectDirs[0]); + const jsonlFiles = fs.readdirSync(projectDir).filter((f) => f.endsWith('.jsonl')); + const content = fs.readFileSync(path.join(projectDir, jsonlFiles[0]), 'utf-8').trim(); + const lines = content.split('\n'); + return JSON.parse(lines[lines.length - 1]); + } + + test('stamps authoritative binding fields (commit_full, tree, wtree, dirty) in a git repo', () => { + const result = run('{"skill":"review","status":"clean"}'); + expect(result.exitCode).toBe(0); + const rec = readNewestRecord(); + expect(rec.commit_full).toMatch(/^[0-9a-f]{40}$/); + expect(rec.tree).toMatch(/^[0-9a-f]{40}$/); + expect(rec.wtree).toMatch(/^[0-9a-f]{40}$/); + expect(typeof rec.dirty).toBe('boolean'); + // Non-binding caller fields pass through untouched. + expect(rec.skill).toBe('review'); + expect(rec.status).toBe('clean'); + }); + + 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); + expect(result.exitCode).toBe(0); + const rec = readNewestRecord(); + expect(rec.wtree).not.toBe('forged'); + expect(rec.tree).not.toBe('forged'); + expect(rec.commit_full).not.toBe('forged'); + expect(rec.dirty).not.toBe('forged'); + expect(rec.wtree).toMatch(/^[0-9a-f]{40}$/); + }); + + test('append still succeeds outside a git repo (binding fields omitted)', () => { + const nonGit = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-nongit-')); + try { + const execOpts: ExecSyncOptionsWithStringEncoding = { + cwd: nonGit, + env: { ...process.env, GSTACK_HOME: tmpDir }, + encoding: 'utf-8', + timeout: 10000, + }; + execSync(`${BIN}/gstack-review-log '{"skill":"review","status":"clean"}'`, execOpts); + // A record landed somewhere under projects/ without a wtree stamp. + const found: string[] = []; + const walk = (d: string) => { + for (const e of fs.readdirSync(d, { withFileTypes: true })) { + const p = path.join(d, e.name); + if (e.isDirectory()) walk(p); + else if (e.name.endsWith('-reviews.jsonl')) found.push(p); + } + }; + walk(slugDir); + expect(found.length).toBeGreaterThan(0); + const rec = JSON.parse(fs.readFileSync(found[0], 'utf-8').trim().split('\n').pop()!); + expect(rec.skill).toBe('review'); + expect(rec.wtree).toBeUndefined(); + expect(rec.commit_full).toBeUndefined(); + } finally { + fs.rmSync(nonGit, { recursive: true, force: true }); + } + }); +}); + +describe('gstack-wtree', () => { + function withScratchRepo(fn: (repoDir: string, wtree: () => string) => void) { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-wtree-')); + try { + const git = (args: string) => + execSync(`git -c user.email=t@test -c user.name=t ${args}`, { cwd: repoDir, encoding: 'utf-8', timeout: 10000 }); + git('init -q -b main'); + fs.writeFileSync(path.join(repoDir, 'a.txt'), 'hello\n'); + fs.writeFileSync(path.join(repoDir, '.gitignore'), 'scratch.txt\n'); + git('add a.txt .gitignore'); + git('commit -q -m init'); + const wtree = () => execSync(`${BIN}/gstack-wtree`, { cwd: repoDir, encoding: 'utf-8', timeout: 10000 }).trim(); + fn(repoDir, wtree); + } finally { + fs.rmSync(repoDir, { recursive: true, force: true }); + } + } + + test('an UNTRACKED source file changes the fingerprint; a gitignored file does not', () => { + withScratchRepo((repoDir, wtree) => { + const clean = wtree(); + expect(clean).toMatch(/^[0-9a-f]{40}$/); + + // Gitignored scratch: invisible to the fingerprint (Conductor scratch stays out). + fs.writeFileSync(path.join(repoDir, 'scratch.txt'), 'noise\n'); + expect(wtree()).toBe(clean); + + // Untracked NEW source file: visible (new files can never be invisible to freshness). + fs.writeFileSync(path.join(repoDir, 'new-source.ts'), 'export {}\n'); + expect(wtree()).not.toBe(clean); + }); + }); + + test('committing identical content does NOT change the fingerprint', () => { + withScratchRepo((repoDir, wtree) => { + fs.writeFileSync(path.join(repoDir, 'a.txt'), 'edited\n'); + const dirtyFingerprint = wtree(); + execSync('git -c user.email=t@test -c user.name=t commit -q -am edit', { cwd: repoDir, timeout: 10000 }); + expect(wtree()).toBe(dirtyFingerprint); + }); + }); + + test('exits non-zero outside a git repo', () => { + const nonGit = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-wtree-nongit-')); + try { + expect(() => execSync(`${BIN}/gstack-wtree`, { cwd: nonGit, timeout: 10000, stdio: 'pipe' })).toThrow(); + } finally { + fs.rmSync(nonGit, { recursive: true, force: true }); + } + }); +}); + +describe('gstack-review-read', () => { + test('emits ---WTREE---, ---TREE--- and ---DIRTY--- sections', () => { + const out = execSync(`${BIN}/gstack-review-read`, { + cwd: ROOT, + env: { ...process.env, GSTACK_HOME: tmpDir }, + encoding: 'utf-8', + timeout: 10000, + }); + expect(out).toContain('---HEAD---'); + expect(out).toContain('---WTREE---'); + expect(out).toContain('---TREE---'); + expect(out).toContain('---DIRTY---'); + const wtreeLine = out.split('---WTREE---')[1].trim().split('\n')[0].trim(); + expect(wtreeLine).toMatch(/^([0-9a-f]{40}|unknown)$/); + const dirtyLine = out.split('---DIRTY---')[1].trim().split('\n')[0].trim(); + expect(['true', 'false']).toContain(dirtyLine); + }); });