Review 5794

Review 5794 — fix(ci): reconcile fork review events safely

PR

#5794 fix(ci): reconcile fork review events safely by cixzhang (bucket: the maintainer)

HEAD REVIEWED

9a1d7cb11b4491c6815d38c10ac0414a03b4311e

VERSIONS

LOOP VERSION: 2.0.0 AUDIT RUBRIC: 1.15

LANE

LANE: full WHY: This changes a privileged workflow boundary, adds a workflow owner, and has 74 added executable configuration/runtime lines; fast-lane trust and architecture conditions do not hold.

CONTEXT & ROUTING

CHANGED CODE: Review events now complete through a permissionless anchor and a write-capable workflow_run companion; the existing reconciler accepts the companion’s explicitly resolved PR number. NEAREST CURRENT COMPONENT CONTRACT: .github/workflows/spec-owner-gate.yml:3-9,35,57-76 and .github/scripts/spec-owner-reconcile.cjs:48-157 — trusted default-branch code owns exact-head status reconciliation and stale-run fencing. CURRENT FAMILY: .github/workflows/review-clear.yml:3-20,24-39 already uses a write-capable workflow_run companion for fork review events; .github/workflows/pr-comment.yml:5-15 explicitly keeps one workflow-run path for fork and same-repo PRs to avoid drift. CURRENT API/THEMING/SYSTEM RECORDS: none; this is internal CI infrastructure with no Astryx package API, theme, or component contract. DRAFT CONTEXT: none — context/evidence only; never policy or precedent. STOPPED AT: family — landed review workflows establish the trusted-companion pattern and one-path preference. REGRESSION EVIDENCE: Before, fork review run 33415373897 received read-only statuses permission and its status POST failed 403. At this head, the same run’s immutable SHA/ref/base identity resolves exactly one open PR (#5791), the privileged workflow checks out only the default branch, actionlint passes, and all 35 focused decision/reconcile/workflow tests pass; the landed Review clear sibling proves this workflow_run boundary receives a write-capable token on fork reviews. OWNER QUESTION (PRIVATE): none.

candidate issue	classification	evidence	disposition
Fork review events cannot write the owner-gate status	preserves	failed run 33415373897; permissionless anchor plus landed write-capable companion pattern; 35 focused tests	no finding
A stale or ambiguous run could mutate the wrong PR	preserves	actual fork run SHA/ref/base selects only #5791; zero or multiple matches throw at .github/workflows/spec-owner-review-reconcile.yml:54-63	no finding
Privileged code could come from the PR	preserves	both workflows check out github.event.repository.default_branch; only API-returned metadata crosses the boundary	no finding
One companion path adds a lightweight anchor run for same-repo reviews	settled	landed pr-comment.yml:5-10 documents one path for fork and same-repo PRs to prevent drift; 37 review events ran in the prior 24 hours	apply decision

PROBLEM

WHY 1: Fork pull_request_review runs receive a read-only token, so owner review events fail when the reconciler writes the exact-head status. WHY 2: The owner’s review does not update the gate, leaving maintainers to use a separate command or retry path. WHY 3: A contributor’s current-record PR can remain blocked even after the responsible owner reviewed it. USER-FACING PROBLEM: A maintainer reviewing a fork-authored current record cannot reliably clear its merge gate through the normal GitHub review flow. PROBLEM SEVERITY: harmful friction — the merge can be recovered through another command, but the standard owner-review path fails. NEW FEATURE CASE: not a new feature; this restores the existing owner-review contract for fork PRs. EARLY STOP: clear — the failed run and existing contract establish the need.

VERDICT: clear

SOLUTION

A review event first records a permissionless success, then GitHub starts a trusted workflow from the default branch. That workflow finds exactly one open PR whose immutable head identity matches the completed run and asks the existing reconciler to evaluate current owner state. No PR-controlled code runs with write permissions, and stale or ambiguous identities stop before mutation.

SOLUTION (1 decision · 74 executable configuration/runtime lines of 123 added lines)

1. Route every review event through one trusted companion that resolves an exact PR identity before reusing the existing reconciler — restores the failed fork-review path.

BURDEN: medium — one new privileged workflow and one explicit PR-number seam; no new durable state, dependency, parallel approval model, or duplicated decision engine. BURDEN MATCH: proportionate — GitHub’s fork token boundary requires a second workflow, while the decision logic remains in the existing reconciler.

VERDICT: clear

ARCHITECTURE

OWNER: the Spec owner gate workflow owns triggering; reconcileSpecOwnerGate remains the sole approval and mutation owner. TIER 1: GitHub Actions trust boundary; reuses the existing exact-head decision resolver and run-order fencing. TIER 2: none. SEAMS: same-repository and fork heads; submitted, commented, and dismissed reviews; stale heads; duplicate branch names; zero or multiple matches. BEHAVIOR UNIT: inline — the companion’s identity filter is a small fail-closed adapter; all owner decisions and mutations remain in the tested reconciler. COMPLEXITY BUDGET: 1 runtime decision, 1 approval owner, 1 trusted companion workflow, 0 new durable maps/schemas. ACTUAL BURDEN: 5 changed files; 74 executable configuration/runtime lines; 1 new workflow; 0 durable representations; 35 focused tests plus actionlint. BURDEN TREND: first reviewed head 9a1d7cb → current head 9a1d7cb: flat; one reviewed head. RESET TRIGGER: not triggered — no second owner, approval representation, or synchronization guard was added.

domain fact	one authoritative writable source	generated / immutable projections	other writable copies
target PR identity	GitHub’s open PR metadata	completed run SHA/ref plus base repository	none
current owner decision	PR reviews, commands, dismissal timeline, and exact-head ready status	reconciler snapshot and final gate status	none

seam	driven result
fork review	historical run 33415373897’s SHA/ref/base resolves exactly #5791
stale or ambiguous identity	zero or multiple matches throws before reconcileSpecOwnerGate
trusted code provenance	both privileged checkouts pin the default branch
owner decision path	explicit pullNumber reaches the unchanged resolver; focused test ends pending for an unapproved contributor PR

The implementation keeps lifecycle ownership in the existing reconciler and adds only the permission bridge GitHub requires. The new adapter derives identity from immutable run and PR metadata rather than creating another approval source.

VERDICT: clear

IMPACT

Maintainers reviewing fork-authored current records can use the normal review flow without the gate’s status write failing. Same-repository reviews keep the same decision semantics; all review events now pay one permissionless anchor job so fork and same-repository paths do not drift. NEW FEATURE IMPACT: not a new feature; this repairs the existing review-to-gate path.

VERDICT: clear

API

No public Astryx API change. Internally, reconcileSpecOwnerGate gains optional pullNumber, used only by the trusted companion.

	change	public?	class	doc'd?	verdict
+	reconcileSpecOwnerGate({ pullNumber?: number })	no — CI helper only	existing event-context PR resolution	not required	ok

PUBLIC API GATE: not applicable API ROW: not applicable — no added or changed public signature NON-DERIVABLE NEED: not applicable MEANING: not applicable PREDICTABILITY: not applicable CAPABILITY: not applicable DOCS OBLIGATION: not required — internal workflow plumbing only.

OSSIFICATION: none; no package export, prop, accepted value, default, or theme target changes.

VERDICT: clear

THEMING

No theming change: no component, token, variable, target, style, or rendered element changed.

VERDICT: clear

BREAKING

BEHAVIOR: intended CI behavior change only — fork review events now reconcile through the companion; same-repository reviews retain the existing decision result. API: no — no released call site changes. VISUAL: no — only .github/scripts and .github/workflows changed. THEME: no — no targets, tokens, or overrides changed.

VERDICT: clear

PERFORMANCE & RESOURCES

EFFECTS: zero; this is non-React CI code.

Effect + deps	external system	why render/handler cannot do it	measured render cost	lifetime + cleanup	focused test
none	not applicable	not applicable	0 React renders	no browser listeners or observers	not applicable

RENDER: no reachable product-render change. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or shipped bundle change. RESOURCE: one source workflow plus one companion replaces the failing single fork workflow; same-repository reviews add one no-op anchor job. The repository saw 37 review events in the preceding 24 hours. The landed PR Comment contract explicitly accepts this one-path cost to prevent fork/same-repository drift.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: all five changed paths are CI workflows, CI scripts, or their tests; no product DOM, CSS, story, component, or documentation rendering changes. Storybook port 6537 was reserved but no server was started.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

No rendered control, role, ARIA state, user-facing string, locale formatting, or direction-sensitive behavior changed. Exact-head pr-a11y, visual, and RTL jobs were skipped by scope as expected.

VERDICT: clear

JUDGEMENT

slot	verdict
PROBLEM	clear
SOLUTION	clear
ARCHITECTURE	clear
IMPACT	clear
API	clear
THEMING	clear
BREAKING	clear
PERFORMANCE	clear
VISUAL	clear
A11Y & I18N	clear

GOAL: met — the historical fork event’s read-only 403 is replaced by a permissionless source run plus a trusted default-branch companion; exact live identity matched #5791, actionlint passed, and 35 focused tests passed. DISPOSITION: no negative findings. ADVICE: omitted — no defect found. AUTHOR CAN PROCEED: yes — undraft when ready; no code change requested. WORST OUTCOME: none found → comment because the PR is draft.

JUDGEMENT NEEDED: none — internal CI repair against the existing contract.

comment — semantic verdict: approve once undrafted.

No findings.

REVIEW

Thanks, this looks good. This is an approve once you undraft.

INLINE (0–3, only when genuinely line-anchored)

None.

EVIDENCE I DID NOT SPEND

• The failing fork run exposed Statuses: read and failed its first status POST with HTTP 403.
• The historical fork run’s SHA/ref/base tuple matched exactly one current open PR despite pull_requests: [].
• The source head remained 9a1d7cb11b4491c6815d38c10ac0414a03b4311e through the evidence pass.

TIME

TIME total 12m setup 3m dedicated worktree + cloned dependencies; warm main reused: yes reading 6m full loop rules, diff, changed files, current family, history measuring 2m 35 focused tests, actionlint, failed-run log, live fork identity, 0 re-measures writing 1m presentation + critic gate waste 1m first worktree checkout used the wrong multi-ref FETCH_HEAD and was corrected before review

WHAT I COULD NOT VERIFY

• The new companion cannot run end-to-end until its workflow file exists on the default branch; the landed Review clear companion verifies the same fork/write-token boundary.

What changed before posting

Added the semantic-verdict line and required Robohands attribution; the substantive review was posted as drafted.

Posted review

https://github.com/facebook/astryx/pull/5794#pullrequestreview-5070306843