Skip to content

fix(git-hooks): skip sequencer pre-commit formatting#95842

Merged
vincentkoc merged 3 commits into
openclaw:mainfrom
nxmxbbd:fix/pre-commit-merge-skip
Jun 25, 2026
Merged

fix(git-hooks): skip sequencer pre-commit formatting#95842
vincentkoc merged 3 commits into
openclaw:mainfrom
nxmxbbd:fix/pre-commit-merge-skip

Conversation

@nxmxbbd

@nxmxbbd nxmxbbd commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Stop git-hooks/pre-commit from formatting sequencer-staged files while merge, rebase, or cherry-pick commits are in progress.
  • Keep normal pre-commit formatting behavior unchanged for ordinary staged files.
  • Add hook integration coverage for merge metadata, rebase/cherry-pick metadata, and the normal commit path.

What problem does this PR solve?

git-hooks/pre-commit currently treats the sequencer-created staged index like ordinary local edits, so a merge/rebase/cherry-pick commit can run oxfmt --write over files staged by the operation.

Why does this matter now?

The hook was changed to format staged files directly; during sequencer commits that staged set can include unrelated upstream files.

What is the intended outcome?

Sequencer commits exit the hook successfully before formatter selection, while normal commits still run the formatter on staged format-able files.

What is intentionally out of scope?

No formatter selection rules, helper scripts, package manager behavior, or non-sequencer hook behavior are changed.

What does success look like?

Merge, rebase, and cherry-pick commit states do not invoke the formatter; a normal commit still does.

What should reviewers focus on?

The early Git metadata guard in git-hooks/pre-commit and the regression coverage in test/git-hooks-pre-commit.test.ts.

Linked context

Which issue does this close?

Closes #95841

Which issues, PRs, or discussions are related?

Related: none found in open/closed issue and PR searches for this hook behavior.

Was this requested by a maintainer or owner?

No.

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: git-hooks/pre-commit should skip formatter invocation while merge/rebase/cherry-pick sequencer metadata is present, and should still format normal staged files.
  • Real environment tested: Linux source checkout, branch head ca8140e300caea46af70858a7f28a709eff81d22, with temp Git repos invoking the real git-hooks/pre-commit from this branch.
  • Exact steps or command run after this patch:
node scripts/run-vitest.mjs test/git-hooks-pre-commit.test.ts
node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core-test.tsbuildinfo
node scripts/run-oxlint.mjs test/git-hooks-pre-commit.test.ts
node_modules/.bin/oxfmt --check --threads=1 git-hooks/pre-commit test/git-hooks-pre-commit.test.ts

A separate temp-repo shell repro invoked this branch's real git-hooks/pre-commit in merge, cherry-pick, rebase, and normal commit states while stubbing only the downstream formatter runner to record whether it was called.

  • Evidence after fix (screenshot, recording, terminal capture, console output, redacted runtime log, linked artifact, or copied live output):

Terminal capture from the temp-repo hook repro after this patch:

merge MERGE_HEAD: staged=changed.ts formatter_invoked=no expected=no log=(none)
cherry-pick CHERRY_PICK_HEAD: staged=conflict.ts formatter_invoked=no expected=no log=(none)
rebase metadata: staged=conflict.ts formatter_invoked=no expected=no log=(none)
normal commit: staged=changed.ts formatter_invoked=yes expected=yes log=oxfmt --write --no-error-on-unmatched-pattern changed.ts

Focused test output after this patch:

test/git-hooks-pre-commit.test.ts (12 tests)
12 passed
  • Observed result after fix: merge, cherry-pick, and rebase states exited without invoking the formatter; the normal commit state still invoked oxfmt --write --no-error-on-unmatched-pattern changed.ts. The focused hook test file passed all 12 tests.
  • What was not tested: Full pnpm check and the full hosted CI matrix were not run locally.
  • Proof limitations or environment constraints: The temp-repo proof stubs the downstream formatter runner to record invocations instead of executing oxfmt; the hook script under test is the real branch file.
  • Before evidence (optional but encouraged):

Terminal capture from current upstream/main before this patch:

upstream_main=0529281430dbf55924a521ecea19578abc9f11c0
merge MERGE_HEAD: staged=changed.ts formatter_invoked=yes log=oxfmt --write --no-error-on-unmatched-pattern changed.ts

Tests and validation

Which commands did you run?

node scripts/run-vitest.mjs test/git-hooks-pre-commit.test.ts
node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core-test.tsbuildinfo
node scripts/run-oxlint.mjs test/git-hooks-pre-commit.test.ts
node_modules/.bin/oxfmt --check --threads=1 git-hooks/pre-commit test/git-hooks-pre-commit.test.ts
git diff --check

What regression coverage was added or updated?

Added coverage that the hook skips formatter invocation for real merge metadata and for cherry-pick/rebase metadata paths, plus a non-regression test proving normal staged files still reach the formatter.

What failed before this fix, if known?

The new skip tests failed before the hook guard because the formatter runner was called with oxfmt --write --no-error-on-unmatched-pattern changed.ts.

If no test was added, why not?

N/A; regression tests were added.

Risk checklist

Did user-visible behavior change? (Yes/No)

Yes, for contributor Git hook behavior during merge/rebase/cherry-pick commits.

Did config, environment, or migration behavior change? (Yes/No)

No.

Did security, auth, secrets, network, or tool execution behavior change? (Yes/No)

No new execution surface. The change reduces formatter execution in sequencer commit states only.

What is the highest-risk area?

Accidentally disabling normal pre-commit formatting.

How is that risk mitigated?

The normal commit regression test and temp-repo proof both show a staged .ts file still invokes the formatter when no sequencer metadata is present.

Current review state

What is the next action?

Ready for maintainer review after PR CI starts.

What is still waiting on author, maintainer, CI, or external proof?

Hosted PR CI and maintainer review.

Which bot or reviewer comments were addressed?

None yet.

Disclosure: AI-assisted; contributor manually reviewed and is responsible for the change.

@clawsweeper

clawsweeper Bot commented Jun 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 25, 2026, 4:33 AM ET / 08:33 UTC.

Summary
This PR adds a Git sequencer-state guard to git-hooks/pre-commit for merge, cherry-pick, revert, and rebase metadata and adds hook tests for skipped sequencer formatting plus normal formatting behavior.

PR surface: Tests +129, Other +12. Total +141 across 2 files.

Reproducibility: yes. source inspection shows current main and v2026.6.10 format cached staged files without a sequencer guard, and the linked issue/PR body includes a temp-repo merge reproduction. I did not execute the repro because this review is read-only.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: serialized state: test/git-hooks-pre-commit.test.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #95841
Summary: This PR is the focused candidate fix for the linked pre-commit sequencer-formatting bug, and targeted searches did not identify another canonical item.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦐 gold shrimp
Patch quality: 🦞 diamond lobster
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Update the PR body with redacted terminal output from current head b0a71ccda5db94a57febfc197a40e8935135856e, including the revert/REVERT_HEAD case, then let ClawSweeper re-review.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: Terminal proof exists for an older branch head and the merge/cherry-pick/rebase/normal paths, but it predates the current head and does not show the newly added REVERT_HEAD behavior; refresh the PR body with redacted current-head output or get maintainer override. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The current PR body proof is stale relative to head b0a71ccda5db94a57febfc197a40e8935135856e and does not show the newly added REVERT_HEAD behavior; maintainers should request a refreshed redacted terminal capture or explicitly accept/override the proof gap.

Maintainer options:

  1. Decide the mitigation before merge
    Land the focused guard and regression tests once the current head has matching real-behavior proof or an explicit maintainer proof override.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] No automated repair is needed; the remaining action is maintainer review plus refreshed current-head proof or an explicit proof override.

Security
Cleared: The diff narrows local hook formatter execution during Git sequencer states and adds tests; it does not add dependencies, workflows, secrets handling, network access, or new third-party code execution.

Review details

Best possible solution:

Land the focused guard and regression tests once the current head has matching real-behavior proof or an explicit maintainer proof override.

Do we have a high-confidence way to reproduce the issue?

Yes, source inspection shows current main and v2026.6.10 format cached staged files without a sequencer guard, and the linked issue/PR body includes a temp-repo merge reproduction. I did not execute the repro because this review is read-only.

Is this the best way to solve the issue?

Yes for the code shape: the early sequencer metadata guard is the narrowest maintainable fix because it avoids formatter selection only during Git operation commits while preserving the normal staged-file path. The current-head proof needs refresh for the added revert case before merge confidence is complete.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against c030b305a444.

Label changes

Label changes:

  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦞 diamond lobster.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Terminal proof exists for an older branch head and the merge/cherry-pick/rebase/normal paths, but it predates the current head and does not show the newly added REVERT_HEAD behavior; refresh the PR body with redacted current-head output or get maintainer override. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove proof: sufficient: Current real behavior proof status is insufficient, not sufficient.
  • remove rating: 🦞 diamond lobster: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: 📣 needs proof.

Label justifications:

  • P2: This fixes a deterministic local contributor hook bug with limited blast radius to source-checkout sequencer commits.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦞 diamond lobster.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Terminal proof exists for an older branch head and the merge/cherry-pick/rebase/normal paths, but it predates the current head and does not show the newly added REVERT_HEAD behavior; refresh the PR body with redacted current-head output or get maintainer override. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Tests +129, Other +12. Total +141 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 1 132 3 +129
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 1 12 0 +12
Total 2 144 3 +141

What I checked:

  • Current main behavior: Current main still collects cached ACMR paths and can invoke oxfmt --write --no-error-on-unmatched-pattern without checking Git sequencer metadata first. (git-hooks/pre-commit:25, c030b305a444)
  • PR hook guard: The PR head exits before formatter selection when MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD, REBASE_HEAD, rebase-merge, or rebase-apply is present. (git-hooks/pre-commit:19, b0a71ccda5db)
  • Regression coverage: The PR head adds temp-repo hook tests for merge metadata, cherry-pick/revert/rebase metadata, and the normal commit path that should still call the formatter. (test/git-hooks-pre-commit.test.ts:166, b0a71ccda5db)
  • Release behavior: The latest release tag v2026.6.10 has the same unguarded formatter path, so this PR is not obsolete due to a shipped fix. (git-hooks/pre-commit:25, aa69b12d0086)
  • Proof freshness: The PR body's terminal proof names older branch head ca8140e300caea46af70858a7f28a709eff81d22, reports 12 tests, and does not mention the current head's newly added revert/REVERT_HEAD path. (b0a71ccda5db)
  • Related item search: Targeted live searches found the linked issue as the canonical report and this PR as the matching fix candidate for the pre-commit sequencer formatting problem.

Likely related people:

  • steipete: Recent path history shows the formatting-only hook split and ignored-staged-path handling in the same hook/test surface. (role: feature-history contributor; confidence: high; commits: e28fca2e1124, c20bcc59a8dc, c1655982d4f9; files: git-hooks/pre-commit, test/git-hooks-pre-commit.test.ts, scripts/pre-commit/filter-staged-files.mjs)
  • vincentkoc: Recent path history shows work on pre-commit dependency hydration and hook/test maintenance, and the current PR head also includes a revert sequencer update from this account. (role: recent area contributor; confidence: high; commits: 9de9562cb78f, c7510e0f1abc, a2174f1ff153; files: scripts/pre-commit/run-node-tool.sh, test/git-hooks-pre-commit.test.ts, git-hooks/pre-commit)
  • Takhoffman: Earlier hook history includes broader pre-commit check-loop changes adjacent to the later formatting-only behavior. (role: adjacent hook contributor; confidence: low; commits: 45535ff433a4, 801e4bede605; files: git-hooks/pre-commit, test/git-hooks-pre-commit.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Jun 23, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 23, 2026
Closed
@clawsweeper clawsweeper Bot added status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels Jun 23, 2026
@vincentkoc
vincentkoc force-pushed the fix/pre-commit-merge-skip branch from 88c0fe8 to b0a71cc Compare June 25, 2026 08:26
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 25, 2026
@vincentkoc
vincentkoc merged commit af90622 into openclaw:main Jun 25, 2026
87 of 94 checks passed
@vincentkoc

vincentkoc commented Jun 25, 2026

Copy link
Copy Markdown
Member

Merged via squash.

@github-actions github-actions Bot mentioned this pull request Jun 25, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 26, 2026
@nxmxbbd @vincentkoc
fix(git-hooks): skip sequencer pre-commit formatting (openclaw#95842)
b10b72b 
* fix(git-hooks): skip sequencer pre-commit formatting

* chore: rerun CI

* fix(git-hooks): skip revert sequencer formatting

---------

Co-authored-by: Vincent Koc <[email protected]>
QiuYuang pushed a commit to QiuYuang/openclaw that referenced this pull request Jul 1, 2026
@nxmxbbd @vincentkoc @QiuYuang
fix(git-hooks): skip sequencer pre-commit formatting (openclaw#95842)
52ba92b 
* fix(git-hooks): skip sequencer pre-commit formatting

* chore: rerun CI

* fix(git-hooks): skip revert sequencer formatting

---------

Co-authored-by: Vincent Koc <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P2 Normal backlog priority with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: pre-commit formats sequencer-staged files during merge/rebase/cherry-pick

2 participants

@nxmxbbd @vincentkoc