From fceb19434c70c022717546d2611f9c3bb93cc780 Mon Sep 17 00:00:00 2001 From: dbrosio3 Date: Wed, 1 Jul 2026 09:55:46 -0300 Subject: [PATCH] Refactor path policy to enhance changed file metrics and streamline selection functions --- bin/pushgate.mjs | 43 +++++++++++++++++++++--------------- docs/architecture/modules.md | 6 ++--- src/ai/guardrails.ts | 19 +++++----------- src/ai/review-context.ts | 16 +++++++------- src/path-policy/filtering.ts | 38 +++++++++++++++++++++++++++---- src/path-policy/index.ts | 5 +++++ src/runner/policies.ts | 13 ++++++----- test/ai.test.ts | 7 ------ test/path-policy.test.ts | 26 ++++++++++++++++++++++ 9 files changed, 113 insertions(+), 60 deletions(-) diff --git a/bin/pushgate.mjs b/bin/pushgate.mjs index 9054896..0c72d70 100755 --- a/bin/pushgate.mjs +++ b/bin/pushgate.mjs @@ -9495,8 +9495,25 @@ function filterIgnoredChangedFiles(files, ignorePaths) { const ignorePathsMatcher = (0, import_ignore.default)().add(ignorePaths); return files.filter((file2) => !ignorePathsMatcher.ignores(file2.path)); } +function countChangedTextLines(files) { + return files.reduce((total, file2) => { + if (file2.binary) { + return total; + } + return total + (file2.additions ?? 0) + (file2.deletions ?? 0); + }, 0); +} +function isLiveChangedFile(file2) { + return file2.status !== "deleted"; +} +function selectLiveChangedFiles(files) { + return files.filter(isLiveChangedFile); +} +function selectLiveChangedFilePaths(files, options = {}) { + return selectLiveChangedFiles(files).filter((file2) => matchesExtension(file2.path, options.extensions)).map((file2) => file2.path); +} function selectToolChangedFilePaths(files, extensions) { - return files.filter((file2) => file2.status !== "deleted").filter((file2) => matchesExtension(file2.path, extensions)).map((file2) => file2.path); + return selectLiveChangedFilePaths(files, { extensions }); } function matchesExtension(path, extensions) { if (extensions === void 0) { @@ -10193,7 +10210,7 @@ function evaluateChangedFileGuardrails(options) { if (options.changedFiles.length === 0) { return { kind: "skip-no-files" }; } - const changedLineCount = countChangedLines(options.changedFiles); + const changedLineCount = countChangedTextLines(options.changedFiles); if (changedLineCount > options.maxChangedLines) { return { kind: "block-changed-lines", @@ -10220,14 +10237,6 @@ function evaluatePromptGuardrail(options) { estimatedPromptTokens }; } -function countChangedLines(changedFiles) { - return changedFiles.reduce((total, file2) => { - if (file2.binary) { - return total; - } - return total + (file2.additions ?? 0) + (file2.deletions ?? 0); - }, 0); -} function estimatePromptTokens(prompt) { if (prompt.length === 0) { return 0; @@ -26539,7 +26548,10 @@ async function collectLocalAiReviewContext(options) { repoRoot: options.repoRoot }); const diffLineCount = countTextLines(diff); - const fullFiles = diffLineCount < options.reviewConfig.max_lines_for_full_file ? await collectFullFiles(options.repoRoot, changedFiles) : []; + const fullFiles = diffLineCount < options.reviewConfig.max_lines_for_full_file ? await collectFullFiles( + options.repoRoot, + selectLiveChangedFiles(changedFiles) + ) : []; return { changedFiles, diff, @@ -26579,9 +26591,6 @@ async function collectReviewDiff(options) { async function collectFullFiles(repoRoot, changedFiles) { const fullFiles = []; for (const file2 of changedFiles) { - if (file2.status === "deleted") { - continue; - } if (file2.binary) { fullFiles.push({ path: file2.path, @@ -27074,9 +27083,7 @@ function runBuiltInPolicies(policies, changedFiles) { return results; } function runDiffSizePolicy(policy, changedFiles) { - const changedLines = changedFiles.reduce((total, file2) => { - return total + (file2.additions ?? 0) + (file2.deletions ?? 0); - }, 0); + const changedLines = countChangedTextLines(changedFiles); if (changedLines <= policy.max_changed_lines) { return { name: "policy:diff_size", @@ -27095,7 +27102,7 @@ function runDiffSizePolicy(policy, changedFiles) { ); } function runForbiddenPathsPolicy(policy, changedFiles) { - const matches = changedFiles.filter((file2) => file2.status !== "deleted").flatMap((file2) => { + const matches = selectLiveChangedFiles(changedFiles).flatMap((file2) => { const pattern = firstMatchingPattern(policy.patterns, file2.path); return pattern ? [{ path: file2.path, pattern }] : []; }); diff --git a/docs/architecture/modules.md b/docs/architecture/modules.md index 17371cd..48b7d0c 100644 --- a/docs/architecture/modules.md +++ b/docs/architecture/modules.md @@ -13,7 +13,7 @@ know. | CLI | `main(argv, io)` and `pushgate` subcommands | `src/cli.ts` | Public command surface for hook use. | | Pre-push workflow | `runPrePushWorkflow(io)` | `src/workflows/pre-push.ts`, `src/workflows/local-push-gate-run.ts`, `src/workflows/pre-push-hook-context.ts` | Owns pre-push context, phase order, and warning confirmation. | | Config | `loadConfig`, `parseConfigYaml`, `PushgateConfig` | `src/config/*`, `schemas/pushgate-config-v2.schema.json` | Converts user YAML into one normalized internal shape. | -| Path policy | `resolveChangedFiles`, `selectToolChangedFilePaths` | `src/path-policy/*`, `src/git/*` | Owns Git range, diff parsing, ignore, and live-path semantics. | +| Path policy | `resolveChangedFiles`, changed-file projections | `src/path-policy/*`, `src/git/*` | Owns Git range, diff parsing, ignore, live-path semantics, and changed-line metrics. | | Process execution | `runCommand`, `runTimedCommand`, `runProcessOutcome`, `runInheritedCommand` | `src/process/*` | Shared child-process mechanics and outcome formatting. | | Deterministic runner | `runDeterministicChecks` | `src/runner/*` | Runs built-in policies, plugin checks, configured tools, transcript, and summary. | | Local AI review | `runLocalAiReview` | `src/ai/*` | Applies guardrails, builds payload, calls provider runtime, builds verdict. | @@ -26,7 +26,7 @@ know. |---|---|---|---| | `PushgateConfig` | Config module | Workflow, runner, AI, path policy | Defaults are normalized; active AI modes require a selected provider block. | | `ChangedFileResolution` | Path policy | Deterministic runner, local AI | Contains target ref, target commit, merge base, filtered files, review range, and scan range. | -| `ChangedFile` | Path policy | Policies, tools, AI payload builder | Includes status, optional previous path, binary marker, additions, and deletions. | +| `ChangedFile` and projections | Path policy | Policies, tools, AI payload builder | Includes status, optional previous path, binary marker, additions, deletions, live-path selection, and changed text-line counts. | | `ToolResult` | Deterministic runner | Transcript and summary | Status is `passed`, `skipped`, `warning`, or `blocked`. | | `LocalAiReviewPayload` | AI review context | Provider adapters | Contains changed files, rendered diff, optional full-file context, and final prompt. | | `RawAiReviewOutput` | Provider output parser | AI verdict and transcript | Must match schema version `1` and strict finding fields. | @@ -53,7 +53,7 @@ helper. | Behavior | Primary tests | |---|---| | Config schema, defaults, provider selection, legacy config errors | `test/config.test.ts` | -| Changed-file parsing, ignored paths, target-ref errors, deleted files | `test/path-policy.test.ts` | +| Changed-file parsing, ignored paths, projections, target-ref errors, deleted files | `test/path-policy.test.ts` | | Deterministic policies, plugin checks, tool commands, fail-fast behavior | `test/deterministic-runner.test.ts`, `test/runner.test.ts` | | Hook protocol, pre-push runner behavior, skip controls, provider stubs | `test/runner.test.ts`, `test/hook.test.ts` | | Installer behavior and installed hook assets | `test/install.test.ts` | diff --git a/src/ai/guardrails.ts b/src/ai/guardrails.ts index 3f77985..b491a02 100644 --- a/src/ai/guardrails.ts +++ b/src/ai/guardrails.ts @@ -1,4 +1,7 @@ -import type { ChangedFileResolution } from "../path-policy/index.js"; +import { + countChangedTextLines, + type ChangedFileResolution, +} from "../path-policy/index.js"; export type ChangedFileGuardrailDecision = | { @@ -33,7 +36,7 @@ export function evaluateChangedFileGuardrails(options: { return { kind: "skip-no-files" }; } - const changedLineCount = countChangedLines(options.changedFiles); + const changedLineCount = countChangedTextLines(options.changedFiles); if (changedLineCount > options.maxChangedLines) { return { @@ -69,18 +72,6 @@ export function evaluatePromptGuardrail(options: { }; } -export function countChangedLines( - changedFiles: ChangedFileResolution["files"], -): number { - return changedFiles.reduce((total, file) => { - if (file.binary) { - return total; - } - - return total + (file.additions ?? 0) + (file.deletions ?? 0); - }, 0); -} - export function estimatePromptTokens(prompt: string): number { if (prompt.length === 0) { return 0; diff --git a/src/ai/review-context.ts b/src/ai/review-context.ts index 5287e50..67e487e 100644 --- a/src/ai/review-context.ts +++ b/src/ai/review-context.ts @@ -3,9 +3,10 @@ import { join } from "node:path"; import type { ReviewConfig } from "../config/index.js"; import { GitCommandError, runGitChecked } from "../git/command.js"; -import type { - ChangedFile, - ChangedFileResolution, +import { + selectLiveChangedFiles, + type ChangedFile, + type ChangedFileResolution, } from "../path-policy/index.js"; import { renderLocalAiPrompt } from "./review-prompt.js"; import type { @@ -56,7 +57,10 @@ export async function collectLocalAiReviewContext(options: { const diffLineCount = countTextLines(diff); const fullFiles = diffLineCount < options.reviewConfig.max_lines_for_full_file - ? await collectFullFiles(options.repoRoot, changedFiles) + ? await collectFullFiles( + options.repoRoot, + selectLiveChangedFiles(changedFiles), + ) : []; return { @@ -114,10 +118,6 @@ async function collectFullFiles( const fullFiles: LocalAiFullFileContext[] = []; for (const file of changedFiles) { - if (file.status === "deleted") { - continue; - } - if (file.binary) { fullFiles.push({ path: file.path, diff --git a/src/path-policy/filtering.ts b/src/path-policy/filtering.ts index 718bd97..e91ea6f 100644 --- a/src/path-policy/filtering.ts +++ b/src/path-policy/filtering.ts @@ -2,6 +2,10 @@ import ignore from "ignore"; import type { ChangedFile } from "./types.js"; +export interface SelectLiveChangedFilePathsOptions { + extensions?: readonly string[]; +} + /** Apply v2 `ignore_paths` rules to repository-relative changed paths. */ export function filterIgnoredChangedFiles( files: readonly ChangedFile[], @@ -16,6 +20,35 @@ export function filterIgnoredChangedFiles( return files.filter((file) => !ignorePathsMatcher.ignores(file.path)); } +export function countChangedTextLines(files: readonly ChangedFile[]): number { + return files.reduce((total, file) => { + if (file.binary) { + return total; + } + + return total + (file.additions ?? 0) + (file.deletions ?? 0); + }, 0); +} + +export function isLiveChangedFile(file: ChangedFile): boolean { + return file.status !== "deleted"; +} + +export function selectLiveChangedFiles( + files: readonly ChangedFile[], +): ChangedFile[] { + return files.filter(isLiveChangedFile); +} + +export function selectLiveChangedFilePaths( + files: readonly ChangedFile[], + options: SelectLiveChangedFilePathsOptions = {}, +): string[] { + return selectLiveChangedFiles(files) + .filter((file) => matchesExtension(file.path, options.extensions)) + .map((file) => file.path); +} + /** * Select paths that later deterministic tool commands may receive as argv. * @@ -26,10 +59,7 @@ export function selectToolChangedFilePaths( files: readonly ChangedFile[], extensions?: readonly string[], ): string[] { - return files - .filter((file) => file.status !== "deleted") - .filter((file) => matchesExtension(file.path, extensions)) - .map((file) => file.path); + return selectLiveChangedFilePaths(files, { extensions }); } function matchesExtension( diff --git a/src/path-policy/index.ts b/src/path-policy/index.ts index eadd5aa..be3d25f 100644 --- a/src/path-policy/index.ts +++ b/src/path-policy/index.ts @@ -7,9 +7,14 @@ export { } from "./errors.js"; import { filterIgnoredChangedFiles as applyIgnorePathFiltering } from "./filtering.js"; export { + countChangedTextLines, filterIgnoredChangedFiles, + isLiveChangedFile, + selectLiveChangedFilePaths, + selectLiveChangedFiles, selectToolChangedFilePaths, } from "./filtering.js"; +export type { SelectLiveChangedFilePathsOptions } from "./filtering.js"; import { readChangedFileDiffs, resolveDiffBase, diff --git a/src/runner/policies.ts b/src/runner/policies.ts index b9d1baa..e5ea0e9 100644 --- a/src/runner/policies.ts +++ b/src/runner/policies.ts @@ -6,7 +6,11 @@ import type { DiffSizePolicyConfig, ForbiddenPathsPolicyConfig, } from "../config/index.js"; -import type { ChangedFile } from "../path-policy/index.js"; +import { + countChangedTextLines, + selectLiveChangedFiles, + type ChangedFile, +} from "../path-policy/index.js"; export type BuiltInPolicyResultStatus = "passed" | "warning" | "blocked"; @@ -55,9 +59,7 @@ function runDiffSizePolicy( policy: DiffSizePolicyConfig, changedFiles: readonly ChangedFile[], ): BuiltInPolicyResult { - const changedLines = changedFiles.reduce((total, file) => { - return total + (file.additions ?? 0) + (file.deletions ?? 0); - }, 0); + const changedLines = countChangedTextLines(changedFiles); if (changedLines <= policy.max_changed_lines) { return { @@ -82,8 +84,7 @@ function runForbiddenPathsPolicy( policy: ForbiddenPathsPolicyConfig, changedFiles: readonly ChangedFile[], ): BuiltInPolicyResult { - const matches = changedFiles - .filter((file) => file.status !== "deleted") + const matches = selectLiveChangedFiles(changedFiles) .flatMap((file) => { const pattern = firstMatchingPattern(policy.patterns, file.path); diff --git a/test/ai.test.ts b/test/ai.test.ts index 4938e72..c4cb7df 100644 --- a/test/ai.test.ts +++ b/test/ai.test.ts @@ -753,13 +753,6 @@ test("evaluates local AI guardrails without provider stubs", () => { path: "src/changed.ts", status: "modified", }, - { - additions: null, - binary: true, - deletions: null, - path: "assets/logo.png", - status: "modified", - }, ], maxChangedLines: 10, }), diff --git a/test/path-policy.test.ts b/test/path-policy.test.ts index 8e8f09e..fea0ae4 100644 --- a/test/path-policy.test.ts +++ b/test/path-policy.test.ts @@ -12,10 +12,13 @@ import test from "node:test"; import { sanitizeGitLocalEnv } from "../src/git/environment.js"; import { + countChangedTextLines, GitChangedFilesError, MissingDiffBaseError, MissingTargetRefError, resolveChangedFiles, + selectLiveChangedFilePaths, + selectLiveChangedFiles, selectToolChangedFilePaths, } from "../src/path-policy/index.js"; @@ -75,6 +78,29 @@ test("resolves filtered changed paths and preserves Git path metadata", async () assert.equal(filesByPath.has("packages/app/dependency.lock"), false); assert.equal(filesByPath.has("dist/generated.ts"), false); + assert.deepEqual( + selectLiveChangedFiles(resolution.files) + .map((file) => file.path) + .sort(), + [ + "assets/logo.bin", + "src/file with spaces.ts", + "src/modified.ts", + "src/note.md", + "src/rename-after.ts", + ], + ); + assert.equal(countChangedTextLines(resolution.files), 5); + assert.deepEqual( + selectLiveChangedFilePaths(resolution.files, { + extensions: [".ts"], + }).sort(), + [ + "src/file with spaces.ts", + "src/modified.ts", + "src/rename-after.ts", + ], + ); assert.deepEqual( selectToolChangedFilePaths(resolution.files, [".ts"]).sort(), [