feat(tracking): add hierarchical user path scoping#189
feat(tracking): add hierarchical user path scoping#189SantiagoDePolonia merged 10 commits intomainfrom
Conversation
…r-tracking # Conflicts: # internal/admin/dashboard/static/js/modules/execution-plans.js # internal/auditlog/reader_postgresql.go # internal/auditlog/reader_sqlite.go # internal/auditlog/store_postgresql.go # internal/auditlog/store_postgresql_test.go # internal/auditlog/store_sqlite.go # internal/auditlog/store_sqlite_test.go
|
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:
📝 WalkthroughWalkthroughAdds first-class canonical user-path handling from the X-GoModel-User-Path header: normalize at ingress, store in request snapshots/context, thread into execution-plan selection/matching with ancestor fallback, persist user_path on audit/usage/batch records across backends, support subtree filtering, and expose filters/UI in the admin dashboard. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as Server (Handler)
participant Snapshot as Request Snapshot
participant Exec as ExecutionPlan Service
participant UsageDB as Usage Store
participant AuditDB as Audit Store
Client->>API: HTTP request with X-GoModel-User-Path
API->>API: core.NormalizeUserPath(raw)
API->>Snapshot: Create snapshot with normalized UserPath
API->>Exec: Build selector(provider, model, userPath)
Exec->>Exec: UserPathAncestors() → match most-specific → resolved policy
Exec-->>API: Resolved execution policy
API->>UsageDB: Write usage entry (UserPath from context)
API->>AuditDB: Write audit entry (UserPath from context)
UsageDB-->>API: Persisted
AuditDB-->>API: Persisted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/core/request_snapshot_test.go (1)
88-103: 🧹 Nitpick | 🔵 TrivialAdd an explicit
UserPathassertion for the owned-body constructor test.Line 88 passes
"/team/a"intoNewRequestSnapshotWithOwnedBody, but this test never verifies the field was retained.✅ Suggested test addition
func TestNewRequestSnapshotWithOwnedBody_TakesOwnershipOfCapturedBytes(t *testing.T) { @@ snapshot := NewRequestSnapshotWithOwnedBody( @@ "/team/a", ) + + if got := snapshot.UserPath; got != "/team/a" { + t.Fatalf("UserPath = %q, want /team/a", got) + } view := snapshot.CapturedBodyView()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/request_snapshot_test.go` around lines 88 - 103, The test constructs a snapshot via NewRequestSnapshotWithOwnedBody passing "/team/a" but never asserts the UserPath was preserved; add an assertion after creating snapshot (the variable named snapshot) that the snapshot's UserPath (snapshot.UserPath or snapshot.UserPath() depending on the API) equals "/team/a" to ensure the owned-body constructor retained the path.internal/executionplans/service.go (2)
261-263:⚠️ Potential issue | 🔴 CriticalDon't treat path-only scopes as global during deactivation.
Path-only plans now legitimately have
Provider == ""andModel == "", so this guard rejects them as if they were the default global plan. Those plans become impossible to deactivate.🔧 Proposed fix
- if scope.Provider == "" && scope.Model == "" { + if scope.Provider == "" && scope.Model == "" && scope.UserPath == "" { return newValidationError("cannot deactivate the global workflow", nil) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/executionplans/service.go` around lines 261 - 263, The guard erroneously treats path-only scopes (which have Provider=="" and Model=="") as the global plan; update the condition to only reject deactivation when the scope is truly global by also checking that the path is empty (i.e., change the if from "if scope.Provider == \"\" && scope.Model == \"\" {" to include scope.Path == \"\"), so only when Provider, Model, and Path are all empty do you return newValidationError("cannot deactivate the global workflow", nil); locate the check around the scope variable in internal/executionplans/service.go (the deactivation/validation block) and adjust the conditional accordingly.
612-625:⚠️ Potential issue | 🟡 MinorKeep view specificity aligned with runtime precedence.
matchCompiledand the design spec both prefer plainpathover plainprovider, but this ranking does the opposite. The active-plan list can therefore present a precedence order that disagrees with what the matcher will actually select.🔧 Proposed fix
case "global": return 0 - case "path": - return 1 case "provider": - return 2 + return 1 + case "path": + return 2 case "provider_path": return 3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/executionplans/service.go` around lines 612 - 625, The specificity ordering in viewScopeSpecificity is inverted for plain "path" vs "provider" (it currently returns path=1, provider=2) which conflicts with matchCompiled and runtime precedence; update viewScopeSpecificity so "path" has higher specificity than "provider" (e.g., swap their returned integers so "path" > "provider") while keeping provider-scoped entries ("provider_path", "provider_model") more specific than both; modify the return values in the viewScopeSpecificity function accordingly to match matchCompiled's precedence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-03-30-user-path-tracking.md`:
- Around line 11-13: The document jumps from the top-level title to a level-3
heading for "Task 1: Canonical User Path Core"; change that heading to level-2
(replace the leading "###" with "##") so headings increment sequentially from h1
to h2, and scan other task headings in the file to ensure all "###" task
headings are updated to "##" for consistent markdown heading level progression.
In `@docs/superpowers/specs/2026-03-30-user-path-design.md`:
- Around line 82-94: Add an explicit statement that the root subtree filter "/"
matches the entire hierarchy (i.e., filter "/" matches "/" and all descendant
paths such as "/team" and "/team/a"), and clarify that applying "/" as an admin
filter selects all users/paths for the listed reports (usage summary, usage
daily, usage by model, usage log, audit log); update the `user_path` subtree
filter section and examples to include this root-case contract so SQL behavior
and tests are anchored to the documented expectation.
In `@internal/admin/dashboard/static/js/modules/execution-plans.js`:
- Around line 525-530: The hydrated-scope equality logic is missing the
scope_user_path check causing false positives; update the sameHydratedScope
computation (where executionPlanHydratedScope and hydratedScope are used) to
also compare String(hydratedScope.scope_user_path || '').trim() ===
String(userPath || '').trim() (or the variable name used for the current user
path) alongside scope_provider and scope_model so the comparison considers
provider, model, and scope_user_path together.
In `@internal/admin/dashboard/static/js/modules/execution-plans.test.js`:
- Around line 1012-1036: Test only verifies validation of a slashless
scope_user_path but doesn't assert that a slashless input matches a persisted
model whose stored scope_user_path includes a leading slash; update the test to
ensure matching logic in executionPlanActiveScopeMatch/executionPlanSubmitMode
is exercised by (1) setting module.models to include an existing workflow with
scope_user_path '/team/alpha' (note the leading slash) and (2) keeping
payload.scope_user_path as 'team/alpha', then assert that
validateExecutionPlanRequest still returns '' and that the module's
executionPlanActiveScopeMatch/executionPlanSubmitMode logic treats them as the
same scope (i.e., selects the persisted model/submit mode accordingly); if
matching currently fails, change the comparison logic in
executionPlanActiveScopeMatch or executionPlanSubmitMode to canonicalize both
values (trim or ensure a leading slash consistently) before comparing.
In `@internal/admin/dashboard/templates/index.html`:
- Line 155: The input for the usage log user path filter (the element bound to
x-model="usageLogUserPath" and calling
`@input.debounce.300ms`="fetchUsageLog(true)" with class "filter-input") is
missing an aria-label; add a descriptive aria-label (e.g., "Filter by user path"
or "User path filter") to match other toolbar filters so screen readers can
identify the control and improve accessibility.
In `@internal/core/execution_plan.go`:
- Around line 84-90: The code currently assigns a trimmed raw value to
selector.UserPath when NormalizeUserPath(userPath[0]) fails; change this to not
keep the raw path so it doesn't mismatch canonical persisted scopes: in the
block that calls NormalizeUserPath, on err set selector.UserPath = "" (or simply
omit assignment) instead of selector.UserPath = strings.TrimSpace(userPath[0]);
reference NormalizeUserPath, selector.UserPath and userPath to locate the change
and ensure no trimmed fallback remains (alternatively, if you prefer surfacing
the error, return or propagate the normalization error from the surrounding
function rather than storing the raw path).
In `@internal/core/user_path_test.go`:
- Around line 44-58: Add a focused unit test for the root path case to ensure
UserPathAncestors("/") returns only the root; create a new test (e.g.,
TestUserPathAncestors_Root) that calls UserPathAncestors("/") and asserts the
result length is 1 and the single element equals "/" so edge-case behavior of
UserPathAncestors is covered alongside the existing multi-segment test.
In `@internal/executionplans/store_sqlite.go`:
- Around line 52-54: The migration currently skips duplicate-column errors by
string-matching err.Error() for "duplicate column" (inside the block that
executes the ALTER for execution_plan_versions and checks variable statement),
which is fragile; change this to a robust existence check before executing ALTER
TABLE: query pragma_table_info('execution_plan_versions') (or SELECT COUNT(*) >
0 FROM pragma_table_info(...) WHERE name = 'scope_user_path') to detect if
scope_user_path already exists and only run ALTER TABLE execution_plan_versions
ADD COLUMN scope_user_path TEXT when it does not, and return any real execution
errors instead of relying on error string matching.
In `@internal/usage/stream_observer.go`:
- Around line 18-34: NewStreamUsageObserver stores the raw variadic userPath
into the StreamUsageObserver.userPath which can contain missing leading slashes
or duplicate slashes and later break canonical subtree filters; update
NewStreamUsageObserver to normalize the provided userPath (e.g., pick
userPath[0] if present, trim whitespace, ensure a single leading slash, collapse
consecutive slashes, and remove trailing slash unless root) before assigning to
the observer so UsageEntry.UserPath is always canonical for downstream
comparisons in StreamUsageObserver.
---
Outside diff comments:
In `@internal/core/request_snapshot_test.go`:
- Around line 88-103: The test constructs a snapshot via
NewRequestSnapshotWithOwnedBody passing "/team/a" but never asserts the UserPath
was preserved; add an assertion after creating snapshot (the variable named
snapshot) that the snapshot's UserPath (snapshot.UserPath or snapshot.UserPath()
depending on the API) equals "/team/a" to ensure the owned-body constructor
retained the path.
In `@internal/executionplans/service.go`:
- Around line 261-263: The guard erroneously treats path-only scopes (which have
Provider=="" and Model=="") as the global plan; update the condition to only
reject deactivation when the scope is truly global by also checking that the
path is empty (i.e., change the if from "if scope.Provider == \"\" &&
scope.Model == \"\" {" to include scope.Path == \"\"), so only when Provider,
Model, and Path are all empty do you return newValidationError("cannot
deactivate the global workflow", nil); locate the check around the scope
variable in internal/executionplans/service.go (the deactivation/validation
block) and adjust the conditional accordingly.
- Around line 612-625: The specificity ordering in viewScopeSpecificity is
inverted for plain "path" vs "provider" (it currently returns path=1,
provider=2) which conflicts with matchCompiled and runtime precedence; update
viewScopeSpecificity so "path" has higher specificity than "provider" (e.g.,
swap their returned integers so "path" > "provider") while keeping
provider-scoped entries ("provider_path", "provider_model") more specific than
both; modify the return values in the viewScopeSpecificity function accordingly
to match matchCompiled's precedence.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f513f198-943f-41b0-8ae4-b82ec5dead87
📒 Files selected for processing (67)
docs/superpowers/plans/2026-03-30-user-path-tracking.mddocs/superpowers/specs/2026-03-30-user-path-design.mdinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/audit-list.jsinternal/admin/dashboard/static/js/modules/execution-plans.jsinternal/admin/dashboard/static/js/modules/execution-plans.test.jsinternal/admin/dashboard/static/js/modules/usage.jsinternal/admin/dashboard/templates/index.htmlinternal/admin/handler.gointernal/admin/handler_executionplans_test.gointernal/admin/handler_test.gointernal/auditlog/auditlog.gointernal/auditlog/middleware.gointernal/auditlog/reader.gointernal/auditlog/reader_mongodb.gointernal/auditlog/reader_postgresql.gointernal/auditlog/reader_sqlite.gointernal/auditlog/store_mongodb.gointernal/auditlog/store_postgresql.gointernal/auditlog/store_postgresql_test.gointernal/auditlog/store_sqlite.gointernal/auditlog/store_sqlite_test.gointernal/auditlog/user_path_filter.gointernal/auditlog/user_path_filter_test.gointernal/batch/store.gointernal/batch/store_test.gointernal/core/execution_plan.gointernal/core/request_snapshot.gointernal/core/request_snapshot_test.gointernal/core/user_path.gointernal/core/user_path_test.gointernal/executionplans/compiler.gointernal/executionplans/service.gointernal/executionplans/service_test.gointernal/executionplans/store_mongodb.gointernal/executionplans/store_postgresql.gointernal/executionplans/store_sqlite.gointernal/executionplans/types.gointernal/executionplans/types_test.gointernal/server/execution_plan_helpers.gointernal/server/handlers_test.gointernal/server/model_validation.gointernal/server/model_validation_test.gointernal/server/native_batch_service.gointernal/server/native_batch_support.gointernal/server/native_batch_support_test.gointernal/server/passthrough_support.gointernal/server/request_snapshot.gointernal/server/request_snapshot_test.gointernal/server/translated_inference_service.gointernal/server/translated_inference_service_test.gointernal/usage/reader.gointernal/usage/reader_mongodb.gointernal/usage/reader_postgresql.gointernal/usage/reader_sqlite.gointernal/usage/reader_sqlite_boundary_test.gointernal/usage/store_mongodb.gointernal/usage/store_postgresql.gointernal/usage/store_postgresql_test.gointernal/usage/store_sqlite.gointernal/usage/stream_observer.gointernal/usage/stream_observer_test.gointernal/usage/usage.gointernal/usage/user_path_filter.gointernal/usage/user_path_filter_test.gotests/integration/dbassert/auditlog.gotests/integration/dbassert/usage.go
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-03-30-user-path-tracking.md`:
- Around line 137-143: Replace the incorrect Go test invocation used in Step 5
("go test ./internal/admin/dashboard/...") with the appropriate JS test runner
command that actually executes the dashboard JS module tests (e.g., the monorepo
test command you use like `pnpm test ./internal/admin/dashboard` or `npm run
test --workspace=./internal/admin/dashboard`); update every occurrence of "go
test ./internal/admin/dashboard/..." in the Step 5 verification text so it
consistently references the JS test runner and matches the JS test files listed
in the section.
In `@internal/admin/dashboard/static/js/modules/execution-plans.js`:
- Around line 245-250: executionPlanPreview() currently only emits 'global',
'provider', and 'provider_model' into scope_display, which causes drafts with
path scopes to show wrong labels; update executionPlanPreview (and the similar
block around the 314-355 region) to map the new scope types ('path',
'provider_path', 'provider_model_path') and the incoming draft scope key
'scope_user_path' into the corresponding scope_display values so they align with
planScopeTypeLabel(plan); ensure executionPlanPreview returns scope_display
values that match planScopeTypeLabel's expected strings (e.g., 'Path', 'Provider
+ Path', 'Provider + Model + Path') and handle any trimming/normalization like
planScopeTypeLabel does.
- Around line 516-522: normalizeExecutionPlanScopeUserPath currently only
prepends '/' and must be updated to mirror backend user_path.go: collapse
repeated slashes and trailing slashes, reject path segments equal to '.' or '..'
or containing ':' and return '' for empty/invalid results, ensuring the final
canonical path starts with a single leading '/' and has no trailing slash;
update the helper function to perform these steps and ensure any other copies of
this logic in the file (the same helper used around lines 524-565) are
consolidated to use the canonicalizer, and also update
validateExecutionPlanRequest() to reject inputs containing '.' or '..' or ':'
segments (returning a validation error before POST) so client-side validation
matches server rules and avoids Create/Save flips and lost fallback state.
- Around line 271-279: executionPlanActiveScopeMatch is failing to treat a
path-only selection as scoped because hasScopedSelection ignores
scope_user_path; update executionPlanActiveScopeMatch (and the duplicate logic
around the same block used at lines 292-299) to consider a non-empty normalized
scope_user_path as a scoped selection: when computing hasScopedSelection or
deciding whether to match the active workflow, check
this.normalizeExecutionPlanScopeUserPath(form.scope_user_path) (or userPath
returned by executionPlanCurrentScope()) and treat a truthy value as indicating
a scoped selection so the active path workflow is recognized and preservation
logic runs.
In `@internal/executionplans/store_sqlite.go`:
- Around line 54-62: The current check-then-act using sqliteTableHasColumn
causes a race where concurrent initializers both attempt to add the column;
change to a race-safe pattern by attempting the ALTER TABLE unconditionally and
treating a "duplicate column" SQLITE error as non-fatal. Replace the existing
logic around sqliteTableHasColumn and the ALTER with an unconditional
db.Exec(`ALTER TABLE execution_plan_versions ADD COLUMN scope_user_path TEXT`)
and if it fails inspect the error (e.g., via an isSQLiteDuplicateColumnError
helper that checks for sqlite3.ErrError code or error text like "duplicate
column" / "already exists"); only return an error for other failures. Keep
references to the table name "execution_plan_versions", column
"scope_user_path", and the helper sqliteTableHasColumn (remove or deprecate its
use here) so reviewers can locate and update the code.
In `@internal/usage/stream_observer.go`:
- Around line 22-27: Normalization errors from core.NormalizeUserPath are
currently swallowed causing normalizedUserPath to remain empty and producing
unscoped usage rows; change the logic so that if
core.NormalizeUserPath(userPath[0]) returns an error you do not silently drop
the input — log or surface the error (using the observer's existing logger) and
set normalizedUserPath to the original userPath[0] (or otherwise mark it as
invalid but non-empty) so scoping is preserved; update the block that references
normalizedUserPath / userPath to reflect this behavior and include the error
details in the log message.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 17da60d8-7e45-450a-98b3-a0bdebc6e19e
📒 Files selected for processing (15)
docs/superpowers/plans/2026-03-30-user-path-tracking.mddocs/superpowers/specs/2026-03-30-user-path-design.mdinternal/admin/dashboard/static/js/modules/execution-plans.jsinternal/admin/dashboard/static/js/modules/execution-plans.test.jsinternal/admin/dashboard/templates/index.htmlinternal/core/execution_plan.gointernal/core/execution_plan_test.gointernal/core/request_snapshot_test.gointernal/core/user_path_test.gointernal/executionplans/service.gointernal/executionplans/service_test.gointernal/executionplans/store_sqlite.gointernal/executionplans/store_sqlite_test.gointernal/usage/stream_observer.gointernal/usage/stream_observer_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/executionplans/store_sqlite.go (1)
54-67: 🧹 Nitpick | 🔵 TrivialHarden duplicate-column detection to avoid message-fragile migration behavior.
This still relies on SQLite error text matching (
"duplicate column"/"already exists"). Prefer a deterministic schema check (PRAGMA table_info) before/afterALTER TABLEso migration behavior is driver/message-format independent.Suggested refactor
- if _, err := db.Exec(`ALTER TABLE execution_plan_versions ADD COLUMN scope_user_path TEXT`); err != nil && !isSQLiteDuplicateColumnError(err) { - return nil, fmt.Errorf("initialize execution plan versions table: %w", err) - } +hasScopeUserPath, err := sqliteTableHasColumn(db, "execution_plan_versions", "scope_user_path") +if err != nil { + return nil, fmt.Errorf("initialize execution plan versions table: %w", err) +} +if !hasScopeUserPath { + if _, err := db.Exec(`ALTER TABLE execution_plan_versions ADD COLUMN scope_user_path TEXT`); err != nil { + // Re-check to tolerate concurrent initializer races. + existsAfterErr, checkErr := sqliteTableHasColumn(db, "execution_plan_versions", "scope_user_path") + if checkErr != nil || !existsAfterErr { + if checkErr != nil { + return nil, fmt.Errorf("initialize execution plan versions table: %w", checkErr) + } + return nil, fmt.Errorf("initialize execution plan versions table: %w", err) + } + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/executionplans/store_sqlite.go` around lines 54 - 67, Replace fragile error-text matching with a deterministic schema check: before executing the ALTER in the initialization path that adds scope_user_path (the db.Exec call for "ALTER TABLE execution_plan_versions ADD COLUMN scope_user_path TEXT"), run a PRAGMA table_info('execution_plan_versions') query and scan for a column named "scope_user_path"; only call db.Exec if that column is not present. Remove or stop relying on isSQLiteDuplicateColumnError for flow control in this migration; keep the helper only for logging if desired but ensure the migration path uses the PRAGMA check to decide whether to alter the table and returns errors from db.Exec unchanged if the PRAGMA check indicates the column is missing and the ALTER fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/usage/stream_observer.go`:
- Around line 28-33: When core.NormalizeUserPath(userPath[0]) fails in the
stream usage observer, do not preserve the raw userPath[0]; instead set
normalizedUserPath to a canonical fallback such as "/" and avoid logging the raw
value. Update the failure branch that currently calls slog.Warn(...) to only log
the error reason (and contextual hint like "invalid user_path") without
including userPath[0], and set normalizedUserPath = "/"; keep references to
core.NormalizeUserPath, normalizedUserPath and slog.Warn to locate the change.
---
Duplicate comments:
In `@internal/executionplans/store_sqlite.go`:
- Around line 54-67: Replace fragile error-text matching with a deterministic
schema check: before executing the ALTER in the initialization path that adds
scope_user_path (the db.Exec call for "ALTER TABLE execution_plan_versions ADD
COLUMN scope_user_path TEXT"), run a PRAGMA
table_info('execution_plan_versions') query and scan for a column named
"scope_user_path"; only call db.Exec if that column is not present. Remove or
stop relying on isSQLiteDuplicateColumnError for flow control in this migration;
keep the helper only for logging if desired but ensure the migration path uses
the PRAGMA check to decide whether to alter the table and returns errors from
db.Exec unchanged if the PRAGMA check indicates the column is missing and the
ALTER fails.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 3ca3b6dc-00ed-4d78-8724-a31f761aab14
📒 Files selected for processing (6)
docs/superpowers/plans/2026-03-30-user-path-tracking.mdinternal/admin/dashboard/static/js/modules/execution-plans.jsinternal/admin/dashboard/static/js/modules/execution-plans.test.jsinternal/executionplans/store_sqlite.gointernal/usage/stream_observer.gointernal/usage/stream_observer_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/executionplans/store_sqlite.go`:
- Around line 57-61: The ALTER TABLE migration error returned when adding
scope_user_path is currently returned raw; update the error return in
NewSQLiteStore to wrap the error with context like the other returns (e.g., use
fmt.Errorf("initialize execution plan versions table: %w", err)) so failures
from the db.Exec(`ALTER TABLE execution_plan_versions ADD COLUMN scope_user_path
TEXT`) are consistent and informative; locate the block that checks
hasUserPathColumn and modify the return to wrap err accordingly.
In `@internal/usage/stream_observer_test.go`:
- Around line 151-215: Add a new unit test function
TestStreamUsageObserverNoUserPath that constructs a trackingLogger, calls
NewStreamUsageObserver without the optional userPath argument, feeds a minimal
JSON usage event via observer.OnJSONEvent and closes it with
observer.OnStreamClose, then asserts that logger.getEntries returns one entry
and that entries[0].UserPath equals the empty string ""; this documents expected
behavior for unscoped requests and uses the existing trackingLogger,
NewStreamUsageObserver, observer.OnJSONEvent, observer.OnStreamClose and
entries[0].UserPath symbols to locate where to add the test.
In `@internal/usage/stream_observer.go`:
- Around line 26-34: The code leaves normalizedUserPath as an empty string when
no userPath is provided which then gets stored into UsageEntry.UserPath, causing
inconsistent behavior vs the invalid-path fallback of "/"; change the logic in
the stream usage observer so that if len(userPath) == 0 you set
normalizedUserPath = "/" (i.e., treat absent userPath as root scope) instead of
leaving it empty, while keeping the existing core.NormalizeUserPath error branch
that logs and falls back to "/".
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9041dd7e-0bdc-468c-8a6b-bc9db6d3bd58
📒 Files selected for processing (4)
internal/executionplans/store_sqlite.gointernal/executionplans/store_sqlite_test.gointernal/usage/stream_observer.gointernal/usage/stream_observer_test.go
…r-tracking # Conflicts: # internal/auditlog/reader_postgresql.go # internal/auditlog/reader_sqlite.go # internal/auditlog/store_postgresql.go # internal/auditlog/store_postgresql_test.go # internal/auditlog/store_sqlite.go # tests/integration/dbassert/auditlog.go
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/dbassert/auditlog.go (1)
64-69:⚠️ Potential issue | 🟡 MinorPostgreSQL scan may fail on NULL
user_pathvalues.The scan binds
&entry.UserPathdirectly to astring, but theuser_pathcolumn in PostgreSQL is defined asTEXTwithoutNOT NULL. If existing rows haveNULLinuser_path(e.g., pre-migration data), the scan will fail with a conversion error.Consider using
sql.NullStringlikeauthKeyID:🛠️ Proposed fix
var entry AuditLogEntry var authKeyID sql.NullString + var userPath sql.NullString var dataJSON []byte err := rows.Scan( &entry.ID, &entry.Timestamp, &entry.DurationNs, &entry.Model, &entry.Provider, &entry.StatusCode, &entry.RequestID, &authKeyID, &entry.ClientIP, &entry.Method, - &entry.Path, &entry.UserPath, &entry.Stream, &entry.ErrorType, &dataJSON, + &entry.Path, &userPath, &entry.Stream, &entry.ErrorType, &dataJSON, ) require.NoError(t, err, "failed to scan audit log row") if authKeyID.Valid { entry.AuthKeyID = authKeyID.String } + if userPath.Valid { + entry.UserPath = userPath.String + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/dbassert/auditlog.go` around lines 64 - 69, The rows.Scan will fail when user_path is NULL because entry.UserPath is a plain string; change the scan to use sql.NullString (similar to authKeyID) by introducing a userPathNull (sql.NullString) variable in the scope where rows.Scan is called (alongside authKeyID and dataJSON), pass &userPathNull to rows.Scan instead of &entry.UserPath, then set entry.UserPath = userPathNull.String if userPathNull.Valid (or ""/nil as appropriate); alternatively change entry.UserPath to sql.NullString in the struct and adjust downstream code to handle that type.
♻️ Duplicate comments (1)
internal/executionplans/store_sqlite.go (1)
53-61:⚠️ Potential issue | 🟠 MajorMigration still has a TOCTOU race during concurrent initialization.
Line 57 does a check-then-act (
sqliteTableHasColumnthenALTER TABLE). Two initializers can both observe missing column and one fails onALTER TABLE, causing avoidable startup failure.Suggested race-safe adjustment
hasUserPathColumn, err := sqliteTableHasColumn(db, "execution_plan_versions", "scope_user_path") if err != nil { return nil, fmt.Errorf("initialize execution plan versions table: %w", err) } if !hasUserPathColumn { if _, err := db.Exec(`ALTER TABLE execution_plan_versions ADD COLUMN scope_user_path TEXT`); err != nil { - return nil, fmt.Errorf("initialize execution plan versions table: %w", err) + // Another initializer may have added it after our check. + existsAfterErr, checkErr := sqliteTableHasColumn(db, "execution_plan_versions", "scope_user_path") + if checkErr != nil { + return nil, fmt.Errorf("initialize execution plan versions table: %w", checkErr) + } + if !existsAfterErr { + return nil, fmt.Errorf("initialize execution plan versions table: %w", err) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/executionplans/store_sqlite.go` around lines 53 - 61, The check-then-act pattern using sqliteTableHasColumn followed by ALTER TABLE creates a TOCTOU race; instead, attempt the ALTER TABLE unconditionally and treat a "duplicate column" error as non-fatal. Replace the sqliteTableHasColumn + conditional db.Exec block with a single db.Exec(`ALTER TABLE execution_plan_versions ADD COLUMN scope_user_path TEXT`) and if Exec returns an error, inspect it: if it indicates the column already exists (e.g., error text like "duplicate column" or "already exists" for scope_user_path) ignore it, otherwise return the error. Keep references to sqliteTableHasColumn, execution_plan_versions, scope_user_path and the db.Exec call so the change is easy to locate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/admin/handler.go`:
- Around line 164-168: The validator helper normalizeUserPathQueryParam
currently hard-codes the query parameter name "user_path", causing validation
errors to reference the wrong field (e.g., scope_user_path); update the helper
and its callers (where normalizeUserPathQueryParam is invoked and
params.UserPath is set) to accept the actual field/key name (or return a
structured error that includes the caller-supplied field name) so the returned
error message references the real request field (e.g., scope_user_path) instead
of "user_path"; ensure callers pass the correct field name string and that the
error produced matches the existing validation semantics in the scope_user_path
validation (internal executionplans types) so all usages (the sites invoking
normalizeUserPathQueryParam and setting params.UserPath) display the correct
field name in their error messages.
In `@internal/auditlog/middleware.go`:
- Line 69: The audit middleware currently uses
core.UserPathFromContext(req.Context()) which can return an empty string when
RequestSnapshot is absent; update the middleware to normalize empty/missing
user-paths to "/" before constructing the audit record (i.e., call
core.UserPathFromContext(req.Context()), and if it returns "" set UserPath to
"/"), or alternatively change core.UserPathFromContext to return "/" by default
so all callers (including the audit middleware and stream_observer) get the same
fallback behavior; reference core.UserPathFromContext and the audit middleware's
UserPath assignment when making the change.
In `@internal/auditlog/reader_mongodb.go`:
- Around line 122-131: The validation error returned by
normalizeAuditUserPathFilter in reader_mongodb.go is being returned raw and gets
categorized as an internal error; instead, when normalizeAuditUserPathFilter
returns an error, wrap and return it as a core.GatewayError with the
invalid_request_error category (so callers/handler see it as a
validation/invalid-request issue). Locate the block using
normalizeAuditUserPathFilter (the userPath / matchFilters logic) and replace the
direct return of err with a wrapped core.GatewayError (preserving the original
error message/details) that sets the category to invalid_request_error before
returning.
In `@internal/auditlog/reader_sqlite.go`:
- Around line 54-57: When building the user_path filter in
internal/auditlog/reader_sqlite.go (the block that appends to conditions and
args using userPath and auditUserPathSubtreePattern), special-case the root
subtree string "/" so the SQL also includes legacy NULL user_path rows (e.g.
append "OR user_path IS NULL" to the condition when userPath == "/"); mirror the
same change in internal/auditlog/reader_postgresql.go so both readers include
NULL user_path rows when filtering the root subtree. Ensure args are only
appended for the non-NULL comparisons (userPath and
auditUserPathSubtreePattern(userPath)) and that the final condition contains the
OR user_path IS NULL clause for the "/" case.
In `@internal/executionplans/store_sqlite.go`:
- Around line 266-270: When constructing the Scope from DB columns (the
version.Scope assignment), normalize legacy NULL/empty user-paths by mapping
scopeUserPath.String (and the equivalent pg var) to "/" instead of "" so
hierarchical matching works; update the assignment that builds Scope (the
Provider/Model/UserPath fields created from scopeProvider.String,
scopeModel.String, scopeUserPath.String) to set UserPath = "/" when
scopeUserPath.String is empty or NULL before passing into
core.UserPathAncestors/plan matching; apply the same normalization in the
PostgreSQL store code path that reads scope_user_path.
In `@internal/usage/stream_observer_test.go`:
- Around line 173-193: Extend TestStreamUsageObserverNoUserPath by adding an
explicit-empty-string scenario: create a trackingLogger and call
NewStreamUsageObserver(logger, "gpt-4", "openai", "req-123",
"/v1/chat/completions", "") (instead of nil), emit the same OnJSONEvent and
OnStreamClose, then use logger.getEntries() and assert the single entry's
UserPath equals "/" to verify the observer falls back to root for an explicit
empty userPath; reference TestStreamUsageObserverNoUserPath,
NewStreamUsageObserver, OnJSONEvent, OnStreamClose, logger.getEntries, and the
UserPath field when adding the assertion.
---
Outside diff comments:
In `@tests/integration/dbassert/auditlog.go`:
- Around line 64-69: The rows.Scan will fail when user_path is NULL because
entry.UserPath is a plain string; change the scan to use sql.NullString (similar
to authKeyID) by introducing a userPathNull (sql.NullString) variable in the
scope where rows.Scan is called (alongside authKeyID and dataJSON), pass
&userPathNull to rows.Scan instead of &entry.UserPath, then set entry.UserPath =
userPathNull.String if userPathNull.Valid (or ""/nil as appropriate);
alternatively change entry.UserPath to sql.NullString in the struct and adjust
downstream code to handle that type.
---
Duplicate comments:
In `@internal/executionplans/store_sqlite.go`:
- Around line 53-61: The check-then-act pattern using sqliteTableHasColumn
followed by ALTER TABLE creates a TOCTOU race; instead, attempt the ALTER TABLE
unconditionally and treat a "duplicate column" error as non-fatal. Replace the
sqliteTableHasColumn + conditional db.Exec block with a single db.Exec(`ALTER
TABLE execution_plan_versions ADD COLUMN scope_user_path TEXT`) and if Exec
returns an error, inspect it: if it indicates the column already exists (e.g.,
error text like "duplicate column" or "already exists" for scope_user_path)
ignore it, otherwise return the error. Keep references to sqliteTableHasColumn,
execution_plan_versions, scope_user_path and the db.Exec call so the change is
easy to locate.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 078d81d9-fcb3-4196-afcc-72ad113bc174
📒 Files selected for processing (15)
internal/admin/dashboard/templates/index.htmlinternal/admin/handler.gointernal/auditlog/auditlog.gointernal/auditlog/middleware.gointernal/auditlog/reader_mongodb.gointernal/auditlog/reader_postgresql.gointernal/auditlog/reader_sqlite.gointernal/auditlog/store_mongodb.gointernal/auditlog/store_postgresql.gointernal/auditlog/store_postgresql_test.gointernal/auditlog/store_sqlite.gointernal/executionplans/store_sqlite.gointernal/usage/stream_observer.gointernal/usage/stream_observer_test.gotests/integration/dbassert/auditlog.go
| if userPath != "" { | ||
| conditions = append(conditions, "(user_path = ? OR user_path LIKE ? ESCAPE '\\')") | ||
| args = append(args, userPath, auditUserPathSubtreePattern(userPath)) | ||
| } |
There was a problem hiding this comment.
Include NULL user_path rows when filtering the root subtree.
Line 54 now treats / as the full subtree, but this reader still accepts NULL user_path values below. On upgraded databases those legacy rows will not match (user_path = ? OR user_path LIKE ? ...), so filtering by / hides part of the audit history. Please special-case / to include NULLs here, and mirror the same fix in internal/auditlog/reader_postgresql.go.
🛠️ Suggested fix
- if userPath != "" {
- conditions = append(conditions, "(user_path = ? OR user_path LIKE ? ESCAPE '\\')")
- args = append(args, userPath, auditUserPathSubtreePattern(userPath))
- }
+ if userPath != "" {
+ if userPath == "/" {
+ conditions = append(conditions, "(user_path IS NULL OR user_path = ? OR user_path LIKE ? ESCAPE '\\')")
+ } else {
+ conditions = append(conditions, "(user_path = ? OR user_path LIKE ? ESCAPE '\\')")
+ }
+ args = append(args, userPath, auditUserPathSubtreePattern(userPath))
+ }Also applies to: 110-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auditlog/reader_sqlite.go` around lines 54 - 57, When building the
user_path filter in internal/auditlog/reader_sqlite.go (the block that appends
to conditions and args using userPath and auditUserPathSubtreePattern),
special-case the root subtree string "/" so the SQL also includes legacy NULL
user_path rows (e.g. append "OR user_path IS NULL" to the condition when
userPath == "/"); mirror the same change in
internal/auditlog/reader_postgresql.go so both readers include NULL user_path
rows when filtering the root subtree. Ensure args are only appended for the
non-NULL comparisons (userPath and auditUserPathSubtreePattern(userPath)) and
that the final condition contains the OR user_path IS NULL clause for the "/"
case.
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 (1)
internal/auditlog/store_sqlite.go (1)
154-179:⚠️ Potential issue | 🟠 MajorDon't persist blank
user_pathvalues.
WriteBatchbindse.UserPathverbatim. Any direct caller that leaves it empty will store'', but the new root-subtree filters only treat/(and legacyNULL) as root, so those rows disappear from root-scoped queries. Canonicalize blank values to/at the storage boundary here, and mirror the same fix ininternal/auditlog/store_postgresql.go.🛠️ Suggested change
var cacheTypeValue any if cacheType := normalizeCacheType(e.CacheType); cacheType != "" { cacheTypeValue = cacheType } + userPath := strings.TrimSpace(e.UserPath) + if userPath == "" { + userPath = "/" + } values = append(values, e.ID, e.Timestamp.UTC().Format(time.RFC3339Nano), e.DurationNs, @@ e.AuthKeyID, e.ClientIP, e.Method, e.Path, - e.UserPath, + userPath, streamInt, e.ErrorType, dataValue, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auditlog/store_sqlite.go` around lines 154 - 179, The WriteBatch code is currently appending e.UserPath verbatim which allows empty strings to be persisted and break root-scoped queries; update the storage boundary in the WriteBatch logic to canonicalize blank or empty e.UserPath to "/" before appending values (i.e., replace e.UserPath with a computed userPathValue = "/" if e.UserPath == "" else e.UserPath), apply this same canonicalization in the analogous function in internal/auditlog/store_postgresql.go, and ensure references like e.UserPath, WriteBatch (or the method building values), and the Postgres store function are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/admin/dashboard/templates/index.html`:
- Around line 705-712: The placeholder and hint around the User Path input
(label with class "alias-form-field", input with classes "filter-input
execution-plan-input" and x-model "executionPlanForm.scope_user_path") imply a
leading slash is required but the backend normalizes paths; update the
placeholder to show both variants (e.g., "team/alpha or /team/alpha") and tweak
the adjacent hint paragraph that begins "Add a path to scope the workflow..." to
explicitly state that leading slashes are optional and will be normalized (e.g.,
"Leading slashes are optional and will be normalized; you can enter 'team/alpha'
or '/team/alpha'").
In `@internal/auditlog/reader_mongodb.go`:
- Around line 124-133: The MongoDB reader only appends a $regex filter from
auditUserPathSubtreeRegex(userPath) which excludes legacy documents with missing
or null user_path; update the block that handles normalized userPath (the code
using normalizeAuditUserPathFilter, auditUserPathSubtreeRegex and appending to
matchFilters) so that when userPath represents the root subtree ("/" or whatever
normalize returns for root) you append an $or condition that matches either the
regex OR documents where user_path is null or does not exist (use bson.D{{Key:
"$or", Value: bson.A{<regex condition>, bson.D{{Key: "user_path", Value:
bson.D{{Key: "$exists", Value: false}}}}, bson.D{{Key: "user_path", Value:
nil}}}}}) instead of the single regex entry; for non-root userPath keep the
existing regex-only match.
In `@internal/auditlog/store_sqlite_test.go`:
- Around line 406-415: The test currently only asserts that reader.GetLogs with
LogQueryParams{UserPath: "/team", Limit: 10} returned one entry and that its ID
is "match-team", but it doesn't verify the scanned UserPath field; update the
assertion to also check that logs.Entries[0].UserPath equals "/team" (or the
expected stored value) so the test fails if the SQLite reader stops
selecting/scanning the user_path column.
In `@internal/auditlog/user_path_filter_test.go`:
- Around line 5-57: Add unit tests to cover LIKE-wildcard escaping and the
MongoDB regex helper: extend TestAuditUserPathSubtreePattern with a case using a
path containing SQL LIKE wildcards (e.g., "/team%a" or "/team_a") and assert the
expected escaped subtree pattern produced by auditUserPathSubtreePattern
(verifying escapeLikeWildcards behavior, e.g., percent/underscore are escaped
before appending "/%"); and add a new TestAuditUserPathSubtreeRegex that calls
auditUserPathSubtreeRegex with representative inputs (root and a
wildcard-containing path) and asserts the returned regex string or pattern
matches the expected escaped/anchored form. Ensure you reference
auditUserPathSubtreePattern, escapeLikeWildcards, and auditUserPathSubtreeRegex
in the tests so failures point to the right helpers.
In `@internal/executionplans/store_sqlite.go`:
- Around line 61-86: The helper function sqliteTableHasColumn is dead code and
should be removed; delete the entire sqliteTableHasColumn function (including
its signature and body) from the file to avoid unused function clutter and
imports, and then run go build/test to ensure no remaining references to
sqliteTableHasColumn exist and to remove any now-unused imports (e.g.,
database/sql or fmt) introduced only for that helper.
---
Outside diff comments:
In `@internal/auditlog/store_sqlite.go`:
- Around line 154-179: The WriteBatch code is currently appending e.UserPath
verbatim which allows empty strings to be persisted and break root-scoped
queries; update the storage boundary in the WriteBatch logic to canonicalize
blank or empty e.UserPath to "/" before appending values (i.e., replace
e.UserPath with a computed userPathValue = "/" if e.UserPath == "" else
e.UserPath), apply this same canonicalization in the analogous function in
internal/auditlog/store_postgresql.go, and ensure references like e.UserPath,
WriteBatch (or the method building values), and the Postgres store function are
updated accordingly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 02935201-f8c5-4621-bae2-a11e82cae366
📒 Files selected for processing (25)
internal/admin/dashboard/templates/index.htmlinternal/admin/handler.gointernal/admin/handler_executionplans_test.gointernal/admin/handler_test.gointernal/auditlog/auditlog.gointernal/auditlog/auditlog_test.gointernal/auditlog/middleware.gointernal/auditlog/reader_mongodb.gointernal/auditlog/reader_mongodb_test.gointernal/auditlog/reader_postgresql.gointernal/auditlog/reader_sqlite.gointernal/auditlog/store_mongodb.gointernal/auditlog/store_postgresql.gointernal/auditlog/store_postgresql_test.gointernal/auditlog/store_sqlite.gointernal/auditlog/store_sqlite_test.gointernal/auditlog/user_path_filter.gointernal/auditlog/user_path_filter_test.gointernal/executionplans/store_helpers.gointernal/executionplans/store_helpers_test.gointernal/executionplans/store_postgresql.gointernal/executionplans/store_sqlite.gointernal/usage/stream_observer.gointernal/usage/stream_observer_test.gotests/integration/dbassert/auditlog.go
Summary
Testing
Summary by CodeRabbit
New Features
Execution Plans
Admin UI
Persistence
Documentation
Tests