diff --git a/agents/postRun.ts b/agents/postRun.ts index 35c64ea..2ffb74c 100644 --- a/agents/postRun.ts +++ b/agents/postRun.ts @@ -237,6 +237,16 @@ export function buildPostRunPrompt(issues: PostRunIssues): string { * the file is the single source of truth — there is no separate MCP tool * call. the server reads the file at end-of-run and persists any edits to * `Repo.learnings`. + * + * the prompt copy is shaped by repo-wide audits of the actual content the + * agent has been writing (issue #619 in pullfrog/app). recurring failure + * modes the framing pushes back on: + * - massive multi-paragraph "bullets" that are really mini-articles + * - PR-/review-/commit-/date-anchored facts that decay within weeks + * - rediscovery of pullfrog-tool quirks that belong in tool descriptions, + * not per-repo learnings + * - sections growing into giant flat lists with no internal structure, + * forcing future runs to read kilobytes to find one fact */ export function buildLearningsReflectionPrompt(filePath: string): string { return [ @@ -244,11 +254,25 @@ export function buildLearningsReflectionPrompt(filePath: string): string { "", `the rolling learnings file is at \`${filePath}\`. read it first if you haven't already, then edit it in place using your native file tools. the server reads this file at end-of-run and persists any changes — there is no tool to call.`, "", - `keep the file healthy:`, - `- only add bullets when the finding is high-confidence AND broadly useful. skip speculative, one-off, or "maybe" findings.`, - `- prune bullets that are clearly wrong, no longer relevant, or low-signal (rarely useful). a focused, accurate file beats a long stale one.`, - `- format: flat bullet list, one fact per line starting with \`- \`. deduplicate against existing entries — if a bullet covers the same fact, update it in place instead of adding a duplicate.`, - `- leave the file alone if you have nothing substantively new to add and the existing entries still look healthy. silence is a valid outcome — just reply "done" and stop.`, + `structure:`, + `- markdown hierarchy: \`## \` for top-level themes, \`### \` and deeper for sub-themes when a section grows. there is no fixed taxonomy — choose headings that fit THIS repo (e.g. for one repo \`## Migrations\` / \`## Local dev\` may make sense; for another, \`## API quirks\` / \`## Failure modes\`).`, + `- **no section over ~300 lines.** when a section is approaching that, split it: introduce \`### \` subsections grouping related bullets, or hoist a coherent group into a new top-level \`## \` section. granular sections mean future runs read targeted line ranges instead of slurping the whole file. this is the most important hygiene rule on long-lived repos.`, + `- if you find a flat unstructured list (legacy content from before this format), restructure it: read it, group related bullets, rewrite the file with \`## \` / \`### \` headings around them. don't preserve bad structure — fix it.`, + "", + `bullet hygiene:`, + `- one fact per line starting with \`- \`. each bullet is ONE specific durable fact, not a paragraph or essay.`, + `- aim for ≤ 240 chars per bullet. longer bullets are almost always mixing multiple facts that should be split, or burying the durable claim under PR-specific context that should be cut.`, + `- only add bullets when the finding is high-confidence AND broadly useful AND will still be true in 3+ months. skip speculative, one-off, or "maybe" findings.`, + `- prune bullets that are clearly wrong, no longer relevant, or low-signal. a focused, accurate file beats a long stale one. compressing two overlapping bullets into one tighter bullet counts as progress.`, + `- deduplicate against existing entries (in any section) — if a bullet covers the same fact, update it in place instead of adding a duplicate.`, + "", + `do NOT add bullets for:`, + `- pullfrog tool quirks (e.g. "\`shell\` timeout is in milliseconds", "\`git\` args must be a JSON array", "\`create_pull_request_review\` drops out-of-hunk comments", "\`push_branch\` may report timeout when push succeeded"). these are universal across repos and belong in the tool descriptions — flag the gap rather than hoarding the workaround per-repo.`, + `- references to specific PR numbers, review IDs, commit SHAs, branch names, or person handles ("PR #595 introduced X", "flagged in review 12345", "as of commit abc123"). repo state changes; these decay into noise within weeks.`, + `- dated assertions ("as of May 2026", "currently...", "for now..."). if a fact needs a date to be true, it isn't durable enough to belong here.`, + `- play-by-play of what THIS run did. learnings are for the NEXT run, not a retrospective.`, + "", + `if you have nothing substantively new to add AND the existing entries still look healthy and well-structured, leave the file alone — just reply "done" and stop. silence is a valid outcome.`, ].join("\n"); } diff --git a/main.ts b/main.ts index 3ee0d3c..7bf799c 100644 --- a/main.ts +++ b/main.ts @@ -770,12 +770,10 @@ export async function main(): Promise { current: runContext.repoSettings.learnings, }); toolState.learningsFilePath = learningsPath; - try { - toolState.learningsSeed = await readFile(learningsPath, "utf8"); - } catch { - // intentionally empty — learningsSeed stays undefined, persistLearnings - // will treat seed as "" and persist any non-empty content - } + // file on disk is the verbatim DB body, so the seed used for + // change-detection is just `current ?? ""` (trimmed). persistLearnings + // byte-compares against the trimmed read-back to skip no-op PATCHes. + toolState.learningsSeed = (runContext.repoSettings.learnings ?? "").trim(); log.info( `» learnings seeded at ${learningsPath} (existing=${runContext.repoSettings.learnings ? "yes" : "no"})` ); @@ -839,6 +837,7 @@ export async function main(): Promise { agentId, outputSchema, learningsFilePath: toolState.learningsFilePath ?? null, + learningsHeadings: runContext.repoSettings.learningsHeadings, }); const logParts = [ instructions.eventInstructions diff --git a/mcp/checkout.ts b/mcp/checkout.ts index 18e64c7..bbf948a 100644 --- a/mcp/checkout.ts +++ b/mcp/checkout.ts @@ -593,7 +593,9 @@ export function CheckoutPrTool(ctx: ToolContext) { name: "checkout_pr", description: "Checkout a pull request branch locally. This fetches the PR branch and sets up push configuration for fork PRs. " + - "Returns diffPath pointing to the formatted diff file.", + "Returns diffPath pointing to the formatted diff file. " + + "Transient fetch timeouts are common — retry the same call up to a few times before treating the failure as terminal. " + + "If the error mentions `.git/shallow.lock: File exists` or `.git/index.lock: File exists`, that's a stale lock from a prior timed-out fetch — remove it via the shell tool (`rm -f .git/shallow.lock .git/index.lock`) and retry.", parameters: CheckoutPr, execute: execute(async ({ pull_number }) => { const prResponse = await ctx.octokit.rest.pulls.get({ diff --git a/mcp/git.ts b/mcp/git.ts index 69af32f..fe9ea99 100644 --- a/mcp/git.ts +++ b/mcp/git.ts @@ -221,7 +221,8 @@ export function PushBranchTool(ctx: ToolContext) { "If specifying branchName, use the LOCAL branch name (e.g., 'pr-1'), not the remote branch name. " + "The correct remote and remote branch are determined automatically from branch config set by checkout_pr. " + "Requires a clean working tree. Runs the repository prepush hook (if configured) before the network push — hook failure means tests/lint or similar in that script failed, not necessarily a Pullfrog timeout. " + - "Never force push unless explicitly requested. Pushes to the default branch are blocked in restricted mode.", + "Never force push unless explicitly requested. Pushes to the default branch are blocked in restricted mode. " + + "If the response reports a timeout, the underlying push may have actually succeeded — verify with `git log origin/` (or this tool with command 'log') before retrying, otherwise you'll push a duplicate.", parameters: PushBranch, execute: execute(async ({ branchName, force }) => { // permission check @@ -444,7 +445,12 @@ const subcommandPattern = regex("^[a-z][a-z0-9-]*$"); const Git = type({ command: type(subcommandPattern).describe("Git command (e.g., 'status', 'log', 'diff')"), - args: type.string.array().describe("Additional arguments for the git command").optional(), + args: type.string + .array() + .describe( + 'Additional arguments for the git command, as a JSON array of strings (NOT a single string). e.g. args: ["HEAD"], args: ["--oneline", "-20"]. Passing a single string fails validation.' + ) + .optional(), }); export function GitTool(ctx: ToolContext) { diff --git a/mcp/review.ts b/mcp/review.ts index b426bf0..658313b 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -320,14 +320,16 @@ export const CreatePullRequestReview = type({ ) .optional(), commit_id: type.string - .describe("Optional SHA of the commit being reviewed. Defaults to latest.") + .describe( + "Optional SHA of the commit being reviewed. Defaults to latest. Must be the FULL 40-character SHA — abbreviated SHAs are rejected by GitHub with `422 Unprocessable Entity`. The PR-synchronize event payload's `head_sha` is already full-length." + ) .optional(), comments: type({ path: type.string.describe( "The file path to comment on (relative to repo root). Must be a file that appears in the PR diff." ), line: type.number.describe( - "Line number to comment on. For multi-line ranges, this is the end line. Use NEW column from diff format." + "Line number to comment on. For multi-line ranges, this is the end line. Use NEW column from diff format. MUST sit inside a `@@` hunk in the PR diff — anchors on context-only or untouched lines are dropped silently (the rest of the review still posts; dropped entries are reported under `droppedComments` in the response)." ), side: type .enumerated("LEFT", "RIGHT") @@ -345,7 +347,7 @@ export const CreatePullRequestReview = type({ .optional(), start_line: type.number .describe( - "Start line for multi-line comment ranges. Omit for single-line comments. The range [start_line, line] defines which lines a suggestion replaces." + "Start line for multi-line comment ranges. Omit for single-line comments. The range [start_line, line] defines which lines a suggestion replaces. BOTH `start_line` AND `line` must sit inside the SAME `@@` hunk — a `start_line` outside the hunk causes the whole comment to be dropped even when `line` is valid. If you need to comment on context just above/below a hunk, shrink the range to a single line that is provably modified." ) .optional(), }) diff --git a/mcp/shell.ts b/mcp/shell.ts index 66f0375..2d9f819 100644 --- a/mcp/shell.ts +++ b/mcp/shell.ts @@ -15,7 +15,9 @@ import { execute, tool } from "./shared.ts"; export const ShellParams = type({ command: "string", description: "string", - "timeout?": "number", + "timeout?": type.number.describe( + "Timeout in MILLISECONDS (not seconds). Default 30000 (30s), max 120000 (2m). e.g. timeout: 180000 for 3 minutes; timeout: 180 means 180ms and will kill the process almost immediately." + ), "working_directory?": "string", "background?": "boolean", }); diff --git a/utils/instructions.ts b/utils/instructions.ts index c7b6419..8ecc482 100644 --- a/utils/instructions.ts +++ b/utils/instructions.ts @@ -4,6 +4,7 @@ import { encode as toonEncode } from "@toon-format/toon"; import { type AgentId, formatMcpToolRef, type PayloadEvent, pullfrogMcpName } from "../external.ts"; import type { Mode } from "../modes.ts"; import type { ResolvedPayload } from "./payload.ts"; +import type { LearningsHeading } from "./runContext.ts"; import type { RunContextData } from "./runContextData.ts"; interface InstructionsContext { @@ -16,6 +17,10 @@ interface InstructionsContext { * 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; + /** server-parsed TOC for the body of the learnings tmpfile. rendered + * inline into the LEARNINGS prompt section so the agent can `read_file` + * targeted line ranges instead of pulling the whole file into context. */ + learningsHeadings: LearningsHeading[]; } interface PromptContext extends InstructionsContext { @@ -369,6 +374,39 @@ export interface ResolvedInstructions { runtime: string; } +/** render the heading list as an indented bullet TOC. ranges shown in + * parentheses (`(L3-L18)`); the start line is always the heading line + * itself, so reading the listed range gives the agent the heading + + * body together. shallowest heading depth in the body sits at the root + * column; deeper levels indent by `(depth - rootDepth) * 2` spaces. */ +export function renderLearningsToc(headings: LearningsHeading[]): string { + if (headings.length === 0) return ""; + const rootDepth = Math.min(...headings.map((h) => h.depth)); + return headings + .map((h) => { + const indent = " ".repeat((h.depth - rootDepth) * 2); + return `${indent}- ${h.title} (L${h.startLine}-L${h.endLine})`; + }) + .join("\n"); +} + +/** assemble the LEARNINGS prompt section: file path + intro + either + * the rendered heading TOC (when the body has structure) or a no-headings + * affordance pointing the agent at the reflection turn for restructuring. + * empty string when the seed step failed and there's no path to surface. */ +export function buildLearningsSection(ctx: { + filePath: string | null; + headings: LearningsHeading[]; +}): string { + if (!ctx.filePath) return ""; + const intro = `Repo-level learnings accumulated by previous agent runs live at \`${ctx.filePath}\`. Use this file as durable context (test commands, conventions, gotchas, architecture notes).`; + const tocBody = + ctx.headings.length === 0 + ? "(no headings yet — file is empty or a flat list. read the whole file. during the post-run reflection turn, structure it with `## ` / `### ` headings so future runs can read targeted ranges.)" + : `Read targeted line ranges via your native file tool — do NOT slurp the whole file. Each range starts at the section heading line, so reading the range gives you heading + body together.\n\n${renderLearningsToc(ctx.headings)}`; + return `************* LEARNINGS *************\n\n${intro}\n\n${tocBody}`; +} + function assembleFullPrompt(ctx: { toc: string; task: string; @@ -376,17 +414,18 @@ function assembleFullPrompt(ctx: { eventContext: string; system: string; learningsFilePath: string | null; + learningsHeadings: LearningsHeading[]; runtime: string; }): string { - // 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.` - : ""; + // server-parsed TOC is rendered inline so the agent can target line + // ranges via its native file tool. the file body itself is never + // inlined — that would re-inflate context every run and clutter CI + // logs. post-run reflection (action/agents/postRun.ts) is where + // editing is encouraged. + const learningsSection = buildLearningsSection({ + filePath: ctx.learningsFilePath, + headings: ctx.learningsHeadings, + }); const runtimeSection = `************* RUNTIME *************\n\n${ctx.runtime}`; @@ -421,7 +460,10 @@ export function resolveInstructions(ctx: InstructionsContext): ResolvedInstructi tocEntries.push({ label: "EVENT CONTEXT", description: "related PR/issue data" }); tocEntries.push({ label: "SYSTEM", description: "persona, security, tools, workflow rules" }); if (pctx.learningsFilePath) - tocEntries.push({ label: "LEARNINGS", description: "repo-specific knowledge file path" }); + tocEntries.push({ + label: "LEARNINGS", + description: "repo-specific knowledge file path + heading TOC", + }); tocEntries.push({ label: "RUNTIME", description: "environment metadata" }); const toc = buildToc(tocEntries); @@ -433,6 +475,7 @@ export function resolveInstructions(ctx: InstructionsContext): ResolvedInstructi eventContext, system, learningsFilePath: pctx.learningsFilePath, + learningsHeadings: pctx.learningsHeadings, runtime: pctx.runtime, }); diff --git a/utils/learnings.test.ts b/utils/learnings.test.ts index 93b80a4..4429c4d 100644 --- a/utils/learnings.test.ts +++ b/utils/learnings.test.ts @@ -1,4 +1,4 @@ -import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; @@ -20,51 +20,68 @@ describe("learnings tmpfile round-trip", () => { 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`"; + it("writes the verbatim DB body to disk and reads it back unchanged", async () => { + const current = [ + "## Build & test", + "", + "- run tests with `pnpm -r test`", + "", + "## Architecture", + "", + "- workers in `worker/`", + ].join("\n"); 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); + + expect(await readFile(path, "utf8")).toBe(current); + expect(await readLearningsFile(path)).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. + it("seeds an empty file when the repo has no learnings yet, round-trip is empty string", async () => { const path = await seedLearningsFile({ tmpdir: dir, current: null }); - const read = await readLearningsFile(path); - expect(read).toBe(""); + expect(await readFile(path, "utf8")).toBe(""); + expect(await readLearningsFile(path)).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(); + expect(await readLearningsFile(learningsFilePath(dir))).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"; + it("trims trailing whitespace so editor newlines never trigger a spurious PATCH", async () => { + const current = "## Build & test\n\n- 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); + expect(await readLearningsFile(path)).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); + it("truncates over-cap bodies at the last newline boundary so the next-seed TOC parse stays clean", async () => { + const padding = `${"x".repeat(80)}\n`.repeat(1300); + const oversized = `## Build & test\n\n${padding}`; 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); + expect(read?.length).toBeLessThanOrEqual(100_000); + const tailLine = read?.split("\n").pop() ?? ""; + expect(/^x+$/.test(tailLine)).toBe(true); + expect(tailLine.length).toBe(80); + }); + + it("falls back to a hard truncate when the only newline is far above the cap (giant single line)", async () => { + const oversized = `## Build & test\n${"x".repeat(110_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(100_000); + expect(read?.startsWith("## Build & test\n")).toBe(true); + }); + + it("preserves legacy free-text without scaffolding or wrapping", async () => { + const legacy = "- this is some old free-text bullet\n- another one"; + const path = await seedLearningsFile({ tmpdir: dir, current: legacy }); + expect(await readFile(path, "utf8")).toBe(legacy); + expect(await readLearningsFile(path)).toBe(legacy); }); }); diff --git a/utils/learnings.ts b/utils/learnings.ts index 17b45ec..dc0ed7d 100644 --- a/utils/learnings.ts +++ b/utils/learnings.ts @@ -7,47 +7,75 @@ import { dirname, join } from "node:path"; * 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) + * 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"; /** 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; + * keeps the PATCH from being rejected with a 400. raised from 10k → 100k + * once the TOC affordance landed: with line-range reads via the + * server-parsed TOC the agent doesn't ingest the whole file, so the cap + * can grow to whatever curation discipline allows. 100k holds ~400-500 + * short bullets. */ +const MAX_LEARNINGS_LENGTH = 100_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. */ +/** 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 { 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; } +/** truncate at the last newline boundary before `cap` so we don't leave + * a partial line at the tail (a half-truncated `## Headi` confuses the + * server's next-seed TOC parse and shrinks visible structure). falls + * back to a hard `slice` when the line boundary would discard a large + * run of content — i.e. when the tail of `head` is one giant line (rare: + * minified pastes, fenced log dumps). losing a partial last line is + * preferable to losing kilobytes of body. */ +const TRUNCATION_LINE_BOUNDARY_TOLERANCE = 4096; +function truncateAtLineBoundary(body: string, cap: number): string { + if (body.length <= cap) return body; + const head = body.slice(0, cap); + const lastNewline = head.lastIndexOf("\n"); + if (lastNewline <= 0) return head; + if (cap - lastNewline > TRUNCATION_LINE_BOUNDARY_TOLERANCE) return head; + return head.slice(0, lastNewline); +} + /** 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. */ @@ -58,7 +86,5 @@ export async function readLearningsFile(path: string): Promise { } catch { return null; } - const trimmed = raw.trim(); - if (trimmed.length > MAX_LEARNINGS_LENGTH) return trimmed.slice(0, MAX_LEARNINGS_LENGTH); - return trimmed; + return truncateAtLineBoundary(raw.trim(), MAX_LEARNINGS_LENGTH); } diff --git a/utils/learningsTocRender.test.ts b/utils/learningsTocRender.test.ts new file mode 100644 index 0000000..154410f --- /dev/null +++ b/utils/learningsTocRender.test.ts @@ -0,0 +1,99 @@ +import { describe, expect, it } from "vitest"; +import { buildLearningsSection, renderLearningsToc } from "./instructions.ts"; +import type { LearningsHeading } from "./runContext.ts"; + +const h = (depth: 1 | 2 | 3 | 4 | 5 | 6, title: string, startLine: number, endLine: number) => ({ + depth, + title, + startLine, + endLine, +}); + +describe("renderLearningsToc", () => { + it("renders flat h2 list with parenthesized ranges, no hashes or backticks", () => { + const headings: LearningsHeading[] = [ + h(2, "Build & test", 1, 18), + h(2, "Architecture", 19, 60), + ]; + expect(renderLearningsToc(headings)).toBe( + `- Build & test (L1-L18) +- Architecture (L19-L60)` + ); + }); + + it("indents deeper headings 2 spaces per depth level past the shallowest", () => { + const headings: LearningsHeading[] = [ + h(2, "Build & test", 1, 42), + h(3, "Local", 3, 18), + h(3, "CI", 19, 42), + h(2, "Architecture", 43, 210), + h(3, "Background workers", 80, 210), + ]; + expect(renderLearningsToc(headings)).toBe( + `- Build & test (L1-L42) + - Local (L3-L18) + - CI (L19-L42) +- Architecture (L43-L210) + - Background workers (L80-L210)` + ); + }); + + it("treats the shallowest depth as the root column when no h2 is present", () => { + const headings: LearningsHeading[] = [h(3, "Only h3", 1, 5), h(4, "Sub h4", 2, 5)]; + expect(renderLearningsToc(headings)).toBe( + `- Only h3 (L1-L5) + - Sub h4 (L2-L5)` + ); + }); + + it("supports depths up through h6 with stable 2-space indent steps", () => { + const headings: LearningsHeading[] = [ + h(2, "Two", 1, 10), + h(3, "Three", 2, 10), + h(4, "Four", 3, 10), + h(5, "Five", 4, 10), + h(6, "Six", 5, 10), + ]; + expect(renderLearningsToc(headings)).toBe( + `- Two (L1-L10) + - Three (L2-L10) + - Four (L3-L10) + - Five (L4-L10) + - Six (L5-L10)` + ); + }); +}); + +describe("buildLearningsSection", () => { + it("returns empty string when no file path (seed step failed)", () => { + expect(buildLearningsSection({ filePath: null, headings: [] })).toBe(""); + }); + + it("renders the no-headings affordance when the body has no structure", () => { + const out = buildLearningsSection({ + filePath: "/tmp/run-1/pullfrog-learnings.md", + headings: [], + }); + expect(out).toContain("************* LEARNINGS *************"); + expect(out).toContain("/tmp/run-1/pullfrog-learnings.md"); + expect(out).toContain("no headings yet"); + expect(out).toContain("structure it with"); + // does not include a TOC list when there are no headings + expect(out).not.toMatch(/\(L\d+-L\d+\)/); + }); + + it("renders the TOC inline with the file path and heading guidance", () => { + const out = buildLearningsSection({ + filePath: "/tmp/run-1/pullfrog-learnings.md", + headings: [h(2, "Build & test", 1, 18), h(2, "Architecture", 19, 60)], + }); + expect(out).toContain("************* LEARNINGS *************"); + expect(out).toContain("/tmp/run-1/pullfrog-learnings.md"); + expect(out).toContain("- Build & test (L1-L18)"); + expect(out).toContain("- Architecture (L19-L60)"); + expect(out).toContain("Each range starts at the section heading line"); + // explicit "no hashes, no backticks" in the rendered list + expect(out).not.toContain("- `## Build"); + expect(out).not.toContain("`## Build"); + }); +}); diff --git a/utils/runContext.ts b/utils/runContext.ts index 9235fb7..ffb7fae 100644 --- a/utils/runContext.ts +++ b/utils/runContext.ts @@ -9,6 +9,22 @@ export interface Mode { prompt: string; } +/** + * server-parsed TOC entry for `Repo.learnings`. depth is 1-6 (h1-h6), + * line numbers are 1-indexed against the raw body. computed by + * `parseLearningsHeadings` in `utils/learningsToc.ts` (server side) and + * shipped over the run-context JSON boundary; the canonical declaration + * lives there. duplicated here because the action runtime can't reach + * across into the proprietary root-level codebase, and the JSON wire + * means typecheck can't enforce shape equality across both sides. + */ +export interface LearningsHeading { + depth: 1 | 2 | 3 | 4 | 5 | 6; + title: string; + startLine: number; + endLine: number; +} + export interface RepoSettings { model: string | null; modes: Mode[]; @@ -21,6 +37,7 @@ export interface RepoSettings { prApproveEnabled: boolean; modeInstructions: Record; learnings: string | null; + learningsHeadings: LearningsHeading[]; envAllowlist: string | null; } @@ -61,6 +78,7 @@ const defaultSettings: RepoSettings = { prApproveEnabled: false, modeInstructions: {}, learnings: null, + learningsHeadings: [], envAllowlist: null, }; @@ -127,6 +145,7 @@ export async function fetchRunContext(params: { postCheckoutScript: data.settings?.postCheckoutScript ?? null, prepushScript: data.settings?.prepushScript ?? null, stopScript: data.settings?.stopScript ?? null, + learningsHeadings: data.settings?.learningsHeadings ?? [], }, apiToken: data.apiToken, oss: data.oss ?? false,