fix(rpc): improve command executor robustness and path sanitization#94
Merged
fix(rpc): improve command executor robustness and path sanitization#94
Conversation
Add early return when no headers remain after filtering empty columns. Implement proper column and row extraction methods for wrapped entities to support multiple data source types (TableProvider, PrettyRow, and plain structs).
Enable loading specific columns from CSV, XLS, and XLSX files using @path:ColumnName syntax. Supports case-insensitive column matching and skips empty cells. Windows drive letters and URLs are deliberately excluded from column parsing to avoid misinterpretation.
Entities can now be nested under a parent command via the Parent field, and have aliases for their command. Parent commands are created lazily by GenerateCLI. New RegisterSubCommand APIs allow deferred attachment of non-entity commands to entity-generated parents.
The --format flag now accepts format specifications like "json=out.json,markdown=summary.md" to write multiple formats to separate files in a single invocation. Bare formats (e.g. "pretty,json=out.json") render to stdout while format=file pairs write to files. Per-sink errors are logged but don't abort other sinks.
Add validation of path parameter names, skip health endpoint when configured, properly handle format parsing errors, and provide detailed logging for executor initialization and route registration.
Contributor
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds hierarchical entity command routing and deferred subcommand registration, multi-sink output parsing and per-sink rendering, CSV/XLS(X) named-column loading for slice flags, robust table rendering early-exit when no columns remain, executor route sanitization, and small UI/formatting tweaks and tests. Changes
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rpc/serve.go (1)
412-435:⚠️ Potential issue | 🟠 MajorDeduplicate on the final sanitized pattern, not the raw path.
The duplicate key is computed before space replacement and wildcard sanitization. Two different operations can collapse to the same registered pattern here, so this guard won't catch the conflict before the second registration.
[suggested fix]
Move duplicate detection after normalization/sanitization
- // Check for duplicate method+path - key := method + " " + path - if existingOp, found := registered[key]; found { - fmt.Printf("⚠️ Warning: Duplicate endpoint detected\n") - fmt.Printf(" Path: %s %s\n", method, path) - fmt.Printf(" Already registered by: %s\n", existingOp) - fmt.Printf(" Skipping: %s\n", op.Name) - continue - } - registered[key] = op.Name - // Replace spaces in path segments with hyphens path = strings.ReplaceAll(path, " ", "-") // Sanitize path parameter names: Go's ServeMux requires alphanumeric wildcard names sanitized, ok := sanitizePathParams(path) if !ok { fmt.Printf("⚠️ Warning: Skipping route with invalid path params: %s %s\n", method, path) continue } // Register the route with method prefix (Go 1.22+ ServeMux) pattern := method + " " + sanitized + if existingOp, found := registered[pattern]; found { + fmt.Printf("⚠️ Warning: Duplicate endpoint detected\n") + fmt.Printf(" Path: %s\n", pattern) + fmt.Printf(" Already registered by: %s\n", existingOp) + fmt.Printf(" Skipping: %s\n", op.Name) + continue + } + registered[pattern] = op.Name mux.HandleFunc(pattern, s.handleExecuteCommand) routeCount++🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpc/serve.go` around lines 412 - 435, The duplicate-check uses the raw path before normalization, so collisions after spaces/wildcard sanitization are missed; move the normalization steps (strings.ReplaceAll on path and the sanitizePathParams call) before computing the duplicate key, then compute the registration key from the sanitized pattern (e.g., pattern := method + " " + sanitized") and check/update the registered map with that key; ensure you bail out on sanitizePathParams failure before dedup and then call mux.HandleFunc(pattern, s.handleExecuteCommand) using the same sanitized pattern.
🧹 Nitpick comments (3)
flags/assignment_test.go (1)
174-181: Minor: Theitoacomment is slightly inaccurate.The comment claims "alloc-free" but the implementation allocates strings via
string(...)conversions. This is inconsequential for test code, but the comment could be misleading.💡 Suggested comment fix
-// itoa is an alloc-free integer-to-string for small values used only by the -// test to build Excel cell references. +// itoa is a simple integer-to-string for small values (1-99) used only by the +// test to build Excel cell references.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flags/assignment_test.go` around lines 174 - 181, Update the comment for the itoa function to accurately reflect its behavior: mention that it converts small integers to two-character or single-character strings but does allocate temporary strings via string conversions, so it is not truly "alloc-free"; locate the itoa function and replace the misleading "alloc-free" wording with a short accurate description referencing itoa and that it’s intended for building Excel cell references in tests.api/table_test.go (1)
79-91: Good test coverage for the edge case, but consider verifying ANSI() output.The test correctly validates that rendering doesn't panic with zero columns. However, Line 90 only checks that
ANSI()doesn't panic without asserting its return value.💡 Suggestion to add assertion for ANSI() return value
Expect(func() { table.String() }).NotTo(Panic()) Expect(table.String()).To(BeEmpty()) Expect(func() { table.ANSI() }).NotTo(Panic()) + // ANSI() prepends "\n" to the lipgloss output, so expect just "\n" for empty table + Expect(table.ANSI()).To(Equal("\n"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/table_test.go` around lines 79 - 91, Add an assertion that TextTable.ANSI() returns the expected empty output in the "does not panic when all columns are filtered out during render" test: after the existing Expect(table.String()).To(BeEmpty()) call, add an Expect(table.ANSI()).To(Equal(table.String())) or Expect(table.ANSI()).To(BeEmpty()) so the test verifies the ANSI() return value (references: TextTable, TextTable.ANSI, TextTable.String, variable table).sub_command.go (1)
60-67: Consider handling the case where bothcmdandbuildare nil.If a
pendingSubCommandhas bothcmdandbuildas nil (which shouldn't happen with the current registration functions, but could occur if the struct is constructed directly), the loop silently does nothing. This could mask bugs.💡 Suggested defensive check
for _, p := range pendingSubCommands { parent := findOrCreateChild(root, p.parentName) if p.build != nil { p.build(parent) - } else { + } else if p.cmd != nil { parent.AddCommand(p.cmd) + } else { + // This shouldn't happen with the public API, but log a warning + // or silently continue based on preference } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sub_command.go` around lines 60 - 67, The loop over pendingSubCommands can silently skip entries when both p.build and p.cmd are nil; add a defensive check inside the loop (where findOrCreateChild(root, p.parentName) and parent.AddCommand are used) that detects if p.build == nil && p.cmd == nil and fails fast—either by returning an error or panicking/logging a clear fatal message that includes p.parentName (and any identifying fields) so these malformed pendingSubCommand structs are surfaced immediately instead of being silently ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@entity.go`:
- Around line 316-334: The adminCmd is shared across entities which causes admin
subcommands to attach only to the first computed target; change the logic in the
loop over GetEntities() to scope adminCmd per parent by keying it by the
computed target (e.g., entity.Parent or the target returned from
findOrCreateChild) instead of a single variable: maintain a
map[string]*cobra.Command (or map[*cobra.Command]*cobra.Command keyed by the
target) and for each entity compute target = parent or findOrCreateChild(parent,
entity.Parent), look up or create an adminCmd for that specific target, call
target.AddCommand(adminCmd) when newly created, and then call
generateEntityCLI(adminCmd, entity) for IsAdmin entities (otherwise call
generateEntityCLI(target, entity)).
In `@flags/assignment.go`:
- Around line 267-276: loadColumnFromExcel currently calls
excelize.OpenFile(path) which fails for HTTP(S) URLs; mirror loadColumnFromCSV
behavior by supporting URLs: call the existing loadFromFileOrURL helper (or
detect URL) to obtain an io.ReadCloser when path is a URL, and if you get a
reader use excelize.OpenReader(reader) instead of OpenFile; ensure the reader is
closed and handle errors appropriately (preserve existing defer-close logging
pattern). Update loadColumnFromExcel to use loadFromFileOrURL/openReader for URL
paths and keep OpenFile for local files so Excel files via URL (e.g.,
excelize.OpenReader) work and produce consistent errors.
In `@format.go`:
- Around line 106-128: PrintAndWriteSinks may format file sinks before
background tasks finish because it only calls MustPrint for stdout sinks; ensure
tasks are awaited by calling task.Wait() at the start of PrintAndWriteSinks
(before iterating opts.Sinks) so all background work completes prior to any
formatting or calls to Formatter.FormatToFile; this is safe because MustPrint
will simply no-op on a second wait.
In `@formatters/options.go`:
- Around line 14-26: The knownFormats map is missing aliases supported by
FormatManager.Format(), causing ParseFormatSpec to reject valid formats; update
the knownFormats variable to include "yml", "excel", and "xlsx" (in addition to
existing "yaml") and also add a mapping for the "yml" alias in the
canonicalFormat logic so it canonicalizes to "yaml" (referencing knownFormats,
canonicalFormat, FormatManager.Format(), and ParseFormatSpec to locate changes).
In `@rpc/serve.go`:
- Around line 92-94: The startup banner prints the health URL unconditionally
even when s.config.SkipHealth is true and the handler registration
(mux.HandleFunc("/health", s.handleHealth)) is skipped; update the Start()
banner logic to check s.config.SkipHealth and only include/print the health URL
when SkipHealth is false (or else remove that line from the banner), ensuring
the advertised endpoint matches actual registration.
- Around line 445-466: The sanitizePathParams function currently treats a
missing closing '}' as benign; change its behavior so that if a '{' is found
(start != -1) but the matching '}' search returns -1, the function returns
(path, false) to reject the malformed template. Specifically modify
sanitizePathParams to check for end == -1 and immediately return the original
path and false when an opened wildcard is never closed, keeping the rest of the
sanitization logic (calls to sanitizeWildcard and isValidWildcard) unchanged.
---
Outside diff comments:
In `@rpc/serve.go`:
- Around line 412-435: The duplicate-check uses the raw path before
normalization, so collisions after spaces/wildcard sanitization are missed; move
the normalization steps (strings.ReplaceAll on path and the sanitizePathParams
call) before computing the duplicate key, then compute the registration key from
the sanitized pattern (e.g., pattern := method + " " + sanitized") and
check/update the registered map with that key; ensure you bail out on
sanitizePathParams failure before dedup and then call mux.HandleFunc(pattern,
s.handleExecuteCommand) using the same sanitized pattern.
---
Nitpick comments:
In `@api/table_test.go`:
- Around line 79-91: Add an assertion that TextTable.ANSI() returns the expected
empty output in the "does not panic when all columns are filtered out during
render" test: after the existing Expect(table.String()).To(BeEmpty()) call, add
an Expect(table.ANSI()).To(Equal(table.String())) or
Expect(table.ANSI()).To(BeEmpty()) so the test verifies the ANSI() return value
(references: TextTable, TextTable.ANSI, TextTable.String, variable table).
In `@flags/assignment_test.go`:
- Around line 174-181: Update the comment for the itoa function to accurately
reflect its behavior: mention that it converts small integers to two-character
or single-character strings but does allocate temporary strings via string
conversions, so it is not truly "alloc-free"; locate the itoa function and
replace the misleading "alloc-free" wording with a short accurate description
referencing itoa and that it’s intended for building Excel cell references in
tests.
In `@sub_command.go`:
- Around line 60-67: The loop over pendingSubCommands can silently skip entries
when both p.build and p.cmd are nil; add a defensive check inside the loop
(where findOrCreateChild(root, p.parentName) and parent.AddCommand are used)
that detects if p.build == nil && p.cmd == nil and fails fast—either by
returning an error or panicking/logging a clear fatal message that includes
p.parentName (and any identifying fields) so these malformed pendingSubCommand
structs are surfaced immediately instead of being silently ignored.
🪄 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: 23812c9b-31c1-4281-9c17-20c9ef9f3ebe
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
api/table.goapi/table_test.gocobra_command.goentity.goentity_test.goflags.goflags/assignment.goflags/assignment_test.goformat.goformatters/format_sinks_test.goformatters/options.gorpc/serve.gorpc/serve_test.gosub_command.gosub_command_test.gotask/task.go
Comment on lines
+267
to
+276
| func loadColumnFromExcel(path, column string) ([]string, error) { | ||
| f, err := excelize.OpenFile(path) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("opening %s: %w", path, err) | ||
| } | ||
| defer func() { | ||
| if cerr := f.Close(); cerr != nil { | ||
| logger.Errorf("failed to close excel file %s: %v", path, cerr) | ||
| } | ||
| }() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if excelize has URL/stream support
rg -n "OpenReader|OpenFile" --type goRepository: flanksource/clicky
Length of output: 120
🌐 Web query:
Does excelize library support opening Excel files from URLs or HTTP streams?
💡 Result:
No, the excelize library does not directly support opening Excel files from URLs. OpenFile expects a local file path, and OpenReader accepts an io.Reader, which requires first fetching the data from the URL using net/http.Get and ioutil.ReadAll (or io.ReadAll), then passing bytes.NewReader(data) to OpenReader. Example from community solution: - Fetch bytes from HTTP URL - Use excelize.OpenReader(bytes.NewReader(data)) Official docs and source code confirm OpenReader(r io.Reader), but no built-in HTTP client or URL support. URLs must be direct downloads (e.g., Dropbox with ?dl=1). HTTP streams are supported indirectly via io.Reader after manual fetch.
Citations:
- 1: https://stackoverflow.com/questions/73827326/how-to-open-an-excel-file-on-excelize-using-url-link
- 2: https://github.com/qax-os/excelize/blob/master/excelize.go
- 3: Fix: Add OpenReader options to GetRows qax-os/excelize#1816
- 4: io.ReadAll impact the OpenReader function memory usage qax-os/excelize#2086
- 5: https://github.com/xuri/excelize
🏁 Script executed:
# Search for loadFromFileOrURL function definition
rg -n "func loadFromFileOrURL" --type go -A 15Repository: flanksource/clicky
Length of output: 879
🏁 Script executed:
# Check for tests or documentation about URL support
rg -n "http|URL|url" flags/assignment.go -i -B 2 -A 2Repository: flanksource/clicky
Length of output: 2278
🏁 Script executed:
# Look for any comments about Excel limitations or URL handling
rg -n "Excel|excelize" flags/assignment.go -i -B 2 -A 2Repository: flanksource/clicky
Length of output: 1096
Excel column loading doesn't support URLs, unlike CSV.
loadColumnFromCSV uses loadFromFileOrURL which supports both local files and HTTP(S) URLs (lines 135-136), but loadColumnFromExcel directly calls excelize.OpenFile(path) which only works with local files. This creates an inconsistency: @https://example.com/data.csv:Column works, but @https://example.com/data.xlsx:Column fails with a confusing "no such file or directory" error.
Either add support for Excel URLs by fetching the content and using excelize.OpenReader(), or add an explicit check that returns a clear error message for URL-based Excel column selectors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flags/assignment.go` around lines 267 - 276, loadColumnFromExcel currently
calls excelize.OpenFile(path) which fails for HTTP(S) URLs; mirror
loadColumnFromCSV behavior by supporting URLs: call the existing
loadFromFileOrURL helper (or detect URL) to obtain an io.ReadCloser when path is
a URL, and if you get a reader use excelize.OpenReader(reader) instead of
OpenFile; ensure the reader is closed and handle errors appropriately (preserve
existing defer-close logging pattern). Update loadColumnFromExcel to use
loadFromFileOrURL/openReader for URL paths and keep OpenFile for local files so
Excel files via URL (e.g., excelize.OpenReader) work and produce consistent
errors.
- rpc: reject unbalanced braces in sanitizePathParams and fix end index - rpc: deduplicate routes on sanitized pattern, not raw path - rpc: gate /health startup banner on SkipHealth - rpc: apply De Morgan's law in isValidWildcard (QF1001) - cobra: drop embedded FormatOptions selector (QF1008) - entity: scope adminCmd per parent target instead of sharing a single var - format: wait for background tasks before rendering sinks - formatters: accept yml/xlsx/excel/tree/html-static in knownFormats - go.sum: run go mod tidy
flankbot
pushed a commit
that referenced
this pull request
Apr 13, 2026
## [1.21.2](v1.21.1...v1.21.2) (2026-04-13) ### ♻️ Code Refactoring * **api:** simplify html rendering with fmt.fprintf and static html provider ([f89e0ef](f89e0ef)) ### ✨ Features * **api:** add admin entity subcommands and entity id wrapping ([1cb238c](1cb238c)) * **api:** add entity parent nesting and command aliases support ([664876f](664876f)) * **ci:** upgrade go to 1.26 and add task-ui frontend builds ([fb0818d](fb0818d)) * **flags:** add support for column selectors in CSV and Excel files ([94692d4](94692d4)) * **format:** support multiple output sinks with format=file syntax ([97b69e9](97b69e9)) * **formatters:** add html-react formatter with embedded react component ([75373a6](75373a6)) * **lint:** add clickylint static analyzer for api.Text usage patterns ([1fa3ab2](1fa3ab2)) * **mcp:** add install command and improve stdio handling ([c39ccce](c39ccce)) * **ui:** add task progress web ui with sse streaming and json api ([29720c1](29720c1)) ### 🐛 Bug Fixes * address PR [#94](#94) review feedback and lint failures ([3865926](3865926)) * restore lint testdata stub dropped during rebase ([644b7df](644b7df)) * **api:** prevent panic when all table columns are filtered out ([5eea6b5](5eea6b5)) * **ci:** add gitignore negations for task-ui and golangci config ([ed1a06f](ed1a06f)) * **ci:** trigger release directly on push to main ([704a544](704a544)) * **rpc:** improve command executor robustness and path sanitization ([e9b776a](e9b776a))
|
🎉 This PR is included in version 1.21.2 |
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
2 participants
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 by CodeRabbit
New Features
Bug Fixes
Improvements
Tests