Skip to content

Commit 9bac08d

Browse files
authored
fix(investigation): reconcile Codex terminal output (#179)
* fix(investigation): reconcile Codex terminal output * chore(release): rebuild ReviewRouter action
1 parent a29e058 commit 9bac08d

6 files changed

Lines changed: 181 additions & 52 deletions

File tree

__tests__/unit/review-investigation/codex-app-server-protocol.test.ts

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1238,7 +1238,22 @@ describe('CodexAppServerProtocolClient', () => {
12381238
});
12391239
});
12401240

1241-
it('rejects an unseen allowed terminal snapshot item as protocol drift', async () => {
1241+
it('reconciles an unseen non-effectful item from the terminal snapshot', async () => {
1242+
const fixture = await activeTurn();
1243+
const item = agentMessageItem(
1244+
'terminal-only',
1245+
'final_answer',
1246+
'{"ok":true}'
1247+
);
1248+
completeUsage(fixture.client);
1249+
completeTurn(fixture.client, [item]);
1250+
1251+
await expect(fixture.result).resolves.toMatchObject({
1252+
finalMessage: '{"ok":true}',
1253+
});
1254+
});
1255+
1256+
it('rejects an unseen MCP item from the terminal snapshot', async () => {
12421257
const fixture = await activeTurn();
12431258
fixture.client.receive(
12441259
notification('turn/completed', {
@@ -1247,13 +1262,14 @@ describe('CodexAppServerProtocolClient', () => {
12471262
id: turnId,
12481263
status: 'completed',
12491264
error: null,
1250-
items: [agentMessageItem('unseen', 'final_answer', '{"ok":true}')],
1265+
items: [mcpToolCallItem('terminal-only', successfulMcpOutcome())],
12511266
},
12521267
})
12531268
);
12541269

12551270
await expect(fixture.result).rejects.toMatchObject({
12561271
failureClass: ReviewAgentFailureClass.StreamIncomplete,
1272+
message: 'review_agent_stream_incomplete_turn_completed',
12571273
});
12581274
});
12591275

__tests__/unit/review-investigation/review-agent-adapters.contract.test.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,28 @@ describe.each([
275275
});
276276
});
277277

278+
it('accepts exactly one schema-valid JSON payload surrounded by provider prose', async () => {
279+
const { adapter, runner, request } = fixture(ReviewAgentProviderKind.Codex);
280+
runner.rawCodexOutput = `Completed review.\n${JSON.stringify(turnOutput)}\nEnd of review.`;
281+
282+
await expect(adapter.executeTurn(request)).resolves.toMatchObject({
283+
outputVersion: 2,
284+
findings: turnOutput.findings,
285+
});
286+
});
287+
288+
it('rejects ambiguous provider prose containing multiple valid payloads', async () => {
289+
const { adapter, runner, request } = fixture(ReviewAgentProviderKind.Codex);
290+
runner.rawCodexOutput = `${JSON.stringify(turnOutput)}\n${JSON.stringify({
291+
...turnOutput,
292+
findings: [],
293+
})}`;
294+
295+
await expect(adapter.executeTurn(request)).rejects.toMatchObject({
296+
failureClass: ReviewAgentFailureClass.SchemaInvalidOutput,
297+
});
298+
});
299+
278300
it('fails malformed provider output as schema-invalid', async () => {
279301
const { adapter, runner, request } = fixture(providerKind);
280302
runner.output = { ...turnOutput, authoritativeClean: true };

dist/index.js

Lines changed: 66 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -108598,10 +108598,16 @@ var CodexAppServerProtocolClient = class {
108598108598
const id = requireIdentifier2(item.id, "item_id");
108599108599
const type2 = requireNonEmptyString(item.type, "item_type");
108600108600
this.validateAllowedItem(item, "completed");
108601-
const observed = this.completedItems.get(id);
108602-
if (!observed || snapshotItemIds.has(id) || observed.type !== type2) {
108601+
if (snapshotItemIds.has(id)) {
108603108602
throw streamFailure2();
108604108603
}
108604+
const observed = this.completedItems.get(id);
108605+
if (!observed) {
108606+
this.reconcileTerminalNonEffectfulItem(item, id, type2);
108607+
snapshotItemIds.add(id);
108608+
continue;
108609+
}
108610+
if (observed.type !== type2) throw streamFailure2();
108605108611
if (type2 === "mcpToolCall" && (observed.server !== item.server || observed.tool !== item.tool)) {
108606108612
throw confinementFailure("mcp_tool_identity_changed");
108607108613
}
@@ -108614,6 +108620,14 @@ var CodexAppServerProtocolClient = class {
108614108620
}
108615108621
}
108616108622
}
108623+
reconcileTerminalNonEffectfulItem(item, id, type2) {
108624+
if (type2 === "mcpToolCall") throw streamFailure2();
108625+
const active = this.activeItems.get(id);
108626+
if (active && active.type !== type2) throw streamFailure2();
108627+
this.activeItems.delete(id);
108628+
this.completedItems.set(id, Object.freeze({ type: type2 }));
108629+
if (type2 === "agentMessage") this.captureFinalMessage(item);
108630+
}
108617108631
validateAllowedItem(item, lifecycle) {
108618108632
const type2 = requireNonEmptyString(item.type, "item_type");
108619108633
if (FORBIDDEN_ITEM_TYPES.has(type2)) {
@@ -109945,7 +109959,7 @@ var CodexReviewAgentAdapter = class extends StrictCliReviewAgent {
109945109959
});
109946109960
let output;
109947109961
try {
109948-
output = parseReviewAgentTurnOutput(parseFinalJson(result2.finalMessage));
109962+
output = parseFinalTurnOutput(result2.finalMessage);
109949109963
} catch (error2) {
109950109964
throw schemaFailure(error2);
109951109965
}
@@ -110014,29 +110028,56 @@ var CodexReviewAgentAdapter = class extends StrictCliReviewAgent {
110014110028
return Object.freeze(args);
110015110029
}
110016110030
};
110017-
function parseFinalJson(message) {
110031+
function parseFinalTurnOutput(message) {
110018110032
const trimmed = message.trim();
110019-
try {
110020-
return JSON.parse(trimmed);
110021-
} catch (error2) {
110022-
const extracted = extractSingleJsonPayload(trimmed);
110023-
if (extracted === null) throw error2;
110024-
return JSON.parse(extracted);
110025-
}
110026-
}
110027-
function extractSingleJsonPayload(message) {
110028-
const fenced = message.match(/^```(?:json)?\s*\n([\s\S]*?)\n```\s*$/iu);
110029-
if (fenced) {
110030-
const payload = fenced[1];
110031-
return typeof payload === "string" ? payload.trim() : null;
110032-
}
110033-
const start = message.indexOf("{");
110034-
const end = message.lastIndexOf("}");
110035-
if (start < 0 || end <= start) return null;
110036-
const prefix = message.slice(0, start).trim();
110037-
const suffix = message.slice(end + 1).trim();
110038-
if (prefix || suffix) return null;
110039-
return message.slice(start, end + 1);
110033+
const valid = /* @__PURE__ */ new Map();
110034+
for (const candidate of extractJsonPayloadCandidates(trimmed)) {
110035+
try {
110036+
const output = parseReviewAgentTurnOutput(JSON.parse(candidate));
110037+
valid.set(JSON.stringify(output), output);
110038+
} catch {
110039+
}
110040+
}
110041+
if (valid.size !== 1) throw new Error("review_agent_output_invalid");
110042+
return [...valid.values()][0];
110043+
}
110044+
function extractJsonPayloadCandidates(message) {
110045+
const candidates = /* @__PURE__ */ new Set();
110046+
if (message) candidates.add(message);
110047+
const fencedPattern = /```(?:json)?\s*\n([\s\S]*?)\n```/giu;
110048+
for (const match2 of message.matchAll(fencedPattern)) {
110049+
const payload = match2[1]?.trim();
110050+
if (payload) candidates.add(payload);
110051+
}
110052+
let start = -1;
110053+
let depth = 0;
110054+
let inString = false;
110055+
let escaped = false;
110056+
for (let index = 0; index < message.length; index += 1) {
110057+
const character = message[index];
110058+
if (inString) {
110059+
if (escaped) escaped = false;
110060+
else if (character === "\\") escaped = true;
110061+
else if (character === '"') inString = false;
110062+
continue;
110063+
}
110064+
if (character === '"' && depth > 0) {
110065+
inString = true;
110066+
continue;
110067+
}
110068+
if (character === "{") {
110069+
if (depth === 0) start = index;
110070+
depth += 1;
110071+
continue;
110072+
}
110073+
if (character !== "}" || depth === 0) continue;
110074+
depth -= 1;
110075+
if (depth === 0 && start >= 0) {
110076+
candidates.add(message.slice(start, index + 1));
110077+
start = -1;
110078+
}
110079+
}
110080+
return Object.freeze([...candidates]);
110040110081
}
110041110082
function tomlString2(value) {
110042110083
return JSON.stringify(value);

dist/index.js.map

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/review-investigation/infrastructure/codex-app-server-protocol.ts

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -631,10 +631,16 @@ export class CodexAppServerProtocolClient {
631631
const id = requireIdentifier(item.id, 'item_id');
632632
const type = requireNonEmptyString(item.type, 'item_type');
633633
this.validateAllowedItem(item, 'completed');
634-
const observed = this.completedItems.get(id);
635-
if (!observed || snapshotItemIds.has(id) || observed.type !== type) {
634+
if (snapshotItemIds.has(id)) {
636635
throw streamFailure();
637636
}
637+
const observed = this.completedItems.get(id);
638+
if (!observed) {
639+
this.reconcileTerminalNonEffectfulItem(item, id, type);
640+
snapshotItemIds.add(id);
641+
continue;
642+
}
643+
if (observed.type !== type) throw streamFailure();
638644
if (
639645
type === 'mcpToolCall' &&
640646
(observed.server !== item.server || observed.tool !== item.tool)
@@ -654,6 +660,19 @@ export class CodexAppServerProtocolClient {
654660
}
655661
}
656662

663+
private reconcileTerminalNonEffectfulItem(
664+
item: Record<string, unknown>,
665+
id: string,
666+
type: string
667+
): void {
668+
if (type === 'mcpToolCall') throw streamFailure();
669+
const active = this.activeItems.get(id);
670+
if (active && active.type !== type) throw streamFailure();
671+
this.activeItems.delete(id);
672+
this.completedItems.set(id, Object.freeze({ type }));
673+
if (type === 'agentMessage') this.captureFinalMessage(item);
674+
}
675+
657676
private validateAllowedItem(
658677
item: Record<string, unknown>,
659678
lifecycle: 'started' | 'completed'

src/review-investigation/infrastructure/codex-review-agent-adapter.ts

Lines changed: 51 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
import {
1212
buildReviewAgentTurnOutputSchema,
1313
parseReviewAgentTurnOutput,
14+
type ReviewAgentTurnOutput,
1415
type ReviewTurnObservation,
1516
} from '../domain/turn-observation';
1617
import {
@@ -97,7 +98,7 @@ export class CodexReviewAgentAdapter extends StrictCliReviewAgent {
9798

9899
let output;
99100
try {
100-
output = parseReviewAgentTurnOutput(parseFinalJson(result.finalMessage));
101+
output = parseFinalTurnOutput(result.finalMessage);
101102
} catch (error) {
102103
throw schemaFailure(error);
103104
}
@@ -171,31 +172,61 @@ export class CodexReviewAgentAdapter extends StrictCliReviewAgent {
171172
}
172173
}
173174

174-
function parseFinalJson(message: string): unknown {
175+
function parseFinalTurnOutput(message: string): ReviewAgentTurnOutput {
175176
const trimmed = message.trim();
176-
try {
177-
return JSON.parse(trimmed);
178-
} catch (error) {
179-
const extracted = extractSingleJsonPayload(trimmed);
180-
if (extracted === null) throw error;
181-
return JSON.parse(extracted);
177+
const valid = new Map<string, ReviewAgentTurnOutput>();
178+
for (const candidate of extractJsonPayloadCandidates(trimmed)) {
179+
try {
180+
const output = parseReviewAgentTurnOutput(JSON.parse(candidate));
181+
valid.set(JSON.stringify(output), output);
182+
} catch {
183+
// Provider prose and malformed JSON are ignored unless no unique
184+
// schema-valid payload remains.
185+
}
182186
}
187+
if (valid.size !== 1) throw new Error('review_agent_output_invalid');
188+
return [...valid.values()][0]!;
183189
}
184190

185-
function extractSingleJsonPayload(message: string): string | null {
186-
const fenced = message.match(/^```(?:json)?\s*\n([\s\S]*?)\n```\s*$/iu);
187-
if (fenced) {
188-
const payload = fenced[1];
189-
return typeof payload === 'string' ? payload.trim() : null;
191+
function extractJsonPayloadCandidates(message: string): readonly string[] {
192+
const candidates = new Set<string>();
193+
if (message) candidates.add(message);
194+
195+
const fencedPattern = /```(?:json)?\s*\n([\s\S]*?)\n```/giu;
196+
for (const match of message.matchAll(fencedPattern)) {
197+
const payload = match[1]?.trim();
198+
if (payload) candidates.add(payload);
190199
}
191200

192-
const start = message.indexOf('{');
193-
const end = message.lastIndexOf('}');
194-
if (start < 0 || end <= start) return null;
195-
const prefix = message.slice(0, start).trim();
196-
const suffix = message.slice(end + 1).trim();
197-
if (prefix || suffix) return null;
198-
return message.slice(start, end + 1);
201+
let start = -1;
202+
let depth = 0;
203+
let inString = false;
204+
let escaped = false;
205+
for (let index = 0; index < message.length; index += 1) {
206+
const character = message[index]!;
207+
if (inString) {
208+
if (escaped) escaped = false;
209+
else if (character === '\\') escaped = true;
210+
else if (character === '"') inString = false;
211+
continue;
212+
}
213+
if (character === '"' && depth > 0) {
214+
inString = true;
215+
continue;
216+
}
217+
if (character === '{') {
218+
if (depth === 0) start = index;
219+
depth += 1;
220+
continue;
221+
}
222+
if (character !== '}' || depth === 0) continue;
223+
depth -= 1;
224+
if (depth === 0 && start >= 0) {
225+
candidates.add(message.slice(start, index + 1));
226+
start = -1;
227+
}
228+
}
229+
return Object.freeze([...candidates]);
199230
}
200231

201232
function tomlString(value: string): string {

0 commit comments

Comments
 (0)