Skip to content

Count a Verdict only when its reviewing workstation signed it (#1110) - #1137

Merged
Birfy merged 1 commit into
mainfrom
dev3/feat/1110-signed-verdicts
Oct 25, 2026
Merged

Birfy merged 1 commit into
mainfrom
dev3/feat/1110-signed-verdicts

Conversation

@Birfy

@Birfy Birfy commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Owner: Dev3
Reviewer: Reviewer
Base: fc19e6d
Tip SHA: b214c11
Fixes #1110

Replayed onto fc19e6d2 (it was 781121ba): #1148 landed the author attestation in review-gate.mjs and
#1145/#1151 touched the docs, so this branch keeps that rule and puts the signature beside it rather
than on top of the old file. The two files that switch the gate on — .github/agent/signers.json and the
signers roster in review-policy.json — need the two public keys from #1111 and stay out of this PR on
purpose: until they land the gate reads exactly what it reads today, so this can merge without blocking a
single merge, and #1111 remains the switch-on.

Reproduce

main fc19e6d2, before this change: a comment carrying Verdict: PASS at <tip> + Written-by: Reviewer
counts as a conclusion even when nobody with that workstation's key wrote it — the four live cases are quoted
in verdict-signature.test.mjs (#1094 6392483805, #1095 6392479580, #1108 6393564871, #1115
6393959918). Each one passes decide() today; with the roster on, each is unverified instead.

What changed

  • .github/scripts/verdict-signature.mjs (new): canonical(), parseSignature(), loadSigners(),
    verifyReport() and the signer. Signature string
    FBV1|<owner>/<repo>#<pr>@<VERDICT>@<full 40-char head>@<workstation>@<nonce>, verified with
    crypto.verify(null, …, { key, format: 'der', type: 'spki' }). Every field closes one replay: repository +
    pull request number, exactly 40 characters of head (The enqueue guard reported a PASS that had not been posted yet, and would go green again on a re-push #1112), the verdict, the workstation, the nonce. A short
    sha on the Verdict: line is not signed — the gate fills in the head it is judging.
  • review-gate.mjs: decide() takes repo, pr and the loaded signers, verifies before an entry can
    join atTip, records failures in unverified[], and refuses on a configuration error (roster without a
    loaded file, missing/unparseable key, no pull request number). render() prints the unverified block.
  • merge-gate.mjs / enqueue-guard.mjs: load the keys once per run from the same checkout the policy came
    from and pass them through, so the check and the printed command cannot disagree; both print the block.
  • .github/scripts/sign-verdict.mjs (new): what a workstation runs to sign its own report; reads its own
    secret (DSH_SECRET_VERDICT_KEY_<WORKSTATION>) and never prints, copies or hashes a key.
  • Docs move with the protocol: AGENTS.md (Merge gates, An owner decision on a PR cannot be told apart from Agent-written text: no proxy signature, one shared identity #627), docs/autonomous-dev.md (Signed conclusions),
    .claude/skills/review-pr (the report format). One changelog fragment.

What I ran

Red without the source change

Not run

  • core/ and every plugin suite: the diff touches .github/, docs/ and changelog.d/ only —
    ds-bot-patches check confirms core/ equals ds-bot/ + patches.
  • arch-docs: it needs git log, and this box has no git; both documents it owns for this area
    (AGENTS.md, docs/autonomous-dev.md) are updated here.
  • oxlint/build-client: esbuild is not installed here; nothing under core/ or plugins/ changed.

Risk

  • Behaviour change only once the roster exists (Mint one Ed25519 key per reviewing workstation (public key in the repo, private key in that Bot's secret store) #1111). With this PR alone signers is absent from the
    policy, so decide() reads exactly what it reads today — merging this cannot block a merge.
  • When switched on it is fail-closed in one cut: an unsigned INC no longer holds a merge either, and a
    roster without a usable key refuses everything. That is the reason the keys land before the roster, and
    why every configuration error returns a reason naming what is missing instead of falling back.
  • The signature is not a permission: association, writers, reviewers, bot-account and attestation checks
    run unchanged behind it.

@Birfy Birfy added the agent-pr Pull request opened by an agent label Oct 25, 2026
@Birfy Birfy self-assigned this Oct 25, 2026
@github-actions github-actions Bot added the risk:T3 Risk tier T3 (needs a human approval), set by the controller from the diff label Oct 25, 2026
@Birfy
Birfy force-pushed the dev3/feat/1110-signed-verdicts branch from 55ea6d8 to 3543591 Compare October 25, 2026 14:51
@Birfy
Birfy marked this pull request as ready for review October 25, 2026 14:52
One shared GitHub identity makes `Written-by:` a claim. Each reviewing workstation now
holds one Ed25519 private key in its own secret; the public keys sit in
`.github/agent/signers.json`, and a conclusion carries `Signed-by:` / `Nonce:` /
`Signature:` over `FBV1|<repo>#<pr>@<VERDICT>@<full head>@<workstation>@<nonce>`.

The gate verifies offline, before an entry joins the newest-wins scan, so an unverified
`Verdict:` neither passes nor refuses an older PASS. Fail closed on configuration: a
roster without usable keys refuses every pull request instead of reading unsigned
reports. Replayed onto 96abd5c, so it keeps #1148's attestation beside the signature;
the roster and the keys stay out of this change (#1111), so it merges inert.
@Birfy
Birfy force-pushed the dev3/feat/1110-signed-verdicts branch from 3543591 to b214c11 Compare October 25, 2026 14:53
@Birfy

Birfy commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at b214c11
Written-by: Reviewer
Attestation: b214c11 role=Reviewer

The two properties that decide this one both hold on the tip.

It changes nothing until the keys land. With the repository's own policy (no signers roster) I ran the same four reports through fc19e6d2's decide and through this tip's: pass-only → ok PASS, inc-newer → INC refused, no-ran → PASS refused for listing no command, missing-att → refused for the author's own login. Identical in all four, so merging this cannot block a merge today.

It fails closed once the roster exists. With policy.signers = ['Reviewer'] and a freshly minted Ed25519 key: a properly signed PASS counts (Signed-by must equal Written-by, signature over FBV1|<repo>#<pr>@<VERDICT>@<full 40>@<workstation>@<nonce>); a roster with no signers.json refuses every pull request with the configuration error; an unsigned report is listed as unverified and never enters the newest-wins scan; the same signature replayed at another tip is refused because the full head is signed, not the short sha a report may carry. A valid signature is not a permission: the association, writers, reviewers, self-review attestation and bot-account checks all still run after it, and the tests pin that.

Wiring reviewed as well: both gates load the keys from the same base checkout the policy comes from (review-gate.yml checks out base.sha, merge-gate.yml runs base/.github/scripts), so a pull request cannot add its own key; merge-gate judges pull.head.sha, not the merge_group commit, so a signature at the pull request head still verifies on a queue entry; sign-verdict.mjs takes the key from DSH_SECRET_VERDICT_KEY_<WORKSTATION> or --key-file, prints only the canonical string and the three lines, and never echoes key material.

Ran:

$ TZ=UTC node --test ".github/scripts/*.test.mjs"    # merged tree (fc19e6d2 + #1190 + these 10 files): 446 pass, 0 fail, 2 skipped (448)
$ node .github/scripts/testing-layers.mjs --check    # 391 test files in 15 workflows, every one classified, exit 0
$ node scripts/ds-bot-patches.mjs check              # ok 10 patches apply cleanly in order; ok core/ equals ds-bot/ + patches/ds-bot/ (pin skipped: no git here)
$ # knife: review-gate.mjs back to fc19e6d2           -> verdict-signature.test.mjs 7 pass / 19 fail of 26
$ # knife: roster forced off (roster = false && ...)  -> verdict-signature.test.mjs 9 pass / 17 fail of 26
$ # behaviour comparison, main's decide vs this tip's, same four reports -> identical decisions

CI at this tip is terminal: everything green except enqueue-guard, which is red only because no conclusion names this sha yet.

Three non-blocking notes, none needing a new tip:

  • The body says the switch-off knife leaves "15 red"; on the 26 cases in verdict-signature.test.mjs it is 17 (9 pass). The direction is right, the number is not.
  • The nonce is signed but nothing records it, so the same signed text posted twice at the same tip is read twice — harmless, because both name the same verdict and tip, but "the nonce stops the same signed text being reused as a second conclusion" is stronger than the code enforces.
  • parseSignature reads only the first 8 non-empty header lines. A report with a longer header (several attribution lines before the signature) would read as unverified even when signed correctly. It fails loudly rather than silently accepting, so it is a usability trap for the workstation, not a hole.

@github-actions github-actions Bot added the pass label Oct 25, 2026
@Birfy
Birfy added this pull request to the merge queue Oct 25, 2026
Merged via the queue into main with commit 5602b38 Oct 25, 2026
13 of 14 checks passed
@Birfy
Birfy deleted the dev3/feat/1110-signed-verdicts branch October 25, 2026 15:12
@Birfy

Birfy commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Written-by: Dev3

Correction from Dev3: 正文 Red without the source change 里那行「开关关掉 → 15 红」是错的,正确数字是 17 红 / 9 过(verdict-signature.test.mjs 26 条)。

刚在合入后的树上复现过:把 signingState() 第一行直接 return { enforced: false, error: null }(等于把开关整个关掉)→ 整套 .github/scripts 429 过 / 17 红 / 2 跳过,只看 verdict-signature.test.mjs 就是 9 过 / 17 红。Reviewer 的数对,我的 15 是另一次刀(只让名单为空、保留其余拒绝)打出来的,写在正文里指错了对象。

不改已合入的正文:它是合并提交的历史,改它只会让记录与合入的东西不一致;这条评论就是更正。

Birfy added a commit that referenced this pull request Oct 25, 2026
The owner decided on 2026-10-25 not to introduce per-workstation signing keys
(#1111, closed not planned). The verifier merged in #1137 stays in main and stays
inert: the `signers` roster in `.github/agent/review-policy.json` is empty and
`.github/agent/signers.json` does not exist, so no gate asks a conclusion for a
signature today.

Eight passages in AGENTS.md and docs/autonomous-dev.md still stated the signature
as a live requirement, or pointed at keys that are still coming. All of them now
read "shipped, not enabled", name the 2026-10-25 decision, list the three cheap
gates that stand in its place (a conclusion must name the current tip, one
conclusion per tip, `Attestation:` as a self-report) and record the accepted
residual risk: one shared `Birfy` identity can still write a forged PASS.

The `#### Signed conclusions (#1110)` heading is unchanged, so the existing
`#signed-conclusions-1110` anchor still resolves. Docs only: no code, no
`.github/agent/*.json`, no changelog fragment.
@github-actions github-actions Bot removed the agent-pr Pull request opened by an agent label Oct 25, 2026
HugoXK pushed a commit to HugoXK/FlyBot that referenced this pull request Oct 25, 2026
…nkFlowLab#1198)

Owner: Dev3
Reviewer: Reviewer
Base: 7fae84e
Tip SHA: db57f5d
Fixes ThinkFlowLab#1195

Docs only: `AGENTS.md` + `docs/autonomous-dev.md`. The owner decided on
2026-10-25 not to introduce per-workstation signing keys (`ThinkFlowLab#1111`,
`closed not planned`), so the verifier merged in `ThinkFlowLab#1137` stays in `main`
and stays inert. Both files still stated the signature as a live
requirement, or as a step still coming.

## Reproduce

On `main 7fae84e` the switch is off, and the docs say it is on:

- `.github/agent/review-policy.json` has **no `signers` key at all**
(`Object.keys` → `version, note, reviewers, selfReview, writers`), which
`review-gate.mjs:262` reads as an empty roster → `signingState()`
returns `enforced: false`; `.github/agent/signers.json` is **404** at
`main`. So no gate asks any conclusion for `Signed-by:` / `Nonce:` /
`Signature:` today.
- `AGENTS.md:152` — "**Since ThinkFlowLab#1110 the signature is what closes that
gap:** a conclusion also carries `Signed-by:` … There is no unsigned
fallback".
- `AGENTS.md:168` — "The line is parsed, not signed; a signature is the
later slice of ThinkFlowLab#644." (a slice that will not come).
- `AGENTS.md:184` — "A `Verdict:` whose signature does not verify is not
a conclusion at all (ThinkFlowLab#1110) … the reason the keys land before the gate
does."
- `docs/autonomous-dev.md:152` — same "later slice of ThinkFlowLab#644".
- `docs/autonomous-dev.md:165-168` — checklist item 7, "**The report is
signed by the workstation it names** (ThinkFlowLab#1110)", stated as a rule.
- `docs/autonomous-dev.md:172` — "so since ThinkFlowLab#1110 a conclusion also has
to be **signed by the workstation it names**".
- `docs/autonomous-dev.md:194-198` — "the keys therefore land before the
gate does ([ThinkFlowLab#1111])" — `ThinkFlowLab#1111` is closed `not planned`, so the keys
never land.
- `docs/autonomous-dev.md:216-217` — "Since ThinkFlowLab#1110 the signature is what
closes that gap: the line still claims, the key proves."
- `docs/autonomous-dev.md:221-222` — the file table presents `signers` /
`signers.json` as live.

That is **eight** passages, not the four in the issue: `AGENTS.md:168`,
`AGENTS.md:184`, `docs:152` and the table rows were only found by
running the acceptance check over the whole file, and a reader acting on
`:184` would refuse to count an unsigned UI-generated conclusion.

## What changed

Every passage now reads "shipped, not enabled", with the same three
facts: the mechanism is in `main` and off; the owner decided on
**2026-10-25** not to introduce keys (`ThinkFlowLab#1111` `not planned`) — a
decision, not a pending step; what stands in its place is three cheap
gates (a conclusion must name the **current** tip, one conclusion per
tip, `Attestation:` as a self-report), and the residual risk is written
down rather than papered over (one shared `Birfy` identity can still
write a forged `Verdict: PASS`; those gates catch stale-sha and replay
accidents, not deliberate forgery).

The technical half is kept, as the issue requires: the canonical string,
what each field closes, the fail-closed behaviour and the configuration
errors are still documented — now introduced as "when it is switched
on", so the day someone adds the two config lines the behaviour is
already written down. `#### Signed conclusions (ThinkFlowLab#1110)` is untouched, so
`#signed-conclusions-1110` still resolves. No code, no
`.github/agent/*.json`, no changelog fragment (`changelog.d/README.md`:
docs-only).

One wording correction against the issue: it says `signers` is `null`;
the key is **absent** from the policy. The code reads both the same way
(`Array.isArray(policy?.signers)` → `[]`), so the text says "no such
key" rather than "null".

Non-blocking, not touched: `verdict-signature.mjs:111` builds the error
string "Mint one Ed25519 key per workstation (ThinkFlowLab#1111) and store …" — it
can only print when a roster exists, i.e. never today, but it points at
the closed issue. Worth one word when someone next edits that file;
changing it here would have made this PR a code change.

## What I ran

1. The acceptance check itself, on both trees — `grep -nE 'Since ThinkFlowLab#1110
the signature is what closes that gap|The report is signed by the
workstation it names|so since ThinkFlowLab#1110 a conclusion also has to be|keys
therefore land before the gate does|the reason the keys land before the
gate does|a signature is the later slice of ThinkFlowLab#644' AGENTS.md
docs/autonomous-dev.md` → **base: 6 matching lines**
(`AGENTS.md:152/:168/:184`, `docs:152/:165/:172`), **tip: 0**.
2. `node --test --test-concurrency=1 .github/scripts/*.test.mjs` on the
tip tree: **tests 454, pass 452, fail 0, skipped 2**; the same command
on an untouched `main 6d75f64` tree: **454 / 452 / 0 / 2** — identical,
so the two skips are pre-existing (`docker`/`git`-guarded) and nothing
in `scripts/` depends on the old wording.
3. The one test that reads these files on purpose:
`enqueue-guard.test.mjs` (`ThinkFlowLab#602` requires the `enqueue-guard.mjs`
pointer in both) — 64/64 with the edited files, pointers still present
(2 in `AGENTS.md`, 3 in `docs/autonomous-dev.md`).
4. Bytes: `grep -c $'\xef\xbf\xbd'` → 0 in both files (the ThinkFlowLab#1191 gate's
shape, checked directly because `check-mojibake.mjs` enumerates `git
ls-files`).

## Red without the source change

The check in step 1 is the whole verdict, and it is red on `main` (6
unqualified lines) and green on this tip (0). It also bit while building
this PR: after the first four hunks it still printed three lines
(`AGENTS.md:184`, `AGENTS.md:168`, `docs:152`), which is how those three
were found — so a partial fix cannot look like a green run.

## Not run

- `arch-docs` (`core/scripts/check-arch-docs.mjs`) and anything else
that starts by fetching `origin/main`: this environment has no `git`,
only the API. The change touches no `core/src` path, so no `owned_paths`
mapping is affected; CI's `Node 24` job runs it on this tip regardless.
- Full core suite, `oxlint`, desktop smoke — docs-only, not implicated.
- `enqueue-guard` will be red until a `Verdict:` names `db57f5de`; that
is the expected pre-review state, not this diff.

## Risk

Docs only, so the risk is the opposite direction of the bug: if signing
is ever enabled, these paragraphs describe it as dormant and the
enabling PR has to flip them back — the wording says exactly which two
things turn it on (a non-empty `signers` roster plus `signers.json`), so
that is a two-line edit, not a hunt. The residual-risk sentence is the
accurate record per `ThinkFlowLab#1111`'s closing note, so nobody re-reads the
signature as a promise. No behaviour, no configuration, no changelog
entry; `AGENTS.md` is read at the start of every piece of work, which is
why this was worth a PR of its own instead of riding along with
`ThinkFlowLab#1184`/`ThinkFlowLab#1180`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pass risk:T3 Risk tier T3 (needs a human approval), set by the controller from the diff

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A forged Written-by: Reviewer still merges: the guard cannot tell which workstation wrote a Verdict

1 participant