From 03e3fe9cabcc4dce0237aea80e650bbf37e566be Mon Sep 17 00:00:00 2001 From: Felipe Cotti Date: Wed, 27 May 2026 15:25:34 -0300 Subject: [PATCH 1/2] changelog: switch prepare-artifact bool CLI params to bool? MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ` 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 ` 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 --- .../ChangelogPrepareArtifactService.cs | 10 +++++-- .../Evaluation/PrepareArtifactArguments.cs | 12 ++++++-- .../docs-builder/Commands/ChangelogCommand.cs | 20 +++++++++---- .../ChangelogPrepareArtifactServiceTests.cs | 28 +++++++++++++++++++ 4 files changed, 58 insertions(+), 12 deletions(-) diff --git a/src/services/Elastic.Changelog/Evaluation/ChangelogPrepareArtifactService.cs b/src/services/Elastic.Changelog/Evaluation/ChangelogPrepareArtifactService.cs index aa932859dd..9fbd1c8cff 100644 --- a/src/services/Elastic.Changelog/Evaluation/ChangelogPrepareArtifactService.cs +++ b/src/services/Elastic.Changelog/Evaluation/ChangelogPrepareArtifactService.cs @@ -79,16 +79,20 @@ public async Task 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, diff --git a/src/services/Elastic.Changelog/Evaluation/PrepareArtifactArguments.cs b/src/services/Elastic.Changelog/Evaluation/PrepareArtifactArguments.cs index 36348d37b4..83739ee128 100644 --- a/src/services/Elastic.Changelog/Evaluation/PrepareArtifactArguments.cs +++ b/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; } diff --git a/src/tooling/docs-builder/Commands/ChangelogCommand.cs b/src/tooling/docs-builder/Commands/ChangelogCommand.cs index 3c6a2c5f70..e1a72efff4 100644 --- a/src/tooling/docs-builder/Commands/ChangelogCommand.cs +++ b/src/tooling/docs-builder/Commands/ChangelogCommand.cs @@ -1409,15 +1409,23 @@ async static (s, collector, state, ctx) => await s.EvaluatePr(collector, state, /// Pull request number /// PR head branch ref /// PR head commit SHA - /// Whether the PR is from a fork - /// Whether the commit strategy allows committing - /// Whether the fork PR allows maintainer edits + /// Whether the PR is from a fork (pass --is-fork / --no-is-fork; omit to leave null which is treated as false) + /// Whether the commit strategy allows committing (pass --can-commit / --no-can-commit; omit to leave null which is treated as false) + /// Whether the fork PR allows maintainer edits (pass --maintainer-can-modify / --no-maintainer-can-modify; omit to leave null which is treated as false) /// Fork repository full name (owner/repo) /// Optional: markdown label table from evaluate-pr /// Optional: markdown product label table from evaluate-pr /// Optional: comma-separated skip labels from evaluate-pr /// Optional: path to changelog.yml /// Optional: filename of a previously committed changelog for this PR + // `isFork`, `canCommit`, `maintainerCanModify` are declared as `bool?` so the + // generated CLI emits both `--flag` and `--no-flag` pairs (Argh convention). + // A plain `bool` parameter would expose presence-only switches: `--can-commit + // "false"` would set `canCommit = true` (the flag is present) and silently + // discard the literal "false" as a stray positional. Callers that pass a + // dynamic value (`--can-commit "$VAR"`) 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. [NoOptionsInjection] public async Task PrepareArtifact( string stagingDir, @@ -1427,9 +1435,9 @@ public async Task 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, diff --git a/tests/Elastic.Changelog.Tests/Evaluation/ChangelogPrepareArtifactServiceTests.cs b/tests/Elastic.Changelog.Tests/Evaluation/ChangelogPrepareArtifactServiceTests.cs index b084a2208a..464bcaf16c 100644 --- a/tests/Elastic.Changelog.Tests/Evaluation/ChangelogPrepareArtifactServiceTests.cs +++ b/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() { From 09650bf4505670d5565fe4a40799a8bf287881f0 Mon Sep 17 00:00:00 2001 From: Felipe Cotti Date: Wed, 27 May 2026 15:35:37 -0300 Subject: [PATCH 2/2] changelog: regenerate cli-schema.json after bool? switch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit moved the bool→bool? rationale into a regular `//` comment block sitting between the XML `` 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 `` block as a ``, 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 --- docs/cli-schema.json | 14 +++++++------- .../docs-builder/Commands/ChangelogCommand.cs | 19 +++++++++++-------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/docs/cli-schema.json b/docs/cli-schema.json index bc6129da99..44e750515d 100644 --- a/docs/cli-schema.json +++ b/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", diff --git a/src/tooling/docs-builder/Commands/ChangelogCommand.cs b/src/tooling/docs-builder/Commands/ChangelogCommand.cs index e1a72efff4..860bd26074 100644 --- a/src/tooling/docs-builder/Commands/ChangelogCommand.cs +++ b/src/tooling/docs-builder/Commands/ChangelogCommand.cs @@ -1401,6 +1401,17 @@ async static (s, collector, state, ctx) => await s.EvaluatePr(collector, state, /// /// 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. + /// + /// + /// The isFork, canCommit and maintainerCanModify parameters are declared + /// as bool? so the generated CLI emits both --flag and --no-flag pairs + /// (Argh convention). A plain bool would expose presence-only switches: passing + /// --can-commit "false" would set canCommit = true (the flag is present) and + /// silently discard the literal "false" as a stray positional. Callers that forward a + /// dynamic value (--can-commit "$VAR") 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. + /// /// /// Directory where changelog add wrote the generated YAML /// Directory to write the artifact (metadata.json + YAML) @@ -1418,14 +1429,6 @@ async static (s, collector, state, ctx) => await s.EvaluatePr(collector, state, /// Optional: comma-separated skip labels from evaluate-pr /// Optional: path to changelog.yml /// Optional: filename of a previously committed changelog for this PR - // `isFork`, `canCommit`, `maintainerCanModify` are declared as `bool?` so the - // generated CLI emits both `--flag` and `--no-flag` pairs (Argh convention). - // A plain `bool` parameter would expose presence-only switches: `--can-commit - // "false"` would set `canCommit = true` (the flag is present) and silently - // discard the literal "false" as a stray positional. Callers that pass a - // dynamic value (`--can-commit "$VAR"`) 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. [NoOptionsInjection] public async Task PrepareArtifact( string stagingDir,