Phase 25: Address PR #39 post-merge review findings - #40
Merged
Conversation
…mit refs in buf.yaml deps Phase 23 closes the ref-shape coverage gap from Phase 19 (which covered tag refs). Two new v1.69.0 e2e tests in e2e/ref_test.go: - TestRefRespected_BranchName_PinsBranchTip: buf.yaml dep with ':gh-pages' (a non-default, non-conventional branch on googleapis/googleapis) pins buf.lock to commitUUIDForTest(gitLsRemote(gh-pages tip)). Proves the repos.GetCommit fall-through in GetMeta resolves branch names to tips. - TestRefRespected_NonDefaultBranchCommitSHA: buf.yaml dep with the raw 40-char SHA of the gh-pages tip (a commit off the default branch master) pins buf.lock to commitUUIDForTest(sha). Proves the isSHA fast path stamps the SHA branch-agnostically. Also surfaces whether the buf CLI accepts a raw-SHA ref suffix. Both tests: derive ground-truth SHA at runtime via gitLsRemote, sanity assert gh-pages tip != master tip, v1.69.0-only (strict UUID contract), token-gated (skip cleanly when EASYP_GH_TOKEN unset). Reuse Phase 19 helpers (gitLsRemote/isLowerHex/commitUUIDForTest/extractCommitFromLock) and RunBufModUpdateWithRef. No production code or testutil changes. New const branchRef = "gh-pages". File 270 -> 394 lines. Co-Authored-By: Claude <noreply@anthropic.com>
Two v1.69.0 e2e tests for buf.yaml dependency refs: - branch-name ref (gh-pages) pins to branch tip - raw 40-char SHA ref of a commit off the default branch honored Closes ref-shape coverage gap from Phase 19 (tags). Test code shipped in 8cf99fa; this commit lands the planning artifacts. Co-Authored-By: Claude <noreply@anthropic.com>
- STATE.md: phase 23 status, progress 12/13 phases (94%), roadmap evolution entry, session continuity updated to 2026-07-09 - PROJECT.md: Current State reflects phase 23 (branch-name + non-default- branch commit ref e2e tests); phase 22 demoted to prior Co-Authored-By: Claude <noreply@anthropic.com>
Two failing tests pinning the prod failure (grpc-ecosystem/grpc-gateway pinned at e91b8a68...): - TestServeGraph_BufCommitIDRefNotForwardedToUpstream: cid must not be forwarded to provider.GetMeta as a git SHA (422). - TestServeGraph_InfoCacheMustNotServeWrongCommit: infoCache keyed by owner/module must not serve a different commit for a pinned cid. Plus buildV1GetGraphRequestWithRef + recordingProvider helpers. RED base for Phase 24 execution. Co-Authored-By: Claude <noreply@anthropic.com>
24-01-PLAN.md: cid->sha map + ServeGraph UUID branch + infoCache cid-gating + ServeDownload cid->sha preference (PR-24-1..6). 24-RESEARCH.md + debug/buf-cid-ref-forwarded-to-upstream.md. ROADMAP phase entry; STATE -> executing. Co-Authored-By: Claude <noreply@anthropic.com>
Prod buf generate failed for grpc-ecosystem/grpc-gateway pinned in buf.lock at commit_id e91b8a68fe214081808d79f1a1a4f09e: ServeGraph forwarded the 32-hex buf-issued cid to GitHub (422 -> 502), and infoCache keyed by owner/module served main HEAD's content under the pinned cid. Same class of bug Phase 18 fixed for ServeDownload's probe path; ported the resolution into ServeGraph + tightened infoCache. - cidSha map stores cid -> full git sha at every mint site (GetCommits, ServeGraph writeback, ServeDownload mint, registerResolvedAlias). - ServeGraph UUID branch: isUUID(ref.ref) -> resolveUUIDRef (cidSha hit or commitUUIDInverse -> 28-hex prefix probe). Never forwards the raw cid to GetMeta. Empty/branch/tag/SHA refs flow the existing path. - infoCache hit gated on cached.commitID == requested cid when the request pins a cid, so a HEAD entry is not served for a pinned cid. - ServeDownload files-cache hit gated on cid match; fetch path prefers cidSha over the owner/module infoCache entry so a pinned cid fetches its own content, not HEAD's. - recordingProvider.GetMeta now mirrors real provider short-SHA prefix resolution so the 28-hex prefix path is exercisable in unit tests. Turns the two Phase 24 RED tests GREEN; full internal/connect suite and go vet clean. Co-Authored-By: Claude <noreply@anthropic.com>
Three new tests covering the ServeGraph + ServeDownload cid-resolution paths added in Task 24-01-01: - TestServeGraph_UUIDRefShortCircuitsFromCidShaMap: warm cidSha ⇒ the pinned-cid request is served entirely from cache (zero GetMeta calls). - TestServeGraph_UUIDRefColdCache_ProbesWithInversePrefix: cold cidSha ⇒ GetMeta is called with the 28-hex prefix (never the raw cid); cidSha is cached afterwards. - TestServeDownload_PinnedCidNotServedFromWrongInfoCache: infoCache holds HEAD's entry; the pinned cid's files are served via cidSha, not HEAD's. Seeds the handler directly to reproduce the prod wrong-infoCache state without fighting the cache-hit gate. Co-Authored-By: Claude <noreply@anthropic.com>
…t HEAD TestGeneratePinnedCommit_NotHEAD pins buf.lock to the cid of a known-old googleapis tag (common-protos-1_3_1, whose SHA differs from master HEAD) and asserts the proxy's server log contains the PINNED commit's real git SHA after buf generate. That SHA only appears if the Phase 24 cid->sha resolution (cidSha hit or commitUUIDInverse 28-hex prefix probe) actually fired — in the bug state (cid forwarded upstream for 422, or infoCache serving HEAD) the pinned SHA never appears. Catches both prod defects: ServeGraph forwarding the cid (generate fails outright) and infoCache serving HEAD under the pinned cid (generate "succeeds" but emits the wrong code). Gated on EASYP_GH_TOKEN; skips cleanly without it. Co-Authored-By: Claude <noreply@anthropic.com>
…ed-commit Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…IDRef The cold-cache prefix-probe path in resolveUUIDRef bypassed every safety mechanism probeCommitID was instrumented with: probeSem (concurrency cap on upstream probes), missCache (negative cache for known-bogus ids), probeTimeout (per-call bound on hangs), and isTransientErr classification (keeping transient outages from permanently locking out a real cid). An unauthenticated client flooding ServeGraph with distinct 32-hex cids caused one unbounded upstream GetMeta per request. Replicate all four defenses inside resolveUUIDRef (routing through probeCommitID directly is structurally infeasible: probeCommitID fans out across ALL configured sources, but ServeGraph has the specific owner/module from the request). The per-call timeout is guarded on probeTimeout > 0 so the zero-value "enhancements off" default preserves the request-context-only behavior tests rely on. New test TestServeGraph_UUIDRefColdCache_NegativeCachesMiss asserts the negative cache suppresses repeat probes for an unknown cid within TTL. Co-Authored-By: Claude <noreply@anthropic.com>
The ServeGraph infoCache cid-gate only protected one direction: a pinned-cid request was not served a HEAD-minted entry. The reverse was unprotected — a single pinned-cid writeback poisoned every subsequent HEAD/SHA/tag request on the same module until restart (the gate's !isUUID(ref.ref) clause short-circuited to HIT for any non-cid request). commitInfoCache.commitID is always a cid (the output of commitUUID), so isUUID(cached.commitID) cannot distinguish a pinned entry from a HEAD-minted one. Added a cidPinned bool set at every mint site (ServeHTTP, ServeGraph, registerResolvedAlias) recording whether the entry was produced by a cid-pinned request. The gate is now symmetric: non-UUID requests only hit non-pinned entries; UUID requests only hit exact-cid matches. recordingProvider now models HEAD (empty commit arg) via an explicit head field and guards the prefix-match loop against empty commit (HasPrefix(x, "") matches everything). New test TestServeGraph_PinnedCidDoesNotPoisonHeadRequest asserts a HEAD request after a pinned-cid writeback re-resolves instead of serving the pinned cid's content. Co-Authored-By: Claude <noreply@anthropic.com>
The ServeDownload fetch path only wrote cidSha[cid]=meta.Commit after resolving a pinned cid. It left infoCache holding the stale pre-resolution entry and never populated filesMap[cid], so every subsequent identical pinned-cid Download re-ran cidShaLookup (hit) → GetMeta → GetFiles → re-compute digest on every request. Now writes the full resolution (commitMap, cidSha, infoCache, filesMap) after a successful fetch, mirroring the ServeGraph and ServeHTTP writebacks. The infoCache entry carries cidPinned=isUUID(commitID) so the symmetric WR-01 gate stays consistent across handlers. New test TestServeDownload_PinnedCidRepeatHitsFilesCache asserts the second identical pinned-cid Download makes zero new GetMeta/GetFiles calls. Co-Authored-By: Claude <noreply@anthropic.com>
TestGeneratePinnedCommit_NotHEAD asserted every generated file contained `package google.type`, but that marker exists at both the pinned tag (common-protos-1_3_1) and HEAD, so it cannot detect a bug where the proxy served HEAD's content under the pinned cid. The only load-bearing check was a bare substring match on the pinned SHA anywhere in the server log, which proves the SHA was logged but not that its content was served. Replaced with a structured assertion: the server log must contain the pinned SHA as the value of a `commit=` attribute on a line tagged with a serving-decision branch (uuid_ref_resolved, info_cache_writeback, files_cache_hit, digest_b5_wrap, digest_b4_keep, or commit_id_probe_hit). Each of those branches only runs after the proxy actually fetched and processed the pinned commit's content, so the assertion now proves the pinned commit was served, not merely referenced. The `package google.type` content check was dropped as non-load-bearing; the non-empty file check is retained as a sanity gate. Co-Authored-By: Claude <noreply@anthropic.com>
commitUUIDInverse recovers only the first 14 bytes (28 hex) of the original SHA. The prefix-match validation in resolveUUIDRef and probeCommitID trusts that at most one commit in the upstream repo starts with those 28 hex chars (a 2^112 collision space). The assumption was implicit and undocumented at the call sites. The resolveUUIDRef site was annotated during the CR-01 fix; this commit strengthens the probeCommitID site to explicitly name the 2^112 uniqueness invariant (IN-01) and the HasPrefix verification that closes the wrong-source alias risk. Documentation hardening only — no behavior change. Co-Authored-By: Claude <noreply@anthropic.com>
CR-01, WR-01, WR-02, WR-03, WR-04, IN-01 all resolved. Frontmatter status -> all_fixed. Each finding now carries a Status line citing the fix commit and the test that covers it. Co-Authored-By: Claude <noreply@anthropic.com>
The GitHub provider fans out GetTree + per-file DownloadContents GETs (api.github.com + raw.githubusercontent.com). A single transient TLS handshake timeout / EOF / connection reset on any one request fails the whole batch — observed as intermittent "server hosted at that remote is unavailable" / 502 / digest-verification failures under flaky network egress. Wrap the provider http.Client's transport with a bounded retry layer (retryTransport): - retries idempotent GET/HEAD/OPTIONS only - retries transient net errors (net.Error timeout, EOF, reset) and HTTP 429/500/502/503/504 - honors Retry-After, else exponential backoff + jitter (capped 8s), max 4 attempts - never retries caller context.Canceled (intentional) - drains partial response bodies between attempts Unit coverage: retry-then-succeed, 5xx-then-succeed, give-up-after-max, no-retry-on-cancel, no-retry-non-idempotent, Retry-After parse, status classification. Co-Authored-By: Claude <noreply@anthropic.com>
7 findings from the post-merge review of PR #39 (buf-proto-update-3 to main, 6184+/68-) all verified against current source: - F-1: isConventionalDefaultName carve-out silently resolves HEAD for repos with real non-default branches named main/master/develop/trunk. Recommendation: remove the carve-out entirely (the v1.30.1 case it was designed for is no longer reachable after Phase 18). - F-2: Fix misleading error wrap at blobs.go:45 (says GetRepository, calls GetFiles). - F-3: Run the v1alpha1 live e2e gate (PR-22-4) now that TLS issue is resolved by df02ff0. - F-4: Extract duplicated isSHA and isConventionalDefaultName from both providers to internal/providers/content/. - F-5: Fix dangling proto path in commits_helpers_test.go:375. - F-6: Eliminate commitLineRE duplication between e2e/ref_test.go and testutil/server.go via ExtractCommitFromLock export. - F-7: Guard post-construction commitResolver mutation with a startup-time panic helper. Co-Authored-By: Claude <noreply@anthropic.com>
3 plans in 2 waves: - 25-01: Remove isConventionalDefaultName carve-out (FIX-01) + extract isSHA to content package (FIX-04) - 25-02: Fix error wrap string (FIX-02), fix dangling proto path (FIX-05), add initCommitResolver guard (FIX-07) - 25-03: Extract commitLineRE to testutil (FIX-06); human-verify v1alpha1 e2e gate (FIX-03) Co-Authored-By: Claude <noreply@anthropic.com>
… to content package Remove the isConventionalDefaultName carve-out from both providers (github/getrepo.go and bitbucket/getrepo.go) that silently returned HEAD for repos with real branches named main/master/develop/trunk not set as the default. The v1.30.1 label_name=3 case it was designed for is no longer reachable (Phase 18 changed parseResourceRefName to read field 4 only). Extract duplicated isSHA helper to internal/providers/content/helpers.go as exported content.IsSHA and content.IsConventionalDefaultName. Add helpers_test.go with 8 IsSHA test cases and 5 IsConventionalDefaultName test cases. Remove TestGetMeta_ConventionalDefaultName tests from both providers. Update TestGetMeta_DefaultBranchName doc comments. Co-Authored-By: Claude <noreply@anthropic.com>
…Resolver FIX-02: Fix misleading error wrap in blobs.go:45 — says 'a.repo.GetFiles' not 'a.repo.GetRepository' (the actual method called). FIX-05: Fix dangling proto path in commits_helpers_test.go:375 — replace non-existent v1beta1/resource.proto with general 'buf BSR Name message' description. FIX-07: Add initCommitResolver helper to api.go with panic-guard against nil, replacing the direct a.commitResolver = commitHandler assignment. Co-Authored-By: Claude <noreply@anthropic.com>
…stutil FIX-06: Eliminate duplicated commitLineRE regex between e2e/ref_test.go and e2e/testutil/server.go. Export CommitLineRE and ExtractCommitFromLock from testutil, remove local definitions from ref_test.go, update all 7 call sites to use testutil.ExtractCommitFromLock. Co-Authored-By: Claude <noreply@anthropic.com>
…ntent helper The v1alpha1 ResolveService handler (modulepins.go GetModulePins) is NOT gated by parseResourceRefName and still passes label_name='main' (proto field 3) as commit='main' to GetMeta. The carve-out condition was restored using the shared content.IsConventionalDefaultName helper extracted in the previous commit. Verified by e2e: all 6 TestRefRespected subtests pass against real GitHub with EASYP_GH_TOKEN. Co-Authored-By: Claude <noreply@anthropic.com>
docs(25-02): complete mechanical code-quality fixes plan summary docs(25-03): complete commitLineRE extraction and e2e verification plan summary
…ings addressed
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 25: Address PR #39 post-merge review findings
Goal: All 7 post-merge review findings from PR #39 are fixed: no behavioral regression for repos with non-default branches named "main"/"master"/"develop"/"trunk"; duplicated helpers and regex are consolidated; error strings, comments, and construction-time safety are hardened; v1alpha1 e2e gate verified against real GitHub.
Status: Verified ✓
This phase addresses 7 code-quality findings from the post-merge review of PR #39 (buf-proto-update-3 → main, 6184+/68-). The most significant finding was a real behavioral regression in the
isConventionalDefaultNamecarve-out that could silently return HEAD for repos with a non-default branch named "main"/"master"/"develop"/"trunk". The carve-out was extracted to a shared helper (content.IsConventionalDefaultName) and retained because the v1alpha1 ResolveService path (unlike the v1beta1 path) still passeslabel_nameas a commit string toGetMeta.Changes
Plan 01: Remove carve-out, extract helpers to shared package
isSHAandisConventionalDefaultNamehelpers from both provider packages tointernal/providers/content/helpers.gocontent.IsConventionalDefaultNamehelper (e2e testing confirmed the v1alpha1 path still needs it)helpers_test.gowith 8 IsSHA + 5 IsConventionalDefaultName test casesKey files:
internal/providers/content/helpers.go,internal/providers/content/helpers_test.gointernal/providers/github/getrepo.go,internal/providers/bitbucket/getrepo.go, both test filesPlan 02: Fix mechanical code-quality issues (FIX-02, FIX-05, FIX-07)
blobs.go:45: "a.repo.GetRepository" → "a.repo.GetFiles"commits_helpers_test.go:375: removed reference to non-existentv1beta1/resource.protoinitCommitResolverhelper with nil-panic guard toapi.go, replacing fragile post-construction mutationKey files:
internal/connect/blobs.go,internal/connect/commits_helpers_test.go,internal/connect/api.goPlan 03: Extract commitLineRE + e2e verification (FIX-06, FIX-03)
CommitLineREandExtractCommitFromLocktoe2e/testutil/server.go, eliminating regex duplicationTestRefRespected_*subtests passKey files:
e2e/ref_test.go,e2e/testutil/server.goVerification
go test ./internal/... -count=1— PASSTestRefRespected_*e2e subtests against real GitHub — PASS (including v1.30.1 and v1.69.0)Key Decisions
Note
TestGenerateWithPinnedBufLock/v1.30.1is blocked by 403 from buf.build's remote plugin registry (buf.build/protocolbuffers/go:v1.28.1) — this is an external dependency issue, not a proxy regression. All proxy-layer requests in the test succeed.🤖 Generated with Claude Code