changelog: switch prepare-artifact bool CLI params to bool? (#3410)

* changelog: switch prepare-artifact bool CLI params to bool?

The `changelog prepare-artifact` command exposed `isFork`, `canCommit`,
and `maintainerCanModify` as plain `bool` parameters. The Argh CLI
framework treats `bool` as a presence-only switch — `--flag` sets true,
omission leaves false. There is no `--flag <value>` form.

When `elastic/docs-actions` shelled `--can-commit "$CAN_COMMIT"` into
this command (where `$CAN_COMMIT` was the string "true" or "false"),
the parser took the bare `--can-commit` as "true" and silently dropped
the literal "false" as a stray positional. Every fork PR ended up with
`can_commit: true` in the artifact metadata, which then routed the
apply step into the commit-and-push branch where it failed on a
detached-HEAD push.

This commit changes the three boolean parameters to `bool?` so:

- Argh now generates `--[no-]flag` pairs for each one. `--can-commit`
  sets true, `--no-can-commit` sets false, omission leaves null. The
  `--help` output makes this explicit.
- Callers that want to express "explicitly false" have a correct
  syntax to do so instead of the silently-broken `--flag false`.
- The service coerces `null` → `false` when writing
  `ChangelogArtifactMetadata`, so an omitted flag never grants
  commit permission to the downstream apply step (fail closed).

Behaviour summary at the CLI:

  Before               After
  -------------------  -------------------------
  --can-commit         --can-commit             → true
  (omitted)            --no-can-commit          → false
  --can-commit false   (omitted)                → false  (null → false)

The companion workflow fix in elastic/docs-actions#172 stops emitting
`--flag <value>` patterns in the first place and only appends the
affirmative flag when the upstream string equals "true".

Tests:

- New PrepareArtifact_NullableBoolsUnspecified_CoerceToFalseInMetadata
  asserts the fail-closed null → false coercion, which is the
  contract the apply step relies on.
- Existing fork-fields tests still cover the explicit true/false paths
  (bool → bool? is an implicit conversion at the call sites).

Verified with the AOT binary against a fixture staging dir:
`docs-builder changelog prepare-artifact --is-fork --no-can-commit
--maintainer-can-modify ...` writes
`is_fork: true, can_commit: false, maintainer_can_modify: true` as
expected.

Co-authored-by: Cursor <cursoragent@cursor.com>

* changelog: regenerate cli-schema.json after bool? switch

The previous commit moved the bool→bool? rationale into a regular
`//` comment block sitting between the XML `<param>` lines and the
method declaration. That terminated the XML doc comment group for the
schema source-generator, which then dropped every param summary for
this command and broke the `Check CLI schema is up to date` CI step.

- Inline the explanation into the existing `<remarks>` block as a
  `<para>`, restoring the unbroken XML doc group above the method.
- Regenerate `docs/cli-schema.json` so it reflects the bool? CLI
  surface: the three flags now serialize with the updated summaries
  and `defaultValue: "default"` (nullable default) instead of `"false"`.

No behaviour change beyond the previous commit; this just makes the
schema check pass again.

Co-authored-by: Cursor <cursoragent@cursor.com>

---------

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: cotti

docs/cli-schema.json

@@ -3394,7 +3394,7 @@
           ],
           "name": "prepare-artifact",
           "summary": "(CI) Package changelog artifact for cross-workflow transfer.",
-          "notes": "Resolves final status from evaluate-pr \u002B changelog add outcomes, copies generated YAML,\nwrites metadata.json, and sets GitHub Actions outputs. Always succeeds (exit 0) so the upload step runs.",
+          "notes": "Resolves final status from evaluate-pr \u002B changelog add outcomes, copies generated YAML,\nwrites metadata.json, and sets GitHub Actions outputs. Always succeeds (exit 0) so the upload step runs.\n\n\nThe isFork, canCommit and maintainerCanModify parameters are declared\nas bool? so the generated CLI emits both --flag and --no-flag pairs\n(Argh convention). A plain bool would expose presence-only switches: passing\n--can-commit \u0022false\u0022 would set canCommit = true (the flag is present) and\nsilently discard the literal \u0022false\u0022 as a stray positional. Callers that forward a\ndynamic value (--can-commit \u0022$VAR\u0022) would then misroute fork PRs into the\ncommit-and-push branch and die on a detached-HEAD push. See elastic/docs-actions#172\nfor the workflow-side fix.",
           "usage": "docs-builder changelog prepare-artifact --staging-dir \u003Cstring\u003E --output-dir \u003Cstring\u003E --evaluate-status \u003Cstring\u003E --generate-outcome \u003Cstring\u003E --pr-number \u003Cint\u003E --head-ref \u003Cstring\u003E --head-sha \u003Cstring\u003E [options]",
           "examples": [],
           "parameters": [
@@ -3452,24 +3452,24 @@
               "name": "is-fork",
               "type": "boolean",
               "required": false,
-              "summary": "Whether the PR is from a fork",
-              "defaultValue": "false"
+              "summary": "Whether the PR is from a fork (pass --is-fork / --no-is-fork; omit to leave null which is treated as false)",
+              "defaultValue": "default"
             },
             {
               "role": "flag",
               "name": "can-commit",
               "type": "boolean",
               "required": false,
-              "summary": "Whether the commit strategy allows committing",
-              "defaultValue": "false"
+              "summary": "Whether the commit strategy allows committing (pass --can-commit / --no-can-commit; omit to leave null which is treated as false)",
+              "defaultValue": "default"
             },
             {
               "role": "flag",
               "name": "maintainer-can-modify",
               "type": "boolean",
               "required": false,
-              "summary": "Whether the fork PR allows maintainer edits",
-              "defaultValue": "false"
+              "summary": "Whether the fork PR allows maintainer edits (pass --maintainer-can-modify / --no-maintainer-can-modify; omit to leave null which is treated as false)",
+              "defaultValue": "default"
             },
             {
               "role": "flag",

src/services/Elastic.Changelog/Evaluation/ChangelogPrepareArtifactService.cs

@@ -79,16 +79,20 @@ public async Task<bool> PrepareArtifact(IDiagnosticsCollector collector, Prepare
 		var changelogDir = config?.Bundle?.Directory ?? "docs/changelog";
 
 		var statusString = status.ToStringFast(true);
+		// Null-coalesce the nullable bool inputs into the metadata's concrete
+		// `bool` fields. Treating "unspecified" as `false` keeps downstream
+		// consumers (apply step) failing closed: an unrecognised or omitted
+		// CLI flag never grants commit permission.
 		var metadata = new ChangelogArtifactMetadata
 		{
 			PrNumber = input.PrNumber,
 			HeadRef = input.HeadRef,
 			HeadSha = input.HeadSha,
 			Status = statusString,
-			IsFork = input.IsFork,
+			IsFork = input.IsFork ?? false,
 			HeadRepo = input.HeadRepo,
-			CanCommit = input.CanCommit,
-			MaintainerCanModify = input.MaintainerCanModify,
+			CanCommit = input.CanCommit ?? false,
+			MaintainerCanModify = input.MaintainerCanModify ?? false,
 			LabelTable = input.LabelTable,
 			ProductLabelTable = input.ProductLabelTable,
 			SkipLabels = input.SkipLabels,

src/services/Elastic.Changelog/Evaluation/PrepareArtifactArguments.cs

@@ -14,10 +14,16 @@ public record PrepareArtifactArguments
 	public required int PrNumber { get; init; }
 	public required string HeadRef { get; init; }
 	public required string HeadSha { get; init; }
-	public bool IsFork { get; init; }
+	// Nullable bool mirrors the CLI surface: null = "flag not specified",
+	// normalized to false when serialized into ChangelogArtifactMetadata.
+	// Using non-nullable bool here would force every call site to choose
+	// between true/false and lose the "not specified" signal, which is what
+	// allowed --can-commit "false" to silently set CanCommit = true at the
+	// CLI before this change.
+	public bool? IsFork { get; init; }
 	public string? HeadRepo { get; init; }
-	public bool CanCommit { get; init; }
-	public bool MaintainerCanModify { get; init; }
+	public bool? CanCommit { get; init; }
+	public bool? MaintainerCanModify { get; init; }
 	public string? LabelTable { get; init; }
 	public string? ProductLabelTable { get; init; }
 	public string? SkipLabels { get; init; }

src/tooling/docs-builder/Commands/ChangelogCommand.cs

@@ -1401,6 +1401,17 @@ async static (s, collector, state, ctx) => await s.EvaluatePr(collector, state,
 	/// <remarks>
 	/// Resolves final status from evaluate-pr + changelog add outcomes, copies generated YAML,
 	/// writes metadata.json, and sets GitHub Actions outputs. Always succeeds (exit 0) so the upload step runs.
+	///
+	/// <para>
+	/// The <c>isFork</c>, <c>canCommit</c> and <c>maintainerCanModify</c> parameters are declared
+	/// as <c>bool?</c> so the generated CLI emits both <c>--flag</c> and <c>--no-flag</c> pairs
+	/// (Argh convention). A plain <c>bool</c> would expose presence-only switches: passing
+	/// <c>--can-commit "false"</c> would set <c>canCommit = true</c> (the flag is present) and
+	/// silently discard the literal <c>"false"</c> as a stray positional. Callers that forward a
+	/// dynamic value (<c>--can-commit "$VAR"</c>) would then misroute fork PRs into the
+	/// commit-and-push branch and die on a detached-HEAD push. See elastic/docs-actions#172
+	/// for the workflow-side fix.
+	/// </para>
 	/// </remarks>
 	/// <param name="stagingDir">Directory where changelog add wrote the generated YAML</param>
 	/// <param name="outputDir">Directory to write the artifact (metadata.json + YAML)</param>
@@ -1409,9 +1420,9 @@ async static (s, collector, state, ctx) => await s.EvaluatePr(collector, state,
 	/// <param name="prNumber">Pull request number</param>
 	/// <param name="headRef">PR head branch ref</param>
 	/// <param name="headSha">PR head commit SHA</param>
-	/// <param name="isFork">Whether the PR is from a fork</param>
-	/// <param name="canCommit">Whether the commit strategy allows committing</param>
-	/// <param name="maintainerCanModify">Whether the fork PR allows maintainer edits</param>
+	/// <param name="isFork">Whether the PR is from a fork (pass --is-fork / --no-is-fork; omit to leave null which is treated as false)</param>
+	/// <param name="canCommit">Whether the commit strategy allows committing (pass --can-commit / --no-can-commit; omit to leave null which is treated as false)</param>
+	/// <param name="maintainerCanModify">Whether the fork PR allows maintainer edits (pass --maintainer-can-modify / --no-maintainer-can-modify; omit to leave null which is treated as false)</param>
 	/// <param name="headRepo">Fork repository full name (owner/repo)</param>
 	/// <param name="labelTable">Optional: markdown label table from evaluate-pr</param>
 	/// <param name="productLabelTable">Optional: markdown product label table from evaluate-pr</param>
@@ -1427,9 +1438,9 @@ public async Task<int> PrepareArtifact(
 		int prNumber,
 		string headRef,
 		string headSha,
-		bool isFork = false,
-		bool canCommit = false,
-		bool maintainerCanModify = false,
+		bool? isFork = null,
+		bool? canCommit = null,
+		bool? maintainerCanModify = null,
 		string? headRepo = null,
 		string? labelTable = null,
 		string? productLabelTable = null,

tests/Elastic.Changelog.Tests/Evaluation/ChangelogPrepareArtifactServiceTests.cs

@@ -149,6 +149,34 @@ public async Task PrepareArtifact_ForkNoMaintainerEdits_CanCommitFalse()
 		metadata.MaintainerCanModify.Should().BeFalse();
 	}
 
+	// Regression for the docs-actions / docs-builder fork-PR detached-HEAD failure
+	// (see elastic/docs-actions#172). The bool? CLI parameters mean an *omitted*
+	// --can-commit flag arrives at the service as null. The service must coerce
+	// that to false in the persisted metadata so the downstream apply step does
+	// NOT attempt a commit+push. Failing closed on "unspecified" is the whole
+	// point of moving these flags to nullable bool.
+	[Fact]
+	public async Task PrepareArtifact_NullableBoolsUnspecified_CoerceToFalseInMetadata()
+	{
+		await SetupStagingYaml();
+		await SetupConfig();
+		var service = CreateService();
+		var args = DefaultArgs() with
+		{
+			IsFork = null,
+			CanCommit = null,
+			MaintainerCanModify = null,
+			HeadRepo = null
+		};
+
+		await service.PrepareArtifact(Collector, args, CancellationToken.None);
+
+		var metadata = ReadMetadata();
+		metadata.IsFork.Should().BeFalse("an omitted --is-fork flag must not be treated as 'fork'");
+		metadata.CanCommit.Should().BeFalse("an omitted --can-commit flag must never grant commit permission to the apply step");
+		metadata.MaintainerCanModify.Should().BeFalse("an omitted --maintainer-can-modify flag must not be treated as granted");
+	}
+
 	[Fact]
 	public async Task PrepareArtifact_ProductLabelTableAndSkipLabels_PersistedInMetadata()
 	{