Skip to content

Issue #3527: [ContentArtifact] 按 metadata key 的 list/filter 查询与同 scope+kind 唯一置顶语义 - #3538

Open
eanz17 wants to merge 2 commits into
feat/2026-08-25_issue-3526from
feat/2026-08-25_issue-3527
Open

Issue #3527: [ContentArtifact] 按 metadata key 的 list/filter 查询与同 scope+kind 唯一置顶语义#3538
eanz17 wants to merge 2 commits into
feat/2026-08-25_issue-3526from
feat/2026-08-25_issue-3527

Conversation

@eanz17

@eanz17 eanz17 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Issue

Closes #3527 — [ContentArtifact] 按 metadata key 的 list/filter 查询与同 scope+kind 唯一置顶语义

Implementation summary

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

Stacked-PR position

  • Base: feat/2026-08-25_issue-3526 (previous issue's branch)
  • Head: feat/2026-08-25_issue-3527
  • Auto-loop iteration: implement-loop / milestone Typed Context & Deterministic Computation (v1)

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

…e+kind 唯一置顶语义

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

Closes #3527

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

eanz17 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Review of PR #3538 — round 1

Verdict: rework
Issue: #3527 — [ContentArtifact] 按 metadata key 的 list/filter 查询与同 scope+kind 唯一置顶语义
Head: feat/2026-08-25_issue-3527 @ 259ca5a2c751dd9c7cdf61508b9bcbdd0a212cdd
Base: feat/2026-08-25_issue-3526
Reviewed by: Claude subagent (codex-implement-loop)

Verdict rationale (one paragraph)

Three-dot range origin/feat/2026-08-25_issue-3526...origin/feat/2026-08-25_issue-3527 is a single commit, 26 production files (under 30), no #3526 feature commits, and the labels/pin split matches the issue’s revised design (immutable creation labels + scopeId + pinKey pin actor, not role: primary on the artifact). Labels, projection, paired list filter, pin actor CAS/idempotency, advisory set checks, proto field numbers, and canon sections are largely in place. The PR still fails issue AC 2 at the only client-visible read surface: ContentArtifactPinService.GetAsync / ClearAsync treat an empty pinned_artifact_id as not-found, while successful clear (and persisted CAS rejection on an empty pointer) keep the actor and advance or retain pin_version. That mixes “404 means version 0” with “clear increments pin_version”, so after clear a client cannot observe pin_version / last_mutation_status, cannot safely re-set, and cannot replay the same mutation_id for clear. Illegal labelKey / pinKey also throw ArgumentException that the new HTTP handlers do not map (half-pair is 400; charset/reserved-prefix is an unhandled 500). Rework those two observation/rejection holes; do not widen scope.

Findings

F1 — Cleared (and empty) pins are hidden, so pin_version is not observable and clear mutation_id replay cannot reach the actor

Issue AC 2: “CAS 冲突拒绝与 mutation_id 幂等” and “pin_version 在 readmodel 单调可见”. Canon (docs/canon/content-artifacts.md) says successful set and clear advance pinVersion, CAS conflict is a persisted rejection without changing the pointer, and “the actor current-state read model exposes both authoritative pinVersion and committed projection stateVersion”. CLAUDE.md: “ACK 诚实:同步返回只承诺已达到阶段(默认 accepted + stable command id);committed / read-model observed 等强保证须通过独立契约或异步观察获取。” and “查询诚实:readmodel 可最终一致,但必须暴露权威源版本或刷新戳”.

Write side is B (tombstone-style): HandleClearAsync persists ContentArtifactPinClearedEvent with PinVersion = State.PinVersion + 1; ApplyCleared builds a new state with empty PinnedArtifactId / PinnedBy and the new version; the projector upserts that document (ProjectAsync only skips empty scope_id / pin_key, not an empty pointer).

Read/auth side is A (delete-style):

// ContentArtifactPinService.GetAsync
if (current == null || string.IsNullOrWhiteSpace(current.PinnedArtifactId))
    throw new ContentArtifactPinNotFoundException(...);

// ContentArtifactPinService.ClearAsync
if (current == null ||
    string.IsNullOrWhiteSpace(current.PinnedArtifactId) ||
    current.PinnedBy == null ||
    !PrincipalEquals(current.PinnedBy, normalizedRequester))
    throw new ContentArtifactPinNotFoundException(...);

HTTP GET/DELETE map that to CONTENT_ARTIFACT_PIN_NOT_FOUND. Mutation receipts are 202 + DispatchAccepted with no pin_version. The only observation path is GET.

Failure scenario:

  1. PUT pin with expectedPinVersion=0 → actor pin_version=1.
  2. DELETE with expectedPinVersion=1, mutationId=mutation-3 → actor pin_version=2, pointer empty, document still in studio-content-artifact-pins.
  3. GET → 404. Client cannot read pinVersion=2 or lastMutationStatus.
  4. Re-PUT with expectedPinVersion=0 (404 treated as “never pinned”) → 202, actor persists PinVersionConflict, pointer still empty, pin_version still 2.
  5. GET still 404 — rejection is not visible either.
  6. Retry of the successful clear (mutation-3) never reaches IsReplay: application 404s because PinnedArtifactId and PinnedBy are empty (ApplyCleared nulls both).

Tests encode the wrong product: ClearAndGet_ShouldHideAbsentOrOtherOwnersPin expects ContentArtifactPinNotFoundException when PinnedArtifactId is null. Actor tests cover in-process HandleClearAsync replay; they never go through the application gate.

Required shape (consistent with “clear advances pinVersion”): if a pin current-state document exists, GET must return it with a nullable pinnedArtifactId, pinVersion, stateVersion, and lastMutationStatus (404 only when no document / never mutated). Clear authorization after a successful clear cannot require a live pinnedBy on an empty pointer if mutation_id replay is in the AC — use the committed last-mutation requester (needs to be on the read model) or an equivalent explicit rule, and let matching mutation_id reach the actor. Do not reset pin_version on clear; the issue forbids that.

F2 — Illegal label/pin keys throw ArgumentException; new HTTP list/pin handlers only catch InvalidOperationException

Issue AC 1: “非法 label(超数 / 字符集 / 保留前缀 / 参数半对)拒绝”. Half-pair is InvalidOperationException in NormalizeQueryHandleListAsync → 400 INVALID_CONTENT_ARTIFACT_QUERY. Charset / reserved prefix / over-count go through ContentArtifactConventions.NormalizeLabelKey / NormalizeLabels / NormalizeLabelValue, which throw ArgumentException.

New surfaces that now call those helpers on the request path:

  • GET /api/scopes/{scopeId}/content-artifacts?labelKey=aevatar.period&labelValue=xNormalizeQueryArgumentException → unhandled (existing kind mismatch is InvalidOperationException → 400).
  • GET|PUT|DELETE /api/scopes/{scopeId}/content-artifact-pins/{pinKey} with pinKey=Uppercase or aevatar.primaryNormalizeIdentityArgumentException. HandleGetPinAsync / HandleSetPinAsync / HandleClearPinAsync catch ContentArtifactPinNotFoundException and InvalidOperationException only.

Failure scenario: GET .../content-artifacts?labelKey=aevatar.period&labelValue=2026-08-25 or GET .../content-artifact-pins/aevatar.primary as an authenticated scope principal. Service-layer tests (CreateAsync_ShouldRejectInvalidLabels, PinKeyAndActorAddress_ShouldUseCanonicalLabelKeyRules) assert ArgumentException and never hit the endpoint mapping, so this stays red at HTTP. Map ArgumentException on these handlers (or throw InvalidOperationException from the new normalize path, matching NormalizeKind) so illegal keys are 400, not 500.

What's good

  • Design split is the issue, not the original title. Labels are creation-time map<string,string> with count/charset/reserved-prefix/aevatar. rules; pin uniqueness is ContentArtifactPinGAgent keyed by scopeId + pinKey (BuildPinActorId), not ContentArtifactKind and not a role: primary label. Matches the 2026-08-25 revision and “聚合必须 actor 化”.
  • Proto hygiene. ContentArtifactState.labels = 20, CreateContentArtifact.labels = 14, ContentArtifactCurrentStateDocument.labels = 43 — append-only, no renumbering, no reserved needed. Pin messages are new. State/events stay protobuf.
  • Labels actually participate in create identity. HashCreateRequest clones the whole CreateContentArtifact (labels included); DuplicateCreate_ShouldTreatLabelsAsImmutableCreationFacts fails closed on a label change; ApplyCreated copies request.Labels; append/advance clone state (immutability test keeps labels).
  • List filter is a single paired equality, ACL still pre-cursor. ProjectionContentArtifactQueryPort.ListAsync adds Eq("labels.<key>", value) beside scope + AnyOf(owner, reader). InMemory round-trip plus ES flattened mapping + "labels.period":"2026-08-25" body check cover the dual-store AC without Task.Delay / live cluster pacing.
  • Pin actor CAS + persisted rejection is the right authority. expected_pin_version mismatch writes ContentArtifactPinMutationRejectedEvent without bumping pin_version or the pointer; mutation_id + hash replay is a no-op; different facts throw. Application set is advisory (same-scope ACTIVE owner) and does not pre-reject stale versions (SetAsync_ShouldDispatchStaleVersionForActorOwnedPersistedRejection). Receipts stay DispatchAccepted.
  • Stacked-PR / CLAUDE.md sample-grep. 1 commit, 33 files / 26 production, no [Workflow] 内联确定性计算 step(版本化签名算法,免外部 connector 微服务) #3526 commits, no NyxID/chrono-storage/chrono-ornn edits. No Task.Delay(, GetAwaiter().GetResult(), TypeUrl.Contains(, JSON actor state, middle-layer ID→fact dictionaries, SubscribeAsync<EventEnvelope>, *WriteActor/*ReadActor splits, production new HttpClient(, or [Skip]. The HandleEventAsync hit is a pre-existing test stub on the fake dispatch actor, not new production code. labels is a constrained partition map, not a new Metadata bag.

REVIEW_VERDICT:rework:clear hides pin_version; illegal keys 500 at HTTP

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

eanz17 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Review of PR #3538 — round 2

Verdict: pass
Issue: #3527 — [ContentArtifact] 按 metadata key 的 list/filter 查询与同 scope+kind 唯一置顶语义
Head: feat/2026-08-25_issue-3527 @ dac656491773362c8615ba80b5fc5adac8dcee9a
Base: feat/2026-08-25_issue-3526
Reviewed by: Claude subagent (codex-implement-loop)

Verdict rationale

Three-dot range is two commits (implement + round-1 fix), 26 production files, no #3526 feature commits. Labels remain creation-time constrained partition keys; pin uniqueness remains one ContentArtifactPinGAgent per scopeId + pinKey. Round-1 F1 is closed at every observation layer: GET returns an existing current-state document even when pinned_artifact_id is empty, so pinVersion / stateVersion / lastMutationStatus stay visible; the projector and query port materialize LastMutationRequestedBy; empty-pointer clear admits only that requester’s exact mutationId so actor IsReplay can run. Round-1 F2 is closed at HTTP: list and GET/PUT/DELETE pin handlers map normalization ArgumentException to 400. Issue AC, proto field numbers, dual-store list equality, CAS/persisted rejection, stacked-PR hygiene, and CLAUDE.md anti-pattern grep are clean. Residual: create still surfaces illegal labels as ArgumentException like other create required-field checks (not an F1/F2 hole, not a new HTTP surface from round 1).

Findings

None.

What's good

  • F1 shape matches the required observation contract. ContentArtifactPinService.GetAsync 404s only when no document exists. CanClear keeps live-pointer pinnedBy auth and, after clear, forwards only last-requester + last mutationId. Actor still hashes the full command; pin_version is not reset.
  • F1 is wired through the read model, not only the application fake. Document fields 30/31, projector copy from state.LastMutationRequestedBy, query-port mapping, ClearedPinCurrentState_ShouldRemainObservable, and HTTP HandleGetPinAsync_ShouldReturnClearedPinDocument.
  • F2 is mapped on the new HTTP surfaces. List → INVALID_CONTENT_ARTIFACT_QUERY; pin GET → INVALID_CONTENT_ARTIFACT_PIN_QUERY; pin PUT/DELETE → INVALID_CONTENT_ARTIFACT_PIN_COMMAND. Endpoint tests cover illegal list label key and all three pin operations.
  • Issue design split is intact. Immutable labels on create (= 20 / = 14 / document = 43); pin actor id content-artifact-pin:{scopeId}:{pinKey}; advisory ACTIVE/same-scope/owner checks stay in application; actor owns CAS + mutation_id + persisted PinVersionConflict.
  • Query honesty and ACL. List still applies scope + AnyOf(owner, reader) before cursor paging, then one Eq("labels.<key>", value). InMemory exact hit plus ES flattened mapping + "labels.period":"2026-08-25" body check cover AC dual-store without Task.Delay.
  • CLAUDE.md / stacked-PR. No NyxID / chrono-storage / chrono-ornn edits. No Task.Delay(, GetAwaiter().GetResult(), TypeUrl.Contains(, JSON actor state, middle-layer ID→fact dictionaries, production SubscribeAsync, *WriteActor/*ReadActor, new HttpClient(, or [Skip]. The only HandleEventAsync hit is the pre-existing test stub. labels is the issue’s bounded partition map, not a Metadata bag.

Round comparison

  • Findings carried over from round 1: F1 (now resolved) / F2 (now resolved)
  • New findings this round: none
  • Net direction: improving

REVIEW_VERDICT:pass:F1 pin_version visible after clear; F2 illegal keys 400

@eanzhao

eanzhao commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Merged into feature/integrate as part of the milestone 45 stack (integration commit adc6cbf10). This PR's head commit is now an ancestor of feature/integrate; GitHub could not auto-close it because its base is still the stacked branch rather than feature/integrate.

The stack was merged as a single update rather than bottom-up, because the intermediate states were broken: #3538 registered MapDelete on the content-artifact pin resource with an inferred request body, which minimal APIs reject for DELETE. Endpoint construction threw and the Mainnet host failed to start, and #3539 did not fix it — so merging bottom-up would have left feature/integrate red in between.

Fixed on the way in (adc6cbf10):

  • bound the pin clear body with [FromBody] and added ContentArtifactEndpointsRouteBindingTests — the handler tests called the method directly and never bound the route, so only MainnetHostCompositionTests caught it;
  • moved the ContentArtifact read contracts and query port into Aevatar.Studio.Application.Abstractions and dropped the Aevatar.Studio.Application reference [NyxID Chat] Conversation 级类型化结构化档案附件(typed structured context attachment) #3525 added to the NyxidChat agent, restoring LayerDependencyTests.NyxidChat_ShouldOnlyDependOnStudioApplicationAbstractions.

Verification on the merged result: build 0 errors; Architecture/Capabilities/Studio/Bootstrap/AI 7166 passed, 0 failed; architecture_guards.sh and test_stability_guards.sh 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.

2 participants