Project Overview
I built TestFlow Agent — an open-source tool that turns plain-English test cases (or a live browser recording) into runnable Postman, Playwright, and JMeter tests.
Its live-discovery feature launches a headed Chromium via Playwright to record real traffic. That only works where there's a display, so when someone runs the backend inside Docker (no X server), the launch fails. To make that failure friendly, I added a guard that catches the error and returns a clear 503 — "live discovery needs a headed browser; run the backend natively."
Good idea. Buggy execution.
Bug Fix or Performance Improvement
My guard decided "this is a no-display environment" by string-matching the launch error message. I included has been closed in the match:
The problem: Playwright throws "Target page, context or browser has been closed" for a whole range of unrelated failures (a crashed context, a closed page mid-call, a killed browser). Every one of those would now be misdiagnosed as "you're in Docker, run natively" — pointing the user at completely the wrong problem.
An AI code review (GitHub Copilot) flagged it on the PR, and it was dead right: the heuristic was too broad.
Code
Before (backend/services/discoveryService.js):
if (/XServer|X server|\$DISPLAY|has been closed/i.test(message)) {
const error = new Error('Live discovery needs a headed browser ... run natively');
error.statusCode = 503;
throw error;
}
throw launchError;
After:
// Match only display/X-server markers — NOT generic Playwright phrases like
// "Target page, context or browser has been closed", which are unrelated failures.
if (/XServer|X server|\$DISPLAY/i.test(message)) {
const error = new Error('Live discovery needs a headed browser ... run natively');
error.statusCode = 503;
throw error;
}
throw launchError;
🔗 PR: https://github.com/sshankar07/test-flow-agent/pull/5
I verified both directions with a quick check:
const re = /XServer|X server|\$DISPLAY/i;
re.test('...without having a XServer running. Missing X server or $DISPLAY'); // true ✅ real no-display error still caught
re.test('Target page, context or browser has been closed'); // false ✅ unrelated error no longer misfires
My Improvements
- The guard now fires only on genuine display/X-server errors — no more false "run it natively" advice when the real cause was something else entirely.
- Unrelated Playwright failures once again surface as themselves, instead of being swallowed by a misleading
503. - Bonus from the same review pass: the Docker builds moved from
npm installtonpm ci(reproducible installs, no accidental lockfile drift).
Best Use of Sentry
Not used in this submission.
Best Use of Google AI
Not applicable — though it's worth noting the bug was caught by an AI code review on the pull request, which is a nice argument for keeping AI reviewers in the loop for exactly this kind of over-broad heuristic.
Top comments (7)
The detail that makes this worth writing up: an over-broad guard doesn't just misfire, it actively points people away from the real cause. A silent failure wastes an hour; "you're in Docker, run natively" wastes an afternoon and costs you their trust in the next message the tool prints. That's a strictly worse failure mode than no guard at all.
The part I'd protect now is the fix itself, because narrowing a heuristic creates the mirror-image regression: too tight, and a genuine no-display failure stops being recognized. You verified both directions once by hand — real display errors still caught, unrelated ones surfacing correctly — and that's exactly the state that quietly rots when someone widens the pattern again six months from now for a new edge case. What worked for me was keeping both sides as fixtures: for a static scanner of mine I plant known-bad inputs it must catch AND known-good inputs it must not flag, and the second set is the one that actually earns its keep, because false-positive regressions are the ones nobody notices until a user is annoyed. Your two cases are already written — a real
$DISPLAYfailure and a mid-call context close — they just need to live somewhere that re-runs.One more thing that fell out of doing that: I ended up with a genuine known false positive I couldn't cheaply fix, and rather than leave it as a permanent note in the report I gave it an expiry — the date it was accepted plus the condition that kills it, and the test goes red once it's older than that. Otherwise a known exception quietly becomes the new baseline. Nice catch by the reviewer, too; an outside pass on the heuristic is the cheapest audit there is.
Really appreciate this — you nailed the actual risk. "Verified once by hand" was doing a lot of load-bearing work. I turned both cases into a re-running fixture: a MUST-catch set (real $DISPLAY/XServer failures) and, more importantly, a MUST-NOT-flag set ("…has been closed", timeouts, connection-refused) — the false-positive side being the one that quietly regresses. It's the repo's first automated test, and the old "has been closed" bug is now pinned so it can't come back. The expiry-for-known-exceptions idea is going in my notes for the day I actually have one to hang it on. And yeah — an outside pass on the heuristic really is the cheapest audit there is.
The MUST-NOT-flag set is the right instinct. One guard I'd add to it, which cost me a real bug to learn: assert the denominator, not just the findings.
My exclusion pattern had a gap — it skipped one fixture directory but not the clean one sitting next to it, so six known-good files were being scored as real defects in production for weeks. Embarrassing, fixed. But fixing it made the other direction obvious: exclusions fail both ways, and the other way is silent. A MUST-NOT-flag fixture passes when the scanner correctly ignores "…has been closed." It also passes when the scanner never read the file at all. Same green, different reason. So the test now asserts how many files were actually examined — silence has to prove it was earned.
On expiry, for the day you have one: write the killing condition, not just a date. Mine reads "until comment/string-literal parsing lands." A date gets bumped by whoever hits it; a condition can actually be met, and then the exemption has to go.
"Assert the denominator" is the best kind of comment — it caught a real hole. My unit test proved the predicate returns the right boolean but said nothing about whether the guard was still wired in; delete the check and everything stays green. So I added a wiring test that stubs the browser launch and asserts startDiscovery actually maps a no-display failure to a 503 and passes unrelated failures through untouched. Then I mutation-tested it — unwired the guard, watched that test (and only that test) go red — so the green is earned, not assumed. The "same green, different reason" framing is exactly what I was missing.
And thank you for the expiry correction: condition, not date. "Until comment/string-literal parsing lands" can actually be satisfied; a date just gets bumped by whoever trips over it. That's going in as the rule for the first real exemption I hit. Your fixture-directory story is a great cautionary tale — an exclusion that fails silently in the keep direction is exactly the kind of bug that hides for weeks.
"And only that test" is the half I didn't say, and it's the better half. A mutation that turns the suite red tells you something noticed. A mutation that turns exactly one thing red tells you the suite can locate the break — that's the difference between an alarm and a diagnostic. I'd make that property an assertion rather than a habit, because specificity is the first thing to erode as tests multiply.
Two things I hit downstream of this exact setup, both cheaper to handle now than in a month.
Your stub is doing quiet work. You assert that startDiscovery maps a no-display failure to a 503 — but the shape of that failure comes from the stub, not from the browser. If the real launch failure ever changes shape (new message, different error class, wrapped after a version bump), the stub keeps emitting the old one, the test keeps passing, and the mapping in production stops matching anything. Same silent direction as my exclusion bug, one layer further in: the guard is fine, the input drifted out from under it.
Second: the mutation proves the guard today. Nothing yet proves the mutation still runs. Mine ended up living in a script that had never actually been scheduled — the file existing was doing all the reassuring. Worth having something outside the drill assert the drill's own proof of life.
Both landed. Reporting back with the part that matters — whether they can fail.
The fixture corpus moved into one module that both files import, so the wiring stub and the predicate test can no longer drift into agreeing about different things. Each fixture now carries a provenance string saying where the message was actually observed, and a test asserts that string is non-empty. That doesn't make a fixture true, but it makes an invented one visible — you can tell at a glance which of these ever described reality.
The real answer to your first point is a contract test that performs an actual headed launch with DISPLAY stripped and asserts the resulting failure still satisfies the guard. It feeds the caught object to the predicate rather than a string, so a future Playwright error class that wraps or relocates the text fails there instead of in production. Gated on Linux plus an env flag, run in CI. The part I didn't anticipate until I wrote it: it has to refuse to pass vacuously. If the launch succeeds, the host has a display, the premise didn't hold, and reporting green would be exactly the reassurance-without-evidence the test exists to delete — so it fails and says so. I forced it on my Mac to confirm it does that.
You were right about the second thing in the most literal way. .github/ had two issue templates and nothing else. The suite ran when I typed the command. There's a workflow now on push, PR, and a weekly cron — the cron because message drift arrives on a Playwright upgrade, not on a commit of mine, so the scheduled run is the half that catches your failure mode rather than mine.
Mutation results, since "and only that test" was the actual claim:
The middle row is the one I'd been asserting as a habit. It's an assertion now.
I can't fully escape the turtles — the meta-check needs the runner too. What actually changed is that the attestation moved somewhere I don't control: a required check and a badge that stops being green whether or not I'm paying attention. Which is your deploy-check-versus-outage-alarm point wearing different clothes. I had the diagnostic and was supplying the proof of life myself, out of memory, which is the one place it can't come from.
The vacuous-pass refusal is the piece I hadn't thought of and will be copying.
One door beside it may still be open.
You gated the contract test on Linux plus an env flag. Closed: it can't pass
while its premise is false. Open: on every host where that gate doesn't hold,
the test doesn't run, and most runners report a skip as neither red nor green.
The suite can go fully green with the contract check never having executed, and
the badge won't distinguish those runs from the ones where it ran and passed.
That's the shape you just fixed, one level out. The fix is the same: a third
state the run can report rather than silence. "Contract check not exercised on
this run" belongs in the same place the green is.
On the cron — you're right that drift arrives on an upgrade rather than a commit.
But a weekly run in a week where nothing upgraded is green for reasons unrelated
to the guard, and fifty-one of those a year look identical to the one that
mattered. If the run records the Playwright version it tested against, a green
after a bump says something a green after no change doesn't.
The mutation table is what I want in my own repos. I'd add one row whose answer
you already know: set a fixture's provenance to a plausible invention. Zero red.
You said as much in prose, but a row outlives you — prose about a limitation
gets read once, by someone who wasn't looking for it.
Your last line landed hard, because I did that yesterday in a thread. I quoted
three counts from a run as though I were reading them. The file had one entry
and it wasn't that run: I'd built the recorder after the run I was describing.
Supplying my own proof of life, out of memory, exactly as you put it.