feat(pi+run): pin pi @earendil-works 0.84.3 (ModelRuntime) + run diagnosability - #120
Conversation
…nosability Assembled onto origin/main (4553fd4) — the canonical, furthest-along Bob, NOT a pre-monorepo husk. #79 deliberately re-flattened the six-package split into a single package, and main already carries every feature branch through PR #97 (flair-native identity) plus CI/dep hardening #100-#116. feat/93-94-flair-native-identity is fully superseded by main (its only delta was older .github/bunfig config). Two changes land on top: 1. pi @earendil-works/pi-coding-agent 0.79.10 -> 0.84.3. Reconciles the older #68/#69 rename (which stopped at 0.79.10, still on the AuthStorage + ModelRegistry API) forward to the 0.84.x ModelRuntime consolidation: createPiRunSession now does `await ModelRuntime.create({authPath, modelsPath})` + `modelRuntime.getModel(provider, model)`, replacing the removed AuthStorage + ModelRegistry pair. 0.84.3 (not 0.84.4) honors bunfig.toml minimumReleaseAge=7d — no hole punched in the gate. 2. run diagnosability: surface the previously-swallowed prompt() error to stderr with provider rate-limit/cap labeling, tee every session event to ~/agents/<name>/runs/<ts>.jsonl (best-effort) with a final {done,exitCode} line, and print the log path. + 3 tests. Verify: bun install clean, bun run build clean, 397 tests pass / 0 fail, bin/bob --help exit 0. pi pinned 0.84.3 across the single package.json; no @mariozechner imports remain in src/test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
tps-sherlock
left a comment
There was a problem hiding this comment.
Security review — approve with two flags to verify before/around merge.
What's here (verified against the diff):
- Run-log tee — best-effort, own-dir, safe filename:
const runsDir = join(agentDir, "runs");
mkdirSync(runsDir, { recursive: true });
const runLogPath = join(runsDir, `${new Date().toISOString().replace(/[:.]/g, "-")}.jsonl`);Filename is toISOString() with :/. swapped — no path traversal, no user input in the path. mkdirSync recursive on the agent's own dir. appendRunLog swallows all errors:
const appendRunLog = (record: unknown) => {
try {
appendFileSync(runLogPath, `${JSON.stringify(record)}\n`);
} catch {
// Best-effort: never throw from the logger (disk full, races, etc.).
}
};Good — a logging failure can't break the run.
- Error surfacing — cap-labeled, no silent swallow:
const msg = err instanceof Error ? err.message : String(err);
const isCap =
/rate.?limit|quota|\b429\b|too many requests|usage limit|capacity|overloaded/i.test(msg);
process.stderr.write(
`bob run ${opts.name}: ${isCap ? "PROVIDER RATE-LIMIT/CAP" : "run failed"} — ${msg}\n`,
);The cap regex is conservative and the label is a real diagnosability win. No secret leakage here — provider rate-limit messages don't carry credentials, and this is the agent's own stderr.
- pi 0.84.3 bump —
minimumReleaseAge=7dhonored,ModelRuntime.createawaited,allowModelNetworkdefaults false so no network catalog fetch at create time. Sound.
Two flags (non-blocking, but verify):
@earendil-works/pi-telemetry dependency. The lockfile adds pi-telemetry@0.84.3 (zero deps) pulled in by pi-agent-core and pi-ai. "Telemetry" in an agent runtime that handles auth.json/models.json is a potential exfiltration surface. Please confirm it's inert/opt-out by default and doesn't phone home with model I/O or credentials. This is the one thing I'd want a human to eyeball before this ships to prod agents.
appendRunLog({ t: ..., event }) serializes the entire AgentSessionEvent — tool-call args, results, model responses — to plaintext JSONL at rest. If any event carries a secret (API key in a tool arg, file contents, auth material), it's now persisted unredacted under ~/agents/<name>/runs/. Best-effort logging is correct, but consider a redaction pass (or at least a note that these logs are secret-bearing and should be treated like the agent's own state).
Neither blocks the diagnosability value here — the run-log and cap-labeling are genuinely good. Just close the telemetry question before prod.
tps-kern
left a comment
There was a problem hiding this comment.
Architecture Review — Approve ✅
ModelRuntime Migration
Clean migration of the 0.79.10 AuthStorage.create() + ModelRegistry.create() pair → the 0.84.x single await ModelRuntime.create({authPath, modelsPath}) + modelRuntime.getModel(). The async factory is correct (old sync constructors are gone in 0.84.x). allowModelNetwork defaults false — no network catalog fetch at startup, static built-ins + agent models.json only. The createAgentSession call correctly passes the unified modelRuntime in place of the old separate authStorage + modelRegistry params.
Run Diagnosability
Architecturally sound:
- JSONL event tee to
~/agents/<name>/runs/<ts>.jsonlwith best-effort swallowing is the right pattern — a logging failure must never break a run. - Error surfacing replaces the old
catch (_err)swallow. The regex-based cap detection (/rate.?limit|quota|\b429\b|...) is pragmatic and low false-positive risk in practice. {done, exitCode}sentinel infinallylets a reader distinguish a clean completion from a truncated log. Good.appendFileSyncper event is synchronous but bounded to single JSON lines under a single prompt exchange — acceptable for the ephemeralbob runpath. If this ever moves to the persistent path, consider switching to a write stream or batching.
Tests
3 new tests are well-structured — error+log on death, cap labeling, happy-path byte-identical capture. The readRunLog and captureStderr helpers are clean and reusable.
Minor Notes (non-blocking)
- No log rotation in
runs/— ephemeral path only, operator can clean. Fine for now. - The regex cap-detection could theoretically false-positive on a model named "capacity" — negligible risk.
CI all green (397 tests, build, audit, CodeQL, Socket). Minimum release age honored (0.84.3, not 0.84.4).
Ship it. 📐
What
@earendil-works/pi-coding-agent0.79.10 → 0.84.3 — the 0.84.x ModelRuntime consolidation: the oldAuthStorage+ModelRegistrypair collapses into a singleawait ModelRuntime.create({authPath, modelsPath})+modelRuntime.getModel(provider, model)(models.json-aware, no network).src/shell/run.ts, ephemeralbob runpath only): surface the previously-swallowedprompt()error to stderr with provider rate-limit/cap labeling, and tee every session event to~/agents/<name>/runs/<ts>.jsonl(best-effort) + a final{done,exitCode}line + print the path.Why
bob runthat died left no trace — in-memory session + a swallowed error meant an ollama cap looked like a silent clean exit, undiagnosable. Now every run leaves an event-log and a cap is labeled loudly. (This is what turned a black-box builder death into a precise diagnosis in practice.)Verification
bun installclean; pi confirmed 0.84.3; zero@mariozechnerimports in src/test (comments only).bun run buildclean;bun run test397 pass / 0 fail (incl. 3 new run-log / error-surfacing tests + the live pi-0.84.3 e2e).node bin/bob --helpok.bunfig.tomlminimumReleaseAge=7dhonored — 0.84.3 is policy-eligible; 0.84.4 (2.8d old) deliberately NOT pinned.Ephemeral run path only; persistent path +
createPiRunSessionconfig unchanged. Dogfoods "The Repo Bar" (STANDARDS): landing onmainthrough CI + K&S, not a local-only branch.