* test(#2554): failing-first suite for path-scoped code review depth overrides Binds the not-yet-built code-review-depth module: segment-aware path-prefix matching of a changed-file set against ordered {paths,depth} rules, resolution order flag > strongest matching rule > global > standard, typed validation errors, and the large-scope downgrade boundary. Also proves behaviorally that workflow.code_review_depth_overrides is not yet a registered config key. Refs #2554 * feat(#2554): resolve code review depth from path-scoped override rules Adds workflow.code_review_depth_overrides — an ordered array of {paths, depth} rules matched against a review's changed-file set by segment-aware path-prefix comparison. Resolution order is --depth= flag, then the strongest matching rule, then workflow.code_review_depth, then standard; a matching rule replaces the global rather than being max'd with it, so quick and standard rules stay meaningful. Glob metacharacters are a hard configuration error rather than sugar for a prefix, and malformed rules halt the review instead of degrading to standard. The resolver is pure and reports its own provenance, so the workflow can print the resolved depth and the rule that matched. The pre-existing >50-file deep-to-standard downgrade moves into the module and now names the rule it overrode. The key is registered centrally rather than as a capability config slice: the federated slice channel admits only boolean/string/number/enum, so an array slice would be dropped as malformed. Closes #2554 * test(#2554): correct depth-provenance assertions and pin out-of-repo paths Two corrections to the failing-first suite. The source assertion for a non-matching rule with no global configured expected 'config'; with no global set the depth comes from the default, and a companion assertion tolerated either value, so both passed against an implementation that derived provenance from whether any rules existed rather than from where the depth came from. The out-of-repo absolute-path case used a home-directory path that matched neither implementation, so it never exercised the defect it named. It now pins the discriminating cases: an absolute path outside the repo root must not match a repo-relative rule, and one under the root must. * docs(#2554): document path-scoped code review depth overrides Reference rows for workflow.code_review_depth_overrides in the configuration, features and commands references plus the locale copies that carry those tables, and in the planning-config reference. Explanation of why escalation is whole-review rather than per-file and why v1 is prefix-only. New how-to for scoping review depth by path, carrying the configuration-error reason table and the distinction between nothing to report and could not look. CONTEXT.md glossary entry and the INVENTORY row for the new CLI module. ja-JP and ko-KR CONFIGURATION.md carry no code_review keys at all, and ko-KR and pt-BR FEATURES.md carry no code-review config table, so those files are deliberately untouched. * fix(#2554): make the depth-misconfiguration halt executable and reject control chars Three review findings, all in this change. The misconfiguration halt was prose rather than shell: the error-printing fence was followed by an unconditional extraction fence, so an ok:false result threw and left the depth empty instead of stopping the review. Prose is not a guard — the two fences are now one block with a real conditional, and anything that is not the literal string true fails closed. An interior control character in a rule path survived validation and reached the provenance string and the summary box; rule paths now reject control characters via a new PATH_CONTROL_CHAR reason, after the glob check so precedence is unchanged. That in turn makes the field record safe to delimit, so the seven node invocations that each re-parsed the same result to read one field collapse to one. Also corrects the glossary entry's illustrative paths, which the glossary-ref check read as real repository references. * fix(#2554): use the fast-check v4 string API and acknowledge workflow growth Two failures from the remote matrix on d3111f45, both this branch's. The property block built its segment arbitrary with fc.stringOf, removed in fast-check v4. Because the arbitrary is constructed in the describe body, the throw took out all four property tests rather than one — they had never executed. Rewritten to fc.string({unit, ...}), the form this repo already uses in emitted-attribution.test.cjs. Every other fast-check helper in the file was audited against the installed module. The emitted-attribution growth arm needed an acknowledgment for code-review.md, which grew 5376 bytes. The pre-existing 3503 fragment keying the same file is spent — its ripple was absorbed when #3503 merged, and the base file is exactly the 34435-byte baseline this growth is measured against — so it cannot clear anything, while the ack lint hard-fails on a duplicate key across two sources. Removed it in favor of the new fragment, which is exactly how #3503 itself replaced the spent 3191 fragment. * docs(#2554): backfill changeset PR number --------- Co-authored-by: sim <sim@local>
409 lines
13 KiB
TypeScript
409 lines
13 KiB
TypeScript
/**
|
|
* 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<Record<CodeReviewDepthReasonKey, string>> = 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<string, unknown> {
|
|
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,
|
|
});
|
|
}
|