install, coverage, reads, and goroutines: twelve entries from the code-quality review by k4rthiknerella · Pull Request #63 · altrace-dev-role/rashomon

k4rthiknerella · 2026-10-14T17:53:05Z

Stacked on #61 (intake: one bounded read and one scan of every hook payload); review and merge after it.

Plugin hooks.json decoding ignored matcher entirely. A hooks.json narrowed to "Bash" still reported present_plugin and verified, while Edit, Write, and MCP calls went unrecorded (the code-quality review's fix list, entry 1 (hld-1)). pluginHooksDoc now decodes each hook group's matcher through a coverage-only check before matching on event name, so a narrowed matcher resolves to hook_entry=absent instead of a false present. A different timeout, or a second hook alongside ours in the same group, does not cost coverage: coverageIntact asks only whether the matcher narrows which calls fire and whether a command hook is in the group, because that is what the coverage record's hook_entry field actually means. intact, the stricter check that also looks at hook count and timeout, is unchanged and still decides whether watch or detach can safely overwrite or remove an entry someone has edited by hand.

RollupCoverage only collected a reason when the state was unverified and the reason was non-nil, so a degraded state, or an unverified state with a null reason, both still rolled up to verified (entry 6 (errors-5)). It now adds records_unreadable for both cases, and the schema requires a non-null reason whenever the state is unverified.

Fail-closed reads

Three read paths treated "cannot read" the same as "does not exist," or did not account for it at all:

An unreadable transcript left the H-10 accounting check silently skipped, and the run still rendered verified (entry 7 (errors-3)). Any error other than fs.ErrNotExist now reports transcript_unreadable.
An unreadable pause marker let the hook record anyway (entry 8 (errors-6)). A marker that fails to parse already paused recording before this change; only the read-error path was wrong. checkPaused now treats a read error on the marker the same way, and records through hook.RecordPaused when a store exists.
Every EvalSymlinks error on settings.json was treated as "a path that does not exist yet," so watch on a dangling settings.json symlink exited 0 and replaced the link with a regular file (entry 9 (errors-4)). Lstat now runs first; only fs.ErrNotExist from Lstat is treated as a first install, and any other symlink resolution failure returns an error naming the link and its target. Load makes the same check before reading, so status reports a dangling settings.json symlink as unreadable instead of "not installed" over a file watch would refuse to write to.

Panics and goroutines

PanicError promised that a recovered panic's error names only the value's type and deliberately nothing else, but internal/safe had no test enforcing that, and a panic value is often a command line or a prompt, either of which can hold a credential printed to stderr on every guarded CLI command (entry 13 (docs-5)). A recovered panic is now tested directly: Guard and Go's onPanic must return exactly "recovered panic of type string" for a string panic value, with nothing else attached.

A recovered panic was also recorded as an ordinary outcome rather than a panic: a labelling panic collapsed into the same value as a missing path, and a Capture panic became a plain internal_error (entry 41 (errors-8)). Both now map to a recovered_panic reason, registered in the store's reasons, the schema enum, and the text layer.

cmdProbe called WatchSignals and checked its phase outside safe.Guard, despite a comment claiming the call was guarded whole (entry 45 (docs-10, errors-13)); both calls now run inside the guarded closure. "Go is the only sanctioned way to start a goroutine" was a rule stated in comments with nothing enforcing it (entry 46 (conc-5)); a parser-based test now fails on any *ast.GoStmt outside internal/safe, and on a call to time.AfterFunc or context.AfterFunc, which start one the same way with no recover of its own. The walk's own health check no longer leans on a file count that could stay above the floor while still walking fewer files than it once did; it names two files from different trees and fails if either stops being walked.

CLI help and install API

--help said watch installs seven events. It installs eight: the eighth is the UserPromptSubmit hook that runs recap pending on every prompt, and neither the usage text nor the install test named it (entry 20 (naming-4)). The help text now lists all eight events and the recap pending subcommand, and a test checks that every installed event and subcommand appears in --help.

rashomon pause printed "no tool call will be recorded," but a call made while paused still writes a coverage record so the gap is visible in the report — a deliberate behavior that the printed line contradicted (entry 38 (docs-2)). The message now says tool calls are not recorded until resume, and that each one still writes a coverage record.

install.Events was the package's one exported mutable variable, controlling what watch writes and detach removes (entry 64 (naming-11)). It is now Events(), a function.

A second pass over the same ground

checkPaused treated any error from store.Paused as a pause, including a store root that's missing or inaccessible, or a pause marker that's actually a directory -- none of which means anyone asked for recording to stop, and none of which left a trace. It now fails closed only when the marker itself exists and can't be read, under its own reason (pause_state_unreadable), and lets any other fault surface through the normal path instead. resume removes an unreadable marker rather than returning the raw read error and leaving the pause stuck.
A panic recovered inside the file-label derivation overrode a coverage reason that cost more: a call with no settings entry at all, whose label also panicked, read recovered_panic instead of hook_entry_absent. recovered_panic now only fills the record's reason when nothing else already has.
cmdHook, cmdPost and cmdProbe each discarded the error from their own outer panic guard. A panic in the prologue, before the store was even open, left nothing on stderr; it's now named there, by type only, never by value.
TranscriptIDs returns the first error it meets walking a session's main transcript and every subagent file beneath it, so a subagent file that vanished mid-walk read the same as a missing main transcript, and the accounting group silently read as missing instead of unreadable. It now classifies on the main transcript's own file check. Each transcript group also carries an explicit status of read, missing, or unreadable, so a consumer holding several groups can tell which one cost the check.
FindPlugin skipped a plugin event whose hooks.json it couldn't read or parse and still returned the plugin, reading the same as no plugin installed at all. It now fails the whole lookup, so status reports it as unreadable and watch refuses instead of installing beside a plugin whose actual state it never saw.
Load treated a dangling settings.json symlink as an absent file, while Write and ReadOriginal already refused it. Load now refuses it the same way, so status and watch agree.
pausedCoverageNote's own doc comment sat directly above cmdPause with no blank line between them, so godoc attached the whole comment -- cmdPause's included -- to the const, and cmdPause itself carried none. They're separated now, each with its own.

CI

Five tests make a pause marker or a transcript file unreadable with chmod 000, which root bypasses. A new CI step runs them and fails if any skip, the same way the existing H-17 step does.

lutic1

@k4rthiknerella, all twelve entries are addressed in code, and install can't leave settings half-written. Please fix the items below.

From the review

cmd/rashomon/main.go:1118-1121: the statusRecording doc says hooks "fall back to recording" when the pause file can't be read. Since entry 8 they treat that as paused, and status prints "recording: unknown" while hooks record nothing. Fix the comment, and have status print "paused (marker unreadable)".
Entry 6 in the description mentions coverage_state_unrecognised, which doesn't exist. Entry 8 says the parse-failure path changed, but only the read-error path did. Correct both.
statusPlugin (main.go:1258-1267) prints every event as "absent" and also reports an overlap for a hand-edited plugin. Print present (hand edited, not counted for coverage) for events in p.Declared but not in p.Events.
TestRubricERRORS8_RecoveredLabelPanicIsACoverageReason sets labelRecovered by hand instead of driving the recover. Acceptable, but weak.
The chmod-000 tests (errors-3, errors-6) skip as root, so their mutants are NOT JUDGED. Make sure CI runs them as a non-root user.

Adversarial pass (please check each one; fix it, or reply if it doesn't apply)

main.go:588 checkPaused: any store.Paused error now counts as paused. That includes EACCES or ENOTDIR on the store root, a pause that is a directory, and EIO. When no store exists it returns true with no coverage record and nothing on stderr, so a filesystem fault silently turns off recording. Fail closed only when the marker is known to exist, and use a distinct reason such as pause_state_unreadable.
rashomon resume calls store.Paused first and returns the raw read error, so it can't clear the very state this PR creates. Remove an unreadable marker, or explain how to fix it.
report.go:1397: TranscriptIDs returns one error for the main transcript and all subagent transcripts. A subagent file deleted mid-walk gives ErrNotExist, the group is classed as missing, and the H-10 check is skipped. Classify on the main transcript's own Stat.
transcript_unreadable depends on the filesystem state when the report runs, so a run can flip from unverified back to verified once the transcript is deleted.
The missing-versus-unreadable distinction is unexported and never reaches JSON. Expose a per-group status.
handle.go:547-551: recovered_panic overrides BuildCoverage's resolution reasons (hook_entry_absent and others), hiding problems that cost more.
main.go:249-283 cmdProbe discards safe.Guard's error, so a probe panic leaves no recovered_panic record and nothing on stderr.
install/plugin.go intact(): a larger timeout or an extra hook makes coverage read absent even though every call is recorded. Separate coverage-narrowing checks from conformance checks.
intact() requires matcher "*" exactly, but an absent or empty matcher is match-all in Claude Code, so complete plugin installs read unverified.
FindPlugin skips a hooks.json it can't read or parse, so Live() is false and watch can install duplicate entries.
settings.Load still treats a dangling symlink as empty while Write/ReadOriginal refuse it, so status says "not installed" while watch errors. On the detach path, a dangling link should warn and exit 0.
main.go:892-899: pausedCoverageNote was inserted under cmdPause's doc comment, so godoc attaches that comment to the const.
store-schema.json / unreadableCoverage: only "unverified with a null reason" fails closed. A verified record with a reason, or a reason outside the enum, is still believed.
safe_test.go CONC5 detects only go statements, not time.AfterFunc or context.AfterFunc. Replace the 50-file floor with a known-file check.
The pausedCoverageNote text understates what's written while paused (install_id, timestamp, probe records), and the decision is made at pause time rather than at hook time.

Generated by Claude Code

The plugin origin never decoded matcher or timeout. pluginDeclaresEvent read type, command and args only, so a hooks.json whose PreToolUse group was narrowed to "Bash", or given a timeout of 600, still resolved hook_entry present_plugin and the run rendered verified while Edit, Write and every MCP call went unrecorded. The settings origin already refused both through intact(). pluginHooksDoc now keeps each event's groups as raw JSON, and pluginDeclaresEvent skips any group intact() does not accept before it reads the command and argument vector. present_plugin and present_settings now mean the same thing: our entry, with our matcher and our timeout. FindPlugin's Events, which status and watch's refusal read, follow the same rule. The comment in internal/hook/coverage.go that claimed a narrowed entry reads absent is now true for both origins and says so. Tests: an internal/install test runs pluginDeclaresEvent over the hooks.json this repository ships for every installed event, and a table narrows one field at a time (matcher Bash on PreToolUse and PostToolUseFailure, matcher removed, a matcher on SessionStart, timeout 600 on PreToolUse and Stop) and requires that event to read undeclared while every other event still reads declared. An acceptance test runs the plugin binary under a narrowed hooks.json and requires the call coverage to read hook_entry absent. Five mutants pin the check and the shipped file.

install.Events was the one exported mutable package variable, and it decides what watch writes, what detach removes, what status lists and which events the plugin check reads. Any importer could append to it or overwrite an element, and every later caller in the process would see the edit. It is now func Events() []string, which builds a fresh slice on every call in the installed order. Every caller moves to the call form. The H-20 mutant that drops PostToolUse from the list is re-anchored on the new indentation and still turns TestH3_ red. A test edits the slice it was handed, including an append into its spare capacity, and requires the next call to return the original list; a mutant that hands every caller one shared slice turns it red.

RollupCoverage collected a reason only from a record that was unverified and named one. A record that was unverified with a null reason, which docs/store-schema.json accepts, and a record carrying any state this build does not write (a newer build's, a hand edit's, an empty one) both added nothing, and the run rendered verified. The turn digest shares the rollup, so a turn read the same way. A branch ahead of the existing collector now adds records_unreadable for any state other than verified, unless it is unverified and names its reason. The existing collector's tail is byte-identical, so the legacy present anchor still applies and still turns its test red. records_unreadable is reused rather than a new code, because a new report reason would need the schema's report_reason enum to move with it; its explanation in text.go now names the case. No hook writes either shape: BuildCoverage sets unverified only beside a reason. Tests: a table of four unreadable records through RollupCoverage and report's build, a control showing readable records add exactly their own reason, and the same table through the turn digest's buildTurnCoverage. Three mutants pin the branch and both halves of its predicate. The schema if/then that makes reason non-null beside unverified is a docs/ change and ships as a patch.

When a transcript a run's calls named existed but could not be read (chmod 000, an over-long line, a subagent directory that would not list), the H-10 accounting check silently did not run and the run rendered verified beside "readable: no". The branch meant to tell this apart from a missing file was a no-op: Readable was already false. accounting now marks a transcript that exists and cannot be read, and build adds a new report reason, transcript_unreadable, registered in report.Reasons() and explained in text.go. The mark is an unexported field, so the JSON shape is unchanged, and the H-10 anchor on the accounting call is untouched. A transcript that does not exist adds no reason, as a written decision: Claude Code deletes transcripts after its cleanup period, so a missing one is the ordinary state of every older run, and a reason for it would turn each of those runs unverified after the fact. The README now says that verified then means the store's own records are complete without the cross-check. Tests: unit tests for a chmod-000 transcript (unverified with the reason) and a missing one (still verified), and an acceptance case through the binary, both skipped on Windows and as root. Three mutants pin the mark, the ENOENT decision and the reason. The report_reason enum in docs/store-schema.json needs the new code; that is a docs/ change and ships as a patch, and until it lands TestStoreSchemaReasonsAreTheCodeReasons names it.

checkPaused read any error from store.Paused as "not paused". With the marker present but unreadable (chmod 000), the hook recorded the call, so declarations went from one to two, while status reported "recording: unknown" for the same file and a marker that does not parse already kept recording paused. checkPaused now returns early only when store.Paused succeeds and says not paused. A read error takes the paused path: nothing is recorded, and where a store exists hook.RecordPaused writes the recording_paused coverage record, so the stretch shows as a deliberate gap. store.Paused's signature is unchanged, and it returns no error for a marker that does not exist, so a machine that never paused is unaffected. Tests: an in-process test runs the hook against a chmod-000 marker and requires no declaration and a recording_paused coverage record, and an acceptance case does the same through the binary next to H-78 to H-81; both skip on Windows and as root. A mutant restoring the old condition turns the unit test red.

Write treated every EvalSymlinks error as "a path that does not exist yet". A settings.json that is a dotfile manager's symlink to a target that was moved or never created therefore read as a first install: watch exited 0 and renamed a regular file over the link, the bug the symlink handling above that line was written to prevent. detach read the same link as an empty file and reported nothing to detach. writeTarget now decides what Write replaces. It calls os.Lstat first: only fs.ErrNotExist there is a first install. A path that resolves is followed as before. A symlink that does not resolve is an error naming the link and its os.Readlink target, and nothing is written, neither over the link nor at the target. ReadOriginal applies the same check, so watch and detach both refuse before they edit anything. Writing through to the missing target was considered and rejected: it would create directories and a file wherever the link points, which is a link following decision (CWE-59) this program should not make for the user. Tests: unit tests for a dangling link (refused, named, link intact, target directory not created), a working link (written through, still a link) and an absent file (first install), and an acceptance case where watch and detach --all both exit non-zero with the link unchanged. Two mutants restore the old fall-through in Write and in ReadOriginal.

internal/safe made two promises that nothing tested. PanicError names a recovered panic's type and nothing else, yet a mutant that formats the value into the error with %v passed every package; the value is often a command line or a prompt, either can hold a credential, and guarded() prints this error to stderr for every CLI command (CWE-209). And the package comment calls safe.Go the only sanctioned way to start a goroutine, which no check enforced. internal/safe/safe_test.go now holds both. Guard over panic("CANARY-...") and over panic(errors.New("CANARY-...")) must return exactly "recovered panic of type string" and "recovered panic of type *errors.errorString", and Go's onPanic must receive the same type-only error. A go/parser walk over every non-test .go file in the module, build tags ignored so every platform's files are read, fails on any go statement outside internal/safe; test files are exempt because they run under go test, not under Claude Code. The rule holds today: the one go statement outside tests is in safe.go. The PanicError comment drops the retired no-content rationale and states the one that stands, keeping credentials out of error text, and the package comment names the test that enforces the goroutine rule. Mutants: the %T: %v format turns the type-only tests red, and a bare go func() {}() in internal/hook turns the walk red.

The usage text said watch installs the recorders, the liveness probe and the Stop/StopFailure recap: seven events. watch writes eight. The eighth is a UserPromptSubmit entry that runs `recap pending` on every prompt, and neither the event nor the subcommand appeared anywhere in --help. The H-3 test's guard message still said seven. The watch line now names UserPromptSubmit as the recap's catch-up, the invoked-by-Claude-Code block lists `rashomon recap pending [--install ID]` with what it does, and `probe start|end` is split into one line per subcommand so each one an entry runs is spelled out. The H-3 guard checks for eight and says why. A test in cmd/rashomon walks install.Events() and requires each event as a whole word within the watch line (so PostToolUse is not found inside PostToolUseFailure) and each entry's invocation, taken from install.Spec.Command, somewhere in the usage text. Two mutants, one dropping UserPromptSubmit from the watch line and one dropping recap pending, turn it red.

rashomon pause printed "no tool call will be recorded until rashomon resume", and the README's pause row said only that it stops recording. Each call made while paused still writes a one-line recording_paused coverage record with its session id and working directory (H-80), deliberately, so the gap shows in that session's report. The sentence said nothing was recorded, and something was. Where a store exists, pause now prints that tool calls are not recorded and that each one still adds a one-line coverage note (session id, working directory) so the gap shows in the report. Where no store exists a paused call writes nothing (H-81), and the line stops after the first clause rather than promise a note that will not appear. The README row keeps its first cell, which arguments_test.go reads, and adds the same sentence to the second. An acceptance test next to H-80 pins both lines byte for byte; two mutants, the old sentence and the note printed with no store, turn it red.

A panic the barrier recovered was recorded as an ordinary outcome. A panic in Capture, Post or the probe became internal_error, the same code as a write the disk refused. A panic in the file-label derivation became LabelUnknown, the value shape.Label also returns for an absent, non-string or empty path field, and the call's coverage read verified; the comment saying the two cannot be confused was wrong. closeReason now maps any error wrapping a *safe.PanicError to a new store reason, recovered_panic, registered in store.Reasons() and explained in text.go; the hook, post and probe paths all go through it, so their terminal and coverage records carry it. fileLabel returns whether its recover ran. The label still reads unknown, no new label value is added (the report clamps unknown values to unknown anyway), the declaration and its ok terminal stand, and Close names the panic in the call's coverage record unless a reason that cost more is already there. The H-1, H-12, H-20 and H-46 acceptance tests now expect recovered_panic for their injected panics, and TestH46 also asserts the coverage record. The H-44 and H-46 mutants are re-anchored on the new fileLabel lines and still turn their tests red. Tests: closeReason over a recovered panic, a wrapped one, an ordinary error and a lock timeout; Close with the label flag set, in process; and an acceptance case that injects a panic into labelling and requires an ok terminal beside recovered_panic coverage. Three mutants pin the mapping, the coverage reason and the flag. The coverage and terminal reason enums in docs/store-schema.json need the new code; that is a docs/ change and ships as a patch.

cmdProbe called hook.WatchSignals and checked its phase argument before its safe.Guard, under a comment saying the body was guarded whole for the reason cmdHook gives; cmdHook's own comment calls exactly that layout a defect, because a panic in the prologue exits 2. cmdHook and cmdPost already started signal watching inside the closure. Signal watching and the phase check now run inside the guarded closure, and a new fault point, probe.start, is its first statement. The probe's body is otherwise unchanged, including the RunProbe line the REC-34 mutant anchors on, which still turns its test red. An acceptance test next to the H-1 cases injects each panicking fault at probe.start for both phases and requires exit 0, no traceback, and no coverage record for that phase, which shows the fault fired; before the change the point did not exist and the probe recorded its phase. A mutant that moves the injection back ahead of the barrier turns the test red.

The store schema is to gain an if/then on the coverage record that makes reason non-null whenever state is unverified (rubric errors-5), the rule the report now enforces by reading such a record as records_unreadable. Any schema that says so needs a reason-named key under the then branch, and TestStoreSchemaHasNoUncheckedReasonVocabulary flags every reason-named key it cannot compare against the code, by design, so the schema change could not land. reasonConstraintPaths names that one path. A key listed there is skipped by the tripwire only while its node constrains nothing but type (and a description); if it ever declares values, through an enum, a const, a $ref, items or a combinator, it is reported unchecked like any other new reason key. Unlike the compared vocabularies, the path need not exist, so this lands ahead of the schema change. A new tripwire case puts an enum under the constraint path and requires it to be reported. With the schema patch applied in a copy of the tree, the schema tests fail only on payload_too_large and flush_failed, two codes the stack already emits that the schema does not yet list.

The mutant that restores pause's old sentence dropped the only use of the note variable, so cmd/rashomon no longer compiled. Judged through test/acceptance alone it still read red, because the acceptance TestMain builds the binary and its failure prints the package FAIL line the drivers count as a failed test; judged with cmd/rashomon in the package list it is a broken build. The replacement now keeps note in the expression with an empty slice of it, so the mutant compiles and is red on the sentence alone.

build was 110 code lines and 142 raw lines at the base, over the 100-line function limit, and the transcript_unreadable reason took it to 113 and 145. The report-only verb-class and program tallies now live in Session.tally and the per-transcript accounting equation in Session.accountTranscripts. Their bodies moved unchanged and at the same indentation, so every mutant anchored on them still matches once. build is now 63 code lines and 76 raw.

FindPlugin.Events went through intact() so that a narrowed or slowed plugin entry reads absent in coverage. watch's duplicate-install refusal, status's overlap line and status's recording line read that same list, so an enabled plugin whose every group was hand edited -- a changed timeout is enough -- stopped refusing watch. watch then installed settings entries beside a plugin that still fires, and every call it fired on was recorded twice. The plugin read now answers two questions per event. Events stays the intact subset coverage needs. Declared lists every event some group runs our command for, intact or not, and Plugin.Live (enabled and declaring at least one event) is what the refusal, the overlap line and the recording line now ask. The refusal names the declared events. The H-73 mutant is re-anchored on the new refusal condition, and the hld-1 mutant file gains one mutant per changed decision, each red under the new TestRubricHLD1_ tests.

The recap line decides between "rashomon report" and "/rashomon:report" by asking, for an unmarked invocation, whether this process runs as the plugin. It asked install.PluginPresent, which since the intact check answers only for a group watch would install. A plugin whose Stop entry was hand edited -- a changed timeout is enough -- is still loaded and still ran this binary, so its slash command exists, but the line pointed at the CLI command instead. install.PluginFires answers the declared-at-all question for the process's own plugin, the same one Plugin.Live asks of a plugin found on disk, and the recap pointer now asks it. Coverage keeps PluginPresent: a hand-edited entry is still absent there. The H-91 marker mutant is re-anchored on the new call, and the hld-1 mutant file gains two mutants, each red under the new TestRubricHLD1_ test.

…plies-reason constraint

checkPaused treated any store.Paused error as a pause, including faults that have nothing to do with the marker itself: a store root that is missing, inaccessible, or not a directory, or a `pause` that is itself a directory. Those faults turned off recording in silence, with no coverage record and nothing on stderr, and status rendered the same faults as plain "unknown". store.MarkerUnreadable now tells an unreadable plain marker apart from everything else. checkPaused fails closed only on the marker itself, recording pause_state_unreadable, and lets any other error surface through the normal path. RecordingState and statusRecording report an unreadable marker as "paused (marker unreadable)" rather than unknown, since checkPaused is already treating it as a pause. Resume removes an unreadable marker and closes the window at now, the same answer it already gives a marker that exists but does not parse, instead of returning the raw read error and leaving the pause stuck.

pausedCoverageNote's explanatory comment sat directly above cmdPause's doc comment with no blank line between the two, so godoc attached the whole block, cmdPause's threat-model explanation included, to the const, and cmdPause carried no doc comment at all. The const now sits above cmdPause with its own short comment, separated from cmdPause's by a blank line, so each has its own. The line itself also listed the fields a paused call still writes -- session id and working directory -- and left out install_id, the recorded timestamp, phase, hook_entry and the probe fields, which a paused probe record also carries. Rather than keep chasing a complete list, the line now just says a paused call still writes a coverage record, which is true of every field it carries and does not need to be kept in sync with the record shape.

statusPlugin read Events alone, so a plugin entry a hand edit had narrowed out of coverage printed "absent" for every one of its events, while the overlap line right below it reads Live(), which reads Declared -- the fires-at-all list -- and named a duplicate-recording overlap for that same plugin. The two lines contradicted each other: one said the plugin's entry was not there at all, the other said it was there twice over. statusPlugin now checks Declared as well as Events. An event the plugin fires on but that is not the entry watch installs reads "present (hand edited, not counted for coverage)", which matches what the overlap line already said and names why coverage does not count it.

Close substituted recovered_panic for the call's coverage reason before calling BuildCoverage whenever the call itself raised no error and the label derivation had panicked. BuildCoverage only resolves hook_entry_absent and the rest of its own reasons when it is given an empty one, so the substitution meant it never got the chance: a call with no settings or plugin entry at all, whose label also panicked, read recovered_panic instead of the absent entry that was the reason its coverage could not be trusted in the first place. BuildCoverage is now called with the call's own reason, unchanged, so it resolves hook_entry_absent and the others as before. recovered_panic now only fills the record's reason when BuildCoverage left it empty, which is what the comment above this code already claimed. The hand-set unit test that pinned the old, overriding behaviour is removed: TestRubricERRORS8_LabelPanicIsNamedInCoverage in test/acceptance/h46_label_scope_test.go already drives the same recover through the real binary and asserts the same coverage record, so it alone judges the mutants that test covered.

cmdHook, cmdPost and cmdProbe each guard their whole body in one safe.Guard call and discard its error, on the reasoning that a record isn't possible before the store is even open. That reasoning covers the record, not the error: a panic in checkPaused, openForHook or standsDown left no trace anywhere, not even a line on stderr, which is a worse answer than the ordinary case these three already give through h.Flush's own guard. guardedHook replaces the discarded Guard call in all three. It prints the recovered panic's type to stderr -- never its value, PanicError's own rule -- and always returns exitOK, because unlike guarded, used by the other commands, an entry point Claude Code invokes as a hook must never exit anything else.

…sent Load's os.ReadFile on a settings.json that is a symlink to nothing fails with the same ErrNotExist a path with nothing at it gives, and Load read both as "no file", returning an empty Document. Write and ReadOriginal already refuse a dangling link by name, so status reported a settings file as not installed in the same moment watch would have errored trying to write through the same link. Load now asks writeTarget first, the same check ReadOriginal already makes, and returns its error rather than reading past it.

accounting read whichever error TranscriptIDs happened to return and asked only whether it was ErrNotExist. TranscriptIDs walks the main transcript and every subagent file beneath it and returns the first error any of them gives, so a subagent file that had vanished mid-walk produced the same ErrNotExist a missing main transcript gives, and the group rendered missing -- readable: no, no coverage reason -- even though the main transcript this group is named for was sitting right there, readable, and the H-10 check against it never ran. accounting now asks the main transcript's own Stat before deciding: if it exists, any later error classifies as transcript_unreadable, whatever file inside the walk produced it. The two states this group can read -- missing and unreadable -- both leave Readable false, which is the fact a consumer with several groups could not previously get out of the JSON: nothing told a transcript this run never wrote apart from one it wrote and then could not read again. Transcript now carries an exported Status of read, missing or unreadable, and the text render carries the same word next to readable.

The per-event loop inside FindPlugin skipped an event whose pluginDeclaration call returned an error and kept going, so a hooks.json that could not be read or did not parse read as a plugin declaring nothing for that event -- the same shape Live() sees for no plugin at all. status reported it as "none installed" and watch would have installed settings entries beside a plugin whose actual state it never read. FindPlugin now returns the error instead of continuing past it, so the callers that already handle a FindPlugin error -- status prints "unreadable", watch's refusal returns it -- see it.

The goroutine-start check walked the module for *ast.GoStmt alone, so either AfterFunc call -- which runs its function argument in a new goroutine with no recover of its own, the same hazard a go statement is -- passed through with no report. The detector is now its own function, unsafeGoroutineStarts, covering both AfterFuncs beside the go statement, with a unit test driving it over literal source instead of only through the real tree. The walk's own health check no longer leans on a file count. A floor only fails once enough files drop out to cross it, so a narrower filter or a module root resolved one level off could still read as healthy. It now names two files from different trees, cmd and a leaf internal package, and fails the moment either stops being walked.

The five tests that make a pause marker or a transcript file unreadable with chmod 000 skip under a runner that runs as root, the same condition H-17's dynamic tests skip under for a missing capability. Unlike H-17, a SKIP here is not caught by anything: `go test` exits 0 for a skipped test, so a CI image change that ran as root would silently stop proving any of these five, and nothing in the log would say so. A new step runs them verbosely and fails on any `--- SKIP`, the same pattern the H-17 step already uses.

…dits Present and pluginDeclaration asked intact's question for coverage: not just "does our command fire on every call" but "is this the exact entry watch installs, same matcher, same timeout, exactly one hook." A timeout different from the one watch installs, or a second hook added to the group, changes how fast or how often something ELSE there runs, never whether our command does, so neither should have cost coverage -- and a nil or empty matcher is match-all in Claude Code the same as the literal "*", so a settings file edited by a tool that wrote the key out, or a plugin manifest that never carried it, read as narrowed when nothing about the call pattern had changed. coverageIntact and matcherFires answer the narrower, coverage-only question: a matcher that does not narrow which calls fire, and a command hook somewhere in the group. intact itself is unchanged, and is still what Apply and RemoveIf ask before touching an entry -- overwriting or removing a hand edit without consent is wrong whether or not that edit costs coverage, which is a different question from this one. The plugin and settings acceptance fixtures that modelled a "hand edited, still fires" plugin by retiming every hook now narrow each group's matcher instead, since retiming no longer has that effect; the ones that modelled "not the entry watch installs" via a shipped timeout of 600 are removed, since that is no longer a coverage defect.

k4rthiknerella · 2026-10-17T19:25:39Z

Thanks for the careful pass. Point by point:

Review 1. Right, the comment was stale since entry 8 and status disagreed with the hook. Fixed in bfbe160 (separate commit; the actual print logic landed earlier in this branch's history) -- status now prints "paused (marker unreadable)" when the marker exists but can't be read, never "unknown".

Review 2. Both were wrong. Entry 6 only adds records_unreadable -- there's no coverage_state_unrecognised -- and entry 8 only changed the read-error path, since a marker that doesn't parse already paused before this PR. Fixed the description.

Review 3. Agreed, the two lines contradicted each other. Status now prints "present (hand edited, not counted for coverage)" for an event the plugin fires on but that fails the coverage check (acc2d91).

Review 4. The unit test did set it by hand. Removed it in 80cca36 -- TestRubricERRORS8_LabelPanicIsNamedInCoverage in test/acceptance/h46_label_scope_test.go already fires the real recover through RASHOMON_FAULT and judges the same mutants, so it alone carries that coverage now.

Review 5. CI already runs go test and the sweep as the unprivileged runner, but nothing failed if these five skipped. Added a step that runs them and fails on any SKIP, same pattern as the existing H-17 step (24f5dc2).

Adversarial 1. Agreed, a fault on the store root shouldn't read as a pause. checkPaused now fails closed only when the marker is there and unreadable, recorded as pause_state_unreadable, not recording_paused -- this was already done on the branch when I picked the PR up.

Adversarial 2. Good catch. Resume now removes an unreadable marker file and closes the window at now, instead of returning the raw read error -- already done on the branch when I picked the PR up.

Adversarial 3. Confirmed with a dangling subagent file: the group read missing and H-10 was skipped. accounting now classifies on the main transcript's own Stat, so a later error -- a vanished subagent file included -- reads transcript_unreadable (b976c41).

Adversarial 4. That's by design and the README already says so: once Claude Code deletes a transcript, verified means the H-10 cross-check wasn't available. transcript_mismatch behaves the same way. Pinning the verdict at session end is bigger, separate work -- no change here.

Adversarial 5. Fair. Transcript now carries an exported Status of read, missing, or unreadable in the JSON and the text render, so a consumer with several groups can see which one cost the check (b976c41).

Adversarial 6. Right, the comment promised the costlier reason wins and the code didn't check it. Close now calls BuildCoverage with the call's own reason first, so hook_entry_absent and the rest get the chance to resolve; recovered_panic only fills an empty slot (80cca36).

Adversarial 7. A panic inside RunProbe was already recorded via closeReason. The prologue was silent, and so were hook and post's outer guards. All three now print the recovered panic's type to stderr through a shared guardedHook (4a48c3c).

Adversarial 8. Agreed, a longer timeout or an extra hook doesn't lose a call, so it shouldn't cost coverage. Split the check: coverageIntact asks only about the matcher and a command hook being present; intact is unchanged and still backs Apply/RemoveIf's refusal to overwrite or remove a hand edit (9ef0421).

Adversarial 9. You're right, the hooks docs list "", "", and an omitted matcher as match-all. coverageIntact now accepts all three; intact (used for Apply/RemoveIf's refusal) is unchanged and still requires the literal "" there, which is correct for deciding whether an edit happened (9ef0421).

Adversarial 10. That swallow predates this PR. Failing closed is cheap: FindPlugin now returns the error instead of continuing past it, so watch refuses and status says "unreadable" instead of reading it as no plugin (237ae29).

Adversarial 11. The status/watch mismatch was real. Load now asks the same writeTarget check ReadOriginal already makes, so a dangling settings.json symlink reads as unreadable in both places (3c94aec). Kept detach's error as is, per your own note -- the target may still hold our entries.

Adversarial 12. Yes, the const took cmdPause's comment -- no blank line separated them. Moved the const above with its own short comment, separated from cmdPause's by a blank line, so each has its own now (bfbe160).

Adversarial 13. The out-of-enum case already failed closed; a verified record carrying a reason was still believed. The schema's verified-implies-null-reason constraint is already in docs/store-schema.json on this branch. I'm leaving unreadableCoverage's code change to you, since that's yours to decide.

Adversarial 14. Fair, AfterFunc starts a goroutine with no recover and the gate wouldn't have seen it. CONC5 now flags both AfterFuncs alongside go statements, and the walk's health check names two files it must walk instead of counting them (1e9a48f).

Adversarial 15. The field list was incomplete and wasn't worth chasing further. The line now just says each paused call still writes a coverage record (bfbe160).

lutic1

@k4rthiknerella, I re-reviewed the PR at 9ef0421. The four serious items are fixed. I ran the code and the chmod-000 tests as a non-root user to check:

Silent pause: a read error now counts only when the marker is a regular file, and it gets the pause_state_unreadable reason.
Resume: resume now removes an unreadable marker.
Matcher: an absent matcher, "" and "*" are all treated as match-all.
Subagent transcripts: classification now uses the main transcript's own Stat.

All the first-review items are fixed too. So are recovered_panic only filling an empty reason, the probe's Guard error, coverageIntact, the unreadable hooks.json error, the godoc placement and CONC5 AfterFunc. Build, vet, -race and --check are clean.

Still open (small)

unreadableCoverage still believes a verified record that carries a reason (report.go:906). Your reply says store-schema.json already enforces "verified means a null reason", but it doesn't: the only conditional on state is "unverified needs a string reason". Add the check, or add the else branch to the schema.
The CI root-skip guard (ci.yaml:162) covers only 5 of the 9 tests that skip as root. Add these four:
- TestCheckPaused_FaultOnStoreRootIsNotAPause
- TestResume_RemovesAnUnreadableMarker
- TestMarkerUnreadable_TrueOnlyWhenTheMarkerIsAPlainFile
- TestRubricERRORS3_UnreadableTranscriptRendersUnverified
The first two are the tests for the silent-pause fix.
Detach on a dangling symlink: keep the error. The target could come back with our entries in it, so failing loudly is the safer call.

Optional

resume writes a gap that runs from now to now, so the paused window reads as zero length.
hasMatcher covers only the three tool events, so SessionStart with "*" reads as narrowed. That's the safe direction, but it's still a false "unverified".
CONC5 matches on package name, so an aliased import slips past.
Deciding the paused note at pause time instead of hook time is still open.

The red "Perf gates (linux)" check is inherited from #59/#61.

Generated by Claude Code

lutic1 requested changes Oct 15, 2026

View reviewed changes

k4rthiknerella added 28 commits October 17, 2026 12:21

schema: recovered_panic, transcript_unreadable, and the unverified-im…

0e134cb

…plies-reason constraint

k4rthiknerella force-pushed the record/intake branch from 1daed7e to a3f9a9e Compare October 17, 2026 19:23

k4rthiknerella force-pushed the quality/hook-install branch from 7936c67 to 9ef0421 Compare October 17, 2026 19:23

k4rthiknerella requested a review from lutic1 October 17, 2026 19:25

lutic1 requested changes Oct 17, 2026

View reviewed changes

Navigation Menu

Repository navigation

install, coverage, reads, and goroutines: twelve entries from the code-quality review - #63

install, coverage, reads, and goroutines: twelve entries from the code-quality review#63
k4rthiknerella wants to merge 28 commits into
record/intakefrom
quality/hook-install

k4rthiknerella commented Oct 14, 2026 •

edited

Loading

Uh oh!

lutic1 left a comment

Uh oh!

k4rthiknerella commented Oct 17, 2026

Uh oh!

lutic1 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

k4rthiknerella commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fail-closed reads

Panics and goroutines

CLI help and install API

A second pass over the same ground

CI

Uh oh!

lutic1 left a comment

Choose a reason for hiding this comment

Uh oh!

k4rthiknerella commented Oct 17, 2026

Uh oh!

lutic1 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

k4rthiknerella commented Oct 14, 2026 •

edited

Loading