Skip to content

fix: Hint at api.name config when 404 source mismatches CLI config#537

Open
kfelternv wants to merge 2 commits into
NVIDIA:mainfrom
kfelternv:nico-cli-api-name-404-hint
Open

fix: Hint at api.name config when 404 source mismatches CLI config#537
kfelternv wants to merge 2 commits into
NVIDIA:mainfrom
kfelternv:nico-cli-api-name-404-hint

Conversation

@kfelternv
Copy link
Copy Markdown
Contributor

@kfelternv kfelternv commented May 15, 2026

Based on a few bug reports, adding a small hint to direct users to the api.name configuration option would help debug 404s when using the cli with nico rest

@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented May 15, 2026

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 15, 2026

Review Change Stack

Walkthrough

This PR adds diagnostic guidance for API configuration mismatches. The APIError struct gains an optional Hint field that formats into error messages. A new helper detects 404 responses where the server-provided source differs from the client's configured API name and generates a remediation suggestion. The client wires this detection into its error path, and tests validate the logic across matching, mismatching, and edge cases.

Changes

API name mismatch hint feature

Layer / File(s) Summary
APIError type and error formatting
cli/pkg/client.go
APIError struct now exports optional Hint field; Error() method conditionally appends Hint: ... line to formatted error message when hint is non-empty.
API name mismatch hint detection and client integration
cli/pkg/client.go
New apiNameMismatchHint() helper detects 404 responses with source mismatch and returns configuration suggestion. Client's HTTP error path calls this helper and assigns the result to APIError.Hint before returning.
Test coverage for hint detection and error formatting
cli/pkg/client_test.go
Table-driven test for hint detection validates 404 vs. non-404 handling and matching/mismatching source values. Integration tests verify Client.Do propagates hints correctly and that APIError.Error() formatting includes Hint: section only when hint is non-empty.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description relates directly to the changeset, explaining the motivation for adding the api.name configuration hint for debugging 404 errors.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title directly addresses the core change: adding a helpful hint when a 404 response indicates an API name configuration mismatch.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@kfelternv kfelternv marked this pull request as ready for review May 15, 2026 19:10
@kfelternv kfelternv requested a review from a team as a code owner May 15, 2026 19:10
@github-actions
Copy link
Copy Markdown

🔐 TruffleHog Secret Scan

No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

🕐 Last updated: 2026-05-15 19:12:19 UTC | Commit: 23cabcd

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 15, 2026

🔍 Container Scan Summary

Service Total Critical High Medium Low Other
nico-flow 66 4 34 18 2 8
nico-nsm 82 2 28 43 9 0
nico-psm 67 4 35 18 2 8
nico-rest-api 100 6 53 30 3 8
nico-rest-cert-manager 65 4 34 18 1 8
nico-rest-db 66 4 34 18 2 8
nico-rest-site-agent 65 4 34 18 1 8
nico-rest-site-manager 65 4 34 18 1 8
nico-rest-workflow 67 4 35 18 2 8
TOTAL 643 36 321 199 23 64

Per-CVE detail lives in the per-service grype-* artifacts (JSON + SARIF). Severity counts only — no CVE IDs published here.

Copy link
Copy Markdown
Contributor

@thossain-nv thossain-nv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kfelternv Could this confuse those who are using /forge?

Copy link
Copy Markdown
Contributor

@thossain-nv thossain-nv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the API will return a json structure for 404 responses where the source field is set to the API name, we could just hint that?

@kfelternv kfelternv force-pushed the nico-cli-api-name-404-hint branch from 4bb126d to 4b0f580 Compare May 29, 2026 18:31
@kfelternv kfelternv changed the title fix: Hint at api.name config when 404 hits an API path segment mismatch fix: Hint at api.name config when 404 source mismatches CLI config May 29, 2026
Copy link
Copy Markdown
Contributor

@thossain-nv thossain-nv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the changes @kfelternv

@kfelternv kfelternv enabled auto-merge (squash) June 1, 2026 14:44
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