Skip to content

⚠️ CI FIXTURE — DO NOT MERGE, DO NOT CLOSE - #370

Draft
mabry1985 wants to merge 1 commit into
mainfrom
ci/gh-tier-fixture-do-not-merge
Draft

⚠️ CI FIXTURE — DO NOT MERGE, DO NOT CLOSE#370
mabry1985 wants to merge 1 commit into
mainfrom
ci/gh-tier-fixture-do-not-merge

Conversation

@mabry1985

@mabry1985 mabry1985 commented Aug 31, 2026

Copy link
Copy Markdown
Member

This PR is infrastructure. It exists to stay open.

tests/test_worktree_gh.py — the requires_gh tier from #361 S2 — exercises 12 worktree.py seams against real GitHub instead of a mock. Reading a real PR requires a real PR.

CI resolves the fixture as:

PB_GH_FIXTURE_PR: ${{ vars.PB_GH_FIXTURE_PR || github.event.pull_request.html_url }}

On a pull_request run the PR under test serves. On a push to main there is none, and PB_REQUIRE_GH=1 deliberately turns "no fixture" into a build failure rather than a silent skip — the #136 lesson, because a tier that quietly skips is not coverage. The repo variable PB_GH_FIXTURE_PR points here so main keeps its real-GitHub coverage.

Why this matters

Those seams had zero real-API coverage until #361. That is how #354 shipped inert for a day: POST /check-runs is GitHub-App-only and 403s under the board's PAT, but every unit test mocked gh — so 1,650 tests passed while the review verdict published to nothing, and a green PR sat there being rebuilt.

Rules
Never merge it is a draft; merging removes the fixture
Never close the tier asserts OPEN; closing turns main red
Never push the head is read, not written

Status writes go to the commit under test, never to this head — GitHub caps statuses at 1,000 per (sha, context), so writing to a stable head would be a slow time bomb (found in review on #361 S2).

If it is ever lost: open any permanently-open PR and repoint vars.PB_GH_FIXTURE_PR.

@mabry1985 @claude
ci: permanently-open fixture PR for the real-GitHub test tier
94dc87f 
Not for merge. tests/test_worktree_gh.py needs a real, open PR to read; on a push
to main there is no PR under test, and PB_REQUIRE_GH=1 correctly fails rather than
silently skipping. vars.PB_GH_FIXTURE_PR points here so main keeps real-GitHub
coverage.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown

requires_gh tier probe — idempotent marked comment (safe to leave; updated in place).

This was referenced Aug 31, 2026
Merged
Closed
mabry1985 added a commit that referenced this pull request Sep 10, 2026
@mabry1985 @claude
fix(board): review findings on the attach edge (#402)
d98aca9 
Adversarial review of #437 found seven defects; each has a test that fails on the previous
head (8c45cd7) and passes here.

1. Ready-gate bypass: eligibility used the board STATE, and `blocked` is a flag on a lane, so
   a blocked backlog card (never Ready) or a blocked epic went straight into review. It is
   now the lane underneath the block (status without the flag) -- ready/in_progress, or
   in_review under a block -- and only an issue_type `feature`.
2. Re-attaching a blocked card's OWN PR lifted the block (e.g. "review findings persist --
   needs human review") and re-armed the gate without a human. A card already tracking the
   PR is a no-op in review and refused anywhere else; the refusal names the block and that
   unblocking is a deliberate board_unblock_feature.
3. The health sweep moved cards on reads taken before a gh round-trip, outside the claim lock
   the attach writes under: the orphan pass and the blocked self-heal requeued an attach that
   had just landed. Both now re-read the card under the same lock and act only if it is
   still what they read (positive evidence of a move skips). When the sweep adopts the PR
   first, the attach reports already_attached, arms review-pending if the gate is on and no
   review state exists, and still writes its audit line.
4. pr_identity is ratcheted REAL but its real-gh tests never ran in CI. They move to
   tests/test_attach_pr_gh_402.py (no br needed) and the `test (real gh)` job runs it against
   the pinned fixture (#370, vars.PB_GH_FIXTURE_PR). The head check now precedes the fork
   check, so the refusal test doesn't depend on the fixture's origin.
5. A replacement PR kept the old PR's merged-verified stamp, letting the merge edge treat new
   code as gated; it is dropped with the other head pins.
6. A gh timeout / missing checkout on the replacement read escaped the tool and route; every
   attach gh read now converts WorktreeError/OSError into an actionable BoardError. The
   tools no-raise contract test now covers async tools (skipping them is how this slipped).
7. The loop parsed review_gate with bool() ("false" -> ON) while attach and the listings used
   knob_bool; the loop now uses knob_bool.

Also: the audit comment is written last and cannot un-say a landed attach (a failure is
reported as a warning); the docs say drafts attach (auto-merge holds them) and that attached
code has not run the board's pre-PR checks (CI + the review gate still apply). The
`_TaskStore`/`_BlockedStore` fakes in tests/test_loop.py now model the re-read the sweep does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F2V6GRejF7mNukAoYjj2Av
@mabry1985 mabry1985 mentioned this pull request Sep 10, 2026
Merged
mabry1985 added a commit that referenced this pull request Sep 10, 2026
@mabry1985 @claude
fix(board): review findings on the attach edge (#402)
a736f41 
Adversarial review of #437 found seven defects; each has a test that fails on the previous
head (8c45cd7) and passes here.

1. Ready-gate bypass: eligibility used the board STATE, and `blocked` is a flag on a lane, so
   a blocked backlog card (never Ready) or a blocked epic went straight into review. It is
   now the lane underneath the block (status without the flag) -- ready/in_progress, or
   in_review under a block -- and only an issue_type `feature`.
2. Re-attaching a blocked card's OWN PR lifted the block (e.g. "review findings persist --
   needs human review") and re-armed the gate without a human. A card already tracking the
   PR is a no-op in review and refused anywhere else; the refusal names the block and that
   unblocking is a deliberate board_unblock_feature.
3. The health sweep moved cards on reads taken before a gh round-trip, outside the claim lock
   the attach writes under: the orphan pass and the blocked self-heal requeued an attach that
   had just landed. Both now re-read the card under the same lock and act only if it is
   still what they read (positive evidence of a move skips). When the sweep adopts the PR
   first, the attach reports already_attached, arms review-pending if the gate is on and no
   review state exists, and still writes its audit line.
4. pr_identity is ratcheted REAL but its real-gh tests never ran in CI. They move to
   tests/test_attach_pr_gh_402.py (no br needed) and the `test (real gh)` job runs it against
   the pinned fixture (#370, vars.PB_GH_FIXTURE_PR). The head check now precedes the fork
   check, so the refusal test doesn't depend on the fixture's origin.
5. A replacement PR kept the old PR's merged-verified stamp, letting the merge edge treat new
   code as gated; it is dropped with the other head pins.
6. A gh timeout / missing checkout on the replacement read escaped the tool and route; every
   attach gh read now converts WorktreeError/OSError into an actionable BoardError. The
   tools no-raise contract test now covers async tools (skipping them is how this slipped).
7. The loop parsed review_gate with bool() ("false" -> ON) while attach and the listings used
   knob_bool; the loop now uses knob_bool.

Also: the audit comment is written last and cannot un-say a landed attach (a failure is
reported as a warning); the docs say drafts attach (auto-merge holds them) and that attached
code has not run the board's pre-PR checks (CI + the review gate still apply). The
`_TaskStore`/`_BlockedStore` fakes in tests/test_loop.py now model the re-read the sweep does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F2V6GRejF7mNukAoYjj2Av
mabry1985 added a commit that referenced this pull request Sep 10, 2026
@mabry1985 @claude
feat(board): attach an externally opened PR to the card it belongs to (…
7db1ca1 
…#402) (#437)

* feat(board): attach an externally opened PR to the card it belongs to (#402)

The loop adopts a PR it did not see opened in exactly one place: crash recovery finds the
PR whose head is the card's canonical branch and moves the card to in_review. A PR a human
opened had no edge. On 2026-09-07 bd-ezs7's recovered implementation went up as PR #3369 on
the card's own branch; the card sat terminal-blocked while the PR went through CI and review
invisible to the board (re-dispatching would have force-removed the candidate worktree the
work still lived in), and the PM hand-closed it with board_mark_done after the merge.

board_attach_pr / POST /features/{fid}/attach-pr is that edge, no wider than recovery:

- the PR must be the one the board would find for the card itself: OPEN, in the card's
  project repo and not a fork, head on the card's canonical branch (every later edge --
  fix-round resume, recovery, the reap -- keys on it; a PR on another branch would be
  abandoned by the first fix round, which opens a second PR), base on the project's base;
- the card must be an in-flight coding card (ready / in_progress / blocked) with no PR of
  its own in review and no open dependency; a card whose earlier PR is CLOSED may take the
  new one in its place (an open earlier PR would be orphaned, a merged one means it's done);
- the loop must not be working it (live drive, claimed build, running review gate) --
  refused, not overridden -- and the write runs under the loop's claim lock, so a ready
  card cannot be claimed halfway through.

The write is one `br update` (store.attach_pr): in_review + external_ref, dropping
ready/blocked/blocked-class and any stale review verdict or head pin, and arming
review-pending when the review gate is on (else the merge edge waits forever for a verdict
nothing produces). The ordinary reconcile then drives it: CI, review, rebase, merge -> done.
An `attached PR:` comment records who, from what state, and why. Every refusal changes
nothing and names what to do instead.

New gh seam worktree.pr_identity (one `gh pr view`), classified REAL and read against the
pinned PR in the GitHub tier; store.attach_pr classified REAL (every shape through real br).

Also: the tools-doc guard matched only `def board_*`, so the async board_dispatch shipped
undocumented and board_attach_pr would have too; it now matches async defs, and both are
documented.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F2V6GRejF7mNukAoYjj2Av

* docs: changelog fragment for #437

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F2V6GRejF7mNukAoYjj2Av

* fix(board): the attach refusal tells the operator the exact branch to push to (#402)

The wrong-branch refusal named the card's canonical branch but phrased the fix as "push
the work there". It now leads with the instruction itself -- push the work to branch
'feat/<id>-<slug>', open the PR from it, and attach that PR -- and the test pins that
instruction, not merely that the branch name appears somewhere in the message.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F2V6GRejF7mNukAoYjj2Av

* fix(board): review findings on the attach edge (#402)

Adversarial review of #437 found seven defects; each has a test that fails on the previous
head (8c45cd7) and passes here.

1. Ready-gate bypass: eligibility used the board STATE, and `blocked` is a flag on a lane, so
   a blocked backlog card (never Ready) or a blocked epic went straight into review. It is
   now the lane underneath the block (status without the flag) -- ready/in_progress, or
   in_review under a block -- and only an issue_type `feature`.
2. Re-attaching a blocked card's OWN PR lifted the block (e.g. "review findings persist --
   needs human review") and re-armed the gate without a human. A card already tracking the
   PR is a no-op in review and refused anywhere else; the refusal names the block and that
   unblocking is a deliberate board_unblock_feature.
3. The health sweep moved cards on reads taken before a gh round-trip, outside the claim lock
   the attach writes under: the orphan pass and the blocked self-heal requeued an attach that
   had just landed. Both now re-read the card under the same lock and act only if it is
   still what they read (positive evidence of a move skips). When the sweep adopts the PR
   first, the attach reports already_attached, arms review-pending if the gate is on and no
   review state exists, and still writes its audit line.
4. pr_identity is ratcheted REAL but its real-gh tests never ran in CI. They move to
   tests/test_attach_pr_gh_402.py (no br needed) and the `test (real gh)` job runs it against
   the pinned fixture (#370, vars.PB_GH_FIXTURE_PR). The head check now precedes the fork
   check, so the refusal test doesn't depend on the fixture's origin.
5. A replacement PR kept the old PR's merged-verified stamp, letting the merge edge treat new
   code as gated; it is dropped with the other head pins.
6. A gh timeout / missing checkout on the replacement read escaped the tool and route; every
   attach gh read now converts WorktreeError/OSError into an actionable BoardError. The
   tools no-raise contract test now covers async tools (skipping them is how this slipped).
7. The loop parsed review_gate with bool() ("false" -> ON) while attach and the listings used
   knob_bool; the loop now uses knob_bool.

Also: the audit comment is written last and cannot un-say a landed attach (a failure is
reported as a warning); the docs say drafts attach (auto-merge holds them) and that attached
code has not run the board's pre-PR checks (CI + the review gate still apply). The
`_TaskStore`/`_BlockedStore` fakes in tests/test_loop.py now model the re-read the sweep does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F2V6GRejF7mNukAoYjj2Av

* fix(board): the attach edge takes the card's lock (#402)

Rebased onto main, where #432 put the multi-call task edges (record_delivery, requeue,
record_verification) under a per-card re-entrant lock: `_br_lock` single-flights each `br`
CALL, not a read-then-write SEQUENCE. store.attach_pr is exactly such a sequence -- read the
card, validate it, write the transition, write the audit line -- so it takes the same lock,
and a concurrent requeue of the same card from another thread (a PM tool) can't land between
its read and its write.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F2V6GRejF7mNukAoYjj2Av

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@mabry1985