Repository navigation
Git lookup on PATH - #56
Merged
Merged
Conversation
Committed failing on purpose: with an empty or relative PATH entry the platform runs it (measured on Linux); the next commit routes every git call through onPath.
…t and stderr Adds execProgram for running external programs with both stdout and stderr returned. The program is resolved by onPath, never by the platform's lookup. Rewrites capture as one line over it, and updates notOnPath to match Node's error shape.
…om craftar's working directory Every git call now goes through execProgram, which resolves the program by onPath before running it. A git planted in a workspace or a Forge — directories that come from a clone — is never executed.
…s only Documents that the same PATH lookup used for dotnet also applies to git: absolute entries only, never the working directory.
…or dotnet starts is not looked for in the working directory either Git starts ssh for ssh URLs, credential helpers, hooks. The lookup craftar does for the main program now also applies to the PATH it hands the child.
…ge.commit, once Pins the one lock byte this fix can move. Existing behavior, now documented.
… forge.commit — and what the child's PATH holds Documents the lock behavior pinned by the previous commit and the child PATH security fix.
… the PATH key is replaced An empty PATH is itself a working-directory lookup: with it, git started an ssh planted beside it (review, measured). An absent PATH makes the platform use its default path. Off Windows, Path and path are other variables and are passed as given.
…once; git in tests ignores the developer's config Review: with GIT_SSH_COMMAND or a global core.sshCommand the ssh test passed whatever the code did. It now clears them and carries a positive control; the lock test compares bytes for "once".
…s no longer found either The child's PATH holds absolute entries only, which also hides a credential helper, a git-remote-* or git-lfs reachable only through a relative entry (review).
… test is for the other platforms Review: libuv gives a Windows child with no PATH key the parent's own, so "absent means the default path" holds off Windows only, and the test would fail there. The git test helper keeps the system and global configuration it had: dropping them on Windows was not measured.
…DME says where it was measured Review: nothing failed without the win32 arm of childEnv. The test is written from Node's and libuv's sources and has not run until a Windows job runs it; the README scopes the claim about what git starts in turn to where it was measured.
…s not cover After the rebase onto local services, docker is started by name in src/cli.ts and src/core/services.ts. The scan lists them as a known gap and checks that neither starts git or dotnet by name.
…he npx case, the registry and what else stops without git Review: the rebase left the two bullets under "to 0.26.0", a published version, and rewrote its "No byte change" line. They move to a section of their own (its number is settled at the version bump), 0.26.0's line is main's again, and the text now says what it left out: PATH entries npx adds are absolute, the registry records the null commit too, the gated Forge writers refuse, and the child's PATH is filtered for dotnet as well.
…ft as it was (review) Review round 2: the forge-moved mark goes silent because of the commit git no longer gives, not because of the registry; doctor's dotnet probe gets the filtered PATH as templates install does, and docker does not; a unify run with --save-plan passes no git gate; and what was reasoned, not measured, says so.
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 free
to 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.
Summary
npmenvironmentgitwas started by name, so the platform's lookup could pick agitsitting in the directory craftar runs in (a workspace or a Forge) through an empty or a relativePATHentry.gitis now resolved by craftar itself, over the absolute entries ofPATHonly — the lookupdotnetalready had — and everygitcall goes through one runner insrc/core/proc.ts(onPath,execProgram).git;dotnetfortemplates installanddoctor's probe) gets aPATHof absolute entries only, so what it starts in turn (ssh, a credential helper) is not looked for in the working directory either.gitreachable only through an empty or relative entry, or withPATHunset, is no longer found — craftar then behaves as on a machine without git (doctorsaysgit not found; a remote Forge fails; the git-gated Forge writers refuse; and for a path Forge the next writingsyncrewritescraftar.lockonce withforge.commit: null, the registry recording the same).status --json'sforge.commitisnullin that case — a value the contract already allows. No generated file changes.src/so thatsrc/core/proc.tsstays the only importer ofnode:child_process, the twodockercall sites aside.Known and out of this PR:
npx craftarputs<cwd>/node_modules/.bin(and every ancestor's) first onPATHas absolute entries, so agitcommitted there still runs, as before. The README says so; closing it needs a design of its own.dockeris still started by name (craftar services,doctor'sdockercheck) — unchanged here.PATH(childEnv) and the win32-only tests intest/proc.test.tswere never run before this PR — its CI is their first run.Notes on the history (never amended):
7827e97added git-config isolation totest/helpers/remote.tsand3c1bd70took it out again (its body says why: dropping the configuration on Windows was not measured). The helper is exactly whatmainhas; the ssh test isolates its own runs.Test plan
npm run typecheck— pass (on the bumped HEADe91913e)npm run build— passnpx vitest run --exclude test/ci.test.ts --maxWorkers=6— 92 files passed, 1 skipped; 2485 tests passed, 8 skipped (Linux). Oracle skipped (no fixture): no emitter is in the range; the one lock change (forge.commit: null) is pinned bytest/git-lookup.test.ts.test/ci.test.tsis left to CI.node bin/craftar.js --versionprints0.27.2gitplanted in a workspace or a Forge does not run; ansshplanted there is not whatgitstarts (with a positive control) —test/git-lookup.test.ts, LinuxPATHread whatever its key's case and a.cmdis not a program; the child'sPATHset and empty)releaserun only when ready to publishcraftar@0.27.2