Repository navigation
Preserve repo-memory files during safe-output validation - #65462
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Excluded files still affect formatting, custom validation, and patch-size measurement when already staged.
Review effort: Balanced
Findings: 3
Open (3)
What changed in this PR
Addresses #65458 by replacing destructive repo-memory filtering with eligibility checks during safe-output validation.
Changes:
- Filters file counting and staging without deleting excluded files.
- Adds preservation regressions, including legacy-path copies.
| File | Description |
|---|---|
| actions/setup/js/safe_outputs_handlers.test.cjs | Verifies excluded files survive validation. |
| actions/setup/js/safe_outputs_handlers.cjs | Applies eligibility checks instead of deleting files. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| const { compiledPatterns } = compileFileGlobPatterns(fileGlobFilter); | ||
| const isEligibleFile = relativePath => isMemoryFileEligible(relativePath, allowedExtensions, compiledPatterns).eligible; |
There was a problem hiding this comment.
Updated formatJSONFiles to receive the eligibility predicate and skip excluded JSON before reading or rewriting it. Regression coverage confirms excluded JSON remains byte-for-byte unchanged while eligible JSON formats successfully. Fixed in 1371073.
| if (!isEligibleFile(relPath)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Custom validation now runs on a temporary view containing only eligible files, preserving paths relative to the memory root and the validator's read-only check. Added coverage for malformed excluded JSON and read-only behavior. Fixed in 1371073.
| if (filesToStage.length > 0) { | ||
| execGitSync(["add", "--sparse", "--all", "--", ...filesToStage.map(file => `:(literal)${file}`)], { cwd: memoryDir, stdio: "pipe" }); | ||
| } | ||
| patchSizeBytes = getStagedPatchDiffSizeBytes({ execGitSyncFn: execGitSync, cwd: memoryDir }); |
There was a problem hiding this comment.
Patch sizing now scopes git diff --cached to literal eligible pathspecs, including eligible files already staged, and returns zero when none qualify without changing excluded index entries. Added a pre-staged excluded-file regression. Fixed in 1371073.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Request changes
The preflight now preserves ineligible repo-memory files, but it still lets those excluded files participate in later validation phases, so push_repo_memory can fail or rewrite content that the eventual push would ignore.
Blocking theme
allowed_extensions/file_glob are now applied to the file-count and staging pass only. formatJSONFiles(...) still walks the whole directory, and validation.script still receives the full memoryDir. In practice that means a legacy or sidecar file outside the persistence filter can still be reformatted or make custom validation fail, even though the later push path would drop it.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 52.1 AIC · ⌖ 7.18 AIC · ⊞ 19.4K
Comment /review to run again
| // Persistence filters apply to validation and staging, but validation must | ||
| // not delete files from the agent's working directory. | ||
| const { compiledPatterns } = compileFileGlobPatterns(fileGlobFilter); | ||
| const isEligibleFile = relativePath => isMemoryFileEligible(relativePath, allowedExtensions, compiledPatterns).eligible; |
There was a problem hiding this comment.
This still lets excluded files change or fail validation, because only the size/count scan is filtered while formatJSONFiles(...) and the custom validator still walk the entire memory directory.
💡 Apply the same persistence filter to every validation phase
With this patch, a file excluded by allowed_extensions or file_glob is preserved on disk, but it is still visible to later validation steps. format_json will still reformat excluded *.json files, and validation.script still receives the full memoryDir, so it can fail on files the later push would silently drop. That means push_repo_memory can still reject or mutate content outside the effective persistence set.
A safe fix is to keep the original files in place, but run formatting/custom validation against the same filtered view used for counting and staging.
There was a problem hiding this comment.
All persistence filters now apply to formatting, custom validation, size/count checks, patch measurement, and staging; excluded files remain untouched in the working tree and index. Fixed in 1371073.
There was a problem hiding this comment.
Reviewed with impeccable critique/audit lens (closest fit for this bug_fix/refactor_cleanup change, applied as a correctness/robustness review since there's no UI surface).
What changed: push_repo_memory validation no longer deletes ineligible files (via filterIneligibleMemoryFiles) before scanning. Instead, isMemoryFileEligible is applied inline during the scanDir walk to exclude ineligible files from the counted/validated set, and only eligible + already-tracked-eligible paths are staged with git add --sparse --all -- :(literal).... This matches the PR's stated goal: validation measures eligible changes without destructively removing files from the agent's working directory.
Verification performed:
- Walked the diff against
memory_file_eligibility.cjsandpush_repo_memory.cjsto confirm the push job's own filtering (filterIneligibleMemoryFilesthere) is unchanged and still the single place where ineligible files are actually dropped before a push — so excluded files remain in the agent's checkout but are still correctly never pushed. - Confirmed
git ls-files -z+ explicit:(literal)pathspecs correctly stage both new eligible files and already-tracked eligible files, and that an untracked/deleted file under an eligible pattern is handled consistently (git add --alltracks deletions too). - Ran
actions/setup/js/safe_outputs_handlers.test.cjs— all push_repo_memory tests pass, including the new regression test asserting mixed-eligibility directories keep excluded files on disk after validation.
No blocking or high-signal issues found; the change is well-scoped, the explanation is clear, and test coverage directly targets the described bug (#65458). Nothing actionable to flag inline.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 143.6 AIC · ⌖ 13.1 AIC · ⊞ 8.1K
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: fcca3aa
|
…ix-push-repo-memory Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |

push_repo_memoryvalidation could delete files excluded by persistence filters, leaving the working directory empty while reporting success. Validation should measure eligible changes without removing the agent’s files.