feat(gitlab): GitLab task layer — full parity with GitHub automation#176
Conversation
…sed imports Move refreshRetrospectiveMemory to finally block with non-fatal catch guard, matching github.ts pattern. Remove unused vi and RunGitlabIssueDeps imports. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add `project` field to the run command type and capture `--gitlab-project` flag when source is gitlab. Pass cmd.project to gitlabDepsFromEnv instead of cmd.repoSlug so --gitlab-project overrides git remote auto-detect. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ure comment, watch-mrs preflight Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
766c0b9 to
93af3b4
Compare
There was a problem hiding this comment.
Vanguard Review
Review: GitLab task layer (PR #176)
I focused on correctness and behavioral regressions in the new GitLab code paths. Three actionable findings, plus minor notes.
1. parseMrList re-filters on labels client-side — can silently drop every MR (medium confidence, high impact)
src/runners/mr-watch.ts:
const candidates = parseMrList(await glab(listArgs), opts.project, opts.label, opts.author);
// ...
if (!labels.includes(triggerLabel)) return [];The list call already constrains server-side with --label opts.label, so this client re-filter is redundant. Worse, it's a single point of total failure: if glab mr list --output json emits labels in any shape other than a flat string[] whose entries exactly equal the trigger label (e.g. label objects {name}, or the field is absent/named differently), labels falls back to [], labels.includes(triggerLabel) is always false, and listReady() returns nothing on every poll — the watch loop appears healthy but never reviews anything.
The issue path (GitLabTaskFetcher.list) does not re-filter and relies solely on the server --label, so the two paths are inconsistent. Recommend dropping the labels.includes(triggerLabel) guard (server already filtered) or hardening it against non-string label shapes. Note the unit test only feeds hand-built JSON with labels: ['ready for review'], so it can't catch a real glab output-shape mismatch.
2. preflight gitlabLabelsFor reads cmd.specClaimedLabel, but the field is specClaimedState (verify)
src/cli/preflight.ts:
cmd.specClaimedLabel ?? GITLAB_DEFAULT_SPEC_CLAIMED_LABEL,The watch command kind in args.ts defines the loop-v1 field as specClaimedState, not specClaimedLabel (see the existing union: specClaimedState?: string;). If this is genuinely a different/undefined property, the branch always falls through to GITLAB_DEFAULT_SPEC_CLAIMED_LABEL, so a user-supplied --spec-claimed-state is never validated by doctor. (The plan version used (cmd as any).specClaimedLabel, which would have masked this; the implemented version accesses it directly.) Please confirm the property name — if it's specClaimedState, the preflight check validates the wrong label.
3. runGitlabIssue refreshes retrospective memory unconditionally and without a task id (low/medium)
src/runners/gitlab.ts:
} finally {
await refreshRetrospectiveMemory(deps.repoPath).catch(...);
await disposeContext(ctx);
}This diverges from runGithubIssue, which calls refreshRetrospectiveMemory(deps.repoPath, ctx.taskId) only on the successful-MR path. Here it runs in finally, so it also fires on the no-changes/early-return path and on failures, and omits the ctx.taskId argument the GitHub runner passes. If the intent was parity, this records memory for runs that produced nothing and may key it differently. Worth confirming this behavioral difference is deliberate.
Minor / non-blocking
- MR dedup degrades silently when
shais empty.hasExistingReviewForHeadreturnsfalsewheneveritem.sha === '', so an MR whose head SHA can't be resolved (even after themr viewenrichment) is re-reviewed on every poll. It's logged, so acceptable — flagging for awareness. fetchMergeRequestForReviewwebUrl fallback is''(the plan used a constructedgitlab.com/...URL). With an emptyweb_url, the review prompt'sURL:line is blank. Cosmetic.isAutomationAuthoruses GitHub's[bot]convention, which doesn't match GitLab bot usernames (project_N_bot/group_N_bot). Thevanguardprefix +--authorfilter cover the main case, but GitLab service accounts won't be excluded.
No injection or secret-exposure issues found: all glab invocations pass arguments as arrays through execa (no shell), and untrusted issue/MR/error text is only ever passed as -m <value> positional args.
The server-side `--label` flag already constrains glab mr list output. The client-side `labels.includes(triggerLabel)` guard was redundant and fragile: if glab returns labels in a non-string shape the filter would silently drop every MR and listReady() would return nothing on every poll. Drop the guard and the now-unused triggerLabel parameter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Vanguard Review
I reviewed the diff focusing on correctness, security, and regressions. One blocking finding, plus a couple of lower-confidence issues worth verifying.
Blocking
vanguard run --gitlab group/project#42 ignores the project in the ref
The PR's headline command and the example file both document the ref form group/project#42 ("full project path + IID"). But the project segment of that ref is never parsed out — only the IID is used.
Trace:
src/cli/args.ts: forrun,cmd.id = 'group/project#42'andcmd.projectis set only when--gitlab-projectis passed (...(typeof values['gitlab-project'] === 'string' && picked[0] === 'gitlab' ? { project: ... } : {})).src/cli/run.tsrunGitlab:gitlabDepsFromEnv(cmd.repoPath, cmd.project, ...). Withcmd.projectundefined,gitlabDepsFromEnvfalls back to auto-detecting the project from the git origin remote.src/runners/gitlab.tsrunGitlabIssue:new GitLabTaskFetcher(deps.project).fetch(issueRef). The fetcher usesdeps.projectfor the--repoflag andissueIID(issueRef)→42. Thegroup/projectprefix in the ref is silently discarded.
Consequences:
vanguard run --gitlab group/project#42(no--gitlab-project) fetches issue 42 from whateveroriginresolves to, not fromgroup/project.parseGitlabProjectFromRemotematches GitHub SSH/HTTPS remotes too (git@github.com:owner/repo.git→owner/repo), so in a GitHub-origin checkout this runsglabagainst the GitHub slug instead of failing clearly.
No test covers this — parseCli(['run','--gitlab','owner/project#42']) only asserts cmd.id, and no test exercises the fetch path.
Fix: derive the project from the ref prefix when --gitlab-project is absent — e.g. in runGitlab/gitlabDepsFromEnv, if cmd.project is undefined and cmd.id contains a /…# prefix, use that prefix as the project (falling back to remote auto-detect only for a bare-number ref).
Worth verifying (lower confidence)
preflight gitlabLabelsFor may reference a non-existent field
src/cli/preflight.ts reads cmd.specClaimedLabel ?? GITLAB_DEFAULT_SPEC_CLAIMED_LABEL, but the watch Command type appears to expose this as specClaimedState (consistent with the GitHub path and the specClaimedState?: string field in args.ts). If the field is actually specClaimedState, then either this wouldn't typecheck, or specClaimedLabel is always undefined and a user-supplied spec-claimed state is ignored by the loop-v1 GitLab label preflight (it would silently check the default vanguard::speccing). Please confirm the field name matches args.ts.
glab flag compatibility is unverified by tests
All tests inject fake glab runners, so the real CLI surface is never exercised. A few flags are worth confirming against the installed glab version before relying on them in CI/E2E:
glab issue view … --output json,glab issue list … --output json,glab mr view … --output json,glab label list … --output json(olderglabused-F/--output-format/-F json).glab issue update … --label/--unlabelandglab mr update … --label/--unlabel.
These are out of scope for a diff-only check but are the most likely real-world break points given the "no direct REST" design.
Minor / non-blocking
src/runners/mr-watch.tsonFailureposts a plain failure note (no dedup marker) and restores the trigger label, so a persistently-failing MR accumulates one failure note per poll. Consider deduping or rate-limiting failure notes.- When
glab mr viewyields nosha, dedup is disabled and the MR is re-reviewed every poll (correctly logged). Acceptable, but worth surfacing in docs since it produces repeated review notes.
…sent `vanguard run --gitlab group/project#42` without --gitlab-project was silently ignoring the group/project prefix and falling back to the git origin remote for project detection, fetching issue 42 from the wrong project in mixed-remote checkouts. Parse the project from the ref prefix (`text before #`) and pass it to gitlabDepsFromEnv, falling back to origin remote detection only for bare-IID refs (no `#` present). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Vanguard Review
I reviewed the implemented code in the diff (treating the 2,494‑line plan doc as non‑shipping context). Findings below, strongest first.
Findings
1. MR review dedup silently caps at 100 notes (duplicate reviews / wasted spend) — Medium
src/runners/mr-watch.ts, hasExistingReviewForHead:
const out = await glab(['api', `projects/${encodeProject(item.project)}/merge_requests/${item.iid}/notes?per_page=100`]);Only the first page of notes is fetched, with no --paginate and no explicit sort. The SHA‑based review marker is what prevents re‑reviewing the same head commit. On an MR with more than 100 non‑system notes (busy MRs are exactly the ones that get re‑polled often), if the page returned does not contain the marker, listReady will re‑emit the MR every poll, posting a fresh ## Vanguard Review note each tick and burning LLM budget. Please paginate, or explicitly request newest‑first (?sort=desc&per_page=100) and document the bound. The same 100‑note cap exists for issue notes in src/tasks/gitlab.ts (/issues/${iid}/notes?per_page=100), though that path is less marker‑sensitive.
2. Preflight requires vanguard::* state labels that GitLab auto‑creates on first use — Medium
src/cli/preflight.ts, gitlabLabelsFor + gitlabLabelsOk. For watch-mrs/doctor-mrs the required set is [label, reviewingLabel, reviewedLabel], and for gitlab issue watch it includes claimedState/reviewState/specClaimedLabel. But nothing in the flow pre‑creates these — GitLab materializes a scoped label (vanguard::running, vanguard::reviewing, …) the first time it is applied via glab ... --label. So on any fresh project doctor-mrs will report gitlab labels: missing vanguard::reviewing, vanguard::reviewed and watch-mrs will hard‑fail its own preflight (watchMrsCommand throws on !report.ok) even though the labels would be created automatically on the first claim. Only the user‑authored trigger label should be required; the Vanguard‑owned state labels should not be preflight‑gated (or should be created up front). This contradicts the PR's stated "GitLab auto‑removes the old one" design premise.
3. Unverified glab flag/output assumptions will fail silently at runtime — Medium (verify)
Several calls assume glab supports --output json on subcommands and that list output is uncapped:
gitlabLabelsOk:glab label list --repo <p> --output json(no limit).ghside uses--limit 200; ifglab label listpaginates/caps or doesn't accept--output json, the parse yields a short/empty set and preflight reports valid labels as "missing".GitLabTaskFetcher.fetch:glab issue view <iid> --repo <p> --output json— assumesviewsupports JSON and thatlabelscome back asstring[](GitLab can return label objects depending on version;toGitLabTaskwould then map non‑strings intotask.labels).fetchMergeRequestForReview: relies onglab mr view --output jsonexposingsha,source_branch,target_branch.
None of these are exercised by the unit tests (all use fakes), so a flag mismatch ships green. Please confirm against the pinned glab version, ideally with one integration smoke test, before merge.
4. Raw error text posted to GitLab issues/MRs — Low (info exposure)
src/runners/watch.ts (onFailure: ... \Vanguard run failed: ${String(error)}`), src/runners/gitlab.ts/mr-watch.tssimilarly.execarejections stringify the full command line and stderr; agent/provider failures can include file paths or upstream API response bodies. This goes into a project‑visible note. Consider truncating/sanitizing (e.g.error.shortMessageor a capped message) rather than dumpingString(error)`. Mirrors existing GitHub behavior, so not a regression, but worth tightening for the new public‑comment paths.
5. parseGitlabProjectFromRemote accepts any git host → wrong‑host run — Low
src/runners/gitlab.ts. The regex parses GitHub/Bitbucket/etc. remotes just as readily as GitLab. With run --gitlab 42 (bare IID, no --gitlab-project) in a repo whose origin is GitHub, gitlabDepsFromEnv resolves a project from the GitHub remote and proceeds to call glab against it. A bare‑IID gitlab run with a non‑GitLab origin should error rather than guess. Minor, but cheap to guard.
6. Minor: draft/automation filtering depends on glab mr list JSON shape — Low (verify)
parseMrList drops drafts via item.draft === true. If glab mr list --output json represents draft state differently (e.g. work_in_progress, or only the Draft: title prefix) in the target version, draft MRs would pass the filter and get auto‑reviewed. Confirm the field name; consider also checking a Draft: title prefix as a fallback.
Non‑blocking notes
editGitlabLabelsguardif (args.length > 5)correctly no‑ops when both add/remove are empty (base args length is 5) and matches the test. Good.- All
glabinvocations use array args viaexeca(no shell) — no command injection. Good. listReadyenrichment inmr-watch.tsdoes up to 2 extraglabcalls per candidate MR (view for sha + notes for dedup); bounded by ready count, acceptable, but note it's an N+1 against the list.
Items 1–3 are the ones I'd want resolved (or explicitly verified against a real glab) before merge.
…gination - preflight: only check user-owned trigger/routing labels; Vanguard state labels (vanguard::running, vanguard::review, vanguard::reviewing, etc.) are scoped :: labels that GitLab auto-creates on first apply, so requiring them to pre-exist causes doctor/watch-mrs to fail on fresh projects - preflight: add --per-page 100 to glab label list (default is 30) - mr-watch: add sort=desc&order_by=created_at to notes query so the most-recent notes (where the SHA dedup marker lives) are in page 1 - gitlabDepsFromEnv: reject github.com/bitbucket.org/dev.azure.com remotes when falling back to origin auto-detection to avoid silently running glab against a non-GitLab host Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Vanguard Review
Review: GitLab task layer (PR #176)
I reviewed the implementation files (not the plan doc) for correctness, security, and regressions. The shell-out surface is safe — every glab/git/gh call uses execa with an argument array (no shell interpolation), so label names, comment bodies, and agent output can't inject commands. Label state-machine logic in the watch/spec/MR primitives is internally consistent, and the preflight refactor correctly gates the GitHub/Linear blocks behind isLoopCommand so watch-mrs/doctor-mrs don't accidentally run GitHub checks.
A few non-blocking findings worth addressing:
1. glab argument vectors are never exercised by tests (test-coverage gap)
Every unit test injects a fake GlabRunner, so the actual glab flags are unverified — including glab issue view … --output json, glab issue list --output json, glab label list … --output json (src/cli/preflight.ts gitlabLabelsOk), glab mr diff, and glab mr view … --output json. The PR's own test plan leaves all three real-glab smoke tests unchecked. If any of those flag spellings are wrong (glab historically uses -F/--output, and --output json support varies by subcommand/version), the feature fails at runtime with green CI. Recommend running at least one real vanguard run --gitlab and doctor-mrs against a live project before merge, as the plan intended.
2. isAutomationAuthor over-broad prefix match
src/runners/mr-watch.ts:
function isAutomationAuthor(username: string): boolean {
const lower = username.toLowerCase();
return lower.startsWith('vanguard') || ...
}Any human whose username merely starts with vanguard (e.g. vanguard-dev) is silently excluded from watch-mrs. Consider an exact match against the configured bot account, or make it configurable. (Note: this also drifted from the plan's includes('vanguard') — fine, but worth a deliberate choice.)
3. Note pagination caps dedup/comment retrieval at 100
GitLabTaskFetcher.fetch (projects/.../issues/{iid}/notes?per_page=100) and hasExistingReviewForHead (merge_requests/{iid}/notes?per_page=100) fetch only the first page. For the dedup check the sort=desc ordering makes a recent marker likely to land on page 1, so it's usually fine — but on a long-lived MR an older marker could fall off, causing a redundant re-review. For fetch, issues with >100 comments silently drop context handed to the agent. Low impact; flag if long threads are expected.
4. refreshRetrospectiveMemory call differs from the GitHub runner
src/runners/gitlab.ts calls refreshRetrospectiveMemory(deps.repoPath) (single arg) inside finally, so it runs even when no commit was produced — whereas the GitHub runner (per the plan) calls it with ctx.taskId only after a successful commit. Worth confirming this is intentional and that the one-arg signature is correct (and that refreshing memory on a no-op run is desired).
5. watch-mrs claim can't detect a concurrent claim (cross-process)
gitlabMergeRequestWatchPrimitives.claim just edits labels, which never throws for an already-claimed MR, so watchMergeRequestsOnce's catch → skipped branch can't fire across processes — two watchers could both review the same MR. The SHA-marker dedup in listReady mitigates within a poll, and this mirrors the existing PR-watch pattern, so it's not a regression. Mentioning for awareness.
None of these prevent merge.
No blocking findings.
Summary
glabCLI (same pattern asghfor GitHub)vanguard::running,vanguard::review, etc.) for automatic mutual-exclusivity on label transitionsNew commands
vanguard run --gitlab group/project#42vanguard watch --source gitlab --label vanguardvanguard watch --source gitlab --spec-label "ready for spec" --agent-label "ready for agent"vanguard review-mr --mr 42 --gitlab-project g/pvanguard watch-mrs --gitlab-project g/p --label "ready for review"vanguard doctor-mrs --gitlab-project g/pNew files
src/gitlab-labels.ts— 5 scoped label constantssrc/tasks/gitlab.ts—GitLabTaskFetcher implements TaskFetcher,encodeProject,issueIID,editGitlabLabels,commentGitlabIssue,linkMergeRequestsrc/runners/gitlab.ts—runGitlabIssue,gitlabDepsFromEnvsrc/runners/mr-review.ts—parseMergeRequestRef,fetchMergeRequestForReview,reviewMergeRequest, SHA-based dedup markersrc/runners/mr-watch.ts—gitlabMergeRequestWatchPrimitives,watchMergeRequestssrc/cli/review-mr.ts,watch-mrs.ts,doctor-mrs.ts— thin CLI handlersModified files
src/pipeline/pipeline.ts—PublishOptions.cli?: 'gh' | 'glab'; backward-compatiblesrc/runners/watch.ts—gitlabWatchPrimitives,gitlabSpecPrimitives,watchGitlab,watchGitlabLoopV1src/cli/args.ts—WatchSource += 'gitlab'; new command kinds + flagssrc/cli/preflight.ts—gitlabAuthOk,gitlabLabelsOk, GitLab preflight wiringsrc/cli/run.ts,watch.ts,index.ts— dispatch wiringsrc/index.ts— all new symbols exportedKey design decisions
glabCLI only — no direct REST; consistent withghpatternvanguard::running/vanguard::review/vanguard::speccingare mutually exclusive within their::scope; GitLab auto-removes the old one onreview, explicit remove only onclaim--source gitlab-board— GitLab boards are label-based; use--source gitlab --label <column-label>publishForReviewextended withcli?: 'gh' | 'glab'—glab mr createuses--source-branch/--target-branch/--descriptioninstead of--head/--base/--bodywatch-mrs/doctor-mrs— separate fromwatch-prsto avoid naming contradictionready for spec,ready for agent,needs info)Workflow notes (for reviewer)
.github/workflows/was not modified. No workflow changes are needed — the GitLab commands run locally / in CI against the GitLab API viaglab. If the repo ever adds a GitLab CI job that invokesvanguard watch --source gitlab, theGITLAB_TOKENenv var is all that's needed (glabpicks it up automatically).Test plan
pnpm typecheck— cleanpnpm test— 647/647 passing (63 test files)vanguard doctor-mrs --gitlab-project <your-project>— run against a real GitLab project to confirm preflightvanguard run --gitlab <project>#<iid>— smoke test against a real issuevanguard watch-mrs --gitlab-project <p> --label "ready for review" --once— confirm MR polling + dedup🤖 Generated with Claude Code