feat: no-production-newtonsoft hook, on shared YAML/reporting helpers - #5
Merged
Conversation
`stevedore-release-scope` grew a MISSING sentinel, a `load_yaml` that treats an absent file as a clean no-op but a malformed one as fatal, and a GHA-aware `emit`. All three are behavior every Python hook in this repo has to get identically right, and the next hook needs them, so move them into a private `pinpredict_hooks._common` rather than copy them. `load_yaml` and `emit` take the caller's prog/label so their messages stay attributed to the hook that failed. No behavior change; the existing stevedore-release-scope tests cover the refactor.
Production .NET code should use System.Text.Json; a Newtonsoft.Json reference belongs only in explicitly approved test and benchmark projects. trading enforced this with a `repo: local` hook plus a second local hook running the guard's own unit tests — duplicated policy that no other .NET repo could pick up, which is exactly what this repo exists to fix. Only the static half moves here. The transitive half resolves each project's package graph with `dotnet list package --include-transitive`, which needs `dotnet restore` and the solution's private feed credentials — too slow and too credential-bound for a commit hook — so consumers keep that guard in their own CI beside the SDK install. Direct references are the ones a commit introduces, and they are now caught before it lands. Nothing is exempt by default: every permitted path is named via --allow-prefix, and the one file allowed to declare the package's central PackageVersion via --central-version-file. The exemption is scoped to that named file, so a PackageVersion element copied into an ordinary project file is still a violation and cannot be used to smuggle a reference in. Files come from `git ls-files` with pass_filenames: false, not from the staged set. The policy is a property of the whole repository, so a violation sitting in a file the commit never touched still fails; scoping to changed files would let a pre-existing reference stay invisible forever.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second half of the trading#3362 guard migration, stacked on #4. Consumer side is trading's
chore/newtonsoft-static-scan-to-shared-hook, which pinsrev: v0.4.0— so this needs merging and tagging v0.4.0 before that branch can even resolve its hooks.The split
The Newtonsoft policy has two halves with completely different costs:
dotnet restore+ private feed credsOnly the static half is portable, and only the static half encodes an org convention — "production code uses
System.Text.Json" is true everywhere, not just in trading. The graph half stays inscripts/ci/check_no_production_newtonsoft.pyas a CI job.no-production-newtonsoftCase-insensitive scan for the token, exempt only under the prefixes named by
--allow-prefixand on the single centralPackageVersionline named by--central-version-file.--tokenand--source-globare configurable, so this is not actually Newtonsoft-specific — it's a banned-dependency scanner with a default.It enumerates via
git ls-filesand setsalways_run: true/pass_filenames: falsedeliberately: a violation in a file the current commit did not touch still fails the tree. A staged-set-only scan would let a bad reference ride in on an unrelated commit and never be seen again.Shared helpers
_common.py(private, not a console script) now holds the two behaviors every hook has to get identically right:MISSING— distinguishes "file absent" from "file parsed toNone". A hook must treat an absent surface as a clean no-op but an empty one as a real, checkable document; collapsing both toNoneloses that distinction.load_yaml— a malformed document is fatal, not a skipped check. Silently passing on unparseable YAML is how a guard stops guarding without anyone noticing.emit— GHA annotations under Actions, plain prefixes in a terminal.stevedore_release_scopeis refactored onto them, which is why this PR touches it. Its public functions keep their signatures, so the existing tests cover the refactor unchanged.Verification
PackageVersionreference there, an untouched file still fails the whole-tree scan, and no--allow-prefixpermits nothing.pre-commit run --all-filesgreen.hook-installjob extended — the try-repo fixture now contains a tracked, clean production source, because with zero matching files the hook would pass without ever exercising the git enumeration path.