fix(credential): return nonrev Commit error instead of panicking (#59) - #62
Conversation
DisclosureProofBuilder.Commit returned errors for every other recoverable failure but panicked on a non-revocation builder Commit error, crashing the process on an operational error the API already models as a return value. Propagate the error like its siblings in the same function. Add a regression test asserting Commit surfaces the error without panicking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Rules Dobby 2 — final review. This is a sign-off; treat as approved. It is posted as a COMMENT rather than an APPROVE only because GitHub blocks approving your own PR (this PR is bot-authored).
The fix is correct and well-scoped: the lone panic(err) on the non-revocation commit path is replaced with return nil, err, matching every sibling error return in DisclosureProofBuilder.Commit. Both callers (builder.go:218, keyshare.go:79) already check the returned error, so nothing relied on the panic. The new regression test genuinely exercises the bug (verified: it panics on the pre-fix code, passes after), and go test ./... is green across all packages.
Per-rule compliance check passed (conventional-commit title, tests-required-on-fixes, concise PR body, promised-vs-delivered all clean). One cosmetic test nit inline — non-blocking, so this signs off rather than holding the PR.
Note: gabi master requires 2 approving reviews from write-access maintainers, so this still needs human approval before it can merge.
Resolves the add/add conflict on gabikeys/keys_test.go: master added permission-regression tests for #56 in package gabikeys_test, this branch added coverage tests in package gabikeys. Kept both test sets in a single file, converted this branch's tests to the external gabikeys_test package that master established. Also drops TestNonrevCacheSharedCredentialConcurrent: master's #62 fix brought TestSharedCredentialConcurrentNonrevDisclosure, a strict superset (same shared-Credential fan-out, but also verifies every proof), so the two were redundant guards for #63.
Summary
Fixes #59.
DisclosureProofBuilder.Commit(credential.go:285) returns anerrorfor every recoverable failure — thecommon.ModPowcalls and range-proof commitments allreturn nil, err. The one exception was the non-revocation commitment, which panicked:A recoverable error from the non-revocation builder's
Commit()therefore crashed the whole process instead of propagating to the caller. This changes it toreturn nil, err, matching its siblings in the same function.Caller safety
Both callers of
DisclosureProofBuilder.Commit(builder.go:218,keyshare.go:79) already check the returnederror, so they handle this path correctly — none relied on the panic.Test
Added
TestDisclosureProofBuilderCommitNonrevError: it builds a real disclosure proof builder with revocation, forces the non-revocation builder'sCommit()to fail (by breaking the non-revocation relation), and assertsCommitreturns an error without panicking. Confirmed the test fails (panics) on the old code and passes with the fix.go test ./...passes across all packages.🤖 Generated with Claude Code