feat: add lint validation, task registry, and formatting helpers#108
Merged
Conversation
…ction Introduce comprehensive linting for clicky render builders and helper-backed types: - Add checkRenderBuilderRenderCalls to detect render extraction method calls (.ANSI(), .HTML(), etc.) inside render builder functions, which should return api.Text/Textable instead - Add isClickyRenderType and related helpers to identify clicky render types including Textable interface implementations - Expand checkCompositeLiteral to detect direct struct literals for all helper-backed types (List, TextList, TextTable, TextTree, Code, Button, ButtonGroup, KeyValuePair, DescriptionList, LabelBadge, Admonition, Collapsed, Diff, StackTrace, HtmlElement) with appropriate helper function suggestions - Add clickyAPITypeName and isClickyNamedType utilities for robust type checking - Add PrettyShort to recognized render builder method names - Create runner.go with Run() function for standalone CLI integration and Result/Violation types for normalized output - Create summary.go with SummaryView for tree-based lint result visualization - Add comprehensive test coverage for summary view grouping and truncation - Update test data with new validation scenarios and helper function stubs
…down APIs Introduce a comprehensive task registry system that enables querying and filtering task groups (runs) by kind, status, and labels. Add stable group IDs distinct from human-facing names to support registry drill-down and SSE filtering. Key additions: - New RunMeta type for listing summaries with metadata, status, and task counts - RunFilter for narrowing runs by kind, status, and label matching - Runs() function to list all runs with optional filtering and garbage collection - SnapshotByID() for drilling down into a specific run by stable ID - GCRuns() to clean up finished runs older than 10 minutes - RegisterHandlers() to wire generic task-manager API endpoints - RunsHandler() and RunHandler() HTTP handlers for JSON responses - GroupMetadata type with Kind, Labels, and Owner fields - WithGroupID(), WithKind(), WithLabels(), WithOwner() options for group configuration - Group methods: ID(), Metadata(), StartedAt(), FinishedAt(), observeTerminal() - TaskSnapshot enriched with GroupID, Kind, Labels, Owner, StartedAt, FinishedAt - SSE handler extended to resolve ?kind= filter to matching run IDs - Comprehensive test coverage for filtering, drill-down, GC, and backward compatibility Backward compatible: name-keyed filtering still works alongside new ID-keyed filtering.
…d expand formatting helpers
Introduce WithOptionalID for entity actions that can be invoked without an ID parameter, generating flat REST paths (/entity/action) instead of ID-scoped paths (/entity/{id}/action) to prevent collision with get-by-id routes.
Add comprehensive formatting helper functions (List, TextList, Table, Tree, Button, LabelBadge, Admonition, Diff, Comment, HTMLElement, ClickyText) to simplify API usage.
Expand test coverage with E2E tests for pretty help, lint command functionality, and optional ID action path generation. Update format flag help text to document all supported output formats.
BREAKING CHANGE: annotateEntityOperationCommand now requires an additional optionalID boolean parameter.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds custom CLI help (special-case pretty), refactors lint to a runner with JSON/pretty output and raw passthrough, extends lint analysis and summary rendering, exports formatting/UI helpers, threads OptionalID into entity annotations/OpenAPI, implements task run registry and GC, adds metrics API and backends, and updates CI and sqlite driver usage. ChangesCLI, Lint, Format, Entity Operations, Task Management, Metrics, and CI
Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
task/manager.go (1)
473-499:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPublish the group only after it is fully initialized, under
global.mu.
StartGroupappends toglobal.groupsbefore options, autogeneratedid, and the semaphore are set, and the append itself is unlocked. That letsRuns/SnapshotAll/GCRunsrace with group creation and observe a transient run with an empty ID or missing metadata. Build the group first, then append it while holdingglobal.mu.Suggested fix
func StartGroup[T any](name string, opts ...TaskGroupOption) TypedGroup[T] { ctx, cancel := context.WithCancel(context.Background()) group := &Group{ name: name, Items: make([]Taskable, 0), startTime: time.Now(), manager: global, ctx: ctx, cancel: cancel, } - global.groups = append(global.groups, group) - for _, opt := range opts { opt(group) } // Assign a stable id unless the caller supplied one via WithGroupID. The id // is what the registry drill-down (SnapshotByID) and SSE id filter key on, // independent of the human-facing name. if group.id == "" { group.id = uuid.NewString() } if group.concurrency > 0 { group.sem = semaphore.NewWeighted(int64(group.concurrency)) } + + global.mu.Lock() + global.groups = append(global.groups, group) + global.mu.Unlock() return TypedGroup[T]{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 `@task/manager.go` around lines 473 - 499, StartGroup currently appends the new Group to global.groups before options are applied and before id/semaphore are initialized, causing races; instead, fully initialize the Group (apply opts, set group.id if empty, set group.sem) and then append it to global.groups while holding global.mu to prevent concurrent readers (Runs/SnapshotAll/GCRuns) from seeing a partially-built group; ensure the append is wrapped by global.mu.Lock()/Unlock() and keep WithGroupID, group.id, group.sem, and StartGroup as the reference points to locate the changes.task/group.go (1)
228-305:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
WaitForcan still return before late-added tasks finish.This loop declares the group complete after a short quiet period, but
Addstays callable the whole time. Any task appended just after the third stable iteration is invisible toGetResults(), so callers can get a terminalWaitResultwhile the group is still growing/running. If dynamic additions are part of the contract here, completion needs explicit task accounting or a close/finalize signal instead of polling for silence.🤖 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 228 - 305, The WaitFor loop in TypedGroup[T].WaitFor relies on a silence window and can miss tasks added after that window; change the design to require an explicit finalization signal instead of polling: add a Close/Finalize method on TypedGroup that sets a closed flag (and prevents further Add calls) or maintain an additions counter that WaitFor waits to reach zero, then modify Add to increment that counter and Close/Finalize to mark no-more-adds (or decrement when adds complete), and update WaitFor to block until the group is closed/finished and all Items (g.Items) are complete before calling GetResults(); also ensure Add checks the closed flag to disallow late adds and update GetResults/Status logic as needed.lint/analyzer.go (1)
218-227:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the remediation text to include
PrettyShort.Line 218 now treats
PrettyShortas an allowed name, but the diagnostic still tells users to rename only toPretty/PrettyFull/PrettyRow. That leaves the linter suggesting an incomplete fix.Suggested fix
- "%s returns api.Text; return api.Textable interface or rename to Pretty/PrettyFull/PrettyRow", + "%s returns api.Text; return api.Textable interface or rename to Pretty/PrettyFull/PrettyShort/PrettyRow", name)🤖 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 `@lint/analyzer.go` around lines 218 - 227, The diagnostic message emitted by pass.Reportf for functions returning api.Text is missing "PrettyShort" even though the name variable allows it; update the remediation string in the pass.Reportf call so it suggests renaming to "Pretty/PrettyFull/PrettyShort/PrettyRow" (keep the rest of the message intact) for the check in the block that uses name, fn, pass.Reportf and isClickyTextTypesType.
🧹 Nitpick comments (2)
lint/testdata/src/bad/bad.go (1)
78-93: ⚡ Quick winAdd fixture coverage for the remaining extraction methods.
The new rule also lists
StaticHTML,AsHTML, andAsMarkdown, but this bad fixture never exercises those branches. A couple of targeted cases here would keep the allowlist from drifting without test failures.🤖 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 `@lint/testdata/src/bad/bad.go` around lines 78 - 93, Add missing negative test cases that exercise the StaticHTML, AsHTML, and AsMarkdown extraction branches by adding small functions similar to BuildTextable/BuildTable/BuildList that call api.Text{}.StaticHTML(), api.Text{}.AsHTML(), and api.Text{}.AsMarkdown() (or call those methods on constructed api.TextTable/api.TextList where appropriate) so the bad fixture triggers the new rule; reference and update the existing functions BuildTextable, BuildTable, and BuildList or add new BuildStaticHTML/BuildAsHTML/BuildAsMarkdown helpers that invoke those methods to ensure the allowlist branches are covered by tests.e2e_integration_test.go (1)
637-645: ⚡ Quick winUpdate: fixture
godirective matches the repo, so the mismatch concern doesn’t apply.The generated fixture
go.modusesgo 1.26.1, which matchesgo.modacross the repo (./go.mod,./examples/go.mod,./examples/uber_demo/go.modall specifygo 1.26.1), and there are notoolchaindirectives present in anygo.mod. Optional: derive the version from the checked-ingo.modto prevent future drift.🤖 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 `@e2e_integration_test.go` around lines 637 - 645, The test currently builds the fixture go.mod by hardcoding the go directive version in the mod string (the mod variable in e2e_integration_test.go) and writing it via os.WriteFile; update this to parse the repository's checked-in go.mod to extract the "go" directive and substitute that version into the generated mod string (instead of "1.26.1"), then continue to write the file using the existing filepath.ToSlash(repoRoot) replacement so the fixture stays in sync and avoids future drift.
🤖 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 `@cmd/clicky/main.go`:
- Around line 592-600: The lint subcommand is treating a sole "help" argument as
a package pattern due to DisableFlagParsing=true; update the RunE logic (or
lintHelpRequested) so that a single arg equal to "help" is treated like
-h/--help. Concretely, before calling parseLintArgs in the RunE handler (the
block that calls rawLintArgs, lintHelpRequested, parseLintArgs), add a check
that if len(args)==1 and args[0]=="help" then return cmd.Help(), or extend
lintHelpRequested to return true for that case; ensure this check runs prior to
parseLintArgs so "clicky lint help" shows help instead of trying to lint package
"help".
In `@lint/render_builder.go`:
- Around line 35-50: The inspector currently descends into nested function
literals and reports calls inside callbacks; modify the ast.Inspect callback
used on fn.Body to skip traversing into func literals by checking for nodes of
type *ast.FuncLit (and optionally *ast.FuncDecl) and returning false for those
nodes before inspecting children, while keeping the existing logic for
CallExpr/SelectorExpr/renderExtractionMethods/isClickyRenderType and
pass.Reportf intact (i.e., add an early branch in the ast.Inspect closure that
if n is *ast.FuncLit return false).
In `@lint/summary.go`:
- Around line 69-79: The linter node always appends the literal "error" instead
of showing the actual number and proper pluralization; update the branch that
handles n.errors to compute errCount := len(n.errors) and render that count
(pluralizing "error"/"errors") in the suffix—e.g. when count>0 include both
violations and the actual errCount, and when no violations include "(error)" for
errCount==1 or "(N errors)" for errCount>1; adjust the code around n.errors,
n.linter and firstErrorLine to use errCount for the formatted suffix.
In `@task/group.go`:
- Around line 61-65: WithLabels currently assigns the caller's labels map by
reference, exposing Group.metadata.Labels to external mutation and potential
concurrent-map panics; modify WithLabels (the function that returns
TaskGroupOption and assigns group.metadata.Labels) to clone the input map
(handle nil input) and store the cloned map on the Group so
runs/snapshot/Metadata() see an immutable copy independent of caller mutations.
In `@task/registry.go`:
- Around line 95-99: SnapshotByID currently delegates to SnapshotAll which
matches both group name and ID; change SnapshotByID to call SnapshotAll(id) then
filter the returned []TaskSnapshot so only entries whose TaskSnapshot.GroupID ==
id are kept and returned, ensuring the function (SnapshotByID) returns only
snapshots with the exact stable group ID; locate SnapshotByID and
TaskSnapshot.GroupID in task/registry.go and implement the post-call filter (or
replace delegation) to produce the filtered slice.
---
Outside diff comments:
In `@lint/analyzer.go`:
- Around line 218-227: The diagnostic message emitted by pass.Reportf for
functions returning api.Text is missing "PrettyShort" even though the name
variable allows it; update the remediation string in the pass.Reportf call so it
suggests renaming to "Pretty/PrettyFull/PrettyShort/PrettyRow" (keep the rest of
the message intact) for the check in the block that uses name, fn, pass.Reportf
and isClickyTextTypesType.
In `@task/group.go`:
- Around line 228-305: The WaitFor loop in TypedGroup[T].WaitFor relies on a
silence window and can miss tasks added after that window; change the design to
require an explicit finalization signal instead of polling: add a Close/Finalize
method on TypedGroup that sets a closed flag (and prevents further Add calls) or
maintain an additions counter that WaitFor waits to reach zero, then modify Add
to increment that counter and Close/Finalize to mark no-more-adds (or decrement
when adds complete), and update WaitFor to block until the group is
closed/finished and all Items (g.Items) are complete before calling
GetResults(); also ensure Add checks the closed flag to disallow late adds and
update GetResults/Status logic as needed.
In `@task/manager.go`:
- Around line 473-499: StartGroup currently appends the new Group to
global.groups before options are applied and before id/semaphore are
initialized, causing races; instead, fully initialize the Group (apply opts, set
group.id if empty, set group.sem) and then append it to global.groups while
holding global.mu to prevent concurrent readers (Runs/SnapshotAll/GCRuns) from
seeing a partially-built group; ensure the append is wrapped by
global.mu.Lock()/Unlock() and keep WithGroupID, group.id, group.sem, and
StartGroup as the reference points to locate the changes.
---
Nitpick comments:
In `@e2e_integration_test.go`:
- Around line 637-645: The test currently builds the fixture go.mod by
hardcoding the go directive version in the mod string (the mod variable in
e2e_integration_test.go) and writing it via os.WriteFile; update this to parse
the repository's checked-in go.mod to extract the "go" directive and substitute
that version into the generated mod string (instead of "1.26.1"), then continue
to write the file using the existing filepath.ToSlash(repoRoot) replacement so
the fixture stays in sync and avoids future drift.
In `@lint/testdata/src/bad/bad.go`:
- Around line 78-93: Add missing negative test cases that exercise the
StaticHTML, AsHTML, and AsMarkdown extraction branches by adding small functions
similar to BuildTextable/BuildTable/BuildList that call api.Text{}.StaticHTML(),
api.Text{}.AsHTML(), and api.Text{}.AsMarkdown() (or call those methods on
constructed api.TextTable/api.TextList where appropriate) so the bad fixture
triggers the new rule; reference and update the existing functions
BuildTextable, BuildTable, and BuildList or add new
BuildStaticHTML/BuildAsHTML/BuildAsMarkdown helpers that invoke those methods to
ensure the allowlist branches are covered by tests.
🪄 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: a9dab2ba-d196-4a62-9c9d-917a05b2faae
📒 Files selected for processing (29)
.gitignorecmd/clicky/main.gocobra_command.goe2e_integration_test.goentity.goentity_annotations.goformat.goformatters/options.goformatters/options_test.golint/analyzer.golint/render_builder.golint/runner.golint/summary.golint/summary_test.golint/testdata/src/bad/bad.golint/testdata/src/github.com/flanksource/clicky/api/stub.golint/testdata/src/github.com/flanksource/clicky/stub.golint/testdata/src/good/good.gorpc/converter.gorpc/openapi_test.gosub_command.gotask/api.gotask/group.gotask/manager.gotask/registry.gotask/registry_test.gotask/snapshot.gotask/sse.gotask/stop_test.go
Comment on lines
592
to
+600
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| if len(args) == 0 { | ||
| args = []string{"./..."} | ||
| if rawArgs, raw := rawLintArgs(args); raw { | ||
| return runRawLint(rawArgs) | ||
| } | ||
| if lintHelpRequested(args) { | ||
| return cmd.Help() | ||
| } | ||
|
|
||
| opts, packages, err := parseLintArgs(args) |
Contributor
There was a problem hiding this comment.
Treat clicky lint help as help, not a package pattern.
With DisableFlagParsing: true, a call like clicky lint help now falls through to parseLintArgs and tries to lint a package named help instead of showing command help. Please special-case a sole help arg alongside -h/--help.
Suggested fix
- if lintHelpRequested(args) {
+ if lintHelpRequested(args) || (len(args) == 1 && args[0] == "help") {
return cmd.Help()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| RunE: func(cmd *cobra.Command, args []string) error { | |
| if len(args) == 0 { | |
| args = []string{"./..."} | |
| if rawArgs, raw := rawLintArgs(args); raw { | |
| return runRawLint(rawArgs) | |
| } | |
| if lintHelpRequested(args) { | |
| return cmd.Help() | |
| } | |
| opts, packages, err := parseLintArgs(args) | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| if rawArgs, raw := rawLintArgs(args); raw { | |
| return runRawLint(rawArgs) | |
| } | |
| if lintHelpRequested(args) || (len(args) == 1 && args[0] == "help") { | |
| return cmd.Help() | |
| } | |
| opts, packages, err := parseLintArgs(args) |
🤖 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 `@cmd/clicky/main.go` around lines 592 - 600, The lint subcommand is treating a
sole "help" argument as a package pattern due to DisableFlagParsing=true; update
the RunE logic (or lintHelpRequested) so that a single arg equal to "help" is
treated like -h/--help. Concretely, before calling parseLintArgs in the RunE
handler (the block that calls rawLintArgs, lintHelpRequested, parseLintArgs),
add a check that if len(args)==1 and args[0]=="help" then return cmd.Help(), or
extend lintHelpRequested to return true for that case; ensure this check runs
prior to parseLintArgs so "clicky lint help" shows help instead of trying to
lint package "help".
Comment on lines
+35
to
+50
| ast.Inspect(fn.Body, func(n ast.Node) bool { | ||
| call, ok := n.(*ast.CallExpr) | ||
| if !ok { | ||
| return true | ||
| } | ||
| sel, ok := call.Fun.(*ast.SelectorExpr) | ||
| if !ok || !renderExtractionMethods[sel.Sel.Name] { | ||
| return true | ||
| } | ||
| if !isClickyRenderType(pass, pass.TypesInfo.TypeOf(sel.X)) { | ||
| return true | ||
| } | ||
| pass.Reportf(call.Pos(), | ||
| "avoid .%s() inside clicky render builders; return api.Text/api.Textable and let the formatter render it", | ||
| sel.Sel.Name) | ||
| return true |
Contributor
There was a problem hiding this comment.
Skip nested closures when walking builder bodies.
This traversal descends into func literals, so any .String()/.HTML() call inside a nested callback gets reported as if the outer render builder made it. That will create false positives in otherwise valid builders and make the new lint rule noisy.
Suggested fix
ast.Inspect(fn.Body, func(n ast.Node) bool {
- call, ok := n.(*ast.CallExpr)
- if !ok {
+ switch n := n.(type) {
+ case *ast.FuncLit:
+ return false
+ case *ast.CallExpr:
+ call := n
+ sel, ok := call.Fun.(*ast.SelectorExpr)
+ if !ok || !renderExtractionMethods[sel.Sel.Name] {
+ return true
+ }
+ if !isClickyRenderType(pass, pass.TypesInfo.TypeOf(sel.X)) {
+ return true
+ }
+ pass.Reportf(call.Pos(),
+ "avoid .%s() inside clicky render builders; return api.Text/api.Textable and let the formatter render it",
+ sel.Sel.Name)
return true
}
- sel, ok := call.Fun.(*ast.SelectorExpr)
- if !ok || !renderExtractionMethods[sel.Sel.Name] {
- return true
- }
- if !isClickyRenderType(pass, pass.TypesInfo.TypeOf(sel.X)) {
- return true
- }
- pass.Reportf(call.Pos(),
- "avoid .%s() inside clicky render builders; return api.Text/api.Textable and let the formatter render it",
- sel.Sel.Name)
return 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.
Suggested change
| ast.Inspect(fn.Body, func(n ast.Node) bool { | |
| call, ok := n.(*ast.CallExpr) | |
| if !ok { | |
| return true | |
| } | |
| sel, ok := call.Fun.(*ast.SelectorExpr) | |
| if !ok || !renderExtractionMethods[sel.Sel.Name] { | |
| return true | |
| } | |
| if !isClickyRenderType(pass, pass.TypesInfo.TypeOf(sel.X)) { | |
| return true | |
| } | |
| pass.Reportf(call.Pos(), | |
| "avoid .%s() inside clicky render builders; return api.Text/api.Textable and let the formatter render it", | |
| sel.Sel.Name) | |
| return true | |
| ast.Inspect(fn.Body, func(n ast.Node) bool { | |
| switch n := n.(type) { | |
| case *ast.FuncLit: | |
| return false | |
| case *ast.CallExpr: | |
| call := n | |
| sel, ok := call.Fun.(*ast.SelectorExpr) | |
| if !ok || !renderExtractionMethods[sel.Sel.Name] { | |
| return true | |
| } | |
| if !isClickyRenderType(pass, pass.TypesInfo.TypeOf(sel.X)) { | |
| return true | |
| } | |
| pass.Reportf(call.Pos(), | |
| "avoid .%s() inside clicky render builders; return api.Text/api.Textable and let the formatter render it", | |
| sel.Sel.Name) | |
| return true | |
| } | |
| return 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 `@lint/render_builder.go` around lines 35 - 50, The inspector currently
descends into nested function literals and reports calls inside callbacks;
modify the ast.Inspect callback used on fn.Body to skip traversing into func
literals by checking for nodes of type *ast.FuncLit (and optionally
*ast.FuncDecl) and returning false for those nodes before inspecting children,
while keeping the existing logic for
CallExpr/SelectorExpr/renderExtractionMethods/isClickyRenderType and
pass.Reportf intact (i.e., add an early branch in the ast.Inspect closure that
if n is *ast.FuncLit return false).
Comment on lines
+69
to
+79
| if len(n.errors) > 0 { | ||
| text := "❌ " + n.linter | ||
| if count > 0 { | ||
| text += fmt.Sprintf(" (%d violations, error)", count) | ||
| } else { | ||
| text += " (error)" | ||
| } | ||
| if summary := firstErrorLine(strings.Join(n.errors, "\n")); summary != "" { | ||
| text += " - " + summary | ||
| } | ||
| return api.Text{Content: text, Style: "text-red-600"} |
Contributor
There was a problem hiding this comment.
Render the actual error count in the linter node.
This branch always formats the suffix as error, even when len(n.errors) > 1. The root node already shows the real count, so the child summary becomes inconsistent as soon as multiple package/analyzer errors are present.
Suggested fix
if len(n.errors) > 0 {
text := "❌ " + n.linter
if count > 0 {
- text += fmt.Sprintf(" (%d violations, error)", count)
+ text += fmt.Sprintf(" (%d violations, %d errors)", count, len(n.errors))
} else {
- text += " (error)"
+ text += fmt.Sprintf(" (%d errors)", len(n.errors))
}
if summary := firstErrorLine(strings.Join(n.errors, "\n")); summary != "" {
text += " - " + summary
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if len(n.errors) > 0 { | |
| text := "❌ " + n.linter | |
| if count > 0 { | |
| text += fmt.Sprintf(" (%d violations, error)", count) | |
| } else { | |
| text += " (error)" | |
| } | |
| if summary := firstErrorLine(strings.Join(n.errors, "\n")); summary != "" { | |
| text += " - " + summary | |
| } | |
| return api.Text{Content: text, Style: "text-red-600"} | |
| if len(n.errors) > 0 { | |
| text := "❌ " + n.linter | |
| if count > 0 { | |
| text += fmt.Sprintf(" (%d violations, %d errors)", count, len(n.errors)) | |
| } else { | |
| text += fmt.Sprintf(" (%d errors)", len(n.errors)) | |
| } | |
| if summary := firstErrorLine(strings.Join(n.errors, "\n")); summary != "" { | |
| text += " - " + summary | |
| } | |
| return api.Text{Content: text, Style: "text-red-600"} | |
| } |
🤖 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 `@lint/summary.go` around lines 69 - 79, The linter node always appends the
literal "error" instead of showing the actual number and proper pluralization;
update the branch that handles n.errors to compute errCount := len(n.errors) and
render that count (pluralizing "error"/"errors") in the suffix—e.g. when count>0
include both violations and the actual errCount, and when no violations include
"(error)" for errCount==1 or "(N errors)" for errCount>1; adjust the code around
n.errors, n.linter and firstErrorLine to use errCount for the formatted suffix.
Comment on lines
+61
to
+65
| // WithLabels attaches filterable key/value facets to the run. | ||
| func WithLabels(labels map[string]string) TaskGroupOption { | ||
| return func(group *Group) { | ||
| group.metadata.Labels = labels | ||
| } |
Contributor
There was a problem hiding this comment.
Copy labels before storing it on the group.
WithLabels keeps the caller's map by reference. If that map is reused or mutated after StartGroup, Runs/SnapshotGroup will observe external mutations, and concurrent writes can panic while Metadata() iterates it. Clone the map here so run metadata is immutable once attached.
Suggested fix
func WithLabels(labels map[string]string) TaskGroupOption {
return func(group *Group) {
- group.metadata.Labels = labels
+ if labels == nil {
+ group.metadata.Labels = nil
+ return
+ }
+ copied := make(map[string]string, len(labels))
+ for k, v := range labels {
+ copied[k] = v
+ }
+ group.metadata.Labels = copied
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // WithLabels attaches filterable key/value facets to the run. | |
| func WithLabels(labels map[string]string) TaskGroupOption { | |
| return func(group *Group) { | |
| group.metadata.Labels = labels | |
| } | |
| // WithLabels attaches filterable key/value facets to the run. | |
| func WithLabels(labels map[string]string) TaskGroupOption { | |
| return func(group *Group) { | |
| if labels == nil { | |
| group.metadata.Labels = nil | |
| return | |
| } | |
| copied := make(map[string]string, len(labels)) | |
| for k, v := range labels { | |
| copied[k] = v | |
| } | |
| group.metadata.Labels = copied | |
| } | |
| } |
🤖 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 61 - 65, WithLabels currently assigns the
caller's labels map by reference, exposing Group.metadata.Labels to external
mutation and potential concurrent-map panics; modify WithLabels (the function
that returns TaskGroupOption and assigns group.metadata.Labels) to clone the
input map (handle nil input) and store the cloned map on the Group so
runs/snapshot/Metadata() see an immutable copy independent of caller mutations.
Comment on lines
+95
to
+99
| // SnapshotByID returns the group + task snapshots for the run with the given | ||
| // stable id (not name). Returns nil when no such run exists. | ||
| func SnapshotByID(id string) []TaskSnapshot { | ||
| return SnapshotAll(id) | ||
| } |
Contributor
There was a problem hiding this comment.
Filter by stable ID here instead of delegating to SnapshotAll.
SnapshotByID is documented as ID-only, but SnapshotAll(id) still matches both g.Name() and g.ID(). That means RunHandler can return a run by legacy name as well, which makes the new /tasks/{id} contract ambiguous and collision-prone. This wrapper should keep only snapshots whose GroupID equals the requested ID.
🤖 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/registry.go` around lines 95 - 99, SnapshotByID currently delegates to
SnapshotAll which matches both group name and ID; change SnapshotByID to call
SnapshotAll(id) then filter the returned []TaskSnapshot so only entries whose
TaskSnapshot.GroupID == id are kept and returned, ensuring the function
(SnapshotByID) returns only snapshots with the exact stable group ID; locate
SnapshotByID and TaskSnapshot.GroupID in task/registry.go and implement the
post-call filter (or replace delegation) to produce the filtered slice.
Add OnBeforeGC callback that fires before evicting expired task groups, allowing callers to capture full snapshots before removal. This enables L2-backed wrappers to snapshot state before GC. Introduce RunsRaw() function that lists runs without triggering GC, allowing callers to manage their own GC timing and avoid double-GC scenarios. Capitalize RunFilter.matches() and runMetaFromSnapshot() to RunFilter.Matches() and RunMetaFromSnapshot() for consistency with Go naming conventions. Extract snapshotGroupWithTasks() helper to reduce duplication and support the OnBeforeGC callback. BREAKING CHANGE: RunFilter.matches() renamed to RunFilter.Matches(); runMetaFromSnapshot() renamed to RunMetaFromSnapshot()
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
task/registry.go (1)
111-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
SnapshotByIDstrictly ID-only.
SnapshotAll(id)still matches bothg.Name()andg.ID(), so a legacy run name that happens to equal another run's stable ID will make this drill-down API return both runs. Filter the snapshots here toTaskSnapshot.GroupID == idinstead of delegating directly.Possible fix
func SnapshotByID(id string) []TaskSnapshot { - return SnapshotAll(id) + snaps := SnapshotAll(id) + out := snaps[:0] + for _, snap := range snaps { + if snap.GroupID == id { + out = append(out, snap) + } + } + if len(out) == 0 { + return nil + } + return out }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@task/registry.go` around lines 111 - 112, SnapshotByID currently delegates to SnapshotAll(id) which matches on name or ID; change SnapshotByID to call SnapshotAll(id) then filter the returned []TaskSnapshot to only include snapshots where TaskSnapshot.GroupID == id so the API is strictly ID-only; update the function SnapshotByID to perform this filter (use SnapshotAll as source but filter by GroupID) and return the filtered slice.
🤖 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 `@task/registry.go`:
- Around line 132-134: OnBeforeGC is being invoked while holding the registry
lock (global.mu), which can block Runs()/GCRuns() readers and cause deadlocks;
instead, inside the critical section (where you currently call OnBeforeGC with
g.ID() and snapshotGroupWithTasks(g)) capture the snapshot of evictees by
calling snapshotGroupWithTasks(g) or equivalent and remove the group while still
holding global.mu, then release the lock and only after unlocking iterate over
the captured snapshot to call OnBeforeGC(g.ID(), capturedSnapshot); ensure you
reference the current call site that uses OnBeforeGC, g.ID(), and
snapshotGroupWithTasks so you snapshot, drop the lock, and then invoke the
callback.
---
Duplicate comments:
In `@task/registry.go`:
- Around line 111-112: SnapshotByID currently delegates to SnapshotAll(id) which
matches on name or ID; change SnapshotByID to call SnapshotAll(id) then filter
the returned []TaskSnapshot to only include snapshots where TaskSnapshot.GroupID
== id so the API is strictly ID-only; update the function SnapshotByID to
perform this filter (use SnapshotAll as source but filter by GroupID) and return
the filtered slice.
🪄 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: 6b6ef7e6-a816-415c-96d6-31f6cf5bb142
📒 Files selected for processing (3)
task/registry.gotask/registry_test.gotask/snapshot.go
Comment on lines
+132
to
+134
| if OnBeforeGC != nil { | ||
| OnBeforeGC(g.ID(), snapshotGroupWithTasks(g)) | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t run OnBeforeGC under the registry lock.
Runs() calls GCRuns() on the read path, so a slow callback here blocks all registry readers/writers behind global.mu. It also turns accidental re-entry into the task package into a hard deadlock. Snapshot the evictees under the lock, remove them, unlock, then invoke the callback.
Possible fix
func GCRuns() {
if global == nil {
return
}
now := time.Now()
+ type evictedRun struct {
+ id string
+ snaps []TaskSnapshot
+ }
+ var evicted []evictedRun
global.mu.Lock()
- defer global.mu.Unlock()
kept := global.groups[:0]
for _, g := range global.groups {
g.observeTerminal(g.Status(), now)
finished := g.FinishedAt()
if !finished.IsZero() && now.Sub(finished) > runRetention {
if OnBeforeGC != nil {
- OnBeforeGC(g.ID(), snapshotGroupWithTasks(g))
+ evicted = append(evicted, evictedRun{
+ id: g.ID(),
+ snaps: snapshotGroupWithTasks(g),
+ })
}
continue
}
kept = append(kept, g)
}
global.groups = kept
+ callback := OnBeforeGC
+ global.mu.Unlock()
+
+ if callback != nil {
+ for _, run := range evicted {
+ callback(run.id, run.snaps)
+ }
+ }
}🤖 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/registry.go` around lines 132 - 134, OnBeforeGC is being invoked while
holding the registry lock (global.mu), which can block Runs()/GCRuns() readers
and cause deadlocks; instead, inside the critical section (where you currently
call OnBeforeGC with g.ID() and snapshotGroupWithTasks(g)) capture the snapshot
of evictees by calling snapshotGroupWithTasks(g) or equivalent and remove the
group while still holding global.mu, then release the lock and only after
unlocking iterate over the captured snapshot to call OnBeforeGC(g.ID(),
capturedSnapshot); ensure you reference the current call site that uses
OnBeforeGC, g.ID(), and snapshotGroupWithTasks so you snapshot, drop the lock,
and then invoke the callback.
Replace separate lint and test workflows with unified gavel workflow for testing and linting. Add new dist workflow to automatically rebuild and commit task-ui bundle when source changes. This consolidates CI/CD configuration and ensures embedded assets stay in sync with source. The gavel workflow runs on push and pull requests, while dist workflow runs on push to main branches only and marks commits with [skip ci] to prevent re-triggering.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+38
to
+53
| name: OpenAPI Integration | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: ${{ env.GO_VERSION }} | ||
| cache: true | ||
|
|
||
| - name: Run OpenAPI integration tests | ||
| run: make test:openapi | ||
| env: | ||
| CGO_ENABLED: 1 |
… backends Introduce a generic timeseries store for recording and querying (timestamp, value) points behind a Timeseries interface. Implementations: - metrics.NewMemory: in-process store with configurable retention and max points, suitable for CLIs, tests, and single-process servers - valkey.New: persistent store backed by valkey/redis sorted sets for cross-process deployments Both share the same wire format (EncodeMember/ParseMember) enabling interoperability. RegisterRoutes serves metrics over HTTP without knowing which backend is used. Includes: - Point codec for lossless round-tripping through sorted set members - HTTP handler with RFC3339 and duration-based time bound parsing - Comprehensive test coverage for both implementations
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
.github/workflows/gavel.yml (4)
41-42: 💤 Low valueConsider setting
persist-credentials: false.Prevents credentials from persisting in the workspace.
🔒 Suggested addition
- name: Checkout code uses: actions/checkout@v4 + with: + persist-credentials: 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 @.github/workflows/gavel.yml around lines 41 - 42, The Checkout code step using actions/checkout@v4 should set persist-credentials: false to avoid leaving repository credentials in the workspace; update the step that references actions/checkout@v4 (the "Checkout code" step) to include the persist-credentials: false input so credentials are not persisted after checkout.
22-22: ⚡ Quick winPin actions to specific commit SHAs.
Both
actions/checkout@v4andactions/setup-go@v5should be pinned to commit SHAs for supply-chain security.Also applies to: 27-27
🤖 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 @.github/workflows/gavel.yml at line 22, Replace the floating version tags with pinned commit SHAs for the GitHub Actions used: change actions/checkout@v4 and actions/setup-go@v5 to their respective full commit SHAs (e.g., actions/checkout@<commit-sha> and actions/setup-go@<commit-sha>), updating both occurrences noted (the checkout at the shown line and setup-go at the other occurrence) by looking up the latest stable commit SHA from each action's repository and using that SHA in the workflow; ensure you commit the workflow file with the SHAs instead of the version tags.
21-24: 💤 Low valueConsider setting
persist-credentials: false.Prevents credentials from persisting in the workspace, reducing risk if artifacts or logs are exposed.
🔒 Suggested addition
- name: Checkout code uses: actions/checkout@v4 with: fetch-depth: 0 + persist-credentials: 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 @.github/workflows/gavel.yml around lines 21 - 24, Update the "Checkout code" GitHub Action step (uses: actions/checkout@v4) to explicitly set persist-credentials: false so credentials are not kept in the workspace; locate the checkout step in the workflow and add the persist-credentials: false key under its with block alongside fetch-depth to prevent tokens from being written to the workspace or retained in artifacts/logs.
42-42: ⚡ Quick winPin actions to specific commit SHAs.
Same supply-chain security concern—pin
actions/checkoutandactions/setup-goto commit SHAs.Also applies to: 45-45
🤖 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 @.github/workflows/gavel.yml at line 42, The workflow currently references actions/checkout@v4 and actions/setup-go (un-pinned); update both "uses" entries to pin them to specific commit SHAs instead of major tags. Replace actions/checkout@v4 and actions/setup-go@... with their corresponding full commit SHAs from the official action repositories (e.g., actions/checkout@<full-commit-sha> and actions/setup-go@<full-commit-sha>), making the change for both occurrences mentioned so the workflow uses immutable action versions..github/workflows/dist.yml (3)
21-21: ⚡ Quick winPin action to specific commit SHA.
Using version tags like
@v4is vulnerable to tag manipulation. Pin to a commit SHA for stronger supply-chain security.🤖 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 @.github/workflows/dist.yml at line 21, The workflow step currently references the floating tag "actions/checkout@v4"; replace it with a pinned commit SHA to prevent tag manipulation by setting uses to "actions/checkout@<full-commit-sha>" (use the official actions/checkout repository commit SHA from GitHub) and commit that change; ensure the uses entry for actions/checkout is updated in the same job and verify any other third‑party actions in the workflow are similarly pinned to their specific commit SHAs.
23-23: ⚡ Quick winConsider removing full history fetch.
fetch-depth: 0fetches the complete git history, but this workflow only needs to build the bundle and commit the result. The default shallow clone should suffice.🔧 Proposed change to use shallow clone
uses: actions/checkout@v4 with: - fetch-depth: 0 persist-credentials: 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 @.github/workflows/dist.yml at line 23, The workflow currently sets fetch-depth: 0 which forces a full git history fetch; change this to use a shallow clone by removing the fetch-depth line or setting fetch-depth: 1 in the checkout step so the job only fetches the latest commit, preserving performance; locate the checkout step that contains "fetch-depth: 0" and replace or remove that key accordingly.
27-27: ⚡ Quick winPin action to specific commit SHA.
Same supply-chain concern as the checkout action—pin to a SHA instead of a version tag.
🤖 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 @.github/workflows/dist.yml at line 27, Replace the floating tag "uses: actions/setup-node@v4" with a pinned commit SHA to prevent supply-chain drift: locate the workflow step containing uses: actions/setup-node@v4 and update it to uses: actions/setup-node@<commit-sha> (the full 40-char commit for the v4 release you intend to track); ensure you pick the exact commit SHA from the actions/setup-node repository for the desired v4 state and commit the change so the workflow references that immutable SHA instead of the version tag.
🤖 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 @.github/workflows/dist.yml:
- Line 42: The git push invocation uses an unquoted expansion of github.ref_name
("git push origin HEAD:${{ github.ref_name }}"), which risks shell injection;
update the workflow to either validate github.ref_name against allowed branch
names (e.g., match expected branches in a conditional) or quote/escape the
variable expansion so it is treated as a single safe ref (e.g., ensure the push
ref is wrapped in quotes or passed as a single argument), and update the step
that runs the push so it references the validated/quoted value instead of the
raw github.ref_name.
In @.github/workflows/gavel.yml:
- Line 33: The workflow currently references the gavel action with the floating
ref "uses: flanksource/gavel@main" which risks CI instability; update that line
to pin to a specific released tag or commit SHA (e.g., replace "`@main`" with
"`@vX.Y.Z`" or "@<commit-sha>") so the action version is immutable and
predictable—locate the "uses: flanksource/gavel@main" entry in the workflow and
change it to the chosen tag or SHA.
- Around line 37-53: The openapi job currently inherits default workflow
permissions; update the openapi job definition to include an explicit
permissions block that grants only the minimum required permissions (for example
permissions: contents: read and set any other permissions to none or read as
appropriate for the steps used). Locate the job named "openapi" and add a
permissions section under it (e.g., permissions: { contents: read }) to restrict
broad defaults while ensuring actions/checkout and the test step have the
required access.
---
Nitpick comments:
In @.github/workflows/dist.yml:
- Line 21: The workflow step currently references the floating tag
"actions/checkout@v4"; replace it with a pinned commit SHA to prevent tag
manipulation by setting uses to "actions/checkout@<full-commit-sha>" (use the
official actions/checkout repository commit SHA from GitHub) and commit that
change; ensure the uses entry for actions/checkout is updated in the same job
and verify any other third‑party actions in the workflow are similarly pinned to
their specific commit SHAs.
- Line 23: The workflow currently sets fetch-depth: 0 which forces a full git
history fetch; change this to use a shallow clone by removing the fetch-depth
line or setting fetch-depth: 1 in the checkout step so the job only fetches the
latest commit, preserving performance; locate the checkout step that contains
"fetch-depth: 0" and replace or remove that key accordingly.
- Line 27: Replace the floating tag "uses: actions/setup-node@v4" with a pinned
commit SHA to prevent supply-chain drift: locate the workflow step containing
uses: actions/setup-node@v4 and update it to uses:
actions/setup-node@<commit-sha> (the full 40-char commit for the v4 release you
intend to track); ensure you pick the exact commit SHA from the
actions/setup-node repository for the desired v4 state and commit the change so
the workflow references that immutable SHA instead of the version tag.
In @.github/workflows/gavel.yml:
- Around line 41-42: The Checkout code step using actions/checkout@v4 should set
persist-credentials: false to avoid leaving repository credentials in the
workspace; update the step that references actions/checkout@v4 (the "Checkout
code" step) to include the persist-credentials: false input so credentials are
not persisted after checkout.
- Line 22: Replace the floating version tags with pinned commit SHAs for the
GitHub Actions used: change actions/checkout@v4 and actions/setup-go@v5 to their
respective full commit SHAs (e.g., actions/checkout@<commit-sha> and
actions/setup-go@<commit-sha>), updating both occurrences noted (the checkout at
the shown line and setup-go at the other occurrence) by looking up the latest
stable commit SHA from each action's repository and using that SHA in the
workflow; ensure you commit the workflow file with the SHAs instead of the
version tags.
- Around line 21-24: Update the "Checkout code" GitHub Action step (uses:
actions/checkout@v4) to explicitly set persist-credentials: false so credentials
are not kept in the workspace; locate the checkout step in the workflow and add
the persist-credentials: false key under its with block alongside fetch-depth to
prevent tokens from being written to the workspace or retained in
artifacts/logs.
- Line 42: The workflow currently references actions/checkout@v4 and
actions/setup-go (un-pinned); update both "uses" entries to pin them to specific
commit SHAs instead of major tags. Replace actions/checkout@v4 and
actions/setup-go@... with their corresponding full commit SHAs from the official
action repositories (e.g., actions/checkout@<full-commit-sha> and
actions/setup-go@<full-commit-sha>), making the change for both occurrences
mentioned so the workflow uses immutable action versions.
🪄 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: 530daf6c-6edf-4cb9-870f-f9b617799298
📒 Files selected for processing (4)
.github/workflows/dist.yml.github/workflows/gavel.yml.github/workflows/lint.yml.github/workflows/test.yml
💤 Files with no reviewable changes (2)
- .github/workflows/lint.yml
- .github/workflows/test.yml
| if ! git diff --cached --quiet; then | ||
| git commit -m "chore(embed): rebuild task-ui bundle [skip ci]" | ||
| git remote set-url origin "https://x-access-token:${GH_TOKEN}@github.com/${{ github.repository }}.git" | ||
| git push origin HEAD:${{ github.ref_name }} |
Contributor
There was a problem hiding this comment.
Validate or quote the branch reference to prevent injection.
github.ref_name is expanded into the git push command without validation or quoting. Although this workflow is restricted to specific branches, branch names containing shell metacharacters could lead to command injection if branch protections are misconfigured.
🛡️ Proposed fix to quote the variable
git remote set-url origin "https://x-access-token:${GH_TOKEN}`@github.com/`${{ github.repository }}.git"
- git push origin HEAD:${{ github.ref_name }}
+ git push origin "HEAD:${{ github.ref_name }}"Alternatively, validate that ref_name matches expected branches:
git remote set-url origin "https://x-access-token:${GH_TOKEN}`@github.com/`${{ github.repository }}.git"
+ case "${{ github.ref_name }}" in
+ main|master|develop) ;;
+ *) echo "Unexpected branch: ${{ github.ref_name }}"; exit 1 ;;
+ esac
git push origin HEAD:${{ github.ref_name }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| git push origin HEAD:${{ github.ref_name }} | |
| git push origin "HEAD:${{ github.ref_name }}" |
🧰 Tools
🪛 zizmor (1.25.2)
[error] 42-42: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/dist.yml at line 42, The git push invocation uses an
unquoted expansion of github.ref_name ("git push origin HEAD:${{ github.ref_name
}}"), which risks shell injection; update the workflow to either validate
github.ref_name against allowed branch names (e.g., match expected branches in a
conditional) or quote/escape the variable expansion so it is treated as a single
safe ref (e.g., ensure the push ref is wrapped in quotes or passed as a single
argument), and update the step that runs the push so it references the
validated/quoted value instead of the raw github.ref_name.
| cache: true | ||
|
|
||
| - name: Gavel | ||
| uses: flanksource/gavel@main |
Contributor
There was a problem hiding this comment.
Pin gavel action to a specific version or commit SHA.
Using @main means this workflow depends on the latest, potentially unstable version of gavel. Every push to gavel's main branch will immediately affect your CI, risking unexpected breakage.
📌 Recommended fix
- name: Gavel
- uses: flanksource/gavel@main
+ uses: flanksource/gavel@v1.2.3 # or commit SHA
with:
args: test --lintCheck the gavel repository for the latest stable release tag or commit SHA.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| uses: flanksource/gavel@main | |
| uses: flanksource/gavel@v1.2.3 # or commit SHA |
🧰 Tools
🪛 zizmor (1.25.2)
[error] 33-33: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/gavel.yml at line 33, The workflow currently references
the gavel action with the floating ref "uses: flanksource/gavel@main" which
risks CI instability; update that line to pin to a specific released tag or
commit SHA (e.g., replace "`@main`" with "`@vX.Y.Z`" or "@<commit-sha>") so the
action version is immutable and predictable—locate the "uses:
flanksource/gavel@main" entry in the workflow and change it to the chosen tag or
SHA.
Comment on lines
+37
to
+53
| openapi: | ||
| name: OpenAPI Integration | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: ${{ env.GO_VERSION }} | ||
| cache: true | ||
|
|
||
| - name: Run OpenAPI integration tests | ||
| run: make test:openapi | ||
| env: | ||
| CGO_ENABLED: 1 |
Contributor
There was a problem hiding this comment.
Add explicit permissions block to the openapi job.
The openapi job inherits default permissions, which may be overly broad. Explicitly grant only the minimum required permissions.
🔐 Proposed fix
openapi:
name: OpenAPI Integration
runs-on: ubuntu-latest
+ permissions:
+ contents: read
steps:🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 38-53: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 zizmor (1.25.2)
[warning] 41-42: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 37-54: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 42-42: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 45-45: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/gavel.yml around lines 37 - 53, The openapi job currently
inherits default workflow permissions; update the openapi job definition to
include an explicit permissions block that grants only the minimum required
permissions (for example permissions: contents: read and set any other
permissions to none or read as appropriate for the steps used). Locate the job
named "openapi" and add a permissions section under it (e.g., permissions: {
contents: read }) to restrict broad defaults while ensuring actions/checkout and
the test step have the required access.
Remove oipa-specific naming from timeseries module to make it more generic and reusable. Update KeyPrefix from 'oipa:' to 'app:' in both production code and tests, and clarify the comment about l2OpTimeout to be implementation-agnostic.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
Gavel summary
Totals: 1409 passed · 4 failed · 24 skipped · 23.2s Failing testsgo test Executiongolangci-lint execution failed: fork/exec /home/runner/work/clicky/clicky/.gavel/golangci-lint: exec format error tsc wrapper failed: exit status 1 tsc wrapper failed: exit status 1 |
… builds Switch from mattn/go-sqlite3 (requires CGO) to modernc.org/sqlite (pure Go implementation) to enable building clicky without CGO dependencies. The modernc.org/sqlite driver supports WAL pragmas and standard DDL required by the cache implementation. Updates: - Replace sqlite3 driver import with modernc.org/sqlite - Update sql.Open() call to use "sqlite" driver name instead of "sqlite3" - Add modernc.org/sqlite and its dependencies to go.mod - Remove github.com/mattn/go-sqlite3 dependency
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
metrics/inmemory.go (1)
58-61: ⚡ Quick winAvoid full re-sort on the common in-order append.
Recordrunssort.Sliceover the whole series on every call. Since the new point is usually the latest, you can skip the sort when it already lands in order and only sort on the rare out-of-order insert, keeping the hot ingest path near O(1) instead of O(n log n).♻️ Proposed guard
- pts := append(m.series[req.ID], Point{At: at, Value: req.Value}) - // Keep ascending by time; the appended point is usually already the - // latest, so this is a near-no-op in the common path. - sort.Slice(pts, func(i, j int) bool { return pts[i].At.Before(pts[j].At) }) + pts := append(m.series[req.ID], Point{At: at, Value: req.Value}) + // Keep ascending by time. The appended point is usually already the + // latest, so only re-sort when it actually lands out of order. + if n := len(pts); n > 1 && pts[n-1].At.Before(pts[n-2].At) { + sort.Slice(pts, func(i, j int) bool { return pts[i].At.Before(pts[j].At) }) + }🤖 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 `@metrics/inmemory.go` around lines 58 - 61, The Record implementation currently always re-sorts pts after appending, which is expensive; modify Record (the function that builds pts from m.series[req.ID]) to check if the new Point's timestamp is >= the last point's At and, if so, append without sorting, otherwise perform an out-of-order insert (use sort.Search to find insertion index and splice in the new Point) or fall back to sort.Slice; reference m.series, pts, Point and Record when making this change so only out-of-order writes pay the sort cost.
🤖 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 `@valkey/timeseries.go`:
- Around line 64-82: The retention cleanup currently computes cutoffMs from the
incoming sample timestamp (at := req.At), which lets backfills extend the
retention window; change the cutoff computation to use the current time instead
(e.g., cutoffMs := time.Now().Add(-s.retention).UnixMilli()) while still using
req.At (or at) only for the stored Point timestamp and ZADD score; update the
reference to cutoffMs in the ZREMRANGEBYSCORE command and leave the rest
(Expire, ZADD) unchanged so retention is anchored to now not the sample time.
- Line 82: The code calls
s.client.B().Expire().Key(key).Seconds(int64(s.retention.Seconds())).Build(),
which truncates fractional seconds and can turn TTLs <1s into 0; change to use a
millisecond-based command (PExpire/PExpireMillis) and pass
int64(s.retention.Milliseconds()) (or use Nanoseconds if the client supports it)
so sub-second retention is preserved; update the call site from
Expire().Seconds(...) to the millisecond variant (e.g.,
PExpire().Key(key).Milliseconds(int64(s.retention.Milliseconds())).Build()) and
ensure you still handle zero/negative retention appropriately.
---
Nitpick comments:
In `@metrics/inmemory.go`:
- Around line 58-61: The Record implementation currently always re-sorts pts
after appending, which is expensive; modify Record (the function that builds pts
from m.series[req.ID]) to check if the new Point's timestamp is >= the last
point's At and, if so, append without sorting, otherwise perform an out-of-order
insert (use sort.Search to find insertion index and splice in the new Point) or
fall back to sort.Slice; reference m.series, pts, Point and Record when making
this change so only out-of-order writes pay the sort cost.
🪄 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: e559a09b-ffb7-4593-ab12-882d4127c755
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumvalkey/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
ai/cache/cache.gogo.modmetrics/codec.gometrics/handler.gometrics/inmemory.gometrics/metrics_suite_test.gometrics/timeseries.gometrics/timeseries_test.govalkey/go.modvalkey/timeseries.govalkey/timeseries_test.govalkey/valkey_suite_test.go
✅ Files skipped from review due to trivial changes (2)
- valkey/timeseries_test.go
- metrics/timeseries.go
Comment on lines
+64
to
+82
| at := req.At | ||
| if at.IsZero() { | ||
| at = time.Now() | ||
| } | ||
| key := s.key(req.ID) | ||
| point := metrics.Point{At: at, Value: req.Value} | ||
| cutoffMs := at.Add(-s.retention).UnixMilli() | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), opTimeout) | ||
| defer cancel() | ||
|
|
||
| cmds := valkey.Commands{ | ||
| s.client.B().Zadd().Key(key).ScoreMember(). | ||
| ScoreMember(float64(at.UnixMilli()), metrics.EncodeMember(point)).Build(), | ||
| // Exclusive lower bound "(<cutoff>" keeps a point recorded exactly at | ||
| // the cutoff edge. | ||
| s.client.B().Zremrangebyscore().Key(key). | ||
| Min("-inf").Max("(" + strconv.FormatInt(cutoffMs, 10)).Build(), | ||
| s.client.B().Expire().Key(key).Seconds(int64(s.retention.Seconds())).Build(), |
Contributor
There was a problem hiding this comment.
Anchor retention cleanup to current time, not req.At.
Right now Line 70 derives the cutoff from the sample timestamp. If a caller backfills an older point, this widens the effective window for that write: already-expired points can remain in the ZSET, and the backfilled point itself can be stored even though it is older than the configured retention. The retention cutoff should be computed from time.Now(), while req.At should only control the point timestamp.
Proposed fix
func (s *store) Record(req metrics.RecordRequest) error {
- at := req.At
- if at.IsZero() {
- at = time.Now()
- }
+ now := time.Now()
+ at := req.At
+ if at.IsZero() {
+ at = now
+ }
key := s.key(req.ID)
point := metrics.Point{At: at, Value: req.Value}
- cutoffMs := at.Add(-s.retention).UnixMilli()
+ cutoffMs := now.Add(-s.retention).UnixMilli()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| at := req.At | |
| if at.IsZero() { | |
| at = time.Now() | |
| } | |
| key := s.key(req.ID) | |
| point := metrics.Point{At: at, Value: req.Value} | |
| cutoffMs := at.Add(-s.retention).UnixMilli() | |
| ctx, cancel := context.WithTimeout(context.Background(), opTimeout) | |
| defer cancel() | |
| cmds := valkey.Commands{ | |
| s.client.B().Zadd().Key(key).ScoreMember(). | |
| ScoreMember(float64(at.UnixMilli()), metrics.EncodeMember(point)).Build(), | |
| // Exclusive lower bound "(<cutoff>" keeps a point recorded exactly at | |
| // the cutoff edge. | |
| s.client.B().Zremrangebyscore().Key(key). | |
| Min("-inf").Max("(" + strconv.FormatInt(cutoffMs, 10)).Build(), | |
| s.client.B().Expire().Key(key).Seconds(int64(s.retention.Seconds())).Build(), | |
| now := time.Now() | |
| at := req.At | |
| if at.IsZero() { | |
| at = now | |
| } | |
| key := s.key(req.ID) | |
| point := metrics.Point{At: at, Value: req.Value} | |
| cutoffMs := now.Add(-s.retention).UnixMilli() | |
| ctx, cancel := context.WithTimeout(context.Background(), opTimeout) | |
| defer cancel() | |
| cmds := valkey.Commands{ | |
| s.client.B().Zadd().Key(key).ScoreMember(). | |
| ScoreMember(float64(at.UnixMilli()), metrics.EncodeMember(point)).Build(), | |
| // Exclusive lower bound "(<cutoff>" keeps a point recorded exactly at | |
| // the cutoff edge. | |
| s.client.B().Zremrangebyscore().Key(key). | |
| Min("-inf").Max("(" + strconv.FormatInt(cutoffMs, 10)).Build(), | |
| s.client.B().Expire().Key(key).Seconds(int64(s.retention.Seconds())).Build(), |
🤖 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 `@valkey/timeseries.go` around lines 64 - 82, The retention cleanup currently
computes cutoffMs from the incoming sample timestamp (at := req.At), which lets
backfills extend the retention window; change the cutoff computation to use the
current time instead (e.g., cutoffMs :=
time.Now().Add(-s.retention).UnixMilli()) while still using req.At (or at) only
for the stored Point timestamp and ZADD score; update the reference to cutoffMs
in the ZREMRANGEBYSCORE command and leave the rest (Expire, ZADD) unchanged so
retention is anchored to now not the sample time.
| // the cutoff edge. | ||
| s.client.B().Zremrangebyscore().Key(key). | ||
| Min("-inf").Max("(" + strconv.FormatInt(cutoffMs, 10)).Build(), | ||
| s.client.B().Expire().Key(key).Seconds(int64(s.retention.Seconds())).Build(), |
Contributor
There was a problem hiding this comment.
Avoid truncating sub-second retention to EXPIRE 0.
Line 82 converts the TTL with int64(s.retention.Seconds()), which truncates fractional seconds. Any retention between 0 and 1 second becomes 0, so the key is deleted immediately on every write.
Proposed fix
+ ttlSeconds := int64(s.retention / time.Second)
+ if s.retention%time.Second != 0 {
+ ttlSeconds++
+ }
+ if ttlSeconds < 1 {
+ ttlSeconds = 1
+ }
cmds := valkey.Commands{
s.client.B().Zadd().Key(key).ScoreMember().
ScoreMember(float64(at.UnixMilli()), metrics.EncodeMember(point)).Build(),
// Exclusive lower bound "(<cutoff>" keeps a point recorded exactly at
// the cutoff edge.
s.client.B().Zremrangebyscore().Key(key).
Min("-inf").Max("(" + strconv.FormatInt(cutoffMs, 10)).Build(),
- s.client.B().Expire().Key(key).Seconds(int64(s.retention.Seconds())).Build(),
+ s.client.B().Expire().Key(key).Seconds(ttlSeconds).Build(),
}🤖 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 `@valkey/timeseries.go` at line 82, The code calls
s.client.B().Expire().Key(key).Seconds(int64(s.retention.Seconds())).Build(),
which truncates fractional seconds and can turn TTLs <1s into 0; change to use a
millisecond-based command (PExpire/PExpireMillis) and pass
int64(s.retention.Milliseconds()) (or use Nanoseconds if the client supports it)
so sub-second retention is preserved; update the call site from
Expire().Seconds(...) to the millisecond variant (e.g.,
PExpire().Key(key).Milliseconds(int64(s.retention.Milliseconds())).Build()) and
ensure you still handle zero/negative retention appropriately.
Expand comments to explain why modernc.org/sqlite is used instead of glebarez/go-sqlite. Both drivers register the same "sqlite" driver name, and using both would cause a panic at initialization. This clarification helps maintainers understand the constraint and prevents accidental driver substitution.
Contributor
Gavel crashed before producing resultsExit code: 1 Last lines of gavel.logFull |
Contributor
Gavel summary
Totals: 1386 passed · 0 failed · 23 skipped · 11.7s |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/gavel.yml (2)
79-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
persist-credentials: falseto the openapi job checkout action.The openapi job checks out code without disabling credential persistence, which could expose credentials if subsequent steps inadvertently leak the repository state.
🔐 Proposed fix
- name: Checkout code uses: actions/checkout@v4 + with: + persist-credentials: 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 @.github/workflows/gavel.yml at line 79, In the openapi job's checkout step (the uses: actions/checkout@v4 entry) add the parameter persist-credentials: false to disable credential persistence after checkout; locate the checkout step in the openapi job and update it to include persist-credentials: false under that action to prevent credentials from being stored for later steps.
22-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
persist-credentials: falseto checkout actions for security.Both the test and lint jobs check out code without explicitly disabling credential persistence. If subsequent steps or artifacts inadvertently expose the repository, persisted credentials could leak.
🔐 Proposed fix to disable credential persistence
For the test job:
- name: Checkout code uses: actions/checkout@v4 with: fetch-depth: 0 + persist-credentials: falseFor the lint job:
- name: Checkout code uses: actions/checkout@v4 with: fetch-depth: 0 + persist-credentials: falseAlso applies to: 52-52
🤖 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 @.github/workflows/gavel.yml at line 22, Add persist-credentials: false to the actions/checkout@v4 steps so credentials are not persisted; locate each checkout invocation in .github/workflows/gavel.yml (e.g., the uses: actions/checkout@v4 entries for the test and lint jobs) and add the persist-credentials: false option under that step to disable credential persistence for both occurrences (also the occurrence around line 52).
🤖 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.
Outside diff comments:
In @.github/workflows/gavel.yml:
- Line 79: In the openapi job's checkout step (the uses: actions/checkout@v4
entry) add the parameter persist-credentials: false to disable credential
persistence after checkout; locate the checkout step in the openapi job and
update it to include persist-credentials: false under that action to prevent
credentials from being stored for later steps.
- Line 22: Add persist-credentials: false to the actions/checkout@v4 steps so
credentials are not persisted; locate each checkout invocation in
.github/workflows/gavel.yml (e.g., the uses: actions/checkout@v4 entries for the
test and lint jobs) and add the persist-credentials: false option under that
step to disable credential persistence for both occurrences (also the occurrence
around line 52).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a196a9fb-7ac3-4110-bc97-ee8f3b7d887f
📒 Files selected for processing (2)
.github/workflows/gavel.ymlai/cache/cache.go
🚧 Files skipped from review as they are similar to previous changes (1)
- ai/cache/cache.go
The serve handler conflated the command render format (pretty/tree) with the HTTP wire format, so a request with format=pretty and Accept: application/json returned ANSI-colored text instead of a JSON envelope, breaking JSON parsing. Treat pretty/tree as render-only formats that flow to the executed command, and let the Accept header pick the wire format. For structured wire formats (json/clicky-json/yaml) serialize the ExecutionResponse envelope so output, success, and exit_code remain addressable.
Each demo declares its own func main() in package main, so they collided and the examples package failed to build, breaking 'go test ./...' in CI. Mark each as //go:build ignore so they are excluded from package builds/tests while remaining runnable via 'go run examples/<file>.go'.
CI ran 'go test ./...' in the examples module and failed with 'updates to go.mod needed'. The module was already untidy on the branch; tagging the demos //go:build ignore also moved echo/commons/pflag from direct to indirect requires. Run go mod tidy so the module builds cleanly in CI.
Add a committed placeholder index.html in examples/enitity/webapp/dist/ to ensure the //go:embed directive in main.go resolves during CI builds without requiring a frontend build step. The real hashed bundles are generated by `pnpm build` which overwrites this placeholder. Updated .gitignore to keep both the dist directory and index.html committed while ignoring other dist contents.
SetStatus cancels the task's own context on terminal statuses (Success/Failed/Warning/Cancelled). A task closure that calls t.Success() before `return result` therefore cancels t.ctx while runFunc is still storing the result. WaitFor watched t.ctx.Done() and returned the instant the closure self-cancelled — before the result landed — so TypedGroup.GetResults() intermittently handed back the zero value. Only treat ctx.Done() as a real cancellation when the status is still non-terminal. On a terminal self-cancel, wait on doneChan (closed by the worker right after it stores the result and flips completed), with the overall timeout as a backstop, instead of bailing out. Adds a regression test that fails reliably on the prior behavior and passes with the fix.
…t-scoped state Add context-aware variants of data functions throughout the codebase to enable handlers to resolve per-request state (e.g., database/config bundles) instead of relying on process globals. Key changes: - Introduce ContextDataFunc type that receives request context alongside flags/args - Add AddNamedCommandWithContext, ActionWithContext, ActionWithFlagsAndContext constructors - Add context-aware variants to Entity: ListWithContext, GetWithContext, GetWithFlagsAndContext, CreateWithContext, UpdateWithContext, DeleteWithContext - Implement SearchableFilter interface for large option sets with server-side search and pagination - Add entityLookupFilter.Truncated and Total fields to signal truncated results - Thread request context through CLI (cmd.Context()) and HTTP (r.Context()) execution paths - Update RPC executor to prefer ContextDataFunc over DataFunc when both are present - Add comprehensive tests for context threading and searchable filter behavior BREAKING CHANGE: None - all changes are additive with backward compatibility maintained through fallback to non-context variants.
|
🎉 This PR is included in version 1.21.14 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Notes
annotateEntityOperationCommandnow requires an additionaloptionalIDboolean parameterSummary by CodeRabbit
New Features
Improvements
Tests
Chores