Make the Signed-Commit Verification Tech-Agnostic - #708
Conversation
Replace the ssh-add -L / gpg-agent liveness probe in git-commit-conventions with a real scratch commit read back via git's own %G? verdict char. That probe drove a Windows agent (#706) to wrongly conclude signing was broken: gpg.format=ssh can sign straight from a key file with no ssh-agent running at all, which is this host's actual setup, and ssh-add -L says nothing about that path. The same scratch commit's %ae output now backs the identity check too, so one probe verifies both instead of trusting git config values that don't prove what lands on the commit object. Also documents why develop/main always show GitHub-committer, GPG-signed commits at every squash-merge point regardless of contributor host config (GitHub signs the merge commit itself, server-side) -- a second red herring from the same issue that had nothing to do with any host being misconfigured.
There was a problem hiding this comment.
Pull request overview
This PR updates the git-commit-conventions skill documentation to make commit-signing verification tech-agnostic by replacing agent-liveness probes with a scratch-commit check that reads Git’s %G? signature verdict and %ae identity output end-to-end.
Changes:
- Replace
ssh-add -L/ agent-liveness probing guidance with a scratch-commit verification flow usinggit log --format='sig=%G? email=%ae'. - Document the expected GitHub squash-merge committer/signature pattern to avoid misdiagnosing mixed signature history.
- Regenerate the distributed
.claude-pluginskill output and update the fleet-skills source digest.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| .agents/skills/git-commit-conventions/SKILL.md | Updates the canonical skill text to use a scratch-commit signing + identity verification and clarifies mixed-signature history expectations. |
| .claude-plugin/fleet-skills/skills/git-commit-conventions/SKILL.md | Updates the generated/distributed copy of the skill. |
| .claude-plugin/fleet-skills/.source-digest | Updates the digest for the regenerated fleet-skills output. |
Suppressed comments (2)
.agents/skills/git-commit-conventions/SKILL.md:79
- This paragraph uses an em dash ("squash-merge — GitHub"), which violates the repo's ASCII-only rule for agent-authored text (spec/project-types.json:178). Please replace it with ASCII punctuation (for example, a spaced hyphen).
`git log --pretty='%G? %GK'` shows two distinct shapes, not two health states: a commit committed
by the PR's own author carries that host's own signature type, while a commit committed by
`GitHub <noreply@github.com>` is a squash-merge — GitHub creates and signs that commit itself,
server-side, with GitHub's own GPG key, regardless of what the PR author signed with locally.
.agents/skills/git-commit-conventions/SKILL.md:99
- This sentence uses an em dash ("end-to-end — read"), which violates the repo's ASCII-only rule for agent-authored text (spec/project-types.json:178). Replace it with ASCII punctuation (for example, a spaced hyphen).
or `ID+username@users.noreply.github.com` form. **Verify it, do not set it**: the scratch commit
from the signing check above already proves this end-to-end — read its `email=` output rather
than trusting `git config --get user.email` alone, since a global config value doesn't prove what
actually lands on a commit object. Match it against that address before committing, rather than
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Restructure six em-dash sentences into ASCII punctuation (period, colon, parens) per the charset rule, never a spaced hyphen. - Fix two prose semicolons the same way, and move the PowerShell probe out of an inline backtick span into its own fenced block so it reads as code, not prose (prose_lint only exempts fenced blocks). - STANDUP.md carried the identical agent-liveness signing check (`ssh-add -L` / `gpg --list-secret-keys`) this PR just retired from git-commit-conventions, plus a paragraph defending it as what GOVERNANCE.md prescribes. Both now match: the same scratch-commit probe, and prose explaining why, not the old agent-liveness framing.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (6)
STANDUP.md:61
- This paragraph also references
git-commit-conventions.md, which is not a file in this repository. Update it to the actual path so the pointer is actionable.
The scratch commit exercises the whole signing pipeline rather than one delivery path, since `ssh-add -L` or `gpg --list-secret-keys` only prove an agent holds a key and say nothing about a host that signs straight from a key file with no agent running at all, a live and correctly configured case documented in `git-commit-conventions.md` "Signing, verified not configured", which [GOVERNANCE.md "Git and Commit Rules"][governance-git-and-commit-rules] points to. Signing is **SSH or GPG**, so this judges the configured format by its actual result (`sig=G`), never by which delivery path produced it. A missing `--global` value, `sig` not reading `G`, or `email` not matching the noreply address is a **host** misconfiguration to surface to the maintainer ([`docs/host-setup.md`][host-setup] is the setup procedure), not something to patch per repo. Patching it locally hides a broken host that then produces wrong identities in every other repo on that machine.
.agents/skills/git-commit-conventions/SKILL.md:57
- This section correctly states that
ssh-add -Lis not a reliable signing verification probe, butdocs/host-setup.mdstill presentsssh-add -Las part of the host verification checklist. That can reintroduce the same false-negative behavior described in #706 unless the host-setup doc is updated to match this guidance.
agent-backed or straight from a keyring. **Probing agent liveness (`ssh-add -L`, a `gpg-agent`
check) is not a valid test and must not be used.** It tests one specific delivery path, not
whether `git commit -S` succeeds, and a host that signs straight from a key file fails that
probe while signing correctly.
STANDUP.md:37
git-commit-conventions.mdis referenced here, but that file does not exist in this repo. Point this at the actual skill file so readers can find the rationale being referenced.
This issue also appears on line 61 of the same file.
# prove signing works with a live scratch commit, not an agent-liveness probe (ssh-add -L,
# gpg --list-secret-keys): a host that signs straight from a key file with no agent running
# passes cleanly and fails that probe. See git-commit-conventions.md "Signing, verified not
# configured" for why.
STANDUP.md:40
- The scratch-commit probe ends with
rm -rf "$d"on its own line, which makes the snippet's exit status always reflect the cleanup, not whethergit init/git commit -S/git logsucceeded. Wrapping cleanup in a subshelltrapkeeps the temp dir cleanup while preserving the failure status for scripted/agent execution.
d=$(mktemp -d) && git init -q "$d" && git -C "$d" commit -S --allow-empty -q -m check \
&& git -C "$d" log -1 --format='sig=%G? email=%ae'
rm -rf "$d"
.agents/skills/git-commit-conventions/SKILL.md:66
- The scratch-commit snippet does cleanup as its final command (
rm -rf "$d"), which means the overall exit status reflects the cleanup rather than whether the signing/git logprobe succeeded. If an agent runs this in a script, failures can be masked. Wrap the body in a subshell with a cleanuptrapso the probe's exit status is preserved while still cleaning up the temp directory.
d=$(mktemp -d) && git init -q "$d" \
&& git -C "$d" commit -S --allow-empty -q -m check \
&& git -C "$d" log -1 --format='sig=%G? email=%ae'
rm -rf "$d"
.claude-plugin/fleet-skills/skills/git-commit-conventions/SKILL.md:66
- Same as the source skill: the scratch-commit snippet ends with cleanup as the last command, which can hide failures if the snippet is run in a script. Use a subshell
trapso the probe's exit status is preserved while still cleaning up the temp directory.
d=$(mktemp -d) && git init -q "$d" \
&& git -C "$d" commit -S --allow-empty -q -m check \
&& git -C "$d" log -1 --format='sig=%G? email=%ae'
rm -rf "$d"
Copilot's round-2 review on 8eb9050 found: - STANDUP.md pointed at 'git-commit-conventions.md', a filename that doesn't exist in this repo (twice). Fixed to the real path, .agents/skills/git-commit-conventions/SKILL.md, added as a reference link where used in prose. - The scratch-commit probe's final 'rm -rf "$d"' ran after the && chain, so the snippet's exit status always reflected cleanup, masking a real signing failure if an agent scripts around it. Wrapped the probe in a subshell with an EXIT trap in all three copies (git-commit-conventions.md, STANDUP.md, host-setup.md) so the probe's own exit status survives; verified locally (sig=G, exit=0, temp dir removed). - docs/host-setup.md's own 'Verify Host Setup' section still ran ssh-add -L ahead of a hardcoded 'git -c gpg.format=ssh commit -S', the same agent-liveness probe this PR retired elsewhere, and the hardcoded format meant it silently tested the wrong format on a GPG-configured host. Replaced both the POSIX and PowerShell blocks with the same tech-agnostic probe used everywhere else.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (11)
docs/host-setup.md:269
- The PowerShell probe output only prints the author email (%ae). Include the committer email (%ce) as well so the identity verification is end-to-end and consistent with GitHub's committer-based verification.
$d = Join-Path $env:TEMP ([guid]::NewGuid())
git init -q $d
git -C $d commit -S --allow-empty -q -m check
git -C $d log -1 --format='sig=%G? email=%ae'
Remove-Item -Recurse -Force $d
.agents/skills/git-commit-conventions/SKILL.md:107
- If the probe output includes both author and committer emails, the surrounding text should tell readers to verify both values (not just
email=) when checking the noreply identity.
or `ID+username@users.noreply.github.com` form. **Verify it, do not set it**: the scratch commit
from the signing check above already proves this end-to-end. Read its `email=` output rather
than trusting `git config --get user.email` alone, since a global config value doesn't prove what
actually lands on a commit object. Match it against that address before committing, rather than
.claude-plugin/fleet-skills/skills/git-commit-conventions/SKILL.md:77
- The PowerShell probe prints only the author email (%ae). Include the committer email (%ce) too so the identity check cannot miss author/committer divergence.
$d = Join-Path $env:TEMP ([guid]::NewGuid())
git init -q $d
git -C $d commit -S --allow-empty -q -m check
git -C $d log -1 --format='sig=%G? email=%ae'
Remove-Item -Recurse -Force $d
**.claude-plugin/fleet-skills/skills/git-commit-conventions/SKILL.md:107**
* If the probe output includes both author and committer emails, the identity section should instruct readers to verify both values (not just `email=`).
or ID+username@users.noreply.github.com form. Verify it, do not set it: the scratch commit
from the signing check above already proves this end-to-end. Read its email= output rather
than trusting git config --get user.email alone, since a global config value doesn't prove what
actually lands on a commit object. Match it against that address before committing, rather than
**docs/host-setup.md:250**
* After adding committer_email to the probe output, this requirement should check both the author and committer emails, not just %ae.
sig must read G and email must match the noreply address from the config line above.
**.agents/skills/git-commit-conventions/SKILL.md:77**
* The PowerShell probe prints only the author email (%ae). Include the committer email (%ce) too so the identity verification cannot miss author/committer divergence.
$d = Join-Path $env:TEMP ([guid]::NewGuid())
git init -q $d
git -C $d commit -S --allow-empty -q -m check
git -C $d log -1 --format='sig=%G? email=%ae'
Remove-Item -Recurse -Force $d
docs/host-setup.md:245
- The scratch commit output only prints the author email (%ae). Since the guidance asks readers to verify the noreply identity, also print the committer email (%ce) so the check can catch cases where author/committer diverge.
This issue also appears in the following locations of the same file:
- line 250
- line 265
git init -q "$d" \
&& git -C "$d" commit -S --allow-empty -q -m check \
&& git -C "$d" log -1 --format='sig=%G? email=%ae'
)
STANDUP.md:42
- The scratch commit output only prints the author email (%ae), but the surrounding guidance emphasizes that committer identity is what GitHub verifies and can diverge from the author (e.g., rebase/amend). Include %ce in the probe output so the pre-init check can validate both identities end-to-end.
&& git -C "$d" commit -S --allow-empty -q -m check \
&& git -C "$d" log -1 --format='sig=%G? email=%ae'
docs/host-setup.md:255
- This paragraph is hard-wrapped and splits the link text across a newline, which is inconsistent with the surrounding one-paragraph-per-line style and makes the link harder to maintain. Prefer a single-line paragraph and keep the link text unbroken.
`ssh-add -L` (or a `gpg --list-secret-keys` equivalent) is not a substitute: it only proves an
agent holds a key, and a host that signs straight from a key file with no agent running passes
this scratch commit while failing that probe, per [GOVERNANCE.md "Git and Commit
Rules"][governance-git-and-commit-rules]. If signing fails locally, the devcontainer will fail
too, so fix here first.
.agents/skills/git-commit-conventions/SKILL.md:67
- The scratch commit probe prints only the author email (%ae). To make this a true end-to-end identity check (and to match the later guidance about committer importance), include %ce in the output as well.
This issue also appears in the following locations of the same file:
- line 72
- line 104
d=$(mktemp -d) && (
trap 'rm -rf "$d"' EXIT
git init -q "$d" \
&& git -C "$d" commit -S --allow-empty -q -m check \
&& git -C "$d" log -1 --format='sig=%G? email=%ae'
)
.claude-plugin/fleet-skills/skills/git-commit-conventions/SKILL.md:67
- The scratch commit probe prints only the author email (%ae). Include %ce so this can actually validate both identities end-to-end, consistent with the later committer-focused guidance.
This issue also appears in the following locations of the same file:
- line 72
- line 104
d=$(mktemp -d) && (
trap 'rm -rf "$d"' EXIT
git init -q "$d" \
&& git -C "$d" commit -S --allow-empty -q -m check \
&& git -C "$d" log -1 --format='sig=%G? email=%ae'
)
Copilot's round-3 review on 1d705d4 found the same class of gap recurring 11 times across all four probe copies: the format string only printed %ae (author email), while both this skill's own Identity section and STANDUP.md's existing verification (which the probe was modeled after) require checking author and committer both, since a rebase, amend, or cherry-pick rewrites the committer while leaving the author untouched and GitHub verifies against the committer. Switched every copy (git-commit-conventions.md, STANDUP.md, host-setup.md POSIX and PowerShell) to STANDUP.md's own established format, 'sig=%G? author=%an <%ae> committer=%cn <%ce>', and updated the surrounding prose to say so. Also folded a hard-wrapped paragraph in host-setup.md back to the file's one-paragraph-per-line style.
|
Answering the suppressed (low-confidence) findings across all three rounds so far, quoting each and giving a verdict: Round on 633bf1a (2 findings)
Fixed in 8eb9050: both rewritten to ASCII punctuation, along with four other em dashes the visible review round already caught. Round on 8eb9050 (6 findings)
Fixed in 1d705d4: the two broken references now point at the real path ( Round on 1d705d4 (11 findings)
Fixed in d3fbb1c: every probe copy ( |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
docs/host-setup.md:264
- The PowerShell scratch-commit snippet does not quote $d. On many Windows hosts $env:TEMP (or the username path segment) can contain spaces, which will break
git init -q $d/git -C $d ...by splitting the path into multiple arguments. Also, cleanup only runs if all commands succeed, leaving temp dirs behind on failure. Quote the path and wrap cleanup in a finally block.
$d = Join-Path $env:TEMP ([guid]::NewGuid())
git init -q $d
git -C $d commit -S --allow-empty -q -m check
git -C $d log -1 --format='sig=%G? author=%an <%ae> committer=%cn <%ce>'
Remove-Item -Recurse -Force $d
.claude-plugin/fleet-skills/skills/git-commit-conventions/SKILL.md:76
- Same issue as the source skill: the PowerShell snippet does not quote $d (breaks when $env:TEMP contains spaces) and cleanup only runs on success. Since this file is the distributed copy, it should reflect the corrected try/finally + quoted-path version after regeneration.
$d = Join-Path $env:TEMP ([guid]::NewGuid())
git init -q $d
git -C $d commit -S --allow-empty -q -m check
git -C $d log -1 --format='sig=%G? author=%an <%ae> committer=%cn <%ce>'
Remove-Item -Recurse -Force $d
.agents/skills/git-commit-conventions/SKILL.md:76
- The PowerShell equivalent does not quote $d, so it will fail on Windows hosts where the temp path contains spaces. It also only removes the temp dir if every prior command succeeds. Quote the path and use try/finally so cleanup runs even when signing fails (which is exactly what this probe is meant to detect).
$d = Join-Path $env:TEMP ([guid]::NewGuid())
git init -q $d
git -C $d commit -S --allow-empty -q -m check
git -C $d log -1 --format='sig=%G? author=%an <%ae> committer=%cn <%ce>'
Remove-Item -Recurse -Force $d
Copilot's round-4 review on d3fbb1c found the PowerShell scratch-commit snippet (all three copies) left $d unquoted, which breaks git init/-C on a Windows profile path containing a space, and ran cleanup as a plain trailing statement, so a failed git command skipped Remove-Item and left the temp dir behind. Quoted every $d use and wrapped the probe in try/finally so cleanup always runs, success or failure.
|
Answering round 4's suppressed findings (all on
Fixed in 9b2e941: every |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
STANDUP.md:38
mktemp -dwithout a template is not portable: the default macOS/BSDmktemprequires a template (or-t), so this signing probe will fail on macOS hosts even when signing is correctly configured.
d=$(mktemp -d) && (
docs/host-setup.md:241
- The POSIX host-setup probe uses
mktemp -dwithout a template. On macOS (supported by this doc), BSDmktemptypically requires a template (or-t), so this step can fail before running the signing check.
d=$(mktemp -d) && (
.agents/skills/git-commit-conventions/SKILL.md:63
- This scratch-commit probe uses
mktemp -dwithout a template. That works on GNU coreutils but is not portable to macOS/BSDmktemp(which expects a template or-t), so the verification step can fail on macOS even when signing works.
d=$(mktemp -d) && (
.claude-plugin/fleet-skills/skills/git-commit-conventions/SKILL.md:63
- This distributed copy also uses
mktemp -dwithout a template, which is not portable to macOS/BSDmktemp(template or-trequired). Update the source skill and regenerate so both copies stay consistent.
d=$(mktemp -d) && (
Copilot's round-5 review on 9b2e941 found 'mktemp -d' with no template in all three POSIX probe copies (git-commit-conventions.md, STANDUP.md, host-setup.md): GNU coreutils accepts it bare, but BSD/macOS mktemp requires a template argument and errors without one, failing the signing check on a macOS host even when signing itself is fine. Switched to 'mktemp -d "${TMPDIR:-/tmp}/sign-check.XXXXXX"' in all three, the portable form both implementations accept; verified locally (sig=G, exit=0).
|
Answering round 5's suppressed findings (all on
Fixed in 0553d34: switched to |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (7)
docs/host-setup.md:263
- Same as the POSIX snippet above:
git commit -Sforces signing and can hide a missing/falsecommit.gpgsign. Using a plaingit commitmakes this an end-to-end verification that signing is enabled by configuration.
git -C "$d" commit -S --allow-empty -q -m check
docs/host-setup.md:267
- In the PowerShell snippet,
Remove-Iteminfinallycan throw ifgit initfails before creating the directory, which can mask the real error. Guard the cleanup withTest-Pathso failures report the underlying signing/identity problem.
} finally {
Remove-Item -Recurse -Force "$d"
}
.agents/skills/git-commit-conventions/SKILL.md:79
- The
finallycleanup can throw ifgit initfails before creating$d, potentially masking the real failure you want the user to see. Guard the cleanup withTest-Pathso errors from signing/identity remain visible.
} finally {
Remove-Item -Recurse -Force "$d"
}
.agents/skills/git-commit-conventions/SKILL.md:75
- In the PowerShell equivalent,
git commit -Sforces signing and can mask a missing/falsecommit.gpgsign. Using a plaingit commitmakes the probe validate the default signing configuration end-to-end.
git -C "$d" commit -S --allow-empty -q -m check
STANDUP.md:41
- The scratch commit uses
git commit -S, which bypasses thecommit.gpgsignconfiguration you are also asking the user to verify. Using a plaingit commithere better validates that the host is configured to sign commits by default (and avoids a false pass whencommit.gpgsignis unset/false).
&& git -C "$d" commit -S --allow-empty -q -m check \
docs/host-setup.md:244
- This verification snippet uses
git commit -S, which can pass even ifcommit.gpgsignis unset/false (because-Sforces signing). If the intent is to prove the host is configured to sign commits by default, use a plaingit commitso the check fails when signing is not enabled by configuration.
This issue also appears in the following locations of the same file:
- line 263
- line 265
&& git -C "$d" commit -S --allow-empty -q -m check \
.agents/skills/git-commit-conventions/SKILL.md:66
- The scratch-commit probe uses
git commit -S, which forces signing and can still succeed even ifcommit.gpgsignis unset/false. Using a plaingit commitbetter validates the default signing configuration the rest of the guidance expects.
This issue also appears in the following locations of the same file:
- line 75
- line 77
&& git -C "$d" commit -S --allow-empty -q -m check \
Copilot's round-6 review on 0553d34 found two real gaps: - The probe used 'git commit -S', which forces signing regardless of commit.gpgsign. A host with signing keys configured but gpgsign unset/false would still pass, which is exactly the default-config gap the probe exists to catch, since every real agent commit is plain (no -S). Dropped -S in all POSIX and PowerShell copies; verified locally that a plain commit still signs correctly here (sig=G), and added a sentence explaining why -S is deliberately absent. - The PowerShell 'finally' block's Remove-Item can throw if git init never created $d, masking the real signing/identity error. Guarded it with Test-Path in both PowerShell copies.
|
Answering round 6's suppressed findings (all on
Fixed in 21aec10: dropped |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
STANDUP.md:62
- The explanation of
sig=Uis too narrow. In Git,%G?can also returnUfor SSH-signed commits when the signature is cryptographically valid but the signer is not trusted/recognized (for example, missing fromgpg.ssh.allowedSignersFile/allowed_signers). The current text saysUis GPG-only, which conflicts with the rest of the guidance (and with how SSH verification works).
The scratch commit exercises the whole signing pipeline rather than one delivery path, since `ssh-add -L` or `gpg --list-secret-keys` only prove an agent holds a key and say nothing about a host that signs straight from a key file with no agent running at all, a live and correctly configured case [git-commit-conventions][git-commit-conventions] documents in "Signing, verified not configured", the same rules [GOVERNANCE.md "Git and Commit Rules"][governance-git-and-commit-rules] points to. Signing is **SSH or GPG**, so this judges the configured format by its actual result (`sig=G`, or `sig=U` for a cryptographically good GPG signature whose key trust is merely undefined), never by which delivery path produced it. A missing `--global` value, `sig` not reading `G` or `U`, or either printed email not matching the noreply address is a **host** misconfiguration to surface to the maintainer ([`docs/host-setup.md`][host-setup] is the setup procedure), not something to patch per repo. Patching it locally hides a broken host that then produces wrong identities in every other repo on that machine.
docs/host-setup.md:247
- This paragraph states that
sig=Uis GPG-only and that SSH has no trust concept. For SSH signing, Git can still report%G? = Uwhen the signature is cryptographically good but the signer is not trusted/recognized (for example, missing fromgpg.ssh.allowedSignersFile). The current wording may mislead readers into treatingUas impossible under SSH.
`sig` must read `G` (good signature) or `U` (good signature, undefined trust: GPG-only, common on a freshly generated key before its trust is set to ultimate; SSH's `allowed_signers` carries no trust concept, so this never applies there), both the `author` and `committer` email must be an actual noreply address, and both must match `user.email` from the config line above, all enforced by the snippet itself. `ssh-add -L` (or a `gpg --list-secret-keys` equivalent) is not a substitute: it only proves an agent holds a key, and a host that signs straight from a key file with no agent running passes this scratch commit while failing that probe, per [GOVERNANCE.md "Git and Commit Rules"][governance-git-and-commit-rules]. If signing fails locally, the devcontainer will fail too, so fix here first.
.agents/skills/git-commit-conventions/SKILL.md:97
- The
sig=Uexplanation is incorrect for SSH signing. Git's%G?can returnUfor SSH-signed commits when the signature is valid but the signer is not trusted/recognized (for example, not listed ingpg.ssh.allowedSignersFile). The current text says SSH has no trust concept and thatUnever applies, which can cause agents to mis-diagnose a working signing setup as broken (or vice versa).
`sig` must read `G` (good signature) or `U` (good signature, undefined trust: GPG-only, common
on a freshly generated key before its trust is set to ultimate). SSH's `allowed_signers` carries
no trust concept, so `U` never applies there. `sig` is git's own verdict char. Don't grep localized
.claude-plugin/fleet-skills/skills/git-commit-conventions/SKILL.md:97
- Same issue as the source skill: the
sig=Uexplanation claims SSH never yieldsU, but Git can return%G? = Ufor SSH signatures when the signer is not trusted/recognized (for example, missing fromgpg.ssh.allowedSignersFile). This should match the corrected wording in the source so the distributed copy is accurate.
`sig` must read `G` (good signature) or `U` (good signature, undefined trust: GPG-only, common
on a freshly generated key before its trust is set to ultimate). SSH's `allowed_signers` carries
no trust concept, so `U` never applies there. `sig` is git's own verdict char. Don't grep localized
Copilot's round-16 review on 4f03107 disputed a claim I wrote in round 12: that SSH signing never produces sig=U since allowed_signers carries no trust concept. That was wrong, and I verified it directly rather than take either side's word: signed a scratch commit with gpg.ssh.allowedSignersFile pointed at an empty file, and got exactly sig=U, 'Good "git" signature ... No principal matched.' The shell logic (accept G or U) was already correct for what this probe actually needs to prove, since a missing local allowed_signers entry doesn't affect whether GitHub itself verifies the commit, only local git verify-commit output. Only the prose explanation was wrong. Corrected it in all three files: U covers both a GPG key with merely undefined trust and an SSH key missing from allowed_signers, neither of which means the commit won't verify on GitHub. Also caught and fixed two more prose semicolons of my own while rewriting these paragraphs. Full local gate set clean, including both docker linters.
|
Answering round 16's suppressed findings (all on
Verified this directly rather than take either side's word: signed a scratch commit with Fixed in 63336a3: the shell logic (accept |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/host-setup.md:243
- The note about CRLF breaking
\line continuations is helpful, but this doc still contains at least one\-continued shell snippet (the allowed_signers creation earlier). As written, it is easy to miss that those other snippets are subject to the same copy/paste failure mode. Consider broadening this note to apply to the whole document (or rewrite the remaining\-continued snippets) so the guidance is internally consistent.
# One physical line, not backslash-joined: this file is CRLF (the repo's Markdown default),
# and a `\` continuation stops working the moment a stray `\r` lands after it.
d=$(mktemp -d "${TMPDIR:-/tmp}/sign-check.XXXXXX") && ( trap 'rm -rf "$d"' 0; email=$(git config --global --get user.email) && git init -q "$d" && git -C "$d" commit --allow-empty -q -m check && out=$(git -C "$d" log -1 --format='sig=%G? author=%an <%ae> committer=%cn <%ce>') && echo "$out" && ae=$(git -C "$d" log -1 --format='%ae') && ce=$(git -C "$d" log -1 --format='%ce') && case "$out" in sig=G\ *|sig=U\ *) true ;; *) false ;; esac && case "$email" in *@users.noreply.github.com) true ;; *) false ;; esac && [ "$ae" = "$email" ] && [ "$ce" = "$email" ] )
git init -q "$d" `&& git commit ... `&& $out = git log ...` fails to
parse in PowerShell 7: `&&` is the pipeline-chain operator and cannot
chain into an assignment statement. This broke on every Windows host,
correctly configured or not, in both docs/host-setup.md's "Verify Host
Setup" section and the PowerShell equivalent in
.agents/skills/git-commit-conventions/SKILL.md's "Signing, verified
not configured" section (and its .claude-plugin/fleet-skills mirror).
Fix: pull the $out assignment out of the && chain onto its own
statement.
Validated on a real Windows 11 Pro 10.0.26200 host with PowerShell
7.6.4: the unpatched block threw a ParserError
("Unexpected token '=' in expression or statement"); the patched
block prints "sig=G author=... committer=..." and exits 0 against a
correctly configured host (SSH signing, users.noreply.github.com
identity), and correctly throws on a deliberately wrong identity in a
negative-case test.
Copilot's round-17 review on 63336a3 found docs/host-setup.md still carried one pre-existing '\'-continued snippet (the allowed_signers file creation, predating this PR) right next to the new CRLF warning comment explaining why the scratch probe avoids exactly that pattern, making the guidance internally inconsistent. Collapsed it to one line, same fix as the probe itself. Rebased onto 840388d first (the Windows-side agent's real fix for the PowerShell '&&'-into-assignment parse error my round-15 change introduced, validated on actual Windows 11 / PowerShell 7.6.4). That's the gap issue #712 exists to name: I could reason the structure matched the tested POSIX version, but I could not execute it, and the Windows-side validation is what actually confirmed or refuted it. Full local gate set clean, including both docker linters.
|
Answering round 17's suppressed finding (on
Fixed in c7fb717: collapsed that snippet to one line, same fix as the scratch probe itself. That same push also carries a real fix from a Windows-side agent ( |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/host-setup.md:140
- The
allowed_signersentry is built fromgit config user.email, which can pick up a repo-local override if this snippet is run from inside an existing repository. Since this section is configuring host-level signing and the rest of the docs now emphasize--globalfor identity checks, read the global value explicitly here to avoid writing a mismatched principal into~/.config/git/allowed_signers.
echo "$(git config user.email) namespaces=\"git\" $(cat ~/.ssh/id_ed25519.pub)" >> ~/.config/git/allowed_signers
Copilot's round-19 review on c7fb717 found the allowed_signers entry was built from a bare 'git config user.email', which picks up a repo-local override if this snippet is run from inside an existing repo checkout, silently writing the wrong principal into ~/.config/git/allowed_signers. Every other identity read this PR touches already reads --global explicitly for exactly this reason (STANDUP.md's own section 0 spells out why). Added it here too. docs/ssh-signing.md carries the identical line (missing --global, same backslash continuation) but is untouched by this PR and outside what the review looked at; flagging it to the maintainer separately rather than expanding scope here. Full local gate set clean, including both docker linters.
|
Answering round 19's suppressed finding (on
Fixed in 1d7cc89: added
|
Adds the rule from #712: an agent must not claim platform-specific code (PowerShell on Windows, a macOS-only `mktemp`/`ssh-agent` behavior, a WSL-specific path quirk) works, is verified, or is fixed unless it actually executed that code on that platform. Reasoning by structural analogy to an already-tested equivalent on a different platform is a plausible first pass, not verification, and has to be reported as exactly that. Three spots, matching the existing pattern where GOVERNANCE.md keeps the canonical rule text and the skills excerpt or reference it at their decision moment: - `GOVERNANCE.md` "Verification Discipline": the full rule, alongside the other green-but-wrong checks in that section. - `.agents/skills/agent-conduct/SKILL.md` "Before Claiming Done": the same rule, abbreviated to match its sibling bullets there. - `.agents/skills/pr-review-conduct/SKILL.md` outcome 1 ("Real, so fix it"): a caveat that a platform-specific review finding is not closed by a SHA alone, since that is the exact moment the incident behind #712 happened (round 4 of PR #708's review loop). Per "Issue-closing keywords" in "Branching Model," the closing keyword belongs on the `develop -> main` promotion PR, not here. This references #712 rather than closing it. Local gates run: `prose_lint.py` (full check set, diff-scoped), `repo_gate.py`, `unittest discover -s scripts/tests` (665 tests), `spec/validate.py`. All green.
…Python CI Gates (#718) Thirty-one squashes, `56f4d7d..d54862a`. 115 files, +20436/-5298. **Merge with a merge commit, never a squash, and never with `--delete-branch`.** This pull request's head is `develop` itself. ## What lands **Fleet Skills.** The `.agents/skills/` source tree, the generated `.claude-plugin/` distribution, `scripts/build_dist.py` with its `--check` gate, and `scripts/skills_install.py` with its host stamp (#676). Packaged as skills on top of the scaffold: PR review conduct and Copilot instructions upkeep (#677), comment and doc style (#678), resync-a-repo and fleet-conformance-check (#679), the per-language codestyles (#680), git commit conventions and operational vs release workflow (#681), stand up a repo (#683), and repo-worktree (#717). Coverage gaps closed in three passes (#690, #691, #692) plus the P4 sentence-length opt-in (#697). **Host setup.** The Windows host-setup tooling and its PowerShell gate (#674), the Windows bootstrap loader (#682), Docker install and upgrade on Linux and Windows with a version floor (#701, #705), a `uv` floor in `spec/host-tools.json` (#698), self-healing of a shadowing `uv`, `jq`, or `git-restore-mtime` copy (#689), node's real winget package id (#696), and a README for the Linux host-setup nuances (#710). **Python and CI.** Python tooling in CI with the script tests moved to `scripts/tests` (#704), `ruff format` adopted and gated (#709), and the PSScriptAnalyzer claim conditioned on repos that carry `.ps1` files (#686). **Conduct rules.** Triage-order and scope guardrails in pr-review-conduct (#684), `pr_review.py wait` requesting a review rather than only polling for one (#685), a tech-agnostic signed-commit verification (#708), execution rather than analogy to verify platform-specific code (#715), and a unique worktree for every task (#717). **Docs.** The fleet map and gap register with peer messaging declared (#687), mermaid flow diagrams in the kept-authority docs (#702), and the map pointed at the shipped diagrams and current tooling (#703). ## Issues this promotion closes Each landed on `develop` on its own pull request. The keyword fires only on a merge into `main`, so it sits here rather than on the feature pull requests. Closes #700 Closes #707 Closes #711 Closes #712 Closes #714 Closes #688 #699 stays open on purpose: #717 shipped the layout convention and the skill, and the physical migration of existing checkouts is still tracked there. ## Review record Every squash closed its own Copilot loop on its own pull request before merging to `develop`. This promotion carries no new content of its own, so its review is the merged tree as a whole. ## Consequence worth stating The `GOVERNANCE.md` and `AGENTS.md` sections these squashes changed become the canonical the moment this reaches `main`, and every carrying repository reads as drifted from that point until it resyncs. That is the ordinary consequence of a canonical moving rather than a defect. The Skills installer added here is also how a machine picks the new skills up, so a session that keeps restating a rule already packaged as a skill is the signal to run it.
Why
Closes the root cause behind #706: an agent on a Windows host concluded commit
signing was broken and stopped work, based on
git config --get commit.gpgsign && ssh-add -L. That check tests one specific delivery path (an agent holdingthe key).
gpg.format=sshcan sign straight from a key file with nossh-agentrunning at all -- this host's actual setup -- so the probe failedwhile signing itself worked cleanly the whole time.
A follow-up comment on the same issue chased a second red herring: mixed
GPG/SSH signatures in
develop's history, read as evidence of astill-misconfigured fleet host. Investigating it here found no such host --
every GPG-signed commit on
developis a squash-merge, committed and signedby
GitHub <noreply@github.com>itself, server-side, regardless of what thePR author signed with locally. That's structural on every fleet repo, not
drift.
What
In
.agents/skills/git-commit-conventions/SKILL.md(.claude-plugin/regenerated via
scripts/build_dist.py):ssh-add -L, agpg-agentcheck) as aninvalid signing test, and states why.
%G?verdict character -- tech-agnostic across SSH agent-backed, SSH key-file,
GPG agent-backed, and GPG keyring signing, and immune to git version/locale
text differences that a "Good" grep would not be.
%aenow backs the identity check too, so oneprobe verifies both signing and identity instead of trusting
git configvalues that don't prove what lands on the commit object.agent checks
commit.committer.namebefore treating a differing signaturetype in history as a clue worth chasing.
Verified the new probe directly on this host (
sig=G email=ptr727@users.noreply.github.com) before committing with it.🤖 Generated with Claude Code