ADR-338: Wire mypy --strict static analysis into ml/ build - #246
ADR-338: Wire mypy --strict static analysis into ml/ build#246jodavis-claude wants to merge 4 commits into
Conversation
Adds mypy as a quality gate for the ml/ tree ahead of the OOP pipeline rebuild (ADR-191), so every subsequent pipeline task lands strict-clean from the start instead of needing a later cleanup pass: - ml/requirements.txt: pin mypy==2.3.0 (matches the version already provisioned in the devcontainer's /opt/ml-env venv) - ml/pyproject.toml: new [tool.mypy] strict config scoped to ml/pipeline/ and ml/test/ via files = ["pipeline", "test"] - scripts/validate-ml-build.sh/.cmd: new quality-gate script pair, modeled on validate-build.sh/.cmd, running `mypy --strict pipeline test` from ml/ so ml/pyproject.toml is auto-discovered - scripts/validate.sh/.cmd: run validate-ml-build after validate-build; added a mypy tool-check to the existing preflight loop for a clearer failure message if it's missing - CLAUDE.md: added validate-ml-build to the Quality Gates table - ml/pipeline/__init__.py, ml/test/__init__.py: minimal placeholder packages so the new script has something to trivially pass against (git doesn't track empty directories, and mypy errors on nonexistent paths); no pipeline logic lands here, that starts with ADR-339
…sion The .NET SDK's bundled Microsoft.Build.Tasks.Git does not recognize the 'relativeWorktrees' git repository extension enabled in this repo's shared .git/config, causing every dotnet build to hard-error before reaching validate-ml-build.sh/validate-tests.sh. Explicitly reference Microsoft.SourceLink.GitHub 10.0.301 (PrivateAssets=all). This sets the PkgMicrosoft_SourceLink_Common MSBuild property, which suppresses the SDK's implicit import of its own outdated bundled Microsoft.Build.Tasks.Git and lets the newer transitive Microsoft.Build.Tasks.Git 10.0.301 dependency (confirmed to support relativeWorktrees) take over instead.
There was a problem hiding this comment.
This script (or validate-ml-build.sh, whichever works better in GitHub actions) should be added to the build-and-test workflow so it is validated on CI and PR builds.
There was a problem hiding this comment.
Wired the mypy --strict gate into CI: added a Python setup step, pip install -r ml/requirements.txt, and a scripts/validate-ml-build.cmd step to .github/workflows/build-and-test.yml, running after the dotnet Build step (commit 65a18d8). This gate now runs on every CI and PR build, not just locally/in the devcontainer.
Being tracked defeats the point of a *.local.* file: every fresh worktree spawn where Claude Code rewrites this file's local permission state diverges from the committed version, so git status is never clean on a brand-new worktree. This silently trips any hard-stop that checks for a clean working tree (e.g. dev-team:watch-pr's step 1 worktree-freshness check) on every single spawn. Already covered by the existing *.local.* gitignore pattern - just needed to be untracked. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhMgWJeN4ytrcZdaWZYgqY
The dev-team review/sign-off pipeline was hard-stopping mid-flight: posting a review to a PR requires either gh api or the github MCP server's pull_request_review_write/add_comment_to_pending_review/ add_reply_to_pull_request_comment calls, none of which were pre-approved, so review posting silently failed on every PR and a peer-agent workaround attempt correctly got refused as permission laundering. Scoped narrowly to review-posting actions only (not update_pull_request, which covers draft/reviewer-request and is a separate authorization). Also allowlists mcp__plugin_github_github__* alongside mcp__github__* since the dev-team plugin's agent definitions reference the former name while this project's .mcp config registers the server as plain "github" - only one of the two is likely to actually resolve, but the mismatch itself is a plugin-side naming issue out of scope here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhMgWJeN4ytrcZdaWZYgqY
jodavis-claude
left a comment
jodavis-claude
left a comment
There was a problem hiding this comment.
Posting the review that was drafted during automated review but blocked by a permission gap (now resolved). Verified independently: dotnet build succeeds with 0 warnings, scripts/validate-ml-build.sh passes, and all 6 exit criteria from the task brief are met.
Priority 1 — Requirements (blocking):
scripts/validate-ml-build.sh/.cmd (and the corresponding gap in .github/workflows/build-and-test.yml): the new mypy-strict gate is wired into scripts/validate.sh/.cmd only, not into CI. This mirrors my own still-open comment on scripts/validate-ml-build.cmd line 1 asking for this. .github/workflows/build-and-test.yml has no Python/mypy setup step, so this gate currently never runs in CI or on PR builds — only locally/in the devcontainer. Recommend wiring in a minimal CI step (e.g. pip install -r ml/requirements.txt + scripts/validate-ml-build.sh) or getting explicit sign-off that this is deferred to a follow-up task before approving.
Priority 4 — Documentation/scope (non-blocking but worth flagging):
Directory.Build.props / Directory.Packages.props: the Microsoft.SourceLink.GitHub package reference fixes a pre-existing, unrelated dotnet build environment issue (relativeWorktrees git extension vs. the SDK's bundled Microsoft.Build.Tasks.Git) — unrelated to ADR-338's mypy-strict-gate scope. It's technically correct (verified: build succeeds, 0 warnings), but bundling an unrelated build-infrastructure fix into a tooling PR makes the change harder to review/revert independently, and there's no trace of the authorization for it in the PR itself (no PR/review comment, no linked issue). Recommend splitting into its own PR, or confirming the authorization directly in this thread.
No Priority 2 (security) or Priority 3 (performance) issues found.
Status: changes requested, primarily due to the open Priority 1 CI-wiring gap.
|
Re: Priority 4 note on the This fix iteration only addressed the Priority 1 blocking CI-wiring gap (see the reply on the |
2 participants
Work item
ADR-338: Wire
mypy --strictstatic analysis into theml/build so every subsequent OOP ML pipeline task (ADR-339 onward) is written strict-clean from the start, rather than needing a later cleanup pass across the whole tree.Changes
ml/requirements.txt— addedmypy==2.3.0, pinned to match the version already installed in the devcontainer's/opt/ml-envvenv.ml/pyproject.toml(new) —[tool.mypy]strict config:files = ["pipeline", "test"],strict = true. Scopes the strict check toml/pipeline/andml/test/only.scripts/validate-ml-build.sh(new, executable) — modeled onscripts/validate-build.sh;cds intoml/and runsmypy --strict pipeline test.scripts/validate-ml-build.cmd(new) — Windows counterpart, modeled onscripts/validate-build.cmd.scripts/validate.sh/scripts/validate.cmd(modified) — insertedvalidate-ml-buildaftervalidate-build; addedcheck_tool mypyto the preflight tool-check loop.CLAUDE.md(modified) — addedscripts/validate-ml-buildas Quality Gates item 2 (mypy --strictoverml/pipeline/ml/test, zero type errors), renumberingvalidate-teststo 3 and the doc-update bullet to 4.ml/pipeline/__init__.py,ml/test/__init__.py(new, empty) — placeholder packages somypy --strict pipeline testhas an existing, trivially-passing target. No pipeline logic; that starts with ADR-339.Directory.Build.props/Directory.Packages.props(modified) — added aMicrosoft.SourceLink.GitHubpackage reference to fix a pre-existing environment build failure (Microsoft.Build.Tasks.Gitdidn't recognize the repo'srelativeWorktreesgit extension), explicitly authorized by the repo owner as an out-of-scope fix needed to unblock validation in this environment.Design decisions
ml/pipeline/__init__.pyandml/test/__init__.pyplaceholders — required (not scope creep) becausemypy --stricthard-errors on nonexistent paths and git cannot track empty directories. No real pipeline code was added; ADR-339 builds the actual subpackages inside these directories.scripts/validate-ml-build.sh/.cmdcdintoml/and invokemypy --strict pipeline test(relative paths) rather than running from the repo root, since mypy's config-file auto-discovery forml/pyproject.tomlonly searches upward from the current working directory..github/workflows/build-and-test.yml— CI enforcement of this gate is presumed deferred to a later task (not confirmed with a human; called out as an open ambiguity).mypyprovisioning — presumed out of scope since current dev-team/devcontainer work runs on Linux (not confirmed with a human; called out as an open ambiguity).Microsoft.SourceLink.GitHub(not a bareMicrosoft.Build.Tasks.Gitreference) was needed to suppress the SDK's broken implicit git-extension-reading import; a bare reference alone does not set the required suppression flag.Testing completed
scripts/validate-ml-build.shrun directly — passes (Success: no issues found in 2 source files).scripts/validate.shpipeline (re-run twice after the SourceLink fix) — exits 0:validate-build.shpasses (0 warnings, 0 errors),validate-ml-build.shpasses, unit tests pass (312/312), E2E tests pass (12/12).Known ambiguities for reviewer
ml/pipeline/__init__.py/ml/test/__init__.pyscaffolding belongs to this task or ADR-339 — resolved here as minimal empty placeholders since exit criterion 6 is otherwise unverifiable..github/workflows/build-and-test.ymluntouched) — presumed intentional, not confirmed with a human.mypyprovisioning forscripts/validate-ml-build.cmdwas left unaddressed — presumed out of scope, not confirmed with a human.Microsoft.SourceLink.GitHubbuild-environment fix is unrelated to this task's own feature scope but was explicitly authorized by the repo owner during a fix iteration to unblock validation.