Repository navigation
chore(demo): add synthetic data seeder - #440
Conversation
|
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 (3)
📝 WalkthroughWalkthroughAdds a ChangesNULL error_type fix in audit log readers
Demo data seeding script
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Confidence Score: 4/5The seeder can overwrite existing budget configuration in a reused SQLite demo database and should be fixed before relying on it around shared budget keys. The changed audit reader behavior is covered, and the remaining concern is localized to the new demo seeding path where existing budget rows can be clobbered. tools/seed-demo-data.sh
What T-Rex did
Reviews (2): Last reviewed commit: "test(audit): cover nullable postgres err..." | Re-trigger Greptile |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tools/seed-demo-data.sh`:
- Around line 382-392: The demo cost calculation in the seed script treats local
response cache hits as if they still paid full provider output costs, which
conflicts with the intended cache-savings story. Update the cost logic around
the SELECT that computes input_cost, output_cost, and total_cost so the
cache_type IS NOT NULL branch zeros out output_cost and excludes it from
total_cost, while leaving prompt_cache_hit behavior unchanged. Use the existing
cache_type and prompt_cache_hit conditions in this cost block to distinguish
local cache hits from provider prompt-cache hits.
- Around line 86-88: The sqlite3 seeding block is missing a fail-fast setting,
so errors can still allow the script to proceed toward COMMIT and leave partial
data behind. Update the sqlite3 heredoc in the seed-demo-data shell script to
enable .bail on before any transactional work, keeping the rest of the seed flow
intact and using the existing sqlite3 invocation/PRAGMA block as the insertion
point.
🪄 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: 4e19e876-0d57-4b8b-8597-beb2ee4f7626
📒 Files selected for processing (4)
Makefileinternal/auditlog/reader_postgresql.gointernal/auditlog/reader_sqlite.gotools/seed-demo-data.sh
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/auditlog/reader_postgresql_test.go`:
- Around line 56-97: The current test only covers scanPostgreSQLLogEntry, but
the NULL handling change also affects the GetLogs row iteration path in
reader_postgresql.go. Add or extend a test around GetLogs that returns a row
with error_type set to NULL and verifies the resulting log entry keeps ErrorType
empty, using the existing scanPostgreSQLLogEntry/GetLogs symbols to locate the
behavior. Ensure the new test exercises the production path end-to-end rather
than only the row scanner helper.
In `@tools/seed-demo-data.sh`:
- Line 684: The cache classification in the `cache_mix` query is treating
zero-valued cache counters as cached because `json_extract(...) IS NOT NULL`
only checks presence, not whether tokens were actually used. Update the `SELECT`
logic in `tools/seed-demo-data.sh` to use a positive-token predicate for
`prompt_cached_tokens`, `cached_tokens`, and `cache_read_input_tokens` so only
rows with values greater than zero are classified as `prompt-cache`.
🪄 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: e26b43a8-a1d5-41c8-aa9b-2735602864ed
📒 Files selected for processing (3)
internal/auditlog/reader_postgresql_test.gointernal/auditlog/store_sqlite_test.gotools/seed-demo-data.sh
| ON CONFLICT(user_path, period_seconds) DO UPDATE SET | ||
| amount = excluded.amount, | ||
| source = excluded.source, | ||
| last_reset_at = excluded.last_reset_at, | ||
| updated_at = excluded.updated_at; |
There was a problem hiding this comment.
Preserve existing budgets
The budget upsert rewrites any existing budget for these shared user_path/period_seconds keys, not just rows from the requested seed prefix. If a SQLite demo database already has a real /engineering monthly budget, rerunning the seeder changes its amount, source, and reset timestamp to ${prefix}, so the advertised prefix-scoped replacement clobbers non-demo budget configuration.
Artifacts
Repro: generated SQLite harness that creates the conflicting budget row and runs the seeder
- Contains supporting evidence from the run (text/x-shellscript; charset=utf-8).
- Keeps the command output available without making the summary code-heavy.
Summary
make seed-demo-datatarget backed by a rolling SQLite demo data seedererror_typevalues in SQLite and PostgreSQL readersTesting
bash -n tools/seed-demo-data.shgo test -count=1 ./internal/auditlog/...make test-race,go mod tidy, dashboard JavaScript unit tests, hot-path performance guard,make lintSummary by CodeRabbit
New Features
seed-demo-datacommand to populate a SQLite database with sample usage traffic, audit logs, and budget data.Bug Fixes
error_typeis missing/NULL so the returnedErrorTypestays empty.