fix: pin UTF-8 stdout in resolve_config so Windows cp1252 doesn't crash - #2693
fix: pin UTF-8 stdout in resolve_config so Windows cp1252 doesn't crash#2693armelhbobdad wants to merge 3 commits into
Conversation
The full-config dump writes ensure_ascii=False JSON to a stdout still bound to the platform default. On Windows that is cp1252, which cannot encode the emoji icons carried by the shipped agent configs, so the script raises UnicodeEncodeError and exits having produced no output. Adopts write_json_stdout() from the sibling resolve_customization.py, and adds the stdout regression test the suite was missing - it fails against the unpatched script. Fixes bmad-code-org#2682 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Greptile SummaryThe PR makes the configuration resolver’s JSON output and its context-script consumers consistently use UTF-8, preventing Windows locale encoding and decoding failures.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/scripts/resolve_config.py | Adds a guarded UTF-8 stdout configuration before emitting non-ASCII JSON. |
| src/scripts/context.py | Explicitly decodes resolver subprocess output as UTF-8, completing the producer-consumer encoding contract. |
| src/bmm-skills/plan/bmad-project-context/scripts/context.py | Mirrors the explicit UTF-8 subprocess decoding fix in the distributed skill copy. |
| src/scripts/tests/test_resolve_config.py | Adds regression tests for cp1252 producer output and the complete UTF-8 consumer round trip. |
Sequence Diagram
sequenceDiagram
participant Context as context.py
participant Resolver as resolve_config.py
Context->>Resolver: Run with project root
Resolver->>Resolver: Reconfigure stdout as UTF-8
Resolver-->>Context: UTF-8 JSON bytes
Context->>Context: Decode explicitly as UTF-8
Context->>Context: Parse JSON configuration
Reviews (3): Last reviewed commit: "test: cover the context.py consumer of r..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| """Pin stdout to UTF-8 — a Windows cp1252 default cannot encode emoji icons.""" | ||
| reconfigure = getattr(sys.stdout, "reconfigure", None) | ||
| if reconfigure is not None: | ||
| reconfigure(encoding="utf-8") |
There was a problem hiding this comment.
When either context.py resolver runs on Windows with a cp1252 locale, this change emits UTF-8 while the parent still uses subprocess.run(..., text=True) without an explicit encoding, causing emoji output to raise UnicodeDecodeError during capture instead of loading the configuration.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/scripts/resolve_config.py
Line: 41
Comment:
**Locale decoding breaks UTF-8 output**
When either `context.py` resolver runs on Windows with a cp1252 locale, this change emits UTF-8 while the parent still uses `subprocess.run(..., text=True)` without an explicit encoding, causing emoji output to raise `UnicodeDecodeError` during capture instead of loading the configuration.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
📝 WalkthroughWalkthrough
ChangesConfig stdout encoding
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/scripts/resolve_config.py`:
- Around line 37-42: Update the relevant documentation for the resolve_config
command to state that JSON output uses UTF-8 encoding and preserves non-ASCII
characters rather than escaping them. Keep the implementation in
write_json_stdout unchanged.
- Around line 37-42: Document the UTF-8 stdout contract for resolve_config.py in
the BMad documentation, including that output is JSON with raw emoji and
external consumers must decode it as UTF-8. Find the existing consumer that
invokes resolve_config with text=True but omits encoding="utf-8", and update
that invocation to specify UTF-8 while preserving its existing behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2795eae5-737a-490c-b375-1c808e647c69
📒 Files selected for processing (2)
src/scripts/resolve_config.pysrc/scripts/tests/test_resolve_config.py
| def write_json_stdout(output) -> None: | ||
| """Pin stdout to UTF-8 — a Windows cp1252 default cannot encode emoji icons.""" | ||
| reconfigure = getattr(sys.stdout, "reconfigure", None) | ||
| if reconfigure is not None: | ||
| reconfigure(encoding="utf-8") | ||
| sys.stdout.write(json.dumps(output, indent=2, ensure_ascii=False) + "\n") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add documentation for the changed JSON output contract.
This change makes resolve_config.py emit UTF-8 bytes with raw non-ASCII characters. The PR includes no corresponding change under docs/. Update the relevant command or output documentation to state this behavior.
As per path instructions, “src/**: Source file changed. Check whether documentation under docs/ needs a corresponding update — new features, changed behavior, renamed concepts, altered CLI flags, or modified configuration options should all be reflected in the relevant doc pages.”
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 41-41: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output, indent=2, ensure_ascii=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/scripts/resolve_config.py` around lines 37 - 42, Update the relevant
documentation for the resolve_config command to state that JSON output uses
UTF-8 encoding and preserves non-ASCII characters rather than escaping them.
Keep the implementation in write_json_stdout unchanged.
Source: Path instructions
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 --glob '!src/scripts/resolve_config.py' \
'resolve_config(?:\.py)?|subprocess\.(run|Popen|check_output)|spawnSync|encoding\s*=|decode\(' .Repository: bmad-code-org/BMAD-METHOD
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== resolve_config.py =="
cat -n src/scripts/resolve_config.py
echo
echo "== resolve_config references (source/docs/tests only, max 200 lines) =="
rg -n -C 4 'resolve_config(?:\.py)?|resolve_customization\.py|uv run .*resolve_config|python .*resolve_config' --glob 'docs/**' --glob 'src/**' --glob 'tests/**' --glob '!*.png' . | head -n 240Repository: bmad-code-org/BMAD-METHOD
Length of output: 35519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== context.py config resolver section =="
sed -n '70,90p' src/scripts/context.py | cat -n
echo
echo "== persona/resolver helpers =="
sed -n '1,90p' src/core-skills/bmad-forge-idea/scripts/resolve_personas.py | cat -n
sed -n '48,72p' src/core-skills/bmad-party-mode/scripts/resolve_party.py | cat -n
echo
echo "== tests around resolve_config stdout =="
sed -n '1,180p' src/scripts/tests/test_resolve_config.py | cat -n
echo
echo "== exact resolve_config docs references =="
rg -n 'resolve_config\.py|resolver (returns|emits|outputs|output)|UTF-8|utf.?8|text=True|encoding|subprocess' src/core-skills src/scripts src/core-skills --glob '!**/*.png' --glob '!**/__pycache__/**' | head -n 120Repository: bmad-code-org/BMAD-METHOD
Length of output: 24276
Document the resolve_config stdout contract.
ResolveConfig now pins resolve_config.py output to UTF-8 JSON with raw emoji. Update BMad docs that call the resolved config so external consumers know they must decode stdout as UTF-8; one existing consumer still omits encoding="utf-8" when using text=True.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 41-41: use jsonify instead of json.dumps for JSON output
Context: json.dumps(output, indent=2, ensure_ascii=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/scripts/resolve_config.py` around lines 37 - 42, Document the UTF-8
stdout contract for resolve_config.py in the BMad documentation, including that
output is JSON with raw emoji and external consumers must decode it as UTF-8.
Find the existing consumer that invokes resolve_config with text=True but omits
encoding="utf-8", and update that invocation to specify UTF-8 while preserving
its existing behavior.
Both copies of context.py capture the resolver with text=True and no encoding, so the parent decodes with the platform locale. Now that the child reliably emits UTF-8, a cp1252 parent raises UnicodeDecodeError on the reader thread - which does not propagate: stdout comes back None with returncode 0, slipping past the returncode guard and dying as an uncaught TypeError in json.loads(None). Matches the same one-line fix already merged for resolve_party.py (bmad-code-org#2687) and resolve_personas.py (bmad-code-org#2688). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Good catch from Greptile — valid, and worse than described. Fixed in 62f2250. I reproduced it before changing anything. The failure is not a propagating Three-way repro on Windows 11 / Python 3.11, config carrying
So the middle row was a genuine regression this PR introduced: it turned a silent-but-graceful fallback into a crash. Worth stating plainly. The bot said "two The fix is the same one-liner already merged for the sibling consumers — No test added for the consumer hop, matching the precedent of those two merged PRs —
🤖 Generated with Claude Code |
Locks the encoding= on context.py's capture. Omitting it makes the decode fall back to the locale, which on Windows raises on the reader thread and surfaces as stdout=None with returncode 0 - past the returncode guard, then an uncaught TypeError in json.loads(None). Runs the real _installed_resolver_config under warn_default_encoding with EncodingWarning escalated to an error, so the omission fails on UTF-8 platforms too rather than only on a cp1252 console. Lives in test_resolve_config.py because test:renderer runs it; test_context.py is wired into no npm script and already fails on main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Added the consumer test in 3d769a0. Where it lives. Not in Why it is not a cp1252 test. The existing precedent ( Instead the driver runs under It exercises the real Verified as a tripwire in both directions:
🤖 Generated with Claude Code |
brain.py prints two kinds of arbitrary user text: --extra overlay techniques (customize.toml additional_techniques) and, on stderr, the technique name echoed back by `show NAME` when it is not found. Either can carry a character the platform default cannot encode, and print() then raises UnicodeEncodeError. The shipped catalog is cp1252-safe (its only non-ASCII is U+2014), so this is an overlay/argv path, not a default-catalog crash. pin_utf8() passes errors= through rather than letting it default: reconfigure(encoding=...) alone resets the handler to strict, which would silently downgrade stderr's POSIX default of backslashreplace and turn a diagnostic about an undecodable path into a traceback. Thanks to @armelhbobdad for catching that on bmad-code-org#2693. Four regression tests, all four red against the unpatched script. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Author of #2578 here. Thanks for the flag on my PR, and for scoping this one to the producer — that is the right call, and I'd rather this land than mine. I've rebased #2578 onto current Two things I reproduced on Windows 11 while doing that, both of which back up your review notes. Your >>> s = io.TextIOWrapper(io.BytesIO(), encoding="ascii", errors="backslashreplace")
>>> s.reconfigure(encoding="utf-8"); (s.encoding, s.errors)
('utf-8', 'strict')
>>> t.reconfigure(encoding="utf-8", errors=t.errors); (t.encoding, t.errors)
('utf-8', 'backslashreplace')My Your reader-thread finding reproduces — and it also lands on the two consumers that already merged. #2687 and #2688 took the Same failure you found in And thank you for the credit note in the description — genuinely appreciated. |
2 participants
What
Pins
resolve_config.py's stdout to UTF-8 before the JSON dump, and adds the stdout regression test the suite was missing.Why
The full-config dump writes
ensure_ascii=FalseJSON to a stdout still bound to the platform default. On Windows that is cp1252, which cannot encode the emoji icons in the shipped agent configs, so the script raisesUnicodeEncodeErrorand exits having produced no output.Fixes #2682
That issue was auto-closed by #2687, but #2687 changed only the consumer side (
encoding="utf-8"on party mode'ssubprocess.run). That governs how the parent decodes the child's bytes; it does not change the child's stdout encoding, so the producer still raises before writing anything and_run_jsonstill sees a non-zero exit. Verified still reproducible onmain.How
write_json_stdout()verbatim from the siblingresolve_customization.py, which already guards the identical write (fixed in fix: write customization JSON as UTF-8 #2414).test_writes_emoji_json_when_stdout_encoding_is_cp1252, mirroring the existing test of the same name intest_resolve_customization.py. The issue noted the suite never exercised the stdout path — it does now.Testing
npm run test:renderer→ 11 tests, OK. The new test fails against the unpatched script (confirmed by reverting the fix and re-running), so it is a real tripwire.Note on #2578
@aranellaeth's #2578 proposed this same
resolve_config.pyguard back on 2026-07-12 and deserves the credit. Its consumer hunks have since been merged piecemeal (#2687 party mode, #2688 forge-idea), leaving the producer fix — the part that actually closes #2682 — outstanding. This PR is deliberately scoped to just that piece plus its test, so it can land independently; close it in favour of #2578 if you would rather merge that whole.One review note for whichever lands:
reconfigure(encoding="utf-8")withouterrors=also resets the handler tostrict. Harmless for stdout (this PR), but #2578 also reconfigures stderr inbrain.py, which silently downgrades POSIX's defaultbackslashreplace— a surrogateescaped path in an error message would turn a clean diagnostic into a traceback. Passingerrors=stream.errorspreserves it.🤖 Generated with Claude Code