Skip to content

Issue #3543: [NyxID Chat] 会话 read model 不暴露已密封的 context attachments,准入失败原因不可区分 - #3546

Open
eanz17 wants to merge 2 commits into
feat/2026-08-27_issue-3542from
feat/2026-08-27_issue-3543
Open

Issue #3543: [NyxID Chat] 会话 read model 不暴露已密封的 context attachments,准入失败原因不可区分#3546
eanz17 wants to merge 2 commits into
feat/2026-08-27_issue-3542from
feat/2026-08-27_issue-3543

Conversation

@eanz17

@eanz17 eanz17 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Issue

Closes #3543 — [NyxID Chat] 会话 read model 不暴露已密封的 context attachments,准入失败原因不可区分

Implementation summary

See .implement-loop/runs/implement-issue-3543.md.

Stacked-PR position

  • Base: feat/2026-08-27_issue-3542 (previous issue's branch)
  • Head: feat/2026-08-27_issue-3543
  • Auto-loop iteration: implement-loop / milestone Typed Context & Deterministic Computation (v1) follow-up

🤖 Generated by codex-implement-loop. Reviewer is a Claude subagent (see PR comments for round-N review reports).

eanzhao and others added 2 commits August 27, 2026 03:00
…入失败原因不可区分

Implemented per .implement-loop/runs/implement-issue-3543.md.

Closes #3543

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pin ai_messages.proto to the #3543 commit so architecture_guards.sh can pass.

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

eanz17 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Review of PR #3546 — round 1

Verdict: pass
Issue: #3543 — [NyxID Chat] 会话 read model 不暴露已密封的 context attachments,准入失败原因不可区分
Head: feat/2026-08-27_issue-3543 @ 4eccaad5ae5e9d3b449a2cd2dc400c40f79fdac5
Base: feat/2026-08-27_issue-3542
Reviewed by: Claude subagent (codex-implement-loop)

Verdict rationale

Three-dot range origin/feat/2026-08-27_issue-3542...origin/feat/2026-08-27_issue-3543 is two commits on merge-base 5f81f721dace9f715d38b00d6eddbb8cc5d89c6f (implementation + NyxID conformance pin of ai_messages.proto). Issue #3543 asked for two consumer-visible facts: (1) the create-only sealed attachment set on both GET /api/chat/conversations/{id}/state and GET /api/chat/conversations, as artifactId + revisionMode + pinnedRevisionId with no body; (2) attachment admission failures split from generic ADMISSION_UNAVAILABLE, with RUN_ERROR(code=ATTACHMENT_ADMISSION_DENIED) carrying a typed reason.

The head does both. The actor-scoped current-state projector copies state.ContextAttachments into a new reference-only document message (studio_projection_readmodels.proto field 45). ProjectionNyxIdChatConversationStateQueryPort maps that into NyxIdChatConversationStateSnapshot and the list overlay (NyxIdChatConversationAttentionSummary). Public state serializes the snapshot directly; public list assigns ContextAttachments = summary.ContextAttachments onto ConversationMeta. Create-time validation now returns ConversationContextAttachmentAdmissionFailureReason; the resolver / start-error / SSE writer keep that reason through ATTACHMENT_ADMISSION_DENIED plus snake_case reason. Profile and route failures still map to ADMISSION_UNAVAILABLE and do not claim an attachment cause.

No CLAUDE.md anti-pattern in the diff: query stays on the existing current-state replica (no IEventStore replay / priming), attachment facts stay proto (ConversationContextAttachment already has no body; admission reason is a new proto enum; read-model document is proto), Host endpoints only map, and no NyxID / chrono-* repo is edited.

Findings

None.

What's good

  • Issue intent — read model. NyxIdChatConversationContextAttachmentDocument is only artifact_id / revision_mode / pinned_revision_id. The projector constructs that document from committed NyxIdChatConversationGAgentState.ContextAttachments and never copies prompt/body/content. State JSON is the snapshot record (HandleGetStateAsyncNyxIdChatConversationStateResponse.Snapshot). List is the same snapshot type overlaid onto the existing index DTO. Both public routes named in the issue are covered by tests that assert the three identity fields (state) or identity + pinned revision (list) and that the response body does not contain inlineContent.
  • Issue intent — distinguishable admission. not_found / access_denied / unsupported_kind / over_limit from the issue body are implemented, plus the narrower pinned_revision_unavailable / invalid_request / inactive / read_model_unavailable. IContentArtifactQueryPort.GetAsync is scope+id lookup (not ACL-filtered), so artifact is null vs !IsAuthorized is actually distinguishable in production. Missing/null query port and query exceptions become read_model_unavailable. Profile/route still use AdmissionUnavailable. No actor is created (runtime.CreateCalls empty) after attachment rejection.
  • CLAUDE.md. Core admission reason is proto enum, not a string bag ("核心语义强类型…必须建模为 proto field / typed option / typed sub-message"). Projection copies committed state_root; query ports only read the document ("查询始终走 readmodel" / "禁止 query-time replay/priming"). Canon updates are in the three cited files. sources.json pin is this branch's proto hash, not an external-repo change ("外部仓库无改动权").
  • Tests are semantic, not call-count. SSE theory locks all eight wire reasons and ATTACHMENT_ADMISSION_DENIED without ADMISSION_UNAVAILABLE. Resolver theory preserves each lifecycle error through NyxIdChatStartError. Create-resolver matrix covers missing / unauthorized / unsupported kind / over-limit / pinned-unavailable / duplicate-id / inactive / missing query port and asserts no actor create. Projector test asserts the sealed tuple including empty pinnedRevisionId on FOLLOW_CURRENT. Query-port tests cover both GetAsync and GetAttentionSummariesAsync.
  • Stacked-PR hygiene. File list is the attachment read-model + admission split plus required canon/pin. No [Workflow] 生产 Host 未注册 deterministic_compute host_callback connector,#3526 能力上线但不可发现 #3542 leftovers. Proto field 45 is unused on the parent document; ConversationContextAttachment itself is still three fields.

Residual (not blocking): list test does not assert revisionMode (state test does; overlay is a single assignment). RevisionMode is inlined in the projector instead of the existing ToWireName overloads. One // Implement (issue #3543) comment is loop-local noise. Bare catch in ValidateContextAttachmentsAsync is pre-existing and now maps to read_model_unavailable rather than a boolean.

REVIEW_VERDICT:pass:sealed attachment refs on state+list; typed ATTACHMENT_ADMISSION_DENIED

@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (feat/2026-08-27_issue-3542@5f81f72). Learn more about missing BASE report.

@@                      Coverage Diff                      @@
##             feat/2026-08-27_issue-3542    #3546   +/-   ##
=============================================================
  Coverage                              ?   83.42%           
=============================================================
  Files                                 ?     1692           
  Lines                                 ?   194081           
  Branches                              ?    23328           
=============================================================
  Hits                                  ?   161910           
  Misses                                ?    21320           
  Partials                              ?    10851           
Flag Coverage Δ
ci 83.42% <100.00%> (?)

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

Files with missing lines Coverage Δ
...tions/Studio/Abstractions/IChatHistoryQueryPort.cs 86.91% <100.00%> (ø)
...stractions/INyxIdChatConversationStateQueryPort.cs 84.52% <100.00%> (ø)
...tors/NyxIdChatConversationCurrentStateProjector.cs 77.29% <100.00%> (ø)
🚀 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

eanzhao commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Merged into feature/integrate (now at 4eccaad5a). This PR's head commit is an ancestor of feature/integrate; GitHub could not auto-close it because its base is still the stacked branch. Needs a manual close by someone with write access.

Reviewed the merged result on top of 2cb4ebdb8 — no blocking findings, merged without changes. Verified: build 0 errors; Architecture 198, Capabilities 841, Studio 2479, AI 3492, Bootstrap 213 — 7224 passed, 0 failed; architecture_guards.sh and test_stability_guards.sh passed.

Note on the red runtime-main-flow-smoke check on this PR: it is not caused by this stack. I ran tools/ci/main_flow_runtime_smoke.sh against clean feature/integrate (2cb4ebdb8, none of #3541/#3542/#3543 applied) and it reproduces the identical failure — workflow_schedule_provisioning_failed / StudioScheduledCredentialMaterializationException. That is a pre-existing baseline defect in the scheduled-credential path and needs fixing separately.

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