Skip to content

Commit 8b5b48b

Browse files
committed
Track answer assignment context
1 parent 39ab862 commit 8b5b48b

5 files changed

Lines changed: 19 additions & 6 deletions

File tree

‎docs/implementation/read-only-review.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,10 @@ PR #11 round 8 returned no inline findings and identified one related summary ga
103103

104104
PR #11 round 9 returned no inline findings and identified shutdown ordering in its summary. Server shutdown now stops accepting connections and drains in-flight HTTP requests before closing the question manager and database. A partial-body request regression proves a question already admitted during shutdown starts its agent and is persisted as interrupted rather than left unanswered.
105105

106-
Current PR validation: 187 unit/integration tests, 32 browser tests, typecheck, and diff checks. Earlier counts above identify the stage when each behavior was added.
106+
Current PR validation: 188 unit/integration tests, 32 browser tests, typecheck, and diff checks. Earlier counts above identify the stage when each behavior was added.
107107

108108
The documentation-only review after adding `AGENTS.md` exposed one more concrete lifecycle race: an expired persisted attempt could show Retry while its cancelled provider was still settling. Question polling now exposes whether the local invocation remains active, displays Finishing cancellation, and keeps polling without exposing Retry until settlement. A browser regression advances the persisted clock while leaving the invocation unresolved, then confirms Retry appears only after settlement.
109109

110110
The follow-up review exposed the same active-job marker missing from the initial review response. Reloading during provider settlement could therefore expose Retry and stop polling. Initial loads and action responses now include the marker used by question polling; the browser regression reloads during settlement and confirms Retry remains hidden until the invocation finishes.
111+
112+
The next review made assignment drift concrete for item-level answers without snippet references. Each answer now records a hash of the changed segments supplied for its item. Moving code into or out of that item preserves the answer but labels it as earlier review context; a regression assigns new code without changing the snapshot or plan revision and checks the historical marker.

‎runner/questions.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ export class Questions {
3232
const provider=this.service.store.questionProvider();
3333
const agent=this.agent ?? (provider ? cliQuestionAgent(provider) : undefined);
3434
const attempt=randomUUID(), controller=new AbortController();
35-
this.service.store.beginAnswer(this.service.config.identity,id,attempt,provider??undefined);
35+
this.service.store.beginAnswer(this.service.config.identity,id,attempt,provider??undefined,note.contextId);
3636
if(this.running.size>=2){this.service.store.finishAnswer(this.service.config.identity,id,attempt,{status:'failed',error:'Two questions are already running. Retry when one finishes.'});return;}
3737
const timeout=setTimeout(()=>controller.abort(new Error('Agent timed out. Try again.')),120_000);
3838
let invocation: Promise<string> | undefined;

‎runner/review.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,8 @@ export class ReviewService {
5252
if (states[item.id] === 'approved' && (segments.some(segment => segment.row === 'Ambiguous' && segment.owners.includes(item.id)) || item.depends_on.some(id => states[id] === 'stale'))) states[item.id] = 'stale';
5353
}
5454
const expected: ReviewState = { revision: plan.revision, snapshotId: snapshot.id, reviewVersion };
55-
const notes = this.store.getReviewNotes(identity).map(note => ({ ...note, answerOutdated: note.snapshotId!==snapshot.id || note.revision!==plan.revision, 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)) }));
55+
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')]));
56+
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)) };});
5657
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.');
5758
const items = plan.items.map(item => {
5859
const owned = segments.filter(segment => segment.row === item.id);

‎runner/store.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ export function requireSupportedNode(version = process.versions.node): void {
1313
export interface Snapshot { id: string; base: string; head: string }
1414
export interface ReviewState { revision: number; snapshotId: string; reviewVersion?: number }
1515
export interface SnippetReference { key: string; path: string; side: 'old' | 'new'; start: number; end: number; text: string; head: string; base: string }
16-
export interface QuestionAnswer { provider?: 'claude' | 'codex'; attempt: string; status: 'pending' | 'complete' | 'failed'; expiresAt: number; text?: string; error?: string }
16+
export interface QuestionAnswer { provider?: 'claude' | 'codex'; attempt: string; contextId?: string; status: 'pending' | 'complete' | 'failed'; expiresAt: number; text?: string; error?: string }
1717
export interface ReviewNote { id: string; item: string; kind: 'question' | 'change'; text: string; reference?: SnippetReference; answer?: QuestionAnswer; createdAt: string; revision: number; snapshotId: string }
1818
export interface LedgerEntry { sha: string; owner: string | null; origin: 'owned' | 'foreign'; sourceSha: string | null }
1919
export interface Checkpoint {
@@ -268,15 +268,15 @@ export class Store {
268268
return note;
269269
});
270270
}
271-
beginAnswer(identity: PlanIdentity, id: string, attempt: string, provider?: 'claude' | 'codex'): void {
271+
beginAnswer(identity: PlanIdentity, id: string, attempt: string, provider?: 'claude' | 'codex', contextId?: string): void {
272272
const key=identityKey(identity);
273273
this.#transaction(()=>{
274274
const row=this.#get('SELECT data FROM review_notes WHERE key=? AND id=?',key,id);
275275
if(!row) throw new Error('Question not found.');
276276
const note=decode<ReviewNote>(row.data);
277277
if(note.kind!=='question' || note.answer?.status==='complete') throw new Error('Question already answered.');
278278
if(note.answer?.status==='pending' && note.answer.expiresAt>Date.now()) throw new Error('Agent is already answering this question.');
279-
note.answer={attempt,status:'pending',expiresAt:Date.now()+125000,...(provider?{provider}:{})};
279+
note.answer={attempt,status:'pending',expiresAt:Date.now()+125000,...(provider?{provider}:{}),...(contextId?{contextId}:{})};
280280
this.#run('UPDATE review_notes SET data=? WHERE key=? AND id=?',encode(note),key,id);
281281
});
282282
}

‎test/questions.test.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,3 +95,13 @@ it('rejects questions whose snippet was reassigned without changing the snapshot
9595
const agent=vi.fn(async()=>'Should not run');const manager=new Questions(service,agent);managers.push(manager);
9696
expect(()=>manager.start(note.id,moved)).toThrow(/older review|outdated/);expect(agent).not.toHaveBeenCalled();
9797
});
98+
it('marks an item-level answer historical when assigned code changes',async()=>{
99+
const service=fixture(),initial=service.load(),foreign=initial.segments.find(s=>s.row==='Unplanned')!;
100+
const asked=service.act({action:'note',item:'P1',kind:'question',text:'Is this item complete?',token:initial.token});
101+
const manager=new Questions(service,async()=>'It is complete for the supplied changes.');managers.push(manager);
102+
manager.start(asked.createdNoteId!,asked);await vi.waitFor(()=>expect(service.store.getReviewNotes(service.config.identity)[0]?.answer?.status).toBe('complete'));
103+
const index=initial.segments.findIndex(segment=>segment.key===foreign.key);
104+
service.store.saveReview(service.config.identity,asked.expected,[],[{action:'assign',item:'P1',key:choiceKeys(initial.segments,service.config.identity)[index]!}]);
105+
const changed=service.load(),note=changed.notes.find(note=>note.id===asked.createdNoteId)!;
106+
expect(changed.snapshot.id).toBe(asked.snapshot.id);expect(note.answer?.text).toContain('complete');expect(note.answerOutdated).toBe(true);
107+
});

0 commit comments

Comments
 (0)