Chore/public release sanitization - #3
Conversation
📝 WalkthroughWalkthroughThe README License section changes the trademark statement to end with two periods instead of one. ChangesDocumentation correction
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/release.yml:
- Around line 50-54: The release-state comments in the workflow are too broad
and inaccurate. Update the description near resolve-release to state that
canonical repository derivation occurs only when Release Please is skipped while
release automation remains enabled, and clarify that an existing immutable tag
prevents creating a new release rather than producing no output; preserve the
downstream normalized-output explanation.
In `@scripts/release_checks.py`:
- Around line 78-81: Update the approved-repository lookahead in
_UNAPPROVED_PICOGRid_REPOSITORY to accept “.git” immediately after both
ecn-sdk-python and legion-system-auth, while preserving the existing delimiters
and rejection behavior; add allow-case tests covering both approved .git URLs.
In `@tests/release/test_release_checks.py`:
- Around line 1083-1090: Update
test_content_scan_allows_the_public_documentation_package_name to invoke
_scan_content for the package-lock.json content, ensuring the allow-case test
exercises _NONPUBLIC_SDK_IMPORT and still accepts picogrid-ecn-sdk-docs.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a16f1bc5-8121-4e40-901d-24199c7c8430
📒 Files selected for processing (30)
.github/CODEOWNERS.github/workflows/release.yml.github/workflows/scorecard.yml.gitignoreCONTRIBUTING.mdMANIFEST.inMakefileNOTICE.mddocs/README.mddocs/cspell.jsondocs/reference/evidence-status.mddocs/reference/original-ecn-integration-parity.mddocs/site/check-built-site.mjsdocs/site/public-routes.mjsdocs/src/content.config.tsdocs/src/styles/picogrid.csspyproject.tomlscripts/installed_examples.pyscripts/installed_wheel_probe.pyscripts/original-guide-inventory.jsonscripts/release-policy.jsonscripts/release_checks.pyscripts/verify_release.pyscripts/verify_types.pytests/contract/test_synthetic_fixtures.pytests/differential/test_public_boundary.pytests/examples/conftest.pytests/examples/test_examples.pytests/release/test_release_checks.pytests/release/test_version_sync.py
💤 Files with no reviewable changes (12)
- pyproject.toml
- scripts/original-guide-inventory.json
- .gitignore
- docs/reference/evidence-status.md
- docs/cspell.json
- scripts/installed_wheel_probe.py
- docs/reference/original-ecn-integration-parity.md
- docs/src/content.config.ts
- docs/site/public-routes.mjs
- scripts/release-policy.json
- tests/differential/test_public_boundary.py
- MANIFEST.in
| # Every downstream job consumes this job's normalized outputs rather than binding | ||
| # directly to one release-state provider. When Release Please is disabled, the | ||
| # canonical repository derives the same identity from its version and immutable tag. | ||
| # A merge that does not change the version resolves to an existing tag and stops | ||
| # without output. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Narrow the release-state description.
resolve-release also requires vars.RELEASE_AUTOMATION_ENABLED == 'true' (Line 59). If that variable is false, the canonical repository does not derive a release identity in this workflow. The existing published-release path also emits release_created=false (Line 154), so it does not stop without output. State that derivation applies when Release Please is skipped in the canonical repository while automation remains enabled, and that an existing tag stops creation of a new 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 @.github/workflows/release.yml around lines 50 - 54, The release-state
comments in the workflow are too broad and inaccurate. Update the description
near resolve-release to state that canonical repository derivation occurs only
when Release Please is skipped while release automation remains enabled, and
clarify that an existing immutable tag prevents creating a new release rather
than producing no output; preserve the downstream normalized-output explanation.
| _NONPUBLIC_SDK_IMPORT = re.compile(rb"picogrid[_-][a-z0-9_-]+[_-]sdk(?![a-z0-9_-])", re.IGNORECASE) | ||
| _UNAPPROVED_PICOGRID_REPOSITORY = re.compile( | ||
| rb"github\.com/picogrid/(?!ecn-sdk-python(?:[/?#\s\"']|$)|legion-system-auth(?:[/?#\s\"']|$))[a-z0-9_.-]+", | ||
| re.IGNORECASE, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Allow .git URLs for approved repositories.
Line 80 rejects github.com/picogrid/ecn-sdk-python.git and github.com/picogrid/legion-system-auth.git. The approved-name lookahead requires /, ?, #, whitespace, a quote, or end of input after the repository name. A normal Git clone URL uses .git instead.
Proposed fix
- rb"github\.com/picogrid/(?!ecn-sdk-python(?:[/?#\s\"']|$)|legion-system-auth(?:[/?#\s\"']|$))[a-z0-9_.-]+",
+ rb"github\.com/picogrid/(?!ecn-sdk-python(?:\.git)?(?:[/?#\s\"']|$)|legion-system-auth(?:\.git)?(?:[/?#\s\"']|$))[a-z0-9_.-]+",Add allow-case tests for both approved .git URLs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _NONPUBLIC_SDK_IMPORT = re.compile(rb"picogrid[_-][a-z0-9_-]+[_-]sdk(?![a-z0-9_-])", re.IGNORECASE) | |
| _UNAPPROVED_PICOGRID_REPOSITORY = re.compile( | |
| rb"github\.com/picogrid/(?!ecn-sdk-python(?:[/?#\s\"']|$)|legion-system-auth(?:[/?#\s\"']|$))[a-z0-9_.-]+", | |
| re.IGNORECASE, | |
| _NONPUBLIC_SDK_IMPORT = re.compile(rb"picogrid[_-][a-z0-9_-]+[_-]sdk(?![a-z0-9_-])", re.IGNORECASE) | |
| _UNAPPROVED_PICOGRID_REPOSITORY = re.compile( | |
| rb"github\.com/picogrid/(?!ecn-sdk-python(?:\.git)?(?:[/?#\s\"']|$)|legion-system-auth(?:\.git)?(?:[/?#\s\"']|$))[a-z0-9_.-]+", | |
| re.IGNORECASE, |
🤖 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/release_checks.py` around lines 78 - 81, Update the
approved-repository lookahead in _UNAPPROVED_PICOGRid_REPOSITORY to accept
“.git” immediately after both ecn-sdk-python and legion-system-auth, while
preserving the existing delimiters and rejection behavior; add allow-case tests
covering both approved .git URLs.
| def test_content_scan_allows_the_public_documentation_package_name() -> None: | ||
| policy = load_policy(POLICY_PATH) | ||
|
|
||
| scan_secret_and_address_content( | ||
| "package-lock.json", | ||
| b'{"name":"picogrid-ecn-sdk-docs"}', | ||
| policy, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Call _scan_content in this allow-case test.
scan_secret_and_address_content does not evaluate _NONPUBLIC_SDK_IMPORT. This test passes even if the new SDK scanner rejects picogrid-ecn-sdk-docs.
Proposed fix
- scan_secret_and_address_content(
+ _scan_content(
"package-lock.json",
b'{"name":"picogrid-ecn-sdk-docs"}',
policy,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_content_scan_allows_the_public_documentation_package_name() -> None: | |
| policy = load_policy(POLICY_PATH) | |
| scan_secret_and_address_content( | |
| "package-lock.json", | |
| b'{"name":"picogrid-ecn-sdk-docs"}', | |
| policy, | |
| ) | |
| def test_content_scan_allows_the_public_documentation_package_name() -> None: | |
| policy = load_policy(POLICY_PATH) | |
| _scan_content( | |
| "package-lock.json", | |
| b'{"name":"picogrid-ecn-sdk-docs"}', | |
| policy, | |
| ) |
🤖 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 `@tests/release/test_release_checks.py` around lines 1083 - 1090, Update
test_content_scan_allows_the_public_documentation_package_name to invoke
_scan_content for the package-lock.json content, ensuring the allow-case test
exercises _NONPUBLIC_SDK_IMPORT and still accepts picogrid-ecn-sdk-docs.
721903a to
8aff4f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@README.md`:
- Line 204: Update the README sentence stating that Picogrid names and marks are
not licensed as trademarks by removing the extra trailing period, leaving
exactly one period.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3126b07a-9eb7-4a83-8346-10942128412e
📒 Files selected for processing (1)
README.md
| The Picogrid ECN SDK is licensed under the | ||
| [Mozilla Public License 2.0](https://github.com/picogrid/ecn-sdk-python/blob/main/LICENSE). | ||
| Picogrid names and marks are not licensed as trademarks. | ||
| Picogrid names and marks are not licensed as trademarks.. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the extra period.
Line 204 ends with two periods (trademarks..). Use one period.
Proposed fix
-Picogrid names and marks are not licensed as trademarks..
+Picogrid names and marks are not licensed as trademarks.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Picogrid names and marks are not licensed as trademarks.. | |
| Picogrid names and marks are not licensed as trademarks. |
🤖 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 `@README.md` at line 204, Update the README sentence stating that Picogrid
names and marks are not licensed as trademarks by removing the extra trailing
period, leaving exactly one period.
1 participant
Sanitation suggestions before public release
Summary by CodeRabbit