fix: enforce the integrity and isolation claims the samples were making (finding 3)

[arst]

Third PR of the four-PR remediation of docs/reviews/2026-08-25-second-review.md. It answers four findings that share one shape: a sample claimed a property its code did not enforce — and, in the fourth case, destroyed the data it claimed to protect.

SkillLearning — approval bound nothing to the approved content

The lifecycle was candidate → validated → tested → approved → active, with a reviewer's name recorded at approval. Nothing tied any of that to what SKILL.md actually said. Anyone who could write the file could edit an already-approved skill in place, and the agent would load it.

SkillManifest now carries ContentSha256, computed once when the candidate is written. ReadVerified refuses content whose digest moved, and every content read goes through it — Validate, MarkTested, ReadActive, and Approve, Activate, Retire. Verification happens before the manifest is persisted, so a file edited between MarkTested and Approve is refused at approval rather than at load time, and manifest.json never records a sign-off against content the reviewer never saw.

The doc says what this buys and what it does not: the digest detects tampering, it does not prevent it — whoever can write SKILL.md can write manifest.json too. Sign the manifest, or keep it outside the agent's write scope.

SemanticCaching — a cache key that could not isolate anything

The partition key covered the conversation shape and three ChatOptions fields. No tenant, no authorization scope, no tool policy, no data revision. Entries never expired and never evicted.

• CacheNamespace(TenantId, PrincipalScopeHash, SystemPromptHash, ToolSchemaHash, ModelVersion, DataRevision) is a required constructor argument — the isolation dimensions cannot be forgotten, only chosen.
• Every field is hashed before joining, so a TenantId containing the delimiter cannot collide with a different tenant's key.
• The prior-turn digest covers all AIContent kinds — a history differing only in a tool call no longer shares a partition.
• The options canonicalisation went from 3 of ChatOptions' 20 properties to 16, including Tools (name + schema), ToolMode, MaxOutputTokens, Seed and StopSequences. A runtime tool set that diverges from the declared ToolSchemaHash can no longer collide.
• Per-entry TTL dropped on read, oldest-first eviction past a bound, one lock guarding the store and the counters, and the response cloned on store as well as on hit.

EvaluationAndMonitoring — traces defaulted to full plaintext capture

The safe modes existed; you had to know to ask for them. RunTrace now defaults to RedactedContent. Full capture is its own mode word, record-full, gated on AGENTIC_PATTERNS_ACKNOWLEDGE_FULL_TRACE_CAPTURE=I_UNDERSTAND_THIS_WRITES_PROMPTS_AND_OUTPUTS_IN_PLAINTEXT — exact ordinal match, so a wrong case or a trailing space is rejected — with a banner in the same shape as CodeAct's unsafe-execution warning, printed before anything is recorded.

The doc is explicit that redaction is best-effort pattern matching, not a guarantee: it recognises a limited set of shapes and misses whatever it was not taught. Traces are plaintext JSON on disk; treat the directory as production data.

GuardRails — the guard destroyed the response it was guarding

PII redaction and output truncation concatenated every TextContent, redacted the string, and returned a brand-new single-message response. Function calls, function results, finish reason, usage, model id: gone, on every turn where a regex happened to match.

Redaction now maps each message's Contents, rewriting only TextContent; truncation trims the last text item rather than collapsing the message. Both the IChatClient path and the agent path route through the same core, so neither flattens. ConversationId and ContinuationToken are carried through — a stateful client would otherwise lose its server-side thread, and a background response its polling handle, intermittently and only when PII was detected. RawRepresentation is deliberately not copied: it would hand the caller a route back to the un-redacted payload.

The SemanticKernel twin gets the same semantic fix in its own shape — new FunctionResult(context.Result, text) preserves the original's metadata and culture instead of discarding them.

Verification

• dotnet build "Agentic Patterns.slnx" -c Release → 85 projects, 0 errors, 0 warnings
• dotnet test "Agentic Patterns.slnx" -c Release --no-build → 238/238
• Every task went through an independent review; three needed a fix round. The whole-branch review then mutation-tested each guarantee — breaking the digest check, the namespace fields, expiry, eviction, the trace default, the acknowledgement gate and the per-content mapping, each in turn — and confirmed a named test fails for every one.
• A uniformity sweep across all 85 projects looked for the same four defect classes elsewhere: another lifecycle trusting unverified on-disk content, another cache keyed without caller identity, another recorder with no privacy mode, another guard rebuilding from .Text. There is no fifth sample.
• Zero test deletions across the branch ([Fact]/[Theory] 184 → 203).

Known gaps, stated rather than papered over

• The digest detects tampering; it does not prevent it. Manifest signing is named as the upgrade path, not implemented.
• The SemanticKernel twin has no automated test — the project is deliberately outside the test assembly, since its types are internal and expose nothing to assert on. Its fix rests on the documented FunctionResult(FunctionResult, object?) contract.
• The twin achieves metadata parity, not structural parity: it still coerces its result value to a string.
• A cache hit returns FinishReason = null, and expired entries in a never-queried partition are reclaimed only on the next read — both bounded by maxEntriesPerPartition, both noted in // ponytail: comments with their upgrade paths.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc