Repository navigation
Surface new post-edit check diagnostics on reborn coding edits - #5979
Merged
Merged
Conversation
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
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. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
risk: low
![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 |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | 8dde2f00a55e |
Head: 8dde2f00a55e0768a8b47ecfc94fde9aa0d7c9c0
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found one blocking security/policy issue in the new post-edit check path.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [HIGH] Post-edit checks bypass process policy for edit tools
Location: crates/ironclaw_host_runtime/src/first_party_tools/mod.rs:490
write_file and apply_patch still declare only filesystem effects, so the planner sets requires_process = false. The resolver then supplies the default local host process port for those invocations, even when the runtime policy has ProcessBackendKind::None or TenantSandbox. With IRONCLAW_POST_EDIT_CHECK configured, every successful edit now runs sh -c through that port, bypassing process-effect planning, sandbox selection, and process accounting. Gate the check behind the same process policy as builtin.shell (or disable it when process execution is unavailable) and add a regression test for ProcessBackendKind::None / tenant sandbox.
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. - Use
@ironloopai statusto check queued/running/completed/failed/superseded state while reviewers run.
| }; | ||
| let Some(check) = crate::post_edit_check::run_post_edit_check( | ||
| &self.post_edit_check_seen, | ||
| request.services.process.as_ref(), |
Contributor
There was a problem hiding this comment.
This uses the invocation's process service for edit tools that do not declare a process effect. For write_file/apply_patch, requires_process is false, so the resolver hands back the default local host port even under ProcessBackendKind::None or tenant-sandbox policies. A configured post-edit check can therefore spawn a host process outside the process policy/sandbox. Please gate this through the same process planning path as shell, or disable it when process execution is not allowed.
ilblackdragon
force-pushed
the
post-edit-diagnostics
branch
from
July 12, 2026 21:29
8dde2f0 to
22758c3
Compare
ilblackdragon
force-pushed
the
edit-guardrails
branch
from
July 12, 2026 21:29
ff1a41d to
7612665
Compare
Member
Author
|
Finding addressed ( Post-edit check now sits behind the process policy, decided at resolve time. Regression tests as requested: resolver-tier matrix (Some only under LocalHost/LocalSingleUser; None under |
ilblackdragon
force-pushed
the
edit-guardrails
branch
from
July 12, 2026 22:25
7612665 to
e5c9efe
Compare
ilblackdragon
force-pushed
the
post-edit-diagnostics
branch
from
July 12, 2026 22:25
22758c3 to
2c2e779
Compare
Benchmark failure analysis shows agents breaking adjacent code without
noticing: a fix passes its own test while a sibling test starts failing,
and nothing in the tool loop tells the model. Mirror Claude Code's
baseline-diff diagnostics push with a minimal reborn seam: after a
SUCCESSFUL builtin.write_file / builtin.apply_patch dispatch, run an
operator-configured check command through the invocation's
RuntimeProcessPort and append only the NEW diagnostic lines to the
edit's model-visible output as a post_edit_check field.
Config seam: IRONCLAW_POST_EDIT_CHECK (shell command; feature off when
unset/blank) and IRONCLAW_POST_EDIT_CHECK_TIMEOUT_SECS (default 30).
Composition resolves it once via the module-owned
PostEditCheckConfig::from_env (build_local_runtime ->
HostRuntimeServices::with_post_edit_check) and threads it through
LocalInvocationServicesResolver into InvocationServices, mirroring
unsafe_raw_diagnostics_allowed - per-call handlers never read env.
Execution seam: the coding fallback arm of
BuiltinFirstPartyTools::dispatch, where the already-resolved
services.process port sits next to the dispatch result. The check runs
with the first read+write mount alias as workdir so the process port
resolves it exactly like a shell workdir (/workspace -> the host
workspace root); read-only coding tools never trigger it.
New-only filtering: a bounded per-scope seen-line registry (same scope
key dimensions as the coding read-state registry; 500 lines/scope FIFO,
512 scopes) reports up to 30 lines / 4000 bytes per edit with a
"+N more new lines" note, marking only reported lines as seen so
trimmed lines surface on a later check.
The check is advisory and never fails the edit: no new findings ->
{"exit_code": N} only; timeout -> {"timed_out": true}; a check that
cannot run at all is debug-logged and omitted.
v1 limitation (documented in the module): the seen-set is global per
scope with no per-file clearing, so a diagnostic that disappears and
reappears unchanged is not re-reported until evicted.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The IRONCLAW_POST_EDIT_CHECK command ran through the invocation's default process port from write_file/apply_patch plans that declare only filesystem effects, so under ProcessBackendKind::None or a tenant-sandbox policy the check spawned a host process outside process-backend selection, and the spawn was never accounted. LocalInvocationServicesResolver now populates InvocationServices::post_edit_check only when the plan's process policy resolves local host execution (ProcessBackendKind::LocalHost under DeploymentMode::LocalSingleUser — the same arm that hands the local port to process-requiring plans like builtin.shell, now shared via one predicate). Under any other backend the advisory check is withheld, not rerouted. A check that does run is accounted as process_count=1 on the edit's ResourceUsage, mirroring builtin.shell. Default local-dev behavior is unchanged. Regression tests: resolver-tier coverage of the LocalHost/None/ TenantSandbox/hosted-LocalHost gating, caller-tier write_file dispatch under a no-process policy and a tenant-sandbox policy (asserting zero local-host and zero sandbox-port invocations), and process accounting asserted in the existing post-edit-check findings test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ilblackdragon
force-pushed
the
post-edit-diagnostics
branch
from
July 12, 2026 23:00
2c2e779 to
a3f2000
Compare
ilblackdragon
force-pushed
the
edit-guardrails
branch
from
July 12, 2026 23:00
e5c9efe to
b9576d6
Compare
ilblackdragon
added a commit
that referenced
this pull request
Jul 17, 2026
Merging main (PR #5979, the post-edit-check diagnostics PR) into this branch surfaced two integration gaps that were failing CI: - main's new `first_party_tools/http.rs` save-request test constructed `FirstPartyCapabilityRequest` / `InvocationServices` without this branch's added `run_id` / `post_edit_check` fields, breaking the default-feature build. GitHub's `pull_request` CI builds the PR merged into base, so it hit this before a local merge did. - `builtin_edit_tools_disable_post_edit_check_under_tenant_sandbox_policy` (introduced by #5979) encoded the pre-rework behavior where the check was withheld outright under a tenant-sandbox policy. This branch's rework (82d7148) instead routes the check to the tenant-sandbox process port so it runs ISOLATED in the tenant's own sandbox — the behavior the authoritative resolver test `local_resolver_routes_post_edit_check_to_the_deployment_isolated_process_port` pins. #5979's test couldn't be updated in that commit because it only existed on main. Rewrote it as `builtin_edit_tools_run_post_edit_check_in_tenant_sandbox_not_on_local_host`, asserting the check runs through the sandbox port (surfacing its diagnostics) and never escapes onto the local host port. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Jul 17, 2026
#5978) * Ride out provider outages and drop the 32-call turn cap in the reborn loop Two failure modes discovered via claw-swe-bench-lite run 9ca133e5 (30% vs hermes 65% on the same model) discarded hours of agent work: - A transient provider 5xx storm aborted the whole run after 2 quick retries (max_attempts_per_class=2, backoff capped at 5s). Availability- class model errors (transient/unavailable/internal) now retry on their own deeper budget: max_model_availability_attempts=12 with a 1s..60s exponential backoff, riding out ~7 minutes of sustained provider failure. MAX_MODEL_RETRIES raised 8 -> 16 to let the strategy govern. - DefaultBudgetStrategy's iteration_limit=32 failed closed mid-task with no synthesis (llm_calls in failed bench tasks clustered at exactly 63/64/127/128). The default is now DEFAULT_ITERATION_BACKSTOP=1024 (subagent 16 -> 256), documented as a runaway backstop: operational bounds are the resource budget system and stop-condition strategy. New seam mirroring IRONCLAW_REBORN_PLANNED_DEFAULT_ITERATION_LIMIT: IRONCLAW_REBORN_MODEL_AVAILABILITY_RETRY_ATTEMPTS -> DefaultPlannedRuntimeConfig.planned_model_availability_retry_attempts -> families::default_with_overrides. The integration group harness pins attempts=1 so scenarios that deliberately script provider failures (failure_category_demasked) reach Failed in seconds, not minutes; the availability-retry tests run under paused tokio time. Family fingerprint digests regenerated for the new strategy parameters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Surface tool-failure reasons to the model for shell and coding tools Benchmark traces showed the model retrying identical failing calls blind: builtin.shell parameter errors and coding-tool path rejections reached it as a bare category ("the tool input could not be encoded") because the handlers built FirstPartyCapabilityError/CodingCapabilityError with no safe_summary — the model-visible Diagnostic detail channel downstream was already wired but starved (one agent burned 13 apply_patch calls against an out-of-scope /testbed path with empty errors). - shell.rs: shell_error/process_error now carry the concrete reason ("missing 'command' parameter", timeout duration, spawn failure), bounded to 512 chars. The strict safe-summary validator still falls back to the fixed category string; the reason always survives on the secret-scrubbed diagnostic channel. - coding/paths.rs: scoped-path rejections name the offending path and the available scoped roots; permission rejections say the operation is not permitted on that mount. Covered at the dispatch tier (coding state dispatch, host-runtime invoke_capability) per test-through-the-caller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Run agent_loop scenario test targets under paused tokio time The deep availability-retry backoff added for provider-outage ride-out made outage-scripting scenario tests sleep for real: safety_nets alone took ~423s (the exact cumulative backoff schedule) because scripted or script-exhausted model errors now retry for minutes. Pause the clock on all executor scenario targets — they drive the in-process mock host exclusively, so timers auto-advance and the suites return to seconds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fail fast when no LLM provider is configured instead of riding availability retries The placeholder unconfigured provider's RequestFailed was mapped through the catch-all Unavailable arm, so the new deep availability retry budget rode a permanent configuration fault through ~7 minutes of exponential backoff. Users with no LLM configured waited minutes for an error that retrying can never fix, and the Reborn CLI smoke tests that pin fast nonzero exits timed out (the 4 failures on CI run 29136954176). Map errors carrying the shared UNCONFIGURED_PROVIDER_ID to CredentialUnavailable, which is unclassified in loop recovery and therefore terminal on first sight; the Settings → Inference hint travels on the scrubbed detail channel. The provider id moves to a shared constant in ironclaw_llm so the composition placeholder and the runner mapping cannot drift. Regression tests: unconfigured_provider_error_maps_to_credential_unavailable_ not_availability and unconfigured_provider_detection_requires_the_placeholder_ provider_id in model_gateway.rs; the existing smoke tests (*_exits_nonzero_when_runtime_does_not_produce_reply) pin the fast-fail at the caller tier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Derive override-built default-family replay identity from resolved config families::default_with_overrides swapped budget/recovery strategies but reused the planner's static version digest, so an overridden composition carried the pure-default replay identity — violating the component-identity contract (family.rs: the digest identifies replay-relevant configuration). - Turn the cfg(test) fingerprint const into a runtime default_family_fingerprint(iteration_limit, model_availability_attempts) builder; override-built families hash it with their resolved values at composition time (BLAKE3, same path as the pinned const). The pure-default composition keeps the static DEFAULT_FAMILY_DIGEST, and overrides spelling out the production defaults hash to that same digest. - Collapse the two Option args into a FamilyOverrides struct and drop the now-dead (None, None) branch in the runner's registry factory. - Tests: digest differs per override knob and is deterministic; explicit production defaults reproduce the static digest; an attempts=1 override reaches the composed recovery strategy (one retry then abort). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Let the recovery strategy own the model retry guard; wake backoff on cancel Two model-stage fixes from the PR 5959 review: MAX_MODEL_RETRIES=16 silently capped any configured availability budget of 16+: the retry loop fell through to a generic ModelError exit with FailedExitDetails::default() — no failure category, no diagnostic ref — before the strategy could reach its own Abort. The executor now derives the loop bound from the composed strategy via RecoveryStrategy::max_total_model_attempts() (DefaultRecoveryStrategy computes it from its per-class + availability budgets with margin), so every accepted override reaches the strategy's abort boundary. The contract-bug fall-through now carries the last observed model error's category and diagnostic ref instead of empty details. The availability backoff sleep (up to 60s per attempt) was not cancellation-aware: a cancel request could wait out the full delay. The sleep now selects over the host's cancellation_requested() future (same pattern as the prompt-compaction and failure-explanation waits), and a boundary cancel check right after the alteration turns the wake into a checkpointed Cancelled exit without issuing another model call. Tests (paused tokio time): an availability budget of 20 — past the old executor cap — fails with the strategy's model_unavailable category and diagnostic ref after exactly 21 model calls; cancellation requested during the first 1s backoff exits Cancelled without riding out the sleep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Clarify DEFAULT_ITERATION_BACKSTOP doc: resource budgets are not yet enforced The doc claimed operational bounds come from the resource budget system, but ResourceBudgetPolicy.max_model_calls and the wall-clock cap are defined and not applied; until they are, this backstop and the stop-condition strategy are the only live ceilings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Carry tool-failure reasons to the model past the strict summary validator PR 5959's headline feature (model-visible tool-failure reasons) never reached the model for path-bearing reasons: LoopSafeSummary rejects path/payload delimiters and newlines, and dispatch_failure_message silently degraded every such reason to the generic category sentence before it could reach the diagnostic channel. - production.rs (failure_from): a host-authored safe_summary that fails LoopSafeSummary validation is preserved as the new DispatchFailureDetail::Diagnostic instead of being dropped; the message keeps the fixed category sentence (host-authored, Invariant 2). - capability_port.rs: maps the Diagnostic detail into the model-visible CapabilityFailureDetail::Diagnostic, scrubbing secret values and normalizing control characters the observation validator rejects (so one stray escape byte cannot drop the whole observation); newlines are preserved. The RetrySameCall arm now forwards structured detail too. - coding/paths.rs: scoped-path rejection summaries render the path and available roots delimiter-free ("path testbed replacer.go", "available roots: workspace") so they pass the strict validator — FilesystemDenied surfaces as a Denied loop outcome whose only model-visible channel is the summary itself. - shell.rs: bounded_failure_reason documents the (now real) diagnostic flow; truncation remains char-based (no byte-boundary panics). Regression tests: - coding/mod.rs: out-of-scope rejection summary passes LoopSafeSummary and names path + roots; new read-only-mount write denial test pins FilesystemDenied with an actionable validated reason. - host_runtime caller tier (first_party_coding_tools.rs): out-of-scope read and read-only write driven through invoke_capability, asserting the loop-boundary message survives validation and names path/root. - host_runtime caller tier (first_party_builtin_tools.rs): shell sensitive-path rejection reason rides the Diagnostic detail while the strict summary channel stays path-free. - production.rs: failure_from preserves rejected summaries (path + newline) on the Diagnostic detail and leaves validator-safe summaries on the message alone. - capability_port.rs: Diagnostic detail reaches the model scrubbed (path kept, newline kept, control char normalized, secret redacted); empty-after-normalization diagnostics are dropped. - shell.rs: bounded_failure_reason boundary tests (exactly 512 kept, 513 truncated with ellipsis, multibyte truncation on char boundaries) and producer pin that paths/newlines are not pre-sanitized away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Pin availability retries to 1 in the parity-QA binary E2E harness The reborn_parity_qa harness scripts deliberately-failing replay gateways (exhausted steps, mismatched requests) whose errors are availability-class; the production 12-attempt backoff budget rode them for minutes and reborn_trace_error_path_parity timed out waiting for Failed. Pin planned_model_availability_retry_attempts=1 in the harness runtime config, mirroring the integration group harness's env pin. The parity suite passes in ~1s again; the pinned test is the regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fail fast on the stub gateway and pin retries in scripted-outage tests Three more real-time test paths rode the new 12-attempt availability backoff for minutes (composition-core / reborn-core CI buckets): - The substrate-only StubGateway returned Unavailable for a missing LLM gateway — a build-configuration fault retrying can never fix. It now returns CredentialUnavailable (unclassified in recovery, terminal), matching the placeholder-provider fail-fast. - send_user_message_preserves_model_unavailable_after_retry_budget scripts a deliberate outage; RebornRuntimeInput gains a test-support with_model_availability_retry_attempts override (wins over the env knob) and the test pins attempts=2, keeping its retry-then-fail pin. - turn_runner_worker_full_reborn_fails_cleanly_when_model_provider_is_offline now builds its registry via build_loop_family_registry_with_overrides with attempts=2 for the same reason. The three tests pass in ~3s each again; they are the regression tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Update stub-gateway failure pins to model_credentials_unavailable The stub gateway now reports a configuration fault instead of an availability blip; the two runtime.rs pins (and one comment) tracking its failure category move from model_unavailable to model_credentials_unavailable accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Detect prompt-cache breaks and stop doomed compaction loops Benchmark analysis showed the provider KV cache hit rate collapsing from ~82% to 29% as reborn agent runs grow past ~200 model calls, roughly 3.5x input cost. Two defenses: ironclaw_runner model gateway: track per-run cache_read continuity across completed provider calls. Every call emits a debug-level per-call cache series (cache_read / cache_creation / input tokens); a drop of BOTH >5% relative and >10K tokens absolute against the previous call in the same run warns "prompt cache break detected" with cheap attribution hints: tool-definition list hash changed, system prompt hash changed, seconds since last call. The pure threshold decision (is_prompt_cache_break) is unit-tested at its edges; per-run state lives in a bounded Mutex<HashMap> keyed by TurnRunId on each gateway. ironclaw_agent_loop compaction circuit breaker: when 3 consecutive compactions complete without the post-compaction prompt estimate dropping below the trigger threshold (compaction fired and didn't help), a one-way breaker in the CompactionStrategyState slot opens and compaction stays disabled for the remainder of the run, with a single tracing::warn!. Claude Code measured ~250K wasted API calls/day from exactly this compact-recompact doom loop before adding a breaker. State follows the typed-slot pattern (strategy reads, executor swaps); CompactionDecision::Trigger now carries the trigger threshold so the executor can score effectiveness. All compaction defaults (128000/20000/8000/...) are unchanged; the family fingerprints gain ineffective_trip_limit=3 and their BLAKE3 digests are regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Extract prompt-cache telemetry into a private model_gateway submodule Review findings N6/N7 on #5975: the ~350 lines of prompt-cache continuity tracking (thresholds, break detection, observation types, per-run scope eviction, logging) lived inline in the 3,300-line model_gateway.rs and PromptCacheActivityLog was needlessly pub. Pure move into model_gateway/prompt_cache_activity.rs (unit tests included); the gateway now only constructs PromptCacheCallScope and records completed calls. All moved types are pub(super) at most, so PromptCacheActivityLog is no longer part of the crate's public API. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Record prompt-cache breaks at debug level and cover tool-capable recording Review findings N1-N4 on #5975: - N4: the cache-break record logged at warn!, but repo logging rules reserve info!/warn! for user-facing REPL output — internal diagnostics must use debug!. Downgraded, and renamed the gateway test to gateway_records_prompt_cache_break_within_a_run with a logs_assert that pins the record to a non-warn level. - N1: added a gateway integration test driving stream_model_with_capabilities with two same-run tool-capable calls, a 200K->50K cached-read collapse, and a changed advertised tool surface — pinning ModelCallCacheUsage::from_tool_response recording and tool-surface break attribution on the tool-capable path. - N2: extended gateway_repairs_oversized_provider_tool_arguments_ before_registration with scripted cache usage so the repair-retry recording seam is covered: both calls must record, and the collapse across the retry is a break with an unchanged request shape. - N3: extended the classification unit test with a changed-system- prompt break so system_prompt_changed=true attribution is pinned. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix compaction circuit breaker: forced bypass, summary-aware effectiveness, trigger-kind baselines Fixes the three blocking review findings on #5975 (B1/B2/B3): - B1: an open breaker suppressed forced/recovery compactions. force_compact_on_next_iteration is how ContextOverflow recovery (RetryAlteration::ShrinkContext) and byte-cap overflow request a shrink; with the breaker open they silently no-oped, the same oversized prompt was rebuilt, and the retry budget burned to abort. DefaultCompactionStrategy::can_evaluate now checks the force flag before the breaker — the breaker only gates automatic threshold-triggered compaction. - B2: effectiveness was measured right after retain_after_sequence, before the injected summary landed in the prompt, so a huge summary that kept the prompt oversized still counted every compaction as effective and the doom loop persisted to the iteration backstop. PromptCompactionStep now stashes the baseline on the new compaction_state.pending_effectiveness_baseline slot; the executor consumes it via observe_pending_compaction_effectiveness once the prompt bundle is next rebuilt (immediately after the in-pipeline rebuild, or on the next iteration's candidate build for compaction-only SkipModel turns), so the comparison sees the summary's tokens. - B3: forced compactions were judged against the unrelated visible-transcript threshold, letting a completely ineffective forced compaction reset the counter. CompactionDecision::Trigger now carries a trigger-kind-specific CompactionEffectivenessBaseline: TriggerThresholdTokens for automatic triggers, PreCompactionPromptTokens (did the prompt actually shrink?) for forced ones. Also downgrades the breaker-opened log from warn! to debug! (N5): internal loop diagnostics must not render into the REPL/TUI. Tests (red-first where the old behavior was pinned): - prompt_stage_circuit_breaker_disables_compaction_after_repeated_ ineffective_runs now drives three REAL ineffective compactions through the prompt stage (no seeded counter) with a retained tail below the threshold so only the summary-aware measurement trips the breaker, and asserts the fourth threshold overflow is suppressed. - prompt_stage_forced_compaction_bypasses_open_circuit_breaker pins B1+B3 at the executor tier: forced compaction runs with the breaker open and an effective shrink (260 -> 140) resets the counter even though 140 is still over the 90-token threshold. - Strategy tests updated: forced-bypass triggers for both DefaultCompactionStrategy and ActiveTaskPreservingCompactionStrategy, threshold-gated skip kept, slots-level forced-baseline coverage, and checkpoint back-compat for the new pending slot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Advertise Reborn skills as a one-line listing; load bodies on activation Reborn injected the FULL SKILL.md body of every keyword-scored skill into the prompt on every run — ~7K tokens of mostly irrelevant instructions per turn (measured in claw-swe-bench traces) — and the skill section churned whenever scores shifted, defeating prompt-cache stability. Flip to the Claude Code model: advertise skills as one `- name: description` line each and load a body only when the model invokes it via builtin.skill_activate. - `SelectableSkillContextSource` gains `SkillInjectionMode`: * `Listing` (Reborn default, `IRONCLAW_REBORN_SKILL_INJECTION` overridable to `full`): non-activated skills collapse into ONE discoverable "available-skills" candidate — a fixed header pointing at builtin.skill_activate plus one line per skill (250-char description cap, 100-entry cap). Full bodies inject only for explicit $name mentions and model-selected activations. Keyword scoring still runs and RANKS the listing; criteria selections no longer consume the model's activation budget. * `Full` stays the library default so non-Reborn consumers and the skill-execution capture plan keep their existing semantics. - Prompt build and by-ref model-message resolution rebuild the snippet set independently, so both now derive body-eligibility and listing ranking from the merged active plan — identical sets, stable msg:snippet refs. - `SkillContextService` safe summaries are now the bounded first line of the description instead of the full text: the multi-skill listing (5.6KB) blew past MODEL_SAFE_SUMMARY_MAX_BYTES and killed runs at the prompt stage — a latent bug for any long skill description. v1 (`src/`) skill behavior is untouched. Tests: reborn_integration_skill_activate pins the one-liner listing (body absent pre-activation, present post-activation) at the captured model-request seam; new selector listing-mode tests in ironclaw_first_party_extension_ports; safe-summary regression in skill_context_service_contract; env parse + config propagation tests in ironclaw_reborn_composition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Upgrade criteria-listed skills to ModelSelected on later skill_activate merge_active_plan deduped merged activations by bundle id alone, so a skill already in the run plan as a listing-only ActivationCriteria entry silently dropped a later ModelSelected activation for the same bundle: the mode never upgraded, body_eligible_bundle_ids never included it, and the SKILL.md body never injected — while the skill_activate tool still reported "activated 1 skill(s)" because the merged plan matched by name. Mode-priority merge: when the incoming activation is body-eligible (ModelSelected/ExplicitMention) and the existing entry for the same bundle is ActivationCriteria, upgrade the existing entry's mode in place; never downgrade. Regression test drives record_user_message (criteria match) then activate_skills_for_run and asserts the body injects on the next prompt build and leaves the listing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Hash full model content into skill snippet model-message refs skill_snippet_model_message_ref hashed only [snippet_ref, safe_summary, ordinal], and the safe summary is first-line-only (capped 256 chars). The available-skills listing's first line is a constant header, so adding, removing, or reordering listed skills changed the listing body without changing the ref — a stale ref resolved against the new content instead of failing closed. Include the full model_content in the hashed fields. Refs are per-run ephemeral (built at prompt time, resolved live by instruction_snippet_messages_by_ref in the same run; never persisted — the executor's message index stores only sequence/kind/token counts), so the deliberate one-time hash rotation is safe; pinned ref literals in the turns/loop-support contract tests are updated to the new layout. Regression tests pin that listings differing only after the first line produce different refs and identical content produces a stable ref. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Require read-before-edit and reject stale edits in reborn coding tools Benchmark traces show models editing files from a stale in-context view and blindly overwriting files they never read: builtin.apply_patch and builtin.write_file had no guard against either, unlike Claude Code's Edit tool (read-before-edit + mid-air collision detection). This brings the reborn coding tools to parity. read_file now records a content fingerprint (cheap std hash of the raw bytes it already holds) per scope + resolved virtual path in a bounded registry on CodingCapabilityState, shared across concurrent runs exactly like edit_locks. Only a full read (no offset/limit window) records state. write_file and apply_patch on an EXISTING file now reject with a recovery-worded, model-visible safe summary when: - no read was recorded ("read it with read_file before editing it"), or - the file's current fingerprint no longer matches the recorded one ("the file changed since it was last read; read it again ..."). Creating a new file needs no prior read, a successful edit refreshes the fingerprint so chained edits don't re-trip, and write-only mounts are exempt (the model cannot read there, so blind overwrite is the intended mode). Eviction from the bounded registry fails safe: a missing entry only forces a fresh read_file. Regression tests: dispatch-tier tests in coding/mod.rs pin all five behaviors (unread-write rejection, read-then-patch, out-of-band staleness, new-file write, chained edits); host_runtime caller-tier tests updated to seed read state through the public read path, and the former builtin_apply_patch_accepts_unique_match_without_prior_read pin is inverted to builtin_apply_patch_requires_prior_read_of_existing_file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Only record read-before-edit state for complete, untruncated reads PR 5978 review finding 2 (ironloopai, MEDIUM): read_file recorded the read-before-edit fingerprint whenever the call had no explicit offset/limit, BEFORE the default output truncation was applied. A default read of a file over the 2,000-line or 64 KiB output cap returned partial content with truncated=true, yet still unlocked whole-file write_file/apply_patch - violating the guard's own stated invariant that only a full read proves the model has seen the file. read_file now computes the rendered output first and records the fingerprint only when the read had no explicit range AND was not truncated at the line or byte cap (missing flag fails safe as truncated). The read-before-edit rejection message now also explains that ranged and truncated reads do not count, so the model understands why its earlier read did not unlock the edit and that a file too large to read in full cannot be blind-edited with these tools. Regression tests (caller-tier, driven through the composed host runtime in first_party_coding_tools.rs): - default read of a >2,000-line file (line truncation) does not unlock write_file, and an explicit offset/limit read wide enough to cover the file still does not unlock it - default read of a >64 KiB-rendered file (byte truncation) does not unlock apply_patch, even when the patch anchor was inside the visible window - rejected edits leave the file untouched and carry the model-visible full-read guidance Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Scope coding read-before-edit state to the loop turn-run PR 5978 review finding 1 (ironloopai, HIGH): CodingReadScopeKey keyed read-before-edit state by tenant/user/agent/project/mission/thread only, and CodingCapabilityState lives in the long-lived BuiltinFirstPartyTools handler. A read_file recorded in one run could therefore authorize write_file/apply_patch in a LATER run that never read the file, as long as the content fingerprint still matched. Security framing: the fingerprint check already guarantees content equality at edit time, so this is a policy tightening - the model must have SEEN the file within the current run - not a data-corruption fix. Why a new wire field: the reviewer suggested keying by request.scope.invocation_id, but invocation_id is minted per tool call (InvocationId::from_uuid(activity_id) in invocation_context_from_visible), so keying on it would break the feature entirely - the read and the subsequent edit are always different tool calls. Nothing between thread_id (spans many runs) and invocation_id (one tool call) existed on the wire, so this adds the prompt-visible turn-run identity that the loop host already owns (LoopRunContext::run_id) as an optional, host-stamped RunId, mirroring the existing authenticated_actor_user_id plumbing hop for hop: ExecutionContext.run_id (stamped in invocation_context_from_visible) -> CapabilityDispatchRequest.run_id (capability host) -> RuntimeAdapterRequest.run_id (dispatcher) -> FirstPartyCapabilityRequest.run_id (first-party adapter) -> CodingCapabilityRequest.run_id -> CodingReadScopeKey.run_id run_id is serde(default) so persisted legacy contexts still deserialize; non-loop callers (system services, spawned processes, one-shot product invocations) carry None, which is its own key bucket, never a wildcard. Reads recorded in run A neither unlock run B nor the None bucket, and vice versa; a read in tool-call N still unlocks an edit in tool-call N+2 of the same run. Regression tests: - caller-tier (first_party_coding_tools.rs, through the composed host runtime): read in run A, write_file/apply_patch with a same-identity scope carrying a different run_id are rejected with the read-before-edit error and leave the file untouched; a later tool call (fresh invocation_id) of the SAME run still edits, pinning run-not-invocation granularity - loop_support: invocation_context_from_visible stamps run_id from LoopRunContext::run_id, so a dropped stamp cannot silently collapse every run into the shared None bucket - host_api: legacy ExecutionContext payloads without run_id (and authenticated_actor_user_id) still deserialize Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Surface new post-edit check diagnostics on reborn coding edits (#5979) * Surface new post-edit check diagnostics on reborn coding edits Benchmark failure analysis shows agents breaking adjacent code without noticing: a fix passes its own test while a sibling test starts failing, and nothing in the tool loop tells the model. Mirror Claude Code's baseline-diff diagnostics push with a minimal reborn seam: after a SUCCESSFUL builtin.write_file / builtin.apply_patch dispatch, run an operator-configured check command through the invocation's RuntimeProcessPort and append only the NEW diagnostic lines to the edit's model-visible output as a post_edit_check field. Config seam: IRONCLAW_POST_EDIT_CHECK (shell command; feature off when unset/blank) and IRONCLAW_POST_EDIT_CHECK_TIMEOUT_SECS (default 30). Composition resolves it once via the module-owned PostEditCheckConfig::from_env (build_local_runtime -> HostRuntimeServices::with_post_edit_check) and threads it through LocalInvocationServicesResolver into InvocationServices, mirroring unsafe_raw_diagnostics_allowed - per-call handlers never read env. Execution seam: the coding fallback arm of BuiltinFirstPartyTools::dispatch, where the already-resolved services.process port sits next to the dispatch result. The check runs with the first read+write mount alias as workdir so the process port resolves it exactly like a shell workdir (/workspace -> the host workspace root); read-only coding tools never trigger it. New-only filtering: a bounded per-scope seen-line registry (same scope key dimensions as the coding read-state registry; 500 lines/scope FIFO, 512 scopes) reports up to 30 lines / 4000 bytes per edit with a "+N more new lines" note, marking only reported lines as seen so trimmed lines surface on a later check. The check is advisory and never fails the edit: no new findings -> {"exit_code": N} only; timeout -> {"timed_out": true}; a check that cannot run at all is debug-logged and omitted. v1 limitation (documented in the module): the seen-set is global per scope with no per-file clearing, so a diagnostic that disappears and reappears unchanged is not re-reported until evicted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Gate the post-edit check behind the process policy and account its spawn The IRONCLAW_POST_EDIT_CHECK command ran through the invocation's default process port from write_file/apply_patch plans that declare only filesystem effects, so under ProcessBackendKind::None or a tenant-sandbox policy the check spawned a host process outside process-backend selection, and the spawn was never accounted. LocalInvocationServicesResolver now populates InvocationServices::post_edit_check only when the plan's process policy resolves local host execution (ProcessBackendKind::LocalHost under DeploymentMode::LocalSingleUser — the same arm that hands the local port to process-requiring plans like builtin.shell, now shared via one predicate). Under any other backend the advisory check is withheld, not rerouted. A check that does run is accounted as process_count=1 on the edit's ResourceUsage, mirroring builtin.shell. Default local-dev behavior is unchanged. Regression tests: resolver-tier coverage of the LocalHost/None/ TenantSandbox/hosted-LocalHost gating, caller-tier write_file dispatch under a no-process policy and a tenant-sandbox policy (asserting zero local-host and zero sandbox-port invocations), and process accounting asserted in the existing post-edit-check findings test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * fix(reborn): PR #5978 review + isolate post-edit check per tenant Read-before-edit guardrail fixes: - Split CodingEditLockKey (run-agnostic) from CodingReadScopeKey (run-scoped): concurrent runs editing the same path now contend on one edit-lock stripe. Folding run_id into the lock key sent them to different stripes, so both could pass their fingerprint check and overwrite each other (lost update). (ironloop) - apply_patch reuses verify_read_before_edit, sharing the recorded-read / size-limit / re-read / fingerprint checks with write_file. (coderabbit) - The post-edit check runs in the writable mount backing the edited file, not an arbitrary first writable mount. (coderabbit) Post-edit check multi-tenant isolation (ironloop/coderabbit #4): - The check no longer runs on the deployment-blind local process port. The invocation-services resolver now routes it to the port matching the plan's process backend -- the tenant sandbox under HostedMultiTenant -- so a tenant's command runs isolated in that tenant's own sandbox, never on the shared provider host, and is disabled when no backend can run it in isolation. - Wired the operator check into production composition (previously local-dev only). Regression tests: cross-run edit-lock serialization, run-agnostic lock key vs run-scoped read key, edited-mount selection + alias-boundary match, and resolver isolation routing (local host vs tenant sandbox vs disabled). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): reconcile post-edit-check merge with main (#5978) Merging main (PR #5979, the post-edit-check diagnostics PR) into this branch surfaced two integration gaps that were failing CI: - main's new `first_party_tools/http.rs` save-request test constructed `FirstPartyCapabilityRequest` / `InvocationServices` without this branch's added `run_id` / `post_edit_check` fields, breaking the default-feature build. GitHub's `pull_request` CI builds the PR merged into base, so it hit this before a local merge did. - `builtin_edit_tools_disable_post_edit_check_under_tenant_sandbox_policy` (introduced by #5979) encoded the pre-rework behavior where the check was withheld outright under a tenant-sandbox policy. This branch's rework (82d7148) instead routes the check to the tenant-sandbox process port so it runs ISOLATED in the tenant's own sandbox — the behavior the authoritative resolver test `local_resolver_routes_post_edit_check_to_the_deployment_isolated_process_port` pins. #5979's test couldn't be updated in that commit because it only existed on main. Rewrote it as `builtin_edit_tools_run_post_edit_check_in_tenant_sandbox_not_on_local_host`, asserting the check runs through the sandbox port (surfacing its diagnostics) and never escapes onto the local host port. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack 4/4 on the edit-guardrails PR.
Benchmark failure analysis shows the dominant remaining agent failure is collateral damage: a fix passes its own reproduction but breaks adjacent code the agent never re-checked. Claude Code pushes NEW diagnostics to the model after each edit (baseline→diff, deduped, capped). This is the reborn v1.
What
IRONCLAW_POST_EDIT_CHECK(operator command, e.g.cargo check --message-format=short 2>&1) +IRONCLAW_POST_EDIT_CHECK_TIMEOUT_SECS(default 30). Env read once in composition (apply_post_edit_check_from_env, following the optional-env pattern), threaded throughHostRuntimeServices→InvocationServices— no per-call env reads.BuiltinFirstPartyTools::dispatch, the one place where the successful edit result, theRuntimeProcessPort(same portbuiltin.shelluses, same/workspaceworkdir-alias semantics), scope, and mounts coexist. Only fires forwrite_file/apply_patch.post_edit_checkfield on the edit's tool output:{"exit_code": N, "new_output": ...}on findings;{"exit_code": 0}when clean (token-lean);{"timed_out": true}on timeout. The check is advisory — it never fails the edit.Testing
Red-then-green caller-tier tests drive
invoke_capabilitythrough the full runtime with a scripted process port: output carries new diagnostics; a second identical check output produces no repeated lines; unconfigured → zero port invocations and no field; timeout-advisory and clean-pass cases. 10 unit tests on filter/config (dedup, caps, FIFO eviction, invalid-timeout). All three crates green; fmt + clippy clean; pre-commit safety script exit 0.Deferred (documented): per-file seen-set clearing, hosted/multi-tenant composition wiring (local-dev only by design), resource accounting for the check run.
🤖 Generated with Claude Code