Classify whether the app has a server side, from positive signals only - #148
Conversation
Step 2 on the evidence layer: `serverSurface` reports `server-runtime-detected`, `static-build-detected` or `unknown`, each with the signals that produced it. Why it is worth answering: an app with no server runtime cannot run a request guard, so its advisories are dependency and bundle hygiene rather than request-path risk. Told plainly that is useful. Told wrongly it is "nothing to protect here" on an app nobody could read, which is the worst thing this analysis can emit — so the static state is the one that has to be hard to reach. Three rules keep it from becoming the default for an unrecognised stack: 1. A static generator must be NAMED. Vite, CRA, Gatsby, Parcel, the SvelteKit static adapter, Astro with no SSR adapter, and Next only when it actually exports statically (a `next export` script or `output: 'export'`, read textually — parsing a config would mean executing it). 2. A server-framework dependency blocks it. `express` installed with no endpoint parsed is a parsing gap, not a static site. 3. Any deployment shape blocks it, including a `layout` one. A root `api/` holding source may be a front-end helper or a pile of platform functions, and from here they are the same folder — that belongs in `unknown` rather than in either claim. A `layout` shape also cannot produce `server-runtime-detected`, which is what the evidence strengths from the previous change exist for: the classifier consumes the distinction instead of re-deriving it. Every state carries its own caveat in the notes, in all three cases. `static-build-detected` says it is not deployment attestation — it describes the source, and a function added at the platform level never appears here. `unknown` says it must not be read as "no server side", and names why: an unparsed framework produces no endpoints, and entry-point recognition has no completeness flag. Mutation-testing found a hole in my own tests rather than in the code. Treating plain `next` as a static generator changed no state, because the server-dependency rule blocks it independently — so that rule was untested, not redundant. The evidence list is consumer-visible, and a `static-generator` signal inside a state that says otherwise invites exactly the wrong displayed reason, so it is now asserted directly. 17 tests here, full suite 1176 passed, typecheck and capability contract clean. Additive, so still version 3.
|
Well-structured server-surface classification added with clear documentation and exhaustive tests. 🎯 Quality: 100% Elite · 📦 Size: Large — consider splitting if possible 🛡️ Standards: no pre-flight fit check ran for this change — wire 📈 This month: Your 87th PR — above team average · Averaging Excellent |
Four review findings, and the first two were wrong in the direction that loses protection.
**A platform config alone no longer means a runtime.** A static site on Netlify has a `netlify.toml`; a
static Vercel project has a `vercel.json`; a Pages project serving only assets has a `wrangler.toml`.
Reading any of those as a server would have classified a large share of purely static apps as having one.
`server-runtime-detected` now requires something that SERVES: a recognized endpoint, a worker entry
(`_worker.js` — that file IS the server), or a provider function directory holding source. A config stays in
the evidence and still blocks the static claim, because a project that deploys somewhere is not one this
analysis can call server-free — but on its own the answer is `unknown`.
The evidence vocabulary carries that distinction now, since it is the classifier's input: `runtime-entry`,
`deployment-config`, `layout`. The rename beats layering a second taxonomy over `config` /
`provider-directory`, which described where an artifact was found rather than what it proves.
**The Next config is parsed, not pattern-matched.** The regex accepted `// output: 'export'`, the same words
inside a string, and an example object nobody exports — each reclassifying an ordinary server-mode Next app
as static. Comments never reach an AST, a string literal is one token, and only a value reachable from the
module's export counts, so dead code is ignored too. Wrapper (`withPlugins({...})`) and named-variable forms
are followed, since that is how real configs are written. Parsing is not executing: the config never runs.
**A real static SvelteKit project is no longer permanently unknown.** Every one ships `@sveltejs/kit`
alongside `@sveltejs/adapter-static`, and kit was an unconditional server dependency — so the adapter's
signal could never win. The veto is now conditional, and the test that appeared to cover this installed the
adapter with no kit, which is not a package set anyone ships.
**Vite alone is not a static build.** It underpins SSR stacks, so an SSR companion (vike, vite-plugin-ssr,
the React Router server packages) vetoes the static reading — conservative in the direction this state's
product meaning requires.
23 tests here, full suite 1183 passed. Mutation-checked: restoring the any-shape rule, the regex, the
unconditional kit veto, and dropping the companion veto each fail their own assertions and nothing else.
|
All four fixed in P1a — config alone is not a runtime. You are right that this is the common case rather than an edge: a I renamed the evidence vocabulary rather than adding a check on top of it: Config-only controls added for all three platforms, plus one asserting the config stays visible while the P1b — the config is parsed now. Comments never reach the AST, a string literal is one token, and only a P2 — SvelteKit. Fixed, and your diagnosis of the test was the useful part: it installed the adapter with Vite. Agreed, and taken as you framed it: SSR companions (vike, vite-plugin-ssr, the React Router server 23 tests here, full suite 1183. Mutation-checked: restoring the any-shape rule, the regex, the unconditional One consequence worth naming for step 3: |
…orever **Supabase shared code is no longer a runtime.** Any source one level under `supabase/functions` counted, but the platform's own convention puts code shared BETWEEN functions in `supabase/functions/_shared/` and does not deploy it — so a project with that directory full of TypeScript and nothing else was reported as serving requests. This is the signal the product messaging leans on, which makes a false positive here a wrong statement to a customer. The check now requires the layout Supabase actually deploys: a child directory not starting with `_`, holding an `index.*` file. A loose file directly under `functions/` no longer qualifies either, because that is not a deployable function. **Alias following is cycle-safe.** `const a = b; const b = a; export default a` recursed until the stack gave out. The surrounding catch turned that into a conservative `unknown`, but a malformed config should be an ordinary answer rather than an exception used as control flow — and a thrown RangeError discards any signal found before it. A visited set of names ends the walk; a legitimate alias chain still resolves, which is what the new test pins. Mutation testing found my test data too weak before it found anything wrong with the code, for the second time on this branch: the first `_shared` case used `cors.ts`, which the `index.*` requirement already rejected, so it passed without exercising the underscore rule at all. `_shared/index.ts` — an ordinary barrel file, and the layout that makes the rule matter — is the case that does. Full suite 1186 passed. Mutation-checked: accepting any source under `functions/`, dropping the underscore skip, and breaking alias following each fail their own assertions and nothing else.
|
Both fixed in Supabase. The check now requires the layout the platform actually deploys — a child directory not Mutation testing then caught my test data rather than the code, which is worth recording because it is the Cycle safety. Visited set of names, so Full suite 1186. Mutation-checked: accepting any source under |
The boundary check ran, but it ran too late. `qualifies` was evaluated as its own statement before the `isDirectory() && inProject() && qualifies` test, so a symlinked `api/` or `functions/` pointing outside the project was walked on every scan and the result discarded. Nothing showed in the output, which is what made it survive review of the previous fix — and "we look and throw it away" is not the guarantee. It also contradicted the helper's own docblock, which says its caller has already confirmed containment. Containment is now settled first, and the contents are only inspected for a directory that passed. The control took two attempts, and the first one was worthless in a way worth recording. It filtered recorded reads by the symlink's TARGET path — but the code reads the link path (`<project>/api`) and the kernel resolves it, so the target never appears in a read call and the assertion would have passed against the unfixed code too. It now asserts on the link path, and the `followOutside` half proves the control can observe a read at all: a control that cannot see the behaviour it forbids is not a control. Reads are recorded through a delegating `node:fs` mock, since an ESM module namespace cannot be spied and the property under test is not visible in the return value. Full suite 1187 passed. Mutation-checked by restoring the original ordering: the new control fails, and nothing else does.
|
Fixed in The control took two attempts, and the first was worthless. I filtered recorded reads by the symlink's Reads are recorded through a delegating Mutation-checked by restoring the original ordering: the new control fails, nothing else does. Full suite Step 3 noted, and I'll build One wording consequence I'd flag now: since |
|
/review |
ENG-3630 step 2, on the evidence layer from #147.
serverSurfacereports one of three states with thesignals behind it.
Why the static state is the one that has to be hard to reach
An app with no server runtime cannot run a request guard, so its advisories are dependency and bundle
hygiene rather than request-path risk. Said plainly, that is genuinely useful to the owner. Said wrongly, it
is "nothing to protect here" on an app nobody could read — the worst output this analysis can produce.
So
static-build-detectedneeds a generator NAMED, and three things block it:endpoints: []is exactly what an unparsed framework looks likeexpressinstalled with no endpoint parsed is a parsing gap, not a static sitelayoutincludedapi/holding source may be a front-end helper or platform functions; from here they are the same folderAnd a
layoutshape cannot produceserver-runtime-detectedeither — which is what the evidence strengthsin #147 were for. The classifier consumes that distinction rather than re-deriving it from folder names.
Generators identified: Vite, CRA, Gatsby, Parcel, the SvelteKit static adapter, Astro without an SSR
adapter, and Next only when it actually exports statically (
next exportin a script, oroutput: 'export'in the config — read textually, because parsing a Next config means executing it).Every state carries its own caveat
Not just the good news.
static-build-detectedstates it is not deployment attestation: it describesthe source, and a function added at the platform level never appears here.
unknownstates it must not beread as "no server side", and names the reason — an unparsed framework produces no endpoints, and
entry-point recognition has no completeness flag (
importsCompletecovers the import inventory only).Your wording from the ticket is what the static note says, and it lives in
coverage.notesso a JSONconsumer gets it too.
The interesting finding is in the tests
Mutation-testing turned up a hole in my own suite rather than in the code. Treating plain
nextas a staticgenerator changed no state, because the server-dependency rule blocks it independently — so that rule
was untested rather than redundant. The evidence list is consumer-visible, and a
static-generatorsignalsitting inside a state that says otherwise invites exactly the wrong displayed reason, so it is now asserted
directly. Both other rules fail their own tests when mutated.
Verification
17 tests here, full suite 1177 passed, 1 skipped; typecheck and capability contract clean. Additive, so
still version 3 — a v3 reader ignoring the field keeps behaving correctly.
Step 3 is the platform side: display the state with its evidence, and say what it means for shielding.