test(reborn): wave-4 integration coverage — auth-gate wire regression pack, triggered auth delivery, attachments, golden/synthetic expansions - #5610
Merged
Conversation
Adds submit_turn_with_attachments (generalizes the image-only submit_turn_with_image_attachment to N attachments of any mime type) and two int-tier tests: a text/plain attachment's extracted text reaching the model, and two attachments in one turn both reaching the model with distinct index ordinals. Closes the doc/multi-attachment gap in C-ATTACH (only single-image coverage existed before).
…o-replay (wave-4 row 1) Pins the #5174/#5180 bug class (empty credential_requirements leaving AuthPromptView.provider null, "Could not save the token" with no network request) through the FULL scripted-gateway integration harness — a tier below the existing crate-level pins, which drive CapabilityHost::invoke_json or HostRuntimeServices::invoke_capability directly and never exercise the real submit_turn -> BlockedAuth wire the WebUI depends on. - tests/reborn_integration_auth_gate.rs: new runtime_401_after_injection_populates_provider_credential_requirement (github credential resolves OK but the runtime HTTP call 401s; asserts the resulting BlockedAuth gate's credential_requirements carries provider=github + ManualToken setup), cancel_blocked_auth_gate_leaves_no_stale_replay (cancelling a BlockedAuth run lands directly on Cancelled with no active worker, and the SAME real gate ref can no longer resume it afterward — closes the #5067/#4957 class of gates staying "live"), and deny_auth_gate_rejects_a_non_auth_gate_ref_prefix (negative companion). Flip-check: temporarily bypassed the host.rs enrichment call site, confirmed the flagship test fails with the exact pre-fix empty-list shape, restored (crates/ironclaw_capabilities/src/host.rs left byte-identical — no production diff). - tests/support/reborn/harness.rs: RecordingNetworkHttpEgress gains an additive FIFO status_queue (default empty -> unchanged hardcoded-200 behavior) + install_network_status_script accessor. Needed because GithubIssueTools' real WASM HTTP call flows through the network-egress lane, not the runtime-egress lane the existing ScriptedHttpResponse matcher scripts (try_with_host_http_egress overwrites the runtime port — see reborn_integration_secret_injection.rs's module doc) — the prior double had no way to script a non-200 status on that lane at all. - tests/support/reborn/builder.rs: with_github_network_status(status) builder method (FIFO) threading github_network_statuses through RebornCapabilityBackend::install. - tests/support/reborn/capability_backend.rs: wires keyed_http_responses (previously dropped for this backend) and the new github_network_statuses into the GithubIssueTools install arm; no-op for existing empty-vec callers. - tests/support/reborn/assertions.rs: assert_network_egress_count, sibling of assert_egress_count for the network-lane call-count proofs above. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-EXT-MANIFEST-ERR)
Narrowed from the originally-scoped manifest-content arms (schema
mismatch/reserved id/forbidden trust level): extension_install's only
input is a catalog-resolved extension_id over a fixed,
compile-time-embedded bundled catalog, so raw manifest TOML never
reaches ManifestV2Error validation through this capability in
production. The one reachable, wired arm is an unknown extension_id,
which fails Failed{invalid_input} rather than panicking or no-oping.
…verage #5001 (PinchBench bucket D) removed the crude SENSITIVE_PROVIDER_TEXT_MARKERS substring scan on provider reasoning/response_reasoning/signature text (bare words like "password"/"traceback" were false-positive-rejected, driving retry/give-up loops); the entropy-based LeakDetector is the real guard now. That contract was pinned only at the private free-function level (capability_port/provider_validation.rs's own unit test calling validate_provider_tool_call directly) — the #5001 caller gap. Adds provider_tool_call_registration_accepts_password_and_traceback_reasoning_text in crates/ironclaw_loop_support/src/capability_port.rs's existing test module, alongside the crate's other caller-level `port.validate_provider_tool_call(&call)` tests: drives the REAL production caller (LoopCapabilityPort::validate_provider_tool_call / register_provider_tool_call / invoke_capability on HostRuntimeLoopCapabilityPort, the same port the agent loop calls) with "password"/"traceback" in all three metadata fields, and proves genuine acceptance through to a real Completed dispatch (not just a non-error return). Flip-check: temporarily bloated response_reasoning past PROVIDER_METADATA_TEXT_MAX_BYTES to confirm the assertion mechanism discriminates a genuine rejection (fails with the expected "exceeds 16384 bytes" error), then restored the password/traceback content. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gh build_reborn_services #5439 fixed NEAR AI MCP token resolution for SSO users: a Google-SSO user in the same tenant/agent as the boot owner, with no NEAR AI token of their own, now falls back to the host-managed (boot-owner) NEAR AI credential instead of being prompted for one. That contract was pinned only at the private rule/selector level (product_auth_runtime_credentials/tests.rs never calls build_reborn_services) — the composition-wiring gap this row targets. Adds local_dev_nearai_runtime_selection_falls_back_to_host_managed_account_for_sso_user to extension_lifecycle_capabilities_auth_tests.rs (extending the existing in-crate #[cfg(test)] composition-test file — same pattern as the sibling github manual-token test above, template: product_auth_refresh_composition.rs's "drive build_reborn_services directly" style). Drives ONLY the public surface: build_reborn_services (local-dev always derives nearai_mcp_host_managed_scope from the boot owner, so no live NEAR AI config injection is needed) plus the crate-internal runtime_credential_account_selection_service() accessor this file already had precedent for calling. Two discriminating arms on one composed `services`: an SSO user in the owner's tenant/agent (different project -- local-dev's host scope is project-unscoped by design) resolves via fallback; an SSO user under a different tenant does not (CredentialMissing) -- proving the positive arm is a real scope match, not the selector always succeeding. Flip-check: temporarily short-circuited RebornProductAuthServices::runtime_credential_account_selection_service to always return the un-decorated selector (pre-#5439 behavior), confirmed the new test's positive arm fails with CredentialMissing, restored (auth.rs left byte-identical -- no production diff). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t_create fault-injection (wave-4 lane C) Two carry-over arms deferred from wave-3 PR #5584: - skill_activate AmbiguousSkill: seed a system-scoped AND a user-scoped skill sharing one name (both SkillTrust::Trusted per FilesystemSkillBundleRoot::system/user) so the real validate_explicit_mentions_are_unambiguous reject path fires end-to-end, not just at the skill_activation.rs unit-test level. New seed_user_skill_for_test harness helper (additive, mirrors seed_system_skill_for_test). - project_create fault-injection: new FaultInjectingProjectService test double (project_service_fault.rs) wrapping the real ProjectService at the production-wired Arc<dyn ProjectService> seam, forcing ProjectServiceError::Denied for a sentinel project name and delegating everything else to the real store. New project_tools_with_fault_injection()/project_lifecycle_fault_injected() harness+group constructors (additive). Deliberately NOT ProjectServiceError::Unavailable/Internal: investigation found both route through DefaultRecoveryStrategy's capability-retry branch, whose retry re-dispatch hits a real, confirmed production bug for provider-tool-call-originated invocations under local-dev composition — LocalDevCapabilityIo::resolve_capability_input rejects the reused input_ref on the retry with InvalidInvocation/"capability input ref was not staged for this loop run", collapsing the documented "retry twice, then a model-visible Failed" contract into an immediate terminal driver_unavailable. Documented in project_service_fault.rs; reported separately (not fixed — production change, out of this lane's scope). Both flip-checked (mutated seed/fault-injection to prove discriminating failure) and reverted before commit.
…attachment, gated-turn resume (wave-4 lane C) Three scenario expansions to tests/reborn_integration_golden_payload.rs (carry-over from wave-3 PR #5584): - golden_parallel_tool_calls: new RebornScriptedReply::tool_calls([..]) constructor (additive to reply.rs) scripts ONE assistant response with TWO tool_calls[] entries, pinning that multiple calls in one turn each get a distinct id and each following tool-role message's tool_call_id lines up in order — a shape the existing single-call golden_tool_call_feedback can't exercise. - golden_image_attachment_turn: an inline image landed through the real submit_inbound_with_attachments entry point (RebornIntegrationGroup::attachment_tools()), routed through a vision-pattern model id, pinning the multimodal ContentPart::ImageUrl data: URL alongside the text part byte-for-byte. - golden_gated_turn_approve: a real BlockedApproval gate raised, approved, and resumed (RebornIntegrationGroup::live_approvals()), snapshotting BOTH inference calls around the gate — proving the resume doesn't drop, duplicate, or reorder accumulated turn history. Two normalization fixes to golden.rs, both needed for these scenarios to be reproducible (discovered while authoring, not pre-existing regressions): - Attachment-landing scenarios embed today's real UTC date in the landed project path (chrono::Utc::now(), no test seam) — added a second <DATE> filter alongside the existing loop-start-clock <TIMESTAMP> filter, or the image golden would bit-rot on every day boundary. - Tool-call ids come from a NEXT_TOOL_CALL_ID counter shared by every test in this one compiled binary; running more than one tool-call-scripting golden test concurrently (the default `cargo test` thread pool) makes the raw id values order-dependent. Added normalize_tool_call_ids: renumbers every call-<N> to a canonical call-1, call-2, … in order of first appearance per rendered payload, preserving the id/tool_call_id linkage the golden actually cares about without depending on the racy raw value. Confirmed behavior-preserving for the four pre-existing snapshots (no diff) and confirmed the race is fixed (5 consecutive full-suite green runs). Flip-checked (forced two parallel tool_calls to share one id; golden correctly failed) and reverted before commit.
…ly once #5306 fixed an unresumable BlockedApproval loop: require_approval_for_profile_policy checked the explicit ask_each_time override (and the hard-floor force-approval class) BEFORE consulting the matching one-shot approval lease a resume carries, so an approved AskEachTime-gated resume re-hit the ask_each_time branch and re-gated instead of completing. Only a Python E2E test (test_tool_approval.py) exercised this class before; no Rust harness coverage existed. Adds scenario_ask_each_time_resumes_once.rs to the reborn_group_approvals binary (both approvals_group_e2e and its libsql variant), run LAST because it installs a persistent, group-wide ToolPermissionOverride::AskEachTime override on builtin.write_file that would force-gate every sibling scenario's plain-Ask-mode writes. Submits under the override, approves the resulting BlockedApproval gate, and proves the resume reaches Completed in ONE round trip with the write actually persisted — plus a companion "resumes exactly once" proof that re-approving the same now-resolved gate_ref fails NotPending (not a fresh re-raised gate). tests/support/reborn/harness.rs: adds a generic tool_permission_overrides: Option<Arc<dyn ToolPermissionOverrideStore>> field (mirrors the existing auto_approve_settings field's pattern — populated only by new_with_options, None elsewhere) and set_ask_each_time_override_for_test, generalizing disable_outbound_target_set_tool's override-store access beyond outbound_target_tools() to any host-runtime-backed harness/group. Flip-check: temporarily restored the pre-#5306 check order in profile_approval_authorization.rs (ask_each_time/hard-floor before the one-shot lease), confirmed the new scenario fails (the approved write never persists), restored (file left byte-identical — no production diff). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Carry-over from wave-3 PR #5584: a triggered fire whose run raises a BlockedApproval gate, gets resolved, then CHAINS into a SECOND BlockedApproval gate in the SAME run (the post-resume model call issues another gated tool call instead of finalizing), driven through submit_triggered_turn_scripted (E-TRIGGERED-SUBMIT). New scenario_triggered_chained_gate::run_chained_approve, registered as its own live_approvals group in reborn_group_triggers::triggered_gate_group. Re-reads TurnOriginKind::ScheduledTrigger fresh at the coordinator boundary at THREE checkpoints (first park, second/chained park, final Completed) — not just trusting the initial TriggeredSubmission — closing the gap that a resume path rebuilding product_context from a non-trigger-aware default on the SECOND hop would otherwise slip through undetected. Also asserts both gate_refs are genuinely distinct, both chained writes persisted, and the final reply persisted in the trigger's own thread. Flip-checked (asserted the wrong origin kind; the checkpoint helper correctly failed with the real ScheduledTrigger value in the diagnostic) and reverted before commit. Also folds in `cargo fmt` whitespace-only fixes surfaced while formatting this new file (golden.rs, harness.rs, reply.rs, and two golden/skill-activate test files touched by prior lane-C commits) — no semantic change, reran their test bins green after formatting.
… (wave-4 lane C) Committed follow-up on PR #5584's review thread: submit_triggered_turn_scripted hand-mirrored ConversationContentRefMaterializer::materialize_prompt (trigger_resolve_request + record_trigger_prompt + the content-ref shape, field-by-field) instead of reusing it, and — as flagged — deliberately SKIPPED authorize_trigger_fire and validate_trusted_trigger_prompt. Flagged as a drift trap (trusted-trigger materialization is an ownership boundary, AGENTS.md:61); the review agreed the fix is a #[cfg(feature = "test-support")] materializer helper returning (TriggerMaterializedPrompt, TurnScope) living beside the real materializer, held out of #5584 as a fast-follow with this exact shape. New production-crate (test-support-gated, compiles out of default builds) surface in ironclaw_reborn_composition: - trigger_poller_trusted_submit.rs: materialize_trigger_prompt_for_test, #[cfg(any(test, feature = "test-support"))] — runs the REAL production pipeline via ConversationContentRefMaterializer::materialize_prompt (authorize + validate + resolve + record + content-ref), then an idempotent second resolve_or_create_binding_with_trusted_scope call (safe — same request, same already-created binding) to also return the TurnScope the trait method computes internally but never exposes. Plus two crate-tier unit tests: positive (returned scope/content-ref match an independent ground-truth resolve) and negative (an unsafe prompt is rejected by the REAL safety validator). - test_support/trigger_materializer.rs: pub, feature="test-support"-gated thin wrapper re-exported from test_support/mod.rs — the established wrap_project_create_capability_for_test-style pattern. tests/support/reborn/triggered_submit.rs: submit_triggered_turn_scripted now calls this ONE production-owned helper instead of hand-mirroring; deletes ~90 net lines of duplicated resolve/thread-record/content-ref logic. Verified default-features build of ironclaw_reborn_composition stays warning-free (function/import correctly compile out). Flip-checked at the INTEGRATION level (not just the new crate-unit tests): forced an injection-pattern prompt through submit_triggered_turn_scripted — every triggered-gate scenario correctly failed with "rejected by safety scan", proving the old hand-mirrored path's skip of validate_trusted_trigger_prompt is now closed. Reverted before commit. All touched integration test bins (reborn_group_triggers, reborn_integration_triggered_submit, plus every other wave-4 lane-C bin) rerun green after the extraction.
…unDeliveryDriver TriggeredRunDeliveryDriver was exercised by exactly one crate-tier test (triggered_approval_prompt_route_resolves_dm_approve_on_foreign_scope), covering only the approval-gate path. Add the auth-gate twin: a BlockedAuth triggered run whose auth-prompt preference resolves to the creator's DM must carry the OAuth setup link (triggered_auth_prompt_route_delivers_dm_setup_link_on_foreign_scope), mirroring slack_dm_delivers_auth_prompt_with_setup_link_after_immediate_ack's assertion shape but driven through the real triggered-delivery driver. TriggeredRunDeliveryDriver only ever targets the creator's personal DM (never a channel), so there is no literal "channel" arm to mirror slack_channel_auth_prompt_omits_setup_link_after_immediate_ack. The discriminating negative arm instead exercises the driver's own send-time OAuth-DM backstop (triggered_auth_prompt_oauth_target_not_dm_suppresses_setup_link_and_cancels_run): when the resolved auth-prompt target is not a personal DM, the setup link must never be posted and the blocked run must be cancelled instead. ScriptedTriggerCoordinator gains an additive new_with_first_poll constructor (script an arbitrary first-poll status/gate_ref instead of the hardcoded BlockedApproval/GATE pair) and a functional cancel_run (previously unreachable!, since the approval-only scenario never called it) to support the OAuth-not-DM arm. Test code only; no production changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… (wave-4 lane C) Committed follow-up on PR #5584's review thread: submit_triggered_turn_scripted hand-mirrored ConversationContentRefMaterializer::materialize_prompt (trigger_resolve_request + record_trigger_prompt + the content-ref shape, field-by-field) instead of reusing it, and — as flagged — deliberately SKIPPED authorize_trigger_fire and validate_trusted_trigger_prompt. Flagged as a drift trap (trusted-trigger materialization is an ownership boundary, AGENTS.md:61); the review agreed the fix is a #[cfg(feature = "test-support")] materializer helper returning (TriggerMaterializedPrompt, TurnScope) living beside the real materializer, held out of #5584 as a fast-follow with this exact shape. New production-crate (test-support-gated, compiles out of default builds) surface in ironclaw_reborn_composition: - trigger_poller_trusted_submit.rs: materialize_trigger_prompt_for_test, #[cfg(any(test, feature = "test-support"))] — runs the REAL production pipeline via ConversationContentRefMaterializer::materialize_prompt (authorize + validate + resolve + record + content-ref), then an idempotent second resolve_or_create_binding_with_trusted_scope call (safe — same request, same already-created binding) to also return the TurnScope the trait method computes internally but never exposes. Plus two crate-tier unit tests: positive (returned scope/content-ref match an independent ground-truth resolve) and negative (an unsafe prompt is rejected by the REAL safety validator). - test_support/trigger_materializer.rs: pub, feature="test-support"-gated thin wrapper re-exported from test_support/mod.rs — the established wrap_project_create_capability_for_test-style pattern. tests/support/reborn/triggered_submit.rs: submit_triggered_turn_scripted now calls this ONE production-owned helper instead of hand-mirroring; deletes ~90 net lines of duplicated resolve/thread-record/content-ref logic. Verified default-features build of ironclaw_reborn_composition stays warning-free (function/import correctly compile out). Flip-checked at the INTEGRATION level (not just the new crate-unit tests): forced an injection-pattern prompt through submit_triggered_turn_scripted — every triggered-gate scenario correctly failed with "rejected by safety scan", proving the old hand-mirrored path's skip of validate_trusted_trigger_prompt is now closed. Reverted before commit. All touched integration test bins (reborn_group_triggers, reborn_integration_triggered_submit, plus every other wave-4 lane-C bin) rerun green after the extraction.
…SSO-WIRING, W4-ASK-EACH-ONCE, W4-TRIGSLACK-SETTLE)
…s, triggered chained journey, materializer extraction)
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5610
July 3, 2026 21:04
Destroyed
github-actions
Bot
added
size: M
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR expands Reborn integration coverage for approvals, triggers, auth, attachments, golden payloads, and harness seams. It also adds fault-injection and scripting hooks for project creation, network statuses, permission overrides, and tool-call canonicalization. ChangesReborn harness seams and scenario coverage
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of integration and unit tests across various modules, including capability port validation, credential fallback, Slack e2e delivery, group approvals, extension lifecycle, triggers, attachments, auth gates, and project creation. Key additions include testing for credential requirements on runtime 401 errors, document and multi-attachment handling, and chained-gate trigger origin persistence. The feedback recommends replacing generic is_err() assertions in the new auth gate tests with specific error message checks to avoid false positives from infrastructure failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
🚅 Deployed to the ironclaw-pr-5610 environment in ironclaw-ci-preview
|
…5608 in fault-injection rationale Factor the three near-identical bounded-poll-for-chat.postMessage helpers in slack_serve/e2e_tests.rs into one predicate-parameterized wait_for_post_messages_matching, and replace "Lane C final report" citations with the filed issue (#5608) in the local-dev retry-path rationale comments.
Contributor
IronLoop Review StatusHead: Current reviewers:
Recent activity:
Commands:
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5610
July 3, 2026 21:32
Destroyed
henrypark133
commented
Jul 4, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add behavior-neutral Wave 4 Reborn integration and regression test coverage across auth gates, triggers, attachments, golden payloads, and test support.
Stats: 5 findings (from 5 raw, 5 after dedup) across 3 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
conventions
- Medium Large Slack test file grows without a decomposition marker (
crates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rs:1183-1190, confidence 75) — anchor: .claude/rules/architecture.md:126
The diff adds several hundred lines to an existing file over 3,000 lines, but the file still has no decomposition marker or tracking reference. The architecture rule says files over 3,000 lines need a tracking issue and PRs adding more than 200 lines need inline justification; the new comments justify the scenarios, but not why this already-oversized file is the right home.
local-patterns
- Low Fault-injection docs name the wrong project_service_outcome arm (
tests/support/reborn/harness.rs:2227-2230, confidence 75) — anchor: tests/reborn_integration_project_create.rs:150
The new helper documents that a sentinel ProjectServiceError::Denied proves the Unavailable arm. That contradicts the scenario doc, which correctly says Denied maps to recoverable Failed(PolicyDenied). This makes the coverage trail misleading for the still-blocked Unavailable/Internal retry cases. - Low Golden normalization docs still claim tool-call ids stay exact (
tests/support/reborn/golden.rs:116-119, confidence 75) — anchor: changed line tests/support/reborn/golden.rs:172
The updated docs say tool-call ids are deterministic and stay exact, but the same change adds normalize_tool_call_ids and assert_golden_payload canonicalizes every call-N. Snapshot reviewers will be told raw ids are pinned even though the harness deliberately masks them.
maintainability
- Low Do not script the first poll twice (
crates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rs:1318-1322, confidence 100) — anchor: crates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rs:1318
The TurnRunState template already carries the first-poll status and gate_ref, but new_with_first_poll takes a second copy and overwrites the template on poll 0. The auth tests now have to keep TurnStatus::BlockedAuth and AUTH_GATE synchronized across two argument lists; if one side changes, the fixture says one thing while the coordinator returns another.
approach
- Low Golden snapshots mask racy tool-call ids instead of fixing the source (
tests/support/reborn/golden.rs:142-158, confidence 75) — anchor: tests/support/reborn/reply.rs:16
The PR identifies the root cause as the process-wide NEXT_TOOL_CALL_ID counter in RebornScriptedReply, but fixes golden failures by regex-renumbering rendered snapshots in normalize_tool_call_ids. That only stabilizes this one assertion path; any other captured-request assertion still observes nondeterministic ids, and the exact-payload golden no longer pins the actual ids emitted by the scripted provider. A smaller source-level boundary is to make scripted trace construction assign deterministic per-trace ids before the model sees them.
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5610
July 4, 2026 03:24
Destroyed
# Conflicts: # crates/ironclaw_reborn_composition/src/test_support/trigger_materializer.rs # crates/ironclaw_reborn_composition/src/trigger_poller_trusted_submit.rs
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5610
July 4, 2026 06:26
Destroyed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/support/reborn/builder.rs (1)
524-602: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
submit_turn_with_image_attachmentandsubmit_turn_with_attachmentsduplicate the lander-guard + envelope + submit + wait logic.The new N-attachment method is a strict generalization of the single-image one (same
attachment_test_support()guard, samebuild_user_envelope/submit_inbound_with_attachments/wait_for_statussequence). Worth collapsing the single-image path into a delegate to avoid the two copies drifting (e.g. a future change to the guard error message or completion-wait logic only landing in one of them).♻️ Proposed refactor
pub async fn submit_turn_with_image_attachment( &self, text: &str, filename: &str, mime_type: &str, bytes: Vec<u8>, ) -> HarnessResult<TurnRunId> { - if self.capability_recorder.attachment_test_support().is_none() { - return Err( - "no attachment lander wired — build the harness via RebornIntegrationGroup::attachment_tools()" - .into(), - ); - } - let (event_id, envelope) = self.build_user_envelope(text)?; - let attachment = ironclaw_attachments::InboundAttachment { - id: format!("{event_id}-att-0"), - mime_type: mime_type.to_string(), - filename: Some(filename.to_string()), - bytes, - }; - let ack = self - .workflow - .submit_inbound_with_attachments(envelope, vec![attachment]) - .await?; - let run_id = Self::run_id_from_ack(ack)?; - self.wait_for_status(run_id, TurnStatus::Completed).await?; - Ok(run_id) + self.submit_turn_with_attachments(text, vec![(filename, mime_type, bytes)]) + .await }🤖 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 `@tests/support/reborn/builder.rs` around lines 524 - 602, Refactor the duplicated attachment submission flow in submit_turn_with_image_attachment and submit_turn_with_attachments by extracting the shared lander guard, build_user_envelope, submit_inbound_with_attachments, and wait_for_status sequence into a common helper. Make submit_turn_with_image_attachment delegate to the generalized multi-attachment path so the attachment_test_support() check and completion handling stay consistent in one place.
🤖 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.
Outside diff comments:
In `@tests/support/reborn/builder.rs`:
- Around line 524-602: Refactor the duplicated attachment submission flow in
submit_turn_with_image_attachment and submit_turn_with_attachments by extracting
the shared lander guard, build_user_envelope, submit_inbound_with_attachments,
and wait_for_status sequence into a common helper. Make
submit_turn_with_image_attachment delegate to the generalized multi-attachment
path so the attachment_test_support() check and completion handling stay
consistent in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2edb7068-4d3e-4731-bb1b-f079bcf91d3c
⛔ Files ignored due to path filters (3)
tests/snapshots/golden_payload__gated_turn_approve.snapis excluded by!**/*.snap,!tests/snapshots/**tests/snapshots/golden_payload__image_attachment.snapis excluded by!**/*.snap,!tests/snapshots/**tests/snapshots/golden_payload__parallel_tool_calls.snapis excluded by!**/*.snap,!tests/snapshots/**
📒 Files selected for processing (27)
crates/ironclaw_loop_support/src/capability_port.rscrates/ironclaw_reborn_composition/src/extension_lifecycle_capabilities_auth_tests.rscrates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rscrates/ironclaw_reborn_composition/src/test_support/trigger_materializer.rscrates/ironclaw_reborn_composition/src/trigger_poller_trusted_submit.rstests/reborn_group_approvals/main.rstests/reborn_group_approvals/scenario_ask_each_time_resumes_once.rstests/reborn_group_extensions/main.rstests/reborn_group_extensions/scenario_install_unknown_extension_id_fails_safely.rstests/reborn_group_triggers/main.rstests/reborn_group_triggers/scenario_triggered_chained_gate.rstests/reborn_integration_attach.rstests/reborn_integration_auth_gate.rstests/reborn_integration_golden_payload.rstests/reborn_integration_project_create.rstests/reborn_integration_skill_activate.rstests/support/reborn/assertions.rstests/support/reborn/builder.rstests/support/reborn/capability_backend.rstests/support/reborn/golden.rstests/support/reborn/group_constructors.rstests/support/reborn/harness.rstests/support/reborn/mod.rstests/support/reborn/project_service_fault.rstests/support/reborn/reply.rstests/support/reborn/scripted_provider.rstests/support_unit_tests.rs
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5610
July 4, 2026 06:45
Destroyed
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/support/reborn/builder.rs (1)
571-602: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate submission logic with
submit_turn_with_image_attachment.This method repeats the lander-guard error string, envelope build, and submit/wait sequence from
submit_turn_with_image_attachment(Lines 532-559) almost verbatim — only theInboundAttachmentconstruction differs. Havesubmit_turn_with_image_attachmentdelegate to this new method to keep the error message and submission flow in one place.♻️ Proposed consolidation
pub async fn submit_turn_with_image_attachment( &self, text: &str, filename: &str, mime_type: &str, bytes: Vec<u8>, ) -> HarnessResult<TurnRunId> { - if self.capability_recorder.attachment_test_support().is_none() { - return Err( - "no attachment lander wired — build the harness via RebornIntegrationGroup::attachment_tools()" - .into(), - ); - } - let (event_id, envelope) = self.build_user_envelope(text)?; - let attachment = ironclaw_attachments::InboundAttachment { - id: format!("{event_id}-att-0"), - mime_type: mime_type.to_string(), - filename: Some(filename.to_string()), - bytes, - }; - let ack = self - .workflow - .submit_inbound_with_attachments(envelope, vec![attachment]) - .await?; - let run_id = Self::run_id_from_ack(ack)?; - self.wait_for_status(run_id, TurnStatus::Completed).await?; - Ok(run_id) + self.submit_turn_with_attachments(text, vec![(filename, mime_type, bytes)]) + .await }🤖 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 `@tests/support/reborn/builder.rs` around lines 571 - 602, submit_turn_with_attachments duplicates the same guard, envelope creation, submission, and wait flow already present in submit_turn_with_image_attachment. Refactor so submit_turn_with_image_attachment delegates to submit_turn_with_attachments, keeping the lander check, build_user_envelope, submit_inbound_with_attachments, run_id_from_ack, and wait_for_status logic in one place. Preserve the existing attachment-specific InboundAttachment construction in submit_turn_with_attachments and reuse the shared turn submission path for both methods.
🤖 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.
Outside diff comments:
In `@tests/support/reborn/builder.rs`:
- Around line 571-602: submit_turn_with_attachments duplicates the same guard,
envelope creation, submission, and wait flow already present in
submit_turn_with_image_attachment. Refactor so submit_turn_with_image_attachment
delegates to submit_turn_with_attachments, keeping the lander check,
build_user_envelope, submit_inbound_with_attachments, run_id_from_ack, and
wait_for_status logic in one place. Preserve the existing attachment-specific
InboundAttachment construction in submit_turn_with_attachments and reuse the
shared turn submission path for both methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9c89c976-dcf7-4dc1-816a-de96516ff610
📒 Files selected for processing (1)
tests/support/reborn/builder.rs
Contributor
Reborn integration-tier coverageLine coverage (Reborn crates): 17.19% — 11063 / 64362 lines Per-crate breakdown (11 crates, lowest-covered first)
This signal is informational: coverage never gates the PR — not the percentage, not the per-crate holes, not the 0-coverage callout. |
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
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.
test(reborn): wave-4 integration coverage — auth-gate wire regression pack, triggered auth delivery, attachments, golden/synthetic expansions, denied-edge follow-ons
Summary
Behavior-neutral coverage wave for the Reborn backend integration harness, following #5584 (wave 3). Zero production-behavior changes: the diff is tests, test-support, and
#[cfg(test)]/#[cfg(any(test, feature = "test-support"))]-gated code compiled out of default builds (verified). Stacked on the materializer test-support extraction PR (#5609).Wave 4 sources: a 3-week bugfix-commit regression audit (Lane A — each row retro-pins a recurring fix class that shipped with only unit/crate-tier nets) and an uncovered-area sweep (Lane B), plus wave-3 carry-overs (Lane C).
Lane A — regression pack
reborn_integration_auth_gate, new bin): flagship pins the Fix Reborn credential delete and same-run reauth #5174/fix(reborn): populate provider on runtime auth-required gates #5180 class (emptycredential_requirements→ nullprovider→ unsubmittable manual-token gate) at fullsubmit_turntier — neither existing crate-tier pin reaches the coordinator/loop. Plus cancel-leaves-no-stale-replay and non-auth gate-ref negative arms. Enabler found en route: the network-egress lane had no way to script a non-200 — additivestatus_queueFIFO onRecordingNetworkHttpEgress. Closes the recurrence class of fix(reborn): keep OAuth auth gates visible without auth URL #5067/fix(reborn): populate provider on runtime auth-required gates #5180/fix(webui): avoid token prompt for OAuth auth gates #4957/fix: Gmail auth-resume failure — stop run-borking + restore persistent-approval grant #5051/[codex] fix gsuite refresh auth classification #4968.slack_serve/e2e_tests.rs): first tests anywhere exercisingTriggeredNotificationFailure::OAuthTargetNotDm— DM setup-link positive arm + non-DM target suppresses the OAuth link and cancels the run (exactly-one-message,cancel_call_count == 1).ScriptedTriggerCoordinatorgains a scriptable first-poll status (additive constructor). Rest of the W4-TRIGSLACK-SETTLE scope confirmed already pinned (approval settlement + claim-replay tests).capability_port.rs#[cfg(test)], sibling convention): "password"/"traceback" metadata accepted through realCompleteddispatch — closes the fix(safety): relax provider-output validation to stop give-up loops (B+C+D) #5001 test-through-the-caller gap.build_reborn_services-driven): same-tenant SSO fallback resolves / cross-tenantCredentialMissing— the Fix NEAR AI MCP token resolution for SSO users #5439 fix had no composition-tier net.reborn_group_approvalsscenario): approved AskEachTime resume completes in one round trip; re-approve →NotPending. Harness gains generictool_permission_overrides. Flip-checked against the pre-fix(reborn): ask-each-time approval resume loop #5306 check order.Lane B — dark areas
reborn_integration_attach): doc attachment → extracted text reaches the model; multi-attachment single turn with ordinal assertions. Newsubmit_turn_with_attachmentsbuilder helper (generalizes image-only path).reborn_group_extensionsscenario 4): unknownextension_id→Failed{invalid_input}with class-discrimination negative arm. (Original "malformed manifest" scope confirmed unreachable — catalog is compile-time-embedded and always valid; dynamic-manifest arm folds into the hosted-MCP enabler follow-up.)Lane C — carry-overs
reborn_group_triggers): one triggered run, two distinct gates;ScheduledTriggerorigin re-read at all three coordinator checkpoints; both writes + reply persisted in the trigger's thread.reborn_integration_golden_payload): parallel tool_calls, image-attachment turn, gated-turn (snapshots both inference payloads around the gate). Two golden-harness defects fixed while authoring: real-UTC-date leak into snapshots (→<DATE>filter) and a binary-wide shared tool-call-id counter race (→ encounter-order normalization). All 4 pre-existing snapshots byte-identical; 5 consecutive full-suite green runs.validate_explicit_mentions_are_unambiguous);project_createfault injection at theArc<dyn ProjectService>seam (Deniedarm;Unavailable/Internalarms blocked on reborn: retry path is unreachable for local-dev synthetic capabilities — reused input_ref fails staging check, collapsing retry contract into terminal driver_unavailable #5608 — see below).validate_trusted_trigger_prompt) via a cfg-gated helper — closes a harness-fidelity gap where the hand-mirrored path skipped the safety scan; net −67 lines.#[ignore]d — reborn: HookedLoopCheckpointPort does not forward stage_checkpoint_payload/load_checkpoint_payload — any hooks-enabled coordinator turn fails at Checkpoint stage #5572 still open.Production findings from this wave (issues, not fixes here)
ProductionMemoryPromptContextServicenever composed;ThreadBackedLoopContextPorthardcodesmemory_snippets: Vec::new(); theUntrusted memory content:injection-hardening envelope never fires. (Blocked W4-MEMCTX-ENVELOPE; test lands with the fix.)input_ref,LocalDevCapabilityIorejects non-staged refs → terminaldriver_unavailable; retryable failure classes kill the run whileInvalidInputcontinues.Verification
cargo build --tests --all-features;cargo clippy --all --tests --examples --all-features -- -D warnings;cargo fmt --check;scripts/check_no_panics.py— all clean--features integration; composition crate--libgreen incl.slack-v2-host-betalaneironclaw_reborn_composition+ironclaw_loop_supportcompile all additions out🤖 Generated with Claude Code