Skip to content

Commit f2f1551

Browse files
mchwangclaude
andauthored
Open a stale item's comparison when the answer that makes it stale arrives after it was selected (#78)
* Open a stale item's comparison when the answer that makes it stale arrives after it was selected The review page chose "Since approval" or "Full change" once, in select(), from the data present at the click. When the reviewer selected P1 while an Assign request was still in flight, P1 was still approved in that data, so the page chose Full change. The Assign answer then made P1 stale and re-rendered its stale reasons, but it kept the earlier choice, so the "At approval" comparison never appeared. This is the intermittent failure of review.spec.ts "reviews real changes, persists approval and conversation, and assigns foreign code" (CI run 36498484408, attempt 1). The page now stores only an explicit toggle, keyed to the item and to the stale state it was made on (reasons, current segments and plan item). With no matching choice, a stale item always opens in Since approval, whatever answer made it stale. An explicit choice survives answers that leave that state unchanged, and ends when the item's code or plan changes or the reviewer selects another item. The new browser test holds the Assign answer with a promise, selects P1 while it is still approved, releases the answer, and asserts the comparison and the stored stale state. It fails on the previous app.js at the race step. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Key the explicit view choice to every segment field the approval fingerprint covers The stale-state key used only path, operation and content, but core/approvals.ts also counts oldPath, kind, context and owners as a change. Moving unchanged text to another function changes only context and makes the item stale, yet the key stayed the same, so an earlier "Full change" choice hid the comparison for the new state. The key now uses the same fields. The new browser test changes context, owners and oldPath one at a time in the review answer and expects the comparison each time. It fails on the previous key, and dropping any one of the three fields from the key fails it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Name each stale state on the server instead of mirroring staleness in the page The page kept an explicit view choice by building its own key from the item's segments, reasons and plan item. That copy of the server's staleness rules missed inputs: the approval the comparison is drawn against, ambiguous segments the item owns, its dependencies' stale states, and the context of a replaced head after a queue attempt. In each case an old choice could apply to a stale state the reviewer had not seen. ReviewService.load() now returns staleKey for each stale item: a hash of the saved approval, the current approval fingerprint (fingerprint() is now exported from core/approvals.ts), the ambiguous segments it owns, its dependencies' stale keys, and the queue-replacement context when it applies. The page compares only this key. Tests: - test/review.test.ts: three unit tests cover a dependency change with the same reasons, re-approval while still stale, re-approval followed by an ambiguous change with identical own segments and reasons, an ambiguous change on a still-stale item, and a second replaced head. Removing any one of the five key inputs fails a test. - The browser test that edited segment fields is replaced by one that changes only staleKey in the answer; it fails if the page ignores the key. - The "explicit choice survives Refresh" step now waits until the Refresh answer is rendered, so it fails if Refresh resets the choice. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Key a stale item's ambiguous segments by every field an approval covers The stale key identified the ambiguous segments an item owns by their choice keys, which leave out context and owners. An ambiguous segment that moved to another function or gained an owner therefore kept the same stale key, so an earlier "Full change" choice survived the new stale state. core/approvals.ts now has reviewedSegment(), the segment fields an approval covers, used by fingerprint() and by the ambiguous part of the stale key. The ambiguous stale-key test now has P3 edit the shared line and restore its text, so the segment keeps its choice key but gains an owner; the key must change. Using only choice keys fails that step. The two longer stale-key tests get the 30 s limit other multi-commit tests in this file use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Hash each ambiguous segment once per load in the stale key The stale key serialized an ambiguous segment's full reviewed fields once for every stale item that owns it. A large segment shared by many items made each synchronous load() repeat that work per owner. load() now hashes reviewedSegment() once per segment, memoized by segment key, and each owner's stale key includes that digest. The fields stay the ones an approval covers, so a change in context or owners still gives a new key. The ambiguous stale-key test now approves P1, P2 and P3 first, so the shared line belongs to three stale items, and counts reviewedSegment() calls from load() through a passthrough mock: one per segment. The previous per-owner form fails that count, and leaving owners out of the digest fails the owners step. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Run this PR's test Git commands with the hardened fixture invocation The Git commands added by this PR's tests inherited the caller's environment and user config and only disabled hooks. An inherited GIT_DIR or GIT_WORK_TREE could redirect their commits outside the fixture, and user config could change their behaviour, against the AGENTS.md rule on subprocess environments and hardened Git. test/fixtures/git.ts adds fixtureGit(): an allowlisted environment (PATH and the Git hardening variables only), no user or system config, no hooks, no replace objects, no lazy fetch and no network protocols, as git/history.ts does. The new stale-key unit tests and the in-flight browser test use it. test/fixture-git.test.ts sets GIT_DIR, GIT_WORK_TREE, GIT_CONFIG_GLOBAL and GIT_AUTHOR_NAME to point elsewhere and checks that a commit stays in the fixture with the fixture's identity. Passing process.env through fails it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Make the fixture Git test fail if user or system config is read The inherited global config in the test set only user.name and core.hooksPath. The fixture's local user.name and the -c hooks flag override both, so the test passed even if fixtureGit read user or system config. The inherited global and system config now define an alias that exists nowhere else, and the test expects running it to fail. Passing GIT_CONFIG_GLOBAL through, or reading GIT_CONFIG_SYSTEM, now fails the test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Canonicalize ambiguous segments like the approval fingerprint in the stale key The ambiguous-segment digest hashed plain JSON, while the approval fingerprint and choice keys use stable(), which normalizes CRLF to LF and sorts object keys. A commit that changed only the line endings of a still-ambiguous segment therefore changed staleKey and dropped the reviewer's explicit view choice, although the stale state was the same. stable() is now exported from core/approvals.ts, and the digest hashes stable(reviewedSegment(segment)). The ambiguous stale-key test has P2 re-commit the shared line with CRLF only and expects the same key, with the segment's content containing \r and its owners unchanged. It fails with the plain-JSON digest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent bfc6b18 commit f2f1551

7 files changed

Lines changed: 189 additions & 17 deletions

File tree

‎core/approvals.ts‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import type { Plan, PlanItem } from './plan.ts';
33
import type { Segment } from './linking.ts';
44

55
/** Stable representation ignores object-key order and normalizes CRLF, not whitespace. */
6-
function stable(value: unknown): string {
6+
export function stable(value: unknown): string {
77
if (typeof value === 'string') return JSON.stringify(value.replace(/\r\n/g, '\n'));
88
if (Array.isArray(value)) return `[${value.map(stable).join(',')}]`;
99
if (value !== null && typeof value === 'object') return `{${Object.entries(value).sort(([a], [b]) => a.localeCompare(b)).map(([key, val]) => `${JSON.stringify(key)}:${stable(val)}`).join(',')}}`;
@@ -33,11 +33,13 @@ export function applyChoices(plan: Plan, segments: readonly Segment[], choices:
3333
});
3434
}
3535
export interface Approval { item: string; fingerprint: string }
36-
function fingerprint(item: PlanItem, segments: readonly Segment[], identity: PlanIdentity): string {
37-
return stable({ identity: identityKey(identity), item, segments: segments.filter(s => s.row === item.id).map(s => ({
38-
path: s.path, oldPath: s.oldPath, kind: s.kind, operation: s.operation,
39-
content: s.content, context: s.context, owners: [...s.owners].sort(),
40-
})) });
36+
/** Every segment field an approval covers. */
37+
export const reviewedSegment = (s: Segment) => ({
38+
path: s.path, oldPath: s.oldPath, kind: s.kind, operation: s.operation,
39+
content: s.content, context: s.context, owners: [...s.owners].sort(),
40+
});
41+
export function fingerprint(item: PlanItem, segments: readonly Segment[], identity: PlanIdentity): string {
42+
return stable({ identity: identityKey(identity), item, segments: segments.filter(s => s.row === item.id).map(reviewedSegment) });
4143
}
4244
export function approveItem(plan: Plan, segments: readonly Segment[], itemId: string, identity: PlanIdentity, confirmNoChange = false): Approval {
4345
const item = plan.items.find(item => item.id === itemId);

‎runner/review.ts‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@ import { Store, type ReviewState, type SnippetReference } from './store.ts';
55
import type { PlanIdentity } from '../core/identity.ts';
66
import { readHistory } from '../git/history.ts';
77
import { linkHistory } from '../core/linking.ts';
8-
import { applyChoices, approvalStates, approveItem, choiceKeys } from '../core/approvals.ts';
8+
import { applyChoices, approvalStates, approveItem, choiceKeys, fingerprint, reviewedSegment, stable } from '../core/approvals.ts';
9+
import type { PlanItem } from '../core/plan.ts';
910
import type { GhMergeConfig } from '../github/merge.ts';
1011

1112
export interface ReviewConfig { database: string; repository: string; identity: PlanIdentity; pathIdentity: { caseSensitive: boolean; unicodeNormalization: 'none' | 'NFC' }; demo?: boolean; github?: GhMergeConfig }
@@ -63,6 +64,29 @@ export class ReviewService {
6364
const contextIds = new Map(plan.items.map(item => [item.id,createHash('sha256').update(JSON.stringify(segments.filter(segment=>segment.row===item.id).map(segment=>segment.key))).digest('hex')]));
6465
const notes = this.store.getReviewNotes(identity).map(note => {const contextId=contextIds.get(note.item)!;return { ...note, contextId, answerOutdated: note.snapshotId!==snapshot.id || note.revision!==plan.revision || (!!note.answer?.contextId && note.answer.contextId!==contextId), outdated: !!note.reference && (note.reference.head !== history.head || note.reference.base !== history.base || !segments.some(segment => segment.key === note.reference!.key && segment.row === note.item)) };});
6566
if (this.store.reviewVersion(identity) !== reviewVersion || this.store.getPlan(identity).revision !== plan.revision || this.store.getSnapshot(identity).id !== snapshot.id) throw new Error('Stale review state. Reload before writing.');
67+
// Names each stale item's stale state from every input that decides it, so the page keeps a reviewer's view choice only while that state is unchanged.
68+
const staleKeys = new Map<string, string | null>();
69+
// Every field an approval covers, in the approval fingerprint's canonical form, hashed once per segment so a segment shared by many stale items is not serialized once per owner.
70+
const reviewedDigests = new Map<string, string>();
71+
const reviewedDigest = (segment: typeof segments[number]) => {
72+
let digest = reviewedDigests.get(segment.key);
73+
if (digest === undefined) reviewedDigests.set(segment.key, digest = createHash('sha256').update(stable(reviewedSegment(segment))).digest('hex'));
74+
return digest;
75+
};
76+
const staleKey = (item: PlanItem): string | null => {
77+
if (staleKeys.has(item.id)) return staleKeys.get(item.id) ?? null;
78+
staleKeys.set(item.id, null);
79+
if (states[item.id] !== 'stale') return null;
80+
const key = createHash('sha256').update(JSON.stringify({
81+
approval: saved.approvals.find(value => value.item === item.id) ?? null,
82+
current: fingerprint(item, segments, identity),
83+
ambiguous: segments.filter(segment => segment.row === 'Ambiguous' && segment.owners.includes(item.id)).map(reviewedDigest),
84+
dependencies: item.depends_on.map(id => { const dependency = plan.items.find(value => value.id === id); return dependency ? staleKey(dependency) : null; }),
85+
replacement: replacementReview ? { snapshotId: snapshot.id, revision: plan.revision, requiresFreshReview: mergeAttempt.requiresFreshReview, reviewVersion: mergeAttempt.reviewVersion } : null,
86+
})).digest('hex');
87+
staleKeys.set(item.id, key);
88+
return key;
89+
};
6690
const items = plan.items.map(item => {
6791
const owned = segments.filter(segment => segment.row === item.id);
6892
const ambiguous = segments.filter(segment => segment.row === 'Ambiguous' && segment.owners.includes(item.id)).length;
@@ -80,7 +104,7 @@ export class ReviewService {
80104
for (const dep of item.depends_on) if (states[dep] === 'stale') reasons.push(`Depends on ${dep}, which changed`);
81105
if (!reasons.length) reasons.push('Code or plan definition changed');
82106
}
83-
return { ...item, state: states[item.id], count: owned.length, ambiguousCount: ambiguous, reasons, before,
107+
return { ...item, state: states[item.id], count: owned.length, ambiguousCount: ambiguous, reasons, before, staleKey: staleKey(item),
84108
checks: { attributed: ambiguous ? `! ${ambiguous} ambiguous` : owned.length ? '✓ Attributed' : '– No changes', scope: outside.length ? `✕ ${new Set(outside).size} out of scope` : owned.length ? '✓ In scope' : '– No changes', tests: item.acceptance.some(check => check.type === 'cmd') ? '– Not run' : '– No tests defined', ai: '– Not run' }, outside: [...new Set(outside)],
85109
};
86110
});

‎test/browser/review.spec.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { choiceKeys } from '../../core/approvals.ts';
1010
import { ReviewService } from '../../runner/review.ts';
1111
import { startServer } from '../../web/server.ts';
1212
import type { MergeGateway, MergeQueueGateway } from '../../github/merge.ts';
13+
import { fixtureGit } from '../fixtures/git.ts';
1314
let root: string, app: Awaited<ReturnType<typeof startServer>>;
1415
// Resolves once the server has parsed the headers of the request that carries `marker` and is reading its body, so that partial request is provably admitted before shutdown starts.
1516
// Matching the marker matters: any other client on the port (a polling tab, a reused ephemeral port) also emits 'request' and would start shutdown too early.
@@ -98,6 +99,43 @@ test('reviews real changes, persists approval and conversation, and assigns fore
9899
await page.getByRole('button',{name:/P1 Bound exponential retries/}).click();await expect(page.getByText('! Stale:',{exact:false})).toBeVisible();await expect(page.getByRole('heading',{name:'At approval'})).toBeVisible();
99100
await page.screenshot({path:'test-results/review-desktop.png',fullPage:true});expect(errors).toEqual([]);
100101
});
102+
test('opens an item selected while an assignment is in flight in the view its answered state calls for',async({page})=>{
103+
await page.goto(app.url);await page.getByRole('button',{name:'Approve P1',exact:true}).click();await expect(page.getByText('1 of 3 approved')).toBeVisible();
104+
let release!:()=>void,arrived!:()=>void;const held=new Promise<void>(resolve=>{release=resolve;}),assignArrived=new Promise<void>(resolve=>{arrived=resolve;});
105+
await page.route('**/api/action',async route=>{if(route.request().postDataJSON().action==='assign'){arrived();await held;}await route.continue();});
106+
await page.getByRole('button',{name:/Unplanned changes/}).click();await page.getByLabel('Assign change 1 to').selectOption('P1');await page.getByRole('button',{name:'Assign',exact:true}).first().click();await assignArrived;
107+
// P1 is still approved in the page when it is selected; the held answer makes it stale.
108+
await page.getByRole('button',{name:/P1 Bound exponential retries/}).click();await expect(page.getByRole('heading',{name:'Bound exponential retries'})).toBeVisible();await expect(page.getByText('! Stale:',{exact:false})).toHaveCount(0);
109+
release();await expect(page.getByText('! Stale:',{exact:false})).toBeVisible();
110+
await expect(page.getByRole('button',{name:'Since approval',exact:true})).toHaveAttribute('aria-pressed','true');await expect(page.getByRole('heading',{name:'At approval'})).toBeVisible();
111+
expect(app.service.load().items.find(item=>item.id==='P1')!.state).toBe('stale');
112+
// An explicit choice survives the next answer.
113+
await page.getByRole('button',{name:'Full change',exact:true}).click();await expect(page.getByRole('heading',{name:'At approval'})).toHaveCount(0);
114+
// Every check below already holds before the Refresh answer, so wait until that answer is rendered.
115+
const answered=page.waitForResponse('**/api/review');await page.getByRole('button',{name:'Refresh',exact:true}).click();await answered;await expect(page.locator('#banner')).not.toContainText('Linking changes');
116+
await expect(page.getByText('! Stale:',{exact:false})).toBeVisible();await expect(page.getByRole('button',{name:'Full change',exact:true})).toHaveAttribute('aria-pressed','true');await expect(page.getByRole('heading',{name:'At approval'})).toHaveCount(0);
117+
// A code change on the still-stale item is a new state, so the choice made on the old one no longer holds.
118+
const repository=app.service.config.repository;writeFileSync(join(repository,'retry.ts'),'export function delay(attempt: number) {\n return 42;\n}\n');fixtureGit(repository,'commit','-am','External change');
119+
await page.getByRole('button',{name:'Refresh',exact:true}).click();await expect(page.getByText('! Stale:',{exact:false}).first()).toBeVisible();await expect(page.getByRole('button',{name:'Since approval',exact:true})).toHaveAttribute('aria-pressed','true');await expect(page.getByRole('heading',{name:'At approval'})).toBeVisible();
120+
// After approval, a new stale state opens the comparison again.
121+
await page.getByRole('button',{name:'Full change',exact:true}).click();await expect(page.getByRole('heading',{name:'At approval'})).toHaveCount(0);
122+
await page.getByRole('button',{name:'Approve P1',exact:true}).click();await expect(page.getByText('! Stale:',{exact:false})).toHaveCount(0);
123+
let view=app.service.load();view=app.service.act({action:'assign',key:view.segments.find(segment=>segment.row==='Unplanned'&&segment.path==='retry.ts')!.key,item:'P1',token:view.token});expect(view.items.find(item=>item.id==='P1')!.state).toBe('stale');
124+
await page.getByRole('button',{name:'Refresh',exact:true}).click();await expect(page.getByText('! Stale:',{exact:false}).first()).toBeVisible();await expect(page.getByRole('button',{name:'Since approval',exact:true})).toHaveAttribute('aria-pressed','true');await expect(page.getByRole('heading',{name:'At approval'})).toBeVisible();
125+
});
126+
test('keeps an explicit view choice only while the server names the same stale state',async({page})=>{
127+
let view=app.service.load();view=app.service.act({action:'approve',item:'P1',confirmNoChange:false,token:view.token});
128+
view=app.service.act({action:'assign',key:view.segments.find(segment=>segment.row==='Unplanned')!.key,item:'P1',token:view.token});expect(view.items.find(item=>item.id==='P1')!.state).toBe('stale');
129+
// The answer is the server's own, except that P1's staleKey can be replaced; nothing else about P1 changes.
130+
let staleKey:string|null=null;
131+
await page.route('**/api/review',async route=>{const response=await route.fetch(),body=await response.json();if(staleKey)body.items.find((item:{id:string})=>item.id==='P1').staleKey=staleKey;await route.fulfill({response,json:body});});
132+
const refresh=async()=>{const answered=page.waitForResponse('**/api/review');await page.getByRole('button',{name:'Refresh',exact:true}).click();await answered;await expect(page.locator('#banner')).not.toContainText('Linking changes');};
133+
await page.goto(app.url);await page.getByRole('button',{name:/P1 Bound exponential retries/}).click();await expect(page.getByRole('heading',{name:'At approval'})).toBeVisible();
134+
await page.getByRole('button',{name:'Full change',exact:true}).click();await expect(page.getByRole('heading',{name:'At approval'})).toHaveCount(0);
135+
await refresh();await expect(page.getByRole('button',{name:'Full change',exact:true})).toHaveAttribute('aria-pressed','true');await expect(page.getByRole('heading',{name:'At approval'})).toHaveCount(0);
136+
staleKey='0'.repeat(64);await refresh();
137+
await expect(page.getByRole('button',{name:'Since approval',exact:true})).toHaveAttribute('aria-pressed','true');await expect(page.getByRole('heading',{name:'At approval'})).toBeVisible();
138+
});
101139
test('shows file metadata and no-change confirmation, supports narrow desktop and keyboard',async({page})=>{
102140
await page.goto(app.url);await page.getByRole('button',{name:/P2 Document retry behavior/}).click();await expect(page.getByText('File mode changed',{exact:false})).toBeVisible();await expect(page.getByText('✕ Out of scope',{exact:true})).toBeVisible();
103141
await page.getByRole('button',{name:/P3 Confirm API compatibility/}).click();await page.getByRole('button',{name:'Confirm no change needed',exact:true}).click();await expect(page.getByText('1 of 3 approved')).toBeVisible();

‎test/fixture-git.test.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
import { afterEach, expect, it, vi } from 'vitest';
2+
import { mkdtempSync, rmSync, writeFileSync } from 'node:fs';
3+
import { tmpdir } from 'node:os';
4+
import { join } from 'node:path';
5+
import { fixtureGit } from './fixtures/git.ts';
6+
7+
const roots: string[] = [];
8+
afterEach(() => { vi.unstubAllEnvs(); roots.splice(0).forEach(root => rmSync(root, { recursive: true, force: true })); });
9+
function repository() {
10+
const root = mkdtempSync(join(tmpdir(), 'codeboost-fixture-git-')); roots.push(root);
11+
const inside = join(root, 'inside'), outside = join(root, 'outside');
12+
for (const path of [inside, outside]) { fixtureGit(root, 'init', '-q', '-b', 'main', path); fixtureGit(path, 'config', 'user.name', 'Fixture'); fixtureGit(path, 'config', 'user.email', 'fixture@example.invalid'); }
13+
return { inside, outside };
14+
}
15+
it('ignores inherited Git variables and user or system config, so a commit stays in the fixture', () => {
16+
const { inside, outside } = repository();
17+
// An alias is set nowhere else, so running it succeeds only if the inherited config file was read.
18+
const inherited = join(inside, '..', 'inherited.gitconfig'); writeFileSync(inherited, '[alias]\n\tinherited = rev-parse --git-dir\n');
19+
vi.stubEnv('GIT_DIR', join(outside, '.git')); vi.stubEnv('GIT_WORK_TREE', outside); vi.stubEnv('GIT_AUTHOR_NAME', 'Inherited');
20+
vi.stubEnv('GIT_CONFIG_GLOBAL', inherited); vi.stubEnv('GIT_CONFIG_SYSTEM', inherited); vi.stubEnv('GIT_CONFIG_NOSYSTEM', '0');
21+
writeFileSync(join(inside, 'file.txt'), 'x\n'); fixtureGit(inside, 'add', 'file.txt'); fixtureGit(inside, 'commit', '-q', '-m', 'Inside');
22+
expect(fixtureGit(inside, 'log', '-1', '--format=%an %s')).toBe('Fixture Inside');
23+
expect(() => fixtureGit(inside, 'inherited')).toThrow(/not a git command/);
24+
vi.unstubAllEnvs();
25+
expect(() => fixtureGit(outside, 'rev-parse', '--verify', 'HEAD')).toThrow();
26+
});

‎test/fixtures/git.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
import { execFileSync } from 'node:child_process';
2+
3+
/**
4+
* Runs Git in a test fixture repository with the repository's hardened invocation: an allowlisted environment,
5+
* no user or system config, no hooks, no lazy fetch and no network protocols. Commit identity comes from the
6+
* fixture's own repository config.
7+
*/
8+
export function fixtureGit(repository: string, ...args: string[]): string {
9+
return execFileSync('git', ['--no-pager', '--no-replace-objects', '-c', 'core.hooksPath=/dev/null', '-c', 'protocol.allow=never', ...args], {
10+
cwd: repository, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'],
11+
env: { PATH: process.env.PATH ?? '', GIT_CONFIG_NOSYSTEM: '1', GIT_CONFIG_GLOBAL: '/dev/null', GIT_TERMINAL_PROMPT: '0', GIT_NO_LAZY_FETCH: '1' },
12+
}).trim();
13+
}

0 commit comments

Comments
 (0)