changelog: switch prepare-artifact bool CLI params to bool?#3410
Conversation
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>
|
Warning Review limit reached
More reviews will be available in 50 minutes and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements tri-state boolean handling for changelog artifact CLI parameters. The 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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>
1 participant
Summary
The
changelog prepare-artifactcommand exposedisFork,canCommit, andmaintainerCanModifyas plainboolparameters. The Argh CLI framework treatsboolas a presence-only switch —--flagsets true, omission leaves false. There is no--flag <value>form.When
elastic/docs-actionsshelled--can-commit \"$CAN_COMMIT\"into this command (where\$CAN_COMMITwas the literal string\"true\"or\"false\"), the parser took the bare--can-commitastrueand silently dropped the literal\"false\"as a stray positional. Every fork PR ended up withcan_commit: truein the artifact metadata, which then routed the apply step into the commit-and-push branch where it failed on a detached-HEAD push.See
elastic/elastic-otel-java#1117's submit run for the user-visible failure mode.Changes
Switching the three boolean parameters to
bool?so:--[no-]flagpairs for each one.--can-commitsetstrue,--no-can-commitsetsfalse, omission leavesnull. The--helpoutput makes this explicit:```
--[no-]is-fork
--[no-]can-commit
--[no-]maintainer-can-modify
```
--no-can-commit) instead of the silently-broken--flag false.null→falsewhen writingChangelogArtifactMetadata, so an omitted flag never grants commit permission to the downstream apply step (fail closed).Behaviour summary at the CLI
--can-committruetrue--no-can-commitfalsefalsefalse(null → false)--can-commit falsetrue(literal "false" dropped silently — the bug)true(literal "false" still dropped — Argh design, but now there's a correct alternative)Companion PR
The companion workflow fix in elastic/docs-actions#172 stops emitting
--flag <value>patterns in the first place — the action now only appends the affirmative flag when the upstream string equals\"true\". The two PRs together close the loop.Tests
PrepareArtifact_NullableBoolsUnspecified_CoerceToFalseInMetadataasserts the fail-closed null → false coercion, which is the contract the apply step relies on.PrepareArtifact_ForkFields_PersistedInMetadata,PrepareArtifact_ForkNoMaintainerEdits_CanCommitFalse) still cover the explicit true/false paths —bool→bool?is an implicit conversion at the call sites so they keep compiling unchanged.Test plan
dotnet format --verify-no-changesclean../build.sh build --skip-dirty-checksucceeds (lint + compile).dotnet publish src/tooling/docs-builder/docs-builder.csproj -c Releasesucceeds with 0 trimmer warnings (AOT path stays clean).docs-builder changelog prepare-artifact --helpnow shows--[no-]is-fork,--[no-]can-commit,--[no-]maintainer-can-modify.--is-fork --no-can-commit --maintainer-can-modifywritesis_fork: true, can_commit: false, maintainer_can_modify: true. Omitting--can-commitwritescan_commit: false.elastic-otel-javafork PR; should produce a comment-only changelog with no commit attempt.Made with Cursor