Skip to content

fix: add structured debug logging to silent catch blocks in update-check #229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iamyhe wants to merge 2 commits into
base: main
Choose a base branch
from
+30 −20
Open

fix: add structured debug logging to silent catch blocks in update-check #229

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f333703
fix: add structured debug logging to silent catch blocks in update-check
iamyhe Jul 7, 2026
330e17f
refactor: extract isDebugLoggingEnabled helper based on review
iamyhe Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 30 additions & 20 deletions src/lib/update-check.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,25 @@
* result cached on disk, advisory printed to stderr so stdout stays parseable.
*
* Behavior (`maybeNotifyUpdate`):
* 1. Gate through `shouldCheckForUpdate`; every gate below must pass.
* 2. Probe the npm registry for the `latest` dist-tag (1.5s hard timeout).
* 3. Stamp the cache with `lastCheckMs` (plus `latestKnown` when the probe
* succeeded) so the next 24h of invocations skip the network entirely.
* The stamp happens even after a failed probe: a dead registry must not
* trigger a retry on every command.
* 4. When `latest` is strictly newer than the running version, write exactly
* one advisory line to stderr. The function never throws or rejects and
* never alters the exit status of the command it rides along with.
* 1. Gate through `shouldCheckForUpdate`; every gate below must pass.
* 2. Probe the npm registry for the `latest` dist-tag (1.5s hard timeout).
* 3. Stamp the cache with `lastCheckMs` (plus `latestKnown` when the probe
* succeeded) so the next 24h of invocations skip the network entirely.
* The stamp happens even after a failed probe: a dead registry must not
* trigger a retry on every command.
* 4. When `latest` is strictly newer than the running version, write exactly
* one advisory line to stderr. The function never throws or rejects and
* never alters the exit status of the command it rides along with.
*
* Gates, in order (`shouldCheckForUpdate`):
* - `TESTSPRITE_NO_UPDATE_NOTIFIER` set to any non-empty value: opted out.
* Presence-style, mirroring gh's GH_NO_UPDATE_NOTIFIER: even "0" disables.
* - `CI` set to anything except the literal "false": CI logs are not the
* place for update nags. `CI=false` explicitly re-enables the notice.
* - stderr is not a TTY: piped or redirected output stays clean.
* - the on-disk cache is fresh (last probe within the TTL). A missing,
* unreadable, corrupt, or wrong-shape cache counts as stale, and so does a
* `lastCheckMs` in the future (clock rollback or corrupt data).
* - `TESTSPRITE_NO_UPDATE_NOTIFIER` set to any non-empty value: opted out.
* Presence-style, mirroring gh's GH_NO_UPDATE_NOTIFIER: even "0" disables.
* - `CI` set to anything except the literal "false": CI logs are not the
* place for update nags. `CI=false` explicitly re-enables the notice.
* - stderr is not a TTY: piped or redirected output stays clean.
* - the on-disk cache is fresh (last probe within the TTL). A missing,
* unreadable, corrupt, or wrong-shape cache counts as stale, and so does a
* `lastCheckMs` in the future (clock rollback or corrupt data).
*
* Why not the npm `update-notifier` package: this CLI's runtime dependency
* budget is commander + valibot only (package.json). `update-notifier` would
Expand Down Expand Up @@ -117,6 +117,10 @@ function resolveUpdateCheckDeps(deps: UpdateCheckDeps): ResolvedUpdateCheckDeps
};
}

function isDebugLoggingEnabled(argv: string[] = process.argv): boolean {
return argv.includes('--debug') || argv.includes('--verbose');
}

/**
* Read and validate the cache file. Every failure mode (missing file,
* unreadable file, invalid JSON, wrong shape) returns undefined, which the
Expand All @@ -128,7 +132,10 @@ function readUpdateCheckCache(resolved: ResolvedUpdateCheckDeps): UpdateCheckCac
const body: unknown = JSON.parse(raw);
const parsed = v.safeParse(UPDATE_CHECK_CACHE_SCHEMA, body);
return parsed.success ? parsed.output : undefined;
} catch {
} catch (err) {
if (isDebugLoggingEnabled()) {
resolved.stderr(`[debug] readUpdateCheckCache: ${err instanceof Error ? err.message : String(err)}\n`);
}
Comment on lines +135 to +138

@coderabbitai coderabbitai Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect resolveUpdateCheckDeps / default stderr implementation for newline handling
rg -n -B2 -A8 'resolveUpdateCheckDeps|stderr:' src/lib/update-check.ts

Repository: TestSprite/testsprite-cli

Length of output: 2763

🏁 Script executed:

#!/bin/bash
sed -n '130,160p' src/lib/update-check.ts
printf '\n---\n'
sed -n '280,295p' src/lib/update-check.ts

Repository: TestSprite/testsprite-cli

Length of output: 2016

Drop the explicit \n in the debug writes. resolved.stderr already adds a newline, so the debug messages print with a doubled blank line. Keep those strings newline-free to match the advisory call.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/update-check.ts` around lines 135 - 138, The debug write in
readUpdateCheckCache is appending an explicit newline even though
resolved.stderr already terminates the message, causing double spacing in debug
output. Update the debug logging strings in this catch block (and any similar
resolved.stderr calls in update-check.ts) to be newline-free so the output
formatting matches the advisory path.

// Missing or unreadable cache: treat as stale.
return undefined;
}
Expand All @@ -143,7 +150,10 @@ function writeUpdateCheckCache(resolved: ResolvedUpdateCheckDeps, cache: UpdateC
try {
resolved.mkdir(dirname(resolved.cachePath));
resolved.writeFile(resolved.cachePath, `${JSON.stringify(cache)}\n`);
} catch {
} catch (err) {
if (isDebugLoggingEnabled()) {
resolved.stderr(`[debug] writeUpdateCheckCache: ${err instanceof Error ? err.message : String(err)}\n`);
}
// Cache persistence is optional; never surface fs errors to the command.
}
}
Expand Down Expand Up @@ -282,4 +292,4 @@ export async function maybeNotifyUpdate(deps: UpdateCheckDeps = {}): Promise<voi
// An update notice must never break, delay, or alter the exit status of
// the command it accompanies. Swallow everything.
}
}
}