fix(review): support focused large frozen scopes - #258
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds native START focus forwarding, canonical compressed manifests for oversized frozen scopes, a bounded scope-reading tool, and candidate-root validation during native FINALIZE. ChangesNative review routing and candidate context
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant gentle_review
participant CandidateContext
participant ReviewActor
participant gentle_review_scope
participant NativeReview
Caller->>gentle_review: START with optional focus
gentle_review->>gentle_review: validate focus and request fields
gentle_review->>CandidateContext: build readable or compact frozen scope
CandidateContext-->>gentle_review: context and manifest digest
gentle_review->>ReviewActor: dispatch review with validated focus
ReviewActor->>gentle_review_scope: request manifest page
gentle_review_scope-->>ReviewActor: validated frozen scope page
Caller->>gentle_review: FINALIZE with candidate binding
gentle_review->>NativeReview: resolve status and mutate from candidate root
NativeReview-->>gentle_review: FINALIZE result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/review-candidate-view.ts`:
- Around line 1005-1093: Add a concise comment near canonicalStringMap or
hasCanonicalRecordOrder documenting that plain-object enumeration forces
integer-like keys into numeric order, which the canonical round-trip validation
relies on for numeric-looking paths. Do not alter the existing canonicalization
or validation behavior.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 9784b9b6-cd04-4b13-be54-ec90b4f54c39
📒 Files selected for processing (4)
extensions/gentle-ai.tslib/review-candidate-view.tstests/review-candidate-view.test.tstests/review-controller-native-routing.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/review-candidate-view.ts (1)
1235-1296: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftRepeated full manifest decode on every paginated read.
export function readCandidateContextManifestPage(encoded: string, sha256: string, cursor = 0): CandidateContextPage {
if (!Number.isSafeInteger(cursor) || cursor < 0) throw new CandidateViewError("candidate context manifest cursor is invalid", "candidate-context-cursor-invalid");
const decoded = decodeCandidateContextManifest(encoded, sha256); Each call toreadCandidateContextManifestPagere-runsdecodeCandidateContextManifest, which base64url-decodes,gunzipSyncs, hashes, checks UTF-8 round-trip, re-gzipSyncs the bytes for the canonical-transport check,JSON.parses, and re-validates the whole manifest structure. It also rebuilds the fullentriesarray fromscopeByModeregardless ofcursor.An agent enumerating a large manifest calls this once per page (bounded to 128 entries each). Every call repeats this full decode-and-recompress cost against the same input, so total work scales with
pages × manifest sizeinstead ofpage size. ThegzipSyncrecompression is the most expensive part of this and is pure overhead on repeat calls with an already-verified manifest.Consider caching the decoded, validated manifest (and its
entriesarray) keyed bysha256for the duration of a pagination sequence, so only the first page pays the full decode cost and subsequent pages slice from the cached result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/review-candidate-view.ts` around lines 1235 - 1296, Update readCandidateContextManifestPage and the surrounding manifest-pagination flow to reuse a decoded, validated manifest and its flattened entries across pages keyed by sha256, instead of calling decodeCandidateContextManifest and rebuilding entries on every request. Ensure the cache is bounded or scoped to the pagination sequence, preserves existing validation and cursor behavior, and avoids returning cached data for a different encoded manifest sharing the key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/review-controller-workspace-root.test.ts`:
- Around line 205-214: The candidateFinalizeTargetStatus function must build the
finalize fixture from the candidate associated with lineageId rather than
creating a new CandidateViewRegistry entry from request.cwd. Resolve the
candidate through the existing lineage-aware finalize lookup, such as
candidateViews.resolveForFinalize(lineageId), and fail closed when no projection
is available; preserve the targetStatusFixture inputs for a successfully
resolved candidate.
---
Outside diff comments:
In `@lib/review-candidate-view.ts`:
- Around line 1235-1296: Update readCandidateContextManifestPage and the
surrounding manifest-pagination flow to reuse a decoded, validated manifest and
its flattened entries across pages keyed by sha256, instead of calling
decodeCandidateContextManifest and rebuilding entries on every request. Ensure
the cache is bounded or scoped to the pagination sequence, preserves existing
validation and cursor behavior, and avoids returning cached data for a different
encoded manifest sharing the key.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e94ca2d5-cecf-4266-824b-52226b1f99dd
📒 Files selected for processing (12)
README.mdassets/agents/review-readability.mdassets/agents/review-reliability.mdassets/agents/review-resilience.mdassets/agents/review-risk.mdextensions/gentle-ai.tslib/review-candidate-view.tstests/review-actor-tool-deny.test.tstests/review-candidate-view.test.tstests/review-controller-native-routing.test.tstests/review-controller-workspace-root.test.tstests/runtime-harness.mjs
1 participant
Closes #242
Summary
risk,resilience,readability, andreliabilitywhile preserving the native default when omitted.gentle_review_scopeactor tool so every compact path, mode, deletion, and gitlink is consumable without shell access or ambient/full-tree fallback.mainand PR fix(review): harden blocking failure paths #257 through normal merge history without rewriting the existing PR branch.Changes
extensions/gentle-ai.tslib/review-candidate-view.tsassets/agents/review-*.mdtests/review-*.test.tstests/runtime-harness.mjsREADME.mdTest Plan
pnpm testunder an isolated home: 997 passed, 1 platform skip; runtime harness passed.node scripts/verify-package-files.mjs: 129 package resources passed, including 64 exact v2.2.2 artifacts.git diff --check.Review Notes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation