-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5740
#5740 fix(ci): accept trusted legacy PR metadata by cixzhang (bucket: the maintainer)
bd6aced7fa963d67957c16417c105c4dfaf6dbf3
LOOP VERSION: 2.0.0 AUDIT RUBRIC: 1.15.1
LANE: full WHY: the runtime change is small, but it changes a trusted default-branch workflow that consumes fork/PR artifacts and decides whether to publish report content. The review needed the full trust-boundary pass: exact workflow-run identity, legacy compatibility, exact-field enforcement, fallback-attack rejection, deployment-proof preservation, workflow syntax, focused tests, and CI. PRIOR REVIEW: checked; no human, Copilot, or inline reviews existed before this pass.
CHANGED CODE: the pr-comment.yml artifact identity step now imports the shared PR analysis metadata validator instead of enforcing the newer exact metadata shape inline; focused tests add legacy compatibility and workflow-wiring coverage.
NEAREST CURRENT COMPONENT CONTRACT: none — this is repository CI infrastructure, not a component contract.
CURRENT FAMILY: current default-branch PR comment reconciliation already uses the shared validator for report rendering, and exact deployment-result validation remains separate and strict.
CURRENT API/THEMING/SYSTEM RECORDS: the workflow comments define the trust boundary: resolve PR identity from the GitHub API, check out trusted default-branch code, and treat PR-built artifacts as untrusted data.
DRAFT CONTEXT: none.
STOPPED AT: changed code + current workflow/library family — the existing shared validator is already the intended authority for PR analysis metadata; this PR removes the stricter duplicated inline check from the default-branch workflow.
REGRESSION EVIDENCE: bug fix/regression — a live #5671 CI artifact from run 33293627054 has only the legacy prNumber, shortHash, storybookUrl, sandboxUrl, runId, and runUrl shape; exact-head validation accepts it for #5671's trusted run/head and rejects adversarial mismatches.
OWNER QUESTION (PRIVATE): none.
| candidate issue | classification | evidence | disposition |
|---|---|---|---|
| Accepting legacy metadata could weaken identity checks | preserves |
validateAnalysisMetadata still requires exact PR number, exact source run id, and a 7–40 char lowercase head prefix matching the trusted GitHub API head; adversarial script rejected wrong PR, wrong run, wrong prefix, and missing prefix. |
no finding |
| Optional exact fields might be ignored when present | preserves | exact-head test and adversarial script rejected mismatched headSha, headRepository, baseRepository, and runAttempt when supplied. |
no finding |
| Fallback artifact from another run/PR could be rendered | preserves | workflow downloads pr-analysis from github.event.workflow_run.id, then reconfirms source run/PR identity before mutation; legacy acceptance still requires the artifact's runId to match the source run. |
no finding |
| Deployment proof and preview-link publication could be relaxed accidentally | preserves | deployment validation remains validateDeploymentResult, exact on PR/head/repo/source run/attempt and published proof; adversarial deployment mismatch still rejected. |
no finding |
WHY 1: older open PRs emitted pr-meta.json before the exact identity/proof fields existed.
WHY 2: default-branch reconciliation could download the exact trusted CI run for one of those PRs and still reject its analysis before rendering the current report.
WHY 3: maintainers then cannot use the fresh, trusted analysis/preview report to review a live PR recovery even though the artifact came from the right run.
USER-FACING PROBLEM: a maintainer reviewing #5671 could trigger a fresh trusted CI run and still lose the PR analysis report because the default-branch workflow rejected the old metadata shape before rendering.
PROBLEM SEVERITY: broken task — #5671's live artifact from run 33293627054 reproduced the pre-proof metadata shape that the old inline check could not accept.
NEW FEATURE CASE: not a new feature.
EARLY STOP: clear — maintainer-owned CI infrastructure fixing a concrete report-publication failure.
VERDICT: clear
The workflow keeps resolving and reconfirming the trusted source run through the GitHub API, then delegates pr-meta.json validation to the shared metadata validator. That validator accepts old artifacts only when the PR number, source run, and head prefix match the trusted identity, and it continues to enforce exact identity fields whenever older artifacts include them. Deployment proof validation stays on the stricter deployment-result schema.
SOLUTION (1 decision · 12 workflow lines changed, 66 test lines added)
- Replace the duplicated exact-only inline metadata check with the shared compatibility validator already used by report reconciliation.
BURDEN: low — no new state, listener, dependency, permission, public package API, or component behavior; one default-branch workflow step now imports an existing helper and two focused test files cover the compatibility/wiring. BURDEN MATCH: proportionate — one legacy artifact shape needs compatibility at the existing trusted validation seam, not a second metadata model.
VERDICT: clear
OWNER: the default-branch PR comment workflow owns privileged report mutation; the shared preview/reconciliation helper owns PR analysis metadata validation.
TIER 1: GitHub Actions workflow-run identity, pull-request API identity, actions/download-artifact scoped to the exact workflow run, shared metadata validator, and strict deployment-result validator.
TIER 2: none.
SEAMS: workflow-run event, PR identity resolution, current-head reconfirmation, pr-analysis artifact, legacy metadata shape, exact metadata shape, deployment proof, stale-report fallback, and preview-link publication.
BEHAVIOR UNIT: pure utility plus workflow step — validateAnalysisMetadata() is exercised directly and through workflow text wiring.
COMPLEXITY BUDGET: 1 runtime decision, 1 validation owner, 0 new durable schemas, 0 new workflows, 0 new permissions.
ACTUAL BURDEN: 3 changed files; 12 workflow lines changed; 66 test lines added; no owners/modules/schemas/workflows/permissions added.
BURDEN TREND: first reviewed head → current head: flat; one head reviewed.
RESET TRIGGER: not triggered — the change removes duplicated validation logic from the workflow and reuses the existing single helper.
| domain fact | one authoritative writable source | generated / immutable projections | other writable copies |
|---|---|---|---|
| trusted PR/source-run identity | GitHub API result reconfirmed by default-branch code | workflow outputs passed to validation and report rendering | none |
| PR analysis metadata compatibility | validateAnalysisMetadata() |
direct unit tests and workflow wiring test | no inline duplicate remains |
| preview deployment proof | validateDeploymentResult() |
preview links and published deployment manifest | none |
| seam | driven result |
|---|---|
| #5671 legacy artifact | accepted only for PR 5671, head prefix 70b2d6a, and run 33293627054
|
| wrong legacy PR | rejected |
| wrong legacy source run | rejected |
| wrong legacy head prefix | rejected |
| missing legacy head prefix | rejected |
exact headSha mismatch when present |
rejected |
exact headRepository mismatch when present |
rejected |
exact baseRepository mismatch when present |
rejected |
exact runAttempt mismatch when present |
rejected |
| deployment proof path | unchanged strict proof accepted for the right identity and rejected for wrong head |
VERDICT: clear
Maintainers can recover and read PR analysis for older open PRs whose CI artifacts predate the full proof fields. The trusted workflow still refuses stale or swapped artifacts before mutating a PR comment or exposing preview links. NEW FEATURE IMPACT: not a new feature.
VERDICT: clear
No public package API, component prop, CLI API, or consumer surface changed. The internal CI metadata contract becomes backward-compatible for pr-analysis/pr-meta.json only at the trusted validation boundary.
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
~ |
pr-meta.json validation accepts legacy shape when exact trusted PR/run/head-prefix identity matches |
internal CI artifact contract | PR analysis report reconciliation | focused tests | ok |
OSSIFICATION: internal workflow compatibility for already-produced artifacts. The cost of being wrong is either blocking legitimate old reports or accepting stale/spoofed reports; both sides are covered by exact-match and adversarial checks.
VERDICT: clear
No styles, theme targets, tokens, variables, or rendered component contracts changed.
VERDICT: clear
BEHAVIOR: intentionally less restrictive for legacy PR analysis metadata from the exact trusted source run; unchanged rejection for mismatched PR/run/head prefix and exact fields when present. API: no public API or consumer call-site change. VISUAL: no component/story pixels changed; visual jobs skipped because there is no stable visual scope. THEME: no theme surface change.
VERDICT: clear
EFFECTS: zero.
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
| none | n/a | n/a | no render path | no listeners/resources | metadata validator suite and workflow wiring test |
RENDER: no React/render path. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or package bundle change; default-branch workflow imports an existing local helper.
VERDICT: clear
VISUAL CHECK: not applicable
WHY: all changed files are CI workflow/test files; no component output, DOM, CSS, Storybook story, or visual asset changes. Exact-head CI marked Stable visual regression, pr-a11y, and pr-rtl skipped; visual-acceptance passed with “No stable visual scope.”
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
No rendered elements, roles, focus behavior, accessible strings, locale formatting, or directional layout changed.
VERDICT: clear
| slot | verdict |
|---|---|
| CONTEXT & ROUTING | clear |
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | clear |
| IMPACT | clear |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear |
| VISUAL | clear — not applicable |
| A11Y & I18N | clear |
GOAL: met — legacy #5671 analysis metadata is accepted only at the already-trusted run/PR/head boundary, exact identity fields remain enforced when present, fallback attacks reject, and deployment proof behavior is unchanged. DISPOSITION: approve once undrafted. No blockers or non-blocking asks found. ADVICE: none. AUTHOR CAN PROCEED: yes. WORST OUTCOME: a stale or attacker-controlled artifact becomes trusted report content; mitigated by exact workflow-run download, API-resolved identity reconfirmation, required PR/run/head-prefix matches for legacy metadata, exact-field enforcement when present, and separate strict deployment proof validation.
JUDGEMENT NEEDED: none.
approve once undrafted
Semantic verdict: approve once undrafted.
🟢 No review blockers found — this preserves the default-branch trust boundary while letting old, already-trusted pr-meta.json artifacts render again. Triage: CI bug fix · non-breaking · trust-sensitive workflow path → full path · checks: exact workflow-run identity, legacy metadata compatibility, exact-field enforcement, mismatch rejection, deployment-proof preservation, actionlint, focused tests, and CI.
I checked exact head bd6aced7fa963d67957c16417c105c4dfaf6dbf3: the #5671 artifact has the old shape (prNumber, shortHash, storybookUrl, sandboxUrl, runId, runUrl) and is accepted only for the exact trusted PR/run/head-prefix identity. Wrong PR, wrong run, wrong head prefix, missing short hash, and mismatched exact identity fields all still reject; deployment proof validation is unchanged. CI is green. Full evidence: https://github.com/cixzhang/astryx/wiki/Review-5740
Because this PR is still draft and self-authored, GitHub may not accept a formal approval event; this is the semantic approval for the reviewed head.
[Reviewed by Agentcloud]
None.
- Reproduced #5671 old metadata shape by downloading
pr-analysisfrom CI run33293627054:pr-meta.jsoncontainsprNumber,shortHash,storybookUrl,sandboxUrl,runId, andrunUrl, with no exactheadSha, repository, or run-attempt fields. - Exact-head adversarial validator script: actual #5671 legacy metadata accepted; wrong PR, wrong source run, wrong head prefix, missing short hash, mismatched exact head, head repository, base repository, and run attempt rejected; current deployment proof accepted; wrong deployment head rejected.
- Focused local tests:
npx --yes pnpm@11.10.0 exec vitest run .github/scripts/lib/pr-preview.test.mjs .github/scripts/visual-gate/workflow-concurrency.test.mjs→ 2 files, 49 tests passed. - Workflow syntax/local checks:
npx --yes pnpm@11.10.0 exec actionlint -ignore 'label "2-core-ubuntu-arm" is unknown' .github/workflows/pr-comment.yml,npx --yes pnpm@11.10.0 check:repo, andgit diff --checkpassed. - Exact-head CI green: CI run
33354414645, Lint run33354414664, CLI Smoke Test run33354414652, Internal Registry run33354414612, Review signal run33354414406, and Spec owner gate run33354414329all passed for this SHA; stable visual/a11y/RTL jobs skipped as expected for the CI-only no-stable-visual-scope diff. - Code references: shared analysis metadata validator is in
pr-preview.mjslines 369–407; workflow import/call is inpr-comment.ymllines 418–440; legacy compatibility tests are inpr-preview.test.mjslines 276–327.
TIME total 84m
- setup/rules: 8m — claim, exact-head resolution, required review kit, ENGOWNER bucket, prior reviews/comments, and untrusted-diff gate
- reading: 12m — workflow, shared validator, reconciliation/deployment paths, test coverage, #5671 context
- measuring: 24m — exact-head checkout/install, focused tests, actionlint, check:repo, diff check, #5671 artifact download, adversarial validator/proof script
- CI wait: 34m — waited for exact-head GitHub CI, lint, smoke, visual-acceptance, and status contexts to settle green
- writing: 6m — public review, critic pass, wiki record, GitHub delivery, cleanup
- waste: 1m — fixed a syntax typo in the first adversarial script draft
Nothing material. I did not mutate or rerun the production PR comment workflow manually; the trust-boundary behavior was verified through exact-head local validator/adversarial checks and exact-head CI.
The critic pass shortened the public review to the user-impact trust-boundary result, made the draft/self-authored semantic-verdict limitation explicit, and kept local paths/private routing out of public text.