From 5b7796043e5f63bacc37ec6902ea02d7115aca97 Mon Sep 17 00:00:00 2001 From: omp-port Date: Tue, 11 Aug 2026 09:19:06 -0400 Subject: [PATCH] port: sync from Mike/pygienium@595ce48f --- README.md | 204 +----------------------------- src/agent-runner.ts | 252 ++++++++++++++++++++++++++++++------- src/checks/comments.ts | 14 ++- src/modes/check-runner.ts | 18 ++- tests/agent-runner.test.ts | 137 ++++++++++++++++++++ tests/check-runner.test.ts | 27 ++++ tests/comments.test.ts | 29 +++++ 7 files changed, 433 insertions(+), 248 deletions(-) create mode 100644 tests/agent-runner.test.ts diff --git a/README.md b/README.md index b82a7f4..9d9dc9e 100644 --- a/README.md +++ b/README.md @@ -1,207 +1,11 @@ -# Pygienium +# Pygienium (omp port) -Code hygiene for [omp](https://github.com/oh-my-pi) — isolated -sub-agent checks that scan a target, apply fixes, and emit a findings + changes -report. Inspired by piolium's sub-agent loops. - -Pygienium runs **highly-structured hygiene passes** over a repo to clean up the -common quality issues LLM-generated code accumulates: restating comments, -shallow pass-through modules, dead exports/files, and redundant defensive -guards. Each check is an isolated, resumable sub-agent run whose progress lands -in a single inspectable run-state file. +Code hygiene extension for omp — isolated sub-agent checks that scan a target, apply fixes, and emit a findings+changes report. ## Install -Pygienium is a local omp extension under `~/.omp/agent/extensions/pygienium/`. -Omp loads it via `omp.extensions` in `package.json` (entry `./src/index.ts`). - ```sh -# from the extension root -bun install # optional: only needed for typecheck/test dev deps -bun run typecheck -bun test +omp install @mikefreno/omp-pygienium ``` -Omp auto-discovers the extension from this location via the `omp.extensions` entry in `package.json` — no settings.json config is needed. On load it emits a TUI notification `Pygienium loaded. Run /pygienium-help for available checks and flags.` (only when a dialog-capable UI is available). - -## Configuration - -Pygienium reads its chat-rendering style from omp's `settings.json` (`~/.omp/agent/settings.json`) under a `pygienium` key: - -```json -{ - "pygienium": { - "chatStyle": "verbose" - } -} -``` - -| Setting | Default | Values | Description | -| --- | --- | --- | --- | -| `pygienium.chatStyle` | `"verbose"` | `"verbose"` \| `"compact"` | Chat rendering for sub-agent tool calls. **verbose** (piolium-style) streams each tool event live as its own chat line (`[Comments: Scanning] → bash ...` / `← (ok)`). **compact** (ralpi-style) suppresses the per-event stream and shows only the final completion message with its expandable phase tree. | - -No entry means `"verbose"` (the default). An unreadable or missing `settings.json` also falls back to `"verbose"`. - -## Commands - -Every command accepts a `[path]` target (default: the current directory) and is -resumable — progress is persisted to `/.pygienium/run-state.json`. - -| Command | What it does | -| --- | --- | -| `/pygienium-help` | Print every command, shipped check, and flag. | -| `/pygienium- [path] [--fix]` | Run one isolated sub-agent that scans a target, applies fixes with `--fix`, and emits a findings + changes report. | -| `/pygienium-all [path] [--fix]` | Run every registered check in sequence under one resumable run-state. | -| `/pygienium-status [path]` | Show per-check progress, artifact line counts, and errors for the latest run. | -| `/pygienium-resume [path] [--fresh]` | Resume the latest in-progress/failed/partial run; complete/skipped checks skip unless `--fresh`. | -| `/pygienium-export [path] [--check=] [--status=] [--out=md\|json]` | Bundle every check's `findings.md` + `changes.md` into `pygienium/export.{md\|json}`. | - -## Flags - -| Flag | Scope | Description | -| --- | --- | --- | -| `[path]` | all check commands | Target file or directory to scan (default: current dir). | -| `--fix` | ``, `all`, `resume` | Apply fixes (default: scan-only; emits findings only). | -| `--fresh` | `resume` | Re-dispatch completed checks too — reset their run-state entries and re-run. | -| `--check=` | `export` | Comma-separated check names to include in the bundle. | -| `--status=` | `export` | Comma-separated statuses to include (e.g. `complete,failed,skipped`). | -| `--out=` | `export` | Bundle format: `md` (default) or `json`. | - -## Checks - -The five shipped checks live in [`src/checks/`](./src/checks/) and self-register -on load. `/pygienium-help` lists whichever checks are currently registered, so -this table and the live help always agree on the registered set. - -| Command | Check | Agent | What it fixes | -| --- | --- | --- | --- | -| `/pygienium-comments` | comments | `scanner` / `fixer` | Remove low-value/restating comments, tighten verbose ones, keep "why" comments. | -| `/pygienium-deep-modules` | deep-modules | `deep-modules` | Detect shallow modules (pass-throughs, trivial wrappers, re-export barrels) and consolidate the safe ones. | -| `/pygienium-dead-code` | dead-code | `scanner` / `fixer` | Find unreferenced exports, dead files, obsolete compat shims, migration helpers, and unused dependencies; remove clearly-dead items and flag dynamic ones. | -| `/pygienium-defensive-guards` | defensive-guards | `defensive-guards` | Remove redundant defensive guarding (null checks on non-nullable types, swallowing try/catch, masking fallbacks) while keeping boundary guards (IO, parsing, untrusted input). | -| `/pygienium-todos` | todos | `todos` / `fixer` | Inventory TODO/FIXME markers and stub implementations; with `--fix`, convert silent stubs (placeholder returns, empty bodies) into loud failures — never implementing TODOs or deleting markers. | - -## Artifacts - -Every check writes its reports under `/.pygienium/checks//` (run -state lives at `/.pygienium/run-state.json`): - -- `findings.md` — what the scan found (per-file line refs). -- `changes.md` — what the fix phase changed + anything deferred for human review. - -`/pygienium-export` merges every check's artifacts into one -`.pygienium/export.md` (or `export.json`). - -On first run in a git work tree, pygienium appends `.pygienium/` to the -target repo's `.gitignore` so a run never stages its own state/artifacts into -git (opt out with `--no-gitignore`). - -## Adding a check - -One file + one `registerCheck()` call. **No `index.ts` command-wiring changes.** -`index.ts` auto-discovers every `checks/*.ts` (except the registry barrel) at -startup, so a new file self-registers and `/pygienium-` appears -automatically. - -1. Create `src/checks/.ts` from the template below. -2. Edit the `name`, `label`, `description`, the rubric in the scan/fix task - builders, and the `gate` precondition. -3. Keep the trailing `registerCheck(Check)`. Done. - -```ts -import { registerCheck, type CheckScope } from "./registry.js"; - -export const myCheck = { - name: "my-check", - label: "My check", - description: "What it fixes (shown in /pygienium-help).", - agentName: "scanner", // reuse a shipped agent, or add agents/.md - fixAgentName: "fixer", - phaseId: "my-check", - buildScanTask: (_cwd: string, scope: CheckScope) => `# Task: my-check scan\n…`, - buildFixTask: (_cwd: string, scope: CheckScope, findings: string) => `# Task: my-check fix\n…`, - gate: (cwd: string) => undefined, -} as const; - -registerCheck(myCheck); -``` - -Reload omp (or `/reload`) and run `/pygienium-help` — `/pygienium-my-check` is -listed and runnable. A `CheckDefinition` supplies the task builders and gate; -the generic check-runner wires the phases together, so a new check never -touches command plumbing. - -## Architecture - -Each check is a "mode" running a fixed phase pipeline: - -``` -registerCheck(def) ← checks/*.ts self-register on load - │ - ▼ -/pygienium- ─► runCheck(def) (src/modes/check-runner.ts) - │ - ├─ Q0 recon (shared, run once per run) (src/recon.ts) - │ git state + source-file inventory → .pygienium/recon.json - ├─ analysis sub-agent (buildScanTask) ← scanner/ agent - │ writes .pygienium/checks//findings.md - ├─ fix sub-agent (buildFixTask) ← fixer, only with --fix - │ writes .pygienium/checks//changes.md - ├─ verify gate (re-runs check.gate) - └─ cleanup (drops transient scratch artifacts) -``` - -- **Sub-agents** are isolated in-memory `AgentSession`s scoped to the target - `cwd` (see `src/agent-runner.ts`), with the agent definition's system prompt - and tool allowlist applied. Agent definitions are plain editable markdown in - [`agents/`](./agents/) (frontmatter `name` + `allowedTools`, body = system - prompt) — tuning a sub-agent never needs TypeScript changes. A scanned - project can ship its own `agents/*.md` at its root: those are loaded as - overrides (repo agent wins on name collision), so teams can tune prompts or - add project-specific agents without touching the extension. -- **Run-state** is a single JSON file at `/.pygienium/run-state.json` - (`src/run-state.ts`): per-check phase progress, captured findings/changes - text, and recon status. `/pygienium-status`, `/pygienium-resume`, and - `/pygienium-export` are pure reads over it; `/pygienium-all` shares one - `RunState` across every check so phases accumulate in one record. -- **The registry** (`src/checks/registry.ts`) is the extensibility seam: a - module-level `Map` of `CheckDefinition`s. `index.ts` iterates it and binds - one `/pygienium-` command per entry, so adding a check is a file + - one `registerCheck()` line. -- **The footer** (`src/footer.ts`) is the piolium-style pipeline-overview - status strip: a single static line in the TUI footer (via - `ui.setStatus(key, text)`) listing the full ordered pipeline with the cursor - on the current phase and what's to come. For a single check the items are - the phases; for `/pygienium-all` they're the checks (the full todo list), - and the per-check footer is suppressed so two overviews never compete over - the same status slot. The chat widget (`phases.ts`) remains the animated - detail view (spinner + tool-call tree + completion tree); the footer is the - overview — the two never overlap. In print/JSON mode the footer is a no-op. - -## Layout - -``` -pygienium/ -├─ src/ -│ ├─ index.ts ← entry: auto-discover checks, bind commands -│ ├─ commands.ts ← slash-command handlers (thin binders) -│ ├─ help.ts ← COMMANDS + CLI_FLAGS → /pygienium-help output -│ ├─ agent-runner.ts ← isolated sub-agent sessions (injectable for tests) -│ ├─ agents.ts ← markdown agent-definition loader -│ ├─ recon.ts ← shared Q0 reconnaissance snapshot -│ ├─ run-state.ts ← persistent, resumable run-state model -│ ├─ status.ts ← /pygienium-status formatter (pure) -│ ├─ export.ts ← /pygienium-export gatherer + md/json renderer -│ ├─ phases.ts ← live chat progress widget + completion-tree helpers -│ ├─ footer.ts ← pipeline-overview status strip (TUI footer) -│ ├─ modes/check-runner.ts ← the per-check phase pipeline -│ └─ checks/ ← one file per check, self-registering -│ ├─ registry.ts ← CheckDefinition + registerCheck -│ ├─ comments.ts deep-modules.ts dead-code.ts -│ ├─ defensive-guards.ts -└─ agents/ ← scanner.md fixer.md deep-modules.md defensive-guards.md -``` - -## License - -MIT +This is the omp port of [Mike/pygienium](https://git.freno.me/Mike/pygienium), regenerated automatically from the source repo. See the source repo for full documentation. diff --git a/src/agent-runner.ts b/src/agent-runner.ts index 11ec09d..768114f 100644 --- a/src/agent-runner.ts +++ b/src/agent-runner.ts @@ -17,9 +17,32 @@ import { mkdir, writeFile } from "node:fs/promises"; import { dirname, isAbsolute, join } from "node:path"; -import type { AgentSessionEvent } from "@oh-my-pi/pi-coding-agent"; +import type { + AgentSession, + AgentSessionEvent, +} from "@oh-my-pi/pi-coding-agent"; import { loadAgents, extensionRoot, type AgentDef } from "./agents.js"; +/** + * Env var overriding the per-agent settle timeout (ms). Guards against a + * sub-agent session that never settles (stalled provider stream, hung retry / + * auto-compaction after the final tool call), which otherwise leaves the run + * stuck mid-phase with no state save, no error, and no completion message. + */ +export const AGENT_TIMEOUT_ENV = "PYGIENIUM_AGENT_TIMEOUT_MS"; + +/** Default settle timeout per agent phase: 60 minutes. */ +const DEFAULT_AGENT_TIMEOUT_MS = 60 * 60_000; + +/** Resolve the per-agent settle timeout, honouring the env override. */ +export function agentTimeoutMs(): number { + const raw = process.env[AGENT_TIMEOUT_ENV]; + if (raw && /^\d+$/.test(raw.trim()) && Number(raw.trim()) > 0) { + return Number(raw.trim()); + } + return DEFAULT_AGENT_TIMEOUT_MS; +} + export interface AgentTaskOptions { /** Absolute working directory for the sub-agent. */ cwd: string; @@ -37,6 +60,62 @@ export interface AgentTaskOptions { * turns tool_execution_start/end + assistant turns into chat messages. */ onEvent?: (event: AgentSessionEvent) => void; + /** + * Maximum time the agent run may take before it is aborted and the phase + * fails loudly (default {@link agentTimeoutMs}). A session that never + * settles — stalled provider stream, hung retry/compaction after its last + * tool call — would otherwise hang the check run silently with no state + * update and no completion message. + */ + timeoutMs?: number; +} + +/** + * Race `promise` against a settle deadline. Returns `{ value }` on success or + * `{ error }` when the deadline elapsed first (the caller aborts the work). + * `promise` is still awaited-then-ignored afterwards so late rejections can + * never surface as unhandled. + */ +export async function withSettleTimeout( + promise: Promise, + timeoutMs: number, + label: string, +): Promise<{ value: T } | { error: string }> { + if (timeoutMs <= 0) { + try { + return { value: await promise }; + } catch (err) { + return { error: err instanceof Error ? err.message : String(err) }; + } + } + const settled = promise.then( + (value) => ({ value }) as { value: T }, + (err) => + ({ error: err instanceof Error ? err.message : String(err) }) as { + error: string; + }, + ); + let timer: ReturnType | undefined; + const deadline = new Promise<{ error: string }>((resolve) => { + timer = setTimeout(() => { + // ~`1m 30s` / `45s` for the message (timeoutMs is ms; tests use small values). + const totalSec = Math.round(timeoutMs / 1000); + const duration = + totalSec >= 60 + ? `${Math.floor(totalSec / 60)}m${totalSec % 60 ? ` ${totalSec % 60}s` : ""}` + : `${totalSec}s`; + resolve({ + error: `${label} did not settle within ${duration}; aborted. Check model/provider connectivity, then resume with /pygienium-resume.`, + }); + }, timeoutMs); + // Never hold the process open just because a deadline is pending. + timer.unref?.(); + }); + try { + return await Promise.race([settled, deadline]); + } finally { + if (timer) clearTimeout(timer); + } } export interface AgentRunResult { @@ -54,8 +133,21 @@ export type AgentRunner = (opts: AgentTaskOptions) => Promise; let currentRunner: AgentRunner = defaultAgentRunner; /** Entry point used by the check-runner. */ -export function runAgentTask(opts: AgentTaskOptions): Promise { - return currentRunner(opts); +export async function runAgentTask( + opts: AgentTaskOptions, +): Promise { + // Backstop: even a third-party/custom runner must not be able to hang the + // run forever. The default runner additionally aborts its session on + // timeout (see defaultAgentRunner); this race covers every other runner. + const settled = await withSettleTimeout( + currentRunner(opts), + opts.timeoutMs ?? agentTimeoutMs(), + `sub-agent "${opts.agentName}"`, + ); + if ("error" in settled) { + return { ok: false, text: "", error: settled.error }; + } + return settled.value; } /** Override the active agent runner (primarily for tests). */ @@ -124,43 +216,40 @@ export async function defaultAgentRunner( agentRegistry: new AgentRegistry(), }); + // A session that never settles (stalled provider stream, hung retry / + // auto-compaction after its last tool call) must fail the phase loudly + // instead of hanging the run mid-transition with zero diagnostics. On + // timeout the session is disposed, which aborts the in-flight run. + const settled = await withSettleTimeout( + runSessionToCompletion(session, opts), + opts.timeoutMs ?? agentTimeoutMs(), + `sub-agent "${opts.agentName}"`, + ); + if ("error" in settled) { + // Abort the still-running session so it can't keep burning provider + // calls; the background settle path then finishes and disposes too. + try { + session.dispose(); + } catch { + /* ignore dispose errors */ + } + return { ok: false, text: "", error: settled.error }; + } + return settled.value; +} + +/** + * Run an in-memory agent session to completion, streaming tool events to the + * chat forwarder, and return the agent's final text. Owns session cleanup. + */ +async function runSessionToCompletion( + session: AgentSession, + opts: AgentTaskOptions, +): Promise { + const acc: SessionEventAccumulator = { text: "" }; try { - let text = ""; - let stopReason: string | undefined; - let errorMessage: string | undefined; const unsubscribe = session.subscribe((event: AgentSessionEvent) => { - if ( - event.type === "message_update" && - event.assistantMessageEvent.type === "text_delta" - ) { - text += event.assistantMessageEvent.delta; - } - if (event.type === "message_end") { - // Capture the full assistant text from the finalized message — - // models that don't stream text_delta (or truncate) still surface - // their output here. Prefer the streamed text when non-empty. - const message = event.message as { - role?: string; - content?: unknown; - stopReason?: string; - errorMessage?: string; - }; - if (message.stopReason) stopReason = message.stopReason; - if (message.errorMessage) errorMessage = message.errorMessage; - if (message.role === "assistant") { - const full = extractAssistantText(message.content).trim(); - if (full && !text.trim()) text = full; - } - } - // Forward the stream-driving events to the chat forwarder; it turns - // each into its own `pygienium-stream` message (see index.ts). - if ( - event.type === "tool_execution_start" || - event.type === "tool_execution_end" || - event.type === "message_end" - ) { - opts.onEvent?.(event); - } + applySessionEvent(acc, event, opts.onEvent); }); await session.prompt(opts.task, { expandPromptTemplates: false }); // Ensure the agent has fully settled (tool calls may still be in-flight @@ -171,17 +260,17 @@ export async function defaultAgentRunner( // Surface session errors that didn't throw but left no useful output. // A session ending with stopReason "error" and no text means the model // call failed silently — treat that as a failed run, not ok:true. - if (errorMessage) { - return { ok: false, text, error: errorMessage }; + if (acc.errorMessage) { + return { ok: false, text: acc.text, error: acc.errorMessage }; } - if (!text.trim() && stopReason === "error") { + if (!acc.text.trim() && acc.stopReason === "error") { return { ok: false, - text, + text: acc.text, error: "sub-agent session ended in error with no output.", }; } - return { ok: true, text }; + return { ok: true, text: acc.text }; } catch (err) { return { ok: false, @@ -197,6 +286,83 @@ export async function defaultAgentRunner( } } +/** Running capture state while a session streams assistant output. */ +export interface SessionEventAccumulator { + /** Joined assistant text seen so far (text_delta stream). */ + text: string; + /** stopReason of the final assistant message, when reported. */ + stopReason?: string; + /** errorMessage of the final assistant message, when reported. */ + errorMessage?: string; +} + +/** + * Interpret one session event into the running accumulator and forward the + * stream-driving events to the chat. + * + * MUST never throw: it runs synchronously inside the SDK's event pipeline + * (`session.subscribe` listeners are invoked from `processEvents`/`_emit`). + * A throw there is not contained — the SDK's run-failure path re-emits + * failure events through the same callback, so a callback that always throws + * recurses until stack overflow and crashes the host, stranding run-state at + * the phase boundary with no save and no completion. Guard every shape (the + * runtime events are partial/streaming and weaker than their types) and never + * let the cosmetic chat forwarder take the run down. + */ +export function applySessionEvent( + acc: SessionEventAccumulator, + event: AgentSessionEvent, + forward?: (event: AgentSessionEvent) => void, +): void { + if (!event) return; + try { + if ( + event.type === "message_update" && + event.assistantMessageEvent?.type === "text_delta" + ) { + acc.text += event.assistantMessageEvent.delta ?? ""; + } + if (event.type === "message_end") { + // Capture the full assistant text from the finalized message — + // models that don't stream text_delta (or truncate) still surface + // their output here. Prefer the streamed text when non-empty. + const message = event.message as + | { + role?: string; + content?: unknown; + stopReason?: string; + errorMessage?: string; + } + | undefined; + if (message) { + if (message.stopReason) acc.stopReason = message.stopReason; + if (message.errorMessage) acc.errorMessage = message.errorMessage; + if (message.role === "assistant") { + const full = extractAssistantText(message.content).trim(); + if (full && !acc.text.trim()) acc.text = full; + } + } + } + } catch (err) { + // A malformed event must not crash the SDK event pipeline; log and skip. + console.error("pygienium: error processing sub-agent event", err); + } + // Forward the stream-driving events to the chat forwarder; it turns each + // into its own `pygienium-stream` message (see index.ts). Cosmetic chat + // UI: a broken forwarder must not fail or hang the agent run. + if ( + event.type === "tool_execution_start" || + event.type === "tool_execution_end" || + event.type === "message_end" + ) { + try { + forward?.(event); + } catch (err) { + console.error("pygienium: error forwarding sub-agent event", err); + } + } +} + /** * Extract joined text from an assistant message's content blocks. * Mirrors piolium's `extractAssistantText`. diff --git a/src/checks/comments.ts b/src/checks/comments.ts index 9a735f1..a9339de 100644 --- a/src/checks/comments.ts +++ b/src/checks/comments.ts @@ -92,8 +92,11 @@ ${scopeRulesMarkdown()} 3. Apply the rubric below to each comment and classify it: RESTATE, VERBOSE, WHY, or OK. 4. Write a findings report to \`${findingsFile}\` with per-file line refs. -5. Return the findings report text as your final message (same content as the - file). The host captures it as the analysis-phase findings. +5. Return a ONE-LINE summary as your final message, e.g. + \` comment smell(s) across file(s).\` The full report lives + in findings.md — do NOT regenerate the report text in your final message + (regenerating a large report doubles the fragile output right after the + file write and can stall the run). ${RUBRIC} @@ -110,7 +113,8 @@ ${RUBRIC} \`\`\` If no smells are found, write \`# comments — findings\n\n0 comment smell(s).\` -and return that text. Always create findings.md so the run has an artifact. +and return \`0 comment smell(s).\` as your final message. Always create +findings.md so the run has an artifact. Write the report under \`${outDir}\` (create directories as needed). `; @@ -128,6 +132,7 @@ export function buildCommentsFixTask( ): string { const outDir = commentsArtifactDir(scope); const changesFile = changesPath(scope); + const findingsFile = findingsPath(scope); return `# Task: comments hygiene fix You are running the **comments** hygiene fix phase. @@ -136,6 +141,9 @@ You are running the **comments** hygiene fix phase. - Fix target: \`${scope.target}\` ## Input: scan findings +Read the detailed per-file findings from \`${findingsFile}\` (created by the +scan phase). If that file is missing or unreadable, fall back to the findings +text below: ${findings.trim().length > 0 ? findings : "(no findings text provided)"} ## What to do diff --git a/src/modes/check-runner.ts b/src/modes/check-runner.ts index dd7eaf1..ad76d41 100644 --- a/src/modes/check-runner.ts +++ b/src/modes/check-runner.ts @@ -142,7 +142,12 @@ export async function runCheck( ): Promise { const startMs = Date.now(); const outcome = await runCheckImpl(opts); - postCheckCompletion(opts, outcome, Date.now() - startMs); + try { + postCheckCompletion(opts, outcome, Date.now() - startMs); + } catch { + // Completion posting is best-effort chat UI: a renderer or send + // failure must never reject the run after its state was persisted. + } return outcome; } @@ -383,7 +388,16 @@ async function runCheckImplInner( error = err instanceof Error ? err.message : String(err); markCheckStatus(state, check.name, "failed", error); markRunStatus(state, reconcileRunStatus(state)); - await saveRunState(state); + try { + await saveRunState(state); + } catch (saveErr) { + // A failing state save inside the error path must not mask the + // original error or escape as an unhandled rejection (which would + // kill the run with nothing persisted or reported). + error += `; (also failed to persist run-state: ${ + saveErr instanceof Error ? saveErr.message : String(saveErr) + })`; + } return { status: "failed", error, findings, changes, state }; } finally { strip.done(); diff --git a/tests/agent-runner.test.ts b/tests/agent-runner.test.ts new file mode 100644 index 0000000..2235472 --- /dev/null +++ b/tests/agent-runner.test.ts @@ -0,0 +1,137 @@ +/** + * agent-runner.test.ts — unit tests for the session event accumulator. + * + * `applySessionEvent` runs synchronously inside the SDK's event pipeline; a + * throw there crashes the host (the SDK's run-failure path re-emits events + * through the same callback → stack overflow), stranding run-state at the + * phase boundary. These tests pin the handler to never throw on the partial / + * malformed event shapes streaming sessions actually emit. + */ +import { describe, expect, it } from "bun:test"; +import { + applySessionEvent, + type SessionEventAccumulator, +} from "../src/agent-runner.js"; +import type { AgentSessionEvent } from "@oh-my-pi/pi-coding-agent"; + +function fresh(): SessionEventAccumulator { + return { text: "" }; +} + +/** Build a typed-as-unknown event so malformed shapes compile in tests. */ +function event(shape: unknown): AgentSessionEvent { + return shape as AgentSessionEvent; +} + +describe("applySessionEvent", () => { + it("accumulates text_delta stream events in order", () => { + const acc = fresh(); + applySessionEvent( + acc, + event({ + type: "message_update", + assistantMessageEvent: { type: "text_delta", delta: "foo" }, + }), + ); + applySessionEvent( + acc, + event({ + type: "message_update", + assistantMessageEvent: { type: "text_delta", delta: "bar" }, + }), + ); + expect(acc.text).toBe("foobar"); + }); + + it("does not throw on a message_update with no assistantMessageEvent", () => { + const acc = fresh(); + expect(() => + applySessionEvent(acc, event({ type: "message_update" })), + ).not.toThrow(); + expect(acc.text).toBe(""); + }); + + it("does not throw on a message_update with an unknown event shape", () => { + const acc = fresh(); + expect(() => applySessionEvent(acc, event({ type: "bogus_event" }))).not.toThrow(); + expect(() => applySessionEvent(acc, event(null))).not.toThrow(); + expect(() => applySessionEvent(acc, event(undefined))).not.toThrow(); + expect(acc.text).toBe(""); + }); + + it("captures full text + stopReason from message_end when nothing streamed", () => { + const acc = fresh(); + applySessionEvent( + acc, + event({ + type: "message_end", + message: { + role: "assistant", + stopReason: "stop", + content: [{ type: "text", text: "full report" }], + }, + }), + ); + expect(acc.text).toBe("full report"); + expect(acc.stopReason).toBe("stop"); + }); + + it("does not throw on a message_end with no message", () => { + const acc = fresh(); + expect(() => applySessionEvent(acc, event({ type: "message_end" }))).not.toThrow(); + expect(acc.text).toBe(""); + }); + + it("records an errorMessage surfaced on the final message", () => { + const acc = fresh(); + applySessionEvent( + acc, + event({ + type: "message_end", + message: { role: "assistant", errorMessage: "upstream 529" }, + }), + ); + expect(acc.errorMessage).toBe("upstream 529"); + }); + + it("forwards stream-driving events and swallows a throwing forwarder", () => { + const originalError = console.error; + console.error = () => {}; + try { + const forwarded: string[] = []; + const forward = (ev: AgentSessionEvent) => { + forwarded.push(ev.type); + if (ev.type === "tool_execution_end") throw new Error("renderer boom"); + }; + const acc = fresh(); + applySessionEvent( + acc, + event({ type: "tool_execution_start", toolName: "bash" }), + forward, + ); + applySessionEvent( + acc, + event({ type: "tool_execution_end", toolName: "bash" }), + forward, + ); + expect(forwarded).toEqual(["tool_execution_start", "tool_execution_end"]); + } finally { + console.error = originalError; + } + }); + + it("does not forward non-stream-driving events", () => { + const forwarded: string[] = []; + const acc = fresh(); + applySessionEvent( + acc, + event({ type: "message_update", assistantMessageEvent: { type: "text_delta", delta: "x" } }), + (ev) => forwarded.push(ev.type), + ); + applySessionEvent(acc, event({ type: "agent_settled" }), (ev) => + forwarded.push(ev.type), + ); + expect(forwarded).toEqual([]); + expect(acc.text).toBe("x"); + }); +}); diff --git a/tests/check-runner.test.ts b/tests/check-runner.test.ts index a02c2e1..a38a519 100644 --- a/tests/check-runner.test.ts +++ b/tests/check-runner.test.ts @@ -20,6 +20,8 @@ import { setAgentRunner, resetAgentRunner, fakeAgentRunner, + AGENT_TIMEOUT_ENV, + type AgentRunResult, } from "../src/agent-runner.js"; import { handleCheckCommand, type PygieniumCtx } from "../src/commands.js"; import { loadRunState, runStatePath } from "../src/run-state.js"; @@ -128,4 +130,29 @@ describe("check-runner integration", () => { expect(state?.checks.gated.status).toBe("skipped"); expect(state?.checks.gated.error).toBe("no source files matched"); }); + + it("fails the check loudly when the agent never settles", async () => { + // Models the silent hang: the sub-agent session never resolves after + // its last tool call (stalled provider stream / hung retry), leaving + // the run stuck mid-phase with no state save and no completion. The + // watchdog must abort the phase with a visible error instead. + setAgentRunner(() => new Promise(() => {})); + const prev = process.env[AGENT_TIMEOUT_ENV]; + process.env[AGENT_TIMEOUT_ENV] = "50"; + try { + await handleCheckCommand(smokeCheck(), "", stubCtx(cwd)); + } finally { + if (prev === undefined) delete process.env[AGENT_TIMEOUT_ENV]; + else process.env[AGENT_TIMEOUT_ENV] = prev; + } + + const state = await loadRunState(cwd); + expect(state?.checks.smoke.status).toBe("failed"); + expect(state?.checks.smoke.error).toContain("did not settle within"); + expect(state?.checks.smoke.error).toContain("/pygienium-resume"); + expect( + state?.checks.smoke.phases.find((p) => p.id === "analysis")?.status, + ).toBe("failed"); + expect(state?.status).toBe("failed"); + }); }); diff --git a/tests/comments.test.ts b/tests/comments.test.ts index 1a70c4a..ba058ea 100644 --- a/tests/comments.test.ts +++ b/tests/comments.test.ts @@ -30,6 +30,8 @@ import { commentsCheck, findingsPath, changesPath, + buildCommentsScanTask, + buildCommentsFixTask, } from "../src/checks/comments.js"; import type { CheckScope } from "../src/checks/registry.js"; @@ -256,4 +258,31 @@ describe("comments check (end-to-end)", () => { expect(check!.findings).toBeDefined(); expect(check!.changes).toBeDefined(); }); + + it("scan task keeps the full report in findings.md, not the final message", () => { + // Regression: the scan task used to demand the agent regenerate the + // whole report as its final message right after writing findings.md — + // a second huge output that stalled the phase transition (observed in + // freno-dev twice). The final message must stay a one-line summary. + const scope: CheckScope = { + cwd, + target, + fix: false, + rest: [], + }; + const task = buildCommentsScanTask(cwd, scope); + expect(task).toContain("ONE-LINE summary"); + expect(task).toContain("do NOT regenerate the report text"); + expect(task).not.toContain( + "Return the findings report text as your final message", + ); + expect(task).not.toContain("same content as the file"); + + // The fix phase must read the detailed findings from the artifact so a + // one-line scan summary can't starve it. + const fixTask = buildCommentsFixTask(cwd, scope, "fallback-findings-text"); + expect(fixTask).toContain("Read the detailed per-file findings"); + expect(fixTask).toContain(findingsPath(scope)); + expect(fixTask).toContain("fallback-findings-text"); + }); });