Conversation
Add support for including a hidden _id column in table rows for wrapped entities (entityWithID). The _id column is automatically added to the Columns() output and included in Row() and PrettyRow() data, but marked as hidden so it doesn't appear as a visible column in the UI. This allows the _id metadata to be available in row data while keeping the table display clean. Updates the HTML React formatter to handle cells that exist in row data but not in the column definitions. Adds comprehensive test coverage for all entity types (TableProvider, PrettyRow, and plain structs) to verify the _id column is properly included in the output data but hidden from the visible columns. Refs: entity wrapping, table formatting
Introduce comprehensive stack trace support with language-agnostic parsing, styled rendering, and extensible source resolution. Adds StackTrace, StackFrame types and ParseJavaStackTrace parser to api package, with public convenience wrappers in clicky root. Includes support for frame filtering, context lines, and max frame truncation. Also adds MultiFilter type for comma-separated flag values with include/exclude semantics, and fixes field value assignment to support type conversion in flags package. Updates cache debug output to use task.GatedStderr() instead of os.Stderr to prevent log lines from breaking interactive renderer frame accounting. Fixes recursive struct handling in OpenAPI schema generation by tracking visited types. Refs: stack trace rendering, source context, Java exception parsing
…port Add shiki syntax highlighter and transformers to webapp dependencies for code block rendering. Implement comprehensive stack trace parsing and display with source resolution for Java exceptions. Add new demo showcases for code samples, label badges, documentation links, and parsed stack traces with configurable context and filtering. Update import paths and module resolution to use published clicky-ui distribution. Reorganize webapp navigation with semantic HTML and icon support. Breaking changes: - Update TypeScript path aliases to reference dist files instead of src - Change clicky-ui import to use published api-explorer module - Update CSS import to use distributed styles.css Refs: Stack trace rendering feature, syntax highlighting integration
Introduce a Builder pattern for configuring MCP servers with a fluent API (NewMcpServer, WithExclude, WithInclude, AutoExpose, IgnoreParams, WithFormat, Build, Command). Add a built-in 'discover-tools' MCP tool that returns a formatted catalogue of all registered tools with their parameters, defaults, and constraints. Supports multiple output formats (markdown, ansi, plain) via FormatOptions. Add IgnoredParamRule and IgnoredParams config to strip parameters from MCP tool schemas (MCP-only, does not affect OpenAPI). Add Format config to apply server-wide format/color overrides to tool execution. Refactor config path functions to be per-application (GetConfigPathFor, GetPromptsPathFor) so multiple clicky-based MCP servers don't share config files. Maintain backward compatibility with legacy GetConfigPath/GetPromptsPath. Remove the special 'discover-tools' prompt and its inline handling from prompts.go and command.go, replacing it with the dedicated MCP tool. Remove Verbose flag from CommandOptions (use clicky's existing --loglevel/-v instead). Includes comprehensive tests for Builder, IgnoredParams application, discover-tools rendering, and registry builtins.
… highlighting Implement dedicated renderHTML() method for StackTrace to produce semantic HTML blocks with: - Exception header in red banner - Individual stack frames with bordered cards for source context - Line-numbered source code with gutter and focal line highlighting - Syntax highlighting via Code.HTML() when language is known Add SourceLineNumbers field to StackFrame to support custom line numbering in source context. Wrap XML formatter in formatXMLBestEffort() to gracefully handle malformed XML by falling back to original content while preserving syntax highlighting. Fix flag value serialization in entity.go to properly round-trip slice-valued flags through CSV encoding, preventing bracket wrapping issues. Refs: improved stack trace presentation and robustness
Add native StackTrace rendering to both HTML and React formatters: - HTMLFormatter: new wrapHTMLBody() method wraps pre-rendered HTML fragments with Tailwind styling and CSS links - HTMLFormatter.format(): detect StackTrace inputs and render them with structured HTML before falling back to Pretty interface - HTMLReactFormatter: new clickyStackFrame struct and convertStackTrace() function to convert StackTrace objects to clicky nodes - React HTML: add StackTrace component import and rendering logic to handle stacktrace nodes and stacktrace maps - Update clicky-ui CDN version from @latest to @^0.2.1 for stability - Add comprehensive test coverage for StackTrace conversion This enables rich, semantic rendering of stack traces with syntax highlighting and source context instead of plain text output.
… support Implement Server-Sent Events (SSE) transport alongside stdio for MCP servers, enabling Claude Desktop and other clients to connect via HTTP. Add `mcp tools` subcommand to preview tool catalogue with format options. Expand `mcp install` to support Claude, Codex, Gemini, Copilot, and Cursor clients with per-client config writers (JSON for most, TOML for Codex). Refactor config merging into reusable `loadCommandConfig` and `mergeInitialConfig` helpers to support both serve and tools commands. Remove built-in discover-tools tool in favor of dedicated `mcp tools` command for cleaner separation of concerns. Add comprehensive tests for SSE round-trip, install permutations across clients/transports, and config merging logic.
Remove unused module entries from go.sum that are no longer referenced in the project. This reduces dependency bloat and improves build clarity. Also add examples/enitity/enitity to .gitignore to exclude the compiled binary from version control.
Convert switch statement from empty case conditions to direct value matching on loc variable. This improves code clarity and reduces redundant equality comparisons without changing behavior.
Add Diff type to api package supporting multiple output formats (plain, ANSI, HTML, Markdown) for rendering unified diffs between strings. Implement grouped flag help output in flags package with category-based organization (Logging, Tasks, Format) via FlagCategoryAnnotation. Add BindAllFlagsToCommand helper to bind flags and install grouped usage template. Update cobra_command.go error logging to use %w format and adjust skip level. Move fatih/color and go-difflib from indirect to direct dependencies in go.mod.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
WalkthroughThis PR introduces comprehensive enhancements across Clicky's text-rendering API, MCP (Model Context Protocol) infrastructure, and web UI. It adds unified diff rendering, extends stack-trace HTML output with per-line numbering and syntax highlighting, hardens XML formatting, expands MCP with multi-client installation and SSE transport, implements tool discovery, adds grouped CLI help, and modernizes the example webapp with React/Tailwind. ChangesAPI, MCP, and UI Platform Modernization
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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: 18
🧹 Nitpick comments (3)
api/code_test.go (1)
272-285: ⚡ Quick winTest doesn't actually exercise the panic-recovery path.
Both assertions (
"AddressScreen"preserved andclass=present) pass regardless of whetherxmlfmt.FormatXMLpanics or succeeds — a successful format run still contains the original tag names, and Chroma always emitsclass=attributes for XML. The test name implies the recovery branch is covered, but there's no mechanism that forces the panic.If
xmlfmtis later patched to handle this input gracefully, the test keeps passing while therecover()branch informatXMLBestEffortbecomes dead code with no regression protection.To pin the panic-fallback path specifically, consider replacing the
xmlfmtcall with a function variable that can be swapped in tests:♻️ Proposed approach to make the panic path testable
In
api/code.go:+// xmlFormatter is a package-level variable so tests can inject a panicking formatter. +var xmlFormatter = func(content string) string { + return xmlfmt.FormatXML(content, "", " ") +} + func formatXMLBestEffort(content string) (formatted string) { defer func() { if recover() != nil || strings.TrimSpace(formatted) == "" { formatted = content } }() - formatted = xmlfmt.FormatXML(content, "", " ") + formatted = xmlFormatter(content) return formatted }In the test:
func TestCodeHTML_XMLFormatterPanicFallsBackToHighlightedContent(t *testing.T) { + orig := xmlFormatter + xmlFormatter = func(_ string) string { panic("simulated xmlfmt panic") } + defer func() { xmlFormatter = orig }() + code := Code{ Content: `<AddressScreen>...</AddressScreen>`, Language: "xml", }🤖 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 `@api/code_test.go` around lines 272 - 285, The test TestCodeHTML_XMLFormatterPanicFallsBackToHighlightedContent doesn't force the panic path; make the xml formatter swap-able so the test can inject a panicking formatter: introduce a package-level variable (e.g., xmlFormatFunc) in the code that formatXMLBestEffort calls instead of calling xmlfmt.FormatXML directly, default it to xmlfmt.FormatXML, then in the test replace xmlFormatFunc with a function that panics (and restore it after the test) so the test exercises the recover() branch in formatXMLBestEffort and asserts the fallback behavior.mcp/install_test.go (1)
213-228: 💤 Low value
t.Setenv("HOME", ...)is a no-op on Windows.
os.UserHomeDir()usesUSERPROFILEon Windows, notHOME, so this sandbox helper won't redirect home on Windows runners. If CI is Linux-only this is fine in practice, but consider also settingUSERPROFILE(or skipping withruntime.GOOS == "windows") to keep the test portable.🤖 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 `@mcp/install_test.go` around lines 213 - 228, The helper withTempHomeAndCwd currently only sets HOME which is ineffective on Windows; update it to also set USERPROFILE to the same temp dir when runtime.GOOS == "windows" (or alternatively skip the home override on Windows by returning early), so that os.UserHomeDir() and other Windows APIs see the sandboxed home; modify the function body around t.Setenv("HOME", home) and use runtime.GOOS and t.Setenv("USERPROFILE", home) (or a Windows conditional skip) to ensure portability.mcp/config.go (1)
81-87: 💤 Low valueClarify ToolGlob documentation around wildcard matching depth.
The
*wildcard inpath.Matchmatches across any depth, not just direct children, since space is not treated as a path separator (only/is special). The current doc reads "ai * matches all subcommands of ai" which could be misunderstood as matching only direct children. Consider rewording to clarify that"ai *"matchesai,ai cache,ai cache foo, etc.—any descendants at any depth.🤖 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 `@mcp/config.go` around lines 81 - 87, The ToolGlob comment for IgnoredParamRule is misleading about wildcard depth: update the comment on the IgnoredParamRule struct (specifically the ToolGlob explanation) to state that path.Match's "*" matches across any depth (space isn't a path separator), and clarify that examples like "ai *" match ai and all descendants such as "ai cache" and "ai cache foo" rather than only direct children. Keep the existing examples but reword them to explicitly say "any depth/descendants" to avoid confusion.
🤖 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 `@api/stacktrace.go`:
- Around line 287-289: The HTML rendering currently skips the source block
whenever f.SourceStartLine == 0 even if f.SourceLines exists with explicit
per-line numbers; update StackTrace.renderHTMLFrame to treat hasSource as true
when len(f.SourceLines) > 0 AND (f.SourceStartLine > 0 OR
len(f.SourceLineNumbers) > 0) so sparse/non-contiguous contexts render.
Likewise, in the text renderer (the code around renderTextFrame / the early
return at lines ~410-412), remove the early return conditioned on
SourceStartLine==0 and instead use f.SourceLineNumbers when present to compute
per-line numbers; ensure both renderHTMLFrame and renderTextFrame fall back to
SourceStartLine when SourceLineNumbers is empty.
In `@examples/enitity/webapp/src/App.tsx`:
- Around line 33-45: The nav currently uses tab semantics (role="tablist" on the
nav and role="tab" on each Link) but these Links perform navigation; remove
role="tablist" from the nav element and remove role="tab" from each Link in the
nav, and instead set aria-current="page" on the active Link (use the existing
topNavLinkClass(showingLinks) / topNavLinkClass(!showingLinks) or the
showingLinks boolean to conditionally add aria-current="page" to the active
Link); keep the visual classes but switch to standard navigation semantics for
accessibility.
In `@examples/uber_demo/main.go`:
- Around line 1294-1313: In showStackTrace, NativeFrames currently calls
clicky.StackTrace(javaNullPointerTrace) without applying opts.Max; update the
NativeFrames invocation to pass the same max-frame option when opts.Max > 0
(reuse headersOnlyOpts or append clicky.WithMaxStackFrames(opts.Max) to the
arguments passed to clicky.StackTrace for the NativeFrames variant) so that
NativeFrames honors opts.Max like WithSource and WithoutSource.
In `@flags_test.go`:
- Around line 56-67: The function sectionAfter currently returns the remainder
of help when it can't find "\n\n", which can let flags from later sections be
mistaken as part of the target section; update sectionAfter (params help,
heading; current vars idx and rest) to locate the end of the section by finding
the next section heading instead of only relying on "\n\n": after finding idx
and rest, search for the earliest boundary that is either "\n\n" or a line that
looks like a section header (e.g., a newline followed by non-space characters
ending with ':'), and return rest up to that boundary (or "" if heading not
found) so only the intended section is returned.
In `@formatters/html_react_formatter.go`:
- Around line 844-845: Replace the incorrect Clicky UI stylesheet path that
points to the source file
"https://cdn.jsdelivr.net/npm/@flanksource/clicky-ui@^0.2.1/src/styles/tokens.css"
with the published distribution asset
"https://cdn.jsdelivr.net/npm/@flanksource/clicky-ui@^0.2.1/dist/styles.css" in
formatters/html_react_formatter.go (update both occurrences of that href string,
the one at the link tag shown and the other occurrence noted around line 901).
In `@go.mod`:
- Line 14: Run `go mod tidy` locally to update the module graph after promoting
direct dependencies (e.g., the github.com/fatih/color v1.18.0 change in go.mod),
then stage and commit the resulting go.sum changes so CI sees the same
dependency checksums; ensure the go.sum delta produced by `go mod tidy` is
included in this PR.
In `@mcp/command.go`:
- Around line 227-233: The command currently overwrites the configured format by
always using the flag default "pretty"; fix it so the runtime
config.Tools.Format is honored when the user does not pass --format. After
loading config.Tools.Format and before creating renderOpts, determine the
effective format by checking whether the flag "format" was explicitly set
(cmd.Flags().Changed("format")) and only use the flag value if set; otherwise
use config.Tools.Format (or initialize the local variable used in StringVar from
config.Tools.Format). Then construct &formatters.FormatOptions{Format:
effectiveFormat} and call RenderDiscoverToolsString with that value.
- Around line 460-461: Update the erroneous help text in mcp/command.go that
currently points users to "app mcp tools discover" — locate the help/usage
string (the message containing "Tool discovery has moved: use 'app mcp tools
discover'") and change it to point to "app mcp tools" (or instruct to call the
"discover-tools" MCP tool) so users are directed to the existing
newToolsCommand; ensure the replacement string reads something like "Tool
discovery has moved: use 'app mcp tools' (or call the 'discover-tools' MCP tool)
instead of 'app mcp prompt discover-tools'".
In `@mcp/install_test.go`:
- Around line 1-253: The file fails gofmt -s cleaning due to inconsistent
struct-literal formatting in the table-driven tests (see TestWriteClientConfig
and the cases []installTest slice and installTest struct), so run gofmt -s -w on
the affected files (e.g. mcp/install.go mcp/install_test.go or simply run make
fmt) and reformat the multi-line installTest literals so fields are consistently
one per line (or consistently grouped) to satisfy gofmt; then commit the
rewritten files.
In `@mcp/install.go`:
- Around line 305-326: The comment in renderCodexBlock is misleading because
writeClientConfig currently allows client=codex with transport=sse and
writeCodexTOML emits only a url field that Codex doesn't consume; add explicit
validation in writeClientConfig (or at the command RunE entrypoint) to reject
the combination client=="codex" && transport=="sse" with a clear error message,
update any success messaging to not claim a working Codex install when this
combo is provided, and either remove/update the renderCodexBlock comment to
reflect forward-compat behavior if you keep url-only output; reference the
renderCodexBlock, writeCodexTOML, writeClientConfig and RunE symbols when making
these changes.
- Around line 73-88: The SSE install sets endpoint to http://127.0.0.1:8080/sse
but only appends --port to serveArgs when port > 0, causing client/server
mismatch; modify the logic around serveArgs, transport, port and endpoint so
that when transport == "sse" you resolve a single port value (set port = 8080 if
it's 0) and always append "--port" with that resolved port to serveArgs, then
build endpoint from that same resolved port variable so both client and server
use the same port.
- Around line 331-341: resolveBinaryPath can overwrite a valid executable path
with an empty string when filepath.EvalSymlinks fails and can prefer a PATH
match that collides with an unrelated system binary; fix by only assigning the
result of filepath.EvalSymlinks to binPath when EvalSymlinks returns no error
(leave the original os.Executable() value on error), and when
exec.LookPath(rootName) returns a candidate p, cross-check p against the current
binPath (after normalizing/evaluating paths) and only replace binPath with p if
it is a true distinct and intended match (otherwise keep the executable-derived
path); refer to resolveBinaryPath, binPath, os.Executable,
filepath.EvalSymlinks, exec.LookPath and rootName when making the change.
- Around line 167-197: The copilot global config path in clientConfigPath
currently hardcodes the Linux location; update the "copilot" case to switch on
runtime.GOOS and return the correct OS-specific path: use
"%APPDATA%/Code/User/mcp.json" semantics for windows (build from
os.Getenv("APPDATA") or filepath.Join(os.Getenv("APPDATA"), "Code", "User",
"mcp.json")), use "~/Library/Application Support/Code/User/mcp.json" for darwin
(use os.UserHomeDir + filepath.Join("Library","Application
Support","Code","User","mcp.json")), and keep the existing
"~/.config/Code/User/mcp.json" for linux/default; keep the non-global branch
unchanged. Ensure you import runtime if not already used.
In `@mcp/server.go`:
- Around line 219-223: The SSE endpoint currently sets
Access-Control-Allow-Origin: * which lets any webpage obtain a session_id;
change this to enforce an origin allowlist and/or require authentication before
issuing sessions: in the SSE handler (the code that sets w.Header and issues the
session_id for "/sse"—look for the function handling SSE connections and any
session creation function like issueSession/createSession) read the Origin
header, validate it against a configured allowlist (or verify an auth
token/session) and if not allowed respond 403 and do not issue a session; when
allowed, set Access-Control-Allow-Origin to the validated origin only (not "*")
and add a Vary: Origin header; ensure the session issuance code is gated behind
this check so untrusted origins cannot receive session_id values.
- Around line 252-289: The /messages handler (registered via mux.HandleFunc)
currently allows overlapping requests which can race on shared tool.Command
state and on swapping os.Stdout/os.Stderr; prevent cross-session corruption by
serializing execution: acquire a mutex (per-session or global) before calling
s.handleJSONRPCRequest and before any tool execution that mutates tool.Command
or swaps stdio, and release it only after the response has been marshaled and
sent via sseSession.send; alternatively, change the execution path to avoid
global stdout/stderr by cloning commands and using per-request io.Pipe or
bytes.Buffer for Stdout/Stderr so each request captures output locally (update
tool.Command usage to accept io.Writer for output), then remove swapping of
os.Stdout/os.Stderr. Ensure references to sessions.Load(sessionID),
s.handleJSONRPCRequest, sseSession.send, and tool.Command are updated
accordingly.
- Around line 265-269: The code currently uses io.ReadAll(r.Body) which allows
unbounded reads; wrap the request body with http.MaxBytesReader (or
io.LimitReader) before reading to enforce a maximum payload size (e.g.,
maxMessageSize) and handle oversized requests by returning
http.StatusRequestEntityTooLarge (413) instead of StatusBadRequest; update the
read block that references io.ReadAll and r.Body to use http.MaxBytesReader(w,
r.Body, maxMessageSize) and check for a size error to call http.Error(w,
"request too large", http.StatusRequestEntityTooLarge).
- Around line 534-537: The format override is applied after applyArgsToCommand
calls cmd.ResetFlags(), which clears flag definitions so setFlagBestEffort
cannot Lookup the "format" or "no-color" flags; move the call to
applyFormatOverride(tool.Command, s.config.Tools.Format) to run before
applyArgsToCommand (i.e., before any ResetFlags() happens) so the flags exist
when setFlagBestEffort/Lookup runs, or alternatively update setFlagBestEffort to
operate on a flagset that survives ResetFlags (e.g., cmd.PersistentFlags() or
re-register the format/no-color flags after ResetFlags) and ensure
applyFormatOverride targets those surviving flags.
In `@mcp/sse_test.go`:
- Around line 94-117: The readSSEEvent helper (readSSEEvent) doesn't enforce a
per-read deadline because it loops on time.Now().Before(deadline) while calling
r.ReadString('\n') which can block indefinitely; fix by making the read itself
time out: either (A) create the HTTP request that supplies the bufio.Reader with
a bounded client/context (use an http.Client with Timeout or Transport settings
like ResponseHeaderTimeout and call the request with context.WithDeadline) so
the response body reads will error when the deadline elapses, or (B) change
readSSEEvent to accept an io.ReadCloser/connection that supports SetReadDeadline
(the underlying net.Conn) and call SetReadDeadline(deadline) before each
r.ReadString('\n') attempt so the read returns on timeout and you can
t.Fatalf("timed out…") as intended; update the test setup to pass the timed
client/conn accordingly.
---
Nitpick comments:
In `@api/code_test.go`:
- Around line 272-285: The test
TestCodeHTML_XMLFormatterPanicFallsBackToHighlightedContent doesn't force the
panic path; make the xml formatter swap-able so the test can inject a panicking
formatter: introduce a package-level variable (e.g., xmlFormatFunc) in the code
that formatXMLBestEffort calls instead of calling xmlfmt.FormatXML directly,
default it to xmlfmt.FormatXML, then in the test replace xmlFormatFunc with a
function that panics (and restore it after the test) so the test exercises the
recover() branch in formatXMLBestEffort and asserts the fallback behavior.
In `@mcp/config.go`:
- Around line 81-87: The ToolGlob comment for IgnoredParamRule is misleading
about wildcard depth: update the comment on the IgnoredParamRule struct
(specifically the ToolGlob explanation) to state that path.Match's "*" matches
across any depth (space isn't a path separator), and clarify that examples like
"ai *" match ai and all descendants such as "ai cache" and "ai cache foo" rather
than only direct children. Keep the existing examples but reword them to
explicitly say "any depth/descendants" to avoid confusion.
In `@mcp/install_test.go`:
- Around line 213-228: The helper withTempHomeAndCwd currently only sets HOME
which is ineffective on Windows; update it to also set USERPROFILE to the same
temp dir when runtime.GOOS == "windows" (or alternatively skip the home override
on Windows by returning early), so that os.UserHomeDir() and other Windows APIs
see the sandboxed home; modify the function body around t.Setenv("HOME", home)
and use runtime.GOOS and t.Setenv("USERPROFILE", home) (or a Windows conditional
skip) to ensure portability.
🪄 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: 11dfda51-caca-457c-8b0b-5d50b29450db
⛔ Files ignored due to path filters (3)
examples/enitity/webapp/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlexamples/uber_demo/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (38)
.gitignoreapi/code.goapi/code_test.goapi/diff.goapi/diff_test.goapi/stacktrace.goapi/stacktrace_java.gocobra_command.goentity.goexamples/enitity/main.goexamples/enitity/webapp/package.jsonexamples/enitity/webapp/pnpm-workspace.yamlexamples/enitity/webapp/src/App.tsxexamples/enitity/webapp/src/index.cssexamples/enitity/webapp/src/main.tsxexamples/enitity/webapp/tsconfig.jsonexamples/enitity/webapp/vite.config.tsexamples/uber_demo/main.goflags.goflags_test.goformatters/html_formatter.goformatters/html_react_formatter.goformatters/html_react_formatter_test.gogo.modmcp/builder.gomcp/builder_test.gomcp/command.gomcp/command_test.gomcp/config.gomcp/discover_tools.gomcp/discover_tools_test.gomcp/install.gomcp/install_test.gomcp/prompts.gomcp/registry.gomcp/registry_test.gomcp/server.gomcp/sse_test.go
💤 Files with no reviewable changes (1)
- examples/enitity/webapp/src/index.css
Comment on lines
+287
to
+289
| func (s StackTrace) renderHTMLFrame(b *strings.Builder, f StackFrame) { | ||
| hasSource := len(f.SourceLines) > 0 && f.SourceStartLine > 0 | ||
|
|
Contributor
There was a problem hiding this comment.
Don’t require SourceStartLine when explicit line numbers are present.
The new SourceLineNumbers field is still ineffective when SourceStartLine == 0: the HTML path suppresses the entire source block up front, and the text path still returns early before these per-line numbers matter. That makes sparse or non-contiguous source contexts impossible to render.
Suggested fix
func (s StackTrace) renderHTMLFrame(b *strings.Builder, f StackFrame) {
- hasSource := len(f.SourceLines) > 0 && f.SourceStartLine > 0
+ hasSource := len(f.SourceLines) > 0 && (f.SourceStartLine > 0 || len(f.SourceLineNumbers) > 0) func appendSourceLines(out Text, f StackFrame, opts stackTraceOptions) Text {
- if len(f.SourceLines) == 0 || f.SourceStartLine <= 0 {
+ if len(f.SourceLines) == 0 || (f.SourceStartLine <= 0 && len(f.SourceLineNumbers) == 0) {
return out
}Also applies to: 410-412
🤖 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 `@api/stacktrace.go` around lines 287 - 289, The HTML rendering currently skips
the source block whenever f.SourceStartLine == 0 even if f.SourceLines exists
with explicit per-line numbers; update StackTrace.renderHTMLFrame to treat
hasSource as true when len(f.SourceLines) > 0 AND (f.SourceStartLine > 0 OR
len(f.SourceLineNumbers) > 0) so sparse/non-contiguous contexts render.
Likewise, in the text renderer (the code around renderTextFrame / the early
return at lines ~410-412), remove the early return conditioned on
SourceStartLine==0 and instead use f.SourceLineNumbers when present to compute
per-line numbers; ensure both renderHTMLFrame and renderTextFrame fall back to
SourceStartLine when SourceLineNumbers is empty.
Comment on lines
+33
to
+45
| <nav | ||
| role="tablist" | ||
| aria-label="Top sections" | ||
| className="flex items-center gap-1 rounded-full border border-border/70 bg-muted/40 p-1" | ||
| > | ||
| Link Examples | ||
| </Link> | ||
| </nav> | ||
| <Link to="/stacks" className={topNavLinkClass(!showingLinks)} role="tab"> | ||
| <Icon name="ph:squares-four" className="text-muted-foreground" /> | ||
| Explorer | ||
| </Link> | ||
| <Link to="/links" className={topNavLinkClass(showingLinks)} role="tab"> | ||
| <Icon name="ph:link" className="text-muted-foreground" /> | ||
| Link Examples | ||
| </Link> |
Contributor
There was a problem hiding this comment.
Use navigation semantics instead of tab semantics here.
Line 34/38/42 applies tablist/tab roles to route links, but these links navigate pages rather than controlling tabpanels. This can confuse screen readers. Prefer plain nav links with aria-current="page" on the active item.
Suggested fix
- <nav
- role="tablist"
- aria-label="Top sections"
- className="flex items-center gap-1 rounded-full border border-border/70 bg-muted/40 p-1"
- >
- <Link to="/stacks" className={topNavLinkClass(!showingLinks)} role="tab">
+ <nav
+ aria-label="Top sections"
+ className="flex items-center gap-1 rounded-full border border-border/70 bg-muted/40 p-1"
+ >
+ <Link
+ to="/stacks"
+ className={topNavLinkClass(!showingLinks)}
+ aria-current={!showingLinks ? "page" : undefined}
+ >
<Icon name="ph:squares-four" className="text-muted-foreground" />
Explorer
</Link>
- <Link to="/links" className={topNavLinkClass(showingLinks)} role="tab">
+ <Link
+ to="/links"
+ className={topNavLinkClass(showingLinks)}
+ aria-current={showingLinks ? "page" : undefined}
+ >
<Icon name="ph:link" className="text-muted-foreground" />
Link Examples
</Link>
</nav>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <nav | |
| role="tablist" | |
| aria-label="Top sections" | |
| className="flex items-center gap-1 rounded-full border border-border/70 bg-muted/40 p-1" | |
| > | |
| Link Examples | |
| </Link> | |
| </nav> | |
| <Link to="/stacks" className={topNavLinkClass(!showingLinks)} role="tab"> | |
| <Icon name="ph:squares-four" className="text-muted-foreground" /> | |
| Explorer | |
| </Link> | |
| <Link to="/links" className={topNavLinkClass(showingLinks)} role="tab"> | |
| <Icon name="ph:link" className="text-muted-foreground" /> | |
| Link Examples | |
| </Link> | |
| <nav | |
| aria-label="Top sections" | |
| className="flex items-center gap-1 rounded-full border border-border/70 bg-muted/40 p-1" | |
| > | |
| <Link | |
| to="/stacks" | |
| className={topNavLinkClass(!showingLinks)} | |
| aria-current={!showingLinks ? "page" : undefined} | |
| > | |
| <Icon name="ph:squares-four" className="text-muted-foreground" /> | |
| Explorer | |
| </Link> | |
| <Link | |
| to="/links" | |
| className={topNavLinkClass(showingLinks)} | |
| aria-current={showingLinks ? "page" : undefined} | |
| > | |
| <Icon name="ph:link" className="text-muted-foreground" /> | |
| Link Examples | |
| </Link> | |
| </nav> |
🤖 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 `@examples/enitity/webapp/src/App.tsx` around lines 33 - 45, The nav currently
uses tab semantics (role="tablist" on the nav and role="tab" on each Link) but
these Links perform navigation; remove role="tablist" from the nav element and
remove role="tab" from each Link in the nav, and instead set aria-current="page"
on the active Link (use the existing topNavLinkClass(showingLinks) /
topNavLinkClass(!showingLinks) or the showingLinks boolean to conditionally add
aria-current="page" to the active Link); keep the visual classes but switch to
standard navigation semantics for accessibility.
Comment on lines
+1294
to
+1313
| func showStackTrace(opts StackTraceOptions) (any, error) { | ||
| resolvedOpts := []api.StackTraceOption{ | ||
| clicky.WithStackInclude("com.example."), | ||
| clicky.WithStackContext(opts.Context), | ||
| clicky.WithSourceResolver(fakeJavaSourceResolver()), | ||
| } | ||
| if opts.Max > 0 { | ||
| resolvedOpts = append(resolvedOpts, clicky.WithMaxStackFrames(opts.Max)) | ||
| } | ||
|
|
||
| headersOnlyOpts := []api.StackTraceOption{clicky.WithStackInclude("com.example.")} | ||
| if opts.Max > 0 { | ||
| headersOnlyOpts = append(headersOnlyOpts, clicky.WithMaxStackFrames(opts.Max)) | ||
| } | ||
|
|
||
| return StackTraceShowcase{ | ||
| WithSource: clicky.StackTrace(javaSampleTrace, resolvedOpts...), | ||
| WithoutSource: clicky.StackTrace(javaNullPointerTrace, headersOnlyOpts...), | ||
| NativeFrames: clicky.StackTrace(javaNullPointerTrace), | ||
| }, nil |
Contributor
There was a problem hiding this comment.
Apply --max-frames to the NativeFrames variant too.
WithSource and WithoutSource honor opts.Max, but NativeFrames always renders the full trace. The new flag becomes inconsistent across the three sections.
Suggested fix
func showStackTrace(opts StackTraceOptions) (any, error) {
resolvedOpts := []api.StackTraceOption{
clicky.WithStackInclude("com.example."),
clicky.WithStackContext(opts.Context),
clicky.WithSourceResolver(fakeJavaSourceResolver()),
}
if opts.Max > 0 {
resolvedOpts = append(resolvedOpts, clicky.WithMaxStackFrames(opts.Max))
}
headersOnlyOpts := []api.StackTraceOption{clicky.WithStackInclude("com.example.")}
if opts.Max > 0 {
headersOnlyOpts = append(headersOnlyOpts, clicky.WithMaxStackFrames(opts.Max))
}
+
+ nativeOpts := []api.StackTraceOption{}
+ if opts.Max > 0 {
+ nativeOpts = append(nativeOpts, clicky.WithMaxStackFrames(opts.Max))
+ }
return StackTraceShowcase{
WithSource: clicky.StackTrace(javaSampleTrace, resolvedOpts...),
WithoutSource: clicky.StackTrace(javaNullPointerTrace, headersOnlyOpts...),
- NativeFrames: clicky.StackTrace(javaNullPointerTrace),
+ NativeFrames: clicky.StackTrace(javaNullPointerTrace, nativeOpts...),
}, nil
}🤖 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 `@examples/uber_demo/main.go` around lines 1294 - 1313, In showStackTrace,
NativeFrames currently calls clicky.StackTrace(javaNullPointerTrace) without
applying opts.Max; update the NativeFrames invocation to pass the same max-frame
option when opts.Max > 0 (reuse headersOnlyOpts or append
clicky.WithMaxStackFrames(opts.Max) to the arguments passed to clicky.StackTrace
for the NativeFrames variant) so that NativeFrames honors opts.Max like
WithSource and WithoutSource.
Comment on lines
+56
to
+67
| func sectionAfter(help, heading string) string { | ||
| idx := strings.Index(help, heading) | ||
| if idx < 0 { | ||
| return "" | ||
| } | ||
| rest := help[idx:] | ||
| end := strings.Index(rest, "\n\n") | ||
| if end < 0 { | ||
| return rest | ||
| } | ||
| return rest[:end] | ||
| } |
Contributor
There was a problem hiding this comment.
sectionAfter can mask mis-grouped flags when separator format changes.
If \n\n is absent, the helper returns the remainder of help output, so a flag in a later section can still satisfy the assertion.
Proposed fix
func sectionAfter(help, heading string) string {
idx := strings.Index(help, heading)
if idx < 0 {
return ""
}
rest := help[idx:]
- end := strings.Index(rest, "\n\n")
- if end < 0 {
- return rest
- }
- return rest[:end]
+ lines := strings.Split(rest, "\n")
+ var out []string
+ for i, line := range lines {
+ trimmed := strings.TrimSpace(line)
+ if i > 0 && (trimmed == "" || strings.HasSuffix(trimmed, "flags:")) {
+ break
+ }
+ out = append(out, line)
+ }
+ return strings.Join(out, "\n")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func sectionAfter(help, heading string) string { | |
| idx := strings.Index(help, heading) | |
| if idx < 0 { | |
| return "" | |
| } | |
| rest := help[idx:] | |
| end := strings.Index(rest, "\n\n") | |
| if end < 0 { | |
| return rest | |
| } | |
| return rest[:end] | |
| } | |
| func sectionAfter(help, heading string) string { | |
| idx := strings.Index(help, heading) | |
| if idx < 0 { | |
| return "" | |
| } | |
| rest := help[idx:] | |
| lines := strings.Split(rest, "\n") | |
| var out []string | |
| for i, line := range lines { | |
| trimmed := strings.TrimSpace(line) | |
| if i > 0 && (trimmed == "" || strings.HasSuffix(trimmed, "flags:")) { | |
| break | |
| } | |
| out = append(out, line) | |
| } | |
| return strings.Join(out, "\n") | |
| } |
🤖 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 `@flags_test.go` around lines 56 - 67, The function sectionAfter currently
returns the remainder of help when it can't find "\n\n", which can let flags
from later sections be mistaken as part of the target section; update
sectionAfter (params help, heading; current vars idx and rest) to locate the end
of the section by finding the next section heading instead of only relying on
"\n\n": after finding idx and rest, search for the earliest boundary that is
either "\n\n" or a line that looks like a section header (e.g., a newline
followed by non-space characters ending with ':'), and return rest up to that
boundary (or "" if heading not found) so only the intended section is returned.
Comment on lines
+844
to
845
| <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@flanksource/clicky-ui@^0.2.1/src/styles/tokens.css" /> | ||
| <script src="https://code.iconify.design/iconify-icon/2.1.0/iconify-icon.min.js"></script> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the npm package @flanksource/clicky-ui version 0.2.1, what stylesheet files are publicly documented/published for CDN usage? Does the package expose src/styles/tokens.css, or should consumers use a built stylesheet such as styles.css instead?
💡 Result:
I wasn’t able to retrieve any authoritative, version-specific evidence for @flanksource/clicky-ui@0.2.1’s published/CDN stylesheet entrypoints (e.g., whether it ships a root-level styles.css, whether src/styles/tokens.css is exposed, or which CSS files are publicly documented). The web results I could access did not include the npm registry “files” list or an unpkg/unpkg directory listing for @flanksource/clicky-ui@0.2.1, and I therefore can’t answer the question with high confidence. What I checked (and what was missing): - Searches surfaced unrelated “clicky” projects and other flanksource repos, but not the npmjs/unpkg listing for @flanksource/clicky-ui@0.2.1 that would enumerate stylesheet artifacts and any documented CDN paths. - Without an unpkg/registry file listing for that exact version, any claim about “publicly documented/published stylesheet files” or whether consumers should use styles.css vs src/styles/tokens.css would be speculative. If you want, paste either: 1) the unpkg link you’re using (e.g., https://unpkg.com/@flanksource/clicky-ui@0.2.1/), or 2) the npmjs registry “files”/tarball listing for 0.2.1, and I’ll immediately determine which CSS files are present and which are intended as CDN-consumable entrypoints (and whether tokens.css under src is accessible or not).
Citations:
- 1: https://github.com/flanksource/clicky
- 2: https://github.com/flanksource/flanksource-ui
- 3: https://dev.to/ddtamn/why-ui-libraries-still-need-explicit-css-imports-5b6o
- 4: https://registry.npmjs.org/@flanksource/flanksource-ui
🏁 Script executed:
# Search for references to clicky-ui or flanksource in the codebase
rg -i "clicky-ui|flanksource" --type-list | head -20Repository: flanksource/clicky
Length of output: 452
🏁 Script executed:
# Look for package.json files that might declare this dependency
fd "package.json" -x cat {} \; 2>/dev/null | grep -A 5 -B 5 "clicky-ui" | head -50Repository: flanksource/clicky
Length of output: 44
🏁 Script executed:
# Check the specific file mentioned in the review
head -n 850 formatters/html_react_formatter.go | tail -n 20Repository: flanksource/clicky
Length of output: 1078
🏁 Script executed:
# Also check around line 901
sed -n '895,910p' formatters/html_react_formatter.goRepository: flanksource/clicky
Length of output: 881
🏁 Script executed:
# Search for any clicky-ui references in the entire codebase
rg "clicky-ui" --type go --type json --type mdRepository: flanksource/clicky
Length of output: 2026
🏁 Script executed:
# Check if there's a package.json or go.mod that lists this dependency
find . -name "package.json" -o -name "go.mod" | head -10Repository: flanksource/clicky
Length of output: 179
🏁 Script executed:
# Check if there are comments or documentation about the CSS import
sed -n '835,860p' formatters/html_react_formatter.goRepository: flanksource/clicky
Length of output: 1542
🏁 Script executed:
cat ./examples/enitity/webapp/package.jsonRepository: flanksource/clicky
Length of output: 1519
🏁 Script executed:
# Look for any CSS imports in the example webapp
rg "clicky-ui.*css|styles\.css" examples/enitity/webapp/ --type json --type ts --type tsx --type jsRepository: flanksource/clicky
Length of output: 90
🏁 Script executed:
# Search for actual CSS imports from clicky-ui in the codebase
rg "@flanksource/clicky-ui.*css|styles\.css" --type-listRepository: flanksource/clicky
Length of output: 452
🏁 Script executed:
# Search more broadly for CSS imports
rg "@flanksource/clicky-ui.*css|styles\.css"Repository: flanksource/clicky
Length of output: 598
🏁 Script executed:
# Check if there's a dist or build directory mentioned for clicky-ui
rg "clicky-ui.*dist|clicky-ui.*styles" -A 2 -B 2Repository: flanksource/clicky
Length of output: 3294
🏁 Script executed:
# Try to understand the actual package structure - look at the import map again
sed -n '900,910p' formatters/html_react_formatter.goRepository: flanksource/clicky
Length of output: 545
Fix the Clicky UI stylesheet CDN path.
The CSS is being loaded from src/styles/tokens.css, which is the source directory and not a published asset. The example webapp and package configuration show that the published stylesheet is distributed at dist/styles.css. Update the CDN link to use the correct distribution path:
CSS import fix
- <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@flanksource/clicky-ui@^0.2.1/src/styles/tokens.css" />
+ <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@flanksource/clicky-ui@^0.2.1/dist/styles.css" />Also applies to: 901-901
🤖 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 `@formatters/html_react_formatter.go` around lines 844 - 845, Replace the
incorrect Clicky UI stylesheet path that points to the source file
"https://cdn.jsdelivr.net/npm/@flanksource/clicky-ui@^0.2.1/src/styles/tokens.css"
with the published distribution asset
"https://cdn.jsdelivr.net/npm/@flanksource/clicky-ui@^0.2.1/dist/styles.css" in
formatters/html_react_formatter.go (update both occurrences of that href string,
the one at the link tag shown and the other occurrence noted around line 901).
Comment on lines
+219
to
+223
| w.Header().Set("Content-Type", "text/event-stream") | ||
| w.Header().Set("Cache-Control", "no-cache") | ||
| w.Header().Set("Connection", "keep-alive") | ||
| w.Header().Set("Access-Control-Allow-Origin", "*") | ||
|
|
Contributor
There was a problem hiding this comment.
Do not allow arbitrary web origins to bootstrap SSE sessions.
Line 222 makes the unauthenticated local control plane readable from any browser origin. A webpage can connect to /sse, receive the session_id, and then drive /messages against the user's local CLI. This needs an origin allowlist and/or explicit auth before a session is issued.
🤖 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 `@mcp/server.go` around lines 219 - 223, The SSE endpoint currently sets
Access-Control-Allow-Origin: * which lets any webpage obtain a session_id;
change this to enforce an origin allowlist and/or require authentication before
issuing sessions: in the SSE handler (the code that sets w.Header and issues the
session_id for "/sse"—look for the function handling SSE connections and any
session creation function like issueSession/createSession) read the Origin
header, validate it against a configured allowlist (or verify an auth
token/session) and if not allowed respond 403 and do not issue a session; when
allowed, set Access-Control-Allow-Origin to the validated origin only (not "*")
and add a Vary: Origin header; ensure the session issuance code is gated behind
this check so untrusted origins cannot receive session_id values.
Comment on lines
+252
to
+289
| mux.HandleFunc("/messages", func(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodPost { | ||
| http.Error(w, "method not allowed", http.StatusMethodNotAllowed) | ||
| return | ||
| } | ||
| sessionID := r.URL.Query().Get("session_id") | ||
| raw, ok := sessions.Load(sessionID) | ||
| if !ok { | ||
| http.Error(w, "unknown session_id", http.StatusNotFound) | ||
| return | ||
| } | ||
| session := raw.(*sseSession) | ||
|
|
||
| body, err := io.ReadAll(r.Body) | ||
| if err != nil { | ||
| http.Error(w, "read error", http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| response, err := s.handleJSONRPCRequest(r.Context(), string(body)) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| w.WriteHeader(http.StatusAccepted) | ||
|
|
||
| if response == nil { | ||
| return | ||
| } | ||
| payload, err := json.Marshal(response) | ||
| if err != nil { | ||
| log.Printf("Error marshaling SSE response: %v", err) | ||
| return | ||
| } | ||
| if !session.send(payload) { | ||
| log.Printf("Failed to deliver SSE response for session %s", sessionID) | ||
| } |
Contributor
There was a problem hiding this comment.
Concurrent SSE requests can corrupt command state and leak output across sessions.
This transport now accepts overlapping /messages requests, but tool execution still mutates shared tool.Command state and swaps process-global os.Stdout/os.Stderr. Two concurrent calls can cross-apply flags and interleave captured output between clients. Until execution is isolated per request, this path should serialize tool runs or clone the command/output state.
🤖 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 `@mcp/server.go` around lines 252 - 289, The /messages handler (registered via
mux.HandleFunc) currently allows overlapping requests which can race on shared
tool.Command state and on swapping os.Stdout/os.Stderr; prevent cross-session
corruption by serializing execution: acquire a mutex (per-session or global)
before calling s.handleJSONRPCRequest and before any tool execution that mutates
tool.Command or swaps stdio, and release it only after the response has been
marshaled and sent via sseSession.send; alternatively, change the execution path
to avoid global stdout/stderr by cloning commands and using per-request io.Pipe
or bytes.Buffer for Stdout/Stderr so each request captures output locally
(update tool.Command usage to accept io.Writer for output), then remove swapping
of os.Stdout/os.Stderr. Ensure references to sessions.Load(sessionID),
s.handleJSONRPCRequest, sseSession.send, and tool.Command are updated
accordingly.
Comment on lines
+265
to
+269
| body, err := io.ReadAll(r.Body) | ||
| if err != nil { | ||
| http.Error(w, "read error", http.StatusBadRequest) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Bound /messages request size before buffering it.
io.ReadAll(r.Body) lets a client force unbounded memory growth on the server. Wrap the body with http.MaxBytesReader or io.LimitReader and reject oversized payloads with 413.
💡 Suggested fix
- body, err := io.ReadAll(r.Body)
+ const maxMessageBytes = 1 << 20 // 1 MiB
+ r.Body = http.MaxBytesReader(w, r.Body, maxMessageBytes)
+ body, err := io.ReadAll(r.Body)
if err != nil {
- http.Error(w, "read error", http.StatusBadRequest)
+ http.Error(w, "read error", http.StatusBadRequest)
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| body, err := io.ReadAll(r.Body) | |
| if err != nil { | |
| http.Error(w, "read error", http.StatusBadRequest) | |
| return | |
| } | |
| const maxMessageBytes = 1 << 20 // 1 MiB | |
| r.Body = http.MaxBytesReader(w, r.Body, maxMessageBytes) | |
| body, err := io.ReadAll(r.Body) | |
| if err != nil { | |
| if err.Error() == "http: request body too large" { | |
| http.Error(w, "request too large", http.StatusRequestEntityTooLarge) | |
| } else { | |
| http.Error(w, "read error", http.StatusBadRequest) | |
| } | |
| return | |
| } |
🤖 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 `@mcp/server.go` around lines 265 - 269, The code currently uses
io.ReadAll(r.Body) which allows unbounded reads; wrap the request body with
http.MaxBytesReader (or io.LimitReader) before reading to enforce a maximum
payload size (e.g., maxMessageSize) and handle oversized requests by returning
http.StatusRequestEntityTooLarge (413) instead of StatusBadRequest; update the
read block that references io.ReadAll and r.Body to use http.MaxBytesReader(w,
r.Body, maxMessageSize) and check for a size error to call http.Error(w,
"request too large", http.StatusRequestEntityTooLarge).
Comment on lines
+534
to
+537
| // Apply server-wide format overrides (e.g. forcing Markdown | ||
| // without ANSI for AI clients). Best-effort: tools that don't | ||
| // declare matching flags are left alone. | ||
| applyFormatOverride(tool.Command, s.config.Tools.Format) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the execution order and flag lookup path involved in the override.
rg -n -C2 'ResetFlags\(|applyFormatOverride|setFlagBestEffort|Lookup\(' mcp/server.goRepository: flanksource/clicky
Length of output: 1518
🏁 Script executed:
# Get broader context around line 534-537 to see the execution order
sed -n '520,560p' mcp/server.goRepository: flanksource/clicky
Length of output: 1270
🏁 Script executed:
# Search for any comments or documentation about ResetFlags in the codebase
rg -n 'ResetFlags' mcp/server.go -B2 -A2Repository: flanksource/clicky
Length of output: 257
🌐 Web query:
Cobra ResetFlags behavior what does it do to flag definitions
💡 Result:
Cobra's ResetFlags method deletes all flags from the command by replacing the flag sets (flags, pflags, lflags, iflags, parentsPflags) with new empty FlagSets created via flag.NewFlagSet(c.DisplayName, flag.ContinueOnError). It also resets the flagErrorBuf. This clears all previously defined flag definitions on that command, requiring them to be re-added if needed. It only affects the command on which it is called and does not recursively reset child commands' flags [1][2][3].
Citations:
- 1: https://github.com/spf13/cobra/blob/master/command.go
- 2: https://github.com/spf13/cobra/blob/main/command.go
- 3: proposal/need help: Command.ResetFlags() not reset its children command flags spf13/cobra#1488
🏁 Script executed:
# Get the full applyArgsToCommand function
sed -n '724,770p' mcp/server.goRepository: flanksource/clicky
Length of output: 1243
The format override cannot modify flags that were deleted by ResetFlags().
applyFormatOverride() runs after applyArgsToCommand(), which calls cmd.ResetFlags() at line 726. This deletes all flag definitions from the command. When setFlagBestEffort() later attempts to locate the format or no-color flags via Lookup(), they no longer exist in the cleared FlagSets, so the override silently fails to apply.
🤖 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 `@mcp/server.go` around lines 534 - 537, The format override is applied after
applyArgsToCommand calls cmd.ResetFlags(), which clears flag definitions so
setFlagBestEffort cannot Lookup the "format" or "no-color" flags; move the call
to applyFormatOverride(tool.Command, s.config.Tools.Format) to run before
applyArgsToCommand (i.e., before any ResetFlags() happens) so the flags exist
when setFlagBestEffort/Lookup runs, or alternatively update setFlagBestEffort to
operate on a flagset that survives ResetFlags (e.g., cmd.PersistentFlags() or
re-register the format/no-color flags after ResetFlags) and ensure
applyFormatOverride targets those surviving flags.
Comment on lines
+94
to
+117
| func readSSEEvent(t *testing.T, r *bufio.Reader, wantEvent string) string { | ||
| t.Helper() | ||
| deadline := time.Now().Add(5 * time.Second) | ||
| var pendingEvent string | ||
| for time.Now().Before(deadline) { | ||
| line, err := r.ReadString('\n') | ||
| if err != nil { | ||
| t.Fatalf("SSE read: %v", err) | ||
| } | ||
| line = strings.TrimRight(line, "\r\n") | ||
| switch { | ||
| case strings.HasPrefix(line, "event: "): | ||
| pendingEvent = strings.TrimPrefix(line, "event: ") | ||
| case strings.HasPrefix(line, "data: "): | ||
| data := strings.TrimPrefix(line, "data: ") | ||
| if pendingEvent == wantEvent { | ||
| return data | ||
| } | ||
| pendingEvent = "" | ||
| } | ||
| } | ||
| t.Fatalf("timed out waiting for SSE event %q", wantEvent) | ||
| return "" | ||
| } |
Contributor
There was a problem hiding this comment.
Read deadline isn't actually enforced — ReadString can block past the 5s deadline.
time.Now().Before(deadline) is only checked between reads. r.ReadString('\n') itself has no deadline, so a stuck/silent server makes this test hang until Go's overall test timeout (10m by default), and the t.Fatalf("timed out…") message is never produced.
Easiest fix is to set an http.Client.Timeout and/or push a per-read deadline onto the underlying net.Conn so the read returns an error when the deadline elapses.
🛠️ Sketch fix using a bounded HTTP client
- resp, err := http.DefaultClient.Do(streamReq)
+ client := &http.Client{Timeout: 10 * time.Second}
+ resp, err := client.Do(streamReq)(or pass an http.Client whose Transport sets a ResponseHeaderTimeout and use a context.WithDeadline for the request)
🤖 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 `@mcp/sse_test.go` around lines 94 - 117, The readSSEEvent helper
(readSSEEvent) doesn't enforce a per-read deadline because it loops on
time.Now().Before(deadline) while calling r.ReadString('\n') which can block
indefinitely; fix by making the read itself time out: either (A) create the HTTP
request that supplies the bufio.Reader with a bounded client/context (use an
http.Client with Timeout or Transport settings like ResponseHeaderTimeout and
call the request with context.WithDeadline) so the response body reads will
error when the deadline elapses, or (B) change readSSEEvent to accept an
io.ReadCloser/connection that supports SetReadDeadline (the underlying net.Conn)
and call SetReadDeadline(deadline) before each r.ReadString('\n') attempt so the
read returns on timeout and you can t.Fatalf("timed out…") as intended; update
the test setup to pass the timed client/conn accordingly.
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.
Description
Brief description of the changes in this PR.
Type of Change
Testing
Checklist
Breaking Changes
If this is a breaking change, please describe the impact and migration path for existing users:
Additional Notes
Add any additional notes, screenshots, or context about the changes here.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores