fix: add directory support to consumer sync drift check#1445
Conversation
The check_consumer_sync_drift.py script crashed with IsADirectoryError when encountering directory entries (minimatch/, brace-expansion/, balanced-match/) in the sync manifest. Extract file comparison into _check_file() helper and add directory handling that recursively compares all files within directory entries, matching the fix applied to the sync workflow in PR #1435.
stranske
temporarily deployed
to
agent-standard
GitHub Actions
Inactive
github-actions
bot
added
the
autofix
The drift check script uses requests.Session() to directly query the GitHub API for file content comparison across consumer repos. This is appropriate for a Python utility script that runs in CI with its own token management, not through the JS-based retry wrapper.
Automated Status SummaryHead SHA: 439a9bd
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopePR #1409 (issue #1407) merged with CONCERNS from both verify:compare providers (openai 78%, anthropic 85%). The core unresolved concern is that custom regex-based glob matching was supposed to be replaced with a standard
Context for AgentRelated Issues/PRsContext for AgentRelated Issues/PRsContext for AgentRelated Issues/PRs
TasksReplace custom glob in bot-comment-dismiss.js
Standardize the vendored minimatch module
Fix consumer repo sync for minimatch
Standardize minimatch options
Acceptance criteria
|
stranske
temporarily deployed
to
agent-standard
GitHub Actions
Inactive
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15eb835fc4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for child in sorted(local_path.rglob("*")): | ||
| if child.is_file(): | ||
| rel = child.relative_to(local_path) | ||
| remote_target = f"{target}/{rel}" |
There was a problem hiding this comment.
When is_directory entries rely on target = entry.get("target", source), the manifest’s directory sources include a trailing slash (for example .../minimatch/), so remote_target = f"{target}/{rel}" builds paths with // in the middle. Those doubled separators are sent directly to the GitHub contents endpoint and can be interpreted as a different path, which leads to false missing reports for every file under that directory. This only appears for directory entries with trailing-slash targets, so trimming the trailing slash (or using path-join semantics) before appending rel avoids the drift false positives.
Useful? React with 👍 / 👎.
🤖 Keepalive Loop StatusPR #1445 | Agent: Codex | Iteration 0/5 Current State
🔍 Failure Classification| Error type | infrastructure | |
There was a problem hiding this comment.
Pull request overview
Fixes the consumer sync drift checker to handle is_directory entries in .github/sync-manifest.yml, avoiding crashes when the manifest includes directory-based dependencies (e.g., vendored minimatch and transitive deps).
Changes:
- Adds a
_check_file()helper to compare a single local file against its GitHub Contents API counterpart. - Introduces directory handling by walking local directories and comparing each contained file.
- Fixes an
errors.append(...)call to useerrors.add(...)(sinceerrorsis aset).
| if is_directory or local_path.is_dir(): | ||
| # Recursively compare all files within the directory | ||
| for child in sorted(local_path.rglob("*")): |
There was a problem hiding this comment.
is_directory or local_path.is_dir() will treat a directory as a directory even when the manifest entry is not marked is_directory, and it can also try to rglob() a path marked is_directory that isn’t actually a directory (leading to NotADirectoryError). This should be driven by the manifest flag, and mismatches between is_directory and local_path.is_dir() should be reported as an error instead of silently choosing one behavior.
| rel = child.relative_to(local_path) | ||
| remote_target = f"{target}/{rel}" | ||
| _check_file(child, remote_target, repo) |
There was a problem hiding this comment.
When constructing remote_target for directory entries, target from the manifest is typically a directory path that already ends with / (e.g. .github/scripts/node_modules/minimatch/). Using f"{target}/{rel}" can produce a double-slash path and also relies on OS-specific path separators via rel (problematic if run on Windows). Normalize target (strip trailing /) and build the remote path with POSIX separators (and ideally URL-encode the path segment) before calling the GitHub Contents API.
Automated Status Summary
Scope
PR #1409 (issue #1407) merged with CONCERNS from both verify:compare providers (openai 78%, anthropic 85%). The core unresolved concern is that custom regex-based glob matching was supposed to be replaced with a standard
minimatchlibrary, but:bot-comment-dismiss.jsstill defines ~140 lines of custom glob functions instead of importingminimatch.minimatchat.github/scripts/node_modules/minimatch/is a0.0.0-localreimplementation, not the real npm package.pr-context-graphql.jsandmerge_manager.jsbothrequire('minimatch'), but the template directory has nonode_modules/minimatch/and the sync manifest does not include it.Context for Agent
Related Issues/PRs
Context for Agent
Related Issues/PRs
Context for Agent
Related Issues/PRs
Tasks
Replace custom glob in bot-comment-dismiss.js
escapeRegExp,expandBraces,globToRegExp, andminimatchfunctions (lines 21–163) from.github/scripts/bot-comment-dismiss.jsconst { minimatch } = require('minimatch');import at the top ofbot-comment-dismiss.jsshouldIgnorePathinbot-comment-dismiss.jsto callminimatch(normalized, pattern, { dot: true, nocomment: true, nonegate: true })— matching the options used inmerge_manager.jstemplates/consumer-repo/.github/scripts/bot-comment-dismiss.jsStandardize the vendored minimatch module
.github/scripts/node_modules/minimatch/package.jsonversion from0.0.0-localto10.0.1-vendored(or current real minimatch major) and add a comment in the file explaining it is a vendored subsetnode --test .github/scripts/__tests__/bot-comment-dismiss.test.js && node --test tests/test/dismiss/dismiss-bot-comment.test.jsFix consumer repo sync for minimatch
.github/scripts/node_modules/minimatch/totemplates/consumer-repo/.github/scripts/node_modules/minimatch/.github/scripts/package.jsontotemplates/consumer-repo/.github/scripts/package.jsonnode_modules/minimatch/directory andpackage.jsonto.github/sync-manifest.ymlunder the scripts sectionStandardize minimatch options
bot-comment-dismiss.js,pr-context-graphql.js, andmerge_manager.jsall use consistent minimatch options ({ dot: true, nocomment: true, nonegate: true })node --test .github/scripts/__tests__/*.test.js && node --test tests/test/dismiss/*.test.jsAcceptance criteria
bot-comment-dismiss.jsdoes NOT contain any of:function escapeRegExp,function expandBraces,function globToRegExp, or a localfunction minimatch.Verification:
grep -c "function escapeRegExp\|function expandBraces\|function globToRegExp" .github/scripts/bot-comment-dismiss.jsreturns0.bot-comment-dismiss.jsimports minimatch viarequire('minimatch').Verification:
grep -c "require.*minimatch" .github/scripts/bot-comment-dismiss.jsreturns1.Template copy matches:
diff .github/scripts/bot-comment-dismiss.js templates/consumer-repo/.github/scripts/bot-comment-dismiss.jsexits 0.Consumer template has
node_modules/minimatch/:test -f templates/consumer-repo/.github/scripts/node_modules/minimatch/index.jsexits 0.All existing JS tests pass:
node --test .github/scripts/__tests__/*.test.js && node --test tests/test/dismiss/*.test.jsexits 0.Glob matching behavior is unchanged: brace expansion (
src/*.{ts,tsx}), character classes (src/[ab].ts), and escaped metacharacters (docs/\\[draft\\].md) continue to pass.Verification: tests in
bot-comment-dismiss.test.jsanddismiss-bot-comment.test.jscover these patterns and pass.Head SHA: bea55c4
Latest Runs: ✅ success — Gate
Required: gate: ✅ success
| Workflow / Job | Result | Logs |
|----------------|--------|------|
| Agents PR meta manager | ❔ in progress | View run |
| CI Autofix Loop | ✅ success | View run |
| Gate | ✅ success | View run |
| Health 40 Sweep | ✅ success | View run |
| Health 44 Gate Branch Protection | ❔ in progress | View run |
| Health 45 Agents Guard | ✅ success | View run |
| Health 50 Security Scan | ✅ success | View run |
| Health 72 Template Sync | ✅ success | View run |
| Health 73 Template Completeness | ✅ success | View run |
| Maint 52 Validate Workflows | ✅ success | View run |
| PR 11 - Minimal invariant CI | ✅ success | View run |
| Selftest CI | ✅ success | View run |
| Validate Sync Manifest | ✅ success | View run |
Head SHA: d50c21d
Latest Runs: ❔ in progress — Gate
Required: gate: ❔ in progress
| Workflow / Job | Result | Logs |
|----------------|--------|------|
| Agents Auto-Pilot | ⏭️ skipped | View run |
| Agents Bot Comment Handler | ✅ success | View run |
| Agents Keepalive Loop | ✅ success | View run |
| Agents PR meta manager | ❔ in progress | View run |
| Agents Verifier | ✅ success | View run |
| Auto-label Dependabot PRs | ⏭️ skipped | View run |
| CI Autofix Loop | ✅ success | View run |
| Copilot code review | ❔ in progress | View run |
| Create Issue from Verification (DEPRECATED) | ⏭️ skipped | View run |
| Create Issue from Verification (Enhanced) | ⏭️ skipped | View run |
| Create New PR from Verification | ⏭️ skipped | View run |
| Gate | ❔ in progress | View run |
| Health 40 Sweep | ✅ success | View run |
| Health 44 Gate Branch Protection | ✅ success | View run |
| Health 45 Agents Guard | ✅ success | View run |
| Health 50 Security Scan | ✅ success | View run |
| Health 73 Template Completeness | ✅ success | View run |
| Maint 52 Validate Workflows | ✅ success | View run |
| PR 11 - Minimal invariant CI | ✅ success | View run |
| Selftest CI | ✅ success | View run |
| Validate Sync Manifest | ✅ success | View run |
Head SHA: 56a8768
Latest Runs: ✅ success — Gate
Required: gate: ✅ success