Repository navigation
Revert unsupported YAML tag rejection in check-yaml - #2670
Conversation
This reverts commit 8555b9a.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: a3efc50177
ℹ️ 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".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2670 +/- ##
==========================================
- Coverage 94.10% 94.10% -0.01%
==========================================
Files 141 141
Lines 30044 30035 -9
==========================================
- Hits 28274 28265 -9
Misses 1770 1770 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
The restored “allow unknown/application tags” behavior is no longer covered by a regression test, increasing the risk of silently reintroducing the original failure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Reverts the recent check-yaml behavior that rejected YAML documents containing application-specific/unknown tags (notably those used by mkdocs.yml), and restores the prior serde-saphyr / granit-parser versions to match that behavior.
Changes:
- Remove
reject_unsupported_tagsfromcheck-yamlload options so tagged YAML can be validated again. - Adjust a config deserialization test expectation for group names containing a tab.
- Downgrade
serde-saphyrandgranit-parserback to1.1.0(with correspondingCargo.lockupdates).
File summaries
| File | Description |
|---|---|
| crates/prek/src/hooks/pre_commit_hooks/check_yaml.rs | Restores permissive tag handling during YAML load validation; removes the prior “unknown tag must fail” test. |
| crates/prek/src/config/mod.rs | Fixes test expectation for whitespace-in-name error message when the input includes a \t escape. |
| Cargo.toml | Downgrades YAML parsing crates to earlier versions consistent with restored behavior. |
| Cargo.lock | Updates lockfile checksums/versions to reflect the dependency downgrade. |
Review details
- Files reviewed: 3/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The intended behavior change is not covered by a replacement regression test, making it easy to accidentally reintroduce tag rejection later.
Review details
Suppressed comments (1)
crates/prek/src/hooks/pre_commit_hooks/check_yaml.rs:315
- The revert intentionally re-allows application-specific/unknown YAML tags, but removing the previous tag-related test leaves this behavior untested and easy to regress. Add a regression test that verifies a non-standard tag (e.g.
!reference) is accepted in load mode and produces no output.
#[tokio::test]
async fn test_yaml_with_binary_scalar() -> Result<()> {
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 898284455d
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
It disables the check-yaml hook and removes coverage for the reverted tag-handling behavior without adding a regression test for the intended “allow application-specific tags” behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
crates/prek/src/hooks/pre_commit_hooks/check_yaml.rs:314
- With unsupported-tag rejection removed, add a regression test that a YAML file containing an application-specific tag (like the mkdocs/ruamel tag
!!python/object/apply:pymdownx.slugs.slugify) loads successfully, so this behavior doesn’t regress again.
#[tokio::test]
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
|
Reverted this revert in #2672 |
Reverts #2656
The
reject_unsupported_tagsbehavior rejects valid application-specific YAML tags used bymkdocs.yml, including!!python/object/apply:pymdownx.slugs.slugify. Restore the previouscheck-yamlbehavior and parser versions.