fix(grok): don't claim auth staged after a failed oauth refresh - #1060
Conversation
grok_oauth_refresh() already warns on refresh failure and correctly falls through to the on-disk token instead of hard-failing (existing, deliberate behavior - test_run_grok.sh case 3d covers rc=0 + warn + keep token). the bug is the very next line: it unconditionally logs "CLI + auth staged" regardless of whether refresh actually worked. that false-positive means a genuinely dead credential (e.g. invalid_grant because an earlier run's rotated refresh token never got persisted back to the secret) looks identical in the logs to a completely healthy setup, right up until the run dies downstream on an already-expired token with no obvious link back to auth. reproduced and confirmed against a real fork instance running this exact code (#79): vuln-scanner failed 21/21 runs over 11 days with this precise signature, and "CLI + auth staged" was in every one of those logs. tracks a GROK_OAUTH_DEGRADED flag through the three genuine-failure paths in grok_oauth_refresh() and makes the final log line honest about it. exit code contract is unchanged.
aaronjmars
left a comment
aaronjmars
left a comment
There was a problem hiding this comment.
Thanks for this - the diagnosis is excellent, and the run-grok.sh change is correct: GROK_OAUTH_DEGRADED=1 is set on all three hard-fail paths, the flag mutates in the current shell (not a subshell), and promoting the degraded log from ::debug:: to ::warning:: gives it a visible signal. The regression test genuinely fails on main and passes with the fix. Nice work.
One gap keeps the PR from fully delivering its title, though. The visible line you quoted in the description - grok: CLI + auth staged (auth: native-oauth) - isn't emitted by run-grok.sh. It comes from scripts/install-harness.sh:68:
echo "grok: CLI + auth staged (auth: ${AUTH_MODE:-native})" ;;
install-harness.sh calls run-grok.sh setup and then unconditionally echoes that line. Your PR gates run-grok.sh but not install-harness.sh, so after this merges a degraded run emits the new ::warning::...DEGRADED... and still prints grok: CLI + auth staged right next to it. The loud warning is a real win, but the contradictory claim you set out to remove is still there.
Could you extend this PR to make install-harness.sh:68 reflect the degraded state too? The one constraint: run-grok.sh setup is deliberately non-fatal (it returns 0 even when degraded), so install-harness.sh can't just check the exit code. It needs a sentinel - e.g. have run-grok.sh touch a marker file (or write a known string to a path/stderr) on the degraded paths, and have install-harness.sh read it before choosing between "auth staged" and "auth degraded (using existing token, may be expired)".
Happy to merge as soon as that line stops lying. Everything else here is good to go.
…aded run-grok.sh now signals a degraded OAuth refresh across the process boundary via a GROK_DEGRADED_MARKER file (the shell flag can't reach install-harness.sh, which runs it as a subprocess and always exits 0 by design). install-harness.sh hands it a marker path and prints an honest "auth DEGRADED" line instead of the false "auth staged" when the marker comes back touched. Adds test_run_grok.sh coverage for the marker on both the degraded and healthy paths.
|
Went ahead and wired the caller side myself so this lands complete. Pushed one commit to your branch:
Full suite is green (15/15) and shellcheck-clean. Your diagnosis and the run-grok.sh fix were spot on; this just closes the last visible-line gap. Thanks for the clean report - merging now. |
Follow-up to the #1042-#1061 docs sync (#1063), which had already merged: - CHANGELOG.md: #1060 (grok: don't claim auth staged after a failed oauth refresh) merged in-window but was omitted from the sync. Add it. - skills/aeon SKILL.md: the "N of 81 skills lead with the procedure" stat kept its numerator at 43 when the denominator moved 80 to 81; miroshark-matchday leads with a Step 1 procedure, so it is 44 of 81. Fixed in both the .claude and plugin copies. Also converts the line's em dash to a hyphen.
2 participants
what
on a fork instance running this exact code,
vuln-scannerfailed 21/21 runs over 11 days. traced the actual production failure:the refresh legitimately failed and fell through to the on-disk token - that's existing, deliberate behavior (
test_run_grok.shcase 3d already covers rc=0 + warn + keep token, and this PR doesn't change it). the bug is the line right after it:grok setup complete (CLI + auth staged)prints unconditionally, so a run with dead auth looks identical in the logs to a completely healthy one. that's why it took 11 days to notice on the fork - the real signal (the warning) was in every single log, just contradicted by the line under it.this doesn't fix any specific credential - that's a per-repo operational issue (reconnect the X account, check the secrets-write PAT scope). what it fixes is that the next time this happens, anywhere this code runs, the logs will say so instead of lying.
how
tracks a
GROK_OAUTH_DEGRADEDflag through the three genuine-failure paths ingrok_oauth_refresh()(request failed, no access_token in the response, couldn't rewrite auth.json) and checks it before the final log line. exit code is unchanged - only the message changes from a false "staged" to an honest "DEGRADED" warning.poc
added test 3d2 to
test_run_grok.sh, reusing the existing invalid_grant fixture (3d). verified against this repo's actual current code: fails on main (reproduces the exact bug), passes with this fix. all pre-existing cases intest_run_grok.shpass unchanged.