Retain seek cursors with git-cas - #777
Conversation
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughSeek cursor state is migrated from Git refs to graph-scoped git-cas stores. The new adapter supports immutable pages, active-cursor expiry, named cursor retention, listing, validation, and sweeping. CLI commands and tests now use the cursor-store abstraction. ChangesSeek cursor storage migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
Release Preflight
If this PR is from a |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@bin/cli/commands/seek.ts`:
- Line 198: Update the status flow around writeActiveCursor so read-only status
checks do not refresh active-cursor retention. Keep recalculated frontier
metadata ephemeral, or preserve the existing expiration when persisting updated
metadata, while retaining normal write behavior elsewhere.
In `@src/infrastructure/adapters/GitCasSeekCursorStoreAdapter.ts`:
- Around line 171-176: Centralize the new error codes on PersistenceError by
adding static members for E_CURSOR_RETENTION and E_CURSOR_CORRUPT alongside
E_MISSING_OBJECT, E_REF_NOT_FOUND, and E_REF_IO. Update the raw string usages in
the seek-cursor retention and corruption paths to reference those members
consistently.
- Around line 86-103: The saved-cursor operations readSaved, writeSaved,
deleteSaved, and savedCursorName must validate bookmark names with a
cursor-name-specific validator instead of validateWriterId. Add or reuse
validation that reports invalid saved-cursor names in cursor-name terms, and
apply it consistently across all four methods while preserving the existing
validation behavior for writer IDs.
- Around line 139-189: Update _maintain so sweep failures are isolated from
foreground cursor operations: invoke this.sweep without allowing a rejected
sweep to propagate through _read, writeActive, writeSaved, listSaved, or
_remove. Ensure a failed maintenance attempt advances or otherwise throttles the
next sweep retry, while preserving normal successful sweep scheduling and
expiration behavior.
- Around line 139-147: Update _readPage to accept a PageHandle, remove
toString() from hit.handle when _read calls it, and add toString() to
entry.handle in listSaved() so both cache paths pass the normalized handle type.
In
`@test/integration/infrastructure/adapters/GitCasSeekCursorStoreAdapter.integration.test.ts`:
- Around line 1-108: Keep this integration test focused on restart persistence
and confirming no mutable cursor refs are written; do not add TTL or retention
coverage here because createSeekCursorStore lacks clock injection. Ensure the
unit tests for GitCasSeekCursorStoreAdapter directly cover 30-day active-cursor
expiry and retention sweep 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 745c8eca-0524-4079-84f9-e1732260193c
📒 Files selected for processing (28)
CHANGELOG.mdbin/cli/commands/check.tsbin/cli/commands/checkpoint.tsbin/cli/commands/debug/shared.tsbin/cli/commands/gc.tsbin/cli/commands/history.tsbin/cli/commands/info.tsbin/cli/commands/materialize.tsbin/cli/commands/path.tsbin/cli/commands/query.tsbin/cli/commands/reindex.tsbin/cli/commands/seek.tsbin/cli/commands/seekCursorHelpers.tsbin/cli/commands/tree.tsbin/cli/commands/verify-index.tsbin/cli/shared.tsbin/cli/types.tsdocs/TECHNICAL_TEARDOWN.mdsrc/domain/utils/RefLayout.tssrc/domain/utils/parseCursorBlob.tssrc/infrastructure/adapters/GitCasRepositoryAdapter.tssrc/infrastructure/adapters/GitCasSeekCursorStoreAdapter.tssrc/ports/RuntimeStorageProviderPort.tssrc/ports/SeekCursorStorePort.tstest/bats/cli-seek.batstest/integration/infrastructure/adapters/GitCasSeekCursorStoreAdapter.integration.test.tstest/unit/domain/utils/RefLayout.test.tstest/unit/infrastructure/adapters/GitCasSeekCursorStoreAdapter.test.ts
💤 Files with no reviewable changes (2)
- test/unit/domain/utils/RefLayout.test.ts
- src/domain/utils/RefLayout.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: preflight
- GitHub Check: test-node (22)
- GitHub Check: coverage-threshold
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx,js,jsx}: Do not introduceany,as any,as unknown as,unknown(outside adapters),Record<string, unknown>(outside adapters),*Likeplaceholder types,JSON.parse/JSON.stringify(outside adapters),fetch(outside adapters),process.env(outside adapters),@ts-ignore, orz.any()in core code; use validated boundary models and ports instead.
Use constructor-injected ports for external capabilities; do not rely on ambient dependencies for I/O, clocks, persistence, or entropy.
Do not createutils.ts,helpers.ts,misc.ts, orcommon.ts; name files after the actual concept they model.
Prefer one file per class, type, or object; if a file accumulates peer concepts, split it.
Keep helper corridors, fake shape trust, transitional duplication, and compile-time theater out of the codebase; runtime-honest TypeScript must reflect actual behavior.
No enum usage; prefer runtime-backed domain forms and unions.
Do not use boolean trap parameters; prefer named option objects or separate methods.
Avoid magic strings or numbers when a named constant should exist.
Keep domain bytes asUint8Array;Bufferbelongs in infrastructure adapters.
Files:
src/domain/utils/parseCursorBlob.tssrc/ports/RuntimeStorageProviderPort.tssrc/infrastructure/adapters/GitCasRepositoryAdapter.tssrc/ports/SeekCursorStorePort.tssrc/infrastructure/adapters/GitCasSeekCursorStoreAdapter.ts
src/domain/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/domain/**/*.{ts,tsx,js,jsx}: Insrc/domain/**, do not useDate.now(),new Date(),Date(),performance.now(),Math.random(),crypto.randomUUID(),crypto.getRandomValues(),setTimeout,setInterval, rawnew Error(...)/new TypeError(...), or direct imports from Node built-ins; time, entropy, and external capabilities must enter through ports or parameters, and domain errors should extendWarpError.
Construct domain objects only in core when doing so establishes validated runtime truth; do not build infrastructure adapters, host APIs, persistence implementations, wall clocks, or entropy sources inside core.
Prefer discriminated unions and explicit result types instead of boolean-flag bags, and model expected failures as return values rather than exceptions.
src/domain/must not import host APIs or Node-specific globals; hexagonal architecture boundaries are mandatory.
Domain code must not use the wall clock directly; time must enter through a port or parameter.
Files:
src/domain/utils/parseCursorBlob.ts
src/domain/**/!(*.test).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use explicit domain concepts with validated constructors,
Object.freeze, andinstanceofdispatch; domain objects should be runtime-backed nouns, not ad hoc shape bags.
Files:
src/domain/utils/parseCursorBlob.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Do not use direct imports fromsrc/infrastructure/**insrc/domain/**orsrc/ports/**; depend on a port instead.
Do not use direct Node built-ins insrc/domain/**orsrc/ports/**; use a port instead.
Files:
src/domain/utils/parseCursorBlob.tsbin/cli/commands/tree.tsbin/cli/commands/verify-index.tsbin/cli/commands/query.tssrc/ports/RuntimeStorageProviderPort.tsbin/cli/commands/path.tsbin/cli/commands/materialize.tstest/integration/infrastructure/adapters/GitCasSeekCursorStoreAdapter.integration.test.tsbin/cli/commands/check.tsbin/cli/commands/history.tsbin/cli/commands/reindex.tsbin/cli/commands/debug/shared.tsbin/cli/types.tssrc/infrastructure/adapters/GitCasRepositoryAdapter.tsbin/cli/commands/gc.tssrc/ports/SeekCursorStorePort.tsbin/cli/commands/checkpoint.tsbin/cli/commands/info.tsbin/cli/commands/seekCursorHelpers.tstest/unit/infrastructure/adapters/GitCasSeekCursorStoreAdapter.test.tsbin/cli/commands/seek.tssrc/infrastructure/adapters/GitCasSeekCursorStoreAdapter.tsbin/cli/shared.ts
src/ports/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
In
src/ports/**, do not import Node built-ins orsrc/infrastructure/**; ports must stay abstract and depend only on boundary-safe types.
Files:
src/ports/RuntimeStorageProviderPort.tssrc/ports/SeekCursorStorePort.ts
🧠 Learnings (1)
📚 Learning: 2026-03-08T19:50:17.519Z
Learnt from: flyingrobots
Repo: git-stunts/git-warp PR: 65
File: CHANGELOG.md:88-88
Timestamp: 2026-03-08T19:50:17.519Z
Learning: Follow the Keep a Changelog convention for CHANGELOG.md. Allow duplicate subheadings across versions (e.g., '### Added', '### Fixed'). Configure markdownlint MD024 with {"siblings_only": true} to avoid cross-version false positives.
Applied to files:
CHANGELOG.md
🔇 Additional comments (26)
src/ports/SeekCursorStorePort.ts (1)
1-35: LGTM!src/ports/RuntimeStorageProviderPort.ts (1)
17-17: LGTM!Also applies to: 42-47
src/infrastructure/adapters/GitCasSeekCursorStoreAdapter.ts (1)
31-37: 🚀 Performance & Scalability | 🏗️ Heavy liftVerify the cost of cross-graph sweeping on effectively every CLI invocation.
CURSOR_NAMESPACEis one constant shared by all graphs (scoping is only in the JSON key), and_nextSweepAtMsis in-memory, so it resets to 0 every time a new adapter is constructed. Since the CLI constructs a freshGitCasRepositoryAdapter/GitCasSeekCursorStoreAdapterper process, this means: (a)sweep()— which operates on the entire sharedgit-warp.seek-cursorscache, not just the current graph — runs on virtually every CLI command touching a cursor rather than at the stated "bounded daily cadence", and (b)listSaved()'scache.inspect()pagination must walk and client-side-filter entries from all graphs to find this graph's saved cursors. Whether this is a real cost depends on internal@git-stunts/git-cassweep()/inspect()complexity, which isn't visible here.Please confirm whether
CacheSet.sweep()/inspect()in@git-stunts/git-casv6.5.3 are cheap regardless of shared-namespace size (e.g. index-backed by expiry rather than a full linear scan), or whether per-graph namespacing / a persisted last-sweep marker is warranted.Also applies to: 105-127, 185-199
src/infrastructure/adapters/GitCasRepositoryAdapter.ts (2)
5-5: LGTM!Also applies to: 37-38, 85-85
137-152: 🎯 Functional CorrectnessNo change needed.
GitCasFacade.cachesis compatible withPick<CacheCapability, 'open'>, so the assertion is only a narrowing.bin/cli/commands/info.ts (1)
12-18: LGTM!Also applies to: 115-116
bin/cli/commands/materialize.ts (1)
6-12: LGTM!Also applies to: 76-77
bin/cli/commands/tree.ts (1)
195-196: LGTM!bin/cli/commands/verify-index.ts (1)
21-22: LGTM!src/domain/utils/parseCursorBlob.ts (1)
2-2: LGTM!bin/cli/types.ts (1)
3-3: LGTM!Also applies to: 14-14
bin/cli/commands/seekCursorHelpers.ts (1)
5-30: LGTM!bin/cli/commands/debug/shared.ts (1)
28-29: LGTM!test/unit/infrastructure/adapters/GitCasSeekCursorStoreAdapter.test.ts (1)
1-224: LGTM!test/bats/cli-seek.bats (1)
121-127: LGTM!docs/TECHNICAL_TEARDOWN.md (1)
433-441: LGTM!Also applies to: 469-482
CHANGELOG.md (1)
88-92: LGTM!Also applies to: 107-109
bin/cli/shared.ts (1)
9-18: LGTM!Also applies to: 121-121, 137-157, 177-187
bin/cli/commands/seek.ts (1)
19-20: LGTM!Also applies to: 181-184, 250-256, 270-274, 291-291, 323-323, 336-336, 351-351, 387-387, 411-411
bin/cli/commands/check.ts (1)
149-150: LGTM!bin/cli/commands/checkpoint.ts (1)
21-23: LGTM!Also applies to: 40-41
bin/cli/commands/gc.ts (1)
21-23: LGTM!Also applies to: 39-40, 50-51
bin/cli/commands/history.ts (1)
64-65: LGTM!bin/cli/commands/path.ts (1)
71-72: LGTM!bin/cli/commands/query.ts (1)
160-161: LGTM!bin/cli/commands/reindex.ts (1)
13-14: LGTM!
Release Preflight
If this PR is from a |
Release Preflight
If this PR is from a |
1 participant
Summary
git-warp.seek-cursorscache namespacerefs/warp/<graph>/cursor/*protocol and its ref builders; v19 intentionally leaves old operator-only cursor refs inert rather than migrating non-authoritative stateIssue scope: replay admission landed in #776; this PR completes the remaining active/saved cursor half of #740.
Closes #740.
Validation
npm run typechecknpm run lintnpm run lint:mdnpm run lint:semgrepnpm run typecheck:surfacenpm run typecheck:policynpx vitest run test/unit test/integration— 632 passed files, 1 skipped; 7,475 passed tests, 2 skippedPATH="$PWD/bin:$PATH" bats test/bats/— 111/111 passedCacheSet.inspect()