Repository navigation
fix(routing): restore request receipts and close consumed proxy responses - #1213
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
exact-head acceptance @ fea207fc459be6c11b09ee399e4102452b23d23f: RED — conduct() can emit a selection receipt for the wrong served deployment after failover. The new code already recognizes the case served_id != agent.id and records served_agent_id / failover_from, but immediately constructs selection_design with selected=agent: self._selection_design_receipt([agent], attempted, agent). _selection_design_receipt hashes the selected object into selected_deployment_id, so a successful fallback can be persisted as though the original failed agent was selected. route_once() handles this correctly by resolving attempt_served_id back to the actual served agent before building the receipt.
Please add a fail-first conduct() regression where the role's first agent fails and _invoke_with_rate_limit_recovery serves a different configured agent. Assert that row.served_agent_id, selection_design.selected_deployment_id, and the final attempted deployment all identify the same served deployment/revision, and that the failed original remains only in candidate/attempt history as appropriate. Then use the same served-agent resolution rule as route_once() (or one canonical helper) rather than passing the pre-failover agent. Also pin the case where the served ID cannot be resolved: do not silently mint a receipt for the wrong deployment; fail closed or preserve an explicit unresolved identity according to the owner contract.
Because these receipts are intended as psychometric/routing provenance, this is a semantic evidence defect, not formatting. Current verdict: request-scoped receipt restoration PASS candidate; conduct failover selected-identity FAIL; current TDD coverage for that path FAIL.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Independent agent review (comment only; this is not an APPROVED review and does not satisfy the nonauthor approval gate).
Actionable finding on the actual stacked diff 898a7cb97fac7b10d35e7ca6e5e0d2484d3a29cf..fea207fc459be6c11b09ee399e4102452b23d23f:
[P1] Keep conduct selection attempts scoped to each workflow step — contextual_orchestrator/orchestrator.py:9812-9850. _REQUEST_SELECTION_ATTEMPTS is request-wide, but unlike route_once this loop never snapshots the list length before _invoke_with_rate_limit_recovery; every row therefore includes all earlier roles as attempts. On this exact head, a four-call template conduct produced thinker=[planner], worker=[planner,builder], verifier=[planner,builder,reviewer], synthesizer=[planner,builder,reviewer,planner]. Those are false per-assignment receipts and will contaminate psychometric evidence; also resolve selected from served_id so a failover receipt does not identify the original candidate. Capture a per-step start index and build the receipt from that slice, as route_once already does.
Focused tests passed: nested judge/receipt, proxy HTTPError cleanup (both cleanup branches), and three catalog snapshot cases -> exit 0, 7 passed. A minimal conduct assertion requiring each row to contain only that step’s served attempt failed with exit 1 at the worker row: (worker, [planner_agent, builder_agent]). I did not run the full suite or rebuild native components; the stack also inherits a separately reported Ruff F821 blocker predating this diff.
|
Independent review findings are reproduced and repaired in #1214 (467d8cd): per-step attempt slices, actual served deployment, and unknown-identity rejection. Three new regression cases failed before repair; 104 related strict tests pass afterward. A separate exact-head independent review of #1214 is assigned on MacBookAir. This does not resolve the findings on the unchanged #1213 head or substitute for protected integration and nonauthor approval. |
The 10 failures left after merging #1213 are covered by open PR #1088's commit e569cef ("restore hashable install and dropped routing contracts"). Port only the minimal hunks for them; #1088 is not merged whole because its install/CI files (requirements.lock, pyproject.toml, uv.lock and the CI wheel bootstrap) duplicate and conflict with this PR's and with #995's pins. Ported from e569cef: - bind psychometric evidence to the deployment-configuration candidate id under one persistence lock, and prune the store with it; - retain evidence only for the pool's current deployments on reload, on discovery refresh and on ledger forget (_retain_psychometric_candidates); - rank candidates through the evidence id instead of the bare agent id; - test_orchestrator_client_boundaries: the passthrough fake response now implements the context manager, read(size) and headers.get that main's _read_bounded_response actually calls (stale test double, unchanged assertions); - test_provider_catalog_bootstrap: expect agent_id_for(...) instead of a pre-fingerprint literal id (main's id scheme appends a model fingerprint), with the assertion just as strict. Not ported: e569cef's _allowlist_upstream_error, which #1212 already covers, and its golden-hash edit, which #1213 already carries. The hash changed only because #1053 added model_timeout_seconds and model_timeout_revision to ModelAgent.to_config(); dropping exactly those two fields reproduces the pre-restack value, and a deployment timeout change must change the deployment identity. Required fix on top of e569cef (CO lead reproduced it RED): _observe_contextual_quality called self._agent(served_id), which raises KeyError when the served agent has left the pool or is filtered by _agent_matches_request_endpoint, so a pool refresh during judging failed a request that already had its answer. The served agent is now looked up in self.candidates without the endpoint filter, and a departed agent skips the observation -- _retain_psychometric_candidates would discard that evidence anyway. Covered by test_departed_served_agent_observation_is_skipped_not_raised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FvosNg4GVUjaV5UfrimrsX
|
Admission correction — exact current head |
90fda79
into
seonghobae/fix-1016-route-once-typed-attempts
1 participant
The remaining route_once failure was a missing selection_design implementation, not an invalid expectation. Restore the existing request/receipt code from #1088 commit e569cef, preserving request policy and effort snapshots across nested calls. Record worker attempts before the judge so auxiliary calls do not enter its receipt. This selectively reuses existing implementation without copying dependency, learned-ranking, or persistence changes.
Expanded validation exposed a second production ownership gap: the virtual proxy converted raw HTTPError responses but never closed them. Close after diagnostics in finally, before failover, preserving the classified outcome if cleanup raises Exception. A 10ms cooldown regression now uses a controlled clock matched to its sleep hook.
Stacked on #1212; #1210–#1212 heads remain unchanged. Three regression cases fail on unchanged #1212. Final strict verification: 179 passed, exit 0; four selection-identity tests separately pass. Coverage includes original KeyError, no-judge-contamination, response-close-before-failover and cleanup failure, request isolation, streaming/race receipt cases, and prior zero-upstream-send allowlist tests.
No native build: isolated source environment uses binary numpy 2.5.3/fast-mlsirm 0.11.3 solely to collect identity tests. Not complete-suite or locked-package acceptance. The runbook records failed intermediate runs, ownership and scope. Hosted gates and independent review remain required.