Conversation
cryodaq.gui.theme calls load_theme() at module level, so whatever resolve_theme does happens before the first window exists. It raised RuntimeError when the default pack could not be read -- missing, unparseable, one token short, or holding a value that is not a colour. On the laboratory machine that is a program that will not start, during a run that is hours in and a cryostat that is cold, because of a file that decides nothing but which greys to draw. The compatibility loader had the same two raises and the same consequence. Owner, 2026-08-20: "файл цветов не должен останавливать". There is now always something to draw with. _LAST_RESORT_PACK holds the values of config/themes/warm_stone.yaml, the default pack, copied rather than invented, and a test pins every required token to that file so the two cannot drift apart unnoticed. Its description is deliberately not the file's own: it says the colours came from the program, so the operator is told rather than quietly handed a working window over a broken configuration. The reason is also recorded at CRITICAL. The check is kept; only the stopping is gone. Nothing changes for any configuration that works today. A readable pack still wins over the built-in copy, and a broken CHOICE still falls back to the default FILE rather than to the copy -- both have their own tests, because a fallback that quietly takes over from a pack that is perfectly fine would be its own defect. tests/gui/test_theme_loader.py::test_missing_default_pack_raises required the old behaviour. It is not a registered guard. Rather than delete it, it keeps its starting condition -- an empty themes directory -- and now states the opposite, so the module that once demanded the raise says why it does not. Putting the raise back turns seven of the twelve new tests red. Two observations found while measuring, neither fixed here. The comment in warm_stone.yaml reading "LOCKED -- identical across all bundled themes" is not true: braun, gost and xcode carry a different status set from the other nine. And write_theme_selection still raises, which is correct -- refusing to SAVE a selection that would not load, and saying so, guides the operator instead of storing a choice that fails at the next start.
|
@codex review Head is |
![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: bde2e5e0ce
ℹ️ 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".
| logger.critical( | ||
| "theme: default pack '%s' is unusable (%s); drawing with the built-in copy", | ||
| DEFAULT_THEME, | ||
| exc, | ||
| ) |
There was a problem hiding this comment.
When the default pack is invalid during normal startup, this record is emitted while cryodaq.gui.theme is being imported: gui/app.py imports the theme at module line 27 but does not call setup_logging until line 431, and the launcher likewise imports GUI modules before configuring logging. Consequently no file handler exists yet; under the frozen/pythonw launcher even the fallback stderr output is not operator-visible, and later logging setup cannot replay it. The special __meta_description__ does not compensate because theme.py discards pack metadata and available_themes() excludes the invalid file, so the operator receives neither the promised journal entry nor a visible loud-exception indication. Preserve this diagnostic until logging is configured or explicitly surface the fallback state in the UI.
AGENTS.md reference: AGENTS.md:L530-L532
Useful? React with 👍 / 👎.
| try: | ||
| return validate_theme_pack(DEFAULT_THEME) | ||
| except ThemePackError as exc: |
There was a problem hiding this comment.
When statting the default pack raises rather than returning False—for example, EACCES from an ACL or unsearchable themes directory, or EIO from an unhealthy filesystem—validate_theme_pack() fails at its Path.is_file() call with an OSError, not ThemePackError. This new call therefore bypasses the narrow handler and resolve_theme() still aborts during module import, leaving a specifically advertised unreadable-colours-file case fatal. Normalize filesystem errors from the stat check into ThemePackError or catch them at this fallback boundary, and exercise that production startup shape in the regression.
AGENTS.md reference: AGENTS.md:L540-L542
Useful? React with 👍 / 👎.
Both findings were right, and the first one meant the case this branch advertises was still fatal. P1 -- a filesystem error walked past the handler. `validate_theme_pack` calls `Path.is_file()`, which does not answer False on an access-control entry over the themes directory or an unhealthy filesystem; it RAISES OSError. That is not a ThemePackError, so it went straight past every handler and still ended the program during `import cryodaq.gui.theme`. The stat is normalised into a ThemePackError now, and `_selected_theme_name` gets the same treatment one level up, because a file that only chooses a theme must never decide whether we start. P2 -- the reason was recorded where nothing could hear it. resolve_theme runs while `cryodaq.gui.theme` is being imported, and every entry point imports GUI modules BEFORE configuring logging: gui/app.py imports the theme at line 27 and calls setup_logging at line 431. So the CRITICAL record reached no file handler, and under the frozen pythonw launcher reached nothing at all. "The reason is in the log" was simply false. logging_setup now holds records that happen before there is anywhere to put them. `defer_record` appends, bounded at 64 so a list filled before logging exists cannot become a leak nothing notices, and setup_logging replays them once handlers are built. It is a general mechanism because the situation is general: anything settled at import time is in the same position. The test for it drives the whole production ORDER -- resolve the pack, then configure logging, then read the file off disk -- rather than calling the replay helper directly. Asserting on a direct call would have stayed green if setup_logging stopped calling it, and that is the entire risk. Proven: removing the replay line reddens exactly that test, and putting the raw stat back reddens exactly the filesystem test. Selection derived rather than guessed: every module naming a symbol this branch touches, nineteen of them, 461 passed and 13 skipped.
I set this to 706 from my own count, which used a staged-diff filter rather than the workflow-exact set the guard actually computes. Two ways of counting the same thing is how a pin drifts; the guard is the authority and now the pin agrees with it.
|
@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: 7e3fb3b04d
ℹ️ 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".
| try: | ||
| present = pack_file.is_file() | ||
| except OSError as exc: | ||
| raise ThemePackError(f"theme pack '{name}' could not be examined") from exc |
There was a problem hiding this comment.
When the themes directory returns EACCES or EIO, this handler lets resolve_theme() use the built-in pack, but startup still reaches the same failure afterward: LauncherWindow.__init__ calls _build_settings_menu() (launcher.py:4439), which calls available_themes() (launcher.py:4516), and that function repeats THEMES_DIR.exists() outside any handler at _theme_loader.py:327. The fresh evidence beyond the earlier import-path finding is this post-resolution launcher path, where the same OSError aborts window construction despite the fallback; harden the inventory scan as well.
AGENTS.md reference: AGENTS.md:L540-L542
Useful? React with 👍 / 👎.
| try: | ||
| settings_present = SETTINGS_FILE.exists() | ||
| except OSError: | ||
| # Same class as the pack stat below: an unsearchable directory raises here rather | ||
| # than answering False, and a settings file must never decide whether we start. | ||
| return DEFAULT_THEME |
There was a problem hiding this comment.
When SETTINGS_FILE.exists() raises EACCES or EIO, this new branch silently selects warm_stone; if that pack is readable, _default_pack_or_last_resort() is never entered, so neither a normal nor deferred record identifies why the operator's configured theme was ignored. Defer a warning here just as for the unreadable default pack so this exception remains loud after logging is configured.
AGENTS.md reference: AGENTS.md:L530-L532
Useful? React with 👍 / 👎.
Two review findings on pull request #96, both correct. Surviving the import is not surviving startup. resolve_theme coming back with the built-in pack only gets the program as far as its window; LauncherWindow.__init__ then builds the settings menu, which calls available_themes, and the same OSError aborted window construction one step later. So the colours file still stopped the program, just further along. The inventory scan is guarded now, at both its stat and its listing -- exists() can answer while the listing still refuses, on a directory that is readable but not searchable. An unreadable themes directory means an empty menu, not a dead program. An ignored theme choice said nothing. When the settings file cannot be read but the default pack CAN, resolve_theme returns the default and _default_pack_or_last_resort is never entered, so that branch was the only place that could explain why the operators configured theme was dropped -- and it was silent. It records and defers now, like the unreadable default pack. Each has a test that fails without it: unguarding the scan reddens the menu test only, and removing the deferred record reddens the ignored-choice test only.
|
@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: 08529d435e
ℹ️ 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".
| # below is never entered, so this is the ONLY place that can say why the operator's | ||
| # chosen theme was ignored. Deferred as well, because this runs before logging. | ||
| logger.error("theme: cannot read %s (%s); using %s", SETTINGS_FILE, exc, DEFAULT_THEME) | ||
| defer_record( |
There was a problem hiding this comment.
When SETTINGS_FILE.exists() succeeds but SETTINGS_FILE.open() then raises because of an ACL, I/O error, or permissions race, the exception reaches the handler at lines 201–208, which only calls logger.warning() before logging is configured. If the default pack remains readable, startup succeeds but the operator's configured theme is silently ignored and no reason reaches the launcher log. Fresh evidence beyond the prior settings-file finding is this distinct exists-then-open path: reproducing it leaves _deferred_records empty, while the new test only makes exists() itself raise. Apply the same deferral to failures from the actual settings read.
AGENTS.md reference: AGENTS.md:L530-L532
Useful? React with 👍 / 👎.
| except ThemePackError as exc: | ||
| raise RuntimeError(f"Default theme pack invalid: {exc}") from exc | ||
| if requested != DEFAULT_THEME: | ||
| logger.error( |
There was a problem hiding this comment.
When settings select a non-default pack that is missing, unreadable, or invalid while warm_stone remains readable, resolve_theme() executes this logger.error() during cryodaq.gui.theme import, before setup_logging(). The subsequent default-pack load succeeds, so _default_pack_or_last_resort() never queues any record; under the frozen pythonw launcher the selected theme is therefore discarded without the promised reason reaching a log. This is distinct from replaying a broken default pack: the new damaged-choice test exercises this branch but only asserts the fallback colors. Defer this rejection record as well.
AGENTS.md reference: AGENTS.md:L530-L532
Useful? React with 👍 / 👎.
…all sites Two more review findings on pull request #96, and they are the same finding a third and fourth time: a reason produced during `import cryodaq.gui.theme` reaches no file handler, because every entry point imports GUI modules before it configures logging. I added the deferral to one branch, then another, then another, and missed the settings file that cannot be OPENED, the chosen pack that was REJECTED, and the invalid pack skipped by the inventory. Adding them one at a time is what caused that, so there is one helper now. _say_and_defer records and defers together, every branch uses it, and a test fails if a bare logger call reappears in the module. Zero remain. Two of the missed ones matter for the same reason as the first: when the DEFAULT pack is readable the last-resort branch is never entered, so those branches are the only places able to say the operator's choice was dropped. Four ways of dropping it now have a test that drives the whole production order -- resolve, configure logging, read the file off disk -- and requires the reason to be in it. The open failure also had a narrower hole: OSError from opening a settings file that exists was caught by the same handler as a parse error but said "failed to parse", which is not what happened. It says "failed to read" now.
|
@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: 36906fbfd0
ℹ️ 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".
| logger.log(level, message, *args) | ||
| defer_record(level, message, *[str(arg) for arg in args]) |
There was a problem hiding this comment.
In the normal launcher path, setup_logging() runs at launcher.py:6076 before _build_settings_menu() calls available_themes() at line 4516, so a scan/list/invalid-pack error is already written by logger.log() here but is still queued unconditionally. Any later supported reconfiguration through the documented-idempotent setup_logging() replays that stale diagnostic into the new component log, while repeated inventory scans can queue it multiple times; distinguish whether logging is already configured rather than deferring every _say_and_defer() call.
AGENTS.md reference: AGENTS.md:L530-L532
Useful? React with 👍 / 👎.
| source = pathlib.Path(_theme_loader.__file__).read_text(encoding="utf-8") | ||
| bare = source.count("logger.warning(") + source.count("logger.error(") + source.count("logger.critical(") | ||
| assert bare == 0, f"{bare} reason(s) would be lost before logging exists; use _say_and_defer" | ||
| assert source.count("_say_and_defer(") >= 10 |
There was a problem hiding this comment.
This guard only detects three exact source spellings. Replacing any current _say_and_defer(logging.ERROR, ...) call with logger.log(logging.ERROR, ...) recreates the lost-diagnostic defect while leaving bare == 0; the helper-name count also falls from 11 to the accepted threshold of 10 because it counts the function definition. logger.exception(...), aliases, or harmless whitespace changes bypass it similarly, so use an AST-level check that permits the one logger call inside _say_and_defer and rejects all other reason-emitting logger calls.
AGENTS.md reference: AGENTS.md:L391-L394
Useful? React with 👍 / 👎.
The deferral existed for a real reason: theme resolution produces its records during `import cryodaq.gui.theme`, which every entry point does BEFORE configuring logging, so a plain call reaches no file handler -- and under the frozen pythonw launcher reaches nothing at all. But it queued EVERY record unconditionally. An inventory scan can happen after `setup_logging` has run, and its record was both written immediately and queued for replay, so a later component reconfiguring logging printed the same diagnostic a second time. A duplicated warning in an unattended week-long log is not harmless: it makes one event look like two. `logging_setup` now says whether it has configured this process, and the helper defers only when it has not. The record still reaches the file for the import-time case, which is what the deferral was written for. Authored by a worker lane, reviewed by a second model, and verified here rather than taken on either one's word: LF endings, ruff clean, and the property falsified -- deferring unconditionally again turns `test_a_reason_after_logging_setup_is_not_replayed_by_later_reconfiguration` red. `tests/gui/test_theme_loader.py` and the colours-file regression are green at 68 passed.
|
@codex review Head is The deferral existed for a real reason: theme resolution produces its records during But it queued every record unconditionally. An inventory scan can happen after
Authored by a worker lane, reviewed by a second model, and verified here rather than taken on either one's word: LF endings, ruff clean, and the property falsified — deferring unconditionally again turns
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24a3d06ea8
ℹ️ 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".
| _logging_configured = bool(root.handlers) | ||
| _replay_deferred_records() |
There was a problem hiding this comment.
When setup_logging() is called with console=False, file=False, or file-handler construction fails while the console is disabled, _logging_configured correctly remains false but the unconditional replay still drains _deferred_records through a logger with nowhere durable to write. A later successful configuration therefore cannot replay the import-time theme failure, so keep the queue intact unless at least one handler was installed.
AGENTS.md reference: AGENTS.md:L530-L532
Useful? React with 👍 / 👎.
Logging configuration is reported through the same path whether or not the theme file could be read, so a malformed or unreadable colours file cannot leave the launcher without a log to say what went wrong.
|
@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: 690d3efd4a
ℹ️ 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".
| _logging_configured = bool(root.handlers) | ||
| if _logging_configured: | ||
| _replay_deferred_records() |
There was a problem hiding this comment.
When the log path exists but cannot be opened—for example, it is read-only or component.log is a directory—TimedRotatingFileHandler is still added because delay=True postpones opening the file, so this check marks logging configured and drains the queue even though replay fails and writes nothing durable. Fresh evidence beyond the earlier handler-construction case is that delayed construction succeeds while the first emission fails; under pythonw, the console handler may also have no usable stderr, leaving the fallback reason permanently lost. Retain the records until a usable sink has successfully accepted them.
AGENTS.md reference: AGENTS.md:L530-L532
Useful? React with 👍 / 👎.
| assert "cannot read" in written, written | ||
|
|
||
|
|
||
| def test_every_reason_this_module_gives_survives_to_the_log() -> None: |
There was a problem hiding this comment.
This test explicitly documents that several variants of the same diagnostic-loss mistake were fixed one at a time and adds an AST guard for the class, but a repository-wide search finds no corresponding entry in governance/agent_preventions.yaml. Without that prevention-map record, the guard has no stable prevention ID, default-CI binding, or immutable evidence binding, so it can later be deleted, skipped, or moved out of CI without automatically reopening the confirmed review escape; register the guard and its evidence before closing this correction.
AGENTS.md reference: AGENTS.md:L387-L390
Useful? React with 👍 / 👎.
…c is not replayed into nothing
The file handler was built with `delay=True`. Nothing opens the file until the
first record arrives, so an unwritable path raises nothing while the handler is
being installed. The handler is added, the process concludes that logging
exists, and every held startup diagnostic is replayed into a sink that fails on
all of them -- silently, because logging swallows handler errors by design. The
operator is left with an empty log and no reason for the degraded display.
Opening the file eagerly turns that into the exception the surrounding block
already handles: warn on stderr, carry on with the console. The console handler
gets the same question asked a different way -- under a windowed launcher
`sys.stderr` can be None or closed, and `StreamHandler` accepts that without
complaint -- so it is added only when its stream is present and open.
Both checks are made WITHOUT emitting anything. An earlier version of this
change probed each handler with a synthetic record; measured, that wrote
`logging sink readiness probe` to the console and into the log file, so the
operator would read a line describing something that never happened.
One existing assertion changed, and it is the part to look at hardest.
`test_a_reason_after_logging_setup_is_not_replayed_by_later_reconfiguration`
asserted that the second component's log file does not EXIST. That was the same
sentence as "nothing was replayed into it" only while the handler opened lazily,
which nothing declared as a contract. It now asserts the file is empty, which is
the property the test is named for. The control below shows it is not weaker:
restoring `delay=True` reddens it.
Measured at these bytes, with only the named file changed each time:
control, unmutated 28 passed
delay=True restored 2 failed -- the unopenable-path guard and the
reconfiguration guard above
console check removed 1 failed -- the no-stderr guard
production reverted to master all three guards error; master carries no
deferral mechanism at all
The prevention record carries those transcripts by SHA-256 and names the guard
and production blobs. The governance baseline is regenerated, which the earlier
version of this branch omitted: it left `test_baseline_is_in_sync_with_the_live_registry`
and `test_deleting_a_single_runtime_record_is_rejected_by_id` red. Governance is
297 passed at these bytes.
|
@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: 8a6070e2fa
ℹ️ 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".
| guards: | ||
| - node: tests/gui/test_a_colours_file_cannot_stop_the_program.py::test_a_deferred_reason_is_retained_when_the_log_file_cannot_be_opened | ||
| ci_partition: gui | ||
| - node: tests/gui/test_a_colours_file_cannot_stop_the_program.py::test_a_deferred_reason_is_retained_when_there_is_no_console_stream | ||
| ci_partition: gui | ||
| - node: tests/gui/test_a_colours_file_cannot_stop_the_program.py::test_the_readiness_check_writes_nothing_of_its_own |
There was a problem hiding this comment.
Fresh evidence beyond the earlier missing-record finding is that THEME-DIAGNOSTIC-DEFERRAL-001 now exists but its guards list contains only the three handler-readiness tests; a repo-wide search still finds no registry binding for test_every_reason_this_module_gives_survives_to_the_log. Consequently the AST guard can be deleted, skipped, or moved out of default CI without reopening this prevention, so add that exact node to the prevention map.
AGENTS.md reference: AGENTS.md:L387-L390
Useful? React with 👍 / 👎.
| applies_to: Theme diagnostics emitted before logging setup and their deferred replay through logging_setup.setup_logging. | ||
| classification: diagnostic_loss |
There was a problem hiding this comment.
The new test explicitly records that three deterministic diagnostic-loss variants were missed one at a time by passing coverage, but this map adds only a diagnostic_loss runtime record and no linked entry under false_green_pairs. That leaves the confirmed coverage escape without its independently enforceable ID and exact guard binding; add the required false-green pair rather than treating the runtime correction as both obligations.
AGENTS.md reference: AGENTS.md:L366-L369
Useful? React with 👍 / 👎.
| logger.log(level, message, *args) | ||
| if not logging_setup.logging_is_configured(): | ||
| logging_setup.defer_record(level, message, *[str(arg) for arg in args]) |
There was a problem hiding this comment.
When the file handler opens successfully but later stops accepting writes—for example, the disk fills before the launcher calls available_themes()—and the pythonw process has no console sink, logger.log() fails but _logging_configured remains true, so this branch does not queue the diagnostic. Fresh evidence at this head is that _last_emission_succeeded is consulted only by deferred replay; reproducing a post-setup failing stream leaves _deferred_records empty. Base deferral on whether the current emission reached a handler, not merely whether setup once installed one.
AGENTS.md reference: AGENTS.md:L530-L532
Useful? React with 👍 / 👎.
…re logger Review's findings at the previous head, answered. Some diagnostics in the theme loader still called the logger directly. Before `setup_logging` runs there is nowhere for those to go, which is the whole reason the deferral exists -- so the operator lost exactly the lines explaining why the appearance is degraded. They now take the same path as the rest. The prevention record is bound to the guards that demonstrate it rather than to the pure helper beside them, and the baseline is regenerated at these bytes, which the lane did not do. 45 passed across the two covering suites 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: 493e74e96b
ℹ️ 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".
| [(level, message, args)] = logging_setup._deferred_records | ||
| assert level == logging.WARNING | ||
| assert message == "theme: ignoring invalid pack %s: %s" | ||
| assert args[0].endswith("themes\\broken.yaml") |
There was a problem hiding this comment.
On the required Ubuntu GUI job, args[0] is produced by str(Path(...)) and therefore ends with themes/broken.yaml, so this Windows-only suffix makes the new guard fail even when production behavior is correct. I checked .github/workflows/main.yml, which runs the GUI suite on both Ubuntu and Windows, and tools/ci_candidate_runner.py, whose GUI selection includes this entire test directory; compare the path via Path(args[0]).name or use the platform separator instead.
AGENTS.md reference: AGENTS.md:L305-L310
Useful? React with 👍 / 👎.
|
|
||
| def handleError(self, record: logging.LogRecord) -> None: | ||
| self._emission_failed = True | ||
| super().handleError(record) |
There was a problem hiding this comment.
When stderr becomes unusable after setup, or merely reports closed=False while rejecting writes, a theme diagnostic reaches the console handler and this delegation invokes the standard Handler.handleError; with the default logging.raiseExceptions=True, that method writes its traceback to the same bad stderr and a ValueError escapes. I reproduced this with a stream whose write() raises ValueError: available_themes() aborts before _say_and_defer can retain the record, and because the console handler is installed before the file handler, even a working file sink is not reached. Catch failures from the error-reporting path so a presentation diagnostic cannot stop startup or inventory construction.
AGENTS.md reference: AGENTS.md:L529-L532
Useful? React with 👍 / 👎.
| if not logging_setup.last_emission_reached_handler(): | ||
| logging_setup.defer_record(level, message, *[str(arg) for arg in args]) |
There was a problem hiding this comment.
When an already-configured sink transiently rejects a theme diagnostic, this queues the record, but _replay_deferred_records() is called only from setup_logging(). The launcher configures logging once at startup, and its debug-mode flow requires a restart rather than reconfiguring it, so after a disk-full or temporary stream failure clears, later successful logging leaves the original diagnostic queued until process exit. I reproduced this by restoring the file handler's working stream and emitting further successful records; the log resumed while _deferred_records remained populated. Trigger replay when a later emission proves that a sink has recovered, rather than requiring another setup call.
AGENTS.md reference: AGENTS.md:L529-L532
Useful? React with 👍 / 👎.
|
@codex review Please review the exact current head This candidate closes the current review packet:
Evidence collected locally:
Please check the failure/recovery transition, bounded retention, replay re-entry protection, and test isolation for any remaining correctness or week-long resource risk. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e388bba1f9
ℹ️ 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".
| pass | ||
| if not any(getattr(handler, "_last_emission_succeeded", False) for handler in handlers): | ||
| retained.append((level, message, args)) | ||
| _deferred_records[:0] = retained |
There was a problem hiding this comment.
When a recovering sink rejects an early retained record but accepts a later one in the same batch, the later success resets _had_emission_failure while the queue is still empty; this line then restores the earlier failure after the recovery transition has already been forgotten. Subsequent successful emissions therefore do not call _replay_deferred_records(), leaving that diagnostic stranded until another setup or failure/recovery cycle. Preserve a pending-replay signal whenever retained is nonempty, or requeue each failure before later emissions can clear the failure state.
AGENTS.md reference: AGENTS.md:L529-L532
Useful? React with 👍 / 👎.
| pass | ||
| if not any(getattr(handler, "_last_emission_succeeded", False) for handler in handlers): | ||
| retained.append((level, message, args)) | ||
| _deferred_records[:0] = retained |
There was a problem hiding this comment.
If theme diagnostics are deferred concurrently while a broken sink is replaying, line 192 has temporarily emptied the queue, so defer_record() can append another 64 entries; this unchecked prepend can then produce 128 entries. Repeating failed recovery cycles can add another batch each time, defeating _MAX_DEFERRED_RECORDS and creating the week-long memory risk the bound is intended to prevent. Serialize replay/defer operations or truncate the merged queue to the configured cap.
AGENTS.md reference: AGENTS.md:L540-L542
Useful? React with 👍 / 👎.
| if not logging_setup.last_emission_reached_handler(): | ||
| logging_setup.defer_record(level, message, *[str(arg) for arg in args]) |
There was a problem hiding this comment.
When the launcher is already configured and a background thread logs between this diagnostic's logger.log() return and this check, that thread overwrites the handlers' shared _last_emission_succeeded flags. A successful unrelated record can therefore make a rejected theme diagnostic appear delivered so it is never retained; an unrelated failure can also queue a diagnostic that was already written. Associate delivery state with the specific LogRecord or otherwise serialize the emission and decision.
AGENTS.md reference: AGENTS.md:L529-L532
Useful? React with 👍 / 👎.
|
@codex review Please review exact head 23f0f64. Why this head changed:
Local evidence at this candidate:
Please check the concurrency model, replay re-entry, queue ordering and cap, and whether the per-record probe can still lose or duplicate a diagnostic. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23f0f64480
ℹ️ 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".
| if not any(getattr(handler, "_last_emission_succeeded", False) for handler in handlers): | ||
| retained.append((level, message, args)) |
There was a problem hiding this comment.
During deferred replay, if another thread logs after this replayed record is dispatched but before this check runs, that foreign emission overwrites each handler's shared _last_emission_succeeded; a foreign success can therefore drop a diagnostic rejected by every sink, while a foreign failure can retain and later duplicate a delivered one. Fresh evidence at this head is that the new per-record probe protects _say_and_defer, but the replay loop still decides delivery from handler-wide state; use a probe on each replayed LogRecord as well.
AGENTS.md reference: AGENTS.md:L529-L532
Useful? React with 👍 / 👎.
| with _deferred_records_lock: | ||
| merged = retained + _deferred_records | ||
| _deferred_records[:] = merged[:_MAX_DEFERRED_RECORDS] | ||
| _replay_pending = bool(retained) |
There was a problem hiding this comment.
When a new theme diagnostic fails and is deferred during an active replay, and a replayed record subsequently succeeds, that success clears the handler's failure transition while re-entry is suppressed. The merge retains the new arrival, but this assignment leaves _replay_pending false whenever the original replay batch had no failures, so later successful emissions never retry the queued diagnostic and it remains stranded until logging is reconfigured; derive the pending state from the merged queue, not only retained.
AGENTS.md reference: AGENTS.md:L529-L532
Useful? React with 👍 / 👎.
| if _replay_in_progress: | ||
| return | ||
| _replay_in_progress = True |
There was a problem hiding this comment.
When two logging threads prove recovery through different root handlers at nearly the same time, both can observe _replay_in_progress as false before either assignment executes. Each then starts replay while Handler.handle() still owns a different handler lock, and replay's traversal of the root handlers can make the threads wait on one another indefinitely; this can freeze logging and the launcher when its main thread participates. Use a nonblocking replay-owner lock or another atomic ownership operation instead of this check-then-set boolean.
AGENTS.md reference: AGENTS.md:L529-L532
Useful? React with 👍 / 👎.
|
Please review this exact head. It closes all three findings reported on
The three new deterministic guards all fail against the previous production head and pass with this correction. The relevant logging and theme-diagnostic set passes, as do read-only Ruff lint and format checks. Please challenge deadlock freedom, exact-record delivery, retained-record liveness, and bounded queue behavior. @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: 2559b95bc9
ℹ️ 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".
| assert all(record[1].startswith("held ") for record in logging_setup._deferred_records) | ||
|
|
||
|
|
||
| def test_replay_delivery_uses_the_target_records_probe(): |
There was a problem hiding this comment.
These three new deterministic guards enforce newly confirmed diagnostic-loss variants, but THEME-DIAGNOSTIC-DEFERRAL-001 in governance/agent_preventions.yaml still ends its guard list with the earlier partial-replay and queue-cap tests, and a repo-wide search finds none of the three new node names. Consequently these guards can be deleted, skipped, or removed from the default partition without reopening the prevention; add all three nodes with their CI and immutable red/green evidence bindings.
AGENTS.md reference: AGENTS.md:L387-L390
Useful? React with 👍 / 👎.
|
|
||
| logging_setup._deferred_records.clear() | ||
| logging_setup.defer_record(logging.ERROR, "held behind replay owner") | ||
| assert logging_setup._replay_owner_lock.acquire(blocking=False) |
There was a problem hiding this comment.
This guard manually owns _replay_owner_lock and calls the helper directly, so it never enters _EmissionTrackingHandlerMixin.emit or creates the two threads holding different logging-handler locks that caused the reported production deadlock. It can therefore remain green if the recovery callback stops using the nonblocking path or blocks before reaching it, and against the previous head it fails merely because the new lock attribute does not exist rather than reproducing the deadlock. Add a bounded test that drives simultaneous recovery through two real tracking handlers and proves both logging threads terminate.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
| with _deferred_records_lock: | ||
| merged = retained + _deferred_records | ||
| _deferred_records[:] = merged[:_MAX_DEFERRED_RECORDS] | ||
| _replay_pending = bool(_deferred_records) |
There was a problem hiding this comment.
When every handler rejects a theme diagnostic but a foreign record succeeds after _say_and_defer() checks its probe and before defer_record() appends the target, that foreign success clears each handler's recovery state while the queue is still empty, so no replay starts. This assignment only arms _replay_pending at the end of an already-active replay; the subsequent enqueue leaves it false, and later successful emissions therefore leave the diagnostic stranded until logging is reconfigured or the process exits. Arm pending state as part of the bounded enqueue, or otherwise make recovery and retention atomic.
AGENTS.md reference: AGENTS.md:L530-L532
Useful? React with 👍 / 👎.
| def write(self, message): | ||
| if target in message: | ||
| second_handler_entered.set() | ||
| assert foreign_reached_first_handler.wait(timeout=30) |
There was a problem hiding this comment.
This assertion runs inside SecondStream.write(), but the emission-tracking handler deliberately catches sink exceptions, including the resulting AssertionError. If the foreign thread does not reach the first handler before the timeout, replay simply treats the target as rejected, the foreign thread can finish afterward, and the final retained-record assertion still passes without exercising the race that distinguishes this fix from the previous handler-wide flags. Record the wait result and assert it from the test thread after the joins so a broken synchronization path cannot false-green this guard.
AGENTS.md reference: AGENTS.md:L363-L369
Useful? React with 👍 / 👎.
The point of this pull request is that a colours file must never be the reason the program will not start. Review found the guards for it were weaker than they looked, in four ways. All three replay guards are now registered in the partition that actually runs them, and the enforced removal baseline was regenerated, so deleting one is detected. The contention test now runs through two real tracking handlers holding different handler locks and asserts both bounded threads terminate. Blocking the owner turns it red in ten seconds; before, it could pass without contending at all. Bounded enqueue arms its pending flag under the queue lock. The new race guard failed before that change and passes after it. Synchronisation results are recorded inside the handlers and asserted after the joins, from the test thread, so a handler-wide delivery mutation turns the ordering assertions red rather than being invisible. Written with AI assistance.
|
@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: 319c5ec438
ℹ️ 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".
| _replay_pending = bool(_deferred_records) | ||
| finally: | ||
| _replay_in_progress = False |
There was a problem hiding this comment.
When a retained record is merged back at line 268 and another thread successfully emits before _replay_in_progress is cleared, the handler sees both the pending queue and a proven recovery but skips replay because the flag is still true. That successful emission also clears the handler's failure edge; if it is the last record before shutdown, the diagnostic remains queued and never reaches the operator. Fresh evidence at this head is the teardown window after _replay_pending has been armed but before replay ownership is released, which is distinct from records enqueued during the replay itself; arrange a follow-up replay when recovery is observed in this window.
AGENTS.md reference: AGENTS.md:L529-L532
Useful? React with 👍 / 👎.
|
Harvest checkpoint at exact head This preserves a delegated-lane correction for a teardown race in deferred log replay. I reverted only I did not wait for another GLM round. The owner explicitly directed publication after local verification so GitHub Codex can inspect the exact remote head. |
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: a9df56a25c
ℹ️ 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".
| assert stream.getvalue().count("arrived during replay") == 1 | ||
|
|
||
|
|
||
| def test_recovery_during_replay_teardown_replays_the_retained_record(monkeypatch): |
There was a problem hiding this comment.
Fresh evidence beyond the earlier registration comments is the newly added test_recovery_during_replay_teardown_replays_the_retained_record node, which is absent from THEME-DIAGNOSTIC-DEFERRAL-001's guard list at this head. It can therefore be deleted, skipped, or removed from the default remaining partition without reopening the prevention; register this exact node with its CI and immutable evidence bindings.
AGENTS.md reference: AGENTS.md:L387-L390
Useful? React with 👍 / 👎.
| assert stream.getvalue().count("arrived during replay") == 1 | ||
|
|
||
|
|
||
| def test_recovery_during_replay_teardown_replays_the_retained_record(monkeypatch): |
There was a problem hiding this comment.
The new guard demonstrates that the deterministic teardown-handoff race survived the previously green guard set, but false_green_pairs contains no independent entry linked to this exact test—its existing theme pair is linked only to the AST diagnostic guard. Add a separate false-green prevention ID with this node and its red/green evidence so the coverage escape is enforced independently from the runtime correction.
AGENTS.md reference: AGENTS.md:L366-L369
Useful? React with 👍 / 👎.
| if _replay_followup_requested: | ||
| _replay_followup_requested = False | ||
| _replay_finalizing = False | ||
| continue |
There was a problem hiding this comment.
When a retained diagnostic is permanently record-specific rejected—for example, because its formatting or encoding fails—while other threads successfully log during every finalization window, each success sets _replay_followup_requested and this continue retries under the same replay ownership. Sustained logging can therefore keep the original logging thread inside this loop indefinitely while repeatedly emitting the rejected record; limit follow-up work per ownership acquisition or release ownership before scheduling another attempt.
Useful? React with 👍 / 👎.
|
@codex review Exact-head review requested for This head addresses the three findings on
Coordinator verification:
This remains a draft. Hosted CI and a clean Codex verdict on this exact head remain open. Please challenge the bounded replay mechanism for lost wakeups, retained-record loss, owner starvation, and any path that could still loop indefinitely. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df4d03be9a
ℹ️ 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".
| if _replay_followup_requested and replay_cycles < _MAX_REPLAY_CYCLES_PER_OWNERSHIP: | ||
| _replay_followup_requested = False | ||
| _replay_finalizing = False | ||
| continue |
There was a problem hiding this comment.
When a retained diagnostic rejects the initial attempt and first follow-up but would accept the next attempt, and a successful foreign log arrives during the second finalization window, this condition declines the requested follow-up because replay_cycles == 2; line 329 then clears the request and releases ownership. If that foreign log is the last emission before shutdown, the diagnostic remains queued despite recovery already having been observed. Fresh evidence at this head is the new cycle cap: carry the final request across the ownership handoff or otherwise schedule a bounded successor instead of discarding it.
AGENTS.md reference: AGENTS.md:L530-L532
Useful? React with 👍 / 👎.
| - node: tests/test_logging_setup.py::test_replay_ownership_bounds_followups_for_record_specific_rejection | ||
| ci_partition: remaining |
There was a problem hiding this comment.
Fresh evidence beyond the teardown false-green pair is this newly added bounded-follow-up guard, which closes the unbounded replay defect that survived the previously green suite, but false_green_pairs contains no separate entry linked to this exact node. Register its own stable false-green ID and red/green evidence binding so deletion, weakening, or deselection of the cap guard reopens the coverage escape independently from the runtime prevention.
AGENTS.md reference: AGENTS.md:L366-L369
Useful? React with 👍 / 👎.
1 participant
A colours file could stop the program from starting
cryodaq.gui.themecallsload_theme()at module level, so whateverresolve_theme()does happens before the first window exists. It raisedRuntimeErrorwhen the default pack could not be read:config/themes/warm_stone.yamlOn the laboratory machine that is a program that will not start, during a run that is hours in and a cryostat that is cold, because of a file that decides nothing but which greys to draw.
_load_theme_packhad the same two raises and the same consequence.The direction
Owner, 2026-08-20:
What replaces it
There is always something to draw with.
_LAST_RESORT_PACKholds the values ofconfig/themes/warm_stone.yaml, the default pack — copied, not invented — and a test pins every required token to that file, so the two cannot drift apart unnoticed.Its description is deliberately not the file's own:
The operator is told, rather than quietly handed a working window over a broken configuration. The reason is also recorded at
CRITICAL. The check is kept; only the stopping is gone — the standing rule that this software never refuses.Nothing changes for a configuration that works
Two tests exist for exactly this, because a fallback that quietly takes over from a pack that is perfectly fine would be its own defect:
The test that demanded the old behaviour
tests/gui/test_theme_loader.py::test_missing_default_pack_raisesrequired theRuntimeError. It is not a registered guard. Rather than delete it, it keeps its starting condition — an empty themes directory — and now states the opposite, so the module that once demanded the raise says why it does not.Evidence
Putting the raise back turns seven of the twelve new tests red:
Run on Ubuntu 22.04, the laboratory target, at this commit, import confirmed to resolve inside the probe worktree:
On Windows:
tests/gui2144 passed, 4 skipped.ruff checkandruff format --checkclean.Two observations found while measuring, neither fixed here
warm_stone.yamlreading "LOCKED — identical across all bundled themes" is not true:braun,gostandxcodecarry a different status set from the other ninewrite_theme_selectionstill raises, which is right — refusing to save a selection that would not load, and saying so, guides the operator instead of storing a choice that fails at the next startWritten with assistance from Claude (Anthropic).