diff --git a/docs/cli-schema.json b/docs/cli-schema.json index bc6129da9..44e750515 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/services/Elastic.Changelog/Evaluation/ChangelogPrepareArtifactService.cs b/src/services/Elastic.Changelog/Evaluation/ChangelogPrepareArtifactService.cs index aa932859d..9fbd1c8cf 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 36348d37b..83739ee12 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 3c6a2c5f7..860bd2607 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) @@ -1409,9 +1420,9 @@ 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 @@ -1427,9 +1438,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 b084a2208..464bcaf16 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() {