feat(ledger): Piece E, queue section in the weekly ledger (row 13, #1508)
The ledger prints a queue section above the weekly table. It checks four things: - open issues named by done rows; - owner registrations for active rows; - closed issues for done rows; - the age of required rows. The result is fail, incomplete or reduced pass. It uses its own Gitea budget of the open list plus at most 10 lookups. A full open page counts only while an issue in some row's closes has no known state (lead decision 40). T3 seats are exempt per run with --unsupported-runtime. The weekly routine is in packages/ledger/README.md. Built by Darkwing (build.patch ab1f12ca, manifest 0b20bbca). Filbert reviewed it: round 1 81f26f2e asked for changes (C1, ISO requiredSince never aged); round 2 ce8ce150 approved. Also carries Filbert's plan amendment for decision 40 (68a25ffe). Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
Claude Opus 5.5
@@ -137,6 +137,10 @@ Changed:
|
||||
`packages/ledger`, so A leaves a one-line hand-written pointer to the
|
||||
weekly routine below the markers. E moves the routine into
|
||||
`packages/ledger/README.md` and removes the pointer.
|
||||
*Amended by lead decision 40 (2026-09-27):* row 7 stays in the queue
|
||||
until Jason closes it, because its gate is his. E still writes the
|
||||
weekly routine into the ledger README. A left no pointer below the
|
||||
markers, so E has none to remove.
|
||||
- `docs/TOOLS.md`: usage lines. This file carries other owners' uncommitted
|
||||
changes; add a scoped patch the way #1511 did.
|
||||
|
||||
@@ -167,6 +171,8 @@ Changed:
|
||||
that was row 7).
|
||||
- One more Gitea call, `state=open`, one page; a full page fails as today
|
||||
(Q7). Referenced issues absent from that list count as closed.
|
||||
*Superseded by 8.10:* absent issues are looked up, and a full page is
|
||||
handled as lead decision 40 rules.
|
||||
- Reads `docs/plans/queue.json` through the queue validator by relative
|
||||
import (`../../queue/src/queue.mjs`). There are no workspaces, so a bare
|
||||
`@mosaic/queue` import would not resolve (R14).
|
||||
@@ -371,6 +377,10 @@ items have no owner or issue.
|
||||
*Decided:* row 7 moves to `packages/ledger/README.md` as the weekly
|
||||
routine (sequenced across A and E; see 2.A). The parked items become rows
|
||||
with state `parked`, owner `unassigned` and issue null.
|
||||
*Amended by lead decision 40 (2026-09-27):* row 7 stays until Jason
|
||||
closes it. Its gate is Jason's, and closing a Jason-gated row needs his
|
||||
cited approval, so the lead can't retire it alone. E writes the weekly
|
||||
routine into `packages/ledger/README.md` anyway.
|
||||
|
||||
## 5. Where the brief contradicts the code or itself
|
||||
|
||||
@@ -1741,7 +1751,7 @@ source is unchanged.
|
||||
| Calls | Purpose |
|
||||
|---|---|
|
||||
| 1 | Metrics, unchanged. |
|
||||
| 1 | Queue checks: `state=open`, limit 50. A full page makes the queue issue checks "incomplete". |
|
||||
| 1 | Queue checks: `state=open`, limit 50. A full page makes the queue issue checks "incomplete" only when some issue in a row's `closes` is left without a known state (lead decision 40). |
|
||||
| up to 10 | Queue checks: `GET issues/N` for issues in some row's `closes` that are neither in the open list nor shown closed in the metric page. Issues beyond 10 are "unknown (budget)", and the run is incomplete. |
|
||||
|
||||
That is at most 12 calls. `--no-issues` makes 0 calls and prints `queue
|
||||
@@ -1784,8 +1794,9 @@ days (legacy lower bound)". Until then its age is undecidable.
|
||||
`missing`, `invalid` or `pid-gone` owner, and any issue or age violation.
|
||||
- `incomplete`: no known violation, but something couldn't be decided. That
|
||||
covers:
|
||||
- an issue check that was `unknown (budget)`, hit a full page, or was not
|
||||
run;
|
||||
- an issue check that was `unknown (budget)`, or was not run;
|
||||
- a full open page while some issue in a row's `closes` has no known
|
||||
state;
|
||||
- a `pid-unknown` owner;
|
||||
- an undecidable legacy age.
|
||||
- `reduced pass`: nothing known and nothing undecided. The liveness evidence
|
||||
@@ -2411,3 +2422,10 @@ The round-2 modifications:
|
||||
with no reviewer approvals, so a Jason-gated row could close without its
|
||||
reviewers. That move now carries the in-review→done checks on a comment
|
||||
round; the table and 8.9's receipts paragraph say so.
|
||||
- 2026-09-27: amended for lead decision 40 (Piece E questions). A full
|
||||
`state=open` page makes the queue issue checks incomplete only when some
|
||||
issue in a row's `closes` is left without a known state. An issue off
|
||||
the page is looked up or left `unknown (budget)`, so truncation can't
|
||||
hide a violation. 8.10's budget table and result list say so. Row 7
|
||||
stays in the queue until Jason closes it; Q9 and 2.A say so. Section 2's
|
||||
E text points to 8.10.
|
||||
|
||||
@@ -0,0 +1,168 @@
|
||||
# Queue Piece E review, round 1 (#1508, row 13)
|
||||
|
||||
Filbert, 2026-09-27. Plan: `queue-as-data-plan-2026-09-26.md` §8.10,
|
||||
amended for lead decision 40 (plan sha256 68a25ffe…, uncommitted).
|
||||
|
||||
## Verdict
|
||||
|
||||
**Changes requested, one item (C1).** The review covers the amended
|
||||
round-1 candidate: `build.patch` sha256
|
||||
02a01c291890626f58ba103d1c65e853b042b609887868bf59dea7f9b50b227d at
|
||||
2333d837, with `build-manifest.sha256` cad51929… and `build.md` d3bdb826….
|
||||
I had also reviewed the first version (patch 78445322…, manifest
|
||||
8f0e4fea…) in full. The amendment changes only the full-page rule, its
|
||||
tests and the README, so everything I checked on the first version still
|
||||
holds.
|
||||
|
||||
C1 is a real bug in the age check, and it's small. Everything else is
|
||||
ready, including the decision-40 change.
|
||||
|
||||
## C1. The age check never flags a row made required after genesis
|
||||
|
||||
**Where:** `packages/ledger/src/queue-checks.mjs`, line 200 in the amended
|
||||
file:
|
||||
|
||||
```js
|
||||
const days = ageDays(Date.parse(`${r.requiredSince}T00:00:00Z`), now);
|
||||
if (days > AGE_LIMIT_DAYS) add(violations, 'age', ...
|
||||
```
|
||||
|
||||
**What happens.** The code assumes `requiredSince` is a date. Genesis
|
||||
rows carry dates (rows 9, 10, 11 and 13 have `2026-09-13`). But the queue
|
||||
writes a full ISO time for any row made required later:
|
||||
`set <id> required true` sets `requiredSince = entry.at`
|
||||
(`packages/queue/src/queue.mjs:860`), and `add --required` does the same
|
||||
(line 904). The validator accepts both forms (line 322, `checkTime(…,
|
||||
{ date: true })`). For an ISO time the template gives
|
||||
`2026-09-01T12:00:00.000ZT00:00:00Z`, `Date.parse` returns NaN, `days`
|
||||
is NaN, and `NaN > 14` is false. The row is never an age violation, and
|
||||
nothing is reported as undecided either.
|
||||
|
||||
**Reproduced** in-process at 2026-09-27T12:00Z on one required,
|
||||
in-progress row:
|
||||
- `requiredSince: "2026-09-01"` → violations `owner-invalid, age`, result
|
||||
`fail`;
|
||||
- `requiredSince: "2026-09-01T12:00:00.000Z"` → violations `owner-invalid`
|
||||
only. The age violation is missing.
|
||||
|
||||
(`owner-invalid` comes from passing no seats; it doesn't matter here.)
|
||||
|
||||
**Why it matters.** Today's rows all have dates, so Monday's run is right.
|
||||
The first row made required through the queue CLI would slip past the
|
||||
14-day gate without a word. A check that goes quiet is the failure the
|
||||
ledger's result levels exist to prevent.
|
||||
|
||||
**Fix.**
|
||||
- A `DATE_RE` value parses as `T00:00:00Z`; anything else goes to
|
||||
`Date.parse` as it stands.
|
||||
- If the result is NaN, fail closed: an `age` undecided item naming the
|
||||
row, or a violation. Never silence. (The validator should make this
|
||||
unreachable, but the check shouldn't depend on that.)
|
||||
- Tests: an ISO `requiredSince` 15 days old fails and one 14 days old
|
||||
doesn't, next to the existing date cases. Your fixtures build
|
||||
`requiredSince` with `since(n)`, so an ISO variant fits in the same test.
|
||||
- Mutant: restore the old template. The new test should kill it.
|
||||
- The README's age paragraph can say that `requiredSince` is a date for
|
||||
genesis rows and an ISO time for later ones.
|
||||
|
||||
## The full page (lead decision 40)
|
||||
|
||||
I agree with the ruling, and the amendment implements it correctly. An
|
||||
issue is never treated as closed because it's missing from the open list.
|
||||
It's looked up or left `unknown (budget)`, and the unknown state already
|
||||
makes the run incomplete. A server that caps pages below 50 would make
|
||||
`full` meaningless anyway, so dropping it as a standalone signal costs
|
||||
nothing.
|
||||
|
||||
What I checked on the delta:
|
||||
- `open-list-full` is added only when the page is full and some issue in
|
||||
`wanted` is `unknown`. `wanted` is the union of every row's `closes`,
|
||||
including rows that aren't done. An unknown issue on a pending row can
|
||||
only affect a disposition, but it still keeps the page undecided. That
|
||||
errs toward `incomplete`, and it matches the decision's wording ("some
|
||||
wanted issue"). Keep it.
|
||||
- The new end-to-end test drives the fake helper through `issueStates` and
|
||||
`queueChecks` for all three cases: resolved, open off the page, and past
|
||||
the budget.
|
||||
- The README's result list and call table match the code.
|
||||
- My plan now says the same thing: 8.10's budget table and result list,
|
||||
a pointer from section 2's E text, and Q9 and 2.A for row 7.
|
||||
|
||||
## What I checked
|
||||
|
||||
All of this ran in scratch clones with push disabled: `/tmp/fqe` at
|
||||
f304eaa5 for the first version, and `/tmp/fqe2` at 2333d837 for the
|
||||
amendment. The inputs were frozen as 0444 copies.
|
||||
|
||||
- **Manifest and suites.** The amended manifest checks 5/5. `node --test
|
||||
packages/ledger/tests/` passes 76/76, and `packages/queue/tests` with
|
||||
`packages/seat/tests` passes 161/161. The first version had passed
|
||||
75/75 and 161/161.
|
||||
- **Source.** I read `queue-checks.mjs` in full, and the diffs to
|
||||
`cli.mjs`, the README and both test files. I compared the amendment with
|
||||
the first version file by file. Only `queue-checks.mjs` (the rule and its
|
||||
comment), the README (two passages) and `queue-checks.test.mjs` changed.
|
||||
- **Local run** on the first version, for 2026-09-27 with `--no-issues
|
||||
--no-t3` and three seats declared: 0 violations, result `incomplete`
|
||||
(issues not run), 18 protected changes.
|
||||
- **Mutations.** I wrote 21 mutants of my own against the first version
|
||||
(E1–E21), apart from Darkwing's 37. The suite kills 19. The two
|
||||
survivors are equivalent:
|
||||
- E2 checks the metric page before the open list. Both are current
|
||||
sources, so they can't disagree about an issue.
|
||||
- E8 applies the age check to rows that aren't required. The validator
|
||||
refuses `requiredSince` on such a row, so the mutant changes nothing.
|
||||
|
||||
I wrote five more against the amendment (F1–F5), and the suite kills
|
||||
all five:
|
||||
- F1: a full page is always undecided;
|
||||
- F2: an unknown issue makes `open-list-full` without a full page;
|
||||
- F3: any state other than open counts as unresolved;
|
||||
- F4: only `unknown (budget)` counts, so a failed lookup doesn't;
|
||||
- F5: it takes two unknown issues.
|
||||
|
||||
None of my 26 mutants touches the age parsing. The C1 bug is in an input
|
||||
shape the fixtures don't use.
|
||||
|
||||
## Darkwing's choices
|
||||
|
||||
I agree with choices 1 to 11 in `build.md`. Two of them depart from the
|
||||
plan's table, and in both cases the change fails closed:
|
||||
- 4: a malformed pid is `invalid`, not `pid-unknown`, so it's a violation
|
||||
rather than undecided;
|
||||
- 5: a registration for another checkout is `missing`, because it matches
|
||||
the seat name but not the root.
|
||||
|
||||
Choice 11 (protected changes listed, never checked) is what R4, R10 and J2
|
||||
asked for.
|
||||
|
||||
## Non-blocking
|
||||
|
||||
- **n1. The call deadline kills the helper, not curl.** `execFileSync`
|
||||
with a 60-second `timeout` sends SIGTERM to `gitea-api.sh` only. I
|
||||
checked the behaviour with a script that runs a child `sleep`:
|
||||
`execFileSync` throws `ETIMEDOUT` at the deadline, so the ledger's wall
|
||||
time stays bounded, but the child keeps running as an orphan. For the
|
||||
ledger that means a stuck curl can outlive the run, and the helper's
|
||||
response file may be left in `/tmp`. It's a GET, so nothing is written
|
||||
to Gitea. D wrapped its helper call in `timeout -s KILL` for this.
|
||||
Doing the same here is a small change. It can go in this round or
|
||||
later.
|
||||
- **n2. `closedInMetric` could also check `state === 'closed'`.** It uses
|
||||
`closed_at` alone. I haven't checked whether Gitea clears `closed_at`
|
||||
when an issue is reopened. If it doesn't, a reopened issue that is off
|
||||
the open page but on the metric page would count as closed. The open
|
||||
page is always full now, so that case can happen. Checking `state` as
|
||||
well settles it either way.
|
||||
- **n3. The unknown-budget message doubles a word.** The test pins
|
||||
`unknown (unknown (budget))`. It would read better as `unknown (budget)`
|
||||
or `unknown (lookup budget)`. Cosmetic.
|
||||
- **n4. `--unsupported-runtime` is self-declared.** The flag is a
|
||||
statement from whoever runs the ledger, not evidence. It's printed on
|
||||
every run, which is what 8.10 asks for. Nothing to change.
|
||||
|
||||
## For Darkwing and Sage
|
||||
|
||||
Fix C1 with its test and mutant, then send the patch and manifest again.
|
||||
I'll review round 2 against C1 only, plus n1 if it's in. My plan
|
||||
amendment (68a25ffe…) goes into E's commit with this review.
|
||||
@@ -0,0 +1,101 @@
|
||||
# Queue Piece E review, round 2 (#1508, row 13)
|
||||
|
||||
Filbert, 2026-09-27. Round 1: `queue-e-review-r1-2026-09-27.md`
|
||||
(sha256 81f26f2e…). This round checks C1, n1, n2 and n3.
|
||||
|
||||
## Verdict
|
||||
|
||||
**Approved.** The review covers `build.patch` sha256
|
||||
ab1f12cad711284f8a722ea51fa73cd8e344c703701f8b76957ae33de091ae84 at
|
||||
2333d837, with `build-manifest.sha256` 0b20bbca… and `build.md`
|
||||
75571f0b…. C1 is fixed, and so are n1, n2 and n3. Two of my mutants
|
||||
survive. Neither hides a defect; see the notes below. Nothing needs a
|
||||
round 3.
|
||||
|
||||
## What I checked
|
||||
|
||||
All of this ran in a scratch clone, `/tmp/fqe3`, at 2333d837 with push
|
||||
disabled, on frozen 0444 copies of the three inputs. Darkwing's `r1/`
|
||||
copies still match the hashes I reviewed in round 1.
|
||||
|
||||
- **Manifest and suites.** The manifest checks 5/5. `node --test
|
||||
packages/ledger/tests/` passes 78/78, and `packages/queue/tests` with
|
||||
`packages/seat/tests` passes 161/161.
|
||||
- **What changed.** I compared all five files with the round-1 candidate.
|
||||
`cli.mjs` and `ledger.test.mjs` are unchanged. `queue-checks.mjs`, the
|
||||
README and `queue-checks.test.mjs` change only for C1, n1, n2 and n3.
|
||||
- **C1.**
|
||||
- `requiredDay` floors `Date.parse` to 00:00Z of its UTC day. A bare date
|
||||
parses as 00:00Z, so the separate date branch I suggested would add
|
||||
nothing. Dropping it is right.
|
||||
- Counting an ISO time from its UTC day rather than its hour is a change
|
||||
from my fix, and I agree with it. Both forms age in whole UTC days. A
|
||||
row can be flagged up to a day early, never late.
|
||||
- A value that doesn't parse is an `age-invalid` violation, so the run
|
||||
fails. Any finding in `violations` counts toward the result, and no
|
||||
other code matches on the check name, so the new name needs no other
|
||||
wiring.
|
||||
- The tests cover an ISO time at 15 days (fails), at 14 days (passes),
|
||||
and at 23:59Z fifteen days back (fails, which needs the floor), and
|
||||
month 13 (`age-invalid`, result fail).
|
||||
- **n1.** Each call runs as `timeout -s KILL 60 gitea-api.sh GET …`. Without
|
||||
`--foreground`, GNU timeout signals the whole process group, which kills
|
||||
curl too. The new test starts a helper with a hanging child and a 1 s
|
||||
deadline, then checks that both pids are gone. Adding `--foreground`
|
||||
leaves the child alive, and the test catches it (R7).
|
||||
- **n2.** Metric-page evidence needs `state === 'closed'` and a
|
||||
`closed_at`. The metric call returns the raw Gitea records, filtered but
|
||||
not mapped (`ledger.mjs` `readIssues`), so `state` is there in real runs.
|
||||
The fixture covers a reopened entry (`closed_at` only) and one with
|
||||
`state` only. Both are looked up.
|
||||
- **n3.** The message reads `is unknown (over the lookup budget)`.
|
||||
- **Mutations.** I wrote 13 mutants of my own for this round. The suite
|
||||
kills 11:
|
||||
- R1: no floor on `requiredDay`;
|
||||
- R2: the NaN guard removed;
|
||||
- R3: `age-invalid` counted as undecided;
|
||||
- R4: metric evidence on `closed_at` alone;
|
||||
- R5: metric evidence on `state` alone;
|
||||
- R6: the default TERM signal in place of KILL (caught by the message);
|
||||
- R7: `--foreground` (caught by the orphan check);
|
||||
- R8: a kill recognised only by exit 137;
|
||||
- R10: exit 127 no longer read as "unavailable";
|
||||
- R11: `ceil` in place of `floor`;
|
||||
- R12: a signal-killed call treated as success.
|
||||
|
||||
Two survive:
|
||||
- **R9**, a kill recognised only by `r.signal === 'SIGKILL'`. Without
|
||||
`--foreground`, GNU timeout sends KILL to its own group and dies with
|
||||
it, so `spawnSync` sees the signal, not exit 137. The `status === 137`
|
||||
branch is defensive and can't be reached in this setup. The mutant is
|
||||
equivalent.
|
||||
- **R13**, `if (r.error) throw r.error;` removed. With `timeout` missing
|
||||
from PATH, the call still fails, but the message says "credential or
|
||||
Gitea request failure" rather than "the timeout command is
|
||||
unavailable". That's still a refusal (exit 2); only the wording is
|
||||
wrong. See n1.
|
||||
|
||||
## Non-blocking
|
||||
|
||||
- **n1. The missing-`timeout` message has no test (R13).** A test could
|
||||
point `PATH` at an empty directory for one call and give the helper by
|
||||
absolute path. That's optional. The failure is closed either way.
|
||||
- **n2. A day past the end of the month doesn't parse as invalid.** V8
|
||||
turns `2026-02-30` and `2026-02-30T00:00:00.000Z` into 2026-03-02. It
|
||||
returns NaN only for values like month 13. So `age-invalid` catches some
|
||||
impossible dates but not all. A row whose date overflows ages from the
|
||||
wrong day and gets no warning. This belongs with Darkwing's follow-up
|
||||
(a): the queue validator checks shape, not calendar. A calendar check
|
||||
there, such as a round trip through `toISOString`, closes both. The CLI
|
||||
never writes such a value, and readQueue refuses hand edits, so it can't
|
||||
happen today.
|
||||
- **Darkwing's follow-ups.** I agree with both:
|
||||
- (a), above;
|
||||
- (b), the metric call's orphan curl in `ledger.mjs`, the same fix as
|
||||
n1 in round 1.
|
||||
Neither is E's scope.
|
||||
|
||||
## For Darkwing and Sage
|
||||
|
||||
E is approved as it stands. My plan amendment (68a25ffe…) and both review
|
||||
files go into E's commit.
|
||||
Reference in New Issue
Block a user