Repository navigation
test(webui): use the live extension id in the notification-setup boundary test - #8058
Conversation
…dary test #8038 added api-boundary.test.ts with "web-push" as an arbitrary example extension id. That spelling was retired for "web-app" in #7477 and the architecture gate retired_web_push_spelling_stays_at_zero_occurrences pins it at zero outside the persisted-compat allowlist, so main has been red on Tests (Reborn) since 666ebcb. The string carries no persisted or compat meaning (the function under test interpolates any id verbatim), so rename rather than widen the allowlist. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6JWfzEzWKThikJ9wsHAer
|
🚅 Deployed to the ironclaw-pr-8058 environment in ironclaw-ci-preview
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
size: XS
There was a problem hiding this comment.
🟢 Approval recommended
The change is a narrow, verified test-fixture string correction that aligns with the enforced extension-id spelling and does not alter runtime behavior.
Pull request overview
Fixes a frontend unit-test fixture in ironclaw_webui so it uses the live Web App extension id (web-app) instead of the retired web-push spelling, unblocking the architecture gate that enforces zero occurrences of the old identifier.
Changes:
- Update the mocked notification-setup API response fixture to return
extension_id: "web-app". - Update the test invocation to call
getNotificationSetupStatuswithextensionId: "web-app".
File summaries
| File | Description |
|---|---|
| crates/product/ironclaw_webui/frontend/src/lib/api-boundary.test.ts | Replaces a retired extension id string in a notification-setup boundary test fixture to satisfy the architecture “zero occurrences” gate. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe notification setup validation test now uses ChangesNotification setup validation
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The notification setup boundary test now uses the current extension identifier while retaining its malformed-response validation. No production behavior changes, and the update is ready to merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is complete and relevant. It covers the summary, change type, linked issue, validation, test strategy, security, trust-boundary, database, blast-radius, rollback, follow-through, and review track. It also identifies the violated zero-occurrence architecture invariant and explains why the fixture rename is safe. 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 |
Review · Status🟩 CompletedIronLoop completed the review and posted it to GitHub. ResultRun detailsAutomatic trigger · attempt 1 of 3 · completed in 5m 29s |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review · Summary
🟢 No actionable findings
No actionable findings. The fixture now consistently uses the live extension ID while preserving the malformed-response boundary assertion.
Validation
- ✅ Frontend boundary test — The notification-setup boundary test passed (4 tests).
- ✅ Retired-identifier architecture gate — The gate confirming no unsanctioned retired extension spelling passed.
- ✅ Diff inspection — The changed patch has no whitespace errors.
Review details
- Run:
6c88f59a-6234-45e6-9f51-40e6a417cbf1 - Attempts: 1
lloydmak99
left a comment
lloydmak99
left a comment
There was a problem hiding this comment.
Straightforward two-line test fixture rename (web-push → web-app) to use the live extension id. The assertion in api-boundary.test.ts:93-109 is driven by the enabled/requires_setup type checks in decodeNotificationSetupStatus; the extension id is interpolated verbatim and never branched on, so the rename can't change behavior. The retired-vocabulary architecture gate confirms api-boundary.test.ts is not in its allowlist, so the old spelling was a genuine violation and web-app is the correct id.
No blocking issues found.
Checks: pnpm vitest run --project unit src/lib/api-boundary.test.ts passed (4 tests); git diff --check passed. The architecture gate couldn't compile locally (linker cc unavailable) but passed in CI.
Summary
mainhas failedTests (Reborn)since666ebcbf0(refactor(webui): type and validate frontend API boundaries #8038): its newapi-boundary.test.tsused"web-push"as an arbitrary example extension id, and the architecture gateretired_web_push_spelling_stays_at_zero_occurrencespins that retired spelling (renamed toweb-appin refactor(channels): normalize ingress and split reply from delivery #7477) at zero outside the persisted-compat allowlist."web-app". The function under test interpolates any id verbatim and has no channel-specific logic, so the string carries no persisted or compat meaning — renaming is correct; widening the allowlist would defeat its shrink-only invariant.Change Type
Linked Issue
None — CI regression on
main; no issue was filed for it.Validation
cargo test -p ironclaw_architecture_tests retired_web_push_spelling_stays_at_zero_occurrences— green with this change applied on the feat(loop): derive the prompt context budget from the model's advertised window #8053 branch (which mergedmainand inherited the failure); red without it.pnpm vitest run src/lib/api-boundary.test.ts) — the assertion is about a malformed response, not the id.Test Strategy
User behavior: none — test fixture string only.
Risk areas: none.
Tests added or updated:
api-boundary.test.tsfixture idweb-push→web-app(2 lines). Reason: the gate requires the live spelling; the test's assertion is unchanged.What the tests prove: the architecture gate passes again on
main; the boundary test still rejects a malformedenabled: "yes"response.Commands run: see Validation.
Security Impact
None.
Reborn Trust-Boundary Checklist
N/A — test fixture string.
Database Impact
None.
Blast Radius
One frontend unit-test file. Unblocks
Tests (Reborn)for every PR that merges currentmain.Rollback Plan
Revert the commit.
Review Follow-Through
Same commit is cherry-picked onto #8053 so its CI is green now; when this lands, that commit becomes a no-op at merge.
Review track: A (test fix)
🤖 Generated with Claude Code