Skip to content

Commit 28afa74

Browse files
committed
docs(devlog): 260712 PR batch landing unit (#96-#103) — plan, audits, DONE record
1 parent 47072be commit 28afa74

8 files changed

Lines changed: 380 additions & 0 deletions
Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
# 260712 — PR batch landing (#96#103) with review fixes
2+
3+
## Objective
4+
5+
Land all 8 open PRs from Wibias on lidge-jun/opencodex, honoring the parallel sol-reviewer
6+
verdicts (2026-07-12 session 019f54d8): merge approved content, fix the named blockers on the
7+
PR branches, merge in stack order, and leave main green with zero open PRs from the batch.
8+
9+
## Ground truth (explored)
10+
11+
- Stack: `cde614a1`(#96) → `c6af7f90`(#97) → `6bb3aecf`(#98) → `e8794d22`(#99) →
12+
`84d601f9`(#100) → `37b4b2d6`(#101) → `21e58b09`(#102), all on fork `Wibias/opencodex`,
13+
all base=main. #103 = `aa888074` (independent; overlaps only `src/server/management-api.ts`,
14+
`gui/src/pages/Debug.tsx`).
15+
- Permissions: admin on repo; `maintainerCanModify: true` on PRs → can push fix commits to fork
16+
branches. main unprotected; merge-commit method is repo precedent (PR #87).
17+
- `origin/dev` == `origin/main` == `182ddae9` (v2.7.8). Land to main; fast-forward dev in WP7.
18+
- Bun 1.3.14 `node:zlib` enforces `maxOutputLength` for gzip AND zstd (verified:
19+
ERR_BUFFER_TOO_LARGE) → bomb fix is straightforward.
20+
- Worktree HEAD has an unrelated unpushed commit `953fb5b9` (another task's work) — do not
21+
touch it; all landing work happens on branches cut from `origin/main` / fork branches.
22+
23+
## Landing mechanics (decision)
24+
25+
Fix-on-fork-branch + `gh pr merge N --merge` in stack order. Identical SHAs reach main, so each
26+
successor PR's ORIGINAL stack prefix disappears from its diff; GitHub marks each PR merged
27+
(attribution preserved). Drafts get `gh pr ready N` first. `--merge` is REQUIRED throughout —
28+
squash/rebase would break the ancestry assumption (audit blocker 3). Fix commits pushed to a
29+
predecessor branch land on main via that predecessor's merge; successor branches never contain
30+
them, but 3-way merges cannot revert them (successor side unchanged since base). After EVERY
31+
merge, verify the next PR's `mergeStateStatus` and effective diff before merging it.
32+
33+
Merge order: #103#96#97#98#99#100#101#102.
34+
35+
Known conflict point (audit blocker 2, verified via merge-tree): after #103 lands,
36+
`gui/src/pages/Debug.tsx` CONFLICTS for #99..#102 tips. Resolution: WP5 merges origin/main into
37+
the #99 branch and resolves Debug.tsx ONCE (combine #99 lint refactor + #103 injection wiring).
38+
After #99 lands, #100#102 merge cleanly (their own commits don't touch Debug.tsx, so 3-way has
39+
only one changed side). #96#98 are unaffected (verified: no conflict vs aa888074).
40+
41+
## Work-phase map (dependency-ordered, one PABCD cycle each)
42+
43+
| WP | Doc | Content |
44+
|----|-----|---------|
45+
| 1 | 010 | Plan (this doc set) + merge #103 |
46+
| 2 | 020 | #96 + bounded decompression + SSRF resolution/reserved-ranges + activation tests → merge |
47+
| 3 | 030 | #97 + restore GUI build gate in ci.yml → merge |
48+
| 4 | 040 | #98 as-is → merge |
49+
| 5 | 050 | #99 + Usage.tsx cancellation guard → merge |
50+
| 6 | 060 | #100/#101/#102 as-is → merge |
51+
| 7 | 070 | Closeout: verify zero open batch PRs, main CI green, dev fast-forward, comments |
52+
53+
## Reviewer verdicts folded in
54+
55+
- #96 BLOCK: bomb cap post-allocation (`src/server/request-decompress.ts:43-48`); SSRF passes
56+
non-literal hostnames unresolved (`src/lib/destination-policy.ts:82-110`); reserved IPv4
57+
ranges incomplete. → WP2 fixes.
58+
- #97 BLOCK: deletes GUI build gate (`.github/workflows/ci.yml:73-76`). → WP3 restores.
59+
- #99 BLOCK: `gui/src/pages/Usage.tsx` fetch has no cancellation → stale overwrite. → WP5 fixes.
60+
- #98/#101/#102/#103 approved (nits recorded, non-blocking). #100 fixture commit legitimate.
61+
62+
## Rebutted residuals (recorded, not implemented)
63+
64+
- DNS rebinding pinning + redirect-hop validation (reviewer #96): the proxy is a loopback
65+
service whose providers are configured by the machine owner; validation-time resolution +
66+
reserved-range default-deny is proportionate. Full dial-time pinning needs a custom dialer
67+
Bun fetch does not expose. Recorded here + in WP2 commit message.
68+
- #103 locale-time nit, #102 exhaustion-test nit, #98 adapter-level regression tests: cosmetic /
69+
non-blocking; noted for future work, not this batch.
70+
71+
## Rollback / partial-landing policy (audit blocker 5)
72+
73+
- Capture each merge-commit SHA in the goalplan ledger as it lands.
74+
- If main CI fails after merge N: STOP merging successors; prefer fix-forward on main; if
75+
infeasible, `git revert -m 1 <merge-sha>` in reverse landing order back to the last green SHA.
76+
- If a pushed fork fix must be withdrawn pre-merge: push a revert commit to the fork branch
77+
(never force-push another author's branch).
78+
79+
## Verification contract
80+
81+
Each WP: targeted `bun test` locally on the fix branch before push; PR checks green before
82+
merge; WP6/WP7 run the full suite. Security fixes carry activation tests
83+
(C-ACTIVATION-GROUNDING-01). Final: `gh pr list` empty for batch, `gh run list --branch main`
84+
green.
85+
86+
## DONE — 2026-07-12 landing record
87+
88+
All 8 PRs merged in planned order; every merge gated on the predecessor's main CI green.
89+
90+
| PR | Merge SHA | Fix commits added on branch |
91+
|----|-----------|------------------------------|
92+
| #103 | d7b0a3fb ||
93+
| #96 | 93fd0897 | 94a1ce72 (inflation-time cap), 74202966 (SSRF DNS + reserved ranges) |
94+
| #97 | d55a1e3a | — (amended: gate returns enhanced in #99; see 030) |
95+
| #98 | 576d8c45 ||
96+
| #99 | 44db2ec1 | a0257db2 (Debug.tsx resolution), 77efcaf7 (Usage.tsx abort guard) |
97+
| #100 | 464fd7d4 ||
98+
| #101 | 5dadf6d8 | 910fcaca (GUI-gate contract test) |
99+
| #102 | 47072be3 ||
100+
101+
Evidence: full suite 2145 pass / 0 fail at WP5 tip; gui lint+build clean; typecheck exit 0;
102+
per-PR CI green at each tip; `dev` fast-forwarded to main (47072be3). Out-of-scope: dependabot
103+
PRs #105+ opened by #97's new dependabot.yml — left for the owner. Deferred nits: #102
104+
retry-exhaustion test, #98 adapter-level regression tests, #103 locale-time format.
105+
What did NOT happen (LOOP-PESSIMIST-01): dial-time DNS pinning/redirect validation
106+
(rebutted residual, loopback threat model); a #97 gate-restore commit (superseded by
107+
stack-internal #99 re-introduction — first plan draft had it wrong).
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
# WP1 — Merge PR #103 (debug log GUI wiring)
2+
3+
Reviewer: MERGE, CI 6/6 green, non-draft, mergeState CLEAN. No code changes needed.
4+
5+
## Steps
6+
7+
1. `gh pr merge 103 --merge` (merge commit, repo precedent).
8+
2. Verify: `gh pr view 103 --json state,mergedAt` shows MERGED.
9+
3. `git fetch origin main` — confirm `aa888074` reachable from origin/main.
10+
11+
## Accept criteria
12+
13+
- c1: `gh pr view 103` state MERGED.
14+
- Expected repository diff: merge commit of `aa888074` only (no local edits) —
15+
merge-only WP (audit blocker 6 declaration).
16+
- Post-merge conflict map (verified via merge-tree, audit blocker 2): #96#98 stay clean;
17+
#99#102 become CONFLICTING on `gui/src/pages/Debug.tsx` — planned resolution happens once
18+
at WP5; do NOT attempt to fix it here.
Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
# WP2 — Land #96 with security fixes
2+
3+
Base: fork branch `codex/harden-request-provider-security` (tip `cde614a1`). Fix commits go on
4+
this branch, pushed to the fork (maintainerCanModify), then `gh pr ready 96` + merge. If #103's
5+
merge made #96 CONFLICTING (management-api.ts), first `git merge origin/main` on the branch.
6+
7+
## Fix 1 — bounded decompression (MODIFY `src/server/request-decompress.ts`)
8+
9+
Current (branch): `Bun.zstdDecompressSync` / `Bun.gunzipSync` / `Bun.inflateSync` fully
10+
allocate, then `assertBodySizeWithinLimit` checks. Verified Bun 1.3.14 `node:zlib` sync fns
11+
enforce `maxOutputLength` during inflation (ERR_BUFFER_TOO_LARGE).
12+
13+
Diff sketch:
14+
15+
```ts
16+
import { gunzipSync, inflateSync, zstdDecompressSync } from "node:zlib";
17+
18+
export function decodeRequestBody(raw, contentEncoding, maxBytes = MAX_DECOMPRESSED_BODY_BYTES) {
19+
// identity path: assertBodySizeWithinLimit(raw, maxBytes)
20+
const opts = { maxOutputLength: maxBytes }; // per-call, from the PARAM
21+
try {
22+
if (encoding === "zstd") decoded = zstdDecompressSync(compressed, opts);
23+
else if (encoding === "gzip" || encoding === "x-gzip") decoded = gunzipSync(compressed, opts);
24+
else if (encoding === "deflate") decoded = inflateSync(compressed, opts);
25+
else throw new UnsupportedContentEncodingError(encoding);
26+
} catch (err) {
27+
if ((err as NodeJS.ErrnoException)?.code === "ERR_BUFFER_TOO_LARGE") {
28+
throw new DecompressedBodyTooLargeError(maxBytes); // PARAM, not constant
29+
}
30+
throw err;
31+
}
32+
return assertBodySizeWithinLimit(decoded, maxBytes); // PARAM threaded through
33+
}
34+
```
35+
36+
(`assertBodySizeWithinLimit` and `DecompressedBodyTooLargeError` gain the byte-limit parameter;
37+
re-audit blocker: options/error/assertion must all derive from the param, never the constant.)
38+
39+
Activation test (MODIFY `tests/request-decompress.test.ts`) — audit blocker 4 fold-back:
40+
refactor `decodeRequestBody(raw, contentEncoding, maxBytes = MAX_DECOMPRESSED_BODY_BYTES)` so
41+
tests INJECT a small cap (e.g. 1024) and feed a ~64KB-inflating payload per codec
42+
(zstd/gzip/deflate) — cheap, deterministic, no giant buffers, and it activates the exact
43+
ERR_BUFFER_TOO_LARGE → DecompressedBodyTooLargeError path during inflation.
44+
45+
## Fix 2 — SSRF hostname resolution + reserved ranges (MODIFY `src/lib/destination-policy.ts`)
46+
47+
a) Extend `classifyIpv4` reserved/non-public ranges (after existing checks):
48+
49+
```ts
50+
if (a === 192 && b === 0 && (o3 === 0 || o3 === 2)) return { kind: "private", detail: "reserved address" };
51+
if (a === 198 && (b === 18 || b === 19)) return { kind: "private", detail: "benchmark address" };
52+
if (a === 198 && b === 51 && o3 === 100) return { kind: "private", detail: "documentation address" };
53+
if (a === 203 && b === 0 && o3 === 113) return { kind: "private", detail: "documentation address" };
54+
if (a >= 224) return { kind: "private", detail: "multicast/reserved address" };
55+
```
56+
57+
(destructure `const [a, b, o3] = octets`.)
58+
59+
b) NEW async export `providerDestinationResolvedError(name, provider): Promise<string|null>`
60+
for `kind === "hostname"` destinations, `dns.promises.lookup(hostname, { all: true, verbatim: true })`,
61+
classify every returned address with classifyIpv4/6; any non-public result → same error string
62+
contract as the sync path. DNS failure (ENOTFOUND etc.) → null (config-time advisory, do not
63+
hard-fail offline startup). Wiring (audit blocker 1 fold-back): the sync helper in
64+
`src/server/auth-cors.ts:155-171` stays sync; the AWAIT goes in the async management write
65+
handler at `src/server/management-api.ts:289-290` right after the existing sync check.
66+
`management-api.ts` overlaps #103 — therefore FIRST merge origin/main (which then contains
67+
#103's aa888074) into the #96 branch, THEN edit. `src/config.ts:147-173` is sync by design —
68+
no DNS there. Router hot path (`src/router.ts:84,114`) stays sync-literal-only — rationale in
69+
000 §Rebutted.
70+
71+
Activation test (MODIFY `tests/*destination*` or the suite covering destination-policy): mock
72+
`node:dns` lookup (bun test `mock.module`) to return `127.0.0.1` / `10.0.0.5` for a hostname →
73+
expect error; `93.184.216.34` → expect null. Literal-range tests for each new reserved block.
74+
75+
## Steps
76+
77+
1. Branch from fork tip; `git merge origin/main` (brings #103, resolves management-api overlap
78+
deterministically — expected clean for #96 per merge-tree); apply fixes;
79+
`bun test tests/request-decompress.test.ts <dest-policy tests>` + full `bun test`.
80+
2. Push to fork branch; wait `gh pr checks 96 --watch`; `gh pr ready 96`; `gh pr merge 96 --merge`.
81+
82+
## Accept criteria
83+
84+
- c2: oversized compressed body rejected during inflation (test output) — no full allocation.
85+
- c3: hostname→private DNS result rejected at management write path; new reserved ranges classified (test output).
86+
- Full suite 0 fail on the branch before merge; PR checks green; #96 MERGED.
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
# WP3 — Land #97 as-is (gate returns enhanced in #99) — AMENDED at WP3-P
2+
3+
Base: fork branch `codex/strengthen-ci-release` (tip `c6af7f90`, contains #96 commits below).
4+
After #96 merges, this PR's effective diff = c6af7f90 only.
5+
6+
## AMENDMENT (WP3-P discovery)
7+
8+
Stack inspection shows `e8794d22` (#99) RE-ADDS the gate STRONGER — separate "GUI lint" +
9+
"GUI build" steps at the same location (verified:
10+
`git diff wibias/codex/strengthen-ci-release wibias/codex/gui-lint-remediation -- .github/workflows/ci.yml`).
11+
The #97 deletion is a stack-internal transition, not a permanent regression. Restoring the old
12+
step on #97 would only create an extra ci.yml conflict at WP5's already-manual merge.
13+
14+
Decision: merge #97 AS-IS. The gate gap on main is bounded to this controlled landing window
15+
(only this batch lands between #97 and #99; both merges happen in this session). Final main
16+
state carries the ENHANCED gate. Reviewer's High blocker is satisfied at batch end; disposition
17+
recorded here + in the ledger. Guard against future silent gate removal moves to WP6: add an
18+
assertion to `tests/ci-workflows.test.ts` (owned by #101's branch) that ci.yml contains the
19+
GUI lint + GUI build steps.
20+
21+
## Steps
22+
23+
1. Wait main CI green for the #96 merge (rollback policy).
24+
2. `gh pr ready 97``gh pr merge 97 --merge` (branch is MERGEABLE/CLEAN, no local edits).
25+
26+
## Accept criteria
27+
28+
- c4 (final-state, verified at WP5 close + WP6): ci.yml on main contains GUI lint+build steps;
29+
ci-workflows test asserts their presence (added at WP6).
30+
- #97 MERGED; checks green. Expected repository diff this WP: none (merge-only).
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
# WP4 — Land #98 (shared upstream retry/error refactor) as-is
2+
3+
Reviewer: MERGE-WITH-NITS (behavior-equivalent; nit = adapter-level regression tests, deferred).
4+
No code changes. After #96+#97 merge, effective diff = `6bb3aecf` only.
5+
6+
## Steps
7+
8+
1. If GitHub reports conflicts post-predecessor-merges: merge origin/main into fork branch, push.
9+
2. Local check on branch tip: `bun test tests/upstream-retry.test.ts tests/upstream-http-error.test.ts` + full `bun test`.
10+
3. Checks green → `gh pr ready 98``gh pr merge 98 --merge`.
11+
12+
## Accept criteria
13+
14+
- c5: full suite 0 fail; #98 MERGED.
15+
- Expected repository diff: none beyond merging existing `6bb3aecf` (merge-only WP).
Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
# WP5 — Land #99 with Usage.tsx cancellation guard restored
2+
3+
Base: fork branch `codex/gui-lint-remediation` (tip `e8794d22`). Reviewer blockers: stale
4+
response can overwrite newer state after the refactor dropped the `cancelled` guard.
5+
6+
ALSO at this WP (audit blocker 2): `git merge origin/main` into the branch and resolve the
7+
known `gui/src/pages/Debug.tsx` conflict ONCE — combine #99's lint refactor with #103's
8+
injection-stream wiring (keep both: injection stream entries + lint-clean patterns). This
9+
resolution reaches main with #99's merge; #100#102 then merge cleanly (verified 3-way logic:
10+
their own commits don't touch Debug.tsx).
11+
12+
## Fix — AbortController (MODIFY `gui/src/pages/Usage.tsx`)
13+
14+
Current (branch, ~:196):
15+
16+
```tsx
17+
const fetchUsage = useCallback(async (nextRange: Range) => {
18+
setLoading(true);
19+
try {
20+
const res = await fetch(`${apiBase}/api/usage?range=${nextRange}`);
21+
if (!res.ok) throw new Error("fetch failed");
22+
setData(await res.json() as UsageResponse);
23+
} catch {
24+
setData(null);
25+
} finally {
26+
setLoading(false);
27+
}
28+
}, [apiBase]);
29+
30+
useEffect(() => {
31+
const timeout = window.setTimeout(() => { void fetchUsage(range); }, 0);
32+
return () => window.clearTimeout(timeout);
33+
}, [fetchUsage, range]);
34+
```
35+
36+
Replace with:
37+
38+
```tsx
39+
const fetchUsage = useCallback(async (nextRange: Range, signal: AbortSignal) => {
40+
setLoading(true);
41+
try {
42+
const res = await fetch(`${apiBase}/api/usage?range=${nextRange}`, { signal });
43+
if (!res.ok) throw new Error("fetch failed");
44+
const json = await res.json() as UsageResponse;
45+
if (signal.aborted) return;
46+
setData(json);
47+
} catch {
48+
if (signal.aborted) return; // stale request: a newer effect owns state now
49+
setData(null);
50+
} finally {
51+
if (!signal.aborted) setLoading(false);
52+
}
53+
}, [apiBase]);
54+
55+
useEffect(() => {
56+
const controller = new AbortController();
57+
const timeout = window.setTimeout(() => { void fetchUsage(range, controller.signal); }, 0);
58+
return () => { window.clearTimeout(timeout); controller.abort(); };
59+
}, [fetchUsage, range]);
60+
```
61+
62+
Covers: range/apiBase change mid-flight AND unmount (abort → no setState).
63+
64+
## Verification
65+
66+
- `cd gui && bun run lint && bun run build` (repo's GUI gates; #99 is the lint PR, lint must stay clean).
67+
- No existing GUI unit-test harness expected — verify at build/lint level + reviewer-cited
68+
behavior reasoning; if a gui test runner exists (check gui/package.json at B), add a
69+
hook-level test; otherwise record C evidence as lint+build plus manual reasoning
70+
(activation = abort path exercised by unmount in dev-mode double-effect, noted in commit msg).
71+
- Reviewer's Low finding (hash-sync via effect) — accepted as-is (observable but harmless),
72+
recorded in 000 §Rebutted-adjacent; no change.
73+
74+
## Steps
75+
76+
1. Branch from fork tip; `git merge origin/main`; resolve Debug.tsx (see above); apply Usage.tsx
77+
fix; `cd gui && bun run lint && bun run build`; full `bun test`.
78+
2. Push, checks green, `gh pr ready 99`, `gh pr merge 99 --merge`.
79+
80+
## Accept criteria
81+
82+
- c6: guard present on main (file:line) + gui lint/build output clean; #99 MERGED.
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
# WP6 — Land #100, #101, #102 (test stabilization + isolation + rename retry)
2+
3+
All three reviewer-approved at focus-commit level; blockers were only "stack not yet merged",
4+
which WP2–WP5 resolve. After predecessors merge, each PR's effective diff = its own commit:
5+
`84d601f9` (fixtures), `37b4b2d6` (--isolate), `21e58b09` (Windows rename retry).
6+
7+
## Steps (per PR, in order 100 → 101 → 102)
8+
9+
1. If conflicted after predecessor merges: merge origin/main into fork branch, push.
10+
2. Local: full `bun test` at #102 tip (covers all three).
11+
3. `gh pr ready N` → checks green → `gh pr merge N --merge`.
12+
13+
Nits deferred (recorded): #102 exhaustion-path test, #100 stale focus-commit metadata in PR body.
14+
15+
## Accept criteria
16+
17+
- c7: full suite 0 fail at final tip; #100/#101/#102 MERGED.
18+
- Expected repository diff: none beyond merging existing `84d601f9`/`37b4b2d6`/`21e58b09`
19+
(merge-only WP; Debug.tsx conflicts already resolved on main by WP5 — expect clean merges,
20+
verify mergeStateStatus per PR before merging).
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
# WP7 — Closeout
2+
3+
## Steps
4+
5+
1. `gh pr list --state open` — confirm none of #96#103 open.
6+
2. `gh run list --branch main --limit 3` — latest run green (CI runs on main push).
7+
3. Fast-forward dev: `git push origin origin/main:dev` (dev==main precedent, keeps flow intact).
8+
4. PR comments: each merged PR gets a short disposition comment naming the review fix folded in
9+
(#96 security hardening amendments, #97 gate restore, #99 cancellation guard) so the fork
10+
author sees what changed on their branch. Approved-as-is PRs need no comment beyond merge.
11+
5. Devlog: `_fin` holds decade-numbered feature units (100_*, 110_*...), while dated units
12+
stay in `_plan` (precedent: 260710_codex_warmup_preview_release) — this unit STAYS in
13+
`_plan`; append a DONE section to 000_plan.md with merge SHAs. Commit devlog updates to main.
14+
6. Goalplan: mark criteria met with captured evidence; `cxc loop validate` must pass (E8).
15+
16+
Expected repository diff: devlog unit updates only (000 DONE section); everything else is
17+
PR-state/CI verification (merge-only closeout). Rollback policy: 000 §Rollback.
18+
19+
## Accept criteria
20+
21+
- c8: zero open batch PRs + disposition comments posted.
22+
- c9: latest main CI run success.

0 commit comments

Comments
 (0)