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); + }); });