Skip to content

fix(#712): skip session-start-hook.sh's stdout buffer under an active trace - #733

Merged
fdaviddpt merged 4 commits into
mainfrom
fix/712
Sep 19, 2026
Merged

fdaviddpt merged 4 commits into
mainfrom
fix/712

Conversation

@fdaviddpt

Copy link
Copy Markdown
Contributor

Round-2 windows-latest leg of #690. bash -x scripts/session-start-hook.sh was observed exiting 0 with zero stdout bytes on windows-latest in test_the_real_hook_traces_past_the_bootstrap, while six neighbouring bootstrap-dirs.sh-only tests in the same file pass on the same runner. No Windows runner was available to this lane (or to the original issue filer) to step through the actual Git-Bash fd mechanism directly, so the fix below is REASONED, not OBSERVED, on the one platform it targets.

session-start-hook.sh buffers its own stdout into a temp file (exec 3>&1; exec > "$_REMEMBER_CTX_FILE") so a plugin-promo systemMessage JSON can wrap the output, then restores the real fd and cats the buffer back at the end. This adds a guard that skips the whole buffer/fd-swap whenever an active xtrace (case "$-" in (*x*))) or REMEMBER_TRACE=1 is set -- mirroring the existing, already-shipped precedent in bootstrap-dirs.sh, which already stands its own fd 2 redirect down under the identical two conditions (don't shadow output an operator deliberately went looking for). When the guard fires, _REMEMBER_CTX_OK stays empty and the hook falls back to the exact code path already proven safe by test_unwritable_ctx_buffer_does_not_burn_the_cooldown (buffer unwritable -> print live, no promo, no cooldown marker burned). The only visible cost of the guard firing is that the plugin-promo banner does not show under an active trace; the memory context itself is never affected.

Also updates the stale xfail reason text on test_the_real_hook_traces_past_the_bootstrap to describe the fix and point at the new test class, while leaving the xfail itself in place (strict=False, scoped to sys.platform == "win32", raises=AssertionError) since there is no Windows runner available in this environment to confirm it flips to XPASS -- that confirmation has to come from the next windows-latest CI run.

New tests

tests/test_trace_not_swallowed_690.py::TestCtxBufferSkippedUnderTrace, using the plugin-promo systemMessage JSON as a cross-platform-observable proxy for whether the buffer engaged:

  • test_positive_control_promo_shows_without_trace -- must-fire control: untraced, buffer writable, promo-eligible fixture -> systemMessage present.
  • test_buffer_skipped_under_bash_x -- the actual guard: bash -x active -> systemMessage absent.
  • test_buffer_skipped_under_remember_trace -- REMEMBER_TRACE=1 -> systemMessage absent, the same explicit opt-out bootstrap-dirs.sh already honours.

Testing

RED (scripts/session-start-hook.sh reverted, new tests kept): pytest tests/test_trace_not_swallowed_690.py -k TestCtxBufferSkippedUnderTrace -q --no-cov -> 2 failed, 1 passed (positive control passed; both guard tests failed against the old code).

GREEN: pytest tests/test_trace_not_swallowed_690.py -q --no-cov -> 10 passed, 1 skipped (pre-existing, unrelated BASH_XTRACEFD-version skip). pytest tests/test_plugin_promo_574.py tests/test_session_start_fork_tax_665.py tests/test_session_start_spawn_reduction_679.py -q --no-cov -> 27 passed, no regressions in neighbouring session-start-hook.sh suites. Full pytest (the whole test_command) was not run locally, narrowed per policy to touched files plus the guard-derived set.

Closes #712

[AI-generated]

…n active trace

`bash -x scripts/session-start-hook.sh` exits 0 with zero stdout bytes on
windows-latest (round 2 of #690), while the six neighbouring
bootstrap-dirs.sh trace tests in test_trace_not_swallowed_690.py pass on the
same runner. What's unique to this hook is its own `_REMEMBER_CTX_FILE`
buffer/fd-swap (exec 3>&1 / exec > file / exec 1>&3 3>&- spanning ~600 lines
of traced execution) -- a plausible site for Git Bash's fd emulation to lose
bytes under -x, with no Windows runner available to say which half.

Rather than debug that interaction blind, the buffer is now skipped entirely
whenever an xtrace or REMEMBER_TRACE=1 is active, the same way
bootstrap-dirs.sh already stands its own fd 2 redirect down for a trace an
operator deliberately started. The fallback path (print live, no promo) is
the exact one test_unwritable_ctx_buffer_does_not_burn_the_cooldown already
proves safe for an unwritable tmp/.

Adds TestCtxBufferSkippedUnderTrace, which pins the new guard directly using
the promo systemMessage as a cross-platform observable -- it does not need a
Windows runner to prove the buffer was skipped. Leaves the xfail on
test_the_real_hook_traces_past_the_bootstrap in place (strict=False) until a
real windows-latest run confirms this and flips it to XPASS.

Co-Authored-By: Max <noreply>
… not the fd-swap

PR #733's own windows-latest CI run (the first real evidence #712 ever had)
surfaced the actual mechanism: tests/test_trace_not_swallowed_690.py invoked
the real hook as `bash [..., str(SESSION_START)]`. On Windows, pathlib's
plain str() on a WindowsPath renders pure backslashes with no forward slash
at all, and session-start-hook.sh's own self-location
(`_HOOK_DIR="${BASH_SOURCE[0]%/*}"`) has no fallback for that shape: the
pattern strip is a no-op, `_HOOK_DIR` becomes ".", and
`source "./resolve-paths.sh"` fails from whatever cwd pytest happened to be
in -- resolve-paths.sh's own `|| exit 0` then exits clean with nothing
printed. That is exactly the symptom #712 was filed for.

This is the same test-harness bug test_windows_native_hook_cwd_448.py hit
and fixed once already (its own docstring documents it), and the same
convention tests/test_hooks_json.py established for exactly this reason --
this file's own real-hook invocation had simply not been updated to follow
it. Fixed by using SESSION_START.as_posix() at all four call sites in the
file (the pre-existing test_the_real_hook_traces_past_the_bootstrap and the
three new tests this issue added). Production is unaffected either way,
since hooks.json always invokes via a literal
${CLAUDE_PLUGIN_ROOT}/scripts/... shape that guarantees at least one forward
slash regardless of platform.

The previous commit's fd-swap/trace-skip guard in session-start-hook.sh
remains in place -- it is a harmless, precedent-matching defensive
improvement (mirrors bootstrap-dirs.sh's own fd 2 guard) -- but given this
finding, it was very likely not the actual cause of the originally observed
symptom; the path bug above is. Updated the xfail reason text and the
changelog fragment to say so plainly rather than let the earlier,
speculative theory stand uncorrected.

Local run (all four `.as_posix()` sites are no-ops on macOS/Linux, so this
does not change local behaviour): tests/test_trace_not_swallowed_690.py and
tests/test_plugin_promo_574.py both green (24 passed, 1 legitimate skip).

Co-Authored-By: Max <noreply>
@fdaviddpt

Copy link
Copy Markdown
Contributor Author

Follow-up commit 22833e7 pushed after CI went red on windows-latest (job 105918977745).

The failing leg was this PR's own new positive control (test_positive_control_promo_shows_without_trace), which failed with assert "systemMessage" in '' and the job's own stderr showing session-start-hook.sh: line 213: ./resolve-paths.sh: No such file or directory.

Root cause: the test invoked the real hook as bash [..., str(SESSION_START)]. On Windows, plain str() on a WindowsPath renders a pure-backslash path with no forward slash at all. session-start-hook.sh's self-location logic (_HOOK_DIR="${BASH_SOURCE[0]%/*}") has no fallback for that shape, so with zero / in BASH_SOURCE[0] the strip is a no-op and _HOOK_DIR becomes . -- source "$_HOOK_DIR/resolve-paths.sh" then fails from whatever directory pytest happens to be running in, and resolve-paths.sh's own || exit 0 swallows that failure silently. This is very likely also the exact original #712 symptom (bash -x session-start-hook.sh exiting 0 with zero stdout bytes) -- a test/harness path-normalization bug, not a production bug (hooks.json always invokes via a literal path with at least one forward slash regardless of platform).

This is a known bug class in this repo: tests/test_windows_native_hook_cwd_448.py documents hitting and fixing the identical thing, and tests/test_hooks_json.py establishes .as_posix() as the standing convention. All four bash [..., str(SESSION_START)] call sites in tests/test_trace_not_swallowed_690.py now use .as_posix() instead.

The original fd-swap/trace guard from 16598ee is kept as a harmless, precedent-matching improvement (mirrors bootstrap-dirs.sh's existing convention), but the path-normalization fix is now believed to be the actual root cause of the CI symptom. changelog.d/712.fixed.md updated to describe both. Not independently reproduced on a Windows runner -- REASONED, confirmed by the next windows-latest CI run rather than locally.

Note: this branch's own CI may still show unrelated red from #734 (a stale _SANCTIONED_DIVERGENCE entry on main at 7ba7609, tracked separately) until that lands -- unrelated to this fix.

[AI-generated]

… own borrowed fixtures

PR #733's windows-latest CI (all 4 legs, 3.9-3.12) confirmed the previous
commit's path-normalization fix works end to end: the hook now runs to
completion and prints its normal plain-text context
("=== REMEMBER ===...") on every leg. That is real, load-bearing evidence
the actual #712 defect (session-start-hook.sh's own self-location failing
on an unnormalized Windows path) is fixed.

It also surfaced a narrower, separate gap: TestCtxBufferSkippedUnderTrace
borrows its fixtures (_env, _payload, _store, _write_installed) from
tests/test_plugin_promo_574.py, whose own module-level pytestmark already
declares "bash hook subprocess + POSIX semantics -- not portable to Windows
runners". The reused fixtures carried no equivalent guard, so this class now
runs on windows-latest and hits the exact wall its source already flagged:
the plugin-promo systemMessage wrapper never fires there (most likely
because _env()'s HOME/CLAUDE_PROJECT_DIR/REMEMBER_DIR are raw str(Path)
rather than normalized, and something downstream -- installed-plugin
detection, most likely -- then reads as "cannot tell" and suppresses the
promo by design, #574 decision 3).

Making the whole plugin-promo subsystem Windows-portable is out of scope
for #712 -- #574's own authors already declined to do it -- so this adds
the same win32 skipif to TestCtxBufferSkippedUnderTrace that its borrowed
fixtures already imply, rather than claiming coverage it cannot back. The
guard the class pins (session-start-hook.sh's fd-swap/trace skip) is
unaffected and remains verified on macOS/Linux; it was already described as
a harmless, likely-unnecessary defensive improvement rather than the actual
#712 fix.

Local run (skip only fires on win32, no local behaviour change):
tests/test_trace_not_swallowed_690.py + tests/test_plugin_promo_574.py both
green, 24 passed / 1 legitimate skip.

Co-Authored-By: Max <noreply>
@fdaviddpt
fdaviddpt merged commit 5657b01 into main Sep 19, 2026
16 checks passed
@fdaviddpt
fdaviddpt deleted the fix/712 branch September 19, 2026 17:13
fdaviddpt added a commit that referenced this pull request Sep 19, 2026
…st XPASSes on a fresh main run (closes #690 for real) (#737)

* fix(#736): remove the strict=False xfail marker now that windows-latest XPASSes on a fresh main run

PR #733 (#712) left the marker in place because no windows-latest CI
evidence existed at review time. A fresh main-based run since then
(commit 5657b01, run 35457414864) shows
test_the_real_hook_traces_past_the_bootstrap passing plainly on all four
windows-latest legs, independent of that PR's own run -- the
re-derivation #690 asked for before trusting a strict=False XPASS as
permanent. Closes #690 for real.

Co-Authored-By: Max <noreply>

* fix(#736): note in the 712 fragment that the xfail marker it references is gone

Self-review (Explore) flagged that changelog.d/712.fixed.md still asserted
"the xfail... stays in place, strict=False, until a real windows-latest run
confirms" -- true when written, false as of the previous commit in this same
branch, which removes that exact marker. Both fragments are still unreleased,
so the fold into CHANGELOG.md would otherwise ship a contradiction. Appends a
short follow-up note rather than editing the original claim, matching the
fragment's own existing "Follow-up:" convention.

Co-Authored-By: Max <noreply>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bash -x session-start-hook.sh produces zero stdout bytes on windows-latest (round 2 of #690)

1 participant