mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-16 01:45:29 +02:00
v1.87.3.0 fix: bind review evidence to the reviewed tree (#2875)
* fix(review): bind evidence to completed unchanged review passes * fix(review): keep unresolved Codex findings unverified * docs: update review evidence documentation for v1.87.3.0 * test(cso): let Windows integration finish within subprocess budgets * docs: update project documentation for v1.87.3.0 --------- Co-authored-by: garrytan <19957+garrytan@users.noreply.github.com>
This commit is contained in:
Garry Tan
@@ -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/<base>`'));
|
||||
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 "<base>...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" "<run-dir>/body.md"');
|
||||
|
||||
+7
-7
@@ -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":
|
||||
|
||||
|
||||
+21
-13
@@ -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/<base>` 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/<base>` 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 <base> 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 <fixed-files> && 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.
|
||||
|
||||
---
|
||||
|
||||
+21
-13
@@ -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/<base>` 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/<base>` 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 <base> 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 <fixed-files> && 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.
|
||||
|
||||
---
|
||||
|
||||
@@ -616,7 +616,7 @@ export const E2E_TOUCHFILES: Record<string, string[]> = {
|
||||
'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',
|
||||
|
||||
+11
-6
@@ -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');
|
||||
|
||||
@@ -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<string, any> = {}) {
|
||||
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');
|
||||
});
|
||||
});
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user