Skip to content

fix(agent): default final-answer directive for bare allowFinalResponse: true (DEV-658) - #68

Merged
LukasParke merged 4 commits into
mainfrom
lukeparke/dev-658-agent-sdk-bare-allowfinalresponse-true-can-emit-raw-tool
Jul 21, 2026
Merged

fix(agent): default final-answer directive for bare allowFinalResponse: true (DEV-658)#68
LukasParke merged 4 commits into
mainfrom
lukeparke/dev-658-agent-sdk-bare-allowfinalresponse-true-can-emit-raw-tool

Conversation

@LukasParke

@LukasParke LukasParke commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Fixes DEV-658

Problem

When stopWhen halted the loop mid-tool-call, the forced final turn stripped tools/toolChoice/parallelToolCalls and appended no signal that this was the model's last turn. Two defects:

  1. Leak (DEV-658): models that emit tool-call syntax as text (GLM) attempt another tool call, which leaks into content as unparsed <tool_call>… text and gets treated as the final answer. Router PR #29461 worked around it by passing a directive string; every consumer using bare true was still exposed.
  2. Cache bust (review feedback): stripping the tools block invalidated the prompt-cache prefix on the final request.

Additionally, the final turn was opt-in — the common failure mode (run ends on a half-finished tool call) was the default.

Change

  • Final turn is on by default. allowFinalResponse: false opts out; omitted/true behave identically.
  • Tools are kept; calling is forbidden via toolChoice: 'none' — preserves the prompt-cache prefix. Same on the empty-final retry path (retryCurrentRequest).
  • Default directive: omitted/true appends DEFAULT_FINAL_RESPONSE_DIRECTIVE (exported) as the final user message; a non-empty string overrides the wording; '' appends nothing.

JSDoc, README, and changeset updated. Changeset bumped to minor: runs that previously ended on a halted tool-call turn now make one additional model request by default.

API example

import { callModel, stepCountIs, DEFAULT_FINAL_RESPONSE_DIRECTIVE } from '@openrouter/agent';

const result = callModel(client, {
  model: 'z-ai/glm-5.2',
  input: 'Research this step by step.',
  tools: [searchTool],
  stopWhen: stepCountIs(3),
  // was: no final turn unless allowFinalResponse was set; bare `true`
  //      stripped tools (cache-busting) and appended no directive, so
  //      GLM-style models could leak raw `<tool_call>…` as the answer
  // now: default-on final turn with toolChoice:'none' (tools kept, cache
  //      preserved) + DEFAULT_FINAL_RESPONSE_DIRECTIVE user message

  // custom wording still overrides the default:
  // allowFinalResponse: 'Summarize what you found.',
  // append no message (turn still happens):
  // allowFinalResponse: '',
  // restore the old opt-out (no final turn):
  // allowFinalResponse: false,
});

Tests

Unit (allow-final-response.test.ts, tool-terminal-empty-final.test.ts): final request keeps tools and forces toolChoice: 'none' (overriding the caller's 'required'); default-on path (option omitted) makes the final turn with the directive; string override, '', and false contracts pinned; retry path asserts tools kept + toolChoice: 'none'.

e2e regression (call-model.test.ts): DEV-658 repro shape — z-ai/glm-5.2, a web_search tool returning deliberately unhelpful results, stopWhen: stepCountIs(1), option omitted (default path under test). Asserts non-empty final text, no leaked tool-call syntax, zero structured function_call items.

Discrimination check (probed live before writing the test): legacy no-directive path leaked 3/3 runs; with the directive 0/3.

Verification

  • Unit: 521/521, typecheck clean, biome clean
  • e2e: full allowFinalResponse block passes against the live API (4/4, incl. GLM regression on the default path)
  • CI e2e job verified actually running after provisioning the OPENROUTER_API_KEY repo secret (was previously skip-passing)

Maintenance

Adds .agents/skills/public-api-examples/SKILL.md: public-API-changing PRs must carry a consumer-perspective code example in the PR description and changeset.

Follow-up (not this PR)

  • DEV-660: drop the router-level SERVER_TOOLS_FINAL_RESPONSE_DIRECTIVE workaround once this ships
  • Python/Go ports carry the same deficiencies (model_result.py _build_final_request, model_result.go sendFinalResponseRequest) — same fix shape applies

…Response: true

Bare `allowFinalResponse: true` stripped tools on the forced final turn
but gave the model no signal that this was its last turn. Models that
emit tool-call syntax as text (e.g. GLM) would attempt another tool call
and leak unparsed `<tool_call>...` text into the final content instead
of producing a real answer (DEV-658).

- `true` now appends DEFAULT_FINAL_RESPONSE_DIRECTIVE (exported) as the
  final user message
- a non-empty string still overrides the wording
- `''` appends no message (the previous bare-true behavior)

Adds an e2e regression test with the DEV-658 repro shape (GLM +
unhelpful search tool + stepCountIs(1)); verified to fail 3/3 on the
legacy no-directive path and pass 3/3 with the directive.

Fixes DEV-658

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@mattapperson mattapperson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowFinalResponse should not strip tools it should just set tool_choice: 'none',. removing the tools busts cache.

allowFinalResponse: true should be the default.

Codifies the review convention requested on #68: any PR changing the
public API must include a consumer-perspective code example in the PR
description and in its changeset (so it flows into CHANGELOG.md).
Brings this PR's changeset into compliance.
@LukasParke

Copy link
Copy Markdown
Contributor Author

Added .agents/skills/public-api-examples/SKILL.md codifying this: any PR changing the public API must include a consumer-perspective code example in the PR description and in its changeset (so it lands in CHANGELOG.md for npm consumers). This PR now complies — see the "API example" section above and the updated changeset.

…allowFinalResponse on

Review feedback from @mattapperson on #68:

1. The forced final turn no longer strips `tools`/`toolChoice`/
   `parallelToolCalls` — it keeps the request intact and forces
   `toolChoice: 'none'`. Stripping the tools block invalidated the
   prompt-cache prefix; forbidding calls preserves it. Same change on
   the empty-final retry path.

2. `allowFinalResponse` now defaults to ON. Omitted or `true` appends
   DEFAULT_FINAL_RESPONSE_DIRECTIVE; a non-empty string overrides the
   wording; '' appends nothing; `false` opts out of the final turn.

Changeset bumped to minor: runs that previously ended on a halted
tool-call turn now make one additional request by default.
@LukasParke

Copy link
Copy Markdown
Contributor Author

Both addressed in a1df1c6:

toolChoice: 'none' instead of stripping — the final turn (and the empty-final retry path) now keeps the full tools block and forces toolChoice: 'none', so the prompt-cache prefix stays intact. Unit tests assert tools are present and toolChoice === 'none' on both paths, including overriding a caller's toolChoice: 'required'.

Default-onallowFinalResponse now defaults to on: omitted/true append DEFAULT_FINAL_RESPONSE_DIRECTIVE, a non-empty string overrides the wording, '' appends nothing, false opts out entirely. The e2e GLM regression test now exercises the default path (option omitted).

Changeset bumped to minor since runs that previously ended on a halted tool-call turn now make one extra request by default — allowFinalResponse: false restores the old behavior. Full suite: unit 521/521, e2e allowFinalResponse block 4/4 live.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

Open in Devin Review

Comment on lines 3210 to +3211
const allowFinalResponse = this.options.allowFinalResponse;
const finalResponseEnabled =
allowFinalResponse === true || typeof allowFinalResponse === 'string';
const finalResponseEnabled = allowFinalResponse !== false;

@devin-ai-integration devin-ai-integration Bot Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: Default-on final turn adds an extra model request for existing stopWhen users

finalResponseEnabled = allowFinalResponse !== false (packages/agent/src/lib/model-result.ts:3211) makes the forced final turn happen by default whenever stopWhen halts mid-executable-tool-call. This is explicitly called out in the PR/changeset as an intentional minor bump, but it is worth confirming that no in-repo stateful/resume flows depend on the old behavior of ending on the halted tool-call turn. I checked the mcp package and found no stopWhen/allowFinalResponse usage, and the all-manual-tool-call path is still gated by hasExecutableToolCalls, so only executable-tool halts are affected.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@LukasParke
LukasParke requested a review from mattapperson July 21, 2026 16:32
@LukasParke
LukasParke merged commit 6807c51 into main Jul 21, 2026
6 checks passed
@LukasParke
LukasParke deleted the lukeparke/dev-658-agent-sdk-bare-allowfinalresponse-true-can-emit-raw-tool branch July 21, 2026 19:20
@github-actions github-actions Bot mentioned this pull request Jul 21, 2026
LukasParke added a commit that referenced this pull request Jul 30, 2026
* ci(release): dispatch the Python and Go ports on publish (HOP C)

The Python and Go ports of @openrouter/agent track this repo as their reference
spec, but nothing told them when a version shipped. Both were generated by hand
against 0.7.2 and have sat a minor version behind since — missing HooksManager,
versioned state serialization, and the #61-#68 fixes.

Adds a HOP C dispatch beside the existing HOP B monorepo dispatch. On a real
publish, python-agent and go-agent each receive openrouter-agent-published and
open a PR porting the delta. Their pipelines gate the result on a mechanical
verifier plus a behavioral parity eval before advancing sync state, so a bad
port cannot land silently.

Details worth noting:

- Sends the release TAG (@openrouter/agent@X.Y.Z), not a branch, so a port
  reproduces the exact published tree rather than whatever main drifted to.
- continue-on-error: the packages are already on npm when this runs, so a failed
  dispatch must not turn a successful release red. It warns instead, and names
  the manual recovery path.
- !cancelled() so a failed HOP B dispatch doesn't also skip the ports.
- Partial failure is tolerated: one port failing still dispatches the other.

Reuses the same GH_TOKEN PAT as HOP B, which additionally needs contents:write
on OpenRouterTeam/python-agent and OpenRouterTeam/go-agent.

Also documents the full publish fan-out in the changeset-versioning skill, since
a breaking callModel change now produces port PRs in two other repos.

Companion PRs: OpenRouterTeam/python-agent#19, OpenRouterTeam/go-agent#1

* fix(release): address review findings on HOP C dispatch

- Use -f (--raw-field) for all dispatch payload fields: -F treats
  values starting with @ (the release tag) as filenames, which made
  every port dispatch fail before the request was sent
- Push release tags on the manual mode=publish path so the dispatched
  ref actually exists on the remote
- Restore set -e; the gh api call sits in an if-condition and is
  already -e-exempt, so the rest of the script stays strict
- Pass source_run_url via env like VERSION instead of inline ${{ }}

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(release): fall back to commit SHA when the release tag is not on origin

Address review findings from cortex and Devin on the HOP C dispatch:

- Make the manual-publish tag push non-fatal. It runs after packages are
  already on npm, so a rejected push (tag protection, or a re-run where the
  tag exists at a different commit) turned a successful publish red and,
  because the HOP B/C steps carry no status guard, skipped both dispatches.

- Verify the tag is on origin before dispatching it, falling back to the
  run's commit SHA. Previously the manual path could dispatch a ref the port
  repos cannot resolve, making them fail on checkout instead of degrading.

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: LukasParke <luke@openrouter.ai>
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