Repository navigation
feat(check): guard that a quest's PR leaves its quest accurate - #169
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
quest check --base <ref> [--branch <name>] flags a quest branch whose diff from the merge base neither deletes nor edits its quest. The guide states the rule once, and the merge skill runs the guard before enabling auto-merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Outcome: the quest is complete. (Written by Claude Opus 5.5) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (2)
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e77cb34de
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with `<ref>`. When the branch is `quest/<path>`, naming `quest/<path>.md` or an | ||
| epic's `quest/<path>/README.md` at the merge base, the diff must delete or edit |
There was a problem hiding this comment.
This introduces quest/<path> as the branch for quest/<path>/README.md, but the shared guide still defines every branch as the document path without .md, making that epic's branch quest/<path>/README. An agent following this new section can therefore open an epic PR on a branch that other agents will not search when checking the quest's canonical branch, allowing a duplicate claim; either retain the /README suffix here and in questAt, or change the branch convention consistently across the guide and claiming workflow.
AGENTS.md reference: assets/AGENTS.md:L122-L123
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf27b05 by changing the guide rather than the guard. The guard already matches the existing convention: kix.quest starts an epic on its directory (cloudflare/src/jobs/routes.ts strips /README.md), and associatedQuest and the board use the same mapping. assets/AGENTS.md now says an epic's branch is its directory, with quest/m1/foo/README.md → quest/m1/foo as the example.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
Update: the maintainer chose the other way in 1cafbdc. The guide keeps its original wording, so an epic's branch is quest/<path>/README, and questAt now looks up only <branch>.md. This retains the /README suffix as you first suggested. The directory form collides with child branches in git's ref namespace. The README-named-epic fix from cce32bd is no longer needed and is removed.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 9e77cb34de
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| and re-run it with `--since` until it returns activity. | ||
|
|
||
| Merge the PR *only* after all outstanding decisions have been confirmed. | ||
| Before enabling auto-merge, fetch the PR's base branch and head, check out the head, and run `quest check --base <remote>/<base> --branch <head-branch>`. |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
When /quest-merge handles an external/fork PR from a trusted checkout, this step checks out the contributor-controlled head and then invokes quest. In this repository, AGENTS.md expands that command to bun run --silent quest, whose package.json script executes src/cli/bin.ts from the checked-out head. A PR can replace either entrypoint and execute code in the maintainer process before merge, with access to the local account and any forge credentials used by this workflow. Invoke an absolute, pinned Quest binary built from the trusted base, passing the PR worktree through --root, or run the head without credentials in a sandbox.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf27b05. The merge skill now runs quest --root <head-checkout> check --base … with a quest the PR cannot modify, such as an installed release or a build from the base, never one built from the head. Running other head code, like CI fixes on a fork PR, is outside this PR's scope.
(Written by Claude Opus 5.5)
# Conflicts: # quest/m0/landed-quests.md
…ranches Address Codex review on #169: the merge skill runs a quest the PR cannot modify, pointed at the head with --root, and the guide states that an epic's branch is its directory, matching the guard and kix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf27b053db
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const path of [`${branch}.md`, `${branch}/README.md`]) { | ||
| if (git(root, ["cat-file", "-e", `${commit}:./${path}`]).status === 0) return path; |
There was a problem hiding this comment.
When a valid tree contains an epic such as quest/m0/epic/README/README.md, its branch is quest/m0/epic/README, but the first candidate here is the parent epic's quest/m0/epic/README.md, which normally also exists. Consequently, editing or deleting the nested epic still produces a finding, while an incidental edit to the parent can incorrectly satisfy the guard. Skip the .md candidate when the branch basename is README, or otherwise resolve candidates by their canonical branch names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cce32bd. A branch ending in /README now resolves only to <branch>/README.md, since README.md belongs to its parent epic, whose branch is the directory. The regression test is "an epic named README is checked against its own README, not its parent's".
(Written by Claude Opus 5.5)
A branch ending in /README names a nested epic called README; README.md is its parent's epic, whose branch is the directory. Address Codex review on #169. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keep the deletion of quest/m0/landed-quests.md over #164's edit, and keep both README sections. With quest rm on main, the merge skill's fix-up deletes a finished quest with quest rm. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3ec5b228e
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| A quest's branch is its path without `.md`, and an epic's is its directory. | ||
| For example, `quest/m1/foo/bar.md` is branch `quest/m1/foo/bar`, and `quest/m1/foo/README.md` is `quest/m1/foo`. |
There was a problem hiding this comment.
When moving an epic with an active canonical claim such as quest/m1/foo, moveQuest still passes its quest/m1/foo/README.md through questBranch(), which removes only .md and therefore searches for quest/m1/foo/README. Because claims() never sees the actual branch or PR, quest mv quest/m1/foo ... can proceed despite its claimed-work guard; update the shared branch derivation to follow this new epic convention.
AGENTS.md reference: assets/AGENTS.md:L122-L123
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. This exposes a split in the codebase, so it is held for a maintainer decision and not fixed in this round. The CLI's questBranch (from #164) names an epic quest/m1/foo/README. kix.quest's start job, associatedQuest, and this guard name it quest/m1/foo. The directory form also collides with git's ref namespace: refs/heads/quest/m0 can't exist alongside any refs/heads/quest/m0/*, and origin has 36 of those today. The options and a recommendation are reported to the maintainer.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
Decided and fixed in 1cafbdc. An epic's branch is its README's path without .md, e.g. quest/m1/foo/README. That matches questBranch and the guide's original wording, and avoids the ref collision with child branches. The guard now resolves a branch only to <branch>.md, so quest mv and the guard agree. Moving kix.quest (start job, associatedQuest, board URLs) to /README is a follow-up.
(Written by Claude Opus 5.5)
An epic's branch is quest/<path>/README, matching the guide and the CLI's questBranch. The directory form collides with child branches in git's ref namespace. Revert the guide's directory wording and drop the README-named epic special case, which no longer applies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 1cafbdcd80
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| Before enabling auto-merge, fetch the PR's base branch and head, check out the head, and run `quest --root <head-checkout> check --base <remote>/<base> --branch <head-branch>`. | ||
| Run a `quest` the PR cannot modify, such as an installed release or a build from the base, not one from the head. |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
After this fix, a fork author can still add bunfig.toml with preload = ["./p.ts"]. If the merge agent follows line 43 from the checked-out head, both the installed Bun-script Quest and the Bun-compiled release load that cwd config before Quest starts, so p.ts runs with the maintainer’s environment/GitHub access. Line 44 protects the binary but not Bun’s runtime config. Fresh evidence after cf27b05: a local probe showed an absolute trusted script and a trusted bun --compile binary both executed the cwd preload; Bun documents preload as pre-entrypoint execution. Require a trusted cwd (using --root only as data) and disable local Bun config or sandbox the check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f685884. The merge skill now runs the trusted quest from a directory outside the head checkout, so --root is only data. It names the head's runtime config, such as bunfig.toml, as untrusted.
(Written by Claude Opus 5.5)
Resolve README and guide conflicts with #167, and run the merge guard from a directory outside the head checkout so the head's runtime config cannot run (Codex security review). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary: Since the first review:
Codex reviewed f685884 with no findings in either code or security review. The only change since then is a clean merge of main that touches none of this PR's files. CI is green on 194a153. Follow-ups: move kix.quest's start job, (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 194a153305
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Before enabling auto-merge, fetch the PR's base branch and head, check out the head, and run `quest --root <head-checkout> check --base <remote>/<base> --branch <head-branch>`. | ||
| Run a `quest` the PR cannot modify, such as an installed release or a build from the base, from a directory outside the head checkout: the head's files, including runtime config such as `bunfig.toml`, are untrusted. | ||
| A stale local base can predate the quest and skip the check. | ||
| On a finding, fix the branch: delete the finished quest with `quest rm`, or rewrite it to the remainder the PR body names. | ||
| Then push, re-check, and continue; ask the user only when what remains is unclear. |
There was a problem hiding this comment.
When the PR head is already checked out with an uncommitted edit or deletion of its quest, landed.ts compares the merge base against that working tree, so this check passes even though the fetched PR head does not contain the change. Because this sequence pushes only after a finding, it can then enable auto-merge without publishing the quest update; git checkout -h confirms that only a forced checkout throws away local modifications. Require a clean checkout or validate the committed fetched head before trusting the result.
AGENTS.md reference: assets/AGENTS.md:L129-L132
Useful? React with 👍 / 👎.
| const diff = git(root, ["diff", "--quiet", "--no-ext-diff", commit, "--", `./${path}`]); | ||
| if (diff.status === 1) return []; |
There was a problem hiding this comment.
On Unix repositories with core.fileMode enabled, merely toggling the executable bit on the quest makes git diff --quiet return 1, so this function reports success even though the quest content was neither deleted nor rewritten. A PR can therefore leave the stale quest intact while satisfying the new guard; compare the blob contents or otherwise reject metadata-only changes.
Useful? React with 👍 / 👎.
Completes
quest/m0/landed-quests.md(plan at d265755). In moq-dev/moq, merged quest-branch PRs left their quests behind: some finished the work without deleting the quest, and some partial drafts wrote the remainder only in the PR body. Those stale quests then ranked as ready work again.Changes
quest check --base <ref> [--branch <name>](src/cli/landed.ts). The head branch names the quest at<branch>.md, so an epic's branch isquest/<path>/README. When that quest exists at the merge base with<ref>, the working tree must delete or edit it. Otherwise it reports one finding that names the quest and the rule. Other branches, and branches that name no quest at the merge base, add nothing. A plainquest checkbehaves as before.assets/AGENTS.mdExecution now states the rule once. The PR that finishes a quest deletes it and every link to it, and a PR that leaves work rewrites the quest to what remains.assets/skills/merge.mdfetches the base and head, checks out the head, and runsquest --root <head-checkout> check --base <remote>/<base> --branch <head-branch>before enabling auto-merge. It uses aquestthe PR cannot modify, such as an installed release or a build from the base. On a finding it fixes the branch, deleting a finished quest withquest rm, then pushes, re-checks, and asks the user only when what remains is unclear.quest/m0/README.md.quest check --base origin/main --branch quest/m0/landed-questspasses on it.Decisions made (routine; recorded for review)
HEAD. This matches how plaincheckreads the working tree. It also works in CI, where the checkout is the PR's merge commit.HEADwith no--branchis an error. The check doesn't skip it silently, because a guard that quietly checks nothing looks the same as one that passed.--branchwithout--baseis a usage error (exit 2). An unknown base or a missing merge base exits 1.--branch <headRefName>applies to them as well. The merge skill can push its fix-up only when the PR allows maintainer edits. Otherwise it asks.quest/m0/README) has to edit its README, which matches the rule.quest rm. kixelated/quest#164 landed first, so this PR wiresquest rminto the merge skill. feat: move and delete quests with quest mv and quest rm #164's edit toquest/m0/landed-quests.mdwas resolved by keeping the deletion..md(quest/m1/foo/README), decided by the maintainer. This overrides the settled plan's line naming an epic's branchquest/<path>. It matches the guide's wording and the CLI'squestBranchfrom feat: move and delete quests with quest mv and quest rm #164. The directory form collides with child branches in git's ref namespace, sincerefs/heads/quest/m0can't coexist withrefs/heads/quest/m0/*. kix.quest still uses the directory form, which is left as a follow-up.Review
Codex findings addressed: the merge skill runs a trusted
questagainst the head with--root(security), and epic branch naming now follows decision 6.Validation
just check: passed.just test: passed. Vitest ran 274 tests, 12 of them intest/landed.test.ts. The Worker suite ran 265. The shell suites and the lifecycle checks on both builds passed./READMEbranch checked against its README, an epic's directory not counting as its branch, a non-quest branch, a quest that doesn't exist at the base,--branchoverriding the current branch, a base that moved on and edited the quest (still a finding), a detached HEAD, an unknown base, and--branchwithout--base.Suggested follow-ups
/READMEepic branch: the start job (cloudflare/src/jobs/routes.ts),associatedQuest(cloudflare/src/github/state.ts), and the board's quest URLs.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code