Skip to content

fix(prepush): never-block credential gate — scope to sessions/, auto-redact JSONL, quarantine the rest (0.8.1)#606

Merged
rsnodgrass merged 2 commits into
mainfrom
ryan/ledger-push-false-positive
May 13, 2026
Merged

fix(prepush): never-block credential gate — scope to sessions/, auto-redact JSONL, quarantine the rest (0.8.1)#606
rsnodgrass merged 2 commits into
mainfrom
ryan/ledger-push-false-positive

Conversation

@rsnodgrass
Copy link
Copy Markdown
Contributor

@rsnodgrass rsnodgrass commented May 13, 2026

Summary

The 0.8.0 pre-push secret gate (cmd/ox/prepush_scan.go, introduced in #597) refused the whole push on any detector hit anywhere in the push range. Two compounding problems shipped together:

  1. The scanner ran against data/github/** PR/Issue caches. PR titles, bodies, and comments contain text matching credential heuristics — sample Authorization: Bearer snippets, the literal phrase "STS session key", and other bytes already public on GitHub. ox doctor reconcile failed on clean machines with "Push refused: 3 credential pattern(s) detected in 2 file(s)" pointing at JSON the user did not author and could not fix via ox session audit / ox session redact (the recovery commands the gate's error message names — neither operates on data/github/).
  2. Even after scoping, a single session with a finding would refuse every other session and unrelated commit in the same push.

What this PR changes

Layer Before (0.8.0) After (0.8.1)
Scanner scope Every file in origin/main...HEAD sessions/** only
data/github/** writer-side redactor Wired (rewrites PR/Issue bytes on every WriteGitHubPR/WriteGitHubIssue) Unwired — pass-through
On finding in JSONL session Refuse push Rewrite via canonical chokepoint, append RedactionPass to meta.json, amend holding commit, push proceeds
On finding the chokepoint can't fix Refuse push Atomically move to .sageox/cache/quarantine/<session>/<file>, carve path out of holding commit, write a debt marker, push proceeds with the rest of the commit
Doctor surface None New ledger-redaction-debt check surfaces every quarantined session + next-step recovery
Return contract Returns error when findings present Never returns an error — recovery errors are logged, push proceeds

OX_ALLOW_SECRETS=1 keeps its short-circuit semantics for explicit "publish as-is" overrides.

Pre-push gate flow

flowchart TD
    A[pushLedger] --> B[runPrePushSecretGate]
    B --> C{scan sessions/ only}
    C -- clean --> Z[return nil]
    C -- findings --> D{OX_ALLOW_SECRETS=1?}
    D -- yes --> W[warn loudly to stderr] --> Z
    D -- no --> E[autoRedactSessionFindings: per session run redactFileInPlace, append RedactionPass to meta.json, stage --sparse, amend]
    E --> F{re-scan}
    F -- clean --> G[stderr: redacted N files] --> Z
    F -- findings remain --> H[quarantineUnredactableFindings]
    H --> H1[move file to .sageox/cache/quarantine/session/]
    H1 --> H2[git reset HEAD~ -- path, or git rm --cached if no HEAD~]
    H2 --> H3[git commit --amend]
    H3 --> H4[write .sageox/cache/redaction-debt/session.json]
    H4 --> I[stderr: quarantined N files, names listed, points at ox doctor]
    I --> Z
Loading

Why quarantine, not block

The daemon's session-finalize watches <ledger>/sessions/ and <ledger>/.sageox/cache/sessions/ (see internal/daemon/agentwork/session_finalize.go). The quarantine path <ledger>/.sageox/cache/quarantine/ is outside both, so anti-entropy will not re-introduce a quarantined file. Bytes are preserved verbatim on disk for forensic inspection or manual recovery; the holding commit drops only those paths.

Why unwire the data/github/** writer-side redactor

data/github/** is a verbatim cache of bytes already published on GitHub. Replicating those bytes into the ledger has the same exposure surface as ingesting the PR/Issue at all. Rewriting them at write time:

  • Adds no real security gain (bytes are already public).
  • Is the upstream cause of the false-positive class — legacy unredacted bytes already on origin/main keep tripping the scanner on every re-touch.

The FieldRedactor plumbing is retained as a no-op pass-through so a future warn-on-detect mode can re-use the same chokepoint without re-plumbing the package boundary.

Scope contract

TestFormatPrePushFindings_RecoveryMatchesScannerScope pins the policy: any future widening of prePushScannerScopePrefixes must come with a deliberate broadening of the recovery message surface. TestInPrePushScannerScope table-tests the scope predicate. TestScanPrePushForSecrets_OnlyScansSessionsScope exercises end-to-end that data/github/**, kb/**, and team-context/** paths are not scanned even when they carry planted canaries.

What's deferred

  • ox-tduv — daemon anti-entropy interactions for sessions quarantined long-term + an optional ox session accept-unredacted <name> command for explicit "I reviewed it, publish as-is" flow. Today the user manually moves the quarantined bytes back to sessions/<name>/ and commits, or uses OX_ALLOW_SECRETS=1 per-push.

Test plan

  • make lint — 0 issues
  • go vet ./... — clean
  • go test ./cmd/ox ./internal/ledger -short -count=1 — all green
  • make verify-version — 0.8.1 across version.go, marketplace.json, plugin.json, CHANGELOG
  • New tests cover: JSONL auto-redact happy path + meta.json provenance + bytes-scrubbed-on-disk; non-JSONL quarantine path + bytes-preserved-in-quarantine + commit-carve-out + sibling-paths-survive; doctor marker parse / aggregate / malformed handling; OX_ALLOW_SECRETS=1 still short-circuits recovery
  • Existing scope-contract suite still green
  • Reviewer to validate on a real ledger with legacy data/github/ content: ox doctor reconcile should now pass; ox doctor should surface any quarantined sessions under "Ledger redaction debt"

Files

Path Change
cmd/ox/prepush_scan.go Scope filter to sessions/; runPrePushSecretGate now never blocks — auto-redact + quarantine flow
cmd/ox/prepush_autoredact.go New — autoRedactSessionFindings, quarantineUnredactableFindings, redactionDebtRecord
cmd/ox/prepush_scan_test.go Scope-contract tests; replaced blocking-behavior tests with never-block assertions
cmd/ox/prepush_autoredact_test.go New — JSONL auto-redact, non-JSONL quarantine, override semantics
cmd/ox/doctor_redaction_debt.go New — ledger-redaction-debt check
cmd/ox/doctor_redaction_debt_test.go New — marker parse / aggregate / malformed handling
cmd/ox/doctor_check_registry.go + doctor_types.go Register new check
cmd/ox/redactor_wiring.go Unwire ledger.SetFieldRedactor
internal/ledger/github_data.go Update doc comments; redactPR / redactIssue now no-op pass-through
internal/version/version.go, .claude-plugin/marketplace.json, claude-plugin/.claude-plugin/plugin.json, CHANGELOG.md Version bump 0.8.0 → 0.8.1 + release notes

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 13, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Pre-push credential gate redesigned to scope scanning to sessions/ paths, auto-redact JSONL findings in-place, quarantine non-redactable files, surface persistent quarantine state via new doctor check, and always allow pushes without blocking. Version bumped to 0.8.1; old field-redactor hook removed. Comprehensive test coverage added.

Changes

Pre-push Secret Gate & Redaction Debt

Layer / File(s) Summary
Pre-push scanner scope definition and validation
cmd/ox/prepush_scan.go, cmd/ox/prepush_scan_test.go
prePushScannerScopePrefixes restricts scanning to sessions/ paths; inPrePushScannerScope filters candidates early; scope contract tests validate filtering and recovery message alignment.
Auto-redaction and quarantine pipeline
cmd/ox/prepush_autoredact.go, cmd/ox/prepush_autoredact_test.go
autoRedactSessionFindings rewrites JSONL in-place and records audit trail to meta.json; quarantineUnredactableFindings atomically moves non-JSONL files to .sageox/cache/quarantine/ and writes debt markers; tests cover happy path (JSONL), quarantine (non-JSONL), and override scenarios.
Redaction debt markers and doctor check
cmd/ox/doctor_types.go, cmd/ox/doctor_check_registry.go, cmd/ox/doctor_redaction_debt.go, cmd/ox/doctor_redaction_debt_test.go
checkLedgerRedactionDebt scans debt marker directory, parses JSON records, and emits WarningCheck with quarantined-session guidance; readDebtSummaries aggregates findings and detector names; unit tests validate empty/valid/malformed marker handling.
Pre-push gate orchestration
cmd/ox/prepush_scan.go
runPrePushSecretGate orchestrates auto-redaction, re-scan, and quarantine phases; emitQuarantineWarning centralizes stderr messaging; gate always returns nil (never blocks push); OX_ALLOW_SECRETS override skips recovery.
Credential redaction removal and version updates
cmd/ox/redactor_wiring.go, cmd/ox/release_notes.md, internal/ledger/github_data.go, .claude-plugin/marketplace.json, claude-plugin/.claude-plugin/plugin.json, internal/version/version.go
Runtime field-redactor hook removed from init wiring; ledger FieldRedactor documented as no-op pass-through; version bumped to 0.8.1 across manifests and version package; release notes document scoped inspection, auto-redaction, quarantine, and doctor visibility.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 The pre-push gate now guides with grace,
Auto-redacting in its rightful place,
No more blocks, just quarantine care,
With doctor checks to surface where,
Debt markers hide, awaiting dawn.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main changes: fixing the pre-push gate to never block pushes, scoping it to sessions/, auto-redacting JSONL, and quarantining unredactable findings for the 0.8.1 release.
Docstring Coverage ✅ Passed Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ryan/ledger-push-false-positive

Comment @coderabbitai help to get the list of available commands and usage tips.

@rsnodgrass rsnodgrass marked this pull request as ready for review May 13, 2026 00:54
@rsnodgrass rsnodgrass merged commit edba6d3 into main May 13, 2026
2 of 3 checks passed
@rsnodgrass rsnodgrass deleted the ryan/ledger-push-false-positive branch May 13, 2026 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant