autoland: per-repo lander model/effort; fix runs use normal AFK settings #189

New issue

Closed

opened 2026-07-20 18:46:53 +02:00 by dominik.polakovics · 1 comment

dominik.polakovics commented

2026-07-20 18:46:53 +02:00

Owner

What to build

Two independent corrections to how autoland runs pick their model, effort, and provider.

1. Lander model/effort are not selectable

#181 shipped lander_provider but no model or effort knob. LaunchLander (internal/afk/lander.go:231) and LaunchEscalate (lander.go:312) both call ResolveModelEffort(..., "", "") with empty requests, and because lander/escalate are not AFK kinds (isAFKKind, internal/instance/credential.go:90) the AFK override layers never apply — so a lander silently lands on the repo base column, then the global spawn_model_default / spawn_effort_default. There is no way to say "validate with a cheaper model" or "validate at max effort" independently of the base spawn defaults.

Add two nullable repo columns, lander_model and lander_effort, exactly mirroring lander_provider's semantics:

NULL = inherit the existing resolution (repo base → global base → provider default). Default OFF for every existing repo; the migration grants nobody a new model.
Non-NULL = a STRICT per-spawn request — an unknown id fails the launch, because the operator asked for it by name. This is what layerSpawnDefault's req != "" arm already does; it is only a matter of feeding it.
Applies to both the lander and the escalate-mode lander. They are one validation class and share landerChainProvider (lander.go:181); a separate escalate knob is explicitly out of scope.

Strictness cuts against the #181 provider precedent deliberately: a wrong lander_model should fail loudly at launch, not degrade to whatever the base chain says while the operator believes their setting is in effect.

Effort must validate against the resolved lander model's own effort list, not the provider union — the #156 rule already implemented inside ResolveModelEffort. Resolving model and effort in one call preserves that for free.

2. Fix runs bypass normal AFK settings

LaunchFix (internal/afk/fix.go:177-193) passes the persisted authoring run's model and effort as per-spawn requests, and fixProvider (fix.go:112) does the same for the provider via store.AuthoringRunForBranch. Because a non-empty request short-circuits layerSpawnDefault (credential.go:211), this deliberately bypasses the AFK override layers — the repo's AFKModelDefault / AFKEffortDefault and the global spawn_model_default_afk / spawn_effort_default_afk are never consulted for a fix run, even though RunKindFix is already an AFK kind.

That inheritance was an ADR-0048 decision and it is now reversed by operator directive: a fix run is a normal AFK run and must resolve model, effort, and provider through the normal AFK chain. Delete the inheritance rather than rewire it:

LaunchFix calls ResolveProvider(ctx, repo, RunKindFix, "") and ResolveModelEffort(ctx, prov, repo, RunKindFix, "", "").
fixProvider and its warn-fallback paths are removed. The paired warn-fallback on model/effort goes with them — with empty requests the skip-layer semantics already make a stale value fall through, so nothing needs to be caught.
store.AuthoringRunForBranch (internal/store/afk.go:140) loses its only caller. Remove it and its tests unless a caller exists that this issue has missed; do not leave it dangling.

Note the practical consequence and do not treat it as a bug: a branch authored by one provider can now be fixed by another, if that is what the repo's AFK chain resolves to. Fix runs adopt the existing branch detached and start fresh, so nothing provider-specific carries over from the authoring session.

Docs

ADR-0048 states the inheritance rule as a decision; amend it in place with the reversal and the reason (operator wants one AFK settings surface, not two). CONTEXT.md gets the two new settings if it names the autoland knobs. The #182 acceptance line "fix runs inherit the authoring run's provider" is superseded.

Acceptance criteria

lander_model and lander_effort exist as nullable repo columns with a migration (both dialects) that leaves every existing repo inheriting as before
Setting either one changes the model/effort of both lander and escalate runs; clearing it restores the inherited resolution
An unknown lander_model or lander_effort fails the lander launch loudly rather than silently falling back
An effort unsupported by the resolved lander model is rejected (the #156 per-model catalog rule holds)
Both settings are editable per repo in the web UI beside the existing autoland block, and round-trip through the repo PATCH endpoint
A fix run resolves model, effort, and provider through the normal AFK chain — repo AFK override, then global AFK override, then base — with the authoring run's values having no influence
fixProvider and AuthoringRunForBranch are gone, with no dangling callers
ADR-0048 records the reversal; no doc still claims fix runs inherit from the authoring run

Out of scope

A separate model/effort knob for escalate runs distinct from the lander
Making MaxEscalateAttempts (internal/afk/decide.go:345) per-repo — noted as a real asymmetry against max_fix_attempts, but a different issue
Changing ResolveSpawnOptions (credential.go:333), which still returns an empty bag for validation-class kinds
Any change to the base or AFK-override settings surface itself

## What to build Two independent corrections to how autoland runs pick their model, effort, and provider. ### 1. Lander model/effort are not selectable `#181` shipped `lander_provider` but no model or effort knob. `LaunchLander` (`internal/afk/lander.go:231`) and `LaunchEscalate` (`lander.go:312`) both call `ResolveModelEffort(..., "", "")` with empty requests, and because lander/escalate are **not** AFK kinds (`isAFKKind`, `internal/instance/credential.go:90`) the AFK override layers never apply — so a lander silently lands on the repo base column, then the global `spawn_model_default` / `spawn_effort_default`. There is no way to say "validate with a cheaper model" or "validate at max effort" independently of the base spawn defaults. Add two nullable repo columns, `lander_model` and `lander_effort`, exactly mirroring `lander_provider`'s semantics: - NULL = inherit the existing resolution (repo base → global base → provider default). Default OFF for every existing repo; the migration grants nobody a new model. - Non-NULL = a **STRICT per-spawn request** — an unknown id fails the launch, because the operator asked for it by name. This is what `layerSpawnDefault`'s `req != ""` arm already does; it is only a matter of feeding it. - Applies to **both** the lander and the escalate-mode lander. They are one validation class and share `landerChainProvider` (`lander.go:181`); a separate escalate knob is explicitly out of scope. Strictness cuts against the `#181` provider precedent deliberately: a wrong `lander_model` should fail loudly at launch, not degrade to whatever the base chain says while the operator believes their setting is in effect. Effort must validate against the **resolved lander model's own** effort list, not the provider union — the `#156` rule already implemented inside `ResolveModelEffort`. Resolving model and effort in one call preserves that for free. ### 2. Fix runs bypass normal AFK settings `LaunchFix` (`internal/afk/fix.go:177-193`) passes the persisted authoring run's model and effort as per-spawn requests, and `fixProvider` (`fix.go:112`) does the same for the provider via `store.AuthoringRunForBranch`. Because a non-empty request short-circuits `layerSpawnDefault` (`credential.go:211`), this **deliberately bypasses the AFK override layers** — the repo's `AFKModelDefault` / `AFKEffortDefault` and the global `spawn_model_default_afk` / `spawn_effort_default_afk` are never consulted for a fix run, even though `RunKindFix` is already an AFK kind. That inheritance was an ADR-0048 decision and it is now reversed by operator directive: **a fix run is a normal AFK run and must resolve model, effort, and provider through the normal AFK chain.** Delete the inheritance rather than rewire it: - `LaunchFix` calls `ResolveProvider(ctx, repo, RunKindFix, "")` and `ResolveModelEffort(ctx, prov, repo, RunKindFix, "", "")`. - `fixProvider` and its warn-fallback paths are removed. The paired warn-fallback on model/effort goes with them — with empty requests the skip-layer semantics already make a stale value fall through, so nothing needs to be caught. - `store.AuthoringRunForBranch` (`internal/store/afk.go:140`) loses its only caller. Remove it and its tests unless a caller exists that this issue has missed; do not leave it dangling. Note the practical consequence and do not treat it as a bug: a branch authored by one provider can now be fixed by another, if that is what the repo's AFK chain resolves to. Fix runs adopt the existing branch detached and start fresh, so nothing provider-specific carries over from the authoring session. ### Docs `ADR-0048` states the inheritance rule as a decision; amend it in place with the reversal and the reason (operator wants one AFK settings surface, not two). `CONTEXT.md` gets the two new settings if it names the autoland knobs. The `#182` acceptance line "fix runs inherit the authoring run's provider" is superseded. ## Acceptance criteria - [ ] `lander_model` and `lander_effort` exist as nullable repo columns with a migration (both dialects) that leaves every existing repo inheriting as before - [ ] Setting either one changes the model/effort of both lander and escalate runs; clearing it restores the inherited resolution - [ ] An unknown `lander_model` or `lander_effort` fails the lander launch loudly rather than silently falling back - [ ] An effort unsupported by the resolved lander model is rejected (the `#156` per-model catalog rule holds) - [ ] Both settings are editable per repo in the web UI beside the existing autoland block, and round-trip through the repo PATCH endpoint - [ ] A fix run resolves model, effort, and provider through the normal AFK chain — repo AFK override, then global AFK override, then base — with the authoring run's values having no influence - [ ] `fixProvider` and `AuthoringRunForBranch` are gone, with no dangling callers - [ ] ADR-0048 records the reversal; no doc still claims fix runs inherit from the authoring run ## Out of scope - A separate model/effort knob for escalate runs distinct from the lander - Making `MaxEscalateAttempts` (`internal/afk/decide.go:345`) per-repo — noted as a real asymmetry against `max_fix_attempts`, but a different issue - Changing `ResolveSpawnOptions` (`credential.go:333`), which still returns an empty bag for validation-class kinds - Any change to the base or AFK-override settings surface itself

dominik.polakovics added the

ready-for-agent

enhancement

labels

2026-07-20 18:46:53 +02:00

dominik.polakovics referenced this issue from a commit

2026-07-20 19:14:26 +02:00

feat(autoland): per-repo lander model/effort; fix runs resolve on the AFK chain

dominik.polakovics referenced this issue from a pull request that will close it,

2026-07-20 19:14:47 +02:00

feat(autoland): per-repo lander model/effort; fix runs resolve on the AFK chain #190

dominik.polakovics commented

2026-07-20 19:40:07 +02:00

Author

Owner

This was generated by AI while landing a PR.

Lander audit — PR #190

Verdict: PASS.

Verification signal relied on: Forgejo Actions ci / native — success (6m40s, run 213). Not re-run. Its reach covers the Go build/test suite (sqlite backend), golangci-lint, and the full web chain (tsc, vitest, eslint, prettier, vite build).

Two gaps in that reach, both stated rather than assumed:

The postgres dialect of migration 0016 is unexercised. internal/store/store_test.go skips the postgres arm of forEachBackend unless LAB_TEST_POSTGRES_DSN is set, and the store-postgres job in ci.yml is still the commented-out M2 stub. So migrations/postgres/0016_lander_model_effort.sql has never been executed. Low risk and consistent with how 0010–0015 all landed: the statement is a bare ALTER TABLE ... ADD COLUMN TEXT, byte-identical to the sqlite dialect, as the file itself notes.
ci-nix did not run — correctly. It is paths-gated to **/*.nix, flake.lock, go.mod, go.sum, none of which this diff touches.

Checked and clean:

Merges cleanly into main (no conflicts); one commit, b686586.
Title is Conventional Commits; body carries a working Closes #189; head branch afk/189 matches the AFK contract.
Migration 0016 does not collide — main tops out at 0015, both dialects, lockstep-numbered.
All eight acceptance criteria met. Notably AC7: fixProvider and store.AuthoringRunForBranch are gone with no dangling callers — a repo-wide grep finds the name only in ADR-0048's prose describing the reversal.
The reversal is genuinely by deletion, not rewiring: LaunchFix and the producer's auth gate both resolve empty-request through ResolveProvider/ResolveModelEffort under RunKindFix, and the fallback ladder is removed rather than rerouted.
The #182 inheritance pins were rewritten to pin the opposite invariant rather than deleted — TestSpawnOnce_rejectSpawnsFixRunOnAFKChain keeps a distinctive sonnet/high authoring row precisely to prove it has no influence, and the integration test keeps its seeded haiku/low for the same reason. That is the right shape for a reversal.
The lander effort picker sources the provider's union catalog, matching the existing base/AFK picker convention (RepoSettings.tsx) — not a new inconsistency.

One non-blocking concern, recorded for a follow-up rather than this PR:

The new strict knobs interact with #187's burn-on-spawn-intent rule. autolandLaunch (internal/afk/autoland.go:275) burns an attempt for fix/escalate on every outcome except ErrOverCap — launch-pad failures included, by design. A lander_model/lander_effort that does not resolve fails LaunchEscalate at the launch pad, so each retry burns an escalate attempt until the bound is spent and the PR goes permanently quiet. The realistic path there is not a typo but a later lander_provider swap: a value valid for provider A silently becomes invalid when the lander chain moves to provider B, whose catalog lacks it. The autoland provider pre-check gates on provider resolution only, so it does not catch this.

Mitigating, which is why this is a concern and not a blocker: the exhaustion is logged at Error level (spawn: escalation exhausted, autoland silent on pull), the same exposure already exists for lander_provider from #181, and the settings UI populates both selects from the live catalog of the lander's effective provider — so it is reachable by a provider swap or the API, not by ordinary use of the form. The deliberate no-write-time-validation decision (dynamic per-provider catalogs, #156/#157) is sound; the gap is that nothing re-validates a stored value when the chain beneath it moves.

> *This was generated by AI while landing a PR.* ## Lander audit — PR #190 **Verdict: PASS.** **Verification signal relied on:** Forgejo Actions `ci / native` — **success** (6m40s, run 213). Not re-run. Its reach covers the Go build/test suite (sqlite backend), `golangci-lint`, and the full web chain (tsc, vitest, eslint, prettier, vite build). **Two gaps in that reach, both stated rather than assumed:** - The **postgres dialect of migration `0016` is unexercised**. `internal/store/store_test.go` skips the postgres arm of `forEachBackend` unless `LAB_TEST_POSTGRES_DSN` is set, and the `store-postgres` job in `ci.yml` is still the commented-out M2 stub. So `migrations/postgres/0016_lander_model_effort.sql` has never been executed. Low risk and consistent with how 0010–0015 all landed: the statement is a bare `ALTER TABLE ... ADD COLUMN TEXT`, byte-identical to the sqlite dialect, as the file itself notes. - `ci-nix` did not run — correctly. It is paths-gated to `**/*.nix`, `flake.lock`, `go.mod`, `go.sum`, none of which this diff touches. **Checked and clean:** - Merges cleanly into `main` (no conflicts); one commit, `b686586`. - Title is Conventional Commits; body carries a working `Closes #189`; head branch `afk/189` matches the AFK contract. - Migration `0016` does not collide — `main` tops out at `0015`, both dialects, lockstep-numbered. - **All eight acceptance criteria met.** Notably AC7: `fixProvider` and `store.AuthoringRunForBranch` are gone with **no dangling callers** — a repo-wide grep finds the name only in ADR-0048's prose describing the reversal. - The reversal is genuinely by deletion, not rewiring: `LaunchFix` and the producer's auth gate both resolve empty-request through `ResolveProvider`/`ResolveModelEffort` under `RunKindFix`, and the fallback ladder is removed rather than rerouted. - The `#182` inheritance pins were rewritten to pin the *opposite* invariant rather than deleted — `TestSpawnOnce_rejectSpawnsFixRunOnAFKChain` keeps a distinctive sonnet/high authoring row precisely to prove it has no influence, and the integration test keeps its seeded haiku/low for the same reason. That is the right shape for a reversal. - The lander effort picker sources the provider's union catalog, matching the existing base/AFK picker convention (`RepoSettings.tsx`) — not a new inconsistency. **One non-blocking concern, recorded for a follow-up rather than this PR:** The new **strict** knobs interact with `#187`'s burn-on-spawn-intent rule. `autolandLaunch` (`internal/afk/autoland.go:275`) burns an attempt for `fix`/`escalate` on every outcome except `ErrOverCap` — launch-pad failures included, by design. A `lander_model`/`lander_effort` that does not resolve fails `LaunchEscalate` *at the launch pad*, so each retry burns an escalate attempt until the bound is spent and the PR goes permanently quiet. The realistic path there is not a typo but a **later `lander_provider` swap**: a value valid for provider A silently becomes invalid when the lander chain moves to provider B, whose catalog lacks it. The autoland provider pre-check gates on provider resolution only, so it does not catch this. Mitigating, which is why this is a concern and not a blocker: the exhaustion is logged at `Error` level (`spawn: escalation exhausted, autoland silent on pull`), the same exposure already exists for `lander_provider` from `#181`, and the settings UI populates both selects from the live catalog of the lander's effective provider — so it is reachable by a provider swap or the API, not by ordinary use of the form. The deliberate no-write-time-validation decision (dynamic per-provider catalogs, `#156`/`#157`) is sound; the gap is that nothing re-validates a *stored* value when the chain beneath it moves.

dominik.polakovics closed this issue

2026-07-20 19:41:20 +02:00