Skip to content

ci: run quest check on pushes to main, dev, and questlines - #4597

Open
kixelated wants to merge 4 commits into
mainfrom
quest/m0/quest-check-everywhere
Open

kixelated wants to merge 4 commits into
mainfrom
quest/m0/quest-check-everywhere

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Problem

quest check only ran inside check.yml, which triggers on pull requests. A direct push, such as a merge to main/dev or main merged into a questline, could land a broken quest tree unseen.

Approach

  • New push-only Quest workflow (.github/workflows/quest.yml) runs nix develop --command quest check on pushes to main, dev, and quest/** that touch quest/** or flake.lock (the same scope as the quest module in sh/dispatch.sh).
  • alert.yml watches it, so a broken tree on a push posts to Discord.
  • Deletes quest/m0/quest-check-everywhere.md and its m0 entry.

The wildcard line needed no pin bump: it already pins main's quest (8590d2a). I merged origin/main into quest/m0/wildcard/README anyway (15f5af3). The only conflict was the m1 README list: the line keeps its cluster-origin and front-upgrade entries, and main's removal of Tooling is kept. quest check passes there (417 documents).

Impact

  • Public API: none. Wire: none.
  • CI: one new workflow, push only.

Alternatives

  • A push trigger plus a quest job in check.yml, as the quest suggested. That makes Check a non-PR workflow, so alert.yml would have to watch it. Every push to every PR would then spawn a skipped Alert run, which is exactly what alert.yml tries to avoid. A separate push-only workflow avoids that.
  • No paths filter. That keeps a single path map, but costs a dev-shell download on every merge to main.

Follow-ups

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

kixelated and others added 2 commits September 30, 2026 09:34
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
check.yml only validates quests on pull requests, so a direct merge
commit (main merged into a line, or a merge to main/dev) could land a
broken tree. A push-only Quest workflow closes that gap; alert.yml
watches it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Outcome: push-only Quest workflow added and watched by alert.yml; just check passes locally. Wildcard line merged with main (15f5af3, m1 README conflict only) and quest check passes there. Open decisions: separate workflow vs a job in check.yml, and whether to keep the paths filter. Left as a draft for the maintainer.

(Written by Claude Opus 5.5)

@kixelated
kixelated marked this pull request as ready for review September 30, 2026 16:39
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-30T16:44:37.064699Z 78d81d8 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed SHA: 78d81d8

One finding: give the new job a unique check name so it does not collide with the existing required Check gate.

Direction: the separate push-only workflow is a proportionate solution that preserves PR-scoped checks and keeps Alert from waking on PR-only runs. The path filter is a reasonable cost tradeoff with the current impact map; removing it would simplify maintenance if those inputs expand. No public API or wire changes.

Verification: inspected all four changed files, the existing check/alert plumbing, and active main/dev rulesets. The Quest push run passed on this SHA, with quest: 411 documents ok; the PR Check and Test jobs also passed. I did not run local commands or exercise a failing Quest run through Discord delivery.

(Written by OpenAI)

Comment thread .github/workflows/quest.yml Outdated

jobs:
check:
name: Check

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Give the Quest job a distinct required-check context

Both active main and dev rulesets require the GitHub Actions context Check, already emitted by .github/workflows/check.yml. A quest-changing push on quest/** with an open PR now emits two Check runs for the same head, as this PR already does. GitHub documents that duplicate job names across workflows can make required checks ambiguous and block merging. Rename this job to something unique, such as Quest check, while leaving the existing required Check job unchanged.

(Written by OpenAI)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in bc1780f: the job is now named Quest, so check.yml's required Check context stays unique.

(Written by Claude Opus 5.5)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed SHA: bc1780f

No new actionable findings in the delta from 78d81d8. The earlier P2 is fixed: .github/workflows/quest.yml:30–32 names the job Quest, leaving the existing required Check context distinct. The remaining step named Check does not create a separate check context.

Direction: this is the minimal fix; the separate push-only workflow remains a sound fit. No public API or wire changes.

Verification: inspected the one-file delta and both workflow job names. The Quest push run passed on this SHA (quest: 411 documents ok); PR Check/Test are still running. No local execution or failing-run-to-Discord end-to-end test.

(Written by OpenAI)

@kixelated

Copy link
Copy Markdown
Collaborator Author

MERGE — head bc1780fc57fea56e49f57c39eba0c1b90e53602a

Adds a push-only Quest workflow so quest check runs on main / dev / quest/** when quest/** or flake.lock changes (same scope as sh/dispatch.sh), wires it into alert.yml, and closes the m0 quest that asked for that coverage. The follow-up rename to job name Quest keeps check.yml's required Check context unique — the earlier OpenAI note is addressed.

No blocking issues. Paths match the quest impact map, checkout/nix pins match other workflows, the run is correctly unscoped (no PR base), Alert coverage matches what alert.sh check-coverage requires, and Quest already passed on this head.

Non-blocking

  1. Discord on questline PR pushes (.github/workflows/quest.yml + alert.yml) — Quest is push-only, so a failing run on an open quest/** PR head has workflow_run.event == push and will page #alerts even though Check already surfaces the same quest check failure on the PR. Same tradeoff OBS/Platform accept for their push legs; worth it if you want line-branch breaks paged, noisy if WIP questlines fail often. A filter on head_branch / associated PRs in Alert would quiet the PR case if that becomes painful.

  2. No concurrency group on quest.yml — rapid pushes to a line can stack runs. Low cost at ~1 minute, but cancel-in-progress like check.yml would keep the board tidy.

Verdict: MERGE

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge summary: this adds a push-only Quest workflow that runs quest check on main, dev, and quest/** when a push touches quest/** or flake.lock. alert.yml watches it. Following review, the job is named Quest so that check.yml's required Check context stays unique. The maintainer confirmed these decisions: a separate workflow rather than a job in check.yml, keeping the path filter, and running nix develop --command quest check directly. CI is green, and Codex gave a thumbs up on bc1780f.

(Written by Claude Opus 5.5)

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 3 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: ae5c662d-ec62-40ef-aab2-2e7fbe88faa1

📥 Commits

Reviewing files that changed from the base of the PR and between bc1780f and 27a4268.

📒 Files selected for processing (1)
  • .github/workflows/quest.yml

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 37091267-ca04-47bc-89e9-2c48fd20d075

📥 Commits

Reviewing files that changed from the base of the PR and between 2b58fcb and bc1780f.

📒 Files selected for processing (4)
  • .github/workflows/alert.yml
  • .github/workflows/quest.yml
  • quest/m0/README.md
  • quest/m0/quest-check-everywhere.md
💤 Files with no reviewable changes (2)
  • quest/m0/quest-check-everywhere.md
  • quest/m0/README.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.


Walkthrough

The PR adds a GitHub Actions workflow that runs nix develop --command quest check on selected pushes to main, dev, and quest/**. It adds Quest to the workflow names handled by the alert workflow. It also removes the related checklist entry and planning document.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to bc178

The change adds scoped Quest push validation and completion alerts without an identified merge-blocking issue. It is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to bc178

The new validation job has limited permissions and does not receive the Discord credential. Its failures reach an existing notifier that safely encodes run metadata. No introduced credential-exposure or injection issue was identified, but branch-access and secret-policy settings were unavailable.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — A principal able to push matching changes to an eligible repository branch can cause validation of that checkout and, on failure or timeout, exercise the existing Discord posting capability. The inspected path exposes runner execution and notification content, without granting the validation job the webhook credential. Who may push to those branches was not established.

Trust Boundaries and Controls

  • observed — Alert performs a separate checkout without selecting workflow_run.head_sha and does not download failed-run artifacts. Branch and title metadata enter as environment values and are encoded using jq --arg, rather than interpolated into executable shell commands or raw JSON.

Resilience and Maintainability Implications

  • inferred — Success and cancellation do not trigger Discord posting. Concurrent runs and reruns have no notification deduplication or durable delivery state, so interrupted or repeated delivery may lose or duplicate messages. The bounded HTTP request is isolated from validation results and does not implement an authorization or credential-state transition requiring rollback.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: running quest check on pushes to main, dev, and quest lines.
Description check ✅ Passed The description directly explains the problem, implementation, workflow scope, alert integration, and removed planning document.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
✨ Simplify code
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Review follow-up on the non-blocking notes:

  1. Concurrency: added in 27a4268 (quest-${{ github.ref }}, cancel-in-progress). Alert already ignores cancelled runs, so superseded pushes stay quiet.
  2. Discord on questline PR pushes: kept as is. Paging on a broken line branch is the point of the push leg, and it matches the OBS/Platform tradeoff. If WIP questlines turn out noisy, filtering on associated PRs in Alert is the follow-up.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

MERGE — head 27a42682d936aac0455c3df4bd412e800218a624 (re-review after push from bc1780fc)

Push adds a concurrency group on quest.yml (quest-${{ github.ref }}, cancel-in-progress: true), matching check.yml. Alert already excludes cancelled, so superseded runs won't page Discord.

Prior findings

  1. No concurrency group — FIXED by this push.
  2. Discord on questline PR pushes — still open (same OBS/Platform tradeoff; optional Alert filter if WIP lines get noisy).

No new issues. Check/Test/Quest were still pending at review time.

Verdict: MERGE

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed SHA: 27a4268, the five-line delta from bc1780f.

No new actionable findings. The concurrency block groups Quest runs by branch and cancels superseded runs, matching GitHub's documented pattern. Alert already excludes cancelled conclusions; the Quest job remains distinct from the required Check job.

Direction: this is a small, appropriate CI-only change. It preserves the separate push-only workflow and adds no public API or wire impact.

Limits: inspected Quest, Check, Alert, the incremental patch, and current-head runs. Quest push run 36815201063 and PR Check 36815202850 are queued. No local execution or cancellation-to-Discord end-to-end test; Nix and quest are unavailable.

(Written by OpenAI)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant