Preserve manual argument and terminator ordering

[thomhurst]

Summary

• keep structured pass-through operands before ordinary manual Arguments
• hoist recognized manual tool options safely while preserving explicit option/terminator ordering
• reuse manual terminators for terminal arguments and reject conflicting RunSettings

Test plan

• CommandLineBuilderTests (57 passed)
• CliAttributeTests (60 passed)
• ModularPipelines.Jq.UnitTests (18 passed)
• ModularPipelines.Rust.UnitTests (3 passed)
• ModularPipelines.DotNet.UnitTests (18 passed, 2 unrelated skips)
• core Release build (0 warnings, 0 errors)

Closes #3776

[thomhurst]

@claude review

[thomhurst]

CodeFactor’s complex-method finding is addressed by extracting the RunSettings validation and manual/pass-through ordering decisions. Focused command-builder tests remain 56/56. @claude review

[claude]

Code review

Reviewed the CLAUDE.md compliance and correctness of this change. No CLAUDE.md violations, but I found real logic regressions in CommandLineBuilder.cs that were validated by actually running the affected test suites — two are confirmed test failures, not speculative.

1. Manual Arguments are hoisted ahead of all pass-through operands, not just ones behind a -- (confirmed, reproduced)

ModularPipelines/src/ModularPipelines/Context/CommandLineBuilder.cs

Lines 205 to 220 in 62dea51

	private static void AddManualAndPassthroughArguments(
	List<string> allArgs,
	IReadOnlyCollection<string> manualArgs,
	IReadOnlyCollection<string> passthroughArgs,
	CommandLineToolOptions options)
	{
	if (!options.ArgumentsContainToolOptions && !options.ArgumentsContainOptionTerminator)
	{
	allArgs.AddRange(manualArgs);
	allArgs.AddRange(passthroughArgs);
	return;
	}
	allArgs.AddRange(passthroughArgs);
	allArgs.AddRange(manualArgs);
	}

AddManualAndPassthroughArguments only checks ArgumentsContainToolOptions / ArgumentsContainOptionTerminator — it never checks whether the pass-through group actually emitted a --. Since every [CliArgument] defaults to CommandLinePhase.Passthrough, this hoists manual args in front of ordinary positional operands too, even when no terminator is emitted at all.

Reproduced by running test/ModularPipelines.Jq.UnitTests:

failed Preserves_Manual_Options_After_Ordinary_Filter
  Expected to be equal to "jq . --compact-output input.json"
  but received "jq --compact-output input.json ."

input.json is now interpreted by jq as the filter and . as an input file — a real behavioral regression, and this existing test was not updated by the PR (only CommandLineBuilderTests/CliAttributeTests were run per the PR description). Suggest gating the hoist on passthroughOptionTerminatorIndex is not null (i.e., a terminator was actually emitted) rather than on the negation of the two Arguments* flags.

2. CargoOptionsTests.Preserves_PassThrough_Switch_After_Test_Arguments now fails (confirmed, reproduced)

ModularPipelines/test/ModularPipelines.Rust.UnitTests/CargoOptionsTests.cs

Lines 9 to 22 in 62dea51

	[Test]
	public async Task Preserves_PassThrough_Switch_After_Test_Arguments()
	{
	var builder = await GetService<ICommandLineBuilder>();
	var commandLine = builder.Build(new CargoTestOptions
	{
	Args = ["filter"],
	Arguments = ["--help"],
	});
	await Assert.That(commandLine.ToString())
	.IsEqualTo("cargo test -- filter --help");
	}

Same root cause as #1. This existing test (untouched by the PR, not in its diff) asserts "cargo test -- filter --help"; running it now produces "cargo test --help -- filter":

failed Preserves_PassThrough_Switch_After_Test_Arguments
  Expected to be equal to "cargo test -- filter --help"
  but received "cargo test --help -- filter"

The PR's stated test plan only covers CommandLineBuilderTests/CliAttributeTests, so this break in the Rust tool tests wasn't caught locally. Worth also spot-checking ModularPipelines.Pulumi.UnitTests and the other ModularPipelines.Jq.UnitTests cases with -- orderings (lines 142/158/226/242/258) for the same issue before merging.

3. Terminal-argument terminator state no longer folds in ArgumentsContainOptionTerminator (low severity, currently unreachable in-repo)

ModularPipelines/src/ModularPipelines/Context/CommandLineBuilder.cs

Lines 100 to 104 in 62dea51

	var terminalArgumentArgs = _commandArgumentBuilder.BuildArguments(
	[.. terminalCommandModel.Where(static part => part is ArgumentPart)],
	options,
	ref emittedOptionTerminator,
	out var terminalArgumentOptionTerminatorIndex);

terminalArgumentArgs is now built against the model-only emittedOptionTerminator, whereas previously the equivalent flag also incorporated options.ArgumentsContainOptionTerminator. For a hypothetical Phase = Terminal [CliArgument] combined with manual Arguments that already contain -- (e.g. Arguments = ["--", "input"], ArgumentsContainOptionTerminator = true, plus a terminal argument), the terminal argument now independently emits its own -- and the subsequent ValidateTerminatorState check throws, where previously this rendered correctly (e.g. jq -- input -x).

No in-repo generated options class currently declares Phase = Terminal on a CliArgument (the one Terminal-phase usage in JqExecuteOptions.Generated.cs is a CliOption, not a CliArgument), so this isn't reachable today except via test fixtures or a downstream consumer's own options type. Flagging as low-priority since it falls out of the same "structured terminator is exclusive" policy this PR intentionally applies elsewhere (e.g. the new ValidateRunSettingsTerminator), but it's an undocumented/untested side effect worth a test or a deliberate call-out if intended.

---

Recommend fixing #1/#2 before merge since they're confirmed regressions in existing, currently-passing tests.

[thomhurst]

Addressed current-head CodeFactor finding at 1a4f5db88c: CommandLineBuilder.Build now delegates tool resolution, command-model partitioning, manual-option extraction/insertion, and terminal-option validation to focused helpers without changing rendering behavior. Validation: CommandLineBuilderTests 56/56; changed-file whitespace clean; core Release build 0 warnings/errors. @claude review

[claude]

Code review (round 2, at 1a4f5db88c)

Re-verified against the CHANGES_REQUESTED review left on the previous commit (62dea51714). That review flagged two confirmed test regressions and one low-severity gap. This commit is a pure refactor addressing CodeFactor's complexity finding (extracting AddManualAndPassthroughArguments, GetManualOptionInsertionIndex, ValidateRunSettingsTerminator, etc.) — it does not touch the actual logic that caused the regressions. I re-ran the affected suites against current head and both confirmed regressions are still present, unchanged.

1. Still broken: manual Arguments hoisted ahead of all pass-through operands (confirmed, reproduced again)

ModularPipelines/src/ModularPipelines/Context/CommandLineBuilder.cs

Lines 264 to 279 in 1a4f5db

	private static void AddManualAndPassthroughArguments(
	List<string> allArgs,
	IReadOnlyCollection<string> manualArgs,
	IReadOnlyCollection<string> passthroughArgs,
	CommandLineToolOptions options)
	{
	if (!options.ArgumentsContainToolOptions && !options.ArgumentsContainOptionTerminator)
	{
	allArgs.AddRange(manualArgs);
	allArgs.AddRange(passthroughArgs);
	return;
	}
	allArgs.AddRange(passthroughArgs);
	allArgs.AddRange(manualArgs);
	}

private static void AddManualAndPassthroughArguments(...)
{
    if (!options.ArgumentsContainToolOptions && !options.ArgumentsContainOptionTerminator)
    {
        allArgs.AddRange(manualArgs);
        allArgs.AddRange(passthroughArgs);
        return;
    }

    allArgs.AddRange(passthroughArgs);
    allArgs.AddRange(manualArgs);
}

This is the exact method flagged last round, moved verbatim into its own function. It still gates on the two Arguments* config flags instead of on whether the passthrough group actually emitted a terminator — a signal the method already has available one call site up as passthroughOptionTerminatorIndex.

Reproduced today on this commit:

dotnet run --project test/ModularPipelines.Jq.UnitTests --framework net10.0 -- --treenode-filter /*/*/*/Preserves_Manual_Options_After_Ordinary_Filter
  failed Preserves_Manual_Options_After_Ordinary_Filter
  Expected to be equal to "jq . --compact-output input.json"
  but received "jq --compact-output input.json ."

(1 of 18 in the full ModularPipelines.Jq.UnitTests run.)

2. Still broken: CargoOptionsTests.Preserves_PassThrough_Switch_After_Test_Arguments (confirmed, reproduced again)

ModularPipelines/test/ModularPipelines.Rust.UnitTests/CargoOptionsTests.cs

Lines 9 to 22 in 1a4f5db

	[Test]
	public async Task Preserves_PassThrough_Switch_After_Test_Arguments()
	{
	var builder = await GetService<ICommandLineBuilder>();
	var commandLine = builder.Build(new CargoTestOptions
	{
	Args = ["filter"],
	Arguments = ["--help"],
	});
	await Assert.That(commandLine.ToString())
	.IsEqualTo("cargo test -- filter --help");
	}

Same root cause as #1:

dotnet run --project test/ModularPipelines.Rust.UnitTests --framework net10.0 -- --treenode-filter /*/*/*/Preserves_PassThrough_Switch_After_Test_Arguments
  failed Preserves_PassThrough_Switch_After_Test_Arguments
  Expected to be equal to "cargo test -- filter --help"
  but received "cargo test --help -- filter"

(1 of 3 in the full ModularPipelines.Rust.UnitTests run.)

I couldn't get a clean run of ModularPipelines.Pulumi.UnitTests locally (hit the agent guard's 2 GB process-tree limit), so that suite is still unverified either way — worth a spot-check before merge as the earlier review suggested.

Suggested fix (same as last round)

Gate the hoist on the actual emitted-terminator signal rather than the config flags:

private static void AddManualAndPassthroughArguments(
    List<string> allArgs,
    IReadOnlyCollection<string> manualArgs,
    IReadOnlyCollection<string> passthroughArgs,
    bool passthroughEmittedOptionTerminator)
{
    if (!passthroughEmittedOptionTerminator)
    {
        allArgs.AddRange(manualArgs);
        allArgs.AddRange(passthroughArgs);
        return;
    }

    allArgs.AddRange(passthroughArgs);
    allArgs.AddRange(manualArgs);
}

passing passthroughOptionTerminatorIndex is not null (or the broader modelEmittedOptionTerminator, if a terminator from an earlier phase should also trigger the hoist) at the call site. This is more than a style nit: the config flags (ArgumentsContainToolOptions / ArgumentsContainOptionTerminator) describe what the manual arguments contain, not whether the passthrough operand actually rendered a --. Using them as a proxy for the passthrough group's terminator state is what caused both regressions — every [CliArgument] defaults to CommandLinePhase.Passthrough, so ordinary filters/positional operands get treated as if a terminator boundary exists even when none was rendered. Tying the branch directly to the terminator index removes the mismatch and is also simpler than what's there now.

Minor: issue #3 from last round unchanged

terminalArgumentArgs (

ModularPipelines/src/ModularPipelines/Context/CommandLineBuilder.cs

Lines 86 to 91 in 1a4f5db

	var terminalArgumentArgs = _commandArgumentBuilder.BuildArguments(
	[.. terminalCommandModel.Where(static part => part is ArgumentPart)],
	options,
	ref emittedOptionTerminator,
	out var terminalArgumentOptionTerminatorIndex);
	var modelEmittedOptionTerminator = emittedOptionTerminator;

) still only folds in the model-only emittedOptionTerminator, not options.ArgumentsContainOptionTerminator. Still low-priority/unreachable in-repo today (no generated options class declares Phase = Terminal on a CliArgument), noting again for completeness rather than blocking.

---

The CommandLineBuilderTests (56/56) and CliAttributeTests (60/60) claims check out — I ran CommandLineBuilderTests myself and got 56/56 — but those suites don't exercise the code path that's actually broken, which is why the regression keeps surviving refactors. Recommend adding a CommandLineBuilderTests case (or running the Jq/Rust suites) that covers "ordinary passthrough argument, no terminator emitted, manual Arguments present" so this stops slipping through the stated test plan.

Recommend fixing #1/#2 before merge — same recommendation as last round, now reconfirmed against the latest commit.

[thomhurst]

Addressed the requested changes at 4a3ba67:

• ordinary positional/pass-through operands keep their established ordering, fixing the reproduced Jq regression
• default Cargo pass-through arguments remain after the structured marker
• recognized manual tool options still hoist before a structured marker
• a declared manual marker is now reused by Terminal arguments instead of emitting a duplicate marker

Validation: CommandLineBuilderTests 57/57, JqOptionsTests 17/17, CargoOptionsTests 2/2, new Terminal-marker regression 1/1. The Pulumi spot-check exceeded the local 2 GB agent guard, so CI will cover it.

@claude review

[claude]

Code review (round 3, at 4a3ba6750)

Re-verified against the two CHANGES_REQUESTED rounds on this PR. Both previously confirmed, reproduced regressions are now fixed, and I re-ran the actual failing tests (not just read the diff) to confirm.

Previously confirmed issues — now fixed

#1 (manual Arguments hoisted ahead of all pass-through operands) and #2 (CargoOptionsTests.Preserves_PassThrough_Switch_After_Test_Arguments) were both traced to AddManualAndPassthroughArguments gating the ordering decision on options.ArgumentsContainToolOptions / ArgumentsContainOptionTerminator (config flags describing the manual input) instead of whether the passthrough group actually emitted a terminator. This commit removes that conditional entirely — passthroughArgs now unconditionally precede manualArgs:

private static void AddManualAndPassthroughArguments(
    List<string> allArgs,
    IReadOnlyCollection<string> manualArgs,
    IReadOnlyCollection<string> passthroughArgs)
{
    allArgs.AddRange(passthroughArgs);
    allArgs.AddRange(manualArgs);
}

Recognized manual tool options are still extracted/hoisted separately earlier in Build, so this simplification is sound rather than just moving the bug around. Confirmed via actual test runs at 4a3ba6750 (not just re-reading the code, since the same diff-reading mistake is what let this regression survive round 1 → round 2):

• ModularPipelines.Jq.UnitTests: 18/18 passed, including Preserves_Manual_Options_After_Ordinary_Filter (previously reproduced as failing).
• ModularPipelines.Rust.UnitTests: 3/3 passed, including Preserves_PassThrough_Switch_After_Test_Arguments (previously reproduced as failing).
• test/ModularPipelines.UnitTests CommandLineBuilderTests: 57/57 passed.
• Spot-checked ModularPipelines.DotNet.UnitTests (a large, heavily-used tool integration not called out in either prior review) as an extra sanity check on the shared CommandLineBuilder path: 18/18 passed (2 pre-existing, unrelated skips).
• ModularPipelines.Pulumi.UnitTests still can't be run locally — it exceeds the agent guard's 2 GB process-tree limit, same as both previous rounds. Still worth a CI spot-check, but this is an environment limitation, not something this review can hold the PR on.

#3 (low-severity: terminal-argument terminator state didn't fold in ArgumentsContainOptionTerminator) is also fixed — terminalArgumentTerminatorState now seeds from emittedOptionTerminator || options.ArgumentsContainOptionTerminator before building terminal arguments, and the new regression test Build_Manual_Terminator_Is_Reused_For_Terminal_Argument (Arguments = ["--", "input"], ArgumentsContainOptionTerminator = true, TerminalArgument = "-x" → "jq -- input -x") locks it in and passes.

CodeFactor's complexity finding from round 2 is also resolved (CodeFactor: pass — No issues found on current head), and the extraction into ResolveTool, GetCommandModels, ExtractManualOptions, GetManualOptionInsertionIndex, ValidateTerminalOptions, ValidateRunSettingsTerminator reads cleanly — each does one thing and Build reads as an ordered pipeline of phases.

Minor: PR description is now stale

The PR description still reads "place ordinary manual Arguments before structured pass-through operands," which described the original (buggy) round-1 approach. The shipped behavior in this commit is the opposite — pass-through operands now unconditionally precede ordinary manual arguments, with only recognized tool options hoisted ahead of them. Worth updating the description before merge so the PR's stated intent matches what actually landed; otherwise a future reader digging through history (e.g. someone debugging a similar ordering issue later) will be misled about what "fixed" actually means here.

Architectural note (non-blocking, for future consideration)

Both regressions in this PR's history trace back to the same root cause: Build threads a single mutable ref bool emittedOptionTerminator (and its derived cousins — modelEmittedOptionTerminator, pendingTerminatorState, hasOptionTerminator, terminalArgumentTerminatorState) through 5+ sequential BuildArguments calls, where each call's local contribution to "did a terminator get emitted" has to be manually disentangled from the running total. That's exactly the class of bug that caused round 1: reusing one flag's value where a different phase's flag was needed. It's fixed correctly here, but the design still requires the same careful discipline to avoid a third recurrence the next time a phase is added or reordered.

If this file sees more churn, consider having each phase return an immutable (Args, EmittedTerminator) result and reducing over an ordered list of phases, rather than accumulating derived booleans by hand. That would make "which phases contribute to the running terminator state" an explicit, checkable list instead of implicit knowledge encoded in variable-naming discipline — the kind of change that would have caught this PR's round-1/round-2 bugs at compile-review time rather than requiring three rounds of test-driven discovery. Not asking for that rewrite in this PR — just flagging it since the same shape of bug has now shown up twice here.

---

Recommendation: the two blocking regressions from prior rounds are fixed and verified by running the actual previously-failing tests. No new blocking issues found. Consider updating the PR description before merge for accuracy.

Superseded by fixes at 4a3ba67 and the bot round-3 re-review confirming both regressions resolved.

[thomhurst]

Required pipeline attempt 1 ended in the known core-suite hang/exit 2 with no deterministic assertion or compile error. Requested the single failed-job rerun.

[thomhurst]

Single allowed rerun diagnosis: attempt 2 ran 24m48s, then RunCoreUnitTestsModule exited 7/hung with no deterministic assertion; every other unit-test module shown before cancellation passed, and Windows/macOS/analyzers/AOT/Fast fail are green. This is the known overloaded core-suite failure, not a command-ordering regression. No second rerun.

[thomhurst]

Refreshed onto current main at 6fab2c0 after the prior full-suite watchdog hang. Current-head validation: CommandLineBuilderTests 57/57; core Release build 0 warnings / 0 errors. Please re-review current head.

[thomhurst]

Refreshed again onto current main after Ubuntu's core suite ran 15m59s and exited 2/134 without a failing-test summary. Validation on d643ad2: CommandLineBuilderTests 57/57 and ModularPipelines.slnx Release build 0 warnings/errors.