[mashkovd]

Part of #372: the DevLoop release pins. The loop-id params (temporal_workflow_id, temporal_run_id, execution_request_id) stay held back until the mctl-agents release containing the investigator CLI flags ships, and human_input_responses needs its gitops CWFT declaration first.

Problem

StripUndeclared dropped agent_image and agent_version for every mctl-agents-* operation. Every DevLoop run therefore used its CWFT's default image, and registry promote or rollback had no effect.

What

• agent_image and agent_version are declared, optional, on mctl-agents-investigate, -implement, -shepherd and -incidents, through agentPinParams(agent).
  • agent_image: ^ghcr\.io/mctlhq/mctl-agents(:<semver>[-pre]|@sha256:<64 hex>)$. The repository is fixed because the value becomes the pod image.
  • agent_version: <agent>@<semver>[-pre], where each operation accepts only its own agent name (issue-investigator, implementer, shepherd, incident-responder).
• New ParameterDef.OmitWhenEmpty. Declaring the pin alone would have been harmful: ApplyDefaults fills a missing param with "", and forwarding agent_image="" overrides the CWFT's default image with an empty one for every unpinned caller (the directive poller, the implement sweep, MCP triggers, and the incident responder when its registry lookup fails). With OmitWhenEmpty, such a param is never defaulted, and it is removed when empty, so the CWFT default still applies.

Audit (mctl-agents 271beaa, mctl-gitops 94cb4db)

Operation (caller)	Sends	CWFT declares	Before this PR
investigate (dev_loop.py)	issue_url, agent_image, agent_version, work_item_id, execution_id	all	pin dropped
investigate continuation	+ human_input_responses	no	dropped (needs gitops first)
implement (dev_loop.py)	service, slug, agent_image, agent_version	yes	pin dropped
shepherd tick (dev_loop.py)	service, slug, agent_image, agent_version	yes	pin dropped
incidents (incidents.py)	mode, agent_image, agent_version	yes	pin dropped
approve, reconcile, the directive poller, the implement sweep	–	yes	ok

Behaviour change on release

Once released, DevLoop runs use the registry-released image (today 1.54.0) instead of the CWFT default (1.53.1). This is the intended fix, and it belongs in the release notes. The comment in mctl-agents incidents.py claiming that mctl-api passes undeclared params through is wrong; it needs a follow-up there.

Tests

• internal/api/handlers_write_devloop_params_test.go, which goes through the real handler as mctl-agent, covers three things:
  • every parameter set the mctl-agents callers submit reaches the executor unchanged;
  • the not-yet-declared params are still stripped, so declaring one fails this test until its row moves;
  • an unpinned caller, with the pin omitted or "", sends no agent_image to Argo.
• registry_test.go: pin pass-through, pin patterns (tag, digest, latest, a doubled tag, a foreign repo, a newline, the wrong agent), and omission when empty.
• Mutation-checked:
  • removing the pin from each of the 4 operations;
  • disabling OmitWhenEmpty;
  • loosening the repo pattern.
• go test -p 1 ./... passes. golangci-lint reports only the existing G710.
• Docs: a "Release pins on mctl-agents-* operations" section in docs/agent-platform-registry.md.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @mashkovd's task in 3m 33s —— View job

---

Review complete — approved (3 P3, no P1/P2)

• Read the diff at a99810e
• Analyze registry.go (OmitWhenEmpty, agentPinParams, pin patterns)
• Trace the full submit path (StripUndeclared → ApplyDefaults → ValidateInput → Executor.Submit)
• Check test coverage and helper availability
• Post inline findings and submit verdict

What I verified

The core mechanism holds end to end:

• ApplyDefaults (internal/operations/registry.go:226) deletes the key rather than filling "", and ValidateInput (registry.go:155) already continues on empty values — so an explicit agent_image="" is neither rejected nor forwarded.
• Executor.Submit builds arguments.parameters from buildArgoParams(params) over the map (internal/operations/executor.go:178), not from op.Parameters. An omitted pin therefore genuinely reaches Argo as an absence, so the CWFT default applies. This is the load-bearing assumption of the PR and it is correct.
• These four operations reach Argo only through the generic handlers_write.go execute path (no other Executor.Submit call site touches them), so ApplyDefaults is not bypassed anywhere.
• agentImagePattern is anchored with Go's default non-multiline $ (equivalent to \z, not \Z), so the trailing-newline case in TestReleasePinPatterns really is rejected. Empty prereleases (1.54.0-) are excluded by {1,64}. Fixing the repository is the right call given the value becomes the pod's container image.
• regexp.QuoteMeta(agent) correctly prevents the agent name from being read as a pattern.
• Test helpers (newTestRouter, postAs, assertStatus, auth.NewServiceUser) all exist, and NewServiceUser is in admins, which these AdminOnly operations require — the new handler test exercises the real RBAC path.

The two-sided test table (devLoopSubmissions / devLoopParamsPendingDeclaration) is a good pattern: declaring a pending parameter early fails loudly instead of shipping a workflow Argo would reject.

Findings (all non-blocking)

	Location	Finding
P3	internal/operations/registry.go:103	agent_image / agent_version are validated independently — a mismatched or orphaned agent_version is accepted and recorded in the audit entry as the release that ran
P3	internal/operations/registry.go:226	OmitWhenEmpty silently makes Default dead; no invariant test would catch a future {OmitWhenEmpty: true, Default: "x"}
P3	internal/operations/registry.go:628	mctl-agents-incidents rides the shared mctl-agents-run CWFT — worth re-confirming against the gitops commit that actually ships

Fix the pairing gap → · Add the OmitWhenEmpty invariant test →

Caveat

go test could not be executed in this review sandbox (command approval denied), so the PR's claim that go test -p 1 ./... passes was not independently confirmed. CI covers it. If you want the reviewer to run tests, go test and go vet would need to be added to its allowed tools.
· Branch feat/372-declare-devloop-pin-params

[claude]

P3 — agent_version and agent_image are validated independently, so nothing enforces the pairing the description promises ("Omit when agent_image is omitted"). A caller can submit agent_version=issue-investigator@1.33.0 with agent_image=ghcr.io/mctlhq/mctl-agents:1.54.0, or agent_version alone with no image at all; both are accepted and the mismatched value lands in the audit entry (redactSecrets(op, input) in handlers_write.go) as the record of which release ran. Since agent_version is documented as "recorded only", a wrong record is the whole failure mode. ValidateInput is per-parameter with no cross-field hook, but handlers_write.go already does per-operation coupling checks (the approver branch), so a small check there — reject agent_version without agent_image, and require the version's semver to match the image tag when the image is a tag — would close it. Non-blocking: today's only sender builds both from the same registry row.

[claude]

P3 — OmitWhenEmpty silently makes Default dead. The branch continues before the defaulting block, so a future ParameterDef{OmitWhenEmpty: true, Default: "something"} would never apply that default, with no compile-time or test-time signal. The doc comment on the field explains why the value must not be filled, but not that Default is ignored. Cheapest fix is an invariant test over builtinOperations asserting !(p.OmitWhenEmpty && p.Default != ""), plus one line on the field comment. Same shape as the existing registry-wide invariant tests in registry_test.go.

[claude]

P3 — verification note, not a defect found in this diff. mctl-agents-incidents points at the shared mctl-agents-run CWFT, which also backs mctl-agents-run, -mentor-only and -single-service (see the block comment at registry.go:549-556). The correctness of declaring the pin here rests entirely on that one template declaring agent_image/agent_version, which I cannot check from this repo — the PR body's audit against mctl-gitops 94cb4db is the only evidence. Worth re-confirming against the gitops commit that will actually be deployed, since a mismatch fails the workflow at submission for every incident-responder run, not just pinned ones. The three siblings stay unpinned and keep the CWFT default, which matches the audit table.

[claude]

No P1/P2 findings (3 P3). Good to merge.

Verified the mechanism end to end: ApplyDefaults deletes the key, ValidateInput already skips empty values so an explicit agent_image="" is not rejected on its way out, and Executor.Submit builds arguments.parameters via buildArgoParams(params) over the map — so an omitted pin genuinely reaches Argo as an absence and the CWFT default applies. The image pattern is anchored with Go's default non-multiline $ (= \z), so the trailing-newline case the test covers is really rejected, and fixing the repository is the right call given the value becomes the pod image on an admin-only operation.

P3s: no cross-field validation between agent_image and agent_version; OmitWhenEmpty silently makes Default dead with no invariant test; and the mctl-agents-incidents declaration depends on the shared mctl-agents-run CWFT, which is worth re-confirming against the gitops commit that ships.

Note: go test could not be executed in this review sandbox, so the PR's claim that go test -p 1 ./... passes was not independently confirmed; CI covers it.

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit a99810e.

No significant issues found.