Sync shared package via repository_dispatch and stop Dependabot duplicates#575
Sync shared package via repository_dispatch and stop Dependabot duplicates#575edvilme wants to merge 14 commits into
Conversation
…icates Adds .github/workflows/shared-package-release.yml to handle repository_dispatch (shared-package-release) events from vscode-common-python-lsp: it bumps the npm dep, recompiles the pip pin via uv, pushes a branch, and opens a tracking issue with a manual-PR link (org settings block auto-created PRs). Removes @vscode/common-python-lsp (npm) and vscode-common-python-lsp (pip) from Dependabot via the ignore lists so Dependabot no longer opens duplicate update PRs for the shared package. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the inline Python rewrite of requirements.in with a sed one-liner, keeping uv pip compile --generate-hashes --upgrade to regenerate the hash-locked requirements.txt (matching the command documented in requirements.in). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Pin uv pip compile to the Python version documented in requirements.in (--python-version) so markers like exceptiongroup are not dropped - Scope the recompile with --upgrade-package so unrelated deps don't move - Assert the pip pin actually changed after sed; fail fast otherwise - Validate release_tag is present and version-shaped before any work - Add concurrency group, --force-with-lease, tracking-issue dedup, and tolerate Issues being disabled (surface compare URL via job summary) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Overall the change is sound. The main theme of the feedback is a few robustness edge cases in the |
* Integrate shared package as a git submodule Replaces the external @vscode/common-python-lsp / vscode-common-python-lsp dependency with a git submodule at external/vscode-common-python-lsp (pinned to v0.8.0). npm consumes it via a file: reference, the Python lib is bundled from the submodule, and the release pipeline now bumps the submodule commit instead of a version pin. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix CI for git submodule integration - Build the shared package TypeScript dist via a postinstall hook so the bundler and type-checker can resolve the file: dependency. - Regenerate package-lock.json to record the file: link and the submodule dependency tree. - Exclude external/ from the extension tsconfig so the submodule sources are not type-checked against the extension rootDir. - Check out submodules (recursive) in PR and push CI so the submodule and its Python library are present for npm and nox. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix VSIX packaging and type-checking for submodule integration - Exclude external/ from the VSIX (.vscodeignore) since webpack already bundles the shared package; avoids case-insensitive path collisions from the submodule node_modules. - Add skipLibCheck to tsconfig where missing so hoisted submodule devDep type definitions do not fail the extension type-check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Checkout submodules in Azure DevOps pipelines The 1ES/DevDiv checkout template does not initialize submodules, so add an explicit git submodule update --init --recursive step before npm ci and nox in the validation, stable, and pre-release pipelines. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Consolidate the sync steps into a single script, add a retry wrapper for network operations, fail when the released tag is missing instead of silently falling back to main, and surface failures as workflow annotations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Pin an explicit --force-with-lease baseline so concurrent branch updates are not silently overwritten - Regenerate the lockfile in the sync workflow so produced branches stay npm ci-mergeable - Require a real semver release_tag in the dispatch payload - Guard the postinstall build so a clone without the submodule warns instead of failing npm install Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| exit 1 | ||
| fi | ||
| echo "Checking out submodule at ${TARGET}." | ||
| git checkout --detach "${TARGET}" |
There was a problem hiding this comment.
npm install --package-lock-only regenerates the whole lockfile, so ^/~-ranged deps with newer satisfying versions can drift into the auto-pushed branch — this PR's own lock already shows an incidental prettier 3.6.2→3.9.4 plus new @types/deep-eql/@types/semver. That lands unreviewed dependency changes under a "submodule bump" branch (higher risk if the branch is ever auto-merged). Consider asserting the lock diff only touches the shared-package subtree before pushing, or regenerating in an npm ci-consistent way.
| process.exit(0); | ||
| } | ||
|
|
||
| execSync(`npm --prefix ${pkgDir} run build`, { stdio: "inherit" }); |
There was a problem hiding this comment.
This unguarded execSync('npm --prefix … run build') couples every install (including CI npm ci) to the submodule's tsc (typescript ^6) build succeeding, and npm doesn't always install a non-workspace file: dep's devDependencies. Please verify a clean recursive-clone npm ci builds this green, and confirm the built output actually ends up bundled into dist/ given .vscodeignore excludes external/**. (The missing-submodule skip guard above is good; this is about the build-success path.)
|
Overall looks good to merge. A few non-blocking robustness notes on the new submodule-sync workflow and the postinstall build step are worth a look, but nothing blocking. Note: several first-pass comments referenced |
- Split sync workflow into prepare (validate/normalize tag) and sync jobs; create-once branch semantics (no force-push), normalized concurrency key, pin actions/checkout@v4 - Fix dependabot.yml comments to reference shared-package-submodule-sync - Drop dangling '# via vscode-common-python-lsp' annotations from requirements.txt - Document the shared-package runtime-dep contract in requirements.in Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| @@ -0,0 +1,157 @@ | |||
| name: Shared Package Submodule Sync | |||
There was a problem hiding this comment.
📍 .github/workflows/shared-package-submodule-sync.yml:1
The PR description is out of sync with this implementation (Advocate: High; Architect: Critical). It still names shared-package-release.yml and claims the workflow "bumps the npm dependency, rewrites the pip pin in requirements.in and recompiles requirements.txt with uv" — none of which exists in the diff; this file moves a git-submodule pointer instead. A reviewer approving against that description approves a design that isn't present. Rewrite the description around the submodule mechanism (file: npm dep, --no-deps install from external/…/python, submodule pointer move) and add the previously-requested issue reference.
[verified]
| exit 1 | ||
| fi | ||
| echo "Checking out submodule at ${TARGET}." | ||
| git checkout --detach "${TARGET}" |
There was a problem hiding this comment.
📍 .github/workflows/shared-package-submodule-sync.yml:111
[unverified] The Skeptic notes this lock regen is a newly introduced failure surface: the submodule declares @vscode/python-environments from pkgs.dev.azure.com/azure-public/vside/_packaging/msft_consumption/…. If npm re-resolves that tarball and the runner has no .npmrc auth, the sync job fails at the lock step and silently produces no branch/issue for that release. The old sed/uv flow never ran npm install, so this is new. If that feed is anonymously readable this is a non-issue; otherwise add the registry auth step (or pin the dep so --package-lock-only reuses the existing entry) before merge.
[verified]
| process.exit(0); | ||
| } | ||
|
|
||
| execSync(`npm --prefix ${pkgDir} run build`, { stdio: "inherit" }); |
There was a problem hiding this comment.
📍 build/postinstall.js:19
[unverified] The missing-submodule skip guard is good, but this unguarded execSync('npm --prefix … run build') couples every npm ci (including CI) to the submodule's tsc (typescript ^6) build succeeding. Please confirm in CI that a fresh recursive npm ci resolves the file: link's devDeps and that the built output actually lands in dist/ given .vscodeignore excludes external/** (webpack likely inlines it, but this build-success path is unconfirmed). This is the remaining High-priority verification gap.
[verified]
|
|
||
| # Retry wrapper for flaky network operations (fetch/push). | ||
| retry() { | ||
| local attempt=1 |
There was a problem hiding this comment.
📍 .github/workflows/shared-package-submodule-sync.yml:64
Asymmetric tag-candidate loop (Skeptic/Architect, Low): for release_tag=v1.2.3 both candidates refs/tags/${RELEASE_TAG} and refs/tags/v${VERSION} resolve to refs/tags/v1.2.3, so a submodule that tagged the release as a bare 1.2.3 would not be found. Add refs/tags/${VERSION} as a third candidate so both v-prefixed and bare payloads probe both forms symmetrically.
[verified]
| "--upgrade", | ||
| "vscode-common-python-lsp==0.6.0", | ||
| "./external/vscode-common-python-lsp/python", | ||
| ) |
There was a problem hiding this comment.
📍 noxfile.py:129
The --no-deps install of the submodule's Python lib creates hidden cross-repo coupling: pygls/packaging (and transitively lsprotocol) must be hand-mirrored in requirements.in. Today's pins satisfy it (verified against requirements.txt), but if a future pinned submodule commit adds a new Python runtime dep, there's no build-time signal — it surfaces only as a runtime ImportError in the shipped extension. Consider a CI guard that diffs the submodule's declared deps against requirements.in. The documenting comment is good but doesn't remove the drift risk.
[verified]
|
Solid submodule-based sync approach, but please realign the PR description with the implemented submodule mechanism and address the lockfile/auth/build-coupling risks in the sync workflow before merge. |
Add a guard to the submodule-sync workflow that fails the run if the extension's minimum Python (runtime.txt) is older than the shared package's requires-python floor, preventing a shipped bundle that the shared library can't import. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Break the single monolithic shell step into focused steps (guard, create branch, move submodule, verify Python floor, detect change, commit/push, tracking issue). Replace in-shell early-exits with step outputs and if: conditions, and expose the retry helper to every step via BASH_ENV so network operations stay clean one-liners. No behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a Development section explaining that the shared vscode-common-python-lsp library is a git submodule and how to initialize it (clone with --recurse-submodules, or git submodule update --init --recursive for an existing clone) before running npm install. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| if git ls-remote --exit-code --heads origin "refs/heads/${BRANCH}" >/dev/null 2>&1; then | ||
| echo "Branch ${BRANCH} already exists; leaving it untouched." | ||
| echo 'exists=true' >>"${GITHUB_OUTPUT}" | ||
| else |
There was a problem hiding this comment.
PKG_FLOOR=$(sed -n 's/.*requires-python[^0-9]*\([0-9]\+\.[0-9]\+\).*/\1/p' …) greedily captures the first major.minor after requires-python, regardless of operator. A pyproject.toml expressing requires-python = "<3.14" (or ==3.11.*) yields an upper bound misread as a floor, which can spuriously fail a legit sync or — worse — let an incompatible floor pass, defeating the guard in the exact case it exists for. Anchor the match on a >=/~= lower-bound operator and hard-fail if none is present, rather than grabbing the first number.
| - name: Point the submodule at the released commit | ||
| if: steps.guard.outputs.exists == 'false' | ||
| working-directory: ${{ env.SUBMODULE_PATH }} | ||
| run: | |
There was a problem hiding this comment.
npm install --package-lock-only re-resolves the whole tree, so ^/~ deps with newer satisfying versions drift into the auto-pushed branch — demonstrated in this PR's own lock (prettier 3.6.2→3.9.4, new @types/deep-eql/@types/semver). That lands unreviewed dependency changes under a "submodule bump" branch (higher risk if ever auto-merged). Assert the lock diff only touches the external/vscode-common-python-lsp/typescript subtree before pushing, or regenerate in an npm ci-consistent way and fail on other changes. The manual PR gate mitigates but doesn't remove this.
| "lib": ["ES2020"], | ||
| "sourceMap": true, | ||
| "rootDir": "src", | ||
| "skipLibCheck": true, |
There was a problem hiding this comment.
skipLibCheck: true is added at compilerOptions scope (project-wide) to absorb the submodule's typescript ^6 toolchain mismatch, even though external/** is already in exclude/out of include. This loses .d.ts verification project-wide to solve a submodule-local problem. strict on src/** is untouched, so this is a warning, not a blocker; prefer isolating the submodule toolchain over relaxing host rigor globally.
|
Overall the submodule-based sync approach is sound. The one blocking item is that the PR description describes a different design than the diff implements — please rewrite it around the actual submodule mechanism. The remaining notes (compat-guard sed parsing, lock-file regen drift/auth on the auto-pushed branch, postinstall/build coupling, and project-wide |
Add a guard that fails the sync if the extension's pinned Node (.nvmrc) is older than the shared package's pinned Node (submodule .nvmrc), since the extension builds the shared package's TypeScript during install. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Solid submodule-based sync mechanism. Heads-up: the PR description still describes an npm/pip-rewrite workflow and a |
| run: | | ||
| set -euo pipefail | ||
| EXT_MIN="$(sed -n 's/^python-\([0-9]\+\.[0-9]\+\).*/\1/p' runtime.txt | head -n1)" | ||
| PKG_FLOOR="$(sed -n 's/.*requires-python[^0-9]*\([0-9]\+\.[0-9]\+\).*/\1/p' "${SUBMODULE_PATH}/python/pyproject.toml" | head -n1)" |
There was a problem hiding this comment.
📍 .github/workflows/shared-package-submodule-sync.yml:135
Not fixed from the prior review, and behaviorally confirmed: sed -n 's/.*requires-python[^0-9]*\([0-9]\+\.[0-9]\+\).*/\1/p' greedily grabs the first major.minor after requires-python regardless of operator. <3.14 → floor 3.14 and ==3.11.* → floor 3.11, so the downstream sort -V gate then spuriously fails a legitimate EXT_MIN=3.9 sync — defeating the guard in the exact case it exists for. Anchor the match on a >=/~= lower-bound operator and hard-fail if none is present, rather than grabbing the first number.
[verified]
| process.exit(0); | ||
| } | ||
|
|
||
| execSync(`npm --prefix ${pkgDir} run build`, { stdio: "inherit" }); |
There was a problem hiding this comment.
📍 build/postinstall.js:19
The Skeptic found a new regression: typescript@6.0.3 is a devDependency of the submodule package (the lockfile marks external/.../typescript/node_modules/typescript "dev": true). Any dev-pruned install path — npm ci --omit=dev, NODE_ENV=production, or a VSIX packager that prunes — runs this unconditional execSync('npm --prefix … run build') with no tsc available, so the build (and the install) fails where the old prebuilt registry dep succeeded. Separately, the build-success + dist/ bundling path is still unverified given .vscodeignore excludes external/**. [unverified] — please confirm a fresh recursive npm ci → webpack → VSIX builds green (dev-included and dev-pruned) before merge, and guard the build (skip if already built) so pruned installs don't hard-fail.
[verified]
| fi | ||
| if [ "$(printf '%s\n%s\n' "${PKG_NODE}" "${EXT_NODE}" | sort -V | head -n1)" != "${PKG_NODE}" ]; then | ||
| echo "::error::Extension Node version (${EXT_NODE}, from .nvmrc) is older than the shared package's pinned Node (${PKG_NODE}). Update .nvmrc, or investigate the shared release before syncing." | ||
| exit 1 |
There was a problem hiding this comment.
📍 .github/workflows/shared-package-submodule-sync.yml:160
Not fixed: npm install --package-lock-only re-resolves the whole tree, so ^/~-ranged deps with newer satisfying versions drift into the auto-pushed branch — demonstrated in this PR's own lock (prettier 3.6.2→3.9.4, new @types/deep-eql/@types/semver). --ignore-scripts doesn't stop range re-resolution. That lands unreviewed dependency bumps under a "submodule bump" branch and dilutes the "one SHA = one reviewable change" contract the redesign establishes. Assert the lock diff only touches the external/vscode-common-python-lsp/typescript subtree before pushing, or regenerate in an npm ci-consistent way and fail on other changes. Additionally [unverified]: if the submodule's Azure-feed dep (@vscode/python-environments) is re-resolved without .npmrc auth, this step fails and silently produces no branch/issue.
[verified]
| }, | ||
| "dependencies": { | ||
| "@vscode/common-python-lsp": "^0.6.0", | ||
| "@vscode/common-python-lsp": "file:external/vscode-common-python-lsp/typescript", |
There was a problem hiding this comment.
📍 package.json:229
The Skeptic flags a new regression surface: converting @vscode/common-python-lsp from a registry dep to file:external/vscode-common-python-lsp/typescript makes the submodule mandatory for every npm resolution. Any npm ci/npm install that runs after actions/checkout@v4 without submodules: recursive now hard-fails (not just at postinstall — the file: target is absent). The author updated pr-check/push-check and the Azure pipelines, but a unified diff only shows changed files: grep the whole repo for npm ci/npm install and confirm every hosting job checks out submodules. Also confirm Dependabot still opens npm PRs, since it clones without submodule init and has no submodules: recursive equivalent. [unverified] — cannot execute Dependabot/full-repo grep in this workspace.
[verified]
|
The submodule-based approach is a reasonable redesign, but two things need attention before merge: (1) the PR description no longer matches the implementation and should be rewritten around the submodule mechanism, and (2) a handful of verified robustness issues in the sync workflow and install path (compat-guard sed parsing, dev-pruned postinstall build, mandatory-submodule file: dep, and lockfile re-resolution drift). |
Address reviewer feedback on the sync workflow: - Retry the branch-existence ls-remote and only treat a definitive 'no such ref' (exit 2) as absent, so a transient network error can no longer bypass the guard and clobber an existing branch. - Run the no-op detection before the compatibility guards so a re-dispatch with nothing to sync exits cleanly instead of failing loudly. - Compare the Python floor against the extension's declared minimum in src/common/constants.ts (not the bundling interpreter in runtime.txt), and the Node floor against the shared package's engines.node build floor (not the exact .nvmrc dev pin). - Treat an unparseable package floor as a warning instead of hard-failing every future sync. - Wrap 'npm install --package-lock-only' in the retry helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| run: | | ||
| set -euo pipefail | ||
| # Require a real dotted release so a branch/ref name can never be malformed. | ||
| if ! printf '%s' "${RELEASE_TAG}" | grep -Eq '^v?[0-9]+\.[0-9]+\.[0-9]+([-+][0-9A-Za-z.]+)*$'; then |
There was a problem hiding this comment.
📍 .github/workflows/shared-package-submodule-sync.yml:29
The Skeptic verified the semver validator matches per-line (grep -q), so "1.2.3\n<anything>" passes and carries a newline into VERSION/BRANCH/refs. Separately, a +build segment with .. (e.g. 1.2.3+a..b) passes the regex but yields an invalid git ref that fails git checkout -B/push loudly. Both are gated behind the repository_dispatch trust boundary (not RCE), but they defeat the prepare job's "a ref name can never be malformed" guarantee. Add git check-ref-format "refs/heads/${BRANCH}" after computing BRANCH (and/or match the full string, not per-line).
[verified]
| exit 1 | ||
| else | ||
| echo "Node versions compatible: extension ${EXT_NODE} >= shared package build floor ${PKG_NODE}." | ||
| fi |
There was a problem hiding this comment.
📍 .github/workflows/shared-package-submodule-sync.yml (tracking-issue step)
The Skeptic notes two robustness gaps in the tracking-issue step: (1) gh issue list --search "in:title ${TITLE}" with a bracketed [Shared Package] title can be tokenized so the search misses an existing issue, and any gh error is swallowed by 2>/dev/null || true → duplicate issue created; and (2) gh issue create/comment … || echo keeps the job green even when no branch reference reaches a maintainer (compare URL only in the job summary). Prefer a quoted "${TITLE}" in:title search (or list-and-jq filter) and surface a real failure rather than swallowing it.
[verified]
| # @vscode/common-python-lsp is handled by the shared-package-submodule-sync | ||
| # dispatch workflow, so ignore it here to avoid duplicate PRs. | ||
| - dependency-name: '@vscode/common-python-lsp' | ||
| - dependency-name: '@types/vscode' |
There was a problem hiding this comment.
📍 .github/dependabot.yml:20
The Architect and Skeptic both note the two new ignore entries now guard nothing: the npm @vscode/common-python-lsp is file: (Dependabot never bumps file: deps) and the pip vscode-common-python-lsp name is deleted from requirements.in/requirements.txt in this same PR. Harmless but dead config that misleads readers into thinking Dependabot would otherwise touch these. Either drop the entries or add a comment explaining they are defensive no-ops.
[verified]
|
The redesign (registry dep → git submodule built at postinstall) is reasonable, but before merge please (1) rewrite the PR description to match the actual submodule-sync mechanism, (2) confirm the VSIX/webpack build succeeds under a dev-pruned install since the shared package now builds tsc from source, and (3) tighten the auto-push lockfile step and the requires-python parsing in the sync workflow. |
Point external/vscode-common-python-lsp at the v0.8.1 release and refresh package-lock.json to keep the branch npm ci-mergeable. Python (>=3.10) and Node (engines.node >=18.0.0) compatibility floors are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
||
| # Determine whether the release actually moves the submodule before running | ||
| # the compatibility guards, so a no-op re-dispatch exits cleanly instead of | ||
| # doing extra work or failing loudly with nothing to sync. |
There was a problem hiding this comment.
The Python compatibility check's floor parse sed -n 's/.*requires-python[^0-9]*\([0-9]\+\.[0-9]\+\).*/\1/p' grabs the first major.minor after requires-python regardless of the operator, so an upper-bound-only spec like <3.14 yields floor 3.14 and could spuriously hard-fail a legitimate EXT_MIN=3.9 sync. Anchor the match on a >=/~= lower-bound operator and hard-fail if no lower bound is present, rather than grabbing the first number.
| # doing extra work or failing loudly with nothing to sync. | ||
| - name: Detect whether the submodule pointer moved | ||
| id: detect | ||
| if: steps.guard.outputs.exists == 'false' |
There was a problem hiding this comment.
In for CANDIDATE in "refs/tags/${RELEASE_TAG}" "refs/tags/v${VERSION}", a v-prefixed release_tag makes both candidates collapse to refs/tags/v1.2.3, so a submodule that tagged the release as a bare 1.2.3 is never probed → spurious "tag not found". Add refs/tags/${VERSION} as a third candidate so both v-prefixed and bare payloads probe both forms.
|
Overall this is a solid redesign — the submodule pointer + |
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR. |
2 participants
Summary
Two related changes so the shared package (
@vscode/common-python-lspnpm /vscode-common-python-lsppip) is updated via arepository_dispatchevent fromvscode-common-python-lspinstead of by Dependabot.1. New
.github/workflows/shared-package-release.ymlHandles
repository_dispatch(shared-package-release): branches offmain, bumps the npm dependency, rewrites the pip pin inrequirements.inand recompilesrequirements.txtwithuv, then pushes the branch and opens a tracking issue with a manual compare/PR link (org settings prevent the workflow from opening PRs automatically).2.
.github/dependabot.ymlMoves the shared package from the group
exclude-patterns(which still produced a standalone Dependabot PR) into the npm and pipignorelists, so Dependabot no longer opens duplicate PRs for it.