golem15/msd-core
4
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

enhance(#2554): resolve code review depth from path-scoped override rules (#3695)

Browse Source 
* 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>
This commit is contained in:
Tom Boucher
2026-08-19 22:45:35 -04:00
committed by GitHub
parent 71e00d426e
commit 2fca0e17e4
27 changed files with 1629 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files

150
gsd-core/workflows/code-review.md
View File

@@ -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.
</step>
<step name="resolve_depth">
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
```
</step>
<step name="compute_file_scope">
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
```
</step>
<step name="resolve_depth">
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.
</step>
<step name="check_empty_scope">
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: