fix(evidence)!: enforce explicit source ownership and bounds - #469
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 053e3d7bde
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to merged #468, addressing the subsequent repository audit rather than reopening those issues. Evidence now has one explicit source-layout contract and one runtime-owned scratch lifecycle. Removes legacy layout inference, duplicate ownership, and redundant publication hashing.
Problem and expected behavior
Change
The repository owns selection and publication layouts; the runtime owns admission, materialization, pins, and eviction. Shared native-source helpers own hashing and bounded copying. Pydantic owns persisted shapes, and ijson owns JSON parsing; no new dependency is introduced.
Format 2 requires explicit member paths, source mappings, and execution attribution. Typed models remain typed inside repository projections and queries. Failed requests discard newly acquired, unretained scratch artifacts; reused materializations are verified before cache hits.
SHA-256 identifies native content and binds manifests to provenance; it is not authentication. Publication no longer repeats the pre-copy hash or unconditionally rehashes already-validated directories immediately after atomic rename. Existing and concurrently published destinations are still verified.
Suggested review order
evidence_models.pyandrepository.py: explicit identity, membership, parsing, and publication.source_files.pyandstateless.py: bounded acquisition, integrity, and lifecycle.Contract and boundary impact
Evidence and regression coverage
Regressions cover aggregate byte/file admission, bundle/member composition, shared-input eviction, changed materializations, empty failed-request cleanup, malformed documents, old-format rejection, JSON projection, and failed-capture cache capacity.
Initial pre-fix regression run: 18 failures and 4 passes. Additional reproduced lifecycle failures now have behavioral regressions.
Example preview for
{"a.b":[42],"a":{"b":[7]}}, omitting the input digest from these rows:[{"section":"a.b","value":42},{"key":"a","value_type":"object"}]Remaining proof gaps: optional AIPerf integration and platform-specific GPU/macOS execution were not exercised. The 10,000-entry scale test isolates inventory/query behavior, not real-manifest disk hashing. No throughput improvement is claimed. Resource bounds are tested with explicit byte/file ceilings, not inferred from timing.
Observed, derived, and inferred claims remain distinguishable.
Inputs, versions, provenance, and relevant corpus or artifact identity remain bound.
Compatibility, limitation, incompleteness, and uncertainty are exposed to callers.
Request-lifecycle audit follow-up
Capture scratch cleanup previously started too late and was split across exception handlers. Admission checked capacity without retaining it across awaits, allowing overlapping captures to spend the same budget. A request scope now owns both scratch and reservations; active captures and cached ancestor inputs remain protected. Partially written output consumes its reservation without double-counting.
The broker now owns shielded finalization under AnyIO cancellation. Provider preparation no longer duplicates its task/shield workaround. Worker staging is covered during request encoding, and heartbeat failure or cancellation settles the child before removing its directory. Consolidated broker cleanup retains the final resource summary and explicitly wakes the observer, including when an escaped descendant holds inherited pipes open. Removed the redundant capture-tool cleanup wrapper and unused resource-settling helper.
Explicit preview offsets at or beyond the end now return
INVALID_INPUT, matching the documented continuation contract. Self-consistent manifests with rooted/drive-relative/backslash paths or file/ancestor collisions now fail as repository corruption before materialization. This addresses all three review threads on the original commit.Research informed the ownership design: AnyIO finalization guidance and its asyncio backend require shielded asynchronous cleanup under level cancellation; Python context managers provide lexical resource ownership. Pathlib's drive/root and joining semantics explain why
is_absolute()alone was insufficient. No new dependency or general lifecycle framework was introduced.Pre-fix tests reproduced capture leaks, overlapping admission, invalid offsets, worker staging leaks, heartbeat child leaks, and all three repository review findings. Additional regressions cover real-child cancellation, observer-thread settlement, partially written reservations, competing materialization, and final-progress ancestor retention. Native Windows and optional GPU/macOS execution remain untested.
Validation
uv run pytest -q -m 'not optional and not performance'— 414 passed, 1 skipped, 4 deselected.uv run pytest -q -m performance tests/performance/test_stateless_scale.py --tb=short— 3 passed.uv run ruff check src tests tools— passed.uv run ruff format --check src tests tools— 122 files formatted.uv run mypy src tests tools— passed, 122 files.uv run lint-imports— 2 contracts kept.uv run vulture— passed.uv run deptry src --optional-dependencies-dev-groups dev,test --per-rule-ignores 'DEP002=aiperf|py-spy|pytest|pytz'— passed.git diff --check— passed.uv run pytest -q -m process tests/execution/test_broker.py --tb=short— 39 passed; Ruff, mypy, and Vulture reran successfully.e3e909e. Full-suite results precede only that dead-helper removal and an equivalent test-string line wrap; affected broker checks passed afterward.Compatibility and safety
Review checklist