-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix(claude): keep Desktop egress alive and retry picker CA untrust across restarts #6103
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
cb9a48e
docs(devlog): plan release-train-4 picker CA recovery
lidge-jun 7e1ecbd
WIP: serialize picker CA publication and preserve pending untrust
lidge-jun 87bd878
WIP: record picker CA lane handoff
lidge-jun 844168b
fix(claude): keep Desktop egress alive and retry picker CA untrust ac…
lidge-jun 26474d9
fix: defer drift heal without a catalog and expire reused legacy pick…
lidge-jun 187fc28
docs(devlog): escape union pipes in the picker CA plan table
lidge-jun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| # Release train 4: picker CA continuity and Codex drift heal | ||
|
|
||
| An applied Claude Desktop picker profile can retain an egress URL after CA rotation is refused. This unit keeps that URL serving blind CONNECT while the old public certificate awaits verified removal, then allows the existing picker enable flow to retry. It also narrows the Codex drift heal to the injected config surface so a stopped auto-refresh generation cannot finish a stale full catalog sync. The source and acceptance record is this unit; the external 2026-09-27 audit is a lead, not an authority. | ||
|
|
||
| ## Loop contract | ||
|
|
||
| | Field | Decision | | ||
| | --- | --- | | ||
| | Archetype / trigger | Satisfy-spec repair for the release-train-4 R1–R3 blocker and B10 verification request. | | ||
| | Goal | Applied Desktop egress stays reachable without picker TLS arming until predecessor trust is gone; CA publication is serialized; pending removal survives process replacement; B10 drift heal respects generation and bounded write-lock waiting. | | ||
| | Non-goals | No `main`/`preview`, release, version, user keychain, real Desktop library, other lane checkout, provider, GUI, or unrelated client change. | | ||
| | Verifier | Isolated focused Bun tests exercise failed untrust, actual CONNECT through the applied URL, lock contention, distinct processes, journal retry, stopped generation, and drift healing; typecheck, `test:changed`, privacy, structure, docs build, exact-head PR CI, then dev CI. | | ||
| | Stop | All criteria have fresh evidence, required PR-head checks succeed, the authorized PR is merged into latest `dev`, and dev CI is checked. | | ||
| | Memory artifact | This numbered unit, PR Verification, and goalplan/receipt records. Detailed pre-fix security working notes remain in ignored `.tmp/`. | | ||
| | Terminal outcomes | DONE only after the stop condition; unresolved native/CI evidence is reported as a limitation, never inferred from Linux tests. | | ||
| | Escalation | A needed write outside this lane, actual keychain mutation, blocked security review, or an unmergeable required check requires coordinator direction. | | ||
| | Resource scope | Only this dedicated worktree, `gpt-6-sol` read-only reviewers, existing repository tools and GitHub PR/CI; no user-set token or time cap. | | ||
|
|
||
| ## Baseline and disposition | ||
|
|
||
| Base is `origin/dev` `24b2f39b77` (fetched 2026-09-27); branch `codex/t4-picker-ca-release-blocker`. The original #6072 picker CA PR and #6074 drift-heal PR are closed and their carried implementations are already on `dev` through #6073/#6075. There is no open picker-CA PR or issue from the current targeted GitHub search. This unit **reimplements the defective edges** of those landed batches on current `dev`; it does not merge an open PR as-is, cherry-pick, or squash a foreign branch. Existing credit in the earlier carry stays in history; this corrective diff does not carry another author's unmerged work. | ||
|
|
||
| Baseline in an isolated `HOME`, `OPENCODEX_HOME`, and `TMPDIR`: the three existing focused files passed 40/40, `bun run typecheck` passed, `bun run privacy:scan` passed, and `bun run structure:check` passed after `bun install --frozen-lockfile`. The focused tests do not assert the applied egress URL after failed untrust; `tests/claude-integration/claude-picker-runtime.test.ts:500` tests only the main intercept port. `src/claude/intercept/picker-ca.ts:89` conflates lock outcome with a void callback. The B10 tick calls full sync at `src/codex/catalog-auto-refresh.ts:86` and checks its generation only after that await at `:143`. | ||
|
|
||
| ## Design decisions | ||
|
|
||
| | ID | Decision | Reason and rejected alternative | | ||
| | --- | --- | --- | | ||
| | T4-PICKER-CA-R1 | Bind a blind-only CONNECT relay on the applied profile's **actual** `egressProxyUrl` port when predecessor cleanup cannot complete; keep the row and previous selection. | A profile pivot requires ownership-sensitive Desktop writes, can discard retry intent, and may not repair the currently running app's pinned URL. Blind relay preserves network access without picker TLS termination. A foreign port holder cannot be replaced safely; report bind failure and preserve the row. | | ||
| | T4-PICKER-CA-R2 | Serialize read/decide/journal/publish under the existing picker CA SQLite lock; return a tagged lock result and never publish on lock failure. | A void callback returns `undefined` on success today. Publication outside the lock permits competing writers. | | ||
| | T4-PICKER-CA-R3 | Journal one outgoing **public** PEM plus SHA-1/SHA-256 before changing `ca.pem`; clear a prior pending item before a fresh rotation and acknowledge only after verified untrust. | The previous public certificate is otherwise lost across process replacement. Refusing a second rotation while one item is pending keeps a single record sufficient. No signing key is serialized. | | ||
| | T4-B10 | Use the existing config injector directly for drifted root keys, with its synchronous commit guard and 1-second write-lock wait. | Full `syncModelsToCodex` gathers/commits a catalog from a tick snapshot and exposes no generation guard or deadline option. The 1-second contract covers lock waiting, not the entire provider fetch; a direct injector avoids that fetch for a config-only heal. | | ||
|
|
||
| ## Architect consultation | ||
|
|
||
| Read-only architect handle `01a0e33c-036c-7dd2-a83f-dfcce911055c` proposed R1 blind relay, R2 tagged lock/no unlocked write, and R3 public pending journal. Main accepted each decision, chose to keep `ensurePickerCa`'s existing return type and throw on unsafe/deferred publication, and added the separate B10 direct-injection decision from the bounded B10 source audit. The same architect reflected on this executable five-document roadmap: its first response was **MISALIGNED** on cached foreign-owner refusal and non-default catalog path preservation. Main amended [010](010_ca_publication.md), [020](020_desktop_continuity.md), and [030](030_codex_drift_heal.md); the second reflection was **ALIGNED**, with both pending drains and acknowledgement required before picker construction. Independent plan audit follows this consultation. | ||
|
|
||
| Independent A audit first found the new documents invisible to its staged-diff reader; the five files were staged in the dedicated worktree. Its substantive round then found that a later controller enable could bypass startup cleanup and that the existing journal path getter mutates an invalid journal. The phase docs now close the former through default `ensurePickerCa` refusal while pending, and the latter through a bounded read-only journal lookup plus a regular/parsed catalog check. These are plan amendments pending the same architect's reflection and the reviewer's next audit. | ||
|
|
||
| Architect reflection on those amendments found a further crash boundary: a pending record may have been written while its PEM remains published and the owner is still live if the subsequent CA replacement fails. Main amended [010](010_ca_publication.md) and [020](020_desktop_continuity.md) so cleanup defers rather than untrusting that incumbent. The independent audit must recheck this reachable fault path before code begins. | ||
|
|
||
| The next reviewer pass found the planned multiple-pending test unreachable because publication refuses while an item is pending. Main simplified the data structure to one pending item and replaced that row with a sequential retry test; architect reflection and final audit follow. | ||
|
|
||
| The same architect confirmed **ALIGNED** for the single-record change. The independent reviewer checked the staged five-document roadmap again, found no remaining blockers, and ended with **VERDICT: PASS**; `git diff --cached --check` also passed. This closes the docs-first design decision. The next cycle begins with serialized CA publication and its process tests in [010](010_ca_publication.md); source files remain unedited at this checkpoint. | ||
|
|
||
| ## Dependency order and review | ||
|
|
||
| The first PABCD work phase records this roadmap only. [010](010_ca_publication.md) establishes publication and journal contracts; [020](020_desktop_continuity.md) consumes them for Desktop continuity; [030](030_codex_drift_heal.md) repairs the independent B10 path; [040](040_integration.md) synchronizes docs/invariants, runs gates, and delivers one ordinary PR to `dev`. Each phase rechecks its prewritten diff plan against the current tree before editing. All implementation remains one release-blocker PR because the final acceptance depends on the combined picker recovery and B10 check; no native GitHub stack is requested. | ||
|
|
||
| ## Verification and enforcement limits | ||
|
|
||
| The focused commands name their target test files directly. `bun run typecheck` reads `src/**/*.ts` and `tests/**/*.ts` via `tsconfig.json`. `bun run structure:check` reads `structure/`; `bun run privacy:scan` scans the tracked tree. `bun run test:changed` uses the `dev` merge base's parsed import graph and cannot discover source-as-data, subprocess, or journal artifacts, so the focused files remain explicit. Baseline output is summarized above; `test:changed`, docs build, skill-surface check, and CI have not yet run on the repair. The seven concurrent release lanes make a full local suite disproportionately costly; the PR will list exact focused commands/results and leave full coverage to CI. | ||
|
|
||
| The CA and generation guards are code-level controls, checked by isolated regression tests and required hosted CI (E3/E4). They do not prevent an arbitrary same-user process from altering its own files or guarantee an occupied egress port; native keychain denial remains simulated locally. There is no claimed unbypassable layer. A Mac workflow on an ordinary source-only PR is gated by `.github/workflows/ci.yml:680-687`; if skipped, an exact-head `macos-control` dispatch is the available explicit coverage path, and its actual job conclusion must be checked separately. |
42 changes: 42 additions & 0 deletions
42
devlog/_plan/260927_release_train_4/picker-ca/010_ca_publication.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| # 010 — Serialized CA publication and public recovery record | ||
|
|
||
| Depends on: `000_plan.md`. Work phase `wp1`, class C4. Source owner: `src/claude/intercept/picker-ca.ts`; consumer: `src/claude/intercept/runtime.ts` in phase 020. No production keychain, service, or home operation. | ||
|
|
||
| ## File changes (diff-level) | ||
|
|
||
| | Path | Change | Before → after | | ||
| | --- | --- | --- | | ||
| | `src/claude/intercept/picker-ca.ts` | MODIFY | `underPickerCaLock` returns `T \| undefined`, and void callbacks are repeated outside the lock → tagged `{ kind: "acquired", value: T } \| { kind: "unavailable", error }`, with no unlocked publication. Track whether the callback entered: an exception after entry or during release propagates as a partial-operation failure, never as mere lock contention. The cached and fresh paths both decide and publish only under the lock. A live foreign owner blocks a fresh publication **and makes a cached mismatched authority throw**; returning the cached CA after merely deferring publication would let callers arm it against a different on-disk owner. Owner records include a process-start identity where the OS supports it, so a reused PID does not falsely count as the recorded owner; legacy records without it conservatively defer. `processAuthorities` is set only after successful locked publication. | | ||
| | `src/claude/intercept/picker-ca.ts` | MODIFY | A replacement overwrites the last public PEM with no durable predecessor → under the same lock, parse the current public PEM, write one `{ sha1, sha256, certPem }` record to `pending-untrust.json` by private temporary file/rename **before** `ca.pem` changes, then publish `ca.pem` and `ca-owner.json`. Public temp files use unique exclusive names and are cleaned after rename failure. Refuse another replacement while the record exists. Reject malformed, nonregular, or oversized pending data rather than dropping it. Expose a bounded read, a locked check of whether the pending PEM is still the published certificate with a matching **live** owner (including this PID), and exact-entry acknowledgement that rereads PEM and both hashes under the lock. A missing record means no pending item. No `keyPem`, signing key, private path, or request data enters the record. | | ||
| | `src/claude/intercept/picker-ca.ts` | MODIFY | `ensurePickerCa(configDir)` returns a CA to controller/CLI/runtime even when predecessor cleanup is unresolved → keep its `PickerCa` return type, but make its default call refuse any pending record or replacement requiring untrust. Add optional `{ rotation: "startup" }` that permits one fresh replacement **only when no prior pending record exists** and returns the CA with its newly queued predecessor for immediate cleanup in phase 020. Only `startClaudeIntercept` uses the option; controller `enable` and picker-runtime material paths use the default and cannot trust or rearm through pending cleanup. | | ||
| | `tests/claude-integration/claude-picker-ca.test.ts` | MODIFY | Existing cached-authority test at `:142-158` expects an ordinary call to overwrite a different valid certificate → change that case to an explicit refusal with no publication, while keeping the missing-file republish as a separate locked success. Existing process-restart and foreign-owner tests check only the final certificate → test a successful void callback executes exactly once, an actual second process holding the SQLite lock causes zero publication outside it, and competing processes leave `ca.pem` and `ca-owner.json` with matching SHA-256 after serialization. Check the pending record contains only public PEM/fingerprints and is written before replacement; malformed record blocks publication. | | ||
|
|
||
| ## Conditional activation and field chain | ||
|
|
||
| - Normal void callback: spy on `node:fs.renameSync` while calling public `ensurePickerCa` in a fresh Bun process. Exactly one `ca.pem` and one `ca-owner.json` replacement prove the locked publisher ran once; the current code produces two of each (four rename calls) with no lock contention. Avoid exporting a private helper only for the test. | ||
| - Busy lock: hold the actual picker SQLite lock in a child process, call `ensurePickerCa` in another process, assert no publication and an explicit deferred outcome; release then retry. No in-process stub substitutes for this contention check. | ||
| - Live foreign owner: present a controlled foreign PEM plus matching owner record with a live child PID, call the cached `ensurePickerCa` in the first process, and assert refusal. The new fresh-owner guard itself prevents a legitimate second `ensurePickerCa` from creating that state. Also run two real publishers against one state dir; one must defer while its peer remains live, and the final CA/owner pair must match. | ||
| - PID reuse: alter only the recorded start identity while keeping the same live PID and fingerprint; on macOS/Linux the owner check rejects the mismatch. A record from an older build with no start identity remains conservative. | ||
| - Prior certificate: start with a valid published public PEM, rotate, assert the record exists with its SHA-1/SHA-256 before the new `ca.pem` is visible. If the write fails, the old PEM remains published. | ||
| - Interrupted publication: fault-inject failure after recording pending but before replacing `ca.pem`; assert the outgoing public PEM stays published and pending is retained. The fresh-owner guard normally refuses rotation while that published owner is still alive, so a live-owner/pending match is a separate controlled fixture (for example a changed process identity after a crash), not a claimed normal result of this fault injection. Phase 020 must defer keychain removal for that fixture and retry after the owner exits. | ||
| - Bad record: stage malformed JSON and attempt rotation, assert no replacement and no private material. No parsing fallback silently erases recovery data. | ||
| - Sequential predecessors: an unresolved A blocks another publication; after exact A acknowledgement, a later B-to-C rotation may create B as the next single pending item. A mismatched acknowledgement leaves the original record intact. | ||
| - Later controller enable: leave a pending entry after a failed removal, invoke the real controller enable path, and assert no trust add or picker rearm. The default `ensurePickerCa` refusal is the gate, not a changed controller implementation. | ||
|
|
||
| New field chain: the startup-only `ensurePickerCa` path creates the single pending entry; the JSON writer serializes it; the bounded reader validates its fingerprints against the PEM; phase 020 consumes it for `untrustPickerCa`, then the locked acknowledgement removes it. Ordinary `ensurePickerCa` callers consume only the no-pending state. The option is created only by `startClaudeIntercept`, is not serialized, and has no other consumer. The lock helper stays private; the public publication path supplies the test oracle. | ||
|
|
||
| wp1 P stale check: `origin/dev` remains `24b2f39b77` and `picker-ca.ts` is unchanged since the docs-only plan. A scratch probe in an isolated `HOME`/`OPENCODEX_HOME`/`TMPDIR` observed **four** `renameSync` destinations on one uncontended `ensurePickerCa` call (two `ca.pem`, two `ca-owner.json`); the regression will require exactly two. The cached-authority test at `claude-picker-ca.test.ts:142-158` and second-process tests at `:162-215` assume unconditional replacement of a different valid/live certificate; update them to the new refusal/retry contract while preserving the still-safe missing-file republish assertion. | ||
|
|
||
| ## Proof before closing this phase | ||
|
|
||
| Run isolated `bun test tests/claude-integration/claude-picker-ca.test.ts`, `bun run typecheck`, and the relevant source-as-data/process tests explicitly. Inspect the final CA/owner pair and journal bytes, and verify no `ca.key` or signing key appears anywhere under the fixture state root. Record exact commands and outcomes in the phase D note. | ||
|
|
||
| ## Results (2026-09-28) | ||
|
|
||
| Implemented as planned in `picker-ca.ts`, with startup draining in the new `picker-ca-cleanup.ts`. Independent `gpt-6-sol` assessments of the committed WIP found two further gaps that are now fixed: a cached ensure did not restore a missing `ca-owner.json` for this process's own published CA (now rewritten under the lock), and pending acknowledgement did not require a confirmed untrust result (it now takes the `untrustPickerCa` result and returns false unless `ok`; a failed removal leaves the record byte-identical). Temporary public files are created inside their cleanup `try`, so a partial write leaves nothing behind. | ||
|
|
||
| **Owner record absent while the owner is alive (accepted limitation).** The second assessment showed that if `ca-owner.json` is deleted while its process is live, a second process may rotate that CA, and a pending record matching the still-published PEM is then drained. No code path deletes the owner record, and `publishAuthority` writes the PEM before the owner, so a crash in between leaves a dead owner, for which rotation is correct. The state is therefore reachable only when another same-user process edits `<OPENCODEX_HOME>/claude-picker/`, which AGENTS.md already places outside what an in-process check can prevent. The suggested strict refusal ("no verifiable owner means defer") was rejected because every upgrade from the current `main` release starts with a published `ca.pem` and no owner record: startup rotation would be refused permanently, and after an interrupted publication the pending drain would defer forever, leaving the picker off for good. Running two proxies on one `OPENCODEX_HOME` is separately refused by the start lifecycle. | ||
|
|
||
| Evidence: isolated focused runs from the leaves — `claude-picker-ca`, `claude-picker-runtime`, `claude-picker-recovery`, test-layout and file-size ratchet tests: 129 pass / 0 fail; `bun run typecheck` and `bun run privacy:scan` passed; the assessor observed pending-file mode `0600` and refusal of a symlinked record and found no new private-key write path. | ||
|
|
||
| PR review follow-up: a legacy owner row with no start identity is now considered stale on macOS when `/bin/ps` supplies an absolute process start time more than 2 seconds after `ca-owner.json` was written; missing/unparseable time or mtime remains conservative. A new live-PID regression failed before the fix and now proves that old owner files permit startup rotation while newer ones still refuse it. Isolated `bun test tests/claude-integration/*picker*.test.ts` passed 103/103, and `bun run typecheck` passed. | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 3662
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 14574
Record the check execution date.
The results section presents checks as completed, but it does not state when they ran or mark them as pending. The available history does not establish that the review occurred on 2026-09-27, so the
2026-09-28heading cannot be called future-dated from this evidence. Add the actual execution date, or mark the results as pending until the checks run.🤖 Prompt for AI Agents