Skip to content

Commit c8c8dc3

Browse files
lidge-junlidge-jun
authored
test(auth): close the startup-prime window that rotates the credential mid-fixture (#3139)
* docs(devlog): plan merge train round 3 Roadmap for landing the green PRs, retiring the superseded ones, and rebasing the rest, frozen at dev=132b557ad. Includes the round-1 audit synthesis: three blockers folded (fork PRs are carried by cherry-pick rather than force-pushed, because enforce-pr-target.yml applies the readiness checklist to authors without push permission; #3039's closure withdrawn because #3104 prints the configured budget where #3039 printed the elapsed wait; the src/service.ts overlap is 330470e, not 0ef04e6) and two rebutted with evidence. * docs(devlog): record wp1 — #3114 landed as abcda8e * docs(devlog): record the wp2 security review for #3122 * docs(devlog): record wp3 — #3134 landed, #3128 flake premise corrected * docs(devlog): record wp5 — #3077 closed, #3109/#3112 rebased * docs(devlog): locate the websocket refresh flake, and correct the #3128 premise * docs(devlog): prove the flake mechanism and correct its direction * docs(devlog): mark the superseded flake explanation in the wp5 record * test(auth): install the fake clock and fetch stub before startServer startServer returns synchronously but arms an async pool-quota prime that outlives its return (src/server/index.ts:2054-2064). That prime calls getValidCodexToken, which can rotate the very credential these assertions read, and fetches a real host unless the stub is up. Both fixtures installed Date.now and globalThis.fetch AFTER startServer, leaving a window two dynamic import() resolutions wide where the prime ran against the real clock and real fetch. On a warm local module cache it resolved before the fixture finished; on a loaded CI runner it did not, and seenAuth[0] was already the rotated token. Measured rather than assumed: OPENCODEX_DEBUG_QUOTA=1 prints refreshed=1 on every run of both the fixed and unfixed trees, so the prime always fires. The fix does not suppress it -- it makes it run inside the fixture's controlled world. The thread-affinity test at :2131 had the identical shape and is fixed too.
1 parent b14b741 commit c8c8dc3

13 files changed

Lines changed: 1078 additions & 9 deletions

‎devlog/_plan/260901_merge_train_round3/000_plan.md‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
# 000 — merge train round 3: land the green, retire the superseded, rebase the rest
2+
3+
Frozen at `dev` = `132b557ad` (2.40.0), 2026-09-01T03:30Z. 55 open PRs, 47 open issues.
4+
5+
## Objective
6+
7+
Land the pull requests whose disposition requires no maintainer judgment, close the ones a
8+
landed reimplementation has already absorbed, and leave the remaining maintainer-authored
9+
PRs rebased onto current `dev` so their next review round reads a live head.
10+
11+
This train does **not** implement anything. Every production line it moves is a line some
12+
other PR already wrote and some other reviewer already read.
13+
14+
## The state that makes this train possible
15+
16+
Three landings in the last hour changed what "red" means on this backlog:
17+
18+
| commit | what it changed |
19+
| --- | --- |
20+
| `33d32b6a3` (#3128) | pinned the WebSocket refresh account — the `server local API auth > websocket passthrough refreshes pool auth for each response.create turn` flake |
21+
| `3e0f99a19` (#3127) | moved `dev` to 2.40.0 after the v2.39.0 release |
22+
| `6f415baef` (#3129) | made the dev version bump actually fire |
23+
24+
Both of those are why the four candidates below currently show a red matrix, and neither red
25+
is about the change under review:
26+
27+
- **#3104, #3109, #3112** are red on the `server-auth` WebSocket assertion. `070_outcome.md`
28+
of `260901_release_train_2390` diagnosed it: the credential is saved with
29+
`expiresAt: now + 120_000` against a `REFRESH_SKEW_MS` of `60_000`, and `startServer(0)`
30+
runs before `Date.now` is pinned, so the first turn can land on the wrong side of the
31+
skew boundary and refresh early. #3128 fixed it. Any head that predates #3128 still shows it.
32+
- **#3122** is red on `release version line > the in-tree version is never behind a released one`.
33+
Its base predates the 2.40.0 bump, so the in-tree version is behind the published 2.39.0.
34+
Rebasing onto `132b557ad` is the whole fix.
35+
36+
**Therefore: no candidate is judged on a pre-rebase matrix.** Every merge in this train waits
37+
for a green matrix on a head rebased onto `132b557ad` or later.
38+
39+
## Work-phase map (dependency-ordered)
40+
41+
```
42+
wp0 roadmap (this unit)
43+
├── wp1 #3114 docs-only, no production surface → 010
44+
├── wp2 #3122 provider PATCH validation exception → 020
45+
├── wp3 #3104 service budget + scheduler ownership → 030 (+ closes #3009 #3064 #3039 #3067)
46+
├── wp4 #3042 test-only pid probe → 040
47+
└── wp5 #3077 close, #3109/#3112 rebase → 050
48+
```
49+
50+
The order is blast-radius ascending, which here coincides with dependency order: wp1 touches no
51+
code, wp2 touches one validation call site, wp3 touches `src/service.ts` and is the only phase
52+
that closes issues, wp4 touches tests only, wp5 touches no `dev` state at all. wp1-wp5 are
53+
independent of each other and depend only on wp0; they are sequenced rather than parallel
54+
because each merge invalidates the next candidate's merge base.
55+
56+
## Scope boundary
57+
58+
**IN**
59+
60+
- Merging #3114, #3122, #3104, #3042 into `dev` after an exact-head green matrix.
61+
- Closing #3039, #3067 (absorbed by #3104), #3077 (stale wrong-branch bump).
62+
- Rebasing #3109 and #3112 onto current `dev` and force-pushing their branches.
63+
- Dropping `926a8d8c4` from #3109 — the same change landed as #3128.
64+
- Closing #3009 and #3064 when #3104 lands.
65+
66+
**OUT**
67+
68+
- **#3117.** It reverses a direction `b46164e78` (#3100) deliberately pinned one day earlier:
69+
"A configured id the provider no longer lists must not be retained on the strength of a
70+
format match alone; #1690 is the explicit opt-in for that." Landing #3117 is a policy
71+
decision about #1690, not a merge-train mechanical.
72+
- **#3061.** `CHANGES_REQUESTED` with a substantive rebuttal: the 90 s budget reproduced the
73+
same failure, so the ceiling was not the only failure mode.
74+
- **Re-implementing the review blockers on #3109/#3112.** Those are real and unresolved;
75+
this train rebases them and stops.
76+
- Any `main`/`preview` promotion, npm publish, or release.
77+
- Any new production logic.
78+
79+
## Verifier
80+
81+
`gh pr checks <n>` on the exact head, requiring every non-skipped check to pass. Run against
82+
the post-rebase head only. Local full suite is prohibited by the operator; focused local checks
83+
are permitted where a rebase produced a textual conflict that needs resolving.
84+
85+
Verified before adoption: `gh pr checks 3122` exits non-zero today and names
86+
`release version line`, and `gh run view --job 99726180475 --log-failed` shows exactly that
87+
one assertion. The command reads the change target because it reports the check suite bound
88+
to the PR's head SHA.
89+
90+
## Terminal outcomes
91+
92+
- `DONE` — four merges landed, three closes recorded, two branches rebased and pushed.
93+
- `BLOCKED` — a specific PR whose CI fails three consecutive times for an infrastructure
94+
reason; report it and continue the others.
95+
- Partial completion is reported per work-phase, never averaged into a single claim.

‎devlog/_plan/260901_merge_train_round3/002_audit_round1_synthesis.md‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
# 002 — audit round 1: what the reviewer caught, and what it over-read
2+
3+
Reviewer: `gpt-5.6-sol`/high, read-only lane, `VERDICT: FAIL` with 5 blockers.
4+
Three are real and change the plan. Two are rebutted with evidence. This document records
5+
both dispositions because a rebuttal I do not write down is a rebuttal the next round
6+
re-litigates.
7+
8+
## Accepted — blocker 5: the conflict attribution was wrong, and so were the distances
9+
10+
The reviewer is right and I checked it myself.
11+
12+
```
13+
$ git show 0ef04e640 --stat | tail -4
14+
src/cli/dispatch.ts | 11 ++++++--
15+
src/cli/index.ts | 9 +++++-
16+
tests/cli-dispatch.test.ts | 69 ++++++++++++++++++++++++++++++++++++++++++++++
17+
```
18+
19+
`0ef04e640` never touches `src/service.ts`. I named it because its subject line
20+
("stop start shadowing a live configured-port proxy") reads like service territory. That is
21+
reasoning from a commit message instead of from a diff, which is exactly the error the
22+
audit exists to catch.
23+
24+
The real dev-side overlap is `330470e74` (#3118, `fix(stop): typed stop outcome`) — 50 files,
25+
and `src/service.ts` is among them. Measured overlap for the four rebase candidates:
26+
27+
| PR | branch | files changed on BOTH sides since merge-base |
28+
| --- | --- | --- |
29+
| #3104 | `codex/3009-windows-cold-start` | `src/service.ts` |
30+
| #3109 | `codex/3063-combo-compact-failover` | `src/adapters/openai-responses.ts`, `tests/server-auth.test.ts` |
31+
| #3112 | `codex/2999-native-main-refresh-claim` | **none** |
32+
| #3042 | `fix/test-dead-pid-probe` | `tests/responses-state.test.ts` |
33+
34+
`030` is amended: expect `src/service.ts` against `330470e74`, not `0ef04e640`.
35+
36+
The behind-counts in `010`/`020`/`040`/`050` were measured before `132b557ad`, `33d32b6a3`,
37+
`3e0f99a19` and `6f415baef` landed during this session. They are stale by exactly the number
38+
of commits that landed while I was writing. Real distances from `132b557ad`: #3114 = 26,
39+
#3122 = 5, #3042 = 59, #3109 = 27, #3112 = 26. Recorded here rather than chased through five
40+
documents, since the number moves again on every merge this train performs.
41+
42+
## Accepted — blocker 4: #3104 drops a behaviour #3039 authored
43+
44+
Verified in both trees.
45+
46+
```
47+
$ git show pr3039:src/service.ts | sed -n '742,753p'
48+
const startedAt = elapsed();
49+
...
50+
+ `${Math.max(1, Math.round((elapsed() - startedAt) / 1000))}s.\n`
51+
52+
$ git show pr3104:src/service.ts | sed -n '742,750p'
53+
const healthBudgetMs = deps.timeoutMs ?? serviceInstallHealthMs();
54+
...
55+
+ `${Math.trunc(healthBudgetMs / 1000)}s.\n`
56+
```
57+
58+
#3039's comment states the intent plainly: "The elapsed time, not the constant: a caller
59+
that passes its own timeoutMs used to be told it had waited 20s whatever it waited."
60+
#3104 prints the budget. Since #3104 also adds a post-deadline grace knock
61+
(`src/service.ts:719`), the printed number can now understate the real wait — the exact
62+
failure mode #3039 set out to fix, reintroduced by the PR that claims to supersede it.
63+
64+
This does not block **merging** #3104: the budget message is honest about the budget, and
65+
the security-relevant half (SID-exact scheduler ownership) is unaffected. It blocks
66+
**closing #3039 as fully superseded**. Amended in `030`: #3039 stays open with a comment
67+
recording precisely which contribution was not carried, so the elapsed-time diagnostic is a
68+
tracked follow-up rather than a silent drop.
69+
70+
## Accepted — blocker 3: a maintainer push resets a contributor PR's readiness
71+
72+
`.github/workflows/enforce-pr-target.yml:740-746`:
73+
74+
```
75+
// The readiness gate applies to contributors (no push permission).
76+
const checklistRequired = !authorIsMaintainer;
77+
```
78+
79+
and `:781-786` — "A completed checklist is an attestation about a specific head" — with the
80+
push resetting the boxes and re-drafting.
81+
82+
Both fork candidates are contributors:
83+
84+
```
85+
$ gh api repos/lidge-jun/opencodex/collaborators/Flowershangfromthebranches/permission --jq .permission
86+
read
87+
$ gh api repos/lidge-jun/opencodex/collaborators/lifrary/permission --jq .permission
88+
read
89+
```
90+
91+
So a maintainer force-push to #3122 or #3042 re-drafts the PR and resets a checklist only
92+
the author can tick. "Rebase, wait for green, merge" is not available for either.
93+
94+
**This is the blocker that reshapes the train**, and it is not a paperwork objection: the
95+
gate exists so an author attests that the code they are shipping is the code that was
96+
tested. Amendment: fork PRs are landed by **cherry-picking onto a maintainer branch** with
97+
authorship preserved (`git cherry-pick -x`, original `Author:` intact), opened as a
98+
maintainer PR that credits and closes the original — the pattern this repository already
99+
uses (#3104 carries #3039/#3067; #3109 carries #3063; #3111 carries #2989). The contributor
100+
keeps authorship in `git log`; the readiness gate is satisfied by a maintainer author rather
101+
than circumvented.
102+
103+
## Rebutted — blocker 2: the security-notes rule does not reach this material
104+
105+
The reviewer reads `AGENTS.md:105-127` as forbidding any devlog note that touches an
106+
unfixed defect. That is broader than the rule, which is scoped to **security** work:
107+
"unreleased findings, severity assessments, draft advisories, exploit or bypass reasoning,
108+
reproduction steps for an unfixed defect, and pre-disclosure patch plans."
109+
110+
The test the file gives is explicit: "is there already a public diff that reveals this
111+
weakness?"
112+
113+
- **#3122** is characterised as "an unshipped destination-policy/SSRF fix". It is not an
114+
SSRF fix. The PR permits the `198.18.0.0/15` fake-IP range on the provider PATCH path
115+
that creation and re-enable already permit — it **relaxes** a validator to match its own
116+
sibling call sites, and the asymmetry is visible in the open PR diff. There is no
117+
weakness disclosed that the public PR does not already show.
118+
- **#3112's** three failure modes are quoted from `Ingwannu`'s **public review** on the open
119+
PR. Restating a public review comment in a devlog discloses nothing.
120+
- **#3114's `070_outcome.md`** discusses #3000's musl `dlopen` and late-cancel grant
121+
discard. #3000 is `CLOSED` (2026-08-31T19:13:28Z) and was never merged — the code never
122+
shipped, so there is no deployed weakness to disclose. The note explains why a PR was
123+
rejected, which is the closure rationale, not an advisory.
124+
125+
There is one thing the reviewer is right about even though the blocker is wrong: **read the
126+
#3114 unit before merging it** rather than approving it because it is docs-only. `010`
127+
already required that. The read stays; the blocker is not accepted.
128+
129+
## Rebutted — blocker 1: no staged diff
130+
131+
The reviewer required a staged index to anchor its audit. That is a habit from reviewing a
132+
patch, not a rule of this repository, and this phase is a P-phase plan audit — the artifact
133+
is the six documents, which the reviewer read and cited by line. Nothing is staged because
134+
nothing is committed yet; `git add` before an audit would not have changed a single blob it
135+
examined. Recorded and dismissed.
136+
137+
## Round verdict
138+
139+
`GO-WITH-FIXES` after amendment: blockers 3, 4, 5 folded into `020`/`030`/`040`; blockers
140+
1 and 2 rebutted with evidence above. The train's shape changes in one material way — fork
141+
PRs are carried, not force-pushed — and one closure is withdrawn.

‎devlog/_plan/260901_merge_train_round3/010_wp1_3114_docs_devlog.md‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
# 010 — wp1: land #3114 (docs devlog, 8/31 non-priority-70 triage round)
2+
3+
PR #3114, author `lidge-jun`, branch `codex/triage-round-devlog-pr`, label `documentation`.
4+
`+820 −0` across 6 files, all under `devlog/_plan/`.
5+
6+
## Why this is mechanical
7+
8+
Nothing in the build, typecheck, or test path reads `devlog/` (`AGENTS.md`, "The `devlog`
9+
directory"). The check suite agrees: on head `d6330f7c` every heavy job reports `skipping`
10+
— `gates`, `macos`, `test ${{ matrix.shard }}/4`, `storage policy`, `api usage`,
11+
`keyring`, `npm-global` — and the five that run (`ci`, `changes`, `hygiene`,
12+
`enforce-target`, `react-doctor`, `label`, `resolve-pr`) all pass.
13+
14+
`privacy:scan` does read `devlog/`, and `hygiene` passes, which is the gate that matters
15+
for a public devlog.
16+
17+
## Pre-merge check
18+
19+
The unit records a triage round that is already closed. Confirm before merging that it
20+
contains no pre-disclosure security material (`AGENTS.md`, "Security working notes"): the
21+
test is whether a public diff already reveals each weakness named. The round's dispositions
22+
are PR closes and supersessions, all visible in public git history.
23+
24+
## Steps
25+
26+
1. `git fetch origin` and confirm `origin/dev` = `132b557ad` or later.
27+
2. `gh pr checks 3114` — every non-skipped check passes.
28+
3. Read the six added files for security-note residue.
29+
4. Approve, then `gh pr merge 3114 --squash`. `mergeStateStatus` is `BLOCKED` only for the
30+
missing approval; no admin override should be needed.
31+
5. `git fetch origin && git log --oneline -1 origin/dev` names #3114.
32+
33+
## Rebase question
34+
35+
Head `d6330f7c` sits 24 commits behind `dev`. A docs-only unit adding new files under a new
36+
directory has no conflict surface, and `enforce-target`'s ancestry heuristic exempts authors
37+
with push permission. Rebase only if GitHub reports a conflict.
38+
39+
## Accept criteria
40+
41+
- `origin/dev` contains the merge commit naming #3114.
42+
- The six documents are present at `origin/dev`.
43+
- No file outside `devlog/_plan/` changed.

‎devlog/_plan/260901_merge_train_round3/011_wp1_outcome.md‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
# 011 — wp1 outcome: #3114 landed
2+
3+
`abcda8e134d7e3222d72877fe83c77fcb492a821`, merged 2026-09-01T03:57:16Z, squash.
4+
6 files / +820 lines, all under `devlog/_plan/260831_bug_triage_nonprio70/`.
5+
6+
## Audit before approval
7+
8+
`010` required reading the unit rather than waving it through because it is docs-only.
9+
Four checks, all clean:
10+
11+
| check | result |
12+
| --- | --- |
13+
| credential/identifier scan over the full diff | zero hits |
14+
| `bun run privacy:scan` | Privacy scan passed |
15+
| `bun test tests/repo-hygiene.test.ts` | 12 pass / 0 fail |
16+
| pre-disclosure test on the one security-adjacent passage | cleared |
17+
18+
The fourth is the one that mattered. `070_outcome.md:213-216` describes #3000's
19+
`libc.so.6` `dlopen` — which throws on musl, so credential publication would fail on
20+
Alpine — and its `signal.aborted` check placed before `persistRefreshedMainAuthJson`,
21+
discarding a grant the provider already rotated. `AGENTS.md` asks whether a public diff
22+
already reveals the weakness. #3000 is `CLOSED` (2026-08-31T19:13:28Z) and was never
23+
merged: the code never shipped, so there is no deployed weakness. The passage is a closure
24+
rationale, and closure rationales are exactly what a `_fin`-bound record is for.
25+
26+
The repository answers this question mechanically too, and it agrees:
27+
`tests/repo-hygiene.test.ts` asserts `no open devlog plan carries an unresolved security
28+
verdict`, and it passes against the merged tree.
29+
30+
## Admin merge, and why
31+
32+
`gh pr review 3114 --approve` is refused by GitHub: *"Can not approve your own pull
33+
request."* `dev` carries a ruleset requiring a reviewed pull request. A self-authored PR
34+
therefore has no non-admin route, and this train was explicitly authorized to use one.
35+
36+
Worth stating plainly rather than burying: **admin merge is not review.** What stands in
37+
for review here is the audit above, and it is weaker than a second pair of eyes would be.
38+
For a 6-file docs-only change whose two mechanical gates both pass, that trade is
39+
defensible. It would not be for the three code PRs later in this train.
40+
41+
## Residual
42+
43+
The remote branch was deleted; the local `codex/triage-round-devlog-pr` survives because
44+
worktree `/Users/jun/.codex/worktrees/2a44/opencodex` still has it checked out. Left alone —
45+
that worktree is not this train's to disturb.

‎devlog/_plan/260901_merge_train_round3/020_wp2_3122_provider_patch_fakeip.md‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
# 020 — wp2: land #3122 (canonical fake-IP addresses on provider PATCH)
2+
3+
PR #3122, author `Flowershangfromthebranches` (fork, `maintainerCanModify = true`),
4+
branch `fix/openai-patch-fake-ip`, labels `bug` + `review-ready`. `+150 −1` across 2 files:
5+
`src/server/management/provider-routes.ts` and `tests/management-provider-validation.test.ts`.
6+
7+
## The defect
8+
9+
Canonical OpenAI provider **creation** and **re-enable** already pass
10+
`allowBenchmarkAddresses`, which permits the `198.18.0.0/15` range that Clash/Mihomo-style
11+
fake-IP DNS returns. The ordinary field-mask **PATCH** path did not pass the same exception,
12+
so a provider that was created successfully rejected a later context-window PATCH against
13+
the identical address.
14+
15+
One call site, one flag, and the asymmetry is the whole bug. The test file is the larger half
16+
of the diff.
17+
18+
## Why the matrix is red, and why it is not this change
19+
20+
`gh run view --job 99726180475 --log-failed` on head `f463e124`:
21+
22+
```
23+
(fail) release version line > the in-tree version is never behind a released one [74.60ms]
24+
1 tests failed:
25+
```
26+
27+
That assertion compares the in-tree `package.json` version against the published release
28+
line. The head's base predates `3e0f99a19` (#3127, "move dev to 2.40.0 after the v2.39.0
29+
release"), so the in-tree 2.39.0 is exactly level with — and by the gate's reading, behind —
30+
the released 2.39.0. It is unrelated to provider validation and disappears on rebase.
31+
32+
The branch is 3 commits behind `dev`, so this is a short rebase.
33+
34+
## Amended by audit round 1 (blocker 3): carry, do not force-push
35+
36+
`maintainerCanModify` is true, so a force-push is technically available. It is the wrong
37+
move. `.github/workflows/enforce-pr-target.yml:740-746` applies the readiness checklist to
38+
authors without push permission, and `Flowershangfromthebranches` has `read`. A maintainer
39+
push re-drafts the PR and resets four boxes only the author can tick — the train would strand
40+
the PR in draft, waiting on a contributor, having done the work.
41+
42+
So this lands the way this repository already lands contributor work (#3104 carries
43+
#3039/#3067, #3109 carries #3063, #3111 carries #2989): **cherry-pick onto a maintainer
44+
branch with authorship preserved.**
45+
46+
## Steps
47+
48+
1. `git checkout -b codex/3122-provider-patch-fake-ip origin/dev`.
49+
2. `git cherry-pick -x f463e124` — `-x` records the source commit; the original
50+
`Author:` line is preserved by cherry-pick without further flags.
51+
3. `git show --format='%an <%ae>' -s` to prove the authorship survived.
52+
4. Push the maintainer branch and open a PR against `dev` that credits
53+
@Flowershangfromthebranches, links #3122, and fills the PR template.
54+
5. Wait for the full matrix. `release version line` must pass — that assertion is the entire
55+
reason the original head is red, and a rebased base is the fix. If it still fails, stop:
56+
the diagnosis is wrong.
57+
6. Merge, then close #3122 with a comment naming the merged commit.
58+
59+
## Accept criteria
60+
61+
- Carrier head's matrix fully green, including `macos` and all four `test` shards.
62+
- `git log origin/dev` shows the commit authored by the original contributor.
63+
- The diff at `dev` is still 2 files.
64+
- #3122 closed with credit, not merged-and-forgotten.

0 commit comments

Comments
 (0)