valiant-newt-664

[briansrls]

Opened from session-dashboard for session valiant-newt-664.

[briansrls]

Review: APPROVE — clean CI optimization.

What it does

Three coordinated changes to .github/workflows/ci.yml:

1. Adds ready_for_review to PR trigger types — without this, promoting a draft → ready doesn't kick CI; you had to push a new commit. With it, the transition itself triggers CI.
2. Concurrency group with cancel-in-progress — new pushes to the same PR cancel the in-flight run. Standard best practice; saves CI minutes on rapid-iteration branches.
3. Draft-skip on each job — if: github.event.pull_request.draft != true on fmt, ci, v3, self_host_ratchet. Drafts don't burn CI; ready-for-review or new pushes trigger it.

Why the details are right

• The concurrency fallback is correct: github.head_ref || github.run_id. On PR events, head_ref is the branch and same-PR pushes group together → new push cancels old. On pushes to main, head_ref is empty, so the fallback run_id makes each run a unique group — nothing on main gets cancelled. The inline comment spells this out.
• The != true guard is correct: on push events, github.event.pull_request.draft is undefined (no PR context). undefined != true evaluates to true, so main pushes still run. Only draft PRs skip — ready PRs and main pushes run normally.
• Inline comments explain non-obvious mechanics where they appear, which is right for YAML gotchas that aren't self-evident.

Impact

• Drafts won't block the CI queue.
• Rapid-iteration branches won't stack queued runs.
• Main pushes are unaffected.
• Required status checks on the main branch continue to require these jobs — when a PR moves draft → ready, the checks fire via ready_for_review before merge.

Minor observations (non-asks)

• The if: github.event.pull_request.draft != true line is repeated 4× across jobs. Could be DRY'd later via a gating job or reusable workflow, but the current form is immediately readable — the repetition is honest about which jobs honor the gate.
• Concurrency cancellation means "cancelled" shows on superseded commits. Usually fine for PR review since reviewers look at the latest SHA, but if someone ever needs a specific-commit CI result, they'd need to re-run. Acceptable trade-off for the CI-minutes saving.

No changes requested

Ready to merge. Also directly addresses the CI thrashing #620 inherited — the regen-drift breakage would've been caught earlier and wouldn't have re-run on every relay.

🤖 Director review

[briansrls]

Review metadata

• Provider / model: claude / claude-opus-4-7
• Commit: 6d772f65 · Trigger: schedule
• Thinking: 20s wall

---

APPROVE — CI-only change. Three targeted workflow tweaks:

• ready_for_review added to PR event types so promoting a draft triggers CI (without this, the default synchronize only fires on new pushes).
• concurrency group keyed on head_ref || run_id correctly cancels in-flight PR runs on new pushes while giving each main push a unique group (empty head_ref falls through to the unique run_id), so main pushes are never cancelled.
• if: github.event.pull_request.draft != true on each job correctly skips drafts while still running on push-to-main (where pull_request is undefined, so draft is undefined, and undefined != true is true).

No substrate, modeling, or Rust code touched — the context docs (INVARIANTS.md, MODELING.md, CODING.md, TESTING.md) don't apply to this diff. Comments in the YAML explain the two non-obvious bits (draft check on push events, and the run_id fallback), which is the right level of explanation for a CI config. Nothing to flag.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: 6d772f65 · Trigger: schedule
• Thinking: 68s wall

---

Findings

• .github/workflows/ci.yml:21 — github.head_ref is only the branch name, not a PR-unique identifier, so two fork PRs opened from identically named branches (for example patch-1) will land in the same concurrency group and cancel each other’s CI. This is outside the compiler modeling rubric, but it is a real workflow correctness issue; keying by github.event.pull_request.number or github.event.pull_request.head.label would avoid cross-PR collisions.

Verdict: APPROVE_WITH_COMMENTS. Nothing in this diff touches compiler substrate/modeling/test structure in a way that violates the pinned docs; the only concern is the concurrency key potentially canceling unrelated fork PR runs.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: b6ee83f6 · Trigger: schedule
• Thinking: 203s wall

---

Non-blocking — Strengths

• .github/workflows/ci.yml PR-number-scoped concurrency uses the canonical pull request identity instead of the lossy head_ref proxy, which is the right single-authority shape for cancelling stale runs.
• .github/workflows/ci.yml Adding ready_for_review preserves review-time CI when draft jobs are skipped, so promoting a draft does not require a no-op push to regain coverage.

✅ This looks clean; the change stays in GitHub Actions control-plane wiring and I do not see a thesis or invariant concern on the added lines.