qa: manifest-check tool for #2225 (failed to download manifest) - #354
qa: manifest-check tool for #2225 (failed to download manifest)#354DTTerastar wants to merge 2 commits into
Conversation
Ports the manifest endpoint probe from ESPresense/ESPresense#2320 into the repo that actually serves the endpoints. The tool validates the ESP Web Tools manifest schema and HEADs each binary URL for every (version, flavor) in targets.txt. CI workflow runs unit tests + a live production probe on PR, push, hourly cron, and dispatch. The probe currently FAILS on production: latest.json?flavor=* returns 404 (tag 'latest' is not a real Git tag; the manifest handler hits /releases/tags/latest instead of /releases/latest). The fix for the handler follows in a separate commit on this branch. Refs: #2225, ESPresense/ESPresense#2320.
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughChangesManifest validation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Deploying espresense with
|
| Latest commit: |
7b0a961
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0d398af6.espresense.pages.dev |
| Branch Preview URL: | https://manifest-check-2225.espresense.pages.dev |
![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 @.github/workflows/manifest-check.yml:
- Around line 46-47: Remove or defer the “Probe manifest URLs” workflow step
that runs tools/manifest_check/check_manifest.py until the endpoint and binary
asset fixes provide a green baseline; do not enable it as a required production
gate while it reports known failures.
In `@tools/manifest_check/check_manifest.py`:
- Around line 90-99: Update the HTTPError handling in the retry loop to treat
HTTP 429 as transient instead of returning immediately, while preserving
immediate handling for other non-5xx responses. When retrying 429 responses,
read and honor the Retry-After header when present, falling back to the existing
backoff schedule otherwise, and keep the final-attempt behavior intact.
- Around line 166-180: Constrain URL handling in the manifest-check loop around
`parts[].path` and `http_request`: preserve local paths resolved against the
documented `--base`, but allow only explicitly permitted schemes and origins for
the initial URL. Configure or extend `http_request` so redirects are validated
against the same allowlist rather than followed to arbitrary destinations,
rejecting disallowed URLs before any request is made.
- Around line 105-125: Update validate_schema to first reject payloads that are
not dictionaries before checking REQUIRED_TOP_KEYS, and validate each part’s
path field as a string so null or other non-string values return a schema error
before startswith() is reached. Add regression cases covering a scalar JSON
payload and a part with a non-string path.
🪄 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: 017eb28a-4738-424d-a956-e07574bcd8da
📒 Files selected for processing (6)
.github/workflows/manifest-check.ymltools/manifest_check/.gitignoretools/manifest_check/README.mdtools/manifest_check/check_manifest.pytools/manifest_check/targets.txttools/manifest_check/test_check_manifest.py
| - name: Probe manifest URLs | ||
| run: python3 tools/manifest_check/check_manifest.py |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not enable a known-red production gate.
This command currently reports 31 failures: latest manifests return 404/429 and multiple versioned binary URLs return 404. The planned latest handler follow-up alone will not clear the stale-binary failures. Land the endpoint/asset fixes before enabling this required probe, or defer the workflow until it has a green baseline.
🤖 Prompt for 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.
In @.github/workflows/manifest-check.yml around lines 46 - 47, Remove or defer
the “Probe manifest URLs” workflow step that runs
tools/manifest_check/check_manifest.py until the endpoint and binary asset fixes
provide a green baseline; do not enable it as a required production gate while
it reports known failures.
Source: Pipeline failures
| def validate_schema(payload: dict) -> str | None: | ||
| for key in REQUIRED_TOP_KEYS: | ||
| if key not in payload: | ||
| return f"missing top-level key: {key}" | ||
| builds = payload.get("builds") | ||
| if not isinstance(builds, list) or not builds: | ||
| return "builds[] must be a non-empty list" | ||
| for idx, build in enumerate(builds): | ||
| if not isinstance(build, dict): | ||
| return f"builds[{idx}] is not an object" | ||
| if "chipFamily" not in build: | ||
| return f"builds[{idx}] missing chipFamily" | ||
| parts = build.get("parts") | ||
| if not isinstance(parts, list) or not parts: | ||
| return f"builds[{idx}].parts must be a non-empty list" | ||
| for pidx, part in enumerate(parts): | ||
| if not isinstance(part, dict): | ||
| return f"builds[{idx}].parts[{pidx}] is not an object" | ||
| for key in REQUIRED_PART_KEYS: | ||
| if key not in part: | ||
| return f"builds[{idx}].parts[{pidx}] missing {key}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject non-object JSON and non-string part paths.
A scalar JSON payload can raise at key not in payload, and "path": null passes validation then crashes at Line 173 on startswith(). Return a schema failure instead, and add regression cases for both inputs.
Proposed fix
-def validate_schema(payload: dict) -> str | None:
+def validate_schema(payload: object) -> str | None:
+ if not isinstance(payload, dict):
+ return "manifest must be a JSON object"
for key in REQUIRED_TOP_KEYS:
if key not in payload:
return f"missing top-level key: {key}"
...
for key in REQUIRED_PART_KEYS:
if key not in part:
return f"builds[{idx}].parts[{pidx}] missing {key}"
+ if not isinstance(part["path"], str) or not part["path"]:
+ return f"builds[{idx}].parts[{pidx}].path must be a non-empty string"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def validate_schema(payload: dict) -> str | None: | |
| for key in REQUIRED_TOP_KEYS: | |
| if key not in payload: | |
| return f"missing top-level key: {key}" | |
| builds = payload.get("builds") | |
| if not isinstance(builds, list) or not builds: | |
| return "builds[] must be a non-empty list" | |
| for idx, build in enumerate(builds): | |
| if not isinstance(build, dict): | |
| return f"builds[{idx}] is not an object" | |
| if "chipFamily" not in build: | |
| return f"builds[{idx}] missing chipFamily" | |
| parts = build.get("parts") | |
| if not isinstance(parts, list) or not parts: | |
| return f"builds[{idx}].parts must be a non-empty list" | |
| for pidx, part in enumerate(parts): | |
| if not isinstance(part, dict): | |
| return f"builds[{idx}].parts[{pidx}] is not an object" | |
| for key in REQUIRED_PART_KEYS: | |
| if key not in part: | |
| return f"builds[{idx}].parts[{pidx}] missing {key}" | |
| def validate_schema(payload: object) -> str | None: | |
| if not isinstance(payload, dict): | |
| return "manifest must be a JSON object" | |
| for key in REQUIRED_TOP_KEYS: | |
| if key not in payload: | |
| return f"missing top-level key: {key}" | |
| builds = payload.get("builds") | |
| if not isinstance(builds, list) or not builds: | |
| return "builds[] must be a non-empty list" | |
| for idx, build in enumerate(builds): | |
| if not isinstance(build, dict): | |
| return f"builds[{idx}] is not an object" | |
| if "chipFamily" not in build: | |
| return f"builds[{idx}] missing chipFamily" | |
| parts = build.get("parts") | |
| if not isinstance(parts, list) or not parts: | |
| return f"builds[{idx}].parts must be a non-empty list" | |
| for pidx, part in enumerate(parts): | |
| if not isinstance(part, dict): | |
| return f"builds[{idx}].parts[{pidx}] is not an object" | |
| for key in REQUIRED_PART_KEYS: | |
| if key not in part: | |
| return f"builds[{idx}].parts[{pidx}] missing {key}" | |
| if not isinstance(part["path"], str) or not part["path"]: | |
| return f"builds[{idx}].parts[{pidx}].path must be a non-empty string" |
🤖 Prompt for 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.
In `@tools/manifest_check/check_manifest.py` around lines 105 - 125, Update
validate_schema to first reject payloads that are not dictionaries before
checking REQUIRED_TOP_KEYS, and validate each part’s path field as a string so
null or other non-string values return a schema error before startswith() is
reached. Add regression cases covering a scalar JSON payload and a part with a
non-string path.
| seen_paths: set[str] = set() | ||
| for build in payload["builds"]: | ||
| for part in build["parts"]: | ||
| path = part["path"] | ||
| if path in seen_paths: | ||
| continue | ||
| seen_paths.add(path) | ||
| if path.startswith(("http://", "https://")): | ||
| part_url = path | ||
| else: | ||
| part_url = urllib.parse.urljoin(base + "/", path.lstrip("/")) | ||
| try: | ||
| pstatus, _, _ = http_request( | ||
| part_url, method="HEAD", timeout=timeout, retries=retries, backoff=backoff | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Constrain manifest-provided binary URLs.
parts[].path is remote input. Absolute URLs are fetched directly, and other schemes can survive urljoin; redirects are also followed automatically. A compromised manifest can make the CI runner probe arbitrary internal or external endpoints. Keep the documented local --base flow, but enforce an allowlist of permitted origins/schemes for initial URLs and redirects.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 172-172: Do not make http calls without encryption
Context: "http://"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🤖 Prompt for 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.
In `@tools/manifest_check/check_manifest.py` around lines 166 - 180, Constrain URL
handling in the manifest-check loop around `parts[].path` and `http_request`:
preserve local paths resolved against the documented `--base`, but allow only
explicitly permitted schemes and origins for the initial URL. Configure or
extend `http_request` so redirects are validated against the same allowlist
rather than followed to arbitrary destinations, rejecting disallowed URLs before
any request is made.
Source: Linters/SAST tools
The manifest handler fetched GitHub's /releases/tags/${tag}, but 'latest'
is not a real Git tag — GitHub returns 404, propagated as
{"error":"Release not found"} to the web installer, surfacing as
"Failed to download manifest" (#2225).
Fix: when tag === 'latest', use /releases/latest (the endpoint GitHub
provides for resolving newest non-prerelease), matching what the sibling
/latest/download/:filename route already does. Binary part paths now use
rel.tag_name (the resolved real tag, e.g. v4.0.6) instead of the literal
${tag}, so 'latest' manifests produce working download/v4.0.6/*.bin URLs
rather than nonexistent download/latest/*.bin paths.
Tool fixes surfaced by GHA run #29879346118 (31 failures / 12 targets):
- check_manifest.py: resolve part paths relative to the manifest URL
(not the base URL), matching esp-web-tools client behavior. Previously
produced /download/... instead of /releases/download/... → false 404s.
- check_manifest.py: retry on HTTP 429 (GitHub rate-limit) in addition
to 5xx, preventing false negatives on unauthenticated CI runs.
wrangler.toml: add compatibility_date (was missing; local dev defaulted
to today's date which exceeds the bundled workerd).
Verified locally: wrangler pages dev + check_manifest.py --base
http://localhost:8788 → 12/12 targets pass (was 31 failures).
Refs: #2225, ESPresense/ESPresense#2320.
|
Split into two focused PRs per request:
|
1 participant
Summary
Ports the manifest endpoint probe from ESPresense/ESPresense#2320 into this repo — the one that actually serves the
/releases/{version}.json?flavor={flavor}endpoints viafunctions/releases/[[path]].ts.Why here, not in the firmware repo: the tool tests endpoints served by this repo, the bug it detects lives in
functions/releases/[[path]].ts, and CI signal belongs next to the code under test. In the firmware repo the check fires on firmware PRs that cannot touch the website.What this PR adds
tools/manifest_check/check_manifest.py— stdlib-only probe. GETs each manifest, validates ESP Web Tools schema, HEADs each binary URL.tools/manifest_check/test_check_manifest.py— offline unit tests (6 tests).tools/manifest_check/targets.txt— 12 (version, flavor) pairs coveringlatest+v4.0.6+v4.0.5.tools/manifest_check/README.md,.gitignore..github/workflows/manifest-check.yml— unit tests + production probe on PR / push / hourly cron / dispatch.Current status: RED by design
The production probe intentionally fails in this first commit. Live confirmation:
Root cause: the manifest handler (
functions/releases/[[path]].ts:127) fetches GitHub's/releases/tags/${tag}endpoint. Whentag === 'latest', GitHub returns 404 becauselatestis not a real Git tag. The sibling/latest/download/:filenameroute (line 204) already uses the correct/releases/latestendpoint — the manifest handler was never updated to match.The fix (resolve
latestvia/releases/latest+ userel.tag_namefor binary paths) follows in the next commit on this branch once GHA confirms the red.Test plan
python3 tools/manifest_check/test_check_manifest.py— 6/6 passprobe-manifestjob fails against production (proves the repro)Refs: #2225, ESPresense/ESPresense#2320.
Summary by CodeRabbit
New Features
Documentation
Tests