The Silent Failure Is the Most Dangerous One: Fail-Closed in CommitBrief

In every CommitBrief release up to and including v1.16.0, a single config set provider openai command turned off secret scanning on a partial config file, one that didn’t spell out every field. There was no warning and no sign that anything had changed.

I fixed it in v1.17.0, but the real lesson isn’t the fix itself. Most of the silent failures I closed between July and October had the same shape: the tool did something wrong and didn’t say so. This post covers those failures and the one rule I drew from all of them: a security gate must fail closed in the face of uncertainty; only a command that merely informs may degrade softly. The details come from the project’s CommitBrief: CHANGELOG.md, v1.21.1.

This isn’t a blame post, it’s a postmortem. CommitBrief is my tool; every one of these bugs came out of it, and I’ll describe the impact exactly as far as the CHANGELOG does, no less and no more.

One command, three things switched off at once

config set and providers use decoded the config file into a full Config struct, then marshaled the whole struct back. Every field absent from the file was written back not with its documented default but with Go’s zero value: boolean false, number 0, string "". Because setup writes only the fields it touches, partial config files were a common shape.

I reproduced it on an older release binary with a one-line provider: openai config. After a single commitbrief config set provider openai, guard.secret_scan, review.baseline and review.architecture were all false: the secret scanner, the baseline that controls noise, and the architecture-aware review had been switched off together. The same command also zeroed fields like cache.enabled, cache.ttl_days and output.lang.

This entry isn’t a leak report, it’s a report of a protection being lifted without notice. I don’t know how many people ran this command on the affected releases, so I’m saying nothing about how many were affected.

The list above is what one starting file produced; it is not a complete inventory for every config shape. That’s how I wrote it in the CHANGELOG, too. If you think you may have been affected, don’t look only at the keys mentioned: check every value in the commitbrief config show output, or re-run commitbrief setup to get a config with every field written out explicitly.

The fix is to patch the YAML in place instead of decoding and rewriting it: a write touches only the requested key, and comments, key order, anchors and aliases, and keys the schema doesn’t recognize all stay as they were.

A second silence next door was closed in the same release. If you wrote regexp: instead of regex: in the config, the file loaded without complaint and what you wrote did nothing. An unknown config key is now a hard error: it names the file, the exact key, the keys allowed at that level, and a “did you mean” suggestion if there’s a close match. If you want to downgrade it to a warning for one run, there’s --ignore-unknown-config; it still lists every key by name.

The tool that looked at the wrong thing

Three more bugs of the same class. In all three the tool was running; it was just looking at, or sending, the wrong thing.

Suppression leaked across files (v1.19.0). A commitbrief-ignore marker on line N of one file also silenced a finding on line N of a different file in the same diff. Markers are now keyed by file and line together. As the CHANGELOG puts it: findings that were previously silenced by mistake will reappear after the upgrade. Nobody goes looking for a suppressed finding, which makes this the most insidious bug on the list. I wrote about how baselines and PR threads recognize a finding in the previous post.

The pre-push hook didn’t look at the commit being pushed (v1.18.1). When a new branch or tag was pushed, the hook ran commitbrief diff <sha>, which is git diff <sha>: it compares the working tree against that commit and doesn’t inspect the pushed commit. It now inspects the range <sha>^..<sha>. In a shallow clone where the parent isn’t available locally, instead of sending the whole repo it skips the ref and writes a warning to stderr. Old installs keep running the old script until it is regenerated:

commitbrief install-hook --hook=pre-push --yes

Wrong headers (v1.19.0). Because of the OpenAI SDK’s environment defaults, the values of OPENAI_ORG_ID and OPENAI_PROJECT_ID were going to the deepseek, mistral and cohere endpoints as OpenAI-Organization / OpenAI-Project headers. The API key was never affected; the headers are no longer sent.

I’m putting my own tests in the same bucket (v1.17.0): the CLI integration tests could read the developer’s real GEMINI_API_KEY and OPENAI_API_KEY values. A temporary HOME doesn’t isolate the process environment. Depending on the machine they ran on, the tests configured extra providers or rendered in the wrong language; in other words, they passed in CI and silently measured something else locally.

An empty result doesn’t mean success

The distinction that made me think hardest came up in the SARIF output. --sarif writes findings as a SARIF 2.1.0 log to be uploaded to GitHub Code Scanning (v1.19.0). If the model response never parsed into structured findings, meaning the review is degraded, no file is written and an error is raised. The reasoning is in the CHANGELOG: an empty results[] tells Code Scanning that every open alert has been fixed.

An empty diff after filtering, on the other hand, is not a failure. There’s nothing to look at; in that case a valid empty log (results: []) is written so the upload-sarif step finds its file. Both look like “zero findings”, but one means “I looked and found nothing” and the other “I couldn’t look”. The moment you squeeze them into the same output, the gate declares clean something it never looked at.

guard --sarif and leaks --sarif, both added in v1.21.0, carried the same principle from the start. leaks --sarif fails closed in every case that would leave the log incomplete: flags that narrow the scan (--no-history, --file, --dir and the like) are a usage error, and a history scan cut short by --max-commits or the 10000-commit walk ceiling exits 1 without writing a file. Truncation warnings are now printed under --quiet too; they used to go silent there.

A side bug in the same release tested this principle from the opposite direction: a history of exactly 10000 commits was counted as truncated because of an off-by-one at the walkLimit boundary, so leaks gave a false truncation warning. A false alarm is annoying but easy to fix: a wrong alarm is visible, a missing alarm is invisible.

The real gap was in guard (v1.21.1). The --sarif path rejected a degraded review, but without --sarif, guard read that same review as “zero findings” and let it through. Two paths through the same gate, two different answers. guard now exits 1 with guard.degraded on a degraded review; the message also says why: the policy cannot be verified, so the gate blocks. Whether the response is fresh, from the cache, or from a --from-json document, the result is the same.

review and diff --fail-on keep their graceful-degrade behavior. This is a deliberate asymmetry. In a command that informs, if the model response came back broken, it’s useful to show what I have while saying it’s broken. A gate, though, is a command that decides; if it can’t decide, saying “pass” is itself a decision.

A confident wrong answer is worse than no answer

This principle doesn’t belong only to CI gates. In v1.14.0, when I added review.sandbox_command, which re-runs a static finding from the flaky-test detector with a command to verify it, I needed to resolve the test name so the command could say which test to run. I first tried a hand-written multi-language scanner. In all three rounds of fixes it produced confident but wrong names.

In the end I narrowed resolution to Go only (reading the *_test.go files with go/parser). A finding whose name can’t be resolved skips the re-run, writes a warning to stderr, and keeps its static finding. Python, JS, PHP and Java tests still benefit from full static detection; they just can’t be verified by re-running. The reasoning in the CHANGELOG: confident wrong names are a worse outcome than no confirmation at all.

The rules I drew

• “Empty” and “couldn’t look” are never the same output. On a gate that decides, zero findings, an empty results[] and exit 0 are written only when something was actually looked at.
• A gate that decides closes on uncertainty; a command that informs degrades, but says that it has degraded.
• Every input that is silently ignored should be an error, by name. An unknown config key used to be silently dropped; now it errors by name.

Limits — an honest list again

• This list is a selection of the silent failures in the CHANGELOG. I’m not saying there are no silent failures I haven’t found yet.
• The fix protects writes from now on. It doesn’t automatically restore a config that was already zeroed by config set in v1.16.0 or earlier; checking it with config show is up to you.
• Not everything fails closed. An unknown commitbrief-ignore[token] marker falls back, with a warning, to an unscoped marker; that is, it silences more findings, not fewer.
• diff --fail-on keeps soft degradation in the degraded case. If you’re going to use it as a gate, prefer guard.
• The gate is closed only as far as it can detect. It sees whether the response parsed; it can’t catch a well-formed but wrong response.

To try it

• github.com/CommitBrief/commitbrief
• commitbrief.com

Run commitbrief config show on one of your configs and look over every value. Then ask your own CI gate: what does the tool do when it can’t look? If the answer is “pass”, it’s worth rethinking what that green check actually proves.