fix(doctor): post-eeqi credential checks + correct user guidance#602
Merged
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: SageOx <ox@sageox.ai> SageOx-Session: https://sageox.ai/repo/repo_019c5812-01e9-7b7d-b5b1-321c471c9777/sessions/2026-05-11T22-36-galexy-OxE2aB/view
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe ox doctor command's ledger health diagnostics were restructured to eliminate in-URL PAT checks and instead rely on credential-helper-based detection and remediation; origin parsing and helper verification were added, URL corrections now write bare origins and install helpers, and guidance/tests were updated. ChangesLedger Credential Handling Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmd/ox/doctor_ledger_git_test.go`:
- Line 40: The test contains a PAT-shaped literal
"https://oauth2:glpat-secret-token-do-not-leak@git.sageox.ai/team/ledger.git"
which triggers secret detectors; replace it with a non-PAT placeholder or
construct the URL at runtime (e.g., use an env var or join pieces like "glpat-"
+ "REDACTED") so the literal pattern is broken. Update the test in
doctor_ledger_git_test.go to use the placeholder constant or
os.Getenv("TEST_GIT_URL") and adjust any assertions accordingly.
In `@cmd/ox/doctor_ledger_git.go`:
- Around line 602-606: When url.Parse(apiURL) fails in the remediation path,
treat it as a failed remediation rather than a warning: replace the WarningCheck
creation with a FailureCheck (or the code path your checker uses for failed
fixes) and return that failure result (include the error text via
parseErr.Error()); ensure the created check uses a message like "failed to fix
(invalid API URL)" and still references checkName and parseErr so callers see
the concrete parse error when --fix is used (update the code that currently
creates r := WarningCheck(checkName, "cannot fix (invalid API URL)",
parseErr.Error()) to construct a failure result instead).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dcb9836e-81bc-475f-8ca2-ab2c7d8176a8
📒 Files selected for processing (11)
cmd/ox/doctor.gocmd/ox/doctor_check_registry.gocmd/ox/doctor_git_repos.gocmd/ox/doctor_ledger_git.gocmd/ox/doctor_ledger_git_test.gocmd/ox/doctor_ledger_secrets.gocmd/ox/doctor_ledger_secrets_test.gocmd/ox/doctor_types.gocmd/ox/prepush_scan.gocmd/ox/prepush_scan_test.gocmd/ox/session_redact_history.go
💤 Files with no reviewable changes (2)
- cmd/ox/doctor_types.go
- cmd/ox/doctor_git_repos.go
…n tests Co-Authored-By: SageOx <ox@sageox.ai> SageOx-Session: https://sageox.ai/repo/repo_019c5812-01e9-7b7d-b5b1-321c471c9777/sessions/2026-05-11T22-36-galexy-OxE2aB/view
Co-Authored-By: SageOx <ox@sageox.ai> SageOx-Session: https://sageox.ai/repo/repo_019c5812-01e9-7b7d-b5b1-321c471c9777/sessions/2026-05-11T22-36-galexy-OxE2aB/view
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/ox/doctor_ledger_secrets.go (3)
143-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake pre-scan hydration failures block a clean result.
hyd.Failedis only folded intoSessionsSkipped, but the decision matrix below only looks atSessionsHydrationFailed. If the pre-pass misses sessions and the later scan finds no secrets, this path can still return a clean result with incomplete coverage. Either carryhyd.FailedintoSessionsHydrationFailedas well, or gate the switch on a separatecoverageIncompleteflag that includes both sources.🤖 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 `@cmd/ox/doctor_ledger_secrets.go` around lines 143 - 145, The current logic adds hyd.Failed to result.SessionsSkipped but the final decision only checks result.SessionsHydrationFailed, allowing a false "clean" result when pre-scan hydration missed sessions; update the handling so pre-scan failures affect the coverage decision: either increment result.SessionsHydrationFailed by hyd.Failed in the same place where you increment SessionsSkipped, or introduce a coverageIncomplete boolean that is set if hyd.Failed>0 or result.SessionsHydrationFailed>0 and use that in the switch/decision below (refer to hyd.Failed, result.SessionsSkipped, result.SessionsHydrationFailed, and the switch that determines the clean result).
313-317:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t count unreadable files as successfully scanned.
scanLedgerFileForSecretscurrently returnsnilonos.Openandscanner.Err()failures, and the caller incrementsFilesScanned/anyFileReadbefore it knows the scan succeeded. That lets unreadable or truncated session files still count as scanned, which can preserve a false “clean” audit despite incomplete coverage.💡 Suggested fix
- anyFileRead = true - result.FilesScanned++ relForReport := filepath.Join("sessions", sessionName, filename) if err := scanLedgerFileForSecrets(redactor, contentPath, relForReport, info.ModTime(), result); err != nil { anyFileFailed = true continue } + anyFileRead = true + result.FilesScanned++f, err := os.Open(abs) if err != nil { - return nil // skip unreadable files; audit is best-effort + return fmt.Errorf("open %s: %w", rel, err) } @@ if err := scanner.Err(); err != nil && !errors.Is(err, io.EOF) { - return nil + return fmt.Errorf("scan %s: %w", rel, err) }Also applies to: 373-389
🤖 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 `@cmd/ox/doctor_ledger_secrets.go` around lines 313 - 317, scanLedgerFileForSecrets currently swallows I/O errors (os.Open and scanner.Err) causing the caller to mark files as scanned even when unreadable; update scanLedgerFileForSecrets to return a non-nil error on open/read/scan failures and only set anyFileRead and increment result.FilesScanned in the caller after scanLedgerFileForSecrets returns nil (look for the caller code around the assignment to anyFileRead and result.FilesScanned in doctor_ledger_secrets.go). Also apply the same change to the other caller block noted (around the 373-389 area) so unreadable/truncated session files are not counted as successfully scanned.
209-217: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReuse
getLedgerPath()instead of adding another resolution path.This helper duplicates ledger-path resolution inside
cmd/ox, which makes it easy for doctor to diverge from the rest of the CLI if the canonical lookup chain changes again. ReusinggetLedgerPath()here keeps these checks aligned with the established local-config/default fallback behavior.Based on learnings, in the
cmd/oxGo CLI commands, usegetLedgerPath()as the canonical way to resolve the ledger path whenrepoIDandendpointURLare not directly available.🤖 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 `@cmd/ox/doctor_ledger_secrets.go` around lines 209 - 217, The helper resolveLedgerPathForAudit duplicates ledger path resolution; replace its custom logic with a call to the canonical getLedgerPath() used by other cmd/ox commands so the doctor uses the same local-config/default fallback behavior. Locate resolveLedgerPathForAudit and remove the manual localCfg/Ledger.Path and ledger.DefaultPath() checks, instead invoking getLedgerPath() (using the same arguments/signature used elsewhere in cmd/ox) and return its result (propagating or handling any error per surrounding callers). This ensures consistency with getLedgerPath() for ledger path resolution.
🧹 Nitpick comments (1)
cmd/ox/prepush_scan_test.go (1)
221-221: ⚡ Quick winAssert the canonical remediation command explicitly
This now checks for
ox session audit, but the PR objective calls outox session redact-history --dry-runas the replacement guidance. Add an assertion for that exact command so this test guards the user-facing fix end-to-end.Suggested diff
assert.Contains(t, msg, "aws_access_key") assert.Contains(t, msg, "leak.jsonl:42") assert.Contains(t, msg, "ox session audit") + assert.Contains(t, msg, "ox session redact-history --dry-run") assert.Contains(t, msg, "OX_ALLOW_SECRETS=1")🤖 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 `@cmd/ox/prepush_scan_test.go` at line 221, The test currently only asserts assert.Contains(t, msg, "ox session audit") but needs to explicitly assert the canonical remediation command; update the test to also assert that msg includes the exact replacement "ox session redact-history --dry-run" (for example add assert.Contains(t, msg, "ox session redact-history --dry-run")) so the user-facing guidance is validated—ensure you modify the same test that sets/reads msg and leave the existing audit assertion in place.
🤖 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.
Outside diff comments:
In `@cmd/ox/doctor_ledger_secrets.go`:
- Around line 143-145: The current logic adds hyd.Failed to
result.SessionsSkipped but the final decision only checks
result.SessionsHydrationFailed, allowing a false "clean" result when pre-scan
hydration missed sessions; update the handling so pre-scan failures affect the
coverage decision: either increment result.SessionsHydrationFailed by hyd.Failed
in the same place where you increment SessionsSkipped, or introduce a
coverageIncomplete boolean that is set if hyd.Failed>0 or
result.SessionsHydrationFailed>0 and use that in the switch/decision below
(refer to hyd.Failed, result.SessionsSkipped, result.SessionsHydrationFailed,
and the switch that determines the clean result).
- Around line 313-317: scanLedgerFileForSecrets currently swallows I/O errors
(os.Open and scanner.Err) causing the caller to mark files as scanned even when
unreadable; update scanLedgerFileForSecrets to return a non-nil error on
open/read/scan failures and only set anyFileRead and increment
result.FilesScanned in the caller after scanLedgerFileForSecrets returns nil
(look for the caller code around the assignment to anyFileRead and
result.FilesScanned in doctor_ledger_secrets.go). Also apply the same change to
the other caller block noted (around the 373-389 area) so unreadable/truncated
session files are not counted as successfully scanned.
- Around line 209-217: The helper resolveLedgerPathForAudit duplicates ledger
path resolution; replace its custom logic with a call to the canonical
getLedgerPath() used by other cmd/ox commands so the doctor uses the same
local-config/default fallback behavior. Locate resolveLedgerPathForAudit and
remove the manual localCfg/Ledger.Path and ledger.DefaultPath() checks, instead
invoking getLedgerPath() (using the same arguments/signature used elsewhere in
cmd/ox) and return its result (propagating or handling any error per surrounding
callers). This ensures consistency with getLedgerPath() for ledger path
resolution.
---
Nitpick comments:
In `@cmd/ox/prepush_scan_test.go`:
- Line 221: The test currently only asserts assert.Contains(t, msg, "ox session
audit") but needs to explicitly assert the canonical remediation command; update
the test to also assert that msg includes the exact replacement "ox session
redact-history --dry-run" (for example add assert.Contains(t, msg, "ox session
redact-history --dry-run")) so the user-facing guidance is validated—ensure you
modify the same test that sets/reads msg and leave the existing audit assertion
in place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 659b290d-f1d7-4aac-86b0-74be62bc9099
📒 Files selected for processing (4)
cmd/ox/doctor_ledger_secrets.gocmd/ox/doctor_ledger_secrets_test.gocmd/ox/prepush_scan.gocmd/ox/prepush_scan_test.go
✅ Files skipped from review due to trivial changes (2)
- cmd/ox/prepush_scan.go
- cmd/ox/doctor_ledger_secrets_test.go
Co-Authored-By: SageOx <ox@sageox.ai> SageOx-Session: https://sageox.ai/repo/repo_019c5812-01e9-7b7d-b5b1-321c471c9777/sessions/2026-05-11T22-36-galexy-OxE2aB/view
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two user-visible breakages introduced by PR #597 ("security: credential redaction improvements"):
ox doctorreports an unfixable "Ledger remote URL match" warning on every run. The check was written for the pre-ox-eeqi world (PAT embedded in origin URL) and never updated when the credential helper migration landed. On a correctly-migrated bare URL, it always reported "missing credentials" and the auto-fix did nothing.redact-historylong help tell users to runox doctor --check=ledger-secrets— but--checkis not a flag.Forest view
There are two models for where the ledger's GitLab PAT lives:
ox git-credential-helperThe codebase had two doctor checks that looked at the same origin URL and disagreed about which model was correct:
Ledger embedded credentials(Check A) — speaks the new model: bare URL = good.Ledger remote URL match(Check B) — still speaks the old model: bare URL = "missing credentials."Check B was registered as
FixLevelAuto, fired on every run, and its "fix" calledMigrateLedgerCredentials(which strips the PAT — correctly, per the new model). Next run, Check B still saw a bare URL, complained again, and called the fix again. Forever.What this PR does
Removes the stale check and its scaffolding.
checkLedgerRemoteURLMatch+CheckSlugLedgerRemoteURLMatch+ registry entrycheckLedgerRemoteURLwrapper +CheckSlugLedgerRemote+ registry entryextractPATFromURL,fixLedgerStalePAT(helpers exclusive to Check B)fixRemoteparameter fromcheckLedgerGitHealth, plus its two inline call sites indoctor.goWires up two registered-but-dead checks so the doctor pipeline actually exercises them.
Ledger embedded credentialswas registered (slugledger-embedded-creds) but never invoked fromrunDoctorChecks— now runs in theLedger Git Healthcategory.Ledger remote URL vs API(ledger-url-api-match) was registered but never invoked — same fix.Extends Check A to catch a silent-success trap that the deleted Check B accidentally papered over: a bare HTTPS origin with no credential helper installed in
.git/configwould pass with the originalno embedded PATlogic, then fetch/push would fail at auth. Check A now verifies thatcredential.https://<host>.helperis configured for the host and offers a fix that installs it. (Codex adversarial review round 1.)Fixes a security regression in
checkLedgerURLAPIMatch's fix path. It was rebuilding the corrected origin URL withparsed.User = url.UserPassword("oauth2", creds.Token)— re-embedding the PAT into origin, directly violating the post-eeqi invariant. The fix is now extracted intoapplyCorrectedLedgerURL, which writes a bare URL and then callsMigrateLedgerCredentialsfor the host. (Codex adversarial review round 2.)Updates user-facing strings that promised the non-existent
--check=ledger-secretsflag:cmd/ox/prepush_scan.go:210— push-refused message now points atox session redact-history --dry-run(which exists, performs a thicker audit than the hypothetical--checkwould have, and classifies findings as pushed vs unpushed).cmd/ox/session_redact_history.go— header comment and Long help corrected.cmd/ox/doctor_ledger_secrets.go— doc comment forcheckLedgerSecretsno longer claims to implement a flag that doesn't exist.cmd/ox/prepush_scan_test.go:219— assertion updated to match the new message.Conceptual flow
graph TD A[ox doctor runs] --> B{origin URL state} B -->|embedded oauth2:TOKEN| C[Check A: FAIL — leak] B -->|bare URL + helper installed| D[Check A: PASS] B -->|bare URL + no helper| E[Check A: FAIL — auth will break] C -->|--fix| F[MigrateLedgerCredentials:<br/>strip PAT + install helper] E -->|--fix| F F --> D G[checkLedgerURLAPIMatch runs] --> H{local URL == API URL?} H -->|yes| I[PASS] H -->|no| J{--fix?} J -->|no| K[FAIL — URL mismatch] J -->|yes| L[applyCorrectedLedgerURL:<br/>set bare URL +<br/>install helper] L --> M[verify with ls-remote]Tests added
TestLedgerHasCredentialHelper_{NotConfigured,Configured}— Codex round 1 regression.TestLedgerOriginState_{BareURL,NonHTTPSReturnsNoHost}— host-resolution invariants.TestApplyCorrectedLedgerURL_LeavesOriginBare— load-bearing security regression: the URL-API-match fix must never write anoauth2:TOKEN@form to origin.TestApplyCorrectedLedgerURL_InstallsHelper— Codex round 2 regression: helper must be installed after URL correction.Test plan
make test— 14,382 pass, 0 failgo vet ./cmd/ox/cleanLedger embedded credentialspasses with precise "ox credential helper installed" message;Ledger remote URL vs APIruns and gracefully skips with "API unavailable" when offline/codex:adversarial-review); both substantive findings addressed and covered by regression testsOut of scope (filing as follow-ups)
docs/human/security/credential-redaction.mdhas 5 remaining references to--check=ledger-secrets/--check=ledger-embedded-creds. File is tagged<!-- doc-audience: human -->so per CLAUDE.md it's not for me to rewrite.Ledger branch status (reconcile failed)on the reporter's ledger — that's the pre-push gate doing its job; content needs to be redacted viaox session redact-history, not a doctor bug.🤖 Generated with Claude Code
Co-Authored-By: SageOx
Summary by CodeRabbit
ox session audit+ox session redact; clarifiedox session redactdocumentation.