Skip to content

Add Grug variant visual diff tooling and PR workflow#3127

Open
dlwh wants to merge 9 commits intomainfrom
codex/grug-variant-diff-workflow
Open

Add Grug variant visual diff tooling and PR workflow#3127
dlwh wants to merge 9 commits intomainfrom
codex/grug-variant-diff-workflow

Conversation

@dlwh
Copy link
Member

@dlwh dlwh commented Feb 28, 2026

Context

Grug code is deliberately template-y/copypasta, which makes reviewing diffs a bit tricky.

As a quick workaround, we made a tool to diff the nearest neighbor grug variant

See a slightly faked version of the workflow in action here

#3046 (comment)

Summary

  • add a one-page HTML visual directory diff tool for Grug/template-style code copies
  • document usage in experiments/grug/README.md
  • add CI helper to detect newly added Grug variants, find closest existing variant, and generate diff reports
  • add a PR workflow that publishes rendered reports to gh-pages and comments links on the PR

Testing

  • uvx ruff check scripts/grug_dir_diff.py scripts/grug_variant_diff_ci.py tests/test_grug_dir_diff.py tests/test_grug_variant_diff_ci.py
  • uv run --with pytest --with pytest-timeout python -m pytest -q tests/test_grug_dir_diff.py tests/test_grug_variant_diff_ci.py

@claude
Copy link
Contributor

claude bot commented Feb 28, 2026

Claude finished @dlwh's task in 1m 18s —— View job


  • Read all changed files
  • Check for bugs, correctness issues, and guideline violations
  • Post review

Review

Adds a local HTML directory-diff tool (scripts/grug_dir_diff.py), a CI wrapper that detects newly added Grug variants and finds the closest existing variant by line-delta (scripts/grug_variant_diff_ci.py), a GitHub Actions workflow to publish reports and comment on PRs, and supporting tests. The tooling works as described.

Specification check: This PR adds 1083 lines of new code. Per project guidelines (docs/recipes/agent-coding.md), a specification (design doc, .agents/projects/ file, or detailed issue description) is expected for PRs of this size. None is linked.

Findings:

  • Duplicate parse_extensions: scripts/grug_variant_diff_ci.py:206-221 re-implements parse_extensions nearly identically to scripts/grug_dir_diff.py:51-67. The CI script already imports several symbols from grug_dir_diff — this function should be imported rather than duplicated. Fix this →

  • Comparison uses HEAD, not base ref: build_manifest in grug_variant_diff_ci.py:154 filters candidate variant directories with (GRUG_ROOT / name).is_dir(), which checks the working tree at HEAD. If a PR modifies an existing variant and adds a new one, the diff report will compare against the modified (HEAD) version of the existing variant rather than the original (base) version. Depending on intent this may be fine for the common case (add-only PRs), but it's worth noting the comparison baseline is HEAD-on-disk rather than the base commit's tree.

  • list_variants_at_ref silently returns empty on git errors (grug_variant_diff_ci.py:45-46): If git ls-tree fails for any reason (bad ref, corrupt state), the function returns an empty set rather than surfacing the error. For the base ref this means all HEAD variants would appear "new" and generate reports. For the head ref it means no variants are detected at all. Consider failing loudly for at least the head ref, since a silent empty set could produce confusing CI output.

@dlwh dlwh marked this pull request as draft February 28, 2026 07:27
@dlwh
Copy link
Member Author

dlwh commented Feb 28, 2026

@claude wdyt now

@claude
Copy link
Contributor

claude bot commented Feb 28, 2026

Claude finished @dlwh's task in 1m 58s —— View job


  • Fetch latest changes and read all modified files
  • Check if previous review findings were addressed
  • Look for any new issues
  • Post updated review

Review

All three findings from the previous review have been addressed:

  1. Duplicate parse_extensions — removed from grug_variant_diff_ci.py; now imported from grug_dir_diff.
  2. Comparison uses HEAD, not base refbuild_manifest now materializes the base tree via materialize_grug_tree_at_ref into a temp directory and compares candidates against that snapshot rather than on-disk HEAD.
  3. list_variants_at_ref silently returns empty on git errors — now has a strict parameter (default True) that raises RuntimeError on failure, with a test confirming the behavior.

I re-ran ./infra/pre-commit.py --all-files (all checks pass) and the test suite (uv run pytest tests/test_grug_dir_diff.py tests/test_grug_variant_diff_ci.py — 6 passed in 0.09s). Code looks good to me.

One minor observation (not blocking): the base variant name is hardcoded in build_manifest at line 206 (candidate_names = sorted(base_variants | {"base"})). If the base template directory were ever renamed, this would need updating — but that's a reasonable assumption given the project layout.

View job ·

@dlwh
Copy link
Member Author

dlwh commented Feb 28, 2026

see a slightly faked version of the workflow in action here

#3046 (comment)

@dlwh dlwh marked this pull request as ready for review February 28, 2026 08:14
@claude
Copy link
Contributor

claude bot commented Feb 28, 2026

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


  • Read all changed files
  • Check for bugs, correctness issues, and guideline violations
  • Post review

Review

Adds HTML directory-diff tooling (scripts/grug_dir_diff.py), a CI helper that detects new Grug variants and generates diff reports against the closest existing variant (scripts/grug_variant_diff_ci.py), a GitHub Actions workflow to publish reports and comment on PRs, and tests. The tooling matches the stated objectives.

Specification check: 1188 lines of new code with no linked specification (issue, design doc, or .agents/projects/ file). Per docs/recipes/agent-coding.md, a specification is expected for PRs of this size.

Findings:

  • Workflow comment step skipped on gh-pages publish failure: .github/workflows/grug-variant-diff.yaml:81 — the "Post PR comment" step has if: github.event.pull_request.head.repo.full_name == github.repository, but no always() or !cancelled() guard. If the preceding "Publish rendered report to gh-pages" step (line 70) fails (e.g., gh-pages branch doesn't exist, transient permission error), GitHub Actions skips all subsequent steps by default. This means no PR comment is posted at all — not even the artifact-only fallback the JS code already handles (lines 121-143). Fix: change the condition to if: "!cancelled() && github.event.pull_request.head.repo.full_name == github.repository" so the comment step runs even when publishing fails. Fix this ->

Previous review findings (duplicate parse_extensions, HEAD-vs-base comparison, silent empty on git error) were all addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant