Skip to content

feat(investigate): forward human_input_responses to the investigator - #1581

Merged
mashkovd merged 2 commits into
mainfrom
feat/investigate-human-input-responses
Oct 4, 2026
Merged

mashkovd merged 2 commits into
mainfrom
feat/investigate-human-input-responses

Conversation

@mashkovd

@mashkovd mashkovd commented Oct 4, 2026

Copy link
Copy Markdown
Member

Why

The human-input producer shipped in mctl-agents#558 (release 1.66.0). After a human answers a sealed question, the DevLoop continuation sends the accepted answers as the human_input_responses parameter (dev_loop.py), and the investigator reads them as --human-input-responses. mctl-agents-investigate did not declare the parameter, so the answer never reached the run. As docs/runbooks/human-input-e2e.md in mctl-agents notes, the continuation then re-asks the question and the workflow skips it by hash. This is the gitops half of mctl-api#372 item 3.

What

cwft-mctl-agents-investigate.yaml:

  • Declares the optional human_input_responses parameter, empty by default.
  • Adds the env WORKFLOW_HUMAN_INPUT_RESPONSES.
  • Appends --human-input-responses "$WORKFLOW_HUMAN_INPUT_RESPONSES" only when the value is non-empty. Every run without answers keeps its exact current argv.
  • Reads the value through env, never spliced into the shell, the same as issue_url and the correlation ids.

Order and what this does not do

  • This must land before mctl-api declares the parameter in registry.go (it is in devLoopParamsPendingDeclaration today). Until then mctl-api strips it, so this PR alone changes no live run.
  • The flag requires an issue-investigator release with mctl-agents#558 (1.66.0+). Releases before that exit non-zero on it. Only continuations send it, and only once mctl-api forwards it.
  • The capability stays inert while the template runs the legacy resolver mode (agent-platform README, "Kill switch"). This PR does not change the resolver mode.

Refs mctlhq/mctl-agents#473, mctlhq/mctl-api#372

🤖 Generated with Claude Code

https://claude.ai/code/session_012kWiYf1BX6wcXB2kc8x15U

The DevLoop continuation (mctl-agents#473, dev_loop.py) sends accepted
human-input answers as human_input_responses, and the investigator
consumes them as --human-input-responses since mctl-agents#558. The
template did not declare the parameter, so the answer never reached the
run and the continuation re-asked the same question.

Declared optional and omitted from argv when empty, so every run that
does not carry answers keeps its exact current argv. Read through env,
never spliced into the shell, like the other caller-supplied values.

Claude-Session: https://claude.ai/code/session_012kWiYf1BX6wcXB2kc8x15U
Copilot AI balanced review requested due to automatic review settings October 4, 2026 10:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@claude

claude Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

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


Reviewing PR #1581

  • Gather context (diff, CLAUDE.md conventions)
  • Analyze cwft-mctl-agents-investigate.yaml changes
  • Post findings / verdict

Verdict: Changes requested (1 P2, 0 P1, 0 P3)

The implementation itself follows the file's established pattern correctly:

  • New human_input_responses parameter defaults to "" — zero behaviour change for every existing caller.
  • Read through WORKFLOW_HUMAN_INPUT_RESPONSES env, never shell-interpolated directly — the safe pattern enforced by scripts/validate-shell-param-interpolation.py for every sibling parameter (work_item_id, execution_id, temporal_workflow_id, temporal_run_id, execution_request_id).
  • Appended to argv only when non-empty, in the same run-investigator container whose env defines it.

P2 — missing test coverage: tests/test_cwft_investigate_work_context_argv.py exists specifically to pin this exact set -- argv-building block for every optional identifier added so far — omit-empty passthrough (T1), flag ordering (T2/T6/T7), and shell-injection safety (T4/T8). This PR extends the same block with --human-input-responses (lines 414-415) but doesn't extend that test, so a future change to the block can silently regress the new flag with nothing to catch it. Posted inline with a suggested fix (add WORKFLOW_HUMAN_INPUT_RESPONSES to the test's run() env dict and mirror T2/T3/T6/T8 checks for the new flag).

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Antigravity (agy) review — informational

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

Findings

[P3] No test visible in this diff for workflow parameter forwarding

  • File: platform-gitops/argo-workflows/cluster-templates/cwft-mctl-agents-investigate.yaml:123
  • Anchor: + - name: human_input_responses
  • Concrete failure scenario: The workflow template adds parameter human_input_responses and conditional forwarding logic to append --human-input-responses when the environment variable is populated. There is no test visible in this diff under tests/ to validate template schema compliance, parameter default value handling, and command argument synthesis when human_input_responses is provided versus when omitted.
  • Proposed fix: Add a test under tests/ that parses cwft-mctl-agents-investigate.yaml to verify schema validation and test shell argument assembly for both empty and non-empty values of WORKFLOW_HUMAN_INPUT_RESPONSES.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has P1/P2 findings, changes requested: 1 P2 (0 P1, 0 P3). The implementation itself is correct and follows the file's established pattern exactly: optional parameter defaulting to empty (zero behaviour change when omitted), env-bound (never shell-interpolated — consistent with scripts/validate-shell-param-interpolation.py's safe pattern), appended to argv only when non-empty, and the env var lives in the same container as the script that reads it. Remaining gap is missing test coverage: tests/test_cwft_investigate_work_context_argv.py exists specifically to pin this exact argv-building block for every prior optional identifier (work_item_id, execution_id, temporal_workflow_id, temporal_run_id, execution_request_id) — omit-empty passthrough, flag ordering, and injection safety — and was not extended for the new human_input_responses flag.

Extends test_cwft_investigate_work_context_argv.py, which pins every
optional identifier this set -- block forwards, to the new flag: a
realistic JSON answers array reaches the investigator as one verbatim
token, appended last, omitted when empty, and a hostile value never
executes. Removing the forwarding block or splicing the value through
eval each fails four of the new checks.

Claude-Session: https://claude.ai/code/session_012kWiYf1BX6wcXB2kc8x15U
@claude

claude Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Claude finished @mashkovd's task in 1m 31s —— View job


Reviewing PR #1581 (commit afb2587d)

Verdict: Approved (0 P1, 0 P2, 0 P3)

This commit (afb2587d) extends the prior commit (bba0419e) with exactly the test coverage flagged as missing in the earlier review round:

  • run()'s env dict now sets WORKFLOW_HUMAN_INPUT_RESPONSES alongside the other five identifiers.
  • T9 pins omit-empty passthrough and that --human-input-responses is appended last, after every other optional flag, both alone and combined with all five siblings.
  • T10 proves injection-safety: a hostile value containing ;, `, and $(...) survives as a single argv token via set -- and never executes (sentinel-file check), the same guarantee T4/T8 establish for the other identifiers.

Verified independently:

  • human_input_responses is bound through env: (WORKFLOW_HUMAN_INPUT_RESPONSES → {{workflow.parameters.human_input_responses}}) and read as "$WORKFLOW_HUMAN_INPUT_RESPONSES", never interpolated directly into script text — scripts/validate-shell-param-interpolation.py's shell_blocks() only scans script.source/command/args, and this value never appears there, so no BASELINE entry is needed.
  • The set --/if [ -n ... ] append block matches the existing five-flag pattern exactly: default "", omit-when-empty, no invented value.
  • CI on this commit is green: Validate Manifests, yamllint, and the PR #1581 test-suite run (which includes this test file) all passed.

No further changes requested.

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Antigravity (agy) review — informational

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

No significant issues found.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No P1/P2 findings (0 P3). Commit afb2587 adds the test coverage (T9/T10) the prior review round asked for — omit-empty passthrough, flag ordering, and injection-safety for human_input_responses, mirroring T2/T4/T6/T8. Good to merge.

@mashkovd
mashkovd merged commit f388365 into main Oct 4, 2026
24 checks passed
@mashkovd
mashkovd deleted the feat/investigate-human-input-responses branch October 4, 2026 10:33
mashkovd added a commit to mctlhq/mctl-api that referenced this pull request Oct 4, 2026
cwftDeclaredParams transcribes cwft-mctl-agents-investigate, which
declares human_input_responses since mctlhq/mctl-gitops#1581.

Claude-Session: https://claude.ai/code/session_012kWiYf1BX6wcXB2kc8x15U
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants