report losers/winners: --payout, so sources that sell at a loss get checked - #198
tracking202 wants to merge 9 commits into
Conversation
…hecked
`p202 report losers` lists CUT rows only: spend with zero conversions, or a
CPC above break-even. Break-even came from --max-cpc or the payout of
--aff_campaign_id, and that filter also narrows the report to one campaign,
which turns the attribution check off. So a traffic source that makes some
sales at a loss (the case the starter check exists for) was WATCH, never
listed, and never checked. On the demo account both flipped sources
(ChatGPT Ads -39%, Meta Prospecting -53% last touch) came back as an empty
list.
--payout takes your revenue per conversion. Each row's break-even CPC is
the payout times its own conversion rate, with no filter, so the check runs.
On the demo with --payout 160 both sources are CUT by break-even and come
back TEST ("CPC $2.82 > breakeven $1.71, but starts sales (First touch ROI
+46.6%, 15 assists)"), while the profitable ones stay off the list.
Precedence: --max-cpc, then --payout, then the campaign payout. A negative
or non-finite --payout is refused before any request.
Course lesson B5 asks Claude Code for this report with the member's average
order value.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 9c3c67e528
ℹ️ 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".
| if err == nil || !strings.Contains(err.Error(), "--payout must be more than 0") { | ||
| t.Fatalf("a negative --payout should be refused naming the flag, got %v", err) |
There was a problem hiding this comment.
This test checks only the message, so it would continue passing if this new path stopped returning a categorized validation error or lost its actionable hint, breaking the CLI's JSON error envelope and agent guidance. Assert exitCodeForError(err) == 1 and the expected hintFor(err)/api.HintFor(err) content as required for new CLI error paths.
AGENTS.md reference: AGENTS.md:L54-L56
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1220e11. TestTriageRefusesABadPayoutBeforeAnyRequest replaces the message-only test. For each refusal it asserts the message, exitCodeForError(err) == ExitValidation, that api.HintFor(err) names "revenue per conversion", and that the stub server received no request. It covers both commands.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Winner classification does not consistently apply the supplied payout, and key validation and agent-facing coverage remain incomplete.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Adds payout-based break-even analysis to optimization reports without campaign filtering.
Changes:
- Adds
--payoutwith validation and precedence rules. - Expands CLI tests and command documentation.
- Updates 1.9.76 release notes.
| File | Description |
|---|---|
go-cli/cmd/report_optimize.go |
Adds payout handling and help text. |
go-cli/cmd/report_optimize_test.go |
Tests loser classification and validation. |
go-cli/cmd/COMMAND_CONTRACTS.md |
Documents request behavior and precedence. |
changelogs.txt |
Adds the release-note entry. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| case payoutFlag > 0: | ||
| payout = payoutFlag |
There was a problem hiding this comment.
Fixed in 1220e11. With --payout, each classic row's total_net becomes total_leads × payout − total_cost before classify. The payout now decides SCALE the same way it decides CUT. Output and sorting use the same value, and each row carries the payout it was valued at. I applied the same rule to the attribution check: under --payout, first-touch ROI is attributed_conversions × payout against the attribution row's cost, not the server's recorded-revenue roi. A TEST or CLOSER reason now values a sale the way its row does. Tests: TestWinnersPayoutValuesEachSaleAtThePayout covers a row at −$50 on recorded income that is SCALE with +$300 at --payout 60 and omitted without it. TestLosersFirstTouchROIUsesThePayout covers ChatGPT Ads at --payout 100: first-touch ROI −8.39%, held as TEST on its 15 assists only. Help, COMMAND_CONTRACTS.md and the changelog are updated.
| c.Flags().Float64("min-clicks", 1, "Ignore rows with fewer than N clicks (significance floor)") | ||
| c.Flags().Float64("max-cpc", 0, "Break-even CPC target (else derived from campaign payout × CVR)") | ||
| c.Flags().Float64("max-cpc", 0, "Break-even CPC target (else payout × each row's CVR, from --payout or the campaign)") | ||
| c.Flags().Float64("payout", 0, "Revenue per conversion, e.g. your average order value: each row's break-even CPC is this × its conversion rate. Unlike --aff_campaign_id it doesn't filter the report, so the attribution check still runs") |
There was a problem hiding this comment.
Added in 36459d2: tests/fixtures/agent-eval/cases/triage.json, a seeded case and its negative twin.
- triage-001. EVAL Loss Source sends a $60-a-click tracker to EVAL Payout Offer. Each run adds one click and one sale recorded at $100, so the source makes $40 a sale on recorded revenue and loses $20 at the $40 the ask states. The agent must run
--payout(runs_one_of) and name the source (reply_includes). A check pins the seeded answer. The rubric fails an answer read from recorded revenue alone, or one that filters to a campaign to get a payout. - triage-002. Same seed, and the ask gives no value per sale.
--payoutmust not run (never_runs), and a check pins that the source is not a loser on recorded revenue.
reference-agent.shroutes both asks. On this push's agent-evals run (37428631791) both cases passed their deterministic half: setup, runs, checks and reply. They reportneeds_judgelike all 23 cases there, since that run has no judge command.
| func TestLosersRefusesANegativePayoutBeforeAnyRequest(t *testing.T) { | ||
| tmp := t.TempDir() | ||
| setTestHome(t, tmp) | ||
| writeTestConfig(t, tmp, "http://127.0.0.1:1", "test-key") | ||
| _, _, err := executeCommand("report", "losers", "--payout", "-5") | ||
| if err == nil || !strings.Contains(err.Error(), "--payout must be more than 0") { | ||
| t.Fatalf("a negative --payout should be refused naming the flag, got %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 1220e11. TestTriageRefusesABadPayoutBeforeAnyRequest is table-driven over losers and winners × -5, -0.01, NaN, Inf, +Inf, -Inf. Each case asserts the message, ExitValidation, the actionable hint, and zero requests to the server.
…e payout error contract - With --payout, each classic row's total_net is total_leads x payout - total_cost, for classification and output: winners lists a row that is profitable at the stated payout even when the campaign's recorded income puts it at a loss (it was WATCH and omitted), and each row carries the payout it was valued at. - The attribution check's first-touch ROI is attributed_conversions x payout against the row's cost under --payout, not the server's recorded-revenue roi, so a TEST or CLOSER reason values a sale the same way the row does. - TestTriageRefusesABadPayoutBeforeAnyRequest: losers and winners, -5, -0.01, NaN, Inf, +Inf, -Inf; asserts the message, ExitValidation, the hint, and that no request was made. - TestWinnersPayoutValuesEachSaleAtThePayout, TestLosersFirstTouchROIUsesThePayout. - Help, COMMAND_CONTRACTS.md and the changelog say what --payout values. Claude-Session: https://claude.ai/code/session_01MxDwKTKzyFMjLzVfQDoRdQ
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1220e11cf3
ℹ️ 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".
| if payoutFlag > 0 { | ||
| // The stated revenue per conversion values every row's sales, so a row profitable at that payout is | ||
| // a winner (and one that isn't, a loser) whatever the campaign's recorded income says. | ||
| net = leads*payoutFlag - cost |
There was a problem hiding this comment.
With a finite but large accepted value such as --payout 1e308 and a row having two conversions, this multiplication produces +Inf. rowsToJSON then cannot marshal total_net (and the attribution calculation can similarly overflow), but RunE still returns success, leaving an agent with no JSON result and an uncategorized stderr message. Reject payouts or computed values that would overflow and return a categorized validation error through the centralized error path.
AGENTS.md reference: AGENTS.md:L56-L56
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a0bc986. --payout is capped at 1,000,000,000 per conversion, far past any real order value. That keeps conversions × payout finite for any count a report can hold, so total_net and the first-touch ROI can't overflow to +Inf. An over-cap value is refused before any request with exit code 1 and the same hint. Test: TestTriageRefusesABadPayoutBeforeAnyRequest covers 1e308 and 1000000000.01 on both commands.
… without one, none is invented Review on #198: the motivating case is an agent choosing the CLI route, which the unit tests can't show. - triage-001: EVAL Loss Source sends a $60-a-click tracker to EVAL Payout Offer and each run adds one click and one $100 sale, so it makes $40 a sale on recorded revenue and loses $20 at the $40 the ask states. The agent must run --payout (runs_one_of) and name the source (reply_includes); a check pins the seeded answer. - triage-002, the negative twin: no value per sale in the ask. --payout must not run (never_runs), and a check pins that the source is not a loser on recorded revenue. - reference-agent.sh routes both asks: --payout with the stated value, or the recorded revenue and an offer to rerun with an average order value. One click and one sale a run keeps the conversion rate at 100 % however often the cases run, so the break-even stays $40 against the $60 CPC. Claude-Session: https://claude.ai/code/session_01MxDwKTKzyFMjLzVfQDoRdQ
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36459d2d68
ℹ️ 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".
| } | ||
| } | ||
| payoutFlag, _ := cmd.Flags().GetFloat64("payout") | ||
| if payoutFlag < 0 || math.IsNaN(payoutFlag) || math.IsInf(payoutFlag, 0) { |
There was a problem hiding this comment.
When a user passes --payout 0, this guard accepts it, but every subsequent payout branch is gated by payoutFlag > 0, so the flag is silently treated as absent and the commands classify rows using recorded revenue instead of zero revenue. This can omit every converting paid row from report losers even though all of them lose money at the requested valuation. Distinguish an explicitly supplied zero from the default and either apply the zero valuation or reject it before any request with the categorized validation error and actionable hint required for CLI input errors.
AGENTS.md reference: AGENTS.md:L54-L56
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 63103e1. An explicit --payout 0 (cmd.Flags().Changed("payout") with a value ≤ 0) is now refused before any request. It gets the same "must be more than 0" error, exit code 1 and the actionable hint. Leaving the flag unset still means no payout. I refused it rather than applying zero valuation because a payout of 0 makes every converting row with spend a loser, which no average order value asks for, and the message already promised more than 0. Test: TestTriageRefusesABadPayoutBeforeAnyRequest adds 0 for both commands.
Review on #198 (Codex): a finite but huge --payout (1e308) times a row's conversions overflows to +Inf, which JSON can't encode, so the rows came out empty with the command still exiting 0. --payout is capped at a billion per conversion, far past any real order value, which keeps conversions x payout finite for any count a report holds. Refused before any request with exit code 1 and the same hint. TestTriageRefusesABadPayoutBeforeAnyRequest covers 1e308 and 1000000000.01 on both commands, each refusal with its own expected message. Claude-Session: https://claude.ai/code/session_01MxDwKTKzyFMjLzVfQDoRdQ
Review on #198 (Codex): --payout 0 passed validation, but every payout branch needs a positive value, so it silently read as no --payout (recorded revenue) instead of zero revenue per conversion. An explicit 0 is now refused with the same "must be more than 0" error, exit code 1 and hint; the flag left unset is unchanged. TestTriageRefusesABadPayoutBeforeAnyRequest adds 0 for both commands. Claude-Session: https://claude.ai/code/session_01MxDwKTKzyFMjLzVfQDoRdQ
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 116e58a656
ℹ️ 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".
| if payoutFlag > 0 { | ||
| // The stated revenue per conversion values every row's sales, so a row profitable at that payout is | ||
| // a winner (and one that isn't, a loser) whatever the campaign's recorded income says. | ||
| net = leads*payoutFlag - cost |
There was a problem hiding this comment.
For accumulate-mode campaigns or repeating goals, one click can have multiple counted conversions, but the classic report's total_leads remains a converted-click count because 202-config/DataEngine/ClickRollupSql.php:66 maps it from the boolean click_lead. Consequently, two purchases on one click are valued only once here, while firstTouchROI values both attribution ledger conversions, so total_net, CUT/SCALE classification, and first-touch ROI can contradict each other and drive incorrect triage. Use an actual conversion count for the classic calculation, or consistently define and calculate the override per converted click.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9c3867b, by counting every sale rather than defining the override per converted click.
With --payout, the check's attribution request now also carries compare_model_id set to the first active, non-recomputing last_touch model. Last-touch credit is one per sale on the converting click, so on the classic rows' own clicks (click cohort) compare_attributed_conversions counts their sales. That's the same ledger the first-touch ROI counts.
Each row the check read is re-valued with sales = max(that, total_leads) (sales is added to the row) and re-classified:
- Losers: a row that covers its break-even once every sale counts is dropped, and stderr says how many were.
- Winners: the check also reads the converting rows that lose money per converted click, and lists the ones that are profitable per sale.
Without a usable Last touch model, or with the check off or skipped, rows stay valued per converted click and stderr says so.
Tests: TestPayoutCountsEverySaleWithTheLastTouchModel: 10 sales on 5 converted clicks drops off losers and becomes a winner; a 6-sale row stays CUT at −140 with a $3.60 break-even.TestPayoutWithoutSalesCountsSaysSo: no Last touch model, the check off, and no--payout.
Help, COMMAND_CONTRACTS.md and the changelog describe it.
…edits), not converted clicks Review on #198 (Codex P1): the classic report's total_leads counts a converted click once (click_lead is a flag), so with --payout two sales on one click were valued once in total_net and the CUT/SCALE call, but twice in the first-touch ROI, which counts ledger conversions. - With --payout the check's attribution request also carries compare_model_id = the first active, non-recomputing last_touch model. Its credits are one per sale on the converting click, so on the classic rows' own clicks (click cohort) compare_attributed_conversions counts their sales. - Each row the check read is re-valued per sale (sales = max(that, total_leads), added to the row) and re-classified. Losers drops a row that covers its break-even once every sale counts, with a note; winners also reads the converting rows that lose money per converted click and lists those that are profitable per sale. - Without a usable Last touch model, or with the check off or skipped, rows stay valued per converted click and stderr says so. Tests: TestPayoutCountsEverySaleWithTheLastTouchModel (losers drops Repeat Buyers, 10 sales on 5 converted clicks; One-Off stays CUT on 6 sales at -140 and a $3.60 break-even; winners lists Repeat Buyers on its 10 sales), TestPayoutWithoutSalesCountsSaysSo (no Last touch model; check off; no --payout). payoutServer answers the model list by type. Claude-Session: https://claude.ai/code/session_01MxDwKTKzyFMjLzVfQDoRdQ
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c3867b675
ℹ️ 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".
| if countSales { | ||
| id, name, pending, err := activeModelOfType(c, "last_touch") |
There was a problem hiding this comment.
On a default account, which is guaranteed to have a Last touch model but not a First touch model (202-config/Attribution/DefaultModel.php:11-12), loadAttributionCheck returns at lines 321-327 for report winners before reaching this new Last-touch lookup. Consequently, winners --payout never revalues repeat sales on the standard setup and can omit sources that are profitable only because one converted click produced multiple sales; run the sales-counting portion independently of the optional First-touch closer check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 42e3455. With --payout, report winners no longer returns at the missing-First-touch-model branch. It keeps the closer-check note ("closer check skipped: no active First touch model…") and still sends the attribution request, with no model_id and compare_model_id set to the Last touch model, so each row's sales are counted. applyCloserCheck stays off because the check carries no first-touch model. Without --payout the early return is unchanged. Test: TestWinnersCountSalesWithoutAFirstTouchModel uses an empty First touch list and a Last touch model. The request carries only compare_model_id=1, and the repeat-sale source is SCALE on its 10 sales. Without --payout it makes no attribution request.
| case wantWinners && payoutFlag > 0 && leads > 0 && net < 0: | ||
| maybe = append(maybe, t) |
There was a problem hiding this comment.
When converted-click valuation is exactly break-even (leads * payout == cost), this strict net < 0 condition puts the row in neither picked nor maybe, so its Last-touch sales are never requested. For example, five converted clicks at a $100 payout against $500 cost are omitted from report winners even if those clicks actually produced six sales and $100 profit; include break-even converting rows among the candidates for sales revaluation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 42e3455. Winners now reads the sales of every converting row that isn't SCALE per converted click: a loss, exact break-even, or CUT by its break-even CPC (leads > 0, any non-SCALE bucket). That replaces the strict net < 0 condition. Test: TestWinnersCountSalesWithoutAFirstTouchModel includes Even: 5 converted clicks × $60 = $300 against $300 cost. Its key is requested, and it's listed as SCALE at +120 on its 7 sales.
…d break-even rows too Review on #198 (Codex): - P1: a default account has a Last touch model but no First touch model, and winners returned before the Last-touch lookup, so --payout never counted repeat sales there. With --payout, winners now reads the sales without a first-touch model (no model_id; the closer check stays off and stderr still says so). - P2: a converting row at exactly break-even per converted click was in neither picked nor maybe, so its sales were never read. Winners now reads every converting row that isn't SCALE per converted click (a loss, break-even, or CUT by its break-even CPC). Test: TestWinnersCountSalesWithoutAFirstTouchModel (no First touch model: compare_model_id only; Repeat Buyers, 10 sales, and Even, break-even per converted click and +120 on 7 sales, are both SCALE; without --payout no attribution request). Claude-Session: https://claude.ai/code/session_01MxDwKTKzyFMjLzVfQDoRdQ
# Conflicts: # changelogs.txt
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Why
p202 report loserslists CUT rows only: spend with zero conversions, or a CPC above break-even. Break-even came from--max-cpcor the payout of--aff_campaign_id. That filter also narrows the report to one campaign, which turns the attribution check (#193) off.So a traffic source that makes some sales at a loss, the exact case the starter check is for, was WATCH: never listed and never checked. On the course demo account, both flipped sources (ChatGPT Ads at −39% and Meta Prospecting at −53% under last touch) came back as an empty list. Course lesson B5 tells members to ask Claude Code for this report, so it needs to work there.
What changes
--payout <revenue per conversion>onreport losersandreport winners. Each row's break-even CPC is the payout × its own conversion rate, with no filter, so the attribution check runs.--max-cpc, then--payout, then the campaign payout from--aff_campaign_id.--payoutis refused before any request.losers --helpnow says that without a break-even, only zero-conversion spend is CUT.COMMAND_CONTRACTS.mdnotes that--payoutmakes noGET /campaigns/{id}request.changelogs.txthas the 1.9.76 entry.Checked on the demo account (Sep 3 to Oct 3, current master server)
--payout:{"data": []}.--payout 160, the two loss-making sources come back TEST:Tests
TestLosersPayoutFindsSourcesThatSellAtALosschecks both cases: no payout gives nothing and no attribution call;--payout 160gives both rows TEST, reads keys7,9, and never fetches a campaign payout.TestLosersRefusesANegativePayoutBeforeAnyRequestandTestClassifyPrefersMaxCPCOverPayout.go test -race ./...passes exceptTestConversionImportStopsAtAConnectionFailure, which fails the same way on unchanged master (3 of 3 runs). It's in conversion import, which fix: six server bugs (click_bot, stale fallback URL, breakdown tie-breaker, timeseries truncation, conversions duplicate/409, CLI NaN/encoding) #195 touches.golangci-lint runreports 0 issues.Merge notes
--payout.report_optimize.go, but onlytoFloatat the top, so the two don't overlap.After review
--payoutvalues every sale the commands report (1220e11).total_netbecomes conversions × payout − cost, for classification and output, sowinnerslists a row that's profitable at your payout even when the recorded income puts it at a loss. Each row says which payout it was valued at. The attribution check's first-touch ROI also values attributed conversions at the payout, instead of using recorded revenue.--payoutalso reads the Last touch model in the check's request; its credits count each row's sales. Each row the check reads is valued per sale:--payout 0is refused too. A negative, non-finite or over-1,000,000,000--payoutis refused before any request with exit code 1 and a hint. The test is table-driven over both commands.tests/fixtures/agent-eval/cases/triage.jsonhas two cases, a positive and its negative twin.report losers --payout.b5-losers.png) shows first-touch ROI, which is now valued at $160 a sale. Re-run the card's data once this merges.