Skip to content

Commit 302ddad

Browse files
mchwangclaude
andauthored
F1e: planning API for lane G and feedback from review actions (#60)
* F1e: planning API for lane G and feedback from review actions Planning endpoints over E3's coordinator and the Store: import, suggestion start/read/cancel/apply, each through Store.userAction; E3's settlement writes use the shutdown capability and the coordinator closes at shutdown. Starting a suggestion is refused until a planning provider is injected. ReviewService.planContext builds the trusted plan context from the base tree. Review actions: change notes, segment accept and assign require an actionId and record their feedback event in the same transaction; a later choice links to the earlier event; choice sources are fixed-size fingerprints. The UI sends an actionId per action. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Start no planning agent for a suggestion request handled after shutdown began Ask and the runner stop admission in shutdown step 1, but the suggestion coordinator only closed in step 6. A request admitted before shutdown and handled during the drain allocated a pending request and started a planning invocation, which step 6 then cancelled. It now answers 503, records nothing, and the UI may resend it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Read the trusted plan context with main's hardened Git invocation main (#82, #83) replaced isolatedGitEnvironment with HARDENED_GIT_OPTIONS and hardenedGitEnvironment. planContext() now uses both. That also adds --no-replace-objects: before, a replace ref for the snapshot's base made ls-tree list the replacement's tree as the trusted base entries. A test replaces the base with the head commit and fails on the old invocation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent b1c6b1d commit 302ddad

7 files changed

Lines changed: 337 additions & 15 deletions

File tree

‎runner/review.ts‎

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@ import { isDeepStrictEqual } from 'node:util';
44
import { Store, type ReviewState, type SnippetReference } from './store.ts';
55
import type { PlanIdentity } from '../core/identity.ts';
66
import { readHistory } from '../git/history.ts';
7+
import { execFileSync } from 'node:child_process';
8+
import { HARDENED_GIT_OPTIONS, hardenedGitEnvironment } from '../scripts/git-environment.ts';
9+
import type { BaseEntry, PlanContext } from '../core/plan.ts';
710
import { linkHistory } from '../core/linking.ts';
811
import { applyChoices, approvalStates, approveItem, choiceKeys, fingerprint, reviewedSegment, stable } from '../core/approvals.ts';
912
import type { PlanItem } from '../core/plan.ts';
@@ -111,7 +114,27 @@ export class ReviewService {
111114
const token = createHash('sha256').update(JSON.stringify({ expected, saved, plan, segments })).digest('hex');
112115
return { repository: basename(repository), demo: this.config.demo ?? false, plan, snapshot, expected, token, items, segments, notes, approved: items.filter(item => item.state === 'approved').length };
113116
}
114-
act(input: unknown) {
117+
/** The trusted plan context for import and Apply: base entries from the snapshot's base tree, and the configured path identity. */
118+
planContext(): PlanContext {
119+
const { identity, repository, pathIdentity } = this.config, plan = this.store.getPlan(identity), snapshot = this.store.getSnapshot(identity);
120+
const pathKey = (path: string) => {
121+
if (!pathIdentity.caseSensitive && /[^\x20-\x7e]/.test(path)) throw new Error('Non-ASCII case-insensitive paths require a filesystem-specific identity adapter.');
122+
const normalized = pathIdentity.unicodeNormalization === 'NFC' ? path.normalize('NFC') : path;
123+
return pathIdentity.caseSensitive ? normalized : normalized.toLowerCase();
124+
};
125+
// Hardened like every repository Git call (#82, #83): no replace objects, hooks, network or inherited environment.
126+
const git = (args: string[], maxBuffer?: number) => execFileSync('git', [...HARDENED_GIT_OPTIONS, ...args], { cwd: repository, env: hardenedGitEnvironment(), encoding: 'utf8', maxBuffer, stdio: ['ignore', 'pipe', 'pipe'] });
127+
const listing = git(['ls-tree', '-rz', snapshot.base], 64 * 1024 * 1024);
128+
const baseEntries: BaseEntry[] = listing.split('\0').filter(Boolean).map(record => {
129+
const split = record.indexOf('\t'), [mode, , oid] = record.slice(0, split).split(' '), path = record.slice(split + 1);
130+
if (mode === '160000') return { path, kind: 'gitlink' };
131+
if (mode === '120000') return { path, kind: 'symlink', target: git(['cat-file', 'blob', oid!]) };
132+
return { path, kind: 'file' };
133+
});
134+
return { identity, issue: plan.issue, baseEntries, pathKey, allowedCommands: [] };
135+
}
136+
/** With an actionId (inside Store.userAction), feedback-producing actions record their event in the same transaction. */
137+
act(input: unknown, actionId?: string) {
115138
if (!input || typeof input !== 'object') throw new Error('Invalid review command.');
116139
const command = input as Record<string, unknown>;
117140
const view = this.load();
@@ -129,6 +152,11 @@ export class ReviewService {
129152
const item = command.action === 'assign' && typeof command.item === 'string' ? command.item : null;
130153
const storedKey = choiceKeys(view.segments, identity)[view.segments.indexOf(segment)]!;
131154
this.store.saveReview(identity, view.expected, [], [{ key: storedKey, action: command.action, item }]);
155+
// Choice keys embed segment content and are unbounded; the event's source is a stable fixed-size fingerprint of the key.
156+
const sourceRef = `choice:${createHash('sha256').update(storedKey).digest('hex')}`;
157+
if (actionId) this.store.recordFeedback(identity, actionId, command.action === 'assign'
158+
? { kind: 'segment-assign', item, sourceRef, supersedeLatest: true }
159+
: { kind: 'segment-accept', sourceRef, supersedeLatest: true });
132160
} else if (command.action === 'note' && typeof command.item === 'string' && typeof command.text === 'string' && (command.kind === 'question' || command.kind === 'change')) {
133161
let reference: SnippetReference | undefined;
134162
if (command.reference !== undefined) {
@@ -145,6 +173,7 @@ export class ReviewService {
145173
reference = { key: segment.key, path: segment.operation === '-' ? segment.oldPath ?? segment.path : segment.path, side: segment.operation === '+' ? 'new' : 'old', start, end, text, head: view.snapshot.head, base: view.snapshot.base };
146174
}
147175
createdNoteId = this.store.addReviewNote(identity, view.expected, command.item, command.kind, command.text, reference).id;
176+
if (actionId && command.kind === 'change') this.store.recordFeedback(identity, actionId, { kind: 'change-request', item: command.item, text: command.text.trim(), sourceRef: createdNoteId });
148177
} else throw new Error('Unknown review command.');
149178
return { ...this.load(), createdNoteId };
150179
}

‎runner/store.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -863,7 +863,7 @@ export class Store {
863863
return { response: outcome.value as T, replayed: true };
864864
}
865865
/** Append one feedback event. Call inside userAction so the event and its action share one transaction. */
866-
recordFeedback(identity: PlanIdentity, actionId: string, event: { kind: Exclude<FeedbackKind, 'task-closed'>; item?: string | null; text?: string | null; sourceRef: string; supersedes?: string | null }): FeedbackEvent {
866+
recordFeedback(identity: PlanIdentity, actionId: string, event: { kind: Exclude<FeedbackKind, 'task-closed'>; item?: string | null; text?: string | null; sourceRef: string; supersedes?: string | null; supersedeLatest?: boolean }): FeedbackEvent {
867867
assertUuidV4(actionId, 'Action ID');
868868
if (this.#depth === 0 || this.#action?.key !== identityKey(identity) || this.#action.actionId !== actionId)
869869
throw new Error('Feedback events are written inside their user action, with its action ID.');
@@ -873,6 +873,9 @@ export class Store {
873873
if (event.text != null && (typeof event.text !== 'string' || event.text.length > 4000)) throw new GuardRefusal('Feedback text is limited to 4000 characters.');
874874
if (typeof event.sourceRef !== 'string' || !event.sourceRef || event.sourceRef.length > 200) throw new GuardRefusal('Invalid feedback source.');
875875
const key = identityKey(identity), plan = this.#current(key);
876+
// A changed segment choice links to the latest earlier event for the same choice key.
877+
if (event.supersedeLatest && event.supersedes == null)
878+
event = { ...event, supersedes: (this.#get("SELECT id FROM feedback_events WHERE plan_key=? AND source_ref=? AND kind IN ('segment-accept','segment-assign') ORDER BY rowid DESC LIMIT 1", key, event.sourceRef)?.id as string | undefined) ?? null };
876879
if (event.supersedes != null && !this.#get('SELECT 1 FROM feedback_events WHERE plan_key=? AND id=? AND source_ref=?', key, event.supersedes, event.sourceRef))
877880
throw new GuardRefusal('A superseded event must belong to the same source.');
878881
const id = randomUUID(), createdAt = new Date().toISOString();

‎test/browser/review.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
1+
import { randomUUID } from 'node:crypto';
12
import { test, expect } from '@playwright/test';
23
import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs';
34
import { tmpdir } from 'node:os';
45
import { join } from 'node:path';
56
import { request as httpRequest, type IncomingMessage } from 'node:http';
6-
import { randomUUID } from 'node:crypto';
77
import { createDemo } from '../../scripts/demo.ts';
88
import { choiceKeys } from '../../core/approvals.ts';
99
import { ReviewService } from '../../runner/review.ts';
@@ -249,7 +249,7 @@ test('shows an honest history error and keeps markup in notes as text',async({pa
249249
test('can assign a large foreign change without sending its content back in the command',async({request})=>{
250250
const repository=app.service.config.repository;writeFileSync(join(repository,'debug.log'),'x'.repeat(20000)+'\n');fixtureGit(repository,'commit','-am','Large foreign change');
251251
const base=app.url.split('#')[0]!,headers={'x-codeboost-token':app.token};const view=await(await request.get(base+'api/review',{headers})).json();const segment=view.segments.find((s:{content:string;row:string})=>s.row==='Unplanned'&&s.content.length>19000);
252-
const response=await request.post(base+'api/action',{headers:{...headers,'Content-Type':'application/json'},data:{action:'assign',item:'P1',key:segment.key,token:view.token}});
252+
const response=await request.post(base+'api/action',{headers:{...headers,'Content-Type':'application/json'},data:{action:'assign',item:'P1',key:segment.key,token:view.token,actionId:randomUUID()}});
253253
expect(response.status()).toBe(200);
254254
});
255255
test('shows bounded raster previews and byte sizes for file-change cards',async({page})=>{

0 commit comments

Comments
 (0)