fix: address post-merge code review findings from PR #556

docs/docs/data-engineering/agent-modes.md

@@ -163,7 +163,7 @@ Refinements are capped at **5 revisions per session** to avoid endless loops. Af
 
 ### Example conversation
 
-```
+```text
 You: Plan a migration of our raw_events table from a view to an incremental model
 
 Plan: Here's my proposed approach:

packages/drivers/src/mongodb.ts

@@ -187,10 +187,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
     if (val instanceof Date) {
       return val.toISOString()
     }
-    // Arrays and plain objects — JSON-serialize for tabular display
-    if (Array.isArray(val) || typeof (val as any).toJSON !== "function") {
-      return JSON.stringify(val)
-    }
+    // Arrays, plain objects, and remaining BSON types — JSON-serialize for tabular display
     return JSON.stringify(val)
   }
 
@@ -333,20 +330,37 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
           if (!parsed.pipeline || !Array.isArray(parsed.pipeline)) {
             throw new Error("aggregate requires a 'pipeline' array")
           }
-          // Cap or append $limit to prevent OOM. Skip for $out/$merge write pipelines.
+          // Block dangerous stages/operators:
+          // - $out/$merge: write operations (top-level stage keys)
+          // - $function/$accumulator: arbitrary JS execution (can be nested in expressions)
           const pipeline = [...parsed.pipeline]
-          const hasWrite = pipeline.some((stage) => "$out" in stage || "$merge" in stage)
-          if (!hasWrite) {
-            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 }
-              }
-            } else {
-              pipeline.push({ $limit: effectiveLimit + 1 })
+          const blockedWriteStages = ["$out", "$merge"]
+          const hasBlockedWrite = pipeline.some((stage) =>
+            blockedWriteStages.some((s) => s in stage),
+          )
+          if (hasBlockedWrite) {
+            throw new Error(
+              `Pipeline contains a blocked write stage (${blockedWriteStages.join(", ")}). Write operations are not allowed.`,
+            )
+          }
+          // $function/$accumulator can appear nested inside $project, $addFields, $group, etc.
+          // Stringify and scan to catch them at any depth.
+          const pipelineStr = JSON.stringify(pipeline)
+          if (pipelineStr.includes('"$function"') || pipelineStr.includes('"$accumulator"')) {
+            throw new Error(
+              "Pipeline contains a blocked operator ($function, $accumulator). Executing arbitrary JavaScript is not allowed.",
+            )
+          }
+          // Cap or append $limit to prevent OOM (write stages already blocked above).
+          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 }
             }
+          } else {
+            pipeline.push({ $limit: effectiveLimit + 1 })
           }
 
           const docs = await coll.aggregate(pipeline).toArray()

packages/opencode/src/altimate/tools/mcp-discover.ts

@@ -23,9 +23,14 @@ async function getPersistedMcpNames(): Promise<Set<string>> {
 /** Redact server details for safe display — show type and name only, not commands/URLs */
 function safeDetail(server: { type: string } & Record<string, any>): string {
   if (server.type === "remote") return "(remote)"
-  if (server.type === "local" && Array.isArray(server.command) && server.command.length > 0) {
+  if (server.type === "local") {
     // Show only the executable name, not args (which may contain credentials)
-    return `(local: ${server.command[0]})`
+    if (Array.isArray(server.command) && server.command.length > 0) {
+      return `(local: ${server.command[0]})`
+    }
+    if (typeof server.command === "string" && server.command.trim()) {
+      return `(local: ${server.command.trim().split(/\s+/)[0]})`
+    }
   }
   return `(${server.type})`
 }

packages/opencode/src/altimate/tools/post-connect-suggestions.ts

@@ -40,14 +40,19 @@ export namespace PostConnectSuggestions {
       )
     }
 
-    suggestions.push(
-      "Run SQL queries against your " +
-        ctx.warehouseType +
-        " warehouse using sql_execute",
-    )
-    suggestions.push(
-      "Analyze SQL quality and find potential issues with sql_analyze",
-    )
+    // MongoDB uses MQL, not SQL — skip SQL-specific suggestions
+    const nonSqlWarehouses = ["mongodb", "mongo"]
+    const isMongo = nonSqlWarehouses.includes(ctx.warehouseType.toLowerCase())
+    if (!isMongo) {
+      suggestions.push(
+        "Run SQL queries against your " +
+          ctx.warehouseType +
+          " warehouse using sql_execute",
+      )
+      suggestions.push(
+        "Analyze SQL quality and find potential issues with sql_analyze",
+      )
+    }
 
     if (ctx.dbtDetected) {
       suggestions.push(

packages/opencode/src/session/prompt.ts

@@ -323,6 +323,7 @@ export namespace SessionPrompt {
     // altimate_change start — plan refinement tracking
     let planRevisionCount = 0
     let planHasWritten = false
+    let planLastUserMsgId: string | undefined
     // altimate_change end
     let emergencySessionEndFired = false
     // altimate_change start — quality signal, tool chain, error fingerprint tracking
@@ -637,10 +638,12 @@ export namespace SessionPrompt {
           const planPath = Session.plan(session)
           planHasWritten = await Filesystem.exists(planPath)
         }
-        // If plan was already written and user sent a new message, this is a refinement
-        if (planHasWritten && step > 1) {
-          // Detect approval phrases in the last user message text
-          const lastUserMsg = msgs.findLast((m) => m.info.role === "user")
+        // If plan was already written and user sent a new message, this is a refinement.
+        // Only count once per user message (not on internal loop iterations).
+        const lastUserMsg = msgs.findLast((m) => m.info.role === "user")
+        const currentUserMsgId = lastUserMsg?.info.id
+        if (planHasWritten && step > 1 && currentUserMsgId && currentUserMsgId !== planLastUserMsgId) {
+          planLastUserMsgId = currentUserMsgId
           const userText = lastUserMsg?.parts
             .filter((p): p is MessageV2.TextPart => p.type === "text" && !("synthetic" in p && p.synthetic))
             .map((p) => p.text.toLowerCase())
@@ -678,7 +681,7 @@ export namespace SessionPrompt {
             const refinementQualifiers = [" but ", " however ", " except ", " change ", " modify ", " update ", " instead ", " although ", " with the following", " with these"]
             const hasRefinementQualifier = refinementQualifiers.some((q) => userText.includes(q))
 
-            const rejectionPhrases = ["don't", "stop", "reject", "not good", "undo", "abort", "start over", "wrong"]
+            const rejectionPhrases = ["don't", "stop", "reject", "not good", "not approve", "not approved", "disapprove", "undo", "abort", "start over", "wrong"]
             // "no" as a standalone word to avoid matching "know", "notion", etc.
             const rejectionWords = ["no"]
             const approvalPhrases = ["looks good", "proceed", "approved", "approve", "lgtm", "go ahead", "ship it", "yes", "perfect"]
@@ -689,7 +692,12 @@ export namespace SessionPrompt {
               return regex.test(userText)
             })
             const isRejection = isRejectionPhrase || isRejectionWord
-            const isApproval = !isRejection && !hasRefinementQualifier && approvalPhrases.some((phrase) => userText.includes(phrase))
+            // Use word-boundary matching for approval phrases to avoid false positives
+            // e.g. "this doesn't look good" should NOT match "looks good"
+            const isApproval = !isRejection && !hasRefinementQualifier && approvalPhrases.some((phrase) => {
+              const regex = new RegExp(`\\b${phrase.replace(/\s+/g, "\\s+")}\\b`, "i")
+              return regex.test(userText)
+            })
             const action = isRejection ? "reject" : isApproval ? "approve" : "refine"
             Telemetry.track({
               type: "plan_revision",

packages/opencode/src/tool/plan.ts

@@ -39,7 +39,7 @@ export const PlanExitTool = Tool.define("plan_exit", {
     })
 
     const answer = answers[0]?.[0]
-    if (answer === "No") throw new Question.RejectedError()
+    if (answer !== "Yes") throw new Question.RejectedError()
 
     const model = await getLastModel(ctx.sessionID)
 
@@ -97,7 +97,7 @@ export const PlanEnterTool = Tool.define("plan_enter", {
 
     const answer = answers[0]?.[0]
 
-    if (answer === "No") throw new Question.RejectedError()
+    if (answer !== "Yes") throw new Question.RejectedError()
 
     const model = await getLastModel(ctx.sessionID)
 

packages/opencode/test/altimate/plan-refinement.test.ts

@@ -119,12 +119,10 @@ describe("plan_revision telemetry", () => {
     const promptTsPath = path.join(__dirname, "../../src/session/prompt.ts")
     const content = await fs.readFile(promptTsPath, "utf-8")
 
-    // Extract region around plan_revision telemetry — generous window
-    const startIdx = content.indexOf('type: "plan_revision"')
-    expect(startIdx).toBeGreaterThan(-1)
-    const regionStart = Math.max(0, startIdx - 200)
-    const regionEnd = Math.min(content.length, startIdx + 400)
-    const trackBlock = content.slice(regionStart, regionEnd)
+    // Find the Telemetry.track({ ... }) block containing plan_revision
+    const trackMatch = content.match(/Telemetry\.track\(\{[^}]*type:\s*"plan_revision"[^}]*\}\)/s)
+    expect(trackMatch).not.toBeNull()
+    const trackBlock = trackMatch![0]
     expect(trackBlock).toContain("timestamp:")
     expect(trackBlock).toContain("session_id:")
     expect(trackBlock).toContain("revision_number:")