[VPEX][5b] Add local-env six-phase pipeline orchestrator#5851
Conversation
Waiting for approvalCould not determine reviewers from git history. Eligible reviewers: Suggestions based on git history. See OWNERS for ownership rules. |
Integration test reportCommit: 6f31579
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 5 slowest tests (at least 2 minutes):
|
The orchestrator runs the layers in order (preflight → resolve → fetch → merge → provision → validate), records per-phase status into Result, and returns typed PipelineErrors carrying FailurePhase and DiskMutated. Under --check it computes and reports the plan (with a diff) and performs no writes — skipping the writability probe and package-manager availability, neither of which is needed to compute the plan. Backups are created only when no .bak exists and the original is a genuine os.ErrNotExist-free read; an unreadable or unstattable existing file fails loudly rather than being misread as greenfield and overwritten. Stacked on the package-manager/detection layer; provisions through the PackageManager interface against a fake in tests. Co-authored-by: Isaac
rugpanov
force-pushed
the
dbconnect/05a-pkgmanager
branch
from
cb166cb to
f24c320
Compare
rugpanov
force-pushed
the
dbconnect/05b-pipeline
branch
from
af9d41b to
c2e8b69
Compare
| } | ||
| if err := os.WriteFile(dst, data, 0o644); err != nil { | ||
| return fmt.Errorf("write %s: %w", dst, err) | ||
| } |
There was a problem hiding this comment.
copyFile creates the .bak at a hardcoded 0o644. If the user's pyproject.toml is locked down (e.g. 0o600 because it carries a private index URL or token), the backup silently becomes world-readable.
The main-file write at applyMerge is fine only incidentally — os.WriteFile ignores the mode arg when the file already exists, so the existing pyproject keeps its perms. But the backup is freshly created here, so 0o644 actually applies.
Suggest either preserving the source mode (os.Stat(src) → pass info.Mode()) or, if 0o644 is deliberate, a one-line comment saying so (the repo's comment-the-why rule).
This comment was generated with GitHub MCP.
There was a problem hiding this comment.
Good catch — fixed in 6f31579. copyFile now stats the source and creates the backup with info.Mode().Perm(), so a 0o600 pyproject (private index URL/token) is no longer widened to world-readable. Added TestCopyFilePreservesMode to lock it in.
| // Phase: preflight — manager detection, writability, package-manager availability. | ||
| // P0 supports only uv; any other detected manager is a clean, non-blaming exit. | ||
| if m := detectManager(p.ProjectDir); m != managerUv { | ||
| return p.fail(PhasePreflight, false, NewError(ErrManagerUnsupported, nil, "%s", managerGuidance(m))) |
There was a problem hiding this comment.
Two preflight exits look untested at the pipeline level:
- this unsupported-manager path →
E_MANAGER_UNSUPPORTED, and - the
EnsureAvailablefailure below →E_UV_MISSING(line ~113).
detect_test.go covers detectManager in isolation, but not the pipeline's wiring of it into a clean preflight failure (correct phase attribution, DiskMutated=false, later phases still pending). The noProvisionPM fixture already exists — a test that drops an environment.yml and asserts the E_MANAGER_UNSUPPORTED / PhasePreflight result would close the gap cheaply.
This comment was generated with GitHub MCP.
There was a problem hiding this comment.
Done in 6f31579. Added TestPipelineManagerUnsupportedFailsAtPreflight (drops an environment.yml → E_MANAGER_UNSUPPORTED) and TestPipelineUvMissingFailsAtPreflight (new uvMissingPM fixture → E_UV_MISSING). Both assert phase attribution (PhasePreflight), DiskMutated=false, and that all later phases stay pending, via a shared assertPreflightFailure helper.
Address review on the pipeline PR: - copyFile created the .bak at a hardcoded 0o644, silently widening a locked-down pyproject.toml (e.g. 0o600 carrying a private index URL) to world-readable. Stat the source and create the backup with its permission bits; os.WriteFile applies the mode only when creating the file, which is always the case for the fresh .bak. - Add pipeline-level tests for the two preflight exits that were only covered in isolation before: E_MANAGER_UNSUPPORTED (a non-uv project) and E_UV_MISSING (package manager unavailable), asserting phase attribution, DiskMutated=false, and that later phases stay pending. Add a direct copyFile mode-preservation test. Co-authored-by: Isaac
3 participants
Why
--checkdry-run mode.What
pipeline.go— the six-phase orchestrator (preflight → resolve → fetch → merge → provision → validate). It records per-phase status intoResultand returns typedPipelineErrors carryingFailurePhaseandDiskMutated. Under--checkit computes and reports the plan (with a diff) and performs no writes, skipping the writability probe and package-manager availability (neither is needed to compute the plan)..bakexists and the original is a genuineos.ErrNotExist-free read; an unreadable or unstattable existing file fails loudly rather than being misread as greenfield and overwritten.Testing strategy
PackageManager+ stub compute +httptestserver:--checkmutates nothing (even on a read-only dir / without a package manager), greenfield vs. existing, backup safety on unreadable/unstattable files, constraints-only omission, and phase/error attribution (pipeline_test.go).go build,go test,golangci-lint(0 issues),deadcode,gofmt— all green.About this stack
Review bottom-up. This PR targets #5850 as its base, so its diff shows only the pipeline layer.
This PR and #5850 together supersede #5828. The
libs/localenvtree at the tip of this branch is byte-identical to the tip of #5828's branch, apart from thepyprojectFileconst relocation and a small deadcode-guard test added in #5850.This pull request and its description were written by Isaac.