Repository navigation
fix(xtest): stop otdf-sdk-mgr mistaking branch names for versions - #674
Draft
dmihalcik-virtru wants to merge 1 commit into
Draft
dmihalcik-virtru wants to merge 1 commit into
dmihalcik-virtru wants to merge 1 commit into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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. Comment |
This was referenced Oct 22, 2026
dmihalcik-virtru
added a commit
that referenced
this pull request
Oct 23, 2026
…pted js installs (#667) Part 2a of the [DSPX-4372-t1 plan](#657), split out of #626: fixes the benchmark needs, which every `otdf-sdk-mgr` install benefits from. **Based on `main`.** ### What it does - **Bare repos refresh their branches.** `git clone --bare` records no fetch refspec, so the `fetch --all --tags` before every checkout refreshed tags only. Every branch stayed where the first clone found it, and `install tip main` 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 `checkout.upstream_ref` maps a branch name onto them. Fetching into the bare repo's own `refs/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 branch `foo` with `foo/bar`. - **Worktrees are detached** and reset to the refreshed ref. A branch origin no longer has is refused rather than built from the clone's stale copy, and `HEAD` means origin's default branch as of the clone; a platform `HEAD` worktree used to reset to itself. - **Go source rebuilds work a second time.** `checkout_go_from_platform` reset an existing worktree to a `FETCH_HEAD` fetched into the bare repo, which no worktree can see, so every rebuild after the first failed with `pathspec 'FETCH_HEAD' did not match`. It now resets to the refreshed ref, like the SDK and platform paths. - **`install tip` reports checkout failures as errors.** A refused branch, or a failed git command with git's stderr, now prints `Error: …` and exits 1 for go, js and java, as the platform path already did, instead of a traceback. - **An interrupted js install leaves nothing behind.** #663 already removes a partial `dist/` when npm fails. A Ctrl-C mid-install still left a directory holding only `cli.sh`, which `install_release` then skipped as installed from then on. Both js install paths now remove it unless the install finished. - `AGENTS.md` claimed the bare clone mapped `refs/heads/*`, which is what hid the first bug. It and the code comments also said `git pull` fails in a worktree; it merges origin's default branch into whatever is checked out. ### Validation - `uv run pytest` in `otdf-sdk-mgr`: 207 passed, and 321 at the top of this track (#642). ruff, ruff format and pyright are clean. - `test_checkout.py` runs 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 named `main`), a deleted branch is refused, tags and SHAs are checked out and then reused, and a force-pushed `refs/pull/N/head` is followed. This replaces the stubbed command-string test for platform PR refs. SDK-only tests guard other worktrees, force pushes and `x` → `x/y` renames; platform-only tests cover the helper-scripts worktree. A `CliRunner` test covers the `install tip` error path. - Mutation-checked: `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 the `install tip` catch each fail a test. - Two new tests interrupt each js install path. - `isolated_git` keeps those tests off the developer's git config, any inherited `GIT_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](#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 (this PR)** | `main` | | 2b: manifest contract | #668 | 2a | | 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 | <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * SDK and platform worktrees now follow updated branches and pull request refs, including after force-pushes. Missing branches produce a clear error instead of a traceback. * Interrupted JavaScript installations no longer leave partial distribution files behind. * **Documentation** * Updated platform installation guidance for branch refreshes, detached worktrees, and pull request refs. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
`install tip --ref my-branch platform`, the example in the tip docstring, failed with "invalid reference: service/vmy-branch". _resolve_platform_ref put the platform's `service/` tag infix on any name that wasn't `main`, `HEAD`, hex, or slashed, and normalize_version only prepends a `v`. It now prefixes only a plain version, one parse_semver accepts, and passes every other name through for git to resolve. The callers that relied on the prefix all pass versions: the LTS pin, `install release platform:X`, and scenario `dist:` pins. A branch whose last segment looked like a version, `release/v1.2.3` or web-sdk's `release/1.3.1`, counted as a tag. is_mutable_ref judges by name alone, so each later checkout reused the worktree instead of resetting it, and install_platform_source served the first build's dist. It also named that dist after the version tail, so the branch and release v1.2.3 shared one. Each install silently built a stale commit, which is what #667 set out to stop. By the time a worktree is reset or reused, upstream_ref has already asked origin: it maps every branch origin has onto refs/remotes/origin/*. The new refs.is_mutable_target takes such a target, or any other `refs/...` ref but a tag, as mutable, and defers to is_mutable_ref for the names upstream_ref passed through. The SDK, go and platform worktree refreshes use it. It only ever adds resets: everything is_mutable_ref called mutable still is, so a reset to a branch origin deleted since the clone still fails loudly, as long as its name doesn't look like a version. install_platform_source looks for a cached dist before it touches the bare repo, so a cached release installs without the network. It now does that only for refs that can't move by construction: a full SHA, or a release tag service/vX.Y.Z (_is_pinned). Those keep their dist names. Anything else goes through _ensure_worktree and a rebuild, into a dist named after the whole ref. Other tags with version-shaped names, such as `sdk/v0.4.0`, now rebuild on every install; the old code cached them in the dist of service v0.4.0. Fetching before every cached release install instead would have broken `install lts` offline. The real-git tests move release/v1.2.3 and release/1.3.1 on origin and check each out again through every entry point, rebuild a version-named platform branch, run the tip docstring's example through the CLI, and reuse a cached release and SHA after origin is deleted. All but the last failed before the fix; it guards the offline path. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-4372-sdkmgr-branch-refs
branch
from
October 23, 2026 14:02
d6e7b99 to
bd2cb95
Compare
dmihalcik-virtru
force-pushed
the
DSPX-4372-sdkmgr-worktree-guard
branch
from
October 23, 2026 14:02
fa27f5f to
03e7e45
Compare
X-Test Failure Report |
|
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.
Second follow-up from #667's review. Based on #673 (
DSPX-4372-sdkmgr-worktree-guard).What it does
/install.otdf-sdk-mgr install tip --ref my-branch platform, the example intip's own docstring, failed withinvalid reference: service/vmy-branch._resolve_platform_refput theservice/tag infix on every name that wasn'tmain,HEAD, hex or slashed. Now only a plain version (v0.9.0,0.9.0) gets it; everything else is passed to git as-is. The LTS pin,install release platform:Xand scenariodist:pins all pass versions, so nothing that relied on the prefix changes.is_mutable_refjudges by name, sorelease/v1.2.3, or web-sdk'srelease/1.3.1, counted as a tag. Its worktree was reused instead of reset, and the platform installer served the first build's dist. It even named that distv1.2.3, the same as the release. Each later install silently built the stale commit, which fix(xtest): refresh otdf-sdk-mgr bare-repo branches; clean up interrupted js installs #667 set out to stop. After the bare-repo refresh,upstream_refhas already mapped every branch origin has ontorefs/remotes/origin/*. The newrefs.is_mutable_targettreats that, or any otherrefs/...but a tag, as mutable, and falls back onis_mutable_refonly for namesupstream_refpassed through. The SDK, go and platform refreshes use it. It only ever adds resets.install_platform_sourcechecks for a cached dist before touching the bare repo. It now skips the fetch only for a full SHA or aservice/vX.Y.Zrelease (_is_pinned). Anything else goes through_ensure_worktreeand a rebuild, into a dist named after the whole ref.AGENTS.md's "Platform tags are namespaced" and "Mutable refs auto-refresh" bullets are updated.Validation
uv run pytestinotdf-sdk-mgr: 246 passed (30 new or newly parametrized). ruff, ruff format and pyright are clean.release/v1.2.3andrelease/1.3.1on origin and check out again through the SDK, go and platform entry points. They rebuild a version-named platform branch, runinstall tip --ref my-branch platformthrough the CLI with only the Go build stubbed, and reuse a cached release and SHA after origin has been deleted.test_refs.pycan't importis_mutable_target. The cached release/SHA tests pass both before and after; they guard the offline path.is_mutable_targetignore the target fails 10 tests; keying the dist cache onis_mutable_refagain fails 4.Not done here
install tip --ref sdk/v0.4.0 platform, now rebuild on every install. They used to be cached, wrongly, in service v0.4.0's dist. Caching them again needs a fetch first, or a dist keyed on the commit.upstream_refpasses the name through and only the name is left to judge. Same as before this PR.v1.2.3) is still taken as the release; passrefs/heads/v1.2.3.Stack
Follow-ups from #667's review, each based on the one before:
checkout go/checkout --all