Repository navigation
test(reborn): int-tier coverage for trigger-management verbs (T0-TRIGGERS) - #5482
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new ChangesTrigger group test suite
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant TokioTest
participant RebornIntegrationGroup
participant ScenarioVerbsLifecycle
participant TriggerCapabilities
TokioTest->>RebornIntegrationGroup: triggers()
TokioTest->>ScenarioVerbsLifecycle: run(&g)
ScenarioVerbsLifecycle->>TriggerCapabilities: builtin.trigger_create
ScenarioVerbsLifecycle->>TriggerCapabilities: builtin.trigger_list
ScenarioVerbsLifecycle->>TriggerCapabilities: builtin.trigger_pause
ScenarioVerbsLifecycle->>TriggerCapabilities: builtin.trigger_resume
ScenarioVerbsLifecycle->>TriggerCapabilities: builtin.trigger_remove
ScenarioVerbsLifecycle->>TriggerCapabilities: builtin.trigger_list
ScenarioVerbsLifecycle-->>TokioTest: ScenarioReport::assert_all_passed()
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 |
There was a problem hiding this comment.
Code Review
This pull request introduces a new integration test suite for trigger-management verbs, including trigger_create, trigger_list, trigger_pause, trigger_resume, and trigger_remove. It adds the reborn_group_triggers test binary, implements the verbs_lifecycle scenario to verify cross-thread persistence and management of triggers, and exposes the necessary harness and group builders (RebornIntegrationGroup::triggers()) along with a new assertion helper tool_result_output to fetch the output of the most recent capability result. I have no feedback to provide as there are no review comments and the implementation is clean and well-documented.
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.
There was a problem hiding this comment.
Pull request overview
Adds an int-tier “group” integration test that exercises the full trigger-management verb lifecycle through the real agent-loop turn → capability dispatch path, and introduces small test-support seams to enable that coverage (shared runtime, auto-approve, and reading server-minted outputs like trigger_id).
Changes:
- Add
reborn_group_triggersgroup integration test (two threads sharing one trigger repo) coveringtrigger_create/list/pause/resume/remove. - Expose a trigger-management harness constructor (
trigger_management_tools) and wire a newRebornIntegrationGroup::triggers()builder path. - Add
RebornIntegrationHarness::tool_result_output(capability_id)to return parsed JSON output of the most recent recorded capability result.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/support/reborn/harness.rs | Makes trigger-management harness constructor pub(crate) so group builder can reuse it. |
| tests/support/reborn/group.rs | Adds RebornIntegrationGroup::triggers() and builder support for trigger-management capabilities. |
| tests/support/reborn/CLAUDE.md | Documents the new group type in the test-support guide. |
| tests/support/reborn/assertions.rs | Adds tool_result_output helper to return recorded tool output JSON. |
| tests/reborn_group_triggers/main.rs | New group test binary orchestrating the trigger lifecycle scenario(s). |
| tests/reborn_group_triggers/scenario_verbs_lifecycle.rs | Scenario implementing create/list in thread A and pause/resume/remove/list in thread B. |
| Cargo.toml | Registers the new reborn_group_triggers test target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
🚅 Deployed to the ironclaw-pr-5482 environment in ironclaw-ci-preview
|
Reborn integration-tier coverageLine coverage (Reborn crates): 14.87% — 9155 / 61547 lines Per-crate breakdown (12 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. |
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add Reborn integration coverage for trigger-management verbs through the real agent-loop and capability-dispatch path.
Stats: 2 findings (from 3 raw, 2 after validation/filtering) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Local Patterns
-
Low New assertion accessor is missing from the harness docs index (
tests/support/reborn/assertions.rs:144-147, confidence 75) — anchor:tests/support/reborn/CLAUDE.md:146
tests/support/reborn/CLAUDE.mdkeeps a navigable list of the richerassertions.rsAPIs, but the newtool_result_outputhelper is not added there. Future group-test authors following that local doc will still only seeassert_tool_result_contains, even though this PR adds the value-returning accessor specifically for server-minted fields. -
Nit Group test entrypoint drops the local
_e2esuffix (tests/reborn_group_triggers/main.rs:31, confidence 75) — anchor:tests/reborn_group_memory/main.rs:25andtests/reborn_group_extensions/main.rs:31
The existing Reborn group binaries name their top-level tokio tests with the<domain>_group_e2eshape, but this new binary usestriggers_group. That makes it the only group binary missed by normal_group_e2esearch or cargo-test filtering patterns.
Note: I did not duplicate the existing unresolved Copilot thread on trigger_id ownership in scenario_verbs_lifecycle.rs; that issue is already open on the current PR head.
- Rename `triggers_group` -> `triggers_group_e2e` to match the sibling group-test entrypoint convention (memory_group_e2e, extensions_group_e2e, approvals_group_e2e), so `_group_e2e`-scoped filters find it. - Add `tool_result_output` to the CLAUDE.md richer-assertions index; it was added to assertions.rs but never indexed alongside its siblings. The Copilot review comment on this PR (trigger_id String reuse across multiple json! calls) is not applicable: serde_json's json! macro expands `json!($other)` to `to_value(&$other)` (borrows, never moves) for arbitrary expressions, so the value is never consumed and no clone is needed — the existing 5x reuse already compiles clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add integration-tier Reborn trigger-management verb coverage through the agent-loop to capability dispatch path.
Stats: 0 findings (from 0 raw, 0 after dedup) across 0 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
No new actionable findings from this forced pass.
Live unresolved threads were checked before posting and not duplicated here, including the existing trigger_id move concern with the author's rebuttal and the existing harness-docs/index comment.
- Rename `triggers_group` -> `triggers_group_e2e` to match the sibling group-test entrypoint convention (memory_group_e2e, extensions_group_e2e, approvals_group_e2e), so `_group_e2e`-scoped filters find it. - Add `tool_result_output` to the CLAUDE.md richer-assertions index; it was added to assertions.rs but never indexed alongside its siblings. The Copilot review comment on this PR (trigger_id String reuse across multiple json! calls) is not applicable: serde_json's json! macro expands `json!($other)` to `to_value(&$other)` (borrows, never moves) for arbitrary expressions, so the value is never consumed and no clone is needed — the existing 5x reuse already compiles clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d3c73c3 to
1ee59f8
Compare
- Rename `triggers_group` -> `triggers_group_e2e` to match the sibling group-test entrypoint convention (memory_group_e2e, extensions_group_e2e, approvals_group_e2e), so `_group_e2e`-scoped filters find it. - Add `tool_result_output` to the CLAUDE.md richer-assertions index; it was added to assertions.rs but never indexed alongside its siblings. The Copilot review comment on this PR (trigger_id String reuse across multiple json! calls) is not applicable: serde_json's json! macro expands `json!($other)` to `to_value(&$other)` (borrows, never moves) for arbitrary expressions, so the value is never consumed and no clone is needed — the existing 5x reuse already compiles clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1ee59f8 to
6129395
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/support/reborn/assertions.rs`:
- Around line 140-199: `assert_tool_error` currently scans the entire thread
history instead of the per-thread baseline delta, which can produce stale or
cross-turn matches. Update `assert_tool_error` in `assertions.rs` to use the
same `[baseline..]` scoping pattern as the other assertion helpers, threading
the baseline history length through the harness if needed, and keep the
`ToolResultReferenceEnvelope`/`safe_summary` matching logic intact. Use the
existing `assert_tool_result_contains` and egress assertion patterns as the
reference for locating the baseline-sliced history handling.
🪄 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: 4a897c23-99b9-4b5c-a112-863a775588b9
📒 Files selected for processing (7)
Cargo.tomltests/reborn_group_triggers/main.rstests/reborn_group_triggers/scenario_verbs_lifecycle.rstests/support/reborn/CLAUDE.mdtests/support/reborn/assertions.rstests/support/reborn/group.rstests/support/reborn/harness.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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 `@tests/support/reborn/assertions.rs`:
- Around line 140-199: `assert_tool_error` currently scans the entire thread
history instead of the per-thread baseline delta, which can produce stale or
cross-turn matches. Update `assert_tool_error` in `assertions.rs` to use the
same `[baseline..]` scoping pattern as the other assertion helpers, threading
the baseline history length through the harness if needed, and keep the
`ToolResultReferenceEnvelope`/`safe_summary` matching logic intact. Use the
existing `assert_tool_result_contains` and egress assertion patterns as the
reference for locating the baseline-sliced history handling.
🪄 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: 4a897c23-99b9-4b5c-a112-863a775588b9
📒 Files selected for processing (7)
Cargo.tomltests/reborn_group_triggers/main.rstests/reborn_group_triggers/scenario_verbs_lifecycle.rstests/support/reborn/CLAUDE.mdtests/support/reborn/assertions.rstests/support/reborn/group.rstests/support/reborn/harness.rs
🛑 Comments failed to post (1)
tests/support/reborn/assertions.rs (1)
140-199: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
assert_tool_errorscans full thread history, not the per-thread baseline delta.The richer
assertions.rshelpers are documented to operate on the[baseline..]delta per thread; this one deliberately doesn't, with an extensive comment flagging it as safe only for single-turn/single-tool-call harnesses and requiring baseline scoping before any multi-turn/group reuse. Since no caller in this PR (or visible elsewhere) exercises it from a multi-turn/group context yet, this is currently inert risk, but flagging since the two-thread trigger scenario in this same PR is exactly the kind of group-test surface that could accidentally reach for this helper next and hit stale/false-positive matches from thread A's history.Based on path instructions: "The richer egress and tool-result assertions ... belong in
assertions.rsand should operate on the per-thread baseline delta."🤖 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/assertions.rs` around lines 140 - 199, `assert_tool_error` currently scans the entire thread history instead of the per-thread baseline delta, which can produce stale or cross-turn matches. Update `assert_tool_error` in `assertions.rs` to use the same `[baseline..]` scoping pattern as the other assertion helpers, threading the baseline history length through the harness if needed, and keep the `ToolResultReferenceEnvelope`/`safe_summary` matching logic intact. Use the existing `assert_tool_result_contains` and egress assertion patterns as the reference for locating the baseline-sliced history handling.Source: Path instructions
…5482) Copilot review: the pause/resume/remove assertions checked the returned `state`/`removed` flag but not that the returned `trigger` is the same `trigger_id` thread A created. In a shared/dirty repo that could mask a `set_scoped_trigger_state`/`remove_scoped_trigger` bug touching the wrong record while still returning updated/removed=true. Fold a `["trigger"]["trigger_id"] == trigger_id` check into each verb's condition — matches this file's own "keep list assertions id-scoped" guidance. Mutation-verified: pointing the pause check at a bogus id turns it RED for the right reason (real returned id != bogus), then reverted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…GERS)
Add `reborn_group_triggers`, a group integration test that drives the five
trigger-management verbs (trigger_create/list/pause/resume/remove) through the
real agent-loop turn -> capability dispatch path — the only int-tier coverage
of the pause/resume/remove/list handlers (composition-tier trigger_poller_e2e
invokes only trigger_create directly, and pause/resume are dispatched through
the real capability path nowhere else).
The single scenario:
- Thread A: create a one-shot `Once{at}` trigger + list it, reading back the
server-minted `trigger_id` via a new `tool_result_output` accessor.
- Thread B (different conversation, same trigger scope over the shared repo):
pause -> resume -> remove by that id, then list to confirm removal — which
also proves cross-thread trigger persistence.
Deliberately does NOT re-cover the one-shot fire -> Completed derivation
(owned by trigger_poller_e2e.rs + repository_contract.rs) or the trigger ->
Slack outbound leg (owned by slack_host_beta.rs) — consolidate, don't
proliferate. TODO blocks record the deferred triggered-turn coverage that
needs an unmerged submit/auth-gate/outbound harness seam.
Seams (test-support only; no production-crate changes):
- `HostRuntimeCapabilityHarness::trigger_management_tools()` -> `pub(crate)`.
- `RebornIntegrationGroup::triggers()` mirroring `extension_lifecycle()`.
- `RebornIntegrationHarness::tool_result_output(capability_id)` to read a
server-minted result field a static script cannot know ahead of time.
Mutation-verified: resume->paused dispatch mutation and a no-op remove
mutation each turn the relevant assertion RED for the right reason.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI flagged unformatted lines (method-chain wrap width) in the new files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Rename `triggers_group` -> `triggers_group_e2e` to match the sibling group-test entrypoint convention (memory_group_e2e, extensions_group_e2e, approvals_group_e2e), so `_group_e2e`-scoped filters find it. - Add `tool_result_output` to the CLAUDE.md richer-assertions index; it was added to assertions.rs but never indexed alongside its siblings. The Copilot review comment on this PR (trigger_id String reuse across multiple json! calls) is not applicable: serde_json's json! macro expands `json!($other)` to `to_value(&$other)` (borrows, never moves) for arbitrary expressions, so the value is never consumed and no clone is needed — the existing 5x reuse already compiles clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…5482) Copilot review: the pause/resume/remove assertions checked the returned `state`/`removed` flag but not that the returned `trigger` is the same `trigger_id` thread A created. In a shared/dirty repo that could mask a `set_scoped_trigger_state`/`remove_scoped_trigger` bug touching the wrong record while still returning updated/removed=true. Fold a `["trigger"]["trigger_id"] == trigger_id` check into each verb's condition — matches this file's own "keep list assertions id-scoped" guidance. Mutation-verified: pointing the pause check at a bogus id turns it RED for the right reason (real returned id != bogus), then reverted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
430cd95 to
eab194e
Compare
What
Adds
reborn_group_triggers, a group integration test driving the fivetrigger-management verbs (
trigger_create/list/pause/resume/remove)through the real agent-loop turn → capability dispatch path (product
workflow → agent loop → real
ironclaw_llmchain → first-party handler →trigger repo). This is the only int-tier coverage of the
pause/resume/remove/listhandlers —pause/resumeare dispatched through the realcapability path nowhere else in the repo (only repo-level unit tests in
repository_contract.rsand input-parse unit tests intrigger_management.rs).Scenario
Once{at}trigger +listit; read back theserver-minted
trigger_id(a static script can't know it ahead of time).pause→resume→removeby that id, thenlistto confirm removal —which also proves cross-thread trigger persistence.
Consolidate, don't proliferate
Deliberately does not re-cover work already owned elsewhere:
Completedderivation →crates/ironclaw_reborn_composition/tests/trigger_poller_e2e.rs(real poller) +crates/ironclaw_triggers/tests/repository_contract.rs.crates/ironclaw_reborn_composition/src/slack_host_beta.rs.TODO blocks in
main.rsrecord the deferred triggered-turn coverage(approval gate mid-fire,
ScheduledTriggerorigin propagation, outbound sink),which needs a not-yet-existing harness seam to submit a turn on the
TrustedTriggerFireSubmitterpath — do not hand-roll a weaker stand-in.Seams (test-support only — no production-crate changes)
HostRuntimeCapabilityHarness::trigger_management_tools()→pub(crate).RebornIntegrationGroup::triggers()mirroringextension_lifecycle().RebornIntegrationHarness::tool_result_output(capability_id)— returns theparsed JSON of the most-recent recorded capability result, to read a
server-minted field a static script can't reference.
Verification
cargo test --features libsql --test reborn_group_triggers→ green.cargo clippy --all-features --tests→ 0 warnings.reason, then reverted): resume dispatch arm
Scheduled→Paused; libsqlremove_scoped_triggermade a no-op.maintainability / bugs); findings addressed.
🤖 Generated with Claude Code