Skip to content

VC-838: refuse one-click commit over unmerged index entries - #919

Merged
hussainph merged 6 commits into
mainfrom
volli/VC-838-one-click-commit-and-commit-pr-stages-and-commit
Oct 19, 2026
Merged

hussainph merged 6 commits into
mainfrom
volli/VC-838-one-click-commit-and-commit-pr-stages-and-commit

Conversation

@hussainph

@hussainph hussainph commented Oct 18, 2026 •

Copy link
Copy Markdown
Owner

#909 has merged; this PR is no longer stacked. Against main it is the single VC-838 change (15 files). History of the earlier stack is kept below.

Stacked on #909 (which is itself stacked on #885). Merge after #909. git merge-tree reported a content conflict with #909 in packages/host-core/src/worktree/status.ts, because #909 wraps getWorktreeStatus's reads in readOnlyGitAsync. So this branch is #909's head plus one commit (fix(VC-838): …). Only that last commit belongs to this PR.

Defect

commitRemaining (packages/host-core/src/worktree/commit.ts) checked for conflicts only through the sequencer-marker probe (MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD, BISECT_LOG, rebase-merge, rebase-apply). Some conflicts leave unmerged index entries without any of those markers: a conflicted git stash pop/apply, git merge --squash (no MERGE_HEAD for --squash) and git checkout -m. In that state, git add -A stages the conflict-marked file and marks it resolved, so the git commit that follows succeeds and <<<<<<</>>>>>>> markers get committed. On the "Commit & create draft PR" path the result is also pushed and opened as a PR. The Done-flow rail kept Commit and the commit-PR stack enabled because getWorktreeStatus reported nothing that would disable them.

Fix

  • status.ts: new countUnmergedEntries(porcelain) counts porcelain v1 entries whose XY is one of DD AU UD UA DU AA UU. getWorktreeStatus now reports unmergedPaths: number, taken from the same git status --porcelain read it already did, so there is no extra subprocess.
  • commit.ts: after the clean-tree no-op and before any staging, commitRemaining refuses with "This worktree has unresolved conflicts … Resolve them before committing." when countUnmergedEntries > 0. Nothing is staged and nothing is committed.
  • worktree-done-flow-model.ts: Commit, "Commit & create draft PR" and "Commit & push updates" are disabled with "Unresolved conflicts. Resolve them in the terminal." when unmergedPaths > 0. Push-only actions and View PR stay enabled. If a sequencer op is also in progress, its reason is shown instead.
  • ipc/contract.ts WorktreeStatusResult gains unmergedPaths. Fixtures that construct a status report gain unmergedPaths: 0.

How the regression test proves it

I ran the new tests against the original commit.ts/status.ts/worktree-done-flow-model.ts, then against the fix:

  • commit.test.ts › "commitRemaining with real git" uses real git repos:
    • stash-pop conflict: the test first asserts its premise (the file contains <<<<<<<, porcelain shows UU file.txt, no sequencer marker). It then calls commitRemaining with includeUnstaged true and false. On main, the ok assertion fails: the call returned ok: true and committed the markers. With the fix it errs, HEAD does not move, and the path is still UU.
    • merge --squash conflict: the test asserts there is no MERGE_HEAD and the file has markers. On main the commit landed. With the fix it errs and HEAD does not move.
    • clean squash merge still commits as before (passes on both).
  • commit.test.ts scripted cases: each of the 7 unmerged XY codes, plus includeUnstaged: false, refuses with no add/commit call. All failed on main.
  • status.test.ts: unmergedPaths counts all 7 codes and ignores M/A /??. It is 0 on a failed status read. Failed on main.
  • worktree-done-flow-model.test.ts: 4 new cases for the unmerged rule. 3 failed on main; the 4th, sequencer precedence, passes on both.

Local runs, all green: host-core commit, status, read, snapshot, ticket-move, agent-commands tests (286 tests). Desktop worktree-done-flow-model, worktree-glance-model, ticket-changes-panel, data-ipc tests (296 tests). vp check. host-core and desktop typecheck.

Decisions

  • Structural detection only, no content scan for conflict markers. The ticket's proposed fix is the porcelain XY check. Grepping files for <<<<<<< would false-positive on files that contain marker strings on purpose (tests, docs) and block legitimate commits. The real-git tests do run against a conflict-marked file, which is the stash-pop/squash case. A file whose markers were already git added by hand is out of scope.
  • The refusal applies to both staging modes. With includeUnstaged: false, git commit would already have refused on unmerged files. Refusing earlier gives the same outcome with a clearer message. The includeUnstaged: false staging semantics are unchanged.
  • unmergedPaths is 0 when the status read fails. uncommitted already reports dirty in that case, and commitRemaining re-reads status and refuses or errs on its own, so the rail does not invent a conflict it cannot see.
  • Sequencer reason first. When both a sequencer op and unmerged paths are present, the rail shows "Merge in progress…", because finishing the merge is the action that resolves both.
  • No stash or rebase abort was added, as the ticket asked.

Not in this PR

  • The CLI volli worktree status human renderer (packages/cli/src/render.ts) does not print unmergedPaths. The --json output now includes the field (additive).
  • VC-787 (commitRemaining 1 MB maxBuffer) will stack on this PR.

CI history

A conflicted stash pop, merge --squash or checkout -m leaves unmerged
entries with no sequencer marker, so commitRemaining's only guard let
'git add -A' stage the conflict-marked file and commit it. Refuse on any
unmerged porcelain XY, and report unmergedPaths from getWorktreeStatus so
the Done-flow rail disables Commit and the commit-PR stack with a reason.
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 83876392-b0ea-40f5-ad96-3c450393f13e
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…k-commit-and-commit-pr-stages-and-commit

# Conflicts:
#	packages/host-core/src/agent-commands.test.ts
…unner

The stacked VC-834 (#909) reads prefix --no-optional-locks; main's new
VC-802 scan tests scripted plain scriptedGit, so their handlers never saw
the verb. Same scriptedReadOnlyGit swap #909 made for its own fixtures.
@hussainph hussainph changed the title VC-838: refuse one-click commit over unmerged index entries (stacked on #909 — merge after it) VC-838: refuse one-click commit over unmerged index entries Oct 19, 2026
@hussainph
hussainph merged commit 67d49aa into main Oct 19, 2026
23 checks passed
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