feat(worker): open change requests on GitLab, not only Forgejo #46
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "gitlab-worker"
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?
Why
ploegd has spoken GitLab since rc.31 —
pkg/provider/gitlabcomments on a merge request and verifies inbound webhooks.ploeg-workernever learned. It polled/api/v1/repos/{owner}/{name}/pullsand its briefing named the Forgejo API as the way to open a change request, so against a GitLab target nothing was ever opened.That one absence removes most of the review loop. No change request means the Shift has nothing for a reviewing Role to comment on,
publishRoundlogsfindings not published: no pull request on this shift yet, no forge webhook can arrive, andclose-the-review-loop's fix Rounds never open. Ingest, routing, Rounds and budgets all work — the work simply never leaves the factory. Every step of that is silent: the writing Run reports success.What was already fine
Three things are forge-agnostic already and are untouched here, which is why this is one change and not three:
pkg/worker/git.gobuilds clone and push URLs from owner and name — already correct for a GitLab subgroup path. Git is git.pkg/shiftengine'sprPathReis/(?:pulls?|merge_requests)/(\d+)/?$and has matched GitLab all along.pullRequest(reports)takes the Shift's change request from the writing Run's OutcomeReport links. The poll is ground truth for a Run that died before reporting, not the primary path.The gap was exactly two places that name a forge: the poll, and the briefing.
What changes
harness.RepoRef.Forge— the API dialect, additive and optional ontaskspec.v1. Empty meansforgejo, so every stored Target and every existing deployment keeps its exact meaning. The value travels on the Work Item (work.Target.Forge, there since ADR-0016) and falls back to a deployment default.findPRdispatches. GitLab filterssource_branchserver-side, so unlike the Forgejo call it cannot be defeated by a repo with more than a page of open requests. The project is addressed by URL-encoded full path —code14nl/internal/poc-silkis three segments and the slashes must survive as%2F.changeRequestNounandopenChangeRequestswitch on the same value so they cannot drift.executor.forgeselects one active forge,executor.gitlabconfigures it, and a newploeg.forgehelper resolves whichever is active so no template reads.forgejoor.gitlabdirectly.Two things worth a reviewer's attention
A nil pointer removed in passing.
ploeg.workerPodTemplatedereferenced.Values.executor.forgejo.urlunconditionally, soforgejo: null— the documented way to empty a block whose defaults name Secrets you have no reason to hold — became a nil pointer the momentexecutor.enabledwent true. The newexecutor-gitlabfixture renders withforgejo: nullon purpose, so it cannot come back without failing a golden.A fourth golden. Selecting a forge changes the worker pod — a different credential Secret, a different API — and the worker pod is the boundary the goldens exist to police.
executor-gitlab.yamlpins ADR-0013 tier 1 holding on GitLab:Process, stated plainly
The code was written before it was proposed. It came out of a survey of what rc.31 could and could not do for code14's staging cluster; the durable commitment was extracted afterwards. That is the opposite of the order AGENTS.md asks for, and it is why ADR-0023 is
proposedand nothing here marks it accepted — the schema's rule against retro-justification is exactly the risk. The record states the two alternatives genuinely weighed (per-Team, per-deployment) and what each would have cost, so ratifying it is a decision rather than a rubber stamp.openspec/changes/open-change-requests-on-gitlab/adr.mdsays so out loud.If ADR-0023 is rejected the change does not shrink, it changes shape: per-Team dialect is about the same code with a different owner for the answer.
A contract gap the gate caught, not me.
RepoRef.Forgefirst landed without its schema edit.repointaskspec.v1.schema.jsonisadditionalProperties: false, so a Task Spec carrying the field would have been rejected by any consumer validating against the published contract. The gate missed it initially becausefullTaskSpec— the fixture whose job is to carry every field — did not set it. Fixed in the order the tasks rules ask for: fixture first, observed to fail withat '/repo': additional properties 'forge' not allowed, then the schema. Second commit.No VIK trailer — this did not come from a board ticket.
Non-goals
/api/v1/admin/users/forge, a Forgejo admin endpoint with no GitLab equivalent. The nearest analogue is a project access token — a different escalation with a different blast radius, deserving its own record rather than an implied one. GitLab runs on the shared token, which is the documented pre-tier-2 behaviour. Tier 1 does port and is configured. Named as a re-evaluation trigger on 0023.Downstream
openspec/.../tasks.mdgroup 5 carries the wiring in code14's staging cluster, which is what this exists for and what proves the loop closes. The ordering constraint that matters: the chart has noadditionalProperties: false, soexecutor.forgeagainst rc.31 is silently ignored — values must not land before theOCIRepositorybump.Gates
Run locally with the toolchain CI pins. Note helm v4.2.3, not 4.2.4 — the two disagree about the blank line before a document separator, exactly as
scripts/helm-golden.shwarns on failure; it cost a diagnosis here.New tests:
pkg/worker/forge_test.go(both dialects, empty-means-forgejo, subgroup path encoding, wrong-base rejection on both, unknown dialect named in the error, HTTP failure) andpkg/worker/prompt_forge_test.go(GitLab writer and reader vocabulary and endpoint, already-open branch, and the Forgejo contract byte-for-byte unchanged).ploegd has spoken GitLab since rc.31 — pkg/provider/gitlab comments on a merge request and verifies inbound webhooks. ploeg-worker never learned: it polled /api/v1/repos/{owner}/{name}/pulls and its briefing named the Forgejo API as the way to open one. On a GitLab target no change request was ever created, so the Shift had none for a reviewer to comment on, publishRound logged "no pull request on this shift yet", and the review loop could not close. Silent all the way to a human. Less was missing than it looks. Three things were already forge-agnostic and are untouched here: git.go's authURL/plainURL build owner/name into a URL that is already correct for a GitLab subgroup; shiftengine's prPathRe already matches /merge_requests/(\d+); and the Shift takes its change-request URL from the OutcomeReport links, not from the poll. The gap was two places that name a forge, so that is what this changes. RepoRef gains Forge, the API DIALECT. Empty means forgejo, so every stored target, every taskspec and every deployment that never set it keeps its exact current meaning. The dialect travels on the work item — pkg/work.Target has carried Forge all along — and falls back to the worker's configured default, matching the "empty = the default forge" promise ploegd's own registry makes. - findPR dispatches. GitLab filters source_branch server-side, so unlike the Forgejo call it cannot be defeated by a repo with 50+ open requests. The project is addressed by URL-ENCODED full path: code14nl/internal/poc-silk is three segments and the slashes must survive as %2F. - The briefing dispatches, in vocabulary as well as endpoint. An agent told to open a "pull request" on GitLab looks for an endpoint that is not there, and the noun is what it searches its tools and the repo's docs for. Noun and endpoint come out of the same switch so they cannot drift. - An unknown dialect fails loudly. Falling back to Forgejo would poll a real endpoint shape against the wrong host and report "no change request" forever — indistinguishable from an agent that never opened one. Chart: executor.forge selects one active forge and executor.gitlab configures it. The new ploeg.forge helper resolves whichever is active into one shape, so no template touches .forgejo or .gitlab directly. That also removes a trap: the worker template used to dereference .Values.executor.forgejo.url unconditionally, which made `forgejo: null` — the documented way to empty an unused block — a nil pointer the moment executor.enabled flipped true. The new GitLab fixture renders with forgejo null precisely so that cannot come back. FORGE_URL is the name; FORGEJO_URL is emitted alongside it and still accepted, so a ScaledJob starting a pod from the previous image mid-upgrade still finds a forge. A fourth golden, executor-gitlab, because selecting a forge changes the worker pod — a different credential Secret and a different API — and the worker pod is the boundary the goldens exist to police. It pins the ADR-0013 tier-1 split holding on GitLab: readers draw agent-reader-token, the writer draws agent-builder-token. Tier 2 is deliberately absent. ploegd mints per-run push credentials through /api/v1/admin/users/forge, a Forgejo admin endpoint with no GitLab equivalent; the nearest analogue is a project access token, a different escalation that deserves its own ADR rather than an implied one. Unset means the shared token, which is the documented pre-tier-2 behaviour. Gates run locally with the pinned toolchain (helm v4.2.3 as CI pins; 4.2.4 disagrees about the blank line before a document separator, as scripts/ helm-golden.sh warns): gofmt -l . clean go vet ./... clean go build ./... ok go test ./... ok, all packages incl. pkg/store helm lint 1 chart linted, 0 failed helm-golden.sh check ok, 4 renders No VIK trailer: this did not come from a board ticket.RepoRef.Forge landed without its schema edit, which docs/contracts/README.md does not allow: v1 changes additively, and the Go type and the published schema change together. `repo` is additionalProperties:false, so a Task Spec carrying the field would have been rejected by any consumer validating against the published contract. The gate did not catch it because fullTaskSpec — the fixture whose whole job is to carry every field — did not set Forge, and omitempty dropped it. Fixed in the order the tasks rules ask for: the fixture first, observed to fail with at '/repo': additional properties 'forge' not allowed then the schema. The enum is constrained to the dialects the worker actually implements, so a Task Spec naming a third forge fails at the contract rather than at run time.Review feedback. Every comment this change added is gone; the diff now adds none. What the prose carried is carried by names, types and the ADR instead. prMatches -> isRunChangeRequest(changeRequest, runBranch, base) findPR -> findOpenChangeRequest listForgejoPRs -> listForgejoPullRequests listGitLabMRs -> listGitLabMergeRequests forgeGet -> getJSON openChangeRequest -> openChangeRequestInstruction Config.Forge -> Config.DefaultForge changeRequest{URL,Head,Base} -> {URL,HeadBranch,BaseBranch} The "no silent fallback" comment is now errUnsupportedForge, a named sentinel the test asserts with errors.Is rather than a substring. The subgroup comment is RepoRef.ProjectPath, which joins and never splits. The chart's helper comment is the helper's own shape. TWO NAMES FOR ONE VALUE, REMOVED. requireEnvOneOf("FORGE_URL", "FORGEJO_URL") was a shim for a rolling upgrade that cannot happen: the release train keeps chart and appVersion in lockstep, so the chart and the image it configures move together. The worker now requires FORGE_URL and nothing else, and the chart emits only that. requireEnvOneOf is deleted. The dialect had the same problem in a worse form: PLOEG_FORGE on the worker was a second spelling of PLOEG_TARGET_FORGE, which ploegd has read since the forge registry landed — the same concept, the same default, two names, one of them invented here. The worker now reads PLOEG_TARGET_FORGE too, and the chart renders it once for both binaries from executor.forge, so they cannot disagree about which forge is the default. Both renames are breaking for anyone setting these by hand and neither is for a chart-driven deployment. That trade is the point: two names for one value is a worse thing to own than a rename under a version bump. values.yaml loses its comment blocks; the meaning moved into values.schema.json descriptions, which is where a Helm chart keeps structured intent — validated, machine-readable, and shown by tooling rather than only to whoever opens the file. Goldens regenerated (helm v4.2.3, as CI pins). Gates: gofmt clean, go vet clean, go build ok, go test ./... ok, helm lint ok, 4 renders ok, helm-golden.sh check ok, go test ./internal/ledger/ ok, openspec validate --all 6 passed.Review feedback addressed in
a790777. Two corrections to the description above, which is now stale on both points.1. Every comment this change added is gone. The Go diff now adds zero. What the prose carried is carried by names, types and the ADR instead:
The "no silent fallback" paragraph is now
errUnsupportedForge, a named sentinel the test asserts witherrors.Israther than a substring match. The subgroup paragraph isRepoRef.ProjectPath, which joins and never splits.values.yamllost its comment blocks and the meaning moved intovalues.schema.jsondescriptions — validated, machine-readable, and surfaced by tooling rather than only to whoever opens the file.2.
FORGE_URL/FORGEJO_URLis gone — one name. The description above says both are emitted. They were, and that was wrong.requireEnvOneOf("FORGE_URL", "FORGEJO_URL")hedged a rolling upgrade that cannot happen: the release train keeps chart and appVersion in lockstep, so the chart and the image it configures move together. The worker now requiresFORGE_URLand the chart emits only that;requireEnvOneOfis deleted.The dialect had the same problem in a worse form.
PLOEG_FORGEon the worker was a second spelling ofPLOEG_TARGET_FORGE, which ploegd has read since the forge registry landed — same concept, same default, two names, and the second one invented here. The worker now readsPLOEG_TARGET_FORGEtoo, and the chart renders it once fromexecutor.forgefor both binaries, so they cannot disagree about the default forge.Both renames break anyone setting these by hand; neither breaks a chart-driven deployment. That trade is the point — two names for one value is a worse thing to own than a rename under a version bump.
Gates re-run, goldens regenerated with helm v4.2.3:
Still open and unchanged: ADR-0023 needs ratification (tasks 1.2) — it is
proposed, and the code predates it, whichadr.mdstates outright.