Claude/understand project goals m8s0g3

[digitaldrreamer]

Summary by CodeRabbit

• New Features
  • Added a full broomsticks CLI with scan, clean, source discovery, setup/installation, and a local proxy that redacts secrets in requests and responses.
  • Expanded transcript support across multiple local tools, including file and SQLite-backed sources.
  • Added allowlist-based suppression, redaction placeholders, and backup manifests; supports human and JSON reports plus safer “apply” workflows.
• Tests
  • Added comprehensive unit and end-to-end CLI tests, including proxy validation and no-network scanning safeguards.

[coderabbitai]

Warning

Review limit reached

@digitaldrreamer, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 50 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b69e7841-3b82-4557-9ddd-9da15af3203c

📥 Commits

Reviewing files that changed from the base of the PR and between b4a2c88 and d8daf84.

📒 Files selected for processing (2)

• package.json
• test/detector.test.mjs

📝 Walkthrough

Walkthrough

Replaces the placeholder npm CLI stub with a full broomsticks tool: secret detection, redaction, backups, allowlisting, transcript discovery, installer assets, a redacting proxy, and tests covering the new workflows.

Changes

Broomsticks CLI and supporting modules

Layer / File(s)	Summary
Detection rules and engine src/rules.mjs, src/detector.mjs, test/detector.test.mjs	Defines entropy-aware secret rules, implements scanText overlap resolution and extra-rule conversion, and adds detector coverage tests.
Redaction placeholder generation src/redactor.mjs, test/redactor.test.mjs	Replaces detected secret spans with stable hashed placeholders and tests placeholder formatting, multi-secret redaction, JSON safety, and idempotency.
Allowlist for false-positive suppression src/allowlist.mjs, test/allowlist.test.mjs	Provides built-in demo secrets, default allowlist content, parsing, loading, and suppression checks with matching tests.
Backup session and manifest writing src/backup.mjs, test/backup.test.mjs	Implements BackupSession for per-run file backups, sidecar copying, manifest writing, and destination-path normalization.
Transcript source adapters src/sources/claude-code.mjs, src/sources/codex.mjs, src/sources/cursor.mjs, test/cursor.test.mjs	Discovers transcript targets for Claude Code, Codex, and Cursor storage and verifies Cursor read/redact/write behavior.
Report formatting src/report.mjs	Renders human-readable and JSON scan or clean reports with masked secrets and hashed JSON fields.
CLI entrypoint wiring bin/broom.mjs, package.json, test/cli.test.mjs	Implements argument parsing, command dispatch, target gathering, scan and clean workflow, sources listing, help text, packaging updates, and end-to-end CLI tests.
First-run installer for skill and Stop hook src/install.mjs, skills/broom-sweep/SKILL.md	Writes the broom-sweep skill, Stop-hook script, and Claude settings registration, and defines the installed skill workflow.
Local redacting HTTP proxy and daemon management src/proxy.mjs, test/no-network.test.mjs	Implements request and response redaction for Anthropic and OpenAI-compatible APIs, request forwarding, proxy env setup, daemon install and uninstall, and network-surface checks.

Estimated code review effort: 4 (Complex) | ~75 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title is too vague and looks like a branch label, so it does not clearly describe the change set.	Rename it to a concise description of the main change, such as "Add broomsticks CLI, proxy, and scanner modules".

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/understand-project-goals-m8s0g3

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gemini-code-assist]

Code Review

This pull request implements the core functionality of broomsticks, transitioning it from a placeholder to a fully functional CLI tool and local redacting proxy for AI coding-assistant transcripts. It introduces robust secret detection rules, target discovery adapters (for Claude Code, Codex, and Cursor), backup sessions, and an interactive onboarding flow. The review feedback focuses on critical robustness and compatibility enhancements, including correcting HTTP header handling (specifically removing transfer-encoding when forwarding buffered payloads), optimizing SQLite database access to prevent locking and performance issues, ensuring Windows compatibility for the hook script, adding defensive checks for JSON parsing and file operations, and validating CLI port arguments.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

If the upstream response was chunked, upRes.headers will contain transfer-encoding: chunked. Since we are sending the response as a single buffer with a fixed content-length, we must delete transfer-encoding from the response headers to prevent client-side protocol errors.

Suggested change

	const outHeaders = { ...upRes.headers, 'content-length': String(responseBody.length) }
	const outHeaders = { ...upRes.headers }
	delete outHeaders['transfer-encoding']
	outHeaders['content-length'] = String(responseBody.length)

[gemini-code-assist]

Calling target.read() inside the nested loop for every single finding is extremely inefficient. For SQLite targets (Cursor), this means re-opening and querying the database repeatedly, which is very slow and can cause locking issues. Read the target's text once per target at the start of the outer loop.

  for (const { target, findings } of dirty) {
    const shortLabel = target.label.replace(process.env.HOME ?? '', '~')
    console.log(`  ${bold(target.source)}  ${shortLabel}`)

    let text = null
    try {
      text = target.read()
    } catch { /* non-blocking */ }

    // Group by severity for ordered display
    const bySev = Object.fromEntries(SEVERITY_ORDER.map(s => [s, []]))
    for (const f of findings) bySev[f.severity].push(f)

    for (const sev of SEVERITY_ORDER) {
      for (const f of bySev[sev]) {
        const tag    = color(sev, `[${sev.padEnd(8)}]`)
        const rule   = f.ruleId.padEnd(26)
        const masked = maskSecret(f.secret)
        let lineInfo = ''
        if (text !== null) {
          lineInfo = dim(`  line ${lineNumber(text, f.start)}`)
        }
        console.log(`    ${tag} ${dim(rule)} ${masked}${lineInfo}`)
      }
    }

[gemini-code-assist]

On Windows, running a .mjs file directly as a command hook will fail or open in a text editor because Windows does not natively support shebangs or execute .mjs files directly. Prepend node to the command on Windows.

Suggested change

	if (!alreadyWired) {
	settings.hooks.Stop.push({ hooks: [{ type: 'command', command: hp }] })
	if (!alreadyWired) {
	const commandStr = process.platform === 'win32' ? `node "${hp}"` : hp
	settings.hooks.Stop.push({ hooks: [{ type: 'command', command: commandStr }] })

[gemini-code-assist]

When discovering targets, the SQLite database is only read from. Opening it in read-write mode by default can cause locking issues (especially if Cursor is actively running and has the database open) or fail if the file is read-only. Open the database in read-only mode (readOnly: true) instead.

Suggested change

	db = new DatabaseSync(dbPath, { open: true })
	db = new DatabaseSync(dbPath, { open: true, readOnly: true })

[gemini-code-assist]

If JSON.parse(raw) returns null or another non-object value, accessing ev.choices will throw a TypeError. Add a defensive check to ensure ev is a non-null object.

    let ev; try { ev = JSON.parse(raw) } catch { continue }
    if (!ev || typeof ev !== 'object') continue
    for (const c of ev.choices ?? []) {

[gemini-code-assist]

Reading or writing to shell profile files can throw errors (e.g., permission denied). Wrap the file operations in a try...catch block to prevent the entire CLI from crashing.

    try {
      const contents = readFileSync(file, 'utf8')
      if (contents.includes(BROOM_MARKER)) continue   // already installed
      appendFileSync(file, ENV_BLOCK(port), 'utf8')
      updated.push(file)
    } catch {
      // Ignore or log error gracefully
    }

[gemini-code-assist]

On Linux, uninstallDaemon tries to stop, disable, and unlink the systemd service without checking if it exists first, and always returns true. This causes the CLI to print "daemon removed" even if there was no daemon installed. Check if the service file exists first.

Suggested change

	if (process.platform === 'linux') {
	try { execFileSync('systemctl', ['--user', 'stop', 'broom-proxy'], { stdio: 'ignore' }) } catch {}
	try { execFileSync('systemctl', ['--user', 'disable', 'broom-proxy'], { stdio: 'ignore' }) } catch {}
	const servicePath = join(home, '.config', 'systemd', 'user', 'broom-proxy.service')
	try { unlinkSync(servicePath) } catch {}
	try { execFileSync('systemctl', ['--user', 'daemon-reload'], { stdio: 'ignore' }) } catch {}
	return true
	}
	if (process.platform === 'linux') {
	const servicePath = join(home, '.config', 'systemd', 'user', 'broom-proxy.service')
	if (!existsSync(servicePath)) return false
	try { execFileSync('systemctl', ['--user', 'stop', 'broom-proxy'], { stdio: 'ignore' }) } catch {}
	try { execFileSync('systemctl', ['--user', 'disable', 'broom-proxy'], { stdio: 'ignore' }) } catch {}
	try { unlinkSync(servicePath) } catch {}
	try { execFileSync('systemctl', ['--user', 'daemon-reload'], { stdio: 'ignore' }) } catch {}
	return true
	}

[gemini-code-assist]

If settings.hooks is not an object or settings.hooks.Stop is not an array, accessing or modifying them will throw a TypeError. Add defensive checks to ensure they are correctly initialized.

Suggested change

	settings.hooks ??= {}
	settings.hooks.Stop ??= []
	if (typeof settings.hooks !== 'object' || settings.hooks === null) {
	settings.hooks = {}
	}
	if (!Array.isArray(settings.hooks.Stop)) {
	settings.hooks.Stop = []
	}

[gemini-code-assist]

If the user passes an invalid port (e.g., --port abc), parseInt returns NaN, which will cause the proxy server to throw an unhandled error when listening. Validate that the port is a valid number between 1 and 65535.

  const portStr = option('--port') ?? '7777'
  const port    = parseInt(portStr, 10)
  if (isNaN(port) || port < 1 || port > 65535) {
    console.error(`broom: invalid port '${portStr}'. Port must be a number between 1 and 65535.`)
    process.exit(1)
  }

[battletest]

🛡️ BattleTest Security Review

1 advisory issue found — no blockers.

View full report

[digitaldrreamer]

/gemini review

[coderabbitai]

Actionable comments posted: 16

🧹 Nitpick comments (6)

src/install.mjs (2)

146-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Skill and hook files are unconditionally overwritten on every runInstall() call.

Unlike the allowlist (which checks existsSync first), the SKILL.md and hook script are rewritten every run regardless of prior state, silently discarding any local edits a user made to these files. If this is intentional (always keep managed files current), consider noting it in the done log so users aren't surprised (e.g. installed vs updated vs unchanged).

🤖 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 `@src/install.mjs` around lines 146 - 157, The SKILL.md and hook script
handling in runInstall() always rewrites existing files, unlike the allowlist
logic that preserves preexisting files. Update the install flow around
skillPath() and hookPath() so it checks whether each file already exists before
writing, or otherwise preserves local edits; if overwriting is intended, make
the done log in runInstall() clearly distinguish installed, updated, or
unchanged states so users know what happened.

---

44-56: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Unpinned npx broomsticks fallback — supply-chain rug-pull risk.

npx broomsticks scan ... (Line 50) resolves to whatever version is currently published at execution time. The same pattern appears in skills/broom-sweep/SKILL.md's allowed-tools: ... Bash(npx broomsticks *). If the package is ever compromised, this hook (which runs automatically after every Claude turn) would execute the malicious version with no version pin to fall back on.

🤖 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 `@src/install.mjs` around lines 44 - 56, The fallback in scan() uses an
unpinned npx broomsticks invocation, which can execute an arbitrary published
version at runtime. Update the install hook’s HOOK_SCRIPT to avoid resolving
broomsticks from the registry without a fixed version, and instead use a pinned
executable path or a versioned package reference with a locked install. Make the
same safety change wherever the allowed-tools entry in SKILL.md references
Bash(npx broomsticks *), so both the hook and tool allowlist point to a
version-pinned broomsticks command.

Source: Linters/SAST tools

skills/broom-sweep/SKILL.md (1)

4-4: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Unpinned npx broomsticks * in allowed-tools — supply-chain rug-pull risk.

Granting Claude blanket permission to run any npx broomsticks command with no version pin means a future compromise of the published package would be auto-executed under this allow rule. Same concern applies to the HOOK_SCRIPT fallback in src/install.mjs (Line 50).

🤖 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 `@skills/broom-sweep/SKILL.md` at line 4, The `allowed-tools` entry currently
permits an unpinned `npx broomsticks *`, which should be tightened to a
version-pinned invocation to avoid executing future package changes. Update the
`allowed-tools` pattern in `SKILL.md` to reference a specific broomsticks
version or an equivalent locked command, and apply the same pinning strategy to
the `HOOK_SCRIPT` fallback in `src/install.mjs`. Use the existing
`allowed-tools` and `HOOK_SCRIPT` symbols to locate and align both places so
they cannot resolve to a floating package version.

Source: Linters/SAST tools

src/sources/claude-code.mjs (1)

39-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate directory-walking logic across source adapters.

walkJsonl here is duplicated almost verbatim in src/sources/codex.mjs (lines 25-37), and src/sources/cursor.mjs's walkVscdb (lines 48-60) is structurally identical modulo the file-match predicate. Consider extracting a shared walkFiles(dir, predicate) generator into a small shared util module used by all three adapters.

♻️ Sketch of shared helper

// src/sources/_walk.mjs
import { readdirSync } from 'node:fs'
import { join } from 'node:path'

export function* walkFiles(dir, predicate) {
  let entries
  try {
    entries = readdirSync(dir, { withFileTypes: true })
  } catch {
    return
  }
  for (const entry of entries) {
    const full = join(dir, entry.name)
    if (entry.isDirectory()) yield* walkFiles(full, predicate)
    else if (entry.isFile() && predicate(entry.name)) yield full
  }
}

🤖 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 `@src/sources/claude-code.mjs` around lines 39 - 55, The directory traversal
logic in walkJsonl is duplicated across multiple source adapters and should be
consolidated. Extract the shared recursion and readdirSync handling into a
reusable walkFiles(dir, predicate) generator in a small shared utility module,
then update walkJsonl, codex.mjs, and cursor.mjs's walkVscdb to call it with
their file-matching predicate. Keep the existing missing/unreadable directory
behavior and preserve each adapter’s current filtering rules via the predicate.

src/sources/cursor.mjs (1)

69-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

readOnly option not actually applied despite comment.

The docstring says "Open a SQLite database read-only," but new DatabaseSync(dbPath, { open: true }) doesn't pass readOnly: true. Functionally this scan-only connection only issues SELECTs so no data is modified today, but the comment is misleading and a future edit to this function could accidentally write through this handle.

♻️ Proposed fix

-    db = new DatabaseSync(dbPath, { open: true })
+    db = new DatabaseSync(dbPath, { open: true, readOnly: true })

Please confirm the installed Node version supports the readOnly constructor option (it was added after node:sqlite's initial 22.5.0 release) — see verification note below.

🤖 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 `@src/sources/cursor.mjs` around lines 69 - 112, The read-only intent in
readAiRows is not enforced because DatabaseSync is opened with open: true only,
which makes the docstring misleading. Update the DatabaseSync constructor call
in readAiRows to use the readOnly option supported by the installed Node sqlite
version, and keep the rest of the SELECT-only logic unchanged. If readOnly is
unavailable in the target runtime, adjust the comment/documentation to match the
actual behavior instead of claiming read-only access.

bin/broom.mjs (1)

244-252: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

One failing source adapter kills discovery for all sources.

gatherTargets spreads claudeCodeTargets(), codexTargets(), and cursorTargets() with no isolation; if any one throws (e.g., cursor.mjs's static node:sqlite import failing on an unsupported Node build — see the corresponding comment in src/sources/cursor.mjs), the whole scan/clean/sources command fails, even though the other two sources are perfectly usable.

♻️ Proposed fix — isolate each source

 function gatherTargets(filter) {
-  const all = [
-    ...claudeCodeTargets(),
-    ...codexTargets(),
-    ...cursorTargets(),
-  ]
+  const adapters = [claudeCodeTargets, codexTargets, cursorTargets]
+  const all = []
+  for (const discover of adapters) {
+    try {
+      all.push(...discover())
+    } catch (e) {
+      console.error(`broom: source adapter failed: ${e.message}`)
+    }
+  }
   if (!filter.length) return all
   return all.filter(t => filter.includes(t.source))
 }

🤖 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 `@bin/broom.mjs` around lines 244 - 252, gatherTargets currently calls
claudeCodeTargets(), codexTargets(), and cursorTargets() without isolation, so a
thrown error from one source blocks discovery for all. Update gatherTargets to
invoke each source adapter independently with per-source error handling,
skipping only the failing source and continuing to collect targets from the
others. Keep the fix centered in gatherTargets and the source helper calls so
scan, clean, and sources remain usable when one adapter fails.

🤖 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 `@bin/broom.mjs`:
- Around line 192-222: Document or guard the non-atomic read-then-write flow in
the main clean/apply loop over targets: the current `target.read()` followed
later by `target.write(redacted)` can overwrite concurrent changes from external
writers. Update the `broom.mjs` apply path to either note this as a known
limitation or, preferably, verify the target has not changed since `read()` (for
example via version/mtime/state check on the `target` abstraction) and abort the
write if it has. Reference the `scanResults` loop and the
`target.read`/`target.write` calls so the fix stays localized to the apply path.
- Around line 62-236: The foreground proxy flow in broom.mjs is falling through
into the scan/clean path, which causes the server started by the `command ===
'proxy'` branch to be terminated by later `process.exit()` calls. Fix this by
making the non-proxy logic mutually exclusive: wrap the existing “Shared
options” through the final report/exit block in an `else` attached to the
top-level `if (command === 'proxy')` (or otherwise guard the remaining code so
it only runs for scan/clean/sources). Keep the terminal behavior inside
`startProxy` and the existing proxy install/uninstall handling unchanged.

In `@src/allowlist.mjs`:
- Around line 8-9: Regex allowlist matching in isAllowlisted is currently using
a substring test instead of the documented full-match behavior. Update the
allowlist regex handling so regex entries only suppress a finding when the
entire secret matches, and keep the exact-string path unchanged. Use the
isAllowlisted logic in src/allowlist.mjs as the main fix point, and make sure
any matching behavior referenced by src/proxy.mjs still aligns with the
documented “exactly or fully” semantics.

In `@src/backup.mjs`:
- Around line 52-61: The sibling copy loop in backup logic is swallowing every
error for the `-wal` and `-shm` files, so only missing-file cases should be
ignored. Update the try/catch around `copyFileSync` in the `src/backup.mjs`
backup flow to check the thrown error’s code and suppress only `ENOENT`, while
rethrowing or surfacing all other errors such as permission or I/O failures.
Keep the change localized to the sibling copy block that handles `abs`/`dest`
and the `suffix` loop.
- Around line 40-64: The backup flow in backup(file) creates copied secret files
and parent directories with default umask-based permissions, which can leave
backups readable by other users. Update the mkdirSync/copyFileSync handling in
backup(file) so the backup directory and all copied files are created with
restrictive permissions (for example, owner-only access) regardless of the
process umask. Keep the fix localized to the backup creation path and the SQLite
sibling copy loop so all entries in _entries remain private.

In `@src/detector.mjs`:
- Around line 80-101: The extrasToRules path currently turns user-provided
--extra entries into RegExp objects that scan the full transcript in scanText,
so add a safety guard before creating each pattern. Update extrasToRules to
reject or fall back for unsafe regex inputs by applying a safe-regex style
validation to reMatch/entry patterns, and keep the existing escaping behavior
for plain strings. If you choose isolation instead, ensure scanText uses a
timeout/worker boundary for the extra rules so pathological patterns cannot
block the main process.

In `@src/install.mjs`:
- Around line 160-178: The settings update in install.mjs should not overwrite a
corrupt existing Claude settings file with a fresh object, because that can
erase unrelated user configuration. In the settingsPath() flow, detect
JSON.parse failures before mutating settings or calling writeFileSync, and skip
auto-writing that file when parsing fails; instead preserve the original file
and surface a clear warning from the install path. Keep the hook registration
logic around settings.hooks.Stop and alreadyWired unchanged for valid JSON, but
make the corrupt-file case non-destructive.
- Around line 79-81: `isInstalled()` currently treats the tool as installed when
the skill and hook files exist, but it does not confirm the Stop hook is
registered in `settings.json`. Update `isInstalled()` in `src/install.mjs` to
also verify the hook entry is present by checking the relevant settings data
alongside `skillPath()` and `hookPath()`, so `bin/broom.mjs` will correctly
re-run `runInstall()` when the hook wiring is missing.

In `@src/proxy.mjs`:
- Around line 75-81: The unbounded buffering in collect() can grow memory
indefinitely for large or never-ending streams. Update collect() and the related
proxy read paths around the upstream response/SSE handling to enforce a maximum
byte limit, stop reading once the cap is reached, and reject the request/stream
with a clear error. Apply the same guardrail consistently in the other affected
proxy I/O handlers so all buffered request and response accumulation stays
bounded.
- Around line 173-177: Strip hop-by-hop headers before rewriting request or
response bodies in forwardRequest and the related response handling path around
the proxy response rewrite. The current logic can leave transfer-encoding or
other hop-by-hop headers in place while also setting a new content-length, which
can produce invalid HTTP. Remove conflicting headers such as transfer-encoding
before sending the rewritten body, and apply the same cleanup in the downstream
response path that mutates body bytes so the forwarded headers always match the
actual payload.
- Around line 179-184: The upstream tlsReq call in proxy handler lacks a
timeout, so a stalled upstream connection can hang the local request
indefinitely. Add a request timeout on the upReq created in the proxy flow (near
the tlsReq invocation in the main request handling path), and make it
abort/reject with a clear error when the timeout is reached. Ensure the timeout
is cleared or otherwise handled on normal completion and error so buffered state
in the proxy path does not linger.
- Around line 294-336: The daemon service templates in launchdPlist and
systemdService are inserting raw paths directly into launch strings, so values
like nodeBin, broomBin, and logFile can break when they contain spaces or
special characters. Update launchdPlist to XML-escape the interpolated path
values, and update systemdService so ExecStart quotes each argument
independently rather than concatenating unescaped paths. Keep the fixes
localized to launchdPlist and systemdService so the generated plist and unit
remain valid for arbitrary install locations.

In `@src/report.mjs`:
- Line 94: The label normalization in report.mjs should not call
target.label.replace with an empty HOME fallback, since that prepends "~" when
HOME is unset. Update the shortLabel logic to only perform the home-directory
replacement when process.env.HOME is present and non-empty, and otherwise leave
target.label unchanged; use the existing shortLabel assignment as the place to
guard this behavior.
- Around line 93-118: Line numbers are being recomputed from post-redaction
content and the file is re-read once per finding in the report loop, which can
make offsets incorrect and is inefficient. Update the reporting flow around
printReport and the ScanResult/target.read() path so the original pre-redaction
text captured during scanning is stored or cached once per target and reused for
lineNumber(text, f.start). Ensure bin/broom.mjs passes that raw text through
before any target.write(redacted) happens, and avoid re-reading the file inside
the per-finding loop.

In `@src/sources/cursor.mjs`:
- Around line 96-112: The Cursor adapter is silently converting BLOB values to
UTF-8 strings, which can corrupt non-UTF8 payloads and change their stored type
when written back. Update `readRows` in `cursor.mjs` to only treat `row.value`
as text when it decodes losslessly, and skip or flag rows that fail a round-trip
check instead of normalizing them. Then make sure `write()` only persists
validated string values for the AI-related keys handled by
`cursorDiskKV`/`ItemTable`, so raw binary data is never overwritten with mangled
text.
- Line 19: The cursor adapter eagerly imports node:sqlite in the top-level of
CursorSource, which can crash startup on supported Node versions that still
require the experimental flag. Fix this by either raising the package
engines.node floor to a Node release where node:sqlite is unflagged, or by
moving the sqlite load inside a lazy dynamic import in the cursor adapter so the
CLI only loads it when CursorSource is actually used and fails closed otherwise.

---

Nitpick comments:
In `@bin/broom.mjs`:
- Around line 244-252: gatherTargets currently calls claudeCodeTargets(),
codexTargets(), and cursorTargets() without isolation, so a thrown error from
one source blocks discovery for all. Update gatherTargets to invoke each source
adapter independently with per-source error handling, skipping only the failing
source and continuing to collect targets from the others. Keep the fix centered
in gatherTargets and the source helper calls so scan, clean, and sources remain
usable when one adapter fails.

In `@skills/broom-sweep/SKILL.md`:
- Line 4: The `allowed-tools` entry currently permits an unpinned `npx
broomsticks *`, which should be tightened to a version-pinned invocation to
avoid executing future package changes. Update the `allowed-tools` pattern in
`SKILL.md` to reference a specific broomsticks version or an equivalent locked
command, and apply the same pinning strategy to the `HOOK_SCRIPT` fallback in
`src/install.mjs`. Use the existing `allowed-tools` and `HOOK_SCRIPT` symbols to
locate and align both places so they cannot resolve to a floating package
version.

In `@src/install.mjs`:
- Around line 146-157: The SKILL.md and hook script handling in runInstall()
always rewrites existing files, unlike the allowlist logic that preserves
preexisting files. Update the install flow around skillPath() and hookPath() so
it checks whether each file already exists before writing, or otherwise
preserves local edits; if overwriting is intended, make the done log in
runInstall() clearly distinguish installed, updated, or unchanged states so
users know what happened.
- Around line 44-56: The fallback in scan() uses an unpinned npx broomsticks
invocation, which can execute an arbitrary published version at runtime. Update
the install hook’s HOOK_SCRIPT to avoid resolving broomsticks from the registry
without a fixed version, and instead use a pinned executable path or a versioned
package reference with a locked install. Make the same safety change wherever
the allowed-tools entry in SKILL.md references Bash(npx broomsticks *), so both
the hook and tool allowlist point to a version-pinned broomsticks command.

In `@src/sources/claude-code.mjs`:
- Around line 39-55: The directory traversal logic in walkJsonl is duplicated
across multiple source adapters and should be consolidated. Extract the shared
recursion and readdirSync handling into a reusable walkFiles(dir, predicate)
generator in a small shared utility module, then update walkJsonl, codex.mjs,
and cursor.mjs's walkVscdb to call it with their file-matching predicate. Keep
the existing missing/unreadable directory behavior and preserve each adapter’s
current filtering rules via the predicate.

In `@src/sources/cursor.mjs`:
- Around line 69-112: The read-only intent in readAiRows is not enforced because
DatabaseSync is opened with open: true only, which makes the docstring
misleading. Update the DatabaseSync constructor call in readAiRows to use the
readOnly option supported by the installed Node sqlite version, and keep the
rest of the SELECT-only logic unchanged. If readOnly is unavailable in the
target runtime, adjust the comment/documentation to match the actual behavior
instead of claiming read-only access.

🪄 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

Run ID: f004e7c4-5401-44ce-bd49-ce95d87cc697

📥 Commits

Reviewing files that changed from the base of the PR and between 3f72e43 and 16a58a0.

📒 Files selected for processing (14)

• bin/broom.mjs
• package.json
• skills/broom-sweep/SKILL.md
• src/allowlist.mjs
• src/backup.mjs
• src/detector.mjs
• src/install.mjs
• src/proxy.mjs
• src/redactor.mjs
• src/report.mjs
• src/rules.mjs
• src/sources/claude-code.mjs
• src/sources/codex.mjs
• src/sources/cursor.mjs

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Read-then-later-write is not atomic — external writers can race with clean --apply.

For each target, target.read() (line 195) captures a snapshot of the file/row; later, target.write(redacted) (line 214) overwrites based on that stale snapshot. If the underlying tool (Claude Code, Codex, Cursor) writes to the same transcript between the read and the write — plausible for Cursor's live SQLite state or an actively-running Claude Code session — the intervening update is silently lost. This is most acute for Cursor, where read()/write() reopen the DB independently (see src/sources/cursor.mjs), so a concurrent app write between the scan-time read and the later write is clobbered.

Not a blocking issue for the common "assistant process idle when you run broom" case, but worth documenting as a known limitation, or worth checking a version/mtime before writing and aborting if the file changed underneath.

🤖 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 `@bin/broom.mjs` around lines 192 - 222, Document or guard the non-atomic
read-then-write flow in the main clean/apply loop over targets: the current
`target.read()` followed later by `target.write(redacted)` can overwrite
concurrent changes from external writers. Update the `broom.mjs` apply path to
either note this as a known limitation or, preferably, verify the target has not
changed since `read()` (for example via version/mtime/state check on the
`target` abstraction) and abort the write if it has. Reference the `scanResults`
loop and the `target.read`/`target.write` calls so the fix stays localized to
the apply path.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Regex allowlist entries do partial (substring) matching, not "fully" as documented.

Header comment says an entry suppresses a finding when it matches "exactly (string) or fully (regex)" (lines 8-9), but isAllowlisted uses re.test(secret) (line 133) which is an unanchored substring test. A user-added allowlist regex without ^...$ anchors will suppress any secret merely containing a match, potentially masking unrelated genuine secrets — a false-negative/leak risk, since suppressed findings are never redacted (see src/proxy.mjs filtering findings with !isAllowlisted(...) before redaction).

Either anchor the test (new RegExp('^(?:' + re.source + ')$', re.flags)) or update the docs/DEFAULT_ALLOWLIST_CONTENT to clarify substring semantics and warn users to anchor their own patterns.

🛡️ Possible fix: require full-string match for regex entries

 export function isAllowlisted(secret, allowlist) {
   if (allowlist.literals.has(secret)) return true
   return allowlist.patterns.some(re => {
     re.lastIndex = 0
-    return re.test(secret)
+    // Require the pattern to match the *entire* secret, not just a substring.
+    const m = re.exec(secret)
+    return !!m && m[0] === secret
   })
 }

Also applies to: 129-135

🤖 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 `@src/allowlist.mjs` around lines 8 - 9, Regex allowlist matching in
isAllowlisted is currently using a substring test instead of the documented
full-match behavior. Update the allowlist regex handling so regex entries only
suppress a finding when the entire secret matches, and keep the exact-string
path unchanged. Use the isAllowlisted logic in src/allowlist.mjs as the main fix
point, and make sure any matching behavior referenced by src/proxy.mjs still
aligns with the documented “exactly or fully” semantics.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Backups of secret-containing files use default (umask-dependent) permissions.

copyFileSync/mkdirSync here don't set restrictive modes, so files under ~/.broom/backups/... inherit the process umask (often 644/755), leaving copies of files that still contain the raw secrets readable by other local users on shared systems. For a tool whose whole purpose is protecting secrets, this weakens the guarantee.

🔒 Restrict backup file/dir permissions

   backup(file) {
     const abs = resolve(file)
     if (this._copied.has(abs)) {
       return this._destPath(abs)
     }

     const dest = this._destPath(abs)
-    mkdirSync(dirname(dest), { recursive: true })
-    copyFileSync(abs, dest)
+    mkdirSync(dirname(dest), { recursive: true, mode: 0o700 })
+    copyFileSync(abs, dest)
+    chmodSync(dest, 0o600)

🤖 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 `@src/backup.mjs` around lines 40 - 64, The backup flow in backup(file) creates
copied secret files and parent directories with default umask-based permissions,
which can leave backups readable by other users. Update the
mkdirSync/copyFileSync handling in backup(file) so the backup directory and all
copied files are created with restrictive permissions (for example, owner-only
access) regardless of the process umask. Keep the fix localized to the backup
creation path and the SQLite sibling copy loop so all entries in _entries remain
private.

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Sibling -wal/-shm copy swallows all errors, not just missing-file.

Catching every error (including EACCES/EIO) treats real failures the same as "sibling doesn't exist," so a permission problem on a WAL/SHM file silently produces an incomplete backup right before the original is redacted in place.

Only ignore ENOENT

       try {
         const sibDest = dest + suffix
         copyFileSync(sib, sibDest)
-      } catch {
-        // sibling doesn't exist — that's fine
+      } catch (e) {
+        if (e.code !== 'ENOENT') throw e
       }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// SQLite siblings — present for Cursor's .vscdb files
	for (const suffix of ['-wal', '-shm']) {
	const sib = abs + suffix
	try {
	const sibDest = dest + suffix
	copyFileSync(sib, sibDest)
	} catch {
	// sibling doesn't exist — that's fine
	}
	}
	// SQLite siblings — present for Cursor's .vscdb files
	for (const suffix of ['-wal', '-shm']) {
	const sib = abs + suffix
	try {
	const sibDest = dest + suffix
	copyFileSync(sib, sibDest)
	} catch (e) {
	if (e.code !== 'ENOENT') throw e
	}
	}

🤖 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 `@src/backup.mjs` around lines 52 - 61, The sibling copy loop in backup logic
is swallowing every error for the `-wal` and `-shm` files, so only missing-file
cases should be ignored. Update the try/catch around `copyFileSync` in the
`src/backup.mjs` backup flow to check the thrown error’s code and suppress only
`ENOENT`, while rethrowing or surfacing all other errors such as permission or
I/O failures. Keep the change localized to the sibling copy block that handles
`abs`/`dest` and the `suffix` loop.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Line numbers can be wrong (and computed inefficiently) once files have already been redacted.

target.read() is called fresh for every finding to compute lineNumber(text, f.start). Per the CLI flow (bin/broom.mjs), writes to target.write(redacted) happen for every target before printReport/printJsonReport are invoked. So in --apply mode this re-read returns the already-redacted file content, while f.start/f.end were computed against the original pre-redaction text captured during scanning. If any earlier finding's placeholder differs in length from the secret it replaced, subsequent offsets no longer point at the same characters in the new text, so reported line numbers can be wrong.

Separately, re-reading the whole file per-finding (rather than once per target) is wasteful, especially for large transcripts.

Fix by threading the original scanned text through ScanResult (or reading/caching it once per target before any writes) instead of re-reading post-write:

Cache text once per target, sourced from scan time

   for (const { target, findings } of dirty) {
     const shortLabel = target.label.replace(process.env.HOME ?? '', '~')
     console.log(`  ${bold(target.source)}  ${shortLabel}`)

+    // Use the text captured at scan time, not a fresh re-read (which may
+    // reflect already-redacted content after --apply writes).
+    const text = target.scannedText ?? null
+
     // Group by severity for ordered display
     const bySev = Object.fromEntries(SEVERITY_ORDER.map(s => [s, []]))
     for (const f of findings) bySev[f.severity].push(f)

     for (const sev of SEVERITY_ORDER) {
       for (const f of bySev[sev]) {
         const tag    = color(sev, `[${sev.padEnd(8)}]`)
         const rule   = f.ruleId.padEnd(26)
         const masked = maskSecret(f.secret)
-        let lineInfo = ''
-        try {
-          const text = target.read()
-          lineInfo = dim(`  line ${lineNumber(text, f.start)}`)
-        } catch { /* non-blocking */ }
+        const lineInfo = text ? dim(`  line ${lineNumber(text, f.start)}`) : ''
         console.log(`    ${tag} ${dim(rule)} ${masked}${lineInfo}`)
       }
     }

This requires the CLI/scan path to pass the raw text (captured before any write) into the ScanResult, e.g. adding a text field alongside target/findings/applied.

🤖 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 `@src/report.mjs` around lines 93 - 118, Line numbers are being recomputed from
post-redaction content and the file is re-read once per finding in the report
loop, which can make offsets incorrect and is inefficient. Update the reporting
flow around printReport and the ScanResult/target.read() path so the original
pre-redaction text captured during scanning is stored or cached once per target
and reused for lineNumber(text, f.start). Ensure bin/broom.mjs passes that raw
text through before any target.write(redacted) happens, and avoid re-reading the
file inside the per-finding loop.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Lossy BLOB↔string round-trip risks corrupting Cursor's stored data.

Buffer.isBuffer(row.value) ? row.value.toString('utf8') : ... silently mangles any non-UTF8 binary payload (replacement characters), and write() then persists that string back via run(text, k), permanently overwriting the original bytes and changing the column value's type affinity from BLOB to TEXT for that row. If cursorDiskKV/ItemTable ever store compressed or otherwise non-UTF8-encoded blobs under AI-related keys, this adapter will corrupt them irrecoverably on the very first clean --apply, defeating the tool's "backup first" safety guarantee for that row's logical content (the backup covers the raw file bytes, but the redaction path itself already lost information before ever getting there).

Consider validating that a BLOB decodes losslessly before treating it as text (e.g., round-trip check Buffer.from(str, 'utf8').equals(original)), and skipping/flagging rows that fail rather than silently normalizing them.

Also applies to: 139-158

🤖 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 `@src/sources/cursor.mjs` around lines 96 - 112, The Cursor adapter is silently
converting BLOB values to UTF-8 strings, which can corrupt non-UTF8 payloads and
change their stored type when written back. Update `readRows` in `cursor.mjs` to
only treat `row.value` as text when it decodes losslessly, and skip or flag rows
that fail a round-trip check instead of normalizing them. Then make sure
`write()` only persists validated string values for the AI-related keys handled
by `cursorDiskKV`/`ItemTable`, so raw binary data is never overwritten with
mangled text.

[gemini-code-assist]

Code Review

This pull request implements the core functionality of broomsticks, transforming it from a placeholder into a fully functional tool that sweeps leaked secrets out of AI coding-assistant transcripts (Claude Code, Codex, and Cursor). It introduces a robust detection ruleset, a scanner with Shannon entropy gating, redaction logic, backup and allowlist systems, and a local redacting proxy. The review feedback highlights several critical issues, particularly around cross-platform compatibility on Windows (such as incorrect path joining in backups, missing USERPROFILE fallback in reporting, and a read-only database connection during write operations in the Cursor adapter). Additionally, suggestions were made to handle proxy startup failures gracefully, prevent potential crashes from missing regex indices, and escape paths containing spaces or XML special characters in systemd and launchd configurations.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

The database connection in the write method is opened with readOnly: true. Attempting to execute an UPDATE statement on a read-only database connection will throw an error and fail to apply the redactions.

Suggested change

	const conn = new DatabaseSync(db, { open: true, readOnly: true })
	const conn = new DatabaseSync(db, { open: true })

[gemini-code-assist]

On Windows, absolute paths start with a drive letter (e.g., C:\Users\alice\...). Since abs.startsWith(sep) will be false on Windows, relative will retain the drive letter and colon. Joining this with this.dir results in an invalid path containing a colon in the middle (e.g., C:\Users\alice\.broom\backups\stamp\C:\Users\alice\...), which will cause file operations to fail with EINVAL or ENOENT.

To fix this, strip the drive letter and any leading separators.

Suggested change

	const relative = abs.startsWith(sep) ? abs.slice(sep.length) : abs
	return join(this.dir, relative)
	const relative = abs.replace(/^[a-zA-Z]:/, '').replace(/^[\\/]+/, '')
	return join(this.dir, relative)

[gemini-code-assist]

If startProxy fails (for example, if the port is already in use), the returned promise will reject. Since there is no try ... catch block around await startProxy in bin/broom.mjs, this will result in an unhandled promise rejection and crash the CLI with a raw stack trace. Wrapping this call in a try ... catch block allows printing a clean, user-friendly error message.

  let server
  try {
    server = await startProxy({ port, verbose, allowlistFile: option('--allowlist') })
  } catch (err) {
    console.error('broom: failed to start proxy on port ' + port + ': ' + err.message)
    process.exit(1)
  }

[gemini-code-assist]

If a RegExp pattern is ever defined or compiled without the d flag, match.indices will be undefined. Accessing match.indices[grp] directly will then throw a TypeError and crash the scanning process. Using optional chaining (match.indices?.[grp]) makes this code more robust and adheres to defensive programming practices.

Suggested change

	const indices = match.indices[grp] ?? match.indices[0]
	const indices = match.indices?.[grp] ?? match.indices?.[0]

[gemini-code-assist]

On Windows, process.env.HOME is typically undefined (Windows uses USERPROFILE). If process.env.HOME is undefined, process.env.HOME ?? '' evaluates to an empty string ''. Calling target.label.replace('', '~') will then prepend ~ to the beginning of every label (e.g., C:\path becomes ~C:\path).

Using process.env.USERPROFILE as a fallback and checking if the home path is non-empty before replacing avoids this issue.

Suggested change

	const shortLabel = target.label.replace(process.env.HOME ?? '', '~')
	const home = process.env.HOME || process.env.USERPROFILE
	const shortLabel = home ? target.label.replace(home, '~') : target.label

[gemini-code-assist]

If any of the paths (nodeBin, broomBin, or logFile) contain XML special characters (such as & in a user's home directory like /Users/bob&alice), the generated plist file will be malformed XML, and launchd will fail to load it. Escaping these values ensures the plist remains valid.

function launchdPlist(nodeBin, broomBin, port) {
  const logFile = join(homedir(), '.broom', 'proxy.log')
  const esc = (s) => s.replace(/[<>&]/g, m => ({ '<': '&lt;', '>': '&gt;', '&': '&amp;' }[m]))
  return `<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN"
  "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
  <key>Label</key>
  <string>com.broomsticks.proxy</string>
  <key>ProgramArguments</key>
  <array>
    <string>${esc(nodeBin)}</string>
    <string>${esc(broomBin)}</string>
    <string>proxy</string>
    <string>--port</string>
    <string>${esc(String(port))}</string>
  </array>
  <key>RunAtLoad</key>
  <true/>
  <key>KeepAlive</key>
  <true/>
  <key>StandardOutPath</key>
  <string>${esc(logFile)}</string>
  <key>StandardErrorPath</key>
  <string>${esc(logFile)}</string>
</dict>
</plist>
`
}

[gemini-code-assist]

If the path to the Node.js executable (nodeBin) or the broomsticks binary (broomBin) contains spaces (which is common on some systems or user directories), systemd will fail to parse the ExecStart directive and the service will fail to start. Wrapping these paths in double quotes ensures they are parsed as single arguments.

Suggested change

	ExecStart=${nodeBin} ${broomBin} proxy --port ${port}
	ExecStart="${nodeBin}" "${broomBin}" proxy --port ${port}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (6)

test/detector.test.mjs (1)

17-33: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Inconsistent secret obfuscation triggers scanner noise.

The slack-bot-token sample is deliberately assembled from parts to avoid tripping secret scanners (per the inline comment), but private-key (line 19), github-pat (line 23), google-api-key (line 25), jwt (line 30), and generic-secret (line 32) are left as raw literals and are all flagged by Betterleaks in this diff. For a security-focused tool, consistently applying the same assembly/obfuscation technique to every planted sample would avoid repeated false-positive noise in scanner reports/CI checks.

🤖 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 `@test/detector.test.mjs` around lines 17 - 33, The sample fixtures in SAMPLES
are inconsistently obfuscated: only slack-bot-token is assembled to avoid
scanner hits, while other planted secrets like the private-key, github-pat,
google-api-key, jwt, and generic-secret entries remain raw literals and trigger
Betterleaks. Update those fixtures in detector.test.mjs to use the same
part-wise assembly/indirection approach used for slack-bot-token so the test
data still exercises detection without embedding scanner-friendly secret
literals.

Source: Linters/SAST tools

test/allowlist.test.mjs (1)

22-28: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Same unobfuscated synthetic GitHub token duplicated across three test files.

ghp_1234567890abcdefABCDEF1234567890abcd (line 27) is identical to the GH constant in test/redactor.test.mjs and the github-pat sample in test/detector.test.mjs, and each is independently flagged by Betterleaks. Consider extracting a shared test-fixtures module with an assembled/obfuscated form (similar to the slack-bot-token technique already used in detector.test.mjs) to reduce duplication and scanner noise.

🤖 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 `@test/allowlist.test.mjs` around lines 22 - 28, The synthetic GitHub token
used in this allowlist test is duplicated across multiple tests and is being
flagged repeatedly by Betterleaks. Update this test to source the token from a
shared test-fixtures module, matching the approach already used in
detector-related tests, and keep the token assembled/obfuscated rather than
inline. Reuse the same fixture from the relevant test constants in
test/allowlist.test.mjs, test/redactor.test.mjs, and test/detector.test.mjs so
the shared sample is defined once and scanner noise is reduced.

Source: Linters/SAST tools

test/cli.test.mjs (2)

26-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add a timeout guard to spawnSync.

If the CLI ever regresses into a hang (e.g., a future change that blocks unexpectedly in the scan/clean path), these spawnSync calls would block the test run indefinitely with no CI-level guard.

♻️ Proposed fix

 function run(args, home) {
   return spawnSync(process.execPath, [BIN, ...args], {
     env: { ...process.env, HOME: home, USERPROFILE: home },
     encoding: 'utf8',
+    timeout: 10_000,
   })
 }

🤖 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 `@test/cli.test.mjs` around lines 26 - 31, Add a timeout guard to the test
helper run so spawnSync cannot hang indefinitely if the CLI blocks. Update the
spawnSync call in run to include a finite timeout and handle the timed-out
result in the existing cli test flow. Keep the change localized to run in
test/cli.test.mjs so all CLI invocations get the guard automatically.

---

79-102: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Several flakiness risks in the long-running proxy test.

• Hardcoded port 39117 (line 81) can collide with another process/parallel test run, causing an EADDRINUSE failure unrelated to the code under test.
• Fixed sleep(800) (line 90) before asserting stdout contains "listening on" is timing-dependent; slower CI machines could fail this assertion even when the proxy is working correctly.
• No 'error' listener is attached to child (lines 82-87); if spawn fails (e.g. ENOENT), Node emits an unhandled 'error' event that will crash the test process instead of failing the assertion cleanly.
• Only 50ms is given between SIGTERM/SIGKILL and the finally-block rmSync (lines 97-100); if file handles aren't released in time, cleanup can fail intermittently, especially on Windows.

♻️ Suggested direction

+  child.on('error', () => {}) // avoid uncaught exception on spawn failure
+
   let stdout = ''
   child.stdout.on('data', d => { stdout += d })

   try {
-    await sleep(800)
+    // poll for the "listening on" banner instead of a fixed delay
+    for (let i = 0; i < 50 && !/listening on/i.test(stdout); i++) {
+      await sleep(50)
+    }
     assert.equal(child.exitCode, null, 'proxy must still be running, not exited')

🤖 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 `@test/cli.test.mjs` around lines 79 - 102, The long-running proxy test in
test/cli.test.mjs is flaky because it uses a hardcoded port, fixed sleeps, no
child process error handling, and an aggressive cleanup delay. Replace the
static port in the proxy spawn test with a dynamically selected free port, wait
for the proxy to signal readiness instead of sleeping a fixed 800ms, and attach
an error listener to the spawned child so spawn failures fail the test cleanly.
Also make the shutdown/cleanup path in the proxy test more robust by waiting for
the child to exit before removing the temp home directory; use the existing test
helpers and the spawn/child handling in the proxy test block to locate the
changes.

test/backup.test.mjs (2)

1-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing coverage for SQLite sibling (-wal/-shm) copy behavior.

BackupSession.backup() also copies -wal/-shm siblings for Cursor's .vscdb files (per src/backup.mjs:47-55), but this test suite only exercises a plain .jsonl file. Adding a case with sibling files present (and absent) would guard the sibling-copy branch described as a regression area in the file header comment.

🤖 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 `@test/backup.test.mjs` around lines 1 - 49, The backup tests miss coverage for
the sibling-copy branch in BackupSession.backup() that handles SQLite WAL/SHM
files for Cursor .vscdb backups. Add a test that creates a main database file
plus matching -wal and -shm siblings, then verifies backup() copies all present
siblings and still succeeds when one or both siblings are absent. Use the
BackupSession.backup and _destPath behaviors in src/backup.mjs as the reference
points so the new assertions exercise the same sibling-handling logic.

---

43-49: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Windows-path test only verifies colon removal, not actual path flattening.

On a POSIX test runner, path.join treats backslashes as literal characters rather than separators, so 'C:\\Users\\alice\\x.jsonl' becomes 'Users\\alice\\x.jsonl' after the drive-letter strip, and join(this.dir, ...) never interprets those backslashes as segment boundaries. The assertion !win.slice(2).includes(':') would pass even if _destPath did nothing more than strip the drive-letter prefix — it doesn't confirm the path is actually flattened into a valid, joinable relative path (e.g., that no residual leading separator remains, or that the segments compose as expected). Consider asserting the full expected string instead of only the colon-absence, so a regression in the flattening logic itself would be caught.

♻️ Suggested stronger assertion

 test('_destPath strips leading separators and Windows drive letters', () => {
   const s = new BackupSession('/backups/ts')
   assert.equal(s._destPath('/home/alice/x.jsonl'), join('/backups/ts', 'home/alice/x.jsonl'))
   // No mid-path colon should survive from a C:\ style absolute path.
   const win = s._destPath('C:\\Users\\alice\\x.jsonl')
   assert.ok(!win.slice(2).includes(':'), 'no drive-letter colon should remain in the joined path')
+  assert.equal(win, join('/backups/ts', 'Users\\alice\\x.jsonl'))
 })

🤖 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 `@test/backup.test.mjs` around lines 43 - 49, The _destPath test in
BackupSession only checks that the Windows drive-letter colon is removed, but it
does not verify that the path is actually flattened into the expected joinable
relative form. Strengthen the assertion in the backup.test.mjs test case by
checking the full expected output from BackupSession._destPath for the
Windows-style input, so a regression in separator stripping or segment
normalization is caught rather than only colon removal.

🤖 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 `@test/cursor.test.mjs`:
- Line 10: The cursor test imports DatabaseSync from node:sqlite, but the
current Node minimum in package.json is too low for stable support on
22.5.x–22.12.x. Update the test setup by either raising the Node engine floor to
>=22.13.0 or adding --experimental-sqlite to the node --test invocation, and
make sure the change is applied wherever the test runner is configured so
cursor.test.mjs can run consistently.

In `@test/detector.test.mjs`:
- Around line 76-87: The overlap test is currently vacuous because it only
checks pairwise non-overlap, so it still passes when scanText returns no
findings. In the detector.test.mjs case around scanText and the
overlapping-matches test, add an explicit assertion that findings contains at
least one result before the nested loop, so a regression that drops all findings
will fail meaningfully.

---

Nitpick comments:
In `@test/allowlist.test.mjs`:
- Around line 22-28: The synthetic GitHub token used in this allowlist test is
duplicated across multiple tests and is being flagged repeatedly by Betterleaks.
Update this test to source the token from a shared test-fixtures module,
matching the approach already used in detector-related tests, and keep the token
assembled/obfuscated rather than inline. Reuse the same fixture from the
relevant test constants in test/allowlist.test.mjs, test/redactor.test.mjs, and
test/detector.test.mjs so the shared sample is defined once and scanner noise is
reduced.

In `@test/backup.test.mjs`:
- Around line 1-49: The backup tests miss coverage for the sibling-copy branch
in BackupSession.backup() that handles SQLite WAL/SHM files for Cursor .vscdb
backups. Add a test that creates a main database file plus matching -wal and
-shm siblings, then verifies backup() copies all present siblings and still
succeeds when one or both siblings are absent. Use the BackupSession.backup and
_destPath behaviors in src/backup.mjs as the reference points so the new
assertions exercise the same sibling-handling logic.
- Around line 43-49: The _destPath test in BackupSession only checks that the
Windows drive-letter colon is removed, but it does not verify that the path is
actually flattened into the expected joinable relative form. Strengthen the
assertion in the backup.test.mjs test case by checking the full expected output
from BackupSession._destPath for the Windows-style input, so a regression in
separator stripping or segment normalization is caught rather than only colon
removal.

In `@test/cli.test.mjs`:
- Around line 26-31: Add a timeout guard to the test helper run so spawnSync
cannot hang indefinitely if the CLI blocks. Update the spawnSync call in run to
include a finite timeout and handle the timed-out result in the existing cli
test flow. Keep the change localized to run in test/cli.test.mjs so all CLI
invocations get the guard automatically.
- Around line 79-102: The long-running proxy test in test/cli.test.mjs is flaky
because it uses a hardcoded port, fixed sleeps, no child process error handling,
and an aggressive cleanup delay. Replace the static port in the proxy spawn test
with a dynamically selected free port, wait for the proxy to signal readiness
instead of sleeping a fixed 800ms, and attach an error listener to the spawned
child so spawn failures fail the test cleanly. Also make the shutdown/cleanup
path in the proxy test more robust by waiting for the child to exit before
removing the temp home directory; use the existing test helpers and the
spawn/child handling in the proxy test block to locate the changes.

In `@test/detector.test.mjs`:
- Around line 17-33: The sample fixtures in SAMPLES are inconsistently
obfuscated: only slack-bot-token is assembled to avoid scanner hits, while other
planted secrets like the private-key, github-pat, google-api-key, jwt, and
generic-secret entries remain raw literals and trigger Betterleaks. Update those
fixtures in detector.test.mjs to use the same part-wise assembly/indirection
approach used for slack-bot-token so the test data still exercises detection
without embedding scanner-friendly secret literals.

🪄 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

Run ID: fc00cc1f-d1cc-4da1-b1fe-eee96e05ef4e

📥 Commits

Reviewing files that changed from the base of the PR and between 16a58a0 and b4a2c88.

📒 Files selected for processing (15)

• bin/broom.mjs
• package.json
• src/backup.mjs
• src/detector.mjs
• src/install.mjs
• src/proxy.mjs
• src/report.mjs
• src/sources/cursor.mjs
• test/allowlist.test.mjs
• test/backup.test.mjs
• test/cli.test.mjs
• test/cursor.test.mjs
• test/detector.test.mjs
• test/no-network.test.mjs
• test/redactor.test.mjs

✅ Files skipped from review due to trivial changes (1)

• test/no-network.test.mjs

🚧 Files skipped from review as they are similar to previous changes (8)

• package.json
• src/backup.mjs
• src/sources/cursor.mjs
• bin/broom.mjs
• src/install.mjs
• src/report.mjs
• src/detector.mjs
• src/proxy.mjs