Skip to content

fix(advisor): five real-world bugs blocking client-side mode - #184

Merged
agentforce314 merged 2 commits into
mainfrom
advisor/3p-fixes
May 20, 2026
Merged

fix(advisor): five real-world bugs blocking client-side mode#184
agentforce314 merged 2 commits into
mainfrom
advisor/3p-fixes

Conversation

@agentforce314

Copy link
Copy Markdown
Owner

Summary

Real-world bug fixes for the client-side advisor mode shipped in #182. The original PR passed all unit tests but failed when actually exercised against a live provider (haiku-4-5 main + opus-4-7 advisor, both routed through litellm.singula.ai). Each bug below was caught during that live verification.

The five bugs

1. `agent_loop.py` had zero advisor wiring
Phase-03 (#179, landed via #181) wired the advisor into `_call_model_sync`, but the REPL, TUI, and headless paths all route through `run_agent_loop` — not `query.query`. The advisor was effectively dead code outside the subagent path. Fix: replicate the activation predicate + schema/instructions injection + ctx snapshot inside the `run_agent_loop` per-turn loop.

2. Duplicate `advisor` tool name in tools[]
`agent_loop.py` iterated `tool_registry.list_tools()` which doesn't filter `is_enabled()` — so the hidden `AdvisorTool` leaked into the base list. When client-side mode then appended the advisor schema, the API got two entries with name `advisor` → `BadRequestError: Tool names must be unique`. Fix: filter by `is_enabled()` at both the agent_loop build site and (belt-and-suspenders) in `_call_model_sync`.

3. Provider constructor rejected `default_model` kwarg
`execute_client_advisor` passed the raw config dict from `get_provider_config()` straight to the provider constructor. That dict uses `default_model`; the constructor expects `model`. Crashed `OpenAIProvider.init()`. Fix: explicit key translation (`api_key`, `base_url`, `model` only).

4. Forwarded conversation ended with assistant message
The advisor is invoked from inside a `tool_use` block, so the natural tail is the assistant message that contains it. Vertex-fronted Anthropic (via litellm) rejects this: "This model does not support assistant message prefill. The conversation must end with a user message." Fix: append a synthetic user "please advise" turn when needed.

5. `tool_use`/`tool_result` blocks in forwarded messages
The advisor is called with `tools=[]` (it just emits text). But proxies reject `tool_use`/`tool_result` blocks when no `tools=` array is sent: "Anthropic doesn't support tool calling without tools= param specified". Fix: flatten all tool blocks to `[Tool call: ...]` / `[Tool result: ...]` single-line text summaries. Preserves substance without the typed schema. Drops `thinking` blocks too.

Verified end-to-end

Live run config:

Observed flow:

  1. Worker called advisor BEFORE writing code
  2. Advisor replied with concrete planning guidance (package.json deps, tsconfig settings, router structure)
  3. Worker wrote 9 React blog files
  4. Worker called advisor again at the end
  5. Advisor caught that the first Write batch had empty content placeholders and instructed the worker to redo with actual content
  6. Worker redid; final state: 9 working files on disk at `/tmp/blog-test/`

Test plan

  • tests/test_advisor_helpers.py — 32 passed
  • tests/test_advisor_orphan_pairing.py — 6 passed
  • tests/test_advisor_chat_response_roundtrip.py — 6 passed
  • tests/test_advisor_command.py — 14 passed
  • tests/test_advisor_request_wiring.py — 13 passed
  • tests/test_advisor_client_side.py — 34 passed (2 new tests for proxy routing + fallback)
  • tests/test_app_state.py — 16 passed
  • tests/integration/test_advisor_smoke.py — 3 passed
  • Total: 124 advisor tests + tool_system/settings regression all green

🤖 Generated with Claude Code

agentforce314 and others added 2 commits May 20, 2026 14:46
Two bugs found by inspecting actual provider signatures (not caught by
the original mocks which used loose MagicMock).

1. **System prompt was silently lost on 3P providers.** Original code
   passed `system=...` as a kwarg to `chat()`. AnthropicProvider pops
   it and passes to the API; OpenAI-compat providers (Gemini,
   OpenRouter, GLM via openai-shim) ignore the kwarg — they expect
   a `{"role": "system", "content": ...}` message at the head of the
   array. Fixed by detecting provider type via isinstance check
   (mirrors the same gate already in src/query/query.py:424) and
   sending the right shape.

2. **`abort_signal` and `stream=False` kwargs were forwarded as
   unknown params.** AnthropicProvider's `chat()` (line 239) forwards
   unknown kwargs straight to `client.messages.create()` which would
   reject `abort_signal`. Fixed by using `chat_stream_response`
   (BaseProvider contract has uniform `abort_signal`) with a fallback
   to plain `chat()` for providers that don't implement streaming.

Tests:
- New: `test_openai_shape_gets_system_as_first_message` — verifies
  the system prompt lands as the first message for non-Anthropic.
- New: `test_falls_back_to_chat_when_stream_unimplemented` — verifies
  the NotImplementedError fallback path.
- Existing tests updated to use `spec=AnthropicProvider` mocks so
  isinstance detection works.

All 122 advisor tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Verified by running real workload: haiku-4-5 main + opus-4-7 advisor,
both routed through litellm.singula.ai. Five bugs surfaced during the
first live run; this commit fixes them all.

## 1. agent_loop.py had zero advisor wiring

The REPL, TUI, and headless paths all route through ``run_agent_loop``
(not ``query.query``), so my Phase-03 wiring in ``_call_model_sync``
never actually fired in production. The advisor was effectively dead
code outside the subagent path.

Fix: replicate the activation predicate + schema/instructions
injection + ctx.messages/_active_provider snapshot inside the
``run_agent_loop`` per-turn loop. Same logic, same cache-friendly
ordering (append at end). Tracked separately to keep the diff
reviewable; long-term should share via a helper.

## 2. Duplicate ``advisor`` tool name in tools[]

``agent_loop.py:310`` iterated ``tool_registry.list_tools()`` which
does NOT filter by ``is_enabled()`` — so the hidden AdvisorTool leaked
into the base tool list. When client-side mode then appended the
advisor schema, the API got two entries with name "advisor" and
returned ``BadRequestError: Tool names must be unique``.

Fix: filter both at the agent_loop build site and (belt-and-suspenders)
in ``_call_model_sync``'s tool-schemas construction.

## 3. ``OpenAIProvider.__init__`` rejected ``default_model`` kwarg

``execute_client_advisor`` was passing the raw dict from
``get_provider_config(...)`` straight to the provider constructor.
That dict's shape is the config-file shape (``api_key``,
``base_url``, ``default_model``) which doesn't match the constructor's
keyword args (``api_key``, ``base_url``, ``model``).

Fix: explicit translation — extract only the keys the constructor
accepts and rename ``default_model`` → ``model``. Forward-compatible
with new config fields (they won't crash the constructor).

## 4. Forwarded conversation ended with assistant message

The advisor is invoked from inside a ``tool_use`` block, so the
natural conversation tail is the assistant message that contains
that block. The Vertex-fronted Anthropic API (used by litellm)
rejects this with ``This model does not support assistant message
prefill. The conversation must end with a user message.``

Fix: ``build_advisor_forwarded_messages`` now appends a synthetic
user turn (``CLIENT_ADVISOR_PROMPT_SUFFIX``) asking for advice when
the conversation doesn't already end with a user message. Doubles as
a clear prompt aligned with ``CLIENT_ADVISOR_SYSTEM_PROMPT``.

## 5. tool_use/tool_result blocks in forwarded messages

The advisor is called with ``tools=[]`` (it just emits text). But
proxies reject ``tool_use``/``tool_result`` blocks when no
``tools=`` array is sent: ``Anthropic doesn't support tool calling
without tools= param specified``.

Fix: flatten all ``tool_use``/``server_tool_use``/``mcp_tool_use``
blocks to single-line ``[Tool call: <name>(<input>)]`` text summaries,
and ``tool_result`` blocks to ``[Tool result: <content>]`` summaries.
Preserves the substance of what happened without the typed schema.
Drops ``thinking`` blocks (advisor doesn't need worker's
chain-of-thought separately).

## Verified

Real run output (worker = haiku-4-5 via litellm, advisor = opus-4-7
via litellm same proxy):

  1. Worker called advisor BEFORE writing code
  2. Advisor replied with concrete planning guidance (package.json
     deps, tsconfig settings, router structure, route shapes)
  3. Worker wrote 9 React blog files
  4. Worker called advisor at the end (per the prompt's "verify
     completeness" instruction)
  5. Advisor caught that the first Write batch had empty content
     and instructed the worker to redo with actual file content
  6. Worker redid; final state: 9 working files on disk

Tests: 221 passed (advisor suite + tool_system + settings regression).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@agentforce314
agentforce314 merged commit edbcd7b into main May 20, 2026
singlaamitesh pushed a commit to singlaamitesh/clawcodex that referenced this pull request Jul 7, 2026
fix(advisor): five real-world bugs blocking client-side mode
singlaamitesh pushed a commit to singlaamitesh/clawcodex that referenced this pull request Jul 7, 2026
…fficient /advisor)

Two new entries at the top of the README News section:

1. Codebase stats: 890 files / 183,768 lines (up from 894 / 177,428
   on 2026-05-16; net +6.3k lines / -4 files in five days). The file
   delta is the agent_loop.py consolidation; the line additions are
   the /advisor multi-provider rewrite + status-bar cost work.

2. /advisor mode as a token-efficient coding agent — narrates the
   PR agentforce314#181agentforce314#193 arc as one story:
   - Cheap worker (haiku-4-5) + expensive reviewer (opus-4-7) only
     at decision points ≈ 6× cheaper than opus-only on typical sessions
   - Explicit <provider>:<model> syntax (agentforce314#192)
   - Cross-provider routing verified (deepseek worker + opus advisor
     via litellm, agentforce314#182/agentforce314#184/agentforce314#192)
   - Reviewer-quality prompt (agentforce314#188) — Gaps/Risks/Do-next format
   - Live cost + token visibility in status bar (agentforce314#190/agentforce314#191/agentforce314#193)
   - /advisor slash command + dedicated TUI row (agentforce314#181)

Docs-only — no code or test changes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant