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)
|
|
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)
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 |
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.
|
|
@qodo review Please perform a fresh review of reconciled exact head |
|
@coderabbitai review Please perform a fresh exact-head review of |
Fresh review of head
No false-security claim or remaining TOCTOU gap is evident in this PR-scoped change. No findings to report. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='cf8b8e19200662cf7d6067c2d08857c12a1c06b5'
main='2b4033e237a5c74f3c45c12fbc7e7bfdc88067b1'
file='crates/commandf-pkg/src/terminology_index.rs'
echo '== exact commits and ancestry =='
git rev-parse HEAD
git rev-parse "$head^{commit}"
git rev-parse "$main^{commit}"
git merge-base "$head" "$main"
git merge-base --is-ancestor "$main" "$head" && echo 'canonical main is an ancestor of requested head'
echo '== merge commit parents =='
git show -s --format='%H%nParents: %P%nSubject: %s' "$head"
echo '== diff against canonical main =='
git diff --stat --no-renames "$main" "$head"
git diff --no-ext-diff --unified=80 "$main" "$head" -- "$file"
echo '== terminology index outline =='
ast-grep outline "$file" --items all --view expanded
echo '== cache API outline =='
cache_file=$(fd -t f '^cache\.rs$' crates/commandf-pkg/src | head -n 1)
printf '%s\n' "cache file: $cache_file"
ast-grep outline "$cache_file" --items all --view expanded
echo '== focused terminology loader and test source =='
sed -n '1,380p' "$file"
echo '== cache implementation =='
sed -n '1,260p' "$cache_file"
echo '== terminology closure: filesystem and verified-reader operations =='
rg -n -C 5 --glob '*.rs' \
'(TerminologyClosure|load_with_reader|read_verified\s*\(|\.verify\s*\(|fs::read\s*\(|File::open\s*\(|archive_path\s*\(|object_path\s*\()' \
crates/commandf-pkg/src
echo '== relevant terminology error and parser definitions =='
rg -n -C 8 --glob '*.rs' \
'(enum\s+TerminologyError\b|struct\s+TerminologyError\b|fn\s+read_manifest\b|fn\s+scan_terminology\b|fn\s+inspect_package\b|Duplicate|Canonical|Malformed)' \
crates/commandf-pkg/srcLength of output: 124025 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== terminology error conversions =='
sed -n '1,180p' crates/commandf-pkg/src/terminology_error.rs
echo '== package manifest dependencies =='
manifest=$(fd -t f '^Cargo\.toml$' crates/commandf-pkg | head -n 1)
printf '%s\n' "manifest: $manifest"
sed -n '1,240p' "$manifest"
echo '== terminology closure test and loader references =='
rg -n -C 6 --glob '*.rs' \
'(TerminologyClosure::load\s*\(|load_with_reader\s*\(|closure_consumes_each_archive_from_verified_reader_once|DuplicateCanonical|MalformedCanonical|duplicate package resource filename)' \
crates
echo '== direct filesystem/cache-root access in terminology path =='
rg -n -C 4 --glob '*.rs' \
'(cache\.root\s*\(\)|\.join\("sha256"\)|fs::read\s*\(|File::open\s*\()' \
crates/commandf-pkg/src/terminology_index.rs crates/commandf-pkg/src/terminology.rs
echo '== verified-read implementation and all direct consumers =='
rg -n -C 4 --glob '*.rs' \
'(\bread_verified\s*\(|\bread_verified_bounded\s*\(|\bverify_cache\s*\(|\bverify\s*\()' \
crates/commandf-pkg/src
echo '== reconciliation check: topic-parent change remains limited =='
git diff --stat --no-renames e463eb0ca2cb04130abdc02574294d99eb9db291 cf8b8e19200662cf7d6067c2d08857c12a1c06b5
git diff --name-status --no-renames e463eb0ca2cb04130abdc02574294d99eb9db291 cf8b8e19200662cf7d6067c2d08857c12a1c06b5Length of output: 19539 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== direct filesystem/cache-root access in terminology modules =='
rg -n -C 4 --glob '*.rs' \
'(cache\.root\s*\(\)|\.join\("sha256"\)|fs::read\s*\(|File::open\s*\()' \
crates/commandf-pkg/src/terminology_index.rs crates/commandf-pkg/src/terminology.rs || true
echo '== verified-read and verify-cache consumers =='
rg -n -C 4 --glob '*.rs' \
'(\bread_verified\s*\(|\bread_verified_bounded\s*\(|\bverify_cache\s*\(|\bverify\s*\()' \
crates/commandf-pkg/src || true
echo '== reconciliation delta from the topic parent =='
git diff --stat --no-renames e463eb0ca2cb04130abdc02574294d99eb9db291 cf8b8e19200662cf7d6067c2d08857c12a1c06b5
git diff --name-status --no-renames e463eb0ca2cb04130abdc02574294d99eb9db291 cf8b8e19200662cf7d6067c2d08857c12a1c06b5
echo '== inspect_package digest assertion =='
sed -n '1,155p' crates/commandf-pkg/src/artifact_inspect.rs
echo '== archive scan signatures =='
rg -n -C 4 --glob '*.rs' \
'(pub\(crate\)\s+fn\s+scan_package_resources\b|fn\s+scan_package_resources\b|pub\(crate\)\s+fn\s+read_manifest\b)' \
crates/commandf-pkg/src/archive.rs crates/commandf-pkg/src/artifact_scan.rs || trueLength of output: 21389
I found no findings in the requested scope.
No direct cache-path reopen remains in The injected-reader regression is meaningful. It asserts one archive request for one locked package and validates the requested digest. It will fail if the closure adds another archive acquisition through the injected boundary. The cache API separately owns and tests verification of the returned bytes. The refactor preserves the existing manifest identity check, package inspection, duplicate resource filename rejection, duplicate canonical rejection, malformed canonical handling, and fail-closed propagation of The merge head contains canonical You are interacting with an AI system. |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
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 cf8b8e1. Summary will update on new commits.
Summary by CodeRabbit