Skip to content

fix(agent): accept a present_answer payload the model serialized - #406

Merged
cevheri merged 5 commits into
mainfrom
fix/accept-serialized-presentation
Aug 17, 2026
Merged

cevheri merged 5 commits into
mainfrom
fix/accept-serialized-presentation

Conversation

@yusuf-gundogdu

@yusuf-gundogdu yusuf-gundogdu commented Aug 16, 2026 •

Copy link
Copy Markdown
Member

Asked "Which part of the company costs us the most in salary?" against the embedded employees sample, qwen3.8 read the data, wrote the right query, got the right figure, and was scored as having answered nothing.

The rail showed a report with no answer beside it. The ledger agreed: no answer-composed entry, verdict unanswered (agent-data-analysis.1: no-answer). Every reading of that run said the model would not present its result.

It was presenting. We were refusing it.

Cause

present_answer was called three times, each time with a correct artifact id and a correct chart spec: right type, right x, right y, columns spelled exactly as the result spells them. Each call came back INVALID_TOOL_INPUT, because the model had serialized the nested object instead of nesting it.

{"artifact": "67fb154a-62bc-4615-af1e-920ec845e963",
 "presentation": "{\"kind\": \"chart\", \"spec\": {\"type\": \"bar\", \"x\": \"dept_name\", \"y\": [\"total_salary\"]}}"}

presentation is a string holding an object. The schema wants the object.

Why it was invisible

A refused ledger-only tool records no event. present_answer, compose_report, recommend_change and compare_plans answer the model and write nothing, so the run's ledger cannot distinguish "never tried" from "tried and was refused". Reading .workflow-data for that run shows the whole of it:

run-opened -> run-started -> context-captured -> statement-drafted
           -> tool-invoked (run_read_query) -> tool-completed
           -> report-composed -> closing-statement -> run-finished

Three refusals, no trace. The only surviving evidence was the model saying so itself, in its closing prose:

The presentation call is being persistently rejected despite conforming to the declared shape. I'll finalize the report, which is my required terminator.

It was conforming. Only the encoding was wrong. Finding this took a temporary log line in presentAnswer, which is a separate gap worth closing on its own.

What this changes

A serialized presentation is read back once at the call boundary, before validation:

function readSerializedPresentation(input: unknown): unknown {
  if (typeof input !== "object" || input === null) return input;
  const { presentation } = input as { presentation?: unknown };
  if (typeof presentation !== "string") return input;
  try {
    return { ...input, presentation: JSON.parse(presentation) };
  } catch {
    return input;
  }
}

The value then goes through the same discriminated union it always did, so nothing is admitted here that would not have been admitted written properly:

  • a serialized payload of the wrong shape is refused exactly as the object would have been
  • a string that is not JSON at all is refused, in the contract's own words
  • the parse is not recursive, so this accepts a serialization a model chose rather than inventing a second encoding

At the boundary, not inside the schema

The first version of this fix wrapped the field in z.preprocess. Review measured that it was not safe, and the boundary read replaced it.

z.preprocess produces a ZodPipe, and the SDK derives the model's own copy of the contract from this same object with toJSONSchema(schema, { target: 'draft-7', io: 'input' }) — where a ZodPipe is not counted as a required key. Measured over every entry of AGENT_TOOL_DEFINITIONS with that wrapper in place, present_answer was the only tool whose two contracts disagreed: runtime [artifact, presentation] against advertised [artifact], while all eight others matched exactly.

So a model that OBEYED the advertised contract would have sent artifact alone and been refused — and because this tool is ledger-only, that refusal records no event and the run is scored no-answer with nothing saying why. The same invisible failure this PR exists to remove, re-created for correct models instead of sloppy ones. Reading at the boundary leaves the advertised schema byte-identical to what it always was.

Scope

The change is in the agent tool layer's input contract. No provider is touched: every file under src/lib/llm, plus provider-registry.ts and model-adapter.ts, is unchanged, so Gemini and OpenAI deployments behave exactly as before.

 src/lib/agent/tools.ts                     |  62 +++++++-
 tests/isolated/agent-investigation.test.ts | 140 +++++++++++++++++
 tests/unit/lib/agent/tools.test.ts         | 244 +++++++++++++++++++++++++++++

Verification

Measured end to end against a local Ollama endpoint, one run per cell, macOS high power mode, Apple M5 Max:

Model Before After
qwen3.8 no-answer in 84.9s answered in 62.0s
muse-glimmer no-answer in 234.4s answered

Two models, same defect, so this reads as a general compatibility fix rather than a workaround for one model. Both successes were confirmed from .workflow-data: the ledger now carries an answer-composed entry that never appeared before.

The figure qwen3.8 reported (14,121,582 for Development) is also the most defensible of the seven models measured. It joined through current_dept_emp, which is the strictest reading of "current", and said in its claims which reading it had used. Two models that already passed report inflated totals because they count employees who changed department twice, or count people who have left.

Unit tests pin the contract: a serialized payload is accepted, a serialized payload of the wrong shape is still refused with INVALID_TOOL_INPUT, a non-JSON string is refused, and the advertised required keys stay identical to the runtime ones.

Two tests in tests/isolated/agent-investigation.test.ts drive the whole path through the real SDK, because the boundary read depends on a detail that is invisible from where it lives. The SDK validates the model's arguments against the same strict schema first, and a serialized presentation fails it: doParseToolCall throws, the SDK catches it, re-parses the raw JSON without a schema and enqueues the tool-call part anyway with invalid: true. The read is reached only because takeTurn dispatches every tool-call part without consulting that flag. Hardening that line to !part.invalid would silently undo this fix — a plausible edit rather than an imagined one, since capability-probe.ts already treats the flag as meaningful. The second test pins the SDK's invalid: true itself, so that edit fails a test rather than a run.

Depends on

#405 (the JWT memo test seam), merged.

yusuf-gundogdu and others added 4 commits August 17, 2026 01:08
getJwtSecret() memoizes its first successful read in a module-level variable.
A suite that shares one process therefore cannot observe the guard's throwing
cases once any earlier file has signed a token: the cached key answers before
the guard is reached.

`bun run test` runs tests/unit in a single process, so both throw cases in
auth-jwt-config.test.ts failed there while passing in isolation. The file's own
header note ("this file runs in its own process") described the workaround
rather than the fix.

resetJwtSecretCache() follows the test-seam pattern already used for
process-wide state in this repository (resetSecurityConfigWarnings,
resetAgentCapabilityCache). The test clears the memo around every case instead
of assuming an empty one.

`bun run test` now passes end to end.
Measured against a local Ollama endpoint, qwen3.8 could not finish a
data-analysis run. It called present_answer three times with a correct artifact
id and a correct chart spec (right type, right x, right y, columns spelled as
the result spells them), and every call was refused as INVALID_TOOL_INPUT
because it had serialized the nested object rather than nesting it:

  {"artifact": "67fb...", "presentation": "{\"kind\": \"chart\", \"spec\": {...}}"}

The run then reported without an answer and was scored no-answer. What a reader
saw was a model that would not present, and the ledger agreed, because a refused
ledger-only tool records no event. The model's own closing prose said the
presentation "is being persistently rejected despite conforming to the declared
shape". It was conforming; only the encoding was wrong.

`presentation` now accepts a string and reads it back once before validating.
The parsed value goes through the same schema, so a string holding the wrong
shape is refused exactly as the object would have been, and a string that is not
JSON is refused too. Nothing is admitted here that would not have been admitted
written properly. The parse is not recursive: this accepts a serialization a
model chose, it does not invent a second encoding.

Scope: the change is in the agent tool layer's input contract and touches no
provider. Every file under src/lib/llm, provider-registry.ts and
model-adapter.ts is untouched, so Gemini and OpenAI deployments behave exactly
as before.

Verified end to end against real models, not only in tests:

  qwen3.8       no-answer in 84.9s   ->  answered in 62.0s
  muse-glimmer  no-answer in 234.4s  ->  answered

Both models had the same defect, so this is a general compatibility fix rather
than a model-specific workaround. Three tests pin the behaviour: a serialized
payload is accepted, a serialized payload of the wrong shape is still refused,
and a string that is not JSON is refused.
#405 landed on main and supersedes this branch's first commit. Both started from
the same observation - the module-level JWT secret memo made the config guard's
throwing cases unobservable in a shared-process run - but reached it differently:
this branch added a resetJwtSecretCache() test seam, main dropped the memo
outright so there is no cache to reset.

Main's answer wins here, and not only because it landed first. 25 files under
tests/api/ replace @/lib/auth wholesale with an object literal that does not
carry the new export, so the seam turns an assertion failure into a link-time
"Export named 'resetJwtSecretCache' not found" under an unlucky load order - a
wider trigger than the problem it fixed.

Resolution:

  src/lib/auth.ts                        git auto-merged this file and produced
                                         code that does not compile: main deleted
                                         _jwtSecret, this branch added a function
                                         assigning to it, and the two edits did
                                         not overlap textually. Took main's file.
                                         The orphaned JSDoc ("the memo above")
                                         went with it.

  tests/unit/lib/auth-jwt-config.test.ts conflicted. Took main's file: it orders
                                         the success case first and pins a secret
                                         rotation, so a reintroduced memo fails
                                         the suite instead of being reset around.

What remains of this branch against main is the agent tool-layer change alone.
…the schema

The previous revision wrapped `presentAnswerSchema`'s `presentation` field in a
`z.preprocess` helper. That accepts the serialization qwen3.8 and muse-glimmer
send, which is the point of this branch and is kept - but `z.preprocess` produces
a `ZodPipe`, and the SDK derives the model's copy of the contract from the same
object with `toJSONSchema(schema, { target: 'draft-7', io: 'input' })`
(@ai-sdk/provider-utils/src/schema.ts:251, reached from `declaredTools()`), where
a `ZodPipe` is not counted as a required key.

Measured over every entry of AGENT_TOOL_DEFINITIONS, comparing the keys each tool
refuses a call for omitting against the keys it advertises as required:

  present_answer   runtime [artifact, presentation]   advertised [artifact]
  the other eight  identical sets

So the schema shown to the model declared `presentation` optional while the
runtime still refused a call without it. A model that OBEYED the advertised
contract sent `artifact` alone, was refused with INVALID_TOOL_INPUT, and because
this tool is ledger-only its refusal records no event: the run scored no-answer
with nothing saying why. That is the invisible failure this branch exists to
remove, re-created for correct models instead of sloppy ones.

The read now happens at the call boundary - `readSerializedPresentation(input)`
ahead of `parseToolInput` in `presentAnswerTool` - so the advertised schema is
byte-identical to main's for all nine tools, verified by rendering both and
diffing. Behaviour is unchanged across 41 differential cases driven against the
previous revision: nested object, serialized table, serialized chart, serialized
wrong shape, double-serialized, "null", non-object, missing presentation.

What the placement costs, named because it is invisible from the call site: the
SDK now validates a serialized presentation against the strict schema and marks
the tool call `invalid: true`, so the call reaches the tool only because
`takeTurn` dispatches tool-call parts without consulting that flag
(investigation.ts:1042). `capability-probe.ts:279` already reads the flag, so
hardening that line is a plausible edit, and it would silently undo this fix.
tests/isolated/agent-investigation.test.ts now drives the whole path through the
real SDK, so that edit fails a test rather than a run. Verified by applying it:
the new test goes red, and green again on revert.

Tests, all mutation-checked rather than merely written:

  - The contract invariant, over every tool, both sets derived and neither listed:
    every key the runtime refuses a call for omitting is a key the advertised
    schema requires. Read through the SDK's own `asSchema`, so an SDK upgrade that
    changes the conversion cannot leave it green.
  - The same pair compared in input and output mode, which covers every depth: the
    key-by-key half sees only the top level, and the same wrapper one level down
    passes it while the parser really does refuse the nested key.
  - The read happens ONCE and never recursively; a double-serialized string is
    still refused.
  - The guard's passthrough cases, including `undefined`, which is the only input
    that distinguishes the object check from a null check.

12 mutants applied and reverted, including deleting the call-site read, a
fabricated `{kind:"table"}` fallback in the catch arm, a recursive read, the
`z.preprocess` wrapper at two depths, and the `takeTurn` hardening. Each is
killed by a named test.

Gates on the merged tree: format, lint (0 errors), typecheck, knip, run-core.sh
(295 files), test:components (30 groups) and build all pass.
@cevheri

cevheri commented Aug 17, 2026

Copy link
Copy Markdown
Member

Merged main in and pushed a follow-up. Two things happened, one of them a defect in the branch as it stood.

The conflict: #405 landed and supersedes the first commit

Both this branch and #405 started from the same observation — the module-level JWT secret memo only ever held a successful read, so once anything in the process had signed a token the config guard was unreachable. This branch added a resetJwtSecretCache() test seam; #405 dropped the memo outright.

I took main's answer, and not only because it landed first: 25 files under tests/api/ replace @/lib/auth wholesale with an object literal that does not carry the new export, so the seam turns an assertion failure into a link-time Export named 'resetJwtSecretCache' not found under an unlucky load order — a wider trigger than the problem it fixed.

Worth flagging for anyone resolving this by hand: src/lib/auth.ts auto-merged without a conflict and produced code that does not compile. main deleted _jwtSecret, this branch added a function assigning to it, and the two edits never overlapped textually. Only the test file conflicted.

The defect: the schema advertised presentation as optional

z.preprocess produces a ZodPipe, and the SDK derives the model's copy of the contract from the same object with toJSONSchema(schema, { target: 'draft-7', io: 'input' }) (@ai-sdk/provider-utils/src/schema.ts:251, reached from declaredTools()) — where a ZodPipe is not counted as a required key.

Measured over every entry of AGENT_TOOL_DEFINITIONS, comparing the keys each tool refuses a call for omitting against the keys it advertises as required:

present_answer   runtime [artifact, presentation]   advertised [artifact]   <- the only gap
the other eight  identical sets

So the schema shown to the model declared presentation optional while the runtime still refused a call without it. A model that obeyed the advertised contract sent artifact alone and was refused with INVALID_TOOL_INPUT — and because present_answer is ledger-only, that refusal records no event and the run scores no-answer with nothing saying why. That is the invisible failure this branch exists to remove, re-created for correct models instead of sloppy ones.

The fix keeps everything the branch bought and moves the read to the call boundary: readSerializedPresentation(input) ahead of parseToolInput in presentAnswerTool. The advertised schema is now byte-identical to main's for all nine tools, verified by rendering both and diffing. Behaviour is unchanged across 41 differential cases driven side by side against the previous revision — nested object, serialized table, serialized chart, serialized wrong shape, double-serialized, "null", non-object, missing presentation.

What the placement costs, stated because it is invisible from the call site

With the strict schema restored, the SDK validates a serialized presentation, fails it, and marks the tool call invalid: true. The call reaches the tool only because takeTurn dispatches tool-call parts without consulting that flag (investigation.ts:1042). capability-probe.ts:279 already reads part.invalid, so hardening that line is a plausible edit — and it would silently undo this fix while every unit test stayed green, since they all call presentAnswerTool directly.

tests/isolated/agent-investigation.test.ts now drives the whole path through the real SDK. Applying the hardening edit turns the new test red; reverting turns it green.

Tests

  • The contract invariant over every tool, both sets derived and neither listed: every key the runtime refuses a call for omitting is a key the advertised schema requires. Read through the SDK's own asSchema, so an SDK upgrade that changes the conversion cannot leave it green.
  • The same schema compared in input and output mode, which covers every depth — the key-by-key half reads only the top level, and the same wrapper one level down passes it while the parser really does refuse the nested key.
  • The read happens once and never recursively; a double-serialized string is still refused.
  • The guard's passthrough cases including undefined, the only input that distinguishes the object check from a null check.

12 mutants were applied and reverted, including deleting the call-site read, a fabricated {kind:"table"} fallback in the catch arm, a recursive read, the z.preprocess wrapper at two depths, and the takeTurn hardening. Each is killed by a named test.

Local gates on the merged tree: format, lint (0 errors), typecheck, knip, run-core.sh (295 files), test:components (30 groups) and build all pass.

Not changed

The review also flagged compose_report and recommend_change as carrying the same nested shape and therefore the same defect. Measured: they do not — neither uses a preprocess, and both advertise their full required set. Nothing to do there.

@sonarqubecloud

Copy link
Copy Markdown

@cevheri
cevheri merged commit df15e56 into main Aug 17, 2026
22 checks passed
@cevheri
cevheri deleted the fix/accept-serialized-presentation branch August 17, 2026 09:23
cevheri added a commit that referenced this pull request Aug 18, 2026
… drive

Three findings from review, all three verified against the code and the history.

**The revision table was missing one.** #406 (`df15e56`) is an ANCESTOR of the
`907e83c` baseline, so the before-figures on `qwen3.8` (`no-answer` in 84.9s) and
`muse-glimmer` (`no-answer` after 234.4s) are reproducible from neither commit this
page named — they are from `7256f93`, before that fix. "Everything else is the
baseline" was therefore wrong for exactly the two rows a reader is most likely to try
to reproduce. All three revisions are named now, in a table that says which figures
belong to which.

**"One-shot" was true of a drive, not of a run.** The three booleans are `let`s inside
`runInvestigation`, and that function is also what resumes a run a dead process left
running — so a resumed drive starts with all three false and can repeat a notice. Two
of the three are partially covered by accident: the present-before-report notice reads
`answer-composed` off the ledger, but a REFUSED `present_answer` writes no event and so
is not covered; nothing bounds the report reminder or the reserve notice across a
resume at all.

That is the same defect B51 already describes rather than a second one, so it is folded
into that entry: recording delivery is what would make "once" mean once. The
declarations in `investigation.ts` say it too — a comment-only change, because a reader
reaching for `reserveAnnounced` should not have to derive the scope from the resume
path three hundred lines up.
cevheri added a commit that referenced this pull request Aug 18, 2026
…d what the nudges do not (#421)

* docs(agent): say which runtime the measured rows came from, and record what the nudges do not

`methodology.md` named one commit, `907e83c`, and the pages under it report figures that
commit cannot produce: the rows marked *(fixed)* and every "after" column were taken with
the report reminder and the present-before-report notice in place, and those landed as
#416 and #417. A reader following the method against `907e83c` alone would fail to
reproduce them and have no way to see why. Both commits are named now, along with the
review narrowing each notice took before it landed and why the recorded runs sit inside
the narrower triggers.

B51 records what those two notices do NOT do. `recordEvent` is called for what a run did
— the capture, the drafted statement, the closing prose, the answer, the report — and
never for what the server said to the model, so a rescued run and a run that needed no
rescue leave the same ledger, and a `compose_report` the loop withheld leaves no trace
that a call was made. That is a measurement problem before it is a design one, because
`docs/llms/` is built by reading those ledgers and its whole claim is that each figure
comes from an observed run.

The entry carries the second half too: three notices now, each with its own boolean, its
own guard set and its own delivery, and the guards are the part that keeps being got
wrong — both of the new ones shipped with a condition that named one thing and read
another, and both were caught in review rather than by a gate.

* docs(agent): name all three measured revisions, and scope "once" to a drive

Three findings from review, all three verified against the code and the history.

**The revision table was missing one.** #406 (`df15e56`) is an ANCESTOR of the
`907e83c` baseline, so the before-figures on `qwen3.8` (`no-answer` in 84.9s) and
`muse-glimmer` (`no-answer` after 234.4s) are reproducible from neither commit this
page named — they are from `7256f93`, before that fix. "Everything else is the
baseline" was therefore wrong for exactly the two rows a reader is most likely to try
to reproduce. All three revisions are named now, in a table that says which figures
belong to which.

**"One-shot" was true of a drive, not of a run.** The three booleans are `let`s inside
`runInvestigation`, and that function is also what resumes a run a dead process left
running — so a resumed drive starts with all three false and can repeat a notice. Two
of the three are partially covered by accident: the present-before-report notice reads
`answer-composed` off the ledger, but a REFUSED `present_answer` writes no event and so
is not covered; nothing bounds the report reminder or the reserve notice across a
resume at all.

That is the same defect B51 already describes rather than a second one, so it is folded
into that entry: recording delivery is what would make "once" mean once. The
declarations in `investigation.ts` say it too — a comment-only change, because a reader
reaching for `reserveAnnounced` should not have to derive the scope from the resume
path three hundred lines up.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants