fix(signing): assert the commit object, not local verification - #163
Merged
Conversation
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
github-actions
Bot
added
documentation
- replace the `grep -q Good` assertion in the signing preflight with a `gpgsig` commit-header check: under `gpg.format=ssh` with no `gpg.ssh.allowedSignersFile`, `--show-signature` prints `No signature` and `%G?` returns `N` on a correctly signed commit, and setting that config flips the identical commit to `G` - note that exit status is no substitute: `git log --show-signature -1` exits 0 while printing the error - anchor the header check to the region before the first blank line, so a message body starting with `gpgsig` cannot report unsigned as signed - keep the commit chained with `&&`, since an unchained check reads the parent commit, which in signed history carries its own header - record the opposite defect of the replaced pattern: unanchored `Good` matches the `Author:` line, so an unsigned commit by an author named "Goodwin" reports ready - fix the same probe in commit-conventions.md and drop the claim that `%G?` is what to use in scripts - note `git tag -v` needs the same config, exits 1 without it, so a failed match means not verifiable here rather than unsigned - read the commit object in checkpoint GW-21 instead of judging signedness from `--show-signature -5` - add evals for the SSH readiness probe and the `%G?`-in-scripts question Signed-off-by: tripleawwy <43851917+tripleawwy@users.noreply.github.com>
tripleawwy
force-pushed
the
fix/signing-probe-commit-object
branch
from
August 10, 2026 12:29
f85dfaf to
b8a0e8d
Compare
The preflight probe committed with `-m probe`. Under a commit-msg hook
enforcing Conventional Commits — the convention this skill itself
recommends — that commit aborts, and the probe prints `SIGNING NOT READY
— commit failed` for a signing setup that works. Measured in a repo with
such a hook and a loaded SSH signing key: NOT READY with `-m probe`,
READY with `-m "chore: signing probe"`. The sibling snippet in
commit-conventions.md already used a conventional message for exactly
this reason.
The same block carried a comment telling the reader to probe on a
throwaway branch while committing on the current one and undoing it with
`reset --soft HEAD~1`, which additionally fails on a root commit. It now
performs the `switch -c` / `branch -D` dance the comment describes.
Also:
- Separate the two signing failures in the preflight intro. "A signing
failure surfaces only at the merge gate" contradicted the paragraph
below it, which documents `git commit -S` aborting with exit 128. A
local failure surfaces at commit time; only host verification waits
for the gate.
- Rename "Verifying a Signed Commit" to "Detecting a Signed Commit" and
restate GW-21 as signature presence. `grep '^gpgsig'` proves a
signature is embedded, never that it verifies or that the host accepts
the key.
- Reword the `%G?` eval into decision form. "Is G a reliable check"
invited "yes", and G does imply signed — it is the G-or-nothing test
that is unreliable, because not-G does not imply unsigned. The
assertion now also accepts precise answers ("does not prove", "false
negative") that the previous pattern scored as misses.
Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
The signing knowledge in this skill lived entirely in reference prose, one LLM checkpoint and regex evals over model output. Nothing executed it, so none of the measured claims could regress visibly. This adds the mechanical layer and keeps the prose as its explanation. signing-preflight.sh answers the three separable questions with one implementation: - default prove that `git commit -S` signs, here, now - --check-commit does an existing commit carry a signature - --config-only report the config without creating a commit It probes on a throwaway branch through a temporary index, so a staged change is never swept into the probe commit and silently unstaged when the branch is deleted. On a rejected commit it retries once with --no-verify: if that succeeds the first failure was a hook, not signing — a distinction the exit code alone cannot make, and one that previously turned a working setup into "SIGNING NOT READY". Checkpoint GW-17 applies the same header rule mechanically to an assessed repository; GW-21 keeps the judgement part. verify-git-workflow.sh grows a Commit Signing section — it had none at all. tests/test_signing_preflight.sh turns every measured claim into a regression test: %G? = N on a correctly signed SSH commit without gpg.ssh.allowedSignersFile (the premise of the whole design — if git ever changes it, the test says so), the Goodwin false positive, a `gpgsig` line in the message body, gpgsig-sha256 in SHA-256 repositories, hook interference, an unloadable key, and that the probe leaves branch, HEAD, index and worktree untouched. Both defects were re-injected and the suite fails in the right position for each. Two defects found while wiring this up, both in verify-git-workflow.sh and both silent: - `set -e` with `((WARNINGS++))`: post-increment returns the old value, so the first increment of a zero counter exits 1 and aborts the script. Any repository that tripped an early warning got one section out of twelve and an exit code that read like an ordinary failed verification. - `[[ ! -d ".git" ]]` as the repository test: in a worktree `.git` is a file, so the verifier refused to run in the layout this skill's own references recommend. tests/test_verify_git_workflow_sections.sh covers both. The header pattern is now `^gpgsig(-sha256)? ` everywhere — script, checkpoint, verifier and both reference docs — and a test asserts script and checkpoint keep using the same rule. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
Findings from a review pass over the branch, after Copilot answered twice with a quota error and never read the code. The script's own header promised exit 2 for "hooks rejected it even with --no-verify" while that path exits 1 — the same doc-contradicts-code defect this branch set out to remove. Exit 2 now means what the code does: only --config-only returns it. The NOT READY message says hooks were ruled out by the retry, since --no-verify skips pre-commit and commit-msg both. GW-21's prompt still carried the weak `^gpgsig` form while the script, GW-17, the verifier and both reference docs had moved to `^gpgsig(-sha256)? `. All five now agree, and the suite asserts it: each consumer uses the shared rule, and the weak form appears nowhere under skills/ — mutation-checked by reintroducing it in a doc. The "never skip hooks" rule now names its one exception, so the skill does not forbid what its own script does. Also covers a rejecting pre-commit hook, not just commit-msg. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
The checkpoint runner rejects the pattern GW-17 shipped with. Measured by
sourcing is_safe_eval_command out of automated-assessment's
run-checkpoints.sh and calling it:
OLD: REJECTED — pattern contains command-chaining metacharacter (; && || ` $())
NEW: accepted
Two independent reasons, both fatal: the allowlist rejects `$(`, `;`,
`&&`, `||` and backticks outright, and a multi-line YAML scalar reaches
the runner as an EMPTY pattern ("checkpoint likely uses multi-line YAML
scalar; use single-line pattern"). A mechanical checkpoint that cannot
execute is the same defect this branch exists to remove, one layer up.
GW-17 is now single-line, HEAD-only, and free of chaining
metacharacters. `sed -n -e '/^$/q' -e p` is the semicolon-free spelling
of the header cut, so the message-body false positive stays closed —
verified against a commit whose body starts with `gpgsig`. The sweep over
several commits stays where a full shell is available:
verify-git-workflow.sh (last 10) and signing-preflight.sh. Mirroring
rather than reproducing is the arrangement skill-repo already documents
for SR-37.
The suite now asserts both allowlist constraints on the pattern, so the
next edit cannot reintroduce either.
Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
|
CybotTM
added a commit
that referenced
this pull request
Aug 10, 2026
The new validate-checkpoints.sh reports GW-15 and GW-16 as rejected by the assessment runner. Neither has ever executed, and nothing said so — a rejected checkpoint is skipped silently and the report stays quiet. GW-16 is expressible without command substitution: `! git ls-files -- … | grep -q .` reaches the same verdict, and the runner strips a leading `!` before checking the base command. GW-15 is not. Counting commits since the last tag needs `<tag>..HEAD` and `$( )`, and the allowlist rejects `..` as path traversal and `$(` as chaining. The checkpoint is removed and the rule moves to verify-git-workflow.sh, where a full shell is available. GW-21 declares `# mechanical-counterpart: GW-17`; GW-30 declares `none (<reason>)` — its commands fetch a staging lockfile for a judgement and decide nothing on their own. A third abort in verify-git-workflow.sh, same family as the two fixed in #163: `git rev-parse origin/<branch>` echoes the ref NAME on stdout when it does not resolve, so the non-empty guard passed, the rev-list failed on the literal string, and `set -e` killed the script three sections early — on every unpushed branch, which is every branch before its first push. Tests for the three shipped gates that had none: merge-gate.sh (denies BLOCKED, denies UNSTABLE, denies an unresolved thread even when CLEAN, passes unrelated commands), conflict-marker-gate.py (denies staged markers, fails open outside a repo), spec-cleanup-guard.sh (reports artifacts and — its stated invariant — modifies nothing). test_checkpoint_patterns.sh asserts every `type: command` pattern is runnable: single line, no chaining metacharacters, no `..`, base command on the allowlist. The rule is mirrored rather than imported, because automated-assessment is not a dependency of this repo and a test that needs an absent checkout does not run. tests.yml is now a thin caller of the skill-repo-skill reusable. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
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
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.
Problem
The signing-readiness preflight in
references/pull-request-workflow.mdasserted signing worked with:--show-signatureand%G?answer "can this machine verify the signature", not "didgit commit -Ssign". Undergpg.format=sshwith nogpg.ssh.allowedSignersFilethey report failure on a setup that signs correctly, so the preflight reportsSIGNING NOT READYand sends the reader to fix a working configuration.Measured (git 2.54.0)
Same commit throughout,
gpg.format=ssh,user.signingkeyset:Nis indistinguishable from genuinely unsigned, and the command exits 0 while printing the error, so a driver reading$?also sees success. Settinggpg.ssh.allowedSignersFileflips the identical commit:git tag -vneeds the same config, and without it exits 1 with the same error, so a failedGoodmatch there means "not verifiable here", not "unsigned".Two defects in the replaced pattern
grep -q Goodis unanchored and--show-signatureoutput includes the author line, so an unsigned commit matches:And the header check must cut at the first blank line: matched over the whole object,
^gpgsigalso matches a message body line starting withgpgsig.The
&&chaining is kept deliberately.git commit -Saborts rather than producing an unsigned commit, but an unchained check then reads the parent, which in signed history carries its owngpgsigheader and reports ready for a probe that never ran.Change
references/pull-request-workflow.md— the preflight assertion, plus why the commit object is the right thing to assert onreferences/commit-conventions.md— the same probe, the%G?-in-scripts claim, thegit tag -vcaveat, and a "Verifying a Signed Commit" sectioncheckpoints.yaml— GW-21 judged signedness from--show-signature -5, which fails a fully signed repositoryWhether the host accepts the key stays a separate question, answered by the existing
unknown_keycommits-API check.