Skip to content

docs: add architecture review record for issue 217#220

Closed
fennoai[bot] wants to merge 2 commits into
mainfrom
xgopilot/issue-217-1775819552
Closed

docs: add architecture review record for issue 217#220
fennoai[bot] wants to merge 2 commits into
mainfrom
xgopilot/issue-217-1775819552

Conversation

@fennoai

@fennoai fennoai Bot commented Apr 10, 2026

Copy link
Copy Markdown
Contributor

Requested by @Cai-Tang-www

Summary

  • add docs/architecture-review-issue-217.md
  • record the architecture review findings gathered for issue 查错查架构 #217
  • keep the current branch scope limited to documentation only

Verification

  • not run (docs only)

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
@fennoai fennoai Bot mentioned this pull request Apr 10, 2026
@codecov

codecov Bot commented Apr 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 18 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/runtime/runtime.go 54.54% 5 Missing and 5 partials ⚠️
internal/runtime/permission.go 82.60% 2 Missing and 2 partials ⚠️
internal/tools/manager.go 77.77% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

@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 a few documentation-accuracy issues where the review record overstates the current code evidence or mischaracterizes behavior.


现状:

- `Gateway` 与 `RuntimePort` 只定义了契约,没有看到对应实现或被主链路消费。

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 section currently treats internal/gateway as if it were only an unused abstraction layer, but the package also contains concrete protocol assets in types.go, validate.go, and errors.go with coverage in internal/gateway/*_test.go. If the concern is specifically the unused Gateway/RuntimePort contracts, please scope the criticism to that port/interface layer; otherwise the recommendation to delete the whole package is broader than the code here supports.


- `buildToolManager` 正常路径会注入 `security.NewWorkspaceSandbox()`,这条链路是合理的。
- 但 `tools.NewManager` 在 `sandbox == nil` 时会回退到 `NoopWorkspaceSandbox`。
- 该实现的 `Check` 直接返回 `nil, ctx.Err()`;在上下文正常时等价于“不做任何工作区限制”。

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.

NoopWorkspaceSandbox is weaker than the real workspace sandbox, but this sentence overstates the effect. With call.WorkspacePlan == nil, the tools still enforce workspace-root boundaries through ResolveWorkspaceTarget and per-tool path/workdir checks, so this is not equivalent to unrestricted execution. The more accurate risk is loss of the extra centralized hardening in security.NewWorkspaceSandbox() such as canonical-root, symlink-escape, and execution-time revalidation checks.


现状:

- `s.emit(...)` 在运行主循环中多次调用,但大多数返回值没有被处理。

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 wording is a bit too broad. emit does not drop events just because the channel is full; it retries and only returns an error when the run context is canceled before the send can complete. Narrowing this section to cancellation/backpressure-induced failures would make the runtime analysis more precise.

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
@fennoai fennoai Bot deleted the xgopilot/issue-217-1775819552 branch April 10, 2026 13:00
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