feat: attest source compiler validation context#33
Conversation
banteg
left a comment
There was a problem hiding this comment.
Requesting changes based on four reproducible project-environment failures at this head. The standard repository tests do not cover these cross-project boundaries: uv workspace members lose the root lock/source mappings; source fallback can force an interpreter rejected by the project; inactive or unsupported dependency declarations are treated as active Vyper authority; and the compiler adapter imports packaging from environments that are not guaranteed to provide it.
|
co-authored by gpt 5.6 sol @banteg, all four issues you reported are addressed in
The schema remains v4, and the report shape is unchanged. Would you please re-review when convenient? |
banteg
left a comment
There was a problem hiding this comment.
The original four findings are verified fixed at b066dc8: all four repros now pass and schema v4 reports the intended authority and workspace evidence. Requesting changes for four additional environment-boundary issues found during the whole-PR re-review: unlocked sibling path sources break in temporary mirrors; marker evaluation uses vyupgrade's Python instead of the selected project Python; target Python pins are discarded; and managed compiler execution depends on POSIX-only signals.
|
|
||
| with tempfile.TemporaryDirectory(prefix="vyupgrade-project-") as raw_directory: | ||
| temporary_root = Path(raw_directory) | ||
| _mirror_project_root(root, temporary_root) |
There was a problem hiding this comment.
[P1] Preserve declared path sources outside the mirrored root. An unlocked project with a valid tool.uv.sources entry such as helper = { path = "../helper" } works under native uv, but this mirror moves the project under a random temporary parent without recreating the sibling. I reproduced both source and target validation failing with Distribution not found at .../T/helper. The dependency context even attests the original helper tree, so the execution no longer matches the reported declared source. Mirror or remap declared external path sources while keeping the original relative layout.
| if root != "vyper": | ||
| roots.add(root) | ||
| return tuple(sorted(roots)) | ||
| if requirement.marker is None or requirement.marker.evaluate({"extra": extra or ""}): |
There was a problem hiding this comment.
[P1] Evaluate markers for the selected project interpreter. Marker.evaluate() defaults to vyupgrade's current Python, which can differ from the interpreter uv selects for the project. With vyupgrade on 3.13, a project requiring >=3.11,<3.12 and declaring vyper==0.4.1 when python_version < 3.12 should give the project compiler authority. This code marks it inactive, injects 0.4.3 from the pragma, and the report contains both Vyper 0.4.1 and 0.4.3 while claiming source authority. That silently validates with the wrong compiler. Build the marker environment from the selected project Python rather than the parent process.
| project_compiler: bool, | ||
| project_declares_vyper: bool, | ||
| ) -> bool: | ||
| if not project_compiler or project_declares_vyper: |
There was a problem hiding this comment.
[P2] Do not silently discard an explicit target Python pin. This always returns true for target compilation, so --target-python is removed in every project environment. A project requiring >=3.13 still passes when invoked with --target-python 3.12, and the attested compiler artifact is under Python 3.13. README documents source-python and target-python as pins for each compiler subprocess. Preserve an explicit compatible pin, or fail clearly when it conflicts with the project constraint instead of silently selecting another interpreter.
| tempfile.TemporaryFile("w+", encoding="utf-8") as stdout, | ||
| tempfile.TemporaryFile("w+", encoding="utf-8") as stderr, | ||
| ): | ||
| previous_handler = signal.signal(signal.SIGALRM, _timeout_handler) |
There was a problem hiding this comment.
[P1] Use a portable timeout for managed compilation. Windows does not provide signal.SIGALRM, signal.ITIMER_REAL, or signal.setitimer, so every managed compiler run fails here before _parse_args executes. The package publishes a generic pure-Python console tool with no OS restriction. Keep the compiler classification boundary, but implement the timeout without POSIX-only signals or explicitly guard and support the Windows path.
|
co-authored by gpt 5.6 sol @banteg, the four environment-boundary findings from the latest review are addressed at
The full suite passes (862 tests), Ruff passes, and all PR checks are green. Ready for re-review when convenient. |
co-authored by gpt 5.6 sol
Summary
Make source-validation attribution explicit and typed. Source compiles use the full selected uv project/workspace environment, report the compiler and dependency artifacts actually loaded, and classify failures without inferring dependencies or rejection from compiler text.
Why
The previous validation path translated selected dependency declarations into
--witharguments and retried failures using import names or compiler stderr. A nonzero process result alone could then be labeled as compiler rejection, even for an incoherent project/compiler declaration, an adapter failure, or a crashed process.The immediate consumer is alan-app's source-translation classifier: it needs producer-owned evidence that distinguishes a declared-context compiler rejection from environment, launch, timeout, adapter, fixed-target, and internal failures.
Design
uv --project.--frozen, or resolve from a temporary project/workspace mirror so the source tree is not mutated.requires-python; otherwise let uv select the project interpreter.packaging.Safety invariants
failure_origin = "compiler"requires a genuine managed Vyper rejection, not merely a nonzero return code.compiler_outputis populated only from compiler process stdout/stderr; environment-manager and adapter prose is excluded.Failure prevented
packages/appworkspace member no longer loses the rootuv.lockor root{ workspace = true }source mapping.packagingno longer crash the adapter before it writes structured evidence.Verification
uv run pytest tests/test_compiler.py tests/test_cli_integration.py— 256 passed.python -S.Diff breakdown
Whole-PR comparison:
master@942b1fb4→b066dc8b.Concept delta
Whole-PR conceptual comparison:
master@942b1fb4→b066dc8b.Added file/module:
compiler_runner.py— isolates compiler execution inside the selected environment and writes typed process evidence before the parent classifies the result.Added production capability abstractions:
--witharguments.Removed production capability abstractions:
Authority changes:
failure_originreplaces exception-text and generic-return-code attribution.Removed retry/inference branches:
Weight and smaller alternative
The added surface is weight-bearing because the report consumer must distinguish user-source rejection from toolchain failure using evidence produced at execution time. The smaller alternatives—nearest-lock search, dependency-name matching, importing
packagingfrom the project, or parsing uv/compiler text—would reproduce individual examples but would restore heuristic attribution.Scope and rollout
Schema v4 supersedes v3 as a clean report-contract cutover. There are no compatibility aliases, heuristic fallbacks, dual-schema mode, app-side changes, or merge in this PR. The PR remains draft with the requested reviewer fixes pushed.