[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: 2ef43e0
10 interesting tests: 4 SKIP, 3 RECOVERED, 2 flaky, 1 KNOWN
Top 10 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
anton-107
left a comment
anton-107
left a comment
There was a problem hiding this comment.
Nice work — phase/error attribution is airtight, the backup-safety branches are exactly right, and --check correctly avoids touching the machine or disk. My concerns below are about behavior, not code hygiene. The first is the headline one.
1. Re-basing the merge on .bak silently discards user edits made after the first sync
mergePlan uses the backup as the canonical merge base whenever a .bak exists (pipeline.go:227-238), and TestPipelineRestoresBackupBeforeMerge asserts this is intended. The consequence is that on re-runs the merge no longer starts from the live pyproject.toml — it starts from the pristine original captured on the first sync.
I confirmed the fallout with a scratch test:
- Sync once (creates
.bak). - Developer hand-edits the live
pyproject.toml— e.g. addsdependencies = ["rich", "requests"]. - Sync again (e.g. after switching compute target).
- The
requestsedit is gone — the second merge re-based on.bak, which never saw it.
This defeats the purpose of the byte-preserving MergeManaged (which exists specifically to touch only the three managed regions and preserve everything else). And re-basing on .bak buys nothing for the managed regions: MergeManaged is idempotent on its own output and preserves non-managed deps, so merging onto the current live file produces identical managed-region results — without throwing away edits. The only marginal difference is self-healing a databricks-connect pin the user manually deleted, which seems like an odd thing to optimize for at the cost of silent edit loss.
The .bak original is recoverable, but edits made between syncs are not — and that's the normal workflow (sync, add a dep, re-sync). Could you confirm whether this is truly intended? I'd argue the live file should be the merge base and .bak should remain purely a one-time safety copy. There's also no test covering "second run preserves a non-managed user edit" — today that test would fail.
2. --check reports a plan a real re-run wouldn't perform (downstream of #1)
On a re-run where .bak already exists, two things in the plan are inaccurate:
plan.WouldBackupis always set for a non-greenfield project (pipeline.go:297-299), but a real re-run keeps the existing.bakand does not write a backup (applyMerge,pipeline.go:317-320). So--checkclaims a backup that won't happen.- The diff base is
.bak, not the live file (pipeline.go:279-289). On an idempotent re-run the live file already equals the merged output — a real re-run changes nothing — yet--checkprints a full diff against the original. I confirmed both: the plan shows the whole managed block as an addition even though the on-disk file is already identical to what the run would write.
Lower severity since it's advisory output, but a dry-run whose plan doesn't match the real run is misleading. Fixing #1 fixes this too.
3. constraints-only doesn't "omit the databricks-connect dependency entirely" on existing projects
The PR description says constraints-only "omits the databricks-connect dependency entirely," and the pipeline clears dbcPin (pipeline.go:144-147). That's accurate for greenfield (dev = []). But on an existing project that already pins databricks-connect, the empty value makes mergeDatabricksConnect a no-op, so the existing pin is retained (confirmed by scratch test). This is intended per the merge.go comment ("left untouched rather than ... blanked out") and is defensible — but the description over-claims. Please align the wording, or document that constraints-only only stops managing the pin rather than removing it.
Address review on the pipeline PR (concerns #1 and #2). Re-basing the merge on .bak silently discarded edits the user made to pyproject.toml between syncs: mergePlan used .bak as the merge base whenever one existed, so a re-run started from the pristine pre-sync original and any dependency the user added afterward was lost. MergeManaged rewrites only the three managed regions and is idempotent on its own output, so merging onto the live file yields identical managed regions without throwing away edits. - mergePlan now reads the live pyproject.toml as the merge base; .bak stays a one-time safety copy of the pre-sync original. - --check no longer reports a backup on a re-run (applyMerge keeps the existing .bak, so WouldBackup is set only when no .bak exists yet), and the diff is now against the live file, so an idempotent re-run reports no change — matching what a real run would do. - Replace TestPipelineRestoresBackupBeforeMerge (which encoded the old discard-edits behavior) with TestPipelineMergesOnLiveFileNotBackup, and add TestPipelineCheckReRunPlanMatchesRealRun. Also clarify the constraints-only comment: it stops managing the databricks-connect pin (greenfield dev = []; existing pin retained) rather than removing it, matching mergeDatabricksConnect's no-op-on-empty behavior. Co-authored-by: Isaac
|
Verified all three against the code (reproduced #1 and #3 with scratch tests first) and pushed fixes in 2ef43e0. #1 — re-basing on
#2 —
#3 — All gates green (build, test, lint, deadcode, gofmt). #5854 was cascade-rebased on top. |
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).pyproject.toml.MergeManagedrewrites only the three managed regions and is idempotent on its own output, so a re-run preserves edits the user made between syncs. The.bakis a one-time safety copy of the pre-sync original (created only when none exists, on the first sync of an existing project); an unreadable or unstattable existing file fails loudly rather than being misread as greenfield and overwritten.constraints-onlystops managing the databricks-connect pin rather than removing it: a greenfield project rendersdev = [](no databricks-connect), and an existing project that already pins databricks-connect keeps its pin untouched.Testing strategy
PackageManager+ stub compute +httptestserver:--checkmutates nothing (even on a read-only dir / without a package manager) and reports a plan that matches a real re-run (no spurious backup, empty diff when idempotent), greenfield vs. existing, the merge basing on the live file so between-sync edits survive, backup safety on unreadable/unstattable files, constraints-only behavior, preflight exits (E_MANAGER_UNSUPPORTED/E_UV_MISSING), 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.
This pull request and its description were written by Isaac.