fix: a replaced archive is graded from its new bytes (P-868)

[Acture]

A student who re-uploaded bob_hw.zip under the same name was graded on the first upload: archive::expand skipped any entry whose output file already existed and never cleared its directory. Because the scan lists that directory, an entry dropped from the new archive also stayed and came back as a student of its own (a dropped util.py became local:util). The Canvas loader shares expand, so a re-downloaded attachment at the same path had the same fault. Reproduced through the CLI: bob scored 50 before and after uploading the fix. With this branch the scores are 50 and then 100.

expand now clears its target before opening the archive and writes every entry. The directory then holds exactly what the archive yields now. A replacement that will not open grades nothing instead of its predecessor's files, and a failed write removes its partial file.

The local target is .scriptmark_extracted/<archive file name>/, no longer <stem>/. Clearing a stem-named directory would let hw.zip and hw.tar delete each other's files. Worse, the stem of ...zip is .., so the scan directory itself would be removed. Canvas keeps attachments/<id>/<stem>.extracted, since that directory holds one archive per id.

Consequences before merging

• A record graded before this change lists <stem>/ paths, so rescore refuses it as "submitted files changed". Grade again.
• Every scan and every Canvas bundle::load re-expands, preview and check included. A read-only submissions directory or bundle reports ArchiveUnreadable instead of reusing an old extraction. Running two commands over the same submissions at once is unsupported.
• Old <stem>/ directories stay on disk until removed by hand. Nothing reads them.

Tests

Nine new tests, all failing before the fix except the guard for an unchanged archive. In archive.rs: same-name same-size content, a dropped entry and a renamed entry, an unchanged archive expanded twice, and an unreadable replacement. In discovery.rs: a replacement with no phantom student, bob_hw.zip and bob_hw.tar side by side, and ...zip/..zip. One Canvas bundle reload test, and the report's steps through the CLI in tests/record.rs.

cargo test --workspace: 418 passed. cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --check and git diff --check are clean. A five-lens review in which two refuters checked each finding found no deletion-safety issues; its three surviving minor findings are fixed. Design and validation notes: notes/scriptmark/docs/plans/2026-10-04-p868-fresh-archive-extraction.md (notes f7bba71).

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Replacing an archive now refreshes its extracted contents during scans and grading. Removed or renamed entries no longer linger, and grading uses the replacement archive’s files.
  • Archives with the same name stem but different extensions now use separate extraction locations.
  • Unreadable archives now produce a clear diagnostic instead of leaving stale extracted files behind.

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 9b96aaeb-1c89-4879-8f91-fecdacc769ac

📥 Commits

Reviewing files that changed from the base of the PR and between b3dda0b and 763e805.

📒 Files selected for processing (6)

• crates/scriptmark/src/archive.rs
• crates/scriptmark/src/canvas/bundle.rs
• crates/scriptmark/src/discovery.rs
• crates/scriptmark/src/models/result.rs
• crates/scriptmark/tests/record.rs
• notes

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Archive expansion and discovery now refresh extracted contents and use full archive filenames for extraction directories. Tests cover archive replacement during scanning, loading, and grading. The notes submodule reference also changed.

Changes

Archive extraction and replacement

Layer / File(s)	Summary
Rebuild archive expansion targets crates/scriptmark/src/archive.rs	Expansion clears and recreates its target, writes accepted entries on each call, and reports preparation failures. Tests cover replacement, removed entries, repeat expansion, and unreadable archives.
Refresh archive discovery crates/scriptmark/src/discovery.rs	Discovery uses the full archive filename for extraction directories. Tests cover refreshed scans, same-stem archive formats, unusual filenames, and provenance.
Verify replacement in loading and grading crates/scriptmark/src/canvas/bundle.rs, crates/scriptmark/src/models/result.rs, crates/scriptmark/tests/record.rs	Loading and grading tests verify that replacement archives supply current contents without stale entries. The submission documentation describes archive replacement as a changed submission.

Notes submodule reference

Layer / File(s)	Summary
Update submodule reference notes	The notes submodule pointer changed.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 763e8

Archive extraction now rebuilds from the current archive contents. Tests cover the replacement, loading, and grading paths. No concrete merge-blocking issue was found.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 763e8

Refreshing extraction fixes stale grading and separates same-stem archives. However, refresh now recursively deletes directories whose ownership is assumed rather than enforced. Modified bundle paths or redirected extraction parents could extend deletion outside the intended workspace. Normal student upload names remain constrained.

Retained concerns

• Medium · security · inferred: Recursive refresh relies on unverified target ownership. Canvas loading accepts manifest attachment paths outside the bundle and deletes their derived sibling extraction directories before decoding. Local target construction is lexically contained but does not reject redirected parent directories. If a less-trusted actor controls those manifest paths or parents, refresh can delete unrelated derived directories writable by the grading process. Normal fetched attachment names are constrained; attacker control of manifests or parents is not established.

Security review details

Security Blast Radius

• inferred — The added destructive authority is filesystem-local and bounded by the grading process's permissions. A manipulated Canvas manifest can select an existing archive-suffixed file and cause its derived stem.extracted sibling to be cleared outside the bundle. A redirected local extraction parent can similarly move a full-filename target outside the scan root. Broader tenant, service, or credential exposure is not established.

Security Findings and Attack Paths

• inferred — The introduced ownership concern requires control of persisted manifest paths or extraction-parent filesystem state, followed by a user loading or scanning that workspace. Deletion occurs before decoding, so a valid archive body is not required once the selected path passes the caller's file check and format recognition. Whether such workspace control belongs to an untrusted actor remains unresolved.

Trust Boundaries and Controls

• observed — Normal Canvas fetch reduces student-controlled display names to one basename and stores attachments under server attachment IDs. Local discovery uses direct entry filenames for target names. These controls counter filename traversal and same-stem interference, but offline manifest paths bypass the fetch path constructor and are not checked for bundle containment.

Resilience and Maintainability Implications

• observed — Sequential refresh recovers from previous extraction leftovers, but rebuilding is in place without locking or atomic publication. Interruption can leave partial output; concurrent refresh can invalidate another command's files and provenance. The PR explicitly declares concurrent commands unsupported, limiting this to an unenforced operational precondition rather than a retained security finding.
• observed — Returning successful entries after a later decoder error, accompanied by an ArchiveUnreadable warning, predates this PR. Grading gates reject Error diagnostics rather than warnings. This is an existing partial-success contract, not evidence that the refresh introduced an archive-level fail-open condition.

Hardening Proposals

• proposed — Make destructive target ownership explicit: constrain loaded attachment paths to their expected bundle directories and reject redirected extraction parents using race-resistant filesystem handling. Document whether externally supplied or shared bundle manifests are trusted.
• proposed — Enforce single-writer operation or introduce staged extraction with synchronized publication and reader lifetime protection. Preserve the intended rule that an invalid replacement must not expose its predecessor, and define explicitly whether partially decoded archives may be graded.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly states the main change: grading now uses the new contents of a replaced archive.
Docstring Coverage	✅ Passed	Docstring coverage is 88.46% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 5 files. (1 skipped: 1 …
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}