fix: harden command and API handling - #2
Conversation
Keep authentication scoped to the Bitbucket API origin, handle nullable API data safely, stream downloads, preserve clean command output, and store credentials with safer file permissions. Add regression tests for each reviewed issue and propagate command cancellation throughout the CLI.
There was a problem hiding this comment.
Pull request overview
This PR hardens the CLI’s command boundary and Bitbucket API interactions by centralizing error handling, tightening credential behavior across redirects, improving streaming/encoding robustness, and adding regression tests to prevent regressions in script-safe output.
Changes:
- Centralize command failure handling at the program boundary (exceptions + consistent non-zero exit codes) and propagate cancellation through handlers.
- Harden API/IO behaviors: stream downloads, scope credentials to API origin across redirects, atomically persist credentials with strict file permissions, and safely escape endpoint path segments.
- Improve resilience to nullable/nested API JSON (including pipelines output shaping) and add regression test coverage for the above.
Reviewed changes
Copilot reviewed 64 out of 64 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Bbx.Tests/ProgramTests.cs | Adds regression coverage for program-boundary error handling and exit codes. |
| tests/Bbx.Tests/Features/Users/UserHandlersTests.cs | Updates user handler tests for revised auth/error behavior. |
| tests/Bbx.Tests/Features/Snippets/SnippetFilesHandlerTests.cs | Verifies per-segment path escaping for snippet file paths. |
| tests/Bbx.Tests/Features/Pipelines/PipelineFormatTests.cs | Validates pipeline formatting tolerates null nested members and preserves JSON shape. |
| tests/Bbx.Tests/Features/NullNestedObjectHandlerTests.cs | Ensures handlers tolerate null nested objects from the API. |
| tests/Bbx.Tests/Features/Downloads/GetDownloadHandlerTests.cs | Updates download handler tests for streaming-to-destination behavior. |
| tests/Bbx.Tests/Features/Auth/AuthTokenHandlerTests.cs | Updates auth token tests for exception-based user errors and return values. |
| tests/Bbx.Tests/Features/Auth/AuthStatusHandlerTests.cs | Updates auth status tests for exception-based failures and API error propagation. |
| tests/Bbx.Tests/Commands/ConfirmationOutputTests.cs | Ensures destructive prompts write to stderr (stdout remains script-safe). |
| tests/Bbx.Tests/Commands/CommandRunnerTests.cs | Updates expectations: errors now bubble to the program boundary. |
| tests/Bbx.Tests/Commands/CommandBindingTests.cs | Verifies System.CommandLine cancellation token reaches bound handlers. |
| tests/Bbx.Tests/Auth/FileCredentialStoreTests.cs | Adds tests for atomic credential writes and strict Unix permissions. |
| tests/Bbx.Tests/Api/BitbucketClientTests.cs | Adds redirect/auth-origin and streaming-copy coverage for the API client. |
| src/Bbx/Program.cs | Centralizes exception handling for the CLI and configures invocation behavior. |
| src/Bbx/Features/Workspaces/ViewWorkspace/ViewWorkspaceHandler.cs | Removes redundant credential checks; hardens nested JSON handling via TryGetObject. |
| src/Bbx/Features/Workspaces/ListWorkspaces/ListWorkspacesHandler.cs | Removes redundant credential checks and updates handler dependency shape. |
| src/Bbx/Features/Workspaces/ListWorkspacePermissions/ListWorkspacePermissionsHandler.cs | Uses TryGetObject for nullable nested JSON members. |
| src/Bbx/Features/Workspaces/ListWorkspaceMembers/ListWorkspaceMembersHandler.cs | Uses TryGetObject to avoid null-object pitfalls. |
| src/Bbx/Features/Users/ViewUser/ViewUserHandler.cs | Removes redundant credential checks and updates handler dependency shape. |
| src/Bbx/Features/Users/SshKeys/ViewSshKey/ViewSshKeyHandler.cs | Removes redundant credential checks and updates handler dependency shape. |
| src/Bbx/Features/Users/SshKeys/ListSshKeys/ListSshKeysHandler.cs | Removes redundant credential checks and updates handler dependency shape. |
| src/Bbx/Features/Users/SshKeys/DeleteSshKey/DeleteSshKeyHandler.cs | Removes redundant credential checks and updates handler dependency shape. |
| src/Bbx/Features/Users/SshKeys/AddSshKey/AddSshKeyHandler.cs | Removes redundant credential checks and updates handler dependency shape. |
| src/Bbx/Features/Users/ListUserWorkspacePermissions/ListUserWorkspacePermissionsHandler.cs | Removes redundant credential checks and updates handler dependency shape. |
| src/Bbx/Features/Users/ListUserRepositoryPermissions/ListUserRepositoryPermissionsHandler.cs | Removes redundant credential checks and updates handler dependency shape. |
| src/Bbx/Features/Users/ListUserEmails/ListUserEmailsHandler.cs | Removes redundant credential checks and updates handler dependency shape. |
| src/Bbx/Features/Source/LsSource/LsSourceHandler.cs | Centralizes safe endpoint path segment escaping. |
| src/Bbx/Features/Source/CatSource/CatSourceHandler.cs | Centralizes safe endpoint path segment escaping. |
| src/Bbx/Features/Snippets/ViewSnippet/ViewSnippetHandler.cs | Uses TryGetObject for nullable nested JSON members. |
| src/Bbx/Features/Snippets/UpdateSnippet/UpdateSnippetHandler.cs | Removes redundant credential checks. |
| src/Bbx/Features/Snippets/SnippetWatch/SnippetWatchHandler.cs | Removes redundant credential checks. |
| src/Bbx/Features/Snippets/SnippetFiles/SnippetFilesHandler.cs | Escapes per-segment file paths while preserving directories. |
| src/Bbx/Features/Snippets/SnippetComments/SnippetCommentsHandler.cs | Removes redundant credential checks. |
| src/Bbx/Features/Snippets/ListSnippets/ListSnippetsHandler.cs | Removes redundant credential checks and updates handler dependency shape. |
| src/Bbx/Features/Snippets/DeleteSnippet/DeleteSnippetHandler.cs | Removes redundant credential checks. |
| src/Bbx/Features/Snippets/CreateSnippet/CreateSnippetHandler.cs | Removes redundant credential checks. |
| src/Bbx/Features/Repos/RepoPermissions/RepoPermissionsHandler.cs | Avoids null nested-object access when shaping permissions output. |
| src/Bbx/Features/Pipelines/ViewPipeline/ViewPipelineHandler.cs | Hardens steps shaping against non-array/null values. |
| src/Bbx/Features/Pipelines/ViewDeploymentEnvironment/ViewDeploymentEnvironmentHandler.cs | Uses TryGetObject for nullable nested JSON members. |
| src/Bbx/Features/Pipelines/PipelineFormat.cs | Replaces dynamic shaping with typed records while preserving output shape. |
| src/Bbx/Features/Pipelines/ListPipelineVariables/ListPipelineVariablesHandler.cs | Safely reads boolean properties without throwing on null/non-bool. |
| src/Bbx/Features/Pipelines/ListPipelineSchedules/ListPipelineSchedulesHandler.cs | Uses TryGetObject for nullable nested JSON members. |
| src/Bbx/Features/Pipelines/ListDeploymentEnvironments/ListDeploymentEnvironmentsHandler.cs | Uses TryGetObject for nullable nested JSON members. |
| src/Bbx/Features/Downloads/GetDownload/GetDownloadHandler.cs | Switches download handling to streaming via a destination stream. |
| src/Bbx/Features/Common/EndpointPath.cs | Introduces shared helper for per-segment path escaping. |
| src/Bbx/Features/Commits/FileHistory/FileHistoryHandler.cs | Centralizes safe endpoint path segment escaping. |
| src/Bbx/Features/Auth/Token/AuthTokenHandler.cs | Returns token string and throws user errors instead of writing/setting exit code. |
| src/Bbx/Features/Auth/Status/AuthStatusHandler.cs | Returns status string and lets API failures propagate to the program boundary. |
| src/Bbx/Commands/WorkspaceCommand.cs | Propagates cancellation and standardizes handler wiring. |
| src/Bbx/Commands/UserCommand.cs | Propagates cancellation and standardizes handler wiring. |
| src/Bbx/Commands/SrcCommand.cs | Propagates cancellation and standardizes handler wiring. |
| src/Bbx/Commands/SnippetCommand.cs | Centralizes direct command error handling and propagates cancellation. |
| src/Bbx/Commands/RepoCommand.cs | Moves destructive prompts to stderr and propagates cancellation. |
| src/Bbx/Commands/PrCommand.cs | Propagates cancellation and standardizes handler wiring. |
| src/Bbx/Commands/PipelineCommand.cs | Propagates cancellation and standardizes handler wiring. |
| src/Bbx/Commands/IssueCommand.cs | Moves destructive prompts to stderr and propagates cancellation. |
| src/Bbx/Commands/DownloadCommand.cs | Streams downloads to file/stdout with safer temp-file handling and cancellation support. |
| src/Bbx/Commands/CommitCommand.cs | Propagates cancellation and standardizes handler wiring. |
| src/Bbx/Commands/CommandRunner.cs | Refactors runner helpers to rely on program-boundary exception handling and adds stream runner. |
| src/Bbx/Commands/CommandBinding.cs | Adds AsyncLocal cancellation token propagation into handlers. |
| src/Bbx/Commands/BranchCommand.cs | Moves destructive prompts to stderr and propagates cancellation. |
| src/Bbx/Commands/AuthCommand.cs | Switches auth commands to throw user errors and standardizes cancellation propagation. |
| src/Bbx/Auth/FileCredentialStore.cs | Makes credential load failures explicit; saves atomically with strict Unix permissions. |
| src/Bbx/Api/BitbucketClient.cs | Adds streaming CopyToAsync and tightens auth scoping across redirects/origins. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| exitCode = await parseResult.InvokeAsync(new InvocationConfiguration | ||
| { | ||
| EnableDefaultExceptionHandler = false, | ||
| }, default); |
There was a problem hiding this comment.
Half right, so I checked it against the library rather than the description. Fixed in 9b855c9.
Ctrl+C already reached handlers.InvocationPipeline.InvokeAsync does CancellationTokenSource.CreateLinkedTokenSource(cancellationToken) and gives the action cts.Token, then hands that same source to ProcessTerminationHandler, which registers for SIGINT and SIGTERM via PosixSignalRegistration on .NET 7+. So the token CommandBinding sees is cancelable even when default goes in, and a signal cancels it.
Verified against the built tool, Ctrl+C sent through a real pty mid-request:
baseline, no Ctrl+C: exit=0, 4988 bytes of JSON
Ctrl+C at 0.4s: exit=130, no output
I did not take the Console.CancelKeyPress suggestion. ProcessTerminationHandler only falls back to CancelKeyPress on platforms without PosixSignalRegistration; adding our own subscriber would be a second handler competing with the library's for the same signal.
Two things were wrong. First, a caller driving RunAsync had no way to cancel, since the token was hardcoded. It now takes an optional CancellationToken.
Second, and more visible: cancellation worked but did not exit cleanly. The OperationCanceledException fell through to the catch-all and printed Error: The operation was canceled. with exit 1. Confirmed on the pre-change build:
OLD build, Ctrl+C at 0.4s: exit=1, "Error: The operation was canceled."
NEW build, Ctrl+C at 0.4s: exit=130, no output
A cancelled run is not an error, so it now exits 130, the shell convention for SIGINT, and prints nothing. An HttpClient timeout also surfaces as an OperationCanceledException, but carries a TimeoutException inside, so the filter lets it through to the failure path and it still exits 1.
Tests.Cancelling_the_supplied_token_stops_the_command_before_it_calls_the_api fails on the old code with Expected exitCode to be 130, but found 0 and passes on the new. Bound_handlers_get_a_cancelable_token_even_with_no_token_supplied pins the library behaviour above, so anyone reading this thread and reaching for a signal hook gets a test explaining why not.
That test needed one test-kit fix: FakeHttpMessageHandler ignored the token. HttpClient passes the token straight to its handler without checking it first, so the fake was answering requests a real handler would have refused.
224 tests pass. Also exercised end to end with the packed tool: read-only commands against real repos, and the write and destructive paths (repo create, src write, branch create, pr create, pr decline, branch delete, repo delete) against a throwaway repo, since deleted.
Program.RunAsync hardcoded `default` as the token for InvokeAsync, so a caller driving RunAsync had no way to stop a command. It now takes an optional token and passes it through. Ctrl+C already reached handlers: System.CommandLine links whatever token it is given to a source of its own and cancels that from ProcessTerminationHandler. What it did not do was exit cleanly. The resulting OperationCanceledException fell to the catch-all and printed "Error: The operation was canceled." with exit 1. A cancelled run is not an error, so it now exits 130 and says nothing. An HttpClient timeout also surfaces as an OperationCanceledException but carries a TimeoutException inside, so it still reports as a failure with exit 1. FakeHttpMessageHandler now honours the token. HttpClient hands it to the handler without checking it first, so a fake that ignored it answered requests a real handler would have refused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
Summary
RunAsync.Cancellation, after review feedback
Copilot flagged
InvokeAsync(..., default)inProgram.cs, reading it asCtrl+C never reaching handlers. Ctrl+C did reach them:
InvocationPipelinelinks the token it is given to a source of its own and cancels it from
ProcessTerminationHandler, which registers for SIGINT and SIGTERM. Two realproblems sat next to it, both fixed in 9b855c9:
RunAsynccould not cancel, since the token was hardcoded.OperationCanceledExceptionfell to the catch-all and printed
Error: The operation was canceled.withexit 1. It now exits 130 and prints nothing. An HttpClient timeout carries a
TimeoutExceptioninside, so it still reports as a failure with exit 1.No
Console.CancelKeyPresshook was added; it would only compete with thelibrary's own signal registration.
Validation
line, new build exits 130 silently.
upload, streamed download to a file and stdout, and SHA-256 checks.
repo create,src write,branch create,pr create,pr decline,branch deleteandrepo delete.absence was verified.
Safety