Repository navigation
feat(OMN-10299): add cost.by_repo.v1 topic to TOPICS constant - #34
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughA new topic constant Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 1 minute and 11 seconds. Comment |
7985b92 to
0d8d29f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shared/types/topics.test.ts (1)
22-26: Strengthen the naming-convention assertion to match the documented prefix.On Line 25,
^onex\.is broader than your documented convention and can pass invalid topic forms.Proposed test tightening
- for (const v of values) { - expect(v).toMatch(/^onex\./); - } + for (const v of values) { + expect(v).toMatch(/^onex\.snapshot\.projection\./); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/types/topics.test.ts` around lines 22 - 26, The test asserting topic naming should be tightened to the documented prefix: update the assertion in the test that iterates over Object.values(TOPICS) (the "all TOPICS values follow the onex.snapshot.projection prefix convention" test) to use a stricter regex that matches the exact documented prefix (for example require "onex.snapshot.projection." at the start) instead of the loose /^onex\./ pattern so invalid topic forms are rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shared/types/topics.ts`:
- Around line 33-34: TOPICS.costByRepo is defined but not exposed in
scripts/generate-registry.ts; open scripts/generate-registry.ts and add a
registry/component entry that references TOPICS.costByRepo (the same pattern
used for llmCost, delegationSummary, baselinesRoi, etc.), providing the required
manifest fields (id/name/topic/projection/handler as used by other entries) so
the costByRepo topic is surfaced in the generated registry; ensure the new entry
uses the TOPICS.costByRepo symbol and matches the existing component shape in
the file.
---
Nitpick comments:
In `@shared/types/topics.test.ts`:
- Around line 22-26: The test asserting topic naming should be tightened to the
documented prefix: update the assertion in the test that iterates over
Object.values(TOPICS) (the "all TOPICS values follow the
onex.snapshot.projection prefix convention" test) to use a stricter regex that
matches the exact documented prefix (for example require
"onex.snapshot.projection." at the start) instead of the loose /^onex\./ pattern
so invalid topic forms are rejected.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6397670-5410-41fd-8430-caaf76f87860
📒 Files selected for processing (2)
shared/types/topics.test.tsshared/types/topics.ts
0d8d29f to
fcf2d78
Compare
Adds costByRepo key to the TOPICS constant in shared/types/topics.ts and ships a companion test file covering the value, TypeScript union membership, uniqueness, and naming convention invariants. Upstream-blocked: repo_name column absent from llm_cost_aggregates (omnibase_infra migration 031:142 uses generic aggregation_key only). Follow-up required in omnibase_infra before consumers can receive data.
fcf2d78 to
b37dd2c
Compare
Summary
Adds
costByRepo: 'onex.snapshot.projection.cost.by_repo.v1'to theTOPICSconstant inshared/types/topics.tsand ships a companion test file.Ticket: OMN-10299
Changes
shared/types/topics.ts: newcostByRepokey at the end ofTOPICSshared/types/topics.test.ts: 4 tests — value assertion,TopicSymbolunion membership (compile-time), uniqueness invariant, naming conventionUpstream blocker —
repo_namecolumn gapThe
repo_namecolumn does NOT exist inllm_cost_aggregates.Migration
omnibase_infra/docker/migrations/forward/031_create_llm_call_metrics_and_cost_aggregates.sql:142defines only a genericaggregation_key VARCHAR(512)column. Until upstream resolves this, consumers will see the topic registered with no data.Two options for omnibase_infra:
(a) Add a new migration (
032+) with an explicitrepo_name VARCHARcolumn tollm_cost_aggregates.(b) Document and standardize an
aggregation_keyencoding scheme (e.g.repo:<name>) and emit/query rows using that encoding.A follow-up ticket in omnibase_infra should be created to track this decision. This PR registers the topic as a leading contract; the manifest entry (OMN-10302) also blocks on that upstream decision.
dod_evidence
shared/types/topics.tscontainscostByRepokey:grep -c 'costByRepo' shared/types/topics.ts→1TOPICS.costByRepotoTopicSymbolwithout error:npm run checkexits 0npm run test:run -- shared/types/topics.test.ts→ 4/4 passnpm run lintexits 0Summary by CodeRabbit
New Features
Tests