fix(plugins): isolate cmdRun results to requested id; gate gmail cursor on dryRun - #2435
Open
Tanmay9223 wants to merge 1 commit into
Open
Conversation
…or on dryRun (closes santifer#2354) runHook() runs every enabled plugin of the requested hook kind and returns [{id, ok, result}] for all of them. cmdRun never filtered by plugin id, so running `node plugins.mjs run linkedin-alerts` with gmail also enabled silently merged gmail's results in and reported them under the linkedin-alerts name. Separately, plugins/gmail/index.mjs called saveProcessedIds() unconditionally, so `--dry-run` permanently marked Gmail messages as processed even though nothing was written to the pipeline. This is the same pattern as ctx.dryRun already used by plugins/notion. - plugins.mjs: filter results to r.id === id before flatMap - plugins/gmail/index.mjs: gate saveProcessedIds behind !ctx.dryRun - test-all.mjs: two regression tests for both invariants (santifer#2354)
Contributor
📝 WalkthroughWalkthroughThe plugin engine now limits collected hook results to the requested plugin. Gmail now avoids saving processed message IDs during dry runs. Regression tests cover both behaviors. ChangesPlugin execution safeguards
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins.mjs (1)
154-158: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winApply the plugin-ID filter to every hook path.
Line 158 filters only
ingestandsearchresults. Theexportandnotifybranches still iterate allrunHookresults, sonode plugins.mjs run <id> exportornotifycan report results from other enabled plugins. Filter results before each branch consumes them, or centralize the filter afterrunHook.🤖 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 `@plugins.mjs` around lines 154 - 158, Apply the requested plugin-ID filtering consistently across all hook branches in the run command: ensure results returned by runHook are restricted to entries whose id matches the requested id, including export and notify, before those branches consume them. Reuse the existing filtering logic near the ingest/search handling rather than leaving those paths to iterate every enabled plugin’s results.
🤖 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 `@test-all.mjs`:
- Around line 10989-11005: Update test-all.mjs lines 10989-11005 so the
regression test invokes production cmdRun or a shared production helper with two
plugin results and verifies only the requested plugin’s result is returned; do
not recreate the filtering logic in the test. Update test-all.mjs lines
11008-11020 to invoke Gmail ingest with dryRun: true and assert that
data/gmail-state.json remains unchanged, exercising the production path rather
than inspecting or reimplementing source behavior.
---
Outside diff comments:
In `@plugins.mjs`:
- Around line 154-158: Apply the requested plugin-ID filtering consistently
across all hook branches in the run command: ensure results returned by runHook
are restricted to entries whose id matches the requested id, including export
and notify, before those branches consume them. Reuse the existing filtering
logic near the ingest/search handling rather than leaving those paths to iterate
every enabled plugin’s results.
🪄 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: 81ce953d-c259-4231-918a-7c4894bacdd6
📒 Files selected for processing (3)
plugins.mjsplugins/gmail/index.mjstest-all.mjs
Comment on lines
+10989
to
+11005
| // #2354 — cmdRun must only collect results from the requested plugin id. | ||
| // If a second plugin of the same hook kind is enabled it must NOT bleed in. | ||
| { | ||
| const { runHook: rh } = await import(pathToFileURL(join(ROOT, 'plugins/_engine.mjs')).href); | ||
| // Simulate runHook returning two plugins' results (as the real function would when two are enabled). | ||
| const fakeResults = [ | ||
| { id: 'linkedin-alerts', ok: true, result: [{ title: 'Correct', url: 'https://a.test/1' }] }, | ||
| { id: 'gmail', ok: true, result: [{ title: 'Wrong', url: 'https://b.test/2' }] }, | ||
| ]; | ||
| // The fix: filter(r => r.id === id && ...) — only linkedin-alerts' result must survive. | ||
| const id = 'linkedin-alerts'; | ||
| const found = fakeResults.filter(r => r.id === id && r.ok && Array.isArray(r.result)).flatMap(r => r.result); | ||
| if (found.length === 1 && found[0].url === 'https://a.test/1') { | ||
| pass('cmdRun result isolation: only the requested plugin id\'s results are collected (#2354)'); | ||
| } else { | ||
| fail(`cmdRun result isolation broken: got ${JSON.stringify(found)}`); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make both regression tests execute the production behavior.
Both tests reimplement or inspect source text instead of exercising the affected code paths.
test-all.mjs#L10989-L11005: callcmdRunor a shared production helper with two plugin results.test-all.mjs#L11008-L11020: invoke GmailingestwithdryRun: trueand assert thatdata/gmail-state.jsonis unchanged.
📍 Affects 1 file
test-all.mjs#L10989-L11005(this comment)test-all.mjs#L11008-L11020
🤖 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 `@test-all.mjs` around lines 10989 - 11005, Update test-all.mjs lines
10989-11005 so the regression test invokes production cmdRun or a shared
production helper with two plugin results and verifies only the requested
plugin’s result is returned; do not recreate the filtering logic in the test.
Update test-all.mjs lines 11008-11020 to invoke Gmail ingest with dryRun: true
and assert that data/gmail-state.json remains unchanged, exercising the
production path rather than inspecting or reimplementing source behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes two bugs in the plugin engine reported in #2354:
Misattribution (
cmdRun):runHook()runs every enabled plugin of the requested hook kind and returns[{id, ok, result}]for all of them.cmdRunnever filtered by the requested pluginidbefore flattening results, so runningnode plugins.mjs run linkedin-alerts --dry-runwithgmailalso enabled silently merged gmail's results in and reported them all under thelinkedin-alertsname.Dry-run not dry (
gmail):plugins/gmail/index.mjscalledsaveProcessedIds()unconditionally — ignoringctx.dryRun. A--dry-runpermanently marked Gmail messages as processed even though nothing was written to the pipeline. Any message seen during a dry-run was hidden from all future real runs.plugins/notion/index.mjsalready handles this correctly withif (ctx?.dryRun) { ... continue; }.Related issue
Fixes #2354
Type of change
Checklist
node test-all.mjsand all tests pass (2783 passed, 1 pre-existing failure onmainunchanged)Summary by CodeRabbit
Bug Fixes
Tests