ci(pyright): adopt basic-mode type check over src/bmad_loop#264
Conversation
Stage 1 of #245 (assessment F-4): no type checker existed, so annotation drift was invisible and the refactor program's code moves lost a cheap safety net. Land a basic-mode pyright gate. Spike: pyright 1.1.411 in basic mode over src/bmad_loop (excluding data/), imports resolved against the uv venv, surfaced 89 errors — well under the ~200 threshold, so the gate lands here rather than a per-module rollout. Config: [tool.pyright] in pyproject.toml (typeCheckingMode=basic, include=src/bmad_loop, exclude=src/bmad_loop/data, pythonVersion=3.11, venvPath/.venv). A `typecheck (pyright)` CI job mirrors the existing job structure: install the project (so optional-extra imports resolve), then run the pinned `uvx pyright@1.1.411`. Cheap fixes (real annotation drift the gate caught, no runtime change): - runsetup: stories/sweep engine params were typed `type[Engine]` though the injected classes are StoriesEngine/SweepEngine — retype them, which clears the "No parameter named spec_folder/triage_adapter/..." errors. - cli/engine: `dict[str, object]` profile maps retyped to `dict[str, CLIProfile]` (+ a return annotation on `_worktree_profiles`). - adapters/generic: bare class annotations declare the host attributes the `_ResultFileMixin`/`_DevSynthesisMixin` mixins read (no runtime effect). The remainder are targeted per-line suppressions, each with a one-line reason: heterogeneous `**dict` splats (pyright unions the values), a few narrowing gaps pyright can't prove (try/finally result, coupled sentinels, boundary `object` types), and stdlib/3rd-party stub shapes (TextIO.reconfigure, signal handler, a pyte deque subclass). No runtime behavior changes. Full suite green (the 2 skill-sync failures are pre-existing local install-copy drift, unrelated).
WalkthroughAdds a Pyright basic-mode configuration and CI job, then applies targeted type annotations, type-only imports, and suppressions across core, composition, adapter, plugin, and TUI code. ChangesPyright adoption
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/bmad_loop/verify.py`:
- Around line 644-654: Update worktree_add to enforce that base is present
before the create=True _git worktree add call; reject the missing-base case
explicitly or invoke git without the optional base argument when absent, and
remove the pyright suppression. Preserve valid caller-supplied base handling.
🪄 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
Run ID: 552bdf47-9b07-4f1f-a438-1c48c18ac10c
📒 Files selected for processing (14)
.github/workflows/ci.ymlpyproject.tomlsrc/bmad_loop/adapters/generic.pysrc/bmad_loop/cli.pysrc/bmad_loop/diagnostics.pysrc/bmad_loop/engine.pysrc/bmad_loop/machine.pysrc/bmad_loop/plugins/registry.pysrc/bmad_loop/runsetup.pysrc/bmad_loop/tui/app.pysrc/bmad_loop/tui/data.pysrc/bmad_loop/tui/screens/settings_screen.pysrc/bmad_loop/tui/settings.pysrc/bmad_loop/verify.py
The pyright gate flagged that `worktree_add(create=True)` passed a possibly-None `base` straight into `_git`, where a None arg would raise in subprocess. It was unreachable today (the sole production caller, open_unit_workspace, types `base: str`), but the signature was dishonest and a future caller omitting `base` would crash. Mirror git's own CLI (`git worktree add -b <branch> <path> [<base>]`): append the start-point only when supplied, else let git cut from HEAD. This drops the reportArgumentType suppression, makes the args type-clean, and turns a latent crash into well-defined behavior. Regression test added.
|
@coderabbitai the two pre-merge-check warnings ("Linked Issues" / "Out of Scope Changes") on The Those are comments — zero runtime impact. No |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
file="src/bmad_loop/machine.py"
base="49242d7"
ast-grep outline "$file" --match emit_document --view expanded
printf '\n--- diff: %s...HEAD (%s) ---\n' "$base" "$file"
git diff --unified=12 "$base"...HEAD -- "$file"
printf '\n--- base context ---\n'
git show "${base}:${file}" | nl -ba | sed -n '/def emit_document/,/^[^[:space:]]/p'
printf '\n--- HEAD context ---\n'
sed -n '/def emit_document/,/^[^[:space:]]/p' "$file" | nl -baLength of output: 2801
I’m withdrawing both pre-merge warnings for 🐇🔎 ✏️ Learnings added
|
1 participant
Closes #245 — Stage 1 (basic mode) of assessment finding F-4.
Why
No type checker existed anywhere (no pyright/mypy config in
pyproject.toml,.trunk/, or CI). Annotations are pervasive but were unverified, so annotation drift was invisible and the wider refactor program's code moves had no cheap safety net. This lands a basic-mode pyright gate that fences those moves.Spike result (the go/no-go)
Ran pyright 1.1.411 (verified current via npm/pip) in basic mode over
src/bmad_loop, excludingdata/, with imports resolved against the uv-managed venv:89 is well under the ~200 threshold, so the gate lands here rather than proposing a per-module rollout. (Notably, only 1 error fell in a Stage-2 "pure" module —
machine.py— a good omen for Stage 2.)What's in the change
Config —
[tool.pyright]inpyproject.toml:typeCheckingMode = "basic",include = ["src/bmad_loop"],exclude = ["src/bmad_loop/data"],pythonVersion = "3.11"(therequires-pythonfloor),venvPath="."/venv=".venv"so optional-extra imports (textual, pyte, httpx, …) resolve instead of reading as missing.CI — a
typecheck (pyright)job mirroring the existing job structure: checkout → install uv →uv sync --locked --all-extras(populates.venvfor import resolution) → pinneduvx pyright@1.1.411.Cheap fixes — real annotation drift the gate caught, all type-only:
runsetup: the injectedstories_engine_cls/sweep_engine_clswere typedtype[Engine], but the real classes areStoriesEngine/SweepEngine. Retyping them clears the "No parameter namedspec_folder/triage_adapter/…" errors — the gate literally caught the annotations lying about the DI seam.cli/engine:dict[str, object]profile maps retyped todict[str, CLIProfile](+ a-> list[CLIProfile]return annotation on_worktree_profiles).adapters/generic: bare class annotations declare the host attributes the_ResultFileMixin/_DevSynthesisMixinmixins read (bare annotations = no runtime effect).Targeted suppressions — the rest, each with a one-line reason. Categories: heterogeneous
**dictsplats (pyright unions the values and flags every kwarg); narrowing gaps pyright can't prove (thetry/finallyresultis non-None past the finally, coupledfirst_ts/last_tssentinels,object-typed values at widget boundaries); and stdlib/third-party stub shapes (TextIO.reconfigure, the signal-handler dict, a pytedequesubclass that adds.dropped).Notes
verify.worktree_add(create=True)passed a possibly-Nonebasestraight into_git, whereNonewould raise in subprocess (unreachable today — the sole caller typesbase: str— but the signature was dishonest). Now mirrors git's own CLI: the start-point is appended only when supplied, else git cuts from HEAD. Drops that one suppression and adds a regression test.test_module_skills_syncfailures are pre-existing local install-copy drift (module_version 0.8.1vs canonical0.9.0in gitignored.agents/.claude), unrelated to this change and absent on a fresh CI checkout.Stage 2 (per-module strict on the pure modules) is a separate follow-up that requires this merged.
Summary by CodeRabbit
Bug Fixes
Quality Improvements