refactor: one claude call site (fixes CC 2.x max-turns #98/#100 + mcp drift #94)#101
Merged
Merged
Conversation
added 3 commits
June 18, 2026 18:09
CC 2.x counts prompt-delivery as turn 1, so --max-turns 1 exits error_max_turns before the model replies — save-session.sh aborts at the exit-1 guard and no memory is ever written. A user Stop hook eats a further turn (#100). Introduce MAX_TURNS=${REMEMBER_MAX_TURNS:-4} and use it at both claude -p call sites; default 4 clears both cases with margin and stays overridable. Co-Authored-By: Max <noreply>
…aiku The claude -p invocation lived in TWO places (save-session.sh inlined it twice, pipeline/haiku.py had call_haiku) and had already drifted: haiku.py was missing --mcp-config/--strict-mcp-config (#94), and the CC-2.x max-turns break (#98/#100) could be half-fixed because there were two call sites. Unify on pipeline/haiku.py as the single owner: - haiku.py: add --mcp-config/--strict-mcp-config (closes #94 drift); max-turns configurable via REMEMBER_MAX_TURNS with validation (_resolve_max_turns, default 4; bad/0/negative/non-numeric -> 4). - shell.py: new `call-haiku` subcommand (invoke + parse in one) sharing the emit contract with parse-haiku via _emit_haiku_result. - save-session.sh: both Haiku calls (main + NDC) delegate to call-haiku; the inline claude -p blocks and the dead bash MAX_TURNS logic are gone. Main call uses `|| { ... }` so a failure is handled under set -e, not swallowed by the ERR trap at the assignment. Tests: max-turns default/override/invalid + mcp flags (test_haiku.py); call-haiku success + error-exit (test_shell.py); save-session delegation contract (test_save_session_max_turns.py rewritten). 416 passed; end-to-end smoke-tested with a stubbed claude binary (success + failure paths). Co-Authored-By: Max <noreply>
…urns cap) Self-review (Sonnet) of the unification found real issues: - NDC timeout regression: cmd_call_haiku hardwired call_haiku's 120s default, but NDC compresses a whole now.md and the sibling consolidate path uses 180s — a large now.md would silently time out. call-haiku now takes a timeout arg; save-session.sh passes 180 for NDC (120 stays for the per-session summary). - open(prompt_file) was outside the try: a missing prompt file raised an uncaught FileNotFoundError → traceback to STDOUT (which bash captures as the shell-var payload) with an empty stderr log. Moved inside try; catch OSError too → clean stderr diagnostic + exit 1, empty stdout. - _resolve_max_turns: added an upper bound (MAX_ALLOWED_TURNS=20) so an absurd REMEMBER_MAX_TURNS is bounded, and normalize leading zeros (007 -> 7). - NDC bash: capture NDC_EXIT=$? immediately (was a fragile bare $? check). - Docstrings list call-haiku. Tests: max-turns cap + normalize (test_haiku.py); missing-prompt-file clean exit + timeout passthrough (test_shell.py). 423 passed; smoke-tested call-haiku with a stubbed claude (timeout arg + missing-file paths). Co-Authored-By: Max <noreply>
fdaviddpt
force-pushed
the
fix/max-turns-cc2x-turn-counting
branch
from
d2b09b1 to
ba935c8
Compare
fdaviddpt
changed the title
This was referenced Jun 18, 2026
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
The Haiku summarizer's
claude -pinvocation lived in two places —save-session.shinlined it twice (main + NDC), andpipeline/haiku.pyhadcall_haiku()— and they'd already drifted: haiku.py was missing--mcp-config/--strict-mcp-config(#94), and the CC-2.x--max-turns 1break (#98/#100) could be half-fixed because there were two call sites. This unifies onhaiku.pyas the single owner.What changed
haiku.py— the one claude call site. Adds--mcp-config/--strict-mcp-config(closes Haiku call robustness: save-session main call lacks timeout + has dead error handler under set -e; haiku.py omits --strict-mcp-config (consolidation 180s timeouts) #94 drift).--max-turnsconfigurable viaREMEMBER_MAX_TURNS(_resolve_max_turns, default 4, validated to[1,20], leading zeros normalized). CC 2.x counts prompt-delivery as turn 1, so a cap of 1 exitserror_max_turnsbefore the model replies.shell.py— newcall-haikusubcommand (invoke + parse in one), sharing the emit contract withparse-haikuvia_emit_haiku_result. Forwards atimeout;open()inside the try so a missing prompt file fails clean (stderr + exit 1, no traceback to stdout).save-session.sh— both Haiku calls delegate tocall-haiku; the inlineclaude -pblocks and dead bashMAX_TURNSlogic are gone. Main path uses|| { ... }so failures are handled underset -e(not swallowed by the ERR trap at the assignment); NDC passestimeout=180(compresses a whole now.md) and captures its exit immediately.Tests
test_haiku.py— max-turns default/override/invalid/cap/normalize, mcp flags.test_shell.py—call-haikusuccess, claude-error exit, missing-file clean exit, timeout passthrough.test_save_session_max_turns.py— rewritten to the delegation contract (no inlineclaude -p, 2 delegations).claudebinary (success / failure / timeout-arg / missing-file paths). Self-reviewed adversarially (Sonnet) — that pass caught the NDC timeout regression, now fixed.🤖 Generated by Max