From fa536b6a8f68d2e390ceffaa3e7e10e673e15254 Mon Sep 17 00:00:00 2001 From: lidge-jun Date: Sat, 29 Aug 2026 04:13:27 +0900 Subject: [PATCH 1/2] docs(devlog): bug-PR zero-remaining campaign roadmap + executable review gate Docs-only work-phase for a campaign whose goal is that no open pull request labeled `bug` remains on this repository. Adds the plan unit and one script. The unit records live triage of every bug PR (four parallel review lanes, each claim carrying its command and output) and the merge lane each PR belongs to: clean merge, rebase, reimplementation, or current-head re-audit. The finding that reorganized the whole campaign is that `dev` was trailing its own published channel. package.json said 2.35.0 while v2.36.0-preview.20260829 was already released, so tests/release-version-line.test.ts failed on every commit descending from dev - red inherited by six bug PRs whose own diffs never touch release tooling. Rebasing them onto an unrepaired dev could not have fixed them, so the version line became the first work-phase (#2836) and the rest of the campaign sequences behind it. scripts/ci/assert-mergeable-review.sh is the executable form of the MAINTAINERS.md approval policy. It exists because an earlier draft of this plan promised in prose not to bypass review with --admin, and prose is not a gate: the live dev ruleset has dismiss_stale_reviews_on_push false and admins can bypass pull-request requirements outright. The script exits nonzero unless one review is simultaneously the reviewer's latest state, APPROVED, bound to the exact current head, authored by someone other than the PR author, and authored by an account parsed live out of the MAINTAINERS.md maintainer table - and it refuses outright if any maintainer's latest state is CHANGES_REQUESTED. Its first version was wrong in a way worth recording (002_audit_round3_synthesis): it matched any historical APPROVED row, so a maintainer who approved a commit and then requested changes on the SAME commit still read as approval. An independent audit found it. The fix collapses history to each reviewer's latest substantive state and drops an `|| true` that had turned a failed API lookup into a verdict of "no approvals". Verification: bun run privacy:scan Privacy scan passed bash -n scripts/ci/assert-mergeable-review.sh exit 0 gate regression harness (fake gh, no network) 5/5 scenarios superseded exit 1, concurrent-blocker exit 1, pagination failure exit 2, non-maintainer exit 1, clean maintainer approval exit 0 live: #2836 exit 1, #2798 exit 0, #2812 exit 1, #2638 exit 1 Nothing in the build, typecheck, or test path reads from devlog/, and the script is not wired into CI: it is a pre-merge tool this campaign runs before each merge. --- .../260829_bugpr_zero_remaining/000_plan.md | 136 ++++++++++++++++ .../001_audit_round1_synthesis.md | 150 ++++++++++++++++++ .../002_audit_round3_synthesis.md | 86 ++++++++++ .../010_wp8_version_line_keystone.md | 95 +++++++++++ .../020_wp2_lane_a_clean_merges.md | 73 +++++++++ .../030_wp3_lane_b_stale_rebase.md | 34 ++++ .../040_wp4_lane_c_oauth_429_rotation.md | 60 +++++++ .../050_wp5_lane_d_reimplementation.md | 118 ++++++++++++++ .../060_wp6_lane_e_prless_bug_issues.md | 44 +++++ .../070_wp7_closeout.md | 22 +++ .../080_wp9_current_head_reaudit.md | 58 +++++++ scripts/ci/assert-mergeable-review.sh | 118 ++++++++++++++ 12 files changed, 994 insertions(+) create mode 100644 devlog/_plan/260829_bugpr_zero_remaining/000_plan.md create mode 100644 devlog/_plan/260829_bugpr_zero_remaining/001_audit_round1_synthesis.md create mode 100644 devlog/_plan/260829_bugpr_zero_remaining/002_audit_round3_synthesis.md create mode 100644 devlog/_plan/260829_bugpr_zero_remaining/010_wp8_version_line_keystone.md create mode 100644 devlog/_plan/260829_bugpr_zero_remaining/020_wp2_lane_a_clean_merges.md create mode 100644 devlog/_plan/260829_bugpr_zero_remaining/030_wp3_lane_b_stale_rebase.md create mode 100644 devlog/_plan/260829_bugpr_zero_remaining/040_wp4_lane_c_oauth_429_rotation.md create mode 100644 devlog/_plan/260829_bugpr_zero_remaining/050_wp5_lane_d_reimplementation.md create mode 100644 devlog/_plan/260829_bugpr_zero_remaining/060_wp6_lane_e_prless_bug_issues.md create mode 100644 devlog/_plan/260829_bugpr_zero_remaining/070_wp7_closeout.md create mode 100644 devlog/_plan/260829_bugpr_zero_remaining/080_wp9_current_head_reaudit.md create mode 100755 scripts/ci/assert-mergeable-review.sh diff --git a/devlog/_plan/260829_bugpr_zero_remaining/000_plan.md b/devlog/_plan/260829_bugpr_zero_remaining/000_plan.md new file mode 100644 index 0000000000..7c343bb56b --- /dev/null +++ b/devlog/_plan/260829_bugpr_zero_remaining/000_plan.md @@ -0,0 +1,136 @@ +# 260829 — Bug-PR zero-remaining campaign + +Objective: no open pull request labeled `bug` remains on `lidge-jun/opencodex`. Every +bug PR reaches a terminal disposition backed by evidence, the 429 OAuth rotation work +(#2807) lands on `dev`, matching issues close with cross-references, and important +bug issues with no PR get a reimplementation merged. + +## Constraints + +- Every change travels through a PR targeting `dev`. No direct push to `dev`. +- CI-first evidence. The repository's own `ci`, `test N/4`, `macos`, `hygiene`, and + `gates` (privacy) checks on the exact head SHA are the primary proof. The full local + `bun run test` suite is not run; when a suite is genuinely needed it runs remotely + via `ssh lidge` + `ocx-run`. +- Commits and pushes use `--no-verify`. +- `--admin` merge is available (the operator holds admin) but never substitutes for the + `MAINTAINERS.md` non-author security review on authentication, credential, OAuth, + workflow, release, or dependency surfaces. +- Unfixed security reproduction detail stays in gitignored `.tmp/`, never `devlog/`. +- Unrelated dirty worktrees and the 10 existing stashes are preserved untouched. + +## Live triage (2026-08-29, four parallel Sol-high lanes) + +**Sixteen** open PRs carry the `bug` label. The first triage pass found fourteen; the A-gate +audit found that #2744 had been missed, and #2836 (this campaign's own keystone PR) is also +`bug`-labeled. The inventory is re-queried at the start of every work-phase and again at +closeout, because the set moves while the campaign runs — see +`001_audit_round1_synthesis.md`. Every row below is live evidence from +`gh pr view`/`gh pr checks`/`gh api compare` at triage time. + +| PR | author | head SHA | mergeable | behind dev | real gates | security surface | matching issue | +|----|--------|----------|-----------|-----------|------------|------------------|----------------| +| #2835 | lidge-jun | 0dc8704531 | MERGEABLE/BLOCKED | 0 | 6 pass / 1 fail | no | none | +| #2828 | luvs01 | 019c792607 | MERGEABLE/BLOCKED (draft) | 0 | 1 pass / 9 never started | yes (grok inject credential fields) | #2830 partially | +| #2822 | luvs01 | 450b1bc60c | MERGEABLE/UNSTABLE | 1 | 6 pass / 1 fail | no | none | +| #2821 | luvs01 | d21ad61d51 | MERGEABLE/UNSTABLE | 11 | 6 pass / 3 fail | no | none | +| #2812 | gaoran1209 | 220a9048ed | MERGEABLE/BLOCKED | 77 | 5 pass / 3 fail | no | #2810 | +| #2807 | lidge-jun | 1c61a7e8cd | CONFLICTING/DIRTY | 80 | 9 pass / 0 fail | yes (OAuth core) | none (Closes #2745 is a PR) | +| #2799 | adtumk | e9a7bb7bb0 | MERGEABLE/CLEAN | 94 | 7 pass / 0 fail | no | none | +| #2798 | olddonkey | 856ad72d41 | MERGEABLE/CLEAN | 97 | 8 pass / 0 fail | no | none | +| #2797 | rrmlima | edaa044f28 | MERGEABLE/BLOCKED (draft) | 97 | 6 pass / 1 fail | yes (doctor reads env_key) | #2713 (partial) | +| #2796 | rrmlima | 2328c16c76 | MERGEABLE/BLOCKED (draft) | 97 | 6 pass / 3 fail | yes (client fingerprint) | #2717 | +| #2793 | smileBeda | 3a6e600eda | MERGEABLE/BLOCKED (draft) | 97 | 4 pass / 3 fail, unsponsored_surface | yes (78 files, auth core) | #2718 | +| #2785 | DevonGithub | 107f2cbb28 | MERGEABLE/UNSTABLE | 97 | 5 pass / 1 fail | no | none | +| #2638 | luvs01 | c8556f3703 | MERGEABLE/BLOCKED | 13 | 4 pass / 4 fail | yes (auth-context, routing) | none | +| #2497 | MarcTCruz | 86a49e8525 | CONFLICTING/DIRTY (draft) | 496 | 6 pass / 2 fail | yes (20 files, auth core) | #2221 | +| #2744 | yxr1995-maker | 1d8e35462a | CONFLICTING/DIRTY (draft) | 140 | CHANGES_REQUESTED | yes (core.ts + package.json) | none | +| #2836 | lidge-jun | befcac3e10 | MERGEABLE (wp8 keystone) | 0 | 23 pass / 0 fail, macOS queued | package.json (maintainer-authored) | n/a | + +#2638 and #2828 moved after triage: both are now zero commits behind at rewritten heads +(`375e6f8fb8`, `019c792607`), so their recorded reviews no longer describe their current +diffs. They are handled by wp9, not by the reimplementation lane. + +## The keystone: `dev` trails its own published channel + +`test 2/4`, `test 3/4`, `test 4/4`, and `macos` fail on #2835, #2822, #2821, #2796, +#2797, and #2785 with one shared assertion, not with anything those PRs changed: + +``` +release version line > the in-tree version is never behind a released one +package.json version 2.35.0 is BEHIND the highest release tag v2.36.0-preview.20260829 +``` + +Live state at triage: + +| ref | package.json version | +|-----|----------------------| +| `dev` | 2.35.0 | +| `main` | 2.35.0 | +| `preview` | 2.36.0-preview.20260829 | +| tag `v2.36.0-preview.20260829` | 2.36.0-preview.20260829 | + +The preview bump was cut on the prerelease train and never came back to `dev`, which is +the exact failure mode `tests/release-version-line.test.ts` was written for — its own +header documents the previous occurrence (repaired by hand in `32529c2b2`, when `dev` +said 2.24.2 against a published 2.26.0). + +Consequence for this campaign: rebasing a stale bug PR onto `dev` does **not** turn its +CI green, because the failure is inherited from the base. The version line is therefore +work-phase wp8 and runs FIRST; every later lane rebases onto the repaired `dev`. + +## Merge lanes + +- **wp8 keystone** — repair `dev`'s version line so inherited red turns green. +- **wp2 Lane A** — approved and CI-clean: #2799, #2798. Rebase onto the repaired `dev`, + confirm exact-head green, merge. #2798 is **security-gated**: `src/lib/destination-policy.ts` + decides whether an OAuth bearer may be sent to an overridden destination, which + `MAINTAINERS.md` covers under "other security-boundary changes" even though the hygiene + gate's restricted-path list does not name it. Its approval must be re-earned on the + rebased head. +- **wp3 Lane B** — approved but stale or inherited-red: #2822, #2821, #2785. Same + treatment; patch integrity proven with `git patch-id --stable` and `git range-diff`. +- **wp4 Lane C** — #2807, the 429 OAuth rotation work. Conflicting, 80 behind, and + carrying one live reviewer blocker (a rotated bearer can still be paired with the + previous account's accepted origin). Reimplement on current `dev` with an executable + A→429→B regression, then non-author security review. +- **wp5 Lane D** — reimplementation lane for PRs whose intent is right but whose branch + cannot land as-is: #2812 (reviewer rejected the equivalence assumption), #2796, #2797, + #2835 (host-identity disclosure in devlog), #2793, #2497, #2744. #2828 and #2638 were + moved OUT of this lane into wp9 after the audit found their reviews bound to superseded + heads. Every member that touches `src/server/responses/core.ts` (#2497, #2793, #2744) runs + AFTER wp4 and re-verifies against the accumulated file. +- **wp9 re-audit** — #2638 and #2828 at their current heads, sequenced after wp4 because + #2638 touches `src/server/responses/core.ts`. +- **wp6 Lane E** — PR-less bug issues worth reimplementing, chosen in wp1 from the 16 + open `bug` issues. +- **wp7 closeout** — issue closure with cross-references and the zero-open-bug-PR proof. + +## Verifier reality check (PLAN-VERIFIER-REAL-01) + +Corrected after the A-gate audit ran each command rather than trusting the plan. + +- **Bootstrap first:** this worktree had no `node_modules`, which made `bun x tsc --noEmit` + exit 1 with `TS2688: Cannot find type definition file for 'bun-types'` — an environment + gap, not a type error. After `bun install`, `bun x tsc --noEmit` exits 0. A fresh rebase + worktree needs `bun install` before its tests mean anything (a missing install surfaced as + `Cannot find module 'zod/v4'` on #2799's rebase). +- `gh pr checks --repo lidge-jun/opencodex` — exists, ran during triage, reads the + exact PR head. This is the campaign's primary verifier. +- `bun x tsc --noEmit` and focused `bun test tests/` — exist in `package.json` + (`typecheck`); used for local implementation loops only, after the bootstrap above. +- `scripts/ci/assert-mergeable-review.sh ` — the executable pre-merge review gate added + by this campaign. Fails closed unless an `APPROVED` review is bound to the exact current + head, authored by someone other than the PR author, and listed as a current maintainer in + `MAINTAINERS.md`. Proven non-vacuous: exit 0 on #2798 (real exact-head approval from + Ingwannu), exit 1 on #2836 and #2812. +- `bun run skill:surface:check` — the read-only verifier. `bun run skill:surface` is a + GENERATOR (`scripts/generate-ocx-skill-surface.ts` calls `writeFileSync`) and must never + be cited as a gate. +- `bun run test` (full suite) — deliberately NOT used locally per the operator's + instruction; the remote equivalent is `ssh lidge 'export PATH=$HOME/bin:$PATH; ocx-run + '`, confirmed present at + `/home/lidgeai/bin/ocx-run`. Remote evidence counts only when the recorded workdir is + proven to sit at the exact head SHA and the child command exercises the change; a bare + `rc=0` from an unrelated directory proves nothing. +- `bun run privacy:scan` — runs inside the `gates` check on every PR head. diff --git a/devlog/_plan/260829_bugpr_zero_remaining/001_audit_round1_synthesis.md b/devlog/_plan/260829_bugpr_zero_remaining/001_audit_round1_synthesis.md new file mode 100644 index 0000000000..77a7061712 --- /dev/null +++ b/devlog/_plan/260829_bugpr_zero_remaining/001_audit_round1_synthesis.md @@ -0,0 +1,150 @@ +# 001 — A-gate audit round 1: synthesis and plan amendments + +An independent Sol-high reviewer audited the roadmap against live repository state and +returned `VERDICT: FAIL` with 8 blockers. Each is recorded below with its disposition. +Two were verified independently before acceptance, because a reviewer claim is evidence to +check, not a verdict to copy. + +## B1 (Critical) — inventory was stale: 16 bug PRs, not 14. ACCEPTED + +Live query returns 16: the 14 triaged, plus **#2744** (missed) and **#2836** (the keystone +PR this campaign created, auto-labeled `bug`). + +#2744 `Recover encrypted agent tasks on the combo path before failing closed` +(yxr1995-maker, draft, CONFLICTING/DIRTY, head `1d8e35462a`) changes 4 files: +`package.json`, `src/server/responses/core.ts`, and two agent-task-recovery tests. + +Amendment: #2744 joins wp5. #2836 is wp8's own PR and needs no lane. The inventory is +re-queried at the start of every work-phase and again at closeout, because the set moves +while the campaign runs — this campaign itself proved that by adding a member. + +## B2 (Critical) — wp8 omitted review of a restricted surface. ACCEPTED WITH CORRECTION + +The reviewer is right that `package.json` is a restricted path +(`.github/scripts/pr-sponsored-surface.cjs`, under `// Dependency surfaces.`) and that +wp8's accept criteria did not mention review. + +The reviewer's implied conclusion — that #2836 would be hygiene-blocked — is WRONG, and the +live gate says so: `hygiene = pass` on #2836. The reason is in the same file: +`assessSponsoredSurface` returns `[]` immediately when `authorHasPushPermission` is true, +because a maintainer's own change carries its own sponsorship. #2836 is authored by +`lidge-jun`, who has admin. + +What survives is the governance point, and it is the stronger one: `MAINTAINERS.md` still +requires a non-author approval, and `gh pr view 2836 --json reviewDecision` returns +`REVIEW_REQUIRED` with no reviews. Self-approval is forbidden. + +Amendment to wp8 accept criteria: a fifth criterion — the merge requires a non-author +**maintainer** approval bound to the exact head. + +**Withdrawn in round 2.** The first version of this amendment allowed "or an explicit +recorded operator decision to admin-merge". The reviewer correctly identified that as the +very bypass B3 exists to close, and it is withdrawn: an alternative that permits skipping +the approval is not a gate. If the approval cannot be obtained, wp8 reports BLOCKED and the +operator decides — the campaign does not pre-authorize the bypass on their behalf. + +## B3 (Critical) — `--admin` bypasses the approval gate; the guard was prose-only. ACCEPTED + +Live `dev` ruleset: `required_approving_review_count: 1`, +`require_code_owner_review: true`, `dismiss_stale_reviews_on_push: false`, and +`current_user_can_bypass: pull_requests_only`. So an admin merge genuinely can bypass the +approval requirement, and GitHub cannot tell a security review from any approval. + +Amendment — an executable, fail-closed pre-merge check for EVERY merge in this campaign. +Round 2 rejected the first version of this amendment because it only PRINTED reviews (a +command that exits 0 on an empty list is not a gate) and because it checked only +`user != author` when `MAINTAINERS.md` requires a *maintainer*. Both points are correct and +are now fixed in code rather than in prose: `scripts/ci/assert-mergeable-review.sh`. + +It exits nonzero unless one review is simultaneously `APPROVED`, bound to the exact current +`headRefOid`, authored by someone other than the PR author, and authored by an account the +script parses out of the `## Current maintainers` table in `MAINTAINERS.md` — so the gate +cannot drift from the policy document it enforces. Merges then use +`--match-head-commit `. + +Proven non-vacuous against live PRs: + +``` +$ bash scripts/ci/assert-mergeable-review.sh 2798 +OK: #2798 approved at head 856ad72d414f27556729d70ed077e04494bb7336 by maintainer Ingwannu (author olddonkey) +EXIT=0 + +$ bash scripts/ci/assert-mergeable-review.sh 2836 +FAIL: #2836 has no maintainer approval bound to head befcac3e10ac175f9aa8de65a799abd0b5e8f7aa + maintainer roster: Ingwannu lidge-jun +EXIT=1 + +$ bash scripts/ci/assert-mergeable-review.sh 2812 +FAIL: #2812 has no maintainer approval bound to head 220a9048edc9e6715c0c4cf7f1388e26a016293e +EXIT=1 +``` + +Residual limitation, stated rather than hidden: GitHub cannot mark an approval as +specifically a *security* review, so for security-boundary PRs the reviewer's own words are +read to confirm the approval addressed the security surface. That is a human judgment the +script cannot make, and pretending otherwise would be the same error as the prose guard. + +## B4 (High) — #2638 and #2828 were assigned from stale review evidence. ACCEPTED + +Both moved since triage. Live: #2638 head `375e6f8fb8`, ahead 6 / behind 0, +`CHANGES_REQUESTED` (bound to the older `c8556f3703`). #2828 head `019c792607`, ahead 5 / +behind 0, no longer draft. + +Amendment: both leave the reimplementation lane and enter a current-head re-audit lane +(wp9). Discarding an author's branch because of a finding already fixed on a newer head +would be both wasteful and unfair to the contributor. Reimplementation stays available if +the current head still fails review. + +## B5 (High) — rebasing destroys the exact-head-approved premise. ACCEPTED + +`dismiss_stale_reviews_on_push: false` means GitHub will happily keep an approval that no +longer describes the code. The plan leaned on approvals granted to pre-rebase heads. + +Amendment: after any rebase, the approval is re-earned on the new head (B3's check enforces +it mechanically). The reviewer's falsification work is recorded as supporting the plan: it +inspected the failing logs of #2822, #2821, #2796, #2835, #2797, and #2785 and found ONLY +the `release version line` assertion — no unrelated failure. The inheritance thesis stands, +now independently confirmed and additionally proven by #2836's own `test 2/4 = pass`. + +## B6 (High) — verifier claims overstated. ACCEPTED, CAUSE CORRECTED + +The reviewer found `bun x tsc --noEmit` exiting 1 with +`error TS2688: Cannot find type definition file for 'bun-types'`. Verified: the cause was +that this worktree had no `node_modules` at all. After `bun install` (103 packages), +`bun x tsc --noEmit` exits 0. So it is a usable verifier once dependencies exist — the +plan's omission was the bootstrap step, not the command. + +Accepted without reservation: `bun run skill:surface` WRITES its target and is a generator; +the verifier is `bun run skill:surface:check`. Also accepted: `ocx-run` evidence is +meaningless unless the remote workdir is proven to be at the exact head SHA and the child +command actually exercises the change. Both are now required in the evidence format. + +## B7 (High) — missed cross-lane collisions on `src/server/responses/core.ts`. ACCEPTED + +The plan named only the `destination-policy.ts` collision. Live intersections: +`src/server/responses/core.ts` is touched by #2807 (wp4), #2497, #2638, #2793, and #2744. +`package.json` is touched by wp8 and #2744. + +Amendment: wp4 (#2807) is serialized BEFORE every other core-touching member, and each +later core-touching member re-verifies against the accumulated `core.ts` rather than +against the tree it was written on. + +## B8 (Critical) — #2798 called "no security surface". ACCEPTED + +The hygiene gate's restricted list does clear all five wp2/wp3 members +(`restricted=NONE` for #2799, #2798, #2822, #2821, #2785), so there is no gate +misclassification. But `src/lib/destination-policy.ts` decides whether an OAuth bearer may +be sent to an overridden destination, and `MAINTAINERS.md` covers "other security-boundary +changes", not just the mechanical list. Calling it non-security was wrong. + +Amendment: #2798 is security-gated in wp2 and needs a fresh exact-head security review +after rebase. #2812 (wp5), which edits the same file, inherits that classification. + +## Residual disagreement + +None outstanding. B2's gate mechanics were corrected and B6's root cause was corrected; +both underlying blockers were accepted rather than rebutted. + +## New work-phase + +wp9 — current-head re-audit lane for #2638 and #2828. diff --git a/devlog/_plan/260829_bugpr_zero_remaining/002_audit_round3_synthesis.md b/devlog/_plan/260829_bugpr_zero_remaining/002_audit_round3_synthesis.md new file mode 100644 index 0000000000..d231f5517c --- /dev/null +++ b/devlog/_plan/260829_bugpr_zero_remaining/002_audit_round3_synthesis.md @@ -0,0 +1,86 @@ +# 002 — A-gate audit round 3: the review gate had a real bug + +Round 3 returned `VERDICT: FAIL` with one Critical blocker, and it was a genuine defect in +code I had just written and called a gate. Recording it plainly, because a gate that is +trusted without being attacked is decoration. + +## The blocker (accepted in full) + +`scripts/ci/assert-mergeable-review.sh` v1 selected **any historical** `APPROVED` review at +the current head. Two reachable sequences defeated it: + +1. A maintainer approves commit `abc`, reads it again, and posts `CHANGES_REQUESTED` on the + **same** commit. v1 still reported the PR as approved — it laundered a live objection into + a green light, which is worse than having no gate. +2. One maintainer approves while another has an outstanding `CHANGES_REQUESTED`. v1 found the + approval and ignored the blocker. + +Separately, the review query ended with `|| true`, so a mid-pagination API failure kept +whatever pages had been fetched and read as "no approvals" — a failed lookup silently +degrading into a verdict. A gate that treats an error as data is not fail-closed. + +## Fix + +v2 collapses the review history to **each reviewer's latest substantive state** +(`sort_by(submitted_at, id) | group_by(user.login) | map(last)`), then: + +- refuses if any maintainer's latest state is `CHANGES_REQUESTED`, regardless of other + approvals; +- requires GitHub's own `reviewDecision == APPROVED` as a second, independent signal; +- requires a latest-state `APPROVED` bound to the exact head, by a non-author maintainer; +- drops `|| true` — every API or parse failure exits 2. + +## Non-vacuity proof + +The regression suite (`.tmp/bugpr-campaign/gate-regression.sh`, a fake `gh` on `PATH`, no +network) drives the exact cases the auditor named: + +``` +PASS superseded exit=1 FAIL: outstanding maintainer CHANGES_REQUESTED from: Ingwannu +PASS concurrent exit=1 FAIL: outstanding maintainer CHANGES_REQUESTED from: Ingwannu +PASS pagefail exit=2 FAIL: could not read reviews (API or pagination failure) +PASS outsider exit=1 FAIL: reviewDecision is 'REVIEW_REQUIRED', not APPROVED +PASS clean exit=0 OK: approved at head ... by maintainer Ingwannu +``` + +And the two logics disagree on the identical payload, which is what makes the suite +meaningful rather than self-congratulatory: + +``` +$ jq -r '[.[]|select(.state=="APPROVED")|select(.commit_id=="aaa")|.user.login]|unique|.[]' +Ingwannu <- v1: "approved" + +$ jq -r 'sort_by(.submitted_at,.id)|group_by(.user.login)|map(last)|.[]|"\(.user.login) \(.state)"' +Ingwannu CHANGES_REQUESTED <- v2: correctly blocked +``` + +Live behaviour after the fix: + +``` +#2836 exit=1 reviewDecision is 'REVIEW_REQUIRED', not APPROVED +#2798 exit=0 approved at head 856ad72d41... by maintainer Ingwannu (author olddonkey) +#2812 exit=1 outstanding maintainer CHANGES_REQUESTED from: Ingwannu +#2638 exit=1 outstanding maintainer CHANGES_REQUESTED from: Ingwannu +``` + +Note #2812 and #2638: v1 reported these as "no approval found", which was the right answer +for the wrong reason. v2 names the actual cause — a live maintainer objection. + +## A second mistake, mine, caught by the same round + +I pushed the keystone-based rebases of #2799 and #2798 to their contributor forks **before** +#2836 merged. Because the keystone commit was not yet an ancestor of `dev`, it appeared +inside those PRs' own diffs, which added `package.json` to a contributor PR and tripped +`hygiene`/`enforce-target` with `unsponsored_surface` — the contributors do not have push +permission, so the restricted path needs a sponsorship label they cannot supply. + +Both branches were restored to their original heads (`e9a7bb7bb0`, `856ad72d41`), so the +PRs are back to the state their approvals describe. The lesson is now an ordering rule: a +dependent rebase is pushed only after its base commit is an ancestor of `dev`. Verifying a +rebase locally in a scratch worktree is free; publishing it early is not. + +## Residual, stated rather than closed + +GitHub cannot mark an approval as specifically a *security* review. For security-boundary +PRs the reviewer's own words are read to confirm the approval addressed that surface. The +script cannot make that judgment and does not pretend to. diff --git a/devlog/_plan/260829_bugpr_zero_remaining/010_wp8_version_line_keystone.md b/devlog/_plan/260829_bugpr_zero_remaining/010_wp8_version_line_keystone.md new file mode 100644 index 0000000000..007883d006 --- /dev/null +++ b/devlog/_plan/260829_bugpr_zero_remaining/010_wp8_version_line_keystone.md @@ -0,0 +1,95 @@ +# wp8 — Keystone: repair `dev`'s version line + +Dependency position: FIRST. Every other work-phase rebases onto the `dev` this phase +produces, because the red `test N/4`/`macos` shards on six bug PRs are inherited from +the base and cannot be repaired by rebasing onto an unrepaired `dev`. + +## Defect + +`tests/release-version-line.test.ts` asserts the in-tree version is never behind the +highest local release tag. Live state: + +- `dev` `package.json`: `2.35.0` +- highest release tag: `v2.36.0-preview.20260829` (published 2026-08-28T16:09:12Z) +- `preview` `package.json`: `2.36.0-preview.20260829` + +`compareReleaseTags("v2.35.0", "v2.36.0-preview.20260829") < 0`, so the assertion fails +on every commit whose tree descends from `dev`. + +## Change map + +``` +MODIFY package.json "version": "2.35.0" -> "2.36.0-preview.20260829" +``` + +One line. No source, test, or workflow change. + +### Why that exact string + +Candidates were RUN through the repository's own comparator rather than reasoned about +(`.tmp/bugpr-campaign/probe.ts` importing `scripts/release-notes.ts`; positive means +ahead of the highest tag `v2.36.0-preview.20260829`): + +``` +2.35.0 -1 +2.35.1 -1 +2.36.0 1 +2.36.0-preview.20260829 0 +2.36.0-preview.20260829.1 1 +2.36.0-preview.20260830 1 +2.37.0-preview.1 1 +``` + +`2.36.0-preview.20260829` returns 0, and the test's equality branch is legal only on the +commit the tag names (`tagPointsAtHead`) — a `dev` merge commit is not that commit, so +equality fails as a duplicate claim. That leaves the strictly-ahead options, and repository +precedent decides between them: `dev` carries the next STABLE version after a release, +never a preview suffix. + +- `e4a85d134` set `dev` to `2.34.0` when it trailed a published `2.33.0`. +- `076ad3036` set `dev` to `2.35.0` immediately after `v2.34.0` shipped. +- `32529c2b2` set `dev` to `2.27.0` when it trailed `2.26.0`. + +So the value is the next stable minor: + +``` +MODIFY package.json "version": "2.35.0" -> "2.36.0" +``` + +Availability confirmed live: `npm view @bitkyc08/opencodex@2.36.0` returns +`E404 No match found for version 2.36.0`, `git tag --list v2.36.0` is empty, and the +published dist-tags are `latest=2.35.0`, `preview=2.36.0-preview.20260829`. Minor rather +than patch follows the same precedent: the range since `v2.35.0` carries behavior +changes, not only fixes. + +## Accept criteria + +1. `bun test tests/release-version-line.test.ts` passes locally on the branch (3/3). +2. Activation scenario (C-ACTIVATION-GROUNDING-01): the failing assertion is the + trigger. Before the change it fails with the quoted BEHIND message; after it passes. + Both runs are recorded. +3. The PR's own `test N/4` and `macos` checks pass on the exact head SHA. +4. `scripts/release.ts` channel logic is untouched, so no release behavior changes. +5. **Review (added after the A-gate audit).** `package.json` is a restricted path in + `.github/scripts/pr-sponsored-surface.cjs` (under `// Dependency surfaces.`). The + `hygiene` gate passes here only because `assessSponsoredSurface` short-circuits when + `authorHasPushPermission` is true, and the PR is maintainer-authored — that is an + exemption from the SPONSORSHIP label, not from review. `MAINTAINERS.md` still requires a + maintainer approval and forbids self-approval, and `gh pr view 2836 --json reviewDecision` + returns `REVIEW_REQUIRED`. + + The gate is executable, not a promise: `scripts/ci/assert-mergeable-review.sh 2836` must + exit 0 before merge, and the merge uses `--match-head-commit ` so a race + cannot land a different tree than the one verified. At the time of writing it exits 1 + (`no maintainer approval bound to head befcac3e10...`), so #2836 is NOT merge-ready. + + The audit's round-2 position is recorded and adopted: there is no admin-merge alternative + for this criterion. An earlier draft of the synthesis offered "or an explicit recorded + operator decision to admin-merge"; that is exactly the bypass the criterion exists to + close, and it is withdrawn. If the approval cannot be obtained, wp8 reports BLOCKED and + the operator decides — the campaign does not decide for them. + +## Out of scope + +- Any promotion to `preview` or `main`. +- Any change to `scripts/release.ts` or `.github/workflows/release.yml`. diff --git a/devlog/_plan/260829_bugpr_zero_remaining/020_wp2_lane_a_clean_merges.md b/devlog/_plan/260829_bugpr_zero_remaining/020_wp2_lane_a_clean_merges.md new file mode 100644 index 0000000000..ee04321a70 --- /dev/null +++ b/devlog/_plan/260829_bugpr_zero_remaining/020_wp2_lane_a_clean_merges.md @@ -0,0 +1,73 @@ +# wp2 — Lane A: approved, CI-clean bug PRs (#2798 security-gated) + +Dependency position: after wp8. These two PRs are the cheapest correct merges in the +campaign, and they are the ones that prove the repaired base actually turns inherited red +into green. + +## Members + +| PR | head SHA | behind dev | state at triage | reviewer | +|----|----------|-----------|-----------------|----------| +| #2799 drop default_verbosity when verbosity is unsupported | e9a7bb7bb00f2ab73b9eee1479fbb115f544ebae | 94 | MERGEABLE/CLEAN, 7 real gates pass / 0 fail | Ingwannu APPROVED (exact head) | +| #2798 classify NAT64-embedded IPv4 instead of refusing the wrapper | 856ad72d414f27556729d70ed077e04494bb7336 | 97 | MERGEABLE/CLEAN, 8 real gates pass / 0 fail | Ingwannu APPROVED, "No reportable security issue remains in this diff." | + +Both ship their own regression tests (tests/catalog-verbosity-default.test.ts, +tests/destination-policy-resolved.test.ts). + +Neither is flagged by the hygiene gate's restricted-path list — running that predicate +returns `restricted=NONE` for both. **But #2799 and #2798 are not equivalent in risk, and +an earlier draft of this doc wrongly called both non-security.** #2799 changes +`src/codex/catalog/parsing.ts`, which is genuinely not a security surface. #2798 changes +`src/lib/destination-policy.ts`, which governs whether an OAuth bearer may be sent to an +overridden destination — a security boundary under `MAINTAINERS.md`'s "other +security-boundary changes", regardless of the mechanical list. #2798 is therefore +security-gated in this lane and needs its approval re-earned on the rebased head. + +## Pre-verification already completed (2026-08-29) + +Both were rebased onto the keystone branch in isolated /tmp worktrees before any push, and +the author patches survived byte-identically: + +| PR | patch-id before | patch-id after | range-diff | +|----|-----------------|----------------|------------| +| #2799 | 358ba635ce5545fd280508f152d9b46c628db98b | 358ba635ce5545fd280508f152d9b46c628db98b | both commits `=` | +| #2798 | c1f9650e04863ecb64a981928091c9a582641ef1 | c1f9650e04863ecb64a981928091c9a582641ef1 | single commit `=` | + +Tests on the repaired base (after `bun install` in each worktree): + +``` +#2799 tests/catalog-verbosity-default.test.ts 4 pass 0 fail +#2798 tests/destination-policy-resolved.test.ts + + tests/release-version-line.test.ts 39 pass 0 fail +``` + +## Procedure per PR + +1. Create a rebase worktree under /tmp (never the session worktree): + git worktree add /tmp/ocx-lane-a- +2. Record the author patch identity BEFORE rebasing: + git diff-tree -p .. | git patch-id --stable +3. git rebase origin/dev (the repaired dev from wp8). +4. Prove the author patch survived: git range-diff .. origin/dev..HEAD + must show only base movement, and git diff --name-only origin/dev...HEAD must list the + same files as before. +5. Push the rebased head with --no-verify to the PR's own branch, then wait for exact-head + CI. Merge only when ci, all four test N/4, macos, hygiene, and gates are green. +6. Merge with a merge commit (preserving the author commits), then record the merged SHA. + +## Accept criteria + +1. Both PRs merged into dev; gh pr view --json state,mergedAt,mergeCommit confirms. +2. patch-id --stable before and after rebase is recorded for each; a changed patch id must + be explained, not silently accepted. +3. Exact-head CI green per required check, quoted from gh pr checks. +4. Both authors keep authorship in git log on dev. +5. `bash scripts/ci/assert-mergeable-review.sh ` exits 0 on the FINAL head, and the merge + uses `--match-head-commit`. A pre-rebase approval does not satisfy this: the repository + ruleset has `dismiss_stale_reviews_on_push: false`, so GitHub keeps approvals that no + longer describe the code. + +## Out of scope + +Any behavioral change to either patch. If a rebase produces a semantic conflict, the PR +leaves this lane and joins wp5. diff --git a/devlog/_plan/260829_bugpr_zero_remaining/030_wp3_lane_b_stale_rebase.md b/devlog/_plan/260829_bugpr_zero_remaining/030_wp3_lane_b_stale_rebase.md new file mode 100644 index 0000000000..f4aa4c760b --- /dev/null +++ b/devlog/_plan/260829_bugpr_zero_remaining/030_wp3_lane_b_stale_rebase.md @@ -0,0 +1,34 @@ +# wp3 — Lane B: approved but stale or inherited-red + +Dependency position: after wp8, sequenced after wp2 so the first merged rebase confirms +the repaired base before three more follow. + +## Members + +| PR | head SHA | behind dev | blocking condition | +|----|----------|-----------|--------------------| +| #2822 fold only message-shaped system items | 450b1bc60ccf58f80abcea5905085683ae9575bf | 1 | approved; test 2/4 red on the inherited version line only | +| #2821 scope model removal selectors to provider | d21ad61d51f2a4ae30447f951ced3c20ba9a7edb | 11 | approved; ci/macos/test 2/4 red on release version line only | +| #2785 raise Muse Spark context window to 1M | 107f2cbb281ae4db506471e911127cc2fc8fbb8b | 97 | approved; test 4/4 red, macOS never completed | + +All three carry an exact-head APPROVED from Ingwannu conditioned on green exact-head CI. +None touches a security surface: src/adapters/openai-responses.ts, src/cli/models.ts, +src/providers/registry.ts respectively, each with its own test file. + +## Per-PR verification obligation + +#2821 and #2822 fail ONLY the release version line assertion, which wp8 repairs. That +claim is checked, not assumed: after rebasing, the failing assertion must pass and the +previously failing shard must go green on the exact head. If a shard still fails for a +different reason, the PR moves to wp5 rather than being merged on the assumption that the +base explained everything. + +#2785 test 4/4 needs its own read: the reviewer called a duplicate final test +non-blocking, which is a claim to verify against the actual failing assertion before merge. + +## Accept criteria + +1. Each PR merged with recorded pre-merge head SHA and merged SHA. +2. For each, the specific pre-rebase failing assertion is quoted, and its post-rebase pass + is quoted from the exact-head run. +3. patch-id --stable + range-diff evidence recorded. diff --git a/devlog/_plan/260829_bugpr_zero_remaining/040_wp4_lane_c_oauth_429_rotation.md b/devlog/_plan/260829_bugpr_zero_remaining/040_wp4_lane_c_oauth_429_rotation.md new file mode 100644 index 0000000000..314fb9e3fd --- /dev/null +++ b/devlog/_plan/260829_bugpr_zero_remaining/040_wp4_lane_c_oauth_429_rotation.md @@ -0,0 +1,60 @@ +# wp4 — Lane C: #2807, the 429 OAuth rotation identity rebind + +Dependency position: after wp8. The operator requires this work to land as a matter of +principle, so it gets its own work-phase rather than sharing a lane. + +## Current state + +- PR #2807, head 1c61a7e8cd3871b374b3dd55d3cd335ae3a9e862, CONFLICTING/DIRTY, 80 behind. +- Exact-head CI was fully green (9 real gates pass / 0 fail) BEFORE the base moved. +- Files: src/server/responses/core.ts (rebind of OAuth snapshot/replay/Cursor identity + during account rotation), tests/generic-oauth-failover.test.ts. +- Ingwannu review state on that head: CHANGES_REQUESTED, with one substantive blocker. + +## The live blocker, restated precisely + +On the generic OAuth 429 rotation path the code clones account A's provider.baseUrl into +the retry. When the newly selected account B has no apiBaseUrl of its own, transport +resolution can pair B's bearer with A's accepted origin. That is a credential-boundary +defect: a token is sent to an origin bound to a different account. + +Second finding: the existing test asserts the resolver's intended expression rather than +driving the reachable path, so it would not catch a regression. What is required is an +executable A->429->B regression that observes the actual outbound pairing. + +## Change map (to be executed on current dev, not the stale branch) + +``` +MODIFY src/server/responses/core.ts + - on OAuth rotation, resolve the retry origin from the SELECTED account, never by + cloning the previous account's baseUrl + - when the selected account has no explicit apiBaseUrl, fall back to the provider + default origin rather than the previous account's origin +MODIFY tests/generic-oauth-failover.test.ts + - add an executable A -> 429 -> B case that captures the outbound request and asserts + (bearer, origin) both belong to B +``` + +Exact line-level shape is derived at wp4's P from the then-current core.ts, because the +file has moved 80 commits since the branch was cut. The stale branch diff is the starting +reference, not the patch. + +## Activation scenario (C-ACTIVATION-GROUNDING-01) + +The rotation branch only runs on a 429 from account A. The test must inject that 429, let +the rotation select B, and observe the resulting fetch. All-tests-green without a test that +drives the 429 does not satisfy this phase. + +## Security review obligation + +src/server/responses/core.ts is an authentication/credential surface, and the PR author is +the repository owner, so MAINTAINERS.md forbids self-approval. Merge requires non-author +security review; --admin does not waive it. If that review cannot be obtained, this +work-phase closes BLOCKED naming the requirement rather than merging. + +## Accept criteria + +1. The bearer/origin pairing defect is fixed on current dev. +2. An executable A->429->B test fails before the fix and passes after; both runs recorded. +3. Exact-head CI green on the new PR. +4. Non-author security review recorded, or the phase reports BLOCKED with the exact reason. diff --git a/devlog/_plan/260829_bugpr_zero_remaining/050_wp5_lane_d_reimplementation.md b/devlog/_plan/260829_bugpr_zero_remaining/050_wp5_lane_d_reimplementation.md new file mode 100644 index 0000000000..ab4db91712 --- /dev/null +++ b/devlog/_plan/260829_bugpr_zero_remaining/050_wp5_lane_d_reimplementation.md @@ -0,0 +1,118 @@ +# wp5 — Lane D: reimplementation lane + +Dependency position: after wp8, wp2, wp3. These PRs have correct intent but a branch that +cannot land as-is: a rejected assumption, an unresolved reviewer finding, a hygiene block, +or a base so old that a rebase is a rewrite. + +Each member below runs as its OWN full PABCD cycle inside this lane's sequence. One decade +doc holds the lane because the members share one procedure; the one-work-phase-one-cycle +invariant is satisfied by appending a work-phase per member at wp5's P (LOOP-UNIT-CHAIN-01) +rather than building several members in one B. + +## Members and the specific defect to carry forward + +### #2812 — IPv4-mapped IPv6 with an explicit zero group (issue #2810) + +Author widened the mapped-IPv6 regex in src/lib/destination-policy.ts. Ingwannu requested +changes on the exact head: the central equivalence assumption is wrong, because a RESERVED +IPv6 address whose tail merely resembles a public IPv4 would then be admitted as public +(::ffff:0:5db8:d822 must stay blocked). The wanted behavior is the narrow benchmark case, +not a general equivalence. Reimplement on current dev with tests that pin BOTH directions: +::ffff:0:c612:1b classified, ::ffff:0:5db8:d822 still refused. Closes #2810. + +Note the ordering dependency: #2798 (wp2) also edits src/lib/destination-policy.ts, so this +member is implemented AFTER #2798 lands and re-reads the merged file. + +### #2796 — AgentRouter openai-chat client fingerprint (issue #2717) + +Reviewer found the patch incomplete on its own terms: src/adapters/anthropic.ts imports the +shared framing helpers but never applies agentRouterDefaultHeaders, so AgentRouter Anthropic +requests can still omit the required originator. Reimplement so every supported adapter +applies the shared identity policy, with an idempotence test. Closes #2717. + +### #2797 — doctor env_key readiness (issue #2713) + +Two findings: src/cli/doctor.ts can throw when env_key is an inherited Object.prototype key +such as toString (truthy but no own .trim()), and the PR claims Closes #2713 while only +diagnosing the condition. Reimplement the diagnostic with an own-property guard and a test +for the prototype-key case; do NOT claim #2713 closed, since that issue asks for +shim-independent injection. + +### #2835 — Kiro one visible answer + +The code/test change is current and focused (src/adapters/kiro.ts + two tests), but the same +commit publishes host identity in devlog/.../030_wp1_live_measurement.md: hostname, internal +address, PID, uptime, flagged P1. Reimplement as a code-and-test-only PR with the +measurement doc redacted or omitted. The disclosure detail itself stays out of any tracked +file. + +### #2828 and #2638 — MOVED OUT of this lane to wp9 + +The A-gate audit found both had advanced since triage: #2828 is now non-draft at +`019c792607` with zero commits behind dev, and #2638 is at a rewritten `375e6f8fb8`, also +zero behind. In both cases the recorded CHANGES_REQUESTED review is bound to a SUPERSEDED +head (#2828: `1031a509a9`; #2638: `c8556f3703`), so the findings that put them in a +reimplementation lane may already be fixed on the current head. Discarding a contributor's +branch over a stale finding is both wasteful and unfair, so they get a current-head re-audit +first: see `080_wp9_current_head_reaudit.md`. Reimplementation stays available if the +current head still fails review. + +### #2744 — Recover encrypted agent tasks on the combo path (added after the audit) + +Missed by the first triage pass. Draft, CONFLICTING/DIRTY, 140 behind, CHANGES_REQUESTED, +head `1d8e35462a`. Four files: `package.json`, `src/server/responses/core.ts`, and two +agent-task-recovery tests. + +Two hard constraints on its replacement: + +1. **The `package.json` hunk must NOT be carried forward.** The stale branch bumps + `2.34.0 -> 2.36.0`, which happens to converge on wp8's value, but a version bump has no + business in an agent-task-recovery fix, and carrying it forward would re-litigate wp8's + restricted-surface decision inside an unrelated PR. The replacement touches core plus + tests only. +2. It touches `src/server/responses/core.ts`, so it runs AFTER wp4 and re-verifies against + the accumulated file. + +Its recorded blocker — recovered plaintext persistence — is a credential surface and needs +non-author security review. + +## Ordering constraint for this lane (from the audit) + +`src/server/responses/core.ts` is touched by #2807 (wp4), #2497, #2793, and #2744. wp4 lands +FIRST, then each core-touching member rebases onto the accumulated core and re-runs its own +regression rather than trusting a result obtained against an older file. #2807's test scans +`core.ts` and counts rotation sites, so a later core merge can silently invalidate that +assertion — which is exactly why wp4 goes first rather than last. + +The verifier for a stale generated surface is `bun run skill:surface:check` (read-only); +`bun run skill:surface` WRITES the file and is a generator, not a gate. + +### #2793 — Codex keyring passthrough (issue #2718) + +78 files, +2211/-238, 97 behind, hygiene-blocked as unsponsored_surface, seven unresolved +current-diff findings including a cache path that overwrites a known managed account id with +null. This is not a rebase candidate. Extract only the defect issue #2718 actually reports +(persistent "Needs re-authentication" plus truncated catalog) and implement that narrowly; +the WebSocket/quota/GUI work is out of scope for a bug campaign. + +### #2497 — native main token refresh (issue #2221) + +496 behind, CONFLICTING, 20 files across the credential core, with unresolved design +findings: non-atomic publication (a crash between two writes leaves no auth.json), account-id +equivalence treated as grant ownership, and one logical replay expanding to as many as nine +physical sends. The previous campaign already aborted a rebase here on semantic conflicts. +Reimplement the #2221 refresh defect narrowly on current dev, atomically, with non-author +security review before merge. + +## Disposition rule for this lane + +A member is only closed as superseded once its replacement PR is MERGED, never on the +promise of one. The close comment names the replacement PR and the merged SHA, and credits +the original author. + +## Accept criteria + +1. Every member reaches merged-replacement or explicitly-closed-with-reason. +2. Security-surface members carry non-author review, or are reported BLOCKED with the + MAINTAINERS.md requirement quoted. +3. No security reproduction detail for an unfixed defect is written into devlog/. diff --git a/devlog/_plan/260829_bugpr_zero_remaining/060_wp6_lane_e_prless_bug_issues.md b/devlog/_plan/260829_bugpr_zero_remaining/060_wp6_lane_e_prless_bug_issues.md new file mode 100644 index 0000000000..974c873e69 --- /dev/null +++ b/devlog/_plan/260829_bugpr_zero_remaining/060_wp6_lane_e_prless_bug_issues.md @@ -0,0 +1,44 @@ +# wp6 — Lane E: important bug issues with no PR + +Dependency position: after wp5, because several wp5 reimplementations already close the +issue that would otherwise appear here (#2810, #2717, #2718, #2221, #2830). + +## Candidate pool (16 open bug-labeled issues at triage) + +Already covered by a wp5 member, so NOT re-picked here: #2810, #2717, #2718, #2221, #2830. + +Remaining, with the selection judgment: + +| issue | title | pick | reason | +|-------|-------|------|--------| +| #2833 | Unable to compact v1 | yes | compaction failure blocks ordinary sessions; user-visible and reproducible from the report | +| #2706 | Shadow Call Intercept forces effort low on every gpt-5.6-luna request | yes | silently downgrades max turns for a whole model; concrete and testable | +| #2813 | Codex Luna Reserve disables routed models after 5-hour quota exhausted | yes | routing regression that disables working capacity | +| #2723 | Quota-limited previous-model compact blocks Sol to routed DeepSeek handoff | candidate | overlaps #2833's compact path; decide after #2833 is diagnosed | +| #2792 | Loading index.js causes ERR_CONTENT_LENGTH_MISMATCH | candidate | GUI asset serving defect; needs a repro before planning | +| #2791 | /api/log timeout loop in Chrome desktop | candidate | same subsystem as #2792; likely one root cause | +| #2804 | Windows tray icon exits after 3s | no (this campaign) | Windows-host-specific; cannot be verified from this macOS session, so a fix would ship unproven | +| #2800 | Second OpenCodex home cannot pass admitCodexWrite under Task Scheduler | no | same Windows verification limit | +| #2686 | codex context issue (Chinese, sparse) | no | needs-info in substance; not actionable without reproduction | +| #1527 | Cursor adapter large-context turns collapse | no | prior session found this needs live Cursor account probing, not a code-only fix | +| #1419 | Bundled Bun SIGTRAP after connection reset | no | labeled needs-info; upstream runtime crash without a local repro | + +Selection rule applied: pick issues whose defect can be TRIGGERED and OBSERVED from this +environment (C-ACTIVATION-GROUNDING-01). An issue whose fix cannot be shown firing is not a +candidate for a blind patch; it is left open with that stated, which is honest rather than +silently skipped. + +## Procedure per picked issue + +1. Reproduce from the report: name the exact code path, quote the failing behavior. +2. Write the failing test FIRST, observe it fail. +3. Fix narrowly; observe the test pass. +4. Open a PR targeting dev with Closes # in the description, filling every section of + .github/PULL_REQUEST_TEMPLATE.md (Summary, Verification, Checklist). +5. Merge on green exact-head CI; then close the issue manually with the merged SHA, because + GitHub only auto-closes on merges into the default branch (main) and these target dev. + +## Accept criteria + +1. Each picked issue has a merged PR whose test drives the reported condition. +2. Each unpicked issue has a recorded reason (above) rather than silence. diff --git a/devlog/_plan/260829_bugpr_zero_remaining/070_wp7_closeout.md b/devlog/_plan/260829_bugpr_zero_remaining/070_wp7_closeout.md new file mode 100644 index 0000000000..b63cb9577d --- /dev/null +++ b/devlog/_plan/260829_bugpr_zero_remaining/070_wp7_closeout.md @@ -0,0 +1,22 @@ +# wp7 — Closeout: issue closure and the zero-open-bug-PR proof + +Dependency position: last. + +## Obligations + +1. For every landed fix, close its matching issue with a cross-reference comment naming the + merged SHA and the PR. PRs here target dev, so GitHub does not auto-close: each closure is + an explicit action. +2. For every bug PR that did not land, post the disposition reason on the PR itself before + closing it, crediting the original author and naming the replacement PR when one exists. +3. Produce the campaign proof: + gh pr list --repo lidge-jun/opencodex --state open --label bug --json number + must return an empty array. This is the c-1 criterion and the campaign's terminal check. +4. Record the final disposition table in this unit, then move the unit to devlog/_fin/. + +## Honest-reporting rule + +A PR left open for a required non-author security review is reported BLOCKED with the +MAINTAINERS.md quote, not counted as done. If any bug PR remains open at closeout, the +campaign's terminal outcome is not DONE, and the D summary says so plainly with the exact +PR numbers and reasons. diff --git a/devlog/_plan/260829_bugpr_zero_remaining/080_wp9_current_head_reaudit.md b/devlog/_plan/260829_bugpr_zero_remaining/080_wp9_current_head_reaudit.md new file mode 100644 index 0000000000..74c2b57694 --- /dev/null +++ b/devlog/_plan/260829_bugpr_zero_remaining/080_wp9_current_head_reaudit.md @@ -0,0 +1,58 @@ +# wp9 — Current-head re-audit: #2638 and #2828 + +Dependency position: **after wp4**, because #2638 touches `src/server/responses/core.ts` and +wp4 (#2807) is serialized ahead of every core-touching member. #2828 does not touch core and +may be audited independently of that ordering; only its merge waits on its own review. + +## Why this lane exists + +Both PRs were originally placed in the reimplementation lane on the strength of a +`CHANGES_REQUESTED` review. The A-gate audit established that in both cases the review is +bound to a head that no longer exists: + +| PR | reviewed head | current head | ahead / behind dev | draft | +|----|---------------|--------------|--------------------|-------| +| #2638 | `c8556f3703` | `375e6f8fb8` | 6 / 0 | no | +| #2828 | `1031a509a9` (maintainer), `387d9f2b10` (bot) | `019c792607` | 5 / 0 | no (was draft) | + +A finding against a superseded commit is not evidence about the current diff. Reimplementing +over a contributor's already-repaired branch would discard their work for no reason, so the +audit happens first and the disposition follows the evidence. + +## Procedure per PR + +1. Read the CURRENT diff, not the review thread: `gh pr diff `. +2. Take each blocker from the old review and check it against the current head + line-by-line. Record for each: still present, fixed, or no longer applicable — with the + file:line that proves it. +3. Dispatch an independent reviewer on the current diff (fresh reviewer, not the one whose + verdict is being re-examined). +4. Disposition: + - all old blockers fixed and no new ones -> request exact-head maintainer review and merge + through `scripts/ci/assert-mergeable-review.sh`; + - some still present -> either the author fixes them, or the PR moves to reimplementation + in wp5 with the specific unfixed blockers named; + - new blockers -> same, with the new findings recorded. + +## Known state to re-verify, not to assume + +- #2638's `gates` failure at triage was a stale generated ocx skill surface. The verifier is + `bun run skill:surface:check` (read-only); `bun run skill:surface` regenerates and is not a + gate. Whether the current head still fails it is an open question for this lane. +- #2638 touches `src/codex/auth-context.ts`, which IS on the hygiene gate's restricted list, + and its earlier security approval was explicitly scoped to `e06ffbaa8a8e`. Non-author + security review on the exact final head is mandatory. +- #2828 touches `src/grok/inject.ts`, whose teardown path can remove config tables; the + reviewer's ownership concern must be re-checked against the current parser, since the + branch claims to have addressed it. +- #2828 was earlier assessed as NOT satisfying issue #2830 (excluded-model reference + clearing). Re-check that claim against the current head before deciding whether #2830 needs + separate work. + +## Accept criteria + +1. Each PR has a recorded current-head blocker table (present / fixed / n/a with evidence). +2. Each PR reaches merge, author-fix, or documented reimplementation — never a stale-review + verdict. +3. Security-surface merges carry non-author maintainer approval bound to the final head, + verified by the executable gate. diff --git a/scripts/ci/assert-mergeable-review.sh b/scripts/ci/assert-mergeable-review.sh new file mode 100755 index 0000000000..0e51e8ab1f --- /dev/null +++ b/scripts/ci/assert-mergeable-review.sh @@ -0,0 +1,118 @@ +#!/usr/bin/env bash +# Fail-closed pre-merge review gate for the bug-PR campaign. +# +# MAINTAINERS.md requires a maintainer approval, forbids self-approval, and requires +# explicit security review on security-boundary changes. GitHub cannot express the last +# part, and `dismiss_stale_reviews_on_push` is false on this repository, so an approval +# granted to an older head survives a force-push that invalidates it. An admin merge can +# bypass the approval requirement entirely. +# +# This script is the executable form of that policy. It prints nothing reassuring and +# exits nonzero unless a review exists that is simultaneously: +# - the reviewer's LATEST review, not merely some historical one +# - state APPROVED +# - bound to the EXACT current head SHA (commit_id == headRefOid) +# - authored by someone other than the PR author +# - authored by an account listed as a current maintainer in MAINTAINERS.md +# and additionally: +# - no maintainer's latest review is CHANGES_REQUESTED +# - GitHub's own reviewDecision is APPROVED +# +# The latest-state requirement is not theoretical. A reviewer can approve a commit and then +# post CHANGES_REQUESTED on the SAME commit after finding something on a second read. A gate +# that scans for any historical APPROVED row would report that PR as approved, which is worse +# than no gate: it launders a live objection into a green light. Likewise, one maintainer's +# approval must not mask another maintainer's outstanding blocker. +# +# Every API call fails the script. An earlier revision ended the review query with `|| true`, +# which meant a mid-pagination failure kept the pages already fetched and could pass on a +# partial view of the review history. A gate that treats a failed lookup as an empty result +# is not fail-closed. +# +# Usage: scripts/ci/assert-mergeable-review.sh [repo] +set -euo pipefail + +PR="${1:?usage: assert-mergeable-review.sh [repo]}" +REPO="${2:-lidge-jun/opencodex}" + +meta=$(gh pr view "$PR" --repo "$REPO" --json headRefOid,author,title) +head=$(printf '%s' "$meta" | jq -r '.headRefOid') +author=$(printf '%s' "$meta" | jq -r '.author.login') + +if [ -z "$head" ] || [ "$head" = "null" ]; then + echo "FAIL: could not resolve head SHA for #$PR" >&2 + exit 2 +fi + +# Maintainer roster comes from MAINTAINERS.md itself, not from a hardcoded list here, so +# the gate cannot drift from the policy document it enforces. +roster=$(gh api "repos/$REPO/contents/MAINTAINERS.md" --jq .content \ + | base64 -d \ + | sed -n '/^## Current maintainers/,/^## Former maintainers/p' \ + | grep -oE '\[@[A-Za-z0-9-]+\]' \ + | tr -d '[@]' \ + | sort -u) + +if [ -z "$roster" ]; then + echo "FAIL: could not parse the maintainer roster from MAINTAINERS.md" >&2 + exit 2 +fi + +# No `|| true`: a failed or partial review fetch must abort, not degrade to "no approvals". +reviews=$(gh api "repos/$REPO/pulls/$PR/reviews" --paginate --slurp) || { + echo "FAIL: could not read reviews for #$PR (API or pagination failure)" >&2 + exit 2 +} + +# Collapse to each reviewer's LATEST substantive review. COMMENTED rows are ignored because +# they neither approve nor block; APPROVED and CHANGES_REQUESTED are the states that decide. +latest=$(printf '%s' "$reviews" | jq -c ' + [ .[] | add? // . ] | flatten + | map(select(type == "object" and (.state // "") != "")) + | map(select(.state == "APPROVED" or .state == "CHANGES_REQUESTED" or .state == "DISMISSED")) + | sort_by(.submitted_at // "", .id) + | group_by(.user.login) + | map(last) + | map({login: .user.login, state: .state, commit: .commit_id}) +') || { + echo "FAIL: could not parse the review payload for #$PR" >&2 + exit 2 +} + +# A maintainer's live objection blocks regardless of anyone else's approval. +blockers=$(printf '%s' "$latest" | jq -r --argjson roster "$(printf '%s\n' "$roster" | jq -R . | jq -s .)" ' + .[] | select(.state == "CHANGES_REQUESTED") | select(.login as $l | $roster | index($l)) | .login +') +if [ -n "$blockers" ]; then + echo "FAIL: #$PR has an outstanding maintainer CHANGES_REQUESTED from: $(printf '%s' "$blockers" | tr '\n' ' ')" >&2 + exit 1 +fi + +decision=$(gh pr view "$PR" --repo "$REPO" --json reviewDecision --jq '.reviewDecision // ""') || { + echo "FAIL: could not read reviewDecision for #$PR" >&2 + exit 2 +} +if [ "$decision" != "APPROVED" ]; then + echo "FAIL: #$PR reviewDecision is '${decision:-none}', not APPROVED" >&2 + exit 1 +fi + +qualified=$(printf '%s' "$latest" | jq -r --arg head "$head" --arg author "$author" --argjson roster "$(printf '%s\n' "$roster" | jq -R . | jq -s .)" ' + .[] + | select(.state == "APPROVED") + | select(.commit == $head) + | select(.login != $author) + | select(.login as $l | $roster | index($l)) + | .login +' | head -1) + +if [ -z "$qualified" ]; then + echo "FAIL: #$PR has no maintainer approval bound to head $head" >&2 + echo " author: $author" >&2 + echo " approvals at head: ${approvals:-(none)}" >&2 + echo " maintainer roster: $(printf '%s' "$roster" | tr '\n' ' ')" >&2 + exit 1 +fi + +echo "OK: #$PR approved at head $head by maintainer $qualified (author $author)" +echo "Merge with: gh pr merge $PR --repo $REPO --match-head-commit $head" From c551c1767e937d08a9dab6aca0b7898dff32a0ed Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 05:30:42 +0900 Subject: [PATCH 2/2] fix(ci): harden exact-head review gate - preserve paginated review rows and normalize reviewer identity - fail closed on pending, missing metadata, and head changes - add durable adversarial coverage and scrub sensitive planning detail --- .../040_wp4_lane_c_oauth_429_rotation.md | 43 +--- .../050_wp5_lane_d_reimplementation.md | 25 +- .../080_wp9_current_head_reaudit.md | 11 +- scripts/ci/assert-mergeable-review.sh | 112 +++++++-- tests/assert-mergeable-review.test.ts | 220 ++++++++++++++++++ 5 files changed, 339 insertions(+), 72 deletions(-) create mode 100644 tests/assert-mergeable-review.test.ts diff --git a/devlog/_plan/260829_bugpr_zero_remaining/040_wp4_lane_c_oauth_429_rotation.md b/devlog/_plan/260829_bugpr_zero_remaining/040_wp4_lane_c_oauth_429_rotation.md index 314fb9e3fd..0b1d4800bc 100644 --- a/devlog/_plan/260829_bugpr_zero_remaining/040_wp4_lane_c_oauth_429_rotation.md +++ b/devlog/_plan/260829_bugpr_zero_remaining/040_wp4_lane_c_oauth_429_rotation.md @@ -11,39 +11,11 @@ principle, so it gets its own work-phase rather than sharing a lane. during account rotation), tests/generic-oauth-failover.test.ts. - Ingwannu review state on that head: CHANGES_REQUESTED, with one substantive blocker. -## The live blocker, restated precisely +## Working-detail boundary -On the generic OAuth 429 rotation path the code clones account A's provider.baseUrl into -the retry. When the newly selected account B has no apiBaseUrl of its own, transport -resolution can pair B's bearer with A's accepted origin. That is a credential-boundary -defect: a token is sent to an origin bound to a different account. - -Second finding: the existing test asserts the resolver's intended expression rather than -driving the reachable path, so it would not catch a regression. What is required is an -executable A->429->B regression that observes the actual outbound pairing. - -## Change map (to be executed on current dev, not the stale branch) - -``` -MODIFY src/server/responses/core.ts - - on OAuth rotation, resolve the retry origin from the SELECTED account, never by - cloning the previous account's baseUrl - - when the selected account has no explicit apiBaseUrl, fall back to the provider - default origin rather than the previous account's origin -MODIFY tests/generic-oauth-failover.test.ts - - add an executable A -> 429 -> B case that captures the outbound request and asserts - (bearer, origin) both belong to B -``` - -Exact line-level shape is derived at wp4's P from the then-current core.ts, because the -file has moved 80 commits since the branch was cut. The stale branch diff is the starting -reference, not the patch. - -## Activation scenario (C-ACTIVATION-GROUNDING-01) - -The rotation branch only runs on a 429 from account A. The test must inject that 429, let -the rotation select B, and observe the resulting fetch. All-tests-green without a test that -drives the 429 does not satisfy this phase. +The unresolved security finding, reproduction, and remediation notes stay in gitignored +scratch space until a public fix ships. This tracked lane records only PR state, ordering, +and merge governance. Implementation begins from current dev rather than the stale branch. ## Security review obligation @@ -54,7 +26,6 @@ work-phase closes BLOCKED naming the requirement rather than merging. ## Accept criteria -1. The bearer/origin pairing defect is fixed on current dev. -2. An executable A->429->B test fails before the fix and passes after; both runs recorded. -3. Exact-head CI green on the new PR. -4. Non-author security review recorded, or the phase reports BLOCKED with the exact reason. +1. The current-dev replacement reaches a terminal disposition. +2. Focused verification and exact-head CI are green on the replacement PR. +3. Non-author security review is recorded, or the phase reports BLOCKED with the exact reason. diff --git a/devlog/_plan/260829_bugpr_zero_remaining/050_wp5_lane_d_reimplementation.md b/devlog/_plan/260829_bugpr_zero_remaining/050_wp5_lane_d_reimplementation.md index ab4db91712..fc0a5167cf 100644 --- a/devlog/_plan/260829_bugpr_zero_remaining/050_wp5_lane_d_reimplementation.md +++ b/devlog/_plan/260829_bugpr_zero_remaining/050_wp5_lane_d_reimplementation.md @@ -73,8 +73,8 @@ Two hard constraints on its replacement: 2. It touches `src/server/responses/core.ts`, so it runs AFTER wp4 and re-verifies against the accumulated file. -Its recorded blocker — recovered plaintext persistence — is a credential surface and needs -non-author security review. +Its unresolved security finding and remediation notes remain in gitignored scratch until a +public fix ships. The replacement still requires non-author security review. ## Ordering constraint for this lane (from the audit) @@ -89,20 +89,19 @@ The verifier for a stale generated surface is `bun run skill:surface:check` (rea ### #2793 — Codex keyring passthrough (issue #2718) -78 files, +2211/-238, 97 behind, hygiene-blocked as unsponsored_surface, seven unresolved -current-diff findings including a cache path that overwrites a known managed account id with -null. This is not a rebase candidate. Extract only the defect issue #2718 actually reports -(persistent "Needs re-authentication" plus truncated catalog) and implement that narrowly; -the WebSocket/quota/GUI work is out of scope for a bug campaign. +78 files, +2211/-238, 97 behind, hygiene-blocked as unsponsored_surface, with seven unresolved +current-diff findings. This is not a rebase candidate. Extract only the defect issue #2718 +actually reports and implement that narrowly; the WebSocket/quota/GUI work is out of scope +for a bug campaign. Security-sensitive review detail remains in gitignored scratch until the +fix ships. ### #2497 — native main token refresh (issue #2221) -496 behind, CONFLICTING, 20 files across the credential core, with unresolved design -findings: non-atomic publication (a crash between two writes leaves no auth.json), account-id -equivalence treated as grant ownership, and one logical replay expanding to as many as nine -physical sends. The previous campaign already aborted a rebase here on semantic conflicts. -Reimplement the #2221 refresh defect narrowly on current dev, atomically, with non-author -security review before merge. +496 behind, CONFLICTING, and 20 files across the credential core. The previous campaign +already aborted a rebase here on semantic conflicts, so this is a narrow current-dev +reimplementation rather than a rebase. Unresolved security findings and remediation notes +remain in gitignored scratch until a public fix ships. Non-author security review is required +before merge. ## Disposition rule for this lane diff --git a/devlog/_plan/260829_bugpr_zero_remaining/080_wp9_current_head_reaudit.md b/devlog/_plan/260829_bugpr_zero_remaining/080_wp9_current_head_reaudit.md index 74c2b57694..f658cbfd04 100644 --- a/devlog/_plan/260829_bugpr_zero_remaining/080_wp9_current_head_reaudit.md +++ b/devlog/_plan/260829_bugpr_zero_remaining/080_wp9_current_head_reaudit.md @@ -31,8 +31,9 @@ audit happens first and the disposition follows the evidence. - all old blockers fixed and no new ones -> request exact-head maintainer review and merge through `scripts/ci/assert-mergeable-review.sh`; - some still present -> either the author fixes them, or the PR moves to reimplementation - in wp5 with the specific unfixed blockers named; - - new blockers -> same, with the new findings recorded. + in wp5 with a non-sensitive status recorded; + - new blockers -> same disposition. Unresolved security detail stays in gitignored scratch + until a public fix ships. ## Known state to re-verify, not to assume @@ -42,9 +43,9 @@ audit happens first and the disposition follows the evidence. - #2638 touches `src/codex/auth-context.ts`, which IS on the hygiene gate's restricted list, and its earlier security approval was explicitly scoped to `e06ffbaa8a8e`. Non-author security review on the exact final head is mandatory. -- #2828 touches `src/grok/inject.ts`, whose teardown path can remove config tables; the - reviewer's ownership concern must be re-checked against the current parser, since the - branch claims to have addressed it. +- #2828 touches `src/grok/inject.ts` and remains subject to current-head maintainer review. + Any unresolved security finding and remediation detail stays in gitignored scratch until + a public fix ships. - #2828 was earlier assessed as NOT satisfying issue #2830 (excluded-model reference clearing). Re-check that claim against the current head before deciding whether #2830 needs separate work. diff --git a/scripts/ci/assert-mergeable-review.sh b/scripts/ci/assert-mergeable-review.sh index 0e51e8ab1f..9f2b337e35 100755 --- a/scripts/ci/assert-mergeable-review.sh +++ b/scripts/ci/assert-mergeable-review.sh @@ -35,14 +35,21 @@ set -euo pipefail PR="${1:?usage: assert-mergeable-review.sh [repo]}" REPO="${2:-lidge-jun/opencodex}" -meta=$(gh pr view "$PR" --repo "$REPO" --json headRefOid,author,title) -head=$(printf '%s' "$meta" | jq -r '.headRefOid') -author=$(printf '%s' "$meta" | jq -r '.author.login') - -if [ -z "$head" ] || [ "$head" = "null" ]; then - echo "FAIL: could not resolve head SHA for #$PR" >&2 +meta=$(gh pr view "$PR" --repo "$REPO" --json headRefOid,author,title) || { + echo "FAIL: could not read required metadata for #$PR" >&2 exit 2 -fi +} +identity=$(printf '%s' "$meta" | jq -er ' + if (.headRefOid | type) != "string" or (.headRefOid | length) == 0 + or (.author.login | type) != "string" or (.author.login | length) == 0 + then error("missing headRefOid or author.login") + else [ .headRefOid, (.author.login | ascii_downcase) ] | @tsv + end +') || { + echo "FAIL: #$PR metadata is missing headRefOid or author.login" >&2 + exit 2 +} +IFS=$'\t' read -r head author <<< "$identity" # Maintainer roster comes from MAINTAINERS.md itself, not from a hardcoded list here, so # the gate cannot drift from the policy document it enforces. @@ -50,7 +57,8 @@ roster=$(gh api "repos/$REPO/contents/MAINTAINERS.md" --jq .content \ | base64 -d \ | sed -n '/^## Current maintainers/,/^## Former maintainers/p' \ | grep -oE '\[@[A-Za-z0-9-]+\]' \ - | tr -d '[@]' \ + | tr -d '@[]' \ + | jq -Rr 'ascii_downcase' \ | sort -u) if [ -z "$roster" ]; then @@ -64,16 +72,61 @@ reviews=$(gh api "repos/$REPO/pulls/$PR/reviews" --paginate --slurp) || { exit 2 } -# Collapse to each reviewer's LATEST substantive review. COMMENTED rows are ignored because -# they neither approve nor block; APPROVED and CHANGES_REQUESTED are the states that decide. +# Validate gh's slurped array-of-page-arrays before flattening every review row. Review +# identity is case-insensitive. COMMENTED is neutral; DISMISSED invalidates an earlier +# approval; PENDING is not an approval and does not hide a prior submitted blocker. latest=$(printf '%s' "$reviews" | jq -c ' - [ .[] | add? // . ] | flatten - | map(select(type == "object" and (.state // "") != "")) - | map(select(.state == "APPROVED" or .state == "CHANGES_REQUESTED" or .state == "DISMISSED")) - | sort_by(.submitted_at // "", .id) - | group_by(.user.login) - | map(last) - | map({login: .user.login, state: .state, commit: .commit_id}) + def allowed_states: ["APPROVED", "CHANGES_REQUESTED", "COMMENTED", "DISMISSED", "PENDING"]; + if type != "array" then + error("review payload is not a slurped page array") + elif any(.[]; type != "array") then + error("review payload contains a non-array page") + else + [ .[][] ] + | if any(.[]; type != "object") then + error("review payload contains a non-object row") + else + to_entries + | map( + .key as $order + | .value as $review + | if ($review.user.login | type) != "string" or ($review.user.login | length) == 0 then + error("review row is missing user.login") + elif ($review.state | type) != "string" then + error("review row is missing state") + else + ($review.state | ascii_upcase) as $state + | if (allowed_states | index($state)) == null then + error("review row has an unknown state") + else + { + login: ($review.user.login | ascii_downcase), + state: $state, + commit: $review.commit_id, + order: $order + } + end + end + ) + | group_by(.login) + | map( + sort_by(.order) as $rows + | ($rows | map(select(.state != "COMMENTED")) | last) as $latest + | ($rows | map(select( + .state == "APPROVED" + or .state == "CHANGES_REQUESTED" + or .state == "DISMISSED" + )) | last) as $submitted + | select($latest != null) + | { + login: $rows[0].login, + state: $latest.state, + commit: $latest.commit, + submitted_state: ($submitted.state // null) + } + ) + end + end ') || { echo "FAIL: could not parse the review payload for #$PR" >&2 exit 2 @@ -81,7 +134,13 @@ latest=$(printf '%s' "$reviews" | jq -c ' # A maintainer's live objection blocks regardless of anyone else's approval. blockers=$(printf '%s' "$latest" | jq -r --argjson roster "$(printf '%s\n' "$roster" | jq -R . | jq -s .)" ' - .[] | select(.state == "CHANGES_REQUESTED") | select(.login as $l | $roster | index($l)) | .login + .[] + | select( + .state == "CHANGES_REQUESTED" + or (.state == "PENDING" and .submitted_state == "CHANGES_REQUESTED") + ) + | select(.login as $l | $roster | index($l)) + | .login ') if [ -n "$blockers" ]; then echo "FAIL: #$PR has an outstanding maintainer CHANGES_REQUESTED from: $(printf '%s' "$blockers" | tr '\n' ' ')" >&2 @@ -114,5 +173,22 @@ if [ -z "$qualified" ]; then exit 1 fi +# The review work above may race a contributor push. Re-read the head immediately before +# success so this verdict and the printed --match-head-commit instruction name one SHA. +final_meta=$(gh pr view "$PR" --repo "$REPO" --json headRefOid) || { + echo "FAIL: could not re-read head SHA for #$PR" >&2 + exit 2 +} +final_head=$(printf '%s' "$final_meta" | jq -er ' + .headRefOid | select(type == "string" and length > 0) +') || { + echo "FAIL: could not resolve final head SHA for #$PR" >&2 + exit 2 +} +if [ "$final_head" != "$head" ]; then + echo "FAIL: #$PR head changed during review validation ($head -> $final_head)" >&2 + exit 1 +fi + echo "OK: #$PR approved at head $head by maintainer $qualified (author $author)" echo "Merge with: gh pr merge $PR --repo $REPO --match-head-commit $head" diff --git a/tests/assert-mergeable-review.test.ts b/tests/assert-mergeable-review.test.ts new file mode 100644 index 0000000000..0f33972d47 --- /dev/null +++ b/tests/assert-mergeable-review.test.ts @@ -0,0 +1,220 @@ +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { delimiter, join } from "node:path"; +import { fileURLToPath } from "node:url"; + +const repoRoot = fileURLToPath(new URL("../", import.meta.url)); +const gate = join(repoRoot, "scripts", "ci", "assert-mergeable-review.sh"); +const fixtureRoot = mkdtempSync(join(tmpdir(), "ocx-mergeable-review-")); +const mockBin = join(fixtureRoot, "bin"); + +const fakeGh = `#!/usr/bin/env bash +set -euo pipefail + +case_name="$CASE_NAME" + +review() { + local login="$1" + local state="$2" + local commit="$3" + local submitted="$4" + local id="$5" + local type="User" + if [ "$#" -ge 6 ]; then + type="$6" + fi + printf '{"user":{"login":"%s","type":"%s"},"state":"%s","commit_id":"%s","submitted_at":"%s","id":%s}' \ + "$login" "$type" "$state" "$commit" "$submitted" "$id" +} + +if [ "$1" = "pr" ] && [ "$2" = "view" ]; then + if printf '%s\n' "$*" | grep -q 'reviewDecision'; then + case "$case_name" in + decision_api_failure) exit 71 ;; + decision_not_approved) printf '%s\n' 'REVIEW_REQUIRED'; exit 0 ;; + decision_missing) printf '%s\n' ''; exit 0 ;; + *) printf '%s\n' 'APPROVED'; exit 0 ;; + esac + fi + + case "$case_name" in + meta_api_failure) exit 72 ;; + missing_head) printf '%s\n' '{"headRefOid":null,"author":{"login":"author"},"title":"fixture"}' ;; + missing_author) printf '%s\n' '{"headRefOid":"HEADSHA","author":null,"title":"fixture"}' ;; + self_approval|case_variant_self_approval) printf '%s\n' '{"headRefOid":"HEADSHA","author":{"login":"lidge-jun"},"title":"fixture"}' ;; + head_changes_after_meta) + if [ -f "$CASE_STATE_DIR/meta-read" ]; then + printf '%s\n' '{"headRefOid":"NEWSHA","author":{"login":"author"},"title":"fixture"}' + else + : > "$CASE_STATE_DIR/meta-read" + printf '%s\n' '{"headRefOid":"OLDSHA","author":{"login":"author"},"title":"fixture"}' + fi + ;; + *) printf '%s\n' '{"headRefOid":"HEADSHA","author":{"login":"author"},"title":"fixture"}' ;; + esac + exit 0 +fi + +if [ "$1" = "api" ]; then + if printf '%s\n' "$2" | grep -q '/contents/MAINTAINERS.md'; then + if [ "$case_name" = "roster_api_failure" ]; then + exit 73 + fi + if [ "$case_name" = "empty_roster" ]; then + printf '%b' '# Maintainers\n\n## Current maintainers\n\n## Former maintainers\n' | base64 + exit 0 + fi + if [ "$case_name" = "bot_rostered" ]; then + printf '%b' '# Maintainers\n\n## Current maintainers\n\n| [@lidge-jun](x) | owner |\n| [@Ingwannu](x) | maintainer |\n| [@coderabbitai](x) | automation |\n\n## Former maintainers\n' | base64 + exit 0 + fi + if [ "$case_name" = "case_variant_self_approval" ]; then + printf '%b' '# Maintainers\n\n## Current maintainers\n\n| [@LIDGE-JUN](x) | owner |\n| [@Ingwannu](x) | maintainer |\n\n## Former maintainers\n' | base64 + exit 0 + fi + printf '%b' '# Maintainers\n\n## Current maintainers\n\n| [@lidge-jun](x) | owner |\n| [@Ingwannu](x) | maintainer |\n\n## Former maintainers\n' | base64 + exit 0 + fi + + if printf '%s\n' "$2" | grep -q '/reviews'; then + case "$case_name" in + review_api_failure) + printf '%s\n' '[[{"user":{"login":"Ingwannu"},"state":"APPROVED","commit_id":"HEADSHA","submitted_at":"2026-08-29T00:00:00Z","id":1}]]' + exit 74 + ;; + malformed_reviews) + printf '%s\n' 'not-json' + ;; + zero_reviews|decision_not_approved|decision_missing|meta_api_failure|missing_head|roster_api_failure|empty_roster|decision_api_failure) + printf '%s\n' '[[]]' + ;; + happy_exact) + printf '[['; review Ingwannu APPROVED HEADSHA 2026-08-29T00:00:00Z 1; printf ']]\n' + ;; + approve_then_changes) + printf '[['; review Ingwannu APPROVED HEADSHA 2026-08-29T00:00:00Z 1; printf ','; review Ingwannu CHANGES_REQUESTED HEADSHA 2026-08-29T00:01:00Z 2; printf ']]\n' + ;; + stale_approval) + printf '[['; review Ingwannu APPROVED OLDSHA 2026-08-29T00:00:00Z 1; printf ']]\n' + ;; + self_approval) + printf '[['; review lidge-jun APPROVED HEADSHA 2026-08-29T00:00:00Z 1; printf ']]\n' + ;; + case_variant_self_approval) + printf '[['; review LIDGE-JUN APPROVED HEADSHA 2026-08-29T00:00:00Z 1; printf ']]\n' + ;; + outside_approval) + printf '[['; review outsider APPROVED HEADSHA 2026-08-29T00:00:00Z 1; printf ']]\n' + ;; + maintainer_blocker_same_page) + printf '[['; review lidge-jun CHANGES_REQUESTED HEADSHA 2026-08-29T00:00:00Z 1; printf ','; review Ingwannu APPROVED HEADSHA 2026-08-29T00:01:00Z 2; printf ']]\n' + ;; + maintainer_blocker_multi_page) + printf '[['; review lidge-jun CHANGES_REQUESTED HEADSHA 2026-08-29T00:00:00Z 1; printf ','; review outsider COMMENTED HEADSHA 2026-08-29T00:00:30Z 2; printf '],['; review Ingwannu APPROVED HEADSHA 2026-08-29T00:01:00Z 3; printf ']]\n' + ;; + dismissed_after_approval) + printf '[['; review Ingwannu APPROVED HEADSHA 2026-08-29T00:00:00Z 1; printf '],['; review Ingwannu DISMISSED HEADSHA 2026-08-29T00:01:00Z 2; printf ']]\n' + ;; + pending_after_approval) + printf '[['; review Ingwannu APPROVED HEADSHA 2026-08-29T00:00:00Z 1; printf '],['; review Ingwannu PENDING HEADSHA 2026-08-29T00:01:00Z 2; printf ']]\n' + ;; + comment_after_approval) + printf '[['; review Ingwannu APPROVED HEADSHA 2026-08-29T00:00:00Z 1; printf '],['; review Ingwannu COMMENTED HEADSHA 2026-08-29T00:01:00Z 2; printf ']]\n' + ;; + case_variant_latest_blocker) + printf '[['; review Ingwannu APPROVED HEADSHA 2026-08-29T00:00:00Z 1; printf '],['; review ingwannu CHANGES_REQUESTED HEADSHA 2026-08-29T00:01:00Z 2; printf ']]\n' + ;; + bot_outside_roster|bot_rostered) + printf '[['; review 'coderabbitai[bot]' APPROVED HEADSHA 2026-08-29T00:00:00Z 1 Bot; printf ']]\n' + ;; + head_changes_after_meta) + printf '[['; review Ingwannu APPROVED OLDSHA 2026-08-29T00:00:00Z 1; printf ']]\n' + ;; + missing_author) + printf '[['; review Ingwannu APPROVED HEADSHA 2026-08-29T00:00:00Z 1; printf ']]\n' + ;; + missing_commit) + printf '%s\n' '[[{"user":{"login":"Ingwannu","type":"User"},"state":"APPROVED","submitted_at":"2026-08-29T00:00:00Z","id":1}]]' + ;; + decision_not_approved|decision_missing) + printf '[['; review Ingwannu APPROVED HEADSHA 2026-08-29T00:00:00Z 1; printf ']]\n' + ;; + *) + printf '%s\n' '[[]]' + ;; + esac + exit 0 + fi +fi + +printf 'unexpected gh invocation: %s\n' "$*" >&2 +exit 75 +`; + +const cases = [ + ["happy_exact", "PASS"], + ["approve_then_changes", "FAIL"], + ["stale_approval", "FAIL"], + ["self_approval", "FAIL"], + ["outside_approval", "FAIL"], + ["maintainer_blocker_same_page", "FAIL"], + ["maintainer_blocker_multi_page", "FAIL"], + ["decision_not_approved", "FAIL"], + ["review_api_failure", "FAIL"], + ["roster_api_failure", "FAIL"], + ["meta_api_failure", "FAIL"], + ["decision_api_failure", "FAIL"], + ["malformed_reviews", "FAIL"], + ["missing_head", "FAIL"], + ["missing_author", "FAIL"], + ["missing_commit", "FAIL"], + ["empty_roster", "FAIL"], + ["zero_reviews", "FAIL"], + ["dismissed_after_approval", "FAIL"], + ["pending_after_approval", "FAIL"], + ["comment_after_approval", "PASS"], + ["case_variant_latest_blocker", "FAIL"], + ["case_variant_self_approval", "FAIL"], + ["bot_outside_roster", "FAIL"], + ["bot_rostered", "FAIL"], + ["head_changes_after_meta", "FAIL"], +] as const; + +beforeAll(() => { + mkdirSync(mockBin); + const fakeGhPath = join(mockBin, "gh"); + writeFileSync(fakeGhPath, fakeGh, "utf8"); + chmodSync(fakeGhPath, 0o755); +}); + +afterAll(() => { + rmSync(fixtureRoot, { recursive: true, force: true }); +}); + +describe.skipIf(process.platform === "win32")("assert-mergeable-review", () => { + for (const [name, expected] of cases) { + test(name, () => { + const caseStateDir = join(fixtureRoot, name); + mkdirSync(caseStateDir); + const result = Bun.spawnSync(["bash", gate, "999", "fixture/repo"], { + cwd: repoRoot, + env: { + ...process.env, + CASE_NAME: name, + CASE_STATE_DIR: caseStateDir, + PATH: `${mockBin}${delimiter}${process.env.PATH ?? ""}`, + }, + stdout: "pipe", + stderr: "pipe", + }); + const output = `${new TextDecoder().decode(result.stdout)}${new TextDecoder().decode(result.stderr)}`; + + if (expected === "PASS") { + expect(result.exitCode, output).toBe(0); + } else { + expect(result.exitCode, output).not.toBe(0); + } + }); + } +});