autoland engine: per-repo settings + lander runs (auto-validate, auto-merge on clean PASS) #181
Labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Cloonar/coding-lab#181
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 autoland engine's first half: per-repo settings and the lander run kind, so a clean AFK PR lands with zero human touches.
autoland_enabled(default off — each repo is opted in deliberately),max_fix_attempts(default 2),auto_merge(default on),lander_provider(default: the repo's default provider).pr approvethenpr merge(closing the issue via the existing merge path). Ifauto_mergeis off, or the verdict is CONCERNS →pr approveonly and stop — the approving review is the forge-observable "validated, awaiting human confirm" state. On FAIL / blocking CONCERNS →pr rejectwith the findings as the review body.Acceptance criteria
auto_mergeoff (or a CONCERNS verdict): the PR ends with an approving review and no mergeBlocked by
Agent Brief
Category: enhancement
Summary: Autoland engine, first half — per-repo autoland settings and the lander run kind, so a clean AFK-authored PR is validated and merged with zero human touches.
Current behavior:
After #180, agents have the full PR review surface (
pr reject/rerequest/approve/comment, review-awarepr view) and a validation-core doc, but nothing spawns validation automatically: an AFK-authored PR sits until a human runs the interactive land-pr skill. The reaper/scheduler already walks PRs state-derived and poll-only (its AFK done-signal is "PR exists on branch"), and repos already carry per-repo AFK scheduler config surfaced in the repo settings UI.Desired behavior:
autoland_enabled(default off — deliberate opt-in per repo),max_fix_attempts(default 2),auto_merge(default on),lander_provider(default: the repo's default provider).pr approvethenpr merge(closing the linked issue via the existing merge path).auto_mergeoff, or verdict CONCERNS →pr approveonly and stop; the approving review is the forge-observable "validated, awaiting human confirm" state. FAIL / blocking CONCERNS →pr rejectwith the findings as the review body.Key interfaces:
afk/<N>convention) as the only spawn scopepr approve/pr reject/pr mergeverbs from #180Acceptance criteria:
auto_mergeoff (or a CONCERNS verdict): the PR ends with an approving review and no mergeOut of scope:
max_fix_attemptsbound enforcement, and escalation (#182) — this issue only stores the settingLand-PR audit — PR #184
Verdict: FAIL — three blockers. CI is green and the diff is high quality, but the blockers are all in the new autoland machinery and two of them are created by this PR.
Signal relied on:
ci / nativesuccess (7m16s) — fullgo build -tags ui,go test -tags ui ./...with real git/tmux/prlimit,golangci-lint 2.12.2, SPA lint/format/vitest/build.ci-nixcorrectly did not run (path-gated to**/*.nix/flake.lock/go.mod/go.sum; this diff touches none).origin/mainis an ancestor ofafk/181— no conflict. Migrations verified empirically: sqlite schema rebuilt from 0001..0012, 0013 applied and diffed — all 19 columns, both CHECKs, the FK cascade and both indexes preserved; sqlite/postgres 0012 statement-for-statement identical; no numbering collision.Blockers
1. Verdict-marker forgery — the autoland trust boundary is bypassable.
handlePRComment(internal/agentapi/handlers.go:1198) is the onlyopensWithVerdictMarkercall site.handleCommentCreate(:376,POST /agent/v1/issues/{n}/comments) has no gate and accepts any number in the repo. Both verbs resolve toissuePath(number)+"/comments"— andPullComments' own doc states "A PR shares the issue-comment number space on Forgejo, so this reads and maps the SAME endpoint."So any run token can
POST /agent/v1/issues/<PR#>/comments {"body":"[autoland] verdict: pass"}. Consequences:VerdictWordsparses it →VerdictPresent=true→ShouldSpawnLandernever fires again on that PR (silent, permanent denial of validation); and a live lander is false-success-reaped viaLanderDone. An AFK agent can exempt its own PR from the validator meant to check it.This directly falsifies the claim at
handlers.go:1168-1174("the ONE agent-writable path onto a PR that composes no marker of its own"). Onmainthe gap was inert — nothing read markers. This PR ships the consumer and makes it live.Fix: apply the gate in
handleCommentCreatetoo, or move it to theCreateComment/CommentPullseam so both verbs are covered structurally.2. A surviving worktree wedges autoland into a permanent, un-backed-off retry loop.
decideTeardownkeeps worktree and branch on a dirty tree (an untracked build artifact suffices), andStopAFKparks both for every non-manual kind. Either leavesafk/<N>checked out with the run row terminal. Next tick:ActiveRunOnBranchfalse, PR open, no marker → predicate passes →AddWorktreeExistinghard-errorsalready checked out at ...(internal/gitx/worktree.go:79-83) →autoland.go:158-160logs and continues. Repeat everyafk_tick_seconds(default 30s) forever: no backoff, and a launch error is not a run outcome soconsecutive_failuresnever rises and the repo never pauses. Cost is onegit fetch+ 2 forge reads per stuck PR per tick until a lab restart.Note the irony: that is the same unbounded-forge-load shape #176 exists to fix, re-introduced.
internal/afk/lander.go:102-107claims the opposite is achieved — but disjoint worktree paths don't help when both want the same branch ref.3. Lander outcomes contaminate the AFK three-strikes counter, in both directions.
reaper.go:122routes landers through the sameclassifyAndClaim→reapRun, whose failure switch has no kind guard:IncrementRepoFailures→ at 3, the repo's AFK auto scheduler pauses and manual starts 409. Lander flakiness silently suspends unrelated AFK work.rejectclassifiesOutcomeSuccess→ResetRepoFailures— so finding a PR unlandable re-arms the pause that was protecting the repo from the AFK run that produced it. Semantically backwards.reapRun's own doc still reads "The counter is kind-agnostic — manual and auto AFK runs both feed it", enumerating only the AFK kinds, which suggests the coupling was not deliberate. Landers likely need their own counter.Non-blocking, worth fixing
ResetRepoFailures.max_fix_attemptsbounds fix runs (#182), not landers.web/src/routes/RepoSettings.tsx): flipping binding away from forge disables the autoland checkbox while still checked (:802), andbuildPatchonly emitsautoland_enabledwhen it differs (:423) — so the patch omits it, the server 400s, and the user cannot uncheck it to recover. ForcesetAutolandEnabled(false)when blocked.reset --hard origin/<branch>(gitx/worktree.go:92) moves the branch ref, silently discarding committed-but-unpushed work on an adopted claim branch — whichRemoveWorktree's own comment acknowledges a success-path reap can carry.autoland.go:135-153):Reviews+PullCommentsfire beforeActiveRunOnBranch, so the steady state (lander working) burns 2 forge round-trips per tick per PR just to be vetoed by aCOUNT(*). Hoist the DB check.DonePullranks open above merged — so a same-head collision can make a lander miss its own merge and burn to timeout.migrations/sqlite/0013is not retry-safe: a crash afterCREATE TABLE runs_newwedges every subsequent boot. AddDROP TABLE IF EXISTS runs_new;to Up and Down.max_fix_attemptshas no upper bound (reposvc.go:473checks only>= 0).instance/launch.go("only a branch this launch created") is the inverse of whatAdoptBranchmeans, andstore/afk.go:67-70claimsidx_runs_outcomenarrowsActiveRunOnBranch— it's onoutcomealone, andactiveis the hot value.Verified correct
Fail-closed on all nine forge-read error paths; no duplicate-spawn race (the run row is written synchronously inside
LaunchLander); done-signal push correctly AFK-only;fix-donecorrectly excluded from lander done-words; unreadable comments skip rather than guess; startup re-adoption does protect a live lander's worktree (parseLanderLabel→afk/N), and thelander-7manual-label collision is unreachable viaUniqueManualLabel; the verdict-grammar move is genuinely behaviour-preserving (differential fuzz, ~460k execs, zero divergence); GitHub pagination keeps its last page;secretscandelegating reads is correct — it is a write-side reject guard, not a read redactor; the PATCH cross-field invariant is enforced server-side in both directions and cannot be bypassed by sending both fields; no omitted-field zeroing bug; defaults match the issue.Land-PR audit — PR #184, final: MERGED
Supersedes the FAIL verdict above. That verdict treated three latent defects in default-off machinery as merge-blocking; on review with the maintainer that bar was wrong.
autoland_enableddefaults FALSE and migration 0012 states plainly that "no repo gains a lander merely from this migration" — so none of the three could fire on any repo as shipped.Resolved before merge — commit
cf072bbThe adopt now checks out the PR head detached at
origin/<branch>(worktree add --detach) instead of holding the branch ref and hard-resetting it. 22 lines of branching collapse to 3, and it closes two of the three findings at once:afk/<N>, so every adopt failed while every spawn-predicate term stayed true — respawning each tick with no backoff and no failure accounting, burning a fetch plus two forge reads per stuck PR until restart. Nothing contends for the ref now.reset --hard origin/<branch>moved the branch ref; a detached checkout cannot. The seed prompt pushesHEAD:refs/heads/<branch>explicitly for the lander's one committing step."Validates exactly what the forge sees" is unchanged — that was the point of the reset, and
origin/<branch>is now checked out directly.Also corrected three comments that asserted things the code did not do, the important one being the agentapi verdict gate: it is not airtight, and the code now says so instead of claiming it is.
Accepted, not fixed
The verdict-marker forgery gap stays open, deliberately.
handleCommentCreate(POST /agent/v1/issues/{n}/comments) reaches the same comment threadPullCommentsreads, ungated. Maintainer's call: the exploit needs a deliberately adversarial agent rather than a mistaken one, and the accidental path is narrow (a body must begin with the marker). This is now documented inhandlers.goas a known accepted gap, with the better fix named (gate theCreateCommentseam so both verbs inherit it structurally) for whenever this stops being a trust-the-agent boundary.Deferred to #185
Lander outcomes contaminating the AFK three-strikes counter — a lander death/timeout pausing the repo's AFK loop, and a lander
rejectclassifying as success and resetting the strikes that broken AFK runs accumulated. Filed as part of #185 along with the spawn-loop unification, because the two are the same piece of work: #185 replaces the racing scheduler/reaper cap consumption with one priority-ordered loop (lander > fix > new AFK), which is also where the counter separation belongs. #182 should emit candidates into that list rather than adding a third spawner.Verification relied on
ci / nativesuccess oncf072bb(run 206, 6m38s) —go build -tags ui,go test -tags ui ./...with real git/tmux/prlimit,golangci-lint 2.12.2, SPA lint/format/vitest/build.ci-nixcorrectly skipped (path-gated; no nix/dep changes). Locally also: fullgo test ./...green exceptinternal/tmuxx, verified to fail identically (8) at the unmodified parent — environmental, no tmux rig in the sandbox. Merged as a merge commit; #181 auto-closed viaCloses #181; head branchafk/181deleted.