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..0b1d4800bc --- /dev/null +++ b/devlog/_plan/260829_bugpr_zero_remaining/040_wp4_lane_c_oauth_429_rotation.md @@ -0,0 +1,31 @@ +# 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. + +## Working-detail boundary + +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 + +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 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 new file mode 100644 index 0000000000..fc0a5167cf --- /dev/null +++ b/devlog/_plan/260829_bugpr_zero_remaining/050_wp5_lane_d_reimplementation.md @@ -0,0 +1,117 @@ +# 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 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) + +`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, 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, 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 + +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..f658cbfd04 --- /dev/null +++ b/devlog/_plan/260829_bugpr_zero_remaining/080_wp9_current_head_reaudit.md @@ -0,0 +1,59 @@ +# 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 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 + +- #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` 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. + +## 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..9f2b337e35 --- /dev/null +++ b/scripts/ci/assert-mergeable-review.sh @@ -0,0 +1,194 @@ +#!/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) || { + echo "FAIL: could not read required metadata for #$PR" >&2 + exit 2 +} +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. +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 '@[]' \ + | jq -Rr 'ascii_downcase' \ + | 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 +} + +# 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 ' + 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 +} + +# 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" + 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 + 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 + +# 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); + } + }); + } +});