Draft PRs: open-as-draft / mark-ready transitions (fixes #613) #616

Merged

crueber merged 1 commit from fix/issue-613 into main

2026-09-15 23:45:03 +00:00

crueber commented

2026-09-15 23:38:58 +00:00

Owner

Implements the binding option-a decision: draft PRs creatable/flippable via API+UI with tests.

API: POST ../pulls accepts draft (omitted = ready); PUT ../pulls/{num} {draft: bool} flips ready<>draft for author-or-triage (same roles as state transitions), 409 on merged. Flips append draft_changed thread events (from/to draft/ready labels, mirroring state_changed) with ready_for_review/converted_to_draft notify + stream fan-out; no-op flips write nothing. Merge task refuses drafts alongside the other step-1 gates (narrated 409, no publish).
UI: PullNew open-as-draft checkbox (ReleaseNew idiom; key rides only when true so plain opens stay byte-identical); header Draft badge on open drafts + mark-ready/convert-to-draft toggle beside it (pullDraftVisibility, the pullCloseVisibility precedent); list draft chips; draft_changed timeline text; the previously dead MergeBox draft arm is live. SDK pulls.open takes draft (undefined omits it), update passes through.
Docs: features/03 (new section 3.1 + route rows + SSE actions + Decisions entry) and go/12_web_ui (law 12, same commit).

Verification: go test ./internal/pulls/... -race green (incl. new draft613_test.go: open default/draft, flip both ways + events/fan-out + roles, merged-refused, merge-refused, mark-ready-then-merge, HTTP wire); make cover gate holds (pulls 96.1%, all packages >=95%); web unit full-minus-smoke 1590/1590 (new draft-613.test.js, 11 tests); vite build + esbuild SDK bundle green; gofmt/vet clean. Notes: one rare TestGetPRHeadDrift flake seen once (stamp-without-stream assertion; passes solo and 10/11 suite runs; untouched code path, per-service/per-env isolation); web/dist was unbuilt in the worktree (rebuilt; .keep restored).

Implements the binding option-a decision: draft PRs creatable/flippable via API+UI with tests. API: POST ../pulls accepts draft (omitted = ready); PUT ../pulls/{num} {draft: bool} flips ready<>draft for author-or-triage (same roles as state transitions), 409 on merged. Flips append draft_changed thread events (from/to draft/ready labels, mirroring state_changed) with ready_for_review/converted_to_draft notify + stream fan-out; no-op flips write nothing. Merge task refuses drafts alongside the other step-1 gates (narrated 409, no publish). UI: PullNew open-as-draft checkbox (ReleaseNew idiom; key rides only when true so plain opens stay byte-identical); header Draft badge on open drafts + mark-ready/convert-to-draft toggle beside it (pullDraftVisibility, the pullCloseVisibility precedent); list draft chips; draft_changed timeline text; the previously dead MergeBox draft arm is live. SDK pulls.open takes draft (undefined omits it), update passes through. Docs: features/03 (new section 3.1 + route rows + SSE actions + Decisions entry) and go/12_web_ui (law 12, same commit). Verification: go test ./internal/pulls/... -race green (incl. new draft613_test.go: open default/draft, flip both ways + events/fan-out + roles, merged-refused, merge-refused, mark-ready-then-merge, HTTP wire); make cover gate holds (pulls 96.1%, all packages >=95%); web unit full-minus-smoke 1590/1590 (new draft-613.test.js, 11 tests); vite build + esbuild SDK bundle green; gofmt/vet clean. Notes: one rare TestGetPRHeadDrift flake seen once (stamp-without-stream assertion; passes solo and 10/11 suite runs; untouched code path, per-service/per-env isolation); web/dist was unbuilt in the worktree (rebuilt; .keep restored).

crueber added 1 commit

2026-09-15 23:38:58 +00:00

Implement draft PRs: open-as-draft / mark-ready transitions (Forgejo #613 , option a) ddf92c33c8

Per the binding user decision, draft PRs are creatable/flippable via
API+UI: POST ../pulls accepts draft (omitted = ready), PUT ../pulls/{num}
{draft: bool} flips for author-or-triage with draft_changed events and
ready_for_review/converted_to_draft fan-out, 409 on merged; the merge task
refuses drafts alongside the other step-1 gates. UI: PullNew open-as-draft
checkbox, header Draft badge + mark-ready/convert-to-draft toggle, live
MergeBox draft arm. Docs: features/03 (section 3.1 + routes + Decisions)
and go/12_web_ui (law 12, same change).

crueber commented

2026-09-15 23:44:30 +00:00

Author

Owner

Independent review (option-a draft PRs, ddf92c3): APPROVE.

Verified each acceptance point against the code, not just the description:

API semantics — pass. POST draft is a non-pointer bool (omitted=false, ready); PUT draft is *bool gated by canMod (author-or-triage, same rule as title/state) with 403 for third party and 401 anon (both asserted in TestDraft613FlipRoles). Merged-terminal refused 409 twice: pre-check on the first read AND post re-read after the fresh loadPR, so a merge landing between the reads still refuses. No-op flip sets draft=nil up front, so no pr.json write, no event, no notify/stream (code path confirmed; event-count assertion in test). CAS discipline matches the §2.3 owned-delta pattern: fresh loadPR before save, only body+draft touched, and reapplyPR preserves Draft cross-direction with the merge-outcome writer (merge branch keeps cur.Draft; merged-fresh branch keeps outcome). One benign residual noted below.
draft_changed event — pass. Shape mirrors state_changed (From/To draft/ready labels, actor/at/participants). P8 fall-through claim verified in internal/notify/emit.go: classReason default → ReasonSubscribed, classAction default → ActionCommented, so the new ready_for_review/converted_to_draft classes degrade to subscribed/activity on older maps. Stream side: web collabKeys switches on frame.kind (pull), not action, so new actions ride existing invalidation; pullEventText default returns ev.type (renders raw, never crashes). Old clients are safe.
Merge gate — pass. Draft refusal sits in runMerge before Dir/resolve/publish (loads only), narrated 409 'is a draft' naming the reason; TestDraft613MergeRefused asserts zero ref updates (no partial publish); mark-ready-then-merge goes TaskOK end to end.
UI — pass. PullNew checkbox follows the ReleaseNew bordered-label idiom (verified the precedent exists), checkbox carries no text-field class. pullDraftVisibility = author-or-triage, merged hides both, closed-but-unmerged still flips (orthogonal per spec); toggle sits inside Show-when-thread with shrink-0 wrap comment for 390px. Badge Draft on open drafts, Closed wins on closed, Merged wins. MergeBox draft arm (pre-existing dead code) now live; mergeabilityDisplay draft shapes tested with the real {draft:true} detail Pull.jsx passes.
SDK — pass. pulls.open threads draft through json() where JSON.stringify drops undefined, and buildOpenCall sets the key only when true (both layers pinned by tests), so plain opens stay byte-identical for old servers (which 400 unknown keys — sane direction, documented). PullRow typedef carries draft. update passes through verbatim.
TestGetPRHeadDrift flake — pre-existing, confirmed myself: reproduced on clean origin/main (3 failures / 30 runs, -race) vs 1/30 with the PR applied — same order, untouched code path (refreshHead/GetPR/ComputeMergeable background interleave; the stamp/stream assertions live in code this PR does not touch). Not a blocker; suggest a follow-up issue for the drift-refresh vs background-recompute race rather than holding this PR.
#521 pin, docs, coverage, deps — pass. The 521 badge pin is stricter (exact badge element, count pin above holds; full unit suite minus smoke green). Docs: features/03 §3.1 + route/SSE rows + Decisions entry and go/12 law-12 entry in the same commit (law 12). Coverage measured: pulls 96.1% (only internal/pulls Go touched, so other packages unaffected). New tests reference symbols absent on main (EventDraftChanged, pullDraftVisibility — grep on main returns nothing), i.e. they fail pre-fix. No go.mod/package.json changes, no new deps (law 1).

Test runs on the worktree: go test ./internal/pulls/ -race green; -cover 96.1%; gofmt/vet clean; node --test unit files minus smoke green (1590 pass; the single smoke failure 'built SPA shell /setup 403' reproduces identically on clean main — sandbox environmental, unrelated).

Two non-blocking observations: (a) a merge landing in the µs between UpdatePR's second merged-check and savePR would persist draft=true on a merged doc via reapplyPR plus a draft_changed event — functionally inert (Merged wins in every reader; merge gate checks Merged first) and the same residual race the state flips already carry (they do not even re-check); acceptable. (b) MergeBox mergeState draft arm is covered by source pins rather than a direct mergeState({pr:{draft:true}}) call — consistent with the #592 pattern used throughout; fine.

No fix commits needed — worktree clean at ddf92c3. Verdict: APPROVE.

Independent review (option-a draft PRs, ddf92c3): APPROVE. Verified each acceptance point against the code, not just the description: 1. API semantics — pass. POST draft is a non-pointer bool (omitted=false, ready); PUT draft is *bool gated by canMod (author-or-triage, same rule as title/state) with 403 for third party and 401 anon (both asserted in TestDraft613FlipRoles). Merged-terminal refused 409 twice: pre-check on the first read AND post re-read after the fresh loadPR, so a merge landing between the reads still refuses. No-op flip sets draft=nil up front, so no pr.json write, no event, no notify/stream (code path confirmed; event-count assertion in test). CAS discipline matches the §2.3 owned-delta pattern: fresh loadPR before save, only body+draft touched, and reapplyPR preserves Draft cross-direction with the merge-outcome writer (merge branch keeps cur.Draft; merged-fresh branch keeps outcome). One benign residual noted below. 2. draft_changed event — pass. Shape mirrors state_changed (From/To draft/ready labels, actor/at/participants). P8 fall-through claim verified in internal/notify/emit.go: classReason default → ReasonSubscribed, classAction default → ActionCommented, so the new ready_for_review/converted_to_draft classes degrade to subscribed/activity on older maps. Stream side: web collabKeys switches on frame.kind (pull), not action, so new actions ride existing invalidation; pullEventText default returns ev.type (renders raw, never crashes). Old clients are safe. 3. Merge gate — pass. Draft refusal sits in runMerge before Dir/resolve/publish (loads only), narrated 409 'is a draft' naming the reason; TestDraft613MergeRefused asserts zero ref updates (no partial publish); mark-ready-then-merge goes TaskOK end to end. 4. UI — pass. PullNew checkbox follows the ReleaseNew bordered-label idiom (verified the precedent exists), checkbox carries no text-field class. pullDraftVisibility = author-or-triage, merged hides both, closed-but-unmerged still flips (orthogonal per spec); toggle sits inside Show-when-thread with shrink-0 wrap comment for 390px. Badge Draft on open drafts, Closed wins on closed, Merged wins. MergeBox draft arm (pre-existing dead code) now live; mergeabilityDisplay draft shapes tested with the real {draft:true} detail Pull.jsx passes. 5. SDK — pass. pulls.open threads draft through json() where JSON.stringify drops undefined, and buildOpenCall sets the key only when true (both layers pinned by tests), so plain opens stay byte-identical for old servers (which 400 unknown keys — sane direction, documented). PullRow typedef carries draft. update passes through verbatim. 6. TestGetPRHeadDrift flake — pre-existing, confirmed myself: reproduced on clean origin/main (3 failures / 30 runs, -race) vs 1/30 with the PR applied — same order, untouched code path (refreshHead/GetPR/ComputeMergeable background interleave; the stamp/stream assertions live in code this PR does not touch). Not a blocker; suggest a follow-up issue for the drift-refresh vs background-recompute race rather than holding this PR. 7. #521 pin, docs, coverage, deps — pass. The 521 badge pin is stricter (exact badge element, count pin above holds; full unit suite minus smoke green). Docs: features/03 §3.1 + route/SSE rows + Decisions entry and go/12 law-12 entry in the same commit (law 12). Coverage measured: pulls 96.1% (only internal/pulls Go touched, so other packages unaffected). New tests reference symbols absent on main (EventDraftChanged, pullDraftVisibility — grep on main returns nothing), i.e. they fail pre-fix. No go.mod/package.json changes, no new deps (law 1). Test runs on the worktree: go test ./internal/pulls/ -race green; -cover 96.1%; gofmt/vet clean; node --test unit files minus smoke green (1590 pass; the single smoke failure 'built SPA shell /setup 403' reproduces identically on clean main — sandbox environmental, unrelated). Two non-blocking observations: (a) a merge landing in the µs between UpdatePR's second merged-check and savePR would persist draft=true on a merged doc via reapplyPR plus a draft_changed event — functionally inert (Merged wins in every reader; merge gate checks Merged first) and the same residual race the state flips already carry (they do not even re-check); acceptable. (b) MergeBox mergeState draft arm is covered by source pins rather than a direct mergeState({pr:{draft:true}}) call — consistent with the #592 pattern used throughout; fine. No fix commits needed — worktree clean at ddf92c3. Verdict: APPROVE.

crueber merged commit e80c169337 into main

2026-09-15 23:45:03 +00:00

crueber referenced this pull request from a commit

2026-09-15 23:45:04 +00:00

Merge pull request 'Draft PRs: open-as-draft / mark-ready transitions (fixes #613)' (#616) from fix/issue-613 into main

crueber referenced this pull request

2026-09-15 23:45:05 +00:00

Draft PRs are unreachable: Draft is never set, so the draft merge-blocker and Draft phrase are dead #613