fix(abtest): resolve cross-PR review findings - #51
Conversation
Addresses unresolved codex/CodeRabbit findings across Change Sets A-E: High: - hypothesis.ts + abtest-service.ts: lockHypothesis and the pre-locked branch now return deep clones (structuredClone) so the stored hypothesis is fully detached from the caller's config object. Post-lock mutation of the caller's nested objects can no longer silently invalidate the stored checksum. - report.ts: ExperimentReport variants now carry optional revenue and revenuePerRecipient fields. The Markdown table conditionally includes revenue columns when any variant has revenue data, so a revenue_per_recipient primary metric is reportable end-to-end. Low: - stratification.ts: groupOrder keys missing from groupExactCounts now throw instead of silently defaulting to zero. - hypothesis.ts: removed dead duplicate experimentFamilyKey empty check.
OpenCodeReview suggestion: unify the revenue/non-revenue table branches into a single headers+cells loop.
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR deep-clones locked hypotheses, tightens experiment family key validation, adds optional revenue metrics to experiment reports, and rejects missing stratification group counts. ChangesAB test follow-ups
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@packages/abtest/src/abtest-service.ts`:
- Around line 209-211: The ab-test service exposes mutable internal test
objects, allowing callers to change stored hypotheses and invalidate checksums.
Update the storage and retrieval paths around abTest creation plus getTest,
getAllTests, and snapshotTests to deep-clone tests both when storing and before
returning them, preserving independent internal state at every service boundary;
add a regression test that mutates a returned hypothesis and verifies the stored
test remains unchanged.
In `@packages/abtest/src/stratification.ts`:
- Around line 222-223: Update the missing-group validation around
groupExactCounts in the stratification logic to first verify that groupKey is an
own property, rather than relying on groupExactCounts[groupKey] === undefined.
Preserve the existing missing-key handling, and only read the exact count after
the own-property check succeeds.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 0823473c-886f-41c3-9fd4-d99b952ca2f5
📒 Files selected for processing (5)
.sampo/changesets/abtest-review-followup.mdpackages/abtest/src/abtest-service.tspackages/abtest/src/hypothesis.tspackages/abtest/src/report.tspackages/abtest/src/stratification.ts
| // Deep-clone so the stored test's hypothesis is | ||
| // detached from the caller's config object. | ||
| return structuredClone(config.hypothesis!); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Return defensive copies of stored tests.
This clone detaches config.hypothesis, but Lines 355-356 store and return the same mutable abTest; getTest, getAllTests, and snapshotTests also expose internal references. A caller can mutate the returned hypothesis and invalidate its checksum after creation. Return deep defensive copies, or encapsulate/freeze stored state, at every service boundary and add a regression test for this mutation path.
🤖 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 `@packages/abtest/src/abtest-service.ts` around lines 209 - 211, The ab-test
service exposes mutable internal test objects, allowing callers to change stored
hypotheses and invalidate checksums. Update the storage and retrieval paths
around abTest creation plus getTest, getAllTests, and snapshotTests to
deep-clone tests both when storing and before returning them, preserving
independent internal state at every service boundary; add a regression test that
mutates a returned hypothesis and verifies the stored test remains unchanged.
| const exactCount = groupExactCounts[groupKey]; | ||
| if (exactCount === undefined) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Check for an own property, not just an undefined lookup.
groupExactCounts[groupKey] traverses the prototype chain, while the earlier Object.entries(groupExactCounts) validation only considers own properties. An object with an inherited group key can therefore bypass this missing-key invariant. Use an own-property check before reading the count.
Proposed fix
for (const [index, groupKey] of groupOrder.entries()) {
+ if (!Object.prototype.hasOwnProperty.call(groupExactCounts, groupKey)) {
+ throw new Error(
+ `Stratified quota invariant: group "${groupKey}" is in groupOrder but missing from groupExactCounts`,
+ );
+ }
const exactCount = groupExactCounts[groupKey];
- if (exactCount === undefined) {
- throw new Error(
- `Stratified quota invariant: group "${groupKey}" is in groupOrder but missing from groupExactCounts`,
- );
- }📝 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.
| const exactCount = groupExactCounts[groupKey]; | |
| if (exactCount === undefined) { | |
| if (!Object.prototype.hasOwnProperty.call(groupExactCounts, groupKey)) { | |
| throw new Error( | |
| `Stratified quota invariant: group "${groupKey}" is in groupOrder but missing from groupExactCounts`, | |
| ); | |
| } | |
| const exactCount = groupExactCounts[groupKey]; |
🤖 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 `@packages/abtest/src/stratification.ts` around lines 222 - 223, Update the
missing-group validation around groupExactCounts in the stratification logic to
first verify that groupKey is an own property, rather than relying on
groupExactCounts[groupKey] === undefined. Preserve the existing missing-key
handling, and only read the exact count after the own-property check succeeds.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8be2fd0c28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| lines.push(""); | ||
| lines.push("| Variant | Sample | Open Rate | Click Rate | Conversion Rate |"); | ||
| lines.push("|---------|--------|-----------|------------|-----------------|"); | ||
| const hasRevenue = report.variants.some((v) => v.revenue !== undefined); |
There was a problem hiding this comment.
When an exported ExperimentReport is constructed with revenuePerRecipient but no total revenue—a shape explicitly permitted by the new interface—hasRevenue is false, so the Markdown report silently omits the available per-recipient metric even when primaryMetric is revenue_per_recipient. Include either optional revenue field in this presence check so direct callers of reportToMarkdown do not lose valid report data.
Useful? React with 👍 / 👎.
| revenue?: number; | ||
| revenuePerRecipient?: number; |
There was a problem hiding this comment.
The new report fields carry monetary totals and per-recipient values without their currency, even though conversion events require an ISO 4217 currency and VariantConversionAggregate preserves it in packages/abtest/src/conversion-events.ts:28-41. Consequently, a report built from USD, KRW, or even different currencies across variants produces identical unlabeled numbers, making the new revenue columns ambiguous and potentially misleading; propagate the currency into TestResults/ExperimentReport and display it with these values.
Useful? React with 👍 / 👎.
| if (hasRevenue) { | ||
| headers.push("Revenue", "Rev/Recipient"); |
There was a problem hiding this comment.
This adds user-visible revenue columns to the Markdown experiment report, but neither README.md nor README_ko.md documents the new output; their hypothesis-report sections still describe only the pre-registration fields. Update both guides together so the documented report contract remains aligned with the released behavior.
AGENTS.md reference: AGENTS.md:L235-L237
Useful? React with 👍 / 👎.
OpenCodeReview finding: revenueCurrency was declared but unused. Now appends the currency code to the Revenue/Rev/Recipient column headers (e.g. 'Revenue (USD)') when present.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43c60c3401
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| revenue: r.revenue, | ||
| revenuePerRecipient: | ||
| r.revenue !== undefined && r.sampleSize > 0 | ||
| ? r.revenue / r.sampleSize | ||
| : undefined, |
There was a problem hiding this comment.
When revenue-bearing results pass through the public buildExperimentReport path, this mapper copies the revenue amount but the returned report never assigns revenueCurrency, and TestResults has no currency field to supply it. Consequently, the new currency suffix in reportToMarkdown is unreachable without manually crafting or mutating an ExperimentReport, so normally generated revenue tables still display unitless monetary values; propagate a consistently validated currency through the builder input and output.
Useful? React with 👍 / 👎.
| const hasRevenue = report.variants.some( | ||
| (v) => v.revenue !== undefined || v.revenuePerRecipient !== undefined, | ||
| ); |
There was a problem hiding this comment.
When a pre-registered test declares revenue_per_recipient but its results contain no revenue values—as happens with the current Listmonk metrics collectors—this condition is false, so the report identifies revenue as its primary metric while the table omits both revenue columns and shows only secondary rates. Include the revenue columns whenever the declared primary metric is revenue, rendering unavailable measurements as N/A rather than hiding the primary metric.
Useful? React with 👍 / 👎.
Addresses @codex P2 findings on PR #51: - hasRevenue now also checks primaryMetric === 'revenue_per_recipient' so the table shows revenue columns even when no variant has revenue data yet (e.g. before metrics collection). - buildExperimentReport sets revenueCurrency to undefined by default; callers that know the currency set it explicitly.
OpenCodeReview finding: the revenue visibility comment belonged in reportToMarkdown, not in the buildExperimentReport return object.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 596750c1cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const currencySuffix = report.revenueCurrency | ||
| ? ` (${report.revenueCurrency})` | ||
| : ""; | ||
| headers.push(`Revenue${currencySuffix}`, `Rev/Recipient${currencySuffix}`); |
There was a problem hiding this comment.
This makes revenue and revenue-per-recipient columns part of the user-visible Markdown report, but neither README.md nor README_ko.md explains when they appear or how they are interpreted. The repository-wide AGENTS.md requires both English and Korean documentation to be updated for user-visible behavior changes.
AGENTS.md reference: AGENTS.md:L233-L237
Useful? React with 👍 / 👎.
| } | ||
| : undefined, | ||
| preRegistration, | ||
| revenueCurrency: undefined, |
There was a problem hiding this comment.
When buildExperimentReport() receives results containing revenue, it always sets revenueCurrency to undefined, and neither its arguments nor TestResults provide another path for supplying a currency. Consequently JSON omits the currency and Markdown renders monetary values under unlabeled Revenue and Rev/Recipient columns, so consumers cannot interpret or safely compare the amounts; carry the validated currency through the report input instead of discarding it here.
Useful? React with 👍 / 👎.
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Addresses unresolved codex/CodeRabbit findings across Change Sets A-E:
High
lockHypothesisand the pre-locked branch increateTestnow returnstructuredCloneresults so the stored hypothesis is fully detached from the caller's config object. Post-lock mutation of nested objects can no longer silently invalidate the checksum.ExperimentReportvariants now carry optionalrevenueandrevenuePerRecipient. The Markdown table conditionally includes revenue columns when data is present.Low
groupOrderkeys missing fromgroupExactCountsnow throw instead of silently defaulting to zero.experimentFamilyKeyempty check.Gates
308 abtest tests pass,
bun run check(201 architecture paths), build succeeds.@codex review
Summary by CodeRabbit