Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
schema: spec-driven
created: 2026-08-07
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
## Why

`gx pr-review` (GitGuardex code-assist) only ever described problems. It had no
write-back path, so every fix was a human action, and the posted review had five
defects that made it fragile and hard to read:

1. Findings were never validated against the diff. GitHub 422s the whole review
when one comment anchors outside it, and in `--gate-review` mode that throw
becomes a merge block — a bad line number from the provider could block a
healthy PR.
2. Every run POSTed a fresh review, so repeated `gx ship` attempts stacked
duplicate inline comments.
3. Only `line` was sent, so a fix spanning multiple lines could not be expressed
as a committable suggestion.
4. The summary body was one bare sentence; the merge-gate verdict was invisible,
so a reader could not tell advisory findings from blocking ones.
5. The whole diff went into the prompt uncapped, and a truncated review read as
a clean one.

`suggestion` was also documented to the provider as optional, so it was routinely
omitted and findings arrived as prose with no committable fix.

## What Changes

- **Diff anchoring** (`src/review-diff.js`, new): parse the unified diff into the
RIGHT-side lines GitHub accepts, split findings into anchored/unanchored, post
only anchored ones inline and report the rest in the summary. A rejected POST
retries once as summary-only so a review never hard-fails on anchoring.
- **Dedupe**: every posted finding carries a fingerprint marker; a re-run reads
the PR's existing comments and skips what it already said.
- **Range findings**: `start_line`/`start_side` are forwarded when a finding
declares a range strictly before `line`.
- **Presentation**: severity alert callouts (`> [!CAUTION|WARNING|IMPORTANT|NOTE]`)
with an emoji + category badge, a lede with the tail folded into `<details>`,
and a summary report card with severity mix, gate verdict, findings table, and
a provider/commit footer.
- **Prompt contract**: `suggestion` is required for bounded edits, `category` and
`start_line` are requested, and the provider is told to drop findings it cannot
anchor.
- **Diff cap**: capped at 220k chars with truncation surfaced in both the summary
and the CLI output.
- **Auto-fix** (`src/review-fix.js`, new): `gx pr-review --fix` runs the provider
in edit mode and commits what changed. `gx branch finish --gate-autofix`
(`--gate-autofix-rounds N`, 1-5) lets the merge gate repair its own blocking
findings: fix in the worktree, push, re-review with a fresh provider run, then
re-evaluate.
- **Parse-order fix**: `extractJsonPayload` now tries the raw text before the
fence extractor, so valid JSON containing a fenced `suggestion` no longer
parses as the suggestion body.

## Impact

- Affected surfaces: `gx pr-review`, `gx branch finish --gate-review`, `gx ship`.
- New CLI flags: `--fix`/`--no-fix` on `pr-review`; `--gate-autofix`,
`--no-gate-autofix`, `--gate-autofix-rounds` on `branch finish`. All default
off, so existing invocations behave as before apart from the improved review
presentation and the anchoring/dedupe safety.
- Write risk is bounded: auto-fix refuses protected branches, refuses uncommitted
tracked edits, stages only paths the fix touched, is pinned to the gated branch
by name, and never pushes or merges on its own outside the gate loop.
- The gate is still fail-closed. Auto-fix only ever converts a block into a merge
after an independent re-review; an unfixable finding falls through to the block.
- Extra provider invocations when auto-fix is enabled: one fix run plus one
re-review per round.
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
## ADDED Requirements

### Requirement: Findings SHALL be validated against the reviewed diff before posting

The system SHALL partition findings into diff-anchored and unanchored sets, post
only anchored findings inline, and report unanchored findings in the review
summary instead of discarding them or failing the post. GitHub rejects an entire
review with 422 when any inline comment anchors outside the diff.

#### Scenario: A finding points at a line outside the diff

- **WHEN** the review provider returns a finding whose line is not present in the PR diff
- **THEN** no inline comment is created for that finding
- **AND** the review summary lists it under a "could not be anchored" section
- **AND** the review post succeeds

#### Scenario: GitHub rejects the inline anchors anyway

- **WHEN** the review POST fails while carrying inline comments
- **THEN** the system SHALL retry once as a summary-only review carrying the findings in the body
- **AND** the summary SHALL record that inline anchoring was rejected

### Requirement: Repeated reviews SHALL NOT stack duplicate inline comments

Each posted finding SHALL carry a stable fingerprint marker. Before posting, the
system SHALL read the PR's existing review comments and skip any finding whose
fingerprint is already present.

#### Scenario: The same review runs twice on an unchanged PR

- **WHEN** a review re-runs and every finding was already reported
- **THEN** zero new inline comments are created
- **AND** the summary reports how many findings were already reported

#### Scenario: Reading existing comments fails

- **WHEN** the GitHub call listing existing comments fails
- **THEN** the review SHALL proceed and post all findings rather than blocking

### Requirement: A finding SHALL be able to address a multi-line range

Findings SHALL accept a `start_line` that is strictly before `line`, forwarded to
GitHub as `start_line` + `start_side` so a suggestion can replace a range.

#### Scenario: start_line is not strictly before line

- **WHEN** a finding declares `start_line` equal to or after `line`
- **THEN** the finding SHALL collapse to a single-line comment, because GitHub rejects such a range

### Requirement: The review summary SHALL state the merge-gate verdict

The summary body SHALL report the finding count, the severity mix, and whether
the findings block the merge gate, so a reader can distinguish advisory findings
from blocking ones.

#### Scenario: Only advisory findings are present

- **WHEN** the review finds medium and low findings and the gate blocks on high/critical
- **THEN** the summary SHALL state that the merge gate passes

#### Scenario: A blocking finding is present

- **WHEN** the review finds a high or critical finding
- **THEN** the summary SHALL state that the merge gate is blocked and how many findings block it

### Requirement: Inline comments SHALL lead with the defect

An inline comment SHALL render a severity alert callout with the severity and
category, followed by a lede. Message text beyond the lede limit SHALL be folded
into a collapsed details block.

#### Scenario: A long finding message is posted

- **WHEN** a finding message exceeds the lede limit
- **THEN** the inline comment SHALL show the lede first
- **AND** the remainder SHALL be inside a collapsed details block

### Requirement: A truncated review SHALL declare itself partial

The diff sent to the provider SHALL be capped. When the cap applies, the review
output SHALL state that the review is partial.

#### Scenario: The PR diff exceeds the cap

- **WHEN** the diff is longer than the cap
- **THEN** the summary and the CLI output SHALL both note that the diff was truncated

### Requirement: The review provider SHALL be asked for applicable suggestions

The review prompt SHALL require a `suggestion` for any finding whose fix is a
bounded edit to the commented line range, so GitHub renders a committable
suggestion rather than prose only.

#### Scenario: The provider wraps a suggestion in a code fence

- **WHEN** a returned `suggestion` is wrapped in a code fence
- **THEN** the fence SHALL be stripped before the suggestion is embedded in the comment

#### Scenario: The provider returns JSON containing a fenced suggestion

- **WHEN** provider output is valid JSON whose `suggestion` value contains a code fence
- **THEN** the payload SHALL parse as findings, not as the fence contents

### Requirement: `gx pr-review --fix` SHALL apply findings as a commit

With `--fix`, the system SHALL invoke the provider in edit mode and commit the
resulting edits as a single commit on the current branch.

#### Scenario: The branch is protected

- **WHEN** `--fix` runs on a protected branch
- **THEN** the provider SHALL NOT run
- **AND** the command SHALL report that it refused

#### Scenario: The working tree has uncommitted tracked edits

- **WHEN** `--fix` runs with uncommitted tracked changes present
- **THEN** the provider SHALL NOT run, so unrelated work is never bundled into the fix commit

#### Scenario: Untracked files already exist in the tree

- **WHEN** `--fix` runs with pre-existing untracked files present
- **THEN** the fix SHALL proceed
- **AND** only paths the fix touched SHALL be staged

#### Scenario: The provider makes no edits

- **WHEN** the provider runs and changes nothing
- **THEN** the result SHALL report a no-op and no commit SHALL be created

### Requirement: `--gate-autofix` SHALL let the merge gate repair blocking findings

With `--gate-autofix`, the merge gate SHALL run up to a bounded number of repair
rounds on blocking findings: fix in the branch's worktree, push, then re-review
with a fresh provider run before re-evaluating.

#### Scenario: Auto-fix is not requested

- **WHEN** the gate finds blocking findings without `--gate-autofix`
- **THEN** no fix SHALL run
- **AND** the gate SHALL block and mention `--gate-autofix` as an option

#### Scenario: Auto-fix clears the findings

- **WHEN** a repair round changes files and the re-review returns no blocking findings
- **THEN** the gate SHALL proceed to the merge
- **AND** the re-review SHALL be a separate provider invocation from the fix

#### Scenario: Auto-fix exhausts its round budget

- **WHEN** every budgeted round runs and blocking findings remain
- **THEN** the gate SHALL block and report that auto-fix did not clear them

#### Scenario: A repair round changes nothing

- **WHEN** a repair round makes no edits
- **THEN** the loop SHALL stop immediately rather than retrying
- **AND** the gate SHALL block

#### Scenario: The resolved worktree holds a different branch

- **WHEN** the checkout used for the fix is not on the gated branch
- **THEN** the fix SHALL be skipped rather than edit the wrong branch
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
## Definition of Done

This change is complete only when **all** of the following are true:

- Every checkbox below is checked.
- The agent branch reaches `MERGED` state on `origin` and the PR URL + state are recorded in the completion handoff.
- If any step blocks (test failure, conflict, ambiguous result), append a `BLOCKED:` line under section 4 explaining the blocker and **STOP**. Do not tick remaining cleanup boxes; do not silently skip the cleanup pipeline.

## Handoff

- Handoff: change=`agent-claude-code-assist-autofix-and-review-presentat-2026-08-07-11-56`; branch=`agent/claude/code-assist-autofix-and-review-presentat-2026-08-07-11-56`; scope=`code-assist diff anchoring, dedupe, presentation, and auto-fix`; action=`continue this sandbox or finish cleanup after a usage-limit/manual takeover`.
- Copy prompt: Continue `agent-claude-code-assist-autofix-and-review-presentat-2026-08-07-11-56` on branch `agent/claude/code-assist-autofix-and-review-presentat-2026-08-07-11-56`. Work inside the existing sandbox, review `openspec/changes/agent-claude-code-assist-autofix-and-review-presentat-2026-08-07-11-56/tasks.md`, continue from the current state instead of creating a new sandbox, and when the work is done run `gx branch finish --branch agent/claude/code-assist-autofix-and-review-presentat-2026-08-07-11-56 --base main --via-pr --wait-for-merge --cleanup`.

## 1. Specification

- [x] 1.1 Finalize proposal scope and acceptance criteria for `agent-claude-code-assist-autofix-and-review-presentat-2026-08-07-11-56`.
- [x] 1.2 Define normative requirements in `specs/code-assist-autofix-and-review-presentation/spec.md`.

## 2. Implementation

- [x] 2.1 Implement scoped behavior changes.
- [x] 2.2 Add/update focused regression coverage.

## 3. Verification

- [x] 3.1 Run targeted project verification commands.
- [x] 3.2 Run `openspec validate agent-claude-code-assist-autofix-and-review-presentat-2026-08-07-11-56 --type change --strict`.
- [x] 3.3 Run `openspec validate --specs`.

## 4. Cleanup (mandatory; run before claiming completion)

- [ ] 4.1 Run the cleanup pipeline: `gx branch finish --branch agent/claude/code-assist-autofix-and-review-presentat-2026-08-07-11-56 --base main --via-pr --wait-for-merge --cleanup`. This handles commit -> push -> PR create -> merge wait -> worktree prune in one invocation.
- [ ] 4.2 Record the PR URL and final merge state (`MERGED`) in the completion handoff.
- [ ] 4.3 Confirm the sandbox worktree is gone (`git worktree list` no longer shows the agent path; `git branch -a` shows no surviving local/remote refs for the branch).
34 changes: 34 additions & 0 deletions src/cli/args.js
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,9 @@ function parsePrReviewArgs(rawArgs) {
post: false,
artifact: '',
timeoutMs: 10 * 60 * 1000,
// Apply the findings as commits on the current agent branch instead of only
// reporting them. Off by default: this writes to the working tree.
fix: false,
};

for (let index = 0; index < parsed.args.length; index += 1) {
Expand All @@ -320,6 +323,14 @@ function parsePrReviewArgs(rawArgs) {
options.post = false;
continue;
}
if (arg === '--fix') {
options.fix = true;
continue;
}
if (arg === '--no-fix') {
options.fix = false;
continue;
}
if (arg === '--artifact' || arg === '--output') {
options.artifact = requireValue(parsed.args, index, arg);
index += 1;
Expand Down Expand Up @@ -1171,6 +1182,10 @@ function parseFinishArgs(rawArgs, defaults = {}) {
gateReview: defaults.gateReview ?? autoShip,
reviewProvider: defaults.reviewProvider || 'codex',
allowNoChecks: false,
// Let the gate repair its own blocking findings instead of only reporting
// them. Off by default: this writes commits to the branch.
gateAutofix: false,
gateAutofixRounds: 1,
// After a bulk `--all` finish, sweep merged-but-stranded worktree dirs whose
// branch was merged out-of-band and never reaped (the post-merge "retained"
// gap). Only fires for `--all`; opt out with --no-sweep-orphans.
Expand Down Expand Up @@ -1311,6 +1326,25 @@ function parseFinishArgs(rawArgs, defaults = {}) {
options.allowNoChecks = true;
continue;
}
if (arg === '--gate-autofix') {
options.gateAutofix = true;
continue;
}
if (arg === '--no-gate-autofix') {
options.gateAutofix = false;
continue;
}
if (arg === '--gate-autofix-rounds') {
const raw = rawArgs[index + 1];
const rounds = Number.parseInt(String(raw ?? ''), 10);
if (!Number.isInteger(rounds) || rounds < 1 || rounds > 5) {
throw new Error('--gate-autofix-rounds requires an integer between 1 and 5');
}
options.gateAutofixRounds = rounds;
options.gateAutofix = true;
index += 1;
continue;
}
if (arg === '--review-provider') {
const next = rawArgs[index + 1];
if (!next || !['codex', 'claude'].includes(next)) {
Expand Down
32 changes: 29 additions & 3 deletions src/cli/commands/branch.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,34 @@ function splitGateReviewFlags(args) {
const scriptArgs = [];
let gateReview = false;
let reviewProvider;
let gateAutofix = false;
let gateAutofixRounds = 1;
for (let index = 0; index < args.length; index += 1) {
const arg = args[index];
if (arg === '--gate-review') {
gateReview = true;
} else if (arg === '--no-gate-review' || arg === '--skip-review-gate') {
gateReview = false;
} else if (arg === '--gate-autofix') {
gateAutofix = true;
} else if (arg === '--no-gate-autofix') {
gateAutofix = false;
} else if (arg === '--gate-autofix-rounds' || arg.startsWith('--gate-autofix-rounds=')) {
// Consume the value too, for the same reason as --review-provider: a bare
// number left behind becomes a positional the shell script rejects.
let raw;
if (arg.includes('=')) {
raw = arg.slice('--gate-autofix-rounds='.length);
} else {
raw = args[index + 1];
index += 1;
}
const rounds = Number.parseInt(String(raw ?? ''), 10);
if (!Number.isInteger(rounds) || rounds < 1 || rounds > 5) {
throw new Error('--gate-autofix-rounds requires an integer between 1 and 5');
}
gateAutofixRounds = rounds;
gateAutofix = true;
} else if (arg === '--review-provider') {
// Consume the value too — leaving it behind would hand the script a bare
// "claude" positional and it would exit 1. A missing value becomes "" so
Expand All @@ -51,7 +73,9 @@ function splitGateReviewFlags(args) {
}
}

return { gateReview, reviewProvider, scriptArgs };
return {
gateReview, reviewProvider, gateAutofix, gateAutofixRounds, scriptArgs,
};
}

// Read `--flag value` or `--flag=value` out of an argv array.
Expand All @@ -73,7 +97,9 @@ function branch(rawArgs) {
if (subcommand === 'finish') {
const { target, passthrough } = extractTargetedArgs(rest);
const repoRoot = resolveRepoRoot(target);
const { gateReview, reviewProvider, scriptArgs } = splitGateReviewFlags(passthrough);
const {
gateReview, reviewProvider, gateAutofix, gateAutofixRounds, scriptArgs,
} = splitGateReviewFlags(passthrough);
// Fail-closed: runReviewGate throws on a dirty review, red CI, or a PR
// GitHub will not merge. Throwing here means the script never runs, so
// the merge never happens.
Expand All @@ -88,7 +114,7 @@ function branch(rawArgs) {
baseBranch: resolveFinishBaseBranch(repoRoot, gatedBranch, readFlagValue(scriptArgs, '--base')),
// review-gate.js falls back to its own default when this is undefined,
// which keeps a bare --gate-review behaving exactly as before.
options: { reviewProvider },
options: { reviewProvider, gateAutofix, gateAutofixRounds },
});
}
invokePackageAsset('branchFinish', scriptArgs, {
Expand Down
Loading
Loading