fix(sdk): parse detached upstream names correctly - #1763
Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have @tgolob on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check' |
🦋 Changeset detectedLatest commit: 5273f0f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Reviewer doubt: the JavaScript parser had the same substring heuristic as Python, so this patch fixes both SDKs even though the issue was reported against Python. Please confirm that the cross-SDK parity change and the shared patch changeset are the preferred release scope. |
There was a problem hiding this comment.
TASTE.md review of the detached-upstream parsing fix.
Principles checked: parity across JS / sync Python / async Python (T-1, T-2), naming and casing (T-9, T-12), SDK vocabulary written down in one place (T-15), no leakage into the public surface (T-18, T-19, T-20), docstrings (T-69, T-71, T-72), changeset/deprecation hygiene (T-65 – T-68).
Violations: 2 — the same one in both SDKs: the detached-HEAD marker HEAD (detached at is a piece of the SDK's own vocabulary that the parser dispatches on, and it is now spelled as a bare literal in two places per SDK (normalizeBranchName / _normalize_branch_name and the new isDetached / is_detached line). T-15 wants that set written down once, so the two dispatch sites can't drift; the leftover rawBranch / raw_branch alias also no longer buys anything now that the includes('detached') arm is gone.
Not tied to a changed line:
- The fix removes the
detachedsubstring false positive but leaves the sibling armnormalizedBranch.startsWith('HEAD')(js utils.ts:386, parse.py:130), which misclassifies the same way for a real branch whose name begins withHEAD—## HEADless-refactor...origin/HEADless-refactorstill reportsdetached: truewith nocurrentBranch/upstream. After normalization the only legitimateHEADvalues are exactlyHEAD(fromHEAD (no branch)) and the detached-at SHA, so an equality check against the two known forms would close the class of bug this PR is fixing rather than half of it. - Otherwise compliant: no public surface change, JS and Python move in lockstep (T-1, T-2) with mirrored unit tests, and the changeset covers both published packages.
The detached field's doc (utils.ts:75, types.py:40) is also a T-72 candidate — "Whether HEAD is detached" restates the field name and doesn't say that currentBranch/upstream are undefined/None in that case, which is what a caller hitting this bug needed to know. Pre-existing, so out of scope here, but this PR is the natural place to settle it.
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
Addressed the review feedback in commit
Validation: JS Vitest 4/4, Python pytest 4/4, TypeScript, Prettier, Oxlint, Ruff, Python compile, and |
|
Hey there, |
Summary
currentBranch/upstreamfor attached branches whose names containdetachedor begin withHEAD.HEAD (detached at <commit>)andHEAD (no branch)) recognized.Why
parseGitStatusused substring and prefix heuristics that could classify an attached branch such asmain...origin/detached-workorHEADless-refactor...origin/HEADless-refactoras detached. That incorrectly removed the branch and upstream values from the SDK result.This change keeps the JavaScript and Python implementations in lockstep, centralizes the detached-HEAD marker, and narrows the normalized
HEADcheck to the exact value emitted for(no branch).Fixes #1373
Validation
git diff --check