docs(ledger): Gate F T3 thread source brief R2, Filbert approved (#1506)
Brief e8300cb6 (Darkwing), review bb02d8d3 (Filbert, approve with three nits), R1 record and R1-to-R2 diff. Lead rulings in item 14. Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
Claude Opus 5.5
@@ -0,0 +1,317 @@
|
||||
# Gate F brief: Filbert's review of the T3 ledger source
|
||||
|
||||
Reviewer: Filbert, 2026-09-26. Requested by Sage.
|
||||
|
||||
Candidate: `docs/plans/2026-09-26_ledger-t3-source.md`, sha256
|
||||
`08959a05574264e4f8243a90af94746e73a2fde3706f22e38f4ff1105b7a45a8`. I
|
||||
verified the hash. The file is uncommitted and has no code.
|
||||
|
||||
I reviewed it against Sage's rulings, not as open choices:
|
||||
1. on by default, where a missing or unreadable database exits 1 and the
|
||||
message names `--no-t3`;
|
||||
2. the `t3:unmapped` row stays;
|
||||
3. the 14 free-text Discord Bot headers stay as recorded;
|
||||
4. case-insensitive classes (the 6a fix) are folded in.
|
||||
|
||||
**Verdict: revise.** The design is sound, and the counts check out against
|
||||
the live database. Three findings would make the build's tests fail or
|
||||
depend on the host, and each needs a text change:
|
||||
- one acceptance check expects the wrong result (W1);
|
||||
- the "never writes" claim is inaccurate (W2);
|
||||
- existing tests would read the real T3 database (F1).
|
||||
|
||||
The rest are smaller additions. None needs a new ruling from Sage or Jason.
|
||||
|
||||
## What I checked
|
||||
|
||||
- **Scratch WAL tests** with `node:sqlite` on Node 26.8.1 (SQLite 3.53.4),
|
||||
in `/tmp`.
|
||||
- **The live database**, opened read-only with `mode=ro`. I read the schema,
|
||||
project roots, thread titles, and message counts per thread. I read no
|
||||
message text except the first line of each user message, which the
|
||||
classifier runs over. I printed text only for my own thread's one human
|
||||
message.
|
||||
- **`packages/ledger/src/ledger.mjs` and `tests/ledger.test.mjs`** at HEAD.
|
||||
|
||||
The brief's numbers hold:
|
||||
|
||||
| Check | Live result |
|
||||
|---|---|
|
||||
| Discord Bot user messages | 68 |
|
||||
| Typed-in-app (human) messages, outside imported and deleted threads | 113 = the brief's 99 + 14 |
|
||||
| Agent messages with the class fix | 101 = the brief's 80 + 16, plus 5 sent since 20:54Z |
|
||||
| Journal and locking mode | `wal` and `normal` |
|
||||
| The three required tables | the columns the brief names, all present |
|
||||
|
||||
The `orchestration_events` diagnostic query costs about 30 ms on the 394 MB
|
||||
database.
|
||||
|
||||
## 1. WAL and the read-only open (the part Sage asked about first)
|
||||
|
||||
The core claim holds. In my test, a `mode=ro` reader saw all three rows
|
||||
while they were still only in the `-wal` file, with the writer attached.
|
||||
`immutable=1` did worse than "lose the newest messages": with the table
|
||||
created inside the WAL, it failed with `no such table`. So the brief's ban
|
||||
on `immutable=1` and on copies is right.
|
||||
|
||||
Two of the brief's statements are wrong, though, and one acceptance check
|
||||
would fail as written.
|
||||
|
||||
**W1. `-wal` without `-shm` does not fail. (Blocking.)** I made that case by
|
||||
copying a SIGKILLed writer's `-wal` without its `-shm`.
|
||||
- A `mode=ro` open succeeded, read all three rows, and created the missing
|
||||
`-shm` itself.
|
||||
- It failed, with SQLite error 14 ("unable to open database file"), only
|
||||
when the directory was not writable.
|
||||
- Adding `readonly_shm=1` didn't help. It made both this case and the
|
||||
clean-stop case fail, and it still created a `-wal`.
|
||||
|
||||
So the acceptance line "Each fail-closed case above has its own test,
|
||||
including … `-wal` without `-shm`" expects a refusal that doesn't happen.
|
||||
Change it to two tests:
|
||||
- **`-wal` without `-shm`, directory writable:** the read counts the WAL
|
||||
rows.
|
||||
- **`-wal` without `-shm`, directory not writable:** exit 1, naming
|
||||
`--no-t3`.
|
||||
|
||||
Also fix line 142, which lists this case as a refusal.
|
||||
|
||||
**W2. The reader can create files in `~/.t3/userdata`. (Blocking as a
|
||||
claim.)** Line 157 says "The source never writes to the database", and line
|
||||
160 says the one outside effect is read locks in `-shm`. Line 44 says "no
|
||||
locks", which contradicts line 160. What actually happens:
|
||||
- **T3 stopped cleanly:** `-wal` and `-shm` are gone. A `mode=ro` open
|
||||
creates an empty `-wal` and a 32 KiB `-shm`, and leaves both after it
|
||||
closes. The main file is untouched, and the read works even with the
|
||||
directory read-only.
|
||||
- **`-wal` without `-shm`:** the reader rebuilds the WAL index into a new
|
||||
`-shm`.
|
||||
|
||||
This is standard SQLite behaviour, and T3 opens normally afterwards.
|
||||
Replace the claim with the accurate one:
|
||||
- the reader never writes the main database file;
|
||||
- it may create or update `-wal` and `-shm` beside it, as any SQLite
|
||||
connection does.
|
||||
|
||||
The T3-stopped test should then assert:
|
||||
- the counts are correct;
|
||||
- the main file's bytes are unchanged.
|
||||
|
||||
**W3. One read transaction, and a busy timeout.** The brief runs several
|
||||
queries (schema, project, threads, messages, the diagnostic's events) while
|
||||
T3 writes.
|
||||
- In autocommit mode, each statement gets its own snapshot. Wrap the whole
|
||||
read in `BEGIN` … `COMMIT` (a deferred read transaction), so every query
|
||||
sees one committed state.
|
||||
- Set `DatabaseSync`'s `timeout` (a few seconds). Then a transient
|
||||
`SQLITE_BUSY`, for example while T3 runs a checkpoint or WAL recovery,
|
||||
waits instead of failing Gate F. A `BUSY` after the timeout is exit 1,
|
||||
like any other open failure.
|
||||
|
||||
**W4. Build the URI with `pathToFileURL`, not string concatenation.** In my
|
||||
test, a string `file:<path>?mode=ro` worked on Node 26.8.1. But a `?`, `#`
|
||||
or `%` in the home path would break it. `new URL` plus `searchParams` avoids
|
||||
that.
|
||||
|
||||
**The two untested cases, as acceptance checks.** With W1 and W2 applied,
|
||||
both are proper checks: a fixture can create each state deterministically.
|
||||
- **Stopped:** close a WAL database cleanly.
|
||||
- **`-wal` without `-shm`:** SIGKILL a child writer that set
|
||||
`wal_autocheckpoint=0`, then delete `-shm`. Copying the `-wal` before any
|
||||
checkpoint also works.
|
||||
|
||||
Add one more: the newest message is in the WAL while the writer holds the
|
||||
database open. The brief has this ("One test opens a database whose newest
|
||||
message is still in the WAL"). Say that the writer is still attached,
|
||||
because that is the live-T3 case.
|
||||
|
||||
## 2. Thread-to-seat title rule
|
||||
|
||||
Checked live:
|
||||
- The five 2026-09-26 seat threads have `title_state_json.source` set to
|
||||
`manual`.
|
||||
- "Darkwing in Claude" and "Dewey in Claude" predate that field (null).
|
||||
- T3 also generates titles itself: the table has `title_regeneration_*`
|
||||
columns and `needsRefinement`.
|
||||
|
||||
**How the rule can misassign:**
|
||||
1. **An auto-generated title.** If Jason opens a thread without naming it,
|
||||
T3 titles it from his first prompt. A title like "Rocko review of the
|
||||
plan" maps to rocko.
|
||||
2. **A rename.** Titles are current state. Renaming a thread moves its
|
||||
whole history to another row.
|
||||
3. **A seat's thread titled for a topic.** For example, "review: queue"
|
||||
drops that seat's messages into `t3:unmapped`.
|
||||
|
||||
Misassignment never changes the Human total or the human-per-closed ratio.
|
||||
It only moves counts between rows. It does matter for Gate F, which reads
|
||||
one seat's row.
|
||||
|
||||
**T1. Add a header cross-check. (Required.)** The headers already carry the
|
||||
answer. Every agent header addressed to a seat thread names the recipient
|
||||
and that thread's own id. Live, every one agrees with the title rule:
|
||||
|
||||
| Thread | Headers addressed to it |
|
||||
|---|---|
|
||||
| Sage | 39, all `to: sage` |
|
||||
| Darkwing | 14, all `to: darkwing` |
|
||||
| Filbert | 18, all `to: filbert` |
|
||||
| Dewey | 15, all `to: dewey` |
|
||||
| Rocko | 15, all `to: rocko` |
|
||||
|
||||
Rule: a user message whose header matches 6a, and whose `to:` id is the
|
||||
message's own thread, must name the seat the title rule gave that thread.
|
||||
The same goes for an unmapped thread receiving a seat-addressed header. On
|
||||
a mismatch, the report refuses with exit 1 and names the thread id and both
|
||||
roles. This uses only message text, not T3 metadata, so it fits the brief's
|
||||
principle that `origin` decides nothing.
|
||||
|
||||
It catches:
|
||||
- a seat thread renamed to another seat;
|
||||
- a seat thread renamed to a topic, once any agent writes to it;
|
||||
- a Jason-started thread auto-titled with a seat name that agents then
|
||||
address by a different seat.
|
||||
|
||||
It doesn't catch a thread that no agent ever writes to. That case can only
|
||||
add human counts to a seat row, never hide them, so for Gate F it errs
|
||||
toward a visible failure. Say so in the README.
|
||||
|
||||
**T2. List the mapping in the JSON.** Give each seat's T3 thread ids and
|
||||
titles, and the unmapped thread ids. A reader of a Gate F result can then
|
||||
see which threads made up the row.
|
||||
|
||||
**T3. Test the rule** with these fixtures:
|
||||
- a same-title thread in another project (the brief has this);
|
||||
- a seat thread renamed to another seat, which must refuse under T1;
|
||||
- a title that starts with a seat name but not followed by a space ("Sagebrush"),
|
||||
which stays unmapped;
|
||||
- "Researcher", a real directory with no thread today.
|
||||
|
||||
## 3. The fail-closed departure
|
||||
|
||||
The reasoning is right, and it follows Sage's ruling 1. A missing `.pi`
|
||||
directory means no Pi seat ran here. A missing or changed T3 database on a
|
||||
host that uses T3 means the evidence moved, and a zero there would be the
|
||||
false pass that Gate F exists to catch. Refusing on a missing table or
|
||||
column, a missing project or more than one project, a bad row, or a symlink
|
||||
is consistent with how the Pi reader refuses malformed JSONL.
|
||||
|
||||
**F1. The existing tests would read the real T3 database. (Blocking.)**
|
||||
- `tests/ledger.test.mjs` spawns `cli.mjs` with `env: { ...process.env, … }`,
|
||||
so `HOME` is the developer's.
|
||||
- With the source on by default, every existing test would open the real
|
||||
`~/.t3/userdata/state.sqlite`, find no project for the temp fixture root,
|
||||
and exit 1.
|
||||
- On a host without T3, they would exit 1 too.
|
||||
|
||||
The brief needs one explicit way to point the reader at a fixture:
|
||||
- either a fixture `HOME` in the spawned environment, or a documented
|
||||
override such as `--t3-db PATH`;
|
||||
- an acceptance line saying no test opens the real `~/.t3`. For example,
|
||||
the tests set `HOME` to a temp dir for every run.
|
||||
|
||||
Existing tests that aren't about T3 should pass `--no-t3` or get an empty
|
||||
fixture database. Say which.
|
||||
|
||||
**F2. Symlink check.** The brief checks `state.sqlite` and `~/.t3/userdata`.
|
||||
Check `~/.t3` as well, because the Pi reader checks every ancestor from the
|
||||
root down ("Check every source ancestor"). Today all three are real
|
||||
directories or files.
|
||||
|
||||
**F3. Match the repository root exactly.** Compare `workspace_root` byte for
|
||||
byte with the ledger's root. The CLI gets that root from the realpath of its
|
||||
own URL. `~/src/mosaic-stack-dev-test` is a compatibility symlink to this
|
||||
checkout. A T3 project opened through it would not match, and then "no
|
||||
project row" is the correct refusal. The README should say this.
|
||||
|
||||
**F4. Name the blind spot.** Live, there is a T3 project at `/home/jwoltje`
|
||||
and a deleted one at `/mnt/storage/src`. A thread in either could work on
|
||||
this repository and would not be counted. The workspace-root rule is still
|
||||
right, but the README's exclusions should name this case.
|
||||
|
||||
**F5. When the diagnostic's table is missing.** The diagnostic (ruling 3)
|
||||
reads `orchestration_events`. Say what happens if that table or its columns
|
||||
are missing:
|
||||
- The report prints the diagnostic as `unknown`, and the counts are
|
||||
unaffected, because the diagnostic decides nothing.
|
||||
- Or add the table to the required schema and refuse.
|
||||
|
||||
I'd take the first, because a T3 change to internal metadata shouldn't stop
|
||||
the counts. Either way, the brief should say which.
|
||||
|
||||
## 4. The class fix and the counts
|
||||
|
||||
The fix is correct and scoped as Sage ruled. HEAD's
|
||||
`packages/ledger/src/ledger.mjs:81` (tmux) and `:83` (T3) both have
|
||||
`class=[a-z-]+`. Make both case-insensitive, not only the T3 one. The
|
||||
acceptance line "an uppercase class counts as agent after the fix and as
|
||||
human before it" should cover both preambles. One thing the count doesn't
|
||||
show: the only human-classified message in my thread is Jason's opening
|
||||
assignment (19:31:07Z). With the fix, all 18 agent headers to Filbert
|
||||
classify as agent.
|
||||
|
||||
## To reach approve
|
||||
|
||||
- **W1:** correct the `-wal`-without-`-shm` expectation and the line-142
|
||||
refusal; split it into writable and non-writable tests.
|
||||
- **W2:** replace the "never writes" and "no locks" statements with the
|
||||
accurate effect; the stopped test asserts the main file's bytes are
|
||||
unchanged.
|
||||
- **W3:** one read transaction and a busy timeout. **W4:** build the URI
|
||||
from a URL.
|
||||
- **T1:** the header cross-check, refusing on a conflict. **T2:** the
|
||||
mapping in the JSON. **T3:** the extra fixtures.
|
||||
- **F1:** a fixture path and no test opening the real `~/.t3`. **F2–F5:**
|
||||
the ancestor symlink check, an exact root match, the blind spot in the
|
||||
README, and the diagnostic when its table is missing.
|
||||
- **§4:** the class fix covers both preambles.
|
||||
|
||||
Send the revision with its hash and I'll review the delta.
|
||||
|
||||
## Correction (Filbert, 2026-09-26, after R2)
|
||||
|
||||
W2 said the clean-stop read "works even with the directory read-only". That
|
||||
was wrong, and Darkwing's measurement is right. My test for that case reused
|
||||
a database that the previous test had already opened, so its leftover `-wal`
|
||||
(0 bytes) and `-shm` were still present. It was not a clean stop. I re-ran it
|
||||
with a truly clean stop (no `-wal`, no `-shm`) in a non-writable directory
|
||||
on Node 26.8.1 / SQLite 3.53.4. The open fails with errcode 1544, "attempt
|
||||
to write a readonly database". So a stopped T3 in a non-writable directory
|
||||
is exit 1, as R2 expects. The rest of W2 stands: the reader never writes the
|
||||
main file, and it may create `-wal` and `-shm`.
|
||||
|
||||
## R2 delta review
|
||||
|
||||
Candidate: `docs/plans/2026-09-26_ledger-t3-source.md`, sha256
|
||||
`e8300cb6abea70819aba7cf10040d19b4d6019b5663c37203209537a5f10ee62`. I
|
||||
verified Darkwing's delta files against `manifest.sha256`:
|
||||
- `r1.md` is `08959a05…45a8`, the R1 I reviewed;
|
||||
- `r1-to-r2.diff` is `aa4740ae…71fc`.
|
||||
|
||||
I read R2 in full.
|
||||
|
||||
**Verdict: approve** `e8300cb6`. Every finding is answered:
|
||||
- W1 and W2: the section 1 table, the accurate claim about side files, and
|
||||
split tests.
|
||||
- W3 and W4: one transaction, a 5 s timeout, and `pathToFileURL`.
|
||||
- T1: as asked, plus Darkwing's addition that an unmapped thread can't
|
||||
receive an own-id header naming a seat.
|
||||
- T2 and T3: the mapping in the JSON, and the fixtures.
|
||||
- F1: `--t3-db`, `HOME` set at both spawn sites, and a no-database test.
|
||||
- F2–F4: done. F5 follows Sage's ruling.
|
||||
- §4: both preambles.
|
||||
|
||||
Three nits, none blocking. Carry them into the build, or fix them in an R3
|
||||
and I'll confirm the hash:
|
||||
1. **The measurement is settled.** Lines 71–74 and 279–282 treat the
|
||||
non-writable clean stop as disputed. I've corrected my review above: it
|
||||
fails with 1544. State it as agreed. Make the test a plain exit-1
|
||||
assertion, and drop the "if the build reads there instead" branch.
|
||||
2. **Real path in the fixture project row.** The fixture's project row
|
||||
should store `fs.realpathSync(root)`. The CLI takes its root from the
|
||||
realpath of `cli.mjs`, and a temp directory under a symlinked `/tmp`
|
||||
would otherwise fail with "no project row".
|
||||
3. **Say which database was read.** `--t3-db` can point a real Gate F run at
|
||||
any file. The JSON should record the database path it read and whether it
|
||||
was the default. The text report should add one line when it wasn't. A
|
||||
Gate F result then can't come from a fixture without saying so.
|
||||
Reference in New Issue
Block a user