Repository navigation
fix(ci): restore main coverage gates - #7493
Conversation
|
🚅 Deployed to the ironclaw-pr-7493 environment in ironclaw-ci-preview
|
github-actions
Bot
added
size: XS
|
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 (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR expands Telegram reaction delivery tests for payloads and vendor outcomes. It also updates end-to-end expectations for admin configuration groups and the Web UI notification-channel empty state. ChangesTelegram delivery tests
End-to-end assertion updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. 🟩 Final result · Completed
Automatic trigger · attempt 1 of 3 · completed in 7m 47s IronLoop completed the review and posted it to GitHub. 🔗 Result |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 IronLoop review
One medium correctness gap was found in the expanded Telegram reaction coverage.
Findings: 🟠 Medium 1
🟠 Medium · Require result:true before reporting a reaction as sent
Inline on crates/extensions/packages/telegram/src/tests/channel_deliver.rs:222. See the inline comment for details.
Validation
- ✅ Telegram reaction tests — Focused reaction delivery tests passed: 5 passed, 0 failed.
- ⚪ Served-browser assertions — Not run. Static review verified the added admin heading against the Web Push manifest and the notification copy against the rendered localization key; no browser run was needed for a concrete concern.
Review details
- Run:
a2766cd9-a350-4d04-a0cc-a107b5171d0e - Workflow: Review
- Attempts: 1
| for (response, expected_reason) in [ | ||
| ( | ||
| Ok(RestrictedEgressResponse { | ||
| status: 429, | ||
| body: Vec::new(), | ||
| }), | ||
| "status 429", | ||
| ), | ||
| (ScriptedEgress::ok("not-json"), "was not valid JSON"), | ||
| ] { |
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🟠 Medium · Require result:true before reporting a reaction as sent
The added failure matrix covers HTTP 429 and invalid JSON, but not parsed `ok:true` responses with false or missing `result`. The reaction implementation treats any parsed `ok:true` response as `Sent`, while the equivalent delete path requires `result:true`; the delivery coordinator consequently records an all-sent report as delivered. A response such as `{"ok":true,"result":false}` can therefore suppress a failed reaction without retrying it. Add cases for false/missing result evidence and require `result:true` before returning `Sent`.
This branch was successfully deployed
2 participants
Summary
Change Type
Linked Issue
None. Unblocks failing
mainruns 31468070491 and 31468070517.Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warnings— Not applicable: test-only changes covered by owning-crate compile/tests and targeted served-browser E2E.cargo build— Not applicable: both the owning-crate tests and served-browser harness compile the affected test targets and shipping binary.cargo test -p ironclaw_telegram_extension(116 passed)cargo test -p ironclaw_telegram_extension deliver_react_(5 passed)uv run --project tests/e2e pytest tests/e2e/scenarios/test_admin_api.py::test_admin_configuration_renders_uninstalled_manifest_groups_and_keeps_secrets_write_only tests/e2e/scenarios/test_reborn_webui_v2_legacy_extensions.py::test_reborn_v2_current_extension_setup_and_delivery_matrix -q(2 passed)cargo llvm-cov -p ironclaw_telegram_extension --summary-only(116 passed; new tests execute more than the six previously missing lines required by the ratchet)cargo test -p <owning-crate> --features integration— Not applicable: no database-backed or runtime-integration behavior changed.review-prorpr-shepherd --fixwas run before requesting review — Not run; narrow test-only diff reviewed locally.Test Strategy
User behavior: Main's deterministic coverage gates accept the shipped Web Push configuration surface, current notification empty state, and Telegram reaction behavior so release branching can proceed from a green commit.
Risk areas:
Tests added or updated:
ChannelAdapter::delivercaller-path tests cover all reaction mappings and provider failure classifications.What the tests prove: Telegram reaction delivery cannot falsely report success for malformed or rejected vendor responses, each neutral reaction maps to the intended Telegram emoji, and the served SPA reflects the current Web Push/admin and notification-empty-state contracts.
Commands run: listed under Validation.
Security Impact
None. Test-only changes; no permissions, network policy, secrets, filesystem, tool execution, or sandbox behavior changed.
Reborn Trust-Boundary Checklist
N/A: test-only changes do not alter Reborn trust-bearing types, ingress, persistence, runtime, or error contracts.
Database Impact
None.
Blast Radius
Limited to Telegram adapter tests and two served-browser E2E expectations. Production code is unchanged.
Rollback Plan
Revert commit
c00270eeccif the updated expectations do not match the intended product contract.Review Follow-Through
The full Code Coverage workflow is push-to-main only. After merge, verify both
Tests (Reborn)andCode Coverageon the merge SHA before cutting the release branch.Review track: A (tests/chore)