Repository navigation
fix(pass-label): tell a reviewer whose PASS the policy does not count - #797
Merged
Merged
Conversation
`verdictOf()` refuses a PASS whose `Written-by:` names someone outside `writers` in
`.github/agent/review-policy.json`, and `planComment()` turns that refusal into
`{ add: false, reason: 'not a PASS by a writer the policy lists' }`: the reason is printed
to a log and nothing else happens. So a reviewer added to the team writes a careful PASS,
the gates refuse it, nobody enqueues a T2/T3 pull request that "has a PASS", and the pull
request waits with every check green — the case that cost #646 an hour of review (#767).
The job already holds `issues`/`pull-requests: write` (it adds the `pass` label) and it
already reaches exactly this decision, so the note belongs here:
- `strangerPassOf()` keeps the refused PASS as a value of its own — a `Verdict: PASS at
<sha>` from a repository member whose `Written-by:` is not in `writers`. A missing
`Written-by:` line is not this case: there is no one to tell.
- `strangerCommentBody()` names the author, the tip, the file to edit (`writers` in
`.github/agent/review-policy.json`) and that the file is T3, so the edit does not go into
the reviewing pull request. It says the conclusion does not count, not that the pull
request cannot merge — a listed writer's PASS still can.
- The marker `<!-- pass-label: writer-not-listed --> tip:<sha>` makes it one note per tip:
a second run adds nothing, and a moved tip may speak once about the new one. The thread
is read only when a stranger's PASS is really on the table.
- `planCommentRun()` is the whole decision for one `--comment` run — label, note, or
nothing — and `main()` only executes it, so the choice is tested without a network.
No label is ever added for this case, no label is removed, `reviewed tip` stays report-only
(the workflow's `continue-on-error` is untouched), `review-policy.json` is not edited, and
`tests-ok`'s `needs` are unchanged.
Pinned by #767: a stranger's PASS leaves one note naming the author, the tip and the file;
the note is idempotent per tip and a moved tip speaks once; everything that is not a
stranger's PASS stays silent (a listed writer, an INC, a passer-by, a missing `Written-by:`,
a correction); and one run picks label, note, or silence, never both. Red without the
change, at this tip: returning `write: 'none'` instead of the note → 9/10, only red *one run
picks the label, the note, or silence*; dropping the marker check → 8/10, red the
idempotency case and that same one; treating a listed writer as a stranger → 9/10, only red
*everything that is not a stranger PASS stays silent*.
Part of #767.
A report may name its tip with 12 characters or with 40, and the note's marker carried the tip verbatim, so the same commit described both ways left two comments. Compare the tip already in a marker with the passing one the way `namesHead()` compares a report's tip with a head, and pin it with a case that writes the note from a 12-character tip and then judges a 40-character one. Part of #767.
Collaborator
Author
|
Verdict: PASS at 517be9b Reviewed the delta from The defect the fix closes was mine: with whole-string marker comparison, one commit named by 12 characters in one run and by 40 in the next left two notes. Checked in all four directions — a 12-character note suppresses a 40-character tip and the reverse, a 7-character note suppresses 12 and 40, and a different commit still speaks once. The 7-character case is ambiguous when two commits share the prefix, but that is the rule The note still cannot be mistaken for a conclusion: Ran (local gate on the PR tree at this tip; CI could not start — all nine checks are 0-step failures with "The job was not started because your account is locked due to a billing issue", so the evidence is local, Node v22.23.3, TZ=UTC):
Not run: Out of scope and tracked as #801: a conclusion posted as a review body starts no run, so a stranger's PASS written that way stays silent. This PR does not try to close that door, and adding |
Birfy
added a commit
that referenced
this pull request
Oct 23, 2026
Written-by: Dev3 Fixes #799 The ledger already stores one row per billed attempt; there was no reader outside the Usage page, so #796 had no number to compare a saving against. `core/dev/token-report.mjs` is that reader: per Bot, per Session and per day it prints calls, fresh input, cache read, cache write, output, and their sum as a **separate** `prompt` column, the per-call prompt percentiles (nearest rank), the top Sessions by prompt, and each Session's prompt growth between consecutive calls. A prompt that shrinks inside a Session starts a new run instead of counting as a huge negative delta. It opens the ledger read-only, defaults `--db` to `$DSH_HOME/usage.sqlite`, and exits non-zero naming the path when it is missing. The first report is `docs/reports/2026-10-23-token-spend.md`: 16,407 calls over 2026-10-20T07:34Z..10-23T02:06Z, prompt 2,117,885,549 of which **96.4% is cache read**, per-call prompt p50 129,242 / p90 210,628 / p99 244,885, and a median turn delta of **+804** tokens. The composition split (fixed overhead / history / tool results / task) is **not measured**: it needs dumped request bodies and real provider replays, which this host does not have, so those rows say `not measured` rather than show an estimate as a measurement. ## Reproduce On `main` there is nothing to run — `node core/dev/token-report.mjs` is `ERR_MODULE_NOT_FOUND` — and the ledger is unreadable outside the UI. The behaviour under test is the report's arithmetic and its input handling, which the new test builds from a fixture ledger. ## What changed - `core/dev/token-report.mjs` — the CLI (`--db`, `--from`, `--to`, `--tz`, `--bot`, `--top`, `--growth`, `--json`). - `core/test/token-report.test.js` — a fixture ledger; hand-computed sums per Bot, per Session and per day; nearest-rank percentiles; an unowned Session staying under its Session id; growth with a compaction boundary; the window, `--bot` name and Session-prefix filters; the missing-`--db` exit; impossible dates; a 25-hour DST day; and a check that every table row of the committed report still carries its provenance. - `docs/reports/2026-10-23-token-spend.md` — the report, its sample, its method, the exact commands, and a `Source` column on every table (`ledger` / `derived: <formula>` / `not measured`). - `docs/subsystems/ds-bot-core.md` — the page that owns `core/dev/` now names the script. - `changelog.d/799-token-report.added.md` — one fragment. - `patches/ds-bot/files/dev/token-report.mjs`, `patches/ds-bot/files/test/token-report.test.js` — the two new `core/` files, stored whole by `generate`. ### Input fixes after review (`ae94644e`) 1. **An impossible date is an error, never an empty report.** `Date.parse` returning `NaN` (or `Date.UTC` normalising `2026-02-31` to 3 March) used to bind as NULL, match no row, and print `calls 0` with exit 0 — the worst answer for a report about where the money went. Month, day and time are range-checked, and an offset date that does not parse names its value and exits 1. 2. **`--to` on a bare date steps a calendar day, not 24 hours.** The day a zone falls back is 25 hours long, so `+ 86_400_000` stayed inside it: `--to 2025-11-02` with `America/New_York` ended at the start of that day and dropped both its `01:30` records. The next day is now computed from the parsed year, month and day. The test pins the bound itself, not just "the two differ": `--to 2025-11-02` in `America/New_York` must equal `1762145999999` (`2025-11-03T04:59:59.999Z`), `--to 2025-11-01` must equal `1762055999999` — 25 hours apart — and the two records at `15:00Z` and `17:30Z` must land inside the day. ## What I ran (at `e91ce70c`) - `TZ=UTC DSH_HARNESS=/opt/flybot node --import ./test/harness-resolve.mjs --test test/token-report.test.js` → **5/5**. - The same command over `test/usage.test.js test/usage-data.test.js test/token-report.test.js` → **17/17**. - The report against a read-only copy of this host's ledger reproduces the committed numbers: `--from 2026-10-20T07:34:24Z --to 2026-10-23T02:06:18Z --tz Asia/Shanghai` gives Dev `3,183 / 8,787,133 / 430,347,136 / 1,688,487 / 439,134,269`, the same as the report's row (the total differs by one call only because a CLI date carries no milliseconds). - `node scripts/ds-bot-patches.mjs check` → `ok ds-bot/ = Fei2411/DeepSeek-Bot@2b10c70`, `ok 10 patches apply cleanly in order`, `ok core/ equals ds-bot/ + patches/ds-bot/`. - `node .github/scripts/changelog.mjs check` → `3 fragment(s) ... well formed`. - `node core/scripts/check-arch-docs.mjs` → `arch-docs: OK (7 changed file(s))`. ## Red without the source change Each mutation was applied to the committed tree, run, and reverted; only the named test reddens: - the script moved away → **2/3 fail** (both CLI tests; only the report-provenance test passes). - `cache_read` dropped from the select → test 1 fails (totals no longer reconcile). - `cache_read` summed into `input` → tests 1 and 2 fail (the separation assertion). - the missing-`--db` guard removed → test 2 fails. - the `owners` join dropped → tests 1 and 2 fail (the unowned Session, and every Bot, disappears). - the date range checks and the `NaN` check removed → **only the impossible-date test fails**. - `endOfDay` reverted to `+ 86_400_000` → **only the DST test fails**. ## Not run - The full `core` suite here: this box's install has no `jsdom` (two files fail identically on unmodified `main`) and default test concurrency spawn-EAGAINs; at `--test-concurrency=2` the suite ran 1279 tests, 1276 passed, the 3 reds unrelated to this diff. CI's `test` job is the authority. - `npm run lint` (oxlint is not installed here) and actionlint (it downloads from github.com, which this box cannot reach). - No provider call: the composition replay needs a provider key and dumped `llm-tap` bodies, neither of which exists here. The report labels that section `not measured`. - CI on this tip is expected to be the billing lock (0-step jobs), like every other open PR. ## Risk - Low: a dev-only script, its test and a report. Nothing under `core/src/`, no prompt, notice, roster, memory or tool-output change, no workflow change, no `client.js`. - The numbers are a snapshot of this host's ledger and will differ on another host; only the fixture is asserted, the snapshot is not. - `node:sqlite` is experimental, so the CLI prints one `ExperimentalWarning` on stderr (Node 22 and 24); the exit status and stdout are unaffected. - The report's provenance test parses the tables of that one committed file: a later table added without a `Source` column fails it on purpose. - `main` moved to `245881c4` (`#800`, `#797`) after this branch was cut; neither touches any of this PR's 7 files, and the pull request still reads `mergeable: true`.
Birfy
added a commit
to Birfy/FlyBot
that referenced
this pull request
Oct 23, 2026
…ng (ThinkFlowLab#801) A conclusion whose Written-by names someone outside the policy's writers is refused by review-gate and merge-gate. ThinkFlowLab#797 made pass-label say so for a comment; a review body fired no workflow at all, so the reviewer saw nothing. A separate read-only workflow now warns. Rebased onto main 10e26bb after the ThinkFlowLab#808 lint fix landed.
Birfy
added a commit
to Birfy/FlyBot
that referenced
this pull request
Oct 23, 2026
…ng (ThinkFlowLab#801) (#23) A conclusion whose Written-by names someone outside the policy's writers is refused by review-gate and merge-gate. ThinkFlowLab#797 made pass-label say so for a comment; a review body fired no workflow at all, so the reviewer saw nothing. A separate read-only workflow now warns. Rebased onto main 10e26bb after the ThinkFlowLab#808 lint fix landed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Written-by: Dev
Tip SHA: 517be9b
Fixes #767
What changed
.github/scripts/pass-label.mjs: a PASS whoseWritten-by:names someone outsidewritersnow leaves one comment per tip naming the author, the tip and the file to edit (.github/agent/review-policy.json→writers), carrying the marker<!-- pass-label: writer-not-listed --> tip:<sha>for idempotency. New pure functions:strangerPassOf()(the refused PASS as a value of its own),strangerCommentBody(),planStrangerComment()andplanCommentRun()(the whole decision for one--commentrun: label, note, or nothing);main()only executes that decision..github/scripts/pass-label.test.mjs: five cases — a stranger's PASS leaves one note naming author, tip and file while still adding no label; the note is idempotent per tip and a moved tip may speak once; the same commit written as a 12-character tip and then as a 40-character one is one note, not two; everything that is not a stranger's PASS stays silent (a listed writer, an INC, a passer-by, a body with noWritten-by:, a correction); one run picks label, note or silence, and the thread is read only when a stranger's PASS is on the table.changelog.d/767-pass-label-stranger.fixed.md.Why
review-gaterefuses the conclusion andmerge-gaterefuses the enqueue, both naming the defect — but the reviewer who wrote the PASS is told nothing, and nobody enqueues a T2/T3 pull request that "has a PASS". That is exactly what happened to #646: Reviewer2's independent PASS was discarded and the pull request sat with every check green.planComment()already made this exact decision and threw it away as{ add: false, reason: 'not a PASS by a writer the policy lists' }, so the loud failure belongs in the job that owns the answer.The note does not claim the pull request cannot merge: a listed writer's PASS still can. It says the conclusion does not count and names the one edit that makes the next one count, including that the policy file is T3. No label is added or removed,
reviewed tipstays report-only (the workflow'scontinue-on-erroris untouched),review-policy.jsonis not edited, andtests-ok'sneedsare unchanged.What I ran
At this tip (base
fd205c40, Node 24.10.0,TZ=UTC), in.github/scripts:node --test pass-label.test.mjs→ 11/11.node --test pass-label.test.mjs review-gate.test.mjs merge-gate.test.mjs enqueue-guard.test.mjs→ 104/104 (run in a tree that also carriesmain's.github/workflows,.github/agentandAGENTS.md, which those cases read).node .github/scripts/changelog.mjs check→ well formed;node .github/scripts/testing-layers.mjs --check→ 315 test files in 12 workflows, all classified.Red without the change, each cut alone at this tip:
write: 'none'instead of the comment → 10/11, only red one run picks the label, the note, or silence — and never both.planStrangerComment()→ 9/11, red the note is idempotent per tip, and a moved tip may speak once and that same run case.if (listed.has(...)) return nullremoved) → 10/11, only red everything that is not a stranger PASS stays silent.namesHead()→ 10/11, only red one note per commit, whether the tip is written 12 characters or 40.Not run on this box:
npm run lint(no oxlint here), the whole.github/scriptssuite (its other cases readcore/,docs/andCHANGELOG.md, which this working tree does not carry — I ran the four files above), smoke and Desktop. CI runs them at this tip; the repository's Actions are currently blocked by the account billing lock, so this tip will be re-run once that is lifted.Risk
T1 (
.github/scripts/**only — a comment from a job that already holdsissues: write). The label path is unchanged; the new path writes one comment, never a label, and only for a PASS that the gates already refuse.