Repository navigation
fix: just check failures on macOS - #8426
Conversation
Quest-Claim: d69d9c2ee0c061b2
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… under symlinked tmp Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…setup off Linux Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ula tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pre-merge checks:
|
…; quest: keep macos-check for cpp and py Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit 843a47b.
No actionable findings in the changes reviewed. Real-path normalization aligns the documentation and coverage lookups with their source paths; the release-plz version floor is inside the rollback guard and the dev shell now supplies it on Darwin. The non-Linux cancellation snapshot addresses descendants reparenting after the direct child exits, while leaving Linux subreaper behavior intact. Profiling refusal now occurs before evidence creation, and the platform-specific doc/test adjustments match their Linux-only dependencies.
Overall direction: the fixes are coherent, but this is not a clean macOS check result: the PR reports remaining C++ linker-flag and Python autosummary failures, and GitHub currently reports merge conflicts.
Verification limits: GitHub-only static review of all 14 changed files and related supervisor tests, launcher, version rollback, documentation generation and coverage code. No builds, tests, Nix evaluation or macOS process execution performed; reported validation was not independently reproduced.
# Conflicts: # sh/drafts/publish.test.sh
There was a problem hiding this comment.
ℹ️ No critical issues — two small gaps worth a look, both about work this change leaves unverified.
Reviewed changes
Reviewed all 6 commits (843a47b, base main c54e5ea): the macOS just check repairs and the quest deletion that finishes quest/m0/macos-check.md.
- Supervisor cancellation off Linux (
sh/local.ts): aps -A -o pid=,ppid=snapshot records the descendant tree when a cancellation signal arrives, and survivors areSIGKILLed after the child exits. Linux keeps the subreaper path. - Doc API real paths (
doc/.vitepress/api/index.ts):generaterealpathsrootand each package dir sorelative(root, dir)matches TypeScript's real-path sources behind macOS's/var->/private/var. - release-plz in the dev shell (
flake.nix): moved out of the!isDarwinlist. Confirmed against the pinned nixpkgs revd357a2b—release-plzis 0.4.1 there with nometa.brokenand a Darwincurlinput, so the removed "marked broken" comment was stale. - 0.4.1 floor at cut time (
sh/version.py): a Rust cut probesrelease-plz --versioninside the restore guard instead of silently leaving versions unset. - Relay profiling refuses at setup (
bench/run.sh,bench/profile.test.py): theuname -scheck moved into the--profilearm after selection validation and beforeARTIFACTS/RUNand the "Benchmark evidence" print, plus the matching test and off-Linux skips. - Real temp paths and Linux-only docs:
pwd -Pinsh/drafts/publish.test.sh,Path(tmp).resolve()insh/gh/gst-formula.test.py,root.resolve()insh/js/coverage.py, andcfg_attr(not(linux), allow(rustdoc::broken_intra_doc_links))indemo/carla/bridge/src/lib.rsandrs/moq-vaapi/src/lib.rs.
ℹ️ The two remaining macOS failures lose their quest with this one
The PR body keeps check cpp (-Wl,-lintl reaching a compile-only clang++ under -Werror) and check py (missing Sphinx autosummary stub for moq.media.catalog) as follow-ups, but nothing under quest/ records them once quest/m0/macos-check.md is deleted. A PR body is not a tracked commitment here, and both outlive this branch.
Technical details
# Two macOS `just check` failures are untracked after this deletion
## Affected sites
- `quest/m0/macos-check.md` — deleted, and its `quest/m0/README.md` entry removed.
- `quest/m1/carla-workspace.md` — edited to drop the now-dangling cross-link.
- `quest/` — no file mentions `-lintl` or `moq.media.catalog`.
## Required outcome
- The two failures the quest's Goal does not cover remain findable in the quest tree, so a later
`/quest-complete` or `/quest-audit` run can pick them up rather than rediscovering them.
## Suggested approach (optional)
- File one or two follow-up quests (or add a line to `quest/m0/nightly/macos.md`, which already
owns macOS-host coverage) naming each failure and its owning check.deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
| # Older release-plz, such as a non-Nix one ahead on PATH, leaves versions unset without failing. | ||
| found = run(root, "release-plz", "--version").split()[-1] | ||
| if version(found) < (0, 4, 1): | ||
| raise ValueError(f"release-plz {found} is older than 0.4.1; use the Nix dev shell's") |
There was a problem hiding this comment.
The floor itself ships untested, and the existing fake release-plz in sh/version.test.py:258 (a shell script that corrupts Cargo.toml/Cargo.lock and exits 1 on any argument) now fails on this --version probe rather than inside set-version, so the test no longer exercises what its comment claims. Since CONTRIBUTING.md leans on that fixture to catch release-plz behavior changes, give the fake a --version answer of release-plz 0.4.1 and add a case with an older one asserting this refusal.
Technical details
# The release-plz floor has no test, and the restore fixture moved its failure point
## Affected sites
- `sh/version.py:248-251` — the new probe and refusal.
- `sh/version.test.py:258` — fake binary is argument-blind; the `--version` probe trips it first.
- `CONTRIBUTING.md:137` — states the fixture catches release-plz upgrades that break the cut.
## Required outcome
- A test proves an older release-plz fails the cut loudly with the versions untouched.
- `test_cut_refuses_bad_targets_and_restores_partial_external_failure` still fails *inside*
`set-version`, after release-plz has edited manifests.
## Suggested approach (optional)
- Make the fake print `release-plz 0.4.1` when `$1 == --version` and keep the corrupting failure
otherwise; add a sibling fake printing `0.3.149` to cover the refusal.
- The error text reads truncated at `use the Nix dev shell's`; name the binary, e.g.
`release-plz {found} is older than 0.4.1; run the cut inside `nix develop``.| for (const name of ["SIGINT", "SIGTERM", "SIGHUP"] as const) { | ||
| process.on(name, () => { | ||
| signaled ??= 128 + constants.signals[name]; | ||
| if (process.platform !== "linux") for (const pid of descendants(process.pid)) tree.add(pid); |
There was a problem hiding this comment.
This branch is the only new logic in the PR and nothing in CI runs it: Linux skips it, and platform.yml's macOS job only runs just rs macos / just rs macos-test, never sh/local.test.py. Taking the snapshot on every platform would make the existing Linux test_cancellation_reaches_descendants_and_waits_for_cleanup cover the parse-and-kill path, since the sweep is a harmless superset of what the subreaper and --no-orphans already do.
Technical details
# The non-Linux descendant sweep is unexercised, and still gaps on normal exit
## Affected sites
- `sh/local.ts:22-37` — `descendants()` walk, platform-independent apart from its caller.
- `sh/local.ts:44-58` — sweep gated on `process.platform !== "linux"` and on a signal.
- `sh/local.test.py:210` — `test_exit_reaches_descendants_orphaned_by_their_parent` stays
Linux-only, so a child that exits normally leaving a stdout-holding grandchild still hangs
off Linux; only the cancellation trigger is covered by the new code.
## Required outcome
- Some automated lane exercises the snapshot/kill path (Linux CI is the only option today).
- Decide whether the normal-exit orphan case is in scope for this PR or a follow-up; if left,
say so, since that is the same hang with a different trigger.
## Suggested approach (optional)
- Drop the platform guard so the tree is recorded on Linux too; the two cancellation tests in
`sh/local.test.py` then cover the new code, and the resulting behavior is one path rather
than two.There was a problem hiding this comment.
ℹ️ No new issues in these commits — the two inline suggestions from the previous review still stand.
Reviewed changes
Reviewed the delta since the last review (843a47b → d05f420, 3 commits): the start-time hardening in the supervisor, the kept-and-narrowed quest, and the merge of main.
- Reused PIDs are no longer killed (
sh/local.ts): oneps -A -o pid=,ppid=,lstart=snapshot now carries each PID's start time throughdescendants(), and the post-exit sweep re-reads the table andSIGKILLs only a recorded PID whose start time still matches. - The quest survives, narrowed (
quest/m0/macos-check.md,quest/m0/README.md): onlycheck cpp(-lintlreaching a compile-onlyclang++under-Werror) andcheck py(missingmoq.media.catalogautosummary stub) remain, each with its recorded failure text, the maintainer's narrowing decision, and undiagnosed-cause leads. This resolves the untracked-follow-up point from the previous review. mainmerged in: imports the quest changes from #8465/#8451/#8418 (quest/m0/ietf-max-age.mdand its README line) and drops this branch'ssh/drafts/publish.test.shfix, since main deleted that file; the body records thatsh/local.test.pypasses again on macOS after the merge.
The new sh/local.ts logic reads correctly: lstart is stable for a live process, both snapshots parse it the same way, and a reused PID (or one that exited) fails the string comparison instead of being killed. The comment on sh/local.ts:47 about the non-Linux sweep having no lane that runs it still applies to this code, and the sh/version.py:251 note about the untested 0.4.1 floor is unchanged, so neither is repeated inline here.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

Fixes the three
just checkfailures seen on macOS hosts (moq-dev/moq#8392, moq-dev/moq#8394) and narrowsquest/m0/macos-check.mdto the two macOS failures left.Changes
doc/.vitepress/api/index.ts):generatenow realpathsrootand every package dir. TypeScript reports sources by real path, so a symlinked root (macOStmpdir()is/var->/private/var) madedirsmiss and links render as../../private/var/.... Reproduced on Linux withTMPDIRpointed at a symlink: 2 failures before, the re-export test passes after (one unrelated 5s test timeout under host load).demo/carla/bridge/src/lib.rs): off Linux the crate is empty, so its//!intra-doc links dangle. Allowrustdoc::broken_intra_doc_linksonly off Linux, with areason(clippy'sallow_attributes_without_reason). Holds whether or not carla-workspace moves the bridge out of the root workspace.flake.nix,sh/version.py): the dev shell left release-plz out on Darwin ("marked broken"), so macOS fell back to a stray~/.cargo/bin0.3.149. The pinned nixpkgs no longer marks it broken and cache.nixos.org has 0.4.1 foraarch64-darwin, so the flake now supplies it on every platform (nix develop --command release-plz --versionprints 0.4.1 on macOS); that is what makes the 0.4.1 floor hold there. A cut also fails loud on release-plz older than 0.4.1 (inside the restore guard, so nothing is left half-edited) instead of silently leaving versions unset.sh/local.ts): the supervisor forwards SIGTERM to its child; on Linux a subreaper keeps the grandchildren in the tree for--no-orphans, but macOS has none, so once the child died its grandchild reparented to launchd, kept stdout open, and hungsh/local.test.py(reproduced). Off Linux, the supervisor now snapshots its descendant tree when cancellation arrives and SIGKILLs the survivors after the child exits, only those whosepsstart time still matches the snapshot, so a reused PID is never killed.bench/run.sh,bench/profile.test.py):--profilerefuses at setup, after validating the selection and before creating evidence, instead of insiderun_profiles. The orchestration tests and the/procSIGUSR1 test skip off Linux; a new test checks the refusal there.rs/moq-vaapi/src/lib.rs,sh/gh/gst-formula.test.py): same two causes as above, surfaced once the supervisor change selected the whole tree.sh/js/coverage.py): same symlinked-temp-dir cause as the doc fix;reportresolvesrootso it matches the real paths of measured sources. Surfaced by the macOS run.Choices made without asking: 0.4.1 is the floor because it is the only version known good; the exact version where
set-versionbecame correct was not bisected.Decisions
test ghfailures: ✅ widen this PR (rejected: land as is with follow-ups). Maintainer, 2026-11-10: the quest's Goal isjust checkpassing on macOS.check cppandcheck py: ✅ land this PR and keepquest/m0/macos-check.mdopen, rewritten to those two with their evidence (rejected: widen this PR again). Maintainer, 2026-11-10.Verification
sh/version.test.py(7 OK), doc API test under symlinkedTMPDIR, biome, ruff,quest check.quest gate nix develop --command just check, no processes killed by hand: the supervisor change selected the whole tree. All failures in this PR's scope pass (sh/local.test.py,bench/profile.test.py,sh/version.test.py,sh/coverage.test.py,sh/gh/gst-formula.test.py, carla and moq-vaapi docs). Two failures remain in code this branch does not touch:check cpp:clang++: error: -Wl,-lintl: 'linker' input unused [-Werror,-Wunused-command-line-argument].check py: Sphinxpy/moq-rs/docs/index.md:147: WARNING: autosummary: stub file not found 'moq.media.catalog'(warnings are errors).Caused by
Public API: none. Wire: none.
Merged
mainon 2026-11-10: it deletedsh/drafts/publish.test.sh(moq-dev/moq#8441), so this PR's fix to it was dropped;sh/local.test.pypasses again on macOS after the merge.Follow-ups
check cppandcheck pyon macOS: tracked by the narrowed macos-check.Models
Size: S
Implementer: Claude Code, claude-opus-5-5, unknown effort
Implementer: Claude Code, claude-opus-5-5, low effort (iterate, S)
(Written by Opus 5.5)
Review
Rewritten by
quest reviewafter each round.01a1c88d-2cc5-7fc1-8f2e-7a3d0562bd4d, round 3The new head merges base changes without altering the previously reviewed implementation, so no actionable findings remain; builds and tests were not run as instructed.