Repository navigation
fix(xtest): refresh otdf-sdk-mgr bare-repo branches; clean up interrupted js installs - #667
Merged
Merged
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 1
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 58.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 41 functions across 8 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarizes the two main changes: refreshing bare-repository branches and cleaning up interrupted JavaScript installs. It is concise, specific, and directly related to the pull reques… Linked Issues check The PR meets the coding requirements in issue #667.checkout.pyrefreshes bare repositories into prunedrefs/remotes/origin/*, resolves upstream refs, refuses deleted branches, and uses detached w…Out of Scope Changes check The changes stay within issue #667. Source changes implement bare-repository refresh, checkout error handling, repeated Go rebuilds, and JavaScript cleanup. The tests, Git isolation fixture, comments,…Full details: Title check
-
Explanation
The title clearly summarizes the two main changes: refreshing bare-repository branches and cleaning up interrupted JavaScript installs. It is concise, specific, and directly related to the pull request.
Full details: Linked Issues check
-
Explanation
The PR meets the coding requirements in issue
#667.checkout.pyrefreshes bare repositories into prunedrefs/remotes/origin/*, resolves upstream refs, refuses deleted branches, and uses detached worktrees with hard resets. The Go path uses the refreshed target.cli_install.pyconverts checkout failures intoError:output with exit status 1.installers.pyremoves incomplete JavaScript install directories infinallyblocks, including interruption cases. The PR also updatesAGENTS.mdand adds focused integration and interruption tests for these behaviors.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue
#667. Source changes implement bare-repository refresh, checkout error handling, repeated Go rebuilds, and JavaScript cleanup. The tests, Git isolation fixture, comments, andAGENTS.mdcorrections support those objectives. No unrelated benchmark, manifest, environment, multi-arm, reporting, or CI implementation is present.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 58.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 41 functions across 8 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the branch refs bright,
Then sets each worktree right.
If installs stop before the end,
The leftover files will not pretend.
The checkout path reports its way,
And leaves clean folders at the day.
Comment @coderabbitai help to get the list of available commands.
This was referenced Oct 21, 2026
Merged
dmihalcik-virtru
changed the title
…s installs `git clone --bare` records no fetch refspec, so the `fetch --all --tags` run on every later checkout refreshed tags only. Every branch stayed at clone time, and a worktree added for it -- `install tip`, or `--platform main` -- built that stale commit. checkout.BARE_REFRESH now fetches origin's branches into refs/remotes/origin/* with --prune, for the SDK and platform bare repos alike, and runs after a fresh clone too. checkout.upstream_ref maps a branch name onto those refs. A branch origin no longer has is refused with a CheckoutError rather than built from the clone's copy, and `HEAD` means origin's default branch as of the clone. Fetching into the bare repo's own refs/heads/* instead would move branches that other worktrees have checked out, and would fail for good once upstream replaced a branch `foo` with `foo/bar`. Worktrees are added --detach and reset to the refreshed ref. That also fixes go source rebuilds: checkout_go_from_platform reset an existing worktree to a FETCH_HEAD fetched into the bare repo, and FETCH_HEAD is per worktree, so every rebuild after the first failed with "pathspec 'FETCH_HEAD' did not match". `install tip` for go, js and java now reports a refused branch, or a failed git command with git's stderr, as an error rather than a traceback, as it already did for the platform. The tests run real git against a local origin, through the SDK, go and platform entry points alike: branches, HEAD, tags, SHAs and force-pushed PR refs. isolated_git keeps them off the developer's config, any inherited GIT_DIR, and the network. AGENTS.md claimed the bare clone mapped refs/heads/*, which is what hid this. It and the code comments also said `git pull` fails in a worktree; it fast-forwards or merges origin's default branch into whatever is checked out. install_js_release already removes a partial dist when npm fails (#663). It now does the same when the install is interrupted: install_release skips any dist that exists, so a Ctrl-C mid-install left a directory with no CLI that every later install accepted as installed. Part 2a of the DSPX-4372-t1 plan. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-4372-s2-sdkmgr-fixes
branch
from
October 22, 2026 19:35
2ead7ec to
96d1425
Compare
This was referenced Oct 22, 2026
dmihalcik-virtru
marked this pull request as ready for review
October 22, 2026 20:27
pflynn-virtru
approved these changes
Oct 22, 2026
|
dmihalcik-virtru
added a commit
that referenced
this pull request
Oct 26, 2026
Mark #667 merged. Record the checkout/make directory fix that goes to main on its own, 2c's rejection of abbreviated commits, the shared manifest fixture at the stage 5 merge point, and the two failures that main's runner and report already hide, which 6a now reports. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
added a commit
that referenced
this pull request
Oct 28, 2026
) Part 2b of the [DSPX-4372-t1 plan](#657), split out of #626. **Based on `main`** (#667 has merged). Adds a benchmark manifest: a JSON representation of the SDK(s) under test, including how it was created or installed (built from what source, what version, etc). This record that connects benchmark preparation to measurement, reviewable on its own before anything writes or reads it. - otdf-sdk-mgr install benchmark` (#626) writes it - pytest reads it (#670) through a reader that imports nothing from this package - this JSON shape is the contract between them. ### What it does - **`manifest.py`, schema `v1alpha1`.** `InstallationManifest` holds the measured arms in request order, the platform and provisioning `otdfctl` pins separately, an explicit status (`success`, `neutral`, `partial` or `failed`) with a message, and `metadata.sdk`. Each `ArtifactIdentity` records the requested alias, repository, commit, `dist/` directory, install method and build settings. - **Strict reads.** `from_dict` and `read()` validate rather than coerce: - The manifest and each arm must have exactly the keys `to_dict` writes; missing and unknown keys are reported together. - Only `platform`/`otdfctl` (no pin), `commit_sha` and `installed_path` may be `null`. `metadata.sdk` must be non-empty. - The status must match the contents: NEUTRAL has two or more arms and nothing installed, SUCCESS has one or more arms that are all installed, and any other status carries a `status_message`. - Every failure raises `ManifestError`, a `ValueError`, and names the JSON path (`sdk_arms[2].commit_sha`) and, for an enum, the allowed values. `read()` adds the file path and wraps JSON decode errors, so one `except ValueError` covers everything. - **Invariants at construction.** `ArtifactIdentity` and `ResolvedArtifact` refuse an unknown install method, an empty `dist/` name, or a `commit_sha` that isn't a full lowercase 40- or 64-character hex SHA. An unknown commit (a js release found on npm but not in git) is `null`, never `"None"` or `""`. `write()` creates the parent directory and replaces the file atomically, and removes its temp file if it fails. - **Checks that run before anything builds.** `dist_slug_for` predicts each arm's `dist/` directory, so `check_collision` can reject two refs that would build into one directory at different commits (`sdk/feature` and `feature`, or `Feature` and `feature` on a case-insensitive disk), and `is_neutral_preparation` can tell when every ref names one build. - **One slug function.** `refs.dist_slug` is shared by `install tip` and `dist_slug_for`, so the predicted directory is the one make creates. It rejects a ref that names no directory. ### Compatibility - `install tip` output paths are unchanged, except that a ref naming no directory (`--ref sdk/`) now exits 1 with an error instead of resetting `src/` itself. That checkout and `make` agree on the `src/` name for `sdk/` and `otdfctl/` refs is fixed separately, on `main`, in #681. - Nothing reads the manifest yet. The xtest reader in #670 needs no change: field names and locations are as before. ### Size About 540 changed lines of non-test code, over the plan's ~400. About 235 of them are code. The rest are the docstrings that the JSON contract needs, because its readers don't import this package. Review asked for the strict reader and those docs in this PR, rather than shipping a lenient reader and tightening it later. ### Validation - `uv run pytest`: 342 passed. ruff, ruff format and pyright are clean on ruff 0.17, with no `noqa`. - `test_manifest.py` covers: - a literal golden of the JSON shape, asserted in both directions, plus round trips of every status by dataclass equality; - every rejection: missing, unknown and mistyped keys, falsy `metadata`, a non-object top level, and the status/contents rules; - null commits, and collisions and neutrality with three arms of which only some share a commit; - aliases at one commit, case-only collisions, release slugs taken from the resolved tag, and the `write()` cleanup. - 20 hand-made mutations of the new checks each fail a test. - `TestCmdTip` checks that `install tip` builds into the directory `dist_slug_for` predicts, across SDKs and ref shapes. ### Stack ([DSPX-4372-t1 plan](#657)) Two parallel tracks from `main`; they share no imports and meet in stage 5. | Part | PR | Base | |---|---|---| | 2a: bare-repo and js install fixes | #667 (merged) | `main` | | 2b: manifest contract | **#668 (this PR)** | `main` | | 2c: `install benchmark` | #626 | 2b | | 2d: `otdf-local env` exports | #642 | 2c | | 3a: arms as a tuple | #669 | `main` | | 3b: two to four arms | #658 | 3a | | 3c: pytest reads the manifest | #670 | 3b | | 4: candidate-to-candidate conclusions | #659 | 3c | | 5, 6: CI and report | #660, #661 | not yet moved onto these stacks | Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
added a commit
that referenced
this pull request
Oct 30, 2026
… record benchmark arms (#626) ## TL;DR Adds **`otdf-sdk-mgr install benchmark <sdk> <ref>...`**: one command that resolves any number of SDK refs (releases, branches, SHAs, `pr:N`) to commits, installs them side by side under `xtest/sdk/<sdk>/dist/`, and writes a manifest recording exactly which commit is in each directory. It makes comparing SDK variants a local, one-command job instead of a CI dispatch or a hand-built set of `dist/` directories. It is part 2c of the [DSPX-4372-t1 plan](#657), based on `main` now that #667 and #668 (the manifest contract) have merged, and it is the preparation step CI will share (#660). ## Why: quicker local testing of multiple SDK variants Today, there are two ways to compare SDK builds: - **Dispatch the xtest workflow with `run-benchmarks`.** That is about 45 minutes per SDK per attempt, and a mis-set input (`main` rather than `main latest`) spends the whole run before it says so. - **Reproduce locally**, per `xtest/perf/README.md`: "whatever two builds you want, side by side under `sdk/<name>/dist/`". You install each build yourself and need to know: - whether each ref is a release or a source build, and the right `--ref` for it; - which directory it will land in: `pr:123` builds into `refs--pull--123--head`, and `otdfctl/v0.24.0` and `v0.24.0` share `dist/v0.24.0`; - how to provision `otdfctl` and the platform separately. Nothing records which commit a directory holds. A `dist/main` left from last week is measured, and labelled, as today's `main`. With this PR: ```bash otdf-sdk-mgr install benchmark go v0.38.0 main pr:123 --platform main --otdfctl v0.38.1 ``` - **Any number of variants in one command.** Refs are kept in order, and the first one is the reference. Stage 3 (#658) measures up to four. - **Fails fast.** Every ref, and the `--otdfctl` pin, is resolved to a full commit before anything builds, so a mistyped branch or tag costs a lookup rather than minutes of compilation. Two refs that would share one `dist/` directory at different commits are rejected before either builds. (A full SHA is not checked to exist; see Follow-ups.) - **Doesn't measure a commit against itself.** If every ref resolves to one commit, preparation stops with `neutral` instead of benchmarking A against A. - **Cheap re-runs.** A source build records its commit in `dist/<slug>/.version` and is reused only when that record matches. Re-running after pushing to a branch rebuilds the branch, not everything. The record is all it checks, so after a hand `make` of local edits, delete `dist/<slug>` (the README says so). - **A manifest instead of hand-built env vars.** `xtest/sdk/benchmark.installed.json` records, for each arm, the requested alias, repository, commit, `dist/` directory and install method, plus the platform and `otdfctl` pins. `otdf-local env` exports its path and the `otdfctl` pin (#642), and from #670 `pytest --bench` reads the arms from it. Until then, name the arms with `--bench-baseline`/`--bench-candidate`, as the README now says. CI runs this same command (#660), so a local result and a CI result come from the same preparation. ## What's in this PR - **`install benchmark`** (`cli_install.py`, `bench_prepare.py`) resolves, checks for collisions, installs releases via `install_release` and source refs via `cmd_tip`, then installs the platform and the `otdfctl` pin. - **Separate provisioning pin.** `--otdfctl` is recorded apart from the measured arms. It joins the collision check only when it shares the arms' `dist/` tree, which is only the case for Go. - **Collision and neutral checks** run before anything builds. They, `dist_slug`, and the manifest's data model are in #668; this PR is the command that uses them. - **Manifest** (schema `v1alpha1`, defined in #668). The status is `success`, `neutral`, `partial` or `failed`, with a `status_message`: - `failed` means nothing was installed; `partial` means some installs completed but not all. - The manifest is written on every exit path, an exception or Ctrl-C included. - The exit code is 0 for `success` or `neutral`, 1 otherwise. **Size.** About 640 non-test lines, over the plan's ~400-line budget. About 315 of them are code; the rest are docstrings, comments and the README section. Resolution, installation and the manifest's exit paths are one command: a PR that resolved without installing would ship a command that does nothing. Two thirds of the diff is tests. **Since the first review:** - Java release arms resolved nothing, because the wrong tag infix was used. Fixed. - The manifest now has one write site: `status` is required, reads check `schema_version`, and writes create the parent directory and replace the file atomically. - JS release arms land in `dist/vX.Y.Z`. - `platform` is rejected as an SDK. - The platform pin records the directory it was installed to. - A first-arm failure is now `failed`, not `partial`. **Since review of #668:** - **Abbreviated commits are rejected.** A ref that resolves to a commit that isn't a full SHA (in practice, an abbreviated SHA that no branch, tag or PR head points at) now fails before anything installs: "give the full commit SHA for …", with a `failed` manifest and exit code 1. Before, `abc1234` and its own full SHA weren't recognised as one build, so the commit was built twice and reported as `success`. Reusing a `dist/` directory and detecting a moved ref now compare full SHAs exactly. - An empty ref list fails instead of writing a `success` manifest with no arms. Every manifest written sets `metadata.sdk` and passes #668's strict reader. - Platform and `otdfctl` install failures are caught narrowly (`PlatformInstallError`, `BenchPrepareError`, `OSError`). Anything else aborts with its traceback, and the manifest records `partial` "Preparation aborted", as arm installs already did. A platform whose `.version` has no full commit is `partial`. **Since the 2026-10-29 review:** - An arm whose stale `dist/` cannot be removed (`OSError`) is now `failed`/`partial` with the error, like the `otdfctl` pin, rather than "Preparation aborted". - If the *first* manifest write fails, the CLI no longer prints "Manifest: …" pointing at an earlier run's file. `prepare_benchmark` raises `BenchPrepareError` for that case. `RuntimeError` is no longer caught around the whole run: `xtest/sdk` is located up front, `--manifest` or not. - The CLI tests now go through Typer (`CliRunner().invoke(install_app, …)`), so a lost registration or renamed option fails them. They also assert a clean `typer.Exit`, since `CliRunner` reports an uncaught exception as exit 1 too. - New tests: - npm-only js releases: one version is `neutral` ("whose commit is unknown"), two versions are not; - a dist of unknown provenance is emptied before rebuilding; - an arm `OSError`; - a failed final write. - Comments: two were wrong and are now corrected: - the repeated-builds example `v0.38.0 main` is in fact neutral, so it is now `v0.38.0 main pr:123`; - the `otdfctl` pin "never provisions the fixtures it is measured against" contradicted `check_collision`. Stale ones fixed: js tags are `sdk-vX.Y.Z`; the go fallback affects `v0.30.x`, not "older tags"; the java Makefile sources `java/<slug>.env`. The `# type: ignore` is gone, and the CLI help example now uses `v0.38.0`/`v0.38.1`. - README: - says reuse trusts `.version`, and that a release `dist/` is reused as is; - says a full SHA is not checked; - says `pytest --bench` doesn't read the manifest yet. **Validation** (on this commit): - `uv run pytest` 404 passed; ruff check, ruff format and pyright are clean, with no `noqa` or `type: ignore`. - Each new test was checked against a deliberate break of the code it covers: the unregistered command, the missing `rmtree`, the uncaught arm `OSError`, the unwrapped first write and the neutral message all fail at least one test. - Live, from an empty `xtest/sdk/go`: - `install benchmark go v0.38.0 main --otdfctl v0.38.1` gave `success`, exit 0. The arms were `v0.38.0` at `308002c2` (release) and `main` at `ef874839` (source). `dist/main/.version` matches `git -C src/main rev-parse HEAD`. The pin was `v0.38.1` at `99b51145`. - The same command re-run gave `success` in 1.4 s, with nothing rebuilt. - `install benchmark go 0e4b0d1 main` gave `failed`, "give the full commit SHA for 0e4b0d1", with exit 1 and no arms. - `--manifest` under a regular file gave "Error: cannot write the manifest …", exit 1, and no "Manifest:" line. - The platform pin (`--platform`) was not exercised live on this commit; its last live run predates the review fixes. ## Stack ([DSPX-4372-t1 plan](#657)) Two parallel tracks from `main`; they share no imports and meet in stage 5. | Part | PR | Base | |---|---|---| | 2a: bare-repo and js install fixes | #667 (merged) | `main` | | 2b: manifest contract | #668 (merged) | `main` | | 2c: `install benchmark` | **#626 (this PR)** | `main` | | 2d: `otdf-local env` exports | #642 | 2c | | 3a: arms as a tuple | #669 | `main` | | 3b: two to four arms | #658 | 3a | | 3c: pytest reads the manifest | #670 | 3b | | 4: candidate-to-candidate conclusions | #659 | 3c | | 5, 6: CI and report | #660, #661 | not yet moved onto these stacks | #667 and #668 were split out of this PR so the fixes and the contract can be reviewed on their own. Split out of an earlier version so they could land first: #638, #639, #640 and #641, all merged. #647, the golden-key configuration CI needs once `otdf-local` starts the platform, is still open. **Related, independent of this stack:** #684 makes go release installs put a real `otdfctl` binary in `dist/`. Before it, a go release arm ran `go run` on every call (about 370 ms wall and 1 s CPU, against 54 ms for the binary), while source-built candidates ran a binary. That made release baselines look slower than they are. ## Follow-ups None of these blocks merge. All but the "Re-run" message are now noted in the code or the README. - **Go repository URL.** Go arms are all recorded under the monorepo URL. The monorepo carries `otdfctl/v0.10.0`–`v0.29.0` at the archived repo's commits, but not `v0.30.x`, which `resolve` answers from the archived repo. That makes `v0.30.x` arms the ones recorded with a commit the monorepo lacks. The fix needs `resolve` to report the repo it matched. - **Fallback on any error.** Go resolution falls back to the archived otdfctl repo whenever the monorepo lookup fails, so a transient error can resolve `main` or `pr:N` against the wrong repo. A source arm then fails the read-back, but the error says "the ref moved". - **Unhelpful "Re-run" advice.** "The ref moved… Re-run" is wrong for a worktree that `is_mutable_ref` treats as fixed (full SHAs; web-sdk `release/x.y.z` branches). Checkout never resets those, so a re-run fails the same way. The message should name `src/<slug>` and say to delete it. - **Reuse trusts `.version`.** The SDK Makefiles rebuild into `dist/<slug>` without touching `.version`, and `_built_commit` doesn't check for a dirty worktree. The README now tells users to delete the directory; a code check (`git status --porcelain`, a binary hash) would be better. - **Release dists reused as is.** A release arm adopts an existing `dist/<tag>` (`install_release`'s skip), including a half-finished install. - **Full SHAs unverified.** A typo fails at checkout after the arms before it have built, giving `partial`. - **`--platform` not resolved up front.** It is not resolved before the arms build: `--platform latest` becomes `service/vlatest` and fails after every build. The release/source label for the pin is also a `startswith("v")` guess; both installers build from source. **Downstream:** #642 is rebased onto this commit with no conflicts (it touches only `otdf-local` and `xtest/perf/README.md`). Its local-run example uses `--otdfctl v0.30.0`, one of the `v0.30.x` tags above; `v0.38.1` would avoid that. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **New Features** * Added an `install benchmark` command to prepare SDK benchmarks from ordered references, with optional platform and CLI tool artifacts. * Benchmark preparation validates references and installation conflicts, reuses matching builds, and records progress and results in a manifest. Results distinguish successful, neutral, partial, and failed preparations. * **Documentation** * Added guidance for preparing benchmarks and troubleshooting common test failures. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
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.
Part 2a of the DSPX-4372-t1 plan, split out of #626: fixes the benchmark needs, which every
otdf-sdk-mgrinstall benefits from. Based onmain.What it does
git clone --barerecords no fetch refspec, so thefetch --all --tagsbefore every checkout refreshed tags only. Every branch stayed where the first clone found it, andinstall tip mainor--platform mainbuilt that stale commit.checkout.BARE_REFRESHnow fetches origin's branches intorefs/remotes/origin/*with--prune, for the SDK and platform bare repos alike, andcheckout.upstream_refmaps a branch name onto them. Fetching into the bare repo's ownrefs/heads/*instead would move branches that other worktrees have checked out, leaving their HEAD ahead of their files, and would fail for good once upstream replaced a branchfoowithfoo/bar.HEADmeans origin's default branch as of the clone; a platformHEADworktree used to reset to itself.checkout_go_from_platformreset an existing worktree to aFETCH_HEADfetched into the bare repo, which no worktree can see, so every rebuild after the first failed withpathspec 'FETCH_HEAD' did not match. It now resets to the refreshed ref, like the SDK and platform paths.install tipreports checkout failures as errors. A refused branch, or a failed git command with git's stderr, now printsError: …and exits 1 for go, js and java, as the platform path already did, instead of a traceback.dist/when npm fails. A Ctrl-C mid-install still left a directory holding onlycli.sh, whichinstall_releasethen skipped as installed from then on. Both js install paths now remove it unless the install finished.AGENTS.mdclaimed the bare clone mappedrefs/heads/*, which is what hid the first bug. It and the code comments also saidgit pullfails in a worktree; it merges origin's default branch into whatever is checked out.Validation
uv run pytestinotdf-sdk-mgr: 207 passed, and 321 at the top of this track (feat(xtest): export benchmark manifest pins from otdf-local env #642). ruff, ruff format and pyright are clean.test_checkout.pyruns real git against a local origin. Every shared behaviour is parametrized over the SDK, go and platform entry points: new and existing worktrees follow their branch,HEAD(including beside a tag namedmain), a deleted branch is refused, tags and SHAs are checked out and then reused, and a force-pushedrefs/pull/N/headis followed. This replaces the stubbed command-string test for platform PR refs. SDK-only tests guard other worktrees, force pushes andx→x/yrenames; platform-only tests cover the helper-scripts worktree. ACliRunnertest covers theinstall tiperror path.symbolic-ref --short, treating every non-origin name as a deleted branch, dropping go's explicit PR-ref fetch or its+, swallowing go's refusal, and dropping theinstall tipcatch each fail a test.isolated_gitkeeps those tests off the developer's git config, any inheritedGIT_DIR, and the network (GIT_ALLOW_PROTOCOL=file).Follow-ups
Older problems found while reviewing this PR are fixed in a stack of drafts based on it: #673 (don't reset a directory that isn't a worktree) → #674 (branch names vs versions) → #675 (tags moved upstream) → #676 (
checkout go/--all).Stack (DSPX-4372-t1 plan)
Two parallel tracks from
main; they share no imports and meet in stage 5.maininstall benchmarkotdf-local envexportsmainSummary by CodeRabbit
Bug Fixes
Documentation