fix: address post-merge review findings from PR #63 - #64
Merged
Conversation
- Fix AIA progressTotal double-counting persistently-unresolved certs by tracking unique certs in a totalSeen set instead of recomputing from processed + queue (which overlap) - Restructure TestCheckKeyStrength to generate keys inside subtests with proper t.Fatal error handling (ERR-5) - Consolidate TestCheckTrustChain_NilRoots and _SelfSigned into a single table-driven TestCheckTrustChain (T-12) - Add missing changelog entries for PR #63 production changes (CL-1) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Follow-up fixes to address review findings from PR #63, improving AIA progress accounting, strengthening validation unit tests, and updating the changelog to reflect shipped behavior.
Changes:
- Update AIA resolution progress tracking to avoid double-counting certs that persist across depth rounds.
- Refactor/merge validation tests to generate keys within subtests and use table-driven structure.
- Add missing
CHANGELOG.mdentries referencing PR #63 production changes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/certstore/validate_test.go | Refactors validation tests for safer key generation and consolidates trust chain tests into a table-driven form. |
| internal/certstore/aia.go | Adjusts progress total calculation to track unique certs seen across rounds. |
| CHANGELOG.md | Adds missing Unreleased entries and a PR link reference for #63. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Use certID (serial+AKI) instead of SubjectKeyId for totalSeen and processed maps — SKI is not unique across cross-signed certs - Add TestResolveAIA_ProgressNoDuplicateCounting asserting completed never exceeds total when fetches fail across depth rounds - Fix changelog: progressTotal fix refs #64 not #63, add [#64] link Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The early-exit branch (when all AIA URLs are already seen) still used rec.SKI while the normal path used certID(rec.Cert). This mismatch could reintroduce the completed > total overflow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
The previous test used an always-failing fetcher, so the loop exited after one round (fetched == 0). Restructure with two leaves: one whose AIA fetch succeeds (keeping the loop alive) and one that always fails (re-entering the queue in round 2). This actually exercises the double-counting fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
The previous assertions (completed <= total, final == 100%) were tautological — they pass even with the old buggy formula. Assert that total stays at exactly 2 across all ticks, which fails with the old len(processed)+len(queue) formula that inflated to 3. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add sections on pre-commit hook recovery, resolving review threads via GraphQL, merge checklist, and changelog ref attribution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code review1 issue found. CHANGELOG.md — CL-2 violation:
|
This was referenced Feb 24, 2026
This was referenced Mar 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
progressTotaldouble-counting persistently-unresolved certs — uses atotalSeenset to track unique certs that ever entered the queue, preventing overlap betweenprocessedandqueuesetsTestCheckKeyStrengthto generate keys inside subtests with propert.Fatalerror handling (ERR-5)TestCheckTrustChain_NilRootsand_SelfSignedinto a single table-drivenTestCheckTrustChain(T-12)Addresses claude-review findings on #63.
Test plan
go test -race ./...passesGOOS=js GOARCH=wasm go build ./cmd/wasm/)🤖 Generated with Claude Code