[johnl-amd]

Summary

• Three fields added to the meta block of every routing and behavioral report: engine, agent_cli_version, and model_resolved. New helpers agent.cli_version() and agent.resolved_model(), joined by cli._provenance() and splatted into both meta dicts.
• Why: a report says which prompts passed and nothing about what graded them. model is the alias the caller asked for, and the target behind opus moves. The CLI that does the discovering and activating updates on its own schedule and is not recorded at all. Two runs a week apart can disagree and the artifacts hold nothing that separates a skill getting worse from a dependency that changed.
• Risk: low. Three keys added to an artifact, no key changed or removed, no new flag, no extra agent call.

What it looks like

Before, two reports a week apart:

routing-20260907-143545.json   6 of 8
routing-20260914-101200.json   4 of 8

After:

"model": "sonnet",
"engine": "legacy",
"agent_cli_version": "2.1.270",
"model_resolved": "claude-sonnet-5",

Same model, new CLI build, so the CLI is where to look.

Where each field comes from

• engine is a constant. There is one engine, and naming it means a reader does not have to date a report to work out what graded it.
• agent_cli_version is claude --version, cached for the process. One subprocess, no tokens.
• model_resolved is read out of the preflight call every graded run already pays for, so it costs nothing. It is null when --skip-preflight was passed, which is honest: nothing spoke to the API, so nothing knows.

The preflight result reports usage per model rather than naming the one that ran, and the CLI bills auxiliary work to a second model in the same block (a run asking for sonnet can come back with a session-title model listed beside it). The alias that was asked for is what picks the right entry; with no alias, the entry that did the most work.

Test plan

• 11 new cases in TestARunRecordsWhatProducedIt. Full suite 237, green on python -m unittest discover -s tests -t ..
• The auxiliary-model case is not hypothetical. The first version of the extraction took the first entry and reported gpt-5.5 for a run that asked for sonnet, against a live API. Reverting the fix with the tests in place turns that test red and nothing else, so it is pinned to the behaviour rather than to the shape of the code.
• End to end against a real catalog: skillscope routing --skills-dir 'skills/*' --routing-room all --only finetune-on-laptop --model sonnet, meta block as shown above.
• Extraction also checked against a missing modelUsage, an empty one, a list where an object belongs, and unparseable output. All degrade to null rather than raising.

[nowycondro]

🤖 Automated review · review-code · head 86d8d161

Review — needs work

Full review of the whole change.
No ticket named. PR #21 describes adding engine/agent_cli_version/model_resolved to the JSON meta block of routing and behavioral reports; the shipped diff matches that description exactly (same 3 files, same field set, and the PR's own before/after example illustrates the feature purely via the JSON artifact, not the markdown report).

Blocking

• resolved_model()'s docstring overclaims what None means — skillscope/agent.py:127-135, corroborated by skillscope/agent.py:180-217 (check_api_reachable)
  The docstring says: "This is None when the preflight was skipped, which is honest: nothing has spoken to the API, so nothing knows." But check_api_reachable returns (True, "ok") based solely on proc.returncode == 0 (line ~213), independent of whether _model_from_result(proc.stdout or "", model) (line 217) actually extracts a model. If the preflight reaches the API successfully but the JSON payload doesn't parse or carries no usable modelUsage entry (a shape _model_from_result already has graceful return None paths for), _RESOLVED_MODEL is set to None even though the API was reached. resolved_model() then returns None, which the docstring wrongly equates exclusively with "preflight was skipped." The PR description repeats the identical framing in prose ("It is null when --skip-preflight was passed, which is honest...") while separately acknowledging elsewhere that malformed/empty modelUsage also degrades to null — so the author knows these are different cases, but the documented contract doesn't say so, inviting a future caller to treat "didn't ask" and "asked, got an ambiguous answer" as the same thing.
  Confidence 78 · logic · Fix: soften the docstring to "None means the API was never asked, or was asked but didn't say which model answered" rather than tying None solely to "skipped"; optionally have check_api_reachable surface the parse failure distinctly.

Non-blocking

• ENGINE = "legacy" collides with a term the codebase already uses for something else — skillscope/cli.py:86 (and its comment at ~84-85) vs. skillscope/routing.py:5,34 and skillscope/behavior.py:13
routing.py's module docstring opens "Routing engine: does the right skill fire..." and closes "...this module is the engine."; behavior.py's docstring likewise ends "...this module is the engine." Those are two pre-existing, distinct "engines" (one per grading module). agent.py's own new comment on _RESOLVED_MODEL ("every engine wants it in its report") uses the word in that same established plural sense. ENGINE = "legacy" introduces a third, singular, repo-wide sense (a harness-generation label), and both cmd_routing and cmd_behavioral reports carry the identical "engine": "legacy" value regardless of which of the two established "engines" produced the report — so the field cannot answer "which engine ran this," only "which harness generation ran this," despite reusing a name the codebase already assigns a narrower meaning.
  Confidence 72 · architectural · Fix: rename the new field (e.g. harness or grading_engine_version) to avoid the collision, or fold the command type into the value.
• New ambient module-global has no setter, unlike the codebase's established convention — skillscope/agent.py:101 (_RESOLVED_MODEL), :127-135 (resolved_model()), :216-217 (mutation inside check_api_reachable); tests/test_skillscope.py:2520-2521, 2568, 2575
  The codebase has two prior precedents for "ambient state for the current process": deadline.py's _active/active()/use(bound) and config.py's _active/active()/use(config) — both pairs install a new value through a public setter that returns the previous one, and every existing test goes through that setter. This PR adds a third such global (_RESOLVED_MODEL) with only a getter (resolved_model()); check_api_reachable mutates it via a bare global statement, and the new test class is the only place in the suite that reaches into a module's private ambient-state variable directly (agent._RESOLVED_MODEL = None, restored via addCleanup(setattr, ...)). No production path is affected today (every real skillscope invocation is a fresh process), but the inconsistency is visible and the test workaround it produces is distinctive.
  Confidence 75 · architectural · Fix: add a use_resolved_model(value) mirroring deadline.use/config.use, and have the test class call it instead of poking the private attribute.

Decisions for the author

• New provenance fields never reach the markdown report, only the JSON artifact — answered-by-intent
  _provenance() (skillscope/cli.py:89-103) is spliced into the meta dict at cli.py:422 and :473, but routing.render_markdown() (routing.py:701+) and behavior.render_markdown() only ever read meta['model']/meta['effort']/meta['skills'] — never engine, agent_cli_version, or model_resolved. _write_report (cli.py:120-138) writes the full summary dict (meta included) to the JSON file, but only the markdown string to stdout and $GITHUB_STEP_SUMMARY — the channel README.md:71 documents as the human-facing report. This would otherwise have been Blocking (confidence 85, logic): the only surface most readers see never shows data the change's own rationale frames as being for exactly that kind of reader. It is retired by the PR's own "What it looks like" section, which illustrates the feature entirely through JSON: "Before, two reports a week apart: routing-20260907-143545.json 6 of 8 / routing-20260914-101200.json 4 of 8" and an "After" example shown as a raw JSON meta-block snippet ("model": "sonnet", "engine": "legacy", ...). The author's own comparison workflow is file-to-file JSON diffing, not the markdown summary. Confirm that's still the intended usage surface — if reviewers are in practice expected to compare via CI's step summary rather than downloaded JSON artifacts, this gap reopens.
• Alias-substring matching has no tie-break for multiple hits — tradeoff
  _model_from_result's alias branch (agent.py:138-176) returns the first modelUsage entry (in dict/insertion order) whose name contains the alias substring; the no-alias branch instead disambiguates by highest outputTokens. If a preflight's billing block ever contains two entries both matching the alias (e.g. two dated snapshots of the same model family), the alias branch silently picks whichever came first rather than the one that did the most work. Given today's model-naming conventions this is unlikely to collide, and simplicity has value — but the no-alias branch already shows the more robust option is cheap to apply here too. Confirm the current asymmetry is deliberate.
• Process-lifetime lru_cache on cli_version() — non-blocking-improvement
  cli_version() is decorated @lru_cache(maxsize=1) (agent.py:104), caching for the lifetime of the Python process. Harmless for the one-shot CLI entry point (one process, one invocation), but any future caller importing skillscope.agent as a library in a longer-lived process (a notebook, a service, a test harness calling main() repeatedly) would get a silently stale CLI version with no automatic reset — unlike _RESOLVED_MODEL, which is at least reassigned on every preflight. Worth a short note if this module is ever expected to be used outside a fresh-process CLI invocation.

Positive signals

• _model_from_result's alias-disambiguation logic is tested against a real, previously-encountered failure (per the PR's test plan: an early version returned an auxiliary session-title model for a sonnet preflight against a live API) rather than a hypothetical — the regression test is pinned to that behavior, not to the code's shape.
• Test isolation discipline is solid despite the new mutable module state: setUp/addCleanup save and restore agent._RESOLVED_MODEL and explicitly clear agent.cli_version's lru_cache, and the mocks patch real dependencies (shutil.which, subprocess.run) rather than the functions under test, so the tests exercise real parsing logic.

Deployment notes

• GitHub reports this PR's mergeable_state as dirty (merge conflicts against current main) at the time of this review — main has moved substantially past this PR's merge-base for unrelated reasons, so a rebase will be needed before merging regardless of this review's findings.
• Purely additive: three new JSON keys in an already-schemaless meta dict (no additionalProperties: false constraint anywhere on the run-report shape, unlike the unrelated skillscope/schema/machine.schema.json which governs a different file). No flag, config, or migration changes.

What this covered

Read completely, at pr21-base (0f734b2dd344cd72b5e016ad6c3b1ac2ce36f6f3) and pr21-head (86d8d16134d8deb82553c16dc3263724459ad59f): skillscope/agent.py, skillscope/cli.py, and tests/test_skillscope.py in full (not just the diff hunks), plus skillscope/routing.py and skillscope/behavior.py's summarize()/render_markdown(), docs/usage.md, README.md, pyproject.toml and tools/compare_skill.py (checked as possible consumers of the new fields; none found). Three independent background workers ran the code pass (4a) and the history, prior-changes, and code-comments passes (4b), each blind to the PR description and to each other's output; all three completed and reported findings, none failed or needed re-spawning. Findings were consolidated and verified against source directly by the orchestrator before the PR description was read (Step 9).

Did not run: agent-instruction-adherence pass — no CLAUDE.md or AGENTS.md exists anywhere in this repository at pr21-head (confirmed by tree listing), so there is nothing to check. Did not independently execute the test suite; relied on GitHub's check-runs API for commit 86d8d16134d8deb82553c16dc3263724459ad59f, which reports all 5 checks (Unit tests on ubuntu/windows × Python 3.10/3.12, plus Action against a throwaway repo) as completed/success, taken at face value.

Not reconciled against any discussion: this PR has no existing reviews or review comments, so there was nothing to reconcile.

---

Not a human review. This is an automated code review; please verify findings independently before acting on them.

[johnl-amd]

Review — needs work

Full review of the whole change (base 0f734b2…head 86d8d16).
Implements exactly what it describes: three provenance fields (engine, agent_cli_version, model_resolved) added to the meta block of routing/behavioral reports. No ticket named.

Blocking

• This PR's base is stale enough to conflict with main, and its central premise is already false there — skillscope/cli.py, skillscope/agent.py
  GitHub reports this PR's merge state as CONFLICTING/DIRTY. main has, since this branch was cut, replaced the single-engine model with a --engine flag (ENGINES = ["claude-code", "claude-code-no-sandbox"], a dynamic "engine": args.engine already written into the same meta dict this PR touches) and no longer recognizes "legacy" as an engine at all. This PR's ENGINE = "legacy" module constant and _provenance() helper are built on the "there is one engine" assumption stated in its own docstring, which main has already outgrown. Landing this as-is would hardcode every future report's engine field to "legacy" regardless of which engine actually produced it — the opposite of what the PR is for. This needs a real rebase (use main's existing args.engine instead of the constant), not just conflict markers resolved.
  Confidence 90 · architectural · Fix: rebase onto current main; source engine from args.engine, not a hardcoded constant.

• _model_from_result crashes instead of degrading to None on a non-dict top-level JSON payload — skillscope/agent.py (_model_from_result, check_api_reachable)
  json.loads(stdout) can legally return a list, None, a number, a string, or a bool; only TypeError/ValueError are caught before the unconditional payload.get("modelUsage"). Reproduced directly: agent._model_from_result('[]', 'opus') raises AttributeError: 'list' object has no attribute 'get', and this propagates uncaught out of check_api_reachable, which crashes cmd_routing/cmd_behavioral with a traceback instead of returning the (False, detail) its own docstring promises. This directly contradicts the PR's stated guarantee that malformed preflight output "degrade[s] to null rather than raising" — that claim was verified for a malformed modelUsage, but not for a malformed top-level payload. No test in the new TestARunRecordsWhatProducedIt covers a non-dict top-level JSON value.
  Confidence 100 (reproduced) · logic · Fix: add if not isinstance(payload, dict): return None right after the json.loads call, matching the isinstance discipline already used for modelUsage and detail.

• Alias matching can silently pick the wrong model when two served entries both match the alias substring — skillscope/agent.py (_model_from_result)

  if alias:
      needle = alias.lower()
      for name in served:
          if needle in name.lower():
              return name

  This returns the first dict-iteration-order match, not the one that did the most work, unlike the no-alias fallback three lines below which explicitly uses max(..., key=tokens). Reproduced: given {"claude-opus-4-1-fallback": 1 token, "claude-opus-5-20260722": 500 tokens} (both match alias "opus"), the function returns whichever one happens to appear first in the raw JSON — the 1-token fallback in one ordering, the 500-token real answer in the other — with identical inputs otherwise. This is exactly the "a model rollover mid-session" scenario the function's own docstring describes as the reason the disambiguation logic exists, except with an alias present instead of absent. No test exercises an alias matching more than one served entry, so a regression or an unlucky real-world ordering here would ship silently — undermining the one correctness property ("the same model produced both" scores) this PR exists to make verifiable.
  Confidence 90 (reproduced, order-dependent on identical data) · logic · Fix: among entries matching the alias, pick by max(tokens) the same way the no-alias branch already does, rather than the first match.

• _RESOLVED_MODEL is bare, unreset module state, unlike this codebase's established pattern for per-command ambient state — skillscope/agent.py (_RESOLVED_MODEL, resolved_model), skillscope/cli.py (main)
  deadline.py sets precedent for exactly this shape of problem: a value that must be scoped to "this command's run," installed via deadline.use(bound) and explicitly cleared via deadline.use(None) in main()'s finally. _RESOLVED_MODEL gets the same kind of "ambient, used by every engine" comment but no matching reset. Reproduced: calling check_api_reachable("opus") once (simulating one graded command) and then calling cli._provenance() again without a second preflight (simulating a later --skip-preflight run in the same process) returns the first run's model_resolved value instead of None — directly contradicting the function's own docstring ("None when the preflight was skipped, which is honest: nothing has spoken to the API, so nothing knows"). No caller in this repo currently invokes cli.main() twice in one process, so this isn't exercised by today's CI usage, but agent.py is also documented and used as a library directly (from skillscope.agent import claude), and the test suite itself has to manually save/restore this exact global in setUp/addCleanup to stay isolated — a sign the state is already awkward to reason about.
  Confidence 85 (contract violation reproduced; real-world trigger path is latent, not currently exercised by any caller in this repo) · architectural · Fix: reset _RESOLVED_MODEL = None at the start of each graded command (e.g. top of _prepare_graded_run), mirroring deadline.use(None).

Non-blocking

• Provenance lands only in the JSON meta block, not the markdown summary — tradeoff
  render_markdown in both routing.py and behavior.py prints meta['model']/meta['effort'] but never the new engine/agent_cli_version/model_resolved fields, so a reader of GITHUB_STEP_SUMMARY or stdout — likely the first place anyone actually looks — still can't tell two runs apart without opening the JSON artifact. The PR's own description only promises the meta block, so this isn't a description mismatch, just worth the author confirming it's deliberate.
• check_api_reachable's docstring doesn't mention its new side effect — non-blocking-improvement
  It now also sets module state (_RESOLVED_MODEL) as well as returning (ok, detail); worth a one-line mention given a reader elsewhere in the file won't expect a "preflight" function to have a side effect outside its return value.

Decisions for the author

• "Most output tokens" as the no-alias disambiguator — tradeoff
  A reasonable choice, but not the only one (e.g. "last billed" or any future "primary" marker the CLI might add). Worth confirming it was a deliberate pick rather than the first thing that worked.

Positive signals

• Good mutation-testing discipline: the PR describes hitting the exact alias-disambiguation bug against a live API, and a dedicated test pins it — verified for real by reverting that branch to "take the first entry" and watching two tests fail (and only those two).
• _model_from_result's degrade-to-None philosophy for malformed modelUsage shapes (missing, empty, wrong type) is well tested and matches its stated intent, even though the same discipline wasn't extended to the top-level payload shape (see Blocking).

Deployment notes

• This PR's merge state is currently CONFLICTING against main (see first Blocking item) — it cannot land without a rebase.
• CI is green at the reviewed commit (86d8d16): all four unit-test matrix legs and "Action against a throwaway repo" passed.

What this covered

Read completely: skillscope/agent.py and skillscope/cli.py in full (both call sites of the new _provenance() splice), the new TestARunRecordsWhatProducedIt test class (11 cases) plus enough of the surrounding test file to confirm convention, and routing.py/behavior.py's summarize()/render_markdown() to see how the meta dict is actually consumed. Checked skillscope/schema/machine.schema.json, docs/usage.md and README.md for documentation this change would make stale (none found — the meta block isn't documented field-by-field anywhere). Ran the full suite (237 tests) at the reviewed commit in a clean venv: green, matching the PR's own claim. Independently reproduced two of the findings above by direct execution against the PR's code, and ran one blind fan-out worker over the same three files (no access to this reasoning); its two headline findings matched and are folded in above after independent re-confirmation. Checked CI status and mergeability via gh pr view --json statusCheckRollup,mergeable,mergeStateStatus, and confirmed the staleness finding against origin/main's actual commit history and current cli.py.

Did not run: agent-instruction adherence (no CLAUDE.md/AGENTS.md in this repository — no subject), prior-changes (no prior review history on these files accessible without reading this PR's own discussion, which is out of scope for this method). Not reconciled against any discussion on this PR.

---

🤖 Automated review by agent-hub on AMD AgentHub.
Model claude-sonnet-5[1m] · Not a human review. Reply here if a finding is wrong.