docs: add pathfinder rule, LOC review metric, and test-duplication clause to AGENTS.md (#118330)

This commit is contained in:
Peter Steinberger
2026-08-02 18:11:29 -07:00
committed by GitHub
parent d6f66b69ff
commit 0839f12ccd

View File

@@ -31,6 +31,7 @@ Skills own workflows; root owns hard policy and routing. Product direction and m
- Repair invalid, missing, or leaked state at its producer or lifecycle owner. Record authoritative facts where they occur; do not compensate downstream for upstream ownership failures.
- Prefer one canonical flow and coherent owner-boundary refactors. Remove connected duplicate policy, obsolete abstractions, wrappers, fallback stacks, dead branches, and unnecessary compatibility in the same change when they share the invariant.
- A larger coherent refactor beats a narrow workaround. Existing product, security, ownership, public-contract, protocol, migration, and SQLite-schema approval gates still apply; broad reading never needs extra approval.
- Pathfinder rule: leave touched code better than found. Never silently walk past an unrelated issue discovered mid-task — fix it in the same PR when small and bounded, otherwise record it as a named follow-up (issue, PR note, or spawned task). A slightly less-pure PR that moves the code toward clean beats a minimal diff that ignores known mess; keep opportunistic fixes coherent and call them out in the PR body.
- Never hardcode the reported provider, channel, command, customer example, identifier, or error text in production unless it is an explicit contract.
- Do not mask root causes with consumer-only guards, forced test environments, retries, larger timeouts, weaker assertions, broader mocks, speculative fallbacks, or parallel execution paths.
- Production LOC is a first-class constraint; count tests separately. Prefer net-neutral or net-negative production changes. Positive production LOC requires a concrete capability, ownership boundary, security invariant, or public/dependency contract that cannot be expressed more simply.
@@ -60,6 +61,7 @@ Skills own workflows; root owns hard policy and routing. Product direction and m
- Optional integrations, providers, channels, skill bundles, MCP surfaces, and service workflows route to plugins, ClawHub, or owner repos when current seams suffice. Keep core items for missing core/plugin APIs, bundled regressions, security/core hardening, or maintainer product decisions.
- Plugin APIs, provider routing, auth/session state, persisted preferences, config loading, config/default additions, migrations, setup, startup checks, and fallback behavior are compatibility/upgrade-sensitive. Treat config breaks, new config/default surfaces, removed fallbacks, fail-closed changes, stricter validation, or new operator action as merge risk even with green CI when they can affect existing users, upgrades, provider/plugin behavior, or maintainer operations.
- For PRs that add, remove, or change config/default surfaces with possible compatibility, upgrade, provider/plugin, operator, setup, startup, or fallback impact, ClawSweeper review should emit a `reviewMetrics` entry when practical. The metric should name the count and direction of the changes, such as added, changed, or removed config/default surfaces, and explain why the metric matters before merge. When the metric indicates concrete merge risk, also surface the concern in `risks`, use `mergeRiskLabels` when the risk matches the label rubric, make `bestSolution` name the desired pre-merge state, and ensure `labelJustifications` explain the specific reason rather than restating the label.
- PR reviews also emit a production-vs-test LOC delta `reviewMetrics` entry when practical, counting production and test lines separately. Positive production growth with no named Repair Doctrine justification (capability, ownership boundary, security invariant, or public/dependency contract) surfaces in `risks` with the missing justification named; justified feature growth and test lines are not findings by themselves.
- Review whole decision surfaces, not only the touched runtime, provider, channel, harness, plugin seam, or context path. Check sibling Codex/Pi-style runtimes, provider/model routing, channel delivery, gateway/protocol, plugin SDK, and context-management paths when relevant.
- Every PR review must explicitly ask whether the PR is the best fix, not merely a plausible fix. Verdicts need a best-fix judgment backed by enough code reading to compare owner boundaries, callers, siblings, tests, docs, current `main`, shipped behavior when relevant, and dependency/Codex contracts when involved.
- Before a PR verdict, build a small evidence map: changed surface, entry point, owner boundary, at least one caller and callee, sibling surfaces that share the invariant, existing tests, and current `main` behavior. If any cell is missing, say the gap instead of concluding.
@@ -278,6 +280,7 @@ Mechanics only; policy lives above.
- Inline simple one-use objects/spreads when clearer. Extract only when it removes duplication or hard logic.
- Tests prove behavior/regressions, not every internal branch.
- Tests are welcome, but review them before landing for duplication and value. Delete useless tests, such as assertions for behavior or paths just removed.
- New tests must not re-prove behavior existing tests already cover. Prefer extending an existing table-driven case or shared fixture over a near-duplicate test; consolidate duplicated setup in the same change.
- Tests protect canonical behavior and migration boundaries, not obsolete internals. Delete tests for removed fallback paths instead of updating them.
- Prefer existing narrow helpers over repeated casts/guards. Add local helpers when 2+ nearby call sites share real boundary logic.
- Prefer ctor parameter properties for injected deps/config. Do not ban them for erasable-syntax purity.