Repository navigation
Skip Design Decision Gate ADR writes for already-closed PRs - #69504
Conversation
Avoid generating an ADR and reporting incomplete when a PR merges and its branch disappears before the gate starts. Keep all open-PR ADR enforcement unchanged and cover lifecycle handling with mocked same-source regressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Verdict
I did not find a changed-line issue that clearly blocks this PR.
Scope and limits
Reviewed the pre-fetched diff only; I did not execute the workflow or tests. The closed/merged short-circuit looks internally consistent, and the regression script covers the key OPEN/CLOSED/MERGED/manual-dispatch paths for this specific fix.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 58.4 AIC · ⌖ 5.84 AIC · ⊞ 12.7K
Comment /review to run again
| --arg pr_number "${PR_NUMBER:-}" \ | ||
| '{ | ||
| pr_number: null, | ||
| pr_number: (if $pr_number == "" then null else ($pr_number | tonumber) end), |
There was a problem hiding this comment.
This skip path coerces the raw workflow_dispatch input with tonumber, which will abort prefetch if the operator passes a PR URL/ref that gh pr view resolves successfully.
💡 Why this is a real regression
workflow_dispatch inputs are untyped strings. gh pr view "$PR_NUMBER" accepts more than bare numeric IDs, so a closed PR supplied as a URL or another resolvable selector reaches this branch and then dies while writing adr-prefetch-summary.json. That turns the intended noop skip into a hard failure.
Use the validated .number already returned in pr.json, or guard the conversion before calling tonumber.
| /^ run: \|$/ { body = 1; next } | ||
| body && /^ / { sub(/^ /, ""); print; next } | ||
| body && /^$/ { print; next } | ||
| body { exit } |
There was a problem hiding this comment.
This harness reads the markdown source instead of the compiled .lock.yml, so the test can pass while the workflow GitHub actually runs still contains stale prefetch logic.
💡 Why this blocks the fix
The regression you are fixing is in the production workflow, but this test extracts the shell body from .github/workflows/design-decision-gate.md. If the lockfile is stale or recompilation is skipped, CI still goes green here while the shipped workflow keeps the old behavior.
A safer pattern is to exercise design-decision-gate.lock.yml directly, or assert that the compiled lock step matches the markdown source before running the harness. That way this test actually proves the runnable workflow was updated.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving.
📋 Key Themes & Highlights
Verification performed
Independently re-extracted and ran the shell body from the PR's committed HEAD content (not the locally-drifted sandbox working tree) across all 6 scenarios: OPEN, CLOSED, MERGED, workflow_dispatch without PR, workflow_dispatch with PR, and gh pr view HTTP 403 failure. All behaved as asserted by the new scripts/design-decision-gate-prefetch_test.sh.
Positive Highlights
- ✅ Root cause fix (
/diagnosing-bugs): addsstateto the existinggh pr view --jsoncall (no extra API round trip) and gates the skip decision before any ADR generation or push attempt — this matches the actual observed failure mode (PR closed before prefetch), not just a symptom patch. - ✅ Regression coverage (
/tdd): follows the existing isolated-shell-execution pattern frompr-data-prefetch_test.sh, asserts exactghcall counts (so an extra unwanted API call would fail the test), and covers the open/closed/merged/dispatch/failure matrix. - ✅ Failed PR-metadata requests still hard-fail via
set -euo pipefailrather than silently downgrading to a skip — preserves the safety invariant that an unverified PR state never results in a false skip. - ✅ PR description transparently discloses the remaining (narrower) race window rather than overclaiming atomic protection.
No actionable issues found.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 60.9 AIC · ⌖ 14.8 AIC · ⊞ 10.1K
Comment /matt to run again
There was a problem hiding this comment.
🔵 Needs a closer look
Independent compiler and scanner validation was unavailable for this production workflow change.
No open issues 🎉
What changed in this PR
Updates Design Decision Gate to skip ADR generation when prefetch confirms that a PR is closed or merged.
Changes:
- Fetch current PR state and reuse the existing skip/noop path.
- Preserve open-PR enforcement and metadata-error failures.
- Regenerate the workflow lock and add shell regressions to
make test-scripts.
| File | Description |
|---|---|
scripts/design-decision-gate-prefetch_test.sh |
Tests PR states, manual dispatch, and metadata failures. |
Makefile |
Registers the new regression script. |
.github/workflows/design-decision-gate.md |
Adds confirmed-closure skipping and explicit noop guidance. |
.github/workflows/design-decision-gate.lock.yml |
Updates generated prefetch code and metadata. |
Review effort: Max
|
🎉 This pull request is included in a new release. Release: |
Root cause
Run 38967187202 was not an ADR policy rejection or another model failure. PR #69467 merged at 21:07:47Z, the same second the run was created. At 21:09:29Z,
Checkout PR branchcould no longer fetch its deleted branch and explicitly logged that the PR was now closed, treating checkout failure as expected. The agent still received an enforcement task in a detached merge checkout (4bbb40da012788fecf1cd437db782e0d649fad68), generated an ADR, failed both push attempts, and emittedreport_incomplete. Its fallback ADR comment succeeded; the conclusion job correctly failed on the incomplete report.Change
skip_reasonenvelope and require noop without ADR generation/push.make test-scripts.This addresses closure before prefetch, as observed in the failed run; it does not claim atomic protection against a PR closing later during the agent's work. No shared runtime failure handling or model settings are changed.
Evidence and validation
make recompile: 344/344; normal scoped compilation passed.make agent-report-progresspassed, including all workflow drift checks. No Go files changed, so impacted Go tests were correctly skipped.workflow_dispatchconcurrency/job-discriminator compiler warning (plus existing experimental-feature notice). Diagnostic lock was emitted and shellcheck passed; other optional scanners were not run. Normal publication lock was regenerated with the repository compiler; no dry-run behavior is published.No workflows were dispatched or rerun, and no PR was merged.