fix(ai-engineer): distinguish REST 5xx from invalid gh credentials - #2438
fix(ai-engineer): distinguish REST 5xx from invalid gh credentials#2438cursor[bot] wants to merge 3 commits into
Conversation
@codex review |
![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: 596ecad6d2
ℹ️ 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".
| `env -u GH_TOKEN -u GITHUB_TOKEN gh api graphql -f query='{viewer{login}}'` (same cleared-env / | ||
| host-level path as the REST probe). Accept the identity when GraphQL returns `devantler`. | ||
| Only an explicit credential rejection from that host-level check proves the saved login invalid — | ||
| meaning HTTP 401/403, or `gh` reporting the token rejected / not logged in — and **only then** recommend |
There was a problem hiding this comment.
When the /user probe is rate-limited, a valid credential can receive HTTP 403; GitHub documents that exceeding a rate limit can return either 403 or 429 (GitHub REST API rate-limit documentation). This rule would therefore declare the saved login invalid and recommend gh auth login without trying the GraphQL fallback, unnecessarily hard-blocking the scheduled run. Check the response's rate-limit headers/message and classify such 403s as service degradation or verification unavailable, reserving reauthentication for an actual credential-rejection signal.
Useful? React with 👍 / 👎.
| `/user` (or `gh auth status`) probe that returns HTTP 5xx, HTML, or other non-JSON service noise is | ||
| **not** proof the credential is bad — classify that outcome as `GitHub service degraded` and run a | ||
| bounded authenticated GraphQL fallback | ||
| `env -u GH_TOKEN -u GITHUB_TOKEN gh api graphql -f query='{viewer{login}}'` (same cleared-env / |
There was a problem hiding this comment.
When GH_HOST selects an enterprise host, this fallback can query that host instead of the github.com host explicitly checked by the preceding REST command; the CLI documents that GH_HOST supplies the hostname when one is not provided (gh environment variables). The fallback can consequently reject a valid GitHub.com login based on an unrelated account, or accept an enterprise account named devantler without verifying the saved GitHub.com credential. Add --hostname github.com so both probes test the same host.
Useful? React with 👍 / 👎.
| `/user` (or `gh auth status`) probe that returns HTTP 5xx, HTML, or other non-JSON service noise is | ||
| **not** proof the credential is bad — classify that outcome as `GitHub service degraded` and run a | ||
| bounded authenticated GraphQL fallback | ||
| `env -u GH_TOKEN -u GITHUB_TOKEN gh api graphql -f query='{viewer{login}}'` (same cleared-env / |
There was a problem hiding this comment.
When the initial machine-local gh auth status is using a valid injected GH_TOKEN or GITHUB_TOKEN and its REST request returns 5xx, this literal fallback removes both tokens and tests the saved login instead. On a host with no saved login, or with a stale one, GraphQL will then report not logged in or rejected and the run can recommend gh auth login even though the injected credential was never checked against GraphQL. Run the fallback in the same credential context as the failing REST probe, clearing the variables only when the failing probe was itself the cleared-environment saved-login check.
Useful? React with 👍 / 👎.
Three Codex P2s assessed: all VALID — parked (#2484)I checked each against what the rule actually says rather than accepting or dismissing them as a block, and all three describe real misclassifications in a gate whose entire purpose is to classify correctly:
All three share one root: the gate infers a credential verdict from signals that also carry service and host state. The fix is to classify on an explicit rejection signal only, and to keep host and credential context identical across both probes. Unpushable from any sibling lane — text on a Closing the Cursor-lane sweep for this run, with the measurement. Six drafts reviewed: #2437 and #2422 clean → merged; #2432, #2488 and this one returned valid findings → parked. The two that landed were pure documentation; all three that stalled assert logic. I am stopping the sweep here rather than spending more of a weekly-metered lane on the remaining CLEAN drafts (#2477, #2467, #2450, #2446, #2447, #2436, #2435, agent-skills#76), which are all feature or logic changes and so fall on the parking side of that split. Reviewing them now would consume review capacity to produce findings nobody may act on. |
> 🤖 Generated by the Daily AI Engineer (Cursor cloud instance) Co-authored-by: Nikolai Emil Damm <devantler@users.noreply.github.com>
Preflight was treating GitHub REST/service failures as a bad saved login. Add a GraphQL viewer.login fallback and classify 5xx separately from 401/403. Fixes #2206 Co-authored-by: Nikolai Emil Damm <devantler@users.noreply.github.com>
596ecad to
2ae87d6
Compare
Pin the GraphQL auth fallback to github.com, keep the failing probe's credential context, and stop classifying rate-limited 403/429 as a bad login. Updates the preflight contract test accordingly. Co-authored-by: ned <ned@devantler.tech>
@coderabbitai review The three previously recorded classification findings were addressed at the current head. Please review the exact head. |
|
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe GitHub authentication procedure now separates invalid credentials from REST service failures, non-JSON responses, and rate limits. It uses a host-pinned GraphQL Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/scripts/maintainer-preflight.test.sh:
- Around line 47-71: Extend the assertions in the maintainer preflight contract
checks to require the documented deployment-scoped viewer.login identity rule
for app/cursor, and verify the corresponding “wrong GitHub identity”
classification. Anchor these checks to the existing run_loop grep assertions
without changing the credential-context or rejection-gating checks.
In @.claude/skills/portfolio-maintenance/SKILL.md:
- Around line 60-75: Update the authentication decision rules in the documented
GitHub probe flow so explicit credential rejection takes precedence: classify
HTTP 401 and confirmed non-rate-limit 403 as invalid credentials before
evaluating response format. Restrict the HTML/non-JSON service-degradation
classification to other statuses, preserving the required gh auth login
recommendation for authentication failures.
- Around line 70-80: Update the GraphQL fallback contract in
.claude/skills/portfolio-maintenance/SKILL.md, around the host-level
authentication guidance at lines 70-80, to compare viewer.login against the
deployment’s expected identity rather than hardcoding devantler; classify any
mismatch as wrong GitHub identity. Extend
.claude/scripts/maintainer-preflight.test.sh lines 47-71 with assertions
covering the deployment-scoped identity and wrong-identity handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 932a4695-eb86-4db3-b82f-91fb7247190a
📒 Files selected for processing (2)
.claude/scripts/maintainer-preflight.test.sh.claude/skills/portfolio-maintenance/SKILL.md
📜 Review details
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-07-20T18:30:45.905Z
Learnt from: devantler
Repo: devantler-tech/monorepo PR: 2309
File: .claude/scripts/portfolio-surveyor.test.sh:35-38
Timestamp: 2026-07-20T18:30:45.905Z
Learning: For prose “subagent definition” documents under `.claude/agents/**` (e.g., `portfolio-surveyor.md`) that are interpreted by an LLM rather than executed deterministically: in CI, use focused textual contract assertions (e.g., checking required sections, prompts, and structural constraints). Do not attempt behavioral validation by simulating it via non-deterministic model invocations; behavioral validation should require live Surveyor runs.
For executable components under `.claude/scripts/**` (e.g., `release-bot-exemption.sh`): use fixture-driven, deterministic tests that exercise the script logic with controlled inputs/outputs (rather than model calls).
Applied to files:
.claude/scripts/maintainer-preflight.test.sh
📚 Learning: 2026-07-29T16:38:50.481Z
Learnt from: devantler
Repo: devantler-tech/monorepo PR: 2554
File: .claude/scripts/worktree-cleanup.test.sh:36-55
Timestamp: 2026-07-29T16:38:50.481Z
Learning: In `.claude/scripts` test fixture scripts, when running Git setup commands (e.g., `git worktree add`, `git submodule add`), do not suppress failures (e.g., by ignoring exit codes or redirecting errors to `/dev/null`) without explicitly checking command status. A setup failure can otherwise cause contract tests to falsely pass (for example, due to empty/incorrect Git porcelain output). Preserve stderr for diagnostics and fail the fixture immediately if setup steps return a non-zero exit code (use `set -e`/`set -o pipefail` and/or explicit `if ! command; then ...; exit 1; fi`).
Applied to files:
.claude/scripts/maintainer-preflight.test.sh
📚 Learning: 2026-07-29T22:21:01.108Z
Learnt from: devantler
Repo: devantler-tech/monorepo PR: 2566
File: .claude/scripts/merge-confirmation-read.test.sh:0-0
Timestamp: 2026-07-29T22:21:01.108Z
Learning: In shell test harness/ablation scripts, capture the exit status of the command being tested immediately into `rc=$?` on the very next line. Avoid using `$?` as an argument to another command (e.g., `printf ... "$?"` or similar) when the same line also includes `$(...)` command substitution, because shell evaluation/expansion order can replace the tested command’s exit status with the exit status of the command substitution, producing incorrect measurements.
Applied to files:
.claude/scripts/maintainer-preflight.test.sh
📚 Learning: 2026-07-07T16:28:59.865Z
Learnt from: devantler
Repo: devantler-tech/monorepo PR: 2077
File: .claude/adr/0001-kagent-agent-fleet-substrate.md:35-35
Timestamp: 2026-07-07T16:28:59.865Z
Learning: In this repo, markdownlint CI only runs for `docs/**`, so Markdown files under `.claude/` (e.g., ADRs like `.claude/adr/*.md`) won’t be automatically linted. During code review, treat `.claude/**/*.md` as needing the same markdownlint-clean style as the docs—e.g., avoid MD022 issues like missing/incorrect blank lines around headings—and flag markdownlint violations even though CI won’t catch them.
Applied to files:
.claude/skills/portfolio-maintenance/SKILL.md
🪛 Shellcheck (0.11.0)
.claude/scripts/maintainer-preflight.test.sh
[info] 65-65: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
🪛 SkillSpector (2.4.4)
.claude/skills/portfolio-maintenance/SKILL.md
[error] 40: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 49: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 59: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 69: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
| grep -Fq "gh api graphql --hostname github.com -f query='{viewer{login}}'" "${run_loop}" || | ||
| fail "missing the authenticated GraphQL viewer.login fallback pinned to github.com" | ||
|
|
||
| grep -Fq 'rate-limited 403/429' "${run_loop}" || | ||
| fail "missing the rate-limit-as-service-degradation classification" | ||
|
|
||
| grep -Fq 'same host and credential context' "${run_loop}" || | ||
| fail "GraphQL fallback does not preserve the failing probe's credential context" | ||
|
|
||
| grep -Fq 'HTTP **401**' "${run_loop}" || | ||
| fail "missing the explicit HTTP 401 authentication-rejection criterion" | ||
|
|
||
| grep -Fq 'non-rate-limit' "${run_loop}" || | ||
| fail "missing the non-rate-limit 403 credential-rejection criterion" | ||
|
|
||
| grep -Fq 'A REST 5xx (or' "${run_loop}" || | ||
| fail "missing the REST-503-plus-GraphQL-success regression rule" | ||
|
|
||
| grep -Fq 'recommend' "${run_loop}" && grep -Fq 'gh auth login' "${run_loop}" || | ||
| fail "missing the gh-auth-login-only-on-confirmed-rejection handoff rule" | ||
|
|
||
| # The handoff must be gated on confirmed rejection — not on every auth-status failure. | ||
| grep -Fq 'and **only then** recommend' "${run_loop}" || | ||
| fail "missing the confirmed-rejection gate before recommending gh auth login" | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add a contract assertion for the deployment-scoped fallback identity.
The test checks the GraphQL command and credential context, but it does not check which viewer.login value is accepted. It can pass while the skill rejects the documented cloud identity app/cursor. Assert the deployment-scoped identity rule and the wrong GitHub identity classification.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 65-65: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/scripts/maintainer-preflight.test.sh around lines 47 - 71, Extend
the assertions in the maintainer preflight contract checks to require the
documented deployment-scoped viewer.login identity rule for app/cursor, and
verify the corresponding “wrong GitHub identity” classification. Anchor these
checks to the existing run_loop grep assertions without changing the
credential-context or rejection-gating checks.
| **Distinguish authentication rejection from GitHub service degradation** (monorepo#2206): a REST | ||
| `/user` (or `gh auth status`) probe that returns HTTP 5xx, HTML, other non-JSON service noise, **or | ||
| a rate-limited 403/429** (GitHub may return either status when the limit is exceeded — check | ||
| `x-ratelimit-*` headers or a rate-limit message body) is **not** proof the credential is bad — | ||
| classify that outcome as `GitHub service degraded` and run a bounded authenticated GraphQL | ||
| fallback against the **same host and credential context** as the failing probe: | ||
| `gh api graphql --hostname github.com -f query='{viewer{login}}'`. Prefix with | ||
| `env -u GH_TOKEN -u GITHUB_TOKEN` **only when the failing probe itself was the cleared-env | ||
| saved-login check**; otherwise keep the injected `GH_TOKEN`/`GITHUB_TOKEN` so a transient REST | ||
| failure cannot be misread as a bad keychain login. Always pass `--hostname github.com` so | ||
| `GH_HOST` cannot redirect the fallback to an unrelated enterprise host. Accept the identity when | ||
| GraphQL returns `devantler`. | ||
| Only an explicit credential rejection proves the login invalid — meaning HTTP **401**, a **non-rate-limit** | ||
| 403 that is clearly a credential/permission rejection (never a rate-limit 403), or `gh` reporting | ||
| the token rejected / not logged in — and **only then** recommend `gh auth login`. A REST 5xx (or | ||
| rate-limit) with a successful GraphQL `viewer.login` must never be reported as an invalid saved |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply authentication-status precedence before service-noise handling.
Line [61] classifies any HTML or non-JSON response as GitHub service degraded, without limiting that rule to non-authentication statuses. Line [72] separately says HTTP 401 and a confirmed non-rate-limit 403 prove credential rejection. State the precedence explicitly: classify 401 and confirmed non-rate-limit 403 as credential rejection first; apply the non-JSON service-degradation rule only to other statuses. Otherwise, an HTML 401 response can skip the required gh auth login recommendation.
🧰 Tools
🪛 SkillSpector (2.4.4)
[error] 40: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 49: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 59: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 69: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/portfolio-maintenance/SKILL.md around lines 60 - 75, Update
the authentication decision rules in the documented GitHub probe flow so
explicit credential rejection takes precedence: classify HTTP 401 and confirmed
non-rate-limit 403 as invalid credentials before evaluating response format.
Restrict the HTML/non-JSON service-degradation classification to other statuses,
preserving the required gh auth login recommendation for authentication
failures.
| `GH_HOST` cannot redirect the fallback to an unrelated enterprise host. Accept the identity when | ||
| GraphQL returns `devantler`. | ||
| Only an explicit credential rejection proves the login invalid — meaning HTTP **401**, a **non-rate-limit** | ||
| 403 that is clearly a credential/permission rejection (never a rate-limit 403), or `gh` reporting | ||
| the token rejected / not logged in — and **only then** recommend `gh auth login`. A REST 5xx (or | ||
| rate-limit) with a successful GraphQL `viewer.login` must never be reported as an invalid saved | ||
| login. If the host-level check instead authenticates a different account, hard-block as | ||
| `wrong GitHub identity` without describing the credential as invalid. | ||
| If the host-level check cannot run or fails for a transport reason (and the GraphQL fallback is | ||
| likewise unreachable), hard-block as `authentication verification unavailable` instead of | ||
| instructing the maintainer to replace a credential that was never tested. Keep the injected-token |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the GraphQL identity contract deployment-aware and test it.
The skill defines deployment-specific identities but hardcodes devantler for the fallback. The contract test does not detect this mismatch.
.claude/skills/portfolio-maintenance/SKILL.md#L70-L80: compareviewer.loginwith the deployment's expected identity and classify a mismatch aswrong GitHub identity..claude/scripts/maintainer-preflight.test.sh#L47-L71: add assertions for the deployment-scoped identity and wrong-identity handling.
🧰 Tools
🪛 SkillSpector (2.4.4)
[error] 40: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 49: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 59: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
[error] 69: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
📍 Affects 2 files
.claude/skills/portfolio-maintenance/SKILL.md#L70-L80(this comment).claude/scripts/maintainer-preflight.test.sh#L47-L71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/portfolio-maintenance/SKILL.md around lines 70 - 80, Update
the GraphQL fallback contract in .claude/skills/portfolio-maintenance/SKILL.md,
around the host-level authentication guidance at lines 70-80, to compare
viewer.login against the deployment’s expected identity rather than hardcoding
devantler; classify any mismatch as wrong GitHub identity. Extend
.claude/scripts/maintainer-preflight.test.sh lines 47-71 with assertions
covering the deployment-scoped identity and wrong-identity handling.
Current-head review triage: 3 valid findingsAt bd40976:
The exact-head contract test passes and CI is green, but those checks do not cover these gaps. This remains a draft with all three current-head threads unresolved. The local sibling is not permitted to push code to a cursor branch; the cross-lane completion gap is tracked in #2484. |
2 participants
Why
When GitHub REST
/userreturns a transient 5xx/HTML outage page, preflight was treating a valid savedghlogin as invalid and telling the maintainer to re-rungh auth login. That stops an otherwise authorized tick for the wrong reason.What
Teach the maintainer preflight to classify REST 5xx / service noise separately from auth rejection, fall back to authenticated GraphQL
viewer.login, and recommendgh auth loginonly on confirmed 401/403. Contract tests pin the regression.Fixes #2206