chore(agents): port latest pr-review engine mechanism into .agents/#807
Open
0xbulma wants to merge 6 commits into
Open
chore(agents): port latest pr-review engine mechanism into .agents/#8070xbulma wants to merge 6 commits into
0xbulma wants to merge 6 commits into
Conversation
Restructure the in-repo review system to mirror upstream facets' pr-review-engine/ layout (hybrid mirror, full port), keeping the 9 Morpho-specialised personas as the agent set. - new engine spec .agents/pr-review-engine/SKILL.md supersedes the English .agents/lib/pr-review-base.md; adds the WHAT/FIX finding schema, +/-15 line snapping, INTENT_CONTEXT injection, merge-base recompute + base-merge warning, doc-only fast path, EXCLUDE_AGENTS - deterministic TS scripts (build-changed-lines, validate-findings, findings-ledger + idempotency cache, review-scope) under scripts/, gated by a new agents-engine Vitest project (124 tests) + strict tsconfig - 9 personas moved to agents/ + new skill-authoring agent (HAS_PLUGIN_SKILLS); references subset ported and adapted - callers repointed: pr-review-local gains Step 2c cache + Step 6b ledger + commit-message intent + SSH->HTTPS fetch fallback; pr-review-gh gains PR-title/body intent + PR-keyed ledger + audit trail; pr-review-ci keeps the repo-only verdict mode; pr-fix wires Fix-rubric discovery - AGENTS.md section 10 inventory + persona backlinks rewritten - docs/tibs/TIB-2026-06-19 records the decision Repo-metadata/tooling only - no published-package source touched, no changeset. Gates: biome clean on changed paths (biome.ci.json too), agents-engine 124 tests pass, tsc --noEmit clean, jsdoc:coverage 41/0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0xbulma
changed the title
list-fix-rubric-agents.sh: drop the CLAUDE_PLUGIN_ROOT branch. It pointed
at the upstream plugin-cache layout (${CLAUDE_PLUGIN_ROOT}/skills/
pr-review-engine/agents), which does not exist in this in-repo port —
with the var set in a plugin host the script resolved to a missing dir
and exited 1, silently breaking /pr-fix's fix-rubric discovery. The
engine's agents/ is always a fixed sibling of scripts/, so the
SCRIPT_DIR-relative fallback is the single correct resolution. Also drop
the stale bats-invariant reference from the usage comment (no bats suite
in this repo).
Verified: with CLAUDE_PLUGIN_ROOT set, the script now lists the 3
fix-rubric agents and exits 0 (was exit 1).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Closes the parity gap between the repo's own /pr-review-local and the upstream plugin command. Ports the autonomous review->fix->re-review loop (--goal, --max-iters) and the --fast cut, adapted to this repo: - --fast excludes the `documentation` agent (repo's name; upstream's is `docs`) via the engine's <EXCLUDE_AGENTS>. - --goal: pre-flight gates (dirty tree / detached HEAD / red base gate), the loop (review -> partition -> stuck/success checks -> batched fix -> re-gate -> commit), leaving-the-branch-clean rollback, and a single post-convergence ledger stamp. GOAL_CLEAN/ABORTED/STUCK/MAXED sentinels. - Re-gate is pinned to repo-scoped, non-watch commands (biome check on changed files, tsc on the touched tsconfig, `vitest run --project <affected>`) — NOT bare `pnpm lint` (pre-existing-red on frozen deprecated pkgs) or `pnpm test` (watch mode / fork suite needs RPC). - No runtime stage / --no-runtime: headless SDK, HAS_ROUTE_UI always false, so no GOAL_RUNTIME_RED path. TIB Future Considerations updated: only pr-review-gh's local->GitHub ledger reuse remains deferred. Repo-metadata/tooling only — no changeset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…w-local
The --goal spec referenced <HAS_ROUTE_UI> in the engine-input angle-bracket
convention, but the ported engine defines only HAS_CI_RELEASE,
HAS_PLUGIN_SKILLS, and HAS_PROTOCOL_SURFACE — HAS_ROUTE_UI is an
upstream-only flag. Reword to plain prose ("headless SDK with no
route-reachable UI") so it no longer reads as a defined-but-missing flag.
Resolves the lone (low) finding from /pr-review-local --goal.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
Rubilmax
previously approved these changes
Jun 19, 2026
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Foulks-Plb
reviewed
Jun 19, 2026
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f27fe459ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Aikido SAST (PR #807) flagged the file reads in the ported review scripts as potential path-traversal sinks. The diff-derived read in validate-findings.ts (the markdown fenced-block check) reads `join(repoRoot, norm)` where `norm` comes from the agent-emitted finding.file. It is already scope-filtered against the git-produced changed-files set (which never yields a `..` path), but add an explicit repoRoot-containment guard as defense-in-depth: resolve the target and skip the read if it escapes repoRoot (the finding is then kept, not dropped as a doc-example). The other flagged reads are operator/caller-controlled (the ledger path and CLI args in findings-ledger.ts) — not attacker input — so they get no code change; documented as such on the PR threads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Scripts:
- findings-ledger.ts, review-scope.ts: fix the isMain guard — use
`process.argv[1] === fileURLToPath(import.meta.url)` (matching the
other two scripts) instead of hand-building `file://${process.argv[1]}`,
which skips percent-encoding and so never matches import.meta.url on a
checkout path containing spaces, silently no-op'ing main(). (devin, Foulques)
- findings-ledger.ts: a missing/unreadable --findings file now exits 2
instead of being parsed as `[]` — the empty array would make --write
mark every open ledger entry `resolved` and silently corrupt the ledger. (Codex)
- validate-findings.ts: write the normalized (repo-relative) path back
into kept findings — PR/CI callers pass `file` into the GitHub review
`comments[].path`, which must be repo-relative. (Codex)
- review-scope.ts: docstring pointed at a non-existent `pnpm verify`
script; point at the `pnpm test` / agents-engine gate. (Foulques)
Caller specs:
- pr-review-local.md: don't stamp the idempotency-cache identity on a run
with agent failures (would replay REVIEW_INCOMPLETE as REVIEW_CLEAN);
gate --goal success on FAILED_AGENTS==0 (new GOAL_INCOMPLETE stop). (Codex)
- pr-review-gh.md: apply the PR-keyed findings ledger inside each --watch
cycle so wontfix/seen findings aren't reposted on every new commit. (Codex)
Tooling:
- bump engines.node to >=22.18 + accurate agents-engine Vitest comment:
the integration tests spawn `node <script>.ts`, so they need native
type-stripping; 22.13-22.17 was advertised supported but ran red. (Foulques)
Repo-metadata/tooling only — no published-package source, no changeset.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 19, 2026
validate-findings: write the normalized repo-relative path back into kept findings
0xbulma/facets#44
Closed
Rubilmax
approved these changes
Jun 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this is strictly better
This is a Pareto upgrade of the in-repo review system: every capability the current
.agents/stack has is kept, a set of deterministic and stateful capabilities is added on top, and no
published-package source is touched — zero blast radius on the shipped SDK (repo-metadata /
dev-tooling only, no changeset).
Strictly retained — no regression:
morpho-protocol, the §2 forbidden-patterns calibration,the §10 CI/release lens, …) — the specialisation the repo's review quality depends on and that the
generic upstream agents do not encode.
AGENTS.mdcontract structure and the> Applied by personas:backlinks.pr-review-civerdict mode — which upstream is local-first and doesn't even have.Strictly added — new functionality:
re-derived every run to four bundled, unit-tested TypeScript scripts (
build-changed-lines,validate-findings,findings-ledger,review-scope) gated by a newagents-engineVitestproject (124 tests) +
tsc --noEmit+ Biome. A regression now fails a gate instead of silentlydegrading review quality.
snapped_line),INTENT_CONTEXTinjection (commit message / PR title + body), and a doc-only fast path — findings land on the right
line with the author's intent in scope.
ballooning the reviewed diff (the upstream case that jumped from 67 → 462 files). The old engine had
no such guard.
(net-new / recurring / resolved / suppressed) + idempotency cache make re-reviews on an evolving PR
cheap and quiet. Both live outside the repo (
~/.claude/facets/reviews/), so they add nostatefulness to the reviewed tree and never trip a clean-tree guard.
skill-authoringpersona on a newHAS_PLUGIN_SKILLSflagguards the engine roster ↔ §10 table ↔ persona-backlink invariants.
pr-review-engine/layout turns improvementslanding upstream into a cheap port rather than a divergent re-derivation.
In short: a strict superset of the prior mechanism, bought entirely with repo-metadata /
dev-tooling changes.
What
Brings the repo's in-repo agentic review system up to the latest facets mechanism. Hybrid mirror + full port: restructures
.agents/to mirror upstream'spr-review-engine/layout (SKILL.md+agents/+references/+scripts/) while keeping the 9 Morpho-specialised personas as the agent set.Decision doc:
docs/tibs/TIB-2026-06-19-adopt-latest-facets-review-mechanism.md.Changes
.agents/pr-review-engine/SKILL.mdsupersedes the English.agents/lib/pr-review-base.md. Adds the WHAT/FIX finding schema, ±15-line snapping (snapped_line),INTENT_CONTEXTinjection, merge-base recompute + base-merge warning, doc-only fast path, andEXCLUDE_AGENTS.build-changed-lines,validate-findings,findings-ledger(stateful ledger + idempotency cache),review-scope, ported from upstream and gated by a newagents-engineVitest project (124 tests) + a stricttsconfig.agents/(+ relinked); new repo-adaptedskill-authoringagent on a newHAS_PLUGIN_SKILLSflag; Fix-rubric sections + reference subset ported.pr-review-localgains the idempotency cache + findings ledger + commit-message intent + SSH→HTTPS fetch fallback;pr-review-ghgains PR-title/body intent + PR-keyed ledger + audit trail;pr-review-cikeeps the repo-only verdict mode (upstream has no CI variant);pr-fixwires Fix-rubric discovery.AGENTS.md§10 inventory + persona backlinks rewritten.Scope / SemVer
Repo-metadata and dev-tooling only — no published-package source touched, no changeset (§7). The live engine invokes the scripts as
node …/foo.ts, requiring Node ≥ 22.18 (native TS stripping); documented inSKILL.mdrather than bumping the monorepoengines.node.Verification
biome checkclean on all changed paths (and underbiome.ci.json).agents-engineVitest project: 124/124 pass;tsc --noEmitclean;jsdoc:coverage:check41/0.build-changed-linesmaps the changed files; ledger round-trip givesnet_new → cache_hit → recurring.pnpm lint(dev-config) errors are pre-existing in frozen deprecated packages — none in this changeset.🤖 Generated with Claude Code