HYPERFLEET-1247 - fix: distinguish broken URL 404 from resource 404#211
Open
rafabene wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough404 handling now classifies RFC 9457 problem-details responses by error code and Sequence Diagram(s)sequenceDiagram
participant Executor
participant APIErrors
participant APIError
participant ExecutorState
Executor->>APIErrors: IsResourceNotFoundError(err)
APIErrors->>APIError: IsResourceNotFound()
APIError->>APIError: parseProblemDetails()
APIError-->>APIErrors: 404 + allowed code
APIErrors-->>Executor: true
Executor->>ExecutorState: skip or stop gracefully
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 1 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 344 lines (>200) | +1 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
rafabene
force-pushed
the
HYPERFLEET-1247-distinguish-precondition-404-from-force-delete
branch
from
e0d0094 to
6486532
Compare
![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
🧹 Nitpick comments (1)
internal/executor/executor_test.go (1)
1672-1707: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTests assert failure status but never prove the graceful branch was bypassed.
Both tests only check
StatusFailed/ non-empty errors. A regression that flips the trace_id gate could still leave the broken-endpoint path failing for an unrelated reason and these would pass. Pin the discriminator explicitly:♻️ Lock in that the broken-URL 404 is NOT treated as resource-not-found
assert.NotEmpty(t, result.Errors, "errors should be recorded for a broken URL 404") + assert.NotEqual(t, ResourceNotFoundReason, result.SkipReason, + "broken URL 404 must NOT be classified as resource-not-found")🤖 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 `@internal/executor/executor_test.go` around lines 1672 - 1707, The two tests in executor_test.go only assert failure, so they do not prove the broken-URL 404 path was routed away from the graceful resource-not-found handling. Update TestBrokenURL404_... and TestPostActionBrokenURL404_ReportsError to explicitly assert the error/result is not classified as the graceful not-found case (for example by checking the relevant error category or absence of the resource-not-found signal on result.Errors / PhasePostActions), using the existing helpers like new404PostActionConfig, mockBrokenEndpoint404Response, and exec.Execute to lock in the discriminator.
🤖 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 `@pkg/errors/api_error_test.go`:
- Around line 84-88: The table-driven assertions in api_error_test should
include descriptive failure messages instead of bare assert.Equal calls. Update
the affected tests that use t.Run(tt.name) so each assert.Equal includes tt.name
or a similar case-specific message, making it clear which expectation failed
without needing to cross-reference the table entry. Apply the same change to the
other shared assertion block noted in the review, using the existing test names
and error-checking helpers like IsResourceNotFound().
---
Nitpick comments:
In `@internal/executor/executor_test.go`:
- Around line 1672-1707: The two tests in executor_test.go only assert failure,
so they do not prove the broken-URL 404 path was routed away from the graceful
resource-not-found handling. Update TestBrokenURL404_... and
TestPostActionBrokenURL404_ReportsError to explicitly assert the error/result is
not classified as the graceful not-found case (for example by checking the
relevant error category or absence of the resource-not-found signal on
result.Errors / PhasePostActions), using the existing helpers like
new404PostActionConfig, mockBrokenEndpoint404Response, and exec.Execute to lock
in the discriminator.
🪄 Autofix (Beta)
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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 84938ea9-ec86-4db7-8f77-0613dcddd643
📒 Files selected for processing (7)
docs/adapter-authoring-guide.mdinternal/executor/executor.gointernal/executor/executor_test.gopkg/errors/api_error.gopkg/errors/api_error_test.gotest/testdata/dryrun/dryrun-404-api-responses.jsontest/testdata/dryrun/dryrun-post-action-404-api-responses.json
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
rafabene
changed the title
rafabene
force-pushed
the
HYPERFLEET-1247-distinguish-precondition-404-from-force-delete
branch
3 times, most recently
from
9f0d9e3 to
3c62abf
Compare
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 `@pkg/errors/api_error.go`:
- Around line 99-113: The IsResourceNotFound method in APIError is too strict
because it only returns true when parseProblemDetails succeeds and the RFC 9457
code matches, which misclassifies valid 404s when the body is missing or
rewritten. Update the logic so a plain IsNotFound() is treated as
resource-not-found by default, and only return false when you have a positive
signal that the endpoint itself is wrong; use the existing APIError,
parseProblemDetails, and resourceNotFoundCode symbols to adjust the fallback
behavior.
🪄 Autofix (Beta)
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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f11709df-6c93-4cd5-8f6c-c79903c2432c
📒 Files selected for processing (7)
docs/adapter-authoring-guide.mdinternal/executor/executor.gointernal/executor/executor_test.gopkg/errors/api_error.gopkg/errors/api_error_test.gotest/testdata/dryrun/dryrun-404-api-responses.jsontest/testdata/dryrun/dryrun-post-action-404-api-responses.json
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
✅ Files skipped from review due to trivial changes (1)
- test/testdata/dryrun/dryrun-404-api-responses.json
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/executor/executor.go
- test/testdata/dryrun/dryrun-post-action-404-api-responses.json
- internal/executor/executor_test.go
rafabene
force-pushed
the
HYPERFLEET-1247-distinguish-precondition-404-from-force-delete
branch
from
3c62abf to
4c33513
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/adapter-authoring-guide.md (1)
790-810: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the
trace_idgate here.This section still reduces the decision to the RFC 9457
code, but the new contract treats empty/missingtrace_idas the broken-URL case. Without that, a malformed 404 can still be documented as a graceful force-delete skip.Suggested wording
- - **Resource not found** (error code `HYPERFLEET-NTF-002` or `HYPERFLEET-NTF-003` in the RFC 9457 response body): the resource handler processed the request but the resource was deleted. The adapter handles this gracefully — resources are skipped and post-actions still execute. - - **Broken endpoint** (any other error code, e.g. `HYPERFLEET-NTF-001`): the precondition URL itself is misconfigured. The adapter treats this as an error and reports failure status. + - **Resource not found** (RFC 9457 body with `HYPERFLEET-NTF-002` or `HYPERFLEET-NTF-003` and a populated `trace_id`): the resource handler processed the request but the resource was deleted. The adapter handles this gracefully — resources are skipped and post-actions still execute. + - **Broken endpoint** (empty/missing `trace_id`, or any other error code such as `HYPERFLEET-NTF-001`): the precondition URL itself is misconfigured. The adapter treats this as an error and reports failure status.🤖 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 `@docs/adapter-authoring-guide.md` around lines 790 - 810, Restore the `trace_id` check in this 404 handling section so the graceful skip path only applies when the RFC 9457 response includes a valid trace identifier; an empty or missing `trace_id` must be documented as the broken-endpoint case even if the `code` looks like `HYPERFLEET-NTF-002` or `HYPERFLEET-NTF-003`. Update the wording around the adapter’s `resourcesSkipped` and `skipReason` behavior to make clear that `ResourceNotFound` is only set for legitimate force-delete responses with the expected `trace_id`, while malformed 404s still fail the precondition or post-action flow.Source: Linked repositories
🤖 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 `@docs/adapter-authoring-guide.md`:
- Line 1761: Broaden the Appendix D 404 guidance for the status-path check so it
covers nodepool lookups, not just the current HYPERFLEET-NTF-002 case. In the
table row describing post-action API calls and the status endpoints, update the
wording to allow both cluster and nodepool status failures by referencing the
status endpoint rules in the Appendix D section, especially the NodePool
statuses path, and make the error-code guidance consistent with
HYPERFLEET-NTF-003 as a valid nodepool-not-found outcome.
---
Outside diff comments:
In `@docs/adapter-authoring-guide.md`:
- Around line 790-810: Restore the `trace_id` check in this 404 handling section
so the graceful skip path only applies when the RFC 9457 response includes a
valid trace identifier; an empty or missing `trace_id` must be documented as the
broken-endpoint case even if the `code` looks like `HYPERFLEET-NTF-002` or
`HYPERFLEET-NTF-003`. Update the wording around the adapter’s `resourcesSkipped`
and `skipReason` behavior to make clear that `ResourceNotFound` is only set for
legitimate force-delete responses with the expected `trace_id`, while malformed
404s still fail the precondition or post-action flow.
🪄 Autofix (Beta)
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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7507b0d2-164a-4807-8273-62573cde289e
📒 Files selected for processing (7)
docs/adapter-authoring-guide.mdinternal/executor/executor.gointernal/executor/executor_test.gopkg/errors/api_error.gopkg/errors/api_error_test.gotest/testdata/dryrun/dryrun-404-api-responses.jsontest/testdata/dryrun/dryrun-post-action-404-api-responses.json
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (6)
- test/testdata/dryrun/dryrun-post-action-404-api-responses.json
- test/testdata/dryrun/dryrun-404-api-responses.json
- pkg/errors/api_error.go
- pkg/errors/api_error_test.go
- internal/executor/executor.go
- internal/executor/executor_test.go
Parse the RFC 9457 response body on 404 errors to check for a populated trace_id. A populated trace_id confirms the endpoint is valid and the resource was genuinely not found (force-delete), while an empty or absent trace_id indicates a misconfigured URL that should be reported as an error.
rafabene
force-pushed
the
HYPERFLEET-1247-distinguish-precondition-404-from-force-delete
branch
from
4c33513 to
9c0e4c5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
HYPERFLEET-NTF-000(catch-all, no route matched) is treated as a configuration errorIsResourceNotFound()/IsResourceNotFoundError()topkg/errors/api_error.goDependencies
Test plan
IsResourceNotFoundwith NTF-000/NTF-002/NTF-003/empty body/unparseable bodyIsResourceNotFoundErrorwith direct and wrapped errorsmake lintpasses with 0 issuesFixes: https://redhat.atlassian.net/browse/HYPERFLEET-1247