Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions .github/scripts/dor_build_flow.sh
Original file line number Diff line number Diff line change
Expand Up @@ -195,11 +195,18 @@ Leave your changes in the working tree — do NOT commit, push or open a PR.%s'
fi

# 2. Open the PR (BOT token — GITHUB_TOKEN can't create PRs here).
#
# DRAFT, deliberately. A ready-for-review PR with green checks reads as "this can go to main", and
# for an agent-built change that is a lie: CI green means the agent's own tests pass, not that the
# requestor accepted the solution. #933 sat non-draft and green through EIGHT rounds of the
# requestor rejecting the design. dor-acceptance.yml marks it ready the moment they reply `approve`.
# CI runs on drafts exactly as on any PR (no workflow here filters on draft), so verify_loop below
# is unaffected — the only thing a draft cannot do is merge.
pr=$(gh pr list --repo "$REPO" --head "$BRANCH" --state open --json number --jq '.[0].number // empty')
if [ -z "$pr" ]; then
pr=$(GH_TOKEN="$BOARD_TOKEN" gh pr create --repo "$REPO" --base main --head "$BRANCH" \
pr=$(GH_TOKEN="$BOARD_TOKEN" gh pr create --repo "$REPO" --base main --head "$BRANCH" --draft \
--title "$(gh issue view "$ISSUE" --repo "$REPO" --json title --jq '.title')" \
--body "$(printf 'Closes #%s\n\nBuilt autonomously by the DoR build agent from the approved spec. Functional-test env: %s\n\nDo not merge until CI is green and the requestor has accepted.' "$ISSUE" "$URL")" \
--body "$(printf 'Closes #%s\n\n> **Requestor acceptance: not yet.** This PR is a draft until the requestor replies `approve` on #%s. It becomes ready for review then, and not before.\n\nBuilt autonomously by the DoR build agent from the certified spec. Functional-test env: %s\n\nGreen checks here mean the agent'\''s own tests pass — they say nothing about whether the solution is the one that was asked for.' "$ISSUE" "$ISSUE" "$URL")" \
| grep -oE '[0-9]+$') || bail "could not open the PR"
fi
claim_sidekick "$pr" # ~/.dor-reservation + the sk:<label> that reset/feedback dispatch off
Expand Down
13 changes: 11 additions & 2 deletions .github/workflows/dor-acceptance.yml
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,8 @@ jobs:
private-key: ${{ secrets.BOT_PRIVATE_KEY }}
owner: Fortigi
permission-organization-projects: write
permission-pull-requests: read
# write (was read): acceptance is what takes the PR out of draft — see below.
permission-pull-requests: write
permission-issues: read
- name: Move to Awaiting merge + notify the Product Board
env:
Expand All @@ -159,9 +160,17 @@ jobs:
set -uo pipefail
pr=$(GH_TOKEN="$BOARD_TOKEN" gh pr list --repo "$REPO" --head "dor/issue-${ISSUE}" --state open --json number --jq '.[0].number // empty')
GH_TOKEN="$BOARD_TOKEN" bash .github/scripts/dor_set_status.sh "$ISSUE" awaiting-merge || true
# Acceptance is the ONLY thing that takes the PR out of draft. The build agent opens it as
# a draft precisely so that "green checks" can never be read as "ready to merge" before
# the requestor has said the solution is right. This is that moment.
if [ -n "${pr:-}" ]; then
GH_TOKEN="$BOARD_TOKEN" gh pr ready "$pr" --repo "$REPO" >/dev/null 2>&1 \
&& echo "::notice::PR #${pr} marked ready for review (accepted by ${ACTOR})" \
|| echo "::warning::could not mark PR #${pr} ready for review — do it by hand"
fi
# Drop build-done so the feedback loop stops firing; the board is now canonical.
gh issue edit "$ISSUE" --repo "$REPO" --remove-label build-done >/dev/null 2>&1 || true
gh issue comment "$ISSUE" --repo "$REPO" --body "$(printf '✅ @%s approved. @WimvandenHeijkant @TaekeK @robb536 — PR #%s is ready for **final merge**.' "$ACTOR" "${pr:-?}")" >/dev/null 2>&1 || true
gh issue comment "$ISSUE" --repo "$REPO" --body "$(printf '✅ @%s approved — PR #%s is out of draft and ready for **final merge**. @WimvandenHeijkant @TaekeK @robb536 — the merge review is yours as usual.' "$ACTOR" "${pr:-?}")" >/dev/null 2>&1 || true
echo "::notice::#${ISSUE} accepted by ${ACTOR} → Awaiting merge (PR #${pr:-?})"

# Which sidekick holds this feature's live env? The build recorded it as an sk:<label> on the issue
Expand Down
62 changes: 59 additions & 3 deletions .github/workflows/dor-build-agent.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ jobs:
issues: write
outputs:
is-member: ${{ steps.auth.outputs.is-member }}
# Exported so the value-gate notice can address the person whose feature this is by name,
# and so the post-approval record can say whether the approver WAS that person.
requestor: ${{ steps.req.outputs.login }}
steps:
- name: Checkout the authorize action + requestor resolver
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
Expand Down Expand Up @@ -133,9 +136,50 @@ jobs:
echo "::notice::autonomous build approved by policy for #$ISSUE"
gh issue comment "$ISSUE" --repo "$REPO" --body "🤖 Building autonomously — certified \`confidence: certain\`, with a blast radius clear of migrations, auth and credential handling, so no value gate is needed. The **merge review is still yours**, and the evidence bundle on the PR is what it reviews." >/dev/null 2>&1 || true

# Posts the "go and approve" notice. This job MUST NOT carry `environment:` — a job gated by
# required reviewers does not run a single step until it has been approved, so a notice posted
# from inside the gate can only ever arrive AFTER the approval it is asking for. #977 moved this
# step into the gated `gate` job below and silently broke exactly that: an issue needing the value
# gate would wait with nothing said on it. Keep the notice here and the gate below, separate.
# (test/ci-scripts/test-dor-gate-notice.sh asserts this separation so it can't regress again.)
notify:
needs: [authorize, policy]
if: needs.authorize.outputs.is-member == 'true' && needs.policy.outputs.auto != 'true'
runs-on: ubuntu-latest
permissions:
issues: write
steps:
- name: Post the value-gate notice (addressed to the requestor, quoting the spec)
env:
GH_TOKEN: ${{ github.token }}
ISSUE: ${{ github.event.issue.number }}
REPO: ${{ github.repository }}
REQUESTOR: ${{ needs.authorize.outputs.requestor }}
RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
run: |
set -uo pipefail
# Deep-link the certified spec. It is posted by the spec agent, often days before this
# gate, so by now it is buried up-thread — asking someone to confirm a solution without
# putting it in front of them is how #370 shipped a spec its requestor never agreed to.
spec="$(gh api "repos/$REPO/issues/$ISSUE/comments" --paginate \
--jq '[.[] | select(.body | test("Certified spec"))] | last | .html_url // empty' 2>/dev/null || echo '')"
if [ -n "$spec" ]; then
spec_line="$(printf '📋 **The certified spec is [here](%s)** — read it before approving.' "$spec")"
else
spec_line='📋 The certified spec is the last bot comment above — read it before approving.'
fi
# Address the requestor of record by name. Anyone on the product board may still approve
# (an absent requestor must not stall a build); who actually did is recorded after the fact
# by the `gate` job, so "approved" can never be mistaken for "the requestor agreed".
gh issue comment "$ISSUE" --repo "$REPO" --body "$(printf \
'🗳️ @%s — this is your request, so this one is yours to confirm: **does the spec below describe the solution you want?**\n\n%s\n\nIf it does: **[Review & approve to build →](%s)** (open the run → *Review deployments* → approve `build-approval`). If it does not, say what is wrong here instead and it goes back to spec — that is far cheaper than correcting it once the code exists.\n\ncc @WimvandenHeijkant @TaekeK — any of you can approve if @%s is unavailable; the issue will record who did. Nothing runs or spends until someone does.' \
"$REQUESTOR" "$spec_line" "$RUN_URL" "$REQUESTOR")" >/dev/null 2>&1 || true

# Holds the Environment gate, and runs ONLY when policy says a human is needed. Its success is what
# the build waits on, so approval semantics are unchanged for everything that still needs one:
# features, low-confidence bugs, restricted blast radii, anything over the cap.
# Every step here runs POST-approval by construction — which is exactly why the record of who
# approved belongs here, and the notice asking for approval does not.
gate:
needs: [authorize, policy]
if: needs.authorize.outputs.is-member == 'true' && needs.policy.outputs.auto != 'true'
Expand All @@ -144,15 +188,27 @@ jobs:
permissions:
issues: write
steps:
- name: Post the one-click approval link (value gate)
- name: Record who passed the value gate, and whether that was the requestor
env:
GH_TOKEN: ${{ github.token }}
ISSUE: ${{ github.event.issue.number }}
REPO: ${{ github.repository }}
RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
REQUESTOR: ${{ needs.authorize.outputs.requestor }}
RUN_ID: ${{ github.run_id }}
run: |
set -uo pipefail
gh issue comment "$ISSUE" --repo "$REPO" --body "$(printf '🗳️ @WimvandenHeijkant @TaekeK @robb536 — ready for the **value gate**. **[Review & approve to build →](%s)** (open the run → *Review deployments* → approve `build-approval`). Nothing runs or spends until you do.' "$RUN_URL")" >/dev/null 2>&1 || true
# Who cleared the Environment gate. Best-effort: if the API is unavailable we still say so
# rather than implying the requestor agreed.
who="$(gh api "repos/$REPO/actions/runs/$RUN_ID/approvals" \
--jq '[.[] | select(.state == "approved")] | last | .user.login // empty' 2>/dev/null || echo '')"
if [ -z "$who" ]; then
line='✅ Value gate passed — the approver could not be read back from the API.'
elif [ "$who" = "$REQUESTOR" ]; then
line="$(printf '✅ Value gate passed by @%s — **the requestor**, so the spec is confirmed by the person who asked for it.' "$who")"
else
line="$(printf '✅ Value gate passed by @%s — **not the requestor** (@%s). This approves the spend, not the design: @%s has not confirmed the spec, so treat the resulting PR as unagreed until they accept it.' "$who" "$REQUESTOR" "$REQUESTOR")"
fi
gh issue comment "$ISSUE" --repo "$REPO" --body "$line" >/dev/null 2>&1 || true

build:
needs: [authorize, policy, gate]
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -874,6 +874,7 @@ jobs:
bash test/ci-scripts/test-path-gates.sh
bash test/ci-scripts/test-crawler-scope.sh
bash test/ci-scripts/test-commit-range.sh
bash test/ci-scripts/test-dor-gate-notice.sh

# ── Gate: single required check for branch protection ───────────────────
# Replaces the individual job requirements in branch protection. Update branch
Expand Down
3 changes: 3 additions & 0 deletions changes/dor-draft-pr-and-gate-notify.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
- A pull request opened by the automated build now starts as a draft and only becomes ready for review once the requestor accepts the result. Previously it was opened ready for review with passing checks from the first build onwards, which read as "this can go to main" while the requestor was still rejecting the solution — one feature went through eight rounds of feedback in that state. The final merge review is unchanged and still yours.
- The request to approve a build is now addressed to the person who asked for the feature, quotes a link to the specification that was written for it, and asks them to confirm it describes the solution they want — rather than only asking whether the work is worth doing. Anyone on the product board can still approve if that person is unavailable, and the issue now records who approved and whether that was the requestor.
- Fixed: the "review and approve to build" notice could not reach anyone, because it was being posted from inside the step that waits for that same approval. Any request needing approval would have sat waiting with nothing said on the issue at all.
112 changes: 112 additions & 0 deletions test/ci-scripts/test-dor-gate-notice.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
#!/usr/bin/env bash
# Structural tests for the DoR value gate and the draft-PR contract.
#
# These guard two invariants that are invisible in review and expensive when broken — both were
# broken in production before this file existed:
#
# 1. The job that POSTS the "go and approve" notice must not carry `environment:`. A job gated by
# required reviewers runs no step until it is approved, so a notice posted from inside the gate
# arrives after the approval it asks for — the issue just goes quiet. #977 moved the notice
# into the gated job and did exactly this.
# 2. The build agent must open its PR as a DRAFT, and only acceptance may take it out of draft.
# A ready-for-review PR with green checks reads as mergeable; for an agent-built change that is
# false until the requestor has accepted it (#933 sat green through eight rejected rounds).
#
# Usage: bash test/ci-scripts/test-dor-gate-notice.sh

set -euo pipefail

REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)"
AGENT="$REPO_ROOT/.github/workflows/dor-build-agent.yml"
ACCEPT="$REPO_ROOT/.github/workflows/dor-acceptance.yml"
FLOW="$REPO_ROOT/.github/scripts/dor_build_flow.sh"

PASS=0
FAIL=0

assert() {
local desc="$1" expected="$2" actual="$3"
if [ "$actual" = "$expected" ]; then
echo " PASS $desc"
PASS=$((PASS + 1))
else
echo " FAIL $desc"
echo " expected: $expected"
echo " actual: $actual"
FAIL=$((FAIL + 1))
fi
}

# Print the body of one top-level job from a workflow: everything from ` <name>:` at two-space
# indent up to the next two-space-indented key. Enough to see that job's own `environment:`/steps
# without a YAML parser (the runner has no PyYAML guarantee, and these tests must stay dependency-free).
job_body() {
local file="$1" job="$2"
awk -v want=" ${job}:" '
$0 == want { inside = 1; next }
inside && /^ [a-zA-Z_-]+:/ { exit }
inside { print }
' "$file"
}

echo "DoR value-gate + draft-PR structure"
echo

# ── 1. The notice must be posted from an UNGATED job ─────────────────────────

notify_body="$(job_body "$AGENT" notify)"

assert "a 'notify' job exists to post the value-gate notice" \
"true" "$([ -n "$notify_body" ] && echo true || echo false)"

assert "the notify job posts the approval link" \
"true" "$(printf '%s' "$notify_body" | grep -q 'Review & approve to build' && echo true || echo false)"

# THE regression guard. If this fails, the notice can only arrive after the approval it requests.
assert "the notify job has NO environment: (else the notice posts after approval)" \
"false" "$(printf '%s' "$notify_body" | grep -qE '^ +environment:' && echo true || echo false)"

# ── 2. The gate still holds the environment, and only records ────────────────

gate_body="$(job_body "$AGENT" gate)"

assert "the gate job still holds the build-approval environment" \
"true" "$(printf '%s' "$gate_body" | grep -qE '^ +environment: build-approval' && echo true || echo false)"

assert "the gate job records who approved" \
"true" "$(printf '%s' "$gate_body" | grep -q 'approvals' && echo true || echo false)"

# The gate must not be the thing asking for approval — that is the notify job's job.
assert "the gate job does NOT post the approval link" \
"false" "$(printf '%s' "$gate_body" | grep -q 'Review & approve to build' && echo true || echo false)"

# ── 3. The notice is addressed to the requestor of record ────────────────────

assert "authorize exports the requestor of record" \
"true" "$(grep -qE '^ +requestor: \$\{\{ steps\.req\.outputs\.login \}\}' "$AGENT" && echo true || echo false)"

assert "the notice addresses the requestor, not a hardcoded list" \
"true" "$(printf '%s' "$notify_body" | grep -q 'REQUESTOR: ${{ needs.authorize.outputs.requestor }}' && echo true || echo false)"

# ── 4. The PR is opened as a draft, and only acceptance clears it ────────────

assert "the build agent opens the PR as a draft" \
"true" "$(grep -qE 'gh pr create .*--draft' "$FLOW" && echo true || echo false)"

approve_body="$(job_body "$ACCEPT" approve)"

assert "acceptance marks the PR ready for review" \
"true" "$(printf '%s' "$approve_body" | grep -q 'gh pr ready' && echo true || echo false)"

# Scoped to the approve job on purpose: another job in this file already holds pull-requests: write,
# so a whole-file grep passes even when the approve job only has `read` and `gh pr ready` would 403.
assert "the approve job holds pull-requests: write (gh pr ready needs it)" \
"true" "$(printf '%s' "$approve_body" | grep -qE '^ +permission-pull-requests: write' && echo true || echo false)"

# `gh pr ready` must appear ONLY in the acceptance flow — nothing else may undraft the PR.
assert "nothing outside acceptance takes the PR out of draft" \
"0" "$(grep -rl 'gh pr ready' "$REPO_ROOT/.github" 2>/dev/null | grep -cv 'dor-acceptance.yml' || true)"

echo
echo " $PASS passed, $FAIL failed"
[ "$FAIL" -eq 0 ]