Skip to content

fix(workflow): validate changed project YAML files on Windows - #274

Open
ranjianghong526-hash wants to merge 2 commits into
bytedance:mainfrom
ranjianghong526-hash:codex/fix-windows-project-yaml-validation-20261011
Open

ranjianghong526-hash wants to merge 2 commits into
bytedance:mainfrom
ranjianghong526-hash:codex/fix-windows-project-yaml-validation-20261011

Conversation

@ranjianghong526-hash

@ranjianghong526-hash ranjianghong526-hash commented Oct 11, 2026 •

Copy link
Copy Markdown

Git reports changed paths with forward slashes on every platform, but str(rel) uses backslashes on Windows. The project YAML hook therefore skips modified files and incorrectly succeeds even when required metadata is missing.

Use rel.as_posix() for the changed-file lookup. The standard-library regression runs the actual checker with both POSIX and Windows path representations, covering staged, unstaged and unchanged cases. It keeps real temporary YAML files and metadata validation, and uses explicit subtest names. The test copyright year is 2026.

Validation:

  • One unittest covering six path/change combinations passes on Windows with Python 3.10.19 and 3.12.14. Restoring the old str(rel) implementation makes the two changed Windows-path subcases fail; the other four controls pass.
  • The exact same patch was verified on Linux on 2026-10-25: the old test incorrectly passed against the old checker, while the strengthened test detects both Windows-path failures and passes all six subcases with the fix.
  • Real temporary Git repositories and CLI executions: five scenarios per platform cover valid/invalid staged and unstaged changes and an unchanged invalid file. Windows was rechecked for this update; the unchanged Linux evidence is retained.
  • All applicable changed-file pre-commit checks pass: Ruff check/format, gitleaks and markdownlint. The YAML hook has no selected YAML files; its behavior is exercised separately by the regressions and real CLI cases.
  • git diff --check passes.

The previous hosted pre-commit run expired while awaiting fork-workflow approval and has zero jobs. Hosted CI must run after maintainer approval; local results are scoped to this hook and are not a full repository or cloud-deployment validation claim.

AI assistance was used for investigation, implementation, regression tests and validation.

@ranjianghong526-hash
ranjianghong526-hash marked this pull request as ready for review October 12, 2026 11:35
@ranjianghong526-hash

Copy link
Copy Markdown
Author

Hi maintainers, the pre-commit workflow for this PR is awaiting approval to run. Could a maintainer approve it when convenient so CI can validate the Windows YAML validation fix? I will address any failures or review feedback. Thank you!

@warm-wm warm-wm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the fix and the new unittest. The one-line fix is correct and minimal; I ran the test locally and verified the behavior. Please see one non-blocking suggestion about making the regression test platform-independent.

The fix

Root cause analysis is accurate: git diff --name-only always reports paths with forward slashes, while str(rel) on Windows uses backslashes, so the changed-file lookup never matched and the hook silently exited 0. rel.as_posix() is the right fix; it is a no-op on POSIX, so no behavior change there.

Test feedback (non-blocking)

  1. The regression test only catches the bug on Windows. I ran the new test on Linux against both the patched code and the original str(rel) line — it passes in both cases (on POSIX str(rel) == rel.as_posix()). So a future Linux CI run would not guard this fix. Consider extracting the matching into a tiny helper and asserting it with a Windows path, e.g.:

    from pathlib import PureWindowsPath
    # this assertion fails against the old str() code on every platform:
    self.assertTrue(is_changed(PureWindowsPath("python/sample/project.yaml"), {"python/sample/project.yaml"}))

    That makes the regression fail on all platforms without needing a Windows runner.

  2. Nit: the staged=None sentinel represents the "unchanged file" case, which reads a bit cryptically in a loop over (False, True, None). Three explicitly named subtests (unstaged / staged / unchanged) would read more clearly.

  3. Nit: other files under workflow_utils/ have no license header; either dropping the header to match siblings or bumping the year to 2026 would be more consistent.

Validation notes

  • I reproduced the test locally: 3 subcases pass with the fix.
  • The PR description's validation is thorough and honestly scoped (Windows unittest, temp-repo CLI runs for staged/unstaged, pre-commit on changed files).
  • No CI checks have run yet — the pre-commit workflow is still awaiting maintainer approval for this fork PR; worth triggering before merge.

Overall LGTM from me on the code change itself; the test hardening in point 1 would be nice to have in this PR but isn't a blocker.

Copy link
Copy Markdown
Author

Thanks @warm-wm for reviewing the fix. I strengthened the existing regression without adding a production helper: it now exercises the actual checker with PureWindowsPath and PurePosixPath at the relative_to() boundary, with explicitly named staged, unstaged and unchanged cases. I also corrected the test file's copyright year to 2026.

The old str(rel) checker fails the two changed Windows-path cases on both platforms; rel.as_posix() passes all six combinations. This update was rechecked on Windows with Python 3.10 and 3.12, including five real Git/CLI scenarios. The same unchanged patch was previously verified on Linux with the same six subcases and five CLI scenarios. All applicable local pre-commit hooks pass.

The earlier pre-commit run has now expired while awaiting approval, with no jobs executed. Could a maintainer approve the workflow triggered by this update so the hosted checks can run? Thank you!

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.

2 participants