learnings: edit-in-place tmpfile (drop update_learnings tool) (#635)

* learnings: edit-in-place tmpfile (drop update_learnings tool)

learnings now follow the PR-summary file pattern: server seeds
`pullfrog-learnings.md` from `Repo.learnings` at startup, agent reads
it as part of context, may edit in place during the post-run reflection
turn, server reads back at end-of-run and PATCHes if changed.

motivation: `update_learnings` required the agent to pass the FULL
merged list as a string parameter — an output-token tax that grew
linearly with the learnings size, and a constant prompt-context
expansion since the contents were also inlined into the LEARNINGS
section. for repos with mature learnings the prompt was getting
visibly noisy in CI logs.

key changes:
- new `action/utils/learnings.ts` (seed/read helpers + 10k cap)
- `main.ts`: always seed; `persistLearnings` mirrors `persistSummary`
  (success path, error path, exit-signal handler, idempotent guard,
  byte-trim equality skip); forwards `model` for `LearningsRevision.model`
- `LEARNINGS` prompt section now contains only the file path + a
  one-line "read it" instruction (no contents inlined)
- `update_learnings` MCP tool deleted; `action/mcp/learnings.ts` removed
- reflection turn (`buildLearningsReflectionPrompt`) reframed around
  file editing with explicit prune-stale + leave-alone-if-nothing-new
  framing
- `learningsStep` removed from every mode checklist — surface lives only
  in the LEARNINGS prompt section + the reflection turn now

* learnings: harden seed step + refresh stale docs (review feedback)

Three findings from PR review, all implemented:

1. wrap learnings seed in best-effort try/catch (action/main.ts) —
   the always-on seed block ran unconditionally and an unwrapped
   `seedLearningsFile` (mkdir + writeFile) failure (ENOSPC, EACCES,
   hostile sandbox) would unwind into the outer main() catch and flip
   an otherwise-successful run to "❌ Pullfrog failed" before the
   agent even started. asymmetric with `persistLearnings`'s own
   best-effort contract. wrap and log on failure; downstream
   consumers (`persistLearnings`, agent harnesses, `resolveInstructions`)
   already handle `learningsFilePath: undefined` cleanly.

2. refresh wiki/main.md — `resolveInstructions` parameter renamed
   from `learnings` to `learningsFilePath` in this PR; the data-flow
   diagram and the resolver dependency table both still showed the
   pre-refactor signature.

3. drop deleted `learnings.ts` from ROADMAP.md + RESEARCH.md
   "missing MCP tool tests" bullets — `action/mcp/learnings.ts` was
   removed in this PR; the bullets are otherwise still accurate.

utils/instructions.ts

@@ -12,7 +12,10 @@ interface InstructionsContext {
   modes: Mode[];
   agentId: AgentId;
   outputSchema?: Record<string, unknown> | undefined;
-  learnings: string | null;
+  /** absolute path to the seeded learnings tmpfile, or null when the file
+   * couldn't be seeded for some reason. main.ts always seeds, so in
+   * practice this is always set; the null case keeps the type honest. */
+  learningsFilePath: string | null;
 }
 
 interface PromptContext extends InstructionsContext {
@@ -350,11 +353,17 @@ function assembleFullPrompt(ctx: {
   procedure: string;
   eventContext: string;
   system: string;
-  learnings: string | null;
+  learningsFilePath: string | null;
   runtime: string;
 }): string {
-  const learningsSection = ctx.learnings
-    ? `************* LEARNINGS *************\n\n${ctx.learnings}`
+  // the LEARNINGS section is intentionally tiny — just the file path and a
+  // one-line "read it" instruction. embedding the contents would re-inflate
+  // the prompt every run (the previous design's failure mode) and clutter
+  // CI logs. the agent reads the file with its native file tool; the
+  // post-run reflection turn (action/agents/postRun.ts) is where editing
+  // is encouraged, with the prune-stale framing.
+  const learningsSection = ctx.learningsFilePath
+    ? `************* LEARNINGS *************\n\nRepo-level learnings accumulated by previous agent runs live at \`${ctx.learningsFilePath}\`. Read this file early and let the entries inform your approach (test commands, conventions, gotchas, etc.). The file may be empty if no learnings have been collected yet.`
     : "";
 
   const runtimeSection = `************* RUNTIME *************\n\n${ctx.runtime}`;
@@ -389,8 +398,8 @@ export function resolveInstructions(ctx: InstructionsContext): ResolvedInstructi
   if (eventContext)
     tocEntries.push({ label: "EVENT CONTEXT", description: "related PR/issue data" });
   tocEntries.push({ label: "SYSTEM", description: "persona, security, tools, workflow rules" });
-  if (pctx.learnings)
-    tocEntries.push({ label: "LEARNINGS", description: "repo-specific knowledge" });
+  if (pctx.learningsFilePath)
+    tocEntries.push({ label: "LEARNINGS", description: "repo-specific knowledge file path" });
   tocEntries.push({ label: "RUNTIME", description: "environment metadata" });
 
   const toc = buildToc(tocEntries);
@@ -401,7 +410,7 @@ export function resolveInstructions(ctx: InstructionsContext): ResolvedInstructi
     procedure,
     eventContext,
     system,
-    learnings: pctx.learnings,
+    learningsFilePath: pctx.learningsFilePath,
     runtime: pctx.runtime,
   });
 

utils/learnings.test.ts

@@ -0,0 +1,70 @@
+import { mkdtemp, rm, writeFile } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { afterEach, beforeEach, describe, expect, it } from "vitest";
+import {
+  LEARNINGS_FILE_NAME,
+  learningsFilePath,
+  readLearningsFile,
+  seedLearningsFile,
+} from "./learnings.ts";
+
+describe("learnings tmpfile round-trip", () => {
+  let dir: string;
+
+  beforeEach(async () => {
+    dir = await mkdtemp(join(tmpdir(), "pullfrog-learnings-test-"));
+  });
+
+  afterEach(async () => {
+    await rm(dir, { recursive: true, force: true });
+  });
+
+  it("seeds with existing learnings and reads them back verbatim", async () => {
+    const current = "- run tests with `pnpm -r test`\n- default branch is `main`";
+    const path = await seedLearningsFile({ tmpdir: dir, current });
+    expect(path).toBe(learningsFilePath(dir));
+    expect(path.endsWith(LEARNINGS_FILE_NAME)).toBe(true);
+    const read = await readLearningsFile(path);
+    expect(read).toBe(current);
+  });
+
+  it("seeds an empty file when the repo has no learnings yet", async () => {
+    // empty seed (vs scaffold-with-comment) keeps the byte-trim equality
+    // gate clean: an untouched first run reads back as "" and persistLearnings
+    // skips the API round-trip rather than writing a placeholder string into
+    // Repo.learnings.
+    const path = await seedLearningsFile({ tmpdir: dir, current: null });
+    const read = await readLearningsFile(path);
+    expect(read).toBe("");
+  });
+
+  it("returns null when the file is missing (treated as no-change by persist)", async () => {
+    const path = learningsFilePath(dir);
+    const read = await readLearningsFile(path);
+    expect(read).toBeNull();
+  });
+
+  it("trims whitespace so trailing newlines never trigger a spurious PATCH", async () => {
+    // editors commonly add a trailing newline on save. without trimming, a
+    // round-trip "read seed → save unchanged" would fail byte-equality and
+    // burn a LearningsRevision row on every run.
+    const current = "- one fact";
+    const path = await seedLearningsFile({ tmpdir: dir, current });
+    await writeFile(path, `${current}\n\n  `, "utf8");
+    const read = await readLearningsFile(path);
+    expect(read).toBe(current);
+  });
+
+  it("truncates content over the 10k server-side cap", async () => {
+    // server enforces MAX_LEARNINGS_LENGTH = 10_000. truncating client-side
+    // avoids a 400 round-trip and keeps the bytes the agent will see in the
+    // next run aligned with what the server actually stored.
+    const oversized = "x".repeat(11_000);
+    const path = await seedLearningsFile({ tmpdir: dir, current: null });
+    await writeFile(path, oversized, "utf8");
+    const read = await readLearningsFile(path);
+    expect(read).toBeTruthy();
+    expect(read?.length).toBe(10_000);
+  });
+});

utils/learnings.ts

@@ -0,0 +1,64 @@
+import { mkdir, readFile, writeFile } from "node:fs/promises";
+import { dirname, join } from "node:path";
+
+/**
+ * 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` from `Repo.learnings` (or empty
+ *      when the repo has none yet)
+ *   2. the agent reads the file at startup as part of its context, and may
+ *      edit it in place at end-of-run when prompted by the reflection turn
+ *   3. main.ts reads the file back at end-of-run and PATCHes
+ *      `/api/repo/[owner]/[repo]/learnings` if it changed (byte-trim equality
+ *      against the seed determines change detection)
+ *
+ * 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).
+ */
+
+export const LEARNINGS_FILE_NAME = "pullfrog-learnings.md";
+
+/** server-side cap mirrors `MAX_LEARNINGS_LENGTH` in
+ * `app/api/repo/[owner]/[repo]/learnings/route.ts`. truncating client-side
+ * keeps the PATCH from being rejected with a 400. */
+const MAX_LEARNINGS_LENGTH = 10_000;
+
+export function learningsFilePath(tmpdir: string): string {
+  return join(tmpdir, LEARNINGS_FILE_NAME);
+}
+
+/** seed the learnings file with the repo's current learnings, or an empty
+ * file when the repo has none yet. returns the absolute path. */
+export async function seedLearningsFile(params: {
+  tmpdir: string;
+  current: string | null;
+}): Promise<string> {
+  const path = learningsFilePath(params.tmpdir);
+  await mkdir(dirname(path), { recursive: true });
+  // empty file when no learnings exist yet — the agent reads it, sees
+  // nothing, and the LEARNINGS prompt section explains what the file is for.
+  // a header comment would risk being persisted as part of the first real
+  // edit, polluting the DB row with placeholder text.
+  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;
+  }
+  const trimmed = raw.trim();
+  if (trimmed.length > MAX_LEARNINGS_LENGTH) return trimmed.slice(0, MAX_LEARNINGS_LENGTH);
+  return trimmed;
+}