chore: repository health check (tests, security, deps, CI) - #297
Draft
shenxianpeng wants to merge 7 commits into
Draft
shenxianpeng wants to merge 7 commits into
shenxianpeng wants to merge 7 commits into
Conversation
Bring main.py to 100% line and branch coverage (95% before): the debug log of the inputs, the PR number read from the event payload, a missing PyGithub, token or repository, stale report clean-up, an unparsable scope in the report table, the footer without a version, and running the script itself. Also move `if __name__ == "__main__": unittest.main()` to the end of the file. It sat in the middle, so `python main_test.py` ran 158 of the tests, and two of those errored on helpers defined after it.
add_pr_comments built its PyGithub client without a base_url, so on GitHub Enterprise Server the comment went to api.github.com, which rejected the server's token, and the report was never posted. The commit listing already read GITHUB_API_URL; both clients now take it from one helper.
…ng on it The runner reads every line of a step's output for workflow commands. It trims indentation before looking for `::`, and it finds the older `##[` syntax anywhere in a line, so the tree's quoted commit bodies, PR titles and branch names could be taken as commands: a commit documenting `::add-mask::` or `::stop-commands::` changed the log it appeared in. A group whose lines could be read that way is now fenced with `::stop-commands::` and a random token; the CLI notices relayed to stderr get the same treatment. Ordinary output carries no fence and is unchanged.
The hidden marker alone decided which comments the action edits and deletes. A marker pasted into someone's own comment leaves it that person's comment, so marked comments now need a bot author too, as legacy ones already did: the action never rewrites or deletes a comment it did not post.
…missions - commit-check.yml: pin actions/checkout to its v7.0.1 SHA, like every other workflow, and give the workflow a read-only default. - checkout in commit-check, coverage, test and used-by no longer leaves the token in .git/config: nothing after it needs authenticated git, and create-pull-request sets up its own auth for the push. - release-drafter: grant the reusable workflow the two write scopes it needs rather than the repository default, as commit-check/commit-check does. - release and used-by: the write scopes move from the workflow to the one job that uses them.
actionlint: "branches" is not a filter for the release event. GitHub ignores it, so the workflow already ran for every published release; removing it changes nothing but the warning.
The example expanded `toJSON(...scopes)` directly inside `run:`. The scopes quote commit text as it is, so follow GitHub's guidance for such values: pass the JSON in through `env:` and read it with jq.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Commit Check✅ All 11 checks passed Show all 11 checkscommit-check 2.18.2 · Rules reference |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
===========================================
+ Coverage 95.29% 100.00% +4.70%
===========================================
Files 1 1
Lines 637 649 +12
===========================================
+ Hits 607 649 +42
+ Misses 30 0 -30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Summary
Repository health check:
main.pyto 100% test coverage, a security review ofmain.py,action.ymland the workflows, a dependency review, and CI warning cleanup. One commit per category. The threefix:commits change behavior and are listed separately below.Coverage
pytest --cov=main --cov-branch main_test.py, scopemain.py(the shipped code):No new exclusions. The existing
# pragma: no coveron the delimiter-collision loop in_write_outputis unchanged. The project has no coverage gate. Now that the suite is at 100%, adding--cov-branch --cov-fail-under=100tocoverage.ymlwould hold it there. I left that as a suggestion.Tests added
25 tests in
main_test.py(207 → 232). All run offline, with git, the CLI and the API mocked.log_env_vars, reading the PR number from the event payload (pull_request_target), a missing PyGithub, token or repository, cleanup of stale reports, an unparsable scope in the report table, the footer without a version, the all-skipped step-log verdict, and runningmain.pyas a script.if __name__ == "__main__": unittest.main()sat in the middle ofmain_test.py. As a result,python main_test.pyran only 158 tests, and two of those errored on helpers defined further down. The guard is now at the end, and all 232 tests also run under plain unittest.Fixes (behavior)
add_pr_commentsbuilt its PyGithub client withoutbase_url. On GHES the comment went to api.github.com, which rejected the token. The client now usesGITHUB_API_URL, as the commit listing already did. Nothing changes on github.com.::and finds##[anywhere in a line, so a commit body quoting a command (::add-mask::…,::stop-commands::…) was acted on. When a group contains such a line, its lines are now wrapped in::stop-commands::<random token>. Ordinary logs are byte-for-byte unchanged.Security hardening
actions/checkoutincommit-check.ymlis now pinned to its v7.0.1 SHA, like the other workflows.persist-credentials: falsewhere no later step needs authenticated git: commit-check, coverage, test and used-by. create-pull-request sets up its own auth.release-drafter.ymlnow grants exactlycontents: writeandpull-requests: write, as commit-check/commit-check does, instead of the repository default.release.yamlandused-by.yml, the write scopes moved from workflow level to job level.resultexample now passes the JSON throughenv:and reads it withjq. It used to expandtoJSON(...)insiderun:, and the scopes quote commit text.action.ymlpasses every input throughenv:, with no${{ }}insiderun:. It installs wheels only, offline, from the verified download.|escaped, and fences longer than any backtick run. Annotations are escaped too.GITHUB_OUTPUTuses a random heredoc delimiter.GITHUB_TOKENanywhere, and fetches parent configs over HTTPS only, with a timeout.@main, the release job's needed credentials, Dependabot cooldown).Dependencies
Everything is current. commit-check 2.18.2 and PyGithub 2.10.0 are the latest releases, every action pin matches its latest release tag, and Python 3.14 is the newest stable release (3.15 is at rc2). Bot PRs worth merging:
TestRealCommitCheckBinaryfailure that fix: run the unmocked commit-check test on the CLI's defaults, not the checkout's #286 fixed on main. It needs an update from main, not changes.CI warnings
release.yaml: removedbranches:under thereleaseevent (cea587a). actionlint flags it because it isn't a filter for that event, and GitHub ignored it.commit-check/.github:.github/release-drafter.yml(categories[*].labels,exclude-labels,version-resolver.major.labels).Needs a maintainer decision
gh attestation verifyinaction.ymlreadsGITHUB_TOKEN, which gh uses for github.com. On a GHES runner that variable holds the server's token, so verification would fail with "Bad credentials" beforemain.pyruns. The README lists GHES as a reason to pick the action. I left this unchanged because it needs a design choice, for example downloading the attestation bundle separately without auth.uv pip compile --universal --generate-hashes) would make that statement true.test.yml,coverage.ymland the README. Dropping it is a separate change.default-days: 0. I left both as they are because they look deliberate.How it was verified
pytest --cov=main --cov-branch main_test.pyon Python 3.10: 232 passed, 100% of lines and branches. The same suite with-W erroron Python 3.14: 232 passed.python main_test.py: 232 tests OK.pre-commit run --all-files: all hooks pass.actionlint: clean.zizmor: only the items above remain.main.pywith the real commit-check 2.18.2 on a scratch repo whose failing commit body quotes::add-mask::and##[warning]. The quoted tree is wrapped in the stop-commands block, while::group::/::endgroup::and the::errorannotation sit outside it.commit-checkwith this repository's config.