115 lines
6.1 KiB
Markdown
115 lines
6.1 KiB
Markdown
# Ledger review verdict, LEDGER-1506-R1
|
||
|
||
Reviewer: Filbert (independent; author is Darkwing, no self-approval).
|
||
Request: `docs/plans/reviews/2026-09-12_ledger-review-request.md`.
|
||
Brief: Piece 3, `docs/plans/2026-09-12_control-board-mvp.md` (approved by
|
||
Jason 2026-09-12). Review date: 2026-09-12.
|
||
|
||
## Verdict: APPROVED
|
||
|
||
Non-blocking observations are listed below; none contradicts the brief, the
|
||
boundaries, or the fail-closed rules. Gate D remains Jason's. This verdict
|
||
authorizes the transition to publication per the approved brief; it grants no
|
||
push, merge, or deployment authority.
|
||
|
||
## Candidate identity
|
||
|
||
Baseline `889d8750` verified on `refactor`: it is an ancestor of current HEAD
|
||
`db0d784b`; the single intervening commit (`db0d784b`) touches only
|
||
`docs/plans/DEFERRED.md` and produces an empty diff on all six candidate
|
||
paths. The reviewed worktree files are therefore byte-identical to the
|
||
baseline candidate.
|
||
|
||
Recomputed SHA-256 (all six match the pinned values):
|
||
|
||
```text
|
||
601db1de6d4838785f334dd93dd191d444276f306e016b3a4137659eaae76561 packages/ledger/README.md
|
||
e33fc5be5cdeb69f6f443a44c5b02a7bc346ac74dd69f984538e424a8d1b5de9 packages/ledger/src/cli.mjs
|
||
fbd95e45c5753cb44e39dfead0c6e0d2a5f07859dd9411f1dd659dde98af2ffc packages/ledger/src/ledger.mjs
|
||
c836ecd64068cccf7f515ab699c6a218e5ea1618c39e7e88be21bfa74d96414e packages/ledger/tests/gitea-helper.test.mjs
|
||
8e838a5333cbe6aafda5d72b810eb2840c4439973841d9c2bf5b55bc660455a3 packages/ledger/tests/ledger.test.mjs
|
||
033e6d5fdd4626fb0ca1c981b6d85eba961a4618a303faf2bb7b30ae024ed9a6 scripts/gitea-api.sh
|
||
```
|
||
|
||
## Independent test evidence
|
||
|
||
- `node --test packages/ledger/tests/` on node v26.8.1: **20 tests, 20 pass,
|
||
0 fail** (duration ~1.4 s). Offline: fixtures are temp git repos, temp JSONL
|
||
logs, and stubbed `gitea-api.sh` on PATH; the helper regression test points
|
||
`MOSAIC_GITEA_CREDENTIAL_FILE` at a nonexistent file and stubs `node` before
|
||
any credential read. No real API call, no private-session read, no
|
||
credential access occurred during review.
|
||
- `node --check` on all four `.mjs` files: OK. `bash -n scripts/gitea-api.sh`:
|
||
OK. No trailing whitespace in any candidate file.
|
||
- Helper defect reproduced mechanically: a `cleanup() { [ -n "$X" ] && rm …; }`
|
||
EXIT trap under `set -euo pipefail` turns a successful run into exit 1;
|
||
the candidate's `if` form exits 0. Verified with an isolated script outside
|
||
the repository. The suite's GET 200 → exit 0, POST 201 → exit 0, and
|
||
GET 403 → exit 1 cases directly regression-cover the fix.
|
||
|
||
## Brief conformance
|
||
|
||
- CLI shape matches: `--since` required; `--until` defaults to today UTC;
|
||
`--json`; `--no-issues`; unknown/duplicate arguments refused (exit 1).
|
||
Sources are consulted in the brief's order: local `git log refactor`
|
||
(subjects only, range self-filtered — correct, since `git --since` prunes at
|
||
out-of-order dates), then the single Gitea call, then repo-seat session
|
||
logs.
|
||
- Table 1 columns, Table 2 board/agent/human classification by first-line
|
||
preamble, and the totals line all match the brief. Issue seats come from
|
||
`#N` in in-range user text.
|
||
- The "unknown, never a guess" rule holds everywhere it applies: missing issue
|
||
metadata, `--no-issues` aggregates, zero-denominator ratios with nonzero
|
||
human counts, negative durations, and malformed/partial session evidence
|
||
(refuses with file and line, never echoing content).
|
||
- Read-only boundaries hold: no filesystem writes; PATH mutation restored in a
|
||
`finally`; no credential reads in ledger code; API error bodies and stderr
|
||
are never echoed (tested); control characters are stripped from printed
|
||
titles; symlinked source directories are refused and symlinked `.jsonl`
|
||
files are skipped; no fleet paths; no network beyond the one Gitea call.
|
||
|
||
The five flagged choices are each implemented as documented in the README and
|
||
internally consistent, each with a covering test:
|
||
|
||
1. UTC dates inclusive at both endpoints (tested at 00:00:00.000 and
|
||
23:59:59.999, next-day exclusive).
|
||
2. Follow-ups counted in range only (`max(commits − 1, 0)`), explicitly not a
|
||
lifetime count.
|
||
3. A commit naming several issues counts once in total commits and once per
|
||
linked row (tested with `both #1 #2 #2`).
|
||
4. Zero/unknown denominators: empty report → 0 totals; human messages with
|
||
zero closed issues → `unknown`; `--no-issues` → `unknown` closed-count,
|
||
median, and ratio.
|
||
5. One Gitea call: updated-since, first page, limit 50; a full 50-row page
|
||
refuses (exit 2) rather than silently undercounting; a commit-linked issue
|
||
the query did not return still gets a row with `unknown` metadata.
|
||
|
||
## Non-blocking observations
|
||
|
||
1. `scripts/gitea-api.sh` writes the API response body to
|
||
`/tmp/gitea-api-response.$$` — predictable name, not `mktemp`, no 0600.
|
||
On a multi-user host this could expose or race a private repo's issue
|
||
content. Single-user POC host; the POST body file already uses
|
||
`mktemp` + `chmod 600`. Suggest `mktemp` here in a later pass.
|
||
2. `REPO_PATH` in `gitea-api.sh` is computed but never used; the header usage
|
||
example also references `repos/mosaicstack/stack-v2` while the canonical
|
||
repo is `mosaicstack/stack`. Cosmetic.
|
||
3. The credential file is validated once for `BASE` and read a second time in
|
||
`gen_curl_cfg` (TOCTOU window). Acceptable on this host.
|
||
4. A literal `|` in an issue title shifts text-table columns (JSON output is
|
||
unaffected; no structured-parse contract exists for the text table).
|
||
5. `readSessions` refuses a partially written final JSONL line. This is
|
||
documented in the README as intended fail-closed behavior; note it can
|
||
refuse while a seat is mid-write.
|
||
6. The `directories()` helper's specific message "Session source must be a
|
||
real directory" is always wrapped by the catch into "Cannot read ledger
|
||
directory: <path>"; the specific string is unreachable. The path is still
|
||
reported, so diagnosis is unimpaired. Cosmetic.
|
||
|
||
## Scope statement
|
||
|
||
Read-only review: hashes, `git rev-parse`/`git log`/`git diff` provenance
|
||
checks (no mutations), the test suite, and the six files. No source edits, no
|
||
git mutations, no real API or private-session reads. This verdict and the
|
||
SESSIONS.md registration are the only files written.
|