fix: keep legal inventory synchronized with upstream updates - #48
Conversation
Summary by CodeRabbit
WalkthroughChangesThird-party inventory automation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRsPossibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/update-third-party-inventory.py`:
- Around line 144-153: Validate repository notice paths with the shared
resolve_notice_path helper before use: replace the direct root / "third_party" /
relative joins in both check_inventory and update_inventory with
resolve_notice_path(root, relative, component.get('id', '<unknown>')). Keep the
existing file validation and atomic write behavior unchanged.
- Around line 57-92: Add unit tests for validate_notice_url, derive_notice_url,
and parse_versions, covering HTTPS enforcement, embedded credentials, exactly
one version occurrence, valid derivation, and malformed KEY=VALUE inputs.
Exercise both accepted and rejected cases, asserting the expected validation
failures and parsed or derived values without involving network or file-output
behavior.
- Around line 76-92: Add defense-in-depth host validation inside download_notice
before urllib.request.urlopen, restricting the parsed HTTPS URL hostname to the
approved allowlist such as raw.githubusercontent.com. Reject any URL with a
non-allowlisted hostname through fail, while preserving the existing HTTPS
redirect, size, and empty-content checks.
- Around line 116-206: Extract the shared component and repository-notice shape
validation from check_inventory and update_inventory into a helper, and call it
from both flows before any version processing or downloads. Have the helper
validate version_key, notices, path, and reviewed_from, while tracking seen
version keys so duplicate keys are rejected identically in both paths. Preserve
each function’s existing version-alignment and update 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c831bad1-514c-4083-8c4f-53af0a2bfd91
📒 Files selected for processing (5)
.github/workflows/build-amd64.yml.github/workflows/check-upstream.ymlscripts/update-third-party-inventory.pythird_party/README.mdthird_party/inventory.json
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (2)
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: Review GitHub Actions for least-privilege permissions, safe event triggers, untrusted input handling,
accidental publication, tag-channel correctness, secret exposure, reproducible builds, SBOM/provenance
generation and appropriate validation before pushing images.
Files:
.github/workflows/build-amd64.yml.github/workflows/check-upstream.yml
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Check that documentation matches the implemented behavior and clearly distinguishes experimental edge builds
from stable releases. Flag outdated versions, unsafe deployment guidance and claims not proven by CI or testing.
Files:
third_party/README.md
🪛 ast-grep (0.45.0)
scripts/update-third-party-inventory.py
[warning] 79-79: Request-controlled URL passed to urlopen; validate against an allowlist to prevent SSRF.
Context: urllib.request.urlopen(request, timeout=30)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(urlopen-unsanitized-data)
[info] 111-111: use jsonify instead of json.dumps for JSON output
Context: json.dumps(data, indent=2, ensure_ascii=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.16.0)
scripts/update-third-party-inventory.py
[warning] 22-22: Avoid specifying long messages outside the exception class
(TRY003)
[error] 78-78: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 80-80: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[warning] 116-116: Too many branches (13 > 12)
(PLR0912)
[warning] 156-156: Too many branches (14 > 12)
(PLR0912)
🔇 Additional comments (7)
scripts/update-third-party-inventory.py (2)
20-113: LGTM!
116-243: LGTM on the remaining check/update/CLI logic. The write-then-revalidate flow inmain()(Lines 232-234) correctly re-reads the file from disk before revalidating, andupdate_inventorydefers all disk writes until after every notice download succeeds, so a mid-loop failure cannot leave a partial commit..github/workflows/build-amd64.yml (1)
31-32: LGTM!.github/workflows/check-upstream.yml (2)
265-287: LGTM!
305-305: LGTM!third_party/inventory.json (1)
3-3: LGTM!Also applies to: 44-51, 76-83
third_party/README.md (1)
30-31: 📐 Maintainability & Code QualityDaily cadence matches the workflow schedule.
on.schedule.cron: "17 5 * * *"schedules the upstream workflow daily at 05:17 UTC.
eXPerience83
left a comment
eXPerience83
left a comment
There was a problem hiding this comment.
Addressed the bounded actionable feedback on the current HEAD:
- added deterministic unit tests for parsing, version-specific URL validation/derivation, approved hosts, path containment and duplicate keys;
- restricted both initial downloads and redirects to the explicit
raw.githubusercontent.comallowlist; - confined repository notice writes to
third_party/; - reject duplicate
version_keyentries before any download; - separated automated
refreshed_onfrom humanreviewed_on.
The larger helper-extraction refactor was intentionally not added: the concrete behavior gap identified by the review is fixed and tested, while restructuring both validation paths would expand this hotfix without changing its contract.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 508b609ebb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for path, content in pending_files: | ||
| atomic_write_bytes(path, content) | ||
|
|
||
| inventory["refreshed_on"] = datetime.now(timezone.utc).date().isoformat() |
There was a problem hiding this comment.
When an automation PR remains open past UTC midnight, check-upstream.yml resets the local automation branch to main and processes the same still-unmerged version change again. This line then produces a different tree solely because the date changed, so the tree comparison in .github/workflows/check-upstream.yml lines 289-295 cannot reuse the existing remote commit and instead force-pushes and dispatches another build every day. Preserve the candidate's original refresh date or otherwise make repeated generation deterministic when the versions and notices are unchanged.
Useful? React with 👍 / 👎.
1 participant
Summary
Repair the
mainmismatch introduced by merging the legal inventory before the latest upstream version update, and prevent normal automated updates from opening CI-broken pull requests.2.97.0and mise2026.7.18withthird_party/inventory.json;refreshed_onfrom humanreviewed_on;third_party/;check-upstream.ymlfor runtime pins, digests, locks and notices.Bounded behavior
For an existing component, the daily workflow updates its version, derives the new exact
reviewed_fromURL from the previous version-specific URL, downloads repository-preserved legal documents and commits all resulting changes together. A completely new tool still requires an explicit inventory entry.This PR remains declarative. It does not add a Docker, shell or package-manager parser.
Validation
unittestcoverage for the updater contract;--checkvalidation;After this PR merges, Renovate PR #45 should be rebased/retried; its Dockerfile frontend update is not the source of the original failure.
Refs #26