fix: validate Claude usage-limit relations #30

Merged
jercik merged 16 commits from feat/usage-ratio-calibration into main 2026-08-05 09:41:06 +00:00
Owner

Implement the empirical Claude usage-limit calibration and make the estimator validate cross-meter relations safely.

  • Claude Max 20x: record the empirical weekly = 6 × 5h calibration (this is not a claim about Max 5x). Because the bundled Max 20x fable readings jointly conflict with that ratio, the estimator intentionally falls back to measured bounds at the default tolerance; the shipped weekly headline therefore widens until the inputs are reconciled.
  • Claude Team Premium/VC: record the empirical weekly = 8 × 5h calibration.
  • Treat worth entries as bidirectional interval constraints with explicit relative tolerance (the empirical ratios use ±2%), solve each connected component to a fixed point independently for every globally surviving display-rounding mode, and union only mode-consistent results. This is independent of JSON key order and preserves disjoint rounding segments.
  • Successful relation headlines name their component and print independent edge implications; conflicts and non-convergence fall back to measured bounds with explicit diagnostics.
  • Carry optional plan cost-basis provenance into monthly-equivalent output; the Team Premium working price is marked annual per-seat (unverified).
  • Document the Prometheus ramp provenance and preserve raw measurements until their provenance is reconciled. The bundled joint conflict is tolerance-sensitive: it holds through about 0.35% USD tolerance and flips near 0.4%, so the tighter high-tolerance result is not stronger evidence.

Validation: node --check skills/usage-limit-estimator/scripts/estimate.ts; estimator runs at the documented tolerance sweeps; node-test and commit-msg CI pass.

Implement the empirical Claude usage-limit calibration and make the estimator validate cross-meter relations safely. - Claude Max 20x: record the empirical `weekly = 6 × 5h` calibration (this is not a claim about Max 5x). Because the bundled Max 20x fable readings jointly conflict with that ratio, the estimator intentionally falls back to measured bounds at the default tolerance; the shipped `weekly` headline therefore widens until the inputs are reconciled. - Claude Team Premium/VC: record the empirical `weekly = 8 × 5h` calibration. - Treat `worth` entries as bidirectional interval constraints with explicit relative tolerance (the empirical ratios use ±2%), solve each connected component to a fixed point independently for every globally surviving display-rounding mode, and union only mode-consistent results. This is independent of JSON key order and preserves disjoint rounding segments. - Successful relation headlines name their component and print independent edge implications; conflicts and non-convergence fall back to measured bounds with explicit diagnostics. - Carry optional plan cost-basis provenance into monthly-equivalent output; the Team Premium working price is marked annual per-seat (unverified). - Document the Prometheus ramp provenance and preserve raw measurements until their provenance is reconciled. The bundled joint conflict is tolerance-sensitive: it holds through about 0.35% USD tolerance and flips near 0.4%, so the tighter high-tolerance result is not stronger evidence. Validation: `node --check skills/usage-limit-estimator/scripts/estimate.ts`; estimator runs at the documented tolerance sweeps; node-test and commit-msg CI pass.
docs: record calibrated Claude usage ratios
Some checks failed
Node tests / node:test (pull_request) Successful in 10s
commit-msg / commitlint (pull_request) Successful in 20s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 1m50s
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 1m5s
PR Review / forgejo-review-approach-3 generator (pull_request_target) Failing after 1m5s
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Failing after 1m5s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Successful in 2m47s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 3m50s
PR Review / Dispatch and observe exact review writers (pull_request_target) Has been cancelled
016f1fbb65
forgejo-actions left a comment

Approach review: Recording the calibrations as worth relations in plans.json is the right home for them — the mechanism already exists and propagates both ways. The problem is that this is the first plan to carry two relation edges, and neither estimate.ts nor the data is ready for that.

relationFor scans relationDefs and returns on the first informative match, so a meter touched by two relations uses one of them and silently ignores the other. weekly is now touched by both weekly = 6 × 5h and weekly-fable = 0.5 × weekly, and with the current measurements.jsonl the two disagree outright:

  • with weekly = 6 × 5h applied: weekly = $1049.48–$1067.75
  • with weekly-fable = 0.5 × weekly applied: weekly = $854.30–$978.77

Those intervals are disjoint, so the calibration contradicts the existing fable measurements. The estimator reports neither a CONFLICT nor a caveat; it just prints the first one. It is also order-dependent — moving the weekly-fable key above weekly in plans.json flips the reported estimate between the two figures above with no other change. Propagation is non-transitive for the same reason (measuredHull reads the measured hull, never the relation-combined one), so weekly-fable keeps its $427.15–$489.39 measured bound even though the new ratio implies $524.74–$533.88 for it.

The design fix is to treat worth as a constraint graph rather than a lookup: intersect every relation-implied interval for a meter (iterating to a fixed point so 5h → weekly → weekly-fable propagates), and report a CONFLICT when the intersection is empty — which is exactly the signal this data should be producing. That machinery is what makes the SKILL.md rule "retain the ratio and revise the weaker dollar estimate" enforceable; as written the rule also doesn't say what to do when two encoded ratios are mutually inconsistent, which is the case this PR creates.

Until the resolver combines relations, landing the 6× edge converts a detectable contradiction into a silently order-dependent number, so I'd resolve the data conflict (or teach the script to surface it) in the same change.

Approach review by Claude Code Opus (opus)

**Approach review:** Recording the calibrations as `worth` relations in `plans.json` is the right home for them — the mechanism already exists and propagates both ways. The problem is that this is the first plan to carry *two* relation edges, and neither `estimate.ts` nor the data is ready for that. `relationFor` scans `relationDefs` and **returns on the first informative match**, so a meter touched by two relations uses one of them and silently ignores the other. `weekly` is now touched by both `weekly = 6 × 5h` and `weekly-fable = 0.5 × weekly`, and with the current `measurements.jsonl` the two disagree outright: - with `weekly = 6 × 5h` applied: `weekly` = $1049.48–$1067.75 - with `weekly-fable = 0.5 × weekly` applied: `weekly` = $854.30–$978.77 Those intervals are disjoint, so the calibration contradicts the existing fable measurements. The estimator reports neither a CONFLICT nor a caveat; it just prints the first one. It is also order-dependent — moving the `weekly-fable` key above `weekly` in `plans.json` flips the reported estimate between the two figures above with no other change. Propagation is non-transitive for the same reason (`measuredHull` reads the measured hull, never the relation-combined one), so `weekly-fable` keeps its $427.15–$489.39 measured bound even though the new ratio implies $524.74–$533.88 for it. The design fix is to treat `worth` as a constraint graph rather than a lookup: intersect *every* relation-implied interval for a meter (iterating to a fixed point so 5h → weekly → weekly-fable propagates), and report a CONFLICT when the intersection is empty — which is exactly the signal this data should be producing. That machinery is what makes the SKILL.md rule "retain the ratio and revise the weaker dollar estimate" enforceable; as written the rule also doesn't say what to do when two encoded ratios are mutually inconsistent, which is the case this PR creates. Until the resolver combines relations, landing the 6× edge converts a detectable contradiction into a silently order-dependent number, so I'd resolve the data conflict (or teach the script to surface it) in the same change. _Approach review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6IjAxNmYxZmJiNjVmNGU5ZTU5Zjg3YzVjMTFhMmVlNmI1MDk2NTBlOTEiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctYXBwcm9hY2gtc21hcnQtMSIsInNvdXJjZVdvcmtmbG93UnVuSWQiOiIxNjExMyIsInNvdXJjZUdlbmVyYXRvckF0dGVtcHQiOiIxIiwicmVzdWx0SWQiOiIxZGFhZDNlMS02MmQ0LTRiN2ItOGU3My04YTg0Mzc1NTgwMmEifQ== -->
@ -7,1 +7,3 @@
"weekly": { "periodHours": 168 },
"weekly": {
"periodHours": 168,
"worth": { "fraction": 6, "of": "5h" }

This makes weekly the target of two relations (weekly = 6 × 5h here, weekly-fable = 0.5 × weekly below), which relationFor in scripts/estimate.ts cannot represent — it returns the first informative match and drops the rest.

Running the estimator on the current measurements.jsonl:

  • as committed: weekly = $1049.48–$1067.75 — combined with relation weekly = 6 × 5h
  • with only the fable relation: weekly = $854.30–$978.77 — combined with relation weekly-fable = 0.5 × weekly

Disjoint intervals, no CONFLICT reported, and the choice between them depends only on JSON key order (swapping the weekly and weekly-fable keys flips the output). Prefer making the resolver intersect all relation-implied intervals for a meter and iterate to a fixed point so constraints propagate along the 5h → weekly → weekly-fable chain, failing loudly when the intersection is empty. That turns this calibration into a checkable fact instead of an order-dependent override — and it will immediately flag that the 6× ratio and the existing fable measurements disagree.

This makes `weekly` the target of two relations (`weekly = 6 × 5h` here, `weekly-fable = 0.5 × weekly` below), which `relationFor` in `scripts/estimate.ts` cannot represent — it returns the first informative match and drops the rest. Running the estimator on the current `measurements.jsonl`: - as committed: `weekly` = $1049.48–$1067.75 — combined with relation `weekly = 6 × 5h` - with only the fable relation: `weekly` = $854.30–$978.77 — combined with relation `weekly-fable = 0.5 × weekly` Disjoint intervals, no CONFLICT reported, and the choice between them depends only on JSON key order (swapping the `weekly` and `weekly-fable` keys flips the output). Prefer making the resolver intersect all relation-implied intervals for a meter and iterate to a fixed point so constraints propagate along the 5h → weekly → weekly-fable chain, failing loudly when the intersection is empty. That turns this calibration into a checkable fact instead of an order-dependent override — and it will immediately flag that the 6× ratio and the existing fable measurements disagree.
Author
Owner

Fixed in 54683ba: worth relations are now bidirectional constraints intersected to a fixed point, so chained edges are order-independent and conflicts are surfaced.

<!-- gh-feedback:reply-to:41155 --> Fixed in 54683ba: worth relations are now bidirectional constraints intersected to a fixed point, so chained edges are order-independent and conflicts are surfaced.
jercik marked this conversation as resolved
@ -8,0 +21,4 @@
"5h": {},
"weekly": {
"periodHours": 168,
"worth": { "fraction": 8, "of": "5h" }

This plan has no rows in measurements.jsonl, and relationFor skips a relation whose meters have no measured hull, so the 8× calibration is inert — it produces no output and nothing validates it. If the supporting Prometheus ramps exist, appending them as measurement lines under plan: "claude-team-premium" would let the estimator derive and cross-check the ratio instead of taking it on faith; otherwise the ratio is documentation that the tooling silently ignores, and SKILL.md's claim that ramps support it isn't reproducible from the repo.

This plan has no rows in `measurements.jsonl`, and `relationFor` skips a relation whose meters have no measured hull, so the 8× calibration is inert — it produces no output and nothing validates it. If the supporting Prometheus ramps exist, appending them as measurement lines under `plan: "claude-team-premium"` would let the estimator derive and cross-check the ratio instead of taking it on faith; otherwise the ratio is documentation that the tooling silently ignores, and SKILL.md's claim that ramps support it isn't reproducible from the repo.
Author
Owner

Fixed in 54683ba: relations with missing measurement series are reported explicitly as unvalidated instead of silently being ignored. The external Team Premium Prometheus ramps are documented as provenance, not fabricated into measurements.

<!-- gh-feedback:reply-to:41156 --> Fixed in 54683ba: relations with missing measurement series are reported explicitly as unvalidated instead of silently being ignored. The external Team Premium Prometheus ramps are documented as provenance, not fabricated into measurements.
jercik marked this conversation as resolved
forgejo-actions left a comment

Summary: Found 1 high and 2 medium issues.

The new weekly = 6 × 5h relation for claude-max-20x is not consistent with the measurement database already in the repo: chained through the existing weekly-fable = 0.5 × weekly, the two constraints on weekly are disjoint ($1049.48–$1067.75 vs $854.30–$978.77). The estimator does not report this, because relation propagation is single-hop and first-match-wins, so the change also makes the reported weekly figure depend on key order in plans.json. Verified by running scripts/estimate.ts on the working tree at --usd-tolerance 0, 0.25 and 0.5, and against HEAD~1's plans.json.

Code review by Claude Code Opus (opus)

**Summary:** Found 1 high and 2 medium issues. The new `weekly = 6 × 5h` relation for `claude-max-20x` is not consistent with the measurement database already in the repo: chained through the existing `weekly-fable = 0.5 × weekly`, the two constraints on `weekly` are disjoint ($1049.48–$1067.75 vs $854.30–$978.77). The estimator does not report this, because relation propagation is single-hop and first-match-wins, so the change also makes the reported `weekly` figure depend on key order in `plans.json`. Verified by running `scripts/estimate.ts` on the working tree at `--usd-tolerance` 0, 0.25 and 0.5, and against `HEAD~1`'s `plans.json`. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6IjAxNmYxZmJiNjVmNGU5ZTU5Zjg3YzVjMTFhMmVlNmI1MDk2NTBlOTEiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE2MTEzIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjM5NDY1NWQzLTQxODktNGFiOS04MDUwLWY2MWE3Y2NhMGE5ZCJ9 -->
@ -32,1 +32,3 @@
`plans.json` describes each plan: its `monthlyCostUsd` and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without one — like `5h`, a throttle whose rate cap is independent of the weekly quotas — deliberately gets no monthly line; don't add a period to one. A meter's `worth: {"fraction": 0.5, "of": "weekly"}` declares a known cross-meter fact (its total is that fraction of a sibling meter's total, e.g. the fable meter is worth half the general weekly limit); the script propagates estimates through it in both directions automatically.
`plans.json` describes each plan: its `monthlyCostUsd` and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without one — like `5h`, a throttle whose rate cap is independent of the weekly quotas — deliberately gets no monthly line; don't add a period to one. A meter's `worth: {"fraction": 0.5, "of": "weekly"}` declares a cross-meter ratio (its total is that fraction of a sibling meter's total; for example, the fable meter is half the general weekly limit, while a plan can express `weekly = 6 × 5h` with `fraction: 6`); the script propagates estimates through it in both directions automatically.
The Claude ratio calibrations are empirical cross-meter facts: long, monotonically increasing Prometheus ramps support approximately `weekly = 6 × 5h` for the regular Max accounts and `weekly = 8 × 5h` for the Team Premium/VC account. Aggregating the whole ramps makes the two endpoint rounding errors small relative to the movement, so these relative ratios are better constrained than the absolute API-dollar totals. When an absolute dollar estimate conflicts with one of these calibrated ratios, retain the ratio and revise the weaker dollar estimate; the capacity contract and `plans.json` should stay consistent.

🟡 Medium: This guidance conflicts with the interpretation rules further down the same file and rests on evidence the reader cannot reach.

  • Line 58 tells the agent that a relation CONFLICT "means the relation and the measurements cannot both be right — surface it rather than smoothing over it", while this paragraph tells it to "retain the ratio and revise the weaker dollar estimate". As written the two rules give opposite instructions for exactly the situation this PR creates, and the second one invites editing recorded measurements to fit a constant.
  • "Prometheus ramps" and "the capacity contract" appear nowhere else in this repository (only unrelated Prometheus metrics in skills/fastify-zod-reference/rules/deployment.md), and the ramps are not present in resources/measurements.jsonl. An agent asked to keep plans.json "consistent with the capacity contract" has no artifact to consult and no way to re-derive 6 or 8.

Defining both terms — or, better, appending the ramp windows as measurement lines so the ratios follow from the database — would make the calibration auditable and remove the conflicting instruction.

🟡 **Medium:** This guidance conflicts with the interpretation rules further down the same file and rests on evidence the reader cannot reach. - Line 58 tells the agent that a relation `CONFLICT` "means the relation and the measurements cannot both be right — surface it rather than smoothing over it", while this paragraph tells it to "retain the ratio and revise the weaker dollar estimate". As written the two rules give opposite instructions for exactly the situation this PR creates, and the second one invites editing recorded measurements to fit a constant. - "Prometheus ramps" and "the capacity contract" appear nowhere else in this repository (only unrelated Prometheus *metrics* in `skills/fastify-zod-reference/rules/deployment.md`), and the ramps are not present in `resources/measurements.jsonl`. An agent asked to keep `plans.json` "consistent with the capacity contract" has no artifact to consult and no way to re-derive 6 or 8. Defining both terms — or, better, appending the ramp windows as measurement lines so the ratios follow from the database — would make the calibration auditable and remove the conflicting instruction.
Author
Owner

Fixed in 54683ba: the skill defines Prometheus ramps and the cluster capacity contract, distinguishes external calibration evidence from recorded measurements, and instructs agents to surface conflicts before revising absolute estimates.

<!-- gh-feedback:reply-to:41174 --> Fixed in 54683ba: the skill defines Prometheus ramps and the cluster capacity contract, distinguishes external calibration evidence from recorded measurements, and instructs agents to surface conflicts before revising absolute estimates.
jercik marked this conversation as resolved
@ -7,1 +7,3 @@
"weekly": { "periodHours": 168 },
"weekly": {
"periodHours": 168,
"worth": { "fraction": 6, "of": "5h" }

🟠 High: This ratio contradicts the measurements already in resources/measurements.jsonl, and the contradiction is silently swallowed rather than reported.

Running node scripts/estimate.ts on the working tree:

  • 5h is measured at $174.91–$177.96, so weekly = 6 × 5h implies $1049.48–$1067.75.
  • weekly-fable is measured at $427.15–$489.39, so the pre-existing weekly-fable = 0.5 × weekly implies weekly$978.77.

Those intervals are disjoint — no value of weekly satisfies both relations plus the recorded data. The gap survives the sensitivity sweep the skill prescribes: at --usd-tolerance 0 the two implied ranges are $1052.11–$1065.09 vs ≤ $976.34, and at 0.5 they are $1019.30–$1070.42 vs ≤ $981.22. Solving the data directly gives weekly ≈ 2 × weekly-fable ≈ 5.2 × 5h, not 6.

No CONFLICT line is printed because relationFor (scripts/estimate.ts:363) returns the first informative relation and stops, so weekly is combined with 6 × 5h and the fable relation is never checked against it. The visible effect is that the headline for claude-max-20x / weekly moved from $854.30–$978.77 (HEAD~1) to $1049.48–$1067.75 with no indication that a declared relation was dropped.

Either record the ramp measurements that support fraction: 6 as measurement lines so the series can be re-derived, or reconcile the constant with the existing data before landing it.

🟠 **High:** This ratio contradicts the measurements already in `resources/measurements.jsonl`, and the contradiction is silently swallowed rather than reported. Running `node scripts/estimate.ts` on the working tree: - `5h` is measured at `$174.91–$177.96`, so `weekly = 6 × 5h` implies `$1049.48–$1067.75`. - `weekly-fable` is measured at `$427.15–$489.39`, so the pre-existing `weekly-fable = 0.5 × weekly` implies `weekly` ≤ `$978.77`. Those intervals are disjoint — no value of `weekly` satisfies both relations plus the recorded data. The gap survives the sensitivity sweep the skill prescribes: at `--usd-tolerance 0` the two implied ranges are `$1052.11–$1065.09` vs ≤ `$976.34`, and at `0.5` they are `$1019.30–$1070.42` vs ≤ `$981.22`. Solving the data directly gives `weekly ≈ 2 × weekly-fable ≈ 5.2 × 5h`, not 6. No `CONFLICT` line is printed because `relationFor` (scripts/estimate.ts:363) returns the first informative relation and stops, so `weekly` is combined with `6 × 5h` and the fable relation is never checked against it. The visible effect is that the headline for `claude-max-20x / weekly` moved from `$854.30–$978.77` (HEAD~1) to `$1049.48–$1067.75` with no indication that a declared relation was dropped. Either record the ramp measurements that support `fraction: 6` as measurement lines so the series can be re-derived, or reconcile the constant with the existing data before landing it.
Author
Owner

Fixed in 54683ba: the estimator now reports the disjoint 6×/fable intervals as CONFLICT at tolerances 0, 0.25, and 0.5 rather than silently selecting the first relation. Recorded measurements remain unchanged pending provenance reconciliation.

<!-- gh-feedback:reply-to:41172 --> Fixed in 54683ba: the estimator now reports the disjoint 6×/fable intervals as CONFLICT at tolerances 0, 0.25, and 0.5 rather than silently selecting the first relation. Recorded measurements remain unchanged pending provenance reconciliation.
jercik marked this conversation as resolved
@ -8,0 +10,4 @@
},
"weekly-fable": {
"periodHours": 168,
"worth": { "fraction": 0.5, "of": "weekly" }

🟡 Medium: weekly is now the target of two relations (weekly = 6 × 5h as the meter, weekly-fable = 0.5 × weekly as the of), but relationFor (scripts/estimate.ts:363-379) iterates relationDefs in Object.entries(p.meters) order and returns the first informative match. The reported estimate therefore depends on JSON key order: with the current ordering claude-max-20x / weekly reports $1049.48–$1067.75 — combined with relation weekly = 6 × 5h; moving the weekly-fable key above weekly (same data, same constants) reports $854.30–$978.77 — combined with relation weekly-fable = 0.5 × weekly, a ~15% swing, and neither run flags a conflict.

Intersecting all relations that touch a meter before deciding between "tightens" and CONFLICT would make the order irrelevant and surface the contradiction described above instead of picking a winner by key position.

🟡 **Medium:** `weekly` is now the target of two relations (`weekly = 6 × 5h` as the meter, `weekly-fable = 0.5 × weekly` as the `of`), but `relationFor` (scripts/estimate.ts:363-379) iterates `relationDefs` in `Object.entries(p.meters)` order and returns the first informative match. The reported estimate therefore depends on JSON key order: with the current ordering `claude-max-20x / weekly` reports `$1049.48–$1067.75 — combined with relation weekly = 6 × 5h`; moving the `weekly-fable` key above `weekly` (same data, same constants) reports `$854.30–$978.77 — combined with relation weekly-fable = 0.5 × weekly`, a ~15% swing, and neither run flags a conflict. Intersecting *all* relations that touch a meter before deciding between "tightens" and `CONFLICT` would make the order irrelevant and surface the contradiction described above instead of picking a winner by key position.
Author
Owner

Fixed in 54683ba: all relation edges are intersected to a fixed point, eliminating JSON-key-order dependence and exposing conflicts.

<!-- gh-feedback:reply-to:41173 --> Fixed in 54683ba: all relation edges are intersected to a fixed point, eliminating JSON-key-order dependence and exposing conflicts.
jercik marked this conversation as resolved
docs: identify the Max 20x calibration
Some checks failed
Node tests / node:test (pull_request) Successful in 9s
commit-msg / commitlint (pull_request) Successful in 12s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 3m5s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 46s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 4m22s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 4m39s
bebf3b5b35
forgejo-actions left a comment

Summary: Found 1 high, 2 medium, and 1 low issue, plus 1 note.

The headline problem is data-level: the new weekly = 6 × 5h relation for claude-max-20x is jointly inconsistent with the recorded weekly-fable measurements through the pre-existing weekly-fable = 0.5 × weekly relation, and the estimator cannot surface that conflict because relation-implied bounds do not chain. Verified by running node skills/usage-limit-estimator/scripts/estimate.ts at --usd-tolerance 0, 0.25, 0.5, 1, and 2 — the two implied intervals are disjoint at every one of them, and no CONFLICT line is printed.

The remaining comments cover the first-match relation lookup, a self-contradiction left in the reworded paragraph, and an unactionable instruction referencing artifacts that do not exist in this repo.

Code review by Claude Code Opus (opus)

**Summary:** Found 1 high, 2 medium, and 1 low issue, plus 1 note. The headline problem is data-level: the new `weekly = 6 × 5h` relation for `claude-max-20x` is jointly inconsistent with the recorded `weekly-fable` measurements through the pre-existing `weekly-fable = 0.5 × weekly` relation, and the estimator cannot surface that conflict because relation-implied bounds do not chain. Verified by running `node skills/usage-limit-estimator/scripts/estimate.ts` at `--usd-tolerance` 0, 0.25, 0.5, 1, and 2 — the two implied intervals are disjoint at every one of them, and no CONFLICT line is printed. The remaining comments cover the first-match relation lookup, a self-contradiction left in the reworded paragraph, and an unactionable instruction referencing artifacts that do not exist in this repo. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6ImJlYmYzYjViMzU5YzY5NTkwN2Q5NzE2MTRjMWYwYTg0NjcyYzE5NjYiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE2MTYwIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6ImM4NmJiZDkxLWRlYTItNDdkYi1iMWE3LWRhNGExZDRhZjVmNSJ9 -->
@ -30,3 +30,3 @@
One spending window usually moves several meters at once; record a separate line per meter, sharing `usd`, `at`, and `session`. `note` stays free-text for provenance only — anything the estimate depends on belongs in the typed fields.
`plans.json` describes each plan: its `monthlyCostUsd` and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without one — like `5h`, a throttle whose rate cap is independent of the weekly quotas — deliberately gets no monthly line; don't add a period to one. A meter's `worth: {"fraction": 0.5, "of": "weekly"}` declares a known cross-meter fact (its total is that fraction of a sibling meter's total, e.g. the fable meter is worth half the general weekly limit); the script propagates estimates through it in both directions automatically.
`plans.json` describes each plan: its `monthlyCostUsd` and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without one — like `5h`, a throttle whose rate cap is independent of the weekly quotas — deliberately gets no monthly line; don't add a period to one. A meter's `worth: {"fraction": 0.5, "of": "weekly"}` declares a cross-meter ratio (its total is that fraction of a sibling meter's total; for example, the fable meter is half the general weekly limit, while a plan can express `weekly = 6 × 5h` with `fraction: 6`); the script propagates estimates through it in both directions automatically.

🟡 Medium: This sentence still describes 5h as "a throttle whose rate cap is independent of the weekly quotas" while the same paragraph now teaches weekly = 6 × 5h — a relation that asserts precisely the dependency the earlier clause denies. An agent reading this cannot tell whether declaring a worth between 5h and weekly is legitimate. Reword the independence claim to what it actually means (no periodHours, hence no monthly line) so it stops contradicting the new example.

🟡 **Medium:** This sentence still describes `5h` as "a throttle whose rate cap is independent of the weekly quotas" while the same paragraph now teaches `weekly = 6 × 5h` — a relation that asserts precisely the dependency the earlier clause denies. An agent reading this cannot tell whether declaring a `worth` between `5h` and `weekly` is legitimate. Reword the independence claim to what it actually means (no `periodHours`, hence no monthly line) so it stops contradicting the new example.
Author
Owner

Fixed in b538e75: the skill now says that a meter without periodHours has no monthly equivalent while it may still participate in worth relations.

<!-- gh-feedback:reply-to:41290 --> Fixed in b538e75: the skill now says that a meter without periodHours has no monthly equivalent while it may still participate in worth relations.
jercik marked this conversation as resolved
@ -32,1 +32,3 @@
`plans.json` describes each plan: its `monthlyCostUsd` and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without one — like `5h`, a throttle whose rate cap is independent of the weekly quotas — deliberately gets no monthly line; don't add a period to one. A meter's `worth: {"fraction": 0.5, "of": "weekly"}` declares a known cross-meter fact (its total is that fraction of a sibling meter's total, e.g. the fable meter is worth half the general weekly limit); the script propagates estimates through it in both directions automatically.
`plans.json` describes each plan: its `monthlyCostUsd` and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without one — like `5h`, a throttle whose rate cap is independent of the weekly quotas — deliberately gets no monthly line; don't add a period to one. A meter's `worth: {"fraction": 0.5, "of": "weekly"}` declares a cross-meter ratio (its total is that fraction of a sibling meter's total; for example, the fable meter is half the general weekly limit, while a plan can express `weekly = 6 × 5h` with `fraction: 6`); the script propagates estimates through it in both directions automatically.
The Claude ratio calibrations are empirical cross-meter facts: long, monotonically increasing Prometheus ramps support approximately `weekly = 6 × 5h` for Claude Max 20x accounts (not Max 5x) and `weekly = 8 × 5h` for the Team Premium/VC account. Aggregating the whole ramps makes the two endpoint rounding errors small relative to the movement, so these relative ratios are better constrained than the absolute API-dollar totals. When an absolute dollar estimate conflicts with one of these calibrated ratios, retain the ratio and revise the weaker dollar estimate; the capacity contract and `plans.json` should stay consistent.

🟡 Medium: This paragraph is not actionable as written for the agent that loads the skill.

  • "Prometheus ramps" and "the capacity contract" have no referent anywhere in this repo (grep finds neither term outside this line), and neither appears in the canonical paths list at SKILL.md:12-16. The agent cannot check plans.json against an artifact it cannot locate.
  • "revise the weaker dollar estimate" has no target: dollar estimates are not stored, they are derived by scripts/estimate.ts from measurements.jsonl. The only way to "revise" one is to edit or drop recorded readings — which conflicts with SKILL.md:59 ("A contradiction is treated as a real change, not a typo — the inputs are hard to misread").

Either name the concrete file/action the agent should take, or state the ratios as provenance for plans.json without an instruction to act on them.

🟡 **Medium:** This paragraph is not actionable as written for the agent that loads the skill. - "Prometheus ramps" and "the capacity contract" have no referent anywhere in this repo (`grep` finds neither term outside this line), and neither appears in the canonical paths list at `SKILL.md:12-16`. The agent cannot check `plans.json` against an artifact it cannot locate. - "revise the weaker dollar estimate" has no target: dollar estimates are not stored, they are derived by `scripts/estimate.ts` from `measurements.jsonl`. The only way to "revise" one is to edit or drop recorded readings — which conflicts with `SKILL.md:59` ("A contradiction is treated as a real change, not a typo — the inputs are hard to misread"). Either name the concrete file/action the agent should take, or state the ratios as provenance for `plans.json` without an instruction to act on them.
Author
Owner

Fixed in 54683ba: the skill names the cluster capacity contract, identifies the external Prometheus-ramp provenance, and no longer instructs agents to edit derived measurements.

<!-- gh-feedback:reply-to:41291 --> Fixed in 54683ba: the skill names the cluster capacity contract, identifies the external Prometheus-ramp provenance, and no longer instructs agents to edit derived measurements.
jercik marked this conversation as resolved
@ -7,1 +7,3 @@
"weekly": { "periodHours": 168 },
"weekly": {
"periodHours": 168,
"worth": { "fraction": 6, "of": "5h" }

🟠 High: This calibration contradicts the measurements already in measurements.jsonl, and the contradiction is invisible in the estimator's output.

Running the estimator on this tree:

  • weekly headline (combined with the new relation): $1049.48–$1067.75
  • weekly-fable measured hull: $427.15–$489.39

The existing weekly-fable = 0.5 × weekly relation makes those mutually exclusive in both directions: the new weekly interval implies fable $524.74–$533.88 (above the measured hull), and the fable measurements imply weekly $854.30–$978.78 (below 6 × 5h's $1049.48–$1067.75). The gap survives --usd-tolerance 0, 0.5, 1, and 2, so it is not a tolerance artifact.

No CONFLICT line appears because relationFor compares each relation against the measured-alone hull of the sibling meter (measuredHull, scripts/estimate.ts:357), so a tightening on weekly never propagates down the 5h → weekly → weekly-fable chain. The result is a weekly estimate reported as 22.8×–23.2× the plan price that the skill's own fable data rules out, with no warning.

Reconcile before landing: either the 6 × ratio, the 0.5 × fable relation, or the fable readings (two of which carry start 7 inferred) is wrong. If the ratio is the trusted quantity per the new SKILL.md paragraph, say explicitly in the PR which fable input it invalidates.

🟠 **High:** This calibration contradicts the measurements already in `measurements.jsonl`, and the contradiction is invisible in the estimator's output. Running the estimator on this tree: - `weekly` headline (combined with the new relation): `$1049.48–$1067.75` - `weekly-fable` measured hull: `$427.15–$489.39` The existing `weekly-fable = 0.5 × weekly` relation makes those mutually exclusive in both directions: the new weekly interval implies fable `$524.74–$533.88` (above the measured hull), and the fable measurements imply weekly `$854.30–$978.78` (below `6 × 5h`'s `$1049.48–$1067.75`). The gap survives `--usd-tolerance` 0, 0.5, 1, and 2, so it is not a tolerance artifact. No `CONFLICT` line appears because `relationFor` compares each relation against the *measured-alone* hull of the sibling meter (`measuredHull`, `scripts/estimate.ts:357`), so a tightening on `weekly` never propagates down the `5h → weekly → weekly-fable` chain. The result is a weekly estimate reported as `22.8×–23.2×` the plan price that the skill's own fable data rules out, with no warning. Reconcile before landing: either the `6 ×` ratio, the `0.5 ×` fable relation, or the fable readings (two of which carry `start 7 inferred`) is wrong. If the ratio is the trusted quantity per the new SKILL.md paragraph, say explicitly in the PR which fable input it invalidates.
Author
Owner

Fixed in 54683ba: the fixed-point relation graph now emits CONFLICT for the disjoint 6×/fable constraints; recorded readings are left intact pending provenance reconciliation.

<!-- gh-feedback:reply-to:41288 --> Fixed in 54683ba: the fixed-point relation graph now emits CONFLICT for the disjoint 6×/fable constraints; recorded readings are left intact pending provenance reconciliation.
jercik marked this conversation as resolved
@ -8,0 +8,4 @@
"periodHours": 168,
"worth": { "fraction": 6, "of": "5h" }
},
"weekly-fable": {

🟡 Medium: With this block in place, weekly participates in two relations (weekly = 6 × 5h and weekly-fable = 0.5 × weekly), but relationFor (scripts/estimate.ts:363) returns the first informative definition in Object.entries order and stops. For the weekly series only the 6 × 5h relation is ever evaluated, so a CONFLICT arising from the fable relation is silently dropped, and which relation gets reported now depends on key ordering inside this file.

That undercuts the guarantee stated in SKILL.md:58 ("A relation CONFLICT means the relation and the measurements cannot both be right — surface it rather than smoothing over it"). Evaluate every relation touching a meter, intersect the non-conflicting implications, and report each conflict rather than returning on the first hit.

🟡 **Medium:** With this block in place, `weekly` participates in two relations (`weekly = 6 × 5h` and `weekly-fable = 0.5 × weekly`), but `relationFor` (`scripts/estimate.ts:363`) returns the *first* informative definition in `Object.entries` order and stops. For the `weekly` series only the `6 × 5h` relation is ever evaluated, so a `CONFLICT` arising from the fable relation is silently dropped, and which relation gets reported now depends on key ordering inside this file. That undercuts the guarantee stated in `SKILL.md:58` ("A relation CONFLICT means the relation and the measurements cannot both be right — surface it rather than smoothing over it"). Evaluate every relation touching a meter, intersect the non-conflicting implications, and report each conflict rather than returning on the first hit.
Author
Owner

Fixed in 54683ba: every relation touching a meter is evaluated and intersected, removing JSON-key-order dependence and surfacing conflicts.

<!-- gh-feedback:reply-to:41289 --> Fixed in 54683ba: every relation touching a meter is evaluated and intersected, removing JSON-key-order dependence and surfacing conflicts.
jercik marked this conversation as resolved
@ -8,0 +21,4 @@
"5h": {},
"weekly": {
"periodHours": 168,
"worth": { "fraction": 8, "of": "5h" }

ℹ️ Info: claude-team-premium has no lines in measurements.jsonl, so this ratio is inert today — the "estimator relation and JSON checks pass" validation in the PR description does not exercise it. Worth confirming that the account really exposes a weekly-fable meter under this plan name before the first measurement is recorded, since the registry check (scripts/estimate.ts:315) only fires once a measurement names the plan.

ℹ️ **Info:** `claude-team-premium` has no lines in `measurements.jsonl`, so this ratio is inert today — the "estimator relation and JSON checks pass" validation in the PR description does not exercise it. Worth confirming that the account really exposes a `weekly-fable` meter under this plan name before the first measurement is recorded, since the registry check (`scripts/estimate.ts:315`) only fires once a measurement names the plan.
jercik marked this conversation as resolved
fix: validate all usage estimate relations
Some checks failed
Node tests / node:test (pull_request) Successful in 9s
commit-msg / commitlint (pull_request) Successful in 27s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been cancelled
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been cancelled
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been cancelled
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Has been cancelled
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Has been cancelled
PR Review / Dispatch and observe exact review writers (pull_request_target) Has been cancelled
PR Review / Prepare immutable review tools (pull_request_target) Has been cancelled
54683bad3f
Author
Owner

Replying to review #9590\n\nAddressed in commit 54683ba: relation edges are now intersected bidirectionally to a fixed point, conflicts are explicit, and missing Team Premium series are reported as unvalidated.

> Replying to review #9590\n\nAddressed in commit 54683ba: relation edges are now intersected bidirectionally to a fixed point, conflicts are explicit, and missing Team Premium series are reported as unvalidated.
Author
Owner

Replying to review #9596\n\nAddressed in commit 54683ba: the estimator now exposes the disjoint ratio/measurement constraints as CONFLICT, removes first-match/order dependence, and the skill documents the external calibration provenance and conflict policy.

> Replying to review #9596\n\nAddressed in commit 54683ba: the estimator now exposes the disjoint ratio/measurement constraints as CONFLICT, removes first-match/order dependence, and the skill documents the external calibration provenance and conflict policy.
docs: clarify non-periodic usage meters
Some checks failed
Node tests / node:test (pull_request) Successful in 15s
commit-msg / commitlint (pull_request) Successful in 22s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 2m52s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 44s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 5m7s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 5m24s
b538e751db
forgejo-actions left a comment

Summary: Found 1 high, 1 medium, and 1 low issue in the new relation fixed-point solver. The plans.json calibrations and the SKILL.md prose are consistent with the implemented behaviour; the issues are all in estimate.ts.

The high-severity one is live with the exact data this PR adds: conflictEdges is keyed on relation text that omits the plan, and the new claude-team-premium plan declares a weekly-fable = 0.5 × weekly relation whose text is byte-identical to claude-max-20x's. Since claude-max-20x's copy currently conflicts against the committed measurements, the first claude-team-premium measurements recorded will be reported as CONFLICTS even when they agree perfectly with the declared ratio, and the valid tightening is discarded.

Verified by running node skills/usage-limit-estimator/scripts/estimate.ts against the committed data and against synthetic measurement sets; the reproductions are in the inline comments.

Code review by Claude Code Opus (opus)

**Summary:** Found 1 high, 1 medium, and 1 low issue in the new relation fixed-point solver. The `plans.json` calibrations and the SKILL.md prose are consistent with the implemented behaviour; the issues are all in `estimate.ts`. The high-severity one is live with the exact data this PR adds: `conflictEdges` is keyed on relation text that omits the plan, and the new `claude-team-premium` plan declares a `weekly-fable = 0.5 × weekly` relation whose text is byte-identical to `claude-max-20x`'s. Since `claude-max-20x`'s copy currently conflicts against the committed measurements, the first `claude-team-premium` measurements recorded will be reported as `CONFLICTS` even when they agree perfectly with the declared ratio, and the valid tightening is discarded. Verified by running `node skills/usage-limit-estimator/scripts/estimate.ts` against the committed data and against synthetic measurement sets; the reproductions are in the inline comments. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6ImI1MzhlNzUxZGIwZmRiYjExYWNiNTJiZmY2MTUwODY5ZDc3NjlhNjQiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE2MjUyIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6ImVkNzBlZWRlLTdiODgtNGFlYS1hN2NhLTliMDg2MTZkMGQxYiJ9 -->
@ -360,0 +380,4 @@
const relationText = (def: RelationDef) => `${def.meter} = ${def.fraction} × ${def.of}`;
const scaled = (iv: Interval, factor: number): Interval => ({ lo: iv.lo * factor, hi: iv.hi * factor });
for (let iteration = 0; iteration < relationDefs.length * 10 + 1; iteration++) {

🟡 Medium: The loop exits either on a fixed point (changed === false) or on exhausting relationDefs.length * 10 + 1 iterations, but the two outcomes are indistinguishable to the caller. On cap exhaustion the partially-narrowed, non-converged bounds are still published as the authoritative combined with relation headline, with no warning — so a contradictory relation graph silently yields an over-tight estimate instead of the CONFLICT SKILL.md promises.

parsePlans rejects only self-reference (w.of === name), so a cycle is expressible. A cycle whose factor product is not 1 is an outright contradiction (a = 1.001 × b and b = 1.001 × a force a = b = 0), but it narrows to empty only geometrically, and the cap cuts that short. With a two-meter cyc plan built exactly that way and one measurement per meter:

cyc / a
  current: $173.20–$240.32 (midpoint $206.76) — combined with relation a = 1.001 × b; b = 1.001 × a
    measured alone: $166.25–$250.63 (midpoint $208.44), 1 measurement in 1 session
cyc / b
  current: $173.38–$240.56 (midpoint $206.97) — combined with relation a = 1.001 × b; b = 1.001 × a
    measured alone: $166.25–$250.63 (midpoint $208.44), 1 measurement in 1 session

No CONFLICT, and the two symmetric meters get different answers — the signature of a truncated iteration rather than a fixed point.

Fix: track whether the loop ended by convergence and treat exhaustion as a hard failure or a conflict, e.g.

let converged = false;
for (let iteration = 0; iteration < relationDefs.length * 10 + 1; iteration++) {
  // ...
  if (!changed) { converged = true; break; }
}
if (!converged) fail("worth relations did not reach a fixed point — check plans.json for a contradictory relation cycle");

Validating the relation graph as acyclic in parsePlans would also work and fails earlier, at the config boundary.

🟡 **Medium:** The loop exits either on a fixed point (`changed === false`) or on exhausting `relationDefs.length * 10 + 1` iterations, but the two outcomes are indistinguishable to the caller. On cap exhaustion the partially-narrowed, non-converged bounds are still published as the authoritative `combined with relation` headline, with no warning — so a contradictory relation graph silently yields an over-tight estimate instead of the `CONFLICT` SKILL.md promises. `parsePlans` rejects only self-reference (`w.of === name`), so a cycle is expressible. A cycle whose factor product is not 1 is an outright contradiction (`a = 1.001 × b` and `b = 1.001 × a` force `a = b = 0`), but it narrows to empty only geometrically, and the cap cuts that short. With a two-meter `cyc` plan built exactly that way and one measurement per meter: ``` cyc / a current: $173.20–$240.32 (midpoint $206.76) — combined with relation a = 1.001 × b; b = 1.001 × a measured alone: $166.25–$250.63 (midpoint $208.44), 1 measurement in 1 session cyc / b current: $173.38–$240.56 (midpoint $206.97) — combined with relation a = 1.001 × b; b = 1.001 × a measured alone: $166.25–$250.63 (midpoint $208.44), 1 measurement in 1 session ``` No `CONFLICT`, and the two symmetric meters get different answers — the signature of a truncated iteration rather than a fixed point. Fix: track whether the loop ended by convergence and treat exhaustion as a hard failure or a conflict, e.g. ```ts let converged = false; for (let iteration = 0; iteration < relationDefs.length * 10 + 1; iteration++) { // ... if (!changed) { converged = true; break; } } if (!converged) fail("worth relations did not reach a fixed point — check plans.json for a contradictory relation cycle"); ``` Validating the relation graph as acyclic in `parsePlans` would also work and fails earlier, at the config boundary.
Author
Owner

Fixed in 21eeab0: relation conflicts are plan-qualified, non-convergent worth graphs fail instead of publishing partial bounds, and relationFor keeps TypeScript's non-null narrowing.

<!-- gh-feedback:reply-to:41364 --> Fixed in 21eeab0: relation conflicts are plan-qualified, non-convergent worth graphs fail instead of publishing partial bounds, and relationFor keeps TypeScript's non-null narrowing.
jercik marked this conversation as resolved
@ -377,0 +421,4 @@
if (!other) continue;
const text = relationText(def);
const implied = scaled(other, factor);
const conflict = conflictEdges.has(text) || conflictNodes.has(ownKey);

🟠 High: conflictEdges is keyed on relationText(def) (line 380), which renders only meter, fraction, and of — it omits def.plan — yet the set is global across all plans. plans.json now declares weekly-fable = 0.5 × weekly under both claude-max-20x and claude-team-premium, so those two distinct edges collapse to one key. A conflict in either plan then marks the other plan's relation as conflicting, which sets combined: null, drops the valid tightening, and prints a false CONFLICTS line.

This is live with the data this PR adds, because claude-max-20x's weekly-fable = 0.5 × weekly already conflicts against the committed measurements. Reproduced by appending two mutually consistent claude-team-premium measurements to resources/measurements.jsonl:

{"at":"2026-08-02T10:00:00+02:00","plan":"claude-team-premium","limit":"weekly","usd":10.00,"start":0,"end":1,"startExact":true,"session":"tp-1"}
{"at":"2026-08-02T10:00:00+02:00","plan":"claude-team-premium","limit":"weekly-fable","usd":10.00,"start":0,"end":2,"startExact":true,"session":"tp-1"}

With both plans registered:

claude-team-premium / weekly
  current: $665.00–$2005.00 (midpoint $1335.00), 1 measurement in 1 session
    relation weekly-fable = 0.5 × weekly CONFLICTS: implies $798.00–$1336.67
    monthly equivalent: $2891.56–$8718.17 (23.1×–69.7× the $125/mo price)

With only claude-team-premium registered in plans.json (identical measurements), the same relation is correctly non-conflicting and tightens the estimate:

claude-team-premium / weekly
  current: $798.00–$1336.67 (midpoint $1067.33) — combined with relation weekly-fable = 0.5 × weekly
    measured alone: $665.00–$2005.00 (midpoint $1335.00), 1 measurement in 1 session
    monthly equivalent: $3469.88–$5812.11 (27.8×–46.5× the $125/mo price)

Note the implied range ($798.00–$1336.67) sits strictly inside the measured hull, so there is no contradiction at all — the CONFLICTS verdict and the ~1.9× wider monthly band are pure cross-plan contamination.

Fix: give the edge set a plan-qualified identity and keep relationText for display only, mirroring how conflictNodes already uses nodeKey:

const edgeKey = (def: RelationDef) => JSON.stringify([def.plan, def.meter, def.of]);

then conflictEdges.add(edgeKey(def)) in the propagation loop and conflictEdges.has(edgeKey(def)) here.

🟠 **High:** `conflictEdges` is keyed on `relationText(def)` (line 380), which renders only `meter`, `fraction`, and `of` — it omits `def.plan` — yet the set is global across all plans. `plans.json` now declares `weekly-fable = 0.5 × weekly` under **both** `claude-max-20x` and `claude-team-premium`, so those two distinct edges collapse to one key. A conflict in either plan then marks the other plan's relation as conflicting, which sets `combined: null`, drops the valid tightening, and prints a false `CONFLICTS` line. This is live with the data this PR adds, because `claude-max-20x`'s `weekly-fable = 0.5 × weekly` already conflicts against the committed measurements. Reproduced by appending two mutually consistent `claude-team-premium` measurements to `resources/measurements.jsonl`: ``` {"at":"2026-08-02T10:00:00+02:00","plan":"claude-team-premium","limit":"weekly","usd":10.00,"start":0,"end":1,"startExact":true,"session":"tp-1"} {"at":"2026-08-02T10:00:00+02:00","plan":"claude-team-premium","limit":"weekly-fable","usd":10.00,"start":0,"end":2,"startExact":true,"session":"tp-1"} ``` With both plans registered: ``` claude-team-premium / weekly current: $665.00–$2005.00 (midpoint $1335.00), 1 measurement in 1 session relation weekly-fable = 0.5 × weekly CONFLICTS: implies $798.00–$1336.67 monthly equivalent: $2891.56–$8718.17 (23.1×–69.7× the $125/mo price) ``` With only `claude-team-premium` registered in `plans.json` (identical measurements), the same relation is correctly non-conflicting and tightens the estimate: ``` claude-team-premium / weekly current: $798.00–$1336.67 (midpoint $1067.33) — combined with relation weekly-fable = 0.5 × weekly measured alone: $665.00–$2005.00 (midpoint $1335.00), 1 measurement in 1 session monthly equivalent: $3469.88–$5812.11 (27.8×–46.5× the $125/mo price) ``` Note the implied range ($798.00–$1336.67) sits strictly inside the measured hull, so there is no contradiction at all — the `CONFLICTS` verdict and the ~1.9× wider monthly band are pure cross-plan contamination. Fix: give the edge set a plan-qualified identity and keep `relationText` for display only, mirroring how `conflictNodes` already uses `nodeKey`: ```ts const edgeKey = (def: RelationDef) => JSON.stringify([def.plan, def.meter, def.of]); ``` then `conflictEdges.add(edgeKey(def))` in the propagation loop and `conflictEdges.has(edgeKey(def))` here.
Author
Owner

Fixed in 21eeab0: relation conflicts are plan-qualified, non-convergent worth graphs fail instead of publishing partial bounds, and relationFor keeps TypeScript's non-null narrowing.

<!-- gh-feedback:reply-to:41363 --> Fixed in 21eeab0: relation conflicts are plan-qualified, non-convergent worth graphs fail instead of publishing partial bounds, and relationFor keeps TypeScript's non-null narrowing.
jercik marked this conversation as resolved
@ -379,0 +427,4 @@
if (constraints.length === 0) return undefined;
const conflict = constraints.some((constraint) => constraint.conflict);
const combined = conflict ? null : relationBounds.get(ownKey) ?? own;
if (!conflict && !tightens(combined, own)) return undefined;

🟢 Low: combined is inferred as Interval | null from the conditional on line 429, and TypeScript does not narrow it from the separate !conflict test here, so tightens(combined, own) is a type error:

estimate.ts(430,30): error TS2345: Argument of type 'Interval | null' is not assignable to parameter of type 'Interval'.

(Confirmed with tsc --strict; the other diagnostics in that run are just missing @types/node.) Runtime is unaffected — Node strips types without checking and combined is genuinely non-null on this path — but the file no longer typechecks. Restructuring so the non-conflict branch computes its own local keeps the narrowing intact:

if (conflict) return { constraints, combined: null, conflict };
const combined = relationBounds.get(ownKey) ?? own;
if (!tightens(combined, own)) return undefined;
return { constraints, combined, conflict };
🟢 **Low:** `combined` is inferred as `Interval | null` from the conditional on line 429, and TypeScript does not narrow it from the separate `!conflict` test here, so `tightens(combined, own)` is a type error: ``` estimate.ts(430,30): error TS2345: Argument of type 'Interval | null' is not assignable to parameter of type 'Interval'. ``` (Confirmed with `tsc --strict`; the other diagnostics in that run are just missing `@types/node`.) Runtime is unaffected — Node strips types without checking and `combined` is genuinely non-null on this path — but the file no longer typechecks. Restructuring so the non-conflict branch computes its own local keeps the narrowing intact: ```ts if (conflict) return { constraints, combined: null, conflict }; const combined = relationBounds.get(ownKey) ?? own; if (!tightens(combined, own)) return undefined; return { constraints, combined, conflict }; ```
Author
Owner

Fixed in 21eeab0: relation conflicts are plan-qualified, non-convergent worth graphs fail instead of publishing partial bounds, and relationFor keeps TypeScript's non-null narrowing.

<!-- gh-feedback:reply-to:41365 --> Fixed in 21eeab0: relation conflicts are plan-qualified, non-convergent worth graphs fail instead of publishing partial bounds, and relationFor keeps TypeScript's non-null narrowing.
jercik marked this conversation as resolved
fix: qualify relation conflicts and detect nonconvergence
Some checks failed
Node tests / node:test (pull_request) Successful in 9s
commit-msg / commitlint (pull_request) Successful in 16s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 1m28s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 38s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 6m37s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 6m57s
21eeab0e8c
Author
Owner

Replying to review #9648

The reported estimator issues were fixed in 54683ba and 21eeab0; all tracked feedback is now addressed.

> Replying to review #9648 The reported estimator issues were fixed in 54683ba and 21eeab0; all tracked feedback is now addressed.
forgejo-actions left a comment

Summary: Found 1 high, 2 medium, 1 low issue, plus 1 informational note.

The fixed-point rewrite is a real improvement over the first-match lookup, and the CONFLICT the new weekly = 6 × 5h calibration produces on the shipped dataset is genuine, not a bug. Two problems remain in the new relation code: a conflict is attributed only to the two endpoints of the failing edge, so neighbours in the same unsatisfiable component still print a confidently tightened estimate (reproduced with a 4-meter plan); and each edge's implied range is read from the post-fixed-point bounds, so which meter gets flagged still depends on meters key order — swapping the weekly / weekly-fable keys moves the marker from weekly-fable to 5h with identical measurements, contradicting the claim added to SKILL.md.

Verified by running scripts/estimate.ts against the committed data, against the pre-PR revision for comparison, and against synthetic plans; node --experimental-strip-types runs clean on Node 26.

Code review by Claude Code Opus (opus)

**Summary:** Found 1 high, 2 medium, 1 low issue, plus 1 informational note. The fixed-point rewrite is a real improvement over the first-match lookup, and the `CONFLICT` the new `weekly = 6 × 5h` calibration produces on the shipped dataset is genuine, not a bug. Two problems remain in the new relation code: a conflict is attributed only to the two endpoints of the failing edge, so neighbours in the same unsatisfiable component still print a confidently tightened estimate (reproduced with a 4-meter plan); and each edge's `implied` range is read from the post-fixed-point bounds, so which meter gets flagged still depends on `meters` key order — swapping the `weekly` / `weekly-fable` keys moves the marker from `weekly-fable` to `5h` with identical measurements, contradicting the claim added to SKILL.md. Verified by running `scripts/estimate.ts` against the committed data, against the pre-PR revision for comparison, and against synthetic plans; `node --experimental-strip-types` runs clean on Node 26. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6IjIxZWVhYjBlOGM4NTU2NDE4YjA2NzQyMTllODc5YzZjMDhiNjE0YzYiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE2MzUxIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6ImM3MjkyM2RhLWViNzMtNDk1NC05NDVlLWE1NTMwNzRjMjI4ZSJ9 -->
@ -30,3 +30,3 @@
One spending window usually moves several meters at once; record a separate line per meter, sharing `usd`, `at`, and `session`. `note` stays free-text for provenance only — anything the estimate depends on belongs in the typed fields.
`plans.json` describes each plan: its `monthlyCostUsd` and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without one — like `5h`, a throttle whose rate cap is independent of the weekly quotas — deliberately gets no monthly line; don't add a period to one. A meter's `worth: {"fraction": 0.5, "of": "weekly"}` declares a known cross-meter fact (its total is that fraction of a sibling meter's total, e.g. the fable meter is worth half the general weekly limit); the script propagates estimates through it in both directions automatically.
`plans.json` describes each plan: its `monthlyCostUsd` and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without `periodHours` — like `5h` — has no monthly equivalent in this estimator; it may still participate in a `worth` relation, so don't add a made-up period. A meter's `worth: {"fraction": 0.5, "of": "weekly"}` declares a cross-meter ratio (its total is that fraction of a sibling meter's total; for example, the fable meter is half the general weekly limit, while a plan can express `weekly = 6 × 5h` with `fraction: 6`). The estimator treats every `worth` entry as a bidirectional interval constraint, intersects all connected relations to a fixed point, and reports `CONFLICT` rather than choosing a relation based on JSON key order. A relation with no measured meters is reported as unvalidated instead of silently affecting an estimate.

🟡 Medium: Two claims in this paragraph do not match the implementation.

  1. reports CONFLICT rather than choosing a relation based on JSON key order — conflict attribution is still key-order dependent. Swapping the weekly and weekly-fable keys under claude-max-20x moves the CONFLICTS marker from weekly-fable to 5h with identical measurements. What is now order-independent is the fixed-point bound, not which meter is reported as conflicting.
  2. A relation with no measured meters is reported as unvalidatedunvalidatedRelations (scripts/estimate.ts:441) filters when either endpoint lacks measurements. The output bullet further down states this correctly (one or both meters have no measurements); this sentence should say the same thing.

Since the skill body is what a future agent reads as ground truth, narrow claim 1 to what the code guarantees (all applicable relations are intersected and every conflict is surfaced) until attribution is order-independent.

🟡 **Medium:** Two claims in this paragraph do not match the implementation. 1. `reports CONFLICT rather than choosing a relation based on JSON key order` — conflict *attribution* is still key-order dependent. Swapping the `weekly` and `weekly-fable` keys under `claude-max-20x` moves the `CONFLICTS` marker from `weekly-fable` to `5h` with identical measurements. What is now order-independent is the fixed-point bound, not which meter is reported as conflicting. 2. `A relation with no measured meters is reported as unvalidated` — `unvalidatedRelations` (scripts/estimate.ts:441) filters when *either* endpoint lacks measurements. The output bullet further down states this correctly (`one or both meters have no measurements`); this sentence should say the same thing. Since the skill body is what a future agent reads as ground truth, narrow claim 1 to what the code guarantees (all applicable relations are intersected and every conflict is surfaced) until attribution is order-independent.
Author
Owner

Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.

<!-- gh-feedback:reply-to:41491 --> Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.
jercik marked this conversation as resolved
@ -7,1 +7,3 @@
"weekly": { "periodHours": 168 },
"weekly": {
"periodHours": 168,
"worth": { "fraction": 6, "of": "5h" }

ℹ️ Info: On the committed measurements this relation makes the claude-max-20x graph unsatisfiable: 6 × 5h puts weekly in $1049.48–$1067.75, while weekly-fable = 0.5 × weekly caps it at $978.77. The default run therefore loses the previous relation-tightened estimate for weekly ($854.30–$978.77, 18.6×–21.3× the price) and falls back to the measured hull ($768.87–$1084.04, 16.7×–23.6×), with CONFLICT on both weekly and weekly-fable. That is what the PR body says should happen — noting it so the widened headline numbers read as a deliberate, visible cost rather than a regression. claude-team-premium has no measurements at all, so its 8 × ratio is recorded and correctly listed as unvalidated.

ℹ️ **Info:** On the committed measurements this relation makes the `claude-max-20x` graph unsatisfiable: `6 × 5h` puts `weekly` in `$1049.48–$1067.75`, while `weekly-fable = 0.5 × weekly` caps it at `$978.77`. The default run therefore loses the previous relation-tightened estimate for `weekly` (`$854.30–$978.77`, `18.6×–21.3×` the price) and falls back to the measured hull (`$768.87–$1084.04`, `16.7×–23.6×`), with `CONFLICT` on both `weekly` and `weekly-fable`. That is what the PR body says should happen — noting it so the widened headline numbers read as a deliberate, visible cost rather than a regression. `claude-team-premium` has no measurements at all, so its `8 ×` ratio is recorded and correctly listed as unvalidated.
jercik marked this conversation as resolved
@ -364,0 +397,4 @@
const nextOf = intersect(of, impliedOf);
if (!nextMeter || !nextOf) {
conflictEdges.add(edgeKey(def));
conflictNodes.add(meterKey);

🟠 High: A conflict marks only the two endpoints of the failing edge, so the rest of the connected component keeps the bounds it was tightened to before the contradiction surfaced and is then reported as a clean, tightened estimate.

Reproduced with a 4-meter plan declared in this key order — B = 1 × A, Z = 1 × B, A = 1 × C — and measured hulls A $100–$200, B $150–$300, C $100–$120, Z >$50. Sweep 1 tightens A and B to $150–$200 via the first edge, propagates that to Z via the second, then finds A = 1 × C empty. The run flags only A and C; B and Z print combined with relation … $150.00–$200.00 with no marker, even though no assignment satisfies all three relations at once.

Fix: once the fixed point is reached, expand the conflict to the whole connected component (union-find or BFS over relationDefs) and either mark those nodes conflicting or drop back to measuredBounds for them. The shipped 5h ← weekly ← weekly-fable chain is exactly this shape, one measurement away from producing an unflagged unsound estimate.

🟠 **High:** A conflict marks only the two endpoints of the failing edge, so the rest of the connected component keeps the bounds it was tightened to *before* the contradiction surfaced and is then reported as a clean, tightened estimate. Reproduced with a 4-meter plan declared in this key order — `B = 1 × A`, `Z = 1 × B`, `A = 1 × C` — and measured hulls A `$100–$200`, B `$150–$300`, C `$100–$120`, Z `>$50`. Sweep 1 tightens A and B to `$150–$200` via the first edge, propagates that to Z via the second, then finds `A = 1 × C` empty. The run flags only A and C; B and Z print `combined with relation … $150.00–$200.00` with no marker, even though no assignment satisfies all three relations at once. Fix: once the fixed point is reached, expand the conflict to the whole connected component (union-find or BFS over `relationDefs`) and either mark those nodes conflicting or drop back to `measuredBounds` for them. The shipped `5h ← weekly ← weekly-fable` chain is exactly this shape, one measurement away from producing an unflagged unsound estimate.
Author
Owner

Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.

<!-- gh-feedback:reply-to:41489 --> Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.
jercik marked this conversation as resolved
@ -364,0 +410,4 @@
break;
}
}
if (!converged) fail("worth relations did not reach a fixed point — check plans.json for a contradictory relation cycle");

🟢 Low: This fail() discards the entire report, including plans unrelated to the bad cycle. A near-consistent contradictory cycle hits the iteration cap instead of producing an empty intersection: with X = 1.0001 × Y and Y = 1.0001 × X (both measured $100–$200) plus a second, independent plan, the run prints only worth relations did not reach a fixed point … and exits 1, so the healthy plan's estimate is lost too. (1.01 in place of 1.0001 converges to a normal CONFLICT inside the cap, so the cap is only reached by cycles whose product is close to 1.)

Scoping the failure to the affected plan — mark its meters conflicting, keep estimating the others — keeps the tool usable while plans.json is being fixed.

🟢 **Low:** This `fail()` discards the entire report, including plans unrelated to the bad cycle. A near-consistent contradictory cycle hits the iteration cap instead of producing an empty intersection: with `X = 1.0001 × Y` and `Y = 1.0001 × X` (both measured `$100–$200`) plus a second, independent plan, the run prints only `worth relations did not reach a fixed point …` and exits 1, so the healthy plan's estimate is lost too. (`1.01` in place of `1.0001` converges to a normal `CONFLICT` inside the cap, so the cap is only reached by cycles whose product is close to 1.) Scoping the failure to the affected plan — mark its meters conflicting, keep estimating the others — keeps the tool usable while `plans.json` is being fixed.
Author
Owner

Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.

<!-- gh-feedback:reply-to:41492 --> Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.
jercik marked this conversation as resolved
@ -377,0 +425,4 @@
const other = relationBounds.get(otherKey);
if (!other) continue;
const text = relationText(def);
const implied = scaled(other, factor);

🟡 Medium: implied is derived from relationBounds (post-fixed-point) instead of the other meter's measured hull, so the per-edge diagnostic reflects whatever the sweep order happened to accumulate rather than what this relation says.

On the committed data the run prints, under weekly-fable:

relation weekly-fable = 0.5 × weekly CONFLICTS: implies $524.74–$533.88

But that relation alone implies $384.43–$542.02 from weekly's measured hull $768.87–$1084.04, which contains the measured $427.15–$489.39 and conflicts with nothing. The $524.74 figure already carries the new weekly = 6 × 5h tightening, so the edge that is actually unsatisfiable is not the one being blamed.

The same coupling reintroduces key-order dependence: swapping the weekly and weekly-fable keys in plans.json, with no data change, moves the CONFLICTS marker off weekly-fable and onto 5h. Computing each constraint's implied from measuredBounds.get(otherKey) makes the per-edge line reproducible and identifies the genuinely empty edge, while combined can keep using the fixed-point bound.

🟡 **Medium:** `implied` is derived from `relationBounds` (post-fixed-point) instead of the other meter's measured hull, so the per-edge diagnostic reflects whatever the sweep order happened to accumulate rather than what this relation says. On the committed data the run prints, under `weekly-fable`: relation weekly-fable = 0.5 × weekly CONFLICTS: implies $524.74–$533.88 But that relation alone implies `$384.43–$542.02` from `weekly`'s measured hull `$768.87–$1084.04`, which contains the measured `$427.15–$489.39` and conflicts with nothing. The `$524.74` figure already carries the new `weekly = 6 × 5h` tightening, so the edge that is actually unsatisfiable is not the one being blamed. The same coupling reintroduces key-order dependence: swapping the `weekly` and `weekly-fable` keys in `plans.json`, with no data change, moves the `CONFLICTS` marker off `weekly-fable` and onto `5h`. Computing each constraint's `implied` from `measuredBounds.get(otherKey)` makes the per-edge line reproducible and identifies the genuinely empty edge, while `combined` can keep using the fixed-point bound.
Author
Owner

Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.

<!-- gh-feedback:reply-to:41490 --> Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.
jercik marked this conversation as resolved
Merge remote-tracking branch 'origin/main' into feat/usage-ratio-calibration
Some checks failed
Node tests / node:test (pull_request) Successful in 8s
commit-msg / commitlint (pull_request) Successful in 19s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 1m28s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 36s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 6m3s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 6m20s
c96ded5dd8
forgejo-actions left a comment

Summary: Reviewed the calibration ratios and the relation fixed-point rewrite by running estimate.ts against the committed measurements and synthetic plan variants. Found 1 high, 2 medium, 2 low, and 1 informational issue.

The ratios themselves rest on external Prometheus evidence I cannot check from this tree, and the resulting CONFLICT on the bundled data is documented as intentional. The actionable defects are all in how conflicts are scoped and reported:

  • Conflicts are marked per-edge/per-node, but bounds propagate across the whole connected component, so a neighbouring meter can keep a relation-tightened headline derived from a system the same run calls contradictory (reproduced with a fourth meter).
  • Which relation is labelled CONFLICTS, and the "implies" range printed beside it, both shift with plans.json key order — the property SKILL.md claims is now order-independent. With the committed data the consistent weekly = 6 × 5h edge is blamed alongside the one that actually fails.
  • A contradictory relation cycle with unbounded upper bounds hits the non-convergence fail() and suppresses all output rather than surfacing a CONFLICT.

Verified: node --version v26.5.0 runs the script directly; all reproductions above are from actual runs.

Code review by Claude Code Opus (opus)

**Summary:** Reviewed the calibration ratios and the relation fixed-point rewrite by running `estimate.ts` against the committed measurements and synthetic plan variants. Found 1 high, 2 medium, 2 low, and 1 informational issue. The ratios themselves rest on external Prometheus evidence I cannot check from this tree, and the resulting CONFLICT on the bundled data is documented as intentional. The actionable defects are all in how conflicts are scoped and reported: - Conflicts are marked per-edge/per-node, but bounds propagate across the whole connected component, so a neighbouring meter can keep a relation-tightened headline derived from a system the same run calls contradictory (reproduced with a fourth meter). - Which relation is labelled `CONFLICTS`, and the "implies" range printed beside it, both shift with `plans.json` key order — the property `SKILL.md` claims is now order-independent. With the committed data the consistent `weekly = 6 × 5h` edge is blamed alongside the one that actually fails. - A contradictory relation cycle with unbounded upper bounds hits the non-convergence `fail()` and suppresses all output rather than surfacing a `CONFLICT`. Verified: `node --version` v26.5.0 runs the script directly; all reproductions above are from actual runs. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6ImM5NmRlZDVkZDhmY2EwZGU5ZDc5OTM0YWE4ZmFjOTQxMjY5ZDRlYjYiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE2NDE1Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjgwM2ZkNWViLTcyMDEtNDJkMC05ODA1LTU1MDg0OWMyOGYxOCJ9 -->
@ -30,3 +30,3 @@
One spending window usually moves several meters at once; record a separate line per meter, sharing `usd`, `at`, and `session`. `note` stays free-text for provenance only — anything the estimate depends on belongs in the typed fields.
`plans.json` describes each plan: its `monthlyCostUsd` and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without one — like `5h`, a throttle whose rate cap is independent of the weekly quotas — deliberately gets no monthly line; don't add a period to one. A meter's `worth: {"fraction": 0.5, "of": "weekly"}` declares a known cross-meter fact (its total is that fraction of a sibling meter's total, e.g. the fable meter is worth half the general weekly limit); the script propagates estimates through it in both directions automatically.
`plans.json` describes each plan: its `monthlyCostUsd` and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without `periodHours` — like `5h` — has no monthly equivalent in this estimator; it may still participate in a `worth` relation, so don't add a made-up period. A meter's `worth: {"fraction": 0.5, "of": "weekly"}` declares a cross-meter ratio (its total is that fraction of a sibling meter's total; for example, the fable meter is half the general weekly limit, while a plan can express `weekly = 6 × 5h` with `fraction: 6`). The estimator treats every `worth` entry as a bidirectional interval constraint, intersects all connected relations to a fixed point, and reports `CONFLICT` rather than choosing a relation based on JSON key order. A relation with no measured meters is reported as unvalidated instead of silently affecting an estimate.

🟢 Low: Two claims here overstate the implementation:

  • "reports CONFLICT rather than choosing a relation based on JSON key order" — the converged bounds are order-independent, but which meters and edges get flagged is not. Reordering the claude-max-20x meter keys to 5h, weekly-fable, weekly moves the CONFLICT from weekly-fable to 5h, and the reported "implies" ranges change with it (see the notes on estimate.ts).
  • "A relation with no measured meters" reads as requiring both endpoints to be unmeasured; the filter at estimate.ts:441 is ||, so one unmeasured endpoint suffices. Line 58 states it correctly as "one or both" — worth matching the wording here.
🟢 **Low:** Two claims here overstate the implementation: - "reports `CONFLICT` rather than choosing a relation based on JSON key order" — the converged bounds are order-independent, but which meters and edges get flagged is not. Reordering the `claude-max-20x` meter keys to `5h`, `weekly-fable`, `weekly` moves the CONFLICT from `weekly-fable` to `5h`, and the reported "implies" ranges change with it (see the notes on `estimate.ts`). - "A relation with no measured meters" reads as requiring *both* endpoints to be unmeasured; the filter at `estimate.ts:441` is `||`, so one unmeasured endpoint suffices. Line 58 states it correctly as "one or both" — worth matching the wording here.
Author
Owner

Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.

<!-- gh-feedback:reply-to:41660 --> Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.
jercik marked this conversation as resolved
@ -7,1 +7,3 @@
"weekly": { "periodHours": 168 },
"weekly": {
"periodHours": 168,
"worth": { "fraction": 6, "of": "5h" }

ℹ️ Info: Recording this ratio makes the bundled dataset self-contradictory, which changes the shipped skill's default output: claude-max-20x / weekly drops its combined with relation headline of $854.30–$978.77 (18.6×–21.3×) back to the measured-alone $768.87–$1084.04 (16.7×–23.6×), and both weekly and weekly-fable now print CONFLICTS. SKILL.md:34 says this is intended until the provenance is reconciled — flagging only so the widened headline is a conscious trade.

ℹ️ **Info:** Recording this ratio makes the bundled dataset self-contradictory, which changes the shipped skill's default output: `claude-max-20x / weekly` drops its `combined with relation` headline of $854.30–$978.77 (18.6×–21.3×) back to the measured-alone $768.87–$1084.04 (16.7×–23.6×), and both `weekly` and `weekly-fable` now print CONFLICTS. `SKILL.md:34` says this is intended until the provenance is reconciled — flagging only so the widened headline is a conscious trade.
jercik marked this conversation as resolved
@ -364,0 +395,4 @@
const impliedOf = scaled(meter, 1 / def.fraction);
const nextMeter = intersect(meter, impliedMeter);
const nextOf = intersect(of, impliedOf);
if (!nextMeter || !nextOf) {

🟠 High: A conflict is recorded only on the failing edge and its two endpoints, but the fixed-point loop has already propagated bounds across the whole connected component. Every other meter in that component keeps its relation-tightened headline with no conflict marker, so an estimate silently rests on a constraint system the same run declares contradictory.

Reproduced by adding a fourth meter to claude-max-20x ("x": {"periodHours": 168, "worth": {"fraction": 1, "of": "weekly"}}) plus one measurement for it, against the committed measurements.jsonl:

claude-max-20x / weekly
  current: $768.87–$1084.04 …
    relation weekly = 6 × 5h CONFLICTS …
claude-max-20x / x
  current: $1049.48–$1067.75 (midpoint $1058.62) — combined with relation x = 1 × weekly
    measured alone: $768.87–$1287.88 …

x is narrowed to a bound that exists only because of the weekly = 6 × 5h propagation the run prints as CONFLICTS, and reports it as a clean tightening.

The same locality also makes conflict attribution depend on JSON key order, which SKILL.md:32 explicitly promises it does not: reordering the claude-max-20x meter keys to 5h, weekly-fable, weekly (semantically identical) moves the CONFLICT off weekly-fable entirely — it then prints no relation diagnostic at all — and onto 5h.

Fix: when any edge intersection is empty, mark the whole connected component as conflicted and fall back to the measured hull for every node in it, rather than only the failing edge's endpoints.

🟠 **High:** A conflict is recorded only on the failing edge and its two endpoints, but the fixed-point loop has already propagated bounds across the whole connected component. Every *other* meter in that component keeps its relation-tightened headline with no conflict marker, so an estimate silently rests on a constraint system the same run declares contradictory. Reproduced by adding a fourth meter to `claude-max-20x` (`"x": {"periodHours": 168, "worth": {"fraction": 1, "of": "weekly"}}`) plus one measurement for it, against the committed `measurements.jsonl`: ``` claude-max-20x / weekly current: $768.87–$1084.04 … relation weekly = 6 × 5h CONFLICTS … claude-max-20x / x current: $1049.48–$1067.75 (midpoint $1058.62) — combined with relation x = 1 × weekly measured alone: $768.87–$1287.88 … ``` `x` is narrowed to a bound that exists only because of the `weekly = 6 × 5h` propagation the run prints as CONFLICTS, and reports it as a clean tightening. The same locality also makes conflict attribution depend on JSON key order, which `SKILL.md:32` explicitly promises it does not: reordering the `claude-max-20x` meter keys to `5h`, `weekly-fable`, `weekly` (semantically identical) moves the CONFLICT off `weekly-fable` entirely — it then prints no relation diagnostic at all — and onto `5h`. Fix: when any edge intersection is empty, mark the whole connected component as conflicted and fall back to the measured hull for every node in it, rather than only the failing edge's endpoints.
Author
Owner

Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.

<!-- gh-feedback:reply-to:41656 --> Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.
jercik marked this conversation as resolved
@ -364,0 +410,4 @@
break;
}
}
if (!converged) fail("worth relations did not reach a fixed point — check plans.json for a contradictory relation cycle");

🟢 Low: Non-convergence aborts the whole run, discarding every series report including plans that have nothing to do with the offending cycle. A contradictory cycle whose upper bounds are unbounded never produces an empty intersection — lower bounds just grow geometrically — so it lands here rather than in the CONFLICT path:

{"plan":"p","monthlyCostUsd":100,"meters":{
  "a":{"periodHours":168,"worth":{"fraction":2,"of":"b"}},
  "b":{"periodHours":168,"worth":{"fraction":2,"of":"a"}}}}

With two start: 0, end: 1 measurements this exits 1 printing only the fail message. Treating the non-converging component as conflicted (measured hulls, CONFLICT on its edges) would keep this consistent with the documented "surface it rather than smoothing over it" behaviour and preserve the rest of the output.

🟢 **Low:** Non-convergence aborts the whole run, discarding every series report including plans that have nothing to do with the offending cycle. A contradictory cycle whose upper bounds are unbounded never produces an empty intersection — lower bounds just grow geometrically — so it lands here rather than in the `CONFLICT` path: ```json {"plan":"p","monthlyCostUsd":100,"meters":{ "a":{"periodHours":168,"worth":{"fraction":2,"of":"b"}}, "b":{"periodHours":168,"worth":{"fraction":2,"of":"a"}}}} ``` With two `start: 0, end: 1` measurements this exits 1 printing only the fail message. Treating the non-converging component as conflicted (measured hulls, `CONFLICT` on its edges) would keep this consistent with the documented "surface it rather than smoothing over it" behaviour and preserve the rest of the output.
Author
Owner

Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.

<!-- gh-feedback:reply-to:41659 --> Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.
jercik marked this conversation as resolved
@ -375,2 +424,2 @@
return { text: `${def.meter} = ${def.fraction} × ${def.of}`, implied, combined };
}
const otherKey = nodeKey(plan, def.meter === limit ? def.of : def.meter);
const other = relationBounds.get(otherKey);

🟡 Medium: implied is computed from the post-fixed-point bound of the other meter, so the printed "implies" range folds in every other relation that tightened it — and, for an edge that already ran, is partly derived from this meter itself. The number is not what the named relation plus the other meter's measurements imply, which is what the message claims.

Reproduced by reordering the claude-max-20x meter keys to 5h, weekly-fable, weekly:

claude-max-20x / 5h
  current: $174.91–$177.96 …
    relation weekly = 6 × 5h CONFLICTS: implies $142.38–$163.13 — limit change or wrong relation

weekly's measured hull is $768.87–$1084.04, so that relation alone implies 5h ∈ $128.14–$180.67, which does not conflict with the measured $174.91–$177.96. The $142.38–$163.13 figure comes from weekly already having been narrowed by the fable relation.

Read measuredBounds (not relationBounds) for the reported implied so each line states what that one relation actually implies; keep relationBounds for the combined headline.

🟡 **Medium:** `implied` is computed from the post-fixed-point bound of the other meter, so the printed "implies" range folds in every *other* relation that tightened it — and, for an edge that already ran, is partly derived from this meter itself. The number is not what the named relation plus the other meter's measurements imply, which is what the message claims. Reproduced by reordering the `claude-max-20x` meter keys to `5h`, `weekly-fable`, `weekly`: ``` claude-max-20x / 5h current: $174.91–$177.96 … relation weekly = 6 × 5h CONFLICTS: implies $142.38–$163.13 — limit change or wrong relation ``` `weekly`'s *measured* hull is $768.87–$1084.04, so that relation alone implies 5h ∈ $128.14–$180.67, which does not conflict with the measured $174.91–$177.96. The $142.38–$163.13 figure comes from `weekly` already having been narrowed by the fable relation. Read `measuredBounds` (not `relationBounds`) for the reported `implied` so each line states what that one relation actually implies; keep `relationBounds` for the combined headline.
Author
Owner

Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.

<!-- gh-feedback:reply-to:41658 --> Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.
jercik marked this conversation as resolved
@ -377,0 +426,4 @@
if (!other) continue;
const text = relationText(def);
const implied = scaled(other, factor);
const conflict = conflictEdges.has(edgeKey(def)) || conflictNodes.has(ownKey);

🟡 Medium: Because conflictEdges.add always also adds both endpoints to conflictNodes, this predicate reduces to conflictNodes.has(ownKey) — every relation touching a conflicted meter is labelled CONFLICTS, including relations that are individually consistent with the measurements. With the committed data the run prints:

    relation weekly = 6 × 5h CONFLICTS: implies $1049.48–$1067.75 — limit change or wrong relation

but $1049.48–$1067.75 overlaps weekly's measured $768.87–$1084.04, so that relation is not the one that fails; the empty intersection is on weekly-fable = 0.5 × weekly. The diagnostic points the reader at the wrong relation to reconcile.

Distinguish the two states in the output — e.g. CONFLICTS only for conflictEdges.has(edgeKey(def)), and a separate "suppressed: a connected relation conflicts" wording for the rest.

(Side effect of the same all-or-nothing behaviour: the suffix = constraint.conflict ? " CONFLICTS" : "" branch at line 520 can never take the empty arm, since a non-conflicting report always goes through the relation?.combined branch.)

🟡 **Medium:** Because `conflictEdges.add` always also adds both endpoints to `conflictNodes`, this predicate reduces to `conflictNodes.has(ownKey)` — every relation touching a conflicted meter is labelled `CONFLICTS`, including relations that are individually consistent with the measurements. With the committed data the run prints: ``` relation weekly = 6 × 5h CONFLICTS: implies $1049.48–$1067.75 — limit change or wrong relation ``` but $1049.48–$1067.75 overlaps `weekly`'s measured $768.87–$1084.04, so that relation is not the one that fails; the empty intersection is on `weekly-fable = 0.5 × weekly`. The diagnostic points the reader at the wrong relation to reconcile. Distinguish the two states in the output — e.g. `CONFLICTS` only for `conflictEdges.has(edgeKey(def))`, and a separate "suppressed: a connected relation conflicts" wording for the rest. (Side effect of the same all-or-nothing behaviour: the `suffix = constraint.conflict ? " CONFLICTS" : ""` branch at line 520 can never take the empty arm, since a non-conflicting report always goes through the `relation?.combined` branch.)
Author
Owner

Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.

<!-- gh-feedback:reply-to:41657 --> Fixed in 163a6ff: relation conflicts now invalidate the full connected component, non-convergent components fall back without aborting unrelated plans, diagnostics use measured bounds with CONFLICTS/SUPPRESSED states, and the skill wording matches that behavior.
jercik marked this conversation as resolved
fix: scope usage relation conflicts by component
Some checks failed
Node tests / node:test (pull_request) Successful in 9s
commit-msg / commitlint (pull_request) Successful in 16s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 1m28s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 33s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 5m49s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 6m11s
163a6ff659
Author
Owner

Replying to review #9673

The component-scoped conflict handling, measured-bound diagnostics, and non-convergence behavior are fixed in 163a6ff; all findings from this review are addressed.

> Replying to review #9673 The component-scoped conflict handling, measured-bound diagnostics, and non-convergence behavior are fixed in 163a6ff; all findings from this review are addressed.
Author
Owner

Replying to review #9692

The final review findings are fixed in 163a6ff: conflicted components now fall back to measured bounds, unrelated plans continue reporting, and diagnostics are deterministic.

> Replying to review #9692 The final review findings are fixed in 163a6ff: conflicted components now fall back to measured bounds, unrelated plans continue reporting, and diagnostics are deterministic.
forgejo-actions left a comment

Summary: Found 2 medium and 2 low issues. The relation fixed point itself checks out — I verified component discovery, order independence (reversed meters key order gives identical bounds), chained propagation, the pairwise CONFLICTS path, and the conflict fallback to measured hulls, all by running node scripts/estimate.ts against the checked-in data and synthetic fixtures.

The substantive findings are about diagnostics and the shipped calibration:

  • The new weekly = 6 × 5h relation for claude-max-20x is jointly contradicted by the repository's own measurements via weekly-fable = 0.5 × weekly ($1049.48–$1067.75 vs $854.30–$978.77, disjoint). The whole component collapses, so the previously working fable tightening is lost and the weekly headline widens from $854.30–$978.77 to $768.87–$1084.04.
  • Because that contradiction is joint rather than pairwise, no edge is ever marked CONFLICTS; every edge prints SUPPRESSED … another relation in this component conflicts, pointing at a conflicting relation that does not exist. The same wording is reused for the nonconvergence bail-out. This also makes SKILL.md's description of SUPPRESSED and its "must remain visible as CONFLICT" claim inaccurate for the data as shipped.
  • Minor: the implied interval printed beside a relation is computed from the neighbour's measured bound, while the headline is the component fixed point, so a transitively tightened headline cannot be derived from the relations shown.

Code review by Claude Code Opus (opus)

**Summary:** Found 2 medium and 2 low issues. The relation fixed point itself checks out — I verified component discovery, order independence (reversed `meters` key order gives identical bounds), chained propagation, the pairwise `CONFLICTS` path, and the conflict fallback to measured hulls, all by running `node scripts/estimate.ts` against the checked-in data and synthetic fixtures. The substantive findings are about diagnostics and the shipped calibration: - The new `weekly = 6 × 5h` relation for `claude-max-20x` is jointly contradicted by the repository's own measurements via `weekly-fable = 0.5 × weekly` ($1049.48–$1067.75 vs $854.30–$978.77, disjoint). The whole component collapses, so the previously working fable tightening is lost and the weekly headline widens from $854.30–$978.77 to $768.87–$1084.04. - Because that contradiction is joint rather than pairwise, no edge is ever marked `CONFLICTS`; every edge prints `SUPPRESSED … another relation in this component conflicts`, pointing at a conflicting relation that does not exist. The same wording is reused for the nonconvergence bail-out. This also makes SKILL.md's description of `SUPPRESSED` and its "must remain visible as `CONFLICT`" claim inaccurate for the data as shipped. - Minor: the `implied` interval printed beside a relation is computed from the neighbour's measured bound, while the headline is the component fixed point, so a transitively tightened headline cannot be derived from the relations shown. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6IjE2M2E2ZmY2NTk0Yzk4NWFlZmIyNTZjNjNkYjk5OWU5ODFjNDMzZWEiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE2ODA4Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjQxZTBjOGE4LTY4YWMtNGJjNC04ZjU4LTg0ZDJkNGRjOTE0ZiJ9 -->
@ -54,3 +56,3 @@
- The headline per meter is the estimate under the assumption that the display uses one rounding mode consistently; a 1-point delta yields only a lower bound ("at least $X"). The `mode-agnostic bound` line is the wider interval the data proves without that assumption, and `per-mode` shows each rounding mode's interval (∅ = that mode cannot explain the data), making near-miss eliminations visible. A "Display rounding: data rules out …" header reports which modes the exact-start readings eliminated; the warning that no mode fits means a `startExact` claim is suspect — check with the user whether that window truly began at a reset.
- `combined with relation` means a declared `worth` relation tightened the estimate beyond the series' own measurements; `measured alone` preserves the unassisted bound. A relation CONFLICT means the relation and the measurements cannot both be right — surface it rather than smoothing over it.
- `combined with relation` means the fixed-point intersection of all applicable `worth` relations tightened the estimate beyond the series' own measurements; `measured alone` preserves the unassisted bound. When a relation component conflicts or fails to converge, the affected meters fall back to measured bounds. `CONFLICTS` marks an edge whose own measured intervals are disjoint; `SUPPRESSED` means another edge in the same component conflicts, so this edge is not blamed individually. `Unvalidated declared relations` means one or both meters have no measurements, so the ratio is recorded but not yet checked by this tool.

🟢 Low: "SUPPRESSED means another edge in the same component conflicts" does not match the implementation. SUPPRESSED is set for every non-CONFLICTS edge whenever componentConflict is true, which includes (a) a joint contradiction where no individual edge is pairwise disjoint and (b) a nonconvergence bail-out at estimate.ts:452. The repository's own data hits case (a): all edges print SUPPRESSED, none prints CONFLICTS. Reword to something like "the component as a whole is unsatisfiable (or did not converge); if no edge is marked CONFLICTS, the contradiction is joint across several relations".

🟢 **Low:** "`SUPPRESSED` means another edge in the same component conflicts" does not match the implementation. `SUPPRESSED` is set for every non-`CONFLICTS` edge whenever `componentConflict` is true, which includes (a) a joint contradiction where no individual edge is pairwise disjoint and (b) a nonconvergence bail-out at `estimate.ts:452`. The repository's own data hits case (a): all edges print `SUPPRESSED`, none prints `CONFLICTS`. Reword to something like "the component as a whole is unsatisfiable (or did not converge); if no edge is marked `CONFLICTS`, the contradiction is joint across several relations".
Author
Owner

Fixed in 65583f5: joint and non-convergent components now emit explicit component-level diagnostics, suppressed-edge wording is accurate, transitive implied ranges use fixed-point bounds, and the skill documents the intentional Max 20x provenance conflict.

<!-- gh-feedback:reply-to:42105 --> Fixed in 65583f5: joint and non-convergent components now emit explicit component-level diagnostics, suppressed-edge wording is accurate, transitive implied ranges use fixed-point bounds, and the skill documents the intentional Max 20x provenance conflict.
jercik marked this conversation as resolved
@ -7,1 +7,3 @@
"weekly": { "periodHours": 168 },
"weekly": {
"periodHours": 168,
"worth": { "fraction": 6, "of": "5h" }

🟡 Medium: This calibration is contradicted by the measurements checked into this repository, and the practical effect is that claude-max-20x loses all relation refinement.

From resources/measurements.jsonl: 5h = $174.91–$177.96, so weekly = 6 × 5h implies $1049.48–$1067.75. weekly-fable = $427.15–$489.39, so weekly = 2 × weekly-fable implies $854.30–$978.77. Those two are disjoint, so the component is infeasible and every meter falls back to its measured hull.

Before this PR (same data, 6× relation removed):

claude-max-20x / weekly
  current: $854.30–$978.77 — combined with relation weekly-fable = 0.5 × weekly
    monthly equivalent: $3714.69–$4255.92 (18.6×–21.3× the $200/mo price)

After this PR:

claude-max-20x / weekly
  current: $768.87–$1084.04, ... (all relations SUPPRESSED)
    monthly equivalent: $3343.22–$4713.62 (16.7×–23.6× the $200/mo price)

SKILL.md's "a contradiction must remain visible as CONFLICT" makes the widening intentional, but no CONFLICT/CONFLICTS marker is actually printed for this data (see the estimate.ts comment). As shipped, the default dataset produces a permanently conflicting, unattributable output and a strictly weaker headline. Worth either reconciling the ramp provenance against the fable relation before landing, or stating in SKILL.md that the max-20x component is currently expected to conflict and why.

🟡 **Medium:** This calibration is contradicted by the measurements checked into this repository, and the practical effect is that `claude-max-20x` loses *all* relation refinement. From `resources/measurements.jsonl`: `5h` = $174.91–$177.96, so `weekly = 6 × 5h` implies $1049.48–$1067.75. `weekly-fable` = $427.15–$489.39, so `weekly = 2 × weekly-fable` implies $854.30–$978.77. Those two are disjoint, so the component is infeasible and every meter falls back to its measured hull. Before this PR (same data, 6× relation removed): ``` claude-max-20x / weekly current: $854.30–$978.77 — combined with relation weekly-fable = 0.5 × weekly monthly equivalent: $3714.69–$4255.92 (18.6×–21.3× the $200/mo price) ``` After this PR: ``` claude-max-20x / weekly current: $768.87–$1084.04, ... (all relations SUPPRESSED) monthly equivalent: $3343.22–$4713.62 (16.7×–23.6× the $200/mo price) ``` SKILL.md's "a contradiction must remain visible as `CONFLICT`" makes the widening intentional, but no `CONFLICT`/`CONFLICTS` marker is actually printed for this data (see the `estimate.ts` comment). As shipped, the default dataset produces a permanently conflicting, unattributable output and a strictly weaker headline. Worth either reconciling the ramp provenance against the fable relation before landing, or stating in SKILL.md that the max-20x component is currently expected to conflict and why.
Author
Owner

Fixed in 65583f5: joint and non-convergent components now emit explicit component-level diagnostics, suppressed-edge wording is accurate, transitive implied ranges use fixed-point bounds, and the skill documents the intentional Max 20x provenance conflict.

<!-- gh-feedback:reply-to:42104 --> Fixed in 65583f5: joint and non-convergent components now emit explicit component-level diagnostics, suppressed-edge wording is accurate, transitive implied ranges use fixed-point bounds, and the skill documents the intentional Max 20x provenance conflict.
jercik marked this conversation as resolved
@ -377,0 +484,4 @@
const other = measuredBounds.get(otherKey);
if (!other) continue;
const text = relationText(def);
const implied = scaled(other, factor);

🟢 Low: implied is derived from the neighbour's measured bound (measuredBounds), while relation.combined reported on the same line is the whole-component fixed point from relationBounds. The printed headline therefore cannot be reconstructed from the printed relations when the tightening arrives transitively.

Reproduced with a 3-meter chain b = 2 × a, c = 2 × b:

p / a
  current: $92.95–$111.39 — combined with relation b = 2 × a
    measured alone: $90.68–$111.39

b = 2 × a alone implies only $90.68–$111.39, i.e. no tightening; the $92.95 lower bound comes from c = 2 × b, which is never shown for a. Either list the transitive path, or compute implied from relationBounds so the shown interval explains the shown result.

🟢 **Low:** `implied` is derived from the neighbour's **measured** bound (`measuredBounds`), while `relation.combined` reported on the same line is the whole-component fixed point from `relationBounds`. The printed headline therefore cannot be reconstructed from the printed relations when the tightening arrives transitively. Reproduced with a 3-meter chain `b = 2 × a`, `c = 2 × b`: ``` p / a current: $92.95–$111.39 — combined with relation b = 2 × a measured alone: $90.68–$111.39 ``` `b = 2 × a` alone implies only $90.68–$111.39, i.e. no tightening; the $92.95 lower bound comes from `c = 2 × b`, which is never shown for `a`. Either list the transitive path, or compute `implied` from `relationBounds` so the shown interval explains the shown result.
Author
Owner

Fixed in 65583f5: joint and non-convergent components now emit explicit component-level diagnostics, suppressed-edge wording is accurate, transitive implied ranges use fixed-point bounds, and the skill documents the intentional Max 20x provenance conflict.

<!-- gh-feedback:reply-to:42103 --> Fixed in 65583f5: joint and non-convergent components now emit explicit component-level diagnostics, suppressed-edge wording is accurate, transitive implied ranges use fixed-point bounds, and the skill documents the intentional Max 20x provenance conflict.
jercik marked this conversation as resolved
@ -455,0 +588,4 @@
: constraint.suppressed
? " SUPPRESSED: connected relation component conflicts"
: "";
const reason = constraint.suppressed ? "another relation in this component conflicts" : "limit change or wrong relation";

🟡 Medium: The SUPPRESSED reason is misleading whenever the contradiction is joint rather than pairwise — which is exactly what this PR's own data produces.

conflictEdges is populated only for edges whose two measured hulls are pairwise disjoint (lines 464-471). A component can be infeasible without any single edge being infeasible, and nonconvergence (line 452) sets componentConflict with no edge flagged at all. In both cases every edge gets suppressed: true and prints another relation in this component conflicts, while no CONFLICTS line is emitted anywhere — sending the reader to hunt for a conflicting relation that does not exist.

Verified against the checked-in resources/measurements.jsonl + the new plans.json:

claude-max-20x / weekly
  ...
    relation weekly = 6 × 5h SUPPRESSED: connected relation component conflicts: implies $1049.48–$1067.75 — another relation in this component conflicts
    relation weekly-fable = 0.5 × weekly SUPPRESSED: ... — another relation in this component conflicts

--json confirms every constraint carries conflict: false, suppressed: true; conflictEdges is empty.

Distinguish the three cases, e.g. keep the current wording only when conflictEdges intersects this component, and otherwise emit something like no single relation is contradicted; the N relations in this component are jointly unsatisfiable (or component did not converge). Also note the line currently reads SUPPRESSED: connected relation component conflicts: implies … — another relation …, stating the reason twice with a stray second colon.

🟡 **Medium:** The `SUPPRESSED` reason is misleading whenever the contradiction is *joint* rather than pairwise — which is exactly what this PR's own data produces. `conflictEdges` is populated only for edges whose two **measured** hulls are pairwise disjoint (lines 464-471). A component can be infeasible without any single edge being infeasible, and nonconvergence (line 452) sets `componentConflict` with no edge flagged at all. In both cases every edge gets `suppressed: true` and prints `another relation in this component conflicts`, while no `CONFLICTS` line is emitted anywhere — sending the reader to hunt for a conflicting relation that does not exist. Verified against the checked-in `resources/measurements.jsonl` + the new `plans.json`: ``` claude-max-20x / weekly ... relation weekly = 6 × 5h SUPPRESSED: connected relation component conflicts: implies $1049.48–$1067.75 — another relation in this component conflicts relation weekly-fable = 0.5 × weekly SUPPRESSED: ... — another relation in this component conflicts ``` `--json` confirms every constraint carries `conflict: false, suppressed: true`; `conflictEdges` is empty. Distinguish the three cases, e.g. keep the current wording only when `conflictEdges` intersects this component, and otherwise emit something like `no single relation is contradicted; the N relations in this component are jointly unsatisfiable` (or `component did not converge`). Also note the line currently reads `SUPPRESSED: connected relation component conflicts: implies … — another relation …`, stating the reason twice with a stray second colon.
Author
Owner

Fixed in 65583f5: joint and non-convergent components now emit explicit component-level diagnostics, suppressed-edge wording is accurate, transitive implied ranges use fixed-point bounds, and the skill documents the intentional Max 20x provenance conflict.

<!-- gh-feedback:reply-to:42102 --> Fixed in 65583f5: joint and non-convergent components now emit explicit component-level diagnostics, suppressed-edge wording is accurate, transitive implied ranges use fixed-point bounds, and the skill documents the intentional Max 20x provenance conflict.
jercik marked this conversation as resolved
Merge branch 'main' into feat/usage-ratio-calibration
Some checks failed
commit-msg / commitlint (pull_request) Successful in 15s
Node tests / node:test (pull_request) Successful in 19s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 1m51s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 39s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 6m5s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 6m25s
8a6c4f9832
forgejo-actions left a comment

Summary: Found 2 medium and 2 low/info issues.

I ran node scripts/estimate.ts on the checked-out tree and on the pre-PR baseline (92d63c4) to compare. The relation fixed-point machinery itself checks out: it is order-independent, direct edge contradictions still print CONFLICTS, non-convergent cycles are caught, and unmeasured relations are listed as unvalidated.

The substantive finding is that the newly recorded weekly = 6 × 5h calibration is jointly contradicted by this repository's own measurements, and the resulting conflict is never surfaced. Default run before this PR: claude-max-20x / weekly = $854.30–$978.77 combined with the fable relation. After: every meter in the component falls back to measured bounds ($768.87–$1084.04 for weekly) and every relation line reads SUPPRESSED: … another relation in this component conflicts — while no relation is printed as CONFLICTS anywhere in the output. The PR description's validation claim ("estimator outputs explicit CONFLICT … diagnostics") does not hold for the shipped dataset; only the --json output carries relation.conflict: true.

Code review by Claude Code Opus (opus)

**Summary:** Found 2 medium and 2 low/info issues. I ran `node scripts/estimate.ts` on the checked-out tree and on the pre-PR baseline (`92d63c4`) to compare. The relation fixed-point machinery itself checks out: it is order-independent, direct edge contradictions still print `CONFLICTS`, non-convergent cycles are caught, and unmeasured relations are listed as unvalidated. The substantive finding is that the newly recorded `weekly = 6 × 5h` calibration is jointly contradicted by this repository's own measurements, and the resulting conflict is never surfaced. Default run before this PR: `claude-max-20x / weekly` = $854.30–$978.77 combined with the fable relation. After: every meter in the component falls back to measured bounds ($768.87–$1084.04 for weekly) and every relation line reads `SUPPRESSED: … another relation in this component conflicts` — while no relation is printed as `CONFLICTS` anywhere in the output. The PR description's validation claim ("estimator outputs explicit CONFLICT … diagnostics") does not hold for the shipped dataset; only the `--json` output carries `relation.conflict: true`. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6IjhhNmM0Zjk4MzJmOTY5MWI0MmRiZGU1MGIwMjYyMDE1MWYwMWRjOWQiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE2ODMxIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjU3N2M0NGY2LTZhY2QtNDViZS1iMGUyLThjYjNiYjZlOTgxMiJ9 -->
@ -54,3 +56,3 @@
- The headline per meter is the estimate under the assumption that the display uses one rounding mode consistently; a 1-point delta yields only a lower bound ("at least $X"). The `mode-agnostic bound` line is the wider interval the data proves without that assumption, and `per-mode` shows each rounding mode's interval (∅ = that mode cannot explain the data), making near-miss eliminations visible. A "Display rounding: data rules out …" header reports which modes the exact-start readings eliminated; the warning that no mode fits means a `startExact` claim is suspect — check with the user whether that window truly began at a reset.
- `combined with relation` means a declared `worth` relation tightened the estimate beyond the series' own measurements; `measured alone` preserves the unassisted bound. A relation CONFLICT means the relation and the measurements cannot both be right — surface it rather than smoothing over it.
- `combined with relation` means the fixed-point intersection of all applicable `worth` relations tightened the estimate beyond the series' own measurements; `measured alone` preserves the unassisted bound. When a relation component conflicts or fails to converge, the affected meters fall back to measured bounds. `CONFLICTS` marks an edge whose own measured intervals are disjoint; `SUPPRESSED` means another edge in the same component conflicts, so this edge is not blamed individually. `Unvalidated declared relations` means one or both meters have no measurements, so the ratio is recorded but not yet checked by this tool.

🟢 Low: "SUPPRESSED means another edge in the same component conflicts" describes a case the code deliberately does not produce here. SUPPRESSED is set for every edge whenever the component is unsatisfiable, including when no single edge is disjoint on its own measured bounds — the joint case the code comment at scripts/estimate.ts:455 calls out, and the one the current dataset actually hits. As written, the reader is told to go find a CONFLICTS line that will not exist. Same issue for line 34's "a contradiction must remain visible as CONFLICT": with the shipped measurements it surfaces only as SUPPRESSED. Wording that covers both: SUPPRESSED means the edge belongs to a component that cannot be satisfied as a whole, with no single edge individually to blame.

🟢 **Low:** "`SUPPRESSED` means another edge in the same component conflicts" describes a case the code deliberately does not produce here. `SUPPRESSED` is set for every edge whenever the component is unsatisfiable, including when no single edge is disjoint on its own measured bounds — the joint case the code comment at `scripts/estimate.ts:455` calls out, and the one the current dataset actually hits. As written, the reader is told to go find a `CONFLICTS` line that will not exist. Same issue for line 34's "a contradiction must remain visible as `CONFLICT`": with the shipped measurements it surfaces only as `SUPPRESSED`. Wording that covers both: `SUPPRESSED` means the edge belongs to a component that cannot be satisfied as a whole, with no single edge individually to blame.
Author
Owner

Fixed in 65583f5: joint and non-convergent components now emit explicit component-level diagnostics, suppressed-edge wording is accurate, transitive implied ranges use fixed-point bounds, and the skill documents the intentional Max 20x provenance conflict.

<!-- gh-feedback:reply-to:42119 --> Fixed in 65583f5: joint and non-convergent components now emit explicit component-level diagnostics, suppressed-edge wording is accurate, transitive implied ranges use fixed-point bounds, and the skill documents the intentional Max 20x provenance conflict.
jercik marked this conversation as resolved
@ -7,1 +7,3 @@
"weekly": { "periodHours": 168 },
"weekly": {
"periodHours": 168,
"worth": { "fraction": 6, "of": "5h" }

🟡 Medium: This ratio is jointly unsatisfiable with the measurements already in resources/measurements.jsonl, so the default run now produces no relation-tightened estimate for the only measured plan.

Running the estimator on this tree: 5h = $174.91–$177.96, so 6 × 5h implies weekly ≥ $1049.48; weekly-fable = $427.15–$489.39 with weekly-fable = 0.5 × weekly implies weekly ≤ $978.77. The two implied intervals are disjoint, so the whole {5h, weekly, weekly-fable} component conflicts and every meter falls back to its measured bound. Before this PR the fable relation tightened weekly to $854.30–$978.77; it is now $768.87–$1084.04, i.e. a strictly weaker headline than the branch point.

SKILL.md's instruction not to edit measurements to force a calibration is a reasonable call, so the fix is not to touch the data — but the conflict has to be legible in the output (see the comment on scripts/estimate.ts), and it is worth stating in SKILL.md which of the three inputs (the 6× ramp, the 0.5× fable ratio, or the fable readings whose start is inferred rather than observed) is expected to give way once provenance is reconciled.

🟡 **Medium:** This ratio is jointly unsatisfiable with the measurements already in `resources/measurements.jsonl`, so the default run now produces no relation-tightened estimate for the only measured plan. Running the estimator on this tree: `5h` = $174.91–$177.96, so `6 × 5h` implies `weekly` ≥ $1049.48; `weekly-fable` = $427.15–$489.39 with `weekly-fable = 0.5 × weekly` implies `weekly` ≤ $978.77. The two implied intervals are disjoint, so the whole `{5h, weekly, weekly-fable}` component conflicts and every meter falls back to its measured bound. Before this PR the fable relation tightened `weekly` to $854.30–$978.77; it is now $768.87–$1084.04, i.e. a strictly weaker headline than the branch point. SKILL.md's instruction not to edit measurements to force a calibration is a reasonable call, so the fix is not to touch the data — but the conflict has to be legible in the output (see the comment on `scripts/estimate.ts`), and it is worth stating in SKILL.md which of the three inputs (the 6× ramp, the 0.5× fable ratio, or the fable readings whose `start` is inferred rather than observed) is expected to give way once provenance is reconciled.
Author
Owner

Fixed in 65583f5: joint and non-convergent components now emit explicit component-level diagnostics, suppressed-edge wording is accurate, transitive implied ranges use fixed-point bounds, and the skill documents the intentional Max 20x provenance conflict.

<!-- gh-feedback:reply-to:42117 --> Fixed in 65583f5: joint and non-convergent components now emit explicit component-level diagnostics, suppressed-edge wording is accurate, transitive implied ranges use fixed-point bounds, and the skill documents the intentional Max 20x provenance conflict.
jercik marked this conversation as resolved
@ -8,0 +16,4 @@
},
{
"plan": "claude-team-premium",
"monthlyCostUsd": 125,

ℹ️ Info: Two things to confirm on this new plan entry, both of which only affect reported numbers rather than correctness:

  • monthlyCostUsd: 125 looks like the annual-billing effective rate for a Team Premium seat, while claude-max-20x uses 200, the month-to-month rate. The N× the $X/mo price output is only comparable across plans if both use the same billing basis.
  • weekly-fable = 0.5 × weekly (lines 26–28) is carried over from claude-max-20x, but the PR description only claims a Team Premium calibration for weekly = 8 × 5h. It is harmless today — the plan has no measurements, so both relations print under Unvalidated declared relations — but it will start constraining estimates silently as soon as the first Team Premium measurement lands.
ℹ️ **Info:** Two things to confirm on this new plan entry, both of which only affect reported numbers rather than correctness: - `monthlyCostUsd: 125` looks like the annual-billing effective rate for a Team Premium seat, while `claude-max-20x` uses `200`, the month-to-month rate. The `N× the $X/mo price` output is only comparable across plans if both use the same billing basis. - `weekly-fable = 0.5 × weekly` (lines 26–28) is carried over from `claude-max-20x`, but the PR description only claims a Team Premium calibration for `weekly = 8 × 5h`. It is harmless today — the plan has no measurements, so both relations print under `Unvalidated declared relations` — but it will start constraining estimates silently as soon as the first Team Premium measurement lands.
jercik marked this conversation as resolved
@ -455,0 +586,4 @@
const suffix = constraint.conflict
? " CONFLICTS"
: constraint.suppressed
? " SUPPRESSED: connected relation component conflicts"

🟡 Medium: When a component's contradiction is joint rather than per-edge — which is exactly the case for the shipped claude-max-20x data — no edge lands in conflictEdges, so every edge prints SUPPRESSED and the word CONFLICT never appears in the text output. The reader is pointed at "another relation in this component conflicts" that is never shown, and the line itself doubles up the same statement twice around the interval:

relation weekly = 6 × 5h SUPPRESSED: connected relation component conflicts: implies $1049.48–$1067.75 (midpoint $1058.62) — another relation in this component conflicts

Two concrete fixes: (1) emit one component-level headline when componentConflict holds and conflictEdges is empty for that component, e.g. relation component {5h, weekly, weekly-fable} CONFLICTS jointly: 6 × 5h implies ≥$1049.48 but 0.5 × weekly implies ≤$978.77, so the disjoint implied pair is named; (2) drop the duplicated reason so each suppressed edge prints its implied interval once. The --json lane already exposes relation.conflict, so only the human-facing path is misleading.

🟡 **Medium:** When a component's contradiction is joint rather than per-edge — which is exactly the case for the shipped `claude-max-20x` data — no edge lands in `conflictEdges`, so every edge prints `SUPPRESSED` and the word `CONFLICT` never appears in the text output. The reader is pointed at "another relation in this component conflicts" that is never shown, and the line itself doubles up the same statement twice around the interval: ``` relation weekly = 6 × 5h SUPPRESSED: connected relation component conflicts: implies $1049.48–$1067.75 (midpoint $1058.62) — another relation in this component conflicts ``` Two concrete fixes: (1) emit one component-level headline when `componentConflict` holds and `conflictEdges` is empty for that component, e.g. `relation component {5h, weekly, weekly-fable} CONFLICTS jointly: 6 × 5h implies ≥$1049.48 but 0.5 × weekly implies ≤$978.77`, so the disjoint implied pair is named; (2) drop the duplicated reason so each suppressed edge prints its implied interval once. The `--json` lane already exposes `relation.conflict`, so only the human-facing path is misleading.
Author
Owner

Fixed in 65583f5: joint and non-convergent components now emit explicit component-level diagnostics, suppressed-edge wording is accurate, transitive implied ranges use fixed-point bounds, and the skill documents the intentional Max 20x provenance conflict.

<!-- gh-feedback:reply-to:42118 --> Fixed in 65583f5: joint and non-convergent components now emit explicit component-level diagnostics, suppressed-edge wording is accurate, transitive implied ranges use fixed-point bounds, and the skill documents the intentional Max 20x provenance conflict.
jercik marked this conversation as resolved
fix: clarify joint relation conflicts
Some checks failed
Node tests / node:test (pull_request) Successful in 16s
commit-msg / commitlint (pull_request) Successful in 27s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 3m0s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 36s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 8m11s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 8m31s
65583f5f65
Author
Owner

Replying to review #9782

The joint-conflict diagnostics, transitive relation reporting, and explicit provenance-conflict documentation are fixed in 65583f5.

> Replying to review #9782 The joint-conflict diagnostics, transitive relation reporting, and explicit provenance-conflict documentation are fixed in 65583f5.
Author
Owner

Replying to review #9786

The shipped dataset’s joint conflict is now named explicitly at component level, with accurate SUPPRESSED wording and documented provenance handling in 65583f5.

> Replying to review #9786 The shipped dataset’s joint conflict is now named explicitly at component level, with accurate SUPPRESSED wording and documented provenance handling in 65583f5.
jercik changed title from docs: record calibrated Claude usage ratios to fix: validate Claude usage-limit relations 2026-08-05 07:21:01 +00:00
forgejo-actions left a comment

Summary: Reviewed the relation fixed-point rewrite in estimate.ts plus the new plans.json calibrations and the SKILL.md documentation. I ran the new script against the bundled measurements, against the pre-PR plans.json, and against synthetic plans covering chained tightening, pairwise conflict, joint conflict, non-convergence, and a missing plans.json. The component discovery, bidirectional propagation, conflict fallback, and unvalidatedRelations reporting all behave as SKILL.md describes; the documented joint conflict for Max 20x reproduces exactly (6×·5h implies weekly $1049–$1068 while 0.5× fable implies $854–$979 — disjoint, with neither edge pairwise disjoint from its own measured bounds). No crashes or unsound propagation found.

Found 1 informational note about the shipped estimate widening and 3 low-severity items (missing provenance for one copied relation, an unrecorded price basis, and one misleading diagnostic line).

Code review by Claude Code Opus (opus)

**Summary:** Reviewed the relation fixed-point rewrite in `estimate.ts` plus the new `plans.json` calibrations and the SKILL.md documentation. I ran the new script against the bundled measurements, against the pre-PR `plans.json`, and against synthetic plans covering chained tightening, pairwise conflict, joint conflict, non-convergence, and a missing `plans.json`. The component discovery, bidirectional propagation, conflict fallback, and `unvalidatedRelations` reporting all behave as SKILL.md describes; the documented joint conflict for Max 20x reproduces exactly (6×·5h implies weekly $1049–$1068 while 0.5× fable implies $854–$979 — disjoint, with neither edge pairwise disjoint from its own measured bounds). No crashes or unsound propagation found. Found 1 informational note about the shipped estimate widening and 3 low-severity items (missing provenance for one copied relation, an unrecorded price basis, and one misleading diagnostic line). _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6IjY1NTgzZjVmNjU0MmMzMGRlMDdhMjg3ZjU1MWM1YTFhMDVkOTI3NTUiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE2ODcwIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6ImE4ZDE2YmNjLTNiOTctNGZkMy1hMzNhLWFiOTI1NTJkOTE3ZiJ9 -->
@ -7,1 +7,3 @@
"weekly": { "periodHours": 168 },
"weekly": {
"periodHours": 168,
"worth": { "fraction": 6, "of": "5h" }

ℹ️ Info: Worth stating explicitly in the PR: declaring this edge makes the shipped Max 20x weekly estimate wider, not tighter. I ran the new script against the pre-PR plans.json and against this one:

weekly estimate monthly equivalent
before $854.30–$978.77 (combined with weekly-fable = 0.5 × weekly) 18.6×–21.3×
after $768.87–$1084.04 (measured alone) 16.7×–23.6×

Because the 6× edge makes the component jointly unsatisfiable, every meter in {5h, weekly, weekly-fable} falls back to measured bounds, so the previously relation-tightened weekly figure is lost. SKILL.md documents this as deliberate ("preserve the component-level conflict"), so this is not a defect — just make sure the trade is intended, since the practical output of "record calibrated ratios" is a less precise headline number until the three inputs are reconciled.

ℹ️ **Info:** Worth stating explicitly in the PR: declaring this edge makes the shipped Max 20x `weekly` estimate **wider**, not tighter. I ran the new script against the pre-PR `plans.json` and against this one: | | weekly estimate | monthly equivalent | | --- | --- | --- | | before | $854.30–$978.77 (combined with `weekly-fable = 0.5 × weekly`) | 18.6×–21.3× | | after | $768.87–$1084.04 (measured alone) | 16.7×–23.6× | Because the 6× edge makes the component jointly unsatisfiable, every meter in `{5h, weekly, weekly-fable}` falls back to measured bounds, so the previously relation-tightened `weekly` figure is lost. SKILL.md documents this as deliberate ("preserve the component-level conflict"), so this is not a defect — just make sure the trade is intended, since the practical output of "record calibrated ratios" is a less precise headline number until the three inputs are reconciled.
Author
Owner

Addressed in 0d9ef97 and the updated PR description: the intentional Max 20x joint-conflict fallback widens the weekly headline from the prior relation-tightened range until the ratio, fable relation, and readings are reconciled.

<!-- gh-feedback:reply-to:42182 --> Addressed in 0d9ef97 and the updated PR description: the intentional Max 20x joint-conflict fallback widens the weekly headline from the prior relation-tightened range until the ratio, fable relation, and readings are reconciled.
jercik marked this conversation as resolved
@ -8,0 +15,4 @@
}
},
{
"plan": "claude-team-premium",

🟢 Low: The new claude-team-premium entry carries weekly-fable = {"fraction": 0.5, "of": "weekly"} (line 28), but the SKILL.md paragraph added in this PR only documents evidence for the two 5h ratios (6 × for Max 20x, 8 × for Team Premium) — nothing is cited for the fable ratio on this plan. It appears copied from the Max 20x block, and the Max 20x data in this same PR is what shows 0.5 and 6 × cannot both hold.

Today the estimator correctly lists it under Unvalidated declared relations because the plan has no measurements. But the moment the first Team Premium measurement lands, this edge becomes active and will silently tighten (or conflict with) the estimate without anyone having checked it. Either drop the fable entry from this plan until it is measured, or add its provenance to SKILL.md alongside the 8× ramp.

🟢 **Low:** The new `claude-team-premium` entry carries `weekly-fable = {"fraction": 0.5, "of": "weekly"}` (line 28), but the SKILL.md paragraph added in this PR only documents evidence for the two 5h ratios (`6 ×` for Max 20x, `8 ×` for Team Premium) — nothing is cited for the fable ratio on this plan. It appears copied from the Max 20x block, and the Max 20x data in this same PR is what shows `0.5` and `6 ×` cannot both hold. Today the estimator correctly lists it under `Unvalidated declared relations` because the plan has no measurements. But the moment the first Team Premium measurement lands, this edge becomes active and will silently tighten (or conflict with) the estimate without anyone having checked it. Either drop the fable entry from this plan until it is measured, or add its provenance to SKILL.md alongside the 8× ramp.
Author
Owner

Addressed in 0d9ef97: removed the copied Team Premium weekly-fable edge because no provenance or measurements validate that relation. The plan now records only the empirically supported 8× weekly-to-5h calibration; it can be added later with evidence.

<!-- gh-feedback:reply-to:42183 --> Addressed in 0d9ef97: removed the copied Team Premium weekly-fable edge because no provenance or measurements validate that relation. The plan now records only the empirically supported 8× weekly-to-5h calibration; it can be added later with evidence.
jercik marked this conversation as resolved
@ -8,0 +16,4 @@
},
{
"plan": "claude-team-premium",
"monthlyCostUsd": 125,

🟢 Low: Nothing in the repo records which billing basis monthlyCostUsd uses. claude-max-20x uses 200, the monthly-billing price for that plan; if 125 here is the annual-billed per-seat rate rather than the month-to-month rate, the × the $X/mo price multiples — the only place monthlyCostUsd is consumed (estimate.ts:536) — are computed on different bases and are not comparable between the two plans. Please confirm the figure and note the basis (monthly vs. annual-billed) in SKILL.md's plans.json paragraph so future plan entries stay consistent. I could not verify current Anthropic pricing from this environment (no network search available).

🟢 **Low:** Nothing in the repo records which billing basis `monthlyCostUsd` uses. `claude-max-20x` uses `200`, the monthly-billing price for that plan; if `125` here is the annual-billed per-seat rate rather than the month-to-month rate, the `× the $X/mo price` multiples — the only place `monthlyCostUsd` is consumed (`estimate.ts:536`) — are computed on different bases and are not comparable between the two plans. Please confirm the figure and note the basis (monthly vs. annual-billed) in SKILL.md's `plans.json` paragraph so future plan entries stay consistent. I could not verify current Anthropic pricing from this environment (no network search available).
Author
Owner

Addressed in 0d9ef97: SKILL.md now defines monthlyCostUsd as the monthly-equivalent denominator and says each plan must record whether it is month-to-month or annual-billing equivalent. The current Team Premium value remains an explicitly unverified working estimate.

<!-- gh-feedback:reply-to:42184 --> Addressed in 0d9ef97: SKILL.md now defines monthlyCostUsd as the monthly-equivalent denominator and says each plan must record whether it is month-to-month or annual-billing equivalent. The current Team Premium value remains an explicitly unverified working estimate.
jercik marked this conversation as resolved
@ -455,0 +607,4 @@
? `relation component {${meters}} DID NOT CONVERGE; using measured bounds`
: relation.componentConflict.reason === "joint"
? `relation component {${meters}} CONFLICTS jointly: no single edge is pairwise disjoint; using measured bounds`
: `relation component {${meters}} CONFLICTS: direct edge contradictions are marked below; using measured bounds`;

🟢 Low: The pairwise headline promises "direct edge contradictions are marked below", but it is printed once per meter in the component while the CONFLICTS-marked edges only appear in the constraint list of the meters incident to them. For a meter whose own edges are all SUPPRESSED, the header points at markers that are not below it.

Reproduced with a three-meter chain a ← b ← c where only b = 2 × a is pairwise disjoint:

p / c
  current: at least $498.75, 1 measurement in 1 session, 2026-08-01
    relation component {a, b, c} CONFLICTS: direct edge contradictions are marked below; using measured bounds
    relation c = 3 × b SUPPRESSED: implies $4987.50–$7518.75 (midpoint $6253.13)

Naming the contradicting edges in the header (they are already known via conflictEdges) would keep the pointer accurate for every meter in the component.

🟢 **Low:** The `pairwise` headline promises "direct edge contradictions are marked below", but it is printed once per meter in the component while the `CONFLICTS`-marked edges only appear in the constraint list of the meters incident to them. For a meter whose own edges are all `SUPPRESSED`, the header points at markers that are not below it. Reproduced with a three-meter chain `a ← b ← c` where only `b = 2 × a` is pairwise disjoint: ``` p / c current: at least $498.75, 1 measurement in 1 session, 2026-08-01 relation component {a, b, c} CONFLICTS: direct edge contradictions are marked below; using measured bounds relation c = 3 × b SUPPRESSED: implies $4987.50–$7518.75 (midpoint $6253.13) ``` Naming the contradicting edges in the header (they are already known via `conflictEdges`) would keep the pointer accurate for every meter in the component.
Author
Owner

Fixed in 0d9ef97: pairwise component diagnostics now name the direct conflicting relation edges in the component headline, so meters whose own edges are suppressed no longer point ambiguously to markers below.

<!-- gh-feedback:reply-to:42185 --> Fixed in 0d9ef97: pairwise component diagnostics now name the direct conflicting relation edges in the component headline, so meters whose own edges are suppressed no longer point ambiguously to markers below.
jercik marked this conversation as resolved
fix: clarify calibration caveats
Some checks failed
Node tests / node:test (pull_request) Successful in 16s
commit-msg / commitlint (pull_request) Successful in 21s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 3m3s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 41s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 7m49s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 8m10s
0d9ef9705a
Author
Owner

Replying to review #9798

Thanks — the fixed-point/component conflict behavior is validated. Addressed in 0d9ef97: removed the unvalidated Team Premium fable edge, documented the monthly-cost billing-basis caveat, named direct conflicting edges in pairwise component diagnostics, and recorded that the intentional Max 20x fallback widens the headline until the inputs are reconciled.

> Replying to review #9798 Thanks — the fixed-point/component conflict behavior is validated. Addressed in `0d9ef97`: removed the unvalidated Team Premium fable edge, documented the monthly-cost billing-basis caveat, named direct conflicting edges in pairwise component diagnostics, and recorded that the intentional Max 20x fallback widens the headline until the inputs are reconciled.
forgejo-actions left a comment

Summary: Reviewed the relation fixed-point solver against the bundled data and crafted inputs. The core change is sound: the pairwise, joint, and non-convergent paths all behave as documented, the propagation is a monotone greatest fixed point (so genuinely key-order independent), the conflict fallback restores measured bounds without leaking tightened values, and the 10n+1 iteration cap is comfortable for legitimate chains while safely catching divergent cycles. I confirmed the documented headline change on the shipped Max 20x data (weekly widens from $854.30–$978.77 to $768.87–$1084.04, 18.6×–21.3× → 16.7×–23.6×) is exactly what the code produces.

Found 1 medium and 3 low issues, plus one observation. The medium one is latent (the bundled data is unaffected): conflict suppression propagates through worth edges that have an unmeasured endpoint and therefore carry no constraint at all, discarding sound relation evidence in sub-graphs that are provably disconnected from the conflict.

Code review by Claude Code Opus (opus)

**Summary:** Reviewed the relation fixed-point solver against the bundled data and crafted inputs. The core change is sound: the pairwise, joint, and non-convergent paths all behave as documented, the propagation is a monotone greatest fixed point (so genuinely key-order independent), the conflict fallback restores measured bounds without leaking tightened values, and the 10n+1 iteration cap is comfortable for legitimate chains while safely catching divergent cycles. I confirmed the documented headline change on the shipped Max 20x data (`weekly` widens from $854.30–$978.77 to $768.87–$1084.04, 18.6×–21.3× → 16.7×–23.6×) is exactly what the code produces. Found 1 medium and 3 low issues, plus one observation. The medium one is latent (the bundled data is unaffected): conflict suppression propagates through `worth` edges that have an unmeasured endpoint and therefore carry no constraint at all, discarding sound relation evidence in sub-graphs that are provably disconnected from the conflict. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6IjBkOWVmOTcwNWEyYjJkNmFhZWYzY2FhZjBlMjA5YjJiMjQ4NTdhY2UiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE2ODkwIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6Ijk1M2JkZTFjLTA3YzktNGQyZC04NzRmLTMzMjYyNjM4YTBmMSJ9 -->
@ -32,1 +32,3 @@
`plans.json` describes each plan: its `monthlyCostUsd` and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without one — like `5h`, a throttle whose rate cap is independent of the weekly quotas — deliberately gets no monthly line; don't add a period to one. A meter's `worth: {"fraction": 0.5, "of": "weekly"}` declares a known cross-meter fact (its total is that fraction of a sibling meter's total, e.g. the fable meter is worth half the general weekly limit); the script propagates estimates through it in both directions automatically.
`plans.json` describes each plan: its `monthlyCostUsd` and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). `monthlyCostUsd` is the monthly-equivalent denominator used for the `× the $X/mo price` output; record whether it is a month-to-month price or an annual-billing per-seat equivalent before comparing plans. The current Team Premium value is a working estimate whose billing basis has not been independently verified. A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without `periodHours` — like `5h` — has no monthly equivalent in this estimator; it may still participate in a `worth` relation, so don't add a made-up period. A meter's `worth: {"fraction": 0.5, "of": "weekly"}` declares a cross-meter ratio (its total is that fraction of a sibling meter's total; for example, the fable meter is half the general weekly limit, while a plan can express `weekly = 6 × 5h` with `fraction: 6`). The estimator treats every `worth` entry as a bidirectional interval constraint and intersects each connected component to a fixed point. If a component conflicts or fails to converge, every measured meter in that component falls back to its measured bound. Direct pairwise contradictions are marked `CONFLICTS`; a joint contradiction is reported at the component level and its edges are `SUPPRESSED`, while a non-convergent component is named explicitly. This keeps the result independent of JSON key order. A relation with one or both meters unmeasured is reported as unvalidated instead of silently affecting an estimate.
The Claude ratio calibrations are empirical cross-meter facts: long, monotonically increasing Prometheus ramps from the `j4k/cluster` usage dashboards support approximately `weekly = 6 × 5h` for Claude Max 20x accounts (not Max 5x) and `weekly = 8 × 5h` for the Team Premium/VC account. A ramp is a contiguous reset-free sequence of whole-percent snapshots; aggregating its endpoints makes the two rounding errors small relative to the movement, so these relative ratios are better constrained than the absolute API-dollar totals. The cluster's capacity contract is `group_vars/all/ax_capacity.yml` and its mirrored regression fixture in `tests/test-axcredrouter-capacity-plan-values.py`. The raw Prometheus ramp snapshots are external evidence and are not currently copied into this estimator repository. The bundled Max 20x measurements currently expose a joint conflict: the 6× ramp, the 0.5× fable relation, and the fable readings cannot all be true. Do not edit measurements to force the calibration; preserve the component-level conflict until those three inputs are reconciled, then revise the weaker absolute estimate.

ℹ️ Info: I confirmed the joint conflict is real and correctly classified — 6 × 5h forces weekly into $1049.48–$1067.75 while weekly-fable = 0.5 × weekly forces it into $854.30–$978.77, disjoint, with every individual edge still overlapping its measured bound. The joint label is right.

One candidate explanation worth adding to the three listed here before anyone reconciles the inputs: every bundled 5h line is annotated fable-only usage, whereas the calibration comes from general j4k/cluster ramps. If the 5h throttle and the weekly quota weight fable spend differently, the ratio would not be expected to hold on these windows at all — which would make the conflict an artifact of measurement provenance rather than of any of the three inputs being wrong. No action needed on this PR; it just narrows what the eventual reconciliation should look for.

ℹ️ **Info:** I confirmed the joint conflict is real and correctly classified — `6 × 5h` forces `weekly` into $1049.48–$1067.75 while `weekly-fable = 0.5 × weekly` forces it into $854.30–$978.77, disjoint, with every individual edge still overlapping its measured bound. The `joint` label is right. One candidate explanation worth adding to the three listed here before anyone reconciles the inputs: every bundled `5h` line is annotated `fable-only usage`, whereas the `6×` calibration comes from general `j4k/cluster` ramps. If the `5h` throttle and the `weekly` quota weight fable spend differently, the `6×` ratio would not be expected to hold on *these* windows at all — which would make the conflict an artifact of measurement provenance rather than of any of the three inputs being wrong. No action needed on this PR; it just narrows what the eventual reconciliation should look for.
jercik marked this conversation as resolved
@ -54,3 +56,3 @@
- The headline per meter is the estimate under the assumption that the display uses one rounding mode consistently; a 1-point delta yields only a lower bound ("at least $X"). The `mode-agnostic bound` line is the wider interval the data proves without that assumption, and `per-mode` shows each rounding mode's interval (∅ = that mode cannot explain the data), making near-miss eliminations visible. A "Display rounding: data rules out …" header reports which modes the exact-start readings eliminated; the warning that no mode fits means a `startExact` claim is suspect — check with the user whether that window truly began at a reset.
- `combined with relation` means a declared `worth` relation tightened the estimate beyond the series' own measurements; `measured alone` preserves the unassisted bound. A relation CONFLICT means the relation and the measurements cannot both be right — surface it rather than smoothing over it.
- `combined with relation` means the fixed-point intersection of all applicable `worth` relations tightened the estimate beyond the series' own measurements; `measured alone` preserves the unassisted bound. When a relation component conflicts or fails to converge, the affected meters fall back to measured bounds. `CONFLICTS` marks an edge whose own measured intervals are disjoint; the component headline names those direct edges. `SUPPRESSED` means the edge belongs to a component that is jointly unsatisfiable or did not converge, with no individual edge blamed. `Unvalidated declared relations` means one or both meters have no measurements, so the ratio is recorded but not yet checked by this tool.

🟢 Low: "SUPPRESSED means the edge belongs to a component that is jointly unsatisfiable or did not converge, with no individual edge blamed" doesn't match the implementation. suppressed is set for any edge in a conflicting component that isn't itself in conflictEdges, including components classified pairwise — where an individual edge is blamed. Observed on a pairwise-conflicting component:

p / b
    relation component {a, b, c} CONFLICTS: direct edges b = 2 × a; using measured bounds
    relation b = 2 × a CONFLICTS: implies $181.36–$222.78 (midpoint $202.07)
    relation c = 2 × b SUPPRESSED: implies at least $249.38

A reader following this sentence would conclude the component had no blamed edge. Suggest wording along the lines of: SUPPRESSED marks an edge that is not itself contradictory but sits in a component whose bounds were discarded — whether the contradiction was pinned to a specific edge or not.

🟢 **Low:** "`SUPPRESSED` means the edge belongs to a component that is jointly unsatisfiable or did not converge, with no individual edge blamed" doesn't match the implementation. `suppressed` is set for any edge in a conflicting component that isn't itself in `conflictEdges`, including components classified `pairwise` — where an individual edge *is* blamed. Observed on a pairwise-conflicting component: ``` p / b relation component {a, b, c} CONFLICTS: direct edges b = 2 × a; using measured bounds relation b = 2 × a CONFLICTS: implies $181.36–$222.78 (midpoint $202.07) relation c = 2 × b SUPPRESSED: implies at least $249.38 ``` A reader following this sentence would conclude the component had no blamed edge. Suggest wording along the lines of: `SUPPRESSED` marks an edge that is not itself contradictory but sits in a component whose bounds were discarded — whether the contradiction was pinned to a specific edge or not.
Author
Owner

Fixed in 4f22a3a: SKILL.md now defines SUPPRESSED as a non-contradictory edge in a component whose propagated bounds were discarded, whether the component has a blamed direct edge or only a joint/non-convergent failure.

<!-- gh-feedback:reply-to:42222 --> Fixed in 4f22a3a: SKILL.md now defines SUPPRESSED as a non-contradictory edge in a component whose propagated bounds were discarded, whether the component has a blamed direct edge or only a joint/non-convergent failure.
jercik marked this conversation as resolved
@ -12,1 +15,4 @@
}
},
{
"plan": "claude-team-premium",

🟢 Low: This is the first entry that makes plans.json a multi-plan array, and parsePlans never checks that plan names are unique. A duplicated name fails silently in three ways, which I verified with a two-entry file both named d:

  • Nodes are keyed [plan, meter], so both entries' worth edges land in one component — output showed relation b = 2 × a and relation b = 5 × a as siblings under a single component {a, b} conflict.
  • edgeKey is [plan, meter, of], so the two edges share a key; conflictEdges cannot distinguish them, and blaming one marks both.
  • monthlyFor resolves through plans.find, so only the first entry's monthlyCostUsd and periodHours are ever used.

Given the file's stated "typos fail loudly" contract, a duplicate-name check in parsePlans alongside the existing plan/meters validation would close this.

🟢 **Low:** This is the first entry that makes `plans.json` a multi-plan array, and `parsePlans` never checks that `plan` names are unique. A duplicated name fails silently in three ways, which I verified with a two-entry file both named `d`: - Nodes are keyed `[plan, meter]`, so both entries' `worth` edges land in one component — output showed `relation b = 2 × a` and `relation b = 5 × a` as siblings under a single `component {a, b}` conflict. - `edgeKey` is `[plan, meter, of]`, so the two edges share a key; `conflictEdges` cannot distinguish them, and blaming one marks both. - `monthlyFor` resolves through `plans.find`, so only the first entry's `monthlyCostUsd` and `periodHours` are ever used. Given the file's stated "typos fail loudly" contract, a duplicate-name check in `parsePlans` alongside the existing `plan`/`meters` validation would close this.
Author
Owner

Fixed in 4f22a3a: parsePlans now rejects duplicate plan names before relation construction, preventing cross-plan component/key collisions and ambiguous price resolution.

<!-- gh-feedback:reply-to:42223 --> Fixed in 4f22a3a: parsePlans now rejects duplicate plan names before relation construction, preventing cross-plan component/key collisions and ambiguous price resolution.
jercik marked this conversation as resolved
@ -360,0 +386,4 @@
type RelationComponent = { id: number; defs: RelationDef[]; nodes: Set<string> };
const relationAdjacency = new Map<string, number[]>();
for (let index = 0; index < relationDefs.length; index++) {

🟡 Medium: Connected components are built from every worth edge, including edges whose endpoints have no measurements. The fixed-point loop skips those edges entirely (if (!meter || !of) continue;), so they propagate nothing — but they still fuse otherwise-independent sub-graphs, and a conflict anywhere in the fused component discards sound relation evidence everywhere in it.

Reproduced with meters w,x,y,z,v, edges x=2×w, y=3×x, z=4×y, v=2×z, measuring everything except y:

i / v
  current: at least $99.75, 1 measurement in 1 session, 2026-08-01
    relation component {v, w, x, y, z} CONFLICTS: direct edges x = 2 × w; using measured bounds
    relation v = 2 × z SUPPRESSED: implies $181.36–$222.78 (midpoint $202.07)

v = 2 × z is consistent and would have tightened v from "at least $99.75" to $181.36–$222.78, but it is suppressed by a conflict it cannot be connected to — the only path runs through unmeasured y. The component headline also names y, a meter with no measurements.

Fix: register an edge in relationAdjacency only when both nodeKey(def.plan, def.meter) and nodeKey(def.plan, def.of) are present in measuredBounds. Those edges are already surfaced separately through unvalidatedRelations, so nothing is lost, and the comment's justification ("bounds propagated to a neighbour are not sound once any edge is impossible") still holds for the edges that actually propagate. The bundled Max 20x data has all three meters measured, so this is latent rather than currently visible.

🟡 **Medium:** Connected components are built from *every* `worth` edge, including edges whose endpoints have no measurements. The fixed-point loop skips those edges entirely (`if (!meter || !of) continue;`), so they propagate nothing — but they still fuse otherwise-independent sub-graphs, and a conflict anywhere in the fused component discards sound relation evidence everywhere in it. Reproduced with meters `w,x,y,z,v`, edges `x=2×w`, `y=3×x`, `z=4×y`, `v=2×z`, measuring everything except `y`: ``` i / v current: at least $99.75, 1 measurement in 1 session, 2026-08-01 relation component {v, w, x, y, z} CONFLICTS: direct edges x = 2 × w; using measured bounds relation v = 2 × z SUPPRESSED: implies $181.36–$222.78 (midpoint $202.07) ``` `v = 2 × z` is consistent and would have tightened `v` from "at least $99.75" to $181.36–$222.78, but it is suppressed by a conflict it cannot be connected to — the only path runs through unmeasured `y`. The component headline also names `y`, a meter with no measurements. Fix: register an edge in `relationAdjacency` only when both `nodeKey(def.plan, def.meter)` and `nodeKey(def.plan, def.of)` are present in `measuredBounds`. Those edges are already surfaced separately through `unvalidatedRelations`, so nothing is lost, and the comment's justification ("bounds propagated to a neighbour are not sound once any edge is impossible") still holds for the edges that actually propagate. The bundled Max 20x data has all three meters measured, so this is latent rather than currently visible.
Author
Owner

Fixed in 4f22a3a: relation adjacency and component discovery now include only edges whose two endpoints have measured bounds. Unvalidated edges remain listed separately, so a conflict cannot suppress sound evidence across an unmeasured bridge.

<!-- gh-feedback:reply-to:42221 --> Fixed in 4f22a3a: relation adjacency and component discovery now include only edges whose two endpoints have measured bounds. Unvalidated edges remain listed separately, so a conflict cannot suppress sound evidence across an unmeasured bridge.
jercik marked this conversation as resolved
@ -447,3 +600,3 @@
const measured = `${segmentsText(era.refined)}, ${counted(era.measurementCount, "measurement")} in ${counted(era.sessionCount, "session")}, ${span}${suspicion}`;
if (i === 0 && relation?.combined) {
console.log(` ${label}: ${rangeText(relation.combined)} — combined with relation ${relation.text}`);
const relationText = relation.constraints.map((constraint) => constraint.text).join("; ");

🟢 Low: This const relationText shadows the module-level relationText(def) helper for the whole enclosing block. It works today only because nothing in the block references the helper, but the script runs under Node's type-stripping with no compile step, so a later call to the helper anywhere in this block becomes a TDZ ReferenceError at runtime rather than a build failure. Renaming this local (e.g. relationsText) removes the trap.

🟢 **Low:** This `const relationText` shadows the module-level `relationText(def)` helper for the whole enclosing block. It works today only because nothing in the block references the helper, but the script runs under Node's type-stripping with no compile step, so a later call to the helper anywhere in this block becomes a TDZ `ReferenceError` at runtime rather than a build failure. Renaming this local (e.g. `relationsText`) removes the trap.
Author
Owner

Fixed in 4f22a3a: renamed the local rendered-relations variable so it no longer shadows the module-level relationText helper.

<!-- gh-feedback:reply-to:42224 --> Fixed in 4f22a3a: renamed the local rendered-relations variable so it no longer shadows the module-level relationText helper.
jercik marked this conversation as resolved
fix: isolate measured relation components
Some checks failed
Node tests / node:test (pull_request) Successful in 17s
commit-msg / commitlint (pull_request) Successful in 27s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 1m50s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 55s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 8m56s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 9m16s
4f22a3a309
Author
Owner

Replying to review #9803

Thanks — the solver stress tests and the intentional Max 20x widening check out. Addressed in 4f22a3a: measured relation components now exclude edges with unmeasured endpoints, duplicate plan names fail loudly, the relation-text helper is no longer shadowed, and the skill clarifies suppressed-edge semantics plus the fable-only/general-ramp provenance caveat.

> Replying to review #9803 Thanks — the solver stress tests and the intentional Max 20x widening check out. Addressed in `4f22a3a`: measured relation components now exclude edges with unmeasured endpoints, duplicate plan names fail loudly, the relation-text helper is no longer shadowed, and the skill clarifies suppressed-edge semantics plus the fable-only/general-ramp provenance caveat.
forgejo-actions left a comment

Summary: No correctness defects found. I ran the estimator against the bundled data and against synthetic plans to exercise the new propagation code: chained tightening, component isolation, pairwise vs. joint conflict classification, the non-convergence cap, and the unmeasured-endpoint path all behave as SKILL.md describes, and the conflict fallback keeps the reported bounds sound (measured hull) rather than propagating unsound relation bounds.

Measured effect of the new weekly = 6 × 5h edge on the shipped dataset: Max 20x weekly goes from $854.30–$978.77 (18.6×–21.3×) to $768.87–$1084.04 (16.7×–23.6×), i.e. the previously valid 0.5× fable tightening is discarded along with the conflicting component — which matches what the PR description says it intends.

3 low/info notes below, all on output provenance and diagnostics rather than on the estimates themselves.

Code review by Claude Code Opus (opus)

**Summary:** No correctness defects found. I ran the estimator against the bundled data and against synthetic plans to exercise the new propagation code: chained tightening, component isolation, pairwise vs. joint conflict classification, the non-convergence cap, and the unmeasured-endpoint path all behave as SKILL.md describes, and the conflict fallback keeps the reported bounds sound (measured hull) rather than propagating unsound relation bounds. Measured effect of the new `weekly = 6 × 5h` edge on the shipped dataset: Max 20x `weekly` goes from $854.30–$978.77 (18.6×–21.3×) to $768.87–$1084.04 (16.7×–23.6×), i.e. the previously valid 0.5× fable tightening is discarded along with the conflicting component — which matches what the PR description says it intends. 3 low/info notes below, all on output provenance and diagnostics rather than on the estimates themselves. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6IjRmMjJhM2EzMDljMGUyMTkwMDljZWIyMWU3Yzg2ZjhhMTFmZDYzNTkiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE2OTEzIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6ImU3YjQ1M2Y1LTM2MGUtNGI2NC05M2Y1LTZhYWE3ZmI5MjgzMyJ9 -->
@ -7,1 +7,3 @@
"weekly": { "periodHours": 168 },
"weekly": {
"periodHours": 168,
"worth": { "fraction": 6, "of": "5h" }

ℹ️ Info: Quantifying the joint conflict this edge introduces, in case it helps the reconciliation the SKILL.md asks for: the three bundled inputs are only ~2.3% away from being mutually consistent. The conflict is still reported at --usd-tolerance 2 and disappears at --usd-tolerance 2.5 (where weekly tightens to $993.69–$1005.62 and 5h to $165.61–$167.60). The binding gap is the implied weekly-fable = 3 × 5h: the measured hulls allow a fable/5h ratio of at most 489.39/174.91 ≈ 2.80 against the 3.00 the two edges require. So the contradiction is a few percent, not an order-of-magnitude input error — consistent with the "meters may weight fable spend differently" hypothesis in the doc. No action needed; the fallback is sound and the widening is documented.

ℹ️ **Info:** Quantifying the joint conflict this edge introduces, in case it helps the reconciliation the SKILL.md asks for: the three bundled inputs are only ~2.3% away from being mutually consistent. The conflict is still reported at `--usd-tolerance 2` and disappears at `--usd-tolerance 2.5` (where `weekly` tightens to $993.69–$1005.62 and `5h` to $165.61–$167.60). The binding gap is the implied `weekly-fable = 3 × 5h`: the measured hulls allow a fable/5h ratio of at most 489.39/174.91 ≈ 2.80 against the 3.00 the two edges require. So the contradiction is a few percent, not an order-of-magnitude input error — consistent with the "meters may weight fable spend differently" hypothesis in the doc. No action needed; the fallback is sound and the widening is documented.
jercik marked this conversation as resolved
@ -13,0 +16,4 @@
},
{
"plan": "claude-team-premium",
"monthlyCostUsd": 125,

🟢 Low: SKILL.md now instructs the reader to "record whether it is a month-to-month price or an annual-billing per-seat equivalent", but the plan schema has nowhere to record it — parsePlans accepts only plan, monthlyCostUsd, and meters, and the output line prints a bare × the $125/mo price. If this 125 is an annual-billing per-seat equivalent while the Max 20x 200 is a month-to-month price, the × multiples are on different bases and are not comparable across plans, and nothing in the tool's output says so. Consider an optional costBasis string on the plan that the monthly line echoes (e.g. × the $125/mo price (annual per-seat)), so the caveat travels with the number instead of living only in prose.

🟢 **Low:** SKILL.md now instructs the reader to "record whether it is a month-to-month price or an annual-billing per-seat equivalent", but the plan schema has nowhere to record it — `parsePlans` accepts only `plan`, `monthlyCostUsd`, and `meters`, and the output line prints a bare `× the $125/mo price`. If this 125 is an annual-billing per-seat equivalent while the Max 20x 200 is a month-to-month price, the `×` multiples are on different bases and are not comparable across plans, and nothing in the tool's output says so. Consider an optional `costBasis` string on the plan that the monthly line echoes (e.g. `× the $125/mo price (annual per-seat)`), so the caveat travels with the number instead of living only in prose.
Author
Owner

Fixed in d5e34a4: plans now carry an optional costBasis, the two committed entries identify their basis, the parser validates it, and monthly-equivalent output echoes it with the price so cross-plan comparisons do not lose that context.

<!-- gh-feedback:reply-to:42252 --> Fixed in d5e34a4: plans now carry an optional costBasis, the two committed entries identify their basis, the parser validates it, and monthly-equivalent output echoes it with the price so cross-plan comparisons do not lose that context.
jercik marked this conversation as resolved
@ -364,0 +434,4 @@
for (const component of relationComponents) {
let componentConflict = false;
let converged = false;
for (let iteration = 0; iteration < component.defs.length * 10 + 1; iteration++) {

🟢 Low: The 10 × |defs| + 1 cap turns slowly-shrinking contradictions into a DID NOT CONVERGE diagnostic rather than a conflict. Reproduced with a two-edge cycle a = 1.01 × b, b = 1 × a over overlapping measurements: the component is unsatisfiable (any fixed point needs a = 1.01·a), but the 1% per-sweep shrink needs ~110 sweeps to empty out against a cap of 21, so the user is told the solver gave up instead of that the relations contradict each other. Since a consistent component reaches its fixed point in at most a component-diameter of sweeps, hitting the cap always means a contradiction; classifying the cap case with the same pairwise/joint logic (or scaling the cap by the interval width ratio) would report it accurately. Also worth noting for the SKILL.md "independent of JSON key order" claim: the number of sweeps depends on component.defs order, so the label can flip with key order near the cap even though the reported bounds cannot.

🟢 **Low:** The `10 × |defs| + 1` cap turns slowly-shrinking *contradictions* into a `DID NOT CONVERGE` diagnostic rather than a conflict. Reproduced with a two-edge cycle `a = 1.01 × b`, `b = 1 × a` over overlapping measurements: the component is unsatisfiable (any fixed point needs a = 1.01·a), but the 1% per-sweep shrink needs ~110 sweeps to empty out against a cap of 21, so the user is told the solver gave up instead of that the relations contradict each other. Since a consistent component reaches its fixed point in at most a component-diameter of sweeps, hitting the cap always means a contradiction; classifying the cap case with the same pairwise/joint logic (or scaling the cap by the interval width ratio) would report it accurately. Also worth noting for the SKILL.md "independent of JSON key order" claim: the number of sweeps depends on `component.defs` order, so the *label* can flip with key order near the cap even though the reported bounds cannot.
Author
Owner

Fixed in d5e34a4: iteration-cap exhaustion is now classified through the same pairwise/joint contradiction logic and the component output explicitly notes that the solver did not converge before classifying the contradiction.

<!-- gh-feedback:reply-to:42254 --> Fixed in d5e34a4: iteration-cap exhaustion is now classified through the same pairwise/joint contradiction logic and the component output explicitly notes that the solver did not converge before classifying the contradiction.
jercik marked this conversation as resolved
@ -447,3 +603,3 @@
const measured = `${segmentsText(era.refined)}, ${counted(era.measurementCount, "measurement")} in ${counted(era.sessionCount, "session")}, ${span}${suspicion}`;
if (i === 0 && relation?.combined) {
console.log(` ${label}: ${rangeText(relation.combined)} — combined with relation ${relation.text}`);
const relationsText = relation.constraints.map((constraint) => constraint.text).join("; ");

🟢 Low: This header credits every relation incident to the meter, not the ones that actually tightened it, so the provenance it reports can be wrong in both directions. With a chain b = 2 × a, c = 2 × b and measurements a=(90.68,111.39), b=(166.25,250.63), c=(332.50,1002.50) the tool prints for b: combined with relation b = 2 × a; c = 2 × b — but c/2 = (166.25, 501.25) tightens nothing, only the a edge did. Conversely c's headline ($362.73–$445.56) is credited to c = 2 × b alone even though the value comes from a propagated through b. Filtering relation.constraints to those whose combined actually tightens(own) — or naming the component instead of the edge list — would keep the line honest about where the number came from.

🟢 **Low:** This header credits every relation incident to the meter, not the ones that actually tightened it, so the provenance it reports can be wrong in both directions. With a chain `b = 2 × a`, `c = 2 × b` and measurements a=(90.68,111.39), b=(166.25,250.63), c=(332.50,1002.50) the tool prints for `b`: `combined with relation b = 2 × a; c = 2 × b` — but `c/2 = (166.25, 501.25)` tightens nothing, only the `a` edge did. Conversely `c`'s headline ($362.73–$445.56) is credited to `c = 2 × b` alone even though the value comes from `a` propagated through `b`. Filtering `relation.constraints` to those whose `combined` actually `tightens(own)` — or naming the component instead of the edge list — would keep the line honest about where the number came from.
Author
Owner

Fixed in d5e34a4: combined headlines now name the relation component rather than attributing a chained fixed-point value to every incident edge.

<!-- gh-feedback:reply-to:42253 --> Fixed in d5e34a4: combined headlines now name the relation component rather than attributing a chained fixed-point value to every incident edge.
jercik marked this conversation as resolved
fix: carry relation provenance in output
Some checks failed
Node tests / node:test (pull_request) Successful in 8s
commit-msg / commitlint (pull_request) Successful in 11s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 1m27s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 33s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 7m39s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 8m3s
d5e34a4997
Author
Owner

Replying to review #9809

Thanks — the solver and isolation checks are clean. Addressed in d5e34a4: cost basis now travels through the plan schema and monthly output, combined headlines name the relation component rather than over-crediting edges, and iteration-cap exhaustion is classified as a contradiction with an explicit non-convergence note.

> Replying to review #9809 Thanks — the solver and isolation checks are clean. Addressed in `d5e34a4`: cost basis now travels through the plan schema and monthly output, combined headlines name the relation component rather than over-crediting edges, and iteration-cap exhaustion is classified as a contradiction with an explicit non-convergence note.
forgejo-actions left a comment

Summary: Reviewed the relation solver, plan config, and docs. I ran the estimator on the bundled data and on synthetic fixtures covering: a relation that tightens, a chained 3-meter component (order-independent propagation confirmed), a direct pairwise conflict, a joint conflict, an inconsistent cycle, and iteration-cap exhaustion. The fixed-point solver, component isolation, conflict/suppression classification, non-convergence note, and the unvalidatedRelations list all behave as documented; no crashes or ordering dependence found. The bundled Max 20x joint conflict reproduces exactly as the PR body describes.

Found 1 medium doc/behaviour mismatch, 1 low config nit, and 1 informational note.

Code review by Claude Code Opus (opus)

**Summary:** Reviewed the relation solver, plan config, and docs. I ran the estimator on the bundled data and on synthetic fixtures covering: a relation that tightens, a chained 3-meter component (order-independent propagation confirmed), a direct pairwise conflict, a joint conflict, an inconsistent cycle, and iteration-cap exhaustion. The fixed-point solver, component isolation, conflict/suppression classification, non-convergence note, and the `unvalidatedRelations` list all behave as documented; no crashes or ordering dependence found. The bundled Max 20x joint conflict reproduces exactly as the PR body describes. Found 1 medium doc/behaviour mismatch, 1 low config nit, and 1 informational note. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6ImQ1ZTM0YTQ5OTdmNzBhODgxZWY3YjgyZTM5ZTg5OTZjOTY2OTFhNWMiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE2OTQ2Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6ImFhYjAyNGZiLTQ1YTEtNGM2My1iZThiLTFlNTM5N2Y3ZTRkYyJ9 -->
@ -7,1 +8,3 @@
"weekly": { "periodHours": 168 },
"weekly": {
"periodHours": 168,
"worth": { "fraction": 6, "of": "5h" }

ℹ️ Info: Verified that the joint conflict this edge creates is not a rounding-tolerance artifact. With the fable relation applied, the bundled data pins weekly to $854.30–$978.77 and 5h to $174.91–$177.96, so the measurements support a ratio of roughly 4.8×–5.6× — the 6× calibration sits about 10% outside that. The conflict also persists at --usd-tolerance 0, 0.5, and 2, so no plausible widening of the dollar tolerance reconciles it. That supports the PR's decision to preserve the conflict rather than adjust inputs; no change requested.

ℹ️ **Info:** Verified that the joint conflict this edge creates is not a rounding-tolerance artifact. With the fable relation applied, the bundled data pins `weekly` to $854.30–$978.77 and `5h` to $174.91–$177.96, so the measurements support a ratio of roughly 4.8×–5.6× — the 6× calibration sits about 10% outside that. The conflict also persists at `--usd-tolerance 0`, `0.5`, and `2`, so no plausible widening of the dollar tolerance reconciles it. That supports the PR's decision to preserve the conflict rather than adjust inputs; no change requested.
jercik marked this conversation as resolved
@ -13,0 +18,4 @@
{
"plan": "claude-team-premium",
"monthlyCostUsd": 125,
"costBasis": "unverified working estimate",

🟢 Low: costBasis is documented at SKILL.md:32 as the field that lets comparisons "distinguish month-to-month from annual-billing per-seat equivalents" — i.e. it names a billing basis, as "month-to-month" does on line 5. "unverified working estimate" is a verification status, not a basis, so the one field now carries two different kinds of information and renders as … × the $125/mo price, unverified working estimate. Consider stating the presumed basis and keeping the caveat separate, e.g. "annual per-seat, unverified", so the value still answers the question the field is documented to answer.

🟢 **Low:** `costBasis` is documented at `SKILL.md:32` as the field that lets comparisons "distinguish month-to-month from annual-billing per-seat equivalents" — i.e. it names a billing basis, as `"month-to-month"` does on line 5. `"unverified working estimate"` is a verification status, not a basis, so the one field now carries two different kinds of information and renders as `… × the $125/mo price, unverified working estimate`. Consider stating the presumed basis and keeping the caveat separate, e.g. `"annual per-seat, unverified"`, so the value still answers the question the field is documented to answer.
Author
Owner

Fixed in c570e94: Team Premium costBasis now reads annual per-seat (unverified), preserving the presumed billing basis while making the uncertainty explicit.

<!-- gh-feedback:reply-to:42316 --> Fixed in c570e94: Team Premium costBasis now reads annual per-seat (unverified), preserving the presumed billing basis while making the uncertainty explicit.
jercik marked this conversation as resolved
@ -447,3 +617,3 @@
const measured = `${segmentsText(era.refined)}, ${counted(era.measurementCount, "measurement")} in ${counted(era.sessionCount, "session")}, ${span}${suspicion}`;
if (i === 0 && relation?.combined) {
console.log(` ${label}: ${rangeText(relation.combined)} — combined with relation ${relation.text}`);
console.log(` ${label}: ${rangeText(relation.combined)} — combined with relation component`);

🟡 Medium: This headline prints the fixed string combined with relation component — it does not identify anything. SKILL.md:58 states "The headline names the relation component because chained bounds may be supported by more than one path", so the docs describe output the tool does not produce.

It is also a diagnostic regression: the old line was combined with relation ${relation.text}, which named the relation that tightened the bound. In the new code the per-edge relation … implies … lines are printed only in the else branch (the conflict path), so on the success path relation.constraints is never rendered in text mode at all. Observed on a synthetic chain a–b–c where c drives both a and b:

p / a
  current: $90.68–$111.39 (midpoint $101.04) — combined with relation component
    measured alone: $66.50–$200.50 (midpoint $133.50), 1 measurement in 1 session, 2026-01-01

Nothing tells the reader that the bound came from c via b. Either name the component here the way the conflict branch already does (it formats {5h, weekly, weekly-fable} from relation.componentConflict.meters, and relationComponentByNode has the node set for the non-conflicting case too), or also print the relation.constraints lines on this branch — and align the SKILL.md:58 sentence with whichever you pick.

🟡 **Medium:** This headline prints the fixed string `combined with relation component` — it does not identify anything. `SKILL.md:58` states "The headline names the relation component because chained bounds may be supported by more than one path", so the docs describe output the tool does not produce. It is also a diagnostic regression: the old line was `combined with relation ${relation.text}`, which named the relation that tightened the bound. In the new code the per-edge `relation … implies …` lines are printed only in the `else` branch (the conflict path), so on the success path `relation.constraints` is never rendered in text mode at all. Observed on a synthetic chain a–b–c where `c` drives both `a` and `b`: ``` p / a current: $90.68–$111.39 (midpoint $101.04) — combined with relation component measured alone: $66.50–$200.50 (midpoint $133.50), 1 measurement in 1 session, 2026-01-01 ``` Nothing tells the reader that the bound came from `c` via `b`. Either name the component here the way the conflict branch already does (it formats `{5h, weekly, weekly-fable}` from `relation.componentConflict.meters`, and `relationComponentByNode` has the node set for the non-conflicting case too), or also print the `relation.constraints` lines on this branch — and align the `SKILL.md:58` sentence with whichever you pick.
Author
Owner

Fixed in c570e94: successful relation headlines now include the measured relation component names, e.g. {a, b, c}, so chained propagation has explicit provenance.

<!-- gh-feedback:reply-to:42315 --> Fixed in c570e94: successful relation headlines now include the measured relation component names, e.g. {a, b, c}, so chained propagation has explicit provenance.
jercik marked this conversation as resolved
fix: identify relation components in output
Some checks failed
Node tests / node:test (pull_request) Successful in 13s
commit-msg / commitlint (pull_request) Successful in 22s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 2m23s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 50s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 8m38s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 8m55s
c570e94b3b
Author
Owner

Replying to review #9815

Addressed in c570e94: successful relation headlines now name the measured relation component, and Team Premium costBasis now records the presumed annual per-seat basis with the estimate explicitly marked unverified. The informational conflict/tolerance finding is acknowledged; no input values were changed.

> Replying to review #9815 Addressed in c570e94: successful relation headlines now name the measured relation component, and Team Premium costBasis now records the presumed annual per-seat basis with the estimate explicitly marked unverified. The informational conflict/tolerance finding is acknowledged; no input values were changed.
forgejo-actions left a comment

Summary: Reviewed the relation solver, the plan/cost-basis plumbing, and the documentation. I ran the estimator against the bundled data and against synthetic plans covering tightening chains, pairwise conflicts, joint conflicts, divergent cycles, partially measured relations, and reversed JSON key order: the fixed point is order-independent, conflicts fall back to measured bounds, and the component/edge diagnostics match what SKILL.md describes. tsc --strict on the script is clean and --json output stays valid.

Found 1 medium issue (an approximate calibration propagated as an exact constraint) plus 1 observation.

Code review by Claude Code Opus (opus)

**Summary:** Reviewed the relation solver, the plan/cost-basis plumbing, and the documentation. I ran the estimator against the bundled data and against synthetic plans covering tightening chains, pairwise conflicts, joint conflicts, divergent cycles, partially measured relations, and reversed JSON key order: the fixed point is order-independent, conflicts fall back to measured bounds, and the component/edge diagnostics match what SKILL.md describes. `tsc --strict` on the script is clean and `--json` output stays valid. Found 1 medium issue (an approximate calibration propagated as an exact constraint) plus 1 observation. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6ImM1NzBlOTRiM2IwNjU1MDdhMDZhOTE1YjBlYmRkYWRmMWJlZDEyNTQiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE2OTU5Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjI4YmVlODgyLTczNDctNGUyMi1iMmJkLTg3YTk2N2Y4ZDBjZSJ9 -->
@ -7,1 +8,3 @@
"weekly": { "periodHours": 168 },
"weekly": {
"periodHours": 168,
"worth": { "fraction": 6, "of": "5h" }

🟡 Medium: fraction is propagated as an exact equality, but SKILL.md describes these calibrations as approximate ("support approximately weekly = 6 × 5h"). The solver intersects scaled(other, fraction) with the measured bound with no slack, so a ratio that is only approximately right either manufactures a conflict or, worse, silently over-tightens the headline. On the bundled Max 20x data the effect is currently the conservative one (joint conflict → measured fallback), so this does not misreport today, but the Team Premium fraction: 8 on line 26 will start tightening as soon as measurements for that plan land, and I measured how large that effect is: on a synthetic two-meter plan an exact fraction: 2 narrowed the reported bound from $90.68–$111.39 to $99.75–$111.39 — a 44% narrower interval, presented as a proven bound and then fed into monthlyFor as the × the $X/mo price multiple.

The script already models measurement uncertainty explicitly (--usd-tolerance widens every dollar interval), so a ratio derived from rounded percent ramps having zero declared uncertainty is inconsistent with the rest of the model. Either give worth an optional tolerance that widens the implied interval before intersecting (e.g. implied = {lo: other.lo * f * (1 - t), hi: other.hi * f * (1 + t)}), or state in SKILL.md that worth fractions must be exactly-known ratios and that empirical calibrations belong somewhere that cannot tighten a headline.

🟡 **Medium:** `fraction` is propagated as an *exact* equality, but SKILL.md describes these calibrations as approximate ("support **approximately** `weekly = 6 × 5h`"). The solver intersects `scaled(other, fraction)` with the measured bound with no slack, so a ratio that is only approximately right either manufactures a conflict or, worse, silently over-tightens the headline. On the bundled Max 20x data the effect is currently the conservative one (joint conflict → measured fallback), so this does not misreport today, but the Team Premium `fraction: 8` on line 26 will start tightening as soon as measurements for that plan land, and I measured how large that effect is: on a synthetic two-meter plan an exact `fraction: 2` narrowed the reported bound from `$90.68–$111.39` to `$99.75–$111.39` — a 44% narrower interval, presented as a proven bound and then fed into `monthlyFor` as the `× the $X/mo price` multiple. The script already models measurement uncertainty explicitly (`--usd-tolerance` widens every dollar interval), so a ratio derived from rounded percent ramps having zero declared uncertainty is inconsistent with the rest of the model. Either give `worth` an optional tolerance that widens the implied interval before intersecting (e.g. `implied = {lo: other.lo * f * (1 - t), hi: other.hi * f * (1 + t)}`), or state in SKILL.md that `worth` fractions must be exactly-known ratios and that empirical calibrations belong somewhere that cannot tighten a headline.
Author
Owner

Fixed in efaf05d: worth relations now accept and propagate an optional relative tolerance with mathematically inverted bounds in the reverse direction. The empirical Claude calibrations use ±2%, and diagnostics print that uncertainty instead of treating 6×/8× as exact.

<!-- gh-feedback:reply-to:42346 --> Fixed in efaf05d: worth relations now accept and propagate an optional relative tolerance with mathematically inverted bounds in the reverse direction. The empirical Claude calibrations use ±2%, and diagnostics print that uncertainty instead of treating 6×/8× as exact.
jercik marked this conversation as resolved
@ -364,0 +444,4 @@
for (const component of relationComponents) {
let componentConflict = false;
let converged = false;
for (let iteration = 0; iteration < component.defs.length * 10 + 1; iteration++) {

ℹ️ Info: I checked whether the defs.length * 10 + 1 cap can misclassify a consistent component as contradictory. It cannot in practice: intersect only narrows, so a component whose cycle ratios multiply to 1 reaches its fixed point in a couple of passes, and the cap is only exhausted when a cycle's product differs from 1 and some bound is unbounded above — which is a genuine contradiction. I reproduced that case (a = 0.2 × b with b = 6 × a, both meters bounded only below) and it reports CONFLICTS jointly; solver did not converge before classifying the contradiction, falling back to measured bounds. Conservative and correctly labelled; no action needed.

ℹ️ **Info:** I checked whether the `defs.length * 10 + 1` cap can misclassify a *consistent* component as contradictory. It cannot in practice: `intersect` only narrows, so a component whose cycle ratios multiply to 1 reaches its fixed point in a couple of passes, and the cap is only exhausted when a cycle's product differs from 1 and some bound is unbounded above — which is a genuine contradiction. I reproduced that case (`a = 0.2 × b` with `b = 6 × a`, both meters bounded only below) and it reports `CONFLICTS jointly; solver did not converge before classifying the contradiction`, falling back to measured bounds. Conservative and correctly labelled; no action needed.
jercik marked this conversation as resolved
fix: model calibration ratio uncertainty
Some checks failed
Node tests / node:test (pull_request) Successful in 15s
commit-msg / commitlint (pull_request) Successful in 17s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 1m40s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 34s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 10m39s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 10m57s
efaf05d22f
Author
Owner

Replying to review #9819

Addressed in efaf05d: worth relations now carry optional relative calibration tolerance (the committed Claude ratios use ±2%), and reverse propagation uses the correctly inverted interval. The solver still reports the bundled joint conflict, while future Team Premium measurements will not be over-tightened by an exact-equality assumption. The convergence-cap observation is acknowledged.

> Replying to review #9819 Addressed in efaf05d: worth relations now carry optional relative calibration tolerance (the committed Claude ratios use ±2%), and reverse propagation uses the correctly inverted interval. The solver still reports the bundled joint conflict, while future Team Premium measurements will not be over-tightened by an exact-equality assumption. The convergence-cap observation is acknowledged.
forgejo-actions left a comment

Summary: Reviewed the relation fixed-point solver, the new plan/tolerance validation, and the SKILL.md calibration notes. The component solver itself checks out: the propagation is sound (it only narrows, and the tolerance is relaxed independently per direction), components are node-disjoint so the shared relationBounds map cannot cross-contaminate, and the joint conflict on the bundled Max 20x data reproduces exactly as the PR describes (weekly implied at $1028–$1089 from 6× 5h vs $838–$999 from the fable relation — disjoint, with neither edge pairwise-disjoint). Half-measured relations, a plans file with no matching measurements, and the --json path all behave. tsc --strict reports nothing beyond missing @types/node.

Found 1 medium and 3 low issues.

  • Medium: relation bounds use hull(refined), so a relation implying a value inside the gap between disjoint per-mode segments is reported as a tightening rather than a conflict — reproduced with a headline that no surviving rounding mode admits.
  • Low: the tightening path's human output dropped the relation ratios, leaving the tightened headline unauditable outside --json.
  • Low: an empty-string costBasis passes validation and is then silently dropped.
  • Low: the documented joint conflict disappears above ~0.4% usd tolerance, which the sweep guidance does not mention.

Code review by Claude Code Opus (opus)

**Summary:** Reviewed the relation fixed-point solver, the new plan/tolerance validation, and the SKILL.md calibration notes. The component solver itself checks out: the propagation is sound (it only narrows, and the tolerance is relaxed independently per direction), components are node-disjoint so the shared `relationBounds` map cannot cross-contaminate, and the joint conflict on the bundled Max 20x data reproduces exactly as the PR describes (`weekly` implied at `$1028–$1089` from 6× 5h vs `$838–$999` from the fable relation — disjoint, with neither edge pairwise-disjoint). Half-measured relations, a plans file with no matching measurements, and the `--json` path all behave. `tsc --strict` reports nothing beyond missing `@types/node`. Found 1 medium and 3 low issues. - **Medium:** relation bounds use `hull(refined)`, so a relation implying a value inside the gap between disjoint per-mode segments is reported as a tightening rather than a conflict — reproduced with a headline that no surviving rounding mode admits. - **Low:** the tightening path's human output dropped the relation ratios, leaving the tightened headline unauditable outside `--json`. - **Low:** an empty-string `costBasis` passes validation and is then silently dropped. - **Low:** the documented joint conflict disappears above ~0.4% usd tolerance, which the sweep guidance does not mention. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6ImVmYWYwNWQyMmY3ZmE0NTQyMGVkNmEwY2IzNzBhM2UwYmQwNDNlYzMiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE2OTk4Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6ImFjNzkyOTExLTYyNWEtNGI4Yi1hNDM2LTA0Y2I0ZDRlYWY4YiJ9 -->
@ -32,1 +32,3 @@
`plans.json` describes each plan: its `monthlyCostUsd` and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without one — like `5h`, a throttle whose rate cap is independent of the weekly quotas — deliberately gets no monthly line; don't add a period to one. A meter's `worth: {"fraction": 0.5, "of": "weekly"}` declares a known cross-meter fact (its total is that fraction of a sibling meter's total, e.g. the fable meter is worth half the general weekly limit); the script propagates estimates through it in both directions automatically.
`plans.json` describes each plan: its `monthlyCostUsd`, optional `costBasis`, and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). `monthlyCostUsd` is the monthly-equivalent denominator used for the `× the $X/mo price` output; `costBasis` travels with that number so comparisons distinguish month-to-month from annual-billing per-seat equivalents. The current Team Premium value is a working estimate whose billing basis has not been independently verified. A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without `periodHours` — like `5h` — has no monthly equivalent in this estimator; it may still participate in a `worth` relation, so don't add a made-up period. A meter's `worth: {"fraction": 0.5, "of": "weekly", "tolerance": 0.02}` declares an approximate cross-meter ratio (the target is within ±2% of that fraction of a sibling meter's total; for example, the fable meter is approximately half the general weekly limit, while a plan can express `weekly = 6 × 5h` with `fraction: 6`). The estimator treats every `worth` entry as a bidirectional interval constraint and intersects each connected component to a fixed point. If a component conflicts or fails to converge, every measured meter in that component falls back to its measured bound. Direct pairwise contradictions are marked `CONFLICTS`; a joint contradiction is reported at the component level and its edges are `SUPPRESSED`, while iteration-cap exhaustion is classified as a contradiction with an explicit non-convergence note. This keeps the result independent of JSON key order. A relation with one or both meters unmeasured is reported as unvalidated instead of silently affecting an estimate.
The Claude ratio calibrations are empirical cross-meter facts: long, monotonically increasing Prometheus ramps from the `j4k/cluster` usage dashboards support approximately `weekly = 6 × 5h` for Claude Max 20x accounts (not Max 5x) and `weekly = 8 × 5h` for the Team Premium/VC account. A ramp is a contiguous reset-free sequence of whole-percent snapshots; aggregating its endpoints makes the two rounding errors small relative to the movement, so these relative ratios are better constrained than the absolute API-dollar totals. The cluster's capacity contract is `group_vars/all/ax_capacity.yml` and its mirrored regression fixture in `tests/test-axcredrouter-capacity-plan-values.py`. The raw Prometheus ramp snapshots are external evidence and are not currently copied into this estimator repository. The bundled Max 20x measurements currently expose a joint conflict: the 6× ramp, the 0.5× fable relation, and the fable readings cannot all be true. The bundled 5h lines are annotated `fable-only usage`, while the 6× calibration came from general cluster ramps; if those meters weight fable spend differently, reconcile that provenance before treating the conflict as evidence that a numeric input is wrong. Do not edit measurements to force the calibration; preserve the component-level conflict until those inputs are reconciled, then revise the weaker absolute estimate.

🟢 Low: Worth recording that the documented joint conflict is tolerance-sensitive, since this paragraph instructs the reader to preserve it and line 47 tells them to rerun the sweep at 0 and 0.5. Measured against the bundled data: the conflict holds at --usd-tolerance 0, 0.25 and 0.35, but disappears at 0.4 and above — at 0.5 all three meters report combined with relation component, with weekly collapsing to $998.91–$1001.24 (21.7×–21.8× the $200/mo price) versus the widened $766.95–$1086.74 fallback. A reader following the sweep guidance will see the conflict vanish and a near-point estimate appear, which reads as more confidence rather than a barely-satisfiable intersection. One sentence naming the ~0.4% flip point would keep the instruction to preserve the conflict from looking contradicted by the tool's own output.

🟢 **Low:** Worth recording that the documented joint conflict is tolerance-sensitive, since this paragraph instructs the reader to preserve it and line 47 tells them to rerun the sweep at `0` and `0.5`. Measured against the bundled data: the conflict holds at `--usd-tolerance` 0, 0.25 and 0.35, but disappears at 0.4 and above — at `0.5` all three meters report `combined with relation component`, with `weekly` collapsing to `$998.91–$1001.24 (21.7×–21.8× the $200/mo price)` versus the widened `$766.95–$1086.74` fallback. A reader following the sweep guidance will see the conflict vanish and a near-point estimate appear, which reads as *more* confidence rather than a barely-satisfiable intersection. One sentence naming the ~0.4% flip point would keep the instruction to preserve the conflict from looking contradicted by the tool's own output.
jercik marked this conversation as resolved
@ -148,3 +151,4 @@
if (typeof p.monthlyCostUsd !== "number" || !(p.monthlyCostUsd > 0)) {
fail(`plans entry ${i}: "monthlyCostUsd" must be a positive number`);
}
if (p.costBasis !== undefined && typeof p.costBasis !== "string") {

🟢 Low: costBasis only rejects non-strings, so "costBasis": "" validates, is retained by parsePlans (line 183), and is then silently dropped by the truthiness guards in monthlyFor (line 579) and the output template (lines 668-669). Verified: a plan with "costBasis": "" prints …× the $100/mo price with no basis and omits the key from --json. Given the file's stated "typos fail loudly" contract and that plan already rejects "", requiring a non-empty string here would keep a mistyped basis from disappearing:

if (p.costBasis !== undefined && (typeof p.costBasis !== "string" || p.costBasis === "")) {
  fail(`plans entry ${i}: "costBasis" must be a non-empty string when present`);
}
🟢 **Low:** `costBasis` only rejects non-strings, so `"costBasis": ""` validates, is retained by `parsePlans` (line 183), and is then silently dropped by the truthiness guards in `monthlyFor` (line 579) and the output template (lines 668-669). Verified: a plan with `"costBasis": ""` prints `…× the $100/mo price` with no basis and omits the key from `--json`. Given the file's stated "typos fail loudly" contract and that `plan` already rejects `""`, requiring a non-empty string here would keep a mistyped basis from disappearing: ```ts if (p.costBasis !== undefined && (typeof p.costBasis !== "string" || p.costBasis === "")) { fail(`plans entry ${i}: "costBasis" must be a non-empty string when present`); } ```
Author
Owner

Fixed in 4564644: costBasis is now validated as a non-empty string and retained in JSON/output whenever present.

<!-- gh-feedback:reply-to:42402 --> Fixed in 4564644: costBasis is now validated as a non-empty string and retained in JSON/output whenever present.
jercik marked this conversation as resolved
@ -360,0 +388,4 @@
const nodeKey = (plan: string, limit: string) => JSON.stringify([plan, limit]);
const measuredBounds = new Map<string, Interval>();
for (const core of cores) measuredBounds.set(nodeKey(core.plan, core.limit), hull(core.eras[0].refined));

🟡 Medium: measuredBounds collapses eras[0].refined with hull(), so the relation fixed point treats the gap between disjoint per-mode segments as feasible. When more than one rounding mode survives, refined can be a union ($169.88–$170.35 ∪ $174.47–$178.40), and an implied interval lying entirely inside the gap still intersects the hull — so the solver reports a tightening instead of a conflict, and the headline is a range no surviving mode admits.

Reproduced against the bundled measurements with one extra tightly-measured meter c and worth: {"fraction": 1, "of": "5h"}, at --usd-tolerance 0.5:

claude-max-20x / 5h
  current: $171.02–$173.97 (midpoint $172.49) — combined with relation component {5h, c}
    measured alone: $169.88–$170.35 ∪ $174.47–$178.40, 4 measurements in 2 sessions
    per-mode: floor $169.88–$170.35 | nearest $174.47–$178.40 | ceil ∅

The headline sits strictly between the two surviving-mode segments, and nothing is flagged. The hull collapse predates this PR (measuredHull did the same), but this diff makes the relation solver load-bearing across a whole component, so the failure now propagates further. Safer fix: carry the segment list per node and intersect implied bounds segment-wise (or, minimally, re-intersect the final combined against era.refined and classify an empty result as a conflict).

🟡 **Medium:** `measuredBounds` collapses `eras[0].refined` with `hull()`, so the relation fixed point treats the gap between disjoint per-mode segments as feasible. When more than one rounding mode survives, `refined` can be a union (`$169.88–$170.35 ∪ $174.47–$178.40`), and an implied interval lying entirely inside the gap still intersects the hull — so the solver reports a tightening instead of a conflict, and the headline is a range no surviving mode admits. Reproduced against the bundled measurements with one extra tightly-measured meter `c` and `worth: {"fraction": 1, "of": "5h"}`, at `--usd-tolerance 0.5`: ``` claude-max-20x / 5h current: $171.02–$173.97 (midpoint $172.49) — combined with relation component {5h, c} measured alone: $169.88–$170.35 ∪ $174.47–$178.40, 4 measurements in 2 sessions per-mode: floor $169.88–$170.35 | nearest $174.47–$178.40 | ceil ∅ ``` The headline sits strictly between the two surviving-mode segments, and nothing is flagged. The hull collapse predates this PR (`measuredHull` did the same), but this diff makes the relation solver load-bearing across a whole component, so the failure now propagates further. Safer fix: carry the segment list per node and intersect implied bounds segment-wise (or, minimally, re-intersect the final `combined` against `era.refined` and classify an empty result as a conflict).
Author
Owner

Fixed in 4564644: relation propagation now checks propagated bounds against each surviving per-mode segment and falls back the whole component when a hull-only value sits in a gap. Successful outputs preserve segment unions instead of presenting a range no surviving mode admits.

<!-- gh-feedback:reply-to:42400 --> Fixed in 4564644: relation propagation now checks propagated bounds against each surviving per-mode segment and falls back the whole component when a hull-only value sits in a gap. Successful outputs preserve segment unions instead of presenting a range no surviving mode admits.
jercik marked this conversation as resolved
@ -448,2 +638,3 @@
if (i === 0 && relation?.combined) {
console.log(` ${label}: ${rangeText(relation.combined)} — combined with relation ${relation.text}`);
const component = relation.componentMeters ? ` {${relation.componentMeters.join(", ")}}` : "";
console.log(` ${label}: ${rangeText(relation.combined)} — combined with relation component${component}`);

🟢 Low: In the tightening path the human-readable output no longer names any relation — only the component's meter set. Previously the line read combined with relation weekly-fable = 0.5 × weekly; now it reads combined with relation component {5h, weekly, weekly-fable}, and relation.constraints is only printed in the conflict branch (lines 654-661), so the ratios that produced the number are reachable solely via --json. Since a reader is being asked to trust a bound that is tighter than the measurement evidence, printing the component's constraints texts here too (they are already computed) would keep the headline auditable.

🟢 **Low:** In the tightening path the human-readable output no longer names any relation — only the component's meter set. Previously the line read `combined with relation weekly-fable = 0.5 × weekly`; now it reads `combined with relation component {5h, weekly, weekly-fable}`, and `relation.constraints` is only printed in the conflict branch (lines 654-661), so the ratios that produced the number are reachable solely via `--json`. Since a reader is being asked to trust a bound that is tighter than the measurement evidence, printing the component's `constraints` texts here too (they are already computed) would keep the headline auditable.
Author
Owner

Fixed in 4564644: successful tightening output now prints each relation constraint and its implied range beneath the component-qualified headline, keeping text output auditable without --json.

<!-- gh-feedback:reply-to:42401 --> Fixed in 4564644: successful tightening output now prints each relation constraint and its implied range beneath the component-qualified headline, keeping text output auditable without --json.
jercik marked this conversation as resolved
fix: preserve disjoint relation bounds
Some checks failed
Node tests / node:test (pull_request) Successful in 14s
commit-msg / commitlint (pull_request) Successful in 26s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 2m31s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 34s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 7m58s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 8m19s
45646441cb
Author
Owner

Replying to review #9825

Addressed in 4564644: relation propagation now preserves disjoint rounding-mode segments, rejects hull-only values in gaps as component conflicts, and prints relation implications in successful text output. Empty costBasis is rejected. The tolerance-sensitivity observation is acknowledged and documented: this bundled conflict holds through approximately 0.35% USD tolerance and flips near 0.4%.

> Replying to review #9825 Addressed in 4564644: relation propagation now preserves disjoint rounding-mode segments, rejects hull-only values in gaps as component conflicts, and prints relation implications in successful text output. Empty costBasis is rejected. The tolerance-sensitivity observation is acknowledged and documented: this bundled conflict holds through approximately 0.35% USD tolerance and flips near 0.4%.
forgejo-actions left a comment

Summary: Reviewed the relation solver rewrite, the plans schema additions, and the SKILL.md documentation. The fixed-point propagation, component detection, conflict classification, and fallback-to-measured-bounds paths all behave as documented — I ran the bundled data at --usd-tolerance 0 / 0.3 / 0.35 / 0.4 / 0.5 / 1 and confirmed the joint-conflict claim and the ~0.4% flip point stated in SKILL.md, plus chained, cyclic, pairwise-conflicting, and unbounded-interval fixtures.

One real defect: the solver propagates over per-node hulls of the union of surviving rounding modes and then checks segment feasibility per node, so it can combine one meter's floor segment with another meter's nearest segment. That contradicts the estimator's own global single-mode assumption and can yield a confidently tightened headline that no consistent mode supports (repro in the comment). Two low-severity notes on reporting and documentation accuracy.

Found 1 high, 2 low.

Code review by Claude Code Opus (opus)

**Summary:** Reviewed the relation solver rewrite, the plans schema additions, and the SKILL.md documentation. The fixed-point propagation, component detection, conflict classification, and fallback-to-measured-bounds paths all behave as documented — I ran the bundled data at `--usd-tolerance 0 / 0.3 / 0.35 / 0.4 / 0.5 / 1` and confirmed the joint-conflict claim and the ~0.4% flip point stated in SKILL.md, plus chained, cyclic, pairwise-conflicting, and unbounded-interval fixtures. One real defect: the solver propagates over per-node hulls of the union of surviving rounding modes and then checks segment feasibility per node, so it can combine one meter's `floor` segment with another meter's `nearest` segment. That contradicts the estimator's own global single-mode assumption and can yield a confidently tightened headline that no consistent mode supports (repro in the comment). Two low-severity notes on reporting and documentation accuracy. Found 1 high, 2 low. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGstb3NzL2FnZW50LXNraWxscyIsIm51bWJlciI6IjMwIiwiaGVhZFNoYSI6IjQ1NjQ2NDQxY2IyMzY0ZjM4NzUwNjBmNTkxMWJjYTMxMzgyOTBjNmMiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE3MDI1Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6ImY2YTI1ZWZiLWQ5OTktNDA5NC1hNTBmLTQ5OTRjNWQxNzM2ZSJ9 -->
@ -32,1 +32,3 @@
`plans.json` describes each plan: its `monthlyCostUsd` and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without one — like `5h`, a throttle whose rate cap is independent of the weekly quotas — deliberately gets no monthly line; don't add a period to one. A meter's `worth: {"fraction": 0.5, "of": "weekly"}` declares a known cross-meter fact (its total is that fraction of a sibling meter's total, e.g. the fable meter is worth half the general weekly limit); the script propagates estimates through it in both directions automatically.
`plans.json` describes each plan: its `monthlyCostUsd`, optional `costBasis`, and a `meters` registry naming every meter the measurements may reference (an unregistered meter name is a validation error, so typos fail loudly). `monthlyCostUsd` is the monthly-equivalent denominator used for the `× the $X/mo price` output; `costBasis` travels with that number so comparisons distinguish month-to-month from annual-billing per-seat equivalents. The current Team Premium value is a working estimate whose billing basis has not been independently verified. A meter with `periodHours` is a fuel gauge: the script annualizes its value to a monthly equivalent (`value × (365.25/12 × 24) / periodHours`) reported as a multiple of the subscription price. A meter without `periodHours` — like `5h` — has no monthly equivalent in this estimator; it may still participate in a `worth` relation, so don't add a made-up period. A meter's `worth: {"fraction": 0.5, "of": "weekly", "tolerance": 0.02}` declares an approximate cross-meter ratio (the target is within ±2% of that fraction of a sibling meter's total; for example, the fable meter is approximately half the general weekly limit, while a plan can express `weekly = 6 × 5h` with `fraction: 6`). The estimator treats every `worth` entry as a bidirectional interval constraint and intersects each connected component to a fixed point. If a component conflicts or fails to converge, every measured meter in that component falls back to its measured bound. Direct pairwise contradictions are marked `CONFLICTS`; a joint contradiction is reported at the component level and its edges are `SUPPRESSED`, while iteration-cap exhaustion is classified as a contradiction with an explicit non-convergence note. This keeps the result independent of JSON key order. A relation with one or both meters unmeasured is reported as unvalidated instead of silently affecting an estimate.
The Claude ratio calibrations are empirical cross-meter facts: long, monotonically increasing Prometheus ramps from the `j4k/cluster` usage dashboards support approximately `weekly = 6 × 5h` for Claude Max 20x accounts (not Max 5x) and `weekly = 8 × 5h` for the Team Premium/VC account. A ramp is a contiguous reset-free sequence of whole-percent snapshots; aggregating its endpoints makes the two rounding errors small relative to the movement, so these relative ratios are better constrained than the absolute API-dollar totals. The cluster's capacity contract is `group_vars/all/ax_capacity.yml` and its mirrored regression fixture in `tests/test-axcredrouter-capacity-plan-values.py`. The raw Prometheus ramp snapshots are external evidence and are not currently copied into this estimator repository. The bundled Max 20x measurements currently expose a joint conflict: the 6× ramp, the 0.5× fable relation, and the fable readings cannot all be true at the default tolerance. That conflict is sensitivity-tested: it holds through roughly `--usd-tolerance 0.35` but disappears around 0.4% and above, where the relation may tighten the headline; treat that near-consistent result as tolerance-dependent, not as stronger evidence. The bundled 5h lines are annotated `fable-only usage`, while the 6× calibration came from general cluster ramps; if those meters weight fable spend differently, reconcile that provenance before treating the conflict as evidence that a numeric input is wrong. Do not edit measurements to force the calibration; preserve the component-level conflict until those inputs are reconciled, then revise the weaker absolute estimate.

🟢 Low: "The bundled 5h lines are annotated fable-only usage" does not match resources/measurements.jsonl: the 5h lines carry fresh 5h window, new 5h window, true 0% start, and cumulative snapshot, same fresh 5h window. The fable-only usage note is on the weekly / weekly-fable lines of those same sessions. The substance holds (the sessions' spend was fable-only, and the 5h lines share that window), but a reader following this sentence to the data will not find the annotation where it says. Suggest "The bundled measurements' sessions are annotated fable-only usage on their weekly lines, and the 5h lines share those spending windows".

🟢 **Low:** "The bundled 5h lines are annotated `fable-only usage`" does not match `resources/measurements.jsonl`: the `5h` lines carry `fresh 5h window`, `new 5h window, true 0% start`, and `cumulative snapshot, same fresh 5h window`. The `fable-only usage` note is on the `weekly` / `weekly-fable` lines of those same sessions. The substance holds (the sessions' spend was fable-only, and the `5h` lines share that window), but a reader following this sentence to the data will not find the annotation where it says. Suggest "The bundled measurements' sessions are annotated `fable-only usage` on their `weekly` lines, and the `5h` lines share those spending windows".
Author
Owner

Fixed in 9e22cd4: SKILL.md now accurately says the sessions are annotated fable-only on weekly lines and the 5h lines share those spending windows.

<!-- gh-feedback:reply-to:42455 --> Fixed in 9e22cd4: SKILL.md now accurately says the sessions are annotated fable-only on weekly lines and the 5h lines share those spending windows.
jercik marked this conversation as resolved
@ -364,0 +493,4 @@
for (const key of component.nodes) {
const bound = relationBounds.get(key);
const segments = measuredSegments.get(key);
if (bound && segments && !segments.some((segment) => intersect(segment, bound))) componentConflict = true;

🟠 High: The relation solve can mix rounding modes across meters, producing a tightened headline that no single consistent mode supports.

measuredBounds is the hull of eras[0].refined, which is the union over surviving modes. Propagation runs on those hulls, and this feasibility check is per node — it only asks whether each node's own bound hits some segment, never whether the surviving segments can be picked under one global mode. But survivors is computed globally (line 356) precisely because one display mode must explain every series at once, so a result that needs meter A on floor and meter B on nearest is not admissible.

Repro (two meters, both with startExact readings so each has disjoint floor/nearest segments, worth: {fraction: 2, of: "a"}, run at --usd-tolerance 0.4):

p / a
  current: $176.01–$178.23 — combined with relation component {a, b}
    measured alone: $170.05–$170.18 ∪ $174.65–$178.23
    per-mode: floor $170.05–$170.18 | nearest $174.65–$178.23 | ceil ∅
p / b
  current: $352.01–$352.27 — combined with relation component {a, b}
    measured alone: $352.01–$352.27 ∪ $361.53–$368.93
    per-mode: floor $352.01–$352.27 | nearest $361.53–$368.93 | ceil ∅

The headline picks a from its nearest segment and b from its floor segment. Under either consistent mode the relation is infeasible: floor gives 2×[170.05,170.18] = [340.10,340.36] vs b [352.01,352.27] (disjoint); nearest gives 2×[174.65,178.23] = [349.30,356.46] vs b [361.53,368.93] (disjoint). The honest output is a component conflict, yet the tool reports a narrow combined bound — and excludes a's floor segment — and feeds it into the monthly-equivalent line.

Safe fix: run the propagation once per surviving mode over that mode's perMode interval per node instead of over the union hull, then union the per-mode fixed points; a mode whose propagation goes infeasible is eliminated, and the component conflicts only when every surviving mode fails. That reuses the existing mode-elimination discipline and makes this per-node segment check unnecessary. (The bundled Max 20x data does not hit this today: only 5h carries startExact lines, so weekly/weekly-fable have identical per-mode intervals.)

🟠 **High:** The relation solve can mix rounding modes across meters, producing a tightened headline that no single consistent mode supports. `measuredBounds` is the hull of `eras[0].refined`, which is the *union over surviving modes*. Propagation runs on those hulls, and this feasibility check is per node — it only asks whether each node's own bound hits some segment, never whether the surviving segments can be picked under one global mode. But `survivors` is computed globally (line 356) precisely because one display mode must explain every series at once, so a result that needs meter A on `floor` and meter B on `nearest` is not admissible. Repro (two meters, both with `startExact` readings so each has disjoint floor/nearest segments, `worth: {fraction: 2, of: "a"}`, run at `--usd-tolerance 0.4`): ``` p / a current: $176.01–$178.23 — combined with relation component {a, b} measured alone: $170.05–$170.18 ∪ $174.65–$178.23 per-mode: floor $170.05–$170.18 | nearest $174.65–$178.23 | ceil ∅ p / b current: $352.01–$352.27 — combined with relation component {a, b} measured alone: $352.01–$352.27 ∪ $361.53–$368.93 per-mode: floor $352.01–$352.27 | nearest $361.53–$368.93 | ceil ∅ ``` The headline picks `a` from its *nearest* segment and `b` from its *floor* segment. Under either consistent mode the relation is infeasible: floor gives 2×[170.05,170.18] = [340.10,340.36] vs b [352.01,352.27] (disjoint); nearest gives 2×[174.65,178.23] = [349.30,356.46] vs b [361.53,368.93] (disjoint). The honest output is a component conflict, yet the tool reports a narrow combined bound — and excludes `a`'s floor segment — and feeds it into the monthly-equivalent line. Safe fix: run the propagation once per surviving mode over that mode's `perMode` interval per node instead of over the union hull, then union the per-mode fixed points; a mode whose propagation goes infeasible is eliminated, and the component conflicts only when every surviving mode fails. That reuses the existing mode-elimination discipline and makes this per-node segment check unnecessary. (The bundled Max 20x data does not hit this today: only `5h` carries `startExact` lines, so `weekly`/`weekly-fable` have identical per-mode intervals.)
Author
Owner

Fixed in 9e22cd4: each surviving global display mode now gets its own relation fixed-point solve; only mode-consistent results are unioned, and a component conflicts when every mode is infeasible. This prevents cross-meter floor/nearest mixing.

<!-- gh-feedback:reply-to:42453 --> Fixed in 9e22cd4: each surviving global display mode now gets its own relation fixed-point solve; only mode-consistent results are unioned, and a component conflicts when every mode is infeasible. This prevents cross-meter floor/nearest mixing.
jercik marked this conversation as resolved
@ -375,2 +538,2 @@
return { text: `${def.meter} = ${def.fraction} × ${def.of}`, implied, combined };
}
const otherKey = nodeKey(plan, def.meter === limit ? def.of : def.meter);
const other = relationBounds.get(otherKey) ?? measuredBounds.get(otherKey);

🟢 Low: other reads the post-propagation relationBounds, so a printed implies line can be the node's own bound reflected back through a neighbour rather than independent evidence. With a chain b = 2 × a, c = 2 × b:

p / b
  current: $181.36–$222.78 — combined with relation component {a, b, c}
    relation b = 2 × a: implies $181.36–$222.78
    relation c = 2 × b: implies $181.36–$222.78

The second line is c's propagated bound divided by 2 — and c's bound came from b — so it reads as a second, corroborating path when it is the same information twice. Using measuredBounds.get(otherKey) for the displayed implied (keeping relationBounds for the headline) makes each line show what that edge independently contributes.

🟢 **Low:** `other` reads the post-propagation `relationBounds`, so a printed `implies` line can be the node's own bound reflected back through a neighbour rather than independent evidence. With a chain `b = 2 × a`, `c = 2 × b`: ``` p / b current: $181.36–$222.78 — combined with relation component {a, b, c} relation b = 2 × a: implies $181.36–$222.78 relation c = 2 × b: implies $181.36–$222.78 ``` The second line is c's propagated bound divided by 2 — and c's bound came from b — so it reads as a second, corroborating path when it is the same information twice. Using `measuredBounds.get(otherKey)` for the displayed `implied` (keeping `relationBounds` for the headline) makes each line show what that edge independently contributes.
Author
Owner

Fixed in 9e22cd4: human-readable relation implication diagnostics now use each edge's independent measured bound, while propagated bounds remain reserved for the component headline.

<!-- gh-feedback:reply-to:42454 --> Fixed in 9e22cd4: human-readable relation implication diagnostics now use each edge's independent measured bound, while propagated bounds remain reserved for the component headline.
jercik marked this conversation as resolved
fix: solve relations per display mode
Some checks failed
Node tests / node:test (pull_request) Successful in 16s
commit-msg / commitlint (pull_request) Successful in 20s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 2m22s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-2 generator (pull_request_target) Failing after 36s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Failing after 11m36s
PR Review / Dispatch and observe exact review writers (pull_request_target) Failing after 11m38s
9e22cd4a7f
Author
Owner

Replying to review #9832

Addressed in 9e22cd4: relation propagation now runs independently for each globally surviving display-rounding mode and unions only mode-consistent results, preventing cross-meter floor/nearest mixing. Text diagnostics use independent measured edge evidence. SKILL.md now accurately describes the fable-only session annotations.

> Replying to review #9832 Addressed in 9e22cd4: relation propagation now runs independently for each globally surviving display-rounding mode and unions only mode-consistent results, preventing cross-meter floor/nearest mixing. Text diagnostics use independent measured edge evidence. SKILL.md now accurately describes the fable-only session annotations.
Author
Owner

Replying to review #9633

This earlier review was processed in 54683ba/b538e75: the relation graph now intersects chained constraints and surfaces the conflict, the 5h documentation no longer claims independence from worth relations, and the provenance guidance names the external evidence without editing recorded measurements. The Team Premium no-measurement observation remains acknowledged.

> Replying to review #9633 This earlier review was processed in 54683ba/b538e75: the relation graph now intersects chained constraints and surfaces the conflict, the 5h documentation no longer claims independence from worth relations, and the provenance guidance names the external evidence without editing recorded measurements. The Team Premium no-measurement observation remains acknowledged.
jercik merged commit ea8d90328f into main 2026-08-05 09:41:06 +00:00
jercik deleted branch feat/usage-ratio-calibration 2026-08-05 09:41:06 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
j4k-oss/agent-skills!30
No description provided.