feat(test): multi-language changed-line coverage providers#295
Open
e-jung wants to merge 11 commits into
Open
Conversation
Owner
|
Thanks for the contribution, @e-jung! One process note on this repo: PRs need to be raised through no-mistakes ( If working from a fork was the blocker, that's fixed as of v1.30.0 (#306). Per CONTRIBUTING.md: point I won't be merging PRs that bypass the gate going forward, but I'd genuinely love to land your work once it's re-raised. Thanks for understanding! 🙏 |
added 8 commits
July 3, 2026 03:53
After the test suite passes, mechanically verify every changed Go source
file has non-zero coverage on its *added* lines. This catches "changed X
but didn't test X" model-agnostically (no AI review needed) and closes the
file-level blind spot where a new function dropped into an already-tested
file passed because the file still showed >0% coverage.
runCoverageCheck parses added-line ranges from a tight `git diff
--unified=0 <base>..<head>` and intersects them with coverprofile blocks.
A finding fires when a changed file has any added line that is executable
but not exercised by a covered block. This subsumes file-level coverage:
a brand-new untested file has all lines added and uncovered, so it still
fires. Blank/comment added lines are ignored; the signal is an added line
in a zero-count block (or, for a fully uninstrumented file, any added
line). Finding id is `uncovered-changed-lines:<path>` with the uncovered
added-line count in the description.
- internal/pipeline/steps/coverage.go: parse `go test -coverprofile`, diff
added-line ranges, emit warning/ask-user findings.
- parseGoCoverProfileBlocks: structured blocks (line span + count) per file.
- parseAddedLineRanges / parseHunkAddedRange: per-file added-line ranges
from unified=0 hunks (the +c,d side).
- uncoveredChangedLineFindings: diff-intersection finding rule.
- internal/pipeline/steps/test.go: run the sub-step after tests pass at
every success return; gate via NO_MISTAKES_COVERAGE_CHECK (default on for
Go projects). Errors degrade to a logged no-op, never blocking the pipeline.
- internal/pipeline/steps/coverage_test.go: pure-logic + real Go module
integration tests; four diff-intersection scenarios (fully covered, fully
uncovered, partially covered, brand-new untested file) plus unit tests
for the new parsers.
- AGENTS.md: document the coverage-aware test step and changed-line semantics.
…d it Make the coverage-aware test step pluggable so JS/TS and Swift providers can be added later as isolated files that touch no shared code. No behavior change for Go. - coverage_provider.go: define coverageProvider interface (Name/Active/ CoverableChangedFiles/RunCoverage/ParseBlocks), the coverageProviders registry + registerCoverageProvider, namespaceFindings, and a shared toRepoRelPOSIX helper that normalizes native paths to the repo-relative POSIX keys git diff emits (encodes the kunchenguid#1 path-key invariant once, handles macOS /var vs /private/var symlink spellings). - coverage_go.go: goCoverageProvider{} implementing the interface, self- registering in init(). Relocates verbatim from coverage.go: goModulePath, parseGoCoverProfile[Blocks], parseCoverLoc, atoiBeforeDot, coverableChangedGoFiles, runGoCoverageProfile. Active() = go.mod present. - coverage.go: rewrite runCoverageCheck as the looping dispatcher (neutral diff plumbing once, then per-active-provider CoverableChangedFiles/RunCoverage/ ParseBlocks -> unchanged uncoveredChangedLineFindings core). Broaden coverageCheckEnabled: default ON when ANY registered provider is Active(); the explicit NO_MISTAKES_COVERAGE_CHECK override still wins. - Namespace finding IDs to uncovered-changed-lines:<lang>:<path> (insert <lang>: after the existing prefix so downstream TUI/filter prefix matching keeps working). test.go:123 (the single call site) is untouched; the dispatcher preserves the ([]Finding, string, error) signature. coverage_test.go passes unchanged.
Adds coverage_swift.go (one isolated file, self-registering in init(), no shared code touched) implementing the coverageProvider interface for Swift projects. Active only when both a Swift manifest (Package.swift or *.xcodeproj/*.xcworkspace) is present AND NM_SWIFT_SSH_HOST names the Mac build executor — no-mistakes runs on Linux with no Swift toolchain. Two build modes selected by NM_SWIFT_BUILD_MODE (default swiftpm): - swiftpm: swift test --enable-code-coverage + xcrun llvm-cov export, parses data[].files[].segments into blocks spanning [seg.line, nextSeg.line-1] (report §4.2B). - xcode: xcodebuild test -enableCodeCoverage + per-file xcrun xccov view --file --json, parses coveredLines/uncoveredLines into 1-line blocks (report §4.2A). Errors clearly when /Applications/Xcode.app is absent so the path degrades gracefully. RunCoverage uses exec.CommandContext(sctx.Ctx, "ssh", ...) and feeds the remote script to over stdin so the Mac's Homebrew/CLT tools are on PATH; cancellation kills the remote build. Remote flow syncs the head SHA via git fetch + checkout, refuses a dirty tree (never hard-resets), and traps /tmp/nm-* cleanup on exit. ParseBlocks dispatches by content: ===FILE: records route to the xccov parser; otherwise llvm-cov export JSON. Both relativize paths via the shared toRepoRelPOSIX so keys match git diff --name-only output. coverage_swift_test.go has fixture JSON for both parse maps (pure-parse unit tests, no Mac needed), plus Active/CoverableChangedFiles/script- shape tests. Live SSH e2e is not gated behind NM_SWIFT_SSH_HOST because the parser tests cover the contract; the SSH path was smoke-tested manually against the Mac (and surfaced a real limitation: a CLT-only Mac cannot run XCTest or Swift Testing — both frameworks ship with Xcode.app — so both modes effectively require Xcode.app for end-to-end collection). Documented in AGENTS.md.
Add jsCoverageProvider as the second language for the pluggable coverage
check. It self-registers in init() and touches no shared code — the whole
point of the Task-0 coverageProvider refactor.
- Active when package.json is at the worktree root.
- CoverableChangedFiles keeps *.{js,jsx,ts,tsx,mjs,cjs}, drops test files
(*.test.*/*.spec.*, __tests__/, {test,tests}/), applies ignore patterns,
drops deletions. Owns its own test-file rule.
- RunCoverage wraps the project's test runner under
`npx --yes c8@latest --reporter=json` (V8-native, preferred over nyc),
writes to a temp --reports-dir, and reads coverage-final.json (canonical
Istanbul format, not coverage-summary.json).
- ParseBlocks emits one coverBlock per statement (branches folded in for
tighter executable-line detection), keys relativized via toRepoRelPOSIX
so they are byte-identical to git diff --name-only output (kunchenguid#1 invariant).
- Runner detection priority: NM_JS_TEST_RUNNER env → commands.test →
package.json scripts.test (npm test) → node --test fallback.
File is coverage_jsts.go (not coverage_js.go) because _js is a Go GOOS
suffix (WebAssembly target); Go silently ignores the latter on non-js
GOOS. AGENTS.md updated with this gotcha and the JS provider specifics.
Verified: go test -race ./... green, make e2e green, gofmt + vet clean,
dogfooded via local binary against a /tmp JS fixture (coverage finding
fired for an uncovered changed file, run paused for approval).
…tive count, xcode gate)
Fixes five defects in the Swift coverage provider proven fatal or
misleading by live verification against a Mac fixture (scout report
nm-swift-live-x7):
1. Dirty-tree guard closed with `}` instead of `fi` → fatal bash syntax
error; the remote script never ran (FATAL, both modes).
2. swiftpm mode called `xcrun llvm-cov export --format=json` on the path
from `swift test --show-code-coverage-path` — doubly wrong:
`--format=json` is invalid on Xcode 26.5's llvm-cov (only text/html)
and the path is a JSON export, not a .profdata. Replaced with
`cat "$PROF"` (the export is already final) (FATAL, swiftpm).
3. EXIT trap's `cleanup() { ... || true; }` masked parse-time syntax
errors → SSH exited 0 with empty stdout. Dropped `|| true` (kept
`2>/dev/null` on the rm) so future typos propagate.
4. Path-key invariant violated: the Mac emits Mac-relative source paths
but ParseBlocks receives the LOCAL workDir → block keys were absolute
Mac paths, missed the coverable/added lookup, and the empty-blocks
fallback fired "all changed lines uncovered" (11 instead of 2).
Added a path bridge in RunCoverage: strings.ReplaceAll(raw,
remotePath, sctx.WorkDir) before returning (STRUCTURAL false positive).
5. xcode mode hardcoded `/Applications/Xcode.app` gate missed versioned
installs (e.g. /Applications/Xcode-26.5.0.app). Replaced with
`xcodebuild -version` which respects xcode-select (FATAL, xcode mode).
Tests added:
- Dirty-tree `fi` regression guard (bug kunchenguid#1).
- Path-bridge regression test (bug kunchenguid#4, highest value): proves Mac-path
JSON keys to local repo-relative after the RunCoverage rewrite.
- Dispatcher e2e (stub provider + real git repo): asserts both the
finding ID and the "N changed line(s)" count match ground truth
(2 uncovered executable, not all 6 added).
Live-verified end-to-end against the Mac fixture: emits
uncovered-changed-lines:swift:Sources/swift-cov-fixture/Calculator.swift
with "2 changed line(s)" (was "11" — the false positive).
Update the Swift provider description to match the fixed code: - swiftpm mode cats the JSON export from --show-code-coverage-path (was the invalid xcrun llvm-cov export --format=json) - xcode gate uses xcodebuild -version (respects xcode-select, works for versioned installs like /Applications/Xcode-26.5.0.app) - Document the path bridge: RunCoverage rewrites remotePath → sctx.WorkDir in the raw JSON so ParseBlocks keys match git diff --name-only output (the kunchenguid#1 correctness concern for any remote-coverage provider)
The three coverage providers spawned their coverage command via bare cmd.CombinedOutput() / cmd.Output(), which only reaps the direct child on context cancellation. On a clean exit (a green test run), a worker pool the command spawned - go test's compiled binary, c8/npx's node test workers, or a helper the ssh client left behind - survives the leader, reparents to init, and accumulates across runs until the host is out of memory and the OS OOM-kills the daemon (the "daemon crashed during execution" failure mode behind kunchenguid#357). Route each command through shellenv.ConfigureShellCommand + the matching reap helper so the whole process group is reaped on every exit path (clean exit, parse error, wait error, and cancellation): - go / jsts: shellenv.CombinedOutputShellCommand (drop-in for CombinedOutput; both consume combined stdout+stderr). - swift: shellenv.OutputShellCommand. Swift keeps stderr separate from the JSON stream, and OutputShellCommand reaps via RunShellCommand (which, unlike cmd.Output, does not populate (*exec.ExitError).Stderr), so stderr is now captured into an explicit buffer and read back on failure. Adds coverage_reap_unix_test.go: each provider gets a clean-exit reap regression (a fake go/npx/ssh backgrounds a detached grandchild and exits 0; the test asserts the grandchild is reaped once the provider returns), plus a cancel-path test for the Go provider. Each guard was verified to fail when the wiring is reverted and pass once restored.
e-jung
force-pushed
the
feat/multi-language-coverage
branch
from
ba192ee to
5d053ec
Compare
e-jung
changed the title
4 tasks
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
2 participants
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.
What Changed
coverageProviderregistry plus a language-neutral changed-line (diff-level) coverage check (runCoverageCheck) that runs after the test suite passes and reportsuncovered-changed-linesfindings, gated byNO_MISTAKES_COVERAGE_CHECK.c8→ Istanbulcoverage-final.json), and Swift (delegated to a Mac over SSH) providers behind the registry, each isolated in its own file so adding a language touches no shared code.shellenvso leaked grandchild worker pools are reaped on clean exit, and documents the new providers and their env vars (environment.md,pipeline-steps.md,AGENTS.md).Risk Assessment
--include=src/**which causes false-positive blocking findings for JS projects with non-src/layouts, but it only affects the JS path (the Go provider this repo dogfoods is correct), is gated behind package.json presence, and is disablable via NO_MISTAKES_COVERAGE_CHECK=0.Testing
Completed 2 recorded test checks.
Pipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
⏭️ **Review** - skipped
internal/pipeline/steps/coverage_jsts.go:94- The c8 invocation hardcodes--include=src/**, butCoverableChangedFilesaccepts accountable JS source files ANYWHERE in the repo (*.js/.jsx/.ts/.tsx/.mjs/.cjs in any directory). For any source file outsidesrc/(lib/, app/, server/, packages/<pkg>/, root), c8 omits it from coverage-final.json even when fully exercised by tests. ParseBlocks then yields NO blocks for that path, and the empty-blocks subsumption fallback inaddedLineExecutable(coverage.go:239,len(blocks)==0 -> executable) flags EVERY added line as uncovered, producing a spurious blockingask-userfinding (NeedsApproval=true, autoFixable=false). I confirmed this empirically: a coveredlib2.js(imported by a passing test) appears in coverage-final.json without--includebut is absent with--include=src/**. Since coverageCheckEnabled defaults ON whenever package.json is present, this fires false positives on every JS PR touching non-src/files for projects not following thesrc/convention. The coverable-filter file set and the include-filter file set are internally inconsistent; either drop the hardcoded include (let c8 report all loaded files), derive it from project structure, or make it configurable.⏭️ **Test** - skipped
internal/pipeline/steps/coverage.go- 16 changed line(s) have no test coverage — add a test that exercises the new codeinternal/pipeline/steps/coverage_go.go- 9 changed line(s) have no test coverage — add a test that exercises the new codeinternal/pipeline/steps/coverage_jsts.go- 19 changed line(s) have no test coverage — add a test that exercises the new codeinternal/pipeline/steps/coverage_swift.go- 61 changed line(s) have no test coverage — add a test that exercises the new codeinternal/pipeline/steps/test.go- 13 changed line(s) have no test coverage — add a test that exercises the new codego test -race ./...go test -cover -coverprofile=/tmp/nm-coverage-524401177.out ./...✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.