autoland foundations: labctl PR review surface, ADR + CONTEXT.md, labctl-native land-pr #180
Labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Cloonar/coding-lab#180
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What to build
The pre-engine foundations for the autoland (reject → fix-forward) pipeline: the agent-side PR review surface, the domain documentation, and a labctl-native landing playbook.
1. Tracker seam + labctl PR review verbs (completes the list ADR-0024 explicitly deferred):
labctl pr reject <n> <body>— post a Forgejo native changes-requested review with the findings as bodylabctl pr rerequest <n>— re-request review (clears changes-requested); this becomes the fix run's done-signal laterlabctl pr approve <n> [body]— post an approving review, making "validated, awaiting merge" forge-observablelabctl pr comment <n> <body>— plain PR discussion commentlabctl pr view <n>— additionally show reviews (state + body) so agents can read rejection findingsAll on the ADR-0014 run-token boundary: Tracker seam methods + httpapi + labctl, mirroring how
pr merge(ADR-0024) is wired.2. Domain docs:
3. Validation core + land-pr skill:
assets/skills/land-pronto labctl (teais fully superseded); purge remainingteareferences fromdocs/agents/issue-tracker.mdand the skill.Acceptance criteria
pr reject,pr rerequest,pr approve,pr comment, and see review state/bodies inpr view, covered by tests at the tracker seam + httpapiteareferences; the interactive skill works end-to-end over labctlBlocked by
None - can start immediately
Agent Brief
Category: enhancement
Summary: Autoland foundations — labctl PR review verbs on the run-token boundary, the autoland ADR + CONTEXT.md concepts, and a labctl-native land-pr skill backed by a single validation-core doc.
Current behavior:
Run tokens can
pr create,pr view,pr list, andpr merge— the merge verb is wired Tracker seam → httpapi run-token route → labctl (ADR-0024). There are no review verbs: an agent cannot post a changes-requested or approving review, re-request review, or comment on a PR, andpr viewdoes not surface review state or bodies. ADR-0024 explicitly deferred "the reject → re-queue loop andpr close/pr comment". The land-pr skill and the issue-tracker agent doc still documenttea, which is fully superseded by labctl, and the definition of "landable" (checks, conventions, conflict policy, verdict rubric) lives only inside the interactive skill text.Desired behavior:
pr mergewiring (Tracker seam method → httpapi → labctl):labctl pr reject <n> <body>— Forgejo native changes-requested review, findings as bodylabctl pr rerequest <n>— re-request review, clearing changes-requested (the future fix run's done-signal)labctl pr approve <n> [body]— approving review ("validated, awaiting merge" becomes forge-observable)labctl pr comment <n> <body>— plain PR discussion commentlabctl pr view <n>— additionally shows reviews (state + body) so agents can read rejection findingsteareferences are purged from it and from the issue-tracker agent doc.Key interfaces:
tracker.Trackerseam — new review methods besideMergePull/CreatePull/Pull/Pulls; the pull view type carries review state + bodiespr mergepatternAcceptance criteria:
pr reject,pr rerequest,pr approve,pr comment, and see review state/bodies inpr view, covered by tests at the tracker seam + httpapiteareferences; the interactive skill works end-to-end over labctlOut of scope:
pr close(still deferred), webhooks of any kind, changes to git push authReview brief for PR #183 (branch
afk/180) — two changes before mergeVerdict from the 2026-07-20 review: one blocking code defect, one verified design gap to record. Amend the existing PR branch; everything else in the PR stands as-is — in particular
handlePRGet's fail-the-whole-view on a reviews error andsubmitReview's any-non-404 →ErrReviewRejectedmapping are documented-deliberate, leave them.Change 1 — BLOCKING:
changesRequestedReviewersmust fold only verdict-bearing statesFiles:
internal/tracker/forgejo/forgejo.go(changesRequestedReviewers, ~line 826) andinternal/tracker/github/github.go(~line 855). The two copies are deliberately parallel — fix both identically.Defect: the fold skips only
Dismissedand"pending", then does last-wins on every other state. Acommented(or, Forgejo-only,review_requested) row arriving after achanges_requestedrow overwrites that reviewer's latest verdict and silently drops them from the set. Neither forge works that way: Forgejo'spreparePullReviewTyperecognizes only APPROVED/REQUEST_CHANGES as verdict events (verified in the deployed v15.0.5 source), and GitHub's review decision is unaffected by COMMENTED reviews.Failure it causes: reviewer requests changes, later replies in the thread (forge records a COMMENT review) →
labctl pr rerequest <n>computes an empty set → returns the convergent no-op200while the forge still shows changes requested. The fix run's done-signal (ADR-0048) reports success on a PR that is still blocked, and the loop stalls permanently.Fix: update
latest[r.Reviewer]only whenr.Stateis a verdict —tracker.ReviewApprovedortracker.ReviewChangesRequested. Keep theDismissedskip. Non-verdict states (commented,review_requested,pending, anything unknown) must neither set nor clear a reviewer's verdict; the explicit"pending"skip is then subsumed. Accepted consequence, note it in the doc comment: after a successful rerequest the reviewer's latest verdict is still changes-requested, so a repeatedpr rerequestre-POSTs the same reviewers instead of no-opping — that is faithful (they are still changes-requested until they re-review) and the forge handles a duplicate request idempotently. Update the doc comments on both copies and on bothRerequestReviewmethods to match the new rule.Tests (both backends): add the case that slipped — same reviewer REQUEST_CHANGES then COMMENT/COMMENTED → reviewer still in the POSTed set. Forgejo additionally: REQUEST_CHANGES then REQUEST_REVIEW → still in the set. Existing pins stay green: approve-after-reject drops out, dismissed skipped, pending-after-reject retained.
Change 2 — amend ADR-0048: lander verdicts require a second forge identity (prerequisite for #181)
No behavior change — the seam and its error mapping are correct. Record the verified deployment constraint in
docs/adr/0048-autoland-fix-forward.md(a new pin bullet plus a status-line note is enough):preparePullReviewType, no configuration escape hatch — and every lab PR is authored by the single server-side forge-token account. Under one identity the lander'spr approve/pr rejectare inert on 100% of lab PRs.pr create,pr merge(least-privilege — the reviewer account stays review-only), and the fix run'spr rerequest(the poster may re-request any reviewer with a prior review; verified againstIsValidReviewRequest).pr commentand human reviews are unaffected.Done means
go test ./...green (the eight live-tmuxinternal/tmuxxfailures are pre-existing sandbox noise),gofmt/go vetclean, both new fold tests in place, ADR-0048 amended, PR #183 updated in place onafk/180.REVISED review brief for PR #183 — supersedes the earlier 2026-07-20 brief above
Design change decided after review (grill, 2026-07-20): the lander's verdict channel moves from native forge reviews to anchored keyword PR comments — eliminating the second forge identity entirely. Background fact (verified in the deployed Forgejo 15.0.5 source and against the live forge): the forge unconditionally 422s approve/reject from the PR poster, and every lab PR is authored by the single server-side forge-token account, so native verdict verbs can never succeed same-identity. Comments on one's own PR are allowed, and merge has no self-restriction — so the comment channel works under one token, on any binding.
ADR-0048 ships inside this PR, so rewrite the affected pins in place — this is pre-merge editing, not an amendment.
Decisions to encode
[autoland] verdict: <word>with<word>∈reject|pass|fix-done. Parse rule: first line only, exact prefix match — a keyword quoted mid-body never triggers. Findings / CONCERNS prose follows below the marker line. Attempt counts stay derived from the runs store; any "(attempt 1/2)" suffix text is informational and never parsed. (Recommend #182's escalation digest use the same grammar,[autoland] verdict: escalate— note it in the ADR, implement there.)rejectcomments ∪ native changes-requested reviews (humans, forge bindings — the forge Review button works cross-identity and stays the human UX).Reviews()/pr view's review listing is the read for the native half; PR comments for the marker half.[autoland] verdict: fix-donecomment, uniform on every binding. When human changes-requested reviewers exist, the fix run ALSO callspr rerequest(best-effort forge ping to the human). Ownership rule, pinned in the ADR:fix-doneclears a lander rejection (a fresh lander run re-validates); a human rejection is cleared only by that human's new review — the loop never merges over an outstanding human changes-requested.auto_mergeon + clean PASS → merge, no comment (the merge is the observable state).auto_mergeoff, or PASS-with-CONCERNS →[autoland] verdict: passcomment (CONCERNS below the marker) = "validated, awaiting operator confirm"; the operator merges. Interactive land-pr skill: replace the approve-on-PASS step (which 409s on every lab PR today) with the samepasscomment before the human confirm gate; FAIL stays report-in-session, no marker — an operator who wants autoland to pick a rejection up posts their own changes-requested review in the forge UI. Document the marker grammar once (validation-core.md is the natural home) and reference it from the skill.pr rejectandpr approvefrom this PR: the labctl verbs, agentapi handlers/routes, theRejectPull/ApprovePullseam methods on all three backends, the forgejo/githubsubmitReviewhelper, and all their tests. They are correct code, but the loop never calls them and they fail same-identity by forge design; re-add if a multi-user deployment appears. Keep: theReviewDTO,Reviews()+pr view's review section (hybrid read, decision 2),RerequestReview/pr rerequest(decision 3),CommentPull/pr comment(the verdict channel), andErrReviewRejected(rerequest refusals still map to it).CommentPullstaysErrUnsupportedin this PR. Rewrite the ADR's forge-only pin to: builtin joins once it grows a PR-comment write + comment listing — its own follow-up issue, not #181's scope.Carried over from the original brief — still blocking
The
changesRequestedReviewersfold fix stands unchanged (it feedspr rerequest, which decision 3 keeps in the loop for human rejections): ininternal/tracker/forgejo/forgejo.go(~826) andinternal/tracker/github/github.go(~855), foldlatest[r.Reviewer]only for verdict-bearing states (tracker.ReviewApproved,tracker.ReviewChangesRequested); keep theDismissedskip; non-verdict states (commented,review_requested,pending, unknown) neither set nor clear a verdict. Accepted consequence (note in doc comments): repeatrerequestre-POSTs the same reviewers instead of no-opping — faithful, and idempotent at the forge. Tests in both backends: same reviewer REQUEST_CHANGES then COMMENT/COMMENTED → still in the POSTed set; Forgejo additionally REQUEST_CHANGES then REQUEST_REVIEW → still in the set; existing pins stay green.Unchanged and deliberate — do not "fix":
handlePRGetfailing the whole view on a reviews-read error; rerequest's non-404 errors mapping toErrReviewRejected.Done means
Verbs stripped cleanly (
go build/go vet/gofmtclean, no orphaned routes or docs), fold fix + new tests in both backends, ADR-0048 pins rewritten per decisions 1–6, land-pr skill + validation-core updated (pass-comment step, marker grammar), fullgo test ./...green (the eight live-tmuxinternal/tmuxxfailures are pre-existing sandbox noise), PR #183 updated in place onafk/180. The PR body still advertises reject/approve — it can't be edited via the deployed labctl, so flag the stale body in your closing issue comment for the operator to update in the forge UI.FINAL review brief for PR #183 — supersedes BOTH earlier briefs in this thread
This brief is self-contained; ignore the two above except as history. Design finalized 2026-07-20 (grill + revision): verdicts ride anchored keyword PR comments, and the five labctl verbs all stay — the marker is a server-side implementation detail. Agents keep the semantic surface (
reject/approve/rerequest) and never learn the grammar.Why (verified): deployed Forgejo 15.0.5 unconditionally 422s approve/reject reviews from the PR poster (no config escape), and every lab PR is authored by the single server-side forge-token account — native review verdicts can never succeed same-identity. Comments on one's own PR are allowed and merge has no self-restriction, so the comment channel works under one token on any binding. A second bot account was considered and rejected.
Target verb behavior (labctl surface unchanged in shape)
pr reject <n> <body>→ posts a PR comment: first line[autoland] verdict: reject, blank line, then the sanitized body. Body required (findings are the point).pr approve <n> [body]→ posts[autoland] verdict: pass+ optional body (CONCERNS go here). This is "validated, awaiting confirm".pr rerequest <n>→ posts[autoland] verdict: fix-done(the fix run's done-signal), THEN best-effort native re-request of every reviewer whose latest verdict-bearing native review is changes-requested (the human forge ping). Comment first: it is the signal. A native-ping failure is non-fatal — warn, exit 0.pr comment <n> <body>→ plain comment, no marker (unchanged).pr view→ unchanged, including the reviews listing (the hybrid read of human native reviews).Implementation decisions
marker + "\n\n" + sanitized bodyand calls the seam'sCommentPull. A body that itself starts with the marker prefix lands below line 1 and is inert — no validation needed. Sanitizer and secretscan see the body exactly as any comment write.RejectPull/ApprovePull(+ the forgejo/githubsubmitReviewhelper) from the Tracker seam and all three backends — verdict verbs become generic handler-level compositions overCommentPull. On builtin they now fail only because builtinCommentPullisErrUnsupported; once builtin grows comments, the verdict verbs work there with zero further changes (record this in the ADR).RerequestReview(native re-request) stays as a seam method on forgejo/github; the handler orchestrates comment-then-ping.ErrReviewRejectedstays for its refusals.[autoland] verdict: <word>,<word>∈reject|pass|fix-done(escalatereserved for #182). Consumers (#181's poller) match first line only, exact prefix — a keyword quoted mid-body never triggers. Attempt counts derive from the runs store, never parsed from comments. Skills and agent docs speak in verbs only and never spell the marker.Loop semantics — rewrite ADR-0048 in place (it ships inside this PR; edit, don't amend)
rejectmarkers ∪ human native changes-requested reviews (the forge Review button works cross-identity and remains the human UX;Reviews()is the read for that half).fix-doneclears a lander rejection (a fresh lander run re-validates). A human rejection is cleared only by that human's new review — the loop never merges over an outstanding human changes-requested.auto_mergeon → merge directly, no comment (the merge is the observable state). Otherwise →pr approve(pass marker) and the operator confirms/merges.CommentPull+ PR-comment listing, its own follow-up issue, not #181's scope. The forge-only pin is rewritten to say so.Blocking bug fix — unchanged from the first brief
changesRequestedReviewersininternal/tracker/forgejo/forgejo.go(~826) andinternal/tracker/github/github.go(~855): foldlatest[r.Reviewer]only for verdict-bearing states (tracker.ReviewApproved,tracker.ReviewChangesRequested); keep theDismissedskip;commented/review_requested/pending/unknown neither set nor clear a verdict. Update both doc comments (accepted consequence: a repeatedrerequestre-POSTs the same reviewers instead of no-opping — faithful, idempotent at the forge). Tests in both backends: same reviewer REQUEST_CHANGES then COMMENT/COMMENTED → still in the POSTed set; Forgejo additionally REQUEST_CHANGES then REQUEST_REVIEW → still in the set; existing pins stay green. This feeds the rerequest human-ping now and #181's poller fold later.land-pr skill (assets/skills/land-pr/)
PASS →
labctl pr approve <n>(CONCERNS as the body when present) before the human confirm gate — the verb now posts the pass comment under the hood, so the skill text stays verb-level. FAIL → report in session, no forge write; an operator who wants autoland to pick a rejection up posts their own changes-requested review in the forge UI. Neither SKILL.md nor validation-core.md documents the marker grammar — verbs only.Do not "fix" (deliberate)
handlePRGetfailing the whole view on a reviews-read error;RerequestReviewmapping non-404 errors toErrReviewRejected; the secretscan/incogni coverage patterns.Done means
go build/go vet/gofmtclean; fullgo test ./...green (the eightinternal/tmuxxlive-tmux failures are pre-existing sandbox noise). Reject/approve reimplemented as comment compositions with agentapi-handler and labctl end-to-end tests asserting the marker first line; seam verdict methods stripped with no orphans; fold fix + new tests in both backends; ADR-0048 rewritten per above; skill updated. Push toafk/180so PR #183 updates in place — no new PR. The PR body still describes native-review verdicts; flag the needed body edits in your closing comment on issue #180 for the operator to apply in the forge UI.Reworked PR #183 per the FINAL review brief (commit
71c9587onafk/180, PR updated in place — no new PR).Done:
CommentPull; all five labctl verbs keep their shape.pr rerequestposts the fix-done signal first, then best-effort native re-requests changes-requested human reviewers (ping failure = stderr warning, exit 0).RejectPull/ApprovePull+ the forgejo/githubsubmitReviewhelper stripped from the seam, all three backends, and both decorators (metrics, secretscan) — no orphaned routes, docs, or tests.ReviewDTO,Reviews(),pr view's review listing,RerequestReview,CommentPull, andErrReviewRejectedkept.changesRequestedReviewersfold fixed identically in both backends: only verdict-bearing states (approved/changes-requested) update a reviewer's latest verdict; Dismissed skip kept. New tests in both backends pin REQUEST_CHANGES→COMMENT/COMMENTED (and Forgejo REQUEST_CHANGES→REQUEST_REVIEW) → reviewer still in the POSTed set; existing pins stay green.pr approvewith CONCERNS as body before the confirm gate; FAIL → report in session, no forge write; no marker grammar outside the ADR).go build/go vet/gofmtclean; fullgo test ./...green except the eight pre-existinginternal/tmuxxlive-tmux sandbox failures. Handler and labctl end-to-end tests assert the marker first line on reject/approve/rerequest.handlePRGetfails the whole view on a reviews-read error;RerequestReviewmaps non-404 errors toErrReviewRejected.Operator action — the PR #183 body is stale (labctl cannot edit PR bodies). Please update it in the forge UI:
[autoland] verdict:marker comments (grammar pinned in ADR-0048), with RerequestReview kept as the best-effort native human-reviewer ping.{number}(no review state); their failure modes are CommentPull's (404 unknown, 409 builtin-unsupported); rerequest answers{number, warning?}with ping failures non-fatal.pr approvebefore the human confirm gate.