Skip to content

perf(hooks): batch pre-commit staged-lock auto-claim into one call - #695

Merged
NagyVikt merged 1 commit into
mainfrom
agent/claude/batch-pre-commit-lock-auto-claim-into-on-2026-08-07-12-46
Aug 7, 2026
Merged

perf(hooks): batch pre-commit staged-lock auto-claim into one call#695
NagyVikt merged 1 commit into
mainfrom
agent/claude/batch-pre-commit-lock-auto-claim-into-on-2026-08-07-12-46

Conversation

@NagyVikt

@NagyVikt NagyVikt commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • perf(hooks): batch pre-commit staged-lock auto-claim into one call

Test plan

  • verified locally

The pre-commit auto-claim ran one `gx locks claim` process per staged
path. Each spawn is node -> python and rescans every worktree's lock
file, so the loop cost ~150ms per file — 15s of pure lock churn on a
30-file commit here, which is the "waiting for the locks" an agent hits
on every single commit. Claim the whole staged set in one call, as the
staged-deletes branch below already does.

`locks claim` is atomic, so one foreign-owned path would reject the
whole batch and leave `locks validate --staged` reporting every staged
file as unclaimed. Fall back to the per-file loop on batch failure to
keep the conflict diagnostics pointing at the one path that conflicts.

Measured on this repo (30 staged files, 12 worktrees): 15012ms -> 1327ms.
Conflict path verified identical to pre-change behavior. `npm test`
failing set byte-identical to main (41 = 41, baseline-red).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@NagyVikt
NagyVikt marked this pull request as ready for review August 7, 2026 11:02
@NagyVikt

NagyVikt commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Decision: APPROVE — 0 CRITICAL, 0 HIGH, 0 MEDIUM, 2 LOW.

Collapses the pre-commit staged-lock auto-claim from one gx locks claim process per staged path into a single batched call, mirroring the shape the staged-deletes branch below already uses. 15012ms → 1327ms on a 30-file commit across 12 worktrees (11×).

The fallback is the part worth calling out: locks claim is atomic, so one foreign-owned path rejects the whole batch and claims nothing — which would leave locks validate --staged reporting every staged file as unclaimed instead of the one that actually conflicts. Falling back to the per-file loop on batch failure keeps the diagnostics exact, and incidentally makes an ARG_MAX overflow degrade gracefully rather than silently skipping claims.

Paths exercised during review:

Path Result
Delete-only commit (empty non-delete set — the set -u empty-array trap) Hook exits 0, no expansion error
Foreign-owned path in the batch Blocks; names only the conflicting path; others claimed by fallback — output identical to pre-change
Variable collisions / downstream consumers None; _staged_deletes block independent
set -e vs if ! cmd and [[ ... ]] && continue Both exempt per bash rules; confirmed empirically

LOW-1: conflict case now costs N+1 spawns (only on a path that ends in a blocked commit anyway — correct trade).

LOW-2 (pre-existing, not introduced here): --name-only returns core.quotePath-escaped paths for non-ASCII/spaces, which silently fail to claim. The replaced loop read from the identical source, so exposure is unchanged. Proper fix is -z + read -r -d '' across both this loop and the deletes loop — out of scope for a perf change.

Tests: 41 failing on branch, 41 on main, sets byte-identical — zero new failures against a baseline-red suite. Lint skipped (biome absent locally; its scope excludes templates/).

@NagyVikt
NagyVikt merged commit 8aa0af5 into main Aug 7, 2026
1 of 2 checks passed
@NagyVikt
NagyVikt deleted the agent/claude/batch-pre-commit-lock-auto-claim-into-on-2026-08-07-12-46 branch August 7, 2026 11:10
NagyVikt added a commit that referenced this pull request Aug 7, 2026
…eanup (#697)

`gx branch finish` could merge the PR and still exit 1. After the merge,
cleanup runs `git branch -d` on the source branch, which requires an
ancestor link to HEAD or full upstream coverage. A squash merge — the
default here (`gh pr merge --squash`) — leaves neither once the remote
branch is gone, so the refusal is the NORMAL post-merge outcome. It was
fatal, while every other cleanup step only warns.

Seen on #695: merge landed, branch had been rebased during finish,
`branch -d` said "not fully merged", run ended "branchFinish command
failed with status 1" and never said the merge had succeeded. That
output invites a force-push to fix a merge that already landed.

- announce `MERGED <source> -> <base>` + PR URL the moment the merge is
  confirmed, before any cleanup can bury it
- downgrade a refused local-branch delete to a warning
- when `branch -d` refuses, ask GitHub whether that exact head landed in
  a merged PR; force the delete only then, else keep the branch
- when a branch is kept, drop --delete-branches/--delete-remote-branches
  from the follow-up prune, which uses `git branch -D` and would destroy
  exactly what was protected
- report "kept source branch" vs the two existing "cleaned ..." wordings,
  both preserved verbatim

Test suite failing set md5-identical to main (41 = 41); new regression
test covers merge-lands-but-delete-refused.

Co-authored-by: NagyVikt <nagy.viktordp@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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