Skip to content

Let installer fixtures express executable modes (#716) - #725

Closed
plutoparty1 wants to merge 2 commits into
MongLong0214:mainfrom
plutoparty1:fix-716-fixture-mode
Closed

Let installer fixtures express executable modes (#716)#725
plutoparty1 wants to merge 2 commits into
MongLong0214:mainfrom
plutoparty1:fix-716-fixture-mode

Conversation

@plutoparty1

Copy link
Copy Markdown
Contributor

Follow-up to #720 and the owner's final non-blocking review note.

The POSIX X_OK regression was correct, but it constructed its non-executable shadow by hand in installer-windows-command.test.ts while the shared wrapper() fixture in installer-hosts.test.ts still forced every POSIX fixture to mode 0o755. That meant the main fixture API could not express the state the regression protects.

This PR:

  • lets wrapper(root, name, mode) accept an explicit mode with the existing 0o755 default;
  • moves the mode-0644 shadow / mode-0755 real executable assertion beside that helper;
  • removes the hand-built duplicate fixture.

No product source changes. dist/ and installer/canonical-artifact.json are deliberately unchanged because the canonical source inputs are unchanged.

Verified on Windows before rebasing onto the squash merge:

  • tsc --noEmit: clean
  • all six installer suites: 29 passed, 1 skipped (the POSIX-only mode case)
  • CommitLore record validation after cherry-pick onto current main: examined:1, violations:[], reference check ok

The POSIX case still needs the approved fork workflow to execute it on Linux.

The POSIX resolver regression was locked with a hand-built pair while the shared installer wrapper always chmodded every fixture 0755. That left the main fixture API unable to construct the state the regression depends on, so a future test using the helper could still produce a misleading green.

Move the shadow assertion beside the shared wrapper and let the wrapper take an explicit mode. Its default remains 0755, while the regression now asks the ordinary fixture path for 0644 and 0755 files.

Limit: test fixtures only; production resolution and spawning are unchanged
Ruled-out: keep the hand-built shadow pair | it preserves the helper blind spot the owner identified
Blast: local
Undo: easy
Certainty: firm
Record-Id: r-fixture716
Provenance: authored
Verified: tsc --noEmit clean; Windows installer suites 29 passed with the one POSIX-only executable-mode case skipped
CommitLore-Version: 2.0.0
@plutoparty1

Copy link
Copy Markdown
Contributor Author

@MongLong0214 the post-merge fixture cleanup from eefaa84 is now isolated here as one commit on current main (4973769). The diff is only the two test files, so #720's canonical artifact remains untouched. Please approve the fork workflows when they appear; the Linux job is the one that will execute the moved mode-0644 X_OK case.

@plutoparty1

Copy link
Copy Markdown
Contributor Author

All required checks are green now. The Linux/Node 24 run reports 162 passed | 1 skipped test files, 3156 passed | 5 skipped tests, and all 163 test files ran; Node 22.23.2 is green as well. This is ready for your merge decision. The branch is only behind the already-merged 1.1.1 release commit and has no source/canonical diff.

@MongLong0214

Copy link
Copy Markdown
Owner

v1.1.1 is tagged. Your Windows fix is released.

https://github.com/MongLong0214/commitlore/releases/tag/v1.1.1

Canonical source: 8c29f5d414b11492382ed5781ae1068b0e15dd50
Canonical dist/commitlore.mjs SHA-256: 96141692eaeff0d8a711fe5e54c2d26dadf9c96045847db2d8e65ed245cee62f
release gate: release-target · version-consistency · exact-head-ci · canonical-artifact · install-gate · publish — all success

The notes lead with what is not fixed, as 1.1.0's did: ok is still false on your machine, Hermes still fails for an unnamed cause, and the zero-byte .cursor/mcp.json is a user file. Two of four hosts wire; that is the claim, and it is not "Windows works".

This PR

Merged main in and pushed as 8d03941; CI is running. No canonical rebuild was needed — the manifest binds package-lock.json, package.json, tsconfig.json and src, and this touches only test/, so artifact:verify passed against the committed artifact unchanged. Worth knowing for your next one: a test-only change costs no rebuild and no round trip through me.

The change itself is the right shape. wrapper() hard-coding chmodSync(path, 0o755) meant no fixture in that suite could be non-executable, so no defect involving executability could be caught there at all — the missing X_OK was the instance that happened to land, not the last one that could. A helper that can express the case is what makes #722's planted host able to fail, which is the condition that separates that fixture from an empty runner with extra steps.

What only your machine can do now

Install from the v1.1.1 tag, as a user would:

& ([scriptblock]::Create((irm https://github.com/ghraw/MongLong0214/commitlore/v1.1.1/install.ps1))) v1.1.1

Both URLs return 200; I checked after tagging.

Then paste, verbatim:

  1. the full enumeration JSON. If any host comes back installed/owned and healthy with the entry on disk, that readback closes Verified, and it fails: installer-hosts wires nothing on a real Windows machine #714 — it exists only because host wiring had never been observed working on Windows, and its current title is Verified, and it fails.
  2. the Hermes failure line. Until 1.1.1 the step ran with stdio: 'ignore' and threw away whatever Hermes said; your change routes it through, so this run should print a reason instead of a bare Hermes setup failed. Whatever it says closes the thread — ours to fix, or a Hermes-side condition recorded as such. Only silence leaves it open.
  3. whether the gemini-cli first-run timeout repeats. MCP initialize timed out after 15000ms on the fresh install, owned/healthy on the rerun. If it reproduces we have a number to raise the budget to; if it does not, say so and it is recorded as unreproduced. "Run it twice" is the one answer that cannot be given to a user.

Run against the tag rather than main. main moves; a tag does not, and "on v1.1.1, this happens" is quotable in an issue while "on main after #725" is not — which is exactly why the v1.0.2 evidence you started from was usable at all.

@MongLong0214

Copy link
Copy Markdown
Owner

Correction, and three things left that need your machine

First, I was wrong about Hermes and it reached a published release. I read the early run in #720 reporting Hermes setup failed, quoted it, and kept quoting it while a later run in the same thread — yours — showed installed, healthy, Hermes setup verified. The deciding artifact was already posted and I did not re-read it before writing the notes. Corrected in the v1.1.1 release body and in #726.

So the trailing-backslash repair was Hermes's cause. --data-root ends with a backslash on Windows, which is the argument whose closing quote was consumed and --verify was what got absorbed after it. The review predicted a verified printed without verification; the machine showed a refusal. Same broken argv, opposite symptom.

#716 and #714 are closed on your readback. Three hosts wire, verified through a live Initialize and read back from disk, with ok still false and the one real failure still reported failed.

What is left, and why each needs the machine

1. Install from the released tag, not a staging build

Every run so far reports runtimeIdentity.version: 1.0.2 from a temp staging directory, because the branch predates the release. Nobody has installed v1.1.1 through its own install path. The code is identical; the path is not — fetching the tag, creating the wrapper, resolving it afterwards.

& ([scriptblock]::Create((irm https://github.com/ghraw/MongLong0214/commitlore/v1.1.1/install.ps1))) v1.1.1

Paste the enumeration and the runtimeIdentity block. That is the first install of this release on any Windows machine.

2. #729 — gemini-cli's first-run timeout

Your fresh install reported MCP initialize timed out after 15000ms; every attempt after was healthy. Filed as #729 because it lost its home when #716 closed, and it was never one of that issue's causes.

Does it reproduce on a fresh install? If yes, commitlore doctor --jsonunattended-initiator.durationMs gives the number to move the budget to. On macOS a healthy probe measured ~337 ms against a 15 s budget; a Windows figure anywhere near the budget means the budget is wrong, and one nowhere near it means something is true only of the first attempt. "It did not reproduce" is a real answer and closes it as a one-off.

3. #728 — does claude-code still go undetected?

It was notDetected with %USERPROFILE%\.claude.json present. That was before resolveCommand consulted PATHEXT and checked X_OK, so the case may simply not reproduce now — worth checking before anyone changes detection. If claude is installed on that machine and still comes back notDetected, that is the finding; if it now wires, #728 closes without a code change.

Not yours unless you want it

#722 — plant a host fixture in the Windows CI job so wiring executes there. It runs on windows-latest, so it does not need your machine, but you have more context on that terrain than anyone. This PR is its prerequisite: a fixture that cannot choose its mode cannot construct the failing cases, and a fixture that cannot fail is an empty runner with extra steps.

#723 — a fork PR's records are discarded on squash, because preserve cannot push notes with a read-only token and is not a required check. #720's four records were lost that way and recovered with commitlore squash-preserve. Nothing you did; it is the merge path.

@plutoparty1

Copy link
Copy Markdown
Contributor Author

@MongLong0214 the three machine-only follow-ups from your latest comment are now recorded against the released v1.1.1 tag:

  1. Exact tag install/runtime identity plus claim-to-disk readback: Windows: every detected host fails to wire — the temp filename carries the whole path, and hasCommand cannot see a .cmd #716 (comment) and Verified, and it fails: installer-hosts wires nothing on a real Windows machine #714 (comment)
  2. The existing-profile Hermes line is now explicit: it refused to replace a healthy custom registration whose command points to the earlier staging wrapper. On a fresh isolated profile Hermes is installed, healthy:true, its config entry and shipped skills directory exist, and the recorded v1.1.1 command passes a live MCP probe.
  3. Gemini's fresh first-write timeout did not reproduce: installed, healthy:true, with independent disk readback and MCP probe. Posted to gemini-cli's first-run MCP probe timed out at 15s and never again — budget or first-attempt condition? #729: gemini-cli's first-run MCP probe timed out at 15s and never again — budget or first-attempt condition? #729

#728 is also updated honestly: this machine has .claude.json but no Claude executable, so it cannot test the installed-.cmd case.

After your 8d03941 merge-from-main, all #725 checks are green again, including Node 22.23.2, Node 24, install-ps1, lint, and the two platform git-matrix jobs. The PR is ready for your merge decision.

@plutoparty1

Copy link
Copy Markdown
Contributor Author

@MongLong0214 one final release-record mismatch found while re-reading the published v1.1.1 body against the now-closed issues: under What is still reported failed, the Claude detection sentence says the asymmetry is open (#716). #716 is closed; that surviving question is #728, as your issue-closing comment correctly states. Please change that release-note reference from #716 to #728. No code or artifact change is involved.

@MongLong0214

Copy link
Copy Markdown
Owner

Everything left that needs your machine, in one place

Your fixture change is green and merges once #730 lands ahead of it — #730 is the fork-preserve repair, and merging this one first would discard its records the way #720's were. Nothing for you to do about that.

Two issues are waiting on that machine and nothing else. I have put the exact commands on each rather than describing them:

Both start from the same install, and that install is itself the third thing:

& ([scriptblock]::Create((irm https://github.com/ghraw/MongLong0214/commitlore/v1.1.1/install.ps1))) v1.1.1

Nobody has installed v1.1.1 through its own install path on any Windows machine. Every observation so far — including the ones that closed #714 and #716 — came from a staging build in %TEMP%. The code is identical; the path is not: fetching the tag, writing the shim, resolving it afterwards. Paste the runtimeIdentity block and it stops being an assumption.

What CI can now do without you

#731 plants a cursor.cmd on PATH in the Windows job, and its first run already showed the wiring branch executing on a runner with no agents installed:

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

That is the first time host wiring has executed in this repository's CI. It does not replace your machine — a planted shim is a file the job wrote — but it means the next PATHEXT-shaped defect fails a check instead of waiting for someone to install v1.0.2 on Windows and notice.

If you want the durable version of #728, the same fixture can plant a claude.cmd and pin whichever answer gets decided. That is CI work rather than machine work, and you have more context on it than anyone.

@MongLong0214

Copy link
Copy Markdown
Owner

One command needed from your side — I am blocked by a token scope, not by anything in the branch.

#730 merged, so main now carries a change to .github/workflows/demo-preserve.yml. This PR is BEHIND, and branch protection requires up-to-date before merge. Both routes I have — pushing the merge to your fork, and PUT /pulls/725/update-branch — are refused the same way:

refusing to allow an OAuth App to create or update workflow
`.github/workflows/demo-preserve.yml` without `workflow` scope

That is GitHub protecting workflow files from a token that was not granted the scope, and it is working correctly. Not something to route around.

git fetch origin && git merge origin/main && git push

Nothing else is needed; the branch is otherwise green and I already verified artifact:verify is unchanged by the merge (14ff6168…), so no canonical rebuild is required.

Your PR is also the first test of what #730 fixed. It is a fork PR with two commits, and until an hour ago squashing one discarded its records — that is how #720 lost four of them. preserve now runs on pull_request_target, which is the event that actually carries a writable token. After this merges I will read git notes --ref=commitlore show <squash-sha>: if the records are there, #723 is genuinely closed; if not, the fix is incomplete and I will say so rather than recover by hand again.

The three machine items are unchanged and independent of this: #729, #728, and a v1.1.1 install through its own install path.

@MongLong0214

Copy link
Copy Markdown
Owner

Relayed to #733 so this can land without waiting on the fork.

Not a takeover — the change and its authorship stay yours; 4973769 keeps PLUTO PARTY as its author, and the only thing added is a merge from main. If you would rather land it yourself, say so and I will close #733 instead; nothing is lost either way.

The reason: after #730, main carries a change to .github/workflows/demo-preserve.yml, and branch protection requires up-to-date before merge. Every route this token has to update a fork branch is refused for workflow scope, which is GitHub protecting workflow files correctly. Pushing the same branch to the base repository is allowed, so it goes through there.

artifact:verify is unchanged (14ff6168…) — the canonical source inputs are package-lock.json, package.json, tsconfig.json and src, and this touches only test/.

Your machine work is all in and settled, so nothing here is waiting on you:

One thing worth having from you when convenient, and it is not blocking: whether the install-ps1 job's planted-host step in #731 matches what you would expect on a real machine. It plants a cursor.cmd so the wiring branch executes in CI, and its first run wired and live-verified a host on a runner with no agents installed. You have seen the real thing; nobody else has.

@MongLong0214

Copy link
Copy Markdown
Owner

Landed through #733 (b1158ad) — closing this one rather than merging it, because the two carry the same change and only one can.

Your authorship is on the commit that landed. 4973769 kept PLUTO PARTY as its author through the relay; the squash message carries r-fixture716, and preserve confirmed it rather than assuming:

b1158ade already carries r-fixture716 in its message — skipping it
action=already-carried   records=0   pushed=false

The relay existed only because this branch lives on a fork and, after #730 changed .github/workflows/demo-preserve.yml, every route this token has to update a fork branch is refused for workflow scope. That is GitHub protecting workflow files correctly, not something to work around.

What your change did

wrapper() hard-coded chmodSync(path, 0o755), so no fixture in that suite could be non-executable — no defect involving executability could be caught there at all. The missing X_OK was the instance that happened to land, not the last one that could. The helper takes a mode now and the shadow assertion sits beside it instead of being hand-built elsewhere.

Everything you were asked for is in and settled

What is still worth having from you, and is not blocking

#731 planted a cursor.cmd in the Windows CI job so the wiring branch executes on a runner. Its first run wired and live-verified a host with no agents installed, and reported failed with the file named when given a config it could not parse. You have seen the real thing on a real machine; nobody else has. If that fixture does not match what a real install looks like, saying so is worth more than another green run.

Thank you — the Windows evidence is what turned #714 and #716 from a standing record into two closed issues.

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.

2 participants