#Disable workspace inheritance branch

1 messages · Page 1 of 1 (latest)

limpid nacelleBOT
#

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 Workspace arg injection when the caller is a module runtime client (core/modfunc.go)
  • stop currentWorkspace binding 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.

sullen minnow
#

@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.

limpid nacelleBOT
# sullen minnow <@1506562465381744641> run a sub-agent to do an expert adversarial review. Pragm...

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

limpid nacelleBOT
# sullen minnow 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.

💻 🛠️ 📝

limpid nacelleBOT
#

workspace inheritance isolation branch

sullen minnow
limpid nacelleBOT
# sullen minnow 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=1
  • go test ./engine/server -run TestEnsureWorkspaceLoaded -count=1
  • git diff --check 0c55e40ba..652faf0f7

Local branch remains clean at 652faf0f7 on codex/workspace-disable-workspace-inheritance.

💻 🛠️

#

Workspace inheritance isolation branch

#

Disable workspace inheritance branch

sullen minnow
limpid nacelleBOT
# sullen minnow Would it be worth adding some tests in `./core/integration`? And if so, in which...

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/--workspace selection 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, in ModuleSuite
  • one Go fixture under core/integration/testdata/modules/go/runtime-workspace-isolation
  • subtests for the two real gates:
    1. A receives contextual workspace, but B’s dag.CurrentWorkspace() fails/no workspace.
    2. B’s required Workspace constructor/arg is not auto-filled when A calls B implicitly.
    3. Optional, if the SDK fixture is clean: A explicitly passes its own Workspace to B and B succeeds.

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.