Skip to content

Refactor/frontend#248

Merged
potter-sun merged 22 commits into
devfrom
refactor/frontend
Apr 20, 2026
Merged

Refactor/frontend#248
potter-sun merged 22 commits into
devfrom
refactor/frontend

Conversation

@potter-sun
Copy link
Copy Markdown
Collaborator

No description provided.

potter-sun and others added 18 commits April 16, 2026 10:53
…/2026-04-15_console-web-ui-refactor

# Conflicts:
#	apps/aevatar-console-web/src/pages/teams/home.tsx
…b-ui-refactor

Refine Studio workspace recovery and teams home semantics
…-fixes

Fix/2026 04 17 frontend review fixes
…into-frontend

Refactor/2026 04 20 merge dev into frontend
@eanzhao
Copy link
Copy Markdown
Contributor

eanzhao commented Apr 20, 2026

/review 结果:2 个 critical + 4 个 informational

整体方向 OK:前端围绕 create-team 草稿流和 scope query-param 做了一轮整理,后端 WorkspaceController 加了显式 scope 解析。但后端有两处需要先对齐再合。

🔴 CRITICAL

1. WorkspaceController.ResolveScopeContextAsync — auth 关闭时盲信 query param
src/Aevatar.Studio.Hosting/Controllers/WorkspaceController.cs:190-224

if (await IsAuthenticationEnabledAsync())
    return (null, Unauthorized(...));

return (new AppScopeContext(normalizedRequestedScopeId, "query:scopeId"), null);

IAuthenticationSchemeProvider.GetAllSchemesAsync() 返空(未注册任何 auth scheme),任何未认证调用方都可以把 ?scopeId=<任意值> 传进来,并被当成合法 scope 写/读 workflow。CLI 本地绑 localhost 时可以接受,但:

  • IsAuthenticationEnabledAsync 作为 "auth 是否强制" 的代理不可靠。scheme.NameAuthenticationScheme 契约必然非空,!string.IsNullOrWhiteSpace(scheme.Name) 这层过滤是多余的;更本质的是,"注册了 scheme" ≠ "endpoint 强制 [Authorize]"。
  • 如果 Studio Hosting 被部署到非 localhost 宿主(Kubernetes、studio 远端、demo 环境)且 auth 没正确配好,就是 scope 越权。

建议:换成显式配置开关(StudioHostingOptions.RequireAuthenticatedScope = true 默认开),或强绑 localhost binding 才启用 query fallback。"query:scopeId" 这个 source 字符串本身说明已经知道这是"弱信任"分支,不如把它做成需要显式 opt-in 的 dev mode。

2. AppScopedWorkflowService.SaveAsync — 拆掉 event-sourced 写路径
src/Aevatar.Studio.Application/AppScopedWorkflowService.cs:196-237

- ScopeWorkflowUpsertResult upsert;
- if (_workflowCommandPort != null) { ... upsert via IScopeWorkflowCommandPort ... }
- else { ... HTTP PUT /api/scopes/.../workflows/{id} ... }
+ await storagePort.UploadWorkflowYamlAsync(workflowId, workflowName, normalizedYaml, ct);

IScopeWorkflowCommandPort 还活着(ScopeWorkflowEndpoints 仍在用),只是从 Studio 的 Save 链路摘掉了。这意味着:

  • POST /api/workspace/workflows 不再触发 committed event → 没有 revision、没有 deployment、没有 read model 更新。
  • 返回值走 ToStoredWorkflowFileResponse,表面看像"草稿语义",但原端点契约是 workflow upsert。任何依赖 "save 后 workflow 就能被 runtime 执行" 的调用方会直接悄悄失活。

如果这是有意的 "save = 只写 draft,publish 才走 command port",请在 PR body / ADR 里把迁移路径写清楚,并至少加一个 test 证明 "save 后 list/get 能读到" + "runtime 执行要走另一条路径"。看了测试 SaveAsync_ShouldRewriteYamlNameFromRequestedWorkflowName 只断言 storage port 被写入,覆盖不到"旧调用方预期运行时立即可见"这条语义。

🟡 INFORMATIONAL

3. ListStoredWorkflowsByIdAsync 吞掉所有异常(含 OperationCanceledException
AppScopedWorkflowService.cs:353-373

catch { return new Dictionary<...>(); }

至少过滤 OperationCanceledException(客户端断联时应该向上传播,不然调用栈会继续跑浪费资源);其它失败也应记个 warning,静默返空 CLAUDE.md 明确反对。

4. ToWorkflowSummary 在 list 端点里对每个 workflow 读一次 layout 文件
AppScopedWorkflowService.cs:316

TryReadPersistedLayout(scopeId, workflow.WorkflowId) != null

ListAsync 里每个条目都一次 disk lookup,N+1。scope 下 workflow 数量大的时候是个问题。建议一次性列目录文件名然后 HashSet 查存在。

5. new.tsx 里 "Delete Draft" 按钮直接 disabled + 自白文案
apps/aevatar-console-web/src/pages/teams/new.tsx:266-273

<Button disabled style={secondaryActionButtonStyle}>Delete Draft</Button>
<Typography.Text type="secondary">Delete Draft 需要后端删除 workflow 接口,当前前端先不提供假删除。</Typography.Text>

这个 PR 的 WorkspaceController.DeleteWorkflow 已经支持 scoped delete(AppScopedWorkflowService.DeleteDraftAsync)。前端不接就算了,但不要把"后端还没做"当成理由写进 UI —— 后端是做了的。要么接上调用,要么先别渲染这个按钮。

6. IsAuthenticationEnabledAsync 语义和契约都有点别扭

AuthenticationScheme 构造函数保证 Name 非空,所以 !string.IsNullOrWhiteSpace(scheme.Name) 永远为真,等价于 schemes.Any()。更大的问题是 "注册了 scheme" 并不代表 controller 上贴了 [Authorize] —— 这和问题 1 是同一个根因。


✅ 看着还不错的部分

  • teams/new.tsx 的草稿恢复流 URL 参数传递、navigation.ts 新增的 teamMode='create' 分支、api.tswithOptionalScopeId helper 都是干净的抽象。
  • 前端新增了 RunsLaunchRail / RunsActionRequiredPanel 等独立组件并带测试,拆分粒度合理。
  • 后端删除 _workflowCommandPort 字段之后,DI 注册也同步删了,没有留悬空引用。
  • CLI auth-free 模式的 scope query param 测试 (api.test.ts) 覆盖到了 ?scopeId= 拼接的边界。

建议阻塞项

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 20, 2026

Codecov Report

❌ Patch coverage is 60.55777% with 99 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.54%. Comparing base (8861b4c) to head (fd9e546).
⚠️ Report is 23 commits behind head on dev.

Files with missing lines Patch % Lines
...tar.Studio.Application/AppScopedWorkflowService.cs 68.87% 38 Missing and 9 partials ⚠️
....Studio.Hosting/Controllers/WorkspaceController.cs 33.33% 36 Missing and 2 partials ⚠️
...ructure/Storage/ChronoStorageWorkflowDraftStore.cs 53.57% 11 Missing and 2 partials ⚠️
...ication/Studio/Abstractions/IWorkflowDraftStore.cs 80.00% 1 Missing ⚠️
@@            Coverage Diff             @@
##              dev     #248      +/-   ##
==========================================
+ Coverage   68.49%   68.54%   +0.04%     
==========================================
  Files        1108     1109       +1     
  Lines       77793    77905     +112     
  Branches    10178    10204      +26     
==========================================
+ Hits        53287    53399     +112     
+ Misses      20612    20600      -12     
- Partials     3894     3906      +12     
Flag Coverage Δ
ci 68.54% <60.55%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/Aevatar.Studio.Hosting/StudioHostingOptions.cs 100.00% <100.00%> (ø)
...osting/StudioHostingServiceCollectionExtensions.cs 100.00% <100.00%> (ø)
...DependencyInjection/ServiceCollectionExtensions.cs 100.00% <100.00%> (ø)
...tructure/Storage/ChronoStorageCatalogBlobClient.cs 56.32% <100.00%> (+4.49%) ⬆️
...ication/Studio/Abstractions/IWorkflowDraftStore.cs 80.00% <80.00%> (ø)
...ructure/Storage/ChronoStorageWorkflowDraftStore.cs 34.83% <53.57%> (ø)
....Studio.Hosting/Controllers/WorkspaceController.cs 30.34% <33.33%> (+2.50%) ⬆️
...tar.Studio.Application/AppScopedWorkflowService.cs 60.64% <68.87%> (+7.00%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@eanzhao
Copy link
Copy Markdown
Contributor

eanzhao commented Apr 20, 2026

Codex 二次评审 — 同意大部分,有重要补充和一处反驳

拉 Codex 复审了一遍 + 我的 comment。Codex 同意大部分,但有一处反驳和两条我漏掉的 P0

Codex 对我 comment 的判定

# 我的 finding Codex 判定
1 auth-off query param 盲信 Challenged — 真正的 scoped runtime workflow endpoint (/api/scopes/{scopeId}/workflows/{workflowId}) 仍走 RequireAuthorization() + scope_id 校验(见 ScopeEndpointAccess.cs:48-79),query fallback 本身不直接穿透到 committed workflow 平面。问题的本质是 correctness(query scope 没流到存储层),不是 security。我认这个 down-grade,从 CRITICAL 降成 INFORMATIONAL。
2 Save 摘掉 IScopeWorkflowCommandPort Confirmed — P0。
3 空 catch 吞 OperationCanceledException 未单独列,但 P0 级问题盖过了这条。
4 layout N+1 disk I/O 未反驳。
5 Delete Draft button 自白不符 未反驳。
6 auth scheme filter 冗余 Agree(Codex 列为 P2)。

Codex 找到的、我漏掉的 P0

A. ChronoStorageWorkflowStoragePort.UploadWorkflowYamlAsync 静默数据丢失
src/Aevatar.Studio.Infrastructure/Storage/ChronoStorageWorkflowStoragePort.cs:17-27

public async Task UploadWorkflowYamlAsync(string workflowId, string workflowName, string yaml, CancellationToken ct)
{
    _ = workflowName;                                    // ← 入参直接丢弃
    var yamlBytes = Encoding.UTF8.GetBytes(yaml);
    var key = $"{WorkflowDirectory}/{workflowId}.yaml";
    var context = _blobClient.TryResolveContext(string.Empty, key);
    if (context == null)
        return;                                          // ← chrono-storage context 没解析出来 → 直接 no-op
    await _blobClient.UploadAsync(context, yamlBytes, "text/yaml", ct);
}

AppScopedWorkflowService.SaveAsync 上层不检查返回值,直接构造 ToStoredWorkflowFileResponse(...) 返回"保存成功"。任何 chrono-storage 没正确配好的部署(包括 CLI 的某些路径),用户按保存 → 前端提示成功 → 后端根本没写。加上 finding #2 里说的 Save 路径已经不再走 command port,这条 bug 的用户可见后果就是:用户以为保存了,重启 / 刷新 / 打开另一个浏览器,所有工作消失。

修法:context 解析失败时抛 exception(CLAUDE.md: "Don't add error handling, fallbacks, or validation for scenarios that can't happen. Trust internal code and framework guarantees."),或者把 storage 配置作为启动门禁(fail-fast on boot if Studio Save 路径启用但 chrono-storage 没接上)。

B. Query scopeId 不流到 storage 寻址
AppScopedWorkflowService.cs:196-237

var normalizedScopeId = NormalizeRequired(scopeId, nameof(scopeId));
...
await storagePort.UploadWorkflowYamlAsync(workflowId, workflowName, normalizedYaml, ct);   // scopeId 没传
PersistLayout(normalizedScopeId, workflowId, request.Layout);                              // layout 用了 scope

IWorkflowStoragePort.UploadWorkflowYamlAsync / ListWorkflowYamlsAsync / GetWorkflowYamlAsync 都不接 scopeId,寻址只看 workflowIdChronoStorageCatalogBlobClient.TryResolveContext("", key) 内部是从 ambient 解析 scope(AppScopeResolver),而 WorkspaceController.ResolveScopeContextAsync 接受的 query scopeId 并没有注入到 ambient —— 它只是局部变量传给 AppScopedWorkflowService

后果:用 query ?scopeId=A 调保存,但存储写入的实际 scope 是 ambient(B 或 default)。scope list/get 同理。这不是"理论问题",是当前实现就写错了。

修法:要么把 scopeId 做成 IWorkflowStoragePort 的显式参数(推荐),要么在 controller 里 enforce 只在 ambient = query 时放行(现在的 ResolveScopeContextAsync 已经在匹配 ambient,但 auth-off fallback 构造了一个局部 AppScopeContext,这个 context 不会被 storage 层感知)。

Codex 的其它发现

  • P1AppScopedWorkflowServiceTests.SaveAsync_ShouldRewriteYamlNameFromRequestedWorkflowName 现在只断言 storage upload 被调用,丢掉了 "scoped save 仍然命中 runtime-commit contract" 的断言。这就是我 comment Feature/cqrs projection suite #2 里说的测试缺口,Codex 确认。
  • P2IsAuthenticationEnabledAsync 改成显式配置开关。
  • P2AppScopedWorkflowService 现在签名带 scopeId 但实际 storage 不用,方法签名和真实语义分裂。
  • P2ChronoStorageWorkflowStoragePort.GetWorkflowYamlAsync 读回来的 WorkflowName 永远是 workflowId(因为 save 时名字被丢了),接口不诚实。

更新后的 blocking 清单

Blockers(P0,merge 前必修):

  1. Save 路径摘掉 command port(我 Feature/cqrs projection suite #2 / Codex confirmed)
  2. UploadWorkflowYamlAsync 静默 no-op + Save 不校验返回(Codex A 发现)
  3. scope query param 不流到 storage,写入错 scope(Codex B 发现)

Should-fix(P1-P2):
4. 测试回归:scoped save 的 runtime-commit 断言被拿掉
5. workflowName 在 storage 层被丢 + 读回变 workflowId
6. 空 catch 吞 cancellation
7. layout N+1
8. Delete Draft button 文案
9. IsAuthenticationEnabledAsync 换显式开关

Downgraded:

建议这个 PR 拆:先把前端部分(这 PR 70% 的 LOC)和 backend 的安全修复合进去;workflow save/storage 这三条 P0 单独起一个 PR 做显式的 draft/publish 拆分。

@eanzhao
Copy link
Copy Markdown
Contributor

eanzhao commented Apr 20, 2026

回到病根:SaveAsync 是一次语义漂移,不是一个 bug

前两条 comment 的 findings 都只是症状(no-op、scope 不传、storage 失败无感知)。用户指出:

SaveAsync 的核心问题不是"保存失败",而是"语义变了,但接口名字和很多外部预期还像旧语义"。

这是对的。我把前面的 P0 重新排序一下——根问题是整条链的命名/契约 vs 实际行为已经错位,CLAUDE.md 里"API 字段单一语义" / "命名跟随职责" / "单一主干,插件扩展" 三条规则同时踩线。

证据:从外到内每层名字都在撒谎

名字 / 契约 实际语义(本 PR 之后)
HTTP endpoint POST /api/workspace/workflows 只写 draft YAML blob
Controller 方法 SaveWorkflow 同上
Application 方法 AppScopedWorkflowService.SaveAsync 同上
返回类型 WorkflowFileResponse 混血:YAML + 合成出来的 FilePath/DirectoryId/DirectoryLabel/Document/Layout/Findings
Port 接口 IWorkflowStoragePort.UploadWorkflowYamlAsync 承担了"scoped workflow draft 的权威当前态"
Port 的 XML doc "Port for uploading workflow YAML artifacts to external storage (e.g., chrono-storage)" 实际是 draft catalog(List / Get / Upload 三联)

WorkflowFileResponse 本身就是双重语义 bag

src/Aevatar.Studio.Application/Studio/Contracts/WorkspaceContracts.cs:31-42

public sealed record WorkflowFileResponse(
    string WorkflowId,
    string Name,
    string FileName,          // ← draft 专属(文件系统寻址)
    string FilePath,          //   draft 专属
    string DirectoryId,       //   draft 专属
    string DirectoryLabel,    //   draft 专属
    string Yaml,
    WorkflowDocument? Document,                 // ← runtime 专属(解析后的文档)
    WorkflowLayoutDocument? Layout,             //   draft / UI 专属
    IReadOnlyList<ValidationFinding> Findings,  //   runtime 专属
    DateTimeOffset? UpdatedAtUtc = null);

同一个 record 在"draft save"路径上只有前半截是真的(后半截 Document 靠客户端 YAML 再 parse 合成,Findings 空数组,UpdatedAtUtc 是当前时间而不是 committed 时间),在"runtime read"路径上后半截才有意义。CLAUDE.md 原文:

API 字段单一语义:一个字段只表达一个含义,禁止双重语义(如"名称查找 + inline 内容")

现在就是一个 type 表达两种语义,靠调用路径猜哪半是真的。

前端把 draft 写回 committed 的 query cache

apps/aevatar-console-web/src/pages/studio/index.tsx:2403-2432

const savedWorkflow = await studioApi.saveWorkflow({...});

queryClient.setQueryData(
  ['studio-workflow', workflowWorkspaceContextKey, savedWorkflow.workflowId],
  savedWorkflow,
);
await queryClient.invalidateQueries({
  queryKey: ['studio-workspace-workflows', workflowWorkspaceContextKey],
});
setSelectedWorkflowId(savedWorkflow.workflowId);

前端信任 savedWorkflow 是"服务端此刻 workflow 的权威状态",把它覆盖写进 React Query cache。在 PR 之前,这个信任是对的(save 走 command port,返回 committed state)。现在 save 只写 draft blob,但前端还按旧契约把它当 committed 用——之后 listWorkflows 刷回来如果同时带 runtime-committed 字段,前端会短暂看到从 draft 跳回 stale committed 的闪烁。

为什么不能靠"修 bug"修这个

Codex 给的修法"chrono-storage context 解析失败就抛"、我之前说的"把 scopeId 传进 storage"—— 都是在 draft 实现上加补丁,假装它仍然是 committed save。只要名字还叫 SaveWorkflow / WorkflowFileResponse / UploadWorkflowYaml,下一个接手的人还会继续按旧契约写调用点,下一轮调用点错配就会继续出 bug。

正确的修法(对齐 CLAUDE.md)

Option A(推荐):把 draft 和 publish 拆成两条显式契约

  1. 端点层:
    • PUT /api/workspace/workflow-drafts/{workflowId} → 写 draft blob,返回 WorkflowDraftResponse(只带 draft 字段 + YAML,不带 Document/Findings/committed timestamp)
    • POST /api/workspace/workflow-drafts/{workflowId}/publish → 把 draft 推进 IScopeWorkflowCommandPort,返回 WorkflowCommittedResponse(只带 runtime 字段)
  2. Application 层:
    • AppScopedWorkflowService.SaveDraftAsync(...) + PublishAsync(...)
  3. Port 层:
    • IWorkflowDraftStore 替掉 IWorkflowStoragePortSaveDraft / ListDrafts / GetDraft / DeleteDraft,删掉"Upload" 这种一次性动作的词汇
  4. 类型层:
    • 删掉 WorkflowFileResponse 这个双重语义 bag,拆成 WorkflowDraftResponse + WorkflowCommittedResponse
    • 前端 saveWorkflow 拆成 saveDraft + publishWorkflow

这条路符合 CLAUDE.md:

  • 命名跟随职责:每个名字都诚实
  • 单一语义 field:每个 response 只表达一种状态
  • 删除优先:不保留 WorkflowFileResponse 作为兼容壳
  • 单一主干,插件扩展:draft 流和 publish 流都是"单一主干",各自独立

Option B(短期止血):保留旧名字,但

  • SaveAsync 内部同时调 UploadWorkflowYamlAsync IScopeWorkflowCommandPort.UpsertAsync,让外部契约行为恢复到 PR 前
  • 然后在独立 PR 里做 Option A

这只是"把 bug 修回去",不解决语义漂移本身。如果团队短期只想 unblock 前端合入,可以走这条,但要在 PR body 里写明"下一个 PR 做 draft/publish 拆分"。

更新后的 review 定调

前两条 comment 列的 3 个 P0(command port removed / silent no-op / scope 不传)建议合并成一条:

P0(root cause):POST /api/workspace/workflows 及其调用链在本 PR 静默从 "runtime-commit write" 迁成 "draft blob write",但 HTTP path、方法名、返回类型、前端缓存策略都没跟着迁。这是 CLAUDE.md 禁止的双重语义 / 命名与职责不一致。修法不是给新实现打补丁,而是把 draft 和 publish 两条契约拆开、命名诚实。

其余 informational(layout N+1 / 空 catch / Delete Draft button 文案 / auth scheme filter 冗余)可以留作后续 PR。

Under PR #248 / #249, POST /api/workspace/workflows silently migrated from
runtime-commit upsert to editor-draft blob write. The behaviour was corrected
in #249, but the contract names still advertised the old semantics:

- IWorkflowStoragePort / UploadWorkflowYamlAsync / StoredWorkflowYaml
- AppScopedWorkflowService.SaveAsync
- ChronoStorageWorkflowStoragePort

Each name claimed "generic storage" or "save workflow", while the actual
responsibility is "scoped workflow draft catalog". Per CLAUDE.md (命名跟随
职责 / API 字段单一语义), callers reading these names form incorrect
expectations about runtime visibility, revision IDs, and commit semantics.

This commit is a pure semantic rename — no behaviour change:

- IWorkflowStoragePort  -> IWorkflowDraftStore
- ChronoStorageWorkflowStoragePort -> ChronoStorageWorkflowDraftStore
- StoredWorkflowYaml    -> WorkflowDraft
- UploadWorkflowYamlAsync -> SaveDraftAsync
- ListWorkflowYamlsAsync  -> ListDraftsAsync
- GetWorkflowYamlAsync    -> GetDraftAsync
- DeleteWorkflowYamlAsync -> DeleteDraftAsync
- AppScopedWorkflowService.SaveAsync -> SaveDraftAsync (internal helpers
  named Stored*/storedWorkflow* also renamed to Draft/draft for consistency)
- Test stubs / fixtures renamed in lock-step

HTTP path POST /api/workspace/workflows and WorkflowFileResponse are
intentionally left unchanged — phase 2 will address the response-type
double semantics and optionally add an explicit publish endpoint.

All 203 Aevatar.Studio.Tests and 339 Aevatar.Tools.Cli.Tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…semantic-rename

Align scoped workflow draft port naming with actual semantics
@potter-sun potter-sun merged commit 017b789 into dev Apr 20, 2026
11 of 12 checks passed
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.

3 participants