Harden integration local watcher command roots#102
Conversation
|
CodeAnt AI is reviewing your PR. |
|
Warning Review limit reached
More reviews will be available in 1 minute and 42 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR refines the validation logic for local command root eligibility by tightening ChangesLocal Command Root Validation
🎯 1 (Trivial) | ⏱️ ~3 minutes
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
There was a problem hiding this comment.
Code Review
This pull request refactors the isBoundedLocalCommandRoot function to only match the '.outbox' segment, removing the check for 'outbox'. Additionally, a corresponding integration test has been added to verify that local fallback watchers require historical download even for command roots. I have no feedback to provide.
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.
kjgbot
left a comment
There was a problem hiding this comment.
Review: Track A hardening follow-up (#82) — re-check of PR #98 findings
This PR is exactly the two non-blocking findings from my PR #98 review, nothing else:
isBoundedLocalCommandRootnow accepts dotted.outboxonly (src/main/integration-event-bridge.ts:487) — closes the latent hole where a provider history resource literally namedoutbox(e.g. a mail provider) with history-on would have received a recursive watcher, violating the:479invariant. ✅- New isolated test:
downloadHistoricalData: false+/slack/.outboxmount/glob →[]watcher roots — pins the history off-switch independently of the command-root gate. ✅
Verified at head 8f17cf9: full suite 39/39 pass locally; CI checks green, CodeRabbit pass (packaged-mcp-smoke pending at review time). Scope is minimal and conforms to Track A; no behavioral surface beyond the watcher gate.
Verdict
APPROVE — merge when CI completes. Coordination note: #101 touches the same file/test regions; second-to-merge rebases.
|
✅ pr-reviewer applied fixes — committed and pushed Reviewed PR #102 and made one small test-only fix: added a regression proving bare Validation run: |
kjgbot
left a comment
There was a problem hiding this comment.
Delta re-check: 8f17cf9 → 7909aed
Verified the delta is test-only: one added test (local fallback watchers reject bare outbox command roots) asserting downloadHistoricalData: true + /slack/outbox mount/glob → [] watcher roots. No production code changed — confirmed by direct range diff. This pins the exact hole the hardening closed (bare outbox no longer counts as a command root even with history on). Independently ran the full suite at 7909aed: 40/40 pass.
Verdict
APPROVE (refreshed at head 7909aed) — merge when CI completes on this head.
7909aed to
fa16a6f
Compare
fa16a6f to
8c6d9bf
Compare
|
Clean rebase note before merge: rebased #102 over #100 merge |
|
Reviewed PR #102 against Validation run: |
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. Reviewed PR #102 against Validation run: |
Summary
.outboxonly, removing the bareoutboxsegment match flagged in review.downloadHistoricalData: falsestill creates zero local watcher roots even for a.outboxcommand mount.Issue / Review Finding
Follow-up to #82 / PR #98 reviewer finding: a provider history resource literally named
outboxcould otherwise be treated as a bounded command root when historical download is enabled. Issue #82 and the local command surface use dotted.outbox; this PR keeps the recursive local fallback watcher limited to that hidden command root.Tests
node --experimental-strip-types --no-warnings --test src/main/__tests__/integration-event-bridge.test.tsnpm testnpx vitest run src/main/integration-mounts.test.tsScope
Track A only. No remote subscription/replay, dispatcher, payload, logging, fanout, or mount-budget changes.