feat: added labelling system#454
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds header-based request tagging across configuration, a new tagging service and persistence layer, admin/server wiring, and label propagation into usage and audit records. It also extends usage storage/read paths to persist and return labels, and updates dashboard UI and docs for tagging rules. ChangesRequest Tagging Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.template:
- Around line 8-19: The tagging env documentation in the .env.template comment
is misleading because applyTaggingEnv() and mergeByKey() replace the entire
TaggingHeaderConfig for a matching header name rather than merging individual
fields. Update the wording around TAGGING_HEADER_* entries to make clear that an
env match overrides the whole header entry from config.yaml, and that unset
_PREFIX, _DELIMITER, or _DONOTPASS values will clear the YAML values instead of
inheriting them.
In `@config/tagging_test.go`:
- Around line 9-98: The tagging tests are written as several nearly identical
one-off cases instead of a table-driven suite. Refactor the checks in
TestApplyTaggingEnv_ParsesAndMerges,
TestApplyTaggingEnv_SortsByIndexAndSkipsGaps, TestApplyTaggingEnv_Unset,
TestNormalizeTaggingConfig, and
TestNormalizeTaggingConfig_RejectsInvalidAndDuplicate into table-driven subtests
so new header options are easier to add and the harness is shared. Keep the same
coverage and assertions, but drive each case from a slice of inputs/expected
results.
In `@config/tagging.go`:
- Around line 83-101: Reject secret-bearing headers during
normalizeTaggingConfig before they are accepted into TaggingConfig.Headers. Add
a small denylist check in normalizeTaggingConfig, using the canonicalized header
name returned by NormalizeHeaderName, and return an error for names like
Authorization, Proxy-Authorization, Cookie, and Set-Cookie (and any equivalent
credential-bearing variants you already treat as sensitive). Keep the existing
duplicate and delimiter handling in place, but fail fast as soon as a denied
header is encountered.
In `@internal/admin/dashboard/static/js/dashboard.js`:
- Around line 257-259: The tagging/settings refresh is only triggered from
_applyRoute(), so the new settings panel can stay stale when submitApiKey() or
refreshDashboardDataAfterRuntimeRefresh() run on /settings. Extract the existing
fetchTaggingSettings() call into a shared helper on the dashboard object and
invoke that helper from _applyRoute(), submitApiKey(), and
refreshDashboardDataAfterRuntimeRefresh() so settings are reloaded regardless of
route changes.
In `@internal/admin/dashboard/templates/page-settings.html`:
- Around line 226-233: The “Add Header” action is missing the loading guard, so
users can add rows while `fetchTaggingSettings` is still populating
`taggingHeaders`. Update the `Add Header` button in `page-settings.html` to also
disable during `taggingLoading` (consistent with `Save Tagging Settings`), and
make sure `addTaggingHeader()` cannot run until loading is finished. Use the
existing `taggingLoading`, `taggingEditable`, and `taggingSaving` state checks
to keep the UI behavior consistent.
In `@internal/admin/handler_tagging.go`:
- Around line 85-95: The error classification in isTaggingValidationError is
brittle because it depends on exact error text from internal/tagging. Expose
typed or sentinel errors there (for example ErrManagedHeader,
ErrInvalidHeaderName, ErrDuplicateHeader) and update isTaggingValidationError in
handler_tagging.go to use errors.Is or errors.As instead of strings.Contains.
Keep the caller/storage split behavior the same, but make the handler depend on
stable error types rather than message wording.
- Around line 38-83: Direct coverage is missing for the new tagging handler
methods in Handler, so add tests for TaggingSettings and UpdateTaggingSettings
that exercise the nil h.tagging path, c.Bind failure, SaveRules validation
errors, SaveRules storage/runtime errors, and the successful response shape. Use
the existing handler methods and tagging dependency methods (Rules, Editable,
SaveRules) to verify both returned errors and the JSON payload contents.
In `@internal/admin/routes.go`:
- Around line 42-47: The tagging routes are interleaved in the middle of the
budgets route group, making the route setup harder to scan. In
internal/admin/routes.go, reorder the router registrations in the block
containing g.PUT("/budgets/settings"), h.TaggingSettings,
h.UpdateTaggingSettings, h.ResetBudget, and h.ResetBudgets so the two tagging
routes are moved together either before or after the complete budgets group,
keeping all budgets routes contiguous.
In `@internal/app/app.go`:
- Line 53: App.Shutdown is missing teardown for the new tagging resource, so add
a Close() call for a.tagging alongside the other managed subsystems. Update the
shutdown path in App to include the tagging.Result field in the same cleanup
sequence as the existing resources, using the App.Shutdown method and a.tagging
identifier to ensure the tagging store/service is released on shutdown.
In `@internal/auditlog/auditlog.go`:
- Around line 113-115: `DoNotPass` header values are still being captured into
persisted `Labels`, which leaks sensitive data beyond forwarding. Update the
label extraction path around `ExtractLabels` so headers marked `DoNotPass: true`
are excluded from the returned labels (or only included behind an explicit
separate opt-in), while keeping the strip-set behavior unchanged. Make sure the
fix flows through `LogData.Labels`, `CreateStreamEntry` in `stream_wrapper.go`,
and `UsageEntry.Labels` in `stream_observer.go` so sensitive header values no
longer reach audit logs, usage records, or dashboards.
In `@internal/server/tagging.go`:
- Around line 23-28: The request-label extraction in DoNotPass still allows
secret-bearing headers to be captured, so `service.ExtractLabels` and
`core.WithRequestLabels` need a denylist/redaction step. Update the tagging flow
in the request-handling path to filter or mask sensitive headers like
`Authorization`, `Cookie`, and API-key headers before labels are attached to the
context, while preserving non-sensitive labels and existing
`service.StripHeaders` behavior.
In `@internal/tagging/service.go`:
- Around line 112-134: SaveRules can return a stale merged view when concurrent
callers interleave between validation, store.SaveRules, and s.Refresh. Serialize
the full validate→persist→refresh flow in Service.SaveRules by holding the
service lock across the whole critical section, and avoid deadlock by using a
lock-free refresh path (for example, a refreshLocked helper) instead of calling
Refresh while already holding the mutex. Ensure the returned s.Rules() always
reflects the same save operation that was just persisted.
In `@internal/tagging/store_postgresql.go`:
- Around line 47-55: Add optimistic locking to the ruleset save path:
`PostgreSQLStore.SaveRules` currently unconditionally upserts the singleton row,
so thread an expected revision/etag from the admin PUT flow into the write API
and use it as a precondition before updating `tagging_settings`. Update
`SaveRules` (and the corresponding save methods in the other two stores) to
compare against the stored revision/`updated_at` value and return a conflict
when the caller’s expected value is stale, instead of silently overwriting the
latest rules.
In `@tests/perf/hotpath_test.go`:
- Around line 399-400: The allocation ceiling in the benchmark is being relaxed
without exercising the labeled request path. Update the benchmark setup in
hotpath_test.go so the case covering usage.NewStreamUsageObserver and the
LogData construction uses non-nil labels/tagged request data, then re-run the
benchmark for that labeled path before keeping the new maxAllocs/maxBytes
thresholds. Use the existing benchmark helpers around
usage.NewStreamUsageObserver and auditlog.LogData to ensure the regression guard
measures the actual tagged-request flow introduced by this PR.
🪄 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: aa423c86-7695-4fca-a26e-eed85f429b57
📒 Files selected for processing (54)
.env.templateCLAUDE.mdconfig/config.example.yamlconfig/config.goconfig/config_test.goconfig/merge.goconfig/tagging.goconfig/tagging_test.goconfig/virtualmodels.gointernal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/tagging.jsinternal/admin/dashboard/templates/layout.htmlinternal/admin/dashboard/templates/page-settings.htmlinternal/admin/handler.gointernal/admin/handler_tagging.gointernal/admin/routes.gointernal/admin/routes_test.gointernal/app/app.gointernal/auditlog/auditlog.gointernal/auditlog/middleware.gointernal/auditlog/stream_wrapper.gointernal/core/context.gointernal/core/labels.gointernal/gateway/usage.gointernal/live/broker.gointernal/responsecache/usage_hit.gointernal/server/audio_service.gointernal/server/http.gointernal/server/internal_chat_completion_executor.gointernal/server/passthrough_support.gointernal/server/realtime_service.gointernal/server/tagging.gointernal/server/tagging_test.gointernal/server/translated_inference_service.gointernal/tagging/factory.gointernal/tagging/service.gointernal/tagging/store.gointernal/tagging/store_mongodb.gointernal/tagging/store_postgresql.gointernal/tagging/store_sqlite.gointernal/tagging/tagging.gointernal/tagging/tagging_test.gointernal/usage/labels_sqlite_test.gointernal/usage/reader.gointernal/usage/reader_mongodb.gointernal/usage/reader_postgresql.gointernal/usage/reader_sqlite.gointernal/usage/store_postgresql.gointernal/usage/store_postgresql_test.gointernal/usage/store_sqlite.gointernal/usage/stream_observer.gointernal/usage/usage.gotests/perf/hotpath_test.go
Comment on lines
+9
to
+98
| func TestApplyTaggingEnv_ParsesAndMerges(t *testing.T) { | ||
| cfg := &Config{Tagging: TaggingConfig{Headers: []TaggingHeaderConfig{ | ||
| {Header: "X-Team", Prefix: "team-"}, | ||
| {Header: "X-Keep"}, | ||
| }}} | ||
| t.Setenv("TAGGING_HEADER_1", "X-Team") | ||
| t.Setenv("TAGGING_HEADER_1_DONOTPASS", "true") | ||
| t.Setenv("TAGGING_HEADER_2", "X-Cost-Center") | ||
| t.Setenv("TAGGING_HEADER_2_PREFIX", "cc-") | ||
| t.Setenv("TAGGING_HEADER_2_DELIMITER", ";") | ||
|
|
||
| if err := applyTaggingEnv(cfg); err != nil { | ||
| t.Fatalf("applyTaggingEnv() error = %v", err) | ||
| } | ||
| headers := cfg.Tagging.Headers | ||
| if len(headers) != 3 { | ||
| t.Fatalf("merged len = %d, want 3: %#v", len(headers), headers) | ||
| } | ||
| // "X-Team" is overridden in place (env wins) and keeps its position. | ||
| team := headers[0] | ||
| if team.Header != "X-Team" || !team.DoNotPass || team.Prefix != "" { | ||
| t.Fatalf("env did not override X-Team: %#v", team) | ||
| } | ||
| // "X-Keep" is untouched; "X-Cost-Center" is appended. | ||
| if headers[1].Header != "X-Keep" { | ||
| t.Fatalf("merge order wrong: %#v", headers) | ||
| } | ||
| cc := headers[2] | ||
| if cc.Header != "X-Cost-Center" || cc.Prefix != "cc-" || cc.Delimiter != ";" || cc.DoNotPass { | ||
| t.Fatalf("env entry wrong: %#v", cc) | ||
| } | ||
| } | ||
|
|
||
| func TestApplyTaggingEnv_SortsByIndexAndSkipsGaps(t *testing.T) { | ||
| cfg := &Config{} | ||
| t.Setenv("TAGGING_HEADER_10", "X-Ten") | ||
| t.Setenv("TAGGING_HEADER_2", "X-Two") | ||
|
|
||
| if err := applyTaggingEnv(cfg); err != nil { | ||
| t.Fatalf("applyTaggingEnv() error = %v", err) | ||
| } | ||
| headers := cfg.Tagging.Headers | ||
| if len(headers) != 2 || headers[0].Header != "X-Two" || headers[1].Header != "X-Ten" { | ||
| t.Fatalf("index ordering wrong: %#v", headers) | ||
| } | ||
| } | ||
|
|
||
| func TestApplyTaggingEnv_Unset(t *testing.T) { | ||
| cfg := &Config{Tagging: TaggingConfig{Headers: []TaggingHeaderConfig{{Header: "X-Team"}}}} | ||
| if err := applyTaggingEnv(cfg); err != nil { | ||
| t.Fatalf("applyTaggingEnv() error = %v", err) | ||
| } | ||
| if len(cfg.Tagging.Headers) != 1 { | ||
| t.Fatalf("no env vars mutated config: %#v", cfg.Tagging.Headers) | ||
| } | ||
| } | ||
|
|
||
| func TestNormalizeTaggingConfig(t *testing.T) { | ||
| cfg := &TaggingConfig{Headers: []TaggingHeaderConfig{ | ||
| {Header: "x-team "}, | ||
| {Header: "X-Cost-Center", Delimiter: ";"}, | ||
| }} | ||
| if err := normalizeTaggingConfig(cfg); err != nil { | ||
| t.Fatalf("normalizeTaggingConfig() error = %v", err) | ||
| } | ||
| if cfg.Headers[0].Header != "X-Team" { | ||
| t.Fatalf("header not canonicalized: %#v", cfg.Headers[0]) | ||
| } | ||
| if cfg.Headers[0].Delimiter != DefaultTaggingDelimiter { | ||
| t.Fatalf("default delimiter not applied: %#v", cfg.Headers[0]) | ||
| } | ||
| if cfg.Headers[1].Delimiter != ";" { | ||
| t.Fatalf("explicit delimiter overwritten: %#v", cfg.Headers[1]) | ||
| } | ||
| } | ||
|
|
||
| func TestNormalizeTaggingConfig_RejectsInvalidAndDuplicate(t *testing.T) { | ||
| invalid := &TaggingConfig{Headers: []TaggingHeaderConfig{{Header: "bad header"}}} | ||
| if err := normalizeTaggingConfig(invalid); err == nil { | ||
| t.Fatalf("expected error for invalid header name") | ||
| } | ||
| empty := &TaggingConfig{Headers: []TaggingHeaderConfig{{Header: " "}}} | ||
| if err := normalizeTaggingConfig(empty); err == nil { | ||
| t.Fatalf("expected error for empty header name") | ||
| } | ||
| dup := &TaggingConfig{Headers: []TaggingHeaderConfig{{Header: "X-Team"}, {Header: "x-team"}}} | ||
| if err := normalizeTaggingConfig(dup); err == nil { | ||
| t.Fatalf("expected error for duplicate header name") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Prefer a table-driven suite for the small env/normalize cases.
Lines 9-98 repeat the same one-case harness several times. Collapsing those cases into a table will make it cheaper to extend the new tagging matrix as more header options are added.
As per coding guidelines, **/*_test.go: Prefer table-driven tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/tagging_test.go` around lines 9 - 98, The tagging tests are written as
several nearly identical one-off cases instead of a table-driven suite. Refactor
the checks in TestApplyTaggingEnv_ParsesAndMerges,
TestApplyTaggingEnv_SortsByIndexAndSkipsGaps, TestApplyTaggingEnv_Unset,
TestNormalizeTaggingConfig, and
TestNormalizeTaggingConfig_RejectsInvalidAndDuplicate into table-driven subtests
so new header options are easier to add and the harness is shared. Keep the same
coverage and assertions, but drive each case from a slice of inputs/expected
results.
Source: Coding guidelines
Comment on lines
+257
to
+259
| if (typeof this.fetchTaggingSettings === "function") { | ||
| this.fetchTaggingSettings(); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Retry settings-page fetches outside route changes.
Lines 257-259 only load tagging settings from _applyRoute(), but submitApiKey() and refreshDashboardDataAfterRuntimeRefresh() never call fetchTaggingSettings(). Opening /settings while unauthenticated, or refreshing runtime on that page, leaves the new panel stale until the user navigates away and back. Please move the settings fetches behind a shared helper and invoke it from those flows too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/admin/dashboard/static/js/dashboard.js` around lines 257 - 259, The
tagging/settings refresh is only triggered from _applyRoute(), so the new
settings panel can stay stale when submitApiKey() or
refreshDashboardDataAfterRuntimeRefresh() run on /settings. Extract the existing
fetchTaggingSettings() call into a shared helper on the dashboard object and
invoke that helper from _applyRoute(), submitApiKey(), and
refreshDashboardDataAfterRuntimeRefresh() so settings are reloaded regardless of
route changes.
Comment on lines
+113
to
+115
| // Labels are request labels extracted from configured tagging headers. | ||
| Labels []string `json:"labels,omitempty" bson:"labels,omitempty"` | ||
|
|
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
DoNotPass headers still leak their values into persisted Labels.
Per internal/server/tagging_test.go's TestTaggingCaptureExtractsLabelsAndStripSet, a header configured with DoNotPass: true (e.g. X-Secret-Tag) still has its value extracted into the labels slice ("internal" shows up in the expected labels). DoNotPass only adds the header to the strip set for upstream forwarding — it does not exclude the value from ExtractLabels. Since Labels is now persisted here in LogData (and propagated through CreateStreamEntry in internal/auditlog/stream_wrapper.go and into UsageEntry.Labels in internal/usage/stream_observer.go), any header an operator flags as "do not pass" for sensitivity reasons still ends up durably stored in audit logs, usage records, and admin dashboards. This likely contradicts operator intent for a header explicitly marked as not to be passed through.
Consider excluding DoNotPass header values from ExtractLabels (or gating them behind a separate flag) so a header marked sensitive is neither forwarded nor logged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/auditlog/auditlog.go` around lines 113 - 115, `DoNotPass` header
values are still being captured into persisted `Labels`, which leaks sensitive
data beyond forwarding. Update the label extraction path around `ExtractLabels`
so headers marked `DoNotPass: true` are excluded from the returned labels (or
only included behind an explicit separate opt-in), while keeping the strip-set
behavior unchanged. Make sure the fix flows through `LogData.Labels`,
`CreateStreamEntry` in `stream_wrapper.go`, and `UsageEntry.Labels` in
`stream_observer.go` so sensitive header values no longer reach audit logs,
usage records, or dashboards.
Comment on lines
+112
to
+134
| func (s *Service) SaveRules(ctx context.Context, rules []Rule) ([]Rule, error) { | ||
| if s == nil || s.store == nil { | ||
| return nil, fmt.Errorf("tagging rules storage is not available") | ||
| } | ||
| if err := NormalizeRules(rules); err != nil { | ||
| return nil, err | ||
| } | ||
| for _, rule := range rules { | ||
| if s.isManagedHeader(rule.Header) { | ||
| return nil, fmt.Errorf("header %q is managed by config/env and is read-only", rule.Header) | ||
| } | ||
| } | ||
| for i := range rules { | ||
| rules[i].Managed = false | ||
| } | ||
| if err := s.store.SaveRules(ctx, rules); err != nil { | ||
| return nil, fmt.Errorf("save tagging rules: %w", err) | ||
| } | ||
| if err := s.Refresh(ctx); err != nil { | ||
| return nil, err | ||
| } | ||
| return s.Rules(), nil | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Concurrent SaveRules calls can return a stale/mismatched merged view.
SaveRules validates, persists via s.store.SaveRules, then calls s.Refresh — but nothing serializes the whole validate→persist→refresh sequence across concurrent callers. If two admin saves (A, B) interleave, A's store.SaveRules(A) can be immediately overwritten by B's store.SaveRules(B), and if A's Refresh runs after B's write, A's call returns s.Rules() reflecting B's data while claiming to be the result of A's save. The persisted store state stays consistent (last-writer-wins), but the API response returned to caller A is silently wrong.
Widening the lock to cover the full critical section fixes this:
🔒 Proposed fix: serialize SaveRules end-to-end
func (s *Service) SaveRules(ctx context.Context, rules []Rule) ([]Rule, error) {
if s == nil || s.store == nil {
return nil, fmt.Errorf("tagging rules storage is not available")
}
if err := NormalizeRules(rules); err != nil {
return nil, err
}
for _, rule := range rules {
if s.isManagedHeader(rule.Header) {
return nil, fmt.Errorf("header %q is managed by config/env and is read-only", rule.Header)
}
}
for i := range rules {
rules[i].Managed = false
}
+ s.refreshMu.Lock()
+ defer s.refreshMu.Unlock()
if err := s.store.SaveRules(ctx, rules); err != nil {
return nil, fmt.Errorf("save tagging rules: %w", err)
}
- if err := s.Refresh(ctx); err != nil {
+ if err := s.refreshLocked(ctx); err != nil {
return nil, err
}
return s.Rules(), nil
}(Refresh would need a lock-free refreshLocked variant, or restructure Refresh to accept an already-held lock, to avoid deadlocking on the mutex.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tagging/service.go` around lines 112 - 134, SaveRules can return a
stale merged view when concurrent callers interleave between validation,
store.SaveRules, and s.Refresh. Serialize the full validate→persist→refresh flow
in Service.SaveRules by holding the service lock across the whole critical
section, and avoid deadlock by using a lock-free refresh path (for example, a
refreshLocked helper) instead of calling Refresh while already holding the
mutex. Ensure the returned s.Rules() always reflects the same save operation
that was just persisted.
Comment on lines
+47
to
+55
| func (s *PostgreSQLStore) SaveRules(ctx context.Context, rules []Rule) error { | ||
| value, err := encodeRules(rules) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| _, err = s.pool.Exec(ctx, ` | ||
| INSERT INTO tagging_settings (key, value, updated_at) VALUES ($1, $2, $3) | ||
| ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value, updated_at = EXCLUDED.updated_at | ||
| `, rulesSettingKey, string(value), time.Now().Unix()) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Add optimistic locking for ruleset saves.
This upsert replaces the singleton rules row unconditionally. With the new admin GET/PUT flow, concurrent edits become silent last-write-wins, so one admin can overwrite another without any conflict signal. updated_at is already stored, but it is never used as a write precondition. Please thread an expected revision/etag through the service API and make the update conditional across all three stores.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tagging/store_postgresql.go` around lines 47 - 55, Add optimistic
locking to the ruleset save path: `PostgreSQLStore.SaveRules` currently
unconditionally upserts the singleton row, so thread an expected revision/etag
from the admin PUT flow into the write API and use it as a precondition before
updating `tagging_settings`. Update `SaveRules` (and the corresponding save
methods in the other two stores) to compare against the stored
revision/`updated_at` value and return a conflict when the caller’s expected
value is stale, instead of silently overwriting the latest rules.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- close the tagging subsystem during app shutdown - classify admin save errors with a typed ValidationError instead of substring matching (a storage error mentioning "read-only" was previously misreported as HTTP 400) - reject credential-bearing headers (Authorization, Cookie, API-key headers) as tagging label sources in config and admin validation - disable "Add Header" while settings load; group tagging routes after the budgets block; document that env entries replace whole YAML entries per header - exercise the labelled path in the streaming perf guard; add handler_tagging tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/tagging/service.go (1)
112-134: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftConcurrent
SaveRulescalls can still return a mismatched merged view.The error-classification fix (typed
ValidationErrorfor the managed-header case) is in place, but the underlying race flagged previously is still present: nothing serializes the validate →s.store.SaveRules→s.Refreshsequence across concurrent callers, so caller A can receives.Rules()reflecting a concurrently-interleaved caller B's write while believing it reflects its own save.🔒 Suggested fix: serialize SaveRules end-to-end
func (s *Service) SaveRules(ctx context.Context, rules []Rule) ([]Rule, error) { if s == nil || s.store == nil { return nil, fmt.Errorf("tagging rules storage is not available") } if err := NormalizeRules(rules); err != nil { return nil, err } for _, rule := range rules { if s.isManagedHeader(rule.Header) { return nil, newValidationError("header %q is managed by config/env and is read-only", rule.Header) } } for i := range rules { rules[i].Managed = false } + s.refreshMu.Lock() + defer s.refreshMu.Unlock() if err := s.store.SaveRules(ctx, rules); err != nil { return nil, fmt.Errorf("save tagging rules: %w", err) } - if err := s.Refresh(ctx); err != nil { + if err := s.refreshLocked(ctx); err != nil { return nil, err } return s.Rules(), nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tagging/service.go` around lines 112 - 134, SaveRules still has a race because concurrent callers can interleave validation, store writes, and Refresh, causing the returned s.Rules() snapshot to reflect another caller’s update. Serialize the full SaveRules flow end-to-end in the Service type (including NormalizeRules, managed-header checks, s.store.SaveRules, and s.Refresh) using the existing service-level synchronization mechanism or a new mutex around the method so each caller sees its own committed view.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/tagging.go`:
- Around line 40-55: The sensitive-header denylist is duplicated between
config/tagging.go and internal/auditlog/auditlog.go, which makes it easy to
drift out of sync. Move the credential-bearing header list into a shared source
of truth that both tagging and audit logging can reference without creating an
import cycle, then update deniedTaggingHeaders and the auditlog redaction logic
to use that shared set.
---
Duplicate comments:
In `@internal/tagging/service.go`:
- Around line 112-134: SaveRules still has a race because concurrent callers can
interleave validation, store writes, and Refresh, causing the returned s.Rules()
snapshot to reflect another caller’s update. Serialize the full SaveRules flow
end-to-end in the Service type (including NormalizeRules, managed-header checks,
s.store.SaveRules, and s.Refresh) using the existing service-level
synchronization mechanism or a new mutex around the method so each caller sees
its own committed view.
🪄 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: 621fdb8a-5212-4f0f-9efb-c853f49c3df8
📒 Files selected for processing (14)
.env.templateCLAUDE.mdconfig/config.example.yamlconfig/tagging.goconfig/tagging_test.gointernal/admin/dashboard/templates/page-settings.htmlinternal/admin/handler_tagging.gointernal/admin/handler_tagging_test.gointernal/admin/routes.gointernal/app/app.gointernal/tagging/service.gointernal/tagging/tagging.gointernal/tagging/tagging_test.gotests/perf/hotpath_test.go
config/tagging.go and auditlog.RedactedHeaders each kept their own copy of the credential-bearing header list, held in sync only by a comment. Move the list into core.IsCredentialHeader as the single source of truth; audit redaction and both tagging validation paths now share it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # internal/usage/stream_observer.go # tests/perf/hotpath_test.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/usage/stream_observer.go (1)
59-65: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
entry.Labelsaliases the observer's shared slice instead of copying.
entry.Labels = o.labelsstores a direct reference to the observer's backing array. IfextractUsageFromEventfires more than once per stream (interim + final usage), every emittedUsageEntryshares the same slice aso.labels. Elsewhere in this PR (internal/live/broker.go'susagePreviewFromEntry) the sameLabelsfield is defensively copied before reuse, suggesting the intended contract is thatLabelsownership shouldn't be shared. A downstream mutation (e.g.,appendwith spare capacity, in-place sort/redaction) on one entry'sLabelscould silently corrupt another entry's or the observer's own labels.🛡️ Proposed fix
- entry.Labels = o.labels + entry.Labels = append([]string(nil), o.labels...)Also applies to: 178-178
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/usage/stream_observer.go` around lines 59 - 65, The Labels field is being shared by reference instead of owned per entry, so each UsageEntry can end up aliasing StreamUsageObserver’s backing slice. Update StreamUsageObserver.SetLabels and the UsageEntry creation path in extractUsageFromEvent so labels are copied before assignment, and keep the same defensive-copy contract used by usagePreviewFromEntry in internal/live/broker.go. Ensure every emitted UsageEntry gets its own slice rather than reusing o.labels.
♻️ Duplicate comments (1)
internal/auditlog/auditlog.go (1)
115-117: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftConfirm
DoNotPassheader values are excluded before landing inLabels.This field persists whatever
ExtractLabelsproduces into audit logs/usage records/dashboards. A prior review on this exact field flagged that headers markedDoNotPass: true(e.g., a secret-tag header) were still being extracted into labels —DoNotPassonly affects upstream forwarding, not label extraction. If that hasn't been fixed upstream ininternal/server/tagging.go, this field is where the leak becomes durably persisted. Please confirmExtractLabelsnow excludesDoNotPassheaders (or gates them behind an explicit opt-in) before this field is populated.#!/bin/bash # Inspect ExtractLabels to confirm DoNotPass headers are excluded from label extraction rg -n -A15 'func.*ExtractLabels' --type=go rg -n 'DoNotPass' --type=go -C5🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auditlog/auditlog.go` around lines 115 - 117, Confirm that `ExtractLabels` in `internal/server/tagging.go` filters out any tagging headers marked `DoNotPass` before assigning results to `auditlog.AuditLog.Labels`. Update the extraction logic so only explicitly allowed headers are returned, or add an opt-in gate for secret-tag headers if needed. Then verify the `Labels` field in `internal/auditlog/auditlog.go` only receives the sanitized output and cannot persist `DoNotPass` values into audit logs or usage records.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/usage/stream_observer.go`:
- Around line 59-65: The Labels field is being shared by reference instead of
owned per entry, so each UsageEntry can end up aliasing StreamUsageObserver’s
backing slice. Update StreamUsageObserver.SetLabels and the UsageEntry creation
path in extractUsageFromEvent so labels are copied before assignment, and keep
the same defensive-copy contract used by usagePreviewFromEntry in
internal/live/broker.go. Ensure every emitted UsageEntry gets its own slice
rather than reusing o.labels.
---
Duplicate comments:
In `@internal/auditlog/auditlog.go`:
- Around line 115-117: Confirm that `ExtractLabels` in
`internal/server/tagging.go` filters out any tagging headers marked `DoNotPass`
before assigning results to `auditlog.AuditLog.Labels`. Update the extraction
logic so only explicitly allowed headers are returned, or add an opt-in gate for
secret-tag headers if needed. Then verify the `Labels` field in
`internal/auditlog/auditlog.go` only receives the sanitized output and cannot
persist `DoNotPass` values into audit logs or usage records.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1abd6c30-5da7-4fe2-8fb0-0a4cc9144dc4
📒 Files selected for processing (8)
config/tagging.gointernal/auditlog/auditlog.gointernal/core/credential_headers.gointernal/core/credential_headers_test.gointernal/live/broker.gointernal/tagging/tagging.gointernal/usage/stream_observer.gotests/perf/hotpath_test.go
Alloc counts are identical across linux/amd64 CI and darwin/arm64 local runs, so every ceiling now sits at baseline +2 allocs with a few hundred bytes of headroom (the responses converter keeps extra bytes for pool cold-starts). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit