Add phase-aware CLI argument escape hatch

[thomhurst]

Summary

• add phase-aware AdditionalArguments for unmodeled CLI tokens
• support global placement before subcommands and semantic phase placement afterward
• document exact Arguments ordering and the generated-options escape hatch

Validation

• CommandLineBuilderTests: 16 passed
• ModularPipelines.slnx Release build: 0 warnings, 0 errors

Closes #3784

[claude]

Code review

Reviewed the phase-aware AdditionalArguments escape hatch (CommandLineBuilder.cs, AdditionalCommandLineArgument.cs, CommandLineToolOptions.cs, docs, and the new CommandLineBuilderTests). The phase-splitting logic itself (index bookkeeping across the EarlyOperand/Normal/Passthrough loop, the global-vs-command-specific split, the terminal-args assembly) checks out — I traced the result.Count + phaseIndex math and the two-call ref/out threading and didn't find an off-by-one or ordering bug there. Two things are worth addressing before merge:

1. A literal -- in AdditionalArguments silently bypasses terminator tracking (bug)

ModularPipelines/src/ModularPipelines/Context/CommandLineBuilder.cs

Lines 252 to 268 in 4823ee7

	ArgumentNullException.ThrowIfNull(argument.Value);
	if (argument is { IsGlobalOption: true, Phase: CommandLinePhase.Terminal })
	{
	throw new ArgumentException(
	"A terminal additional argument cannot be a global option.",
	nameof(CommandLineToolOptions.AdditionalArguments));
	}
	if (argument.Phase == LegacyEndOfOptionsPhase && argument.Value != "--")
	{
	throw new ArgumentException(
	"The legacy end-of-options phase only accepts the '--' marker.",
	nameof(CommandLineToolOptions.AdditionalArguments));
	}
	}

ValidateAdditionalArguments only constrains the -- value when argument.Phase == LegacyEndOfOptionsPhase. Nothing stops a caller from putting the literal string "--" into EarlyOperand, Normal, Passthrough, or Terminal. Look at BuildNonTerminalArguments:

ModularPipelines/src/ModularPipelines/Context/CommandLineBuilder.cs

Lines 188 to 223 in 4823ee7

	foreach (var phase in Enum.GetValues<CommandLinePhase>()
	.Where(static phase => phase != CommandLinePhase.Terminal))
	{
	var phaseAdditionalArguments = GetAdditionalArguments(
	additionalArguments,
	phase,
	isGlobalOption)
	.ToList();
	if (phase == LegacyEndOfOptionsPhase && phaseAdditionalArguments.Count > 0)
	{
	if (emittedOptionTerminator)
	{
	throw new InvalidOperationException(
	"An additional end-of-options marker cannot follow one that was already emitted.");
	}
	emittedOptionTerminatorIndex = result.Count;
	emittedOptionTerminator = true;
	}
	result.AddRange(phaseAdditionalArguments);
	var phaseModel = commandModel.Where(part => part.Phase == phase).ToList();
	var phaseArguments = _commandArgumentBuilder.BuildArguments(
	phaseModel,
	options,
	ref emittedOptionTerminator,
	out var phaseOptionTerminatorIndex);
	if (emittedOptionTerminatorIndex is null
	&& phaseOptionTerminatorIndex is { } phaseIndex)
	{
	emittedOptionTerminatorIndex = result.Count + phaseIndex;
	}
	result.AddRange(phaseArguments);
	}

Only the phase == LegacyEndOfOptionsPhase branch flips emittedOptionTerminator. For every other phase, phaseAdditionalArguments is appended via result.AddRange(...) with zero awareness that one of those tokens is a terminator.

Concrete failure: given the existing TestTerminalOptions fixture (which has a [CliOption(..., Phase = CommandLinePhase.Terminal)] property, RunTests), calling Build with AdditionalArguments = [new("--"), new("-1")] renders -- into the output but leaves emittedOptionTerminator == false. That means:

• the guard at the end of Build() ("Terminal options cannot be combined with arguments that emit or supply an end-of-options marker") never fires, and
• the terminal option gets appended after the literal -- in the actual rendered command line — which most CLI parsers treat as "everything after this is positional," so the terminal option is silently swallowed/misinterpreted by the underlying tool instead of being rejected or correctly ordered.

This is exactly the invariant the rest of the method exists to protect — compare how the pre-existing Arguments/ArgumentsContainOptionTerminator pair requires an explicit opt-in flag before a manual -- is trusted as a real terminator. AdditionalArguments has no equivalent safeguard for any phase besides the dedicated legacy one.

Suggested fix: in ValidateAdditionalArguments, reject a literal -- value for any phase other than LegacyEndOfOptionsPhase (the mirror image of the existing "legacy phase only accepts --" check), so users are forced through the one pathway that actually updates the terminator state.

2. New duplicate of the LegacyEndOfOptionsPhase workaround (architecture/DRY)

ModularPipelines/src/ModularPipelines/Context/CommandLineBuilder.cs

Lines 26 to 31 in 4823ee7

	ICommandArgumentBuilder commandArgumentBuilder) : ICommandLineBuilder
	{
	private const CommandLinePhase LegacyEndOfOptionsPhase = (CommandLinePhase) 2;
	private static readonly IReadOnlyList<PropertyCommandLinePart> RunSettingsCommandModel =
	[

This PR adds a second, textually identical copy of private const CommandLinePhase LegacyEndOfOptionsPhase = (CommandLinePhase) 2;. CommandArgumentBuilder already has the same constant:

ModularPipelines/src/ModularPipelines/Helpers/Internal/CommandArgumentBuilder.cs

Lines 12 to 16 in 4823ee7

	internal sealed class CommandArgumentBuilder : ICommandArgumentBuilder
	{
	private const CommandLinePhase LegacyEndOfOptionsPhase = (CommandLinePhase) 2;
	/// <inheritdoc/>

Both classes now independently special-case the same obsolete phase — ordering it between Normal and Passthrough, treating it as "only -- is a valid value," and toggling terminator state on it — with no shared source for either the constant or the associated rules. That's a real drift risk: if this ordinal or its semantics ever need to change, it's easy to update one copy and miss the other. Since this PR is already touching CommandLineBuilder.cs to add a second consumer of this concept, it'd be a good time to hoist the constant (and maybe the "is this the legacy end-of-options phase" predicate) into a small shared internal location — e.g. next to the CommandLinePhase enum itself — so both classes reference one definition instead of two.

---

Aside from those, the feature is well-scoped: the phase-aware placement, global/command-specific split, and validation rules (single legacy marker, terminal-can't-be-global, etc.) are consistent with the existing terminator-handling conventions in this file, the docs update clearly explains the new ordering, and the new tests cover the phase/scope placement cases well — they just don't yet cover a literal -- value outside the legacy phase, which is how issue #1 above slipped through.

[thomhurst]

Addressed both findings at 7c2f9d4: literal -- is now rejected in every non-legacy additional-argument phase, with regression coverage across EarlyOperand/Normal/Passthrough/Terminal; the legacy phase ordinal is centralized beside CommandLinePhase and shared by both builders. After merging current main: CommandLineBuilderTests 59/59, core Release build 0 warnings/errors. @claude review

[thomhurst]

Required Ubuntu pipeline failed on an unrelated timing-sensitive Flush_CancellationInterruptsRenderGateWait test while this PR's focused validation passed. The full workflow completed, so I triggered the one allowed failed-job rerun.

[thomhurst]

The single allowed rerun (attempt 2) failed in two different unrelated timing-sensitive core tests: SendAsync_CustomClientKeepsTimeoutOutsideLoggedReplayContent did not throw the expected OperationCanceledException, and StopOnFirstException_PendingModuleAwaiterReturnsTerminatedResult timed out instead of observing ModuleFailedException. Neither exercises the phase-aware CLI argument changes. No second rerun triggered.

[thomhurst]

The single failed-job rerun also failed on the prior base, so I merged current main at 952ca4d to trigger a fresh full workflow. CommandLineBuilderTests pass 59/59 after the refresh.

[claude]

Code review (re-review)

Re-reviewed after the fixes pushed at 7c2f9d4 / 952ca4d5e0. Both issues from the prior review are verifiably resolved:

1. Literal -- outside the legacy phase — ValidateAdditionalArguments (CommandLineBuilder.cs:260-274) now rejects any AdditionalCommandLineArgument whose Value == "--" unless Phase == CommandLinePhaseCompatibility.LegacyEndOfOptions, and the reverse (legacy phase requires the value to be --). The new Build_Rejects_Additional_Terminator_Outside_Legacy_Phase test exercises all four non-legacy phases.
2. Duplicated LegacyEndOfOptionsPhase constant — hoisted into CommandLinePhaseCompatibility.LegacyEndOfOptions in Attributes/CommandLinePhase.cs:37-40, and both CommandLineBuilder.cs and CommandArgumentBuilder.cs now reference the single shared constant instead of each declaring their own copy.

Additional verification this pass

I traced a scenario the fix doesn't explicitly test: combining the new AdditionalArguments legacy -- with an existing legacy-phase model property (the kind emitted by older generated packages, e.g. the TestLegacyEndOfOptionsOptions fixture's [CliFlag("--", Phase = (CommandLinePhase)2)]). Worth checking because that's a second, independent source of a literal -- that isn't covered by the "at most one legacy marker in AdditionalArguments" count check. Traced it through BuildNonTerminalArguments → CommandArgumentBuilder.BuildArguments → ValidateOptionTerminatorOrdering: because BuildNonTerminalArguments sets emittedOptionTerminator = true from the additional argument before calling into CommandArgumentBuilder.BuildArguments for that phase, ValidateOptionTerminatorOrdering's pre-existing guard (optionTerminatorAlreadyEmitted && renderedOptions.Length > 0 → throw) correctly rejects the combination rather than silently emitting two -- tokens. No gap here — the existing terminator-ordering validation generalizes correctly to the new argument source.

Design note (non-blocking)

CommandLineBuilder.cs was already large before this PR and is now ~820 lines mixing several concerns: phase-aware argument assembly, manual-option extraction/matching (short-option cluster parsing, attached-value parsing, etc.), and terminator validation. This PR's addition (BuildNonTerminalArguments, ValidateAdditionalArguments, GetAdditionalArguments) is consistent with the file's existing static-method style, so it's not something to fix in this PR — but if AdditionalArguments grows more validation rules over time, pulling the ValidateAdditionalArguments/GetAdditionalArguments pair into a small dedicated internal helper (mirroring how ICommandArgumentBuilder is already split out) would keep CommandLineBuilder focused on orchestration rather than also owning validation-rule details for each argument source. Just a maintainability observation, not required for merge.

Verdict

The phase-aware AdditionalArguments escape hatch is well-scoped, the terminator-safety invariants are consistently enforced across all the ways -- can now enter the command line (structured arguments, manual Arguments, and now AdditionalArguments), and the new tests cover the placement/scope/rejection cases well. No blocking issues found. CI is currently re-running on the latest merge commit (952ca4d) — worth confirming green before merge, but nothing here should require further code changes.

[thomhurst]

CI diagnosis: Ubuntu failed one unrelated core timing test out of 1,904: EngineCancellationTokenTests.StopOnFirstException_PendingModuleAwaiterReturnsTerminatedResult timed out after 20s. This PR only changes command-line argument placement. Run attempt is 1; GitHub will not accept the one allowed failed-job rerun until the active macOS sibling finishes, so the queue loop will retry then.

[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]

@claude review

Please re-review the exact current head. Report blocking/actionable findings only. If none exist, state plainly that there are no blocking or actionable findings; omit optional design notes and future-maintainability suggestions.

[thomhurst]

@claude Please provide a concise current-head merge verdict. The previous review found no blocking issues; please omit non-actionable design-note category headings because the repository merge gate conservatively parses those headings as actionable. No code changes were made for that explicitly non-blocking note.

[thomhurst]

Refreshed with current main at be99f8e. Merge was clean. Post-refresh validation: CommandLineBuilderTests 59/59, lightweight core Release build 0 warnings/errors. @claude Please review current head and provide a concise merge verdict without restating previously noted non-blocking design observations.

[claude]

Re-reviewed at current head (be99f8eca7). No blocking or actionable findings.

The PR-relevant files (CommandLineBuilder.cs, CommandArgumentBuilder.cs, CommandLinePhase.cs, AdditionalCommandLineArgument.cs, CommandLineToolOptions.cs, the new tests, and the docs) are byte-for-byte identical to 952ca4d5e0, which was already reviewed and cleared — the merges since then (952ca4d5e0 → be99f8eca7) only pulled in unrelated main changes. Both previously-flagged issues (literal -- bypassing terminator tracking outside the legacy phase; the duplicated LegacyEndOfOptionsPhase constant) remain fixed as verified last pass.

Merge verdict: approve on code content. CI on the latest merge commit is still pending at review time — confirm green before merging.

[thomhurst]

Refreshed onto current main after the prior Ubuntu core-suite run hung for 16 minutes and exited 2/134 without a failing-test summary. Validation on 6b4dd63: CommandLineBuilderTests 59/59, ModularPipelines.slnx Release build 0 warnings/errors, and OptionsGenerator solution Release build 0 warnings/errors.

[claude]

Code review

Re-reviewed at current head (6b4dd639b8). No blocking or actionable findings.

Confirmed via diff that all PR-relevant files (CommandLineBuilder.cs, CommandArgumentBuilder.cs, CommandLinePhase.cs, AdditionalCommandLineArgument.cs, CommandLineToolOptions.cs, CommandLineBuilderTests.cs, the docs, and the OptionsGenerator csproj) are byte-for-byte identical to be99f8eca7, which was already reviewed and approved on code content — the merges since then only pulled in unrelated main changes (Directory.Packages.props, docs/docs/_sidebar.md, FileSystemModuleCache.cs/tests).

Merge verdict: approve on code content. CI on the latest merge commit is still in progress at review time — confirm green before merging.