fix: address PR review findings (functional bugs, swallowed errors, doc drift) - #23
Merged
Conversation
…eutils mktemp -d -t panefind fails outright with GNU mktemp, which requires at least three trailing Xs in a -t template, breaking prefix+F on the Linux boxes. Spell out the full path with a XXXXXXXX suffix instead.
…ackup Any nonzero ssh exit fell through to "a backup or restore is running", so a dead NAS (ssh 255) looked like an active job. Treat rc >= 2 as a transport failure and die loudly with the ssh exit code and output; only the remote command's own exit 1 means a job is running.
…tate cmd_cancel ignored the ssh exit status, so a failed cancel ran straight into cmd_running and misreported whatever it saw as a completed cancel. Die on a nonzero exit instead.
… endpoint resolveAuth falls back to an "openai" provider key, but the fetch still POSTed everything to the Codex-only chatgpt.com endpoint, which rejects plain API keys. Send non-codex auth to api.openai.com/v1/responses — the body shape is identical.
Both window-usage extensions caught every fetch error silently, so a dead token or an API change just made the status vanish. Log the first failure of a streak (and the recovery) to console.error with an extension-name prefix; stale generations still stay quiet.
readStoredCredential swallowed every read/parse error, so a corrupt auth.json looked identical to no credential at all. Narrow the catch to ENOENT and let anything else propagate to the fetch-failure logging.
…ropping them Failures were fully discarded, so an offline coop silently kept its old theme with no trace anywhere. Append timestamped failures (with the ssh exit code) to ~/.cache/appearance-push.log, trimmed to ~50 lines; stdout/stderr stay suppressed.
The sed-failure branch cleaned up the temp file and said nothing, leaving btop silently on a stale theme.
…eb_fetch The catch fell back to tag-stripped text with no indication that formatting was lost. Prepend a notice so the degraded output is recognizable.
…e.json The header claimed we avoid adding a package.json to this repo, but one now exists (marking .pi-agent ESM). Reword to describe its actual role: type marker only, no pinned dependencies.
README quotes the measured figure (716→602ms on macOS, 738→616ms on Linux) but bin/pi-bundle and mise.toml still said ~150ms.
The sketch predated the boot timer and bypass marker, and still showed numbered ahead/behind icons the p10k-lean format never renders. Also fix "only the extension-status row uses the theme": the dim theme foreground is shared by the boot timer and peer session name.
…e the failure The bare catch turned a user cancel or fetch timeout (AbortError) into degraded unformatted output instead of propagating the cancellation, and the banner never said why pandoc was skipped. Rethrow aborts; include the error message in the fallback banner.
… sudo Exit 1 from the is-running probe was read as "a backup or restore is running", but sudo also exits 1 when the remote command fails — and cmd_cancel's safety re-check depends on that answer being truthful. A genuine answer is silent (the idle case prints nothing either), so exit 1 with output now dies loudly; only silent exit 1 counts as running.
…de as "running" die exited 1, the same code the running subcommand uses to report an active job — a scripted caller couldn't tell a failed probe from a backup in progress. die now exits 2 and the usage text documents the running subcommand's full exit-code contract.
…r and record ssh's reason Three problems in the new logging: tail of a not-yet-existing log made the write group exit nonzero on the very first failure, so the log was never created; a missing ~/.cache failed the redirect silently; and racing toggles shared one tmp file name. mkdir -p the log dir, suffix the tmp file with $$, warn loudly if recording itself fails, and fold ssh's last stderr line into the record so a 255-auth vs 255-timeout is distinguishable.
An unset PANEFIND_DIR would turn the EXIT trap into rm -rf with an empty argument and the next line would write to /mode. Die with a message instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes from the post-merge review of the previous changeset, verified against HEAD before each fix. Organized by review priority.
Priority 1 — functional bugs
-ttemplates; prefix+F was broken on Linux boxes. Now uses${TMPDIR:-/tmp}/panefind.XXXXXXand bails cleanly if mktemp fails.api.openai.com/v1/responses.Priority 2 — swallowed errors
~/.cache/appearance-push.log(≤50 lines) with the last stderr line, so 255-auth vs 255-timeout is distinguishable. Also fixes a bug where the log could never be created on first failure (tail of nonexistent file broke the write group).Priority 3 — comment/doc reconciliation
Review round 2 hardening
dieexits 2 so scripted callers can tell check failure from job state; usage documents the contractTest plan
bash -non all touched shell scriptsbin/pi-ext-check(typecheck + tests) green after every TS change--is-backup-restore-runningidle case (rc 0, silent)