Repository navigation
feat(action): report usage and extend integration attribution - #1703
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d89564450
ℹ️ 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".
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the reporting and attribution improvements. My owner-level merge-safety review still reproduces the existing cached-token finding at d3c875d27a91e7b0a71c8ffcaeb69f6ddb4cc325. A result serialized by ScanResult with {input_tokens: 1200, output_tokens: 50} is accepted as completed, and its cost calculation defaults cached input to zero, but the new summary calls toLocaleString() on the omitted cached count. Through runAction, that changes the job to failure and disables SARIF upload. Please normalize the optional cached count consistently before formatting, with a regression through the result adapter and Action entrypoint. I have not duplicated the existing inline comment.
All 155 Action tests and validation checks passed, including bundle reproducibility. Another 55 focused SDK/example/cost tests passed. I checked both examples' shell syntax and full/diff arguments, their pinned CLI parsers, and 16 local-only native request probes across the pinned and candidate Codex versions; attribution and explicit analytics settings were preserved in both request headers and bodies. The full local CLI integration suite stops at an existing trusted-owner environment restriction before the new fixture; the matching hosted CI run passed that suite. This review is against the current #1691 stack base; retain the documented merge order.
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the update. At 3934360, the existing cached-token finding still needs attention. ScanResult accepts usage with input and output counts but no cached-input count, and cost calculation defaults that count to zero. The summary instead calls toLocaleString() on the absent value. I reproduced the completed scan becoming an Action failure with SARIF upload disabled through the actual Action entrypoint. Please normalize this optional count consistently and add an adapter-to-Action regression.
Fresh SDK compilation and all three reproduction/control cases passed. The Action code, tests, generated bundle and dependency graph match the reviewed 651d5ad inputs, so its 173 Action tests, 56 focused SDK tests, eight native localhost probes, and build/type/format checks remain applicable; those broader checks were not repeated. The updated example pin and new-base integration were reviewed.
Full installed-CLI/platform suites and external model scans were not exercised locally; the established fixture ownership limitation remains.
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed 25e19529ccd3 against its exact updated base. The full feature patch and dependency inputs are unchanged from the independently reviewed 39343608e7a8; the intervening changes match the separately reviewed main integration. The previously verified issues remain in unchanged code and still need attention. See the existing detailed review for the reproduction and requested corrections.
- Normalize the optional cached-input count before formatting the summary
No duplicate inline comments added.
Validation: exact feature/dependency comparisons support reusing the prior focused tests and reproductions with their recorded limitations; those suites were not rerun on this rebase. The shared main integration was independently reviewed and its 189 owner/configuration/CLI checks passed. This does not claim a complete current-head integration or platform run.
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for fixing the optional cached-token count at the result boundary. The completed-scan and SARIF regression is resolved, while missing whole usage remains unavailable and measured zero stays visible. I found no remaining actionable issue in the full 11-file change.
Validation: 174 Action tests, 19 workflow-example tests, two real SDK serializer-to-Action regressions, and eight native request-attribution probes passed. Hosted validation on a merge tree identical to this head also passed the pinned CLI integration fixture. The local full installed-CLI suite was not repeated because of the established filesystem-owner guard; native attribution probes use a local synthetic endpoint and do not claim live provider scans.
Summary
The standalone GitHub and Azure workflow examples identify their integration, and the GitHub Action summary shows the configured provider, token usage, and estimated cost. Missing usage or pricing remains unavailable. Usage that omits the optional cached-input count uses zero, matching the SDK, so a completed scan remains successful.
Changes
Testing
Merged
mainatfe9d77e1c3d9into1b8e477b8877. This refresh changes only the Node 26.12.1 image tag and digest in both Dockerfile stages; all other tracked files are identical to4d0fd244b789.git diff --checkpassed. Runtime tests and container builds were not rerun for this refresh; prior results below remain historical, and fresh exact-head hosted CI is required.Previous integration merged
mainat5f521759e7c6into4d0fd244b789. Linux checks on that recorded head passed:build:ci, SDK/MCP types, full SDK formatting, andgit diff --check.Validation used Node 22.23.3 and Bun 1.3.14. Dependency lockfiles and unchanged plugin sources were matched before reusing dependency installations and private copies of existing bundled/native artifacts. Native binaries were not rebuilt. Full SDK and cross-platform suites were not rerun; fresh hosted CI must qualify this head.
Earlier Action and installed-CLI results remain historical. The latter used the pinned 0.2.0 package with local synthetic OpenAI/Bedrock fixtures and no external model calls. Earlier hosted status does not establish readiness of this refreshed head.
Risk and rollout
No new CLI flags, Action inputs or outputs, dependency upgrades, or telemetry collectors. Existing analytics opt-outs remain unchanged. Request fixtures use synthetic credentials and localhost endpoints; they verify outbound attribution rather than provider-side reporting or completed model scans.
Public disclosure review