Skip to content

refactor(ai): one side-effect check, one fallback executor, shared entry steps - #24

Merged
scarletkc merged 4 commits into
mainfrom
refactor/ai-unify-duplicated-flows
Nov 8, 2026
Merged

scarletkc merged 4 commits into
mainfrom
refactor/ai-unify-duplicated-flows

Conversation

@scarletkc

@scarletkc scarletkc commented Nov 8, 2026 •

Copy link
Copy Markdown
Member

Part of #23 (§1, §5, §6). The issue found the same AI decisions written several times over; this PR gives each of them one implementation. It is three commits that can be reviewed one at a time.

Problem

  • Whether a turn already had side effects was decided in four places: complete() looked at tool_logs, the post-tool retry looked for a tool_result, the router probed four attributes on the visible-content handler, and the idle follow-up looked at tool_logs again. The answer depended on where an exception was raised (§1).
  • Provider × model fallback was written four times: the router plus run_chat_provider's fallback model, run_ai_task, the summary agent and the recap agent. They disagreed on what stops a fallback, on capability checks and on how the tool request context is set and cleared (§6).
  • The user turn, the scheduled task and the idle follow-up each built the user-state prompt, the tool context and the steps after the model call on their own (§5).

The re-run in §1 was a static inference in the issue. It is now reproduced: with a fake provider whose second call returns no choices, update_impression ran three times on main (Gemini primary model, Gemini fallback model, next provider) and the user got the "all services failed" notice. On this branch it runs once, and the user gets the "failed after tool execution" notice with the tool call and result kept in history.

Change

1. fix(ai): stop re-running tools after a failure later in the same turn (§1)

  • types.has_side_effects(tool_logs) is the only check: a tool was called or visible content was sent. run_tool_loop wraps every exception after that point in PartialAIResponseError, not only model errors, and the in-place retry after tools uses the same check.
  • The router no longer probes the handler. delivered_to_user(tool_logs) decides whether the partial-failure notice is needed.
  • Tool results reach the model with the same JSON serialisation as history (default=str). A response without choices raises a clear error instead of IndexError.

2. refactor(ai): one provider and model fallback executor for every AI task (§6)

  • task_runner.run_with_fallback(task, attempt, *, tools_are_read_only, circuit, deadline) is the only loop. The caller says what one attempt does: run_ai_task makes one model call; chat, summary and recap run a tool loop. The executor owns the order, the skips (unknown provider, missing capability, no model, open circuit), the failures that must not fall back (context limit anywhere in the cause chain, turn deadline, a failure after side effects), safety blocks as declared by the provider, and the circuit breaker (ProviderCircuit; CHAT_CIRCUIT for chat).
  • chat_provider.py is removed. The router's _run_chat_candidate runs one attempt in its own tool_request_scope and turns the outcome into the same fixed notices as before.
  • Summary and recap declare requires=("tools",) and pass tools_are_read_only=True, so they still re-run after a failure past their read-only tools.
  • completion_kwargs_for_task always returned {} and is removed.

3. refactor(ai): share the steps the three AI entries each wrote on their own (§5)

  • user_state: one formatter and one reader for the impression and diary flags; the diary query is no longer duplicated.
  • tools.context.build_tool_context defines the tool-context keys once. Background turns get the chat fields as None, so delegated commands stay unavailable there.
  • outbound.finish_reply (strip [no_response], recognise the fixed error notices, check sticker directives) and outbound.send_private_reply.
  • tool_history.reply_record_entries: the final reply as a redacted history entry.
  • history_writer.write_history: insert, then send archived snapshots, refresh the summary on overflow or queue one for a new snapshot. summary.refresh_summary_after_overflow is the single overflow rule and also backs history_hooks.handle_history_overflow.

The job_claims stages (claimed → generating → delivering) and their recovery rules are unchanged.

Behaviour changes

All of them are on failure paths or in the two background entries. Commands, charging (still no refunds after charging), job stages and what a successful user turn writes to history are unchanged.

  • A failure after tools no longer re-runs the turn on the fallback model or the next provider. When the user had already seen visible content, the tool logs are now kept instead of dropped.
  • After visible content alone (no tool result yet), a transient model error is retried in place. Before, only a tool result enabled that retry.
  • An unknown or unconfigured chat provider is skipped instead of counting against its circuit. A safety block on Gemini's fallback model no longer counts as a circuit failure. An expired turn deadline is also checked before trying a fallback model.
  • Summary and recap stop at the context limit instead of trying every model, as chat and run_ai_task already did.
  • A sandbox cleanup failure is logged instead of being treated as a provider failure, which used to re-run the turn.
  • Scheduled and idle follow-up replies are redacted before they are written to history, as the user turn already did. An idle follow-up that overflows the history refreshes the summary right away, like every other writer.

Issue #23 checklist

  • §1: all three items. The unserializable-result case now continues the turn instead of failing it; the tool still runs once.
  • §5: one constructor each for tool_context and the user-state prompt; job_claims stages unchanged and both recovery suites pass. Only partly done: the background entries now share the steps but keep their own control flow. Moving them into ConversationTurn behind an entry policy is the stage 4 PR.
  • §6: one executor for chat, run_ai_task, summary and recap; completion_kwargs_for_task removed; summary and recap require tools. Still open: whether other tasks get a circuit breaker. The executor accepts one; only chat passes it.

Not in this PR: §2 skip_tools, §4 ToolSpec (including the four rate-limit implementations and the hard-coded media tool names), §7 metering, §8 the delegated-command allowlist, §9 and §10 pricing.

Tests

  • uv run pytest --run-slow: 1333 passed.
  • tests/integration against a local MySQL 8.4: 648 passed, including test_schedule_recovery.py and test_idle_followup_recovery.py.
  • uv run ruff check ., uv run mypy, uv lock --check, uv run fogmoe-telegram-bot --check: clean. task_runner, user_state, outbound, history_writer and tools/context are added to the mypy files.
  • New tests:
    • test_ai_router.py::test_a_failure_after_tools_never_reruns_them: the §1 scenario end to end.
    • test_tool_runner.py: errors after a tool round or after visible content leave as PartialAIResponseError; errors before any side effect are raised unchanged; an unserializable result reaches the model as text.
    • test_ai_task_runner.py: order, skips, failures that stop the fallback, read-only agents, safety blocks, circuit, turn deadline.
    • test_async_execution.py::test_an_expired_turn_gets_its_notice_even_when_every_provider_is_skipped: an expired turn still gets the timeout (or restart) notice when every remaining provider is skipped (review fix in fdaa08a).
    • test_ai_turn_steps.py: the shared steps, including redaction of background replies.
    • test_chat_provider.py is removed; its cases now run against the executor in test_ai_router.py.

Docs: ai-provider-architecture.md has a new 「回退规则」 section and a rewritten 「主聊天的 fallback」; runtime.md, architecture.md, ai-conversation-flow.md, job-recovery.md and sensitive-data.md follow the code.

The tool loop only wrapped errors from the model call itself in
PartialAIResponseError. Any other exception after a tool had run (for
example a response without choices) left the loop as a plain error, so the
Gemini fallback model and then the next provider re-ran the whole turn from
the original messages and executed the tools again. With a fake provider,
update_impression ran three times and the user got the "all services
failed" notice.

Whether a turn already has side effects is now decided in one place,
types.has_side_effects (a tool was called or visible content was sent):

- run_tool_loop wraps every exception after that point, not only model
  errors, so no caller can restart the turn;
- the in-place retry after tools reads the same predicate;
- the router no longer probes four attributes of the visible-content
  handler; delivered_to_user tells from the logs whether the user already
  saw something;
- the idle follow-up's "safe to retry" check reads it too.

Tool results reach the model with the same JSON serialisation that history
uses (default=str), and a response without choices raises a clear error.

Refs #23
The provider x model fallback was written four times: the chat router plus
run_chat_provider's fallback model, run_ai_task, the summary agent and the
idle recap agent. They disagreed on what stops a fallback (only run_ai_task
and chat stopped at the context limit), on capability checks, on how the
tool request context was set and cleared, and only chat had a circuit
breaker.

task_runner.run_with_fallback is now the only loop. Callers pass what one
attempt does: run_ai_task makes one model call; chat, summary and recap run
a tool loop. The executor owns the order, the skips (unknown provider,
missing capability, no model, open circuit), the failures that must not
fall back (context limit anywhere in the cause chain, turn deadline, a
failure after side effects unless the agent's tools are read-only), safety
blocks as declared by the provider, and the circuit breaker, now a
ProviderCircuit instance (CHAT_CIRCUIT for chat).

- chat_provider.py is gone. The router's _run_chat_candidate runs one
  attempt in its own tool_request_scope and turns the outcome into the same
  fixed notices as before. A sandbox cleanup failure is logged instead of
  being treated as a provider failure, which used to re-run the turn.
- summary and recap declare requires=("tools",) and pass
  tools_are_read_only=True: they still re-run after a failure past their
  read-only tools, and now stop at the context limit like everything else.
- completion_kwargs_for_task always returned {} and is removed.

Behaviour changes, all on failure paths: an unknown or unconfigured chat
provider is skipped instead of counting against its circuit, a safety block
on Gemini's fallback model no longer counts as a circuit failure, and an
expired turn deadline is also checked before trying a fallback model.

Refs #23
…r own

The user turn, the scheduled task and the idle follow-up each built the
user-state prompt, the tool context and the steps after the model call
themselves. Each now uses one implementation:

- user_state: one formatter and one reader for the impression and diary
  flags. The turn passes the balances from its charge, the background
  entries read the user row; the diary query is no longer duplicated.
- tools.context.build_tool_context defines the tool-context keys once.
  Background turns get the same keys with the chat fields set to None
  (every consumer reads them with .get), so delegated commands are still
  unavailable there.
- outbound.finish_reply strips [no_response], recognises the fixed error
  notices and checks sticker directives; send_private_reply sends a
  background reply in the right history scope.
- tool_history.reply_record_entries turns the final reply into a history
  entry: redacted, nothing for empty replies or error notices.
- history_writer.write_history inserts, then sends archived snapshots,
  refreshes the summary on overflow or queues one for a new snapshot.
  summary.refresh_summary_after_overflow is the single overflow rule and
  also backs history_hooks.handle_history_overflow.

Behaviour changes, background entries only: their final reply is now
redacted before it is written to history, as the user turn already did
(docs/sensitive-data.md only listed the user turn), and an idle follow-up
that overflows the history refreshes the summary right away like every
other writer instead of only queueing one.

Refs #23
devin-ai-integration[bot]

This comment was marked as resolved.

…is skipped

The chat deadline was only checked inside an attempt, so a provider that
run_with_fallback skipped (open circuit, no model, unknown name) never
reached the check. If the deadline had passed and every remaining provider
was skipped, the turn ended with the "all services failed" notice instead
of the timeout notice, or instead of the restart notice during shutdown.
On main the router checked the deadline before each provider's circuit.

run_with_fallback now takes the turn deadline and checks it before looking
at each provider and before each model, raising
TurnDeadlineError(phase="fallback") without touching the circuit. The
per-attempt check in _run_chat_candidate is gone.

Reported in #24 (comment)

Refs #23
@scarletkc
scarletkc added this pull request to the merge queue Nov 8, 2026
Merged via the queue into main with commit 1548234 Nov 8, 2026
6 checks passed
@scarletkc
scarletkc deleted the refactor/ai-unify-duplicated-flows branch November 8, 2026 09:45
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