[Refactor & Feature] Harden runtime reliability and recovery safety: side-effect-gated recovery, centralized logging, and architecture contract tests - #24
Merged
Conversation
- engine: treat an empty successful LLM response as a failure signal (one bounded retry with an explicit nudge, then a transparent degraded message) instead of emitting a fake-success filler - daemon: stream a degraded status chunk when deferred init times out or fails, so warm-up degradation is visible to the client - logging: add leapflow.logging_setup as the single owner of process log configuration (idempotent, always redacting); wire CLI/TUI, leapd, observer and dashboard entry points through it - config: add daemon.log_level (default INFO) for leapd file-log evidence; runtime.log_level now defaults to WARNING to match interactive behavior - tools: persist the ripgrep provisioning attempt per profile so a failed install never re-spawns the installer on restart; code_search accepts batched patterns (OR-combined) to reduce process spawns
Brings ruff from 162 findings to 0 and re-enables the CI workflow, which had been fully commented out (no lint or test gate on push/PR). Beyond mechanical cleanup: - recovery_audit: import the envelope/decision/budget types at runtime (no cycle) so create_audit_entry's annotations stay resolvable for typing.get_type_hints(), which previously raised NameError - scheduler triggers: drop the redundant trigger_type ClassVar shadowed by the property required by the Trigger protocol (plus the now-moot type: ignore[override]) in cron/interval/event/condition - F821: declare annotation-only names under TYPE_CHECKING, keeping hub.client / gateway.connectors.protocol free of import cycles - preserve side-effecting probe calls (hub pull, DuckDB catalog probe, event-loop detection) while dropping their unused bindings - tiler: keep the ImageFont import (noqa) since it is part of the PIL availability probe - pyproject: pin the ruff lint select set so an upgrade cannot silently change what is enforced
The board opened and the status bar showed a watch, but the page stayed empty: conversation state lives on per-session engines from SessionRegistry, while get_history() read ctx.engine — the base engine, which is only a template for building them and never carries a conversation. The session-analysis watch therefore analyzed an empty transcript and produced nothing to render. Verified: even the first (primary) session does not reuse the base engine, so its working memory and session id stay empty. - session_coordinator: add resolve_session_engine() and resolve history through it (requested session -> most recently active -> base engine for in-process mode, which is unchanged) - session_registry: add read-only get()/most_recent() lookups that never materialize an engine; correct the module docstring, which still claimed the first session reuses the base engine - /board: bind the caller's session id into the session-watch params and thread it through command_execute -> producer -> session_history so several TUIs sharing one daemon each analyze their own conversation - session producer: forward the bound id, tolerating hosts that predate the session-scoped signature Slash-command behavior change confirmed by the maintainer per AGENTS.md.
The existing rule enumerated structural edits (add/remove/rename/reroute, completion, rendering). The /board defect fell outside that list: the command name, arguments, and help text were unchanged while what it observed changed, so the rule read as not applicable. - state that a functional change counts even when the command surface is unchanged (what it observes/targets/arms/schedules/sends/opens/persists, or which session/workspace/profile it resolves against) - bring command_execute's RPC signature and parameter plumbing in scope - record that passing tests and clean lint do not substitute for the check - require naming the pending confirmation and the behavior to exercise in the handoff
test_fragmented_english_single_line_paste_compacts failed intermittently under full-suite load while passing in isolation. Root cause: fragment detection is time-based (PASTE_FRAGMENT_WINDOW_S = 80ms between inserts). The tests fed 3600 chars as ~225 real-time inserts, so a single scheduling stall over the window split one paste into two: the compactor restarted and plaintext leaked into the visible buffer, which is exactly the observed failure (plaintext head, marker only at the tail). Reproduced deterministically by injecting one 100ms stall. Fixed in the tests rather than by widening the window: 80ms is an interaction design value separating a paste from fast human typing, so enlarging it to suit a test would regress the product. A shared _paste_fragments() helper now drives a stepped fake clock, stating the intent that the fragments form one paste and keeping the assertion about compaction instead of machine speed. The same latent sensitivity in the Chinese-paste test is fixed too. Verified the assertions still bite: sabotaging the compaction threshold makes both tests fail. Four full-suite runs plus three runs under eight busy-loop processes are green.
Four restart-required settings did not tell the user a restart was needed: request_ledger_max_entries and request_ledger_ttl_s had no hand-written description at all and fell back to the generated "Configure <key>." string, while max_live_sessions and session_idle_ttl_s described their effect without mentioning the restart. A user edits the value, sees no change, and has no way to find out why from `leap config show`. Found by the new config catalog contract test.
Audit of the suite found the weakness was not missing tests but tests asserting
the wrong thing: implementation snapshots that break on refactor while proving
nothing, and no coverage at all for several contracts AGENTS.md states as hard
requirements.
Removed (snapshot / too fine-grained):
- two tests that counted substrings in Context.initialize_deferred's source
("await asyncio.sleep(0)" >= 6, "_run_deferred_db" >= 6). Both files already
assert the real contract behaviorally: a concurrent task stays scheduled, and
status() answers while the DB worker blocks.
- 18 per-strategy assertions that copied priority / applicable_sources /
applicable_categories constants back out of the implementation, plus a
len(strategies) == 8 count. Replaced by a table-driven routing test that runs
11 (source, category) cases through a real coordinator over the real registry
and asserts which strategy wins — a stronger guard, since a mis-registered
strategy now fails where a constant copy could not.
Added (contract-level, cross-module):
- test_architecture_contracts.py: gateway core imports no platform package or
vendor SDK; ExecutionBackend and BackendEventSource stay disjoint
(transport-lifecycle separation); domain types are frozen and reject
mutation; extension points are runtime_checkable Protocols; key modules
import standalone.
- test_recovery_contract_e2e.py: the documented pipeline through a real
coordinator — every decision explainable and attributed, outcome feedback
closes the loop, and per-category / global / deadline budgets each halt
deterministically and idempotently.
- test_config_and_path_contracts.py: the catalog contract over all 254 writable
fields at once (previously each feature asserted only its own key), secrets
are refs and profile-scoped, managed paths stay under the layout root,
profiles are mutually isolated, and the vault sits outside config/.
Two gaps are recorded as strict xfail rather than allow-listed, because both
need a human decision and would otherwise stay invisible:
- Side-Effect Gating is specified but not implemented: side_effect_state is set
by unified_classifier and never read by the coordinator or any strategy, so a
tool that already applied an effect is retried automatically. Note that
'external_side_effect' maps to UNKNOWN, so outbound sends are ungated too.
- gateway/validators.py keeps three vendor implementations (hardcoded Feishu /
DingTalk / Telegram endpoints and their differing error JSON shapes) in core.
Relocating them is risky as-is: validate_credentials() returns (True, "") for
an unregistered validator, so a registration-timing change would silently
skip credential validation.
Each new assertion was verified to bite by sabotaging the implementation it
guards. Suite: 1197 -> 1257 passing, ruff clean.
…tered ones gateway/validators.py mixed the platform-neutral registry with three vendor implementations that hardcoded Feishu/DingTalk/Telegram endpoints and their differing error shapes (msg / errmsg / description), which the Platform vs App Business Boundary forbids in core. Relocating them naively would have been unsafe: configure_platform() validates credentials *before* it builds the adapter, so a validator registered from an adapter module would not exist yet, and validate_credentials() returned (True, "") for an unregistered name — the check would have been skipped silently. Instead the module becomes a package whose __init__ keeps the neutral registry and imports the vendor modules eagerly, so registration timing is unchanged while vendor code sits in validators/<platform>.py, mirroring how adapters/ and normalizers/ are organized. Also: - an unregistered-but-named validator is now an error, not a pass: silently storing unverified credentials surfaces much later as an opaque failure. An empty method still means "manifest opted out" and passes. - dropped _feishu_token_check: feishu.yaml declares validation.method: "", so it was never reachable. The architecture guard for this contract drops its xfail (it was XPASS(strict)).
…uest Side-Effect Gating was specified in AGENTS.md but only half built: the batch already stopped after a failed side-effecting call (policy-driven, engine.py:312), while FailureEnvelope.side_effect_state was set by unified_classifier and then never read. Neither the coordinator nor any strategy consulted it, and the InteractionRequest that a terminal decision may carry was dropped on the floor. Three changes, one per layer: 1. Uncertain effects are reported instead of retried blindly. A failed call under external_side_effect or mutating_once now carries side_effect_uncertain plus retry_guidance, because an error is not proof that nothing happened — an outbound send can time out after delivery. This matters most for the path that never reaches the coordinator: recoverable tool failures are fed back to the model (engine.py:1401-1422), so the model is who decides whether to repeat the call, and it needs to know the effect's fate is unknown. mutating_idempotent is exempt: re-applying it converges, so flagging it would only stall safe retries. Both allow-lists that filter tool metadata were updated, otherwise the fields would have been stripped before the model or the transcript saw them. 2. RecoveryCoordinator now gates replaying actions (retry / transform-and-retry / failover) whenever side_effect_state is anything other than NONE, and returns HALT_WITH_CHECKPOINT carrying an InteractionRequest. UNKNOWN blocks like COMMITTED and PARTIAL do: it is the classifier's fallback *and* what external_side_effect maps to, so exempting it (the literal reading of the old contract text) would have left outbound sends ungated — the highest-risk case. Being blocked consumes no budget: it is not an attempt. 3. Terminal decisions surface their InteractionRequest. ASK_USER previously fell into the else branch and only set fatal_error = decision.reason, so a typed prompt with suggested actions and a resumption key degraded into an audit-log sentence. All five terminal sites now render the request for the user and pass the structured payload on the stream event. AGENTS.md updated to match: the gating rule now states both levels and why UNKNOWN is included, adds the uncertain-effect reporting rule, requires terminal decisions to surface their InteractionRequest, and notes that per-vendor code (validators included) belongs in a platform sub-package. The four strict xfails become real assertions. Every new guard was verified to bite: exempting UNKNOWN, and dropping the interaction, each fail the tests that cover them. Suite 1257 -> 1276 passing, ruff clean.
… redaction Deep review of ab2b60a/c754198 (per AGENTS.md: runtime behavior + gateway structure + safety changes require one) found three real gaps, all fixed: 1. HALT_WITH_CHECKPOINT only persisted a checkpoint on one of five terminal dispatch sites, and the saved checkpoint carried nothing linking it to the InteractionRequest shown to the user. Both were latent until now — before the side-effect gate, nothing ever emitted HALT_WITH_CHECKPOINT — but the gate emits it from any path, so a streaming halt saved no checkpoint and the prompt's resumption_key pointed at nothing. A shared _save_halt_checkpoint() now runs at all five sites and records the envelope id, the interaction_request_id, and the resumption_key on the checkpoint, so list_pending(session_id) resolves the prompt back to the saved state. A failed save logs and never masks the halt itself. 2. ToolExecutionLedger.duplicate_result() synthesized a fresh payload, dropping the original failure's side_effect_uncertain/retry_guidance — losing exactly the warning that told the model to verify before retrying. The verdict is now lifted from the recorded result. (Annotating the duplicate through _annotate_uncertain_effect would not have worked: duplicates carry counts_as_failure=False and are skipped by design.) 3. validate_credentials() redacted exception text but returned a vendor's own error string verbatim; third-party APIs can echo request parameters back. Failure messages are now redacted centrally before any caller sees them. Also verified during review (no change needed): rollback on a gated candidate is symmetric with commit — decide() does not mutate state directly, phase advancement is coordinator-committed; and the coordinator gate is a no-op for LLM-failure paths, whose side_effect_state is always NONE. Four regression tests: checkpoint linkage fields, save-failure containment, and duplicate-verdict preservation both ways. Suite 1276 -> 1280 passing.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
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.
Feature List
NONEside effect (includingUNKNOWN, which outbound sends map to); halt with a resumable checkpoint and a structuredInteractionRequestsurfaced to the userside_effect_uncertain+retry_guidance, so the model verifies before resending instead of duplicating the effectstatus(), RPC-probing recovery with force restart/boardanalyzes the calling TUI session (fixes the empty board)validators/; unregistered validators rejected instead of silently passing; vendor errors redactedruntime.log_level, bypassed redaction, duplicate handlers; addsdaemon.log_levelInteractionRequestcontracts codified; slash-command behavior changes now require human confirmation