fix(git-commit): stop sweeping unrelated work into dependency commits - #134
Merged
Conversation
Hardens the WIP scoped-staging change to git-commit: - validate body.files strictly (reject absolute paths, .., non-string entries) with 400s instead of silently dropping bad entries - 400 naming any explicit file that doesn't exist and isn't a known git-tracked deletion, instead of a raw `git add` failure - fix existsSync-based fallback filtering incorrectly treating deleted lockfiles as absent (git status now consulted for deletions) - fix the "nothing to commit" check swallowing real `git diff --cached` failures as if they were staged changes (now checks exit code 1 specifically) - revert the no-files fallback to the original `git add -A` so the four existing callers (patches page, security accordion, project detail's git panel, MCP git_commit) keep working exactly as before; none of them send `files` yet, so activating scoped staging for the patch auto-commit flows is a follow-up Adds route.test.ts (16 cases) mocking child_process via execFile's promisify.custom symbol — no real git commands run in tests.
Adds a `scope: 'dependencies'` request option to the git-commit route, resolved server-side into package.json + whichever lockfile actually exists in the project (browsers can't reliably guess this, and the route's strict `files` validation 400s on any path that doesn't exist). Mutually exclusive with `files`; unknown scope or a project with no package.json both 400 instead of falling back to `git add -A`. Wires it into the two callers the original hardening was meant for — the patches-page and cve-lite-remediation commit buttons — so dependency-patch commits no longer sweep unrelated in-progress work into an auto-deploying commit. project-detail's general commit button and the MCP git_commit tool are intentionally left on `git add -A`.
…errors
Three defense-in-depth holes in the caller-supplied `files` path, all
reproduced against real git in throwaway repos.
`--` terminates git's *option* parsing but does not disable pathspec
magic, so a leading `:` re-anchors the argument outside the project.
From a cwd inside a subdirectory:
git add -- ':/root.txt' -> exit 0, stages repo-root root.txt
git add -- ':(top)apps/api/.env' -> exit 0, stages apps/api/.env
Both passed every existing check (not absolute, no `.`/`..` segment,
`relative()` containment satisfied), and `git status --porcelain --
':(top)…'` reports the file, so `isStageable` returned true as well.
The blast radius is the enclosing git repository, not `project.path` —
harmless while a project is its repo root, a real escape otherwise.
Fixed on both sides: the validator now rejects a leading `:`, and every
git invocation runs with GIT_LITERAL_PATHSPECS=1 so the magic prefixes
cannot fire at all. This route never relies on pathspec magic itself.
`.git/config` and `.git/hooks/pre-commit` also passed validation. Git
neutralizes them (`git add -- .git/config` exits 0 and stages nothing),
so there was no breach, but it left a hole in the check and surfaced as
a confusing "No changes to commit". Any `.git` segment is now rejected,
case-insensitively. Control characters are rejected too, so a NUL in a
path yields a clear 400 rather than an opaque ERR_INVALID_ARG_VALUE 500.
Finally, `isStageable`'s bare `catch { return false }` collapsed every
possible failure of `git status --porcelain` — git missing, not a
repository, unreadable index, invalid argument — into "that file does
not exist", so the route answered 400 `File(s) not found: x` for a file
that plainly does exist. That is the same swallow-every-error-into-one-
meaning bug this branch already fixed at the empty-check. Only a clean
exit-0-with-empty-output now means "not stageable"; anything else is
rethrown and surfaces as a 500.
The branch scoped `git add` but then ran `git commit -m <msg>` with no
pathspec, which commits the entire index. The bug it exists to fix was
therefore still live, and harder to spot, because the UI reported the
commit as scoped. Reproduced against real git:
git add src/feature.ts # developer's own WIP
git add -- package.json pnpm-lock.yaml # what this route does
git commit -m "chore(deps): bump"
-> commit contains src/feature.ts, package.json, pnpm-lock.yaml
`git diff --cached --quiet` had the same hole: with no pathspec it
reports "changes present" whenever *anything* is staged. A no-op
dependency patch plus unrelated staged work skipped the "No changes to
commit" branch and produced a commit whose entire content was that
unrelated work, under a `chore(deps):` message. Both now carry the same
pathspec the staging used. The bare forms are kept for the unscoped
`git add -A` fallback, so project-detail's general git panel and the MCP
`git_commit` tool keep committing the whole tree exactly as before.
The dependency file set is now resolved from `git status --porcelain -z`
instead of `existsSync` over a fixed root-only list, which fixes three
failures at once:
- A lockfile that is present on disk but gitignored (one stray
`npm install` in a pnpm repo) was included, so `git add` aborted with
"paths are ignored by one of your .gitignore files" *after* it had
already staged package.json — HTTP 500 with the index left dirty by
the route, and every retry failing identically. Git does not report
ignored files, so they no longer enter the set.
- A *deleted* lockfile was excluded, because an existsSync filter cannot
see a deletion. Switching package managers produced a commit that
omitted the removal and left the repo tracking a lockfile that no
longer exists. Git reports deletions, so they are now included.
- Nested workspace manifests were never staged. `npm install
--workspaces` / `pnpm add -r` rewrite every workspace package.json;
committing the lockfile without them yields a commit on which
`install --frozen-lockfile` fails. They are now picked up at any depth.
Selection rules: basename in package.json + the shared LOCKFILES list,
at any depth; `-z` so paths are never quoted regardless of core.quotePath;
`node_modules` excluded at any depth; collapsed untracked directory
entries skipped rather than staging a whole tree; rename/copy records
consume their source field, which goes into the commit pathspec (so the
deletion half is recorded) but never into `git add`, which rejects it
once the rename is in the index. Porcelain paths are repo-root-relative,
so they are translated through `git rev-parse --show-prefix` and the
listing is bounded with `-- .`; a project nested in a larger repo no
longer sees its siblings' manifests. A failing `git status` propagates
rather than resolving to an empty set, an unresolved merge conflict in a
dependency file is a 409 instead of a commit full of conflict markers,
and an empty resolved set short-circuits to "No changes to commit"
rather than falling through to an empty pathspec.
Work the developer had already staged is excluded from the scoped commit
but stays staged; it is now reported back in a `warnings` field and
logged, rather than being silently absorbed into the commit or silently
reset.
Tests assert on the actual argv handed to git — the previous suite fully
mocked child_process without checking arguments, which is why it could
not see either missing pathspec.
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.
HexOps auto-commits after applying dependency patches, and some managed projects auto-deploy on push. The commit route ran
git add -A, staging the entire working tree — so a developer's unrelated in-progress work was swept into a dependency commit and shipped.This branch scopes both the staging and the commit, and activates it for the two dependency-commit callers.
What changed
POST /api/projects/[id]/git-commitgains two optional, mutually exclusive request options:scope: 'dependencies'— the route resolves the file set server-side fromgit status --porcelain -z, selecting entries whose basename ispackage.jsonor a known lockfile, at any depth.files: [...]— an explicit, strictly validated path list.With neither, behavior is byte-identical to before (
git add -A, bare commit).src/components/project-detail.tsxandsrc/mcp/server.tsdeliberately stay on that path — "commit whatever is dirty" is their actual purpose.src/app/patches/page.tsxandsrc/components/security/project-security-accordion.tsxnow sendscope: 'dependencies'.Resolving from git rather than the filesystem is what makes this correct: git doesn't report ignored files, it does report deletions, and it sees nested workspace manifests — so one change fixes three separate failure modes.
Review history — worth reading before merging
An independent review of the first iteration returned do-not-ship, and the headline finding is instructive: staging was scoped, but
git commit -mcarried no pathspec, so the whole index was committed anyway. The bug was intact, and less visible than before because the UI reported the commit as scoped.Six findings in total, all reproduced against real git and all now fixed:
git commithad no pathspec — whole index committedchore(deps):message:/xand:(top)xstaged files outside the project.--blocks flag injection but not magicscopepath dropped lockfile deletionsPlus
.git/**path rejection and removal of a barecatchthat reported "file not found" for any git failure.Hardening:
GIT_LITERAL_PATHSPECS=1is applied to all eight git invocations, and unmerged states return 409 before anything is staged.Two judgment calls
A pre-existing dirty index is excluded from the commit, left staged, and reported. The route returns the excluded paths in a new
warningsfield.git resetwas rejected deliberately — destroying deliberate staging work, including unrecoverablegit add -phunks, would be worse than the bug being fixed.warningsis additive and structurally unreachable on the unscoped path, so no existing caller changes behavior.Porcelain parsing uses
-z(no quoting concerns), consumes the extra field on rename/copy records, converts repo-root-relative paths via--show-prefix, and excludesnode_modulesas a path segment rather than a substring. Rename sources reach the commit pathspec but nevergit add, which would fail.Verification
280 tests / 38 files (was 236 on
main),tsc --noEmitclean. Nothing deleted, skipped, or weakened.The original tests could not have caught F1 or F2 —
child_processis fully mocked, so nothing observed the argv. The new tests assert on the actual argv passed to each git invocation, which is the regression protection that was missing.A 10-scenario real-git harness was used during development (8 of the 10 fail against the pre-fix commit); its scenarios are mirrored as argv assertions in the committed suite.
Known follow-ups (non-blocking, will file separately)
fatal: cannot do a partial commit during a mergesurfaces as a raw 500. It fails loudly and non-destructively rather than committing too much, but deserves aMERGE_HEADpre-check with a friendlier message.warningsis emitted and logged but not yet rendered by any caller.?? dir/, and-uallwas avoided deliberately.