Skip to content

refactor(workspace): rebuild runtime bundle across /cwd changes#216

Closed
fennoai[bot] wants to merge 7 commits into
mainfrom
xgopilot/codex/issue-214-1775802178
Closed

refactor(workspace): rebuild runtime bundle across /cwd changes#216
fennoai[bot] wants to merge 7 commits into
mainfrom
xgopilot/codex/issue-214-1775802178

Conversation

@fennoai

@fennoai fennoai Bot commented Apr 10, 2026

Copy link
Copy Markdown
Contributor

Requested by @Yumiue

This PR implements the /cwd cross-workspace rebuild refactor from #214.

Summary

  • add internal/workspace for unified path normalization and Git-root-aware workspaceRoot resolution
  • bind runtime bundles to a single workspaceRoot + workdir, and route same-workspace updates through UpdateSessionWorkdir
  • rebuild TUI dependencies in-process on cross-workspace /cwd, reset to a clean draft, and refresh sessions/files/providers/models
  • pin tool sandbox/session-store scope to workspaceRoot while keeping execution workdir independent
  • add runtime, app bootstrap, workspace, tools, and TUI tests for same-workspace updates and cross-workspace rebuilds

Verification

  • gofmt -w ./cmd ./internal
  • go test ./internal/tui/core/app ./internal/app ./internal/runtime ./internal/workspace
  • go test ./...

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Yumiue <188874804+Yumiue@users.noreply.github.com>
@codecov

codecov Bot commented Apr 10, 2026

Copy link
Copy Markdown

@wynxing

wynxing commented Apr 10, 2026

Copy link
Copy Markdown
Collaborator

/code 补充测试

1 similar comment
@wynxing

wynxing commented Apr 10, 2026

Copy link
Copy Markdown
Collaborator

/code 补充测试

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I found 2 regressions in the workspace rebuild refactor.

@@ -73,11 +92,14 @@ func Build(options Options) (Container, error) {
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Build now forwards options.WorkspaceRoot/options.Workdir verbatim, but tui.New(...) still calls NewWithBootstrap(...) without setting either field. That leaves App.workspaceRoot empty for callers using the legacy constructor, so /cwd always looks like a cross-workspace switch and immediately fails with workspace rebuild is not configured. Please preserve the old constructor by defaulting these fields from the resolved config snapshot when the option is empty.

Comment thread internal/tui/core/app/update.go Outdated
if cmd := a.requestModelCatalogRefresh(a.state.CurrentProvider); cmd != nil {
cmds = append(cmds, cmd)
}
cmds = append(cmds, ListenForRuntimeEvent(a.runtime.Events()))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This starts a new runtime listener after every cross-workspace rebuild, but the old listener is never cancelled. Because ListenForRuntimeEventCmd blocks on the previous channel until it emits or closes, each rebuild leaves a goroutine waiting on the obsolete runtime, and any late event/close from that runtime can still mutate the new UI state. The listener needs an identity/cancellation mechanism when rebinding the app to a new runtime.

@wynxing

wynxing commented Apr 10, 2026

Copy link
Copy Markdown
Collaborator

/simplify

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Found 1 issue worth addressing.

Comment thread internal/tui/core/app/update.go Outdated
a.state.ExecutionError = ""
a.state.StatusText = statusApplyingCommand
a.appendActivity("workspace", "Rebuilding workspace bundle", typed.Workdir, false)
cmds = append(cmds, runWorkspaceRebuild(a.rebuildWorkspace, typed.Notice, typed.Workdir))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

runWorkspaceRebuild is dispatched asynchronously, but the app never enters a distinct "rebuilding" busy state. Between this return and the later workspaceRebuildFinishedMsg, the user can still submit another prompt or trigger another /cwd, so the old runtime keeps accepting work and the eventual rebuild result then hard-resets the UI back to a fresh draft. This creates a real race/regression for cross-workspace switches. Please block input while a rebuild is in flight, or tag/cancel stale rebuild results before applying them.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I found two compatibility regressions in the new workspace-binding flow.

Comment thread internal/tui/bootstrap/builder.go Outdated
ConfigManager: options.ConfigManager,
Runtime: runtimeSvc,
ProviderService: providerSvc,
WorkspaceRoot: options.WorkspaceRoot,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Build now forwards WorkspaceRoot/Workdir/RebuildWorkspace exactly as provided, but the preserved tui.New(...) wrapper still constructs Options with only the old fields. In that path WorkspaceRoot stays empty and RebuildWorkspace is nil, so /cwd <path> is misclassified as a cross-workspace switch (SameRoot("", resolved.WorkspaceRoot) is always false) and then fails with workspace rebuild is not configured. Please derive sane defaults here or wire the compatibility wrapper so the legacy constructor still supports same-workspace /cwd.

if configManager != nil {
workdir = strings.TrimSpace(configManager.Get().Workdir)
}
return NewWithWorkspace(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

NewWithFactory is documented as falling back to the current config workdir for the workspace root, but it passes "" as workspaceRoot. Any caller that still uses this compatibility constructor skips the new root-boundary checks in resolveSessionWorkdir and tool execution, so it can move a session outside the original workspace instead of forcing a rebuild. Passing workdir here would preserve the previous single-workdir behavior and match the comment.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I found 3 issues in the workspace-switching refactor: a symlink-based workspace escape, a compatibility regression in the legacy tui.New(...) path, and stale runtime subscriptions after cross-workspace rebuilds.

target := strings.TrimSpace(requestedWorkdir)
if !filepath.IsAbs(target) {
target = filepath.Join(base, target)
if strings.TrimSpace(s.workspaceRoot) != "" && !isWithinWorkspaceRoot(s.workspaceRoot, resolved) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

isWithinWorkspaceRoot only does a lexical filepath.Rel check here. A repo-local symlink such as repo/link -> /tmp/outside still passes, so /cwd link keeps the old workspaceRoot instead of forcing a rebuild/reject. After that the TUI's file browser and local & commands run against the symlink target outside the workspace. This path needs canonicalization (or the same sandbox validation you use for tool execution) before accepting the new session workdir.

@@ -73,11 +92,14 @@ func Build(options Options) (Container, error) {
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This leaves WorkspaceRoot, Workdir, and RebuildWorkspace empty for callers that still use the exported tui.New(...) compatibility entry point. In that path workspaceRoot starts empty, so /cwd <path> is always treated as a cross-workspace switch and then fails with workspace rebuild is not configured. The compatibility wrapper needs to seed these fields from the existing config/runtime inputs, otherwise this PR breaks the documented old API.

Comment thread internal/tui/core/app/update.go Outdated
if cmd := a.requestModelCatalogRefresh(a.state.CurrentProvider); cmd != nil {
cmds = append(cmds, cmd)
}
cmds = append(cmds, ListenForRuntimeEvent(a.runtime.Events()))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

After rebinding you start listening to the new runtime, but the pending ListenForRuntimeEvent command that was already blocked on the old runtime is never cancelled, and runtime.Service never closes its event channel. Rebuilding a few times will leak blocked listeners, and any late event from the old runtime can still mutate the new app state. This needs a generation guard or an explicit teardown path for the old subscription.

@fennoai

fennoai Bot commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

已补充测试并推送到当前 PR 分支,提交为 5e9692a

结果

  • 补了 internal/workspace/resolver_test.go,覆盖空路径走当前目录和 SameRoot 空值边界。
  • 补了 internal/app/bootstrap_test.go,覆盖嵌套 workdir override 下 workspaceRoot/workdir 的绑定结果。
  • 补了 internal/tui/core/commands/workspace_test.go,覆盖活动会话跨工作区时只请求 rebuild、不直接更新 runtime。
  • 补了 internal/tui/core/app/workspace_rebuild_test.go 和新增 internal/tui/core/app/app_test.go,覆盖 legacy constructor 绑定、rebuild 成功/失败、依赖校验等分支。
  • 顺手修了 internal/tui/bootstrap/builder.go 的兼容默认值:旧 tui.New(...) 路径现在会默认回填 workspaceRoot/workdir,否则新增用例会暴露 /cwd 误判问题。

验证

  • go test ./internal/app ./internal/tui/bootstrap ./internal/tui/core/app ./internal/tui/core/commands ./internal/workspace
  • go test ./...

未做的事:没有直接查看外部 Codecov 结果,需要等 PR 上的新覆盖率回传。

View job run

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Yumiue <188874804+Yumiue@users.noreply.github.com>
@fennoai

fennoai Bot commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

Result

  • Read the historical comments and only addressed the remaining review findings that were still worth fixing in the current PR state.
  • Added cancelable runtime listener binding in TUI so cross-workspace rebuilds stop the old subscription, ignore stale runtime events / stale rebuild completions, and rebind to the correct runtime afterward.
  • Marked workspace rebuild as an explicit busy state so input is blocked while the bundle rebuild is in flight.
  • Bound runtime.NewWithFactory(...) to the config workdir as its default workspaceRoot, and made workspace containment checks symlink-aware.
  • Ran a simplification pass after the fixes and folded duplicated rebuild-failure handling into one helper; also normalized workspaceRoot once during runtime construction.

Validation

  • gofmt -w ./internal/runtime/runtime.go ./internal/tui/core/app/update.go
  • go test ./internal/runtime ./internal/tui/core/app ./internal/tui/core/utils ./internal/tui/services
  • go test ./...

Commit

  • Pushed e5c1ec2 to the current PR branch.

Not done:

  • I did not implement unrelated historical suggestions that were already fixed earlier or were outside the trigger comment scope.

View job run

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Yumiue <188874804+Yumiue@users.noreply.github.com>
@wynxing

wynxing commented Apr 10, 2026

Copy link
Copy Markdown
Collaborator

/simplify

@fennoai

fennoai Bot commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

Result

  • Read the repo instructions and the historical comments file. The earlier unresolved review comments in that log were already fixed on the current PR branch, so I only applied a small remaining follow-up fix that was still worth doing.
  • Fixed the cross-workspace rebuild flow so the app refreshes workspace file candidates and the file browser directory after rebinding to the new workspace, instead of carrying stale file state from the previous workspace.
  • Simplified the new binding path fallback logic by collapsing the repeated trim-and-default handling into one helper in applyWorkspaceBinding.
  • Added tests covering rebuilt workspace file state and empty binding path fallback behavior.

Verification

  • go test ./internal/tui/core/app ./internal/app ./internal/runtime ./internal/workspace
  • go test ./...

Commit

  • The branch already contains the simplifier follow-up as e707311.
  • git push origin HEAD reported Everything up-to-date.

Not done:

  • I did not implement unrelated historical suggestions outside the trigger scope, since the request was to evaluate unresolved comments and fix the ones still worth fixing.

View job run

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Yumiue <188874804+Yumiue@users.noreply.github.com>
@wynxing

wynxing commented Apr 10, 2026

Copy link
Copy Markdown
Collaborator

/code 补上测试

@fennoai

fennoai Bot commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

结果

  • 补了 internal/tools/manager_test.go,覆盖 buildPermissionActionbash 场景下优先使用 WorkspaceRoot 作为会话作用域、并在未显式传 workdir 时回退到当前执行目录的分支。
  • 补了 internal/workspace/resolver_test.go,覆盖 .git 为文件时的工作区根识别,以及 SameRoot 对当前目录相对路径的归一化比较。
  • 补了 internal/runtime/workdir_branch_test.go,覆盖缺失目录场景下 normalizeWorkspaceRoot 回退到绝对路径的分支。
  • 补了 internal/app/bootstrap_test.go,覆盖 resolveBootstrapWorkdir(".") 基于当前目录解析相对路径的分支。

验证

  • go test ./internal/app ./internal/runtime ./internal/tools ./internal/workspace
  • go test ./...

提交

  • 已推送 35a4ff1 到当前 PR 分支。

未做的事:没有改动任何生产代码,只补了这次 /cwd / workspace 绑定重构相关的测试缺口。

View job run

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Yumiue <188874804+Yumiue@users.noreply.github.com>
@wynxing

wynxing commented Apr 10, 2026

Copy link
Copy Markdown
Collaborator

/code 解决测试错误

@fennoai

fennoai Bot commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

结果

我先读取了仓库规则和这条 PR 的历史评论,再按当前分支状态直接复现测试。

本地未复现测试错误,当前 PR 分支在工作区里通过了以下验证:

  • go test ./...
  • go test ./... -count=1

补充确认:

  • 当前分支 HEAD35a4ff1
  • 工作区干净,没有未提交改动。
  • 这次没有做代码修改,因此也没有新的 commit / push。

未做的事:

  • 没有盲改代码,因为现有分支下没有复现失败用例。
  • 无法直接查看外部 CI 具体报错日志;如果你指的是某一条 CI 失败,请把失败包名、测试名或报错栈贴出来,我再按那条失败继续定位。

View job run

@wynxing

wynxing commented Apr 10, 2026

Copy link
Copy Markdown
Collaborator

/code 解决冲突

@fennoai

fennoai Bot commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

结果

  • 已把这次和 origin/main 真正冲突的两处内容拆开处理并推送到当前 PR 分支,提交为 7c15eff
  • docs/session-persistence-design.md 对齐到了主干当前版本,避免这份文档继续在同一段落上冲突。
  • internal/config/config_test.go 去掉了和主干同文件尾部的测试堆叠冲突,把 selection.go 相关工具函数覆盖独立挪到新文件 internal/config/selection_helpers_test.go,这样既保留当前分支测试覆盖,也让后续合并不再卡在 delete/modify 冲突。
  • 额外用临时三方合并验证了当前工作树内容:docs/session-persistence-design.mdinternal/config/config_test.go 再和 origin/main 合并时都不会产出冲突标记。

验证

  • go test ./internal/config ./internal/app
  • go test ./...

未做的事:

  • 没有顺手并入 origin/main 上其他与本次“解决冲突”无关的 provider / TUI 重构,只处理了当前 PR 可合并所需的冲突面。

View job run

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Yumiue <188874804+Yumiue@users.noreply.github.com>
@wynxing

wynxing commented Apr 10, 2026

Copy link
Copy Markdown
Collaborator

/code 修复以通过cli测试检查, 现在build_test错误,可能还有

@fennoai

fennoai Bot commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

结果

  • 已修复 internal/app 里一组真实可复现的测试问题,并推送到当前 PR 分支,提交为 ad1bd68
  • 问题根因是 buildMCPRegistry / buildToolRegistry 相关测试会在 t.Parallel() 下共享并覆盖全局 MCP bootstrap hook,导致并发读写 data race;这类问题在不同执行顺序下容易表现成不稳定的 build-test 失败。
  • 处理方式是把 internal/app/mcp_bootstrap.go 中测试使用的 MCP stdio hook 改成原子读取,并把 internal/app/bootstrap_test.go 中的测试替换为原子 store/restore 的覆盖方式,避免并行测试互相踩全局状态。

验证

  • go test ./internal/app -race
  • go test ./...
  • go build ./...

未做

  • 本地仍未直接复现你提到的那条外部 CI 报错日志,因为当前工作区常规 go test ./... 本来就是通过的;这次修的是我在本地确认能稳定打出来的测试并发问题。
  • 没有扩散改动到 internal/app 之外的模块,也没有做与这次测试稳定性无关的重构。

View job run

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Yumiue <188874804+Yumiue@users.noreply.github.com>
@fennoai

fennoai Bot commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

结果

  • 已读取仓库规则、PR 历史评论和当前 CI 工作流,按 build-test 的实际命令复现检查。
  • 当前 PR 分支 ad1bd68 在本地没有再复现新的 build_test 失败;我也继续补做了更接近 CI 的稳定性检查,结果都通过。

已验证

  • go build ./...
  • go test ./...
  • go test ./... -count=1
  • go test ./... -covermode=atomic -coverprofile=/tmp/coverage.out
  • go test ./internal/app -race
  • go test ./internal/app -count=20 -shuffle=on

结论

  • 这次没有再做代码修改,也没有新的 commit / push,因为当前分支下我无法稳定复现新的失败。
  • 目前最像 build_test 的已知问题,就是这条分支上刚修过的 internal/app 全局 MCP hook 并发踩踏;该修复已经在 ad1bd68 里。
  • 如果你说的是 GitHub 上仍然失败的某一条 CLI / build-test,请直接贴失败包名、测试名或报错栈;没有外部 CI 详细日志时,我这边再继续盲改的信号不够。

未做的事:

  • 没有凭猜测继续改生产代码或测试代码。
  • 没有提交新变更,因为现有工作区验证均通过。

View job run

@wynxing wynxing closed this Apr 10, 2026
@fennoai fennoai Bot deleted the xgopilot/codex/issue-214-1775802178 branch April 10, 2026 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants