[mashkovd]

Why

This is item 3 of #372. After a human answers a sealed question, the DevLoop continuation sends the accepted answers to mctl-agents-investigate as human_input_responses (mctl-agents#473, dev_loop.py). The parameter was deliberately left undeclared until the CWFT declared it, and TestExecuteOperation_PendingDevLoopParamsAreStillStripped pinned that. As a result StripUndeclared dropped the answers, and the continuation asked the same question again.

mctl-gitops#1581 (merged as f388365) now declares the parameter in cwft-mctl-agents-investigate and forwards it as --human-input-responses. The investigator has consumed it since mctl-agents#558, released as 1.66.0 and promoted to production.

What

• Registry. registry.go declares human_input_responses on mctl-agents-investigate: optional, with OmitWhenEmpty and the pattern (?s)^\[.*\]$.
  • The value is opaque to mctl-api. The investigator validates every entry (ids, hashes, bounded values) before any clone or model call. Here only the array shape is pinned.
  • An empty value is omitted, so a run without answers keeps the template's exact argv.
• Tests.
  • The row moves from devLoopParamsPendingDeclaration to devLoopSubmissions as "investigate, human-input continuation".
  • New TestExecuteOperation_HumanInputResponsesShape: a non-array value gets a 400 and never reaches the executor, and an empty value is omitted.
  • Mutation-checked: dropping the declaration fails the never-dropped test, and dropping either the pattern or OmitWhenEmpty fails the shape test.

go test ./... passes locally.

Refs #372, mctlhq/mctl-agents#473

🤖 Generated with Claude Code

https://claude.ai/code/session_012kWiYf1BX6wcXB2kc8x15U

[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 2m 36s —— View job

---

Reviewing PR #443

• Gather context (CLAUDE.md, diff)
• Analyze registry.go changes
• Analyze test changes
• Post findings
• Submit verdict

Verdict: APPROVE (0 P1, 0 P2, 1 P3)

Reviewed git diff 90d7eb76ef59ba24fecb8fd5cfd58ab39a0beeb4...18994c5f2f240cafbf6bdab7d441955e87b3fda3.

• registry.go: human_input_responses is declared optional, OmitWhenEmpty: true, with pattern (?s)^\[.*\]$. Confirmed the pattern is RE2-safe (no ReDoS), correctly anchored, and consistent with the existing agentPinParams/devLoopCorrelationParams conventions. StripUndeclared/ApplyDefaults interplay is correct — an omitted or empty value never reaches Argo, matching the stated "keep the CWFT's exact argv" goal.
• Tests: the new devLoopSubmissions row round-trips correctly through TestExecuteOperation_DevLoopParamsAreNeverDropped, and TestExecuteOperation_HumanInputResponsesShape correctly checks both the 400-before-executor rejection of a non-array value and omission of an empty value.
• No security, concurrency, or error-handling issues — the value stays opaque to mctl-api and is forwarded as a structured Argo parameter (not shell-interpolated).

P3 (non-blocking): internal/operations/registry_cwft_params_test.go's cwftDeclaredParams["mctl-agents-investigate"] is documented as a transcription of the real CWFT's declared parameters, but wasn't updated to include human_input_responses even though the PR says cwft-mctl-agents-investigate now declares it (mctl-gitops#1581). TestCWFTParamsAreDeclaredOrExplicitlyInternal only checks one direction (every listed name must be declared/exempted), so this won't fail CI — but it leaves that inventory quietly stale. Couldn't post this inline since the file isn't part of this diff, so it's included in the review body instead.

Note: I was unable to run go test ./... in this sandboxed environment (Bash commands required approval that wasn't available), so this review is based on static analysis of the diff and surrounding code rather than a local test run. The PR description states tests pass locally.

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit 18994c5.

No significant issues found.

[claude]

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

P3 (nit, not blocking): internal/operations/registry_cwft_params_test.go's cwftDeclaredParams["mctl-agents-investigate"] — explicitly documented as a transcription of the live CWFT's spec.arguments.parameters — wasn't updated to list human_input_responses, even though the PR description says cwft-mctl-agents-investigate now declares it (mctl-gitops#1581). TestCWFTParamsAreDeclaredOrExplicitlyInternal only checks the direction "every name in the list is declared or exempted," not the converse, so this doesn't fail CI, but it leaves that inventory quietly out of date. Not inline-commentable since the file isn't part of this diff.

Verified: Pattern (?s)^\[.*\]$ is RE2-safe (no backtracking/ReDoS risk), anchored on the full value; OmitWhenEmpty + StripUndeclared/ApplyDefaults interplay is correct, matching the existing agentPinParams/devLoopCorrelationParams pattern; new test row in devLoopSubmissions round-trips through TestExecuteOperation_DevLoopParamsAreNeverDropped; new TestExecuteOperation_HumanInputResponsesShape correctly asserts a non-array value is rejected pre-executor and an empty value is omitted. No security/concurrency/error-handling issues — the value stays opaque to mctl-api and is forwarded as a structured Argo parameter, not shell-interpolated.

[claude]

Claude finished @mashkovd's task in 2m 23s —— View job

---

Reviewing PR #443 (commit b258796)

• Gather context (CLAUDE.md, diff)
• Analyze registry.go changes
• Analyze test changes
• Post findings
• Submit verdict

Verdict: APPROVE (0 P1, 0 P2, 1 P3)

Reviewed git diff 90d7eb76ef59ba24fecb8fd5cfd58ab39a0beeb4...b258796084f885430cd705ce43bafea39c57fa2c.

• registry.go: human_input_responses is declared optional, OmitWhenEmpty: true, pattern (?s)^\[.*\]$. Confirmed the ValidateInput/ApplyDefaults/StripUndeclared interplay is correct: an empty or omitted value never reaches Argo (pattern check is skipped for empty values, and ApplyDefaults strips the key when OmitWhenEmpty is set), while a non-empty value is pattern-checked before the executor ever sees it. This matches the existing agentPinParams/devLoopCorrelationParams conventions elsewhere in the file.
• Tests: TestExecuteOperation_HumanInputResponsesShape correctly asserts a non-array value gets a 400 and never reaches exec.submittedParams, and that an empty value is omitted. The new devLoopSubmissions row round-trips through TestExecuteOperation_DevLoopParamsAreNeverDropped.
• The P3 from the earlier review (stale cwftDeclaredParams["mctl-agents-investigate"] inventory in registry_cwft_params_test.go) is fixed in this commit — human_input_responses was added to that list.
• No security, concurrency, or error-handling issues. The value stays opaque to mctl-api and is forwarded as a structured Argo parameter, not shell-interpolated. The operation is AdminOnly, limiting exposure.

P3 (non-blocking, posted inline on internal/operations/registry.go:818): (?s)^\[.*\]$ has no upper bound on length, unlike every other Pattern in this file (e.g. work_item_id/execution_id cap at 1–64 chars, temporal_* at 0–127). A length bound (even a generous one) would keep an unexpectedly large answer set from reaching Argo at all, rather than relying entirely on the investigator's downstream validation. Not blocking given the admin gate and the deliberately-opaque design.

Posted as a formal PR review (approving) bound to commit b258796084f885430cd705ce43bafea39c57fa2c.

[github-actions]

Antigravity (agy) review — informational

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

No significant issues found.

[claude]

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