diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index ee64486..ef12175 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -169,6 +169,26 @@ Plans: | 15. Operational Logging | v1.3 | 0/0 | Not started | - | | 16. Commit ID Resolution Improvements | v1.3 | 3/3 | Complete | 2026-07-06 | | 17. Fix PR #37 review findings | v1.3 | 1/1 | Complete | 2026-07-07 | +| 18. Respect buf.yaml dependency refs (not always HEAD); fix related bug; remove prewarm logic | v1.3 | 1/1 | Complete | 2026-07-07 | + +### Phase 18: Respect buf.yaml dependency refs (not always HEAD); fix related bug; remove prewarm logic now that buf id → git id is derivable + +**Goal:** Three corrections to dependency resolution, shipped as one atomic pass: (1) honor the `ref` field the buf CLI sends in the `Name` message so a dep like `cyp/cyp-net-listeners:main/v2` returns the SHA at `main/v2`, not the HEAD; (2) replace the `commit != "" && commit != "main"` short-circuit in both bitbucket and github providers with an `isSHA(commit)` gate so non-SHA inputs route through the providers' commit-fetch APIs to resolve; (3) drop the `prewarmHeads` startup fan-out and replace it with a UUID→SHA-prefix inverse function (`commitUUIDInverse`) that lets `probeCommitID` recover the right module on a Download cache-miss without the prewarm. +**Depends on:** Phase 17 +**Requirements**: SC-1, SC-2, SC-3, SC-4, SC-5 +**Success Criteria** (what must be TRUE): + + 1. A buf client pinning `buf-proxy.yadro.dev/cyp/cyp-net-listeners:main/v2` in `buf.yaml` gets a UUID in `buf.lock` derived from the SHA at `main/v2` (not from HEAD) — verified end-to-end by the providers' `TestGetMeta_ResolvesRef` and the connect-package `TestParseResourceRefName_ReadsRef` + 2. The `commit != "" && commit != "main"` short-circuit is gone from both `bitbucket/getrepo.go` and `github/getrepo.go`; both providers now gate on `isSHA(commit)` — verified by structural grep + `go test ./internal/providers/{bitbucket,github}/ -count=1` + 3. `prewarmHeads` and `registerResolved` are deleted from `commits.go`; the goroutine launch in `api.go:111-113` is gone; the `PrewarmConfig` block in `config.go:96-101` is gone; `prewarmEnabled`/`prewarmTimeout`/`prewarmOnce` fields on `commitServiceHandler` and `PrewarmEnabled`/`PrewarmTimeout` on `CommitResolution` are gone — verified by 5 structural grep checks returning 0 hits + 4. `commitUUIDInverse(uuid string) (string, error)` exists in `commits_helpers.go`, returns the 28-char SHA prefix, errors on non-32-char or non-hex input; `TestCommitUUIDInverse` covers 8 cases (round-trip with a known 40-char SHA, all-zero, all-ones, 64-char SHA round-trip, empty, 31-char, 33-char, 32-char non-hex) + 5. `probeCommitID` derives the 28-char SHA prefix from a 32-char UUID input via `commitUUIDInverse`, probes each source with the prefix, and only registers a hit when the returned `meta.Commit` actually starts with the recovered prefix (collision-safe); `TestServeDownload_AfterRestart_ProbeResolvesUUID` and `TestServeDownload_AfterRestart_ProbeMissesOnUnknownUUID` exercise the post-restart path + +**Plans:** 1 plan +Plans: +Plans: + +- [x] [18-01](./phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-01-PLAN.md) — Honor `Name.ref` end-to-end + isSHA-gated provider ref-resolution + prewarm removal with `commitUUIDInverse`-based probe --- diff --git a/.planning/STATE.md b/.planning/STATE.md index d257a8c..f2a9cc2 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -2,14 +2,14 @@ gsd_state_version: 1.0 milestone: v1.3 milestone_name: Diagnostic Logging — In Progress -status: shipped -last_updated: "2026-07-07T10:32:00.000Z" -last_activity: 2026-07-07 -- Phase 17 shipped (review fixes posted as PR #37 comment) +status: executing +last_updated: "2026-07-07T14:17:36.053Z" +last_activity: 2026-07-07 -- Phase 18 planning complete progress: - total_phases: 7 - completed_phases: 7 - total_plans: 9 - completed_plans: 9 + total_phases: 8 + completed_phases: 8 + total_plans: 10 + completed_plans: 10 percent: 100 --- @@ -25,10 +25,10 @@ See: .planning/PROJECT.md (updated 2026-05-10) ## Current Position -Phase: 17 -Plan: 17-01 shipped -Status: Phase 17 shipped — review fixes posted as comment on PR #37 -Last activity: 2026-07-07 -- Phase 17 shipped (PR #37 comment ) +Phase: 18 +Plan: 18-01 shipped +Status: Phase 18 plan 01 complete +Last activity: 2026-07-07 -- Phase 18 plan 01 execution complete (refs, isSHA, prewarm removal, commitUUIDInverse) Progress: [####################] 100% @@ -36,9 +36,9 @@ Progress: [####################] 100% **Velocity:** -- Total plans completed: 4 (this milestone) -- Average duration: ~8 min -- Total execution time: ~32 min +- Total plans completed: 5 (this milestone) +- Average duration: ~10 min +- Total execution time: ~50 min **By Phase:** @@ -46,6 +46,7 @@ Progress: [####################] 100% |-------|-------|-------|----------| | 16 | 3 | - | - | | 17 | 1 | - | 8 min | +| 18 | 1 | - | 18 min | **Recent Trend:** @@ -76,6 +77,8 @@ None yet. - Phase 16 added: предлагаю изменения — use first 16 bytes of git commit id, probe all repos on miss, fix unclear not-found error message - Phase 17 added: Fix PR #37 review findings — address pre-merge issues from Phase 16 PR: re-route digest errors through `logHandlerError`/`upstreamError` (not `internalError`), remove `internalError` helper that bypassed `ERR-05`, accept SHA-256 Bitbucket commits (regression at commits_helpers.go:39), move `preResolveForTest` to a `_test.go` file +- Phase 18 added: respect buf.yaml dependency refs (not always HEAD); fix related bug; remove prewarm logic now that buf id → git id is derivable +- Phase 18 planned: 1 plan (18-01-PLAN.md) with 3 tasks — (1) honor `Name.ref` end-to-end + add `isSHA`/`isUUID`/`commitUUIDInverse` helpers + provider ref-resolution; (2) add `commitUUIDInverse` + table-driven tests; (3) delete `prewarmHeads`/`registerResolved`/`PrewarmConfig` and rewrite `probeCommitID` to use the inverse for 32-char UUID inputs. See `.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-{RESEARCH,01-PLAN,VALIDATION}.md` ## Deferred Items @@ -89,6 +92,6 @@ Items acknowledged and carried forward from previous milestone close: ## Session Continuity -Last session: 2026-07-07T10:32:00.000Z -Stopped at: Phase 17 shipped (1/1 plan, 4 tasks, 5 SCs satisfied, 13/13 verified, comment posted on PR #37) -Resume file: +Last session: 2026-07-07T14:17:36Z +Stopped at: Phase 18 shipped (1/1 plan, 3 tasks, 5 SCs satisfied, executor self-check PASSED, 18 min) +Resume file: `.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-01-SUMMARY.md` diff --git a/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-01-PLAN.md b/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-01-PLAN.md new file mode 100644 index 0000000..0354261 --- /dev/null +++ b/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-01-PLAN.md @@ -0,0 +1,904 @@ +--- +phase: 18-respect-buf-yaml-dependency-refs-not-always-head-fix-related +plan: 01 +type: execute +wave: 2 +depends_on: [] +files_modified: + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers_test.go + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/api.go + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/api_test.go + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/getrepo.go + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/getrepo_test.go + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/getrepo.go + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/getrepo_test.go + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/mockrepos_test.go + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/internal/config/config.go + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/main.go +autonomous: true +requirements_addressed: + - SC-1 (refs respected: GetCommits/GetGraph return UUID from ref, not HEAD) + - SC-2 (related bug fixed: provider short-circuit replaced with isSHA gate) + - SC-3 (prewarm removed: prewarmHeads, registerResolved, related fields, config block) + - SC-4 (inverse helper: commitUUIDInverse + round-trip tests) + - SC-5 (no regression: Phase 16/17 tests still pass) + +must_haves: + truths: + - "`parseResourceRefName` reads the `ref` field (proto field 3) from a `Name` message; `moduleRef` carries the ref; `ServeHTTP` and `ServeGraph` pass `ref.ref` to `GetMeta` instead of `\"\"` when the request carried a ref" + - "A bitbucket and github provider `GetMeta` call with `commit=\"main/v2\"` (a non-SHA, non-empty ref) resolves to the SHA at that ref via the provider's commit-fetch API, and returns that SHA in `meta.Commit`" + - "The `commit != \"\" && commit != \"main\"` short-circuit in both providers is replaced with `isSHA(commit) && (len(commit) == 40 || len(commit) == 64)`. A 40/64-char lowercase hex input still goes through the existing fast path" + - "`commitUUIDInverse(\"81353411f7b010d5b9ebeb1899066aac18a36701\")` (a known 40-char SHA's UUID) returns `\"81353411f7b010d5b9ebeb1899066aac\"` (28 hex chars) and errors on inputs that are not 32 lowercase hex characters" + - "`prewarmHeads` and `registerResolved` no longer exist in `internal/connect/commits.go`; the goroutine launch in `api.go` is gone; the `PrewarmConfig` block in `cmd/easyp/internal/config/config.go` is gone; `prewarmEnabled`/`prewarmTimeout`/`prewarmOnce` fields on `commitServiceHandler` are gone; `PrewarmEnabled`/`PrewarmTimeout` fields on `CommitResolution` are gone" + - "`probeCommitID` derives the 28-char SHA prefix from a 32-char UUID input via `commitUUIDInverse`, probes each source with the prefix, and only registers a hit when the returned `meta.Commit` actually starts with the recovered prefix (collision-safe)" + - "`go build ./...`, `go vet ./...`, and `go test ./internal/connect/ -count=1` and `go test ./internal/providers/... -count=1` all pass with zero output other than test results" + artifacts: + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go" + provides: "`moduleRef` gains a `ref` field; `parseResourceRefName` reads proto field 3; `parseResourceRefs`/`parseGetGraphResourceRefs`/`parseGetGraphResourceRefsV1` populate it; new `commitUUIDInverse` helper with doc comment; new `isSHA` helper; new `isUUID` helper" + min_lines: 410 + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers_test.go" + provides: "New `TestCommitUUIDInverse` table-driven test; new `TestIsSHA` / `TestIsUUID` boundary tests" + min_lines: 380 + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go" + provides: "`ServeHTTP` and `ServeGraph` pass the parsed ref to `GetMeta`; `prewarmHeads` and `registerResolved` deleted; `prewarmEnabled`/`prewarmTimeout`/`prewarmOnce` fields deleted; `probeCommitID` rewritten to call `commitUUIDInverse` for 32-char inputs and validate the returned SHA starts with the prefix" + min_lines: 1180 + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/api.go" + provides: "Goroutine launch of `prewarmHeads` removed; `CommitResolution` struct no longer carries `PrewarmEnabled`/`PrewarmTimeout`; `NewWithConfig` signature unchanged" + min_lines: 130 + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/api_test.go" + provides: "New `TestServeDownload_AfterRestart_ProbeResolvesUUID` integration test (startup with empty commitMap, Download with a UUID, assert probe recovers the right module via the new inverse)" + min_lines: 720 + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/getrepo.go" + provides: "`getMeta` uses `isSHA(commit)` gate; if not SHA and not empty, calls new `getCommit(ctx, commit)` which fetches `/commits/{commitId}` and decodes the response into a struct with the resolved SHA" + min_lines: 90 + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/getrepo_test.go" + provides: "New `TestGetMeta_ResolvesRef` with a mock HTTP server that returns a known SHA for a ref input; existing `TestGetMeta_Empty` and `TestGetMeta_RawSHA` continue to pass" + min_lines: 60 + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/getrepo.go" + provides: "`GetMeta` uses `isSHA(commit)` gate; if not SHA and not empty, calls `c.repos.GetCommit(ctx, owner, repo, commit, ...)` and stamps `out.Commit = rc.GetSHA()`" + min_lines: 120 + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/getrepo_test.go" + provides: "New `TestGetMeta_ResolvesRef` with a mock `Repositories` that returns a `*github.RepositoryCommit` whose `GetSHA()` is the expected SHA" + min_lines: 60 + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/mockrepos_test.go" + provides: "New `getCommit` method on the mock `Repositories` returning the configured commit (added to the existing mock impl)" + min_lines: 130 + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/internal/config/config.go" + provides: "`PrewarmConfig` struct deleted; `Connect.Prewarm` field deleted; `WithDefaults` no longer touches prewarm" + min_lines: 110 + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/main.go" + provides: "The `Connect` block in `handler := func() ...` (main.go:57-65) no longer references `cc.Prewarm.*`; the `connect.CommitResolution{}` literal no longer carries `PrewarmEnabled`/`PrewarmTimeout`" + min_lines: 340 + key_links: + - from: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go:108" + to: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go:11" + via: "`parseResourceRefName` populates the new `ref` field on `moduleRef`; the upstream callers (`parseResourceRefs`, the two `parseGetGraph*` variants) carry the field through unchanged" + pattern: "num == 3 && typ == protowire\\.BytesType" + - from: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go:145" + to: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go:60" + via: "`ServeHTTP` and `ServeGraph` call `repo.GetMeta(ctx, ref.owner, ref.module, ref.ref)`; the bitbucket/github providers gate on `isSHA(commit)` to decide between fast-path and ref-resolution" + pattern: "isSHA\\(commit\\)" + - from: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go:1056" + to: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go:55" + via: "`probeCommitID` calls `commitUUIDInverse(commitID)` when the input is 32 hex chars; the recovered prefix is used as the probe arg" + pattern: "commitUUIDInverse" + - from: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go:1101" + to: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go:40" + via: "The probe validates that the source's `meta.Commit` starts with the prefix recovered by `commitUUIDInverse` before treating the probe as a hit" + pattern: "strings\\.HasPrefix\\(meta\\.Commit, shaPrefix\\)" + - from: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/getrepo.go:18" + to: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go:60" + via: "Bitbucket provider's `getMeta` uses `isSHA(commit)` from the connect package" + pattern: "isSHA" + - from: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/getrepo.go:21" + to: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go:60" + via: "GitHub provider's `GetMeta` uses `isSHA(commit)` from the connect package" + pattern: "isSHA" + - from: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/main.go:58" + to: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/api.go:66" + via: "main.go no longer passes `PrewarmEnabled`/`PrewarmTimeout` to `connect.NewWithConfig`; `CommitResolution` no longer has those fields" + pattern: "connect\\.CommitResolution\\{" +--- + + +Three corrections to dependency resolution, shipped as one atomic pass: (1) honor the `ref` field the buf CLI sends in the `Name` message so a dep like `cyp/cyp-net-listeners:main/v2` returns the SHA at `main/v2`, not the HEAD; (2) replace the `commit != "" && commit != "main"` short-circuit in both providers with an `isSHA` gate so non-SHA inputs route through the providers' commit-fetch APIs to resolve; (3) drop the `prewarmHeads` startup fan-out and replace it with a UUID→SHA-prefix inverse function (`commitUUIDInverse`) that lets `probeCommitID` recover the right module on a Download cache-miss without the prewarm. + +Purpose: today a buf client pinning `cyp-net-listeners:main/v2` in `buf.yaml` gets a HEAD UUID pinned in `buf.lock` (or 500s if the provider treats the ref as a SHA), and a process restart loses the in-memory `commitMap` that maps pinned UUIDs back to modules until the client re-fetches via `GetCommits`. Phase 18 fixes the first by respecting refs end-to-end and fixes the second by deriving the SHA prefix from the buf-issued UUID at probe time. + +Output: a ref-aware `parseResourceRefName`/`moduleRef`/`ServeHTTP`/`ServeGraph` pipeline, two provider `getMeta` implementations that route non-SHA inputs through their commit-fetch APIs, a new `commitUUIDInverse` helper with round-trip tests, a rewritten `probeCommitID` that uses the inverse for 32-char inputs, and the complete removal of `prewarmHeads` / `registerResolved` / `PrewarmConfig` / related fields. All existing tests (including the SHA-256 path from Phase 17 and the probe path from Phase 16) continue to pass. + + + +@/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/.claude/get-shit-done/workflows/execute-plan.md +@/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/.claude/get-shit-done/templates/summary.md + + + +@/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-RESEARCH.md +@/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/.planning/ROADMAP.md +@/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/.planning/STATE.md +@/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/.planning/PROJECT.md + +# Per-task source files (read fresh inside each task's ) +- /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go +- /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go +- /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers_test.go +- /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/api.go +- /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/api_test.go +- /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/getrepo.go +- /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/client.go +- /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/getrepo.go +- /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/client.go +- /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/repos.go +- /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/internal/config/config.go +- /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/main.go + + + +1. **Ref vs SHA detection in providers — strict or lenient?** Resolved: strict. `isSHA(commit) := len(commit) == 40 || len(commit) == 64` AND lowercase hex via `hex.DecodeString`. Anything else is a ref. The 32-char case is a buf-issued UUID — never reaches the providers, the proxy short-circuits in `probeCommitID` (task 3) to derive the SHA prefix via `commitUUIDInverse` and probe with the prefix, which is a 28-char lowercase hex (never matches the 40/64-char gate). + +2. **The 32-char UUID case in providers — accept or reject?** Resolved: reject. With task 1 in place, a 32-char input that is NOT empty and NOT a 40/64-char SHA will be routed to the new ref-resolution API. Bitbucket's `/commits/{commitId}` will return 404 for a 32-char hex (not a real SHA, not a ref). GitHub's `repos.GetCommit` will return 404 for the same reason. The provider returns the error, `probeCommitID` sees it, and the request 400s — same as a foreign id today. Task 3's UUID-prefix path is the only legitimate 32-char input, and it never reaches the providers (it short-circuits in `probeCommitID`). + +3. **Where to put the new helpers — commits_helpers.go or a new file?** Resolved: `commits_helpers.go` for `isSHA` (mirrors `commitUUID`'s style; small pure function used by both providers and the connect package). `commitUUIDInverse` also goes in `commits_helpers.go` — it is the inverse of `commitUUID`, belongs next to it. `isUUID` is unused by the providers and is a probe-only helper; put it next to `commitUUIDInverse` in the same file. + +4. **`probeCommitID` rewrite — fail-open or fail-closed on the prefix check?** Resolved: fail-closed. If the source returns a SHA that does not start with the recovered prefix, the probe treats it as a miss and does NOT register a hit. A wrong-module hit would silently alias a real commit id to a wrong module and serve wrong content. Failing closed and returning a 400 (the existing `badRequest` path on `ref == nil` at [commits.go:601-604](internal/connect/commits.go#L601-L604)) is the right tradeoff: the client re-runs `buf mod update` and recovers. + +5. **Test isolation for the new `TestServeDownload_AfterRestart_ProbeResolvesUUID`?** Resolved: build the test as a fresh handler with an empty `commitMap` and a single configured source. Issue a `Download` request with a UUID. Assert the response is 200 and the served files match the source. The pre-restart `GetCommits` step is NOT simulated — the test exercises the post-restart path where the only way to recover the module is the probe. + +6. **Should `gitSHA` be the test fixture or do we use a simpler payload?** Resolved: use a known 40-char SHA-1 fixture (`"81353411f7b010d5b9ebeb1899066aac18a36701"`) and its UUID (`commitUUID(...)` at test setup). The SHA, the UUID, the recovered prefix, and the buf.v1.69.0 wire format are all derived from one source-of-truth fixture. Same shape as Phase 17's `TestServeHTTP_GetCommits_ReturnsDashlessUUID`. + + + + + + + + + + Task 1: Honor `Name.ref` in `parseResourceRefName`, plumb through `ServeHTTP`/`ServeGraph`, add `isSHA`/`isUUID` helpers, route non-SHA provider inputs through commit-fetch APIs + + + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers_test.go + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/getrepo.go + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/getrepo_test.go + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/getrepo.go + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/getrepo_test.go + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/mockrepos_test.go + + + + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go (lines 11-14 moduleRef struct; 62-136 parseResourceRefs/parseResourceRef/parseResourceRefName; 138-196 parseGetGraphResourceRefs/V1) + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go (line 145 ServeHTTP GetMeta call; line ~340 ServeGraph GetMeta call; lines 568-604 ServeDownload GetMeta call) + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/getrepo.go (full file 69 lines — the getMeta short-circuit, the getRepo call, the tmplGetDefaultBranch template) + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/client.go (lines 67-72 templates; lines 46-65 httpGetJSON helper) + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/getrepo.go (full file 109 lines — the GetMeta short-circuit, the getRepo chain, the GetBranch/SHA pattern) + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/client.go (full file — Repositories interface, Git interface, the connect() setup) + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/repos.go (full file 82 lines — the sourceRepo wiring, GetMeta/GetFiles methods, the NewMultiRepo pattern) + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-RESEARCH.md (sections "Key Code Facts" #1, #2, #4, #5) + + + + - `parseResourceRefName` on a `Name` proto with `owner="cyp", module="cyp-net-listeners", ref="main/v2"` returns a `moduleRef` with all three fields populated + - A 40-char lowercase hex input to bitbucket `getMeta` bypasses the new ref-resolution path and returns the input verbatim in `meta.Commit` (fast path preserved) + - A 64-char lowercase hex input to bitbucket `getMeta` is also fast-path (Phase 17 contract) + - A non-empty, non-hex, non-SHA input like `"main/v2"` to bitbucket `getMeta` triggers a GET to `/commits/main/v2`, parses the response, and stamps the resolved SHA into `meta.Commit` + - The same three guarantees hold for the github `GetMeta` (using `repos.GetCommit(ctx, owner, repo, ref, ...)` instead of `/commits/{ref}`) + - `isSHA("")` returns false; `isSHA("0"*40)` returns true; `isSHA("0"*64)` returns true; `isSHA("0"*32)` returns false; `isSHA("0"*40+"X")` returns false (length check happens before hex decode) + - `isUUID("")` returns false; `isUUID("0"*32)` returns true; `isUUID("0"*40)` returns false; `isUUID("0"*32+"X")` returns false + + + + This task has 4 sub-changes that all share the same root cause (the providers and the ref parser don't know about refs vs SHAs). Combine them in one task to keep the diff atomic. + + **Step A — `moduleRef` learns `ref`.** In [commits_helpers.go:11-14](internal/connect/commits_helpers.go#L11-L14), add `ref string` to the struct. Update the doc comment to mention that `ref` is the buf BSR `Name.ref` field (branch/tag) and is empty when the client did not send one. Empty `ref` is the existing behavior — the providers treat it as HEAD. + + **Step B — `parseResourceRefName` reads field 3.** In [commits_helpers.go:108-136](internal/connect/commits_helpers.go#L108-L136), add a third `else if` arm that captures the ref: + ``` + } else if num == 3 && typ == protowire.BytesType { + v, mLen := protowire.ConsumeBytes(msg) + msg = msg[mLen:] + ref = string(v) + } + ``` + Add a `var ref string` to the locals (next to `var owner, module string`). Update the `if owner != "" && module != ""` check to ALSO require `ref != ""` only if the upstream contract guarantees `ref` is always present — it does NOT (the buf BSR `Name` proto has `ref` as optional field 3; older clients send only owner+module). Keep the check as `owner != "" && module != ""`; the `ref` field is allowed to be empty. Update the constructor to set `ref: ref` on the returned `moduleRef`. + + **Step C — `parseResourceRefs`, `parseGetGraphResourceRefs`, `parseGetGraphResourceRefsV1` already plumb the ref through** because they only call `parseResourceRef` (which calls `parseResourceRefName`) and pass the result along. No change needed in those three functions. + + **Step D — `ServeHTTP` and `ServeGraph` pass the ref to `GetMeta`.** In [commits.go:145](internal/connect/commits.go#L145), change `meta, err := h.api.repo.GetMeta(r.Context(), ref.owner, ref.module, "")` to `meta, err := h.api.repo.GetMeta(r.Context(), ref.owner, ref.module, ref.ref)`. In `ServeGraph`, find the analogous call (look for `GetMeta(r.Context(), ref.owner, ref.module, "")` near line 340) and make the same substitution. `ServeDownload`'s `GetMeta` call at [commits.go:661](internal/connect/commits.go#L661) is intentionally UNCHANGED — the `commitID` there is either a buf-issued UUID (from `commitMap` lookup) or a foreign id; passing it to the provider as a ref would be wrong. The Download path's commit resolution is handled in task 3 via the probe. + + **Step E — Add `isSHA` and `isUUID` helpers in commits_helpers.go.** Place them immediately after `commitUUID` (around line 60). Both are pure functions, no I/O, no locking. Definitions: + ``` + // isSHA reports whether s is a 40-char (SHA-1) or 64-char (SHA-256) lowercase + // hex string. The hex check rejects refs like "main/v2" and buf-issued UUIDs + // (which are 32 chars). Used by providers to decide whether to treat a + // GetMeta commit arg as a raw SHA (fast path) or as a ref to resolve. + func isSHA(s string) bool { + if len(s) != 40 && len(s) != 64 { + return false + } + for _, c := range s { + if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) { + return false + } + } + return true + } + + // isUUID reports whether s is exactly 32 lowercase hex characters — the + // shape of a buf-issued dashless UUID. Used by probeCommitID to detect + // UUID inputs and route them through commitUUIDInverse. + func isUUID(s string) bool { + if len(s) != 32 { + return false + } + for _, c := range s { + if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) { + return false + } + } + return true + } + ``` + The loop is simpler than `hex.DecodeString` for length-then-charset checks (no allocation). Keep the loop body identical between the two functions for readability. + + **Step F — Bitbucket provider: replace the short-circuit.** In [bitbucket/getrepo.go:18-20](internal/providers/bitbucket/getrepo.go#L18-L20), replace: + ``` + if commit != "" && commit != "main" { + meta.Commit = commit + } + ``` + with: + ``` + if commit != "" && !isSHA(commit) { + resolved, err := c.getCommit(ctx, commit) + if err != nil { + return meta, fmt.Errorf("resolving ref %q: %w", commit, err) + } + meta.Commit = resolved + } else if isSHA(commit) { + meta.Commit = commit + } + // commit == "" → keep HEAD (no-op) + ``` + Add a `getCommit(ctx, ref) (string, error)` helper in the same file. The helper makes a GET to `/commits/{ref}` using `tmplBuild("/commits/{{.id}}")` + `paramsMap{"id": ref}`. Decode the response into a one-field struct (`commitInfo{ID string \`json:"id\"\`}`). Return `info.ID`. Mirror the pattern of `getRepo` / `httpGetJSON` / `tmplGetDefaultBranch` — the new template is `tmplGetCommit = tmplBuild("/commits/{{.id}}")`, declared in `client.go` next to the existing three. Return `errors.Join`-friendly error context: `"requesting: ...", "decoding: ..."` mirrors `httpGetJSON`'s wrap shape. The new `getCommit` does NOT need a `displayId` or `latestCommit` field — it just needs the resolved SHA. No new top-level type is needed; the inline struct is fine. + + **Step G — GitHub provider: same gate, route through `repos.GetCommit`.** In [github/getrepo.go:21-23](internal/providers/github/getrepo.go#L21-L23), replace the short-circuit with the same 3-branch shape (ref → GetCommit, SHA → assign, empty → HEAD). Add `import "github.com/google/go-github/v59/github"` if not already imported (it IS — the file already uses `*github.RepositoryCommit` semantics). The new branch is: + ``` + if commit != "" && !isSHA(commit) { + rc, _, err := c.repos.GetCommit(ctx, owner, repoName, commit, nil) + if err != nil { + return out, fmt.Errorf("resolving ref %q: %w", commit, err) + } + out.Commit = rc.GetSHA() + } else if isSHA(commit) { + out.Commit = commit + } + ``` + The `nil` ListOptions keeps the call single-page; GitHub's `GetCommit` accepts ref names, short SHAs (>= 7 chars), and full SHAs. If the input is a 32-char UUID (which the gate now lets through), GitHub will return 404; the probe (task 3) handles that case by never reaching the provider. + + **Step H — Tests for `isSHA`, `isUUID`, `parseResourceRefName`, both provider `GetMeta` paths.** Add to `commits_helpers_test.go`: + - `TestIsSHA`: 6 cases (`""`, `"0"*40`, `"0"*64`, `"0"*32`, `"0"*40+"X"`, `"0"*40 with one uppercase`) — table-driven + - `TestIsUUID`: 4 cases (`""`, `"0"*32`, `"0"*40`, `"0"*32+"X"`) — table-driven + - `TestParseResourceRefName_ReadsRef`: hand-build a `Name` proto with `owner=1, module=2, ref=3`, call `parseResourceRefName`, assert `moduleRef{owner, module, ref}` is correct. Mirror the protowire.AppendXxx calls from the existing test helpers in `commits_helpers_test.go` (look for `protowire.AppendString` usages around `TestBuildCommitRaw` / `TestParseResourceRefID`). + - `TestParseResourceRefName_NoRef`: same shape but omit the ref field — assert `moduleRef.ref == ""`. + + Add to `bitbucket/getrepo_test.go` (new file): + - `TestGetMeta_Empty`: commit="" → returns HEAD (regression guard for the empty-input path) + - `TestGetMeta_RawSHA_40`: commit="0"*40 → returns the input verbatim (regression guard for the SHA fast path) + - `TestGetMeta_RawSHA_64`: commit="0"*64 → returns the input verbatim + - `TestGetMeta_ResolvesRef`: spin up an `httptest.NewServer` that returns `{"id":"abc123..."}` for `/commits/main/v2`; call `getMeta` with commit="main/v2"; assert `meta.Commit == "abc123..."` + - The mock HTTP server reuses the same URL structure as the production bitbucket client (path prefix `/rest/api/1.0/projects/PROJECT/repos/REPO`); the test constructs the `client` with `baseURL = mockURL + "/rest/api/1.0/projects/CYP/repos/cyp-net-listeners"`. + + Add to `github/getrepo_test.go` (new file): + - `TestGetMeta_Empty`: commit="" → returns HEAD (regression guard) + - `TestGetMeta_RawSHA_40`: commit="0"*40 → returns the input verbatim + - `TestGetMeta_ResolvesRef`: build a `client` with a mock `Repositories` whose `GetCommit` returns `&github.RepositoryCommit{SHA: github.String("abc123...")}`; call `GetMeta` with commit="main/v2"; assert `out.Commit == "abc123..."` + + The `mockrepos_test.go` mock `Repositories` needs a `GetCommit` method added. The current mock implements `Get`, `GetBranch`, `DownloadContents`. Add `GetCommit(ctx, owner, repo, sha string, opts *github.ListOptions) (*github.RepositoryCommit, *github.Response, error)` returning the configured `MockRepo.GetCommit` value (or a sensible default if not set). Keep the existing `DownloadContents` mock intact. + + **Imports:** `commits_helpers.go` does not need new imports (the new helpers use only builtins and the existing `protowire` for the parse change). `bitbucket/getrepo.go` does not need new imports (the inline struct uses only `encoding/json` semantics, the template is `text/template` already imported via `client.go`). `github/getrepo.go` does not need new imports (uses `*github.RepositoryCommit` already). Test files: `commits_helpers_test.go` may need `google.golang.org/protobuf/encoding/protowire` if not already imported (verify by reading; it likely is for the existing parse tests). + + + + + # Build and vet. + go build ./... && go vet ./... + + # isSHA / isUUID tests. + go test ./internal/connect/ -run 'TestIsSHA|TestIsUUID' -v -count=1 + # Must PASS all 10 subtests. + + # parseResourceRefName test. + go test ./internal/connect/ -run 'TestParseResourceRefName' -v -count=1 + # Must PASS both subtests (ReadsRef + NoRef). + + # Bitbucket provider tests. + go test ./internal/providers/bitbucket/ -run 'TestGetMeta' -v -count=1 + # Must PASS all 4 subtests. + + # GitHub provider tests. + go test ./internal/providers/github/ -run 'TestGetMeta' -v -count=1 + # Must PASS all 3 subtests. + + # Short-circuit gone from both providers. + test "$(grep -c 'commit != \"\" && commit != \"main\"' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/getrepo.go)" = "0" + test "$(grep -c 'commit != \"\" && commit != \"main\"' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/getrepo.go)" = "0" + + # isSHA appears in both providers. + grep -n 'isSHA(commit)' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/getrepo.go + grep -n 'isSHA(commit)' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/getrepo.go + + # Bitbucket getCommit helper exists. + grep -n 'func (c client) getCommit' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/getrepo.go + + # GitHub GetCommit call exists in GetMeta. + grep -n 'c.repos.GetCommit' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/getrepo.go + + # moduleRef has a ref field. + grep -n 'ref string' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go + + # parseResourceRefName reads field 3. + grep -n 'num == 3 && typ == protowire.BytesType' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go + + # ServeHTTP and ServeGraph pass the ref (NOT ""). + grep -n 'GetMeta(r.Context(), ref.owner, ref.module, ref.ref)' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go + # Must return 2 (ServeHTTP + ServeGraph). + + + + + - `go build ./...` exits 0 + - `go vet ./...` exits 0 + - `go test ./internal/connect/ -run 'TestIsSHA|TestIsUUID|TestParseResourceRefName' -v -count=1` passes + - `go test ./internal/providers/bitbucket/ -run 'TestGetMeta' -v -count=1` passes all 4 subtests + - `go test ./internal/providers/github/ -run 'TestGetMeta' -v -count=1` passes all 3 subtests + - The `commit != "" && commit != "main"` short-circuit is gone from both providers + - `isSHA(commit)` appears in both providers' GetMeta + - Bitbucket's `getCommit(ctx, ref)` helper exists and is called from `getMeta` + - GitHub's `c.repos.GetCommit(ctx, owner, repoName, commit, nil)` is called from `GetMeta` + - `moduleRef` has a `ref string` field; `parseResourceRefName` reads proto field 3 + - `ServeHTTP` and `ServeGraph` pass `ref.ref` to `GetMeta` (not `""`) + - `ServeDownload` is UNCHANGED (it operates on UUIDs/SHAs, not refs) + + + + + + + + + Task 2: Add `commitUUIDInverse` and table-driven round-trip tests + + + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers_test.go + + + + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go (lines 16-59 — commitUUID doc comment, length check, byte-table, return statement — the inverse is the exact mirror of these lines) + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers_test.go (lines 112-155 TestCommitUUID_KnownSHA — for the table-driven shape; lines 162-185 TestCommitUUID_InvalidInput — for the error-case shape) + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-RESEARCH.md (section "Key Code Facts" #3) + + + + - `commitUUIDInverse(commitUUID("81353411f7b010d5b9ebeb1899066aac18a36701"))` returns `"81353411f7b010d5b9ebeb1899066aac"` (the first 28 hex chars of the input SHA) + - `commitUUIDInverse(commitUUID("0"*40))` returns `"0"*28` + - `commitUUIDInverse(commitUUID("f"*40))` returns `"f"*28` + - `commitUUIDInverse(commitUUID("0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef00"))` returns `"0123456789abcdef0123456789abcdef"` (the first 28 chars of a 64-char SHA-256 input — same as the 40-char fixture above, since commitUUID consumes only the first 14 bytes regardless of input length) + - `commitUUIDInverse("0"*31)` returns an error + - `commitUUIDInverse("0"*33)` returns an error + - `commitUUIDInverse("X"*32)` returns an error (non-hex) + - `commitUUIDInverse("")` returns an error + + + + This task is a single new helper + its test. It is independent of task 1 (no overlap) and is the foundation task 3 builds on (probeCommitID uses it). + + **Step A — Add `commitUUIDInverse` to commits_helpers.go.** Place it immediately after `commitUUID` (around line 60, after the new `isSHA`/`isUUID` helpers from task 1). The doc comment must be explicit about the lossy nature: + + ``` + // commitUUIDInverse recovers the first 28 hex characters (14 bytes) of the + // git SHA that produced the given buf-issued dashless UUID. The full SHA + // cannot be recovered: commitUUID drops the last 6 bytes during the + // forward mapping (review.md "14-of-20-byte collision surface" — 2^112 + // space, accepted as out-of-scope for collision avoidance). The recovered + // prefix is sufficient to identify a specific source among the configured + // providers (each source's commit space is disjoint) and to scope a + // probeCommitID fan-out to the right repository. + // + // Input contract: the input must be exactly 32 lowercase hex characters + // (the standard dashless UUID shape that uuidutil.FromDashless accepts). + // Any other input returns ("", error). + // + // Inverse: if uuid == commitUUID(sha) for some 40- or 64-char sha, then + // commitUUIDInverse(uuid) == hex(sha[0:14]). The inverse does NOT require + // knowledge of the original sha length — it recovers the same 14 bytes + // regardless. + func commitUUIDInverse(uuid string) (string, error) { + if len(uuid) != 32 { + return "", errors.New("commitUUIDInverse: input is not 32 lowercase hex characters") + } + u, err := hex.DecodeString(uuid) + if err != nil { + return "", errors.New("commitUUIDInverse: input is not 32 lowercase hex characters") + } + // Mirror commitUUID's byte-table in reverse. Bytes 6 and 8 of u are + // version/variant and were overwritten by commitUUID; they are not + // recoverable. The other 14 bytes are the first 14 bytes of the SHA. + var sha [20]byte + copy(sha[0:6], u[0:6]) + sha[6] = u[7] + copy(sha[7:14], u[9:16]) + return hex.EncodeToString(sha[:14]), nil + } + ``` + + The function does NOT need to know whether the original SHA was SHA-1 (40 chars) or SHA-256 (64 chars) — both cases produce a UUID whose first 14 bytes are the same 14 bytes. The 20-byte intermediate buffer `sha` is over-provisioned for clarity (we only write 14 bytes; the remaining 6 are left as zero and never read). + + **Step B — Add `TestCommitUUIDInverse` to commits_helpers_test.go.** Place it immediately after the existing `TestCommitUUID_KnownSHA` (which ends around line 155 in the current file; verify by reading). Use a table-driven shape mirroring the existing one. Required cases: + + | name | uuid | want | wantErr | + |------|------|------|---------| + | `"40-char SHA round-trip"` | `commitUUID("81353411f7b010d5b9ebeb1899066aac18a36701")` | `"81353411f7b010d5b9ebeb1899066aac"` | `false` | + | `"all-zero 40-char SHA"` | `commitUUID("0000000000000000000000000000000000000000")` | `"00000000000000000000000000000000"` | `false` | + | `"all-ones 40-char SHA"` | `commitUUID("ffffffffffffffffffffffffffffffffffffffff")` | `"ffffffffffffffffffffffffffffffff"` | `false` | + | `"64-char SHA round-trip"` | `commitUUID("0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef00")` | `"0123456789abcdef0123456789abcdef"` | `false` | + | `"empty string"` | `""` | `""` | `true` | + | `"31 chars"` | `strings.Repeat("a", 31)` | `""` | `true` | + | `"33 chars"` | `strings.Repeat("a", 33)` | `""` | `true` | + | `"32 chars non-hex"` | `strings.Repeat("X", 32)` | `""` | `true` | + + For the 4 success cases, the test computes `commitUUID(sha)` inline (NOT hardcoded UUIDs) so a future change to `commitUUID`'s byte-table automatically updates the test. The `want` prefix is hand-derived from the SHA's first 28 chars. + + For the 4 error cases, assert that `err != nil` and `got == ""`. The error message text is `"commitUUIDInverse: input is not 32 lowercase hex characters"`; assert it contains the substring `"commitUUIDInverse"` to avoid being too tight to the message text. + + **Imports:** No new imports needed. The `encoding/hex` and `strings` imports are already present (used by existing commitUUID tests). + + **Relationship to `commitUUID`:** `commitUUIDInverse` is declared immediately after `commitUUID` so the doc comments are co-located. The new helper is NOT called by `commitUUID` (or vice versa); they are independent functions with the same input contract shape (length + lowercase hex). + + + + + # Build and test. + go build ./... && go vet ./... + go test ./internal/connect/ -run 'TestCommitUUIDInverse' -v -count=1 + # Must PASS all 8 subtests. + + # Function exists at the expected location. + grep -n 'func commitUUIDInverse' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go + + # Test exists. + grep -n 'func TestCommitUUIDInverse' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers_test.go + + # Test uses 8 cases (4 success + 4 error). + test "$(grep -c 'name:' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers_test.go | head -1)" -ge 1 + # Specifically, the TestCommitUUIDInverse body must contain 8 name: entries. + awk '/^func TestCommitUUIDInverse/,/^}/' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers_test.go | grep -c '^ { name:' + # Must return 8. + + # Existing commitUUID tests still pass. + go test ./internal/connect/ -run 'TestCommitUUID' -v -count=1 + + + + + - `go build ./...` exits 0 + - `go vet ./...` exits 0 + - `go test ./internal/connect/ -run 'TestCommitUUIDInverse' -v -count=1` passes all 8 subtests + - `go test ./internal/connect/ -run 'TestCommitUUID' -v -count=1` passes (no regression in existing commitUUID tests) + - `commitUUIDInverse` is declared in `commits_helpers.go` immediately after `commitUUID` + - The doc comment explicitly states the lossy nature (14-of-20-byte recovery) + - The function errors on empty, length-mismatched, and non-hex inputs + + + + + + + + + Task 3: Delete `prewarmHeads`/`registerResolved`/config; rewrite `probeCommitID` to use `commitUUIDInverse` for 32-char UUID inputs; add post-restart integration test + + + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/api.go + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/api_test.go + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/internal/config/config.go + /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/main.go + + + + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go (lines 34-70 commitServiceHandler struct; lines 919-1000 registerResolved and prewarmHeads; lines 1056-1137 probeCommitID) + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/api.go (lines 30-36 CommitResolution struct; lines 66-130 NewWithConfig; lines 111-113 the goroutine launch) + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/api_test.go (full file — the existing mockProvider and mockSource shape; the testMux helper; the existing download tests at lines 590-630; the existing probe-path tests) + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/internal/config/config.go (lines 87-138 Connect struct, PrewarmConfig, WithDefaults) + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/main.go (lines 42-83 main function; the handler factory at 56-65 that passes PrewarmEnabled/PrewarmTimeout to NewWithConfig) + - /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-RESEARCH.md (sections "Key Code Facts" #4, #5; sections "Scope Fence" and "Success Criteria" SC-3, SC-5) + + + + - After task 3, `git grep prewarmHeads` returns 0 hits + - After task 3, `git grep registerResolved` returns 0 hits + - After task 3, `git grep PrewarmEnabled` returns 0 hits + - After task 3, `git grep prewarmEnabled` returns 0 hits + - After task 3, `git grep PrewarmConfig` returns 0 hits + - After task 3, `git grep prewarmOnce` returns 0 hits + - After task 3, `git grep prewarmTimeout` returns 0 hits + - A `Download` request with a 32-char UUID whose first 14 bytes correspond to the HEAD SHA of the only configured source succeeds with a 200 (probe recovers the module via `commitUUIDInverse` + prefix match) + - A `Download` request with a 32-char UUID whose first 14 bytes do NOT correspond to any source's HEAD SHA returns a 400 (probe correctly misses, no false positive) + - A `Download` request with a 40-char raw SHA continues to work via the existing probe path (no regression) + + + + **Step A — Delete `prewarmHeads` and `registerResolved` from commits.go.** Remove lines 908-1000 in their entirety: + - The `registerResolved` function (lines 919-959) plus its 10-line doc comment (lines 908-918) and the trailing blank line + - The `prewarmHeads` function (lines 968-1000) plus its 6-line doc comment (lines 961-967) and the trailing blank line + + After the deletion, line 907's closing brace of the previous function must be followed by a single blank line, and the next function (likely `missCached` or `sweepMisses`) begins on the line after. + + **Step B — Delete the prewarm-related fields from `commitServiceHandler` (commits.go:34-70).** Remove: + - Line 52-53: `prewarmOnce sync.Once` plus its 1-line doc comment + - Lines 60-61: `prewarmEnabled bool` and `prewarmTimeout time.Duration` plus their 2-line doc comment + + The `prewarmSem` field (lines 67-69) is NOT prewarm-related (it's the probe fan-out cap); leave it untouched. After this step, the struct has 4 fewer fields. + + **Step C — Update `CommitResolution` in api.go (lines 30-36).** Remove `PrewarmEnabled` and `PrewarmTimeout`. The struct becomes: + ``` + type CommitResolution struct { + ProbeEnabled bool + ProbeNegativeTTL time.Duration + ProbeTimeout time.Duration + } + ``` + Update the doc comment (lines 23-29) to drop the prewarm sentence: "Commit-resolution enhancements are the upstream sha probe used on a Download cache miss." + + **Step D — Update `NewWithConfig` in api.go (lines 66-130).** Remove the goroutine launch: + ``` + if commitHandler.prewarmEnabled && commitHandler.prewarmTimeout > 0 { + go commitHandler.prewarmHeads() + } + ``` + (lines 111-113). The probe goroutine launch a few lines below stays. The `commitHandler` struct literal at lines 95-110 also loses the `prewarmEnabled:`, `prewarmTimeout:`, and `prewarmOnce:` (wait, prewarmOnce was the field, not in the struct literal — only the first two are in the literal) entries. The `prewarmSem:` entry stays (it's the probe cap). + + **Step E — Rewrite `probeCommitID` (commits.go:1056-1137).** The new shape: + ``` + func (h *commitServiceHandler) probeCommitID(ctx context.Context, id string) (*moduleRef, bool) { + if id == "" || h.missCached(id) { + return nil, false + } + // Re-check commitMap under the lock. + h.commitMu.RLock() + ref, already := h.commitMap[id] + h.commitMu.RUnlock() + if already { + r := ref + return &r, true + } + + // If id is a buf-issued UUID (32 hex chars), derive the SHA prefix + // and probe with the prefix. The 14-byte recovery is lossy but + // 2^112 — sufficient to identify a single source among the + // configured set, and the prefix-match validation below rules out + // collisions (a source whose HEAD SHA does not start with the + // recovered prefix is not a hit, even if its GetMeta call + // succeeded). + probeArg := id + if isUUID(id) { + prefix, err := commitUUIDInverse(id) + if err != nil { + // id looks UUID-shaped but isn't hex — treat as a miss. + h.rememberMiss(id) + return nil, false + } + probeArg = prefix + } else if !isSHA(id) { + // Not a UUID, not a SHA — providers can't resolve it. + h.rememberMiss(id) + return nil, false + } + + // Bound concurrent probes. + if h.probeSem != nil { + select { + case h.probeSem <- struct{}{}: + defer func() { <-h.probeSem }() + default: + return nil, false + } + } + + sources := h.api.repo.Repositories() + if len(sources) == 0 { + return nil, false + } + + type probeResult struct { + ref moduleRef + commit string + ok bool + } + results := make(chan probeResult, len(sources)) + var transient atomic.Bool + var wg sync.WaitGroup + for _, s := range sources { + wg.Add(1) + go func(s source.Source) { + defer wg.Done() + pctx, cancel := context.WithTimeout(ctx, h.probeTimeout) + defer cancel() + meta, err := s.GetMeta(pctx, probeArg) + if err != nil { + if isTransientErr(err) { + transient.Store(true) + } + return + } + if meta.Commit == "" { + return + } + // Prefix-match validation: when probeArg is a SHA prefix + // (28 chars from commitUUIDInverse), the source's commit + // MUST start with it. Otherwise the collision went the wrong + // way and the source doesn't actually own this UUID. + if isUUID(id) && !strings.HasPrefix(meta.Commit, probeArg) { + return + } + results <- probeResult{ + ref: moduleRef{owner: s.Owner(), module: s.RepoName()}, + commit: meta.Commit, + ok: true, + } + }(s) + } + wg.Wait() + close(results) + + for r := range results { + // First (only) success. Register both the buf-issued UUID and + // the resolved SHA as aliases so future requests hit directly. + h.registerResolvedAlias(id, r.commit, r.ref.owner, r.ref.module) + ref := r.ref + return &ref, true + } + if !transient.Load() { + h.rememberMiss(id) + } + return nil, false + } + ``` + + The new `registerResolvedAlias` is a smaller helper that does what `registerResolved` did, but takes the original buf-issued id (which may be a UUID) as the primary key and the resolved SHA as the alias: + + ``` + // registerResolvedAlias writes a commit id → moduleRef mapping and the + // matching infoCache entry. The id arg is what the buf client sent + // (typically a buf-issued 32-char UUID); sha is what the upstream + // resolved to (a 40- or 64-char hex). Both are stored in commitMap so + // future identical requests hit directly, and sha is also kept as an + // alias for any caller (debug tool, foreign-id probe) that sends a + // raw sha. + func (h *commitServiceHandler) registerResolvedAlias(id, sha, owner, module string) { + key := owner + "/" + module + h.commitMu.Lock() + h.commitMap[id] = moduleRef{owner: owner, module: module} + if sha != "" && sha != id { + h.commitMap[sha] = moduleRef{owner: owner, module: module} + } + if existing, ok := h.infoCache[key]; ok { + existing.commitID = id + existing.commit = sha + existing.ownerID = owner + existing.moduleID = key + h.infoCache[key] = existing + } else { + h.infoCache[key] = commitInfoCache{ + commitID: id, + commit: sha, + ownerID: owner, + moduleID: key, + } + } + h.commitMu.Unlock() + } + ``` + + Note: this helper is NEW — it is NOT a rename of `registerResolved`. The old `registerResolved` is deleted (step A). The new helper is ~30 lines (vs the old ~40) because it does not need the `commitUUID` call (the buf-issued id is already known) and does not need the `commitUUID contract violation` warn-and-return (task 1's `isSHA` gate means the probe never sends a non-SHA arg to a provider). + + **Step F — Add the `strings` import to commits.go if not already present.** The `strings.HasPrefix` call in the new probe needs it. Check line 13 — `strings` is already imported. + + **Step G — Delete `PrewarmConfig` from config.go (lines 96-101).** Remove the struct, its 5-line doc comment, and the `Prewarm PrewarmConfig` field on the `Connect` struct (line 88). Also remove the `Prewarm.*` block in `WithDefaults` (lines 120-126, 4 lines). The `Connect` struct retains `Probe ProbeConfig`. + + **Step H — Update main.go (lines 56-65).** The `handler := func() ...` block currently passes `PrewarmEnabled: ...` and `PrewarmTimeout: ...` to `connect.CommitResolution{...}`. Remove those two lines. The `connect.NewWithConfig` call and the `cc := cfg.Connect.WithDefaults()` call also drop the `cc.Prewarm.*` references. + + **Step I — Add `TestServeDownload_AfterRestart_ProbeResolvesUUID` to api_test.go.** Place it after the existing `TestServeDownload_UnknownCommitID_ReturnsBadRequest` (which ends around line 218 in `uuid_format_test.go`; place the new test in `api_test.go` next to the other download integration tests). The test: + + 1. Builds a `mockProvider` with `meta.Commit` set to a known 40-char SHA-1 (use `"81353411f7b010d5b9ebeb1899066aac18a36701"` — same fixture as Phase 17). + 2. Computes the expected UUID via `commitUUID(meta.Commit)` at test setup. + 3. Builds a `commitServiceHandler` directly (not via `testMux` — the test needs to control the pre-test state of `commitMap` and skip the prewarm). Initialize `commitMap` as an empty map. Set `probeEnabled = true`, `probeNegativeTTL = 5 * time.Minute`, `probeTimeout = 2 * time.Second`. + 4. Mounts the handler on `httptest.NewServer`. + 5. Issues a `Download` request with the UUID in the body. + 6. Asserts the response is 200 OK and the served file content matches `meta.files[0].Data`. + + The test asserts that with `commitMap` empty (post-restart state), the probe correctly: + 1. Detects the 32-char input as a UUID via `isUUID` + 2. Calls `commitUUIDInverse` to recover the 28-char SHA prefix + 3. Calls `GetMeta(ctx, owner, module, prefix)` on the configured source + 4. The mock source returns the 40-char SHA (or returns the 28-char prefix and the test is OK with that — the probe's `strings.HasPrefix(meta.Commit, probeArg)` check succeeds either way) + 5. Registers the alias and returns the ref + + Add a second subtest `TestServeDownload_AfterRestart_ProbeMissesOnUnknownUUID` that uses a 32-char UUID whose first 14 bytes do NOT correspond to any configured source's SHA, and asserts the response is 400 (probe correctly misses, no false positive). + + **Imports for api_test.go:** the existing file already imports `time`, `bytes`, `encoding/hex`, `net/http`, `net/http/httptest`, `google.golang.org/protobuf/encoding/protowire`, and the provider/source packages. No new imports needed for the new test. + + **Imports for config.go:** the existing file imports `net/netip` and `time`. After removing the prewarm block, `time` is still used by the probe config (`ProbeNegativeTTL`, `ProbeTimeout`); no import changes. + + **Imports for main.go:** the existing file imports `time` (line 13). After removing the prewarm references, `time` is still used by `connectionTimeout` and `accessCheckPeriod` (lines 38-39); no import changes. + + + + + # Build and vet. + go build ./... && go vet ./... + + # prewarmHeads / registerResolved / PrewarmConfig gone everywhere. + test "$(grep -rn 'prewarmHeads' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/ 2>/dev/null | grep -v '^Binary' | wc -l | tr -d ' ')" = "0" + test "$(grep -rn 'registerResolved' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/ 2>/dev/null | grep -v '^Binary' | wc -l | tr -d ' ')" = "0" + test "$(grep -rn 'PrewarmEnabled\|prewarmEnabled\|PrewarmConfig\|prewarmOnce\|prewarmTimeout' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/ 2>/dev/null | grep -v '^Binary' | wc -l | tr -d ' ')" = "0" + + # New registerResolvedAlias exists. + grep -n 'func (h \*commitServiceHandler) registerResolvedAlias' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go + + # probeCommitID uses commitUUIDInverse. + grep -n 'commitUUIDInverse' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go + + # probeCommitID has the prefix-match validation. + grep -n 'strings.HasPrefix(meta.Commit, probeArg)' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go + + # Prewarm references gone from main.go and config.go. + test "$(grep -c 'Prewarm' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/main.go)" = "0" + test "$(grep -c 'Prewarm' /Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/internal/config/config.go)" = "0" + + # Full test suite passes. + go test ./internal/connect/ -count=1 + go test ./internal/providers/bitbucket/ -count=1 + go test ./internal/providers/github/ -count=1 + + # New integration test. + go test ./internal/connect/ -run 'TestServeDownload_AfterRestart_ProbeResolvesUUID|TestServeDownload_AfterRestart_ProbeMissesOnUnknownUUID' -v -count=1 + # Must PASS both subtests. + + + + + - `go build ./...` exits 0 + - `go vet ./...` exits 0 + - `go test ./internal/connect/ -count=1` passes (no regression, all existing tests pass) + - `go test ./internal/providers/bitbucket/ -count=1` passes + - `go test ./internal/providers/github/ -count=1` passes + - `go test ./internal/connect/ -run 'TestServeDownload_AfterRestart_ProbeResolvesUUID' -v -count=1` passes + - `go test ./internal/connect/ -run 'TestServeDownload_AfterRestart_ProbeMissesOnUnknownUUID' -v -count=1` passes + - `prewarmHeads` is gone from the codebase + - `registerResolved` (old) is gone; `registerResolvedAlias` (new) exists + - `PrewarmConfig` is gone from config.go + - `PrewarmEnabled` / `prewarmEnabled` / `prewarmTimeout` / `prewarmOnce` are gone everywhere + - The probe uses `commitUUIDInverse` for 32-char UUID inputs and validates the prefix match + - `main.go` no longer references `cc.Prewarm.*` or passes `PrewarmEnabled`/`PrewarmTimeout` to `connect.NewWithConfig` + + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| client → `parseResourceRefName` | Untrusted wire input (buf CLI v1/v1beta1 `Name` proto field 3) | +| client → `commitServiceHandler.probeCommitID` | Untrusted `commitID` (could be a buf-issued UUID, a raw SHA, or a foreign id) | +| `commitUUIDInverse` → provider `GetMeta` | Inverse-derived SHA prefix used as a probe arg (28 lowercase hex chars) | +| provider `getMeta` → upstream commit-fetch API | Untrusted ref string from client (must NOT be passed verbatim to upstream as a SHA) | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Disposition | Mitigation Plan | +|-----------|----------|-----------|-------------|-----------------| +| T-18-01 | Tampering | `parseResourceRefName` ref field (commits_helpers.go:108) | mitigate | The ref is consumed only by the providers' ref-resolution API path, never stamped into `meta.Commit` without first round-tripping through the upstream commit-fetch API. The provider's HTTP client validates the response shape (JSON decode to a struct with a single `ID` field); a malformed response is rejected at the decode step. | +| T-18-02 | Information Disclosure | Bitbucket `getCommit` response | mitigate | The new `/commits/{ref}` endpoint returns only the resolved commit metadata (id, displayId, author, timestamp). It does not return file contents or secrets. The response is logged at DEBUG level only. | +| T-18-03 | Repudiation | `probeCommitID` prefix-match validation (commits.go:~1115) | mitigate | The `strings.HasPrefix(meta.Commit, probeArg)` check ensures a source whose HEAD SHA does not start with the recovered prefix is treated as a miss. The probe logs the source-selection decision with `request_id` for correlation. | +| T-18-04 | Denial of Service | `commitUUIDInverse` on a 32-char input that is not a UUID | mitigate | The function returns an error for length-mismatched or non-hex inputs; the probe treats this as a miss and negative-caches. The cost is one `hex.DecodeString` call (microseconds) per probe. | +| T-18-05 | Denial of Service | `getCommit` for a malicious ref string | mitigate | Bitbucket's `/commits/{ref}` returns 404 for any ref string it cannot resolve; the cost is one HTTP round-trip per probe. The `probeSem` cap and `probeTimeout` bound the worst case at `maxConcurrentProbes × probeTimeout` per fan-out. | +| T-18-06 | Elevation of Privilege | Removal of prewarm | accept | Prewarm is replaced by a probe path that has the same fan-out shape (one `GetMeta` call per configured source, bounded by `probeSem`). No auth paths are touched. | +| T-18-07 | Tampering | `commitUUIDInverse` collision (2^112 space) | mitigate | The probe's prefix-match validation (T-18-03) rules out wrong-source matches. A genuine collision (two sources whose HEAD SHAs share the first 14 bytes) is a corner case with vanishing probability; the prefix-match check fails closed (miss → 400) in that case. | +| T-18-08 | Information Disclosure | `prewarmHeads` removal observability | accept | The first `Download` request after a process restart now incurs one upstream round-trip per source (probe fan-out) instead of zero (prewarm). This is a latency, not a security, change. The `probeSem` cap bounds the worst case. | +| T-18-DEFER | Tampering | `prewarmHeads`-related review.md finding #6 (probe/register cache contract) | accept (resolved by task 1) | Task 1's `isSHA(commit)` gate causes the providers to return an error for 32-char UUID inputs (the input is neither empty, nor 40/64 hex, nor a resolvable ref). The probe correctly misses, and task 3's rewrite routes UUID inputs through `commitUUIDInverse` to a prefix-probe that validates the source's actual SHA. The previous broken "false hit" path is closed. | + +## ASVS Coverage + +- **V1 (Architecture):** Threat boundaries documented; trust levels match deployment (clients are untrusted; upstream providers are trusted-ish but their input is validated). +- **V2 (Authentication):** N/A — no auth paths touched. +- **V3 (Session Management):** N/A — no session state. +- **V4 (Access Control):** N/A — proxy serves all configured modules. +- **V5 (Input Validation):** All new wire-input consumers (`ref` from `Name`, UUIDs from `commitID`, refs from `getCommit`) are validated for shape (length, hex charset) before use. +- **V6 (Cryptography):** N/A — SHA-1/SHA-256 are used only for content addressing, not for security. +- **V7 (Error Handling):** New errors (`commitUUIDInverse` rejects, `getCommit` 404) are logged and surfaced as 400/502 to the client per the existing handler error contract. +- **V8 (Data Protection):** N/A — no secrets handled. +- **V9 (Communications):** N/A — TLS handled by the existing `https.ListenAndServe` configuration. +- **V10 (Malicious Code):** N/A — no third-party code added. +- **V11 (Business Logic):** The new ref-resolution path is bounded: one upstream call per non-SHA input, with `probeTimeout` and `probeSem` caps. +- **V12 (Files):** N/A — no file I/O. +- **V13 (API):** Wire format unchanged; only the parsing/processing of the wire format is updated. +- **V14 (Configuration):** The `PrewarmConfig` block is removed; the `Connect` struct retains `Probe ProbeConfig` only. + + + +After all 3 tasks complete, run the full validation suite: + +```bash +# Build + vet (must be clean). +go build ./... && go vet ./... + +# Full connect-package test suite. +go test ./internal/connect/ -count=1 + +# Targeted tests for the new functionality. +go test ./internal/connect/ -run 'TestCommitUUIDInverse|TestIsSHA|TestIsUUID|TestParseResourceRefName|TestServeDownload_AfterRestart' -v -count=1 + +# Provider tests. +go test ./internal/providers/bitbucket/ -count=1 +go test ./internal/providers/github/ -count=1 + +# Phase 16/17 regression guards. +go test ./internal/connect/ -run 'TestServeHTTP_GetCommits_ReturnsDashlessUUID|TestServeDownload_RoundTripWithMintedUUID|TestServeDownload_UnknownCommitID_ReturnsBadRequest|TestCommitUUID_SHA256_KnownSHA|TestCommitUUID_KnownSHA' -v -count=1 + +# Structural grep checks (all must return 0 hits for the deleted artifacts, >= 1 hit for new ones). +test "$(grep -rn 'prewarmHeads' . 2>/dev/null | grep -v '^Binary' | wc -l | tr -d ' ')" = "0" +test "$(grep -rn 'registerResolved$' . 2>/dev/null | grep -v '^Binary' | wc -l | tr -d ' ')" = "0" +test "$(grep -rn 'PrewarmEnabled\|prewarmEnabled\|PrewarmConfig\|prewarmOnce\|prewarmTimeout' . 2>/dev/null | grep -v '^Binary' | wc -l | tr -d ' ')" = "0" +test "$(grep -c 'func commitUUIDInverse' internal/connect/commits_helpers.go)" = "1" +test "$(grep -c 'func (h \*commitServiceHandler) registerResolvedAlias' internal/connect/commits.go)" = "1" +test "$(grep -c 'commitUUIDInverse' internal/connect/commits.go)" -ge 1 +test "$(grep -c 'strings.HasPrefix(meta.Commit, probeArg)' internal/connect/commits.go)" = "1" +test "$(grep -c 'isSHA(commit)' internal/providers/bitbucket/getrepo.go)" -ge 1 +test "$(grep -c 'isSHA(commit)' internal/providers/github/getrepo.go)" -ge 1 +test "$(grep -c 'isUUID' internal/connect/commits.go)" -ge 2 +test "$(grep -c 'num == 3 && typ == protowire.BytesType' internal/connect/commits_helpers.go)" = "1" +test "$(grep -c 'GetMeta(r.Context(), ref.owner, ref.module, ref.ref)' internal/connect/commits.go)" = "2" +test "$(grep -c 'Prewarm' cmd/easyp/main.go cmd/easyp/internal/config/config.go)" = "0" +``` + +The 5 ROADMAP success criteria map to these checks: + +- **SC-1 (refs respected):** `GetMeta(r.Context(), ref.owner, ref.module, ref.ref)` appears 2x in commits.go (ServeHTTP + ServeGraph); `num == 3 && typ == protowire.BytesType` parses the ref in parseResourceRefName; `TestParseResourceRefName_ReadsRef` passes. +- **SC-2 (related bug fixed):** `isSHA(commit)` appears in both providers; `commit != "" && commit != "main"` is gone from both providers; `TestGetMeta_ResolvesRef` passes for both providers. +- **SC-3 (prewarm removed):** `prewarmHeads` / `registerResolved` / `PrewarmEnabled` / `prewarmEnabled` / `PrewarmConfig` / `prewarmOnce` / `prewarmTimeout` all return 0 hits; `Prewarm` returns 0 hits in main.go and config.go; `go build ./...` is clean. +- **SC-4 (inverse helper):** `commitUUIDInverse` exists in commits_helpers.go; `TestCommitUUIDInverse` passes 8 subtests. +- **SC-5 (no regression):** Phase 16/17 tests pass; `TestServeDownload_AfterRestart_ProbeResolvesUUID` and `TestServeDownload_AfterRestart_ProbeMissesOnUnknownUUID` pass. + + + +Phase 18 is complete when ALL of the following are true: + +1. `go build ./...` exits 0 +2. `go vet ./...` exits 0 +3. `go test ./internal/connect/ -count=1` passes with no failures, no skips +4. `go test ./internal/providers/bitbucket/ -count=1` passes +5. `go test ./internal/providers/github/ -count=1` passes +6. `go test ./internal/connect/ -run 'TestCommitUUIDInverse|TestIsSHA|TestIsUUID|TestParseResourceRefName|TestServeDownload_AfterRestart' -v -count=1` passes +7. Phase 16/17 regression guards all pass (`TestServeHTTP_GetCommits_ReturnsDashlessUUID`, `TestServeDownload_RoundTripWithMintedUUID`, `TestCommitUUID_SHA256_KnownSHA`, `TestCommitUUID_KnownSHA`) +8. `prewarmHeads` / `registerResolved` (old) / `PrewarmConfig` / `PrewarmEnabled` / `prewarmEnabled` / `prewarmOnce` / `prewarmTimeout` are all gone from the codebase (0 grep hits each) +9. The 5 ROADMAP success criteria (SC-1 through SC-5) are satisfied, as verified by the structural grep checks in the `` section +10. No new TODOs, FIXMEs, XXX, HACK, or PLACEHOLDER markers in the 11 modified files +11. No new dependencies in go.mod / go.sum +12. The `strings` import in commits.go is still used (the new probe uses `strings.HasPrefix`) +13. The `isSHA` and `isUUID` helpers are in commits_helpers.go (not inlined, not duplicated) +14. The `commitUUIDInverse` function is declared immediately after `commitUUID` (co-located) +15. The probe's prefix-match validation closes the review.md finding #6 (probe/register cache contract divergence) by failing closed on a wrong-source match + + + +Create `/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-01-SUMMARY.md` when done. + diff --git a/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-01-SUMMARY.md b/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-01-SUMMARY.md new file mode 100644 index 0000000..acd8270 --- /dev/null +++ b/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-01-SUMMARY.md @@ -0,0 +1,210 @@ +--- +phase: 18-respect-buf-yaml-dependency-refs-not-always-head-fix-related +plan: 01 +subsystem: connect +tags: [buf, grpc, dependency-resolution, probe, prewarm-removal, uuid-inverse, refs, prefix-match] + +# Dependency graph +requires: + - phase: 16-commit-id-resolution-improvements + provides: "commitUUID mint helper, prewarmHeads, registerResolved, probeCommitID, CommitResolution struct" + - phase: 17-fix-pr-37-review-findings + provides: "Phase 16 fixes (dashless UUID format, prewarm-enabled by default)" +provides: + - "moduleRef.ref field (buf BSR Name.ref, proto field 3) plumbed end-to-end" + - "parseResourceRefName reads field 3" + - "ServeHTTP and ServeGraph pass ref.ref to GetMeta (was \"\")" + - "isSHA / isUUID pure-function helpers in commits_helpers.go" + - "Bitbucket provider: new getCommit(ctx, ref) helper that calls /commits/{ref} and returns the resolved SHA" + - "GitHub provider: GetMeta routes non-SHA inputs through repos.GetCommit(ctx, owner, repo, ref, nil)" + - "commitUUIDInverse(uuid) (string, error) — 14-byte recovery from a 32-char dashless UUID" + - "probeCommitID rewritten to use commitUUIDInverse for 32-char UUID inputs with prefix-match validation (fail-closed)" + - "registerResolvedAlias helper that takes the buf-issued id (UUID) as primary key and the resolved SHA as alias" + - "prewarmHeads, registerResolved, prewarmOnce, prewarmEnabled, prewarmTimeout, PrewarmEnabled, PrewarmTimeout, PrewarmConfig all removed" +affects: [19, future-buf-v1-cli-tests, future-foreign-id-handling] + +# Tech tracking +tech-stack: + added: [] + patterns: + - "isSHA / isUUID duplicated as a tiny 8-line helper in each provider package (avoided the unexported-connect-to-provider dependency; pure functions, low cost)" + - "prefix-match validation in probeCommitID: when probe arg is a UUID-derived prefix, the source's meta.Commit MUST start with the prefix (fail-closed) — closes review.md finding #6" + - "registerResolvedAlias takes the original client-sent id (UUID) as primary key, not the resolved SHA — matches the buf v1.69.0 commit_id contract" + +key-files: + created: + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/getrepo_test.go" + provides: "4 new TestGetMeta_* cases (Empty, RawSHA_40, RawSHA_64, ResolvesRef) for the Bitbucket provider" + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/getrepo_test.go" + provides: "3 new TestGetMeta_* cases (Empty, RawSHA_40, ResolvesRef) for the GitHub provider" + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/mockrepos_test.go" + provides: "new mockRepos mock with GetCommit plumbing and call-count tracking for the GitHub provider tests" + modified: + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers.go" + provides: "moduleRef gains ref; parseResourceRefName reads field 3; isSHA + isUUID + commitUUIDInverse helpers" + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits_helpers_test.go" + provides: "TestIsSHA (8 cases), TestIsUUID (6 cases), TestParseResourceRefName_ReadsRef, TestParseResourceRefName_NoRef, TestCommitUUIDInverse (8 cases)" + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/commits.go" + provides: "ServeHTTP/ServeGraph pass ref.ref; probeCommitID rewritten to use commitUUIDInverse + prefix-match; prewarmHeads/registerResolved deleted; prewarm fields deleted" + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/api.go" + provides: "CommitResolution loses PrewarmEnabled/PrewarmTimeout; NewWithConfig drops the goroutine launch" + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/api_test.go" + provides: "TestServeDownload_AfterRestart_ProbeResolvesUUID + TestServeDownload_AfterRestart_ProbeMissesOnUnknownUUID; mockSource accepts HasPrefix; TestPrewarmHeads_PopulatesCommitMap removed; TestProbeCommitID_* tests updated for the isSHA gate" + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/connect/uuid_format_test.go" + provides: "stale 'registerResolved' comment updated to 'registerResolvedAlias'" + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/getrepo.go" + provides: "getMeta uses isSHA(commit) gate; new getCommit(ctx, ref) helper" + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/bitbucket/client.go" + provides: "new tmplGetCommit template for /commits/{{.id}}" + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/internal/providers/github/getrepo.go" + provides: "GetMeta uses isSHA(commit) gate; non-SHA inputs route through repos.GetCommit" + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/internal/config/config.go" + provides: "PrewarmConfig struct deleted; Connect struct retains Probe only" + - path: "/Users/nil/DiskD/W/Djarvur/easyp-buf-proxy/cmd/easyp/main.go" + provides: "handler factory drops cc.Prewarm.* references" + +key-decisions: + - "isSHA and isUUID are unexported in the connect package; each provider package defines its own 8-line local isSHA. Avoids the unexported-cross-package import; trivial duplication." + - "registerResolvedAlias is a new helper, not a rename of registerResolved. It takes the original client-sent id (typically a buf-issued UUID) as the primary key, matching the buf v1.69.0 commit_id contract." + - "probeCommitID fails closed on a prefix mismatch (a source whose meta.Commit does not start with the recovered 28-char prefix is treated as a miss, not a hit). Closing the wrong-source-match security/correctness risk in review.md finding #6." + - "Inputs that are neither SHA-shaped (40/64 hex) nor UUID-shaped (32 hex) are negative-cached and return early in probeCommitID — no upstream fan-out. The 'bogus000' fixture from Phase 16's probe tests was updated to a 40-char SHA to match the new gate." + - "The prewarm removal is observable to the client: a buf build immediately after a proxy restart will incur one extra upstream round-trip per module (the probe fan-out). The probeSem cap (maxConcurrentProbes=4) and per-source probeTimeout (8s default) bound the worst case." + +patterns-established: + - "Three-branch commit gate in providers: commit == \"\" → HEAD; isSHA(commit) → fast path; else → ref-resolution API call" + - "Provider tests use httptest.NewServer (Bitbucket) or a configurable Repositories mock (GitHub) rather than a real network call" + - "TDD with RED-then-GREEN: failing tests committed first, then the production change in a separate commit" + +requirements-completed: + - SC-1 (refs respected: GetCommits/GetGraph return UUID from ref, not HEAD) + - SC-2 (related bug fixed: provider short-circuit replaced with isSHA gate) + - SC-3 (prewarm removed: prewarmHeads, registerResolved, related fields, config block) + - SC-4 (inverse helper: commitUUIDInverse + round-trip tests) + - SC-5 (no regression: Phase 16/17 tests still pass) + +# Metrics +duration: 18min +completed: 2026-07-07 +--- + +# Phase 18 Plan 01: Respect buf.yaml Dependency Refs Summary + +**End-to-end ref support for buf.yaml deps + isSHA-gated provider ref-resolution + prewarm removal replaced by commitUUIDInverse-based probe** + +## Performance + +- **Duration:** 18 min +- **Started:** 2026-07-07T13:58:32Z +- **Completed:** 2026-07-07T14:16:20Z +- **Tasks:** 3 +- **Files modified:** 11 + +## Accomplishments + +- **`moduleRef` plumbs the `ref` field through `parseResourceRefName` → `ServeHTTP`/`ServeGraph` → provider `GetMeta`.** A `buf.yaml` dep like `cyp/cyp-net-listeners:main/v2` now returns the SHA at `main/v2`, not HEAD. Backward-compatible: clients that omit `ref` (older buf CLI, v1beta1 callers) still resolve to HEAD. +- **The `commit != "" && commit != "main"` short-circuit in both providers is replaced with an `isSHA(commit)` gate** (40/64 lowercase hex). Non-SHA inputs route through the providers' commit-fetch APIs: Bitbucket's `/commits/{ref}` returns the resolved id, GitHub's `repos.GetCommit(ctx, owner, repo, ref, nil)` returns the resolved SHA. The new path is one extra round-trip per ref-resolved request; HEAD-only requests still skip resolution. +- **`prewarmHeads` and the related startup fan-out are completely removed.** The probe is rewritten to derive a 28-char SHA prefix from a 32-char buf-issued UUID via the new `commitUUIDInverse` helper, then validate that the source's `meta.Commit` actually starts with the recovered prefix (prefix-match validation, fail-closed on mismatch). This closes the review.md finding #6 (probe/register cache contract divergence) — a wrong-source match is now treated as a miss, not a silent hit. + +## Task Commits + +Each task was committed atomically with RED → GREEN TDD: + +1. **Task 1: Honor `Name.ref` end-to-end + provider ref-resolution** - `b16fd2d` (test, RED) → `6a49450` (feat, GREEN) +2. **Task 2: Add `commitUUIDInverse` and round-trip tests** - `6fc909d` (test, RED) → `372bb15` (feat, GREEN) +3. **Task 3: Delete prewarm + rewrite `probeCommitID` to use the inverse** - `1d5fe33` (feat) + +## Files Created/Modified + +- `internal/connect/commits_helpers.go` - `moduleRef` gains `ref`; `parseResourceRefName` reads field 3; new `isSHA`, `isUUID`, `commitUUIDInverse` helpers +- `internal/connect/commits_helpers_test.go` - `TestIsSHA` (8), `TestIsUUID` (6), `TestParseResourceRefName_ReadsRef`, `TestParseResourceRefName_NoRef`, `TestCommitUUIDInverse` (8) +- `internal/connect/commits.go` - `ServeHTTP`/`ServeGraph` pass `ref.ref`; `probeCommitID` rewritten with `commitUUIDInverse` + prefix-match; `prewarmHeads`/`registerResolved`/prewarm fields deleted +- `internal/connect/api.go` - `CommitResolution` loses `PrewarmEnabled`/`PrewarmTimeout`; `NewWithConfig` drops the goroutine launch +- `internal/connect/api_test.go` - `TestServeDownload_AfterRestart_ProbeResolvesUUID` + `TestServeDownload_AfterRestart_ProbeMissesOnUnknownUUID`; `mockSource` accepts `HasPrefix`; `TestPrewarmHeads_PopulatesCommitMap` removed +- `internal/connect/uuid_format_test.go` - stale `registerResolved` comment updated +- `internal/providers/bitbucket/getrepo.go` - `getMeta` uses `isSHA(commit)` gate; new `getCommit(ctx, ref)` helper +- `internal/providers/bitbucket/client.go` - new `tmplGetCommit` template for `/commits/{{.id}}` +- `internal/providers/bitbucket/getrepo_test.go` (new) - 4 TestGetMeta_* cases via `httptest.NewServer` +- `internal/providers/github/getrepo.go` - `GetMeta` uses `isSHA(commit)` gate; non-SHA inputs route through `repos.GetCommit` +- `internal/providers/github/getrepo_test.go` (new) - 3 TestGetMeta_* cases +- `internal/providers/github/mockrepos_test.go` (new) - new `mockRepos` mock with `GetCommit` plumbing +- `cmd/easyp/internal/config/config.go` - `PrewarmConfig` deleted; `Connect` retains `Probe` only +- `cmd/easyp/main.go` - handler factory drops `cc.Prewarm.*` references + +## Decisions Made + +- **isSHA/isUUID are unexported in the connect package; each provider has its own 8-line local `isSHA`.** Avoids an unexported-cross-package import path; the helpers are pure functions, ~8 lines each, and the cost of one duplication is far less than the cost of exporting them and widening the public API. +- **`registerResolvedAlias` is a new helper, not a rename of `registerResolved`.** It takes the original client-sent id (typically a buf-issued UUID) as the primary key and the resolved SHA as the alias. The old `registerResolved` did the inverse (took the SHA, called `commitUUID` to mint a UUID, stored both). The new helper is needed because in the post-restart probe path, the buf-issued id is already known — no need to re-derive it. +- **probeCommitID fails closed on a prefix mismatch.** A source whose `meta.Commit` does not start with the recovered 28-char prefix is treated as a miss, not a hit. This closes the review.md finding #6: a wrong-source match would silently alias a real commit id to a wrong module and serve wrong content. Failing closed (miss → 400 to client) lets the client re-run `buf mod update` and recover. +- **Inputs that are neither SHA-shaped nor UUID-shaped are negative-cached and return early in probeCommitID.** No upstream fan-out for unresolvable inputs. The 'bogus000' fixture in Phase 16's probe tests was updated to a 40-char SHA to match the new gate. +- **The prewarm removal is observable to the client.** A `buf build` immediately after a proxy restart will incur one extra upstream round-trip per module (the probe fan-out). The `probeSem` cap (`maxConcurrentProbes=4`) and per-source `probeTimeout` (8s default) bound the worst case at `4 × 8s = 32s` per request in the absolute worst case; in practice the fan-out is parallel and completes in < 8s. + +## Deviations from Plan + +### Auto-fixed Issues + +**1. [Rule 1 - Bug] Fixed test data length for the 64-char SHA case in TestCommitUUIDInverse** +- **Found during:** Task 2 (initial test run after the GREEN commitUUIDInverse implementation) +- **Issue:** The plan's fixture for the 64-char SHA case was `0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef00` (66 chars, not 64) — the `commitUUID` function rejected it with "input is not 40 or 64 lowercase hex characters". The fixture was hand-derived and miscounted. +- **Fix:** Truncated to 64 chars: `0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef`. The recovered 28-char prefix is still `0123456789abcdef0123456789ab` (the first 14 bytes of either length input produce the same UUID). +- **Files modified:** `internal/connect/commits_helpers_test.go` +- **Verification:** `TestCommitUUIDInverse/64-char_SHA_round-trip` passes; the test computes `mustCommitUUID(t, sha)` inline so the recovery is automatic. +- **Committed in:** `372bb15` (Task 2 GREEN commit) + +**2. [Rule 1 - Bug] Fixed test expectation length (28 vs 32 chars) in TestCommitUUIDInverse** +- **Found during:** Task 2 (initial test run after the GREEN commitUUIDInverse implementation) +- **Issue:** The plan's `want` values for 3 of the 4 success cases were 32 chars (the full SHA) but the function correctly returns 28 chars (the first 14 bytes, the recoverable portion). The plan author's hand-derivation of "expected prefix" was off — `hex(sha[0:14])` is 28 chars, not the full 40/64. +- **Fix:** Trimmed the 3 affected `want` values to 28 chars: `0000000000000000000000000000` (28 zeros), `ffffffffffffffffffffffffffff` (28 f's), `0123456789abcdef0123456789ab` (28 hex chars). The function output is unchanged. +- **Files modified:** `internal/connect/commits_helpers_test.go` +- **Verification:** `TestCommitUUIDInverse` passes all 8 subtests; `len(got) == 28` for the 4 success cases. +- **Committed in:** `372bb15` (Task 2 GREEN commit) + +**3. [Rule 1 - Bug] Updated existing TestProbeCommitID_* tests for the new isSHA gate** +- **Found during:** Task 3 (full test suite run after probe rewrite) +- **Issue:** The existing `TestProbeCommitID_MissNegativeCaches` and `TestProbeCommitID_TransientNotNegativeCached` used short placeholder commits like `"bogus000"` (8 chars) and `"real"` (4 chars) which now bypass the probe entirely via the new `!isSHA(id)` early-return branch (negative-cached before any GetMeta call). The test assertions (1 GetMeta call, retry after transient) no longer held. +- **Fix:** Updated both tests to use 40-char lowercase hex strings (`aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa` for the source's owned commit, `bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb` for the bogus test input). The transient-error test now uses the same 40-char shape. The test semantics (negative-cache, transient non-caching) are preserved. +- **Files modified:** `internal/connect/api_test.go` +- **Verification:** Both tests pass; `calls.Load() == 1` on first miss, `== 1` on negative-cache hit; `calls.Load() == 2` after retry on transient. +- **Committed in:** `1d5fe33` (Task 3 commit) + +**4. [Rule 2 - Missing Critical] Updated mockSource.GetMeta to accept a SHA prefix** +- **Found during:** Task 3 (RED test for TestServeDownload_AfterRestart_ProbeResolvesUUID) +- **Issue:** The existing `mockSource.GetMeta` returns an error if `commit != "" && commit != s.commit`. With the new probe path, `commit` will be a 28-char SHA prefix (the `commitUUIDInverse` output), and the mock must accept a prefix match (`strings.HasPrefix(s.commit, commit)`) so the post-restart probe can resolve. Without this update, the test would fail (the probe would never see a hit), but production behavior is correct (real providers accept short SHAs). +- **Fix:** Added a `!strings.HasPrefix(s.commit, commit)` clause to the `mockSource.GetMeta` mismatch check. The mock now accepts HEAD, exact SHA match, and SHA prefix match. +- **Files modified:** `internal/connect/api_test.go` +- **Verification:** `TestServeDownload_AfterRestart_ProbeResolvesUUID` passes; the probe fans out to the source with the 28-char prefix, the source returns the full SHA, the prefix-match validation succeeds. +- **Committed in:** `1d5fe33` (Task 3 commit) + +--- + +**Total deviations:** 4 auto-fixed (3 bug fixes, 1 missing critical test infrastructure) +**Impact on plan:** All auto-fixes were test-data or test-infrastructure corrections that did not change the production code shape. The plan's structural intent (end-to-end ref support, isSHA gate, prewarm removal, commitUUIDInverse-based probe) is preserved exactly. + +## Issues Encountered + +- The plan's hand-derived test fixtures for `commitUUIDInverse` (the `want` values) were 32 chars but the function correctly returns 28 chars. This is a 4-char off-by-one in the plan's expected output. The implementation is correct; the test expectations were fixed. Documented as deviation #2. +- The plan's 64-char SHA fixture had 66 chars (off-by-two). The implementation rejected it with a clear "input is not 40 or 64 lowercase hex characters" error message; truncated to 64 chars and re-ran. Documented as deviation #1. +- The prewarm removal means `TestPrewarmHeads_PopulatesCommitMap` (Phase 16) no longer applies — the test is removed as part of the deletion of `prewarmHeads`. Not a deviation; the plan explicitly noted this in Step A. + +## Next Phase Readiness + +- Phase 18 deliverables complete; all 5 ROADMAP success criteria met. +- The proxy is now ref-aware: a `buf.yaml` with `cyp/cyp-net-listeners:main/v2` returns the SHA at `main/v2`, not HEAD. +- The prewarm removal is a behavioral change observable to the client: the first `Download` after a process restart now incurs one upstream round-trip per source (the probe fan-out) instead of zero (prewarm). Operators should be aware of this latency shift; the `probeSem` cap and `probeTimeout` bound the worst case. +- No new dependencies, no go.mod / go.sum changes. +- All Phase 16/17 regression tests still pass. + +## Self-Check: PASSED + +- All commits exist (`git log --oneline | grep -E 'b16fd2d|6a49450|6fc909d|372bb15|1d5fe33'` — 5 commits) +- All created files exist on disk (3 new test files) +- All modified files exist on disk (8 modified files) +- `go build ./...` exits 0 +- `go vet ./...` exits 0 +- All 3 test packages pass: `go test ./internal/connect/`, `./internal/providers/bitbucket/`, `./internal/providers/github/` — all with `-count=1` +- Targeted tests pass: `TestCommitUUIDInverse|TestIsSHA|TestIsUUID|TestParseResourceRefName|TestServeDownload_AfterRestart` +- Phase 16/17 regression guards pass +- All structural grep checks pass (prewarmHeads, registerResolved$ — 0 hits; commitUUIDInverse, registerResolvedAlias, prefix-match validation — present; isSHA, isUUID, num==3, GetMeta(ref.ref) — correct hit counts; no Prewarm in main.go/config.go) + +--- +*Phase: 18-respect-buf-yaml-dependency-refs-not-always-head-fix-related* +*Completed: 2026-07-07* diff --git a/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-RESEARCH.md b/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-RESEARCH.md new file mode 100644 index 0000000..f302603 --- /dev/null +++ b/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-RESEARCH.md @@ -0,0 +1,151 @@ +# Phase 18: Respect buf.yaml dep refs, fix related bug, drop prewarm - Research + +**Researched:** 2026-07-07 +**Domain:** Bugfix + small refactor in connect/ — three independent corrections bundled into one phase +**Confidence:** HIGH (all claims verifiable against the source files; no external library research needed) + +## Summary + +Phase 18 is a tightly-scoped bugfix that lands three corrections to how the proxy resolves a `buf.yaml` dependency: + +1. **Make the proxy return the ref the user wrote in `buf.yaml`, not always HEAD.** Today the proxy ignores the `ref` field on the `Name` message (commits_helpers.go:108-136 parses only `owner` and `module`) and the bitbucket/github providers treat any non-`""`/non-`"main"` input as a raw SHA (bitbucket/getrepo.go:18-20, github/getrepo.go:21-23). A dependency like `buf-proxy.yadro.dev/cyp/cyp-net-listeners:main/v2` therefore either collapses to HEAD (ref ignored) or 500s (ref treated as raw SHA, fails hex decode / BitBucket API). +2. **Fix the related bug.** The same `commit != "" && commit != "main"` short-circuit in both providers means that ANY non-`""`/non-`"main"` input — including a properly-resolved ref — gets stamped into `meta.Commit` as a literal string. The fix is a single check: if the input is not 40 or 64 lowercase hex, treat it as a ref and resolve it to a SHA via the provider's commit-fetch API (Bitbucket `/commits/{id}`, GitHub `GET /repos/{o}/{r}/commits/{sha}` — both endpoints accept ref names and short SHAs in addition to full SHAs). +3. **Remove the prewarm logic.** With the new inverse function (`commitUUIDInverse` — recovers the 28-char SHA prefix from a 32-char buf UUID), `probeCommitID` can use the UUID → SHA-prefix derivation to identify the right source on a Download cache-miss without the startup fan-out. The prewarm at [commits.go:968-999](internal/connect/commits.go#L968-L999) and its 3 supporting helpers (the goroutine launch in api.go:111-113, the `prewarmEnabled` / `prewarmTimeout` / `prewarmOnce` fields, the `PrewarmConfig` block in config.go) all go away. + +**Primary recommendation:** One plan with three tasks. The three tasks share the `commitUUID` module (task 1 needs the inverse for ref-resolution sanity checks; task 2 adds the inverse; task 3 uses the inverse to replace prewarm in the probe path). Splitting them into separate plans would require shipping task 1 with a hand-rolled inverse or duplicating the SHA-prefix logic across files. + +## Key Code Facts (verified against source) + +### 1. The ref-parsing gap (commits_helpers.go:108-136) + +```go +func parseResourceRefName(msg []byte) *moduleRef { + var owner, module string + for len(msg) > 0 { + num, typ, n := protowire.ConsumeTag(msg) + ... + if num == 1 && typ == protowire.BytesType { owner = string(v) } + else if num == 2 && typ == protowire.BytesType { module = string(v) } + else { n = protowire.ConsumeFieldValue(num, typ, msg) ... } + } + ... +} +``` + +The `Name` proto in buf v1/v1beta1 has `owner=1, module=2, ref=3`. The proxy throws `ref` away. To preserve byte-for-byte the current parsing for `owner`+`module`, add a third `else if num == 3` arm that captures the ref. The `moduleRef` struct (commits_helpers.go:11-14) gains a `ref string` field. + +### 2. The provider short-circuit (bitbucket/getrepo.go:18-20, github/getrepo.go:21-23) + +```go +if commit != "" && commit != "main" { + meta.Commit = commit +} +``` + +The `commit != "main"` carve-out is the load-bearing bug. The intent was "main branch → HEAD", but the carve-out accidentally also excludes every other ref. A correct check is `isSHA(commit)` (40 or 64 lowercase hex). If the input is not a SHA and not empty, route to a ref-resolution API call. + +### 3. `commitUUID` and its inverse (commits_helpers.go:40-59) + +```go +func commitUUID(gitSHA string) (string, error) { + if len(gitSHA) != 40 && len(gitSHA) != 64 { ... } + sha, _ := hex.DecodeString(gitSHA) + var result [16]byte + copy(result[0:6], sha[0:6]) + result[6] = 0x40 // version-4 nibble (overwritten, drop) + result[7] = sha[6] // 7th input byte + result[8] = 0x80 // variant (overwritten, drop) + copy(result[9:16], sha[7:14]) // bytes 7..13 of input + return hex.EncodeToString(result[:]), nil +} +``` + +The inverse is straightforward: decode the 32-char UUID as 16 bytes, drop bytes 6 and 8 (they are version/variant), concatenate the remaining 14 bytes as the SHA prefix. The inverse function is lossy by design (review.md "14-of-20-byte collision surface" note at the end). The function does NOT need to know the input length — the function only emits the first 28 hex chars of the SHA. + +```go +func commitUUIDInverse(uuid string) (string, error) { + if len(uuid) != 32 { return "", errors.New("commitUUIDInverse: input is not 32 lowercase hex characters") } + u, _ := hex.DecodeString(uuid) + var sha [20]byte + copy(sha[0:6], u[0:6]) + sha[6] = u[7] + copy(sha[7:14], u[9:16]) + return hex.EncodeToString(sha[:14]), nil // 28 chars, the recoverable SHA prefix +} +``` + +### 4. The prewarm logic (commits.go:961-1000, 919-959, 1101-1103, 919-959) + +Three pieces: + +- `prewarmHeads` ([commits.go:968-999](internal/connect/commits.go#L968-L999)) — iterates all configured sources at startup, calls `s.GetMeta(ctx, "")` (HEAD), and registers the result. +- `registerResolved` ([commits.go:919-959](internal/connect/commits.go#L919-L959)) — writes the prewarm result into `commitMap` and `infoCache` under both the raw SHA and the UUID. +- The launch in [api.go:111-113](internal/connect/api.go#L111-L113) — `go commitHandler.prewarmHeads()`. + +Plus the related fields on `commitServiceHandler` (commits.go:52-69): `prewarmOnce`, `prewarmEnabled`, `prewarmTimeout`, the `CommitResolution` struct fields in [api.go:30-36](internal/connect/api.go#L30-L36), the `PrewarmConfig` in [config.go:96-101](cmd/easyp/internal/config/config.go#L96-L101), and the `WithDefaults` block at config.go:118-128. + +The prewarm's only practical effect is to make the FIRST Download of a fresh buf client (one whose `buf.lock` pins the HEAD UUID) hit `commitMap` after a process restart. With the inverse function, the existing `probeCommitID` ([commits.go:1056-1137](internal/connect/commits.go#L1056-L1137)) can recover the same behavior on first request by deriving the SHA prefix from the UUID and probing each source with the prefix. The probe already handles the "first request after restart" case for raw-SHA requests; extending it to handle UUID-requests is a one-function-call change. + +### 5. The probeCommitID bug (commits.go:1106, review.md finding #6) + +```go +meta, err := s.GetMeta(pctx, sha) // sha is a 32-char UUID here +if err != nil { ... } +if meta.Commit == "" { return } +results <- probeResult{...} // <-- reports a hit regardless of what meta.Commit is +``` + +If `sha` is a 32-char UUID, the provider (with the new `isSHA(commit)` gate from task 1) returns 500 (input is neither a SHA, nor empty, nor "main"; the ref-resolution path can't resolve a UUID as a ref). The probe today calls the OLD short-circuit, which overwrites `meta.Commit` with the UUID literal, then `meta.Commit != ""` is true, and the probe claims a hit. With task 1's `isSHA` check, the provider returns an error, the probe correctly misses, and the next refactor (task 3) can route the UUID through the inverse function. + +This means task 1 ALSO fixes finding #6 from review.md ("probeCommitID reports a hit while registerResolved silently no-ops") as a side effect — task 3 then reworks the probe to use the inverse for legitimate UUID inputs. + +## Scope Fence + +In scope: + +- `parseResourceRefName` learns to read `ref` (commits_helpers.go) +- `moduleRef` struct gets a `ref` field +- `parseResourceRefs` / `parseGetGraphResourceRefs` / `parseGetGraphResourceRefsV1` plumb the ref through +- `ServeHTTP` and `ServeGraph` pass `ref.ref` (or empty for HEAD) into `GetMeta` +- Bitbucket provider: new `isSHA(commit)` gate; if not SHA, call `/commits/{commit}` to resolve to SHA +- GitHub provider: same `isSHA` gate; if not SHA, call `repos.GetCommit(ctx, owner, repo, commit, ...)` to resolve +- New `commitUUIDInverse(uuid) (string, error)` helper in commits_helpers.go +- New `TestCommitUUIDInverse` table-driven test +- Update `probeCommitID` to use the inverse when the input is a 32-char UUID +- Delete `prewarmHeads`, `registerResolved`, the related fields, the launch, the `PrewarmConfig` block +- Update `main.go` (cmd/easyp/main.go:48-65) to drop the prewarm args to `connect.NewWithConfig` +- Update `CommitResolution` struct (api.go:30-36) to remove `PrewarmEnabled` / `PrewarmTimeout` + +Out of scope: + +- Changing the `commitUUID` byte-table (Phase 17 already finalized it) +- Changing `commitMap` keys (still the buf-issued UUID for client-facing lookups, the raw SHA for upstream lookups) +- Adding new public RPCs +- Renaming any existing function + +## Risks + +- **R1:** The ref-resolution API call (Bitbucket `/commits/{ref}`) is one extra round-trip per ref-resolved request. For HEAD-only requests (the common case), the new path is a no-op: `commit == ""` still skips resolution. The cost is only paid for non-empty, non-SHA inputs. +- **R2:** The `commitUUIDInverse` is lossy: 2^112 collision space. A probe that recovers the wrong source for a UUID that collides under the first 14 bytes will register a wrong moduleRef. The `meta.Commit` SHA returned by the source MUST be validated to start with the recovered prefix before `registerResolved` is called. If the prefix doesn't match, the probe falls through to the "all-fail" branch and the request 400s. +- **R3:** Removing prewarm means a process restart is observable to the client. Specifically: a `buf build` immediately after a proxy restart will incur one extra upstream round-trip per module (the probe fan-out). The `probeSem` cap ([commits.go:75](internal/connect/commits.go#L75)) and the per-source `probeTimeout` ([commits.go:75](internal/connect/commits.go#L75)) bound the worst case at `maxConcurrentProbes × probeTimeout` per fan-out. For a 5-source deployment at 8s per source, that's 40s in the absolute worst case, but in practice the fan-out is parallel and completes in < 8s. +- **R4:** The Bitbucket provider does not currently have a `getCommit` helper. Task 1 has to add one. It should mirror the `getRepo` shape (HTTP GET, JSON decode into a struct, return a typed `content.Meta`-like value). The struct needs only one field: `id` (the resolved SHA). +- **R5:** The GitHub provider's `repos.GetCommit` returns a `*github.RepositoryCommit` whose `GetSHA()` is the resolved commit. The mock provider in tests (`internal/connect/api_test.go`) will need to be extended to simulate a ref-resolution response (returning a `*github.RepositoryCommit` with the resolved SHA). + +## Success Criteria (from user) + +The 3 user-supplied items are the success criteria. The 5 ROADMAP success criteria are TBD (the phase was just created); this plan will fill in: + +- **SC-1 (refs respected):** `GetCommits` and `GetGraph` for a `Name` with `ref="main/v2"` returns a UUID derived from the SHA at `main/v2`, not HEAD. Verified by an integration test that registers a bitbucket mock source, sets `meta.Commit` for a ref, and asserts the returned UUID matches `commitUUID(meta.Commit)`. +- **SC-2 (related bug fixed):** The `commit != "" && commit != "main"` short-circuit in both providers is replaced with `isSHA(commit) && (len(commit) == 40 || len(commit) == 64)`. A ref input goes through the resolution path. A SHA input goes through the existing fast path. Verified by per-provider unit tests in `getrepo_test.go` (new) for the bitbucket provider, and an extension to the github mock tests. +- **SC-3 (prewarm removed):** `prewarmHeads`, `registerResolved`, the `prewarmEnabled` / `prewarmTimeout` / `prewarmOnce` fields, the goroutine launch in api.go:111-113, the `PrewarmConfig` block in config.go:96-101, the `PrewarmEnabled` / `PrewarmTimeout` fields in `CommitResolution`, and the corresponding `WithDefaults` entries are all gone. `go build ./...` is clean. A new test `TestServeDownload_AfterRestart_ProbeResolvesUUID` simulates a proxy restart (empty commitMap) and a Download with a UUID, asserts the probe-derived SHA prefix recovers the right module. +- **SC-4 (inverse helper):** `commitUUIDInverse(uuid)` exists in `commits_helpers.go`, returns the 28-char SHA prefix, errors on non-32-char or non-hex input. `TestCommitUUIDInverse` covers 4 cases (round-trip with a known 40-char SHA, all-zero, all-ones, error on 31-char and 33-char inputs). +- **SC-5 (no regression):** All existing tests pass, including the SHA-256 path from Phase 17, the probe path from Phase 16, and the buf v1.69.0 UUID format from Phase 16. + +## Recommended Plan + +One plan, three tasks, two waves: + +- **Wave 1:** Tasks 1 + 2 (independent — ref support and inverse helper) +- **Wave 2:** Task 3 (prewarm removal — uses inverse from task 2) + +Each task is a separate `` block in the single plan. The plan is committed in one go; the executor can run tasks in the wave order. diff --git a/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-VALIDATION.md b/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-VALIDATION.md new file mode 100644 index 0000000..5b78b83 --- /dev/null +++ b/.planning/phases/18-respect-buf-yaml-dependency-refs-not-always-head-fix-related/18-VALIDATION.md @@ -0,0 +1,139 @@ +--- +phase: 18 +slug: respect-buf-yaml-dependency-refs-not-always-head-fix-related +status: draft +nyquist_compliant: false +wave_0_complete: false +created: 2026-07-07 +--- + +# Phase 18 — Validation Strategy + +> Per-phase validation contract for feedback sampling during execution. +> Phase 18 is a tightly-scoped bugfix + refactor: (1) honor `Name.ref` end-to-end, (2) replace the `commit != "" && commit != "main"` short-circuit in both providers with an `isSHA` gate, (3) add a `commitUUIDInverse` helper, (4) drop the prewarm startup fan-out. All changes are local to `internal/connect/`, `internal/providers/{bitbucket,github}/`, and `cmd/easyp/`. Verification is the existing `go test ./...` suite plus the new tests added in each task and 15 structural grep checks. + +--- + +## Test Infrastructure + +| Property | Value | +|----------|-------| +| **Framework** | Go standard `testing` (packages `connect`, `bitbucket`, `github`) | +| **Config file** | None — Go test discovery (`*_test.go` in package) | +| **Quick run command** | `go test ./internal/connect/ -count=1` | +| **Full suite command** | `go build ./... && go vet ./... && go test ./internal/connect/ -count=1 && go test ./internal/providers/bitbucket/ -count=1 && go test ./internal/providers/github/ -count=1` | +| **Estimated runtime** | ~5 seconds (existing suite is ~3s; new tests add <2s) | + +--- + +## Sampling Rate + +- **After every task commit:** Run `go test ./internal/connect/ -count=1` (full connect-package suite, ~3s) +- **After Task 1 commit (ref parsing + provider ref-resolution):** Run `go test ./internal/connect/ -run 'TestIsSHA|TestIsUUID|TestParseResourceRefName' -v -count=1` AND `go test ./internal/providers/bitbucket/ -run 'TestGetMeta' -v -count=1` AND `go test ./internal/providers/github/ -run 'TestGetMeta' -v -count=1` to confirm the new tests pass +- **After Task 2 commit (inverse helper):** Run `go test ./internal/connect/ -run 'TestCommitUUIDInverse' -v -count=1` to confirm the 8 subtests pass; also run `go test ./internal/connect/ -run 'TestCommitUUID' -v -count=1` to confirm no regression in the existing `commitUUID` tests +- **After Task 3 commit (prewarm removal + probe rewrite):** Run `go test ./internal/connect/ -run 'TestServeDownload_AfterRestart' -v -count=1` to confirm the new integration test passes; also run `go build ./...` to confirm `cmd/easyp/main.go` and `cmd/easyp/internal/config/config.go` are still clean after the prewarm removal +- **After all 3 tasks complete:** Run `go build ./... && go vet ./... && go test ./internal/connect/ -count=1 && go test ./internal/providers/bitbucket/ -count=1 && go test ./internal/providers/github/ -count=1` (full suite, ~5s) + the 15 structural grep checks +- **Max feedback latency:** ~5 seconds (one full suite run) + +--- + +## Per-Task Verification Map + +### Task 1 — Refs respected + provider ref-resolution + +| Test | What it proves | +|------|----------------| +| `TestIsSHA` (6 subtests) | `isSHA` correctly accepts 40/64-char hex, rejects everything else | +| `TestIsUUID` (4 subtests) | `isUUID` correctly accepts 32-char hex, rejects everything else | +| `TestParseResourceRefName_ReadsRef` | proto field 3 (`ref`) is captured into `moduleRef.ref` | +| `TestParseResourceRefName_NoRef` | missing ref field is tolerated (old clients still work) | +| `bitbucket.TestGetMeta_Empty` | empty input still returns HEAD (regression guard) | +| `bitbucket.TestGetMeta_RawSHA_40` | 40-char SHA still bypasses ref-resolution (regression guard) | +| `bitbucket.TestGetMeta_RawSHA_64` | 64-char SHA still bypasses ref-resolution (regression guard) | +| `bitbucket.TestGetMeta_ResolvesRef` | non-SHA input routes to `/commits/{ref}` and gets the resolved SHA | +| `github.TestGetMeta_Empty` | empty input still returns HEAD (regression guard) | +| `github.TestGetMeta_RawSHA_40` | 40-char SHA still bypasses ref-resolution (regression guard) | +| `github.TestGetMeta_ResolvesRef` | non-SHA input routes to `repos.GetCommit` and gets the resolved SHA | + +### Task 2 — `commitUUIDInverse` helper + +| Test | What it proves | +|------|----------------| +| `TestCommitUUIDInverse` (8 subtests) | round-trip with 40-char and 64-char SHAs; error on empty, length-mismatched, and non-hex inputs | + +### Task 3 — Prewarm removal + probe rewrite + +| Test | What it proves | +|------|----------------| +| `TestServeDownload_AfterRestart_ProbeResolvesUUID` | a fresh handler with empty `commitMap` correctly recovers the module from a UUID via the probe path | +| `TestServeDownload_AfterRestart_ProbeMissesOnUnknownUUID` | a UUID whose first 14 bytes do not correspond to any source's SHA is correctly treated as a miss (400) | + +### Phase 16/17 Regression Guards + +| Test | What it proves | +|------|----------------| +| `TestServeHTTP_GetCommits_ReturnsDashlessUUID` | buf v1.69.0 wire format still works | +| `TestServeDownload_RoundTripWithMintedUUID` | UUID-pinned Download still works | +| `TestServeDownload_UnknownCommitID_ReturnsBadRequest` | 400 path still works | +| `TestCommitUUID_KnownSHA` | `commitUUID` byte-table still produces expected UUIDs | +| `TestCommitUUID_SHA256_KnownSHA` | 64-char SHA-256 inputs still work | +| `TestCommitUUID_InvalidInput` | 40/64-char gate still rejects bad inputs | + +--- + +## Structural Grep Checks (15 checks) + +All must pass at the end of the phase: + +| Check | Expected | What it proves | +|-------|----------|----------------| +| `grep -rn 'prewarmHeads' .` | 0 hits | prewarmHeads is gone | +| `grep -rn 'registerResolved$' .` | 0 hits | old registerResolved is gone | +| `grep -rn 'PrewarmEnabled\|prewarmEnabled\|PrewarmConfig\|prewarmOnce\|prewarmTimeout' .` | 0 hits | all prewarm config is gone | +| `grep -c 'func commitUUIDInverse' internal/connect/commits_helpers.go` | 1 | inverse helper exists | +| `grep -c 'func (h \*commitServiceHandler) registerResolvedAlias' internal/connect/commits.go` | 1 | new alias helper exists | +| `grep -c 'commitUUIDInverse' internal/connect/commits.go` | >= 1 | probe uses the inverse | +| `grep -c 'strings.HasPrefix(meta.Commit, probeArg)' internal/connect/commits.go` | 1 | prefix-match validation exists | +| `grep -c 'isSHA(commit)' internal/providers/bitbucket/getrepo.go` | >= 1 | bitbucket uses the gate | +| `grep -c 'isSHA(commit)' internal/providers/github/getrepo.go` | >= 1 | github uses the gate | +| `grep -c 'isUUID' internal/connect/commits.go` | >= 2 | probe uses the UUID check | +| `grep -c 'num == 3 && typ == protowire.BytesType' internal/connect/commits_helpers.go` | 1 | ref field is parsed | +| `grep -c 'GetMeta(r.Context(), ref.owner, ref.module, ref.ref)' internal/connect/commits.go` | 2 | ServeHTTP and ServeGraph pass the ref | +| `grep -c 'Prewarm' cmd/easyp/main.go` | 0 | prewarm gone from main.go | +| `grep -c 'Prewarm' cmd/easyp/internal/config/config.go` | 0 | prewarm gone from config.go | +| `grep -c 'commit != "" && commit != "main"' internal/providers/{bitbucket,github}/getrepo.go` | 0 each | old short-circuit gone | + +--- + +## Coverage Matrix (Dimension 8: Validation Coverage) + +| SC | What it claims | What test proves it | What prevents silent breakage | +|----|----------------|---------------------|-------------------------------| +| SC-1 (refs respected) | `GetCommits` returns UUID from ref | `TestParseResourceRefName_ReadsRef` + `TestGetMeta_ResolvesRef` (both providers) | The provider mock returns a different SHA for HEAD vs the ref input; the test asserts the UUID matches the ref-resolved SHA, not HEAD | +| SC-2 (related bug fixed) | non-SHA inputs route through ref-resolution | `TestGetMeta_ResolvesRef` (both providers) | The provider mock returns a 404 for non-SHA inputs that don't match a real ref; the test asserts the function returns an error, not a silent fallthrough to HEAD | +| SC-3 (prewarm removed) | `prewarmHeads` / `registerResolved` are gone | Structural grep + `go build ./...` clean | The 7 structural grep checks (`prewarmHeads`, `registerResolved`, `PrewarmConfig`, etc.) must all return 0 hits; `go build ./...` is the final compile-time guard | +| SC-4 (inverse helper) | `commitUUIDInverse` recovers the 28-char SHA prefix | `TestCommitUUIDInverse` (8 subtests) | The 4 success subtests use real `commitUUID(sha)` calls (not hardcoded UUIDs) so a future change to `commitUUID`'s byte-table automatically propagates | +| SC-5 (no regression) | Phase 16/17 tests still pass | `go test ./internal/connect/ -count=1` + 6 targeted tests | The full connect-package suite is the regression guard; the 6 targeted tests are the named-property guards (UUID wire format, SHA-256 path, etc.) | + +--- + +## Known Gaps + +- The `mockProvider` in `api_test.go` is constructed at test setup with a single configured source. The new `TestServeDownload_AfterRestart_*` tests rely on this single-source shape. A multi-source test (verifying that the probe correctly identifies the right source among several) is a future phase, not in this one. +- The `isSHA` / `isUUID` helpers are simple linear scans. For a 64-char SHA-256 input, this is 64 character comparisons. A future optimization could use a precomputed lookup table, but the linear scan is fast enough for the proxy's request rate (~hundreds of requests per second at most). +- The Bitbucket `/commits/{ref}` endpoint is documented to accept short SHAs (>= 7 chars) in addition to full SHAs and ref names. The new `getCommit` helper does not test the short-SHA case explicitly (the existing `TestGetMeta_RawSHA_40` covers the full-SHA case). A future test could add a 7-char short-SHA case. + +--- + +## Rollback Plan + +If the prewarm removal causes a regression in production, the rollback is: + +1. Revert the commit that deletes `prewarmHeads` and `registerResolved`. +2. Revert the `probeCommitID` rewrite to its pre-task-3 shape. +3. Re-add the `PrewarmConfig` block in `config.go`. +4. Re-add the `PrewarmEnabled` / `PrewarmTimeout` fields in `CommitResolution` and the goroutine launch in `api.go:111-113`. + +The prewarm removal is a single atomic commit (Task 3 is one `` block, but it can be split into Task 3a "probe rewrite" and Task 3b "prewarm deletion" if a more granular rollback is desired — for the initial plan, the atomic version is fine). + +The ref-respect changes (Task 1) and the inverse helper (Task 2) are independent of the prewarm removal. If only those need to be rolled back, the rollback is the inverse of Task 1 + Task 2. diff --git a/cmd/easyp/internal/config/config.go b/cmd/easyp/internal/config/config.go index fe9c8fd..8fa5584 100644 --- a/cmd/easyp/internal/config/config.go +++ b/cmd/easyp/internal/config/config.go @@ -85,19 +85,7 @@ type Artifactory struct { // internal/connect. All fields default sensibly (WithDefaults) so the proxy // behaves as intended with no connect: block in the config file. type Connect struct { - Prewarm PrewarmConfig `json:"prewarm"` - Probe ProbeConfig `json:"probe"` -} - -// PrewarmConfig controls startup HEAD pre-warming: the proxy resolves the -// current HEAD commit of every configured module at startup so that clients -// caching a current HEAD sha hit the commit map without a prior in-session -// GetCommits. -type PrewarmConfig struct { - // Enabled is a pointer so we can distinguish "unset" (default true) from - // an explicit false. Set enabled: false to disable. - Enabled *bool `json:"enabled"` - PerCallTimeout time.Duration `json:"per_call_timeout"` + Probe ProbeConfig `json:"probe"` } // ProbeConfig controls the upstream sha probe used on a Download cache miss: @@ -117,13 +105,6 @@ type ProbeConfig struct { // is loaded. func (c Connect) WithDefaults() Connect { out := c - if out.Prewarm.Enabled == nil { - t := true - out.Prewarm.Enabled = &t - } - if out.Prewarm.PerCallTimeout == 0 { - out.Prewarm.PerCallTimeout = 10 * time.Second - } if out.Probe.Enabled == nil { t := true out.Probe.Enabled = &t diff --git a/cmd/easyp/main.go b/cmd/easyp/main.go index 75fdb79..e231438 100644 --- a/cmd/easyp/main.go +++ b/cmd/easyp/main.go @@ -56,8 +56,6 @@ func main() { handler = func() *http.ServeMux { cc := cfg.Connect.WithDefaults() return connect.NewWithConfig(log, storage, cfg.Domain, connect.CommitResolution{ - PrewarmEnabled: cc.Prewarm.Enabled != nil && *cc.Prewarm.Enabled, - PrewarmTimeout: cc.Prewarm.PerCallTimeout, ProbeEnabled: cc.Probe.Enabled != nil && *cc.Probe.Enabled, ProbeNegativeTTL: cc.Probe.NegativeTTL, ProbeTimeout: cc.Probe.PerCallTimeout, diff --git a/internal/connect/api.go b/internal/connect/api.go index e7113e2..4d05745 100644 --- a/internal/connect/api.go +++ b/internal/connect/api.go @@ -21,15 +21,13 @@ type provider interface { } // CommitResolution configures the buf v1 commit-id resolution enhancements in -// commitServiceHandler: startup HEAD pre-warm and the upstream sha probe used -// on a Download cache miss. It is the connect-package mirror of the user-facing -// connect config — kept here (not imported from cmd/easyp/internal/config) to -// avoid an internal→cmd layering violation. A zero value disables both -// enhancements (the historical behavior), so callers that construct the mux -// via New (tests) are unaffected; production threads it via NewWithConfig. +// commitServiceHandler: the upstream sha probe used on a Download cache miss. +// It is the connect-package mirror of the user-facing connect config — kept +// here (not imported from cmd/easyp/internal/config) to avoid an +// internal→cmd layering violation. A zero value disables the enhancement +// (the historical behavior), so callers that construct the mux via New +// (tests) are unaffected; production threads it via NewWithConfig. type CommitResolution struct { - PrewarmEnabled bool - PrewarmTimeout time.Duration ProbeEnabled bool ProbeNegativeTTL time.Duration ProbeTimeout time.Duration @@ -61,8 +59,7 @@ func New( } // NewWithConfig is like New but enables commit-resolution enhancements per cfg. -// PrewarmEnabled launches a best-effort background HEAD sweep; ProbeEnabled -// turns on the upstream sha probe for Download cache misses. +// ProbeEnabled turns on the upstream sha probe for Download cache misses. func NewWithConfig( log *slog.Logger, core provider, @@ -93,23 +90,17 @@ func NewWithConfig( knownOwners := buildKnownOwners(core.Repositories()) singleModule := buildKnownModules(core.Repositories()) commitHandler := &commitServiceHandler{ - api: a, - commitMap: make(map[string]moduleRef), - infoCache: make(map[string]commitInfoCache), - filesMap: make(map[string][]content.File), - knownOwners: knownOwners, - singleModule: singleModule, - missCache: make(map[string]time.Time), - - prewarmEnabled: cfg.PrewarmEnabled, - prewarmTimeout: cfg.PrewarmTimeout, - probeEnabled: cfg.ProbeEnabled, + api: a, + commitMap: make(map[string]moduleRef), + infoCache: make(map[string]commitInfoCache), + filesMap: make(map[string][]content.File), + knownOwners: knownOwners, + singleModule: singleModule, + missCache: make(map[string]time.Time), + probeEnabled: cfg.ProbeEnabled, probeNegativeTTL: cfg.ProbeNegativeTTL, - probeTimeout: cfg.ProbeTimeout, - probeSem: make(chan struct{}, maxConcurrentProbes), - } - if commitHandler.prewarmEnabled && commitHandler.prewarmTimeout > 0 { - go commitHandler.prewarmHeads() + probeTimeout: cfg.ProbeTimeout, + probeSem: make(chan struct{}, maxConcurrentProbes), } if commitHandler.probeEnabled && commitHandler.probeNegativeTTL > 0 { go commitHandler.sweepMisses(context.Background()) diff --git a/internal/connect/api_test.go b/internal/connect/api_test.go index 5dcb43a..2b6d446 100644 --- a/internal/connect/api_test.go +++ b/internal/connect/api_test.go @@ -92,9 +92,11 @@ func (s *mockSource) GetMeta(_ context.Context, commit string) (content.Meta, er if s.getMetaErr != nil { return content.Meta{}, s.getMetaErr } - // HEAD (empty arg) resolves to the source's own commit; a specific commit - // resolves only if it is the one this source owns. - if commit != "" && commit != s.commit { + // HEAD (empty arg) resolves to the source's own commit; a specific + // commit resolves only if it is the one this source owns. A SHA + // prefix (a 28-char substring of s.commit) also resolves — used by + // the post-restart probe path (commitUUIDInverse + prefix-match). + if commit != "" && commit != s.commit && !strings.HasPrefix(s.commit, commit) { return content.Meta{}, fmt.Errorf("mock: commit %q not found in %s/%s", commit, s.owner, s.repoName) } return content.Meta{Commit: s.commit, DefaultBranch: "main"}, nil @@ -1341,9 +1343,8 @@ func TestOwnerServiceV1MethodNotAllowed(t *testing.T) { } // newTestCommitHandler builds a commitServiceHandler wired to a provider with -// minimal state, for direct unit testing of prewarmHeads / probeCommitID -// without the HTTP layer. Enhancements are off by default; tests flip the -// knobs they exercise. +// minimal state, for direct unit testing of probeCommitID without the HTTP +// layer. Enhancements are off by default; tests flip the knobs they exercise. func newTestCommitHandler(repo provider) *commitServiceHandler { return &commitServiceHandler{ //nolint:exhaustruct api: &api{ //nolint:exhaustruct @@ -1354,43 +1355,12 @@ func newTestCommitHandler(repo provider) *commitServiceHandler { infoCache: make(map[string]commitInfoCache), filesMap: make(map[string][]content.File), missCache: make(map[string]time.Time), - prewarmTimeout: time.Second, probeTimeout: time.Second, probeNegativeTTL: time.Minute, probeSem: make(chan struct{}, maxConcurrentProbes), } } -// TestPrewarmHeads_PopulatesCommitMap verifies the startup sweep resolves the -// HEAD commit of each configured source and registers it so a later Download -// of that sha hits without a prior in-session GetCommits. -func TestPrewarmHeads_PopulatesCommitMap(t *testing.T) { - repo := &mockProvider{repos: []source.Source{ - &mockSource{owner: "googleapis", repoName: "googleapis", commit: "aaa1110000000000000000000000000000000000"}, - &mockSource{owner: "cyp", repoName: "cyp-logger", commit: "bbb2220000000000000000000000000000000000"}, - }} - h := newTestCommitHandler(repo) - h.prewarmEnabled = true - - h.prewarmHeads() // synchronous (sync.Once guards the goroutine-launched path) - - h.commitMu.RLock() - ref, ok := h.commitMap["aaa1110000000000000000000000000000000000"] - h.commitMu.RUnlock() - if !ok || ref.owner != "googleapis" || ref.module != "googleapis" { - t.Fatalf("aaa111... not resolved to googleapis/googleapis; ok=%v ref=%+v", ok, ref) - } - h.commitMu.RLock() - _, ok = h.commitMap["bbb2220000000000000000000000000000000000"] - h.commitMu.RUnlock() - if !ok { - t.Fatal("bbb222... (cyp/cyp-logger HEAD) not pre-warmed") - } - - // Idempotent: a second run must not panic or duplicate work. - h.prewarmHeads() -} - // TestProbeCommitID_HitResolvesAndCaches verifies that a sha owned by exactly // one source is resolved by the probe and registered as an alias so later // requests for it hit the commit map directly. @@ -1421,25 +1391,29 @@ func TestProbeCommitID_HitResolvesAndCaches(t *testing.T) { // re-probe (no extra upstream GetMeta calls). func TestProbeCommitID_MissNegativeCaches(t *testing.T) { var calls atomic.Int32 + // Use a 40-char SHA that does NOT match the source's commit. The + // mockSource's GetMeta returns the error "not found in cyp/cyp-apis" + // for any commit arg that is neither s.commit nor a prefix of + // s.commit, so the probe sees a real fan-out miss. repo := &mockProvider{repos: []source.Source{ - &mockSource{owner: "cyp", repoName: "cyp-apis", commit: "real", getMetaCalls: &calls}, + &mockSource{owner: "cyp", repoName: "cyp-apis", commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", getMetaCalls: &calls}, }} h := newTestCommitHandler(repo) h.probeEnabled = true - ref, ok := h.probeCommitID(context.Background(), "bogus000") + ref, ok := h.probeCommitID(context.Background(), "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb") if ok || ref != nil { t.Fatalf("bogus sha should miss; got ok=%v ref=%+v", ok, ref) } if first := calls.Load(); first != 1 { t.Fatalf("probe should issue exactly 1 GetMeta call on miss, got %d", first) } - if !h.missCached("bogus000") { + if !h.missCached("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb") { t.Fatal("bogus sha not negative-cached after miss") } // Second call within TTL: must be served from the negative cache. - ref2, ok2 := h.probeCommitID(context.Background(), "bogus000") + ref2, ok2 := h.probeCommitID(context.Background(), "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb") if ok2 || ref2 != nil { t.Fatalf("negative-cached sha should still miss; got ok=%v ref=%+v", ok2, ref2) } @@ -1456,22 +1430,22 @@ func TestProbeCommitID_TransientNotNegativeCached(t *testing.T) { var calls atomic.Int32 repo := &mockProvider{repos: []source.Source{ // Even the owning source errors transiently (simulates upstream outage). - &mockSource{owner: "cyp", repoName: "cyp-apis", commit: "real", getMetaErr: context.DeadlineExceeded, getMetaCalls: &calls}, + &mockSource{owner: "cyp", repoName: "cyp-apis", commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", getMetaErr: context.DeadlineExceeded, getMetaCalls: &calls}, }} h := newTestCommitHandler(repo) h.probeEnabled = true - if ref, ok := h.probeCommitID(context.Background(), "real"); ok || ref != nil { + if ref, ok := h.probeCommitID(context.Background(), "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); ok || ref != nil { t.Fatalf("transient failure should be a miss; got ok=%v ref=%+v", ok, ref) } - if h.missCached("real") { + if h.missCached("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") { t.Fatal("transient miss must not be negative-cached") } if calls.Load() != 1 { t.Fatalf("expected 1 GetMeta call, got %d", calls.Load()) } // Retry: not negative-cached, so it probes again. - if _, ok := h.probeCommitID(context.Background(), "real"); ok { + if _, ok := h.probeCommitID(context.Background(), "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); ok { t.Fatal("retry should still miss") } if calls.Load() != 2 { @@ -1536,3 +1510,132 @@ func TestServeDownload_NonHeadShaServesThatCommit(t *testing.T) { t.Errorf("response must NOT serve HEAD's content; got %x", respBody) } } + +// TestServeDownload_AfterRestart_ProbeResolvesUUID pins the post-restart +// probe path: with an empty commitMap (no in-session GetCommits has run), +// a Download request with a buf-issued 32-char UUID is resolved by +// probeCommitID via the commitUUIDInverse + prefix-match path. The +// probe derives the 28-char SHA prefix from the UUID, asks each +// configured source for the prefix, validates the returned SHA starts +// with the prefix, and registers the alias. This is the regression +// guard for the prewarm removal: without the inverse, the probe +// would 400 the request and a process restart would be observable +// to the client as a foreign-id failure. +func TestServeDownload_AfterRestart_ProbeResolvesUUID(t *testing.T) { + const headSha = "81353411f7b010d5b9ebeb1899066aac18a36701" + uuid, err := commitUUID(headSha) + if err != nil { + t.Fatalf("commitUUID(%q) unexpected error: %v", headSha, err) + } + prefix, err := commitUUIDInverse(uuid) + if err != nil { + t.Fatalf("commitUUIDInverse(%q) unexpected error: %v", uuid, err) + } + t.Logf("fixture: headSha=%s uuid=%s prefix=%s", headSha, uuid, prefix) + + var sourceCalls atomic.Int32 + // mockProvider is the connect-package provider. It serves the same + // head SHA for any commit arg the post-probe ServeDownload might + // try (the UUID, the prefix, the resolved SHA). Without byCommit + // set, it returns m.meta regardless of arg, mirroring the + // pre-restart production behavior of "HEAD lookup always returns + // the resolved commit". + repo := &mockProvider{ + meta: content.Meta{Commit: headSha, DefaultBranch: "main"}, + files: []content.File{ + {Path: "test.proto", Data: []byte("syntax = \"proto3\";"), Hash: shake256.Hash{}}, + }, + repos: []source.Source{ + &mockSource{ + owner: "cyp", repoName: "cyp-apis", + commit: headSha, + getMetaCalls: &sourceCalls, + }, + }, + } + h := newTestCommitHandler(repo) + h.probeEnabled = true + h.probeNegativeTTL = 5 * time.Minute + h.probeTimeout = 2 * time.Second + // commitMap is intentionally empty (post-restart state — no in-session GetCommits). + + mux := http.NewServeMux() + mux.HandleFunc("/buf.registry.module.v1.DownloadService/", h.ServeDownload) + mux.HandleFunc("/buf.registry.module.v1beta1.DownloadService/", h.ServeDownload) + server := httptest.NewServer(mux) + defer server.Close() + + for _, path := range []string{ + "/buf.registry.module.v1.DownloadService/Download", + "/buf.registry.module.v1beta1.DownloadService/Download", + } { + t.Run(path, func(t *testing.T) { + body := buildDownloadRequest(uuid) + resp, err := http.Post(server.URL+path, "application/proto", bytes.NewReader(body)) + if err != nil { + t.Fatalf("request failed: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + b, _ := io.ReadAll(resp.Body) + t.Fatalf("status = %d, want 200 (post-restart probe should resolve via inverse); body: %s", resp.StatusCode, b) + } + respBody, _ := io.ReadAll(resp.Body) + if len(respBody) == 0 { + t.Fatal("empty response body") + } + }) + } + if sourceCalls.Load() < 1 { + t.Errorf("expected at least 1 source GetMeta call (probe fan-out), got %d", sourceCalls.Load()) + } +} + +// TestServeDownload_AfterRestart_ProbeMissesOnUnknownUUID pins the +// safety net: when the 32-char UUID's first 14 bytes do NOT correspond +// to any configured source's SHA, the probe must miss (no false +// positive) and the request must 400. This is the prefix-match +// validation closing the review.md finding #6: a wrong-source match +// would silently alias a real commit id to a wrong module and serve +// wrong content. Failing closed is the right tradeoff. +func TestServeDownload_AfterRestart_ProbeMissesOnUnknownUUID(t *testing.T) { + // A 32-char UUID whose first 14 bytes are all-0xff — no configured + // source's HEAD SHA starts with "ff" * 14. The mockSource owns + // "0000000000000000000000000000000000000000" (starts with "00" * 14), + // so the prefix-match validation rejects the probe hit. + const ( + unknownUUID = "ffffffffffffffffffffffffffffffff" // first 14 bytes = "ff" * 14 + headSha = "0000000000000000000000000000000000000000" + ) + _ = unknownUUID // referenced below; declared here for clarity + + repo := &mockProvider{ + repos: []source.Source{ + &mockSource{owner: "cyp", repoName: "cyp-apis", commit: headSha}, + }, + } + h := newTestCommitHandler(repo) + h.probeEnabled = true + h.probeNegativeTTL = 5 * time.Minute + h.probeTimeout = 2 * time.Second + + mux := http.NewServeMux() + mux.HandleFunc("/buf.registry.module.v1beta1.DownloadService/", h.ServeDownload) + server := httptest.NewServer(mux) + defer server.Close() + + body := buildDownloadRequest(unknownUUID) + resp, err := http.Post( + server.URL+"/buf.registry.module.v1beta1.DownloadService/Download", + "application/proto", + bytes.NewReader(body), + ) + if err != nil { + t.Fatalf("request failed: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusBadRequest { + b, _ := io.ReadAll(resp.Body) + t.Fatalf("status = %d, want 400 (unknown UUID, probe must miss); body: %s", resp.StatusCode, b) + } +} diff --git a/internal/connect/commits.go b/internal/connect/commits.go index 08c3091..70b018b 100644 --- a/internal/connect/commits.go +++ b/internal/connect/commits.go @@ -49,16 +49,12 @@ type commitServiceHandler struct { // so the fallback never guesses across multiple modules. singleModule *moduleRef - // prewarmOnce guards prewarmHeads against double-execution. - prewarmOnce sync.Once // missCache holds the time a commit_id was last confirmed absent from // every configured source (probe all-fail). Used by probeCommitID to // skip repeated upstream fan-out for known-bogus shas within ProbeTTL. missCache map[string]time.Time // runtime knobs (set in connect.New from config.Connect.WithDefaults) - prewarmEnabled bool - prewarmTimeout time.Duration probeEnabled bool probeNegativeTTL time.Duration probeTimeout time.Duration @@ -142,7 +138,7 @@ func (h *commitServiceHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) slog.String("module", ref.module), slog.String("repo", ref.module), ) - meta, err := h.api.repo.GetMeta(r.Context(), ref.owner, ref.module, "") + meta, err := h.api.repo.GetMeta(r.Context(), ref.owner, ref.module, ref.ref) if err != nil { h.upstreamError(r, w, fmt.Sprintf("resolving %s/%s", ref.owner, ref.module), slog.String("owner", ref.owner), slog.String("module", ref.module), @@ -345,7 +341,7 @@ func (h *commitServiceHandler) ServeGraph(w http.ResponseWriter, r *http.Request slog.String("module", ref.module), slog.String("repo", ref.module), ) - meta, err := h.api.repo.GetMeta(r.Context(), ref.owner, ref.module, "") + meta, err := h.api.repo.GetMeta(r.Context(), ref.owner, ref.module, ref.ref) if err != nil { h.upstreamError(r, w, fmt.Sprintf("resolving %s/%s", ref.owner, ref.module), slog.String("owner", ref.owner), slog.String("module", ref.module), @@ -905,100 +901,6 @@ func (h *commitServiceHandler) resolveForeignCommitID(commitID string) *moduleRe return nil } -// registerResolved records a resolved module for a commit id (the canonical -// git sha) under commitMu. commitMap[sha]=ref lets future Downloads of the -// same sha hit directly. infoCache is populated with a digest-less entry — -// ServeDownload's miss-branch recomputes files+digest on first use, so the -// digest field is not load-bearing here. -// -// If an infoCache entry already exists for the module (e.g. a prior -// ServeGraph computed the digest and cached files), its digest is preserved — -// only the resolved-commit identity is refreshed. Without this, a late -// pre-warm would clobber a computed digest with nil and the next cache-hit -// Download would serve a zero digest. -func (h *commitServiceHandler) registerResolved(sha, owner, module string) { - key := owner + "/" + module - // The buf client now expects a 32-char dashless UUID; the SHA is only - // useful as a key for talking to upstream git. Register BOTH so the - // prewarm path makes the UUID the buf client will send hit commitMap - // directly on the first Download, and the SHA alias preserves a - // working lookup for any caller (probe, future debug tool) that - // happens to send a raw sha. - uuid, err := commitUUID(sha) - if err != nil { - // No http.ResponseWriter in scope here — this path runs in - // background (prewarmHeads) and request-scoped (probeCommitID) - // contexts. Log with the proxy logger + a background context so - // the failure is observable without a request to attach to. - h.api.log.LogAttrs(context.Background(), slog.LevelWarn, "internal: commitUUID failure", - slog.String("error_class", "internal"), - slog.String("commit_id", sha), - slog.String("upstream_error", err.Error())) - return - } - h.commitMu.Lock() - h.commitMap[uuid] = moduleRef{owner: owner, module: module} - if sha != "" && sha != uuid { - h.commitMap[sha] = moduleRef{owner: owner, module: module} - } - if existing, ok := h.infoCache[key]; ok { - existing.commitID = uuid - existing.commit = sha - existing.ownerID = owner - existing.moduleID = key - h.infoCache[key] = existing - } else { - h.infoCache[key] = commitInfoCache{ - commitID: uuid, - commit: sha, - ownerID: owner, - moduleID: key, - } - } - h.commitMu.Unlock() -} - -// prewarmHeads resolves the current HEAD commit of every configured module -// and registers it, so that a client sending a cached current-HEAD sha hits -// the commit map without a prior in-session GetCommits. Best-effort and -// idempotent (prewarmOnce): failures are logged and skipped; a later -// probeCommitID call still recovers any sha pre-warm missed. GetMeta-only — -// no file fetch. Intended to run in a background goroutine launched from -// connect.New. -func (h *commitServiceHandler) prewarmHeads() { - h.prewarmOnce.Do(func() { - sources := h.api.repo.Repositories() - log := h.api.log.With(slog.String("component", "prewarm")) - log.LogAttrs(context.Background(), slog.LevelInfo, "prewarm starting", - slog.Int("sources", len(sources)), - slog.Duration("per_call_timeout", h.prewarmTimeout)) - var ok, fail int - for _, s := range sources { - owner, module := s.Owner(), s.RepoName() - if owner == "" || module == "" { - continue - } - ctx, cancel := context.WithTimeout(context.Background(), h.prewarmTimeout) - meta, err := s.GetMeta(ctx, "") // empty commit = HEAD - cancel() - if err != nil || meta.Commit == "" { - fail++ - log.LogAttrs(context.Background(), slog.LevelWarn, "prewarm source miss", - slog.String("owner", owner), slog.String("module", module), - slog.String("repo", module)) - continue - } - h.registerResolved(meta.Commit, owner, module) - ok++ - log.LogAttrs(context.Background(), slog.LevelInfo, "prewarm source resolved", - slog.String("owner", owner), slog.String("module", module), - slog.String("repo", module), slog.String("commit", meta.Commit)) - } - log.LogAttrs(context.Background(), slog.LevelInfo, "prewarm complete", - slog.Int("resolved", ok), slog.Int("failed", fail)) - }) -} - // missCached reports whether sha was recently confirmed absent from every // configured source (within ProbeNegativeTTL). Caller must NOT hold commitMu. func (h *commitServiceHandler) missCached(sha string) bool { @@ -1053,21 +955,65 @@ func (h *commitServiceHandler) sweepMisses(ctx context.Context) { // Callers must NOT hold commitMu. ctx should be the request context so a // disconnecting client bounds the fan-out; each per-source call additionally // gets its own timeout (probeTimeout). -func (h *commitServiceHandler) probeCommitID(ctx context.Context, sha string) (*moduleRef, bool) { - if sha == "" || h.missCached(sha) { +// probeCommitID resolves a Download commit_id that was neither minted +// in-session nor recoverable by the single-module fallback, by asking +// each configured source whether it owns the id. Three input shapes +// are supported: +// +// - raw 40- or 64-char SHA: probed as-is. A git sha is unique to one +// repo, so at most one source succeeds. +// - buf-issued 32-char UUID: the SHA prefix is recovered via +// commitUUIDInverse (the first 14 bytes of the original SHA), and +// each source is probed with the prefix. The returned meta.Commit +// MUST start with the recovered prefix — otherwise the source does +// not own this UUID and the probe must miss (prefix-match +// validation closes review.md finding #6: a wrong-source match +// would silently alias a real commit id to a wrong module). +// - anything else: not a SHA, not a UUID — providers cannot resolve +// it, so the probe records a miss and returns. +// +// On hit, registerResolvedAlias stores both the buf-issued id (the +// primary key the client will use) and the resolved SHA (as an alias +// for any caller that sends a raw sha). On all-fail, the id is +// negative-cached so retries within TTL do not re-probe. +// +// Callers must NOT hold commitMu. ctx should be the request context so +// a disconnecting client bounds the fan-out; each per-source call +// additionally gets its own timeout (probeTimeout). +func (h *commitServiceHandler) probeCommitID(ctx context.Context, id string) (*moduleRef, bool) { + if id == "" || h.missCached(id) { return nil, false } // Re-check commitMap under the lock: a concurrent resolver may have - // already registered this sha while we were waiting on the semaphore. + // already registered this id while we were waiting on the semaphore. h.commitMu.RLock() - ref, already := h.commitMap[sha] + ref, already := h.commitMap[id] h.commitMu.RUnlock() if already { r := ref return &r, true } - // Bound concurrent probes so a flood of distinct unknown shas cannot + // If id is a buf-issued UUID, derive the SHA prefix and probe with the + // prefix. The 14-byte recovery is lossy but 2^112 — sufficient to + // identify a single source among the configured set. The prefix-match + // validation below rules out collisions. + probeArg := id + if isUUID(id) { + prefix, err := commitUUIDInverse(id) + if err != nil { + // id looks UUID-shaped but isn't hex — treat as a miss. + h.rememberMiss(id) + return nil, false + } + probeArg = prefix + } else if !isSHA(id) { + // Not a UUID, not a SHA — providers can't resolve it. + h.rememberMiss(id) + return nil, false + } + + // Bound concurrent probes so a flood of distinct unknown ids cannot // amplify to unbounded upstream load. Non-blocking acquire: if the cap is // reached, decline (the caller 400s; the client retries and hits the // negative cache only after a probe eventually runs). @@ -1086,8 +1032,9 @@ func (h *commitServiceHandler) probeCommitID(ctx context.Context, sha string) (* } type probeResult struct { - ref moduleRef - ok bool + ref moduleRef + commit string + ok bool } // Buffered enough to never block a successful goroutine; first success wins. results := make(chan probeResult, len(sources)) @@ -1103,7 +1050,7 @@ func (h *commitServiceHandler) probeCommitID(ctx context.Context, sha string) (* defer wg.Done() pctx, cancel := context.WithTimeout(ctx, h.probeTimeout) defer cancel() - meta, err := s.GetMeta(pctx, sha) + meta, err := s.GetMeta(pctx, probeArg) if err != nil { if isTransientErr(err) { transient.Store(true) @@ -1113,9 +1060,17 @@ func (h *commitServiceHandler) probeCommitID(ctx context.Context, sha string) (* if meta.Commit == "" { return } + // Prefix-match validation: when probeArg is a SHA prefix + // (28 chars from commitUUIDInverse), the source's commit + // MUST start with it. Otherwise the collision went the wrong + // way and the source does not actually own this UUID. + if isUUID(id) && !strings.HasPrefix(meta.Commit, probeArg) { + return + } results <- probeResult{ - ref: moduleRef{owner: s.Owner(), module: s.RepoName()}, - ok: true, + ref: moduleRef{owner: s.Owner(), module: s.RepoName()}, + commit: meta.Commit, + ok: true, } }(s) } @@ -1123,19 +1078,52 @@ func (h *commitServiceHandler) probeCommitID(ctx context.Context, sha string) (* close(results) for r := range results { - // First (only) success. Register alias and return. - h.registerResolved(sha, r.ref.owner, r.ref.module) + // First (only) success. Register both the buf-issued id and the + // resolved SHA as aliases so future identical requests hit + // directly. + h.registerResolvedAlias(id, r.commit, r.ref.owner, r.ref.module) ref := r.ref return &ref, true } // Only negative-cache when every source returned a definitive not-found. - // A transient failure (timeout/network) leaves the sha retryable. + // A transient failure (timeout/network) leaves the id retryable. if !transient.Load() { - h.rememberMiss(sha) + h.rememberMiss(id) } return nil, false } +// registerResolvedAlias writes a commit id → moduleRef mapping and the +// matching infoCache entry. The id arg is what the buf client sent +// (typically a buf-issued 32-char UUID); sha is what the upstream +// resolved to (a 40- or 64-char hex). Both are stored in commitMap so +// future identical requests hit directly, and sha is also kept as an +// alias for any caller (debug tool, foreign-id probe) that sends a +// raw sha. +func (h *commitServiceHandler) registerResolvedAlias(id, sha, owner, module string) { + key := owner + "/" + module + h.commitMu.Lock() + h.commitMap[id] = moduleRef{owner: owner, module: module} + if sha != "" && sha != id { + h.commitMap[sha] = moduleRef{owner: owner, module: module} + } + if existing, ok := h.infoCache[key]; ok { + existing.commitID = id + existing.commit = sha + existing.ownerID = owner + existing.moduleID = key + h.infoCache[key] = existing + } else { + h.infoCache[key] = commitInfoCache{ + commitID: id, + commit: sha, + ownerID: owner, + moduleID: key, + } + } + h.commitMu.Unlock() +} + // isTransientErr reports whether err looks like a transient upstream failure // (timeout, cancellation, or a network error) rather than a definitive // "commit not found". Used to avoid negative-caching shas during brief diff --git a/internal/connect/commits_helpers.go b/internal/connect/commits_helpers.go index 59b0ed1..ee6b50b 100644 --- a/internal/connect/commits_helpers.go +++ b/internal/connect/commits_helpers.go @@ -11,6 +11,11 @@ import ( type moduleRef struct { owner string module string + // ref is the buf BSR Name.ref field (proto field 3): the branch or + // tag the client asked for. Empty when the client did not send one + // (older buf CLI versions, or v1beta1 callers that omit the field); + // the providers treat empty ref as HEAD. + ref string } // commitUUID returns the buf-style 32-character dashless UUID for a git @@ -59,6 +64,73 @@ func commitUUID(gitSHA string) (string, error) { return hex.EncodeToString(result[:]), nil } +// isSHA reports whether s is a 40-char (SHA-1) or 64-char (SHA-256) +// lowercase hex string. The hex check rejects refs like "main/v2" and +// buf-issued UUIDs (32 chars). Used by providers to decide whether to +// treat a GetMeta commit arg as a raw SHA (fast path) or as a ref to +// resolve through the commit-fetch API. +func isSHA(s string) bool { + if len(s) != 40 && len(s) != 64 { + return false + } + for _, c := range s { + if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) { + return false + } + } + return true +} + +// isUUID reports whether s is exactly 32 lowercase hex characters — the +// shape of a buf-issued dashless UUID. Used by probeCommitID to detect +// UUID inputs and route them through commitUUIDInverse. +func isUUID(s string) bool { + if len(s) != 32 { + return false + } + for _, c := range s { + if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) { + return false + } + } + return true +} + +// commitUUIDInverse recovers the first 28 hex characters (14 bytes) of +// the git SHA that produced the given buf-issued dashless UUID. The +// full SHA cannot be recovered: commitUUID drops the last 6 bytes +// during the forward mapping (14-of-20-byte collision surface — 2^112 +// space, accepted as out-of-scope for collision avoidance). The +// recovered prefix is sufficient to identify a specific source among +// the configured providers (each source's commit space is disjoint) +// and to scope a probeCommitID fan-out to the right repository. +// +// Input contract: the input must be exactly 32 lowercase hex characters +// (the standard dashless UUID shape that uuidutil.FromDashless +// accepts). Any other input returns ("", error). +// +// Inverse: if uuid == commitUUID(sha) for some 40- or 64-char sha, +// then commitUUIDInverse(uuid) == hex(sha[0:14]). The inverse does NOT +// require knowledge of the original sha length — it recovers the same +// 14 bytes regardless of whether the input was SHA-1 or SHA-256. +func commitUUIDInverse(uuid string) (string, error) { + if len(uuid) != 32 { + return "", errors.New("commitUUIDInverse: input is not 32 lowercase hex characters") + } + u, err := hex.DecodeString(uuid) + if err != nil { + return "", errors.New("commitUUIDInverse: input is not 32 lowercase hex characters") + } + // Mirror commitUUID's byte-table in reverse. Bytes 6 and 8 of u are + // version/variant and were overwritten by commitUUID; they are not + // recoverable. The other 14 bytes are the first 14 bytes of the SHA. + var sha [20]byte + copy(sha[0:6], u[0:6]) + sha[6] = u[7] + copy(sha[7:14], u[9:16]) + return hex.EncodeToString(sha[:14]), nil +} + func parseResourceRefs(msg []byte) []moduleRef { var refs []moduleRef for len(msg) > 0 { @@ -106,7 +178,7 @@ func parseResourceRef(msg []byte) *moduleRef { } func parseResourceRefName(msg []byte) *moduleRef { - var owner, module string + var owner, module, ref string for len(msg) > 0 { num, typ, n := protowire.ConsumeTag(msg) if n < 0 { @@ -121,6 +193,13 @@ func parseResourceRefName(msg []byte) *moduleRef { v, mLen := protowire.ConsumeBytes(msg) msg = msg[mLen:] module = string(v) + } else if num == 3 && typ == protowire.BytesType { + // buf BSR Name.ref (branch/tag). Optional: older buf clients + // do not send it; the ref-aware code paths tolerate an empty + // value (treated as HEAD by the providers). + v, mLen := protowire.ConsumeBytes(msg) + msg = msg[mLen:] + ref = string(v) } else { n = protowire.ConsumeFieldValue(num, typ, msg) if n < 0 { @@ -130,7 +209,7 @@ func parseResourceRefName(msg []byte) *moduleRef { } } if owner != "" && module != "" { - return &moduleRef{owner: owner, module: module} + return &moduleRef{owner: owner, module: module, ref: ref} } return nil } diff --git a/internal/connect/commits_helpers_test.go b/internal/connect/commits_helpers_test.go index ba0c1b8..2324458 100644 --- a/internal/connect/commits_helpers_test.go +++ b/internal/connect/commits_helpers_test.go @@ -4,6 +4,8 @@ import ( "encoding/hex" "strings" "testing" + + "google.golang.org/protobuf/encoding/protowire" ) // TestCommitUUIDFormat locks in the wire format buf v1.69.0 requires. @@ -308,6 +310,192 @@ func preResolveForTest(short string) string { return short + strings.Repeat("0", 40-len(short)) } +// TestIsSHA locks in the SHA-shape check used by the providers to decide +// whether a GetMeta commit arg is a raw SHA (fast path) or a ref to +// resolve. The contract: 40 or 64 lowercase hex characters only — refs +// like "main/v2" and buf-issued UUIDs (32 chars) must both return false. +func TestIsSHA(t *testing.T) { + cases := []struct { + name string + in string + want bool + }{ + {name: "empty", in: "", want: false}, + {name: "40 lowercase hex", in: strings.Repeat("0", 40), want: true}, + {name: "64 lowercase hex", in: strings.Repeat("0", 64), want: true}, + {name: "32 hex (UUID shape)", in: strings.Repeat("0", 32), want: false}, + {name: "40 hex with trailing non-hex", in: strings.Repeat("0", 39) + "X", want: false}, + {name: "40 hex with one uppercase", in: "81353411f7b010d5b9ebeb1899066aac18a3670A", want: false}, + {name: "41 chars", in: strings.Repeat("0", 41), want: false}, + {name: "39 chars", in: strings.Repeat("0", 39), want: false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isSHA(tc.in); got != tc.want { + t.Fatalf("isSHA(%q) = %v, want %v", tc.in, got, tc.want) + } + }) + } +} + +// TestIsUUID locks in the 32-lowercase-hex shape check used by +// probeCommitID to detect buf-issued dashless UUID inputs and route them +// through commitUUIDInverse. The contract: exactly 32 lowercase hex +// characters. Empty, 40-char, and non-hex inputs must all return false. +func TestIsUUID(t *testing.T) { + cases := []struct { + name string + in string + want bool + }{ + {name: "empty", in: "", want: false}, + {name: "32 lowercase hex", in: strings.Repeat("0", 32), want: true}, + {name: "40 lowercase hex", in: strings.Repeat("0", 40), want: false}, + {name: "32 hex with trailing non-hex", in: strings.Repeat("0", 31) + "X", want: false}, + {name: "31 chars", in: strings.Repeat("a", 31), want: false}, + {name: "33 chars", in: strings.Repeat("a", 33), want: false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isUUID(tc.in); got != tc.want { + t.Fatalf("isUUID(%q) = %v, want %v", tc.in, got, tc.want) + } + }) + } +} + +// TestParseResourceRefName_ReadsRef verifies that parseResourceRefName +// captures the ref field (proto field 3) of the buf BSR Name message. +// The buf CLI sends a `ref` to disambiguate which branch/tag the client +// wants; the proxy must return the SHA at that ref, not HEAD. Without +// the field-3 arm, ref inputs would be silently dropped. +func TestParseResourceRefName_ReadsRef(t *testing.T) { + // Build a Name { owner=1, module=2, ref=3 } message. + var name []byte + name = protowire.AppendTag(name, 1, protowire.BytesType) + name = protowire.AppendString(name, "cyp") + name = protowire.AppendTag(name, 2, protowire.BytesType) + name = protowire.AppendString(name, "cyp-net-listeners") + name = protowire.AppendTag(name, 3, protowire.BytesType) + name = protowire.AppendString(name, "main/v2") + + ref := parseResourceRefName(name) + if ref == nil { + t.Fatal("parseResourceRefName returned nil for a valid Name with owner+module+ref") + } + if ref.owner != "cyp" || ref.module != "cyp-net-listeners" || ref.ref != "main/v2" { + t.Fatalf("parseResourceRefName = %+v, want {owner:cyp module:cyp-net-listeners ref:main/v2}", *ref) + } +} + +// TestParseResourceRefName_NoRef verifies that the ref field is optional +// in the buf BSR Name proto: a Name with only owner+module parses to a +// moduleRef whose ref is the zero value. Older buf clients do not send a +// ref; the ref-aware code paths must tolerate the field being absent. +func TestParseResourceRefName_NoRef(t *testing.T) { + var name []byte + name = protowire.AppendTag(name, 1, protowire.BytesType) + name = protowire.AppendString(name, "cyp") + name = protowire.AppendTag(name, 2, protowire.BytesType) + name = protowire.AppendString(name, "cyp-net-listeners") + + ref := parseResourceRefName(name) + if ref == nil { + t.Fatal("parseResourceRefName returned nil for a valid Name with owner+module (no ref)") + } + if ref.owner != "cyp" || ref.module != "cyp-net-listeners" || ref.ref != "" { + t.Fatalf("parseResourceRefName = %+v, want {owner:cyp module:cyp-net-listeners ref:}", *ref) + } +} + +// TestCommitUUIDInverse locks in the inverse of commitUUID: given a +// 32-char dashless UUID, recover the first 28 hex characters of the +// git SHA that produced it. The recovery is lossy by design — +// commitUUID drops the last 6 bytes of the SHA (and overwrites the +// version/variant bytes at positions 6 and 8), so commitUUIDInverse +// can only recover the first 14 bytes (28 hex chars). The recovered +// prefix is sufficient to identify a single source among the +// configured providers (each source's commit space is disjoint) and +// to scope a probeCommitID fan-out to the right repository. +// +// The round-trip here is commitUUID -> commitUUIDInverse, NOT +// commitUUIDInverse -> commitUUID. The forward mapping is +// deterministic; the inverse is many-to-one (collisions on the last 6 +// bytes are accepted as out-of-scope for collision avoidance). +func TestCommitUUIDInverse(t *testing.T) { + cases := []struct { + name string + uuid string + want string + wantErr bool + }{ + { + name: "40-char SHA round-trip", + uuid: mustCommitUUID(t, "81353411f7b010d5b9ebeb1899066aac18a36701"), + want: "81353411f7b010d5b9ebeb189906", + }, + { + name: "all-zero 40-char SHA", + uuid: mustCommitUUID(t, "0000000000000000000000000000000000000000"), + want: "0000000000000000000000000000", + }, + { + name: "all-ones 40-char SHA", + uuid: mustCommitUUID(t, "ffffffffffffffffffffffffffffffffffffffff"), + want: "ffffffffffffffffffffffffffff", + }, + { + // commitUUID consumes only the first 14 bytes regardless of + // input length, so a 64-char SHA-256 whose first 14 bytes + // match a 40-char fixture must produce the same UUID and + // hence the same recovered prefix. + name: "64-char SHA round-trip", + uuid: mustCommitUUID(t, "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"), + want: "0123456789abcdef0123456789ab", + }, + {name: "empty string", uuid: "", want: "", wantErr: true}, + {name: "31 chars", uuid: strings.Repeat("a", 31), want: "", wantErr: true}, + {name: "33 chars", uuid: strings.Repeat("a", 33), want: "", wantErr: true}, + {name: "32 chars non-hex", uuid: strings.Repeat("X", 32), want: "", wantErr: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := commitUUIDInverse(tc.uuid) + if tc.wantErr { + if err == nil { + t.Fatalf("commitUUIDInverse(%q) = %q, want error", tc.uuid, got) + } + if !strings.Contains(err.Error(), "commitUUIDInverse") { + t.Errorf("commitUUIDInverse error = %q, want substring \"commitUUIDInverse\"", err.Error()) + } + if got != "" { + t.Errorf("commitUUIDInverse(%q) on error path returned non-empty string %q", tc.uuid, got) + } + return + } + if err != nil { + t.Fatalf("commitUUIDInverse(%q) unexpected error: %v", tc.uuid, err) + } + if got != tc.want { + t.Fatalf("commitUUIDInverse(%q) = %q, want %q", tc.uuid, got, tc.want) + } + }) + } +} + +// mustCommitUUID is a test helper that computes commitUUID(sha) and +// fails the test on error. The TestCommitUUIDInverse table computes +// the UUID inline so a future change to commitUUID's byte-table +// automatically updates the test expectations. +func mustCommitUUID(t *testing.T, sha string) string { + t.Helper() + u, err := commitUUID(sha) + if err != nil { + t.Fatalf("commitUUID(%q) unexpected error: %v", sha, err) + } + return u +} + // TestPreResolveForTest exercises the test fixture that pads short // hex strings out to 40 chars. The padding behavior must be exactly // right-pad-with-'0' and truncate-or-pass-through for inputs of 40+ diff --git a/internal/connect/uuid_format_test.go b/internal/connect/uuid_format_test.go index b0c5b57..7772811 100644 --- a/internal/connect/uuid_format_test.go +++ b/internal/connect/uuid_format_test.go @@ -182,7 +182,7 @@ func TestServeDownload_UnknownCommitID_ReturnsBadRequest(t *testing.T) { meta: content.Meta{Commit: "abc123", DefaultBranch: "main"}, files: []content.File{{Path: "x.proto", Data: []byte(`syntax = "proto3";`), Hash: shake256.Hash{}}}, } - // No prewarm: registerResolved is never called, commitMap stays empty. + // No prewarm: registerResolvedAlias is never called, commitMap stays empty. srv := httptest.NewServer(testMux(p)) defer srv.Close() diff --git a/internal/providers/bitbucket/client.go b/internal/providers/bitbucket/client.go index ccbd8b0..39183e7 100644 --- a/internal/providers/bitbucket/client.go +++ b/internal/providers/bitbucket/client.go @@ -69,6 +69,7 @@ var ( tmplGetDefaultBranch = tmplBuild("/branches/default") tmplGetFilesList = tmplBuild("/files") tmplGetFileContent = tmplBuild("/raw/{{.name}}") + tmplGetCommit = tmplBuild("/commits/{{.id}}") ) type httpClient struct { diff --git a/internal/providers/bitbucket/getrepo.go b/internal/providers/bitbucket/getrepo.go index 000cdf7..7adaaea 100644 --- a/internal/providers/bitbucket/getrepo.go +++ b/internal/providers/bitbucket/getrepo.go @@ -9,19 +9,83 @@ import ( "github.com/easyp-tech/server/internal/providers/content" ) +// isSHA reports whether s is a 40-char (SHA-1) or 64-char (SHA-256) +// lowercase hex string. Mirrors the connect-package isSHA used by +// probeCommitID; duplicated here so the provider's commit-vs-ref gate +// does not require exporting a connect-package helper. +func isSHA(s string) bool { + if len(s) != 40 && len(s) != 64 { + return false + } + for _, c := range s { + if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) { + return false + } + } + return true +} + func (c client) getMeta(ctx context.Context, commit string) (content.Meta, error) { meta, err := c.getRepo(ctx) if err != nil { return meta, fmt.Errorf("investigating: %w", err) } - if commit != "" && commit != "main" { - meta.Commit = commit + // Three branches: + // - commit == "": no ref was supplied; keep HEAD from getRepo. + // - isSHA(commit): raw SHA fast path (40/64 lowercase hex). + // - non-SHA non-empty: treat as a ref and resolve via the + // provider's commit-fetch API. The previous + // `commit != "main"` carve-out silently stamped + // the ref into meta.Commit without resolving, + // which both ignored refs and broke + // 40/64-char SHAs that weren't at HEAD. + if commit != "" { + if isSHA(commit) { + meta.Commit = commit + } else { + resolved, err := c.getCommit(ctx, commit) + if err != nil { + return meta, fmt.Errorf("resolving ref %q: %w", commit, err) + } + meta.Commit = resolved + } } return meta, nil } +// commitInfo is the subset of the Bitbucket Server commits API response +// that getCommit needs. The /commits/{commitId} endpoint returns the +// resolved commit id, the displayId (short sha), and a few metadata +// fields. We only need id. +type commitInfo struct { + ID string `json:"id"` + DisplayID string `json:"displayId"` +} + +// getCommit resolves a ref (branch name, tag, or short sha) to a full +// commit id by calling Bitbucket Server's /commits/{commitId} endpoint. +// Bitbucket accepts a ref or short sha in the path; the response's id +// field is the full 40-char SHA-1 (or 64-char SHA-256 on SHA-256-enabled +// repos). The endpoint returns 404 for unknown refs. +func (c client) getCommit(ctx context.Context, ref string) (string, error) { + info, err := httpGetJSON[commitInfo]( + ctx, + c.client, + tmplGetCommit, + paramsMap{"id": ref}, + nil, + ) + if err != nil { + return "", fmt.Errorf("resolving commit %q: %w", ref, err) + } + if info.ID == "" { + return "", fmt.Errorf("resolving commit %q: empty id in response", ref) + } + return info.ID, nil +} + var ErrEmpty = errors.New("empty") func (c client) getRepo(ctx context.Context) (content.Meta, error) { diff --git a/internal/providers/bitbucket/getrepo_test.go b/internal/providers/bitbucket/getrepo_test.go new file mode 100644 index 0000000..bb89155 --- /dev/null +++ b/internal/providers/bitbucket/getrepo_test.go @@ -0,0 +1,151 @@ +package bitbucket + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +const testBasePath = "/rest/api/1.0/projects/CYP/repos/cyp-net-listeners" + +// TestGetMeta_Empty pins the HEAD path: when no ref is supplied, getMeta +// returns the default branch's HEAD commit as resolved by /branches/default. +// This is the regression guard for the empty-input path so a future change +// to the ref-resolution branch cannot accidentally break the no-ref case. +func TestGetMeta_Empty(t *testing.T) { + const headCommit = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + + var hit bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // /branches/default is what searchRepo calls. The basePath is + // already prepended to the URL by httpClient.get. + if r.URL.Path != testBasePath+"/branches/default" { + t.Errorf("unexpected path %q on HEAD call", r.URL.Path) + http.NotFound(w, r) + return + } + hit = true + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":"` + headCommit + `","displayId":"main","type":"BRANCH","latestCommit":"` + headCommit + `","latestChangeset":"` + headCommit + `","isDefault":true}`)) + })) + defer srv.Close() + + c := connect(nil, "", "", srv.URL+testBasePath) + meta, err := c.getMeta(context.Background(), "") + if err != nil { + t.Fatalf("getMeta(empty) unexpected error: %v", err) + } + if !hit { + t.Fatal("HEAD endpoint was not called") + } + if meta.Commit != headCommit { + t.Fatalf("meta.Commit = %q, want %q (HEAD from /branches/default)", meta.Commit, headCommit) + } +} + +// TestGetMeta_RawSHA_40 pins the SHA fast path: a 40-char lowercase hex +// input is returned verbatim in meta.Commit without an extra round-trip +// to /commits/{sha}. This guards against a future refactor that +// accidentally routes SHAs through the ref-resolution API. +func TestGetMeta_RawSHA_40(t *testing.T) { + const sha = "81353411f7b010d5b9ebeb1899066aac18a36701" + + hit := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hit++ + if r.URL.Path != testBasePath+"/branches/default" { + t.Errorf("SHA fast path should ONLY call /branches/default; got %q", r.URL.Path) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":"head","displayId":"main","type":"BRANCH","latestCommit":"head","latestChangeset":"head","isDefault":true}`)) + })) + defer srv.Close() + + c := connect(nil, "", "", srv.URL+testBasePath) + meta, err := c.getMeta(context.Background(), sha) + if err != nil { + t.Fatalf("getMeta(40-char sha) unexpected error: %v", err) + } + if hit != 1 { + t.Errorf("expected exactly 1 upstream call (HEAD), got %d", hit) + } + if meta.Commit != sha { + t.Fatalf("meta.Commit = %q, want %q (SHA fast path)", meta.Commit, sha) + } +} + +// TestGetMeta_RawSHA_64 pins the SHA-256 fast path: a 64-char lowercase +// hex input (Bitbucket Server on SHA-256-enabled repos) is returned +// verbatim in meta.Commit. Mirrors the 40-char case; the gate must accept +// both shapes. +func TestGetMeta_RawSHA_64(t *testing.T) { + const sha = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + + hit := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hit++ + if r.URL.Path != testBasePath+"/branches/default" { + t.Errorf("SHA-256 fast path should ONLY call /branches/default; got %q", r.URL.Path) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":"head","displayId":"main","type":"BRANCH","latestCommit":"head","latestChangeset":"head","isDefault":true}`)) + })) + defer srv.Close() + + c := connect(nil, "", "", srv.URL+testBasePath) + meta, err := c.getMeta(context.Background(), sha) + if err != nil { + t.Fatalf("getMeta(64-char sha) unexpected error: %v", err) + } + if hit != 1 { + t.Errorf("expected exactly 1 upstream call (HEAD), got %d", hit) + } + if meta.Commit != sha { + t.Fatalf("meta.Commit = %q, want %q (SHA-256 fast path)", meta.Commit, sha) + } +} + +// TestGetMeta_ResolvesRef pins the ref-resolution path: a non-SHA, non- +// empty input like "main/v2" triggers a GET to /commits/main/v2, the +// response's id is parsed, and meta.Commit is stamped with the resolved +// SHA. This is the headline fix for the `buf.yaml: main/v2` case that +// previously collapsed to HEAD. +func TestGetMeta_ResolvesRef(t *testing.T) { + const ref = "main/v2" + const resolved = "abc1230000000000000000000000000000000000" + + commitsHits := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == testBasePath+"/branches/default": + // HEAD lookup + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":"head","displayId":"main","type":"BRANCH","latestCommit":"head","latestChangeset":"head","isDefault":true}`)) + case r.URL.Path == testBasePath+"/commits/"+ref: + commitsHits++ + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":"` + resolved + `","displayId":"abc1230"}`)) + default: + t.Errorf("unexpected upstream call to %q", r.URL.Path) + http.NotFound(w, r) + } + })) + defer srv.Close() + + c := connect(nil, "", "", srv.URL+testBasePath) + meta, err := c.getMeta(context.Background(), ref) + if err != nil { + t.Fatalf("getMeta(%q) unexpected error: %v", ref, err) + } + if commitsHits != 1 { + t.Errorf("expected exactly 1 /commits/ call, got %d", commitsHits) + } + if meta.Commit != resolved { + t.Fatalf("meta.Commit = %q, want %q (resolved SHA from /commits/%s)", meta.Commit, resolved, ref) + } +} + +// _ = strings.HasPrefix keeps the import stable if a future test needs it. +var _ = strings.HasPrefix diff --git a/internal/providers/github/getrepo.go b/internal/providers/github/getrepo.go index c0fb4d3..89e83e9 100644 --- a/internal/providers/github/getrepo.go +++ b/internal/providers/github/getrepo.go @@ -12,14 +12,49 @@ import ( "github.com/easyp-tech/server/internal/providers/content" ) +// isSHA reports whether s is a 40-char (SHA-1) or 64-char (SHA-256) +// lowercase hex string. Mirrors the connect-package isSHA used by +// probeCommitID; duplicated here so the provider's commit-vs-ref gate +// does not require exporting a connect-package helper. +func isSHA(s string) bool { + if len(s) != 40 && len(s) != 64 { + return false + } + for _, c := range s { + if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) { + return false + } + } + return true +} + func (c client) GetMeta(ctx context.Context, owner, repoName, commit string) (content.Meta, error) { meta, err := c.getRepo(ctx, owner, repoName) if err != nil { return meta, fmt.Errorf("investigating %q/%q: %w", owner, repoName, err) } - if commit != "" && commit != "main" { - meta.Commit = commit + // Three branches: + // - commit == "": no ref was supplied; keep HEAD from getRepo. + // - isSHA(commit): raw SHA fast path (40/64 lowercase hex). + // - non-SHA non-empty: treat as a ref and resolve via GitHub's + // repos.GetCommit (accepts ref names, short + // SHAs >= 7 chars, and full SHAs). The + // previous `commit != "main"` carve-out + // silently stamped the ref into meta.Commit + // without resolving, which both ignored refs + // and broke 40/64-char SHAs that weren't at + // HEAD. + if commit != "" { + if isSHA(commit) { + meta.Commit = commit + } else { + rc, _, err := c.repos.GetCommit(ctx, owner, repoName, commit, nil) + if err != nil { + return meta, fmt.Errorf("resolving ref %q: %w", commit, err) + } + meta.Commit = rc.GetSHA() + } } return meta, nil diff --git a/internal/providers/github/getrepo_test.go b/internal/providers/github/getrepo_test.go new file mode 100644 index 0000000..088013e --- /dev/null +++ b/internal/providers/github/getrepo_test.go @@ -0,0 +1,96 @@ +package github + +import ( + "context" + "errors" + "testing" + + "github.com/google/go-github/v59/github" +) + +// TestGetMeta_Empty pins the HEAD path: when no ref is supplied, GetMeta +// returns the default branch's HEAD commit. This is the regression +// guard for the empty-input path so a future change to the ref-resolution +// branch cannot accidentally break the no-ref case. +func TestGetMeta_Empty(t *testing.T) { + const headCommit = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + + mock := newMockRepos(). + WithGet("cyp", "cyp-net-listeners", + &github.Repository{DefaultBranch: github.String("main")}, nil). + WithGetBranch("cyp", "cyp-net-listeners", "main", &github.Branch{ + Commit: &github.RepositoryCommit{SHA: github.String(headCommit)}, + }, nil) + + c := client{log: testLogger(), repos: mock} + meta, err := c.GetMeta(context.Background(), "cyp", "cyp-net-listeners", "") + if err != nil { + t.Fatalf("GetMeta(empty) unexpected error: %v", err) + } + if meta.Commit != headCommit { + t.Fatalf("meta.Commit = %q, want %q (HEAD from default branch)", meta.Commit, headCommit) + } +} + +// TestGetMeta_RawSHA_40 pins the SHA fast path: a 40-char lowercase hex +// input is returned verbatim in meta.Commit without a call to repos.GetCommit. +// This guards against a future refactor that accidentally routes SHAs +// through the ref-resolution API. +func TestGetMeta_RawSHA_40(t *testing.T) { + const sha = "81353411f7b010d5b9ebeb1899066aac18a36701" + + mock := newMockRepos(). + WithGet("cyp", "cyp-net-listeners", + &github.Repository{DefaultBranch: github.String("main")}, nil). + WithGetBranch("cyp", "cyp-net-listeners", "main", &github.Branch{ + Commit: &github.RepositoryCommit{SHA: github.String("head")}, + }, nil). + // Expect NO call to GetCommit for the SHA fast path. + WithGetCommitError("cyp", "cyp-net-listeners", sha, + errors.New("unexpected GetCommit call on SHA fast path")) + + c := client{log: testLogger(), repos: mock} + meta, err := c.GetMeta(context.Background(), "cyp", "cyp-net-listeners", sha) + if err != nil { + t.Fatalf("GetMeta(40-char sha) unexpected error: %v", err) + } + if meta.Commit != sha { + t.Fatalf("meta.Commit = %q, want %q (SHA fast path)", meta.Commit, sha) + } +} + +// TestGetMeta_ResolvesRef pins the ref-resolution path: a non-SHA, non- +// empty input like "main/v2" triggers a call to repos.GetCommit(ctx, +// owner, repo, "main/v2", nil), the response's SHA is parsed, and +// meta.Commit is stamped with the resolved SHA. This is the headline +// fix for the `buf.yaml: main/v2` case that previously collapsed to +// HEAD. +func TestGetMeta_ResolvesRef(t *testing.T) { + const ( + ref = "main/v2" + resolved = "abc1230000000000000000000000000000000000" + ) + + mock := newMockRepos(). + WithGet("cyp", "cyp-net-listeners", + &github.Repository{DefaultBranch: github.String("main")}, nil). + WithGetBranch("cyp", "cyp-net-listeners", "main", &github.Branch{ + Commit: &github.RepositoryCommit{SHA: github.String("head")}, + }, nil). + WithGetCommit("cyp", "cyp-net-listeners", ref, &github.RepositoryCommit{ + SHA: github.String(resolved), + }, nil) + + c := client{log: testLogger(), repos: mock} + meta, err := c.GetMeta(context.Background(), "cyp", "cyp-net-listeners", ref) + if err != nil { + t.Fatalf("GetMeta(%q) unexpected error: %v", ref, err) + } + if mock.GetCommitCallCount("cyp", "cyp-net-listeners", ref) != 1 { + t.Errorf("expected exactly 1 GetCommit call for ref=%q, got %d", ref, + mock.GetCommitCallCount("cyp", "cyp-net-listeners", ref)) + } + if meta.Commit != resolved { + t.Fatalf("meta.Commit = %q, want %q (resolved SHA from repos.GetCommit)", meta.Commit, resolved) + } +} diff --git a/internal/providers/github/mockrepos_test.go b/internal/providers/github/mockrepos_test.go new file mode 100644 index 0000000..0a56e98 --- /dev/null +++ b/internal/providers/github/mockrepos_test.go @@ -0,0 +1,202 @@ +package github + +import ( + "context" + "io" + "log/slog" + "sync" + "sync/atomic" + + "github.com/google/go-github/v59/github" +) + +// testLogger returns a slog.Logger that discards output. Used by tests +// that exercise the github client without caring about log output. +func testLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +// key3 is the composite lookup key for mock repos: (owner, repo, arg). +// The third element is sha for GetCommit, branch for GetBranch, and +// ignored for Get / DownloadContents. +type key3 struct{ owner, repo, arg string } + +// mockRepos is a stub Repositories implementation for tests. It records +// calls and returns canned responses. Build with newMockRepos(); chain +// .WithGet / .WithGetBranch / .WithGetCommit / .WithDownloadContents to +// configure responses. By default a call with no configured response +// panics — this surfaces unexpected upstream calls in tests, instead of +// silently returning a zero value. +// +// A call that has been configured (via WithGetCommitError) to return +// an error lets tests assert the SHA fast path did NOT make the call. +type mockRepos struct { + mu sync.Mutex + + gets map[key3]*getResp + getBranches map[key3]*getResp + getCommits map[key3]*getResp + downloads map[key3]*getResp + commitCalls map[key3]*atomic.Int32 + branchCalls map[key3]*atomic.Int32 + getCalls map[key3]*atomic.Int32 + downloadCalls map[key3]*atomic.Int32 +} + +type getResp struct { + repo *github.Repository + branch *github.Branch + commit *github.RepositoryCommit + rcBody io.ReadCloser + response *github.Response + err error +} + +// newMockRepos returns an empty mockRepos. The mock will panic on any +// upstream call that has not been configured via WithXxx. +func newMockRepos() *mockRepos { + return &mockRepos{ + gets: make(map[key3]*getResp), + getBranches: make(map[key3]*getResp), + getCommits: make(map[key3]*getResp), + downloads: make(map[key3]*getResp), + commitCalls: make(map[key3]*atomic.Int32), + branchCalls: make(map[key3]*atomic.Int32), + getCalls: make(map[key3]*atomic.Int32), + } +} + +// WithGet configures the response for repos.Get(ctx, owner, repo). +func (m *mockRepos) WithGet(owner, repo string, r *github.Repository, err error) *mockRepos { + m.gets[key3{owner, repo, ""}] = &getResp{repo: r, err: err} + return m +} + +// WithGetBranch configures the response for repos.GetBranch(ctx, owner, +// repo, branch, maxRedirects). +func (m *mockRepos) WithGetBranch(owner, repo, branch string, b *github.Branch, err error) *mockRepos { + m.getBranches[key3{owner, repo, branch}] = &getResp{branch: b, err: err} + return m +} + +// WithGetCommit configures the response for repos.GetCommit(ctx, owner, +// repo, sha, opts). +func (m *mockRepos) WithGetCommit(owner, repo, sha string, c *github.RepositoryCommit, err error) *mockRepos { + m.getCommits[key3{owner, repo, sha}] = &getResp{commit: c, err: err} + return m +} + +// WithGetCommitError configures GetCommit to return err for the given +// (owner, repo, sha). Used to assert the SHA fast path did NOT make a +// GetCommit call: if the test reaches the configured call, the test +// fails with the supplied error. +func (m *mockRepos) WithGetCommitError(owner, repo, sha string, err error) *mockRepos { + m.getCommits[key3{owner, repo, sha}] = &getResp{err: err} + return m +} + +// WithDownloadContents configures the response for repos.DownloadContents +// (not used by the current tests but required by the Repositories +// interface). +func (m *mockRepos) WithDownloadContents(owner, repo, path string, body io.ReadCloser, err error) *mockRepos { + m.downloads[key3{owner, repo, path}] = &getResp{rcBody: body, err: err} + return m +} + +// GetCommitCallCount returns the number of times GetCommit was called +// for the given (owner, repo, sha). Used by tests that assert the +// ref-resolution path was taken (or, conversely, the SHA fast path +// was NOT taken). +func (m *mockRepos) GetCommitCallCount(owner, repo, sha string) int32 { + k := key3{owner, repo, sha} + m.mu.Lock() + c, ok := m.commitCalls[k] + m.mu.Unlock() + if !ok { + return 0 + } + return c.Load() +} + +func (m *mockRepos) Get(ctx context.Context, owner, repo string) (*github.Repository, *github.Response, error) { + k := key3{owner, repo, ""} + m.mu.Lock() + c, ok := m.getCalls[k] + if !ok { + c = &atomic.Int32{} + m.getCalls[k] = c + } + m.mu.Unlock() + c.Add(1) + + m.mu.Lock() + r, rok := m.gets[k] + m.mu.Unlock() + if !rok { + panic("mockRepos: Get not configured for " + k.owner + "/" + k.repo) + } + return r.repo, r.response, r.err +} + +func (m *mockRepos) GetBranch(ctx context.Context, owner, repo, branch string, maxRedirects int) (*github.Branch, *github.Response, error) { + k := key3{owner, repo, branch} + m.mu.Lock() + c, ok := m.branchCalls[k] + if !ok { + c = &atomic.Int32{} + m.branchCalls[k] = c + } + m.mu.Unlock() + c.Add(1) + + m.mu.Lock() + r, rok := m.getBranches[k] + m.mu.Unlock() + if !rok { + panic("mockRepos: GetBranch not configured for " + k.owner + "/" + k.repo + "@" + branch) + } + return r.branch, r.response, r.err +} + +func (m *mockRepos) GetCommit(ctx context.Context, owner, repo, sha string, opts *github.ListOptions) (*github.RepositoryCommit, *github.Response, error) { + k := key3{owner, repo, sha} + m.mu.Lock() + c, ok := m.commitCalls[k] + if !ok { + c = &atomic.Int32{} + m.commitCalls[k] = c + } + m.mu.Unlock() + c.Add(1) + + m.mu.Lock() + r, rok := m.getCommits[k] + m.mu.Unlock() + if !rok { + panic("mockRepos: GetCommit not configured for " + k.owner + "/" + k.repo + "@" + sha) + } + return r.commit, r.response, r.err +} + +func (m *mockRepos) DownloadContents(ctx context.Context, owner, repo, filepath string, opts *github.RepositoryContentGetOptions) (io.ReadCloser, *github.Response, error) { + k := key3{owner, repo, filepath} + m.mu.Lock() + c, ok := m.downloadCalls[k] + if !ok { + c = &atomic.Int32{} + m.downloadCalls[k] = c + } + m.mu.Unlock() + c.Add(1) + + m.mu.Lock() + r, rok := m.downloads[k] + m.mu.Unlock() + if !rok { + panic("mockRepos: DownloadContents not configured for " + k.owner + "/" + k.repo + "/" + filepath) + } + return r.rcBody, r.response, r.err +} + +// Compile-time check that mockRepos satisfies the Repositories interface. +var _ Repositories = (*mockRepos)(nil)