coordinator: actionable rejections + outcome disposition (fault tolerance) - #136
Conversation
…ance) Make the semantic coordinator boundary tolerant of imperfect invocation and self-describing about every outcome, so a caller recovers in place instead of re-deriving a request or misreading a transient failure as a provider outage. - Rejections carry a specific error_code + bounded, actionable detail (field, constraint, admitted values or required source) instead of a bare invalid_request. detail never reflects unbounded/untrusted input. - Every non-usable response carries a deterministic disposition (fix_request/retry/inspect/unavailable) + recovery hint; provider_error, teardown_error, and protocol_error are never classified unavailable. - One in-place recovery: empty target_agent coerced to null (recorded in normalized). Nothing cost/depth/security-relevant is rewritten; the closed-request, fail-closed posture is unchanged. - README timeout_ms bound corrected to the enforced 1-600000 (Addressed: #125). Additive response fields only; 35 coordinator tests (16 new regression cases from the historical failures) + full suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lback inspect) Distinct-family review (Grok, xai) REQUEST_CHANGES round 1, both integrated: - ADVERSARIAL: detail echo filter admitted C1 controls (U+0080-9F) and arbitrary Unicode; restrict _field_name/_bounded_reason to ASCII-printable (0x20-0x7E) so 'detail' never reflects raw untrusted input. Also cap the missing/unexpected key lists (_bounded_key_list) so a hostile key set cannot bloat a rejection payload. - CORRECTNESS: the un-pre-classified generic ValueError fallback (repo_root / documents deep validators) mapped to disposition fix_request, mislabeling an environmental 'repo_root is unavailable' as 'adjust the request'. It now maps to 'inspect' with an honest recovery hint; the caller reads detail.reason. +3 regression tests; 38 coordinator tests + full suite (196) green. Verified live: hostile logical_action echoes ASCII-only; malformed documents -> disposition inspect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fee938136a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if normalized: | ||
| response["normalized"] = normalized |
There was a problem hiding this comment.
When an empty target_agent is normalized and the runtime then returns any non-usable status, this branch attaches normalized only to successful/advisory responses. The failure response therefore hides that the runtime actually received an untargeted request, which can mislead diagnosis or a subsequent retry and contradicts the advertised recorded normalization. Attach the normalization record to both usable and non-usable runtime responses.
Useful? React with 👍 / 👎.
| if document.get("operation") != "readiness": | ||
| raise ValueError("coordinator readiness operation is invalid") | ||
| raise _InvalidRequest( | ||
| "readiness_operation_invalid", | ||
| {"field": "operation", "admitted": ["readiness"]}, |
There was a problem hiding this comment.
This actionable error is unreachable through process(), because that function selects validate_readiness_request() only when operation == "readiness". For example, a readiness-shaped request with operation: "readines" is sent to validate_request() and reports unrelated missing routed-request fields instead of readiness_operation_invalid, so the caller is not told which field to correct. Dispatch requests carrying the readiness operation field through this validator or classify the invalid operation before dispatch.
Useful? React with 👍 / 👎.
1 participant
Coordinator fault tolerance: actionable rejections + outcome disposition
Investigated the recurring coordinator failures recorded in the workspace
.learnings/ledger and this repo's issues, and fixed the two dominant classes at the one authoritative, unsealed boundary (coordinator.py); no signed-runtime change.The evidence (two failure classes).
{"status":"invalid_request","error_code":"invalid_request"}with no field-level reason, so a caller could not self-correct without re-deriving. The smoking gun:timeout_ms: 900000(over the enforced 600000 cap) was rejected identically to a structurally broken request, while600000was accepted (ledgercoordinator.timeout.ms.over.cap.bare.invalid.request; issue coordinator: README timeout_ms bound (86400000) exceeds the enforced 6.1.1 validation ceiling #125). The README also documented the cap as1-86400000while the code enforced600000.teardown_error/provider_error/protocol_error; which the contract itself defines as attempt-local, not route unavailability; and concluded "the reviewer is down". The overloaded codes gave no way to tell a transient hiccup from a deterministic contract rejection (ledgermanaged.reviewer.invocation.error.retry.not.outage).The fix (additive, fail-closed preserved).
error_code+ bounded structureddetail(field, constraint, admitted values, or required source).detailnever reflects unbounded or untrusted input back to the caller (scalar-typed + length-bounded).disposition(fix_request/retry/inspect/unavailable) + a shortrecoveryhint, table-driven from the closed runtime status. By constructionprovider_error,teardown_error, andprotocol_errorare neverunavailable; the invariant that stops an invocation error being read as an outage.target_agentis coerced tonull(recorded innormalized). Over-captimeout_msis rejected actionably (not silently clamped; per the cross-check's reliability ruling). Nothing cost/depth/security-relevant is rewritten; the closed-request, fail-closed posture and the admitted-request set are unchanged.timeout_msbound corrected to the enforced1-600000(Addressed: coordinator: README timeout_ms bound (86400000) exceeds the enforced 6.1.1 validation ceiling #125). New response fields documented.Verification. 35 coordinator unit tests (16 new regression cases built from the historical failures, including the exact over-cap and attempt-local-not-outage cases) + full plugin suite (193 tests) green. Live end-to-end against the signed runtime: the historical
timeout_ms:900000request now returnserror_code: timeout_ms_over_cap,disposition: fix_request,detail{max:600000,given:900000}.Reliability/simplicity. Pure functions + one closed lookup table; no new service, state, retry loop, or provider change. Additive response keys only; consumers keying on
status/error_codeare unaffected; an unknown future status defaults toinspect(never a silent "outage").🤖 Generated with Claude Code
author: claude
standing_directives: operator direction (investigate + fix recurring coordinator false-flag and bad-invocation failures; reliable/non-brittle/simple/elegant baseline); dir #6 Tier-3 contract boundary; dir #8a cheapest-capable delegation; adversarial-architecture-review (input-validation boundary)
tier: 3
cross_check: Two distinct families beyond the author (claude/Anthropic). Plan stage: Gemini (Google) dual-lens (correctness + adversarial) verdict PROCEED-WITH-MODIFICATIONS (drop the timeout auto-clamp for an actionable rejection; strictly bound detail.given), both integrated. Implementation stage: Grok (xai, distinct from author and cross-check) review, round 1 REQUEST_CHANGES CONFIDENCE H (C1/Unicode echo in detail; environmental repo_root mislabeled fix_request), both integrated; round 2 VERDICT APPROVE CONFIDENCE H with a succeeded execution receipt (request_id coord-grok-docs-review-2). The repo-grounded review.repository route hit an environmental protocol_error/source_seal_failed on the linked worktree (ledger A12/B9); recovered to the documents route per this change's own disposition=inspect guidance instead of declaring the reviewer unavailable.
post_condition: the coordinator returns a specific error_code + bounded actionable detail on every rejection, and a closed disposition + recovery on every non-usable outcome; provider_error/teardown_error/protocol_error are never classified unavailable; fail-closed admitted-request set unchanged; additive fields only
mcp_coverage_gap: NONE
plugin_affected: agent-collab coordinator.py (client behavior); signed runtime unchanged (4.0.6); version 6.1.1 -> 6.2.0, skills/manifests/marketplace regenerated, both READMEs refreshed
readme_refresh_status: root README What's new - v6.2.0 + plugins/agent-collab/README.md Results section updated
contributor_rights: OWNER-AUTHORED
operator_reserved: no