diff --git a/agents/postRun.ts b/agents/postRun.ts index d78d14a..c60f0ac 100644 --- a/agents/postRun.ts +++ b/agents/postRun.ts @@ -279,11 +279,18 @@ export function shouldRunReflection(mode: string | undefined): boolean { * 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 + * - facts anchored to moving repo state (PR / review / commit / branch + * refs, dates, version pins, line numbers) that decay within weeks * - sections growing into giant flat lists with no internal structure, * forcing future runs to read kilobytes to find one fact + * + * single litmus delivered in the prompt: "would a future run on this repo + * do its work better because this bullet exists?". tool-quirk workarounds + * are explicitly allowed when the agent burned calls discovering the + * quirk this run — recording the workaround prevents next run from + * repeating the waste. tradeoff: the same quirk gets duplicated across + * repos, so when a quirk is fixed upstream in tool descriptions the + * per-repo bullets go stale and we have no batch-invalidation path. */ export function buildLearningsReflectionPrompt(filePath: string): string { return [ @@ -296,18 +303,16 @@ export function buildLearningsReflectionPrompt(filePath: string): string { `- **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.`, + `the only test: would a future run on this repo do its work better because this bullet exists? useful for future runs in this repo — prevent wasted tool calls, rabbit holes, and mistakes.`, "", - `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.`, + `bullet hygiene:`, + `- one fact per line starting with \`- \`, ≤ 240 chars.`, + `- only add when high-confidence, broadly useful, evergreen.`, + `- prune wrong or low-signal bullets; merge overlaps; dedupe across sections.`, + "", + `don't anchor facts to repo state that will move: PR / review / commit / branch refs, dates, version pins, line numbers. state the rule directly. if it needs the anchor to be load-bearing, it isn't evergreen.`, + "", + `tool-quirk bullets are fine when you burned calls discovering the quirk and a future run would repeat them. write the workaround, not the war story.`, "", `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/internal/index.ts b/internal/index.ts index c9e20da..40a6816 100644 --- a/internal/index.ts +++ b/internal/index.ts @@ -45,6 +45,7 @@ export { isLeapingIntoActionCommentBody, LEAPING_INTO_ACTION_PREFIX, } from "../utils/leapingComment.ts"; +export { MAX_LEARNINGS_LENGTH, truncateAtLineBoundary } from "../utils/learningsTruncate.ts"; export type { CreateProgressCommentTarget, ProgressComment, diff --git a/mcp/issueEvents.ts b/mcp/issueEvents.ts index d4a856b..008288b 100644 --- a/mcp/issueEvents.ts +++ b/mcp/issueEvents.ts @@ -27,10 +27,11 @@ export function GetIssueEventsTool(ctx: ToolContext) { const relevantEventTypes = new Set(["cross_referenced", "referenced"]); const parsedEvents = events.flatMap((event) => { - // Filter to only events with an 'event' property and relevant types - if (!("event" in event) || !relevantEventTypes.has(event.event)) { - return []; - } + // octokit's timeline-event union includes members with `event?: + // string`, so `"event" in event` does not narrow it to defined. + // require a string before the Set.has() check. + if (!("event" in event) || typeof event.event !== "string") return []; + if (!relevantEventTypes.has(event.event)) return []; const baseEvent: Record = { event: event.event, diff --git a/utils/instructions.ts b/utils/instructions.ts index c723846..18b4001 100644 --- a/utils/instructions.ts +++ b/utils/instructions.ts @@ -397,11 +397,13 @@ export function buildLearningsSection(ctx: { 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).`; + // intro is neutral about whether content exists so an empty fresh-repo + // file doesn't open with "accumulated by previous agent runs" (false). + const intro = `The repo-level learnings file at \`${ctx.filePath}\` holds durable context (test commands, conventions, gotchas, architecture notes) maintained across runs.`; 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)}`; + ? "(no headings yet — the file is empty or contains a flat list. read the whole file if it has content. 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. The ranges below are a run-start snapshot: any edit shifts the line numbers of every later section, so re-read the TOC range you need before relying on it.\n\n${renderLearningsToc(ctx.headings)}`; return `************* LEARNINGS *************\n\n${intro}\n\n${tocBody}`; } diff --git a/utils/learnings.ts b/utils/learnings.ts index e9b4b04..e18a7e7 100644 --- a/utils/learnings.ts +++ b/utils/learnings.ts @@ -3,6 +3,9 @@ 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 @@ -34,15 +37,6 @@ import { log } from "./cli.ts"; 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. 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); } @@ -62,23 +56,6 @@ export async function seedLearningsFile(params: { 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. */ diff --git a/utils/learningsTocRender.test.ts b/utils/learningsTocRender.test.ts index 154410f..de405f2 100644 --- a/utils/learningsTocRender.test.ts +++ b/utils/learningsTocRender.test.ts @@ -82,6 +82,17 @@ describe("buildLearningsSection", () => { expect(out).not.toMatch(/\(L\d+-L\d+\)/); }); + it("intro phrasing does not assert prior runs — works for fresh empty repos too", () => { + const out = buildLearningsSection({ + filePath: "/tmp/run-1/pullfrog-learnings.md", + headings: [], + }); + // load-bearing: fresh repos have zero previous runs. the prior copy + // ("accumulated by previous agent runs") was a lie in that case. + expect(out).not.toContain("accumulated by previous agent runs"); + expect(out).toContain("maintained across runs"); + }); + it("renders the TOC inline with the file path and heading guidance", () => { const out = buildLearningsSection({ filePath: "/tmp/run-1/pullfrog-learnings.md", @@ -92,6 +103,12 @@ describe("buildLearningsSection", () => { expect(out).toContain("- Build & test (L1-L18)"); expect(out).toContain("- Architecture (L19-L60)"); expect(out).toContain("Each range starts at the section heading line"); + // re-read affordance: ranges reflect the run-start snapshot, so the + // agent needs an explicit nudge to re-read after any mid-run edits. + // mid-run edits shift the line numbers of every later section, not + // just the edited one — wording is explicit about that. + expect(out).toContain("run-start snapshot"); + expect(out).toContain("any edit shifts the line numbers of every later section"); // explicit "no hashes, no backticks" in the rendered list expect(out).not.toContain("- `## Build"); expect(out).not.toContain("`## Build"); diff --git a/utils/learningsTruncate.ts b/utils/learningsTruncate.ts new file mode 100644 index 0000000..a435b79 --- /dev/null +++ b/utils/learningsTruncate.ts @@ -0,0 +1,42 @@ +/** + * pure string helpers for capping and line-boundary-truncating the + * `Repo.learnings` body. lives in its own module (vs alongside + * `learnings.ts`) so the proprietary root app can re-export it through + * `action/internal/index.ts` without dragging the entire MCP type graph + * along — `learnings.ts` imports `ToolContext` for its runtime helpers, + * and pulling that into the SDK-facing `internal` barrel expands the + * type graph reachable from root `tsc` and `cf-worker-indexing` to every + * tool module under `action/mcp/`. keeping these helpers MCP-free is the + * cheap structural fix. + * + * see `action/utils/learnings.ts` for the full learnings-file lifecycle. + */ + +/** maximum size of `Repo.learnings` body in chars. action truncates the + * read-back BEFORE the PATCH to avoid sending an oversized payload; the + * server applies the same truncation as a defense-in-depth backstop (any + * caller that misses the client-side step would otherwise persist a + * mid-line tail, breaking the next-run TOC parse). + * + * 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 is governed by curation discipline rather than a + * tight byte ceiling. 100k holds ~400-500 short bullets. */ +export const MAX_LEARNINGS_LENGTH = 100_000; + +/** 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; +export 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); +}