feat: measure security scan tokens across workers - #265
Conversation
|
@codex review Please review the exact current head |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…elo/codex/sync-cross-worker-token-usage
|
@codex review Please review the exact updated head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 593c550e3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result["totalTokens"] = ( | ||
| result["inputTokens"] - result["cachedInputTokens"] + result["outputTokens"] | ||
| ) |
There was a problem hiding this comment.
For every snapshot containing cached input, this replaces the producer's required total_tokens value with inputTokens - cachedInputTokens + outputTokens. The repository's existing parser defines the same payload's total as input + output (sdk/typescript/src/cost.ts:339-345), so a snapshot with 100 input, 40 cached, and 20 output is now reported as 80 total tokens instead of 120. Since completion guidance publishes this value to users, normal cache usage makes the new accounting materially underreport scan consumption.
Useful? React with 👍 / 👎.
| accepted_thread_ids.add(session.thread_id) | ||
| observed_thread_count += 1 | ||
| _add_token_usage(total, session_usage) |
There was a problem hiding this comment.
When a rollout has valid identity/ownership records but contains no valid token_count event in the scan window—for example, when a provider omits usage metadata—_read_rollout_usage returns an all-zero map without a warning, yet these lines count the thread as successfully observed. If every thread is in that state, the result is coverage: "complete" with zero tokens, causing the completion workflow to present zero as a measurement rather than the documented unavailable result.
Useful? React with 👍 / 👎.
| return None | ||
| result: dict[str, int] = {} | ||
| for source_key, result_key in TOKEN_FIELDS.items(): | ||
| value = usage.get(source_key, 0) |
There was a problem hiding this comment.
When total_token_usage uses the supported cache_write_tokens spelling, this lookup silently defaults cacheWriteInputTokens to zero. The existing parser for the same payload explicitly accepts both cache_write_input_tokens and cache_write_tokens (sdk/typescript/src/cost.ts:324-325), so affected scans are marked completely measured while underreporting cache-write usage; apply the same fallback here.
Useful? React with 👍 / 👎.
| SELECT DISTINCT sdk_thread_id | ||
| FROM deep_scan_workers | ||
| WHERE scan_id = ? AND sdk_thread_id IS NOT NULL |
There was a problem hiding this comment.
When a deep worker retries on a fresh SDK thread, deep_scan_workbench.py:989-1002 overwrites that worker's prior sdk_thread_id, while this query loads only the final value. A scan that succeeds after such a retry therefore omits all tokens consumed by the earlier attempt even though those tokens were scan-owned; persist attempt thread IDs separately or derive every historical attempt root when collecting usage.
Useful? React with 👍 / 👎.
| workspace = connection.execute( | ||
| "SELECT thread_id FROM workspaces WHERE id = ?", | ||
| (scan["workspace_id"],), | ||
| ).fetchone() | ||
| if workspace is not None: | ||
| candidates.append(workspace["thread_id"]) |
There was a problem hiding this comment.
When continuation_thread_id differs from the workspace's original thread, both IDs are added as roots. The original workspace thread is long-lived, and for a root session without a recorded parent _read_rollout_usage treats ownership as established immediately, so any unrelated turns or subagents used on that setup thread while the scan continuation runs are included solely because their timestamps fall inside the scan window. Use the workspace thread only as a fallback when no continuation or deep owner is available, or establish a scan-specific ownership boundary for it.
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…elo/codex/sync-cross-worker-token-usage
|
@codex review Please review the exact updated head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e616407d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| complete_scan.add_argument("--scan-id", required=True) | ||
| complete_scan.add_argument("--claim-token") | ||
| complete_scan.add_argument("--cost-json") | ||
| complete_scan.add_argument("--thread-id") |
There was a problem hiding this comment.
When an MCP completion runs from a thread that is not already persisted as the workspace, continuation, or deep-scan owner—such as a recovered continuation—this new argument is never populated. I inspected the bundled complete_codex_security_scan handler in mcp/server.mjs.br.part-*: unlike other thread-aware handlers, its callback does not read extra and invokes complete-scan with only --scan-id and --claim-token. Consequently collect_scan_usage cannot use the current thread as a root and can report unavailable or omit that thread and its descendants; pass threadIdFromExtra(extra) through --thread-id.
Useful? React with 👍 / 👎.
| if source_key in {"input_tokens", "output_tokens", "total_tokens"} and ( | ||
| source_key not in usage | ||
| ): | ||
| return None |
There was a problem hiding this comment.
When a valid total_token_usage snapshot contains input_tokens and output_tokens but omits the redundant total_tokens field, this check rejects the whole record even though the value is immediately recomputed below. The repository's established parser accepts this payload and derives the total (src/cost.ts:320-345), and existing session fixtures use that shape, so affected rollouts are marked token_record_invalid and completion publishes partial zero usage instead of the measured tokens; require only input and output here.
Useful? React with 👍 / 👎.
| boundary_reached = False | ||
|
|
||
| with session.path.open("rb") as source: | ||
| for line_number, raw_line in enumerate(source, start=1): |
There was a problem hiding this comment.
When a discovered rollout contains a very large JSONL event, such as a command or tool result with oversized output, iterating the binary file this way materializes the entire record and the subsequent json.loads builds the full object even though almost all event types are irrelevant to accounting. The existing session reader explicitly enforces a 1 MiB event limit (src/cost.ts:59-61,252-261), but this completion path has no bound, so repository-influenced output can make complete-scan exhaust memory or hit its MCP timeout instead of returning usage; read incrementally with a maximum record size and mark oversized records unavailable.
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb4ed32c1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if result["cachedInputTokens"] + result["cacheWriteInputTokens"] > result["inputTokens"]: | ||
| return None | ||
| result["totalTokens"] = result["inputTokens"] + result["outputTokens"] |
There was a problem hiding this comment.
When a rollout reports reasoning_output_tokens > output_tokens, this snapshot is accepted and can be published with coverage: "complete" even though the counts are internally impossible under this repository's token model. The established parser rejects exactly this condition in sdk/typescript/src/cost.ts:328-336; apply the same check here so malformed provider metadata produces partial coverage rather than trusted usage.
Useful? React with 👍 / 👎.
| delta = { | ||
| key: value - previous[key] if value >= previous[key] else value | ||
| for key, value in snapshot.items() |
There was a problem hiding this comment.
When cumulative usage resets and the new combined total is lower but one component remains above its previous value, this per-field reset detection mixes epochs. For example, moving from input/output 10/100 to 15/20 produces deltas 5/20/35, so input is underreported and totalTokens no longer equals input plus output while coverage can remain complete. Detect a reset from the combined snapshot and then treat every current field as fresh.
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
1 participant
Summary
Stack
mdangelo/codex/sync-scan-lifecycle-metadataso this review contains only its atomic upstream change.Verification