Skip to content

fix: add directory support to consumer sync drift check #1445

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stranske merged 2 commits into from
Feb 10, 2026
Merged

fix: add directory support to consumer sync drift check #1445

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15eb835
fix: add directory support to consumer sync drift check
stranske Feb 10, 2026
56a8768
fix: add check_consumer_sync_drift.py to API wrapper guard skip list
stranske Feb 10, 2026
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
48 changes: 30 additions & 18 deletions scripts/check_consumer_sync_drift.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,25 @@ def main() -> int:
errors: set[str] = set()
obsolete: set[str] = set()

def _check_file(local_file: Path, remote_target: str, repo: str) -> None:
"""Compare a single local file against its remote counterpart."""
local_digest = file_hash(local_file.read_bytes())
url = f"https://api.github.com/repos/{repo}/contents/{remote_target}"
response = session.get(url)
if response.status_code == 404:
missing.add(f"{repo}: {remote_target}")
return
if response.status_code >= 400:
errors.add(f"{repo}: {remote_target} (HTTP {response.status_code})")
return
data = response.json()
if data.get("encoding") != "base64" or "content" not in data:
errors.add(f"{repo}: {remote_target} (unexpected content encoding)")
return
remote_content = base64.b64decode(data["content"])
if file_hash(remote_content) != local_digest:
drift.add(f"{repo}: {remote_target}")

for section in sections:
for entry in manifest.get(section, []) or []:
source = entry.get("source")
Expand All @@ -97,29 +116,22 @@ def main() -> int:
if entry.get("sync_mode") == "create_only":
continue
target = entry.get("target", source)
is_directory = entry.get("is_directory", False)
local_path = local_path_for(source)
if not local_path:
errors.append(f"{section}: missing local file for {source}")
errors.add(f"{section}: missing local file for {source}")
continue

local_digest = file_hash(local_path.read_bytes())

for repo in repos:
url = f"https://api.github.com/repos/{repo}/contents/{target}"
response = session.get(url)
if response.status_code == 404:
missing.add(f"{repo}: {target}")
continue
if response.status_code >= 400:
errors.add(f"{repo}: {target} (HTTP {response.status_code})")
continue
data = response.json()
if data.get("encoding") != "base64" or "content" not in data:
errors.add(f"{repo}: {target} (unexpected content encoding)")
continue
remote_content = base64.b64decode(data["content"])
if file_hash(remote_content) != local_digest:
drift.add(f"{repo}: {target}")
if is_directory or local_path.is_dir():
# Recursively compare all files within the directory
for child in sorted(local_path.rglob("*")):
Comment on lines +126 to +128

Copilot AI Feb 10, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
if child.is_file():
rel = child.relative_to(local_path)
remote_target = f"{target}/{rel}"

@chatgpt-codex-connector chatgpt-codex-connector Bot Feb 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Normalize directory target before building child API path

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 👍 / 👎.

_check_file(child, remote_target, repo)
Comment on lines +130 to +132

Copilot AI Feb 10, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
else:
_check_file(local_path, target, repo)

for entry in manifest.get("removals", []) or []:
target = entry.get("target")
Expand Down
Loading