Skip to content

fix: the Windows batch — rename retry, root canonicalization, POSIX-shaped expectations - #411

Merged
TheAmericanMaker merged 4 commits into
mainfrom
claude/gracious-mendel-s3i0g2
Sep 17, 2026
Merged

TheAmericanMaker merged 4 commits into
mainfrom
claude/gracious-mendel-s3i0g2

Conversation

@TheAmericanMaker

Copy link
Copy Markdown
Member

Summary

The three issues the test-windows job's first run produced, in the order the 2026-09-15 closeout names. Ten failures on windows-latest: two product defects and eight test expectations written for POSIX paths.

atomicWriteFile's rename retries while a holder blocks it (#393). Every canonical write in the framework routes through atomicWriteFile, which renames a sibling temp file over the destination. On Windows that rename fails with EPERM while another writer holds or is replacing the same file, so two concurrent writers — two MCP hosts, Pi and MCP, the usage log appending as a phase completes — lost a write with an error rather than serializing. The rename now goes through a new retryOnTransientFsError: a bounded, jittered retry on EPERM/EBUSY/EACCES that propagates any other error on the first attempt and the last transient one once the 500 ms budget is spent. The retry wraps only the rename, so a permission failure on the temp write still fails immediately, and the temp-file cleanup is unchanged. No win32 branch — a POSIX rename-over-existing is atomic and does not report those codes for contention, so one code path means the Windows job exercises what Linux does.

A containment root that does not exist yet resolves like its target (#394). isWithinPathResolved resolved the target's existing prefix through symlinks (#223) but the root through realpath alone, which throws on a .codecarto/ that is not there yet and fell back to the root as spelled. Wherever an ancestor needed expanding the two operands then disagreed — the 8.3 short name the runner puts in %TEMP% (C:\Users\RUNNER~1\…), macOS' /var → /private/var — and a phase session's write to .codecarto/findings/… was blocked while that directory did not exist yet. Both operands now resolve through resolveExistingPrefix, and the primitive takes the base a relative operand resolves against. The Pi orchestrator hook and the phase hook now call that one primitive instead of each hand-rolling resolveExistingPrefix + canonicalPath + isWithinPath — the duplication one of them had drifted from — and the MCP spec_path check inherits the fix. What containment refuses is unchanged.

Eight expectations no longer assume POSIX paths (#395). An outputDir's last segment taken with split("/"), which does not split a backslash path; four comparisons against a raw library.path or source_repo line, where a path with backslashes is correctly emitted as a quoted YAML scalar with those backslashes escaped; a path interpolated into a RegExp source, where \b becomes a word boundary; a refusal message matched against a /-rooted pattern; and a joined path compared against a /-joined literal. Each now compares a parsed value, a basename, a path.join on both sides, or a literal prefix.

Type of change

  • Bug fix
  • Feature
  • Refactor (no behavior change)
  • Documentation
  • CI / build
  • Framework / pipeline change (template, skills, validation, prompts)

Checklist

  • Tests added or updated — 943/948 pass locally, with one pre-existing failure unrelated to this branch (see notes)
  • Pipeline invariants still hold (Pi extension and MCP server byte-identical with template)
  • Documentation updated — CHANGELOG.md gains an [Unreleased] section with one entry per issue
  • No new dependencies added without discussion
  • Commit messages follow the repo style — one uses test:, which appears once in history and is the accurate prefix for a test-only change

Related issues

Closes #393
Closes #394
Refs #395

Additional notes

continue-on-error stays on the test-windows job, deliberately. #395 makes the gate flip the consequence of these three fixes, but whether the suite is green on Windows can only be settled by a run — flipping it blind would turn an unverified job into a PR blocker. That is why #395 is Refs and not Closes: the flip is a one-line follow-up once this PR's Windows job reports green, and that is the moment to close the issue.

Nine new tests, each failing on the previous code. Verified by stashing the source change and re-running. Six cover the retry loop directly (each transient code retried until the operation lands, the budget spent still propagating the last error, permanent codes rejected on the first attempt, a thrown non-Error passed through); three cover containment (an unborn root under a symlinked ancestor, the phase guard's first write through one, the base a relative operand resolves against).

How the Windows-specific claims were checked without a Windows runner. #394 reproduces on Linux through a symlinked ancestor — the same divergence the 8.3 short name causes — so no Windows run was needed to find or fix it, and the issue's "needs a run on Windows with the two resolved paths printed" turned out not to hold. For #395 every mechanism was replayed against Windows-shaped data on Linux, using win32 path semantics and the repo's own YAML emitter fed a C:\… path: the old form fails and the new form holds in each of the five cases. What could not be verified here is the end-to-end behaviour of #393's fix, because a POSIX rename cannot be made to report EPERM for contention — the forty-writer concurrency test is the reproduction, and this PR's Windows job is what settles it.

One correction to #394 worth recording. Its "on Windows a phase sub-agent cannot write its findings at all and the Pi surface does not work" overstates what the code shows. The guard only misfires while .codecarto/ is absent; in an initialized workspace the root realpaths fine and both sides agree, and the orchestrator hook's roots always exist at hook time. It was a latent consistency bug whose only reachable manifestation was the test — still worth fixing, since the Windows job cannot go green without it and the invariant is now true rather than accidentally true.

Reviewers may want to look at the 500 ms retry budget (the issue asked for "a few hundred milliseconds"; graceful-fs uses 60 s, sized for dev tooling) and the decision to fix #394 at the primitive rather than patching each root call. I checked the other four containment sites and left them alone: agent-runner.ts:51 derives its candidate from the canonicalized root so both sides already agree, and the three sameWorkspace-style comparisons (index.ts, mcp-server/server.ts, core/library.ts) are each guarded by a pathExists immediately before, so both operands always exist.

The pre-existing local failure, for the record. tests/library.test.mjs → resolvePublishSourceRepo records origin's fetch URL verbatim fails in any environment carrying a global url.<base>.insteadOf git setting, which rewrites what git remote get-url reports. tests/pi-publish.test.mjs already guards against exactly this by pointing GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM at a nonexistent path; tests/library.test.mjs has the same assertion with no such guard. It is green on CI's bare runners and untouched by this branch, so it is left for its own issue rather than folded in here.

🤖 Generated with Claude Code

https://claude.ai/code/session_011ucQwVaxXUXxzHAvmgaNp7


Generated by Claude Code

Every canonical write in the framework routes through atomicWriteFile,
which writes a uniquely named sibling temp file and renames it over the
destination. On Windows that rename fails with EPERM while another writer
holds or is replacing the same file, so two concurrent writers -- two MCP
hosts, Pi and MCP, the usage log appending as a phase completes -- lost a
write with an error rather than serializing. The forty-writer concurrency
test in tests/state-store.test.mjs failed with twenty EPERM rejections on
windows-latest; status.yaml, the usage log, Broad-Side state.json and the
library index all land through this function.

The rename now goes through retryOnTransientFsError: a bounded, jittered
retry on EPERM/EBUSY/EACCES that propagates any other error on the first
attempt, and the last transient one once the 500ms budget is spent. The
temp-file cleanup is unchanged, and the retry wraps only the rename, so a
permission failure on the temp write still fails immediately.

A POSIX rename-over-existing is atomic and does not report those codes
for contention, so the loop is one behavior on every platform rather than
a win32 branch. Six tests exercise it directly -- all six fail on the
previous code -- and the forty-writer test proves the wiring end to end
on the test-windows job.

Closes #393

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011ucQwVaxXUXxzHAvmgaNp7
isWithinPathResolved resolved the target's existing prefix through
symlinks (#223) but the root through realpath alone, which throws on a
.codecarto/ that is not there yet and fell back to the root as spelled.
Wherever an ancestor needed expanding the two operands then disagreed --
the 8.3 short name the Windows runner puts in %TEMP%
(C:\Users\RUNNER~1\...), macOS' /var -> /private/var -- and a phase
session's write to .codecarto/findings/contracts/out.md, the one place it
may write, was blocked while that directory did not exist yet. That is
the windows-latest failure in tests/phase-compaction.test.mjs; it
reproduces on Linux through a symlinked ancestor, so settling it needed
no Windows run.

Both operands now go through resolveExistingPrefix, and the primitive
takes the base a relative operand resolves against. The Pi orchestrator
hook and the phase hook call that one primitive instead of each
hand-rolling resolveExistingPrefix + canonicalPath + isWithinPath -- the
duplication one of them had drifted from -- and the MCP spec_path
containment check inherits the fix. What containment refuses is
unchanged.

Three tests, each failing on the previous code: an unborn root under a
symlinked ancestor, the phase guard's first write through one, and the
base a relative operand resolves against.

Closes #394

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011ucQwVaxXUXxzHAvmgaNp7
The windows-latest job's first run reported ten failures. Two were
product defects, fixed in the preceding two commits; the other eight were
expectations written for /-separated paths.

- tests/broadside.test.mjs took an outputDir's last segment with
  split("/"), which does not split a backslash path. basename does.
- tests/config-problems.test.mjs and tests/pi-publish.test.mjs compared
  raw library.path and source_repo lines. A path containing backslashes
  is correctly emitted as a quoted YAML scalar with those backslashes
  escaped, so no raw-line pattern matches it; all four now compare the
  value the parser returns, which round-trips.
- tests/mcp-library.test.mjs interpolated a path into a RegExp source,
  where backslashes read as escapes and \b becomes a word boundary.
  includes says what was meant.
- tests/config-problems.test.mjs matched an invalid-namespace refusal
  against a /-rooted pattern. The refusal table now also takes a literal
  prefix, which a path can be and a pattern cannot.
- tests/synthesis.test.mjs compared a joined specPath against a /-joined
  literal. Both sides now come from path.join, and the expectation
  carries the entries/ segment the anchored regex never reached.

One doesNotMatch in the same family, which a quoted backslash path would
have satisfied vacuously rather than failing, is a parsed-value
comparison too.

Every mechanism was checked against Windows-shaped data on Linux -- win32
path semantics, and the repo's own YAML emitter fed a C:\... path: the old
form fails and the new form holds in each case. No product code changed.

continue-on-error stays on the test-windows job. Whether the suite is
green there can only be settled by a run, so the gate flip belongs in a
follow-up once one reports green.

Refs #395

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011ucQwVaxXUXxzHAvmgaNp7
The windows-latest run on this branch went from ten failures to two, and
both are the same family as the eight already fixed: a test stops at its
first failing assertion, so each of these sat behind one the run had
already reported.

- tests/config-problems.test.mjs had a third raw library.path comparison,
  after the refusals loop, where a successful --namespace init writes both
  keys. Now a parsed deepEqual.
- tests/pi-publish.test.mjs had a second /-rooted source_repo pattern, the
  one checking that publishing v2 leaves v1's recorded path alone. Now a
  parsed equality against fx.cwd, which says what was meant and is
  stronger than "starts with a slash".

Both were reproduced against the exact paths from the failing job before
fixing: the repo's emitter, fed
C:\Users\RUNNER~1\AppData\Local\Temp\cc-config-problems-VYK5zA\pi-library,
produces the quoted, backslash-escaped scalar the log shows as `actual`
byte for byte, the old forms fail on it, and the new forms hold on both
path shapes.

Rather than patch these two and risk a third round, tests/ was swept for
every instance of the shapes involved: raw-line comparisons against an
interpolated path, a path interpolated into a pattern, and split("/") on a
path. There are no others. What the sweep turns up is writes that feed the
parser (unquoted backslash scalars, which it reads verbatim), URLs, which
are always /-separated, and patterns that already escape their input or
interpolate a count, version or model id.

The changelog entry is now ten expectations rather than eight, and says
why the issue's list could only name eight.

Refs #395

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011ucQwVaxXUXxzHAvmgaNp7

Copy link
Copy Markdown
Member Author

The test-windows job on 5bfe6db went from ten failures to two: 948 tests, 942 pass, 2 fail, 4 skipped. Both remaining failures were the same family as the eight already fixed, so b20c8cf fixes them.

A test stops at its first failing assertion, which is why the run could only ever report one bad expectation per test — and why #395's list of eight was what was visible rather than the whole set:

  • tests/config-problems.test.mjs had a third raw library.path comparison, after the refusals loop, where a successful --namespace team init writes both keys.
  • tests/pi-publish.test.mjs had a second /-rooted source_repo pattern — the one asserting that publishing v2 leaves v1's recorded path alone. It is now a parsed equality against fx.cwd, which says what was meant and is stronger than "starts with a slash".

Both were reproduced before fixing, against the exact paths from the failing job: the repo's own emitter, fed C:\Users\RUNNER~1\AppData\Local\Temp\cc-config-problems-VYK5zA\pi-library, produces the quoted backslash-escaped scalar the log reports as actual byte for byte. The old forms fail on it; the new forms hold on both path shapes.

Rather than patch two more and risk a third round, I swept tests/ for every instance of the shapes involved — raw-line comparisons against an interpolated path, a path interpolated into a pattern, and split("/") on a path. There are no others. What the sweep turns up is writes that feed the parser (unquoted backslash scalars, which it reads verbatim), URLs, which are always /-separated, and patterns that already escape their input or interpolate a count, version or model id.

So the PR description's "eight" is now ten — the changelog entry says ten and explains why the issue could only name eight. Everything else in the description stands, including continue-on-error staying on the job until a run reports it green.


Generated by Claude Code

Copy link
Copy Markdown
Member Author

Correcting one claim in the description above, and filing what it was about: #412.

The description's last note says tests/library.test.mjs "has the same assertion with no such guard". That is wrong — it carries the identical guard and comment block, as do six other test files. The guard is incomplete in all seven: GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM only redirect where git looks for config files, so a url.<base>.insteadOf rewrite injected through GIT_CONFIG_COUNT / GIT_CONFIG_KEY_n / GIT_CONFIG_VALUE_n still reaches the fixtures. Measured:

result
baseline https://github.com/Acme/Tool.git
today's guard https://github.com/Acme/Tool.git ← still rewritten
GIT_CONFIG_COUNT=0 added git@github.com:Acme/Tool.git

So the fix is one more line wherever the guard appears, not a guard added to one file. #412 has the reproduction, the seven files, and why changing production to read git config --get remote.origin.url instead would be a provenance decision rather than a test fix.

Nothing in this PR changes — the failure is pre-existing, green on both CI platforms, and untouched here. Only my description of its cause was off.

Also noted for the record: the continue-on-error removal is deferred by decision, not forgotten — #395 now carries the green evidence (948 tests, 944 pass, 0 fail on windows-latest) and the three steps it needs, and stays open as its tracker.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants