Skip to content

feat(tracking): add hierarchical user path scoping#189

Merged
SantiagoDePolonia merged 10 commits intomainfrom
feature/usage-per-user-tracking
Mar 31, 2026
Merged

feat(tracking): add hierarchical user path scoping#189
SantiagoDePolonia merged 10 commits intomainfrom
feature/usage-per-user-tracking

Conversation

@SantiagoDePolonia
Copy link
Copy Markdown
Contributor

@SantiagoDePolonia SantiagoDePolonia commented Mar 30, 2026

Summary

  • add hierarchical user path scoping across request tracking, execution plans, usage, and audit logs
  • fix the review regressions by stripping the internal user-path header from upstream passthrough requests, treating / as the full subtree in SQL filters, and allowing slashless scope paths in the dashboard
  • merge the latest origin/main into this branch

Testing

  • go test ./internal/server ./internal/usage ./internal/auditlog
  • node --test internal/admin/dashboard/static/js/modules/execution-plans.test.js

Summary by CodeRabbit

  • New Features

    • Introduced canonical User Path header handling: normalize at ingress, expose in request snapshot, and propagate to logs/usage.
  • Execution Plans

    • Add scope_user_path support; matching is most-specific-first with deepest-to-root fallback; scope types updated.
  • Admin UI

    • Usage/Audit views gain User Path filter/column; Execution Plan editor accepts path-scoped scopes with validation.
  • Persistence

    • Persist user_path on audit_logs, usage, batches, and execution-plan versions; DB indexes/migrations updated.
  • Documentation

    • Added design spec and stepwise rollout plan.
  • Tests

    • Expanded unit/integration tests for normalization, matching, persistence, and UI behavior.

…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
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 30, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • ✅ Review completed - (🔄 Check again to review again)
📝 Walkthrough

Walkthrough

Adds 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

Cohort / File(s) Summary
Design & Docs
docs/superpowers/plans/2026-03-30-user-path-tracking.md, docs/superpowers/specs/2026-03-30-user-path-design.md
New implementation plan and spec: normalization rules, scope semantics (scope_user_path), persistence and subtree-filter semantics, and test matrix.
Core utilities & types
internal/core/user_path.go, internal/core/user_path_test.go, internal/core/execution_plan.go, internal/core/execution_plan_test.go, internal/core/request_snapshot.go, internal/core/request_snapshot_test.go
Add NormalizeUserPath, UserPathAncestors, UserPathFromContext; add UserPath fields to RequestSnapshot/ExecutionPlanSelector/ResolvedExecutionPolicy; tests added.
Request plumbing & middleware
internal/server/request_snapshot.go, internal/server/request_snapshot_test.go, internal/server/handlers_test.go, internal/server/model_validation.go, internal/server/execution_plan_helpers.go
Read/normalize header at ingress, validate and update header, include normalized user path in snapshots/context and ExecutionPlanSelector construction; tests updated to assert normalization and non-forwarding.
Execution plans logic & storage
internal/executionplans/types.go, internal/executionplans/service.go, internal/executionplans/compiler.go, internal/executionplans/store_sqlite.go, internal/executionplans/store_postgresql.go, internal/executionplans/store_mongodb.go, internal/executionplans/store_sqlite_test.go, internal/executionplans/*.test.go
Scope gains UserPath; scope normalization/validation; scopeKey extended with path, provider_path, provider_model_path; in-memory snapshot maps and matching extended to use UserPathAncestors for most-specific fallback; persistence and schema updated across backends; tests added.
Admin handlers & API
internal/admin/handler.go, internal/admin/handler_executionplans_test.go, internal/admin/handler_test.go
Parse/normalize user_path query and scope_user_path JSON, return validation errors, and store normalized scope; tests adjusted.
Admin dashboard UI
internal/admin/dashboard/static/js/dashboard.js, internal/admin/dashboard/static/js/modules/execution-plans.js, internal/admin/dashboard/static/js/modules/execution-plans.test.js, internal/admin/dashboard/static/js/modules/audit-list.js, internal/admin/dashboard/static/js/modules/usage.js, internal/admin/dashboard/templates/index.html
Add usageLogUserPath/auditUserPath state and inputs; integrate scope_user_path into execution-plan forms with normalization/validation; include user_path in API requests and UI tables; JS tests updated.
Audit log core, readers & stores
internal/auditlog/auditlog.go, internal/auditlog/middleware.go, internal/auditlog/reader*.go, internal/auditlog/store_*.go, internal/auditlog/user_path_filter.go, internal/auditlog/*.test.go
Add LogEntry.UserPath; populate from context in middleware; readers apply normalized subtree filtering (regex/LIKE) and include user_path in selects/scans; stores add column/index and migration updates; tests added.
Usage tracking core, readers & stores
internal/usage/usage.go, internal/usage/reader*.go, internal/usage/store_*.go, internal/usage/user_path_filter.go, internal/usage/*.test.go
Add UsageEntry.UserPath and UsageQueryParams.UserPath; readers support normalization and subtree filtering; stores persist user_path and add indexes; streaming and batch consumers updated; tests added.
Streaming & batch flows
internal/usage/stream_observer.go, internal/usage/stream_observer_test.go, internal/batch/store.go, internal/batch/store_test.go, internal/server/native_batch_*.go, internal/server/native_batch_support_test.go
StreamUsageObserver accepts/normalizes userPath and stamps entries; StoredBatch includes UserPath and flows through persistence; batch usage logs use stored UserPath; tests added.
Translated inference & usage logging
internal/server/translated_inference_service.go, internal/server/translated_inference_service_test.go
logUsage becomes context-aware, sets UsageEntry.UserPath from context; streaming observer instantiation receives user-path; tests updated.
Integration test helpers
tests/integration/dbassert/usage.go, tests/integration/dbassert/auditlog.go
Test helper structs extended with UserPath and DB/Mongo query helpers updated to select/scan user_path for assertions.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐰 I trim every slash and tidy each trace,

I hop from the leaf up the path to the base,
I thread through snapshots, plans, DB and UI,
In logs and in usage my rabbit-steps fly,
Canonical paths neat — hop, normalize, race!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.75% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(tracking): add hierarchical user path scoping' accurately and concisely describes the primary change—introducing user path scoping with hierarchical fallback semantics across the service.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/usage-per-user-tracking

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🔵 Trivial

Add an explicit UserPath assertion for the owned-body constructor test.

Line 88 passes "/team/a" into NewRequestSnapshotWithOwnedBody, 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 | 🔴 Critical

Don't treat path-only scopes as global during deactivation.

Path-only plans now legitimately have Provider == "" and Model == "", 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 | 🟡 Minor

Keep view specificity aligned with runtime precedence.

matchCompiled and the design spec both prefer plain path over plain provider, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 64320e7 and 3e5710c.

📒 Files selected for processing (67)
  • docs/superpowers/plans/2026-03-30-user-path-tracking.md
  • docs/superpowers/specs/2026-03-30-user-path-design.md
  • internal/admin/dashboard/static/js/dashboard.js
  • internal/admin/dashboard/static/js/modules/audit-list.js
  • internal/admin/dashboard/static/js/modules/execution-plans.js
  • internal/admin/dashboard/static/js/modules/execution-plans.test.js
  • internal/admin/dashboard/static/js/modules/usage.js
  • internal/admin/dashboard/templates/index.html
  • internal/admin/handler.go
  • internal/admin/handler_executionplans_test.go
  • internal/admin/handler_test.go
  • internal/auditlog/auditlog.go
  • internal/auditlog/middleware.go
  • internal/auditlog/reader.go
  • internal/auditlog/reader_mongodb.go
  • internal/auditlog/reader_postgresql.go
  • internal/auditlog/reader_sqlite.go
  • internal/auditlog/store_mongodb.go
  • internal/auditlog/store_postgresql.go
  • internal/auditlog/store_postgresql_test.go
  • internal/auditlog/store_sqlite.go
  • internal/auditlog/store_sqlite_test.go
  • internal/auditlog/user_path_filter.go
  • internal/auditlog/user_path_filter_test.go
  • internal/batch/store.go
  • internal/batch/store_test.go
  • internal/core/execution_plan.go
  • internal/core/request_snapshot.go
  • internal/core/request_snapshot_test.go
  • internal/core/user_path.go
  • internal/core/user_path_test.go
  • internal/executionplans/compiler.go
  • internal/executionplans/service.go
  • internal/executionplans/service_test.go
  • internal/executionplans/store_mongodb.go
  • internal/executionplans/store_postgresql.go
  • internal/executionplans/store_sqlite.go
  • internal/executionplans/types.go
  • internal/executionplans/types_test.go
  • internal/server/execution_plan_helpers.go
  • internal/server/handlers_test.go
  • internal/server/model_validation.go
  • internal/server/model_validation_test.go
  • internal/server/native_batch_service.go
  • internal/server/native_batch_support.go
  • internal/server/native_batch_support_test.go
  • internal/server/passthrough_support.go
  • internal/server/request_snapshot.go
  • internal/server/request_snapshot_test.go
  • internal/server/translated_inference_service.go
  • internal/server/translated_inference_service_test.go
  • internal/usage/reader.go
  • internal/usage/reader_mongodb.go
  • internal/usage/reader_postgresql.go
  • internal/usage/reader_sqlite.go
  • internal/usage/reader_sqlite_boundary_test.go
  • internal/usage/store_mongodb.go
  • internal/usage/store_postgresql.go
  • internal/usage/store_postgresql_test.go
  • internal/usage/store_sqlite.go
  • internal/usage/stream_observer.go
  • internal/usage/stream_observer_test.go
  • internal/usage/usage.go
  • internal/usage/user_path_filter.go
  • internal/usage/user_path_filter_test.go
  • tests/integration/dbassert/auditlog.go
  • tests/integration/dbassert/usage.go

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 3e5710c and eb12f4c.

📒 Files selected for processing (15)
  • docs/superpowers/plans/2026-03-30-user-path-tracking.md
  • docs/superpowers/specs/2026-03-30-user-path-design.md
  • internal/admin/dashboard/static/js/modules/execution-plans.js
  • internal/admin/dashboard/static/js/modules/execution-plans.test.js
  • internal/admin/dashboard/templates/index.html
  • internal/core/execution_plan.go
  • internal/core/execution_plan_test.go
  • internal/core/request_snapshot_test.go
  • internal/core/user_path_test.go
  • internal/executionplans/service.go
  • internal/executionplans/service_test.go
  • internal/executionplans/store_sqlite.go
  • internal/executionplans/store_sqlite_test.go
  • internal/usage/stream_observer.go
  • internal/usage/stream_observer_test.go

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
internal/executionplans/store_sqlite.go (1)

54-67: 🧹 Nitpick | 🔵 Trivial

Harden 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/after ALTER TABLE so 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

📥 Commits

Reviewing files that changed from the base of the PR and between eb12f4c and 7d3979c.

📒 Files selected for processing (6)
  • docs/superpowers/plans/2026-03-30-user-path-tracking.md
  • internal/admin/dashboard/static/js/modules/execution-plans.js
  • internal/admin/dashboard/static/js/modules/execution-plans.test.js
  • internal/executionplans/store_sqlite.go
  • internal/usage/stream_observer.go
  • internal/usage/stream_observer_test.go

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 7d3979c and 23749f6.

📒 Files selected for processing (4)
  • internal/executionplans/store_sqlite.go
  • internal/executionplans/store_sqlite_test.go
  • internal/usage/stream_observer.go
  • internal/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
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟡 Minor

PostgreSQL scan may fail on NULL user_path values.

The scan binds &entry.UserPath directly to a string, but the user_path column in PostgreSQL is defined as TEXT without NOT NULL. If existing rows have NULL in user_path (e.g., pre-migration data), the scan will fail with a conversion error.

Consider using sql.NullString like authKeyID:

🛠️ 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 | 🟠 Major

Migration still has a TOCTOU race during concurrent initialization.

Line 57 does a check-then-act (sqliteTableHasColumn then ALTER TABLE). Two initializers can both observe missing column and one fails on ALTER 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

📥 Commits

Reviewing files that changed from the base of the PR and between 23749f6 and a8a46f8.

📒 Files selected for processing (15)
  • internal/admin/dashboard/templates/index.html
  • internal/admin/handler.go
  • internal/auditlog/auditlog.go
  • internal/auditlog/middleware.go
  • internal/auditlog/reader_mongodb.go
  • internal/auditlog/reader_postgresql.go
  • internal/auditlog/reader_sqlite.go
  • internal/auditlog/store_mongodb.go
  • internal/auditlog/store_postgresql.go
  • internal/auditlog/store_postgresql_test.go
  • internal/auditlog/store_sqlite.go
  • internal/executionplans/store_sqlite.go
  • internal/usage/stream_observer.go
  • internal/usage/stream_observer_test.go
  • tests/integration/dbassert/auditlog.go

Comment on lines +54 to +57
if userPath != "" {
conditions = append(conditions, "(user_path = ? OR user_path LIKE ? ESCAPE '\\')")
args = append(args, userPath, auditUserPathSubtreePattern(userPath))
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Don't persist blank user_path values.

WriteBatch binds e.UserPath verbatim. Any direct caller that leaves it empty will store '', but the new root-subtree filters only treat / (and legacy NULL) as root, so those rows disappear from root-scoped queries. Canonicalize blank values to / at the storage boundary here, and mirror the same fix in internal/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

📥 Commits

Reviewing files that changed from the base of the PR and between 23749f6 and f5ab9ff.

📒 Files selected for processing (25)
  • internal/admin/dashboard/templates/index.html
  • internal/admin/handler.go
  • internal/admin/handler_executionplans_test.go
  • internal/admin/handler_test.go
  • internal/auditlog/auditlog.go
  • internal/auditlog/auditlog_test.go
  • internal/auditlog/middleware.go
  • internal/auditlog/reader_mongodb.go
  • internal/auditlog/reader_mongodb_test.go
  • internal/auditlog/reader_postgresql.go
  • internal/auditlog/reader_sqlite.go
  • internal/auditlog/store_mongodb.go
  • internal/auditlog/store_postgresql.go
  • internal/auditlog/store_postgresql_test.go
  • internal/auditlog/store_sqlite.go
  • internal/auditlog/store_sqlite_test.go
  • internal/auditlog/user_path_filter.go
  • internal/auditlog/user_path_filter_test.go
  • internal/executionplans/store_helpers.go
  • internal/executionplans/store_helpers_test.go
  • internal/executionplans/store_postgresql.go
  • internal/executionplans/store_sqlite.go
  • internal/usage/stream_observer.go
  • internal/usage/stream_observer_test.go
  • tests/integration/dbassert/auditlog.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant