实现端点审计捕获管线#2608
Merged
eanz17 merged 13 commits intoJul 3, 2026
Merged
Conversation
…trail' into refactor/iter2596-issue-2596
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## crnd/milestone32-platform-audit-trail #2608 +/- ##
=========================================================================
+ Coverage 83.83% 83.84% +0.01%
=========================================================================
Files 1288 1296 +8
Lines 97884 98301 +417
Branches 12819 12857 +38
=========================================================================
+ Hits 82063 82423 +360
- Misses 10109 10142 +33
- Partials 5712 5736 +24
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
eanz17
added
crnd:human:auto
Contributor
Author
📊 当前状态 — 协议标签修复(不需要人介入)
下一步自动会做:wakeup-runner 重新把这个 PR 纳入 review/CI 路线，处理当前 CI 红项并派必要 codex。 何时需要人介入:
🤖 controller status banner ⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 测试审查结论：需要补关键覆盖后再合并TL;DR
测试证据参考框架：test pyramid + privacy-by-design audit logging。这里新增的是可复用审计边界，所以测试不能只证明一个合成路由能跑通，还要证明关键分支和至少一个真实端点绑定不会丢记录、错标目标或泄露敏感值。 我给
需要补的测试很具体：
📎 Full raw codex output (archival)---
pr: 2608
role: tests
head_sha: edbe27c513d45b09aca3d92798141d98cf4fcf8f
verdict: reject
---
## Verdict
Reject: the new endpoint audit pipeline has meaningful behavior tests for the generic middleware path, but net-new public branches and real endpoint audit bindings still lack coverage.
Reference-frame note: test pyramid + privacy-by-design audit logging; the PR adds a reusable audit boundary, so tests need to cover core behavior and at least one real integration binding that proves sensitive values and target IDs are captured as intended.
## Evidence
- `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:28` covers accepted records, `:68` covers 403 denied, `:98` covers unauthenticated 401 zero-record behavior, `:110` and `:133` cover JWT/email redaction, and `:150` covers appender failure isolation. These are meaningful behavior assertions, not bump-only tests.
- `src/Aevatar.Bootstrap/Hosting/EndpointAuditOutcomeClassifier.cs:18` introduces terminal outcome branches for `>=500`, generic `>=400`, and non-2xx/non-4xx/5xx `Success`, but the new tests only exercise 200/202 and 403. There is no test proving an endpoint exception or 500 appends an `AuditOutcome.Error` terminal record with `endpoint_error`, nor a test for the fallback `Success` branch.
- `src/Aevatar.Audit.Abstractions/EndpointAudit/EndpointAuditTargetResolvers.cs:44` adds public `FromRouteValues`, and `src/platform/Aevatar.GAgentService.Hosting/Endpoints/ScopeServiceEndpoints.cs:136` uses it for the real Scope Service audit helper. The new tests only use `FromRouteValue` and `FromQuery`; no test asserts multi-route target IDs such as `scopeId/memberId/runId`, duplicate/blank route names, or redaction across joined values.
- `src/workflow/Aevatar.Workflow.Infrastructure/CapabilityApi/WorkflowRunObservatoryEndpoints.cs:62` through `:103`, `src/platform/Aevatar.GAgentService.Hosting/Endpoints/ScopeServiceEndpoints.cs:59` through `:130`, and `agents/channels/Aevatar.GAgents.Channel.NyxIdRelay/ChannelCallbackEndpoints.cs:34` through `:59` add audit metadata to real endpoint surfaces, but no existing endpoint tests assert that those routes now append the expected operation name, target kind/id, and sanitized request summary. This leaves the most important integration risk untested: a future route/helper change could silently drop or mislabel audit records while the synthetic `/audited/widgets` middleware tests still pass.
- `bash tools/ci/test_stability_guards.sh` passes. The diff did not add `Task.Delay`, `WaitUntilAsync`, skip/disable/manual-test markers, or polling allowlist entries.
## What would change your verdict
Add focused tests that:
- Exercise an annotated endpoint returning/throwing 500 and assert the terminal audit record has `AuditOutcome.Error`, `ErrorCode == "endpoint_error"`, and a sanitized error summary.
- Cover `EndpointAuditTargetResolvers.FromRouteValues` directly or through one real Scope Service route, asserting the joined target id and route-value request summary for multiple route values, including sensitive route value redaction.
- Add at least one real host endpoint integration test for a newly annotated Scope/Workflow/Channel/CQRS route that verifies the appended `OperationName`, `Target.Kind`, `Target.Id`, and that bearer/query secrets do not appear in any appended record field.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2608:tests:rejectreview_round: r1 |
Contributor
Author
🤖 架构审查结论：reject，端点审计方向对，但边界落点需要收回TL;DR
架构证据参考框架：Ports and Adapters / Clean Architecture dependency rule。意思是 HTTP 框架适配器留在 Host 边界，共享合同包保持 framework-neutral，只让外层依赖内层。
我没有把端点审计本身判成问题：annotated endpoint、attempted + terminal outcome、fail-open append 这些方向和 issue 目标一致。阻断点是实现位置和依赖显式性。 另一个上下文提醒：提示里给的 改到什么程度我会改判
📎 Full raw codex output (archival)---
pr: 2608
role: architect
head_sha: edbe27c513d45b09aca3d92798141d98cf4fcf8f
verdict: reject
---
## Verdict
reject: endpoint audit direction is sound, but the PR moves ASP.NET Host adapter types into `Aevatar.Audit.Abstractions` and adds hidden service-locator dependency resolution in the capture middleware.
## Evidence
- Reference-frame: Ports and Adapters / Clean Architecture dependency rule. Framework-specific HTTP adapters should stay at the Host edge; shared contracts should remain framework-neutral and depend inward.
- `src/Aevatar.Audit.Abstractions/Aevatar.Audit.Abstractions.csproj:18` adds `FrameworkReference Include="Microsoft.AspNetCore.App"` to the audit abstractions package, and `src/Aevatar.Audit.Abstractions/EndpointAudit/EndpointAuditFilter.cs:5` implements ASP.NET `IEndpointFilter` inside that abstractions package. This regresses the clause: "严格分层：`Domain / Application / Infrastructure / Host`；`API` 仅做宿主与组合，不承载业务编排。" It also regresses: "边界清晰：协议适配、业务编排、状态管理分属不同层；禁止跨层偷渡语义。" The endpoint filter, route-builder extension, `HttpContext` state bridge, and route/query resolvers are HTTP protocol adapter code and should live in a Host/Hosting/Bootstrap boundary package, not in the shared audit contract package.
- `src/Aevatar.Audit.Abstractions/EndpointAudit/EndpointAuditDelegates.cs:1`, `src/Aevatar.Audit.Abstractions/EndpointAudit/EndpointAuditRouteBuilderExtensions.cs:2`, and `src/Aevatar.Audit.Abstractions/EndpointAudit/EndpointAuditSanitizationContext.cs:6` expose `Microsoft.AspNetCore.*`, `RouteHandlerBuilder`, and `HttpContext` from `Aevatar.Audit.Abstractions`. This regresses the dependency direction clause: "依赖反转：上层依赖抽象，禁止跨层反向依赖和对具体实现的直接耦合。" A contract assembly that existing audit core/ports depend on should not force consumers to reference the concrete ASP.NET hosting framework.
- `src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs:216` resolves audit ports through `context.RequestServices.GetService<IAuditTrailAppender>()` and `context.RequestServices.GetService<IAuditActorIdentityHasher>()`. This regresses the clause: "不用 `Service Locator` 隐藏依赖——依赖一律构造注入、显式可见。" The middleware should make the audit capture dependency explicit through constructor/Invoke injection or a narrow injected recorder/ports facade, rather than silently no-oping when hidden lookups fail.
- Context note: the prompt named `b967030af259414428aa6b9fab7e96ea850dd278`, but GitHub currently reports PR #2608 head `edbe27c513d45b09aca3d92798141d98cf4fcf8f`; I reviewed the current three-dot diff from `origin/crnd/milestone32-platform-audit-trail` to `origin/refactor/iter2596-issue-2596`.
## What would change your verdict
- Move ASP.NET-specific endpoint audit pieces (`IEndpointFilter`, `RouteHandlerBuilder` extensions, `HttpContext` state helpers, route/query target resolvers, default HTTP sanitizers) out of `Aevatar.Audit.Abstractions` into an HTTP/Host boundary package such as `Aevatar.Audit.Hosting` or `Aevatar.Bootstrap`. Keep only framework-neutral audit contracts/proto/ports in `Aevatar.Audit.Abstractions`.
- Remove `Microsoft.AspNetCore.App` from `Aevatar.Audit.Abstractions.csproj` unless the remaining package truly has a Host-boundary-only name and responsibility.
- Replace the middleware's `RequestServices.GetService<...>` lookups with explicit dependency injection, or inject a narrow endpoint audit recorder facade that owns the optional/no-op behavior at composition time.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2608:architect:rejectreview_round: r1 |
Contributor
Author
🤖 质量审查结论：commentTL;DR
审查依据与证据参考框架：我按 ASP.NET Core middleware / minimal API filter 的单一职责形状，以及 DRY / SRP 可维护性检查来对照。这个 PR 基本维持了 metadata、filter、middleware 的薄层分工，但下面几处会让新审计入口显得比实际更复杂。
建议改动：删除这些未使用参数/局部变量；让 📎 Full raw codex output (archival)---
pr: 2608
role: quality
head_sha: b967030af259414428aa6b9fab7e96ea850dd278
verdict: comment
---
## Verdict
Comment: the endpoint audit capture pipeline is mostly readable and focused, but a few stale parameters and duplicated sanitizer logic should be cleaned up to keep the new surface simple.
## Evidence
- Reference-frame pass: ASP.NET Core middleware/minimal API filter single-responsibility shape plus DRY/SRP maintainability checks; the PR generally follows a thin metadata/filter/middleware split, but the points below drift from that shape.
- `src/workflow/Aevatar.Workflow.Infrastructure/CapabilityApi/WorkflowRunObservatoryEndpoints.cs:275` keeps `ownScopeId`, `targetScope`, `runId`, `action`, and `loggerFactory` in `AuthorizeCrossScopeAsync`, but after the inline audit logger was removed the method only uses `http`, `authorizer`, and `ct`. The call sites at lines 170, 196, 222, and 244 now pass context that is never read, which makes the authorization helper look more complex than it is.
- `src/Aevatar.Mainnet.Host.Api/Cqrs/CqrsObservatoryApiEndpoints.cs:62` and `src/Aevatar.Mainnet.Host.Api/Cqrs/CqrsObservatoryApiEndpoints.cs:107` still inject `ILoggerFactory`, and lines 68 and 110 still bind `callerScopeId`, but the new endpoint audit metadata removed the only consumers. These are dead parameters/locals from the old inline audit path.
- `src/Aevatar.Audit.Abstractions/EndpointAudit/EndpointAuditSanitizers.cs:13` and `src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs:23` define the same sensitive fragments and regex-based redaction rules, and `EndpointAuditCaptureMiddleware.SanitizeRecordText` at line 361 reimplements the sanitizer. This is small duplication now, but it creates two authorities for what counts as sensitive audit text.
## What would change your verdict (only if comment or reject)
Remove the unused authorization/audit parameters and locals, and make `EndpointAuditCaptureMiddleware` reuse the endpoint audit sanitizer/redaction helper instead of carrying a second copy of the same patterns.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2608:quality:commentreview_round: 1 |
Contributor
Author
🤖 质量审查结论：commentTL;DR
质量证据参考框架：DRY + YAGNI。这里是审计边界胶水代码，重复的脱敏规则和未使用参数很容易让后续维护者误判“这里还有业务分支”。
建议把 record-level redaction 收敛到 📎 Full raw codex output (archival)---
pr: 2608
role: quality
head_sha: edbe27c513d45b09aca3d92798141d98cf4fcf8f
verdict: comment
---
## Verdict
整体可读、范围聚焦，但有几处新增重复规则和死参数会增加后续维护成本，建议评论后清理。
## Evidence
- Reference frame: DRY + YAGNI, applied to security/audit boundary glue where duplicated redaction rules and unused parameters tend to drift.
- `src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs:20` 重新定义了 `Redacted`、JWT/API key/email/phone 正则和敏感片段列表，`src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs:361` 又实现 `SanitizeRecordText`; 同一套规则已经在 `src/Aevatar.Audit.Abstractions/EndpointAudit/EndpointAuditSanitizers.cs:10` 和 `src/Aevatar.Audit.Abstractions/EndpointAudit/EndpointAuditSanitizers.cs:96` 存在。两套 audit redaction 规则几乎相同，后续一边加规则另一边漏加时很容易产生审计口径漂移。
- `src/Aevatar.Mainnet.Host.Api/Cqrs/CqrsObservatoryApiEndpoints.cs:62` / `src/Aevatar.Mainnet.Host.Api/Cqrs/CqrsObservatoryApiEndpoints.cs:107` 注入的 `ILoggerFactory loggerFactory` 已不再使用；`src/Aevatar.Mainnet.Host.Api/Cqrs/CqrsObservatoryApiEndpoints.cs:68` / `src/Aevatar.Mainnet.Host.Api/Cqrs/CqrsObservatoryApiEndpoints.cs:110` 的 `callerScopeId` 也只为 TryGet 返回值存在。旧 logger audit 删除后这些名字变成阅读噪音。
- `src/workflow/Aevatar.Workflow.Infrastructure/CapabilityApi/WorkflowRunObservatoryEndpoints.cs:170`、`src/workflow/Aevatar.Workflow.Infrastructure/CapabilityApi/WorkflowRunObservatoryEndpoints.cs:196`、`src/workflow/Aevatar.Workflow.Infrastructure/CapabilityApi/WorkflowRunObservatoryEndpoints.cs:222`、`src/workflow/Aevatar.Workflow.Infrastructure/CapabilityApi/WorkflowRunObservatoryEndpoints.cs:244` 仍把 `ownScopeId`、`runId`、`action`、`loggerFactory` 传入 `AuthorizeCrossScopeAsync`; 但 `src/workflow/Aevatar.Workflow.Infrastructure/CapabilityApi/WorkflowRunObservatoryEndpoints.cs:275` 的 helper 只使用 `http`、`authorizer` 和 `ct`。这是删除旧 inline audit 后留下的死参数 surface，会让读者误以为 helper 仍按 action/target 做行为分支。
## What would change your verdict (only if comment or reject)
把 record-level redaction 收敛到 `EndpointAuditSanitizers` 的单一实现或共享内部 helper；删除 CQRS observatory 和 Workflow observatory 中不再参与逻辑的 logger/action/target/run 参数与未使用变量。
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2608:quality:commentreview_round: 2 |
Contributor
Author
🤖 PR 2608 第 1 轮 reject 已修完TL;DR
修复说明本轮采用的参考框架是 Ports and Adapters / Clean Architecture 加 privacy-by-design audit logging。也就是说，ASP.NET endpoint audit 只放在 Host/Hosting 边界，审计抽象保持 framework-neutral；审计记录只保留明确允许的 endpoint 摘要，不把 bearer token、query token、邮箱等敏感值写入审计记录。
验证结果：
review-thread completion 检查： 📎 Full raw codex output (archival)# Fix report for PR 2608 round 1
Reference-frame note: Ports and Adapters / Clean Architecture plus privacy-by-design audit logging. The repair keeps ASP.NET endpoint audit code at the Host/Hosting boundary, keeps audit abstractions framework-neutral, makes middleware dependencies explicit through DI, and verifies both synthetic middleware behavior and one real endpoint binding.
refactor self-doc: not applicable (HOST_REFACTOR_COMMENT_POLICY=none)
## Applied
- (A) `src/Aevatar.Audit.Hosting/EndpointAudit/EndpointAuditFilter.cs:1`: moved ASP.NET-specific endpoint audit adapter types (`IEndpointFilter`, route builder extensions, `HttpContext` state, route/query target resolvers, and default HTTP sanitizers) into `Aevatar.Audit.Hosting.EndpointAudit`; `src/Aevatar.Audit.Abstractions/Aevatar.Audit.Abstractions.csproj:11` now keeps only protobuf/package dependencies and `rg -n "Microsoft.AspNetCore|FrameworkReference" src/Aevatar.Audit.Abstractions` returns no matches. This addresses reviewer:architect's evidence #1 and #2.
- (A) `src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs:25`: replaced hidden `HttpContext.RequestServices.GetService<...>` lookups with constructor-visible DI inputs for `IAuditTrailAppender` and `IAuditActorIdentityHasher`; `rg -n "RequestServices\\.GetService|GetService<IAuditTrailAppender>|GetService<IAuditActorIdentityHasher>" src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs` returns no matches. This addresses reviewer:architect's evidence #3.
- (A) `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:85`: added endpoint-audit behavior coverage for handler exceptions and 500 status terminal records with `AuditOutcome.Error`, `ErrorCode == "endpoint_error"`, and sanitized error summaries; `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:121` covers fallback `Success`; `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:136` covers `FromRouteValues` multi-route target ids and sensitive route-value redaction. This addresses reviewer:tests' evidence #1 and #2.
- (B) `test/Aevatar.GAgentService.Integration.Tests/ScopeServiceEndpoints/ScopeServiceEndpointTestKit.cs:439`: scope-extend: the original remote PR three-dot file list did not include GAgentService endpoint tests, but reviewer:tests made one real annotated endpoint binding blocking and these files are in the same logical endpoint-audit refactor. Added test-host audit appender/identity hasher wiring and `EndpointAuditCaptureMiddleware` in the real Scope Service test pipeline.
- (B) `test/Aevatar.GAgentService.Integration.Tests/ScopeServiceEndpoints/ScopeServiceBindingEndpointTests.cs:90`: scope-extend: added a real `PUT /api/scopes/{scopeId}/binding` integration test asserting the appended attempted/terminal records, `OperationName`, target kind/id, request summary, capture plane, and non-leakage of both bearer and query token-shaped secrets. This addresses reviewer:tests' evidence #3.
## Rejected as false positive
- None.
## Blocked (cannot fix this round)
- None.
## Build status
- build: pass. `dotnet build aevatar.slnx --nologo` passed with existing warnings only; final run reported 426 warnings and 0 errors.
- tests: pass. `dotnet test test/Aevatar.Bootstrap.Tests/Aevatar.Bootstrap.Tests.csproj --nologo --filter FullyQualifiedName~EndpointAuditCaptureMiddlewareTests` passed 13/13; `dotnet test test/Aevatar.GAgentService.Integration.Tests/Aevatar.GAgentService.Integration.Tests.csproj --nologo --filter FullyQualifiedName~ScopeServiceBindingEndpointTests` passed 10/10; `bash tools/ci/test_stability_guards.sh` passed. Full solution tests were not run because this fix role requested changed test projects only.
- diff hygiene: pass. `git diff --check` passed.
- review-thread completion: skipped. `/Users/zhaoyiqi/Code/aevatar/.refactor-loop/state/review-thread-completion/pr2608.json` does not exist, so there is no seeded `review_thread_driven=true` completion evidence to close.
## Recommendation for next round
- expect MERGE or MERGE_WITH_COMMENTS: all round-1 reject evidence has concrete fixes, no false positives were needed, and no blocker remains.
⟦AI:AUTO-LOOP⟧
FIX_DONE:2608:round-1:applied-3:rejected-0:blocked-0⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 架构审查结论：reject，需收敛审计文档事实源TL;DR
架构证据参考框架：架构治理的单一事实源原则。稳定架构规则应放在唯一权威 canon 中，否则实现、评审和 CI 会在多份文档之间漂移。 阻断点： 对应 CLAUDE.md 条款： 需要修改：把 endpoint audit capture 的 middleware placement、sanitizer/target resolver 约束、Bootstrap 边界说明并入现有 📎 Full raw codex output (archival)---
pr: 2608
role: architect
head_sha: c0039dea1a615a1b639dffa241e8507c75367a95
verdict: reject
---
## Verdict
reject: the endpoint audit implementation shape is now aligned, but the PR adds a second active audit-trail architecture document instead of updating the existing canon.
## Evidence
- Reference-frame: single source of truth for architecture governance. Stable architecture rules should live in one authoritative canon so implementation, reviews, and CI do not drift across competing documents.
- `docs/architecture/audit-trail.md:1` adds a new active architecture document for endpoint audit capture, while the branch already has `docs/canon/audit-trail.md` as the active Platform Audit Trail canon, including boundary endpoint capture rules at `docs/canon/audit-trail.md:53`. The new document states architecture semantics for the same audit-trail topic, such as "not a business read model" at `docs/architecture/audit-trail.md:11` and Bootstrap/middleware boundary ownership at `docs/architecture/audit-trail.md:30`. This regresses the CLAUDE.md documentation clauses: "`docs/canon/` 是权威参考，`docs/adr/` 是不可变 ADR，`docs/history/` 是非权威归档；架构词汇见 `docs/canon/architecture-vocabulary.md`。" and "`docs/canon/` 一个 topic 一个文件，不重复建权威文档；新增或调整架构口径时优先更新既有 canon。"
## What would change your verdict
- Move the endpoint-audit capture details into the existing `docs/canon/audit-trail.md` section for boundary endpoint capture, or make the new material an explicitly non-authoritative implementation note that links back to the canon and does not duplicate/override architecture rules.
- Delete `docs/architecture/audit-trail.md` after the canonical audit-trail document has the needed endpoint capture placement, sanitizer, and middleware boundary details.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2608:architect:rejectreview_round: 2 |
Contributor
Author
🤖 测试审查结论：approveTL;DR
测试证据参考框架：test pyramid + privacy-by-design audit logging。这里的关键不是堆测试数量，而是用低层 host pipeline 测试覆盖审计捕获、结果分类、脱敏和失败隔离，再用一个真实 Host endpoint 集成测试证明 route metadata、filter、middleware 在实际路由上能串起来。
📎 Full raw codex output (archival)---
pr: 2608
role: tests
head_sha: c0039dea1a615a1b639dffa241e8507c75367a95
verdict: approve
---
## Verdict
approve: test coverage and test quality are adequate for this endpoint audit capture diff.
## Evidence
- Reference-frame note: test pyramid plus privacy-by-design audit logging; this PR now has low-level host pipeline coverage for audit capture/redaction/outcome branches and one real endpoint integration binding.
- `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:28` covers the accepted attempted + terminal audit pair with exact operation names, target identity, actor hash, capture plane, and status result summary.
- `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:68`, `:85`, `:104`, and `:121` cover denied, exception, 500/error, and fallback success outcomes with meaningful assertions on `AuditOutcome`, `ErrorCode`, and error summary.
- `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:136`, `:182`, and `:205` cover multi-route target composition plus token/email redaction across appended record fields, so the sensitive-value rules are regression-tested rather than just executed.
- `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:157`, `:170`, `:222`, and `:239` cover unannotated endpoints, unauthenticated challenge behavior, appender failure isolation, and missing audit port behavior.
- `test/Aevatar.GAgentService.Integration.Tests/ScopeServiceEndpoints/ScopeServiceBindingEndpointTests.cs:90` verifies a real `PUT /api/scopes/{scopeId}/binding` host endpoint appends the expected attempted/terminal operation names, target kind/id, request summary, capture plane, and does not leak the bearer/query token-shaped secret.
- `test/Aevatar.GAgentService.Integration.Tests/ScopeServiceEndpoints/ScopeServiceEndpointTestKit.cs:442` and `:525` wire `IAuditTrailAppender`, `IAuditActorIdentityHasher`, and `EndpointAuditCaptureMiddleware` into the real Scope Service test host rather than checking mocks only.
- No new fixed-duration wait pacing, skip/disable/manual-test markers, or polling allowlist entries were added; `bash tools/ci/test_stability_guards.sh` passed locally.
## What would change your verdict (only if comment or reject)
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2608:tests:approvereview_round: 2 |
Contributor
Author
🤖 质量审查结论：commentTL;DR
质量证据参考框架：YAGNI + Clean Code 的 dead-parameter 规则。这里的判断标准是：route handler 和 private helper 的签名应只暴露当前仍然拥有的行为，不应保留只服务于已删除日志路径的上下文参数。
建议改法：删除 CQRS 两个 handler 的 📎 Full raw codex output (archival)---
pr: 2608
role: quality
head_sha: c0039dea1a615a1b639dffa241e8507c75367a95
verdict: comment
---
## Verdict
Comment: the endpoint audit capture shape is readable and focused overall, but the PR leaves dead signature parameters behind after moving inline audit logging into the new metadata/middleware path.
## Evidence
- Reference-frame pass: YAGNI + Clean Code dead-parameter rule; route handlers and private helpers should expose the behavior they still own, not context kept only for deleted logging.
- `src/Aevatar.Mainnet.Host.Api/Cqrs/CqrsObservatoryApiEndpoints.cs:62` and `src/Aevatar.Mainnet.Host.Api/Cqrs/CqrsObservatoryApiEndpoints.cs:107` still inject `[FromServices] ILoggerFactory loggerFactory`, but the old inline audit logger was removed and the parameter is no longer read. This is harmless at runtime, but it is dead DI noise and suggests a logging side effect that no longer exists.
- `src/workflow/Aevatar.Workflow.Infrastructure/CapabilityApi/WorkflowRunObservatoryEndpoints.cs:275` keeps `ownScopeId`, `targetScope`, `runId`, `action`, and `loggerFactory` in `AuthorizeCrossScopeAsync`, yet the method body now only uses `http`, `authorizer`, and `ct`. The callers at `:170`, `:196`, `:222`, and `:244` still thread those unused values through, making the authorization helper look more complex than its actual post-refactor role.
## What would change your verdict
Remove the unused `ILoggerFactory loggerFactory` parameters from the CQRS handlers, and shrink `AuthorizeCrossScopeAsync` plus its four call sites to only the parameters it still uses.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2608:quality:commentreview_round: 3 |
Contributor
Author
🤖 PR 2608 round 2 修复已完成TL;DR
修复细节本轮 reference-frame 是“架构治理单一事实源”：稳定架构规则应该进入既有 canon，而不是在 处理结果：
验证结果：
📎 Full raw codex output (archival)⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 质量审阅：commentTL;DR
质量证据参考框架：YAGNI + API surface hygiene。YAGNI 是“现在不用就不要保留”的工程原则；API surface hygiene 指公开或半公开调用面要只暴露真实语义，避免读者误以为旧路径仍在工作。
建议改法：CQRS 两个 handler 去掉 📎 Full raw codex output (archival)---
pr: 2608
role: quality
head_sha: 006f547baa5de5cc10795d3bd5090900c1038c8d
verdict: comment
---
## Verdict
comment：整体命名、抽象和范围都比较克制；只有一组审计迁移后遗留的未用参数会让读者误以为旧 inline audit 仍参与逻辑，建议清掉。
## Evidence
- Reference frame: YAGNI + API surface hygiene；成熟形态是行为迁移到 endpoint metadata/middleware 后，同步删除旧路径残留参数，避免“看起来有语义、实际无效果”的接口噪音。
- `src/Aevatar.Mainnet.Host.Api/Cqrs/CqrsObservatoryApiEndpoints.cs:62` 和 `:107` 的 `[FromServices] ILoggerFactory loggerFactory` 已不再使用；同两个方法里的 `callerScopeId` 也只作为未读 out 变量存在。旧 logger audit 被 `.WithEndpointAudit(...)` 替代后，这些名字已经不表达当前业务意图，只增加阅读负担。
- `src/workflow/Aevatar.Workflow.Infrastructure/CapabilityApi/WorkflowRunObservatoryEndpoints.cs:141`、`:186`、`:212`、`:237` 继续注入并传递 `ILoggerFactory loggerFactory`，但 `AuthorizeCrossScopeAsync` 内已经没有日志审计逻辑；`:275`-`:283` 的 `ownScopeId`、`targetScope`、`runId`、`action`、`loggerFactory` 参数也没有被读取。这个私有 helper 现在实际只需要 `HttpContext`、`IPlatformAdminAuthorizer` 和 `CancellationToken`，保留旧参数会误导后续维护者以为它仍在按 action/target 做审计。
## What would change your verdict (only if comment or reject)
删除上述未用参数和对应传参：CQRS 两个 handler 去掉 `ILoggerFactory loggerFactory`，未使用的 scope out 改为 discard；Workflow observatory 去掉 handler 注入的 `ILoggerFactory`，并把 `AuthorizeCrossScopeAsync` 收窄到实际使用的参数。
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2608:quality:commentreview_round: 4 |
Contributor
Author
🤖 Tests reviewer: 需要补真实路由审计回归测试TL;DR
测试证据我采用的参考框架是 cross-cutting middleware 的 contract/integration testing：横切 middleware 不能只测一个 synthetic host，还要对每个接入它的真实路由族保留至少一条“路由 metadata、sanitizer、target resolver、auth outcome、middleware append”一起工作的回归测试。 这次新增的 阻塞点在真实接入面：
没有发现本 PR 新增 需要补的测试很具体：workflow observatory、CQRS observatory、channel callback 每个 route family 至少一条真实 mapped route，通过 TestServer 或同等 pipeline 执行，注册 📎 Full raw codex output (archival)---
pr: 2608
role: tests
head_sha: 006f547baa5de5cc10795d3bd5090900c1038c8d
verdict: reject
---
## Verdict
Reject: the new generic middleware tests are meaningful, but several real endpoint families that gained endpoint audit capture have no route-level regression test proving their metadata/sanitizers feed the audit pipeline.
## Evidence
- Reference-frame note: using the contract/integration testing frame for cross-cutting middleware, each production route family that opts into endpoint audit metadata should have at least one route-level test proving the actual route metadata, target resolver, sanitizer, auth outcome, and middleware append behavior work together.
- `src/workflow/Aevatar.Workflow.Infrastructure/CapabilityApi/WorkflowRunObservatoryEndpoints.cs:55` adds `WithEndpointAudit` to `/api/workflow/observatory/me`, and lines 65/76/87/98 add custom audited observatory routes using `ResolveWorkflowObservatoryTarget` / `WorkflowObservatoryRequestSummary`; however `test/Aevatar.Workflow.Host.Api.Tests/WorkflowRunObservatoryEndpointsAdminTests.cs:32` still calls `WorkflowRunObservatoryEndpoints.ListRuns(...)` directly, bypassing endpoint routing/filter/middleware, and this PR does not touch that test file. There is no assertion that these real observatory routes append attempted/terminal audit records or redact `scope`/`runId` summaries.
- `src/Aevatar.Mainnet.Host.Api/Cqrs/CqrsObservatoryApiEndpoints.cs:38` and `:48` replace the old explicit platform-read audit logging with `WithEndpointAudit` metadata on `/api/cqrs/scopes` and `/api/cqrs/readmodels`; no test file was added or updated for CQRS observatory routes, so there is no behavior assertion that admin allow/deny requests now produce endpoint audit records after the logger path was removed.
- `agents/channels/Aevatar.GAgents.Channel.NyxIdRelay/ChannelCallbackEndpoints.cs:34`, `:44`, and `:54` add endpoint audit metadata to channel registration create/delete/test-reply, including the new `ChannelRegistrationRequestSummary` at `:66`. Existing `test/Aevatar.GAgents.ChannelRuntime.Tests/ChannelCallbackEndpointsTests.cs:34` maps the routes and checks authorization/handler behavior, but the PR does not update it to assert `EndpointAuditMetadata` or end-to-end append behavior for these real routes.
- The positive side: `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:29` through `:247` are real behavior tests for the generic middleware/classifier/sanitization path, and `test/Aevatar.GAgentService.Integration.Tests/ScopeServiceEndpoints/ScopeServiceBindingEndpointTests.cs:91` covers one actual scope-service route. I found no newly added skip/disable/manual marker and no new fixed-duration wait pacing in the touched tests; `dotnet test test/Aevatar.Bootstrap.Tests/Aevatar.Bootstrap.Tests.csproj --filter EndpointAuditCaptureMiddlewareTests --nologo` and `bash tools/ci/test_stability_guards.sh` both passed in the PR worktree.
## What would change your verdict (only if comment or reject)
Add route-level tests for at least one audited route in each uncovered endpoint family: workflow observatory, CQRS observatory, and channel callback. Each should drive the real mapped route through the ASP.NET Core pipeline with `IAuditTrailAppender`/`IAuditActorIdentityHasher` test doubles and assert operation name, attempted + terminal record count, outcome, target kind/id, sanitized request summary, and no bearer/token/email leakage.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2608:tests:rejectreview_round: 3 |
Contributor
Author
🤖 架构审查通过：endpoint audit 管线没有发现 CLAUDE/AGENTS 回归TL;DR
架构证据参考框架：privacy-by-design security audit logging 加 CQRS/event-sourcing。也就是审计记录应是 append-only、allowlist-driven、sanitized 的治理 artifact，不能反过来成为业务事实源、read model 或第二条 projection rail。 我按实际三点 diff、
补充检查： 📎 Full raw codex output (archival)---
pr: 2608
role: architect
head_sha: 006f547baa5de5cc10795d3bd5090900c1038c8d
verdict: approve
---
## Verdict
approve - no architectural compliance concerns found from the CLAUDE.md / AGENTS.md angle.
## Evidence
- Reference frame: privacy-by-design security audit logging plus CQRS/event-sourcing; the known-good shape is append-only, allowlisted, sanitized audit artifacts that stay separate from business authority.
- `docs/canon/audit-trail.md:16` and `docs/canon/audit-trail.md:19` explicitly frame audit trail as not a domain read model / actor state replica / second projection rail, and as append-only sanitized governance logging separated from business authority. This matches CLAUDE.md's read/write separation and "not default readmodel history view" constraints.
- `docs/canon/audit-trail.md:66` documents strongly typed endpoint audit metadata (`operation_name`, `sensitivity_level`, `target_kind`, resolver, sanitizers), and `src/Aevatar.Audit.Hosting/EndpointAudit/EndpointAuditMetadata.cs:5` implements those as typed constructor-required fields rather than a generic bag. This aligns with the CLAUDE.md strong typing and single-semantic-field rules.
- `src/Aevatar.Audit.Hosting/EndpointAudit/EndpointAuditFilter.cs:11` only runs for annotated endpoints and captures request/result summaries; `EndpointAuditHttpContextState.cs:21` stores target/summary in `HttpContext.Items`, which is request-local execution context rather than a cross-request fact registry. I did not see a new process-wide actor/session/entity mapping or second owner for business state.
- `src/Aevatar.Bootstrap/Hosting/WebApplicationBuilderExtensions.cs:137` places routing before audit capture, and `src/Aevatar.Bootstrap/Hosting/WebApplicationBuilderExtensions.cs:149` inserts `EndpointAuditCaptureMiddleware` after authentication and before authorization. That matches the issue's need to capture authenticated 403 short-circuits without moving authorization logic into application/query services.
- `src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs:20` depends on `IAuditTrailAppender` and `IAuditActorIdentityHasher`, while `src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs:105` builds an `AuditRecord` and appends best-effort via `src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs:133`. Bootstrap remains capture glue; it does not define audit schema, storage, query, retention, or hasher implementation.
- `src/platform/Aevatar.GAgentService.Hosting/Endpoints/ScopeServiceEndpoints.cs:59`, `agents/channels/Aevatar.GAgents.Channel.NyxIdRelay/ChannelCallbackEndpoints.cs:33`, `src/workflow/Aevatar.Workflow.Infrastructure/CapabilityApi/WorkflowRunObservatoryEndpoints.cs:52`, and `src/Aevatar.Mainnet.Host.Api/Cqrs/CqrsObservatoryApiEndpoints.cs:35` attach endpoint audit metadata at endpoint-owner boundaries. The CQRS/workflow pseudo-audit logger helpers are removed in the same diff, so I do not see a parallel logger-audit pathway left behind.
- Host production SSOT check: the diff does not move host rules, branch topology, durable ledger authority, or machine paths into `.refactor-loop/` or `.refactor-loop/host.env`. The only `.refactor-loop` changes are the requested local review artifact and existing implementation/debug artifacts.
- Old/New comment policy check: with `HOST_REFACTOR_COMMENT_POLICY` empty (`none`), I found no new Old/New refactor-history source comments in the PR diff.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2608:architect:approvereview_round: 3 |
…trail' into refactor/iter2596-issue-2596
Contributor
Author
🤖 PR 2608 round 2 fix 已补齐 route-level audit 测试TL;DR
修复详情本轮使用的判断框架是 cross-cutting middleware 的 contract/integration testing： 阻塞需求只来自 tests reviewer 的 已补的覆盖点：
验证结果：
📎 Full raw codex output (archival)# Fix report for PR 2608 round 2
Reference-frame note: contract/integration testing for cross-cutting middleware. The known-good shape is that each production route family opting into endpoint audit metadata has at least one real ASP.NET Core route/pipeline test proving route metadata, target resolution, sanitization, auth outcome, and audit append behavior together.
refactor self-doc: not applicable (HOST_REFACTOR_COMMENT_POLICY=none)
## Applied
- (B) test/Aevatar.Workflow.Host.Api.Tests/WorkflowRunObservatoryEndpointsAdminTests.cs:223: scope-extend: this test file was outside the original three-dot PR diff, but reviewer:tests' reject evidence required route-level coverage for `WorkflowRunObservatoryEndpoints.cs:55,65,76,87,98`; added a real mapped `GET /api/workflow/observatory/runs/{runId}` pipeline test asserting attempted + terminal audit records, operation names, `Accepted` outcomes, workflow target kind/id, sanitized request summary, boundary capture plane, and no raw token/email leakage (addresses reviewer:tests' evidence #1).
- (B) test/Aevatar.Capabilities.Tests/CqrsObservatoryApiEndpointsAuditTests.cs:24: scope-extend: this new test file was outside the original three-dot PR diff, but reviewer:tests' reject evidence required route-level coverage for `CqrsObservatoryApiEndpoints.cs:38,48`; added a real mapped `GET /api/cqrs/scopes` TestServer pipeline test asserting attempted + terminal audit records, operation names, `Accepted` outcomes, CQRS target kind/id, sanitized request summary, boundary capture plane, and no raw token/email leakage (addresses reviewer:tests' evidence #2).
- (B) test/Aevatar.GAgents.ChannelRuntime.Tests/ChannelCallbackEndpointsTests.cs:84: scope-extend: this test file was outside the original three-dot PR diff, but reviewer:tests' reject evidence required route-level coverage for `ChannelCallbackEndpoints.cs:34,44,54`; added a real mapped `POST /api/channels/registrations` pipeline test asserting attempted + terminal audit records, operation names, `Accepted` outcomes, channel-registration target kind/id, sanitized request summary, boundary capture plane, and no raw token/email/secret/verification-token leakage (addresses reviewer:tests' evidence #3).
- (B) test/Aevatar.GAgents.ChannelRuntime.Tests/Aevatar.GAgents.ChannelRuntime.Tests.csproj:29: scope-extend: the channel callback route-level test must execute the real `EndpointAuditCaptureMiddleware`; added an existing in-repo project reference to `src/Aevatar.Bootstrap/Aevatar.Bootstrap.csproj` without adding any package dependency.
## Rejected as false positive
- None.
## Blocked (cannot fix this round)
- None. reviewer:quality's dead-parameter cleanup request is a `comment`, not a `reject`, so it was recorded as advisory context and intentionally not treated as a blocking fix demand in this round.
## Build status
- build: pass (`dotnet build aevatar.slnx --nologo`; existing NuGet/obsolete warnings only)
- tests: pass
- `dotnet test test/Aevatar.Workflow.Host.Api.Tests/Aevatar.Workflow.Host.Api.Tests.csproj --filter FullyQualifiedName~WorkflowRunObservatoryEndpointsAdminTests --nologo` passed: 13 passed, 0 failed.
- `dotnet test test/Aevatar.GAgents.ChannelRuntime.Tests/Aevatar.GAgents.ChannelRuntime.Tests.csproj --filter ChannelRegistrationRoute_ShouldAppendEndpointAuditRecords --nologo` passed: 1 passed, 0 failed.
- `dotnet test test/Aevatar.Capabilities.Tests/Aevatar.Capabilities.Tests.csproj --filter CqrsObservatoryRoute_ShouldAppendEndpointAuditRecords --nologo` passed: 1 passed, 0 failed.
- `bash tools/ci/test_stability_guards.sh` passed.
## Recommendation for next round
- Expect merge or merge-with-comments. The only reject demand in round 2 asked for route-level endpoint audit regression coverage across workflow observatory, CQRS observatory, and channel callback; this round adds those three tests and verifies build/targeted tests.
⟦AI:AUTO-LOOP⟧
FIX_DONE:2608:round-2:applied-4:rejected-0:blocked-0⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 PR 2608 round 2 修复已完成TL;DR
修复摘要本轮采用 cross-cutting middleware 的 contract/integration testing 框架：跨切面 middleware 不能只靠通用单元测试兜底，每个选择接入 endpoint audit metadata 的生产 route family 至少要有一条真实路由测试，证明路由 metadata、target resolver、sanitizer、auth outcome 和 audit append 一起工作。 已补齐或确认的覆盖点：
另外， 验证结果： review-thread completion artifact 不存在： 📎 Full raw codex output (archival)# Fix report for PR 2608 round 2
Reference-frame note: contract/integration testing for cross-cutting middleware. The known-good shape is that each production route family opting into endpoint audit metadata has at least one real ASP.NET Core route/pipeline test proving route metadata, target resolution, sanitization, auth outcome, and audit append behavior together.
refactor self-doc: not applicable (HOST_REFACTOR_COMMENT_POLICY=none)
## Applied
- (B) test/Aevatar.Workflow.Host.Api.Tests/WorkflowRunObservatoryEndpointsAdminTests.cs:223: scope-extend: this test file was outside the original three-dot PR diff, but reviewer:tests' reject evidence required route-level coverage for `WorkflowRunObservatoryEndpoints.cs:55,65,76,87,98`; added a real mapped `GET /api/workflow/observatory/runs/{runId}` pipeline test asserting attempted + terminal audit records, operation names, `Accepted` outcomes, workflow target kind/id, sanitized request summary, boundary capture plane, and no raw token/email leakage (addresses reviewer:tests' evidence #1).
- (B) test/Aevatar.Capabilities.Tests/CqrsObservatoryApiEndpointsAuditTests.cs:24: scope-extend: this new test file was outside the original three-dot PR diff, but reviewer:tests' reject evidence required route-level coverage for `CqrsObservatoryApiEndpoints.cs:38,48`; added a real mapped `GET /api/cqrs/scopes` TestServer pipeline test asserting attempted + terminal audit records, operation names, `Accepted` outcomes, CQRS target kind/id, sanitized request summary, boundary capture plane, and no raw token/email leakage (addresses reviewer:tests' evidence #2).
- (B) test/Aevatar.GAgents.ChannelRuntime.Tests/ChannelCallbackEndpointsTests.cs:84: scope-extend: this test file was outside the original three-dot PR diff, but reviewer:tests' reject evidence required route-level coverage for `ChannelCallbackEndpoints.cs:34,44,54`; added a real mapped `POST /api/channels/registrations` pipeline test asserting attempted + terminal audit records, operation names, `Accepted` outcomes, channel-registration target kind/id, sanitized request summary, boundary capture plane, and no raw token/email/secret/verification-token leakage (addresses reviewer:tests' evidence #3).
- (B) test/Aevatar.GAgents.ChannelRuntime.Tests/Aevatar.GAgents.ChannelRuntime.Tests.csproj:29: scope-extend: the channel callback route-level test must execute the real `EndpointAuditCaptureMiddleware`; added an existing in-repo project reference to `src/Aevatar.Bootstrap/Aevatar.Bootstrap.csproj` without adding any package dependency.
- (A) test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:441, test/Aevatar.Workflow.Host.Api.Tests/WorkflowRunObservatoryEndpointsAdminTests.cs:449, test/Aevatar.GAgents.ChannelRuntime.Tests/ChannelCallbackEndpointsTests.cs:977, test/Aevatar.GAgentService.Integration.Tests/ScopeServiceEndpoints/ScopeServiceEndpointTestKit.cs:651, test/Aevatar.Capabilities.Tests/CqrsObservatoryApiEndpointsAuditTests.cs:168: aligned route-audit test doubles with the current `IAuditTrailAppender.AppendAsync -> AuditTrailAppendResult` contract and removed obsolete `AppendManyAsync`/`AuditTrailAppendReceipt` test code after `dotnet build` exposed the stale signature.
## Rejected as false positive
- None.
## Blocked (cannot fix this round)
- None. reviewer:quality's dead-parameter cleanup request is a `comment`, not a `reject`, so it was recorded as advisory context and intentionally not treated as a blocking fix demand in this round.
## Build status
- build: pass (`dotnet build aevatar.slnx --nologo`; existing NuGet/analyzer/obsolete warnings only; final run reported 455 warnings and 0 errors)
- tests: pass
- `dotnet test test/Aevatar.Bootstrap.Tests/Aevatar.Bootstrap.Tests.csproj --filter FullyQualifiedName~EndpointAuditCaptureMiddlewareTests --nologo` passed: 13 passed, 0 failed.
- `dotnet test test/Aevatar.Workflow.Host.Api.Tests/Aevatar.Workflow.Host.Api.Tests.csproj --filter FullyQualifiedName~WorkflowRunObservatoryEndpointsAdminTests --nologo` passed: 13 passed, 0 failed.
- `dotnet test test/Aevatar.GAgents.ChannelRuntime.Tests/Aevatar.GAgents.ChannelRuntime.Tests.csproj --filter ChannelRegistrationRoute_ShouldAppendEndpointAuditRecords --nologo` passed: 1 passed, 0 failed.
- `dotnet test test/Aevatar.Capabilities.Tests/Aevatar.Capabilities.Tests.csproj --filter CqrsObservatoryRoute_ShouldAppendEndpointAuditRecords --nologo` passed: 1 passed, 0 failed.
- `dotnet test test/Aevatar.GAgentService.Integration.Tests/Aevatar.GAgentService.Integration.Tests.csproj --filter ScopeBindingEndpoint_ShouldAppendEndpointAuditRecord --nologo` passed: 1 passed, 0 failed.
- `bash tools/ci/test_stability_guards.sh` passed.
- diff hygiene: pass (`git diff --check`)
- review-thread completion: skipped. `/Users/zhaoyiqi/Code/aevatar/.refactor-loop/state/review-thread-completion/pr2608.json` does not exist, so there is no seeded `review_thread_driven=true` completion evidence to close.
## Recommendation for next round
- Expect merge or merge-with-comments. The only reject demand in round 2 asked for route-level endpoint audit regression coverage across workflow observatory, CQRS observatory, and channel callback; this round adds those three tests and verifies build/targeted tests.
⟦AI:AUTO-LOOP⟧
FIX_DONE:2608:round-2:applied-5:rejected-0:blocked-0⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 Quality review: 建议提取重复的审计测试 fixtureTL;DR
质量证据参考框架：DRY / shared test fixture extraction。这里的意思是，测试基础设施一旦在 3 个以上位置出现同形复制，就应该有一个单一维护点，否则之后审计字段或脱敏规则一变，多个测试文件容易漏改。 我没有发现生产路径里明显的命名、死代码、单实现抽象或复杂度 blocker。 需要提醒的是测试代码重复：
建议把 📎 Full raw codex output (archival)---
pr: 2608
role: quality
head_sha: 73f74f6aa16c58a0f175d3406c9c767ac799ba53
verdict: comment
---
## Verdict
Comment: production code is readable and focused, but the new endpoint-audit test harness is copied across several test projects and should be extracted before it keeps spreading.
## Evidence
- Reference frame: DRY / shared test fixture extraction; repeated test infrastructure should have one owner once the same setup appears in three or more places.
- `test/Aevatar.Capabilities.Tests/CqrsObservatoryApiEndpointsAuditTests.cs:90`, `test/Aevatar.GAgents.ChannelRuntime.Tests/ChannelCallbackEndpointsTests.cs:917`, `test/Aevatar.Workflow.Host.Api.Tests/WorkflowRunObservatoryEndpointsAdminTests.cs:355`, and `test/Aevatar.GAgentService.Integration.Tests/ScopeServiceEndpoints/ScopeServiceBindingEndpointTests.cs:331` each carry a near-identical `RecordStrings(AuditRecord)` helper to scan audit records for leaked sensitive text.
- `test/Aevatar.Capabilities.Tests/CqrsObservatoryApiEndpointsAuditTests.cs:164`, `test/Aevatar.GAgents.ChannelRuntime.Tests/ChannelCallbackEndpointsTests.cs:973`, `test/Aevatar.Workflow.Host.Api.Tests/WorkflowRunObservatoryEndpointsAdminTests.cs:445`, and `test/Aevatar.GAgentService.Integration.Tests/ScopeServiceEndpoints/ScopeServiceEndpointTestKit.cs:647` each define the same recording `IAuditTrailAppender` fixture.
- `test/Aevatar.Capabilities.Tests/CqrsObservatoryApiEndpointsAuditTests.cs:180`, `test/Aevatar.GAgents.ChannelRuntime.Tests/ChannelCallbackEndpointsTests.cs:989`, `test/Aevatar.Workflow.Host.Api.Tests/WorkflowRunObservatoryEndpointsAdminTests.cs:461`, and `test/Aevatar.GAgentService.Integration.Tests/ScopeServiceEndpoints/ScopeServiceEndpointTestKit.cs:663` each define the same stable audit identity hasher fixture; the first three files also duplicate a bearer-only route audit auth handler at `test/Aevatar.Capabilities.Tests/CqrsObservatoryApiEndpointsAuditTests.cs:190`, `test/Aevatar.GAgents.ChannelRuntime.Tests/ChannelCallbackEndpointsTests.cs:999`, and `test/Aevatar.Workflow.Host.Api.Tests/WorkflowRunObservatoryEndpointsAdminTests.cs:471`.
- `${HOST_REFACTOR_COMMENT_POLICY}` is empty and normalizes to `none`; the PR diff adds no new `Refactor` / `Old pattern` / `New principle` source comments, so self-doc handling is compliant for this quality review.
## What would change your verdict (only if comment or reject)
Extract the shared audit-test pieces into one test helper location, such as a linked file under `test/Shared` or a narrow test support project, covering `RecordStrings`, the recording appender, the stable identity hasher, and the simple bearer auth handler. Keep endpoint-specific fake services and route registration local to each test.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2608:quality:commentreview_round: 5 |
Contributor
Author
🤖 Tests review：通过TL;DR
测试证据我采用的参照系是 test pyramid + behavior-driven boundary testing：端点审计是一个 ASP.NET Core 横切 middleware，合理形态是核心语义在 focused middleware tests 里覆盖完整矩阵，再用真实 HTTP route integration tests 抽样验证各接入面没有漏挂、没有泄漏敏感信息。
📎 Full raw codex output (archival)---
pr: 2608
role: tests
head_sha: 73f74f6aa16c58a0f175d3406c9c767ac799ba53
verdict: approve
---
## Verdict
Approve: the changed audit pipeline has meaningful behavior coverage for the core middleware matrix and representative endpoint integrations, with no test-quality blocker found.
## Evidence
- Reference frame: test pyramid + behavior-driven boundary testing for a cross-cutting ASP.NET Core middleware; shared audit semantics should be covered once in focused middleware tests, then sampled through real HTTP route integrations at each attached surface.
- `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:28` through `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:240` cover the core business outcomes: attempted + terminal records, accepted/denied/error/success classification, unauthenticated zero-record behavior, unannotated zero-record behavior, route/query redaction, appender failure isolation, and missing audit port fallback.
- `test/Aevatar.Capabilities.Tests/CqrsObservatoryApiEndpointsAuditTests.cs:28` verifies the CQRS observatory route through real routing/auth/audit middleware and asserts operation names, target, request summary, capture plane, query execution, and token/email non-leakage.
- `test/Aevatar.GAgentService.Integration.Tests/ScopeServiceEndpoints/ScopeServiceBindingEndpointTests.cs:90` verifies the Scope binding audit path through the existing integration host and asserts the attempted/terminal pair, target identity, request summary, capture plane, and token non-leakage.
- `test/Aevatar.GAgents.ChannelRuntime.Tests/ChannelCallbackEndpointsTests.cs:84` verifies the NyxID relay registration route through real routing/auth/audit middleware and asserts operation names, target, request summary, capture plane, and that bearer token, email, app secret, and verification token do not enter audit records.
- `test/Aevatar.Workflow.Host.Api.Tests/WorkflowRunObservatoryEndpointsAdminTests.cs:223` verifies workflow observatory audit capture for a cross-scope run lookup and asserts target construction, sanitized request summary, capture plane, and token/email non-leakage while preserving the existing authorization matrix tests in the same file.
- Static review found no added `[Skip]`/manual-disable markers, no added `Task.Delay(...)` or `WaitUntilAsync(...)` pacing, and no assertion weakening. `bash tools/ci/test_stability_guards.sh` passed locally.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2608:tests:approvereview_round: 4 |
Contributor
Author
🤖 架构审查通过：endpoint audit 管线当前 head 无 CLAUDE/AGENTS 回归TL;DR
架构证据参考框架：privacy-by-design security audit logging 加 CQRS/event-sourcing。这里的好形态是 append-only、allowlist-driven、sanitized 的审计 artifact；它只记录治理事实，不能反过来成为业务事实源、read model，或第二条 projection rail。
补充检查： 提示里的 📎 Full raw codex output (archival)---
pr: 2608
role: architect
head_sha: c6192bee9b44d71b5a3bd0fb74fa30d1f8b213e6
verdict: approve
---
## Verdict
approve - no architectural compliance concerns found for the current PR head; the prompt head `73f74f6aa16c58a0f175d3406c9c767ac799ba53` was superseded by `c6192bee9b44d71b5a3bd0fb74fa30d1f8b213e6`, and the intervening change is test-helper-only.
## Evidence
- Reference-frame: privacy-by-design security audit logging plus CQRS/event-sourcing. The mature shape is append-only, allowlisted, sanitized audit artifacts that remain separate from business authority and projection/readmodel ownership.
- `docs/canon/audit-trail.md:16` and `docs/canon/audit-trail.md:19` state the audit trail is not a domain read model / actor state replica / second projection rail and is append-only, allowlist-driven, sanitized, and separated from business authority. This aligns with `CLAUDE.md:11` (`统一投影链路`), `CLAUDE.md:13` (`读写分离`), and `CLAUDE.md:60-85` (readmodel/projection authority constraints).
- `docs/canon/audit-trail.md:53` through `docs/canon/audit-trail.md:104` define boundary endpoint capture as Host/adapter-edge capture for annotated HTTP endpoints, with strong endpoint metadata and narrow sanitized summaries. `src/Aevatar.Audit.Hosting/EndpointAudit/EndpointAuditMetadata.cs:5` through `src/Aevatar.Audit.Hosting/EndpointAudit/EndpointAuditMetadata.cs:58` implements that as typed required fields (`operationName`, `sensitivityLevel`, `targetKind`, target resolver, request/result sanitizers), matching `CLAUDE.md:16-17` and `AGENTS.md:10-12` on strong core semantics, single-field meaning, and constrained `Metadata` usage.
- `src/Aevatar.Bootstrap/Hosting/WebApplicationBuilderExtensions.cs:137` through `src/Aevatar.Bootstrap/Hosting/WebApplicationBuilderExtensions.cs:153` places routing before endpoint audit capture and inserts `EndpointAuditCaptureMiddleware` after authentication and before authorization. That matches `docs/canon/audit-trail.md:81-86` and keeps the capture as Host glue rather than Application query/write-model logic.
- `src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs:19` through `src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs:36` explicitly receives `IAuditTrailAppender` and `IAuditActorIdentityHasher`; `src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs:60` through `src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs:90` writes attempted plus exactly one terminal record, and `src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs:133` through `src/Aevatar.Bootstrap/Hosting/EndpointAuditCaptureMiddleware.cs:154` keeps append failure best-effort. This stays within the `CLAUDE.md:10` Host boundary and does not define storage/query/retention/hasher implementation in Bootstrap.
- `src/Aevatar.Audit.Hosting/EndpointAudit/EndpointAuditHttpContextState.cs:21` through `src/Aevatar.Audit.Hosting/EndpointAudit/EndpointAuditHttpContextState.cs:35` and `src/Aevatar.Audit.Hosting/EndpointAudit/EndpointAuditHttpContextState.cs:47` through `src/Aevatar.Audit.Hosting/EndpointAudit/EndpointAuditHttpContextState.cs:55` use `HttpContext.Items` for per-request target/summary capture. This is request-local execution context, not a cross-request fact registry, so I do not see a new violation of `CLAUDE.md:107-113` or `AGENTS.md:117-120`.
- `src/Aevatar.Audit.Hosting/EndpointAudit/EndpointAuditSanitizers.cs:13` through `src/Aevatar.Audit.Hosting/EndpointAudit/EndpointAuditSanitizers.cs:42` and `src/Aevatar.Audit.Hosting/EndpointAudit/EndpointAuditSanitizers.cs:96` through `src/Aevatar.Audit.Hosting/EndpointAudit/EndpointAuditSanitizers.cs:120` centralize redaction for token/API-key/email/phone-shaped values before audit append, aligning with `docs/canon/audit-trail.md:74` and `docs/canon/audit-trail.md:102-104`.
- Endpoint-owner surfaces attach audit metadata in the scoped issue boundary: `src/platform/Aevatar.GAgentService.Hosting/Endpoints/ScopeServiceEndpoints.cs:59-147`, `agents/channels/Aevatar.GAgents.Channel.NyxIdRelay/ChannelCallbackEndpoints.cs:33-60`, `src/workflow/Aevatar.Workflow.Infrastructure/CapabilityApi/WorkflowRunObservatoryEndpoints.cs:52-104`, and `src/Aevatar.Mainnet.Host.Api/Cqrs/CqrsObservatoryApiEndpoints.cs:35-53`. This matches #2596's authorized inventory and does not introduce an unrelated business workflow expansion.
- Old/New policy: `HOST_REFACTOR_COMMENT_POLICY` is empty, normalized to `none`; a diff grep found no newly added Old/New/iter refactor-history source comments. Host-specific grep variables and proto policy were empty; no `.proto` file changed. Additional diff greps found no new `.refactor-loop/host.env` production SSOT movement, external repo dependency additions, forbidden ports `5000`/`5050`, `ConcurrentDictionary`/`Dictionary<>` fact registries, query-time `IEventStore` replay, `GetAwaiter().GetResult()`, `TypeUrl.Contains`, or `ChatAsync` additions.
- Verification signal: `bash tools/ci/architecture_guards.sh` passed locally on the reviewed worktree; output had existing NuGet warnings only.
- Current-head delta check: `73f74f6aa16c58a0f175d3406c9c767ac799ba53..c6192bee9b44d71b5a3bd0fb74fa30d1f8b213e6` changes only test helper implementations in five test files, switching fake appenders to `AuditTrailAppendResult.Appended(...)` and deleting obsolete `AppendManyAsync` stubs. No production architecture files changed in that delta.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2608:architect:approvereview_round: r4 |
Contributor
Author
🤖 质量审查：comment，建议清掉旧审计路径留下的死参数TL;DR
证据参考框架：YAGNI / dead-parameter minimization。这里的
我会把 verdict 保持为 📎 Full raw codex output (archival)---
pr: 2608
role: quality
head_sha: c6192bee9b44d71b5a3bd0fb74fa30d1f8b213e6
verdict: comment
---
## Verdict
Comment: the endpoint-audit capture pipeline is generally readable and scoped, but the old inline audit removal left a few dead parameters that should be cleaned up for clarity.
## Evidence
- Reference-frame note: YAGNI / dead-parameter minimization; once the endpoint audit middleware owns capture, handler signatures should not keep parameters that only served the deleted inline logging path.
- `src/Aevatar.Mainnet.Host.Api/Cqrs/CqrsObservatoryApiEndpoints.cs:62` and `:107`: `loggerFactory` remains in `ListScopes` / `ListReadModels`, but the method bodies no longer use it after the local `Audit(...)` helper was deleted.
- `src/Aevatar.Mainnet.Host.Api/Cqrs/CqrsObservatoryApiEndpoints.cs:68` and `:110`: `callerScopeId` is still materialized from `TryGetCallerScopeId(...)`, but is never read; using discard would better express that the call is only a guard.
- `src/workflow/Aevatar.Workflow.Infrastructure/CapabilityApi/WorkflowRunObservatoryEndpoints.cs:275`: `AuthorizeCrossScopeAsync(...)` still accepts `targetScope`, `runId`, `action`, and `loggerFactory`, yet none of those values are read after the inline audit logger was removed. The call sites at `:170`, `:196`, `:222`, and `:244` now carry dead ceremony.
## What would change your verdict
Remove the unused endpoint parameters and call-site arguments, or replace unused guard outputs with discards where the value is intentionally ignored.review_round: 6 |
Contributor
Author
🤖 tests reviewer r5：通过TL;DR
测试证据我用的参考框架是 test pyramid 加行为驱动的边界测试：这种跨 ASP.NET Core middleware 的能力，应该用聚焦单元/管线测试覆盖共享审计语义，再用真实 HTTP route integration 测试抽样证明 endpoint metadata、filter、middleware、auth 顺序能一起工作。 核心管线覆盖足够： 隐私和 sanitizer 回归也有行为断言： 真实端点家族也已经补齐 route-level 集成测试： 测试质量红线未发现问题：没有新增 📎 Full raw codex output (archival)---
pr: 2608
role: tests
head_sha: c6192bee9b44d71b5a3bd0fb74fa30d1f8b213e6
verdict: approve
---
## Verdict
approve: test coverage and test quality are adequate for the endpoint audit capture diff.
## Evidence
- Reference-frame note: test pyramid plus behavior-driven boundary testing for a cross-cutting ASP.NET Core middleware; shared audit semantics are covered in focused middleware tests, then sampled through real HTTP route integrations for each attached endpoint family.
- `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:29` covers the accepted attempted + terminal audit pair with exact operation names, outcome, target identity, actor hash, capture plane, and result summary.
- `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:69`, `:86`, `:105`, and `:122` cover denied, thrown exception, explicit 500/error, and non-2xx fallback success classification with meaningful assertions on `AuditOutcome`, `ErrorCode`, and `ErrorSummary`.
- `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:137`, `:183`, and `:206` cover multi-route target composition plus JWT/email redaction across appended record fields, so the sensitive-value rule is regression-tested instead of just executed.
- `test/Aevatar.Bootstrap.Tests/EndpointAuditCaptureMiddlewareTests.cs:158`, `:171`, `:223`, and `:240` cover unannotated endpoint zero records, unauthenticated zero records, appender failure isolation, and missing audit ports preserving business response.
- `test/Aevatar.Capabilities.Tests/CqrsObservatoryApiEndpointsAuditTests.cs:29` drives the real CQRS observatory route through routing/auth/audit middleware and asserts query execution, operation names, target, sanitized request summary, capture plane, and no token/email leakage.
- `test/Aevatar.GAgents.ChannelRuntime.Tests/ChannelCallbackEndpointsTests.cs:85` drives the real NyxID relay registration route and asserts attempted/terminal audit records plus no bearer token, email, app secret, or verification token in appended records.
- `test/Aevatar.GAgentService.Integration.Tests/ScopeServiceEndpoints/ScopeServiceBindingEndpointTests.cs:91` verifies a real Scope binding route appends the expected attempted/terminal records, target id, route summary, capture plane, and token non-leakage.
- `test/Aevatar.Workflow.Host.Api.Tests/WorkflowRunObservatoryEndpointsAdminTests.cs:224` verifies workflow observatory cross-scope route audit capture, target construction, sanitized request summary, capture plane, and token/email non-leakage while preserving the direct authorization matrix tests.
- Static review found no newly added `[Skip]`/manual-disable markers, no added `Task.Delay(...)` or `WaitUntilAsync(...)` pacing, and no polling allowlist change. Focused verification passed: `dotnet test` for `EndpointAuditCaptureMiddlewareTests` (13 tests), CQRS/Channel/Workflow route audit filters (1 test each), and `bash tools/ci/test_stability_guards.sh`.
⟦AI:AUTO-LOOP⟧
REVIEW_DONE:2608:tests:approvereview_round: 5 |
eanz17
merged commit 972f88e
into
crnd/milestone32-platform-audit-trail
13 of 14 checks passed
This was referenced Jul 3, 2026
eanz17
added a commit
that referenced
this pull request
Jul 3, 2026
* 实现 issue #2593 (#2601) Co-authored-by: eanzhao <yiqi.zhao@aelf.io> * 新增平台审计链路合同主干 (#2602) * 实现 issue #2594 * 实现 issue #2594 * PR #2602 review-fix output --------- Co-authored-by: eanzhao <yiqi.zhao@aelf.io> * 为 workflow 审计报告增加统一脱敏入口 (#2605) * 实现 issue #2600 * 实现 issue #2600 --------- Co-authored-by: eanzhao <yiqi.zhao@aelf.io> * 添加审计追踪查询能力面 (#2604) * 实现 issue #2599 * 实现 issue #2599 * 实现 issue #2599 * PR #2604 review-fix output * 实现 issue #2599 --------- Co-authored-by: eanzhao <yiqi.zhao@aelf.io> * 接入工具执行审计终端观察链路 (#2607) * 实现 issue #2597 * 实现 issue #2597 * 实现 issue #2597 * 实现 issue #2597 * 实现 issue #2597 --------- Co-authored-by: eanzhao <yiqi.zhao@aelf.io> * 接入 committed audit projection artifact sink (#2606) * 实现 issue #2598 * 实现 issue #2598 * 实现 issue #2598 * 实现 issue #2598 * PR #2606 review-fix output * 实现 issue #2598 * PR #2606 review-fix output * 实现 issue #2598 * 实现 issue #2598 * 实现 issue #2598 * 实现 issue #2598 * 实现 issue #2598 * 实现 issue #2598 * 实现 issue #2598 * PR #2606 review-fix output * 实现 issue #2598 --------- Co-authored-by: eanzhao <yiqi.zhao@aelf.io> * 实现端点审计捕获管线 (#2608) * 实现 issue #2596 * 实现 issue #2596 * 实现 issue #2596 * 实现 issue #2596 * 实现 issue #2596 * 实现 issue #2596 * 实现 issue #2596 * 实现 issue #2596 * 修复 PR 2608 第 2 轮评审反馈 * PR #2608 review-fix output --------- Co-authored-by: eanzhao <yiqi.zhao@aelf.io> --------- Co-authored-by: eanzhao <yiqi.zhao@aelf.io>
jason-aelf
added
crnd:phase:merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changed files
Aevatar.Audit.Abstractions/EndpointAudit强类型端点审计声明、过滤器、目标解析与敏感值 sanitizer。attempted与 exactly-one terminal outcome，并保持 appender failure 不影响业务响应。docs/architecture/audit-trail.md。Test results
bash -lc "dotnet build aevatar.slnx --nologo": passed。bash -lc "dotnet test aevatar.slnx --nologo": passed。bash tools/ci/architecture_guards.sh: passed。bash tools/ci/test_stability_guards.sh: passed。Deviations
test/Aevatar.Architecture.Tests/Rules/MainnetAgentProjectionDocumentStoreTests.cs，让现有 ACL guard 测试在显式Strict模式下验证 fail-closed 行为，以匹配当前默认 Warn 语义。⟦AI:AUTO-LOOP⟧