Skip to content

fix(channels): propagate audience on Slack history-fetched ChannelInput - #993

Merged
Aaronontheweb merged 1 commit into
netclaw-dev:devfrom
Aaronontheweb:fix/slack-history-fetcher-audience-propagation
May 14, 2026
Merged

fix(channels): propagate audience on Slack history-fetched ChannelInput#993
Aaronontheweb merged 1 commit into
netclaw-dev:devfrom
Aaronontheweb:fix/slack-history-fetcher-audience-propagation

Conversation

@Aaronontheweb

Copy link
Copy Markdown
Collaborator

Summary

SlackThreadHistoryFetcher.ConvertMessageAsync built ChannelInput without setting the resolved historical audience. After PR #984 / #45f4c57b made hydration run once per actor lifetime through a Hydrating gate, fresh DM sessions get their backfill from history (the live inbound is dropped as stale by _pendingCursorTs advancing inside hydration). With Audience null on the produced ChannelInput, MessageSourceFactory.Create fell back to SessionPipelineOptions.DefaultAudience = TrustAudience.Public, silently denying shell_execute with tool_not_allowed_for_audience_profile — even when the operator had Slack.ChannelAudiences["dm"] = "personal" configured.

Discord's twin (DiscordThreadHistoryFetcher) already sets Audience correctly. Slack regressed at the original introduction (#576) and the recent hydration refactor exposed it on the very first DM message of a fresh session.

Symptom observed in session D0AC6CKBK5K/1778736039.290249: a Personal-audience DM saw shell tools blocked.

Fix

One-line addition in SlackThreadHistoryFetcher.ConvertMessageAsync:

return new ChannelInput
{
    SenderId = senderId,
    ChannelId = channelId.Value,
    MessageId = $"{channelId.Value}:{message.Ts ?? string.Empty}",
    Audience = audience,   // ← was missing
    Contents = contents,
    ReceivedAt = receivedAt
};

The audience value is already resolved upstream in FetchRepliesAsync via ResolveHistoricalAudience and passed down — it just wasn't propagated to the resulting ChannelInput.

Tests

  • SlackThreadHistoryFetcherTests.Hydrated_channel_input_carries_resolved_historical_audience — regression test that fails on the pre-fix code.
  • DiscordThreadHistoryFetcherTests.Hydrated_channel_input_carries_resolved_historical_audience — lock-in test for the already-correct Discord twin so it can't regress symmetrically.

Follow-up

Filed as a separate issue: make ChannelInput.Audience a required property so future callsites can't omit it and silently fall back. That refactor touches every new ChannelInput { ... } callsite plus a handful of tests; out of scope for this hotfix.

Test plan

  • Targeted xUnit runs of both new tests pass.
  • dotnet slopwatch analyze clean (0 issues).
  • ./scripts/Add-FileHeaders.ps1 -Verify clean.

…hannelInput

SlackThreadHistoryFetcher.ConvertMessageAsync constructed ChannelInput
without the resolved historical audience. On fresh DM sessions, hydration
synthesizes the backfill from history (the live inbound is dropped as
stale by the post-#45f4c57b Hydrating-then-Active gating), so
MessageSource.Audience fell back to the channel pipeline's DefaultAudience
(Public) — silently denying shell_execute with
tool_not_allowed_for_audience_profile even when the operator had
ChannelAudiences["dm"] = "personal" configured.

Discord's twin (DiscordThreadHistoryFetcher) already sets Audience
correctly. Symptom observed in session D0AC6CKBK5K/1778736039.290249;
fix is one line.

Tests:
- Slack: regression test asserting hydrated ChannelInput.Audience
  matches the resolved historical audience under dm=personal config
  (would have caught this bug).
- Discord: lock-in test mirroring the same invariant.

Follow-up: filed separate refactor proposal to make
ChannelInput.Audience a required property so this class of bug becomes
a compile-time error instead of a silent runtime degrade.
@Aaronontheweb
Aaronontheweb merged commit 95edfc7 into netclaw-dev:dev May 14, 2026
6 checks passed
@Aaronontheweb
Aaronontheweb deleted the fix/slack-history-fetcher-audience-propagation branch May 14, 2026 05:59
Aaronontheweb added a commit that referenced this pull request May 15, 2026
* docs(openspec): add type-system-stiffening change (#994)

Umbrella OpenSpec change for issue #994 — make the type system a primary
correctness gate. Covers the trust-tier hardening: required trust fields on
ChannelInput/MessageSource/SourceProvenance, removal of the
SessionPipelineOptions.DefaultX fallback escape hatch, elevated-fallback
throws, ToolExecutionContext audience typing, and required trust fields on
persisted job/reminder records.

* refactor(channels): make trust context required on inbound records (#994)

PR-A of the type-system-stiffening effort. The PR #993 bug — a Personal
audience silently downgraded to Public — was a type-shape error: trust
fields were nullable-with-fallback, so the compiler could not tell a
forgetful adapter from a deliberate caller.

- SourceProvenance becomes a 2-param primary constructor; the Unknown
  sentinel defaults and StrictDefault() factory are removed. The two
  genuinely conservative callers (ChannelAclDecision.Deny, TrustContextDeriver
  sourceless path) construct an explicit Unverified/Public value.
- ChannelInput and MessageSource make Audience, Boundary, Principal, and
  Provenance required and non-nullable.
- MessageSourceFactory.Create drops the four ?? options.DefaultX fallback
  arms and copies trust context verbatim.
- SessionPipelineOptions.DefaultAudience/Boundary/Principal/Provenance are
  deleted — the escape hatch the bug exploited. All five channel adapters
  (Slack, Discord, SignalR, Webhook, Reminder) now stamp explicit trust
  context onto every ChannelInput.
- DaemonClient.SendAsync takes a string instead of a fabricated ChannelInput;
  the daemon already derives trust context server-side from the SignalR
  principal, so the client never supplied real trust fields.

Build clean, slopwatch 0 issues, headers verified, all affected test
projects green.

* refactor(security): replace elevated-privilege fallbacks with throws (#994)

PR-B of type-system-stiffening. These sites silently defaulted a missing
value to an *elevated* trust level — a silent privilege escalation, the
inverse of the PR #993 degradation.

- SessionToolExecutionPipeline background-job submission no longer defaults
  a missing turn source to Personal audience / personal boundary; a null
  source is a programming error and throws.
- SubAgentActor no longer defaults a missing spawn audience to Personal; a
  RunSubAgent without an audience throws. A sub-agent always inherits its
  parent session's audience.
- SlackChannel, DiscordChannel, SlackThreadBindingActor, and
  DiscordSessionBindingActor no longer substitute a no-op prompt-injection
  detector for a null one — a null detector silently disables injection
  scanning, so they throw on broken wiring instead.
- NullPromptInjectionDetector is deleted; it existed only to be a silent
  fallback.

Build clean, slopwatch 0 issues, headers verified, all test projects green.

* refactor(tools): type tool-execution audience as parsed TrustAudience (#994)

PR-C of type-system-stiffening. ToolExecutionContext.Audience and
RunSubAgent.Audience were wire-string `string?`s; every tool gate re-parsed
them via ParseAudienceOrPublic, and a malformed value silently degraded to
Public at gate-check time instead of failing at the boundary.

- ToolExecutionContext.Audience and RunSubAgent.Audience become
  TrustAudience? — a parsed enum, not a wire string.
- Netclaw.Tools.Abstractions takes a project reference to
  Netclaw.Configuration for the TrustAudience type.
- Write sites (SessionToolExecutionPipeline, LlmSessionActor, SubAgentActor,
  daemon REST reminder path) assign the typed audience directly; no
  ToWireValue() round-trip.
- SecurityPolicyDefaults.ParseAudienceOrPublic is deleted. ResolveAudienceWithFallback
  and MemoryPolicyScopeResolver.ResolveAudience are retyped to take
  TrustAudience? — they no longer parse wire strings, so an unparseable
  audience can no longer reach a tool gate.
- TryParseAudience is retained for genuine wire/config boundaries (channel
  overrides, CLI commands, reminder-tool args, MCP commands).

Build clean, slopwatch 0 issues, headers verified, all three test projects
green.

* refactor(persistence): require trust fields on persisted job/reminder records (#994)

PR-D of type-system-stiffening. BackgroundJobDefinition, ActiveJobInfo, and
ReminderDefinition carried trust fields with permissive sentinel defaults
(Audience = Personal) — a forgotten field on a persisted record was a silent
privilege escalation that survived a restart.

- All three records make Audience and Boundary `required`, so every in-process
  construction is compiler-enforced. ReminderDefinition's fields also drop
  their nullability.
- Legacy JSON documents that predate this change are handled at load, not by a
  migration: LegacyTrustFieldBackfill detects an absent-or-null Audience/Boundary
  in a job/reminder document, logs a warning naming the file, and injects a
  conservative fail-closed value (Public / public boundary — never the old
  Personal). The reminder store applies this before deserialization so a legacy
  reminder converts on read instead of failing the required-member check and
  being pruned as invalid.
- ActiveJobInfo is protobuf-serialized; proto3 defaults a missing audience to
  enum 0 = Public (fail-closed), so it needs no JSON handling.
- SetReminderTool and ReminderManagerActor supply explicit trust context;
  ReminderExecutionActor's now-unreachable null-audience guards are removed.

Regression tests cover a legacy reminder and a legacy job converting on read
with all non-trust fields intact and the file left in place, plus positive
controls that current documents round-trip verbatim.

Build clean, slopwatch 0 issues, headers verified, all test projects green.

* fix(jobs): fail closed when check_background_job has no context audience (#994)

CheckBackgroundJobTool defaulted a missing execution-context audience and
boundary to the elevated Personal scope — the exact elevated-default
anti-pattern issue #994 targets, and inconsistent with the sibling tool gates
that fail closed to Public. Default to Public / public boundary instead so a
missing audience can never widen a job cancel/query's authority.

* refactor(persistence): reject legacy job/reminder docs instead of backfilling (#994)

Supersedes the conservative-backfill approach from the previous commit. A
persisted job or reminder document that predates #994 carries no trust
context, and these features are typically disabled at the most-restrictive
audience — so a Public backfill fabricates a contradictory state (a feature
gated off) and a Personal backfill silently escalates privilege. Rejecting the
document is the only safe option.

- LegacyTrustFieldBackfill becomes LegacyTrustFieldGuard: it now reports which
  trust keys are absent/null rather than injecting substitutes.
- BackgroundJobDefinitionStore and ReminderDefinitionStore reject a legacy
  document at load — log an error naming the file and the missing fields, and
  exclude it from Get/List so the job/reminder never runs. The reminder store
  preserves the file (operator data, not corrupt JSON) so it can be repaired.
- The required trust fields on the three records are unchanged — that
  compile-time enforcement stands.

Regression tests now assert rejection: a legacy job/reminder is excluded from
Get/List, an error is logged, and the reminder file is left on disk.

Build clean, 0 warnings, slopwatch 0 issues, headers verified, all test
projects green.

* refactor(subagents,reminders): fail-fast sub-agent spawn, drop dead branches (#994)

- SubAgentActor: a RunSubAgent with no audience now replies with an
  unsuccessful SubAgentResult and stops, instead of throwing. The throw
  crashed the child actor and made the caller wait out its Ask timeout; the
  result reply fails the caller fast. Regression test added.
- ReminderManagerActor: drop two dead null-coalesce branches left behind by the
  required-field conversions — EffectiveAudience is always non-null past the
  IsSuccess guard, and ValidateRequestedAudience's requestedAudience parameter
  is now non-nullable (ReminderDefinition.Audience is required).

Build clean, 0 warnings, slopwatch 0 issues, headers verified, tests green.

* refactor(tools): make ToolExecutionContext.Audience required (#994)

ToolExecutionContext.Audience becomes required and non-nullable (was a
settable TrustAudience?). Every construction resolves the audience once, so
tool gates read context.Audience directly with no per-gate ?? Public
fallback. The context-less Empty sentinel carries the most-restrictive Public.

Construction sites resolve a concrete audience: the pipeline and LlmSessionActor
fall closed to Public when a turn has no source; SubAgentActor passes the
guarded parent audience; CreateContext passes its argument.

The five tool gates (SpawnAgentTool, SubAgentSpawner, SkillLoadTool,
SkillReadResourceTool, CheckBackgroundJobTool) drop their `?? TrustAudience.Public`
— now a compile error against the non-nullable property — and read the audience
directly. Behavior is unchanged: the default simply moved from the gates to the
single construction point.

Also fixes an authorization regression this conversion exposed: the REST
POST /api/reminders handler set the tool context's audience from a nullable
authorization context. ResolveReminderAuthorizationContext returns null for a
non-Operator caller; with the audience now required, a `?? Public` default
would have silently admitted the request past ReminderManagerActor's
Operator-authority check. The handler now rejects a null authorization with
403 Forbidden before building the context — restoring the prior denial.

Build clean, 0 warnings, slopwatch 0 issues, headers verified, all 7 test
projects green.

* test(daemon): real integration test for the reminder endpoints (#994)

The reminder-create authorization regression slipped through because
ReminderEndpointAuthorizationTests re-implemented a simplified fake of the
endpoints rather than exercising the real wiring.

- Extract MapReminderEndpoints from a Program.cs local function into a
  public extension method (Netclaw.Daemon/Reminders/ReminderEndpointRouteBuilderExtensions.cs),
  matching MapWebhookEndpoints and MapProviderOAuthEndpoints — pure mechanical
  move, no logic change.
- Replace the fake-reimplementation test with one that calls the real
  app.MapReminderEndpoints(): a minimal ActorSystem hosts a recording test
  actor injected as IRequiredActor<ReminderManagerActorKey>, and a custom
  test auth handler produces an authenticated non-Operator principal.
- Covers the regression (non-Operator POST /api/reminders -> 403, no command
  reaches the actor — verified to fail when the guard is removed), the
  Operator golden path, unauthenticated 401, invalid-audience 400, import as
  Operator/non-Operator, and DELETE ?permanent routing.

Build clean, 0 warnings, slopwatch 0 issues, headers verified, Daemon.Tests
537 green.

* test(daemon): real integration test for the pairing endpoints (#994)

POST /api/pair/exchange is security-critical — anonymous, rate-limited,
issues bearer tokens, enforces a per-IP failure lockout — but had only
fake-reimplementation tests, the same gap that hid the reminder regression.

- Extract the three pairing endpoints (POST /api/pair/exchange,
  GET/DELETE /api/pair/devices) from inline Program.cs handlers into a
  MapPairingEndpoints extension method (Netclaw.Daemon/Security/
  PairingEndpointRouteBuilderExtensions.cs), matching MapWebhookEndpoints /
  MapReminderEndpoints — pure mechanical move, no logic change.
- Replace the fake-reimplementation PairingExchangeEndpointTests and
  PairingCodeEndpointTests with PairingEndpointRouteBuilderExtensionsTests,
  which calls the real app.MapPairingEndpoints() with real PairingCodeService,
  PairingExchangeGuard, and DeviceRegistry.
- Covers: no pending code (404), valid code (200 + token, anonymous, device
  registered), invalid code (401), guard lockout (429 + Retry-After), missing
  fields (400), duplicate device (409), consumed/expired code, the issued
  token authenticating a later request, and the authenticated device-list /
  revoke endpoints.
- Reinstates the two reverse-proxy tests the extraction would otherwise have
  dropped: behind a trusted proxy, both the per-IP lockout and the rate
  limiter must partition by the X-Forwarded-For client IP, not the proxy's.

Build clean, 0 warnings, slopwatch 0 issues, headers verified, Daemon.Tests
537 green.

* test(daemon): real integration tests for MCP and lifecycle endpoints (#994)

Closes the last endpoint coverage gaps — the MCP OAuth/status endpoints and
the lifecycle-shutdown endpoint were inline Program.cs handlers with no real
integration test.

- Extract the six MCP endpoints into MapMcpEndpoints
  (Netclaw.Daemon/Mcp/McpEndpointRouteBuilderExtensions.cs) and the
  lifecycle-shutdown endpoint into MapLifecycleEndpoints
  (Netclaw.Daemon/Lifecycle/), matching the webhook/reminder/pairing pattern.
- The OAuth callback handler now resolves IMcpReconnectable instead of the
  concrete, heavyweight McpClientManager — behavior-identical (the DI
  container already registers IMcpReconnectable as that same singleton) and
  no longer couples the callback to a type that cannot be stubbed in a test.
- McpEndpointRouteBuilderExtensionsTests (16 tests): auth gates on all five
  authorized endpoints; the AllowAnonymous OAuth callback's missing-params
  and unknown-state (500) branches; oauth/start not-found / no-URL / happy
  path; the trivial status GETs; and the callback happy path driven through
  a FakeHttpMessageHandler with a TaskCompletionSource-gated reconnect.
- LifecycleEndpointRouteBuilderExtensionsTests (3 tests): unauthenticated
  401, missing-reason 400, and the authenticated happy path verifying
  DaemonLifecycleNotifier fires.

Build clean, 0 warnings, slopwatch 0 issues, headers verified, Daemon.Tests
556 green.

* fix(reminders,jobs): preserve trust boundaries and surface legacy rejections (#994)
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.

1 participant