Repository navigation
refactor(server/api): share the Cache-Control table and name the confidence thresholds - #1266
Conversation
…idence thresholds setCacheHeaders (container spaHandler) carried a byte-identical copy of the extension switch in cacheControlForExt (Lambda static path), so adding an extension to one gave the same asset a different cache policy per deployment mode. setCacheHeaders now calls cacheControlForExt. confidenceBucketFor inlined 200 / 3 / 50. They are now named constants documented as mirrors of pkg/exchange's ConfidenceHighSavingsUSD, ConfidenceHighMinCount and ConfidenceMediumSavingsUSD. Importing those needs a cloud-commitments-go/pkg bump past d11bdde, which is left out here. The count-0 clamp is kept and documented as a deliberate difference from the exchange ranking's neutral score. No behavior change. New table tests pin every Cache-Control value through both static paths and the exact confidence boundaries, including count 0 and negative counts; they pass before and after. Refs LeanerCloud#343 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 4 billable files and costs up to $1.00. Or wait 59 minutes for your next included review. View limit details
Comment |
Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB4. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on |
Root cause
Two drift hazards from the 2026-09-02 audit (A06-020, A09-012):
setCacheHeaders(containerspaHandler) ininternal/server/static.gohad a byte-identical copy of the extension switch incacheControlForExt(LambdaserveStaticForLambda). If you added.avifor.mapto only one of them, the same asset got a different cache policy depending on whether the deployment was a container or a Lambda Function URL.confidenceBucketForininternal/api/handler_recommendations.goinlined200/3/50, the same cut-offs thatpkg/exchangeuses for the reshape ranking.Fix
setCacheHeadersnow callscacheControlForExt(strings.ToLower(path.Ext(urlPath))), as the issue suggested. There is now one extension-to-policy table.confidenceBucketFornow uses the named constantsconfidenceHighSavingsUSD,confidenceHighMinCountandconfidenceMediumSavingsUSD. Their doc comment says they must equalexchange.ConfidenceHighSavingsUSD,exchange.ConfidenceHighMinCountandexchange.ConfidenceMediumSavingsUSD.count < 1to 1, so savings alone decide between medium and low. This differs on purpose from the exchange ranking, which returns a neutral 0.5 on a zero count: a badge must always show a bucket, while a ranking score should stay neutral when data is missing. The go side documents the same split (refactor(exchange): export confidence bucket thresholds as named constants cloud-commitments-go#522 godoc). Tests now cover this, including negative counts.There is no behavior change: every served Cache-Control value and every bucket is the same as before.
Refs #343
Why
Refs, notCloses: the import is left out on purposeThe issue's second acceptance criterion asks this repo to import the exported constants from
pkg/exchange. Those constants landed in cloud-commitments-god11bdde(LeanerCloud/cloud-commitments-go#522).go.modpinscloud-commitments-go/pkg v0.0.0-20261018003623-541e05e5fd29, which is older, so the import needs a pkg bump. I left the bump out of this PR because:d11bdde) also brings in 7 unrelated go commits, includingpkg/scorer(fix(scorer): distinguish an explicit 0% savings floor from unset cloud-commitments-go#516, explicit 0% savings floor vs unset),pkg/common/drop_summary.go(fix(common): panic on a negative DropSummary count cloud-commitments-go#518, panics on a negative count) andpkg/config/load.go. That would break this PR's "no behavior change" guarantee.pkgline, each to a different pseudo-version. A third bump here would conflict with both.Once one of those bumps merges, the follow-up is mechanical: replace the three literals in the
constblock withexchange.Confidence*(the pinned test table stays the same and keeps guarding the values). After that, #343 can be closed.Verification
Unit tests only. This is a pure refactor with no DB or integration surface, so no integration test was needed.
Environment:
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, Go 1.26.9.New and extended tests:
internal/server/static_test.go:TestCachePolicyPinnedpins the exact header for every listed extension, plus upper- and mixed-case variants and default fall-through cases (.json,.avif,.js.map,.txt, no extension). It checks bothsetCacheHeadersandcacheControlForExt.TestCachePolicyParity_ContainerAndLambdaserves the same files throughspaFileServerandserveStaticForLambdaand requires the same header from both.internal/api/handler_recommendations_test.goTestConfidenceBucketForadds cases at and just below each threshold, and for count 0 with high and low savings and a negative count.Before/after evidence. This is a behavior-preserving refactor, so the pins are meant to pass both before and after:
origin/main'sstatic.go/handler_recommendations.goswapped back in..avifto only the Lambda switch makesTestCachePolicyPinned/hero.aviffail on thecacheControlForExtassertion while thesetCacheHeadersassertion still passes (the copies diverge).TestCachePolicyParity_ContainerAndLambda/hero.avifalso fails. With the fix, the same one-line edit moves both paths together.confidenceBucketFor, each killed byTestConfidenceBucketFor: 200→199, 3→2, 50→51,>=→>on high savings, and removing the count clamp.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope
cloud-commitments-go/pkgand importingexchange.Confidence*(see above).RecommendationDetailResponse/confidenceBucketForcomments still say the heuristic "previously lived" infrontend/src/recommendations.ts. That is historically accurate (the copy no longer exists), so I left them.No frontend change is needed.
Maintainer: please mirror the issue's triage labels on this PR:
impact/internal,type/chore,triaged,priority/p3,severity/low,urgency/eventually,effort/xs.🤖 Generated with Claude Code