chore: enforce supply chain age gating#10
Conversation
PR Review: chore: enforce supply chain age gatingSummary: The intent - adding a quarantine window so newly published (or hijacked) packages cannot enter the dependency tree - is a solid supply chain security practice. However, the implementation has a critical issue that prevents it from working as described. Critical Issue: Invalid exclude-newer value The file sets exclude-newer to "7 days". exclude-newer does not accept relative duration strings. It requires an absolute RFC 3339 timestamp (e.g., 2026-03-25T00:00:00Z) or a date string (e.g., 2026-03-25). The value "7 days" is not a valid format and will cause a parse error or be silently ignored depending on the uv version. This means the PR does not achieve its stated goal - there is no rolling 7-day quarantine mechanism built into uv. What exclude-newer is actually designed for: It pins dependency resolution to a fixed point in time for reproducibility - e.g., only consider packages that existed before this date. It is not a sliding window. Recommendations:
Other Notes:
Verdict: Request changes. The current value is invalid and will not provide the intended protection. Please update to a valid RFC 3339 timestamp and consider which approach (static pin vs. dynamic CI step) best fits the project workflow. Reviewed by Claude |
PR Review: chore: enforce supply chain age gatingOverall AssessmentLooks good. This is a small, well-motivated supply chain security hardening change. A few points to consider before merging: Code Quality & Best Practices
Potential Issues
Security
Performance
Test Coverage
SummaryThe idea is solid, but please verify that 🤖 Generated with Claude Code |
PR Review: chore: enforce supply chain age gatingOverall AssessmentApprove. The third commit fixes the critical issue flagged in previous reviews (invalid relative duration Code Quality & Best Practices
Potential Issues
Security
Performance
Test Coverage
SummaryThe fix in the third commit resolves the original blocker. The main remaining concern is operational: without a process to periodically advance the date, it will drift and eventually block legitimate dependency updates. Consider adding automation or at least documenting the expected cadence before merging. 🤖 Generated with Claude Code |
Code Review — PR #10: Supply Chain Age GatingGood idea overall — a 7-day quarantine on new packages is a practical supply chain defense, and the evolution from a static config to a dynamic CI check was the right call. A few issues worth addressing before merging: 🔴 Security:
|
| Finding | Severity | Actionable? |
|---|---|---|
setup-uv not pinned to SHA |
High | Yes — pin it |
Missing permissions block |
Medium | Yes — add contents: read |
Narrow paths trigger |
Low | Document or widen later |
| macOS date fallback | Nit | Optional cleanup |
| Extra blank line in pyproject.toml | Nit | Optional cleanup |
CI is green. The concept is sound — the main action item is pinning the setup-uv action to a commit hash for consistency with the rest of the repo's supply chain posture.
Code Review — PR #10: chore: enforce supply chain age gatingGood initiative — supply chain age gating is a sound defensive measure. Pinned action SHAs, minimal 1. PR description / actual change mismatchThe PR body says "Adds 2. Project doesn't use
|
Code Review — PR #10: chore: enforce supply chain age gating (final state)Previous reviews drove meaningful improvements — the PR has evolved from an invalid static config to a well-structured dynamic CI check. Reviewing the final state of the single added file ( What's Good
Remaining Issues1.
|
| Finding | Severity | Status |
|---|---|---|
| SHA-pinned actions | — | Done |
| Minimal permissions | — | Done |
| Timeout | — | Done |
| Dynamic cutoff | — | Done |
| Failure message UX | Low | Suggested improvement |
| In-file override docs | Low | Suggested improvement |
| Ephemeral lockfile | Info | Note for future |
| Unchecked test plan item | Info | Recommend completing |
| Resolution vs. install gap | Info | Documenting for team |
Verdict: Approve with minor suggestions. The implementation is solid and addresses all previously raised concerns. The two low-severity items (failure message UX, in-file override docs) are nice-to-haves that would improve developer experience but aren't blockers. Good supply chain hardening contribution.
🤖 Generated with Claude Code
Supply chain hardening — reject PyPI packages published less than 7 days ago via uv's exclude-newer setting. Override for emergency updates: uv lock --exclude-newer "0 seconds" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move exclude-newer from standalone uv.toml into [tool.uv] section of pyproject.toml, consistent with how ruff, mypy, and pytest are already configured. One fewer file in the repo root. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
exclude-newer requires an absolute RFC 3339 timestamp or date string, not a relative duration like "7 days". The previous value was silently ignored or would error depending on uv version. Set to a fixed date (2026-04-03) with a comment explaining the update cadence and override mechanism. uv does not support rolling windows — this date should be bumped periodically (e.g., weekly via CI or manual). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
A fixed timestamp in pyproject.toml goes stale — if nobody bumps it, uv lock silently refuses to resolve packages published after the date, causing confusing "no matching version" errors months later. Instead: - Remove [tool.uv] exclude-newer from pyproject.toml (no stale date risk for local dev) - Add dependency-age-check.yml CI workflow that computes a 7-day-ago cutoff dynamically on every PR that touches pyproject.toml - uv lock --exclude-newer <cutoff> fails if any dependency was published in the last 7 days This enforces the quarantine where it matters (merge gate) without breaking local development. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CR feedback: 1. Pin astral-sh/setup-uv to commit SHA (d0d8abe) instead of mutable v6 tag — ironic to have an unpinned action in a supply chain PR 2. Add permissions: contents: read — restrict workflow to minimum required permissions 3. Add uv.lock to paths trigger — catches lockfile-only updates if the project adopts uv.lock in the future 4. Remove macOS date fallback — runs on ubuntu-latest only, GNU coreutils date is sufficient 5. Remove spurious blank line in pyproject.toml from [tool.uv] removal Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CR feedback: 1. Add timeout-minutes: 5 — prevents 6-hour hang if PyPI is slow 2. Add comment explaining uv is used only as a resolver, not as the project's package manager (project uses pip/hatchling) PR description also updated to match actual implementation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wrap uv lock in an if-block with ::error:: annotations so developers see a clear explanation when the age gate fails, not just a cryptic "no matching version" from uv. Add inline override instructions so they're visible where CI is red, not buried in a PR description. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d6f80b3 to
1a375e6
Compare
CR feedback addressedFixed
Deferred
Also rebased onto current main. |
Code Review: PR #10 — chore: enforce supply chain age gatingOverall AssessmentSolid supply chain security addition. The approach of using Positive Highlights
Issues & Suggestions1. Spurious
|
Project uses pip/hatchling, not uv — the uv.lock path trigger would never fire. Replace with workflow_dispatch for manual audit runs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CR feedback addressedFixed
Deferred
|
Code Review: PR #10 — chore: enforce supply chain age gatingOverallGood supply-chain hardening. The approach of using Positives
Issues & Suggestions1. Resolution gap between uv and pip (low severity)
This is an inherent limitation of the approach and probably acceptable for the stated threat model (catching entirely-new or no-history packages). Worth noting in a comment in the workflow so future maintainers understand the boundary. 2. Missing trigger on
|
Matches ci.yml pattern — catches dependency changes that bypass PR checks (API merges, loosened branch protection). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CR feedback addressed (round 2)Fixed
Deferred
|
Code Review: chore: enforce supply chain age gatingOverall: This is a solid supply chain security measure. The approach of using Positives
Suggestions
SecurityNo concerns — this workflow is well-scoped. It reads the repo, runs a resolver, and exits. No secrets, no write permissions, no artifacts. The 7-day quarantine is a reasonable heuristic for catching typosquatting and hijacked packages. VerdictLooks good. The suggestions above are minor improvements, none are blockers. Nice addition to the project's security posture. Reviewed with Claude Code |
The age check CI uses uv as a resolver only — the project uses pip/hatchling. Prevent accidental commits if someone runs uv lock locally for debugging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Review: chore: enforce supply chain age gatingOverall AssessmentThis is a well-executed supply chain hardening measure. The approach — using Positives
Suggestions & Considerations1. Squash merge recommendedThe branch has 10 commits, several using 2. Optional/dev dependency coverage
3. Path filter gap for
|
Ten items from the latest PR review. Reviewer marked #1 as blocking and #8 + #10 as strongly recommended; the rest picked up on the "don't be lazy" directive. 1. batch_create 400 shape guard (reviewer's blocking item). _utils.py gains _validate_batch_create_shape() which verifies that a passthrough 400 body has the expected BatchCreateOutput envelope (succeeded/failed are ints, results is a list, each entry carries a boolean success discriminant). If the API ever returns 400 with a different body (plain error envelope, proxy error, malformed JSON), batch_create now raises a QURLError with status=0 and code="unexpected_response" instead of silently returning (succeeded=0, failed=0, results=[]). Defense in depth matches the qurl-typescript fix. Wired into both client.py and async_client.py. 2. QURLError docstring now documents that .detail is guaranteed non-empty at the instance level. The constructor falls back to title when the API omits detail per RFC 7807, so consumers shouldn't inspect .detail to detect "was it absent?" — use .code / .status / .type instead. 3. QURLError docstring now explains why .type shadows Python's built-in. Intentional for RFC 7807 field-name parity and consistency with qurl-typescript/qurl-mcp; the shadowing only matters inside QURLError method definitions, not external code. 4. target_url scheme check in validate_create_input. Reviewer's observation that the length check didn't catch the most common mistake (forgetting http(s)://). New _ALLOWED_URL_SCHEMES tuple with a startswith() guard; the server still owns SSRF validation. 5. Sync/async parity comment added to client.py's module docstring (async_client.py already had one). Calls out the contract so a future change can't silently update one client without the other. 6. Tag regex comment expanded with a note about keeping it in lockstep with the openapi.yaml schema, and why. 7. Quota.plan empty-string default now documented — it only exists so the dataclass can be instantiated with no arguments for tests/ bootstrap paths; the real /v1/quota endpoint always returns a populated plan. Tests (108 -> 116): - test_get_response_parses_nested_ai_agent_policy (reviewer gap #8) — mocks a GET response with a fully-populated ai_agent_policy inside a token's access_policy and asserts the deserialization round-trip. - test_list_serializes_datetime_filter_params_as_isoformat (reviewer gap #9) — passes an actual datetime to client.list(created_after=) and asserts the URL-encoded ISO 8601 output. - test_async_delete_rejects_q_prefix_client_side (reviewer gap #10) — async symmetry for the existing sync delete() q_ prefix test. - test_create_rejects_target_url_without_scheme — the new URL scheme check catches bare "example.com". - test_create_rejects_target_url_with_unsupported_scheme — rejects ftp:// etc. - test_create_accepts_http_and_https_schemes — both valid schemes pass. - test_batch_create_rejects_unexpected_400_body_shape — defense-in- depth for the new _validate_batch_create_shape. - test_batch_create_rejects_400_body_with_non_boolean_success — the per-entry discriminant check. Also updated three existing tests that depended on the pre-URL-check create() accepting invalid URLs: - test_422_raises_validation_error - test_400_raises_validation_error - test_batch_create_partial_failure Each now uses a syntactically valid URL that passes client-side validation; the mocked API response payload is unchanged, so the tests still exercise the API error-parsing paths they intended to. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
dependency-age-check.yml) that rejects dependencies published in the last 7 daysuv lock --exclude-newer <7-days-ago>as a standalone resolver to validate dependency agesuvis only used as a validation tool hereWhy
Newly published or hijacked packages are typically detected and removed within days. A 7-day quarantine at the CI merge gate ensures compromised packages never enter our dependency tree.
Override for critical updates
Test plan
pyproject.tomluv lock --exclude-newer <RFC3339 timestamp>resolves successfully for current depscontents: read🤖 Generated with Claude Code