-
Notifications
You must be signed in to change notification settings - Fork 424
feat(repo-memory): add format-json option to pretty-print JSON files before commit #39540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,7 @@ async function main() { | |
| const maxFileCount = parseInt(process.env.MAX_FILE_COUNT || "100", 10); | ||
| const maxPatchSize = parseInt(process.env.MAX_PATCH_SIZE || "10240", 10); | ||
| const fileGlobFilter = process.env.FILE_GLOB_FILTER || ""; | ||
| const formatJSON = process.env.FORMAT_JSON === "true"; | ||
|
|
||
| // Parse allowed extensions with error handling | ||
| let allowedExtensions = [".json", ".jsonl", ".txt", ".md", ".csv"]; | ||
|
|
@@ -74,6 +75,7 @@ async function main() { | |
| core.info(` ALLOWED_EXTENSIONS: ${JSON.stringify(allowedExtensions)}`); | ||
| core.info(` FILE_GLOB_FILTER: ${fileGlobFilter ? `"${fileGlobFilter}"` : "(empty - all files accepted)"}`); | ||
| core.info(` FILE_GLOB_FILTER length: ${fileGlobFilter.length}`); | ||
| core.info(` FORMAT_JSON: ${formatJSON}`); | ||
|
|
||
| /** @param {unknown} value */ | ||
| function isPlainObject(value) { | ||
|
|
@@ -359,6 +361,58 @@ async function main() { | |
| } | ||
| } | ||
|
|
||
| // Format JSON files if requested | ||
| if (formatJSON) { | ||
| core.info("FORMAT_JSON is enabled: formatting .json files as human-readable..."); | ||
|
|
||
| /** | ||
| * Recursively find and format all .json files under a directory | ||
| * @param {string} dirPath - Directory to scan | ||
| */ | ||
| function formatJSONFilesInDir(dirPath) { | ||
| const entries = fs.readdirSync(dirPath, { withFileTypes: true }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💡 Suggested fixWrap the recursive call and the function formatJSONFilesInDir(dirPath) {
let entries;
try {
entries = fs.readdirSync(dirPath, { withFileTypes: true });
} catch (err) {
core.warning(`Skipping directory (cannot read): ${path.relative(destMemoryPath, dirPath)}: ${err.message}`);
return;
}
for (const entry of entries) {
// ...
if (entry.isDirectory() && entry.name !== ".git") {
formatJSONFilesInDir(fullPath); // inner failures now self-contained
}
}
} |
||
| for (const entry of entries) { | ||
| const fullPath = path.join(dirPath, entry.name); | ||
| if (entry.isDirectory()) { | ||
| if (entry.name !== ".git") { | ||
| formatJSONFilesInDir(fullPath); | ||
| } | ||
| } else if (entry.isFile() && entry.name.endsWith(".json")) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The docs state it('should only format .json files, not .jsonl or other types', () => {
expect(scriptContent).toContain('.endsWith(".json")');
// Verify .jsonl is not in the same condition
expect(scriptContent).not.toMatch(/endsWith\(".jsonl"\)/);
});This also documents the intended exclusion for future maintainers. |
||
| try { | ||
| const raw = fs.readFileSync(fullPath, "utf8"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No file-size guard before 💡 Suggested fixAdd a stat check before reading, consistent with the copy-phase guard: const statResult = fs.statSync(fullPath);
if (statResult.size > maxFileSize) {
core.warning(`Skipping large JSON file (> ${maxFileSize} bytes): ${path.relative(destMemoryPath, fullPath)}`);
continue;
}
const raw = fs.readFileSync(fullPath, "utf8"); |
||
| if (!raw.trim()) { | ||
| continue; | ||
| } | ||
| const parsed = JSON.parse(raw); | ||
| const formatted = JSON.stringify(parsed, null, 2) + "\n"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/grill-with-docs] This is a V8 spec behaviour, not a bug in this code, but the docs (
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💡 Details and fixReproduction: const o = JSON.parse('{"b":1,"10":3,"2":4}');
console.log(JSON.stringify(o, null, 2));
// Output:
// {
// "2": 4,
// "10": 3,
// "b": 1
// }This is a one-time normalization (idempotent after first write), but the silent reordering may break downstream consumers that rely on key order. Consider logging a per-file warning when the stringified form differs from the original to give visibility into what was mutated, not just that it was mutated: core.info(`Formatted JSON (structural change): ${path.relative(destMemoryPath, fullPath)}`); |
||
| if (raw !== formatted) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The 💡 Suggested test (source-content check)it('should skip writeFileSync when JSON is already formatted', () => {
// Verify the guard exists in source — protects against accidental removal
expect(scriptContent).toContain('if (raw !== formatted)');
});For a more behavioural test, extract |
||
| const formattedSize = Buffer.byteLength(formatted, "utf8"); | ||
| if (formattedSize > maxFileSize) { | ||
| const sizeError = new Error(`Formatted JSON exceeds MAX_FILE_SIZE: ${path.relative(destMemoryPath, fullPath)} (${formattedSize} bytes > ${maxFileSize} bytes)`); | ||
| sizeError.name = "FormatJSONSizeLimitError"; | ||
| throw sizeError; | ||
| } | ||
| fs.writeFileSync(fullPath, formatted, "utf8"); | ||
| core.info(`Formatted JSON: ${path.relative(destMemoryPath, fullPath)}`); | ||
| } | ||
| } catch (/** @type {any} */ error) { | ||
| if (error?.name === "FormatJSONSizeLimitError") { | ||
| throw error; | ||
| } | ||
| core.warning(`Skipping JSON formatting for ${path.relative(destMemoryPath, fullPath)}: ${error.message}`); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| formatJSONFilesInDir(destMemoryPath); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First-enable will always exceed 💡 Suggested fixOption A (preferred) — only format files that were just copied: const copiedPaths = new Set(
filesToCopy.map(f => path.resolve(path.join(destMemoryPath, f.relativePath)))
);
// inside formatJSONFilesInDir, change the condition to:
} else if (entry.isFile() && entry.name.endsWith('.json') && copiedPaths.has(path.resolve(fullPath))) {Option B (documentation-only) — add a note in the docs that first enable on a populated branch reformats ALL pre-existing JSON files and may require increasing The same scope expansion inflates |
||
| } catch (error) { | ||
| core.setFailed(`Failed to format JSON files: ${getErrorMessage(error)}`); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Check if we have any changes to commit | ||
| let changedFileCount = 0; | ||
| try { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd]
formatJSONFilesInDiris defined inside theif (formatJSON)block insidemain()— making it untestable in isolation.push_repo_memory.test.cjsalready has 1566 lines of coverage for this file, but FORMAT_JSON handling has zero test coverage despite being the core runtime behaviour of this feature.💡 Two paths to fix
Option A — extract to a module (same pattern as
glob_pattern_helpers.cjs):This makes the function importable and testable with a mock
fs.Option B — source-content checks (already used at lines 1502–1565):
Lower fidelity but consistent with existing test style.
Behaviours worth covering:
\nwriteFileSynccall (no-op)core.warning, notcore.setFailed.jsonl/.txt→ untouched.git/→ not traversed