[noxsystems]

Review size: this slice adds 609 changed lines on top of #1919: 171 module, 252 unit tests, 168 tests against a real SessionManager session file, 18 docs. GitHub's Files tab shows the cumulative diff with #1918/#1919 until they merge; review only commit 4d56f74b5. It is over the 400-line budget on purpose: the only cohesive cut (module + unit tests ≈ 440, SDK test ≈ 170) still leaves the first part over budget and separates the module from the evidence that it works on Pi's real JSONL. If you prefer that split anyway, it is a one-minute change.

Summary

Part of #1064, slice 3b-i (persistence). Supplies the corroboration that replaySessionProfileBranch (#1919) requires but cannot establish: whether the selected record on the active branch is actually on disk.

• readSessionProfileDisk(source, options): stateless, synchronous. Takes the session id, session file path and public active branch; selects the newest profile-family entry on the branch, excluding caller-supplied knownFailedEntryIds; parses the JSONL file; admits the entry only when the disk record is identical (isDeepStrictEqual over a JSON snapshot, so getters and mutable references cannot leak in).
• Result: the decoder result plus entryIndex and lineNumber, or indeterminate with one closed reason (missing-source, invalid-candidate-metadata, unreadable-file, source-changed, invalid-json, invalid-record, session-header-mismatch, duplicate-id, missing-header, selected-record-missing, selected-record-mismatch, unserializable-source). Reasons never carry record contents or exception messages.
• Conservative by design: no writes, no fallback policy, no cache, no ancestry repair, no fsync. A missing or unreadable file never restores a profile found only in memory. Source identity is re-checked after the file read; any change is source-changed.
• Still no consumer wiring: Enter, startup and routing do not change.

This is PR 3 of the chain.

Issue

Part of #1064

PR type

• New feature (type:feature)

Changes

Commit	Change
4d56f74b5	lib/session-profile-disk-reader.ts, tests/session-profile-disk-reader.test.ts, tests/session-profile-disk-reader-pi.test.ts, Disk corroboration section in docs/session-profile-format.md.

Test plan

Verified on top of #1919, main 9782d26df, Node 25.2.1:

• tests/session-profile-disk-reader.test.ts: 29 pass, 0 fail (every indeterminate reason, failed-ID exclusion, source-changed detection, hostile getters, duplicate and header checks).
• tests/session-profile-disk-reader-pi.test.ts: 4 pass, 0 fail, against a real SessionManager from @earendil-works/pi-coding-agent writing to a temp directory.
• tests/session-profile-persistence.test.ts + agent-profiles + model-routing-authority: 77 pass.
• node scripts/check-types.mjs: 186 recorded diagnostics, no regressions.
• Native review (RDD): not run on this slice.

Review follow-ups (non-blocking)

• The reader scans for the newest profile-family entry itself instead of calling replaySessionProfileBranch (feat(profiles): add session profile encoder and active-branch replay #1919), because it needs the raw branch index and the failed-ID exclusion before decoding. The "newest family entry is terminal" rule therefore lives in two places; happy to fold one into the other if you'd rather.

Chain Context

Field	Value
Chain	#1064 slice 3b-i: session profile persistence
Tracker PR	Not needed
Position	3
Base	main (fork PRs cannot stack bases; rebased as #1918/#1919 land)
Depends on	#1919
Follow-up	append controller, authority publication, Enter persistence wiring around #1824's selection path
Review budget	609 changed lines, of which 420 are tests and 18 docs.

main
 └─ #1918 decoder: record format, readSessionProfileEntry
   └─ #1919 encoder + replaySessionProfileBranch
     └─ PR 3 readSessionProfileDisk: on-disk corroboration        📍 this PR
       └─ (later) append controller, authority, Enter persistence wiring

Summary by CodeRabbit

• New Features

  • Added read-only session profile verification against on-disk records.
  • Reports bound, cleared, invalid, unsupported, absent, or indeterminate profile states with relevant branch and line information.
  • Supports recovery from malformed log lines while detecting mismatches, duplicates, unreadable files, and failed entries.
  • Added session profile encoding, validation, and active-branch replay support.

• Documentation

  • Documented disk corroboration behavior, validation outcomes, line numbering, malformed records, and BOM handling.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 94805e76-caae-4c3b-82b5-a5cb06b5dc9a

📥 Commits

Reviewing files that changed from the base of the PR and between f08be2b and 339ca4a.

📒 Files selected for processing: 5

• docs/session-profile-format.md
• lib/session-profile-disk-reader.ts
• lib/session-profile-persistence.ts
• tests/session-profile-disk-reader-pi.test.ts
• tests/session-profile-disk-reader.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Adds a v1 session-profile codec and branch replay helper. Adds a read-only disk reader that checks the selected profile entry against session JSONL records. Adds unit and integration tests and documents the payload, replay, and disk-reader contracts.

Changes: Session Profile Persistence and Disk Reader

Layer / File(s)	Summary
Profile codec and branch replay lib/session-profile-persistence.ts, tests/session-profile-persistence.test.ts	Adds v1 bind and clear payload constructors, validation and normalization, entry classification, and reverse-order replay that returns the newest non-absent profile-family result. Tests cover payloads, validation, defensive snapshots, and replay ordering.
Disk corroboration and source validation lib/session-profile-disk-reader.ts, tests/session-profile-disk-reader*.test.ts, docs/session-profile-format.md	Adds a reader that matches the latest eligible branch entry to a session JSONL record, or returns absent or indeterminate. Tests cover selection, malformed or mismatched records, source changes, and persistence behavior. The documentation describes the payload, replay, and disk-reader contracts.

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant SessionProfileSource
  participant readSessionProfileDisk
  participant SessionJSONL
  SessionProfileSource->>readSessionProfileDisk: Provide session ID, file path, and branch
  readSessionProfileDisk->>SessionJSONL: Read session records
  SessionJSONL-->>readSessionProfileDisk: Return JSONL contents
  readSessionProfileDisk->>SessionProfileSource: Recheck captured source state

Loading

Merge Risk: ⚪ Minimal · up to 339ca

No concrete issue remains that would prevent merging after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 5 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: adding a session profile disk corroboration reader.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 5 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[barbatdev]

Good seam, and the corroboration design is exactly the fail-closed shape we agreed on: deep-equal over a detached JSON snapshot, failed ids never admitted even when present on disk, all twelve reasons reachable without leaking contents, and an unreadable file never restores a memory-only profile. An independent adversarial QA sweep (33 probes: 100k-entry files, deep nesting, duplicate ids across lineages, source-changed windows, unserializable sources) found zero deviations, linear performance, and no escaping throws. That part I fully endorse.

Three things block this one for me:

1. The scan aborts on the first malformed line, but Pi's own parser skips malformed lines. A file Pi opens fine can end up permanently indeterminate here, and that blocks append-controller recovery in #1948, which is the whole point of this slice. Skipping malformed lines as session-manager does, with a test for that exact scenario, would close it.
2. The carried test debt from #1918/#1919 is still open on the new seam. Non-string customType, non-string kind and empty agent name remain untested anywhere, and reason is typed as a bare string, so #1948 cannot exhaustively match on it. Type it as the twelve-literal union and add the missing branches while you are in there.
3. The family prefix is re-derived with slice(lastIndexOf) instead of an exported constant. If SESSION_PROFILE_CUSTOM_TYPE ever loses its slash, every custom entry becomes a candidate. Export the constant and share it.

Two doc-level notes from the same sweep, non-blocking: BOM-prefixed files fail closed as invalid-json (fine if Pi never emits one, worth a line in the format doc), and missing-header is only reachable on an empty or all-blank file, which is internally consistent but surprising.

None of the three touch the design; they are execution. Happy to review the follow-up push quickly.

[barbatdev]

Checking in on this one, since it now gates the whole 3b-i stack: #1918 and #1919 landed on the 12th, and #1948 → #1993 all wait behind it.

The three blockers from the last review still stand:

1. Skip malformed lines the way Pi's session manager does, with a test for a file Pi opens fine but that contains one bad line.
2. Cover non-string customType, non-string kind, and the empty agent name, and type reason as the twelve-literal union so feat(profiles): add session profile append controller with failure quarantine #1948 can match on it exhaustively.
3. Export the family prefix as a shared constant instead of re-deriving it with slice(lastIndexOf).

Now that #1919 is in, a rebase should also shrink the Files tab to just this slice.

Are you still on this? If your time is tight right now, say so and we can pick up these three fixes on the branch ourselves so the stack keeps moving. Either way works, we just want to keep 3b-i from stalling.

[noxsystems]

Yes, still on it, thanks for the nudge. Working on the three blockers now (malformed-line skipping with a test, the missing type/empty-name cases plus the twelve-literal reason union, and the shared family-prefix constant), then rebasing onto main so the Files tab shrinks to just this slice. Will ping when it's pushed.

[noxsystems]

@barbatdev pushed, rebased onto main. The Files tab is now just this slice (5 files), and the fixes are a separate commit on top (339ca4a) so they're easy to review.

1. Malformed lines are skipped, matching parseSessionEntryLine in Pi's session manager. lineNumber still counts physical lines. New tests: a real SessionManager.open on a file with a torn trailing line, which Pi opens and the reader now corroborates both before and after a new append. Also a malformed line before the header, between records and after the selected one.
2. reason is now DiskProfileIndeterminateReason, a literal union. A satisfies Record<…, true> test makes typecheck reject any extra or missing literal. One heads-up: it's 11 literals, not 12. invalid-json came only from the parse catch, and skipping makes it unreachable, so I dropped it rather than ship a case feat(profiles): add session profile append controller with failure quarantine #1948 could never hit. The fail-closed outcomes stay: a malformed selected record is selected-record-missing, and an all-blank or all-malformed file is missing-header. Also added: non-string customType is ordinary (absent), and non-string kind and an empty agent name are invalid.
3. SESSION_PROFILE_FAMILY is exported from session-profile-persistence.ts and shared; the slice(lastIndexOf) is gone.

Docs: session-profile-format.md now covers the skip behavior, the BOM note (a BOM-prefixed line is skipped like any malformed one) and what missing-header means.

Checks: the session-profile tests (98), typecheck, provider-contract and runtime-harness pass. Unit-test failures are compared against main: no new ones. The binary-digest reuse test flakes on main too (2 of 4 runs).

[barbatdev]

Thanks for the quick turnaround here, the fixes land all three blockers cleanly. On the malformed lines, matching Pi's session parser with physical line numbers preserved is the shape I wanted, and testing it through a real SessionManager.open on a file with a torn trailing line is the strongest possible evidence. Corroborating both before and after a new append in that same test covers the exact scenario I was worried about.

The typed reason union is better than what I asked for. The satisfies Record<…, true> guard means typecheck rejects any drift in either direction, so the union can't rot. On the count, dropping invalid-json is the right call. It was only reachable from the parse catch and skipping makes that path unreachable, so shipping a literal #1948 could never hit would be worse than an eleven literal union. The fail-closed outcomes through selected-record-missing and missing-header preserve the contract, and both the docs and your comment here make the deviation traceable. Consider that blocker closed.

The shared SESSION_PROFILE_FAMILY constant closes the last one, no more re-deriving the family with a slice. The BOM and missing-header doc notes also cover both of my non-blocking items from the sweep. CI is green and nothing in this round touches the fail-closed design I endorsed on the 12th, so I am approving. This unblocks the 3b-i stack, and when you rebase #1948 the Enter-before-flush and append-failure boundaries we confirmed on the issue this week apply directly to your persistence-outcome handling in runProfilesPanelAction.