Repository navigation
refactor: full-repo audit — correctness fixes and waste removal - #59
Merged
Merged
Conversation
Replace eleven private isRecord copies (two of which accepted arrays) with one non-array guard in src/guards.ts. Every former array-accepting call site goes on to require named fields, so accept/reject outcomes are unchanged; an array workflow meta now reports "meta export must be an object" instead of "meta.name must be a string". Also route the gh runner's error text through unknownErrorMessage.
Five private bounded* helpers and two inline variants each re-implemented the same cut. They now call truncateText from src/text.ts with their existing limits. The retry-reason log line and the inline workflow preview keep their caps, but a cut value now fills the whole cap.
FINGERPRINT_EXCLUDED_RELATIVE_PATHS moves to tree-fingerprint.ts and replaces the identical tool-source set. resume-context's untracked-path pre-filter and worktree's snapshot filter now use the exported isExcludedDeclaredInput instead of private copies of the same rule. parseGitTopLevel and portableRelativePath also live there once instead of being duplicated across resume-context, worktree and replay-path-identity.
usage.ts had its own formatUsageCount, so the widget footer could show the same magnitude as "12k" in the usage line and "12.3k" in a counter beside it. Both now use formatCount from src/text.ts. Millions use "M", the usage line's existing suffix, so they are not confused with the "m" minutes that formatDuration renders in the same widget.
engine.ts and the result renderer each formatted perf aggregates; both now use formatPerfSummary in perf.ts, so the progress log line matches the rendered "Perf:" detail line. The dropped count > 0 filter was dead: aggregates only exist for names with at least one sample. index.ts, background-workflows.ts and the renderer each re-derived a result's summary; they now share workflowResultSummary from workflow-execution.ts and keep their own fallbacks.
Schema agents have thrown WorkflowStructuredOutputError instead of returning null since v0.12.0, so a null journal entry (legacy or corrupted) is never a legitimate structured result. Let Value.Check decide, so such an entry misses the cache and the agent runs live instead of handing null to the workflow.
BoundedFingerprint wrote raw file bytes between NUL separators, so a file whose content embedded the serialization of the next entries hashed the same as the two-file tree. Hash each file into a fixed-length digest before it enters the outer hash; the shared byte budget still counts raw content. Fingerprints of file-backed sources and inputs change once. Stored identities that no longer match fail closed: journal lookup reports a mismatch and the agent runs live, and a background resume refuses the changed source.
Discovery, workflow resume revalidation and tool-source identity each kept their own copy of the 32 MiB / 4096-file source-tree bounds. Built-in workflow provenance captured at discovery must equal the resume-time capture, so the copies were coupled only by convention. Capture source trees through one captureSourceTreeFingerprint helper beside the shared exclusions.
agentJournalKey and hashAgentCall had no production callers and turned an unverifiable capture into a random key, so the key-sensitivity notEqual assertions would still pass if a variant silently stopped canonicalizing. Delete both wrappers and assert through the production capture, failing the test unless the key is verified.
validateRepositoryAfterCleanup compared the whole resume identity against a copy of itself with the repository swapped in, canonicalizing the session and skills twice each to learn nothing. Extract repositoryMismatchReason and call it directly. Drop the evidence-kind mismatch branches, which agent-attempt makes unreachable by construction, and fold the single-use mutation-guard check into validateReplayEvidence. Also remove pass-through seams in resume-context: createAgentResumeContext and VerifiedAgentResumeBaseContext (build the identity literal instead), unverifiableRepositoryResumeContext (return the capture), and the captureGitVisibleStateUnchecked split (one function owns its try/catch).
…ture - canonicalizeIdentity: drop the never-passed options parameter and read its byte, depth and node bounds directly. - captureDeclaredInputFingerprint: drop the never-set shallowDirectories mode. - agent-session-identity: forward ToolSourceIdentityOptions instead of a renamed copy remapped field by field, drop IdentityComponents and the unused type re-export, and reuse tool-source-identity's nonEmptyString. - tool-source-identity: resolveDeclaredPath was resolve(sessionCwd, path) and uniquePaths a single-use Set spread, so inline both.
A schema agent() resolves to validated output or rejects; parallel null slots come from the combinator, not from the thunk's agent() call. Remove the unreachable scope/plan/result guards in diagnose, perf-review, refactor-scout and research, and the diagnose test that scripted an off-contract null scope.
Synthesis was awaited bare, so a recoverable failure rejected the run and discarded every verified claim; the fallback report was unreachable. Run it through parallel() so the fallback applies. Gather now uses settled parallel: failed lanes are listed as a limitation, and a run whose lanes failed no longer reports a lack of evidence.
No code read the laneId each gather agent had to echo, and the sanitized lane gaps were discarded; buildClaimCandidates now sanitizes evidence directly. The lane and query caps are already enforced by the plan schema, so normalizeResearchLanes no longer repeats them.
perf-review and refactor-scout stripped --challenge[=N] for the scope target but rebuilt the verbatim user-instructions section from the raw args, so every finder, verifier and challenger prompt saw the control flag as a user instruction. Use the parsed args instead.
…ge refutations diagnose, perf-review and refactor-scout each copied the same tail: write- once counters, a makeStats closure, the kept filter, progress, the empty report, ranking, the ranked-findings block, synthesis and finish. Move it into concludeLensReview; each workflow keeps its scope, lenses, prompts and rank function. stats.refuted now comes from the final records, so findings the challenge stage refutes are counted; the verify-only refuted counter plumbing is gone.
Final finding confidence is derived from verdicts, so the optional verifier confidence was requested from every verifier, stored on the record and then ignored. Remove it from the verdict schema and the verified record.
…lizePath AdvisoryStageCoverage restated the coverage entry already defined in AdvisoryReportSchema; derive it so the two cannot drift. candidateDedupKey inlined normalizePath because the helper lived in a module that imports advisory-evidence; move it down and re-export it for existing importers.
runBoundedProcess decoded every pipe chunk on its own, so a multi-byte character split across chunks became U+FFFD. Large worktree and review diffs with non-ASCII text came back corrupted and could apply wrong bytes. Each stream now keeps a StringDecoder that is flushed on close.
When git worktree add itself failed, the registry still registered the path and its removal failed with 'is not a working tree'. The required worktree-cleanup finalizer then retried that removal and turned a run that had handled the agent failure into a failed run. Paths that do not exist after a failed add are no longer registered; failures after git created the worktree are still removed.
fs.cp resolves symlink targets by default, so the unborn-repo snapshot fallback rewrote a relative link into an absolute path inside the user's checkout. An isolated agent writing through that link modified the real working tree. Copy links verbatim, as a git checkout does.
Resuming a provider-limit pause starts a new run and leaves the source record paused with autoResume still set. Every later session start re-armed that source with a zero delay and relaunched it from its old journal, burning quota on each pi -c or /reload. A paused run that a stored run already names as its resume source is no longer scheduled, and a pending timer for it does nothing. Manual resume stays available.
diff-capture set GIT_EXTERNAL_DIFF and GIT_DIFF_OPTS to empty strings, which makes any git diff run without --no-ext-diff fail with 'external diff died'. worktree.ts already deleted them. Both now share one scrubbedGitEnv helper with the delete semantics.
Nothing imported the review-diff-target re-export block, the perf sink was never passed, and captureDiff(command) only existed so tests could pass a string. captureDiffTarget now returns the BoundedProcessResult it already produced. The non-diff executable rejection moves into the parseAllowedDiffCommand allowlist test.
Six inline catch-to-failure lambdas and nine error-or-stderr fallbacks become toGitFailure and gitFailureMessage, with every fallback message kept. The unborn snapshot receives the already-resolved timeout once, spawnGitRunner forwards runGitCommand directly, and WorktreeRegistryOptions drops the repoCwd field that only existed to be omitted. The test that only proved the patchCapture option forwards its arguments is removed; the fixtures exercise that seam on every isolated agent test.
…eping The coordinator's forced stop and the controller's paused-run stop built the same stopped transition with the same usage fallback; both now call stopWorkflowRunRecord. The background start promise uses Promise.withResolvers instead of a flag guarding settle-once semantics, the settlement hook holds its one listener directly, relaunch reads the text its typed result always carries without dead fallbacks or options startBackgroundWorkflowTool overrides, and the never-used transition back to queued is removed.
…ly API formatWorkflowRunHistory re-sliced records its only caller already caps, so its 'older runs hidden' branch could not run. The scheduler's has() and exported default clock existed only for tests; those tests now observe re-arming and the fake clock's pending timers instead.
save() removed the temp file in a finally block, so every successful progress write paid for an unlink of a file rename had already moved. list() and prune() also share one record-path listing now.
Renderers took name/result/usage/metadata/perf positionally through three seven-argument signatures, and index.ts unpacked the envelope field by field at four call sites. Pass one view object instead and drop the unread WorkflowExecution.metadata, which only duplicated envelope.runId.
The host model only receives the tool content, never the envelope details,
and that content kept only a string result or result.summary. A research
report or a { summary, patch } result reached the model as
"Workflow finished.". Append a pretty JSON copy of non-advisory object
results, capped at 20,000 characters with a truncation note; summary-only
results still render as the bare summary.
… into parseWorkflowMeta Replace the nested ternary that rebuilt the run outcome with a plain override, and inline the single-call persistTerminalWorkflowError as a flat if/else chain. parseWorkflowModule now delegates to parseWorkflowMeta instead of repeating its record check through a private parseWorkflowMetaObject.
Compiling the default export inside parentheses does not reject trailing
code: `async (api) => {...}, 5` is a comma expression, and
`async function run(api) {...}\n(api)` is an immediate call that starts
the workflow body unawaited while the run itself fails, leaving the body
launching agents. Both compiled; master rejected them.
The compiler now finds the function body's closing brace in the
code-only view (strings, comments, template text and regex literals
blanked) and rejects anything after it but an optional `;` and comments.
Accepted shapes are async functions and async arrows with a block body,
including destructured parameters; expression-bodied arrows are rejected
again, as on master. The parenthesised compile stays as a second check.
The mask also reads a `/` after an if/for/while head as a regex, so
`if (x) /import\(/.test(s)` is no longer flagged as an import.
…esumed Auto-resume skipped a paused run once another stored run named it as its resume source, but /workflow:runs listings, completions and the action gate still offered `resume` for it, so a user could replay the same journal again while the first resume was running or after it completed, duplicating agents and spend. A paused run is now superseded while some stored run that resumed it is queued, running, paused or completed: it lists `resumed as <run-id>`, offers no resume, and a manual resume names the superseding run. When every resume of it failed or was stopped, manual resume is offered again, since it is the only journal-replay path back. The supersession map is built from the store listing the history already loads. Automatic provider-limit resume keeps the stricter rule (any resume at all spends the pause): its attempt count lives on the source record, which a resume never updates, so re-arming after a failed or stopped resume would relaunch the source on every session start.
… column Export the labelled section helper so result screens align with finding details, and let callers size the finding-row location column to the paths actually listed instead of a fixed 34-column gutter.
…ions Background delivery details now carry the completed run's retained result value and start time, so the delivery message renders like a foreground result. The host-facing text still uses the summary.
…ents Results are drawn per width instead of handing pi long strings to wrap: - a status header with right-aligned duration and compact usage that sheds cost, tokens and the severity breakdown before it would wrap; - advisory reports list findings most severe first, hide zero counts, warn with the failed stage and first gap when incomplete, and expand to finding details, coverage, funnel, follow-up and the full run id; - research reports show the answer, numbered claims with [n] citations that match a numbered source list (OSC 8 links where supported), and only the non-empty claim groups; - generic results show the summary, expanding to the remaining JSON; - background runs that ended without a result point at /workflow:runs. Footers advertise the configured triage shortcut and pi's expand key. The workflow tool row names the workflow (inline scripts by their literal meta), shows a live "running" marker, a background receipt and errors.
… split detail pane The findings viewer now draws through the shared kit: a rounded frame with the title and counts in the top border and key hints in the bottom one, rows from renderFindingRow (most severe first) with a cursor and selection mark, and renderFindingDetail beside the list from 96 columns or below it when narrow. The overlay is sized to its content and keeps one height while the cursor moves; scroll hints appear only when content is clipped. Enter now toggles the selection like space; the collapse toggle, digit jumps and range indicators are gone. Patch preview and PR comment are gated on a usable selection, and the warning replaces the hint line.
/workflow with no name now opens pi-tui's SelectList with pi's select-list theme in the TUI, so names and descriptions align like the slash autocomplete. Registered workflows come first in discovery order and the author option is the last row (+ author a one-off workflow); a title and key hints frame the list. Hosts without custom components keep the native select. Dismissing the picker or one of its follow-up prompts is a cancel and no longer prints the usage warning; usage is shown only when there is no UI.
/workflow:runs in the TUI opens a framed runs browser instead of two native selects: aligned rows (state glyph and word, workflow, duration, age, cost when known, short run id) above a details pane with the facts the row does not show (wall-clock times, phases, agents, tokens, resume lineage, pause details, outcome, and why a relaunch is unavailable). enter/i inspect, r resume, s stop (pressed twice) and R restart act on the cursor run, gated by availableWorkflowRunActions; the footer lists only the valid actions and an unavailable key explains itself in place of the hints. The listing refreshes while a run is queued or running, and the browser reopens on the same run after an action. RPC keeps the native selects and headless sessions keep the text listing.
…rt paths Profiles render as aligned profile/model/thinking/source columns, the source names the config scope instead of repeating its path, and the two config paths follow with the home directory as ~ and the project file relative to the cwd, so the notification no longer wraps mid-path.
Every run's details are measured on each render to keep the overlay height stable, so a large stored result was serialised and wrapped for every row on every key and refresh tick. Outcomes are now computed once per record and capped before wrapping.
The inspector now uses the shared kit frame: workflow name in the top border with the run state on the right (running phase and progress, or completed/failed/stopped with failures and a short run id), a tab bar with counts, and key hints plus a scroll position in the bottom border. Height follows content and stays stable across tabs. - Overview: chronological phase timeline with agent counts, durations and running/failed notes, then usage, the review funnel, lower-cased verdicts, non-zero counters, summary values and failed agents. - Agents: phase-grouped aligned table; enter expands the full error, last tool, model and token usage. - Findings: a candidate re-emitted under a verdict is shown once, and unverified candidates of a finished run no longer read as pending. - Logs: failures and warnings marked, run ids shortened; Result keeps JSON indentation when wrapping. - Keys: tab/shift+tab/arrows/digits, up/down/j/k, g/G, enter, q/esc. Live runs refresh once a second. Durable run state reaches the inspector through the progress source, so a reopened failed run says failed.
The workflow tool enabled the live inspector whenever a TUI existed, so a host-agent run opened a modal that took focus and stayed over the agent's reply. Tool runs now leave it closed; the widget shows progress, and the inspector shortcut or /workflow:inspector opens it on demand. --inspect remains the explicit opt-in for slash-command runs. USAGE.md documents the change and the inspector keys.
The TUI widget is now a component that fits every line to the terminal width instead of a string list pi wraps. Phases stay in chronological order: finished phases collapse to one line, the active phase lists its running, failed and done agents (a share of rows is reserved for failures), overflow reads +N more on the phase line, and the phases declared in meta.phases but not reached yet follow on one dim line. The header carries a progress bar, finished/total agents, elapsed time and compact usage; the footer shows non-zero counters with verdict labels lower-cased and the configured inspector shortcut. Logs only appear when they report a failure no agent already accounts for. The footer status shrinks to '◆ review · Find 6/10 · 12s'. RPC keeps a pre-rendered string widget.
A run started with background: true no longer draws the full live tree or a footer status. Its tracker renders one line, '◆ background · code-review 56b57550 · Find 6/10 · 12s', with failures when there are any, refreshed like the live widget and cleared when the run ends. The coordinator's separate name-only widget is removed.
The footer status reads '◆ dynamax' while sticky, '◆ dynamax · next prompt' while a one-shot waits and '◆ dynamax · this turn' while that run is active. The command usage and inspector hint are gone (the live widget names the inspector shortcut), and the status no longer repeats the running workflow's name: that run's own status sits next to it, and a workflow started without Dynamax no longer shows a Dynamax status at all.
…, which the header already states
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
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.
Full-repo audit for correctness and waste. 156 candidate findings (about 120 distinct issues after de-duplication) came from per-subsystem correctness and quality finders plus test, docs and cross-module sweeps. Each finding was checked by an adversarial verifier and then re-checked against the code before it was fixed. Findings that did not hold up, or whose fix would have added more than it removed, were skipped; they are listed at the end.
Net: 171 files, +7740 / −6516 lines (the TUI redesign below adds most of the new lines), plus about 5.4 MB of unused PNGs removed from the tree. Typecheck is clean, 582 tests pass (523 before), and
tsconfignow enforcesnoUnusedLocals/noUnusedParametersso dead code can't come back silently.Correctness fixes
Host agent and commands
summaryfield were reduced to "Workflow finished." for the host model. The built-inresearchreport and{ summary, patch }results were affected. The host now gets the summary plus bounded JSON of the result (capped at 20k characters).--concurrencyand--parallel-limitno longer take the next flag as their value, and they reject invalid values. All six numeric flags now use theoptions.tsparsers.Agent runner
agentRetriesrestart and after a schema re-prompt, so resume no longer re-runs them.read, totools: []agents. The replay workspace check now honours an explicitskills: [].Engine
done()no longer bring back the cleared widget or leave a 1s timer running.Resume and identity
nullis validated against the schema, not accepted as is.Lifecycle
git worktree addno longer registers a phantom path that then breaks required cleanup.fs.cpused to rewrite them to absolute paths into the real repo.GIT_EXTERNAL_DIFF=""breaks plaingit diff), through onescrubbedGitEnv.Review
--no-color --src-prefix=a/ --dst-prefix=b/. User settings such asdiff.noprefix,diff.mnemonicPrefixorcolor.ui=alwaysused to make findings vanish without any error.a/orb/directories.blocked, notrejected.ghresolved the PR on, so GitHub Enterprise works.Advisory and research
--challengeflag no longer leaks into "User instructions (verbatim)".Waste removed
isRecord: 11 copies.truncateText: 7 copies.isFiniteNumberandisGitObjectId.hashAgentCall/agentJournalKey,captureDiff, theBUILTIN_WORKFLOWSexports, schedulerhas(), andrunFinalizersreturn values. The tests now go through the production paths.shallowDirectoriesoption.ReviewIssue.index/workflowName, verifierconfidence, the researchlaneId, and perf samples.createRunContextand git-commit fixtures replace hand-copied setup.npm packcontents contract.plans/.package-dry-runjob; the package test runs the samenpm pack --dry-run..pi/settings.json, which pi never reads from an installed package.Behaviour changes to be aware of
--concurrency,--parallel-limitandPI_WORKFLOW_CONCURRENCYvalues are now rejected or fall back to the default, like the other numeric flags.1.2M(was1.2m, easy to confuse with minutes). The usage line readsfresh 12.3k · cache write 5k.Deliberately not done
workflow-run-background.tsintoworkflow-run-record.ts; theagent-runner.tstype façade; the type-onlyimport()cycle between the advisory modules. In each case the churn outweighs the gain..pi/settings.jsonfrom the repo: pi's project auto-discovery only applies to trusted projects, so the file may still matter for local development.Review follow-ups (code-review, unslop, thermonuclear on this PR)
Fixed:
…}, 5compiled, and…}\n(api)parsed as an immediate call, which ran the workflow body orphaned. Both are now rejected at compile time again, and theif (x) /re/lint false positive is gone.skills: []. Web-derived evidence could otherwise infer a skill and grantread.resume. It becomes resumable again if every resumer failed or was stopped. Auto-resume keeps the stricter rule, so a failed resume cannot put the source back on the timer at every session start.guards.tsadopted everywhere.concludeLensReviewshared with code-review, which now also reportsrefuted.src/git.ts);diff-capture.tsis deleted.WorkflowExecutionwrapper removed.index.tsonto one location formatter and one JSON guard.options.tswith a typed picker result.Not done:
tools: []disable prompt skill inference engine-wide. That would remove a documented feature, so the affected built-in call sites opt out explicitly instead./after a block's}as a regex. It is documented as a lint limitation.Screens (end-to-end tested in tmux)
Every screen the extension adds was driven end to end in tmux against pi 1.0.4, at 140×45 and 90×30. A dev-only faux-provider harness scripts the subagents and stays out of the repo. Each screen was then redesigned around a shared, zero-dependency visual kit (
src/ui/kit.ts). Everything renders as width-aware components, so nothing wraps.↑in ↓outusage and the configured inspector shortcut.◆ review · Find 3/6 · 3s, and a background run is one line./workflowpicker: an alignedSelectList; cancelling no longer warns./workflow:runs: a browser overlay with a details pane and gated action keys./workflow:models: an aligned table.Runtime bugs the end-to-end runs exposed (all fixed, with regression tests):
/workflowfailed with "No API key found".workflowtool, so they could start nested runs with their own concurrency cap and budget.searchtool hint matched web-search tools, so code-review agents could send repository context to them.Before/after captures of every screen: https://claude.ai/artifact/YK8jupJdfF22HkxhEtxRBy
Testing
bun run typecheck: clean, including the newnoUnusedLocalsandnoUnusedParameters.bun run test: 582 pass, 0 fail. Earlier round: 540 pass across 14 full runs and 4 concurrent runs. One earlier full run had a single unreproduced failure inworktree.test.ts("captured patches reconstruct…"); it passed 30/30 in isolation afterwards.ghon the global PATH.npm pack --dry-run: 104 files, includingassets/preview.png.