Skip to content

fix(delete): fail when the deletion prompt cannot be answered - #1058

Merged
mbevc1 merged 2 commits into
mainfrom
20260729_delete_err
Jul 29, 2026
Merged

fix(delete): fail when the deletion prompt cannot be answered#1058
mbevc1 merged 2 commits into
mainfrom
20260729_delete_err

Conversation

@mbevc1

@mbevc1 mbevc1 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

kosli delete service-account and kosli delete api-key read the confirmation answer with bufio.Reader.ReadString and excluded io.EOF from the error path, so an EOF was indistinguishable from a typed refusal. When stdin is not interactive (any CI job, < /dev/null, a closed stdin) both commands printed the prompt, read EOF, reported "was cancelled." and returned nil - exit 0 without deleting anything. A pipeline that revokes credentials could not tell that the revocation had not happened, since the exit code is the only signal it consumes.

EOF with nothing read means there was nobody to ask, which is not a refusal: it is now an error naming --assume-yes, so the command exits non-zero. A typed "n", any other word, or a bare Enter still cancels with exit 0, and an answer with no trailing newline (printf 'y' | kosli delete) is still honoured - ReadString returns io.EOF together with the bytes read before it.

The EOF rule lives in one new shared helper, confirmDeletion, next to the other shared cmd helpers; the two per-command functions now only build their prompt. The test harness gained a stdin seam (cmdTestCase.stdin) so an answer can be typed at the prompt.

Fix requests and warn instead of exiting when payload logging fails. The debug-only payload log used Logger.Error, which is log.Fatalf and so exits 1, while the message it printed said "Continuing with the request...". The message and the behaviour disagreed

Fixes #1056

Checklist

@mbevc1 mbevc1 added go Pull requests that update go code fix labels Jul 29, 2026
@mbevc1
mbevc1 enabled auto-merge (squash) July 29, 2026 17:08
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mbevc1's task in 2m 18s —— View job


Review complete ✅

  • Gather context (read diff + changed files)
  • Static analysis (go build, go vet)
  • Review code quality, edge cases, bugs
  • Post review feedback

Summary

This is a well-scoped, well-tested fix. The core bug — EOF being indistinguishable from a typed refusal, so a non-interactive kosli delete exited 0 without deleting — is a real and dangerous silent failure for credential-revocation pipelines. The fix is correct and the reasoning is captured clearly in code comments.

Static analysis: go build ./... ✅ · go vet ./cmd/... ./internal/requests/...

What's done well

  • Right fix, right place. Extracting the EOF rule into a single shared confirmDeletion helper (cli_utils.go) and reducing the two per-command functions to prompt-builders removes the duplicated bug rather than patching it twice. I confirmed cli_utils.go:92 is now the only bufio.NewReader confirmation read in cmd/kosli, so there's no lingering copy of the old behavior.
  • Correct error propagation. run() returns the error from confirmApiKeyDeletion/confirmServiceAccountDeletion (deleteApiKey.go:91-93), so a non-interactive prompt now yields a non-zero exit — exactly the signal a pipeline needs.
  • The printf 'y' | kosli delete case is preserved. ReadString returns bytes with io.EOF, and the strings.TrimSpace(answer) == "" guard only errors on a genuinely empty read. This subtle-but-important path is explicitly covered by tests (serviceAccount_test.go, apiKey_test.go).
  • Excellent test coverage. The table-driven TestConfirmDeletion nails the tricky cases: EOF-with-nothing, blank-line-vs-blank-EOF, no-trailing-newline, case-insensitivity, and the error message asserting it names --assume-yes. The cmdTestCase.stdin seam (testHelpers.go) is a clean, reusable addition, and keeping the old executeCommandC as a thin wrapper avoids churn in existing tests.
  • The requests.go change is correct. Logger.Error is log.Fatalf (internal/logger/logger.go:64-67), so the old code aborted the request on a debug-only payload-log failure while printing "Continuing with the request…". Switching to Warn resolves the message/behaviour contradiction and matches the response-body handling directly below it (requests.go:285). It's correctly guarded by if c.Debug.
  • Help text updated on both commands to tell CI users to pass --assume-yes, keeping the autogenerated docs accurate.

Minor observations (non-blocking, no change requested)

  • The distinction between blank-then-EOF (printf ' ' → error) and blank-then-newline (printf ' \n' → refusal) is subtle, but it's the fail-safe choice (when unsure whether anyone answered, error rather than silently skip a destructive op) and it's thoroughly documented and tested. 👍
  • Logger.Warn respects QuietEnabled whereas the old Error/Fatalf did not, so under --debug combined with a quiet mode this warning could be suppressed. This is harmless (debug-only diagnostic) and consistent with the sibling Warn calls — noting only for completeness.

Nothing blocking here — clean fix, good tests, clear rationale. LGTM. 👍
· 20260729_delete_err

mbevc1 pushed a commit that referenced this pull request Jul 29, 2026
Review feedback on #1058: the two blank-looking inputs take different
exit paths, which was deliberate but undocumented. A blank line
terminated by a newline is a submitted answer and so a refusal (exit 0);
blank input terminated by EOF was never submitted and errors. Say so in
the doc comment and pin both in the table test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013uJHgA1TfGX9zNqe79DWRw
mbevc1 added 2 commits July 29, 2026 18:19
`kosli delete service-account` and `kosli delete api-key` read the
confirmation answer with bufio.Reader.ReadString and excluded io.EOF from
the error path, so an EOF was indistinguishable from a typed refusal. When
stdin is not interactive (any CI job, < /dev/null, a closed stdin) both
commands printed the prompt, read EOF, reported "was cancelled." and
returned nil - exit 0 without deleting anything. A pipeline that revokes
credentials could not tell that the revocation had not happened, since the
exit code is the only signal it consumes.

EOF with nothing read means there was nobody to ask, which is not a
refusal: it is now an error naming --assume-yes, so the command exits
non-zero. A typed "n", any other word, or a bare Enter still cancels with
exit 0, and an answer with no trailing newline (printf 'y' | kosli delete)
is still honoured - ReadString returns io.EOF together with the bytes read
before it.

The EOF rule lives in one new shared helper, confirmDeletion, next to the
other shared cmd helpers; the two per-command functions now only build
their prompt. The test harness gained a stdin seam (cmdTestCase.stdin) so
an answer can be typed at the prompt.

Fix requests and warn instead of exiting when payload logging fails.
The debug-only payload log used Logger.Error, which is log.Fatalf and so
exits 1, while the message it printed said "Continuing with the
request...". The message and the behaviour disagreed

Fixes #1056
Review feedback on #1058: the two blank-looking inputs take different
exit paths, which was deliberate but undocumented. A blank line
terminated by a newline is a submitted answer and so a refusal (exit 0);
blank input terminated by EOF was never submitted and errors. Say so in
the doc comment and pin both in the table test.
@mbevc1
mbevc1 force-pushed the 20260729_delete_err branch from 13bdb5d to c4ced51 Compare July 29, 2026 17:19
@mbevc1
mbevc1 merged commit 86c6eb3 into main Jul 29, 2026
16 checks passed
@mbevc1
mbevc1 deleted the 20260729_delete_err branch July 29, 2026 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix go Pull requests that update go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kosli delete service-account / delete api-key exit 0 without deleting when stdin is not interactive

2 participants