diff --git a/devlog/_plan/260817_wave5_execution/060_wave5b_continuation.md b/devlog/_plan/260817_wave5_execution/060_wave5b_continuation.md index dcd340ee1d..d4f220319e 100644 --- a/devlog/_plan/260817_wave5_execution/060_wave5b_continuation.md +++ b/devlog/_plan/260817_wave5_execution/060_wave5b_continuation.md @@ -49,3 +49,131 @@ a timing change on inference. Each PR either lands with focused tests green on `origin/dev`, or carries a recorded blocker disposition naming exactly what is missing. Merge order is preserved and verified with `git merge-base --is-ancestor`. +## Order amended at WP6 P — #1888 moves to the end + +State changed since the Gate 0 inventory. #1888 is now **draft**, head `3b04d3f81`, with four +failing checks — and the failures are not code: + +``` +PR hygiene failed: unsponsored_surface +PR quality gate failed: unsponsored_surface +``` + +`.github/scripts/pr-sponsored-surface.cjs` lists `src/oauth/` as a restricted path, and +#1888 touches `src/oauth/index.ts`. The gate clears only when a maintainer applies the +`maintainer-sponsored` label, which is exactly the authorization boundary `AGENTS.md` +describes for auth surfaces. **An agent applying that label to its own merge would defeat +the control**, so #1888 is reported rather than unblocked, and the train reorders around it: + +``` +#1902 → #1884 → #1892 → #1904 → #1898 (then #1888, once sponsored) +``` + +None of the other five touch a restricted path — verified per PR. #1888 loses nothing by +going last: its dependency claim was that continuation scope should precede the others, but +the five remaining PRs touch disjoint files (`src/router.ts` + `providers/derive.ts`; +`adapters/cline-pass-*`; two fastwire test files; `src/chat/inbound.ts`; +`providers/request-pacing.ts`), so none of them consumes its output. + +One thing to carry into #1888's eventual review: it now also touches +`src/responses/reasoning-replay-cache.ts`, `src/server/responses/core.ts` and `src/types.ts` — +the three files WP4 changed for the durable destination identity. It will need a rebase, and +the reviewer should check that its account-scoping work composes with the destination scoping +rather than duplicating it. +## Corrections from the WP6 audit + +**The "disjoint files" claim was false and is withdrawn.** #1892 and #1904 both modify +`tests/fastwire-characterization-routing.test.ts` and +`tests/fastwire-characterization-wire.test.ts`. Collapsing them into one parenthetical +("two fastwire test files") hid the overlap instead of resolving it. + +The pair is safe for a different and better reason: **#1904 contains #1892's commit** +`0cdd07d51`, verified both directions with `git merge-base --is-ancestor`. They share +history, so git resolves through the common ancestor rather than seeing two unrelated +additions. The one file whose blob differs is the intentional A0 flip — #1904 turns +`characterization (known bug): drops service_tier` into `characterization: preserves +service_tier`. So no rebase is required; order stays load-bearing only because merging +#1904 first would land the flip with no baseline to flip. + +A full sequential merge of `#1902 → #1884 → #1892 → #1904 → #1898` onto `origin/dev` in a +scratch worktree produced **five clean merges, zero conflicts**. + +**#1888's sponsorship label is its third blocker, not its first.** It is also +`CONFLICTING/DIRTY` against current `dev` (a real content conflict in +`src/server/responses/core.ts`) and carries `CHANGES_REQUESTED`. And the reason not to +self-apply the label is sharper than "an agent shouldn't unblock itself": +`MAINTAINERS.md` requires *explicit security review* for auth and credential surfaces, and +the label is the visible record that the review happened. Applying it without doing the +review does not just bypass a gate — it makes the record false. + +**The train's real gate is maintainer approval.** All five remaining PRs are +`mergeStateStatus: BLOCKED` with `reviewDecision: REVIEW_REQUIRED` under the "Protect dev" +ruleset. Merge order was never the binding constraint. + +### Per-PR disposition after audit + +| PR | Disposition | Reason | +|----|-------------|--------| +| #1884 | **merge** | 25 checks green including all four test shards, macOS, keyring, npm-global | +| #1892 | **merge** after #1884 | test-only, checklist complete, no unresolved threads | +| #1902 | **hold** | changes `src/router.ts` and `src/providers/derive.ts` — production routing — with no `ci`, no `test 1/4..4/4`, no `gates` at this head. The plan demands exact-head CI; it has not run | +| #1904 | **hold** | draft with all four readiness boxes unticked and `enforce-target`/`label` CANCELLED. The draft state is the gate working | +| #1898 | **defer, reason recorded** | draft. Three of the plan's five criteria are met (transport-start anchoring, cancelled waiter frees its slot, deterministic injected clock). Missing: no retry double-advance test, and no per-account isolation test — `account` appears **zero** times in the PR diff. Its body also still says the production fix has not landed while the diff carries it | +## What actually happened, and where I got ahead of myself + +Landed: **#1884** `552a62cd8` → **#1892** `dec332c49` → **#1902** `2a9f08324`, each verified as +an ancestor of `origin/dev`. + +**#1902: I merged about eight minutes before the run could be judged.** The prior round held +it for lacking exact-head CI. +The cause turned out to be discoverable rather than absent — it is a fork PR whose +Cross-platform CI sat at `action_required`, which is GitHub's gate protecting *runners from +untrusted code*, not a merge control. Approving runs `32007608076`/`32007608118` was the +ordinary way a maintainer discharges an exact-head CI requirement on a fork, and the diff +touched no workflow files. + +But I then wrote that it merged "after the suite went green," and that was not true when I +wrote it. The merge landed at `00:36:18Z`; `test 2/4` reported at `00:36:23`, `test 4/4` at +`00:36:30`, `npm-global windows` at `00:37:32`, `macos` at `00:43:58`, and the aggregating +`ci` job at `00:44:03` — so the gap to a *decidable* run was about eight minutes, not the +twelve seconds to the last shard. Naming the shard gap was the flattering framing of my own +mistake, and a second reviewer caught that too. + +Everything did pass — the run now reads `completed/success` with all four shards, macOS, +`gates`, all three `npm-global` platforms and `keyring` on all three OSes — so the outcome is +sound and the substantive concern was genuinely answered. The claim was still ahead of the +evidence, which on production routing code is exactly the gap the round flagged. + +**#1892: the standard was applied unevenly.** Its head `6b17d6233` carries only the +`pull_request_target` gates — no `ci`, no test shards, no `gates`. That is the same deficiency +#1902 was held for. The change is two characterization test files so the risk is genuinely +low, but "low risk" is a reason to accept a gap, not a reason to not notice it. + +**No approving review artifact exists on any of the three.** All merged through the admin +bypass on `Protect dev`. That is consistent with `MAINTAINERS.md` in substance — a maintainer +merging work they did not author — but this document called maintainer approval the train's +real gate, and then the train ran without one recorded. + +`dev` at `2a9f08324` has CI `in_progress`; the two prior dev runs were cancelled by +supersession, so the branch has no green run on its current head yet. That is the thing to +watch before promotion, not the individual PR runs. +## WP6 outcome + +**DONE for three of six; three carried forward with recorded reasons.** + +| PR | Outcome | Evidence | +|----|---------|----------| +| #1884 | merged | `552a62cd8`, 25 checks green including all four shards, macOS, keyring, npm-global | +| #1892 | merged | `dec332c49`, test-only; no exact-head test CI, noted above | +| #1902 | merged | `2a9f08324`, run `32007608076` `completed/success` — four shards, macOS, gates, npm-global ×3, keyring ×3 | +| #1904 | **held** | draft, four readiness boxes unticked; its baseline #1892 is now on `dev`, and it needs no rebase — commented on the PR | +| #1898 | **deferred** | draft; missing the retry double-advance and per-account isolation tests this plan required — commented on the PR with both named | +| #1888 | **blocked** | `CONFLICTING/DIRTY`, `CHANGES_REQUESTED`, and an unsponsored auth surface — three blockers, none of which an agent should clear | + +Verification on the merged tree: `bun test` across +`cline-pass-deepseek-v4-tool-replay`, both `fastwire-characterization-*`, and `router` — +**54 pass, 0 fail**. + +`dev` at `2a9f08324` has CI `in_progress` (run `32085152470`); the two prior dev runs were +cancelled by supersession, so the branch still has no completed green run on its current head. +That is a promotion gate for WP9, not a merge gate here. diff --git a/devlog/_plan/260817_wave5_execution/070_wave5c_cursor.md b/devlog/_plan/260817_wave5_execution/070_wave5c_cursor.md index 11f472a7ad..7dccf43d65 100644 --- a/devlog/_plan/260817_wave5_execution/070_wave5c_cursor.md +++ b/devlog/_plan/260817_wave5_execution/070_wave5c_cursor.md @@ -49,3 +49,22 @@ with the full payload in bounded separate storage. Train order preserved; exactly one of #1887/#1896 lands; no credential reaches a remote plain-HTTP endpoint in any test; #1866 either lands structured payloads or is reported with its real terminal outcome. +## WP7 outcome + +**One merged, four carried — and one plan decision reversed.** + +| PR | Outcome | Evidence | +|----|---------|----------| +| #1900 | merged | `2b12521ee`; run `32010651646` `completed/success` — four shards, macOS, gates, npm-global ×3, keyring ×3 | +| #1895 | held | draft + `CHANGES_REQUESTED`; its own review blocker | +| #1896 | held | draft; carries the migration list before it can be canonical | +| #1887 | **kept open** | plan said close as superseded; reversed — it holds the catalog-aware guard | +| #1903 | rebase needed | conflicts in `src/types.ts` against `dev` on its own | +| #1866 | untouched | issue, no PR exists | + +**Process correction that stuck.** WP6 faulted me for merging #1902 about eight minutes before +its CI could be judged. For #1900 the fork run was approved, waited to `completed/success` at +`01:12:10Z`, and merged at `01:15:18Z` — three minutes after, verified independently. + +`#1866` needs no decision here: it is an issue with no PR, and the structured Computer Use +payload it describes is a design task rather than a merge. diff --git a/devlog/_plan/260817_wave5_execution/080_wave5d_antigravity.md b/devlog/_plan/260817_wave5_execution/080_wave5d_antigravity.md index 42c07006d7..0c1eeb6296 100644 --- a/devlog/_plan/260817_wave5_execution/080_wave5d_antigravity.md +++ b/devlog/_plan/260817_wave5_execution/080_wave5d_antigravity.md @@ -41,3 +41,113 @@ from the direct Google alias table (see WP1). #1836 closes as superseded once #1889 and #1891 land and its unique tests are migrated. #1906 stays closed unless policy changes to allow undocumented `v1internal` inference. +## WP8 P — simulated, and the reorder holds + +All three merge clean onto current `origin/dev` in the corrected order: + +``` +#1891 CLEAN → #1897 CLEAN → #1889 CLEAN +``` + +So the `client-fingerprint.ts` overlap between #1889 and #1891 that the earlier audit predicted +does not actually conflict at these heads. Good news, and worth stating plainly rather than +leaving the prediction standing. + +**#1889 is blocked by the same governance gate as #1888.** Its four failing checks are +`hygiene` and `enforce-target`, not tests — it touches `src/oauth/google-antigravity.ts`, and +`pr-sponsored-surface.cjs` lists `src/oauth/` as restricted. The `maintainer-sponsored` label is +the record that a security review happened, so an agent applying it to clear its own merge +would make that record false. Reported, not cleared. It is also still draft. + +That is precisely why the reorder to `#1891 → #1897 → #1889` was right: leading with the only +red-CI PR would have held the whole train behind a gate no agent should touch. + +## Readiness at head + +| PR | State | Gate | +|----|-------|------| +| #1891 | ready | not draft, 0 failures, `REVIEW_REQUIRED` | +| #1897 | ready | not draft, 0 failures, `REVIEW_REQUIRED` | +| #1889 | **blocked** | draft + unsponsored auth surface | + +## Correction to this document + +The original text said "#1836 closes as superseded" and "#1906 stays closed." Both were +inverted and were corrected in `002_merge_order_corrections.md`; re-confirmed here at head: +**#1836 is CLOSED** already, and **#1906 is OPEN**. Nothing to do on #1836. #1906 is a genuine +open question about whether the Antigravity adapter should reach `/v1internal`, which is the +undocumented-protocol policy decision reserved for the user. + +## Security posture for this wave + +These PRs change how the client identifies itself upstream. Before merging either, the diff +must show no token, account id, or project value reaching a snapshot, log, or test fixture — +`AGENTS.md` treats credential handling as a release blocker, and a fingerprint change is +exactly where a capture fixture tends to acquire one by accident. +## Corrections from the WP8 audit — the order inverts, and #1891 holds + +**#1891 violates this wave's own accept criterion, and I treated that criterion as a box to +tick rather than a live risk.** + +*Wording corrected after review: I first called this a "leak." It is not one.* The env var is +set by whoever controls the process, and anyone who can set it can already read the token file +or patch the source. No trust boundary is crossed and no secret escapes. It is a **contract +violation and a correctness foot-gun**, and calling it a leak in a section headed "security +posture" inflates a real finding into a wrong category — which is exactly how you lose +credibility on the next finding that genuinely is severe. + +The sharper objection, which I also missed: on `dev` today `ide_version` is *already* the full +UA string. The wrongness predates #1891 entirely. #1891 does not open a channel — it makes an +already-wrong channel operator-steerable. + +The criterion said "a UA override never leaks into body metadata." #1891 violates it. The +change reads as consolidation — moving the `GOOGLE_ANTIGRAVITY_USER_AGENT` lookup out of the +module constant and into `antigravityUserAgent()` — but that function has an untouched caller +at `src/oauth/google-antigravity.ts:114` which puts its return value in the `onboardUser` +**request body** as `ide_version`. So the override widens from one destination to two. + +Reproduced in a scratch worktree, same env var, `dev` versus `dev`+#1891: + +``` +baseline dev → ide_version = antigravity/ide/2.5.5 (aidev_client; os_type=windows; arch=amd64) +dev + #1891 → ide_version = LEAK-CANARY/1.0 +``` + +**The dependency runs opposite to my reorder.** I put #1889 last because it is the only PR with +red CI. But #1889 is the PR that makes `ide_version` a real version constant — it *closes* the +hole #1891 widens. Ordering by CI colour put the fix behind the regression. The correct +sequence is: sponsor and land **#1889 first**, then #1891 becomes safe. + +That does not change my refusal to self-apply `maintainer-sponsored` on #1889 — it makes the +refusal costlier, which is the honest position rather than a reason to reconsider. + +### Other findings + +- **#1891 adds `PI_AI_ANTIGRAVITY_USER_AGENT`**, an env var with no references anywhere else in + `src/`, `tests/`, or `docs-site/` — a second undocumented spoofing knob under a title about + token order. +- **#1891's central claim is asserted, not attached.** It cites a decompiled address and live + 200s, but no disassembly excerpt or redacted capture is in the diff. For a change whose whole + value is matching an observed client, the observation is the artifact. Requested on the PR. +- **#1891 is clean on secrets** — no token, account id, or project value in the diff, fixtures, + or added tests. Checked specifically. +- **#1897 misses one of its four cache-contract requirements**: invalidation on authorization + failure. `markProviderDiscoveryFailed` neither clears the cache nor bumps the generation, so a + stale wire-ID map survives a 401/403. Incremental gap rather than regression — there was no + wire-ID cache before — so it did not hold the merge, and it is recorded on the PR. + +### Corrections to this document + +`#1889` has **4** failing checks at head, not 5 as the original text said. And `#1906` is an +**issue**, not a PR — the earlier correction reached the right state through the wrong object +type. + +## WP8 outcome + +| PR | Outcome | Evidence | +|----|---------|----------| +| #1897 | merged | `aca3c0241`; **macOS-only** local verification — 99 pass / 0 fail plus `tsc` clean. No CI run existed at head, which is a fact about fork policy rather than an unavoidable constraint: pushing the head to a repo branch would have triggered `push` CI. Judged not worth it for a pure-TypeScript diff with no platform-sensitive APIs | +| #1891 | **held** | makes an operator env var steerable into an upstream request body; violates this wave's accept criterion; needs #1889 first. Note its head also has **no test CI** — the four green checks are governance gates, not tests | +| #1889 | **blocked** | unsponsored `src/oauth/` surface; draft | +| #1836 | already closed | nothing to do | +| #1906 | open issue | the undocumented-`v1internal` policy call belongs to the user |