feat(vc): implement SSH commit signing (ENG-2002)#435
feat(vc): implement SSH commit signing (ENG-2002)#435
Conversation
|
Claude finished @hieuntg81's task in 2m 46s —— View job PR Review: feat(vc): implement SSH commit signing (ENG-2002)Tasks
Overall Score: 3 / 5Solid foundation — the SSH agent path, Critical Issues1. Architecture boundary violation:
// add.ts:4, config.ts:4
import {parseSSHPrivateKey, resolveHome} from '../../../server/infra/ssh/index.js'Per the documented architecture (CLAUDE.md): "All commands are daemon-routed: 2. Encrypted OpenSSH key decryption is silently not implemented
throw new Error(
'Encrypted OpenSSH private keys are not yet supported. ' +
'Please use an unencrypted key or load it via ssh-agent.',
)The user is prompted for a passphrase → sends it → gets the exact same error on every retry. The passphrase prompt loop is a dead end for OpenSSH-format encrypted keys. The PR description says "prompts for passphrase interactively (up to 3 retries) on encrypted keys" — this is misleading. Either: (a) implement bcrypt KDF + AES-256-CTR decryption, or (b) add a clear 3. RSA signing algorithm is wrong in
const rawSignature = sign(null, signedData, key.privateKeyObject)The code comment on line 51 correctly states "RSA: sign('sha512', data, key) — must specify hash", but the actual call always passes Medium Issues4. Surprising auto-sign default when only
const shouldSign = data.sign ?? config?.commitSign ?? config?.signingKey !== undefinedDue to operator precedence this evaluates as 5. No authentication guard in
private async createService(): Promise<ISigningKeyService> {
const token = await this.tokenStore.load()
const sessionKey = token?.sessionKey ?? '' // ← empty string fallback
const httpClient = new AuthenticatedHttpClient(sessionKey)
return new HttpSigningKeyService(httpClient, this.iamBaseUrl)
}When the user isn't logged in, an empty session key is used and the IAM API returns a generic 6. Missing test coverage for key new components Per CLAUDE.md ("80% coverage minimum, critical paths must be covered"), the following have zero tests:
The existing Minor Issues7. Deprecated Lines 62, 115 use const blob = response.slice(offset, offset + blobLen) // line 62
settle(() => resolve(accumulated.slice(4, 4 + responseLen))) // line 1158. Duplicate The 9. TTL mismatch: code says 30 min, PR says 60 s
constructor(ttlMs: number = 30 * 60 * 1000) { // 30 minutesThe PR description says "default 60 s". These should be consistent; 30-minute in-memory caching of a decrypted private key object is also a wider window than expected. 10. Sign indicator doesn't fire for config-based signing
const sigIndicator = sign === true ? ' 🔏' : ''When 11. Redundant
client.requestWithAck<IVcConfigResponse>(VcEvents.CONFIG, {importGitSigning: true, key: 'user.signingkey'})
Positive Notes
|
Critical:
- Move parseSSHPrivateKey/probeSSHKey/resolveHome to src/shared/ssh/ to fix
oclif/ → server/ import boundary violation
- Detect encrypted OpenSSH keys in resolveSigningKey and throw non-retryable
SIGNING_KEY_NOT_SUPPORTED instead of looping PASSPHRASE_REQUIRED prompts
- Fix RSA signing: use sign('sha512',...) and blob key-type 'rsa-sha2-512'
instead of null/'ssh-rsa' (per PROTOCOL.sshsig spec)
Medium:
- Fix shouldSign default: signingKey alone no longer auto-enables signing
(matches git behaviour — commit.sign must be explicitly set)
- Add NotAuthenticatedError guard to SigningKeyHandler.createService()
- Add missing tests: signing-key-handler, http-signing-key-service,
handleImportGitSigning edge cases
Minor:
- Replace deprecated Buffer.slice() with Buffer.subarray() in ssh-agent-signer
- Deduplicate sshStr helper to module scope in ssh-key-parser
- Clarify 30-minute TTL default in SigningKeyCache JSDoc
- Use result.signed from IVcCommitResponse for 🔏 indicator (shows when
signing is triggered via config, not only via --sign flag)
- Replace IVcConfigRequest with discriminated union — key is no longer
spuriously required when importGitSigning:true
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Claude finished @hieuntg81's task in 3m 15s —— View job PR Re-Review: feat(vc): implement SSH commit signing (ENG-2002)Tasks
Overall Score: 4.5 / 5All 3 critical issues, all 3 medium issues, and all 5 minor issues from the prior review have been addressed. The PR is in strong shape. A few small observations remain, noted below. Previously Critical — All Fixed ✅1. Architecture boundary violation — Fixed
// ssh-key-parser.ts (server/infra/ssh)
export { parseSSHPrivateKey, probeSSHKey, resolveHome, ... }
from '../../../shared/ssh/key-parser.js'Both 2. OpenSSH encrypted key dead loop — Fixed
if (probe.needsPassphrase && probe.opensshEncrypted) {
throw new VcError(
`Encrypted OpenSSH private keys are not supported for direct signing. ` +
`Load the key into ssh-agent first: ssh-add ${keyPath}`,
VcErrorCode.SIGNING_KEY_NOT_SUPPORTED,
)
}
3. RSA signing algorithm — Fixed
const isRsa = key.keyType === 'ssh-rsa'
const rawSignature = sign(isRsa ? 'sha512' : null, signedData, key.privateKeyObject)
const blobKeyType = isRsa ? 'rsa-sha2-512' : key.keyTypeRSA now correctly uses SHA-512 and produces Previously Medium — All Fixed ✅4. Auto-sign default — Fixed
const shouldSign = data.sign ?? config?.commitSign ?? falseSetting 5. Auth guard in
const token = await this.tokenStore.load()
if (!token?.isValid()) throw new NotAuthenticatedError()No longer falls back to an empty session key. 6. Missing test coverage — Fixed Two new test files added:
Previously Minor — All Addressed ✅
Remaining Minor ObservationsA. Dead code in if (cipherName !== 'none') {
if (!passphrase) {
throw new Error('Passphrase required for encrypted key') // ← unreachable via resolveSigningKey
}
throw new Error('Encrypted OpenSSH private keys are not yet supported...')
}Since B.
C.
Positive NotesAll the strengths from the original review still hold:
|
…(ENG-2002) - Remove dead `if (!passphrase)` branch in parseSSHPrivateKey — unreachable since resolveSigningKey short-circuits on opensshEncrypted before calling it - Add optional signingKeyService injection seam to SigningKeyHandlerDeps so action-routing paths (add/list/remove) can be unit tested without stubbing ES module instantiation; auth guard is still enforced when seam is used - Add 4 action-routing tests to signing-key-handler.test.ts (add/list/remove routing + auth guard with injected service) - Add missing handleImportGitSigning test: gpg.format=gpg → INVALID_CONFIG_VALUE Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Claude finished @hieuntg81's task in 2m 2s —— View job PR Re-Review (Round 3): feat(vc): implement SSH commit signing (ENG-2002)Tasks
Overall Score: 5 / 5All 3 minor observations from the Round 2 review have been fully addressed. The PR is ready to merge. Previously Minor — All Fixed ✅A. Dead code in The unreachable if (cipherName !== 'none') {
// resolveSigningKey short-circuits on opensshEncrypted before reaching here,
// so this is a safety net for direct callers.
throw new Error('Encrypted OpenSSH private keys are not yet supported...')
}B.
private async createService(): Promise<ISigningKeyService> {
const token = await this.tokenStore.load()
if (!token?.isValid()) throw new NotAuthenticatedError() // ← guard fires first
if (this.injectedService) return this.injectedService // ← seam used only if authed
...
}Four new action-routing tests were added in C. Three scenarios now covered in
Also confirmed: Positive NotesAll prior strengths still hold, and the PR is now complete end-to-end:
|
|
Claude finished @hieuntg81's task in 3m 58s —— View job PR Re-Review (Round 4): feat(vc): implement SSH commit signing (ENG-2002)Tasks
Overall Score: 5 / 5The trigger for this round was a Merge Impact AnalysisThe 38 files brought in from
All fixes from Rounds 1–3 remain intact after the merge:
One Pre-Existing Minor Observation (carried over, not newly introduced)
const parsed = await parseSSHPrivateKey(keyPath) // ← fails for encrypted keysWhen the user passes an encrypted OpenSSH private key (the most common case) and there is no Impact: Low — the workaround is to pass the SummaryThe merge from |
…NG-2002) signing-key add and vc config --import-git-signing previously called parseSSHPrivateKey to extract the public key, which fails for encrypted OpenSSH private keys with no .pub sidecar (the most common real-world case). The OpenSSH file format stores the public key in the unencrypted file header — decryption is never needed to derive it. Add extractPublicKey() to shared/ssh/key-parser.ts which: 1. Reads the .pub sidecar if present (preserves the comment field) 2. Falls back to parseOpenSSHKey() for OpenSSH-format files (no decryption) 3. Falls back to parseSSHPrivateKey() for PEM/PKCS8 keys Update add.ts and config.ts to call extractPublicKey() instead of parseSSHPrivateKey(). The .pub workaround still works and the sidecar comment is now used as the default title. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Claude finished @hieuntg81's task in 2m 2s —— View job PR Re-Review (Round 5): feat(vc): implement SSH commit signing (ENG-2002)Tasks
Overall Score: 5 / 5The trigger for this round is commit Round 4 Pre-Existing Observation — Fixed ✅
A new // 1. .pub sidecar (also captures comment field — used as default title)
// 2. OpenSSH native format — public key lives in the unencrypted header,
// so this works even when the private key is passphrase-protected
// 3. PEM/PKCS8 fallback — requires an unencrypted key (documented in JSDoc)
export async function extractPublicKey(keyPath: string): Promise<{
comment?: string
keyType: string
publicKeyBlob: Buffer
}>Both Bonus UX improvement: Test Coverage ✅Four new
The One Cosmetic Observation (Non-blocking)Assembled public key string omits the comment for private key inputs In publicKey = `${extracted.keyType} ${b64}` // ← no comment appendedThe SummaryAll 3 critical issues, all 3 medium issues, all 5 minor issues from Round 1, all 3 minor observations from Round 2, the pre-existing Round 4 observation — all addressed. The SSH commit signing feature is complete, architecturally sound, and well-tested. |
|
Claude finished @hieuntg81's task in 1m 20s —— View job PR Re-Review (Round 6): feat(vc): implement SSH commit signing (ENG-2002)Tasks
Overall Score: 5 / 5The trigger for this round was a Merge Impact AnalysisThe 35 files brought in from
All fixes from Rounds 1–5 remain intact after the merge:
SummaryThe merge from |
…cure passphrase handling for git commits
|
Claude finished @hieuntg81's task in 4m 5s —— View job PR Re-Review (Round 7): feat(vc): implement SSH commit signing (ENG-2002)Tasks
Overall Score: 4.5 / 5Commit Medium Issues1.
return /bad decrypt|passphrase|bad password|interrupted or cancelled|unsupported/.test(msg)Adding // probeSSHKey:221
throw new Error(`Unsupported OpenSSH key type: ${parsed.keyType}. Only ssh-ed25519 is supported natively. Please load this key into ssh-agent instead.`)
// probeSSHKey:236
throw new Error(`Unsupported PEM key type: ${pk.asymmetricKeyType}. Only ed25519 is supported natively. Please load this key into ssh-agent instead.`)These errors contain "Unsupported" → Resulting UX for an RSA/ECDSA key not loaded in the agent:
Recommended fix: Remove Minor Issues2.
passphrase: Flags.string({
description: 'SSH key passphrase (prefer BRV_SSH_PASSPHRASE env var)',
}),Values passed as CLI flags are visible in 3. Redundant
export type VcConfigKey = 'commit.sign' | 'user.email' | 'user.name' | 'user.signingkey' | 'user.signingKey'
export const VC_CONFIG_KEYS: readonly string[] = ['user.name', 'user.email', 'user.signingkey', 'user.signingKey', 'commit.sign']The server already normalises with 4. Docs page is in Vietnamese only
Positive NotesAll fixes from Rounds 1–6 remain intact. The new commit also adds several genuine improvements:
SummaryThe PR is solid overall. Issue #1 (confusing passphrase prompt for unsupported key types) is the only one worth fixing before merge. Issues #2–#4 are minor improvements that can go in this PR or a follow-up. |
bao-byterover
left a comment
bao-byterover
left a comment
There was a problem hiding this comment.
Please resolve, i also add a comment on ticket 2002.
There was a problem hiding this comment.
issue (block):
I think this file should not exits, or if really needed, please make it 100% english.
|
|
||
| let pp: string | ||
| try { | ||
| pp = await password({ |
There was a problem hiding this comment.
issue(block):
Oclif command imports @inquirer/prompts and calls password() — violates your 2026-04-17 rule "oclif non-interactive; all input via flags/args." TTY gate at :69 narrows but doesn't remove the interactive path.
|
|
||
| // Derive public key fingerprint from key file (reads only public key — no passphrase needed) | ||
| const parsed = await getPublicKeyMetadata(keyPath).catch(() => null) | ||
| if (!parsed) return null |
There was a problem hiding this comment.
issue (block):
Please let claude code read the CLAUDE.md latest, it strict about return value undefined instead of null.
|
Claude finished @hieuntg81's task in 4m 29s —— View job PR Re-Review (Round 8): feat(vc): implement SSH commit signing (ENG-2002)Tasks
Changes in commit
|
PROTOCOL.sshsig defines MAGIC_PREAMBLE as byte[6] "SSHSIG" for both the envelope and the signed-data structure. The previous 'SSHSIG\0' (7 bytes, with null terminator) made every signature reject under `git verify-commit` and CoGit's verifier. Add a round-trip test against `ssh-keygen -Y check-novalidate` so this class of spec-misreading bug cannot recur silently. Also fix two pre-existing self-asserting tests that re-encoded the wrong preamble. Fixes ENG-2002 AC4 (and unblocks AC5). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
isPassphraseError() previously matched /unsupported/, which collided with parseOpenSSHKey's own "Unsupported OpenSSH key type: ..." error string. probeSSHKey then reported needsPassphrase:true for any non-Ed25519 OpenSSH key, triggering a spurious passphrase prompt instead of surfacing the real "unsupported key type" error. Add regression tests for RSA and ECDSA OpenSSH-format keys. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
B7 reported that RSA/ECDSA OpenSSH keys triggered repeated passphrase prompts (RSA: 2, ECDSA: 3) and theorised an "implicit PASSPHRASE_REQUIRED conversion path" elsewhere in the transport layer. Tracing the code shows the only PASSPHRASE_REQUIRED throw is in resolveSigningKey, gated on probe.needsPassphrase. With B6's regex fix in place probeSSHKey now throws "Unsupported OpenSSH key type" directly for non-Ed25519 OpenSSH keys, so PASSPHRASE_REQUIRED is never produced and the CLI retry loop never starts. This integration test exercises handleCommit end-to-end with a synthetic ssh-rsa OpenSSH key and asserts the propagated error is the unsupported keytype error, NOT PASSPHRASE_REQUIRED. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…2002 B1) Per Bao's review (2026-04-17), oclif commands run non-interactively and must not collect user input via inquirer prompts — passphrase entry belongs in the TUI's Ink layer per ticket §Signing Flow step 2. Replace the @inquirer/prompts password() + retry-recursion logic with a clear actionable error directing users to --passphrase or BRV_SSH_PASSPHRASE. Add two examples covering both invocation styles (addresses B1-examples). The Ink-layer passphrase component for the TUI remains an open follow-up (ticket §Signing Flow step 2) — out of scope for this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…NG-2002 B2) Bao's review (2026-04-17) noted the prior doc was Vietnamese-only and missing several critical pieces. This rewrite addresses all four gaps: - English (replaces VN-only — accessible to the full team) - Cross-platform notes for macOS, Linux, Windows native, and WSL - `--passphrase` flag and `BRV_SSH_PASSPHRASE` env var documented - Encrypted-OpenSSH and RSA/ECDSA caveats with the ssh-add → ssh-agent recovery path Also adds a supported-key-formats matrix that makes the narrowed v1 scope explicit (Ed25519-unencrypted-file native; everything else via ssh-agent), plus a troubleshooting table covering the four user-facing error strings in the current implementation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…2 C2)
The B6 fix dropped /unsupported/ from the message-fallback regex but left
the code-based check at `code.startsWith('ERR_OSSL')`, which still matches
`ERR_OSSL_UNSUPPORTED`. Node.js crypto emits this code for any PEM body
it cannot decode (malformed PKCS8, unsupported algorithm OIDs, garbage
payload), so probeSSHKey continued to false-report needsPassphrase:true
for any unparseable PEM file — the same B6 class of bug on a different
code path.
Replace the prefix match with an explicit whitelist of the two codes that
genuinely indicate a passphrase issue: ERR_OSSL_BAD_DECRYPT and
ERR_OSSL_CRYPTO_INTERRUPTED_OR_CANCELLED.
Adds a regression test that constructs a minimal malformed PEM whose body
is two characters of base64 garbage — reliably surfaces ERR_OSSL_UNSUPPORTED
across Node versions.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…(ENG-2002 C1+M1) After B0 fix the envelope and signed-data preambles are both 6-byte "SSHSIG" — having two named constants (SSHSIG_MAGIC and SSHSIG_SIGNED_DATA_MAGIC) for identical buffers misled future readers into thinking the spec required different prefixes. ssh-agent-signer.ts also still carried the pre-B0 inline comment "Build signed data (uses 7-byte magic WITH null terminator)" which now contradicted the code outright. - Export a single SSHSIG_MAGIC from sshsig-signer.ts and import it in ssh-agent-signer.ts (single source of truth). - Drop the stale 7-byte comment, replace with a one-line spec reference. - Remove the redundant SSHSIG_SIGNED_DATA_MAGIC alias. No behavior change. All 52 ssh unit tests still pass including the ssh-keygen round-trip verifier. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three test-quality fixes from review:
M2: vc-handler.test.ts created tempdirs inline (brv-enc-key-test-*,
brv-rsa-key-test-*) without cleanup, leaking one dir per run. Wrap
both bodies in try/finally + rmSync.
M3: B7 regression test wrapped the PASSPHRASE_REQUIRED-must-not-fire
assertion in `if (error instanceof VcError)`. Because probeSSHKey
now throws a plain Error for unsupported keytypes, that branch was
skipped — the assertion silently disappeared. Pin behavior with
`expect(error).to.not.be.instanceOf(VcError)` so any future wrap
into VcError is caught and reviewed deliberately.
M5: ssh-key-parser.test.ts had beforeEach() that mkdtempSync'd a fresh
tempdir for every test in three describe blocks but no matching
afterEach/after. Added cleanup hooks to all three.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- m1: drop `as {code: string}` assertions in commit.ts and key-parser.ts.
TS narrows `err.code` correctly via `'code' in err && typeof err.code === 'string'`.
- m2: remove ticket reference (ENG-2002 §Signing Flow step 2) from
commit.ts comment — explanation already conveys WHY.
- m3: rename test payload "B0 round-trip" → "round-trip test payload"
so it doesn't bake a task ID into test data.
- m4: rephrase docs PEM/PKCS8 jargon as "the narrow exception" with a
pointer to ssh-agent as the expected path for most users.
- m6: drop `console.warn` from skipped-suite branch in sshsig round-trip
test; Mocha already prints "pending" when this.skip() fires.
No behavior change. All 52 ssh tests + typecheck + lint clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rationale (ENG-2002 M6)
Both signers had `const HASH_ALGORITHM = 'sha512'` inlined, and bare
`createHash('sha512')` / `sign('sha512', ...)` calls mirrored it.
Bao flagged this as "sha512 hard-coded, no sha256 fallback".
The hard-coding is intentional, not an oversight:
- Ed25519 (primary supported key type) mandates SHA-512 — the EdDSA
algorithm has no other choice.
- RSA via ssh-agent already uses the RSA-SHA2-512 flag per OpenSSH
default.
- Every OpenSSH verifier from 2017+ accepts sha512-labelled sshsig
signatures; there is no consumer-compat benefit to offering sha256.
Move the constant into sshsig-constants.ts alongside SSHSIG_MAGIC_PREAMBLE
and spell out the three reasons inline, so the next reviewer does not
ask the same "why is this hard-coded" question. Both signers import the
shared constant; all four `'sha512'` string literals become references.
If a future need arises, the doc explicitly tells the next developer to
thread the constant through signCommitPayload and SshAgentSigner.sign
as a parameter rather than duplicating the literal.
No behavior change. 57 SSH tests + ssh-keygen round-trip still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @hieuntg81's task in 5m 53s —— View job PR Re-Review (Round 10): feat(vc): implement SSH commit signing (ENG-2002)Tasks
Overall Score: 4.5 / 5Six new commits since Round 9 bring several genuine improvements; one blocking issue (B3) remained unfixed by the developer and has been resolved in commit Blocking Issue Fixed in this RoundB3 — Three files still returned
All New Commits — All Reviewed ✅
Project boundary isolation: two projects sharing a keyPath no longer share a cached
Generic
Extracted to
Minor Observations (Non-blocking)A. Dead
onSubmit={(pp) => {
passphraseRef.current = pp // ← written but never read
dispatch({type: 'passphrase-submitted'})
fireCommit(pp) // ← pp passed directly
}}
B.
description: 'SSH key passphrase (or set BRV_SSH_PASSPHRASE env var)',CLI flag values are visible in Positive NotesAll strengths from prior rounds hold, plus the new commits add:
|
…ew A) The ref was written on InlinePassword.onSubmit but never read — the passphrase flows directly through fireCommit(pp) in the same synchronous handler, so there is no transition to bridge. Bao PR #435 Round 10 Minor A. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…2002 review B) CLI flag values appear in \`ps aux\` (same UID) and shell history. Explicit warning at point-of-use so users don't need to read the standalone docs first. Repeats the BRV_SSH_PASSPHRASE / ssh-agent guidance already in docs/ssh-commit-signing.md. Bao PR #435 Round 7 Minor #2 / Round 10 Minor B. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @hieuntg81's task in 3m 30s —— View job PR Re-Review (Round 11): feat(vc): implement SSH commit signing (ENG-2002)Tasks
Overall Score: 5 / 5Both minor observations from Round 10 have been fully addressed. The B3 Round 10 Observations — All Fixed ✅A. Dead The B.
description:
'SSH key passphrase — WARNING: visible in process list and shell history. Prefer BRV_SSH_PASSPHRASE env var or ssh-agent.',Clear, actionable, and correctly steers users toward the safer alternatives. ✅ Blocking Issue (B3) — Fixed Locally, Push Blocked
CLAUDE.md requires
All callers (
SummaryOnce |
Conflicts resolved: - src/server/core/interfaces/services/i-git-service.ts: Keep onSign callback on CommitGitParams (SSH signing, HEAD) and add filepath? to LogGitParams (new from main). - src/server/infra/process/feature-handlers.ts: Register both SigningKeyHandler (HEAD) and ContextTreeHandler (main). - test/unit/infra/transport/handlers/vc-handler.test.ts: Keep all 5 handleCommit tests — 3 SSH signing regression cases (HEAD) plus 2 conflict detection cases (main). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…2002 review #17) The PEM branch of extractPublicKey called parseSSHPrivateKey(keyPath) with no passphrase. For an encrypted PEM key without a .pub sidecar, this surfaced either a misleading "Wrong passphrase" (user never entered one) or a raw OpenSSL code — violating AC9-b/c. Wrap the call with passphrase-error detection and throw a user-grade error pointing at `ssh-keygen -y -f` as the canonical workaround. Affects: `brv signing-key add --key <private-key>` and `brv vc config --import-git-signing` when the configured key is an encrypted PEM without a .pub sidecar. `vc commit` is unaffected — it goes through probeSSHKey + explicit passphrase flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Added in 2e4d228 with the intent of safe payload logging, but never wired into any log or error-construction site. The commit path builds VcError instances with pre-formatted messages rather than serialising the incoming payload, so there is no consumer for the helper today. Delete rather than wire speculatively — re-introduce with a concrete log site if one ever materialises. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… review #23) The test under describe('list action') asserted only that the request handler is a function — not that `list` maps IAM responses. Setup registration is covered by describe('setup'), and the injectable-service seam tests under describe('action routing (via injectable service seam)') already exercise the real list → mapResource → toSigningKeyItem path. Removing the fake eliminates a false coverage signal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…g change (ENG-2002 review #14) When user ran `brv vc config user.signingkey <newpath>`, handleConfig cached the new ParsedSSHKey but did not evict the entry for the previously configured path. The stale ParsedSSHKey (with its decrypted crypto.KeyObject) sat in daemon memory until its 30-min TTL expired — low-severity but avoidable leak since SigningKeyCache already exposes invalidate(projectPath, keyPath) for exactly this case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…iew #16) resolveHome only expands a leading ~. Relative paths like `./id_ed25519` or bare `id_ed25519` passed straight through and then resolved against the daemon's CWD — so the same config value worked today and broke tomorrow if the daemon restarted from a different directory. Reject non-absolute paths at both entry points (handleConfig set and handleImportGitSigning) with INVALID_CONFIG_VALUE. Matches the semantic of `git config user.signingKey`, which also requires an absolute path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…(ENG-2002 review #18) handleConfig (and handleImportGitSigning) wrapped parseSSHPrivateKey in an empty catch block, so any non-passphrase failure (corrupt file, race between probe and parse, unexpected format) disappeared silently. The config set returned success with no hint and no warning — the user only discovered the problem at the next `brv vc commit --sign`, with no breadcrumb back to the config step. Detect passphrase-class errors via isPassphraseError (exported for this use) and skip the hint for those (intended behavior — encrypted keys need the passphrase to derive a fingerprint). For any other error, surface the message in the hint so the failure is visible at the point the user took the action. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…te (ENG-2002 review #26) React-Query retains mutation.variables until `reset()` is called explicitly or gcTime elapses (default 5 min). Because this PR added `passphrase` to the mutation payload, the plaintext passphrase lived in TUI memory for ~5 min after every successful commit — exposed to devtools, error boundaries, and any diagnostic dumper that walks the query cache. Call commitMutation.reset() in the same useEffect that bubbles the terminal state up to the dialog manager. No dedicated test: the project has no React/Ink testing library wired up, and the change is a one-liner inside a clearly-scoped useEffect. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…iew #21+#22) Two protocol-hardening guards against a misbehaving or truncated SSH agent response: #21 — request() used to resolve with whatever bytes the agent delivered, even when the body was zero-length. Downstream `response[0] !== SSH_AGENT_IDENTITIES_ANSWER` then formatted "undefined" into the error message, losing the actual fault. Reject empty bodies at the request() boundary with a clear message. #22 — sign() read `sigLen = readUInt32(response, 1)` and returned `subarray(5, 5 + sigLen)` without checking the declared length against the actual body length. A truncated response yielded a short signature silently; downstream `crypto.verify()` would then reject it with a generic "bad signature" error instead of surfacing the agent-boundary problem. Compare sigLen against `response.length - 5` and throw a descriptive error when mismatched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e.success (ENG-2002 review #19+#20) #19 — `brv vc config --import-git-signing` previously sniffed four English substrings ("already exists", "Duplicate", "ALREADY_EXISTS", "409") on the error message to decide whether a key was already registered. If IAM ever returned "Conflict: ...", "RESOURCE_EXISTS", "DUPLICATE_KEY", or any localized variant, the substring check fell through into the "run brv signing-key add" hint — which would then 409 again. Translate the HTTP 409 at the boundary that knows about it (HttpSigningKeyService) into a new VcErrorCode.SIGNING_KEY_ALREADY_EXISTS, and match that code structurally at the CLI via TransportRequestError.code. #20 — listKeys silently returned [] when the response envelope carried `{success: false}`. Callers had no way to distinguish "no keys" from "request semantically failed." AuthenticatedHttpClient throws on non-2xx responses, so this only bites on 200 OK + success:false — but that's an envelope we shouldn't trust implicitly. addKey and listKeys now both throw on success:false for symmetric defensive posture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… gitconfig (ENG-2002 review #24) Existing handleImportGitSigning tests all write config into the per-repo local config, but the most common real-world setup is a single user.signingKey in ~/.gitconfig. If a later refactor skips the global lookup, every existing test still passes while the most common user configuration ships broken. Add a regression test that points GIT_CONFIG_GLOBAL at a temp config file (so git child processes inherit it) and asserts the signingKey is imported with no local config involvement. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eview #25) Every existing agent-signer test uses ed25519, so the RSA-specific branch (`flags = this.keyType === 'ssh-rsa' ? SSH_AGENT_RSA_SHA2_512 : 0`) is untested. A refactor from `=== 'ssh-rsa'` to `.startsWith('rsa-')` or `=== 'rsa'` would go green across the whole suite while silently signing RSA with SHA-1 — breaking the v1 recovery path documented for users whose encrypted ~/.ssh/id_rsa lives in ssh-agent. This test drives tryGetSshAgentSigner via a .pub sidecar labeled "ssh-rsa" (keyType detection happens there) and a mock agent that captures the flags uint32 from the SIGN request. Asserting capturedFlags === 4 pins the SSH_AGENT_RSA_SHA2_512 branch exactly. Full crypto round-trip (verify the agent signature) is out of scope — a valid RSA SSH private key in OpenSSH format would need to be generated outside Node and checked in as a test fixture. The flag assertion catches the only branch we actually care about at this layer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…#2+#3+#4+#5) Four unrelated-by-line-number but related-by-pattern cleanups, all CLAUDE.md "Avoid as Type / ! non-null assertions" hygiene. #2 key-parser.ts parseOpenSSHKey — runtime check went through VALID_SSH_KEY_TYPES.has(string), then cast to SSHKeyType. Extract that into an exported isValidSSHKeyType predicate so the narrowing is visible to the compiler. #3 key-parser.ts parseSSHPrivateKey — KeyObject.export has a string | Buffer overload; the cast masked Node's narrowing. Swap to Buffer.isBuffer and throw if the export ever returns a string. A naive Buffer.from(exported) would UTF-8-encode and silently corrupt DER. #4 ssh-agent-signer.ts tryGetSshAgentSigner — narrow the return type of getPublicKeyMetadata to {keyType: SSHKeyType} (enforced via isValidSSHKeyType on the .pub sidecar branch, already guaranteed on the OpenSSH branch after #2). The call-site cast drops out naturally. Also normalises the sentinel from null to undefined along the way (addresses review #13 since both touch the same signature). #5 signing-key/add.ts — `publicKey!` was papering over oclif's lack of an `asserts never` declaration on this.error(). Replace with an explicit `if (!publicKey) this.error(...)` narrow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…#7+#8+#9+#10) Per CLAUDE.md: "Prefer type for data-only shapes (DTOs, payloads, configs); prefer interface for behavioral contracts with method signatures." All of the following are plain data bags — no method signatures on the shape itself: #6 SigningKeyResource (i-signing-key-service.ts) #7 ApiEnvelope<T>, CreateSigningKeyData, ListSigningKeysData, RawSigningKeyResource (http-signing-key-service.ts) #8 SigningKeyHandlerDeps (signing-key-handler.ts) — record of service refs, no methods on the deps struct itself #9 ParsedSSHKey, SSHSignatureResult, SSHKeyProbeResult, SSHKeyProbeResultFound (shared/ssh/types.ts) #10 SigningKeyItem (shared/transport/events/vc-events.ts) ISigningKeyService stays as an interface — it's a behavioral contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
#12) Per CLAUDE.md: "Default to undefined over null; reserve null only for external boundaries (storage, HTTP APIs) that force it." These are pure in-memory utilities with no external boundary: #11 SigningKeyCache.get — Map-backed TTL wrapper. Switch the miss / expired sentinel to undefined, update the 10 call-site assertions in signing-key-cache.test.ts. #12 tryGetSshAgentSigner — local factory for the agent-signer chain. Switch the "no signer available" sentinel to undefined and propagate through the `.catch(() => undefined)` in the inner getPublicKeyMetadata call. Update ssh-agent-signer.test.ts assertions. #13 (getPublicKeyMetadata) was handled in the earlier as/! refactor commit because it sits on the same signature. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…iew #15) The sweep loop that evicts expired entries used to run only inside set(). Long-running daemons that are read-heavy (cache once, sign many) therefore never reclaimed memory for keys the user had stopped using — get() only deleted the single entry it happened to touch. Extract `sweep(now)` as a private method and call it from both get() and set(). Ceiling growth was already practically bounded (projects × keys × ~1KB) so this is mostly a code-smell / symmetry fix rather than a memory leak. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @hieuntg81's task in 5m 29s —— View job PR Re-Review (Round 12): feat(vc): implement SSH commit signing (ENG-2002)Tasks
Overall Score: 5 / 5Fifteen new commits landed since Round 11. All prior blocking/medium/minor items through Round 11 are confirmed intact. The new commits resolve a comprehensive additional review pass (labeled #1–#26 in commit messages). One cosmetic formatting nit remains, noted below. Previously Blocking (B3) — Fixed ✅
All New Commits — All Reviewed ✅
One Cosmetic Nit (Non-blocking)Missing indentation on export class SigningKeyCache {
private readonly cache = new Map<string, CacheEntry>()
private readonly ttlMs: number // ← missing 2-space indentLine 30 is missing its 2-space class-member indent — likely a copy/paste artefact from commit Positive NotesAll strengths from prior rounds remain intact, plus the new batch adds:
The PR is clean, architecturally sound, and well-covered by tests. Ready to merge once the cosmetic indentation nit is addressed (or accepted as-is). |
|
Claude finished @hieuntg81's task in 1m 47s —— View job PR Re-Review (Round 13): feat(vc): implement SSH commit signing (ENG-2002)Tasks
Overall Score: 5 / 5The trigger for this round was commit Merge Impact AnalysisThe 4 files brought in from
All fixes from Rounds 1–12 remain intact after the merge:
Cosmetic Nit Carried Over from Round 12 — Fixed ✅Missing indentation on // Before
export class SigningKeyCache {
private readonly cache = new Map<string, CacheEntry>()
private readonly ttlMs: number // ← missing 2-space indent// After
export class SigningKeyCache {
private readonly cache = new Map<string, CacheEntry>()
private readonly ttlMs: number // ✅
SummaryThe merge from |
|
Claude finished @hieuntg81's task in 59s —— View job PR Re-Review (Round 14): feat(vc): implement SSH commit signing (ENG-2002)Tasks
Overall Score: 5 / 5The sole trigger for this round is commit Round 13 Cosmetic Nit — Fixed ✅Missing indentation on // Before (copy/paste artefact from 57db09e)
export class SigningKeyCache {
private readonly cache = new Map<string, CacheEntry>()
private readonly ttlMs: number // ← missing 2-space indent
// After
export class SigningKeyCache {
private readonly cache = new Map<string, CacheEntry>()
private readonly ttlMs: number // ✅Only All Fixes From Rounds 1–13 Remain Intact ✅
SummaryAll 3 critical issues, 3 medium issues, 5 minor issues (Round 1), 3 observations (Round 2), pre-existing observation (Round 4), medium issue (Round 7), blocking issues B1/B2/B3 (Rounds 8–11), and the cosmetic indentation nit (Rounds 12–13) — all addressed. The SSH commit signing feature is complete, architecturally sound, spec-compliant, and well-tested. Ready to merge. |
|
Claude finished @hieuntg81's task in 2m 17s —— View job PR Re-Review (Round 15): feat(vc): implement SSH commit signing (ENG-2002)Tasks
Overall Score: 5 / 5The trigger for this round was commit Merge Impact AnalysisThe 3 files brought in from
All fixes from Rounds 1–14 remain intact after the merge:
SummaryThe 3.8.2 merge brings only a CHANGELOG entry, a version bump, and a All 3 critical issues, 3 medium issues, 5 minor issues (Round 1), 3 observations (Round 2), pre-existing observation (Round 4), medium issue (Round 7), blocking issues B1/B2/B3 (Rounds 8–11), and the cosmetic indentation nit (Rounds 12–13) — all resolved. The SSH commit signing feature is complete, architecturally sound, spec-compliant, and well-tested. Ready to merge. |
2 participants
Summary
user.name/user.emailarbitrarily.brv signing-keycommand group (add,list,remove) to manage SSH public keys registered in the Byterover IAM API.brv vc commitgains--sign/--no-signflags; prompts for passphrase interactively (up to 3 retries) on encrypted keys.brv vc configsupports two new keys (user.signingkey,commit.sign) and a--import-git-signingflag that reads local/global git config and imports SSH signing settings automatically.ssh-keygensubprocess), SSH agent client (Unix socket protocol),sshsigenvelope signer, and an in-process signing-key fingerprint cache.HttpSigningKeyService) for signing-key CRUD against/api/v3/users/me/signing-keys.vc:signing-keyand error codesPASSPHRASE_REQUIRED,SIGNING_KEY_NOT_CONFIGURED,SIGNING_KEY_NOT_FOUND,INVALID_CONFIG_VALUE.Type of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)N/A
Test plan
test/unit/infra/ssh/signing-key-cache.test.tstest/unit/infra/ssh/ssh-agent-signer.test.tstest/unit/infra/ssh/ssh-key-parser.test.tstest/unit/infra/ssh/sshsig-signer.test.tstest/unit/infra/transport/handlers/vc-handler.test.ts(extended)ed25519,rsa, andecdsakey types (with and without passphrase)sshsigenvelope construction and base64 armoringvc-handlersigning path: sign flag propagation,PASSPHRASE_REQUIREDerror surfacingUser-visible changes
brv signing-key add -k ~/.ssh/id_ed25519 [--title "My laptop"]? registers SSH public key with Byterover.brv signing-key list? lists registered signing keys with fingerprints.brv signing-key remove <id>? removes a registered key.brv vc commit -m "msg" --sign? signs the commit;--no-signoverridescommit.sign=truein config.brv vc config user.signingkey ~/.ssh/id_ed25519? sets the signing key path.brv vc config commit.sign true? enables auto-signing for all commits.brv vc config --import-git-signing? importsuser.signingKey+gpg.format=ssh+commit.gpgSignfrom local or global git config.Evidence
Checklist
npm test)npm run lint)npm run typecheck)npm run build)mainRisks and mitigations
sshsig-signer.tswhich uses Node's nativecrypto.sign. Unsupported formats surface a clear error rather than silently producing a bad signature.SIGNING_KEYtransport event handler is a new surface area that calls the IAM API on every signed commit.SigningKeyCachecaches fingerprints with a configurable TTL (default 60 s) to avoid per-commit IAM round-trips.