refactor(tape): establish layered architecture#1996
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR introduces a layered Tape subsystem with domain contracts, capability-scoped ports, application services, SQLite storage and lifecycle adapters, compatibility re-exports, updated runtime composition, memory inspection routing, transactional cleanup, and expanded architecture and native SQLite test coverage. ChangesTape layering refactor
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DeepChatLoopRunner
participant TapeToolFactWriter
participant SessionTape
participant SQLiteTapeEntries
DeepChatLoopRunner->>TapeToolFactWriter: appendToolFact(input)
TapeToolFactWriter->>SessionTape: persist tool fact
SessionTape->>SQLiteTapeEntries: append idempotent tape entry
SQLiteTapeEntries-->>SessionTape: entry reference
SessionTape-->>TapeToolFactWriter: entry reference
TapeToolFactWriter-->>DeepChatLoopRunner: persistence result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 `@scripts/check-memory-test-scope.mjs`:
- Around line 9-10: Update the NATIVE_TAPE_TEST pattern to recognize both
.test.ts and .spec.ts suffixes for tape suites and deepchatTapeEntriesTable,
matching the walker’s supported naming conventions while preserving the existing
native-test path constraints.
In `@src/main/tape/application/forkService.ts`:
- Around line 288-294: Update the receipt data construction for the
external-merge event, including the corresponding block noted at the second
occurrence, so metadata cannot overwrite the authoritative forkId,
forkSessionId, or referencedEntryCount fields. Move the metadata spread before
these explicitly assigned receipt fields while preserving the existing metadata
values for all other keys.
- Around line 103-113: Update createFork to also check for an existing merge
receipt using the corresponding merge provenance key for parentSessionId and
forkIdValue. If either a discard or merge receipt exists, reject reuse by
throwing an error before creating the fork, preserving the existing discard
behavior.
In `@src/main/tape/application/lineageService.ts`:
- Around line 390-438: Update linkSubagentTape to validate both
normalized.parentSessionId and normalized.childSessionId inside the transaction
before appendEvent, ensuring the parent exists and the child is a direct
subagent session as required by resolveLinkedTapeSources(). Preserve the
existing unavailable-child error behavior while rejecting invalid parent/child
lineage before persisting the link.
In `@src/main/tape/application/reconcilerService.ts`:
- Around line 32-34: Update the historyRecords construction in the reconciler
flow to shallow-copy the array returned by messageStore.getMessages(sessionId)
before applying the orderSeq sort. Preserve the existing ascending sort behavior
while ensuring the retrieved or cached messages array is not mutated.
In `@src/main/tape/application/viewReplayService.ts`:
- Around line 435-453: The toViewManifestRecord method must reject manifests
whose embedded messageId or requestSeq disagrees with the persisted event
envelope. Validate the normalized manifest against the row’s
source_id/source_seq values before returning a record, and return null for
mismatches while preserving the existing normalization and hash verification
flow for valid rows.
In `@src/main/tape/domain/effectiveSemantics.ts`:
- Around line 45-61: Update the block predicates used to build
pendingInteractionToolIds and determine the final blocks.some result so they
safely handle null or primitive entries from parsed JSON. Use optional property
access for block fields, including nested action/tool_call data as needed, while
preserving the existing matching behavior for valid block objects.
In `@src/main/tape/infrastructure/sqlite/tapeSearchProjectionStore.ts`:
- Around line 794-838: Update the catch path in searchFts to invalidate the FTS
derivative when the SQLite query fails by resetting ftsReady and clearing the
current FTS metadata through the existing state-management mechanism. Preserve
the empty-result fallback behavior so callers continue to fall back to LIKE,
while ensuring the next search retries FTS recovery instead of reusing stale
readiness state.
In `@test/main/app/compositionBoundaries.test.ts`:
- Around line 12-13: Update the assertions in the composition boundary test to
target the LegacyChatImportService constructor call structurally or with a
constructor-specific pattern, rather than matching a raw whitespace-sensitive
substring. Ensure the assertion verifies memoryDatabase and
sessionData.tapeStore are passed within that constructor invocation, and format
the test according to Oxfmt.
In `@test/main/routes/dispatcher.test.ts`:
- Around line 1275-1278: Separate Tape inspection mocking from the SQLite-backed
deepchatTapeEntriesTable in the affected route test setup: create a dedicated
tapeInspection mock implementing getEffectiveMessageSourceSpan and
listMemoryViewManifestsByAgent, and have the relevant dependency container
return it through getTapeInspection. Remove these DTO methods from
deepchatTapeEntriesTable while preserving the existing storage mock behavior
across all referenced test cases.
In `@test/main/session/data/tapeTestHarness.ts`:
- Around line 93-99: Update the idempotent lookup in the tape test harness
around the existing and entries.find logic to run only when provenanceKey is
non-null. Preserve the current session and provenance matching for keyed writes,
while treating idempotent writes without a provenance key as having no existing
entry.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28becfcc-3a9b-4e9b-a1fb-203e2be7c4f3
📒 Files selected for processing (87)
docs/FLOWS.mddocs/architecture/baselines/agent-system-layered-runtime-baseline.jsondocs/architecture/memory-system.mddocs/architecture/session-management.mddocs/architecture/tape-layering/plan.mddocs/architecture/tape-layering/spec.mddocs/architecture/tape-layering/tasks.mddocs/architecture/tape-system.mdscripts/check-memory-test-scope.mjsscripts/generate-architecture-baseline.mjssrc/main/agent/acp/compatibility/adapters.tssrc/main/agent/acp/compatibility/dependencies.tssrc/main/agent/deepchat/loop/ports.tssrc/main/agent/deepchat/memory/memoryRuntimeCoordinator.tssrc/main/agent/deepchat/runtime/contextBuilder.tssrc/main/agent/deepchat/runtime/deepChatLoopRunner.tssrc/main/agent/deepchat/runtime/deepChatRuntimeCoordinator.tssrc/main/agent/deepchat/runtime/process.tssrc/main/agent/deepchat/runtime/turnCoordinator.tssrc/main/agent/deepchat/runtime/types.tssrc/main/app/composition.tssrc/main/app/startupMigrations/legacyChatImportService.tssrc/main/data/schemaCatalog.tssrc/main/memory/data/tables/deepchatMemoryIngestionProjection.tssrc/main/memory/routes.tssrc/main/session/data/database.tssrc/main/session/data/index.tssrc/main/session/data/settings.tssrc/main/session/data/tables/deepchatTapeEffectiveSemantics.tssrc/main/session/data/tables/deepchatTapeEntries.tssrc/main/session/data/tables/deepchatTapeSearchProjection.tssrc/main/session/data/tape.tssrc/main/session/data/tapeEffectiveView.tssrc/main/session/data/tapeFacts.tssrc/main/session/data/tapeViewManifest.tssrc/main/session/data/transcript.tssrc/main/session/transcriptMutations.tssrc/main/tape/application/common.tssrc/main/tape/application/contracts.tssrc/main/tape/application/factPersistence.tssrc/main/tape/application/factService.tssrc/main/tape/application/forkService.tssrc/main/tape/application/generationLifecycle.tssrc/main/tape/application/lineageService.tssrc/main/tape/application/recallProjection.tssrc/main/tape/application/recallService.tssrc/main/tape/application/reconcilerService.tssrc/main/tape/application/sessionTape.tssrc/main/tape/application/viewReplayService.tssrc/main/tape/domain/effectiveSemantics.tssrc/main/tape/domain/effectiveView.tssrc/main/tape/domain/entry.tssrc/main/tape/domain/facts.tssrc/main/tape/domain/replay.tssrc/main/tape/domain/viewManifest.tssrc/main/tape/infrastructure/sqlite/tapeEntryStore.tssrc/main/tape/infrastructure/sqlite/tapeLifecycleAdapter.tssrc/main/tape/infrastructure/sqlite/tapeSearchProjectionStore.tssrc/main/tape/ports/application.tssrc/main/tape/ports/capabilities.tssrc/main/tape/ports/storage.tstest/main/agent/acp/compatibility/adapters.test.tstest/main/agent/deepchat/memory/memoryRuntimeCoordinator.test.tstest/main/agent/deepchat/runtime/deepChatRuntimeCoordinator.test.tstest/main/agent/deepchat/runtime/process.test.tstest/main/app/compositionBoundaries.test.tstest/main/app/startupMigrations/legacyChatImportService.test.tstest/main/evals/nativeAgent/harness.tstest/main/memory/deepchatMemoryIngestionProjection.test.tstest/main/routes/dispatcher.test.tstest/main/scripts/memoryTestScope.test.tstest/main/session/data/settings.test.tstest/main/session/data/tables/deepchatTapeEntriesTable.test.tstest/main/session/data/tape.test.tstest/main/session/data/tapeFork.test.tstest/main/session/data/tapeLifecycle.test.tstest/main/session/data/tapeLineage.test.tstest/main/session/data/tapeRecall.test.tstest/main/session/data/tapeReconciler.test.tstest/main/session/data/tapeTestHarness.tstest/main/session/data/tapeViewReplay.test.tstest/main/session/data/transcript.test.tstest/main/session/runtimeIntegration.test.tstest/main/session/transcriptMutations.test.tstest/main/session/usageStatsService.test.tstest/main/tape/layerBoundaries.test.tstest/memory-test-scope.json
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
2 participants
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
src/main/tape/with domain, ports, application, and SQLite infrastructure layers.SessionTapeimplementation into Fact, Reconciler, Recall, Lineage, View/Replay, and Fork services.SessionTapeas a compatibility facade while narrowing consumers to capability-specific ports.SessionTapePort, renderer APIs, View policies, and compaction behavior.Reliability and boundary hardening
clearMessages.Architecture changes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests