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>
…_OR_CANCELLED Empirically (Node v24): createPrivateKey() on an encrypted PEM with NO passphrase argument throws ERR_OSSL_CRYPTO_INTERRUPTED_OR_CANCELLED. The previous comment described it as "Node-internal cancellation during prompt", suggesting the code path only fires under interactive entry. That was wrong — the code is the normal failure mode for the entire non-interactive encrypted-PEM-no-passphrase flow that brv relies on. Future maintainers reading the wrong comment could justify removing the whitelist entry as "dead code", which would break passphrase prompting for every encrypted PEM key. Add a regression test that constructs an encrypted Ed25519 PEM key, calls probeSSHKey with no passphrase, and asserts needsPassphrase:true — pinning both the OpenSSL behavior and the corrected comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…odule The previous setup exported SSHSIG_MAGIC from sshsig-signer.ts and imported it into ssh-agent-signer.ts. That leaked an implementation detail (the spec preamble byte sequence) onto the public surface of a module whose responsibility is signing, not constants. Move SSHSIG_MAGIC into a tiny sshsig-constants.ts dedicated to PROTOCOL.sshsig spec literals. Both signers import from it equally, no circular imports, and the spec context lives in one place where parser / verifier code can join it later if needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nce (review-2) Three small test-quality fixes from the second review pass: - vc-handler B7 test: replace `(error as Error).message` with a proper `if (!(error instanceof Error)) expect.fail(...)` narrowing pattern so the assertion no longer relies on a CLAUDE.md-banned `as` cast. - ssh-key-parser parseSSHPrivateKey: add a one-line comment explaining why this describe block uses before/after instead of beforeEach (every test is read-only against keyPath; recreating per-test is wasted I/O). - sshsig-signer round-trip suite header: drop the lingering "B0/ENG-2002" bug-tag reference; reword to describe the trap more generally so the comment is still meaningful after the ticket is closed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…review-2) Symptom column previously truncated each error mid-sentence (e.g. 'Error: Encrypted OpenSSH private keys are not supported for direct signing.' ending at the first period), so a user grepping their terminal output for the documented string would not get an exact match and might assume the doc covered a different error. Reword to show the actual leading substring of each error verbatim, ending with `…` to mark the tail (key path or key type) that varies per invocation. Add a one-line note above the table making the prefix match convention explicit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three test-quality fixes from the third review pass: CRITICAL: vc-handler B7 test had `expect(error).to.be.instanceOf(Error)` followed by `if (!(error instanceof Error)) expect.fail(...)`. Because the Chai assertion already throws, the if-block was 100% dead — TS narrowing was satisfied but the guard never ran. Restructure so the `expect.fail`-based narrowing comes first; subsequent assertions then operate on a properly typed Error instance. MAJOR: ssh-key-parser RSA and ECDSA "unsupported key type" tests still contained `(caught as Error).message` — bypassed the same review-2 fix that landed in vc-handler. Apply the same `expect.fail` narrowing pattern so all three test sites are consistent. MINOR: parseSSHPrivateKey before/after comment now warns future contributors that tests in this block must stay read-only against keyPath; if mutation is needed, use a separate per-test tempDir. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… Buffer (review-3)
Buffer is index-mutable in Node.js — `MAGIC[0] = 0` is legal and
silently corrupts every signature thereafter. The previous
`export const SSHSIG_MAGIC = Buffer.from('SSHSIG')` shared one Buffer
reference across both signers, so any accidental write anywhere in the
process would taint both. For a cryptographic protocol constant that's
unacceptable.
Export the source string instead. Each signer module converts to its
own local Buffer at module load — fresh allocation, no cross-module
mutation risk.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eview-4)
Three small follow-ups from the fourth review pass:
- sshsig-constants.ts: previous comment claimed "each caller converts
to its own Buffer at module load" which implied per-call allocation.
In reality each importing module materialises one module-private
Buffer once at load. Reword so the safety claim matches the actual
behavior — the Buffer is per-module, not per-call.
- ssh-key-parser.test.ts: a leftover `await import('node:crypto')`
inside `it('privateKeyObject is a valid KeyObject that can sign')`
remained from before review-2's static `generateKeyPairSync` import
landed. Hoist `sign` into the same static import line.
- ssh-key-parser.test.ts: the ECDSA "unsupported key type" test used a
technical fail message (`Expected Error, got ${typeof caught}`)
while the sibling RSA test used a behavior-contract message. Make
them consistent (behavior contract wins — it explains why the test
exists, not just what failed).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…G-2002 AC9-b/c) End-to-end verification revealed that two AC9 paths still surface raw OpenSSL codes to the user: - AC9-b (wrong format): a non-key file at user.signingkey causes `createPrivateKey` to throw `error:1E08010C:DECODER routines::unsupported` during config validation. The raw error propagates unchanged. - AC9-c (wrong passphrase): a valid encrypted PEM with the wrong passphrase surfaces `error:1C800064:Provider routines::bad decrypt`. Introduce a small `formatCryptoError` helper that maps the two known OpenSSL codes to actionable English and leaves unknown errors untouched (so rare failures still surface for debugging): - ERR_OSSL_UNSUPPORTED → "File at <path> is not a valid SSH private key (unrecognised or malformed PEM / PKCS8 body)." - ERR_OSSL_BAD_DECRYPT → "Wrong passphrase for SSH key at <path>." Called from the probeSSHKey outer catch (after passphrase detection so encrypted-PEM-no-passphrase still returns needsPassphrase:true) and around the PEM createPrivateKey call in parseSSHPrivateKey. Regression tests assert the new messages and that neither contains the raw OpenSSL code fragment. End-to-end smoke verified against a real brv vc commit flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @hieuntg81's task in 4m 20s —— View job PR Re-Review (Round 9): feat(vc): implement SSH commit signing (ENG-2002)Tasks
Changes since Round 8 (commits after
|
…2002 §Signing Flow step 2) The oclif `vc commit` command deliberately has no interactive prompt (ENG-2002 B1 per Bao's "oclif is non-UI" rule), so users on encrypted keys must supply `--passphrase` or BRV_SSH_PASSPHRASE. The TUI — which does support interactive input — must still fulfil ticket §Signing Flow step 2 "prompt user for passphrase (Ink input)". Adds: - InlinePassword (src/tui/components/inline-prompts/inline-password.tsx) — masked-input variant of InlineInput. Renders `*` per character, never writes the raw value to the Ink text stream, supports Escape for cancellation and rejects empty submissions. - vc-commit-flow-state.ts — pure reducer for the commit flow state machine (committing → awaiting-passphrase → done). Split out of the React component so the transitions are unit-testable without spinning up an Ink tree. Caps retries at MAX_PASSPHRASE_RETRIES=3 (matches the pre-B1 oclif limit). - VcCommitFlow rewired to useReducer(reduceCommitFlow). When the daemon returns PASSPHRASE_REQUIRED, state transitions to awaiting-passphrase, InlinePassword renders, and submitting fires another commit mutation with the passphrase in the payload. Cancel on Escape yields "Passphrase entry cancelled." Tests: 13 unit tests cover the reducer (happy path, retry cap, terminal absorbing, out-of-order events, signed/unsigned SHA formatting). The component itself is not rendered in tests — this project has no .tsx test files and no ink-testing-library dependency, consistent with its convention of testing logic rather than Ink trees. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ENG-2002 M4) VcConfigKey type used to include both `'user.signingkey'` and `'user.signingKey'`, and VC_CONFIG_KEYS listed them both — a duplicate masquerading as two valid keys. The `git config` CLI convention is all lowercase, the internal FIELD_MAP lookup already normalises via toLowerCase(), and the file store always writes a single canonical `signingKey` field. The dual spelling was surface noise. Canonicalise on lowercase. isVcConfigKey() keeps the case-insensitive parse so legacy camel-case callers still work at runtime, but new code cannot depend on the variant via the type. Regression test covers lowercase / camel / upper inputs and rejects unrelated keys. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…002 M5)
`brv signing-key remove <id>` previously executed the remote IAM delete
immediately, with no safeguard. A typo in the ID (or a stale key from
`brv signing-key list` output) silently destroyed the wrong credential.
Add a mandatory --yes flag. Per Bao's oclif non-interactive rule, the
command cannot prompt; instead, without --yes the command exits 2 with:
Refusing to remove signing key '<id>' without explicit --yes
confirmation. This action is irreversible. Re-run with --yes to proceed.
Users who really mean it pass --yes. The help text declares the flag.
Test stubs this.error and asserts the guard fires before any network
call, plus confirms the flag is declared on the command.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…) (ENG-2002 M2)
The cache previously keyed solely on the resolved signing-key file path.
Two projects on the same machine whose users happened to configure the
same keyPath would share one decrypted KeyObject — switching between
them silently inherited credentials across project boundaries.
Change the API to require projectPath alongside keyPath on get/set/
invalidate. Entries are composed internally via `${project}\0${key}`;
the null byte cannot appear in POSIX or Windows path components, so the
separator is collision-free without escaping.
Update the four call sites in vc-handler.ts (resolveSigningKey,
handleCommit path, handleConfig signingKey hint, handleImportGitSigning)
to thread the projectPath already in scope.
Tests: new "project isolation" describe block exercises same keyPath
across two projects. Full cache suite migrated to the two-arg API.
vc-handler regression suite still passes (205 tests).
Note on `KeyObject.destroy()`: Bao's original review item also asked for
KeyObject.destroy() on TTL eviction. Node's crypto.KeyObject has no such
API — it is GC-managed. Dropping the Map entry is the only cleanup hook
available. That part of M2 is a no-op by API constraint, noted here for
the record.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…002 M3) No current server-side code path logs or stringifies an IVcCommitRequest — audit of src/server/infra turned up zero active leak sites. M3 is therefore a defensive primitive: anyone adding telemetry, error serialisation, or debug output touching a commit request in future should import scrubPassphrase() instead of handling the secret ad-hoc. The helper is generic over any object shape with an optional `passphrase: string` field, returns the original reference when there is nothing to redact (cheap common case), never mutates its input, and replaces a non-empty secret with literal "***". 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 |
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.