Repository navigation
Honour codex --cd and keep the provider across a registry round-trip - #75
Merged
Merged
Conversation
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.
Two defects found by reviewing the eight already-merged provider PRs (#67–#74) as unreviewed code. Both are silent: neither produces an error, a wrong-looking row, or a failing test.
1.
codex -C/--cd <DIR>sessions were never discoveredscan_codex_sessionscorrelates rollouts to processes by cwd, reading the process side from/proc/<pid>/cwd.But
codex -C/--cd <DIR>moves the agent's working root, not the process's cwd (codex --help, codex-cli 0.148.0). So the rollout records<DIR>while/procreports whatever directory the shell was in, the two never match, and the session simply never appears.The failure mode is an absent row rather than a wrong one, which is the hardest kind to notice — the dashboard looks fine, it is just missing sessions.
Fixed by preferring the flag when present:
working_root_overrideparses the command line agentctl already captures inLiveClaudeProc::args, accepting all three spellings clap accepts (-C dir,-Cdir,--cd=dir,--cd dir).Writing the test caught a bug in the parser and in my own expectation: I asserted
-C=xshould yieldNone, and the parser returnedSome("=x"). clap accepts-C=valuefor a short flag, so the right answer isSome("x")— both are corrected.2. A registry round-trip silently converted Codex to Claude
SessionEntryhad no provider, andAgentSession::from_registry_entrybuilds viafrom_raw, which stampsAgentProvider::Claudeunconditionally. Any Codex session reaching the registry would come back as Claude — and be resumed withclaude --resume <ULID>: wrong binary, wrong id format.Not reachable today, and I checked rather than assumed:
try_live_sessions— the registry mirror source — reads the Claude pointer directory directly and never callsscan_sessions, so Codex rows never enter the registry. It is a trap armed for whoever wires them in.SessionEntry.provideris#[serde(default)], so every registry file already on disk keeps parsing and keeps meaning Claude. The file is shared between the laptop and every sandbox, so an older binary must still read a newer one — serde ignores unknown fields by default, which it does. Asserted directly: a legacy JSON entry with noproviderkey parses as Claude.Every
SessionEntryconstruction site was audited, not just the ones the compiler flagged. The only production site the compiler did not flag spreads..entry.clone(), which preserves the field; the hook writer now sets Claude explicitly, correct because only Claude Code fires our hooks.Coverage, stated honestly
a_registry_entry_keeps_its_provider_across_a_round_tripfails against the unfixed code.the_working_root_override_is_read_from_the_command_linecovers the parser but not its wiring intoscan_codex_sessions— reverting that call site leaves the test green. That path needs a livecodexprocess and a realps, so it is not unit-testable here. This is the same gap class that #74's mutation test exposed in the terminal layer, and I would rather name it than imply coverage I do not have.Process note
These eight PRs went out without the adversarial multi-agent review the repo's own conventions require. It ran on #66 and caught three defects invisible to tests, clippy and fmt; skipping it for #67–#74 is why both of the above shipped. The multi-agent pass is running now against the same cumulative diff; anything it surfaces beyond these two lands as a follow-up.
Verification
cargo fmt --checkandcargo clippy --all-targets -- -D warningsclean. 754 lib tests plus 7 discovery tests.perf_do_refresh_io_…andrefresh_nonblocking_kicks_worker_under_runtimefail on a loaded box and pass in isolation — wall-clock assertions, untouched here.