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
12 changes: 6 additions & 6 deletions .github/workflows/metrics-collector.lock.yml

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

1 change: 1 addition & 0 deletions .github/workflows/metrics-collector.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ tools:
repo-memory:
branch-name: memory/meta-orchestrators
file-glob: "metrics/**"
max-patch-size: 131072 # 128KB - handles large daily metrics snapshots without patch-size gate failures
timeout-minutes: 15
safe-outputs:
noop:
Expand Down
26 changes: 21 additions & 5 deletions actions/setup/js/push_repo_memory.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -498,10 +498,20 @@ async function main() {
}
}

// Check if we have any changes to commit
// Build literal pathspecs from the relative paths of files to copy.
// The :(literal) magic prefix tells Git to treat each entry as a plain string,
// preventing glob expansion or pathspec-magic interpretation (e.g. :(top),
// wildcards) even when a filename happens to contain those characters.
const literalPathspecs = Array.from(new Set(filesToCopy.map(file => `:(literal)${file.relativePath}`))).sort();

// Check if we have any changes to commit, scoped to managed memory files only.
let changedFileCount = 0;
try {
const status = execGitSync(["status", "--porcelain"]);
const statusArgs = ["status", "--porcelain"];
if (literalPathspecs.length > 0) {
statusArgs.push("--", ...literalPathspecs);
}
const status = execGitSync(statusArgs, { cwd: workspaceDir });
const changedEntries = status
.split("\n")
.map(line => line.trim())
Expand Down Expand Up @@ -534,7 +544,13 @@ async function main() {
// sparse-checkout, causing a plain "git add ." to silently skip or reject
// files on the first run for a new memory branch.
try {
execGitSync(["add", "--sparse", "."], { stdio: "inherit" });
const addArgs = ["add", "--sparse"];
if (literalPathspecs.length > 0) {
addArgs.push("--", ...literalPathspecs);
} else {

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.

[/diagnosing-bugs] Dead code: the else { addArgs.push(".") } branch (lines 546–548) is unreachable — filesToCopy is guaranteed non-empty at this point (line 400 returns early when it is empty), so changedPathspecs will always be non-empty here.

💡 Suggestion

Remove the dead branch and rely on the early-return guarantee:

// changedPathspecs is always non-empty here (filesToCopy emptiness is checked at line 400)
addArgs.push("--", ...changedPathspecs);

Leaving the fallback "." in place makes the sparse-checkout safety invariant harder to reason about and could mask a future regression if the early-return is ever removed.

@copilot please address this.

addArgs.push(".");
}
execGitSync(addArgs, { stdio: "inherit", cwd: workspaceDir });
} catch (error) {
core.setFailed(`Failed to stage changes: ${getErrorMessage(error)}`);
return;
Expand All @@ -546,7 +562,7 @@ async function main() {
// (e.g. a regenerated JSON object) from being counted as "entire source code size"
// even though only a small portion of the data actually changed.
try {
const patchSizeBytes = getStagedPatchDiffSizeBytes({ execGitSyncFn: execGitSync });
const patchSizeBytes = getStagedPatchDiffSizeBytes({ execGitSyncFn: execGitSync, cwd: workspaceDir });
const patchSizeKb = Math.ceil(patchSizeBytes / 1024);
const maxPatchSizeKb = Math.floor(maxPatchSize / 1024);
// Allow 20% overhead to account for git diff format (headers, context lines, etc.)
Expand All @@ -559,7 +575,7 @@ async function main() {
// Add per-file diff stats to diagnose what's causing the large patch
// (e.g. a full rewrite of an accumulated history file shows old + new content in the diff)
try {
const diffStat = execGitSync(["diff", "--cached", "--stat"], { stdio: "pipe" });
const diffStat = execGitSync(["diff", "--cached", "--stat"], { stdio: "pipe", cwd: workspaceDir });
core.warning(`Patch content breakdown (git diff --stat):\n${diffStat}`);
} catch (statError) {
core.warning(`Could not retrieve diff stat: ${getErrorMessage(statError)}`);
Expand Down
30 changes: 27 additions & 3 deletions actions/setup/js/push_repo_memory.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -1073,13 +1073,36 @@ describe("push_repo_memory.cjs - shell injection security tests", () => {
const scriptPath = path.join(import.meta.dirname, "push_repo_memory.cjs");
const scriptContent = fs.readFileSync(scriptPath, "utf8");

// Must use "git add --sparse ." to stage files regardless of sparse-checkout state.
expect(scriptContent).toContain('"add", "--sparse", "."');
// Must use git add --sparse with literal pathspec staging so only managed memory paths are included.
expect(scriptContent).toContain('"add", "--sparse"');
expect(scriptContent).toContain('addArgs.push("--", ...literalPathspecs)');

// Must NOT use plain "git add ." which breaks under sparse-checkout.
expect(scriptContent).not.toContain('"add", "."');
});

it("should encode pathspecs as :(literal) to prevent Git glob/magic interpretation (source check)", () => {
// Regression test for: filenames with glob metacharacters or pathspec-magic prefixes
// (e.g. ":(top)foo", "metrics/*.json") being interpreted as Git pathspecs rather
// than literal paths, which could cause git status/add to match files outside the
// managed memory scope.
//
// Fix: all artifact-derived paths are wrapped with the :(literal) magic prefix so
// Git treats them as plain strings regardless of their content.

const fs = require("fs");
const path = require("path");

const scriptPath = path.join(import.meta.dirname, "push_repo_memory.cjs");
const scriptContent = fs.readFileSync(scriptPath, "utf8");

// Must build literalPathspecs using the :(literal) magic prefix.
expect(scriptContent).toContain("`:(literal)${");
// Must pass literalPathspecs (not raw paths) to git status and git add.
expect(scriptContent).toContain("literalPathspecs");
expect(scriptContent).not.toContain("changedPathspecs");
});

it("should safely handle malicious branch names", () => {
// Test that malicious branch names would be rejected by git, not executed as shell commands
const maliciousBranchNames = [
Expand Down Expand Up @@ -1562,7 +1585,8 @@ describe("push_repo_memory.cjs - changed-file limit checks", () => {
const scriptContent = nodeFs.readFileSync(scriptPath, "utf8");

expect(scriptContent).toContain("changedFileCount");
expect(scriptContent).toContain('execGitSync(["status", "--porcelain"])');
expect(scriptContent).toContain('["status", "--porcelain"]');
expect(scriptContent).toContain('statusArgs.push("--", ...literalPathspecs)');
expect(scriptContent).toContain("Too many changed files");
expect(scriptContent).not.toContain("if (filesToCopy.length > maxFileCount)");
expect(scriptContent).toContain("if (changedFileCount > maxFileCount)");
Expand Down
Loading