diff --git a/lambda/agentcore/commands/run-stage.js b/lambda/agentcore/commands/run-stage.js index cf9cd3194..cd7dfff62 100644 --- a/lambda/agentcore/commands/run-stage.js +++ b/lambda/agentcore/commands/run-stage.js @@ -97,7 +97,7 @@ import { } from '../git-engine.js'; import { resolveStageModel } from '../model-resolver.js'; import { createGraphWriter, closeGraphSource } from '../mcp/graph-writer.js'; -import { CHECKPOINTS, chosenLabel } from '../mcp/process-bridge.js'; +import { CHECKPOINTS, answerFreeText, chosenLabel } from '../mcp/process-bridge.js'; import { LOOP_BACK_OPTION, parseChoice } from '../../shared/gate-answer.js'; import { ingestStageCodeTraceability as defaultIngestStageCodeTraceability } from '../code-traceability.js'; import { createSensorRunner, isReleaseDependencyError } from '../sensor-runner.js'; @@ -1314,10 +1314,11 @@ const formatResumeAnswer = (gate, stageId) => { const checkpoint = gate?.detail?.checkpoint ?? null; if (checkpoint) { // The same label parser and labels the bridge uses to decide whether a - // receipt is written, so this message never contradicts the receipt. - const label = chosenLabel(a); + // receipt is written, so this message never contradicts the receipt. It also + // reads the gate UI's `answers[]` shape (option indices into `gate.questions`). + const label = chosenLabel(gate); const approved = label === CHECKPOINTS[checkpoint]?.approve; - const free = typeof a === 'string' ? '' : (a?.freeText ?? a?.feedback ?? ''); + const free = answerFreeText(a); if (approved) { return ( `The human answered the ${checkpoint} checkpoint: "${label}".` + diff --git a/lambda/agentcore/mcp/process-bridge.js b/lambda/agentcore/mcp/process-bridge.js index 413ddc3a3..55d7c3d20 100644 --- a/lambda/agentcore/mcp/process-bridge.js +++ b/lambda/agentcore/mcp/process-bridge.js @@ -82,12 +82,42 @@ const authorizationRefusal = (gate) => { const sha256 = (value) => createHash('sha256').update(value).digest('hex'); -// The human's chosen label, across every answer shape the answer endpoints write -// (`perQuestion[]`, `freeText`, or a bare string — see run-stage formatResumeAnswer). -export const chosenLabel = (answer) => { +// The gate UI's answer shape: `answers[0].selectedOptions` holds indices into the +// gate's own `questions[0].options`, so the label is read from the gate row. Only +// one well-formed index names a label; anything else names none. +const selectedOptionLabel = (answer, questions) => { + const selected = answer?.answers?.[0]?.selectedOptions; + if (!Array.isArray(selected) || selected.length !== 1) return null; + const [index] = selected; + if (!Number.isInteger(index)) return null; + try { + return JSON.parse(questions)?.[0]?.options?.[index]?.label ?? null; + } catch { + return null; + } +}; + +// The human's free-text note: top-level `freeText`, the gate UI's +// `answers[0].freeText`, or `feedback`; '' for a bare string. +export const answerFreeText = (answer) => + answer?.freeText ?? answer?.answers?.[0]?.freeText ?? answer?.feedback ?? ''; + +// The human's chosen label on an answered gate row, across every answer shape the +// answer endpoints write (`perQuestion[]`, the gate UI's `answers[0].selectedOptions` +// resolved against the row's `questions`, `freeText`, `decision`, or a bare string — +// see run-stage formatResumeAnswer). The gate UI's `answers[0].freeText` is +// feedback, never a label. +export const chosenLabel = (gate) => { + const answer = gate?.answer; if (typeof answer === 'string') return answer.trim(); const perQuestion = Array.isArray(answer?.perQuestion) ? answer.perQuestion[0]?.answer : null; - return String(perQuestion ?? answer?.freeText ?? answer?.decision ?? '').trim(); + return String( + perQuestion ?? + selectedOptionLabel(answer, gate?.questions) ?? + answer?.freeText ?? + answer?.decision ?? + '', + ).trim(); }; export const createProcessBridge = ({ @@ -342,11 +372,8 @@ export const createProcessBridge = ({ // the human's words, and any other shape is a re-ask with no receipt — so a // free-text answer can never be mistaken for a confirmation. const resolveCheckpointAnswer = async ({ spec, checkpoint, task }) => { - const label = chosenLabel(task.answer); - const feedback = - typeof task.answer === 'string' - ? task.answer - : (task.answer?.freeText ?? task.answer?.feedback ?? label); + const label = chosenLabel(task); + const feedback = answerFreeText(task.answer) || label; if (label === spec.approve) { // The same provenance rule the resume leg applies: an answer that records // no human, or no digest of what was shown, authorizes nothing. @@ -480,6 +507,7 @@ export const createProcessBridge = ({ if (!policy) return null; for (const [checkpoint, spec] of Object.entries(CHECKPOINTS)) { const { latest } = await latestCheckpointGate(checkpoint); + const latestLabel = latest ? chosenLabel(latest) : ''; const round = latest ? checkpointRoundOf(checkpoint, latest) : 0; const existing = await store .getReceipt(executionId, { @@ -518,18 +546,14 @@ export const createProcessBridge = ({ continue; } // A "Request changes" answered after this confirmation withdrew it. - if ( - latest && - latest.humanTaskId !== existing.humanTaskId && - chosenLabel(latest.answer) === spec.reject - ) { + if (latest && latest.humanTaskId !== existing.humanTaskId && latestLabel === spec.reject) { continue; } activeAuthorizationId = existing.sk; continue; } if (!latest || latest.status === 'pending') continue; - if (chosenLabel(latest.answer) === spec.reject) { + if (latestLabel === spec.reject) { // The rejection was answered while no container existed, so nothing has // recorded the withdrawal yet. Writing it here is what stops an EARLIER // round's approval from still satisfying the durable evaluator — and it is @@ -559,7 +583,7 @@ export const createProcessBridge = ({ } continue; } - if (chosenLabel(latest.answer) !== spec.approve) continue; + if (latestLabel !== spec.approve) continue; const refusal = authorizationRefusal(latest); if (refusal) { await refuseAuthorization({ diff --git a/lambda/agentcore/test/checkpoint-confirmation.test.js b/lambda/agentcore/test/checkpoint-confirmation.test.js index 58efde862..60ecff264 100644 --- a/lambda/agentcore/test/checkpoint-confirmation.test.js +++ b/lambda/agentcore/test/checkpoint-confirmation.test.js @@ -210,27 +210,54 @@ describe('confirm_summary — the authorization it does and does not create', () expect(eventTypes(store)).toEqual(['v2.summary.requested']); }); - it('loops on Request changes without creating authority, then succeeds on the retry', async () => { - const store = fakeStore(); - const rejecting = inlineBridge(store, { - answer: { perQuestion: [{ answer: 'Request changes' }], freeText: 'model refunds too' }, - }); + // The gate UI (QuestionEditor) answers with option indices into the gate's own + // questions, not with the label: `{ answers: [{ selectedOptions: [i] }] }`. + it.each([ + [ + 'a perQuestion answer', + { perQuestion: [{ answer: 'Request changes' }], freeText: 'model refunds too' }, + { perQuestion: [{ answer: 'Looks correct' }] }, + ], + [ + 'a gate UI answer', + { answers: [{ selectedOptions: [1], freeText: 'model refunds too' }] }, + { answers: [{ selectedOptions: [0] }] }, + ], + ])( + 'loops on Request changes without creating authority, then succeeds on the retry (%s)', + async (_shape, rejection, approval) => { + const store = fakeStore(); + const rejecting = inlineBridge(store, { answer: rejection }); + + const first = await rejecting.confirmSummary({ summary: 'v1' }); + expect(first.decision).toBe('changes-requested'); + expect(first.choice).toBe('Request changes'); + expect(first.feedback).toBe('model refunds too'); + expect(store.receipts.size).toBe(0); + expect(eventTypes(store)).toEqual(['v2.summary.requested', 'v2.summary.changes_requested']); + + // The revision is a SECOND gate, not a re-answer of the first: the first is + // already answered, so the derived id advances a round. + const accepting = inlineBridge(store, { answer: approval }); + const second = await accepting.confirmSummary({ summary: 'v2 with refunds' }); + expect(second.decision).toBe('approved'); + expect(second.choice).toBe('Looks correct'); + expect(store.humanTasks.size).toBe(2); + expect(store.receipts.size).toBe(1); + }, + ); - const first = await rejecting.confirmSummary({ summary: 'v1' }); - expect(first.decision).toBe('changes-requested'); - expect(first.feedback).toBe('model refunds too'); + it.each([ + ['a multi-select names no label', { selectedOptions: [0, 1] }], + ['an out-of-range index names no label', { selectedOptions: [2] }], + ['a non-integer index names no label', { selectedOptions: ['0'] }], + ['a typed note is not a label', { selectedOptions: [], freeText: 'Looks correct' }], + ])('creates NO authority from a malformed gate UI answer: %s', async (_rule, given) => { + const store = fakeStore(); + const answer = { answers: [given] }; + const result = await inlineBridge(store, { answer }).confirmSummary({ summary: 'a summary' }); + expect(result.decision).toBe('re-ask'); expect(store.receipts.size).toBe(0); - expect(eventTypes(store)).toEqual(['v2.summary.requested', 'v2.summary.changes_requested']); - - // The revision is a SECOND gate, not a re-answer of the first: the first is - // already answered, so the derived id advances a round. - const accepting = inlineBridge(store, { - answer: { perQuestion: [{ answer: 'Looks correct' }] }, - }); - const second = await accepting.confirmSummary({ summary: 'v2 with refunds' }); - expect(second.decision).toBe('approved'); - expect(store.humanTasks.size).toBe(2); - expect(store.receipts.size).toBe(1); }); it('refuses to raise the checkpoint forever', async () => { @@ -345,30 +372,36 @@ describe('park / resume across a confirmation', () => { expect(store.receipts.size).toBe(0); }); - it('mints the receipt on the resume leg from the answered gate, not from memory', async () => { - const store = fakeStore(); - // The container that raised the checkpoint parked and is gone. - await parkingBridge(store).confirmSummary({ summary: 'please confirm' }); - answerLatestGate(store, { perQuestion: [{ answer: 'Looks correct' }] }); - - // A FRESH bridge — as the resume leg builds — must recover the authorization. - const resumed = createProcessBridge({ store, scope: SCOPE }); - const authorizationId = await resumed.rehydrateAuthorizations(); - - expect(authorizationId).toMatch(/^RECEIPT#summary-confirmation#si-1#0#-$/); - expect(store.receipts.size).toBe(1); - expect(eventTypes(store)).toContain('v2.summary.confirmed'); - // A write made after the resume carries the recovered authorization. - await resumed.stampArtifact({ - artifactId: 'a1', - artifactType: 'business-logic-model', - contentHash: 'deadbeef', - }); - expect(store.events.at(-1).detail).toMatchObject({ - artifactType: 'business-logic-model', - authorizationId, - }); - }); + it.each([ + ['a perQuestion answer', { perQuestion: [{ answer: 'Looks correct' }] }], + ['a gate UI answer', { answers: [{ selectedOptions: [0] }] }], + ])( + 'mints the receipt on the resume leg from the answered gate, not from memory (%s)', + async (_shape, approval) => { + const store = fakeStore(); + // The container that raised the checkpoint parked and is gone. + await parkingBridge(store).confirmSummary({ summary: 'please confirm' }); + answerLatestGate(store, approval); + + // A FRESH bridge — as the resume leg builds — must recover the authorization. + const resumed = createProcessBridge({ store, scope: SCOPE }); + const authorizationId = await resumed.rehydrateAuthorizations(); + + expect(authorizationId).toMatch(/^RECEIPT#summary-confirmation#si-1#0#-$/); + expect(store.receipts.size).toBe(1); + expect(eventTypes(store)).toContain('v2.summary.confirmed'); + // A write made after the resume carries the recovered authorization. + await resumed.stampArtifact({ + artifactId: 'a1', + artifactType: 'business-logic-model', + contentHash: 'deadbeef', + }); + expect(store.events.at(-1).detail).toMatchObject({ + artifactType: 'business-logic-model', + authorizationId, + }); + }, + ); it('does not invent an authorization from a parked or rejected gate', async () => { const pending = fakeStore(); @@ -850,28 +883,34 @@ describe('checkpoint authority is the latest decision, not the first', () => { ]); }); - it('withdraws it on the resume leg too, when the rejection was answered while parked', async () => { - const store = fakeStore(); - const confirming = inlineBridge(store, { - answer: { perQuestion: [{ answer: 'Looks correct' }] }, - }); - await confirming.confirmSummary({ summary: 'v1' }); - await confirming.stampArtifact({ artifactId: 'd1', artifactType: 'design' }); - expect(evaluate(store).ok).toBe(true); - - // Round 1 parks and is answered by the human while no container exists. - await parkingBridge(store).confirmSummary({ summary: 'a different summary' }); - answerLatestGate(store, { perQuestion: [{ answer: 'Request changes' }] }); - - const resumed = createProcessBridge({ store, scope: SCOPE }); - expect(await resumed.rehydrateAuthorizations()).toBeNull(); - - const result = evaluate(store); - expect(result.ok).toBe(false); - expect(result.findings.map((finding) => finding.code)).toEqual([ - 'summary_confirmation_missing', - ]); - }); + it.each([ + ['a perQuestion answer', { perQuestion: [{ answer: 'Request changes' }] }], + ['a gate UI answer', { answers: [{ selectedOptions: [1] }] }], + ])( + 'withdraws it on the resume leg too, when the rejection was answered while parked (%s)', + async (_shape, rejection) => { + const store = fakeStore(); + const confirming = inlineBridge(store, { + answer: { perQuestion: [{ answer: 'Looks correct' }] }, + }); + await confirming.confirmSummary({ summary: 'v1' }); + await confirming.stampArtifact({ artifactId: 'd1', artifactType: 'design' }); + expect(evaluate(store).ok).toBe(true); + + // Round 1 parks and is answered by the human while no container exists. + await parkingBridge(store).confirmSummary({ summary: 'a different summary' }); + answerLatestGate(store, rejection); + + const resumed = createProcessBridge({ store, scope: SCOPE }); + expect(await resumed.rehydrateAuthorizations()).toBeNull(); + + const result = evaluate(store); + expect(result.ok).toBe(false); + expect(result.findings.map((finding) => finding.code)).toEqual([ + 'summary_confirmation_missing', + ]); + }, + ); it('records the parked withdrawal once, however many containers resume after it', async () => { const store = fakeStore(); diff --git a/lambda/agentcore/test/checkpoint-ladder.test.js b/lambda/agentcore/test/checkpoint-ladder.test.js index b2f16573d..38373e1d4 100644 --- a/lambda/agentcore/test/checkpoint-ladder.test.js +++ b/lambda/agentcore/test/checkpoint-ladder.test.js @@ -542,6 +542,27 @@ describe('a parked checkpoint is delivered to the agent as a decision, not a Q&A } }); + it('reads the gate UI answer shape by resolving the option index on the gate', () => { + const questions = JSON.stringify([ + { type: 'single', options: [{ label: 'Looks correct' }, { label: 'Request changes' }] }, + ]); + const approved = formatResumeAnswer({ + detail: { checkpoint: 'summary-confirmation' }, + questions, + answer: { answers: [{ selectedOptions: [0] }] }, + }); + expect(approved).toContain('"Looks correct"'); + expect(approved).toContain('authorization is recorded'); + + const rejected = formatResumeAnswer({ + detail: { checkpoint: 'summary-confirmation' }, + questions, + answer: { answers: [{ selectedOptions: [1], freeText: 'split the aggregate' }] }, + }); + expect(rejected).toContain('"Request changes" — nothing is authorized yet'); + expect(rejected).toContain('What they want changed: split the aggregate'); + }); + it('leaves an ordinary question and a validation gate exactly as before', () => { // The checkpoint branch must not capture the two shapes that already worked. expect(