repo-memory: filter disallowed files before validation/upload instead of failing downstream in push#58120
Conversation
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the implementation label and has <=100 new lines of code in business logic directories.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"
- "github.com"See Network Configuration for more information.
|
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new repo-memory filtering path still breaks valid workflows that persist files at the repo-memory root, so this is not safe to merge as-is.
Blocking theme
The shared eligibility helper now forces every slashless file-glob pattern through matchSubfolderRoot, which only matches files exactly one directory below the memory root. That silently changes existing semantics for common patterns like .json/*.json/*.md: root-level files that previously matched are now filtered out before validation, upload, and push. The regression is visible in this PR's generated lockfiles as well (daily-awf-spec-compiler-surfacing now emits FILE_GLOB_FILTER: ".json .md"), so workflows that write root-level memory files will degrade to no-op runs instead of persisting their data.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 36 AIC · ⌖ 7.51 AIC · ⊞ 21.8K
Comment /review to run again
| return { patternStrs: [], compiledPatterns: [] }; | ||
| } | ||
| const patternStrs = fileGlobFilter.trim().split(/\s+/).filter(Boolean); | ||
| const compiledPatterns = patternStrs.map(pattern => globPatternToRegex(pattern, { matchSubfolderRoot: !pattern.includes("/") })); |
There was a problem hiding this comment.
This helper now rewrites every slashless file-glob into a matchSubfolderRoot regex, which means patterns like *.json or .json stop matching files at the repo-memory root. Existing workflows use those patterns for root-level state files, so this change will silently filter out valid memory output and turn successful runs into no-op uploads/pushes.
💡 Why this blocks
globPatternToRegex(..., { matchSubfolderRoot: true }) only matches subdir/file.ext, never file.ext. That is a semantic change, not just refactoring: the previous push path matched the full relative path directly, and the compiler is still generating slashless filters in lockfiles such as daily-awf-spec-compiler-surfacing (FILE_GLOB_FILTER: ".json .md"). After this lands, a root-level processed-discussions.json or latest-run.md will be deleted by the new filter step before custom validation and before artifact upload, so the workflow quietly stops persisting its main outputs. Keep the old slashless-pattern semantics here, or normalize those patterns in the compiler before routing them through the shared helper.
There was a problem hiding this comment.
🟡 Changes recommended
Preflight and downstream validation remain inconsistently scoped, and filter failures can still allow unfiltered artifacts to upload.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds consistent repo-memory eligibility filtering before validation, upload, and downstream persistence.
Changes:
- Adds a shared extension/glob eligibility helper with tests.
- Filters ineligible files during push scanning and supports clean no-op exits.
- Generates pre-upload filtering steps and refreshes affected workflow lock files.
File summaries
| File | Description |
|---|---|
pkg/workflow/repo_memory.go |
Generates the pre-upload filter step. |
actions/setup/js/memory_file_eligibility.cjs |
Implements shared eligibility filtering. |
actions/setup/js/memory_file_eligibility.test.cjs |
Tests extension and glob filtering. |
actions/setup/js/push_repo_memory.cjs |
Filters files during downstream scanning. |
actions/setup/js/push_repo_memory.test.cjs |
Adds push filtering regression checks. |
.github/workflows/workflow-health-manager.lock.yml |
Adds generated filtering step. |
.github/workflows/smoke-ci.lock.yml |
Adds generated filtering step. |
.github/workflows/sergo.lock.yml |
Adds generated filtering step. |
.github/workflows/security-compliance.lock.yml |
Adds generated filtering step. |
.github/workflows/pr-triage-agent.lock.yml |
Adds generated filtering step. |
.github/workflows/metrics-collector.lock.yml |
Adds generated filtering step. |
.github/workflows/eslint-refiner.lock.yml |
Adds generated filtering step. |
.github/workflows/delight.lock.yml |
Adds generated filtering step. |
.github/workflows/deep-report.lock.yml |
Adds generated filtering step. |
.github/workflows/daily-testify-uber-super-expert.lock.yml |
Adds generated filtering step. |
.github/workflows/daily-storify.lock.yml |
Adds generated filtering step. |
.github/workflows/daily-safeoutputs-git-simulator.lock.yml |
Adds generated filtering step. |
.github/workflows/daily-news.lock.yml |
Adds generated filtering step. |
.github/workflows/daily-harness-experiment-proposer.lock.yml |
Adds generated filtering step. |
.github/workflows/daily-formal-spec-verifier.lock.yml |
Adds generated filtering step. |
.github/workflows/daily-cli-performance.lock.yml |
Adds generated filtering step. |
.github/workflows/daily-awf-spec-compiler-surfacing.lock.yml |
Adds generated filtering step. |
.github/workflows/copilot-session-insights.lock.yml |
Adds generated filtering step. |
.github/workflows/copilot-pr-prompt-analysis.lock.yml |
Adds generated filtering step. |
.github/workflows/copilot-pr-nlp-analysis.lock.yml |
Adds generated filtering step. |
.github/workflows/copilot-cli-deep-research.lock.yml |
Adds generated filtering step. |
.github/workflows/copilot-centralization-optimizer.lock.yml |
Adds generated filtering step. |
.github/workflows/copilot-agent-analysis.lock.yml |
Adds generated filtering step. |
.github/workflows/audit-workflows.lock.yml |
Adds generated filtering step. |
.github/workflows/agentic-token-optimizer.lock.yml |
Adds generated filtering step. |
.github/workflows/agentic-token-audit.lock.yml |
Adds generated filtering step. |
.github/workflows/agent-performance-analyzer.lock.yml |
Adds generated filtering step. |
Review details
- Files reviewed: 32/32 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| builder.WriteString(" const memoryDir = process.env.MEMORY_DIR || '';\n") | ||
| builder.WriteString(" const allowedExtensions = JSON.parse(process.env.ALLOWED_EXTENSIONS || '[]');\n") | ||
| builder.WriteString(" const fileGlobFilter = process.env.FILE_GLOB_FILTER || '';\n") | ||
| builder.WriteString(" filterIneligibleMemoryFiles(memoryDir, allowedExtensions, fileGlobFilter, core);\n") |
| // Allowed extensions and file-glob are persistence filters: files that do not | ||
| // pass are logged and ignored (never uploaded, validated, counted toward | ||
| // max-file-count/size/patch-size, or pushed) rather than causing a hard failure. | ||
| const eligibility = isMemoryFileEligible(relativeFilePath, allowedExtensions, compiledPatterns); |
| fmt.Fprintf(builder, " - name: Filter %s files (%s)\n", memoryLabel, memory.ID) | ||
| builder.WriteString(" if: always()\n") |
🧪 Test Quality Sentinel Report
📊 Metrics (56+ tests)
Test Analysis by File
|
| Test | Classification | Value | Status |
|---|---|---|---|
| allows all files when no config | design_test | high_value | ✅ |
| rejects disallowed extensions | design_test | high_value | ✅ REGRESSION |
| accepts allowed extension | design_test | high_value | ✅ |
| case-insensitive, trim whitespace | design_test | medium_value | ✅ |
| rejects no glob match | design_test | high_value | ✅ |
| requires both filters pass | design_test | high_value | ✅ |
| compileFileGlobPatterns empty | implementation_test | low_value | ✓ |
| compileFileGlobPatterns space-separated | implementation_test | low_value | ✓ |
| removes disallowed, keeps allowed | design_test | high_value | ✅ REGRESSION |
| no-op success when no eligible files | design_test | high_value | ✅ KEY FIX |
| glob + extension filtering | design_test | high_value | ✅ |
| nested dirs, skip .git | design_test | high_value | ✅ |
| non-existent directory | design_test | medium_value | ✅ |
Strengths:
- ✅ Direct regression test for the reported issue (notes.json.new)
- ✅ Validates the critical fix: "no eligible files → clean success, not hard failure"
- ✅ Comprehensive edge case coverage (.git skip, missing directory, whitespace handling)
- ✅ File I/O safety:
fs.rmSynccleanup inafterEach - ✅ All design tests (84.6%); only trivial implementation tests for compile helpers
actions/setup/js/push_repo_memory.test.cjs ✅ (MODIFIED, +30 lines)
43 new tests added to existing file, split into three suites:
Test Suite Breakdown
Suite 1: globPatternToRegex helper (24 tests)
- Basic pattern matching (exact, *, **, nesting): 6 tests ✅
- Special character escaping (dots, backslashes, multiple): 3 tests ✅
- Real-world patterns (*.jsonl, nested metrics, prefix wildcards): 6 tests ✅
- Edge cases (empty, *, **, complex nesting): 5 tests ✅
- Regex output format (RegExp, anchors, conversions): 4 tests ✅
Suite 2: Security tests (19+ tests)
- Glob-to-regex conversion with escape-order validation: 11 tests ✅
- Includes security fix verification: backslash escape BEFORE dot escape
- Demonstrates CWE-20/80/116 prevention
- Subdirectory glob patterns (**, , **/): 5 tests ✅
- Security implications (bypass attempts, validation): 3+ tests ✅
Strengths:
- ✅ Security-critical design tests: All 43 tests validate behavioral contracts (regex output, wildcard semantics, escape order)
- ✅ Dedicated security suite with CWE references (CWE-20, CWE-80, CWE-116)
- ✅ Escape-order validation explicitly tests the fix (backslashes first, then dots)
- ✅ Wildcard disambiguation: clear tests for *, **, and */ semantics
- ✅ Real-world use case: daily-code-metrics workflow validation with *.jsonl pattern
- ✅ No mocking; pure regex/string behavior validation
- ✅ Comprehensive edge cases: empty patterns, special chars, nested paths
Verdict
✅ Passed. 96.4% design tests (threshold: ≤30% implementation). No violations.
Key findings:
- Strong behavioral coverage of both core issue (file filtering) and security fix (glob pattern matching)
- Regression test explicitly addresses issue repo-memory: ignore disallowed files before artifact upload and push #58119 (notes.json.new rejection)
- Critical fix validated: "no eligible files" → clean no-op, not hard failure
- Security fix validated: glob-to-regex escape order prevents ReDoS/bypass attacks
- Test inflation acceptable in refactor context (push_repo_memory: -25 net production lines, +30 test lines for bugfix/security improvements)
Recommendation: ✅ Approve — tests demonstrate strong design validation and comprehensive behavioral coverage.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🧪 Test quality analysis by Test Quality Sentinel · copilot · haiku45 · 25.9 AIC · ⌖ 8.88 AIC · ⊞ 8.4K · ◷
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd (via pr-triage: change_type=bug_fix) — the fix itself is sound and well-designed (single source of truth for eligibility via memory_file_eligibility.cjs, applied consistently at filter/scan/validate time), but test coverage has two gaps worth closing.
📋 Key Themes & Highlights
Key Themes
- Missing compiler-level test: the new "Filter repo-memory files" step in
pkg/workflow/repo_memory.go(step name, env vars, ordering) has no Go test asserting it's emitted correctly — only the JS side is tested. - Source-string-only regression tests: the two new tests in
push_repo_memory.test.cjsgrep the source file for expected substrings rather than exercising the actual scan/filter behavior end-to-end, so they wouldn't catch subtle wiring regressions.
Positive Highlights
- ✅ Clean extraction of
isMemoryFileEligible/compileFileGlobPatterns/filterIneligibleMemoryFilesinto a single shared helper, eliminating the duplicated glob-compile logic that caused the original inconsistency. - ✅ Thorough, well-named unit tests in
memory_file_eligibility.test.cjscovering the actual regression scenario, nested dirs,.gitexclusion, and the no-eligible-files no-op case. - ✅ Root cause fix — replaces the downstream hard-fail with consistent upstream filtering, plus fixes the latent
return→continuebug that silently aborted directory scans.
Non-blocking — the fix logic is correct and behaviorally tested at the helper level; addressing the two comments would harden regression coverage at the integration points.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 81.1 AIC · ⌖ 14.9 AIC · ⊞ 10.3K
Comment /matt to run again
| } | ||
| builder.WriteString(" with:\n") | ||
| builder.WriteString(" script: |\n") | ||
| builder.WriteString(" const { setupGlobals } = require('${{ runner.temp }}/gh-aw/actions/setup_globals.cjs');\n") |
There was a problem hiding this comment.
[/tdd] No Go-level test asserts the new "Filter" step is emitted correctly (name, env vars, script content, and ordering vs. Sanitize/Validation/Upload steps). Existing coverage for this behavior lives only in .cjs source-string-matching tests, which don't verify the compiler wiring itself.
💡 Suggested test
Add a case in pkg/workflow/repo_memory_test.go alongside TestGenerateRepoMemoryArtifactUpload asserting:
- The
Filter repo-memory files (%s)step name appears whenAllowedExtensions/FileGlobare set, and that it appears after "Sanitize" and before "Validate"/"Upload". ALLOWED_EXTENSIONSandFILE_GLOB_FILTERenv vars are populated with the expected values.- Since
AllowedExtensionsdefaults to a non-empty list viaextractRepoMemoryConfig, confirm the "no filter step" branch is only reachable when a caller explicitly empties both fields (an edge case not currently exercised).
@copilot please address this.
There was a problem hiding this comment.
Added TestRepoMemoryFilterStepGatesUpload (now asserts the "Filter repo-memory files" step name/ordering and the exact ALLOWED_EXTENSIONS/FILE_GLOB_FILTER env values) and TestRepoMemoryFilterStepEmptyBothFieldsSkipsFilter (asserts the no-filter branch is only reachable when both fields are empty) in 91e55b2.
| }); | ||
|
|
||
| describe("push_repo_memory.cjs - allowed-extensions persistence filter (regression: notes.json.new)", () => { | ||
| it("filters ineligible files before validation/upload instead of hard-failing on them (source check)", () => { |
There was a problem hiding this comment.
[/tdd] Both new tests only grep the source file for expected substrings — they never actually invoke the scanDirectory/eligibility logic against real files, so they'd pass even if the wiring were subtly broken (e.g. wrong argument order, wrong reason string, filter applied after size validation instead of before).
💡 Suggested behavioral test
Since memory_file_eligibility.test.cjs already unit-tests isMemoryFileEligible/filterIneligibleMemoryFiles directly and thoroughly, complement these with a behavioral test that runs the real push_repo_memory.cjs main() (or an extracted scanDirectory-equivalent) against a temp source directory containing an eligible and an ineligible file, then asserts on filesToCopy/filteredOutFiles results and that the ineligible file never reaches the destination — mirroring the regression fixture from issue #58119 end-to-end rather than just matching strings in the source text.
@copilot please address this.
There was a problem hiding this comment.
Replaced the source-grep-only test with a real behavioral test in 91e55b2: it writes actual notes.json/notes.json.new fixtures to a temp dir and drives the real isMemoryFileEligible/compileFileGlobPatterns helpers through the identical scan loop used in scanDirectory, asserting the ineligible file is filtered out and the eligible one is kept.
Note: I couldn't drive this through main() directly — vi.doMock can't intercept the CJS require() for git_helpers.cjs in this dynamic-import test setup (there's already a documented case of this in the same file, "should propagate git fetch authentication failure..."), so a true end-to-end run needs a real git remote. The new test instead exercises the exact real eligibility logic push_repo_memory.cjs runs inline, without relying on string matching.
There was a problem hiding this comment.
Reviewed the repo-memory persistence-filter change (memory_file_eligibility.cjs, push_repo_memory.cjs, repo_memory.go, regenerated lock files).
Correctness: the shared isMemoryFileEligible/filterIneligibleMemoryFiles helper is applied consistently across the new compiler-emitted filter step and the scan loop in push_repo_memory.cjs, replacing the old whole-directory hard-fail (validateMemoryFiles) with per-file skip-and-log — matches the PR description and fixes the reported notes.json.new regression. The .git directory is correctly excluded from the recursive scan/delete. Bug fix (return → continue) is verified in the diff.
Tests: JS-side logic is thoroughly covered by memory_file_eligibility.test.cjs (extension/glob interaction, nested dirs, .git exclusion, no-op case) and updated push_repo_memory.test.cjs. One gap: the new Go compiler logic in repo_memory.go (conditional "Filter %s files" step emission, env var wiring) has no dedicated Go test — left an inline comment with a concrete suggestion.
No blocking issues found; the inline comment is a suggestion for improved coverage, not a correctness concern.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 99.7 AIC · ⌖ 14.4 AIC · ⊞ 8.3K
Comments that could not be inline-anchored
pkg/workflow/repo_memory.go:416
New compiler logic that emits the "Filter %s files" step (env vars ALLOWED_EXTENSIONS/FILE_GLOB_FILTER, step ordering before validate/upload) has no Go-level test coverage. TestGenerateRepoMemoryArtifactUpload in repo_memory_test.go already asserts step ordering and env vars for the sibling sanitize/upload steps — consider adding a similar case asserting that: (1) the Filter step is only emitted when AllowedExtensions or FileGlob is set, (2) it appears between the sanitize and valid…
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
Addressed in 872b7d2:
Also split |
Confirmed: HEAD (e52731a) still contains all fixes (root-level glob matching, preflight/downstream/push consistency, fail-open gating, doc corrections) — no new code changes needed this pass. I don't have a tool to dismiss/re-request the stale |
|
Category: bug | Risk: high | Score: 78 | Action: fast_track Note: Core repo-memory persistence bug fix with broad downstream impact.
|
|
@copilot Please take another pass on this PR: refresh the branch if needed, address any remaining review feedback, and run the pr-finisher skill before handing back to maintainers.
|
No new review feedback has appeared since the last pass — HEAD is still e52731a with all prior fixes and test coverage in place, and the working tree has no pending changes. There's nothing new to act on; the outstanding blocker remains the stale |
|
🎉 Thanks for this fix! This PR is part of the core team's agentic development workflow and looks ready for review. ✅ What's covered:
The PR does one thing well and includes all necessary tests and documentation. Ready to merge! 🚀 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
Category: bug | Risk: medium | Score: 71 (impact 34 + urgency 19 + quality 18) | Action: fast_track | CI: passing (2/2) Core repo-memory reliability fix with green checks.
|
@copilot Please take the next forward-progress pass on this PR.
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
@copilot Please take the next forward-progress pass on this PR.
|
Confirmed explicitly: the remaining blocker is not a code issue. HEAD (e52731a) already contains the root-level slashless file-glob fix, the shared script-file pattern, and consistent filtering across preflight/agent-validation/push for all memory types, with test coverage. The |
@copilot Please take the next forward-progress pass on this PR.
|
Confirmed again: HEAD is unchanged at Explicitly, per this comment's own request: the remaining blocker is not a code issue. It is the stale |
allowed-extensions/file-globon repo-memory were enforced inconsistently: the agent-side validation step never receivedALLOWED_EXTENSIONS, so ineligible files (e.g. a straynotes.json.new) passed artifact upload and only surfaced as a hard failure in the downstreampush_repo_memoryjob — after the eligible file had already been staged.Persistence filtering, applied consistently
actions/setup/js/memory_file_eligibility.cjs:isMemoryFileEligible,compileFileGlobPatterns,filterIneligibleMemoryFiles. Disallowed files are logged and skipped, never a hard failure.pkg/workflow/repo_memory.go) now emits a "Filter repo-memory files" step before custom validation and artifact upload, wheneverallowed-extensionsorfile-globis set — so validation, the uploaded artifact, and the push job all see the same effective file set.push_repo_memory.cjsscanDirectorynow applies extension + glob eligibility inline during the scan, dropping ineligible files from consideration entirely (they no longer count toward max-file-count/size/patch-size).validateMemoryFileshard-fail over the whole directory — the old failure mode this issue reports.returninside the per-file loop, silently aborting the scan of the rest of that directory instead of just skipping one entry (continue).Result
Regenerated
.lock.ymlfiles for all workflows using repo-memory to include the new filter step.Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
pr-sous-chef https://github.com/github/gh-aw/actions/runs/33831313247