Conversation
The stderr pump read every line from the engine and logged a fixed string
instead of it. Measured on the laboratory machine: one soak run produced one
hundred and four records, every one of them the identical placeholder, with the
capture's own record showing an empty allowlist. At the one moment the engine
explains itself, nothing it said was kept.
Owner, this morning, on whether to keep it and whether to redact:
конечно сохранять, ничего не чистить. секреты хранятся на том компе, их
нужно чистить только если происходит вынос с компа, а не внутри работы
программы
So redaction belongs to export off the machine, not to running on it, and
nothing here scrubs anything.
Two properties that were already there survive the change and are now tested.
The forwarding bound still holds, so an over-long line is drained and reported
rather than forwarded whole; keeping the content must not become keeping an
unbounded amount of it. And the decode replaces undecodable bytes instead of
raising, because a pump that dies on one bad byte loses every line after it.
The four tests were confirmed by putting the placeholder back, which fails all
four.
This also unblocks the next piece. The owner asked that a faulted engine be
replaced and the reason recorded in the log; the reason does not exist until the
engine's own words are kept, so this lands first.
The new test module imported pytest and never used it; none of its four tests needs pytest.raises, because each one asserts what reached the log rather than that something was refused. The linter caught it before the push.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9789569b45
ℹ️ 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".
| said = b"connect failed: token=abcdef123456 host=10.0.0.4\n" | ||
| messages = _pump(said, tmp_path, monkeypatch) |
There was a problem hiding this comment.
When the engine logs an actual Telegram-shaped token such as 7701234567:<35 chars>, engine.py first calls setup_logging(), whose _TokenRedactFilter rewrites the value to bot*** on stderr before the launcher pump receives it. This sample is both injected directly into _pump_engine_stderr and unlike the token syntax that the production filter recognizes, so the test passes while the new “nothing is scrubbed” contract remains false for real engine log records. Drive the assertion through the engine's production logging setup with a matching token and move redaction to the export boundary as required.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
| stderr_logger.error( | ||
| "engine child stderr; phase=runtime: %s", | ||
| raw_line.decode("utf-8", "replace").rstrip("\r\n"), | ||
| ) |
There was a problem hiding this comment.
On a Windows installation whose redirected Python stderr uses a non-UTF-8 code page (for example CP1251), or whenever PYTHONIOENCODING is inherited with such a value, the engine encodes its Russian diagnostics using that encoding while this unconditional UTF-8 decode replaces their bytes with �. _start_engine sets only PYTHONUNBUFFERED and does not establish the child's stderr encoding, so the exact failure explanation this change is intended to retain can remain unreadable on the laboratory target. Set a known UTF-8 child encoding before spawn or decode using the actual child encoding, and cover the subprocess boundary with a non-ASCII diagnostic.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
P1 -- the child was never told to encode in the encoding the pump decodes. The pump decodes UTF-8, but the spawn set only PYTHONUNBUFFERED. On a host whose stream encoding is a single-byte code page, or with PYTHONIOENCODING inherited from the environment, the engine encoded its Russian diagnostics in that encoding and the pump replaced every one of those bytes. The exact explanation this change exists to keep arrived unreadable. The engine child's base environment now comes from one helper, _engine_child_environment, which pins PYTHONIOENCODING to utf-8 beside the existing PYTHONUNBUFFERED, so both ends agree by construction. The helper exists so the test can drive the real thing rather than rebuild a lookalike beside it: the new test spawns a real subprocess with a deliberately hostile inherited encoding, confirms the control genuinely mangles the text, then passes the same environment through production's builder and requires the Russian line to arrive intact. Removing the one pinned line turns that test red and leaves the other six green. P2 -- the "nothing is scrubbed" test proved less than its name claimed. Its payload was a shape the production filter would never have matched, so it passed without touching the question. Worse, the claim itself was too broad. The pump scrubs nothing, and that is now tested with a token in the exact shape production DOES recognise, checked against logging_setup._redact first so the sample cannot be vacuous. But the engine end is a different matter, and it is stated rather than hidden: setup_logging installs _TokenRedactFilter, which replaces a Telegram bot token before the bytes leave the engine. Telegram carries that token in the URL, so a logged URL would put a live credential into a file that leaves this machine in a support bundle. That one narrow replacement is the export boundary doing its job, not an operational scrub, and it stays. A second new test drives the real filter and pins what it does. The module docstring now says which end each test speaks for.
|
@codex review Head is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94ae9de70e
ℹ️ 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".
| fixed = launcher._engine_child_environment(hostile) | ||
| child = subprocess.run([sys.executable, "-c", program], capture_output=True, env=fixed) |
There was a problem hiding this comment.
Fresh evidence in this revision is that the subprocess receives fixed directly from _engine_child_environment, while the separate inspect.getsource assertion only checks that _start_engine contains the helper's name. Both tests therefore remain green if _start_engine later overwrites the encoding, passes another mapping to Popen, or the Windows/frozen invocation ignores it, so the original target-OS failure can recur without the corrective guard failing; exercise the actual _start_engine spawn boundary and inspect the resulting diagnostic instead.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
| args=(f"https://api.telegram.org/{_REAL_TOKEN_SHAPE}/getMe",), | ||
| exc_info=None, | ||
| ) | ||
| assert logging_setup._TokenRedactFilter().filter(record) is True |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new engine-end test manually invokes a newly constructed _TokenRedactFilter rather than calling setup_logging() and emitting through its stderr handler. It therefore stays green if production stops attaching the filter, attaches it only to the file handler, or otherwise writes the token to stderr before the pump, leaving the previous credential-path finding without a production-path regression guard.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
…beside them Closes both review findings on pull request #94. Both were right, and both were the same weakness: a test that reproduced production's behaviour instead of running it stays green while production changes underneath it. The encoding guard now intercepts subprocess.Popen inside the real _start_engine and reads the environment that was actually on its way to the operating system. Before, it built the environment through the helper and spawned its own child, so _start_engine could have overwritten the value, or handed a different mapping to the spawn, and nothing would have failed. The inspect.getsource assertion that stood in for this is gone, because it only checked that a name appeared in the source. The engine-end redaction guard now runs the production setup_logging in a child and reads what actually reached that child's stderr, which is the stream the pump reads. Before, it constructed a _TokenRedactFilter by hand, which proved the class works and nothing about whether production attaches it. If setup_logging ever stopped adding it to the console handler, or added it only to the file handler, a live Telegram token would reach a file that leaves this machine and the hand-built filter would have gone on passing. The same child carries a Russian diagnostic beside the token, so one run shows both halves of the contract: the one credential shape is replaced, and everything else survives byte for byte, encoding included. Each property was falsified separately. Removing the pinned child encoding turns the spawn test and the subprocess test red and leaves the other five green; removing the redaction filter from the console handler turns only the engine-end test red.
|
@codex review Head is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 128e402642
ℹ️ 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".
|
|
||
|
|
||
| def test_the_line_the_engine_wrote_is_what_is_logged(tmp_path, monkeypatch) -> None: | ||
| """The defect itself: the content used to be replaced by a constant.""" |
There was a problem hiding this comment.
Because this module explicitly records a confirmed machine-testable defect and the follow-up false-green review escape, leaving all of its new guards absent from governance/agent_preventions.yaml means neither obligation has a stable prevention ID or a binding to its production-path guard and immutable red/green evidence; ordinary pytest discovery could later be renamed, deselected, or removed without reopening either prevention. Add separate runtime-failure and coverage-escape records before treating this correction as closed.
AGENTS.md reference: AGENTS.md:L361-L369
Useful? React with 👍 / 👎.
Continuous integration caught what my test selection did not:
tests/test_launcher_bridge_handshake.py::test_child_environments_always_strip_launcher_only_descriptor_authority
asserted `source.count("env = _without_soak_bridge_environment(os.environ)") >= 3`.
Putting the engine's child environment behind _engine_child_environment removed
one of those literals. The PROPERTY it guards -- that no child environment is
built from a bare os.environ -- was never broken; the proxy for it was.
The guard now runs the helper instead of describing it. It builds an
environment carrying both soak variables, passes it through
_engine_child_environment, and requires both to be gone and the unrelated one
to survive, exactly as the first half of the test already does for
_without_soak_bridge_environment. The source check stays, but counts BOTH
builders, so moving a spawn behind the helper is allowed and dropping the
stripping is not.
The selection I ran before pushing was tests/launcher, and this test is at the
tests/ root. The selection is now derived rather than guessed: every module
naming a symbol this branch touches -- _without_soak_bridge_environment,
_engine_child_environment, _start_engine, PYTHONIOENCODING, _pump_engine_stderr
-- which is eighteen modules, 495 passed and 17 skipped.
|
@codex review Head is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9829f3f26
ℹ️ 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".
| builders = source.count("env = _without_soak_bridge_environment(") + source.count( | ||
| "env = _engine_child_environment(" | ||
| ) | ||
| assert builders >= 3, builders |
There was a problem hiding this comment.
The count is currently four because it includes _engine_child_environment's internal env = _without_soak_bridge_environment(base) as well as the three production spawn builders. If either the assistant or standalone-GUI spawn is changed back to a bare os.environ copy, the count merely falls to three and this test still passes; the helper assertions also remain green because they never exercise those spawn paths, allowing launcher-only descriptor variables into that child's environment undetected. Inspect each Popen environment or otherwise bind the assertion to every production spawn site rather than using this aggregate threshold.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
…lpers Closes the review finding on pull request #94, which was right: the count was four because it included _engine_child_environment's own internal `_without_soak_bridge_environment(base)`. A spawn regressing to a bare os.environ merely dropped the total to three and still satisfied `>= 3`, and the helper assertions could not see it either, because they never touch a spawn. Two changes, and the weaker one is now only a backstop. Each production spawn is intercepted at subprocess.Popen and the environment on its way to the operating system is read directly -- the engine and the assistant, each with its own literal window state. The launcher-only descriptor variables grant authority over the runner's evidence stream, so a child that inherits them can write into it; the test requires all four to be absent and an unrelated variable to survive, so "strips everything" would fail too. The count now includes only builders that take os.environ, since the helper takes `base`. That makes each counted call a production spawn, and the number is exact rather than a floor. Proven by regressing each spawn in turn: with the engine's builder replaced by `dict(os.environ)` the engine case turns red, with the assistant's replaced the assistant case turns red, and the tightened count catches both as well. Finding the assistant's spawn state took a throwaway probe that added whatever attribute the traceback named until Popen was reached; it needed none. The engine's needed two, which is why they are written down rather than guessed.
|
@codex review Head is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f614dc633
ℹ️ 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".
| [ | ||
| ("engine", launcher.LauncherWindow._start_engine, _engine_window), | ||
| ("assistant", launcher.LauncherWindow._start_assistant, _assistant_window), | ||
| ], |
There was a problem hiding this comment.
Fresh evidence in this revision is that the parameterization intercepts only the engine and assistant, while LauncherWindow._on_open_full_gui is a third production Popen path at src/cryodaq/launcher.py:5151-5167. If that call passes os.environ directly while leaving its builder assignment in place, both these runtime assertions and the exact textual count remain green, yet the GUI child receives the launcher-only evidence-authority variables. Add this spawn to the direct interception rather than relying on the structural backstop.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
Third and last of the launcher spawns. It builds its child environment inline, inherits the same launcher-only descriptor variables if it stops, and was simply missing from the interception. A count could never have said so, which is the whole reason the count is only a backstop now. Regressing that spawn to a bare os.environ reddens exactly its case.
|
@codex review Head is |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…-stderr # Conflicts: # docs/CLAIM_CORRECTIONS.md # docs/architecture-montana-important.svg # docs/current_candidate_metrics.md
|
@codex review Head is |
|
@codex review Head is
Both matter for the same reason: over a week the log is the only witness, and it must keep the last thing said and none of the secrets. Authored by a worker lane, reviewed by a second model, and verified here rather than taken on either one's word. Two corrections were needed and I would rather name them than let the review look cleaner than it was: the lane wrote CRLF into three LF files, which would have shown as whole-file diffs; and my first falsification of the redaction property ran the wrong test module and briefly looked like an unpinned property. Both properties are pinned — restoring the newline condition, and formatting without redaction, each turn exactly one test red.
|
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
# Conflicts: # docs/architecture-montana-important.svg # docs/current_candidate_metrics.md
|
@codex review this pull request at head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38d62e5036
ℹ️ 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".
| guard: tests/launcher/test_engine_stderr_is_kept.py::test_the_engine_end_replaces_a_token_through_its_real_logging_setup | ||
| ci_partition: remaining | ||
| red_evidence: the_no_scrub_test_used_a_shape_production_never_matched_and_the_builder_count_included_the_helpers_own_call_so_a_spawn_regressing_to_a_bare_environ_stayed_green_2026_08_20_pending_immutable_capture |
There was a problem hiding this comment.
This record combines two distinct false-green reproductions, but its sole guard exercises only logging redaction. If the aggregate spawn-builder check again permits a production child to inherit the launcher-only descriptor authority, test_every_production_spawn_strips_the_launcher_only_authority may catch it, yet the registered false-green obligation here never runs that test and remains green. Give the spawn-coverage escape its own prevention ID bound to the production-spawn guard rather than combining it with the token-shape escape.
AGENTS.md reference: AGENTS.md:L366-L369
Useful? React with 👍 / 👎.
…alse-green pair The reviewer objected that a single false-green record carried two different defects: the no-scrub test used a shape production never matched, and the builder count included the helper's own call. One record cannot be closed by two independent guards, and while they shared an entry neither could be shown to have gone red on its own. Record 501 keeps the first defect and stays bound to the real-logging-setup guard. New record 502 carries the second and binds to the spawn-authority guard. Neither claims red-before evidence it does not have; both say pending. Measured at these bytes: the baseline regenerates to itself, so the floors are the generator's own count and not a hand-written number, and the governance suite is 297 passed.
|
@codex review this pull request at head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4aef260eb7
ℹ️ 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".
| - id: LAUNCHER-SPAWN-COVERAGE-FALSE-GREEN-502 | ||
| status: open | ||
| scope: product_contract | ||
| runtime_prevention_id: LAUNCHER-STDERR-CONTENT-001 |
There was a problem hiding this comment.
This false-green record still points to LAUNCHER-STDERR-CONTENT-001, whose classification and consequence cover discarded diagnostics, while the guarded failure lets a child inherit the launcher’s evidence-stream descriptor authority. If this guard regresses, the prevention map therefore reopens the wrong runtime obligation and never tracks the actual authority escape; bind it to LAUNCHER-DESCRIPTOR-EVIDENCE-021 (and add the guard there) or create a matching runtime record with the descriptor-forgery consequence and invariant.
AGENTS.md reference: AGENTS.md:L361-L369
Useful? React with 👍 / 👎.
| ci_partition: remaining | ||
| - node: tests/launcher/test_engine_stderr_is_kept.py::test_the_engine_end_replaces_a_token_through_its_real_logging_setup | ||
| ci_partition: remaining | ||
| - node: tests/launcher/test_engine_stderr_is_kept.py::test_the_engine_end_replaces_a_token_in_formatted_exception_text |
There was a problem hiding this comment.
When formatter-level redaction is removed while the existing message/argument filter remains, the registered normal-record guard at line 583 stays green and only this newly added exception-path guard detects that traceback text leaks the token. The confirmed formatter escape therefore needs its own false_green_pairs entry bound to this exact guard; listing it only under the runtime record does not satisfy the independently enforceable coverage obligation.
AGENTS.md reference: AGENTS.md:L366-L369
Useful? React with 👍 / 👎.
| ci_partition: remaining | ||
| - node: tests/launcher/test_engine_stderr_is_kept.py::test_the_engine_end_replaces_a_token_in_formatted_exception_text | ||
| ci_partition: remaining | ||
| - node: tests/launcher/test_engine_stderr_is_kept.py::test_a_bounded_final_stderr_fragment_is_forwarded_at_eof |
There was a problem hiding this comment.
If the pump again treats every non-newline read as overlong, the existing overlong-line guard remains green while a bounded final diagnostic is silently discarded; only this newly added EOF guard catches that previously escaped failure. Add a distinct false-green prevention ID bound to this node so renaming, deselecting, or weakening it reopens the coverage obligation rather than leaving the runtime record apparently enforced.
AGENTS.md reference: AGENTS.md:L366-L369
Useful? React with 👍 / 👎.
… shows it Review asked for three corrections and all three are the same mistake: a record pointing at a guard that does not demonstrate it. The spawn-coverage pair named the stderr-content runtime record, but the guard it binds -- the production spawn authority test -- belongs to the descriptor-evidence record; the node moves with it. Two further false-greens were folded into the stderr-content record's history rather than registered: an exception's formatted traceback text leaking a token while the message and argument tests stayed green, and a bounded final fragment discarded at end of stream while the over-long-line tests stayed green. Each is now its own record against its own guard. None of them claims red-before evidence it does not have; all say pending. The baseline is regenerated at these bytes, which the lane did not do -- it left the two baseline synchronisation guards red. Governance is 297 passed here.
|
@codex review this pull request at head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 602ef3c2ae
ℹ️ 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".
| - node: tests/launcher/test_engine_stderr_is_kept.py::test_a_russian_diagnostic_survives_the_real_subprocess_boundary | ||
| ci_partition: remaining |
There was a problem hiding this comment.
If _start_engine stops passing _engine_child_environment to Popen, the registered test_a_russian_diagnostic_survives_the_real_subprocess_boundary still stays green because it invokes that helper directly; only the unregistered test_the_spawn_hands_the_operating_system_the_environment_under_test exercises the production handoff. Register that exact test, including the corresponding false-green obligation, so it cannot be renamed or deselected while LAUNCHER-STDERR-CONTENT-001 remains apparently enforced.
AGENTS.md reference: AGENTS.md:L366-L369
Useful? React with 👍 / 👎.
| status: open | ||
| scope: product_contract | ||
| runtime_prevention_id: LAUNCHER-STDERR-CONTENT-001 | ||
| guard: tests/launcher/test_engine_stderr_is_kept.py::test_the_engine_end_replaces_a_token_through_its_real_logging_setup |
There was a problem hiding this comment.
If the pump starts selectively redacting Telegram-shaped text, this registered engine-setup test remains green because setup_logging() already removes the token before the pump receives it. The exact guard for this false-green shape is test_the_pump_adds_and_removes_nothing_even_for_a_real_token_shape, but that node is absent from both this pair and the runtime record, so the pump contract can regress without reopening the prevention; bind the pair to that guard instead.
AGENTS.md reference: AGENTS.md:L387-L390
Useful? React with 👍 / 👎.
| status: open | ||
| scope: product_contract | ||
| runtime_prevention_id: LAUNCHER-DESCRIPTOR-EVIDENCE-021 | ||
| guard: tests/test_launcher_bridge_handshake.py::test_every_production_spawn_strips_the_launcher_only_authority |
There was a problem hiding this comment.
If the soak-capability path passes the launcher's original artifact descriptor or broadens pass_fds, this registered generic spawn guard remains green because its assistant fixture has no _soak_artifact_capability. Only the newly added but unregistered test_assistant_spawn_delegates_only_its_bounded_soak_artifact_grant exercises child_grant() and checks the delegated descriptor boundary; register that node under LAUNCHER-DESCRIPTOR-EVIDENCE-021 with its own false-green pair so this distinct authority escape cannot be deselected without reopening the prevention.
AGENTS.md reference: AGENTS.md:L366-L369
Useful? React with 👍 / 👎.
|
@codex review Head is |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review Exact head: df1eaf7 This test-only update isolates the two Popen observers from ambient data directories and port occupancy. Coordinator verification on this exact tree: C:\Python314\python.exe -m pytest -q --basetemp .pytest-tmp/pr94-verify-20260831 tests/launcher/test_engine_stderr_is_kept.py tests/test_launcher_bridge_handshake.py completed with 29 passed and 19 skipped. Ruff check passed. Ruff format --check reported both files already formatted. The exact new slice and the pull-request metadata passed the privacy search. The pull request remains a draft. Hosted CI and a clean exact-head Codex verdict remain open merge gates. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df1eaf7a55
ℹ️ 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".
| monkeypatch.setattr(launcher.LauncherWindow, "_do_shutdown", lambda _host: False) | ||
|
|
||
| def fail_after_capturing_engine_stderr() -> None: | ||
| launcher._pump_engine_stderr(io.BytesIO(stderr), stderr_logger, tail) |
There was a problem hiding this comment.
_start_engine
When _start_engine reaches _wait_engine_ready and the child exits, production depends on the stderr thread receiving stderr_tail; this guard instead populates the tail manually before invoking _run_construction_step. Removing the tail from the real thread arguments would therefore lose the child’s diagnostic while this registered guard remained green, so drive the failure through the actual engine-start path.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
| raise _Stop("spawn intercepted after the environment was fixed") | ||
|
|
||
| monkeypatch.setattr(launcher.subprocess, "Popen", _capture) | ||
| monkeypatch.setattr("cryodaq.paths.get_data_dir", lambda: tmp_path) |
There was a problem hiding this comment.
When CRYODAQ_STATE_ROOT is unset or points outside the test sandbox, _start_engine still calls _create_engine_stderr_logger() before reaching the intercepted Popen, and get_logs_dir() consequently opens the ambient logs/engine.stderr.log. Patching only get_data_dir leaves this observer dependent on and mutating ambient writable state, so it can fail in a read-only checkout or collide with another launcher observer; redirect the logs directory too, or set the complete state root to tmp_path.
Useful? React with 👍 / 👎.
1 participant
The engine explained itself and the log kept none of it
_pump_engine_stderrread every line the engine wrote and logged a fixed string instead of it. Measured on the laboratory machine, one soak run:Every record for the whole run was
engine child stderr record received; phase=runtime. At the one moment the engine says what is wrong with it, nothing it said survived.The direction
Owner, 2026-08-20, asked directly whether to keep it and whether to redact:
So redaction belongs to export off the machine, not to running on it, and nothing here scrubs anything.
Two properties that were already there, kept and now tested
Keeping the content must not become keeping an unbounded amount of it, and it must not make the pump fragile:
Both were already in the function and both now have a test, because a property nothing exercises is a property that can quietly go.
Evidence
Four tests in a new sibling module, and all four fail when the fixed placeholder is put back:
tests/launcher/is 144 passed. The changed-Python count is re-derived as a set difference in both directions: 706 here, 705 at master, one path entered and none left.Why this lands before the restart change
The owner also asked that a faulted engine be replaced and the reason recorded in the log. That reason does not exist until the engine's own words are kept, so this comes first.
The restart change is deliberately not in this pull request. It touches
LAUNCHER-STARTUP-AUTHORITY, whose registered invariant is that process death must not be reported as successful shutdown — five guards hold it. Restarting after an observed exit and claiming the shutdown settled are two different things, and separating them is its own slice.Written with assistance from Claude (Anthropic).