feat(session): ground recovery in the platform harness - #526
Open
yordis wants to merge 13 commits into
Open
Conversation
yordis
commented
Aug 4, 2026
yordis
commented
Member
- Keep the core Session contract independent from third-party transcript and resume semantics.
- Make authoritative event replay the recovery baseline while allowing safe checkpoint acceleration.
- Preserve the Claude SDK findings as comparative evidence without making them platform dependencies.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryMedium Risk Overview Recovery is explicitly layered. Four durable concepts must not be conflated: authoritative event log, advisory aggregate snapshot, opaque harness recovery checkpoint (wire ADR#0035 aligns the session store with that model: Tooling and research: ADR link lint ignores Reviewed by Cursor Bugbot for commit 40fdaaf. Bugbot is set up for automated code reviews on this repo. Configure here. |
WalkthroughThe PR makes the platform-managed harness the normative v1 implementation, formalizes checkpoint admission and restoration, removes ChangesHarness and session recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR tightens the Session recovery/restore contract by removing third-party resume semantics from ExecutionAttemptStarted and making event replay + digest-coupled checkpoint restoration the baseline, while expanding the internal research/docs around session-store recovery boundaries.
Changes:
- Removed
resume_cursorfromExecutionAttemptStarted(proto + generated Rust + tests) and reserved the retired field number. - Added per-event validation ensuring a restored checkpoint’s
session_execution_plan_digestmatches the containingExecutionAttemptStarted’s plan digest, plus a new regression test. - Updated ADR/glossary/research docs to sharpen the distinction between event logs, aggregate snapshots, harness recovery checkpoints, and read-side checkpoints; added a pinned Claude Agent SDK session-type snapshot doc.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rsworkspace/crates/platform/trogonai-proto/src/session/sessions/validate/tests.rs | Removes resume_cursor from test fixtures and adds a plan-digest mismatch validation test for restored checkpoints. |
| rsworkspace/crates/platform/trogonai-proto/src/session/sessions/validate.rs | Adds RestoredCheckpointPlanDigestMismatch and enforces per-event digest equality between restored checkpoints and the attempt event. |
| rsworkspace/crates/platform/trogonai-proto/src/session/sessions/codec/tests.rs | Updates codec test fixtures to match the removed resume_cursor field. |
| rsworkspace/crates/platform/trogonai-proto/src/gen/trogonai.session.sessions.v1alpha1.execution_attempt_started.rs | Regenerated type removing resume_cursor from the Rust surface and codec implementation. |
| rsworkspace/crates/platform/trogonai-proto/src/gen/trogonai.session.sessions.v1alpha1.execution_attempt_started.__view.rs | Regenerated view/encode/serialize logic removing resume_cursor. |
| rsworkspace/crates/platform/trogonai-proto/src/gen/trogonai.session.sessions.v1alpha1.checkpoint.rs | Updates generated docs to reflect per-event plan digest validation vs aggregate binding. |
| rsworkspace/crates/platform/trogonai-proto/src/gen/trogonai.session.sessions.v1alpha1.checkpoint.__view.rs | Same doc clarification in the generated view file. |
| proto/trogonai/session/sessions/v1alpha1/execution_attempt_started.proto | Removes resume_cursor and reserves tag/name for forward-compatibility. |
| proto/trogonai/session/sessions/v1alpha1/checkpoint.proto | Clarifies checkpoint semantics and validation boundaries in comments. |
| docs/research/session-store/products/claude-agent-sdk.md | Tightens language around Claude SessionStore being non-authoritative and links to the pinned type snapshot. |
| docs/research/session-store/products/claude-agent-sdk-session-types.md | New pinned inspection snapshot of Claude Agent SDK session-related TypeScript surface and comparison. |
| docs/research/session-store/index.md | Adds the new Claude session type snapshot to the research index. |
| docs/glossary/checkpoint.md | Disambiguates read-side checkpoint vs aggregate snapshot vs protobuf Checkpoint. |
| docs/adr/0035-session-store-decider-aggregate.md | Refines ADR terminology and validation-boundary ownership around checkpoints/snapshots/recovery. |
| docs/adr/0031-agent-implementation-and-session-plan.md | Updates ownership model to emphasize platform harness as v1 baseline and separates recovery artifacts/positions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/glossary/checkpoint.md (1)
9-16: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valuePoint to ADR#0013 for the read-side checkpoint contract.
physical stream sequenceand read-side checkpoint authority now come from ADR#0013, with ADR#0035 forwarding to it, not from ADR#0031. Update the referenced authority.🤖 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 `@docs/glossary/checkpoint.md` around lines 9 - 16, Update the checkpoint glossary entry’s ADR reference to point to ADR#0013 as the authority for the read-side checkpoint contract, replacing the current ADR#0031 reference while preserving the existing distinction from snapshots and protobuf checkpoints.
🤖 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 `@docs/adr/0031-agent-implementation-and-session-plan.md`:
- Around line 206-213: The ADR’s normative SessionExecutionPlan requirements
conflict with AgentConfiguration’s current model-selection contract and
ADR#0032’s unresolved ownership decision. Reconcile the AgentConfiguration
model-selection contract before treating SessionExecutionPlan as normative:
either expose typed primary and auxiliary ModelSelection values from
AgentConfiguration, or mark the plan fields and admission reads provisional
while explicitly defining their authoritative interim source.
- Around line 321-343: Define an opaque checkpoint admission proof for the
CheckpointProduced gate, such as a manifest, attestation, verifier, or recorded
replay evidence, that binds the sealed artifact to the producing
ExecutionAttempt, immutable SessionExecutionPlan digest, effective Session
history, covered platform ordinal, checkpoint format, and captured state. Update
the admission requirements around Checkpoint.covers_through so this proof
establishes semantic coverage and replay equivalence before CheckpointProduced
is recorded; unavailable or failed proof must reject the checkpoint.
In `@docs/adr/0035-session-store-decider-aggregate.md`:
- Around line 214-219: The ADR’s CheckpointProduced idempotency contract must
distinguish requests that share checkpoint_id and artifact bytes but differ in
other checkpoint evidence. Update the command matrix and event identity
description to derive the key from checkpoint_id plus a digest of the canonical
complete checkpoint evidence or request, ensuring conflicting later evidence is
retained while byte-identical redelivery still collapses.
---
Nitpick comments:
In `@docs/glossary/checkpoint.md`:
- Around line 9-16: Update the checkpoint glossary entry’s ADR reference to
point to ADR#0013 as the authority for the read-side checkpoint contract,
replacing the current ADR#0031 reference while preserving the existing
distinction from snapshots and protobuf checkpoints.
🪄 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: CHILL
Plan: Pro Plus
Run ID: f77fe418-a6dd-4715-9d11-86fab65cebcc
⛔ Files ignored due to path filters (4)
rsworkspace/crates/platform/trogonai-proto/src/gen/trogonai.session.sessions.v1alpha1.checkpoint.__view.rsis excluded by!**/gen/**rsworkspace/crates/platform/trogonai-proto/src/gen/trogonai.session.sessions.v1alpha1.checkpoint.rsis excluded by!**/gen/**rsworkspace/crates/platform/trogonai-proto/src/gen/trogonai.session.sessions.v1alpha1.execution_attempt_started.__view.rsis excluded by!**/gen/**rsworkspace/crates/platform/trogonai-proto/src/gen/trogonai.session.sessions.v1alpha1.execution_attempt_started.rsis excluded by!**/gen/**
📒 Files selected for processing (11)
docs/adr/0031-agent-implementation-and-session-plan.mddocs/adr/0035-session-store-decider-aggregate.mddocs/glossary/checkpoint.mddocs/research/session-store/index.mddocs/research/session-store/products/claude-agent-sdk-session-types.mddocs/research/session-store/products/claude-agent-sdk.mdproto/trogonai/session/sessions/v1alpha1/checkpoint.protoproto/trogonai/session/sessions/v1alpha1/execution_attempt_started.protorsworkspace/crates/platform/trogonai-proto/src/session/sessions/codec/tests.rsrsworkspace/crates/platform/trogonai-proto/src/session/sessions/validate.rsrsworkspace/crates/platform/trogonai-proto/src/session/sessions/validate/tests.rs
💤 Files with no reviewable changes (1)
- rsworkspace/crates/platform/trogonai-proto/src/session/sessions/codec/tests.rs
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 2d8bc8c Minimum allowed coverage is ♻️ This comment has been updated with latest results |
The corpus described how ten products store sessions but only measured one of them against our own catalog, so nothing weighted a product's evidence by how proven its store is, and the two gaps the synthesis leaves open stayed open. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.
Suppressed comments (1)
proto/trogonai/session/sessions/v1alpha1/execution_attempt_started.proto:30
reservedfield names in protobuf must be quoted string literals.reserved resume_cursor;is invalid proto syntax and will fail compilation with protoc/buf.
Use reserved "resume_cursor"; (and keep reserved 7;) to reserve both the field number and the former name.
reserved 7;
reserved resume_cursor;
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…convention The rest of docs/ uses no em dash, and a corpus that mixes both makes the punctuation itself look like a signal about which artifact came from where. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…atalog Our Session Store decisions were drawn from ten products, and the sample was too small to tell a considered design apart from an accident of who we happened to read. Weighting each store by how proven it is rather than how popular its product is, is what lets a recommendation cite evidence instead of taste. Two decisions in ADR#0035 are the ones this evidence is meant to test, so the comparisons are written to argue against them where the sources support it. Mechanical verification only earns trust when it runs over the whole corpus at once: run per artifact as each landed, it passed six files that were not clean, because a basename unique in one tree becomes ambiguous the moment a sibling tree joins the run. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (23)
docs/research/session-store/index.md-42-59 (1)
42-59: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCorrect the verification-status claim.
Lines 42-48 say that every listed artifact passed both verification layers. Lines 56-59 then say that ten listed dossiers received hand verification only. Separate the locally swept artifacts from the hand-only dossiers so the corpus does not claim mechanical coverage that it does not have.
🤖 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 `@docs/research/session-store/index.md` around lines 42 - 59, Update the verification-status wording in the introductory claims around the corpus sweep to distinguish artifacts covered by both mechanical and hand verification from the ten hand-only dossiers. Remove the blanket claim that every listed artifact passed both layers, while preserving the reported 1912-citation sweep and its resolved-status details for the locally checked-out products.docs/research/session-store/RESEARCH_PROMPT_COMPARISON.md-40-42 (1)
40-42: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse markdown links for every
ADR#0035reference.The ADR-reference lint failure has the same cause in both files. Replace each bare reference with the canonical relative link.
docs/research/session-store/RESEARCH_PROMPT_COMPARISON.md#L40-L42: link the ADR reference.docs/research/session-store/RESEARCH_PROMPT_COMPARISON.md#L112-L118: link the ADR reference in the recommendation requirements.docs/research/session-store/RESEARCH_PROMPT_COMPARISON.md#L159-L166: link the ADR reference in the method.docs/research/session-store/RESEARCH_PROMPT_COMPARISON.md#L172-L180: link the ADR reference in the output skeleton.docs/research/session-store/index.md#L15-L20: link the ADR reference in the stage-two description.🤖 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 `@docs/research/session-store/RESEARCH_PROMPT_COMPARISON.md` around lines 40 - 42, Replace every bare ADR#0035 reference with the canonical relative Markdown link in docs/research/session-store/RESEARCH_PROMPT_COMPARISON.md at lines 40-42, 112-118, 159-166, and 172-180, and in docs/research/session-store/index.md at lines 15-20. Ensure all five references use the same canonical link format.Source: Pipeline failures
docs/research/session-store/products/letta/vs-session-events.md-6-6 (1)
6-6: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winConvert bare ADR references to markdown links.
The file uses bare
ADR#0031andADR#0035references. The ADR reference linter rejects these references, and CI currently fails.Use the repository-relative links below, then apply the same change to every bare ADR reference in this file and the affected comparison documents.
Proposed fix
-Compared against `proto/trogonai/session/sessions/v1alpha1/` and ADR#0035 on 2026-08-04. +Compared against `proto/trogonai/session/sessions/v1alpha1/` and [ADR#0035](../../../../adr/0035-session-store-decider-aggregate.md) on 2026-08-04. -because ADR#0031 already scopes a Session +because [ADR#0031](../../../../adr/0031-agent-implementation-and-session-plan.md) already scopes a SessionAlso applies to: 164-168
🤖 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 `@docs/research/session-store/products/letta/vs-session-events.md` at line 6, Replace every bare ADR reference in this document and the affected comparison documents with repository-relative Markdown links, including ADR#0035 in the comparison sentence and the references near the other flagged section. Preserve the surrounding text while ensuring no unlinked ADR#NNNN references remain.Source: Pipeline failures
docs/research/session-store/products/mastra/vs-session-events.md-6-6 (1)
6-6: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReplace all bare ADR references with Markdown links.
Line 6 and many later lines use plain
ADR#0035text. Thelint-adr-refspipeline rejects these references. Convert every occurrence in this file to a relative link todocs/adr/0035-session-store-decider-aggregate.md. Preserve any surrounding line-number text.The pipeline currently reports 335 bare ADR references across the product documentation, so CI cannot pass until these references are linked.
Proposed fix
-Compared against `proto/trogonai/session/sessions/v1alpha1/` and ADR#0035 on 2026-08-04. +Compared against `proto/trogonai/session/sessions/v1alpha1/` and [ADR#0035](../../../../adr/0035-session-store-decider-aggregate.md) on 2026-08-04.🤖 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 `@docs/research/session-store/products/mastra/vs-session-events.md` at line 6, Replace every bare ADR#0035 reference in the document with a relative Markdown link targeting docs/adr/0035-session-store-decider-aggregate.md, preserving any surrounding text and line-number references.Source: Pipeline failures
docs/research/session-store/products/google-adk/vs-session-events.md-6-6 (1)
6-6: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winLink all unlinked ADR references before merge.
The affected documents use plain ADR references, and the ADR validation job reports 335 unresolved references.
docs/research/session-store/products/google-adk/vs-session-events.md#L6-L6: LinkADR#0035and convert the remaining bareADR#...references in the file.docs/research/session-store/products/hermes-agent/index.md#L492-L492: LinkADR 0031to../../../../adr/0031-agent-implementation-and-session-plan.md.🤖 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 `@docs/research/session-store/products/google-adk/vs-session-events.md` at line 6, The ADR references are unlinked and fail ADR validation. In docs/research/session-store/products/google-adk/vs-session-events.md:6, link ADR#0035 and convert every remaining bare ADR#... reference in the file; in docs/research/session-store/products/hermes-agent/index.md:492, link ADR 0031 to the specified agent implementation and session plan document.Source: Pipeline failures
docs/research/session-store/products/crush/index.md-943-951 (1)
943-951: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCorrect the
ListNewFilesstatus.This paragraph says that
db.Neweagerly preparesListNewFilesand would fail becausefiles.is_newdoes not exist. That makes the mismatch a potential database-initialization failure, not a dormant issue. Change the conclusion to reflect the startup impact.🤖 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 `@docs/research/session-store/products/crush/index.md` around lines 943 - 951, The `ListNewFiles`/`is_new` mismatch is incorrectly described as dormant. Update the paragraph’s conclusion to state that `db.New` eagerly prepares `ListNewFiles`, so initialization can fail when the missing `files.is_new` column is encountered; retain the existing evidence and unresolved-origin note.docs/research/session-store/products/aider/vs-session-events.md-6-6 (1)
6-6: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winConvert all bare ADR references before merge.
CI reports 335 ADR-reference failures. Replace every bare
ADR#0035occurrence with a relative Markdown link todocs/adr/0035-session-store-decider-aggregate.md.
docs/research/session-store/products/aider/vs-session-events.md#L6-L6: update the comparison line and all later bare ADR references.docs/research/session-store/products/amazon-q/vs-session-events.md#L6-L6: update the comparison line and all later bare ADR references.docs/research/session-store/products/continue/vs-session-events.md#L6-L6: update the comparison line and all later bare ADR references.🤖 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 `@docs/research/session-store/products/aider/vs-session-events.md` at line 6, Replace every bare ADR#0035 reference with a relative Markdown link to docs/adr/0035-session-store-decider-aggregate.md, including the comparison line and later references in docs/research/session-store/products/aider/vs-session-events.md (line 6), docs/research/session-store/products/amazon-q/vs-session-events.md (line 6), and docs/research/session-store/products/continue/vs-session-events.md (line 6).Source: Pipeline failures
docs/research/session-store/products/crush/vs-session-events.md-6-6 (1)
6-6: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winLink every ADR reference.
This file contains multiple bare
ADR#0035andADR#0029references. CI reports 335 ADR-reference errors. Convert every bare reference to a relative Markdown link. Do not fix only Line 466.Also applies to: 42-47, 118-123, 134-138, 211-217, 244-254, 256-260, 390-396, 454-466
🤖 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 `@docs/research/session-store/products/crush/vs-session-events.md` at line 6, Update every bare ADR#0035 and ADR#0029 reference throughout the document, including the locations identified by the review, to use the appropriate relative Markdown ADR links. Ensure no unlinked ADR references remain; do not limit the change to the cited line.Source: Pipeline failures
docs/research/session-store/products/aws-strands/index.md-467-476 (1)
467-476: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCorrect the missing-field compatibility claim.
from_dictfilters unknown keys, but it does not default every missing field.Session.session_id,Session.session_type,SessionAgent.agent_id,SessionAgent.state,SessionAgent.conversation_manager_state,SessionMessage.message, andSessionMessage.message_idare required fields. Missing values fail construction. Limit the conclusion to unknown-field tolerance and fields with dataclass defaults.🤖 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 `@docs/research/session-store/products/aws-strands/index.md` around lines 467 - 476, Correct the compatibility finding in the documented analysis around Session, SessionAgent, and SessionMessage: state only that from_dict filters unknown keys and preserves fields with dataclass defaults, while explicitly noting that required fields such as session_id, session_type, agent_id, state, conversation_manager_state, message, and message_id fail construction when missing. Remove the claim that missing fields generally default, while retaining the conclusion about lacking migration and version negotiation.docs/research/session-store/products/aws-strands/vs-session-events.md-93-93 (1)
93-93: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSeparate session-log deletion from artifact erasure.
This row describes
ArtifactErasedas per-artifact byte destruction, then says v1alpha1 does not attempt erasure “at all.” Clarify that v1alpha1 does not delete session-log bytes, whileArtifactErasedcovers out-of-band artifacts. The current wording gives an incorrect privacy and retention comparison.🤖 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 `@docs/research/session-store/products/aws-strands/vs-session-events.md` at line 93, Update the comparison row describing v1alpha1 deletion semantics to distinguish session-log retention from artifact handling: state that v1alpha1 does not delete session-log bytes, while ArtifactErased represents out-of-band artifact erasure. Replace the wording that says v1alpha1 does not attempt erasure “at all,” and preserve the existing Strands deletion comparison.docs/research/session-store/products/cline/vs-session-events.md-87-87 (1)
87-87: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCorrect the Cline deletion comparison.
Cline’s explicit
deleteSession()path deletes session artifacts, including messages, manifests, compaction state, and checkpoint references. The statement “neither side deletes bytes” is incorrect. State that Cline performs manual physical deletion but has no automatic retention policy, while the platform’s v1 session log remains append-only.🤖 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 `@docs/research/session-store/products/cline/vs-session-events.md` at line 87, Update the Cline deletion comparison in the table row to state that Cline’s manual deleteSession() path physically deletes session artifacts, while it has no automatic retention or TTL policy. Clarify that the platform’s v1 session log remains append-only, and remove the inaccurate claim that neither side deletes bytes.docs/research/session-store/products/aws-strands/vs-session-events.md-6-6 (1)
6-6: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winConvert all bare ADR references to markdown links.
The ADR-reference lint reports 335 failures. Update every affected site to use
[ADR#0035](../../../../adr/0035-session-store-decider-aggregate.md).
docs/research/session-store/products/aws-strands/vs-session-events.md#L6-L6: link the comparison header reference.docs/research/session-store/products/aws-strands/vs-session-events.md#L111-L114: link the facet 3 reference.docs/research/session-store/products/aws-strands/vs-session-events.md#L155-L160: link the facet 7 reference.docs/research/session-store/products/aws-strands/vs-session-events.md#L195-L200: link the optional tiering reference.docs/research/session-store/products/cline/vs-session-events.md#L6-L6: link the comparison header reference.docs/research/session-store/products/cline/vs-session-events.md#L47-L50: link the append-only mutation reference.docs/research/session-store/products/cline/vs-session-events.md#L69-L71: link the decision 1, 2, and 6 references.docs/research/session-store/products/cline/vs-session-events.md#L84-L84: link the decision 6 reference.docs/research/session-store/products/cline/vs-session-events.md#L93-L97: link the decision 6 reference.docs/research/session-store/products/cline/vs-session-events.md#L141-L146: link the decision 6 reference.docs/research/session-store/products/cline/vs-session-events.md#L164-L172: link the decision 6 reference.docs/research/session-store/products/cline/vs-session-events.md#L312-L318: link the decision 6 reference.docs/research/session-store/products/cline/vs-session-events.md#L361-L367: link the decision 6 reference.docs/research/session-store/products/cline/vs-session-events.md#L418-L424: link the decision 7 reference.🤖 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 `@docs/research/session-store/products/aws-strands/vs-session-events.md` at line 6, Replace every bare ADR#0035 reference with the markdown link [ADR#0035](../../../../adr/0035-session-store-decider-aggregate.md) at all listed sites: docs/research/session-store/products/aws-strands/vs-session-events.md lines 6, 111-114, 155-160, and 195-200; and docs/research/session-store/products/cline/vs-session-events.md lines 6, 47-50, 69-71, 84, 93-97, 141-146, 164-172, 312-318, 361-367, and 418-424. Update each comparison header, facet, tiering, mutation, and decision reference without changing surrounding content.Source: Pipeline failures
docs/research/session-store/products/cline/vs-session-events.md-447-456 (1)
447-456: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winLabel the snapshot recovery path as draft design.
session-types.mdLines 376-385 states that no Session-specific decider, event store composition, projection, snapshot policy, or reconciler exists yet. Replace “our runtime resumes” and similar present-tense claims with “the draft design intends” unless implementation evidence is added.🤖 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 `@docs/research/session-store/products/cline/vs-session-events.md` around lines 447 - 456, Revise the claims in this discussion around decision 7 to describe snapshot recovery as a draft design, replacing present-tense statements such as “our runtime resumes” with wording that says the draft design intends to resume from the newest snapshot and replay only the tail. Do not present the session-specific decider, event store, projection, snapshot policy, or reconciler as implemented.docs/research/session-store/products/continue/index.md-765-766 (1)
765-766: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winLink the ADR reference so CI passes.
ADR 0031is a bare ADR reference. Convert it to the required linked form, such as[ADR#0031](../../../../adr/0031-agent-implementation-and-session-plan.md). The ADR validation job currently reports 335 bare-reference errors.🤖 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 `@docs/research/session-store/products/continue/index.md` around lines 765 - 766, Update the ADR 0031 reference in the surrounding documentation text to the required linked form, using the repository’s ADR 0031 document path and preserving the existing sentence meaning.Source: Pipeline failures
docs/research/session-store/products/continue/index.md-455-457 (1)
455-457: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCorrect the
Session.modecompatibility conclusion.The dossier says format evolution is entirely additive and that old sessions simply lack
mode. The earlier union included"edit", but the current union excludes it. Persisted sessions can therefore containmode: "edit";JSON.parsedoes not migrate or validate this value. Document this as a removed enum value and define the recovery behavior: migrate, normalize, or reject it.Also applies to: 497-504
🤖 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 `@docs/research/session-store/products/continue/index.md` around lines 455 - 457, Update the Session.mode compatibility discussion and the related section around the earlier union to acknowledge that persisted sessions may contain the removed "edit" enum value even though the current union excludes it. Document the selected recovery behavior—migration, normalization, or rejection—and distinguish this enum removal from additive optional-field evolution.docs/research/session-store/products/openai-agents-sdk/vs-session-events.md-157-163 (1)
157-163: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAccount for legacy digest compatibility before calling this change non-breaking.
If the canonical digest input changes, old
CheckpointProducedrecords retain their previous digest. A redelivery computed with the new rule will not compare equal unless verification supports both canonicalizations or the digest algorithm is versioned. “No persisted-event rewrite needed” is not established.🤖 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 `@docs/research/session-store/products/openai-agents-sdk/vs-session-events.md` around lines 157 - 163, Revise the “Blast radius” discussion to account for legacy digest compatibility: state that changing the canonical digest input requires verification to support both old and new canonicalizations or to version the digest algorithm, otherwise redelivered evidence will not match existing CheckpointProduced digests. Remove the claim that no persisted-event rewrite is needed unless this compatibility behavior is explicitly established.docs/research/session-store/products/openhands/index.md-47-77 (1)
47-77: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDocument the cross-file commit boundary.
events/andbase_state.jsonare written through separate operations.append_eventappends the event, then advancesleaf_event_idin the mutable base-state document. A crash between those writes can leave a durable event that the persisted HEAD does not reference. Define the recovery rule or add an integrity check before presenting both files as authoritative.🤖 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 `@docs/research/session-store/products/openhands/index.md` around lines 47 - 77, Document the cross-file commit boundary for EventLog.append() and the subsequent leaf_event_id update in ConversationState, including the crash window between writes. Define and document recovery behavior for durable events not referenced by persisted HEAD, or add an integrity check that detects this mismatch before treating events/ and base_state.json as authoritative.docs/research/session-store/products/openai-agents-sdk/vs-session-events.md-6-6 (1)
6-6: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winLink all bare ADR references in the research corpus.
The ADR-reference validator rejects plain ADR references. Apply the same relative-link fix at every affected site.
docs/research/session-store/products/openai-agents-sdk/vs-session-events.md#L6-L6: link allADR#0035andADR#0031references.docs/research/session-store/products/void/vs-session-events.md#L6-L6: link allADR#0035references.docs/research/session-store/products/zed/index.md#L566-L566: link theADR 0031/0035references.🤖 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 `@docs/research/session-store/products/openai-agents-sdk/vs-session-events.md` at line 6, Replace every bare ADR reference with the appropriate relative ADR link: in docs/research/session-store/products/openai-agents-sdk/vs-session-events.md:6, link all ADR#0035 and ADR#0031 references; in docs/research/session-store/products/void/vs-session-events.md:6, link all ADR#0035 references; and in docs/research/session-store/products/zed/index.md:566, link the ADR 0031/0035 references.Source: Pipeline failures
docs/research/session-store/products/openhands/index.md-166-177 (1)
166-177: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDocument event-file recovery before claiming non-crash-safe compaction.
LocalFileStore.write()writes directly without temp-file-and-rename atomicity, so partial JSON from a crash can remain. The compaction guarantee needs an explicit resume policy for malformed files, such as repairing or quarantining them before compaction runs.🤖 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 `@docs/research/session-store/products/openhands/index.md` around lines 166 - 177, The durability documentation should explicitly describe recovery for malformed or partially written event files before compaction proceeds. Update the event-store behavior described around the referenced event append flow to state that invalid JSON is detected and repaired or quarantined according to an explicit resume policy, then compaction continues safely; do not claim crash-safe compaction without documenting this policy.docs/research/session-store/products/zed/vs-session-events.md-6-6 (1)
6-6: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftLink every ADR reference before merging.
This file contains bare
ADR#0026andADR#0035references. CI reports 335 bare-reference failures across the session-events documentation. Replace every bare reference in this file with a relative Markdown link.For example:
-Compared against `proto/trogonai/session/sessions/v1alpha1/` and ADR#0035 on 2026-08-04. +Compared against `proto/trogonai/session/sessions/v1alpha1/` and [ADR#0035](../../../../adr/0035-session-store-decider-aggregate.md) on 2026-08-04.Also applies to: 325-327, 551-555, 619-623
🤖 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 `@docs/research/session-store/products/zed/vs-session-events.md` at line 6, Replace every bare ADR reference in the session-events documentation, including ADR#0026 and ADR#0035, with relative Markdown links to the corresponding ADR documents. Update all listed occurrences throughout the file and ensure no unlinked ADR# references remain.Source: Pipeline failures
docs/research/session-store/products/zed/vs-session-events.md-385-390 (1)
385-390: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not describe the delete walk as effectively atomic.
Mutex<Connection>serializes callers in one process, but it does not prove transactionally atomic or crash-atomic deletion across the full walk. The document later states that a crash can leave a partially deleted subtree at lines 580-589. Replace “effectively atomic from the caller's perspective” with wording that states the mutex serialization and the missing transaction guarantee.Proposed wording
- at the cost of a single-process, single-database assumption that cannot - survive a cross-stream write ... effectively atomic from the caller's perspective. + at the cost of a single-process, single-database assumption that cannot + survive a cross-stream write ... serialized under one process mutex, but not + proven transactionally or crash atomic across the full delete walk.🤖 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 `@docs/research/session-store/products/zed/vs-session-events.md` around lines 385 - 390, Update the Zed ThreadsDatabase::delete_thread description to remove “effectively atomic from the caller's perspective.” State that Mutex<Connection> serializes callers within one process, while the missing explicit BEGIN/COMMIT means the full recursive deletion has no transaction or crash-atomicity guarantee.docs/research/session-store/products/openhands/vs-session-events.md-6-6 (1)
6-6: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReplace bare ADR references with Markdown links.
The ADR lint failure has one root cause. CI reports 335 bare ADR references. Convert every bare
ADR#...reference in these files to a relative Markdown link.
docs/research/session-store/products/openhands/vs-session-events.md#L6-L6: replace the first and all repeated bare ADR references.docs/research/session-store/products/pi/index.md#L14-L14: replace the first and all repeated bare ADR references.docs/research/session-store/products/pi/vs-session-events.md#L6-L6: replace the first and all repeated bare ADR references.Proposed link form
-Compared against `proto/...` and ADR#0035 on 2026-08-04. +Compared against `proto/...` and [ADR#0035](../../../../adr/0035-session-store-decider-aggregate.md) on 2026-08-04.🤖 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 `@docs/research/session-store/products/openhands/vs-session-events.md` at line 6, Replace every bare ADR#... reference with a relative Markdown link in docs/research/session-store/products/openhands/vs-session-events.md#L6-L6, docs/research/session-store/products/pi/index.md#L14-L14, and docs/research/session-store/products/pi/vs-session-events.md#L6-L6, including the initial and repeated references. Preserve each ADR identifier and link it to the corresponding ADR document using the repository’s relative-link convention.Source: Pipeline failures
docs/research/session-store/products/swe-agent/vs-session-events.md-6-6 (1)
6-6: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConvert bare ADR references to Markdown links.
CI fails because this file uses bare
ADR#0035andADR#0024references. Convert every occurrence, including Lines 6 and 313, to relative links. Use the exact ADR filenames fromdocs/adr/.Also applies to: 313-313
🤖 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 `@docs/research/session-store/products/swe-agent/vs-session-events.md` at line 6, Update every bare ADR#0035 and ADR#0024 reference in the document, including the occurrences near the comparison note and later referenced line, to relative Markdown links using the exact matching filenames from docs/adr/.Source: Pipeline failures
🟡 Minor comments (14)
docs/research/session-store/products/google-adk/vs-session-events.md-98-99 (1)
98-99: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not claim that contract drift is impossible.
A single
SessionProjectioncontract reduces implementation surfaces. It does not prevent an implementation from diverging from the contract. Replace this claim with a testable guarantee that requires conformance tests or equivalent validation.Also applies to: 328-334
🤖 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 `@docs/research/session-store/products/google-adk/vs-session-events.md` around lines 98 - 99, Revise the claim in the comparison table around the `SessionProjection` entry so it no longer states that documentation/implementation drift is impossible. Replace it with a testable guarantee that explicitly depends on conformance tests or equivalent validation, while preserving the point that one authoritative contract reduces implementation surfaces. Apply the same correction to the corresponding claim near the referenced second occurrence.docs/research/session-store/products/google-adk/vs-session-events.md-258-258 (1)
258-258: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the
ADR#0027link target.The target contains
tenancity, but that file does not exist. Use the existingtenancyfilename.Proposed fix
-[ADR#0027](../../../../adr/0027-decider-multi-tenancity-primitive.md) +[ADR#0027](../../../../adr/0027-decider-multi-tenancy-primitive.md)🤖 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 `@docs/research/session-store/products/google-adk/vs-session-events.md` at line 258, Update the ADR#0027 Markdown link target in the session-store documentation to reference the existing tenancy filename instead of the misspelled tenancity filename; leave the link text and surrounding content unchanged.Source: Pipeline failures
docs/research/session-store/products/amazon-q/index.md-693-698 (1)
693-698: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the orphaned sentence fragment.
The bullet ends at Line 697. Line 698 contains the stray text
the field.. Delete it or merge it into the preceding sentence.🤖 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 `@docs/research/session-store/products/amazon-q/index.md` around lines 693 - 698, Remove the orphaned trailing text “the field.” after the tangent_state bullet, leaving the preceding sentence complete and the bullet ending cleanly.docs/research/session-store/products/amazon-q/index.md-171-173 (1)
171-173: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDistinguish checkpoint metadata from shadow-Git storage.
CheckpointManageris serialized insideConversationState, so its metadata and history snapshots are persisted throughDatabase. Only the workspace checkpoint contents use the separate shadow-Git store. Revise this sentence to avoid contradicting Lines 444-476.Proposed wording
-There is no read/write interface at all for the checkpoint or delegate -subsystems through `Database` -- those are separate, file-based stores -described under Rewind/Subagents below. +There is no dedicated `Database` interface for the checkpoint or delegate +subsystems. Checkpoint metadata and history snapshots are persisted through +`ConversationState`; workspace checkpoint contents and delegate bookkeeping +use separate file-based stores.🤖 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 `@docs/research/session-store/products/amazon-q/index.md` around lines 171 - 173, Revise the sentence in the database-storage description to distinguish `CheckpointManager` metadata and history snapshots, which are serialized in `ConversationState` and persisted through `Database`, from workspace checkpoint contents stored in the separate shadow-Git store. Keep the separate file-based treatment for delegate subsystems accurate and avoid stating that all checkpoint data bypasses `Database`.docs/research/session-store/products/crush/vs-session-events.md-255-255 (1)
255-255: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the reversed source range.
internal/agent/agent.go:1696-1502is not a valid ascending line range. Replace it with the verifiedgetSessionMessagesrange. The nearby citation uses1692-1711.🤖 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 `@docs/research/session-store/products/crush/vs-session-events.md` at line 255, Correct the source citation in the Compacted records sentence by replacing the reversed internal/agent/agent.go range with the verified getSessionMessages range 1692-1711, leaving the surrounding text unchanged.docs/research/session-store/products/zed/index.md-499-510 (1)
499-510: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDescribe fallback persistence loss accurately.
open_fallback_dbemitslog::warn!before using an in-memory database. Threads are lost at process exit, but the loss is not silent at the implementation level. Replace “silently lost” with “lost after a warning” unless the warning is not visible to users.🤖 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 `@docs/research/session-store/products/zed/index.md` around lines 499 - 510, Update the fallback persistence description in the Crash/concurrency handling bullet to replace “silently lost” with “lost after a warning,” accurately reflecting the warning emitted by open_fallback_db before using the in-memory database.docs/research/session-store/products/openai-agents-sdk/index.md-51-58 (1)
51-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the blockquote structure.
The blank line between the two quoted paragraphs triggers Markdownlint
MD028. Prefix the blank line with>or split the content into separate blockquotes.🤖 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 `@docs/research/session-store/products/openai-agents-sdk/index.md` around lines 51 - 58, Update the blockquote containing the session description and run behavior so it does not have an unquoted blank line: prefix the separating blank line with “>” or split the text into separate blockquotes, preserving both quoted paragraphs.Source: Linters/SAST tools
docs/research/session-store/products/zed/index.md-145-149 (1)
145-149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve the
SessionIdminting contradiction.Lines 83-85 identify two
Uuid::new_v4()call sites that mintacp::SessionId. This section says the exact minting call site was not found. State that the call sites were found and only ownership or completeness is uncertain, or remove the earlier uncertainty.🤖 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 `@docs/research/session-store/products/zed/index.md` around lines 145 - 149, Update the Session ids discussion in the Zed research document to reconcile it with the identified Uuid::new_v4() call sites: state that the acp::SessionId minting sites were found, while limiting uncertainty to ownership or completeness, and remove the contradictory claim that no exact call site was found.docs/research/session-store/products/openai-agents-sdk/vs-session-events.md-42-58 (1)
42-58: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMark product-intent claims as inference.
The cited code establishes two different storage models. It does not establish that OpenAI made a deliberate design choice or that the difference is not a tooling-age artifact. Mark those sentences
[inference]or state only the observed divergence.🤖 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 `@docs/research/session-store/products/openai-agents-sdk/vs-session-events.md` around lines 42 - 58, Revise the comparison around the Agents SDK Session protocol to distinguish observed implementation facts from inferred product intent. Remove or mark as [inference] the claims that the divergence is deliberate, not caused by tooling age, or reflects specific vendor optimization goals; retain the cited evidence that Codex uses append-only markers while the SDK backends perform destructive deletion and replacement.docs/research/session-store/products/openai-agents-sdk/index.md-100-103 (1)
100-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCall the persisted history a list, not a set.
The backends can store duplicate items. Deduplication is Runner-side and content-based. “Ordered set” implies uniqueness that the storage contract does not provide. Use “ordered list” or “ordered sequence.”
🤖 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 `@docs/research/session-store/products/openai-agents-sdk/index.md` around lines 100 - 103, Update the durable session description in the referenced paragraph to call the conversation-item collection an “ordered list” or “ordered sequence” instead of an “ordered set,” preserving the statement that backends may store duplicate items and avoiding any implication of storage-level uniqueness.docs/research/session-store/products/openai-agents-sdk/index.md-61-98 (1)
61-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReconcile the backend count with the list.
The current list names eight concrete storage backends, but then says the SDK ships “nine concrete backends” while also listing
OpenAIResponsesCompactionSessionandEncryptedSessionas decorators that wrap existingSessionimplementations. Add the missing backend or change the count to eight.🤖 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 `@docs/research/session-store/products/openai-agents-sdk/index.md` around lines 61 - 98, Reconcile the backend count in the listed storage implementations: either add the omitted concrete backend to match the claim of nine, or change “at least nine concrete backends” to “eight concrete backends.” Keep OpenAIResponsesCompactionSession and EncryptedSession identified as decorators rather than counting them as storage backends.docs/research/session-store/products/zed/vs-session-events.md-43-43 (1)
43-43: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse an H2 for
Resolving the maturity tension.Line 43 jumps from the document-level H1 to an H3 and triggers markdownlint MD001. Change
###to##.🤖 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 `@docs/research/session-store/products/zed/vs-session-events.md` at line 43, Change the heading `Resolving the maturity tension` from H3 to H2 so it follows the document-level H1 and satisfies markdownlint MD001.Source: Linters/SAST tools
docs/research/session-store/products/pi/index.md-769-771 (1)
769-771: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDescribe the harness version gate precisely.
The text says the harness “has no version concept,” but Lines 823-838 show that
parseHeader()rejects headers unlessversion === 3. This is a strict version gate without migration support. Change the wording to avoid implying that the harness accepts unversioned data.🤖 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 `@docs/research/session-store/products/pi/index.md` around lines 769 - 771, Update the format-versioning description near the harness discussion to state that the harness enforces a hard-coded version 3 header check via parseHeader(), with no migration support, rather than implying it accepts unversioned data or has no version gate.docs/research/session-store/products/pi/index.md-1246-1259 (1)
1246-1259: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScope Pi’s concurrency claims by backend.
The dossier documents SQLite transactions, WAL, and
busy_timeout, so the statements below are too broad if they include every Pi implementation.
docs/research/session-store/products/pi/index.md#L1246-L1259: limit the no-coordination statement to the CLI and JSONL/in-memory backends; note SQLite transaction and locking behavior separately.docs/research/session-store/products/pi/vs-session-events.md#L88-L89: compareAt(current_position)with the CLI/JSONL backends, and state that SQLite lacks this expected-position contract rather than having no coordination.🤖 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 `@docs/research/session-store/products/pi/index.md` around lines 1246 - 1259, Scope the concurrency comparison to distinguish Pi’s CLI and JSONL/in-memory backends from SQLite: revise the no-coordination claim in docs/research/session-store/products/pi/index.md lines 1246-1259, and separately acknowledge SQLite transactions, WAL, busy_timeout, and locking while noting it lacks the expected-position append contract. In docs/research/session-store/products/pi/vs-session-events.md lines 88-89, compare At(current_position) specifically with the CLI/JSONL backends and state that SQLite lacks this contract rather than lacking coordination entirely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56c25821-cffd-4c42-b1e7-8cf8803c7726
📒 Files selected for processing (53)
docs/research/acp/products/gemini-cli.mddocs/research/acp/products/opencode.mddocs/research/session-store/RESEARCH_PROMPT.mddocs/research/session-store/RESEARCH_PROMPT_COMPARISON.mddocs/research/session-store/backlog.mddocs/research/session-store/index.mddocs/research/session-store/products/aider/index.mddocs/research/session-store/products/aider/vs-session-events.mddocs/research/session-store/products/amazon-q/index.mddocs/research/session-store/products/amazon-q/vs-session-events.mddocs/research/session-store/products/aws-strands/index.mddocs/research/session-store/products/aws-strands/vs-session-events.mddocs/research/session-store/products/claude-agent-sdk/index.mddocs/research/session-store/products/claude-agent-sdk/session-types.mddocs/research/session-store/products/cline/index.mddocs/research/session-store/products/cline/vs-session-events.mddocs/research/session-store/products/codex-cli/index.mddocs/research/session-store/products/continue/index.mddocs/research/session-store/products/continue/vs-session-events.mddocs/research/session-store/products/crush/index.mddocs/research/session-store/products/crush/vs-session-events.mddocs/research/session-store/products/fx/index.mddocs/research/session-store/products/fx/session-detail-json-reference.mddocs/research/session-store/products/fx/vs-session-events.mddocs/research/session-store/products/gemini-cli/index.mddocs/research/session-store/products/google-adk/index.mddocs/research/session-store/products/google-adk/vs-session-events.mddocs/research/session-store/products/goose/index.mddocs/research/session-store/products/grok-build/index.mddocs/research/session-store/products/hermes-agent/index.mddocs/research/session-store/products/kilo-code/index.mddocs/research/session-store/products/langgraph/index.mddocs/research/session-store/products/letta/index.mddocs/research/session-store/products/letta/vs-session-events.mddocs/research/session-store/products/mastra/index.mddocs/research/session-store/products/mastra/vs-session-events.mddocs/research/session-store/products/openai-agents-sdk/index.mddocs/research/session-store/products/openai-agents-sdk/vs-session-events.mddocs/research/session-store/products/opencode/index.mddocs/research/session-store/products/openhands/index.mddocs/research/session-store/products/openhands/vs-session-events.mddocs/research/session-store/products/pi/index.mddocs/research/session-store/products/pi/vs-session-events.mddocs/research/session-store/products/qwen-code/index.mddocs/research/session-store/products/roo-code/index.mddocs/research/session-store/products/swe-agent/index.mddocs/research/session-store/products/swe-agent/vs-session-events.mddocs/research/session-store/products/t3code/index.mddocs/research/session-store/products/void/index.mddocs/research/session-store/products/void/vs-session-events.mddocs/research/session-store/products/zed/index.mddocs/research/session-store/products/zed/vs-session-events.mddocs/research/session-store/synthesis.md
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a1b2432. Configure here.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…-side gap Findings that only ever existed as conversation are findings we will re-derive from scratch, so the ones the sixteen comparisons produced collectively, rather than one product at a time, need somewhere to live that is not a chat log. The provider side of the message payload turns out to be unstudied, which is the one gap that bears on whether the schema's typed content arms are the right ones. Naming it as a queued stage keeps it from reading as covered by the per-product work that surrounds it. Two dossiers document an entry's envelope and never open the payload inside it. Nothing they say is wrong, which is why no citation check could have caught it. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…avigable Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…e the history A command whose state read is none cannot reject a join at append time, so claiming decide enforces one leaves the invariant with no owner at all; the fold is where these facts were always going to be reconciled. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
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 (11)
docs/research/session-store/products/mastra/vs-session-events.md (1)
174-177: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTest canonical stream order, not
SessionOrdinalbefore folding.The recommendation asks raw event readers to sort by fold-derived
SessionOrdinal. The document states thatSessionOrdinalis created during the canonical fold and is not read from JetStream metadata. Raw events cannot be ordered by a value that does not exist before folding. Test that the fold consumes canonical stream order and that derived projections preserve the assigned ordinal. Do not add a query-timeORDER BY SessionOrdinal.🤖 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 `@docs/research/session-store/products/mastra/vs-session-events.md` around lines 174 - 177, The proposed repo-level check incorrectly requires raw event readers to sort by fold-derived SessionOrdinal before folding, even though that value is created only during the canonical fold. Revise the change to test that the fold consumes canonical stream order and that derived projections preserve the assigned SessionOrdinal; do not add query-time ordering or independent sort keys for raw events.docs/research/session-store/products/crush/vs-session-events.md (1)
445-450: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not present cascade as an active guarantee before the sweep exists.
Line 445 says the default policy actively terminates orphaned children. Lines 446-450 state that the orphan-closure sweep is not built or verified.
CascadePolicydefines intended behavior, but it does not execute the reconciler. Describe this as a design target until the sweep is deployed and tested. Otherwise, operators can assume orphan closure is available when orphaned sessions can still persist.🤖 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 `@docs/research/session-store/products/crush/vs-session-events.md` around lines 445 - 450, Update the documentation’s description of the default cascade policy to identify orphan-child termination as a design target, not an active guarantee. Clarify that CascadePolicy only defines the intended behavior and that enforcement depends on the orphan-closure sweep being implemented, deployed, and verified; preserve the existing discussion of the sweep’s current absence.docs/research/session-store/products/letta/vs-session-events.md (1)
183-188: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not allow OCC to make a derived context pointer authoritative.
These lines require model-visible context to remain a pure fold, but then allow a directly writable current-window field when it has OCC. OCC protects concurrent writes. It does not prevent the field from diverging from the event log. Keep the prohibition absolute: a cache may be advisory and rebuildable, but it must not become an authoritative write path.
🤖 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 `@docs/research/session-store/products/letta/vs-session-events.md` around lines 183 - 188, Update the ADR#0035 decision 8 or Non-Goal guidance referenced by the session-store documentation to prohibit any directly writable or authoritative derived context pointer, even when protected by OCC. State that model-visible context remains a pure event-log fold; caches may only be advisory and rebuildable, while all invariant-bearing transitions continue using the existing OCC discipline.docs/research/session-store/products/swe-agent/vs-session-events.md (2)
129-137: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not classify
SessionHiddenas a terminal outcome.This paragraph groups
SessionHiddenwith terminal events. Lines 324-327 define it as a visibility tombstone. Keep it out of terminal-outcome claims and do not state that all four events share a typed reason enum.🤖 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 `@docs/research/session-store/products/swe-agent/vs-session-events.md` around lines 129 - 137, Update the paragraph describing terminal markers to exclude SessionHidden, treating it only as a visibility tombstone. Limit terminal-outcome claims and typed-reason-enum claims to SessionClosed, SessionCancelled, and SessionFailed.
345-350: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNarrow the retention conclusion.
The absence of automatic cleanup in SWE-agent does not validate our keep-forever policy. It shows that this benchmark harness has no deliberate retention contract. Rephrase this as neutral evidence or as evidence that unbounded storage is a default when retention is unspecified.
🤖 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 `@docs/research/session-store/products/swe-agent/vs-session-events.md` around lines 345 - 350, Revise the retention conclusion in the discussion of decision 7 to avoid treating SWE-agent’s lack of automatic cleanup as validation of a keep-forever policy. Describe it as neutral evidence that the harness has no deliberate retention contract, or that unbounded storage is the default when retention is unspecified.docs/research/session-store/products/void/vs-session-events.md (1)
110-112: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRemove quotation marks from the paraphrase.
The text says it is paraphrasing the dossier, but it presents the paraphrase as a direct quote. The stage-one prompt now requires quoted text to match the source exactly. Copy the source wording exactly, or remove the quotation marks.
🤖 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 `@docs/research/session-store/products/void/vs-session-events.md` around lines 110 - 112, In the dossier conclusion sentence, remove the quotation marks around the paraphrased “non-destructive rewind” wording, preserving the existing attribution and link to “What this implies for our Session Store (our inference).”docs/research/session-store/index.md (1)
108-113: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign the nested-directory claim with the product list.
Lines 108-113 say every product has at least two artifacts. The list shows only
index.mdfor Codex CLI, Gemini CLI, Goose, Grok Build, Hermes, LangGraph, OpenCode, and T3 Code. Add the missing artifact links, or change the claim to cover only products with multiple artifacts.🤖 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 `@docs/research/session-store/index.md` around lines 108 - 113, Update the documentation around the nested product-directory description to match the actual product listings: either add links for the missing second artifacts for Codex CLI, Gemini CLI, Goose, Grok Build, Hermes, LangGraph, OpenCode, and T3 Code, or revise the claim so it only states that products with multiple artifacts use nested directories.docs/research/session-store/products/pi/vs-session-events.md (2)
24-31: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse full repository-relative paths for first citations.
The updated prompt permits a basename only after a full, unambiguous path is established. These passages introduce abbreviated filenames before that point. Mechanical verification can therefore resolve the wrong file or fail when sibling files share a basename.
docs/research/session-store/products/pi/vs-session-events.md#L24-L31: qualifyrepository.tsandjsonl-repo.ts.docs/research/session-store/products/pi/vs-session-events.md#L36-L42: qualify the firstsession-selector.tsreference.docs/research/session-store/products/pi/vs-session-events.md#L86-L94: qualifyworkspace.proto.docs/research/session-store/products/void/vs-session-events.md#L74-L89: qualifyworkspace.proto.docs/research/session-store/products/zed/vs-session-events.md#L141-L150: qualifyexecution_attempt_started.proto.🤖 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 `@docs/research/session-store/products/pi/vs-session-events.md` around lines 24 - 31, Qualify abbreviated repository citations with full repository-relative paths. Update docs/research/session-store/products/pi/vs-session-events.md lines 24-31 to qualify repository.ts and jsonl-repo.ts, lines 36-42 to qualify the first session-selector.ts reference, and lines 86-94 to qualify workspace.proto; make the same workspace.proto qualification in docs/research/session-store/products/void/vs-session-events.md lines 74-89 and qualify execution_attempt_started.proto in docs/research/session-store/products/zed/vs-session-events.md lines 141-150.
74-80: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not model an unused session as
SessionHidden.
SessionHiddenis a visibility tombstone with separate retention and cascade semantics. ReusingSESSION_HIDDEN_REASON_UNSPECIFIEDor adding an “unused” reason would make an unused session appear hidden and apply the wrong lifecycle rules. Define a separate typed fact, or state thatSessionStartedalone is sufficient.🤖 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 `@docs/research/session-store/products/pi/vs-session-events.md` around lines 74 - 80, The session-without-a-real-turn decision must not reuse SessionHidden or SESSION_HIDDEN_REASON_UNSPECIFIED, since those represent visibility and lifecycle semantics. Update the decision section to either explicitly declare SessionStarted alone sufficient or define a separate typed unused-session fact; preserve the documented additive-versus-documentation-only outcome.docs/research/session-store/products/zed/vs-session-events.md (2)
110-118: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDescribe the whole-document stale-write trade-off accurately.
A whole-document overwrite can still lose a concurrent, earlier update. SQLite WAL mode and
busy_timeoutserialize or delay lock contention; they are not application-level compare-and-swap. Reword this so the document says Zed’s write unit does not guard against stale reads.🤖 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 `@docs/research/session-store/products/zed/vs-session-events.md` around lines 110 - 118, Revise the whole-document overwrite discussion in the Zed comparison so it explicitly acknowledges that a stale read can overwrite an earlier concurrent update. Clarify that SQLite WAL mode and the busy-timeout only serialize or delay lock contention, rather than providing application-level compare-and-swap or stale-write protection, and preserve the contrast with the per-command WRITE_PRECONDITION classification.
384-401: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not describe Zed’s cascade as effectively atomic.
The
Mutex<Connection>only serializes one process’s walk. The delete statements still run autocommit by default, so a crash mid-walk can leave a partial subtree. Replace “effectively atomic from the caller's perspective” with “serialized but not crash-atomic,” matching lines 581-588.🤖 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 `@docs/research/session-store/products/zed/vs-session-events.md` around lines 384 - 401, Update the Zed cascade comparison around ThreadsDatabase::delete_thread to remove the claim that it is effectively atomic; describe the Mutex<Connection> walk as “serialized but not crash-atomic,” consistent with the documented autocommit behavior and the discussion at lines 581-588.
🤖 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 `@proto/trogonai/session/sessions/v1alpha1/checkpoint.proto`:
- Around line 41-52: Do not add capture_attestation_ref,
capture_attestation_digest, or effective_history_digest as LEGACY_REQUIRED
fields to the released v1alpha1 Checkpoint contract. Move them to a new
compatible message or package version, or implement an explicit legacy
validation path that permits older checkpoints without fields 9–11 while
requiring them for newly produced checkpoints.
---
Outside diff comments:
In `@docs/research/session-store/index.md`:
- Around line 108-113: Update the documentation around the nested
product-directory description to match the actual product listings: either add
links for the missing second artifacts for Codex CLI, Gemini CLI, Goose, Grok
Build, Hermes, LangGraph, OpenCode, and T3 Code, or revise the claim so it only
states that products with multiple artifacts use nested directories.
In `@docs/research/session-store/products/crush/vs-session-events.md`:
- Around line 445-450: Update the documentation’s description of the default
cascade policy to identify orphan-child termination as a design target, not an
active guarantee. Clarify that CascadePolicy only defines the intended behavior
and that enforcement depends on the orphan-closure sweep being implemented,
deployed, and verified; preserve the existing discussion of the sweep’s current
absence.
In `@docs/research/session-store/products/letta/vs-session-events.md`:
- Around line 183-188: Update the ADR#0035 decision 8 or Non-Goal guidance
referenced by the session-store documentation to prohibit any directly writable
or authoritative derived context pointer, even when protected by OCC. State that
model-visible context remains a pure event-log fold; caches may only be advisory
and rebuildable, while all invariant-bearing transitions continue using the
existing OCC discipline.
In `@docs/research/session-store/products/mastra/vs-session-events.md`:
- Around line 174-177: The proposed repo-level check incorrectly requires raw
event readers to sort by fold-derived SessionOrdinal before folding, even though
that value is created only during the canonical fold. Revise the change to test
that the fold consumes canonical stream order and that derived projections
preserve the assigned SessionOrdinal; do not add query-time ordering or
independent sort keys for raw events.
In `@docs/research/session-store/products/pi/vs-session-events.md`:
- Around line 24-31: Qualify abbreviated repository citations with full
repository-relative paths. Update
docs/research/session-store/products/pi/vs-session-events.md lines 24-31 to
qualify repository.ts and jsonl-repo.ts, lines 36-42 to qualify the first
session-selector.ts reference, and lines 86-94 to qualify workspace.proto; make
the same workspace.proto qualification in
docs/research/session-store/products/void/vs-session-events.md lines 74-89 and
qualify execution_attempt_started.proto in
docs/research/session-store/products/zed/vs-session-events.md lines 141-150.
- Around line 74-80: The session-without-a-real-turn decision must not reuse
SessionHidden or SESSION_HIDDEN_REASON_UNSPECIFIED, since those represent
visibility and lifecycle semantics. Update the decision section to either
explicitly declare SessionStarted alone sufficient or define a separate typed
unused-session fact; preserve the documented additive-versus-documentation-only
outcome.
In `@docs/research/session-store/products/swe-agent/vs-session-events.md`:
- Around line 129-137: Update the paragraph describing terminal markers to
exclude SessionHidden, treating it only as a visibility tombstone. Limit
terminal-outcome claims and typed-reason-enum claims to SessionClosed,
SessionCancelled, and SessionFailed.
- Around line 345-350: Revise the retention conclusion in the discussion of
decision 7 to avoid treating SWE-agent’s lack of automatic cleanup as validation
of a keep-forever policy. Describe it as neutral evidence that the harness has
no deliberate retention contract, or that unbounded storage is the default when
retention is unspecified.
In `@docs/research/session-store/products/void/vs-session-events.md`:
- Around line 110-112: In the dossier conclusion sentence, remove the quotation
marks around the paraphrased “non-destructive rewind” wording, preserving the
existing attribution and link to “What this implies for our Session Store (our
inference).”
In `@docs/research/session-store/products/zed/vs-session-events.md`:
- Around line 110-118: Revise the whole-document overwrite discussion in the Zed
comparison so it explicitly acknowledges that a stale read can overwrite an
earlier concurrent update. Clarify that SQLite WAL mode and the busy-timeout
only serialize or delay lock contention, rather than providing application-level
compare-and-swap or stale-write protection, and preserve the contrast with the
per-command WRITE_PRECONDITION classification.
- Around line 384-401: Update the Zed cascade comparison around
ThreadsDatabase::delete_thread to remove the claim that it is effectively
atomic; describe the Mutex<Connection> walk as “serialized but not
crash-atomic,” consistent with the documented autocommit behavior and the
discussion at lines 581-588.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 46fda845-bbef-4b37-8542-7cef243ba43a
⛔ Files ignored due to path filters (2)
rsworkspace/crates/platform/trogonai-proto/src/gen/trogonai.session.sessions.v1alpha1.checkpoint.__view.rsis excluded by!**/gen/**rsworkspace/crates/platform/trogonai-proto/src/gen/trogonai.session.sessions.v1alpha1.checkpoint.rsis excluded by!**/gen/**
📒 Files selected for processing (27)
.config/mise/tasks/github-actions/lint-adr-refsdocs/adr/0031-agent-implementation-and-session-plan.mddocs/adr/0035-session-store-decider-aggregate.mddocs/research/session-store/RESEARCH_PROMPT.mddocs/research/session-store/RESEARCH_PROMPT_COMPARISON.mddocs/research/session-store/backlog.mddocs/research/session-store/index.mddocs/research/session-store/products/aider/vs-session-events.mddocs/research/session-store/products/amazon-q/vs-session-events.mddocs/research/session-store/products/aws-strands/vs-session-events.mddocs/research/session-store/products/cline/vs-session-events.mddocs/research/session-store/products/continue/vs-session-events.mddocs/research/session-store/products/crush/vs-session-events.mddocs/research/session-store/products/google-adk/vs-session-events.mddocs/research/session-store/products/letta/vs-session-events.mddocs/research/session-store/products/mastra/vs-session-events.mddocs/research/session-store/products/openai-agents-sdk/vs-session-events.mddocs/research/session-store/products/openhands/vs-session-events.mddocs/research/session-store/products/pi/vs-session-events.mddocs/research/session-store/products/swe-agent/vs-session-events.mddocs/research/session-store/products/void/vs-session-events.mddocs/research/session-store/products/zed/vs-session-events.mddocs/research/session-store/synthesis.mdproto/trogonai/session/sessions/v1alpha1/checkpoint.protorsworkspace/crates/platform/trogonai-proto/src/session/sessions/codec/tests.rsrsworkspace/crates/platform/trogonai-proto/src/session/sessions/validate.rsrsworkspace/crates/platform/trogonai-proto/src/session/sessions/validate/tests.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/research/session-store/products/openhands/vs-session-events.md
- rsworkspace/crates/platform/trogonai-proto/src/session/sessions/validate/tests.rs
- docs/research/session-store/RESEARCH_PROMPT_COMPARISON.md
- docs/research/session-store/products/continue/vs-session-events.md
- docs/research/session-store/products/cline/vs-session-events.md
- docs/research/session-store/products/aws-strands/vs-session-events.md
- docs/research/session-store/products/openai-agents-sdk/vs-session-events.md
- docs/adr/0035-session-store-decider-aggregate.md
…d field Reviewers keep reading the version suffix as the licence and flagging these additions as breaking; the licence is actually the absence of a deployed producer, and that is the fact a future reviewer needs stated, since buf breaking cannot see it. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/adr/0035-session-store-decider-aggregate.md`:
- Around line 678-698: Clarify the checkpoint evidence contract so first-wins
selection does not reject and discard later conflicting evidence before append.
Define conflicting evidence as separately retained fold data with audit
visibility, or specify the separate audit sink and key it by complete evidence;
update both the ownership summary and the corresponding checkpoint rules to
match.
- Around line 128-138: The ADR and proto comment currently define conflicting
compatibility gates for LEGACY_REQUIRED fields. Align the guidance to use one
rule, preferably the first deployed producer described in the ADR: retain that
producer-based rule in docs/adr/0035-session-store-decider-aggregate.md lines
128-138, and remove or revise the package-promotion condition in
proto/trogonai/session/sessions/v1alpha1/events.proto lines 54-61 to match;
update both sites as needed.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 235d1139-fc58-4a39-8c5c-0425a5710697
📒 Files selected for processing (2)
docs/adr/0035-session-store-decider-aggregate.mdproto/trogonai/session/sessions/v1alpha1/events.proto
Comment on lines
+128
to
+138
| Within `v1alpha1`, a field may still be added as `LEGACY_REQUIRED`, and the | ||
| reason it is admissible is narrower than the version suffix: no deployed producer | ||
| has written these events yet. A new required field breaks by having a current | ||
| validator reject already-stored bytes, and there are no stored bytes until a | ||
| producer ships. The gate is therefore the first deployed producer, not the | ||
| promotion to `v1` -- a producer shipping on `v1alpha1` would close this window | ||
| early, and from then on a new required field needs a new package version. Note | ||
| that `buf breaking` under `WIRE_JSON` passes either way, since it compares fields | ||
| present on both sides and a field new to one side is not among them; the check | ||
| here is a review obligation, not a mechanical one. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use one LEGACY_REQUIRED compatibility gate.
The ADR uses the first deployed producer as the gate. The proto comment also uses package promotion. Align both files so required-field evolution cannot follow conflicting rules.
docs/adr/0035-session-store-decider-aggregate.md#L128-L138: keep the producer-based rule, or update it if promotion is intentionally a gate.proto/trogonai/session/sessions/v1alpha1/events.proto#L54-L61: remove or retain the promotion condition to match the ADR.
📍 Affects 2 files
docs/adr/0035-session-store-decider-aggregate.md#L128-L138(this comment)proto/trogonai/session/sessions/v1alpha1/events.proto#L54-L61
🤖 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 `@docs/adr/0035-session-store-decider-aggregate.md` around lines 128 - 138, The
ADR and proto comment currently define conflicting compatibility gates for
LEGACY_REQUIRED fields. Align the guidance to use one rule, preferably the first
deployed producer described in the ADR: retain that producer-based rule in
docs/adr/0035-session-store-decider-aggregate.md lines 128-138, and remove or
revise the package-promotion condition in
proto/trogonai/session/sessions/v1alpha1/events.proto lines 54-61 to match;
update both sites as needed.
Source: Learnings
Comment on lines
+678
to
+698
| `decide` and `evolve` own every history-dependent relationship, and which of the | ||
| two owns a given one follows from whether its command reads state at all. | ||
| `decide` owns the relationships whose command declares a state read in the | ||
| command matrix: in-session ordinal existence and compaction ordering, exact | ||
| attempt lineage, first-wins checkpoint evidence selection, complete | ||
| restored-checkpoint equality with that evidence, continued effectiveness of | ||
| `covers_through` after rewind, and equality with the stored Session plan. Those | ||
| are checked before the append and rejected if they fail. | ||
| `evolve` owns the relationships carried by commuting facts whose state read is | ||
| `none` -- the assistant start/completion id and model joins, and the tool | ||
| lifecycle joins. Those append under `Any` with nothing to check against, so they | ||
| are fold rules rather than append-time rejections: an unmatched or disagreeing | ||
| fact lands on the log and is surfaced by a projection flag, exactly as the `Any` | ||
| fold rules above prescribe. | ||
| This split prevents a local payload validator from claiming facts that only | ||
| the command context or folded history can prove, and it keeps the command | ||
| matrix honest: a command whose state read is `none` cannot enforce a join, so | ||
| the matrix states its join as the fold rule it is. | ||
|
|
||
| Every unset oneof, unspecified enum, and malformed same-event shape above is | ||
| rejected before append, never persisted and reconciled later. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define how conflicting checkpoint evidence is retained.
Lines 682-685 place first-wins checkpoint selection among checks that reject before append. The checkpoint contract also requires complete-evidence identity and audit visibility for conflicting evidence. Rejecting the later evidence before append would discard that conflict. Define first-wins as fold selection over separately retained evidence, or document the separate audit sink and key it by the complete evidence.
Also applies to: 1054-1062
🤖 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 `@docs/adr/0035-session-store-decider-aggregate.md` around lines 678 - 698,
Clarify the checkpoint evidence contract so first-wins selection does not reject
and discard later conflicting evidence before append. Define conflicting
evidence as separately retained fold data with audit visibility, or specify the
separate audit sink and key it by complete evidence; update both the ownership
summary and the corresponding checkpoint rules to match.
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
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.