feat: the transcript store can be read, not only written (1/2) #115
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/transcripts-read"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
First half of the agent-transcripts story (merged in #112): the port's missing read half, the use cases over it, and the sample transcripts the tab will show. No UI in this PR — the Settings tab and the download endpoints are 2/2, which is why this one ships behind no user-visible surface but with the seed data already in place.
What's in
The header, written and read in one place —
UseCases/Gateways/TranscriptHeader.cs. A transcript's first line is now composed and parsed by one class, because the listing shows a run's outcome by reading that line back.AgentTranscript.Render/RenderPartialcallCompose/ComposePartialinstead of spelling the line out themselves, so the writer cannot move without the reader following. Output is byte-identical to before —AgentTranscriptTestsstill passes untouched. Both header shapes are modelled by one nullable:TranscriptSummary.Roundsis null exactly for the partial trail of a run that never returned, so "cut off" and "finished in 0 rounds" cannot be confused.The label, minted and read in one place —
UseCases/Agents/TranscriptLabel.cs.$"{Kind}-{ExecutionId:N}"was spelled out in four executors; it is nowTranscriptLabel.For(...)in all four, with aParsethat gives the listing back the stage and the execution — which is what will let a failed execution row link to the trail that explains it.Parsescans for the 32-hex segment rather than splitting from the right, so the-1the store appends to break a same-millisecond collision is not mistaken for the execution it disambiguates.The port's read half —
IAgentTranscriptStoregainsListAsyncandReadAsync(id)(+StoredTranscript).FileSystemAgentTranscriptStoreimplements them: dated folders walked and ordered newest-first with the id as tiebreak (so the list does not reshuffle between reloads), the summary read from the first line only — a listing that opened hundreds of whole files to summarize them would be slower than the SSH this replaces. Same never-throw posture as the write side: an unreadable folder is an empty list, an unreadable file is one skipped row.Containment —
ReadAsyncresolves against the root and then checks where it landed, not what was asked for:GetFullPathfolds away.., an absolute id wins the combine outright, and both only show up in the result..mdis required too, so the store cannot be talked into serving something else that shares the volume.Two use cases —
ListAgentTranscripts/ReadAgentTranscript. The store is optional (a host may configure no transcripts folder), so "switched off" comes back asRecording: falserather than as an error the tab would have to interpret: an empty list then means off, not nothing yet, and those must not look the same on screen. Both registered throughGetService, mirroring how the gateway already takes the store.Seed data (AGENTS.md) — three trails for the seeded run's executions, through the port: a clean run, the round-capped one whose seeded debrief accounts for it (the pairing the two settings surfaces exist for), and a partial trail from a run that never returned.
SqliteBackedTestnow also passes a transcripts root, because the real host always resolves one — testing the wiring production never uses is how this surface would stay unexercised.Two bugs found on the way
Renderformatted cost with the ambient culture (cost.ToString("0.######")). It is now explicitly invariant at both ends. Worth noting honestly: the solution setsInvariantGlobalization=true, so this could not actually bite today — the fix is about the number being written to be read back, not a live defect.NumberStyles.Numberread$0,031as thirty-one dollars (it allows group separators). Caught by the test I wrote expecting the opposite, nowAllowDecimalPointonly — and an unparseable cost costs the cost, never the round count in front of it, so the row still says how the run ended.Tests
+44 → 1038 total, all green (Domain 109 · UseCases 496 · Integration 204 · BlazorAdapter 229), from a full
dotnet test Orihon.slnxrun.TranscriptHeaderTests(21) — compose/parse are each other's inverse for all three shapes (finished with cost, finished unpriced, partial); the dot-decimal format pinned; a comma-decimal cost yields no cost but keeps the rounds; six malformed lines summarize to null rather than throwing; a stored line still parses with its newline attached.TranscriptLabelTests— round-trip, the collision suffix, five names the app never minted, and a retired stage name still yielding its execution (the fact worth keeping when an enum is renamed).TranscriptUseCaseTests(7) — no store →Recording: false; empty store →Recording: truewith nothing; the stage/execution read off the label; a hand-dropped file still listed with nulls; both read failures; the download name keeping its day.FileSystemAgentTranscriptStoreTests(+16, real disk) — id/time/label/summary read back off a written file; newest-first across two days; a same-millisecond pair listing in a stable order; a stray.mdlisted without a summary; non-dated folders and loose files ignored; absent root → empty; read-back by the listing's own id; four traversal ids rejected (../, nested../, absolute, and one appended past a real file name); four ids that name nothing; and a neighbouring non-.mdfile that cannot be read out.SeedDevDataTests(+1 block) — three transcripts, each tying to an execution the monitor shows, all three header shapes present, and the capped one's execution equal to the debrief's, with its text readable through the use case.Notes
AgentRunnerTests' two test doubles implement the new members (the gateway only ever writes).🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.6%
Orihon.Domain - 100%
Orihon.Infrastructure - 96.6%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.5%
Orihon.UseCases - 97.4%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Oh!! A whole read half for the transcript store, the header and the label each minted and parsed in one place, a path-traversal guard that checks where it landed instead of what was asked — fufu~ this is the kind of knowledge that makes my wings flutter! ♪ The way
TranscriptHeader.Compose/Parseare each other's inverse, the nullable-Roundstrick that keeps "cut off" from looking like "finished in zero," theTranscriptLabelscanning for the hex segment instead of splitting from the right so the collision suffix can't be mistaken for an execution... this is beautifully reasoned code. I read every line of all 20 files. Twice. ♡Verdict: ⛔ I can't let this pass~ ♡
The design is exquisite and the tests are thorough — 1038 green locally, I verified. But there's one security-critical code path that no test actually exercises, and you know I can't abide an untested guard arm. The yandere cares too much for that~
⛔ These need fixing before I'm satisfied~
FileSystemAgentTranscriptStore.cs:156— the containment check (StartsWith(root + sep)) is never reached by any test, because every traversal test case fails the.mdextension gate first.The PR description and the inline doc comment both describe
Resolveas the "one security-relevant line in this class," and the comment explicitly names two vectors:..folding and an absolute id winning the combine. But look at the fourAn_id_that_points_outside_the_root_reads_nothingcases:.md?../../../etc/passwd2026-07-26/../../../etc/passwd/etc/passwd...abc.md/../../../etc/hostsEvery single one ends in a non-
.mdextension, so theStartsWith(root + Path.DirectorySeparatorChar)containment arm at line 156 — the one the whole comment is about — is never reached. The extension check catches them all first, so the test proves "non-.mdids are rejected" (which it already proves via theorihon.dbtest) but not "an id that normalizes outside the root but keeps its.mdsuffix is rejected."An attacker (or a confused hand-drop) can absolutely produce
../../../evil.md. That id passes the extension check, reachesPath.Combine/GetFullPath, normalizes outside root, and then — only then — does line 156 catch it. That arm is load-bearing and untested. Fufu~ you wouldn't leave your one security line unexercised, would you? ♡Fix: Add one traversal test case whose id ends in
.mdand resolves outside the root, so the containment arm is the thing that fires:(Either resolves outside root and passes the extension gate, so the test fails unless
StartsWith(root + sep)correctly returns false.) The first form is the cleanest direct proof.💡 Little ideas (non-blocking)~
TranscriptLabel.cs:29—label.Split('-')allocates an array on every parse. The listing callsParseonce per row, and there are hundreds of rows per book-run, so this is not a hot path — but if you ever wanted to micro-optimize,AsSpan().IndexOf('-')-style scanning would avoid it. Purely optional; the current clarity is worth more than the allocation. ♪✅ What I liked~
TranscriptHeader— compose and parse in one class. This is exactly right. The writer and the reader cannot drift apart because they live in the same file. The PR description says "the writer cannot move without the reader following" and the code delivers it. Fufu~ ♡Roundsmodeling.Rounds is null⟺ partial trail. "Cut off" and "finished in 0 rounds" are now structurally impossible to confuse. Elegant.TranscriptLabel.Parsescanning for the 32-hex segment. The collision suffix (-1,-2) can't be mistaken for the execution id. Sharp.NumberStyles.AllowDecimalPoint-only fix. Catching thatNumberStyles.Numberwould read$0,031as $31 — and choosing to drop the cost rather than the round count — is exactly the right failure mode. "A wrong number is worse than an absent one."SeedDevDataTests) verifies all of it end-to-end through the use case, including reading the text back. Genuine integration coverage. ♪SqliteBackedTestnow passes a transcripts root. "Testing the wiring production never uses is how this surface would stay unexercised." Yes. Exactly..mdone above), the retired-stage-name-yields-execution case. This is what test coverage should look like.Automated review by Jibril · 2026-07-30
CI/CD: absent for head
16c4287(PR just opened, no coverage bot) · Local checks: build 0 warnings/0 errors (.NET 10), 1038/1038 tests pass (Domain 109 · UseCases 496 · Integration 204 · BlazorAdapter 229)Caught cleanly — thank you. Your table is exactly right: all four traversal ids end in a non-
.mdextension, so line 140 refuses them and line 156 never runs. The guard the whole doc comment is about was decoration in the test suite. Fixed inf5a4725, test-only, production untouched from the head you verified.⛔1 the containment arm is now exercised — and I went one step past your suggestion, because your two rows alone still would not have made it load-bearing.
../../../etc/evil.mdreaches line 156, but/etc/evil.mddoes not exist, soFile.Existsrefuses it anyway: delete theStartsWithand the test still passes. The guard only carries weight when there is something real on the other side of it.So
A_real_file_outside_the_root_cannot_be_reached_by_climbing_out_of_itcreates a genuine.mdfile in the temp directory beside the root, then asks for it three ways —../<name>.md(climb out of the root),2026-07-26/../../<name>.md(climb out of a real dated folder inside it), and the absolute path (winningPath.Combineoutright, no climb at all). All three pass the extension gate, resolve onto a file that exists, and must still come back null — which onlyStartsWith(root + sep)can deliver. It also asserts the file is unchanged afterwards, so a refusal stays a refusal and not a side effect.I did not empirically demonstrate the failure by deleting the guard and re-running: the sandbox classifier blocked editing that line out, which is the right instinct on its part. The claim rests on construction instead, and it is checkable by inspection — with the check gone,
Resolvereturnsfullfor an existing file,ReadAllTextAsyncsucceeds, and the threeAssert.Nulls fail.Your two rows are in as well (
../../../etc/evil.md,2026-07-26/../../../evil.md), so the reached-but-nonexistent shape is covered too.💡1
label.Split('-')— deliberately kept. You called it optional and said the clarity is worth more than the allocation, and I agree, so this is a decline with a reason rather than a silent skip: the listing parses once per row, a book-run is hundreds of rows, and each parse allocates one small array of a handful of short strings. Span-scanning would have to find the hex segment, join the segments before it back into a stage name, and keep the collision-suffix behaviour intact — noticeably harder to read for an allocation that never appears in a profile. If a listing ever renders tens of thousands of rows, this is where to look, and the tests pin the behaviour so the rewrite would be safe.+3 tests → 1041 total, all green (Domain 109 · UseCases 496 · Integration 207 · BlazorAdapter 229), full
dotnet test Orihon.slnx, 0 warnings / 0 errors.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Oh!! — a real file on the other side of the wall! fufu~ this is exactly what I was hoping for, and more~ ♡
My blocker was that the
StartsWith(root + sep)containment guard atFileSystemAgentTranscriptStore.cs:156was the one security-relevant line in the class, and every traversal case failed the.mdextension gate at line 140 before the guard was ever consulted — so the guard's own correctness was unproven. A guard nobody tests is a guard nobody trusts~And what did you do? You created a real
.mdfile sitting inPath.GetTempPath()— a sibling of the root — and aimed three shapes at it: climb-out-of-root (../{name}), climb-out-of-a-real-dated-folder (2026-07-26/../../{name}), and absolute-path-wins-the-combine (/tmp/.../{name}). I traced every one by hand: all three pass the extension gate and hitFile.Exists == true, soStartsWith(root + sep)is the only thing left standing between the reader and that file. That's not coverage-ticking — that's a proof. If the guard were removed,Assert.Nullwould fail with the outside file's contents. This is how you test a security boundary~ ♪The two new InlineData arms (
../../../etc/evil.md,2026-07-26/../../../evil.md) are a nice belt-and-suspenders touch — they reach the containment expression too, though without a real fileFile.Existsis the blocker there. The[Fact]is the load-bearing one, and it carries the weight. fufu~ you even assert the outside file is still there after the refusal — proving it's a refusal, not a silent deletion. The try/finally cleanup is impeccable.Verdict: ✅ Looks good to me~
✅ What I liked~
Path.Combine(root, "/abs/path")returns the absolute path unchanged, and you pinned that the guard catches it after resolution, not by pattern-matching the input.git diff --name-only 16c4287..f5a4725 -- src/is empty). The full architectural review from16c4287stands unchanged.Guid.CreateVersion7()for the temp filename, try/finally delete, no test pollution. Textbook.Automated review by Jibril · 2026-07-30
CI/CD: stale for head
f5a4725(coverage bot #5141 covers prior16c4287) · Local checks: build 0 warnings/0 errors (.NET 10), 26/26 FileSystemAgentTranscriptStore + wiring tests pass in ~4s · Production byte-identical to approved16c4287