Cloonar/coding-lab
3
0
Fork 0
Code Issues 23 Pull requests 4 Projects Releases Packages 1 Wiki Activity Actions

feat(autoland): agent PR review verbs, ADR-0048, validation core, labctl-native land-pr #183

Merged
dominik.polakovics merged 3 commits from afk/180 into main 2026-07-20 13:13:41 +02:00
Conversation 0 Commits 3 Files changed 27 +2475 -100
dominik.polakovics commented 2026-07-20 03:54:36 +02:00
Owner
Copy link

Closes #180

The pre-engine foundations for the autoland (reject → fix-forward) pipeline — the deferral in ADR-0024's status line, now shipped.

Note on this PR's history. The first commit implemented the verdict channel as native forge reviews. That design is dead: the deployed Forgejo 15.0.5 unconditionally 422s an approve/reject review from the PR poster, and every lab PR is authored by the single server-side forge-token account, so a native verdict can never succeed same-identity. The rework moved the channel to anchored marker comments. The description below reflects what actually ships; the review-verb surface is unchanged, only its implementation.

What's in here

1. PR verdict verbs on the run-token boundary (agent API → labctl, the exact pr merge wiring):

  • labctl pr reject <n> <body> — records a rejection verdict, findings as the body
  • labctl pr rerequest <n> — posts the fix-done verdict, then best-effort natively re-requests review from every reviewer whose latest verdict-bearing review requests changes (a later comment-only review no longer clears that verdict); the future fix run's done-signal
  • labctl pr approve <n> [body] — records a validation-passed verdict; "validated, awaiting confirm" becomes forge-observable
  • labctl pr comment <n> <body> — plain PR discussion comment
  • labctl pr view <n> — now prints submitted reviews (--- review by <reviewer> (<state>) + body) after the PR body

How the verdicts are carried. Not native reviews (dead by forge design, above) but anchored keyword PR comments: the agent API composes a first line [autoland] verdict: reject|pass|fix-done with the sanitized body below, exactly as pr create injects Closes #N. Agents speak verbs only and never see the grammar. The marker is a trust anchor that #181's poller will read first-line-only, so pr comment — the one PR-write path composing no marker of its own — rejects a body opening with [autoland] verdict: (400); without that gate any run token, which is repo-scoped and not confined to its own PR, could forge a verdict. Both that guard and the reject verb's non-empty check run on the sanitized body, because stripping an attribution line can promote a line-2 marker to line 1.

Seam. Review DTO, Reviews() (read), RerequestReview (the native ping), and CommentPull on all three backends; builtin defers the writes via tracker.ErrUnsupported → 409. There is deliberately no RejectPull/ApprovePull — native review writes do not exist at the seam. tracker.ErrReviewRejected remains the typed refusal for the re-request ping, surfacing the forge's own words verbatim as a 409 (mirroring ErrMergeRejected); only that refusal degrades to a warning, while ErrNotFound/ErrRateLimited keep their status. The secretscan leak guard scans every content-bearing write at the seam — all three verdict verbs funnel through the single scanned CommentPull — and every body passes the incogni sanitizer like any agent-authored body. Tests at all three layers (backend httptest, agentapi handlers, labctl end-to-end through the real agentapi).

2. Domain docs: ADR-0048 records the autoland design — state-derived poll-only loop (ADR-0015 stands), lander + fix run kinds with explicit forge-observable done-signals, attempt bound = fix-run spawns per PR (default 2), agent-executed terminal escalation (the engine never writes to a forge), per-repo settings default-off, plus the comment-marker grammar and its guard, the hybrid rejected-state (lander markers ∪ human native changes-requested reviews), and the second-bot-account option considered and rejected. CONTEXT.md names autoland, fix-forward, and escalation in the AFK-engine vocabulary, distinguishing fix-forward (bounded re-engagement carrying the rejection findings) from the forbidden auto-requeue — every auto-requeue mention stays in negative/Avoid framing, so the avoid-list grep guard holds.

3. Validation core + labctl-native land-pr: assets/skills/land-pr/validation-core.md is the single, mode-neutral definition of "landable" (checks aggregate per ADR-0032, conventions, conflict policy, PASS/CONCERNS/FAIL rubric) — seeded next to SKILL.md into every worktree so both the interactive skill and the future lander seed template read one definition. The land-pr skill is rewritten onto labctl (including the reversal: PASS now posts pr approve before the human merge gate), and docs/agents/issue-tracker.md + docs/agents/triage-labels.md are de-tea'd — no tea reference remains outside the frozen docs/reference/lab-v0/ snapshot, which is history and stays as-is.

Verification

  • CI ci / native green on the final head. The path-gated ci-nix hermetic gate correctly does not run: this diff touches no .nix, go.mod, or go.sum.
  • Full go test ./... green (the only failures are the eight pre-existing internal/tmuxx live-tmux integration tests, which fail in this sandbox on unchanged code with zero dependency on any touched package); go vet ./... clean, gofmt clean, go build ./... OK.
  • golangci-lint is unavailable in the sandbox — CI runs the real gate.
  • The marker guard is mutation-tested: with the guard removed, every new assertion fails, including the attribution-promotion bypass.

Review history

Landed after three review rounds (/land-pr, 2026-07-20):

  1. Blocking — the reviewer-verdict fold folded every non-dismissed state, so a later comment-only review cleared a reviewer's changes-requested verdict and pr rerequest reported a convergent no-op success on a still-blocked PR, stalling the fix loop permanently. Fixed in both forgejo and github; the slipped cases are pinned by tests.
  2. Design — native review writes are dead same-identity (the 422 above). Reworked to marker comments; RejectPull/ApprovePull and both submitReview helpers deleted from the seam, all three backends, and both decorators.
  3. Blocking — the rework made the marker a trust anchor while shipping pr comment with no guard on it, and asserted the opposite in both the code comment and ADR-0048 ("so no validation is needed"). Guard added, both claims corrected, plus two lows (reject's emptiness check and the re-request error downgrade, both above).

Out of scope (per the issue)

Poller rule, settings, lander run kind (#181); fix runs, attempt bound enforcement, escalation (#182); pr close; webhooks.

Open question for #181, recorded here deliberately: the guard makes the channel safe but does not make the marker authoritative — lab's own server writes every verdict and then plans to learn it back by parsing text out of a channel agents can write to. The sturdier shape is for the server to record the verdict in its own store and let the comment be the human-visible rendering. Cheaper to decide before the poller ships against the text grammar.

🤖 Generated with Claude Code

Closes #180 The pre-engine foundations for the autoland (reject → fix-forward) pipeline — the deferral in ADR-0024's status line, now shipped. > **Note on this PR's history.** The first commit implemented the verdict channel as *native forge reviews*. That design is dead: the deployed Forgejo 15.0.5 unconditionally 422s an approve/reject review from the PR poster, and every lab PR is authored by the single server-side forge-token account, so a native verdict can never succeed same-identity. The rework moved the channel to anchored marker comments. The description below reflects **what actually ships**; the review-verb *surface* is unchanged, only its implementation. ## What's in here **1. PR verdict verbs on the run-token boundary** (agent API → labctl, the exact `pr merge` wiring): - `labctl pr reject <n> <body>` — records a rejection verdict, findings as the body - `labctl pr rerequest <n>` — posts the fix-done verdict, then best-effort natively re-requests review from every reviewer whose latest **verdict-bearing** review requests changes (a later comment-only review no longer clears that verdict); the future fix run's done-signal - `labctl pr approve <n> [body]` — records a validation-passed verdict; "validated, awaiting confirm" becomes forge-observable - `labctl pr comment <n> <body>` — plain PR discussion comment - `labctl pr view <n>` — now prints submitted reviews (`--- review by <reviewer> (<state>)` + body) after the PR body **How the verdicts are carried.** Not native reviews (dead by forge design, above) but **anchored keyword PR comments**: the agent API composes a first line `[autoland] verdict: reject|pass|fix-done` with the sanitized body below, exactly as `pr create` injects `Closes #N`. Agents speak verbs only and never see the grammar. The marker is a trust anchor that #181's poller will read first-line-only, so `pr comment` — the one PR-write path composing no marker of its own — **rejects** a body opening with `[autoland] verdict:` (400); without that gate any run token, which is repo-scoped and not confined to its own PR, could forge a verdict. Both that guard and the reject verb's non-empty check run on the *sanitized* body, because stripping an attribution line can promote a line-2 marker to line 1. **Seam.** `Review` DTO, `Reviews()` (read), `RerequestReview` (the native ping), and `CommentPull` on all three backends; builtin defers the writes via `tracker.ErrUnsupported` → 409. There is deliberately **no** `RejectPull`/`ApprovePull` — native review writes do not exist at the seam. `tracker.ErrReviewRejected` remains the typed refusal for the re-request ping, surfacing the forge's own words verbatim as a 409 (mirroring `ErrMergeRejected`); only that refusal degrades to a warning, while `ErrNotFound`/`ErrRateLimited` keep their status. The secretscan leak guard scans every content-bearing write at the seam — all three verdict verbs funnel through the single scanned `CommentPull` — and every body passes the incogni sanitizer like any agent-authored body. Tests at all three layers (backend httptest, agentapi handlers, labctl end-to-end through the real agentapi). **2. Domain docs:** ADR-0048 records the autoland design — state-derived poll-only loop (ADR-0015 stands), lander + fix run kinds with explicit forge-observable done-signals, attempt bound = fix-run spawns per PR (default 2), agent-executed terminal escalation (the engine never writes to a forge), per-repo settings default-off, plus the comment-marker grammar and its guard, the hybrid rejected-state (lander markers ∪ human native changes-requested reviews), and the second-bot-account option considered and rejected. CONTEXT.md names **autoland**, **fix-forward**, and **escalation** in the AFK-engine vocabulary, distinguishing fix-forward (bounded re-engagement carrying the rejection findings) from the forbidden auto-requeue — every auto-requeue mention stays in negative/Avoid framing, so the avoid-list grep guard holds. **3. Validation core + labctl-native land-pr:** `assets/skills/land-pr/validation-core.md` is the single, mode-neutral definition of "landable" (checks aggregate per ADR-0032, conventions, conflict policy, PASS/CONCERNS/FAIL rubric) — seeded next to SKILL.md into every worktree so both the interactive skill and the future lander seed template read one definition. The land-pr skill is rewritten onto labctl (including the reversal: PASS now posts `pr approve` before the human merge gate), and `docs/agents/issue-tracker.md` + `docs/agents/triage-labels.md` are de-tea'd — no `tea` reference remains outside the frozen `docs/reference/lab-v0/` snapshot, which is history and stays as-is. ## Verification - CI `ci / native` green on the final head. The path-gated `ci-nix` hermetic gate correctly does not run: this diff touches no `.nix`, `go.mod`, or `go.sum`. - Full `go test ./...` green (the only failures are the eight pre-existing `internal/tmuxx` live-tmux integration tests, which fail in this sandbox on unchanged code with zero dependency on any touched package); `go vet ./...` clean, `gofmt` clean, `go build ./...` OK. - `golangci-lint` is unavailable in the sandbox — CI runs the real gate. - The marker guard is mutation-tested: with the guard removed, every new assertion fails, including the attribution-promotion bypass. ## Review history Landed after three review rounds (`/land-pr`, 2026-07-20): 1. **Blocking** — the reviewer-verdict fold folded *every* non-dismissed state, so a later comment-only review cleared a reviewer's changes-requested verdict and `pr rerequest` reported a convergent no-op success on a still-blocked PR, stalling the fix loop permanently. Fixed in both forgejo and github; the slipped cases are pinned by tests. 2. **Design** — native review writes are dead same-identity (the 422 above). Reworked to marker comments; `RejectPull`/`ApprovePull` and both `submitReview` helpers deleted from the seam, all three backends, and both decorators. 3. **Blocking** — the rework made the marker a trust anchor while shipping `pr comment` with no guard on it, and asserted the opposite in both the code comment and ADR-0048 ("so no validation is needed"). Guard added, both claims corrected, plus two lows (reject's emptiness check and the re-request error downgrade, both above). ## Out of scope (per the issue) Poller rule, settings, lander run kind (#181); fix runs, attempt bound enforcement, escalation (#182); `pr close`; webhooks. **Open question for #181, recorded here deliberately:** the guard makes the channel safe but does not make the marker *authoritative* — lab's own server writes every verdict and then plans to learn it back by parsing text out of a channel agents can write to. The sturdier shape is for the server to record the verdict in its own store and let the comment be the human-visible rendering. Cheaper to decide before the poller ships against the text grammar. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
feat(autoland): agent PR review verbs, ADR-0048, validation core, labctl-native land-pr
All checks were successful
ci / native (pull_request) Successful in 8m25s
Details
040f5078ff 
The pre-engine foundations for the autoland (reject → fix-forward)
pipeline, closing the deferral in ADR-0024's status line:

- Tracker seam: Review DTO + Reviews/RejectPull/ApprovePull/
  RerequestReview/CommentPull on all three backends (Forgejo, GitHub,
  builtin-deferred via ErrUnsupported), forge refusals verbatim as
  ErrReviewRejected, metrics decorator ops, secretscan on every
  content-bearing review write.
- Agent API: POST /prs/{n}/reject|approve|rerequest|comments and
  reviews[] on GET /prs/{n}, run-token-scoped, mirroring the pr merge
  wiring; ErrReviewRejected/ErrUnsupported → 409.
- labctl: pr reject/approve/rerequest/comment verbs and reviews in
  pr view, wired through the same client/exit-code contract.
- ADR-0048 records the autoland design: state-derived poll-only loop,
  lander + fix run kinds with forge-observable done-signals,
  fix-run-spawn attempt bound, agent-executed terminal escalation,
  per-repo default-off settings. CONTEXT.md names autoland,
  fix-forward, and escalation, distinguishing fix-forward from the
  forbidden auto-requeue.
- assets/skills/land-pr/validation-core.md is the single seeded
  definition of "landable" (checks, conventions, conflict policy,
  PASS/CONCERNS/FAIL); the land-pr skill and docs/agents tracker docs
  are rewritten onto labctl — tea is fully superseded.

Closes #180

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per the FINAL review brief on #180 (grill, 2026-07-20): the deployed
Forgejo 15.0.5 unconditionally 422s approve/reject reviews from the PR
poster and every lab PR is authored by the single server-side
forge-token account, so native review verdicts can never succeed
same-identity. The lander's verdict channel moves to anchored keyword PR
comments; the five labctl verbs keep their shape and the marker stays a
server-side implementation detail.

- Agent API: reject/approve/rerequest become handler-level compositions
  over the seam's CommentPull (precedent: pr create's Closes-#N
  injection) — first line `[autoland] verdict: reject|pass|fix-done`,
  sanitized body below; rerequest posts the fix-done signal FIRST, then
  best-effort native re-requests the changes-requested human reviewers
  (ping failure = {number, warning} on 200, labctl warns and exits 0).
- Tracker seam: RejectPull/ApprovePull and the forgejo/github
  submitReview helper stripped from the interface, all three backends,
  and both decorators; Review DTO, Reviews(), RerequestReview,
  CommentPull, and ErrReviewRejected stay.
- Blocking fold fix (both forgejo and github):
  changesRequestedReviewers now folds latest[reviewer] only for
  verdict-bearing states (approved, changes_requested) — a later
  COMMENT/COMMENTED (or Forgejo REQUEST_REVIEW) row no longer clears a
  reviewer's changes-requested verdict, which made pr rerequest report a
  convergent no-op success on a still-blocked PR and would have stalled
  the fix loop permanently. Repeat rerequest now re-POSTs the same
  reviewers — faithful, idempotent at the forge. Tests pin the slipped
  cases in both backends.
- ADR-0048 rewritten in place: comment-marker grammar (escalate reserved
  for #182), hybrid rejected-state (lander markers ∪ human native
  changes-requested reviews), fix-done clears lander rejections only,
  clean PASS + auto_merge merges with no comment, builtin
  feasible-but-deferred behind builtin CommentPull, second bot account
  considered and rejected.
- land-pr skill/validation-core: PASS → labctl pr approve (CONCERNS as
  body) before the human confirm gate; FAIL → report in session, no
  forge write. Verbs only — the grammar's canonical home is ADR-0048.

Closes #180

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fix(autoland): guard the verdict grammar on the plain PR-comment verb
All checks were successful
ci / native (pull_request) Successful in 7m47s
Details
b5abaa5df0 
The marker rework made `[autoland] verdict: <word>` a trust anchor that
#181's poller will read first-line-only, but shipped `pr comment` — the
one PR-write path that composes no marker of its own — with no guard on
it. A run token could post a body whose first line IS a marker and forge
a verdict no lander ever reached; run tokens are repo-scoped, so the
forgery was not even confined to the run's own PR. A forged `pass` would
present an unvalidated PR to the operator as "validated, awaiting
confirm"; a forged `fix-done` would clear a real rejection.

- handlePRComment rejects a body opening with `[autoland] verdict:`
  (400). The check runs on the SANITIZED body: stripAttribution deletes
  leading lines, so a marker on line 2 under an attribution line is
  promoted to line 1 — a raw-body check would have passed it through.
  Trimming is deliberately stricter than the pinned exact-prefix parse
  rule, so a future consumer that trims before matching cannot turn an
  already-accepted body into a verdict retroactively.
- handlePRReject judges emptiness post-sanitize too, for the same
  ordering reason: an all-attribution body sanitized to nothing posted a
  bare reject marker with no findings, defeating the gate whose whole
  point is that the findings are the reject.
- handlePRRerequest degrades only ErrReviewRejected to a warning.
  ErrNotFound and ErrRateLimited are real errors and keep their status —
  reporting 200 for a ping that never reached the forge hid a genuine
  failure behind a done-signal that had merely been posted.
- The false safety claim is corrected in both places that made it:
  the marker doc comment and ADR-0048's grammar pin, which concluded
  "so no validation is needed".

Tests pin all four marker shapes, the attribution-promotion bypass, and
that a marker quoted mid-body stays legal; each fails without the guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug
Something is broken

  
chore

   
enhancement
New feature or improvement

  
needs-info
Waiting on reporter for more information

  
needs-triage
Maintainer needs to evaluate this issue

  
ready-for-agent
Fully specified, ready for an AFK agent

  
ready-for-human
Requires human implementation

  
wontfix
Will not be actioned

No labels
bug
chore
enhancement
needs-info
needs-triage
ready-for-agent
ready-for-human
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Cloonar/coding-lab!183
No description provided.