refactor: type-system trust-context hardening (#994)

openspec/changes/type-system-stiffening/.openspec.yaml

@@ -0,0 +1,2 @@
+schema: spec-driven
+created: 2026-05-15

openspec/changes/type-system-stiffening/design.md

@@ -0,0 +1,213 @@
+## Context
+
+Trust context in Netclaw flows from an inbound channel adapter through the
+session pipeline into every tool-access and memory-scoping decision. The data
+path is:
+
+```
+ChannelInput (adapter)
+  → MessageSourceFactory.Create
+    → MessageSource (per-turn trust snapshot)
+      → TrustContextDeriver.Derive → EffectiveTrustContext
+        → ToolAccessPolicy / memory gates / background jobs / sub-agents
+```
+
+Today, `ChannelInput`'s four trust fields (`Audience`, `Boundary`, `Principal`,
+`Provenance`) are nullable with no default. `MessageSourceFactory.Create`
+materialises a value with `input.X ?? options.DefaultX`, where the
+`SessionPipelineOptions.DefaultX` properties carry permissive sentinels
+(`TrustAudience.Public`, `SourceProvenance.StrictDefault()`). `MessageSource`'s
+own trust fields carry the same sentinels as property-init defaults. The
+compiler therefore cannot distinguish an adapter that deliberately omits trust
+context from one that simply forgot — and a forgotten field silently produces
+the most permissive trust label. PR #993 was exactly this failure: a
+Personal-audience Slack DM lost its audience and was gated as Public.
+
+Three persisted record types (`BackgroundJobDefinition`, `ActiveJobInfo`,
+`ReminderDefinition`) carry the same sentinel-default shape on disk, with an
+*elevated* default (`TrustAudience.Personal`) — a forgotten field there is a
+silent privilege escalation, not just a degradation.
+
+Constraints:
+- The constitution forbids silent fallbacks, especially on security paths.
+- No on-disk or on-wire format change is permitted in this change. Legacy
+  documents must remain loadable through an explicit, loud path.
+- Actor message types crossing the wire are protobuf-mapped; their record
+  *shape* cannot change, but the trust fields involved here are not
+  wire-serialized as nullable in a way this change alters.
+
+## Goals / Non-Goals
+
+**Goals:**
+
+- Make the four trust fields (`Audience`, `Boundary`, `Principal`,
+  `Provenance`) impossible to omit at any actor boundary — enforced by the
+  compiler, not by review.
+- Delete the `SessionPipelineOptions.DefaultX` escape hatch and the
+  `MessageSourceFactory` fallback arms so there is no code path that
+  synthesizes trust context.
+- Convert elevated-fallback escalation sites to explicit `throw`.
+- Type `ToolExecutionContext.Audience` and `RunSubAgent.Audience` as parsed
+  `TrustAudience`, moving parse failure to construction time.
+- Make persisted trust fields `required` while keeping legacy JSON documents
+  loadable through a loud, operator-visible path (no on-disk migration).
+
+**Non-Goals:**
+
+- The broad value-object adoption pass (wrapping `SenderId`, `TurnId`,
+  `ToolCallId`, etc.) — tracked separately.
+- The Pass 5/6 primary-constructor and `required`-keyword cleanups on
+  non-security records — cosmetic, separate change.
+- Any change to wire or on-disk serialization format.
+- Changing the *values* of fail-closed conservative fallbacks in
+  `TrustContextDeriver` (`UntrustedExternal`, `StrictDefault()` when source is
+  genuinely absent) — those are correct.
+
+## Decisions
+
+### D1 — `ChannelInput` / `MessageSource`: `required` properties, not primary constructors
+
+Both records have ~15 properties. A primary constructor with 15 positional
+parameters is unreadable. Use `required` on the four trust fields and leave the
+rest as property-init. `required` gives the same compile-time enforcement
+(every object initializer must set the field) without the positional-argument
+noise. *Alternative considered*: primary constructor — rejected on readability
+for types this wide.
+
+### D2 — `SourceProvenance`: 2-parameter primary constructor
+
+`SourceProvenance` has two trust fields (`TransportAuthenticity`,
+`PayloadTaint`) and two optional metadata fields (`SourceScope`, `SourceKind`).
+Callsite inspection confirms every construction site sets both trust fields
+explicitly and most set `SourceKind`; `SourceScope` is frequently omitted.
+A 2-parameter primary constructor forces the trust fields and keeps the
+metadata as optional `init`:
+
+```csharp
+public sealed record SourceProvenance(
+    TransportAuthenticity TransportAuthenticity,
+    PayloadTaint PayloadTaint) : IWireType
+{
+    public string? SourceScope { get; init; }
+    public string? SourceKind { get; init; }
+}
+```
+
+The `StrictDefault()` factory is removed; the one genuinely conservative
+fallback (in `TrustContextDeriver` when `source` is null) constructs
+`new SourceProvenance(TransportAuthenticity.Unverified, PayloadTaint.Public)`
+explicitly so the conservatism is visible at the callsite.
+
+### D3 — Delete `SessionPipelineOptions.DefaultX` rather than make it `required`
+
+The four `Default*` properties exist only to feed the `MessageSourceFactory`
+fallback arms. Making them `required` would preserve the escape hatch. Deleting
+them forces each of the five `BuildOptions()` consumers (Slack, Discord,
+SignalR, Webhook, Reminder binding actors) to stamp explicit trust context onto
+the `ChannelInput` they construct. The per-adapter values that previously lived
+in `DefaultX` move to the adapter as named local constants or computed values.
+
+### D4 — Elevated-fallback sites become `throw`, not fail-closed defaults
+
+`SessionToolExecutionPipeline` (background-job submission) and `SubAgentActor`
+both default a missing audience to `TrustAudience.Personal` — an escalation.
+After D1/D3 the only way `source` is null at these sites is a programming
+error. They become `throw new InvalidOperationException(...)`. This is not a
+fail-closed default (which would be `Public`); it is a loud assertion that the
+invariant held by D1 was violated. `NullPromptInjectionDetector` substitution
+becomes `throw` for the same reason — the real detector is a DI singleton, so
+null means broken wiring.
+
+### D5 — `ToolExecutionContext.Audience`: `string?` → `TrustAudience?`
+
+`ToolExecutionContext` is a mutable `class` (not a record); tools mutate it.
+Changing `Audience` from wire-string `string?` to `TrustAudience?` moves the
+parse to the point where the context is built (`SessionToolExecutionPipeline`,
+`SubAgentActor`), so an unparseable value fails there rather than silently
+degrading to `Public` inside `ToolAccessPolicy`. `Boundary` stays `string?` —
+it is a free-form partition label with no parse step. `SecurityPolicyDefaults.ParseAudienceOrPublic`
+and `ResolveAudienceWithFallback` become dead code on the read path and are
+deleted. `RunSubAgent.Audience` changes correspondingly.
+
+### D6 — Persisted records: reject legacy documents at load, no backfill
+
+`BackgroundJobDefinition`, `ActiveJobInfo`, `ReminderDefinition` make their
+trust fields `required` — this is the type-system win: every in-process
+construction is compiler-enforced.
+
+`ActiveJobInfo` is protobuf-serialized; proto3 has no notion of an absent
+field, and a legacy record deserializes its audience to enum `0`, which is
+`TrustAudience.Public` (fail-closed). So `ActiveJobInfo` needs no special
+handling — `required` is purely a compile-time change there.
+
+`BackgroundJobDefinition` and `ReminderDefinition` are JSON
+(`BackgroundJobDefinitionStore`, `ReminderDefinitionStore`). A legacy document
+that omits the trust keys (or carries an explicit `null`) is **rejected** at
+load — not coerced to a substitute audience. On the deserialization path each
+store parses the document into a `JsonObject` and checks for the trust keys via
+a shared helper (`LegacyTrustFieldGuard.MissingTrustFields`); if any are
+absent, the store logs an **error** naming the file and the missing fields,
+and excludes the document — `Get` returns null, `List` skips it. The reminder
+store returns the rejection without deleting the file (it is operator-authored
+data, distinct from corrupt JSON, so the operator can repair or remove it).
+
+There is no backfill. A job or reminder with no persisted trust context cannot
+be run safely: its trust tier is unknown, and these features are typically
+disabled at the most-restrictive audience — so a `Public` substitute would
+fabricate a nonsensical state (a feature that is gated off), and a `Personal`
+substitute would silently escalate privilege. *Alternatives considered*:
+(a) backfill `Public` — rejected, it produces a job/reminder in a
+contradictory state (running at an audience where the feature is disabled);
+(b) backfill `Personal` — rejected, an elevated default is precisely the
+anti-pattern this change exists to remove. Rejecting the document is the only
+choice that neither escalates nor fabricates. Pre-#994 a legacy reminder
+already failed (it threw at execution for a missing audience); rejecting it at
+load is the same outcome, surfaced earlier and without a per-fire crash.
+
+### D7 — Sequencing as four independent PRs
+
+PR-A (`ChannelInput`/`MessageSource`/`SourceProvenance`/`MessageSourceFactory`/
+`SessionPipelineOptions` + adapters), PR-B (elevated-fallback throws), PR-C
+(`ToolExecutionContext`/`RunSubAgent` typing), PR-D (persisted records:
+`required` trust fields + legacy-document rejection). PR-A is a prerequisite
+for PR-B (it establishes the non-null `source` invariant). PR-C and PR-D are
+independent of A/B. Each is independently reviewable and compiler-verified.
+
+## Risks / Trade-offs
+
+- **Large mechanical diff across channel adapters** → The compiler drives the
+  refactor: every missing `required` field is a build error pointing at the
+  exact callsite. Fix per error, no guesswork. Tests adapt the same way.
+- **Legacy persisted documents stop loading** → A pre-#994 job/reminder file
+  with no trust fields is rejected at load and no longer runs. Mitigation: the
+  rejection is logged at error level naming the file and the missing fields,
+  and the file is preserved so the operator can repair (add the fields) or
+  remove it. For reminders this matches the pre-#994 outcome (a missing
+  audience already failed at execution); the failure simply moves earlier and
+  loses the per-fire crash loop.
+- **`throw` on a missing turn source could crash a session if the invariant is
+  wrong** → The invariant (every tool execution and background-job submission
+  has a turn source) is established by D1/D3 making `MessageSource` mandatory.
+  If a path genuinely has no source, the `throw` surfaces it in testing rather
+  than letting it escalate silently in production. Acceptable: loud failure in
+  a test beats silent escalation in prod.
+- **`ToolExecutionContext.Audience` retype touches every tool** → Blast radius
+  is bounded to tools that read `context.Audience` (enumerated in the proposal
+  impact section). Mechanical; compiler-verified.
+
+## Migration Plan
+
+1. PR-A → PR-B → PR-C → PR-D land in order; each is a normal `dev`-branch PR
+   with green build + tests.
+2. No deployment-time migration. On first daemon start after PR-D, any legacy
+   persisted job/reminder document missing trust fields is rejected at load
+   with an error log; the file is preserved. An operator who wants such a job
+   or reminder back adds the `audience`/`boundary` fields or recreates it.
+   Regression tests exercise the legacy-document rejection for both stores.
+3. **Rollback**: each PR is independently revertable. PR-D's rejection is
+   confined to the two stores' deserialization paths; reverting it restores
+   the prior behavior. No on-disk data is rewritten or deleted by this change.
+
+## Open Questions
+
+- None.

openspec/changes/type-system-stiffening/proposal.md

@@ -0,0 +1,115 @@
+## Why
+
+PR #993 fixed a production bug where a Personal-audience Slack DM was silently
+downgraded to Public, denying the operator's `shell_execute`. The root cause was
+not logic — it was type shape: `ChannelInput.Audience` is `TrustAudience?`
+(nullable, optional) and `MessageSourceFactory.Create` invents a default via
+`input.Audience ?? options.DefaultAudience`. The compiler could not tell a
+forgetful adapter from a deliberate caller. The constitution's "No silent
+fallbacks" rule names this anti-pattern, but trust-bearing records across the
+codebase still carry security-relevant fields as nullable-with-fallback or
+sentinel-default rather than `required`. The audience field bit us first; it is
+unlikely to be the last. This change makes the type system a primary correctness
+gate so the next PR #993 cannot compile.
+
+## What Changes
+
+- **BREAKING** (internal API) — `ChannelInput`'s trust fields (`Audience`,
+  `Boundary`, `Principal`, `Provenance`) become `required` and non-nullable.
+  Every inbound channel adapter must supply explicit trust context.
+- **BREAKING** (internal API) — `MessageSource`'s four trust fields become
+  `required`; the permissive sentinel-default initializers
+  (`= TrustAudience.Public`, `= SourceProvenance.StrictDefault()`, etc.) are
+  removed.
+- `SourceProvenance` converts to a 2-parameter primary constructor
+  (`TransportAuthenticity`, `PayloadTaint` required; `SourceScope`/`SourceKind`
+  remain optional `init` metadata). The `Unknown`/`Unknown` sentinel defaults
+  are removed.
+- The four `?? options.DefaultX` fallback arms in `MessageSourceFactory.Create`
+  are deleted (unreachable once `ChannelInput` is required).
+- **BREAKING** (internal API) — `SessionPipelineOptions.DefaultAudience`,
+  `DefaultBoundary`, `DefaultPrincipal`, `DefaultProvenance` are removed. They
+  exist only to feed the deleted fallback arms.
+- Elevated-fallback escalation sites
+  (`source?.Audience ?? TrustAudience.Personal` in `SessionToolExecutionPipeline`,
+  `msg.Audience ?? TrustAudience.Personal.ToWireValue()` in `SubAgentActor`)
+  are replaced with explicit `throw` — a missing turn source is a programming
+  error, not a runtime condition.
+- `NullPromptInjectionDetector` substitution via `?? new NullPromptInjectionDetector()`
+  is replaced with `throw`; the null detector silently disables injection
+  scanning and must never be selected by accident.
+- `ToolExecutionContext.Audience` changes from wire-string `string?` to parsed
+  `TrustAudience?`, so an unparseable value fails at construction rather than
+  silently degrading to `Public` at gate-check time. `RunSubAgent.Audience`
+  changes correspondingly.
+- Persisted records (`BackgroundJobDefinition`, `ActiveJobInfo`,
+  `ReminderDefinition`) make their trust fields `required` — enforcing every
+  in-process construction at compile time. A legacy JSON document missing trust
+  fields is **rejected** at load: the job/reminder store logs an error naming
+  the file, excludes the document (it is not loaded or scheduled), and
+  preserves the file for operator inspection. There is no backfill — a job or
+  reminder with no persisted trust context cannot be run safely, and these
+  features are typically disabled at the most-restrictive audience, so coercing
+  a substitute audience would fabricate a nonsensical or privilege-escalating
+  state. No on-disk migration and no doctor tooling.
+
+## Capabilities
+
+### New Capabilities
+
+- `trust-context-integrity`: Establishes the cross-cutting invariant that
+  trust-bearing context (audience, principal, boundary, provenance, transport
+  authenticity, payload taint) is mandatory and non-optional at every actor
+  boundary, that no security-relevant field may carry a permissive or elevated
+  sentinel default, and that missing trust context fails loud rather than
+  silently defaulting.
+
+### Modified Capabilities
+
+- `netclaw-input-adapters`: Inbound channel adapters SHALL supply complete,
+  explicit trust context on every `ChannelInput`; the pipeline SHALL NOT
+  synthesize a default audience/principal/provenance/boundary.
+- `audience-context-filtering`: The session pipeline SHALL derive audience only
+  from an explicitly-supplied turn source; there is no `DefaultAudience`
+  fallback.
+- `background-job-execution`: Background-job submission SHALL fail loud when no
+  turn source is present rather than defaulting to `Personal` audience;
+  persisted job records SHALL carry explicit, required trust fields, and a
+  legacy job document missing them SHALL be rejected at load rather than
+  coerced.
+- `reminder-execution-history`: Persisted reminder definitions SHALL carry
+  explicit, required trust fields; a legacy document missing them SHALL be
+  rejected at load — logged as an error, excluded from scheduling, and the file
+  preserved — never coerced to a substitute audience.
+- `netclaw-tools`: `ToolExecutionContext` SHALL carry audience as a parsed
+  `TrustAudience`, not a wire string; an unparseable audience SHALL fail at
+  construction.
+- `netclaw-subagents`: Sub-agent spawn messages SHALL carry an explicit parsed
+  audience; a missing audience SHALL fail loud rather than defaulting to
+  `Personal`.
+
+## Impact
+
+- **Affected code**: `Netclaw.Actors` (`Channels/`, `Sessions/Pipelines/`,
+  `SubAgents/`, `Jobs/`, `Reminders/`, `Persistence/`), `Netclaw.Tools.Abstractions`
+  (`ToolExecutionContext`), `Netclaw.Configuration` (`SecurityPolicyDefaults` —
+  `ParseAudienceOrPublic` deleted, `ResolveAudienceWithFallback` retyped),
+  `Netclaw.Channels.Slack` / `Netclaw.Channels.Discord` (binding actors and
+  history fetchers), `Netclaw.Daemon` (`SignalRSessionActor`,
+  `WebhookExecutionActor`).
+- **APIs**: Internal-only. No wire-format or on-disk-format change. No public
+  NuGet surface.
+- **Persistence**: No on-disk or on-wire format change. A legacy
+  `BackgroundJobDefinition` / `ReminderDefinition` JSON document that predates
+  this change and lacks trust fields is rejected at load — logged as an error,
+  excluded, the file preserved. The job/reminder does not run. `ActiveJobInfo`
+  is protobuf-serialized; proto3 cannot express an absent field, so a legacy
+  record deserializes its audience to enum `0` = `Public` (fail-closed) — it
+  needs no special handling.
+- **Tests**: `Netclaw.Actors.Tests`, `Netclaw.Channels.Slack.Tests`,
+  `Netclaw.Channels.Discord.Tests`, `Netclaw.Daemon.Tests` adapt mechanically
+  to the required-property and primary-constructor shapes.
+- **Out of scope**: The broader value-object adoption pass (Pass 7 in the
+  planning doc — wrapping raw-string identifiers in value objects) is tracked
+  separately and not part of this change. This change is the trust-tier
+  hardening only (Passes 1–4).

openspec/changes/type-system-stiffening/specs/audience-context-filtering/spec.md

@@ -0,0 +1,24 @@
+## ADDED Requirements
+
+### Requirement: Audience derivation has no default-audience fallback
+
+The session pipeline SHALL derive a turn's audience only from the explicitly
+supplied turn source. There SHALL be no pipeline-level `DefaultAudience`,
+`DefaultBoundary`, `DefaultPrincipal`, or `DefaultProvenance` configuration
+property. A turn that reaches audience derivation without a turn source SHALL
+fail loudly rather than adopt a default audience.
+
+#### Scenario: No default-audience configuration exists
+
+- **WHEN** session pipeline options are constructed
+- **THEN** there is no `DefaultAudience` (or sibling `Default*` trust) property
+  to set
+- **AND** trust context can only enter the pipeline by way of an inbound
+  `ChannelInput`
+
+#### Scenario: Audience derivation uses the supplied turn source
+
+- **GIVEN** a turn with an explicit turn source carrying `TrustAudience.Personal`
+- **WHEN** the pipeline derives the effective audience
+- **THEN** the derived audience reflects the Personal source audience
+- **AND** no default-audience value participates in the derivation