fix: small independent contracts + pyright canary (#433, 6C) - #439
Merged
Conversation
added 4 commits
August 2, 2026 16:27
`UnicodeDecodeError` is a `ValueError`, not an `OSError`, so a `policy.toml` or `_bmad/bmm/config.yaml` saved in UTF-16 or latin-1 escaped every `except (PolicyError, OSError)` / `except BmadConfigError` handler in the codebase — and those handlers exist precisely to degrade to defaults rather than take the process down. It reached `cli._configure_mux`, which runs before argument dispatch on every command, and `DashboardScreen.__init__`, which runs before the TUI can draw anything. `policy.load` and `bmadconfig.load_paths` convert it to their own typed errors, which fixes every handler at once instead of asking each to name a second exception type it has no other reason to know about. The command-level tests go through `machine_json`, not an rc assertion: `cli.main`'s bare `except Exception` backstop already returns 1, so an rc-only test is green with the conversion reverted. Forward-ported from the 0.9.1 hotfix (`dac373b`).
…433) `_stories_defaults` caught tomlkit's `ParseError` around a `policy.load` call, but `policy.py` parses with `tomllib` — the branch has been dead since it was written. `policy.load`'s new decode conversion makes the remaining `(PolicyError, OSError)` pair total for that call. Honest note: no test can distinguish the two tuples, so this is a redundant-code deletion rather than an unpinned claim — restoring `ParseError` reddens nothing (measured against the whole suite on both the 3.13 and 3.14 lanes). The import stays; `action_settings` catches a genuine tomlkit `ParseError` from `PolicyDoc.load`.
A journal record's `spec` field carries the customer's feature name. A bare basename is identifier-shaped, so `scrub_json` waved it through verbatim, and the egress backstop could not rescue it — it only repairs values already in the legend, and an unrouted value never enters one. `spec` is routed to an alias namespace of its OWN, not `story`: the epic lookup is keyed on `ns == "story"`, so a filename aliased there always misses and renders as an epic-less `story-<hex>` — indistinguishable from a story key whose epic could not be resolved. Routing alone would have been half a fix. The producers disagree on shape — `engine.py` journals `str(spec_path)` (absolute), `stories_engine.py` journals `task.spec_file`, which `StoryTask` persists worktree-relative — so one spec drew two aliases in a single dump, defeating the correlation these fields are aliased rather than dropped to preserve, and parked an absolute home path in the local `--legend` file. `_alias_input` reduces the value to its basename first, splitting on BOTH separators because a journal written on Windows is routinely diagnosed on POSIX, where `PurePath` treats a backslash as an ordinary character. The `or value` fallback covers a trailing separator, whose empty tail `alias()` would pass through unaliased. Forward-ported from the 0.9.1 hotfix (`e3dba25`, `2edd753`).
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
WalkthroughThe change adds typed handling for undecodable policy and BMAD configuration files. It also adds dedicated, basename-normalized pseudonyms for diagnostic spec values across POSIX and Windows path formats. ChangesInvalid UTF-8 handling
Diagnostic spec pseudonymization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_policy.py`:
- Around line 326-337: Update test_non_utf8_file_raises_policy_error to use the
project sandbox fixture instead of tmp_path, and create policy.toml beneath
project.project while preserving the existing invalid-UTF-8 bytes and
PolicyError assertion.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f18a3ef3-932c-47f2-90c5-4266aa600eb3
📒 Files selected for processing (11)
CHANGELOG.mdsrc/bmad_loop/bmadconfig.pysrc/bmad_loop/diagnostics.pysrc/bmad_loop/policy.pysrc/bmad_loop/tui/app.pysrc/bmad_loop/tui/screens/dashboard.pytests/test_bmadconfig.pytests/test_cli.pytests/test_diagnostics.pytests/test_policy.pytests/test_tui_app.py
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.
Forward-port of the 0.9.1 hotfix to
main, sub-phase 6C. Part of #433.Three unrelated one-file robustness fixes, landed together because they are independent of the
rest of the program and clear noise from the sub-phases that follow. The other reason they go
first is the pyright canary — see below.
What lands
1. An undecodable
policy.toml/config.yamlis reported, not a crash.UnicodeDecodeErroris a
ValueError, not anOSError, so a config saved in UTF-16 or latin-1 escaped everyexcept (PolicyError, OSError)/except BmadConfigErrorhandler in the codebase — and thosehandlers exist precisely to degrade to defaults rather than take the process down. It reached
cli._configure_mux, which runs before argument dispatch on every command, andDashboardScreen.__init__, which runs before the TUI can draw anything: the app died atconstruction, not at a keypress.
policy.loadandbmadconfig.load_pathsconvert it to their owntyped errors, fixing every handler at once instead of asking each to name a second exception type
it has no other reason to know about.
2.
_stories_defaultsdrops the unreachableParseErrorleg.policy.pyparses withtomllib; tomlkit'sParseErrorhas never been reachable from that call. With the conversionabove,
(PolicyError, OSError)is total there. The import stays —action_settingscatches agenuine tomlkit
ParseErrorfromPolicyDoc.load.3.
diagnosealiases the spec name, and gives one spec one alias. A journal record'sspecfield carries the customer's feature name. A bare basename is identifier-shaped, so
scrub_jsonwaved it through verbatim, and the egress backstop could not rescue it — it only repairs
values already in the legend, and an unrouted value never enters one.
specgets a namespace of its own, notstory: the epic lookup is keyed onns == "story",so a filename aliased there always misses and renders as an epic-less
story-<hex>,indistinguishable from a story key whose epic could not be resolved.
Routing alone would have been half a fix, and on the path shape strictly worse than today. The
producers disagree on shape —
engine.pyjournalsstr(spec_path)(absolute),stories_engine.pyjournals
task.spec_file, whichStoryTaskpersists worktree-relative — so one spec drew twoaliases in a single dump, defeating the correlation these fields are aliased rather than dropped to
preserve, and parked an absolute home path in the local
--legendfile._alias_inputreduces thevalue to its basename first, splitting on both separators, because a journal written on Windows
is routinely diagnosed on POSIX, where
PurePathtreats a backslash as an ordinary character andthe normalization would silently no-op. The
or valuefallback covers a trailing separator, whoseempty tail
alias()passes through unaliased — the event would render a blankspecand lose itsonly reference, with no
<redacted:str>marker to show anything was removed.The pyright canary — the reason 6C is early
policy.pyis in pyright's strict list, and none of this code had ever been typechecked:pyrightis configured and gated onmain, and was not configured at all onrelease/0.9.x.Landing the 8-line strict change here means any 0.9.x→main strict surprise surfaces on 8 lines
rather than confounded with
model.py's ~110 in 6I.Baseline extracted from
origin/mainbefore any edit: 0 errors, 0 warnings, 0 informations.After: 0 / 0 / 0 — zero delta, not zero-compared-to-nothing. The gate was proven to still bite
by appending
def _gate_probe(x: int) -> str: return xtopolicy.py, confirming it reported(
reportUnusedFunction+reportReturnType), and reverting.pyright==1.1.411is untouched —its pin comment declares it the single pin keeping CI and local identical, and a forward-port is
not the change that renegotiates a gate.
Ablation matrix — whole suite, both lanes
origin/mainbaseline: 3929 collected (3901 passed / 24 skipped / 4 pre-existingtest_module_skills_syncfailures from local dev-box skill drift, identical on a clean checkout).This branch: 3938 collected (+9 tests). Every row below was run against the whole suite,
__pycache__cleared between flips, and the source digest verified byte-identical after eachrevert. The 3.14 lane is a separate checkout with its own
--all-extrasvenv, import sentinelconfirmed, and 3938 collected there too.
policy.loaddecode conversion reverted--json, both TUIbmadconfig.load_pathsreverted--json"spec": "spec"routing deletedno_canary_leaks+routing_gap_repaired+ 3 intest_cli.pyspecaliased understoryfullmatchand both correlation legs_alias_inputcall deleted[\\/]→/or valuefallback dropped_stories_defaultsnarrowed toexcept OSErrorParseErrorrestored to the tupleRow I is reported, not hidden. Restoring the removed leg reddens nothing on either lane,
because a wider-but-unreachable except tuple is behaviorally identical. Naming which kind of
finding that is: redundant code, not an unpinned claim — no test can distinguish the two
tuples, and one that appeared to would be reading a coincidence. (A single red did appear in row I
on the first 3.13 run,
test_decision_modal_scrolls_when_content_long; re-running the whole rowgave 0. It is the known modal-scroll flake, not an effect of the ablation.)
Two rows worth reading because they did not move:
test_no_repairs_on_fully_routed_runstaysgreen under row C — the backstop cannot repair a value that was never routed, which is exactly why
the three new tests exist and why the backstop could not substitute for them. And row C's red in
test_routing_gap_is_repaired_end_to_endwas not predicted: it fails withLEAK after repair: 'AcmeVaultRotation', a second independent canary witness of the same leak.Test notes
The command-level decode tests go through
machine_json, not an rc assertion —cli.main's bareexcept Exceptionbackstop already returns 1, so an rc-only test is green with the conversionreverted.
SPEC_NAMEdeliberately does not embedSTORY_KEY: a name like1.2-Acme….mdwould berescued by the story key already in the legend, and the fixture could not express the failure.
The Windows witness asserts on strings only, never the filesystem, so it runs unguarded on the
POSIX lanes — that is the only reason the separator divergence is covered at all.
Both TUI tests carry a precondition asserting the same policy text, when decodable, would have
produced a different result — so the degraded-default assertions cannot pass on an inert fixture.
Port adaptations
_findings_by_checkdoes not exist onmain; the tests use main's inlinenext(f for f in doc["findings"] if …)idiom rather than importing a helper that isn't there.0.9.x seeds the fixture with journal kind
spec-deferrals-harvested, which does not exist onmain— it arrives with the harvest slice in a later sub-phase. The fixture uses kinds mainactually emits (
checkpoint-pause,spec-status-reconciled). The routing is keyed on the fieldname, not the kind, so nothing is weakened; the comment naming the producers was re-provenanced to
main accordingly.
Gates
pytest3910 passed / 24 skipped ·pyright0 errors / 0 warnings / 0 informations(baseline delta zero) ·
trunk fmtand fulltrunk check --no-fixclean · full ablation matrix onthe 3.13 and 3.14 lanes.
The 4
tests/test_module_skills_sync.pyfailures are pre-existing local dev-box skill drift,present identically on a clean
origin/main, and are not reproduced in CI.Summary by CodeRabbit