Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .sampo/changesets/abtest-review-followup.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
npm/@listmonk-ops/abtest: patch
---

Fix unresolved review findings across Change Sets A-E: deep-clone locked hypotheses to prevent caller-mutation checksum invalidation, include revenue columns in reports when revenue_per_recipient is the primary metric, reject missing group keys in stratification, and remove dead duplicate validation.
4 changes: 3 additions & 1 deletion packages/abtest/src/abtest-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,9 @@ export class AbTestService {
"Pre-locked hypothesis checksum verification failed; the metadata may have been tampered with",
);
}
return config.hypothesis!;
// Deep-clone so the stored test's hypothesis is
// detached from the caller's config object.
return structuredClone(config.hypothesis!);
Comment on lines +209 to +211

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

})()
: lockHypothesis(config.hypothesis)
: undefined,
Expand Down
11 changes: 5 additions & 6 deletions packages/abtest/src/hypothesis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -272,11 +272,6 @@ export function validateHypothesisMetadata(
"experimentScope.experimentFamilyKey must be a non-empty string",
);
}
if (!scope.experimentFamilyKey || scope.experimentFamilyKey.trim().length === 0) {
throw new HypothesisValidationError(
"experimentScope.experimentFamilyKey must be a non-empty string",
);
}
// Reject delimiter-only and empty-segment keys (".", "foo.", "foo..bar")
// by requiring one or more alphanumeric segments joined by single
// separators from [._-].
Expand Down Expand Up @@ -379,7 +374,11 @@ export function lockHypothesis(
);
}
const checksum = computeHypothesisChecksum(metadata);
return { ...metadata, lockedAt, checksum };
// Deep-clone so nested objects (primaryMetric, expectedLift, owner,
// experimentScope) are detached from the caller's references. This
// prevents post-lock mutation of the caller's object from silently
// invalidating the stored checksum.
return structuredClone({ ...metadata, lockedAt, checksum });
}

/**
Expand Down
39 changes: 34 additions & 5 deletions packages/abtest/src/report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ export interface ExperimentReport {
clickRate: number;
conversionRate: number;
openRate: number;
revenue?: number;
revenuePerRecipient?: number;
Comment on lines +49 to +50

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the currency alongside reported revenue

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 👍 / 👎.

}>;
srmPassed?: boolean;
srmPValue?: number;
Expand Down Expand Up @@ -116,6 +118,11 @@ export function buildExperimentReport(
clickRate: r.clickRate,
conversionRate: r.conversionRate,
openRate: r.openRate,
revenue: r.revenue,
revenuePerRecipient:
r.revenue !== undefined && r.sampleSize > 0
? r.revenue / r.sampleSize
: undefined,
Comment on lines +123 to +127

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Propagate the revenue currency into built reports

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 👍 / 👎.

};
});

Expand Down Expand Up @@ -258,12 +265,34 @@ export function reportToMarkdown(report: ExperimentReport): string {

lines.push("## Variant Results");
lines.push("");
lines.push("| Variant | Sample | Open Rate | Click Rate | Conversion Rate |");
lines.push("|---------|--------|-----------|------------|-----------------|");
const hasRevenue = report.variants.some((v) => v.revenue !== undefined);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Check both revenue fields before omitting columns

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 👍 / 👎.

const headers = [
"Variant",
"Sample",
"Open Rate",
"Click Rate",
"Conversion Rate",
];
if (hasRevenue) {
headers.push("Revenue", "Rev/Recipient");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Document the revenue report output in both user guides

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 👍 / 👎.

}
lines.push(`| ${headers.join(" | ")} |`);
lines.push(`|${headers.map(() => "---------").join("|")}|`);
for (const v of report.variants) {
lines.push(
`| ${v.variantName} | ${v.sampleSize} | ${v.openRate.toFixed(2)}% | ${v.clickRate.toFixed(2)}% | ${v.conversionRate.toFixed(2)}% |`,
);
const cells = [
v.variantName,
String(v.sampleSize),
`${v.openRate.toFixed(2)}%`,
`${v.clickRate.toFixed(2)}%`,
`${v.conversionRate.toFixed(2)}%`,
];
if (hasRevenue) {
cells.push(
v.revenue?.toFixed(2) ?? "N/A",
v.revenuePerRecipient?.toFixed(4) ?? "N/A",
);
}
lines.push(`| ${cells.join(" | ")} |`);
}
lines.push("");

Expand Down
7 changes: 6 additions & 1 deletion packages/abtest/src/stratification.ts
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,12 @@ export function computeStratifiedQuotas(params: {
[];

for (const [index, groupKey] of groupOrder.entries()) {
const exactCount = groupExactCounts[groupKey] ?? 0;
const exactCount = groupExactCounts[groupKey];
if (exactCount === undefined) {
Comment on lines +222 to +223

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

Suggested change
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.

throw new Error(
`Stratified quota invariant: group "${groupKey}" is in groupOrder but missing from groupExactCounts`,
);
}
const ideal = (stratumSize * exactCount) / totalAudience;
ideals.push({ groupKey, ideal, index });
rowQuotas[groupKey] = Math.floor(ideal);
Expand Down