#Disable workspace inheritance branch
1 messages · Page 1 of 1 (latest)
Done. I created codex/workspace-disable-workspace-inheritance from origin/workspace (b1caab7ff) and committed the handoff doc as f579bb5e8 (docs: handoff workspace inheritance fix).
Doc: future/workspace-disable-inheritcance.md
Key result: the handoff calls out both required fixes, because either one alone is incomplete:
- block omitted
Workspacearg injection when the caller is a module runtime client (core/modfunc.go) - stop
currentWorkspacebinding inheritance when a runtime child would inherit through a module parent (engine/server/session_workspaces.go)
Repo status is clean. I ran git diff --cached --check before committing; no Go tests were run because this turn only added the design handoff.
💻 🛠️ 📝
Workspace inheritance branch for dagger/dagger
Disable workspace inheritance branch
Implementation committed at 652faf0f7 on codex/workspace-disable-workspace-inheritance. Second adversarial sub-agent review found no blocking code issues; residual risks are missing full A->B generated-SDK integration regression and the intentionally narrow module-runtime-only inheritance boundary. Focused Go tests and diff check passed; working tree clean.
@limpid nacelle run a sub-agent to do an expert adversarial review. Pragmatic, tough but fair. No over-engineering for the sake of it. Is this mergeable by the project's standard? Is this is the smallest diff that gets the job done? Does it introduce useless indirections? Does it preserve DRY? Is it easy to maintain or does it require arcane implicit context? Go.
Adversarial review done. Verdict: mergeable as a handoff doc after one fix; not an implementation PR yet.
The sub-agent found one real issue: the doc used two subtly different predicates for “module runtime client”: Step 1 used CurrentModule/client.mod, while Step 2 suggested client.mod || client.fnCall. That is exactly the kind of implicit context drift that would become maintenance debt or a boundary hole. I agreed and amended the doc to align on client.mod / CurrentModule, with a warning not to use fnCall server-side unless the core-side gate also checks CurrentFunctionCall.
Current branch: codex/workspace-disable-workspace-inheritance
Commit: 0c55e40ba docs: handoff workspace inheritance fix
Status: clean
Tough read: the design is small and not over-engineered. It correctly identifies both necessary gates: call-time omitted Workspace injection and runtime-client currentWorkspace inheritance. The diff is just one handoff doc; no useless abstractions in the branch. For the future implementation, the main project-standard requirement is to keep the predicate single-source and add the two regression tests called out in the doc. No tests run, since this remained a doc/review pass.
💻 🛠️ 📝
Workspace inheritance branch review
Workspace inheritance branch for dagger/dagger
Now implement it
Implemented and committed on codex/workspace-disable-workspace-inheritance.Commit: 652faf0f7 fix: isolate workspace from module dependenciesWhat changed:- core/modfunc.go: omitted Workspace arg injection now refuses module-runtime callers with an explicit “pass a Workspace explicitly” error.- engine/server/session_workspaces.go: currentWorkspace inheritance now stops only for module runtime -> module runtime inheritance, preserving non-module nested-client inheritance.- Added focused regressions in core/modfunc_test.go and engine/server/session_test.go.- Updated future/workspace-disable-inheritcance.md to match the implemented predicate and test coverage.Verification passed:- go test ./core -run TestModuleFunction -count=1- go test ./engine/server -run TestEnsureWorkspaceLoaded -count=1- git diff --checkWorking tree is clean. I also tried to build a CLI integration fixture, but the current workspace branch’s nested workspace-module CLI path fails before reaching this behavior, so I did not land a brittle integration test. The doc now calls that out as future optional coverage once that harness is stable.
💻 🛠️ 📝
workspace inheritance isolation branch
Run another adversarial review subagent
Second adversarial review completed.
Verdict: no blocking findings. Bacon says this is mergeable by project standards if maintainers accept two explicit residual risks: no full end-to-end A->B generated-SDK regression yet, and the boundary is intentionally narrow: block module-runtime -> module-runtime workspace inheritance while preserving non-module nested-client inheritance.
Verification rerun by the reviewer:
go test ./core -run TestModuleFunction -count=1go test ./engine/server -run TestEnsureWorkspaceLoaded -count=1git diff --check 0c55e40ba..652faf0f7
Local branch remains clean at 652faf0f7 on codex/workspace-disable-workspace-inheritance.
💻 🛠️
Workspace inheritance isolation branch
Disable workspace inheritance branch
Would it be worth adding some tests in ./core/integration? And if so, in which file? Be very mindful of the answer, we want to preserve the integrity of the test suite overall - it should all be easy to read and navigate, to keep clean and avoid blind spots in coverage from sheer complexity and messiness.
Yes, it would be worth adding integration coverage, but only if it stays very small and sits with the module dependency runtime tests. I would put it in core/integration/module_dependency_runtime_test.go, not a new file.
Reasoning: the behavior under test is not workspace selection or the Workspace API itself. It is: module A runs, calls dependency module B, and B must not inherit A/client workspace authority unless A explicitly passes it. That is exactly the ownership of module_dependency_runtime_test.go, which already covers parent modules calling installed/local dependencies.
I would not put it in:
contextual_workspace_test.go: that owns client -> module workspace inference before module code runs.workspace_selection_test.go: that owns-W/--workspaceselection and host I/O consequences.workspace_api_test.go: that owns Workspace methods after a Workspace exists.- a new
workspace_inheritance_test.go: one behavior file would fragment the suite and make future readers guess where dependency-boundary coverage lives.
Shape I’d want:
- one test, maybe
TestRuntimeDependencyDoesNotInheritWorkspace, inModuleSuite - one Go fixture under
core/integration/testdata/modules/go/runtime-workspace-isolation - subtests for the two real gates:
- A receives contextual workspace, but B’s
dag.CurrentWorkspace()fails/no workspace. - B’s required
Workspaceconstructor/arg is not auto-filled when A calls B implicitly. - Optional, if the SDK fixture is clean: A explicitly passes its own Workspace to B and B succeeds.
- A receives contextual workspace, but B’s
I would skip landing this right now if it requires fighting the current workspace CLI/module harness or adding bespoke helpers. The existing unit tests are good guardrails; the integration test is valuable only if it reads like a normal A -> B module dependency scenario. If it needs a clever
… truncated; ask for the full reply.