diff --git a/.changeset/patient-koalas-roar.md b/.changeset/patient-koalas-roar.md new file mode 100644 index 000000000..25be33c78 --- /dev/null +++ b/.changeset/patient-koalas-roar.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 3695 +--- +**Code review depth can now be scoped by repository path** — set `workflow.code_review_depth_overrides` to a list of `{paths, depth}` rules and a review touching a sensitive directory such as `src/auth` automatically runs at the stronger tier, while the rest of the repository keeps the standard depth. Paths are matched as directory prefixes on whole path segments (glob syntax is rejected with a clear configuration error), `--depth=` still wins, and the resolved depth and the rule that matched are printed in the review output. (#2554) diff --git a/.gitignore b/.gitignore index d7b9c17a1..0f2b1eef4 100644 --- a/.gitignore +++ b/.gitignore @@ -132,6 +132,7 @@ build/ /gsd-core/bin/lib/config-types.cjs /gsd-core/bin/lib/cli-exit.cjs /gsd-core/bin/lib/code-review-flags.cjs +/gsd-core/bin/lib/code-review-depth.cjs /gsd-core/bin/lib/context-utilization.cjs /gsd-core/bin/lib/artifacts.cjs /gsd-core/bin/lib/command-arg-projection.cjs diff --git a/CONTEXT.md b/CONTEXT.md index ffe447ccd..c2f791865 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -152,6 +152,9 @@ Module owning the projection from a **declared reviewer lane** plus resolved con ### Reviewer Lane Runner Module owning **execution** of an invocation plan (ADR-2782 Phase 5b, #2799): probe, spawn or HTTP call, empty-output policy, and dispatch of the three first-party `handler` modules D6 names (`antigravity`, `openai-compatible`, `opencode`). Replaces ~640 lines of hand-authored per-CLI bash in `invoke_reviewers`. Interface: `runLane`, `probeLane`, `checkEgressHost`, `writeReviewOrStub`, and the handler entry points (`handleOpencodeOutput`, `antigravityArgv`, `antigravityPrompt`, `antigravityWatermark`, `antigravityTranscriptFallback`, `antigravityDiagnostic`, `stampBlindReview`, `runOpenAiCompatible`). Every dependency is injected, so behaviour is testable without a network or a spawn. **Every subprocess call passes `timeout` + `killSignal` + `maxBuffer`** (`DEFECT.UNBOUNDED-SUBPROCESS`): a frozen synchronous spawn cannot be interrupted and hangs a whole CI chunk to its kill with `# fail 0`. Three runtime dependencies disappear here — `jq`, `curl`, and external `timeout`/`gtimeout` — which also closes two platform holes: five lanes were unavailable on stock Windows/Git-Bash for want of `jq`, and the Antigravity lane ran unbounded on stock macOS, which ships neither killer. Owns ADR-2782 D5 rules 2–4: the egress destination is **re-resolved at invocation** and a changed host blocks the lane rather than silently redirecting it; absence of a consent record allows, since first-party lanes are never consent-gated. `antigravityWatermark`'s final read can throw (permissions, mid-write truncation) on a transcript that indisputably exists, which is not the same fact as a genuinely empty or absent one; that case now sets `unreadable: true` on the returned mark rather than folding into `lines: 0`, and `antigravityTranscriptFallback` declines (`''`) for a same-conv-id unreadable mark instead of skipping zero lines and replaying a stale pre-run response (#3118). **The resolved model (#2295)** adds `MODEL_SOURCE`, `UNRESOLVED_MODEL`, `parseModelBanner`, `parseTranscriptModel`, `antigravityModel`, `resolveSpawnModel`, `BANNER_SCAN_LINES` and `MODEL_VALUE_MAX` to the interface: `LaneRunResult` now carries `model: {value, source}`, where `value === null` iff `source === 'unknown'`. The banner arm is gated on `outputTarget.kind === 'file'`, because a stdout lane's own review text would otherwise be scanned as its banner. `antigravityWatermark` now snapshots BOTH transcripts — `lines` for `transcript.jsonl`, `fullLines` for `transcript_full.jsonl` — since they are different files with different counts and one cannot offset the other. The model arm's staleness rule is deliberately looser than the review body's: a matching conv-id means the same `agy` session, whose model is this run's. Every arm is total and degrades to `unknown` — it may never fail a lane (#2295). +### Code Review Depth Module +Module owning the single resolution of a code review's depth tier (#2554), consumed by `gsd-core/workflows/code-review.md`'s `resolve_depth` step. Interface: `resolveCodeReviewDepth({flagDepth, configDepth, overrides, files, repoRoot})`, `REASON` (frozen reason enum — adding a member is three coordinated changes: enum + emitting site + the test locking `Object.keys(...).sort()`), `DEPTH_TIERS` (`quick` < `standard` < `deep`, ordered weakest-first), `LARGE_SCOPE_THRESHOLD`, and the two matching primitives `normalizeRelPath` / `ruleMatchesFile`. Resolution order is `--depth=` flag → strongest matching path rule → `workflow.code_review_depth` → `standard`, and the result reports its own `source` (`flag`/`rule`/`config`/`default`) per `### Resolution Provenance` — a depth with no provenance is what let the operator surface claim a global setting produced a result it did not. `workflow.code_review_depth_overrides` is an ordered array of `{paths, depth}` rules matched against the review's changed-file set; a matching rule REPLACES the global rather than being max'd with it, because folding the global in would make every `quick` and `standard` rule inert whenever the global was stronger — a config that parses and does nothing, which is the silently-discarded failure `### Federated Config` exists to prevent. Escalation is **whole-review, not per-file**: depth is a single scalar handed to `gsd-code-reviewer`, so one matched file raises the tier for the entire review and a sensitive file is never reviewed shallowly. Matching is **segment-aware** path-prefix — a rule naming a directory matches that directory and everything beneath it, and never a sibling whose name merely shares the prefix (a rule for "src/auth" must not match "src/authfoo") — and case-sensitive, following git — the same anchoring rule `### Emitted Artifact Provenance` states for its `sources` prefixes. **Glob metacharacters are a hard configuration error, not sugar for a prefix**: accepting a trailing double-star as a prefix would make a mid-path star look supported while matching nothing, arming a policy the operator believes is live (v1 scope decision on #2554 — no glob engine exists in this tree and none was added; runtime dependencies stay at two). A rule path carrying an interior control character (U+0000-U+001F or U+007F, checked after the glob check so a path that is both a glob and control-bearing still reports the glob reason) is likewise a hard configuration error — an unrejected newline or NUL would otherwise flow through the matched rule into the workflow's provenance string and corrupt the rendered review summary. Malformed rules halt the review rather than degrading to `standard`, since a misconfigured sensitive-path policy reviewing shallowly is the exact hole the module closes. The module is PURE — no fs, no clock, no subprocess, no require — so the workflow reaches it through the same `node -e` idiom it already uses for `src/code-review-flags.cts`; the changed-file list crosses on **stdin**, never argv (`DEFECT.WINDOWS-ARGV-OVERFLOW`). The pre-existing large-scope downgrade (`deep` → `standard` above `LARGE_SCOPE_THRESHOLD` files) moved into this module from the workflow so one seam owns depth end to end; it preserves `matchedRule` through the downgrade so the operator surface can name the rule it overrode. Note the key is registered in the CENTRAL config schema, not as a capability config slice: `### Federated Config`'s `VALID_SLICE_TYPES` admits only `boolean`/`string`/`number`/`enum`, so an array slice is dropped as malformed — the `ship.pr_body_sections` precedent is the one this follows. + ### Resolution Provenance Cross-seam principle (ADR-1411, epic #1411): context resolution — config loading, project-root anchoring, workstream resolution — must report its provenance, not fall open silently to defaults. A resolver anchors deterministically to the project root (one walk-up module, no dependence on an arbitrary descendant cwd), returns *what* it resolved **and** *where it came from* (`source`/`degraded`), and surfaces a diagnostic when a *configured* input resolves empty (`not configured` and `configured-but-empty` are distinguishable). The resolution-side analog of ADR-227 (input-validation shape). Target seams: Config Loader Module (`loadConfig` → `ConfigResolution { config, source, degraded }`), Project-Root Resolution Module (single nearest-`.planning/` walk-up, retiring ad-hoc resolvers like `resolvePlanningCwd`), I/O Module (`Resolution { value, configured, reason, warnings }` output envelope). A configured input resolving empty without a reason is a CI-guarded regression. **P1 (nearest-.planning/ heuristic) shipped in #1413; P2 (loadConfigResolved + agent-skills diagnostic) shipped in #1415 / closes #1366**: `loadConfigResolved` now implements the Config Loader seam target; `cmdAgentSkills` uses `findProjectRoot` + `loadConfigResolved` and emits `configured`/`reason`/`source`/`degraded` in its `--json` IR. **Corrupt is not absent (ADR-1411 amendment 2026-07-26, epic #1879 Phase 0 / #2674):** the principle above governs a resolution *miss*; input that is present but *not usable* (a `SyntaxError`, an errno such as `EACCES`/`EIO`, or a malformed structure with no exception at all) is a distinct class that must stay distinguishable from genuine absence. The defect in that class is not the fallback — ADR-227 requires malformed input to be coerced rather than propagated, and this ADR already permits a fallback — it is that the fallback is **invisible**. So every current return value is preserved and the cause is made visible by one of two mechanisms: **in-band**, where the result already carries a provenance envelope, name the cause in it (`ConfigResolution` gains a `reason`; `Resolution`'s four documented values all describe a miss, so new unusable-input values are introduced with the first adopter) and also expose it on the surface callers actually use, since a `reason` no caller reads is an unreachable field; **out-of-band**, where the read returns a bare sentinel or a plausible default it cannot extend, keep that value and emit a deduplicated `stderr` diagnostic keyed on resolved-path + errno, reusing the `_warnedUnknownConfigKeys` guard pattern. The diagnostic is unconditional — a deliberate divergence from ADR-227's never-implemented `GSD_DEBUG` opt-in, since an opt-in nobody sets is the same silence. Throwing is **not** the cluster's answer — it stays confined to ADR-227's genuinely-fatal carve-out, decided per call, never inferred from the return shape. diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 11b07cfa6..18b1f5b01 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -1562,7 +1562,7 @@ Review source files changed during a phase for bugs, security vulnerabilities, a | Argument | Required | Description | |----------|----------|-------------| | `N` | **Yes** | Phase number whose changes to review (e.g., `2` or `02`) | -| `--depth=quick\|standard\|deep` | No | Review depth level (overrides `workflow.code_review_depth` config). `quick`: pattern-matching only (~2 min). `standard`: per-file analysis with language-specific checks (~5–15 min, default). `deep`: cross-file analysis including import graphs and call chains (~15–30 min) | +| `--depth=quick\|standard\|deep` | No | Review depth level. Overrides both `workflow.code_review_depth` and any matching `workflow.code_review_depth_overrides` path rule — the flag always wins. `quick`: pattern-matching only (~2 min). `standard`: per-file analysis with language-specific checks (~5–15 min, default). `deep`: cross-file analysis including import graphs and call chains (~15–30 min) | | `--files file1,file2,...` | No | Explicit comma-separated file list; skips SUMMARY/git scoping entirely | | `--fix` | No | Auto-fix issues after review — reads REVIEW.md, spawns fixer agent, commits each fix atomically | | `--fix --all` | No | Include Info findings in fix scope (default: Critical + Warning only) | diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 650e3ed85..557100417 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -47,6 +47,7 @@ GSD stores project settings in `.planning/config.json`. Created during `/gsd-new "use_worktrees": true, "code_review": true, "code_review_depth": "standard", + "code_review_depth_overrides": [], "plan_bounce": false, "plan_bounce_script": null, "plan_bounce_passes": 2, @@ -369,6 +370,7 @@ All workflow toggles follow the **absent = enabled** pattern. If a key is missin | `workflow.worktree_skip_hooks` | boolean | `false` | When `true`, executor agents in worktree mode pass `--no-verify` (skipping pre-commit hooks) and post-wave hook validation runs against the merged result instead. Opt-in escape hatch for projects whose hooks cannot run in agent worktrees. Default `false` runs hooks on every commit (#2924). | | `workflow.code_review` | boolean | `true` | Enable `/gsd-code-review` and `/gsd-code-review --fix` commands. When `false`, the commands exit with a configuration gate message. Added in v1.34 | | `workflow.code_review_depth` | string | `standard` | Default review depth for `/gsd-code-review`: `quick` (pattern-matching only), `standard` (per-file analysis), or `deep` (cross-file with import graphs). Can be overridden per-run with `--depth=`. Added in v1.34 | +| `workflow.code_review_depth_overrides` | array | `[]` | Ordered list of `{ paths: string[], depth }` rules that escalate `/gsd-code-review` depth for specific directories, e.g. `[{ "paths": ["src/auth"], "depth": "deep" }]`. Each rule's `paths` are matched against the review's changed-file set by whole-segment directory-path prefix (`src/auth` matches `src/auth/token.ts`, never `src/authfoo/x.ts` or `docs/src/auth/x.ts`); matching is case-sensitive, following git. Glob syntax (`*`, `?`) is a configuration error, not sugar for a prefix. One matched file escalates the entire review — depth is not applied per file. Resolution order: `--depth=` flag → strongest matching rule → `workflow.code_review_depth` → `standard`; a matching rule wins even when its tier is weaker than the global default. A malformed rule (bad `depth`, glob syntax, absolute path, `..` segment, empty path, non-array `overrides`, non-object rule, malformed `paths`) is a configuration error and the review halts rather than falling back silently. The resolved depth and the matching rule are printed in the review output. Added in #2554 | | `workflow.plan_bounce` | boolean | `false` | Run external validation script against generated plans. When enabled, the plan-phase orchestrator pipes each PLAN.md through the script specified by `plan_bounce_script` and blocks on non-zero exit. Added in v1.36 | | `workflow.plan_bounce_script` | string | (none) | Path to the external script invoked for plan bounce validation. Receives the PLAN.md path as its first argument. Required when `plan_bounce` is `true`. Added in v1.36 | | `workflow.plan_bounce_passes` | number | `2` | Number of sequential bounce passes to run. Each pass feeds the previous pass's output back into the validator. Higher values increase rigor at the cost of latency. Added in v1.36 | diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 1e9e4ef50..5df58b8d3 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -2219,6 +2219,15 @@ Test suite that scans all agent, workflow, and command files for embedded inject |---------|------|---------|-------------| | `workflow.code_review` | boolean | `true` | Enable code review commands | | `workflow.code_review_depth` | string | `standard` | Default review depth: `quick`, `standard`, or `deep` | +| `workflow.code_review_depth_overrides` | array | `[]` | Ordered `{ paths, depth }` rules that escalate depth for directories matched by path prefix against the changed-file set (#2554). See below. | + +**Path-scoped code review depth overrides** + +`workflow.code_review_depth_overrides` matches rules against the review's changed-file set by whole-segment directory-path prefix — `src/auth` matches `src/auth/token.ts` and `src/auth` itself, never `src/authfoo/x.ts` or `docs/src/auth/x.ts` — and is case-sensitive, following git. + +Escalation is **whole-review, not per-file**: depth is a single scalar handed to the reviewer agent, not a per-file setting, so the strongest matching tier across the whole rule set applies to every file in the review — a sensitive file is never reviewed shallowly because it shared a review with an unrelated one. + +v1 supports **directory-prefix matching only, not glob syntax**: no glob engine (`minimatch`, `picomatch`, `fast-glob`) exists in this project and none was added for this feature. A path containing `*` or `?` (e.g. `src/auth/**`) is a configuration error rather than a silent near-miss, because accepting it as sugar for a prefix would make unsupported patterns look armed when they match nothing. Every use case in the issue is expressible as a directory prefix. See [Scope code review depth by path](how-to/scope-code-review-depth-by-path.md) for the resolution order, error table, and a worked example. --- diff --git a/docs/INVENTORY-MANIFEST.json b/docs/INVENTORY-MANIFEST.json index 966755143..edecccc76 100644 --- a/docs/INVENTORY-MANIFEST.json +++ b/docs/INVENTORY-MANIFEST.json @@ -339,6 +339,7 @@ "cli-skew-check.cjs", "clock.cjs", "clusters.cjs", + "code-review-depth.cjs", "code-review-flags.cjs", "codex-agent-toml.cjs", "command-aliases.cjs", diff --git a/docs/INVENTORY.md b/docs/INVENTORY.md index 0d1628d91..19c3ddc3c 100644 --- a/docs/INVENTORY.md +++ b/docs/INVENTORY.md @@ -472,6 +472,7 @@ Full listing: `gsd-core/bin/lib/*.cjs`. | `host-integration-adapters/cline-sdk-binding.cjs` | Cline SDK binding — pure AgentPlugin `beforeTool` planning-artifact guard and `createAgentModel` model-override resolution adapters, no `@cline/sdk` import (ADR-1239 Phase D, #2090) | | `clock.cjs` | Injectable clock seam (now/sleep) for deterministic lock testing | | `clusters.cjs` | Skill cluster definitions for the runtime surface module (ADR-0011 Phase 2) | +| `code-review-depth.cjs` | Pure resolver for a code review's depth tier (#2554); exports `resolveCodeReviewDepth({ flagDepth, configDepth, overrides, files, repoRoot })` (→ `{ ok, depth, resolvedDepth, source, matchedRule, downgraded, fileCount }` or `{ ok: false, errors }`), the frozen `REASON` enum, `DEPTH_TIERS`, `LARGE_SCOPE_THRESHOLD`, and the matching primitives `normalizeRelPath`/`ruleMatchesFile`; resolves `--depth=` flag → strongest matching `workflow.code_review_depth_overrides` rule (segment-aware path prefix, no globs) → `workflow.code_review_depth` → `standard`, and owns the >50-file `deep`→`standard` downgrade | | `code-review-flags.cjs` | Typed flag parser for `/gsd-code-review`; exports `parseCodeReviewFlags(argv)` (→ `{ fix, all, auto, depth, files }`) and `resolveCodeReviewWorkflow(flags)` (→ `'code-review.md' \| 'code-review-fix.md'`); canonical dispatch seam for `--fix`/`--all`/`--auto` routing | | `codex-agent-toml.cjs` | Typed IR (genuine leaf) for `~/.codex/agents/.toml` — `parseCodexAgentToml`/`renderCodexAgentToml` round-trip byte-identically; `stripModel`/`stripReasoningEffort` remove exactly one targeted line; `scanTomlLines`/`stripBOM`/`findDeveloperInstructionsBlockRange`/`unquoteTomlValue` are the lenient reader primitives moved here from `agent-install-check.cjs` (#3242 Phase 2); consumed by the Codex `.toml` sync (`commands.cjs cmdEffortSyncCodex`, ADR-2313 D7, #3243) | | `command-aliases.cjs` | Alias/subcommand metadata for manifest-backed family routers | diff --git a/docs/README.md b/docs/README.md index e39cda3be..c5a03fb6a 100644 --- a/docs/README.md +++ b/docs/README.md @@ -38,6 +38,7 @@ Language versions: [English](README.md) · [Português (pt-BR)](pt-BR/README.md) - [Configure model profiles](how-to/configure-model-profiles.md) — switch between quality, balanced, and budget model tiers - [Control which host runtime GSD reports](how-to/control-the-reported-host-runtime.md) — read the `agent_runtime` ladder, understand what host detection looks at, and pin the runtime when detection is not what you want - [Set up cross-AI review](how-to/set-up-cross-ai-review.md) — configure a second AI to review code produced by the primary agent +- [Scope code review depth by path](how-to/scope-code-review-depth-by-path.md) — escalate `/gsd-code-review` to `deep` for sensitive directories while the rest of the repo stays at the default depth - [Work in parallel with workstreams](how-to/work-in-parallel-with-workstreams.md) — run independent lines of work simultaneously using workstreams - [Isolate work with workspaces](how-to/isolate-work-with-workspaces.md) — use workspaces to sandbox experimental or risky changes - [Debug a failed execution](how-to/debug-a-failed-execution.md) — diagnose and recover from broken or incomplete phase execution diff --git a/docs/how-to/scope-code-review-depth-by-path.md b/docs/how-to/scope-code-review-depth-by-path.md new file mode 100644 index 000000000..09f747c30 --- /dev/null +++ b/docs/how-to/scope-code-review-depth-by-path.md @@ -0,0 +1,124 @@ +# How to scope code review depth by path + +**Goal:** Escalate `/gsd-code-review` to `deep` for sensitive directories (auth, billing, payments) while the rest of the repository keeps reviewing at the project's normal default — without having to remember `--depth=deep` on every review that happens to touch one of those paths. + +**Prerequisites:** A project with `/gsd-code-review` enabled (`workflow.code_review: true`). This guide assumes you already have a working default depth via `workflow.code_review_depth`; see [Configuration Reference](../CONFIGURATION.md#workflow-toggles) if you don't. + +--- + +## The shortest working sequence + +1. **Set the global default**, if you haven't already: + + ```bash + gsd config-set workflow.code_review_depth standard + ``` + +2. **Add path-scoped rules** to `workflow.code_review_depth_overrides` in `.planning/config.json`. It's an ordered array of `{ "paths": [...], "depth": "quick" | "standard" | "deep" }` objects: + + ```json + { + "workflow": { + "code_review_depth": "standard", + "code_review_depth_overrides": [ + { "paths": ["src/auth"], "depth": "deep" }, + { "paths": ["src/billing"], "depth": "deep" } + ] + } + } + ``` + +3. **Run the review** as usual: + + ```bash + /gsd-code-review 12 + ``` + +4. **Read the provenance line** in the output to confirm which rule (if any) fired — see [Read the resolved-depth line](#read-the-resolved-depth-line) below. + +--- + +## Worked example: escalate `src/auth` and `src/billing`, leave everything else at `standard` + +With the config above, a phase that only touches `src/lib/formatter.ts` reviews at `standard` (no rule matches, falls through to the global default). A phase that touches `src/auth/token.ts` and nothing else in an escalated path reviews at `deep`, because a rule matched. + +**Escalation is whole-review, not per-file.** If a phase touches both `src/lib/formatter.ts` and `src/auth/token.ts`, the *entire* review — including `formatter.ts` — runs at `deep`. Depth is a single scalar handed to the reviewer agent; one matched sensitive file is enough to raise the whole review, so a sensitive file is never reviewed shallowly because it shared a phase with unrelated code. + +Matching is by **whole path segment**, not substring: + +| Changed file | Matches rule `src/auth`? | +|---|---| +| `src/auth/token.ts` | Yes | +| `src/auth` (the file itself) | Yes | +| `src/authfoo/x.ts` | No — `authfoo` is a different segment | +| `docs/src/auth/x.ts` | No — prefix is anchored at the path root, not a substring search | + +Matching is case-sensitive, following git: a rule written `Src/Auth` will not match `src/auth/x.ts`. + +**Rules win over the global default, even when weaker.** If `workflow.code_review_depth` is `deep` but a matched rule says `quick`, the review runs at `quick` — the rule always replaces the global for files it matches. This is deliberate: if the strongest tier always won, a `quick` or `standard` rule could never actually take effect whenever the project default was `deep`, making it silently inert. + +**Only globs are rejected — not the paths themselves.** `workflow.code_review_depth_overrides` supports directory-prefix strings only (`src/auth`, not `src/auth/**`). There is no glob engine in this project; write the prefix and let segment-aware matching do the rest. + +--- + +## Read the resolved-depth line + +Every review prints one line naming the resolved depth and why: + +``` +Review depth: deep (matched rule 0: src/auth) +``` + +The parenthetical names the source: + +| Provenance text | Meaning | +|---|---| +| `from --depth flag` | The `--depth=` CLI flag was passed; it always wins over both rules and config. | +| `matched rule N: ` | Rule at index `N` (0-based, in declaration order) matched on prefix `` and set the depth. | +| `from workflow.code_review_depth` | No rule matched this review's file set; the global config value was used. | +| `default` | Neither a rule, config value, nor flag applied; the built-in `standard` default was used. | + +If the review scope exceeds 50 files and the resolved depth is `deep`, the existing large-scope downgrade still fires — but now it names the rule it overrode: + +``` +Switching from deep to standard depth for large file count (overrides matched rule 0: src/auth). +``` + +A configured sensitive-path policy is not exempt from this downgrade — the same guard that downgrades `--depth=deep` on a large scope also downgrades a rule-sourced `deep`. + +--- + +## Nothing to report vs. could not look + +Two outcomes look similar but mean opposite things: + +- **A rule matches nothing.** This is not an error and is not reported specially — it means this review simply didn't touch any path the rule covers. The provenance line falls through to `from workflow.code_review_depth` or `default`, exactly as if the rule didn't exist. This is "nothing to report": the policy exists, was checked, and had nothing to say about this particular review. +- **The configuration is rejected.** This is "could not look": the review halts before doing any file-level work and prints every collected validation error. Never conflate the two — a validly-configured rule set with no match for this review is a healthy, silent no-op; a malformed rule set is a hard stop. + +--- + +## Configuration error reasons + +If `workflow.code_review_depth_overrides` is malformed, `/gsd-code-review` prints one error per defect (all of them, not just the first) and stops — it never silently falls back to a default depth. Errors report the reason as one of the following typed values (from `src/code-review-depth.cts`): + +| Reason | Meaning | Fix | +|---|---|---| +| `not_an_array` | `workflow.code_review_depth_overrides` itself is not an array (object, string, number, `null`, etc.) | Set it to an array of rule objects, or `[]` to disable overrides. | +| `rule_not_object` | An entry in the array is not a plain object (a string, an array, `null`, `0`, etc.) | Each entry must be a `{ "paths": [...], "depth": "..." }` object. | +| `paths_malformed` | A rule's `paths` is missing, not an array, empty, or contains a non-string entry | Give `paths` a non-empty array of strings, e.g. `["src/auth"]`. | +| `invalid_depth` | A rule's `depth` is missing or not one of `quick`, `standard`, `deep` | Set `depth` to exactly one of `quick`, `standard`, or `deep`. | +| `glob_unsupported` | A rule path contains `*` or `?` (e.g. `src/auth/**`) | Use a directory prefix instead: `src/auth`, not `src/auth/**` or `src/auth/*.ts`. | +| `path_traversal` | A rule path contains a `..` segment | Remove the `..` segment; write a plain repo-relative prefix. | +| `path_absolute` | A rule path is absolute (`/src/auth`, `C:\src\auth`) | Use a path relative to the repo root: `src/auth`, not `/src/auth`. | +| `path_empty` | A rule path is empty, whitespace-only, or normalizes to empty or `.` | Give the path real content, e.g. `src/auth` rather than `""` or `"."`. | + +Each printed error names the rule index (and, where applicable, the offending path or depth value) so you can find the exact entry to fix without guessing which rule in the array is broken. + +--- + +## Related + +- [Configuration Reference](../CONFIGURATION.md#workflow-toggles) — full schema for `workflow.code_review_depth_overrides` and `workflow.code_review_depth` +- [Feature Reference — Code Review Pipeline](../FEATURES.md#93-code-review-pipeline) — why escalation is whole-review and why v1 is prefix-only, not glob +- [`/gsd-code-review`](../COMMANDS.md#gsd-code-review) — command reference and the `--depth=` flag +- [docs index](../README.md) diff --git a/docs/ja-JP/COMMANDS.md b/docs/ja-JP/COMMANDS.md index 97d60ae6d..9bef43e2c 100644 --- a/docs/ja-JP/COMMANDS.md +++ b/docs/ja-JP/COMMANDS.md @@ -1163,7 +1163,7 @@ AI システムの構築を含むフェーズの AI-SPEC.md デザインコン | 引数 | 必須 | 説明 | |----------|----------|-------------| | `N` | **Yes** | レビューする変更のフェーズ番号（例: `2` または `02`） | -| `--depth=quick\|standard\|deep` | No | レビューの深さレベル（`workflow.code_review_depth` 設定を上書き）。`quick`: パターンマッチングのみ（約2分）。`standard`: 言語固有のチェックを含むファイルごとの分析（約5〜15分、デフォルト）。`deep`: インポートグラフとコールチェーンを含むクロスファイル分析（約15〜30分） | +| `--depth=quick\|standard\|deep` | No | レビューの深さレベル。`workflow.code_review_depth` と、一致する `workflow.code_review_depth_overrides` のパスルールの両方を上書きします — フラグが常に優先します。`quick`: パターンマッチングのみ（約2分）。`standard`: 言語固有のチェックを含むファイルごとの分析（約5〜15分、デフォルト）。`deep`: インポートグラフとコールチェーンを含むクロスファイル分析（約15〜30分） | | `--files file1,file2,...` | No | 明示的なカンマ区切りのファイルリスト; SUMMARY/git スコーピングを完全にスキップ | | `--fix` | No | レビュー後に問題を自動修正 — REVIEW.md を読み込み、修正エージェントを起動し、各修正をアトミックにコミット | | `--fix --all` | No | 修正スコープに Info の発見事項を含める（デフォルト: Critical + Warning のみ） | diff --git a/docs/ja-JP/FEATURES.md b/docs/ja-JP/FEATURES.md index 9fefa6d19..59e94be59 100644 --- a/docs/ja-JP/FEATURES.md +++ b/docs/ja-JP/FEATURES.md @@ -2119,6 +2119,15 @@ Claude が GSD ワークフローコンテキスト外でファイル編集を |------|-----|-----------|------| | `workflow.code_review` | boolean | `true` | コードレビューコマンドを有効化 | | `workflow.code_review_depth` | string | `standard` | デフォルトのレビュー深度：`quick`、`standard`、または `deep` | +| `workflow.code_review_depth_overrides` | array | `[]` | 変更ファイル集合に対するパスプレフィックス一致で特定ディレクトリのレビュー深度をエスカレートする、順序付き `{ paths, depth }` ルール（#2554）。詳細は下記参照。 | + +**パススコープのコードレビュー深度オーバーライド** + +`workflow.code_review_depth_overrides` は、レビュー対象の変更ファイル集合に対して、セグメント単位のディレクトリパスプレフィックスでルールを照合します。`src/auth` は `src/auth/token.ts` および `src/auth` 自体に一致しますが、`src/authfoo/x.ts` や `docs/src/auth/x.ts` には一致しません。照合は git と同様に大文字小文字を区別します。 + +エスカレーションは**レビュー全体単位であり、ファイル単位ではありません**：深度はレビューエージェントに渡される単一のスカラー値であり、ファイルごとの設定ではないため、ルールセット全体で一致した最も強いティアがレビュー内のすべてのファイルに適用されます — 機密ファイルが無関係なファイルと同じレビューに含まれたために浅くレビューされることはありません。 + +v1 は**ディレクトリプレフィックス一致のみをサポートし、glob 構文はサポートしません**：このプロジェクトには glob エンジン（`minimatch`、`picomatch`、`fast-glob`）が存在せず、この機能のために追加もされていません。`*` や `?` を含むパス（例：`src/auth/**`）は、静かな近似一致ではなく設定エラーとして扱われます。 --- diff --git a/docs/ko-KR/COMMANDS.md b/docs/ko-KR/COMMANDS.md index 4b1352caf..474179357 100644 --- a/docs/ko-KR/COMMANDS.md +++ b/docs/ko-KR/COMMANDS.md @@ -1169,7 +1169,7 @@ AI 시스템 구축을 포함하는 단계에 대한 AI-SPEC.md 디자인 계약 | 인수 | 필수 | 설명 | |----------|----------|-------------| | `N` | **예** | 검토할 변경사항이 있는 단계 번호 (예: `2` 또는 `02`) | -| `--depth=quick\|standard\|deep` | 아니요 | 검토 깊이 수준 (`workflow.code_review_depth` 설정 재정의). `quick`: 패턴 매칭만 (~2분). `standard`: 언어별 검사를 통한 파일별 분석 (~5–15분, 기본값). `deep`: 임포트 그래프와 호출 체인을 포함한 크로스 파일 분석 (~15–30분) | +| `--depth=quick\|standard\|deep` | 아니요 | 검토 깊이 수준. `workflow.code_review_depth`와 일치하는 `workflow.code_review_depth_overrides` 경로 규칙을 모두 재정의합니다 — 플래그가 항상 우선합니다. `quick`: 패턴 매칭만 (~2분). `standard`: 언어별 검사를 통한 파일별 분석 (~5–15분, 기본값). `deep`: 임포트 그래프와 호출 체인을 포함한 크로스 파일 분석 (~15–30분) | | `--files file1,file2,...` | 아니요 | 명시적 쉼표 구분 파일 목록; SUMMARY/git 범위 지정을 완전히 건너뜀 | | `--fix` | 아니요 | 검토 후 자동 문제 수정 — REVIEW.md를 읽고, 수정자 에이전트를 생성하고, 각 수정을 원자적으로 커밋 | | `--fix --all` | 아니요 | 수정 범위에 Info 결과 포함 (기본값: Critical + Warning만) | diff --git a/docs/pt-BR/COMMANDS.md b/docs/pt-BR/COMMANDS.md index 8a61c2533..1ff705d99 100644 --- a/docs/pt-BR/COMMANDS.md +++ b/docs/pt-BR/COMMANDS.md @@ -1166,7 +1166,7 @@ Revisa arquivos de código-fonte alterados durante uma fase em busca de bugs, vu | Argumento | Obrigatório | Descrição | |-----------|-------------|-----------| | `N` | **Sim** | Número da fase cujas mudanças revisar (por exemplo, `2` ou `02`) | -| `--depth=quick\|standard\|deep` | Não | Nível de profundidade da revisão (substitui a configuração `workflow.code_review_depth`). `quick`: somente correspondência de padrões (~2 min). `standard`: análise por arquivo com verificações específicas de linguagem (~5–15 min, padrão). `deep`: análise entre arquivos incluindo grafos de importação e cadeias de chamadas (~15–30 min) | +| `--depth=quick\|standard\|deep` | Não | Nível de profundidade da revisão. Substitui tanto `workflow.code_review_depth` quanto qualquer regra de caminho correspondente em `workflow.code_review_depth_overrides` — a flag sempre prevalece. `quick`: somente correspondência de padrões (~2 min). `standard`: análise por arquivo com verificações específicas de linguagem (~5–15 min, padrão). `deep`: análise entre arquivos incluindo grafos de importação e cadeias de chamadas (~15–30 min) | | `--files file1,file2,...` | Não | Lista explícita de arquivos separados por vírgula; ignora completamente o escopo SUMMARY/git | | `--fix` | Não | Corrige automaticamente problemas após a revisão — lê REVIEW.md, cria agente corretor, faz commit de cada correção atomicamente | | `--fix --all` | Não | Inclui descobertas Info no escopo de correção (padrão: somente Critical + Warning) | diff --git a/docs/pt-BR/CONFIGURATION.md b/docs/pt-BR/CONFIGURATION.md index 4f04fbdcf..4bcddad32 100644 --- a/docs/pt-BR/CONFIGURATION.md +++ b/docs/pt-BR/CONFIGURATION.md @@ -47,6 +47,7 @@ O GSD armazena as configurações do projeto em `.planning/config.json`. Criado "use_worktrees": true, "code_review": true, "code_review_depth": "standard", + "code_review_depth_overrides": [], "plan_bounce": false, "plan_bounce_script": null, "plan_bounce_passes": 2, @@ -248,6 +249,7 @@ Todos os controles de fluxo de trabalho seguem o padrão **ausente = habilitado* | `workflow.worktree_skip_hooks` | boolean | `false` | Quando `true`, os agentes executores no modo worktree passam `--no-verify` (ignorando hooks de pré-commit) e a validação de hook pós-onda é executada contra o resultado mesclado. Válvula de escape opt-in para projetos cujos hooks não podem ser executados em worktrees de agente. Padrão `false` executa hooks em cada commit (#2924). | | `workflow.code_review` | boolean | `true` | Habilita os comandos `/gsd-code-review` e `/gsd-code-review --fix`. Quando `false`, os comandos saem com uma mensagem de gate de configuração. Adicionado na v1.34 | | `workflow.code_review_depth` | string | `standard` | Profundidade de revisão padrão para `/gsd-code-review`: `quick` (somente correspondência de padrão), `standard` (análise por arquivo) ou `deep` (entre arquivos com grafos de importação). Pode ser substituído por execução com `--depth=`. Adicionado na v1.34 | +| `workflow.code_review_depth_overrides` | array | `[]` | Lista ordenada de regras `{ paths: string[], depth }` que aumentam a profundidade de `/gsd-code-review` para diretórios específicos, ex.: `[{ "paths": ["src/auth"], "depth": "deep" }]`. Cada `paths` da regra é comparado com o conjunto de arquivos alterados da revisão por prefixo de diretório com segmentos completos (`src/auth` corresponde a `src/auth/token.ts`, nunca a `src/authfoo/x.ts` ou `docs/src/auth/x.ts`); a comparação diferencia maiúsculas/minúsculas, como o git. Sintaxe glob (`*`, `?`) é um erro de configuração, não um atalho para prefixo. Um único arquivo correspondido eleva a profundidade de toda a revisão — a profundidade não é aplicada por arquivo. Ordem de resolução: flag `--depth=` → regra correspondente mais forte → `workflow.code_review_depth` → `standard`; uma regra correspondente prevalece mesmo quando seu nível é mais fraco que o padrão global. Uma regra malformada (`depth` inválido, sintaxe glob, caminho absoluto, segmento `..`, caminho vazio, `overrides` que não é array, regra que não é objeto, `paths` malformado) é um erro de configuração e a revisão é interrompida em vez de usar um valor padrão silenciosamente. A profundidade resolvida e a regra correspondente são exibidas na saída da revisão. Adicionado em #2554 | | `workflow.plan_bounce` | boolean | `false` | Executa script de validação externo nos planos gerados. Quando habilitado, o orquestrador de fase de planejamento encaminha cada PLAN.md pelo script especificado por `plan_bounce_script` e bloqueia em saída diferente de zero. Adicionado na v1.36 | | `workflow.plan_bounce_script` | string | (nenhum) | Caminho para o script externo invocado na validação de bounce de plano. Recebe o caminho do PLAN.md como primeiro argumento. Obrigatório quando `plan_bounce` é `true`. Adicionado na v1.36 | | `workflow.plan_bounce_passes` | number | `2` | Número de passagens sequenciais de bounce a executar. Cada passagem alimenta a saída da passagem anterior de volta no validador. Valores maiores aumentam o rigor ao custo de latência. Adicionado na v1.36 | diff --git a/docs/zh-CN/COMMANDS.md b/docs/zh-CN/COMMANDS.md index 003b99c3d..e24b79363 100644 --- a/docs/zh-CN/COMMANDS.md +++ b/docs/zh-CN/COMMANDS.md @@ -1163,7 +1163,7 @@ node gsd-tools.cjs intel api-surface # 渲染 api-map.json → API- | 参数 | 必填 | 描述 | |----------|----------|-------------| | `N` | **是** | 要审查的阶段编号（例如 `2` 或 `02`） | -| `--depth=quick\|standard\|deep` | 否 | 审查深度级别（覆盖 `workflow.code_review_depth` 配置）。`quick`：仅模式匹配（约 2 分钟）。`standard`：按文件分析，含特定语言检查（约 5-15 分钟，默认）。`deep`：跨文件分析，包括导入图和调用链（约 15-30 分钟） | +| `--depth=quick\|standard\|deep` | 否 | 审查深度级别。同时覆盖 `workflow.code_review_depth` 和任何匹配的 `workflow.code_review_depth_overrides` 路径规则——该标志始终优先。`quick`：仅模式匹配（约 2 分钟）。`standard`：按文件分析，含特定语言检查（约 5-15 分钟，默认）。`deep`：跨文件分析，包括导入图和调用链（约 15-30 分钟） | | `--files file1,file2,...` | 否 | 显式逗号分隔的文件列表；完全跳过 SUMMARY/git 范围界定 | | `--fix` | 否 | 审查后自动修复问题 — 读取 REVIEW.md，生成修复代理，原子性地提交每个修复 | | `--fix --all` | 否 | 将 Info 级别的发现纳入修复范围（默认：仅 Critical + Warning） | diff --git a/docs/zh-CN/CONFIGURATION.md b/docs/zh-CN/CONFIGURATION.md index 438b7563b..57efc8ab5 100644 --- a/docs/zh-CN/CONFIGURATION.md +++ b/docs/zh-CN/CONFIGURATION.md @@ -47,6 +47,7 @@ GSD 将项目设置存储在 `.planning/config.json` 中。该文件在 `/gsd-ne "use_worktrees": true, "code_review": true, "code_review_depth": "standard", + "code_review_depth_overrides": [], "plan_bounce": false, "plan_bounce_script": null, "plan_bounce_passes": 2, @@ -248,6 +249,7 @@ API 密钥字段接受字符串值（密钥本身）。也可以设置为哨兵 | `workflow.worktree_skip_hooks` | boolean | `false` | 为 `true` 时，worktree 模式下的执行器 agent 传递 `--no-verify`（跳过提交前钩子），波次后的钩子验证改为针对合并结果运行。适用于钩子无法在 agent worktree 中运行的项目的可选逃生舱口。默认 `false` 对每次提交运行钩子（#2924）。 | | `workflow.code_review` | boolean | `true` | 启用 `/gsd-code-review` 和 `/gsd-code-review --fix` 命令。为 `false` 时，命令以配置门禁消息退出。v1.34 新增 | | `workflow.code_review_depth` | string | `standard` | `/gsd-code-review` 的默认审查深度：`quick`（仅模式匹配）、`standard`（按文件分析）或 `deep`（带导入图的跨文件）。可通过 `--depth=` 按次运行覆盖。v1.34 新增 | +| `workflow.code_review_depth_overrides` | array | `[]` | 有序的 `{ paths: string[], depth }` 规则列表，按目录前缀匹配对特定目录提升 `/gsd-code-review` 的审查深度，例如 `[{ "paths": ["src/auth"], "depth": "deep" }]`。每条规则的 `paths` 按整段目录路径前缀与本次审查的变更文件集合匹配（`src/auth` 匹配 `src/auth/token.ts`，但绝不匹配 `src/authfoo/x.ts` 或 `docs/src/auth/x.ts`）；匹配区分大小写，与 git 一致。glob 语法（`*`、`?`）是配置错误，而非前缀的简写形式。一个匹配的文件会将整次审查提升到该深度——深度并非逐文件应用。解析顺序：`--depth=` 标志 → 匹配到的最强规则 → `workflow.code_review_depth` → `standard`；即使规则的档位比全局默认值弱，匹配到的规则依然生效。格式错误的规则（`depth` 无效、glob 语法、绝对路径、`..` 段、路径为空、`overrides` 非数组、规则非对象、`paths` 格式错误）是配置错误，审查会中止，而不会静默回退。解析出的深度及匹配的规则会打印在审查输出中。#2554 新增 | | `workflow.plan_bounce` | boolean | `false` | 针对生成的计划运行外部验证脚本。启用后，计划阶段编排器将每个 PLAN.md 通过 `plan_bounce_script` 指定的脚本管道处理，并在非零退出时阻塞。v1.36 新增 | | `workflow.plan_bounce_script` | string | （无） | 用于计划反弹验证的外部脚本路径。接收 PLAN.md 路径作为第一个参数。当 `plan_bounce` 为 `true` 时必需。v1.36 新增 | | `workflow.plan_bounce_passes` | number | `2` | 顺序执行的反弹轮数。每轮将上一轮的输出反馈给验证器。较高的值提升严格性，但会增加延迟。v1.36 新增 | diff --git a/docs/zh-CN/FEATURES.md b/docs/zh-CN/FEATURES.md index 79efe6c0b..c14b11e00 100644 --- a/docs/zh-CN/FEATURES.md +++ b/docs/zh-CN/FEATURES.md @@ -2135,6 +2135,15 @@ PreToolUse 钩子，检测 Claude 在 GSD 工作流上下文之外尝试文件 |---------|------|---------|-------------| | `workflow.code_review` | boolean | `true` | 启用代码审查命令 | | `workflow.code_review_depth` | string | `standard` | 默认审查深度：`quick`、`standard` 或 `deep` | +| `workflow.code_review_depth_overrides` | array | `[]` | 按目录路径前缀匹配变更文件集合、为特定目录提升审查深度的有序 `{ paths, depth }` 规则列表（#2554）。详见下文。 | + +**按路径限定代码审查深度** + +`workflow.code_review_depth_overrides` 通过整段目录路径前缀，将规则与本次审查的变更文件集合进行匹配 —— `src/auth` 匹配 `src/auth/token.ts` 及 `src/auth` 本身，但绝不匹配 `src/authfoo/x.ts` 或 `docs/src/auth/x.ts`。匹配区分大小写，与 git 保持一致。 + +升级是**针对整次审查，而非逐文件**的：深度是传递给审查代理的单一标量值，而非逐文件设置，因此规则集中匹配到的最强档位适用于本次审查中的每一个文件 —— 一个敏感文件不会因为与无关文件同处一次审查中而被浅层审查。 + +v1 **仅支持目录前缀匹配，不支持 glob 语法**：本项目中不存在 glob 引擎（`minimatch`、`picomatch`、`fast-glob`），本功能也未引入。路径中包含 `*` 或 `?`（例如 `src/auth/**`）会被视为配置错误，而不是悄悄地按前缀近似处理。 --- diff --git a/eslint.config.mjs b/eslint.config.mjs index e3b4ff68e..7f9a17a4f 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -109,6 +109,7 @@ export default tseslint.config( 'gsd-core/bin/lib/prohibition-enforcement.cjs', 'gsd-core/bin/lib/ui-consideration-probe.cjs', 'gsd-core/bin/lib/code-review-flags.cjs', + 'gsd-core/bin/lib/code-review-depth.cjs', 'gsd-core/bin/lib/context-utilization.cjs', 'gsd-core/bin/lib/broken-windows.cjs', 'gsd-core/bin/lib/complexity-trigger.cjs', diff --git a/gsd-core/bin/shared/config-defaults.manifest.json b/gsd-core/bin/shared/config-defaults.manifest.json index cd799fd12..26bbb0b8c 100644 --- a/gsd-core/bin/shared/config-defaults.manifest.json +++ b/gsd-core/bin/shared/config-defaults.manifest.json @@ -46,6 +46,7 @@ "context_coverage_gate": true, "code_review": true, "code_review_depth": "standard", + "code_review_depth_overrides": [], "code_review_command": null, "pattern_mapper": true, "plan_bounce": false, diff --git a/gsd-core/bin/shared/config-schema.manifest.json b/gsd-core/bin/shared/config-schema.manifest.json index f7c5e8727..2e3a8749d 100644 --- a/gsd-core/bin/shared/config-schema.manifest.json +++ b/gsd-core/bin/shared/config-schema.manifest.json @@ -29,6 +29,7 @@ "workflow.use_worktrees", "workflow.worktree_skip_hooks", "workflow.code_review_command", + "workflow.code_review_depth_overrides", "workflow.plan_bounce", "workflow.plan_bounce_script", "workflow.plan_bounce_passes", diff --git a/gsd-core/references/planning-config.md b/gsd-core/references/planning-config.md index 6be2a27e4..5362777dc 100644 --- a/gsd-core/references/planning-config.md +++ b/gsd-core/references/planning-config.md @@ -278,6 +278,7 @@ Set via `workflow.*` namespace in config.json (e.g., `"workflow": { "research": | `workflow.inline_plan_threshold` | number | `2` | `0`–`10` | Plans with ≤N tasks execute inline instead of spawning a subagent | | `workflow.code_review` | boolean | `true` | `true`, `false` | Enable built-in code review step in the ship workflow | | `workflow.code_review_depth` | string | `"standard"` | `"quick"`, `"standard"`, `"deep"` | Depth level for code review analysis in the ship workflow | +| `workflow.code_review_depth_overrides` | array | `[]` | Array of `{paths, depth}` rule objects | Ordered path-scoped depth rules for `/gsd:code-review` (#2554). Each rule's `paths` are matched against the review's changed-file set by whole-segment directory-path prefix (`src/auth` matches `src/auth/token.ts`, never `src/authfoo/x.ts`); matching is case-sensitive. Glob syntax (`*`, `?`) is a configuration error. One matched file escalates the entire review — depth is not applied per file. Resolution order: `--depth=` flag → strongest matching rule → `workflow.code_review_depth` → `standard`. A malformed rule halts the review with a typed error rather than falling back silently. | | `workflow._auto_chain_active` | boolean | `false` | `true`, `false` | Internal: tracks whether autonomous chaining is active | | `workflow.security_enforcement` | boolean | `true` | `true`, `false` | Enable threat-model-anchored security verification via `/gsd:secure-phase`. When `false`, security checks are skipped entirely | | `workflow.security_asvs_level` | number | `1` | `1`, `2`, `3` | OWASP ASVS verification level. Level 1 = opportunistic, Level 2 = standard, Level 3 = comprehensive. Scales both planner threat-disposition rigor (which threats must be mitigated vs. accepted) and auditor verification depth (grep-level → boundary-placement check → full data-flow trace). See `gsd-core/references/security-asvs-levels.md`. | diff --git a/gsd-core/workflows/code-review.md b/gsd-core/workflows/code-review.md index cfed1c539..b86ae66ea 100644 --- a/gsd-core/workflows/code-review.md +++ b/gsd-core/workflows/code-review.md @@ -98,36 +98,6 @@ Exit workflow. Default is active through the Capability Registry schema — only skip when the registry resolves no active code-review step hook. This check runs AFTER phase validation so invalid phase errors are shown first. - -Determine review depth with priority order: - -1. DEPTH_OVERRIDE from --depth flag (highest priority) -2. Config value: `gsd-tools.cjs query config-get workflow.code_review_depth 2>/dev/null` -3. Default: "standard" - -```bash -if [ -n "$DEPTH_OVERRIDE" ]; then - REVIEW_DEPTH="$DEPTH_OVERRIDE" -else - CONFIG_DEPTH=$(gsd_run query config-get workflow.code_review_depth 2>/dev/null || echo "") - REVIEW_DEPTH="${CONFIG_DEPTH:-standard}" -fi -``` - -**Validate depth value:** -```bash -case "$REVIEW_DEPTH" in - quick|standard|deep) - # Valid - ;; - *) - echo "Warning: Invalid depth '${REVIEW_DEPTH}'. Valid values: quick, standard, deep. Using 'standard'." - REVIEW_DEPTH="standard" - ;; -esac -``` - - Three-tier scoping with explicit precedence: @@ -394,14 +364,124 @@ echo "File scope: ${#REVIEW_FILES[@]} files from ${TIER}" if [ ${#REVIEW_FILES[@]} -gt 50 ]; then echo "Warning: ${#REVIEW_FILES[@]} files is a large review scope." echo "Consider using --files to narrow scope, or --depth=quick for a faster pass." - if [ "$REVIEW_DEPTH" = "deep" ]; then - echo "Switching from deep to standard depth for large file count." - REVIEW_DEPTH="standard" - fi fi ``` + +Determine review depth via the path-scoped depth resolver (`code-review-depth.cjs`). This step runs after `compute_file_scope` because rule matching needs the final `REVIEW_FILES` set. + +```bash +CONFIG_DEPTH=$(gsd_run query config-get workflow.code_review_depth 2>/dev/null || echo "") +DEPTH_OVERRIDES=$(gsd_run query config-get workflow.code_review_depth_overrides --default '[]' 2>/dev/null || echo '[]') +REPO_ROOT=$(git rev-parse --show-toplevel 2>/dev/null) + +# Files travel on stdin, never argv — a 50+-file scope with long paths approaches the +# Windows execFileSync 32,767-char argv ceiling; stdin has no such bound. +DEPTH_PAYLOAD=$(printf '%s\n' "${REVIEW_FILES[@]}" | FLAG_DEPTH="$DEPTH_OVERRIDE" CONFIG_DEPTH="$CONFIG_DEPTH" DEPTH_OVERRIDES="$DEPTH_OVERRIDES" REPO_ROOT="$REPO_ROOT" node -e " + const files = require('fs').readFileSync('/dev/stdin', 'utf-8').split('\n').filter(Boolean); + process.stdout.write(JSON.stringify({ + flagDepth: process.env.FLAG_DEPTH || '', + configDepth: process.env.CONFIG_DEPTH || '', + overrides: JSON.parse(process.env.DEPTH_OVERRIDES || '[]'), + files, + repoRoot: process.env.REPO_ROOT || '', + })); +") + +DEPTH_JSON=$(echo "$DEPTH_PAYLOAD" | node -e " + const { resolveCodeReviewDepth } = require('./gsd-core/bin/lib/code-review-depth.cjs'); + const input = JSON.parse(require('fs').readFileSync('/dev/stdin', 'utf-8')); + process.stdout.write(JSON.stringify(resolveCodeReviewDepth(input))); +") + +DEPTH_OK=$(echo "$DEPTH_JSON" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf-8')).ok))") +``` + +The guard and the extraction it protects must run as one shell control-flow decision — a prose sentence between two fenced blocks is not a guard, since fenced blocks do not share shell state. Anything other than the literal string `true` (including an empty `DEPTH_OK`, which is what a crashed or missing resolver produces) is treated as failure, and the failure branch exits before any extraction can run: + +```bash +if [ "$DEPTH_OK" = "true" ]; then + # Single spawn: emit all seven fields as Unit-Separator-delimited (U+001F) values, + # fixed order. Field '\x1f' (not '\n') is deliberate: bash's `read` treats '\n' as + # "IFS whitespace" and collapses runs of it, silently dropping an empty field (e.g. + # DEPTH_MATCHED_RULE_PATH when matchedRule is null) — '\x1f' is not IFS-whitespace, + # so each empty field survives as its own zero-length token. This is safe only + # because validateRulePath (src/code-review-depth.cts) rejects any rule path + # carrying an interior control character, including U+001F itself — so + # DEPTH_MATCHED_RULE_PATH below can never collide with the delimiter. Do not + # remove that validation without revisiting this split. + DEPTH_FIELDS=$(echo "$DEPTH_JSON" | node -e " + const d = JSON.parse(require('fs').readFileSync('/dev/stdin', 'utf-8')); + const r = d.matchedRule; + process.stdout.write([ + d.depth, + d.source, + r ? String(r.index) : '', + r ? r.path : '', + String(!!d.invalidFlagDepth), + String(!!d.invalidConfigDepth), + String(!!d.downgraded), + ].join('\x1f')); + ") + IFS=$'\x1f' read -r -d '' REVIEW_DEPTH DEPTH_SOURCE DEPTH_MATCHED_RULE_INDEX DEPTH_MATCHED_RULE_PATH \ + DEPTH_INVALID_FLAG DEPTH_INVALID_CONFIG DEPTH_DOWNGRADED <<< "$DEPTH_FIELDS" || true + # <<< always appends a trailing newline to its input; strip it from the last field. + DEPTH_DOWNGRADED="${DEPTH_DOWNGRADED%$'\n'}" + + case "$DEPTH_SOURCE" in + flag) DEPTH_PROVENANCE="from --depth flag" ;; + rule) DEPTH_PROVENANCE="matched rule ${DEPTH_MATCHED_RULE_INDEX}: ${DEPTH_MATCHED_RULE_PATH}" ;; + config) DEPTH_PROVENANCE="from workflow.code_review_depth" ;; + *) DEPTH_PROVENANCE="default" ;; + esac + echo "Review depth: ${REVIEW_DEPTH} (${DEPTH_PROVENANCE})" + + if [ "$DEPTH_INVALID_FLAG" = "true" ]; then + echo "Warning: Invalid depth '${DEPTH_OVERRIDE}'. Valid values: quick, standard, deep. Using 'standard'." + fi + if [ "$DEPTH_INVALID_CONFIG" = "true" ]; then + echo "Warning: Invalid depth '${CONFIG_DEPTH}'. Valid values: quick, standard, deep. Using 'standard'." + fi + + if [ "$DEPTH_DOWNGRADED" = "true" ]; then + if [ -n "$DEPTH_MATCHED_RULE_INDEX" ]; then + echo "Switching from deep to standard depth for large file count (overrides matched rule ${DEPTH_MATCHED_RULE_INDEX}: ${DEPTH_MATCHED_RULE_PATH})." + else + echo "Switching from deep to standard depth for large file count." + fi + fi +else + # DEPTH_OK is anything but the literal string "true" — including empty, which is + # what a crashed or missing resolver produces. workflow.code_review_depth_overrides + # is misconfigured. Print every collected error — never fall back to a default + # depth, since silently reviewing a misconfigured sensitive-path policy at + # `standard` is the exact hole this feature closes — then hard-stop before + # REVIEW_DEPTH can be read by any later step. + echo "$DEPTH_JSON" | node -e " + let parsed; + try { + parsed = JSON.parse(require('fs').readFileSync('/dev/stdin', 'utf-8')); + } catch { + parsed = {}; + } + const errors = Array.isArray(parsed.errors) ? parsed.errors : []; + for (const err of errors) { + const parts = []; + if (err.ruleIndex !== undefined) parts.push('rule ' + err.ruleIndex); + if (err.path !== undefined) parts.push('path \"' + err.path + '\"'); + if (err.value !== undefined) parts.push('depth \"' + err.value + '\"'); + console.error('Error: workflow.code_review_depth_overrides' + (parts.length ? ' (' + parts.join(', ') + ')' : '') + ': ' + err.reason); + } + " + echo "Error: Fix workflow.code_review_depth_overrides and retry." + echo "Error: Depth resolution failed (DEPTH_OK=\"${DEPTH_OK}\"). Halting before agent spawn." + exit 1 +fi +``` +This `if`/`else`/`fi` is the entire guard: when `DEPTH_OK` is not the literal string `true`, execution never reaches the `DEPTH_FIELDS`/`REVIEW_DEPTH` extraction — the `else` branch prints the errors, prints the final `Error:` line above, and `exit 1`s out of the fenced block, so `REVIEW_DEPTH` is never set. Exit workflow. Do NOT spawn agent or create REVIEW.md. + + If REVIEW_FILES is empty: ``` @@ -616,7 +696,7 @@ Display inline summary to user: ─────────────────────────────────────────────────────────────── - Depth: ${REVIEW_DEPTH} + Depth: ${REVIEW_DEPTH} (${DEPTH_PROVENANCE}) Files Reviewed: ${FILES_REVIEWED} Findings: diff --git a/src/code-review-depth.cts b/src/code-review-depth.cts new file mode 100644 index 000000000..975c97360 --- /dev/null +++ b/src/code-review-depth.cts @@ -0,0 +1,408 @@ +/** + * Path-scoped code-review depth resolution for #2554 (ADR-457 build-at-publish: + * a pure TypeScript source of truth compiled to + * gsd-core/bin/lib/code-review-depth.cjs). + * + * Zero I/O, zero clock, zero deps. Resolves the effective review depth + * (`quick` | `standard` | `deep`) from, in priority order: the `--depth=` + * invocation flag, the strongest matching path-scoped rule in + * `workflow.code_review_depth_overrides`, the global `workflow.code_review_depth` + * config value, and finally the `standard` default — then applies the + * pre-existing large-scope `deep` → `standard` downgrade. + * + * See .gsd/phase/feat-2554-support-path-scoped-code-review-depth-ov/40-design.md + * for the full behavior table and negative-space notes this module encodes. + */ + +/** Depth tiers, weakest to strongest. */ +export type DepthTier = 'quick' | 'standard' | 'deep'; + +/** One `workflow.code_review_depth_overrides` rule entry, pre-validation. */ +export interface CodeReviewDepthRule { + paths: string[]; + depth: DepthTier; +} + +/** Frozen enum of validation-failure reasons (CONTRIBUTING's typed-reason rule). */ +export type CodeReviewDepthReasonKey = + | 'NOT_AN_ARRAY' + | 'RULE_NOT_OBJECT' + | 'PATHS_MALFORMED' + | 'INVALID_DEPTH' + | 'GLOB_UNSUPPORTED' + | 'PATH_CONTROL_CHAR' + | 'PATH_TRAVERSAL' + | 'PATH_ABSOLUTE' + | 'PATH_EMPTY'; + +export const REASON: Readonly> = Object.freeze({ + NOT_AN_ARRAY: 'not_an_array', + RULE_NOT_OBJECT: 'rule_not_object', + PATHS_MALFORMED: 'paths_malformed', + INVALID_DEPTH: 'invalid_depth', + GLOB_UNSUPPORTED: 'glob_unsupported', + PATH_CONTROL_CHAR: 'path_control_char', + PATH_TRAVERSAL: 'path_traversal', + PATH_ABSOLUTE: 'path_absolute', + PATH_EMPTY: 'path_empty', +}); + +/** Depth tiers, weakest → strongest. Frozen. */ +export const DEPTH_TIERS: readonly DepthTier[] = Object.freeze(['quick', 'standard', 'deep']); + +/** + * A `deep` resolution over more than this many changed files downgrades to + * `standard` (pre-existing scope guard; see gsd-core/workflows/code-review.md). + */ +export const LARGE_SCOPE_THRESHOLD = 50; + +/** One validation-failure entry. `ruleIndex`/`path`/`value` are present when applicable. */ +export interface CodeReviewDepthError { + reason: string; + ruleIndex?: number; + path?: string; + value?: unknown; +} + +/** The rule that decided the resolved depth, when `source === 'rule'`. */ +export interface MatchedRule { + index: number; + path: string; + depth: DepthTier; +} + +export interface ResolveCodeReviewDepthInput { + /** `--depth=` flag value, or '' if not supplied. */ + flagDepth?: string; + /** `workflow.code_review_depth` config value, or '' if unset. */ + configDepth?: string; + /** `workflow.code_review_depth_overrides` config value — validated here, so `unknown`. */ + overrides?: unknown; + /** The review's changed-file set (plain strings, any of the normalizable shapes). */ + files: string[]; + /** Repo root, for relativizing absolute file paths. */ + repoRoot: string; +} + +export interface ResolveCodeReviewDepthOk { + ok: true; + depth: DepthTier; + /** The tier before the large-scope downgrade, if any, was applied. */ + resolvedDepth: DepthTier; + source: 'flag' | 'rule' | 'config' | 'default'; + matchedRule: MatchedRule | null; + downgraded: boolean; + fileCount: number; + invalidFlagDepth?: true; + invalidConfigDepth?: true; +} + +export interface ResolveCodeReviewDepthErr { + ok: false; + errors: CodeReviewDepthError[]; +} + +export type ResolveCodeReviewDepthResult = ResolveCodeReviewDepthOk | ResolveCodeReviewDepthErr; + +/** True for a non-null, non-array object — the shape a JSON rule entry must have. */ +function isPlainObject(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +/** + * Normalize a repo-relative-ish path for matching: coerce non-strings to '', + * trim whitespace/CR, convert `\` to `/` unconditionally + * (RULESET.CONTENT-PATH-NORMALIZATION — never conditional on platform), collapse + * repeated `/`, strip a leading `repoRoot/` prefix, strip leading `./` segments + * (repeatably), and strip leading/trailing `/` — but ONLY when the path was + * relativized against `repoRoot` (or was never absolute to begin with). An + * absolute input (POSIX `/…` or a Windows drive-absolute `C:/…`) that is not + * under `repoRoot` stays absolute, so it can never spuriously match a + * repo-relative rule path in `ruleMatchesFile`. Idempotent. + */ +export function normalizeRelPath(p: unknown, repoRoot?: string): string { + let value = typeof p === 'string' ? p : ''; + value = value.trim(); + value = value.replace(/\\/g, '/'); + value = value.replace(/\/+/g, '/'); + + const isAbsolute = value.startsWith('/') || /^[A-Za-z]:\//.test(value); + let relativized = false; + + if (typeof repoRoot === 'string' && repoRoot !== '') { + const rootNormalized = repoRoot.trim().replace(/\\/g, '/').replace(/\/+$/, ''); + if (rootNormalized !== '' && value.startsWith(`${rootNormalized}/`)) { + value = value.slice(rootNormalized.length + 1); + relativized = true; + } + } + + let prev: string; + do { + prev = value; + if (value.startsWith('./')) { + value = value.slice(2); + } + } while (value !== prev); + + if ((relativized || !isAbsolute) && value.startsWith('/')) { + value = value.slice(1); + } + if (value.endsWith('/') && value !== '/') { + value = value.slice(0, -1); + } + + return value; +} + +/** + * Segment-aware match: true iff `filePath === rulePath` or `filePath` starts + * with `rulePath + '/'`. Both arguments must already be normalized. Case-sensitive. + */ +export function ruleMatchesFile(rulePath: string, filePath: string): boolean { + return filePath === rulePath || filePath.startsWith(`${rulePath}/`); +} + +interface RulePathValidation { + error: CodeReviewDepthReasonKey | null; + normalized?: string; +} + +/** + * Validate + normalize a single rule path (not yet matched against files). + * Check order: glob syntax, interior control character (U+0000-U+001F or + * U+007F, surviving the leading/trailing trim), `..` traversal segment, + * absolute (leading `/` with real content, or a Windows drive letter), then + * empty-after-normalization (covers a bare `/`, `.`, `./`, or whitespace-only + * path). + */ +function validateRulePath(rawPath: string): RulePathValidation { + const trimmed = rawPath.trim(); + const slashified = trimmed.replace(/\\/g, '/'); + + if (/[*?]/.test(slashified)) { + return { error: 'GLOB_UNSUPPORTED' }; + } + + if (/[\x00-\x1f\x7f]/.test(slashified)) { + return { error: 'PATH_CONTROL_CHAR' }; + } + + const collapsed = slashified.replace(/\/+/g, '/'); + const segments = collapsed.split('/'); + if (segments.includes('..')) { + return { error: 'PATH_TRAVERSAL' }; + } + + const looksAbsolute = slashified.startsWith('/') || /^[A-Za-z]:/.test(slashified); + + let normalized = collapsed; + let prev: string; + do { + prev = normalized; + if (normalized.startsWith('./')) { + normalized = normalized.slice(2); + } + } while (normalized !== prev); + if (normalized.startsWith('/')) { + normalized = normalized.slice(1); + } + if (normalized.endsWith('/')) { + normalized = normalized.slice(0, -1); + } + + if (looksAbsolute && normalized !== '' && normalized !== '.') { + return { error: 'PATH_ABSOLUTE' }; + } + if (normalized === '' || normalized === '.') { + return { error: 'PATH_EMPTY' }; + } + + return { error: null, normalized }; +} + +interface ValidatedRule { + index: number; + paths: string[]; + depth: DepthTier; +} + +/** Validate the full `overrides` value. Collects every malformed-rule error, in rule order. */ +function validateOverrides( + overridesInput: unknown, +): { ok: true; rules: ValidatedRule[] } | { ok: false; errors: CodeReviewDepthError[] } { + const overridesValue = overridesInput === undefined ? [] : overridesInput; + + if (!Array.isArray(overridesValue)) { + return { ok: false, errors: [{ reason: REASON.NOT_AN_ARRAY }] }; + } + + const errors: CodeReviewDepthError[] = []; + const rules: ValidatedRule[] = []; + + for (let index = 0; index < overridesValue.length; index += 1) { + const rule: unknown = overridesValue[index]; + + if (!isPlainObject(rule)) { + errors.push({ reason: REASON.RULE_NOT_OBJECT, ruleIndex: index }); + continue; + } + + const hasPaths = Object.prototype.hasOwnProperty.call(rule, 'paths'); + const rawPaths = hasPaths ? rule.paths : undefined; + if ( + !Array.isArray(rawPaths) + || rawPaths.length === 0 + || rawPaths.some((entry: unknown) => typeof entry !== 'string') + ) { + errors.push({ reason: REASON.PATHS_MALFORMED, ruleIndex: index }); + continue; + } + + let pathError: CodeReviewDepthError | null = null; + const normalizedPaths: string[] = []; + for (const rawPath of rawPaths as string[]) { + const validation = validateRulePath(rawPath); + if (validation.error) { + pathError = { reason: REASON[validation.error], ruleIndex: index, path: rawPath }; + break; + } + normalizedPaths.push(validation.normalized as string); + } + if (pathError) { + errors.push(pathError); + continue; + } + + const hasDepth = Object.prototype.hasOwnProperty.call(rule, 'depth'); + const depthValue = hasDepth ? rule.depth : undefined; + if (typeof depthValue !== 'string' || !DEPTH_TIERS.includes(depthValue as DepthTier)) { + errors.push({ reason: REASON.INVALID_DEPTH, ruleIndex: index, value: depthValue }); + continue; + } + + rules.push({ index, paths: normalizedPaths, depth: depthValue as DepthTier }); + } + + if (errors.length > 0) { + return { ok: false, errors }; + } + return { ok: true, rules }; +} + +interface FinalizeInput { + resolvedDepth: DepthTier; + source: 'flag' | 'rule' | 'config' | 'default'; + matchedRule: MatchedRule | null; + fileCount: number; + invalidFlagDepth?: true; + invalidConfigDepth?: true; +} + +/** Applies the large-scope `deep` → `standard` downgrade and assembles the ok result. */ +function finalize(input: FinalizeInput): ResolveCodeReviewDepthOk { + const downgraded = input.resolvedDepth === 'deep' && input.fileCount > LARGE_SCOPE_THRESHOLD; + const result: ResolveCodeReviewDepthOk = { + ok: true, + depth: downgraded ? 'standard' : input.resolvedDepth, + resolvedDepth: input.resolvedDepth, + source: input.source, + matchedRule: input.matchedRule, + downgraded, + fileCount: input.fileCount, + }; + if (input.invalidFlagDepth) result.invalidFlagDepth = true; + if (input.invalidConfigDepth) result.invalidConfigDepth = true; + return result; +} + +/** + * Resolve the effective code-review depth. Overrides are always validated + * first, even when the `--depth=` flag would win outright — a malformed + * config must never silently pass through unreported. See the design doc's + * behavior table for the full row-by-row contract. + */ +export function resolveCodeReviewDepth(input: ResolveCodeReviewDepthInput): ResolveCodeReviewDepthResult { + const { flagDepth, configDepth, overrides, files, repoRoot } = input; + + const validated = validateOverrides(overrides); + if (!validated.ok) { + return { ok: false, errors: validated.errors }; + } + const rules = validated.rules; + + if (typeof flagDepth === 'string' && flagDepth !== '') { + if (DEPTH_TIERS.includes(flagDepth as DepthTier)) { + return finalize({ + resolvedDepth: flagDepth as DepthTier, + source: 'flag', + matchedRule: null, + fileCount: files.length, + }); + } + return finalize({ + resolvedDepth: 'standard', + source: 'flag', + matchedRule: null, + fileCount: files.length, + invalidFlagDepth: true, + }); + } + + const normalizedFiles = files.map((f) => normalizeRelPath(f, repoRoot)); + let best: MatchedRule | null = null; + let bestTierIndex = -1; + for (const rule of rules) { + const tierIndex = DEPTH_TIERS.indexOf(rule.depth); + let matchedPath: string | null = null; + for (const rulePath of rule.paths) { + if (normalizedFiles.some((file) => ruleMatchesFile(rulePath, file))) { + matchedPath = rulePath; + break; + } + } + if (matchedPath === null) continue; + if (best === null || tierIndex > bestTierIndex) { + best = { index: rule.index, path: matchedPath, depth: rule.depth }; + bestTierIndex = tierIndex; + } + } + if (best !== null) { + return finalize({ + resolvedDepth: best.depth, + source: 'rule', + matchedRule: best, + fileCount: files.length, + }); + } + + // No rule matched this file set. Provenance depends solely on whether a + // global config depth was configured — the presence, absence, or emptiness + // of `overrides` has no bearing here (a validly-configured rule set that + // simply didn't match this review is indistinguishable, for provenance + // purposes, from no rule set at all). + if (typeof configDepth === 'string' && configDepth !== '') { + if (DEPTH_TIERS.includes(configDepth as DepthTier)) { + return finalize({ + resolvedDepth: configDepth as DepthTier, + source: 'config', + matchedRule: null, + fileCount: files.length, + }); + } + return finalize({ + resolvedDepth: 'standard', + source: 'config', + matchedRule: null, + fileCount: files.length, + invalidConfigDepth: true, + }); + } + + return finalize({ + resolvedDepth: 'standard', + source: 'default', + matchedRule: null, + fileCount: files.length, + }); +} diff --git a/tests/code-review-depth.test.cjs b/tests/code-review-depth.test.cjs new file mode 100644 index 000000000..de8c1f9fc --- /dev/null +++ b/tests/code-review-depth.test.cjs @@ -0,0 +1,920 @@ +/** + * Failing-first (RED) tests for #2554 — path-scoped code-review depth overrides. + * + * Binds to the not-yet-built module `gsd-core/bin/lib/code-review-depth.cjs` + * (see .gsd/phase/feat-2554-support-path-scoped-code-review-depth-ov/40-design.md + * for the behavior table and .gsd/phase/.../50-test-matrix.md for the case + * matrix these tests are numbered against). The module does not exist yet — + * this file is expected to fail until it is built. + * + * Rows 1-41 and 44-45 (matrix numbering) are pure-function unit tests against + * the resolver directly — no fs, no clock, no subprocess. Rows 42-43 are CLI + * integration tests through the real `gsd-tools` binary (via runGsdTools), + * because CONTRIBUTING names the CLI round-trip as the only non-source-grep + * way to prove a config key is registered and survives a load. Row 44 has no + * quoted "Test name" in the matrix (asserted structurally by construction: every + * test below builds its own rules/files fixtures, no module-level mutable + * state) so it is not given its own test(). + */ +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const fc = require('fast-check'); +const { + REASON, + DEPTH_TIERS, + LARGE_SCOPE_THRESHOLD, + ruleMatchesFile, + resolveCodeReviewDepth, + normalizeRelPath, +} = require('../gsd-core/bin/lib/code-review-depth.cjs'); +const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); + +// ─── enum lock ──────────────────────────────────────────────────────────── + +describe('REASON enum', () => { + test('REASON is frozen and carries exactly the nine documented keys', () => { + assert.strictEqual(Object.isFrozen(REASON), true); + assert.deepEqual( + Object.keys(REASON).sort(), + [ + 'GLOB_UNSUPPORTED', + 'INVALID_DEPTH', + 'NOT_AN_ARRAY', + 'PATHS_MALFORMED', + 'PATH_ABSOLUTE', + 'PATH_CONTROL_CHAR', + 'PATH_EMPTY', + 'PATH_TRAVERSAL', + 'RULE_NOT_OBJECT', + ].sort(), + ); + }); +}); + +// ─── resolution order (rows 1-10) ────────────────────────────────────────── + +describe('resolveCodeReviewDepth — resolution order', () => { + test('resolves to standard when nothing is configured', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [], + files: ['src/lib/a.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'standard'); + assert.strictEqual(result.source, 'default'); + }); + + test('falls through to the global config depth', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: 'deep', + overrides: [], + files: ['src/lib/a.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'deep'); + assert.strictEqual(result.source, 'config'); + }); + + test('escalates when a changed file sits under a rule path', () => { + // Production shape: plain cwd-relative POSIX strings, as git diff --name-only emits. + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: ['src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'deep'); + assert.strictEqual(result.source, 'rule'); + assert.deepEqual(result.matchedRule, { index: 0, path: 'src/auth', depth: 'deep' }); + }); + + test('ignores rules that match no changed file', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: ['src/billing/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'standard'); + assert.strictEqual(result.source, 'default'); + assert.strictEqual(result.matchedRule, null); + }); + + test('a single matched file escalates the whole review', () => { + // Production shape: plain cwd-relative POSIX strings. + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: ['src/lib/a.ts', 'src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'deep'); + assert.strictEqual(result.source, 'rule'); + }); + + test('takes the strongest tier among matching rules', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [ + { paths: ['src/lib'], depth: 'quick' }, + { paths: ['src/auth'], depth: 'deep' }, + ], + files: ['src/lib/a.ts', 'src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'deep'); + assert.strictEqual(result.source, 'rule'); + assert.strictEqual(result.matchedRule.path, 'src/auth'); + }); + + test('reports the first rule declared at the winning tier', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [ + { paths: ['src/auth'], depth: 'deep' }, + { paths: ['src/billing'], depth: 'deep' }, + ], + files: ['src/auth/x.ts', 'src/billing/y.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'deep'); + assert.deepEqual(result.matchedRule, { index: 0, path: 'src/auth', depth: 'deep' }); + }); + + test('a matching rule replaces the global depth even when weaker', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: 'deep', + overrides: [{ paths: ['src/auth'], depth: 'quick' }], + files: ['src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'quick'); + assert.strictEqual(result.source, 'rule'); + }); + + test('the invocation flag outranks every configured rule', () => { + const result = resolveCodeReviewDepth({ + flagDepth: 'deep', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'quick' }], + files: ['src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'deep'); + assert.strictEqual(result.source, 'flag'); + }); + + test('validates rules even when the flag would win', () => { + const result = resolveCodeReviewDepth({ + flagDepth: 'deep', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'not-a-tier' }], + files: ['src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, false); + assert.strictEqual(result.errors[0].reason, REASON.INVALID_DEPTH); + }); +}); + +// ─── segment-aware matching / negative space (rows 11-14, 26 counterpart) ── + +describe('resolveCodeReviewDepth — segment-aware matching', () => { + test('does not match a sibling directory sharing a name prefix', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: ['src/authfoo/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'standard'); + assert.strictEqual(result.matchedRule, null); + }); + + test('anchors the prefix at the path root, not anywhere in the path', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: ['docs/src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'standard'); + assert.strictEqual(result.matchedRule, null); + }); + + test('treats a trailing slash as insignificant', () => { + const withSlash = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth/'], depth: 'deep' }], + files: ['src/auth/x.ts'], + repoRoot: '/repo', + }); + const withoutSlash = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: ['src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.deepEqual(withSlash, withoutSlash); + }); + + test('a file-shaped rule matches only that exact file', () => { + const hit = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth.ts'], depth: 'deep' }], + files: ['src/auth.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(hit.ok, true); + assert.strictEqual(hit.depth, 'deep'); + assert.strictEqual(hit.source, 'rule'); + + const miss = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth.ts'], depth: 'deep' }], + files: ['src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(miss.ok, true); + assert.strictEqual(miss.depth, 'standard'); + assert.strictEqual(miss.matchedRule, null); + }); +}); + +// ─── path normalization (rows 15-19) ─────────────────────────────────────── + +describe('resolveCodeReviewDepth — path normalization', () => { + test('normalizes a leading dot-slash on a changed file', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: ['./src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'deep'); + }); + + test('normalizes backslash separators on a changed file', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: ['src\\auth\\x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'deep'); + }); + + test('normalizes backslash separators in a rule path', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src\\auth'], depth: 'deep' }], + files: ['src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'deep'); + }); + + test('relativizes an absolute changed-file path under the repo root', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: ['/repo/src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'deep'); + assert.strictEqual(result.source, 'rule'); + }); + + test('an out-of-repo absolute path matches no repo-relative rule', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: ['/home/user/.claude/notes.md'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'standard'); + assert.strictEqual(result.matchedRule, null); + + // Discriminating case: a filesystem-root absolute path that merely + // *looks* like it could be repo-relative once a leading `/` is + // stripped. If normalizeRelPath ever unconditionally strips the + // leading `/`, this would false-escalate by matching rule `src/auth`. + const outsideRepo = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: ['/src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(outsideRepo.ok, true); + assert.strictEqual(outsideRepo.depth, 'standard'); + assert.strictEqual(outsideRepo.matchedRule, null); + + // Sibling case: the same file, but genuinely under repoRoot, must + // still relativize and match — proving the fix does not over-correct. + const insideRepo = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: ['/repo/src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(insideRepo.ok, true); + assert.strictEqual(insideRepo.source, 'rule'); + assert.strictEqual(insideRepo.matchedRule && insideRepo.matchedRule.path, 'src/auth'); + + // Unit-level pin on normalizeRelPath itself. + assert.strictEqual(normalizeRelPath('/src/auth/x.ts', '/repo'), '/src/auth/x.ts'); + }); +}); + +// ─── totality on empty inputs (rows 20-21) ───────────────────────────────── + +describe('resolveCodeReviewDepth — totality on empty inputs', () => { + test('is total on an empty changed-file set', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'standard'); + assert.strictEqual(result.source, 'default'); + assert.strictEqual(result.fileCount, 0); + assert.strictEqual(result.matchedRule, null); + }); + + test('an empty rule array is valid and changes nothing', () => { + const withEmptyArray = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: 'quick', + overrides: [], + files: ['src/lib/a.ts'], + repoRoot: '/repo', + }); + const withNoOverridesField = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: 'quick', + files: ['src/lib/a.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(withEmptyArray.ok, true); + assert.strictEqual(withEmptyArray.depth, 'quick'); + assert.strictEqual(withEmptyArray.source, 'config'); + assert.deepEqual(withEmptyArray, withNoOverridesField); + }); +}); + +// ─── malformed rule paths (rows 22-27) ───────────────────────────────────── + +describe('resolveCodeReviewDepth — malformed rule paths', () => { + test('rejects glob syntax with a configuration error', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth/**'], depth: 'deep' }], + files: ['src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, false); + assert.strictEqual(result.errors[0].reason, REASON.GLOB_UNSUPPORTED); + assert.strictEqual(result.errors[0].ruleIndex, 0); + }); + + test('rejects single-star and question-mark globs', () => { + const star = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/*.ts'], depth: 'deep' }], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(star.ok, false); + assert.strictEqual(star.errors[0].reason, REASON.GLOB_UNSUPPORTED); + + const question = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['a?b'], depth: 'deep' }], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(question.ok, false); + assert.strictEqual(question.errors[0].reason, REASON.GLOB_UNSUPPORTED); + }); + + test('rejects a rule path containing a parent-directory segment', () => { + for (const badPath of ['../etc', 'src/../../etc']) { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: [badPath], depth: 'deep' }], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, false, `expected ${badPath} to fail`); + assert.strictEqual(result.errors[0].reason, REASON.PATH_TRAVERSAL); + } + }); + + test('rejects an absolute rule path', () => { + for (const badPath of ['/etc/passwd', 'C:\\Windows']) { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: [badPath], depth: 'deep' }], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, false, `expected ${badPath} to fail`); + assert.strictEqual(result.errors[0].reason, REASON.PATH_ABSOLUTE); + } + }); + + test('rejects a rule path that normalizes to nothing', () => { + for (const badPath of ['', ' ', '/', '.', './']) { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: [badPath], depth: 'deep' }], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, false, `expected ${JSON.stringify(badPath)} to fail`); + assert.strictEqual(result.errors[0].reason, REASON.PATH_EMPTY); + } + }); + + test('trims whitespace and CR before validating a rule path', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: [' src/auth\r\n'], depth: 'deep' }], + files: ['src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'deep'); + assert.strictEqual(result.matchedRule.path, 'src/auth'); + }); + + test('rejects a rule path containing an interior control character', () => { + // 'src/au th' below is 'src/au\tth' (tab); 'src/auth' below is + // 'src/au\x7fth' (DEL) — both non-printing, so they render as + // near-invisible in this test's source. + const cases = [ + ['src/au\nth', 'embedded newline'], + ['src/au\rth', 'embedded carriage return'], + ['src/au\tth', 'embedded tab'], + ['src/au\x7fth', 'embedded DEL'], + ]; + for (const [badPath, label] of cases) { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: [badPath], depth: 'deep' }], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, false, `expected ${label} to fail`); + assert.strictEqual(result.errors[0].reason, REASON.PATH_CONTROL_CHAR, `expected ${label} to fail`); + } + }); + + test('a glob-and-control-char path still reports GLOB_UNSUPPORTED (precedence)', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/*\nauth'], depth: 'deep' }], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, false); + assert.strictEqual(result.errors[0].reason, REASON.GLOB_UNSUPPORTED); + }); +}); + +// ─── malformed rule shapes (rows 28-34) ──────────────────────────────────── + +describe('resolveCodeReviewDepth — malformed rule shapes', () => { + test('rejects a rule depth outside quick, standard and deep', () => { + for (const badDepth of ['light', 'DEEP', '']) { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: badDepth }], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, false, `expected depth ${JSON.stringify(badDepth)} to fail`); + assert.strictEqual(result.errors[0].reason, REASON.INVALID_DEPTH); + } + }); + + test('rejects a rule with no depth', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'] }], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, false); + assert.strictEqual(result.errors[0].reason, REASON.INVALID_DEPTH); + }); + + test('rejects a rule whose paths is absent, empty, or not a string array', () => { + const variants = [{ depth: 'deep' }, { paths: [], depth: 'deep' }, { paths: 'src', depth: 'deep' }]; + for (const rule of variants) { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [rule], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, false, `expected ${JSON.stringify(rule)} to fail`); + assert.strictEqual(result.errors[0].reason, REASON.PATHS_MALFORMED); + } + }); + + test('rejects a paths array containing a non-string', () => { + const withNumber = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['ok', 42], depth: 'deep' }], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(withNumber.ok, false); + assert.strictEqual(withNumber.errors[0].reason, REASON.PATHS_MALFORMED); + + const withNull = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['ok', null], depth: 'deep' }], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(withNull.ok, false); + assert.strictEqual(withNull.errors[0].reason, REASON.PATHS_MALFORMED); + }); + + test('rejects an overrides value that is valid JSON but not an array', () => { + for (const badOverrides of [{}, 'deep', 0, true, null]) { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: badOverrides, + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, false, `expected ${JSON.stringify(badOverrides)} to fail`); + assert.strictEqual(result.errors[0].reason, REASON.NOT_AN_ARRAY); + } + }); + + test('rejects a rule entry that is not an object', () => { + for (const badRule of ['src/auth', [], null, 0]) { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [badRule], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, false, `expected ${JSON.stringify(badRule)} to fail`); + assert.strictEqual(result.errors[0].reason, REASON.RULE_NOT_OBJECT); + } + }); + + test('reports every malformed rule, not only the first', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [ + { paths: ['src/auth/**'], depth: 'deep' }, + { paths: ['src/billing'], depth: 'nonsense' }, + ], + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, false); + assert.strictEqual(result.errors.length, 2); + assert.strictEqual(result.errors[0].reason, REASON.GLOB_UNSUPPORTED); + assert.strictEqual(result.errors[0].ruleIndex, 0); + assert.strictEqual(result.errors[1].reason, REASON.INVALID_DEPTH); + assert.strictEqual(result.errors[1].ruleIndex, 1); + }); +}); + +// ─── hostile input (row 35-36) ───────────────────────────────────────────── + +describe('resolveCodeReviewDepth — hostile input', () => { + test('a rule carrying a __proto__ key does not pollute Object.prototype', () => { + const hostileRule = JSON.parse('{"__proto__": {"polluted": true}, "paths": ["src/auth"], "depth": "deep"}'); + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [hostileRule], + files: ['src/auth/x.ts'], + repoRoot: '/repo', + }); + assert.strictEqual(({}).polluted, undefined); + // The call must still behave sanely — not necessarily error, but must not + // let the pollution attempt masquerade as a valid escalation. + assert.ok(result.ok === true || result.ok === false); + }); + + test('accepts unusual but well-formed rule paths', () => { + const longSegment = 'a'.repeat(4096); + const rulePath = `café/日本語 dir/${longSegment}`; + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: [rulePath], depth: 'deep' }], + files: [`${rulePath}/x.ts`], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'deep'); + assert.strictEqual(result.source, 'rule'); + }); +}); + +// ─── large-scope downgrade boundary (rows 37-39) ─────────────────────────── + +describe('resolveCodeReviewDepth — large-scope downgrade boundary', () => { + function filesUnder(rulePath, count) { + return Array.from({ length: count }, (_, i) => `${rulePath}/f${i}.ts`); + } + + test('applies the large-scope downgrade at the file-count boundary', () => { + const below = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: filesUnder('src/auth', LARGE_SCOPE_THRESHOLD - 1), + repoRoot: '/repo', + }); + assert.strictEqual(below.ok, true); + assert.strictEqual(below.depth, 'deep'); + assert.strictEqual(below.resolvedDepth, 'deep'); + assert.strictEqual(below.downgraded, false); + + const at = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: filesUnder('src/auth', LARGE_SCOPE_THRESHOLD), + repoRoot: '/repo', + }); + assert.strictEqual(at.ok, true); + assert.strictEqual(at.depth, 'deep'); + assert.strictEqual(at.resolvedDepth, 'deep'); + assert.strictEqual(at.downgraded, false); + + const above = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides: [{ paths: ['src/auth'], depth: 'deep' }], + files: filesUnder('src/auth', LARGE_SCOPE_THRESHOLD + 1), + repoRoot: '/repo', + }); + assert.strictEqual(above.ok, true); + assert.strictEqual(above.depth, 'standard'); + assert.strictEqual(above.resolvedDepth, 'deep'); + assert.strictEqual(above.downgraded, true); + assert.strictEqual(above.source, 'rule'); + assert.deepEqual(above.matchedRule, { index: 0, path: 'src/auth', depth: 'deep' }); + }); + + test('downgrades a flag-sourced deep without naming a rule', () => { + const result = resolveCodeReviewDepth({ + flagDepth: 'deep', + configDepth: '', + overrides: [], + files: filesUnder('src/misc', LARGE_SCOPE_THRESHOLD + 1), + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'standard'); + assert.strictEqual(result.resolvedDepth, 'deep'); + assert.strictEqual(result.downgraded, true); + assert.strictEqual(result.source, 'flag'); + assert.strictEqual(result.matchedRule, null); + }); + + test('leaves a non-deep depth alone above the file-count threshold', () => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: 'standard', + overrides: [], + files: filesUnder('src/misc', LARGE_SCOPE_THRESHOLD + 1), + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, true); + assert.strictEqual(result.depth, 'standard'); + assert.strictEqual(result.resolvedDepth, 'standard'); + assert.strictEqual(result.downgraded, false); + }); +}); + +// ─── independence (row 45) ───────────────────────────────────────────────── + +describe('resolveCodeReviewDepth — independence', () => { + test('does not mutate its inputs', () => { + const overrides = [{ paths: ['src/auth'], depth: 'deep' }]; + const files = ['src/auth/x.ts', 'src/lib/a.ts']; + const overridesSnapshot = JSON.stringify(overrides); + const filesSnapshot = JSON.stringify(files); + + const first = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides, + files, + repoRoot: '/repo', + }); + const second = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides, + files, + repoRoot: '/repo', + }); + + assert.strictEqual(JSON.stringify(overrides), overridesSnapshot); + assert.strictEqual(JSON.stringify(files), filesSnapshot); + assert.deepEqual(first, second); + }); +}); + +// ─── property-based tests (rows 40-41, plus one additional) ─────────────── + +describe('resolveCodeReviewDepth — properties', () => { + const segment = fc.string({ + unit: fc.constantFrom(..."abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789".split('')), + minLength: 1, + maxLength: 8, + }); + const rulePathArb = fc.array(segment, { minLength: 1, maxLength: 3 }).map((parts) => parts.join('/')); + const depthArb = fc.constantFrom(...DEPTH_TIERS); + const ruleArb = fc.record({ paths: fc.array(rulePathArb, { minLength: 1, maxLength: 2 }), depth: depthArb }); + const fileArb = fc.array(rulePathArb, { minLength: 0, maxLength: 5 }).map((parts) => parts.join('/')); + + test('property: a successful resolution is internally consistent', () => { + fc.assert( + fc.property( + fc.array(ruleArb, { minLength: 0, maxLength: 4 }), + // Bounded well below LARGE_SCOPE_THRESHOLD so the large-scope downgrade + // (rows 37-39, tested separately above) never fires here — this + // property is about matchedRule/depth consistency, not the downgrade. + fc.array(fileArb, { minLength: 0, maxLength: 10 }), + depthArb, + (overrides, files, configDepth) => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth, + overrides, + files, + repoRoot: '/repo', + }); + // ruleArb/rulePathArb only ever generate well-formed rules (alnum + // segments, no glob/traversal/absolute chars), so this must hold. + assert.strictEqual(result.ok, true); + assert.ok(DEPTH_TIERS.includes(result.depth)); + if (result.source === 'rule') { + assert.ok(result.matchedRule !== null); + assert.strictEqual(result.matchedRule.depth, result.depth); + } + return true; + }, + ), + { numRuns: 200, seed: 25540 }, + ); + }); + + test('property: prefix matching is segment-aware', () => { + fc.assert( + fc.property(rulePathArb, segment, (rule, suffix) => { + assert.strictEqual(ruleMatchesFile(rule, `${rule}/${suffix}`), true); + assert.strictEqual(ruleMatchesFile(rule, `${rule}${suffix}`), false); + return true; + }), + { numRuns: 200, seed: 25540 }, + ); + }); + + test('property: an overrides value that is not a JSON array is always rejected with NOT_AN_ARRAY', () => { + const nonArrayArb = fc.oneof( + fc.integer(), + fc.string(), + fc.boolean(), + fc.constant(null), + fc.object(), + ); + fc.assert( + fc.property(nonArrayArb, (overrides) => { + const result = resolveCodeReviewDepth({ + flagDepth: '', + configDepth: '', + overrides, + files: [], + repoRoot: '/repo', + }); + assert.strictEqual(result.ok, false); + assert.strictEqual(result.errors[0].reason, REASON.NOT_AN_ARRAY); + return true; + }), + { numRuns: 200, seed: 25540 }, + ); + }); +}); + +// ─── config-key registration (rows 42-43) ────────────────────────────────── + +describe('workflow.code_review_depth_overrides — config registration', () => { + test('the new key survives a config-set / config-get round trip', (t) => { + const cwd = createTempProject(); + t.after(() => cleanup(cwd)); + + const value = JSON.stringify([{ paths: ['src/auth'], depth: 'deep' }]); + const setResult = runGsdTools( + ['config-set', 'workflow.code_review_depth_overrides', value, '--raw'], + cwd, + { HOME: cwd }, + ); + assert.strictEqual(setResult.success, true, `config-set failed: ${setResult.error}`); + + const getResult = runGsdTools( + ['config-get', 'workflow.code_review_depth_overrides'], + cwd, + { HOME: cwd }, + ); + assert.strictEqual(getResult.success, true, `config-get failed: ${getResult.error}`); + assert.deepEqual(JSON.parse(getResult.output), [{ paths: ['src/auth'], depth: 'deep' }]); + }); + + test('a configured override array survives a configuration load', (t) => { + const cwd = createTempProject(); + t.after(() => cleanup(cwd)); + + const ensureResult = runGsdTools('config-ensure-section', cwd, { HOME: cwd }); + assert.strictEqual(ensureResult.success, true, `config-ensure-section failed: ${ensureResult.error}`); + + const configPath = path.join(cwd, '.planning', 'config.json'); + const config = JSON.parse(fs.readFileSync(configPath, 'utf-8')); + config.workflow = config.workflow || {}; + config.workflow.code_review_depth_overrides = [{ paths: ['src/billing'], depth: 'quick' }]; + fs.writeFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8'); + + const getResult = runGsdTools( + ['config-get', 'workflow.code_review_depth_overrides'], + cwd, + { HOME: cwd }, + ); + assert.strictEqual(getResult.success, true, `config-get failed: ${getResult.error}`); + assert.deepEqual(JSON.parse(getResult.output), [{ paths: ['src/billing'], depth: 'quick' }]); + }); +}); diff --git a/tests/emitted-drift-acks/2554-code-review-depth-overrides.json b/tests/emitted-drift-acks/2554-code-review-depth-overrides.json new file mode 100644 index 000000000..563a83829 --- /dev/null +++ b/tests/emitted-drift-acks/2554-code-review-depth-overrides.json @@ -0,0 +1,8 @@ +{ + "version": 1, + "paths": { + "code-review.md": { + "reason": "#2554: +5376 bytes vs next. resolve_depth was rewritten to call the new path-scoped depth resolver (src/code-review-depth.cts) — the two config reads, the JSON payload construction that passes config via env vars and the changed-file list via stdin, an executable misconfiguration guard that halts the review instead of a prose instruction, the provenance/warning/downgrade output, and the large-scope downgrade relocated out of compute_file_scope so one step owns depth. Replaces the spent 3503-diff-base-scope-anchor.json fragment (its code-review.md entry was consumed when #3503 merged at 49b60070a and can no longer clear anything — the base file is exactly the 34435-byte baseline this growth is measured against), mirroring how #3503 itself replaced the spent 3191-unanchored-grep-sites.json." + } + } +} diff --git a/tests/emitted-drift-acks/3503-diff-base-scope-anchor.json b/tests/emitted-drift-acks/3503-diff-base-scope-anchor.json deleted file mode 100644 index 702a86b20..000000000 --- a/tests/emitted-drift-acks/3503-diff-base-scope-anchor.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "version": 1, - "paths": { - "code-review.md": "#3503: +915 bytes vs next. The emitted workflow copy grows with its source gsd-core/workflows/code-review.md — the scope-anchored diff-base grep, the PHASE_SCOPE_NUM padded/unpadded prep, and the #3503 contract comments at both derivation sites (Tier-3 fallback and spawn_reviewer). Replaces the spent 3191-unanchored-grep-sites.json fragment (its code-review.md entry was consumed when #3191 merged and could no longer clear anything). The source-path ripples (gsd-core/workflows/code-review.md, gsd-core/workflows/code-review/steps/structural-pre-pass.md) are identity-attributed by the table and need no ack." - } -}