fix: address review feedback, add CI annotations and step summary
Address alexeyv's review findings on PR #1494: - Fix exec-attr prefix handling for {_bmad}/ and bare _bmad/ paths - Fix mapInstalledToSource fallback (remove phantom src/modules/ mapping) - Switch extractYamlRefs to parseDocument() for YAML line numbers Add CI integration (stories 2-1, 2-2): - Emit ::warning annotations for broken refs and abs-path leaks - Write markdown table to $GITHUB_STEP_SUMMARY - Guard both behind environment variable checks Harden CI output: - escapeAnnotation() encodes %, \r, \n per GitHub Actions spec - escapeTableCell() escapes pipe chars in step summary table
This commit is contained in:
parent
b7cfbefb6f
commit
96595244e7
|
|
@ -68,6 +68,16 @@ const LOAD_DIRECTIVE = /Load[:\s]+`(\.[^`]+)`/g;
|
||||||
// Pattern: absolute path leaks
|
// Pattern: absolute path leaks
|
||||||
const ABS_PATH_LEAK = /(?:\/Users\/|\/home\/|[A-Z]:\\\\)/;
|
const ABS_PATH_LEAK = /(?:\/Users\/|\/home\/|[A-Z]:\\\\)/;
|
||||||
|
|
||||||
|
// --- Output Escaping ---
|
||||||
|
|
||||||
|
function escapeAnnotation(str) {
|
||||||
|
return str.replaceAll('%', '%25').replaceAll('\r', '%0D').replaceAll('\n', '%0A');
|
||||||
|
}
|
||||||
|
|
||||||
|
function escapeTableCell(str) {
|
||||||
|
return String(str).replaceAll('|', String.raw`\|`);
|
||||||
|
}
|
||||||
|
|
||||||
// Path prefixes/patterns that only exist in installed structure, not in source
|
// Path prefixes/patterns that only exist in installed structure, not in source
|
||||||
const INSTALL_ONLY_PATHS = ['_config/'];
|
const INSTALL_ONLY_PATHS = ['_config/'];
|
||||||
|
|
||||||
|
|
@ -150,8 +160,8 @@ function mapInstalledToSource(refPath) {
|
||||||
return path.join(SRC_DIR, cleaned);
|
return path.join(SRC_DIR, cleaned);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Other modules are under src/modules/
|
// Fallback: map directly under src/
|
||||||
return path.join(SRC_DIR, 'modules', cleaned);
|
return path.join(SRC_DIR, cleaned);
|
||||||
}
|
}
|
||||||
|
|
||||||
// --- Reference Extraction ---
|
// --- Reference Extraction ---
|
||||||
|
|
@ -181,44 +191,57 @@ function isInstallOnly(cleanedPath) {
|
||||||
function extractYamlRefs(filePath, content) {
|
function extractYamlRefs(filePath, content) {
|
||||||
const refs = [];
|
const refs = [];
|
||||||
|
|
||||||
let parsed;
|
let doc;
|
||||||
try {
|
try {
|
||||||
parsed = yaml.parse(content);
|
doc = yaml.parseDocument(content);
|
||||||
} catch {
|
} catch {
|
||||||
return refs; // Skip unparseable YAML (schema validator handles this)
|
return refs; // Skip unparseable YAML (schema validator handles this)
|
||||||
}
|
}
|
||||||
|
|
||||||
function walkValues(obj, keyPath) {
|
function checkValue(value, range, keyPath) {
|
||||||
if (typeof obj === 'string') {
|
if (typeof value !== 'string') return;
|
||||||
if (!isResolvable(obj)) return;
|
if (!isResolvable(value)) return;
|
||||||
|
|
||||||
// Check for {project-root}/_bmad/ refs
|
const line = range ? offsetToLine(content, range[0]) : undefined;
|
||||||
const prMatch = obj.match(/\{project-root\}\/_bmad\/[^\s'"<>})\]`]+/);
|
|
||||||
if (prMatch) {
|
|
||||||
refs.push({ file: filePath, raw: prMatch[0], type: 'project-root', key: keyPath });
|
|
||||||
}
|
|
||||||
|
|
||||||
// Check for {_bmad}/ refs
|
// Check for {project-root}/_bmad/ refs
|
||||||
const bmMatch = obj.match(/\{_bmad\}\/[^\s'"<>})\]`]+/);
|
const prMatch = value.match(/\{project-root\}\/_bmad\/[^\s'"<>})\]`]+/);
|
||||||
if (bmMatch) {
|
if (prMatch) {
|
||||||
refs.push({ file: filePath, raw: bmMatch[0], type: 'project-root', key: keyPath });
|
refs.push({ file: filePath, raw: prMatch[0], type: 'project-root', line, key: keyPath });
|
||||||
}
|
}
|
||||||
|
|
||||||
// Check for relative paths
|
// Check for {_bmad}/ refs
|
||||||
const relMatch = obj.match(/^\.\.?\/[^\s'"<>})\]`]+\.(?:md|yaml|yml|xml|json|csv|txt)$/);
|
const bmMatch = value.match(/\{_bmad\}\/[^\s'"<>})\]`]+/);
|
||||||
if (relMatch) {
|
if (bmMatch) {
|
||||||
refs.push({ file: filePath, raw: relMatch[0], type: 'relative', key: keyPath });
|
refs.push({ file: filePath, raw: bmMatch[0], type: 'project-root', line, key: keyPath });
|
||||||
}
|
}
|
||||||
} else if (Array.isArray(obj)) {
|
|
||||||
for (const [i, item] of obj.entries()) walkValues(item, `${keyPath}[${i}]`);
|
// Check for relative paths
|
||||||
} else if (obj && typeof obj === 'object') {
|
const relMatch = value.match(/^\.\.?\/[^\s'"<>})\]`]+\.(?:md|yaml|yml|xml|json|csv|txt)$/);
|
||||||
for (const [key, val] of Object.entries(obj)) {
|
if (relMatch) {
|
||||||
walkValues(val, keyPath ? `${keyPath}.${key}` : key);
|
refs.push({ file: filePath, raw: relMatch[0], type: 'relative', line, key: keyPath });
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
walkValues(parsed, '');
|
function walkNode(node, keyPath) {
|
||||||
|
if (!node) return;
|
||||||
|
|
||||||
|
if (yaml.isMap(node)) {
|
||||||
|
for (const item of node.items) {
|
||||||
|
const key = item.key && item.key.value !== undefined ? item.key.value : '?';
|
||||||
|
const childPath = keyPath ? `${keyPath}.${key}` : String(key);
|
||||||
|
walkNode(item.value, childPath);
|
||||||
|
}
|
||||||
|
} else if (yaml.isSeq(node)) {
|
||||||
|
for (const [i, item] of node.items.entries()) {
|
||||||
|
walkNode(item, `${keyPath}[${i}]`);
|
||||||
|
}
|
||||||
|
} else if (yaml.isScalar(node)) {
|
||||||
|
checkValue(node.value, node.range, keyPath);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
walkNode(doc.contents, '');
|
||||||
return refs;
|
return refs;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -285,6 +308,12 @@ function resolveRef(ref) {
|
||||||
if (execPath.includes('{project-root}')) {
|
if (execPath.includes('{project-root}')) {
|
||||||
return mapInstalledToSource(execPath);
|
return mapInstalledToSource(execPath);
|
||||||
}
|
}
|
||||||
|
if (execPath.includes('{_bmad}')) {
|
||||||
|
return mapInstalledToSource(execPath);
|
||||||
|
}
|
||||||
|
if (execPath.startsWith('_bmad/')) {
|
||||||
|
return mapInstalledToSource(execPath);
|
||||||
|
}
|
||||||
// Relative exec path
|
// Relative exec path
|
||||||
return path.resolve(path.dirname(ref.file), execPath);
|
return path.resolve(path.dirname(ref.file), execPath);
|
||||||
}
|
}
|
||||||
|
|
@ -334,6 +363,7 @@ let totalRefs = 0;
|
||||||
let brokenRefs = 0;
|
let brokenRefs = 0;
|
||||||
let totalLeaks = 0;
|
let totalLeaks = 0;
|
||||||
let filesWithIssues = 0;
|
let filesWithIssues = 0;
|
||||||
|
const allIssues = []; // Collect for $GITHUB_STEP_SUMMARY
|
||||||
|
|
||||||
for (const filePath of files) {
|
for (const filePath of files) {
|
||||||
const relativePath = path.relative(PROJECT_ROOT, filePath);
|
const relativePath = path.relative(PROJECT_ROOT, filePath);
|
||||||
|
|
@ -391,10 +421,19 @@ for (const filePath of files) {
|
||||||
const location = ref.line ? `line ${ref.line}` : ref.key ? `key: ${ref.key}` : '';
|
const location = ref.line ? `line ${ref.line}` : ref.key ? `key: ${ref.key}` : '';
|
||||||
console.log(` [BROKEN] ${ref.raw}${location ? ` (${location})` : ''}`);
|
console.log(` [BROKEN] ${ref.raw}${location ? ` (${location})` : ''}`);
|
||||||
console.log(` Target not found: ${resolved}`);
|
console.log(` Target not found: ${resolved}`);
|
||||||
|
allIssues.push({ file: relativePath, line: ref.line || 1, ref: ref.raw, issue: 'broken ref' });
|
||||||
|
if (process.env.GITHUB_ACTIONS) {
|
||||||
|
const line = ref.line || 1;
|
||||||
|
console.log(`::warning file=${relativePath},line=${line}::${escapeAnnotation(`Broken reference: ${ref.raw} → ${resolved}`)}`);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
for (const leak of leaks) {
|
for (const leak of leaks) {
|
||||||
console.log(` [ABS-PATH] Line ${leak.line}: ${leak.content}`);
|
console.log(` [ABS-PATH] Line ${leak.line}: ${leak.content}`);
|
||||||
|
allIssues.push({ file: relativePath, line: leak.line, ref: leak.content, issue: 'abs-path' });
|
||||||
|
if (process.env.GITHUB_ACTIONS) {
|
||||||
|
console.log(`::warning file=${relativePath},line=${leak.line}::${escapeAnnotation(`Absolute path leak: ${leak.content}`)}`);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -423,4 +462,19 @@ if (hasIssues) {
|
||||||
|
|
||||||
console.log('');
|
console.log('');
|
||||||
|
|
||||||
|
// Write GitHub Actions step summary
|
||||||
|
if (process.env.GITHUB_STEP_SUMMARY) {
|
||||||
|
let summary = '## File Reference Validation\n\n';
|
||||||
|
if (allIssues.length > 0) {
|
||||||
|
summary += '| File | Line | Reference | Issue |\n';
|
||||||
|
summary += '|------|------|-----------|-------|\n';
|
||||||
|
for (const issue of allIssues) {
|
||||||
|
summary += `| ${escapeTableCell(issue.file)} | ${issue.line} | ${escapeTableCell(issue.ref)} | ${issue.issue} |\n`;
|
||||||
|
}
|
||||||
|
summary += '\n';
|
||||||
|
}
|
||||||
|
summary += `**${files.length} files scanned, ${totalRefs} references checked, ${brokenRefs + totalLeaks} issues found**\n`;
|
||||||
|
fs.appendFileSync(process.env.GITHUB_STEP_SUMMARY, summary);
|
||||||
|
}
|
||||||
|
|
||||||
process.exit(hasIssues && STRICT ? 1 : 0);
|
process.exit(hasIssues && STRICT ? 1 : 0);
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue