build(db): Update commons-db to v0.1.14 - #145
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
…edge case Gavel-Issue-Id: 6b2997e35e373dc3d3209e33280094ab Claude-Session-Id: f7ecddc5-467d-4fd4-a7b8-f0778f653659
…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.
feat(entity): list/get default to auto-run and create/update/delete to ask at registration; EntityBuilder.ToolPermission and ActionSpec.WithToolPermission override; promoted entity roots inherit the list permission
Replace duplicated repository-specific Grite instructions with a pointer to shared agent guidance, project memory, and Clicky skills. This keeps workflow documentation centralized and easier to maintain.
Add semantic column metadata and structured value rendering while preserving raw values for exports. Improve terminal width calculations so capped columns behave correctly in wide and narrow terminals.
Add bounded-memory streaming exports for JSON, NDJSON, YAML, CSV, Markdown, HTML, Excel, and PDF. Expose export capabilities through operation metadata and support NDJSON content negotiation. Render paged clicky-json responses as table documents with pagination retained in headers. BREAKING CHANGE: Paged clicky-json responses no longer wrap rows in the data/page envelope.
Refresh related transitive dependencies to align with the updated database module.
WalkthroughChangesThe pull request updates AI model and tool metadata integration, structured and streaming formatters, RPC and MCP contracts, task rendering and concurrency, frontend linting, documentation, and dependency manifests. AI catalog and tool integration
Entity, RPC, and MCP contracts
Structured and streaming formatting
Task runtime
Project tooling and examples
Sequence Diagram(s)sequenceDiagram
participant EntityBuilder
participant RPCConverter
participant MCPRegistry
participant AichatToolCatalog
EntityBuilder->>RPCConverter: propagate MCPToolHints
RPCConverter->>MCPRegistry: convert operation tool metadata
MCPRegistry->>AichatToolCatalog: expose annotations and Clicky metadata
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
|
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.
|
Gavel crashed before producing resultsExit code: 1 Last lines of gavel.logFull |
Delegate model pricing to captain's generated catalog so aliases and dated model snapshots stay current, while retaining explicit rates for non-catalog models. Remove MCP server configuration and tool registration from the chat server. BREAKING CHANGE: Remove Options.MCPServers, MCPServer, MCPRegisteredTools, and MCPTools. Claude-Session-Id: 15389a35-778b-486b-bb10-0be18fee5b76
Refreshes Clicky, Captain, Commons DB, and transitive Go dependencies across aichat and the entity example. Aligns the example modules with Clicky 1.21.48 and updates dependency checksums without changing application behavior.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
task/manager.go (1)
441-463: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset
finalRenderedbefore you start the render loop.
enqueuestarts the render loop at Line 447, then resetsfinalRenderedat Line 462. IfstopRenderruns between these two points,renderFinalobserves the stalefinalRendered == truefrom the previous batch and prints no closing output for the new batch. Move the state reset ahead of the loop start so the new batch is always armed.🐛 Proposed reordering
func (tm *Manager) enqueue(task *Task) *Task { + tm.mu.Lock() + tm.finalRendered = false + idle := tm.renderState == renderIdle + tm.mu.Unlock() + if !tm.noRender.Load() && !tm.noProgress.Load() { - tm.mu.RLock() - idle := tm.renderState == renderIdle - tm.mu.RUnlock() if idle { tm.startRenderLoop() } } @@ tm.mu.Lock() tm.tasks = append(tm.tasks, task) - tm.finalRendered = false tm.mu.Unlock()🤖 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.go` around lines 441 - 463, Move the tm.finalRendered = false reset in Manager.enqueue ahead of the startRenderLoop call, ensuring it occurs before any render-loop startup can race with stopRender; preserve the existing task enqueue and synchronization behavior.
🧹 Nitpick comments (8)
formatters/stream.go (2)
366-374: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBuild the textable only for the HTML branch.
streamCellcallsapi.ColumnTextableon every cell, but the default branch discardstextand callsapi.ColumnString, which decodes the same value again. For CSV and Markdown exports this doubles the decode and allocation work per structured cell. The million-row smoke test informatters/stream_test.goexercises this path.♻️ Proposed refactor
func streamCell(value any, column api.ColumnDef, format string) string { - text := api.ColumnTextable(column, value) switch format { case "html": - return text.HTML() + return api.ColumnTextable(column, value).HTML() default: return api.ColumnString(column, value) } }🤖 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/stream.go` around lines 366 - 374, Update streamCell so api.ColumnTextable is created only inside the "html" case; return api.ColumnString directly for the default branch, preserving the existing output while avoiding redundant decoding and allocation.
132-161: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDefine the behavior when
MaxRowsis exceeded mid-stream.
emitRowreturns an error after previous rows are written. The writer then holds a partial document, for example a JSON array without its closing bracket. Callers that already flushed HTTP headers cannot replace the body with an error response.Consider stopping at the limit, closing the document, and reporting truncation through a distinct sentinel error or a return value. If the current behavior is intentional, document that
wcan hold partial output when the function returns an error.🤖 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/stream.go` around lines 132 - 161, Update streamRows and its emitRow limit handling to define an explicit mid-stream MaxRows behavior: stop emitting once maxRows is reached, preserve a valid closed document by allowing the caller to finalize output, and report truncation through a distinct sentinel error or return status that callers can handle after headers are flushed. If existing partial-output-on-error behavior must remain, document that contract at the streamRows API and ensure callers handle it consistently.task/task_logs_render.go (1)
105-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild the log entry with
Append.The manual
api.Text{...}literal violates the rendering guideline. Append the formatted log entry totextwith its style instead.Proposed fix
- text.Children = append(text.Children, api.Text{ - Content: fmt.Sprintf("\n%s", lo.Ellipsis(log.Message, 500)), - Style: logStyle, - }) + text = 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 105 - 108, Update the log rendering code to use the `text.Append` method for the formatted, truncated log message and its `logStyle` instead of manually appending an `api.Text` literal to `text.Children`. Preserve the existing newline prefix and 500-character ellipsis behavior.Source: Coding guidelines
aichat/strict_tools_test.go (1)
136-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the tool count from
anthropicMaxStrictTools.Line 138 hard-codes 21 tools, but Lines 143 and 146 assert against
anthropicMaxStrictTools. If the constant changes, the loop bound does not follow and both assertions become incorrect for the wrong reason.♻️ Proposed change
- tools := make([]registeredTool, 0, 21) - for i := range 21 { + const total = anthropicMaxStrictTools + 1 + tools := make([]registeredTool, 0, total) + for i := range total { name := fmt.Sprintf("tool_%02d", i) tools = append(tools, registeredTool{ref: strictTestTool(name), info: ToolInfo{Name: name, Strict: boolPointer(true)}}) } - selected := registeredToolsForRequest(tools, ToolPreferences{"tool_20": ToolModeOff}) + selected := registeredToolsForRequest(tools, ToolPreferences{fmt.Sprintf("tool_%02d", total-1): ToolModeOff})🤖 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/strict_tools_test.go` around lines 136 - 149, Update TestAnthropicStrictToolsMiddlewareCountsAfterPreferences to derive the generated tool count from anthropicMaxStrictTools, including the additional tool needed to exercise the limit after disabling tool_20. Keep the existing assertions against anthropicMaxStrictTools and preserve the preference-selection scenario.aichat/models_test.go (1)
99-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese tests couple to captain-owned identifiers and an internal budget constant.
Line 59 states the reason to sample the catalog dynamically: captain owns the menu, so specific ids retire on a version bump. The following assertions contradict that reason:
- Lines 100-104 hard-code three Anthropic ids.
- Line 129 asserts that
openai/gpt-4o-miniproduces no effort configuration. The assertion inverts if captain later adds that model to its registry.- Line 149 asserts
max_tokens == 24576+1000.24576is captain's internal thinking budget for the model. The test breaks when captain retunes it.Derive these values from the catalog, or export the expected budget from captain instead of repeating the literal.
Also applies to: 125-129, 148-152
🤖 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/models_test.go` around lines 99 - 113, Decouple the tests from captain-owned identifiers and constants: update TestAnthropicCatalogIncludesRequestedModels to iterate over Anthropic models discovered from the catalog, make the effort-configuration assertion derive its expectation from the catalog rather than assuming openai/gpt-4o-mini lacks one, and replace the literal 24576 budget in the max_tokens assertion with the catalog-derived value or an exported captain budget symbol. Preserve the existing provider and configuration validations.aichat/agent_test.go (1)
120-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the registration result and reset the global catalog.
Line 124 discards the
RegisterModelerror. If registration fails,LookupModeldoes not route to the agent path and the failure surfaces later as a confusing assertion error. The call also mutates the process-global catalog with no cleanup, so the test model stays visible to every later test in the package, includingTestAgentModelsInCatalog.Pass
*testing.TintoagentServer, fail on the error, and registerResetModelCatalogas cleanup.♻️ Proposed change
-func agentServer(opts Options, fake *fakeStreamProvider, configs *[]capapi.Config) *Server { +func agentServer(t *testing.T, opts Options, fake *fakeStreamProvider, configs *[]capapi.Config) *Server { + t.Helper() // Register a stable test agent model so LookupModel routes these requests to // the agent path regardless of the concrete ids captain's catalog ships. The // upsert is idempotent and process-global; a fake provider factory serves the // turn, so no real backend is contacted. - _ = RegisterModel(Model{ID: "claude-agent-sonnet", Backend: capapi.BackendClaudeAgent, Label: "Claude Agent · Sonnet (test)", Reasoning: true, ContextWindow: 200000}) + if err := RegisterModel(Model{ID: "claude-agent-sonnet", Backend: capapi.BackendClaudeAgent, Label: "Claude Agent · Sonnet (test)", Reasoning: true, ContextWindow: 200000}); err != nil { + t.Fatalf("RegisterModel: %v", err) + } + t.Cleanup(ResetModelCatalog)Update every
agentServercaller accordingly.🤖 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/agent_test.go` around lines 120 - 124, Update agentServer to accept *testing.T, assert that RegisterModel succeeds, and register ResetModelCatalog with t.Cleanup after the test model is added. Update every agentServer caller to pass its testing handle, preserving the existing agent-routing behavior while restoring the global catalog after each test.aichat/server.go (1)
412-416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMigrate to
ai.WithUse.ai.WithUserequires the newerai.Middlewareinterface. UpdateanthropicStrictToolsMiddlewareif it returns the deprecatedai.ModelMiddleware; replacing the option alone is not sufficient.🤖 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/server.go` around lines 412 - 416, Update the Anthropic middleware setup in the model configuration flow to use ai.WithUse instead of ai.WithMiddleware, and change anthropicStrictToolsMiddleware to return and implement the newer ai.Middleware interface required by ai.WithUse. Preserve the existing selected-tools behavior and only append the option when the middleware is non-nil.Source: Linters/SAST tools
aichat/strict_tools.go (1)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMigrate the strict-tools middleware to Genkit’s current middleware API.
ai.ModelMiddlewareandai.WithMiddlewareare deprecated in v1.10.0. Adapt the wrapper withai.MiddlewareFuncandai.Hooks{WrapModel: ...}, then pass it throughai.WithUse.🤖 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/strict_tools.go` at line 20, The anthropicStrictToolsMiddleware wrapper still uses deprecated middleware APIs; migrate it to ai.MiddlewareFunc with ai.Hooks{WrapModel: ...}, and update the middleware application to use ai.WithUse instead of ai.WithMiddleware while preserving the existing strict-tools behavior.Source: Linters/SAST tools
🤖 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/column_value.go`:
- Around line 186-191: Update normalizeJSONColumnValue so a nil input returns
failure rather than success with a nil payload, allowing callers to use the
shared empty-cell fallback. Preserve the existing decodeStructuredColumnValue
behavior for non-nil values.
- Around line 193-234: Update decodeStructuredColumnValue to decode JSON through
a shared helper using json.Decoder.UseNumber() instead of the three
json.Unmarshal calls, preserving json.Number values for large integers in every
string, []byte, and marshaled-value branch. Keep the existing validation,
container checks, and fallback behavior unchanged.
- Around line 84-130: Restrict keyValuePairFromObject to return a shortcut pair
only when the object contains the recognized key/name and value fields with no
additional fields. For objects with extra fields, return false so
keyValuePairsFromDecoded falls back to normal map expansion and preserves all
data in exports. Apply this validation consistently for both the map[string]any
and []any paths.
In `@api/table_test.go`:
- Around line 36-42: Update the width-test fixtures and assertions around
cappedWidthTableRow and the narrow table test so they use values longer than the
configured MaxWidth limits, including an over-limit wide-fixture value. Add an
explicit assertion that every narrow rendered line stays within terminal width
30, while preserving the existing content assertions.
In `@entity/entity.go`:
- Around line 638-644: Update RegisterEntity when constructing adminInfo to copy
admin.ToolHints and admin.ToolGroup, then synchronize the group fields using the
same fallback behavior as the primary EntityInfo. Ensure generated admin
commands retain the admin group, permissions, strictness, and safety metadata.
In `@entity/toolpermission_test.go`:
- Line 91: Remove the synthetic Cobra RunE handlers from the test fixtures: in
entity/toolpermission_test.go lines 91-91, remove the handler or create the list
operation through entity.AddCommand; in rpc/converter_flags_test.go lines 28-32
and rpc/converter_toolgroup_test.go lines 15-19, remove the direct RunE
handlers. Keep runnable behavior only through the entity registration path when
required.
In `@examples/enitity/webapp/src/ChatWidget.tsx`:
- Around line 15-18: Update the className values in TYPE_CONFIG to use semantic
Clicky UI theme tokens instead of fixed palette classes for stack, cluster, and
team. Preserve each context type’s intended visual distinction, then verify the
change with the lint:clicky-ui script.
In `@examples/enitity/webapp/tsconfig.json`:
- Line 18: Update the webapp TypeScript configuration’s compilerOptions.types to
include both “vite/client” and “node”, and ensure the project has `@types/node`
available so vite.config.ts can resolve __dirname. Verify with the specified tsc
--noEmit command.
In `@formatters/stream.go`:
- Around line 277-288: Update the HTML row-rendering callback in streamRows to
escape scalar cell content before writing it into <td>, while preserving the
output of structured renderers. Reuse the existing HTML-escaping utility or
renderer behavior, and add a regression test covering executable markup supplied
through a scalar row value.
In `@mcp/registry.go`:
- Around line 125-137: The metadata switch must prioritize generic action
semantics when verb is "action", including POST actions. Move the verb ==
"action" case before the POST and other HTTP-method cases so it returns
conservative destructive metadata, and add a test covering a POST action.
In `@rpc/http/middleware.go`:
- Around line 56-63: Update the timingRecorder wrapping flow around
timingRecorder and its Flush method so the recorder does not always implement
http.Flusher; return/use a separate flushing wrapper only when the original
ResponseWriter supports http.Flusher, while preserving timestamping and
forwarding behavior. Add coverage verifying a non-flushing writer does not
expose http.Flusher.
In `@task/group.go`:
- Around line 329-340: Update Duration to snapshot g.startTime and g.Items under
one g.mu.RLock/RUnlock section, then iterate over that captured item slice
instead of calling GetTasks separately. Add a regression test that interleaves
two Add calls with Duration and verifies the duration never combines state from
different group snapshots.
In `@task/manager_lifecycle.go`:
- Around line 186-201: Evaluate the teardown decision once at the start of this
shutdown sequence, before renderFinal and cleanupTerminal, using the existing
loopWasRunning/renderState state under tm.mu as needed. Gate renderFinal,
cleanupTerminal, uninstallLogSerializer, and releaseRenderTerminal on that same
decision so an idle-path enqueue that restarts the loop cannot affect the live
renderer or terminal ownership.
---
Outside diff comments:
In `@task/manager.go`:
- Around line 441-463: Move the tm.finalRendered = false reset in
Manager.enqueue ahead of the startRenderLoop call, ensuring it occurs before any
render-loop startup can race with stopRender; preserve the existing task enqueue
and synchronization behavior.
---
Nitpick comments:
In `@aichat/agent_test.go`:
- Around line 120-124: Update agentServer to accept *testing.T, assert that
RegisterModel succeeds, and register ResetModelCatalog with t.Cleanup after the
test model is added. Update every agentServer caller to pass its testing handle,
preserving the existing agent-routing behavior while restoring the global
catalog after each test.
In `@aichat/models_test.go`:
- Around line 99-113: Decouple the tests from captain-owned identifiers and
constants: update TestAnthropicCatalogIncludesRequestedModels to iterate over
Anthropic models discovered from the catalog, make the effort-configuration
assertion derive its expectation from the catalog rather than assuming
openai/gpt-4o-mini lacks one, and replace the literal 24576 budget in the
max_tokens assertion with the catalog-derived value or an exported captain
budget symbol. Preserve the existing provider and configuration validations.
In `@aichat/server.go`:
- Around line 412-416: Update the Anthropic middleware setup in the model
configuration flow to use ai.WithUse instead of ai.WithMiddleware, and change
anthropicStrictToolsMiddleware to return and implement the newer ai.Middleware
interface required by ai.WithUse. Preserve the existing selected-tools behavior
and only append the option when the middleware is non-nil.
In `@aichat/strict_tools_test.go`:
- Around line 136-149: Update
TestAnthropicStrictToolsMiddlewareCountsAfterPreferences to derive the generated
tool count from anthropicMaxStrictTools, including the additional tool needed to
exercise the limit after disabling tool_20. Keep the existing assertions against
anthropicMaxStrictTools and preserve the preference-selection scenario.
In `@aichat/strict_tools.go`:
- Line 20: The anthropicStrictToolsMiddleware wrapper still uses deprecated
middleware APIs; migrate it to ai.MiddlewareFunc with ai.Hooks{WrapModel: ...},
and update the middleware application to use ai.WithUse instead of
ai.WithMiddleware while preserving the existing strict-tools behavior.
In `@formatters/stream.go`:
- Around line 366-374: Update streamCell so api.ColumnTextable is created only
inside the "html" case; return api.ColumnString directly for the default branch,
preserving the existing output while avoiding redundant decoding and allocation.
- Around line 132-161: Update streamRows and its emitRow limit handling to
define an explicit mid-stream MaxRows behavior: stop emitting once maxRows is
reached, preserve a valid closed document by allowing the caller to finalize
output, and report truncation through a distinct sentinel error or return status
that callers can handle after headers are flushed. If existing
partial-output-on-error behavior must remain, document that contract at the
streamRows API and ensure callers handle it consistently.
In `@task/task_logs_render.go`:
- Around line 105-108: Update the log rendering code to use the `text.Append`
method for the formatted, truncated log message and its `logStyle` instead of
manually appending an `api.Text` literal to `text.Children`. Preserve the
existing newline prefix and 500-character ellipsis behavior.
🪄 Autofix
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 Plus
Run ID: d0b4b692-18f7-41a0-80ea-2235d4565bf8
⛔ Files ignored due to path filters (4)
aichat/go.sumis excluded by!**/*.sumexamples/enitity/go.sumis excluded by!**/*.sumexamples/enitity/webapp/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlvalkey/go.sumis excluded by!**/*.sum
📒 Files selected for processing (96)
.gitignoreAGENTS.mdCONTRIBUTING.mdMakefileaichat/agent.goaichat/agent_test.goaichat/approval.goaichat/approval_test.goaichat/cost.goaichat/cost_test.goaichat/genkit.goaichat/go.modaichat/models.goaichat/models_test.goaichat/server.goaichat/strict_tools.goaichat/strict_tools_test.goaichat/tool_catalog.goaichat/tool_registry.goaichat/tool_registry_test.goaichat/tools_clicky.goaichat/tools_clicky_test.goaichat/tools_mcp.goapi/column.goapi/column_value.goapi/column_value_test.goapi/table.goapi/table_test.goapi/types.goentity/annotations.goentity/builder.goentity/command.goentity/entity.goentity/operation.goentity/sub_command.goentity/tool_hints.goentity/toolgroup_test.goentity/toolpermission_test.goentity_aliases.goexamples/enitity/go.modexamples/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/html_react_formatter.goformatters/html_react_formatter_test.goformatters/http/http.goformatters/map_input_test.goformatters/markdown_formatter.goformatters/options.goformatters/stream.goformatters/stream_test.gomcp/registry.gomcp/registry_test.gorpc/converter.gorpc/converter_flags_test.gorpc/converter_toolgroup_test.gorpc/entities_test.gorpc/http/middleware.gorpc/http/middleware_test.gorpc/http/timing.gorpc/http/timing_test.gorpc/openapi.gorpc/serve.gorpc/serve_test.gorpc/types.goskills/clicky-tasks/SKILL.mdskills/clicky-tasks/agents/openai.yamlskills/clicky-tasks/references/refactoring-patterns.mdskills/pretty-printing/SKILL.mdskills/pretty-printing/agents/openai.yamlskills/pretty-printing/references/api-patterns.mdtask/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.gotask/worker.gotask/worker_release_test.go
💤 Files with no reviewable changes (1)
- aichat/tools_mcp.go
| func keyValuePairsFromDecoded(value any) ([]KeyValuePair, bool) { | ||
| switch typed := value.(type) { | ||
| case map[string]any: | ||
| if pair, ok := keyValuePairFromObject(typed); ok { | ||
| return []KeyValuePair{pair}, true | ||
| } | ||
| keys := make([]string, 0, len(typed)) | ||
| for key := range typed { | ||
| keys = append(keys, key) | ||
| } | ||
| sort.Strings(keys) | ||
| pairs := make([]KeyValuePair, 0, len(keys)) | ||
| for _, key := range keys { | ||
| pairs = append(pairs, KeyValuePair{Key: key, Value: normalizedPairValue(typed[key]), Style: "compact"}) | ||
| } | ||
| return pairs, true | ||
| case []any: | ||
| pairs := make([]KeyValuePair, 0, len(typed)) | ||
| for _, item := range typed { | ||
| object, ok := item.(map[string]any) | ||
| if !ok { | ||
| return nil, false | ||
| } | ||
| if pair, ok := keyValuePairFromObject(object); ok { | ||
| pairs = append(pairs, pair) | ||
| continue | ||
| } | ||
| objectPairs, ok := keyValuePairsFromDecoded(object) | ||
| if !ok { | ||
| return nil, false | ||
| } | ||
| pairs = append(pairs, objectPairs...) | ||
| } | ||
| return pairs, true | ||
| default: | ||
| return nil, false | ||
| } | ||
| } | ||
|
|
||
| func keyValuePairFromObject(object map[string]any) (KeyValuePair, bool) { | ||
| key, hasKey := lookupObjectString(object, "key", "name") | ||
| value, hasValue := lookupObjectValue(object, "value") | ||
| if !hasKey || !hasValue { | ||
| return KeyValuePair{}, false | ||
| } | ||
| return KeyValuePair{Key: key, Value: normalizedPairValue(value), Style: "compact"}, true | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Restrict the pair shortcut to objects that only hold key/value fields.
keyValuePairFromObject accepts any object that contains a key/name field and a value field, then discards every other field. An input such as {"name":"latency","value":42,"unit":"ms"} renders as latency=42, so unit disappears from CSV, Markdown, and Excel exports. The map[string]any branch at Line 87 uses the same helper, so a plain map with name and value keys also collapses to a single pair.
Apply the shortcut only when the object carries no additional data fields.
🐛 Proposed fix
func keyValuePairFromObject(object map[string]any) (KeyValuePair, bool) {
key, hasKey := lookupObjectString(object, "key", "name")
value, hasValue := lookupObjectValue(object, "value")
if !hasKey || !hasValue {
return KeyValuePair{}, false
}
+ for name := range object {
+ switch strings.ToLower(name) {
+ case "key", "name", "value", "style":
+ default:
+ return KeyValuePair{}, false
+ }
+ }
return KeyValuePair{Key: key, Value: normalizedPairValue(value), Style: "compact"}, true
}📝 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 keyValuePairsFromDecoded(value any) ([]KeyValuePair, bool) { | |
| switch typed := value.(type) { | |
| case map[string]any: | |
| if pair, ok := keyValuePairFromObject(typed); ok { | |
| return []KeyValuePair{pair}, true | |
| } | |
| keys := make([]string, 0, len(typed)) | |
| for key := range typed { | |
| keys = append(keys, key) | |
| } | |
| sort.Strings(keys) | |
| pairs := make([]KeyValuePair, 0, len(keys)) | |
| for _, key := range keys { | |
| pairs = append(pairs, KeyValuePair{Key: key, Value: normalizedPairValue(typed[key]), Style: "compact"}) | |
| } | |
| return pairs, true | |
| case []any: | |
| pairs := make([]KeyValuePair, 0, len(typed)) | |
| for _, item := range typed { | |
| object, ok := item.(map[string]any) | |
| if !ok { | |
| return nil, false | |
| } | |
| if pair, ok := keyValuePairFromObject(object); ok { | |
| pairs = append(pairs, pair) | |
| continue | |
| } | |
| objectPairs, ok := keyValuePairsFromDecoded(object) | |
| if !ok { | |
| return nil, false | |
| } | |
| pairs = append(pairs, objectPairs...) | |
| } | |
| return pairs, true | |
| default: | |
| return nil, false | |
| } | |
| } | |
| func keyValuePairFromObject(object map[string]any) (KeyValuePair, bool) { | |
| key, hasKey := lookupObjectString(object, "key", "name") | |
| value, hasValue := lookupObjectValue(object, "value") | |
| if !hasKey || !hasValue { | |
| return KeyValuePair{}, false | |
| } | |
| return KeyValuePair{Key: key, Value: normalizedPairValue(value), Style: "compact"}, true | |
| } | |
| func keyValuePairsFromDecoded(value any) ([]KeyValuePair, bool) { | |
| switch typed := value.(type) { | |
| case map[string]any: | |
| if pair, ok := keyValuePairFromObject(typed); ok { | |
| return []KeyValuePair{pair}, true | |
| } | |
| keys := make([]string, 0, len(typed)) | |
| for key := range typed { | |
| keys = append(keys, key) | |
| } | |
| sort.Strings(keys) | |
| pairs := make([]KeyValuePair, 0, len(keys)) | |
| for _, key := range keys { | |
| pairs = append(pairs, KeyValuePair{Key: key, Value: normalizedPairValue(typed[key]), Style: "compact"}) | |
| } | |
| return pairs, true | |
| case []any: | |
| pairs := make([]KeyValuePair, 0, len(typed)) | |
| for _, item := range typed { | |
| object, ok := item.(map[string]any) | |
| if !ok { | |
| return nil, false | |
| } | |
| if pair, ok := keyValuePairFromObject(object); ok { | |
| pairs = append(pairs, pair) | |
| continue | |
| } | |
| objectPairs, ok := keyValuePairsFromDecoded(object) | |
| if !ok { | |
| return nil, false | |
| } | |
| pairs = append(pairs, objectPairs...) | |
| } | |
| return pairs, true | |
| default: | |
| return nil, false | |
| } | |
| } | |
| func keyValuePairFromObject(object map[string]any) (KeyValuePair, bool) { | |
| key, hasKey := lookupObjectString(object, "key", "name") | |
| value, hasValue := lookupObjectValue(object, "value") | |
| if !hasKey || !hasValue { | |
| return KeyValuePair{}, false | |
| } | |
| for name := range object { | |
| switch strings.ToLower(name) { | |
| case "key", "name", "value", "style": | |
| default: | |
| return KeyValuePair{}, false | |
| } | |
| } | |
| return KeyValuePair{Key: key, Value: normalizedPairValue(value), Style: "compact"}, true | |
| } |
🤖 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/column_value.go` around lines 84 - 130, Restrict keyValuePairFromObject
to return a shortcut pair only when the object contains the recognized key/name
and value fields with no additional fields. For objects with extra fields,
return false so keyValuePairsFromDecoded falls back to normal map expansion and
preserves all data in exports. Apply this validation consistently for both the
map[string]any and []any paths.
| func normalizeJSONColumnValue(value any) (any, bool) { | ||
| if value == nil { | ||
| return nil, true | ||
| } | ||
| return decodeStructuredColumnValue(value, false) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return the empty representation for nil JSON values.
normalizeJSONColumnValue(nil) reports success with a nil payload. The callers then marshal it, so ColumnString returns the literal null and ColumnTextable returns a JSON code block containing null. Missing cells are common: formatters/stream.go passes row[column.Name], which is nil when the key is absent. Non-structured columns render nil as an empty cell, so exports become inconsistent.
Report failure for nil so the shared fallback path produces an empty cell.
🐛 Proposed fix
func normalizeJSONColumnValue(value any) (any, bool) {
if value == nil {
- return nil, true
+ return nil, false
}
return decodeStructuredColumnValue(value, false)
}📝 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 normalizeJSONColumnValue(value any) (any, bool) { | |
| if value == nil { | |
| return nil, true | |
| } | |
| return decodeStructuredColumnValue(value, false) | |
| } | |
| func normalizeJSONColumnValue(value any) (any, bool) { | |
| if value == nil { | |
| return nil, false | |
| } | |
| return decodeStructuredColumnValue(value, false) | |
| } |
🤖 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/column_value.go` around lines 186 - 191, Update normalizeJSONColumnValue
so a nil input returns failure rather than success with a nil payload, allowing
callers to use the shared empty-cell fallback. Preserve the existing
decodeStructuredColumnValue behavior for non-nil values.
| func decodeStructuredColumnValue(value any, requireContainer bool) (any, bool) { | ||
| switch typed := value.(type) { | ||
| case string: | ||
| trimmed := strings.TrimSpace(typed) | ||
| if (strings.HasPrefix(trimmed, "{") || strings.HasPrefix(trimmed, "[")) && json.Valid([]byte(trimmed)) { | ||
| var decoded any | ||
| if json.Unmarshal([]byte(trimmed), &decoded) == nil { | ||
| return decoded, true | ||
| } | ||
| } | ||
| if requireContainer { | ||
| return nil, false | ||
| } | ||
| return typed, true | ||
| case []byte: | ||
| if json.Valid(typed) { | ||
| var decoded any | ||
| if json.Unmarshal(typed, &decoded) == nil { | ||
| if !requireContainer || isJSONContainer(decoded) { | ||
| return decoded, true | ||
| } | ||
| } | ||
| } | ||
| if requireContainer { | ||
| return nil, false | ||
| } | ||
| return string(typed), true | ||
| default: | ||
| data, err := json.Marshal(value) | ||
| if err != nil { | ||
| return nil, false | ||
| } | ||
| var decoded any | ||
| if err := json.Unmarshal(data, &decoded); err != nil { | ||
| return nil, false | ||
| } | ||
| if requireContainer && !isJSONContainer(decoded) { | ||
| return nil, false | ||
| } | ||
| return decoded, true | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Decode JSON numbers with UseNumber to keep large integers exact.
Every branch decodes into any with the default decoder, so all numbers become float64. A 64-bit identifier above 2^53 changes value after the round trip, and ColumnString then writes the corrupted number into CSV, Markdown, and Excel exports. normalizedPairValue already accepts json.Number at Line 163, so the rest of the path supports exact numbers.
Use a json.Decoder with UseNumber() in place of the three json.Unmarshal calls.
🐛 Proposed helper and usage
func decodeJSONPreservingNumbers(data []byte) (any, error) {
decoder := json.NewDecoder(bytes.NewReader(data))
decoder.UseNumber()
var decoded any
if err := decoder.Decode(&decoded); err != nil {
return nil, err
}
return decoded, nil
}- var decoded any
- if json.Unmarshal([]byte(trimmed), &decoded) == nil {
- return decoded, true
- }
+ if decoded, err := decodeJSONPreservingNumbers([]byte(trimmed)); err == nil {
+ return decoded, true
+ }🤖 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/column_value.go` around lines 193 - 234, Update
decodeStructuredColumnValue to decode JSON through a shared helper using
json.Decoder.UseNumber() instead of the three json.Unmarshal calls, preserving
json.Number values for large integers in every string, []byte, and
marshaled-value branch. Keep the existing validation, container checks, and
fallback behavior unchanged.
| func (cappedWidthTableRow) Row() map[string]any { | ||
| return map[string]any{ | ||
| "agent": "codex-agent", | ||
| "session": "019f5c3c", | ||
| "title": "Title uses all terminal space left after capped columns", | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the width tests exercise over-limit content.
The wide fixture uses codex-agent with MaxWidth(12) and 019f5c3c with MaxWidth(8), so the existing assertions do not detect a capped column that ignores its maximum. The narrow test only checks for $1.25; it can pass even when rendered lines exceed terminal width 30. Add an over-limit value to the wide fixture and assert the narrow output width.
Test improvement
- "agent": "codex-agent",
+ "agent": "codex-agent-with-a-long-name",
...
rendered := NewTableFrom([]shrinkingCappedWidthTableRow{{}}).String()
Expect(rendered).To(ContainSubstring("$1.25"))
+ for _, line := range strings.Split(rendered, "\n") {
+ Expect(len([]rune(line))).To(BeNumerically("<=", 30))
+ }Also applies to: 145-172
🤖 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/table_test.go` around lines 36 - 42, Update the width-test fixtures and
assertions around cappedWidthTableRow and the narrow table test so they use
values longer than the configured MaxWidth limits, including an over-limit
wide-fixture value. Add an explicit assertion that every narrow rendered line
stays within terminal width 30, while preserving the existing content
assertions.
| ToolHints: e.ToolHints, | ||
| } | ||
| if info.ToolHints.Group == "" { | ||
| info.ToolHints.Group = info.ToolGroup | ||
| } | ||
| if info.ToolGroup == "" { | ||
| info.ToolGroup = info.ToolHints.Group |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate tool hints to admin entities.
RegisterEntity copies e.ToolHints only into the primary EntityInfo. The adminInfo created later in this function does not receive admin.ToolHints or admin.ToolGroup.
When e.Admin defines tool metadata, generated admin commands omit it. RPC and MCP then lose the admin group, permission, strictness, and safety hints.
Copy and synchronize the admin tool fields when constructing adminInfo.
Proposed fix
adminInfo := EntityInfo{
Name: admin.Name,
Type: info.Type,
ListType: info.ListType,
ValidArgs: adminValidArgs,
IsAdmin: true,
+ ToolGroup: admin.ToolGroup,
+ ToolHints: admin.ToolHints,
}
+ if adminInfo.ToolHints.Group == "" {
+ adminInfo.ToolHints.Group = adminInfo.ToolGroup
+ }
+ if adminInfo.ToolGroup == "" {
+ adminInfo.ToolGroup = adminInfo.ToolHints.Group
+ }🤖 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 `@entity/entity.go` around lines 638 - 644, Update RegisterEntity when
constructing adminInfo to copy admin.ToolHints and admin.ToolGroup, then
synchronize the group fields using the same fallback behavior as the primary
EntityInfo. Ensure generated admin commands retain the admin group, permissions,
strictness, and safety metadata.
| count, err := streamRows(ctx, rows, first, ok, maxRows, func(row map[string]any) error { | ||
| if _, err := io.WriteString(w, "<tr>"); err != nil { | ||
| return err | ||
| } | ||
| for _, column := range columns { | ||
| if _, err := fmt.Fprintf(w, "<td>%s</td>", streamCell(row[column.Name], column, "html")); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| _, err := io.WriteString(w, "</tr>") | ||
| return err | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect HTML() implementations in the api package for escaping.
set -euo pipefail
ast-grep run --pattern $'func ($_ $_) HTML() string {
$$$
}' --lang go api
rg -nP --type=go -C 3 'html\.EscapeString|template\.HTMLEscape' -g 'api/**'Repository: flanksource/clicky
Length of output: 15860
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- formatters/stream.go ---'
sed -n '1,180p' formatters/stream.go
sed -n '240,310p' formatters/stream.go
printf '%s\n' '--- api text and description-list implementations ---'
sed -n '1,80p' api/html.go
sed -n '560,625p' api/text.go
sed -n '1,165p' api/code.go
printf '%s\n' '--- relevant constructors and stream tests ---'
rg -n -C 4 'streamCell|WriteTableStream|DescriptionList|Code\{' formatters api --glob '*.go'Repository: flanksource/clicky
Length of output: 48255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- streamCell and column conversion ---'
sed -n '350,390p' formatters/stream.go
sed -n '1,95p' api/column_value.go
rg -n -C 5 'func formatHTML|func \(.*\) HTML\(\) string|func convertToTextable' api --glob '*.go'
printf '%s\n' '--- HTML escaping tests and Text construction ---'
rg -n -C 5 'TextHTML|HTML.*escape|EscapeString|<script>|Content:.*<' api formatters --glob '*_test.go'
rg -n -C 3 'func Text|func \(.*Text.*Append|Content.*any|Textable' api/text.go api/aliases.go api/format.goRepository: flanksource/clicky
Length of output: 33150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
stream = Path("formatters/stream.go").read_text()
html = Path("api/html.go").read_text()
column = Path("api/column.go").read_text()
checks = {
"HTML stream writes streamCell directly inside td":
'fmt.Fprintf(w, "<td>%s</td>", streamCell(row[column.Name], column, "html"))' in stream,
"streamCell delegates HTML to Textable":
'return text.HTML()' in stream,
"Text.HTML uses raw Content":
'content := t.Content' in html and 'htmlEscapeString(content)' not in html,
"scalar conversion returns Text":
'return Text{Content: fmt.Sprintf("%v", v)}' in column or
'return Text{Content: v}' in column,
}
for name, result in checks.items():
print(f"{name}: {'YES' if result else 'NO'}")
if not all(checks.values()):
raise SystemExit("unexpected source shape")
print("reachable scalar payload: <script>alert(1)</script>")
print("Text.HTML payload remains executable because Text.HTML does not escape Content")
PYRepository: flanksource/clicky
Length of output: 344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '205,270p' api/column.goRepository: flanksource/clicky
Length of output: 474
Escape untrusted HTML cell content
streamCell(..., "html") inserts Text.HTML() output directly into <td>. Text.HTML() leaves Content unescaped, so scalar row values can inject executable markup. Escape scalar content while preserving structured renderers, and add an XSS regression test.
🤖 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/stream.go` around lines 277 - 288, Update the HTML row-rendering
callback in streamRows to escape scalar cell content before writing it into
<td>, while preserving the output of structured renderers. Reuse the existing
HTML-escaping utility or renderer behavior, and add a regression test covering
executable markup supplied through a scalar row value.
| switch { | ||
| case method == "GET" || method == "HEAD" || verb == "list" || verb == "get": | ||
| return boolPtr(true), boolPtr(false), boolPtr(true) | ||
| case method == "DELETE" || verb == "delete": | ||
| return boolPtr(false), boolPtr(true), boolPtr(true) | ||
| case method == "PUT" || verb == "update": | ||
| return boolPtr(false), boolPtr(true), boolPtr(true) | ||
| case method == "PATCH": | ||
| return boolPtr(false), boolPtr(true), boolPtr(false) | ||
| case method == "POST" || verb == "create": | ||
| return boolPtr(false), boolPtr(false), boolPtr(false) | ||
| case verb == "action": | ||
| return boolPtr(false), boolPtr(true), nil |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Prioritize generic action semantics over POST semantics.
When an operation has Clicky.Verb == "action" and Method == "POST", line 134 runs first. The generated metadata sets DestructiveHint to false.
A generic action can mutate or delete state. Publish conservative destructive metadata for it. Check verb == "action" before the HTTP-method cases. Add a POST action test.
Proposed fix
switch {
+ case verb == "action":
+ return boolPtr(false), boolPtr(true), nil
case method == "GET" || method == "HEAD" || verb == "list" || verb == "get":
return boolPtr(true), boolPtr(false), boolPtr(true)
@@
- case verb == "action":
- return boolPtr(false), boolPtr(true), nil
default:📝 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.
| switch { | |
| case method == "GET" || method == "HEAD" || verb == "list" || verb == "get": | |
| return boolPtr(true), boolPtr(false), boolPtr(true) | |
| case method == "DELETE" || verb == "delete": | |
| return boolPtr(false), boolPtr(true), boolPtr(true) | |
| case method == "PUT" || verb == "update": | |
| return boolPtr(false), boolPtr(true), boolPtr(true) | |
| case method == "PATCH": | |
| return boolPtr(false), boolPtr(true), boolPtr(false) | |
| case method == "POST" || verb == "create": | |
| return boolPtr(false), boolPtr(false), boolPtr(false) | |
| case verb == "action": | |
| return boolPtr(false), boolPtr(true), nil | |
| switch { | |
| case verb == "action": | |
| return boolPtr(false), boolPtr(true), nil | |
| case method == "GET" || method == "HEAD" || verb == "list" || verb == "get": | |
| return boolPtr(true), boolPtr(false), boolPtr(true) | |
| case method == "DELETE" || verb == "delete": | |
| return boolPtr(false), boolPtr(true), boolPtr(true) | |
| case method == "PUT" || verb == "update": | |
| return boolPtr(false), boolPtr(true), boolPtr(true) | |
| case method == "PATCH": | |
| return boolPtr(false), boolPtr(true), boolPtr(false) | |
| case method == "POST" || verb == "create": | |
| return boolPtr(false), boolPtr(false), boolPtr(false) |
🤖 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 125 - 137, The metadata switch must prioritize
generic action semantics when verb is "action", including POST actions. Move the
verb == "action" case before the POST and other HTTP-method cases so it returns
conservative destructive metadata, and add a test covering a POST action.
| // Flush forwards to the inner Flusher so SSE handlers that assert | ||
| // http.Flusher keep streaming. | ||
| func (rec *timingRecorder) Flush() { | ||
| rec.stamp() | ||
| if f, ok := rec.ResponseWriter.(http.Flusher); ok { | ||
| f.Flush() | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect ResponseWriter optional-interface handling and related tests.
rg -n -C 4 'TimingMiddleware|timingRecorder|http\.Flusher|ResponseController' rpcRepository: flanksource/clicky
Length of output: 6661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- middleware.go ---'
cat -n rpc/http/middleware.go
printf '%s\n' '--- middleware_test.go ---'
cat -n rpc/http/middleware_test.go
printf '%s\n' '--- module Go version ---'
rg -n '^go ' go.mod '**/go.mod' 2>/dev/null || true
printf '%s\n' '--- Flusher implementations in the standard test writers ---'
go env GOVERSION 2>/dev/null || trueRepository: flanksource/clicky
Length of output: 5567
🏁 Script executed:
#!/bin/bash
set -euo pipefail
go run /dev/stdin <<'EOF'
package main
import (
"fmt"
"net/http"
)
type nonFlushingWriter struct{}
func (nonFlushingWriter) Header() http.Header { return make(http.Header) }
func (nonFlushingWriter) Write([]byte) (int, error) { return 0, nil }
func (nonFlushingWriter) WriteHeader(int) {}
type timingRecorder struct {
http.ResponseWriter
}
func (rec *timingRecorder) Flush() {
if f, ok := rec.ResponseWriter.(http.Flusher); ok {
f.Flush()
}
}
func main() {
original := nonFlushingWriter{}
wrapped := &timingRecorder{ResponseWriter: original}
_, originalFlush := any(original).(http.Flusher)
_, wrappedFlush := any(wrapped).(http.Flusher)
fmt.Printf("original implements http.Flusher: %t\n", originalFlush)
fmt.Printf("timingRecorder implements http.Flusher: %t\n", wrappedFlush)
}
EOFRepository: flanksource/clicky
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
probe="$(mktemp /tmp/flusher-probe-XXXXXX.go)"
trap 'rm -f "$probe"' EXIT
cat >"$probe" <<'EOF'
package main
import (
"fmt"
"net/http"
)
type nonFlushingWriter struct{}
func (nonFlushingWriter) Header() http.Header { return make(http.Header) }
func (nonFlushingWriter) Write([]byte) (int, error) { return 0, nil }
func (nonFlushingWriter) WriteHeader(int) {}
type timingRecorder struct {
http.ResponseWriter
}
func (rec *timingRecorder) Flush() {
if f, ok := rec.ResponseWriter.(http.Flusher); ok {
f.Flush()
}
}
func main() {
original := nonFlushingWriter{}
wrapped := &timingRecorder{ResponseWriter: original}
_, originalFlush := any(original).(http.Flusher)
_, wrappedFlush := any(wrapped).(http.Flusher)
fmt.Printf("original implements http.Flusher: %t\n", originalFlush)
fmt.Printf("timingRecorder implements http.Flusher: %t\n", wrappedFlush)
}
EOF
go run "$probe"Repository: flanksource/clicky
Length of output: 268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
probe="$(mktemp /tmp/flusher-probe-XXXXXX.go)"
trap 'rm -f "$probe"' EXIT
cat >"$probe" <<'EOF'
package main
import (
"fmt"
"net/http"
)
type nonFlushingWriter struct{}
func (nonFlushingWriter) Header() http.Header { return make(http.Header) }
func (nonFlushingWriter) Write([]byte) (int, error) { return 0, nil }
func (nonFlushingWriter) WriteHeader(int) {}
type timingRecorder struct {
http.ResponseWriter
}
func (rec *timingRecorder) Flush() {
if f, ok := rec.ResponseWriter.(http.Flusher); ok {
f.Flush()
}
}
func main() {
original := nonFlushingWriter{}
wrapped := &timingRecorder{ResponseWriter: original}
_, originalFlush := any(original).(http.Flusher)
_, wrappedFlush := any(wrapped).(http.Flusher)
fmt.Printf("original implements http.Flusher: %t\n", originalFlush)
fmt.Printf("timingRecorder implements http.Flusher: %t\n", wrappedFlush)
}
EOF
CGO_ENABLED=0 go run "$probe"Repository: flanksource/clicky
Length of output: 241
Preserve the optional http.Flusher interface.
timingRecorder always satisfies http.Flusher, even when the wrapped writer does not. A handler can therefore assert w.(http.Flusher) successfully, although Flush performs no operation. Use a separate flushing wrapper only when the original writer implements http.Flusher, and add coverage for a non-flushing writer.
🤖 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/http/middleware.go` around lines 56 - 63, Update the timingRecorder
wrapping flow around timingRecorder and its Flush method so the recorder does
not always implement http.Flusher; return/use a separate flushing wrapper only
when the original ResponseWriter supports http.Flusher, while preserving
timestamping and forwarding behavior. Add coverage verifying a non-flushing
writer does not expose http.Flusher.
| g.mu.RLock() | ||
| startTime := g.startTime | ||
| g.mu.RUnlock() | ||
| if startTime.IsZero() { | ||
| return 0 | ||
| } | ||
|
|
||
| // Find the latest end time among all items | ||
| var latestEnd time.Time | ||
| allCompleted := true | ||
|
|
||
| for _, item := range g.Items { | ||
| for _, item := range g.GetTasks() { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Capture startTime and Items in one group snapshot.
Add can update both fields between the lock release at Line 331 and GetTasks() at Line 340. Duration can then combine an old start time with a newly added task. This can underreport the duration. It can also return a negative duration if the new task started and completed before the stale start time.
Copy g.Items while reading g.startTime under the same read lock. Add a regression test that interleaves two Add calls with Duration.
Proposed fix
- g.mu.RLock()
- startTime := g.startTime
- g.mu.RUnlock()
+ g.mu.RLock()
+ startTime := g.startTime
+ items := make([]Taskable, len(g.Items))
+ copy(items, g.Items)
+ g.mu.RUnlock()
if startTime.IsZero() {
return 0
}
- for _, item := range g.GetTasks() {
+ for _, item := range items {📝 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.
| g.mu.RLock() | |
| startTime := g.startTime | |
| g.mu.RUnlock() | |
| if startTime.IsZero() { | |
| return 0 | |
| } | |
| // Find the latest end time among all items | |
| var latestEnd time.Time | |
| allCompleted := true | |
| for _, item := range g.Items { | |
| for _, item := range g.GetTasks() { | |
| g.mu.RLock() | |
| startTime := g.startTime | |
| items := make([]Taskable, len(g.Items)) | |
| copy(items, g.Items) | |
| g.mu.RUnlock() | |
| if startTime.IsZero() { | |
| return 0 | |
| } | |
| // Find the latest end time among all items | |
| var latestEnd time.Time | |
| allCompleted := true | |
| for _, item := range items { |
🤖 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/group.go` around lines 329 - 340, Update Duration to snapshot
g.startTime and g.Items under one g.mu.RLock/RUnlock section, then iterate over
that captured item slice instead of calling GetTasks separately. Add a
regression test that interleaves two Add calls with Duration and verifies the
duration never combines state from different group snapshots.
| 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 | 🟡 Minor | ⚡ Quick win
Evaluate the teardown decision before you call renderFinal and cleanupTerminal.
The comment at Lines 190-192 states the idle path must not disturb a concurrently restarted loop. The guard at Lines 193-198 applies only to uninstallLogSerializer and releaseRenderTerminal. renderFinal (Line 187) and cleanupTerminal (Line 189) run first and are not covered.
On the idle path, an enqueue can restart the loop before those two calls. cleanupTerminal then passes its ownsRenderTerminal() check, because the restarted loop already set renderOwnsTTY = true, and it calls ShowCursor and Reset against the live frame. renderFinal can likewise emit a closing summary while the new loop renders.
Read the state once at the start and gate all four steps with it.
🐛 Proposed fix
func (tm *Manager) finishRenderTeardown(loopWasRunning bool) {
+ // 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
+ tm.mu.Unlock()
+ if !teardown {
+ return
+ }
+
// 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()
- }
+ tm.mu.Lock()
+ tm.uninstallLogSerializer()
+ tm.mu.Unlock()
+ tm.releaseRenderTerminal()
}🤖 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 186 - 201, Evaluate the teardown
decision once at the start of this shutdown sequence, before renderFinal and
cleanupTerminal, using the existing loopWasRunning/renderState state under tm.mu
as needed. Gate renderFinal, cleanupTerminal, uninstallLogSerializer, and
releaseRenderTerminal on that same decision so an idle-path enqueue that
restarts the loop cannot affect the live renderer or terminal ownership.
1 participant
What
commons-dbdependency to v0.1.14.Summary by CodeRabbit
Server-Timingresponse headers and NDJSON content negotiation.oneOfschemas.