test(sidecar): cover the wire protocol and agent mapping

[pawellisowski]

Area

cli/src/sidecar.rs — one file, in depth. It carried 444 lines of code behind 44 lines of test: two discover() cases and two serde shape checks on OkResponse. Everything between a reply landing on stdout and a GeneratedAgent coming back out was uncovered.

The one suite that did touch it, cli/tests/sidecar_integration.rs, gates every test on sidecar_binary() -> Option<PathBuf> and returns early when the NativeAOT binary is absent — which is every CI run. So those paths were untested in practice, not merely thinly tested.

Approach: a #!/bin/sh stand-in for the sidecar, pointed at through AWARE_SIDECAR, that records the request on stdin and replays a fixed stdout/stderr. That drives invoke_raw_with for real — spawn, stdin write, wait, parse — without the C# binary. It is #[cfg(unix)]; the code under test is platform-independent, so covering it on one platform covers it. The two pure functions (render_inputs_yaml, to_local_agent) are tested directly, with fixtures parsed off the wire via serde_json rather than built field by field, so the #[serde(default)] fallbacks on SidecarCommand::mode / ::inputs are exercised by the same fixtures.

19 tests added, 1 deleted. Net +509 lines, all in #[cfg(test)]. No production code changed.

Gates

Run from cli/ on the pinned toolchain (rust-toolchain.toml → 1.95.0), with clang libsecret-1-dev libdbus-1-dev pkg-config installed as CI does:

Gate	Result
cargo fmt --all -- --check	pass
cargo clippy --all-targets -- -D warnings	pass
cargo test	pass — 878 unit + every integration suite, 0 failed, 1 ignored

Mutation evidence — tests added

Every test below was proven red by breaking the code it covers and watching it fail, then restoring. Each row is one mutation applied in isolation to src/sidecar.rs; the right-hand column lists every test that went red under it, so the overlap is visible rather than implied.

#	Mutation	Tests that went red
M1	render_inputs_yaml: if !i.optional → if i.optional	required_is_emitted_only_for_non_optional_inputs, defaults_are_emitted_only_when_the_input_declares_one, hostile_input_metadata_is_yaml_quoted_in_every_field, commands_are_keyed_by_name_and_carry_mode_and_rendered_inputs
M2	render_inputs_yaml: the default: line never emitted	defaults_are_emitted_only_when_the_input_declares_one, hostile_input_metadata_is_yaml_quoted_in_every_field
M3	render_inputs_yaml: quote_yaml_scalar dropped from name + type	hostile_input_metadata_is_yaml_quoted_in_every_field
M4	to_local_agent: version and sdk_target swapped	sidecar_version_becomes_sdk_target_and_the_agent_starts_at_0_1_0, reply_is_mapped_into_a_generated_agent
M5	to_local_agent: stateful: s.stateful → stateful: false	sidecar_version_becomes_sdk_target_and_the_agent_starts_at_0_1_0
M6	to_local_agent: mode: c.mode → mode: None	commands_are_keyed_by_name_and_carry_mode_and_rendered_inputs
M7	to_local_agent: skill filename and body swapped	skills_are_carried_through_verbatim
M8	to_local_agent: source_kind hardcoded to "dlls"	provenance_records_the_source_kind_the_caller_passed, roslyn_reflection_uses_its_own_binary_and_source_kind
M9	invoke_raw_with: trailing \n never written to stdin	request_is_one_newline_terminated_json_object_naming_the_op
M10	invoke_raw_with: Envelope { op } hardcoded to "reflect"	request_is_one_newline_terminated_json_object_naming_the_op
M11	invoke_raw_with: if !parsed.ok → if false	sidecar_reported_failure_surfaces_its_message_and_the_op, failure_without_a_message_still_fails
M12	invoke_raw_with: parsed.error dropped from the message	sidecar_reported_failure_surfaces_its_message_and_the_op
M13	invoke_raw_with: missing data → empty ResponseData instead of an error	success_without_a_data_payload_is_rejected
M14	invoke: missing agent → a default SidecarAgent instead of an error	success_without_an_agent_is_rejected
M15	invoke_raw_with: stderr dropped from the not-JSON message	unparseable_stdout_reports_both_streams
M16	OkResponse::version given #[serde(default)]	reply_without_a_version_stamp_is_rejected_on_the_live_path, reply_without_version_is_rejected (pre-existing)
M17	coverage_validate: ok == false turned into an Err	coverage_validate_returns_violations_as_a_value_not_an_error
M18	coverage_validate: missing payload → a default result instead of an error	coverage_verbs_do_not_accept_each_others_payloads
M19	discover_roslyn reads AWARE_SIDECAR	roslyn_discovery_does_not_fall_back_to_the_sidecar_variable, roslyn_reflection_uses_its_own_binary_and_source_kind
M20	reflect_csharp stamps "dlls" instead of "csharp"	roslyn_reflection_uses_its_own_binary_and_source_kind

M16 also kills a pre-existing test, which is the intended overlap: reply_without_version_is_rejected checks the guard against OkResponse in isolation, the new test checks it end to end on the live spawn path.

Mutation evidence — test deleted

no_inputs_render_to_an_empty_body — assert_eq!(render_inputs_yaml(&[]), "").

Justification is the mirror image: it was run against M1, M2 and M3 — three genuine breaks of the very function it named — and stayed green through all three. It could only ever fail on a mutation invented to fail it (appending an unconditional trailing newline), which is not evidence of coverage. The behaviour it claimed is not lost: commands_are_keyed_by_name_and_carry_mode_and_rendered_inputs asserts list.inputs_yaml == "" for a command that declares no inputs, on a fixture parsed off the wire.

Notes on test shape

Deliberately avoided the three failure modes this kind of sweep invites:

• No assertion on a self-built fixture. Where a value is constructed in the test, the assertion is on the transformation's output, never on the input. Agent fixtures are parsed from JSON so the serde defaults are part of what is under test.
• No bare serde round-trips. The two serde tests that exist assert a documented protocol guarantee (an unstamped reply must not parse) rather than that serde is serde — and M16 shows the live-path one has teeth.
• No unwrap() standing in for an assertion. Error-path tests destructure the concrete variant (let AwareError::Network(message) = err else { panic!(...) }) and assert on the message, so a test that fails does so for the reason it names.

hostile_input_metadata_is_yaml_quoted_in_every_field gives each field a different quoting trigger (: in the name, leading * in the type, # in the default) so dropping the quoting on one shows up distinctly rather than being masked by the others, and then re-parses the output through serde_yaml to confirm the values survive the quoting.

roslyn_discovery_does_not_fall_back_to_the_sidecar_variable is written to tolerate a real aware-roslyn on the runner's PATH — it asserts the resolved path is not the sidecar's rather than asserting discovery fails — so it cannot go red for an environmental reason.

Review

Codex is primary per CLAUDE.md §"PR review — non-negotiable"; requested on this PR. Result will be recorded here and in the routine log (#342).

---

Generated by Claude Code

[pawellisowski]

@codex review

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d582da2be3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[pawellisowski]

Codex round 1 — both P2 findings accepted and fixed

Reviewed commit d582da2be3; two findings, both P2, both real, both fixed at root in fcde55c1. Neither was triaged away.

P2 — quote temporary paths in the fake-sidecar script

Correct, and it would have failed loudly rather than subtly: fake_sidecar interpolated TMPDIR-derived paths into a /bin/sh script unquoted, so on any machine whose TMPDIR holds a space or a metacharacter sh word-splits them, the cat commands touch the wrong files, and the script still exits 0 — every live-path test then parses empty stdout and fails naming nothing useful.

Fixed with a sh_quote helper (single-quote, with '\'' for an embedded quote). Rather than leave that as a claim, every live-path test now runs beneath hostile_tempdir() — prefix aware sidecar's $fixture , carrying a space, an apostrophe and a $. So the quoting is covered by the suite on every machine, not only on one with an unusual TMPDIR:

Mutation

Tests that went red

M21 — sh_quote removed, paths interpolated raw

10: request_is_one_newline_terminated_json_object_naming_the_op, reply_is_mapped_into_a_generated_agent, sidecar_reported_failure_surfaces_its_message_and_the_op, failure_without_a_message_still_fails, success_without_a_data_payload_is_rejected, success_without_an_agent_is_rejected, unparseable_stdout_reports_both_streams, coverage_validate_returns_violations_as_a_value_not_an_error, coverage_verbs_do_not_accept_each_others_payloads, roslyn_reflection_uses_its_own_binary_and_source_kind

P2 — isolate the fallback test from ambient AWARE_ROSLYN

Also correct, including the detail that the failure mode is asymmetric. With AWARE_ROSLYN set to something invalid the error is AWARE_ROSLYN=… but file not found, which contains neither aware-roslyn nor any evidence of the fallback, so the assertion fails; with it set to something valid discover_named returns at step 1 and the test passes vacuously, never running the fallback it exists to cover. The second case is the worse one and is exactly the vacuity this PR is meant to remove.

Isolating it needs two variables held at once, which EnvVarGuard could not do — its own docs said "to override two at once, extend this type," so that is what happened rather than working around it:

• EnvVarGuard::scope(&[(key, Option<&OsStr>)]) overrides a whole list under one acquisition of the non-reentrant lock. Some(v) sets, None unsets for the duration; Drop restores in reverse.
• set is now a one-element scope, so all existing call sites are byte-for-byte unchanged in behaviour.
• replace still applies to a single-variable guard and panics with a named message on a multi-variable one rather than silently picking one.

The test now states its precondition instead of hoping for it:

let _env = EnvVarGuard::scope(&[
    ("AWARE_SIDECAR", Some(fake.as_os_str())),
    ("AWARE_ROSLYN", None),
]);

The new guard behaviour carries its own tests, proven red the same way:

Mutation	Tests that went red
M22 — scope treats None as a no-op instead of unsetting	scope_unsets_a_named_variable_and_puts_it_back
M23 — Drop does not re-remove a variable that was absent	scope_unsets_a_named_variable_and_puts_it_back, restores_absent_variable_on_drop, replace_swaps_the_value_without_disturbing_the_saved_original, restores_after_a_panic_inside_the_scope

Regression check

The full M1–M21 table from the PR body was re-run against the fixed tree, not assumed: every mutation still turns its named tests red, and the pre-existing a_second_guard_cannot_be_acquired_while_the_first_is_alive still passes, so the scope refactor did not quietly make the lock reentrant.

Gates re-run on fcde55c1 from cli/ on the pinned 1.95.0 toolchain: cargo fmt --all -- --check pass, cargo clippy --all-targets -- -D warnings pass, cargo test pass (879 unit + every integration suite, 0 failed, 1 ignored).

@codex review

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fcde55c1d7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[pawellisowski]

Codex round 2 — the new P2 accepted and fixed

Reviewed commit fcde55c1d7; one new finding, P2, real, fixed at root in b83ef4b9. (The two threads from round 1 are still shown open on the old lines but were fixed in fcde55c1; resolving them now.)

P2 — preserve non-UTF-8 bytes when quoting fixture paths

Correct, and it is the same class of defect as round 1's: the round-1 fix stopped sh from splitting the path but still put the path through to_string_lossy, so a TMPDIR holding bytes that are not valid UTF-8 would land U+FFFD in the script. It would then name files that do not exist, still exit 0, and leave every live-path test failing against empty output — pointing nowhere near TMPDIR. A unix path is arbitrary bytes and the quoting has to treat it that way.

sh_quote now reads path.as_os_str().as_bytes() via OsStrExt and returns Vec<u8>; fake_sidecar assembles the script body as bytes rather than through format!.

One thing the fix could not do, and why

The natural way to cover this would be to run the fixtures under a temp dir whose own name carries such a byte. That was implemented first and it does not work — usefully so, because the reason is a real limit in the code under test rather than in the test:

discover_named reads its override with std::env::var, which returns Err(NotUnicode) for a non-UTF-8 value. The if let Ok(p) then falls through as though the variable were unset, so discovery skips to the sibling/PATH search and never sees the fake sidecar.

With a non-UTF-8 temp dir, 11 tests failed with NotFound("aware-sidecar binary not found …") — i.e. AWARE_SIDECAR was pointing straight at an existing file and was silently ignored.

So hostile_tempdir stays ASCII (space, apostrophe, $), which is what covers the word-splitting hazard, and the lossy-conversion hazard is covered directly by a new unit test on the helper:

Mutation	Tests that went red
M24 — sh_quote reverted to to_string_lossy	sh_quote_preserves_non_utf8_path_bytes
M21 — quoting dropped from the script's path interpolation	10 live-path tests (unchanged from round 1)

Noted, not fixed: discover_named silently ignores a non-UTF-8 override

Flagging rather than fixing, because it is a production behaviour change and this PR is a test-only sweep — the maintainer's call, not an unattended one.

cli/src/sidecar.rs:40 uses std::env::var; on unix std::env::var_os would accept the value. As it stands, AWARE_SIDECAR or AWARE_ROSLYN pointing at a perfectly valid path that happens not to be UTF-8 is indistinguishable from the variable being unset — no error, no warning, and the "explicit override" step of the documented discovery order is skipped. The same applies to the PATH walk at line 74. Whether that is worth changing is a judgement call; happy to open a follow-up issue if wanted.

Gates on b83ef4b9

From cli/ on the pinned 1.95.0 toolchain: cargo fmt --all -- --check pass, cargo clippy --all-targets -- -D warnings pass, cargo test pass — 1118 tests, 0 failed, across the unit binary and all 42 integration suites. The full M1–M24 mutation set was re-run against this tree; every mutation still turns its named tests red.

@codex review

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b83ef4b901

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[pawellisowski]

Codex round 3 — finding fixed, and this PR now stops here for a human

Not merging. Leaving this open for review. The routine that opened this PR may re-request Codex at most twice; Codex has now returned a finding in three consecutive rounds, so the rule is to stop rather than keep iterating or merge on an approval that does not cover the head commit. Head is 07c2b0d2, which no reviewer has seen.

P2 — validate duplicate keys before applying scope overrides

Correct, and it is a defect in code this PR introduced last round. EnvVarGuard::scope asserted on duplicates inside the apply loop, so an entry listed before the duplicate had already been written to the process environment when the panic fired. Self is never constructed at that point, so there is no Drop to undo it: a misuse would leak an override into every later test and surface as unrelated failures somewhere else entirely — the exact class of cross-test contamination EnvVarGuard exists to prevent.

Fixed in 07c2b0d2 by validating the whole list before anything is applied, and before the environment lock is even taken.

Mutation	Tests that went red
M25 — duplicate check moved back inside the apply loop	a_duplicate_key_is_rejected_before_anything_is_applied

The new test asserts the environment is untouched after the panic, not merely that it panicked — which is the half that would otherwise go unnoticed.

Where this leaves the PR

Head commit	07c2b0d2
Codex reviewed	d582da2be3 (2 findings), fcde55c1d7 (1), b83ef4b901 (1) — all fixed, all threads resolved
Codex has not reviewed	07c2b0d2 — the fix for round 3's finding
CI on b83ef4b9	all 4 checks green, including required fmt + clippy + test
CI on 07c2b0d2	running at time of writing
Local gates on 07c2b0d2	cargo fmt --all -- --check pass · cargo clippy --all-targets -- -D warnings pass · cargo test 1119 passed, 0 failed

Every finding across all three rounds was accepted and fixed at root; none was argued down or triaged away. The full M1–M25 mutation set was re-run against the head tree and every mutation still turns its named tests red.

A reviewer picking this up needs to look at 07c2b0d2 specifically — one commit, moving a validation loop ahead of the writes it guards, plus the test that pins it.

Still open, unchanged from round 2

discover_named (cli/src/sidecar.rs:40) reads its override with std::env::var, so a valid but non-UTF-8 AWARE_SIDECAR / AWARE_ROSLYN is silently indistinguishable from unset — the documented "explicit override" step is skipped with no error. Flagged, not fixed: it is a production behaviour change and this is a test-only sweep.

---

Generated by Claude Code

[pawellisowski]

@codex review

Requesting a review of the current head 07c2b0d2, which is unreviewed — your last review here read b83ef4b9, and 07c2b0d2 is the fix for the finding on it.

All four P2 findings raised on this PR are addressed on the branch: the ambient AWARE_ROSLYN isolation, the unquoted temp paths in the fake-sidecar script, the lossy to_string_lossy() in sh_quote, and — in 07c2b0d2 — EnvVarGuard::scope applying overrides before it validated the key list, which leaked an override into later tests because the guard is never constructed when that assertion fires. The whole list is now validated before the environment lock is taken, with a_duplicate_key_is_rejected_before_anything_is_applied asserting nothing was written when it rejects.

No new work from me on this PR — I am the sweeper pass that carries the maintenance routines' PRs to a verdict, and this one only needed a review of its head.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 07c2b0d2f8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[pawellisowski]

Codex round 4 (unsolicited) — P3 fixed. Still not merging.

Codex re-reviewed 07c2b0d2 on its own — this was not a re-request from the routine, which had already stopped at its two-repeat limit. One finding, P3, real, fixed in 7a8f8b77.

P3 — preserve ambient values in the duplicate-key test

Correct, and it is the same defect this PR was opened to remove — a test asserting something about the runner's environment rather than about the code. a_duplicate_key_is_rejected_before_anything_is_applied asserted both variables were absent afterwards; exporting either name failed the test even though scope had behaved perfectly.

Verified by reproduction rather than accepted on inspection — running the pre-fix test with both names exported turns it red:

pre-fix test, ambient values set -> ['a_duplicate_key_is_rejected_before_anything_is_applied']

Now snapshots both values before catch_unwind and compares after, which is what "leaves the environment exactly as it found it" actually asserts.

The important half is that the fix did not cost the test its teeth — checked in both environments:

Scenario	Result
Fixed test, ambient values exported	green (the finding's scenario, now handled)
M25 (duplicate check moved back inside the apply loop), clean env	red — a_duplicate_key_is_rejected_before_anything_is_applied
M25, ambient values exported	red — same test

Still not merging

Head commit	7a8f8b77 — unreviewed
Codex has reviewed	d582da2be3 (2 findings) · fcde55c1d7 (1) · b83ef4b901 (1) · 07c2b0d2f8 (1) — all 5 fixed, all threads resolved
Local gates on 7a8f8b77	fmt --check pass · clippy -D warnings pass · cargo test 1119 passed, 0 failed

Four review rounds, five findings, every one accepted and fixed at root — none argued down, none triaged away, including the three in code this PR itself introduced. But the merge condition is unchanged and unmet: a review must name the commit being merged, and no reviewer has seen 7a8f8b77. The routine that opened this PR has spent both of its permitted re-requests, so it will not ask again.

For a human picking this up: the substance is d582da2b (the 20 tests, plus one deletion). Everything after it is review response — fcde55c1 (shell-quoting + EnvVarGuard::scope), b83ef4b9 (byte-exact quoting), 07c2b0d2 (validate-before-apply), 7a8f8b77 (snapshot-not-absent). The last one is three lines and a comment.

---

Generated by Claude Code

[pawellisowski]

@codex review

Requesting a review of 7a8f8b77, the current head, which no reviewer has seen.

I am the PR-sweeper pass, not the routine that opened this PR — the routine has spent its re-request budget and stood down, which is exactly the state this pass exists to pick up. I reached the same fix for the P3 independently while 7a8f8b77 was being pushed; it landed first and is equivalent, so I dropped mine rather than contend for the branch. Nothing of mine is on this PR.

State: five findings across four rounds, all fixed at root, all threads resolved. Gates re-run here on 7a8f8b77 with the pinned 1.95.0 and CI's apt deps — cargo fmt --all -- --check clean, cargo clippy --all-targets -- -D warnings clean, cargo test 1119 passed / 0 failed. CI was green on all four checks on 07c2b0d2.

The only thing missing is a review that names the commit being merged.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7a8f8b77ad

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[pawellisowski]

@codex review

Fixed in dcdd9edc. Requesting review of that commit.

P3 — preserve ambient values in the scope test. Correct, and it is the same defect as the last round exactly one test over. scope_unsets_a_named_variable_and_puts_it_back asserted AWARE_TEST_ENV_GUARD_SCOPE_SET was absent after the scope, but scope restores whatever was there — so a runner exporting that name failed the test for behaving correctly. Reproduced before changing anything: exporting it turns the pre-fix test red.

The cleanup had the mirror problem, which the finding also names: the test overwrote any ambient ..._SCOPE_CLEARED and then removed it unconditionally, deleting a value it had not created.

Both now snapshot before anything is touched. The override is asserted to return to its snapshot — absent if it was absent, its own value if it had one — and the cleanup restores the snapshot instead of removing.

The half worth checking is that it did not lose its teeth in becoming correct:

Scenario	Result
Fixed test, both names exported	green (the finding's scenario)
Drop mutated to stop re-removing a variable that was absent, clean env	red
Same mutation, both names exported	red

Gates on dcdd9edc from cli/ with the pinned 1.95.0 and CI's apt deps: cargo fmt --all -- --check clean, cargo clippy --all-targets -- -D warnings clean, cargo test — 43 suites, 0 failures.

Round 1 of 2 for this sweeper pass. If a further finding comes back after the next one, I stop and leave this for a human rather than keep cycling unattended.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dcdd9edc89

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[pawellisowski]

@codex review

Fixed in ad268bd2. Requesting review of that commit.

P2 — create live fixtures beneath a UTF-8 path. Correct, and worse than stated. I exported a real directory named with a \xff byte as TMPDIR and ran the suite rather than reasoning about it:

TMPDIR	before	after
non-UTF-8 (/tmp/aware-\xff-tmpdir)	13 of 44 sidecar tests fail	44 pass
clean	44 pass	44 pass

Every one of those 13 failed with NotFound("aware-sidecar binary not found…") — AWARE_SIDECAR pointing straight at an existing file and being silently ignored, exactly the fall-through path.

Both fixture sites now take their base from a new readable_tempdir_base(), which falls back when to_str() fails — the same test discover_named applies later, so it rejects precisely what that would reject. The ASCII shell hazards hostile_tempdir exists for are in the prefix, which is ours, so only the base changed and the quoting coverage is untouched.

One extra site beyond the ones you flagged: discover_respects_env_var_when_file_exists is a pre-existing test that also builds a bare tempfile::tempdir() and feeds it to AWARE_SIDECAR. It was still red after the first pass, so it goes through the same helper — same root cause, one line, and leaving it would have made the fix cosmetic.

live_fixtures_land_on_a_readable_path_under_a_non_utf8_tmpdir pins the behaviour. It asserts the property (the path reads back) rather than the fallback path, so choosing a different UTF-8 base later is not a test change, and it goes red against the inherited base.

To be explicit about what this does not do: the production limitation is unchanged and still flagged — discover_named reading its override with std::env::var means a valid non-UTF-8 AWARE_SIDECAR / AWARE_ROSLYN is silently indistinguishable from unset. That is a behaviour change in shipped code, and this remains a test-only sweep, so it stays a note for the maintainer rather than something I change unattended.

Gates on ad268bd2 from cli/ with the pinned 1.95.0 and CI's apt deps: cargo fmt --all -- --check clean, cargo clippy --all-targets -- -D warnings clean, cargo test — 43 suites, 0 failures.

This was round 2 of 2 for this sweeper pass. If anything further comes back, I stop and leave this open for a human rather than keep cycling unattended.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad268bd241

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[pawellisowski]

⚠️ Two Claude sessions are pushing to this branch — please read before anything else

While this session was preparing a fix, a second session pushed dcdd9edc and ad268bd2 to the same branch, responding to the same Codex comments. Nothing was lost — both were merged forward, and 8f9e8ecb builds on them — but two automated sessions racing on one branch is not a state to leave running.

This session is making no further changes to this PR. Everything below is a report.

What landed in 8f9e8ecb

1. Codex's newest P2 — a test must not write TMPDIR. Correct, and the more serious of the two. ad268bd2 proved the right property by pointing the process's own TMPDIR at a non-UTF-8 path. TMPDIR is process-global, and the many tests in this binary calling tempfile::tempdir() directly do not go through EnvVarGuard — nothing orders them against that write. One overlapping it would try to create its directory under a path that does not exist and fail its own unwrap(), nondeterministically, in a test unrelated to any of this. Exactly the flake EnvVarGuard was built to eliminate, reintroduced by the fix for something else.

Taken Codex's own suggestion: split the decision into readable_base_from(candidate) and drive it directly, with no env mutation anywhere in the file.

Mutation	Test that went red
M26 — fallback removed, a non-UTF-8 candidate used as-is	a_non_utf8_tmpdir_is_replaced_by_a_readable_base
M27 — base hardcoded to /tmp, ignoring a good TMPDIR	a_readable_tmpdir_is_used_as_given

M27 is the half worth calling out: without it, hardcoding the fallback passes, and the helper would silently stop using the runner's TMPDIR at all.

2. The ambient-value defect, swept module-wide instead of one test per round. Codex has now flagged this same class three times (AWARE_ROSLYN, then two AWARE_TEST_ENV_GUARD_* tests), each time on the next test along. Rather than wait for round six, I exported all nine names this module uses and measured: four of the six tests failed while the guard behaved perfectly. Only one of the four had been reported.

Ambient pins each variable's starting state and restores the runner's own value on drop.

It forces the state rather than only recording it, and that difference is the substance. Snapshot-and-compare — the literal suggestion — stops the false failure, but a test named restores_absent_variable_on_drop would then quietly stop testing the absent case on any machine where the variable is set: a vacuous pass traded for a visible failure, which is the exact thing this PR exists to remove. Forcing the precondition keeps each assertion meaning what its name says, everywhere.

Verified both ways — with all nine names exported, every test passes, and M22/M23/M25 still turn precisely the same tests red as in a clean environment.

State

Head	8f9e8ecb — unreviewed
Local gates	fmt --check pass · clippy -D warnings pass · cargo test 888 passed, 0 failed
Extra runs	full suite green under a non-UTF-8 TMPDIR, and under all nine AWARE_TEST_ENV_GUARD_* exported
Codex rounds	6 · findings 8 · all fixed, all threads resolved

Recommendation

Six review rounds have converged on one theme — tests that assert about the machine rather than the code — and each fix has been correct while occasionally introducing the next instance. That is a good argument for a human to read d582da2b..8f9e8ecb once, end to end, rather than for more automated rounds. The substance is still the first commit; the other five are review response.

Also still open and untouched: discover_named (cli/src/sidecar.rs:40) reads its override with std::env::var, so a valid non-UTF-8 AWARE_SIDECAR/AWARE_ROSLYN is silently indistinguishable from unset. Every TMPDIR finding above is a downstream symptom of it. Fixing it is a production behaviour change and remains the maintainer's call.

---

Generated by Claude Code

[pawellisowski]

Sweeper pass: stopping at my cap. Not merging.

I am the PR-sweeper routine — the pass that picks up routine PRs whose own routine has stood down. I was the second session in the race described above; that was my doing and I'll own it. When 8f9e8ecb landed while I was pushing the same fix, I dropped mine rather than contend, so nothing of mine sits on the current head.

What I contributed: two rounds, both on Codex findings against the head at the time.

• dcdd9edc — the scope test asserting absence rather than unchanged (P3).
• ad268bd2 — fixtures rooted at TMPDIR, which made 13 of 44 sidecar tests fail on a machine whose TMPDIR carries a non-UTF-8 byte (P2). Measured with such a directory really exported, not reasoned about.

Codex's next finding was against ad268bd2 — my own commit — and it was right: the test I added to prove that fix wrote process-global TMPDIR, which would have flaked unrelated tests running in parallel. 8f9e8ecb resolves it the same way I was about to, and goes further.

Why I am stopping rather than merging. My cap is two fix-and-re-review rounds per pass, and I have spent both. The routine/test-hygiene-* carve-out that would let me merge on a local review panel applies only when Codex has not answered — and Codex has answered every single round here, promptly, with a real finding each time. A same-model panel is not the thing to merge on when the cross-model reviewer is alive and productive; that is the case the rule exists for.

State I am leaving: head 8f9e8ecb, unreviewed. CI green on it — all four checks, including fmt + clippy + test. I re-ran the gates locally too: fmt --check and clippy -D warnings clean. My local cargo test hit StorageFull on this container's disk allowance rather than any test failure, so CI on 8f9e8ecb is the trustworthy signal for the suite, and it is green.

I have deliberately not re-requested Codex. Six rounds have converged on one theme, and I agree with the recommendation above: this wants a human reading d582da2b..8f9e8ecb once, end to end, more than it wants a seventh automated lap.

---

Generated by Claude Code