From bd6c1072bf331f402d192b30824e19e9bf193119 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 11 Aug 2026 23:09:01 -0400 Subject: [PATCH] =?UTF-8?q?fix(docs):=20drop=20the=20signoff=20hook=20?= =?UTF-8?q?=E2=80=94=20it=20cannot=20know=20the=20commit=20author?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 was right that the hook signs with the configured identity while the DCO check requires a trailer matching the commit *author*, and that the two diverge on cherry-pick and `git commit -c`. I tried the suggested author-aware guard and it does not work: inside prepare-commit-msg, `git var GIT_AUTHOR_IDENT` returns the config identity, not the preserved author. Verified end to end — cherry-picking Bob's commit as Alice yields author=Bob, committer=Alice, `Signed-off-by: Alice`. That commit both still fails the gate and certifies work Alice did not write, which is the one thing the last bullet of this section forbids. Three review rounds have now found three holes in this hook, each fix opening the next. That is a wrong mechanism rather than an unfinished one, so the hook is out. A subtly-broken hook in the file that governs how this repo is worked on is worse than no hook. The bullet now recommends `git commit -s` and names BOTH false automations — `format.signOff` and the hook — recording the cherry-pick finding so the next person doesn't rediscover it, and pointing anyone who wants automation at a layer that actually knows the author. Mirror updated to match. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall --- .github/copilot-instructions.md | 2 +- AGENTS.md | 18 ++++-------------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index ffc4c38f..5e7edd68 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -103,7 +103,7 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo - **A test or smoke must not touch real user state.** The web smokes run against a throwaway catalog via the shared `scripts/lib/prod-web-server.mjs` helper, never the developer's `~/.mcp-inspector/mcp.json` (#1977); the cli/tui smokes drive a temp `--catalog`. A new smoke spawning its own server, or teardown that removes a work dir without first awaiting `stopChild` (the #1801 race — `child-cleanup.mjs` exports both halves and both are required), should be flagged. - **Every PR references an issue**, first body line `Closes #`. - **Every PR carries exactly one version label**, `v1` or `v2`, matching its base branch. -- **Every commit carries a `Signed-off-by:` trailer.** The DCO check is a hard merge gate and fails on a single unsigned commit. Use `git commit -s` (note `format.signOff` does *not* sign `git commit` — only `format-patch`; automate with a `prepare-commit-msg` hook instead). Repairing pushed commits means `git rebase HEAD~ --signoff` + `git push --force-with-lease`; remediation commits are not enabled on this repo. +- **Every commit carries a `Signed-off-by:` trailer.** The DCO check is a hard merge gate and fails on a single unsigned commit. Use `git commit -s` (`format.signOff` does *not* sign `git commit` — only `format-patch` — and a `prepare-commit-msg` hook signs with the committer's identity, not the author's, so it breaks on cherry-picks). Repairing pushed commits means `git rebase HEAD~ --signoff` + `git push --force-with-lease`; remediation commits are not enabled on this repo. - Update the relevant `README.md` / `AGENTS.md` when a change adds, removes, renames, or repurposes a file or folder, changes the structure or tech stack, or introduces a command, dependency, or architectural pattern. ## What to prioritize in review diff --git a/AGENTS.md b/AGENTS.md index fc495082..ac9fb499 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -208,22 +208,12 @@ All work should be driven by items on the project board. - When work is complete: - Run `npm run ci` from the root — the mandatory pre-push gate (see [Mandatory pre-push gate](#mandatory-pre-push-gate)). `npm run validate` is the fast inner-loop check and is **not** a substitute: it runs no coverage gate, no smokes, and no Storybook tests. - **Sign off every commit — the DCO check is a hard merge gate.** The repo runs the [probot DCO app](https://probot.github.io/apps/dco/), which fails the PR unless **every** commit in it carries a `Signed-off-by: Author Name ` trailer matching its author. There is no partial credit: one unsigned commit out of six fails the whole check. The app *does* expose an override button to anyone with write access, but treat it as unavailable — see the repair bullet below for why. - - **Prevent it, don't remember it** — the failure is invisible until after you have pushed. Commit with **`git commit -s`**. To stop relying on memory, add a `prepare-commit-msg` hook, which is the only mechanism that actually signs an ordinary `git commit`: + - **Prevent it with `git commit -s`** — the failure is invisible until after you have pushed, so make the flag habitual. Two things that look like automation and are not: - ```sh - cat > .git/hooks/prepare-commit-msg <<'HOOK' - #!/bin/sh - # Match THIS identity's exact trailer, not any Signed-off-by: a message - # reused from another commit (cherry-pick, `git commit -c`) already carries - # someone else's, and the DCO check requires one matching the author — so a - # bare `grep '^Signed-off-by:'` would skip and leave the commit failing. - SOB="Signed-off-by: $(git config user.name) <$(git config user.email)>" - grep -qF "$SOB" "$1" || git interpret-trailers --in-place --trailer "$SOB" "$1" - HOOK - chmod +x .git/hooks/prepare-commit-msg - ``` + - ⚠️ **`git config format.signOff true` does nothing here.** Despite the name it only defaults the `-s` flag for `git format-patch`; `git commit` never reads it, and there is no `commit.signoff` equivalent. Setting it looks like a fix and silently changes nothing. + - ⚠️ **Don't reach for a `prepare-commit-msg` hook either**, however tempting. A hook can only sign with your *configured* identity, while the DCO check requires a trailer matching the commit **author** — and the two diverge exactly when you least want a wrong answer. Verified: on `git cherry-pick`, the resulting commit keeps the original author but the hook signs as you, producing a commit that both still fails the gate and certifies someone else's work. `git var GIT_AUTHOR_IDENT` does **not** resolve the true author inside that hook, so an author-aware guard doesn't rescue it. - ⚠️ **`git config format.signOff true` does NOT do this** — despite the name, it only defaults the `-s` flag for `git format-patch`; `git commit` never reads it, and there is no `commit.signoff` equivalent. Setting it looks like a fix and silently changes nothing, so it is a trap worth naming rather than omitting. + If you want automation anyway, put it somewhere that knows the author — a CI pre-merge check, or a shell alias you invoke deliberately — not a blanket commit hook. - **Repairing already-pushed commits** means rewriting them: `git rebase HEAD~ --signoff`, then `git push --force-with-lease`. Use `--force-with-lease` rather than `--force` so a concurrent push can't be silently clobbered, and only rewrite when you are the sole author and nobody else has based work on the branch — the usual [perils of rebasing](https://git-scm.com/book/en/v2/Git-Branching-Rebasing). **Rewriting is the only legitimate repair, and the two apparent alternatives are not.** The app's empty "remediation commit" flow requires `allowRemediationCommits.individual`; this repo ships no `.github/dco.yml`, so it runs with that disabled and the original unsigned commits keep failing. The app *does* also give anyone with write access an override button that forces the check green — but it only silences the check, it does not make the author certify anything, and the signoff is the certification (see below). Using it on your own unsigned commits asserts nothing while looking like compliance, so don't: sign the commits instead. If others are working on the branch, coordinate with them before rewriting. - The signoff is a **Developer Certificate of Origin** assertion in the author's own name, so it must be that author's identity — never sign off someone else's commit.