Repository navigation
Conversation
Deicyde
left a comment
There was a problem hiding this comment.
Review result: changes needed at b9fc9e9.
- The portable fallback is not a safe generation boundary.
_capture_portablechecks a child withlstat, then descends through its live pathname. A concurrent directory-to-symlink or junction swap can redirect traversal outside the root; repeating the swap for both comparison passes produced an accepteda/b/secret.leanfrom outside the tree. Use native no-follow handles, or fail closed when safe descriptor traversal is unavailable. BoundDirectoryTree.capture()reuses one retained directory descriptor. Concurrent captures interfere through the shared directory offset: on an unchanged 300-file tree, 196 of 200 paired captures failed withTreeSnapshotError, while sequential capture returned all 300 files. Serialize capture/verify/close or open an independent directory handle per operation.- Root and exclusion normalization disagree. The direct snapshot APIs reject ordinary macOS paths under
/tmpand/varbecause those ancestors are symlinks, whileindex_projectresolves them first. In addition,index_project(alias, exclude_roots=(alias / "generated",))silently ignores the absolute exclusion whenaliasis a supported symlinked root, and indexes the excluded Lean file. Normalize the root and its exclusions as one mapping. build_linker(source_index=...)detects liveHEADindependently from the supplied snapshot. Capturing at commit A, advancing to B, then building the linker emitted a B permalink for a declaration that existed only in A. Downstream #35 uses this sequence. Require a ref bound to the snapshot or an explicit matching ref.
The focused snapshot suite passes (103 passed, 3 skipped), and the full suite passes (649 passed, 4 skipped), so these need concurrency and temporal-binding regressions rather than adjustments to existing assertions.
Deicyde
left a comment
There was a problem hiding this comment.
Second pass on exact head b9fc9e9. Additional findings:
- [P1] Audit escapes the captured Lean generation.
_source_spans()rereadsindex.root / pathataudit.py:428even though the newSourceIndexalready stores capturedline_counts. In a deterministic repro, a one-line captured file was rewritten to 251 lines after indexing;index.line_countsremained 1 while_source_spans(index)returned 251. This can produce false size findings from a mixture of generations. Consume only the captured line counts. - [P2] The new fail-closed source errors escape public CLI boundaries. With a valid bundled blueprint and a Lean root containing
Linked.lean -> outside,autoform checkprinted its initialOKline and then raised an uncaughtOSError: unsafe Lean source ...;audit,render, and runtime callers have the same unadapted boundary. Translate snapshot failures into stable command-specific errors/exit codes, and do not print success before all requested validation completes. - [P2] Publication-root classification is quadratic.
lean.py:326-341scans the growingpublication_rootsset for every candidate, then repeats linear root scans per entry. Synthetic snapshots took 1.19s, 4.71s, and 19.45s for 500, 1,000, and 2,000 sibling roots. The capture limits do not bound this post-capture CPU. Use a prefix index or a linear sorted ancestor walk. - [P2] Valid bounded inputs can leak raw exceptions.
TreeCaptureLimits(max_file_bytes=sys.maxsize)on a one-byte file raisesOverflowErrorbecause_read_filerequestsmax_bytes + 1; a 5 KB non-markerpublication.jsonwith an integer over Python's digit limit raises plainValueErrorbecause_is_publication_manifest_bytescatches onlyJSONDecodeError. Chunk/cap reads safely and convert all bounded parser failures to the documented snapshot/index error contracts.
Validation: focused snapshot/graph/runtime tests 133 passed, 3 skipped; four independent regression repros passed; Ruff passed.
|
Ready-review blockers are fixed at The final design fails closed where descriptor-safe source traversal is unavailable instead of accepting repeatable path captures. It also prunes managed outputs before source capture, restores exact source/output casing semantics, sanitizes path failures, binds captured indexes by root identity, and prevents Git-ref/source-generation mismatches. Validation is green: Python 3.10, Python 3.13, Windows capability/publication hardening, and the real-Lean fixture job all pass. Local affected tests, focused regressions, lint, and the bundled example/site build pass as recorded in the updated PR description. |
|
Current head |
|
Re-audit result: current head The four reverts restore the exact Two further blockers reproduce:
The follow-up mapping in the body is also incorrect: #63–#65 do not track the Recommended repair: reapply |
Summary
directory identity through descriptor APIs where available and using a
repeatable, revalidated portable fallback elsewhere
the same captured bytes
accepting case-alias escapes
and aggregate captured bytes
paths, and unavailable bounded enumeration
skeleton caller boundaries without exposing host paths
Stack and scope restoration
PR #15 is merged in upstream main. Commit
758ede0additively merged that maininto reviewed candidate
5e34514;c54be00then repaired Windows path-stat vsdescriptor-stat normalization while preserving full same-interface checks.
Four later commits attempted the independent follow-up work now tracked in
#63, #64, and #65, but expanded this PR and introduced new source-classification
regressions. Current head
f7acc1econtains four ordinary revert commits—noforce-push—and its tree is byte-for-byte identical to reviewed
c54be00:30b140806c706114c232fd3967077a74e6accc9dgit diff c54be00..f7acc1e: emptyThe follow-up hardening remains in its focused issues rather than inflating or
changing #28's reviewed contract.
Validation
37068399307: Python 3.10, Python3.13, Windows hardening, real Lean, and CLA all green
make lintmake check-examplec54be00Fresh CI on
f7acc1eremains the merge gate; the PR will be relabeled readyonly after every required job passes.