Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a full publish-and-verify flow for OpenVerifiableLLM: new verifier and publish helpers, an ChangesVerified Model Publish Pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ovllm
participant src/publish.py
participant src/verifier.py
participant model_signing
participant huggingface_hub
User->>ovllm: prepare-publish / sign / publish-hf / verify
ovllm->>src/publish.py: prepare_publish_dir() / sign_model_dir()
src/publish.py->>model_signing: sign sigstore
ovllm->>src/publish.py: publish_huggingface()
src/publish.py->>huggingface_hub: upload_folder()
ovllm->>src/verifier.py: verify_model_reference()
src/verifier.py-->>ovllm: CheckResult list + verdict
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
README.md (1)
260-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWindows path in an otherwise bash-oriented doc.
C:\tmp\ovllm-smokemixes a Windows-only absolute path into a section using bash-style commands throughout the rest of the README, which can confuse Linux/macOS readers following along.✏️ Suggested tweak
```bash -ovllm prepare-publish --weights mid_checkpoint.safetensors --out C:\tmp\ovllm-smoke -ovllm verify C:\tmp\ovllm-smoke --allow-unsigned --skip-replay +ovllm prepare-publish --weights mid_checkpoint.safetensors --out /tmp/ovllm-smoke +ovllm verify /tmp/ovllm-smoke --allow-unsigned --skip-replay</details> <details> <summary>🤖 Prompt for AI Agents</summary>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.mdaround lines 260 - 265, The local verifier example uses a
Windows-only absolute path in a bash-oriented README section, which is
inconsistent for cross-platform readers. Update the commands in the “Check the
verifier locally” snippet to use a POSIX-style temporary path instead, keeping
the sameovllm prepare-publishandovllm verifyworkflow but replacing the
C:\tmp\ovllm-smoketarget with a Unix-like path so the example matches the
rest of the document.</details> <!-- cr-comment:v1:2efc1b1d6b4862d0791164bb --> </blockquote></details> <details> <summary>.gitignore (1)</summary><blockquote> `46-49`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Stray Windows-specific ignore entry.** `C:\tmp\*` is an absolute Windows path, not a repo-relative gitignore pattern — it won't match anything meaningful on Linux/macOS checkouts and reads like a copy-paste leftover from local testing (same path shows up in README's `C:\tmp\ovllm-smoke` example). Worth dropping or replacing with a portable pattern. <details> <summary>🧹 Suggested cleanup</summary> ```diff # Local smoke tests and temp files openverifiable-smoke/ -C:\tmp\*🤖 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 @.gitignore around lines 46 - 49, Remove the stray Windows-only ignore entry from .gitignore and keep the smoke-test/temp ignore rules portable. Update the ignore pattern near the local smoke tests entries so it uses a repo-relative or cross-platform path instead of the absolute C:\tmp\* entry, matching the intended cleanup around openverifiable-smoke/.
🤖 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/publish-verified-model.yml:
- Around line 56-61: Harden the workflow’s shell steps by removing direct `${{
inputs.model_name }}` and `${{ inputs.hf_repo_id }}` interpolation from the
`run:` blocks. Update the “Prepare publish directory” and the later publish step
to read these values from environment variables instead, so the shell never
parses raw workflow input as script. Keep the existing `ovllm prepare-publish`
and publish logic the same, but reference the inputs via env in the workflow
step definition rather than inline expansion.
- Around line 29-30: The Checkout step in the publish-verified-model workflow is
persisting the ambient GITHUB_TOKEN in git config, so update the
actions/checkout@v4 usage to disable credential persistence by setting
persist-credentials to false on that step. Locate the Checkout job entry in the
workflow and keep the change scoped to that checkout action invocation.
In `@src/artifacts.py`:
- Around line 55-59: The tensor hash logic in tensor_mapping_sha256 is too
brittle because calling .numpy() can fail on NumPy-unsupported dtypes like bf16,
and it only hashes raw bytes. Update tensor_mapping_sha256 (and any caller such
as prepare_publish_dir that depends on it) to hash tensors without relying on
.numpy() for unsupported dtypes, and include each tensor’s name, dtype, and
shape in the digest so the integrity check is stable and less ambiguous.
In `@src/publish.py`:
- Line 165: The manifest signature field in publish.py is writing an absolute
path when the signature is not directly under the model directory, which breaks
portability. Update the manifest-building logic around the signature_path
handling to always store a path relative to model_path, using only the filename
for same-directory cases and a relative subpath otherwise, so
ovllm_manifest.json remains valid after upload on other machines.
---
Nitpick comments:
In @.gitignore:
- Around line 46-49: Remove the stray Windows-only ignore entry from .gitignore
and keep the smoke-test/temp ignore rules portable. Update the ignore pattern
near the local smoke tests entries so it uses a repo-relative or cross-platform
path instead of the absolute C:\tmp\* entry, matching the intended cleanup
around openverifiable-smoke/.
In `@README.md`:
- Around line 260-265: The local verifier example uses a Windows-only absolute
path in a bash-oriented README section, which is inconsistent for cross-platform
readers. Update the commands in the “Check the verifier locally” snippet to use
a POSIX-style temporary path instead, keeping the same `ovllm prepare-publish`
and `ovllm verify` workflow but replacing the `C:\tmp\ovllm-smoke` target with a
Unix-like path so the example matches the rest of the document.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0682063-4174-4286-9cfb-293dad03772b
📒 Files selected for processing (11)
.github/workflows/publish-verified-model.yml.gitignoreREADME.mdRUNBOOK.mdpyproject.tomlrequirements.txtsrc/artifacts.pysrc/ovllm.pysrc/publish.pysrc/verifier.pytests/test_verifier.py
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false on checkout.
zizmor flags credential persistence (artipacked) here — the ambient GITHUB_TOKEN stays in the local git config for the rest of the job unless explicitly disabled.
🔒 Suggested fix
- name: Checkout
uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 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.
| - name: Checkout | |
| uses: actions/checkout@v4 | |
| - name: Checkout | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 29-30: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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/publish-verified-model.yml around lines 29 - 30, The
Checkout step in the publish-verified-model workflow is persisting the ambient
GITHUB_TOKEN in git config, so update the actions/checkout@v4 usage to disable
credential persistence by setting persist-credentials to false on that step.
Locate the Checkout job entry in the workflow and keep the change scoped to that
checkout action invocation.
Source: Linters/SAST tools
| - name: Prepare publish directory | ||
| run: | | ||
| ovllm prepare-publish \ | ||
| --weights openverifiable-smoke.safetensors \ | ||
| --out dist/openverifiable-smoke \ | ||
| --name "${{ inputs.model_name }}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Harden run: blocks against script injection from workflow inputs.
inputs.model_name and inputs.hf_repo_id are interpolated directly into run: bash via ${{ }}, which substitutes the raw string into the script before the shell parses it — a value containing shell metacharacters can inject arbitrary commands. Static analysis (zizmor) flags this at lines 61 and 87.
🔒 Suggested fix: pass inputs through env vars
- name: Prepare publish directory
run: |
ovllm prepare-publish \
--weights openverifiable-smoke.safetensors \
--out dist/openverifiable-smoke \
- --name "${{ inputs.model_name }}"
+ --name "$MODEL_NAME"
+ env:
+ MODEL_NAME: ${{ inputs.model_name }} - name: Publish to Hugging Face
if: ${{ inputs.publish_to_hf && inputs.hf_repo_id != '' }}
env:
HF_TOKEN: ${{ secrets.HF_TOKEN }}
+ HF_REPO_ID: ${{ inputs.hf_repo_id }}
run: |
- ovllm publish-hf "${{ inputs.hf_repo_id }}" dist/openverifiable-smoke
+ ovllm publish-hf "$HF_REPO_ID" dist/openverifiable-smokeAlso applies to: 82-88
🧰 Tools
🪛 zizmor (1.26.1)
[error] 61-61: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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/publish-verified-model.yml around lines 56 - 61, Harden
the workflow’s shell steps by removing direct `${{ inputs.model_name }}` and
`${{ inputs.hf_repo_id }}` interpolation from the `run:` blocks. Update the
“Prepare publish directory” and the later publish step to read these values from
environment variables instead, so the shell never parses raw workflow input as
script. Keep the existing `ovllm prepare-publish` and publish logic the same,
but reference the inputs via env in the workflow step definition rather than
inline expansion.
Source: Linters/SAST tools
|
|
||
| manifest = _load_manifest(model_path) | ||
| original_manifest = dict(manifest) | ||
| manifest["signature"] = signature_path.name if signature_path.parent == model_path else str(signature_path) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Storing an absolute signature path in the published manifest breaks portability.
When the signature lives in a subdirectory, this writes str(signature_path) — a machine-specific absolute path — into ovllm_manifest.json, which then gets uploaded to HF. On any other box the verifier's model_dir / signature resolves back to that absolute path and won't be found. Storing the path relative to model_path keeps it portable.
🔧 Suggested relative-path fix
- manifest["signature"] = signature_path.name if signature_path.parent == model_path else str(signature_path)
+ manifest["signature"] = str(signature_path.relative_to(model_path))📝 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.
| manifest["signature"] = signature_path.name if signature_path.parent == model_path else str(signature_path) | |
| manifest["signature"] = str(signature_path.relative_to(model_path)) |
🤖 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 `@src/publish.py` at line 165, The manifest signature field in publish.py is
writing an absolute path when the signature is not directly under the model
directory, which breaks portability. Update the manifest-building logic around
the signature_path handling to always store a path relative to model_path, using
only the filename for same-directory cases and a relative subpath otherwise, so
ovllm_manifest.json remains valid after upload on other machines.
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 `@tests/test_verifier.py`:
- Around line 125-133: The test_local_signing_allows_bypass case is not
isolating the intended OVLLM_ALLOW_LOCAL_SIGNING bypass path because it inherits
ambient CI variables. Update the environment patch in
test_local_signing_allows_bypass to clear existing os.environ, mirroring the
sibling test, so sign_model_dir is exercised only through the local-signing
override and not the GITHUB_ACTIONS branch.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: 97d1d979-9bc7-4395-bde1-f8adfe6c6b39
📒 Files selected for processing (7)
.gitignoreCONTRIBUTING.mdREADME.mdRUNBOOK.mdsrc/publish.pysrc/verifier.pytests/test_verifier.py
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- CONTRIBUTING.md
- RUNBOOK.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/publish.py
- src/verifier.py
| def test_local_signing_allows_bypass(self): | ||
| with tempfile.TemporaryDirectory() as tmp: | ||
| model_dir = self._prepared_model_dir(tmp) | ||
| with patch("publish.subprocess.run") as mock_run: | ||
| mock_run.return_value.returncode = 0 | ||
| with patch.dict("os.environ", {"OVLLM_ALLOW_LOCAL_SIGNING": "true"}): | ||
| code = sign_model_dir(str(model_dir), dry_run=False) | ||
| self.assertEqual(code, 0) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Bypass test doesn't isolate the bypass path.
test_local_signing_allows_bypass only patches in OVLLM_ALLOW_LOCAL_SIGNING without clearing the rest of os.environ (unlike the sibling _fails_ test at Line 120 which uses clear=True). On an actual GitHub Actions runner, GITHUB_ACTIONS is already true in the ambient environment, so this test would pass even if the OVLLM_ALLOW_LOCAL_SIGNING bypass logic were broken — it'd just be exercising the "already in CI" branch instead.
🦘 Suggested fix
def test_local_signing_allows_bypass(self):
with tempfile.TemporaryDirectory() as tmp:
model_dir = self._prepared_model_dir(tmp)
with patch("publish.subprocess.run") as mock_run:
mock_run.return_value.returncode = 0
- with patch.dict("os.environ", {"OVLLM_ALLOW_LOCAL_SIGNING": "true"}):
+ with patch.dict("os.environ", {"OVLLM_ALLOW_LOCAL_SIGNING": "true"}, clear=True):
code = sign_model_dir(str(model_dir), dry_run=False)
self.assertEqual(code, 0)📝 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_local_signing_allows_bypass(self): | |
| with tempfile.TemporaryDirectory() as tmp: | |
| model_dir = self._prepared_model_dir(tmp) | |
| with patch("publish.subprocess.run") as mock_run: | |
| mock_run.return_value.returncode = 0 | |
| with patch.dict("os.environ", {"OVLLM_ALLOW_LOCAL_SIGNING": "true"}): | |
| code = sign_model_dir(str(model_dir), dry_run=False) | |
| self.assertEqual(code, 0) | |
| def test_local_signing_allows_bypass(self): | |
| with tempfile.TemporaryDirectory() as tmp: | |
| model_dir = self._prepared_model_dir(tmp) | |
| with patch("publish.subprocess.run") as mock_run: | |
| mock_run.return_value.returncode = 0 | |
| with patch.dict("os.environ", {"OVLLM_ALLOW_LOCAL_SIGNING": "true"}, clear=True): | |
| code = sign_model_dir(str(model_dir), dry_run=False) | |
| self.assertEqual(code, 0) |
🤖 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/test_verifier.py` around lines 125 - 133, The
test_local_signing_allows_bypass case is not isolating the intended
OVLLM_ALLOW_LOCAL_SIGNING bypass path because it inherits ambient CI variables.
Update the environment patch in test_local_signing_allows_bypass to clear
existing os.environ, mirroring the sibling test, so sign_model_dir is exercised
only through the local-signing override and not the GITHUB_ACTIONS branch.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken:
Lines 28–33 steps:
- name: Checkout
uses: actions/checkout@v4
+ with:
+ persist-credentials: false
- name: Set up Python
uses: actions/setup-python@v5Lines 53–63 --keep-artifact
- name: Prepare publish directory
+ env:
+ MODEL_NAME: ${{ inputs.model_name }}
run: |
ovllm prepare-publish \
--weights artifacts/gpt10m_shakespeare_fp32_deton_s99.safetensors \
--out dist/gpt10m-shakespeare \
- --name "${{ inputs.model_name }}"
+ --name "$MODEL_NAME"
- name: Sign with GitHub Actions OIDC
env:Lines 82–86 if: ${{ inputs.publish_to_hf && inputs.hf_repo_id != '' }}
env:
HF_TOKEN: ${{ secrets.HF_TOKEN }}
+ HF_REPO_ID: ${{ inputs.hf_repo_id }}
run: |
- ovllm publish-hf "${{ inputs.hf_repo_id }}" dist/gpt10m-shakespeare
+ ovllm publish-hf "$HF_REPO_ID" dist/gpt10m-shakespeare |
Fixed 1 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <[email protected]>
2 participants
Summary
Adds the Phase B verifier/publish loop for OpenVerifiableLLM.
This PR introduces an installable
ovllmCLI that can prepare, sign, publish, and verify small model artifacts. The verifier recomputes artifact SHA-256, Merkle metadata, safetensors tensor hashes, optional replay metadata, and Sigstore/model-transparency provenance.What changed
pyproject.tomlwithovllmconsole entry point.ovllmcommands.HfApiHF_TOKENValidation
python -m unittest discover -s testspython -m py_compile src\publish.py src\verifier.py src\ovllm.py tests\test_verifier.pygit diff --checkovllm verify C:\tmp\ovllm-proactive-smoke --allow-unsigned --skip-replayDemo path
pip install -r requirements.txt pip install -e . ovllm verify Ryoari/openverifiable-smoke --skip-replayCheck one of the checkboxes below:
I have used the following AI models and tools: Claude Fable for validation and checking
Checklist
Summary by CodeRabbit
ovllmCLI workflow for verifying models, preparing publish packages, signing, publishing to Hugging Face, and generating Ollama artifacts.