Repository navigation
fix: unify workspace path authorization and local refusal reporting - #1118
Conversation
XuPeng-SH
left a comment
There was a problem hiding this comment.
审查版本:ad85875164232fda2c501fef0d9ef2409757ae91。结论:当前需要修改,暂不建议合并。
从实际操作追踪了路径解析/拒绝/授权恢复、CLI 本地拒绝与失败结算、JSON/stream-json 输出、harness 空事件证据和多轮失败。路径授权归入 astra-sandbox、移除旧共享校验、复用既有 admission 事实的方向合理;没有发现新增数据库查询、写入或轮询,也不需要添加兼容分支。发现 3 个具体问题,见行级评论:相对工作目录的新文件误拒绝、未执行的恢复尝试清空工具限制、JSON 模式丢失 stderr 诊断。
验证与测试质量:
- 在隔离的合成目录中调用 sandbox 库,并对比基线/HEAD 的原始解析函数:
.根下已有文件成功,新文件在 HEAD 误报 BoundaryEscape;绝对根对照正常。所有新增路径测试使用绝对 TempDir 根,遗漏了这个边界。 - 新增本地拒绝结算测试使用默认空恢复状态,不能发现恢复限制被清空;现有
unstarted_initial_and_retry_preserve_input_and_recovery_state已明确要求未开始的尝试保留它们。 - CLI CI 实际运行 5018 项:5017 通过、1 失败。失败为
headless_hard_failure_preserves_only_the_admitted_root_identity,真实子进程的 chat-json/402 场景 stdout 有诊断而 stderr 为空,对应第 3 条评论。几个 astra-cli 红色检查共享这一次执行,不是多个独立故障。 - 另有一处测试迁移遗漏:Runtime CI 6050 项中 6049 通过、1 失败。
crates/runtime/src/server/runtime_tool_executor.rs:14032仍以../../etc/passwd同时触发敏感路径和越界规则,却只断言SANDBOX_DENIED。共享校验现在优先拒绝敏感路径,需像本 PR 已修改的 tools 测试一样,用非敏感的外部合成文件单独验证遍历拒绝,并保留独立的敏感文件拒绝断言。不要把这次失败解释成已经发生了文件泄露,也不要只放宽成任意错误。
文档增量与主路径契约基本一致;“尚未执行”语义需要落实到已有 NotStarted 结算职责,不能只在失败分支提前返回。此次未重跑全部本地测试,复用了既有验证记录和本次真实 CI 日志,并新增上述隔离反例核验;工作树未改动。
审查开始时,该 PR 及 #1115/#1116 没有 GitHub review/comment 可逐条核销。此前本地审查的机器事件清理、后续轮次拒绝分类、原始 terminal 字段校验三项已能看到对应修复,但仍需处理本轮问题及上述 CI 失败。
bd1579d to
b4fb38d
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
审查版本:b4fb38d279353b77fc86beb068f1c7cad1774812,基线 3d8af2edd206be430c89b649bc4657a46f5155f8。
结论:本轮未发现新增 P1/P2 或其他合并阻塞项。已有三条 review 评论均有实质修复;不是仅凭线程 resolved 或 CI 绿色判定。
设计与用户体验
- #1115 的修复放在
astra-sandbox::validate_path:相对输入只能落在工作区,绝对输入可使用既有授权。共享文件工具删除自己的安全解析路径,CLI 删除重复 canonicalize guard;显示相对路径和文件变更绑定保留各自职责。没有新增授权状态机或存储。 - 缺失路径逐组件解析保留 symlink/
..顺序;相对根.从 canonical cwd 起步。已有文件、新文件、深层新目录、内部/外部 symlink、悬空/循环链接及绝对授权对照均有针对性控制。相对越界不能靠 temp/additional grants 放行,明确的绝对外部访问仍可按权限处理。 - #1116 保留 typed
tool_unavailable/workspace_host_unsupported和 exit 1。writer-capable surface 仍 fail closed;无工具/只读 surface 无需 writer host。修复提示指向 host 前置条件,避免把本地环境问题错误呈现成 API 故障。 - 非派发失败进入既有
TurnAttempt::NotStarted,显式取消不触发认证重试;draft、恢复提示、诊断、工具限制、历史和 cursor 不应被未执行尝试消耗。已派发/不确定的请求保留现有失败、取消和持久化责任。 - harness 只有严格终端 envelope、OS exit 和空事件文件一致且无执行证据时才保留本地失败。缺失/损坏/矛盾的证据仍为 protocol failure。后续轮次拒绝不会把先前实际执行的根轮次整体免于证据收集和清理,也不会继续后续步骤或调用语义 judger。
旧评论核销
| 评论 | 当前核验 |
|---|---|
缺失相对路径丢 cwd/前导 .. |
已在 canonical resolver 修复;新增 for_project(".") 新文件、多级目录、前导父路径和绝对根对照。 |
| 未执行恢复尝试清空限制 | 删除重复结算分支,复用 NotStarted;真实 host preflight → producer → settlement 测试带恢复限制/提示/诊断,另有初次、重试、取消状态矩阵。 |
| JSON 模式缺 stderr 原因 | 诊断归到共享 failure-reporting owner,在格式分支前输出;真实 cli_pipe 覆盖 text、chat-json、print-json、stream-json,保留 exit 3 并检查 JSON 模式只打印一次。 |
增量与工程质量
按测试模块/文件边界分别检查:实现 +390/-251,测试 +737/-21,文档 +30/-1。新增实现复杂度主要用于保留 typed failure、严格证据校验和已有生命周期接入;没有兼容分支、新数据库查询/写入/轮询或第二套执行路径。
测试不是只验证自造终端:有公开文件 executor 的拒绝且不修改外部数据、真实 turn producer/settlement、真实 CLI 子进程;shell shim 用于构造真实 CLI 不应产生的坏证据。Runtime 的遍历与敏感文件夹具已拆开,redaction 测试改用普通文件但保留合成秘密内容,没有简单放宽成任意错误。没有发现必须再抽象一层的重复搭建。
缺失路径回退增加逐组件文件系统解析,用于解决真实符号链接/父路径正确性;已有目标仍有 canonicalize 快路径。未新增数据库负担。未执行性能基准,不作性能提升结论。
验证与限制
本轮为只读源代码/契约/评论审查,未修改代码、未启动 subagent、未重复构建或重跑已验证测试。复用完全相同当前代码树的验证:
- 最终 Test Suite:28 个 job 成功;integration 297/297,原超时 fanout 用例本次 12.278 秒;必需 online aggregate 成功。
- 最终静态检查:lint/check 成功,SDK/web 按无影响跳过。
- live harness:第一次在任何用例开始前发现已注册模型 inactive;单次受控重跑 preflight 和 18/18 用例成功。首次前置条件失败的上游原因/terminal-protocol 细节未确认。
- 既有修复验证包含 2399 项相关 UT/公开入口检查,作为补充;本轮未把它们称为新执行结果。
原 60 秒超时只是未复现,不能宣称已找到根因或修复竞态;本次没有放宽 timeout/retry/oracle。路径检查与实际文件访问间已有 TOCTOU 限制仍存在,本 PR 不应被描述成 descriptor-based race elimination。以上是证据边界,不是新发现的 PR 回归。
Capability assessment: Unchanged: filesystem isolation and local CLI execution retain their existing provider/profile boundaries; this fixes authorization and failure reporting within supported paths. No new capability or deployment profile is introduced.
Summary
.also resolves new files correctly.tool_unavailable/workspace_host_unsupported, non-retryable metadata and process/JSON exit1, before Server admission. Restore the draft and preserve recovery guidance/tool restrictions until an actual attempt starts.Related issue
Closes #1115
Closes #1116
Change type
User and compatibility impact
Relative traversal and relative external symlinks cannot use broad temp-prefix or additional-directory grants. Use an explicitly authorized absolute input when external access is intended. Shared filesystem tools also enforce the canonical never-readable credential-file rule; redaction still applies to credential-shaped contents in ordinary files. Explicit Permissive behavior retains its existing sensitive-file exclusions.
Unsupported local coordination hosts still fail closed. Human/JSON diagnostics now identify the local prerequisite accurately; human error reasons remain on stderr with text, JSON and stream-json stdout. No compatibility shim, new exit status, provider call or database migration is introduced.
Architecture and complexity delta
astra-sandboxresolves and authorizes paths; existingClassifiedError, host error metadata and sticky dispatch fact carry local failures; the existing strict harness terminal parser owns evidence validity.NotStartedsettlement and retain recovery state; the duplicate local-refusal settlement branch is removed. Local refusals skip execution journal/usage/accounting and harness execution capture/session cleanup/digest/judging.Verification
Pre-rebase repair validation: 2399 tests pass; 16 preexisting tool tests remain ignored. The full PR delta remains byte-identical after rebase onto
3d8af2edd.Built test targets with
cargo test --locked -p astra-cli -p astra-sandbox -p astra-tools -p astra-test-harness -p astra-runtime --lib --test cli_pipe --no-run. Ran the resulting feature-unified test executables with--test-threads=2.Full sandbox: 216 pass. Full tools: 1014 pass / 16 existing ignored. Full test harness: 628 pass, including cross-process workspace ownership, event drain, cancellation, cleanup, follow-up refusal and raw-terminal integrity controls.
CLI: 503 pass across
cli::turn::(159),cli::chat_stream::sse_loop::(117),cli::command_router::exit_code_tests(39),edge_tools::fs_tools::tests(127),sandbox_retry::(40), andtests::chat_stream_tests::(21).All 21 real
cli_pipesubprocess tests pass. Hard failures cover chat-text, chat-json, print-json and stream-json with/without admitted identity; JSON modes retain a single stderr diagnostic, correct exit 3, structured stdout and bounded identity authority. Existing signal/pipe controls pass.Runtime: 17 direct-consumer tests pass across read_file, write_file, str_replace and workspace-path guard. Traversal and credential refusal use independent synthetic fixtures with specific denial/no-disclosure assertions.
cargo clippy --locked -p astra-cli -p astra-sandbox -p astra-tools -p astra-test-harness -p astra-runtime --all-targets -- -D warnings: pass.cargo fmt --all -- --check,make toolchain-check,git diff --check: pass.Rust 1.97.0, build jobs 2, disk temporary/target directories. No shared database, live provider or host-permission change was used.
Required
gpt-6.1-sol/xhighdesign and overall source review completed; all material review findings are addressed and the final reviewed delta is bound to the current validation evidence. The rebase continuation also passed the required design and overall review; final-head Test Suite (all 28 jobs) and Static Checks passed. The separate live harness also passed on its single controlled rerun (18/18 cases).Public entrypoints: real CLI subprocess failures plus real turn execution/settlement under the existing typed host-failure injection. Mock catalog only, no admission/refresh POST or Run/tool records; draft, recovery guidance, restrictions, cursor and history remain intact. Public file executor read/write/edit refusals preserve target bytes/directories.
Unhappy paths: traversal despite temp/additional grants, symlink component order and missing tails, dangling/loop links, strict/standard/permissive controls, relative
.workspace roots, initial/retry/cancelled unstarted recovery, admitted/ambiguous failure controls, malformed/missing/contradictory machine evidence and process-status mismatches.Database impact: no schema, query, transaction, polling or provider-call additions. Original boundary tests use offline fixtures. For CI timeout diagnosis, three focused fanout integration executions used a separate, owned MatrixOne instance and in-process mock Memoria/scripted model providers; no existing development service or external model was used.
Race applicability: safe synchronous filesystem checks and typed projections, exclusively borrowed SessionState, and no unsafe or synchronization-protocol change; existing observer synchronization ownership is unchanged. Rust TSan cannot establish filesystem TOCTOU safety. Deterministic subprocess/evidence-drain and cross-process workspace controls provide applicable validation. Descriptor-based race elimination is outside this change.
CI and rebase continuation
3d8af2edd; range-diff preserves both repair commits exactly. Rebased headb4fb38d27has tree99791738e9fe3005d3dc6d5e28dba29a05ba29e3, identical to the prior CI merge checkout. Its successful static analysis, CLI/runtime/core and online core checks therefore already validate the same full source and dependencies.cargo fmt --all -- --checkandgit diff --checkpass. Final-head Static Checks also passed lint/check; unaffected SDK/web lanes were skipped. The separate live harness initially stopped before any case with a registered but inactive model. Its single controlled rerun passed preflight and all 18/18 cases, with complete coverage and exit 0. The provider cause and initial protocol failure are not established. All selected final-head checks now pass; SDK/web were skipped as unaffected.Final checklist