AGENTS.md

AGENTS.md

Project conventions for AI-assisted development on umm-actually.

What this project is

A Docker-based GitHub Action that reviews pull requests with an LLM via OpenRouter and posts one consolidated PR review with inline findings. The review is diff-anchored but not diff-bounded: changed code is traced into its callers, and pre-existing bugs in traced code are valid findings.

Structure

.claude/                   # committed Claude Code session hooks (the rest of .claude/ is gitignored)
  settings.json            # SessionStart + PostToolUse(EnterWorktree) → install-deps.sh
  hooks/
    install-deps.sh        # loads nvm, runs npm ci when a fresh clone or worktree has no node_modules
action.yml                 # action metadata — inputs/outputs, runs.using: docker
Dockerfile                 # multi-stage: build (tsc) → slim runtime
fixtures/                  # test fixtures (event payloads, sample diff, LLM responses)
src/
  main.ts                  # entrypoint — collects/validates inputs, wires clients into orchestrate, sets outputs, closes the check run on cancellation signals, exits explicitly
  config.ts                # action inputs → validated ActionConfig
  logger.ts                # structured JSON logger — levels, child contexts, lazy props
  github/                  # GitHub I/O: event payload → PrContext, octokit wrappers (diff fetch, review posting)
  openrouter/              # OpenRouter I/O: @openrouter/sdk wrapper, per-attempt and shared review deadlines, retry and fallback ladder (HTTP errors, timeouts, invalid structured output), cost summary
  diff/                    # pure transforms over parse-diff output: git-quoted path decoding, annotation, commentable lines, diff-level exclusion (patterns, gitattributes linguist rules, wildcard safety cap)
  context/                 # workspace I/O: conventions file, root .gitattributes, changed files, import-trace scan, doc-mention scan, priority docs
  review/                  # pure review logic: finding schema, phases + stage dispatch, prompt, non-finding filter, unknown-file filter, cross-phase merge, path normalization, selection, comment mapping, markdown code spans, title similarity, context notes, summary
  orchestrate.ts           # pipeline + createPromptedGenerateFindings — fully testable with stub clients

Module layering

Pure leaves (diff/, review/) → I/O clients (github/, openrouter/, context/) → composition (orchestrate.ts). Only main.ts touches real process.env and constructs SDK clients. A pure module importing an I/O module is a backwards dependency and a bug — enforced by a @typescript-eslint/no-restricted-imports block in eslint.config.ts (type-only imports are allowed; they're erased at compile time).

Use the official SDKs — don't hand-roll what @actions/core, @actions/github (octokit), @openrouter/sdk, or parse-diff already do. Before writing any parsing or validation helper, check the SDK's utility surface first: @actions/core ships getBooleanInput (strict YAML 1.2 booleans), which replaced a hand-rolled boolean-string Zod transform here. Values an SDK util already parses arrive in RawInputs pre-parsed from the collection boundary (main.ts) — "validation lives in config.ts" is about where our rules live, not a reason to reimplement the platform's. Zod validates the things that are genuinely ours: LLM structured output and action config.

Types are colocated with the code that uses them — no standalone types files. Prefer SDK-provided types over redefining shapes.

Code style

• Functional over OOP. Arrow functions over function declarations.
• Factory/closure pattern for stateful modules; single namespace export for cohesive service surfaces (githubClient.submitReview(…)).
• type over interface. TypeScript strict mode. node: prefix for built-ins.
• Explicit return types on exports. No any. No as or ! — use runtime guards or schema validation to narrow. Truthy/falsy checks over explicit !== undefined comparisons — use if (value) not if (value !== undefined) unless distinguishing undefined from other falsy values (null, 0, "", false) actually matters for correctness.
• Model states in the type system. A result with modes is a discriminated union with never exhaustiveness, not optional fields documented as "present only in mode X". One discriminant per fact, so a new case becomes a union member, never a second result shape with a converter. A param read only for truthiness is typed boolean.
• Immutable by default; avoid let. A reduce must return a new accumulator each step — never mutate-and-return. When mutation is genuinely needed, add a comment justifying it. Prefer the non-mutating array methods (toSorted, toSpliced, with, at(-1), findLast). A helper never mutates its inputs; it returns the new value and types collection params as readonly views (ReadonlyArray, ReadonlySet). Readability gates any refactor toward a more functional shape.
• Explicit names over abbreviations, everywhere — params, callbacks, locals. Value-returning functions name what they return (getX, not ensureX); side-effect functions say what they do. A generic destructured key keeps its source (const { on: modifiedOn } = filters.modified).
• Wire names stay at the boundary. Action inputs are snake_case and ActionConfig is camelCase; map once in main.ts/config.ts.
• Early returns over nested if/else. When a function has a primary path and a secondary path (e.g. first-run vs re-run), return early from the simpler branch so the remaining code flows linearly without nesting. Extract multi-clause conditionals into named booleans. Name booleans for the affirmative state.
• Never a chained ternary (a second ? inside a ternary) or a let assigned through if/else branches — use one if … return per branch in a small helper, or one named const per decision step. while (condition) over for (;;) + break when the exit condition fits the loop head.
• Blank lines separate logical steps inside a function — each declaration-plus-comment block, guard, or step gets one, and a comment never sits directly under the previous statement. ESLint enforces the declaration-before-if case; the rest is on the author.
• Block bodies {} for any multiline function response — expression bodies only for one-liners. A multi-clause boolean spanning lines gets { return (...) }; guard chains get explicit early returns, never a chained ||/ternary expression body.
• Named params for functions with more than two args or adjacent same-typed args.
• Data-layer and I/O functions take (params, logger) — logger is required.
• process.env is never read via raw property access. Action inputs (INPUT_*) go through @actions/core getInput/getBooleanInput; the event name and payload come from @actions/github context (it reads GITHUB_EVENT_NAME/GITHUB_EVENT_PATH and parses the JSON — don't hand-read those); remaining ambient environment (GITHUB_WORKSPACE, …) goes through the env-var package (envVar.from(env).get("NAME").required().asString()), with the env record injectable for tests.
• Comment decision at write time (use /** */; only when earned): (1) Can a reader understand this from name + params + return type? → no comment — this is most functions. (2) Something non-obvious? → one-line JSDoc stating the constraint the signature doesn't convey. (3) Does the JSDoc restate the function name? → delete it. (4) More than 2 lines? → pick the format the reader absorbs quickest (bullets, numbered steps), never multi-paragraph prose. Inline comments go directly above the relevant line — don't stuff implementation details into the docstring. Regex constants get doc comments. A guard's comment states the scenario, the mechanism, and what breaks without it. Comments carry durable rationale only, never transition history or decision narrative. Every chosen number in workflow or action YAML (caps, timeouts) gets its why directly above it. Comment prose uses short complete sentences, with no colon-hinged labels and no verbless fragments.
• Named constants over bare magic literals, with the one-line why at the definition. Scope constants to where they're used — module level overstates visibility when only one function needs the value.
• A boolean mode param means the function does two things — split into two single-responsibility functions; the caller owns the gating.
• Decomposition must earn its seams, and DRY targets shared decisions, not repeated text. Extract when code holds one decision that must change in several places together, or to name a decision worth naming. A one-expression wrapper over an SDK or Zod call stays inline even with several call sites (z.boolean().default(true) at each input, not a booleanOrDefault(true) helper), because each site carries its own value and the reader wants the logic at the line. A parameter a function only forwards means the seam is wrong — compile configuration once into a factory/closure and pass the resulting collaborator, never thread config through layers that don't read it. One concern stays in one module: files that only ever import each other are fragmentation, not separation — a module boundary needs an independent consumer.
• Type-only imports over structural duplication — don't clone interfaces for "module purity"; type imports are erased at compile time.
• Extract multi-step .map()/.reduce() callbacks into named functions when they nest chains or build intermediates. Conditional spreads and .filter(Boolean) are both fine — pick whichever reads clearer; don't convert mechanically. Name non-trivial .filter() predicates.
• Per-operation try/catch — each catch encloses one operation with one failure meaning. Broad catch-alls are banned. Every catch logs or re-throws; a swallowed error is worse than an uncaught one.
• Throw on an unreachable null instead of a ?? "" sentinel that silently degrades data.
• Required inputs enforced at every entry point — fail fast at boot/load. Making an already-expected value mandatory is a bug fix, not a breaking change.
• The declared contract and the runtime say the same thing. action.yml and the README never advertise a default or a rejection that config.ts doesn't perform.
• For an external limit that varies per endpoint (a model's context window), parse the real limit from the rejection and retry to fit. A documented, uniform limit can be a constant.
• LLM-facing contracts (prompt sections, the finding schema) omit an optional field whose absence is unambiguous — no always-present zero placeholders. Keep the field when 0/[] is a legitimate value or it carries structural attribution.
• Platform built-ins before hand-rolled parsing (URL.parse, node:path); with none, exact-value comparison beats string surgery. Parse genuinely varying structured strings with a declarative, doc-commented regex (named groups), not index arithmetic.
• Boolean(x) over !!x. TS ≥5.5 infers .filter() predicates from bare comparisons — omit explicit type-guard annotations on .filter() with a bare null/undefined check.
• Relative imports use explicit .js extensions (ESM runtime requirement).
• Vet a new dependency's maintainers, release history and downloads before adopting it; at negligible adoption with a small core, prefer a local implementation. Pin dependency overrides to exact versions.
• Simple over clever: before settling, ask whether fewer moving parts do the job. A dead-code claim is proven by enumerating every trigger, not by log silence.

Test conventions

• Tests read as a behavioral spec: one focused it() per behavior, named so a failure identifies the regression without reading the body.
• const per test via factory helpers; beforeEach only when per-test creation is genuinely impractical.
• Exact assertions over loose matchers; assert whole values over substrings when output is deterministic. When fixtures and stubs produce deterministic results, assert the entire return value or call params — not just individual fields. For large deterministic strings (prompts, rendered output), assert the full section or constant in one toContain — not multiple fragments that each check a phrase. Asserting fragments is the cheap option; asserting the whole value catches drift in formatting, structure, and attribution that field-level checks miss.
• Two-bar rule: a test must (1) fail when the behavior breaks and (2) pass only because the intended behavior occurred. Five traps against bar 2: silent no-op (assert the trigger happened, not just that state was retained), wrong-error (rejects.toThrow() with no argument matches ANY error — assert the specific message), early-return (assert a side effect only the intended path produces), wrong-item (assert the specific expected item, not just "something came back"), coincidental equality (when production and test read the same source, both being undefined passes — set a predictable value and assert it exactly; expect.any(String) still matches "").
• Deterministic error messages get exact assertions, ordered results get a positional toEqual, and objects are matched whole, not with objectContaining. A looser neighboring test is never the standard; write the new test to this bar and surface the older one as a fix candidate.
• A drift-catching test owns its expected value. Define the expected prompt text or default in the test file; importing the production constant makes both sides drift together.
• Assert stub interactions with toHaveBeenCalledTimes(1) + toHaveBeenCalledWith(exactArgs), not mock.calls[i][j] readback. Derive an expected value test-side; never read it back out of the call log.
• Register cleanup at creation (onTestFinished, afterEach), because cleanup at the end of a test body is skipped when an assertion throws. Use Vitest helpers (vi.mocked, vi.restoreAllMocks) over module-level let plumbing.
• it.each is only for identical assertion shapes, with labeled case objects and $label in the title. Structurally different checks get separate it() blocks.
• Cover error paths and boundaries (zero, one, empty), not just the happy path. For every guard, ask what unrelated change could silently undo it, and write the test that catches that erosion.
• Test helpers and stubs earn their place by removing real duplication. A stub must not allow a condition production can't produce, and must keep the dimension under test (order, timing, size) varied.
• For retry and deadline logic, prefer a controllable seam (an injected timeout, stubbed client outcomes) over fake timers. When timing is the contract, assert outcomes after advancing the clock, never the tick-by-tick schedule.
• Production code never carries test-serving structure (extra branches or cache keys that exist only to isolate tests); tests own isolation through factories. Production type rules apply in tests: no !.
• Test a workflow shell snippet under bash -e before committing — Actions runs run: steps with errexit, so use if/then/fi over [ test ] && cmd.
• When unsure a test can fail for the right reason, mutate the production code and watch it fail — for that specific reason.
• Never decompose: toHaveLength(1) + index-based checks is weaker than one toEqual on the mapped result — the decomposed form misses extra items, ordering, and unexpected properties.
• Filter tests seed data both inside AND outside the filter — exclusion is half the behavior.
• Stub SDK clients are plain objects injected through factories — no HTTP mocking libraries.
• Every test file maps to a real source module — don't spawn a standalone test file to mock differently.
• Fixtures live in fixtures/ and are shared across test files.

Logging & observability

• Use logger.ts, never console.log — logger params are required, not optional.
• Thread the caller's logger into domain functions so deep events inherit request context.
• Levels: error (failed, needs attention), warn (degraded but handled — state the fallback taken), info (state changes an operator cares about), debug (diagnostic detail, off by default). Per-item loops log debug; their summary logs info.
• Never log PII, credentials, tokens, or secrets — log identifiers, not identity payloads. Redact via destructuring, not delete on copies.
• Every catch logs the error AND enough context (path, operation) to diagnose from the log alone. Errors log as one self-contained [ErrorName]: message string; property names are spelled out (message, not msg).
• Code that posts to GitHub logs its decision trail: the inputs it considered, what it posted (findings, anchors), and a stated reason whenever it skips. A bare count or "skipping" line can't be debugged.
• Internal functions describe what went wrong in their own domain, using the module's own names — never name API surfaces or prescribe caller-level remediation. Input names (max_related_files) belong at the config boundary.
• Reject explicitly instead of normalizing silently when the normalized value would post or write the wrong thing.

Docs

• Docs update in the same change that alters behavior — README, action.yml inputs, and the AGENTS.md structure tree all update in the same PR that changes the feature surface.
• Adding a concept (env var, input, file, feature) means sweeping every doc that lists its peers.
• Write-time format decision: information gets structured format (table for lookups, bullets for parallel items, numbered steps for sequences); narrative goes in the PR description, not committed files. More than 3 sentences of prose → wrong format. Match sibling sections in length.
• Setting references (the README input table, action.yml descriptions, workflow comments) state the knob, its effect, and its limits. Put the valid values and the default on their own labelled lines (Valid values: / Default:), never mid-sentence and never as the implementation's step order.
• Factual claims match the implementation. Mechanism words ("retries", "caches", "falls back") appear only when the code implements that mechanism, and conditional capabilities are stated conditionally.
• Describe what a feature does, not why someone would use it. Use plain words, gloss a jargon term once, and name the concrete referent where the reader is (the input's name, not "the setting above").
• A correction states the current design directly, with no walk-back parenthetical. Before cutting "redundant" reviewed copy, check why it exists; a behavior change replaces only the clause it made stale.
• After editing a section, re-read the whole document, not just the diff. A clarity pass keeps the README's value-proposition copy engaging: cut empty intensifiers and keep vivid lines that are literally true.
• A feature section that describes intrusive behavior states its opt-out inline, not only in the inputs table.
• No internal references in any public artifact — issue/PR numbers, task-board IDs, incident dates, deployment names, and investigation chronology never enter committed files, PR descriptions, or comments.

Review instruction authoring

The bot's system-prompt instructions live in src/review/phases.ts (dimension constants, the per-phase pass-scope line, reporting rules, and the phase groups each phases mode dispatches) and src/review/prompt.ts (identity/scope, proof-of-work, severity rubric, output discipline).

Phase/stage mechanics: a phase is one model call carrying a set of review dimensions. A stage groups the phases that run concurrently; stages run in order, and each later stage sees the earlier stages' findings. combined = 1 stage, 1 phase; parallel = 1 stage, 3 phases; sequential = 3 stages, 3 phases (1 each). The dispatch stack is runStages → runStage → runPhase in src/review/run-stages.ts; cross-phase finding collapse lives in src/review/merge-phase-findings.ts.

When writing or updating a review instruction, follow this formula — each element is here because its absence measurably cost findings in live runs:

• Trigger, not preference. Action + condition + boundary: "when you see X → derive/trace Y → flag if Z. Boundary: keep quiet when W." Preference statements ("prefer exact assertions") get skipped; procedural triggers fire. The boundary is what separates a finding from noise — never ship a trigger without one.
• Name literal scan targets. Spell out the exact tokens the model should pattern-match in a diff (toBeTruthy, .catch(() => {}), ${VAR:-}). A rule whose tokens never appear in the instruction text relies on concept-matching, which misses.
• Wrong/Right micro-examples on high-yield rules. A few-line pair steers the model harder than a paragraph of prose. Budget them — the system prompt loads on every review call.
• Proof-of-work coupling for skippable checklists. A check the model can silently skip needs a required enumeration in the "analysis" field (quote each doc sentence checked; name each changed it() and its derivable exact value). Skipping must be visible in the output, not just discouraged.
• Single-call constraint. Every check must be resolvable by reasoning over the prompt-provided files — no instruction may assume tools, test runs, grep, or repository access beyond the prompt context.
• Drift-guard the load-bearing rules. Each rule that matters gets a targeted fragment assertion in src/review/__tests__/phases.test.ts (test-owned strings, whitespace-normalized) so accidental removal fails the suite.
• Validate live with a planted finding. Before trusting a new check, plant a violation it should catch on a PR (self-review builds from the branch, so the PR's own instructions review it), verify the catch, then revert the plant.

CI conventions

• Pin third-party actions to full commit SHAs with a version comment.
• Top-level permissions: contents: read; jobs escalate individually.
• Secrets scoped to steps, never job-level env.
• persist-credentials: false on checkout unless a push is required.