Skip to content

Stop building the temporary filename out of the whole path (#716) - #717

Merged
MongLong0214 merged 2 commits into
mainfrom
fix-716-windows-temp-name
Aug 17, 2026
Merged

Stop building the temporary filename out of the whole path (#716)#717
MongLong0214 merged 2 commits into
mainfrom
fix-716-windows-temp-name

Conversation

@MongLong0214

Copy link
Copy Markdown
Owner

Partially addresses #716; the observation that produced it is on #714.

An owner ran v1.0.2 on a real Windows machine with agents present — the run #714 was holding open. Every detected host failed and no config was written. gemini-cli printed the cause:

open 'C:\Users\u\.gemini\.C:\Users\u\.gemini\settings.json.commitlore-41756-….tmp'
      └── dirname ──────┘  └── the whole path, used as a filename ────┘

Both atomic writers built the temporary name with path.split('/').pop(), which returns its argument unchanged when there is no / in it — true of every Windows absolute path. A drive letter cannot be part of a filename, so the write was ENOENT before it started.

Why not basename

basename is correct here on Windows. It is also unfalsifiable anywhere else, and this repository has no Windows evidence to lean on: CI has no coding agent, so the install-ps1 job detects nothing and never reaches this line — the whole of #714. Splitting on either separator makes the property one a POSIX runner can fail on, which is the only kind of guard that can hold a Windows-only defect here.

Not fixed here

The second cause in #716: hasCommand stats each PATH entry with the bare command name, so it never finds cursor.cmd or claude.cmd, and spawnSync(..., { shell: false }) cannot execute a .cmd shim. That accounts for codex mcp add failed, Hermes setup failed, and claude-code reported notDetected with its config present. Fixing it needs a design that preserves the reason shell: false was chosen — the wrapper path and config paths reach those calls — so it is not folded in here.

Verification

Restoring split('/') fails 2 of the 4 new cases, including the Windows path producing .C:\Users\u\.gemini\settings.json.commitlore-pid-uuid.tmp where .settings.json.commitlore-pid-uuid.tmp is required. 7 cases pass across the two installer-hosts suites; tsc --noEmit clean; build:canonical then artifact:verify8613363cdee2183e7a6f693b31450d715f3f316ba32a4ed77272b8af9d9c9c0f.

No Windows machine has run this fix. The guard proves the name, not the install. #716 stays open for the second cause and for a Windows readback.

An owner ran v1.0.2 on Windows with agents present -- the observation #714 was holding open. Every detected host failed and nothing was written. gemini-cli named the cause exactly:

    open 'C:\Users\u\.gemini\.C:\Users\u\.gemini\settings.json.commitlore-41756-....tmp'
          |__ dirname _____|  |__ the whole path, used as a filename __|

Both atomic writers took the name from `path.split('/').pop()`, which returns the argument unchanged when it contains no `/` -- true of every Windows absolute path. A drive letter cannot appear inside a filename, so the write was ENOENT before it began, and each host reported `atomic config write failed` rather than wiring anything.

The name is now taken by splitting on either separator instead of deferring to `basename`. `basename` is correct on Windows and not provable off it, and this repository has no Windows evidence to lean on: CI has no coding agent, so the install-ps1 job detects nothing and never reaches this line -- which is the whole of #714. A defect that appears only on Windows has to be one a POSIX runner can fail on, or nothing here can hold it.

This is one of two causes #716 records. The other -- `hasCommand` stats PATH entries with the bare name, so it never finds `cursor.cmd`, and `spawnSync` with `shell: false` cannot run a `.cmd` shim -- is not touched here: it needs a design that keeps the reason `shell: false` was chosen, and it cannot be closed without another Windows run either way.

Limit: this fixes the write, not the detection -- codex, hermes and claude-code failed for the second cause and still will
Limit: no Windows machine has run this fix; the guard proves the name, not the install
Ruled-out: use basename() | correct on Windows, unfalsifiable on the platform CI actually runs
Blast: module
Undo: easy
Certainty: firm
Record-Id: r-wintmp716
Provenance: authored
Verified: restoring `split('/')` fails 2 of the 4 new cases, including the Windows path yielding '.C:\Users\u\.gemini\settings.json.commitlore-pid-uuid.tmp' where '.settings.json.commitlore-pid-uuid.tmp' is required; 7 cases pass across the two installer-hosts suites; npx tsc --noEmit clean; build:canonical then artifact:verify -> 8613363cdee2183e7a6f693b31450d715f3f316ba32a4ed77272b8af9d9c9c0f
CommitLore-Version: 2.0.0
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown

CommitLore — record lint

Trailers: clean — 2 commits in origin/main..bd297e12bf599e7fe9038be7abb5d1ca3b695afa
Active constraints: not read — commitlore: git log --follow accepts exactly one pathspec, so renames are not followed for 7 paths; query one path at a time to follow its rename chain (7 changed paths)

Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR.

…name

# Conflicts:
#	dist/commitlore.mjs
#	installer/canonical-artifact.json
@MongLong0214

Copy link
Copy Markdown
Owner Author

Post-merge note on the artifact digest.

This branch's record (r-wintmp716) verifies artifact:verify -> 8613363cdee2183e7a6f693b31450d715f3f316ba32a4ed77272b8af9d9c9c0f. That is true of the commit it sits on. After #715 landed, main moved and this branch merged it; the two committed build artifacts conflicted, which is expected — three open PRs each regenerate dist/commitlore.mjs and installer/canonical-artifact.json.

Resolved by regenerating rather than by picking a side. A generated file has no correct side; the only answer is to rebuild it from the merged source:

build:canonical then artifact:verify -> d20d47b227b226b1b352fc5c6f9527d9b1653115d54536196948b79fdbfe3a6f

So a reader checking the record against the merged tree finds two different digests, and both are right about different commits. Recorded here so that difference is not read as a build that drifted.

No force push was used — the update is a merge of origin/main into this branch.

@MongLong0214
MongLong0214 merged commit 5dcc02b into main Aug 17, 2026
12 checks passed
MongLong0214 added a commit that referenced this pull request Aug 17, 2026
Squashed: nothing is stacked above this. #717 went in as a merge commit so this PR's base stayed an ancestor of main and its diff never re-absorbed #717's changes.

Seven failure paths reported what went wrong without saying which file it went wrong on, and HostResult carried no path field, so the information was never recorded anywhere -- an installer transcript could not have answered the cursor question because nothing had ever written the answer down. That is a different defect from withholding it.

Toward #716, not closing it. The second cause a real Windows run found is unfixed: hasCommand never consults PATHEXT so .cmd shims are invisible, and spawnSync with shell:false cannot execute them. That needs another Windows run to close.
@MongLong0214
MongLong0214 deleted the fix-716-windows-temp-name branch August 17, 2026 02:51
MongLong0214 added a commit that referenced this pull request Aug 17, 2026
…731)

* Plant a host in Windows CI so the wiring branch actually runs (#722)

A GitHub runner has no coding agents, so detection short-circuits and the Windows job has printed `"hosts":[]` on every run it has ever made. Every branch that wires a host is unexecuted there -- not under-tested, unreachable -- and an assertion reading that list passes whether the wiring code works, is broken, or has been deleted.

That vacancy is why #714 existed as a standing record rather than a task, and why both Windows defects in #716 were found by a person on a real machine instead of by any job here. The response until now was to make the guards falsifiable somewhere the code does run: #717 pinned a temporary-filename property as a pure function, #720 added a mode-0644 shadow for executable resolution. That works and it is second best, because it proves properties of helpers rather than executing the path.

A planted `cursor.cmd` removes the vacancy instead of working around it. Detection looks for the command on PATH, and nothing here executes Cursor -- Cursor plays no part in wiring, since what gets written is its config and what gets probed is CommitLore's own wrapper. So the fixture is a file, and the branch it unlocks is the real one, end to end: detection, the atomic config write, and a live MCP initialize.

Two cases, and the second is what makes the first mean anything. A fixture that can only succeed proves the installer reports what the fixture told it, which is the empty runner again with more steps. So the second gives the same detected host a config it cannot parse and requires `outcome: "failed"`, `ok: false`, the file named in the reason, and the unusable config unmodified afterwards.

The suggestion and its framing -- so this class cannot hide behind an empty runner again -- came from kantorcodes1 in a public discussion. It is a better answer than the one I was giving.

Limit: one host of seven, on one platform. gemini-cli, windsurf and opencode take the same JSON path and are not planted; codex, hermes and claude-code take different ones and are not covered at all
Limit: this executes wiring, not a real agent -- a planted shim is a file the job wrote, and it says nothing about what Cursor itself does with the registration
Ruled-out: assert on the existing `"hosts":[]` output | it is the same on a working installer and a deleted one, which is the defect this replaces
Ruled-out: a stub that always registers cleanly | it proves the installer reports what the stub told it, and cannot fail, so it restores the vacancy under a new name
Blast: system
Undo: easy
Certainty: firm
Verified: the workflow parses as YAML and the job gains one step; `EXPECTED_CI_WORKFLOW_SHA256` is re-locked to the edited file, which is the mechanism that would otherwise let a workflow body change unreviewed, and 29 release-prerequisite cases pass against it. The step itself cannot be run locally -- it needs windows-latest, which is the whole point -- so its first execution is this pull request's own `install-ps1` job.
Record-Id: r-plantedhost722
Provenance: authored
CommitLore-Version: 2.0.0

* Assert the exit code the planted host was already producing

The step's first run did everything it claimed. On a GitHub runner, with no coding agent installed, the enumeration wired a host and live-verified it:

    ok:true   cursor  installed  healthy  "registration created and live-verified"
    ok:false  cursor  failed     "D:\a\_temp\cl-host-bad\.cursor\mcp.json is not parseable JSON..."

The job still failed, and the reason is worth keeping rather than papering over. GitHub's pwsh shell exits with the last native command's status, and the second case deliberately produces a failing one -- so the step was reporting, accurately, that a host had failed.

That is a property nothing asserted. `install.ps1` returns this command's status, so a host that failed must make it non-zero or an install reports success over a repository with nothing wired -- the exact claim the installer is for. Both codes are now checked: zero when the host wired, non-zero when it did not, and an explicit `exit 0` at the end because every assertion throws before reaching it.

Limit: the second case's non-zero code is asserted, not its value -- a change from 1 to 2 passes here
Blast: module
Undo: easy
Certainty: firm
Record-Id: r-plantedexit722
Provenance: authored
Verified: the run this repairs is the evidence -- both enumerations are in its log with the outcomes the step requires, and the only thing that failed was the shell's exit status; the workflow parses, the digest is re-locked to it, and 29 release-prerequisite cases pass
CommitLore-Version: 2.0.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant