refactor(reborn): one failure vocabulary — collapse five failure-kind enums into host_api FailureKind with fate projections (#6284) - #6684
Merged
Conversation
…ojections One failure vocabulary carried unchanged from mint site to loop, replacing the five overlapping enums whose folds destroyed 17 precise mechanism names (and every remediation hint) on the way up (#6284). Closed set — no open Unknown escape hatch; historical tags from the retired coarse vocabularies stay readable via from_tag aliases. Lossless 1:1 injections from the dispatch-lane contract enums replace the retired 22-to-12 coarsening fold. Fates pin the recoverability contract: exactly one variant (Cancelled) may end a run, policy denials never retry, and every wire tag passes the event layer's safe-shape validator (the silent-Unclassified rewrite hazard). Regression tests: fate/tag round-trips over ::ALL, historical-tag aliases, event-layer tag shape, single-terminal pin.
…FailureKind Delete the loop-local CapabilityFailureKind + CapabilityFailureKindValue open-set enum (run_profile/host/capability.rs) and its hand-written serde; ironclaw_turns now carries ironclaw_host_api::FailureKind (the closed 35-variant unified vocabulary from 115648e) everywhere: - CapabilityFailure.error_kind, milestones CapabilityFailed.reason_kind, progress CapabilityActivityFailed.reason_kind, and model_observation GenericFailure.failure_kind are now FailureKind. - resolution::failed takes FailureKind directly; the identity map failure_kind_of is deleted (the types are now the same type). - The CapabilityFailureKind/CapabilityFailureKindValue re-exports in run_profile/mod.rs and run_profile/host/mod.rs are removed — consumers import from ironclaw_host_api (type-placement rule); downstream crates are migrated in later slices. - Old wire tags (invalid_input, invalid_output, process, dispatcher, permanent) stay readable via FailureKind::from_tag aliases; the legacy generic_failure JSON round-trip test still passes unchanged. - Deleted the now-tautological enum-to-enum mapping test failed_carries_its_error_kind_on_the_verdict (it asserted the identity of a map that no longer exists); the remaining diagnostic/verdict tests were updated to construct FailureKind::{InputEncode,Backend} directly. No Unknown-funnel production sites existed in this crate; the only unknown(...) constructions were in the deleted tautology test. Downstream crates that imported ironclaw_turns::CapabilityFailureKind are intentionally broken until their own migration slices land. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…api FailureKind Delete the crate-local RuntimeFailureKind (17 coarse variants) and carry ironclaw_host_api::FailureKind (the closed 35-variant unified vocabulary from 115648e) everywhere in ironclaw_host_runtime: - The 22->12 coarsening fold (From<DispatchFailureKind> for RuntimeFailureKind in production.rs) is deleted; the Dispatch arm of failure_kind_from now uses host_api's lossless From injections, so precise mechanism names (MethodMissing, UndeclaredCapability, FilesystemDenied, SecretDenied, NetworkDenied, Client, Executor, Manifest, ExitFailure, OutputDecode, InvalidResult, ...) survive upward instead of being squashed into InvalidInput/InvalidOutput/ Backend/Authorization/Process. - Non-dispatch producers keep their 1:1 names (UnknownCapability invocation error -> MissingRuntime, obligation failures, store outages -> Backend, gate-declined -> GateDeclined, default trait impls -> Unavailable). InvocationFingerprint and the sandbox spawn-input rejections move from the retired InvalidInput to its successor InputEncode. - capability_failure_disposition now delegates to FailureKind::fate() (Retry -> RetrySameCall, everything else -> ModelVisibleToolError); the local runtime_failure_is_retryable set is deleted. Dispositions are preserved per kind; NetworkDenied keeps the retired fold's retryable behavior via an explicit carve-out that the follow-up fix commit removes. - Pinning tests rewritten to pin the lossless mapping; test expectations updated from coarse to precise kinds (that precision is the point of the migration). Events/metrics now carry the precise snake_case tags (e.g. method_missing instead of invalid_input). Downstream crates that imported ironclaw_host_runtime::RuntimeFailureKind are intentionally broken until their own migration slices land. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The retired coarsening fold mapped the NetworkDenied dispatch kind onto the retryable Network bucket, so egress POLICY denials were quietly retried — policy does not change between attempts, so every retry burned loop budget on a call that could never succeed. The unified FailureKind keeps NetworkDenied distinct with fate() = ModelVisible: the denial surfaces to the model as a tool error it can route around, and it is excluded from the quiet-retry set. This commit removes the mechanical-migration carve-out that had preserved the old retryable disposition, and adds the regression test network_denied_dispatch_failure_is_model_visible_not_retried driving the production failure_from -> disposition() chain. The test fails before this fix — it would also have failed against the retired fold, where a NetworkDenied dispatch failure produced kind Network with disposition RetrySameCall (verified red against the carve-out state). Note (report-surfaced, not changed here): the first-party HTTP mint maps RuntimeHttpEgressReasonCode::NetworkError (transport failures, offline egress) to the NetworkDenied dispatch kind, so those now also surface model-visibly instead of retrying; the dispatch-lane vocabulary has no transport-network kind for the mint to use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…spatch kind The dispatch lane had no transport-network variant, so the first-party HTTP mint routed RuntimeHttpEgressReasonCode::NetworkError (unreachable, reset, timeout) onto NetworkDenied — the egress POLICY kind. After NetworkDenied correctly stopped retrying, genuine network blips would have stopped retrying with it. RuntimeDispatchErrorKind::Network now exists, injects 1:1 into the unified FailureKind::Network (fate: Retry), and the mint distinguishes transport faults from policy denials. Regression surface: builtin_http_offline_runtime_egress test now pins transport offline egress as retryable Network, while the three policy-denial sites stay pinned NetworkDenied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hrough the loop seam Migrate ironclaw_loop_host and ironclaw_reborn_composition onto the closed 35-variant ironclaw_host_api::FailureKind (115648e), deleting the two identity maps that shuttled the now-unified vocabulary between the retired RuntimeFailureKind and CapabilityFailureKind enums: - capability_port.rs: runtime_failure_kind_to_loop and model_visible_runtime_failure_kind_to_loop mapped RuntimeFailureKind -> CapabilityFailureKind name-for-name; both types are now the same type, so the functions are deleted and failure.kind is used directly. - The open-set capability_failure_kind(String) funnel is replaced at its two call classes: AgentLoopHostErrorKind funnel sites (terminal capability- failure milestones) now go through the new exhaustive, wildcard-free failure_kind_for_host_error_kind match (Unauthorized/ScopeMismatch -> Authorization, CredentialUnavailable -> AuthRequired, StaleSurface -> StaleSurface, InvalidInvocation/Invalid -> InputEncode, InvalidOutput -> OutputDecode, ContentFiltered -> OperationFailed, PolicyDenied -> PolicyDenied, Budget* -> Resource, Unavailable -> Unavailable, Cancelled -> Cancelled, CheckpointRejected/TranscriptWriteFailed/Internal -> Internal); RuntimeCapabilityOutcome::Unknown tag-string sites parse via the total FailureKind::from_tag (unrecognized legacy open-set tags land on Internal instead of erroring the run with "could not be represented"). - denied_reason_kind_for keeps emitting the literal "auth_denied" for Authorization: the loop-safe identifier validator rejects the substring "authorization", and that special case remains load-bearing (pinned by the existing conversion tests). - Retired coarse names are renamed at their surviving sites: InvalidInput -> InputEncode, InvalidOutput -> OutputDecode. - ironclaw_reborn_composition: mechanical import/rename migration of its reference sites (runtime/local_dev/{project_create,result_read, skill_activation,outbound_delivery}, product_capability, approval_test_support, factory tests, service_factory tests). No behavior changes. - ironclaw_capabilities: doc-comment reference updated (no code change). Deleted the now-tautological sync-parity test runtime_failure_kind_mapping_preserves_current_categories (it asserted agreement between the two deleted enums, which no longer exist as distinct types). Updated the invalid-request milestone tests to pin the precise InputEncode reason kind, and rewrote runtime_capability_unknown_outcome_with_invalid_kind_does_not_emit_failure_milestone as ..._with_wild_kind_maps_to_internal_failure: the closed vocabulary's total from_tag makes every tag representable, so a wild unknown-outcome tag now becomes a model-visible Internal failure with its milestone instead of a run-ending internal error. ironclaw_reborn_composition still fails to build against this commit because its dependency ironclaw_agent_loop has not yet been migrated (separate slice); the composition changes here are the mechanical rename half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ending the run SubagentSpawnCapabilityPort::invoke_capability and the pre-decode pass in invoke_capability_batch used `?` on spawn_input_codec.decode, which decodes MODEL-SUPPLIED JSON. A malformed spawn_subagent payload (bad JSON shape, schema-violating wire args, model-correctable rejections like requesting the disabled background mode) therefore propagated as Err(AgentLoopHostError), which the executor maps to a run-ending HostUnavailable — killing the whole run on output the model could simply correct. Route InvalidInvocation decode failures to the existing spawn_rejected -> Resolution::Denied channel (reason tag invalid_spawn_input, carrying the codec's sanitized summary) so the model sees a denial and can retry with fixed input; genuine host faults (input-store outages and every other error kind) still propagate as errors. In the batch path the denial is parked per-invocation so sibling invocations keep their positions and the batch no longer aborts. Regression tests (fail before this fix): invoke_spawn_denies_malformed_ model_input_without_side_effects and invoke_spawn_batch_denies_malformed_ model_input_without_side_effects drive the real JsonSpawnSubagentInputCodec through the port with malformed JSON and a background-mode request, assert a Denied resolution with the corrective summary, and assert no spawn side effects (no child runs, goals, or await edges). They replace the two propagates_decode_rejection tests that pinned the old run-ending behavior; the unused RejectingSpawnInputCodec fixture is removed with them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ailureKind fates
Replace the loop-private CapabilityFailureKind/CapabilityErrorClass pair with
the unified ironclaw_host_api::FailureKind and its fate() projection:
- CapabilityErrorSummary now carries `kind: FailureKind`; the class enum and
its re-declared recoverability domain are gone.
- DefaultRecoveryStrategy::on_capability_error is a wildcard-free match on
FailureFate (Retry/ModelVisible/Park/Terminal); the `_ => Abort(DriverBug)`
catch-all is dead. Park (AuthRequired) surfaces model-visibly — gates still
ride Resolution::Blocked.
- The three byte-identical kind→LoopFailureKind mappings collapse into one
pub(crate) capability_error_to_failure_kind in strategies/recovery.rs,
wildcard-free over the full vocabulary.
- capability_error_failure_category keeps the seven-string wire contract with
the runner/product layers; StaleSurface pins "capability_policy_denied"
(the retired mint sites lied with PolicyDenied; the wire category must not
change under the honest rename) and "capability_permanent" is reachable
only through Cancelled.
- Direct mint sites get honest kinds: surface-filtered and Resolution::Denied
mint PolicyDenied; the stale-surface host-error branches mint StaleSurface.
- The serde round-trip funnel (capability_failure_kind_from) is deleted; the
verdict already carries the unified kind.
- Behavior deltas from the prescribed kind merges: the retired Permanent kind
(now OperationFailed) is model-visible instead of run-ending, and Dispatcher
(now Internal) retries instead of surfacing; the terminal capability set is
exactly {Cancelled}. Tests that used Permanent as the abort vehicle for the
failure-explanation flow now drive it through the iteration-limit exit, and
the classification-lock tests pin fate()-driven outcomes and the
seven-string category set over FailureKind::ALL.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eKind Completes the mechanical migration: runner milestone/disclosure seams, webui HTTP status mapping (exhaustive over the unified vocabulary, statuses preserved per retired variant), product projections, composition runtime services, and extension_host test support all consume ironclaw_host_api::FailureKind directly. Precision upgrades ride the wire: missing first-party handler now reports undeclared_capability (was invalid_input), skill-write denial reports filesystem_denied (was authorization) — same fates, same retry buckets. Verified per-crate before the disk-full interruption and re-verified after on a clean build: runner, extension_host, product suites green; composition green except pre-existing frontend-toolchain static-asset failures (SKIP_FRONTEND_BUILD machine issue, tracked separately). Zero-warning clippy across all six migrated crates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…te instead of ending the run
capability_host_error mapped EVERY non-Cancelled capability-stage port
Err(AgentLoopHostError) to a run-ending HostUnavailable{Capability}, so
Unauthorized / ScopeMismatch / InvalidInvocation / CredentialUnavailable /
PolicyDenied etc. from the ~66 loop_host mint sites silently killed runs
the model could have recovered (epic #6284 item 1).
- The kind projection moves to its owner: AgentLoopHostErrorKind::failure_kind
(crates/ironclaw_turns/src/run_profile/host/error.rs), replacing loop_host's
private failure_kind_for_host_error_kind byte-for-byte (table exactly as
slice 2fbcdea committed it); both loop_host milestone sites now call the
method.
- The executor splits capability-stage port Errs by an exhaustive,
wildcard-free terminal predicate (capability_port_error_is_terminal):
genuine host faults (Cancelled, Unavailable, Internal, BudgetExceeded,
BudgetApprovalRequired, BudgetAccountingFailed, CheckpointRejected,
TranscriptWriteFailed) keep today's terminal capability_host_error path;
caller-shaped kinds (Unauthorized, CredentialUnavailable, ScopeMismatch,
StaleSurface, InvalidInvocation, Invalid, InvalidOutput, ContentFiltered,
PolicyDenied) are routed through handle_capability_error at both dispatch
sites (batch + retry), so the model sees a tool-error observation carrying
the unified FailureKind and the secret-scrubbed detail, and the recovery
strategy dispositions it by FailureKind::fate.
- The four .map_err(capability_host_error) sites in capability_helpers.rs are
NOT dispatch — they are gate-record reads and transcript result-ref appends
(surface/definition plumbing) — and deliberately stay terminal.
- Every self.handle_capability_error(...) await site is now Box::pin'ed: the
executor future was at the stack margin (the inline arm made
reborn_integration_tool_call's
current_tool_surface_overrides_stale_assistant_unavailable_claim overflow
its test-thread stack; boxing the sites removes the repeated inline
embedding of that huge future). Behavior-neutral.
Regression test: recoverable_batch_port_error_surfaces_as_model_visible_tool_error
(crates/ironclaw_agent_loop/src/executor/tests.rs) — a scripted batch port
Err(Unauthorized) must complete the run with a GenericFailure{Authorization}
tool observation; red-verified pre-fix failing with
HostUnavailable{Capability}.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pre-commit-safety's PANIC check flagged the new FailureKind assertion in composition's local_dev_host_tests (a test-only module outside the script's /tests/ path heuristic); annotate it with the file's existing "// safety: test-only assertion." convention. No ratchet re-baselines were needed: composition budget is 770bp vs 2428bp effective ceiling, and the coverage-floor denominator moved ~0.2% (immaterial per its own header). [skip-regression-check] CI-gate annotation only; no production behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6684
July 26, 2026 15:22
Destroyed
github-actions
Bot
added
size: XL
Closed
59 tasks done
📝 WalkthroughWalkthroughThe PR replaces capability-specific failure enums with a closed shared ChangesUnified failure taxonomy
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CapabilityPort
participant HostRuntime
participant FailureKind
participant RecoveryStrategy
participant Model
CapabilityPort->>HostRuntime: invoke capability
HostRuntime-->>FailureKind: classify runtime result
CapabilityPort->>RecoveryStrategy: submit failure summary
RecoveryStrategy-->>CapabilityPort: retry, model-visible error, or abort
CapabilityPort-->>Model: emit tool observation for recoverable failure
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
⚠️ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| 0 | 0 | 0 | ce893f09f3fa |
Head: ce893f09f3fa161e1d7b24dd744362f866cad375
Next: Human review or validation is required before merging.
Run details
Status: Current
Needs human: no
Needs validation: yes
Summary
Static review found no concrete actionable defect. Runtime validation is required because the Rust toolchain is unavailable in this review environment.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
|
🚅 Deployed to the ironclaw-pr-6684 environment in ironclaw-ci-preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/ironclaw_turns/src/run_profile/model_observation.rs (1)
503-519: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExtend the legacy-JSON test to an actually-legacy tag.
"backend"is an identity tag under both the old and new vocabularies, so this test cannot catch alias regressions at the seam where persisted observations rehydrate. Add one aliased tag (e.g."invalid_input"→InputEncode) so this suite owns the historical-payload contract forGenericFailurerather than delegating it toresult_meta's unit test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_turns/src/run_profile/model_observation.rs` around lines 503 - 519, The legacy deserialization test generic_failure_deserializes_legacy_json_without_detail currently uses a tag unchanged across vocabularies. Change its failure_kind payload to an aliased historical tag such as invalid_input and assert it deserializes to FailureKind::InputEncode with detail: None, preserving the legacy GenericFailure contract.crates/ironclaw_host_runtime/src/production.rs (1)
2006-2089: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the new
Networkvariant to the exhaustive dispatch-kind test.
RuntimeDispatchErrorKindhas 25 variants, butdispatch_kind_to_failure_pins_every_runtime_dispatch_error_kindonly exercises 23 and omitsRuntimeDispatchErrorKind::Network. Since this pinned array drives the stated 1:1 contract, add(RuntimeDispatchErrorKind::Network, FailureKind::Network)before it can silently miss additions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_host_runtime/src/production.rs` around lines 2006 - 2089, Add the missing `(RuntimeDispatchErrorKind::Network, FailureKind::Network)` case to the `cases` array in `dispatch_kind_to_failure_pins_every_runtime_dispatch_error_kind`, alongside the other 1:1 runtime dispatch mappings, so the exhaustive contract covers this variant.crates/ironclaw_host_runtime/tests/first_party_coding_tools.rs (1)
1038-1059: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale Denied-summary comment.
runtime_model_visible_failure_to_loop(crates/ironclaw_loop_host/src/capability_port.rs:3794-3816) only mapsAuthorizationandPolicyDeniedtoLoopFailureClass::Denied;FilesystemDeniedis handled by the fallbackFailedpath and carries diagnostic details. Soften lines 1041-1044 to describe the diagnostic cross-layer channel, or update the mapping if mount denials are intended to be Denied outcomes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_host_runtime/tests/first_party_coding_tools.rs` around lines 1038 - 1059, Update the comment in builtin_read_file_out_of_scope_rejection_reaches_the_model_through_the_summary to reflect that FilesystemDenied follows the fallback Failed path and exposes diagnostic details, rather than claiming it becomes a Denied loop outcome with summary-only visibility. Do not change runtime_model_visible_failure_to_loop unless mount denials are explicitly intended to map to LoopFailureClass::Denied.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_agent_loop/src/executor/tests/failure_matrix.rs`:
- Around line 586-605: Make the recoverable CapabilityOperationFailed assertion
in the failure-matrix test require a model-visible observation: replace the
optional if-let handling around observed.appended_result_refs with an
expect-based lookup, then assert the existing GenericFailure details on the
returned observation. Align its failure behavior with the sibling
CapabilityInvalidOutputRecoverable check.
In `@crates/ironclaw_host_api/src/dispatch.rs`:
- Around line 210-262: Update the RuntimeDispatchErrorKind conversion in
From<RuntimeDispatchErrorKind> for crate::FailureKind so Unknown does not map to
retryable Internal. Map it to the same non-retryable fallback used by
FailureKind::from_tag, while preserving all other variant mappings unchanged.
In `@crates/ironclaw_host_api/src/result_meta.rs`:
- Around line 343-397: Add a distinct Unclassified failure kind with
ModelVisible fate, keeping Internal reserved for genuine retryable host faults.
In crates/ironclaw_host_api/src/result_meta.rs lines 343-397, change
ResultMeta::from_tag’s unknown-tag fallback to Unclassified and pin the behavior
in failure_kind_historical_tags_stay_readable. In
crates/ironclaw_host_api/src/dispatch.rs lines 210-262, map
RuntimeDispatchErrorKind::Unknown to Unclassified so redacted lane failures are
not retried.
- Around line 399-436: Strengthen FailureKind::ALL completeness by adding an
exhaustive all_membership_is_exhaustive method with one index-mapped match arm
for every variant, and add a test that iterates ALL and verifies each kind maps
to its index. Keep ALL at 35 entries while ensuring adding any new FailureKind
variant requires updating the exhaustive match and therefore cannot silently
bypass downstream ALL-based coverage.
In `@crates/ironclaw_host_runtime/src/lib.rs`:
- Around line 677-693: Soften the cross-layer guarantee in the documentation
above capability_failure_disposition to describe the expected production-path
intent rather than asserting that Park and Terminal failures never reach this
function. Keep the existing FailureKind::fate mapping and conservative
ModelVisibleToolError behavior unchanged.
In `@crates/ironclaw_host_runtime/src/production.rs`:
- Around line 1802-1817: Update the CapabilityInvocationError::ObligationFailed
mapping so CapabilityObligationFailureKind::Network returns
FailureKind::NetworkDenied, Mount returns FailureKind::FilesystemDenied, and
Secret returns FailureKind::SecretDenied. Leave the Audit, Output, and Resource
mappings unchanged.
In `@crates/ironclaw_loop_host/src/subagent_spawn_port.rs`:
- Around line 1168-1176: Malformed spawn JSON is rejected before provider-tool
execution can produce a denied resolution. In
crates/ironclaw_loop_host/src/subagent_spawn_port.rs#L1168-L1176, defer or
translate validation from validate_provider_tool_call and
register_spawn_provider_tool_call so failures become denials; apply the same
behavior to batch registration/invocation at `#L1196-L1216`. In
crates/ironclaw_loop_host/src/subagent_spawn_port/tests.rs#L2852-L2974, add a
regression test through the real provider-tool registration-to-execution path
confirming malformed model-supplied spawn JSON yields Resolution::Denied.
In `@crates/ironclaw_reborn_composition/src/runtime/local_dev/result_read.rs`:
- Around line 249-262: The result-reference helpers currently misclassify
failures as FailureKind::InputEncode. Update unavailable_result_reference() to
use FailureKind::Unavailable and assign non_text_result_content() a precise
output/result classification such as OutputDecode or InvalidResult, following
the taxonomy in result_meta.rs; then update the corresponding local_dev/tests.rs
assertions.
In
`@crates/ironclaw_reborn_composition/src/runtime/local_dev/skill_activation.rs`:
- Around line 232-241: Update the SelectionError::ContextBudgetExceeded arm in
the skill activation error mapping to use FailureKind::Resource instead of
FailureKind::InputEncode, while keeping AmbiguousSkill mapped to InputEncode.
Update the associated test helper to assert the expected FailureKind for each
case, including Resource for context-budget exhaustion.
In `@crates/ironclaw_runner/src/tool_disclosure_port.rs`:
- Around line 2759-2761: Strengthen the caller-level verdict assertions so each
model-correctable failure verifies both its FailureKind and
ToolVerdict::RecoverableFailure, ensuring only Cancelled remains terminal. Apply
this in crates/ironclaw_runner/src/tool_disclosure_port.rs at lines 2759-2761,
2817-2819, 2891-2893, and 3068-3069, and assert ToolVerdict::RecoverableFailure
for the unknown runtime outcome in
crates/ironclaw_runner/tests/loop_driver_host.rs at lines 7095-7098.
In `@crates/ironclaw_turns/src/run_profile/host/error.rs`:
- Around line 85-92: Update the error-to-FailureKind projection around the Self
variants so BudgetApprovalRequired uses the park/approval disposition,
BudgetAccountingFailed uses the fail-closed disposition, and CheckpointRejected
uses a non-retryable permanent-failure disposition rather than Internal. Split
these variants into separate match arms, select the existing canonical
FailureKind values used by result_meta.rs, and apply the same honest
classifications at the originating decision site.
In `@crates/ironclaw_webui/src/webui_v2/handlers.rs`:
- Around line 3012-3038: Extract the duplicated Forbidden ProductSurfaceError
construction in admin_configuration_done_failure into a shared
admin_configuration_forbidden() helper, mirroring
admin_configuration_unavailable(). Replace both the GateDeclined branch and
CapabilityFailureHttpClass::Forbidden branch with calls to this helper while
preserving their existing behavior.
- Around line 1807-1845: The capability_failure_http_class function currently
classifies FailureKind::Resource as retryable Unavailable despite its canonical
ModelVisible fate. Remove Resource from the Unavailable match arm and include it
in the non-retryable Internal arm, preserving the existing classifications for
all other FailureKind variants.
---
Outside diff comments:
In `@crates/ironclaw_host_runtime/src/production.rs`:
- Around line 2006-2089: Add the missing `(RuntimeDispatchErrorKind::Network,
FailureKind::Network)` case to the `cases` array in
`dispatch_kind_to_failure_pins_every_runtime_dispatch_error_kind`, alongside the
other 1:1 runtime dispatch mappings, so the exhaustive contract covers this
variant.
In `@crates/ironclaw_host_runtime/tests/first_party_coding_tools.rs`:
- Around line 1038-1059: Update the comment in
builtin_read_file_out_of_scope_rejection_reaches_the_model_through_the_summary
to reflect that FilesystemDenied follows the fallback Failed path and exposes
diagnostic details, rather than claiming it becomes a Denied loop outcome with
summary-only visibility. Do not change runtime_model_visible_failure_to_loop
unless mount denials are explicitly intended to map to LoopFailureClass::Denied.
In `@crates/ironclaw_turns/src/run_profile/model_observation.rs`:
- Around line 503-519: The legacy deserialization test
generic_failure_deserializes_legacy_json_without_detail currently uses a tag
unchanged across vocabularies. Change its failure_kind payload to an aliased
historical tag such as invalid_input and assert it deserializes to
FailureKind::InputEncode with detail: None, preserving the legacy GenericFailure
contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4f182963-6b15-409b-80e6-00f7fe994cdc
📒 Files selected for processing (67)
crates/ironclaw_agent_loop/src/executor.rscrates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/capability_helpers.rscrates/ironclaw_agent_loop/src/executor/mapping.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/executor/tests/cancellation.rscrates/ironclaw_agent_loop/src/executor/tests/failure_matrix.rscrates/ironclaw_agent_loop/src/executor/tests/support.rscrates/ironclaw_agent_loop/src/strategies/mod.rscrates/ironclaw_agent_loop/src/strategies/recovery.rscrates/ironclaw_agent_loop/src/test_support/mod.rscrates/ironclaw_capabilities/src/trust.rscrates/ironclaw_extension_host/src/extension_lifecycle_capabilities.rscrates/ironclaw_extension_host/src/host_remediation_contract_tests.rscrates/ironclaw_extension_host/src/test_support/lifecycle.rscrates/ironclaw_host_api/src/dispatch.rscrates/ironclaw_host_api/src/resolution.rscrates/ironclaw_host_api/src/result_meta.rscrates/ironclaw_host_runtime/src/first_party_tools/http.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/src/production.rscrates/ironclaw_host_runtime/tests/builtin_obligation_handler_contract.rscrates/ironclaw_host_runtime/tests/first_party_builtin_tools.rscrates/ironclaw_host_runtime/tests/first_party_coding_tools.rscrates/ironclaw_host_runtime/tests/first_party_runtime_contract.rscrates/ironclaw_host_runtime/tests/github_wasm_runtime_contract.rscrates/ironclaw_host_runtime/tests/host_runtime_contract.rscrates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rscrates/ironclaw_host_runtime/tests/host_runtime_services_contract.rscrates/ironclaw_host_runtime/tests/production_trust_contract.rscrates/ironclaw_host_runtime/tests/reborn_durable_restart_integration.rscrates/ironclaw_host_runtime/tests/reborn_e2e_gate.rscrates/ironclaw_host_runtime/tests/reborn_invoke_vertical_slice.rscrates/ironclaw_host_runtime/tests/support/host_runtime_harness.rscrates/ironclaw_host_runtime/tests/support/trace_commons_dispatch.rscrates/ironclaw_host_runtime/tests/tool_surface_contract.rscrates/ironclaw_loop_host/src/capability_port.rscrates/ironclaw_loop_host/src/capability_port/tests/runtime_lifecycle_tests.rscrates/ironclaw_loop_host/src/subagent_spawn_port.rscrates/ironclaw_loop_host/src/subagent_spawn_port/tests.rscrates/ironclaw_product/src/projection/tests/live_progress_stream.rscrates/ironclaw_product/src/reborn_services.rscrates/ironclaw_reborn_composition/src/approval_test_support.rscrates/ironclaw_reborn_composition/src/factory/local_dev_host_tests/approval_gates.rscrates/ironclaw_reborn_composition/src/factory/tests.rscrates/ironclaw_reborn_composition/src/product_capability.rscrates/ironclaw_reborn_composition/src/product_surface/tests.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/project_create.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/result_read.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/skill_activation.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/tests/service_factory.rscrates/ironclaw_runner/src/milestone_events.rscrates/ironclaw_runner/src/tool_disclosure_port.rscrates/ironclaw_runner/tests/loop_driver_host.rscrates/ironclaw_runner/tests/loop_milestone_event_projection.rscrates/ironclaw_turns/src/run_profile/host/capability.rscrates/ironclaw_turns/src/run_profile/host/error.rscrates/ironclaw_turns/src/run_profile/host/mod.rscrates/ironclaw_turns/src/run_profile/host/progress.rscrates/ironclaw_turns/src/run_profile/milestones.rscrates/ironclaw_turns/src/run_profile/mod.rscrates/ironclaw_turns/src/run_profile/model_observation.rscrates/ironclaw_turns/src/run_profile/resolution.rscrates/ironclaw_webui/src/webui_v2/handlers.rstests/integration/support/doubles/recording_test_capability_port.rs
…s surface, never retry `Internal` was doing two incompatible jobs: the retryable host-fault bucket AND the sink for anything the system could not classify (`from_tag`'s unrecognized-tag fallback, `RuntimeDispatchErrorKind::Unknown`'s redaction bucket). Because `Internal` is Retry-fated, every unclassifiable failure — possibly permanent — silently consumed retry budget before the model ever saw it. Adds a 36th variant `Unclassified` (tag `unclassified`, fate ModelVisible, never retryable) and routes both unknown funnels to it. `Internal` now means exactly one thing: a classified, retryable host fault. Also compiler-owns `ALL`: the enum, its wire tags, and `ALL` now expand from a single `declare_failure_kinds!` list, so a new variant cannot exist outside `ALL` and the downstream exhaustiveness pins that iterate it (host_runtime's production disposition pin, agent_loop's category and recovery pins) cannot silently stop covering it. Previously a variant could be added by touching only `fate`/`as_str`/`from_tag` and every `ALL`-driven pin would keep passing. Second honesty fix in the same lane: a `CapabilityObligationFailureKind:: Network` failure is always deterministic policy/config (duplicate ApplyNetworkPolicy, empty allowed_targets, missing policy store) — never a transport fault. It now maps to the never-retryable `NetworkDenied` instead of the retryable transport `Network`, with a regression test. Softens the `capability_failure_disposition` doc: the "Park and Terminal never reach this on production paths" claim is intent, not a code-enforced invariant, and is now stated as such. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three deliberately distinct dispositions were collapsing into one in
`AgentLoopHostErrorKind::failure_kind()`, against what the variant docs
prescribe:
- `BudgetApprovalRequired` documents a PARK semantic ("callers surface an
approval gate and retry after the user resolves it"), but mapped to
`Resource` (ModelVisible) — handing the model a tool error to route
around instead of parking on the gate. Now `AuthRequired` (Park).
- `BudgetAccountingFailed` documents "callers must fail closed" and is
explicitly distinct from the budget outcome, but folded into the same
model-visible `Resource` bucket. Now `Unclassified` — not a quota the
model can work around, and not retryable.
- `CheckpointRejected` mapped to `Internal`, which is Retry-fated. A schema
id/version mismatch is deterministic and cannot succeed on re-attempt;
retrying only burns budget. Now `OperationFailed` (non-retryable).
`BudgetExceeded` keeps `Resource` — that one really is a quota the model can
work around. agent_loop's terminal set is unchanged (Budget*/Checkpoint*
remain terminal there), so this projection governs the milestone/observation
surface; both sides are now honest independently.
Regression test pins all four projections and their retryability.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lt-read denials Three local-dev mints named the wrong mechanism, losing the precision the unified vocabulary exists to carry (same fate in every case, so no recoverability change): - skill activation `ContextBudgetExceeded` is a resource limit, not an encoding fault → `Resource` (`AmbiguousSkill` stays `InputEncode`); the test helper now asserts the expected kind per case instead of assuming one kind for both. - `unavailable_result_reference()` — a result ref absent from this thread — is a domain failure of the read, not malformed input → `OperationFailed`. Deliberately NOT `Unavailable`: that is Retry-fated and would quietly retry a ref that can never appear (genuine storage faults already take the separate `AgentLoopHostErrorKind::Unavailable` path). - `non_text_result_content()` is an output-decode failure → `OutputDecode`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`capability_failure_http_class` sent `FailureKind::Resource` through the `Unavailable` class — documented as "the backing service hiccuped; retryable (503)" — while `fate()` classifies `Resource` as ModelVisible and non-retryable. WebUI callers were being told to retry a quota/limit failure that can never succeed. It now takes the non-retryable internal path. Adds `http_class_retryability_agrees_with_failure_kind_fate`, an exhaustive pin over `FailureKind::ALL` asserting no non-retryable kind rides the retryable-503 class (`StaleSurface` is the one documented exception: at the HTTP seam a re-issued request races a refreshed surface and can succeed). Also dedupes the twice-inlined Forbidden literal in `admin_configuration_done_failure` into `admin_configuration_forbidden()`, mirroring the existing `admin_configuration_unavailable()` helper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ations
Test-only follow-ups from the review, plus the CI expectation fallout of the
precise-rename slices in this PR.
Hardened assertions:
- failure_matrix's `OperationFailed` recovery check was a soft `if let Some`
that no-oped when no observation was present — now `.expect(...)`, matching
its `InvalidOutput` sibling. Hardening it exposed that the row scripted a
non-provider call (`calls_response()`, `provider_replay: None`), and
model-visible observations are persisted on provider result refs only, so
the assertion had never actually run. The row now uses
`provider_calls_response()` and genuinely proves the cause survives.
- the five `error_kind()`-only tool-verdict checks in tool_disclosure_port /
loop_driver_host now match `ToolVerdict::RecoverableFailure { .. }`
explicitly.
- `generic_failure_deserializes_legacy_json_without_detail` also covers a
retired coarse tag (`invalid_input`) decoding through `from_tag`'s alias.
- new `malformed_provider_tool_call_registration_errors_stay_model_repairable`
pins the model_gateway seam both the validate and register loops call:
a malformed model-supplied provider tool call (bad spawn_subagent JSON)
maps to the model-stage InvalidOutput repair lane, never a run-ending
host fault.
Expectation alignment (the renames this PR performs, and one fate change):
- `invalid_input` → `input_encode` (outbound_target, project_create,
group_extensions install), `backend` → `client` (MCP client faults).
- `RecordingTestCapabilityPort::invocation_error()` now mints the terminal
`Unavailable` instead of `InvalidInvocation`: caller-shaped port errors
recover in-loop by fate now, which defeats the run-failed → user-retry
journey this double exists to drive.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6684
July 26, 2026 17:42
Destroyed
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/ironclaw_host_runtime/src/lib.rs (1)
822-834: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win"Unsupported by this host runtime" is permanent, but
Unavailableis Retry-fated.
FailureKind::Unavailable.fate()isRetry(result_meta.rsLine 302-306), so these default trait bodies burn the whole capability retry budget on an operation the runtime will never support — the same budget-burn this PR removes forNetworkDenied. The unified vocabulary now has honest permanent kinds;UnsupportedRunnerisModelVisible.This is a rename-preserved behavior, not a regression, but the lines are already being touched and the closed vocabulary makes the honest kind available.
♻️ Use a non-retryable kind for the unsupported defaults
- FailureKind::Unavailable, + FailureKind::UnsupportedRunner, Some("capability spawn is unsupported by this host runtime".to_string()),(apply to all three defaults:
spawn_capability,auth_resume_capability,resume_spawn_capability)Also applies to: 853-865, 880-892
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_host_runtime/src/lib.rs` around lines 822 - 834, Update the default implementations of spawn_capability, auth_resume_capability, and resume_spawn_capability to use FailureKind::UnsupportedRunner instead of FailureKind::Unavailable when constructing RuntimeCapabilityFailure. Preserve the existing failure messages and capability IDs while making all three permanently non-retryable.crates/ironclaw_host_api/src/result_meta.rs (1)
399-401: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTwo comments still describe the retired open
Unknownfunnel. Both were written against the pre-migration vocabulary and now contradict the code they annotate.
crates/ironclaw_host_api/src/result_meta.rs#L399-L401: drop the "18 named variants" / "only anUnknowntag needs an owned copy" rationale — the vocabulary is closed at 36 variants and never retains the tag.crates/ironclaw_host_runtime/src/production.rs#L2014-L2017: change "the redaction bucketUnknown->Internal" toUnknown->Unclassified, matching the case asserted at Line 2086.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_host_api/src/result_meta.rs` around lines 399 - 401, The comments use outdated terminology. In crates/ironclaw_host_api/src/result_meta.rs lines 399-401, remove the rationale about 18 named variants and an owned Unknown tag, leaving only an accurate description of the Cow deserialization; in crates/ironclaw_host_runtime/src/production.rs lines 2014-2017, update the redaction mapping comment to say Unknown maps to Unclassified, matching the asserted case.crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs (1)
3566-3570: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale assertion diagnostic.
Line [3568] now expects
FailureKind::InputEncode, but the failure message still reports the retiredInvalidInputtaxonomy.Proposed fix
- "{label}: expected InvalidInput verdict" + "{label}: expected InputEncode verdict"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs` around lines 3566 - 3570, Update the assertion message in the failure-kind check to report the current InputEncode taxonomy instead of the retired InvalidInput wording, while leaving the expected FailureKind::InputEncode assertion unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_runner/src/model_gateway.rs`:
- Around line 2762-2786: Add a caller-level regression test around the
provider-tool validation/registration flow that submits malformed spawn_subagent
JSON and verifies the caller enters model-stage InvalidOutput recovery with a
model-visible observation, not a terminal host error. Exercise the real
provider-tool caller and its recovery-gating helper rather than invoking
map_provider_tool_output_error directly; keep the existing mapping unit test
unchanged.
---
Outside diff comments:
In `@crates/ironclaw_host_api/src/result_meta.rs`:
- Around line 399-401: The comments use outdated terminology. In
crates/ironclaw_host_api/src/result_meta.rs lines 399-401, remove the rationale
about 18 named variants and an owned Unknown tag, leaving only an accurate
description of the Cow deserialization; in
crates/ironclaw_host_runtime/src/production.rs lines 2014-2017, update the
redaction mapping comment to say Unknown maps to Unclassified, matching the
asserted case.
In `@crates/ironclaw_host_runtime/src/lib.rs`:
- Around line 822-834: Update the default implementations of spawn_capability,
auth_resume_capability, and resume_spawn_capability to use
FailureKind::UnsupportedRunner instead of FailureKind::Unavailable when
constructing RuntimeCapabilityFailure. Preserve the existing failure messages
and capability IDs while making all three permanently non-retryable.
In `@crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs`:
- Around line 3566-3570: Update the assertion message in the failure-kind check
to report the current InputEncode taxonomy instead of the retired InvalidInput
wording, while leaving the expected FailureKind::InputEncode assertion
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b3217a1b-028b-4951-94ef-d7f1611a5317
📒 Files selected for processing (25)
crates/ironclaw_agent_loop/src/executor/capability_helpers.rscrates/ironclaw_agent_loop/src/executor/mapping.rscrates/ironclaw_agent_loop/src/executor/tests/failure_matrix.rscrates/ironclaw_agent_loop/src/strategies/recovery.rscrates/ironclaw_host_api/src/dispatch.rscrates/ironclaw_host_api/src/result_meta.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/src/production.rscrates/ironclaw_loop_host/src/capability_port.rscrates/ironclaw_loop_host/src/capability_port/tests/runtime_lifecycle_tests.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/result_read.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/skill_activation.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_runner/src/model_gateway.rscrates/ironclaw_runner/src/tool_disclosure_port.rscrates/ironclaw_runner/tests/loop_driver_host.rscrates/ironclaw_turns/src/run_profile/host/error.rscrates/ironclaw_turns/src/run_profile/model_observation.rscrates/ironclaw_webui/src/webui_v2/handlers.rstests/integration/group_extensions/scenario_install_unknown_extension_id_fails_safely.rstests/integration/mcp.rstests/integration/outbound_target.rstests/integration/project_create.rstests/integration/support/doubles/recording_test_capability_port.rstests/reborn_failure_retry_resume_e2e.rs
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.53% — 307563 / 359615 lines Per-crate breakdown (60 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
Contributor
|
🧪 Started |
…own promise Four review findings on the tests added in the previous commit. Two are substantive; both are the failure mode this PR exists to remove, committed in the code meant to prevent it. 1. The gate failed open. `read_dir`, directory entries, and `read_to_string` errors were silently skipped, so a scan that broke — bad path, permissions, a rename — would have passed while checking nothing. A guard that cannot fail is worse than no guard because it manufactures confidence. Every IO error now propagates with its path. (The silent-skip shape was inherited from `reborn_retired_taxonomy.rs`, which has the same flaw. Out of scope here; worth a follow-up.) 2. Block comments were not exempt. The module doc promises comments are exempt so prose explaining the retired vocabulary stays legal, but the implementation only stripped `//`, so a `/* … */` or `/** … */` naming a retired type would have failed the build. `strip_comments` now blanks both forms, preserving newlines so reported line numbers stay accurate. Verified in both directions with a temporary probe file: a block comment naming `CapabilityFailureKind`/`RuntimeFailureKind` passes; adding `pub enum CapabilityErrorClass` to the same file is caught at `_probe_block.rs:5`. 3. `builder.rs` — the inserted helper had orphaned the no-progress helper's doc comment onto itself. Restored to its own function. 4. `capability_backend.rs` — doc still said `Unauthorized` after the double was switched to `InvalidInvocation` (switched because `Authorization`'s summary prefix contains the banned marker "authorization:", so it fail-softs to the redacted fallback and would hide the kind the test asserts on). Verified: architecture suite 2 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6684
July 27, 2026 18:54
Destroyed
…bling doc Follow-up on the review of the previous commit. Two real gaps, one of them the reviewer's second pass on a fix I had called done. 1. Nested block comments. Rust permits `/* outer /* inner */ still comment */`, and the bool-based stripper closed at the FIRST `*/`, so a retired name after an inner comment was scanned as live code. Now a depth counter. Verified by reverting depth to a bool: `nested_block_comments_do_not_end_early` fails on the reviewer's own example. 2. The comment contract had no tests. It is a promise this gate makes to every file it scans, and an exemption rule nobody tests is one that silently changes meaning — the same class as the gate failing open. Five cases now cover it: line comments (and that code *before* a trailing comment still scans), multiline blocks, `/** */` doc blocks, nesting, code after a closed block, and line-number preservation (hits are reported by line, so blanking must keep newlines). 3. `group.rs:321` still said `Unauthorized`. The previous commit fixed only the anchor file the reviewer cited and missed the sibling site in the same consolidated finding. Verified: 7 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6684
July 27, 2026 18:57
Destroyed
Contributor
🧪 nearai-bench
|
Collaborator
Author
|
/canary |
Contributor
|
Started Reborn WebUI v2 live canary for |
17 of 29 tasks
serrrfirat
added a commit
that referenced
this pull request
Jul 28, 2026
…ase artifact Three review threads plus a formatting failure I introduced. 1. Silent swallow at the sanitized resolution boundary (`run_profile/resolution.rs`). `Err(_)` discarded the `HostApiError` and substituted the fixed fallback with no signal anywhere. Reaching that arm means a producer let credential-shaped text travel to the boundary — exactly the event an operator wants to see. Now carries the required `// silent-ok:` rationale and binds the cause on `debug!` (`info!`/`warn!` corrupt the REPL/TUI per repo CLAUDE.md). 2. Stale contract doc (`run_profile/model_observation.rs`). The `CapabilityFailureDetail` docs still promised untrusted output "keeps collapsing to the safe-summary placeholder" — the behavior this PR replaced. Rewritten to state what is now true: `Diagnostic` preserves the scrubbed cause and fails closed to the fixed sentence only for credential-shaped values, so the trusted arm exists for PROVENANCE rather than redaction strength. 3. Formatting CI failure — mine, from the #6684 rebase. `tests/integration/ mcp.rs` carried a dangling `.expect(...)` fragment whose receiver the merge had consumed, i.e. a syntax error. My rebase verification ran tests on three library crates and never compiled the integration tests, so a syntax error sat in the tree while I reported the rebase verified. Removed; the assertions this branch added above it already cover the same ground, and `cargo check -p ironclaw_reborn_integration_tests --tests` is now clean. Not changed: the thread claiming the preserved diagnostic is dropped downstream because the transcript validator rejects credential vocabulary and injection phrases. `validate_model_observation_detail` rejects only empty, over-cap and control characters — `"password field is required"` passes — and this branch's own `generic_failure_detail_allows_paths_and_payload_delimiters` and the `tool_result_reference` diagnostic test already pin that a path-bearing detail validates and round-trips. That finding predates this branch's second commit. Verified: 1475 passed / 0 failed across ironclaw_host_api, ironclaw_loop_host, ironclaw_turns; clippy --all-targets --all-features -D warnings clean; cargo fmt --all --check clean; integration tests compile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
serrrfirat
added a commit
that referenced
this pull request
Jul 28, 2026
* fix: make capability diagnostics actionable * test: align diagnostic contract after rebase * fix: address review on the diagnostic boundary, and repair my own rebase artifact Three review threads plus a formatting failure I introduced. 1. Silent swallow at the sanitized resolution boundary (`run_profile/resolution.rs`). `Err(_)` discarded the `HostApiError` and substituted the fixed fallback with no signal anywhere. Reaching that arm means a producer let credential-shaped text travel to the boundary — exactly the event an operator wants to see. Now carries the required `// silent-ok:` rationale and binds the cause on `debug!` (`info!`/`warn!` corrupt the REPL/TUI per repo CLAUDE.md). 2. Stale contract doc (`run_profile/model_observation.rs`). The `CapabilityFailureDetail` docs still promised untrusted output "keeps collapsing to the safe-summary placeholder" — the behavior this PR replaced. Rewritten to state what is now true: `Diagnostic` preserves the scrubbed cause and fails closed to the fixed sentence only for credential-shaped values, so the trusted arm exists for PROVENANCE rather than redaction strength. 3. Formatting CI failure — mine, from the #6684 rebase. `tests/integration/ mcp.rs` carried a dangling `.expect(...)` fragment whose receiver the merge had consumed, i.e. a syntax error. My rebase verification ran tests on three library crates and never compiled the integration tests, so a syntax error sat in the tree while I reported the rebase verified. Removed; the assertions this branch added above it already cover the same ground, and `cargo check -p ironclaw_reborn_integration_tests --tests` is now clean. Not changed: the thread claiming the preserved diagnostic is dropped downstream because the transcript validator rejects credential vocabulary and injection phrases. `validate_model_observation_detail` rejects only empty, over-cap and control characters — `"password field is required"` passes — and this branch's own `generic_failure_detail_allows_paths_and_payload_delimiters` and the `tool_result_reference` diagnostic test already pin that a path-bearing detail validates and round-trips. That finding predates this branch's second commit. Verified: 1475 passed / 0 failed across ironclaw_host_api, ironclaw_loop_host, ironclaw_turns; clippy --all-targets --all-features -D warnings clean; cargo fmt --all --check clean; integration tests compile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(host_api): a corpus that fails when the credential guard gets greedy Every existing test on this channel proves the guard REJECTS what it should. Nothing proved it still ACCEPTS what it should, so a tightened detector could only break silently — and a diagnostic that says nothing is indistinguishable from one that had nothing to say. Strictness could ratchet up unnoticed forever. Adds the other direction: eleven realistic diagnostics that must survive `ModelDiagnostic::new` — bare credential vocabulary ("the api key is missing from the request"), remediation prose that has to name what the user must go fix ("rotate your client secret in the provider console"), paths and schema refs, and already-redacted payloads. The four value cases sit beside them, so tightening one direction cannot quietly loosen the other. Red-verified: adding a single greedy clause (`lower.contains("api key")`) to `contains_unredacted_credential_value` fails with over-scrubbed a legitimate diagnostic: "the api key is missing from the request" rejected as InvalidModelDiagnostic — a detector got greedy; the model needs this sentence Worth recording what the corpus showed on its first run: every entry already passed. This channel's values-only rules are correct as written. The over-scrubbing felt elsewhere comes from WHICH detector guards WHICH channel — `safe_summary.rs` and `model_result_preview.rs` still gate on the crude whole-word marker list, so a sentence dies for containing "password" at all. That is what makes `FailureKind::Authorization` unable to name itself (#6284). This corpus is the guardrail that makes migrating those two callers safe: it fails if they are loosened too far, and it fails if anyone tightens them back. Verified: 1476 passed / 0 failed; clippy --all-targets --all-features -D warnings clean; cargo fmt --all --check clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Jul 28, 2026
henrypark133
added a commit
that referenced
this pull request
Jul 28, 2026
…runk main's `sandbox_process::ca` and `credential_firewall` are strictly ahead of trunk's copies (5 review rounds fixed a Debug impl leaking the leaf private key/staged secret handle, a post-clone-stale authorize() deadline, leaf certs uncapped at the root's expiry, and a cache that could serve an expired cert). Take main's version of both files wholesale plus their split-out test modules; adapt trunk-only tls_intercept.rs's cfg(test)-gated cached_leaf_count to the merged CA API (already compatible, no changes needed); drop the fully retired RuntimeFailureKind from first_party_builtin_tools.rs in favor of FailureKind per #6684; and de-duplicate/re-derive the struct test-support/dead-code ratchet counts for the two files from the actual scanner output.
serrrfirat
added a commit
that referenced
this pull request
Jul 28, 2026
… observations to drift #6792 was broken and its tests could not see it. `ironclaw_threads` validates the persisted observation against a hand-copied list of valid `recovery_hint` / `same_call_retry` strings. It cannot import `ironclaw_turns`, where the enums lived — the two are siblings — so the vocabulary existed twice with nothing keeping the copies in step. #6792 added six recovery hints and missed the copy. Every one was rejected, and rejection is not a loud failure: `normalized_model_ observation` DROPS THE WHOLE OBSERVATION. So every denial that PR was written to improve persisted with no observation at all — the model lost the cause *and* the guidance and saw a bare summary. Strictly worse than before the change. Every crate-level test passed. Only an assertion at the persistence seam — the bytes the model is handed next turn — revealed it. That is exactly the case `.claude/rules/testing.md` describes, and skipping that tier is how this shipped green. The fix is the same move #6684 made for `FailureKind`, not another copy: - `CapabilityRecoveryHint` and `SameCallRetryConstraint` now live in `ironclaw_host_api`, beside `FailureKind` and its `fate()` projection. `host_api` sits below both the emitting crate and the persisting one, so it is the only home where the vocabulary can have a single definition. - `ironclaw_threads` deserializes those enums instead of re-declaring their variants. Drift is now structurally impossible rather than merely tested: a variant added in `host_api` is accepted the moment it exists. - `ironclaw_turns` and `ironclaw_agent_loop` import from the owning crate (CLAUDE.md: import directly from the extracted crate); the `run_profile` re-export is gone. - `retry_after_ms` is accepted as a key. The old list rejected unknown KEYS the same silent way, so #6792's delay field would have dropped observations too. Coverage, red-verified: - `assert_denial_recovery_hint` in the integration harness, asserted in `hook_deny_blocks_capability_without_wedging_run`. Fails on the pre-fix code with `saw [None]` — the seam that caught this. - `every_recovery_hint_the_loop_can_emit_survives_persistence` in `ironclaw_loop_host` (the lowest crate seeing both sides) walks every variant through the real envelope constructor. Fails if a hint OR the delay key stops surviving; verified red both ways. - #6802's degrade test now drives `new_best_effort_model_observation` rather than the private normalizer, per "test through the caller". Gate: host_api, turns, threads, agent_loop, loop_host, runner, hooks — 3,185 tests pass; reborn_integration_hooks 16/16; ironclaw_architecture 85/85; clippy and fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
serrrfirat
added a commit
that referenced
this pull request
Jul 28, 2026
* feat(turns): every failure tells the model what to do next #6284 item 4. `CapabilityRecoveryHint` had two variants and one was a constant: 18 of 19 failure kinds got `RespectFailureConstraint` with an always-empty `repairs` vec. That says "obey the retry rule you already have" — it names no action. A missing credential, an absent runtime, a rate limit and a permanent refusal were all told the same nothing. Six new hint variants, each naming a different next move: `AuthenticateThenRetry`, `RequestApproval`, `WaitThenRetry`, `CompleteSetup`, `UseDifferentCapability`, `ReviseApproach`. `RespectFailureConstraint` survives as the honest answer for genuinely opaque failures, not as the default for everything. `CapabilityRecoveryHint::for_failure_kind` assigns one per kind in a wildcard-free match, so a new `FailureKind` refuses to compile until someone decides what the model should do about it — the same discipline `fate()` uses for the retry/terminal decision. The two answer different questions: `fate()` is what the loop does, this is what the model does. **Denials now carry an observation.** `executor/capabilities.rs` passed `model_observation: None`, so nothing structured reached the model for any denial — no recovery, no retry constraint, no repairs. #6781 made the denial *reason* specific; it was legible only as text glued into the summary string. `deny_recovery` maps each `DenyReason` to its retry constraint and hint, exhaustively. **A delay payload exists.** `retry_after_ms` rides `ToolRecoveryObservation` beside the constraint rather than inside `AllowedAfterDelay`, so the addition is wire-compatible: the constraint still serializes as a bare string and observations persisted before the field still load (pinned by `recovery_observation_without_a_delay_still_loads`). Scope note, stated plainly: **nothing fills `retry_after_ms` yet.** I swept for a producer and there is none on the capability path — `LlmError::RateLimited { retry_after }` is the model-provider path, and `ScopeRecoveryInProgress::retry_after_hint` is consumed internally as backoff and never reaches the model. Threading a value needs a carrier on `RuntimeCapabilityFailure`/`CapabilityFailure` plus a producer that captures `Retry-After`; filed rather than half-built here. Regression coverage, all red-verified: - `only_genuinely_unclassifiable_failures_may_decline_to_name_an_action` is the conformance rule the epic asks for: every kind outside a small opaque set must name an action. Fails against the old constant. - `recovery_hints_stay_distinguishable_across_kinds` guards the collapse returning. - `a_denial_tells_the_model_what_would_unlock_it` drives a real denial through the executor and asserts the appended result carries a recovery observation (it was `None` before). - `recovery_observation_without_a_delay_still_loads` and `recovery_observation_carries_the_providers_requested_wait` pin the wire contract in both directions. Gate: turns, agent_loop, hooks, loop_host, runner, host_api, threads — 3,178 tests pass, clippy clean, fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(agent-loop,threads): a severed cause must say so, and must not take the advice with it (#6802) #6284 item 3, the two boxes that matter for agent-authored code. When a skill or a generated program crashes, the *category* of failure tells the model nothing useful — "your program exited non-zero" is not a fix. The compiler error or traceback is the fix. Both defects below silently damaged exactly that text. **1. Truncation was unmarked.** `bounded_diagnostic_detail` and `truncate_model_observation_text` each sliced to a UTF-8 boundary and returned, leaving no sign the value was cut. A traceback severed mid-frame reached the model looking complete, so the model would fix the wrong line with confidence — worse than receiving no detail at all. Both now route through `truncate_marked`, which appends `" […truncated]"` **inside** the cap rather than on top of it, so the persistence validator's bound still holds. Text that fits is returned unmarked; a marker on a complete value would defeat the distinction. Multi-byte characters are not split when making room. Correction to the epic's framing: it says "the sibling *summary* path appends `"..."`". It does not — `truncate_summary_detail` cuts silently too, and no truncation marker existed anywhere in the product. The structured `next_offset`/`total_bytes` on the *preview* path is a different mechanism and never covered free-text diagnostics. **2. An unstorable diagnostic dropped the whole observation.** `normalized_model_observation` had two narrow repairs; when neither applied it returned `None`, discarding `failure_kind`, `status` and the recovery guidance along with the offending text. Worst precisely where it hurts most: agent-authored code fails as `generic_failure` carrying a raw traceback, which is the text most likely to trip the untrusted content scan — so the model lost the cause *and* the next move. `strip_unstorable_generic_failure_detail` is a third, last-resort repair that drops only the free text. The field is optional in the schema, so the result is a valid observation that still carries the kind and the recovery hint. Losing the cause is a real loss; losing the cause and the advice is a worse one. Regression coverage, all red-verified: - `a_severed_diagnostic_says_it_was_severed`, `a_severed_observation_field_says_it_was_severed` — marker present, cap still honored. - `text_that_fits_is_never_marked` — the distinction stays meaningful. - `marking_a_truncation_respects_utf8_boundaries` — no replacement character from a split boundary. - `an_unstorable_diagnostic_degrades_instead_of_dropping_the_guidance` fails on the old code with "the observation must survive with its guidance intact"; `a_storable_diagnostic_keeps_its_text` pins that the degrade path does not fire on clean text. Stacked on #6792. Gate: threads, agent_loop, turns, loop_host, runner, hooks, host_api — 3,184 tests pass, clippy clean, fmt clean. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * fix(host-api): give the recovery vocabulary one home, and stop losing observations to drift #6792 was broken and its tests could not see it. `ironclaw_threads` validates the persisted observation against a hand-copied list of valid `recovery_hint` / `same_call_retry` strings. It cannot import `ironclaw_turns`, where the enums lived — the two are siblings — so the vocabulary existed twice with nothing keeping the copies in step. #6792 added six recovery hints and missed the copy. Every one was rejected, and rejection is not a loud failure: `normalized_model_ observation` DROPS THE WHOLE OBSERVATION. So every denial that PR was written to improve persisted with no observation at all — the model lost the cause *and* the guidance and saw a bare summary. Strictly worse than before the change. Every crate-level test passed. Only an assertion at the persistence seam — the bytes the model is handed next turn — revealed it. That is exactly the case `.claude/rules/testing.md` describes, and skipping that tier is how this shipped green. The fix is the same move #6684 made for `FailureKind`, not another copy: - `CapabilityRecoveryHint` and `SameCallRetryConstraint` now live in `ironclaw_host_api`, beside `FailureKind` and its `fate()` projection. `host_api` sits below both the emitting crate and the persisting one, so it is the only home where the vocabulary can have a single definition. - `ironclaw_threads` deserializes those enums instead of re-declaring their variants. Drift is now structurally impossible rather than merely tested: a variant added in `host_api` is accepted the moment it exists. - `ironclaw_turns` and `ironclaw_agent_loop` import from the owning crate (CLAUDE.md: import directly from the extracted crate); the `run_profile` re-export is gone. - `retry_after_ms` is accepted as a key. The old list rejected unknown KEYS the same silent way, so #6792's delay field would have dropped observations too. Coverage, red-verified: - `assert_denial_recovery_hint` in the integration harness, asserted in `hook_deny_blocks_capability_without_wedging_run`. Fails on the pre-fix code with `saw [None]` — the seam that caught this. - `every_recovery_hint_the_loop_can_emit_survives_persistence` in `ironclaw_loop_host` (the lowest crate seeing both sides) walks every variant through the real envelope constructor. Fails if a hint OR the delay key stops surviving; verified red both ways. - #6802's degrade test now drives `new_best_effort_model_observation` rather than the private normalizer, per "test through the caller". Gate: host_api, turns, threads, agent_loop, loop_host, runner, hooks — 3,185 tests pass; reborn_integration_hooks 16/16; ironclaw_architecture 85/85; clippy and fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Jul 28, 2026
23 of 29 tasks
This was referenced Jul 29, 2026
16 of 29 tasks
l3ocifer
pushed a commit
to l3ocifer/frick-ironclaw
that referenced
this pull request
Sep 3, 2026
… enums into host_api FailureKind with fate projections (nearai#6284) (nearai#6684) * feat(host_api): unified closed FailureKind (35 variants) with fate projections One failure vocabulary carried unchanged from mint site to loop, replacing the five overlapping enums whose folds destroyed 17 precise mechanism names (and every remediation hint) on the way up (nearai#6284). Closed set — no open Unknown escape hatch; historical tags from the retired coarse vocabularies stay readable via from_tag aliases. Lossless 1:1 injections from the dispatch-lane contract enums replace the retired 22-to-12 coarsening fold. Fates pin the recoverability contract: exactly one variant (Cancelled) may end a run, policy denials never retry, and every wire tag passes the event layer's safe-shape validator (the silent-Unclassified rewrite hazard). Regression tests: fate/tag round-trips over ::ALL, historical-tag aliases, event-layer tag shape, single-terminal pin. * refactor(turns): replace CapabilityFailureKind with unified host_api FailureKind Delete the loop-local CapabilityFailureKind + CapabilityFailureKindValue open-set enum (run_profile/host/capability.rs) and its hand-written serde; ironclaw_turns now carries ironclaw_host_api::FailureKind (the closed 35-variant unified vocabulary from 115648e) everywhere: - CapabilityFailure.error_kind, milestones CapabilityFailed.reason_kind, progress CapabilityActivityFailed.reason_kind, and model_observation GenericFailure.failure_kind are now FailureKind. - resolution::failed takes FailureKind directly; the identity map failure_kind_of is deleted (the types are now the same type). - The CapabilityFailureKind/CapabilityFailureKindValue re-exports in run_profile/mod.rs and run_profile/host/mod.rs are removed — consumers import from ironclaw_host_api (type-placement rule); downstream crates are migrated in later slices. - Old wire tags (invalid_input, invalid_output, process, dispatcher, permanent) stay readable via FailureKind::from_tag aliases; the legacy generic_failure JSON round-trip test still passes unchanged. - Deleted the now-tautological enum-to-enum mapping test failed_carries_its_error_kind_on_the_verdict (it asserted the identity of a map that no longer exists); the remaining diagnostic/verdict tests were updated to construct FailureKind::{InputEncode,Backend} directly. No Unknown-funnel production sites existed in this crate; the only unknown(...) constructions were in the deleted tautology test. Downstream crates that imported ironclaw_turns::CapabilityFailureKind are intentionally broken until their own migration slices land. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(host_runtime): replace RuntimeFailureKind with unified host_api FailureKind Delete the crate-local RuntimeFailureKind (17 coarse variants) and carry ironclaw_host_api::FailureKind (the closed 35-variant unified vocabulary from 115648e) everywhere in ironclaw_host_runtime: - The 22->12 coarsening fold (From<DispatchFailureKind> for RuntimeFailureKind in production.rs) is deleted; the Dispatch arm of failure_kind_from now uses host_api's lossless From injections, so precise mechanism names (MethodMissing, UndeclaredCapability, FilesystemDenied, SecretDenied, NetworkDenied, Client, Executor, Manifest, ExitFailure, OutputDecode, InvalidResult, ...) survive upward instead of being squashed into InvalidInput/InvalidOutput/ Backend/Authorization/Process. - Non-dispatch producers keep their 1:1 names (UnknownCapability invocation error -> MissingRuntime, obligation failures, store outages -> Backend, gate-declined -> GateDeclined, default trait impls -> Unavailable). InvocationFingerprint and the sandbox spawn-input rejections move from the retired InvalidInput to its successor InputEncode. - capability_failure_disposition now delegates to FailureKind::fate() (Retry -> RetrySameCall, everything else -> ModelVisibleToolError); the local runtime_failure_is_retryable set is deleted. Dispositions are preserved per kind; NetworkDenied keeps the retired fold's retryable behavior via an explicit carve-out that the follow-up fix commit removes. - Pinning tests rewritten to pin the lossless mapping; test expectations updated from coarse to precise kinds (that precision is the point of the migration). Events/metrics now carry the precise snake_case tags (e.g. method_missing instead of invalid_input). Downstream crates that imported ironclaw_host_runtime::RuntimeFailureKind are intentionally broken until their own migration slices land. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(host_runtime): NetworkDenied is a policy denial — never retry it The retired coarsening fold mapped the NetworkDenied dispatch kind onto the retryable Network bucket, so egress POLICY denials were quietly retried — policy does not change between attempts, so every retry burned loop budget on a call that could never succeed. The unified FailureKind keeps NetworkDenied distinct with fate() = ModelVisible: the denial surfaces to the model as a tool error it can route around, and it is excluded from the quiet-retry set. This commit removes the mechanical-migration carve-out that had preserved the old retryable disposition, and adds the regression test network_denied_dispatch_failure_is_model_visible_not_retried driving the production failure_from -> disposition() chain. The test fails before this fix — it would also have failed against the retired fold, where a NetworkDenied dispatch failure produced kind Network with disposition RetrySameCall (verified red against the carve-out state). Note (report-surfaced, not changed here): the first-party HTTP mint maps RuntimeHttpEgressReasonCode::NetworkError (transport failures, offline egress) to the NetworkDenied dispatch kind, so those now also surface model-visibly instead of retrying; the dispatch-lane vocabulary has no transport-network kind for the mint to use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(host_api,host_runtime): transport network faults get their own dispatch kind The dispatch lane had no transport-network variant, so the first-party HTTP mint routed RuntimeHttpEgressReasonCode::NetworkError (unreachable, reset, timeout) onto NetworkDenied — the egress POLICY kind. After NetworkDenied correctly stopped retrying, genuine network blips would have stopped retrying with it. RuntimeDispatchErrorKind::Network now exists, injects 1:1 into the unified FailureKind::Network (fate: Retry), and the mint distinguishes transport faults from policy denials. Regression surface: builtin_http_offline_runtime_egress test now pins transport offline egress as retryable Network, while the three policy-denial sites stay pinned NetworkDenied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(loop_host,composition): carry unified host_api FailureKind through the loop seam Migrate ironclaw_loop_host and ironclaw_reborn_composition onto the closed 35-variant ironclaw_host_api::FailureKind (115648e), deleting the two identity maps that shuttled the now-unified vocabulary between the retired RuntimeFailureKind and CapabilityFailureKind enums: - capability_port.rs: runtime_failure_kind_to_loop and model_visible_runtime_failure_kind_to_loop mapped RuntimeFailureKind -> CapabilityFailureKind name-for-name; both types are now the same type, so the functions are deleted and failure.kind is used directly. - The open-set capability_failure_kind(String) funnel is replaced at its two call classes: AgentLoopHostErrorKind funnel sites (terminal capability- failure milestones) now go through the new exhaustive, wildcard-free failure_kind_for_host_error_kind match (Unauthorized/ScopeMismatch -> Authorization, CredentialUnavailable -> AuthRequired, StaleSurface -> StaleSurface, InvalidInvocation/Invalid -> InputEncode, InvalidOutput -> OutputDecode, ContentFiltered -> OperationFailed, PolicyDenied -> PolicyDenied, Budget* -> Resource, Unavailable -> Unavailable, Cancelled -> Cancelled, CheckpointRejected/TranscriptWriteFailed/Internal -> Internal); RuntimeCapabilityOutcome::Unknown tag-string sites parse via the total FailureKind::from_tag (unrecognized legacy open-set tags land on Internal instead of erroring the run with "could not be represented"). - denied_reason_kind_for keeps emitting the literal "auth_denied" for Authorization: the loop-safe identifier validator rejects the substring "authorization", and that special case remains load-bearing (pinned by the existing conversion tests). - Retired coarse names are renamed at their surviving sites: InvalidInput -> InputEncode, InvalidOutput -> OutputDecode. - ironclaw_reborn_composition: mechanical import/rename migration of its reference sites (runtime/local_dev/{project_create,result_read, skill_activation,outbound_delivery}, product_capability, approval_test_support, factory tests, service_factory tests). No behavior changes. - ironclaw_capabilities: doc-comment reference updated (no code change). Deleted the now-tautological sync-parity test runtime_failure_kind_mapping_preserves_current_categories (it asserted agreement between the two deleted enums, which no longer exist as distinct types). Updated the invalid-request milestone tests to pin the precise InputEncode reason kind, and rewrote runtime_capability_unknown_outcome_with_invalid_kind_does_not_emit_failure_milestone as ..._with_wild_kind_maps_to_internal_failure: the closed vocabulary's total from_tag makes every tag representable, so a wild unknown-outcome tag now becomes a model-visible Internal failure with its milestone instead of a run-ending internal error. ironclaw_reborn_composition still fails to build against this commit because its dependency ironclaw_agent_loop has not yet been migrated (separate slice); the composition changes here are the mechanical rename half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(loop_host): deny malformed model-supplied spawn input instead of ending the run SubagentSpawnCapabilityPort::invoke_capability and the pre-decode pass in invoke_capability_batch used `?` on spawn_input_codec.decode, which decodes MODEL-SUPPLIED JSON. A malformed spawn_subagent payload (bad JSON shape, schema-violating wire args, model-correctable rejections like requesting the disabled background mode) therefore propagated as Err(AgentLoopHostError), which the executor maps to a run-ending HostUnavailable — killing the whole run on output the model could simply correct. Route InvalidInvocation decode failures to the existing spawn_rejected -> Resolution::Denied channel (reason tag invalid_spawn_input, carrying the codec's sanitized summary) so the model sees a denial and can retry with fixed input; genuine host faults (input-store outages and every other error kind) still propagate as errors. In the batch path the denial is parked per-invocation so sibling invocations keep their positions and the batch no longer aborts. Regression tests (fail before this fix): invoke_spawn_denies_malformed_ model_input_without_side_effects and invoke_spawn_batch_denies_malformed_ model_input_without_side_effects drive the real JsonSpawnSubagentInputCodec through the port with malformed JSON and a background-mode request, assert a Denied resolution with the corrective summary, and assert no spawn side effects (no child runs, goals, or await edges). They replace the two propagates_decode_rejection tests that pinned the old run-ending behavior; the unused RejectingSpawnInputCodec fixture is removed with them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(agent_loop): delete CapabilityErrorClass, wire recovery to FailureKind fates Replace the loop-private CapabilityFailureKind/CapabilityErrorClass pair with the unified ironclaw_host_api::FailureKind and its fate() projection: - CapabilityErrorSummary now carries `kind: FailureKind`; the class enum and its re-declared recoverability domain are gone. - DefaultRecoveryStrategy::on_capability_error is a wildcard-free match on FailureFate (Retry/ModelVisible/Park/Terminal); the `_ => Abort(DriverBug)` catch-all is dead. Park (AuthRequired) surfaces model-visibly — gates still ride Resolution::Blocked. - The three byte-identical kind→LoopFailureKind mappings collapse into one pub(crate) capability_error_to_failure_kind in strategies/recovery.rs, wildcard-free over the full vocabulary. - capability_error_failure_category keeps the seven-string wire contract with the runner/product layers; StaleSurface pins "capability_policy_denied" (the retired mint sites lied with PolicyDenied; the wire category must not change under the honest rename) and "capability_permanent" is reachable only through Cancelled. - Direct mint sites get honest kinds: surface-filtered and Resolution::Denied mint PolicyDenied; the stale-surface host-error branches mint StaleSurface. - The serde round-trip funnel (capability_failure_kind_from) is deleted; the verdict already carries the unified kind. - Behavior deltas from the prescribed kind merges: the retired Permanent kind (now OperationFailed) is model-visible instead of run-ending, and Dispatcher (now Internal) retries instead of surfacing; the terminal capability set is exactly {Cancelled}. Tests that used Permanent as the abort vehicle for the failure-explanation flow now drive it through the iteration-limit exit, and the classification-lock tests pin fate()-driven outcomes and the seven-string category set over FailureKind::ALL. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(runner,webui,product,composition): migrate to unified FailureKind Completes the mechanical migration: runner milestone/disclosure seams, webui HTTP status mapping (exhaustive over the unified vocabulary, statuses preserved per retired variant), product projections, composition runtime services, and extension_host test support all consume ironclaw_host_api::FailureKind directly. Precision upgrades ride the wire: missing first-party handler now reports undeclared_capability (was invalid_input), skill-write denial reports filesystem_denied (was authorization) — same fates, same retry buckets. Verified per-crate before the disk-full interruption and re-verified after on a clean build: runner, extension_host, product suites green; composition green except pre-existing frontend-toolchain static-asset failures (SKIP_FRONTEND_BUILD machine issue, tracked separately). Zero-warning clippy across all six migrated crates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(agent_loop,turns,loop_host): capability port errors recover by fate instead of ending the run capability_host_error mapped EVERY non-Cancelled capability-stage port Err(AgentLoopHostError) to a run-ending HostUnavailable{Capability}, so Unauthorized / ScopeMismatch / InvalidInvocation / CredentialUnavailable / PolicyDenied etc. from the ~66 loop_host mint sites silently killed runs the model could have recovered (epic nearai#6284 item 1). - The kind projection moves to its owner: AgentLoopHostErrorKind::failure_kind (crates/ironclaw_turns/src/run_profile/host/error.rs), replacing loop_host's private failure_kind_for_host_error_kind byte-for-byte (table exactly as slice 2fbcdea committed it); both loop_host milestone sites now call the method. - The executor splits capability-stage port Errs by an exhaustive, wildcard-free terminal predicate (capability_port_error_is_terminal): genuine host faults (Cancelled, Unavailable, Internal, BudgetExceeded, BudgetApprovalRequired, BudgetAccountingFailed, CheckpointRejected, TranscriptWriteFailed) keep today's terminal capability_host_error path; caller-shaped kinds (Unauthorized, CredentialUnavailable, ScopeMismatch, StaleSurface, InvalidInvocation, Invalid, InvalidOutput, ContentFiltered, PolicyDenied) are routed through handle_capability_error at both dispatch sites (batch + retry), so the model sees a tool-error observation carrying the unified FailureKind and the secret-scrubbed detail, and the recovery strategy dispositions it by FailureKind::fate. - The four .map_err(capability_host_error) sites in capability_helpers.rs are NOT dispatch — they are gate-record reads and transcript result-ref appends (surface/definition plumbing) — and deliberately stay terminal. - Every self.handle_capability_error(...) await site is now Box::pin'ed: the executor future was at the stack margin (the inline arm made reborn_integration_tool_call's current_tool_surface_overrides_stale_assistant_unavailable_claim overflow its test-thread stack; boxing the sites removes the repeated inline embedding of that huge future). Behavior-neutral. Regression test: recoverable_batch_port_error_surfaces_as_model_visible_tool_error (crates/ironclaw_agent_loop/src/executor/tests.rs) — a scripted batch port Err(Unauthorized) must complete the run with a GenericFailure{Authorization} tool observation; red-verified pre-fix failing with HostUnavailable{Capability}. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(ci): suppress pre-commit-safety panic flag on test-only assertion pre-commit-safety's PANIC check flagged the new FailureKind assertion in composition's local_dev_host_tests (a test-only module outside the script's /tests/ path heuristic); annotate it with the file's existing "// safety: test-only assertion." convention. No ratchet re-baselines were needed: composition budget is 770bp vs 2428bp effective ceiling, and the coverage-floor denominator moved ~0.2% (immaterial per its own header). [skip-regression-check] CI-gate annotation only; no production behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(host_api,host_runtime): add Unclassified — unclassifiable failures surface, never retry `Internal` was doing two incompatible jobs: the retryable host-fault bucket AND the sink for anything the system could not classify (`from_tag`'s unrecognized-tag fallback, `RuntimeDispatchErrorKind::Unknown`'s redaction bucket). Because `Internal` is Retry-fated, every unclassifiable failure — possibly permanent — silently consumed retry budget before the model ever saw it. Adds a 36th variant `Unclassified` (tag `unclassified`, fate ModelVisible, never retryable) and routes both unknown funnels to it. `Internal` now means exactly one thing: a classified, retryable host fault. Also compiler-owns `ALL`: the enum, its wire tags, and `ALL` now expand from a single `declare_failure_kinds!` list, so a new variant cannot exist outside `ALL` and the downstream exhaustiveness pins that iterate it (host_runtime's production disposition pin, agent_loop's category and recovery pins) cannot silently stop covering it. Previously a variant could be added by touching only `fate`/`as_str`/`from_tag` and every `ALL`-driven pin would keep passing. Second honesty fix in the same lane: a `CapabilityObligationFailureKind:: Network` failure is always deterministic policy/config (duplicate ApplyNetworkPolicy, empty allowed_targets, missing policy store) — never a transport fault. It now maps to the never-retryable `NetworkDenied` instead of the retryable transport `Network`, with a regression test. Softens the `capability_failure_disposition` doc: the "Park and Terminal never reach this on production paths" claim is intent, not a code-enforced invariant, and is now stated as such. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(turns): honest fates for budget/checkpoint port errors Three deliberately distinct dispositions were collapsing into one in `AgentLoopHostErrorKind::failure_kind()`, against what the variant docs prescribe: - `BudgetApprovalRequired` documents a PARK semantic ("callers surface an approval gate and retry after the user resolves it"), but mapped to `Resource` (ModelVisible) — handing the model a tool error to route around instead of parking on the gate. Now `AuthRequired` (Park). - `BudgetAccountingFailed` documents "callers must fail closed" and is explicitly distinct from the budget outcome, but folded into the same model-visible `Resource` bucket. Now `Unclassified` — not a quota the model can work around, and not retryable. - `CheckpointRejected` mapped to `Internal`, which is Retry-fated. A schema id/version mismatch is deterministic and cannot succeed on re-attempt; retrying only burns budget. Now `OperationFailed` (non-retryable). `BudgetExceeded` keeps `Resource` — that one really is a quota the model can work around. agent_loop's terminal set is unchanged (Budget*/Checkpoint* remain terminal there), so this projection governs the milestone/observation surface; both sides are now honest independently. Regression test pins all four projections and their retryability. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(composition): precise failure kinds for skill-activation and result-read denials Three local-dev mints named the wrong mechanism, losing the precision the unified vocabulary exists to carry (same fate in every case, so no recoverability change): - skill activation `ContextBudgetExceeded` is a resource limit, not an encoding fault → `Resource` (`AmbiguousSkill` stays `InputEncode`); the test helper now asserts the expected kind per case instead of assuming one kind for both. - `unavailable_result_reference()` — a result ref absent from this thread — is a domain failure of the read, not malformed input → `OperationFailed`. Deliberately NOT `Unavailable`: that is Retry-fated and would quietly retry a ref that can never appear (genuine storage faults already take the separate `AgentLoopHostErrorKind::Unavailable` path). - `non_text_result_content()` is an output-decode failure → `OutputDecode`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(webui): a quota failure is not a retryable 503 `capability_failure_http_class` sent `FailureKind::Resource` through the `Unavailable` class — documented as "the backing service hiccuped; retryable (503)" — while `fate()` classifies `Resource` as ModelVisible and non-retryable. WebUI callers were being told to retry a quota/limit failure that can never succeed. It now takes the non-retryable internal path. Adds `http_class_retryability_agrees_with_failure_kind_fate`, an exhaustive pin over `FailureKind::ALL` asserting no non-retryable kind rides the retryable-503 class (`StaleSurface` is the one documented exception: at the HTTP seam a re-issued request races a refreshed surface and can succeed). Also dedupes the twice-inlined Forbidden literal in `admin_configuration_done_failure` into `admin_configuration_forbidden()`, mirroring the existing `admin_configuration_unavailable()` helper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: address PR 6684 review — harden assertions and align tag expectations Test-only follow-ups from the review, plus the CI expectation fallout of the precise-rename slices in this PR. Hardened assertions: - failure_matrix's `OperationFailed` recovery check was a soft `if let Some` that no-oped when no observation was present — now `.expect(...)`, matching its `InvalidOutput` sibling. Hardening it exposed that the row scripted a non-provider call (`calls_response()`, `provider_replay: None`), and model-visible observations are persisted on provider result refs only, so the assertion had never actually run. The row now uses `provider_calls_response()` and genuinely proves the cause survives. - the five `error_kind()`-only tool-verdict checks in tool_disclosure_port / loop_driver_host now match `ToolVerdict::RecoverableFailure { .. }` explicitly. - `generic_failure_deserializes_legacy_json_without_detail` also covers a retired coarse tag (`invalid_input`) decoding through `from_tag`'s alias. - new `malformed_provider_tool_call_registration_errors_stay_model_repairable` pins the model_gateway seam both the validate and register loops call: a malformed model-supplied provider tool call (bad spawn_subagent JSON) maps to the model-stage InvalidOutput repair lane, never a run-ending host fault. Expectation alignment (the renames this PR performs, and one fate change): - `invalid_input` → `input_encode` (outbound_target, project_create, group_extensions install), `backend` → `client` (MCP client faults). - `RecordingTestCapabilityPort::invocation_error()` now mints the terminal `Unavailable` instead of `InvalidInvocation`: caller-shaped port errors recover in-loop by fate now, which defeats the run-failed → user-retry journey this double exists to drive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(host_runtime): unsupported operations are permanent, not retryable The `HostRuntime` default trait bodies for `spawn_capability`, `auth_resume_capability`, and `resume_spawn_capability` minted `FailureKind::Unavailable` for "unsupported by this host runtime". `Unavailable.fate()` is `Retry`, so a permanently-unsupported operation burned the whole capability retry budget on a call that can never succeed — the same budget-burn class this PR already fixed for `NetworkDenied`. All three sites mean "this implementation does not provide the operation", which is a permanent property of the implementation, never a temporary outage; none of them means "the backing service is down right now". The honest kind is `FailureKind::UnsupportedRunner`: model-visible and non-retryable, so the loop surfaces the gap instead of retrying it. This was rename-preserved behavior carried over from the retired `RuntimeFailureKind` vocabulary, not a regression introduced by this PR; the unified enum simply made the wrong fate visible. Regression test: `unsupported_operation_defaults_are_permanent_not_retryable` drives a `HostRuntime` that overrides only the required methods, so every optional operation falls through to its default body, and pins kind, `is_retryable()`, and `fate()` for all three. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(runner): drive malformed spawn-input recovery through the provider-tool caller The regression test added last round called `map_provider_tool_output_error` directly. `.claude/rules/testing.md` ("Test Through the Caller, Not Just the Helper") requires driving the real caller when a classifier gates a side effect with a wrapper in between, and this case matches exactly: the gateway derives the classifier's input from the provider response, and two separate loops (validation and registration) call it. Adds `malformed_spawn_subagent_input_is_model_repairable_through_the_gateway`, which submits a malformed `builtin.spawn_subagent` call through the real path — `LlmProviderModelGateway::stream_model_with_capabilities` -> `complete_model_request` -> `tool_response_to_host` -> the capability port — and asserts, for a rejection at each stage, that the result is a model-visible `InvalidOutput`, that no capability call is registered, and that the in-gateway repair retry does not fire, so the error reaches the loop's invalid-output recovery. The gateway is the nearest reachable seam for `RetryAlteration::RepairInvalidModelOutput`: the remaining hops are `pub(crate)`/`pub(super)` in their own crates and are already pinned there (`ironclaw_loop_host` model-gateway error mapping, `ironclaw_agent_loop` `executor::mapping` and `model_invalid_output_retries_then_observes_once_before_abort`). The helper-level test stays as the unit pin. Adds a registration-stage rejection knob to the existing `GatewayCapabilityPort` double so the second loop is genuinely reached rather than short-circuited by validation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(runner): tie the spawn-registration rejection to the malformed payload The GatewayCapabilityPort double rejected at registration whenever the error knob was armed, without inspecting the payload — so malformed_spawn_subagent_input_is_model_repairable_through_the_gateway proved that an injected error routes to model-visible InvalidOutput, not that the missing `mission` field is what triggers it. The double now rejects only when `mission` is absent, and a control case drives the same armed error with a well-formed payload and asserts it registers. Red-verified: with the payload check removed the control case fails at the registration assertion (llm_gateway.rs:1070); green with it. Full llm_gateway suite 78 passed, clippy clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(reborn): pin the failure-vocabulary collapse and the recovery it buys Two gaps against `.claude/rules/testing.md` that this PR was relying on without saying so. 1. Architecture gate (`crates/ironclaw_architecture/tests/ reborn_retired_failure_vocabulary.rs`). The rule requires an architecture test when ownership changes, and this PR moved four types out of three crates. Without a gate the collapse is a one-time cleanup: nothing stops a failure enum being re-declared elsewhere next month, which is the drift the PR exists to remove. Pins the four retired type names and the six retired cross-spelling mappers at zero live occurrences, following the `reborn_retired_taxonomy.rs` precedent. Comments are exempt on purpose — prose explaining what was retired is worth keeping, so only code is policed. A second test pins that the survivor stays closed (`Unclassified` present, no payload-carrying open-set variant, not `#[non_exhaustive]`); without it a reintroduced escape hatch would let a producer skip classification without ever naming a retired type. Red-verified: re-adding `pub enum CapabilityFailureKind` to ironclaw_turns is caught with file and line. 2. Integration test (`tests/integration/tool_call.rs`). The rule allows crate-tier as a fallback only when the integration harness cannot reach the path, and says to explain that in the PR — this PR made no such claim, it simply had no integration coverage for its headline behavior. Extends the suite that owns the capability-dispatch seam rather than adding a binary. Asserts at the durable seam (persisted ToolResultReference + finalized reply), not `wait_for_status(Completed)`. Red-verified against pre-fix behavior — reverting `capability_port_error_is_terminal` to all-terminal yields: expected Completed but run reached terminal status Failed; failure=host_stage_unavailable_capability, detail: None which is the bug in one line: a model-fixable invocation error reported as a host outage, with no detail, killing the turn. Harness: adds a `RecoverablePortErrorEcho` capability backend selecting a caller-shaped port error from the existing double — the same shape as the `no_progress()` / `invocation_error()` modes, not test-only wiring for unwired behavior. `InvalidInvocation` rather than `Unauthorized` on purpose, recorded at the double: `Authorization`'s summary prefix contains the banned marker "authorization:", so that kind fail-softs to the redacted fallback and would hide the kind the test asserts on. Verified: architecture suite green; full reborn_integration_tool_call 30 passed (the shared double is used by all 30, so the whole binary ran, not just the new case). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(test): the vocabulary gate must not fail open, and must keep its own promise Four review findings on the tests added in the previous commit. Two are substantive; both are the failure mode this PR exists to remove, committed in the code meant to prevent it. 1. The gate failed open. `read_dir`, directory entries, and `read_to_string` errors were silently skipped, so a scan that broke — bad path, permissions, a rename — would have passed while checking nothing. A guard that cannot fail is worse than no guard because it manufactures confidence. Every IO error now propagates with its path. (The silent-skip shape was inherited from `reborn_retired_taxonomy.rs`, which has the same flaw. Out of scope here; worth a follow-up.) 2. Block comments were not exempt. The module doc promises comments are exempt so prose explaining the retired vocabulary stays legal, but the implementation only stripped `//`, so a `/* … */` or `/** … */` naming a retired type would have failed the build. `strip_comments` now blanks both forms, preserving newlines so reported line numbers stay accurate. Verified in both directions with a temporary probe file: a block comment naming `CapabilityFailureKind`/`RuntimeFailureKind` passes; adding `pub enum CapabilityErrorClass` to the same file is caught at `_probe_block.rs:5`. 3. `builder.rs` — the inserted helper had orphaned the no-progress helper's doc comment onto itself. Restored to its own function. 4. `capability_backend.rs` — doc still said `Unauthorized` after the double was switched to `InvalidInvocation` (switched because `Authorization`'s summary prefix contains the banned marker "authorization:", so it fail-softs to the redacted fallback and would hide the kind the test asserts on). Verified: architecture suite 2 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(test): nest-aware comment stripping, its own coverage, and the sibling doc Follow-up on the review of the previous commit. Two real gaps, one of them the reviewer's second pass on a fix I had called done. 1. Nested block comments. Rust permits `/* outer /* inner */ still comment */`, and the bool-based stripper closed at the FIRST `*/`, so a retired name after an inner comment was scanned as live code. Now a depth counter. Verified by reverting depth to a bool: `nested_block_comments_do_not_end_early` fails on the reviewer's own example. 2. The comment contract had no tests. It is a promise this gate makes to every file it scans, and an exemption rule nobody tests is one that silently changes meaning — the same class as the gate failing open. Five cases now cover it: line comments (and that code *before* a trailing comment still scans), multiline blocks, `/** */` doc blocks, nesting, code after a closed block, and line-number preservation (hits are reported by line, so blanking must keep newlines). 3. `group.rs:321` still said `Unauthorized`. The previous commit fixed only the anchor file the reviewer cited and missed the sibling site in the same consolidated finding. Verified: 7 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
l3ocifer
pushed a commit
to l3ocifer/frick-ironclaw
that referenced
this pull request
Sep 3, 2026
* fix: make capability diagnostics actionable * test: align diagnostic contract after rebase * fix: address review on the diagnostic boundary, and repair my own rebase artifact Three review threads plus a formatting failure I introduced. 1. Silent swallow at the sanitized resolution boundary (`run_profile/resolution.rs`). `Err(_)` discarded the `HostApiError` and substituted the fixed fallback with no signal anywhere. Reaching that arm means a producer let credential-shaped text travel to the boundary — exactly the event an operator wants to see. Now carries the required `// silent-ok:` rationale and binds the cause on `debug!` (`info!`/`warn!` corrupt the REPL/TUI per repo CLAUDE.md). 2. Stale contract doc (`run_profile/model_observation.rs`). The `CapabilityFailureDetail` docs still promised untrusted output "keeps collapsing to the safe-summary placeholder" — the behavior this PR replaced. Rewritten to state what is now true: `Diagnostic` preserves the scrubbed cause and fails closed to the fixed sentence only for credential-shaped values, so the trusted arm exists for PROVENANCE rather than redaction strength. 3. Formatting CI failure — mine, from the nearai#6684 rebase. `tests/integration/ mcp.rs` carried a dangling `.expect(...)` fragment whose receiver the merge had consumed, i.e. a syntax error. My rebase verification ran tests on three library crates and never compiled the integration tests, so a syntax error sat in the tree while I reported the rebase verified. Removed; the assertions this branch added above it already cover the same ground, and `cargo check -p ironclaw_reborn_integration_tests --tests` is now clean. Not changed: the thread claiming the preserved diagnostic is dropped downstream because the transcript validator rejects credential vocabulary and injection phrases. `validate_model_observation_detail` rejects only empty, over-cap and control characters — `"password field is required"` passes — and this branch's own `generic_failure_detail_allows_paths_and_payload_delimiters` and the `tool_result_reference` diagnostic test already pin that a path-bearing detail validates and round-trips. That finding predates this branch's second commit. Verified: 1475 passed / 0 failed across ironclaw_host_api, ironclaw_loop_host, ironclaw_turns; clippy --all-targets --all-features -D warnings clean; cargo fmt --all --check clean; integration tests compile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(host_api): a corpus that fails when the credential guard gets greedy Every existing test on this channel proves the guard REJECTS what it should. Nothing proved it still ACCEPTS what it should, so a tightened detector could only break silently — and a diagnostic that says nothing is indistinguishable from one that had nothing to say. Strictness could ratchet up unnoticed forever. Adds the other direction: eleven realistic diagnostics that must survive `ModelDiagnostic::new` — bare credential vocabulary ("the api key is missing from the request"), remediation prose that has to name what the user must go fix ("rotate your client secret in the provider console"), paths and schema refs, and already-redacted payloads. The four value cases sit beside them, so tightening one direction cannot quietly loosen the other. Red-verified: adding a single greedy clause (`lower.contains("api key")`) to `contains_unredacted_credential_value` fails with over-scrubbed a legitimate diagnostic: "the api key is missing from the request" rejected as InvalidModelDiagnostic — a detector got greedy; the model needs this sentence Worth recording what the corpus showed on its first run: every entry already passed. This channel's values-only rules are correct as written. The over-scrubbing felt elsewhere comes from WHICH detector guards WHICH channel — `safe_summary.rs` and `model_result_preview.rs` still gate on the crude whole-word marker list, so a sentence dies for containing "password" at all. That is what makes `FailureKind::Authorization` unable to name itself (nearai#6284). This corpus is the guardrail that makes migrating those two callers safe: it fails if they are loosened too far, and it fails if anyone tightens them back. Verified: 1476 passed / 0 failed; clippy --all-targets --all-features -D warnings clean; cargo fmt --all --check clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
l3ocifer
pushed a commit
to l3ocifer/frick-ironclaw
that referenced
this pull request
Sep 3, 2026
* feat(turns): every failure tells the model what to do next nearai#6284 item 4. `CapabilityRecoveryHint` had two variants and one was a constant: 18 of 19 failure kinds got `RespectFailureConstraint` with an always-empty `repairs` vec. That says "obey the retry rule you already have" — it names no action. A missing credential, an absent runtime, a rate limit and a permanent refusal were all told the same nothing. Six new hint variants, each naming a different next move: `AuthenticateThenRetry`, `RequestApproval`, `WaitThenRetry`, `CompleteSetup`, `UseDifferentCapability`, `ReviseApproach`. `RespectFailureConstraint` survives as the honest answer for genuinely opaque failures, not as the default for everything. `CapabilityRecoveryHint::for_failure_kind` assigns one per kind in a wildcard-free match, so a new `FailureKind` refuses to compile until someone decides what the model should do about it — the same discipline `fate()` uses for the retry/terminal decision. The two answer different questions: `fate()` is what the loop does, this is what the model does. **Denials now carry an observation.** `executor/capabilities.rs` passed `model_observation: None`, so nothing structured reached the model for any denial — no recovery, no retry constraint, no repairs. nearai#6781 made the denial *reason* specific; it was legible only as text glued into the summary string. `deny_recovery` maps each `DenyReason` to its retry constraint and hint, exhaustively. **A delay payload exists.** `retry_after_ms` rides `ToolRecoveryObservation` beside the constraint rather than inside `AllowedAfterDelay`, so the addition is wire-compatible: the constraint still serializes as a bare string and observations persisted before the field still load (pinned by `recovery_observation_without_a_delay_still_loads`). Scope note, stated plainly: **nothing fills `retry_after_ms` yet.** I swept for a producer and there is none on the capability path — `LlmError::RateLimited { retry_after }` is the model-provider path, and `ScopeRecoveryInProgress::retry_after_hint` is consumed internally as backoff and never reaches the model. Threading a value needs a carrier on `RuntimeCapabilityFailure`/`CapabilityFailure` plus a producer that captures `Retry-After`; filed rather than half-built here. Regression coverage, all red-verified: - `only_genuinely_unclassifiable_failures_may_decline_to_name_an_action` is the conformance rule the epic asks for: every kind outside a small opaque set must name an action. Fails against the old constant. - `recovery_hints_stay_distinguishable_across_kinds` guards the collapse returning. - `a_denial_tells_the_model_what_would_unlock_it` drives a real denial through the executor and asserts the appended result carries a recovery observation (it was `None` before). - `recovery_observation_without_a_delay_still_loads` and `recovery_observation_carries_the_providers_requested_wait` pin the wire contract in both directions. Gate: turns, agent_loop, hooks, loop_host, runner, host_api, threads — 3,178 tests pass, clippy clean, fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(agent-loop,threads): a severed cause must say so, and must not take the advice with it (nearai#6802) nearai#6284 item 3, the two boxes that matter for agent-authored code. When a skill or a generated program crashes, the *category* of failure tells the model nothing useful — "your program exited non-zero" is not a fix. The compiler error or traceback is the fix. Both defects below silently damaged exactly that text. **1. Truncation was unmarked.** `bounded_diagnostic_detail` and `truncate_model_observation_text` each sliced to a UTF-8 boundary and returned, leaving no sign the value was cut. A traceback severed mid-frame reached the model looking complete, so the model would fix the wrong line with confidence — worse than receiving no detail at all. Both now route through `truncate_marked`, which appends `" […truncated]"` **inside** the cap rather than on top of it, so the persistence validator's bound still holds. Text that fits is returned unmarked; a marker on a complete value would defeat the distinction. Multi-byte characters are not split when making room. Correction to the epic's framing: it says "the sibling *summary* path appends `"..."`". It does not — `truncate_summary_detail` cuts silently too, and no truncation marker existed anywhere in the product. The structured `next_offset`/`total_bytes` on the *preview* path is a different mechanism and never covered free-text diagnostics. **2. An unstorable diagnostic dropped the whole observation.** `normalized_model_observation` had two narrow repairs; when neither applied it returned `None`, discarding `failure_kind`, `status` and the recovery guidance along with the offending text. Worst precisely where it hurts most: agent-authored code fails as `generic_failure` carrying a raw traceback, which is the text most likely to trip the untrusted content scan — so the model lost the cause *and* the next move. `strip_unstorable_generic_failure_detail` is a third, last-resort repair that drops only the free text. The field is optional in the schema, so the result is a valid observation that still carries the kind and the recovery hint. Losing the cause is a real loss; losing the cause and the advice is a worse one. Regression coverage, all red-verified: - `a_severed_diagnostic_says_it_was_severed`, `a_severed_observation_field_says_it_was_severed` — marker present, cap still honored. - `text_that_fits_is_never_marked` — the distinction stays meaningful. - `marking_a_truncation_respects_utf8_boundaries` — no replacement character from a split boundary. - `an_unstorable_diagnostic_degrades_instead_of_dropping_the_guidance` fails on the old code with "the observation must survive with its guidance intact"; `a_storable_diagnostic_keeps_its_text` pins that the degrade path does not fire on clean text. Stacked on nearai#6792. Gate: threads, agent_loop, turns, loop_host, runner, hooks, host_api — 3,184 tests pass, clippy clean, fmt clean. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * fix(host-api): give the recovery vocabulary one home, and stop losing observations to drift nearai#6792 was broken and its tests could not see it. `ironclaw_threads` validates the persisted observation against a hand-copied list of valid `recovery_hint` / `same_call_retry` strings. It cannot import `ironclaw_turns`, where the enums lived — the two are siblings — so the vocabulary existed twice with nothing keeping the copies in step. nearai#6792 added six recovery hints and missed the copy. Every one was rejected, and rejection is not a loud failure: `normalized_model_ observation` DROPS THE WHOLE OBSERVATION. So every denial that PR was written to improve persisted with no observation at all — the model lost the cause *and* the guidance and saw a bare summary. Strictly worse than before the change. Every crate-level test passed. Only an assertion at the persistence seam — the bytes the model is handed next turn — revealed it. That is exactly the case `.claude/rules/testing.md` describes, and skipping that tier is how this shipped green. The fix is the same move nearai#6684 made for `FailureKind`, not another copy: - `CapabilityRecoveryHint` and `SameCallRetryConstraint` now live in `ironclaw_host_api`, beside `FailureKind` and its `fate()` projection. `host_api` sits below both the emitting crate and the persisting one, so it is the only home where the vocabulary can have a single definition. - `ironclaw_threads` deserializes those enums instead of re-declaring their variants. Drift is now structurally impossible rather than merely tested: a variant added in `host_api` is accepted the moment it exists. - `ironclaw_turns` and `ironclaw_agent_loop` import from the owning crate (CLAUDE.md: import directly from the extracted crate); the `run_profile` re-export is gone. - `retry_after_ms` is accepted as a key. The old list rejected unknown KEYS the same silent way, so nearai#6792's delay field would have dropped observations too. Coverage, red-verified: - `assert_denial_recovery_hint` in the integration harness, asserted in `hook_deny_blocks_capability_without_wedging_run`. Fails on the pre-fix code with `saw [None]` — the seam that caught this. - `every_recovery_hint_the_loop_can_emit_survives_persistence` in `ironclaw_loop_host` (the lowest crate seeing both sides) walks every variant through the real envelope constructor. Fails if a hint OR the delay key stops surviving; verified red both ways. - nearai#6802's degrade test now drives `new_best_effort_model_observation` rather than the private normalizer, per "test through the caller". Gate: host_api, turns, threads, agent_loop, loop_host, runner, hooks — 3,185 tests pass; reborn_integration_hooks 16/16; ironclaw_architecture 85/85; clippy and fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This branch was successfully deployed
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
2 participants
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.
Summary
Collapses five overlapping failure-kind enums into one closed
ironclaw_host_api::FailureKind(36 variants) plus projection functions, and fixes six wrongful-terminal / mis-retry bugs the collapse exposed — each with a red-verified regression test.The five enums all answered one question — why did this operation fail — at different altitudes, and the folds between them destroyed 17 mechanism-precise names (and with them every remediation hint the model could act on).
fate()now decides Retry / ModelVisible / Park / Terminal once, in a wildcard-free match beside the single definition, so a new kind cannot compile until every consumer classifies it. Terminal is pinned by test to{Cancelled}at the capability stage.Supersedes #6677 (closed). Detail below.
Change Type
Linked Issue
#6284 (items 1, 4's
InvalidInputsplit, and 7's compile-forced gate)Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warnings— ran the workspace-wide equivalent, both feature lanescargo build— not run separately; clippy compiled the full target graphironclaw_host_api,ironclaw_turns(634),ironclaw_host_runtime(26 targets),ironclaw_loop_host,ironclaw_agent_loop(430 lib),ironclaw_runner,ironclaw_product,ironclaw_reborn_composition,ironclaw_extension_host,ironclaw_architecture,reborn_integration_tool_call(30)cargo test --features integration— not applicable; no DB-backed behavior changed. The affected paths ran through their hermetic Reborn integration binaries.review-prorpr-shepherd --fixwas run before requesting reviewTest Strategy
from_tagaliasesNetworkDenied,PolicyDenied) are now provably non-retryableRisk areas: the terminal-set split at the capability stage, and the wire-tag rename on persisted
error_kind.Integration tier:
caller_shaped_capability_port_error_is_a_tool_error_not_a_dead_rundrives a caller-shaped port error through production composition and asserts at the durable seam (persistedToolResultReference+ finalized reply), notwait_for_status(Completed). Red-verified: reverting the fix yieldsexpected Completed but run reached terminal status Failed; failure=host_stage_unavailable_capability, detail: None.Security Impact
Two changes tighten behavior, none loosen it:
NetworkDeniedwas mapped onto a retryable transport kind, so a call denied by egress policy burned the full retry budget re-attempting something policy would never permit.FailureKindis closed — no openUnknown(String)escape hatch. Unclassifiable failures land on the explicitUnclassifiedvariant (model-visible, never retried) rather than free text, so a producer cannot skip classification.Wire tags stay lowercase snake_case; a test pins every variant against
ironclaw_events'is_safe_error_kindshape, because a tag failing that validator is silently rewritten toUnclassifiedon write — a lossy write no read-side migration could recover.No new secret handling, no auth/permission change, no new external surface.
Reborn Trust-Boundary Checklist
FailureKindis a plain redacted vocabulary inhost_api; constructible anywhere by design, carries no secret, path, or backend detail.Authorization-prefix redaction it triggers is documented at the test double.crates/ironclaw_architecture/tests/reborn_retired_failure_vocabulary.rspins the retired names at zero live occurrences and pins that the survivor stays closed.serde(default)fields fail closed —from_tagis total; historical tags map to their nearest surviving variant, and an unrecognized tag lands onInternal(retryable host fault), never a success.fate(); the runner's seven auto-retry category strings are preserved byte-for-byte as a projection.Database Impact
No schema change and no migration.
FailureKind's wire tags are written into durable milestones, observations, and the event log as opaqueerror_kindstrings; the product layer documents them as opaque and the WebUI branches on exactly one (gate_declined, preserved and pinned by test).New writes emit precise tags (
method_missing,undeclared_capability, …) where they previously emitted coarse ones (invalid_input). Historical rows stay readable —from_tagmaps every retired spelling to its nearest surviving variant, covered byfailure_kind_historical_tags_stay_readable.Anything grepping logs or dashboards for the old coarse tags needs updating.
Blast Radius
73 files, 12 crates. Deletes four enums, eleven cross-spelling mapping functions, and the 22→12 coarsening fold. Every deletion is behavior-preserving except the six fixes listed in the body below, which are deliberate and individually red-verified.
Operational note for rollout: failures that were previously invisible become countable. Runs that quietly returned a truncated or refused result now surface as errors. The error rate goes up while the system gets better — warn anyone watching failure-rate dashboards, or it reads as a regression caused by the deploy.
Rollback Plan
Revert the merge commit. No migration to unwind, no schema change, no persisted state that only the new code can read — historical and new tags are both readable by
from_tagin either direction, so a revert leaves rows written by this code fully interpretable by the old code's coarse vocabulary via its own alias handling.Review Follow-Through
Filed rather than folded in:
BudgetExceededa third timeAuthorization's summary prefix trips the loop-safe validator's bannedauthorization:marker, redacting the model-visible summary for the most common recoverable error class (filed on [EPIC] error-recoverability endgame — the model recovers from 100% of the errors it sees #6284, belongs with item 4's remediation work)Secret/Mount→ generalAuthorizationrather thanSecretDenied/FilesystemDenied(same fate, precision only)reborn_retired_taxonomy.rsretains the silent-skip scan pattern this PR fixed in its own gateWhat
Collapses the five overlapping failure-kind enums into one closed 35-variant
ironclaw_host_api::FailureKindwith projection functions, and fixes four wrongful-terminal bugs the collapse exposed — each with a red-verified regression test. Supersedes #6677 (closed); epic #6284.Deleted (each was a full or partial re-declaration of the same domain):
turns::CapabilityFailureKind(19 variants — byte-identical names to host_api's oldFailureKind)host_runtime::RuntimeFailureKind(17 — strict subset)agent_loop::CapabilityErrorClass(7 — coarsened again, crate-private)FailureKindshape (Unknown(String)escape hatch +FailureKindValue)host_runtime/production.rsthat destroyed 17 precise mechanism names (and with them every remediation hint) on the way to the loopReplaced by: one enum + questions asked of it —
fate()(Retry | ModelVisible | Park | Terminal),is_retryable(),as_str()/from_tag()(wire), the 7-string runner retry-category bucketing, and webui's HTTP-status classifier. Every projection is an exhaustive wildcard-free match beside the enum: a new failure kind refuses to compile until every consumer decides what it means. The_ => Abort {{ DriverBug }}arm that silently turned unclassified errors into dead runs is gone.RuntimeDispatchErrorKind(the §11 dispatch-lane contract, 800+ refs in extension crates) stays as the mint-site vocabulary and now injects losslessly 1:1 into the unified kind (+ a new transportNetworkvariant, see fix 2).Recoverability contract, now pinned by test
Exactly one variant may end a run:
Cancelled. Policy denials never retry. Every wire tag passesironclaw_events'is_safe_error_kindshape (the silent-Unclassifiedrewrite hazard) — pinned overFailureKind::ALL. Historical stored tags (invalid_input,process,auth_denied, …) stay readable viafrom_tagaliases; new writes emit precise tags (method_missing,undeclared_capability, …), which the product/frontend layers already treat as opaque (single load-bearing literalgate_declinedpreserved).The four behavior fixes (separate commits, each red-verified)
NetworkDeniednever retries — the old fold mapped egress policy denials onto retryableNetwork, burning retry budget on calls that could never succeed.RuntimeDispatchErrorKind::Network(retryable) and split the mint.subagent_spawn_portroutes decode failures to theDeniedchannel so the model can correct.capability_host_errorno longer maps every non-Cancelledport error to terminalHostUnavailable.Unauthorized,CredentialUnavailable,ScopeMismatch,StaleSurface,InvalidInvocation,Invalid,InvalidOutput,ContentFiltered,PolicyDeniedsurface model-visibly and the run continues; genuine host faults (Unavailable,Internal, budget/checkpoint/transcript) stay terminal-with-diagnostics. Split is an exhaustive match — a new port kind forces a decision. The kind map lives once, asAgentLoopHostErrorKind::failure_kind()inironclaw_turns.Also: the two stale-surface sites that recorded a surface-version race as
PolicyDeniednow mint the honestStaleSurface.Precision upgrades visible on the wire (review surface)
Coarse→precise expectation changes are enumerated per-crate in the slice commit messages; representative: missing first-party handler
invalid_input→undeclared_capability, mount/path denialsauthorization→filesystem_denied, offline egressnetwork_denied→network, MCP client faultsbackend→client. Same fates and retry buckets throughout except where a fix above deliberately changed them. Dispatch mints previously coarsened to retryableBackend(Client,Executor,Manifest,UnsupportedRunner,RuntimeMismatch) are now model-visible non-retryable perfate()— no test asserted the old retry behavior.Validation
--no-fail-fastsuites green at every slice: host_api, turns (634), host_runtime (26 targets), loop_host, capabilities, agent_loop (425 lib), runner, extension_host, product, composition, webuicargo test -p ironclaw_architecturegreen;scripts/pre-commit-safety.shgreenreborn_integration_tool_call29/29,auth_failure17/17,group_approvals16/16,channel_connection_projection14/14-D warnings, both feature lanes, zero warningsSKIP_FRONTEND_BUILD=1; CI builds the frontend normallyFollow-ups (not this PR)
UnknownCapabilityinvocation-error path in host_runtime maps toMissingRuntimerather than the preciseUnknownCapability(local classification, noted in slice commit)Authorizationrather thanSecretDenied/FilesystemDeniedErrmint sites could return failed resolutions instead of port errors (audit contract); fix 4 makes them recoverable at the receiving end regardless🤖 Generated with Claude Code