Skip to content

agents-consilium: fix Windows/MSYS compatibility (CRLF, fcntl, os.getuid, destructive pid_alive, POSIX-only Popen) - #7

Open
av-frox wants to merge 3 commits into
CodeAlive-AI:mainfrom
av-frox:fix/windows-msys-compat
Open

agents-consilium: fix Windows/MSYS compatibility (CRLF, fcntl, os.getuid, destructive pid_alive, POSIX-only Popen)#7
av-frox wants to merge 3 commits into
CodeAlive-AI:mainfrom
av-frox:fix/windows-msys-compat

Conversation

@av-frox

@av-frox av-frox commented Aug 27, 2026

Copy link
Copy Markdown

Fixes #5. @rodion-m — as offered in the issue.

agents-consilium assumed a POSIX host in several places. Under Git Bash / MSYS2 with a native Windows python3, review code failed end-to-end and the whole steer package failed at import time. This PR makes those paths cross-platform. POSIX behavior is unchanged — every new branch is gated on os.name == "nt" or on fcntl being importable.

Shell / encoding

File Fix
scripts/lib/config.sh _cfg_python() now forces sys.stdout.reconfigure(newline='\n'). Windows python3 print() emits \r\n; consumed line-by-line, every id carried a trailing CR, so lookups failed with unknown agent id: grok\r. This is the primary break in #5.
scripts/lib/common.sh export PYTHONIOENCODING=utf-8, so embedded non-ASCII (e.g. ) does not raise UnicodeEncodeError on a console-codepage stdout.
scripts/lib/discovery-pass.sh, scripts/lib/judge-runner.sh Backend resolution passes the config path and agent id through the environment instead of splicing them into the Python source string, plus the same newline reconfigure. Removes a shell-injection surface as well as the CRLF hazard.
scripts/lib/dedup-findings.py Explicit encoding="utf-8" on read and write (newline="\n" on write). Previously went through the process codepage and silently mojibaked UTF-8 agent output (—).
scripts/lib/steer/adapters/{claude,opencode}.py Decode backend output as UTF-8 with errors="replace".

steer package (previously unimportable on Windows)

File Fix
steer/util.py, steer/mailbox.py Guarded import fcntl with msvcrt-based lock_exclusive() / unlock() fallbacks. Unguarded module-scope import fcntl took down the entire package.
steer/util.py pid_alive() used os.kill(pid, 0). On Windows CPython maps every signal except CTRL_C_EVENT/CTRL_BREAK_EVENT onto TerminateProcess, so the liveness probe killed the process it was asking about. Replaced with OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION) + GetExitCodeProcess. kill_process_group() falls back to taskkill /F /T (os.getpgid/os.killpg don't exist on Windows).
steer/registry.py Added current_uid() (os.getuid() on POSIX, 0 on Windows) and a %LOCALAPPDATA% registry root; os.uname() / os.getuid() raised AttributeError before any cross-platform logic ran.
steer/jsonrpc.py start_new_session is POSIX-only → CREATE_NEW_PROCESS_GROUP on Windows. argv[0] now resolves through shutil.which(), so npm-style .cmd/.bat shims and shebang scripts stop failing with WinError 193.
steer/supervisor.py Tolerate a missing os.setsid.

CI (second commit — drop it if you'd rather not take it)

.github/workflows/consilium-smoke.yml runs on ubuntu-latest and windows-latest (Git Bash) and:

  • imports every steer module,
  • asserts pid_alive() reports a live child without terminating it, and rejects a bogus pid,
  • asserts config_enabled_agents emits LF-only ids whose backends resolve via config_get_field,
  • runs consilium --help.

Those checks would have caught the import-time failures and the CRLF break immediately. The repo has no workflows today, so this is the whole CI surface — happy to drop the commit or narrow it to windows-latest only.

Verification

Run on Windows 11 / Git Bash (MSYS2) with native Windows Python against this branch:

steer imports OK: C:\Users\...\AppData\Local\agents-consilium\steer
pid_alive(self): True      pid_alive(999999): False
config_enabled_agents | cat -A:
  grok$
  opencode-go-kimi-k3$      # no ^M
  claude-opus$
  claude-fable$
grok -> grok-build          opencode-go-kimi-k3 -> opencode
claude-opus -> claude-code  claude-fable -> claude-code
consilium --help  exit=0

Note on the process point from #5

The skill ships vendored (no .git in the skill dir), so a local patch is silently discarded on every update with no diff or changelog entry. That is how these exact defects came back after being fixed locally in July. The CI job above is the cheapest guard against a repeat; a line in SKILL.md/README stating the Windows support level would also help users know what to expect. Glad to add that wording if you tell me which level you want to commit to.

The skill assumed a POSIX host in several places, which broke it entirely
under Git Bash / MSYS2 with a native Windows python3. Refs CodeAlive-AI#5.

Shell/encoding:
- config.sh: force sys.stdout.reconfigure(newline='\n') in _cfg_python.
  Native Windows python3 emits \r\n from print(), so every config lookup
  consumed line-by-line matched against ids with a trailing CR and failed
  with "unknown agent id: <id>\r".
- common.sh: export PYTHONIOENCODING=utf-8 so embedded non-ASCII output
  does not raise UnicodeEncodeError on a non-UTF-8 console codepage.
- discovery-pass.sh, judge-runner.sh: resolve the backend by passing the
  config path and agent id through the environment instead of splicing
  them into the python source, and reconfigure stdout newlines. Removes a
  shell-injection surface as well as the CRLF hazard.
- dedup-findings.py: read/write findings with an explicit encoding="utf-8"
  (and newline="\n" on write) instead of the process codepage, which
  silently mojibaked UTF-8 agent output.
- steer/adapters/{claude,opencode}.py: decode backend output as UTF-8 with
  errors="replace".

steer package (previously failed at import time on Windows):
- util.py: guard "import fcntl" and add msvcrt-based lock_exclusive()/
  unlock() fallbacks; mailbox.py now uses those helpers.
- util.py: pid_alive() used os.kill(pid, 0), which on Windows maps onto
  TerminateProcess and killed the process it was probing. Use
  OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION) + GetExitCodeProcess.
  kill_process_group() falls back to taskkill /F /T, since os.getpgid and
  os.killpg do not exist on Windows.
- registry.py: add current_uid() (os.getuid() on POSIX, 0 on Windows) and
  a %LOCALAPPDATA% registry root, so os.uname()/os.getuid() no longer
  raise AttributeError before the cross-platform logic runs.
- jsonrpc.py: start_new_session is POSIX-only; use CREATE_NEW_PROCESS_GROUP
  on Windows. Resolve argv[0] via shutil.which() so npm-style .cmd/.bat
  shims and shebang scripts do not fail with WinError 193.
- supervisor.py: tolerate a missing os.setsid.

POSIX behavior is unchanged: every branch is gated on os.name == "nt" or on
the availability of fcntl.
Runs on ubuntu-latest and windows-latest (Git Bash). Imports every steer
module, asserts pid_alive() probes without terminating its target, and
checks that config_enabled_agents emits LF-only agent ids whose backends
resolve. These are the checks that would have caught the import-time and
CRLF regressions in CodeAlive-AI#5.
Follow-up to the previous two commits, from an external review pass.

Correctness:
- util.py: msvcrt.locking() locks a byte range starting at the CURRENT file
  offset, unlike flock() which locks the whole file. lock_exclusive() locked
  at wherever the "a+" handle happened to sit (EOF for a non-empty lock file)
  while unlock() always seeked to 0, so the two named different bytes and the
  lock leaked. Both helpers now seek to 0. lock_exclusive() also retried every
  OSError forever; it now uses LK_NBLCK, re-raises anything that is not
  EACCES contention, and fails with TimeoutError after a bounded wait.
  flock_exclusive() delegates to the same primitive instead of repeating it.
- util.py: declare argtypes/restype for OpenProcess/GetExitCodeProcess/
  CloseHandle. Without them ctypes assumes c_int and truncates a 64-bit
  HANDLE. pid_alive() no longer reports a live process as dead when the query
  is denied (ERROR_ACCESS_DENIED) or when GetExitCodeProcess fails.
- adapters/{claude,opencode}.py passed start_new_session=True directly to
  Popen, which raises on Windows — the previous commit only fixed the
  JSON-RPC child. Added util.detached_popen_kwargs() and routed all three
  call sites through it.
- terminal_guard.py was entirely POSIX-only (signal.SIGHUP, os.killpg,
  start_new_session) and is invoked for every opencode run, so that backend
  still failed on Windows. Added a taskkill-based process-tree termination
  path, a platform-aware termination signal set (SIGBREAK for SIGHUP), and
  the shared detach kwargs.

POSIX invariance:
- Dropped errors="replace" from the Popen calls in jsonrpc.py and both
  adapters. encoding="utf-8" alone fixes the Windows codepage decode; adding
  errors="replace" would also have relaxed decoding on POSIX, where invalid
  bytes previously raised.

Security:
- kill_process_group() and terminal_guard.py invoke taskkill through an
  absolute %SystemRoot%\System32 path. Resolving the bare name would let a
  taskkill.exe in the working directory run during cancellation.
- kill_process_group() now waits for the tree to actually go away instead of
  assuming taskkill succeeded.

CI:
- Added steps for the lock round-trip (on a deliberately non-empty lock
  file), the detach kwargs, and terminal_guard stopping a completed backend.
  pid_alive is also checked against a reaped pid.

Metadata:
- Patch bump to 9.8.1 in plugin.json and marketplace.json, per AGENTS.md.
@av-frox

av-frox commented Aug 27, 2026

Copy link
Copy Markdown
Author

Pushed a third commit (Harden the Windows paths after review) after an external review pass. It fixes two real bugs I had introduced, one Windows break I had missed, and one correctness point about the POSIX claim in this description.

Bugs in my own patch

  • msvcrt.locking() locks a byte range from the current file offset, unlike flock() which locks the whole file. lock_exclusive() locked wherever the "a+" handle sat (EOF for a non-empty lock file) while unlock() always seeked to 0 — different bytes, so the lock leaked. Both seek to 0 now, flock_exclusive() delegates to the same primitive, and acquisition uses LK_NBLCK with a bounded retry that re-raises anything that is not EACCES contention instead of looping forever.
  • The ctypes calls had no argtypes/restype, so the pointer-sized HANDLE was treated as c_int. Declared all three signatures. pid_alive() also treated ERROR_ACCESS_DENIED as "dead", which would mark a live supervisor failed; access-denied and a failed GetExitCodeProcess now count as alive.

Windows break I had missed

  • adapters/claude.py and adapters/opencode.py pass start_new_session=True straight to Popen — my earlier commit only fixed the JSON-RPC child. Added util.detached_popen_kwargs() and routed all three call sites through it.
  • terminal_guard.py was entirely POSIX-only (signal.SIGHUP, os.killpg, start_new_session) and backend_run.sh invokes it for every opencode run, so that backend still failed on Windows even with the rest of the fixes. It now has a taskkill-based process-tree path and a platform-aware termination signal set. Verified end-to-end: the guard forwards the terminal event and stops a backend that would otherwise sleep 60s, in ~4s.

Correction to this description

"POSIX behavior is unchanged" was too strong as written. errors="replace" on the Popen calls would also have relaxed decoding on POSIX, where invalid bytes previously raised. Dropped it — encoding="utf-8" alone is what fixes the Windows codepage decode. PYTHONIOENCODING=utf-8 in common.sh is still exported unconditionally; on POSIX that is a no-op in practice (PEP 538/540 already coerce a C locale to UTF-8), and making it deterministic is the point.

Also

  • taskkill is invoked through an absolute %SystemRoot%\System32 path in both places; resolving the bare name would let a taskkill.exe in the working directory run during cancellation. kill_process_group() now waits for the tree to actually go away instead of assuming success.
  • Patch bump to 9.8.1 in plugin.json and marketplace.json, per AGENTS.md. Tagging/release is yours to make.
  • CI grew steps for the lock round-trip (on a deliberately non-empty file — the exact bug above), the detach kwargs, and terminal_guard.

One thing I did not touch: scripts/tests/run.sh fails on Windows at the "Argv safety" block because the harness hands an MSYS /tmp/... path to native Windows Python. I confirmed this fails identically on unmodified origin/main, so it is pre-existing and not a regression from this PR — but it does mean the suite cannot gate Windows today. Happy to add a bash scripts/tests/run.sh step on ubuntu-latest to guard POSIX, or to fix the harness path handling, if you want either in scope.

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.

agents-consilium: recent update reintroduced multiple Windows/MSYS compatibility breaks (fcntl, os.getuid, CRLF-in-python3-c, POSIX-only Popen)

1 participant