Explain cost variance and root cause from the CLI - #64
Conversation
…e6125a/integration-0
|
Linked to Plane Work Item(s) References This comment was auto-generated by Plane |
![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: 8fa8ab4347
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }, nil | ||
| } | ||
|
|
||
| var timeUnits = map[string]float64{"second": 1, "minute": 60, "hour": 3600, "day": 86400, "week": 604800} |
There was a problem hiding this comment.
The cost contract permits month and year for cadence denominators and uptime quantities (pkg/contracts/cost/v1/validate.go:417-424), but neither is in this conversion map. Consequently, a valid paired uptime driver expressed as months in the forecast and years in the actual is rejected as having incompatible units at the check above, whereas the supported hour/minute case is normalized. Add the remaining canonical time units (with the repository's intended calendar basis) so equivalent valid inputs can be compared.
AGENTS.md reference: AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| if err := ctx.Err(); err != nil { | ||
| return nil, err | ||
| } | ||
| if report == nil || report.SchemaVersion != variancev1.SchemaVersion || (report.Status != "available" && report.Status != "unavailable") { |
There was a problem hiding this comment.
When profitctl explain reads a hand-edited or corrupt diff file, this gate accepts any report with the right version and status if its arithmetic reconciles, then copies unchecked fields into the output. For example, blank observation IDs, an empty window, and empty money currencies can pass with a zero forecast and reconciled actual/variance/residual, but the emitted explanation is rejected by schemas/cost-variance/v1/schema.json (which requires non-empty IDs, RFC3339 window timestamps, and three-letter currencies). Validate the complete Result contract before constructing the report.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
| q := d.Quantity.Value | ||
| per := d.UnitPrice.Per.Value | ||
| if scale := timeUnits[d.Quantity.Unit]; scale > 0 { | ||
| q *= scale | ||
| per *= scale |
There was a problem hiding this comment.
CostDriver.Per is the required cadence time denominator, but factors never reads it. Therefore the same cadence entered as 10 command/minute for the forecast and 600 command/hour for the actual is treated as a 60-fold quantity change, producing a false quantity contribution (and offsetting residual) even when both describe the same workload over the same window. Normalize the driver scale basis, or reject differing bases, before applying the decomposition.
AGENTS.md reference: AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| if d.Distribution.Floor != nil { | ||
| dist = math.Max(dist, *d.Distribution.Floor) |
There was a problem hiding this comment.
math.Max only clamps an unconstrained distribution mean; it does not calculate E[max(X, floor)]. For example, a valid normal variable driver with mean 1, standard deviation 10, and floor 0—used to mirror the simulator's per-sample non-negative clamp—gets a factor of 1 here even though the simulator's expected factor is about 4.51. This understates modeled variance for such workloads and incorrectly moves it into residual; use the truncated-distribution expectation (and analogous floor calculations for the other distributions).
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
| return nil, errors.New("missing_evidence must identify actual cost and name unavailable_reason when actual is unavailable") | ||
| } | ||
| } | ||
| out := &ExplainReport{SchemaVersion: report.SchemaVersion, Status: report.Status, Window: report.Window, ForecastObservationID: report.ForecastObservationID, ActualObservationID: report.ActualObservationID, UnavailableReason: report.UnavailableReason, Forecast: report.Forecast, Actual: report.Actual, VarianceAbsolute: report.AbsoluteVariance, VariancePercent: report.PercentageVariance, Residual: report.Residual, DriverContributions: []DriverContribution{}, MissingEvidence: report.MissingEvidence, Sources: report.Sources, Confidence: costv1.ConfidenceLow, ConfidenceScore: 0} |
There was a problem hiding this comment.
The diff result and explain report both emit profitctl.cost-variance/v1, but their required shapes are incompatible: Result uses absolute_variance, percentage_variance, and contributions, while ExplainReport uses variance_absolute, variance_percent, and driver_contributions. Because the explanation copies the result's version here, downstream consumers cannot route or validate a document by its declared schema version—each output is rejected by the other cost-variance/v1 schema. Give the explanation its own schema version/type, or retain a compatible envelope.
AGENTS.md reference: AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
1 participant
Compare forecast and actual costs, preserve measured totals and residuals, and rank evidenced driver contributions without asserting delivery causation. Add local
diffandexplaincommands plus versioned variance contracts.Integrates current main and fixes a reproduced round-trip defect: medium-confidence synthetic inputs were valid, but their diff contributions were rejected by
explain. Synthetic contribution confidence is now capped at low. The new regression failed before the fix and passes afterward.Validation on
8fa8ab43472238019657bba4af631970e5ef2c86:validation-61b1836c-8938-4681-b273-cdcb815ed62cispassed: all six validators passed, including full Go vet/tests, schema, economics, CLI workflows, cost standards, and security boundaries, with zero provider cost.Tracking: PCTL-17. Original Pi run
dc767eb795e6125aand candidates remain preserved in history. Owner authorized merge. Use a merge commit to preserve stacked PR #66's dependency. No external release or website deployment.