Address review findings; bound bggrep matching - #11
Merged
Merged
Conversation
Contributor
CI report
Ref: |
lloydsk
force-pushed
the
lloydsk/Feedback-Improvements
branch
2 times, most recently
from
September 19, 2026 00:40
ffdb243 to
5d7eea5
Compare
Squashed review-fix series (was 12 commits, rebased onto main after the project-local-jobs default flip). Contents: Review fixes (Carper feedback + adversarial pass) - Exit marker argv handoff, tail-read caps (LOG_TAIL_BYTES/LOG_READ_BYTES), kill-process-group docs, worktree commondir, spawn-error handling. - redactForSlug rewritten: underscore env keys (GITHUB_TOKEN, AWS_SECRET_ACCESS_KEY, MYSQL_PWD), space-separated flags, JSON, --header=, URL userinfo, curl -u; consumes the auth scheme so a bare "Authorization: Bearer <tok>" no longer leaks. Table test. - bgstatus <id>: live child handle is authoritative; persist:false no longer leaves the transcript card stuck "running" (donePersisted). - readLogSlice honors the short-read count (no NUL padding). - cleanup: sweepStaleMarkers runs after the ownership gate and only touches .tmp-*.log; bgstatus list skips the per-file exit-marker parse when it is not listing finished/foreign jobs. - temp log opened O_EXCL + 0600 with a randomBytes name; sanitizeName strips control characters. - worker matcher: any exit before a message is a failure; late 'error' swallowed. Feature - bggrep matches under a wall-clock budget in a worker (default 2s, PI_BGRUN_GREP_TIMEOUT_MS); a runaway regex errors instead of hanging. Docs / packaging / CI - README + SKILL corrected (2 MB window, 10k line pre-truncation, marker retention, PI_BGRUN_USER_CONFIG, includeDone for finished foreign logs, type shown in status, .last-clean scope); engines.node >= 20. - Tests: redaction table, bgstatus by-id branches, .tmp- sweep, pathological regex bound; digest fallthrough test no longer spawns real npm.
lloydsk
force-pushed
the
lloydsk/Feedback-Improvements
branch
from
September 19, 2026 00:56
5d7eea5 to
c9f58a0
Compare
Correctness (from adversarial review): - C1 cleanOldJobs: a live pid now protects the log unless our record knows the job finished. Previously the __BGRUN_EXIT__ marker was trusted first, so a running job whose own output contained that line had its log deleted — violating "never removes a running job's log". Regression test added. This intentionally drops the old "finished-but-reused-pid is removable" behavior, which the test that encoded it was rewritten around. - C2 session_start reconstruction now sets donePersisted, so a resumed done job is no longer re-persisted (duplicate done entry on every resume). - C3 autoCleanJobs bypasses the ownership gate for the known machine-global dir (it never gets a .bgrun-jobs marker under the project-local default), so the global orphan sweep actually runs instead of silently stamping .last-clean. - C4 a relative jobsDir that escapes the project root is no longer flagged project-local. C5 "Last output" no longer prints the exit marker for an empty-output job. C6 marker-only log reports 0 lines. C7 the exit handler early-returns when a spawn error already recorded the job (no double wake). DRY (behavior-preserving): - resolveLogForJob() single-sources the "record first, then config dir" log resolution shared by bgtail/bggrep. - projectMarkerPath() backs the two exported marker-path helpers. - resolveConfig uses projectRootFor() instead of inlining findProjectRoot(). 164 tests pass; tsc clean.
Test-only consolidation (net -194 LOC, extension/index.test.ts only):
- loadModule() replaces 31 copies of the pathToFileURL+import pair.
- withEnv(name, value, fn) replaces 5 env save/restore blocks.
- withJobsDir(fn, opts) replaces 31 exact-shape
mkTmp("pi-bgrun-test-") + PI_BGRUN_DIR + try/finally blocks.
Variable shifting sites (mkdtempSync, missing rmSync, extra env) were left
unchanged on purpose. Assert count (538) and test count (164) are unchanged;
164 pass, tsc clean.
C1 (live-pid protection) traded one documented guarantee for another: a finished log whose id-pid was later reused by a live process could never be reclaimed — not by the auto-sweep and not by explicit `bgclean all`. Resolve both by making the exit marker count only when it is the log's TERMINAL (last non-empty) line — which is exactly how the wrapper writes it: - terminal marker => the job finished; trust it over pid liveness, so a reused pid cannot pin the log forever; - non-terminal marker (a running job that merely printed the string) => not completion evidence, so pid liveness protects the log. Applied in parseExitFromContent, so cleanup, revalidateStaleJobs and bgstatus share one rule. Tests: the C1 regression now uses a non-terminal marker; a new test asserts a terminal marker reclaims a finished log whose pid is alive. Also from the review: only bypass the ownership gate for the machine-global dir when PI_BGRUN_GLOBAL_DIR is unset (a custom global dir stays gated per README); drop a duplicate validateJobId; stop treating a `..foo` sibling as an escape. 165 tests pass; tsc clean.
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
Addresses the adversarial review of
pi-background-runv0.5.0 (fb97d3e) and adds a hard bound tobggrepmatching. Branch is rebased ontomain(dd5b3e6, the digest feature); both the review fixes and the digest feature are kept.Fixes
redactForSlug) — a bareAuthorization: Bearer <token>no longer leaks the token (the auth scheme is now consumed, not justBearer).-His matched case-sensitively, sogrep -h patternis no longer mangled intogrep -h-REDACTED.bgstatus <id>— derives done/running from the log marker or pid liveness when the in-memory record has noexitCode, so by-id and the list agree without waiting for the 30s poller. Read-only (no mutation/persist).scanLogFiles(no exit parse) is split fromscanJobsDir;cleanOldJobsnow parses the marker only for files past the retention cutoff instead of tail-reading every log.LOG_TAIL_BYTES/LOG_READ_BYTES,kill -- -<pid>, exclude rules resolved viacommondir)..tmp-*.logfiles are skipped in job scans and reclaimed bysweepStaleMarkers.New: bounded
bggrepmatchingbggreptakes a caller-supplied regex, and a pathological pattern (e.g./^(a+)+$/) can backtrack catastrophically; V8 cannot interrupt a regex on the main thread, so the call could hang the session. The match loop now runs in a worker with a wall-clock budget (default2000ms,PI_BGRUN_GREP_TIMEOUT_MS); on expiry the worker is terminated and an error is returned instead of hanging. Invalid patterns still fail fast in the main thread. Falls back to a between-line-bounded sync matcher only ifworker_threadsis unavailable.Docs
.bgrun-jobs,.last-clean,.bgrun-used-<hash>,.digest-nudge-<hash>) plus the fact that the ownership gate applies only to the automatic sweep, while explicitbgclean allbypasses it.### Bounded matching, env-table rows, and corrected linked-worktree wording ("common git dir", resolved viacommondir).@types/nodebumped^22→^24;npm deprecatestep dropped (OIDC cannot deprecate).Verification
bun test— 129 pass, 0 failbun run lint(tsc --noEmit) — cleannpm pack --dry-run— 7 files (test file excluded)Notes
LOG-SIZE-CEILING.md.