fix(session): relax successful same-target loop gate - #416
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR extends loop diagnostics with ChangesLoop diagnostics + gate-selection wiring
Sequence DiagramsequenceDiagram
participant Client as Client/Prompt
participant Gate as applyLoopGate
participant Diag as SessionDiagnostics
participant Proc as Processor
participant Export as Snapshot Export
Client->>Gate: applyLoopGate(args, loopCtx)
Gate->>Diag: deriveParentLoopState(..., currentMutationEpoch)
Gate->>Diag: queryGateAction(outcome, inputKey, targetKey)
alt outcome === "success"
Diag-->>Gate: evaluate candidates for inputKey (bucketed by input+outputHash)
else non-success
Diag-->>Gate: evaluate targetKey then inputKey
end
alt decision is "block"
Gate->>Proc: recordSyntheticBlock({attemptedInput: compactDiagnosticValue(args), ...})
Proc->>Diag: persist diagnostics.loop.attemptedInput and loop.mutationEpoch
else decision is "stop"
Gate->>Proc: recordSyntheticStop({attemptedInput: compactDiagnosticValue(args), ...})
Proc->>Diag: persist diagnostics.loop.attemptedInput and loop.mutationEpoch
end
Note over Export,Diag: sanitizeSnapshot calls sanitizeDiagnostics to redact attemptedInput
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the loop gating mechanism to allow successful repeats of the same target, such as reading different parts of a file, while maintaining hard-blocking for exact-input repeats. It also introduces an attemptedInput field across diagnostic interfaces and session exports to improve debugging. Feedback was provided regarding a potential security risk where the new attemptedInput field in session exports might leak sensitive data because it is not currently processed by the sanitizeSnapshot function.
![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: 2
🤖 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/export.ts`:
- Around line 145-146: Update Export.sanitizeSnapshot() to redact or remove
diagnostics.loop.last.attemptedInput (so raw tool args can't leak) by
normalizing that key to a safe placeholder (e.g., "[REDACTED]") or omitting it;
perform this as part of a single sanitizer pass that runs over both state.error
and loop/metadata fields (including diagnostics.loop.* and any top-level
metadata) rather than only runtime_context/session—modify the
Export.sanitizeSnapshot() implementation to explicitly locate
diagnostics.loop.last and sanitize attemptedInput and to apply the same
redaction logic to state.error so both sources are covered in one pass.
In `@packages/opencode/test/session/loop-gate.test.ts`:
- Around line 200-224: The test currently only asserts the gate decision but not
that the successful records actually map to the same target-signature bucket;
update the test (around the use of SessionDiagnostics.deriveParentLoopState and
the three successfulToolCallRecord entries) to assert that all three success
records resolve to the same target signature key (e.g., compare the result of
the same target-signature helper you use in production or recompute via
targetHashForInput/inputHashFor) and/or assert that the state's grouping or the
computed query hash equals targetSigKey/targetHash—specifically verify that each
successfulToolCallRecord produces the same bucket key (targetSigKey) before
calling SessionDiagnostics.queryGateAction and keep the existing
expect(decision.action).toBe("observe").
🪄 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: CHILL
Plan: Pro Plus
Run ID: e27b8f04-a3f3-494c-b646-6ec505861696
📒 Files selected for processing (7)
packages/opencode/src/session/diagnostics.tspackages/opencode/src/session/export.tspackages/opencode/src/session/processor.tspackages/opencode/src/session/prompt.tspackages/opencode/test/session/export.test.tspackages/opencode/test/session/loop-gate.test.tspackages/opencode/test/session/prompt-effect.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/processor.ts`:
- Around line 219-240: The mutationEpoch is being reset to 0 for each assistant
message, causing epochs to be compared only per-message rather than per-parent;
move the mutationEpoch counter out of the per-message scope so it is tracked per
parentID (parent-scoped) and incremented when encountering "patch" parts across
all messages for that parent; update both places where mutationEpoch is
declared/used (the block around message.parts handling starting at the shown
diff and the similar block at lines ~328-360) to read/increment the same
parent-scoped mutationEpoch and ensure loop.mutationEpoch falls back to that
parent counter when undefined so deriveParentLoopState() sees consistent epochs
across all records for the same parentID.
🪄 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: CHILL
Plan: Pro Plus
Run ID: d71f2619-7c6a-4d8e-85d6-d93a2c944019
📒 Files selected for processing (6)
packages/opencode/src/session/diagnostics.tspackages/opencode/src/session/processor.tspackages/opencode/src/session/prompt.tspackages/opencode/test/session/diagnostics.test.tspackages/opencode/test/session/loop-gate.test.tspackages/opencode/test/session/prompt-effect.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/opencode/test/session/diagnostics.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/test/session/loop-gate.test.ts
11 tasks
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.
Summary
Why
PR #406 made repeated successful tool calls observable, but the same-target hard gate was too broad. A normal debugging session can read different ranges of the same file, which repeats the target while still making progress. The exported quick-river session showed this exact shape: successful read calls against the same file with different offsets were blocked and then stopped.
The follow-up review also identified a second false-positive shape: repeating the same successful input after edits or after output changes can be valid verification. This update keeps hard stops for true no-progress success loops, but requires exact input plus same output within the same parent-turn mutation epoch before escalating.
Related Issue
No issue. Follow-up to the merged #406 behavior after a false-positive exported-session diagnosis.
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
Please focus on loop-gate policy semantics:
Compatibility Notes
Risk Notes
Behavior change in the session loop gate. This intentionally reduces hard stops for successful same-target repeats and for repeated successful inputs when output or mutation state changed, while preserving the stricter failure loop gate and same-output success gate. No migrations, dependencies, generated files, permissions, credentials, or visible UI changes.
How To Verify
Screenshots or Recordings
Not applicable. No visible UI changes.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Improvements
Tests