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
1 change: 1 addition & 0 deletions apps/gittensory-ui/src/routes/docs.privacy-security.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ GITTENSORY_REVIEW_UNIFIED_COMMENT="true" # one in-place unified PR comme
GITTENSORY_REVIEW_ENRICHMENT="true" # external analyzer registry (REES) findings
GITTENSORY_REVIEW_INLINE_COMMENTS="true" # diff-anchored inline PR review comments
GITTENSORY_REVIEW_TEST_GENERATION="true" # boundary-safe test-gen action spec (contributor-run)
GITTENSORY_REVIEW_FIX_HANDOFF="true" # machine-readable fix-handoff block (contributor-run)
GITTENSORY_REVIEW_PLANNER="true" # @gittensory plan on-demand implementation plan
GITTENSORY_REVIEW_SCREENSHOTS="true" # before/after visual capture for UI changes

Expand Down
5 changes: 5 additions & 0 deletions apps/gittensory-ui/src/routes/docs.tuning.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,11 @@ function Tuning() {
for the repo's detected framework; the contributor's own agent scaffolds and runs the
tests locally. Per-PR.
</li>
<li>
<code>GITTENSORY_REVIEW_FIX_HANDOFF</code> — renders a review finding as a structured,
machine-readable "apply this fix" block for the contributor's own local agent to consume —
content only, no server-side write, no execution. Per-PR.
</li>
<li>
<code>GITTENSORY_REVIEW_PLANNER</code> — enables <code>@gittensory plan</code>, an
on-demand structured implementation plan posted to the PR thread. Per-PR.
Expand Down
6 changes: 6 additions & 0 deletions src/env.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,12 @@ declare global {
* on the contributor's own machine (no source upload, no server-side write). Default OFF — unset/false
* keeps the review path byte-identical (no spec is ever built). */
GITTENSORY_REVIEW_TEST_GENERATION?: string;
/** Fix-handoff blocks (#2176, config slice of #1962): when truthy (AND the repo is in GITTENSORY_REVIEW_REPOS
* AND the repo's `.gittensory.yml` sets `review.fixHandoff: true`), a review finding is ALSO rendered as a
* structured, machine-readable "apply this fix" block (src/review/fix-handoff-render.ts) for the
* contributor's OWN local agent to consume — content only, no server-side write, no execution. Default
* OFF — unset/false keeps the review path byte-identical (no block is ever built). */
GITTENSORY_REVIEW_FIX_HANDOFF?: string;
/** Convergence (safety): when truthy, the ported safety scan runs in the review path — (1) untrusted PR
* title/body/diff is defanged (prompt-injection neutralized) before it reaches the AI reviewer, and (2)
* the PR diff is scanned for leaked secrets, surfacing a `secret_leak` blocker. Default OFF —
Expand Down
122 changes: 37 additions & 85 deletions src/mcp/local-write-tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,91 +43,6 @@ export function buildFileIssueSpec(input: { repoFullName: string; title: string;
return spec("file_issue", "File a new issue.", { repoFullName: input.repoFullName, title: input.title, body: input.body, labels }, command);
}

export type DeferredReviewFinding = {
title: string;
detail: string;
path?: string | undefined;
action?: string | undefined;
};

const FOLLOW_UP_ISSUE_TITLE_MAX = 120;
const FOLLOW_UP_ISSUE_BODY_MAX = 4000;

function stripFollowUpMarkers(value: string): string {
return value.replace(/<!--[\s\S]*?-->/g, "").replace(/\r\n/g, "\n").trim();
}

function boundFollowUpLine(value: string, max: number): string {
const cleaned = stripFollowUpMarkers(value).replace(/\s+/g, " ").trim();
if (cleaned.length <= max) return cleaned;
return `${cleaned.slice(0, Math.max(0, max - 1)).trimEnd()}…`;
}

function boundFollowUpBody(value: string, max: number): string {
const cleaned = stripFollowUpMarkers(value).trim();
if (cleaned.length <= max) return cleaned;
return `${cleaned.slice(0, Math.max(0, max - 1)).trimEnd()}…`;
}

function composeFollowUpIssueTitle(finding: DeferredReviewFinding): string {
const cleaned = stripFollowUpMarkers(finding.title);
if (/^follow-up:/i.test(cleaned)) {
return boundFollowUpLine(cleaned, FOLLOW_UP_ISSUE_TITLE_MAX);
}
const prefix = "Follow-up: ";
return `${prefix}${boundFollowUpLine(cleaned, FOLLOW_UP_ISSUE_TITLE_MAX - prefix.length)}`;
}

function composeFollowUpIssueBody(input: { finding: DeferredReviewFinding; pullNumber?: number | undefined }): string {
const lines: string[] = [];
if (input.pullNumber !== undefined) lines.push(`Deferred from review on PR #${input.pullNumber}.`);
if (input.finding.path) lines.push(`File: \`${input.finding.path}\``);
lines.push("", boundFollowUpLine(input.finding.detail, FOLLOW_UP_ISSUE_BODY_MAX));
if (input.finding.action) {
lines.push("", "**Suggested next step**", boundFollowUpLine(input.finding.action, 500));
}
lines.push("", "_Filed locally from a deferred review finding — gittensory supplies content only._");
return boundFollowUpBody(lines.join("\n"), FOLLOW_UP_ISSUE_BODY_MAX);
}

function sanitizeFollowUpFinding(finding: DeferredReviewFinding): Record<string, string> {
const sanitized: Record<string, string> = {
title: stripFollowUpMarkers(finding.title),
detail: stripFollowUpMarkers(finding.detail),
};
if (finding.path) sanitized.path = finding.path;
if (finding.action) sanitized.action = stripFollowUpMarkers(finding.action);
return sanitized;
}

/** File a follow-up issue for a deferred review finding (#2177, #1962 slice). */
export function buildFollowUpIssueSpec(input: {
repoFullName: string;
finding: DeferredReviewFinding;
labels?: string[] | undefined;
pullNumber?: number | undefined;
}): LocalWriteActionSpec {
const sanitizedFinding = sanitizeFollowUpFinding(input.finding);
const title = composeFollowUpIssueTitle(input.finding);
const body = composeFollowUpIssueBody({ finding: input.finding, pullNumber: input.pullNumber });
const fileSpec = buildFileIssueSpec({
repoFullName: input.repoFullName,
title,
body,
labels: input.labels,
});
return {
...fileSpec,
action: "follow_up_issue",
description: `File a follow-up issue for a deferred review finding: ${title}`,
inputs: {
...fileSpec.inputs,
finding: sanitizedFinding,
...(input.pullNumber !== undefined ? { pullNumber: input.pullNumber } : {}),
},
};
}

/** Add labels to an issue or PR (gh issue edit also targets PRs). */
export function buildApplyLabelsSpec(input: { repoFullName: string; number: number; labels: string[] }): LocalWriteActionSpec {
const labelArgs = input.labels.map((label) => ` --add-label ${sq(label)}`).join("");
Expand Down Expand Up @@ -181,3 +96,40 @@ export function buildTestGenSpec(input: {
command,
);
}

// #2177 (follow-up-issue slice of #1962). Reuses buildFileIssueSpec's exact spec shape ("file_issue") — a
// deferred review finding is just another issue-worth-filing content source, so there is no new spec verb or
// no-cloud-write boundary here, only a deterministic title/body composer in front of the SAME builder.
const FOLLOW_UP_ISSUE_TITLE_MAX = 200;
const FOLLOW_UP_ISSUE_BODY_MAX = 4000;

// Strip any machine-readable marker (e.g. fix-handoff's HTML comment marker, or a stray fenced block) before
// the finding's text becomes issue content — a follow-up issue is read by a HUMAN triaging a backlog, not a
// harness, so it should read as prose, not carry an internal marker meant for a different consumer.
function stripMachineMarkers(text: string): string {
return text
.replace(/<!--[\s\S]*?-->/g, "")
.replace(/```[\s\S]*?```/g, "")
.replace(/\s+/g, " ")
.trim();
}

/** Build a LOCAL-execution spec to file a follow-up issue for a review finding a maintainer wants TRACKED
* rather than blocked on this PR. Composes a bounded, public-safe title/body from the finding and delegates to
* {@link buildFileIssueSpec}'s exact "file_issue" spec shape — no new write path. `label` is optional: when the
* caller supplies a point-bearing label (e.g. "gittensor:bug"), the follow-up carries it so the tracked issue
* is itself a scored, actionable contribution target; omitted ⇒ no labels at all (empty-label branch). */
export function buildFollowUpIssueSpec(input: {
repoFullName: string;
path: string;
line?: number | undefined;
finding: string;
label?: string | null | undefined;
}): LocalWriteActionSpec {
const location = input.line && input.line > 0 ? `${input.path}:${input.line}` : input.path;
const safeFinding = stripMachineMarkers(input.finding).slice(0, FOLLOW_UP_ISSUE_BODY_MAX);
const title = `Follow up: ${location}`.slice(0, FOLLOW_UP_ISSUE_TITLE_MAX);
const body = `Deferred review finding at \`${location}\`:\n\n${safeFinding}`.slice(0, FOLLOW_UP_ISSUE_BODY_MAX);
const labels = input.label ? [input.label] : [];
return buildFileIssueSpec({ repoFullName: input.repoFullName, title, body, labels });
}
19 changes: 19 additions & 0 deletions src/mcp/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ import {
buildCreateBranchSpec,
buildDeleteBranchSpec,
buildFileIssueSpec,
buildFollowUpIssueSpec,
buildOpenPrSpec,
buildPostEligibilityCommentSpec,
buildTestGenSpec,
Expand Down Expand Up @@ -325,6 +326,14 @@ const testGenShape = {
testDir: z.string().min(1).max(255).optional(),
criteria: z.array(z.string().min(1).max(300)).max(20).optional(),
};
// #2177 (follow-up-issue slice of #1962): composes a file_issue spec from a single deferred review finding.
const followUpIssueShape = {
repoFullName: z.string().min(3).max(SCENARIO_MAX_REPO_FULL_NAME_CHARS),
path: z.string().min(1).max(500),
line: z.number().int().positive().optional(),
finding: z.string().min(1).max(WRITE_TOOL_BODY_MAX),
label: z.string().min(1).max(100).optional(),
};
const localWriteActionOutputSchema = {
action: z.string(),
description: z.string(),
Expand Down Expand Up @@ -1484,6 +1493,16 @@ export class GittensoryMcp {
},
async (input) => this.toolResult(this.localWriteSpec(buildTestGenSpec(input))),
);
server.registerTool(
"gittensory_file_follow_up_issue",
{
description:
"Build a LOCAL-execution spec to file a follow-up issue for a review finding a maintainer wants TRACKED rather than blocked on this PR. Composes a bounded, public-safe title/body from the finding (run it with your own gh creds; gittensory never performs the write).",
inputSchema: followUpIssueShape,
outputSchema: localWriteActionOutputSchema,
},
async (input) => this.toolResult(this.localWriteSpec(buildFollowUpIssueSpec(input))),
);

// #783 multi-step plan DAG — stateless: pass the plan back each call.
server.registerTool(
Expand Down
70 changes: 70 additions & 0 deletions src/review/fix-handoff-render.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Fix-handoff block RENDERER (#2175, render slice of #1962 — the config/gate slice lives in
// src/review/fix-handoff.ts's isFixHandoffEnabled/shouldEmitFixHandoff). Turns a single review finding into a
// structured, machine-readable "apply this fix" block a CONTRIBUTOR'S OWN local coding agent can consume —
// content only, no server-side write, no execution. Mirrors formatInlineBody's severity-label composition
// (inline-comments.ts) and reuses the exact no-cloud-write boundary text every other local-execution artifact
// carries (local-write-tools.ts's LOCAL_WRITE_BOUNDARY), so the guarantee reads identically everywhere
// gittensory hands a contributor something to run themselves.
//
// The caller is responsible for gating emission via shouldEmitFixHandoff (fix-handoff.ts) BEFORE calling into
// this module — this file is pure rendering, public-safe by construction: it only renders fields the caller
// already produced through the public-safe filter (InlineFinding.body/suggestion are sanitized upstream by
// composeInlineFindings before they ever reach here — this module adds no new free text of its own beyond the
// fixed label/marker strings below).
import { LOCAL_WRITE_BOUNDARY } from "../mcp/local-write-tools";
import type { InlineFinding } from "../services/ai-review";

/** A single finding rendered as a structured, LOCAL-execution fix-handoff block. `line` is `0` when the
* finding has no commentable diff line (mirrors the codebase's existing path-only sentinel — see
* `secretLeakFinding`/`scanDiffForSecretsWithLocations` in review/safety.ts, review/secrets-scan.ts) so the
* block still identifies WHERE to look, even path-only. */
export type FixHandoffBlock = {
path: string;
line: number;
severity: "blocker" | "nit";
instruction: string;
suggestedChange?: string | undefined;
/** The rendered, machine-readable markdown block (fenced + an HTML comment marker a harness can grep for). */
body: string;
boundary: string;
};

/** The HTML comment marker prefixing every rendered block, so a contributor's own agent can reliably locate and
* parse fix-handoff blocks in a comment body without depending on markdown structure alone. */
const FIX_HANDOFF_MARKER = "<!-- gittensory:fix-handoff -->";

/** PURE: build a single finding's fix-handoff block. Never throws; a finding whose `line` is not a positive
* integer (0, negative, non-finite — i.e. "no commentable line") still yields a valid PATH-ONLY block rather
* than being dropped, since the finding itself is still actionable context even without a line anchor. */
export function buildFixHandoffBlock(finding: InlineFinding): FixHandoffBlock {
const hasLine = Number.isInteger(finding.line) && finding.line > 0;
const line = hasLine ? finding.line : 0;
const location = hasLine ? `${finding.path}:${line}` : `${finding.path} (no specific line)`;
const label = finding.severity === "blocker" ? "Blocker" : "Nit";
const suggestedChange = finding.suggestion?.trim() || undefined;
const suggestionBlock = suggestedChange ? `\n\nSuggested change:\n\`\`\`\n${suggestedChange}\n\`\`\`` : "";
const body = [
FIX_HANDOFF_MARKER,
`**Fix handoff — ${label} at \`${location}\`**`,
finding.body,
suggestionBlock,
`\n_${LOCAL_WRITE_BOUNDARY}_`,
]
.filter((part) => part.length > 0)
.join("\n");
return {
path: finding.path,
line,
severity: finding.severity,
instruction: finding.body,
...(suggestedChange !== undefined ? { suggestedChange } : {}),
body,
boundary: LOCAL_WRITE_BOUNDARY,
};
}

/** PURE: build a fix-handoff block for every finding in order. Empty in ⇒ empty out — no-op when there is
* nothing to hand off. */
export function buildFixHandoffBlocks(findings: InlineFinding[]): FixHandoffBlock[] {
return findings.map((finding) => buildFixHandoffBlock(finding));
}
85 changes: 85 additions & 0 deletions test/unit/fix-handoff-render.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import { describe, expect, it } from "vitest";
import { buildFixHandoffBlock, buildFixHandoffBlocks } from "../../src/review/fix-handoff-render";
import { LOCAL_WRITE_BOUNDARY } from "../../src/mcp/local-write-tools";
import type { InlineFinding } from "../../src/services/ai-review";

function finding(over: Partial<InlineFinding> = {}): InlineFinding {
return { path: "src/a.ts", line: 12, severity: "blocker", body: "Null check missing before dereference.", ...over };
}

describe("buildFixHandoffBlock (#2175)", () => {
it("renders a path:line location and blocker label", () => {
const block = buildFixHandoffBlock(finding({ line: 12, severity: "blocker" }));
expect(block).toMatchObject({ path: "src/a.ts", line: 12, severity: "blocker", instruction: "Null check missing before dereference." });
expect(block.body).toContain("src/a.ts:12");
expect(block.body).toContain("**Fix handoff — Blocker at `src/a.ts:12`**");
});

it("renders a nit label", () => {
const block = buildFixHandoffBlock(finding({ severity: "nit" }));
expect(block.body).toContain("Nit at");
});

it("includes the suggestedChange fenced block when present", () => {
const block = buildFixHandoffBlock(finding({ suggestion: "if (!value) return null;" }));
expect(block.suggestedChange).toBe("if (!value) return null;");
expect(block.body).toContain("Suggested change:");
expect(block.body).toContain("```\nif (!value) return null;\n```");
});

it("omits suggestedChange entirely when absent", () => {
const block = buildFixHandoffBlock(finding());
expect(block.suggestedChange).toBeUndefined();
expect(block.body).not.toContain("Suggested change:");
});

it("omits suggestedChange when the suggestion is whitespace-only", () => {
const block = buildFixHandoffBlock(finding({ suggestion: " " }));
expect(block.suggestedChange).toBeUndefined();
expect(block.body).not.toContain("Suggested change:");
});

it("yields a path-only block (line 0) when the finding has no commentable line (line <= 0)", () => {
const block = buildFixHandoffBlock(finding({ line: 0 }));
expect(block.line).toBe(0);
expect(block.body).toContain("src/a.ts (no specific line)");
expect(block.body).not.toContain("src/a.ts:0");
});

it("yields a path-only block when line is negative or non-finite (defensive)", () => {
expect(buildFixHandoffBlock(finding({ line: -1 })).line).toBe(0);
expect(buildFixHandoffBlock(finding({ line: Number.NaN })).line).toBe(0);
expect(buildFixHandoffBlock(finding({ line: 1.5 })).line).toBe(0); // non-integer isn't a valid diff line either
});

it("always includes the exact LOCAL_WRITE_BOUNDARY text (boundary-safe)", () => {
const block = buildFixHandoffBlock(finding());
expect(block.boundary).toBe(LOCAL_WRITE_BOUNDARY);
expect(block.body).toContain(LOCAL_WRITE_BOUNDARY);
});

it("includes the fix-handoff HTML comment marker so a harness can locate the block", () => {
const block = buildFixHandoffBlock(finding());
expect(block.body).toContain("<!-- gittensory:fix-handoff -->");
});

it("carries the finding's body as the instruction verbatim (already public-safe upstream)", () => {
const block = buildFixHandoffBlock(finding({ body: "Add a guard for the empty-array case." }));
expect(block.instruction).toBe("Add a guard for the empty-array case.");
expect(block.body).toContain("Add a guard for the empty-array case.");
});
});

describe("buildFixHandoffBlocks (#2175)", () => {
it("maps every finding in order", () => {
const blocks = buildFixHandoffBlocks([finding({ path: "a.ts", line: 1 }), finding({ path: "b.ts", line: 2, severity: "nit" })]);
expect(blocks).toHaveLength(2);
expect(blocks[0]?.path).toBe("a.ts");
expect(blocks[1]?.path).toBe("b.ts");
expect(blocks[1]?.severity).toBe("nit");
});

it("returns an empty array for no findings (no-op)", () => {
expect(buildFixHandoffBlocks([])).toEqual([]);
});
});
Loading
Loading