Apply quality-of-life fixes

[sangwa]

• refactor: use dustin/go-humanize for http.cache.size parsing
• fix: default bind address to 127.0.0.1 instead of 0.0.0.0
• feat: make HTTP cache default TTL configurable (http.cache.default_ttl)
• fix: fail on invalid duration and byte-size config values

Summary by CodeRabbit

• Configuration

  • Default server bind address changed from 0.0.0.0 to 127.0.0.1.
  • New http.cache.default_ttl setting added (default 1h) to control fallback cache TTL.

• Improvements

  • Revocation collateral and endorsement caching now use the configurable default TTL and enforce a maximum cap.
  • Duration and byte-size parsing strengthened (broader format support, stricter validation/overflow checks).

• Tests

  • Expanded TTL and byte-size parsing tests and fuzz corpus.

• Chores

  • Updated Go module dependency declarations.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

The PR narrows the server default bind address from 0.0.0.0 to 127.0.0.1, adds http.cache.default_ttl = "1h" with an environment binding, and exposes it as Config.HTTPCacheDefaultTTL. Duration and byte-size parsing moved to helpers that return parse errors (no silent fallbacks); byte-size parsing now uses go-humanize with overflow checks. Cache TTL logic for external fetches (cosign, endorsements, TDX revocation) now uses the configured default TTL (capped) when headers are absent. Two modules were promoted from indirect to direct requires in go.mod. Tests were updated and expanded.

Sequence Diagram(s)

sequenceDiagram
  participant Client as Client
  participant Server as Server
  participant Fetcher as Fetcher (cachedHTTPSGetter)
  participant External as External API
  participant Cache as HTTP Cache

  Client->>Server: Request resource
  Server->>Fetcher: Request external data
  Fetcher->>Cache: Check cache for item
  alt Cache hit
    Cache-->>Fetcher: Return cached response + TTL
  else Cache miss
    Fetcher->>External: HTTP request
    External-->>Fetcher: HTTP response (with/without Cache-Control/Expires)
    Fetcher->>Fetcher: parseCacheTTL(response headers, defaultTTL)
    Fetcher->>Cache: Store response with computed TTL
  end
  Fetcher-->>Server: Return data
  Server-->>Client: Respond

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I nudged the host to cozy loopback ground,
An hour of TTL in config found,
Parsers tidy, bytes counted true,
Cache will sleep until headers coo,
Hop, fetch, and stash — a rabbit's bound.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 58.82% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title "Apply quality-of-life fixes" is vague and generic, using a non-descriptive term that doesn't convey the specific changes in the PR.	Consider a more specific title that highlights the primary changes, such as "Make HTTP cache default TTL configurable and fix default bind address" or "Improve cache configuration with default TTL and tighten server binding."

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch qol-fixes

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

internal/endorsements_test.go (1)

384-386: Use a non-default sentinel TTL in the no-header case.

This still exercises the new parameter with time.Hour, so an implementation that ignores defaultTTL and hardcodes 1h would also pass. Giving that case its own distinct TTL would actually lock down the new behavior.

Also applies to: 425-425

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/endorsements_test.go` around lines 384 - 386, The test case "no
headers returns default" currently uses time.Hour (the existing default) which
masks implementations that ignore the provided defaultTTL; change the sentinel
TTL in that case to a distinct non-default value (e.g., time.Minute or another
constant) so the test exercises the defaultTTL parameter, and make the same
change for the other occurrence referenced (the case at the later line). Update
the test cases in endorsements_test.go that set want: time.Hour for the
no-headers scenarios to use the new sentinel TTL and ensure any helper variables
(e.g., defaultTTL) or assertions still compare against that sentinel value.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/config.go`:
- Around line 335-339: The code converts humanize.ParseBytes result (n uint64)
to int64 without checking overflow; add a guard after ParseBytes in the same
function (where n, err := humanize.ParseBytes(s) is used) that returns an error
if n > uint64(math.MaxInt64) (use math.MaxInt64) so the function returns a clear
overflow error instead of silently truncating/producing a negative int64; keep
the existing error message style (e.g., fmt.Errorf) when returning the overflow
error.

In `@internal/cosign.go`:
- Around line 134-136: The code currently forces a default cache TTL whenever
ttl <= 0 which overwrites explicit non-cacheable or already-expired responses;
change the condition so the default is applied only when ttl == 0 (unspecified),
and preserve ttl when it's negative (explicit no-cache/no-store/expired). Update
the check that references ttl and s.cfg.HTTPCacheDefaultTTL in
internal/cosign.go so it sets ttl = s.cfg.HTTPCacheDefaultTTL only for ttl == 0
and does not override ttl < 0.

In `@internal/endorsements.go`:
- Around line 104-106: The current check in endorsements.go overrides a TTL of 0
(which parseCacheTTL uses to mean "no-cache"/"no-store"/expired), causing
responses that should not be cached to get the default TTL; locate the code that
calls parseCacheTTL (reference parseCacheTTL and the local variable ttl) and
change the conditional so you only apply s.cfg.HTTPCacheDefaultTTL when ttl is
negative (e.g. change "if ttl <= 0" to "if ttl < 0"), preserving ttl==0 values
returned intentionally by parseCacheTTL.

In `@internal/fetch.go`:
- Around line 123-124: The function parseCacheTTL currently returns defaultTTL
as-is, allowing configured defaults >24h to bypass the documented cap; update
parseCacheTTL to clamp defaultTTL to a maximum of 24 hours (time.Hour*24) at the
start (or before any early return) so any return path that yields defaultTTL
respects the 24h cap; ensure existing logic that parses the Expires header still
applies the same cap to parsed values as well.

---

Nitpick comments:
In `@internal/endorsements_test.go`:
- Around line 384-386: The test case "no headers returns default" currently uses
time.Hour (the existing default) which masks implementations that ignore the
provided defaultTTL; change the sentinel TTL in that case to a distinct
non-default value (e.g., time.Minute or another constant) so the test exercises
the defaultTTL parameter, and make the same change for the other occurrence
referenced (the case at the later line). Update the test cases in
endorsements_test.go that set want: time.Hour for the no-headers scenarios to
use the new sentinel TTL and ensure any helper variables (e.g., defaultTTL) or
assertions still compare against that sentinel value.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5c300660-fd7c-4163-83da-8f6bd23d7d4d

📥 Commits

Reviewing files that changed from the base of the PR and between 56c9322 and ad7ddbf.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (10)

• CLAUDE.md
• cmd/root.go
• config/config.toml
• go.mod
• internal/config.go
• internal/cosign.go
• internal/endorsements.go
• internal/endorsements_test.go
• internal/fetch.go
• internal/server.go

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/config.go`:
- Around line 316-325: Update the comment for parseDuration to accurately
reflect its behavior: state that it reads a viper string key and parses it as a
time.Duration and returns an error if the value is empty or cannot be parsed
(i.e., for empty strings and any unparseable duration strings). Reference the
parseDuration function and ensure the docstring no longer implies empty values
are allowed.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 65248c58-2399-481a-ad3a-db093a1ac4ec

📥 Commits

Reviewing files that changed from the base of the PR and between ad7ddbf and 8b20cae.

📒 Files selected for processing (5)

• internal/config.go
• internal/cosign.go
• internal/endorsements.go
• internal/endorsements_test.go
• internal/fetch.go

✅ Files skipped from review due to trivial changes (1)

• internal/fetch.go

🚧 Files skipped from review as they are similar to previous changes (3)

• internal/cosign.go
• internal/endorsements.go
• internal/endorsements_test.go

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

internal/config.go (1)

316-344: Please add tests for these new startup-failure paths.

internal/config_test.go still doesn't cover invalid durations or byte-size overflow, even though this code now decides whether the server starts. A small table-driven test here would lock in the new fail-fast contract.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/config.go` around lines 316 - 344, Add table-driven tests in
internal/config_test.go to cover the new fail-fast paths in parseDuration and
parseByteSize: write cases that pass an empty/invalid duration string to
parseDuration (e.g., "" and "5xs") and assert it returns a non-nil error, and
cases for parseByteSize that pass empty string and an oversized value (a string
that parses to > math.MaxInt64) and assert errors; call the functions directly
(parseDuration, parseByteSize) and verify the error messages contain the
problematic input to lock in the behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/config.go`:
- Around line 118-136: Parsed durations from parseDuration (used to populate
endorsementTimeout, httpCacheDefaultTTL, revocationRefreshInterval,
rateLimitStallTimeout, etc.) can be negative and must be rejected; after each
parseDuration call check if the returned duration is < 0 and return an error
(e.g., fmt.Errorf("%s: negative duration %s", "<config.key>", d)) consistent
with existing parseByteSize error handling so invalid negative timeouts/TTLs
fail fast. Ensure you update all places that call parseDuration in this block
and include the specific config key name in the error message to aid debugging.

---

Nitpick comments:
In `@internal/config.go`:
- Around line 316-344: Add table-driven tests in internal/config_test.go to
cover the new fail-fast paths in parseDuration and parseByteSize: write cases
that pass an empty/invalid duration string to parseDuration (e.g., "" and "5xs")
and assert it returns a non-nil error, and cases for parseByteSize that pass
empty string and an oversized value (a string that parses to > math.MaxInt64)
and assert errors; call the functions directly (parseDuration, parseByteSize)
and verify the error messages contain the problematic input to lock in the
behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 187685cb-3fa3-4cdf-92a5-7a0002beaef8

📥 Commits

Reviewing files that changed from the base of the PR and between 8b20cae and c601b56.

📒 Files selected for processing (1)

• internal/config.go

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/config.go`:
- Around line 118-121: parseDuration currently accepts zero which breaks callers
that require positive durations (RevocationRefreshInterval used by
time.NewTicker, EndorsementClientTimeout used with
context.WithTimeout/http.Client.Timeout, and RateLimitStallTimeout used with
context.WithTimeout). Fix by making parseDuration return an error for zero
durations (treat zero as invalid) and update callers (e.g., where
endorsementTimeout is assigned in internal/config.go) to handle that error path;
alternatively, if you prefer per-callsite validation, add explicit checks that
parsed values for RevocationRefreshInterval, EndorsementClientTimeout, and
RateLimitStallTimeout are > 0 and return a clear error instead of allowing zero
to propagate.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2f34da73-1561-497b-b113-4124e2d77367

📥 Commits

Reviewing files that changed from the base of the PR and between c601b56 and ae5e012.

📒 Files selected for processing (2)

• internal/config.go
• internal/config_test.go

✅ Files skipped from review due to trivial changes (1)

• internal/config_test.go

[coderabbitai]

🧹 Nitpick comments (1)

internal/config.go (1)

122-146: Consider extracting a parsePositiveDuration helper to reduce repetition.

The pattern of calling parseDuration followed by a zero-check is repeated three times for fields that require positive values. A dedicated helper could consolidate this:

func parsePositiveDuration(key string) (time.Duration, error) {
    d, err := parseDuration(key)
    if err != nil {
        return 0, err
    }
    if d == 0 {
        return 0, fmt.Errorf("%s: must be positive", key)
    }
    return d, nil
}

This is a minor DRY improvement and entirely optional.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/config.go` around lines 122 - 146, Extract a helper named
parsePositiveDuration that wraps parseDuration and enforces a non-zero result
(signature: parsePositiveDuration(key string) (time.Duration, error)); replace
the repeated parseDuration + zero-check blocks (used for keys
"http.cache.default_ttl", "revocation.refresh_interval", and
"ratelimit.stall_timeout") with calls to parsePositiveDuration, and update the
surrounding code to return the helper's error directly where those durations are
assigned (e.g., httpCacheDefaultTTL, revocationRefreshInterval,
rateLimitStallTimeout).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/config.go`:
- Around line 122-146: Extract a helper named parsePositiveDuration that wraps
parseDuration and enforces a non-zero result (signature:
parsePositiveDuration(key string) (time.Duration, error)); replace the repeated
parseDuration + zero-check blocks (used for keys "http.cache.default_ttl",
"revocation.refresh_interval", and "ratelimit.stall_timeout") with calls to
parsePositiveDuration, and update the surrounding code to return the helper's
error directly where those durations are assigned (e.g., httpCacheDefaultTTL,
revocationRefreshInterval, rateLimitStallTimeout).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 20152d2d-0a75-4bf6-ba55-ee24d285077b

📥 Commits

Reviewing files that changed from the base of the PR and between ae5e012 and 11b3304.

📒 Files selected for processing (1)

• internal/config.go