harden: follow-ups for merged contributor PRs #90 and #99#102
Merged
Conversation
added 2 commits
June 18, 2026 18:20
…ollow-up) #99 made _remember_date load-critical in session-start-hook.sh, which sources log.sh with stderr suppressed. A silent sourcing failure would leave _remember_date undefined and the hook would limp on with an empty TODAY. Add a command -v guard that exits 127 with a clear diagnostic — 127 stays inside the degraded-env contract (0, 127) the read-only test relies on. Covered by a red/green test that stubs log.sh without the function and asserts the guard's diagnostic in hook-errors.log. Co-Authored-By: Max <noreply>
…low-up) #90's core invariant — on ConsolidationSkipped, emit CONSOLIDATION_STATUS=skip and NO output-path vars so run-consolidation.sh writes nothing and retires no staging files — had no automated coverage. Add a test that patches consolidate() to raise ConsolidationSkipped and asserts the skip status with no RECENT_OUT/ARCHIVE_OUT/STAGING_PATHS_FILE in the output. Co-Authored-By: Max <noreply>
fdaviddpt
pushed a commit
that referenced
this pull request
Jun 18, 2026
…ch-up PATH ordering can't fix the WSL-bash problem: Windows CreateProcess searches System32 BEFORE PATH, so subprocess.run(['bash']) always hits System32\bash.exe (the WSL launcher). Add _find_bash() (plain 'bash' on POSIX, explicit Git-for-Windows bash.exe on Windows, mirroring test_hooks_json._find_git_bash) and invoke it everywhere; module skipif when Git Bash is absent. Revert the ineffective GITHUB_PATH workflow shim. Combined with the earlier MSYS path normalization, the three modules now run on Windows (TestDispatchOwnershipChecks stays skipped — NTFS has no POSIX mode bits). Also bring CHANGELOG [Unreleased] current — it was untouched across this session's 7 merged PRs (#90/#93/#99/#101/#102/#103/#104). Added Fixed/Security entries with issue/PR links + contributor credit, plus the #79 and encoding test-module notes. Co-Authored-By: Max <noreply>
fdaviddpt
added a commit
that referenced
this pull request
Jun 19, 2026
* ci: run shell-subprocess tests on Windows via Git Bash on PATH (#79) The Windows runner's PATH bash resolves to the WSL launcher (no distro installed) → every subprocess.run(['bash', ...]) returned the UTF-16 'no distributions' error, so #78 skipped these modules on win32 — shipping no Windows coverage for code paths that DO run there (Claude Code invokes hooks via Git Bash). - tests.yml: prepend C:\Program Files\Git\bin to PATH on the Windows row so bash resolves to Git Bash. - Drop the module-level skipif(win32) from test_log_sh, test_migration, test_security_fixes. - Keep TestDispatchOwnershipChecks skipped on win32: POSIX ownership + world-writable (0o777) semantics genuinely don't map to NTFS (Git Bash fakes mode bits) — the dispatch() guard is a no-op there. If a now-unskipped test surfaces a real Windows bug (CRLF, path form), it gets a targeted fix/follow-up, not a re-skip. Co-Authored-By: Max <noreply> * ci: prepend Git Bash in-step + MSYS-normalize paths for Windows shell tests (#79) Two real Windows layers surfaced once the skips were removed: - bash still resolved to the WSL launcher (System32\bash.exe) — appending to GITHUB_PATH didn't override it. Prepend C:\Program Files\Git\bin to $env:PATH in the same pwsh step that runs pytest, so subprocess.run(['bash']) children inherit Git Bash. - the tests interpolate Windows backslash/drive paths into bash scripts (source C:\..., export DIR=C:\...), which bash mangles (\a/\c escapes, spaces). Add _bash_path() (C:\x -> /c/x, backslashes -> /) and apply+quote it at every path injected into a bash script or path-valued env var. No-op on POSIX. RCE-payload paths inside safe_eval heredocs left literal (intentional). Co-Authored-By: Max <noreply> * ci: invoke bash by explicit Git path on Windows (#79) + CHANGELOG catch-up PATH ordering can't fix the WSL-bash problem: Windows CreateProcess searches System32 BEFORE PATH, so subprocess.run(['bash']) always hits System32\bash.exe (the WSL launcher). Add _find_bash() (plain 'bash' on POSIX, explicit Git-for-Windows bash.exe on Windows, mirroring test_hooks_json._find_git_bash) and invoke it everywhere; module skipif when Git Bash is absent. Revert the ineffective GITHUB_PATH workflow shim. Combined with the earlier MSYS path normalization, the three modules now run on Windows (TestDispatchOwnershipChecks stays skipped — NTFS has no POSIX mode bits). Also bring CHANGELOG [Unreleased] current — it was untouched across this session's 7 merged PRs (#90/#93/#99/#101/#102/#103/#104). Added Fixed/Security entries with issue/PR links + contributor credit, plus the #79 and encoding test-module notes. Co-Authored-By: Max <noreply> * test: use forward-slash drive paths (C:/x) for bash+Windows-Python crossing (#79) The MSYS form (/c/x) works for Git Bash but the test scripts also invoke the Windows python3 (jq fallback's $PYTHON, migration's REMEMBER_DIR -> Path()), and Windows Python can't open('/c/x'). Forward-slash drive form (C:/x) is accepted by BOTH Git Bash and Windows Python. Switch _bash_path to emit C:/x, and route the migration data_dir config through it so REMEMBER_DIR round-trips to Python Path(). No-op on POSIX. Co-Authored-By: Max <noreply> * test: instrument migration test to capture Windows runner state (#79, temp) * fix: recognize Windows drive paths (C:/, C:\) as absolute in lib-memory-dir.sh (#79) External-mode data_dir resolution only treated /... and ~... as absolute, so a Windows drive path (C:/Users/.../mem/{slug}) fell through to the relative branch and got prepended to PROJECT_DIR — REMEMBER_DIR became proj/C:/.../{{slug}} (path doubling + slug never substituted, since substitution lives in the absolute branch). Real Windows users with a C:/-form data_dir hit this. Add [A-Za-z]:/* and [A-Za-z]:\\* to the absolute case. Diagnosed from a Windows-CI state dump; instrumentation reverted. Co-Authored-By: Max <noreply> * docs(changelog): record the lib-memory-dir.sh Windows data_dir fix (#79) --------- Co-authored-by: Florian DAVID <fdavid@digital-village.fr>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Two review findings on the just-merged contributor PRs were correct but not blocking, so the PRs were merged as-is and the fixes split out here (keeping the original authors' history clean). Both are defensive/test-coverage items — no behavior change to the happy path.
#99 follow-up — fail loudly on a broken log.sh source
session-start-hook.shsourceslog.shwith2>/dev/null. #99 made_remember_dateload-critical there; a silent sourcing failure would leave it undefined and the hook would continue with an emptyTODAY. Added acommand -v _remember_dateguard that exits 127 (stays inside the degraded-env contract(0, 127)the read-only test relies on) with a clear diagnostic tohook-errors.log.#90 follow-up — cover the shell-level skip path
#90's key invariant (on
ConsolidationSkipped: emitCONSOLIDATION_STATUS=skip, no output-path vars, so nothing is written and no staging file is retired) had zero automated coverage. Added acmd_consolidatetest asserting exactly that.Tests
test_unsourceable_log_sh_fails_loudly_not_silently— red without the guard, green with (verified by neutering the guard).test_cmd_consolidate_skip_emits_status_and_no_output_paths.🤖 Generated by Max