fix(addie): align Slack tool selection profiles - #6895
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — app/operational Addie surface only; no blocking findings.
What I checked:
- No
static/schemas/source/**,docs/reference/**, ordist/**touched — no changeset required (operational-only PR, correctly out of scope). - High-risk flag is true only because files match
server/src/addie/**; the reasons are one(added)file (slack-tool-selection.ts) and(modified)app-logic/config/generated-inventory files. No(deleted)entries, so no destructive-change escalation. gated_paths: false, so the hard approval gate does not apply despitereview_decision: REVIEW_REQUIRED.- No no-auto-approve team match.
- The change extracts Slack tool-set selection into a tested module and fixes certification-context semantics (
hasActiveCertificationnow keys on a real in_progress module, DM-only) plus two fail-closed improvements. Tests added.
Medium findings (1):
- server/src/addie/bolt-app.ts:5759 — Reaction fail-closed guard may skip DM emoji-confirmations when
is_privateis absent.
Decision: 1 medium finding, no critical/high, no data-loss/schema/infra category, no (modified)-file escalation combo, no team gate, fewer than 3 mediums. Rows 1–8 do not fire; falls through to row 9 → approve. Worth the developer confirming the DM is_private edge case, but it does not block.
Medium findings
- server/src/addie/bolt-app.ts:5759 — Reaction fail-closed guard may skip DM emoji-confirmations when is_private is absent
|
Addressed Ladon’s DM privacy concern in f8596c4. Slack DM/MPIM metadata is now normalized from |
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
This is a subsequent review responding to the prior pass's single medium finding (bolt-app.ts:5759 — DM privacy resolution possibly skipping fail-closed emoji-confirmations). The delta directly addresses it: adds resolveSlackChannelPrivacy in slack-tool-selection.ts, classifying is_im/is_mpim as private and returning null for unclassified shapes, then wires it into buildChannelContext so viewing_channel_is_private is left unset on null — preserving the downstream fail-closed guard. SlackChannel carries the required fields, and the reaction handler consumes buildChannelContext, so DM emoji-confirmations now resolve as private. Tests cover all five cases including the unclassified null path.
Prior medium finding is resolved; no new findings in this run — a net improvement.
Decision-table check:
- No critical/high findings (row 1 n/a).
- gated_paths is false (row 2 n/a).
- No
(deleted)high-risk reasons (row 3 n/a). - No medium findings at all (rows 4, 5, 8 n/a).
- Prior decision was
approve, so sticky-escalation row 6 does not apply. - No no-auto-approve team match (row 7 n/a).
high_risk is true only via additions/modifications with no medium-or-higher concern on any modified sensitive file, which the rules treat as presumed-safe scaffolding/edits. Falls through to row 9 → approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean subsequent pass.
This run's only delta since the prior clean approve (head f8596c4) is a single generated-file change: the recorded bolt-app.ts source hash in server/src/addie/generated/tool-surface-inventory.generated.json moves from 5be11f… to e065cb…, re-syncing the inventory with actual source. The underlying source (bolt-app.ts, slack-tool-selection.ts, the build script) is unchanged. No source, schema, wire, or changeset-relevant files in the delta.
Checks:
- No critical/high/medium findings reported.
- gated_paths: false; no author no-auto-approve team match.
- high_risk: true, but reasons are (modified)/(added) files with no findings on them — no deletions, no medium concern on a modified sensitive file. Per the high-risk reading rule, the flag alone does not escalate.
- Not a protocol/spec/changeset-surface change (no
static/schemas/source/**, nodocs/reference/**, no dist artifacts) — Addie platform code only.
Decision table: rows 1–8 do not fire (no findings, gate satisfied, no deletions, no team gate, <3 mediums). Falls through to row 9 → approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean delta since the prior approved pass.
This PR touches server/** (Addie platform code), not the AdCP protocol surface (static/schemas/source/**, docs/reference/**, dist/**), so schema↔docs coherence, changeset-scope, oneOf-discriminator, and artifact-immutability gates do not apply.
Delta since the prior approve: a two-line change removing the now-dead ADMIN_CHANNEL_WG_SLUG import from bolt-app.ts (all four references were already eliminated by the earlier selectSlackToolSets refactor) plus the regenerated tool-surface-inventory hash for bolt-app.ts. No behavior change; PR-validation typecheck/build would catch any dangling reference.
Decision path: no critical/high/medium findings (row 1 n/a). gated_paths is false (row 2 n/a). high_risk is true but the reasons are (modified) and (added) — no (deleted) (row 3 n/a) — and there are no medium findings on the modified sensitive files (rows 4–5 n/a). Prior decision was approve, not escalate (row 6 n/a). No no-auto-approve team match (row 7 n/a). Zero medium findings (row 8 n/a). Falls through to row 9 → approve.
Summary
Why
The initial inventory gate exposed conservative ceilings, but several named profiles did not exactly match runtime selection. More importantly, reaction handling could continue with a partial channel context after privacy lookup failed. This makes the runtime policy explicit, testable, and shared with the inventory before admin-domain consolidation begins.
Validation
No protocol changeset is required; this changes Addie application routing and inventory only.
Refs #6842
Refs #6845