Repository navigation
Include GitHub API rate limits in unified agent sessions - #66047
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed for PR #66047: has_implementation_label=false and default_business_additions=0 (<=100) across 9 changed files, with no custom .design-gate.yml. Neither Condition A nor Condition B is met.
|
|
🧠 Matt Pocock Skills Reviewer is reviewing this pull request using Matt Pocock's engineering skills... |
|
✅ PR Code Quality Reviewer completed the code quality review. Attempting recovery after write command approval denial while preparing PR #66047 review output.
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Credential attribution can be incorrect and does not cover per-handler GraphQL calls.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds GitHub API rate-limit observations and credential attribution to unified agent sessions.
Changes:
- Collects and normalizes rate-limit JSONL events.
- Classifies credentials for per-handler REST clients.
- Extends tests, types, schema, and specification.
| File | Description |
|---|---|
docs/src/content/docs/specs/unified-agent-session-specification.md |
Documents rate-limit events. |
docs/public/schemas/unified-session.schema.json |
Defines the new payload schema. |
actions/setup/js/unified_session.test.cjs |
Tests collection and fallback behavior. |
actions/setup/js/unified_session.cjs |
Adds the rate-limit source. |
actions/setup/js/unified_session_payload.cjs |
Normalizes allowed fields. |
actions/setup/js/types/unified_session.d.ts |
Adds TypeScript contracts. |
actions/setup/js/setup_globals.test.cjs |
Tests credential categories. |
actions/setup/js/setup_globals.cjs |
Classifies per-handler credentials. |
actions/setup/js/github_rate_limit_logger.cjs |
Records credential attribution. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if (process.env.GITHUB_TOKEN && token === process.env.GITHUB_TOKEN) return "github_actions"; | ||
| if (token.startsWith("github_pat_") || token.startsWith("ghp_")) return "pat"; | ||
| if (token.startsWith("ghs_") || token.startsWith("ghu_")) return "app"; |
There was a problem hiding this comment.
Removed the generic GITHUB_TOKEN comparison and token-prefix guessing. The compiler now passes canonical source provenance for the built-in client, and the explicitly PAT-backed handler auth path passes pat; other unproven tokens remain unknown. Fixed in 74f2487.
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES
The credential-attribution change is incomplete: it only tags getOctokit(...) clients, while the default github client still emits unattributed rate-limit events.
Blocking themes
- The built-in
githubclient is still wrapped without a credential category, so the session trace will stay incomplete for the majority ofgithub.rest.*traffic. - The new sink trusts arbitrary
credentialSourcestrings instead of enforcing the promised small category set at the write boundary.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 37.9 AIC · ⌖ 6.03 AIC · ⊞ 21.1K
Comment /review to run again
| applyGitHubApiVersion(requestOptions.headers, options.headers); | ||
| }); | ||
| return client; | ||
| return createRateLimitAwareGithub(client, credentialSource(token)); |
There was a problem hiding this comment.
This only annotates getOctokit(...) clients; the default global.github client is still wrapped without a credential source, so most handler calls will keep producing unattributed rate-limit events and the new session field will miss the main quota consumer.
💡 Why this blocks merge
The stated goal is to identify which credential consumed GitHub API quota, but this change only passes credentialSource(token) on the custom-client path. The injected github client that actions/github-script gives us is still initialized as createRateLimitAwareGithub(githubModule) with no source, and that client is what the bulk of github.rest.* usage in this tree goes through.
A minimal fix is to tag the built-in client when globals are set up, for example by threading the known Actions token source into that wrapper too:
global.github = createRateLimitAwareGithub(githubModule, "github_actions");If you need to avoid assuming the runtime token, derive the category once from the authenticated context and pass that canonical value into both wrapper sites so response_headers observations stay comparable.
There was a problem hiding this comment.
The Process Safe Outputs step now passes its compiler-derived credential source to setupGlobals, so rate-limit events from the default global.github client are attributed too. Fixed in 74f2487.
| source: "response_headers", | ||
| operation, | ||
| }; | ||
| if (credentialSource) entry.credentialSource = credentialSource; |
There was a problem hiding this comment.
logRateLimitFromResponse now persists whatever truthy credentialSource string the caller hands it, so one bad call site will write raw auth context straight into the JSONL log.
💡 Why this needs hardening
This helper is exported and the new parameter is a free-form string. Right now the only in-tree caller passes a sanitized category, but the sink itself does not enforce that contract, so a future caller can accidentally log a token value, a header blob, or other high-cardinality data and the unified-session collector will happily preserve it.
Clamp the value at the write boundary instead of trusting every caller, e.g. only allow the canonical categories (github_actions, pat, app, unknown) and drop anything else:
const allowed = new Set(["github_actions", "pat", "app", "unknown"]);
if (allowed.has(credentialSource)) entry.credentialSource = credentialSource;That keeps the observability field safe even if another caller gets this wrong later.
There was a problem hiding this comment.
The rate-limit logger now writes credentialSource only for the four canonical categories and omits invalid values; a test verifies raw token-like strings are not persisted. Fixed in 74f2487.
There was a problem hiding this comment.
Impeccable Skills Review
Change type: refactor_cleanup / backend observability (no UI) — applied general code-quality review (correctness, security, maintainability) since Impeccable's UI-focused modes (critique, audit, harden, distill, extract, clarify) do not apply to this backend JS/schema change.
Findings
No blocking or high-signal issues found. The change is well-scoped and consistent:
credentialSource()classification logic (setup_globals.cjs) correctly checks exactGITHUB_TOKENequality before prefix matching, never persists/logs raw tokens, and defaults safely to"unknown".logRateLimitFromResponse/createRateLimitAwareGithubthread the optionalcredentialSourcethrough without altering existing behavior when omitted (step-levelglobal.githubremains unclassified, matching the "per-handler" scope described in the PR).unified_session.cjscollector change follows the existing[candidates, component, phase, type]pattern;choose()already prefers the first existing candidate, satisfying the "prefer original over usage copy" requirement without new logic.- New
github_api.rate_limitevent type is consistently wired through.d.tstypes, the JSON Schema (additionalProperties:false, matching field set), the payload field-alias map (snake_case/camelCase aliases forcredential_source/delay_ms), and the specification doc. - Tests cover the credential classification matrix, malformed-line skipping, dedup across
github_rate_limits.jsonlvs. itsusage/copy, and unknown-field stripping (authorizationis correctly dropped).
Nothing actionable to request changes on.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 139.5 AIC · ⌖ 13.2 AIC · ⊞ 8.2K
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 33789a5
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Unified agent sessions did not include available GitHub API rate-limit evidence or identify which credential consumed the quota. This change brings those observations into the session trace where the information is available.