Testing
-
Pinned Fixtures Need a Separate Freshness Check
The logger’s conformance suite passed against a pinned fixture release that didn’t include the new cases its changes were meant to satisfy.
The pin made the fixture selection explicit. It didn’t tell CI whether the selected fixtures were current.
The fix kept the pin and added a separate check for upstream changes. It also added a guard against running the job without its fixtures.
Keep the test input identifiable
The logging spec and its fixtures live in the separate treering repository. The logger’s workflow checks out a named fixture release into
.treeringand runs the conformance suite against it.That is useful when investigating an old result. The workflow identifies the intended fixture release rather than asking whatever happens to be on the upstream branch at the time.
A tag can still move, so a commit SHA is a stronger pin. Recording the resolved revision also helps establish exactly what ran. Pinning fixtures is one part of reproducibility, not a guarantee about the entire build environment.
The problem was coverage, not the existence of the pin. A suite can pass all of its selected cases without including the case an implementation change was supposed to address.
The older fixtures hadn’t ceased to exist or become invalid. They answered a narrower question than the review needed.
Check freshness separately
The added step fetches treering’s default branch and compares its
fixtures/directory with the pinned checkout. If the contents differ, the job fails and reports the difference along with the newest available fixture tag.That makes the mismatch visible without silently replacing the test input.
It also introduces a deliberate dependency on current upstream state. Re-running an old commit can now produce a different freshness result after treering changes, even if the conformance result against the pinned fixtures remains the same.
I want those results understood separately: the selected cases may still pass while the pin needs review. An upstream difference isn’t, by itself, proof that the implementation fails a new case.
The workflow limits the comparison to
fixtures/. That keeps unrelated repository changes from triggering this particular guard. But it doesn’t establish that specification prose is irrelevant. A prose change can alter or clarify the contract before a fixture captures it.The directory comparison detects fixture-content drift. It isn’t a complete check for every change in the specification.
The failure message helps with the next step, but the newest tag is only a candidate to review. It might not contain every change on the default branch, and adopting it may require implementation work. Automatically changing the pin would skip that decision.
Missing fixtures are another failure
The runner allows a local contributor to run the library’s ordinary tests without checking out treering. In that case, it skips conformance tests.
That can be reasonable locally. In a CI job whose purpose is conformance, missing fixtures must be an error.
The workflow added this guard:
test -f .treering/fixtures/README.mdIt catches a missing checkout at the expected path before the runner can skip the suite. The skipped tests don’t become passing tests; the risk is that the overall job can succeed without exercising them.
The existence check is intentionally limited. A README doesn’t prove the fixture cases are present, parse correctly, or run. The runner still needs to validate its inputs and make the executed coverage visible.
Both fixes preserve information the final green check couldn’t explain on its own. One says whether the fixture checkout is present. The other says whether its contents differ from upstream. The conformance run says whether the implementation passes the selected cases.
Keeping those questions separate makes a failure easier to act on. It also makes a pass easier to describe without claiming more than the job checked.
Sources
- logan-logger-ts conformance workflow — the fixture pin, checkout guard, and freshness comparison.
- actions/checkout — selecting a revision and checkout path.
I’d appreciate a follow. You can subscribe with your email below. The emails go out once a week, or you can find me on Mastodon at @[email protected].
-
An Unsafe Autofix Can Change What a Test Covers
The serializer has a test for an array hole:
// biome-ignore lint/suspicious/noSparseArray: an array hole is the case under test expect(safeStringify([1, , 3])).toBe('[1,"[undefined]",3]');Replacing the hole with an explicit
undefinedleaves the expected output unchanged. It also removes the case the test was meant to cover.Biome offers that replacement as an unsafe lint fix. It isn’t an ordinary formatting change, and it doesn’t happen with
--writealone.Absent and undefined aren’t the same input
The middle position in
[1, , 3]has no own element. In[1, undefined, 3], the element exists and its value isundefined.Reading either position by index produces
undefinedin these arrays, but array methods don’t always treat them alike.Array.prototype.mapskips empty slots. If the serializer mapped a sanitizing function over the sparse array, the callback wouldn’t run for the hole. The hole would remain in the result, and JSON serialization would render it asnull.The implementation instead uses an indexed loop. It reads each position and passes the value through the sanitizer, producing the logger’s explicit
"[undefined]"marker for the hole.With that implementation, both inputs produce:
[1,"[undefined]",3]The output is intentionally the same. The inputs still need separate tests because a future implementation change could handle one correctly and the other incorrectly.
The original comment blamed the wrong command
The comment above the test said
biome check --writewould replace the hole. Without the suppression, that command reports the rule violation and leaves the sparse array intact.Adding
--unsafeenabled the replacement:[1, , 3] → [1, undefined, 3]Biome classifies the fix as unsafe because it can change behavior. The corrected comment needs to name the command that opts into that change.
I like having the distinction in the tooling. A normal cleanup command should not quietly make this decision for the test. An explicit unsafe-fix pass still needs review, in source files as well as tests.
The suppression is narrow and has a concrete reason: this particular hole is deliberate test input.
A passing rewritten test can lose its purpose
After the replacement, the assertion still passes against the indexed-loop implementation. It now checks explicit
undefined, not an absent element.A later refactor from the loop to
mapcould keep that rewritten test passing while changing sparse-array output. The original hole test would catch the difference.Coverage numbers don’t settle this. The line can still execute, and some coverage measures may remain unchanged, without preserving the original input case. There is no general guarantee that every coverage metric would be identical.
The review question is simpler: does this test still contain the input it was written to exercise?
Odd syntax in a fixture can be a mistake, but it can also be the entire reason the test exists. Before accepting an autofix, read the assertion and the reason for the unusual input together.
Here, the extra comma isn’t clutter. Removing it removes the sparse-array case.
Sources
- Biome noSparseArray — replacing holes with explicit
undefinedis an unsafe fix. - Biome unsafe fixes — opting into behavior-changing fixes.
- Array methods and empty slots — differences in how array methods handle holes.
I’d appreciate a follow. You can subscribe with your email below. The emails go out once a week, or you can find me on Mastodon at @[email protected].
- Biome noSparseArray — replacing holes with explicit
-
An Empty Environment Variable Is Set. My Code Disagreed.
LOG_TIMESTAMP=""is not the same as not settingLOG_TIMESTAMP. One of those is a user who typed something and got it wrong. The other is a user who never typed anything. My config loader collapsed them into the same branch, and the bug it produced is one I’d never have found by reading the code, because the output was correct.Here’s the guard:
if (env.LOG_TIMESTAMP) { config.timestamp = env.LOG_TIMESTAMP.toLowerCase() === 'true'; }Empty string is falsy in JavaScript. So
LOG_TIMESTAMP=""skips the branch,config.timestampstays unset, and the logger falls through to its default. Which is the right value! An empty string isn’t a valid boolean, so ignoring it is exactly what should happen.What doesn’t happen is the warning.
The spec says be noisy about it
The contract this library is judged against has a clause, §6.3, saying an environment variable set to a value the implementation doesn’t recognize must be ignored and diagnosed. Ignore the garbage, then tell the user you ignored it, because a config value that silently does nothing is how people lose an afternoon.
An empty string is unrecognized. So
LOG_TIMESTAMP=""should ignore the value and print something like:[logan-logger] LOG_TIMESTAMP="" is not a recognized boolean and was ignored. Accepted: true, 1, yes, on / false, 0, no, off.Instead it printed nothing, and did the right thing, and looked from the outside exactly like a user who hadn’t set the variable at all. Somebody with a broken
.envline, a shell expansion that came back empty, a Kubernetes ConfigMap key with no value, all of them get silence and the default.Now think about what kind of test catches that. Not an output comparison, because the output is identical either way. Not a value assertion, because the value is right. The only thing that distinguishes the bug from correct behavior is the presence of a warning nobody wrote a test for. You have to be asserting on diagnostics, and you have to be asserting on their absence too.
This bug can’t happen in Go or Rust
I mean that as a claim about the languages, not a compliment to myself, and it’s the part I keep chewing on.
In Go you cannot write
if os.Getenv("LOG_TIMESTAMP"). It doesn’t compile. There’s no truthiness, so you’re forced into an explicit comparison, and at that moment you have to decide what you meant. If you want presence, the standard library hands you a two-value form that makes the distinction impossible to skip:value, ok := os.LookupEnv("LOG_TIMESTAMP")okis presence.valueis content. They’re separate variables because they’re separate questions.Rust does the same thing through the type system.
env::vargives you aResult<String, VarError>, andVarError::NotPresentis a distinct variant fromOk(""). You can’t accidentally treat an empty value as an absent one, because they aren’t the same shape and the compiler will make you handle both.JavaScript gives you
string | undefinedand an idiom,if (value), that quietly foldsundefinedand""together along with"0"andNaN. The idiom is so normal that it reads as correct in review. I wrote it, I reviewed it, and it survived three releases in a section of the code I’d already been editing.The fix is one operator and a comment explaining why the obvious version is wrong:
// Presence, not truthiness. An empty string is set, matches no accepted value, // and so takes the unrecognized path including its diagnostic (SPEC 6.3). A // truthy guard skips it silently: the resulting value stays accidentally right // while the required warning disappears. if (env.LOG_TIMESTAMP !== undefined) { const timestamp = parseBooleanEnvironment('LOG_TIMESTAMP', env.LOG_TIMESTAMP); if (timestamp !== undefined) { config.timestamp = timestamp; } }!== undefinedon all four variables. Empty takes the unrecognized path and warns. A variable that was never set stays silent, which is also required, because warning about variables the user never set would be its own kind of useless.How you test for silence
The conformance runner found this, not code review. Which meant teaching it a vocabulary it didn’t have:
input.envfor ambient environment,input.filesfor config files on disk, andexpect.diagnosticsfor what the implementation should have said.The subtle part is the assertion that nothing was printed. That test is only meaningful if you control the environment completely, and a developer’s shell does not cooperate. If I’ve got
LOG_LEVEL=debugexported in my terminal from an hour ago, a case asserting “no diagnostic for an unset variable” will fail on my machine and pass in CI, or the reverse, and either way the suite is now reporting on my shell rather than on the library.So the runner clears all four variables for any case that carries an
envblock, then applies only what the case declares. It restores the environment afterward including on failure, and removes temp directories including on failure. It doesn’t useprocess.chdir(), since that’s process-global state and a parallel test runner will happily hand you somebody else’s working directory.There’s one more guard I’d recommend to anybody building this kind of harness. A fixture format that silently ignores keys it doesn’t understand is a fixture format where a typo becomes a vacuous pass. Write
expect.diagnosticinstead ofexpect.diagnosticsand your case asserts nothing while reporting green. So: an unknowninputkey throws, an unknownexpectkey throws, and a knownexpectkey that the chosen assertion kind never actually produces fails as undischarged. The suite has to be unable to pass by accident before you can believe anything it tells you.Conformance went from 62 to 119 cases with that work, covering the config and file sections for the first time. It’s at 129 today, and I ran it while writing this. All green, 323ms.
Four things worth keeping:
if (env.FOO)is a bug in config code. Use!== undefinedand decide what an empty value means on purpose.- A correct value can hide a missing diagnostic. If your spec requires a warning, assert on the warning, not just the resulting config.
- Test the silence too, and clear the ambient state first, or you’re testing your shell.
- Make your fixture format reject unknown keys. A typo that produces a vacuous pass is worse than no test.
The boolean parser now accepts
true, 1, yes, onandfalse, 0, no, off. The old one only matched the literal stringtrue, soLOG_TIMESTAMP=1used to mean false. Nobody hit that, because at the time nothing read the result anyway. That’s a different post.Sources
- Node.js: process.env — where an unset variable and an empty one both arrive as something falsy
- MDN: Truthy — the coercion table, including the empty string
- Go: os.LookupEnv — presence and value as separate return values
- Rust: std::env::var and VarError —
NotPresentas a distinct variant from an emptyOk - PR #84 — the fix, the ambient-state vocabulary, and the anti-vacuous guards
- treering — the spec and fixtures that caught it
I’d appreciate a follow. You can subscribe with your email below. The emails go out once a week, or you can find me on Mastodon at @[email protected].
-
The Test Checked the Arguments, Not the CLI Contract
The gateway passed
--print-timeout 120to a CLI that expected120s. Its argument test also expected120, so the test passed while the command failed during flag parsing.The formatter and the test agreed. Neither established what the CLI accepted.
The missing unit
The Go service wraps a CLI with a fixed argument list. Callers can supply a prompt and a bounded timeout; the server chooses the executable, agent, output format, and other options.
It uses
exec.Commandwithout an intervening shell. That keeps shell interpretation out of this path, though it doesn’t remove the need to validate inputs or understand how the CLI handles them.The timeout formatter converted a duration to whole seconds, rounding up and enforcing a minimum of one second. It then returned the number as a string.
The fix retained that behavior and added the unit:
return strconv.FormatInt(seconds, 10) + "s"The command’s duration parser rejected
120with a missing-unit error.120sexpressed the intended value.Go’s
time.ParseDurationaccepts unit-bearing strings such as300msand2m. The special value0is also accepted without a unit, so “bare integers never work” would be too broad. The positive values this formatter produced needed the suffix.The assertion repeated the mistake
The test compared the generated arguments with an expected list containing:
--print-timeout 5That was useful coverage of argument assembly. The service deliberately owns most of the command line, and a test can catch an accidental change to those fixed options.
But this expected value came from the same assumption as the formatter: the timeout flag takes a number of seconds.
There was no independent check against the parser on the other side. The test could detect a change away from the expected string while preserving the wrong string indefinitely.
A default displayed as
5m0sin the CLI’s help was a useful clue. It wasn’t a substitute for checking the flag’s contract or exercising the actual parser.The regression cases covered exact seconds, fractional seconds rounding up, and the minimum:
120s,2sfor a 1500ms input, and1sfor zero. Those cases check the formatter’s policy. A small integration check with the supported CLI version adds evidence that the resulting arguments are accepted.Diagnosis still needs to respect redaction
The gateway returned a generic subprocess failure rather than exposing captured stderr. That kept raw CLI diagnostics out of public errors and structured logs, where they could reveal prompts, tokens, or authentication state.
It also meant the ordinary error message didn’t contain the missing-unit explanation.
I wouldn’t resolve that by logging raw stderr everywhere internally. Internal logs can leak secrets too. A controlled reproduction with synthetic input, or a narrowly defined safe diagnostic, is a better way to investigate without weakening the default policy.
An early exit with empty stdout can suggest a startup or parsing problem, but it doesn’t identify the cause by itself. In this case, the parser’s error supplied the evidence.
The argument test was worth keeping. It just needed a companion check at the boundary with the CLI. An exact match to the expected command line doesn’t help when the expected command line is wrong.
Sources
- Go time.ParseDuration — duration-string syntax.
- Go flag.Duration — duration-valued command-line flags.
- Go os/exec — subprocess execution without implicit shell interpretation.
I’d appreciate a follow. You can subscribe with your email below. The emails go out once a week, or you can find me on Mastodon at @[email protected].
-
Four Environment Settings Never Reached the Logger
My logging library documented
LOG_LEVEL,LOG_FORMAT,LOG_TIMESTAMP, andLOG_COLOR. It also had a helper for reading those environment variables, with unit tests that called the helper directly.The logger’s creation path didn’t call it. Setting a documented environment variable didn’t apply that setting to the logger.
The tests covered parsing. They missed the connection between parsing and the API users called.
A tested helper outside the active path
In the last 1.x release, a search for
loadConfigFromEnvironmentin the source found its definition but no call fromcreateLogger().That search was a useful clue, not proof that nobody could call the exported helper. External code could import it. The relevant problem was more specific: creating a logger didn’t load the environment configuration as documented.
The helper returned a partial configuration. A unit test could supply
LOG_LEVEL=debug, call the helper, and assert that the returned level was correct.A user didn’t take those steps. They set the variable and created a logger. Without a call that merged the parsed settings into the active configuration, the successful unit test said nothing about their result.
This is an easy gap to leave in wrapper code. The helper gets an isolated test because it’s convenient to exercise. The connection to the public entry point still needs coverage of its own.
Another configuration field had a similar problem. The public type accepted a
transportsarray, while the Node implementation constructed its own transport list instead of using that field. Type checking accepted the option; it couldn’t establish that the runtime honored it.Making the settings work changed existing behavior
The 2.0 release connected the environment settings to logger creation. It also made them take precedence over configuration passed to
createLogger().That precedence rule matters during an upgrade. A process can already contain one of these variables even if the previous logger ignored it. Once the variable becomes active, the same application code can produce different logs.
The release treated this as a breaking change and documented it. I think that was appropriate: the old behavior was wrong according to the docs, but it was still the behavior an existing installation had been running.
Boolean parsing changed too. The old helper recognized the literal string
true;LOG_TIMESTAMP=1produced false when parsed directly. The replacement accepted additional forms, including1.Activating the environment settings and changing accepted values are related changes, but they aren’t interchangeable. An upgrade note needs to explain both what is now read and how it is interpreted.
A documented feature doesn’t become harmless to activate just because it should have worked before.
Exercise the API the documentation describes
The small regression test I want starts where the user starts: set an environment variable, create the logger through its public API, and observe the resulting output.
It also needs to control the test environment and restore anything it changes. Otherwise, one configuration test can affect another.
The helper tests should stay. They can cover accepted values and invalid inputs without creating a logger for every case. A public-API test adds different evidence: the parsed setting reaches the behavior users see.
For configuration options, accepting a value is only part of the feature. The value has to make it through the application path where the documentation says it takes effect.
Sources
- The logan-logger-ts 2.0 commit — environment settings becoming active, precedence, and boolean parsing changes.
I’d appreciate a follow. You can subscribe with your email below. The emails go out once a week, or you can find me on Mastodon at @[email protected].
-
The Rate Limiter Trusted a Client-Controlled Header
The search API in my docs site used the first address in
X-Forwarded-Foras the rate-limit key. A nearby comment warned that clients could spoof that value unless the proxy handled incoming headers correctly.The code enabled that trust by default. It didn’t require the deployment to have a trusted proxy in front of it.
A client that could choose the header could choose a new rate-limit bucket. The request counter still worked; the identity going into it wasn’t reliable.
The header needed a deployment policy
The old address selection was short:
request.headers.get('x-forwarded-for')?.split(',')[0]?.trim()It selected a value without establishing who had supplied it.
A proxy may append the address it sees to a forwarded-address chain, but position alone doesn’t establish trust. A usable security policy needs to know which proxies handle the request and how they treat headers supplied by clients.
Counting from the right can work with a known proxy topology. It isn’t a universal rule that the rightmost addresses are safe. An application reachable directly can receive a header with no trusted proxy involved at all.
The fix made forwarded-header handling opt-in through
RATE_LIMIT_TRUSTED_PROXY_HEADER, with an explicit hop configuration. Without that setting, the resolver uses the server-provided request address.That fallback still needs to be understood in the deployed environment. Astro exposes
clientAddress, but the adapter and proxy configuration determine where it comes from. Describing it as impossible to forge would promise more than the application code establishes.I prefer having the trust decision visible in deployment configuration. A comment about “common proxy setups” can’t tell the server which setup it is running in.
Address parsing and storage had separate limits
The change also replaced handwritten IP patterns with
isIPfromnode:net. The resolver rejects a forwarded chain containing an invalid address.A syntactically valid address isn’t necessarily trustworthy. Validation handles malformed input; the proxy policy handles whether the input should be believed. Both checks have a job.
The bucket store needed a bound as well. It was an in-memory map keyed by request-derived identities, so distinct keys could keep adding entries. The change capped the map and evicted the entry with the earliest reset time when full.
That limits retained state, not total traffic. Evicting a bucket can discard its rate-limit history. An in-memory limiter also doesn’t coordinate counters across separate server processes.
One fallback deliberately remained shared: requests with no usable address go into an
unknownbucket. That can make unrelated clients share a limit. Giving every unidentified request a fresh bucket would avoid that contention by abandoning the limit instead.Neither behavior is ideal. The shared bucket makes the limitation explicit without treating missing identity as permission for unlimited requests.
Test the identity before the counter
The fix included tests around proxy configuration, address resolution, bounded state, and the search endpoint. Those are more useful here than checking only that a counter eventually rejects requests.
A counter test can pass while a caller keeps selecting fresh identities.
The deployment deserves a check too: send a client-supplied forwarding header through the real request path and confirm it cannot choose the bucket. Changing the proxy topology should trigger that check again.
The original comment named the risk. The implementation needed a default and a deployment policy that addressed it.
Sources
- MDN: X-Forwarded-For — proxy trust, header parsing, and security-sensitive address selection.
- Astro API reference — the
clientAddressAPI. - semantic-docs PR #93 — explicit proxy configuration, bounded limiter state, and regression coverage.
I’d appreciate a follow. You can subscribe with your email below. The emails go out once a week, or you can find me on Mastodon at @[email protected].
-
I Wrote 993 Lines of Tests for a Shell Script, Then Deleted the Script
My MCP server has a
just mcp-installrecipe that prints aclaude mcp add ...command you paste into a terminal. It reads your.envand emits one--env NAME=valuepair per line. Above that block sits a banner:Secret values are redacted below — substitute them by hand.The thing doing the redacting was a single
sed:sed -E 's/^(EMBED_API_KEY|TEI_API_KEY|NEO4J_PASSWORD)=.*/\1=<redacted>/'Three names on an allowlist. Everything else prints in full, under a banner that promises otherwise.
It took me about 19 hours to go from noticing that to deleting the entire replacement I’d built. Here’s the trip.
An allowlist fails open, and mine had already failed twice
The problem with an allowlist for secrets isn’t theoretical. It’s that the default is print the value, so every new credential leaks until somebody remembers to extend the list.
Mine had already broken twice, and I only worked that out while writing the fix. First, I renamed the
TEI_*env vars toEMBED_*and left the allowlist matchingTEI_API_KEY, a name that no longer existed. Second, the grep feeding the sed was^[A-Z_]+=with no digits in the character class, which silently dropped everyNEO4J_*row before the sed ever saw it. So theNEO4J_PASSWORDarm of that allowlist was unreachable for its entire existence. It never redacted anything. It just sat there looking reassuring.Switching to a pattern match takes ten seconds:
sed -E 's/^([A-Z0-9_]*(KEY|TOKEN|SECRET|PASSWORD|CREDENTIAL)[A-Z0-9_]*)=.*/\1=<redacted>/'I checked it against a synthetic
.envfull of sentinel values. 7 of 7 secrets redacted, includingOAUTH2_TOKEN,S3_SECRET_KEYandEMBED_API_KEY2, all three of which printed in cleartext before. All 10 non-secret variables kept their values, which matters, because the emitted command is useless withoutQDRANT_HOSTandEMBED_MODELintact.Over-redacting a non-secret costs the user a hand-edit. Under-redacting a real one costs them a rotated key. Pick the direction you fail in.
Then I gave it tests, and the tests got mean
A security-relevant expression inlined in a
justrecipe has no test coverage by construction. So I pulled it intoscripts/render-mcp-env.sh(73 lines) and wrotescripts/render_mcp_env_test.go(352 lines) to drive the script throughos/exec.The tests immediately found things the sed one-liner couldn’t have handled.
Credentials hiding in values, not names. The rule only ever inspected the variable name. A value like
bolt://neo4j:hunter2@hostmatches no keyword, so it printed in full.NEO4J_URLandDATABASE_URLare the obvious cases, and they’re exactly the vars people paste into chat.PATis a trap. Adding it for personal access tokens also swallowsPATH,GOPATH,CONFIG_PATH,LOG_PATTERNandCOMPATIBILITY_MODE. A*_PATHvalue is one of the things the command needs to keep. It now matches only as a whole underscore-delimited segment.And my redaction broke the output it was redacting. This one’s my favorite. The emitted line was unquoted, so:
--env FOO=<redacted>parses in a shell as the word
FOO=plus a redirection from a file namedredacted. The block failed to paste correctly whenever a secret was present, which is to say for every actual user. The feature that existed to protect people was the feature that broke the thing for them. ShellCheck flags exactly this and I’d have caught it a week earlier if the logic had been in a file a linter could see, instead of hidden in a recipe body.Fixing the quoting meant leaving sed behind. Per-character shell quoting isn’t expressible in portable sed, so the engine became
grep | awk. I kept the awk POSIX-only and checked it byte-for-byte under mawk (what CI’s Ubuntu runner ships), gawk, andgawk --posix.The script was now 238 lines. The test file was 993.
The tests were right and the script was wrong
Here’s what those 993 lines were actually telling me, once I stopped admiring them.
The awk had to parse
.envto find names and values. The Go binary parses the same file with godotenv v1.5.1. Two parsers, one file, and nobody checking they agreed.They didn’t. Six shapes, each one verified rather than assumed:
$VARand${VAR}expansion- Whitespace trimming
NAME=with an empty value, emitted by one and dropped by the other- Names containing a
. - Indented assignments
#inline comments
Every one of those is a case where the command I told you to paste differs from what the server actually reads. That’s a worse bug than the leak, because it’s silent and it looks like it worked.
So I deleted it. The 238-line script and its 993-line test both went in a single commit, replaced by
internal/envblockat 134 lines with an 868-line test, called from the Justfile as:go run ./cmd/mem0-mcp --print-env-block .envThe binary already links godotenv. Now the pasted block can’t drift from the server’s own parsing, because there’s one parser.
The classifier also got simpler in a way that matters. It’s
strings.Containsover the uppercased name and nothing else:func isSecretName(name string) bool { upper := strings.ToUpper(name) for _, keyword := range secretKeywords { if strings.Contains(upper, keyword) { return true } } // PAT only as a whole underscore-delimited segment, or it would swallow // PATH, LOG_PATTERN, COMPATIBILITY_MODE and friends. return strings.Contains("_"+upper+"_", "_PAT_") }No regexp is reachable from it, on purpose. An anchored or whole-name match classifies
MY.API_KEYas non-secret and prints it in the clear, which was a real bug in the awk version. You can’t write that mistake in this shape.The old test suite had a guard that scanned the script’s source for forbidden patterns. The replacement is a behavioural test, and it’s stronger: it caught a deliberately mutated classifier with a hidden regexp by finding a sentinel value leaking into fixture output. Same evasion class, caught by observing output instead of reading code.
The part that made me laugh
I added a ShellCheck CI job, with a
lint-shell.shthat hard-fails if it discovers fewer than 4 shell scripts, so nobody can quietly delete one past the linter.I deleted one of the scripts it was guarding, and had to lower the floor from 4 to 3. There are three
.shfiles inscripts/now, and the default sits atmin_scripts="${MEM0_MIN_SHELL_SCRIPTS-3}".Five hours from “protect these scripts” to “one fewer script to protect.”
I don’t think the shell version was wasted. I couldn’t have argued for the Go rewrite on day one, because “awk might diverge from godotenv” is a hunch. It only became an argument once I’d written enough tests to enumerate six specific divergences and point at them. The tests didn’t make the script correct. They made the case for its deletion, which was the more useful outcome.
If you’ve got security-relevant logic inlined in a Makefile, a Justfile, or a CI step, that’s the same shape my bug was. Nothing lints it, nothing tests it, and it fails open. Pull it into a file first. You might end up deleting the file, and that’s a fine place to land.
Sources
- godotenv — the Go dotenv parser the server links, v1.5.1
- ShellCheck SC2086 — unquoted expansion, word splitting and redirection
- ShellCheck — the linter itself
- POSIX awk specification — the subset I held the script to for mawk/gawk parity
- POSIX shell command language — quoting and redirection rules behind the
<redacted>bug
I’d appreciate a follow. You can subscribe with your email below. The emails go out once a week, or you can find me on Mastodon at @[email protected].
-
My MIME Allowlist Wasn't Enforcing Anything
I wrote a comment months ago that asserted a security property. Last week I read it back and didn’t believe myself.
The comment sat above the function in
src/lib/r2.tsthat hands out presigned upload URLs, and it said this:Only ContentType is signed, the browser replays it on the PUT.
That reads like a guarantee. The signature covers the content type, so a client can’t lie about what it’s uploading. Confident, specific, and load-bearing, because the whole upload design leans on it.
It was wrong. Not subtly wrong. The content type was never in the signature at all.
Why the uploads work this way
My site takes file uploads and puts them in Cloudflare R2. The bytes never touch my server. A client POSTs metadata to
/api/upload, gets back a presigned PUT URL, and sends the file straight to R2.There are good reasons for that shape. It saves bandwidth and memory on a small box, and my host’s edge proxy blocks multipart POSTs anyway, so routing file bytes through the app was never on the table.
The tradeoff is that the server stops being a gatekeeper. Once you hand someone a signed URL, the signature is the only thing standing between them and the bucket. So the server does its checking up front.
src/pages/api/upload.tsvalidates the request against a Zod schema, and the type field is an enum derived from an allowlist:const MIME_EXTENSIONS = { "image/png": ["png"], "image/jpeg": ["jpg", "jpeg"], // ... "application/pdf": ["pdf"], } as const;There’s a size cap at 10 MB, an extension cross-check so
evil.htmlcan’t ride in claiming to be a PNG, and the signed URL expires after 300 seconds. That’s a reasonable set of controls. I felt fine about it.The probe
The doubt was cheap to resolve, so I resolved it. Two presigned URLs, same command, one with the fix and one without, printing the one query parameter that matters:
const cmd = () => new PutObjectCommand({ Bucket: "b", Key: "uploads/x.png", ContentType: "image/png", }); const before = new URL(await getSignedUrl(client, cmd(), { expiresIn: 300 })); const after = new URL(await getSignedUrl(client, cmd(), { expiresIn: 300, signableHeaders: new Set(["content-type"]), }));Output:
BEFORE SignedHeaders: host AFTER SignedHeaders: content-type;hosthost. That’s the entire list. I passedContentTypeinto the command, the SDK accepted it without complaint, and it never made it intoX-Amz-SignedHeaders.SigV4 query signing only binds the headers named in
X-Amz-SignedHeaders. Everything else is free. A header that isn’t listed isn’t covered by the signature, so the server won’t reject a request that changes it. My URL committed to exactly one thing, the hostname, and left the rest open.So the allowlist was real, the validation ran, the enum rejected anything outside those types, and none of it reached storage. A URL signed for
image/pngwould accept an HTML file, a script, whatever you wanted, for five minutes. The check happened. It just wasn’t binding.The parameter that looks like the fix
While digging through this I hit the trap that probably explains how I got it wrong.
There’s a query parameter called
response-content-type. It shows up on presigned URLs, it has “content type” in the name, and it does nothing to protect an upload. It sets theContent-Typeheader S3 sends back when the object is read later. It’s response metadata. It doesn’t constrain the PUT.If you’re skimming a signed URL looking for evidence that content type is handled, that parameter is right there, looking like proof.
The fix
One option on the presigner call:
const uploadUrl = await getSignedUrl(client, command, { expiresIn: PRESIGN_EXPIRY_SECONDS, signableHeaders: new Set(["content-type"]), });Now the header is in the signature. If the browser PUTs a different
Content-Typethan the one the server signed, R2 rejects the request. The allowlist finally reaches the bucket.The comment got rewritten too, into something that describes a mechanism instead of promising an outcome:
Content-Type is part of the signature, the browser must replay the same header value on the PUT.
What I actually changed my mind about
The one line fix isn’t the interesting part. The interesting part is that a comment made a security claim, sat in the file for months, and nothing anywhere could tell me it had gone stale. Comments don’t run.
So the fix shipped with a contract test that does. It builds a real presigned URL with the actual SDK, no mocks, and asserts on the URL:
expect(signedHeaders).toContain("host"); expect(signedHeaders).toContain("content-type"); expect(url.searchParams.get("X-Amz-Expires")).toBe("300"); expect(url.searchParams.get("response-content-type")).toBeNull();That last line is worth reviewing. It’s a test that exists to say “don’t come back and mistake the lookalike for the real thing.” Presigning is a pure local operation, no network call to R2, so the whole thing runs in about 300ms in CI.
Four things worth stealing from this if you hand out presigned upload URLs:
- Print
X-Amz-SignedHeaderson a URL you’re issuing right now. If it sayshost, your content type isn’t enforced, no matter what you passed to the command. - Passing
ContentTypetoPutObjectCommandis not enforcement. You needsignableHeaders: new Set(["content-type"])on the presigner. response-content-typeis a read-time hint. It’s not an upload control.- Server-side validation that never reaches storage is decoration. Mine was correct and thorough and completely bypassable.
The uncomfortable version of this: I built the allowlist, wrote the enum, added the extension cross-check, capped the size, set a short expiry, and then documented a property the library never gave me. Every individual piece was right. The thing connecting them to the bucket wasn’t there.
Go print your signed headers.
Sources
- AWS s3-request-presigner README — documents
signableHeadersfor enforcing non-x-amz-*headers on presigned requests - Authenticating Requests: Using Query Parameters (SigV4) — how
X-Amz-SignedHeadersdefines what the signature covers - Cloudflare R2 presigned URLs — R2 generates presigned URLs server-side with no network call, and can restrict Content-Type
- Cloudflare R2 S3 API compatibility — which S3 behaviors R2 actually implements
I’d appreciate a follow. You can subscribe with your email below. The emails go out once a week, or you can find me on Mastodon at @[email protected].
- Print
-
Your Vibes Are Not an Agent Eval
Do you really know what you’re doing? You swapped a model, you tuned an agentic workflow, you had the agent rewrite a chunk of a system prompt. You added a skill. Now the output feels sharper. Is that feeling a measurement? No, it’s an impression. This is when the vibes start seeping into your agentic engineering world view.
How Impressions Fail
A subjective assessment isn’t necessarily useless, since it’s how you can notice something is wrong in the first place, and it can be a good signal, or an early signal that leads to a corrective action.
A few things here conspire to work against you.
Recency. You remember the last three runs vividly and the forty before them not at all. If the last three happened to be easy tasks, the model got better. If they were gnarly, it got worse.
Confirmation. You just spent an hour rewriting a prompt. You are not a neutral judge of whether that hour helped. Nobody is.
Prompt drift. This one is sneakier. You’re not asking the same thing you asked last month. Your prompts got better because you got better at prompting, and that improvement gets silently credited to the model.
Task drift. The work changed. You were doing greenfield scaffolding in June and you’re doing debugging in August. Those exercise completely different capabilities, and comparing across them tells you nothing.
All of these will sneak up and bite you in the ass. A decent working knowledge of the system is not a measurement.
The actual risk with an agentic workflow isn’t a sharp and dramatic decline in quality. It’s a slow regression over time as you start missing things that slip through the cracks when you’re not paying attention as closely as you should on that day.
I talked about evals that are worth building in previous posts. You should go have a look at some examples there on how to get started.
How do you test a harness? You need to separate the model from the harness. It turns out the harness changes frequently along with the model. Is it even worth testing the harness?
A model swap tripwire is a good place to get started. A tripwire asks whether this specific change made things worse. It’s a binary operation. You run it before the swap, save the results, and run it again after the swap, and compare the results. Same task, same prompt, only the model changed.
If you keep going on vibes, they will keep telling you things. That may or may not matter. At the end of the day, vibes are a decent smoke alarm, but make for a terrible way to measure quality.
I’d appreciate a follow. You can subscribe with your email below. The emails go out once a week, or you can find me on Mastodon at @[email protected].
-
Start With Ten Tasks You Actually Do
Public benchmarks of large language models are a fine way to compare models in the abstract, but they’re close to useless for answering questions about things that actually matter. Generally, it’s helpful to know which model is the best in general at a specific benchmark, but it doesn’t answer the question of which model is the best at that specific thing you ask it to do all day long.
You need to build a golden set. Ten tasks where you understand the input and the output.
Scoring the code is not the hard part. Picking the ten tasks is going to be the hardest thing. How do you pick something that has a true pass or fail, but also applies to your specific problem?
The temptation is to sit down and try to come up with representative tasks. Chances are you’re going to waste a ton of time and not produce any better results.
Instead, you should be harvesting your tasks from a variety of sources.
What happens when the benchmark that matters is your last ten pull requests?
- Git history. What have you actually been changing? A month of commits will show you the shape of your work faster than introspection will.
- Issues and TODOs. These are tasks somebody already wrote down in task-shaped language.
- Prior agent sessions. If you have logs, this is the best source, because it’s literally the distribution you’re trying to measure.
- The things you retry. Anything you’ve asked an agent twice because the first answer was wrong is a high-value task. It’s already demonstrated it can discriminate.
A good eval task is one where you already know what the failure is and that it’s possible, because you’ve seen it fail.
Scoreable Means Checkable
So now you have your tasks, and it’s a different problem. You need to decide how to score the task and whether or not the agent got it right. This doesn’t necessarily mean you have to build automation from day one. Some good starting points that would qualify are the following:
- Tests pass or don’t
- Output parses or doesn’t
- The right files changed and no others
- A required field is present and well-formed
- The result matches a known-good output you saved earlier
Keep the amount of comparisons small. Don’t expand and keep evaluating. You can keep your old evaluations, but they shouldn’t impact future decisions forever.
Some amount of change in passing or failing over time is representative of a healthy set.
Here’s how you can get started.
Open your git log. Find things that the agent did well and things the agent could have done better. Write a definition of done.
You now have a small golden set that’s going to be more relevant and useful than any leaderboard online, because it was built from the results of your actual work.
The leaderboard tells you which model wins on average. You are not the average.
I’d appreciate a follow. You can subscribe with your email below. The emails go out once a week, or you can find me on Mastodon at @[email protected].