Pr/rpc fixes#140
Conversation
A completed task kept its runFunc closure for the manager's 10-minute run-retention window. When the closure captured large state (parsed inputs, accumulators, intermediate outputs), that state stayed live for every retained task — a leak that scales with the number of past runs. Release runFunc once executeTask finishes: it is invoked nowhere else and retries are already exhausted by then. Completed tasks keep only their lightweight result and snapshot for viewing.
Implement request-scoped timing accumulation for HTTP handlers with Server-Timing header emission. Provides AddTiming and Track APIs for measuring operation phases within the RPC executor and custom handlers, summing durations by phase name and emitting millisecond-precision metrics in the response header. The middleware preserves http.Flusher for streaming responses and integrates without importing rpc package dependencies.: feat
📝 WalkthroughWalkthroughThis PR adds frontend lint tooling, updates the entity webapp icon wiring, refreshes aichat model and dependency data, adds hidden flag propagation, changes markdown ordering, filters RPC converter flags, adds HTTP timing instrumentation, normalizes route deduplication, updates entity tool-hint metadata plumbing, and refactors task manager/render lifecycle behavior. ChangesClicky-UI Lint Tooling
Webapp Icon Component Migration
AI Chat Default Model and Tool Metadata Update
Hidden CLI Flag Support
Schema-Ordered Markdown Formatting
RPC Converter Flag Filtering
HTTP Server-Timing Middleware
RPC Route Dedup for Wildcard Names
Entity Tool Hint Metadata
Task Manager Render and Output Lifecycle
Possibly related PRs
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Gavel crashed before producing resultsExit code: 1 Last lines of gavel.logFull |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flags/binding.go (1)
125-134: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject hidden+required flags for body-less RPC calls.
ParseStructFieldsallowshidden:"true"andrequired:"true"together, butrpc/converter.godrops hidden fields from the generated parameters. That makes the value unreachable for GET/HEAD-style RPCs, so Cobra will still fail with a missing required flag. Add a parser check or a test for this combination.🤖 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/binding.go` around lines 125 - 134, ParseStructFields currently allows hidden and required flags to be combined, but hidden fields are omitted by rpc/converter.go so body-less RPC calls can never supply them. Add a validation in ParseStructFields or the flag metadata path to reject hidden:"true" with required:"true" together, and cover the behavior with a test around the info.Hidden/info.Required handling and cmd.MarkFlagRequired/MarkHidden setup.
🧹 Nitpick comments (1)
rpc/serve.go (1)
498-501: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRoute output should go through clicky logging, not
fmt.Printf.These warning lines write directly to
os.Stdout. Per the repository guidelines, library code should route output through the clicky logging path (e.g.clicky.Warnf) rather thanfmt.Printf/fmt.Println, so live task rendering isn't corrupted. The pre-existingfmt.Printfcalls elsewhere in this function share the same issue.As per coding guidelines: "Do not write directly to
os.Stdoutoros.Stderrin library code; route output through the clicky logging/writer path instead" and "emit logs throughclicky.Infof,clicky.Errorf,clicky.Warnf, orclicky.Debugfrather thanfmt.Println".🤖 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 `@rpc/serve.go` around lines 498 - 501, The duplicate-endpoint warning in the RPC registration flow is writing directly to standard output via fmt.Printf, which should be routed through clicky logging instead. Update the warning path in the endpoint registration logic in serve.go to use clicky.Warnf (or the appropriate clicky logger) for the duplicate endpoint message and the related Path/Already registered/Skipping details, and replace the other fmt.Printf calls in the same function with clicky logging so library output does not bypass the task rendering path.Source: Coding guidelines
🤖 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 `@examples/enitity/webapp/.oxlintrc.json`:
- Around line 3-8: The oxlint config in the webapp example references the
clicky-ui plugin via a specifier that resolves outside this repository, so
update the setup accordingly. Either change the plugin path in the
.oxlintrc.json entry for clicky-ui to point to a location inside this repo, or
add clear documentation in the relevant setup docs explaining that a sibling
clicky-ui checkout is required. Use the clicky-ui plugin entry in the jsPlugins
array as the locator.
In `@rpc/http/middleware.go`:
- Around line 12-18: TimingMiddleware currently relies on timingRecorder.stamp()
being triggered by WriteHeader/Write/Flush, so handlers that return without
writing never set Server-Timing. Update TimingMiddleware to call rec.stamp()
after next.ServeHTTP returns, and keep the existing write-path behavior in
timingRecorder unchanged. Add a regression test around TimingMiddleware to cover
a handler that returns without writing and assert the Server-Timing header is
still emitted.
---
Outside diff comments:
In `@flags/binding.go`:
- Around line 125-134: ParseStructFields currently allows hidden and required
flags to be combined, but hidden fields are omitted by rpc/converter.go so
body-less RPC calls can never supply them. Add a validation in ParseStructFields
or the flag metadata path to reject hidden:"true" with required:"true" together,
and cover the behavior with a test around the info.Hidden/info.Required handling
and cmd.MarkFlagRequired/MarkHidden setup.
---
Nitpick comments:
In `@rpc/serve.go`:
- Around line 498-501: The duplicate-endpoint warning in the RPC registration
flow is writing directly to standard output via fmt.Printf, which should be
routed through clicky logging instead. Update the warning path in the endpoint
registration logic in serve.go to use clicky.Warnf (or the appropriate clicky
logger) for the duplicate endpoint message and the related Path/Already
registered/Skipping details, and replace the other fmt.Printf calls in the same
function with clicky logging so library output does not bypass the task
rendering path.
🪄 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: 354ff0d0-8083-4437-8944-fae7e2198e29
⛔ Files ignored due to path filters (1)
examples/enitity/webapp/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
.gitignoreCONTRIBUTING.mdMakefileaichat/agent.goaichat/agent_test.goaichat/models.goaichat/models_test.goexamples/enitity/webapp/.oxlintrc.jsonexamples/enitity/webapp/package.jsonexamples/enitity/webapp/pnpm-workspace.yamlexamples/enitity/webapp/src/App.tsxexamples/enitity/webapp/src/ChatWidget.tsxexamples/enitity/webapp/tsconfig.jsonexamples/enitity/webapp/vite.config.tsflags/binding.goflags/parser.goflags/parser_test.goflags/types.goformatters/map_input_test.goformatters/markdown_formatter.gorpc/converter.gorpc/converter_flags_test.gorpc/http/middleware.gorpc/http/middleware_test.gorpc/http/timing.gorpc/http/timing_test.gorpc/serve.gorpc/serve_test.gotask/worker.gotask/worker_release_test.go
…edge case Gavel-Issue-Id: 6b2997e35e373dc3d3209e33280094ab Claude-Session-Id: f7ecddc5-467d-4fd4-a7b8-f0778f653659
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
…ure drain race, blank-line loss, noColor default, StartGroup race
Introduce MCPToolHints struct to hold tool-level metadata including title, icon, group, parent, boolean hints (readOnly, destructive, idempotent, openWorld), default permission, and strict. Update entity building, action specs, and command annotation to use the new hints instead of a plain tool-group string. This enables richer MCP tool annotations and Clicky UI metadata inheritance.
…anonical mode names Introduce canonical tool permission modes (on/off/ask/auto) with backward compatibility for legacy labels. Add DefaultPermission, Parent, Icon, and Strict fields to ToolInfo and ToolCatalogEntry to control tool behavior without user preferences. Refactor effortConfig to delegate to captain's shared EffortConfig, removing duplicate provider-specific logic. Update tests to verify mode normalization, default permission filtering, and temperature gating.
Introduce ToolAnnotations struct for well-known MCP tool properties. Infer readOnly, destructive, idempotent hints from HTTP method/verb. Propagate tool hints (icon, group, parent, permission, strict) into _meta field. Update RPCOperation to carry ToolHints from clicky annotations.
Add mutex protection to Group and Task methods to fix data races. Introduce render lifecycle state machine (renderIdle, renderRunning, renderStopping) to allow the render loop to be restarted after stopRender, enabling subsequent enqueues to trigger fresh rendering. Implement prettyPlainDelta to emit only new log entries per PlainRender tick, preserving full log history for snapshots and final tree. Add plainSummaryText for a concise one-line summary after plain loop. Ensure renderFinal is idempotent and final output includes summary without duplicating task lines. Update Wait/WaitSilent to flush captured output via StopCapturingOutput. Fix WaitTime and StartTime to be thread-safe.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
aichat/tool_catalog.go (1)
105-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
customCatalogEntryconstructs a fullToolInfobut onlyGroupandNameare read.The
infovariable is used solely forpreferenceKey(info), which only accessesinfo.Groupandinfo.Name. Fields likeOperationName,Method,Path,ClickyVerb,ClickyScope,Parent,Icon,DefaultPermission, andStrictare set but never read. Additionally,defaultPermissionMode(def.DefaultPermission)is called twice — once ininfoand once in the return value.♻️ Simplify info construction in customCatalogEntry
func customCatalogEntry(def ToolDefinition, name string, schema map[string]any) ToolCatalogEntry { - info := ToolInfo{ - Name: name, - OperationName: def.Name, - Method: def.Method, - Path: def.Path, - ClickyVerb: def.Verb, - ClickyScope: def.Scope, - Group: def.Group, - Parent: def.Parent, - Icon: def.Icon, - DefaultPermission: defaultPermissionMode(def.DefaultPermission), - Strict: def.Strict, - } + info := ToolInfo{Name: name, Group: def.Group} return ToolCatalogEntry{ Name: name, Title: def.Name, Description: def.Description, Source: "custom", Group: def.Group, Parent: def.Parent, Icon: def.Icon, PreferenceKey: preferenceKey(info), DefaultPermission: defaultPermissionMode(def.DefaultPermission), Strict: def.Strict, Method: def.Method, Path: def.Path, OperationName: def.Name, InputSchema: objectSchema(schema), } }🤖 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 `@aichat/tool_catalog.go` around lines 105 - 131, customCatalogEntry is building a full ToolInfo even though preferenceKey(info) only needs Group and Name. Simplify the local info construction to include only the fields actually used by preferenceKey, and avoid setting unused fields like OperationName, Method, Path, ClickyVerb, ClickyScope, Parent, Icon, Strict, and DefaultPermission. Also compute defaultPermissionMode(def.DefaultPermission) once and reuse it in the ToolCatalogEntry return value instead of calling it twice.task/task.go (1)
689-698: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
prettyPlainDeltareturnsapi.Textbut is not namedPretty,PrettyFull, orPrettyRow.As per coding guidelines, a function that returns
api.Textmust be namedPretty,PrettyFull, orPrettyRow; otherwise it should return theapi.Textableinterface instead.prettyPlainDeltais an internal helper, so this is low-priority, but returningapi.Textablewould align with the guideline.♻️ Proposed refactor: return api.Textable from prettyPlainDelta
-func (t *Task) prettyPlainDelta() api.Text { +func (t *Task) prettyPlainDelta() api.Textable { t.mu.Lock() defer t.mu.Unlock() text, total := t.prettyWithLogOffset(t.plainLogsRendered) t.plainLogsRendered = total return text }Note: callers in
render.gothat userendered.String()/rendered.ANSI()would need to resolve theapi.Textablefirst (e.g., via.ToText()or equivalent), so verify the call sites before applying.🤖 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 `@task/task.go` around lines 689 - 698, prettyPlainDelta currently returns api.Text, which conflicts with the naming guideline for non-Pretty helpers. Update Task.prettyPlainDelta to return api.Textable instead, and adjust the render.go callers that consume its result so they explicitly resolve it to text before calling String() or ANSI(). Keep the existing behavior of prettyWithLogOffset and plainLogsRendered unchanged while making the return type consistent with the api.Textable contract.Source: Coding guidelines
mcp/registry.go (1)
87-91: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
operationToolHintsis computed twice perNewMcpToolcall.Both
toolAnnotations(line 96) andclickyToolMeta(line 155) independently calloperationToolHints(rpcOp), duplicating the merge work. Consider computing it once inNewMcpTooland passing the result to both helpers.♻️ Proposed refactor: compute hints once in NewMcpTool
func NewMcpTool(rpcOp *rpc.RPCOperation) *ToolDefinition { + hints := operationToolHints(rpcOp) // Convert RPC schema to MCP schema inputSchema := Schema{ Type: rpcOp.Schema.Type, Properties: make(map[string]Property), Required: rpcOp.Schema.Required, } // ... schema conversion unchanged ... appName := "app" if rpcOp.Command != nil { appName = rpcOp.Command.RootName() } return &ToolDefinition{ Name: rpcOp.Name, Title: fmt.Sprintf("%s %s", appName, rpcOp.Name), Description: rpcOp.Description, InputSchema: inputSchema, - Annotations: toolAnnotations(rpcOp), - Meta: clickyToolMeta(rpcOp), + Annotations: toolAnnotationsFromHints(hints, rpcOp), + Meta: clickyToolMetaFromHints(hints), Command: rpcOp.Command, } }Also applies to: 95-96, 154-155
🤖 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/registry.go` around lines 87 - 91, `operationToolHints` is being recomputed twice inside `NewMcpTool` via `toolAnnotations` and `clickyToolMeta`; compute the hints once in `NewMcpTool` and pass them into both helpers instead of calling `operationToolHints(rpcOp)` separately. Update the signatures of `toolAnnotations` and `clickyToolMeta` as needed, keeping the behavior of `Annotations`, `Meta`, and `Command` unchanged.task/render.go (1)
185-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
api.Text{}.Append(...)instead ofapi.Text{...}struct literal.Line 221 uses
api.Text{Content: summary, Style: "text-gray-400"}. As per coding guidelines, when buildingapi.Text, useclicky.Text(...)or chainedapi.Text{}.Append(...)calls; do not useapi.Text{...}struct literals orChildren:slice literals.Additionally, verify that
plainSummaryTextis actually called — it does not appear in the provided code context or graph references. If it is dead code, consider removing it or wiring it into the final render path.♻️ Proposed refactor
- return api.Text{Content: summary, Style: "text-gray-400"} + return api.Text{}.Append(summary, "text-gray-400")🤖 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 `@task/render.go` around lines 185 - 222, plainSummaryText currently returns api.Text via a struct literal, which violates the text-building convention. Update plainSummaryText to build the summary using api.Text{}.Append(...) (or clicky.Text(...)) instead of api.Text{Content: ..., Style: ...}. Also check whether plainSummaryText is referenced by the render flow; if it is unused, remove it or wire it into the final render path.Source: Coding guidelines
task/task_logs_render.go (1)
20-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
api.Text{}.Append(...)instead ofapi.Text{...}struct literals.Line 105–108 uses
api.Text{Content: ..., Style: ...}struct literals. As per coding guidelines, when buildingapi.Text, useclicky.Text(...)or chainedapi.Text{}.Append(...)calls; do not useapi.Text{...}struct literals orChildren:slice literals.♻️ Proposed refactor
- text.Children = append(text.Children, api.Text{ - Content: fmt.Sprintf("\n%s", lo.Ellipsis(log.Message, 500)), - Style: logStyle, - }) + text.Children = append(text.Children, api.Text{}. + Append(fmt.Sprintf("\n%s", lo.Ellipsis(log.Message, 500)), logStyle))🤖 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 `@task/task_logs_render.go` around lines 20 - 112, The log rendering in prettyWithLogOffset is building child entries with an api.Text struct literal, which violates the api.Text construction guideline. Update the log append path in Task.prettyWithLogOffset to build each child using api.Text{}.Append(...) or clicky.Text(...) instead of api.Text{Content: ..., Style: ...}, and keep the existing log-style selection and ellipsis handling intact.Source: Coding guidelines
🤖 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 `@aichat/tool_catalog.go`:
- Around line 208-232: `applyToolMetadata` treats `DefaultPermission`
differently from `Group`, `Parent`, `Icon`, and `Strict`, so update the function
in `tool_catalog.go` to make the overwrite behavior consistent. Either change
`DefaultPermission` to only set when `entry.DefaultPermission` is still unset,
matching the fill-if-empty pattern used by the other fields, or intentionally
make the other fields always overwrite too if metadata should be authoritative.
Use the existing `stringMetadata`, `boolMetadata`, and `defaultPermissionMode`
flow in `applyToolMetadata` as the place to apply the consistency fix.
In `@task/manager_lifecycle.go`:
- Around line 172-201: `finishRenderTeardown` has a race where `renderFinal` can
run after a new render loop has already started, causing the next batch’s final
output to be skipped. Update the teardown flow in `Manager.finishRenderTeardown`
to check `renderState` while holding `tm.mu` and only call `renderFinal` when no
new loop is already `renderRunning`; keep the existing `cleanupTerminal`,
`uninstallLogSerializer`, and `releaseRenderTerminal` logic tied to the locked
teardown decision.
---
Nitpick comments:
In `@aichat/tool_catalog.go`:
- Around line 105-131: customCatalogEntry is building a full ToolInfo even
though preferenceKey(info) only needs Group and Name. Simplify the local info
construction to include only the fields actually used by preferenceKey, and
avoid setting unused fields like OperationName, Method, Path, ClickyVerb,
ClickyScope, Parent, Icon, Strict, and DefaultPermission. Also compute
defaultPermissionMode(def.DefaultPermission) once and reuse it in the
ToolCatalogEntry return value instead of calling it twice.
In `@mcp/registry.go`:
- Around line 87-91: `operationToolHints` is being recomputed twice inside
`NewMcpTool` via `toolAnnotations` and `clickyToolMeta`; compute the hints once
in `NewMcpTool` and pass them into both helpers instead of calling
`operationToolHints(rpcOp)` separately. Update the signatures of
`toolAnnotations` and `clickyToolMeta` as needed, keeping the behavior of
`Annotations`, `Meta`, and `Command` unchanged.
In `@task/render.go`:
- Around line 185-222: plainSummaryText currently returns api.Text via a struct
literal, which violates the text-building convention. Update plainSummaryText to
build the summary using api.Text{}.Append(...) (or clicky.Text(...)) instead of
api.Text{Content: ..., Style: ...}. Also check whether plainSummaryText is
referenced by the render flow; if it is unused, remove it or wire it into the
final render path.
In `@task/task_logs_render.go`:
- Around line 20-112: The log rendering in prettyWithLogOffset is building child
entries with an api.Text struct literal, which violates the api.Text
construction guideline. Update the log append path in Task.prettyWithLogOffset
to build each child using api.Text{}.Append(...) or clicky.Text(...) instead of
api.Text{Content: ..., Style: ...}, and keep the existing log-style selection
and ellipsis handling intact.
In `@task/task.go`:
- Around line 689-698: prettyPlainDelta currently returns api.Text, which
conflicts with the naming guideline for non-Pretty helpers. Update
Task.prettyPlainDelta to return api.Textable instead, and adjust the render.go
callers that consume its result so they explicitly resolve it to text before
calling String() or ANSI(). Keep the existing behavior of prettyWithLogOffset
and plainLogsRendered unchanged while making the return type consistent with the
api.Textable contract.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9190406-4cd9-4c9c-ba2c-b19f71364a32
📒 Files selected for processing (37)
aichat/approval.goaichat/approval_test.goaichat/genkit.goaichat/models.goaichat/models_test.goaichat/tool_catalog.goaichat/tool_registry.goaichat/tool_registry_test.goapi/table_test.goentity/annotations.goentity/builder.goentity/command.goentity/entity.goentity/operation.goentity/sub_command.goentity/tool_hints.goentity/toolgroup_test.goentity_aliases.gomcp/registry.gomcp/registry_test.gorpc/converter.gorpc/converter_toolgroup_test.gotask/group.gotask/group_race_test.gotask/manager.gotask/manager_color_test.gotask/manager_lifecycle.gotask/manager_output.gotask/manager_output_test.gotask/manager_wait.gotask/render.gotask/render_dedupe_test.gotask/render_hook_test.gotask/render_restart_test.gotask/task.gotask/task_log_display_test.gotask/task_logs_render.go
✅ Files skipped from review due to trivial changes (1)
- api/table_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- rpc/converter.go
- aichat/models.go
|
|
||
| func applyToolMetadata(entry *ToolCatalogEntry, meta map[string]any) { | ||
| if entry == nil || len(meta) == 0 { | ||
| return | ||
| } | ||
| if group, ok := stringMetadata(meta, "group", "toolGroup"); ok && entry.Group == "" { | ||
| entry.Group = group | ||
| entry.PreferenceKey = group | ||
| } | ||
| if parent, ok := stringMetadata(meta, "parent", "toolParent"); ok && entry.Parent == "" { | ||
| entry.Parent = parent | ||
| } | ||
| if icon, ok := stringMetadata(meta, "icon", "toolIcon"); ok && entry.Icon == "" { | ||
| entry.Icon = icon | ||
| } | ||
| if permission, ok := stringMetadata(meta, "defaultPermission", "permission", "defaultMode"); ok { | ||
| entry.DefaultPermission = defaultPermissionMode(ToolMode(permission)) | ||
| } | ||
| if strict, ok := boolMetadata(meta, "strict"); ok && entry.Strict == nil { | ||
| entry.Strict = &strict | ||
| } | ||
| if nested, ok := meta["com.flanksource.clicky/tool"].(map[string]any); ok { | ||
| applyToolMetadata(entry, nested) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
applyToolMetadata inconsistently overrides DefaultPermission while other fields use fill-if-empty.
Group, Parent, Icon, and Strict only overwrite when the entry field is empty/nil, but DefaultPermission is always overwritten when the metadata key exists. This means metadata from a tool definition can override the info-derived DefaultPermission but cannot override other info-derived fields. If this is intentional (metadata is more authoritative for permissions), the other fields should follow the same pattern; if not, DefaultPermission should also check before overwriting.
🐛 Proposed fix for consistent fill-if-empty behavior
if permission, ok := stringMetadata(meta, "defaultPermission", "permission", "defaultMode"); ok && entry.DefaultPermission == "" {
entry.DefaultPermission = defaultPermissionMode(ToolMode(permission))
}Or, if metadata should always take precedence, remove the empty checks from the other fields for consistency.
📝 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.
| func applyToolMetadata(entry *ToolCatalogEntry, meta map[string]any) { | |
| if entry == nil || len(meta) == 0 { | |
| return | |
| } | |
| if group, ok := stringMetadata(meta, "group", "toolGroup"); ok && entry.Group == "" { | |
| entry.Group = group | |
| entry.PreferenceKey = group | |
| } | |
| if parent, ok := stringMetadata(meta, "parent", "toolParent"); ok && entry.Parent == "" { | |
| entry.Parent = parent | |
| } | |
| if icon, ok := stringMetadata(meta, "icon", "toolIcon"); ok && entry.Icon == "" { | |
| entry.Icon = icon | |
| } | |
| if permission, ok := stringMetadata(meta, "defaultPermission", "permission", "defaultMode"); ok { | |
| entry.DefaultPermission = defaultPermissionMode(ToolMode(permission)) | |
| } | |
| if strict, ok := boolMetadata(meta, "strict"); ok && entry.Strict == nil { | |
| entry.Strict = &strict | |
| } | |
| if nested, ok := meta["com.flanksource.clicky/tool"].(map[string]any); ok { | |
| applyToolMetadata(entry, nested) | |
| } | |
| } | |
| func applyToolMetadata(entry *ToolCatalogEntry, meta map[string]any) { | |
| if entry == nil || len(meta) == 0 { | |
| return | |
| } | |
| if group, ok := stringMetadata(meta, "group", "toolGroup"); ok && entry.Group == "" { | |
| entry.Group = group | |
| entry.PreferenceKey = group | |
| } | |
| if parent, ok := stringMetadata(meta, "parent", "toolParent"); ok && entry.Parent == "" { | |
| entry.Parent = parent | |
| } | |
| if icon, ok := stringMetadata(meta, "icon", "toolIcon"); ok && entry.Icon == "" { | |
| entry.Icon = icon | |
| } | |
| if permission, ok := stringMetadata(meta, "defaultPermission", "permission", "defaultMode"); ok && entry.DefaultPermission == "" { | |
| entry.DefaultPermission = defaultPermissionMode(ToolMode(permission)) | |
| } | |
| if strict, ok := boolMetadata(meta, "strict"); ok && entry.Strict == nil { | |
| entry.Strict = &strict | |
| } | |
| if nested, ok := meta["com.flanksource.clicky/tool"].(map[string]any); ok { | |
| applyToolMetadata(entry, nested) | |
| } | |
| } |
🤖 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 `@aichat/tool_catalog.go` around lines 208 - 232, `applyToolMetadata` treats
`DefaultPermission` differently from `Group`, `Parent`, `Icon`, and `Strict`, so
update the function in `tool_catalog.go` to make the overwrite behavior
consistent. Either change `DefaultPermission` to only set when
`entry.DefaultPermission` is still unset, matching the fill-if-empty pattern
used by the other fields, or intentionally make the other fields always
overwrite too if metadata should be authoritative. Use the existing
`stringMetadata`, `boolMetadata`, and `defaultPermissionMode` flow in
`applyToolMetadata` as the place to apply the consistency fix.
| // finishRenderTeardown emits the final frame where needed and restores | ||
| // terminal and logger state. Shared by both stopRender paths: after a running | ||
| // loop exits (loopWasRunning), and when no loop ever ran — noProgress/CI mode | ||
| // prints its final tree here. | ||
| func (tm *Manager) finishRenderTeardown(loopWasRunning bool) { | ||
| // In interactive mode the stop branch of renderLoop already emitted | ||
| // the authoritative final frame (via interactiveRender, which | ||
| // ClearLines(lastLines)+writes atomically). Calling renderFinal | ||
| // here would append a second copy of the summary below the live | ||
| // frame, doubling every summary line. PlainRender-based mode gets its | ||
| // closing output from renderFinal: the loop's stop branch already | ||
| // flushed every dirty task via PlainRender, so a running loop only | ||
| // needs the one-line summary; when no loop ran, nothing has been | ||
| // printed yet and the full tree is emitted. | ||
| if !tm.noRender.Load() && !tm.isInteractive.Load() { | ||
| tm.renderFinal(loopWasRunning) | ||
| } | ||
| tm.cleanupTerminal() | ||
| // On the idle path, only tear down while still idle: a concurrent enqueue | ||
| // may have restarted the loop, which still needs its freshly installed | ||
| // serializer and TTY ownership (its own stopRender releases them). | ||
| tm.mu.Lock() | ||
| teardown := loopWasRunning || tm.renderState == renderIdle | ||
| if teardown { | ||
| tm.uninstallLogSerializer() | ||
| } | ||
| tm.mu.Unlock() | ||
| if teardown { | ||
| tm.releaseRenderTerminal() | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Race condition: renderFinal can emit after a new loop starts, suppressing the new batch's final output.
In finishRenderTeardown, renderFinal (line 187) is called before the teardown check (line 194). When stopRender takes the renderIdle path, it unlocks tm.mu at line 167 before calling finishRenderTeardown(false). If a concurrent enqueue resets finalRendered and startRenderLoop transitions renderState to renderRunning in that window, renderFinal will emit the full tree and set finalRendered = true. The new loop's eventual stopRender → renderFinal then sees finalRendered == true and skips, losing the new batch's final summary.
The fix is to guard renderFinal with a renderState == renderRunning check while already holding tm.mu:
🔒 Proposed fix: skip renderFinal when a new loop is already running
func (tm *Manager) renderFinal(loopRan bool) {
if tm.noRender.Load() {
return
}
tm.mu.Lock()
- if tm.finalRendered || len(tm.tasks) == 0 {
+ if tm.finalRendered || len(tm.tasks) == 0 || tm.renderState == renderRunning {
tm.mu.Unlock()
return
}This is race-free: startRenderLoop sets renderState = renderRunning under tm.mu, and renderFinal checks it under tm.mu. In the normal stop path, renderState is renderStopping when renderFinal runs, so the check doesn't interfere.
🤖 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 `@task/manager_lifecycle.go` around lines 172 - 201, `finishRenderTeardown` has
a race where `renderFinal` can run after a new render loop has already started,
causing the next batch’s final output to be skipped. Update the teardown flow in
`Manager.finishRenderTeardown` to check `renderState` while holding `tm.mu` and
only call `renderFinal` when no new loop is already `renderRunning`; keep the
existing `cleanupTerminal`, `uninstallLogSerializer`, and
`releaseRenderTerminal` logic tied to the locked teardown decision.
1 participant
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
New Features
Server-Timingresponse headers.Bug Fixes