fix(codex): resolve the app-server from the port file when the variable is absent - #591
Merged
Conversation
…le is absent #583 seats a role from the app-server's loaded threads, but the probe was gated on AGMSG_CODEX_BRIDGE_APP_SERVER. Under codex 0.146 --remote an agent's shell_command runs inside the app-server process, and codex-monitor.sh cannot export into that context: the URL does not exist until the banner has been parsed, which is after the server is running. So the fix never ran on the path it was written for. The port file carries the same string. codex-monitor.sh writes the port, builds ws://127.0.0.1:$PORT from it, and hands that to both the variable and --remote — reading the file reconstructs the value byte for byte rather than adding a second way to reach the server. "No URL" still means "could not ask", never "asked and got nothing", so the rollout fallback keeps its fail-closed shape. Also: delivery.sh status called a missing seat unexpected whenever one thread was loaded. A thread seats one role, so when that thread is already seated elsewhere the missing seat is correct. The count is now compared against the seated set, from the list the probe already fetched.
…ames A numeric prefix of a real port is itself a valid port, so no reader-side check can tell a half-written file from a good one. 5296 read while 52962 is being written names a DIFFERENT app-server — on this machine, plausibly another project's, which would answer and let its thread be seated here. That is the exact outcome #583 exists to prevent, so the partial state has to be unobservable rather than filtered: the writer publishes through the existing atomic helper. The reader additionally requires a port a TCP stack could have handed out. Digits alone never established that, and the bound limits what anything other than the writer could leave behind. Also: the unseated count now returns nothing when it cannot be worked out instead of 0, which had asserted "another role holds it" on a failed mktemp. Reported by review of #591.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
#583 seats a codex role from the app-server's loaded threads instead of scanning rollout files. On a real machine it has never run.
Reported with the mechanism attached on #567: the spawned agent does execute the seeding step — the rollout shows the call, the right workdir and exit 0 — and still nothing is recorded.
Why it never ran
The probe was gated on
AGMSG_CODEX_BRIDGE_APP_SERVER:Under codex 0.146
--remote, an agent'sshell_commandruns inside the app-server process, not the TUI client.codex-monitor.shcannot export into that context: the URL does not exist until the server's banner has been parsed, which is after the server is already running.CODEX_THREAD_IDis absent on this path too (0.141+), so with both missing it fell through to the rollout scan, found the ambiguity #579 is about, and failed closed. Exactly as designed — on a path the design could not reach.The reporter confirmed the logic itself is right: supplying
ws://127.0.0.1:<port>by hand seated the correct thread first try.The fix reconstructs the value, it does not add a route
codex-monitor.shis the only writer, and one port produces every form:So the port file and the variable are two carriers of one value.
_agmsg_codex_app_server_urlprefers the variable when present and otherwise rebuilds the identical string from the file. It cannot produce an endpoint the variable would not have named, which is what makes this safe to ship in a release: no new connection target exists.A port file that is missing, empty or not all digits yields nothing, and nothing still means could not ask — never asked and found nothing. The
probe_randistinction #583 was reviewed for is untouched, so an unreachable app-server still cannot let the rollout scan overrule a probe.Second fix in the same report
delivery.sh statustold correctly-unseated roles:A loaded thread seats one role. When the only loaded thread already belongs to another role, this role having no seat is the correct state. The count answered "how many threads are loaded", not "how many are unclaimed", and only the second makes a missing seat odd. The probe now keeps the ids it already fetched, and the branch subtracts the seated set — the same subtraction
codex-record-session.shmakes — instead of re-asking.Evidence
Every new test was checked against a mutation that removes the property it names:
seats from the port file …failsa half-written port file …failsa thread already seated elsewhere …failsSuites:
test_delivery165,test_codex_resume32,test_codex_monitor9,test_codex_bridge37 — 0 failures.Scope held back
A new helper carries the resolution, but only the new caller is wired to it.
_delivery.shand_session-start.shkeep their own resolution in this PR — this is a release blocker, and a behaviour-preserving-looking refactor is how a shared assumption gets split by accident.Left for #579, so the next person does not repeat the survey:
printf '%s' "$X" | agmsg_sha1is open-coded in six or more places.codex-monitor.sh:103and_delivery.sh:102._session-start.sh:135readscodex-app-server.<hash>.sock, and nothing in the tree writes that path — a reader with no writer.*branch of the same statuscasehas the shape just fixed in the1branch: with several loaded threads all already seated, "none identifiable as its session" is true but the suggested remedy (recreate the app-server) is not.loaded − seatedsubtraction now exists twice: incodex-record-session.sh, where it decides whether a seat may be written, and in_agmsg_codex_unseated_count, where it decides what the diagnostic says. Fixing one alone would make the diagnostic and the decision disagree. Not merged here because doing so reaches intocodex-record-session.sh, which is the half of this PR a release is waiting on.