[steipete]

ZIP extraction binds physical entry names and kinds to decoder metadata before callbacks or member selection. Ambiguous associations, conflicting interpretations, and unsupported decoder naming behavior fail closed instead of allowing metadata from one physical entry to authorize another. Portable and native extraction preserve the same name/kind boundary.

The binding covers decoder key ordering, Unicode name overrides, directory interpretations, and conflicting names. Admission remains ahead of member callbacks and content selection, with documented rejection of unsupported interpretations. The integrated archive paths retain the current Windows namespace guards.

The package and native addon build successfully. Validation passed all 103 Rust tests and all 420 focused ZIP, native-buffer, and archive-boundary tests. Fresh independent review through P2 is clean. Complete platform CI passed on the final PR head.

CI also exposed two test-fixture defects: extraction work could outlive the fsync fixtures, and parallel Rust copy tests could choose identical timestamp-based directory names. The fixtures now retain extraction through teardown and use an atomic sequence for unique copy directories; all original operation counts and assertions remain in force.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed September 16, 2026, 9:42 AM ET / 13:42 UTC (Revision 12).

ClawSweeper review

What this changes

The branch binds ZIP names and entry kinds to physical archive records before filtering or reading, adds coverage and benchmarks, and improves lock cleanup and test fixture lifetimes.

Merge readiness

⛔ Blocked before merge - 5 items remain

The ZIP hardening remains necessary on current main. No concrete introduced defect was found; compatibility acceptance, current runtime evidence, and resolution of the historical performance hold remain outstanding.

Likely related people: steipete (high-confidence routing candidate).

Priority: P2
Reviewed head: 001e0b77dad7bd0a679dc5434ff1d401c700516b
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The implementation is focused and well covered in source, but current behavior proof and the declared compatibility/performance gates remain incomplete.
Proof confidence	🦐 gold shrimp (3/6)	Needs stronger real behavior proof before merge: Historical loader benchmarks provide useful execution context, but the captured body and discussion do not show current native and portable extraction/read entrypoints accepting ordinary archives and rejecting conflicting kinds before callbacks, returned member bytes, or output files; the reported tests remain supplemental. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: Historical loader benchmarks provide useful execution context, but the captured body and discussion do not show current native and portable extraction/read entrypoints accepting ordinary archives and rejecting conflicting kinds before callbacks, returned member bytes, or output files; the reported tests remain supplemental. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	10 items	Repository policy and clean checkout: Read the complete root AGENTS.md and contribution/security guides. Only the root AGENTS.md was found; no maintainer-notes directory exists. Applied the fail-closed, compatibility, focused-regression, and changelog guidance. Artifact-producing checks and autoreview were not run under this read-only review contract; git diff --check passed and the checkout remained clean. Review continuity: The exact previous reviewed head differs only in native copy-test fixture uniqueness. The ZIP implementation and its previously identified proof, compatibility, and performance follow-ups are unchanged; the previous completed review reported no findings. Central behavior remains absent from main and release: Current main and v0.12.0 validate portable decoder counts and paths without associating decoded kinds with physical records. Main's native extraction association omits kind, and bounded native reads lack the new full manifest association. The lock cleanup and lock stress changes already match main, but the ZIP work remains distinct. No verified merged replacement was established.
Findings	None	None.
Security	None	None.

How this fits together

fs-safe admits untrusted ZIP metadata before native or JavaScript decoders expose entries to application policy. Accepted entries then feed bounded member reads or guarded filesystem extraction.

flowchart TD
  A[Untrusted ZIP archive] --> B[Physical metadata admission]
  B --> C[Native or JavaScript decoder]
  C --> D{Name and kind agree?}
  D -->|No| E[Reject archive]
  D -->|Yes| F[Filter or select member]
  F --> G[Guarded extraction or bounded bytes]

Loading

Decision needed

Question	Recommendation
Should the documented portable ZIP rejection boundary be accepted for existing consumers?	Accept the documented boundary: Retain fail-closed association after representative existing archives demonstrate the accepted upgrade outcomes.

Why: The stricter interpretation is security-motivated but can stop existing archive workflows; source correctness alone cannot establish the acceptable compatibility tradeoff.

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: Historical loader benchmarks provide useful execution context, but the captured body and discussion do not show current native and portable extraction/read entrypoints accepting ordinary archives and rejecting conflicting kinds before callbacks, returned member bytes, or output files; the reported tests remain supplemental. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Resolve merge risk (P1) - Previously accepted ZIPs can now fail before filters or unrelated member reads; representative upgrade outcomes and explicit acceptance of that compatibility boundary remain unestablished.
• Resolve merge risk (P1) - Additional per-entry association work affects archive admission latency. Historical source and control benchmarks failed the contributor's declared gates, and current-candidate evidence has not resolved that hold.
• Complete next step (P2) - Before merge, add current native and portable runtime proof, resolve the historical performance hold, and obtain explicit acceptance of the ZIP upgrade boundary. Redacted terminal output or logs count; screenshots or recordings are useful when they show the result directly. Remove credentials, private paths, endpoints, and other private details. Updating the PR body should trigger re-review; otherwise ask a maintainer to comment @clawsweeper re-review.
• Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production versus test growth	Net production +100 lines; tests +434 lines	The pinned introduced delta adds association checks and focused regression coverage; these totals include lock work already present on main.
Change since previous review	1 test-only file changed	The latest commit makes native fixture names unique without altering the previously reviewed ZIP behavior.

Merge-risk options

Maintainer options:

1. Resolve compatibility and performance evidence (recommended)
   Document representative upgrade results and current-candidate performance conclusions while preserving the historical failed source and control observations.
2. Pause pending an acceptable boundary
   Keep the branch unmerged if acceptable archive compatibility or admission cost cannot yet be established.

Technical review

Best possible solution:

Preserve complete pre-effect ZIP association, with an explicitly accepted compatibility boundary, demonstrated upgrade outcomes, and a resolved performance budget.

Do we have a high-confidence way to reproduce the issue?

Yes, source establishes the missing main-branch kind association, and the added fixtures identify conflicting terminal markers and decoder metadata cases. No runtime reproduction was executed during this read-only review.

Is this the best way to solve the issue?

Yes, binding all physical entries before callbacks or member selection is a focused repair, and canonical-key association correctly avoids relying on JavaScript object order. Merge readiness still depends on the declared compatibility and performance tradeoffs.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 2374204cc8d0.

Labels

Label justifications:

• P2: This is bounded archive security hardening without evidence of an active emergency or widespread user outage.
• merge-risk: 🚨 compatibility: Stricter ZIP interpretation can reject existing archives before application filters can skip unsupported entries.
• merge-risk: 🚨 other: Added admission work has an unresolved performance hold supported by historical workload measurements, independently of CI status.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Historical loader benchmarks provide useful execution context, but the captured body and discussion do not show current native and portable extraction/read entrypoints accepting ordinary archives and rejecting conflicting kinds before callbacks, returned member bytes, or output files; the reported tests remain supplemental. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

• Repository policy and clean checkout: Read the complete root AGENTS.md and contribution/security guides. Only the root AGENTS.md was found; no maintainer-notes directory exists. Applied the fail-closed, compatibility, focused-regression, and changelog guidance. Artifact-producing checks and autoreview were not run under this read-only review contract; git diff --check passed and the checkout remained clean. (AGENTS.md:24, 001e0b77dad7)
• Review continuity: The exact previous reviewed head differs only in native copy-test fixture uniqueness. The ZIP implementation and its previously identified proof, compatibility, and performance follow-ups are unchanged; the previous completed review reported no findings. (native/src/file_copy.rs:381, 001e0b77dad7)
• Central behavior remains absent from main and release: Current main and v0.12.0 validate portable decoder counts and paths without associating decoded kinds with physical records. Main's native extraction association omits kind, and bounded native reads lack the new full manifest association. The lock cleanup and lock stress changes already match main, but the ZIP work remains distinct. No verified merged replacement was established. (src/archive-zip-loader.ts:29, 2374204cc8d0)
• Association precedes member effects: The portable loader consumes each admitted canonical key once and checks the decoded entry name and kind. Native manifest validation checks ordinal, size, kind, known path, and UNIX-creator mode before planning or member selection. Existing planners reject accepted links, and native extraction rechecks kind and size before fd-relative writes. (src/archive-zip-loader.ts:39, 001e0b77dad7)
• Explicit compatibility boundary: The documentation declares rejection before filters for conflicting terminal markers and unsupported portable interpretations, including attribute-only directories, backslash-only directories, and non-UNIX high-word symlinks. Existing consumers may consequently lose previously accepted archive workflows. (docs/archive.md:142, 001e0b77dad7)
• Dependency contract signals: The changed loader directly imports JSZip; package.json and the lockfile select JSZip 3.10.2. The native patch directly consumes zip metadata, with native/Cargo.toml selecting zip 8.6.0. Their actual decoder contracts were inspected rather than inferred from names. (src/archive-zip-loader.ts:68, 001e0b77dad7)

Likely related people:

• unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Attach redacted current-candidate native and portable runtime output showing ordinary success and conflicting-kind rejection before member effects.
• Resolve the historical performance hold with current-candidate evidence while retaining failed source and control observations.
• Record explicit acceptance of representative upgrade outcomes under the documented ZIP rejection boundary.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (11 earlier review cycles; latest 8 shown)

• reviewed 2026-09-16T04:33:49.610Z sha e9c42c6 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-16T04:58:24.041Z sha e9c42c6 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-16T05:27:53.552Z sha 563208d :: needs real behavior proof before merge. :: none
• reviewed 2026-09-16T05:33:59.275Z sha 563208d :: needs real behavior proof before merge. :: none
• reviewed 2026-09-16T05:39:06.729Z sha 563208d :: needs real behavior proof before merge. :: none
• reviewed 2026-09-16T12:21:13.237Z sha af2e543 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-16T12:51:55.402Z sha 0549925 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-16T13:30:25.093Z sha a4c9c83 :: needs real behavior proof before merge. :: none

[steipete]

@clawsweeper re-review

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

[steipete]

HOLD on exact head 4fc63420b3d3d928d09f1410172d70c9f220624e / tree 4ab39486bae924fc30ef7ca653c76fb96de97ae9: the required independent performance gate is blocked.

Fresh GPT-6 Astra xhigh raw-evidence audit verified all 16 artifact digests, 288 report hashes and provenance bindings, 4,896 workload rows, and 34,272 samples. It independently recomputed 1,632 strict gates from raw samples: source ABBA 361 failures, source BAAB 370, same-artifact ABBA 234, and same-artifact BAAB 256; 1,221 total failures, including 307 pooled. The exact candidate therefore cannot merge under the predeclared median/max thresholds. Same-artifact failures make precise attribution inconclusive, but consistent Linux mixed-kind loader increases across every source block also indicate real added work.

Representative pooled source failure: macOS BAAB/off Unicode-deflate loader median +69.725% (+2,641.529 us), maximum sample average +85.381% (+6,995.883 us). Evidence root: C:\Users\Peter Steinberger\AppData\Local\Temp\fs-safe-pr421-independent-audit-d32e4b5131494594af7dd4cee0aef4fc.

Astra also challenged the implementation and proposed only security-preserving optimizations: reuse exact-equal validated names, avoid the intermediate Map tuple array, and normalize native paths only after exact-string mismatch while retaining complete archive association/kind checks. Those changes are being prepared as a separate exact revision and will receive a fresh Astra correctness challenge plus a full new ABBA/BAAB source/control study. No threshold, failed block, control, order, or workload is being discarded.

[steipete]

Optimized-head performance remains a strict BLOCK after fresh independent GPT-6 Astra xhigh raw-sample audit:

• source ABBA 35056061202: 300/408 failures
• source BAAB 35056062867: 345/408 failures
• control ABBA 35056064529: 192/408 failures
• control BAAB 35056066454: 264/408 failures
• total: 1,101/1,632 (645 source, 456 controls)

All 16 API artifact digests/sizes, 288 report seals and runner receipts, plans, exact trees/harness/workflow identities, builds, native modes, runtimes, and order were independently verified (16,666 checks, zero integrity failures). All 17 selected workloads were retained. The matrix directly enters the changed loader; both controls also fail, so attribution is noisy but no gate may be waived or subtracted.

The optimization improved the earlier result but did not establish a pass. HOLD / do not merge.

[steipete]

Performance HOLD for exact head 563208d31390516c61d1327a730b1ccef1dfc386 / tree ed98c15f94b68182e7fbf3f8523f584f70837d39.

All exact candidate workflows passed, and the fresh Astra exact-tree review found no P0-P3 issues. The independent raw performance audit also passed every provenance/integrity check, but the fixed gates did not:

• source ABBA: 232/408 failures
• source BAAB: 231/408
• same-artifact ABBA: 222/408
• same-artifact BAAB: 262/408
• total: 463/816 source, 484/816 controls, 947/1,632 overall

All rows and raw samples were retained, including two substring-selected gzip workloads; orders and controls stayed separate, with no subtraction, exclusions, cross-order pooling, or selective reruns. Mixed-kind portable-loader source rows on macOS repeated about +13.5% median latency in both orders, while same-artifact controls also showed large instability.

Do not merge this head.