fix: actionable diagnostics, PR review comments, and SKILL.md size check#88
fix: actionable diagnostics, PR review comments, and SKILL.md size check#88
Conversation
Changeset summaryDetected 1 changeset file(s): .changeset/skill-authoring-size-limits.md: ---
"fusion-skill-authoring": patch
---
Document SKILL.md size limits and CI guardrails
- Document 300-line recommended limit (triggers CI warning)
- Document 500-line hard limit (fails CI)
- Clarify expectation to move overflow to references/ early
- Add failure signal for exceeding size thresholds
Relates to: equinor/fusion-core-tasks#84 |
…comments, SKILL.md size check - Add per-code fix hints to validate-scripts diagnostics - Separate regex-explanation issues from intent-comment heading - Add --json-output flag for structured CI diagnostics - Post inline PR review comments via actions/github-script - Dismiss stale bot reviews on subsequent pushes - Add SKILL.md line-count validation (warn 300+, error 500+) - Document regex-explanation rule in README and contributor guide - Fix pre-existing intent-comment issues in release-prepare Resolves #84
odinr
force-pushed
the
fix/issue-84-regex-explanation-guidance
branch
from
0c54596 to
4a2da66
Compare
There was a problem hiding this comment.
Pull request overview
Improves contributor feedback loops for script validation failures by making diagnostics actionable, adding CI-friendly structured output, and introducing a guardrail against oversized SKILL.md files.
Changes:
- Add fix-hinted diagnostics and separate headings for regex-explanation vs intent-comment issues in
validate:scripts, plus optional JSON diagnostics output. - Post inline PR review comments from
validate-scriptsworkflow using JSON diagnostics; dismiss prior bot reviews on subsequent runs. - Introduce
validate:skill-sizes(300 warn / 500 error) and wire it into the skills catalog validation workflow.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/validate-skills/check-skill-sizes.ts | Adds SKILL.md line-count scanning and severity sorting. |
| scripts/validate-skills/check-skill-sizes-cli.ts | Adds CLI wrapper and CI-friendly output/exit behavior for size findings. |
| scripts/validate-scripts/index.ts | Adds --json-output, splits issue headings, and emits JSON diagnostics. |
| scripts/validate-scripts/format-intent-comment-issues.ts | Adds Fix: hints via FIX_HINTS and appends them to formatted diagnostics. |
| scripts/validate-scripts/README.md | Documents the regex-explanation requirement and updates the Mermaid flow. |
| scripts/release-prepare/index.ts | Filters fusion-skills from root changelog package entries. |
| scripts/tests/validate-skills-sizes.test.ts | Adds tests for SKILL.md size findings and ordering. |
| scripts/tests/validate-scripts-format-issues.test.ts | Adds tests asserting Fix: hints are included in formatted issue output. |
| package.json | Adds validate:skill-sizes script entry. |
| contribute/06-scripts-code-rules.md | Documents the regex-explanation comment rule enforced by validate:scripts. |
| .github/workflows/validate-skills-catalog.yml | Runs the new SKILL.md size check in CI and reports it in the summary. |
| .github/workflows/validate-scripts.yml | Enables PR review comments via --json-output + actions/github-script and adds required permissions. |
You can also share your feedback on Copilot code review. Take the survey.
| console.error( | ||
| `${prefix}: ${finding.skillPath} has ${finding.lineCount} lines (${finding.level === "error" ? "max 500" : "recommended max 300"}).`, |
There was a problem hiding this comment.
Resolved: exported WARN_THRESHOLD and ERROR_THRESHOLD from check-skill-sizes.ts and updated the CLI to use these constants in error messages. This ensures the output won't drift if thresholds are updated in the future. Changes pushed in commit 91e8341.
| // Build inline review comments from diagnostics. | ||
| const comments = diagnostics.map(d => ({ | ||
| path: d.path, | ||
| line: d.line, | ||
| body: `${marker}\n**\`${d.code}\`**: ${d.message}`, | ||
| })); | ||
|
|
||
| // Post a new review with inline comments. | ||
| await github.rest.pulls.createReview({ | ||
| owner, repo, pull_number, commit_id, | ||
| event: 'COMMENT', | ||
| body: `${marker}\n**validate-scripts** found ${diagnostics.length} issue(s). Fix them to pass CI.`, |
scripts/validate-scripts/index.ts
Outdated
| // Write all diagnostics to JSON so CI can post inline PR review comments. | ||
| const allIntentIssues = [...intentCommentIssues, ...regexExplanationIssues, ...disallowedIssues]; | ||
| writeDiagnosticsJson(options.jsonOutput, allIntentIssues, repoRoot); |
| const findings = checkSkillSizes(tempRoot); | ||
| expect(findings).toHaveLength(1); | ||
| expect(findings[0]?.level).toBe("warning"); | ||
| expect(findings[0]?.lineCount).toBe(351); |
| await github.rest.pulls.createReview({ | ||
| owner, repo, pull_number, commit_id, | ||
| event: 'COMMENT', | ||
| body: `${marker}\n**validate-scripts** found ${diagnostics.length} issue(s). Fix them to pass CI.`, | ||
| comments, | ||
| }); |
| packages: entry.skillNames | ||
| // Exclude the meta-package so only real skill names appear in release notes. | ||
| .filter((skillName) => skillName !== "fusion-skills") | ||
| // Format each skill name as skill@version for the root changelog. | ||
| .map((skillName) => { |
| const content = readFileSync(filePath, "utf8"); | ||
| const lineCount = content.split("\n").length; | ||
| const skillPath = relative(repoRoot, filePath); |
…thoring workflow - Add runtime size comparison table to contribute/02-skill-structure-and-content.md - Add SKILL.md size limits section to copilot-instructions.md - Add size awareness to fusion-skill-authoring Step 4 and validation failure signals - Warning at 300 lines, error at 500 lines
- Add --only-diff and --base-ref flags to validate:skill-sizes - CI only checks SKILL.md files changed in the PR, not the entire catalog - Full scan still available locally without flags
Export WARN_THRESHOLD and ERROR_THRESHOLD constants from check-skill-sizes.ts so the CLI uses these values rather than hard-coding them. This ensures the output stays in sync if thresholds are updated in the future. - Export thresholds from check-skill-sizes.ts - Update CLI to import and use thresholds in error messages - Tests verified and code formatting checked
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Filter JSON diagnostics to changed hunks in --only-diff mode to avoid invalid inline review positions.\n\nHandle both b/path and --no-prefix diff header formats in diff-line parsing.\n\nAdd regression tests for diff-line map extraction and fallback behavior.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Split on "diff --git" markers to isolate per-file blocks. | ||
| const fileBlocks = diffOutput.split(/^diff --git/m).slice(1); | ||
|
|
||
| for (const block of fileBlocks) { |
There was a problem hiding this comment.
missing-intent-comment: missing-intent-comment: for (const block of fileBlocks) {
Fix: Add a // comment on the line above explaining why this control-flow or iterator exists.
| const lines = block.split("\n"); | ||
| const firstLine = (lines[0] || "").trim(); | ||
| const headerParts = firstLine.split(/\s+/); | ||
| if (headerParts.length < 2) continue; |
There was a problem hiding this comment.
missing-intent-comment: missing-intent-comment: if (headerParts.length < 2) continue;
Fix: Add a // comment on the line above explaining why this control-flow or iterator exists.
| if (headerParts.length < 2) continue; | ||
|
|
||
| const rawNewPath = headerParts[1]; | ||
| if (!rawNewPath || rawNewPath === "/dev/null") continue; |
There was a problem hiding this comment.
missing-intent-comment: missing-intent-comment: if (!rawNewPath || rawNewPath === "/dev/null") continue;
Fix: Add a // comment on the line above explaining why this control-flow or iterator exists.
| for (const line of lines) { | ||
| // Hunk header format: @@ -oldStart,oldCount +newStart,newCount @@ | ||
| const hunkMatch = line.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/); | ||
| if (!hunkMatch) continue; |
There was a problem hiding this comment.
missing-intent-comment: missing-intent-comment: if (!hunkMatch) continue;
Fix: Add a // comment on the line above explaining why this control-flow or iterator exists.
| const lineMap: DiffLineMap = {}; | ||
|
|
||
| // Split on "diff --git" markers to isolate per-file blocks. | ||
| const fileBlocks = diffOutput.split(/^diff --git/m).slice(1); |
There was a problem hiding this comment.
missing-regex-explanation: missing-regex-explanation: const fileBlocks = diffOutput.split(/^diff --git/m).slice(1);
Fix: Add a // comment containing the word "regex" on the line above to explain what the pattern matches.
| for (const block of fileBlocks) { | ||
| const lines = block.split("\n"); | ||
| const firstLine = (lines[0] || "").trim(); | ||
| const headerParts = firstLine.split(/\s+/); |
There was a problem hiding this comment.
missing-regex-explanation: missing-regex-explanation: const headerParts = firstLine.split(/\s+/);
Fix: Add a // comment containing the word "regex" on the line above to explain what the pattern matches.
| // Find hunk headers (@@ -old,count +new,count @@) to extract line ranges in the new file. | ||
| for (const line of lines) { | ||
| // Hunk header format: @@ -oldStart,oldCount +newStart,newCount @@ | ||
| const hunkMatch = line.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/); |
There was a problem hiding this comment.
missing-regex-explanation: missing-regex-explanation: const hunkMatch = line.match(/^@@ -\d+(?:,\d+)? +(\d+)(?:,(\d+))? @@/);
Fix: Add a // comment containing the word "regex" on the line above to explain what the pattern matches.
| @@ -0,0 +1,99 @@ | |||
| import { execFileSync } from "node:child_process"; | |||
There was a problem hiding this comment.
disallowed-multiple-exported-functions: disallowed-multiple-exported-functions: exported-functions:2
Fix: Split this file so each module exports at most one function.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
scripts/validate-scripts/index.ts:232
- The success log message still says "TSDoc, intent-comment, and disallowed-pattern checks passed" but this command now also enforces the regex-explanation rule. Consider updating the message so it reflects the full set of checks, otherwise contributors may think regex-explanation isn’t being validated.
: collectIntentCommentIssues(scriptsRoot);
// Keep disallowed-pattern violations separate from general intent-comment findings.
const disallowedIssueCodes = new Set([
"disallowed-while-loop",
You can also share your feedback on Copilot code review. Take the survey.
| // Generate a SKILL.md with 350 lines (above warning, below error). | ||
| writeFileSync(join(skillDir, "SKILL.md"), "line\n".repeat(350), "utf8"); | ||
|
|
||
| try { | ||
| const findings = checkSkillSizes(tempRoot); | ||
| expect(findings).toHaveLength(1); | ||
| expect(findings[0]?.level).toBe("warning"); | ||
| expect(findings[0]?.lineCount).toBe(351); | ||
| } finally { |
|
|
||
| /** | ||
| * Filters issues to only those reported on changed diff lines. | ||
| * | ||
| * @param issues - Candidate issues to include in JSON diagnostics. | ||
| * @param repoRoot - Absolute repository root for relative path display. | ||
| * @param diffLineMap - Map of file paths to changed line ranges. | ||
| * @returns Issues that map to changed lines in the current diff. | ||
| */ | ||
| function filterIssuesToDiffLines( | ||
| issues: IntentCommentIssue[], | ||
| repoRoot: string, | ||
| diffLineMap: Record<string, Array<{ start: number; end: number }>>, | ||
| ): IntentCommentIssue[] { | ||
| // Iterate each issue so PR review comments only target changed diff hunks. | ||
| return issues.filter((issue) => { | ||
| // This regex normalizes Windows path separators to POSIX for diff-map key lookup. | ||
| const relPath = relative(repoRoot, issue.filePath).replace(/\\/g, "/"); | ||
| const ranges = diffLineMap[relPath]; | ||
| return ranges ? isLineInDiff(issue.line, ranges) : false; | ||
| }); | ||
| } | ||
|
|
||
| /** |
| // Post a new review with inline comments. | ||
| await github.rest.pulls.createReview({ | ||
| owner, repo, pull_number, commit_id, | ||
| event: 'COMMENT', | ||
| body: `${marker}\n**validate-scripts** found ${diagnostics.length} issue(s). Fix them to pass CI.`, | ||
| comments, | ||
| }); | ||
|
|
|
|
||
| // Keep CLI execution scoped to direct invocation to avoid side effects in imports. | ||
| if (import.meta.main) { | ||
| try { |
| - name: Validate SKILL.md file sizes | ||
| id: validate_sizes | ||
| run: bun run validate:skill-sizes --only-diff --base-ref origin/${{ github.base_ref || 'main' }} |
| const content = readFileSync(filePath, "utf8"); | ||
| const lineCount = content.split("\n").length; | ||
| const skillPath = relative(repoRoot, filePath); |
| // Split on "diff --git" markers to isolate per-file blocks. | ||
| const fileBlocks = diffOutput.split(/^diff --git/m).slice(1); | ||
|
|
||
| for (const block of fileBlocks) { |
There was a problem hiding this comment.
missing-intent-comment: missing-intent-comment: for (const block of fileBlocks) {
Fix: Add a // comment on the line above explaining why this control-flow or iterator exists.
| const lines = block.split("\n"); | ||
| const firstLine = (lines[0] || "").trim(); | ||
| const headerParts = firstLine.split(/\s+/); | ||
| if (headerParts.length < 2) continue; |
There was a problem hiding this comment.
missing-intent-comment: missing-intent-comment: if (headerParts.length < 2) continue;
Fix: Add a // comment on the line above explaining why this control-flow or iterator exists.
| if (headerParts.length < 2) continue; | ||
|
|
||
| const rawNewPath = headerParts[1]; | ||
| if (!rawNewPath || rawNewPath === "/dev/null") continue; |
There was a problem hiding this comment.
missing-intent-comment: missing-intent-comment: if (!rawNewPath || rawNewPath === "/dev/null") continue;
Fix: Add a // comment on the line above explaining why this control-flow or iterator exists.
| for (const line of lines) { | ||
| // Hunk header format: @@ -oldStart,oldCount +newStart,newCount @@ | ||
| const hunkMatch = line.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/); | ||
| if (!hunkMatch) continue; |
There was a problem hiding this comment.
missing-intent-comment: missing-intent-comment: if (!hunkMatch) continue;
Fix: Add a // comment on the line above explaining why this control-flow or iterator exists.
| const lineMap: DiffLineMap = {}; | ||
|
|
||
| // Split on "diff --git" markers to isolate per-file blocks. | ||
| const fileBlocks = diffOutput.split(/^diff --git/m).slice(1); |
There was a problem hiding this comment.
missing-regex-explanation: missing-regex-explanation: const fileBlocks = diffOutput.split(/^diff --git/m).slice(1);
Fix: Add a // comment containing the word "regex" on the line above to explain what the pattern matches.
| for (const block of fileBlocks) { | ||
| const lines = block.split("\n"); | ||
| const firstLine = (lines[0] || "").trim(); | ||
| const headerParts = firstLine.split(/\s+/); |
There was a problem hiding this comment.
missing-regex-explanation: missing-regex-explanation: const headerParts = firstLine.split(/\s+/);
Fix: Add a // comment containing the word "regex" on the line above to explain what the pattern matches.
| // Find hunk headers (@@ -old,count +new,count @@) to extract line ranges in the new file. | ||
| for (const line of lines) { | ||
| // Hunk header format: @@ -oldStart,oldCount +newStart,newCount @@ | ||
| const hunkMatch = line.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/); |
There was a problem hiding this comment.
missing-regex-explanation: missing-regex-explanation: const hunkMatch = line.match(/^@@ -\d+(?:,\d+)? +(\d+)(?:,(\d+))? @@/);
Fix: Add a // comment containing the word "regex" on the line above to explain what the pattern matches.
| @@ -0,0 +1,99 @@ | |||
| import { execFileSync } from "node:child_process"; | |||
There was a problem hiding this comment.
disallowed-multiple-exported-functions: disallowed-multiple-exported-functions: exported-functions:2
Fix: Split this file so each module exports at most one function.
Limit JSON diagnostics in --only-diff mode to changed lines so PR inline review comments don't target non-diff lines. Improve diff parsing for --no-prefix headers and keep validate-scripts rule compliance in parser/test updates.
…sting - Fix off-by-one line count caused by trailing newlines via getNormalizedLineCount() - Support both Unix (LF) and Windows (CRLF) line endings in SKILL.md size validation - Add boundary-value tests for 300-line warn and 500-line error thresholds - Chunk PR review comments into 50-per-review batches to avoid GitHub API limits - Add error handling and fallback for createReview() failures - Clarify inline comments scope (intent/regex/disallowed diagnostics only) - Fix validate-skills-catalog.yml checkout fetch-depth for diff-only size checks - All validation checks pass: validate:scripts, biome:check, test suite
Update the success log message to explicitly list regex-explanation as one of the checks performed, since it's now a distinct validation category (separate from general intent-comment checks).
Document the new SKILL.md size limits (300 warn, 500 error) in the skill-authoring guide, including expectations for moving overflow to references/.
- Added "@biomejs/biome" version "^2.4.5" to devDependencies. - Removed duplicate entry for "@biomejs/biome". - Added "biome" version "^0.3.3" to devDependencies.
Why
The
validate:scriptsCI check rejects regex literals without a specific comment format, but the error output gives no actionable fix guidance. The error heading groups unrelated issue types together, no documentation surface mentions the rule, and contributors have no self-service path to resolve failures.Additionally, SKILL.md files have no size guardrails — skills can grow indefinitely without feedback.
Current behavior
missing-regex-explanationdiagnostics emitfile:line:code:statementwith no fix hint.New behavior
Fix:hint explaining exactly what to do.missing-regex-explanationissues print under their own "Regex-explanation check failed" heading, separate from intent-comment issues.--json-output <path>flag writes structured diagnostics for downstream tooling.validate:skill-sizescommand warns at 300+ lines and errors at 500+, wired into the skills catalog workflow.References
Reviewer focus
bun run validate:scriptslocally to see new diagnostic format; check workflow YAML for correct github-script logic; review SKILL.md size thresholds (300 warn / 500 error)pull-requests: writepermission for review comments — review the github-script scope carefullySelf review checklist