fix: address post-merge code review findings from PR #556#578
fix: address post-merge code review findings from PR #556#578anandgupta42 merged 2 commits intomainfrom
Conversation
- fix(security): MongoDB aggregate now blocks nested `$function`/`$accumulator`
via `JSON.stringify` scan (not just top-level stage keys)
- fix(security): block `$out`/`$merge` write stages separately with clear error
- fix(consent): `PlanExitTool` rejects dismissed/undefined dialog (`!== "Yes"`)
- fix(telemetry): plan revision counter only increments once per user message,
not on internal loop iterations (`planLastUserMsgId` dedup)
- fix(telemetry): approval phrase detection uses word-boundary regex + negation
phrases to prevent false positives ("not approved", "disapprove")
- fix(ux): skip SQL suggestions (`sql_execute`, `sql_analyze`) for MongoDB
- fix(mcp): `safeDetail` handles string commands with `trim()` + `/\s+/` split
- fix(docs): add `text` language tag to markdown code fence
- fix(test): use regex instead of hardcoded slice for telemetry block matching
- chore: remove dead `hasWrite` code and BSON serialization dead branch
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughThis PR addresses post-merge code review findings by implementing stricter MongoDB pipeline safety checks for Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/drivers/src/mongodb.ts (1)
354-364: Minor inconsistency withfindtruncation detection.The
$limitcapping logic differs from thefindcommand (lines 304-305). Infind, the limit is always incremented by 1 (queryLimit + 1) to detect truncation. Here, when the user's limit is ≤effectiveLimit, it's kept as-is without the +1.Edge case: If a user specifies
{ $limit: 1000 }andeffectiveLimitis also 1000, truncation won't be detected even if more documents exist.This appears to be pre-existing behavior, but you may want to align with
findfor consistency:🔧 Optional fix to align with `find` behavior
const limitIdx = pipeline.findIndex((stage) => "$limit" in stage) if (limitIdx >= 0) { - // Cap user-specified $limit against effectiveLimit const userLimit = (pipeline[limitIdx] as any).$limit - if (typeof userLimit === "number" && userLimit > effectiveLimit) { - pipeline[limitIdx] = { $limit: effectiveLimit + 1 } + if (typeof userLimit === "number") { + // Cap user-specified $limit and add +1 for truncation detection + pipeline[limitIdx] = { $limit: Math.min(userLimit, effectiveLimit) + 1 } } } else { pipeline.push({ $limit: effectiveLimit + 1 }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/drivers/src/mongodb.ts` around lines 354 - 364, The $limit handling in the aggregation pipeline differs from the find path: when a $limit stage exists and the user's $limit is ≤ effectiveLimit you currently leave it unchanged, which prevents detection of truncation; update the logic in the block that examines pipeline, limitIdx and userLimit so that whenever a $limit is present you set it to (min(userLimit, effectiveLimit) + 1) (i.e., always add +1 for truncation detection), and when no $limit exists continue to push { $limit: effectiveLimit + 1 }; ensure you still only modify numeric userLimit values and preserve behavior for non-numeric/missing cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/session/prompt.ts`:
- Line 326: The tracker variable planLastUserMsgId is currently unset and later
updated from the last user message of any kind, causing synthetic/internal user
turns to be counted as new plan revisions; modify the logic that
initializes/updates planLastUserMsgId (the variable declared as
planLastUserMsgId) so that when the plan file is first detected you seed
planLastUserMsgId from the most recent user message that contains a
non-synthetic text part (i.e., skip synthetic user turns created in the
synthetic-user-turn creation block around Lines 571-589), and apply the same fix
to the analogous update logic around Lines 641-646 so only genuine user text
messages move currentUserMsgId/planLastUserMsgId and increment revision_number.
---
Nitpick comments:
In `@packages/drivers/src/mongodb.ts`:
- Around line 354-364: The $limit handling in the aggregation pipeline differs
from the find path: when a $limit stage exists and the user's $limit is ≤
effectiveLimit you currently leave it unchanged, which prevents detection of
truncation; update the logic in the block that examines pipeline, limitIdx and
userLimit so that whenever a $limit is present you set it to (min(userLimit,
effectiveLimit) + 1) (i.e., always add +1 for truncation detection), and when no
$limit exists continue to push { $limit: effectiveLimit + 1 }; ensure you still
only modify numeric userLimit values and preserve behavior for
non-numeric/missing cases.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 99efba67-0cff-4668-9824-1c35815f41f9
📒 Files selected for processing (7)
docs/docs/data-engineering/agent-modes.mdpackages/drivers/src/mongodb.tspackages/opencode/src/altimate/tools/mcp-discover.tspackages/opencode/src/altimate/tools/post-connect-suggestions.tspackages/opencode/src/session/prompt.tspackages/opencode/src/tool/plan.tspackages/opencode/test/altimate/plan-refinement.test.ts
| // altimate_change start — plan refinement tracking | ||
| let planRevisionCount = 0 | ||
| let planHasWritten = false | ||
| let planLastUserMsgId: string | undefined |
There was a problem hiding this comment.
This still counts non-feedback turns as plan revisions.
planLastUserMsgId starts unset, so once the plan file is first created the next internal loop will treat the same user prompt as a new revision. It also pulls currentUserMsgId from the last user message of any kind, so the synthetic user turns created on Lines 571-589 can increment the counter again. That makes revision_number telemetry drift and can hit the 5-revision cap early. Seed the tracker when the plan file first appears, and derive it from the last user message with a non-synthetic text part.
Also applies to: 641-646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/session/prompt.ts` at line 326, The tracker variable
planLastUserMsgId is currently unset and later updated from the last user
message of any kind, causing synthetic/internal user turns to be counted as new
plan revisions; modify the logic that initializes/updates planLastUserMsgId (the
variable declared as planLastUserMsgId) so that when the plan file is first
detected you seed planLastUserMsgId from the most recent user message that
contains a non-synthetic text part (i.e., skip synthetic user turns created in
the synthetic-user-turn creation block around Lines 571-589), and apply the same
fix to the analogous update logic around Lines 641-646 so only genuine user text
messages move currentUserMsgId/planLastUserMsgId and increment revision_number.
1 participant
What does this PR do?
Addresses 8 findings from Kulvir's post-merge multi-model code review on PR #556 (comment).
Security fixes:
$function/$accumulatorviaJSON.stringifyscan (not just top-level stage keys which could be bypassed)$out/$mergewrite stage blocking with clear error messagesBug fixes:
PlanExitToolnow rejects dismissed/undefined dialog (!== "Yes"instead of=== "No")sql_execute,sql_analyze) for MongoDB warehousessafeDetailhandles string commands withtrim()+/\s+/splitCode quality:
hasWritecode and BSON serialization dead branchtextlanguage tag to markdown code fenceType of change
Issue for this PR
Closes #577
How did you verify your code works?
Checklist
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements
Documentation