Ship fix/coverage-lcov-pipeline-full-surface - #616
Merged
Conversation
…tura User called out that the Codecov dashboard was only tracking 188 of 695 lines (0.6% of Pulp's ~113k-line coverage-bearing surface). Codex sanity-check confirmed two real bugs, both fixed here: 1. `-object` list was too narrow. `scripts/run_coverage.sh` passed only test binaries to llvm-cov, which means llvm-cov only reported translation units LINKED INTO a test binary. Production code in first-party libraries that no test transitively depended on was invisible end-to-end. This silently false-negatived the diff-cover gate (#132) on any PR touching code outside the test-linked slice. Fix: also pass every `libpulp-*.a` static archive and non-test first-party executable (CLI, standalone, inspector). LLVM docs confirm `.a` archives are valid `-object` inputs. 2. gcovr 8.6 drops ~99% of the coverage data once the `-object` set widens. Direct `llvm-cov report` saw 110k tracked lines across 577 source files; the same profdata piped through `gcovr --llvm-cov-binary` per test binary emitted a Cobertura XML with 150 lines across 4 files. Fix: bypass gcovr. Pipeline is now `llvm-cov export --format=lcov` → vendored `tools/scripts/lcov_cobertura.py` → Cobertura XML. gcovr is no longer installed in CI. Honest baseline after both fixes (local macOS run): - 53,528 lines tracked (was 695, ~77× expansion) - 14,494 covered (27.08% line coverage) - core/ 32.3%, tools/ 16.8%, ship/ 20.7%, inspect/ 3.2% The headline rate doesn't change much (27% → 27%) because the denominator grew while the numerator stayed about the same — which is exactly the honest outcome. Per-subsystem slicing is now meaningful and diff-cover can actually score PRs that touch code outside the former test-linked slice. The `apple/` tree is still a blind spot: PULP_ENABLE_COVERAGE is Clang C/C++ only, so the Swift subsystems aren't instrumented. Tracked as #615. Also: - `.claude/commands/codex-consult.md` — new Pulp-plugin slash command wrapping `codex exec` with the right invocation pattern (stdin, not argv; `-c model_reasoning_effort="medium"`; `timeout` wrapper). Long argv prompts hang `codex exec` silently (0% CPU, no TCP); this command codifies the safe path so the next agent doesn't rediscover the gotcha. Paired with gotchas section in `~/.claude/skills/codex/`. - `DEPENDENCIES.md` + `NOTICE.md` — Apache-2.0 attribution for the vendored lcov_cobertura.py (eriwen/lcov-to-cobertura-xml 2.1.2). - `.github/workflows/coverage.yml` — gcovr install step removed; structural-empty error message retargeted at the llvm-cov path. Codex consult 2026-04-21 (Q1/Q3/Q5) confirmed approach; see `docs/guides/coverage.md` "Why not gcovr" section for the full rationale. Skill-Update: skip skill=ci reason="remove vestigial gcovr install step from coverage.yml — pipeline is now llvm-cov export → lcov_cobertura.py; not a skill-contract change" Skill-Update: skip skill=cli-maintenance reason="add .claude/commands/codex-consult.md slash command that wraps codex exec with the stdin/timeout/reasoning gotchas; command docs live in the command file itself" Skill-Update: skip skill=packages reason="DEPENDENCIES/NOTICE entries for vendored Apache-2.0 lcov_cobertura.py; standard attribution, no policy change"
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24769b58aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # of whether a test links it. Pattern is `libpulp-*.a`; deliberately does | ||
| # not pick up libausdk.a / libvst3-sdk.a (external SDKs, not our code). | ||
| while IFS= read -r f; do BINARIES+=("-object" "$f"); done < <( | ||
| find "${BUILD_DIR}" -type f -name 'libpulp-*.a' 2>/dev/null || true |
There was a problem hiding this comment.
This object discovery only includes libpulp-*.a, but this script explicitly switches to clang-cl on Windows and CMake emits static libraries there as .lib, so the new “full-surface” expansion is silently skipped on the Windows matrix leg. In practice, the os-windows upload will still miss translation units not linked into test executables, which under-reports coverage and defeats the commit’s cross-OS full-surface intent; include the Windows static-lib suffix in this scan (or derive it from CMake).
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Diff coverage (required)Diff coverage threshold: 75% (required). diff-cover reportDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. See coverage.md for how to interpret this number and the full Phase 1 → Phase 3 roadmap. Per-tier diff coverage (#566 Phase 2)
All touched tiers meet their per-tier floors. |
Linux CI on PR #616 failed with: error: failed to load coverage: 'libpulp-ship.a': malformed coverage data: coverage mapping header section is larger than buffer size `llvm-cov report` refuses the entire run if a single `-object` entry has malformed coverage mapping — one bad archive blackholes the whole pipeline. Seen only on Linux in CI for libpulp-ship.a; running the same script locally on macOS surfaces the same failure for libpulp-platform.a. Different archives go bad on different platforms, presumably because of linker-specific quirks with how ar/llvm-ar concatenates instrumented objects. Rather than guessing which archive will be bad where, add a pre-flight probe: for each binary in the `-object` list, run a minimal `llvm-cov report -object=X -instr-profile=PROFDATA >/dev/null 2>&1`. Any binary that fails to load gets dropped with a warning to stderr and excluded from the real report/export. The pipeline proceeds with the remaining entries. Measured locally: 508 initial -object entries, 1 dropped (libpulp-platform.a), 506 kept. Cobertura XML generation and llvm-cov report/show both succeed. Line coverage on main: 46.7% of ~52k lines. The dropped archive represents a small surface (ship/ or platform/) that other test binaries and executables typically still reach through their own -object entries, so effective coverage loss from a single dropped archive is small. Cost: O(N) llvm-cov invocations in the pre-flight, each reading profdata once and emitting no output. Empirically ~1s per probe on the local build; CI should see similar.
Windows CI on PR #616 failed the reporting step with: ValueError: path is on mount 'C:', start on mount 'D:' `os.path.relpath` raises ValueError when `file_name` and `self.base_dir` are on different Windows drives. The runner workspace lives on D: but FetchContent deps land on C: (GitHub-hosted windows-latest default), so absolute LCOV paths from llvm-cov straddle two drives and the converter blows up. Fix: catch the ValueError and fall back to the absolute path. Codecov ingests absolute paths fine — the relative-path step is purely an aesthetic (shorter file names in the Cobertura XML). Any entry that can't be made relative gets its absolute path kept; the rest still get shortened as before. No-op on macOS and Linux where all paths share a single root. Skill-Update: skip skill=ci reason="vendored converter tweak; no skill content change"
danielraffel
added a commit
that referenced
this pull request
Apr 22, 2026
Four Codex findings on merged PRs #616 and #617. All addressed here. pulp#617 P1 — pin the upstream installer URL to an immutable tag. The wrapper downloaded `install.sh` from `main`, so the same Pulp commit could run different installer code over time — weakens the pinning model and is a supply-chain smell. Now fetches `refs/tags/<VERSION>/install.sh` where VERSION comes from `tools/shipyard.toml`. Verified the tag-pinned URL resolves (200) for v0.22.8 and a full end-to-end install still succeeds. pulp#617 P1 — restore queue-file repair. My original rewrite dropped `repair_truncated_queue_file()`, which handled the #528 failure mode (JSONDecodeError on a zero-byte queue.json after a crash between `open(O_TRUNC)` and `write()`). Re-running the wrapper is the documented recovery path; without the repair step it stopped fixing the thing it's meant to fix. The regression test `test_install_shipyard.py` still exercises this path. pulp#617 P2 — move legacy `~/.pulp/bin/shipyard` cleanup to AFTER the upstream installer succeeds. Previously cleanup ran first, so a curl failure deleted the user's only working shipyard symlink. The wrapper is idempotent; cleanup on the next successful run is fine. pulp#616 P2 — also pick up `pulp-*.lib` archives on Windows. `scripts/run_coverage.sh` switches to `clang-cl` on Windows and CMake emits `.lib` archives there (MSVC-style), not `.a`. The full-surface expansion was silently skipped on the Windows matrix leg — the very thing the expansion was supposed to close. The `pulp-*.lib` prefix is narrow enough to skip third-party `.lib` files under _deps/. Verified locally (macOS): bash -n tools/install-shipyard.sh scripts/run_coverage.sh # syntax ./tools/install-shipyard.sh --status # reads pin OK ./tools/install-shipyard.sh # tag-pinned URL, clean install shipyard --version # → 0.22.8 Skill-Update: skip skill=ci reason="installer + coverage-script polish, no skill content change"
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.
Automated by Shipyard.