From de9fd37ff8ae8f18fd16b5e2764890440f1a4d4e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 19:03:14 +0000 Subject: [PATCH 1/4] fix: distinguish AllUsers timeout from failure; audit-tool redirect/%* gaps CodeRabbit review round on PR #405, 5 findings: - run_setup.bat: :run_installer_timeout hardcodes its RC to 1 on a genuine 60-minute timeout (a sentinel, not the installer's real exit code) -- the AllUsers WARN was presenting that sentinel as exitCode=1 alongside reason=installer_failed. Stopped clearing the subroutine's own HP_INSTALLER_TIMEDOUT flag before return (each call re-sets it fresh at entry, so leaving it live is safe) and branch the WARN on it: reason=timeout with no fabricated exitCode on a real timeout, unchanged wording otherwise. - tests/selfapps_justme.ps1: the failedWordingAbsent negative assertion had silently degraded into one that could never fail -- it matched the exact pre-exitCode-annotation sentence, which no longer appears anywhere verbatim now that the WARN always carries a suffix. Matches the stable message prefix instead. - tools/audit_console_messages.py: normalize() now handles %* (all positional args); extract_records' redirect-skip is now escape-aware (skips a genuine trailing `>`/`>>` file redirect, but not a caret-escaped redirect belonging to a nested command, and not a literal '>=' inside real message text like "running (>=30 days since last update)") -- the old `>>`-only check was letting several genuine single-`>` file-redirected lines (JSON status writes, simulated-failure marker files) through as if they were console output. 4 new regression tests. Declined one CodeRabbit finding on this round (dedicated CI coverage for the AllUsers attempted-failure/timeout branches via a new test-only failure hook) as new-feature scope disproportionate to a review-comment quick-fix pass, per CLAUDE.md's own iteration-loop rule -- reasoning posted on the PR. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01A4iE1BRSkUETwz237XeuTW --- docs/agent-closed-backlog.md | 23 +++++++++++++++++ docs/demo-bootstrapper-output.md | 9 +++++++ run_setup.bat | 18 ++++++++++++-- tests/selfapps_justme.ps1 | 20 +++++++++------ tests/test_audit_console_messages.py | 37 ++++++++++++++++++++++++++++ tools/audit_console_messages.py | 30 +++++++++++++++++----- 6 files changed, 122 insertions(+), 15 deletions(-) diff --git a/docs/agent-closed-backlog.md b/docs/agent-closed-backlog.md index 8ed7df6d..f1e1fe4c 100644 --- a/docs/agent-closed-backlog.md +++ b/docs/agent-closed-backlog.md @@ -528,6 +528,29 @@ here, since those were the two items this pass touched for unrelated reasons). envsmoke-scoped log rather than the combined root+envsmoke text a second CodeRabbit comment flagged as a staleness risk for a negative assertion) confirming the new INFO wording fires and the old WARN wording does NOT, in the non-elevated simulation this test already runs. + **Follow-up fix, 2026-07-31 (same day, a later CodeRabbit review round on the follow-up PR):** + the genuine-failure WARN's `exitCode=%HP_CONDA_ALLUSERS_RC%` was itself sometimes a fabrication + -- `HP_CONDA_ALLUSERS_RC` is captured straight from `:run_installer_timeout`'s own return value, + which that subroutine's own header comment documents as "the installer's real exit code (or 1 + on timeout)": on a genuine 60-minute installer timeout, `HP_CONDA_ALLUSERS_RC` reads `1` as a + pure sentinel, not a real exit code, so the WARN was presenting a fabricated `exitCode=1` as if + the installer itself had returned it. Fixed by no longer clearing `:run_installer_timeout`'s own + `HP_INSTALLER_TIMEDOUT` flag before it returns (each call re-sets it fresh at entry regardless, + so leaving it live across `exit /b` is safe -- no caller can read a stale value from an earlier, + unrelated call), capturing it into a new `HP_CONDA_ALLUSERS_TIMEDOUT` variable right alongside + `HP_CONDA_ALLUSERS_RC`, and branching `:tci_justme`'s genuine-failure WARN a second time on it: + `reason=timeout` with no `exitCode` field on a real timeout, the existing + `exitCode=..., reason=installer_failed` wording otherwise. `tests/selfapps_justme.ps1`'s own + `failedWordingAbsent` negative assertion was separately found to have silently degraded into an + assertion that could never fail: it matched the OLD, pre-exitCode-annotation exact sentence + (`Miniconda AllUsers install failed; retrying with JustMe.`), which no longer appears anywhere + verbatim now that the WARN always carries an `exitCode=.../reason=...` or `reason=timeout` + suffix -- fixed to match the stable `Miniconda AllUsers install failed` prefix instead, so it + once again actually catches a skip-path regression to any failure-wording variant. No CI hook + can force a genuine timeout deterministically (would need a real 60-minute hang or a dedicated + fake-timeout test hook, neither built here), so the timeout branch itself remains + `[Extrapolated Branch]`-only in `docs/demo-bootstrapper-output.md`, same status as the + genuine-failure branch it refines. --- diff --git a/docs/demo-bootstrapper-output.md b/docs/demo-bootstrapper-output.md index d3f9b1d3..012c0b76 100644 --- a/docs/demo-bootstrapper-output.md +++ b/docs/demo-bootstrapper-output.md @@ -1555,6 +1555,15 @@ current CI hook forces without also forcing the skip path): [WARN] Miniconda AllUsers install failed (exitCode=1, reason=installer_failed); retrying with JustMe. ``` +If the installer instead hits `:run_installer_timeout`'s own 60-minute ceiling (see that +subroutine's header comment), the exit code is a hardcoded sentinel, not the installer's real +exit code -- reported as `reason=timeout` with no fabricated `exitCode` field instead +(`[Extrapolated Branch]`, an even rarer sub-case of the one above, never observed in CI): + +``` +[WARN] Miniconda AllUsers install failed (reason=timeout); retrying with JustMe. +``` + **If JustMe ALSO fails** (both installation options exhausted; REAL CI CAPTURE for the skip-path lines, `[Extrapolated Branch]` for the now-corrected wording): diff --git a/run_setup.bat b/run_setup.bat index e66eddb5..0a5be00d 100644 --- a/run_setup.bat +++ b/run_setup.bat @@ -4475,7 +4475,11 @@ set "HP_INSTALLER_ARGS=" set "HP_INSTALLER_TIMEOUT_MS=" set "HP_INSTALLER_RESULT=" set "HP_INSTALLER_LABEL=" -set "HP_INSTALLER_TIMEDOUT=" +rem derived requirement: HP_INSTALLER_TIMEDOUT is deliberately NOT cleared here -- it is the +rem only way a caller can tell a timeout (HP_INSTALLER_RC hardcoded to 1, not a real installer +rem exit code) apart from a genuine installer failure that happens to also exit 1. Each call +rem freshly re-sets it at entry (either "0" or from the result file), so leaving it live across +rem return is safe: no caller can ever read a stale value from an earlier, different call. exit /b %HP_INSTALLER_RC% :try_conda_install rem derived requirement: AllUsers install can fail when UAC rejects elevation even for admin accounts. @@ -4487,6 +4491,7 @@ rem only skipped, so :tci_justme's own log line can tell the two apart instead o rem claiming AllUsers "failed" even when it was never attempted. Reset defensively at entry in case rem a future caller invokes this subroutine more than once in the same process. set "HP_CONDA_ALLUSERS_ATTEMPTED=" +set "HP_CONDA_ALLUSERS_TIMEDOUT=" rem derived requirement: non-admin machines produce a UAC prompt when AllUsers install is attempted; rem skip directly to JustMe when the process is not elevated. rem HP_TEST_NOT_ELEVATED=1 simulates a non-admin environment for CI coverage of this branch. @@ -4509,13 +4514,22 @@ set "HP_CONDA_ALLUSERS_ATTEMPTED=1" set "HP_CONDA_ALLUSERS_RC=" call :run_installer_timeout "%TEMP%\miniconda.exe" "/InstallationType=AllUsers /AddToPath=0 /RegisterPython=0 /S /D=%MINICONDA_ROOT%" 3600000 "Miniconda AllUsers" set "HP_CONDA_ALLUSERS_RC=%ERRORLEVEL%" +set "HP_CONDA_ALLUSERS_TIMEDOUT=%HP_INSTALLER_TIMEDOUT%" if not "%HP_CONDA_ALLUSERS_RC%"=="0" goto :tci_justme set "HP_CONDA_INSTALL_MODE=AllUsers" call :log "[INFO] Miniconda installed successfully." goto :eof :tci_justme +rem derived requirement: a genuine timeout (HP_CONDA_ALLUSERS_TIMEDOUT=1, see +rem :run_installer_timeout's own header comment) hardcodes HP_CONDA_ALLUSERS_RC to 1 -- that is +rem a sentinel, not the installer's real exit code, so it must not be presented as one; report +rem reason=timeout instead of a fabricated exitCode. if defined HP_CONDA_ALLUSERS_ATTEMPTED ( - call :log "[WARN] Miniconda AllUsers install failed (exitCode=%HP_CONDA_ALLUSERS_RC%, reason=installer_failed); retrying with JustMe." + if "%HP_CONDA_ALLUSERS_TIMEDOUT%"=="1" ( + call :log "[WARN] Miniconda AllUsers install failed (reason=timeout); retrying with JustMe." + ) else ( + call :log "[WARN] Miniconda AllUsers install failed (exitCode=%HP_CONDA_ALLUSERS_RC%, reason=installer_failed); retrying with JustMe." + ) ) else ( call :log "[INFO] Miniconda AllUsers install skipped (not elevated); trying JustMe install instead." ) diff --git a/tests/selfapps_justme.ps1 b/tests/selfapps_justme.ps1 index f1f48fe0..958466f1 100644 --- a/tests/selfapps_justme.ps1 +++ b/tests/selfapps_justme.ps1 @@ -60,14 +60,20 @@ $justmeInstalled = $combinedText -match 'Miniconda installed \(JustMe fallback\) # derived requirement: [Active Backlog item 16, renumbered from 11 -- see docs/agent-closed- # backlog.md] the shared :tci_justme label must NOT claim AllUsers "failed" when it was only ever # skipped (never launched) -- this scenario's own HP_TEST_NOT_ELEVATED=1 takes the skip path, so -# the correct line is the INFO "skipped" wording, and the old unconditional WARN "failed" wording -# must NOT appear at all in this run. Checked against $setupText (the envsmoke-scoped log for -# THIS specific sub-bootstrap) rather than $combinedText -- unlike the two pre-existing presence -# checks above, a negative ("must NOT appear") assertion would be vulnerable to stale/unrelated -# content in the shared repo-root ~setup.log if it were ever written by an earlier, different step -# in the same job. +# the correct line is the INFO "skipped" wording, and no "AllUsers install failed" WARN wording +# (any variant -- exitCode/reason or timeout) must appear at all in this run. Checked against +# $setupText (the envsmoke-scoped log for THIS specific sub-bootstrap) rather than $combinedText +# -- unlike the two pre-existing presence checks above, a negative ("must NOT appear") assertion +# would be vulnerable to stale/unrelated content in the shared repo-root ~setup.log if it were +# ever written by an earlier, different step in the same job. +# +# Matches on the stable "Miniconda AllUsers install failed" PREFIX, not a full literal sentence -- +# a prior version of this assertion matched the exact pre-exitCode-annotation wording, which no +# longer appears anywhere (the WARN line always carries exitCode=.../reason=... or reason=timeout +# now), so that check had silently degraded into an assertion that could never fail regardless of +# whether the skip-path regression it exists to catch actually recurred. $skippedWordingCorrect = $setupText -match 'Miniconda AllUsers install skipped \(not elevated\); trying JustMe install instead\.' -$failedWordingAbsent = -not ($setupText -match 'Miniconda AllUsers install failed; retrying with JustMe\.') +$failedWordingAbsent = -not ($setupText -match 'Miniconda AllUsers install failed') $pass = $notElevatedSkip -and $justmeInstalled -and $skippedWordingCorrect -and $failedWordingAbsent diff --git a/tests/test_audit_console_messages.py b/tests/test_audit_console_messages.py index 254fc15d..5a26b9f2 100644 --- a/tests/test_audit_console_messages.py +++ b/tests/test_audit_console_messages.py @@ -27,6 +27,10 @@ def test_normalize_adjacent_expansions(): assert acm.normalize('rc=%RC% size=%SIZE%') == 'rc= size=' +def test_normalize_all_args_param(): + assert acm.normalize('[INFO] Forwarding arguments: %*') == '[INFO] Forwarding arguments: ' + + def test_extract_records_skips_echo_control_tokens(tmp_path): bat = tmp_path / 'run_setup.bat' bat.write_text('@echo off\necho.\necho on\necho [INFO] real message\n', encoding='ascii') @@ -53,6 +57,39 @@ def test_extract_records_skips_redirected_lines(tmp_path): assert records == [(3, '[INFO] visible line')] +def test_extract_records_skips_single_arrow_redirected_lines(tmp_path): + bat = tmp_path / 'run_setup.bat' + bat.write_text( + 'echo not visible > log.txt\n' + 'call :log "not visible either" > log.txt\n' + 'echo [INFO] visible line\n', + encoding='ascii', + ) + records = acm.extract_records(bat) + assert records == [(3, '[INFO] visible line')] + + +def test_extract_records_keeps_ge_symbol_in_message_content(tmp_path): + # A message containing a literal '>=' comparison (not a shell redirect) must not be + # mistaken for a redirected line. + bat = tmp_path / 'run_setup.bat' + bat.write_text('call :log "[INFO] running (>=30 days since last update)."\n', encoding='ascii') + records = acm.extract_records(bat) + assert records == [(1, '[INFO] running (>=30 days since last update).')] + + +def test_extract_records_keeps_caret_escaped_redirect_of_nested_command(tmp_path): + # A caret-escaped redirect belongs to a NESTED command (e.g. a powershell subprocess's own + # stderr), not to this call :log line itself -- it must not cause a skip. + bat = tmp_path / 'run_setup.bat' + bat.write_text( + 'for /f "delims=" %%P in (`powershell -Command "1" 2^>nul`) do call :log "[INFO] Value: %%P"\n', + encoding='ascii', + ) + records = acm.extract_records(bat) + assert records == [(1, '[INFO] Value: ')] + + def test_is_covered_true_when_all_segments_present(): assert acm.is_covered('[INFO] Log: ', '... [INFO] Log: C:\\work\\ ...') is True diff --git a/tools/audit_console_messages.py b/tools/audit_console_messages.py index 1f0dbf2d..f21df4ec 100644 --- a/tools/audit_console_messages.py +++ b/tools/audit_console_messages.py @@ -8,9 +8,10 @@ What it does: 1. Extracts every `echo ` and `call :log ""` line from run_setup.bat (case-insensitively, tolerating a leading `@`; skipping blank/`echo off`/ - `echo on` control lines and lines redirected to a file with `>>`, which never - reach the live console) and normalizes `%VAR%`, `%~dp0`/`%~1`-style - positional parameters, and `%%M`-style for-loop variables to a single + `echo on` control lines and lines redirected to a file with `>`/`>>` (caret- + escaped redirects and `>=` inside message text are not treated as redirects), + which never reach the live console) and normalizes `%VAR%`, `%~dp0`/`%~1`/`%*`- + style positional parameters, and `%%M`-style for-loop variables to a single placeholder so lines differing only by runtime substitution match. 2. Splits each normalized message on its placeholder tokens and checks whether every resulting literal segment (length >= 6, to skip noise) appears anywhere @@ -41,14 +42,23 @@ re.IGNORECASE, ) +# A real redirect operator: one or two literal '>' not preceded by a caret escape (a caret +# protects an INNER command's own redirect, e.g. the `2^>nul` on a nested `powershell -Command` +# call, from being read as this line's own redirect) and not immediately followed by '=' (so a +# genuine console message containing '>=' -- e.g. "running (>=30 days since last update)" -- +# is never mistaken for a redirect). +REDIRECT_RE = re.compile(r'(?{1,2}(?!=)') + def normalize(text: str) -> str: # %%VAR / %%~zS -style for-loop variable references -- must run before the single-percent # patterns below, since %% would otherwise look like an empty %...% pair to them. text = re.sub(r'%%~?[A-Za-z][A-Za-z0-9]*', '', text) - # %~dp0 / %~1 / %~nx1 -style positional/modified batch parameters, and bare %1-%9. + # %~dp0 / %~1 / %~nx1 -style positional/modified batch parameters, bare %1-%9, and %* + # (all positional arguments). text = re.sub(r'%~[A-Za-z$:]*[0-9]', '', text) text = re.sub(r'%[0-9]\b', '', text) + text = re.sub(r'%\*', '', text) # %VAR% environment variable references. Requires no whitespace inside the delimiters (real # batch variable names never contain spaces) so a literal, isolated '%' earlier in the same # line (e.g. "10% free on %DRIVE%") can't be greedily treated as this pattern's opening @@ -67,13 +77,21 @@ def extract_records(bat_path: Path): body = m.group(1) if body.strip().lower() in ('.', 'off', 'on'): continue - if '>>' in raw: + # `echo`'s body is unquoted plain text, so a trailing redirect (`> file`, `>> file`) + # is indistinguishable from message content by position alone -- scan the whole raw + # line. No current echo message contains a real (non-'>=', non-caret-escaped) '>', + # confirmed by a full-file sweep at the time this check was added. + if REDIRECT_RE.search(raw): continue records.append((i, normalize(body))) continue m = re.search(r'call :log\s+["\']([^"\']*)["\']', line, re.IGNORECASE) if m: - if '>>' in raw: + # call :log's message is quote-delimited, so only the text AFTER the closing quote + # can be a real redirect -- this correctly leaves a message that itself contains + # '>=' (e.g. "running (>=30 days since last update)") untouched, since that '>=' is + # inside the matched group, never scanned here. + if REDIRECT_RE.search(line[m.end():]): continue records.append((i, normalize(m.group(1)))) return records From 95fb59113601b96f86dfa5aac37bffe6379b4cc2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 20:42:40 +0000 Subject: [PATCH 2/4] fix: audit tool must not skip call :log on a later chained command's redirect CodeRabbit review round on PR #406: extract_records' redirect check scanned the ENTIRE tail after a call :log's closing quote for a real '>'/'>>', which incorrectly treated a redirect on a separately-chained LATER command (e.g. `call :log "[INFO] visible" & echo hidden > log.txt`) as if it belonged to the call :log itself, dropping a genuinely console-visible record. Truncate the tail at the first real (non-caret-escaped) command separator (& or |) before checking for a redirect, so only a redirect in call :log's OWN command segment causes a skip. No current run_setup.bat line hits this (verified via grep), so this is a latent-bug fix, not a live false negative. 2 new regression tests. Declined two other findings on this round, with reasoning posted on the PR: dedicated CI coverage for the AllUsers timeout/failure branches (same new-feature-scope reasoning already given on PR #405 for the identical ask), and validating :run_installer_timeout's result-file shape before accepting it (pre-existing behavior this PR doesn't touch, explicitly "Heavy lift", and the proposed remedy -- not retrying JustMe on an indeterminate result -- has its own real design tradeoff against the current conservative default). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01A4iE1BRSkUETwz237XeuTW --- tests/test_audit_console_messages.py | 19 +++++++++++++++++++ tools/audit_console_messages.py | 20 ++++++++++++++++++-- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/tests/test_audit_console_messages.py b/tests/test_audit_console_messages.py index 5a26b9f2..e88aa929 100644 --- a/tests/test_audit_console_messages.py +++ b/tests/test_audit_console_messages.py @@ -90,6 +90,25 @@ def test_extract_records_keeps_caret_escaped_redirect_of_nested_command(tmp_path assert records == [(1, '[INFO] Value: ')] +def test_extract_records_keeps_call_log_with_redirect_on_later_chained_command(tmp_path): + # A redirect on a LATER command chained via '&' belongs to that command, not to the + # call :log that already reached the console before the separator -- it must not cause + # the call :log record to be dropped. + bat = tmp_path / 'run_setup.bat' + bat.write_text('call :log "[INFO] visible" & echo hidden > log.txt\n', encoding='ascii') + records = acm.extract_records(bat) + assert records == [(1, '[INFO] visible')] + + +def test_extract_records_skips_call_log_with_redirect_in_same_segment(tmp_path): + # A redirect immediately after the closing quote, with no intervening command + # separator, DOES belong to this call :log -- it must still be skipped. + bat = tmp_path / 'run_setup.bat' + bat.write_text('call :log "[INFO] hidden" > log.txt\n', encoding='ascii') + records = acm.extract_records(bat) + assert records == [] + + def test_is_covered_true_when_all_segments_present(): assert acm.is_covered('[INFO] Log: ', '... [INFO] Log: C:\\work\\ ...') is True diff --git a/tools/audit_console_messages.py b/tools/audit_console_messages.py index f21df4ec..f6942dff 100644 --- a/tools/audit_console_messages.py +++ b/tools/audit_console_messages.py @@ -49,6 +49,20 @@ # is never mistaken for a redirect). REDIRECT_RE = re.compile(r'(?{1,2}(?!=)') +# A real command separator (& / | / && / ||), not caret-escaped. A redirect appearing AFTER one +# of these belongs to a later, separately-chained command -- it must not cause the FIRST +# command's own record (e.g. a `call :log` that already reached the console before the +# separator) to be dropped. +SEGMENT_END_RE = re.compile(r'(? str: + """Truncate `tail` at the first real command separator, so a redirect belonging to a + later chained command is never mistaken for one that applies to the command being + checked.""" + m = SEGMENT_END_RE.search(tail) + return tail[:m.start()] if m else tail + def normalize(text: str) -> str: # %%VAR / %%~zS -style for-loop variable references -- must run before the single-percent @@ -90,8 +104,10 @@ def extract_records(bat_path: Path): # call :log's message is quote-delimited, so only the text AFTER the closing quote # can be a real redirect -- this correctly leaves a message that itself contains # '>=' (e.g. "running (>=30 days since last update)") untouched, since that '>=' is - # inside the matched group, never scanned here. - if REDIRECT_RE.search(line[m.end():]): + # inside the matched group, never scanned here. Truncate at the first real command + # separator first: a redirect on a LATER, `&`-chained command (e.g. `call :log "..." + # & echo hidden > file`) does not mean call :log itself was redirected. + if REDIRECT_RE.search(_own_command_segment(line[m.end():])): continue records.append((i, normalize(m.group(1)))) return records From e8ec14c0118f286eeea860ed58a8afe2a7596ed4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 21:05:00 +0000 Subject: [PATCH 3/4] docs: file cache-lane self-perpetuating-corruption bug (item 19) Investigated a maintainer report that the cache CI lane "never works," always logging "Cache corrupted, skipping fast-path tests." Traced the mechanism: once a restored cache fails its health check, the one step capable of a fresh install is skipped (gated on HP_CACHE_CORRUPTED), and the save step is gated on the same flag -- so a poisoned cache blob can never be replaced by a fresh one, only re-detected as corrupted forever. Documented as CLAUDE.md Active Backlog item 19 (checked for a number collision against the closed backlog first, per this repo's own established discipline -- 19 was confirmed unused) with a reasoned-through but not-yet-implemented fix, since verifying it needs multiple real cache-lane CI cycles rather than fitting safely into a downtime aside. Added a matching entry to docs/open-questions.md asking whether the fix is worth the multi-cycle verification effort given the lane is already non-gating by design. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01A4iE1BRSkUETwz237XeuTW --- CLAUDE.md | 53 ++++++++++++++++++++++++++++++++++++++++++ docs/open-questions.md | 22 ++++++++++++++++-- 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 35dd6d03..084878b5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -729,6 +729,59 @@ were renumbered to 17 and 16 respectively when archived -- see item 18 below and `docs/agent-closed-backlog.md` only if it was ALSO independently resolved -- an item can be renumbered without being closed, if its underlying finding is still open. +- **19. The `cache` CI lane's corruption recovery is a one-way trap: once a restored cache is + flagged corrupted, nothing in that lane ever produces a fresh, valid cache again -- found + 2026-08-01 while investigating a maintainer report that the lane "never works," always logging + `Cache corrupted, skipping fast-path tests (HP_CACHE_CORRUPTED=1)`, confirmed against the + current `.github/workflows/batch-check.yml` source, not just the symptom report.** Traced the + full mechanism: the cache key is `win-...-conda-${{ hashFiles('run_setup.bat') }}-` + with `restore-keys: win-...-conda-` as a prefix fallback. `run_setup.bat` changes on nearly + every PR in this repo, so the EXACT primary key rarely matches twice -- the restore step almost + always falls through to the `restore-keys` PREFIX match instead, which returns whatever cache + blob currently exists under that prefix (GitHub Actions cache entries are immutable once saved; + a "stale" blob can only be replaced by a NEW save under a NEW key, never overwritten in place). + The "Validate restored conda binary" step (`cache_health`) then runs `conda.bat info` against + whatever got restored; on failure it sets `HP_CACHE_CORRUPTED=1` (informational, `exit 0`, by + design -- this part is fine). The trap is downstream: the "Bootstrap environment (run_setup.bat)" + step -- the ONLY step in this lane capable of performing a fresh Miniconda install -- is gated on + `env.HP_CACHE_CORRUPTED != '1'`, so once corruption is flagged, bootstrap is SKIPPED ENTIRELY for + that run; no fresh install is ever attempted. The save step (`actions/cache/save`) is gated on + BOTH `steps.conda_cache_restore.outputs.cache-hit != 'true'` AND `env.HP_CACHE_CORRUPTED != '1'` + -- since a `restore-keys` prefix match reports `cache-hit: false` (only an EXACT primary-key + match reports `true`, confirmed against `actions/cache`'s own documented behavior), the + `cache-hit` half of the save gate is usually already satisfied when corruption is the actual + blocker -- the `HP_CACHE_CORRUPTED` half is what stops the save. Net effect: the SAME poisoned + blob (saved once, likely before this health-check mechanism existed, or from a one-off flake) + gets restored via the prefix fallback on every subsequent run, is correctly detected as + corrupted every time, but the detection itself prevents the one action (a fresh install this + run, followed by a fresh save) that would ever replace it -- a permanent, self-perpetuating + loop with no exit, fully consistent with "never works, always says corrupted." + **Confirmed this is real, not a one-off**: every `if:` gate on the lane's ~25 self-test steps + after "Bootstrap environment" already depends on `HP_CACHE_CORRUPTED != '1'`, so once corrupted, + the entire lane short-circuits to placeholder `pass:true, skip`-style NDJSON rows + (`self.cache.corrupted`) and reports overall green -- exactly the "always green to avoid being + gating" behavior observed, and exactly why this has been invisible in CI: nothing ever fails + loud enough to surface it as a real problem, it just silently never does its job. + **Not fixed in this pass** -- diagnosed only, per the maintainer's own "if you see an easy fix, + maybe put that in the backlog" framing; the fix touches shared CI workflow gating logic that + ~30 other steps also depend on, and can only be verified by watching real cache-lane runs + (multi-cycle, since the fix's own effect -- "does a fresh cache finally get saved" -- isn't + observable from a single run), so it doesn't fit safely into a downtime aside. **Suggested fix, + reasoned through but not implemented:** on a corruption detection that came from a `restore-keys` + PREFIX match specifically (`steps.conda_cache_restore.outputs.cache-hit != 'true'`, i.e. not an + exact primary-key hit), delete the corrupted `C:\Users\Public\Documents\Miniconda3` directory + and do NOT set `HP_CACHE_CORRUPTED` at all -- let the run fall through exactly like a genuine + cache miss (the health-check step's own existing "No conda binary found; fresh install will + proceed normally" branch already handles this shape correctly for a true miss). This lets + "Bootstrap environment" run a real fresh install, and lets the save step create a genuinely + fresh, valid cache entry under the current key afterward, breaking the loop. The narrower case + -- an EXACT primary-key hit that's ALSO corrupted (only plausible when `run_setup.bat` is + byte-identical to a previously-poisoned save, e.g. two runs on the same unchanged commit) -- + would still be stuck, since that specific key's blob can never be overwritten; fully closing + that gap needs an explicit cache-deletion API call (`gh cache delete` / the GitHub Actions cache + REST API, `DELETE /repos/{owner}/{repo}/actions/caches`) gated on `cache-hit == 'true'` at + corruption-detection time, a smaller follow-on refinement once the main fix is proven working. + ## Cold Storage (promising ideas, deliberately shelved -- revisit only if a named trigger fires) Moved to `docs/agent-cold-storage.md` (2026-07-31, to reduce this file's per-session context diff --git a/docs/open-questions.md b/docs/open-questions.md index eff281dd..fc7f4b68 100644 --- a/docs/open-questions.md +++ b/docs/open-questions.md @@ -9,9 +9,27 @@ changelog-style sections are for. --- -_No open items right now._ The cascade-consent design question (was item 1: whether the cascade +**1. Is the `cache` CI lane's self-perpetuating-corruption bug (CLAUDE.md Active Backlog item 19) +worth fixing, given the lane is explicitly non-gating/informational by design?** Diagnosed +2026-08-01: once a restored cache is flagged corrupted, the lane has no code path that ever +produces a fresh valid cache again (the one step that could do a fresh install is skipped +whenever corruption is detected, and the save step is gated on the same flag) -- so it likely +never recovers on its own, consistent with the maintainer's own report that it "never works." +A concrete fix is reasoned through in item 19's own writeup (treat a `restore-keys` prefix-match +corruption the same as a genuine cache miss: delete the bad directory, let a fresh install and +fresh save proceed normally) but verifying it needs multiple real cache-lane CI cycles to observe +"does a fresh cache finally get saved," which is slower and more failure-prone to get right than +a typical code fix. Since this lane exists purely to save ~99 MB of download time and is +deliberately excluded from PR-merge gating either way, is it worth the multi-cycle verification +effort, or should it stay as documented, low-priority backlog debt? No strong reason not to +attempt it in a dedicated future loop either way -- flagging so the priority call is explicit +rather than assumed. + +--- + +The cascade-consent design question (was item 1: whether the cascade decision should remain an interruptive consent prompt at all, given `HP_CASCADE_CANDIDATE`'s own -mixed reliability) is now resolved -- kept exactly as shipped, no code change. See CLAUDE.md's +mixed reliability) is resolved -- kept exactly as shipped, no code change. See CLAUDE.md's Known Findings entry for the decision and `docs/agent-interconnect.md`'s "Cascade signal reliability" subsection (under "Post-execution checkpoint") for the full truth-table analysis that informed it. From eda56235deddd573c9737051b70733ed1d11e3b3 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 22:06:06 +0000 Subject: [PATCH 4/4] docs: drop backlog renumber-on-collision rule; note demo-doc reorg TODO Owner decision 2026-08-01: the renumber-on-collision convention (and the effort spent hunting for number collisions against the closed-backlog archive) is more rigor than a plain-text backlog needs. Relaxed the Active Backlog numbering intro to treat item numbers as informal, non- unique labels; moved the decision itself into Known Findings; removed the old item 18 (which existed only to track renumbering items 8, 10, 12, 13, 14, 15 -- no longer needed under the new rule, those items keep their current numbers permanently). Item 19 (filed the same session, the cache-lane finding) is unaffected by this and keeps its number. Also added a TODO note to docs/demo-bootstrapper-output.md's own intro (owner request) for a future flow-reorg pass: move Scenario 38 ("No .py files at all") from the very end of the doc to the front (it's the most foundational case), push Part I/Part II further down to make room, and update cross-references -- flagged for its own dedicated pass, not attempted here given the scope (renumbering ~8 parts' worth of anchors in a 2500+ line doc) and this session's CI-babysitting context. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01A4iE1BRSkUETwz237XeuTW --- CLAUDE.md | 72 ++++++++++++-------------------- docs/demo-bootstrapper-output.md | 26 +++++++++--- 2 files changed, 46 insertions(+), 52 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 084878b5..1a128182 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -481,17 +481,16 @@ a fact confirmed with no action needed, or a recurring/periodic check belongs in "Known Findings", `docs/agent-lessons-learned.md`, or "Periodic Maintenance Checks" below instead; a promising idea deliberately shelved pending a specific, named trigger belongs in "Cold Storage" below instead of here (see that section's own scope note for the distinction from this one). -Item numbers are stable cross-reference identifiers, not sequential list positions -- once an -item is fully resolved it is removed from here entirely and archived (keeping its original -number) in `docs/agent-closed-backlog.md`, which is why the numbering below does not start at 1. -**Exception: if a new item's number collides with a number already permanently retired by an -older, unrelated closed item, renumber the new item to the next never-used number when archiving -it, and document the collision (old number, why, cite the colliding entry) in its -closed-backlog entry.** Keeping the original number is still the default; renumbering only -happens to resolve a genuine collision, never for its own sake. Concrete precedent: items 9 and -11 (filed 2026-07-29) each collided with an older, already-closed item of the same number and -were renumbered to 17 and 16 respectively when archived -- see item 18 below and -`docs/agent-closed-backlog.md`'s Item 16/17 entries for the full trace. +Item numbers are informal labels for cross-referencing within a session or PR, not a +guaranteed-unique ID scheme -- pick anything that looks free in the list below when filing a new +item; do not cross-check it against `docs/agent-closed-backlog.md`'s history first, and do not +renumber an item if it later turns out to coincidentally repeat an older, already-closed item's +number. (Owner decision 2026-08-01, see Known Findings below: the earlier renumber-on-collision +convention was more rigor than a plain-text backlog needs -- a real uniqueness guarantee belongs +in an actual issue tracker, e.g. GitHub Issues, not a hand-maintained numbering scheme here.) +Once an item is fully resolved it is removed from here entirely and archived (keeping its +original number) in `docs/agent-closed-backlog.md`, which is why the numbering below does not +start at 1 and has gaps. - **8. `[WARN] UNC paths not supported` fires unconditionally in CI on an ordinary (non-UNC) local path -- found 2026-07-29 while gathering real console-output evidence for @@ -694,41 +693,6 @@ were renumbered to 17 and 16 respectively when archived -- see item 18 below and of what's shown) -- but a future pass fixing this should also confirm no currently-passing test silently relies on the unbounded behavior before adding a timeout. -- **18. Active Backlog items 8, 10, 12, 13, 14, and 15 all appear to reuse item numbers already - permanently retired by older, unrelated closed items -- found 2026-07-31 while closing out - items 9 and 11 during a `/goal`-directed backlog-fix pass, both of which turned out to have the - identical problem (fixed for those two; this item tracks the rest).** The batch of findings - filed 2026-07-29 while documenting the bootstrapper for `docs/demo-bootstrapper-output.md` - (this file's current items 8, 10, 12, 13, 14, 15, plus the now-fixed 9 and 11) appears to have - picked its numbers by eyeballing what looked unused in THIS file at the time, without checking - `docs/agent-closed-backlog.md`'s own "Closed Backlog" changelog section (2026-07-25 through - 2026-07-27 work) for numbers already retired there. Confirmed for 9 and 11 specifically (both - collided with real, already-closed, differently-numbered items -- see - `docs/agent-closed-backlog.md`'s Item 16 and Item 17 entries for the full trace of each) and - both renumbered to 16/17 when moved out of this file in the same pass that found this. A quick - grep-based check (`grep -n "item N\b"` across `docs/agent-closed-backlog.md`, `docs/agent- - ndjson.md`, `docs/agent-interconnect.md`, `docs/agent-lessons-learned.md` for each of N in - 8, 10, 12, 13, 14, 15) shows a same-number hit in the older Closed Backlog section for every - single one of them, strongly suggesting the same mistake repeats across the whole batch -- - but each was only confirmed by number match, not individually read and verified the way 9 and - 11 were, so **treat this as a strong lead, not a certainty, until each one gets the same - individual check.** Notably, item 14's collision is NOT purely a docs problem: `run_setup.bat` - itself has a live `rem derived requirement: [Active Backlog item 14]` comment (in - `:try_conda_install`, next to the Miniconda installer timeout) that refers to the OLDER, - already-closed item 14 (the 60-minute installer-timeout work), not the current active item 14 - (the misleading post-exhaustion syntax-error message) -- so fixing this properly means checking - inline source comments too, not just docs. **Not fixed in this pass** -- renumbering the - remaining six items correctly requires reading each one's full closed-backlog collision - individually (to write an accurate, non-templated "renumbered from X because Y" note the way - 16/17 got), then re-numbering every cross-reference to each (docs and, per the item-14 finding - above, possibly `run_setup.bat`'s own comments) -- real, careful work, not a batch find-replace, - and disproportionate to fold into an unrelated backlog-fix pass. Suggested approach for a - future pass: process one item at a time (matching this repo's own iteration discipline), confirm - its collision, pick the next genuinely-unused number (19 is next after this item, assuming no - further items get filed first), update its own text plus every cross-reference, and move it to - `docs/agent-closed-backlog.md` only if it was ALSO independently resolved -- an item can be - renumbered without being closed, if its underlying finding is still open. - - **19. The `cache` CI lane's corruption recovery is a one-way trap: once a restored cache is flagged corrupted, nothing in that lane ever produces a fresh, valid cache again -- found 2026-08-01 while investigating a maintainer report that the lane "never works," always logging @@ -972,6 +936,22 @@ of a second or third pin actually needing it. ## Known Findings (diagnosed, no action warranted) +- **Backlog item numbering: renumber-on-collision convention dropped, 2026-08-01 owner decision.** + A prior pass found that Active Backlog items 9 and 11 (filed 2026-07-29) each collided with an + older, already-closed item of the same number, and -- following a convention this file used to + document -- renumbered them to 17 and 16 when archiving (see `docs/agent-closed-backlog.md`'s + Item 16/17 entries; left as-is, not worth unwinding already-completed, harmless work). That same + pass flagged the remaining active items (8, 10, 12, 13, 14, 15) as likely sharing the same + collision, filed as its own Active Backlog item to renumber them individually in a future pass. + **Owner call: this is unnecessary maintenance overhead for a plain-text backlog** -- there is no + practical way to keep hand-tracking collision-free numbers against a growing closed-history + archive without an extra bookkeeping system, and a real uniqueness guarantee belongs in an + actual issue tracker (e.g. GitHub Issues) if it's ever genuinely needed, not a markdown + convention here. Decision: item numbers are now informal, non-unique labels (see the Active + Backlog section's own updated intro text above); items 8, 10, 12, 13, 14, 15 keep their current + numbers permanently, coincidental collisions with closed-item numbers are not a defect, and the + item that tracked renumbering them is closed with no further action. + - **Cascade consent gate design (timed prompt, decline-by-default) kept exactly as shipped, 2026-07-26 owner decision -- closes `docs/open-questions.md` item 1.** Following the cascade-vs-postexec fix (see Closed Backlog), a deeper investigation into the reliability of diff --git a/docs/demo-bootstrapper-output.md b/docs/demo-bootstrapper-output.md index 012c0b76..d07853e3 100644 --- a/docs/demo-bootstrapper-output.md +++ b/docs/demo-bootstrapper-output.md @@ -14,12 +14,26 @@ job log (cited with run ID, job ID, lane, and test file) or, where noted, taken `run_setup.bat`'s current source because no CI run has exercised that exact wording yet -- always labeled explicitly which case applies, never presented as a real capture when it isn't. -**Scope:** two parts, grouped by feature area and roughly in the order each was reviewed. Part I -covers the AV-Safe Build Path work (Tier A Nuitka fallback, its interaction with hidden-import -auto-recovery, and the requirement-9 optimized-build offer). Part II covers the CLI-interactivity -plan (`docs/plan-cli-interactive-verification.md`): live-tee verification, argv passthrough -(REQ-026), and honest ambiguous-exit messaging (REQ-027). Extend with a new Part as new feature -areas get reviewed, rather than growing either existing Part indefinitely. +**Scope:** grouped by feature area, roughly in the order each was reviewed (now eight Parts, not +the original two -- this paragraph covers only the first two below since they were the doc's +starting point; see the table of contents for the full current list). Part I covers the AV-Safe +Build Path work (Tier A Nuitka fallback, its interaction with hidden-import auto-recovery, and the +requirement-9 optimized-build offer). Part II covers the CLI-interactivity plan +(`docs/plan-cli-interactive-verification.md`): live-tee verification, argv passthrough (REQ-026), +and honest ambiguous-exit messaging (REQ-027). Extend with a new Part as new feature areas get +reviewed, rather than growing any existing Part indefinitely. + +**TODO for the next reorg pass (owner request, 2026-08-01, not done yet -- flow only, no content +change):** the current ordering front-loads two fairly narrow/advanced topics (Part I, Part II) +before the reader ever sees the basic happy path. Move Scenario 38 ("No `.py` files at all -- the +graceful `no_python_files` exit," currently the very LAST scenario in the doc, in Part VIII) up to +the front -- it is the simplest, most foundational case (what happens before anything else can +even run) and reads more naturally as an early scenario than a footnote at the end. Push Part I +and Part II further down, or to the end, to make room. This is a pure reordering/flow pass: move +the existing sections, then update the table of contents anchors and any in-doc cross-references +that name a Part by number (e.g. "see Part I" / "see Part III") so they still point at the right +content after the move -- no scenario text itself should change. Sized for its own dedicated pass, +not a drive-by edit alongside unrelated content changes. **Console vs. `~setup.log`:** the bootstrapper writes to two different places that are easy to conflate: