-
Notifications
You must be signed in to change notification settings - Fork 134
feat(workspace): attach the bound workspace's engine as a derived MCP overlay #1167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ralphstodomingo
wants to merge
13
commits into
main
Choose a base branch
from
feat/workspace-engine-overlay
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
c9ae8aa
feat(workspace): attach the bound workspace's engine as a derived MCP…
3022613
fix(workspace): count only declared tools in the inventory line
73c5180
fix(workspace): keep overlay state per directory and the retry budget…
42f1f8d
fix(workspace): guard the v2 MCP add endpoint; hand the key back on u…
96194a9
fix(workspace): run turn hooks for one directory one at a time
7169ea7
fix(workspace): hand the key back after unlinking a refused overlay too
d0ebb1b
fix(workspace): fix a turn's engine for the whole turn
7895003
fix(workspace): run the turn boundary on the first catalog, not the f…
72dcae1
fix(workspace): close the review findings on the engine overlay
b2e5c6a
fix(workspace): guard connect/disconnect; keep the engine through a t…
7632549
fix(workspace): refuse the managed key in /mcps; fail closed on a rel…
52cbd41
fix(workspace): identify a workspace by credential scope and id, not …
4d2dae6
fix(workspace): keep the key owned while a retained engine runs; one …
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Major — required to merge (#1 of 4). Violates C5 and C2.
datamate_manager addstill reaches the managed key whennameis passed explicitlyThis looks like a regression from the bot-round-2 fix. The guard used to key off the resolved server name:
Moving it ahead of the API lookup — so the refusal would not depend on the API being reachable, which is the right goal — meant
serverNamewas not yet computed, since it depends ondatamate.namefrom the API response.transport !== nullwas substituted as a proxy, and the proxy is not equivalent to the thing it replaced.args.nameis a free-text tool argument the model chooses. With no IDE transport andname: "datamate", line 234 resolvesserverNametoDATAMATE_KEY, this guard never runs, and the standalone branch executes:That writes the key to a config file (C2: "nothing is written to any config file") and replaces the running workspace engine with a hosted cloud entry mid-turn (C5) — the "answers for the workspace with tools it did not declare" case the design rules out.
handleRemovestill guards correctly onargs.server_name === DATAMATE_KEY; onlyhandleAddlost it.E2E row 9 exercises the IDE-transport path, which is why this survived.
Fix — keep the guard ahead of the API lookup, but cover both routes to the key:
Worth a test with an explicit
name: "datamate"and no IDE transport — the current suite covers the IDE-transport path only, which is what let the substitution through.