Add companion-check workflow: docs PRs auto-merge once their companion code PR lands - #629
Conversation
…repos Adds a workflow posting a companion-check commit status driven by Depends-on: markers in PR bodies, so a docs PR can be approved and armed for auto-merge, then merge automatically once the feature PR it documents lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fail closed on unparseable markers, support repo#N shorthand, bound and dedupe refs, isolate per-dep/per-PR errors, restrict the cross-repo token to same-org refs, guard sweep/event races, and skip no-marker PRs in the cron sweep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round-2 review fixes: any non-ref residue on a Depends-on line fails the check; empty markers fail closed; token-confirmed 404s fail as not-found instead of pending on a token hint; the sweep reconciles every open PR (backfill + dropped-webhook healing); dep lookups are memoized per run; path-traversal segments rejected; API base from context.apiUrl. Adds a self-contained node test harness covering the embedded script. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A missing or unreadable-without-secret companion now blocks with an honest red failure instead of pending on a token hint that misdiagnoses typos; transport/auth errors keep the pending Cannot-read form. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request adds a test harness (scripts/companion-check.test.mjs) and registers it under package.json to validate the embedded GitHub script in the companion-check workflow. The reviewer pointed out that the current YAML script extraction logic is fragile and suggested replacing it with a stateful parser to robustly extract only the script block.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-629 This preview will update automatically when you push new commits. |
…on (review feedback) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-629 This preview will update automatically when you push new commits. |
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
Reviewed this as code rather than docs, since it's a pull_request_target workflow with a secret in scope on a public repo. The security posture holds up on all three of the ways that normally goes wrong:
- No checkout. There's no
actions/checkoutin the job, which is the single load-bearing property. Stating it as a maintenance rule at the top of the file is the right way to keep it holding — that invariant is exactly the kind of thing a well-meaning future PR breaks. - No PR-controlled text in
run:or expressions. The body is only ever reached aspr.bodyinsidegithub-script. The only${{ }}in the job are the secret intoenv:and a PR number into the concurrency group. - The secret can't be aimed.
allowSecretrequires a non-fork head, and the call site additionally requires a same-orgdep.owner. A fork PR naming a private repo blocks with an explanatory failure rather than probing. The tests assertfetched.length === 0— that no request was made, not merely that the answer came back right — which is the correct assertion for an oracle guard and the one I'd have expected to be missing.
The worst a fork author can do is set their own PR's status on their own head sha. Not an escalation.
I also checked the regex-state hazard, since lineRe and refRe are both /g and both reused within a call: matchAll clones rather than advancing lastIndex, and Symbol.replace resets it, so the residue check can't be poisoned by the preceding matchAll. Both are declared inside parseDeps, so nothing leaks across PRs either. Clean.
State precedence (closed → missing → unreadable → open → success) puts definitive-bad before indefinite-bad before waiting, which is right, and choosing failure over an eternal pending for a 404 is the honest call. The error path is the detail that makes this trustworthy — a gate that keeps a stale success when its own refresh fails isn't a gate, and this downgrades to pending and tests the downgrade.
Four non-blocking notes:
contents: readis unused — no checkout, no content API calls. Worth dropping precisely because this is the workflow whose permissions block people will copy.- Prose beginning a line with "Depends on:" becomes a marker.
lineReis case-insensitive and accepts a space, soDepends on: whether the caching layer lands first.parses as a marker with zero refs and fails the PR. It fails closed and the description explains itself, and residual #7 covers the adjacent fenced-code case — but the header comment tells authors what a marker is without warning them what accidentally becomes one. One line would cover it. - The test's extraction assert only checks for
parseDeps, which is defined near the top — so a truncated extraction still passes and then tests a partial script. Prettier runs over.yamlin this repo, so the 12-space indent isn't inert. Cheap hardening: also assert on something from the tail, e.g.core.setFailed. cancel-in-progressis false for the sweep, so a sweep slower than 15 minutes queues behind itself. GitHub caps the queue at one pending so it self-limits — noting it only so the cadence is read as a floor, not a guarantee.
Approving and merging. Flagging the post-merge admin sequence so it doesn't get lost, since until step 1 this posts statuses that gate nothing:
- Enable
companion-checkas a required status check in branch protection — only selectable after a first run onmain, so strictly after this merge. - One
workflow_dispatchsweep to backfill the open PRs. COMPANION_CHECK_TOKENonly if we need private-repo refs;harperis public and works onGITHUB_TOKEN.- Then #623 as the live trial, and a
Depends-on:marker on #620.
sent with Claude Opus 5
🧹 Preview CleanupThe preview deployment for this PR has been removed. |
Adds a
companion-checkcommit status driven byDepends-on:markers in PR bodies, so a docs PR documenting an unmerged code change can be approved and armed for auto-merge, then land automatically once its companion PR (e.g. harper#2147) merges. PRs without a marker get an immediatesuccess, so oncecompanion-checkbecomes a required check it never blocks ordinary docs PRs.For the human reviewer
Depends-on: noneopt-out) reaches every author's habits. To cover the typo risk this creates, everything else fails closed: empty markers, unparseable refs, and any non-ref residue on a marker line all postfailure. Reversible in-script.HarperFast/.githubis the moment to deduplicate; the committed test harness makes drift detectable.github-script, tested by extraction. The ~150-line script stays in the YAML (no build step, no action packaging);scripts/companion-check.test.mjsextracts it by its 12-space indent and runs 25+ scenario assertions, wired into the Validate matrix asWorkflow Tests. The extraction is a stateful parse of thescript: |block (until dedent) and asserts loudly if it comes back empty.pull_request_targetwith a secret in scope, on a public repo. Mitigations: the workflow never checks out PR code, the body is only ever data insidegithub-script, and the optionalCOMPANION_CHECK_TOKENis used exclusively for same-org refs on non-fork PRs — a fork PR referencing a private repo blocks with an explanatory failure rather than probing with the secret. A maintenance rule at the top of the file states the invariant.failure"not found or inaccessible" rather than an eternalpendingblaming a token. Both states block; the failure is more honest about needing human attention.pending, which blocks); markers inside fenced code blocks are parsed as live (quote the word, not the marker form, when writing about it); statuses (not check runs) were chosen for required-check simplicity, so there is no re-run button — the 15-minute sweep or aworkflow_dispatchis the retry path.Not in this PR (needs repo admin, after merge): enable the
companion-checkrequired status check in branch protection, run oneworkflow_dispatchbackfill sweep, and optionally addCOMPANION_CHECK_TOKEN(fine-grained PAT or app token with PR read on harper-pro) for docs PRs that depend on private-repo PRs. Repo auto-merge (allow_auto_merge) is already enabled.Verification
npm run test:workflows(added to the Validate matrix) passes: 25+ assertions over the extracted production script covering marker parsing (all three documented forms, shorthand, dedupe, cap, empty/partial/traversal markers failing closed), state reduction (merged/open/closed/missing/unreadable precedence), sweep semantics (no-repost dedup, no-marker backfill, dropped-webhook healing, race-guard skip on changed PRs), error isolation (HTTP 500 degrades to pending; a thrown refresh posts pending rather than preserving stale success), and secret guards (withheld for foreign orgs and fork PRs).npm run format:checkclean. The workflow itself cannot run end-to-end from this branch (schedulefires on the default branch only, and the status context becomes selectable in branch protection only after a firstmainrun) — post-merge verification is a manualworkflow_dispatchsweep plus arming #623 as the live trial.Review coverage
Authored by Claude Fable 5. Cross-model review across 4 rounds (full @ f2c4478, deltas @ 2fd988e / e6093a1, final @ 71750b6 = HEAD): codex graded leg ✓ (gpt-5.6-sol @ HEAD), gemini via agy (default model) ✓ every round, Harper-domain adjudication ✓ (claude-opus-5, last @ e6093a1; pruned on the final narrow delta), cursor-grok ✗ (output-format failure), cursor-composer ✗ (pruned round 1, format failure @ e6093a1). Verdict trail CHANGES → CHANGES → CHANGES → COMMENTS; the remaining findings are the accepted residuals in the ledger above.
Human-Review-Need: 4 @ b3681a1