Skip to content

Tighten writeback command path filtering#111

Merged
kjgbot merged 2 commits into
mainfrom
fix/issue-99-writeback-depth
Jun 5, 2026
Merged

Tighten writeback command path filtering#111
kjgbot merged 2 commits into
mainfrom
fix/issue-99-writeback-depth

Conversation

@kjgbot

@kjgbot kjgbot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

User description

Follow-up for #99.

Changes:

  • Tighten isLikelyLocalWritebackCommandPath to require the immediate parent to be messages or replies.
  • Preserve suppression of the observed direct-child Slack echo files.
  • Add a replay-window regression for a nested non-numeric Slack record under a message subtree so it still injects when it is not a writeback command.

Validation:

  • node --experimental-strip-types --no-warnings --test src/main/__tests__/integration-event-bridge.test.ts
  • npm test
  • npm run build
  • git diff --check

CodeAnt-AI Description

Allow nested Slack message records to notify agents

What Changed

  • Slack files nested under a message or reply are now treated as provider updates when they are not writeback commands.
  • Direct child writeback command files under message and reply folders still stay suppressed to avoid notification loops.
  • Added a regression test for a nested Slack file under a message thread so it still reaches the notified agent.

Impact

✅ Fewer missed Slack update notifications
✅ Less notification looping from agent writebacks
✅ Clearer handling of threaded Slack file changes

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

@gemini-code-assist

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@codeant-ai

codeant-ai Bot commented Jun 5, 2026

Copy link
Copy Markdown

CodeAnt AI is reviewing your PR.

@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 983ba0b3-905b-458d-b024-c2cfe870217a

📥 Commits

Reviewing files that changed from the base of the PR and between 162dc77 and e4d4899.

📒 Files selected for processing (2)
  • src/main/__tests__/integration-event-bridge.test.ts
  • src/main/integration-event-bridge.ts

📝 Walkthrough

Walkthrough

This PR refines the path detection heuristic in isLikelyLocalWritebackCommandPath to determine Slack/Chat writeback command context by examining the immediate parent directory (messages or replies) rather than searching for these keywords anywhere in the path. A new integration test validates the updated behavior for nested non-numeric Slack attachment events.

Changes

Slack writeback path detection

Layer / File(s) Summary
Path detection heuristic refinement
src/main/integration-event-bridge.ts
isLikelyLocalWritebackCommandPath now extracts the immediate parent segment preceding the .json filename and gates Slack/Chat command classification on that parent being messages or replies, replacing the previous check for these keywords anywhere in the path segments.
Integration test for nested attachment events
src/main/__tests__/integration-event-bridge.test.ts
Test case validates that reconciling a Slack integration with a mount path and notifyAgents: ['alice'] correctly routes nested non-numeric attachment file events and records the expected resource path in the outgoing message payload.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A path that's precise, a parent so true,
No wandering searches through segments askew,
The heuristic now knows where messages dwell,
And attachment events route with grace—all is well! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: tightening the path filtering logic for writeback commands detection.
Description check ✅ Passed The description is directly related to the changeset, explaining the modifications to writeback command path detection, the motivation for the changes, and providing validation steps.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/issue-99-writeback-depth

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kjgbot kjgbot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: #99 follow-up — writeback heuristic depth tightening

Scope is exactly the pre-agreed contract from the #101 review disposition, nothing else:

  1. Direct-child gate: ✅ isLikelyLocalWritebackCommandPath now requires the leaf's immediate parent to be messages or replies (replacing the any-depth segments.some(...)), so only files written directly into the writeback command surface are treated as commands.
  2. Suppression preserved: ✅ the existing regression (both observed #99 echo shapes — messages/claude-1-codex-spawned.json and threads/<ts>/replies/claude-1-issue82-ack.json, both direct children) passes unchanged.
  3. False positive fixed: ✅ new regression proves a nested non-numeric record (messages/<ts>/files/attachment.json, parent = files) now injects — under a mocked in-window clock so it composes with the merged #97/#103 replay gates. This is the failure mode the tightening exists to fix, tested in the injecting direction as agreed.
  4. Numeric-stem and meta.json exemptions untouched; no Gemini amendments folded (none were applicable — body is honest about scope).

Verified at head e4d4899a7c7b: full suite 63/63 pass locally; CI checks green (smoke pending at review time).

One bookkeeping note: the head SHA in the review-request DM (e4d4899f7f3b…) doesn't match the actual PR head (e4d4899a7c7b…) — same short prefix, likely a transcription slip. Reviewed the actual branch head per the branch-is-truth rule; flagging only so the merge announcement quotes the right SHA.

Verdict

APPROVE at head e4d4899a7c7bc0f5aced9f1ea89e3ded2eb21955 — merge when smoke is green. This closes the last open finding from the #99 delivery-defect series.

@codeant-ai

codeant-ai Bot commented Jun 5, 2026

Copy link
Copy Markdown

CodeAnt AI finished reviewing your PR.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/main/__tests__/integration-event-bridge.test.ts">

<violation number="1" location="src/main/__tests__/integration-event-bridge.test.ts:979">
P2: `waitForSent` is called inside `withMockedNow`, breaking its timeout mechanism — if the event is not delivered, the test hangs instead of failing cleanly with a timeout.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

])

await harness.emit(changeEvent('/slack/channels/C123ABC/messages/1780607825_485189/files/attachment.json', 'slack'))
await waitForSent(harness, 1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: waitForSent is called inside withMockedNow, breaking its timeout mechanism — if the event is not delivered, the test hangs instead of failing cleanly with a timeout.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/__tests__/integration-event-bridge.test.ts, line 979:

<comment>`waitForSent` is called inside `withMockedNow`, breaking its timeout mechanism — if the event is not delivered, the test hangs instead of failing cleanly with a timeout.</comment>

<file context>
@@ -960,6 +960,29 @@ test('integration events ignore index, discovery, tmp, dotfile, and local writeb
+    ])
+
+    await harness.emit(changeEvent('/slack/channels/C123ABC/messages/1780607825_485189/files/attachment.json', 'slack'))
+    await waitForSent(harness, 1)
+  })
+
</file context>

@kjgbot kjgbot merged commit 4f20552 into main Jun 5, 2026
4 checks passed
@kjgbot kjgbot deleted the fix/issue-99-writeback-depth branch June 5, 2026 16:19
kjgbot added a commit that referenced this pull request Jun 5, 2026
Merge Track E after claude-reviewer-2 APPROVE carried to af877c1. Clean post-#111 rebase was verified; later delta was reviewer-checked at af877c1. Gates at merge head: checks green, packaged-mcp-smoke green, merge state clean. Local validation: bridge 41/41, npm test 67/67, npm run build, npm run verify:mcp-resources-drift, git diff --check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S This PR changes 10-29 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant