feat(skill): review a pull or merge request and post it back to the forge by LeTuR · Pull Request #26 · Thurbeen/thurview

LeTuR · 2026-09-12T17:08:02Z

Intent

Add a PR/MR code review skill to thurview that leverages thurview's review capabilities and lets an agent automatically add comments and run a clean review process. The operator's own fleet ran this workflow by hand over two outside contributions to Thurbeen/thurbox (#1107, #1108), and nine rules learned there are requirements to encode: (1) a fork change request has almost no CI, so the review is the gate and the skill must establish what CI actually ran before forming an opinion; (2) a green tick can mean 'never ran' - passed/failed/skipped/cancelled must be distinguished explicitly, because two real bugs shipped when cancelled jobs read as 'no failures'; (3) re-review must read the prior review back point by point, marking each finding addressed, partially addressed or untouched - the single most important rule; (4) diff only what moved, via range-diff against the recorded previous head; (5) resolve a thread only when its point is verified at the current head; (6) security review is per-surface, derived from what the diff touches, and 'no security findings' must be stated explicitly; (7) approve is a state change with consequences and the skill must say so at the point of approving; (8) never merge, never close, never push to a fork; (9) a finding you cannot reproduce is a question, not an assertion. Deliberate decisions made while doing the work, which a reviewer reading only the diff will not know: (a) thurview stays the authoring and evidence surface and the document is still published - by default a human approves the pass before it is posted, because comments land on the operator's account on someone else's work, and the existing publish/wait loop is that gate; an unattended run is opt-in. (b) A NEW skill at skills/forge-review/ rather than a section in skills/thurview/SKILL.md - the brief explicitly forbade bolting it onto that file, and the two have different triggers and terminal states, with thurview explicitly not for reaching verdicts while this one is. (c) A real forge seam in src/forge/ with two adapters (GitHub via gh, GitLab via glab) modelled on ~/code/fleet/scripts/lib/forge.py, which the operator named as the bar, rather than shelling out to gh from skill prose - because reading prior review threads and resolving them needs GraphQL, and rules 2, 5 and 7 are enforceable in code (a check that could not be read is 'unknown' never 'passed'; reply --resolve refuses any --at that is not the current head; submit refuses an approve without --confirm). There is deliberately no merge, close or push method in the seam. (d) scaffold --pr was routed through the seam so it pins a GitLab merge request too. (e) The skill points at ~/.agent-rules/VOICE.md rather than restating it, since the operator may change it, and routes evidence that will not fit a comment into a forge permalink at the pinned head. (f) The GitLab adapter is driven by tests with a fake glab on PATH but has NOT been run against a live instance, and skills/forge-review/references/forges.md says so and lists what GitLab cannot do. (g) The brief also required that SKILL.md frontmatter parses as YAML; main already had test/skill-frontmatter.test.ts from an earlier task, so the new skill's checks were folded into it rather than duplicated. Out of scope by instruction: rewriting the existing review document kind, the explainer, or the browser UI.

What Changed

Add the skills/forge-review/ skill (SKILL.md plus references/forges.md and references/security-surfaces.md) that drives a full PR/MR review through thurview: establishing what CI actually ran (distinguishing passed/failed/skipped/cancelled), re-reading a prior review point by point on re-review, diffing only what moved via range-diff, doing a per-surface security pass, and gating thread-resolution and approval on the current head.
Add a new src/forge/ seam (index.ts, github.ts, gitlab.ts, types.ts, run.ts, submission.ts) with GitHub (gh) and GitLab (glab) adapters for reading change requests, CI status, and review threads, and for submitting reviews, replying to and resolving threads, and approving — with no merge, close, or push capability, and reply --resolve/submit refusing calls against a stale head or an unconfirmed approve.
Wire src/cli.ts with a new forge command (status/prior/submit/reply) and extend scaffold --pr with --forge/--repo flags so it can pin a GitLab merge request as well as a GitHub pull request; update src/store.ts, .no-mistakes.yaml, CONTRIBUTING.md, README.md, and skills/thurview/SKILL.md accordingly.
Add test/forge.test.ts and test/fake-forge.mjs covering both forge adapters, and extend test/skill-frontmatter.test.ts to also validate the new skill's frontmatter.

Risk Assessment

✅ Low: The fix round is a small, well-scoped change (9 lines in src/cli.ts, one new e2e test) that adds --forge/--repo flags to scaffold exactly as prescribed, correctly reuses the existing repoOf/forgeFor override parameters, preserves the resolved forge id in the stored binding for future --update calls, and is verified by a real end-to-end CLI test against a fake glab fixture.

Testing

Ran the new targeted test test/forge.test.ts > thurview scaffold --pr, through the forge seam, which spins up a real git repo with a self-hosted GitLab remote, a fake glab on PATH, and exercises the actual thurview scaffold --pr 7 --forge gitlab CLI process end-to-end, asserting the review binds to MR !7 at the correct head and only the named adapter (glab) is invoked. It passed against the target commit. To confirm this is a genuine regression test and not incidental, I reverted only the src/cli.ts hunk from the fix commit (75bc19e) while keeping the test, reran it, and confirmed it failed for the right reason (unknown flag --forge for scaffold), then restored the fix — worktree is clean. I also captured a real CLI transcript of thurview scaffold --help showing the new --forge/--repo flags with help text identical to the forge command's, matching the fix's exact requirement. All targeted tests (test/forge.test.ts, test/skill-frontmatter.test.ts) pass; no issues found.

Evidence: CLI transcript: scaffold --help and forge --help showing matching --forge/--repo flags

=== thurview scaffold --help (real CLI transcript, target commit 75bc19e) ===
command: thurview scaffold
description: "Create a review pinned to exact base and head commits, or re-pin one"
flags:
  "--pr <value>": "review a pull or merge request (number or URL, needs gh or glab)"
  "--base <value>": "base revision (default: trunk fork point)"
  "--head <value>": "head revision (default: current branch)"
  "--title <value>": initial title
  "--new": create another review even if one matches the binding
  "--update": re-pin an existing review from its binding
  "--review <value>": review to update (id prefix)
  "--forge <value>": "github or gitlab, when the host is not one of the two known ones"
  "--repo <value>": "host/path, when `origin` is not the repository to post to"
examples[4]: thurview scaffold,thurview scaffold --pr 123,thurview scaffold --base main --head HEAD,thurview scaffold --update --review <id>

=== node -e comparison: forge command's --forge/--repo help text ===
command: "thurview forge status|prior|submit --file <path>|reply <threadId> --body \"<text>\""
description: "Read a pull or merge request through its forge, and post the review back to it"
flags:
  "--review <value>": review id prefix; its binding names the change request
  "--change <value>": "change request number or URL, instead of a review binding"
  "--forge <value>": "github or gitlab, when the host is not one of the two known ones"
  "--repo <value>": "host/path, when `origin` is not the repository to post to"
  "--file <value>": "submit: the JSON submission to post"
  "--body <value>": "reply: the answer text"
  "--resolve": "reply: resolve the thread as well as answering it"
  "--at <value>": "reply --resolve: the commit the point was verified at"
  "--confirm": "submit: required to post an approve"
  "--dry-run": "submit: validate and report, post nothing"
  "--max-lines <value>": "submit: warn above this many lines per comment (default: 5)"
  "--mine": "prior: only threads this account wrote"
  "--full": "prior, status: do not truncate or filter"
examples[4]: thurview forge status --change 123,thurview forge prior --change 123 --mine,thurview forge submit --file pass.json --dry-run,"thurview forge reply <threadId> --body \"<answer>\" --resolve --at <sha>"

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 1 issue found → auto-fixed ✅

⚠️ src/cli.ts:637 - thurview scaffold --pr resolves the forge via forgeFor(repo.host, b?.forge) and the repo via repoOf(worktree) with no way to override either (scaffold's SPECS at src/cli.ts:384-397 declare only pr, base, head, title, new, update, review). When auto-detection cannot determine the forge for a host (a self-hosted instance where the CLI isn't authenticated yet), forgeFor's error tells the user to 'pass --forge github|gitlab to name it' - a flag scaffold does not accept, so the hint is a dead end for that command even though the forge command that shares the same helper does support --forge/--repo. Since intent point (d) explicitly calls out routing scaffold --pr through the seam so it pins a GitLab MR too, self-hosted GitLab users hitting an auth gap get a misleading recovery path.

🔧 Fix: Add --forge/--repo flags to scaffold, matching forge command
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

pnpm test && bats scripts/ci/*.bats
npx vitest run test/forge.test.ts -t "self-hosted GitLab merge request with --forge" (passes on target commit)
Reverted the src/cli.ts hunk from commit 75bc19e only, reran the same test, confirmed it fails with unknown flag --forge for scaffold (proves the test is a real regression check), then restored the fix
npx vitest run test/forge.test.ts test/skill-frontmatter.test.ts (22/22 passed)
Manual CLI run: node node_modules/tsx/dist/cli.mjs src/main.ts scaffold --help and ... forge --help, compared flag help text for parity

✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

…orge thurview could pin a pull request, anchor every claim to a line at a pinned commit and answer what the diff does not show, but nothing in it reached the forge. A review that stays in the browser does not tell the contributor anything. `skills/forge-review/` is the bridge, and `thurview forge` is what it drives: - `status` counts passed, failed, cancelled, skipped and running checks separately and compares them against what the target branch's own tip runs. A fork change request runs a fraction of the checks an internal branch does, and a cancelled job shows no failure while asserting nothing; `trustworthy` is the only field that means the tests passed. A baseline that could not be read never reads as a pass. - `prior` reads the previous pass back thread by thread, with what each one was written against, so a re-review can mark every point addressed, partially addressed or untouched. - `submit` posts one pass from one JSON file, refuses an approve without `--confirm`, and warns about comments too long to be read. - `reply --resolve` takes `--at <sha>` and refuses any commit but the current head, so no thread is closed against code nobody looked at. There is no merge, close or push command, deliberately. `src/forge/` is a seam: one interface, a GitHub adapter driving `gh` and a GitLab adapter driving `glab`, with hosts other than github.com and gitlab.com matched against what those CLIs are authenticated for and an unmatched host refused rather than guessed. `scaffold --pr` now goes through it, so it pins a GitLab merge request too. Decisions a reviewer cannot see in the diff: - The thurview document is still published, and by default a human approves the pass before it is posted. Comments land on the operator's account, on someone else's work; the existing publish/wait loop is the gate for that. An unattended run is opt-in and the skill says which happened. - A new skill rather than a section in `skills/thurview/SKILL.md`. Different trigger, different terminal state, and the thurview skill is explicitly not for reaching verdicts while this one is. It also keeps ~250 lines of forge rules out of every thurview run. - The skill points at `~/.agent-rules/VOICE.md` rather than restating it, and routes evidence that will not fit a comment into a permalink at the pinned head instead of a wall of prose on someone's diff. Verified: `test/forge.test.ts` drives both adapters with a fake `gh` and `glab` on PATH, asserting the calls made and the answers parsed. The GitHub adapter was also run against live pull requests. The GitLab adapter has not been run against a live instance; `references/forges.md` says so and lists what GitLab cannot do - no atomic review, no changes-requested state, no multi-line comment anchor. Claude-Session: https://claude.ai/code/session_015KDtHyH9U2xWn5HzBjm3Tc

…ching forge command

…er scaffold --pr gained glab

## Intent thurview could explain code and help a human review it, but it had no way to review a change and fix it on its own. The operator asked for one simple review-and-autofix skill built on thurview's code graph, to replace `forge-review` (#26), and for the graph to show what a diff alone can't. ## What Changed - `skills/review-fix/` replaces `skills/forge-review/`. It pins a branch, a range or a PR/MR, asks the graph who depends on each changed symbol, and records findings with file:line, severity, a one-line why and graph evidence. It fixes what it is sure of, keeps only fixes that pass the repo's own tests and lint, commits them once and reports the rest. `--post` sends the unfixed findings through `thurview forge submit`. It never pushes or posts unless asked. 165 lines and one reference, down from 276 and two. - `thurview graph interfaces|impact|callers|tests-for|architecture` take `--base`/`--head`, so they run on two commits with no published review. They pin the same way `scaffold` does (head defaults to HEAD, base to the trunk fork point); `--review` together with either is refused. - `impact.reach` rows gain `at` (the call-site line) and `tested` (whether any test reaches that caller). Every reach row is already code the diff left alone, so a row reads as a finding: ```text reach[1]{symbol,file,line,depth,via,at,tested}: checkout,src/cart.js,3,1,"src/price.js:discount",5,false ``` - README, CONTRIBUTING, `.no-mistakes.yaml`, the `setup skill` hint and the one sentence in `skills/thurview/SKILL.md` now point at `review-fix`. ## Risk Assessment - `impact.reach` has two new columns. Anything reading that table by position sees them; nothing in this repo does. - `thurview forge status|prior|submit|reply` stay. Removing them would break the CLI beyond this change; the skill uses only `status` and `submit`. - `security-surfaces.md` is dropped on purpose. The skill keeps a short definition of a security finding instead. - The skill needs the new flags, so an install from `main` needs a released CLI. It tells the agent to run `thurview update` on `unknown flag`. ## Testing Each of these failed before its change and passes after: - `e2e`: answers on two commits without a review. Failed on `unknown flag --base`, then on the `data.yaml` hint. - `graph.test`: gives each caller the change left alone its call site and whether a test reaches it. Failed on missing `at`/`tested`. - `skill-frontmatter`: ships one review skill. Failed on `skills/review-fix/SKILL.md` missing. The skill itself ran on a seeded bug. In a throwaway repo, a branch changed `discount(total, percent)` to take a rate in [0, 1] and updated its own test, but not `checkout` in `src/cart.js`, which is outside the diff and untested. A fresh agent ran the skill with an empty request, without being told where the bug was: ```mermaid flowchart LR A["graph impact --head HEAD"] -->|"reach: checkout src/cart.js:5, tested false"| B["finding, high: checkout passes 15, discount now takes 0.15"] C["graph interfaces"] -->|"discount (total, percent) to (total, rate)"| B B --> D["new test/cart.test.js fails with RangeError"] D --> E["fix: coupon.percent / 100"] E --> F["npm test 3/3, one fix commit"] ``` `npm test` went 1/1 before, 2/3 with the new test, 3/3 after the fix commit. The agent's notes on the skill (watch the new test fail first, the repo's commit convention wins, a place for problems already present at base) are folded into the second commit. Not verified: `--post` against a live forge, since the demo repo has no remote (`forge submit` itself is covered by `test/forge.test.ts`), and the new `reach` fields on languages other than JavaScript and TypeScript. <details> <summary>Evidence: Manual seeded-bug reproduction transcript</summary> ```text Manual end-to-end reproduction of the seeded-bug scenario from the user intent ("discount(total, percent) -> discount(total, rate)" caller left untested), run against a throwaway TypeScript repo (/tmp/rate-demo-ts, discarded after this run) using the `thurview graph` CLI at commit d5fa83c (feat/review-fix-skill), via `npx tsx src/main.ts`. === graph impact --head HEAD === base: d7290d9c head: 9d64d2be changed[1]: src/price.ts:discount (function, modified) reach[1]{symbol,file,line,depth,via,at,tested}: checkout,src/cart.ts,2,1,"src/price.ts:discount",3,false tests[1]: test/price.test.ts covers src/price.ts:discount untested: [] === graph interfaces --head HEAD === verdict: 1 changed. interfaces[1]: src/price.ts:discount changed was: export function discount(total: number, percent: number): number now: export function discount(total: number, rate: number): number === graph callers discount --head HEAD === callers[2]{symbol,file,line,at}: checkout,src/cart.ts,2,3 <module>,test/price.test.ts,1,2 Result: the graph correctly flags `checkout` (src/cart.ts) as a caller of the changed `discount` signature that lies outside the diff and is not reached by any test (tested: false), while the discount's own test in test/price.test.ts is marked tested: true elsewhere (see test/graph.test.ts "gives each caller the change left alone its call site and whether a test reaches it"). This is exactly the class of bug (untested caller broken by a signature change) the operator's brief describes as the target use case, and demonstrates impact.reach's new `at`/`tested` fields ("useful insights") working end-to-end, not just in unit tests. === --review together with --base/--head is refused === $ thurview graph impact --review someid --base main error: "pass --review or --base/--head, not both" code: VALIDATION_ERROR exit=2 ``` </details> ## Pipeline Updates from [git push no-mistakes](https://github.com/kunchenguid/no-mistakes)  <details> <summary>✅ **intent** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Rebase** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Review** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Test** - passed</summary> ✅ No issues found. - `pnpm test && bats scripts/ci/*.bats` - `npx vitest run test/skill-frontmatter.test.ts test/graph.test.ts (16/16 passed, including the new tests)` - `npx vitest run test/e2e.test.ts full file (34/34 passed)` - `Manual repro with throwaway TS repo and thurview graph impact/interfaces/callers via npx tsx src/main.ts` - `git diff d617e3c..d5fa83c -- skills/thurview/SKILL.md` - `grep -rn forge-review across md/yaml/ts files` </details> <details> <summary>✅ **Document** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Lint** - passed</summary> ✅ No issues found. </details> <details> <summary>✅ **Push** - passed</summary> ✅ No issues found. </details> https://claude.ai/code/session_01XEkc6BULdsADTNu1mABPVe — LeTuR's agent

LeTuR added 3 commits September 12, 2026 18:49

chore: no-mistakes review - Add --forge/--repo flags to scaffold, mat…

75bc19e

…ching forge command

chore: no-mistakes document - Fix stale gh-only prerequisite docs aft…

bf99232

…er scaffold --pr gained glab

LeTuR changed the title ~~feat(skill): add forge-review skill for PR/MR code review~~ feat(skill): review a pull or merge request and post it back to the forge Sep 12, 2026

LeTuR merged commit d617e3c into main Sep 12, 2026
8 checks passed

LeTuR deleted the feat/pr-review-skill branch September 12, 2026 20:53

LeTuR mentioned this pull request Sep 13, 2026

feat(skill): replace forge-review with review-fix skill #27

Merged

Navigation Menu

Uh oh!

feat(skill): review a pull or merge request and post it back to the forge - #26

feat(skill): review a pull or merge request and post it back to the forge#26
LeTuR merged 3 commits into
mainfrom
feat/pr-review-skill

LeTuR commented Sep 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

LeTuR commented Sep 12, 2026

Intent

What Changed

Risk Assessment

Testing

Pipeline

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant