Uh oh!
There was an error while loading. Please reload this page.
Fix the CI failures on macOS and Linux, and move workflows off Node 20 - #19
Merged
Merged
Conversation
…ment `api_running_blocks_squad_start` failed the fast job on main. Three separate causes were behind the recent red runs; all three are fixed here. 1. Exec-identity race in the daemon fixtures (the fast-job failure). `Command::spawn` returns once the child *exists*, not once `execve` has landed. Until then the child still reports the command name it inherited from the spawning thread — under `cargo test` a test-function name like `api_running_blo`. `pid_is_awman` correctly rejects that, so a guard checked inside the window read the pidfile, judged it to name a foreign process, cleared it as stale, and allowed a start that had to be refused. Both fixtures now wait on the identity they exist to present, not merely on liveness. Under load the reproduction went from 9/120 runs failing to 0/150. 2. ETXTBSY on the data-layer helper's `execve` (the Linux job failure). A concurrent fork elsewhere in the process inherits the still-open write descriptor on the copied helper binary, so `execve` reports the file busy until that fork execs. Retried the same way `squad_mutual_exclusion.rs` already retries its own spawn. 3. Asymmetric canonicalization in `validate_under_root` (the macOS failures). The root was canonicalized while a not-yet-created child fell back to its lexical form, so any root behind a symlink compared two different prefixes and read as an escape. Every macOS temp dir is such a root (`/var/folders/...` -> `/private/var/folders/...`), which is why only that job saw it. Now both sides resolve the deepest existing ancestor and apply the remaining components lexically. Worth noting that (3) also closed a real hole rather than only a false positive: a `..` traversal whose target did not exist on disk was previously resolved lexically against an unresolved root and *accepted*. It is now rejected, and the new test pins both directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
With the fast job green, the Linux and macOS jobs ran for the first time in
several pushes and surfaced two more failures. Neither is a regression from
the previous commit — the earlier runs aborted before these tests were
reached.
Linux: docker_e2e_live_container_observes_rotated_fingerprint... returned
`NotNeeded { expires_in: 7199s }` while the fixture plants a credential
expiring in 10 seconds — so the rotation had already happened before the
assertion. `run_monitor_loop` ticks BEFORE it sleeps, so `register` lands a
tick immediately no matter how long `tick_interval` is, and that first tick
performed the whole rotation while the test still believed (per the
`monitor()` helper's comment) that a long tick kept the background loop out
of the way. The container was then started against an already-rotated staged
file, so it could never observe two fingerprints either.
Fixed by making the test own its timing: plant a far-from-expiry credential
so the first tick is a no-op, barrier on that tick recording an outcome, and
arm the rotation only once the container is live. The host-refresh path is
still exercised end to end; it is now unambiguously `refresh_now` that drives
it.
macOS: two credential_refresh monitor tests build their fixture by planting
`~/.claude/.credentials.json` under a temp HOME. `claude_source` branches on
`cfg!(target_os = "macos")` and returns the Keychain there, ignoring the
resolver, so the planted file is never read and setup fails with `NotFound`.
Gated both to non-macOS with the reason recorded. The reconciliation logic
they cover is platform independent; only the credential's source is not.
Also unified the macOS `launchctl` stub in daemon_primitives onto the
module's `PATH_LOCK` instead of a private `ENV_LOCK`. Two locks over one
process-global `PATH` serialize nothing: one test installs a stub that exits
0 and the spawn-failure test needs one that exits 1, so whichever ran first
decided the other's result. That is the likely mechanism behind the earlier
macOS `squad_spawn_failure...` failure, which passed this run only on timing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>… macOS `claude_source` branches on `cfg!(target_os = "macos")` and, on that branch, ignores the `AuthPathResolver` entirely and reads the Keychain. So every caller that rebound HOME to isolate itself isolated nothing on macOS: `CredentialRefreshMonitor::with_resolver` documented that tests using it "never read a developer's real credential", and that guarantee was simply false on one of the three supported platforms. It held on CI only because a runner's Keychain is empty — the same code reads real secrets on the machine where secrets actually exist. Adds `CredentialBinding`, which pairs a resolver with an OPTIONAL explicit `HostCredentialSource`: CredentialBinding::platform_default(resolver) // production, unchanged CredentialBinding::to_file(resolver, path) // isolation on every OS `refresh_host_credential` and the monitor now take a binding instead of a bare resolver, and `with_resolver` is preserved as a thin wrapper so no production call site changes behavior. The three remaining `(spec.source)(resolver)` sites are all production paths that should keep the platform default, and do. This also fixes CI rather than working around it. The previous commit gated two monitor tests off macOS because their file fixture could not be built there; both are now ungated and pass on every platform. The three integration tests that failed the macOS job for the identical reason (`materialized_delivery` deriving its source from a temp HOME) are fixed at the root instead of gated. Verified: architecture-lint, fmt, clippy -D warnings, and the full hermetic suite pass; the two formerly-gated unit tests and all seven credential refresh integration tests pass unconditionally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`collect_workspace_choice` canonicalizes the folder the interview collects before handing it to Layer 2, so the stored workspace is the resolved path, not the string the user typed. The test compared against the raw tempdir path, which is identical on Linux but differs on macOS, where the temp directory lives under the `/var` -> `/private/var` symlink. Compare against the canonical path, matching what the sibling unit tests in `squad/commands.rs` already assert. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TJ3kXPuuXeb69EvX7Lngui
GitHub is forcing actions that declare `node20` onto Node 24 and warning on every job. Bump each one to its latest major, all of which declare `node24` natively. Verified the majors we skip past are safe for how these workflows use them: checkout v7 only blocks fork checkouts for `pull_request_target`/ `workflow_run`, and both workflows trigger on `push`/`pull_request`; upload-artifact v7's direct-upload `archive` input defaults to the previous behavior; download-artifact v8 skips unzipping only for non-zipped direct uploads, which we never produce. cache v6 and gh-release v3 are the runtime bump plus an ESM migration. `dtolnay/rust-toolchain` is a composite action with no Node runtime, so it stays as it is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TJ3kXPuuXeb69EvX7Lngui
The skill-library tests inject a `url.*.insteadOf` rewrite through git's `GIT_CONFIG_COUNT` / `GIT_CONFIG_KEY_n` / `GIT_CONFIG_VALUE_n` trio so a pull never reaches the network. Those variables are process-global, and the lock around them only serializes the tests that *inject* them — every other test's `git` child still inherits whatever is set at the instant it is spawned. Both fixtures set the count first, which opens a window where the environment promises a key that has not been written yet. A git process spawned inside that window does not ignore the shortfall, it dies: $ env GIT_CONFIG_COUNT=1 git config user.name awman-test error: missing config key GIT_CONFIG_KEY_0 fatal: unable to parse command-line config That is the macOS CI failure, hit by `Upstream::new`'s `git config user.name` in a test that never touches the rewrite itself. It is a race, not a macOS bug; macOS just loses it more often because spawning a process there is slow enough to land in the window. Write the indexed pair first and the count last. While the count is absent git ignores the indexed keys entirely, so the transient state is inert rather than fatal, and the restore already ran in that order. Also give the unit-test fixture a `Drop` guard, so a panicking test no longer leaves the rewrite set for every later git invocation in the process. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TJ3kXPuuXeb69EvX7Lngui
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Gets the
Testsworkflow green on all three jobs. The failures were realportability bugs in the tests, not flakes — they only ever reproduced on the
runners because each depends on something the Linux dev environment hides.
Why this took several rounds
cargo teststops at the first failing binary, and the macOS job runs thesame hermetic suite as
fast. Each fix unblocked the next binary inalphabetical order, so every push surfaced exactly one new failure rather
than the whole set. The last one was
squad_blockers, 18th of ~30 — the 13binaries after it had never executed on macOS on this branch at all.
What was wrong
Daemon identity races (
daemon_primitives,squad_mutual_exclusion).pid_is_awmandistinguishes a live daemon from a stale pidfile by readingthe process command name. The fixtures spawned a child and read that name
before
execvehad landed, so they intermittently observed the spawningprocess's identity instead. They now wait for the identity the fixture
exists to present.
Symlinked-root containment (
path_guard). Path containment comparedunresolved paths, so a root reached through a symlink failed to match itself.
Host credential isolation (
credential,credential_refresh). Therefresh monitor read the host credential source directly, so on macOS the
tests picked up the developer's real credentials. The source is now
injectable.
Workspace canonicalization (
squad_blockers).collect_workspace_choicecanonicalizes the folder collected in the interview before handing it to
Layer 2, so the stored workspace is the resolved path. The test compared
against the raw temp path — identical on Linux, but macOS temp dirs live
under the
/var->/private/varsymlink. Now matches what the sibling unittests in
squad/commands.rsalready assert.Node 20 deprecation. Every job warned that
actions/checkout@v4andactions/cache@v4were being forced onto Node 24. All actions are bumped totheir latest major, each of which declares
node24natively.Verification
All three jobs pass as of
42023234:https://github.com/prettysmartdev/awman/actions/runs/33640181498
The macOS job now runs the suite to completion, so the 13 binaries that had
never executed there are covered.
Note on
pid_is_awmanNot a current failure, but worth knowing. On Linux it reads
/proc/<pid>/comm, which is the truncated basename. On macOS it shells outto
ps -o comm=, which returns the full executable path — and the CIcheckout is
/Users/runner/work/awman/awman/..., so on macOS effectively anytest process's PID matches "awman". Every consumer today either waits for the
match to become true or accepts both outcomes, so nothing breaks. It would
bite a future test that asserts a non-awman process gets refused.
🤖 Generated with Claude Code
https://claude.ai/code/session_01TJ3kXPuuXeb69EvX7Lngui