Execute shortcuts from server catalog#17
Merged
Conversation
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 30 minutes and 56 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
WalkthroughServer ChangesCatalog-기반 동적 명령 실행
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
akcorca
force-pushed
the
codex/dynamic-server-shortcuts
branch
from
631d815 to
fdb2828
Compare
akcorca
force-pushed
the
codex/dynamic-server-shortcuts
branch
from
fdb2828 to
fd35e68
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: 3
🤖 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 `@internal/craken/catalog_command.go`:
- Around line 441-458: runCatalogDownloadCommand currently calls client.raw with
an empty requestSpec and only the path, so catalog query parameters and header
overrides (e.g., Accept) are ignored; update runCatalogDownloadCommand to build
and pass a populated requestSpec (including QueryParams from the route/catalog
binding and Headers such as an overridden "Accept" from cmd.string or the route)
to client.raw instead of requestSpec{} so the download request includes the
catalog's query and header overrides (locate changes around
runCatalogDownloadCommand, client.raw call, requestSpec construction, and where
cmd.string("output", "") is read).
- Around line 86-105: The mergeExecution function currently replaces
base.PathParams, base.QueryParams, and base.BodyFields when variant provides
them, which drops existing base bindings (e.g., workspaceId); instead, when
variant.PathParams/QueryParams/BodyFields are non-nil, merge their key/value
pairs into the corresponding base maps (creating the base map if nil) so variant
values override specific keys but other base entries remain; keep the existing
behavior for scalar fields (OperationID, Transport, Output) but update the
map-handling logic in mergeExecution to perform an in-place merge rather than
whole-map replacement.
- Around line 495-501: The optionText function currently returns an unused error
which triggers golangci-lint unparam; change the signature from func
optionText(cmd command, names []string) (string, error) to func optionText(cmd
command, names []string) string, have it return the found value or "" (keeping
the same loop and return ""), and then update all callers of optionText to stop
expecting an error (remove error variables / checks) and use the returned string
directly; references to command.string remain unchanged inside the function.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 944c8bc6-cd6d-4029-8a5a-54c6ac2fd7e9
📒 Files selected for processing (7)
docs/architecture.mdinternal/craken/catalog_command.gointernal/craken/command.gointernal/craken/command_test.gointernal/craken/generic.gointernal/craken/resources.gointernal/craken/wiki_agent.go
💤 Files with no reviewable changes (1)
- internal/craken/wiki_agent.go
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: CI / 0_test.txt
internal/craken/catalog_command.go
[error] 495-495: golangci-lint reported: optionText - result 1 (error) is always nil (unparam)
🪛 GitHub Actions: CI / test
internal/craken/catalog_command.go
[error] 495-495: golangci-lint: optionText - result 1 (error) is always nil (unparam)
🪛 GitHub Check: test
internal/craken/catalog_command.go
[failure] 495-495:
optionText - result 1 (error) is always nil (unparam)
🔇 Additional comments (1)
docs/architecture.md (1)
5-5: LGTM!Also applies to: 9-9
Comment on lines
+86
to
+105
| func mergeExecution(base commandExecution, variant commandExecutionVariant) commandExecution { | ||
| if variant.OperationID != "" { | ||
| base.OperationID = variant.OperationID | ||
| } | ||
| if variant.Transport != "" { | ||
| base.Transport = variant.Transport | ||
| } | ||
| if variant.Output != "" { | ||
| base.Output = variant.Output | ||
| } | ||
| if variant.PathParams != nil { | ||
| base.PathParams = variant.PathParams | ||
| } | ||
| if variant.QueryParams != nil { | ||
| base.QueryParams = variant.QueryParams | ||
| } | ||
| if variant.BodyFields != nil { | ||
| base.BodyFields = variant.BodyFields | ||
| } | ||
| return base |
There was a problem hiding this comment.
variant 바인딩을 통째로 덮어쓰면 기본 실행 계획이 사라집니다.
여기서는 variant의 PathParams/QueryParams/BodyFields가 있을 때 base 맵을 병합하지 않고 전체 교체합니다. 그래서 variant가 일부 필드만 오버라이드해도 base에 있던 workspaceId 같은 공통 바인딩이 사라져 실행이 깨집니다.
예시 수정
func mergeExecution(base commandExecution, variant commandExecutionVariant) commandExecution {
if variant.OperationID != "" {
base.OperationID = variant.OperationID
}
if variant.Transport != "" {
base.Transport = variant.Transport
}
if variant.Output != "" {
base.Output = variant.Output
}
- if variant.PathParams != nil {
- base.PathParams = variant.PathParams
- }
- if variant.QueryParams != nil {
- base.QueryParams = variant.QueryParams
- }
- if variant.BodyFields != nil {
- base.BodyFields = variant.BodyFields
- }
+ base.PathParams = mergeCommandBindings(base.PathParams, variant.PathParams)
+ base.QueryParams = mergeCommandBindings(base.QueryParams, variant.QueryParams)
+ base.BodyFields = mergeCommandBindings(base.BodyFields, variant.BodyFields)
return base
}
+
+func mergeCommandBindings(base, override map[string]commandBinding) map[string]commandBinding {
+ if len(base) == 0 && len(override) == 0 {
+ return nil
+ }
+ out := make(map[string]commandBinding, len(base)+len(override))
+ for key, value := range base {
+ out[key] = value
+ }
+ for key, value := range override {
+ out[key] = value
+ }
+ return out
+}🤖 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/craken/catalog_command.go` around lines 86 - 105, The mergeExecution
function currently replaces base.PathParams, base.QueryParams, and
base.BodyFields when variant provides them, which drops existing base bindings
(e.g., workspaceId); instead, when variant.PathParams/QueryParams/BodyFields are
non-nil, merge their key/value pairs into the corresponding base maps (creating
the base map if nil) so variant values override specific keys but other base
entries remain; keep the existing behavior for scalar fields (OperationID,
Transport, Output) but update the map-handling logic in mergeExecution to
perform an in-place merge rather than whole-map replacement.
Comment on lines
+441
to
+458
| func runCatalogDownloadCommand(ctx context.Context, client *client, route route, cmd command, path string, stdout io.Writer) error { | ||
| response, err := client.raw(ctx, route.Method, path, requestSpec{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer func() { _ = response.Body.Close() }() | ||
| bytes, err := io.ReadAll(response.Body) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if response.StatusCode < 200 || response.StatusCode >= 300 { | ||
| return fmt.Errorf("%s %s failed with %d: %s", route.Method, path, response.StatusCode, string(bytes)) | ||
| } | ||
| if output := cmd.string("output", ""); output != "" { | ||
| return os.WriteFile(filepath.Clean(output), bytes, 0o600) | ||
| } | ||
| _, err = stdout.Write(bytes) | ||
| return err |
There was a problem hiding this comment.
download 요청이 catalog query/header 바인딩을 전혀 반영하지 않습니다.
이 경로는 client.raw를 빈 requestSpec와 순수 path로 호출해서, catalog의 queryParams와 --accept 같은 헤더 오버라이드가 모두 무시됩니다. 다운로드 명령이 버전/포맷 쿼리에 의존하면 잘못된 리소스를 받게 됩니다.
예시 수정 방향
-func runCatalogDownloadCommand(ctx context.Context, client *client, route route, cmd command, path string, stdout io.Writer) error {
- response, err := client.raw(ctx, route.Method, path, requestSpec{})
+func runCatalogDownloadCommand(
+ ctx context.Context,
+ client *client,
+ route route,
+ plan commandExecution,
+ cmd command,
+ path string,
+ resolved map[string]string,
+ consumed map[string]bool,
+ stdout io.Writer,
+) error {
+ values, err := catalogValues(ctx, client, cmd, plan.QueryParams, resolved, consumed)
+ if err != nil {
+ return err
+ }
+ if plan.QueryParams == nil {
+ values = requestValuesFromOptions(cmd, consumed)
+ }
+ response, err := client.raw(ctx, route.Method, appendQuery(path, values), requestSpec{
+ Headers: requestHeadersFromOptions(cmd),
+ })
if err != nil {
return err
}🤖 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/craken/catalog_command.go` around lines 441 - 458,
runCatalogDownloadCommand currently calls client.raw with an empty requestSpec
and only the path, so catalog query parameters and header overrides (e.g.,
Accept) are ignored; update runCatalogDownloadCommand to build and pass a
populated requestSpec (including QueryParams from the route/catalog binding and
Headers such as an overridden "Accept" from cmd.string or the route) to
client.raw instead of requestSpec{} so the download request includes the
catalog's query and header overrides (locate changes around
runCatalogDownloadCommand, client.raw call, requestSpec construction, and where
cmd.string("output", "") is read).
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\n- dispatch product shortcuts through the server-owned /api/client command catalog\n- add a catalog execution engine for HTTP, multipart, download, websocket, and shaped output commands\n- remove resource/action hardcoded shortcut dispatch from the Go CLI\n\n## Validation\n- go test ./...
Summary by CodeRabbit
릴리스 노트
새로운 기능
문서
리팩토링