feat(core): Implement Budget-Aware Recovery Subsystem, Tool Idempotency, and Core Stability Fixes#19
Merged
Merged
Conversation
…y design document - Add ToolExecutionLedger for idempotent tool execution with fail-stop semantics - Add scm_tools with typed action semantics (pull/push/status/diff/commit) - Add comprehensive agent loop error recovery design document - Update engine with tool concurrency and context control improvements - Update gateway tool with capability health integration - Update TUI with command queue and stream improvements
…lassifier, and 8 strategies P0: Core type system and coordinator skeleton - FailureEnvelope: immutable failure encapsulation with source, category, recoverability, side-effect state, and provider hints - RecoveryDecision: explicit decision representation with action, reason, retry semantics, and audit metadata - RecoveryCoordinator: unified decision entry point with Strategy Protocol, priority-sorted evaluation, and terminal fallback - RecoveryBudget: global constraint system with per-category limits and turn-level deadline enforcement - OneShotGuard: generalized one-shot deduplication replacing 9 boolean flags P1: Unified classifier, interaction protocol, and 8 built-in strategies - UnifiedErrorClassifier: bridges LLM errors, tool results, and system exceptions into unified FailureEnvelope instances - InteractionRequest: structured user interaction protocol with 10 types, severity levels, suggested actions, and timeout behaviors - 8 recovery strategies (priority-ordered): - ContextCompress (10): 3-phase progressive compression - MultimodalStrip (15): image content removal - ProviderFailover (20): provider/model failover - CredentialRotate (25): credential pool rotation - ThinkingDisable (30): thinking mode disable on format error - NativeToText (35): native-to-text tool calling fallback - ToolSchemaExpand (40): unknown tool schema expansion - JitteredRetry (100): catch-all with category-specific backoff Tests: 125 new tests, 692 total passing, zero regressions.
Critical fixes: - OneShotGuard: add new_turn() for per-turn reset with audit history - RecoveryState: add current_turn_id and turn-aware phase reset - RecoveryCoordinator: add new_turn() method for turn boundary reset Design alignment: - FailureContext: add provider, model, attempt_number, elapsed_ms fields - RecoveryDecision: add interaction, transform_description, failover_target - RecoveryStrategy Protocol: can_apply() now accepts budget parameter SOLID & extensibility: - RecoverabilityRegistry: extensible category→recoverability mapping - ProviderFailover/CredentialRotate: proper budget-aware can_apply() - All 8 strategies updated with budget parameter UX improvement: - Terminal decision reasons include context (strategies tried, budget state) Tests: 692 passing, zero regressions.
…AGENTS.md Recovery system fixes (5 critical issues): - Add 'repeatable' property to Strategy Protocol; non-repeatable strategies (ThinkingDisable, MultimodalStrip, NativeToText, ToolSchemaExpand) are now one-shot guarded to prevent infinite transform loops - Fix type-specific budget consumption: coordinator now calls consume_transform/ consume_failover/consume_rotation after decisions - Fix strategy can_apply budget checks to use can_failover()/can_rotate() instead of category_remaining() which was never consumed - Add system_timeout/system_network to JitteredRetry applicable_categories - Add RecoveryBudget.new_turn() reset called from coordinator.new_turn() AGENTS.md integration: - Architecture Principles: single decision point, side-effect gating, budget-constrained recovery, strategy-as-protocol - Code Quality: structured error propagation via typed envelopes - Implementation Guidelines: coordinator pattern, strategy registration, InteractionRequest for user engagement - Testing: strategy isolation, budget boundary tests - What to Avoid: scattered if/break recovery, unbounded retries, unstructured error text, parallel error handling paths Tests: 698 passing, zero regressions.
P2: Recovery Checkpoint System
- RecoveryCheckpoint: mutable dataclass with lifecycle states
(PENDING → CONSUMED/EXPIRED/CANCELLED), optimistic locking, TTL
- CheckpointStore Protocol with CAS (Compare-And-Swap) semantics
preventing concurrent resumption
- InMemoryCheckpointStore: full implementation with atomic consume,
expiry cleanup, and session-scoped listing
- CheckpointResumer: orchestrates safe resumption with precondition
drift detection (critical vs warning classification)
- Precondition/StepRecord/ResumeResult value objects
P3: Recovery Audit System + Engine Integration
- RecoveryAuditEntry: immutable audit record with envelope/decision/budget data
- JsonlAuditSink: JSONL file backend with graceful degradation on IO failure
- create_audit_entry() factory bridging coordinator outputs to audit entries
- Engine integration (non-stream _unified_tool_loop):
- RecoveryCoordinator created per-turn with default_strategies()
- LLM errors classified via UnifiedErrorClassifier → coordinator.evaluate()
- Decision dispatch: RETRY_WITH_BACKOFF, TRANSFORM_AND_RETRY, FAILOVER,
HALT_CLEAN, HALT_WITH_CHECKPOINT all handled
- Tool failures classified and routed through coordinator
- Audit entries recorded for every recovery decision
- Checkpoint saved on HALT_WITH_CHECKPOINT
- Legacy _handle_api_error marked deprecated (retained for stream loop)
Tests: 755 passing, zero regressions.
…integration issues Critical: - Remove 'if coordinator is not None' legacy fallback — coordinator always present - No more dual code paths; old _handle_api_error fully deprecated High: - Stream loop (_unified_tool_loop_stream): all 3 exception handlers now use unified coordinator pattern, matching non-stream loop - Fix premature on_strategy_outcome for RETRY_WITH_BACKOFF — outcome reported only after actual execution result Medium: - Wire 3-phase compression correctly: _execute_transform_decision() reads phase from audit_metadata and applies phase-specific action - Enrich checkpoint data: full envelope fields + context_data with tool state and budget position - Fix to_json_dict: no longer strips turn_id=0 or budget fields at zero - Add try/except guard around coordinator.evaluate() for graceful degradation All 6 flows verified: LLM retry, budget exhaustion, permission hard-stop, 3-phase compression, checkpoint save, and audit trail completeness. Tests: 755 passing, zero regressions.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a structured, budget-aware, and strategy-based recovery subsystem for the agent loop, featuring a RecoveryCoordinator, RecoveryBudget, UnifiedErrorClassifier, and several pluggable recovery strategies. It also implements a centralized ToolExecutionLedger for tool idempotency, adds a structured scm_sync tool, and updates the TUI to skip duplicate commands. The review feedback highlights several critical issues, including a state machine flaw in the checkpoint system that transitions checkpoints to CONSUMED before verifying preconditions, potential state corruption in the coordinator, and double-counting of consecutive failures. Additionally, there are memory leaks from unpurged expired checkpoints and unbounded audit entries, a TypeError in the classifier, and a signature-checking vulnerability in the daemon service.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Critical fixes: - Checkpoint state machine: split load_and_consume into load() + consume() with precondition verification between phases. Failed preconditions now increment resume_attempts keeping state PENDING (not prematurely CONSUMED) - Coordinator state corruption: remove state mutation from strategy.decide(), add _commit_state_changes() called only after budget check passes High severity: - JitteredRetry: use empty frozenset() as wildcard for applicable_categories, real gating via can_apply() recoverability check (supports future categories) - Double-counting consecutive_failures: removed from _update_state(), only incremented in on_strategy_outcome(success=False) Medium severity: - UnifiedClassifier: fix ErrorClassifier.friendly_message class-method call to use instance self._classifier - Daemon service: replace fragile TypeError string matching with inspect.signature() pre-check for request_id parameter - Checkpoint store: pop expired entries from dict on load (prevent memory leak) - Audit sink: use deque(maxlen=1000) for bounded in-memory buffer - Classifier retryable: handle None value correctly (default to True) Tests: 755 passing, zero regressions.
wangxingjun778
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1. Robust Agent Recovery Subsystem
RecoveryCoordinatorandRecoveryBudgetto govern recovery strategies within strict resource boundaries.UnifiedErrorClassifieralongside pluggable, strategy-based recovery behaviors for adaptive failure handling.2. Execution Safeguards & Tooling
ToolExecutionLedgerto guarantee single-execution semantics and prevent redundant tool invocations.scm_synctool for reliable, structured source control operations.3. Core Stability & Security Hardenings (Addressing Review Feedback)
CONSUMEDstate.TypeErrorin the error classifier, and prevents potential state corruption in the coordinator.