feat(connect): Phase 16 — commit-id resolution improvements (first-16-bytes-of-SHA format) - #37
Merged
Merged
Conversation
buf v1.69.0 (and later) validates every commit id it receives with
uuidutil.FromDashless, which:
1. Asserts length == 32
2. Calls uuid.Parse, which validates version and variant bits
Before this fix, ServeHTTP/ServeGraph/ServeDownload returned the raw
40-char git SHA, which buf v1.69.0 rejected with:
"Failure: expected dashless uuid to be of length 32 but was 40"
buf v1.30.1 happened to accept the raw SHA, so the existing smoke
test (TestSmokeBufModUpdate, which only runs v1.30.1) missed the bug
on the v1.69.0 path.
The fix is a deterministic SHA-256-derived UUIDv4-shaped id, computed
from the git SHA. Determinism is load-bearing: buf.lock entries stay
valid across proxy restarts, so a buf client that pinned a commit id
in a previous session will find it again on the next. SHA-256 of the
40-char input is overkill for non-security id-minting but lets us
reuse the stdlib without pulling google/uuid.
Files:
- internal/connect/commits_helpers.go: new commitUUID() helper
- internal/connect/commits.go: use commitUUID() in GetCommits,
GetGraph, Download (response + files cache key), and
registerResolved (commitMap keying + SHA alias for foreign-id path)
- internal/connect/commits_helpers_test.go: unit tests for the UUID
format (length, hex, version, variant, determinism, distinctness)
- internal/connect/uuid_format_test.go: 400-error regression tests
that lock in the wire format the buf client parses
- e2e/all_versions_test.go: matrix test that runs every cached buf
version with buf mod update and buf dep update (skipping the latter
for v1.30.x, which does not have the command). AvailableBufVersions
discovers the versions dynamically so adding a new binary extends
the matrix.
- e2e/testutil/{bufbin,config}.go: accept either EASYP_GH_TOKEN
(current) or EASYP_GITHUB_TOKEN (legacy) so tests do not silently
skip when only one of the two names is set.
Co-Authored-By: Claude <noreply@anthropic.com>
- Drop SHA-256 path; reuse first 14 SHA bytes (positions 0..5, 7, 9..15) - Stamp UUID version-4 (0x40) at byte 6 and RFC 4122 variant (0x80) at byte 8 - Change signature to (string, error) with strict 40-lowercase-hex input contract - Add preResolveForTest fixture (test-only) that pads short SHA to 40 chars - Call sites in commits.go are updated in plan 16-02 (known build break) - Drops "crypto/sha256" import, adds "errors" - Inverse: take bytes 0..5, 7, 9..15 of id to recover first 14 SHA bytes
Documents the hard cutover in 16-01/16-02: the proxy now mints the first 14 bytes of the git SHA combined with UUID version/variant bits (D-01/D-02) instead of SHA-256. Existing buf.lock entries are invalidated; operators are pointed at buf mod update / buf dep update as the recovery action (D-10/D-11/D-12). Co-Authored-By: Claude <noreply@anthropic.com>
…; add 4 new cases - Update existing TestCommitUUIDFormat/Determinism/Distinct to call new 2-return signature - Replace empty/leading-zero tests (subsumed by InvalidInput/KnownSHA) - Add TestCommitUUID_KnownSHA: table-driven, 5 SHAs lock in exact UUID bytes - Add TestCommitUUID_InvalidInput: empty, 39, 41, non-hex, mixed non-hex, 1 char - Add TestCommitUUID_InverseRecovery: 4 SHAs, recover sha[0..13] from id bytes - Add TestPreResolveForTest: empty/short/exact-40/over-40 + anti-collision assertion - Update 3 commitUUID call sites in uuid_format_test.go to 2-return signature (D-13 explicitly required both test files be updated; plan files_modified listed only commits_helpers_test.go — deviation per Rule 1)
- Documents Tasks 1 (commitUUID rewrite, preResolveForTest) and 2 (test updates) - Records deviation: uuid_format_test.go call sites updated (D-13 scope was incomplete in files_modified) - Documents known build break in commits.go (5 call sites, fix is in plan 16-02 per plan split) - Records standalone verification of helper logic (full test build cannot run until commits.go is fixed)
…ernalError helper - Add internalError helper (D-04): slog warn with error_class=internal, commit_id, upstream_error via h.hlog(r); http.Error(w, 'internal error', 500) - ServeHTTP / ServeGraph: rename local err to cidErr to avoid loop shadowing - ServeDownload: reuse existing err var for the helper error check - computeB4Digest: propagate helper error to caller; both callers (ServeHTTP, ServeGraph) now turn the resulting error into 500 via internalError instead of 502 via upstreamError - registerResolved: background path logs with h.api.log + context.Background - probeCommitID call site in ServeDownload is preserved (D-07/D-08)
… hex Production: - commits.go:582: change 'unknown commit id: must call CommitService/GetCommits first' to 'unknown commit id: re-run buf mod update / buf dep update' (D-12). Surrounding slog attrs and 400 status unchanged. - commits.go:399: update the comment that quoted the old 400 message to reference the new text verbatim so it does not drift from the code. Tests: - api_test.go + uuid_format_test.go: add a Contains assertion for the D-12 substring in the existing 400-body checks so a future regression that drops 're-run buf mod update / buf dep update' fails loudly. - (deviation, Rule 1) api_test.go: pad the 4 short hex commit literals in test mocks from 6/8 chars to 40 chars (e.g. 'deadbeef' -> 'deadbeef00000000000000000000000000000000'). The new commitUUID helper rejects non-40-char input as a contract violation (D-03), so the old short fixtures would have made 6 tests 500 with 'internal error' instead of running the assertion. Padded literals preserve the test intent (same prefix, same mockSource owner/module) while satisfying the strict input contract.
- internalError helper added in commits.go; 5 commitUUID call sites wired to the new (string, error) signature with structured 500 handling - 400 message at commits.go:582 updated to D-12 text: 'unknown commit id: re-run buf mod update / buf dep update' - probeCommitID contract preserved (D-07/D-08) - Test files: D-12 substring assertion added; short mock commits padded to 40 hex chars to satisfy the new commitUUID strict-input contract - All internal/connect tests pass; go build ./... exits 0 Deviation (Rule 1): padded 4 mockProvider mock commits from 6/8 chars to 40 hex chars so they satisfy the new commitUUID D-03 contract. The plan's files_modified list did not include fixture updates, but the strict contract would have caused 6 tests to 500 with 'internal error' instead of exercising their real assertions.
…helpers Findings #1, #2, #5 of the PR #37 review: - Remove internalError helper from commits.go; it bypassed the v1.3 ERR-05 logging contract (dropped server/protocol/request_id/status, downgraded to LevelWarn for 5xx, hardcoded 'commitUUID failure' even when the cause was GetFiles upstream). - Add errCommitUUIDContract sentinel; dispatch on errors.Is so digest failures route correctly: 500 (contract violation, logHandlerError) vs 502 (upstream, h.upstreamError). - Thread already-minted cid into computeB4Digest so the redundant commitUUID(commit) re-derivation is replaced with a contract-violation guard wrapped via fmt.Errorf('%w: %v', errCommitUUIDContract, ...). - All 5 former internalError call sites (160, 174-178, 351, 354-358, 668) now call h.logHandlerError directly; the 2 digest-error branches at 174-178 and 354-358 dispatch on errors.Is.
Finding #3 of the PR #37 review: - commitUUID now accepts exactly 40 or 64 lowercase hex characters. Bitbucket Server on SHA-256-enabled repos returns out.Commit as 64 chars (bitbucket/getrepo.go:40); the old strict-40 check 500-ed every request to such a module. - The byte-table reads sha[0:6], sha[6], sha[7:14] which are all within the 20-byte SHA-1 and 32-byte SHA-256 decoded buffers, so the function is byte-safe for both lengths. The UUID only reflects the first 14 decoded bytes regardless of input length. - Updated the doc comment to call out the 40/64 contract and the Bitbucket driver. - New TestCommitUUID_SHA256_KnownSHA covers 4 cases: all-zero, all-ones, 14-byte-prefix-matching-existing-40-char-fixture (the load-bearing regression-guard for 'function actually consumes SHA-256 bytes, not just the first 40 chars of a string'), and deadbeef. The 14-byte-prefix case is the structural invariant: same first 14 bytes in -> same UUID out. - TestCommitUUID_InvalidInput extended with 63-char and 65-char inputs (off-by-one boundaries around the new 64-char length).
Finding #4 of the PR #37 review: - The 400 wire body at commits.go:582 changed from 're-run buf mod update / buf dep update' to 're-resolve via buf mod update / buf dep update'. The original text overfit to the stale-lockfile miss class — for a foreign id (resolveForeignCommitID miss class) the prescribed command is a no-op and operators would chase a re-run that cannot help. - 'unknown commit id:' prefix preserved, so the substring assertions at api_test.go:590 and uuid_format_test.go:209 continue to pass unchanged. - Inline comment in ServeGraph (commits.go:399) updated to quote the new wire body so future readers see the live text. - Both test assertions updated: api_test.go:597 and uuid_format_test.go:215 now look for the new substring. Error message text in the t.Errorf lines updated to match.
Finding #7 of the PR #37 review: - preResolveForTest has 5 callers, all in _test.go files. The only thing keeping it out of production was its name. Moved the function (with its 5-line doc comment) from commits_helpers.go to commits_helpers_test.go where Go's build constraints exclude it. - The strings import in commits_helpers.go is retained: it remains used by strings.SplitN in parseModuleRefByID (line 288). - No caller changes — package-internal calls resolve transparently across the .go / _test.go split. - All 5 TestPreResolveForTest subtests continue to pass.
- STATE.md: phase 17 marked complete, plan 17-01 finished (4/4 tasks, 5/5 success criteria). 9/9 milestone plans now complete (100%). Velocity updated. - ROADMAP.md: Phase 17 checkbox flipped to [x]; Progress table row updated to 1/1 Complete 2026-07-07; plan bullet at line 149 marked [x].
Pre-existing MD060 warnings on the By-Phase table from the column widths going out of sync when the new row was added. Re-pad the column separators so all rows have matching widths. The Deferred Items table MD060 warning predates this commit (it was already present on the main branch) and is left as-is to keep this change narrowly scoped to the post-Phase-17 update.
…ssed All 5 ROADMAP success criteria (SC-1 through SC-5) verified via structural greps and the existing + new unit tests. Single deferred item (T-17-DEFER, 32-char hex probe/cache path) carried forward per plan question #3 — not blocking.
Contributor
Author
Phase 17 — PR #37 review findings addressedPushed 4 fix commits + 3 doc commits (7 total) to this branch on top of the existing Phase 16 work. All four in-scope review findings from Findings addressed
Commits added on top of the existing PR #37 baselineFiles modified
Verification
Diff impact on PR #37Net change vs the original PR #37 baseline (
Notes for the reviewer
Full plan, research, summary, and verification artifacts in |
Phase 17 status: shipped. The 7 new commits (4 fix + 3 doc) are pushed to feat/phase-16-commit-id-resolution alongside the existing PR #37 baseline. Posted a summary comment on PR #37 listing the findings addressed, the commits, the verification results, and the files modified. Awaiting re-review on PR #37.
Two new MD034 no-bare-urls warnings fired on the URLs I added in the previous commit (lines 31 and 94). Wrapping both in <> so the markdown linter is happy. Renders identically; the link is still clickable.
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
1 participant
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
Phase 16 of v1.3 (Diagnostic Logging). Three changes bundled into one phase:
Format change (SC-1) —
commitUUIDnow derives the 32-char dashless id from the first 14 bytes of the git SHA + UUID version/variant bits (positions 6 and 8) instead of SHA-256 of the SHA. Deterministic function of the git SHA; round-trips for both full 40-char and short (7-byte / 14-byte) inputs. Signature changed to(string, error)with strict 40-hex-char input contract.Resolution path (SC-2) —
probeCommitIDruns unconditionally on cache miss (single-source deployments keep theresolveForeignCommitIDfast path). Already on the branch; this phase preserves it.Failure mode (SC-3) — the 400 response for an unresolvable commit id names the id in both the wire body and the structured log line. Message updated to
\"unknown commit id: re-run buf mod update / buf dep update\". NewinternalErrorhelper emitserror_class=internalwarn + 500 for the(string, error)contract violations.Wire-format impact
Hard cutover. Existing
buf.lockentries referencing the old SHA-256-derived UUIDs are rejected after upgrade (look like unknown commit ids). Clients must re-runbuf mod updateorbuf dep update. SeeCHANGELOG.md.Files
internal/connect/commits_helpers.go— rewrittencommitUUID(D-01/D-02/D-03)internal/connect/commits.go— 5 call sites wired to(string, error);internalErrorhelper; 400 message (D-04/D-06/D-12)internal/connect/{commits_helpers,uuid_format,api}_test.go— updated + new testsCHANGELOG.md— format cutover entry (D-11).planning/phases/16-*— plan/summary/verification artifactsVerification
go build ./...✓go test ./...✓ (all packages)TestAllBufVersionsModUpdate,TestAllBufVersionsDepUpdate,TestSmokeBufModUpdate,TestNewProtocolBufDepUpdateall PASS against real GitHub. The new 32-char format mints correctly end-to-end.TestNewProtocolBufModUpdate,TestOldProtocolBufModUpdateTwice) failed only due tonet/http: TLS handshake timeoutreachingraw.githubusercontent.com— network issue, not code..planning/phases/16-commit-id-resolution-improvements/16-VERIFICATION.mdNotes
commits.go:158, 349, 666, 753, 901) each return 500 + structured warn oncommitUUIDerror. Per D-04 this is treated as a programming bug (callers always pass full 40-char SHAs from upstreamGetMeta), not client error.mainpost-PR-fix(connect): mint 32-char dashless UUID for commit ids (buf v1.69.0 compatibility) #36-squash; the oldfix/buf-v1.69-commit-uuid-formatorigin branch is stale (its content merged via the fix(connect): mint 32-char dashless UUID for commit ids (buf v1.69.0 compatibility) #36 squash).🤖 Generated with Claude Code