Add automated templates health check - #420
Conversation
Greptile SummaryThis PR adds an automated health check for templates. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (9): Last reviewed commit: "fix: only skip credential-caused failure..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
This pull request has been automatically marked as stale because it has not had any activity for 7 days. It will be closed in 7 days if no further activity occurs. If you believe this PR is still relevant, please add a comment or push new commits to keep it open. Thank you for your contributions! |
fc8679a to
3933d5c
Compare
Adds `pnpm health`, a script that checks every template and reports across five dimensions: build (install plus each template's ci or build script), Rust compile (cargo check or build, with --cargo-test for the full suite), dependency freshness, npm audit, deprecated direct deps, and doc drift. Only build, Rust, and boot can mark a template "fail". Outdated deps, audit advisories, and deprecations cap at "warn" so the tool does not duplicate dependabot or flag a working template as broken. Templates that need setup or credentials are marked "skip", and functional failures are retried once in isolation to tell flaky apart from real. Each template installs in an isolated temp dir using its pinned package manager, and temp dirs are swept on exit so artifacts never pile up. Output is a JSON artifact plus a Markdown report under health-reports (gitignored). Adds unit tests via `pnpm health:test` with no new dependencies.
health-check.md covers the judgment layer the script does not decide: runtime correctness on devnet, whether a whole stack is still current, and how to read the report. ui-testing-plan.md records the agreed path for browser-level testing, which is the fidelity ladder, the headless wallet-standard approach, and the boot to Playwright seam, so future work starts from a clear plan. AGENTS.md links both docs and the new commands.
A dev server that boots but serves error pages was counted as healthy because any HTTP response passed the boot check. Now only successful statuses pass. Error statuses keep polling until the 60s deadline since dev servers can return 500 while still compiling, and if the server never recovers the check fails with the last status and server output recorded.
3933d5c to
13fe8f9
Compare
Tools like Next.js hard-code ANSI color sequences into their error output even with FORCE_COLOR=0 set, so build failure notes in the Markdown report rendered as unreadable escape-code soup. Strips CSI and OSC sequences in tail(), the single point every human-facing note flows through, and sets NO_COLOR=1 on spawned commands so well-behaved tools skip colors entirely.
The repo's own scripts were never type-checked: tsx strips types without checking and lint only ran the structure check plus prettier. Adds tsc --noEmit to pnpm lint, which validate-templates already runs on every PR, so CI picks it up with no workflow changes. Fixes the existing errors: a nullable packageManager reaching run() and spawn() in the health checks (now a ResolvedRunOptions type), result narrowing when report writes fail, four unused imports, and an ArrayBufferLike assertion in image-utils. Also declares react and @types/react at the root, which image-utils imported without any package declaring it.
| const deadline = Date.now() + 60_000 | ||
| const kill = () => { | ||
| try { | ||
| child.kill('SIGKILL') |
There was a problem hiding this comment.
This only kills the direct package-manager process from run dev. The shell and the actual next dev or vite child can keep running after the boot check returns, so later template checks can see ports held by an older template server. When that stale server responds successfully, the boot result can be based on the wrong template instead of the one currently being checked. Please terminate the full process tree, for example by starting the dev command in its own process group and killing that group in finally.
dev-jodee
left a comment
dev-jodee
left a comment
There was a problem hiding this comment.
Some couple of potential issues + we need a rename of all single letter variables, functions, etc. :)
| const r = await run(pm, args, workDir, 120_000) | ||
| let parsed: Record<string, { current?: string; latest?: string }> | ||
| try { | ||
| parsed = JSON.parse(r.output || '{}') |
There was a problem hiding this comment.
r.output contains both stdout and stderr, so I think this can crash, and then the try catch will hide the issue? (same for audit section below)
| child.stderr.on('data', cap) | ||
| const timer = setTimeout(() => { | ||
| timedOut = true | ||
| child.kill('SIGKILL') |
There was a problem hiding this comment.
This will only kill the direct process (npm or pnpm or wtv package manager), but if that process spawns other children (like vite serve, cargo, dev servers, etc.) they won't get killed. I think a possible solution is to spawn it detached and then sigkill the process group instead of just the direct child. (same for the kill a couple hundred lines below too
| const envExample = join(dir, '.env.example') | ||
| if (existsSync(envExample)) { | ||
| const body = readFileSync(envExample, 'utf-8').toLowerCase() | ||
| if (SECRET_HINTS.some((h) => body.includes(h.replace(' ', '_')) || body.includes(h))) { |
There was a problem hiding this comment.
this will "skip" if i.e. a .env.example file has secret in a comment (instead of an actual env missing), think we'd need to parse line by line and check for actual _KEY= or something
| import type { OutdatedDep } from './types.js' | ||
|
|
||
| /** Parse a version like "^1.2.3" / ">=2.0.0" / "1.2.3-rc.1" into [major, minor, patch]. */ | ||
| export const parseVersion = (v: string): [number, number, number] | null => { |
There was a problem hiding this comment.
in general across the pr, we shouldn't have var names that are 1 letter, if v here is version then it should be named version, same for c, l, etc. below it makes the code hard to read and doesn't really give us any advantages
Kills the full process tree of spawned commands (detached process groups) so orphaned dev servers can't hold ports and answer a later template's boot probe. Parses JSON from stdout only since stderr warnings corrupted the parse. Detects credential requirements from .env.example variable names instead of raw text so comments can't cause a false skip, with unit tests. Renames all single letter variables per review.
| export const sweepOrphanTempDirs = (): number => { | ||
| let removed = 0 | ||
| try { | ||
| for (const name of readdirSync(tmpdir())) { | ||
| if (name.startsWith(TEMP_PREFIX)) { | ||
| rmSync(join(tmpdir(), name), { recursive: true, force: true }) | ||
| removed++ | ||
| } | ||
| } | ||
| } catch { | ||
| /* tmpdir not readable - nothing to sweep */ | ||
| } | ||
| return removed | ||
| } |
There was a problem hiding this comment.
This sweep only narrows the prefix, but it still deletes every matching temp directory in the shared system temp folder. If two pnpm health runs overlap on the same runner, the later run removes the first run's active template copies because both use sf-templates-health-. The first run can then keep installing or building from a deleted working directory and report templates as broken when they are not. Use a per-run parent directory, a lock, or another ownership marker so startup cleanup cannot remove active work from another process.
| const start = Date.now() | ||
| // detached: the dev command becomes its own process-group leader, so killProcessTree in | ||
| // the finally below takes out the whole tree (pm -> next/vite -> workers), not just the pm. | ||
| const child = spawn(opts.packageManager, ['run', 'dev'], { | ||
| cwd: workDir, | ||
| env: { ...process.env, FORCE_COLOR: '0', NO_COLOR: '1' }, | ||
| detached: true, |
There was a problem hiding this comment.
pnpm health --boot still checks templates in parallel, but each dev script keeps its default port. Two web templates can start at the same time and both try to bind the same Next or Vite port. The second server exits with EADDRINUSE, and this path reports that template as a boot failure even though it would boot by itself. Assign an isolated port per template or run boot checks serially so parallel health runs do not create false failures.
|
This pull request has been automatically marked as stale because it has not had any activity for 7 days. It will be closed in 7 days if no further activity occurs. If you believe this PR is still relevant, please add a comment or push new commits to keep it open. Thank you for your contributions! |
| const CREDENTIAL_KEY_PATTERN = /(API_?KEY|SECRET|PASSWORD|CREDENTIAL|PRIVATE_?KEY|SUPABASE|PRIVY)/i | ||
| /** Names that look credential-ish but are public config (mint addresses, RPC endpoints, ...). */ |
There was a problem hiding this comment.
This still treats public provider IDs as setup credentials. For example, mobile/kit-expo-privy/.env.example uses EXPO_PUBLIC_PRIVY_APP_ID and EXPO_PUBLIC_PRIVY_CLIENT_ID; both match PRIVY here and are not excluded as public config. If that template has an unrelated build break, checkTemplate sees ref.needsSecrets && build.status === 'fail' and changes the result to skip, so the health report hides a broken template instead of failing it. Please only mark private credential names as setup-blocking, or exclude public app/client IDs such as EXPO_PUBLIC_* from this detector.
There was a problem hiding this comment.
Fixed, app, client and project ID style names are now excluded from the credential detector, real secrets like anon keys still count
| // This dominates the overall status (see toTemplateReport): we genuinely couldn't validate | ||
| // it, so it must read "skip", not get bubbled up to pass/warn by advisory checks. | ||
| let needsSetupSkip = false | ||
| if (ref.needsSecrets && build.status === 'fail') { |
There was a problem hiding this comment.
needsSecrets && build.status === "fail"does not establish that missing credentials caused the build failure; an unrelated type, dependency, or syntax error is converted toskip, and needsSetupSkipthen dominates the template status. Could we keep build failures as failures and useneedsSecrets` only to skip checks that actually require credentials?
| continue | ||
| } | ||
| if (before !== 'fail' && template.status === 'fail') regressions.push(template.id) | ||
| if (before === 'fail' && template.status !== 'fail') fixed.push(template.id) |
There was a problem hiding this comment.
skip means unverified, but this condition reports fail → skip as fixed because every non-fail state qualifies. It also leaves pass/warn → skip out of regressions. Could we treat skip as unknown: only mark fail → pass/warn as fixed, and never report a transition to skip as a fix?
…n diffs Build failures in templates that need credentials now stay failures unless the error output actually references one of the required env var names, which enumerate now exposes as credentialKeys. Re-running the check surfaced three templates whose real breakage the old skip was hiding. The baseline diff treats skip as unknown: only fail to pass or warn counts as fixed, transitions to skip surface as became unverifiable, and a skip that comes back as fail alerts as a regression.
3 participants
What this adds
A script,
pnpm health, that checks the health of every template in the repo.Why
CI scaffolds and builds the non-community templates, but the community templates are not tested at all. When a dependency update breaks one, nobody notices until a user runs into it. This script checks every template and reports what is broken.
How it works
Run
pnpm healthfrom the repo root. For each template it copies the template into a temporary folder, installs dependencies with that template's own package manager, runs its build orciscript, and collects a few extra signals. It writes a JSON file and a readable Markdown report underhealth-reports/. It does not change anything in the repo and never touches mainnet or real funds.This is maintainer tooling that lives at the repo root, so it is not included when someone scaffolds a single template with create-solana-dapp.
What it checks
ciscriptcargo checkorcargo build)npm auditA template only counts as failing if it does not build. Outdated or vulnerable dependencies show as warnings, not failures, so this does not duplicate Dependabot. Templates that need setup or credentials are skipped rather than marked as failing.
Where this is going
This is the first step. The longer term goal is an autonomous agent that does not just build each template but runs it and confirms it actually works. Planned next steps, not part of this PR: