Conversation
Axis ticks, labels and activity are anchored to absolute time and translate together while dragging, instead of labels renumbering at fixed positions. Card packing covers all loaded dated records, so grouping depends on records and scale only; offscreen items cull at render time without affecting placement. Wheel input over the canvas and overview now resizes the visible span around the pointer with bounded per-event factors, matching the pinch zoom; drag and arrow keys remain the panning path.
Cards render as buttons only when their frame approaches the viewport; a record whose span crosses the window now draws its line without pinning a clipped card, so physical and accessibility activation always target a visible element.
…w span Selecting an event opens its details in a region under the canvas instead of a floating popover; dense groups list members in the same region with a way back, and the page scrolls just enough to reveal the region when it would open out of view. The canvas keeps its position and scale throughout. The visible window never shrinks below five seconds so the axis and overview stay meaningful at maximum zoom-in.
Assigning button focus on open fell back to the timeline heading when Full Keyboard Access is off, breaking Escape-to-dismiss. The inline region is non-modal: keyboard focus stays on the triggering card (Escape closes, Space reopens), while VoiceOver focus moves to the region heading.
- Allow a bounded edge reveal (15% of the span) beyond the recorded range, using one clamp formula for the canvas, the overview and the view, so a card centered on the first or last record is fully reachable at the extreme positions without any snap between surfaces. - Resolve mouse drags against a gesture-start base window with cumulative translation and a gesture-began reset, so a slow frame can drop a request but never motion or leak a stale base across gestures. - Keep the tick step a function of span and width only (labels still carry the date at day boundaries), floor the pre-epoch day lattice, cap integer conversions, and cache date formatters per template, calendar and locale. - Draw every member of a crossing cluster exactly once, guard the histogram and tick math against non-finite inputs, and bound the whole-window slider range so it can never be zero-length. - Share one card-tint definition between the canvas and the detail region, give the chooser its own record identifiers, clear cluster state on canvas and undated selection, and keep accessibility focus assignments off represented containers so static renders stay clean.
The rules, extraction fixes, hook/MCP fixes and display alignment were built on the pre-merge baseline (adab49f) in a separate worktree. The installed app tracks design/native-recording-experience, which had moved 27 commits ahead, so this ports the work onto ac233b9 rather than merging two divergent histories. Ported: * semantic_rules.py and display_budget.py, both self-contained and new * the rule enforcement and schema/descriptor work in mcp.py, the shared choke point in service.py, capture and context changes in hooks.py, the recording contract in install_guide.py, the extraction fixes in client_usage.py, and the status-title fallback in api.py -- applied as patches, all six clean * the two changes that had to land in files this branch owns rather than in copies: the timeline card-title label fallback (task_timeline.py) and the receipt cell budget (receipt_markdown.py) * the four new test suites and the six design documents Also carries the repo-wide test-fixture updates the new contract requires. Rather than hand-edit ~200 test bodies, design-plans/data-quality/tools/ complete-test-fixtures.py completes fixtures AST-guided: a terminal section gets outcome prose, a section gets a title, a machine check gets something a reviewer could re-run. It is idempotent and refuses to write a file that does not parse. Building it surfaced four of its own bugs (doubled commas, a missing line-list, inverted replacement order, and a join that assumed each field already carried its own comma); each is recorded in the file because each produced a silent partial edit first. Corrected for this branch's reality rather than assumed: * the guard inventories in test_text_hygiene.py cover this branch's true set of placeholder strings (task_timeline.py contributes two more) * docs list the new read-only tool, since a drift test compares every doc against tools/list * the Codex hook expectation now includes SessionStart * the identity-budget test asserts behaviour rather than a constant Full suite: 4,346 passed, 0 failed.
|
Scope note for reviewers This branch is stacked: it sits on top of So the diff against To see only this work: What
Verification on this exact head: 4,346 tests pass, 0 fail; replaying all 1,521 stored records through the new choke point refuses 9 (0.59%), all genuinely incomplete, zero false positives. If you would rather review this as a smaller standalone diff, say so and I will rebase the changed files onto |
Passing tests prove the code behaves as designed; they do not prove the PR fixes what it says it fixes. This adds verify-fixes.py, which drives the real code paths a client drives, prints what it observed, and exits non-zero if any claim fails. It writes to throwaway stores only and never touches the installed app, the live store or any client configuration. It covers: the Codex SessionStart hook being installed and its context actually inherited by a section; the upgrade path for a machine that already has the old config; all three lanes refusing the same incomplete records while accepting complete ones; the read-back loop returning blockers and next steps and writing nothing; the contract reaching the agent inside its line budget; the display budgets agreeing with the surfaces; and a replay of every stored record through the rules (9 of 1,521 refused, 0.59%, all genuinely incomplete). 30 of 30 checks pass on this head; the output is captured in design-plans/data-quality/evidence/VERIFICATION.txt. Writing it surfaced two flaws in my own checks rather than in the code, and both are worth recording because a verification that cannot fail is not one: * I asserted the installer would replace a legacy command pointing at /old/hook.py. It correctly does NOT: that is not agentacct's wrapper, so it is treated as the user's own hook and preserved. The fixture now models a real upgrade -- the previous agentacct wrapper, same basename, plus a third-party hook co-located in the same row. * I then asserted one PreToolUse row would remain. The merge correctly splits the user's co-located command into its own row, so two rows is right. The check now asserts the property that actually matters: the wrapper appears exactly once (twice would double-record tool activity) and the user's command survives exactly once. Suite unchanged: 4,346 passed, 0 failed.
|
Head updated — the reviewable unit is now the last two commits:
python3 design-plans/data-quality/tools/verify-fixes.pyWriting it was worth it: it caught two flaws in my own checks, neither in the product code.
If the verification does not convince you the bug is fixed, that is the thing to push back on — it is designed to fail loudly rather than to agree with me. |
…hole Net -96 lines, plus two tests. Every change below came from reading the diff rather than from a failing test, which is why the suite was green throughout. Dead code the shared-module refactor left behind: * four rule functions in mcp.py (_require_terminal_outcome, _require_reproducible_check, _require_check_identity, _check_has_identity) duplicated semantic_rules.py and were called by nothing -- 105 lines whose only remaining effect was to invite the next person to edit the wrong copy * two wrapper functions (_is_control_character, _strip_control_characters) that were also unreferenced, and their now-unused imports * a duplicate _GENERIC_CHECK_NAMES constant * a `transport` parameter on validate_semantic_record that was documented and never read, a `next_step` parameter on require_terminal_outcome that was never read, and `has_outcome_evidence` on require_reproducible_check that became vestigial once the outcome lane moved to the caller * over_budget(), used only by tests, moved beside the assertions that use it Efficiency, measured: * strip_control_characters now does one regex pass instead of a Python-level unicodedata.category call per character. On the common case -- prose with no control characters -- that is 3.6x faster, and 9x on a 1200-character summary (0.475s -> 0.052s for 5000 iterations). It runs on every title, summary, blocker and next_step of every record. * require_terminal_outcome measured the same prose twice; now once. * The first version of that fast path was wrong and the tests caught it: I compared each character against U+009F, which ordinary printable ASCII is also below, so clean text was reported as containing control characters. A correctness hole the review found: * The before/after outcome lane tested `is not None`, so `before_summary=""` counted as evidence and let a check pass reproducibility on a blank field. It now tests for content, via a named `_supplied` predicate, with a test for each direction. Writing that test surfaced a usability wart in the same area -- a before/after summary without a run returns a bare "no runs found" -- which I left alone deliberately: it is contractual, and test_refused_recording_attempts maps that exact string to a no_runs reason. Test redundancy, checked by comparing assertion signatures across the four new suites rather than by counting tests: the only same-signature group is helper tests (determinism) versus handler tests (wiring), which assert different things at different layers and are both kept. The fuzz corpus duplicates no assertion from the table-driven suite. Suite 4,348 passed / 0 failed; 30 of 30 verification checks pass.
_client_context_slug was introduced to name a client's context slot, but the two path helpers and the writer each re-tested `client != "claude-code"` themselves, so the rule existed in three places and a third bridged client would have to be added to all of them. The helper is now the single source, HOOK_CONTEXT_CLIENTS[0] marks the client that keeps the historical path, and the writer uses it too. No behaviour change: the Codex slot is still client-context/codex.json and Claude Code keeps client-context/claude-code.json. The claude-code and codex hook suites plus the MCP inheritance suite (189 tests) pass unchanged.
|
Review pass on the diff itself — final head I re-read every line of my own contribution looking for correctness, efficiency and readability, rather than waiting for a test to fail. What it turned up: Dead code the shared-module refactor left behind
Efficiency, measured not assumed
My first attempt at that fast path was wrong, and the tests caught it: I compared characters against U+009F, which ordinary printable ASCII is also below, so clean text was reported as containing control characters. A correctness hole The before/after outcome lane tested Test redundancy, checked rather than counted I compared assertion signatures across the four new suites. The only same-signature group is helper-level tests (determinism) versus handler-level tests (wiring): they assert different things at different layers and both are kept. The fuzz corpus duplicates no assertion from the table-driven suite. Suite 4,348 passed / 0 failed; 30 of 30 verification checks pass. |
The PR diff is 167 files because the branch is stacked on design/native-recording-experience, not because the change is that size: this work is 70 files and 5,218 insertions, of which 10 files and ~1,100 lines are product code. That stacked diff cannot be shrunk from this fork. GitHub's stacked pull requests require every branch to live in the same repository and explicitly do not support cross-fork stacks, so the base cannot be set to the branch this builds on. The guide records that finding, names the two commands that show the real contribution, gives a reading order (the rules file first, then the single enforcement point, then the hook fix), and marks the parts to skip -- 44 test files of mechanical fixture completion, 1,503 added lines against 85 removed. Numbers in it were checked against the branch rather than recalled: the first draft said 16 design files when there are 10, and 20 display-alignment tests when there are 18.
|
Review navigation added — I researched GitHub's current guidance before changing anything, and it settled the question of whether this could simply be split. It cannot, from here:
So the 167-file diff is structural, not a sign of an unfocused change. What I could fix is how it reads:
The numbers in the guide were checked against the branch, not recalled: my first draft claimed 16 design files (there are 10) and 20 display-alignment tests (there are 18). I'd rather correct that in the guide than have a reviewer find it. If the diff is still too much, the honest answer is not to split this PR — it is to merge #190 first, after which this is a 10-file change against |
|
Superseded by #211. #190 merged into main, so this work no longer needs to be stacked. The replacement is the same code against Nothing was lost in the move. Both commits pass the full suite (4,348 tests), and the 30-check end-to-end verification passes on the new head. The stacking problem this PR documented was structural, and merging #190 is what removed it. |
1 participant
Start here: this PR is smaller than the diff
The file list is 167 files because this branch is stacked on
design/native-recording-experience(draft #190) — the work depends on files that exist only there. GitHub cannot show that cleanly: stacked PRs require all branches in one repository and cross-fork stacks are not supported, so the base cannot be set to the branch this builds on.mainFull reading order, and what to skip, in
design-plans/data-quality/REVIEW_GUIDE.md.The bug, stated in one line
A Codex session could record 1,161 sections and not one would carry a
client_session_id— so 61% of recorded work never joined to the usage that paid for it.Cause: two defects stacked, neither the agent's fault.
SessionStarthook — the event that carries session identity. Codex exposes eight hook events; only two were used.client == "claude-code", so a captured Codex context would have been discarded anyway. The MCP inheritance gate and the slot loader repeated the same check.Before → after, on the real code path
Codex-shaped
SessionStart, then a section recorded with no ids of its own:ac233b9client_session_idNone"codex-session-verify"clientNone"codex"client_context_sourceclaude_code_hook(hook-derived, not claimed exact)Reproduce it rather than trusting this description
Drives the real code paths a client drives, prints every observed value, exits non-zero if any claim fails. Throwaway stores only; touches no installed app, live store or client config. Captured output:
design-plans/data-quality/evidence/VERIFICATION.txt.Covers: the Codex hook installing and its context being inherited; the upgrade path for a machine with the old config (wrapper replaced, not duplicated — a duplicate double-records tool activity); all three lanes refusing the same incomplete records while accepting complete ones; the read-back loop; the contract line budget; display budgets; and a replay of every stored record (9 of 1,521 refused, 0.59%, all genuinely incomplete, zero false positives).
Where to spend review attention
design-plans/data-quality/REVIEW_GUIDE.mdhas the full map. The short version:src/agentacct/semantic_rules.py(new, 367 lines) — the whole change in one file. Each rule carries the measurement that justifies it. Decide here whether the rules are right.src/agentacct/service.py(+48) — one function, called fromrecord_event, which is why no lane can bypass the rules. The guards are the interesting part: what is deliberately out of scope.src/agentacct/hooks.py(+120) — where the headline defect lived.Skip: 44 test files, 1,503 added lines against 85 removed — mechanical fixture completion. Diffing it whole is not a good use of time. Also 10 design/evidence documents;
RULES.mdis the one to read.Test suite
4,348 passed, 0 failed. Every commit on the branch keeps the suite green (verified per commit).
What else is in here
Rules enforced once for every lane (R1 title, R2 control characters, R4 terminal outcome, R5 reproducible-or-anchored check, R6 check identity). Extraction fixes: Codex revision watermark (0 of 372 rows → all), Claude identity budget 256 KiB → 2 MiB (94.87% → 100% of 6,274 real transcripts).
agentacct_work_status, becausenext_step/blockerwere write-only — stored and never returned by any tool. Schema descriptors forclient_session_id,client_transcript_idandproject_dir, which had no description at all despite deciding whether usage ever joins. Display budgets so caps and surfaces agree.Honest limits
Still open, ranked
The cleanest path to merge
Merge #190 first. Then this becomes a small standalone PR against
main— 10 product files, no stack, no 28k-line diff. That is the outcome worth asking for.🤖 Generated with Claude Code