Conversation
📝 WalkthroughWalkthroughAdded three documentation files to the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
borbelyr-kong
added
release-note/ignore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
.agents/skills/billing/references/subscription-sync.md (1)
20-30: Add language tags to fenced blocksNice doc overall — just a markdownlint cleanup: Line 20 and Line 66 fenced blocks should declare a language (
textis enough here).Also applies to: 66-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/billing/references/subscription-sync.md around lines 20 - 30, The fenced code blocks in .agents/skills/billing/references/subscription-sync.md that contain the ASCII flow (the block starting with "SynchronizeSubscriptionAndInvoiceCustomer(ctx, SubscriptionView, asOf)" and the other block around lines showing the later flow) lack a language tag; update both fenced blocks to declare a language (use "text") so they become ```text ... ``` for markdownlint compliance..agents/skills/billing/SKILL.md (1)
16-30: Add language tags to fenced code blocksQuick docs polish: the fenced blocks at Line 16, Line 116, Line 222, Line 235, Line 273, and Line 309 are missing language identifiers (MD040). Adding
text/gowhere appropriate will keep lint green.Also applies to: 116-132, 222-232, 235-241, 273-275, 309-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/billing/SKILL.md around lines 16 - 30, The markdown fenced code blocks in SKILL.md that contain the directory listing and other examples are missing language identifiers (causing MD040); update each triple-backtick fence that wraps the directory tree (the block beginning with "openmeter/billing/ # Domain types + service/adapter interfaces...") and the other similar blocks to include an appropriate language tag (e.g., use "text" for plain listings and "go" for Go snippets) so each fenced block becomes ```text or ```go respectively; ensure you update all listed blocks so linting passes..agents/skills/billing/references/testing.md (1)
5-9: Add a language tag to the fenced code blockLine 5 starts a fenced block without a language, which triggers MD040. Easy cleanup.
Suggested patch
-``` +```text billingtest.BaseSuite (test/billing/suite.go) ↳ billingtest.SubscriptionMixin (test/billing/subscription_suite.go) ↳ subscriptionsync.SuiteBase (worker/subscriptionsync/service/suitebase_test.go)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.agents/skills/billing/references/testing.md around lines 5 - 9, The fenced
code block containing the test hierarchy lines ("billingtest.BaseSuite",
"billingtest.SubscriptionMixin", "subscriptionsync.SuiteBase") is missing a
language tag and triggers MD040; fix it by changing the opening fence fromtotext (or another appropriate language likenone) so the block becomestext followed by the three lines and the closing ```, ensuring the fenced
block has an explicit language tag.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/billing/references/subscription-sync.md:
- Line 70: Update the "ChildUniqueReferenceID" note to remove the incorrect
claim that one-time (non-recurring) items omit "/period[N]" and instead state
that the generator always includes "period[...]" in the UniqueID format; edit
the wording in the ChildUniqueReferenceID section so it accurately reflects that
"period[N]" is present for one-time items as generated, and clarify any
implications for sync/debug work (reference the "ChildUniqueReferenceID" and
"UniqueID format"/"period[...]" terminology to locate the paragraph to change).In @.agents/skills/billing/references/testing.md:
- Line 30: Update the example type for MeterAdapter to match the actual
BaseSuite API: replace references to *metermock.MockRepository with
*meteradapter.TestAdapter in the docs/example so it reflects
BaseSuite.MeterAdapter. Search for the symbol MeterAdapter and ensure any
example signatures or variable declarations use *meteradapter.TestAdapter
instead of *metermock.MockRepository.- Line 8: The docs reference points to the wrong test file for
subscriptionsync.SuiteBase; update the two occurrences (currently pointing to
worker/subscriptionsync/service/suitebase_test.go at lines 8 and 54) to the
correct path openmeter/billing/worker/subscriptionsync/service/base_test.go and
ensure the symbol name subscriptionsync.SuiteBase remains intact so readers are
directed to the actual SuiteBase definition in base_test.go.In @.agents/skills/billing/SKILL.md:
- Around line 392-393: Update the wording to remove the contradiction about
advancement strategies: change the sentence that starts “Worker uses
BackgroundAdvancementStrategy” so it consistently reflects that the
billing-worker binary uses async/event-based advancement but the
asyncadvance.Handler within it specifically uses ForegroundAdvancementStrategy
to avoid event loops (and that tests use ForegroundAdvancementStrategy), and
ensure references to ForegroundAdvancementStrategy and QueuedAdvancementStrategy
are not mixed with BackgroundAdvancementStrategy; keep mentions of
billing-worker, asyncadvance.Handler, ForegroundAdvancementStrategy,
QueuedAdvancementStrategy, and BackgroundAdvancementStrategy to make the
intended behavior explicit and consistent.
Nitpick comments:
In @.agents/skills/billing/references/subscription-sync.md:
- Around line 20-30: The fenced code blocks in
.agents/skills/billing/references/subscription-sync.md that contain the ASCII
flow (the block starting with "SynchronizeSubscriptionAndInvoiceCustomer(ctx,
SubscriptionView, asOf)" and the other block around lines showing the later
flow) lack a language tag; update both fenced blocks to declare a language (use
"text") so they becometext ...for markdownlint compliance.In @.agents/skills/billing/references/testing.md:
- Around line 5-9: The fenced code block containing the test hierarchy lines
("billingtest.BaseSuite", "billingtest.SubscriptionMixin",
"subscriptionsync.SuiteBase") is missing a language tag and triggers MD040; fix
it by changing the opening fence fromtotext (or another appropriate
language likenone) so the block becomestext followed by the three lines
and the closing ```, ensuring the fenced block has an explicit language tag.In @.agents/skills/billing/SKILL.md:
- Around line 16-30: The markdown fenced code blocks in SKILL.md that contain
the directory listing and other examples are missing language identifiers
(causing MD040); update each triple-backtick fence that wraps the directory tree
(the block beginning with "openmeter/billing/ # Domain types +
service/adapter interfaces...") and the other similar blocks to include an
appropriate language tag (e.g., use "text" for plain listings and "go" for Go
snippets) so each fenced block becomestext orgo respectively; ensure you
update all listed blocks so linting passes.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `407bbf71-7535-4afc-b011-ac15aea8e02e` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between bbdcb11facb89ef833b12452ec8ac488892cb097 and 2fb7d083a534d6d391a400f7a8f042fd8c40b936. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `.agents/skills/billing/SKILL.md` * `.agents/skills/billing/references/subscription-sync.md` * `.agents/skills/billing/references/testing.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| {subscriptionID}/{phaseKey}/{itemKey}/v[{version}]/period[{periodIndex}] | ||
| ``` | ||
|
|
||
| One-time (non-recurring) items omit `/period[N]`. Version increments when the rate card changes within a phase. |
There was a problem hiding this comment.
ChildUniqueReferenceID note for one-time items is incorrect
Line 70 says one-time items omit /period[N], but the generator includes period[...] in the UniqueID format consistently. This part should be corrected to avoid wrong assumptions in sync/debug work.
Suggested patch
-One-time (non-recurring) items omit `/period[N]`. Version increments when the rate card changes within a phase.
+One-time and recurring items both include `/period[N]`. Version increments when the rate card changes within a phase.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| One-time (non-recurring) items omit `/period[N]`. Version increments when the rate card changes within a phase. | |
| One-time and recurring items both include `/period[N]`. Version increments when the rate card changes within a phase. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/billing/references/subscription-sync.md at line 70, Update
the "ChildUniqueReferenceID" note to remove the incorrect claim that one-time
(non-recurring) items omit "/period[N]" and instead state that the generator
always includes "period[...]" in the UniqueID format; edit the wording in the
ChildUniqueReferenceID section so it accurately reflects that "period[N]" is
present for one-time items as generated, and clarify any implications for
sync/debug work (reference the "ChildUniqueReferenceID" and "UniqueID
format"/"period[...]" terminology to locate the paragraph to change).
| ``` | ||
| billingtest.BaseSuite (test/billing/suite.go) | ||
| ↳ billingtest.SubscriptionMixin (test/billing/subscription_suite.go) | ||
| ↳ subscriptionsync.SuiteBase (worker/subscriptionsync/service/suitebase_test.go) |
There was a problem hiding this comment.
SuiteBase file path looks incorrect in docs
Small doc fix: Line 8 and Line 54 point to worker/subscriptionsync/service/suitebase_test.go, but SuiteBase is in openmeter/billing/worker/subscriptionsync/service/base_test.go. This can send folks to the wrong file.
Suggested patch
- ↳ subscriptionsync.SuiteBase (worker/subscriptionsync/service/suitebase_test.go)
+ ↳ subscriptionsync.SuiteBase (openmeter/billing/worker/subscriptionsync/service/base_test.go)
...
-## SuiteBase for Sync Tests (`worker/subscriptionsync/service/suitebase_test.go`)
+## SuiteBase for Sync Tests (`openmeter/billing/worker/subscriptionsync/service/base_test.go`)Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/billing/references/testing.md at line 8, The docs reference
points to the wrong test file for subscriptionsync.SuiteBase; update the two
occurrences (currently pointing to
worker/subscriptionsync/service/suitebase_test.go at lines 8 and 54) to the
correct path openmeter/billing/worker/subscriptionsync/service/base_test.go and
ensure the symbol name subscriptionsync.SuiteBase remains intact so readers are
directed to the actual SuiteBase definition in base_test.go.
| BillingService billing.Service | ||
| BillingAdapter billing.Adapter | ||
| MockStreamingConnector *streamingtestutils.MockStreamingConnector | ||
| MeterAdapter *metermock.MockRepository |
There was a problem hiding this comment.
MeterAdapter type in example is stale
At Line 30, the shown type is *metermock.MockRepository, but BaseSuite exposes MeterAdapter *meteradapter.TestAdapter. Worth correcting so examples match the actual suite API.
Suggested patch
-MeterAdapter *metermock.MockRepository
+MeterAdapter *meteradapter.TestAdapter📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MeterAdapter *metermock.MockRepository | |
| MeterAdapter *meteradapter.TestAdapter |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/billing/references/testing.md at line 30, Update the example
type for MeterAdapter to match the actual BaseSuite API: replace references to
*metermock.MockRepository with *meteradapter.TestAdapter in the docs/example so
it reflects BaseSuite.MeterAdapter. Search for the symbol MeterAdapter and
ensure any example signatures or variable declarations use
*meteradapter.TestAdapter instead of *metermock.MockRepository.
| - **Worker uses `BackgroundAdvancementStrategy`**: the billing-worker binary uses async advancement (events), but the `asyncadvance.Handler` within it uses `ForegroundAdvancementStrategy` to prevent event loops. Tests always use `ForegroundAdvancementStrategy`. | ||
|
|
There was a problem hiding this comment.
Advancement strategy wording is inconsistent here
At Line 392, “Worker uses BackgroundAdvancementStrategy” conflicts with earlier sections that describe ForegroundAdvancementStrategy and QueuedAdvancementStrategy, and with Line 393 itself. I’d reword this section to avoid mixed signals.
Suggested patch
-- **Worker uses `BackgroundAdvancementStrategy`**: the billing-worker binary uses async advancement (events), but the `asyncadvance.Handler` within it uses `ForegroundAdvancementStrategy` to prevent event loops. Tests always use `ForegroundAdvancementStrategy`.
+- **Worker advancement behavior**: the billing-worker processes async advancement via events, while `asyncadvance.Handler` uses `ForegroundAdvancementStrategy` to prevent event loops. Tests also use `ForegroundAdvancementStrategy`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **Worker uses `BackgroundAdvancementStrategy`**: the billing-worker binary uses async advancement (events), but the `asyncadvance.Handler` within it uses `ForegroundAdvancementStrategy` to prevent event loops. Tests always use `ForegroundAdvancementStrategy`. | |
| - **Worker advancement behavior**: the billing-worker processes async advancement via events, while `asyncadvance.Handler` uses `ForegroundAdvancementStrategy` to prevent event loops. Tests also use `ForegroundAdvancementStrategy`. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/billing/SKILL.md around lines 392 - 393, Update the wording
to remove the contradiction about advancement strategies: change the sentence
that starts “Worker uses BackgroundAdvancementStrategy” so it consistently
reflects that the billing-worker binary uses async/event-based advancement but
the asyncadvance.Handler within it specifically uses
ForegroundAdvancementStrategy to avoid event loops (and that tests use
ForegroundAdvancementStrategy), and ensure references to
ForegroundAdvancementStrategy and QueuedAdvancementStrategy are not mixed with
BackgroundAdvancementStrategy; keep mentions of billing-worker,
asyncadvance.Handler, ForegroundAdvancementStrategy, QueuedAdvancementStrategy,
and BackgroundAdvancementStrategy to make the intended behavior explicit and
consistent.
|
|
||
| Primary location: `openmeter/billing/worker/subscriptionsync/` | ||
|
|
||
| ## Entry Points |
There was a problem hiding this comment.
There's a subscriptionsync skill already afaik, let's merge that.
Current billing structure doesn't help either, but that should have more context.
2 participants
Summary by CodeRabbit
Release Notes