Repository navigation
fix(provider): OpenRouter generation ID collection and provider attribution - #295
Conversation
PR Summary by QodoFix OpenRouter generation ID collection and provider attribution
AI Description
Diagram
High-Level Assessment
Files changed (24)
|
|
🤖 Finished Review · ✅ Success · Started 5:23 PM UTC · Completed 5:43 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $2.68 |
Code Review by Qodo
1. Cleanup lacks root and home guards
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if not projects_dir.exists(): | ||
| return | ||
| encoded = "-" + str(workspace).replace("/", "-") | ||
| encoded = claude_session_dir_name(workspace) |
There was a problem hiding this comment.
2. Cleanup lacks root and home guards 📜 Skill insight ⛨ Security
_cleanup_session passes session_dir to shutil.rmtree after only an existence/type check, without resolving it and rejecting / or Path.home() immediately before deletion. The changed session-directory encoding feeds this deletion path during normal runs, so any unexpected path resolution that widens session_dir reaches recursive cleanup without the required boundary check.
Agent Prompt
## Issue description
`_cleanup_session` recursively deletes the computed Claude session directory without resolving the target and rejecting the filesystem root or the current user's home directory immediately before deletion.
## Fix Focus Areas
- agent_eval/agent/claude_code.py[831-837]
## Recommended Fix
Resolve `session_dir` immediately before `shutil.rmtree`, reject targets equal to `Path('/').resolve()` or `Path.home().resolve()`, and then perform the recursive deletion only after the safety check passes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # A pinned run with zero generation rows means collection or backfill | ||
| # failed, not that every request was compliant. | ||
| audit_complete = unattributed == 0 and (audited > 0 or not declared) |
There was a problem hiding this comment.
3. Runs with preference-only routing always fail strict audit 🐞 Bug ≡ Correctness
The new audit_complete = unattributed == 0 and (audited > 0 or not declared) checks whether
declared holds any non-empty routing spec. But audited only goes up for rows whose spec is
pinned (only, or order with allow_fallbacks: false), so an order that allows fallbacks, a
quantizations-only entry, or an entry for a model this run never used is declared yet can never be
audited. Every such run now reports audit_complete: false, which makes --strict-routing exit 2
('routing audit incomplete'), makes compare/anova refuse to pool the run, and gets the run flagged
as unaudited in reports and MLflow tags, even though nothing was pinned and nothing went wrong.
Agent Prompt
## Issue description
`routing_audit` marks the audit incomplete whenever `declared` is non-empty and `audited == 0`. `declared` includes unpinned preference specs (an `order` that allows fallbacks, quantizations only, `sort`, ...) and specs for models the run never used, and rows under those keys are never audited. Every such run reports `audit_complete: false` and fails `--strict-routing`.
## Fix Focus Areas
- agent_eval/providers/openrouter/audit.py[110-122]
## Recommended Fix
Build a set of pinned routing keys, i.e. keys in `declared` for which `pinned_providers(table.for_model(key))` is not None. Apply the zero-rows rule only to those, for example: `audit_complete = unattributed == 0 and (audited > 0 or not pinned_keys)`. Better still, restrict it to pinned keys the run actually requested, such as the plan's skill/subagent routing keys. Add tests for a preference-only table and for a table that pins a model the run did not use.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| fb_input += u.get("input_tokens") or 0 | ||
| fb_output += u.get("output_tokens") or 0 | ||
| fb_cache_read += u.get("cache_read_input_tokens") or 0 | ||
| fb_cache_create += u.get("cache_creation_input_tokens") or 0 |
There was a problem hiding this comment.
5. A null usage block still crashes token accounting 🐞 Bug ☼ Reliability
extract_usage still reads obj.get("message", {}).get("usage", {}), which returns None when the
event contains "usage": null, so the following u.get(...) raises AttributeError. The same
happens with a null per-model stats entry in modelUsage. On the success path extract_usage
runs outside any try, so a provider that nulls the whole usage object (not just its fields) still
crashes the case, which the PR's null-usage fix was meant to prevent.
Agent Prompt
## Issue description
A `"usage": null` or a null `modelUsage` entry still makes `extract_usage` raise AttributeError.
## Fix Focus Areas
- agent_eval/agent/stream_capture.py[106-139]
## Recommended Fix
Use `msg = obj.get("message") or {}` and `u = msg.get("usage") or {}`, and skip or empty-default `stats` when it is not a dict (`stats = stats if isinstance(stats, dict) else {}`). Add a test that feeds a stream with `"usage": null`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Risk Assessment: moderate (2/5) DetailsModerate risk driven by a large blast radius across 26 files and a protected script edit in execute.py, balanced by an experienced contributor, low 30-day commit churn, no CI or dependency changes, accompanying unit test coverage, and full alignment with linked issue #296. Previous run<!-- fullsend:risk-assessment --> DetailsModerate risk driven by a large blast radius across 25 files and a protected script edit in execute.py, balanced by an experienced contributor, low 30-day commit churn, no CI or dependency changes, and accompanying unit test coverage. Previous run (2)<!-- fullsend:risk-assessment --> DetailsModerate risk driven by 24 changed files with large blast radius and a protected script edit, balanced by an experienced contributor, low 30-day commit churn, no CI or dependency changes, and accompanying unit test coverage. |
ReviewFindingsMedium
Low
Earlier findings
Blocking prior findings:
Next steps:
2 finding(s) below the Previous run<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJyZXF1ZXN0LWNoYW5nZXMiLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImVkZ2UtY2FzZSIsImZpbGUiOiJhZ2VudF9ldmFsL2FnZW50L3N0cmVhbV9jYXB0dXJlLnB5IiwiaWQiOiJmXzdkMDAxYmM2Nzc3MzVlYTkiLCJsaW5lIjoyODMsImFjdGlvbmFibGUiOnRydWUsImRpbWVuc2lvbiI6ImNvcnJlY3RuZXNzIn0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoiYXBpLWNvbnRyYWN0IiwiZmlsZSI6ImFnZW50X2V2YWwvaGFyYm9yL3Jlc3VsdHMucHkiLCJpZCI6ImZfNDBjOTE5NDNkYjlmZjI2ZSIsImxpbmUiOjEyMSwiYWN0aW9uYWJsZSI6dHJ1ZSwiZGltZW5zaW9uIjoiY29ycmVjdG5lc3MifSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJiYWNrd2FyZC1pbmNvbXBhdGlibGUiLCJmaWxlIjoiYWdlbnRfZXZhbC9ldmFsaHViL2FkYXB0ZXIucHkiLCJpZCI6ImZfNDkwYzM0MGJmMTczNGQ4YiIsImxpbmUiOjI2MCwiYWN0aW9uYWJsZSI6dHJ1ZSwiZGltZW5zaW9uIjoiY3Jvc3MtcmVwby1jb250cmFjdHMifSx7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJlZGdlLWNhc2UiLCJmaWxlIjoiYWdlbnRfZXZhbC9hZ2VudC9zdHJlYW1fY2FwdHVyZS5weSIsImlkIjoiZl85OTU2NmY0NGRiMmZjYzRjIiwibGluZSI6Mjg0LCJhY3Rpb25hYmxlIjp0cnVlLCJkaW1lbnNpb24iOiJjb3JyZWN0bmVzcyJ9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImJhY2t3YXJkLWluY29tcGF0aWJsZSIsImZpbGUiOiJhZ2VudF9ldmFsL2V2YWxodWIvYWRhcHRlci5weSIsImlkIjoiZl9hM2Y0ZTg2YzYwNGUxMjQxIiwibGluZSI6MjYyLCJhY3Rpb25hYmxlIjp0cnVlLCJkaW1lbnNpb24iOiJjcm9zcy1yZXBvLWNvbnRyYWN0cyJ9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluamVjdGlvbi12dWxuIiwiZmlsZSI6ImFnZW50X2V2YWwvYWdlbnQvc3RyZWFtX2NhcHR1cmUucHkiLCJpZCI6ImZfZWMzMWRhMmZhMjkzOTk2ZSIsImxpbmUiOjMwOCwiYWN0aW9uYWJsZSI6dHJ1ZSwiZGltZW5zaW9uIjoic2VjdXJpdHkifSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJtaXNzaW5nLWF1dGhvcml6YXRpb24iLCJmaWxlIjoiYWdlbnRfZXZhbC9hZ2VudC9iYXNlLnB5IiwiaWQiOiJmX2IzZTliZTYzYzFjZWNkMGEiLCJhY3Rpb25hYmxlIjpmYWxzZSwiZGltZW5zaW9uIjoiaW50ZW50LWNvaGVyZW5jZSJ9XSwiZGlzcG9zaXRpb25zIjpbeyJpZCI6ImZfNDBjOTE5NDNkYjlmZjI2ZSIsInN0YXR1cyI6InJlY2xhc3NpZmllZCJ9LHsiaWQiOiJmXzk5NTY2ZjQ0ZGIyZmNjNGMiLCJzdGF0dXMiOiJvcGVuIn0seyJpZCI6ImZfZWMzMWRhMmZhMjkzOTk2ZSIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmX2IzZTliZTYzYzFjZWNkMGEiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl9hM2Y0ZTg2YzYwNGUxMjQxIiwic3RhdHVzIjoib3BlbiJ9XX0= --> ReviewFindingsMedium
Low
Earlier findings
Labels: PR fixes a bug in OpenRouter generation ID collection Next steps:
Previous run (2)<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJyZXF1ZXN0LWNoYW5nZXMiLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImFwaS1jb250cmFjdCIsImZpbGUiOiJhZ2VudF9ldmFsL2hhcmJvci9yZXN1bHRzLnB5IiwiaWQiOiJmXzQwYzkxOTQzZGI5ZmYyNmUiLCJsaW5lIjoxMjEsImFjdGlvbmFibGUiOnRydWUsImRpbWVuc2lvbiI6ImNvcnJlY3RuZXNzIn0seyJzZXZlcml0eSI6Im1lZGl1bSIsImNhdGVnb3J5IjoiZWRnZS1jYXNlIiwiZmlsZSI6ImFnZW50X2V2YWwvYWdlbnQvc3RyZWFtX2NhcHR1cmUucHkiLCJpZCI6ImZfOTk1NjZmNDRkYjJmY2M0YyIsImxpbmUiOjI4NCwiYWN0aW9uYWJsZSI6dHJ1ZSwiZGltZW5zaW9uIjoiY29ycmVjdG5lc3MifSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJpbmplY3Rpb24tdnVsbiIsImZpbGUiOiJhZ2VudF9ldmFsL2FnZW50L3N0cmVhbV9jYXB0dXJlLnB5IiwiaWQiOiJmX2VjMzFkYTJmYTI5Mzk5NmUiLCJsaW5lIjozMDgsImFjdGlvbmFibGUiOnRydWUsImRpbWVuc2lvbiI6InNlY3VyaXR5In0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoibWlzc2luZy1hdXRob3JpemF0aW9uIiwiZmlsZSI6ImFnZW50X2V2YWwvYWdlbnQvYmFzZS5weSIsImlkIjoiZl9iM2U5YmU2M2MxY2VjZDBhIiwiYWN0aW9uYWJsZSI6ZmFsc2UsImRpbWVuc2lvbiI6ImludGVudC1jb2hlcmVuY2UifSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJiYWNrd2FyZC1pbmNvbXBhdGlibGUiLCJmaWxlIjoiYWdlbnRfZXZhbC9ldmFsaHViL2FkYXB0ZXIucHkiLCJpZCI6ImZfYTNmNGU4NmM2MDRlMTI0MSIsImxpbmUiOjI2MiwiYWN0aW9uYWJsZSI6dHJ1ZSwiZGltZW5zaW9uIjoiY3Jvc3MtcmVwby1jb250cmFjdHMifV19 --> ReviewFindingsHigh
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: apetruni <apetruni@redhat.com>
109a8f5 to
83cac99
Compare
|
🤖 Finished Review · ❌ Failure (validation failed after 1 iteration(s)) · Started 9:59 AM UTC · Completed 10:22 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $0.52 |
Signed-off-by: apetruni <apetruni@redhat.com>
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 12:08 PM UTC · Completed 12:20 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $3.01 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
astefanutti
left a comment
astefanutti
left a comment
There was a problem hiding this comment.
Reviewed against main at d87a7ab. Verified locally on 83cac99: the full suite passes (2880), no new lint findings in the changed files, mkdocs build --strict is clean, and the branch merges cleanly onto today's main. #296 covers the execute.py protected-path edit.
Direction is right and the plumbing is consistent. Separating Claude's msg_* ids (turn accounting) from OpenRouter's gen-* ids (cost truth, attribution) is the correct model, and generation_ids reaches every consumer — runner, provider session, backfill, reconcile, Harbor, EvalHub, execute.py — with legacy fallbacks for old run results. The persisted-transcript reader is hardened (session-id format, symlink rejection, containment). The null-usage fix is right. On Claude Code 2.1.314 I can confirm the premise locally: the transcript's top-level requestId and message.id are different identifiers.
Before merge (details inline):
- The local runner has no fallback for the old id shape.
_extract_transcript_metrics(Harbor) accepts agen-message.idwhenrequestIdis absent;assistant_event_ids,extract_generation_idsandsubagent_generation_idsdo not. Any Claude Code + OpenRouter combination that still echoes the generation id asmessage.id— what spec 014 probe #12 verified in September — would now collect zero generation ids locally, the symptom this PR fixes. Same fallback instream_capture.py, plus a test with the legacy event shape. extract_session_idreads onlyresultevents (fullsend's finding above, agreed): on a timeout or crash the persisted transcript is never read on exactly the paths where stream-json is least complete. Fall back to thesystem/initevent'ssession_id.- Spec 014 evidence. The edit replaces a statement labelled VERIFIED (probe #12) without recording what changed. Label the new claim VERIFIED with the live-run evidence (date, Claude Code version, model, and the observation that
requestIdwas in the persisted transcript but not always in stream-json), mark probe #12 as superseded, and update the rows that still namemessage_idsas the key set (cost_confidence/cost_coverageat ~423-424, the architecture table at ~111). One question that decides the Harbor follow-up: under which condition does stream-json omitrequestId?
Worth recording, no code change asked:
- The root-cause narrative in the description is slightly off. The old cleanup encoded the path with a double leading dash (
"-" + "/Users/..."), so it never matched a real session directory: transcripts were not deleted before extraction, they were never read — and session directories accumulated. The new encoding makes cleanup actually delete, which is why reading before cleanup now matters. Worth saying so, and expect long-accumulated session directories to start disappearing per run. - The
audit_completechange is good and intended: a pinned run with zero generation rows now reads as incomplete in report, compare, anova and MLflow instead of compliant. Mention it in the description / release note. Same for fullsend's point onmessage_ids_countin the EvalHub provenance: a sentence in the docs pointing consumers togeneration_ids_count. - Harbor trials capture stream-json only (fullsend's finding, agreed): if a container's Claude Code omits
requestId,generation_idsstays empty there. Separate issue — exporting the session transcript from the trial. - Qodo's remaining root/home guard before
rmtree: the encoding cannot produce either path, so a one-linesession_dir != projects_dirguard is only there to close the thread. - The PR title is not a conventional commit; squash-merge as
fix(provider): …so semantic-release cuts the patch.
| return ids | ||
|
|
||
|
|
||
| def assistant_event_ids(obj): |
There was a problem hiding this comment.
No fallback for the older shape here (nor in extract_generation_ids at 352 and subagent_generation_ids at 237), while harbor/results.py:122 has one: when requestId is absent and message.id starts with gen-, that id is the generation id (spec 014 probe #12, verified 2026-09-16). Without it, a CLI/OpenRouter combination that still echoes gen ids as message.id collects zero generation ids on the local runner. Suggest the same rule in all three helpers and a test with the legacy event shape (message.id: gen-…, no requestId).
| return sorted(ids) | ||
|
|
||
|
|
||
| def extract_session_id(stdout_lines): |
There was a problem hiding this comment.
Agree with the review bot above: on a timeout or crash there may be no result event, so the persisted transcript is never read on exactly the paths where stdout is least complete. The system/init event carries session_id as well — prefer the latest result, fall back to init, with a test for a truncated stream plus a persisted transcript.
| `gen-…` generation id (probe #12: 2 per turn, including the `generate_session_title` | ||
| background call, whose cost is real spend and is therefore included), so each id is handed | ||
| to the `generation.py` worker **as the stream is read** (a per-message callback the | ||
| (:165) transcripts. Current OpenRouter transcripts carry the generation id in the |
There was a problem hiding this comment.
This replaces a VERIFIED statement (probe #12, 2026-09-16: message.id was the gen-… id) without saying what changed. Please label the new claim VERIFIED with the live-run evidence — date, Claude Code version, model, and that requestId was present in the persisted transcript but not always in stream-json — and annotate probe #12 in the checklist as superseded. The cost_confidence / cost_coverage rows (~423-424) and the architecture table (~111) still describe message_ids as the key set. Also: under which condition does stream-json omit requestId? That answer decides the Harbor follow-up.
| if not projects_dir.exists(): | ||
| return | ||
| encoded = "-" + str(workspace).replace("/", "-") | ||
| encoded = claude_session_dir_name(workspace) |
There was a problem hiding this comment.
Good fix. For the record: the old encoding ("-" + str(workspace).replace("/", "-")) produced a double leading dash, so cleanup never matched a real session directory — the transcripts were not being deleted before extraction, they were never read, and session directories accumulated. With this change cleanup really deletes, which is why reading the transcript first now matters. Worth a sentence in the description; users will see long-accumulated session directories disappear per run.
| if k in ("order", "only", "ignore", "allow_fallbacks", "quantizations")} | ||
| # A pinned run with zero generation rows means collection or backfill | ||
| # failed, not that every request was compliant. | ||
| audit_complete = unattributed == 0 and (audited > 0 or not declared) |
There was a problem hiding this comment.
Intended and correct: a pinned run with zero generation rows now reads as incomplete rather than compliant. Note that report.py, compare.py, analyze.py and log_results.py all key on audit_complete is False, so such runs will now be flagged in every consumer. Worth a line in the description / release note.
Aliciapet11
changed the title
Signed-off-by: apetruni <apetruni@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 1:00 PM UTC · Completed 1:11 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $3.07 |
| return set() | ||
| try: | ||
| projects = (Path(projects_dir) if projects_dir is not None | ||
| else Path.home() / ".claude" / "projects").resolve(strict=True) |
There was a problem hiding this comment.
[low] edge-case
claude_session_generation_ids reads persisted transcripts from Path.home()/.claude/projects unless projects_dir is passed, and no ClaudeCodeRunner call site passes it. If a user overrides CLAUDE_CONFIG_DIR (or HOME) via runner.env/extra_env, ids present only in the persisted transcript are lost; stream and subagent ids are still collected. The base-branch _cleanup_session makes the same assumption.
Suggested fix: Derive the projects directory from the subprocess environment _build_env produces (CLAUDE_CONFIG_DIR, then HOME), pass it as projects_dir, use the same directory in _cleanup_session, and add a runner-level test with a non-default config directory.
| @@ -255,6 +258,7 @@ def _finish_provider_plan(session, run_dir: Path, case_results: list) -> dict: | |||
| final = payload | |||
| provenance = {k: final.get(k) for k in PROVENANCE_FIELDS if k in final} | |||
| provenance["message_ids_count"] = len(final.get("message_ids") or []) | |||
There was a problem hiding this comment.
[low] backward-incompatible
Previously reported finding remains open: low backward-incompatible in agent_eval/evalhub/adapter.py:260.
|
Thanks @astefanutti , I addressed your review comments in d895f34:
|
astefanutti
left a comment
astefanutti
left a comment
There was a problem hiding this comment.
Thanks @Aliciapet11!
## [1.62.2](v1.62.1...v1.62.2) (2026-11-05) ### Bug Fixes * **provider:** OpenRouter generation ID collection and provider attribution ([#295](#295)) ([c8687a9](c8687a9))
Summary
message_idsfrom OpenRoutergeneration_ids.requestIdvalues beginning withgen-*from:generation_idsto the OpenRouter provider session and backfill.nullusage fields without crashing token accounting.After sharing the results of the POC, we got Michael’s feedback about needing to report the specific provider that served each request. While digging into that, we found that the harness was collecting Claude’s
msg_*IDs instead of OpenRouter’sgen-*generation IDs. A live run confirmed the symptom:message_idswere populated, butgeneration_idswas empty, no OpenRouter ledger rows were written, and the routing audit showed0 audited.Further investigation showed that Claude Code’s raw
stream-jsonoutput did not always include the top-levelrequestId, even though the persisted Claude session transcript did. The harness was deleting that session transcript before extracting IDs, so thegen-*IDs were lost before OpenRouter backfill could use them. The same live test also exposed a crash when OpenRouter emittednullusage fields; that is fixed here too.Validation
Review follow-ups