review: trim verbose comments to minimal why-notes · Entire

review: trim verbose comments to minimal why-notes

d120107→main·

dipree·2w ago·8 files·+145 added/-82 removed

Sessions

a115f772e914View transcript

[?
Fix Multi-Agent Run Hanging and Finalization IssuesPi·Opus 4.8·2 steps](/content/gh/entireio/cli/session/019f13ee-df40-718d-b2b1-aac05df371ca#timeline-a115f772e914/index.html)

Changes

8

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120

// Entire CLI extension for pi
// Auto-generated by `entire enable --agent pi`
// Do not edit manually — changes will be overwritten on next install.
//
// Forwards pi lifecycle events to `entire hooks pi <event>` so Entire can
// create checkpoints, capture transcripts, and offer rewind/resume.
//
// Events forwarded:
//   - session_start  → entire hooks pi session-start
//   - before_agent_start (first turn of a prompt) → entire hooks pi turn-start
//   - agent_end      → entire hooks pi turn-end
//   - session_compact → entire hooks pi compaction
//   - session_shutdown → entire hooks pi session-end
//
// All hooks receive a JSON payload on stdin:
//   { session_id, session_ref, prompt?, model? }
// where session_ref is the absolute path to the JSONL session file (or empty
// for in-memory/ephemeral sessions).

import type { ExtensionAPI } from "@earendil-works/pi-coding-agent";

export default function (pi: ExtensionAPI) {
  // ENTIRE_CMD is replaced at install time by Entire's installer.
  const ENTIRE_CMD = "entire";

// Track the current model so turn-start/turn-end can report it.
  let currentModel = "";

function sessionRef(ctx: any): string {
    try {
      return ctx?.sessionManager?.getSessionFile?.() ?? "";
    } catch {
      return "";
    }
  }

function sessionIdOf(ctx: any): string {
    try {
      return ctx?.sessionManager?.getSessionId?.() ?? "";
    } catch {
      return "";
    }
  }

// Fire `entire hooks pi <hookName>` with a JSON payload on stdin.
  // Errors are swallowed — extension failures must never crash pi.
  async function callHook(hookName: string, payload: Record<string, unknown>) {
    try {
      const json = JSON.stringify(payload) + "\n";
      // sh -c lets ENTIRE_CMD include shell substitutions (local-dev mode).
      const result = await pi.exec(
        "sh",
        ["-c", `${ENTIRE_CMD} hooks pi ${hookName}`],
        {
          input: json,
          timeout: 30_000,
        } as any,
      );
      void result;
    } catch {
      // Silently ignore — extension failures must not crash pi.
    }
  }

pi.on("session_start", async (event: any, ctx: any) => {
    const session_id = sessionIdOf(ctx);
    if (!session_id) return;
    await callHook("session-start", {
      session_id,
      session_ref: sessionRef(ctx),
      reason: event?.reason ?? "",
    });
  });

pi.on("before_agent_start", async (event: any, ctx: any) => {
    const session_id = sessionIdOf(ctx);
    if (!session_id) return;
    await callHook("turn-start", {
      session_id,
      session_ref: sessionRef(ctx),
      prompt: typeof event?.prompt === "string" ? event.prompt : "",
      model: currentModel,
    });
  });

pi.on("model_select", async (event: any) => {
    const m = event?.model;
    if (m && typeof m.id === "string") {
      currentModel = m.provider ? `${m.provider}/${m.id}` : m.id;
    }
  });

pi.on("agent_end", async (_event: any, ctx: any) => {
    const session_id = sessionIdOf(ctx);
    if (!session_id) return;
    await callHook("turn-end", {
      session_id,
      session_ref: sessionRef(ctx),
      model: currentModel,
    });
  });

pi.on("session_compact", async (_event: any, ctx: any) => {
    const session_id = sessionIdOf(ctx);
    if (!session_id) return;
    await callHook("compaction", {
      session_id,
      session_ref: sessionRef(ctx),
    });
  });

pi.on("session_shutdown", async (_event: any, ctx: any) => {
    const session_id = sessionIdOf(ctx);
    if (!session_id) return;
    await callHook("session-end", {
      session_id,
      session_ref: sessionRef(ctx),
    });
  });
}

A.pi/extensions/entire.ts+120

27 unmodified lines

28
29
30
31
32
33
34
35
36
37
38
31
32
33
34
35
5 unmodified lines

41
42
43
50
51
52
44
45
46

27 unmodified lines

// is available. Matches the cap used by status_style.getTerminalWidth.
const DefaultTerminalWidth = 80

// MaxRenderBytes caps the markdown size handed to glamour. Glamour's render
// cost is strongly super-linear: ~1s at 500KB, ~6s at 2MB, ~49s at 4MB, and
// minutes-to-effectively-never beyond that. A long-running agent can emit
// multi-MB narratives, and review's post-run DumpSink renders each one on the
// orchestrator's finalize goroutine — so an unbounded render wedges the whole
// run on a frozen "Finalizing output..." with no way forward. Above this cap
// we skip styling and return the raw markdown unchanged: still fully readable,
// just unstyled, and bounded. 256KB renders in well under a second.
// MaxRenderBytes caps glamour input: its render cost is super-linear (~6s at
// 2MB, minutes beyond), so above this we return raw markdown unchanged.
const MaxRenderBytes = 256 * 1024

// Render produces a glamour-styled string from markdown using the entire
5 unmodified lines

// than a runtime condition. Renderer panics are recovered and returned as
// errors so callers can fall back to raw markdown instead of crashing.
func Render(markdown string, width int, darkBackground bool) (rendered string, err error) {
  // Guard against glamour's super-linear blowup on very large inputs: above
  // MaxRenderBytes, return the raw markdown unchanged rather than hang the
  // caller for minutes. See MaxRenderBytes for the cost curve.
  if len(markdown) > MaxRenderBytes {
    return markdown, nil;
  }
}

Mcmd/entire/cli/mdrender/mdrender.go+2/-11

109 unmodified lines

110
111
112
113
114
115
116
117
113
114
115
116
117
121
122
118
119
120
121

109 unmodified lines

// TestRender_EmptyInputDoesNotPanic verifies the renderer handles edge cases
// (empty string, whitespace-only) without erroring.
// TestRender_OversizedInputReturnsRawQuickly pins the guard against glamour's
// super-linear blowup: inputs larger than MaxRenderBytes must skip styling and
// return the raw markdown unchanged (and near-instantly) instead of wedging the
// caller for minutes. Regression for the review run that hung forever on
// "Finalizing output..." while DumpSink rendered a multi-MB agent narrative.
// Inputs over MaxRenderBytes must return raw markdown quickly, not wedge the
// caller in glamour's super-linear render.
func TestRender_OversizedInputReturnsRawQuickly(t *testing.T) {
  t.Parallel()

// 8MB of markdown took >4 minutes through glamour in benchmarking; the
  // guard must make this effectively free.
  // 8MB takes >4 minutes through glamour; the guard must make it instant.
  big := strings.Repeat("# Heading\n\nparagraph text here\n\n", (8*1024*1024)/30)
  if len(big) <= mdrender.MaxRenderBytes {
    t.Fatalf("setup: test input %d should exceed MaxRenderBytes %d", len(big), mdrender.MaxRenderBytes)
  }
}

Mcmd/entire/cli/mdrender/mdrender_test.go+3/-7

1443 unmodified lines

1444
1445
1446
1447
1448
1449
1450
1451
1452
1453
1447
1448
1449
1450
1451

1443 unmodified lines

return
}

// The findings were produced by worker sessions that already finished. If
// the user Ctrl+C's out of a slow final-report/synthesis step, the run
// context is cancelled AFTER summary.Cancelled was computed (so the guard
// above doesn't fire), and the git/disk work below would fail with
// "context canceled" — silently discarding findings the user waited
// minutes for. Detach from the run context and give persistence its own
// short deadline so completed findings survive a cancel during finalize.
// Detach from the run context: a Ctrl+C during a slow finalize cancels ctx
// after the workers finished, and must not discard their findings.
ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second)
defer cancel()
}

Mcmd/entire/cli/review/cmd.go+2/-7

4 unmodified lines

5
6
7
8
9
10
11
12
13
14
15
8
9
10
11
12
13
14
25 unmodified lines

40
41
42
47
48
49
43
44
45
46

4 unmodified lines

// AgentEvent is a no-op; events are read from RunSummary.AgentRuns[].Buffer
// in RunFinished.
//
// Output format: each agent's block is composed as markdown (`# claude-code
// review`, with failure context in blockquotes/bold) and written as-is. Worker
// narratives are raw material, not a deliverable — the human reads the final
// synthesized report (which IS styled) and can drill into a worker's buffer
// interactively. So DumpSink deliberately does NOT glamour-render: styling
// every worker's narrative is wasted work that, on multi-MB output, made
// glamour's super-linear cost wedge the whole finalize phase. Plain markdown is
// fully readable, grep-able in pipelines, and bounded.
// Each agent's block is plain markdown written as-is — NOT glamour-rendered.
// Worker narratives are raw material (the final report is styled, and drill-in
// shows the buffer); styling multi-MB output here wedged the finalize phase on
// glamour's super-linear cost.
package review

import (
25 unmodified lines

s.dumpCounts(summary)
}

// dumpAgent composes one agent's section as plain markdown and writes it
// directly to W (no glamour styling — see the package comment). The counts line
// at the end of the run is likewise a terse single-line status summary.
// dumpAgent writes one agent's section as plain markdown directly to W.
//
// Markdown structure per agent:

Mcmd/entire/cli/review/dump.go+5/-11

331 unmodified lines

332
333
334
335
336
337
338
339
335
336
337
338
339
340
341
345
342
343
344
8 unmodified lines

353
354
355
360
361
362
356
357
358

331 unmodified lines

}
}

// TestWritePostReviewManifest_SurvivesCancelledRunContext pins the fix for
// findings being silently discarded when the user Ctrl+C's out of a slow
// final-report step: the workers already finished (summary not cancelled), so
// persistence must run on a detached context and not fail with
// "context canceled" just because the run context was cancelled afterward.
// Findings from finished workers must persist even if the run context is
// cancelled during finalize (summary not cancelled).
func TestWritePostReviewManifest_SurvivesCancelledRunContext(t *testing.T) {
    repoRoot := t.TempDir()
    testutil.InitRepo(t, repoRoot)
    t.Chdir(repoRoot)

// Simulate the run context cancelled during finalization.
    ctx, cancel := context.WithCancel(context.Background())
cancel()

writePostReviewManifest(ctx, &out, repoRoot, "abc123", summary, "")

got := out.String()
    // The git/disk work must have run despite the cancelled input context: we
    // reach the normal "no matching sessions" path, not a context-canceled
    // failure.
    if strings.Contains(got, "context canceled") {
        t.Fatalf("persistence used the cancelled run context; findings would be lost:\n%s", got)
    }
}

Mcmd/entire/cli/review/manifest_test.go+2/-9

92 unmodified lines

93
94
95
96
97
96
97
98
99
76 unmodified lines

176
177
178
180
181
182
183
184
185
186
187
188
189
179
180
181
182
183
184
37 unmodified lines

222
223
224
233
234
235
236
237
225
226
227
228
229
316 unmodified lines

546
547
548
560
561
562
563
549
550
551
552

92 unmodified lines

termHeight int

finished bool
    // finishedAt records when runFinishedMsg arrived, so the finalize footer can
    // show how long post-run work has run and a genuine hang stays visible.
    // finishedAt drives the finalize footer's elapsed timer.
    finishedAt time.Time
    summary    reviewtypes.RunSummary

76 unmodified lines

// emitted) or Failed (process exit non-zero, no Finished emitted)
        // would still render as "running" in the final frame.
        //
        // The summary is authoritative for terminal classification:
        //   - Unknown row: adopt the summary status outright.
        //   - Summary Failed: override an optimistic stream Succeeded. An agent
        //     can emit Finished{Success:true} just before its process exits
        //     non-zero; classifyStatus sees the exit code, the event stream
        //     does not. Without this, the row renders "✓ done" while the counts line
        //     (built from the summary) reports it failed — the two
        //     halves of the dashboard disagreeing in the final frame.
        //   - Summary Cancelled: same downgrade, but stream Failed still wins
        //     (a real RunError is more specific than a blanket ctx cancel).
        // The summary is authoritative: let it downgrade an optimistic stream
        // Succeeded to Failed/Cancelled so rows match the counts line. Stream
        // Failed (a real RunError) still wins over a blanket Cancelled.
        now := time.Now()
        for i, run := range msg.summary.AgentRuns {
            if i >= len(m.rows) {

Mcmd/entire/cli/review/tui_model.go+7/-21

300 unmodified lines

301
302
303
304
305
306
307
308
304
305
306
307
117 unmodified lines

425
426
427
432
433
434
435
436
437
428
429
430
431
432
433
442
434
435
436
437
438
439
449
440
441
442
8 unmodified lines

451
452
453
464
465
466
454
455
456
457

300 unmodified lines

}
}

// TestTUIModel_FinishedKeepsTicking pins that the dashboard keeps animating
// after the run finishes. The finished frame is the finalize window (post-run
// sinks still running before PostRunComplete quits the program); a frozen
// "Finalizing output..." line is indistinguishable from a hang, so the spinner
// and elapsed timer must keep ticking to signal liveness.
// The dashboard must keep ticking through finalize so the footer animates.
func TestTUIModel_FinishedKeepsTicking(t *testing.T) {
    t.Parallel()
    m := newTestModel([]string{"agent-a"}, func() {})
117 unmodified lines

}
}

// TestTUIModel_RunFinishedMsg_SummaryFailedOverridesStreamSucceeded pins the
// fix for a dashboard that disagreed with itself: an agent that emitted
// Finished{Success:true} but whose process later exited non-zero was rendered
// "✓ done" in its row while the counts line (built from the summary) reported it
// failed. The summary is authoritative for terminal classification, so the row
// must downgrade to failed to match.
// A summary Failed must override an optimistic stream Succeeded so the row and
// the counts line agree.
func TestTUIModel_RunFinishedMsg_SummaryFailedOverridesStreamSucceeded(t *testing.T) {
    t.Parallel()
    m := newTestModel([]string{"agent-a"}, func() {})

// Stream optimistically marks the row Succeeded.
    updated, _ := m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.Finished{Success: true}})
    m = mustModel(t, updated)
    if m.rows[0].status != reviewtypes.AgentStatusSucceeded {
        t.Fatalf("setup: want stream Succeeded, got %v", m.rows[0].status)
    }

// Orchestrator summary saw the non-zero process exit: Failed.
    summary := reviewtypes.RunSummary{AgentRuns: []reviewtypes.AgentRun{
        {Name: "agent-a", Status: reviewtypes.AgentStatusFailed},
    }}
8 unmodified lines

}
}

// TestTUIModel_RunFinishedMsg_StreamFailedStickyOverCancel pins that a real
// RunError (Failed) is not blanket-downgraded to Cancelled when the run context
// was cancelled — the specific failure is more useful than the generic cancel.
// Stream Failed (a real RunError) must not be downgraded to a blanket Cancelled.
func TestTUIModel_RunFinishedMsg_StreamFailedStickyOverCancel(t *testing.T) {
    t.Parallel()
    m := newTestModel([]string{"agent-a"}, func() {})
}

Mcmd/entire/cli/review/tui_model_test.go+4/-16