-
Notifications
You must be signed in to change notification settings - Fork 924
docs(devlog): record the igwanu bug-PR merge round #2770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
94c7406
b8a2957
6d2196c
f61f979
20c71b8
63dc0a5
190412a
e75dae3
09acc19
aae84fc
9a87a39
683c851
44fbea3
8356213
6f949da
1ce74c4
f0505a3
6db157b
8a1deef
1878ab7
7cbf8b2
d229993
8ae1338
aff0580
730bd9e
7ae8213
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| # 000 — igwanu bug-PR merge round: plan | ||
|
|
||
| Round base: `dev @ 8b1b65b8d` (local == `origin/dev`, verified 2026-08-27). | ||
| Scope: the 13 open **bug**-labelled PRs. Four are Ingwannu's (#2767, #2766, #2764, | ||
| #2761); nine are other authors' (#2747, #2745, #2740, #2733, #2729, #2726, #2693, | ||
| #2638, #2497). | ||
|
|
||
| Enhancement-labelled PRs are explicitly out of scope for this round. | ||
|
|
||
| ## The finding that orders the whole round | ||
|
|
||
| Three PRs (#2767, #2764, #2747) show **failing required CI** — `ci`, `macos`, | ||
| `test 3/4`, `gates` — while their own merged trees compile clean. The failure is | ||
| not theirs. Every one of them fails the same repository-wide assertion: | ||
|
|
||
| ``` | ||
| error: package.json version 2.34.0 equals release tag v2.34.0, but this commit is | ||
| not the one that tag names. The tree claims an already-published version. | ||
| (fail) release version line > the in-tree version is never behind a released one | ||
| ``` | ||
|
|
||
| `dev` still carries `2.34.0` after tag `v2.34.0` shipped, so *every* PR opened | ||
| after the release train inherits a red matrix. A second shared failure hits | ||
| `gates`: `privacy:scan` reads the scp-style SSH remote principal recorded in | ||
| `devlog/_plan/260827_release_train/020_preview_release.md` as an email address. | ||
|
|
||
| **#2766 repairs both.** It is the keystone: until it lands, no other PR in this | ||
| round can produce a trustworthy green matrix, and re-running their CI is wasted | ||
| work. This is the inverse of the previous round's lesson — there, green checks | ||
| were not evidence of health; here, red checks are not evidence of harm. | ||
|
|
||
| Evidence: run `33081644562` job `98550259965` (#2767), run `33080634739` job | ||
| `98546624127` (#2764), run `33059606933` job `98534630924` (#2747) — each shows | ||
| `1 fail` and that one failure is `release version line`. | ||
|
|
||
| ## Merged-tree gate (this round's own evidence, not GitHub's) | ||
|
|
||
| Every PR head was fetched, merged against `dev @ 8b1b65b8d` with | ||
| `git merge-tree --write-tree`, committed as `mtp/<n>`, checked out to an isolated | ||
| worktree sharing this repo's `node_modules`, and compiled. | ||
|
|
||
| | PR | ahead | behind dev | merge-tree | tsc on MERGED tree | | ||
| |---|---|---|---|---| | ||
| | #2767 | 1 | 0 | CLEAN | OK | | ||
| | #2766 | 2 | 0 | CLEAN | OK | | ||
| | #2764 | 1 | 0 | CLEAN | OK | | ||
| | #2761 | 1 | 2 | CLEAN | OK | | ||
| | #2747 | 1 | 26 | CLEAN | OK | | ||
| | #2745 | 2 | 26 | CLEAN | OK | | ||
| | #2740 | 1 | 26 | CLEAN | OK | | ||
| | #2733 | 1 | 43 | CLEAN | OK | | ||
| | #2729 | 2 | 89 | CLEAN | OK | | ||
| | #2726 | 1 | 63 | CLEAN | OK | | ||
| | #2693 | 2 | 118 | CLEAN | OK | | ||
| | #2638 | 2 | 179 | CLEAN | OK | | ||
| | #2497 | 1 | **386** | **CONFLICT** | not reachable | | ||
|
|
||
| The typecheck gate was itself verified rather than trusted: 12 runs finishing in | ||
| ~12s looked like a no-op, so a deliberate `const x: number = 'str'` was injected | ||
| into a merged worktree and `tsc` returned `error TS2322`, exit 1. The speed is | ||
| real — this repository is on the native TypeScript 7.0.2 compiler (~0.44s full | ||
| typecheck). The gate works. | ||
|
|
||
| ## Cross-PR file contention | ||
|
|
||
| `src/server/responses/core.ts` — **#2745, #2638, #2497**. Pairwise | ||
| `git merge-tree` required before any second one of those lands; textual | ||
| mergeability is not behavioral compatibility on the auth/routing boundary. | ||
|
|
||
| `src/adapters/openai-chat.ts` — #2764 only. `src/adapters/openai-responses.ts` | ||
| — #2767 only. `src/codex/auth-context.ts` — #2638 and #2497. | ||
| No other file is touched by two in-scope PRs. | ||
|
|
||
| ## Loop-spec | ||
|
|
||
| - Loop archetype: verifier-defined (spec-satisfaction repair per PR). | ||
| - Write scope: `devlog/_plan/260827_igwanu_bug_pr_merge_round/`, `src/` and | ||
| `tests/` only where needed to land or reimplement a PR, plus PR metadata on | ||
| GitHub and `codex/` topic branches. | ||
| - Out of scope: `main`, `preview`, releases, tags, npm publish, docs deploy, | ||
| enhancement PRs, force-push, history rewrite. | ||
| - Bounds: `dev` is push-protected — every lane travels a `codex/` branch and a PR | ||
| targeting `dev`. `bun test` takes a machine-wide lock: one suite at a time, | ||
| long suites on `ssh lidge` via `ocx-run`. Never `OCX_TEST_NO_QUEUE=1`. | ||
|
|
||
| ## Work-phase map (one phase = one full PABCD cycle) | ||
|
|
||
| | WP | Doc | Slice | Depends on | | ||
| |----|-----|-------|------------| | ||
| | wp1 | 000 | Docs-only roadmap: intake, merged-tree gate, contention map, lanes | — | | ||
| | wp2 | 010 | **Keystone** #2766 — unblock the repository-wide CI gates | wp1 | | ||
| | wp3 | 020 | Ingwannu remainder #2761, #2764, #2767 | wp2 | | ||
| | wp4 | 030 | Clean approved lane #2733, #2726, #2747 | wp2 | | ||
| | wp5 | 040 | Maintainer changes-requested #2745, #2729 | wp2 | | ||
| | wp6 | 050 | Contributor remainder #2740, #2693, #2638 | wp2, wp5 | | ||
| | wp7 | 060 | #2497 adjudication + round close-out | all | | ||
|
|
||
| ## Standing gates (inherited, all mandatory) | ||
|
|
||
| 1. Compile evidence comes from the MERGED tree, never the PR head alone. | ||
| 2. Any two PRs touching a shared file get `git merge-tree` before either merges. | ||
| 3. Green checks are not health unless the list includes `ci` / `test N/4` / | ||
| `macos`. **Corollary discovered this round: red checks are not harm until the | ||
| shared baseline is green.** | ||
| 4. One `bun test` suite at a time; remove a stale | ||
| `/tmp/opencodex-bun-test.lock` rather than bypassing the queue. | ||
| 5. Every lane travels a `codex/` branch and a PR targeting `dev`. | ||
| 6. A safety net that exists in code is not a safety net that functions. | ||
|
|
||
| ## Accept criteria (mirrored into goalplan criteria[]) | ||
|
|
||
| - c1 — all 13 PRs carry a recorded terminal disposition with SHA or reason. | ||
| - c2 — merged-tree compile gate ran for every candidate (this doc's table). | ||
| - c3 — each landed change carries a focused test receipt from the merged tree. | ||
| - c4 — `dev` advanced only through PRs targeting `dev`. | ||
| - c5 — auth/credential/OAuth surfaces are not landed autonomously. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| # 010 — wp2: the keystone, #2766 | ||
|
|
||
| **Lane L1 (commit-then-merge). Every other MERGE in this round serializes behind | ||
| this one.** | ||
|
|
||
| Precisely scoped: review, focused verification, rebases, and approval requests for | ||
| other PRs run in parallel — their changed paths are disjoint from this one's | ||
| (`package.json` plus a release-runbook document). What must wait is the act of | ||
| merging, because landing anything else first leaves `dev` sitting on known-red | ||
| release-version and privacy gates. | ||
|
|
||
| PR #2766 `ingw/fix-release-doc-privacy-scan-2762` — head `076ad3036`, ready | ||
| (not draft), MERGEABLE, 0 commits behind `dev`, 30 checks with **zero failures**. | ||
| It is the only PR in the round whose matrix is already green, because it is the | ||
| one that repairs the matrix. | ||
|
|
||
| ## Why it is the keystone | ||
|
|
||
| Two repository-wide gates went red after the v2.34.0 release train, and they fail | ||
| on `dev` itself, not on any contributor's code: | ||
|
|
||
| 1. `tests/release-version-line.test.ts` — `package.json` is `2.34.0` and tag | ||
| `v2.34.0` is published, so every commit after the tag "claims an | ||
| already-published version". Fails `ci`, `macos`, `test 3/4`. | ||
| 2. `privacy:scan` — `devlog/_plan/260827_release_train/020_preview_release.md:36` | ||
| contains a literal scp-style SSH remote whose `user@host` principal the scanner | ||
| reads as an email address. Fails `gates`. | ||
|
|
||
| Confirmed inherited by #2767, #2764, #2747. Merging anything else first means | ||
| reading a red matrix that says nothing about the PR under review. | ||
|
|
||
| ## MODIFY map (exact, already authored by the PR) | ||
|
|
||
| MODIFY `package.json`: | ||
|
|
||
| ```diff | ||
| - "version": "2.34.0", | ||
| + "version": "2.35.0", | ||
| ``` | ||
|
|
||
| MODIFY `devlog/_plan/260827_release_train/020_preview_release.md`: | ||
|
|
||
| The runbook's push line is rewritten to build the destination from two shell | ||
| variables (`release_host=github.com`, `release_repo=lidge-jun/opencodex.git`) and | ||
| interpolate them, so the scp-style principal never appears as one literal token. | ||
| The exact diff is on the PR; it is not reproduced here, because quoting it | ||
| verbatim would reintroduce the very literal the scan rejects — this document is | ||
| itself scanned. | ||
|
|
||
| The push destination is byte-identical after expansion and the deploy-key override | ||
| is preserved. This is documentation text, not executed release automation. | ||
|
|
||
| ## Security-boundary judgement (MAINTAINERS.md) | ||
|
|
||
| The PR touches `package.json` version metadata and a release runbook document. | ||
| `AGENTS.md` flags release automation — `scripts/release.ts`, | ||
| `.github/workflows/release.yml` — for mandatory security review. **Neither file is | ||
| touched.** Verified: `release.yml` triggers on `workflow_dispatch` only, with an | ||
| explicit `version` input that must equal `package.json` and an immutable commit | ||
| input. A version bump on `dev` therefore cannot initiate a publish; a human | ||
| dispatch with an explicit version is required. | ||
|
|
||
| Dependencies and lockfiles are unchanged, and no scheduled, push-triggered, | ||
| auto-merge, or version-keyed publish path exists: `release.yml` is | ||
| `workflow_dispatch`-only and additionally rejects any ref that is not `main` or | ||
| `preview`. A version bump on `dev` cannot publish. | ||
|
|
||
| **It is still not unreviewed-autonomous.** | ||
| `.github/scripts/pr-sponsored-surface.cjs` lists `package.json` as a restricted | ||
| surface, and `MAINTAINERS.md` requires approval from at least one maintainer who | ||
| is not the author, plus explicit security review for release/package boundaries. | ||
| The PR body's unticked box says exactly this. | ||
|
|
||
| A round-level instruction to "merge the bug PRs" is not the exact-head PR approval | ||
| that `MAINTAINERS.md` and GitHub require. **Approval gate: before merge, a | ||
| non-author maintainer approves #2766 at its exact head.** `Ingwannu` is the | ||
| author, so the approval must come from another maintainer account. Cannot be | ||
| self-satisfied and cannot be inferred from this document. | ||
|
|
||
| ## TESTS | ||
|
|
||
| No new test. The behavior proof is that the two already-red repository gates turn | ||
| green, which is observable on the merged tree and on post-merge `dev` CI. | ||
|
|
||
| ## Verification (C) | ||
|
|
||
| ```bash | ||
| # merged tree already built as mtp/2766 | ||
| bun x tsc --noEmit # expect exit 0 | ||
| bun test tests/release-version-line.test.ts # expect 3 pass / 0 fail | ||
| bun run privacy:scan # expect exit 0 | ||
| ``` | ||
|
|
||
| Post-merge, the decisive evidence is the *next* PR's matrix: re-run CI on #2767 or | ||
| #2764 and confirm `ci`, `macos`, `test 3/4`, `gates` go green with no change to | ||
| their own diffs. That is the proof the keystone actually was the keystone. | ||
|
|
||
| ## Lane execution | ||
|
|
||
| Ready, mergeable, green, 0 behind, targets `dev`, needs no rebase — so no | ||
| `codex/` branch is required. | ||
|
Comment on lines
+98
to
+101
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Resolve the Lines 82-83 of 🤖 Prompt for AI Agents |
||
|
|
||
| Merge sequence, in order, none skippable: | ||
|
|
||
| 1. Confirm the merged-tree receipts above. | ||
| 2. **Obtain a non-author maintainer approval at the exact head `076ad3036`.** | ||
| `gh pr view 2766 --json reviewDecision` must read `APPROVED`, not | ||
| `REVIEW_REQUIRED`. | ||
| 3. `gh pr merge 2766`. | ||
|
|
||
| If step 2 cannot be satisfied in this round, #2766 exits as **NEEDS_HUMAN | ||
| (approval)** — and because it is the keystone, every PR gated behind it inherits | ||
| that outcome. That is a real possible terminal state for this round, not a | ||
| formality to route around. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| # 011 — wp1 outcome: roadmap lock | ||
|
|
||
| Docs-only cycle. No production code changed. Deliverable is the plan unit itself: | ||
| `000_plan.md` plus six decade docs, locked at `77635d8c9`. | ||
|
|
||
| ## A-gate: two independent Sol-high reviewers, four rounds | ||
|
|
||
| | Round | Lane | Verdict | Result | | ||
| |---|---|---|---| | ||
| | r1 | facts | NEAR-PASS | 1 correction applied (`27b040931`) | | ||
| | r1 | judgement | **FAIL** | 2 blockers, both accepted | | ||
| | — | judgement re-verify | **FAIL** | 1 blocker incompletely closed | | ||
| | — | judgement re-verify | PASS | both closed | | ||
| | r2 | judgement confirm | PASS | rebound to the repaired files | | ||
|
|
||
| ### What the facts reviewer independently re-derived | ||
|
|
||
| Re-ran `git merge-tree` for all 13 PRs from `dev@8b1b65b8d` and confirmed all 12 | ||
| clean tree hashes matched `mtp/<n>^{tree}` exactly before typechecking, plus the | ||
| `#2497` conflict. Counted every changed path across all 13 PRs and confirmed the | ||
| contention map is exhaustive: `src/codex/auth-context.ts` (#2638, #2497) and | ||
| `src/server/responses/core.ts` (#2745, #2638, #2497), nothing else shared. | ||
|
|
||
| Its correction: the plan had collapsed two distinct shared defects into one per-PR | ||
| line. `test 3/4` and `macos` fail on `release version line`; `gates` fails on | ||
| `privacy:scan`; `ci` is the fan-in. #2747 has no `gates` failure at all because | ||
| its head predates the runbook document. | ||
|
|
||
| ### What the judgement reviewer caught — the two that mattered | ||
|
|
||
| **1. I violated this repository's own security rule.** The plan reproduced the | ||
| unfixed #2745 credential-boundary defect — mechanism, activation sequence, | ||
| remediation direction — inside `devlog/`, a public tracked directory, while the PR | ||
| is open. `AGENTS.md` §"Security working notes" forbids exactly that, and says so in | ||
| a section written because maintainer-authored triage had done it before. | ||
|
|
||
| My error in reasoning: I treated the detail as publishable because the reviewer had | ||
| already written it in a public PR comment. But the rule keys on whether the **fix | ||
| has shipped**, not on where the analysis first appeared. An open PR means | ||
| pre-disclosure. | ||
|
|
||
| The first repair was incomplete — the `TESTS` section still named the regression | ||
| design, which carries the activation shape without the prose. The reviewer caught | ||
| that too. Both are now in `.tmp/2745-security-triage.md` (gitignored, confirmed via | ||
| `git check-ignore`). | ||
|
|
||
| **2. Every merge lane skipped the approval `MAINTAINERS.md` requires.** The plan | ||
| went from "CI green" straight to `gh pr merge`. `MAINTAINERS.md:57-59` requires | ||
| approval from at least one maintainer who is not the author, and | ||
| `.github/scripts/pr-sponsored-surface.cjs:52` lists `package.json` as a restricted | ||
| surface. All four Ingwannu PRs read `REVIEW_REQUIRED`. A round-level instruction | ||
| from the user is not an exact-head PR approval. | ||
|
|
||
| It also refuted the release-safety framing as incomplete rather than wrong: no | ||
| scheduled, push-triggered, auto-merge, or version-keyed publish path exists | ||
| (`release.yml` is `workflow_dispatch`-only and rejects any ref that is not `main` | ||
| or `preview`), but `package.json` is still a restricted surface needing review. | ||
|
|
||
| ### Approval path, resolved | ||
|
|
||
| The operator is authenticated as `lidge-jun` (`gh auth status`), listed in | ||
| `MAINTAINERS.md:10` as project owner. `Ingwannu` is a separate maintainer. A | ||
| `lidge-jun` approval of an Ingwannu-authored PR is therefore a valid non-author | ||
| maintainer approval, confirmed by the reviewer against `MAINTAINERS.md:57-59`. | ||
| The gate is satisfiable without self-approval. | ||
|
|
||
| ## Keystone verification (full suite, remote) | ||
|
|
||
| `mtp/2766` pushed as `codex/mtp-2766-probe`, checked out on `lidge` | ||
| (`~/ocx-ci/opencodex`), full `bun run test` under `ocx-run`: | ||
|
|
||
| ``` | ||
| k2766: OK rc=0 finished 2026-08-28T00:25:04+09:00 | ||
| 15334 pass / 0 fail | ||
| ``` | ||
|
|
||
| Focused, on the same merged tree: | ||
|
|
||
| ``` | ||
| tests/release-version-line.test.ts 3 pass / 0 fail | ||
| bun run privacy:scan Privacy scan passed (exit 0) | ||
| bun x tsc --noEmit exit 0 | ||
| ``` | ||
|
|
||
| On plain `dev` the same scan fails on the runbook literal, and the same test fails | ||
| repository-wide. The keystone claim is proven on both sides. | ||
|
|
||
| ## Gate honesty note | ||
|
|
||
| The typecheck gate was verified rather than trusted: 12 merged trees compiling in | ||
| ~12s looked like a no-op, so a deliberate `const x: number = "str"` was injected | ||
| into a merged worktree — `error TS2322`, exit 1. The speed is real; this repository | ||
| runs the native TypeScript 7.0.2 compiler at ~0.44s for a full typecheck. | ||
|
|
||
| ## Carried into wp2 | ||
|
|
||
| 1. Merge #2766 first; it is the only PR that can produce a trustworthy green | ||
| matrix for the others. Approval at exact head `076ad3036` before merge. | ||
| 2. Review, rebase, and verification of other PRs may proceed in parallel — only | ||
| the merges serialize. | ||
| 3. Follow-up outside this round's scope: the same pre-disclosure material exists | ||
| in `devlog/_plan/260826_wp7e_presence_driven_oauth_failover/` and | ||
| `devlog/_plan/260827_dev_hardening/`. Pre-existing, belongs to other active | ||
| work streams, needs separate authority. **Escalate; do not silently rewrite.** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Correct the CI-failure narrative across the round records.
The current text attributes four failing required jobs and a
gatesfailure to all three PRs, but the phase evidence records that#2747had greengatesand only the release-version failure. Assign failures per PR: thegates/privacy:scanissue applies to#2764and#2767, while#2747should be documented separately. Update the corresponding summary in070_outcome.mdand the phase statement in021_wp2_outcome.mdso the keystone evidence and retrospective agree.📍 Affects 2 files
devlog/_plan/260827_igwanu_bug_pr_merge_round/000_plan.md#L12-L34(this comment)devlog/_plan/260827_igwanu_bug_pr_merge_round/021_wp2_outcome.md#L21-L23🤖 Prompt for AI Agents