Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .github/workflows/smoke-claude-on-copilot.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 3 additions & 6 deletions actions/setup/js/validate_memory_files.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@ const path = require("path");
* @returns {ValidationResult} Validation result with list of invalid files
*/
function validateMemoryFiles(memoryDir, memoryType = "cache", allowedExtensions) {
const allowAll = !allowedExtensions?.length;

if (allowAll) {
if (!allowedExtensions?.length) {
core.info(`All file extensions are allowed in ${memoryType}-memory directory`);
return { valid: true, invalidFiles: [] };
}
Expand All @@ -33,6 +31,7 @@ function validateMemoryFiles(memoryDir, memoryType = "cache", allowedExtensions)
}

const extensions = new Set(allowedExtensions.map(ext => ext.trim().toLowerCase()));
/** @type {string[]} */
const invalidFiles = [];

/**
Expand All @@ -50,9 +49,7 @@ function validateMemoryFiles(memoryDir, memoryType = "cache", allowedExtensions)
if (entry.isDirectory()) {
// Skip .git directory — it is git metadata used for integrity branching
// and contains files with no extension (e.g. HEAD, ORIG_HEAD, packed-refs).
if (entry.name === ".git") {
continue;
}
if (entry.name === ".git") continue;
scanDirectory(fullPath, relativeFilePath);
} else if (entry.isFile()) {
const ext = path.extname(entry.name).toLowerCase();
Expand Down
62 changes: 52 additions & 10 deletions actions/setup/js/validate_memory_files.test.cjs
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
// @ts-check

import { describe, it, expect, beforeEach, afterEach } from "vitest";
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
import fs from "fs";
import path from "path";
import os from "os";

const { validateMemoryFiles } = require("./validate_memory_files.cjs");

// Mock core globally
// Mock core globally with vi.fn() so we can assert on calls
global.core = {
info: () => {},
error: () => {},
warning: () => {},
debug: () => {},
info: vi.fn(),
error: vi.fn(),
warning: vi.fn(),
debug: vi.fn(),
};

describe("validateMemoryFiles", () => {
Expand All @@ -21,13 +21,15 @@ describe("validateMemoryFiles", () => {
beforeEach(() => {
// Create a temporary directory for testing
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "validate-memory-test-"));
vi.resetAllMocks();
});

afterEach(() => {
// Clean up temporary directory
if (tempDir && fs.existsSync(tempDir)) {
fs.rmSync(tempDir, { recursive: true, force: true });
}
vi.restoreAllMocks();
});

it("returns valid for empty directory", () => {
Expand Down Expand Up @@ -243,10 +245,50 @@ describe("validateMemoryFiles", () => {
expect(result.invalidFiles).toEqual([]);
});

it("uses repo as memoryType in error messages", () => {
fs.writeFileSync(path.join(tempDir, "invalid.log"), "log");
const result = validateMemoryFiles(tempDir, "repo", [".txt"]);
it("uses 'cache' as the default memoryType", () => {
const result = validateMemoryFiles(tempDir);
expect(result.valid).toBe(true);
expect(global.core.info).toHaveBeenCalledWith(expect.stringContaining("cache-memory"));
});

it("calls core.error with details when files fail custom extension validation", () => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The return value of validateMemoryFiles is not captured here, so the test only verifies the side effect (error logging) but not the function's contract (what it returns on failure). Adding return value assertions documents the full spec.

💡 Suggested addition
it("calls core.error with details when files fail custom extension validation", () => {
  fs.writeFileSync(path.join(tempDir, "bad.log"), "log");
  const result = validateMemoryFiles(tempDir, "repo", [".json"]);
  expect(result.valid).toBe(false);
  expect(result.invalidFiles).toEqual(["bad.log"]);
  expect(global.core.error).toHaveBeenCalledWith(expect.stringContaining("Found 1 file(s) with invalid extensions in repo-memory:"));
  expect(global.core.error).toHaveBeenCalledWith(expect.stringContaining("bad.log (extension: .log)"));
  expect(global.core.error).toHaveBeenCalledWith(expect.stringContaining("Allowed extensions: .json"));
});

The return-value check also provides a regression guard: if the implementation ever accidentally returns valid: true while still logging errors, the side-effect assertions alone would pass silently.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added result.valid and result.invalidFiles assertions to both the calls core.error with details and reports files with no extension tests in abced73.

fs.writeFileSync(path.join(tempDir, "bad.log"), "log");
const result = validateMemoryFiles(tempDir, "repo", [".json"]);
expect(result.valid).toBe(false);
expect(result.invalidFiles).toEqual(["invalid.log"]);
expect(result.invalidFiles).toContain("bad.log");
expect(global.core.error).toHaveBeenCalledWith(expect.stringContaining("Found 1 file(s) with invalid extensions in repo-memory:"));
expect(global.core.error).toHaveBeenCalledWith(expect.stringContaining("bad.log (extension: .log)"));
expect(global.core.error).toHaveBeenCalledWith(expect.stringContaining("Allowed extensions: .json"));
});

it("reports files with no extension as '(no extension)' in error output", () => {
fs.writeFileSync(path.join(tempDir, "noext"), "content");
const result = validateMemoryFiles(tempDir, "cache", [".json"]);
expect(result.valid).toBe(false);
expect(result.invalidFiles).toContain("noext");
expect(global.core.error).toHaveBeenCalledWith(expect.stringContaining("noext (extension: (no extension))"));
});

it("detects invalid files in deeply nested directories with custom extensions", () => {
const level1 = path.join(tempDir, "level1");
const level2 = path.join(level1, "level2");
fs.mkdirSync(level1);
fs.mkdirSync(level2);
fs.writeFileSync(path.join(level2, "valid.json"), "{}");
fs.writeFileSync(path.join(level2, "invalid.bin"), "binary");
const result = validateMemoryFiles(tempDir, "cache", [".json"]);
expect(result.valid).toBe(false);
expect(result.invalidFiles).toContain(path.join("level1", "level2", "invalid.bin"));
expect(result.invalidFiles).not.toContain(path.join("level1", "level2", "valid.json"));
});

it("returns valid=false and empty invalidFiles when directory scan throws", () => {
vi.spyOn(fs, "readdirSync").mockImplementationOnce(() => {
throw new Error("Permission denied");
});
const result = validateMemoryFiles(tempDir, "cache", [".json"]);
expect(result.valid).toBe(false);
expect(result.invalidFiles).toEqual([]);
expect(global.core.error).toHaveBeenCalledWith(expect.stringContaining("Failed to scan cache-memory directory: Permission denied"));
});
});
Loading