Repository navigation
fix(xtest): stop otdf-sdk-mgr resetting the tests repo through a broken worktree - #673
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 |
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>
Refreshing an existing worktree runs `git -C <worktree> reset --hard
<target>`. The worktrees live under xtest/sdk/<lang>/src,
xtest/sdk/go/platform-src and xtest/platform/src, inside the
opentdf/tests checkout. When one has lost its `.git` file -- an
interrupted `otdf-sdk-mgr clean` or a killed `git worktree add` -- git
climbs past the gitignored directory to opentdf/tests itself. A clone of
that has refs/remotes/origin/main, so the reset meant for a `main`
worktree succeeds there instead and throws away the developer's
uncommitted work. A clone made by hand in a worktree's place was reset
the same way.
checkout.require_worktree runs `git rev-parse --git-common-dir` in the
directory with GIT_CEILING_DIRECTORIES set to its parent, so git cannot
climb, and requires the answer to be the expected bare repo. The SDK,
go, platform and helper-scripts refreshes call it before resetting. A
directory that fails it raises CheckoutError, or PlatformInstallError
for the platform, naming the directory and saying to remove it. Nothing
is deleted automatically.
Removing it was not enough on its own: the bare repo still listed the
directory as a worktree, and `git worktree add` refuses a listed path
("missing but already registered worktree"). For the same reason no SDK
worktree that `otdf-sdk-mgr clean` removed could be checked out again.
Each bare-repo refresh now also runs `git worktree prune`.
The tests make the directory holding the bare repos and worktrees a git
repo with uncommitted work and refs/remotes/origin/main, as the tests
checkout is. A worktree that lost its `.git` is refused through each
entry point and the helper scripts, and that repo's HEAD and files are
left alone; before the fix the reset landed there. They also cover a
hand-made clone in a worktree's place, a worktree of a re-cloned bare
repo, and re-adding a removed worktree.
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-4372-sdkmgr-worktree-guard
branch
from
October 23, 2026 14:02
fa27f5f to
03e7e45
Compare
|
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.
First follow-up from #667's review. Based on
main, now that #667 has merged.What it does
git -C <worktree> reset --hard <target>. The worktrees live underxtest/sdk/<lang>/src,xtest/sdk/go/platform-srcandxtest/platform/src, inside this repo's own checkout. If one has lost its.gitfile (an interruptedotdf-sdk-mgr clean, or a killedgit worktree add), git climbs past the gitignored directory to opentdf/tests itself. A clone of that hasrefs/remotes/origin/main, so the reset succeeds there and throws away the developer's uncommitted work. A clone made by hand in a worktree's place got reset too.checkout.require_worktreenow runsgit rev-parse --git-common-dirwithGIT_CEILING_DIRECTORIESset to the directory's parent, and requires the answer to be the expected bare repo. The SDK, go, platform and helper-scripts refreshes call it first. Anything else raisesCheckoutError(PlatformInstallErrorfor the platform), naming the directory and saying to remove it. Nothing is deleted automatically.git worktree addrefuses a listed path ("missing but already registered worktree"). So "remove it and rerun" didn't work, and neither did re-checking out an SDK worktree afterotdf-sdk-mgr clean. Each bare-repo refresh now also runsgit worktree prune.AGENTS.mdgains a gotcha about the worktrees sitting inside this checkout.Validation
uv run pytestinotdf-sdk-mgr: 216 passed (9 new). ruff, ruff format and pyright are clean.refs/remotes/origin/main, as this checkout is. A worktree without its.gitis refused through the SDK, go and platform entry points and the helper scripts, and that repo's HEAD and files are left alone. Other tests cover a hand-made clone in a worktree's place, a worktree of a re-cloned bare repo, and re-adding a removed worktree through each entry point.git worktree add.GIT_CEILING_DIRECTORIESis extra protection; the comparison alone already rejects the enclosing repo.Not done here
otdf-sdk-mgr cleanonly emptiesxtest/sdk/<lang>/src, so it never reaches the go and platform worktrees, and it fails on the go symlinks inxtest/sdk/go/src(shutil.rmtreeon a symlink). That's older and left for a later follow-up.Stack
Follow-ups from #667's review, each based on the one before:
checkout go/checkout --all