fix(optimize): rank findings in core instead of importing the CLI

[myukitty]

GET /optimize imported _rank_findings from tokenjam.cli.cmd_optimize, which inverts the core/cli/api layering rule.

Ranking now lives in tokenjam.core.optimize.rank. The API imports it from core. The CLI keeps thin wrappers so existing renderer tests stay put. CARD_FINDING_NAMES is the card-bearing set, not the full analyzer registry, and a drift test pins it to _FINDING_RENDERERS.

Closes #579

[greptile-apps]

Greptile Summary

The PR moves optimize-finding ranking from the CLI into the shared core layer while preserving thin CLI compatibility wrappers.

• Adds shared ranking constants and helpers under tokenjam.core.optimize.
• Updates the API to consume ranking directly from core.
• Pins the core card-name ordering to the CLI renderer registry with unit tests.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
tokenjam/core/optimize/rank.py	Introduces the shared ranking implementation and canonical card-bearing finding order without an identified blocking regression.
tokenjam/core/optimize/init.py	Re-exports the new ranking API from core; the added import is acyclic and included by package-wide wheel configuration.
tokenjam/api/routes/optimize.py	Removes the API-to-CLI dependency and obtains finding ranking from the core layer while preserving persona filtering.
tokenjam/cli/cmd_optimize.py	Replaces duplicated ranking logic with a thin compatibility wrapper that retains CLI renderer-specific inputs.
tests/unit/test_optimize_rank.py	Covers ranking, unranked findings, requested subsets, unknown names, renderer drift, and the API layering constraint.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Report[OptimizeReport] --> Rank[tokenjam.core.optimize.rank_findings]
  Rank --> API[GET /optimize finding_rank]
  Rank --> Wrapper[CLI compatibility wrapper]
  Wrapper --> CLI[tj optimize renderer]

Loading

_{Reviews (2): Last reviewed commit: "Merge branch 'main' into fix/move-rank-f..." | Re-trigger Greptile}

[anilmurty]

Approving — and welcome, @myukitty, this is a strong first contribution. CI is now green across all seven checks (I approved the workflow run; fork PRs from first-time contributors need that manually).

Verification

The premise checked out on fresh main — one offending import, exactly where #579 said:

tokenjam/api/routes/optimize.py:34: from tokenjam.cli.cmd_optimize import _rank_findings

After this PR, a full-package grep for module-level and function-local imports returns zero hits from core/ or api/ into cli/. All three of #579's "Done when" criteria are met.

Behaviour-preservation was checked rather than assumed: main's _rank_findings was run against both new paths over 30,000 randomized reports, plus an exhaustive sweep of insertion-order permutations at identical shares to exercise tie-breaking specifically — zero mismatches. The three-key sort tuple is intact, and CARD_FINDING_NAMES is byte-identical to _FINDING_RENDERERS's key order. tj optimize --since 30d against a real database still ranks biggest-first, and a persona-gated subset still emits its "Not run" line.

All 233 added lines account for: 109 test file + 100 rank.py + 10 re-exports + 11 CLI wrapper + 3 API import swap. Nothing in the diff is neither a move nor a test.

What you did well — specifically

You noticed the card-bearing name set is not the analyzer registry. Reaching for ANALYZER_REGISTRY or ANALYZER_ORDER is the obvious move when relocating ranking into core/ — it's in the same package and it looks canonical. It would have been wrong: budget-projection is registered but has no ranked card, and placement has a card but is deliberately unregistered (it's produced inside downsize). That's the single easiest way this refactor could have quietly broken tj optimize placement, and you walked past it and said so in the PR body.

You pinned the duplication you introduced, in the same PR. Having created a second declaration of the name ordering, you wrote the drift test binding it to _FINDING_RENDERERS — and bound _ALWAYS_FULL_FINDINGS by identity rather than equality, so a re-declared literal fails instead of passing on equal contents.

You preserved relearn's unranked rule and tested it. That's a subtle, hard-won behaviour that a refactor could very easily have dropped on the floor.

Scope discipline. You kept two CLI re-exports with an honest noqa comment rather than rewriting six unrelated test modules. That was the right call.

Nits — none blocking, all fine as follow-ups

1. CARD_FINDING_NAMES being a second declaration of the renderer key order is safe because your drift test catches desync — but the stronger shape is for core to own the names and cmd_optimize to build its renderer table keyed off them, so drift becomes impossible rather than merely detected.
2. known_names / always_full are parameters that provably can only hold their defaults (your own drift test proves it). Dropping them would remove the injection seam and about 8 lines. Defensible as-is.
3. test_api_optimize_route_does_not_import_cli guards one file by reading its source text. Two upgrades worth considering: resolve the path via inspect.getsourcefile(...) so it survives a non-editable install, and generalize it to walk every module under core/ and api/. There's no repo-wide guard on main today, so that would close the whole class rather than the one instance #579 found. Good fast-follow if you want another.
4. _reclaimable_share's original docstring explained why None isn't 0.0, with the concrete example (cache-recommend recommends a placement, not a token count) and the consequence (it would hide an analyzer's own empty-state message). rank.py condenses that to "they are not de-minimis". That sentence is precisely what stops a future reader "simplifying" the None into a 0.0 — worth carrying across.

One housekeeping note: I just merged #726, so this will need Update branch before it goes in.

[anilmurty]

Great first contribution @myukitty - thanks!