Skip to content

fix(dor): draft the build PR until acceptance, and un-gate the value-gate notice - #994

Merged
TaekeK merged 1 commit into
mainfrom
bugfixes/dor-draft-pr-and-gate-notify
Aug 10, 2026
Merged

fix(dor): draft the build PR until acceptance, and un-gate the value-gate notice#994
TaekeK merged 1 commit into
mainfrom
bugfixes/dor-draft-pr-and-gate-notify

Conversation

@TaekeK

@TaekeK TaekeK commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Three problems, all surfaced by #370 → PR #933, where the requestor said the PR should never have existed because they had not agreed to the solution.

1. The PR was ready-for-review from the first build

dor_build_flow.sh opened it non-draft the moment the first build finished, and every feedback round re-advertised "PR #933 (CI green)". It sat like that through eight rounds of the requestor rejecting the design — one of which added a schema migration (061_business_role_covers_itself.sql).

Green checks mean the agent's own tests pass. They say nothing about whether the solution is the one that was asked for — but a green, ready-for-review PR reads as mergeable.

Fix: open it as a draft. dor-acceptance.yml marks it ready on approve, the signal it already detects (:119) and already uses to move the board to Awaiting merge. CI runs on drafts unchanged — no workflow here filters on draft, and docs-review.yml / test-coverage-review.yml already open drafts — so verify_loop is unaffected. The only thing a draft cannot do is merge.

Branch protection and the human merge review are untouched. Wim, Taeke and Rob approve the merge exactly as today.

2. The value gate asked the wrong person the wrong question

The notice was a hardcoded @WimvandenHeijkant @TaekeK @robb536, and its framing was purely spend — "Nothing runs or spends until you do". It never showed the spec, which by then was days up-thread (on #370: spec Jul 29, gate Aug 4).

So on #370 a maintainer approved, and the PR then claimed it was built "from the approved spec" — while the requestor had never confirmed it.

Fix: address the requestor of record (authorize already resolves it via dor_requestor_of_record.sh; now exported as a job output), deep-link the certified spec, and ask whether it describes the solution they want. Anyone may still approve, so an absent requestor cannot stall a build — but the gate now records who did, and says plainly when it was not the requestor.

3. #977 made the notice unreachable

#977 moved that notice into the gate job, which carries environment: build-approval. A job gated by required reviewers runs no step until it is approved — so the notice could only ever arrive after the approval it was asking for. An issue needing the value gate would wait with nothing said on it.

Still latent: the one build since #977 merged took the autonomous path and skipped the gate. It would have bitten the next feature.

Fix: split into an ungated notify job (asks) and the gated gate job (records — post-approval by construction, which is exactly where a record belongs).

Tests

test/ci-scripts/test-dor-gate-notice.sh, wired into the existing ci-scripts job in pr.yml. 12 structural assertions, 9 fail against the pre-change tree. The load-bearing one:

PASS  the notify job has NO environment: (else the notice posts after approval)

That is the #977 shape, asserted directly so it cannot regress silently again.

One assertion was initially weaker than it looked — a whole-file grep for permission-pull-requests: write passed against main, which has read on the approve job, because another job in the same file has write. It is now scoped to the approve job.

Not addressed here

  • [Feature] Collapse managed resources under business roles in the matrix #933 itself still needs a decision — it is non-draft with eight rounds of accumulated work. This change only affects PRs opened from now on.
  • The died-sidekick gap: on Aug 5–6 two adjustment runs (31016109943, 31076341084) died when their sidekick was reclaimed; nothing was committed for ~21 hours and the feedback had to be re-posted by hand. That is the class #963's hourly sweep was built to catch, and it did not. Separate issue.

🤖 Generated with Claude Code

…gate notice

Three problems, all surfaced by #370 → PR #933, where the requestor said the PR
should never have existed because they had not agreed to the solution.

1. The PR was opened ready-for-review on the FIRST build and every feedback round
   re-advertised "CI green". #933 sat like that through eight rounds of the
   requestor rejecting the design — one of which added a schema migration. Green
   checks mean the agent's own tests pass; they say nothing about whether the
   solution is the one that was asked for, and a green ready-for-review PR reads
   as mergeable. Open it as a draft; dor-acceptance.yml takes it out of draft on
   `approve`, the signal it already detects. CI runs on drafts unchanged (no
   workflow here filters on draft), so verify_loop is unaffected — the only thing
   a draft cannot do is merge. Branch protection and the human merge review are
   untouched.

2. The value gate asked the wrong person the wrong question. Its notice was a
   hardcoded @-list and its framing was purely spend ("nothing runs or spends
   until you do"); it never showed the spec, which by then was days up-thread. On
   #370 a maintainer approved and the PR then claimed it was built "from the
   approved spec" — while the requestor had never confirmed it. Address the
   requestor of record (authorize already resolves it; now exported), deep-link
   the certified spec, and ask whether it describes the solution they want.
   Anyone may still approve so an absent requestor cannot stall a build — the
   gate now records who did and says plainly when it was not the requestor.

3. #977 moved that notice INTO the `gate` job, which holds
   `environment: build-approval`. A job gated by required reviewers runs no step
   until approved, so the notice could only arrive after the approval it was
   asking for: an issue needing the gate would wait with nothing said on it.
   Latent so far — the one build since #977 took the autonomous path and skipped
   the gate. Split into an ungated `notify` job (asks) and the gated `gate` job
   (records, post-approval by construction).

test/ci-scripts/test-dor-gate-notice.sh guards all three structurally, including
the exact #977 shape: the job posting the notice must carry no `environment:`.
12 tests; 9 fail against the pre-change tree. Wired into the existing ci-scripts
job in pr.yml.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@TaekeK
TaekeK merged commit c872f06 into main Aug 10, 2026
30 checks passed
@TaekeK
TaekeK deleted the bugfixes/dor-draft-pr-and-gate-notify branch August 10, 2026 12:20
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