fix: stop identity delete command printing internal sentinel error - #4591
fix: stop identity delete command printing internal sentinel error#4591klimavojtech2002 wants to merge 1 commit into
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe identity deletion command now uses positional ID help and reports errors for each failed identity. Tests cover unknown IDs, mixed deletion requests, partial deletion, output separation, and transport errors. ChangesIdentity deletion command
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/identities/delete.go`:
- Around line 46-54: Update the error handling in the identity deletion loop to
capture the error returned by cmdx.PrintOpenAPIError and print it unless
errors.Is(printErr, cmdx.ErrNoPrintButFail) is true, preserving transport and
timeout errors while retaining the existing failure tracking. Add a regression
test covering a transport error with no response body and verify that the
returned error is printed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 059a3107-9547-4bb0-96c4-c244df12d327
📒 Files selected for processing (2)
cmd/identities/delete.gocmd/identities/delete_test.go
| failed []string | ||
| ) | ||
|
|
||
| for _, a := range args { | ||
| _, err := c.IdentityAPI.DeleteIdentity(cmd.Context(), a).Execute() | ||
| if err != nil { | ||
| failed[a] = cmdx.PrintOpenAPIError(cmd, err) | ||
| _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Failed to delete identity %s:\n", a) | ||
| _ = cmdx.PrintOpenAPIError(cmd, err) | ||
| failed = append(failed, a) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f 'go\.(mod|sum)$' -x rg -n 'github.com/ory/x' {}
rg -n -C 12 'func PrintOpenAPIError|ErrNoPrintButFail' --glob '*.go' .Repository: ory/kratos
Length of output: 394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- delete command ---'
cat -n cmd/identities/delete.go | sed -n '1,110p'
printf '%s\n' '--- local cmdx implementation and sentinel ---'
rg -n -C 20 'func PrintOpenAPIError|ErrNoPrintButFail' oryx --glob '*.go'
printf '%s\n' '--- relevant tests and error handling ---'
rg -n -C 10 'DeleteIdentity|PrintOpenAPIError|ErrNoPrintButFail|Failed to delete identity' --glob '*_test.go' cmd oryxRepository: ory/kratos
Length of output: 13887
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- complete PrintOpenAPIError implementation ---'
cat -n oryx/cmdx/printing.go | sed -n '322,380p'
printf '%s\n' '--- complete delete command tests ---'
cat -n cmd/identities/delete_test.go | sed -n '1,180p'
printf '%s\n' '--- client transport error paths ---'
rg -n -C 12 'HTTPClient\.Do|\\.Do\\(' --glob '*.go' . | head -240
printf '%s\n' '--- command test setup and injectable client configuration ---'
rg -n -C 10 'func setup|RegisterClientFlags|HTTPClient|NewClient' cmd/identities cmd/cliclient --glob '*.go' | head -320Repository: ory/kratos
Length of output: 4405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- generated client files and transport calls ---'
fd -t f -i 'client.go' . -x sh -c 'printf "%s\n" "$1"; rg -n -C 8 "HTTPClient|\\.Do\\(" "$1" || true' sh {}
printf '%s\n' '--- command test setup ---'
rg -n -C 12 'func setup|RegisterClientFlags|NewClient|HTTPClient' cmd/identities cmd/cliclient --glob '*.go' || true
printf '%s\n' '--- read-only verifier for PrintOpenAPIError control flow ---'
python3 - <<'PY'
from pathlib import Path
p = Path("oryx/cmdx/printing.go")
s = p.read_text()
required = [
"if !errors.As(err, &be) {",
"return err",
"if didPrettyPrint {",
"return FailSilently(cmd)",
'json.MarshalIndent(json.RawMessage(body), "", " ")',
]
missing = [x for x in required if x not in s]
if missing:
raise SystemExit(f"missing source markers: {missing}")
no_body = s.index("if !errors.As(err, &be)")
no_body_return = s.index("return err", no_body)
pretty = s.index("if didPrettyPrint")
pretty_return = s.index("return FailSilently(cmd)", pretty)
marshal = s.index("json.MarshalIndent")
marshal_return = s.index("return FailSilently(cmd)", marshal)
assert no_body_return < pretty, "no-Body path must return before OpenAPI formatting"
assert pretty_return < marshal, "pretty-printed OpenAPI errors must fail silently"
assert marshal_return > marshal, "JSON-body path must fail silently after printing"
print("source control-flow checks passed")
print("no Body() => original error is returned")
print("recognized OpenAPI body => error is printed and ErrNoPrintButFail is returned")
PYRepository: ory/kratos
Length of output: 24249
Preserve non-OpenAPI errors.
cmdx.PrintOpenAPIError returns transport and timeout errors when the client receives no response body. The command discards these errors and returns only cmdx.ErrNoPrintButFail.
Print each returned error unless errors.Is(printErr, cmdx.ErrNoPrintButFail) is true. Add a transport-error regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/identities/delete.go` around lines 46 - 54, Update the error handling in
the identity deletion loop to capture the error returned by
cmdx.PrintOpenAPIError and print it unless errors.Is(printErr,
cmdx.ErrNoPrintButFail) is true, preserving transport and timeout errors while
retaining the existing failure tracking. Add a regression test covering a
transport error with no response body and verify that the returned error is
printed.
Source: MCP tools
There was a problem hiding this comment.
Good catch, thanks. Fixed: PrintOpenAPIError now has its return value checked against the ErrNoPrintButFail sentinel, and the original error is printed explicitly whenever it isn't that sentinel (i.e. transport-level errors that never had a body to pretty-print in the first place). Added a regression test that points the client at an unreachable endpoint to cover that path.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- delete command error handling ---'
sed -n '35,80p' cmd/identities/delete.go
printf '%s\n' '--- delete command test outline ---'
ast-grep outline cmd/identities/delete_test.go --items all --view expanded
printf '%s\n' '--- transport-error and error-output assertions ---'
rg -n -C 12 -i 'unreachable|transport|connection refused|PrintOpenAPIError|ErrNoPrintButFail|Failed to delete identity|never be printed' \
cmd/identities/delete_test.goLength of output: 5110
@klimavojtech2002, the implementation correctly preserves and prints non-sentinel errors.
The new transport-error test does not yet verify the transport error output. assert.NotEmpty passes when the command prints only Failed to delete identity <id>:.
Please assert a transport-specific value, such as 127.0.0.1:1, in stdErr. This assertion verifies that the error returned by cmdx.PrintOpenAPIError is printed.
You are interacting with an AI system.
0e11ad2 to
4719c4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/identities/delete_test.go`:
- Around line 97-103: Update the transport-error test around
unreachable.ExecExpectedErr to inject a custom http.Client via
cliclient.ClientContextKey, using a RoundTripper that returns a known error
instead of relying on http://127.0.0.1:1. Replace the weak non-empty assertion
with an assertion that the known underlying error is present in stdErr, while
retaining the check that the unexpected fixed failure header is absent.
- Around line 72-77: Update the delete command test around cmd.Exec to pass
unknownID before ids[0], ensuring deletion continues after the initial failure;
also assert that unknownID does not appear in stdOut while retaining the
existing stdout, stderr, and forbidden-error assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 61fdbc03-7fae-4b06-8f33-66143cd33fcd
📒 Files selected for processing (2)
cmd/identities/delete.gocmd/identities/delete_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/identities/delete.go
4719c4c to
edee21c
Compare
`ory identity delete` stored the return value of PrintOpenAPIError (the internal ErrNoPrintButFail sentinel) as the per-ID error, so PrintErrors later printed "<id>: this error should never be printed" right after the real, already-printed message. The loop now prints which ID failed alongside the real error message. PrintOpenAPIError only prints the message itself for API errors that carry a parsed response body, returning the sentinel in that case; for transport-level errors (no body, e.g. a timeout) it prints nothing and returns the original error instead, so that error is now printed explicitly rather than discarded. Failed IDs are tracked in a []string only to decide whether to return FailSilently. Also clarifies the confusing "id-0 [id-1] [id-2] [id-n]" usage line, which read like either a literal token or an index into `ory list identities`. Fixes ory#4588
edee21c to
1e8a7b1
Compare
2 participants
Related issue(s)
Fixes #4588
Summary
ory identity delete <id>stored the return value ofPrintOpenAPIError(the internalErrNoPrintButFailsentinel) as the per-ID error.PrintErrorsthen printed that sentinel's own message, "this error should never be printed", right after the real error had already been printed byPrintOpenAPIError. The loop now writes which ID failed alongside the real message and only tracks failed IDs to decide whether to returnFailSilently, so the sentinel is never surfaced.Also updates the confusing
identity id-0 [id-1] [id-2] [id-n]usage line, which read as either a literal token or an index intoory list identitiesoutput, neither of which is correct. It's nowidentity <id> [<id> ...].Checklist
Further Comments
Root cause and the two fix options were already diagnosed in the issue thread by a maintainer; this implements the smaller-diff option, plus including the failed ID in the message since
PrintOpenAPIErrordoesn't otherwise say which one failed.Summary by CodeRabbit