-
Notifications
You must be signed in to change notification settings - Fork 25
fix: prevent TTSR messagesTransformHook infinite correction loop #2203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1502,7 +1502,10 @@ const BUILTIN_TTSR_RULES = [ | |
| { | ||
| id: "shell-local-params", | ||
| description: "Use local var=\"$1\" pattern in shell functions", | ||
| pattern: "\\$[1-9](?!.*local\\s+\\w+=.*\\$[1-9])", | ||
| // Only match bare $N at the start of a line or after whitespace in what looks | ||
| // like a shell assignment/command context — avoids matching $1 inside prose, | ||
| // documentation, quoted examples, or tool output from file reads. | ||
| pattern: "^\\s+(?:echo|printf|return|if|\\[\\[).*\\$[1-9](?!.*local\\s+\\w+=)", | ||
| correction: "Use `local var=\"$1\"` pattern — never use positional parameters directly (SonarCloud S7679).", | ||
|
Comment on lines
+1505
to
1509
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regex misses unindented shell lines (Line 1508). 🔧 Suggested fix- pattern: "^\\s+(?:echo|printf|return|if|\\[\\[).*\\$[1-9](?!.*local\\s+\\w+=)",
+ pattern: "^\\s*(?:echo|printf|return|if|\\[\\[).*\\$[1-9](?!.*local\\s+\\w+=)",🤖 Prompt for AI Agents |
||
| severity: "warn", | ||
| systemPrompt: "Shell scripts: use `local var=\"$1\"` — never use $1 directly in function bodies.", | ||
|
|
@@ -1639,16 +1642,39 @@ async function systemTransformHook(_input, output) { | |
| * Extract text content from a Part array. | ||
| * Only extracts text from TextPart objects (type === "text"). | ||
| * @param {Array<object>} parts - Array of Part objects | ||
| * @param {object} [options] - Extraction options | ||
| * @param {boolean} [options.excludeToolOutput=false] - Skip tool-result/tool-invocation parts | ||
| * @returns {string} Concatenated text content | ||
| */ | ||
| function extractTextFromParts(parts) { | ||
| function extractTextFromParts(parts, options = {}) { | ||
| if (!Array.isArray(parts)) return ""; | ||
| return parts | ||
| .filter((p) => p && p.type === "text" && typeof p.text === "string") | ||
| .filter((p) => { | ||
| if (!p || typeof p.text !== "string") return false; | ||
| if (p.type !== "text") return false; | ||
| // When excludeToolOutput is set, skip parts that contain tool output. | ||
| // Tool results are embedded in assistant messages as text parts whose | ||
| // content is the serialized tool response. We detect these by checking | ||
| // for the tool-invocation/tool-result type or by the presence of | ||
| // toolCallId/toolInvocationId fields that OpenCode attaches. | ||
| if (options.excludeToolOutput) { | ||
| if (p.toolCallId || p.toolInvocationId) return false; | ||
| } | ||
| return true; | ||
| }) | ||
|
Comment on lines
+1652
to
+1664
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The filtering logic here is correct, but it could be made more concise and arguably more readable by combining the conditions into a single boolean expression. This avoids the nested .filter(
(p) =>
p &&
p.type === "text" &&
typeof p.text === "string" &&
!(options.excludeToolOutput && (p.toolCallId || p.toolInvocationId))
)
Comment on lines
1644
to
+1664
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # Find the file and examine the context around the function
head -1700 .agents/plugins/opencode-aidevops/index.mjs | tail -100Repository: marcusquinn/aidevops Length of output: 4340 🏁 Script executed: # Search for Part-related type definitions, schemas, or comments
rg -n "Part\s*=|Part\s*{|interface Part|type Part|class Part" -A 5Repository: marcusquinn/aidevops Length of output: 216 🏁 Script executed: # Look for tool-result and tool-invocation type usage
rg -n "tool-result|tool-invocation" -B 2 -A 2Repository: marcusquinn/aidevops Length of output: 1142 🏁 Script executed: # Search for where toolCallId and toolInvocationId are set or checked
rg -n "toolCallId|toolInvocationId" -B 2 -A 2Repository: marcusquinn/aidevops Length of output: 752 🏁 Script executed: # Look for OpenCode-specific documentation or comments about Part structure
rg -n "OpenCode|tool output|tool response|Part.*schema" -iRepository: marcusquinn/aidevops Length of output: 50377 🏁 Script executed: # Search for where parts are created or populated in the file
rg -n "type.*tool|toolCallId|toolInvocationId" .agents/plugins/opencode-aidevops/index.mjs -B 2 -A 2Repository: marcusquinn/aidevops Length of output: 444 🏁 Script executed: # Look for function calls that create or process parts with tool data
rg -n "parts\[|parts\.push|\.parts\s*=" .agents/plugins/opencode-aidevops/index.mjs -B 1 -A 1 | head -100Repository: marcusquinn/aidevops Length of output: 524 🏁 Script executed: # Search for any type definitions or interfaces for Part
fd "\.d\.ts|\.ts$|\.mjs$" .agents/plugins/opencode-aidevops/ -exec grep -l "Part" {} \;Repository: marcusquinn/aidevops Length of output: 262 🏁 Script executed: # Check OpenCode documentation or configuration comments about Part structure
rg -n "TextPart|ToolResultPart|type.*:.*string" .agents/plugins/opencode-aidevops/ -A 2 | head -80Repository: marcusquinn/aidevops Length of output: 1620 🏁 Script executed: # Search for how messagesTransformHook processes parts and messages
rg -n "messagesTransformHook|tool-result|tool output" .agents/plugins/opencode-aidevops/index.mjs -B 3 -A 3Repository: marcusquinn/aidevops Length of output: 3230 🏁 Script executed: # Look for where extractTextFromParts is called to understand the context
rg -n "extractTextFromParts" .agents/plugins/opencode-aidevops/index.mjs -B 2 -A 2Repository: marcusquinn/aidevops Length of output: 522 🏁 Script executed: # Search for any test or validation code that shows Part structure
rg -n "toolCallId|toolInvocationId|excludeToolOutput" .agents/plugins/opencode-aidevops/ -B 2 -A 2Repository: marcusquinn/aidevops Length of output: 2198 🏁 Script executed: # Check if there's OpenCode API documentation in the repo
find .agents -name "*opencode*.md" -type f | xargs grep -l "Part\|part\|tool-result" 2>/dev/null | head -5Repository: marcusquinn/aidevops Length of output: 160 🏁 Script executed: # Search for any real usage of parts with tool information in the entire codebase
rg -n "\.parts|message.*parts|msg\.parts" .agents/plugins/opencode-aidevops/ -B 2 -A 2 | head -100Repository: marcusquinn/aidevops Length of output: 1578 🏁 Script executed: # Look for type field being checked elsewhere in the code
rg -n "p\.type|\.type\s*===|\.type\s*===" .agents/plugins/opencode-aidevops/index.mjs -C 1Repository: marcusquinn/aidevops Length of output: 414 🏁 Script executed: # Search OpenCode documentation files for Part schema
find . -name "*.md" -path "*opencode*" -exec grep -l "Part\|TextPart\|ToolResult" {} \; 2>/dev/nullRepository: marcusquinn/aidevops Length of output: 110 🏁 Script executed: # Check the opencode-server.md for Part schema info
cat .agents/tools/ai-assistants/opencode-server.md | grep -A 30 -i "part\|textpart\|tool.*result" | head -60Repository: marcusquinn/aidevops Length of output: 1318 🏁 Script executed: # Check the t1305 issue file which might have plugin details
cat ./todo/t1305-opencode-streaming-hooks-issue.md | head -100Repository: marcusquinn/aidevops Length of output: 4282 🏁 Script executed: # Look at the actual plugin hook parameters to understand Part structure better
rg -n "experimental\.chat\.messages\.transform|plugin.*part" .agents/plugins/opencode-aidevops/index.mjs -B 5 -A 5Repository: marcusquinn/aidevops Length of output: 1009 Update comment to accurately reflect the code flow — type filtering happens before excludeToolOutput check. The function filters parts by 🤖 Prompt for AI Agents |
||
| .map((p) => p.text) | ||
| .join("\n"); | ||
| } | ||
|
|
||
| /** | ||
| * Cross-turn TTSR dedup state. | ||
| * Tracks which rules have already fired and on which message IDs, | ||
| * preventing the same rule from firing repeatedly on the same content | ||
| * across multiple LLM turns (which caused an infinite correction loop). | ||
| * @type {Map<string, Set<string>>} ruleId -> Set of messageIDs already corrected | ||
| */ | ||
| const _ttsrFiredState = new Map(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
This state should be scoped to a single conversation/session. Consider either:
|
||
|
|
||
| /** | ||
| * Hook: experimental.chat.messages.transform (t1304) | ||
| * | ||
|
|
@@ -1660,6 +1686,13 @@ function extractTextFromParts(parts) { | |
| * If violations are found, append a synthetic correction message to the | ||
| * message history so the model sees the feedback before generating. | ||
| * | ||
| * Bug fix: Three changes to prevent infinite correction loops: | ||
| * 1. Only scan assistant-authored text, excluding tool output (Read/Bash | ||
| * results contain code the assistant *read*, not code it *wrote*). | ||
| * 2. Cross-turn dedup — track which rules fired on which messages to | ||
| * prevent the same rule re-firing on the same content every turn. | ||
| * 3. Skip messages that are themselves synthetic TTSR corrections. | ||
| * | ||
| * @param {object} _input - {} | ||
| * @param {object} output - { messages: { info: Message, parts: Part[] }[] } (mutable) | ||
| */ | ||
|
|
@@ -1669,28 +1702,52 @@ async function messagesTransformHook(_input, output) { | |
| // Scan the last 3 assistant messages for violations | ||
| const scanWindow = 3; | ||
| const assistantMessages = output.messages | ||
| .filter((m) => m.info && m.info.role === "assistant") | ||
| .filter((m) => { | ||
| if (!m.info || m.info.role !== "assistant") return false; | ||
| // Skip synthetic TTSR correction messages that were injected previously | ||
| if (m.info.id && m.info.id.startsWith("ttsr-correction-")) return false; | ||
| return true; | ||
| }) | ||
| .slice(-scanWindow); | ||
|
|
||
| if (assistantMessages.length === 0) return; | ||
|
|
||
| const allViolations = []; | ||
|
|
||
| for (const msg of assistantMessages) { | ||
| const text = extractTextFromParts(msg.parts); | ||
| const msgId = msg.info?.id || ""; | ||
|
|
||
| // Extract only assistant-authored text, excluding tool output. | ||
| // Tool results (Read, Bash, etc.) contain code the assistant *read*, | ||
| // not code it *wrote* — scanning those causes false positives. | ||
| const text = extractTextFromParts(msg.parts, { excludeToolOutput: true }); | ||
| if (!text) continue; | ||
|
|
||
| const violations = scanForViolations(text); | ||
| for (const v of violations) { | ||
| // Deduplicate by rule id | ||
| if (!allViolations.some((av) => av.rule.id === v.rule.id)) { | ||
| allViolations.push(v); | ||
| const ruleId = v.rule.id; | ||
|
|
||
| // Cross-turn dedup: skip if this rule already fired on this message | ||
| const firedOn = _ttsrFiredState.get(ruleId); | ||
| if (firedOn && firedOn.has(msgId)) continue; | ||
|
|
||
| // Deduplicate by rule id within this scan | ||
| if (!allViolations.some((av) => av.rule.id === ruleId)) { | ||
| allViolations.push({ ...v, msgId }); | ||
| } | ||
|
Comment on lines
+1735
to
1737
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using While the number of violations is likely small in practice, using a You could introduce a For example: const allViolations = [];
const seenRuleIdsInScan = new Set(); // New
for (const msg of assistantMessages) {
// ...
for (const v of violations) {
const ruleId = v.rule.id;
// ... cross-turn dedup
// Deduplicate by rule id within this scan
if (!seenRuleIdsInScan.has(ruleId)) {
allViolations.push({ ...v, msgId });
seenRuleIdsInScan.add(ruleId);
}
}
} |
||
| } | ||
| } | ||
|
|
||
| if (allViolations.length === 0) return; | ||
|
|
||
| // Record that these rules fired on these messages (cross-turn dedup) | ||
| for (const v of allViolations) { | ||
| if (!_ttsrFiredState.has(v.rule.id)) { | ||
| _ttsrFiredState.set(v.rule.id, new Set()); | ||
| } | ||
| _ttsrFiredState.get(v.rule.id).add(v.msgId); | ||
| } | ||
|
Comment on lines
1717
to
+1749
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, let's search for messageID and sessionID patterns in the codebase
rg -n "messageID|sessionID|msg\.info\.id" --type mjs -B 2 -A 2Repository: marcusquinn/aidevops Length of output: 91 🏁 Script executed: # Look for where _ttsrFiredState is defined and initialized
rg -n "_ttsrFiredState" --type mjs -B 3 -A 3Repository: marcusquinn/aidevops Length of output: 91 🏁 Script executed: # Search for how info.id is set or created in messages
rg -n "info.*id|\.id\s*=" --type mjs | head -50Repository: marcusquinn/aidevops Length of output: 91 🏁 Script executed: # Look for session-related handling and function context
rg -n "sessionID|session\s*ID|session.*id" --type mjs -B 2 -A 2Repository: marcusquinn/aidevops Length of output: 91 🏁 Script executed: # Check the function containing the dedup code to see what parameters/scope it has
sed -n '1700,1760p' ".agents/plugins/opencode-aidevops/index.mjs"Repository: marcusquinn/aidevops Length of output: 2131 🏁 Script executed: # Search for the function definition and _ttsrFiredState initialization
rg -n "function|const.*=.*function|\(.*sessionID" ".agents/plugins/opencode-aidevops/index.mjs" -B 5 -A 2 | head -100Repository: marcusquinn/aidevops Length of output: 3737 🏁 Script executed: # Find _ttsrFiredState definition/initialization
rg -n "_ttsrFiredState\s*=" ".agents/plugins/opencode-aidevops/index.mjs" -B 5 -A 5Repository: marcusquinn/aidevops Length of output: 580 🏁 Script executed: # Check the file structure to understand context better
head -100 ".agents/plugins/opencode-aidevops/index.mjs"Repository: marcusquinn/aidevops Length of output: 2773 🏁 Script executed: # Look for where assistantMessages come from and the broader context
sed -n '1650,1720p' ".agents/plugins/opencode-aidevops/index.mjs"Repository: marcusquinn/aidevops Length of output: 2963 🏁 Script executed: # Look for how output is structured and if it contains sessionID
rg -n "output\.|session" ".agents/plugins/opencode-aidevops/index.mjs" | grep -E "(output\.|sessionID|session.*context)" | head -30Repository: marcusquinn/aidevops Length of output: 1813 🏁 Script executed: # Check if there's any session/conversation ID tracking in the hook context
sed -n '1680,1800p' ".agents/plugins/opencode-aidevops/index.mjs"Repository: marcusquinn/aidevops Length of output: 4313 🏁 Script executed: # Look for where messagesTransformHook is called and what parameters are available
rg -n "messagesTransformHook|experimental\.chat\.messages\.transform" ".agents/plugins/opencode-aidevops/index.mjs" -B 5 -A 5Repository: marcusquinn/aidevops Length of output: 1503 Include sessionID in the dedup key to prevent cross-session suppression. The global 🤖 Prompt for AI Agents |
||
|
|
||
| // Build correction context | ||
| const corrections = allViolations.map((v) => { | ||
| const severity = v.rule.severity === "error" ? "ERROR" : "WARNING"; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated regex is a great improvement for reducing false positives by being more context-aware. However, it seems to have become a bit too restrictive and might miss valid cases. For instance, it won't catch direct use of positional parameters in variable assignments like
my_var=$1, which this rule should probably flag.Also, the negative lookahead
(?!.*local\s+\w+=)might not reliably prevent matches on the declaration line itself (e.g.,local var="$1"), because the lookahead only checks characters after the$1.Consider adjusting the pattern to include assignment contexts and use a negative lookahead at the start of the line to more robustly exclude
localdeclarations.