Skip to content

fix: set origin fetch refspec and update workflow permissions - #136

Merged
dclong merged 1 commit into
mainfrom
push-ssxotmltqpun
Aug 16, 2026
Merged

dclong merged 1 commit into
mainfrom
push-ssxotmltqpun

Conversation

@dclong

@dclong dclong commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • fix(github): set origin fetch refspec and update workflow permissions

Changed files

Modified

  • .github/workflows/claude.yaml (+1/-0)
  • .github/workflows/release.yaml (+4/-0)
  • github_rest_api/scripts/github/create_github_repo.py (+9/-1)

Commits

  • 320be2e fix(github): set origin fetch refspec and update workflow permissions

@dclong

dclong commented Aug 16, 2026 •

Copy link
Copy Markdown
Collaborator Author

Claude finished @dclong's task in 2m 1s —— View job


Code Review

  • Gather context (PR diff, files changed)
  • Review the change
  • Post review findings and merge-safety decision

Note: I was unable to run uv sync, pytest, ruff, or ty/pyright in this environment — the uv run ... commands required approval that isn't available in this non-interactive context. This review is based on reading the diff and surrounding code directly.

Summary of changes

  1. github_rest_api/scripts/github/create_github_repo.py — _ensure_remote now explicitly sets the fetch refspec (+refs/heads/*:refs/remotes/origin/*) on the origin remote after adding it, since porcelain.remote_add only writes the URL.
  2. .github/workflows/claude.yaml — adds contents: read to the job's permissions block.
  3. .github/workflows/release.yaml — adds a permissions block (contents: write, issues: write, pull-requests: write) to the release job.

Findings

Correctness — non-blocking edge case
In _ensure_remote (github_rest_api/scripts/github/create_github_repo.py:44-57), the new fetch-refspec logic only runs on the success path of porcelain.remote_add. When porcelain.RemoteExists is caught, the function returns immediately and skips setting the fetch refspec:

    except porcelain.RemoteExists:
        return
    # fetch refspec fix below only runs if remote_add succeeded

If origin already exists (e.g. a retry after a prior partial run, or a remote added by an older, unpatched version of this script), the refspec is never repaired and the exact bug this PR fixes can persist for that repo. Since porcelain.remote_add "only writes the remote's url" regardless of whether it's a fresh or pre-existing remote (per the new comment), it would be more robust to set the fetch refspec unconditionally (moving it outside the try/except, or into a finally), rather than only for the newly-added-remote path. This isn't a regression and doesn't affect the primary case described in the PR (adding a brand-new remote), so I don't consider it blocking.

Test coverage — minor suggestion
No test was added/updated in tests/test_create_github_repo.py to assert the fetch config is set after _ensure_remote/_init_local_repo runs. Given this is the crux of the fix, a small regression test (e.g. asserting r.get_config().get((b"remote", b"origin"), b"fetch") after _init_local_repo) would guard against future regressions.

Workflow permission changes
Both workflow permission additions look reasonable and additive (no scopes removed). I can't verify the internals of the reusable legendu-net/.github/.github/workflows/*.yml@main workflows they call into, but the added scopes (contents: read for Claude's checkout/git operations, contents/issues/pull-requests: write for the release job) are consistent with what those jobs would plausibly need.

Verdict

No correctness bugs or blocking issues found. The one edge case noted above is narrow, doesn't affect the primary documented use case, and is a good opportunity for a follow-up rather than a blocker.

Fix this →

AUTO_MERGE_APPROVED

@dclong
dclong force-pushed the push-ssxotmltqpun branch from 320be2e to b4e8c50 Compare August 16, 2026 16:12
@dclong

dclong commented Aug 16, 2026 •

Copy link
Copy Markdown
Collaborator Author

Claude finished @dclong's task in 5m 19s —— View job


Code Review (re-run after new commits)

  • Gather context (PR diff, changed files)
  • Run /code-review:code-review on the current PR state
  • Post review findings and merge-safety decision

Review is running in the background (finder agents analyzing correctness and cleanup/convention angles across the diff, which now also includes deletion of several workflow files and changes to create_github_repo.py / tests/test_create_github_repo.py).

@dclong
dclong force-pushed the push-ssxotmltqpun branch from b4e8c50 to 0953e17 Compare August 16, 2026 18:46
@dclong

dclong commented Aug 16, 2026 •

Copy link
Copy Markdown
Collaborator Author

Claude finished @dclong's task in 2m 2s —— View job


Code Review (re-run after new commits)

  • Gather context (PR diff, changed files)
  • Run code review on the current PR state
  • Post review findings and merge-safety decision

Note: As in the previous run, I was unable to execute uv run pytest/ruff/ty/pyright or even plain python3/git fetch in this environment — all non-trivial Bash invocations require an approval that isn't available in this non-interactive context. This review is based on reading the diff and surrounding code directly (the local origin/main ref was already present, so the diff itself was verifiable).

Summary of changes since the last review

This push significantly expands the PR:

  1. _ensure_remote fix is now more robust (github_rest_api/scripts/github/create_github_repo.py:48-71): the previous review flagged that the fetch-refspec repair only ran on the newly-added-remote path and was skipped when porcelain.RemoteExists was caught. That's now fixed — the except porcelain.RemoteExists: pass no longer returns early, and the refspec repair logic runs unconditionally afterward, guarded by checking the existing origin URL matches before touching it. Good.
  2. Test coverage added: tests/test_create_github_repo.py now has test_ensure_remote_repairs_fetch_refspec_for_matching_existing_origin and test_ensure_remote_leaves_mismatched_existing_origin_untouched, directly covering the fix and the edge case raised previously.
  3. _add_workflow reworked to fetch templates from a GitHub repo via the REST API instead of copying bundled local files, and the local github_rest_api/scripts/github/workflows/* template files are deleted (superseded by fetching from .github/workflows/ of a configurable workflows_repo, defaulting to this repo). The corresponding CI sync-check step in .github/workflows/check_workflow_template.yaml is removed since there's no longer a local copy to keep in sync.
  4. New -w/--workflows-repo CLI flag, empty string to opt out.
  5. Workflow permission additions (claude.yaml, release.yaml) — unchanged from the previous review, still additive only.

Findings

No correctness bugs found. Specifically checked:

  • _download_workflow (create_github_repo.py:229-260): correctly omits the Authorization header for the download_url path (raw.githubusercontent.com embeds a short-lived signed token for private-repo content in the URL itself; forwarding a token scoped to a possibly-unrelated repo can cause spurious 404s) and correctly adds it plus the raw Accept header for the contents-API fallback.
  • _list_workflow_entries/_add_workflow: correctly filter to .yaml files, exclude _REPO_ONLY_WORKFLOWS, skip existing local files (don't clobber customizations), and raise FileNotFoundError when no templates are found — all covered by new tests (including the dir-type entry being skipped, and the empty/workflows_repo="" skip path).
  • No leftover references to the deleted github_rest_api/scripts/github/workflows/ directory anywhere else in the codebase (pyproject, other scripts, docs).
  • Workflow permission additions remain additive/non-blocking, as previously assessed.

Minor, non-blocking observations:

  • This changes _add_workflow's default behavior from a purely local/offline file copy to a network call against the GitHub API on every create_github_repo invocation (unless --workflows-repo "" is passed). This is clearly intentional given the deleted local templates, but worth being aware of for any CI/offline usage of this script that doesn't already expect network access.
  • _list_workflow_entries assumes the contents API returns a list (i.e., .github/workflows is a directory); if it were ever a file, resp.json() would return a dict and the iteration would raise a TypeError rather than a clear error. Extremely unlikely in practice, not worth guarding against.

Verdict

No correctness bugs or blocking issues. The fetch-refspec edge case flagged in the previous review has been fixed and covered by new tests, and the workflow-template rework is coherent and well-tested.

AUTO_MERGE_APPROVED

@dclong
dclong merged commit 56c080f into main Aug 16, 2026
21 checks passed
@dclong
dclong deleted the push-ssxotmltqpun branch August 18, 2026 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant