Expand unit test coverage across all scripts - #88
Conversation
Adds coverage tooling to CI (installs `coverage`, runs tests under `coverage run`, and appends a coverage report to the step summary). Expands unit tests for check_repo_consistency, format_json, gen_spec, package_plugin, release_transaction, and validate_manifest — covering edge cases, error paths, and main() entry points. Also adds `.coverage` to .gitignore via the existing pattern and ignores `/.claude/worktrees`.
Adds test methods covering main()/argparse paths and error branches for package_plugin.py (60% -> 98%), gen_spec.py (56% -> 99%), format_json.py (68% -> 97%), check_repo_consistency.py, release_transaction.py, and validate_manifest.py. Wires coverage.py into the repo-safety CI job (informational only, no fail-under threshold): runs tests via `coverage run`, reports via `coverage report -m` to the job summary.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @tonythethompson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
📝 WalkthroughWalkthroughThe PR adds manifest and repository consistency tests, strengthens packaging archive handling, expands release transaction coverage, and updates the repository safety workflow to collect and publish test coverage. ChangesRepository safety coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. Analyzed 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 |
PR Summary by QodoExpand script unit tests and add CI coverage reporting
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
![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: f31d374809
ℹ️ 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".
Greptile SummaryThe PR substantially expands unit coverage for repository-management scripts and adds informational coverage reporting to repository-safety CI. It also hardens plugin packaging by refusing to overwrite an existing archive.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| .github/workflows/repo-safety.yml | Installs pinned coverage tooling, runs the existing unit suite under coverage, and appends an informational report to the job summary. |
| scripts/package_plugin.py | Adds an explicit guard that prevents existing package archives from being overwritten. |
| scripts/test_package_plugin.py | Adds coverage for CLI failures and success paths, archive output records, overwrite protection, and Windows ZIP contents. |
| scripts/test_release_transaction.py | Expands coverage for claim validation, asset upload and finalization guards, cleanup ownership, and CLI output handling. |
| scripts/test_validate_manifest.py | Retains commit-snapshot contract tests and adds coverage for target selection, malformed manifests, tag resolution, and CLI success. |
| scripts/test_gen_spec_source.py | Adds coverage for malformed package records, missing assets, target mismatches, and specification-generation CLI paths. |
| scripts/test_check_repo_consistency.py | Adds coverage for malformed manifest data, README mismatches, backlog checks, and top-level CLI outcomes. |
| scripts/test_format_json.py | Adds coverage for primitive and nested formatting plus multi-file CLI return behavior. |
Reviews (8): Last reviewed commit: "Split active-list validation test into e..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/repo-safety.yml:
- Around line 28-30: Pin the coverage dependency in the “Install coverage”
workflow step by specifying the tested version, or reference an existing
constraints file that pins it. Keep the installation command otherwise
unchanged.
In `@scripts/test_package_plugin.py`:
- Around line 83-103: Update test_main_writes_tar_gz_and_prints_record to
capture stdout while invoking main, then assert the emitted package record
contains the expected package path and SHA-256 digest fields. Keep the existing
archive and return-code assertions, and verify the captured digest matches the
computed digest.
In `@scripts/test_release_transaction.py`:
- Around line 302-307: Update test_record_release_id_appends_line to initialize
the output file with an existing output line before calling record_release_id,
then assert the original content remains unchanged with release_id=42\n appended
after it.
- Around line 203-215: Update test_claim_rejects_release_with_wrong_tag_name and
the corresponding claim cleanup flow so a mismatched tag_name does not trigger
deletion of the unverified draft release ID 9. After ownership verification
fails, clean up only the created p-1 tag and assert a single DELETE command
while preserving the RuntimeError behavior.
In `@scripts/test_validate_manifest.py`:
- Around line 118-120: Extend the direct tests in test_validate_manifest.py
beyond test_selected_names_rejects_duplicates to cover selected_names or the
relevant manifest-validation entry points for malformed, blank,
self-referential, and unauthorized upstream_repo values, plus commit-snapshot
intake validation and verify_upstream() rejecting an invalid commit SHA. Assert
the expected validation failures at the validate_manifest.py boundary rather
than relying on downstream build_spec coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e664e723-2bd7-46cc-8bbc-661096c2ab23
📒 Files selected for processing (7)
.github/workflows/repo-safety.ymlscripts/test_check_repo_consistency.pyscripts/test_format_json.pyscripts/test_gen_spec_source.pyscripts/test_package_plugin.pyscripts/test_release_transaction.pyscripts/test_validate_manifest.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (6)
.github/workflows/**/*.{yml,yaml}
📄 CodeRabbit inference engine (REVIEW.md)
.github/workflows/**/*.{yml,yaml}: Workflow linting withactionlintmust pass, and workflows must not publish releases on pushes or pull requests.
Publishing is manual-only: releases requireworkflow_dispatchwith a non-emptyonly=package list; pull-request and push paths may validate but must not release.
Files:
.github/workflows/repo-safety.yml
**/*.{cs,py}
📄 CodeRabbit inference engine (Custom checks)
**/*.{cs,py}: Ensure managed host restarts are safe: modifications involvingManagedVenvHostManager,ContainerizedServiceProbe,ContainerizedInferenceClient, orContainerizedProviderReadinessmust not callStopTrackedHost,RestartAsync, or equivalent host-kill methods unless active request leases are zero. Hosts reportingbusy_reasonoractive_requests > 0must not be restarted, and readiness probing must not downgrade a known-available host merely because a background refresh is in flight.
MaintainSessionWorkflowStageordering asFoundation < MediaLoaded < Transcribed < Diarized < Translated < TtsGenerated. Comparisons must use enum members rather than raw integer literals. New or renumbered members require a legacy-compatible JSON converter for old numeric values, and reordered enums require checking every inequality comparison across the solution for preserved semantic meaning.
Files:
scripts/test_format_json.pyscripts/test_release_transaction.pyscripts/test_validate_manifest.pyscripts/test_gen_spec_source.pyscripts/test_check_repo_consistency.pyscripts/test_package_plugin.py
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Use Serena's semantic, symbol-aware tools as the primary tools for code work; use built-in Read, Glob, Grep, and Edit only when Serena cannot perform the task or an explicitly listed exception applies.
Use the appropriate Serena symbol tool for code operations: get_symbols_overview for structure, find_symbol for symbol bodies, find_referencing_symbols for callers, find_declaration or implementations for declarations, replace_symbol_body for edits, insertion tools for nearby additions, replace_content for pattern replacement, and rename, move, safe-delete, or inline tools for refactoring.
Before editing code, obtain a symbol overview of the target file, inspect the specific symbols with bodies included, and edit using Serena's symbol-aware tools.
Understand existing code before changing it and make the smallest change that satisfies the request; avoid unrelated cleanup, premature abstractions, speculative error handling, feature flags, and compatibility shims unless requested.
Prefer editing existing files over creating new ones, and never create Markdown or README files unless explicitly requested.
For exploratory questions, provide a recommendation and the main tradeoff in two or three sentences without implementing changes until the user agrees.
For UI or frontend changes that cannot be tested in a browser, explicitly state that browser testing was not performed rather than claiming success.
Identify and fix security issues when discovered, including injection, XSS, SQL injection, path traversal, and secret leaks.
Pause and obtain confirmation before destructive, hard-to-reverse, externally visible, or third-party-upload actions, including deleting files or branches, dropping tables, killing processes, force-pushing, changing CI/CD, posting externally, or uploading content.
When encountering an obstacle, investigate the root cause instead of bypassing it with --no-verify, --force, or deletion; investigate unfamiliar files, branches, and configuration befor...
Files:
scripts/test_format_json.pyscripts/test_release_transaction.pyscripts/test_validate_manifest.pyscripts/test_gen_spec_source.pyscripts/test_check_repo_consistency.pyscripts/test_package_plugin.py
**/*.{js,jsx,ts,tsx,py,java,go,rs,rb,php,c,cpp,h,hpp,cs,swift,kt,kts}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add comments unless the rationale is non-obvious, such as a hidden constraint, workaround, subtle invariant, or other necessary explanation; do not narrate behavior or reference the current task in comments.
Files:
scripts/test_format_json.pyscripts/test_release_transaction.pyscripts/test_validate_manifest.pyscripts/test_gen_spec_source.pyscripts/test_check_repo_consistency.pyscripts/test_package_plugin.py
scripts/**/*.py
📄 CodeRabbit inference engine (REVIEW.md)
scripts/**/*.py: The CI gates must pass:python3 -m compileall -q scriptsandpython3 -m unittest discover -s scripts -p "test_*.py" -v.
Packaging and release scripts must preserve draft ownership, refuse to overwrite existing release tags or assets, and fail closed when the complete asset set is unavailable.
Generated specifications must omit authoredsha256hashes; hashes are downloaded and generated bynuman-registryduring intake.
Files:
scripts/test_format_json.pyscripts/test_release_transaction.pyscripts/test_validate_manifest.pyscripts/test_gen_spec_source.pyscripts/test_check_repo_consistency.pyscripts/test_package_plugin.py
scripts/**/*.{py,json}
📄 CodeRabbit inference engine (REVIEW.md)
Packaging and release helpers must have tests covering failure modes, including incomplete asset sets, overwrite attempts, and release transaction errors.
Files:
scripts/test_format_json.pyscripts/test_release_transaction.pyscripts/test_validate_manifest.pyscripts/test_gen_spec_source.pyscripts/test_check_repo_consistency.pyscripts/test_package_plugin.py
🪛 ast-grep (0.45.0)
scripts/test_validate_manifest.py
[info] 195-195: use jsonify instead of json.dumps for JSON output
Context: json.dumps(self.manifest())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
scripts/test_gen_spec_source.py
[info] 367-367: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"active": [{"name": "other"}]})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 421-426: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"default_targets": ["x86_64-unknown-linux-gnu"],
"active": [self._manifest_entry()],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 462-467: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"default_targets": ["x86_64-unknown-linux-gnu"],
"active": [self._manifest_entry()],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
scripts/test_check_repo_consistency.py
[info] 128-128: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"active": "not-a-list"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 203-203: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"note": "Tracked in this PR", "plugins": []})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 222-222: use jsonify instead of json.dumps for JSON output
Context: json.dumps(backlog)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 GitHub Actions: Repository safety / 0_Validate manifest and packaging.txt
.github/workflows/repo-safety.yml
[error] 41-41: actionlint failed because ShellCheck reported SC2129: consider using '{ cmd1; cmd2; } >> file' instead of individual redirects. Command failed with exit code 1.
🪛 GitHub Actions: Repository safety / Validate manifest and packaging
.github/workflows/repo-safety.yml
[error] 41-41: ShellCheck SC2129: Consider using '{ cmd1; cmd2; } >> file' instead of individual redirects. The actionlint check failed with exit code 1.
🪛 Ruff (0.16.1)
scripts/test_format_json.py
[warning] 70-70: Boolean positional value in function call
(FBT003)
[warning] 77-77: Boolean positional value in function call
(FBT003)
scripts/test_release_transaction.py
[warning] 212-212: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 218-219: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
[warning] 219-219: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 225-225: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 232-232: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 239-239: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 246-246: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 275-275: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
scripts/test_validate_manifest.py
[warning] 119-119: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 127-127: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 133-133: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 139-139: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 145-145: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 149-149: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 154-154: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 160-160: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 164-164: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 176-181: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
[warning] 181-181: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 185-190: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
[warning] 190-190: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
scripts/test_gen_spec_source.py
[warning] 370-370: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
[warning] 377-377: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
[warning] 384-384: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
[warning] 391-391: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
[warning] 397-397: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 407-407: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 410-410: Missing return type annotation for private function _manifest_entry
(ANN202)
scripts/test_check_repo_consistency.py
[warning] 130-130: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 134-134: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
🔍 Remote MCP Context7, DeepWiki, GitHub Copilot
Review-relevant context
- PR
#88is open with 703 additions and 114 deletions. Its “Validate manifest and packaging” check failed, while Cline and roadmap-drift checks passed. - The base workflow already runs formatting, consistency, unit tests, manifest/upstream validation, and actionlint. This PR changes only test execution to
coverage runand installscoveragewithout a pinned version. - The safety modules introduced by PR
#3enforce immutable release tags, exact asset hashes, expected target sets, manifest validation, and draft-release ownership. The new tests directly exercise these invariants, especially release cleanup/finalization and packaged-asset validation. - PR
#80established the currentcommit-snapshotcontract: tags may be absent,source_commitis mandatory, and snapshot versions are derived consistently across packaging, release tags, and specs. This PR removes several tests covering those rules, as well asupstream_repovalidation tests; verify that this reduction is intentional. - Coverage.py documents
coverage run -m unittest discoverfollowed bycoverage report -m; the proposed commands match the documented usage. - DeepWiki could not retrieve architectural context because neither requested repository was indexed.
🔇 Additional comments (14)
scripts/test_package_plugin.py (4)
6-10: LGTM!
56-66: LGTM!
68-81: LGTM!
105-125: LGTM!scripts/test_release_transaction.py (5)
217-240: LGTM!
242-276: LGTM!
278-300: LGTM!
309-327: LGTM!
329-384: LGTM!.github/workflows/repo-safety.yml (1)
41-45: LGTM!scripts/test_check_repo_consistency.py (1)
10-10: LGTM!Also applies to: 126-291
scripts/test_validate_manifest.py (1)
122-197: LGTM!scripts/test_format_json.py (1)
5-12: LGTM!Also applies to: 69-134
scripts/test_gen_spec_source.py (1)
8-13: LGTM!Also applies to: 364-493
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Qodo FixerNo findings are within the configured fix scope. To change which findings are fixed, adjust the setting on your Qodo configuration page. |
test_validate_manifest.py and test_check_repo_consistency.py lost coverage for still-live production logic when their test files were rewritten: validate_upstream_repo's ADR 0001 fork-identity checks, commit-snapshot intake_mode handling (null/absent tag, unknown mode, non-string mode, verify_upstream's commit-snapshot branch), source_commit null/non-string rejection, and expected_readme_lines' missing-tag fallback. None of these were covered by the newly-added tests. Restored verbatim from the pre-rewrite versions; all pass alongside the new tests (130 total, up from 111).
…erage/scripts-tests
The merge-base changed after approval.
- Pin coverage==7.15.4 in the CI "Install coverage" step (was unpinned). - test_main_writes_tar_gz_and_prints_record now captures stdout and asserts the printed PACKAGED record contains the expected filename and matches the computed sha256 digest, instead of only asserting the digest string is truthy. - test_record_release_id_appends_line now seeds the output file with existing content and asserts it's preserved with the new line appended, instead of only exercising the write-to-empty-file case (which can't distinguish append from overwrite). Skipped as not valid: - The claim() cleanup-flow suggestion (don't delete the draft release on tag_name mismatch) would be a regression: `release` in claim() is the direct response to our own POST-create call, so `release['id']` is unambiguously the release we just created regardless of what tag_name it echoes back. Skipping the delete would leak an orphaned draft release on this failure path. The existing test already correctly asserts the real (correct) two-DELETE behavior. - The test_validate_manifest.py coverage-gap finding was stale: it was generated against an earlier revision of this PR, before a prior review pass already restored the 19 tests it's asking for (malformed/ blank/self-referential/unauthorized upstream_repo, commit-snapshot intake validation, verify_upstream() rejecting invalid commits).
Three separate >> "$GITHUB_STEP_SUMMARY" redirects in the "Run unit
tests" step tripped shellcheck's SC2129 (prefer a single grouped
redirect), which failed the "Validate GitHub Actions workflows"
actionlint step in CI. Grouped them into a single { ...; } >> file
block per shellcheck's own suggested fix.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
scripts/test_package_plugin.py (2)
110-129: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify the ZIP member, not only the archive path.
The Windows test passes when
main()creates any ZIP at the expected path. It does not prove that the archive containsplugin.exewith the expected bytes. Inspect the ZIP and assert its member name and content.[details]
[summary]Suggested assertion[/summary]+with zipfile.ZipFile(output) as archive: + self.assertEqual(archive.namelist(), ["plugin.exe"]) + self.assertEqual(archive.read("plugin.exe"), b"binary")[/details]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test_package_plugin.py` around lines 110 - 129, Extend test_main_writes_zip_for_windows_target to open the generated archive with zipfile.ZipFile and assert it contains plugin.exe with the expected binary bytes b"binary", while retaining the existing return-code and archive-path assertions.
85-108: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd a fail-closed overwrite test.
The new packaging coverage checks a missing binary and successful archive creation, but it does not cover an existing output path.
scripts/package_plugin.py:45-69opens tar and ZIP outputs in write modes, andscripts/package_plugin.py:72-100has no output-exists check. A rerun can replace bytes at the same package path. Pre-create the expected output, requiremain()to fail, and verify that the original bytes remain unchanged. Add the guard in the packaging helper if the test exposes the current overwrite.As per coding guidelines, packaging and release helpers must have tests covering failure modes, including overwrite attempts, and release assets must be immutable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test_package_plugin.py` around lines 85 - 108, The packaging flow in main must fail closed when the expected archive output already exists. Add an output-exists guard in the packaging helper before opening tar or ZIP outputs, make main return failure without replacing the file, and extend test_main_writes_tar_gz_and_prints_record with a pre-created output whose original bytes are verified unchanged after the failed call.Source: Coding guidelines
scripts/test_release_transaction.py (2)
235-240: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject remote asset replacement.
The upload failure test covers only a non-zero command result.
scripts/release_transaction.py:139-184invokesgh release uploadwith--clobber, so an existing release asset can be replaced while this suite passes. Remove--clobberor add an explicit existence check that fails closed. Add a test for an overwrite attempt.As per coding guidelines, release assets must be immutable, and packaging and release helpers must test overwrite attempts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test_release_transaction.py` around lines 235 - 240, Update upload_assets and its gh release upload invocation to prevent replacing existing release assets by removing --clobber or enforcing a fail-closed existence check. Extend test_upload_assets_raises_when_upload_fails with coverage for an overwrite attempt, asserting the operation fails and the existing asset is not replaced.Source: Coding guidelines
278-300: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert exact cleanup targets.
The ownership-change test checks only command count. Assert that it issues no
DELETE. The owned-cleanup test checks only theDELETEverb. Assert the exact paths:releases/7andgit/refs/tags/p-1. Otherwise, a wrong release or tag can pass the test.As per coding guidelines, packaging and release scripts must preserve draft ownership.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test_release_transaction.py` around lines 278 - 300, Strengthen the assertions in test_cleanup_refuses_when_tag_ownership_changed to verify no issued command contains DELETE. In test_cleanup_deletes_owned_draft_and_tag, assert the final two commands target exactly releases/7 and git/refs/tags/p-1, while preserving the existing command-count check.Source: Coding guidelines
♻️ Duplicate comments (1)
scripts/test_release_transaction.py (1)
203-216: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not require deletion of the unverified release.
The fake response has release ID
9buttag_name="different".claim()cannot prove that this draft belongs to the current transaction. Requiring twoDELETEcalls can remove an unrelated draft release. Delete only the tag created by this workflow and assert one tag deletion.As per coding guidelines, packaging and release scripts must preserve draft ownership.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test_release_transaction.py` around lines 203 - 216, Update test_claim_rejects_release_with_wrong_tag_name and the corresponding claim cleanup behavior so an unverified draft release is not deleted. Retain deletion of only the tag created by this workflow, and change the test to assert exactly one DELETE command while preserving the RuntimeError assertion.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/test_package_plugin.py`:
- Around line 110-129: Extend test_main_writes_zip_for_windows_target to open
the generated archive with zipfile.ZipFile and assert it contains plugin.exe
with the expected binary bytes b"binary", while retaining the existing
return-code and archive-path assertions.
- Around line 85-108: The packaging flow in main must fail closed when the
expected archive output already exists. Add an output-exists guard in the
packaging helper before opening tar or ZIP outputs, make main return failure
without replacing the file, and extend test_main_writes_tar_gz_and_prints_record
with a pre-created output whose original bytes are verified unchanged after the
failed call.
In `@scripts/test_release_transaction.py`:
- Around line 235-240: Update upload_assets and its gh release upload invocation
to prevent replacing existing release assets by removing --clobber or enforcing
a fail-closed existence check. Extend
test_upload_assets_raises_when_upload_fails with coverage for an overwrite
attempt, asserting the operation fails and the existing asset is not replaced.
- Around line 278-300: Strengthen the assertions in
test_cleanup_refuses_when_tag_ownership_changed to verify no issued command
contains DELETE. In test_cleanup_deletes_owned_draft_and_tag, assert the final
two commands target exactly releases/7 and git/refs/tags/p-1, while preserving
the existing command-count check.
---
Duplicate comments:
In `@scripts/test_release_transaction.py`:
- Around line 203-216: Update test_claim_rejects_release_with_wrong_tag_name and
the corresponding claim cleanup behavior so an unverified draft release is not
deleted. Retain deletion of only the tag created by this workflow, and change
the test to assert exactly one DELETE command while preserving the RuntimeError
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9deced28-ad26-435c-b57c-94abb3b84b59
📒 Files selected for processing (3)
.github/workflows/repo-safety.ymlscripts/test_package_plugin.pyscripts/test_release_transaction.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (6)
.github/workflows/**/*.{yml,yaml}
📄 CodeRabbit inference engine (REVIEW.md)
.github/workflows/**/*.{yml,yaml}: Workflow linting withactionlintmust pass, and workflows must not publish releases on pushes or pull requests.
Publishing is manual-only: releases requireworkflow_dispatchwith a non-emptyonly=package list; pull-request and push paths may validate but must not release.
Files:
.github/workflows/repo-safety.yml
**/*.{cs,py}
📄 CodeRabbit inference engine (Custom checks)
**/*.{cs,py}: Ensure managed host restarts are safe: modifications involvingManagedVenvHostManager,ContainerizedServiceProbe,ContainerizedInferenceClient, orContainerizedProviderReadinessmust not callStopTrackedHost,RestartAsync, or equivalent host-kill methods unless active request leases are zero. Hosts reportingbusy_reasonoractive_requests > 0must not be restarted, and readiness probing must not downgrade a known-available host merely because a background refresh is in flight.
MaintainSessionWorkflowStageordering asFoundation < MediaLoaded < Transcribed < Diarized < Translated < TtsGenerated. Comparisons must use enum members rather than raw integer literals. New or renumbered members require a legacy-compatible JSON converter for old numeric values, and reordered enums require checking every inequality comparison across the solution for preserved semantic meaning.
Files:
scripts/test_release_transaction.pyscripts/test_package_plugin.py
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Use Serena's semantic, symbol-aware tools as the primary tools for code work; use built-in Read, Glob, Grep, and Edit only when Serena cannot perform the task or an explicitly listed exception applies.
Use the appropriate Serena symbol tool for code operations: get_symbols_overview for structure, find_symbol for symbol bodies, find_referencing_symbols for callers, find_declaration or implementations for declarations, replace_symbol_body for edits, insertion tools for nearby additions, replace_content for pattern replacement, and rename, move, safe-delete, or inline tools for refactoring.
Before editing code, obtain a symbol overview of the target file, inspect the specific symbols with bodies included, and edit using Serena's symbol-aware tools.
Understand existing code before changing it and make the smallest change that satisfies the request; avoid unrelated cleanup, premature abstractions, speculative error handling, feature flags, and compatibility shims unless requested.
Prefer editing existing files over creating new ones, and never create Markdown or README files unless explicitly requested.
For exploratory questions, provide a recommendation and the main tradeoff in two or three sentences without implementing changes until the user agrees.
For UI or frontend changes that cannot be tested in a browser, explicitly state that browser testing was not performed rather than claiming success.
Identify and fix security issues when discovered, including injection, XSS, SQL injection, path traversal, and secret leaks.
Pause and obtain confirmation before destructive, hard-to-reverse, externally visible, or third-party-upload actions, including deleting files or branches, dropping tables, killing processes, force-pushing, changing CI/CD, posting externally, or uploading content.
When encountering an obstacle, investigate the root cause instead of bypassing it with --no-verify, --force, or deletion; investigate unfamiliar files, branches, and configuration befor...
Files:
scripts/test_release_transaction.pyscripts/test_package_plugin.py
**/*.{js,jsx,ts,tsx,py,java,go,rs,rb,php,c,cpp,h,hpp,cs,swift,kt,kts}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add comments unless the rationale is non-obvious, such as a hidden constraint, workaround, subtle invariant, or other necessary explanation; do not narrate behavior or reference the current task in comments.
Files:
scripts/test_release_transaction.pyscripts/test_package_plugin.py
scripts/**/*.py
📄 CodeRabbit inference engine (REVIEW.md)
scripts/**/*.py: The CI gates must pass:python3 -m compileall -q scriptsandpython3 -m unittest discover -s scripts -p "test_*.py" -v.
Packaging and release scripts must preserve draft ownership, refuse to overwrite existing release tags or assets, and fail closed when the complete asset set is unavailable.
Generated specifications must omit authoredsha256hashes; hashes are downloaded and generated bynuman-registryduring intake.
Files:
scripts/test_release_transaction.pyscripts/test_package_plugin.py
scripts/**/*.{py,json}
📄 CodeRabbit inference engine (REVIEW.md)
Packaging and release helpers must have tests covering failure modes, including incomplete asset sets, overwrite attempts, and release transaction errors.
Files:
scripts/test_release_transaction.pyscripts/test_package_plugin.py
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
-
DeepWiki could not access
mta1124-1629472/Babel-Player; no architectural context was available. The target PR also returned404via GitHub tools. -
Related PR
#3establishes the relevant safety contract: manifest validation verifies immutable source commits, generated specs verify exact target coverage and local asset hashes, and release finalization checks draft ownership, tag commit, and exact asset digests before publication. -
Related PR
#80adds commit-snapshot behavior: nullable/absent tags are valid only forintake_mode: commit-snapshot; tagged entries still require non-empty tags; snapshot versions use0.0.0-snapshot.<YYYYMMDD>.<7-char-sha>; upstream verification checks that the SHA exists and is a commit object. -
Related PR
#82defines theupstream_repoinvariants covered by the new tests: onlynuman-maintainedentries may specify it, such entries must specify it, it must beowner/name, and it must not self-reference. Generated specs expose it assource.upstream.
🔇 Additional comments (3)
scripts/test_package_plugin.py (1)
5-7: LGTM!Also applies to: 58-83
scripts/test_release_transaction.py (1)
217-234: LGTM!Also applies to: 242-276, 302-309, 311-386
.github/workflows/repo-safety.yml (1)
28-30: LGTM!Also applies to: 41-47
- package_plugin.py: main() now fails closed if the target archive already exists, instead of silently overwriting it. - test_package_plugin.py: cover the new guard, and verify the Windows zip archive's actual contents (entry name + bytes) rather than just its existence. - test_release_transaction.py: assert cleanup's DELETE commands target the exact release/tag, and that a refused cleanup never issues DELETE.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/test_release_transaction.py (1)
314-332: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winProve that the missing-output path has no side effects.
The test checks only
rc == 1. It can pass even ifmain()callsclaim()and fails later. Patchclaim()and assertclaim.assert_not_called()whenGITHUB_OUTPUTis absent. This proves that the guard runs before creating the tag and draft release.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test_release_transaction.py` around lines 314 - 332, Update test_main_claim_requires_github_output to mock the module’s claim function while GITHUB_OUTPUT is absent, then assert the mock was not called alongside the existing rc == 1 assertion. Ensure the test verifies main() returns before attempting tag or draft-release creation.
♻️ Duplicate comments (1)
scripts/test_release_transaction.py (1)
203-215: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not require deletion of an unverified draft release.
When the API returns
tag_name == "different", release ID9is not owned by this transaction. The test currently requires deletion of both release9and tagp-1. Assert cleanup of only the transaction-created tag. Preserve the mismatched draft release.As per coding guidelines, packaging and release scripts must preserve draft ownership.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test_release_transaction.py` around lines 203 - 215, Update test_claim_rejects_release_with_wrong_tag_name so cleanup asserts deletion only of the transaction-created tag, while preserving the mismatched draft release with ID 9. Remove the assertion requiring deletion of the unverified release and retain validation that tag p-1 is deleted.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/package_plugin.py`:
- Around line 91-94: Replace the non-atomic out.exists() check in the packaging
flow with exclusive output reservation or a temporary-file build followed by
atomic no-replace publication, ensuring build_tar_gz() and build_zip() cannot
overwrite an existing archive during concurrent invocations. Preserve the
refusal behavior for existing destinations and add a regression test covering
concurrent packaging attempts.
---
Outside diff comments:
In `@scripts/test_release_transaction.py`:
- Around line 314-332: Update test_main_claim_requires_github_output to mock the
module’s claim function while GITHUB_OUTPUT is absent, then assert the mock was
not called alongside the existing rc == 1 assertion. Ensure the test verifies
main() returns before attempting tag or draft-release creation.
---
Duplicate comments:
In `@scripts/test_release_transaction.py`:
- Around line 203-215: Update test_claim_rejects_release_with_wrong_tag_name so
cleanup asserts deletion only of the transaction-created tag, while preserving
the mismatched draft release with ID 9. Remove the assertion requiring deletion
of the unverified release and retain validation that tag p-1 is deleted.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c2d22a41-7619-43be-a277-d76970640c16
📒 Files selected for processing (3)
scripts/package_plugin.pyscripts/test_package_plugin.pyscripts/test_release_transaction.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{cs,py}
📄 CodeRabbit inference engine (Custom checks)
**/*.{cs,py}: Ensure managed host restarts are safe: modifications involvingManagedVenvHostManager,ContainerizedServiceProbe,ContainerizedInferenceClient, orContainerizedProviderReadinessmust not callStopTrackedHost,RestartAsync, or equivalent host-kill methods unless active request leases are zero. Hosts reportingbusy_reasonoractive_requests > 0must not be restarted, and readiness probing must not downgrade a known-available host merely because a background refresh is in flight.
MaintainSessionWorkflowStageordering asFoundation < MediaLoaded < Transcribed < Diarized < Translated < TtsGenerated. Comparisons must use enum members rather than raw integer literals. New or renumbered members require a legacy-compatible JSON converter for old numeric values, and reordered enums require checking every inequality comparison across the solution for preserved semantic meaning.
Files:
scripts/package_plugin.pyscripts/test_release_transaction.pyscripts/test_package_plugin.py
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Use Serena's semantic, symbol-aware tools as the primary tools for code work; use built-in Read, Glob, Grep, and Edit only when Serena cannot perform the task or an explicitly listed exception applies.
Use the appropriate Serena symbol tool for code operations: get_symbols_overview for structure, find_symbol for symbol bodies, find_referencing_symbols for callers, find_declaration or implementations for declarations, replace_symbol_body for edits, insertion tools for nearby additions, replace_content for pattern replacement, and rename, move, safe-delete, or inline tools for refactoring.
Before editing code, obtain a symbol overview of the target file, inspect the specific symbols with bodies included, and edit using Serena's symbol-aware tools.
Understand existing code before changing it and make the smallest change that satisfies the request; avoid unrelated cleanup, premature abstractions, speculative error handling, feature flags, and compatibility shims unless requested.
Prefer editing existing files over creating new ones, and never create Markdown or README files unless explicitly requested.
For exploratory questions, provide a recommendation and the main tradeoff in two or three sentences without implementing changes until the user agrees.
For UI or frontend changes that cannot be tested in a browser, explicitly state that browser testing was not performed rather than claiming success.
Identify and fix security issues when discovered, including injection, XSS, SQL injection, path traversal, and secret leaks.
Pause and obtain confirmation before destructive, hard-to-reverse, externally visible, or third-party-upload actions, including deleting files or branches, dropping tables, killing processes, force-pushing, changing CI/CD, posting externally, or uploading content.
When encountering an obstacle, investigate the root cause instead of bypassing it with --no-verify, --force, or deletion; investigate unfamiliar files, branches, and configuration befor...
Files:
scripts/package_plugin.pyscripts/test_release_transaction.pyscripts/test_package_plugin.py
**/*.{js,jsx,ts,tsx,py,java,go,rs,rb,php,c,cpp,h,hpp,cs,swift,kt,kts}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add comments unless the rationale is non-obvious, such as a hidden constraint, workaround, subtle invariant, or other necessary explanation; do not narrate behavior or reference the current task in comments.
Files:
scripts/package_plugin.pyscripts/test_release_transaction.pyscripts/test_package_plugin.py
scripts/**/*.py
📄 CodeRabbit inference engine (REVIEW.md)
scripts/**/*.py: The CI gates must pass:python3 -m compileall -q scriptsandpython3 -m unittest discover -s scripts -p "test_*.py" -v.
Packaging and release scripts must preserve draft ownership, refuse to overwrite existing release tags or assets, and fail closed when the complete asset set is unavailable.
Generated specifications must omit authoredsha256hashes; hashes are downloaded and generated bynuman-registryduring intake.
Files:
scripts/package_plugin.pyscripts/test_release_transaction.pyscripts/test_package_plugin.py
scripts/**/*.{py,json}
📄 CodeRabbit inference engine (REVIEW.md)
Packaging and release helpers must have tests covering failure modes, including incomplete asset sets, overwrite attempts, and release transaction errors.
Files:
scripts/package_plugin.pyscripts/test_release_transaction.pyscripts/test_package_plugin.py
🔍 Remote MCP GitHub Copilot
Relevant review context
- Related PR
#3established the release-safety contract: immutable upstream commit verification, exact target coverage, local asset hash validation, run-owned draft releases, and cleanup ownership checks. - Related PR
#80introduced commit-snapshot semantics:intake_mode: commit-snapshotmay omittag.- Snapshot versions use
0.0.0-snapshot.<YYYYMMDD>.<7-char-sha>. - Snapshot commits must be fetchable and resolve to Git commit objects.
- Generated specs include
provenance: "commit-snapshot".
- Related PR
#82definedupstream_repoinvariants:- Allowed only for
owner: numan-maintained. - Required for such entries.
- Must match
owner/nameformat and differ fromrepo. - Generated specs expose it as
source.upstream.
- Allowed only for
- The referenced DeepWiki repository was not applicable to this Python-only PR; no C#/Python boundary, workflow-stage, or managed-host changes are involved.
- GitHub code search was rate-limited, so no additional repository-wide caller information was retrieved.
🔇 Additional comments (6)
scripts/test_release_transaction.py (5)
217-240: LGTM!
242-276: LGTM!
278-303: LGTM!
305-312: LGTM!
334-388: LGTM!scripts/test_package_plugin.py (1)
5-13: LGTM!Also applies to: 59-70, 71-84, 86-110, 111-131, 133-155
Confirms the fail-fast check in main() short-circuits before any tag/draft-release creation, not just that it returns rc=1.
There was a problem hiding this comment.
Pull request overview
This PR expands unit test coverage for the repository’s Python maintenance/packaging scripts and wires coverage.py into the existing repo-safety.yml validation job to publish an informational coverage report in the workflow summary.
Changes:
- Add/extend unit tests to cover
main()/argparse flows and additional error branches across multiple scripts. - Harden packaging behavior by refusing to overwrite an existing output archive in
scripts/package_plugin.py. - Update CI validation to run tests under
coverageand appendcoverage report -mto$GITHUB_STEP_SUMMARY; ignore.coverageartifacts.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/test_validate_manifest.py | Adds tests for selected_names, target selection/exclusion validation, tag resolution errors, and main() success path. |
| scripts/test_release_transaction.py | Adds tests for claim/upload/finalize/cleanup error paths and CLI subcommands. |
| scripts/test_package_plugin.py | Adds tests for Windows vs non-Windows packaging behavior and main() exit paths (including overwrite refusal). |
| scripts/test_gen_spec_source.py | Adds tests for manifest/packaged parsing failures and main() success/failure behavior. |
| scripts/test_format_json.py | Adds tests for primitive formatting and main() exit codes across common CLI scenarios. |
| scripts/test_check_repo_consistency.py | Adds tests for README/manifest mismatch reporting and additional error branches in consistency checks. |
| scripts/package_plugin.py | Refuses to overwrite existing output archives to avoid accidental replacement of artifacts. |
| .gitignore | Ignores coverage artifacts and local Claude worktree directory. |
| .github/workflows/repo-safety.yml | Installs pinned coverage, runs unit tests via coverage run, and appends a coverage report to the job summary. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
test_validate_manifest_rejects_non_list_active exercised active: [] (the empty-list branch), not a non-list value. Renamed it and added a dedicated non-list case so both branches of the shared check are covered.
Summary
main()/argparse paths and error branches forpackage_plugin.py(60% -> 98%),gen_spec.py(56% -> 99%),format_json.py(68% -> 97%)check_repo_consistency.py,release_transaction.py,validate_manifest.pytest coveragecoverage.pyinto therepo-safety.ymlvalidatejob — informational only, reports to the job summary, no--fail-undergateTest plan
python3 -m unittest discover -s scripts -p "test_*.py" -v— 111 tests, all greenpython3 -m compileall -q scripts— cleanSummary by cubic
Expands unit test coverage across all scripts and prevents
package_plugin.pyfrom overwriting an existing archive. CI now runs tests under pinnedcoverageand appends a coverage report to the job summary.Tests
package_plugin.py,gen_spec.py,format_json.py,check_repo_consistency.py,release_transaction.py, andvalidate_manifest.py.GITHUB_OUTPUTis unset; manifest target selection and exclusions; Windows zip contents and deterministic archives.Operational notes
coverage==7.15.4, runs tests withcoverage run, and writescoverage report -mto$GITHUB_STEP_SUMMARY(informational only). Adds.coverageand/.claude/worktreesto.gitignore.Written for commit 895bee1. Summary will update on new commits.