fix(terminology): consume verified cache bytes - #39
Conversation
|
ⓘ Your Qodo trial ends soon. Ask your workspace admin to set up billing to keep reviews running after the trial. Manage billing |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
@qodo review Please review exact current head |
|
@coderabbitai review Please perform a fresh review of exact current head |
PR Summary by QodoFix terminology parsing to consume verified cache bytes
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthrough
ChangesTerminology verified loading
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change makes terminology processing consume the exact bytes returned by verified cache reads while preserving existing validation and fail-closed behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Review resultI found no correctness issues in the current head for the areas requested.
No approval or findings are carried over from another PR. |
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can start a comment with 'qodo' or '@qodo' to chat about any finding |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
requested='e463eb0ca2cb04130abdc02574294d99eb9db291'
echo '== checked-out HEAD =='
git rev-parse HEAD
echo '== requested commit =='
git rev-parse "$requested^{commit}"
echo '== diff summary at requested commit =='
git show --stat --oneline --decorate --no-renames "$requested"
echo '== changed source and test diff =='
git diff "${requested}^" "$requested" -- crates/commandf-pkg/src/terminology_index.rs
echo '== terminology index outline =='
ast-grep outline crates/commandf-pkg/src/terminology_index.rs --items all --view expanded
echo '== PackageCache declarations and verified-read call sites =='
rg -n -C 4 --glob '*.rs' \
'(struct\s+PackageCache\b|impl\s+PackageCache\b|fn\s+read_verified\b|\bread_verified\s*\(|\bverify\s*\()' \
crates
echo '== terminology closure call sites and archive/path reads =='
rg -n -C 5 --glob '*.rs' \
'(TerminologyClosure|load_with_reader|TerminologyClosure::load|fs::read|read_to_end|File::open|archive_path|cache_path)' \
crates/commandf-pkgLength of output: 29459 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PackageCache implementation =='
sed -n '1,180p' crates/commandf-pkg/src/cache.rs
echo '== TerminologyClosure implementation and tests =='
sed -n '1,310p' crates/commandf-pkg/src/terminology_index.rs
echo '== Terminology error definitions and conversions =='
rg -n -C 6 --glob '*.rs' \
'(enum\s+TerminologyError\b|impl\s+From<PackageError>|PackageError|fn\s+read_manifest\b|fn\s+scan_terminology)' \
crates/commandf-pkg/src
echo '== Exact cache verification followed by direct cache-object reads =='
rg -n -U -P --glob '*.rs' \
'(?s)\.verify\(&?[A-Za-z0-9_.$\[\]]+\.sha256\)\?;.{0,900}(?:fs::read|File::open|archive_path|object_path|read_locked_archive)\s*\(' \
crates
echo '== All terminology closure loading and lockfile cache verification uses =='
rg -n -C 3 --glob '*.rs' \
'(TerminologyClosure::load\s*\(|\.verify_cache\s*\()' \
cratesLength of output: 85082
I found no findings in the requested terminology closure scope.
The regression uses a valid archive and asserts one reader invocation per locked archive. It guards against another archive acquisition inside the closure. No direct cache-path reopen remains in I also found existing verify-then-reopen patterns in separate CLI flows. They are outside this terminology closure path and outside this PR diff. I completed a static source review. I could not execute repository tests in this sandbox.
|
1 participant
Scope
Fix issue #35 by binding terminology closure parsing to the exact bytes returned by the verified cache boundary.
Canonical base:
Candidate head:
Defect
TerminologyClosure::loadpreviously calledlockfile.verify_cache(cache)and then independently reopened each cache archive withfs::read. A local mutation between verification and the second read could cause terminology analysis to consume bytes whose digest was never verified.Repair
PackageCache::read_verified;Vec<u8>to manifest parsing, package inspection, and resource scanning;Explicit non-goals
Qualification
Merge only from an exact head with mandatory CI and all path-applicable proof/oracle workflows terminal green, fresh Qodo and CodeRabbit review truth, and zero unresolved substantive findings.
Closes #35.
Summary by cubic
Fixes a cache verification gap in
TerminologyClosure::loadwhere packages were verified, then re-read from disk before parsing, leaving a window where mutated bytes could be consumed. Packages are now read exactly once through the verified cache reader, so terminology analysis always operates on digest-verified bytes.Repair
PackageCache::read_verifiedcall per archive.Written for commit e463eb0. Summary will update on new commits.
Summary by CodeRabbit