diff --git a/.github/workflows/repo-analysis.yml b/.github/workflows/repo-analysis.yml index 8520c9e5..82b27451 100644 --- a/.github/workflows/repo-analysis.yml +++ b/.github/workflows/repo-analysis.yml @@ -29,12 +29,14 @@ jobs: ref: ${{ inputs.ref }} fetch-depth: 0 + - uses: pnpm/action-setup@b906affcce14559ad1aafd4ab0e942779e9f58b1 # v4 + - uses: denoland/setup-deno@e95548e56dfa95d4e1a28d6f422fafe75c4c26fb # v2.0.3 with: deno-version: v2.9.5 - - name: Install dependencies - run: deno task deps + - name: Prepare the checked-out xmd binary + run: deno task setup - name: Build the checked-out xmd binary run: deno task build diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index 3abbda17..cf3cc40d 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -15,12 +15,14 @@ jobs: with: fetch-depth: 0 + - uses: pnpm/action-setup@b906affcce14559ad1aafd4ab0e942779e9f58b1 # v4 + - uses: denoland/setup-deno@e95548e56dfa95d4e1a28d6f422fafe75c4c26fb # v2.0.3 with: deno-version: v2.9.5 - - name: Install dependencies - run: deno task deps + - name: Prepare the checked-out xmd binary + run: deno task setup - name: Build the checked-out xmd binary run: deno task build diff --git a/.reviews/.oxlintrc.json b/.reviews/.oxlintrc.json index 274f333c..1064bf78 100644 --- a/.reviews/.oxlintrc.json +++ b/.reviews/.oxlintrc.json @@ -29,6 +29,19 @@ "eslint/no-console": ["warn", { "allow": ["warn", "error"] }], "eslint/no-debugger": "warn", + "eslint/func-style": "off", + "eslint/no-magic-numbers": "off", + "eslint/require-yield": "off", + "eslint/sort-keys": "off", + "eslint/sort-imports": "off", + "typescript/prefer-readonly-parameter-types": "off", + "import/exports-last": "off", + "import/group-exports": "off", + "import/no-named-export": "off", + "import/prefer-default-export": "off", + "import/consistent-type-specifier-style": "off", + "unicorn/filename-case": "off", + "typescript/no-unnecessary-type-arguments": "warn", "typescript/no-unnecessary-type-assertion": "warn", "typescript/no-redundant-type-constituents": "warn", diff --git a/.reviews/AnalyzeRepo.md b/.reviews/AnalyzeRepo.md index ed8e7ced..e10db393 100644 --- a/.reviews/AnalyzeRepo.md +++ b/.reviews/AnalyzeRepo.md @@ -2,171 +2,42 @@ title: Repository Analysis --- -```bash silent exec -mkdir -p .reviews -cat > .reviews/tsconfig.oxlint.json << 'TSCONFIG' -{ - "compilerOptions": { - "target": "ESNext", - "module": "ESNext", - "moduleResolution": "bundler", - "strict": true, - "noEmit": true, - "skipLibCheck": true, - "resolveJsonModule": true, - "lib": ["ESNext", "DOM"], - "types": [] - }, - "include": ["packages/*/src/**/*.ts", "packages/*/*.ts", "durable-effects/**/*.ts"], - "exclude": ["node_modules", "dist", ".vendor", "**/*.test.ts"] -} -TSCONFIG -``` - -```bash silent exec -ollama show qwen3:30b-a3b >/dev/null 2>&1 || ollama pull qwen3:30b-a3b -``` + + - - -```bash exec -find durable-effects packages -name '*.ts' -not -name '*.test.ts' -not -name '*.spec.ts' -not -path '*/node_modules/*' 2>/dev/null | tee /tmp/xmd-repo-files.txt | wc -l | tr -d ' ' -``` - - - - - -```bash exec -cat /tmp/xmd-repo-files.txt | xargs wc -l 2>/dev/null | tail -1 | awk '{print $1}' -``` - - - - - -```bash exec -cat /tmp/xmd-repo-files.txt -``` - - + ```ts eval -const fileCount = parseInt(repoStats.trim(), 10) || 0; -const lineCount = parseInt(repoLineCount.trim(), 10) || 0; - +const fileList = inventory.fileList; +const fileCount = inventory.fileCount; +const lineCount = inventory.lineCount; const pr = { - files: [], added: [], removed: [], created: [], modified: [], deleted: [], + files: [], + added: [], + removed: [], + created: [], + modified: [], + deleted: [], directories: new Set(), - addedSource: "", diffPreview: "", + addedSource: "", + diffPreview: "", stats: { totalFiles: fileCount, additions: lineCount, deletions: 0, totalChanges: lineCount }, meta: { title: "Repo Analysis", body: "", number: "" }, }; ``` - - - - - - -```ts eval -import { parseDoctorResult } from "@executablemd/code-review-agent"; - -const doctor = parseDoctorResult(doctorJson); -``` - - - - - -```bash exec -OUT=$(OXLINT_TSGOLINT_PATH=.reviews/.oxlint/tsgolint .reviews/.oxlint/oxlint --config .reviews/.oxlintrc.json --type-aware --tsconfig .reviews/tsconfig.oxlint.json --format json 2>/dev/null || true) -if [ -z "$OUT" ] || ! printf '%s' "$OUT" | jq -c ' - def entries: - if type == "array" then . - elif (.diagnostics? | type) == "array" then .diagnostics - else [] - end; - def span_line: - if (.line? | type) == "number" then .line - elif (.labels?[0].span.line? | type) == "number" then .labels[0].span.line - else 0 - end; - def span_column: - if (.column? | type) == "number" then .column - elif (.labels?[0].span.column? | type) == "number" then .labels[0].span.column - else 0 - end; - entries - | map({ - message: (if (.message? | type) == "string" then .message else "" end), - ruleId: (if (.ruleId? | type) == "string" then .ruleId elif (.code? | type) == "string" then .code else "unknown" end), - severity: (if .severity == "error" then "error" else "warning" end), - file: (if (.file? | type) == "string" then .file elif (.filename? | type) == "string" then .filename else "" end), - line: span_line, - column: span_column - }) -'; then - printf '[]' -fi -``` - - + - - -```bash exec -OUT=$(.reviews/.oxlint/oxlint --config .reviews/.oxlintrc.json --format json 2>/dev/null || true) -if [ -z "$OUT" ] || ! printf '%s' "$OUT" | jq -c ' - def entries: - if type == "array" then . - elif (.diagnostics? | type) == "array" then .diagnostics - else [] - end; - def span_line: - if (.line? | type) == "number" then .line - elif (.labels?[0].span.line? | type) == "number" then .labels[0].span.line - else 0 - end; - def span_column: - if (.column? | type) == "number" then .column - elif (.labels?[0].span.column? | type) == "number" then .labels[0].span.column - else 0 - end; - entries - | map({ - message: (if (.message? | type) == "string" then .message else "" end), - ruleId: (if (.ruleId? | type) == "string" then .ruleId elif (.code? | type) == "string" then .code else "unknown" end), - severity: (if .severity == "error" then "error" else "warning" end), - file: (if (.file? | type) == "string" then .file elif (.filename? | type) == "string" then .filename else "" end), - line: span_line, - column: span_column - }) -'; then - printf '[]' -fi -``` - - - - - -[] - - - - + ```ts eval -import { - buildCleanupAnalysis, - parseDiagnostics, -} from "@executablemd/code-review-agent"; +import { buildCleanupAnalysis, buildDiagnostics } from "@executablemd/code-review-agent"; -const diagnostics = parseDiagnostics(rawDiagnostics, pr, doctor); +const diagnostics = buildDiagnostics(rawDiagnostics, pr, doctor); const cleanupAnalysis = buildCleanupAnalysis(diagnostics); ``` @@ -177,3 +48,5 @@ const cleanupAnalysis = buildCleanupAnalysis(diagnostics); + + diff --git a/.reviews/AnalyzeRepoCI.md b/.reviews/AnalyzeRepoCI.md index 90539595..4ee99b25 100644 --- a/.reviews/AnalyzeRepoCI.md +++ b/.reviews/AnalyzeRepoCI.md @@ -4,167 +4,42 @@ title: Repository Analysis (CI) -```bash silent exec -mkdir -p .reviews -cat > .reviews/tsconfig.oxlint.json << 'TSCONFIG' -{ - "compilerOptions": { - "target": "ESNext", - "module": "ESNext", - "moduleResolution": "bundler", - "strict": true, - "noEmit": true, - "skipLibCheck": true, - "resolveJsonModule": true, - "lib": ["ESNext", "DOM"], - "types": [] - }, - "include": ["packages/*/src/**/*.ts", "packages/*/*.ts", "durable-effects/**/*.ts"], - "exclude": ["node_modules", "dist", ".vendor", "**/*.test.ts"] -} -TSCONFIG -``` - - - -```bash exec -find durable-effects packages -name '*.ts' -not -name '*.test.ts' -not -name '*.spec.ts' -not -path '*/node_modules/*' 2>/dev/null | tee /tmp/xmd-repo-files.txt | wc -l | tr -d ' ' -``` - - - - - -```bash exec -cat /tmp/xmd-repo-files.txt | xargs wc -l 2>/dev/null | tail -1 | awk '{print $1}' -``` + + - - - - -```bash exec -cat /tmp/xmd-repo-files.txt -``` - - + ```ts eval -const fileCount = parseInt(repoStats.trim(), 10) || 0; -const lineCount = parseInt(repoLineCount.trim(), 10) || 0; - +const fileList = inventory.fileList; +const fileCount = inventory.fileCount; +const lineCount = inventory.lineCount; const pr = { - files: [], added: [], removed: [], created: [], modified: [], deleted: [], + files: [], + added: [], + removed: [], + created: [], + modified: [], + deleted: [], directories: new Set(), - addedSource: "", diffPreview: "", + addedSource: "", + diffPreview: "", stats: { totalFiles: fileCount, additions: lineCount, deletions: 0, totalChanges: lineCount }, meta: { title: "Repo Analysis", body: "", number: "" }, }; ``` - - - - - - -```ts eval -import { parseDoctorResult } from "@executablemd/code-review-agent"; - -const doctor = parseDoctorResult(doctorJson); -``` - - - - - -```bash exec -OUT=$(OXLINT_TSGOLINT_PATH=.reviews/.oxlint/tsgolint .reviews/.oxlint/oxlint --config .reviews/.oxlintrc.json --type-aware --tsconfig .reviews/tsconfig.oxlint.json --format json 2>/dev/null || true) -if [ -z "$OUT" ] || ! printf '%s' "$OUT" | jq -c ' - def entries: - if type == "array" then . - elif (.diagnostics? | type) == "array" then .diagnostics - else [] - end; - def span_line: - if (.line? | type) == "number" then .line - elif (.labels?[0].span.line? | type) == "number" then .labels[0].span.line - else 0 - end; - def span_column: - if (.column? | type) == "number" then .column - elif (.labels?[0].span.column? | type) == "number" then .labels[0].span.column - else 0 - end; - entries - | map({ - message: (if (.message? | type) == "string" then .message else "" end), - ruleId: (if (.ruleId? | type) == "string" then .ruleId elif (.code? | type) == "string" then .code else "unknown" end), - severity: (if .severity == "error" then "error" else "warning" end), - file: (if (.file? | type) == "string" then .file elif (.filename? | type) == "string" then .filename else "" end), - line: span_line, - column: span_column - }) -'; then - printf '[]' -fi -``` - - + - - -```bash exec -OUT=$(.reviews/.oxlint/oxlint --config .reviews/.oxlintrc.json --format json 2>/dev/null || true) -if [ -z "$OUT" ] || ! printf '%s' "$OUT" | jq -c ' - def entries: - if type == "array" then . - elif (.diagnostics? | type) == "array" then .diagnostics - else [] - end; - def span_line: - if (.line? | type) == "number" then .line - elif (.labels?[0].span.line? | type) == "number" then .labels[0].span.line - else 0 - end; - def span_column: - if (.column? | type) == "number" then .column - elif (.labels?[0].span.column? | type) == "number" then .labels[0].span.column - else 0 - end; - entries - | map({ - message: (if (.message? | type) == "string" then .message else "" end), - ruleId: (if (.ruleId? | type) == "string" then .ruleId elif (.code? | type) == "string" then .code else "unknown" end), - severity: (if .severity == "error" then "error" else "warning" end), - file: (if (.file? | type) == "string" then .file elif (.filename? | type) == "string" then .filename else "" end), - line: span_line, - column: span_column - }) -'; then - printf '[]' -fi -``` - - - - - -[] - - - - + ```ts eval -import { - buildCleanupAnalysis, - parseDiagnostics, -} from "@executablemd/code-review-agent"; +import { buildCleanupAnalysis, buildDiagnostics } from "@executablemd/code-review-agent"; -const diagnostics = parseDiagnostics(rawDiagnostics, pr, doctor); +const diagnostics = buildDiagnostics(rawDiagnostics, pr, doctor); const cleanupAnalysis = buildCleanupAnalysis(diagnostics); ``` @@ -176,4 +51,6 @@ const cleanupAnalysis = buildCleanupAnalysis(diagnostics); + + diff --git a/.reviews/ReviewPR.local.md b/.reviews/ReviewPR.local.md index c852eab9..6a4a21da 100644 --- a/.reviews/ReviewPR.local.md +++ b/.reviews/ReviewPR.local.md @@ -2,198 +2,31 @@ title: PR Review (local) --- -```ts eval -const BASE_SHA = process.env.BASE_SHA ?? "HEAD~1"; -const HEAD_SHA = process.env.HEAD_SHA ?? "HEAD"; -const PR_TITLE = process.env.PR_TITLE ?? ""; -const PR_BODY = process.env.PR_BODY ?? ""; -const PR_NUMBER = process.env.PR_NUMBER ?? ""; -``` - - - -```bash exec -git diff {BASE_SHA}...{HEAD_SHA} -``` + + - - - - -```bash exec -git diff --name-status {BASE_SHA}...{HEAD_SHA} -``` - - + + ```ts eval -import { parseDiff } from "@executablemd/code-review-agent"; - -const pr = parseDiff(rawDiff, rawFiles, { - title: PR_TITLE, - body: PR_BODY, - number: PR_NUMBER, -}); - -// TODO: we need an easier way to work with diffs here. -const changedFilePaths = pr.files.map((file) => file.path); -``` - -```bash silent exec -mkdir -p .reviews -cat > .reviews/tsconfig.oxlint.json << 'TSCONFIG' -{ - "compilerOptions": { - "target": "ESNext", - "module": "ESNext", - "moduleResolution": "bundler", - "strict": true, - "noEmit": true, - "skipLibCheck": true, - "resolveJsonModule": true, - "lib": ["ESNext", "DOM"], - "types": [] - }, - "include": [ - "packages/*/src/**/*.ts", - "packages/*/*.ts", - "durable-effects/**/*.ts" - ], - "exclude": ["node_modules", "dist", ".vendor", "**/*.test.ts"] -} -TSCONFIG -``` - -```bash silent exec -ollama show qwen3:30b-a3b >/dev/null 2>&1 || ollama pull qwen3:30b-a3b +const pr = context.pr; +const changedFilePaths = context.changedFilePaths; +const changedTsFiles = pr.files + .filter((file) => file.language === "typescript" && !file.isTest && !file.isTypeDeclaration) + .map((file) => file.path) + .slice(0, 200); ``` - - - - - + ```ts eval -import { parseDoctorResult } from "@executablemd/code-review-agent"; - -const doctor = parseDoctorResult(doctorJson); -``` - - +import { buildDiagnostics } from "@executablemd/code-review-agent"; -```bash silent exec -git diff --name-only {BASE_SHA}...{HEAD_SHA} -- '*.ts' '*.tsx' | grep -v '\.test\.' | grep -v '\.spec\.' | grep -v '\.d\.ts$' | head -200 -``` - - - - - - - -```bash exec -changed_files=$(cat <<'FILES' -{changedTsFiles} -FILES -) -if [ -z "$changed_files" ]; then - printf '[]' -else - raw=$(printf '%s\n' "$changed_files" | OXLINT_TSGOLINT_PATH=.reviews/.oxlint/tsgolint xargs .reviews/.oxlint/oxlint --config .reviews/.oxlintrc.json --type-aware --tsconfig .reviews/tsconfig.oxlint.json --format json 2>/dev/null || true) - if [ -z "$raw" ] || ! printf '%s' "$raw" | jq -c --arg changed "$changed_files" ' - def entries: - if type == "array" then . - elif (.diagnostics? | type) == "array" then .diagnostics - else [] - end; - def span_line: - if (.line? | type) == "number" then .line - elif (.labels?[0].span.line? | type) == "number" then .labels[0].span.line - else 0 - end; - def span_column: - if (.column? | type) == "number" then .column - elif (.labels?[0].span.column? | type) == "number" then .labels[0].span.column - else 0 - end; - entries - | map({ - message: (if (.message? | type) == "string" then .message else "" end), - ruleId: (if (.ruleId? | type) == "string" then .ruleId elif (.code? | type) == "string" then .code else "unknown" end), - severity: (if .severity == "error" then "error" else "warning" end), - file: (if (.file? | type) == "string" then .file elif (.filename? | type) == "string" then .filename else "" end), - line: span_line, - column: span_column - }) - | map(select(.file as $file | ($changed | split("\n") | index($file)) != null)) - '; then - printf '[]' - fi -fi -``` - - - - - -```bash exec -changed_files=$(cat <<'FILES' -{changedTsFiles} -FILES -) -if [ -z "$changed_files" ]; then - printf '[]' -else - raw=$(printf '%s\n' "$changed_files" | xargs .reviews/.oxlint/oxlint --config .reviews/.oxlintrc.json --format json 2>/dev/null || true) - if [ -z "$raw" ] || ! printf '%s' "$raw" | jq -c --arg changed "$changed_files" ' - def entries: - if type == "array" then . - elif (.diagnostics? | type) == "array" then .diagnostics - else [] - end; - def span_line: - if (.line? | type) == "number" then .line - elif (.labels?[0].span.line? | type) == "number" then .labels[0].span.line - else 0 - end; - def span_column: - if (.column? | type) == "number" then .column - elif (.labels?[0].span.column? | type) == "number" then .labels[0].span.column - else 0 - end; - entries - | map({ - message: (if (.message? | type) == "string" then .message else "" end), - ruleId: (if (.ruleId? | type) == "string" then .ruleId elif (.code? | type) == "string" then .code else "unknown" end), - severity: (if .severity == "error" then "error" else "warning" end), - file: (if (.file? | type) == "string" then .file elif (.filename? | type) == "string" then .filename else "" end), - line: span_line, - column: span_column - }) - | map(select(.file as $file | ($changed | split("\n") | index($file)) != null)) - '; then - printf '[]' - fi -fi -``` - - - - - -[] - - - - - -```ts eval -import { parseDiagnostics } from "@executablemd/code-review-agent"; - -const diagnostics = parseDiagnostics(rawDiagnostics, pr, doctor); +const diagnostics = buildDiagnostics(rawDiagnostics, pr, doctor); ``` @@ -205,3 +38,5 @@ const diagnostics = parseDiagnostics(rawDiagnostics, pr, doctor); + + diff --git a/.reviews/ReviewPR.md b/.reviews/ReviewPR.md index ecea60f1..7d8063fc 100644 --- a/.reviews/ReviewPR.md +++ b/.reviews/ReviewPR.md @@ -4,202 +4,31 @@ title: PR Review -```ts eval -const BASE_SHA = process.env.BASE_SHA ?? "HEAD~1"; -const HEAD_SHA = process.env.HEAD_SHA ?? "HEAD"; -const PR_NUMBER = process.env.PR_NUMBER ?? ""; -const PR_TITLE = process.env.PR_TITLE ?? ""; -const GITHUB_REPOSITORY = process.env.GITHUB_REPOSITORY ?? ""; -``` - - - -```bash exec -git diff {BASE_SHA}...{HEAD_SHA} -``` - - + + - - -```bash exec -git diff --name-status {BASE_SHA}...{HEAD_SHA} -``` - - - - - -```bash exec -gh api repos/{GITHUB_REPOSITORY}/pulls/{PR_NUMBER} --jq '.body' 2>/dev/null || echo "" -``` - - + + ```ts eval -import { parseDiff } from "@executablemd/code-review-agent"; - -const pr = parseDiff(rawDiff, rawFiles, { - title: PR_TITLE, - body: prBody.trim(), - number: PR_NUMBER, -}); - -// TODO: we need an easier way to work with diffs here. -const changedFilePaths = pr.files.map((file) => file.path); -``` - -```bash silent exec -mkdir -p .reviews -cat > .reviews/tsconfig.oxlint.json << 'TSCONFIG' -{ - "compilerOptions": { - "target": "ESNext", - "module": "ESNext", - "moduleResolution": "bundler", - "strict": true, - "noEmit": true, - "skipLibCheck": true, - "resolveJsonModule": true, - "lib": ["ESNext", "DOM"], - "types": [] - }, - "include": [ - "packages/*/src/**/*.ts", - "packages/*/*.ts", - "durable-effects/**/*.ts" - ], - "exclude": ["node_modules", "dist", ".vendor", "**/*.test.ts"] -} -TSCONFIG +const pr = context.pr; +const changedFilePaths = context.changedFilePaths; +const changedTsFiles = pr.files + .filter((file) => file.language === "typescript" && !file.isTest && !file.isTypeDeclaration) + .map((file) => file.path) + .slice(0, 200); ``` - - - - - + ```ts eval -import { parseDoctorResult } from "@executablemd/code-review-agent"; +import { buildDiagnostics } from "@executablemd/code-review-agent"; -const doctor = parseDoctorResult(doctorJson); -``` - - - -```bash silent exec -git diff --name-only {BASE_SHA}...{HEAD_SHA} -- '*.ts' '*.tsx' | grep -v '\.test\.' | grep -v '\.spec\.' | grep -v '\.d\.ts$' | head -200 -``` - - - - - - - -```bash exec -changed_files=$(cat <<'FILES' -{changedTsFiles} -FILES -) -if [ -z "$changed_files" ]; then - printf '[]' -else - raw=$(printf '%s\n' "$changed_files" | OXLINT_TSGOLINT_PATH=.reviews/.oxlint/tsgolint xargs .reviews/.oxlint/oxlint --config .reviews/.oxlintrc.json --type-aware --tsconfig .reviews/tsconfig.oxlint.json --format json 2>/dev/null || true) - if [ -z "$raw" ] || ! printf '%s' "$raw" | jq -c --arg changed "$changed_files" ' - def entries: - if type == "array" then . - elif (.diagnostics? | type) == "array" then .diagnostics - else [] - end; - def span_line: - if (.line? | type) == "number" then .line - elif (.labels?[0].span.line? | type) == "number" then .labels[0].span.line - else 0 - end; - def span_column: - if (.column? | type) == "number" then .column - elif (.labels?[0].span.column? | type) == "number" then .labels[0].span.column - else 0 - end; - entries - | map({ - message: (if (.message? | type) == "string" then .message else "" end), - ruleId: (if (.ruleId? | type) == "string" then .ruleId elif (.code? | type) == "string" then .code else "unknown" end), - severity: (if .severity == "error" then "error" else "warning" end), - file: (if (.file? | type) == "string" then .file elif (.filename? | type) == "string" then .filename else "" end), - line: span_line, - column: span_column - }) - | map(select(.file as $file | ($changed | split("\n") | index($file)) != null)) - '; then - printf '[]' - fi -fi -``` - - - - - -```bash exec -changed_files=$(cat <<'FILES' -{changedTsFiles} -FILES -) -if [ -z "$changed_files" ]; then - printf '[]' -else - raw=$(printf '%s\n' "$changed_files" | xargs .reviews/.oxlint/oxlint --config .reviews/.oxlintrc.json --format json 2>/dev/null || true) - if [ -z "$raw" ] || ! printf '%s' "$raw" | jq -c --arg changed "$changed_files" ' - def entries: - if type == "array" then . - elif (.diagnostics? | type) == "array" then .diagnostics - else [] - end; - def span_line: - if (.line? | type) == "number" then .line - elif (.labels?[0].span.line? | type) == "number" then .labels[0].span.line - else 0 - end; - def span_column: - if (.column? | type) == "number" then .column - elif (.labels?[0].span.column? | type) == "number" then .labels[0].span.column - else 0 - end; - entries - | map({ - message: (if (.message? | type) == "string" then .message else "" end), - ruleId: (if (.ruleId? | type) == "string" then .ruleId elif (.code? | type) == "string" then .code else "unknown" end), - severity: (if .severity == "error" then "error" else "warning" end), - file: (if (.file? | type) == "string" then .file elif (.filename? | type) == "string" then .filename else "" end), - line: span_line, - column: span_column - }) - | map(select(.file as $file | ($changed | split("\n") | index($file)) != null)) - '; then - printf '[]' - fi -fi -``` - - - - - -[] - - - - - -```ts eval -import { parseDiagnostics } from "@executablemd/code-review-agent"; - -const diagnostics = parseDiagnostics(rawDiagnostics, pr, doctor); +const diagnostics = buildDiagnostics(rawDiagnostics, pr, doctor); ``` @@ -215,4 +44,6 @@ const diagnostics = parseDiagnostics(rawDiagnostics, pr, doctor); + + diff --git a/.reviews/components/CleanupIssues.md b/.reviews/components/CleanupIssues.md index 110458b9..8ff93ca9 100644 --- a/.reviews/components/CleanupIssues.md +++ b/.reviews/components/CleanupIssues.md @@ -11,31 +11,34 @@ props: --- ```ts persist eval -const repo = process.env.GITHUB_REPOSITORY; - -function githubHeaders() { - return { - "Authorization": `Bearer ${process.env.GITHUB_TOKEN}`, - "Accept": "application/vnd.github+json", - "Content-Type": "application/json", - }; +import { env as runtimeEnv } from "@executablemd/runtime"; + +function* githubConfiguration() { + const token = yield* runtimeEnv("GITHUB_TOKEN"); + const repo = yield* runtimeEnv("GITHUB_REPOSITORY"); + if (!token || !repo) { + return undefined; + } + const [owner, repoName] = repo.split("/"); + return { api: `https://api.github.com/repos/${owner}/${repoName}` }; } -if (!process.env.GITHUB_TOKEN || !repo) { +const github = yield* githubConfiguration(); +if (!github) { return ""; } -const [owner, repoName] = repo.split("/"); -const api = `https://api.github.com/repos/${owner}/${repoName}`; +const { api } = github; + const LABEL = "cleanup"; const TOP_N = 5; // 1. Ensure label exists -const labelResponse = yield* fetch(`${api}/labels/${LABEL}`, { headers: githubHeaders() }); +const labelResponse = yield* fetch(`${api}/labels/${LABEL}`); if (labelResponse.status === 404) { yield* fetch(`${api}/labels`, { method: "POST", - headers: githubHeaders(), + headers: { "Content-Type": "application/json" }, body: JSON.stringify({ name: LABEL, description: "Auto-generated cleanup finding from repo analysis", @@ -50,7 +53,6 @@ let page = 1; while (true) { const batch = yield* fetch( `${api}/issues?labels=${LABEL}&state=open&per_page=100&page=${page}`, - { headers: githubHeaders() }, ).expect().json(); if (!Array.isArray(batch) || batch.length === 0) break; @@ -109,14 +111,14 @@ for (const cluster of topClusters) { if (existing) { yield* fetch(api + "/issues/" + existing.number, { method: "PATCH", - headers: githubHeaders(), + headers: { "Content-Type": "application/json" }, body: JSON.stringify({ title, body }), }).expect(); updated++; } else { yield* fetch(api + "/issues", { method: "POST", - headers: githubHeaders(), + headers: { "Content-Type": "application/json" }, body: JSON.stringify({ title, body, @@ -132,7 +134,7 @@ for (const [file, issue] of issuesByFile.entries()) { if (!topFiles.has(file)) { yield* fetch(api + "/issues/" + issue.number + "/comments", { method: "POST", - headers: githubHeaders(), + headers: { "Content-Type": "application/json" }, body: JSON.stringify({ body: "Resolved โ€” file no longer in top-5 cleanup targets. Closing automatically.", }), @@ -140,7 +142,7 @@ for (const [file, issue] of issuesByFile.entries()) { yield* fetch(api + "/issues/" + issue.number, { method: "PATCH", - headers: githubHeaders(), + headers: { "Content-Type": "application/json" }, body: JSON.stringify({ state: "closed" }), }).expect(); closed++; diff --git a/.reviews/components/CommentReview.md b/.reviews/components/CommentReview.md index 944b2eaf..0619d620 100644 --- a/.reviews/components/CommentReview.md +++ b/.reviews/components/CommentReview.md @@ -8,126 +8,10 @@ props: additionalProperties: false --- -```ts eval -// --------------------------------------------------------------------------- -// 1. Build comment/code pairs with file/line metadata -const pairs = []; -const lines = props.pr.added.filter(l => !l.isTest); - -for (let i = 0; i < lines.length - 1; i++) { - const current = lines[i].content.trim(); - const next = lines[i + 1].content.trim(); - if (current.startsWith("//") && !next.startsWith("//") && next.length > 0) { - pairs.push({ - comment: current, - code: next, - file: lines[i].file, - lineNumber: lines[i].lineNumber, - }); - } -} - -const hasPairs = pairs.length >= 3; -const pairsText = hasPairs - ? pairs.map((p, i) => - `[${i}] COMMENT: ${p.comment}\nCODE: ${p.code}` - ).join("\n---\n") - : ""; - -let hasChecklist = false; -let checklistMd = ""; - -// --------------------------------------------------------------------------- -// 2. Fetch previous bot review comments and human replies -const repo = process.env.GITHUB_REPOSITORY; -const prNumber = process.env.PR_NUMBER; - -function githubHeaders() { - return { - "Authorization": `Bearer ${process.env.GITHUB_TOKEN}`, - "Accept": "application/vnd.github+json", - }; -} - -let previousFindings = []; -let dismissedReplies = []; -let repliesForClassification = []; - -if (process.env.GITHUB_TOKEN && repo && prNumber) { - const [owner, name] = repo.split("/"); - const api = `https://api.github.com/repos/${owner}/${name}`; - - const allComments = yield* fetch( - `${api}/pulls/${prNumber}/comments?per_page=100`, { headers: githubHeaders() } - ).expect().json(); - - const botComments = allComments.filter(c => - c.user.login === "github-actions[bot]" && - c.body && c.body.includes("Redundant comment") - ); - - // Build map of bot comment id โ†’ { file, line, comment } - const botCommentMap = new Map(); - for (const bc of botComments) { - // Extract the comment text from the diff hunk (last + line with //) - const hunkLines = (bc.diff_hunk ?? "").split("\n"); - const commentLine = hunkLines.filter(l => l.startsWith("+")).pop() ?? ""; - const commentText = commentLine.replace(/^\+\s*/, "").trim(); - botCommentMap.set(bc.id, { - file: bc.path, - lineNumber: bc.original_line ?? bc.line, - comment: commentText, - }); - } - - const humanReplies = allComments.filter(c => - c.in_reply_to_id && botCommentMap.has(c.in_reply_to_id) && - c.user.type !== "Bot" - ); - - // Check which replies already have a ๐Ÿ‘ reaction (already processed) - for (const reply of humanReplies) { - const location = botCommentMap.get(reply.in_reply_to_id); - const entry = { - ...location, - botCommentId: reply.in_reply_to_id, - replyText: reply.body, - replyId: reply.id, - }; - try { - const reactions = yield* fetch( - `${api}/pulls/comments/${reply.id}/reactions`, { headers: githubHeaders() } - ).expect().json(); - const alreadyAcked = reactions.some(r => - r.user.login === "github-actions[bot]" && r.content === "+1" - ); - if (alreadyAcked) { - dismissedReplies.push({ ...entry, alreadyProcessed: true }); - } else { - repliesForClassification.push(entry); - } - } catch { - repliesForClassification.push(entry); - } - } - - previousFindings = botComments.map(bc => ({ - file: bc.path, - lineNumber: bc.original_line ?? bc.line, - })); -} - -const hasRepliesToClassify = repliesForClassification.length > 0; -const repliesText = hasRepliesToClassify - ? repliesForClassification.map((r, i) => - `[${i}] FILE: ${r.file}:${r.lineNumber}\nREPLY: "${r.replyText}"` - ).join("\n---\n") - : ""; -``` - - + + @@ -140,52 +24,15 @@ ACCEPT โ€” the user agrees the comment should be removed Format: [index] DISMISS or [index] ACCEPT -{repliesText} +{reviewData.repliesText} - - -```ts eval -const classPattern = /\[(\d+)\]\s*(DISMISS|ACCEPT)/gi; -let cm; -while ((cm = classPattern.exec(classificationResult)) !== null) { - const idx = parseInt(cm[1], 10); - const intent = cm[2].toUpperCase(); - if (idx >= 0 && idx < repliesForClassification.length && intent === "DISMISS") { - dismissedReplies.push(repliesForClassification[idx]); - } -} -``` - - -```ts eval -// --------------------------------------------------------------------------- -// 3. Build dismissed set and detect applied suggestions -// --------------------------------------------------------------------------- - -const dismissedSet = new Set( - dismissedReplies.map(d => `${d.file}:${d.lineNumber}`) -); - -const addedLineSet = new Set( - props.pr.added.map(l => `${l.file}:${l.lineNumber}`) -); -const appliedFindings = previousFindings.filter(pf => - pf.lineNumber && !addedLineSet.has(`${pf.file}:${pf.lineNumber}`) && - !dismissedSet.has(`${pf.file}:${pf.lineNumber}`) -); -const appliedSet = new Set( - appliedFindings.map(af => `${af.file}:${af.lineNumber}`) -); - -const hasHistory = previousFindings.length > 0; -``` - - + + @@ -196,85 +43,29 @@ Format each finding as: REDUNDANT[index]: comment text If none are obvious: "No obvious comments found." -{pairsText} +{reviewData.pairsText} + -```ts eval -const redundantIndices = []; -const indexPattern = /REDUNDANT\[(\d+)\]/g; -let m; -while ((m = indexPattern.exec(sampleResult)) !== null) { - const idx = parseInt(m[1], 10); - if (idx >= 0 && idx < pairs.length) redundantIndices.push(idx); -} - -const allFindings = redundantIndices.map(i => pairs[i]); -const pendingFindings = allFindings.filter(f => - !dismissedSet.has(`${f.file}:${f.lineNumber}`) -); -const hasFindings = pendingFindings.length > 0; + -const checklistItems = []; + -for (const af of appliedFindings) { - checklistItems.push({ - status: "applied", - file: af.file, - lineNumber: af.lineNumber, - label: "removed", - }); -} - -for (const df of dismissedReplies) { - checklistItems.push({ - status: "dismissed", - file: df.file, - lineNumber: df.lineNumber, - comment: df.comment ?? "", - label: df.replyText, - }); -} - -for (const pf of pendingFindings) { - checklistItems.push({ - status: "pending", - file: pf.file, - lineNumber: pf.lineNumber, - comment: pf.comment, - }); -} - -// Redeclared, not reassigned: bindings from earlier eval blocks arrive in -// later blocks as consts, so assignment throws. A fresh declaration shadows -// the injected binding and its export overrides env for the below. -const hasChecklist = checklistItems.length > 0; -const checklistMd = checklistItems.map(item => { - const checked = item.status !== "pending" ? "x" : " "; - if (item.status === "applied") { - return `- [${checked}] \`${item.file}:${item.lineNumber}\` (removed)`; - } - if (item.status === "dismissed") { - return `- [${checked}] \`${item.file}:${item.lineNumber}\` โ€” \`${item.comment}\` (kept: "${item.label}")`; - } - return `- [${checked}] \`${item.file}:${item.lineNumber}\` โ€” \`${item.comment}\``; -}).join("\n"); - -const newDismissReplies = dismissedReplies.filter(d => !d.alreadyProcessed); -``` - - - - - - + - + -{checklistMd} +{state.checklistMd} diff --git a/.reviews/components/CommentReviewData.ts b/.reviews/components/CommentReviewData.ts new file mode 100644 index 00000000..8edd0da8 --- /dev/null +++ b/.reviews/components/CommentReviewData.ts @@ -0,0 +1,246 @@ +import type { Operation } from "effection"; +import { fetch } from "@effectionx/fetch"; +import type { PR } from "@executablemd/code-review-agent"; +import { env as runtimeEnv } from "@executablemd/runtime"; +import { + commentLocations, + commentPayload, + formatPairs, + formatReplies, + nonEmpty, + numberValue, + pairsFor, + records, + replyLocation, + stringValue, + type Location, + type Pair, + type Reply, + type ReviewData, + userLogin, + userType, +} from "../../packages/code-review-agent/src/comment-review-data.ts"; + +export const props = { + type: "object", + properties: { pr: { type: "object" } }, + required: ["pr"], + additionalProperties: false, +}; + +const locationProperties = { + file: { type: "string" }, + lineNumber: { type: "number" }, +}; + +const replyProperties = { + ...locationProperties, + comment: { type: "string" }, + botCommentId: { type: "number" }, + replyText: { type: "string" }, + replyId: { type: "number" }, + alreadyProcessed: { type: "boolean" }, +}; + +export const returns = { + type: "object", + properties: { + pairs: { + type: "array", + items: { + type: "object", + properties: { + comment: { type: "string" }, + code: { type: "string" }, + ...locationProperties, + }, + required: ["comment", "code", "file", "lineNumber"], + additionalProperties: false, + }, + }, + hasPairs: { type: "boolean" }, + pairsText: { type: "string" }, + previousFindings: { + type: "array", + items: { + type: "object", + properties: locationProperties, + required: ["file", "lineNumber"], + additionalProperties: false, + }, + }, + dismissedReplies: { + type: "array", + items: { + type: "object", + properties: replyProperties, + required: ["file", "lineNumber", "replyText"], + additionalProperties: false, + }, + }, + repliesForClassification: { + type: "array", + items: { + type: "object", + properties: replyProperties, + required: ["file", "lineNumber", "replyText"], + additionalProperties: false, + }, + }, + hasRepliesToClassify: { type: "boolean" }, + repliesText: { type: "string" }, + }, + required: [ + "pairs", + "hasPairs", + "pairsText", + "previousFindings", + "dismissedReplies", + "repliesForClassification", + "hasRepliesToClassify", + "repliesText", + ], + additionalProperties: false, +}; + +interface CommentReviewProps { + pr: PR; +} + +function* githubApi(): Operation { + const token = yield* runtimeEnv("GITHUB_TOKEN"); + const repository = yield* runtimeEnv("GITHUB_REPOSITORY"); + const number = yield* runtimeEnv("PR_NUMBER"); + if (!nonEmpty(token) || !nonEmpty(repository) || !nonEmpty(number)) { + return undefined; + } + const [owner, name] = repository.split("/"); + if (nonEmpty(owner) && nonEmpty(name)) { + return `https://api.github.com/repos/${owner}/${name}`; + } + return undefined; +} + +function* fetchReplyReactions(api: string, reply: Reply): Operation { + try { + const reactions = records( + yield* fetch(`${api}/pulls/comments/${reply.replyId}/reactions`).expect().json(), + ); + return reactions.some( + (reaction) => userLogin(reaction) === "github-actions[bot]" && reaction.content === "+1", + ); + } catch { + return false; + } +} + +function* collectReplies( + api: string, + comments: Record[], + botCommentMap: Map, +): Operation<{ dismissed: Reply[]; pending: Reply[] }> { + const dismissed: Reply[] = []; + const pending: Reply[] = []; + for (const reply of comments) { + const entry = yield* replyForClassification(api, reply, botCommentMap); + if (entry !== undefined) { + if (entry.alreadyProcessed === true) { + dismissed.push(entry); + } else { + pending.push(entry); + } + } + } + return { dismissed, pending }; +} + +function* replyForClassification( + api: string, + reply: Record, + botCommentMap: Map, +): Operation { + const parentId = numberValue(reply, "in_reply_to_id"); + const location = replyLocation(reply, botCommentMap); + const replyText = stringValue(reply, "body"); + const replyId = numberValue(reply, "id"); + if ( + location === undefined || + userType(reply) === "Bot" || + replyText === undefined || + replyId === undefined + ) { + return undefined; + } + + const entry: Reply = { ...location, botCommentId: parentId, replyText, replyId }; + if (yield* fetchReplyReactions(api, entry)) { + return { ...entry, alreadyProcessed: true }; + } + return entry; +} + +interface ReviewDataParts { + pairs: Pair[]; + previousFindings: { file: string; lineNumber: number }[]; + dismissedReplies: Reply[]; + repliesForClassification: Reply[]; +} + +function reviewData({ + pairs, + previousFindings, + dismissedReplies, + repliesForClassification, +}: ReviewDataParts): ReviewData { + const hasPairs = pairs.length >= 3; + return { + pairs, + hasPairs, + pairsText: formatPairs(pairs, hasPairs), + previousFindings, + dismissedReplies, + repliesForClassification, + hasRepliesToClassify: repliesForClassification.length > 0, + repliesText: formatReplies(repliesForClassification), + }; +} + +function emptyReviewData(pairs: Pair[]): ReviewData { + return reviewData({ + pairs, + previousFindings: [], + dismissedReplies: [], + repliesForClassification: [], + }); +} + +function* fetchedReviewData(api: string, number: string, pairs: Pair[]): Operation { + const comments = commentPayload( + yield* fetch(`${api}/pulls/${number}/comments?per_page=100`).expect().json(), + ); + const locations = commentLocations(comments); + const replies = yield* collectReplies(api, comments, locations.botCommentMap); + return reviewData({ + pairs, + previousFindings: locations.previousFindings, + dismissedReplies: replies.dismissed, + repliesForClassification: replies.pending, + }); +} + +function* reviewDataFor(pr: PR): Operation { + const pairs = pairsFor(pr); + const api = yield* githubApi(); + if (api === undefined) { + return emptyReviewData(pairs); + } + const number = yield* runtimeEnv("PR_NUMBER"); + if (!nonEmpty(number)) { + return emptyReviewData(pairs); + } + return yield* fetchedReviewData(api, number, pairs); +} + +export default function* CommentReviewData({ pr }: CommentReviewProps): Operation { + return yield* reviewDataFor(pr); +} diff --git a/.reviews/components/CommentReviewState.ts b/.reviews/components/CommentReviewState.ts new file mode 100644 index 00000000..6d752fa6 --- /dev/null +++ b/.reviews/components/CommentReviewState.ts @@ -0,0 +1,251 @@ +import type { Operation } from "effection"; +import type { PR } from "@executablemd/code-review-agent"; + +const locationProperties = { + file: { type: "string" }, + lineNumber: { type: "number" }, +}; + +const findingProperties = { + ...locationProperties, + comment: { type: "string" }, + code: { type: "string" }, +}; + +const replyProperties = { + ...locationProperties, + comment: { type: "string" }, + botCommentId: { type: "number" }, + replyText: { type: "string" }, + replyId: { type: "number" }, + alreadyProcessed: { type: "boolean" }, +}; + +export const props = { + type: "object", + properties: { + pr: { type: "object" }, + data: { type: "object" }, + classificationResult: { type: "string", default: "" }, + sampleResult: { type: "string", default: "" }, + }, + required: ["pr", "data", "classificationResult", "sampleResult"], + additionalProperties: false, +}; + +export const returns = { + type: "object", + properties: { + hasChecklist: { type: "boolean" }, + checklistMd: { type: "string" }, + hasFindings: { type: "boolean" }, + pendingFindings: { + type: "array", + items: { + type: "object", + properties: findingProperties, + required: ["comment", "code", "file", "lineNumber"], + additionalProperties: false, + }, + }, + newDismissReplies: { + type: "array", + items: { + type: "object", + properties: replyProperties, + required: ["file", "lineNumber", "replyText"], + additionalProperties: false, + }, + }, + }, + required: ["hasChecklist", "checklistMd", "hasFindings", "pendingFindings", "newDismissReplies"], + additionalProperties: false, +}; + +interface Pair { + comment: string; + code: string; + file: string; + lineNumber: number; +} + +interface Reply { + file: string; + lineNumber: number; + comment?: string; + botCommentId?: number; + replyText: string; + replyId?: number; + alreadyProcessed?: boolean; +} + +interface ReviewData { + pairs: Pair[]; + previousFindings: { file: string; lineNumber: number }[]; + dismissedReplies: Reply[]; + repliesForClassification: Reply[]; +} + +interface CommentReviewStateProps { + pr: PR; + data: ReviewData; + classificationResult: string; + sampleResult: string; +} + +interface CommentReviewStateValue { + hasChecklist: boolean; + checklistMd: string; + hasFindings: boolean; + pendingFindings: Pair[]; + newDismissReplies: Reply[]; +} + +type ChecklistItem = + | { status: "applied"; file: string; lineNumber: number; label: string } + | { status: "dismissed"; file: string; lineNumber: number; comment: string; label: string } + | { status: "pending"; file: string; lineNumber: number; comment: string }; + +const CLASSIFICATION_PATTERN = /\[(?\d+)\]\s*(?DISMISS|ACCEPT)/giu; +const REDUNDANT_PATTERN = /REDUNDANT\[(?\d+)\]/gu; + +function key(file: string, lineNumber: number): string { + return `${file}:${lineNumber}`; +} + +function matchedIndices(input: string, pattern: RegExp): number[] { + const indices: number[] = []; + for (const match of input.matchAll(pattern)) { + const index = Number(match.groups?.index); + if (Number.isInteger(index)) { + indices.push(index); + } + } + return indices; +} + +function dismissedIndices(input: string): number[] { + const indices: number[] = []; + for (const match of input.matchAll(CLASSIFICATION_PATTERN)) { + if (match.groups?.action?.toUpperCase() === "DISMISS") { + const index = Number(match.groups?.index); + if (Number.isInteger(index)) { + indices.push(index); + } + } + } + return indices; +} + +function dismissedRepliesFor(data: ReviewData, classificationResult: string): Reply[] { + const dismissedReplies = [...data.dismissedReplies]; + for (const index of dismissedIndices(classificationResult)) { + const reply = data.repliesForClassification[index]; + if (reply !== undefined) { + dismissedReplies.push(reply); + } + } + return dismissedReplies; +} + +function dismissedSetFor(replies: Reply[]): Set { + return new Set(replies.map((reply) => key(reply.file, reply.lineNumber))); +} + +function appliedFindingsFor( + data: ReviewData, + pr: PR, + dismissedSet: Set, +): { file: string; lineNumber: number }[] { + const addedLineSet = new Set(pr.added.map((line) => key(line.file, line.lineNumber))); + return data.previousFindings.filter((finding) => { + const findingKey = key(finding.file, finding.lineNumber); + return !addedLineSet.has(findingKey) && !dismissedSet.has(findingKey); + }); +} + +function pendingFindingsFor( + data: ReviewData, + sampleResult: string, + dismissedSet: Set, +): Pair[] { + return matchedIndices(sampleResult, REDUNDANT_PATTERN) + .map((index) => data.pairs[index]) + .filter( + (finding): finding is Pair => + finding !== undefined && !dismissedSet.has(key(finding.file, finding.lineNumber)), + ); +} + +function checklistItemsFor( + appliedFindings: { file: string; lineNumber: number }[], + dismissedReplies: Reply[], + pendingFindings: Pair[], +): ChecklistItem[] { + return [ + ...appliedFindings.map( + (finding): ChecklistItem => ({ + status: "applied", + file: finding.file, + lineNumber: finding.lineNumber, + label: "removed", + }), + ), + ...dismissedReplies.map( + (reply): ChecklistItem => ({ + status: "dismissed", + file: reply.file, + lineNumber: reply.lineNumber, + comment: reply.comment ?? "", + label: reply.replyText, + }), + ), + ...pendingFindings.map( + (finding): ChecklistItem => ({ + status: "pending", + file: finding.file, + lineNumber: finding.lineNumber, + comment: finding.comment, + }), + ), + ]; +} + +function checklistItemText(item: ChecklistItem): string { + let checked = "x"; + if (item.status === "pending") { + checked = " "; + } + if (item.status === "applied") { + return `- [${checked}] \`${item.file}:${item.lineNumber}\` (removed)`; + } + if (item.status === "dismissed") { + return `- [${checked}] \`${item.file}:${item.lineNumber}\` โ€” \`${item.comment}\` (kept: "${item.label}")`; + } + return `- [${checked}] \`${item.file}:${item.lineNumber}\` โ€” \`${item.comment}\``; +} + +function checklistMarkdown(items: ChecklistItem[]): string { + return items.map((item) => checklistItemText(item)).join("\n"); +} + +export default function* CommentReviewState({ + pr, + data, + classificationResult, + sampleResult, +}: CommentReviewStateProps): Operation { + const dismissedReplies = dismissedRepliesFor(data, classificationResult); + const dismissedSet = dismissedSetFor(dismissedReplies); + const appliedFindings = appliedFindingsFor(data, pr, dismissedSet); + const pendingFindings = pendingFindingsFor(data, sampleResult, dismissedSet); + const checklistItems = checklistItemsFor(appliedFindings, dismissedReplies, pendingFindings); + + return { + hasChecklist: checklistItems.length > 0, + checklistMd: checklistMarkdown(checklistItems), + hasFindings: pendingFindings.length > 0, + pendingFindings, + newDismissReplies: dismissedReplies.filter((reply) => reply.alreadyProcessed !== true), + }; +} diff --git a/.reviews/components/DeepInfraProvider.md b/.reviews/components/DeepInfraProvider.md index b0b474a7..c7433238 100644 --- a/.reviews/components/DeepInfraProvider.md +++ b/.reviews/components/DeepInfraProvider.md @@ -9,6 +9,8 @@ props: --- ```ts persist eval +import { env as runtimeEnv } from "@executablemd/runtime"; + yield* Sample.around({ *sample([context], next) { if (context.model !== undefined && context.model !== props.model) { @@ -20,19 +22,27 @@ yield* Sample.around({ messages.push({ role: "system", content: context.system }); } messages.push({ role: "user", content: context.content }); + const token = yield* runtimeEnv("DEEPINFRA_TOKEN"); + if (!token) { + throw new Error("DeepInfraProvider requires DEEPINFRA_TOKEN"); + } const result = yield* fetch("https://api.deepinfra.com/v1/openai/chat/completions", { method: "POST", headers: { "Content-Type": "application/json", - "Authorization": `Bearer ${process.env.DEEPINFRA_TOKEN}`, + "Authorization": `Bearer ${token}`, }, body: JSON.stringify({ model: props.model, messages, temperature: 0, max_tokens: 4096 }), }) .expect() .json(); - return result.choices[0].message.content; + const content = result.choices?.[0]?.message?.content; + if (typeof content !== "string") { + throw new Error("DeepInfra response did not contain model content"); + } + return content; }, }, { at: 'min' }); ``` diff --git a/.reviews/components/Doctor.md b/.reviews/components/Doctor.md deleted file mode 100644 index b9fb0605..00000000 --- a/.reviews/components/Doctor.md +++ /dev/null @@ -1,239 +0,0 @@ ---- -props: - type: object - properties: - pr: - type: object - tsconfigPath: - type: string - default: ".reviews/tsconfig.oxlint.json" - required: [pr] - additionalProperties: false ---- - -Checking environment for Oxlint static analysis... - - - - - -```bash exec -.reviews/.oxlint/oxlint --version 2>/dev/null || echo "NOT_INSTALLED" -``` - - - - - -```bash exec -test -x .reviews/.oxlint/tsgolint && echo "INSTALLED" || echo "NOT_INSTALLED" -``` - - - - - -```bash exec -test -x .reviews/.oxlint/oxlint && echo "EXISTS" || echo "MISSING" -``` - - - - - -```bash exec -test -f {props.tsconfigPath} && echo "EXISTS" || echo "MISSING" -``` - - - -```ts eval -const oxlintInstalled = !oxlintVersion.includes("NOT_INSTALLED"); -const tsgolintInstalled = !tsgolintVersion.includes("NOT_INSTALLED"); -const nodeModulesExists = nodeModulesCheck.trim() === "EXISTS"; -const tsconfigExists = tsconfigCheck.trim() === "EXISTS"; - -const canProbeTypeAware = oxlintInstalled && tsgolintInstalled - && nodeModulesExists && tsconfigExists; -``` - -Scanning source files for scheme specifiers (jsr:, npm:)... - - - -```bash exec -grep -rn --include='*.ts' --include='*.tsx' -E '^\s*(import|export)\s.*from\s+['"'"'"](jsr:|npm:)' packages/ src/ 2>/dev/null | head -50 || echo "NONE" -``` - - - -```ts eval -const hasNativeSpecifiers = specifierScan.trim() !== "NONE" - && specifierScan.trim().length > 0; - -const specifierLines = hasNativeSpecifiers - ? specifierScan.trim().split("\n") : []; - -const specifierFiles = [...new Set( - specifierLines.map(l => l.split(":")[0]).filter(Boolean) -)]; - -const jsrCount = specifierLines.filter(l => l.includes("jsr:")).length; -const npmCount = specifierLines.filter(l => l.includes("npm:")).length; -``` - -Running type-aware probe to test Oxlint compatibility... - - - - - -```bash exec -RESULT=$(OXLINT_TSGOLINT_PATH=.reviews/.oxlint/tsgolint .reviews/.oxlint/oxlint --config .reviews/.oxlintrc.json --type-aware --tsconfig {props.tsconfigPath} --format json 2>.reviews/probe-stderr.tmp || true) -STDERR=$(cat .reviews/probe-stderr.tmp 2>/dev/null || echo "") -rm -f .reviews/probe-stderr.tmp -if [ -n "$RESULT" ] && printf '%s' "$RESULT" | jq -c --arg stderr "$STDERR" ' - def entries: - if type == "array" then . - elif (.diagnostics? | type) == "array" then .diagnostics - else [] - end; - def file: - if (.file? | type) == "string" then .file - elif (.filename? | type) == "string" then .filename - else "" - end; - def rule: - if (.ruleId? | type) == "string" then .ruleId - elif (.code? | type) == "string" then .code - else "unknown" - end; - def message: - if (.message? | type) == "string" then .message else "" end; - def import_noise: - ((.message | ascii_downcase | contains("cannot find module")) - or (.ruleId | ascii_downcase | contains("import"))); - def crashed($value): - ($value | ascii_downcase) as $lower - | ($lower | contains("tsgolint")) - and (($lower | contains("panic")) - or ($lower | contains("oom")) - or ($lower | contains("fatal"))); - entries - | map({file: file, ruleId: rule, message: message}) as $diagnostics - | { - diagnosticCount: ($diagnostics | length), - importNoiseCount: ([$diagnostics[] | select(import_noise)] | length), - filesAnalyzed: ([$diagnostics[].file | select(length > 0)] | unique | length), - filesSkipped: ([$diagnostics[] | select(import_noise) | .file | select(length > 0)] | unique | length), - importErrors: ([$diagnostics[] | select(import_noise)] | length), - availableRuleIds: ([$diagnostics[].ruleId | select(length > 0)] | unique), - tsgolintCrashed: crashed($stderr) - } -'; then - : -else - jq -cn --arg stderr "$STDERR" ' - def crashed($value): - ($value | ascii_downcase) as $lower - | ($lower | contains("tsgolint")) - and (($lower | contains("panic")) - or ($lower | contains("oom")) - or ($lower | contains("fatal"))); - { - diagnosticCount: 0, - importNoiseCount: 0, - filesAnalyzed: 0, - filesSkipped: 0, - importErrors: 0, - availableRuleIds: [], - tsgolintCrashed: crashed($stderr) - } - ' -fi -``` - - - - - -```ts eval -const BLOAT_RULES = [ - "no-unused-vars", "no-inferrable-types", "no-empty-function", - "no-empty-object-type", "no-useless-empty-export", - "no-unnecessary-type-constraint", - "no-unnecessary-parameter-property-assignment", - "no-static-only-class", "no-console", "no-debugger", - "no-unnecessary-type-assertion", "no-redundant-type-constituents", - "no-unnecessary-type-arguments", - "no-unnecessary-boolean-literal-compare", -]; -const TYPE_AWARE_RULES = [ - "no-unnecessary-type-assertion", "no-redundant-type-constituents", - "no-unnecessary-type-arguments", - "no-unnecessary-boolean-literal-compare", -]; - -let probe = { - diagnosticCount: 0, - importNoiseCount: 0, - filesAnalyzed: 0, - filesSkipped: 0, - importErrors: 0, - availableRuleIds: [], - tsgolintCrashed: false, -}; -try { probe = { ...probe, ...JSON.parse(probeResult) }; } catch { } - -const diagnosticCount = typeof probe.diagnosticCount === "number" - ? probe.diagnosticCount : 0; -const importNoiseCount = typeof probe.importNoiseCount === "number" - ? probe.importNoiseCount : 0; -const noiseRatio = diagnosticCount > 0 - ? importNoiseCount / diagnosticCount : 0; - -const tsgolintCrashed = probe.tsgolintCrashed === true; - -const typeAwareAvailable = canProbeTypeAware && !tsgolintCrashed; - -let recommendation = "syntax-only"; -if (typeAwareAvailable && noiseRatio < 0.3) { - recommendation = "type-aware"; -} else if (typeAwareAvailable && noiseRatio >= 0.3) { - recommendation = "type-aware-filtered"; -} - -const bloatRulesAvailable = typeAwareAvailable - ? BLOAT_RULES - : BLOAT_RULES.filter(r => !TYPE_AWARE_RULES.includes(r)); -const bloatRulesMissing = typeAwareAvailable - ? [] - : TYPE_AWARE_RULES; - -const doctor = { - oxlintInstalled, - oxlintVersion: oxlintVersion.trim(), - tsgolintInstalled, - tsgolintVersion: tsgolintVersion.trim(), - tsconfigExists, - nodeModulesExists, - typeAwareAvailable, - filesAnalyzed: typeof probe.filesAnalyzed === "number" ? probe.filesAnalyzed : 0, - filesSkipped: typeof probe.filesSkipped === "number" ? probe.filesSkipped : 0, - importErrors: typeof probe.importErrors === "number" ? probe.importErrors : 0, - availableRuleIds: Array.isArray(probe.availableRuleIds) - ? probe.availableRuleIds : [], - bloatRulesAvailable, - bloatRulesMissing, - recommendation, - nativeSpecifiers: { - count: hasNativeSpecifiers ? specifierLines.length : 0, - files: specifierFiles, - jsr: jsrCount, - npm: npmCount, - }, -}; - -return '```json\n' + JSON.stringify(doctor) + '\n```'; -``` diff --git a/.reviews/components/Doctor.ts b/.reviews/components/Doctor.ts new file mode 100644 index 00000000..7b29a216 --- /dev/null +++ b/.reviews/components/Doctor.ts @@ -0,0 +1,249 @@ +import { + type DoctorEnvironment, + type DoctorResult, + type OxlintDiagnostic, + buildDoctorResult, + isOxlintCrash, + normalizeOxlintOutput, + summarizeDoctorProbe, +} from "@executablemd/code-review-agent"; +import { exec, glob, readTextFile, stat } from "@executablemd/runtime"; +import type { Operation } from "effection"; + +const OXLINT_PATH = ".reviews/.oxlint/oxlint"; +const TSGOLINT_PATH = ".reviews/.oxlint/tsgolint"; +const OXLINT_CONFIG = ".reviews/.oxlintrc.json"; +const NATIVE_SPECIFIER_PATTERN = /^\s*(?:import|export)\s.*?from\s+["'](?jsr:|npm:)/u; + +export const props = { + type: "object", + properties: { + pr: { type: "object" }, + tsconfigPath: { + type: "string", + default: ".reviews/tsconfig.oxlint.json", + }, + }, + required: ["pr"], + additionalProperties: false, +}; + +export const returns = { + type: "object", + properties: { + oxlintInstalled: { type: "boolean" }, + oxlintVersion: { type: "string" }, + tsgolintInstalled: { type: "boolean" }, + tsgolintVersion: { type: "string" }, + tsconfigExists: { type: "boolean" }, + nodeModulesExists: { type: "boolean" }, + typeAwareAvailable: { type: "boolean" }, + filesAnalyzed: { type: "number" }, + filesSkipped: { type: "number" }, + importErrors: { type: "number" }, + availableRuleIds: { type: "array", items: { type: "string" } }, + bloatRulesAvailable: { type: "array", items: { type: "string" } }, + bloatRulesMissing: { type: "array", items: { type: "string" } }, + recommendation: { type: "string" }, + nativeSpecifiers: { + type: "object", + properties: { + count: { type: "number" }, + files: { type: "array", items: { type: "string" } }, + jsr: { type: "number" }, + npm: { type: "number" }, + }, + required: ["count", "files", "jsr", "npm"], + additionalProperties: false, + }, + }, + required: [ + "oxlintInstalled", + "oxlintVersion", + "tsgolintInstalled", + "tsgolintVersion", + "tsconfigExists", + "nodeModulesExists", + "typeAwareAvailable", + "filesAnalyzed", + "filesSkipped", + "importErrors", + "availableRuleIds", + "bloatRulesAvailable", + "bloatRulesMissing", + "recommendation", + "nativeSpecifiers", + ], + additionalProperties: false, +}; + +interface DoctorProps { + pr: object; + tsconfigPath?: string; +} + +function* version(command: string[]): Operation { + const result = yield* exec({ command }); + if (result.exitCode === 0) { + return result.stdout.trim(); + } + return ""; +} + +interface NativeSpecifierCounts { + jsr: number; + npm: number; +} + +function nativeSpecifierCounts(source: string): NativeSpecifierCounts { + const counts = { jsr: 0, npm: 0 }; + for (const line of source.split(/\r?\n/u)) { + const scheme = NATIVE_SPECIFIER_PATTERN.exec(line)?.groups?.scheme; + if (scheme === "jsr:") { + counts.jsr++; + } + if (scheme === "npm:") { + counts.npm++; + } + } + return counts; +} + +interface NativeSpecifierFile { + path: string; + counts: NativeSpecifierCounts; +} + +function* nativeSpecifierFile(entry: { + isFile: boolean; + path: string; +}): Operation { + if (!entry.isFile) { + return undefined; + } + return { path: entry.path, counts: nativeSpecifierCounts(yield* readTextFile(entry.path)) }; +} + +function nativeSpecifierValue(files: NativeSpecifierFile[]): DoctorResult["nativeSpecifiers"] { + let jsr = 0; + let npm = 0; + const paths: string[] = []; + for (const file of files) { + jsr += file.counts.jsr; + npm += file.counts.npm; + if (file.counts.jsr + file.counts.npm > 0) { + paths.push(file.path); + } + } + return { count: jsr + npm, files: [...new Set(paths)], jsr, npm }; +} + +function* nativeSpecifierFiles( + entries: { isFile: boolean; path: string }[], +): Operation { + const files: NativeSpecifierFile[] = []; + for (const entry of entries) { + const file = yield* nativeSpecifierFile(entry); + if (file !== undefined) { + files.push(file); + } + } + return files; +} + +function* nativeSpecifierSummary(): Operation { + const entries = yield* glob({ + root: ".", + patterns: ["packages/**/*.ts", "durable-effects/**/*.ts"], + exclude: ["**/*.test.ts", "**/*.spec.ts", "**/node_modules/**"], + }); + return nativeSpecifierValue(yield* nativeSpecifierFiles(entries)); +} + +function probeCommand(tsconfigPath: string): string[] { + return [ + OXLINT_PATH, + "--config", + OXLINT_CONFIG, + "--type-aware", + "--tsconfig", + tsconfigPath, + "--format", + "json", + ]; +} + +function probeFailure(exitCode: number, stdout: string, stderr: string): Error | undefined { + if (exitCode > 1 && !isOxlintCrash(stderr)) { + return new Error(stderr || `Oxlint probe failed with exit code ${exitCode}`); + } + if (stdout.trim().length === 0 && exitCode !== 0 && !isOxlintCrash(stderr)) { + return new Error(stderr || "Oxlint probe returned no JSON output"); + } + return undefined; +} + +function probeDiagnostics(stdout: string): OxlintDiagnostic[] { + if (stdout.trim().length === 0) { + return []; + } + return normalizeOxlintOutput(stdout); +} + +function* probeTypeAware(canProbe: boolean, tsconfigPath: string) { + if (!canProbe) { + return summarizeDoctorProbe({ diagnostics: [], stderr: "", exitCode: 2 }); + } + + const result = yield* exec({ + command: probeCommand(tsconfigPath), + env: { OXLINT_TSGOLINT_PATH: TSGOLINT_PATH }, + }); + const failure = probeFailure(result.exitCode, result.stdout, result.stderr); + if (failure !== undefined) { + throw failure; + } + const diagnostics = probeDiagnostics(result.stdout); + return summarizeDoctorProbe({ + diagnostics, + stderr: result.stderr, + exitCode: result.exitCode, + }); +} + +function* toolVersion(path: string): Operation { + const result = yield* stat(path); + if (!result.isFile) { + return ""; + } + return yield* version([path, "--version"]); +} + +function* doctorEnvironment(tsconfigPath: string): Operation { + const oxlint = yield* stat(OXLINT_PATH); + const tsgolint = yield* stat(TSGOLINT_PATH); + const tsconfig = yield* stat(tsconfigPath); + const nodeModules = yield* stat("node_modules"); + return { + oxlintInstalled: oxlint.isFile, + oxlintVersion: yield* toolVersion(OXLINT_PATH), + tsgolintInstalled: tsgolint.isFile, + tsgolintVersion: yield* toolVersion(TSGOLINT_PATH), + tsconfigExists: tsconfig.isFile, + nodeModulesExists: nodeModules.isDirectory, + nativeSpecifiers: yield* nativeSpecifierSummary(), + }; +} + +export default function* Doctor({ + tsconfigPath = ".reviews/tsconfig.oxlint.json", +}: DoctorProps): Operation { + const environment = yield* doctorEnvironment(tsconfigPath); + const canProbe = + environment.oxlintInstalled && + environment.tsgolintInstalled && + environment.tsconfigExists && + environment.nodeModulesExists; + const probe = yield* probeTypeAware(canProbe, tsconfigPath); + return buildDoctorResult(environment, probe); +} diff --git a/.reviews/components/EnsureOxlint.md b/.reviews/components/EnsureOxlint.md index 09b67771..a1ae93ee 100644 --- a/.reviews/components/EnsureOxlint.md +++ b/.reviews/components/EnsureOxlint.md @@ -8,70 +8,84 @@ props: additionalProperties: false --- +```ts eval +import { platform } from "@executablemd/runtime"; + +const OXLINT_TAG = "apps_v1.74.0"; +const TSGOLINT_VERSION = "0.25.0"; +const host = yield* platform(); +const target = { + oxlintOs: host.os === "darwin" ? "apple-darwin" : host.os === "linux" ? "unknown-linux-gnu" : "", + tsgolintOs: host.os === "darwin" ? "darwin" : host.os === "linux" ? "linux" : "", + oxlintArch: host.arch === "arm64" || host.arch === "aarch64" + ? "aarch64" + : host.arch === "x86_64" || host.arch === "amd64" || host.arch === "x64" + ? "x86_64" + : "", + tsgolintArch: host.arch === "arm64" || host.arch === "aarch64" + ? "arm64" + : host.arch === "x86_64" || host.arch === "amd64" || host.arch === "x64" + ? "x64" + : "", +}; +if (!target.oxlintOs || !target.oxlintArch) { + throw new Error(`EnsureOxlint: unsupported platform ${host.os}/${host.arch}`); +} + +const oxlintHashes = { + "aarch64-apple-darwin": "768a2d00e7e0a95cbf89837086f475d25dc1a1ba605b8831fb5a1db6d590a643", + "x86_64-apple-darwin": "04ae38d56ae4990ac96320c03f05f38cf1103b5fe64b08cd7203b87e767b45b4", + "aarch64-unknown-linux-gnu": "ced0d2433bda2b4295e1ab93b40c3f24224713c32a44e13abcda656590dba1cb", + "x86_64-unknown-linux-gnu": "fd3ed5d2dc55ab6f7a243583c69dd5da4ac97cd1f6e10321225ca6343c9451a9", +}; +const tsgolintHashes = { + "darwin-arm64": "3ad51d1b88070b491b81a4f5c6169148914127b9da65f8087823b25568431e1e", + "darwin-x64": "6b78caa20db383c055cead96724aeafab3cd277b00b1f95c6f56b4bfcf22fd60", + "linux-arm64": "20bcbab4bb37dd396102566740ee39e67d1e3d16d06096802180426c022bc414", + "linux-x64": "f6ea083842395d7439eadbbbf380f23793a1fa890fbda92013ee0f6033e75630", +}; +const oxlintKey = `${target.oxlintArch}-${target.oxlintOs}`; +const tsgolintKey = `${target.tsgolintOs}-${target.tsgolintArch}`; +const oxlintUrl = `https://github.com/oxc-project/oxc/releases/download/${OXLINT_TAG}/oxlint-${target.oxlintArch}-${target.oxlintOs}.tar.gz`; +const tsgolintUrl = `https://registry.npmjs.org/@oxlint-tsgolint/${target.tsgolintOs}-${target.tsgolintArch}/-/${target.tsgolintOs}-${target.tsgolintArch}-${TSGOLINT_VERSION}.tgz`; +const oxlintSha = oxlintHashes[oxlintKey]; +const tsgolintSha = tsgolintHashes[tsgolintKey]; +if (!oxlintSha || !tsgolintSha) { + throw new Error(`EnsureOxlint: no checksum for ${oxlintKey}/${tsgolintKey}`); +} +``` + ```bash silent exec -# Provisions the pinned oxlint + tsgolint binaries with mandatory sha256 -# verification, failing closed โ€” a failed or tampered download must abort the -# review, never silently degrade it. Version pins live next to their hashes so -# neither can change without the other. The workflow guards on the resulting -# binaries (see review.yml). set -euo pipefail DIR="{props.dir}" -OXLINT_TAG="apps_v1.74.0" -TSGOLINT_VERSION="0.25.0" mkdir -p "$DIR" -os="$(uname -s)" -arch="$(uname -m)" -case "$os" in - Darwin) ox_os="apple-darwin"; tg_os="darwin" ;; - Linux) ox_os="unknown-linux-gnu"; tg_os="linux" ;; - *) echo "EnsureOxlint: unsupported OS $os" >&2; exit 1 ;; -esac -case "$arch" in - arm64 | aarch64) ox_arch="aarch64"; tg_arch="arm64" ;; - x86_64 | amd64) ox_arch="x86_64"; tg_arch="x64" ;; - *) echo "EnsureOxlint: unsupported arch $arch" >&2; exit 1 ;; -esac - -case "${ox_arch}-${ox_os}" in - aarch64-apple-darwin) ox_sha="768a2d00e7e0a95cbf89837086f475d25dc1a1ba605b8831fb5a1db6d590a643" ;; - x86_64-apple-darwin) ox_sha="04ae38d56ae4990ac96320c03f05f38cf1103b5fe64b08cd7203b87e767b45b4" ;; - aarch64-unknown-linux-gnu) ox_sha="ced0d2433bda2b4295e1ab93b40c3f24224713c32a44e13abcda656590dba1cb" ;; - x86_64-unknown-linux-gnu) ox_sha="fd3ed5d2dc55ab6f7a243583c69dd5da4ac97cd1f6e10321225ca6343c9451a9" ;; -esac -case "${tg_os}-${tg_arch}" in - darwin-arm64) tg_sha="3ad51d1b88070b491b81a4f5c6169148914127b9da65f8087823b25568431e1e" ;; - darwin-x64) tg_sha="6b78caa20db383c055cead96724aeafab3cd277b00b1f95c6f56b4bfcf22fd60" ;; - linux-arm64) tg_sha="20bcbab4bb37dd396102566740ee39e67d1e3d16d06096802180426c022bc414" ;; - linux-x64) tg_sha="f6ea083842395d7439eadbbbf380f23793a1fa890fbda92013ee0f6033e75630" ;; -esac - -if command -v sha256sum >/dev/null 2>&1; then sha_cmd="sha256sum"; else sha_cmd="shasum -a 256"; fi - verify() { - actual="$($sha_cmd "$1" | awk '{print $1}')" - if [ "$actual" != "$2" ]; then - echo "EnsureOxlint: checksum mismatch for $1 (expected $2, got $actual)" >&2 + expected="$2" + actual="$(sha256sum "$1" 2>/dev/null || shasum -a 256 "$1")" + actual="${actual%% *}" + if [ "$actual" != "$expected" ]; then + echo "EnsureOxlint: checksum mismatch for $1" >&2 exit 1 fi } -# oxlint: standalone binary from the oxc release (no Node/npm needed). if [ ! -x "$DIR/oxlint" ]; then tmp="$(mktemp -d)" - curl -fsSL -o "$tmp/oxlint.tar.gz" "https://github.com/oxc-project/oxc/releases/download/${OXLINT_TAG}/oxlint-${ox_arch}-${ox_os}.tar.gz" - verify "$tmp/oxlint.tar.gz" "$ox_sha" + trap 'rm -rf "$tmp"' EXIT + curl -fsSL -o "$tmp/oxlint.tar.gz" "{oxlintUrl}" + verify "$tmp/oxlint.tar.gz" "{oxlintSha}" tar xz -C "$tmp" -f "$tmp/oxlint.tar.gz" - mv "$tmp/oxlint-${ox_arch}-${ox_os}" "$DIR/oxlint" + mv "$tmp/oxlint-{target.oxlintArch}-{target.oxlintOs}" "$DIR/oxlint" chmod +x "$DIR/oxlint" rm -rf "$tmp" fi -# tsgolint (type-aware engine): npm-only, so pull the platform tarball directly. if [ ! -x "$DIR/tsgolint" ]; then tmp="$(mktemp -d)" - curl -fsSL -o "$tmp/tsgolint.tgz" "https://registry.npmjs.org/@oxlint-tsgolint/${tg_os}-${tg_arch}/-/${tg_os}-${tg_arch}-${TSGOLINT_VERSION}.tgz" - verify "$tmp/tsgolint.tgz" "$tg_sha" + trap 'rm -rf "$tmp"' EXIT + curl -fsSL -o "$tmp/tsgolint.tgz" "{tsgolintUrl}" + verify "$tmp/tsgolint.tgz" "{tsgolintSha}" tar xz -C "$tmp" -f "$tmp/tsgolint.tgz" mv "$tmp/package/tsgolint" "$DIR/tsgolint" chmod +x "$DIR/tsgolint" diff --git a/.reviews/components/GitHubAuth.md b/.reviews/components/GitHubAuth.md new file mode 100644 index 00000000..5affdd12 --- /dev/null +++ b/.reviews/components/GitHubAuth.md @@ -0,0 +1,46 @@ +--- +props: + type: object + properties: {} + additionalProperties: false +--- + +```ts persist eval +import { FetchApi } from "@effectionx/fetch"; +import { env as runtimeEnv } from "@executablemd/runtime"; + +function* installGitHubAuth() { + const token = yield* runtimeEnv("GITHUB_TOKEN"); + if (!token) { + return; + } + yield* FetchApi.around({ + *fetch([input, init, shouldExpect], next) { + let url; + try { + url = input instanceof Request ? new URL(input.url) : new URL(input); + } catch { + return yield* next(input, init, shouldExpect); + } + + if (url.protocol !== "https:" || url.hostname !== "api.github.com") { + return yield* next(input, init, shouldExpect); + } + + const headers = new Headers(input instanceof Request ? input.headers : undefined); + const requestHeaders = new Headers(init?.headers); + requestHeaders.forEach((value, key) => headers.set(key, value)); + headers.set("Authorization", `Bearer ${token}`); + if (!headers.has("Accept")) { + headers.set("Accept", "application/vnd.github+json"); + } + + return yield* next(input, { ...init, headers }, shouldExpect); + }, + }); +} + +yield* installGitHubAuth(); +``` + + diff --git a/.reviews/components/GitHubComment.md b/.reviews/components/GitHubComment.md index 3d7ee32f..30a6f03b 100644 --- a/.reviews/components/GitHubComment.md +++ b/.reviews/components/GitHubComment.md @@ -9,47 +9,39 @@ props: --- ```ts eval -const content = yield* renderChildren(); -const body = props.marker + "\n" + content.trim(); +import { env as runtimeEnv } from "@executablemd/runtime"; -const repo = process.env.GITHUB_REPOSITORY; -const prNumber = process.env.PR_NUMBER; -const [owner, name] = repo.split("/"); -const api = `https://api.github.com/repos/${owner}/${name}`; - -function githubHeaders() { - return { - "Authorization": `Bearer ${process.env.GITHUB_TOKEN}`, - "Accept": "application/vnd.github+json", - }; +function* reviewConfiguration() { + const repository = yield* runtimeEnv("GITHUB_REPOSITORY"); + const number = yield* runtimeEnv("PR_NUMBER"); + if (!repository || !number || !repository.includes("/")) { + throw new Error("GitHubComment requires GITHUB_REPOSITORY and PR_NUMBER"); + } + const [owner, name] = repository.split("/"); + return { api: `https://api.github.com/repos/${owner}/${name}`, number }; } -const commentsResult = yield* fetch(`${api}/issues/${prNumber}/comments`, { - headers: githubHeaders(), -}) - .expect() - .json(); +const content = yield* renderChildren(); +const body = props.marker + "\n" + content.trim(); +const { api, number } = yield* reviewConfiguration(); +const comments = yield* fetch(`${api}/issues/${number}/comments`).expect().json(); +if (!Array.isArray(comments)) { + throw new Error("GitHub comments response was not an array"); +} -const existing = commentsResult.find(c => - c.user.type === "Bot" && c.body.includes(props.marker) +const existing = comments.find((comment) => + comment.user?.type === "Bot" && typeof comment.body === "string" && comment.body.includes(props.marker) ); - if (existing) { yield* fetch(`${api}/issues/comments/${existing.id}`, { method: "PATCH", - headers: { - ...githubHeaders(), - "Content-Type": "application/json", - }, + headers: { "Content-Type": "application/json" }, body: JSON.stringify({ body }), }).expect(); } else { - yield* fetch(`${api}/issues/${prNumber}/comments`, { + yield* fetch(`${api}/issues/${number}/comments`, { method: "POST", - headers: { - ...githubHeaders(), - "Content-Type": "application/json", - }, + headers: { "Content-Type": "application/json" }, body: JSON.stringify({ body }), }).expect(); } diff --git a/.reviews/components/OllamaProvider.md b/.reviews/components/OllamaProvider.md index 69e24eab..f4573423 100644 --- a/.reviews/components/OllamaProvider.md +++ b/.reviews/components/OllamaProvider.md @@ -32,7 +32,11 @@ yield* Sample.around({ .expect() .json(); - return result.choices[0].message.content; + const content = result.choices?.[0]?.message?.content; + if (typeof content !== "string") { + throw new Error("Ollama response did not contain model content"); + } + return content; }, }, { at: 'min' }); ``` diff --git a/.reviews/components/OxlintConfig.md b/.reviews/components/OxlintConfig.md new file mode 100644 index 00000000..ee07d8f2 --- /dev/null +++ b/.reviews/components/OxlintConfig.md @@ -0,0 +1,34 @@ +--- +props: + type: object + properties: + path: + type: string + default: ".reviews/tsconfig.oxlint.json" + additionalProperties: false +--- + +```ts eval +import { ensureDir, writeTextFile } from "@executablemd/runtime"; + +yield* ensureDir(".reviews"); +yield* writeTextFile(props.path, JSON.stringify({ + compilerOptions: { + target: "ESNext", + module: "ESNext", + moduleResolution: "bundler", + strict: true, + noEmit: true, + skipLibCheck: true, + resolveJsonModule: true, + lib: ["ESNext", "DOM"], + types: [], + }, + include: [ + "packages/*/src/**/*.ts", + "packages/*/*.ts", + "durable-effects/**/*.ts", + ], + exclude: ["node_modules", "dist", ".vendor", "**/*.test.ts"], +}, null, 2)); +``` diff --git a/.reviews/components/OxlintDiagnostics.ts b/.reviews/components/OxlintDiagnostics.ts new file mode 100644 index 00000000..2c102610 --- /dev/null +++ b/.reviews/components/OxlintDiagnostics.ts @@ -0,0 +1,96 @@ +import { type OxlintDiagnostic, normalizeOxlintOutput } from "@executablemd/code-review-agent"; +import { exec } from "@executablemd/runtime"; +import type { Operation } from "effection"; + +const OXLINT_PATH = ".reviews/.oxlint/oxlint"; +const OXLINT_CONFIG = ".reviews/.oxlintrc.json"; +const OXLINT_CRASH_PATTERN = /panic|oom|out of memory|fatal|segmentation fault/iu; + +export const props = { + type: "object", + properties: { + files: { type: "array", items: { type: "string" } }, + typeAware: { type: "boolean", default: false }, + tsconfigPath: { type: "string", default: ".reviews/tsconfig.oxlint.json" }, + }, + required: ["files"], + additionalProperties: false, +}; + +export const returns = { + type: "array", + items: { + type: "object", + properties: { + message: { type: "string" }, + ruleId: { type: "string" }, + severity: { type: "string" }, + file: { type: "string" }, + line: { type: "number" }, + column: { type: "number" }, + }, + required: ["message", "ruleId", "severity", "file", "line", "column"], + additionalProperties: false, + }, +}; + +interface OxlintDiagnosticsProps { + files: string[]; + typeAware?: boolean; + tsconfigPath?: string; +} + +interface OxlintCommand { + command: string[]; + environment: Record; +} + +function oxlintCommand(files: string[], typeAware: boolean, tsconfigPath: string): OxlintCommand { + const command = [OXLINT_PATH, "--config", OXLINT_CONFIG, "--format", "json"]; + const environment: Record = {}; + if (typeAware) { + command.push("--type-aware", "--tsconfig", tsconfigPath); + environment.OXLINT_TSGOLINT_PATH = ".reviews/.oxlint/tsgolint"; + } + command.push(...files); + return { command, environment }; +} + +function oxlintFailure(exitCode: number, stdout: string, stderr: string): Error | undefined { + if (exitCode > 1) { + return new Error(stderr || `Oxlint failed with exit code ${exitCode}`); + } + if (OXLINT_CRASH_PATTERN.test(stderr)) { + return new Error(stderr); + } + if (stdout.trim().length === 0) { + return new Error(stderr || "Oxlint returned no JSON output"); + } + return undefined; +} + +function* runOxlint( + files: string[], + typeAware: boolean, + tsconfigPath: string, +): Operation { + if (files.length === 0) { + return []; + } + + const { command, environment } = oxlintCommand(files, typeAware, tsconfigPath); + const result = yield* exec({ command, env: environment }); + const failure = oxlintFailure(result.exitCode, result.stdout, result.stderr); + if (failure !== undefined) { + throw failure; + } + return normalizeOxlintOutput(result.stdout, files); +} + +export default function* OxlintDiagnostics({ + files, + typeAware = false, + tsconfigPath = ".reviews/tsconfig.oxlint.json", +}: OxlintDiagnosticsProps): Operation { + return yield* runOxlint(files, typeAware, tsconfigPath); +} diff --git a/.reviews/components/RepositoryInventory.ts b/.reviews/components/RepositoryInventory.ts new file mode 100644 index 00000000..c60d7448 --- /dev/null +++ b/.reviews/components/RepositoryInventory.ts @@ -0,0 +1,71 @@ +import { glob, readTextFile } from "@executablemd/runtime"; +import type { Operation } from "effection"; + +export const props = { + type: "object", + properties: {}, + additionalProperties: false, +}; + +export const returns = { + type: "object", + properties: { + fileList: { type: "array", items: { type: "string" } }, + fileCount: { type: "number" }, + lineCount: { type: "number" }, + }, + required: ["fileList", "fileCount", "lineCount"], + additionalProperties: false, +}; + +interface RepositoryInventoryValue { + fileList: string[]; + fileCount: number; + lineCount: number; +} + +function lineCount(source: string): number { + if (source.length === 0) { + return 0; + } + const lines = source.split(/\r?\n/u); + if (source.endsWith("\n")) { + return lines.length - 1; + } + return lines.length; +} + +function filePaths(entries: { isFile: boolean; path: string }[]): string[] { + const paths = entries.filter((entry) => entry.isFile).map((entry) => entry.path); + const ordered: string[] = []; + for (const path of paths) { + const index = ordered.findIndex((candidate) => candidate > path); + if (index === -1) { + ordered.push(path); + } else { + ordered.splice(index, 0, path); + } + } + return ordered; +} + +function* totalLineCount(paths: string[]): Operation { + let total = 0; + for (const path of paths) { + total += lineCount(yield* readTextFile(path)); + } + return total; +} + +export default function* RepositoryInventory( + _props: Record, +): Operation { + const entries = yield* glob({ + root: ".", + patterns: ["durable-effects/**/*.ts", "packages/**/*.ts"], + exclude: ["**/*.test.ts", "**/*.spec.ts", "**/node_modules/**"], + }); + const fileList = filePaths(entries); + const totalLines = yield* totalLineCount(fileList); + return { fileList, fileCount: fileList.length, lineCount: totalLines }; +} diff --git a/.reviews/components/ReviewContext.ts b/.reviews/components/ReviewContext.ts new file mode 100644 index 00000000..7105255e --- /dev/null +++ b/.reviews/components/ReviewContext.ts @@ -0,0 +1,97 @@ +import { type PR, parseDiff } from "@executablemd/code-review-agent"; +import { exec, env as runtimeEnv } from "@executablemd/runtime"; +import { fetch } from "@effectionx/fetch"; +import type { Operation } from "effection"; + +export const props = { + type: "object", + properties: {}, + additionalProperties: false, +}; + +export const returns = { + type: "object", + properties: { + pr: { type: "object" }, + changedFilePaths: { type: "array", items: { type: "string" } }, + }, + required: ["pr", "changedFilePaths"], + additionalProperties: false, +}; + +interface PullRequestResponse { + body?: unknown; +} + +interface ReviewContextValue { + pr: Omit & { directories: string[] }; + changedFilePaths: string[]; +} + +interface ReviewInputs { + base: string; + head: string; + title: string; + number: string; + repository: string; + localBody: string; +} + +function isPullRequestResponse(value: unknown): value is PullRequestResponse { + return typeof value === "object" && value !== null; +} + +function* reviewInputs(): Operation { + return { + base: (yield* runtimeEnv("BASE_SHA")) ?? "HEAD~1", + head: (yield* runtimeEnv("HEAD_SHA")) ?? "HEAD", + title: (yield* runtimeEnv("PR_TITLE")) ?? "", + number: (yield* runtimeEnv("PR_NUMBER")) ?? "", + repository: (yield* runtimeEnv("GITHUB_REPOSITORY")) ?? "", + localBody: (yield* runtimeEnv("PR_BODY")) ?? "", + }; +} + +function* gitOutput(command: string[], description: string): Operation { + const result = yield* exec({ command }); + if (result.exitCode !== 0) { + throw new Error(result.stderr || `${description} failed with exit code ${result.exitCode}`); + } + return result.stdout; +} + +function* pullBody(repository: string, number: string, fallback: string): Operation { + if (fallback || !repository || !number) { + return fallback; + } + + const payload: unknown = yield* fetch( + `https://api.github.com/repos/${repository}/pulls/${number}`, + ) + .expect() + .json(); + if (!isPullRequestResponse(payload)) { + return fallback; + } + if (typeof payload.body === "string") { + return payload.body; + } + return fallback; +} + +export default function* ReviewContext( + _props: Record, +): Operation { + const inputs = yield* reviewInputs(); + const range = `${inputs.base}...${inputs.head}`; + const diff = yield* gitOutput(["git", "diff", range], "git diff"); + const names = yield* gitOutput(["git", "diff", "--name-status", range], "git diff --name-status"); + const body = yield* pullBody(inputs.repository, inputs.number, inputs.localBody); + const parsed = parseDiff(diff, names, { + title: inputs.title, + body, + number: inputs.number, + }); + const pr = { ...parsed, directories: [...parsed.directories] }; + return { pr, changedFilePaths: pr.files.map((file) => file.path) }; +} diff --git a/.reviews/components/ReviewSetup.md b/.reviews/components/ReviewSetup.md new file mode 100644 index 00000000..f210e7fd --- /dev/null +++ b/.reviews/components/ReviewSetup.md @@ -0,0 +1,30 @@ +--- +props: + type: object + properties: + pullModel: + type: boolean + default: false + model: + type: string + default: "qwen3:30b-a3b" + additionalProperties: false +--- + + + + + +```ts eval +import { exec } from "@executablemd/runtime"; + +if (props.pullModel) { + const installed = yield* exec({ command: ["ollama", "show", props.model] }); + if (installed.exitCode !== 0) { + const pulled = yield* exec({ command: ["ollama", "pull", props.model] }); + if (pulled.exitCode !== 0) { + throw new Error(pulled.stderr || `Unable to provision Ollama model ${props.model}`); + } + } +} +``` diff --git a/.reviews/components/SuggestRemoval.md b/.reviews/components/SuggestRemoval.md index 82b31ef9..895de97b 100644 --- a/.reviews/components/SuggestRemoval.md +++ b/.reviews/components/SuggestRemoval.md @@ -12,28 +12,36 @@ props: --- ```ts eval -const repo = process.env.GITHUB_REPOSITORY; -const prNumber = process.env.PR_NUMBER; -const headSha = process.env.HEAD_SHA; +import { env as runtimeEnv } from "@executablemd/runtime"; -function githubHeaders() { +function* githubConfiguration() { + const token = yield* runtimeEnv("GITHUB_TOKEN"); + const repo = yield* runtimeEnv("GITHUB_REPOSITORY"); + const prNumber = yield* runtimeEnv("PR_NUMBER"); + const headSha = yield* runtimeEnv("HEAD_SHA"); + if (!token || !repo || !prNumber || !headSha) { + return undefined; + } + const [owner, name] = repo.split("/"); return { - "Authorization": `Bearer ${process.env.GITHUB_TOKEN}`, - "Accept": "application/vnd.github+json", - "Content-Type": "application/json", + api: `https://api.github.com/repos/${owner}/${name}`, + graphql: "https://api.github.com/graphql", + prNumber, + headSha, + owner, + name, }; } -if (!process.env.GITHUB_TOKEN || !repo || !prNumber || !headSha) { +const github = yield* githubConfiguration(); +if (!github) { return ""; } -const [owner, name] = repo.split("/"); -const api = `https://api.github.com/repos/${owner}/${name}`; -const graphql = "https://api.github.com/graphql"; +const { api, graphql, prNumber, headSha, owner, name } = github; const existingReviews = yield* fetch( - `${api}/pulls/${prNumber}/reviews`, { headers: githubHeaders() } + `${api}/pulls/${prNumber}/reviews` ).expect().json(); const botReviews = existingReviews.filter(r => @@ -45,7 +53,6 @@ for (const review of botReviews) { try { yield* fetch(`${api}/pulls/${prNumber}/reviews/${review.id}`, { method: "DELETE", - headers: githubHeaders(), }).expect(); } catch { // Review may already be submitted (can't delete submitted reviews). @@ -75,7 +82,7 @@ if (props.dismissedReplies.length > 0) { try { const threadsResult = yield* fetch(graphql, { method: "POST", - headers: githubHeaders(), + headers: { "Content-Type": "application/json" }, body: JSON.stringify({ query: threadsQuery, variables: { owner, name, pr: parseInt(prNumber, 10) }, @@ -99,7 +106,7 @@ if (props.dismissedReplies.length > 0) { try { yield* fetch(`${api}/pulls/comments/${reply.replyId}/reactions`, { method: "POST", - headers: githubHeaders(), + headers: { "Content-Type": "application/json" }, body: JSON.stringify({ content: "+1" }), }).expect(); } catch {} @@ -112,7 +119,7 @@ if (props.dismissedReplies.length > 0) { try { yield* fetch(graphql, { method: "POST", - headers: githubHeaders(), + headers: { "Content-Type": "application/json" }, body: JSON.stringify({ query: `mutation($threadId: ID!) { resolveReviewThread(input: { threadId: $threadId }) { @@ -138,7 +145,7 @@ if (props.findings.length > 0) { yield* fetch(`${api}/pulls/${prNumber}/reviews`, { method: "POST", - headers: githubHeaders(), + headers: { "Content-Type": "application/json" }, body: JSON.stringify({ commit_id: headSha, event: "COMMENT", diff --git a/.reviews/components/Threshold.md b/.reviews/components/Threshold.md index 14738b9d..438e65f1 100644 --- a/.reviews/components/Threshold.md +++ b/.reviews/components/Threshold.md @@ -25,7 +25,7 @@ const metrics = { totalFiles: props.pr.stats.totalFiles, additions: props.pr.stats.additions, deletions: props.pr.stats.deletions, - directories: props.pr.directories.size, + directories: Array.isArray(props.pr.directories) ? props.pr.directories.length : props.pr.directories.size, }; const actual = metrics[props.metric]; diff --git a/deno.json b/deno.json index 209a8f0f..bdd04bfb 100644 --- a/deno.json +++ b/deno.json @@ -59,7 +59,7 @@ "test": "deno test --allow-all --frozen", "verify": "deno run --allow-all --node-modules-dir=none --cached-only --frozen scripts/preflight.ts scripts/verify.ts", "vendor:verify": "deno run --allow-read --allow-write=/tmp --allow-env --allow-run --cached-only --frozen scripts/verify-cloudflare-dofs.ts", - "check": "deno check --frozen", + "check": "deno check --frozen && deno check --frozen .reviews/components/*.ts", "check:jsr": "deno publish --dry-run --allow-dirty", "review": "deno run --allow-all packages/cli/src/deno.ts run .reviews/ReviewPR.md --component-dir .reviews/components --component-dir .reviews/policies --component-dir packages/core/components -j .reviews/journal.jsonl", "review:local": "deno run --allow-all packages/cli/src/deno.ts run .reviews/ReviewPR.local.md --component-dir .reviews/components --component-dir .reviews/policies --component-dir packages/core/components -j .reviews/journal.local.jsonl", diff --git a/package.json b/package.json index a9aea57d..9252ebfb 100644 --- a/package.json +++ b/package.json @@ -71,8 +71,8 @@ "test:node": "tsx scripts/runtime-tests.ts node", "test:bun": "bun scripts/runtime-tests.ts bun", "test:deno": "deno task test", - "lint": "oxlint -c .oxlintrc.json --ignore-pattern 'scripts/tests/fixtures/**' --ignore-pattern '**/npm/**' --ignore-pattern 'packages/workflow/vendor/cloudflare-computer-dofs/**' packages scripts && oxfmt --check packages scripts", - "fmt": "oxfmt --write packages scripts" + "lint": "oxlint -c .oxlintrc.json --ignore-pattern 'scripts/tests/fixtures/**' --ignore-pattern '**/npm/**' --ignore-pattern 'packages/workflow/vendor/cloudflare-computer-dofs/**' packages scripts .reviews/components && oxfmt --check packages scripts .reviews/components/*.ts", + "fmt": "oxfmt --write packages scripts .reviews/components/*.ts" }, "workspaces": [ "packages/*" diff --git a/packages/code-review-agent/mod.ts b/packages/code-review-agent/mod.ts index 47266d56..fa70278c 100644 --- a/packages/code-review-agent/mod.ts +++ b/packages/code-review-agent/mod.ts @@ -6,8 +6,11 @@ */ export { parseDiff } from "./src/parse-diff.ts"; -export { parseDiagnostics } from "./src/parse-diagnostics.ts"; +export { buildDiagnostics, parseDiagnostics } from "./src/parse-diagnostics.ts"; export { parseDoctorResult } from "./src/parse-doctor.ts"; +export { buildDoctorResult, isOxlintCrash, summarizeDoctorProbe } from "./src/doctor.ts"; +export { normalizeDiagnostic, normalizeOxlintOutput } from "./src/parse-oxlint.ts"; +export type { DoctorEnvironment, DoctorProbeInput, DoctorProbeSummary } from "./src/doctor.ts"; export { buildCleanupAnalysis, clusterByFile, diff --git a/packages/code-review-agent/src/comment-review-data.ts b/packages/code-review-agent/src/comment-review-data.ts new file mode 100644 index 00000000..9a801e08 --- /dev/null +++ b/packages/code-review-agent/src/comment-review-data.ts @@ -0,0 +1,196 @@ +import type { PR } from "./types.ts"; + +export interface Pair { + comment: string; + code: string; + file: string; + lineNumber: number; +} + +export interface Reply { + file: string; + lineNumber: number; + comment?: string; + botCommentId?: number; + replyText: string; + replyId?: number; + alreadyProcessed?: boolean; +} + +export interface Location { + file: string; + lineNumber: number; + comment: string; +} + +export interface ReviewData { + pairs: Pair[]; + hasPairs: boolean; + pairsText: string; + previousFindings: { file: string; lineNumber: number }[]; + dismissedReplies: Reply[]; + repliesForClassification: Reply[]; + hasRepliesToClassify: boolean; + repliesText: string; +} + +export function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null; +} + +export function stringValue(record: Record, key: string): string | undefined { + const value = record[key]; + if (typeof value === "string") { + return value; + } + return undefined; +} + +export function numberValue(record: Record, key: string): number | undefined { + const value = record[key]; + if (typeof value === "number") { + return value; + } + return undefined; +} + +export function records(value: unknown): Record[] { + if (!Array.isArray(value)) { + return []; + } + return value.filter((entry) => isRecord(entry)); +} + +export function nonEmpty(value: string | undefined): value is string { + return value !== undefined && value.length > 0; +} + +export function userLogin(record: Record): string | undefined { + const { user } = record; + if (isRecord(user)) { + return stringValue(user, "login"); + } + return undefined; +} + +export function userType(record: Record): string | undefined { + const { user } = record; + if (isRecord(user)) { + return stringValue(user, "type"); + } + return undefined; +} + +function isBotReview(record: Record): boolean { + return ( + userLogin(record) === "github-actions[bot]" && + (stringValue(record, "body")?.includes("Redundant comment") ?? false) + ); +} + +export function commentPayload(value: unknown): Record[] { + if (!Array.isArray(value) || !value.every((entry) => isRecord(entry))) { + throw new Error("GitHub comments response was not an array"); + } + return value; +} + +export function pairsFor(pr: PR): Pair[] { + const pairs: Pair[] = []; + const lines = pr.added.filter((line) => !line.isTest); + for (let index = 0; index < lines.length - 1; index++) { + const current = lines[index].content.trim(); + const next = lines[index + 1].content.trim(); + if (current.startsWith("//") && !next.startsWith("//") && next.length > 0) { + pairs.push({ + comment: current, + code: next, + file: lines[index].file, + lineNumber: lines[index].lineNumber, + }); + } + } + return pairs; +} + +export function replyLocation( + reply: Record, + botCommentMap: Map, +): Location | undefined { + const parentId = numberValue(reply, "in_reply_to_id"); + if (parentId === undefined) { + return undefined; + } + return botCommentMap.get(parentId); +} + +interface CommentLocationEntry { + id: number; + location: Location; +} + +function commentLine(diffHunk: string): string { + const addedLine = diffHunk + .split("\n") + .filter((line) => line.startsWith("+")) + .pop(); + return addedLine?.replace(/^\+\s*/u, "").trim() ?? ""; +} + +function commentLocation(comment: Record): CommentLocationEntry | undefined { + if (!isBotReview(comment)) { + return undefined; + } + const id = numberValue(comment, "id"); + const path = stringValue(comment, "path"); + const lineNumber = numberValue(comment, "original_line") ?? numberValue(comment, "line"); + if (id === undefined || !nonEmpty(path) || lineNumber === undefined) { + return undefined; + } + return { + id, + location: { + file: path, + lineNumber, + comment: commentLine(stringValue(comment, "diff_hunk") ?? ""), + }, + }; +} + +export function commentLocations(comments: Record[]): { + botCommentMap: Map; + previousFindings: { file: string; lineNumber: number }[]; +} { + const entries = comments + .map((comment) => commentLocation(comment)) + .filter((entry): entry is CommentLocationEntry => entry !== undefined); + const botCommentMap = new Map(); + for (const entry of entries) { + botCommentMap.set(entry.id, entry.location); + } + return { + botCommentMap, + previousFindings: entries.map(({ location }) => ({ + file: location.file, + lineNumber: location.lineNumber, + })), + }; +} + +export function formatPairs(pairs: Pair[], hasPairs: boolean): string { + if (!hasPairs) { + return ""; + } + return pairs + .map((pair, index) => `[${index}] COMMENT: ${pair.comment}\nCODE: ${pair.code}`) + .join("\n---\n"); +} + +export function formatReplies(replies: Reply[]): string { + return replies + .map( + (reply, index) => + `[${index}] FILE: ${reply.file}:${reply.lineNumber}\nREPLY: "${reply.replyText}"`, + ) + .join("\n---\n"); +} diff --git a/packages/code-review-agent/src/doctor.ts b/packages/code-review-agent/src/doctor.ts new file mode 100644 index 00000000..cb722bac --- /dev/null +++ b/packages/code-review-agent/src/doctor.ts @@ -0,0 +1,133 @@ +import type { DoctorResult, OxlintDiagnostic } from "./types.ts"; +import { TYPE_AWARE_RULES } from "./categories.ts"; + +const BLOAT_RULES = [ + "no-unused-vars", + "no-inferrable-types", + "no-empty-function", + "no-empty-object-type", + "no-useless-empty-export", + "no-unnecessary-type-constraint", + "no-unnecessary-parameter-property-assignment", + "no-static-only-class", + "no-console", + "no-debugger", + ...TYPE_AWARE_RULES, +]; +const TYPE_AWARE_RULE_SET = new Set(TYPE_AWARE_RULES); +const IMPORT_NOISE_RATIO = 0.3; +const OXLINT_CRASH_PATTERN = /panic|oom|out of memory|fatal|segmentation fault/iu; + +export interface DoctorProbeSummary { + typeAwareAvailable: boolean; + filesAnalyzed: number; + filesSkipped: number; + importErrors: number; + availableRuleIds: string[]; + bloatRulesAvailable: string[]; + bloatRulesMissing: string[]; + recommendation: DoctorResult["recommendation"]; +} + +export interface DoctorProbeInput { + diagnostics: readonly OxlintDiagnostic[]; + stderr: string; + exitCode: number; +} + +function bloatRulesFor(typeAwareAvailable: boolean): { + available: string[]; + missing: string[]; +} { + if (typeAwareAvailable) { + return { available: [...BLOAT_RULES], missing: [] }; + } + return { + available: BLOAT_RULES.filter((rule) => !TYPE_AWARE_RULE_SET.has(rule)), + missing: [...TYPE_AWARE_RULES], + }; +} + +function recommendationFor( + typeAwareAvailable: boolean, + ratio: number, +): DoctorResult["recommendation"] { + if (!typeAwareAvailable) { + return "syntax-only"; + } + if (ratio < IMPORT_NOISE_RATIO) { + return "type-aware"; + } + return "type-aware-filtered"; +} + +function noiseRatio(total: number, importNoise: number): number { + if (total === 0) { + return 0; + } + return importNoise / total; +} + +function availableRuleIds(diagnostics: readonly OxlintDiagnostic[]): string[] { + return [...new Set(diagnostics.map((diagnostic) => diagnostic.ruleId).filter(Boolean))].sort(); +} + +function isImportNoise(diagnostic: OxlintDiagnostic): boolean { + return ( + diagnostic.message.includes("Cannot find module") || + diagnostic.message.includes("cannot find") || + diagnostic.ruleId.includes("import") + ); +} + +export function isOxlintCrash(stderr: string): boolean { + return OXLINT_CRASH_PATTERN.test(stderr); +} + +/** Summarize a bounded type-aware probe without retaining its raw output. */ +export function summarizeDoctorProbe(input: DoctorProbeInput): DoctorProbeSummary { + const importNoise = input.diagnostics.filter(isImportNoise); + const files = new Set(input.diagnostics.map((diagnostic) => diagnostic.file).filter(Boolean)); + const skippedFiles = new Set(importNoise.map((diagnostic) => diagnostic.file).filter(Boolean)); + const available = input.exitCode <= 1 && !isOxlintCrash(input.stderr); + const ratio = noiseRatio(input.diagnostics.length, importNoise.length); + const rules = bloatRulesFor(available); + + return { + typeAwareAvailable: available, + filesAnalyzed: files.size, + filesSkipped: skippedFiles.size, + importErrors: importNoise.length, + availableRuleIds: availableRuleIds(input.diagnostics), + bloatRulesAvailable: rules.available, + bloatRulesMissing: rules.missing, + recommendation: recommendationFor(available, ratio), + }; +} + +export interface DoctorEnvironment { + oxlintInstalled: boolean; + oxlintVersion: string; + tsgolintInstalled: boolean; + tsgolintVersion: string; + tsconfigExists: boolean; + nodeModulesExists: boolean; + nativeSpecifiers: DoctorResult["nativeSpecifiers"]; +} + +/** Construct the durable Doctor value from bounded environment observations. */ +export function buildDoctorResult( + environment: DoctorEnvironment, + probe: DoctorProbeSummary, +): DoctorResult { + return { + ...environment, + ...probe, + nativeSpecifiers: { + count: environment.nativeSpecifiers.count, + files: [...environment.nativeSpecifiers.files], + jsr: environment.nativeSpecifiers.jsr, + npm: environment.nativeSpecifiers.npm, + }, + }; +} diff --git a/packages/code-review-agent/src/parse-diagnostics.ts b/packages/code-review-agent/src/parse-diagnostics.ts index a27630fa..6f23dc43 100644 --- a/packages/code-review-agent/src/parse-diagnostics.ts +++ b/packages/code-review-agent/src/parse-diagnostics.ts @@ -1,13 +1,10 @@ -/** - * Parses raw Oxlint JSON output into structured Diagnostics. - * - * Groups by ruleId, categorizes into structural/verbosity/typeAware/other, - * filters import noise when doctor recommends it, computes density, and - * generates a human-readable summary string. - */ - +import type { DiagnosticGroup, Diagnostics, DoctorResult, OxlintDiagnostic, PR } from "./types.ts"; import { categorizeRule } from "./categories.ts"; -import type { Diagnostics, DiagnosticGroup, DoctorResult, OxlintDiagnostic, PR } from "./types.ts"; +import { normalizeOxlintOutput } from "./parse-oxlint.ts"; + +const IMPORT_NOISE_MARKERS = ["Cannot find module", "cannot find"]; +const SUMMARY_FILE_LIMIT = 3; +const DENSITY_PRECISION = 1000; function emptyDiagnostics(): Diagnostics { return { @@ -26,191 +23,190 @@ function emptyDiagnostics(): Diagnostics { }; } -function extractDiagnosticsArray(parsed: unknown): unknown[] { - if (Array.isArray(parsed)) { - return parsed; - } - - if (parsed && typeof parsed === "object") { - const direct = (parsed as { diagnostics?: unknown }).diagnostics; - if (Array.isArray(direct)) { - return direct; - } +function isImportNoise(diagnostic: OxlintDiagnostic): boolean { + return ( + IMPORT_NOISE_MARKERS.some((marker) => diagnostic.message.includes(marker)) || + diagnostic.ruleId.includes("import") + ); +} - if (direct && typeof direct === "object") { - const nested = (direct as { diagnostics?: unknown }).diagnostics; - if (Array.isArray(nested)) { - return nested; - } - } +function filteredDiagnostics( + raw: readonly OxlintDiagnostic[], + doctor: DoctorResult, +): OxlintDiagnostic[] { + if (doctor.recommendation !== "type-aware-filtered") { + return [...raw]; } - - return []; + return raw.filter((diagnostic) => !isImportNoise(diagnostic)); } -function parseRuleId(code: string): string { - const match = /\(([^)]+)\)/.exec(code); - return match?.[1] ?? code; +function groupedDiagnostics(raw: readonly OxlintDiagnostic[]): Map { + const groups = new Map(); + for (const diagnostic of raw) { + const { ruleId } = diagnostic; + const group = groups.get(ruleId); + let next = [diagnostic]; + if (group !== undefined) { + next = [...group, diagnostic]; + } + groups.set(ruleId, next); + } + return groups; } -function normalizeDiagnostic(entry: unknown): OxlintDiagnostic | null { - if (!entry || typeof entry !== "object") { - return null; +function filesForGroup(instances: readonly OxlintDiagnostic[]): string[] { + const files = new Set(); + for (const diagnostic of instances) { + if (diagnostic.file.length > 0) { + files.add(diagnostic.file); + } } - - const diagnostic = entry as Record; - - const ruleId = - typeof diagnostic.ruleId === "string" - ? diagnostic.ruleId - : typeof diagnostic.code === "string" - ? parseRuleId(diagnostic.code) - : "unknown"; - - const severity = diagnostic.severity === "error" ? "error" : "warning"; - const message = typeof diagnostic.message === "string" ? diagnostic.message : ""; - - const file = - typeof diagnostic.file === "string" - ? diagnostic.file - : typeof diagnostic.filename === "string" - ? diagnostic.filename - : ""; - - const firstLabel = Array.isArray(diagnostic.labels) ? diagnostic.labels[0] : undefined; - const firstSpan = - firstLabel && typeof firstLabel === "object" - ? (firstLabel as { span?: unknown }).span - : undefined; - - const line = - typeof diagnostic.line === "number" - ? diagnostic.line - : firstSpan && - typeof firstSpan === "object" && - typeof (firstSpan as { line?: unknown }).line === "number" - ? (firstSpan as { line: number }).line - : 0; - - const column = - typeof diagnostic.column === "number" - ? diagnostic.column - : firstSpan && - typeof firstSpan === "object" && - typeof (firstSpan as { column?: unknown }).column === "number" - ? (firstSpan as { column: number }).column - : 0; - - return { - ruleId, - severity, - message, - file, - line, - column, - }; + return [...files]; } -/** - * Parse raw Oxlint JSON output into structured diagnostics. - */ -export function parseDiagnostics(rawJson: string, pr: PR, doctor: DoctorResult): Diagnostics { - let raw: OxlintDiagnostic[]; - try { - const parsed = JSON.parse(rawJson); - raw = extractDiagnosticsArray(parsed) - .map((entry) => normalizeDiagnostic(entry)) - .filter((entry): entry is OxlintDiagnostic => entry !== null); - } catch { - return emptyDiagnostics(); +function fileCountFor(raw: readonly OxlintDiagnostic[]): number { + const files = new Set(); + for (const diagnostic of raw) { + if (diagnostic.file.length > 0) { + files.add(diagnostic.file); + } } + return files.size; +} - const filtered = - doctor.recommendation === "type-aware-filtered" - ? raw.filter((d) => { - const msg = d.message ?? ""; - return !msg.includes("Cannot find module") && !msg.includes("cannot find"); - }) - : raw; - - const groupMap = new Map(); - for (const d of filtered) { - const key = d.ruleId ?? "unknown"; - const arr = groupMap.get(key); - if (arr) { - arr.push(d); +function diagnosticGroups(raw: readonly OxlintDiagnostic[]): DiagnosticGroup[] { + const groups = [...groupedDiagnostics(raw).entries()].map(([ruleId, instances]) => ({ + ruleId, + count: instances.length, + files: filesForGroup(instances), + instances, + })); + const ordered: DiagnosticGroup[] = []; + for (const group of groups) { + const index = ordered.findIndex((candidate) => candidate.count < group.count); + if (index === -1) { + ordered.push(group); } else { - groupMap.set(key, [d]); + ordered.splice(index, 0, group); } } + return ordered; +} - // Build groups sorted by count descending - const groups: DiagnosticGroup[] = [...groupMap.entries()] - .map(([ruleId, instances]) => ({ - ruleId, - count: instances.length, - files: [...new Set(instances.map((d) => d.file).filter(Boolean))], - instances, - })) - .sort((a, b) => b.count - a.count); - +function categorizedGroups(groups: readonly DiagnosticGroup[]): Diagnostics["byCategory"] { const byCategory: Diagnostics["byCategory"] = { structural: [], verbosity: [], typeAware: [], other: [], }; - for (const group of groups) { - const cats = categorizeRule(group.ruleId); - for (const cat of cats) { - byCategory[cat].push(group); + for (const category of categorizeRule(group.ruleId)) { + byCategory[category].push(group); } } + return byCategory; +} - const total = filtered.length; - const allFiles = new Set(filtered.map((d) => d.file).filter(Boolean)); - const fileCount = allFiles.size; - const ruleCount = groups.length; - const density = - pr.stats.additions > 0 ? Math.round((total / pr.stats.additions) * 1000) / 1000 : 0; - - const lines: string[] = []; - lines.push( - `Oxlint: ${total} diagnostic${total !== 1 ? "s" : ""} across ${fileCount} file${fileCount !== 1 ? "s" : ""} (${ruleCount} rule${ruleCount !== 1 ? "s" : ""})`, - ); - lines.push(`Density: ${density.toFixed(3)} violations/added-line`); - lines.push(""); +function densityFor(pr: PR, total: number): number { + if (pr.stats.additions === 0) { + return 0; + } + return Math.round((total / pr.stats.additions) * DENSITY_PRECISION) / DENSITY_PRECISION; +} - for (const g of groups) { - const fileList = g.files.slice(0, 3).join(", "); - const more = g.files.length > 3 ? ` (+${g.files.length - 3})` : ""; - lines.push(` ${g.ruleId} (${g.count}): ${fileList}${more}`); +function pluralized(count: number, singular: string, plural: string): string { + if (count === 1) { + return singular; } + return plural; +} + +interface SummaryInput { + total: number; + fileCount: number; + ruleCount: number; + density: number; + groups: readonly DiagnosticGroup[]; + doctor: DoctorResult; +} - // Coverage annotations +function summaryFor({ + total, + fileCount, + ruleCount, + density, + groups, + doctor, +}: SummaryInput): string { + const lines = [ + `Oxlint: ${total} ${pluralized(total, "diagnostic", "diagnostics")} across ${fileCount} ${pluralized(fileCount, "file", "files")} (${ruleCount} ${pluralized(ruleCount, "rule", "rules")})`, + `Density: ${density.toFixed(3)} violations/added-line`, + "", + ]; + for (const group of groups) { + const fileList = group.files.slice(0, SUMMARY_FILE_LIMIT).join(", "); + const moreFiles = group.files.length - SUMMARY_FILE_LIMIT; + let more = ""; + if (moreFiles > 0) { + more = ` (+${moreFiles})`; + } + lines.push(` ${group.ruleId} (${group.count}): ${fileList}${more}`); + } + appendDoctorNotes(lines, doctor); + return lines.join("\n"); +} + +function appendDoctorNotes(lines: string[], doctor: DoctorResult): void { if (doctor.bloatRulesMissing.length > 0) { - lines.push(""); lines.push( + "", `Note: ${doctor.bloatRulesMissing.length} type-aware rules unavailable (${doctor.bloatRulesMissing.join(", ")}). Density may be understated.`, ); } - if (doctor.nativeSpecifiers.count > 0) { - lines.push(""); lines.push( + "", `Note: ${doctor.nativeSpecifiers.count} source files use scheme specifiers (jsr:, npm:).`, + "Run `deno lint --fix` with no-scheme-specifiers plugin to migrate.", ); - lines.push("Run `deno lint --fix` with no-scheme-specifiers plugin to migrate."); } +} +/** Group bounded Oxlint diagnostics for review policies. */ +export function buildDiagnostics( + raw: readonly OxlintDiagnostic[], + pr: PR, + doctor: DoctorResult, +): Diagnostics { + const filtered = filteredDiagnostics(raw, doctor); + const groups = diagnosticGroups(filtered); + const total = filtered.length; + const fileCount = fileCountFor(filtered); + const density = densityFor(pr, total); return { groups, total, fileCount, - ruleCount, - byCategory, - summary: lines.join("\n"), + ruleCount: groups.length, + byCategory: categorizedGroups(groups), + summary: summaryFor({ + total, + fileCount, + ruleCount: groups.length, + density, + groups, + doctor, + }), density, }; } + +/** Parse raw Oxlint JSON into structured diagnostics. */ +export function parseDiagnostics(rawJson: string, pr: PR, doctor: DoctorResult): Diagnostics { + try { + return buildDiagnostics(normalizeOxlintOutput(rawJson), pr, doctor); + } catch { + return emptyDiagnostics(); + } +} diff --git a/packages/code-review-agent/src/parse-doctor.ts b/packages/code-review-agent/src/parse-doctor.ts index 69cf4482..44900648 100644 --- a/packages/code-review-agent/src/parse-doctor.ts +++ b/packages/code-review-agent/src/parse-doctor.ts @@ -1,8 +1,3 @@ -/** - * Parses the JSON string produced by Doctor.md into a typed DoctorResult. - * Applies defaults for every field so downstream code never sees undefined. - */ - import type { DoctorResult } from "./types.ts"; const DEFAULTS: DoctorResult = { @@ -28,18 +23,118 @@ const DEFAULTS: DoctorResult = { }, }; +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +function booleanValue(record: Record, key: string): boolean | undefined { + const value = record[key]; + if (typeof value === "boolean") { + return value; + } + return undefined; +} + +function numberValue(record: Record, key: string): number | undefined { + const value = record[key]; + if (typeof value === "number" && Number.isFinite(value)) { + return value; + } + return undefined; +} + +function stringValue(record: Record, key: string): string | undefined { + const value = record[key]; + if (typeof value === "string") { + return value; + } + return undefined; +} + +function isStringArray(value: unknown): value is string[] { + return Array.isArray(value) && value.every((item): item is string => typeof item === "string"); +} + +function stringArrayValue(record: Record, key: string): string[] | undefined { + const value = record[key]; + if (isStringArray(value)) { + return value; + } + return undefined; +} + +function recommendationValue( + record: Record, +): DoctorResult["recommendation"] | undefined { + const value = record.recommendation; + if (value === "type-aware" || value === "type-aware-filtered" || value === "syntax-only") { + return value; + } + return undefined; +} + +function nativeSpecifierValue(value: unknown): DoctorResult["nativeSpecifiers"] { + if (!isRecord(value)) { + return cloneNativeSpecifiers(DEFAULTS.nativeSpecifiers); + } + return { + count: numberValue(value, "count") ?? DEFAULTS.nativeSpecifiers.count, + files: stringArrayValue(value, "files") ?? [...DEFAULTS.nativeSpecifiers.files], + jsr: numberValue(value, "jsr") ?? DEFAULTS.nativeSpecifiers.jsr, + npm: numberValue(value, "npm") ?? DEFAULTS.nativeSpecifiers.npm, + }; +} + +function cloneNativeSpecifiers( + value: DoctorResult["nativeSpecifiers"], +): DoctorResult["nativeSpecifiers"] { + return { ...value, files: [...value.files] }; +} + +function defaultDoctorResult(): DoctorResult { + return { + ...DEFAULTS, + bloatRulesAvailable: [...DEFAULTS.bloatRulesAvailable], + bloatRulesMissing: [...DEFAULTS.bloatRulesMissing], + nativeSpecifiers: cloneNativeSpecifiers(DEFAULTS.nativeSpecifiers), + }; +} + +function parseDoctorObject(record: Record): DoctorResult { + return { + oxlintInstalled: booleanValue(record, "oxlintInstalled") ?? DEFAULTS.oxlintInstalled, + oxlintVersion: stringValue(record, "oxlintVersion") ?? DEFAULTS.oxlintVersion, + tsgolintInstalled: booleanValue(record, "tsgolintInstalled") ?? DEFAULTS.tsgolintInstalled, + tsgolintVersion: stringValue(record, "tsgolintVersion") ?? DEFAULTS.tsgolintVersion, + tsconfigExists: booleanValue(record, "tsconfigExists") ?? DEFAULTS.tsconfigExists, + nodeModulesExists: booleanValue(record, "nodeModulesExists") ?? DEFAULTS.nodeModulesExists, + typeAwareAvailable: booleanValue(record, "typeAwareAvailable") ?? DEFAULTS.typeAwareAvailable, + filesAnalyzed: numberValue(record, "filesAnalyzed") ?? DEFAULTS.filesAnalyzed, + filesSkipped: numberValue(record, "filesSkipped") ?? DEFAULTS.filesSkipped, + importErrors: numberValue(record, "importErrors") ?? DEFAULTS.importErrors, + availableRuleIds: stringArrayValue(record, "availableRuleIds") ?? [ + ...DEFAULTS.availableRuleIds, + ], + bloatRulesAvailable: stringArrayValue(record, "bloatRulesAvailable") ?? [ + ...DEFAULTS.bloatRulesAvailable, + ], + bloatRulesMissing: stringArrayValue(record, "bloatRulesMissing") ?? [ + ...DEFAULTS.bloatRulesMissing, + ], + recommendation: recommendationValue(record) ?? DEFAULTS.recommendation, + nativeSpecifiers: nativeSpecifierValue(record.nativeSpecifiers), + }; +} + +/** Parse the published Doctor JSON contract while applying safe defaults. */ export function parseDoctorResult(json: string): DoctorResult { try { - const parsed = JSON.parse(json); - return { - ...DEFAULTS, - ...parsed, - nativeSpecifiers: { - ...DEFAULTS.nativeSpecifiers, - ...(parsed.nativeSpecifiers ?? {}), - }, - }; + const parsed: unknown = JSON.parse(json); + if (isRecord(parsed)) { + return parseDoctorObject(parsed); + } + return defaultDoctorResult(); } catch { - return { ...DEFAULTS }; + return defaultDoctorResult(); } } diff --git a/packages/code-review-agent/src/parse-oxlint.ts b/packages/code-review-agent/src/parse-oxlint.ts new file mode 100644 index 00000000..a4b4192b --- /dev/null +++ b/packages/code-review-agent/src/parse-oxlint.ts @@ -0,0 +1,155 @@ +import type { OxlintDiagnostic } from "./types.ts"; + +const RULE_CODE_PATTERN = /\((?[^)]+)\)/u; + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null; +} + +function stringValue(record: Record, key: string): string | undefined { + const value = record[key]; + if (typeof value === "string") { + return value; + } + return undefined; +} + +function numberValue(record: Record, key: string): number | undefined { + const value = record[key]; + if (typeof value === "number" && Number.isFinite(value)) { + return value; + } + return undefined; +} + +function firstSpan(record: Record): Record | undefined { + const { labels } = record; + if (!Array.isArray(labels)) { + return undefined; + } + const [label] = Array.from(labels, (entry: unknown) => entry); + if (!isRecord(label) || !isRecord(label.span)) { + return undefined; + } + return label.span; +} + +function spanNumber(span: Record | undefined, key: string): number | undefined { + if (span === undefined) { + return undefined; + } + return numberValue(span, key); +} + +function ruleIdFromCode(code: string): string { + const match = RULE_CODE_PATTERN.exec(code); + return match?.groups?.ruleId ?? code; +} + +function ruleIdFor(value: Record, code: string | undefined): string { + const explicitRuleId = stringValue(value, "ruleId"); + if (explicitRuleId !== undefined) { + return explicitRuleId; + } + if (code !== undefined) { + return ruleIdFromCode(code); + } + return "unknown"; +} + +function severityFor(value: string | undefined): "error" | "warning" { + if (value === "error") { + return "error"; + } + return "warning"; +} + +function positionFor( + value: Record, + span: Record | undefined, + key: string, +): number { + return numberValue(value, key) ?? spanNumber(span, key) ?? 0; +} + +/** Convert one Oxlint object to the bounded representation used by reviews. */ +export function normalizeDiagnostic(value: unknown): OxlintDiagnostic | undefined { + if (!isRecord(value)) { + return undefined; + } + + const code = stringValue(value, "code"); + const span = firstSpan(value); + const severity = stringValue(value, "severity"); + const file = stringValue(value, "file") ?? stringValue(value, "filename") ?? ""; + return { + message: stringValue(value, "message") ?? "", + ruleId: ruleIdFor(value, code), + severity: severityFor(severity), + file, + line: positionFor(value, span, "line"), + column: positionFor(value, span, "column"), + }; +} + +function diagnosticEntries(value: unknown): unknown[] | undefined { + if (Array.isArray(value)) { + return Array.from(value, (entry: unknown) => entry); + } + if (!isRecord(value)) { + return undefined; + } + + const { diagnostics } = value; + if (Array.isArray(diagnostics)) { + return Array.from(diagnostics, (entry: unknown) => entry); + } + if (isRecord(diagnostics) && Array.isArray(diagnostics.diagnostics)) { + return Array.from(diagnostics.diagnostics, (entry: unknown) => entry); + } + return undefined; +} + +function parseOxlintJson(stdout: string): unknown { + try { + const parsed: unknown = JSON.parse(stdout); + return parsed; + } catch (error) { + let message = String(error); + if (error instanceof Error) { + const { message: errorMessage } = error; + message = errorMessage; + } + throw new Error(`Oxlint returned malformed JSON: ${message}`, { cause: error }); + } +} + +function changedFiles(files: readonly string[] | undefined): Set | undefined { + if (files === undefined) { + return undefined; + } + return new Set(files); +} + +/** + * Parse and bound Oxlint JSON before it enters an executable document value. + * Unknown fields never cross this boundary. + */ +export function normalizeOxlintOutput( + stdout: string, + files?: readonly string[], +): OxlintDiagnostic[] { + const entries = diagnosticEntries(parseOxlintJson(stdout)); + if (entries === undefined) { + throw new Error("Oxlint JSON did not contain a diagnostics array"); + } + + const changed = changedFiles(files); + return entries.flatMap((entry) => { + const diagnostic = normalizeDiagnostic(entry); + if (diagnostic === undefined || (changed !== undefined && !changed.has(diagnostic.file))) { + return []; + } + return [diagnostic]; + }); +} diff --git a/packages/code-review-agent/tests/doctor.test.ts b/packages/code-review-agent/tests/doctor.test.ts new file mode 100644 index 00000000..ca43e1a2 --- /dev/null +++ b/packages/code-review-agent/tests/doctor.test.ts @@ -0,0 +1,61 @@ +import { describe, it } from "@executablemd/test-support/bdd"; +import { expect } from "@executablemd/test-support/expect"; +import { buildDoctorResult, summarizeDoctorProbe } from "../src/doctor.ts"; + +describe("Doctor analysis", () => { + it("classifies import noise and chooses filtered type-aware mode", function* () { + const summary = summarizeDoctorProbe({ + exitCode: 1, + stderr: "", + diagnostics: [ + { + message: "Cannot find module x", + ruleId: "import/no-unresolved", + severity: "error", + file: "a.ts", + line: 1, + column: 1, + }, + { + message: "unused", + ruleId: "no-unused-vars", + severity: "warning", + file: "b.ts", + line: 2, + column: 1, + }, + ], + }); + + expect(summary.typeAwareAvailable).toBe(true); + expect(summary.recommendation).toBe("type-aware-filtered"); + expect(summary.importErrors).toBe(1); + expect(summary.filesAnalyzed).toBe(2); + expect(summary.filesSkipped).toBe(1); + expect(summary.availableRuleIds).toEqual(["import/no-unresolved", "no-unused-vars"]); + }); + + it("classifies a type-aware crash without exporting raw process output", function* () { + const summary = summarizeDoctorProbe({ + exitCode: 1, + stderr: "tsgolint panic: OOM", + diagnostics: [], + }); + const result = buildDoctorResult( + { + oxlintInstalled: true, + oxlintVersion: "oxlint 1", + tsgolintInstalled: true, + tsgolintVersion: "tsgolint 1", + tsconfigExists: true, + nodeModulesExists: true, + nativeSpecifiers: { count: 0, files: [], jsr: 0, npm: 0 }, + }, + summary, + ); + + expect(result.typeAwareAvailable).toBe(false); + expect(result.recommendation).toBe("syntax-only"); + expect(JSON.stringify(result)).not.toContain("panic"); + }); +}); diff --git a/packages/code-review-agent/tests/parse-diagnostics.test.ts b/packages/code-review-agent/tests/parse-diagnostics.test.ts index cb02e139..7e4209aa 100644 --- a/packages/code-review-agent/tests/parse-diagnostics.test.ts +++ b/packages/code-review-agent/tests/parse-diagnostics.test.ts @@ -4,7 +4,7 @@ */ import { describe, it } from "@executablemd/test-support/bdd"; import { expect } from "@executablemd/test-support/expect"; -import { parseDiagnostics } from "../src/parse-diagnostics.ts"; +import { buildDiagnostics, parseDiagnostics } from "../src/parse-diagnostics.ts"; import type { DoctorResult, OxlintDiagnostic, PR } from "../src/types.ts"; function makePR(additions = 100): PR { @@ -56,6 +56,13 @@ function makeDiag(ruleId: string, file: string, line = 1, message = ""): OxlintD } describe("parseDiagnostics", () => { + it("builds directly from normalized diagnostics", function* () { + const result = buildDiagnostics([makeDiag("no-console", "src/a.ts")], makePR(), makeDoctor()); + + expect(result.total).toBe(1); + expect(result.groups[0].ruleId).toBe("no-console"); + }); + it("PD1: groups diagnostics by ruleId", function* () { const raw = JSON.stringify([ makeDiag("no-unused-vars", "src/a.ts"), @@ -246,11 +253,17 @@ describe("parseDiagnostics", () => { expect(result.byCategory.structural.map((g) => g.ruleId)).toContain("no-unused-vars"); }); - it("PD8: malformed JSON returns empty diagnostics", function* () { + it("preserves tolerant parsing for malformed JSON", function* () { const result = parseDiagnostics("not valid json {{{", makePR(), makeDoctor()); expect(result.total).toBe(0); - expect(result.density).toBe(0); + expect(result.groups).toHaveLength(0); + }); + + it("preserves tolerant parsing for unsupported JSON shapes", function* () { + const result = parseDiagnostics(JSON.stringify({ output: [] }), makePR(), makeDoctor()); + + expect(result.total).toBe(0); expect(result.groups).toHaveLength(0); }); diff --git a/packages/code-review-agent/tests/parse-doctor.test.ts b/packages/code-review-agent/tests/parse-doctor.test.ts new file mode 100644 index 00000000..7000b20f --- /dev/null +++ b/packages/code-review-agent/tests/parse-doctor.test.ts @@ -0,0 +1,27 @@ +import { describe, it } from "@executablemd/test-support/bdd"; +import { expect } from "@executablemd/test-support/expect"; +import { parseDoctorResult } from "../mod.ts"; + +describe("parseDoctorResult", () => { + it("parses a complete Doctor value", function* () { + const result = parseDoctorResult( + JSON.stringify({ + oxlintInstalled: true, + recommendation: "type-aware", + nativeSpecifiers: { count: 2, files: ["a.ts"], jsr: 1, npm: 1 }, + }), + ); + + expect(result.oxlintInstalled).toBe(true); + expect(result.recommendation).toBe("type-aware"); + expect(result.nativeSpecifiers.count).toBe(2); + }); + + it("returns defaults for malformed input", function* () { + const result = parseDoctorResult("not json"); + + expect(result.oxlintInstalled).toBe(false); + expect(result.recommendation).toBe("syntax-only"); + expect(result.nativeSpecifiers.files).toEqual([]); + }); +}); diff --git a/packages/code-review-agent/tests/parse-oxlint.test.ts b/packages/code-review-agent/tests/parse-oxlint.test.ts new file mode 100644 index 00000000..d3e0336f --- /dev/null +++ b/packages/code-review-agent/tests/parse-oxlint.test.ts @@ -0,0 +1,55 @@ +import { describe, it } from "@executablemd/test-support/bdd"; +import { expect } from "@executablemd/test-support/expect"; +import { normalizeOxlintOutput } from "../src/parse-oxlint.ts"; + +describe("Oxlint normalization", () => { + it("keeps only the bounded diagnostic fields and filters files", function* () { + const result = normalizeOxlintOutput( + JSON.stringify({ + diagnostics: [ + { + message: "unused", + code: "eslint(no-unused-vars)", + severity: "warning", + filename: "src/a.ts", + labels: [{ span: { line: 4, column: 7 } }], + source: "SECRET_SOURCE_EXCERPT", + cause: "ARBITRARY_CAUSE", + url: "https://example.invalid/diagnostic", + }, + { message: "other", file: "src/b.ts" }, + ], + stderr: "UNBOUNDED_STDERR", + }), + ["src/a.ts"], + ); + + expect(result).toEqual([ + { + message: "unused", + ruleId: "no-unused-vars", + severity: "warning", + file: "src/a.ts", + line: 4, + column: 7, + }, + ]); + }); + + it("rejects malformed or unsupported output", function* () { + let caught: unknown; + try { + normalizeOxlintOutput("not json"); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(Error); + if (caught instanceof Error) { + expect(caught.message).toContain("malformed JSON"); + expect(caught.cause).toBeInstanceOf(SyntaxError); + } + expect(() => normalizeOxlintOutput(JSON.stringify({ output: [] }))).toThrow( + "diagnostics array", + ); + }); +}); diff --git a/scripts/tests/review-infrastructure.test.ts b/scripts/tests/review-infrastructure.test.ts new file mode 100644 index 00000000..9f982c69 --- /dev/null +++ b/scripts/tests/review-infrastructure.test.ts @@ -0,0 +1,646 @@ +import { describe, it } from "@executablemd/test-support/bdd"; +import { expect } from "@executablemd/test-support/expect"; +import { FetchApi, type FetchResponse } from "@effectionx/fetch"; +import { readTextFile } from "@effectionx/fs"; +import { InMemoryStream } from "@executablemd/durable-streams"; +import { API } from "@executablemd/runtime"; +import { useStubFs } from "@executablemd/runtime/test"; +import { execute } from "../../packages/core/src/execute.ts"; +import { useTempFileCompiler } from "../../packages/core/src/temp-file-compiler.ts"; +import { forEach } from "@effectionx/stream-helpers"; +import { scoped, until } from "effection"; +import type { Operation } from "effection"; + +interface RequestRecord { + input: string; + init?: RequestInit; + shouldExpect: boolean; +} + +interface ProcessResult { + exitCode: number; + stdout: string; + stderr: string; +} + +interface DocumentRunOptions { + componentDirs?: string[]; + env?: Record; + glob?: Array<{ path: string; isFile: boolean; isDirectory: boolean }>; + stat?: (path: string) => { exists: boolean; isFile: boolean; isDirectory: boolean } | undefined; + process?: ProcessResult | ((command: readonly string[]) => ProcessResult); +} + +function response(body: string): FetchResponse { + const raw = new Response(body, { status: 200 }); + return { + raw, + get bodyUsed() { + return raw.bodyUsed; + }, + get ok() { + return raw.ok; + }, + get status() { + return raw.status; + }, + get statusText() { + return raw.statusText; + }, + get headers() { + return raw.headers; + }, + get url() { + return raw.url; + }, + get redirected() { + return raw.redirected; + }, + get type() { + return raw.type; + }, + *json(parse?: (value: unknown) => T): Operation { + const value: unknown = JSON.parse(body); + return parse ? parse(value) : (value as T); + }, + *text(): Operation { + return body; + }, + *arrayBuffer(): Operation { + return yield* until(raw.arrayBuffer()); + }, + *blob(): Operation { + return yield* until(raw.blob()); + }, + *formData(): Operation { + return yield* until(raw.formData()); + }, + body() { + throw new Error("body streaming is not used by this test"); + }, + *expect(): Operation { + return this; + }, + }; +} + +function* run( + provider: string, + token?: string, +): Operation<{ requests: RequestRecord[]; journal: string }> { + const requests: RequestRecord[] = []; + const files: Record = { + "doc.md": [ + "", + "", + '', + '', + '', + "", + "", + '', + ].join("\n"), + "components/GitHubAuth.md": provider, + "components/Probe.md": [ + "---", + "props:", + " type: object", + " properties:", + " label: { type: string }", + " url: { type: string }", + " required: [label, url]", + " additionalProperties: false", + "---", + "", + "```ts eval", + "const value = yield* fetch(props.url, { method: 'POST', headers: { 'X-Caller': 'kept', 'Content-Type': 'application/custom' }, body: 'request-body' }).expect().json();", + "```", + "", + "{props.label}={value.message}", + ].join("\n"), + "components/Wrapper.md": '', + "components/NoExpect.md": [ + "```ts eval", + 'yield* fetch("https://api.github.com/repos/no-expect");', + "```", + ].join("\n"), + }; + + yield* useStubFs(files); + yield* API.Env.around({ + *env([name], next) { + if (name === "GITHUB_TOKEN") { + return token; + } + return yield* next(name); + }, + }); + yield* FetchApi.around( + { + *fetch([input, init, shouldExpect]) { + const request = + input instanceof Request ? input.url : input instanceof URL ? input.href : input; + requests.push({ input: request, init, shouldExpect }); + return response(JSON.stringify({ message: "ok" })); + }, + }, + { at: "min" }, + ); + + const stream = new InMemoryStream(); + const execution = yield* execute({ path: "doc.md", stream, componentDirs: ["components"] }); + yield* forEach(function* () {}, execution.output); + const result = yield* execution; + expect(result.ok).toBe(true); + return { requests, journal: JSON.stringify(stream.snapshot()) }; +} + +function* runDocumentResult( + document: string, + components: Record, + options: DocumentRunOptions = {}, +): Operation<{ ok: boolean; journal: string }> { + return yield* scoped(function* () { + const environment = options.env; + if (environment) { + yield* API.Env.around({ + *env([name], next) { + return name in environment ? environment[name] : yield* next(name); + }, + }); + } + const stat = options.stat; + const entries = options.glob; + if (stat || entries) { + yield* API.Fs.around({ + *stat([path], next) { + const value = stat?.(path); + return value ?? (yield* next(path)); + }, + *glob([parameters], next) { + return entries ?? (yield* next(parameters)); + }, + }); + } + yield* useStubFs({ "doc.md": document, ...components }); + const process = options.process; + if (process) { + yield* API.Process.around({ + *exec([parameters]) { + return typeof process === "function" ? process(parameters.command) : process; + }, + }); + } + const stream = new InMemoryStream(); + const execution = yield* execute({ + path: "doc.md", + stream, + componentDirs: options.componentDirs ?? ["components"], + }); + yield* forEach(function* () {}, execution.output); + const result = yield* execution; + return { ok: result.ok, journal: JSON.stringify(stream.snapshot()) }; + }); +} + +function* runDocument( + document: string, + components: Record, + options: DocumentRunOptions = {}, +): Operation { + const result = yield* runDocumentResult(document, components, options); + return result.ok; +} + +describe("review infrastructure", () => { + it("uses the real GitHubAuth component for exact-host scoped middleware", function* () { + yield* useTempFileCompiler(); + const provider = yield* readTextFile(".reviews/components/GitHubAuth.md"); + const result = yield* run(provider, "secret-token"); + + expect(result.requests).toHaveLength(6); + expect(result.requests[0].input).toBe("https://api.github.com/repos/nested"); + expect(result.requests[0].init?.headers).toBeInstanceOf(Headers); + const nestedHeaders = new Headers(result.requests[0].init?.headers); + expect(nestedHeaders.get("Authorization")).toBe("Bearer secret-token"); + expect(nestedHeaders.get("Accept")).toBe("application/vnd.github+json"); + expect(nestedHeaders.get("X-Caller")).toBe("kept"); + expect(result.requests[0].init?.body).toBe("request-body"); + expect(result.requests[0].shouldExpect).toBe(true); + expect(new Headers(result.requests[1].init?.headers).get("Authorization")).toBe(null); + expect(new Headers(result.requests[2].init?.headers).get("Authorization")).toBe(null); + expect(new Headers(result.requests[3].init?.headers).get("Authorization")).toBe(null); + expect(new Headers(result.requests[4].init?.headers).get("Authorization")).toBe( + "Bearer secret-token", + ); + expect(result.requests[4].shouldExpect).toBe(false); + expect(new Headers(result.requests[5].init?.headers).get("Authorization")).toBe(null); + expect(result.journal).not.toContain("secret-token"); + }); + + it("delegates unchanged when the token is unavailable and restores scope", function* () { + yield* useTempFileCompiler(); + const provider = yield* readTextFile(".reviews/components/GitHubAuth.md"); + const result = yield* run(provider); + + for (const request of result.requests) { + expect(new Headers(request.init?.headers).get("Authorization")).toBe(null); + if (request.init?.body !== undefined) { + expect(request.init.body).toBe("request-body"); + } + } + expect(result.journal).not.toContain("secret-token"); + }); + + it("fails on malformed and unexpected Oxlint results, but skips empty input", function* () { + yield* useTempFileCompiler(); + + const valid = yield* runDocumentResult( + '\n```ts eval\nconst summary = `${diagnostics.length}:${diagnostics[0].ruleId}`;\n```\n{summary}', + { ".reviews/components/OxlintDiagnostics.ts": "" }, + { + componentDirs: [".reviews/components"], + process: { + exitCode: 1, + stdout: JSON.stringify([ + { + message: "unused", + code: "eslint(no-unused-vars)", + severity: "warning", + filename: "a.ts", + labels: [{ span: { line: 2, column: 3 } }], + source: "raw source must not cross the boundary", + cause: { secret: "unbounded payload" }, + }, + ]), + stderr: "", + }, + }, + ); + expect(valid.ok).toBe(true); + expect(valid.journal).toContain("1:no-unused-vars"); + expect(valid.journal).not.toContain("raw source must not cross the boundary"); + expect(valid.journal).not.toContain("unbounded payload"); + + expect( + yield* runDocument( + '', + { ".reviews/components/OxlintDiagnostics.ts": "" }, + { + componentDirs: [".reviews/components"], + process: { exitCode: 0, stdout: "not json", stderr: "" }, + }, + ), + ).toBe(false); + expect( + yield* runDocument( + '', + { ".reviews/components/OxlintDiagnostics.ts": "" }, + { + componentDirs: [".reviews/components"], + process: { exitCode: 2, stdout: "[]", stderr: "invocation failed" }, + }, + ), + ).toBe(false); + + let calls = 0; + yield* API.Process.around({ + *exec() { + calls++; + return { exitCode: 2, stdout: "", stderr: "must not run" }; + }, + }); + expect( + yield* runDocument( + '', + { + ".reviews/components/OxlintDiagnostics.ts": "", + }, + { componentDirs: [".reviews/components"] }, + ), + ).toBe(true); + expect(calls).toBe(0); + }); + + it("extracts human comment replies and rejects malformed GitHub comments", function* () { + yield* useTempFileCompiler(); + const comments = [ + { + id: 100, + user: { login: "github-actions[bot]", type: "Bot" }, + body: "Redundant comment: this repeats the code", + path: "src/example.ts", + original_line: 4, + diff_hunk: "@@\n+const value = 1;", + }, + { + id: 200, + in_reply_to_id: 100, + user: { login: "human", type: "User" }, + body: "Please keep this comment", + }, + { + id: 201, + in_reply_to_id: 100, + user: { login: "acknowledged", type: "User" }, + body: "Remove it", + }, + { + id: 202, + in_reply_to_id: 100, + user: { login: "automation", type: "Bot" }, + body: "Bot reply must not be classified", + }, + ]; + let malformed = false; + yield* FetchApi.around({ + *fetch([input]) { + const url = + input instanceof Request ? input.url : input instanceof URL ? input.href : input; + if (url.includes("/pulls/1/comments?")) { + return response(malformed ? JSON.stringify({ comments }) : JSON.stringify(comments)); + } + if (url.includes("/comments/201/reactions")) { + return response( + JSON.stringify([ + { user: { login: "github-actions[bot]", type: "Bot" }, content: "+1" }, + ]), + ); + } + return response("[]"); + }, + }); + + const document = [ + "", + "```ts eval", + "const pr = { added: [{ content: '// note', file: 'src/example.ts', lineNumber: 4, isTest: false }] };", + "```", + '', + "```ts eval", + "const summary = `${data.previousFindings.length}:${data.repliesForClassification.length}:${data.dismissedReplies.length}`;", + "```", + "{summary}", + "{data.repliesText}", + "", + ].join("\n"); + const options = { + componentDirs: [".reviews/components"], + env: { + GITHUB_TOKEN: "test-token", + GITHUB_REPOSITORY: "taras/executable.md", + PR_NUMBER: "1", + }, + }; + const result = yield* runDocumentResult( + document, + { ".reviews/components/CommentReviewData.ts": "" }, + options, + ); + expect(result.ok).toBe(true); + expect(result.journal).toContain("1:1:1"); + expect(result.journal).toContain("Please keep this comment"); + expect(result.journal).not.toContain("Bot reply must not be classified"); + + malformed = true; + const malformedResult = yield* runDocumentResult( + document, + { ".reviews/components/CommentReviewData.ts": "" }, + options, + ); + expect(malformedResult.ok).toBe(false); + }); + + it("parses CommentReviewState classifications and durable checklist state", function* () { + yield* useTempFileCompiler(); + const document = [ + "", + "```ts eval", + "const pr = { added: [{ file: 'new.ts', lineNumber: 3 }] };", + "const data = {", + " pairs: [{ comment: 'redundant', code: 'const value = 1', file: 'new.ts', lineNumber: 3 }],", + " previousFindings: [{ file: 'old.ts', lineNumber: 1 }],", + " dismissedReplies: [],", + " repliesForClassification: [{ file: 'new.ts', lineNumber: 2, replyText: 'keep it', replyId: 7 }],", + "};", + "```", + '', + "```ts eval", + "const summary = `${state.hasChecklist}:${state.hasFindings}:${state.newDismissReplies.length}`;", + "```", + "{summary}", + "{state.checklistMd}", + "", + ].join("\n"); + const result = yield* runDocumentResult( + document, + { ".reviews/components/CommentReviewState.ts": "" }, + { componentDirs: [".reviews/components"] }, + ); + expect(result.ok).toBe(true); + expect(result.journal).toContain("true:true:1"); + expect(result.journal).toContain("old.ts:1"); + expect(result.journal).toContain("keep it"); + }); + + it("covers Doctor availability, probes, crashes, and failures", function* () { + yield* useTempFileCompiler(); + const document = [ + "", + "```ts eval", + "const pr = {};", + "```", + '', + "```ts eval", + "const summary = `${doctor.typeAwareAvailable}:${doctor.recommendation}`;", + "```", + "{summary}", + "", + ].join("\n"); + const component = { ".reviews/components/Doctor.ts": "" }; + const stats = (available: boolean) => (path: string) => { + if (path === ".reviews/.oxlint/oxlint" || path === ".reviews/.oxlint/tsgolint") { + return { exists: available, isFile: available, isDirectory: false }; + } + if (path === ".reviews/tsconfig.oxlint.json") { + return { exists: available, isFile: available, isDirectory: false }; + } + if (path === "node_modules") { + return { exists: available, isFile: false, isDirectory: available }; + } + return undefined; + }; + const baseOptions = { componentDirs: [".reviews/components"], glob: [] }; + + const unavailable = yield* runDocumentResult(document, component, { + ...baseOptions, + stat: stats(false), + }); + expect(unavailable.ok).toBe(true); + expect(unavailable.journal).toContain("false:syntax-only"); + + const successful = yield* runDocumentResult(document, component, { + ...baseOptions, + stat: stats(true), + process: (command) => { + if (command.at(-1) === "--version") { + return { exitCode: 0, stdout: `${command[0]} 1.0\n`, stderr: "" }; + } + return { + exitCode: 1, + stdout: JSON.stringify([ + { + message: "unused", + code: "no-unused-vars", + severity: "warning", + filename: "a.ts", + labels: [{ span: { line: 1, column: 1 } }], + }, + ]), + stderr: "", + }; + }, + }); + expect(successful.ok).toBe(true); + expect(successful.journal).toContain("true:type-aware"); + + const crash = yield* runDocumentResult(document, component, { + ...baseOptions, + stat: stats(true), + process: (command) => + command.at(-1) === "--version" + ? { exitCode: 0, stdout: "tool 1.0\n", stderr: "" } + : { exitCode: 1, stdout: "", stderr: "tsgolint panic: OOM" }, + }); + expect(crash.ok).toBe(true); + expect(crash.journal).toContain("false:syntax-only"); + + const malformed = yield* runDocumentResult(document, component, { + ...baseOptions, + stat: stats(true), + process: (command) => + command.at(-1) === "--version" + ? { exitCode: 0, stdout: "tool 1.0\n", stderr: "" } + : { exitCode: 0, stdout: "not json", stderr: "" }, + }); + expect(malformed.ok).toBe(false); + + const failed = yield* runDocumentResult(document, component, { + ...baseOptions, + stat: stats(true), + process: { exitCode: 2, stdout: "[]", stderr: "invocation failed" }, + }); + expect(failed.ok).toBe(false); + }); + + it("constructs ReviewContext from git and fails on git errors", function* () { + yield* useTempFileCompiler(); + const document = [ + "", + '', + "```ts eval", + "const summary = `${context.changedFilePaths.length}:${context.pr.files[0].path}`;", + "```", + "{summary}", + "", + ].join("\n"); + const diff = [ + "diff --git a/src/example.ts b/src/example.ts", + "new file mode 100644", + "--- /dev/null", + "+++ b/src/example.ts", + "@@ -0,0 +1,1 @@", + "+const value = 1;", + ].join("\n"); + const result = yield* runDocumentResult( + document, + { ".reviews/components/ReviewContext.ts": "" }, + { + componentDirs: [".reviews/components"], + env: { BASE_SHA: "base", HEAD_SHA: "head", PR_BODY: "local body" }, + process: (command) => + command.includes("--name-status") + ? { exitCode: 0, stdout: "A\tsrc/example.ts\n", stderr: "" } + : { exitCode: 0, stdout: diff, stderr: "" }, + }, + ); + expect(result.ok).toBe(true); + expect(result.journal).toContain("1:src/example.ts"); + + const failed = yield* runDocumentResult( + document, + { ".reviews/components/ReviewContext.ts": "" }, + { + componentDirs: [".reviews/components"], + process: { exitCode: 1, stdout: "", stderr: "git diff failed" }, + }, + ); + expect(failed.ok).toBe(false); + }); + + it("fails when a provider returns 2xx without model content", function* () { + yield* useTempFileCompiler(); + const deepInfra = yield* readTextFile(".reviews/components/DeepInfraProvider.md"); + const ollama = yield* readTextFile(".reviews/components/OllamaProvider.md"); + const sample = yield* readTextFile("packages/core/components/Sample.md"); + const requests: string[] = []; + yield* FetchApi.around({ + *fetch([input]) { + requests.push( + input instanceof Request ? input.url : input instanceof URL ? input.href : input, + ); + return response(JSON.stringify({ choices: [] })); + }, + }); + + expect( + yield* runDocument( + '', + { + "components/DeepInfraProvider.md": deepInfra, + "components/Sample.md": sample, + }, + { env: { DEEPINFRA_TOKEN: "test-token" } }, + ), + ).toBe(false); + expect(requests).toHaveLength(1); + expect(requests[0]).toBe("https://api.deepinfra.com/v1/openai/chat/completions"); + expect( + yield* runDocument( + '', + { + "components/OllamaProvider.md": ollama, + "components/Sample.md": sample, + }, + ), + ).toBe(false); + expect(requests).toHaveLength(2); + expect(requests[1]).toBe("http://localhost:11434/v1/chat/completions"); + }); + + it("requires GitHubComment metadata", function* () { + yield* useTempFileCompiler(); + const comment = yield* readTextFile(".reviews/components/GitHubComment.md"); + expect( + yield* runDocument("finding", { + "components/GitHubComment.md": comment, + }), + ).toBe(false); + }); + + it("executes the real RepositoryInventory function component", function* () { + yield* useTempFileCompiler(); + const inventoryResult = yield* runDocumentResult( + '\n{inventory.fileCount}:{inventory.lineCount}', + { + ".reviews/components/RepositoryInventory.ts": "", + "packages/example.ts": "first\nsecond\n", + }, + { + componentDirs: [".reviews/components"], + glob: [{ path: "packages/example.ts", isFile: true, isDirectory: false }], + }, + ); + expect(inventoryResult.ok).toBe(true); + }); +}); diff --git a/specs/code-review-agent-spec.md b/specs/code-review-agent-spec.md index aac4620c..6312d1d6 100644 --- a/specs/code-review-agent-spec.md +++ b/specs/code-review-agent-spec.md @@ -7,16 +7,18 @@ ## 1. Architecture -A PR review is an executable markdown document. The document gathers -the diff, parses it into a structured object, passes it through -composable check components, optionally sends it to an LLM for -semantic analysis, and posts the rendered output as a GitHub comment. +A PR review is an executable Markdown document. Typed function components +gather the diff, environment observations, and bounded diagnostics. Markdown +then composes the policy components, model provider, and optional GitHub +delivery. ``` ReviewPR.md - โ”œโ”€ Capture: git diff โ†’ rawDiff - โ”œโ”€ Capture: git diff --name-status โ†’ rawFiles - โ”œโ”€ eval: parseDiff(rawDiff, rawFiles) โ†’ pr + โ”œโ”€ โ†’ execution failures fail the document + โ”œโ”€ โ†’ scoped exact-host GitHub authentication + โ”œโ”€ ReviewContext โ†’ git diff and PR metadata โ†’ pr + โ”œโ”€ Doctor โ†’ bounded environment recommendation + โ”œโ”€ OxlintDiagnostics โ†’ normalized diagnostics โ”‚ โ””โ”€ DeepInfraProvider (or OllamaProvider) โ””โ”€ Instructions (system prompt) @@ -39,7 +41,7 @@ ReviewPR.md โ””โ”€ SemanticReview โ†’ Sample ``` -Three layers of concern, three layers of middleware: +The review composition has three layers of concern: | Layer | Component | Responsibility | |---|---|---| @@ -77,8 +79,12 @@ All executable.md core changes and the full agent implementation are complete: ## 3. Package: `@executablemd/code-review-agent` -One export: `parseDiff`. Takes raw `git diff` and `git diff --name-status` -output, returns a typed `PR` object. +The package exports the published diff, diagnostics, Oxlint normalization, and +Doctor parsing helpers. `parseDiff` takes raw `git diff` and +`git diff --name-status` output and returns a typed `PR` object. Existing +`parseDoctorResult` and tolerant `parseDiagnostics` behavior remains stable; +strict Oxlint validation belongs to `normalizeOxlintOutput` at the review +component boundary. ### 3.1 `PR` type @@ -92,7 +98,7 @@ interface PR { deleted: DiffFile[]; directories: Set; addedSource: string; - diffPreview: string; // addedSource truncated to 40K chars + diffPreview: string; // addedSource truncated to 80K chars stats: { totalFiles: number; additions: number; @@ -149,8 +155,7 @@ function parseDiff( - Test file detection: `*.test.ts`, `*.spec.ts`, `__tests__/`, `test/` - Config file detection: `*.config.*`, `.*rc`, `tsconfig*`, `package.json` - Type declaration detection: `*.d.ts` -- `diffPreview`: `addedSource` truncated to 40,000 characters so the - correctness prompt stays within the provider context limit on large PRs +- `diffPreview`: `addedSource` truncated to 80,000 characters - `directories`: unique top-level dirs at depth 2 ### 3.4 Package structure @@ -254,145 +259,36 @@ scope.around(Sample, function* ([context], next) { ### 4.4 `GitHubComment.md` -````markdown ---- -props: - type: object - properties: - marker: - type: string - default: "" - additionalProperties: false ---- - -```ts eval -const content = yield* renderChildren(); -const body = marker + "\n" + content; - -const token = process.env.GITHUB_TOKEN; -const repo = process.env.GITHUB_REPOSITORY; -const prNumber = process.env.PR_NUMBER; -const [owner, name] = repo.split("/"); -const api = `https://api.github.com/repos/${owner}/${name}`; - -const headers = { - "Authorization": `Bearer ${token}`, - "Accept": "application/vnd.github+json", -}; - -const { json: comments } = yield* fetch( - `${api}/issues/${prNumber}/comments`, { headers } -).expect(); - -const existing = comments.find(c => - c.user.type === "Bot" && c.body.includes(marker) -); - -if (existing) { - yield* fetch(`${api}/issues/comments/${existing.id}`, { - method: "PATCH", - headers: { ...headers, "Content-Type": "application/json" }, - body: JSON.stringify({ body }), - }).expect(); -} else { - yield* fetch(`${api}/issues/${prNumber}/comments`, { - method: "POST", - headers: { ...headers, "Content-Type": "application/json" }, - body: JSON.stringify({ body }), - }).expect(); -} - -return content; -``` -```` +CI roots place this component inside `` and ``. `GitHubAuth` +reads `GITHUB_TOKEN` once in a private generator closure and installs +`@effectionx/fetch`'s `FetchApi` middleware around its projected content. It +adds authorization only to HTTPS requests whose exact hostname is +`api.github.com`, preserves request headers and bodies, and forwards the +`shouldExpect` argument unchanged. Missing credentials and non-GitHub requests +delegate unchanged. `GitHubComment` therefore keeps only request-specific +headers such as `Content-Type`; it requires repository and PR metadata instead +of silently returning an empty report. + +`GitHubComment.md` renders the report, reads the configured repository and PR +metadata through contextual environment access, and uses the fluent +`fetch().expect()` operations to create or update the marked comment. It +requires its metadata and validates the comments payload. Authentication is +inherited from the surrounding `GitHubAuth` provider; the component supplies +only request-specific headers such as `Content-Type`. ### 4.5 `DeepInfraProvider.md` -````markdown ---- -props: - type: object - properties: - model: - type: string - required: [model] - additionalProperties: false ---- - -```ts persist eval -const scope = yield* useScope(); -scope.around(Sample, function* ([context], next) { - if (context.model !== undefined && context.model !== model) { - return yield* next(context); - } - - const messages = []; - if (context.system) { - messages.push({ role: "system", content: context.system }); - } - messages.push({ role: "user", content: context.content }); - - const result = yield* fetch("https://api.deepinfra.com/v1/openai/chat/completions", { - method: "POST", - headers: { - "Content-Type": "application/json", - "Authorization": `Bearer ${process.env.DEEPINFRA_TOKEN}`, - }, - body: JSON.stringify({ model, messages, temperature: 0, max_tokens: 4096 }), - }) - .expect() - .json(); - - return result.choices[0].message.content; -}); -``` - - -```` +`DeepInfraProvider` is a Markdown provider for the `Sample` API. It keeps +request construction and response validation in its scoped middleware. The +token is read through contextual environment access and stays in the private +request operation; a successful 2xx response without model content fails the +provider. ### 4.6 `OllamaProvider.md` -````markdown ---- -props: - type: object - properties: - model: - type: string - baseUrl: - type: string - default: "http://localhost:11434" - required: [model] - additionalProperties: false ---- - -```ts persist eval -const scope = yield* useScope(); -scope.around(Sample, function* ([context], next) { - if (context.model !== undefined && context.model !== model) { - return yield* next(context); - } - - const messages = []; - if (context.system) { - messages.push({ role: "system", content: context.system }); - } - messages.push({ role: "user", content: context.content }); - - const result = yield* fetch(`${baseUrl}/v1/chat/completions`, { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ model, messages, temperature: 0 }), - }) - .expect() - .json(); - - return result.choices[0].message.content; -}); -``` - - -```` +`OllamaProvider` uses the same `Sample` provider contract with its configured +local base URL. It validates that a successful response contains model +content and otherwise fails the enclosing document. --- @@ -779,55 +675,29 @@ const triggered = touchesPkg && !mentionsDeps; ### 5.10 `CommentReview.md` -````markdown ---- -props: - type: object - properties: - pr: - type: object - required: [pr] - additionalProperties: false ---- - -```ts eval -const pairs = []; -const lines = pr.added.filter(l => !l.isTest); - -for (let i = 0; i < lines.length - 1; i++) { - const current = lines[i].content.trim(); - const next = lines[i + 1].content.trim(); - if (current.startsWith("//") && !next.startsWith("//") && next.length > 0) { - pairs.push({ comment: current, code: next }); - } -} +`CommentReview.md` keeps the authored ``, ``, and `` +composition. `CommentReviewData.ts` performs pair extraction and bounded +GitHub response parsing, while `CommentReviewState.ts` parses model responses +and constructs the checklist and pending findings. Both are typed function +components with explicit schemas; no procedural eval block crosses the +Markdown boundary. -const hasPairs = pairs.length >= 3; -const pairsText = hasPairs - ? pairs.slice(0, 20).map(p => - `COMMENT: ${p.comment}\nCODE: ${p.code}` - ).join("\n---\n") - : ""; +```markdown + + + + {reviewData.pairsText} + + + ``` - - - - -Review these comment/code pairs. List ONLY obvious/redundant ones -where the comment restates what the code does. - -Format: "- `` โ€” restates ``" - -If none are obvious: "No obvious comments found." - -{pairsText} - - - - -```` - --- ## 6. Policy Documents (zero JavaScript) @@ -1043,180 +913,49 @@ Zero eval blocks. ## 7. Entry Points -### 7.1 `.reviews/ReviewPR.md` (CI with DeepInfra) - -````markdown ---- -title: PR Review ---- +The CI and local roots keep workflow composition in Markdown. Both use +`ReviewSetup`, `ReviewContext`, `Doctor`, and `OxlintDiagnostics`; the CI root +places the complete review inside `` and wraps it in `GitHubAuth`. +The local root uses the same composition without `GitHubComment`. +```markdown - -```ts eval -const BASE_SHA = process.env.BASE_SHA ?? "HEAD~1"; -const HEAD_SHA = process.env.HEAD_SHA ?? "HEAD"; -``` - - - -```bash exec -git diff {BASE_SHA}...{HEAD_SHA} -``` - - - - - -```bash exec -git diff --name-status {BASE_SHA}...{HEAD_SHA} -``` - - - + + + + + ```ts eval -import { parseDiff } from "@executablemd/code-review-agent"; - -const pr = parseDiff(rawDiff, rawFiles, { - title: process.env.PR_TITLE ?? "", - body: process.env.PR_BODY ?? "", - number: process.env.PR_NUMBER ?? "", -}); +import { buildDiagnostics } from "@executablemd/code-review-agent"; +const diagnostics = buildDiagnostics(rawDiagnostics, context.pr, doctor); ``` - - - - - - + + + - + -```` - -### 7.2 `.reviews/ReviewPR.local.md` (local with Ollama) - -````markdown ---- -title: PR Review (local) ---- - -```ts eval -const BASE_SHA = process.env.BASE_SHA ?? "HEAD~1"; -const HEAD_SHA = process.env.HEAD_SHA ?? "HEAD"; -``` - - - -```bash exec -git diff {BASE_SHA}...{HEAD_SHA} -``` - - - - - -```bash exec -git diff --name-status {BASE_SHA}...{HEAD_SHA} -``` - - - -```ts eval -import { parseDiff } from "@executablemd/code-review-agent"; - -const pr = parseDiff(rawDiff, rawFiles, { - title: process.env.PR_TITLE ?? "", - body: process.env.PR_BODY ?? "", - number: process.env.PR_NUMBER ?? "", -}); ``` - - - - - -```` - -Output goes to stdout. No `` wrapper. - ---- +The two eval blocks in the checked-in roots only select bounded values and +adapt package results. Git, GitHub, Oxlint execution, configuration, and +normalization live in typed function components or package modules. ## 8. CI Workflow -### `.github/workflows/review.yml` - -```yaml -name: PR Review -on: - pull_request: - types: [opened, synchronize, reopened] - -jobs: - review: - runs-on: ubuntu-latest - permissions: - pull-requests: write - contents: read - steps: - - uses: actions/checkout@v4 - with: { fetch-depth: 0 } - - - uses: denoland/setup-deno@v2 - - - name: Install dependencies - run: deno task deps - - - name: Build the checked-out xmd binary - run: deno task build - - - name: Run review - env: - PR_NUMBER: ${{ github.event.pull_request.number }} - PR_TITLE: ${{ github.event.pull_request.title }} - BASE_SHA: ${{ github.event.pull_request.base.sha }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - GITHUB_REPOSITORY: ${{ github.repository }} - DEEPINFRA_TOKEN: ${{ secrets.DEEPINFRA_TOKEN }} - run: | - ./dist/xmd run .reviews/ReviewPR.md \ - --component-dir .reviews/components \ - --component-dir .reviews/policies \ - --component-dir packages/core/components \ - -j .reviews/journal.jsonl \ - --verbose -``` - -The executable body of the CI root is enclosed in one top-level `` -region. An execution failure therefore remains a failed `DocumentResult`, -fails `execute()`, and makes `xmd run` exit nonzero. Review findings are report -text and do not fail the run. The workflow does not inspect journal records or -rendered error markers after the command; it only uploads the journal with -`if: always()`. - -### Journal artifact - -The journal is uploaded by head SHA after every run, including failed runs, so -an execution failure remains inspectable without making the artifact a second -workflow result channel. - -### Durable-boundary safety - -Doctor compatibility probes emit aggregate facts only: diagnostic count, -import-noise count or ratio, file counts, crash state, and available rule IDs. -Actual Oxlint output is normalized before it enters a capture or durable event -to `message`, `ruleId` or `code`, `severity`, `file`, and the minimal line and -column span. PR review keeps diagnostics for changed files; repository analysis -may keep all files. Source excerpts, causes, rendered source, URLs, and other -arbitrary payload are discarded. - -GitHub credentials are created inside non-serializable header factories at the -request site. Tokens are not placed in eval bindings or durable output, and -secret detection remains enabled. - ---- +The review workflow checks out the requested revision, installs the pinned +Deno toolchain, runs `deno task setup`, and executes that checkout's +`./dist/xmd` binary with the review component directories. Credentials stay +in the workflow environment and are consumed by the scoped `GitHubAuth` +provider. The CI root uses `` so execution errors fail the CLI while +ordinary review findings remain successful report text. The journal is +uploaded under `if: always()`; Actions does not parse journal records or +rendered error markers. ## 9. Deterministic Analysis (separate CI jobs, unchanged) @@ -1258,7 +997,9 @@ These block merges. The executable.md review is advisory. ConfigSourceMix.md Config + source mixing AbstractionNames.md Suspicious file names NewDependencies.md Dependency justification - CommentReview.md Pair extraction + LLM review + CommentReview.md Prompt composition for comment review + CommentReviewData.ts Pair extraction + GitHub response parsing + CommentReviewState.ts Model-response and checklist state # Policy documents (zero JavaScript) ScopeCheck.md Composes Threshold, Finding checks @@ -1289,18 +1030,20 @@ These block merges. The executable.md review is advisory. | `ConfigSourceMix.md` | 1 | File classification for `` | | `AbstractionNames.md` | 1 | Name pattern for `` | | `NewDependencies.md` | 1 | Dependency check for `` | -| `CommentReview.md` | 1 | Pair extraction | +| `CommentReview.md` | 0 | Prompt and capture composition | +| `CommentReviewData.ts` | 0 | Pair extraction and GitHub response parsing | +| `CommentReviewState.ts` | 0 | Model-response and checklist state | | **`ScopeCheck.md`** | **0** | | | **`StructuralBloat.md`** | **0** | | | **`VerbosityCheck.md`** | **0** | | | **`SemanticReview.md`** | **0** | | | **`ReviewBody.md`** | **0** | | -| `ReviewPR.md` | 2 | Env vars + parseDiff | -| `ReviewPR.local.md` | 2 | Env vars + parseDiff | +| `ReviewPR.md` | 2 | Changed-file selection + diagnostic grouping | +| `ReviewPR.local.md` | 2 | Changed-file selection + diagnostic grouping | -17 eval blocks across 17 reusable components. 5 policy documents -and `ReviewBody` have zero. The documents a team edits day-to-day -contain no JavaScript. +The authored Markdown keeps the review prompts and policy composition. The +procedural components are typed TypeScript modules, while only short binding +adapters remain in the two root documents. --- @@ -1329,29 +1072,32 @@ output. All rules at `"warn"` โ€” Oxlint collects signals, not verdicts. ### 13.1 Sensor configuration -`.reviews/.oxlintrc.json` โ€” committed config with `pedantic: "warn"` -and `style: "warn"` enabled (14 bloat-relevant rules). All oxlint -invocations in capture blocks reference this config via -`--config .reviews/.oxlintrc.json`. +`.reviews/.oxlintrc.json` โ€” committed review-sensor config with +`pedantic: "warn"` and `style: "warn"` enabled. All oxlint invocations +reference this config via `--config .reviews/.oxlintrc.json`. The sensor +turns off rule families that conflict with repository conventions: component +filename casing, named-export and export-order rules, generator/function +style rules, and mechanical import/key-order and magic-number rules (including +`sort-imports`, `sort-keys`, and `no-magic-numbers`). The normal repository +lint configuration remains authoritative for those rules; this +review-only exclusion prevents the advisory report from treating required +component structure as a finding. -### 13.2 Environment detection (`Doctor.md`) +### 13.2 Environment detection (`Doctor.ts`) The Doctor component probes the environment before oxlint runs: oxlint binary, tsgolint binary, `node_modules/`, tsconfig, scheme specifier scan (`jsr:`, `npm:`), and a type-aware test run. Outputs a recommendation: `type-aware`, `type-aware-filtered`, or -`syntax-only`. Includes prose narration for local visibility. -Its JSON output is wrapped in a `` ```json `` code fence and -extracted via `` (see executable.md spec -ยง6.5), isolating the structured data from surrounding narration. +`syntax-only`. The typed function component returns the bounded Doctor +object directly; raw process output never becomes a document binding. ### 13.3 PR-scoped analysis -PR entry points (`ReviewPR.md`, `ReviewPR.local.md`) scope oxlint -to changed `.ts`/`.tsx` files only via `git diff --name-only` + -`xargs`. Density against `pr.stats.additions` is only meaningful -when diagnostics come from the same files the additions are in. -Repo analysis entry points run on everything. +PR entry points (`ReviewPR.md`, `ReviewPR.local.md`) pass the parsed changed +`.ts`/`.tsx` paths to the typed `OxlintDiagnostics` component. Density against +`pr.stats.additions` is only meaningful when diagnostics come from the same +files the additions are in. Repo analysis entry points run on everything. ### 13.4 Density calibration @@ -1422,3 +1168,36 @@ Local runs skip issue creation (no `GITHUB_TOKEN` โ†’ return empty). lint-plugins/ no-scheme-specifiers.ts Deno lint plugin ``` + +## 14. Current review-infrastructure boundary + +The CI entrypoints are declarative compositions. Their executable shape is: + +```markdown + + + + + + + + + +``` + +`Doctor`, `ReviewContext`, `RepositoryInventory`, and `OxlintDiagnostics` are +typed function components. They use contextual runtime operations directly; +their props and return schemas stay explicit at the module boundary. Git and +GitHub calls use argument-array `exec` and the fluent +`fetch().expect().json()` operations. The code-review-agent package owns diff, +Oxlint normalization, Doctor classification, and structured result +construction. Raw process output, response objects, and credentials remain +inside generator-local variables; only bounded values become document +bindings. Markdown remains responsible for the visible composition and +provider hierarchy. + +The review workflow runs `deno task setup` and then `./dist/xmd`, so the binary +and executable Markdown are from the same checkout. The two CI roots use +`` error mode: execution failures return a failed document result and a +nonzero CLI exit, while ordinary finding text remains successful output. The +journal is uploaded with `if: always()` and is not parsed by Actions. diff --git a/specs/oxlint-sensor-spec.md b/specs/oxlint-sensor-spec.md index 92c67747..4d948bd4 100644 --- a/specs/oxlint-sensor-spec.md +++ b/specs/oxlint-sensor-spec.md @@ -9,10 +9,11 @@ enforcement for Deno/tsgo interoperability. ## 1. Concept -Oxlint runs inside a `Capture` block alongside `git diff`. Its -JSON output becomes a structured input to the review pipeline. The -LLM receives both the diff and the diagnostic map, interpreting -the density and pattern of violations rather than individual hits. +Oxlint runs through the typed `OxlintDiagnostics` component after +`ReviewContext` has constructed the diff. Its bounded diagnostic value becomes +a structured input to the review pipeline. The LLM receives both the diff and +the diagnostic map, interpreting the density and pattern of violations rather +than individual hits. Oxlint runs permissively โ€” all bloat-relevant rules enabled at `"warn"`, zero rules at `"error"`. It collects signals, not @@ -31,20 +32,18 @@ in the same files suggest unreviewed generated code. ``` ReviewPR.md - โ”œโ”€ Capture: git diff โ†’ rawDiff - โ”œโ”€ Capture: git diff --name-status โ†’ rawFiles - โ”œโ”€ eval: parseDiff(rawDiff, rawFiles) โ†’ pr - โ”œโ”€ silent exec: generate tsconfig - โ”œโ”€ Doctor (as="doctorJson") + โ”œโ”€ โ†’ execution failures fail the document + โ”œโ”€ โ†’ scoped exact-host GitHub authentication + โ”œโ”€ ReviewContext โ†’ git diff and PR metadata โ†’ pr + โ”œโ”€ Doctor (as="doctor") โ”‚ โ”œโ”€ Check: oxlint binary โ”‚ โ”œโ”€ Check: tsgolint binary โ”‚ โ”œโ”€ Check: node_modules/ โ”‚ โ”œโ”€ Check: tsconfig โ”‚ โ”œโ”€ Scan: scheme specifiers (jsr:, npm:) โ”‚ โ””โ”€ Probe: type-aware test run - โ”œโ”€ eval: parseDoctorResult โ†’ doctor - โ”œโ”€ Capture: oxlint (mode per doctor) โ†’ rawDiagnostics - โ”œโ”€ eval: parseDiagnostics โ†’ diagnostics + โ”œโ”€ OxlintDiagnostics (as="rawDiagnostics") + โ”œโ”€ eval: buildDiagnostics(rawDiagnostics) โ†’ diagnostics โ”‚ โ””โ”€ DeepInfraProvider (or OllamaProvider) โ””โ”€ Instructions @@ -61,8 +60,8 @@ Three layers of concern: | Layer | What | Components | |---|---|---| -| Environment | Can Oxlint run? How much of it? | `Doctor.md`, tsconfig generation | -| Collection | Run Oxlint, parse output | `Capture`, `parseDiagnostics` | +| Environment | Can Oxlint run? How much of it? | `Doctor.ts`, `ReviewSetup` | +| Collection | Run Oxlint, normalize output | `OxlintDiagnostics`, `buildDiagnostics` | | Interpretation | LLM reads signals + diff | `SemanticReview`, `OxlintSummary` | --- @@ -253,8 +252,8 @@ is `"error"`. Oxlint always exits 0. "categories": { "correctness": "warn", "suspicious": "warn", - "pedantic": "off", - "style": "off" + "pedantic": "warn", + "style": "warn" }, "rules": { @@ -276,6 +275,18 @@ is `"error"`. Oxlint always exits 0. "eslint/no-console": ["warn", { "allow": ["warn", "error"] }], "eslint/no-debugger": "warn", + "eslint/func-style": "off", + "eslint/no-magic-numbers": "off", + "eslint/require-yield": "off", + "eslint/sort-keys": "off", + "eslint/sort-imports": "off", + "import/exports-last": "off", + "import/group-exports": "off", + "import/no-named-export": "off", + "import/prefer-default-export": "off", + "import/consistent-type-specifier-style": "off", + "unicorn/filename-case": "off", + "typescript/no-unnecessary-type-arguments": "warn", "typescript/no-unnecessary-type-assertion": "warn", "typescript/no-redundant-type-constituents": "warn", @@ -296,7 +307,13 @@ is `"error"`. Oxlint always exits 0. ### 4.2 Rule catalog -14 bloat-relevant rules, split into two groups by whether they +The sensor collects the bloat-relevant rules that are compatible with the +repository's component and generator conventions. It excludes filename-case, +named-export, export-order, function-style, generator-yield, import-order, +key-order, and magic-number rules from its advisory report. These exclusions apply only to +the review sensor; the normal lint gate remains unchanged. + +The remaining bloat-relevant rules are split into two groups by whether they require type information: **Syntax-only (10 rules, always available):** @@ -379,6 +396,12 @@ function parseDiagnostics( pr: PR, doctor: DoctorResult, ): Diagnostics; + +function buildDiagnostics( + diagnostics: OxlintDiagnostic[], + pr: PR, + doctor: DoctorResult, +): Diagnostics; ``` **Rule categorization:** @@ -393,9 +416,15 @@ function parseDiagnostics( Rules may appear in multiple categories. **Noise filtering:** When `doctor.recommendation === "type-aware-filtered"`, -`parseDiagnostics` drops diagnostics matching import resolution +`buildDiagnostics` drops diagnostics matching import resolution noise (`"Cannot find module"`, `"cannot find"`) before grouping. +`parseDiagnostics` retains the published tolerant parser contract: malformed +JSON and unsupported JSON shapes produce an empty `Diagnostics` value. The +review execution path uses `normalizeOxlintOutput` first; that boundary is +strict and fails malformed output or an unexpected process exit before a +document binding is created. + **Coverage annotation:** When `doctor.bloatRulesMissing.length > 0`, summary appends: @@ -425,7 +454,18 @@ Density: 0.12 violations/added-line no-redundant-type-constituents (1): src/types.ts ``` -### 5.3 `DoctorResult` type +### 5.3 `parseDoctorResult` + +```typescript +function parseDoctorResult(json: string): DoctorResult; +``` + +`parseDoctorResult` remains a published tolerant parser. It applies defaults +to missing fields and returns the default Doctor value for malformed or +non-object JSON. The typed `Doctor` component returns its structured result +directly and does not serialize it for this parser. + +### 5.4 `DoctorResult` type ```typescript interface DoctorResult { @@ -439,6 +479,7 @@ interface DoctorResult { filesAnalyzed: number; filesSkipped: number; importErrors: number; + availableRuleIds: string[]; bloatRulesAvailable: string[]; bloatRulesMissing: string[]; recommendation: "type-aware" | "type-aware-filtered" | "syntax-only"; @@ -451,13 +492,21 @@ interface DoctorResult { } ``` -### 5.4 `parseDoctorResult` +### 5.5 Doctor helpers ```typescript -function parseDoctorResult(json: string): DoctorResult; +function summarizeDoctorProbe(input: DoctorProbeInput): DoctorProbeSummary; +function buildDoctorResult( + environment: DoctorEnvironment, + probe: DoctorProbeSummary, +): DoctorResult; ``` -### 5.5 Package structure +These helpers accept only the bounded diagnostic representation. The typed +`Doctor` function component keeps process output local and returns the +validated Doctor object directly. + +### 5.6 Package structure ``` packages/code-review-agent/ @@ -465,6 +514,8 @@ packages/code-review-agent/ parse-diff.ts parse-diagnostics.ts parse-doctor.ts + parse-oxlint.ts + doctor.ts categories.ts types.ts mod.ts @@ -474,27 +525,28 @@ packages/code-review-agent/ ## 6. Components -### 6.1 `Doctor.md` +### 6.1 `Doctor.ts` Compatibility analysis for the Oxlint static analysis sensor. Probes the environment to determine which Oxlint capabilities are available, scans for import specifier compatibility issues, and -recommends a run mode. The result is a JSON string consumed by -`parseDoctorResult`. +recommends a run mode. The typed function component returns the bounded +Doctor object directly. -All shell checks are `exec` blocks captured into bindings. On -replay, stored results are returned from the journal โ€” no commands -re-run. +Contextual `stat`, `exec`, `glob`, and `readTextFile` operations provide the +observations. Pure parsing and classification live in +`@executablemd/code-review-agent`; raw process output does not become a +document binding. **Checks performed:** | Check | What | Why | |---|---|---| -| Oxlint binary | `npx oxlint --version` | May not be installed. Without it, all static analysis signals are unavailable. | -| tsgolint binary | `npx oxlint-tsgolint --version` | Separate Go binary for type-aware linting via typescript-go. Without it, 4 type-dependent rules are unavailable. | -| `node_modules/` | `test -d node_modules` | tsgolint resolves imports through Node module resolution. Created by `deno install` when `nodeModulesDir: "auto"`. | -| Generated tsconfig | `test -f {tsconfigPath}` | tsgolint requires tsconfig. Generated by the workflow in `.reviews/`. | -| Scheme specifiers | `grep` for `jsr:` and `npm:` in source | These break tsgo resolution. Doctor reports them and explains the fix. | +| Oxlint binary | contextual `exec --version` | May not be installed. Without it, all static analysis signals are unavailable. | +| tsgolint binary | contextual `exec --version` | Separate Go binary for type-aware linting via typescript-go. Without it, 4 type-dependent rules are unavailable. | +| `node_modules/` | contextual `stat` | tsgolint resolves imports through Node module resolution. Created by `deno task setup`. | +| Generated tsconfig | contextual `stat` | tsgolint requires tsconfig. Generated by the typed `OxlintConfig` component. | +| Scheme specifiers | `glob` + `readTextFile` | These break tsgo resolution. Doctor reports them and explains the fix. | | Type-aware probe | Full `oxlint --type-aware` run | Measures what actually works โ€” noise ratio, crash detection, file coverage. | **Recommendations:** @@ -505,327 +557,12 @@ re-run. | `"type-aware-filtered"` | Type-aware works but noise โ‰ฅ 30%. Run type-aware, filter import noise in `parseDiagnostics`. | | `"syntax-only"` | Prerequisites missing or probe crashed. 10 syntax-only rules, 4 type-aware missing. | -````markdown ---- -props: - type: object - properties: - pr: - type: object - tsconfigPath: - type: string - default: ".reviews/tsconfig.oxlint.json" - required: [pr] - additionalProperties: false ---- - -### Oxlint Compatibility Check - -Checking whether Oxlint and its type-aware backend are available -in this environment. - -**Oxlint binary:** - - - -```bash silent exec -npx oxlint --version 2>/dev/null || echo "NOT_INSTALLED" -``` - - - -`{oxlintVersion}` - -**tsgolint binary** (type-aware backend โ€” uses typescript-go for -full TypeScript type system access): - - - -```bash silent exec -npx oxlint-tsgolint --version 2>/dev/null || echo "NOT_INSTALLED" -``` - - - -`{tsgolintVersion}` - -**node_modules/** (required by tsgolint for import resolution โ€” -created by `deno install` when `nodeModulesDir: "auto"` is set -in `deno.json`): - - - -```bash silent exec -test -d node_modules && echo "EXISTS" || echo "MISSING" -``` - - - -`{nodeModulesCheck}` - -**Generated tsconfig** at `{tsconfigPath}` (required by tsgolint -to build TypeScript programs โ€” generated by the review workflow, -not committed to the repo): - - - -```bash silent exec -test -f {tsconfigPath} && echo "EXISTS" || echo "MISSING" -``` - - - -`{tsconfigCheck}` - -```ts eval -const oxlintInstalled = !oxlintVersion.includes("NOT_INSTALLED"); -const tsgolintInstalled = !tsgolintVersion.includes("NOT_INSTALLED"); -const nodeModulesExists = nodeModulesCheck.trim() === "EXISTS"; -const tsconfigExists = tsconfigCheck.trim() === "EXISTS"; - -const canProbeTypeAware = oxlintInstalled && tsgolintInstalled - && nodeModulesExists && tsconfigExists; -``` - -**Import specifier compatibility.** Oxlint's type-aware backend -uses typescript-go, which resolves imports through standard Node -module resolution. Deno-native scheme specifiers โ€” `jsr:`, `npm:` -โ€” in source files are invisible to this resolver and produce -"Cannot find module" noise. - -The fix is to use bare specifiers in source and map them in -`deno.json` `imports`. Both Deno and typescript-go resolve bare -specifiers โ€” Deno through the import map, tsgo through -`node_modules/`. - - - -```bash silent exec -grep -rn --include='*.ts' --include='*.tsx' -E '^\s*(import|export)\s.*from\s+['"'"'"](jsr:|npm:)' packages/ src/ 2>/dev/null | head -50 || echo "NONE" -``` - - - -```ts eval -const hasNativeSpecifiers = specifierScan.trim() !== "NONE" - && specifierScan.trim().length > 0; - -const specifierLines = hasNativeSpecifiers - ? specifierScan.trim().split("\n") : []; - -const specifierFiles = [...new Set( - specifierLines.map(l => l.split(":")[0]).filter(Boolean) -)]; - -const jsrCount = specifierLines.filter(l => l.includes("jsr:")).length; -const npmCount = specifierLines.filter(l => l.includes("npm:")).length; -``` - - - -Found **{specifierLines.length}** scheme specifiers across -**{specifierFiles.length}** files ({jsrCount} `jsr:`, -{npmCount} `npm:`). These will produce import noise in -type-aware mode. - -To fix, add entries to `deno.json` `imports` and use bare -specifiers in source: - -``` -// Before (source file): -import { assertEquals } from "jsr:@std/assert"; -import express from "npm:express@4"; - -// After (deno.json imports): -{ "@std/assert": "jsr:@std/assert", "express": "npm:express@4" } - -// After (source file): -import { assertEquals } from "@std/assert"; -import express from "express"; -``` - -To enforce this going forward, add the `no-scheme-specifiers` -lint plugin to `deno.json`: - -```json -{ - "lint": { - "plugins": ["./lint-plugins/no-scheme-specifiers.ts"] - } -} -``` - -Then run `deno lint --fix` to auto-replace specifiers. Add the -`deno.json` `imports` entries manually. - -Files with scheme specifiers: - -{specifierFiles.slice(0, 20).map(f => "- `" + f + "`").join("\n")} - - 20}> - -...and {specifierFiles.length - 20} more. - - - - - - - -No scheme specifiers found in source files. All imports use bare -specifiers โ€” compatible with both Deno and typescript-go. - - - -**Type-aware probe.** All four prerequisites must pass before -attempting a type-aware run. Even then, the probe may fail โ€” tsgo -can't resolve scheme specifiers that Deno handles natively, -tsgolint may OOM on very large monorepos, or the generated -tsconfig's include globs may not match the actual source tree. - - - -Skipping type-aware probe โ€” prerequisites not met. - - - - - - - -```bash silent exec -RESULT=$(npx oxlint --type-aware --tsconfig {tsconfigPath} --format json 2>/dev/null || true) -printf '%s' "$RESULT" | jq -c ' - def entries: - if type == "array" then . - elif (.diagnostics? | type) == "array" then .diagnostics - else [] - end; - def rule: if (.ruleId? | type) == "string" then .ruleId elif (.code? | type) == "string" then .code else "unknown" end; - entries | map({message: (.message // ""), ruleId: rule, file: (.file // .filename // "")}) as $diagnostics - | {diagnosticCount: ($diagnostics | length), importNoiseCount: ([$diagnostics[] | select((.message | ascii_downcase | contains("cannot find module")) or (.ruleId | ascii_downcase | contains("import")))] | length), filesAnalyzed: ([$diagnostics[].file | select(length > 0)] | unique | length), filesSkipped: 0, importErrors: ([$diagnostics[] | select((.message | ascii_downcase | contains("cannot find module")) or (.ruleId | ascii_downcase | contains("import")))] | length), availableRuleIds: ([$diagnostics[].ruleId] | unique), tsgolintCrashed: false} -' || echo '{"diagnosticCount":0,"importNoiseCount":0,"filesAnalyzed":0,"filesSkipped":0,"importErrors":0,"availableRuleIds":[],"tsgolintCrashed":false}' -``` - - - -{"diagnosticCount":0,"importNoiseCount":0,"filesAnalyzed":0,"filesSkipped":0,"importErrors":0,"availableRuleIds":[],"tsgolintCrashed":false} - - - - - - -Analyzing probe results. When source files use Deno-native -scheme specifiers, tsgo can't resolve them and emits "Cannot find -module" diagnostics. These aren't code quality signals โ€” they're -environment incompatibilities. If more than 30% of diagnostics are -import noise, the signal-to-noise ratio is too low for reliable -density calculations. - -```ts eval -const BLOAT_RULES = [ - "no-unused-vars", "no-inferrable-types", "no-empty-function", - "no-empty-object-type", "no-useless-empty-export", - "no-unnecessary-type-constraint", - "no-unnecessary-parameter-property-assignment", - "no-static-only-class", "no-console", "no-debugger", - "no-unnecessary-type-assertion", "no-redundant-type-constituents", - "no-unnecessary-type-arguments", - "no-unnecessary-boolean-literal-compare", -]; -const TYPE_AWARE_RULES = [ - "no-unnecessary-type-assertion", "no-redundant-type-constituents", - "no-unnecessary-type-arguments", - "no-unnecessary-boolean-literal-compare", -]; - -let probe = { - diagnosticCount: 0, - importNoiseCount: 0, - filesAnalyzed: 0, - filesSkipped: 0, - importErrors: 0, - availableRuleIds: [], - tsgolintCrashed: false, -}; -try { probe = { ...probe, ...JSON.parse(probeResult) }; } catch { } - -const diagnosticCount = typeof probe.diagnosticCount === "number" - ? probe.diagnosticCount : 0; -const importNoiseCount = typeof probe.importNoiseCount === "number" - ? probe.importNoiseCount : 0; -const noiseRatio = diagnosticCount > 0 - ? importNoiseCount / diagnosticCount : 0; -const tsgolintCrashed = probe.tsgolintCrashed === true; - -const typeAwareAvailable = canProbeTypeAware && !tsgolintCrashed; - -let recommendation = "syntax-only"; -if (typeAwareAvailable && noiseRatio < 0.3) { - recommendation = "type-aware"; -} else if (typeAwareAvailable && noiseRatio >= 0.3) { - recommendation = "type-aware-filtered"; -} - -const bloatRulesAvailable = typeAwareAvailable - ? BLOAT_RULES - : BLOAT_RULES.filter(r => !TYPE_AWARE_RULES.includes(r)); -const bloatRulesMissing = typeAwareAvailable - ? [] - : TYPE_AWARE_RULES; - -const doctor = { - oxlintInstalled, - oxlintVersion: oxlintVersion.trim(), - tsgolintInstalled, - tsgolintVersion: tsgolintVersion.trim(), - tsconfigExists, - nodeModulesExists, - typeAwareAvailable, - filesAnalyzed: typeof probe.filesAnalyzed === "number" ? probe.filesAnalyzed : 0, - filesSkipped: typeof probe.filesSkipped === "number" ? probe.filesSkipped : 0, - importErrors: typeof probe.importErrors === "number" ? probe.importErrors : 0, - availableRuleIds: Array.isArray(probe.availableRuleIds) ? probe.availableRuleIds : [], - bloatRulesAvailable, - bloatRulesMissing, - recommendation, - nativeSpecifiers: { - count: hasNativeSpecifiers ? specifierLines.length : 0, - files: specifierFiles, - jsr: jsrCount, - npm: npmCount, - }, -}; - -return JSON.stringify(doctor); -``` - -**Result:** {recommendation} - - - -Type-aware linting available. {bloatRulesAvailable.length} bloat -rules active across {probe.filesAnalyzed} files. -Import noise: {probe.importNoiseCount} diagnostics -({(noiseRatio * 100).toFixed(1)}%). - - - - - -Falling back to syntax-only mode. {bloatRulesAvailable.length} -bloat rules active, {bloatRulesMissing.length} type-aware rules -unavailable. - - - - - -Oxlint not installed. Static analysis signals unavailable. - - -```` +Doctor is the production typed function component. It obtains observations +through contextual `stat`, `exec`, `glob`, and `readTextFile` operations, +passes bounded diagnostics to the package helpers, and returns a structured +Doctor result. A missing prerequisite or recognized type-aware crash selects +`syntax-only`; malformed output and unexpected process exits fail the +enclosing ``. ### 6.2 `OxlintSignals.md` @@ -1064,291 +801,54 @@ DIFF: ### 8.1 `ReviewPR.md` (CI with DeepInfra) -````markdown ---- -title: PR Review ---- +The production entrypoint keeps the workflow hierarchy in Markdown. The +contextual components own Git and GitHub I/O, while root eval blocks only +select files and adapt bounded package values: +```markdown - -```ts eval -const BASE_SHA = process.env.BASE_SHA ?? "HEAD~1"; -const HEAD_SHA = process.env.HEAD_SHA ?? "HEAD"; -``` - - - -```bash exec -git diff {BASE_SHA}...{HEAD_SHA} -``` - - - - - -```bash exec -git diff --name-status {BASE_SHA}...{HEAD_SHA} -``` - - - -```ts eval -import { parseDiff } from "@executablemd/code-review-agent"; - -const pr = parseDiff(rawDiff, rawFiles, { - title: process.env.PR_TITLE ?? "", - body: process.env.PR_BODY ?? "", - number: process.env.PR_NUMBER ?? "", -}); -``` - -```bash silent exec -mkdir -p .reviews -cat > .reviews/tsconfig.oxlint.json << 'TSCONFIG' -{ - "compilerOptions": { - "target": "ESNext", - "module": "ESNext", - "moduleResolution": "bundler", - "strict": true, - "noEmit": true, - "skipLibCheck": true, - "resolveJsonModule": true, - "lib": ["ESNext", "DOM"], - "types": [] - }, - "include": ["packages/*/src/**/*.ts", "packages/*/*.ts"], - "exclude": ["node_modules", "dist", ".vendor", "**/*.test.ts"] -} -TSCONFIG -``` - - - -```ts eval -import { parseDoctorResult } from "@executablemd/code-review-agent"; - -const doctor = parseDoctorResult(doctorJson); -``` - - - - - -```bash exec -npx oxlint --type-aware --tsconfig .reviews/tsconfig.oxlint.json --format json 2>&1 || true -``` - - - - - -```bash exec -npx oxlint --format json 2>&1 || true -``` - - - - - -```ts eval -import { parseDiagnostics } from "@executablemd/code-review-agent"; - -const diagnostics = parseDiagnostics(rawDiagnostics, pr, doctor); -``` - - - - - - - - - + + + + + + + + + + + -```` - -### 8.2 `ReviewPR.local.md` (local with Ollama) - -Same structure, different provider, no `` wrapper. - -````markdown ---- -title: PR Review (local) ---- - -```ts eval -const BASE_SHA = process.env.BASE_SHA ?? "HEAD~1"; -const HEAD_SHA = process.env.HEAD_SHA ?? "HEAD"; -``` - - - -```bash exec -git diff {BASE_SHA}...{HEAD_SHA} -``` - - - - - -```bash exec -git diff --name-status {BASE_SHA}...{HEAD_SHA} -``` - - - -```ts eval -import { parseDiff } from "@executablemd/code-review-agent"; - -const pr = parseDiff(rawDiff, rawFiles, { - title: process.env.PR_TITLE ?? "", - body: process.env.PR_BODY ?? "", - number: process.env.PR_NUMBER ?? "", -}); -``` - -```bash silent exec -mkdir -p .reviews -cat > .reviews/tsconfig.oxlint.json << 'TSCONFIG' -{ - "compilerOptions": { - "target": "ESNext", - "module": "ESNext", - "moduleResolution": "bundler", - "strict": true, - "noEmit": true, - "skipLibCheck": true, - "resolveJsonModule": true, - "lib": ["ESNext", "DOM"], - "types": [] - }, - "include": ["packages/*/src/**/*.ts", "packages/*/*.ts"], - "exclude": ["node_modules", "dist", ".vendor", "**/*.test.ts"] -} -TSCONFIG ``` - +The root keeps only short eval adapters: it selects changed TypeScript +paths and calls `buildDiagnostics` on the bounded return from +`OxlintDiagnostics`. Git, configuration, Oxlint execution, and normalization +remain in the typed components. `DeepInfraProvider` and `GitHubComment` stay +inside the same `` scope. -```ts eval -import { parseDoctorResult } from "@executablemd/code-review-agent"; - -const doctor = parseDoctorResult(doctorJson); -``` - - - - - -```bash exec -npx oxlint --type-aware --tsconfig .reviews/tsconfig.oxlint.json --format json 2>&1 || true -``` - - - - - -```bash exec -npx oxlint --format json 2>&1 || true -``` - - - - - -```ts eval -import { parseDiagnostics } from "@executablemd/code-review-agent"; - -const diagnostics = parseDiagnostics(rawDiagnostics, pr, doctor); -``` +### 8.2 `ReviewPR.local.md` (local with Ollama) - - - - - -```` +The local entrypoint has the same component composition and replaces +`DeepInfraProvider`/`GitHubComment` with `OllamaProvider`. Its root eval +adapters select files and build the bounded diagnostics value; no shell +capture or raw Oxlint JSON crosses the document boundary. --- ## 9. CI Workflow -### `.github/workflows/review.yml` - -```yaml -name: PR Review -on: - pull_request: - types: [opened, synchronize, reopened] - -jobs: - review: - runs-on: ubuntu-latest - permissions: - pull-requests: write - contents: read - steps: - - uses: actions/checkout@v4 - with: { fetch-depth: 0 } - - - uses: denoland/setup-deno@v2 - - - name: Install dependencies - run: deno task deps - - - name: Build the checked-out xmd binary - run: deno task build - - - name: Run review - env: - PR_NUMBER: ${{ github.event.pull_request.number }} - PR_TITLE: ${{ github.event.pull_request.title }} - BASE_SHA: ${{ github.event.pull_request.base.sha }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - GITHUB_REPOSITORY: ${{ github.repository }} - DEEPINFRA_TOKEN: ${{ secrets.DEEPINFRA_TOKEN }} - run: | - ./dist/xmd run .reviews/ReviewPR.md \ - --component-dir .reviews/components \ - --component-dir .reviews/policies \ - --component-dir packages/core/components \ - -j .reviews/journal.jsonl \ - --verbose - - - name: Upload journal - if: always() - uses: actions/upload-artifact@v4 - with: - name: review-journal-${{ github.event.pull_request.head.sha }} - path: .reviews/journal.jsonl - retention-days: 30 -``` - -The workflow builds and runs the checked-out `./dist/xmd` after -`deno task deps`, so the review documents and executable use one revision. -The root document encloses its executable body in ``; execution -failures therefore exit nonzero through the normal `DocumentResult` path. -Review finding text remains ordinary report output and does not fail the job. - -The workflow uploads the journal with `if: always()` and does not interpret -journal records or rendered Markdown after XMD exits. - -### Durable diagnostic boundary - -The Doctor compatibility probe emits only aggregate facts: availability or -crash state, diagnostic and import-noise counts, file counts, and available -rule identifiers. Actual Oxlint output is normalized before capture to -`message`, `ruleId` or `code`, `severity`, `file`, and minimal line/column -information. PR review restricts diagnostics to changed files; repository -analysis retains all files. Source excerpts, causes, rendered source, URLs, -and arbitrary payload do not cross the exec/durable boundary. - -Credential-bearing headers are created inside non-serializable functions at -request time. Default secret detection remains enabled. +The review workflow checks out the requested revision, installs the pinned +Deno toolchain, runs `deno task setup`, and executes that checkout's +`./dist/xmd` binary. It passes credentials through the workflow environment +to the lexically scoped `GitHubAuth` component. The root is inside +``, so execution failures produce a nonzero CLI exit while ordinary +review findings remain report text. The journal is uploaded with `if: always()` +and Actions performs no journal or rendered-output postflight parsing. ### Separate enforcement jobs (unchanged from base spec) @@ -1438,7 +938,10 @@ false positive and false negative rates. ReviewBody.md Updated (+ diagnostics, doctor) # New components (this spec) - Doctor.md Compatibility analysis + Doctor.ts Compatibility analysis + ReviewContext.ts PR context and GitHub body + RepositoryInventory.ts Repository file inventory + OxlintDiagnostics.ts Bounded Oxlint execution OxlintSignals.md Per-category signal list OxlintSummary.md Diagnostic summary + doctor status @@ -1451,7 +954,8 @@ packages/code-review-agent/ src/ parse-diff.ts Unchanged parse-diagnostics.ts New - parse-doctor.ts New + doctor.ts New + parse-oxlint.ts New categories.ts New types.ts Updated mod.ts @@ -1463,16 +967,21 @@ packages/code-review-agent/ | Document | Eval blocks | Change | |---|---|---| -| `Doctor.md` | 4 | **New** (prerequisites, specifier scan, probe analysis, doctor result) | +| `Doctor.ts` | 0 | Typed contextual probe and bounded result | +| `ReviewContext.ts` | 0 | PR diff and metadata acquisition | +| `RepositoryInventory.ts` | 0 | File and line inventory | +| `OxlintDiagnostics.ts` | 0 | Bounded Oxlint execution | | `OxlintSignals.md` | 1 | **New** | | `OxlintSummary.md` | 0 | **New** | -| `ReviewPR.md` | 4 | Modified (added tsconfig gen, parseDoctorResult, parseDiagnostics) | -| `ReviewPR.local.md` | 4 | Modified (same as above) | +| `ReviewPR.md` | 2 | Modified (composition and binding adapters) | +| `ReviewPR.local.md` | 2 | Modified (same as above) | | `ReviewBody.md` | 0 | Modified (added diagnostics, doctor props) | | `StructuralBloat.md` | 0 | Modified (added OxlintSignals) | | `SemanticReview.md` | 0 | Modified (added diagnostics to prompt) | -Net new eval blocks: 5 (4 in Doctor, 1 in OxlintSignals). +The new contextual components contain no Markdown eval blocks. Root documents +retain only short binding adapters; prompt and policy composition remains +Markdown. --- @@ -1524,7 +1033,7 @@ Net new eval blocks: 5 (4 in Doctor, 1 in OxlintSignals). | PD5 | Annotates missing rules | Summary includes missing rule note | | PD6 | Annotates scheme specifiers | Summary includes migration note | | PD7 | Empty input | `total: 0`, `density: 0`, clean summary | -| PD8 | Malformed JSON | Graceful fallback to empty diagnostics | +| PD8 | Malformed JSON | The diagnostic boundary fails rather than hiding an invocation error | ### Integration @@ -1548,3 +1057,30 @@ Net new eval blocks: 5 (4 in Doctor, 1 in OxlintSignals). | Cross-file unused exports | Requires project-wide graph | Knip CI job (base spec ยง9) | | Effection correctness | `yield` vs `yield*`, `async function*` | Separate correctness policy | | `https://` URL imports | Different problem from scheme specifiers | Deno's built-in `no-external-import` rule | + +## 16. Current document boundary + +The production sensor keeps Markdown responsible for composition and uses +typed function components for contextual I/O: + +```markdown + + + +``` + +`Doctor` uses `stat`, `exec`, `glob`, and `readTextFile`; its parsing, +classification, import-noise aggregation, and result construction live in +`@executablemd/code-review-agent`. `OxlintDiagnostics` invokes Oxlint only +when its file list is nonempty and passes stdout through +`normalizeOxlintOutput`, which retains only `message`, `ruleId`, `severity`, +`file`, `line`, and `column`. It filters changed files in that package. +`buildDiagnostics` consumes that structured result without serializing it back +to JSON. A malformed JSON result, crash, or unexpected invocation exit fails the enclosing +`` instead of becoming an empty diagnostic list. A valid diagnostic +exit may contain zero diagnostics. + +The workflow provisions the checked-out binary through `deno task setup` and +executes `./dist/xmd`. It relies on the CLI exit status, keeps the journal +artifact under `if: always()`, and performs no journal-result or rendered-error +postflight parsing. diff --git a/specs/release-process-spec.md b/specs/release-process-spec.md index 1d478011..9e6218c8 100644 --- a/specs/release-process-spec.md +++ b/specs/release-process-spec.md @@ -71,9 +71,11 @@ they already satisfy the lockfile. Left alone they keep the previous release's number. Only the members whose `name` is an `@executablemd` package change; an unrelated dependency that happens to share the old version number must not. -The bump touches nothing else. PR Review and Repo Analysis build the checked-out -revision with the repository-pinned Deno version, so their executable documents -run against the source they review rather than against a published binary. +The bump touches nothing else. PR Review and Repo Analysis prepare and build +the checked-out revision with `deno task setup` and `deno task build`, then run +`./dist/xmd`. They do not +install the latest published release, so a review always understands the +documents at the revision it checks. ## 3. Workflows @@ -84,19 +86,6 @@ run against the source they review rather than against a published binary. version is already released, the draft's notes carry a warning banner saying the manifests need bumping; once bumped, the banner clears and the draft's tag and title default to the guard-passing `v`. -- **`review.yml`** (`pull_request`): checks out the pull request, installs the - repository-pinned Deno version, runs `deno task deps`, and runs `deno task - build`. It executes the resulting `./dist/xmd` against `.reviews/ReviewPR.md`. - The root review body is enclosed in a top-level `` region, so an - execution failure propagates through `DocumentResult` and makes the command - fail. Review findings remain ordinary report text and do not fail execution. - The workflow uploads `.reviews/journal.jsonl` with `if: always()` and does - not parse journal records or rendered error markers after XMD exits. -- **`repo-analysis.yml`** (`workflow_dispatch`): checks out the requested ref, - prepares and builds that ref's `./dist/xmd`, and executes - `.reviews/AnalyzeRepoCI.md`, whose root body uses the same `` failure - contract. Its report, journal, and run metadata remain unconditional - artifacts. - **`release.yml`** (`push: tags v*`): a preflight job validates the tag against `packages/cli/deno.json`; on mismatch it flags the just-published release on the Releases page โ€” caution note in the notes, a failed title, and the @@ -107,6 +96,12 @@ run against the source they review rather than against a published binary. sha256 checksums to the tag's GitHub Release. That module is the compiled-binary entrypoint: it installs the `API.Env.command` adapter that relaunches the binary as itself, which a source entrypoint cannot do. +- **`review.yml`** and **`repo-analysis.yml`**: install the repository-pinned + Deno and pnpm actions, run `deno task setup` and `deno task build`, and + execute the checked-out `./dist/xmd` against the checked-out Markdown. Their CI roots use `` + error mode, so execution failures fail the workflow through the CLI exit + status. Journals and reports are uploaded with `if: always()`; Actions does + not interpret journal records or rendered error markers. - **`publish-packages.yml`** (`push: tags v*`): GENERATED by `scripts/gen-publish-workflow.md` โ€” an executable markdown document that expands the root `workspace` entries (including one-level globs such as