Skip to content

CLI: polish three rough edges from prod soft-delete shake-down#477

Merged
digaobarbosa merged 1 commit into
cli/follow-up-fixes-1.3.8from
claude/fix-broken-tests-isayE
May 9, 2026
Merged

CLI: polish three rough edges from prod soft-delete shake-down#477
digaobarbosa merged 1 commit into
cli/follow-up-fixes-1.3.8from
claude/fix-broken-tests-isayE

Conversation

@digaobarbosa
Copy link
Copy Markdown
Contributor

End-to-end testing v1.3.7 against prod surfaced three quality-of-life
issues — none correctness bugs, but they all lead users (and agents)
into wrong conclusions. All three fixed centrally so future trash /
destructive endpoints inherit the right behavior for free.

1. Auth errors now exit 2 (was: exit 3)

Per CONTRIBUTING.md: 0=success, 1=error, 2=auth, 3=not-found.
Every `rfapi.RoboflowError` from a trash endpoint was being caught
with `exit_code=3`, so a script couldn't tell "your key is bad,
re-auth" apart from "this resource doesn't exist." Reproducer:

\$ roboflow -k bogus-key trash list --json ; echo \$?
{"error": ...}
3                ← should be 2

Fix:

  • `RoboflowError(message, status_code=None)` now stores `status_code`
    on the instance (defaulted to None for back-compat with the 30+
    existing call sites that pass message-only).
  • `_raise_for_trash_response` stamps `response.status_code` on the
    raised exception so callers can branch on it.
  • New `output_api_error(args, exc, ...)` helper in `_output.py` does
    the central status→exit-code mapping (401→2, 404→3, else→1) plus
    optional `auth_hint` / `not_found_hint` overrides.
  • All four trash handlers (project/version/workflow/trash) replace
    hardcoded `output_error(..., exit_code=3)` with `output_api_error`.

2. `--json` no longer bypasses the destructive-action prompt

Old logic was `if not yes and not json: prompt`. That conflated
output formatting with destructive intent — anyone piping
`roboflow project delete X --json | jq` got their project nuked
without any confirmation because `--json` was treated as "I'm in
non-interactive context, skip the prompt." Reproducer:

\$ roboflow project delete moondream-example --json < /dev/null
{"deleted": true, "trash": true, ...}    ← deleted, no prompt!

Fix: new `confirm_destructive(args, prompt)` helper in `_output.py`
gates only on `--yes`/`-y` and TTY availability:

  • `--yes` set → proceed.
  • TTY without `--yes` → prompt (`--json` doesn't matter).
  • No TTY without `--yes` → bail with exit 1 and a hint pointing at
    `--yes` (would otherwise hang on `typer.confirm` reading a closed
    stdin, or — in some runtimes — silently default through).

`project delete`, `version delete`, `workflow delete` all use it.

3. Idempotent re-delete returns success, not "missing scope"

When a project/version/workflow is already in Trash, the public API's
URL filter excludes it from the active set, so a follow-up `DELETE`
returns 404 ("Unsupported request. `DELETE /ws/proj` does not
exist..."). The CLI was surfacing that as exit 3 with the
`Check your API key has 'project:update' scope` hint, which is
factually wrong — the user has the scope, the item is just already
where they wanted it.

Fix: each delete handler now intercepts a 404 from `rfapi.delete_X`,
probes `list_trash`, and if the slug/url is found in the appropriate
`sections` array, emits a synthetic success payload that mirrors the
shape of a real successful delete plus an `alreadyInTrash: true`
marker. If the slug is genuinely not in trash either, the original
404 propagates as exit 3 (so typos still surface as errors).

Sample payload from a re-delete after first move:

{
  "deleted": true,
  "type": "project",
  "workspace": "q3-board-meeting",
  "project": "moondream-example",
  "projectId": "Jjk3Tjq2yjxenyA9g5er",
  "trash": true,
  "alreadyInTrash": true
}

Tests

`tests/cli/test_trash_polish.py` adds 14 tests covering all three:

  • `output_api_error` mapping for 401 / 404 / 500 / no-status-code.
  • `_raise_for_trash_response` actually attaches `status_code`.
  • `confirm_destructive` honors `--yes`, refuses no-TTY without
    `--yes`, ignores `--json` when deciding (regression guard), and
    prompts via typer on a TTY (both accept and decline paths).
  • Idempotent re-delete success path on project / version / workflow,
    plus the negative case where a 404 with empty trash propagates as
    exit 3 (so we don't mask real "not found" errors).

All 526 existing tests still pass; ruff + mypy clean.

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

Same bug pattern fixed in commit 7c7037e for ImageUploadError /
AnnotationSaveError, missed in DeviceApiError because it lives in
devicesapi.py rather than rfapi.py.

After this PR moved `self.status_code = status_code` into the parent
RoboflowError.__init__, any subclass that set the attribute itself and
then called super().__init__(message) without forwarding status_code
got `None` written back over the value — every DeviceApiError raised
by _raise_for_status had status_code=None instead of the real HTTP
code, breaking the 7 status_code assertions in tests/test_device.py.

Forward status_code through to super(), matching the rfapi.py fix.
@digaobarbosa digaobarbosa changed the base branch from main to cli/follow-up-fixes-1.3.8 May 9, 2026 12:18
@digaobarbosa digaobarbosa merged commit 959ff3c into cli/follow-up-fixes-1.3.8 May 9, 2026
13 checks passed
digaobarbosa added a commit that referenced this pull request May 11, 2026
* CLI: polish three rough edges from prod soft-delete shake-down

End-to-end testing v1.3.7 against prod surfaced three quality-of-life
issues — none correctness bugs, but they all lead users (and agents)
into wrong conclusions. All three fixed centrally so future trash /
destructive endpoints inherit the right behavior for free.

### 1. Auth errors now exit 2 (was: exit 3)

Per CONTRIBUTING.md: 0=success, 1=error, 2=auth, 3=not-found.
Every \`rfapi.RoboflowError\` from a trash endpoint was being caught
with \`exit_code=3\`, so a script couldn't tell "your key is bad,
re-auth" apart from "this resource doesn't exist." Reproducer:

    \$ roboflow -k bogus-key trash list --json ; echo \$?
    {"error": ...}
    3                ← should be 2

Fix:
- \`RoboflowError(message, status_code=None)\` now stores \`status_code\`
  on the instance (defaulted to None for back-compat with the 30+
  existing call sites that pass message-only).
- \`_raise_for_trash_response\` stamps \`response.status_code\` on the
  raised exception so callers can branch on it.
- New \`output_api_error(args, exc, ...)\` helper in \`_output.py\` does
  the central status→exit-code mapping (401→2, 404→3, else→1) plus
  optional \`auth_hint\` / \`not_found_hint\` overrides.
- All four trash handlers (project/version/workflow/trash) replace
  hardcoded \`output_error(..., exit_code=3)\` with \`output_api_error\`.

### 2. \`--json\` no longer bypasses the destructive-action prompt

Old logic was \`if not yes and not json: prompt\`. That conflated
*output formatting* with *destructive intent* — anyone piping
\`roboflow project delete X --json | jq\` got their project nuked
without any confirmation because \`--json\` was treated as "I'm in
non-interactive context, skip the prompt." Reproducer:

    \$ roboflow project delete moondream-example --json < /dev/null
    {"deleted": true, "trash": true, ...}    ← deleted, no prompt!

Fix: new \`confirm_destructive(args, prompt)\` helper in \`_output.py\`
gates only on \`--yes\`/\`-y\` and TTY availability:
- \`--yes\` set → proceed.
- TTY without \`--yes\` → prompt (\`--json\` doesn't matter).
- No TTY without \`--yes\` → bail with exit 1 and a hint pointing at
  \`--yes\` (would otherwise hang on \`typer.confirm\` reading a closed
  stdin, or — in some runtimes — silently default through).

\`project delete\`, \`version delete\`, \`workflow delete\` all use it.

### 3. Idempotent re-delete returns success, not "missing scope"

When a project/version/workflow is already in Trash, the public API's
URL filter excludes it from the active set, so a follow-up \`DELETE\`
returns 404 ("Unsupported request. \`DELETE /ws/proj\` does not
exist..."). The CLI was surfacing that as exit 3 with the
\`Check your API key has 'project:update' scope\` hint, which is
factually wrong — the user has the scope, the item is just already
where they wanted it.

Fix: each delete handler now intercepts a 404 from \`rfapi.delete_X\`,
probes \`list_trash\`, and if the slug/url is found in the appropriate
\`sections\` array, emits a synthetic success payload that mirrors the
shape of a real successful delete plus an \`alreadyInTrash: true\`
marker. If the slug is genuinely not in trash either, the original
404 propagates as exit 3 (so typos still surface as errors).

Sample payload from a re-delete after first move:

    {
      "deleted": true,
      "type": "project",
      "workspace": "q3-board-meeting",
      "project": "moondream-example",
      "projectId": "Jjk3Tjq2yjxenyA9g5er",
      "trash": true,
      "alreadyInTrash": true
    }

### Tests

\`tests/cli/test_trash_polish.py\` adds 14 tests covering all three:
- \`output_api_error\` mapping for 401 / 404 / 500 / no-status-code.
- \`_raise_for_trash_response\` actually attaches \`status_code\`.
- \`confirm_destructive\` honors \`--yes\`, refuses no-TTY without
  \`--yes\`, ignores \`--json\` when deciding (regression guard), and
  prompts via typer on a TTY (both accept and decline paths).
- Idempotent re-delete success path on project / version / workflow,
  plus the negative case where a 404 with empty trash propagates as
  exit 3 (so we don't mask real "not found" errors).

All 526 existing tests still pass; ruff + mypy clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* rfapi: forward status_code through subclass __init__ to parent

Per review on #468 (codex bot + @digaobarbosa):
RoboflowError.__init__ now sets `self.status_code = status_code`. The
ImageUploadError / AnnotationSaveError subclasses were assigning
`self.status_code` themselves and then calling `super().__init__(message)`
without forwarding the second arg, so the parent silently overwrote the
HTTP code with None — losing useful error context for retry/auth handling
on every existing call site that raised
`ImageUploadError(..., status_code=...)` /
`AnnotationSaveError(..., status_code=...)`.

Forward `status_code` through to `super().__init__` so the subclasses
keep the HTTP code they were given. Add regression coverage in
test_trash_polish.py.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* devicesapi: forward status_code through DeviceApiError to parent (#477)

Same bug pattern fixed in commit 7c7037e for ImageUploadError /
AnnotationSaveError, missed in DeviceApiError because it lives in
devicesapi.py rather than rfapi.py.

After this PR moved `self.status_code = status_code` into the parent
RoboflowError.__init__, any subclass that set the attribute itself and
then called super().__init__(message) without forwarding status_code
got `None` written back over the value — every DeviceApiError raised
by _raise_for_status had status_code=None instead of the real HTTP
code, breaking the 7 status_code assertions in tests/test_device.py.

Forward status_code through to super(), matching the rfapi.py fix.

Co-authored-by: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Peter Robicheaux <peter@roboflow.com>
Co-authored-by: Rodrigo Barbosa <rodrigo@roboflow.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants