feat: add Mobius Loop hooks (loop-detector, definition-gates)#1188
feat: add Mobius Loop hooks (loop-detector, definition-gates)#1188agno01 wants to merge 6 commits intocode-yeongyu:devfrom
Conversation
|
All contributors have signed the CLA. Thank you! ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
|
There was a problem hiding this comment.
2 issues found across 7 files
Confidence score: 3/5
- Definition-of-Ready checks can fail incorrectly because the ambiguity regex in
src/hooks/definition-gates/index.tsmatches “or” inside other words, leading to frequent false positives. - The
dependencies_mappedcriterion is hardcoded true insrc/hooks/definition-gates/index.ts, so missing dependencies won’t block readiness as intended. - Given these medium-severity, user-facing validation issues, there’s some risk of incorrect gate outcomes if merged as-is.
- Pay close attention to
src/hooks/definition-gates/index.ts- ambiguity regex false positives and the hardcoded dependencies gate.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/hooks/definition-gates/index.ts">
<violation number="1" location="src/hooks/definition-gates/index.ts:143">
P2: Ambiguity regex matches `or` inside other words, causing frequent false positives and DoR failures.</violation>
<violation number="2" location="src/hooks/definition-gates/index.ts:149">
P2: `dependencies_mapped` criterion is always marked as met because `dependenciesMapped` is hardcoded to true, so the Definition-of-Ready check can never fail for missing dependencies.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Use word boundaries for ambiguity detection to avoid false positives (e.g., 'factor' no longer triggers on 'or') - Detect dependency mentions in prompt instead of hardcoding true - Addresses cubic-dev-ai review comments on PR code-yeongyu#1188
There was a problem hiding this comment.
1 issue found across 7 files
Confidence score: 4/5
- Medium-risk gap:
src/hooks/loop-detector/index.test.tsnever constructs a loop inoldHistory, so the “recent-history-window” case can falsely pass and fails to prove older loops are ignored. - Overall functional impact seems modest because the flaw is limited to a single test case, so the production loop detector logic is unaffected.
- Pay close attention to
src/hooks/loop-detector/index.test.ts- ensure the test actually simulates a loop inoldHistoryto confirm the recent-history window behavior.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/hooks/loop-detector/index.test.ts">
<violation number="1" location="src/hooks/loop-detector/index.test.ts:97">
P2: The recent-history-window test doesn't create a loop in oldHistory, so it can't validate that older loops are ignored.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Create actual loop pattern (3 identical calls) in oldHistory to properly test that loops outside the recent history window are not detected. Uses varied tools in filler to avoid triggering alternating pattern. Addresses cubic-dev-ai review on PR code-yeongyu#1188
code-yeongyu
left a comment
There was a problem hiding this comment.
Great feature addition for preventing infinite loops and enforcing quality gates!
Review notes:
The Mobius Loop hooks concept is solid:
- loop-detector: Good pattern detection for repeated calls, error loops, and ping-pong patterns
- definition-gates: DoR/DoD enforcement helps maintain task quality
cubic identified issues (now resolved in latest commit):
- The ambiguity regex false positives were fixed
- The hardcoded
dependencies_mappedwas addressed - Test coverage for recent-history-window was improved
Questions:
- Are these hooks enabled by default, or do users need to opt-in via config?
- What's the default threshold for loop detection (number of repeated calls before triggering)?
- Is there a way to customize the DoR/DoD checklists per project?
Overall this looks good. Once the questions above are clarified, I'm happy to approve.
code-yeongyu
left a comment
There was a problem hiding this comment.
Approving this feature addition. The Mobius Loop hooks (loop-detector, definition-gates) are well-implemented.
Summary:
- loop-detector prevents infinite tool loops (repeated calls, error loops, ping-pong patterns)
- definition-gates enforces DoR before delegation and DoD reminders on completion
- cubic review passed on latest commit (5/5 confidence)
- CLA signed
- 18 tests passing
The hooks address real problems (#1177, #1178) with token-wasting infinite loops.
Note for docs: Consider adding a brief section in configurations.md about these new hooks once merged.
|
Thanks for the review and approval! Answering your questions: 1. Are these hooks enabled by default? Yes, both hooks are enabled by default. The Users can opt-out via config: {
"disabled_hooks": ["loop-detector", "definition-gates"]
}2. What's the default threshold for loop detection? From
3. Is there a way to customize the DoR/DoD checklists per project? Not currently. The checklists are hardcoded in Let me know if you'd like any changes before merge! |
|
@sisyphus-dev-ai please review this PR |
Code Review: Mobius Loop Hooks (loop-detector, definition-gates)Reviewer: @sisyphus-dev-ai SummaryThis PR adds two new hooks to prevent infinite loops and enforce quality gates:
The feature addresses real problems documented in #1177 and #1178. Review FindingsCritical Issue: TypeScript Import ErrorsThe new hooks have TypeScript compilation errors that need to be fixed: Root Cause: The hooks incorrectly import Fix Required: Follow the pattern used by other hooks (e.g., // Instead of:
import type { PluginInput, PreToolUseInput, PreToolUseOutput } from "@opencode-ai/plugin"
// Use:
import type { PluginInput } from "@opencode-ai/plugin"
// And define inline types for the hook signatures:
"tool.execute.before": async (
input: { tool: string; sessionID: string; callID: string },
output: { args: Record<string, unknown>; message?: string }
): Promise<void> => {Or, for loop-detector which needs "tool.execute.before": async (
input: { tool: { name: string; input?: Record<string, unknown> }; sessionID?: string },
output: { output?: string }
): Promise<typeof output> => {cubic-identified Issues (Verified Resolved)
Tests
Integration
Minor Observations
VerdictRequest Changes - The TypeScript import errors must be fixed before merge. The tests pass in isolation but the project won't compile. Once the import errors are fixed, this is a solid feature addition with well-thought-out detection logic and proper test coverage. Quick Fix ReferenceFor loop-detector/index.ts: // Line 1 - Remove PreToolUseInput, PreToolUseOutput from import
import type { PluginInput } from "@opencode-ai/plugin"
// Lines 169-175 - Update interface
export interface LoopDetectorHook {
"tool.execute.before": (
input: { tool: { name: string; input?: unknown }; sessionID?: string },
output: { output?: string }
) => Promise<{ output?: string }>
event: (input: { event: { type: string; properties?: unknown } }) => Promise<void>
}
// Lines 179-182 - Update implementation signature accordinglyFor definition-gates/index.ts: // Line 1 - Remove PreToolUseInput, PreToolUseOutput from import
import type { PluginInput } from "@opencode-ai/plugin"
// Lines 157-162 - Update interface
export interface DefinitionGatesHook {
"tool.execute.before": (
input: { tool: { name: string; input?: unknown }; sessionID?: string },
output: { output?: string }
) => Promise<{ output?: string }>
}
// Lines 166-169 - Update implementation signature accordingly |
|
@agno01 Thanks for this feature! However, @sisyphus-dev-ai identified a critical issue: TypeScript Import Errors Fix: Follow the pattern used by other hooks (comment-checker, prometheus-md-only) which define inline types instead of importing non-existent types from @opencode-ai/plugin. Please update the imports and type definitions as suggested in sisyphus-dev-ai's review above, then we can merge! |
TypeScript Import Errors Fixed@agno01 I've implemented the fix for the TypeScript import errors. Since this PR is from a fork, I've pushed the fix to our repo: Commit: Changes Made1. Removed invalid imports: // Before (line 1 in both files):
import type { PluginInput, PreToolUseInput, PreToolUseOutput } from "@opencode-ai/plugin"
// After:
import type { PluginInput } from "@opencode-ai/plugin"2. Updated type definitions to match the actual hook API: The hooks were using
// Before:
export interface LoopDetectorHook {
"tool.execute.before": (
input: PreToolUseInput,
output: PreToolUseOutput
) => Promise<PreToolUseOutput>
}
// After:
export interface LoopDetectorHook {
"tool.execute.before": (
input: { tool: string; sessionID: string; callID: string },
output: { args: Record<string, unknown>; output?: string }
) => Promise<{ args: Record<string, unknown>; output?: string }>
}3. Updated implementation to use correct API: // Before:
const toolName = input.tool.name
const toolArgs = (input.tool.input ?? {}) as Record<string, unknown>
// After:
const toolName = input.tool
const toolArgs = (output.args ?? {}) as Record<string, unknown>Verification
@agno01 You have two options:
Once the fix is applied, this PR should be ready to merge! |
|
@sisyphus-dev-ai cherry-picked & merged, pls recheck 👍 |
|
@agno01 Thanks for this valuable contribution! The feature is approved and addresses real infinite-loop bugs. However, PR is showing CONFLICTING because dev has advanced 171 commits ahead with new hooks (stop-continuation-guard, unstable-agent-babysitter, tasks-todowrite-disabler). Next Steps:
Once rebased and CI passes, we can merge! Feel free to ask if you need help with the rebase. |
cdabb90 to
d319557
Compare
- Use word boundaries for ambiguity detection to avoid false positives (e.g., 'factor' no longer triggers on 'or') - Detect dependency mentions in prompt instead of hardcoding true - Addresses cubic-dev-ai review comments on PR code-yeongyu#1188
Create actual loop pattern (3 identical calls) in oldHistory to properly test that loops outside the recent history window are not detected. Uses varied tools in filler to avoid triggering alternating pattern. Addresses cubic-dev-ai review on PR code-yeongyu#1188
|
@code-yeongyu conflicts resolved - ready to merge! 👍 |
- Use word boundaries for ambiguity detection to avoid false positives (e.g., 'factor' no longer triggers on 'or') - Detect dependency mentions in prompt instead of hardcoding true - Addresses cubic-dev-ai review comments on PR code-yeongyu#1188
Create actual loop pattern (3 identical calls) in oldHistory to properly test that loops outside the recent history window are not detected. Uses varied tools in filler to avoid triggering alternating pattern. Addresses cubic-dev-ai review on PR code-yeongyu#1188
d319557 to
9466ab4
Compare
Issue AnalysisThis PR directly addresses #1349 and may help several other open issues related to infinite loops. Fixes #1349[Bug]: Sisyphus-Junior stuck in infinite loop endlessly refactoring Python type hints The
The hook issues warnings with actionable recommendations ("Try a DIFFERENT approach") and escalates after 3 warnings. May Help#735 - Tool call JSON parse error causes infinite retry loopRelevance: The #78 - Agents stuck on eternal referencing of patternsRelevance: The #806 - Momus agent enters infinite loop on WindowsRelevance: General loop detection applies to any agent behavior. If Momus is cycling through the same tool calls or errors, the hook will detect and warn. Does NOT Address
|
|
@code-yeongyu I guess it's ready? :) |
Implements loop detection and prevention based on Mobius Loop methodology. Detects: - Repeated identical tool calls (threshold: 3) - Error loop patterns (same error repeated) - Alternating tool patterns (ping-pong loops) Addresses issues code-yeongyu#1177 and code-yeongyu#1178 where users experienced infinite loops draining tokens. When a loop is detected, injects a warning message prompting the agent to stop, analyze the failure, and try a different approach.
Implements Definition of Ready and Definition of Done gates based on Open Practice Library methodology. Definition of Ready checks before task delegation: - Goal is atomic (one thing) - Files identified - Test criteria defined - Dependencies mapped - No ambiguity Definition of Done reminder when marking tasks complete: - Tests pass - Types pass (lsp_diagnostics clean) - No forbidden patterns - Follows codebase style Helps ensure quality at task boundaries, reducing rework.
- Export new hooks from src/hooks/index.ts - Add hook names to HookNameSchema in config - Initialize and wire hooks in main plugin entry point - Add to tool.execute.before and event lifecycle methods
- Use word boundaries for ambiguity detection to avoid false positives (e.g., 'factor' no longer triggers on 'or') - Detect dependency mentions in prompt instead of hardcoding true - Addresses cubic-dev-ai review comments on PR code-yeongyu#1188
Create actual loop pattern (3 identical calls) in oldHistory to properly test that loops outside the recent history window are not detected. Uses varied tools in filler to avoid triggering alternating pattern. Addresses cubic-dev-ai review on PR code-yeongyu#1188
- Remove non-existent PreToolUseInput/PreToolUseOutput imports from @opencode-ai/plugin - Use inline type definitions matching the actual hook API - Change input.tool.name to input.tool (tool name is a string, not object) - Change input.tool.input to output.args (tool args are in output, not input) - Follows pattern established by comment-checker and prometheus-md-only hooks
9466ab4 to
50dee07
Compare
Summary
Changes
src/hooks/loop-detector/- Detects repeated calls, error loops, ping-pong patternssrc/hooks/definition-gates/- DoR checks before delegation, DoD reminders on completionsrc/hooks/index.ts- Export new hookssrc/config/schema.ts- Add hook names to HookNameSchemasrc/index.ts- Initialize and wire hooks to lifecycle methodsTesting
Related Issues
Closes #1177
Closes #1178
Summary by cubic
Adds two Mobius Loop–inspired hooks: loop-detector to prevent infinite tool loops and definition-gates to enforce Definition of Ready/Done. This reduces token waste and improves task quality during delegation and completion.
New Features
Bug Fixes
Written for commit 50dee07. Summary will update on new commits.