Skip to content

fix: register Dev Container Agent Host settings via idempotent helper (build fix for vscode-engineering#3738) - #333815

Closed
VS Code PR Bot (vscodebot-pr) wants to merge 1 commit into
microsoft:mainfrom
vscodebot-pr:fix/devcontainer-agenthost-setting-test-isolation-aw-33535250329
Closed

fix: register Dev Container Agent Host settings via idempotent helper (build fix for vscode-engineering#3738)#333815
VS Code PR Bot (vscodebot-pr) wants to merge 1 commit into
microsoft:mainfrom
vscodebot-pr:fix/devcontainer-agenthost-setting-test-isolation-aw-33535250329

Conversation

@vscodebot-pr

Copy link
Copy Markdown
Contributor

Build failure

The Electron unit-test job failed on Linux, Windows, and macOS with a single failing test:

Dev Container Agent Host Connector
  registers an experimental, disabled-by-default user setting:
  TypeError: Cannot read properties of undefined (reading 'default')

The test reads getConfigurationProperties()[DevContainerAgentHostEnabledSettingId] and the returned property is undefined, so accessing .default throws. Because the unit-test job exits before writing .build/logs, downstream tasks fail only on the missing path and are not independent problems.

Root cause

The Dev Container Agent Host settings were registered once, unconditionally, as a side effect of module import in devContainerAgentHostConnector.contribution.ts. The IConfigurationRegistry is a process-global singleton shared across the whole unit-test run. Other suites call deregisterConfigurations on that shared registry (e.g. test isolation/teardown), which removes these properties from getConfigurationProperties(). Once removed, they are never re-registered because the module was already imported and its top-level registration ran only once. When the registers an experimental, disabled-by-default user setting test then reads the property back, it is gone, and property.default throws.

This is a repository-local defect in microsoft/vscode, still present on main, and directly caused by the setting registration/test introduced in commit f83aa9cd (#333782).

How the fix works

The top-level registration is extracted into an exported, idempotent registerDevContainerAgentHostConfiguration() helper. The module still calls it once on import to preserve existing production behavior. The test suite adds a suiteSetup that re-registers the configuration only when it is missing from the shared registry, making the suite resilient to registry deregistration performed by other suites. This restores the property before the assertions read it, without changing runtime behavior or suppressing any real error.

Rollback evaluation

  • Recommendation: Do not roll back.
  • Public culprit commit/PR: f83aa9cd40904b8a52333089558d6fe40f43fde7 (Agent Host: Gate Dev Container worktree sessions #333782).
  • Rationale: Impact is limited to a single new experimental Dev Container test on main insider. A bounded fix-forward that makes the setting registration idempotent and the test isolation-safe is smaller and safer than unwinding the four-file gating change and its dependent worktree work.
  • Owners to consult: @chrmarti

Validation

Unable to run the unit test in this environment: npm ci / build tooling could not be executed here (dependency install and git/build operations were unavailable). The change is verified by source inspection: the setting is now re-registered before the suite runs whenever the shared IConfigurationRegistry no longer contains it, so getConfigurationProperties()[DevContainerAgentHostEnabledSettingId] is defined when the assertions execute.

Risk

Low. Production behavior is unchanged (the helper is still invoked exactly once on import). The added suiteSetup only re-registers when the property is absent, so it cannot double-register during a normal run.

Recommended reviewer

Recommended owner: @chrmarti

Fixes microsoft/vscode-engineering#3738

Generated by build-fix · opus48 · 372.9 AIC · ⌖ 13.4 AIC · ⊞ 11.7K ·

… (build fix for vscode-engineering#3738)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Extracts Dev Container Agent Host setting registration to recover from shared registry resets during unit tests.

Changes:

  • Adds a reusable configuration registration helper.
  • Restores missing settings during test-suite setup.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
devContainerAgentHostConnector.contribution.ts Extracts setting registration into a helper.
devContainerAgentHostConnector.test.ts Re-registers missing configuration before tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@bryanchen-d

Copy link
Copy Markdown
Collaborator

Closing as superseded by #333811 (commit 0352a616). That merged maintainer change fixes the same test-order failure by capturing the registered configuration properties before other suites clear the global registry, and its Electron-unit checks passed on Linux, macOS, and Windows. This alternative also has an unresolved design flaw in the proposed exported helper, so it should not be revived.

Co-authored with Copilot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

vscode-build VS Code build process issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants