Files
shockbot/utils/learnings.ts
T
David Blass dd26d35137 learnings: audit fixes — preamble in TOC, server-side line-boundary truncation, empty-repo intro (#743)
* learnings: surface preamble in TOC, mirror line-boundary truncation server-side, fix empty-repo intro copy

three audit fixes on top of the recent learnings overhaul (#717):

- `parseLearningsHeadings` now prepends a synthetic `(preamble)` entry
  when a body has non-whitespace content before the first heading. the
  prompt instructs the agent NOT to slurp the whole file when a TOC is
  present, so without this any preamble lines were silently invisible
  (realistic transitional case: an agent partially restructures a
  legacy free-text body and leaves bullets above the first `## `).

- server-side PATCH route now applies the same line-boundary-aware
  truncation as the action (defense in depth via a shared
  `truncateAtLineBoundary` + `MAX_LEARNINGS_LENGTH` exported from
  `action/internal`). the raw `.slice` it used before could leave a
  mid-heading tail on any caller that bypassed the client-side
  truncate, breaking the next-seed TOC parse. removes the duplicated
  cap constant.

- `buildLearningsSection` intro no longer asserts "accumulated by
  previous agent runs" — false for fresh repos with zero history. new
  copy is tense-neutral and works for empty + populated bodies. also
  nudges the agent to re-read after mid-run edits (the inlined TOC
  ranges are a run-start snapshot).

Co-authored-by: Cursor <cursoragent@cursor.com>

* learnings prompt: tighten to single evergreen test, allow tool-quirk bullets when they prevent repeat waste

The blanket "no pullfrog tool quirks" ban was wrong — if the agent burned
calls discovering a quirk this run, recording the workaround prevents the
next run from repeating the waste. Reframe around one litmus ("would a
future run do its work better because this bullet exists?") and trust it
to subsume the scattered don'ts. Drop the 3+ months timeframe (arbitrary)
and the four-example pullfrog/PR/date/play-by-play list (the rule
underneath is "don't anchor facts to repo state that will move"). Cuts
~10 lines from a prompt the model was already mostly ignoring; the
remaining anchor list is narrower and more enforceable.

* audit-learnings-r2: align wiki + tighten re-read nudge

- wiki/prompt.md described the post-run reflection prompt as "bans pullfrog-tool quirks (those belong in tool descriptions, not per-repo learnings), bans PR/review/commit/date references" — that's stale after the prompt rewrite. update to: single-litmus framing, expanded anchor list (now includes version pins + line numbers), and explicit allowance for tool-quirk workarounds when discovery burned calls.
- buildLearningsSection re-read nudge said "re-read after editing" which can be read as "re-read the section you edited". in fact any edit shifts the line numbers of every later section in the TOC, not just the edited one. tighten to make that explicit. mirror the new wording in the wiki example block. update the test substring assertion accordingly.

* postRun: refresh JSDoc to match the reflection prompt rewrite

`buildLearningsReflectionPrompt`'s JSDoc still listed "PR-/review-/commit-/date-anchored facts" and "rediscovery of pullfrog-tool quirks" as failure modes the prompt pushes back on. after b586b4f8 the prompt no longer bans tool-quirk bullets (it explicitly allows them when the agent burned calls discovering the quirk), and the anchor list expanded to cover branch refs, version pins, and line numbers too. update the JSDoc so it describes the prompt that actually exists, and call out the cross-repo drift tradeoff that comes with allowing tool-quirk bullets.

* fix(mcp/issueEvents): narrow event.event before Set.has lookup

octokit's listEventsForTimeline union includes timeline-event members where `event` is `event?: string`. `("event" in event)` does not narrow that property to non-undefined, so `relevantEventTypes.has(event.event)` was passing `string | undefined` to a `Set<string>.has`. typescript only flagged this once `cf-worker-indexing` started seeing the file via the type graph that now reaches mcp through the new `truncateAtLineBoundary` re-export in `action/internal/index.ts`. fix the latent bug at the source: require `typeof event.event === "string"` before the Set lookup.

* learnings: split truncation helpers into MCP-free module

re-exporting `truncateAtLineBoundary` + `MAX_LEARNINGS_LENGTH` from `action/utils/learnings.ts` through `action/internal/index.ts` accidentally pulled the entire MCP type graph into the SDK barrel: `learnings.ts` imports `ToolContext` from `mcp/server.ts`, which transitively wires every tool module under `action/mcp/` into anything that imports from `pullfrog/internal`. for `cf-worker-indexing/tsconfig.json` (`customConditions: ["@pullfrog/source"]`) and the root `tsc` (which compiles the proprietary app routes that import from `pullfrog/internal`), this expanded the type-checked surface and surfaced two latent issues in unrelated files (`mcp/issueEvents.ts`, `utils/subprocess.ts`). a 6-line pure string helper has no business dragging mcp/server.ts into anyone else's type graph.

move both symbols to `action/utils/learningsTruncate.ts`. `learnings.ts` re-exports them so existing callers keep working; `internal/index.ts` re-exports from the truncate-only module so the SDK barrel stays MCP-free.

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Colin McDonnell <colinmcd94@gmail.com>
2026-05-19 21:47:10 +00:00

131 lines
5.4 KiB
TypeScript

import { mkdir, readFile, writeFile } from "node:fs/promises";
import { dirname, join } from "node:path";
import type { ToolContext } from "../mcp/server.ts";
import { apiFetch } from "./apiFetch.ts";
import { log } from "./cli.ts";
import { MAX_LEARNINGS_LENGTH, truncateAtLineBoundary } from "./learningsTruncate.ts";
export { MAX_LEARNINGS_LENGTH, truncateAtLineBoundary };
/**
* Repo-level learnings — operational facts about a repo (setup steps, test
* commands, conventions, gotchas) that accumulate across agent runs and feed
* back into future runs as durable context. Modeled on the PR-summary tmpfile
* pattern (see action/utils/prSummary.ts):
*
* 1. server seeds `pullfrog-learnings.md` with the verbatim body of
* `Repo.learnings` (or empty for fresh repos), and parses headings
* server-side (`utils/learningsToc.ts`) — the parsed TOC is rendered
* into the LEARNINGS prompt section, not into the file
* 2. the agent reads the TOC in the prompt and uses listed line ranges
* to read just the sections relevant to the current task — file can
* grow large, but only targeted ranges hit the agent's context
* 3. agent edits the file in place at end-of-run during the reflection
* turn (see action/agents/postRun.ts buildLearningsReflectionPrompt)
* 4. main.ts reads the file back at end-of-run and PATCHes
* `/api/repo/[owner]/[repo]/learnings` if the body changed
*
* Edit-in-place avoids stuffing the entire learnings list into both the
* prompt context and an `update_learnings` MCP tool call (which previously
* required passing the FULL merged list as a string parameter — an
* output-token tax that grew linearly with the learnings size).
*
* Section structure is agent-curated. The reflection prompt teaches
* hierarchy + a soft 300-line-per-section cap to keep TOC ranges
* agent-targetable on long-lived repos; there is no fixed taxonomy.
*/
export const LEARNINGS_FILE_NAME = "pullfrog-learnings.md";
export function learningsFilePath(tmpdir: string): string {
return join(tmpdir, LEARNINGS_FILE_NAME);
}
/** seed the rolling learnings tmpfile with the verbatim DB body (or empty
* string for fresh repos). returns the absolute path. the parsed TOC is
* carried separately via `RepoSettings.learningsHeadings` and rendered
* into the prompt by `resolveInstructions`, so the file on disk is just
* the body — no markers, no scaffold, no in-file TOC. */
export async function seedLearningsFile(params: {
tmpdir: string;
current: string | null;
}): Promise<string> {
const path = learningsFilePath(params.tmpdir);
await mkdir(dirname(path), { recursive: true });
await writeFile(path, params.current ?? "", "utf8");
return path;
}
/** read the agent-edited learnings file. returns null when the file is
* missing or unreadable (treated as "no change"). caps content at the
* server's max length to avoid a 400 round-trip. */
export async function readLearningsFile(path: string): Promise<string | null> {
let raw: string;
try {
raw = await readFile(path, "utf8");
} catch {
return null;
}
return truncateAtLineBoundary(raw.trim(), MAX_LEARNINGS_LENGTH);
}
/**
* Read the agent-edited repo-level learnings tmpfile and PATCH it to
* `Repo.learnings`.
*
* Best-effort: any failure is logged and does not affect the run's success
* status. Skips the PATCH when the file is byte-trim-identical to its seed —
* the agent didn't touch it, so writing the same content back would just
* burn a `LearningsRevision` row and an API round-trip.
*
* `ctx.toolState.model` is forwarded so `LearningsRevision.model` keeps
* populating; it powers the per-revision attribution badge in the UI
* history view.
*
* `learningsPersistAttempted` guards against double-execution between the
* normal end-of-run path and the SIGINT/SIGTERM handler.
*/
export async function persistLearnings(ctx: ToolContext): Promise<void> {
const filePath = ctx.toolState.learningsFilePath;
if (!filePath) return;
if (ctx.toolState.learningsPersistAttempted) return;
ctx.toolState.learningsPersistAttempted = true;
const current = await readLearningsFile(filePath);
if (current === null) {
log.debug(`learnings tmpfile missing or unreadable at ${filePath} — skipping persist`);
return;
}
const seed = ctx.toolState.learningsSeed?.trim() ?? "";
if (current === seed) {
log.debug("learnings tmpfile unchanged from seed — skipping persist");
return;
}
try {
const response = await apiFetch({
path: `/api/repo/${ctx.repo.owner}/${ctx.repo.name}/learnings`,
method: "PATCH",
headers: {
authorization: `Bearer ${ctx.apiToken}`,
"content-type": "application/json",
},
body: JSON.stringify({
learnings: current,
model: ctx.toolState.model,
}),
signal: AbortSignal.timeout(10_000),
});
if (!response.ok) {
const error = await response.text().catch(() => "(no body)");
// promoted from debug → warning: this path means the agent edited the
// file (we already short-circuited the unchanged-from-seed case above)
// but the PATCH dropped it on the floor. silently losing real work is
// worse than the noise of a CI warning.
log.warning(`learnings persist failed (${response.status}): ${error}`);
return;
}
log.info("» learnings updated");
} catch (err) {
log.warning(`learnings persist failed: ${err instanceof Error ? err.message : String(err)}`);
}
}