diff --git a/.changelog/next/fixed-issue-4166.md b/.changelog/next/fixed-issue-4166.md new file mode 100644 index 0000000000..3530afc1de --- /dev/null +++ b/.changelog/next/fixed-issue-4166.md @@ -0,0 +1 @@ +- Layered Intelligence now records a wrong-typed reasoner envelope field (e.g. a non-string `analysis`, an unresolvable `pause`) as `unparseable-response` instead of counting it as a successful `no-proposal` run diff --git a/client/src/components/apps/LayeredIntelligenceTab.test.jsx b/client/src/components/apps/LayeredIntelligenceTab.test.jsx index a7993d46e6..ff0a6ff346 100644 --- a/client/src/components/apps/LayeredIntelligenceTab.test.jsx +++ b/client/src/components/apps/LayeredIntelligenceTab.test.jsx @@ -29,7 +29,7 @@ describe('describeLastRun', () => { it('surfaces an unparseable reasoning response as a warning with actionable prose', () => { const r = describeLastRun({ lastRunAt: at, lastRunAction: 'no-op', lastRunReason: 'unparseable-response' }); expect(r.tone).toBe('warn'); - expect(r.text).toMatch(/no usable JSON/i); + expect(r.text).toMatch(/no usable answer/i); }); it('treats a genuine no-proposal run as neutral (not an error)', () => { @@ -65,7 +65,7 @@ describe('LayeredIntelligenceTab last-run status', () => { it('renders the durable last-run line when the config carries a run outcome', () => { render(); expect(screen.getByText(/Last run/i)).toBeInTheDocument(); - expect(screen.getByText(/no usable JSON/i)).toBeInTheDocument(); + expect(screen.getByText(/no usable answer/i)).toBeInTheDocument(); }); it('omits the last-run line before the loop has ever run', () => { diff --git a/client/src/utils/layeredIntelligenceReasons.js b/client/src/utils/layeredIntelligenceReasons.js index 9658657656..df2ab2691d 100644 --- a/client/src/utils/layeredIntelligenceReasons.js +++ b/client/src/utils/layeredIntelligenceReasons.js @@ -20,7 +20,7 @@ export const LI_NEUTRAL_REASONS = new Set([ const LI_REASON_LABELS = { 'no-provider': 'no AI provider is configured for it', - 'unparseable-response': 'the reasoning model returned no usable JSON — try a non-reasoning model or an API provider', + 'unparseable-response': 'the reasoning model returned no usable answer — no JSON at all, or an envelope field it got wrong (see the server log for which) — try a non-reasoning model or an API provider', 'no-proposal': 'the loop had nothing to propose', 'scope-suppressed': "the proposal's scope isn't allowed for this app", 'hard-gate-excluded': "excluded before filing — the loop's own execution is degraded and this proposal maps to self-improve scope or a chronically-failing domain", diff --git a/client/src/utils/layeredIntelligenceReasons.test.js b/client/src/utils/layeredIntelligenceReasons.test.js index 825833469f..8ebd6f5e56 100644 --- a/client/src/utils/layeredIntelligenceReasons.test.js +++ b/client/src/utils/layeredIntelligenceReasons.test.js @@ -3,7 +3,7 @@ import { formatLiReason, liReasonTone, LI_NEUTRAL_REASONS } from './layeredIntel describe('formatLiReason', () => { it('glosses a plain reason token', () => { - expect(formatLiReason({ action: 'no-op', reason: 'unparseable-response' })).toMatch(/no usable JSON/i); + expect(formatLiReason({ action: 'no-op', reason: 'unparseable-response' })).toMatch(/no usable answer/i); expect(formatLiReason({ action: 'no-op', reason: 'no-provider' })).toMatch(/no AI provider/i); }); diff --git a/server/services/autonomousJobs/layeredIntelligenceHooks.js b/server/services/autonomousJobs/layeredIntelligenceHooks.js index 461b1fa28a..99a7d7584e 100644 --- a/server/services/autonomousJobs/layeredIntelligenceHooks.js +++ b/server/services/autonomousJobs/layeredIntelligenceHooks.js @@ -589,20 +589,34 @@ export async function processTaskOutput({ appId, success, payload, agentId } = { // envelope only requires `payload` to be an object, and salvageSentinelPayload's // lenient extractor can surface a non-envelope object out of prose. const envelope = isTaskOutputPayload(payload) ? payload : null - const { proposal, pause } = validateReasonerResponse(envelope) - - // A reasoner that SUPPLIED a non-null proposal which then failed validation - // (missing/unknown scope, no title, unnormalizable slug) did not "look and find - // nothing" — it tried to propose and emitted the wrong shape. Both used to land - // on `no-proposal` → success. `proposal: null` stays the legitimate empty answer. - // Narrow on purpose: validateReasonerResponse is documented to drop invalid - // pieces leniently, and this only reclassifies the field that IS the deliverable. - const proposalAttemptedButInvalid = envelope != null && !proposal && envelope.proposal != null + const { proposal, invalidFields = [], pause } = validateReasonerResponse(envelope) + + // A reasoner that SUPPLIED an envelope field which then failed validation did not + // "look and find nothing" — it tried to answer and emitted the wrong shape. That + // used to land on `no-proposal` → success. Originally (#2727) only the `proposal` + // field was reclassified; #4166 extends it to every supplied-but-unusable field + // (`{"analysis": 7}`, a `pause` with no resolvable target), since + // validateReasonerResponse now reports them all in `invalidFields`. A `null` field + // is ABSENT, not malformed, so `proposal: null` stays the legitimate empty answer. + // A null envelope never reaches here with a non-empty list (the validator returns + // the empty triple for it), and the ternary below short-circuits on it anyway; the + // `= []` destructuring default keeps a stubbed validator from throwing. + // + // Asymmetry, unchanged from #2727 and deliberate: a SURVIVING proposal overwrites + // `reason` on every branch of the filing path below, so a junk `analysis` alongside + // a filed proposal records the filing outcome — but a surviving `pause` does NOT + // re-derive `reason`, so it still applies its blocking label while the run records + // `unparseable-response`. The proposal is the run's deliverable; a pause is a side + // effect, and a reasoner that got another field wrong is worth surfacing. + const malformedEnvelope = invalidFields.length > 0 + // Name the offending fields — `unparseable-response` alone can't tell an operator + // whether the model returned no JSON at all or one wrong-typed key. + if (malformedEnvelope) console.warn(`⚠️ Layered Intelligence: ${app.name} reasoner envelope has unusable fields: ${invalidFields.join(', ')}`) let filedNumber = null let filedKey = null let filedAction = 'no-op' - let reason = envelope == null || proposalAttemptedButInvalid ? 'unparseable-response' : 'no-proposal' + let reason = envelope == null || malformedEnvelope ? 'unparseable-response' : 'no-proposal' let handedOff = false // §4 (#2764): when the deterministic routing gate files-for-human instead of // auto-handing-off a trivial+safe proposal, surface WHY on the returned result. diff --git a/server/services/autonomousJobs/layeredIntelligenceHooks.test.js b/server/services/autonomousJobs/layeredIntelligenceHooks.test.js index f38628af87..cd1c61cb7a 100644 --- a/server/services/autonomousJobs/layeredIntelligenceHooks.test.js +++ b/server/services/autonomousJobs/layeredIntelligenceHooks.test.js @@ -18,7 +18,11 @@ vi.mock('../../lib/workTracker.js', async (importActual) => { }; }); -vi.mock('../../lib/fileUtils.js', () => ({ +// Only `tryReadFile` is stubbed — the rest passes through so the real validator this +// suite imports for the #4166 end-to-end case can load its own module graph (its +// constants reach `PATHS`/`DAY` through here). fileUtils has no load-time side effects. +vi.mock('../../lib/fileUtils.js', async (importOriginal) => ({ + ...(await importOriginal()), tryReadFile: vi.fn().mockResolvedValue(null) })); @@ -51,7 +55,7 @@ vi.mock('../layeredIntelligence.js', () => ({ listForgeIssues: vi.fn().mockResolvedValue({ ok: true, issues: [] }), listBlockingIssues: vi.fn().mockResolvedValue({ ok: true, issues: [] }), isAppParked: vi.fn(() => false), - validateReasonerResponse: vi.fn(() => ({ proposal: null, pause: null })), + validateReasonerResponse: vi.fn(() => ({ proposal: null, pause: null, invalidFields: [] })), isScopeAllowed: vi.fn(() => true), isProposalDuplicate: vi.fn(() => false), checkSemanticDuplicate: vi.fn().mockResolvedValue({ available: false, duplicate: false }), @@ -133,6 +137,10 @@ import { resolveAppWorkTracker } from '../../lib/workTracker.js'; import { tryReadFile } from '../../lib/fileUtils.js'; import { getProviderById } from '../providers.js'; import { getTaskInterval } from '../taskSchedule.js'; +// The REAL validator (its own module, so untouched by the `../layeredIntelligence.js` +// mock above) — used by the #4166 end-to-end case so the envelope→reason verdict is +// exercised through production validation, not a hand-written stub of it. +import { validateReasonerResponse as realValidateReasonerResponse } from '../layeredIntelligence/proposal.js'; const APP = { id: 'app-1', name: 'App One', repoPath: '/repo', taskTypeOverrides: {} }; @@ -158,6 +166,9 @@ beforeEach(() => { // so without this a test that arms the gate would leak into later ones. li.computeHardExclusionGate.mockReturnValue({ excluded: false, reason: null }); li.computeHardExclusionNotice.mockReturnValue(''); + // Same leak guard for the validator (#4166): a test that hands back a non-empty + // `invalidFields` would otherwise make every later run read as unparseable-response. + li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null, invalidFields: [] }); }); describe('buildTaskInput', () => { @@ -557,7 +568,7 @@ describe('processTaskOutput', () => { }); it('records unparseable-response when the payload is null', async () => { - li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null }); + li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null, invalidFields: [] }); const out = await processTaskOutput({ appId: 'app-1', success: true, payload: null }); expect(out).toMatchObject({ action: 'no-op', reason: 'unparseable-response' }); }); @@ -569,7 +580,7 @@ describe('processTaskOutput', () => { // indistinguishable from a correct empty answer and got recorded as a // successful run. `{}` is reachable: the sentinel envelope only requires // `payload` to be an object. - li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null }); + li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null, invalidFields: [] }); for (const payload of ['just some prose', 42, ['a', 'b'], true, {}, { foo: 1 }]) { const out = await processTaskOutput({ appId: 'app-1', success: true, payload }); expect(out).toMatchObject({ action: 'no-op', reason: 'unparseable-response' }); @@ -580,7 +591,7 @@ describe('processTaskOutput', () => { // The other side of the sentinel: the reasoner answered correctly and simply // had nothing to file. That is a successful run, not malformed output. Any ONE // documented key makes it a real answer. - li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null }); + li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null, invalidFields: [] }); for (const payload of [{ analysis: 'nothing worth proposing', proposal: null }, { proposal: null }, { analysis: '' }]) { const out = await processTaskOutput({ appId: 'app-1', success: true, payload }); expect(out).toMatchObject({ action: 'no-op', reason: 'no-proposal' }); @@ -591,13 +602,53 @@ describe('processTaskOutput', () => { // The reasoner ATTEMPTED a proposal and emitted the wrong shape (no scope/title, // bad slug). That is malformed output, not "I looked and found nothing" — both // used to land on `no-proposal` and count as a successful run. - li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null }); + li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null, invalidFields: ['proposal'] }); const out = await processTaskOutput({ appId: 'app-1', success: true, payload: { analysis: 'x', proposal: { title: '' } } }); expect(out).toMatchObject({ action: 'no-op', reason: 'unparseable-response' }); }); + it('records unparseable-response for a wrong-typed NON-deliverable field (#4166)', async () => { + // `{"analysis": 7}` is an envelope (it carries a documented key) that supplies a + // field it then gets wrong. Pre-#4166 the deliverable-only check let it through as + // `no-proposal`, i.e. a successful run indistinguishable from a correct empty + // answer. Any field validateReasonerResponse reports in `invalidFields` now counts. + for (const [payload, invalidFields] of [ + [{ analysis: 7 }, ['analysis']], + [{ analysis: 'x', pause: { reason: 'no target' } }, ['pause']], + [{ analysis: [], pause: 3 }, ['analysis', 'pause']] + ]) { + li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null, invalidFields }); + const out = await processTaskOutput({ appId: 'app-1', success: true, payload }); + expect(out).toMatchObject({ action: 'no-op', reason: 'unparseable-response' }); + } + }); + + it('reaches the same verdict through the REAL validator (#4166)', async () => { + // End-to-end over the two halves this suite otherwise stubs apart: the real + // envelope validation feeding the real reason classification. + li.validateReasonerResponse.mockImplementation(realValidateReasonerResponse); + for (const payload of [{ analysis: 7 }, { analysis: 'x', pause: { reason: 'no target' } }, { proposal: { title: '' } }]) { + const out = await processTaskOutput({ appId: 'app-1', success: true, payload }); + expect(out).toMatchObject({ action: 'no-op', reason: 'unparseable-response' }); + } + for (const payload of [{ analysis: 'nothing worth proposing', proposal: null }, { analysis: '' }]) { + const out = await processTaskOutput({ appId: 'app-1', success: true, payload }); + expect(out).toMatchObject({ action: 'no-op', reason: 'no-proposal' }); + } + }); + + it('logs which envelope fields were unusable (#4166)', async () => { + // `unparseable-response` alone can't tell an operator whether the model returned + // no JSON at all or one wrong-typed key — the offending names must reach the log. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null, invalidFields: ['analysis', 'pause'] }); + await processTaskOutput({ appId: 'app-1', success: true, payload: { analysis: 7, pause: 3 } }); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('analysis, pause')); + warn.mockRestore(); + }); + it('treats an explicit proposal:null as a legitimate empty answer, not malformed (#2727)', async () => { - li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null }); + li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null, invalidFields: [] }); const out = await processTaskOutput({ appId: 'app-1', success: true, payload: { analysis: 'nothing to propose', proposal: null } }); expect(out).toMatchObject({ action: 'no-op', reason: 'no-proposal' }); }); @@ -606,7 +657,7 @@ describe('processTaskOutput', () => { // readIssues is an unbounded forge call and only the has-a-proposal branch // consumes it. Since the #2727 hoist the hook runs while the agent still holds // a CoS concurrency slot, so the common no-op path must not shell out to `gh`. - li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null }); + li.validateReasonerResponse.mockReturnValue({ proposal: null, pause: null, invalidFields: [] }); li.listForgeIssues.mockClear(); await processTaskOutput({ appId: 'app-1', success: true, payload: { analysis: 'nothing to do', proposal: null } }); expect(li.listForgeIssues).not.toHaveBeenCalled(); diff --git a/server/services/layeredIntelligence.test.js b/server/services/layeredIntelligence.test.js index c406cb08e5..e80a615a0a 100644 --- a/server/services/layeredIntelligence.test.js +++ b/server/services/layeredIntelligence.test.js @@ -375,9 +375,10 @@ describe('validateReasonerResponse', () => { }); it('handles garbage input without throwing', () => { - expect(validateReasonerResponse(null)).toEqual({ analysis: '', proposal: null, pause: null }); - expect(validateReasonerResponse('nope')).toEqual({ analysis: '', proposal: null, pause: null }); - expect(validateReasonerResponse({ proposal: [], pause: [] })).toEqual({ analysis: '', proposal: null, pause: null }); + expect(validateReasonerResponse(null)).toEqual({ analysis: '', proposal: null, pause: null, invalidFields: [] }); + expect(validateReasonerResponse('nope')).toEqual({ analysis: '', proposal: null, pause: null, invalidFields: [] }); + expect(validateReasonerResponse({ proposal: [], pause: [] })) + .toEqual({ analysis: '', proposal: null, pause: null, invalidFields: ['proposal', 'pause'] }); }); it('normalizes proposal complexity + safe (defaults: null / false)', () => { @@ -394,6 +395,67 @@ describe('validateReasonerResponse', () => { expect(junk.proposal.complexity).toBe(null); expect(junk.proposal.safe).toBe(false); }); + + describe('invalidFields (#4166)', () => { + it('reports a wrong-typed analysis and drops it', () => { + const r = validateReasonerResponse({ analysis: 7 }); + expect(r.analysis).toBe(''); + expect(r.invalidFields).toEqual(['analysis']); + }); + + it('reports a supplied-but-unusable proposal', () => { + expect(validateReasonerResponse({ proposal: { scope: 'nuke', slug: 'x', title: 'X' } }).invalidFields).toEqual(['proposal']); + expect(validateReasonerResponse({ proposal: 'do the thing' }).invalidFields).toEqual(['proposal']); + }); + + it('reports a supplied-but-unusable pause', () => { + expect(validateReasonerResponse({ pause: { blockOnIssue: 42, reason: '' } }).invalidFields).toEqual(['pause']); + expect(validateReasonerResponse({ pause: { reason: 'no target' } }).invalidFields).toEqual(['pause']); + // "this" with nothing to block on is malformed, not an empty answer. + expect(validateReasonerResponse({ proposal: null, pause: { blockOnIssue: 'this', reason: 'x' } }).invalidFields).toEqual(['pause']); + }); + + it('does not double-report a "this" pause dropped as collateral of an invalid proposal', () => { + // The pause is well-formed — it was dropped only because the proposal it pointed + // at failed validation. One root cause, one report. + const r = validateReasonerResponse({ + proposal: { scope: 'nuke', slug: 'x', title: 'X' }, + pause: { blockOnIssue: 'this', reason: 'block on the new one' } + }); + expect(r.pause).toBe(null); + expect(r.invalidFields).toEqual(['proposal']); + }); + + it('still reports a "this" pause that is itself malformed alongside a bad proposal', () => { + // No reason → the pause is broken on its own terms, not collateral. + const r = validateReasonerResponse({ + proposal: { scope: 'nuke', slug: 'x', title: 'X' }, + pause: { blockOnIssue: 'this', reason: '' } + }); + expect(r.invalidFields).toEqual(['proposal', 'pause']); + }); + + it('treats an absent or explicitly-null field as absent, never malformed', () => { + expect(validateReasonerResponse({ analysis: 'nothing to propose', proposal: null, pause: null }).invalidFields).toEqual([]); + expect(validateReasonerResponse({ proposal: null }).invalidFields).toEqual([]); + expect(validateReasonerResponse({ analysis: '' }).invalidFields).toEqual([]); + expect(validateReasonerResponse({}).invalidFields).toEqual([]); + }); + + it('stays empty for a fully valid envelope', () => { + const r = validateReasonerResponse({ + analysis: 'a', + proposal: { scope: 'app-improvement', slug: 'x', title: 'X' }, + pause: { blockOnIssue: 'this', reason: 'block on the new one' } + }); + expect(r.invalidFields).toEqual([]); + }); + + it('collects every unusable field, in envelope order', () => { + const r = validateReasonerResponse({ analysis: [], proposal: {}, pause: 3 }); + expect(r.invalidFields).toEqual(['analysis', 'proposal', 'pause']); + }); + }); }); describe('isHandoffEligible', () => { diff --git a/server/services/layeredIntelligence/proposal.js b/server/services/layeredIntelligence/proposal.js index be6f7fdbc2..2defadbffb 100644 --- a/server/services/layeredIntelligence/proposal.js +++ b/server/services/layeredIntelligence/proposal.js @@ -2,7 +2,8 @@ * Layered Intelligence — reasoner-output validation & hand-off shaping * (#2842 split of layeredIntelligence.js). * - * Validates the model's JSON into a `{ analysis, proposal, pause }` triple, resolves + * Validates the model's JSON into an `{ analysis, proposal, pause, invalidFields }` + * result (the usable triple plus the malformed-field report, #4166), resolves * the pause target, and decides whether/how a filed proposal becomes an autonomous * CoS hand-off task. Also the tracker→filer dispatch table. */ @@ -12,17 +13,32 @@ import { normalizeSlug } from './dedup.js'; /** * Validate + normalize the reasoner's JSON. Returns - * `{ analysis, proposal, pause }` with invalid pieces dropped (never throws): + * `{ analysis, proposal, pause, invalidFields }` with invalid pieces dropped + * (never throws): + * - `analysis` kept only when it is a string. * - `proposal` kept only when it has a recognized scope + a normalizable slug * + a non-empty title. `slug` is normalized in place. * - `pause` kept only when it has a reason AND a resolvable target: an integer * issue number, or `"this"` WITH a surviving proposal to block on. A * `pause.blockOnIssue: "this"` with a null proposal is invalid → dropped. + * + * `invalidFields` (#4166) names every envelope key that was SUPPLIED (present and + * non-null) but could not be used — the sentinel that separates "the reasoner + * emitted the wrong shape" from "the reasoner had nothing to say". Dropping is + * still lenient (callers that only want the usable pieces can ignore it), but a + * caller deciding whether a run was malformed must not have to re-derive per-field + * validity: `{"analysis": 7}` reads as a legitimate empty answer without this. + * `null`/`undefined` is ABSENT, not wrong-typed — an explicit `proposal: null` is + * the documented "nothing to propose" answer and never lands here. Deliberately + * ONLY `null`: the prompt documents `null` for "not proposing"/"not pausing", so a + * model emitting `false` or `"none"` instead is a wrong-typed field and is reported + * (this already matched the pre-#4166 treatment of a `false` proposal). */ export function validateReasonerResponse(parsed) { - const out = { analysis: '', proposal: null, pause: null }; + const out = { analysis: '', proposal: null, pause: null, invalidFields: [] }; if (!parsed || typeof parsed !== 'object') return out; if (typeof parsed.analysis === 'string') out.analysis = parsed.analysis; + else if (parsed.analysis != null) out.invalidFields.push('analysis'); const p = parsed.proposal; if (p && typeof p === 'object' && !Array.isArray(p)) { @@ -43,8 +59,16 @@ export function validateReasonerResponse(parsed) { }; } } + if (p != null && !out.proposal) out.invalidFields.push('proposal'); const pause = parsed.pause; + // A well-formed `blockOnIssue: "this"` pause is dropped as COLLATERAL when the + // proposal it pointed at was itself supplied-but-invalid. The pause isn't the + // malformed part, so it doesn't earn its own `invalidFields` entry — one root + // cause, one report. With no proposal supplied at all, `"this"` points at nothing + // and the pause IS malformed. Either way `invalidFields` is already non-empty, so + // this only sharpens the report, never the malformed/clean verdict. + let pauseCollateral = false; if (pause && typeof pause === 'object' && !Array.isArray(pause)) { const reason = typeof pause.reason === 'string' ? pause.reason.trim() : ''; const target = pause.blockOnIssue; @@ -53,8 +77,12 @@ export function validateReasonerResponse(parsed) { // "this" requires a surviving proposal to block on; else an explicit issue number. if (reason && ((isThis && out.proposal) || num)) { out.pause = { blockOnIssue: isThis ? 'this' : num, reason }; + } else if (reason && isThis) { + pauseCollateral = out.invalidFields.includes('proposal'); } } + if (pause != null && !out.pause && !pauseCollateral) out.invalidFields.push('pause'); + return out; }