Skip to content

Let fresh Codex starts through the session proxy - #2215

Merged
shiny-code-app[bot] merged 2 commits into
mainfrom
work/cs2213-c1
Oct 18, 2026
Merged

shiny-code-app[bot] merged 2 commits into
mainfrom
work/cs2213-c1

Conversation

@shiny-code-app

Copy link
Copy Markdown
Contributor

Every fresh Codex launch through the Supervisor launcher has failed since #2148 with thread/start failed: native thread identity is unavailable. Existing sessions kept running.

Why

The real Codex 0.163.0 TUI was captured against a throwaway app server. Its thread/start sends no thread ID. It does send dynamicTools, and their JSON Schema inputSchema declares a threadId property. thread_targets() walked into that schema, saw a threadId key whose value was a dict, and refused. The protocol has two more fields of the same kind: turn/start outputSchema and the server elicitation's requestedSchema. Several requests (apps/list, mcpServerStatus/list and others) also allow threadId: null.

What changed

  • thread_targets() skips inputSchema, outputSchema and requestedSchema, and treats a null optional ID as no target. Wrong-typed and empty IDs still refuse. Thread and turn actions with no ID still refuse at the existing request check.
  • When a thread/start reply arrives, the ingress takes the ownership lease and durable action barrier on the returned thread.id before forwarding the reply. If the ID is missing or the lease fails, the client gets an RPC error instead. This matches how resume metadata failures are reported.
  • helpers.md describes the fresh-start rule.

Every ownership guarantee from #2148 stays as it was: resume, fork, queue, approval and admission paths are unchanged.

Verification

  • New test_fresh_start_owns_its_returned_thread_and_malformed_identities_refuse drives a fresh start through the real ingress and the suite's fake Unix app server. The start carries the TUI's dynamic-tool schema shape. The test then checks that the returned thread has its barrier and a free lease, that a turn/start with an outputSchema passes, and that a start reply with no usable ID errors. It also checks that five malformed or missing identities refuse without reaching the server. Without the fix it fails at the start (KeyError: 'result').
  • The real captured TUI thread/start payload yields no targets with the fix; the current main refuses it.
  • Full affected modules pass locally: test_codex_session_proxy (32), test_codex_session (8), test_github_plan_sessions (42), test_gh_plan_claim (253), test_release_session_claims (94).
  • validate-skill-repo, validate-public-safety and validate_role_words pass. check_test_quality shows only the three existing unrelated warnings.
  • Inspection (headless Qodana, changed files): RED, all on existing code: name shadowing in acquire/release, unresolved sys.path imports, ParamSpec warnings, existing doc spelling. None are on the new test or doc lines.

The remaining finish line is one real fresh launch under tmux -L supervisor after this lands.

Refs #2213

The stock TUI's thread/start carries dynamic tool JSON Schemas that describe a threadId property. The proxy read those schema keys as thread identities and refused every fresh launch. Skip schema payloads and null optional IDs, keep refusing malformed or missing identities on thread actions, and take the ownership lease from the thread ID the start returns.

Refs #2213
@shiny-code-app

Copy link
Copy Markdown
Contributor Author

Review prompt for PR #2215 at 1cb0ba027e7b509ccad6f18ee54922caff9100b8. Author: Claude (anthropic). Review of record: provider openai. Supervisor runs the review (codex-skills#2069), from its terminal session against worktree /Volumes/Developer-Artifacts/worktrees/codex-skills/cs2213-c1.

Review the committed change on branch work/cs2213-c1 against origin/main in this repository. Read the diff file the helper names, then whatever files you need. Your access is read-only; read whatever files you need, and do not modify anything.

Purpose: since PR #2148, the per-client Codex RPC ingress in skills/supervisor/scripts/codex_session_proxy.py refused every fresh Codex TUI launch. The stock TUI's thread/start sends dynamicTools whose JSON Schema inputSchema declares a threadId property, and thread_targets() treated that schema key as a thread identity. The change skips JSON Schema payloads (inputSchema, outputSchema, requestedSchema) and null optional thread IDs. It also takes the ownership lease and durable action barrier on the thread ID that a thread/start reply returns, before forwarding that reply.

Required invariants: every ownership guarantee added by #2148 must still hold. That covers per-action leases until acknowledgement, approvals held until answered, resume/fork exact identity, frozen predecessors refused, and admission/closure sharing the lease. Requests that name a thread with a malformed identity must still refuse. Thread and turn actions with no identity must still refuse.

Relevant files: skills/supervisor/scripts/codex_session_proxy.py, skills/supervisor/scripts/test_codex_session_proxy.py, skills/github/scripts/github_plan_closure.py, skills/github/scripts/test_github_plan_sessions.py (ReceiptFixture fake app server), skills/supervisor/references/helpers.md, and the Codex app-server protocol (ThreadStartParams, TurnStartParams, McpServerElicitationRequestParams) if you know it.

Look for: a way the new skips let a request act on a thread without its lease, for example a real identity hidden under a schema key or a null used to dodge a lease. Look for a thread/start reply path that leaks the lease, double-releases, or forwards a reply without ownership. Look for regressions in other reply handling such as resume metadata, approvals, or out-of-order replies, and for tests that would pass with the bug present.

For each finding give the file and line, a realistic trigger, and a severity (blocking, medium, low, informational). "none" is an acceptable answer.

@shiny-code-app

Copy link
Copy Markdown
Contributor Author

Review of record: OpenAI, run by the Supervisor from its iTerm session (route on codex-skills#2069), at head 1cb0ba027, with the prompt posted above. Metadata at /Volumes/Developer-Artifacts/task-evidence/codex-skills/supervisor-reviews-2026-10-17/r-codex-skills-2215.json. The authoring session, or the next session on this PR, weighs it before ready.


Medium — skills/supervisor/scripts/codex_session_proxy.py:229: Replacing guarded leaks any leases acquired from the thread/start request parameters. For example, a start request with an MCP server environment override containing threadId: "existing-thread" acquires that thread's lease through the recursive scanner. When start succeeds with a new thread ID, this assignment discards the original guarded set; the finally block releases only the new thread's lease. Other clients' actions, closure, and admission for existing-thread remain blocked until this client disconnects. Preserve and release both acquisitions, including their reference counts when IDs overlap. The added test starts with no request-side targets, so it misses this case.

A thread/start whose params name an existing thread acquired that lease, and taking the new thread's lease replaced the guarded set, so the request's lease was never released. Hold the start lease as its own references and release both.

Refs #2213
@shiny-code-app

Copy link
Copy Markdown
Contributor Author

Review result: weighed
Head: 7a3c0e0

Reviewer: OpenAI, review of record, run by the Supervisor at 1cb0ba027. One finding, medium.

Acted on (medium, reproduced): a thread/start whose params name an existing thread acquired that thread's lease, and taking the new thread's lease replaced the guarded set, so the request's lease was never released. Fixed in 7a3c0e0c: the start lease is held as its own references and both sets are released. When the IDs overlap, both reference counts are released. The test now sends starts whose config names an existing thread, and the same thread the start returns. It checks that both leases are free afterward. On 1cb0ba02 it fails with the lease still held (BlockingIOError); with the fix it passes.

The same commit also simplifies the pending-request unpack (same behavior). This clears the inspection warning on that line.

Declined: none. Unresolved: none.

Since the review, the only change is that commit. The 429 tests in the five affected modules pass again. Inspection of the changed files is RED only for the existing ParamSpec warnings on asyncio.to_thread and other existing findings, none on changed lines.

@shiny-code-app

Copy link
Copy Markdown
Contributor Author

Review prompt for PR #2215 at new head 7a3c0e0c73c8c0990455e8544e1be97db88eea20. Author: Claude (anthropic). Review of record: provider openai.
Supervisor runs the review (codex-skills#2069), from its terminal session against worktree /Volumes/Developer-Artifacts/worktrees/codex-skills/cs2213-c1.

Review the committed change on branch work/cs2213-c1 against origin/main in this repository, focusing on commit 7a3c0e0c (the fix since the last review at 1cb0ba02). Read the diff file the helper names, then whatever files you need. Your access is read-only; read whatever files you need, and do not modify anything.

Background: the earlier review found that, in skills/supervisor/scripts/codex_session_proxy.py, taking the lease on the thread ID a thread/start reply returns replaced the request's guarded set, so leases acquired from the thread/start params were never released. Commit 7a3c0e0c holds the start lease as separate references (owned), sets the action barrier on both sets, and releases both in the finally block. It also rewrites the pending-request unpack without changing behavior.

Check that every acquire on this path has exactly one matching release, including when the request params and the reply name the same thread (reference counts), when the reply is an error, when the lease cannot be created, and when client.send raises. Check that no lease is released before the client receives the reply. Check that the unpack rewrite keeps approvals and resume metadata handling unchanged. Check that the extended test in skills/supervisor/scripts/test_codex_session_proxy.py (test_fresh_start_owns_its_returned_thread_and_malformed_identities_refuse) would fail if either release were missing. Every ownership guarantee from #2148 must still hold.

For each finding give the file and line, a realistic trigger, and a severity (blocking, medium, low, informational). "none" is an acceptable answer.

@shiny-code-app

Copy link
Copy Markdown
Contributor Author

Review of record: OpenAI, run by the Supervisor from its iTerm session (route on codex-skills#2069), at head 7a3c0e0c7, with the prompt posted above. Metadata at /Volumes/Developer-Artifacts/task-evidence/codex-skills/supervisor-reviews-2026-10-17/r-codex-skills-2215.json. The authoring session, or the next session on this PR, weighs it before ready.


none

@shiny-code-app

Copy link
Copy Markdown
Contributor Author

Review result: weighed
Head: 7a3c0e0

Reviewer: OpenAI, review of record, run by the Supervisor at 7a3c0e0c7 with the new-head prompt. Findings: none.

The earlier medium finding (start leases replacing request leases) was acted on in 7a3c0e0c and is weighed. Declined: none. Unresolved: none.

@shiny-code-app

Copy link
Copy Markdown
Contributor Author

Ready for the merge train

Head: 7a3c0e0

@shiny-code-app

shiny-code-app Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor Author

Launchplane completed the merge-train step for this pull request.

Launchplane finished the merge-train step for this pull request.

Launchplane manages this comment and will update it as the train moves.

shiny-code-app Bot added a commit that referenced this pull request Oct 18, 2026
@shiny-code-app
shiny-code-app Bot merged commit 84572d5 into main Oct 18, 2026
8 checks passed
@shiny-code-app
shiny-code-app Bot deleted the work/cs2213-c1 branch October 18, 2026 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants