fix(diagnose): repair stray pseudonymized originals instead of refusing to emit (#186) - #187
Conversation
…ng to emit (#186) The leak self-check could fire unresolvably: a per-field routing gap put a legend original into the rendered bytes, assert_no_leak reported only an opaque sensitive[N] index, no flag recovered, and the --legend decode file was only written on success — so diagnose was unusable on exactly the runs that needed a dump. - sanitize: _iter_standalone shared by detection and the new replace_standalone (identical word-boundary semantics by construction); assert_no_leak accepts (value, label) extras reported as sensitive[<label>]; Pseudonymizer.entries() exposes (ns, original, alias). - diagnostics: _guard now repairs sensitive[*] hits by substituting each original's alias (longest-first) and re-verifying, bounded at 3 passes; hard rules (email/secret/home-path/url-creds/username) still raise immediately and are never auto-repaired. Repairs are disclosed in the dump itself (markdown 'Backstop repairs' section / JSON backstop_repairs key), and the disclosure is re-checked so it sits inside verified bytes. - cli: residual failures name sensitive[<ns>:<alias>] with a decode hint; --legend is written even on refusal (local-only, 0600); repaired dumps emit rc 0 with a stderr warning asking the operator to report the routing gap. Closes #186
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Walkthrough
ChangesDiagnose backstop repair flow
Command reference documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant cmd_diagnose
participant diagnostics._guard
participant sanitize.assert_no_leak
cmd_diagnose->>diagnostics._guard: render sanitized diagnostic output
diagnostics._guard->>sanitize.assert_no_leak: check output and labeled originals
sanitize.assert_no_leak-->>diagnostics._guard: leak rules
diagnostics._guard->>diagnostics._guard: substitute standalone originals with aliases
diagnostics._guard-->>cmd_diagnose: output and repair counts
cmd_diagnose->>cmd_diagnose: write legend and apply emission gate
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@README.md`:
- Line 82: Update the README command-reference row for probe-adapter
(collect-adapter-data) to document the public --model and --timeout options
alongside the existing flags, indicating they configure live captures and their
timeout.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dcdd6d2c-f0f6-458b-ba60-34b36e4a23dc
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mddocs/FEATURES.mdsrc/bmad_loop/cli.pysrc/bmad_loop/diagnostics.pysrc/bmad_loop/sanitize.pytests/test_cli.pytests/test_diagnostics.pytests/test_sanitize.py
Closes #186.
Problem
bmad-loop diagnosecould fail withFAIL: refusing to emit — leak self-check fired: sensitive[2]and leave the operator with nothing: no dump, no recovery knob, no way to tell what kind of value tripped the check (the index-only rule name is deliberate — echoing the value would itself leak it), and no--legendfile to decode it (the legend write sat below the failure return). This blocked the #161 reporter from attaching any dump — the exact situation diagnose exists for.The leak class is a per-field routing gap:
_scrub_entry's catch-all routes unlisted journal fields throughscrub_json, which passes identifier-shaped strings verbatim — so a story key/branch carried in such a field survives into the render while its aliased twin put the original into the legend, and the backstop fires.Fix
The backstop already knows both the leaked original and its safe alias, so it can finish the routing job it detected instead of refusing:
_guardsubstitutes each stray original's alias (longest-first, identical word-boundary semantics as detection — both walk one shared_iter_standaloneloop) and re-verifies, bounded at 3 passes. Hard rules (email / secret / home-path / url-creds / username) still raise immediately and are never auto-repaired.### Backstop repairssection, JSONbackstop_repairskey — labels and counts only) and the disclosure is re-checked so it sits inside the verified bytes; the CLI prints a stderr warning asking the operator to include it in their bug report, so routing gaps keep surfacing as fixable bugs instead of being silently normalized.sensitive[<ns>:<alias>]— printable by construction — plus a decode hint, and--legendis now written even on refusal (local-only, 0600) so the operator can map the alias back to the real value privately.No escape-hatch flag: the issue framed it as a fallback, and repair makes it moot — the hard rules stay absolutely fail-closed.
Verification
LeakDetected/rc 1, and the legend-on-failure test hitFileNotFoundError.refusing to emitCLI branch is now covered; a zero-repair test pins that the canonical seeded run needs no repairs (CI still catches new routing gaps); an adversarial substitution-cycle test pins that the pass bound fails closed.trunk checkclean.backstop_repairsdisclosed, stderr warning namesstory:<alias>, legend decodes it; clean-run rerun emits with no warning.Summary by CodeRabbit
Bug Fixes
bmad-loop diagnosecan now repair certain stray pseudonymized identifiers and emit sanitized reports with repair details.--legendfile is written even when report generation is refused, with clearer sensitive-field names in error messages.Documentation