mirror of
https://github.com/J3vb/OwnCord.git
synced 2026-09-03 03:50:00 +03:00
fix: batch of 29 correctness fixes across server and client (#1369)
* fix(ws): 2 defect(s) (OC-0013, OC-0140) * fix(voice): 1 defect(s) (OC-0044) * fix(ws): 1 defect(s) (OC-0024) * fix(server): 1 defect(s) (OC-0027) * fix(ws): 1 defect(s) (OC-0028) * fix(server): 7 defect(s) (OC-0033, OC-0066, OC-0067, OC-0068, OC-0074, OC-0077, OC-0106) * fix(ws): 1 defect(s) (OC-0051) * fix(client): 1 defect(s) (OC-0053) * fix(client): 1 defect(s) (OC-0055) * fix(service): 1 defect(s) (OC-0069) * fix(voice): 1 defect(s) (OC-0072) * fix(service): 1 defect(s) (OC-0082) * fix(client): 1 defect(s) (OC-0083) * fix(plugin): 1 defect(s) (OC-0088) * fix(plugin): 4 defect(s) (OC-0104, OC-0126, OC-0127, OC-0133) * fix(admin): 1 defect(s) (OC-0110) * fix(client): 1 defect(s) (OC-0114) * fix(api): 1 defect(s) (OC-0139) * fix(client): 1 defect(s) (OC-0149) * test(server): adapt existing tests to updated OpenDM and IncrementMentionCounts signatures * style(plugin): modernize loops and goroutine spawns in race test * fix(ws): mirror the focus admission gate in the post-subscribe revalidation * fix(service): detach DM post-commit side effects from the request ctx, fail delete closed, add empty-fan-out fallback * fix(plugin): preserve enabled intent when upgrade reactivation hits a runtime-less build * chore(skills): harden bughunt-fix workflow and fold review lessons into bughunt-run/db-change * Add comprehensive documentation for task-observer skill - Introduced environments.md to outline activation setup, compaction behavior, and handoff-doc mode. - Created skill-authoring.md detailing taxonomy, licensing, confidentiality, and editing rules for skill creation. - Added weekly-review.md for a structured review process of OPEN observations, including scheduled and in-session fallback modes. * chore(go): pin toolchain go1.26.6 (stdlib CVE fixes flagged by govulncheck)
This commit is contained in:
J3vb
@@ -147,6 +147,12 @@ When it returns, for each entry in `result.results`:
|
||||
fix run, exactly the retry loop the design deliberately excludes ("one human
|
||||
look beats three agent attempts").
|
||||
|
||||
Ledger writes select records by id or `fix.commit` — never by date fields,
|
||||
which collide when two batches reconcile on the same day — and every bulk
|
||||
mutation asserts its expected match count before writing (a same-day sibling
|
||||
batch once inflated a 29-record update to 44 matches; only the count
|
||||
assertion caught it).
|
||||
|
||||
Check `result.gate`. A failed gate leaves the commits in place on the branch —
|
||||
fix it yourself, do not re-run the workflow over it.
|
||||
|
||||
@@ -165,10 +171,22 @@ It reverts each commit's source files to the parent, runs that stack's tests,
|
||||
and requires them to FAIL — then restores and requires them to PASS. This is the
|
||||
only check in the pipeline no agent can fabricate.
|
||||
|
||||
While it runs it checkouts and restores source files, so the working tree is
|
||||
not a stable read surface: anything reading concurrently — review agents,
|
||||
scanners, hooks — must read committed objects (`git show HEAD:<path>`) or
|
||||
pre-taken snapshots, and any live-tree scanner finding from that window needs
|
||||
re-verification against HEAD before it is believed.
|
||||
|
||||
Any `FAIL ... VACUOUS TEST` means the fix was committed behind a test that
|
||||
proves nothing. Revert that commit and set its findings back to `open`; do not
|
||||
talk yourself into keeping it because the code change looks right.
|
||||
|
||||
One class is exempt from the insta-revert: a race/deadlock-class fix whose
|
||||
test only fails under its detector. verify-fixes escalates to `-race` before
|
||||
declaring vacuity; if an older copy of the script reports VACUOUS on such a
|
||||
fix, re-prove red/green by hand under the class's detector
|
||||
(`go test -race ./<pkg>/`) before reverting anything.
|
||||
|
||||
For every commit `verify-fixes.mjs` reports `PASS`, upgrade that commit's
|
||||
findings' `fix.revertProof` from `"self-reported"` to `"pass"` — this
|
||||
independent run is the only check in the pipeline no agent can fabricate, and
|
||||
@@ -179,7 +197,21 @@ Re-render. Before you review the branch and open the PR, inspect the working
|
||||
tree: a blocked or declined cluster can leave its edits and any new failing
|
||||
test it wrote sitting uncommitted. Discard what you don't want — a reflexive
|
||||
`git add -A` would commit tests that describe unfixed defects into a public
|
||||
repo. Then review the branch and open the PR by hand. The workflow never
|
||||
repo. Audit what got COMMITTED, too: parallel fix agents share the tree, so a
|
||||
prove agent can commit a sibling cluster's content that happened to sit in a
|
||||
shared test file or in regenerated output. Grep the committed tests for
|
||||
finding ids outside the run's fixed set, and re-run the generated-code
|
||||
verifies (sqlc/protocol) after the debris discard — a mismatch means a commit
|
||||
carries foreign regen content. Anything pinning or describing an UNFIXED
|
||||
finding must be excised from history (amend + rebase onto the amended
|
||||
commit), not merely removed by a follow-up commit.
|
||||
|
||||
Then review the branch against the merge-base — `git diff
|
||||
origin/main...HEAD` (three-dot), never two-dot: a concurrent merge plus a
|
||||
background fetch can move origin/main mid-run and turn the two-dot diff into
|
||||
phantom deletions. If origin moved, confirm zero file overlap and a clean
|
||||
`git merge-tree --write-tree origin/main HEAD` before opening the PR by
|
||||
hand. The workflow never
|
||||
pushes and never opens a PR.
|
||||
|
||||
## Testing the workflows themselves
|
||||
|
||||
@@ -36,6 +36,13 @@ and SQLite reports "incomplete input".
|
||||
in comment prose orphans the rest of that comment as a bogus statement
|
||||
("near <word>: syntax error").
|
||||
|
||||
**Regenerate from a tree where the query files carry only YOUR change.**
|
||||
sqlc regenerates every `dbgen/` file from every query file on each run, so
|
||||
unrelated working-tree edits to any `queries/*.sql` — a parallel agent's
|
||||
half-finished work, leftover debris — are silently baked into generated
|
||||
output you then commit. Check `git status` on `Server/db/` before
|
||||
`sqlc generate`, and diff the regen for hunks that are not yours.
|
||||
|
||||
**Do not put `LIMIT 1` on a `:one` query.** It is emitted as a bare `LIMIT`.
|
||||
A `:one` uses `QueryRow` and reads a single row regardless — use `ORDER BY` to
|
||||
choose which one.
|
||||
|
||||
@@ -0,0 +1,446 @@
|
||||
---
|
||||
name: task-observer
|
||||
description: >
|
||||
Monitors task execution for skill improvement opportunities. Use this skill
|
||||
during ANY multi-step task, agentic workflow, or substantive work session where
|
||||
the agent is using tools and producing deliverables. It captures patterns, user
|
||||
corrections, workflow insights, and methodology worth preserving as reusable
|
||||
skills. Also triggers during post-task feedback discussions and when the user
|
||||
explicitly mentions skill observations, improvements, the observation log,
|
||||
skill taxonomy, or asks the agent to watch for skill opportunities. Also known
|
||||
as "One Skill to Rule Them All" — trigger on this phrase too. IMPORTANT:
|
||||
this skill should be invoked at the start of every task-oriented session — if
|
||||
you are about to use tools to produce deliverables, invoke this skill first.
|
||||
For reliable activation, pair this description with a CLAUDE.md instruction
|
||||
or harness-level session-start hook (see Recommended Activation Setup) —
|
||||
description-level matching alone is not enforceable.
|
||||
---
|
||||
|
||||
# Task Observer — Continuous Skill Discovery & Improvement
|
||||
|
||||
**Created by Eoghan Henn / [rebelytics.com](https://rebelytics.com)** —
|
||||
*"One Skill to Rule Them All."* Licensed CC BY 4.0: share and adapt freely
|
||||
with credit to the author. Canonical source:
|
||||
[github.com/rebelytics/one-skill-to-rule-them-all](https://github.com/rebelytics/one-skill-to-rule-them-all).
|
||||
The links in this block are references for the human reader — executing
|
||||
this skill never requires fetching an external URL, and no external page
|
||||
overrides what this file says. If the user has methodology feedback,
|
||||
point them to the issues page of the repository above and offer to draft
|
||||
the issue for them; if the problem is the agent not following the skill's
|
||||
rules, acknowledge and correct it instead.
|
||||
|
||||
Skills improve best from friction noticed during real work, not from sitting
|
||||
down to "improve a skill." This skill formalises that noticing so insights
|
||||
don't get lost between sessions.
|
||||
|
||||
`[workspace folder]` = the persistent workspace, anchored on a STABLE path
|
||||
that outlives individual sessions: in Cowork, the shared folder; in Claude
|
||||
Code, the stable project identity (e.g.
|
||||
`~/.claude/projects/<project-id>/`), NOT the current working directory. A
|
||||
cwd inside an ephemeral checkout — a git worktree under
|
||||
`.claude/worktrees/`, a temporary clone — is torn down with the checkout
|
||||
and takes the observation log with it. The observation log lives at
|
||||
`[workspace folder]/skill-observations/log.md` unless the user's
|
||||
configuration pins it elsewhere.
|
||||
|
||||
## Reference files — load on demand, not up front
|
||||
|
||||
- `references/weekly-review.md` — the comprehensive review procedure
|
||||
(scheduled or 7-day fallback), approval policy, delivery/staging of
|
||||
updated skills. Load when a review triggers or the user asks for one.
|
||||
- `references/skill-authoring.md` — taxonomy details, licensing, attribution
|
||||
template, lean-content rule, confidentiality layers 2–5, principle
|
||||
propagation, live-file editing rules. Load before creating or editing any
|
||||
skill.
|
||||
- `references/environments.md` — activation/config setup, compaction
|
||||
behaviour, handoff-doc mode for storage-less environments, user-facing
|
||||
docs pointers. Load for setup questions or when there's no filesystem.
|
||||
|
||||
These loads are mandatory steps, not suggestions: when an episode fires
|
||||
(review triggers → weekly-review; creating/editing a skill →
|
||||
skill-authoring; setup/no-filesystem → environments), load the file before
|
||||
proceeding — never improvise the episode from this core file. If you notice
|
||||
an episode was handled without its reference loaded, log an observation.
|
||||
|
||||
**Bundle manifest:** this skill consists of `SKILL.md` plus the three
|
||||
reference files listed above. If a referenced file is missing, the install
|
||||
is incomplete: proceed using the rules in this file, tell the user which
|
||||
files are missing, and point them to the full bundle at the canonical
|
||||
source (for the published version, the repository in the attribution
|
||||
above).
|
||||
|
||||
## Session Start Protocol
|
||||
|
||||
1. If `skill-observations/log.md` or `cross-cutting-principles.md` don't
|
||||
exist, create them (templates below / in the principles section of
|
||||
`references/skill-authoring.md`). Also create
|
||||
`skill-observations/last-review-date.txt` containing the literal value
|
||||
`never` if it doesn't exist — never write a date into it at setup; a
|
||||
date means a review actually ran. Before creating or writing anything:
|
||||
if the resolved workspace folder sits under an ephemeral path (e.g.
|
||||
`.claude/worktrees/`, a temporary clone), warn the user and re-anchor
|
||||
on the stable project path first — state written to an ephemeral
|
||||
checkout is lost at teardown.
|
||||
2. Scan OPEN observations and active principles; hold them in awareness,
|
||||
don't surface unprompted.
|
||||
3. Read `skill-observations/last-review-date.txt`. The value carries the
|
||||
truth: a date = when the last review actually ran; `never` = no review
|
||||
has run yet. A missing file is abnormal (step 1 creates it) — recreate
|
||||
it with `never`, don't invent a date. If the value is `never` or older
|
||||
than 7 days AND there are OPEN observations: in an interactive session,
|
||||
offer the review in one line ("the observation backlog hasn't been
|
||||
reviewed [in N days / yet] — run it now, or carry on with your task?")
|
||||
and proceed with the user's task unless they opt in; never gate their
|
||||
work on the review. Only a scheduled/autonomous run loads
|
||||
`references/weekly-review.md` and runs the review unprompted.
|
||||
4. Once per session: if no CLAUDE.md (or equivalent) activation instruction
|
||||
for this skill exists, briefly suggest adding one (see
|
||||
`references/environments.md`). Skip if already configured.
|
||||
5. Note the log's modification time. If modified in the last few hours,
|
||||
another session may be writing to it — re-read immediately before every
|
||||
append, never trust a remembered "current number".
|
||||
|
||||
## When to Observe
|
||||
|
||||
Active for the entire task session: execution, post-task feedback and
|
||||
review discussion, meta-discussion about skills or methodology, and
|
||||
reflective/strategy conversations about how work should be done. **The
|
||||
observation mindset does not deactivate when the conversation shifts from
|
||||
doing the work to discussing it** — user feedback in review phases is often
|
||||
the highest-signal input. Inactive only for casual conversation and quick
|
||||
factual questions with no tools or deliverables involved.
|
||||
|
||||
## What to Watch For
|
||||
|
||||
**Signals for a NEW skill:** a reusable multi-step workflow; a methodology
|
||||
the user explains that no existing skill captures; a recurring task type
|
||||
with similar structure; a process with clear inputs, phases, outputs; the
|
||||
user describing a refined process ("I always do it this way"); a structured
|
||||
approach emerging naturally during work.
|
||||
|
||||
**Signals for IMPROVING an existing skill:** anything from a task that used
|
||||
a skill and could make it better — problems, positive signals, or neutral
|
||||
gaps. Examples: the agent violates a documented rule (the skill needs
|
||||
enforcement, not louder rules); a user correction reveals a missing rule or
|
||||
edge case; a better workflow emerges than the skill recommends; a technique
|
||||
works well enough to promote from incidental to recommended; an undocumented
|
||||
use case; feedback that generalises; a wrong assumption; new tooling
|
||||
obsoletes a step; corrections forming a pattern; a principle that applies to
|
||||
other skills too; a naming/framing/structural suggestion, even
|
||||
conversational.
|
||||
|
||||
**Signals for SIMPLIFYING a skill:** a section never relevant across many
|
||||
sessions; a rule from a single unvalidated observation; workflows users
|
||||
consistently shortcut; sections loaded but never acted on; contradictory
|
||||
rules; "just in case" complexity that never triggered; a rule the agent
|
||||
consistently fails to follow (convert to structural enforcement — checklist,
|
||||
verification step, unskippable tool call — or remove it). Treat these as a
|
||||
review checklist; ask "what can we remove?" as deliberately as "what should
|
||||
we add?"
|
||||
|
||||
**Do NOT log:** one-off corrections that don't generalise; preferences
|
||||
already captured in a skill; tool bugs unrelated to methodology;
|
||||
observations that would need proprietary client information to be useful in
|
||||
an open-source skill (unless an internal skill is the right home).
|
||||
|
||||
## How to Log
|
||||
|
||||
Append to the log **silently, within the same turn or the next** — never
|
||||
batch mentally for later; the act of writing is the enforcement mechanism.
|
||||
|
||||
**Mandatory observation checkpoint after every 3rd TodoWrite completion:** After
|
||||
marking the 3rd, 6th, 9th (etc.) TodoWrite item as completed in a session, you
|
||||
must **write to the log** — not merely pause to ask yourself a question. Either
|
||||
append any pending observations, or, if genuinely none have accumulated, append
|
||||
an explicit acknowledgement marker (a one-line `no observations` note for that
|
||||
checkpoint). The required action is a concrete log write; a remembered "ask
|
||||
whether" is not enforcement. This is a hard checkpoint, not a suggestion — the
|
||||
skill has demonstrated that softer "check when completing items" or "pause and
|
||||
ask" guidance gets lost during cognitively demanding analytical work, exactly
|
||||
when the most observations accumulate. The count doesn't need to be precise;
|
||||
the rule is: roughly every third completion, write to the log (observations or
|
||||
the acknowledgement marker). The write itself is the enforcement mechanism: it
|
||||
forces the mental check to surface as a recorded action, and it prevents the
|
||||
common failure mode where the skill is loaded but no observations are written
|
||||
until the user explicitly asks.
|
||||
|
||||
**Deliverable-event flush:** Hard enforcement that hooks onto tool calls you are
|
||||
already making is the only reliable mechanism; soft prompts that rely on memory
|
||||
don't survive cognitive load during long substantive sessions (when the most
|
||||
insights surface). So tie observation-flushing to deliverable and workflow events
|
||||
that already involve a tool call. Whenever you present or render a major
|
||||
deliverable — `present_files`, a deck or PDF render, a staged skill file handed
|
||||
to the user — or complete a task/todo batch, flush any pending observations to
|
||||
the log at that moment, before moving on. These are natural, already-occurring
|
||||
checkpoints; piggy-backing the flush onto them means the write happens as a
|
||||
side effect of work you were doing anyway, rather than depending on a separate
|
||||
act of memory.
|
||||
|
||||
**Numbering discipline (mandatory, every append):**
|
||||
|
||||
1. *Pre-check:* read the actual log and find the highest existing number —
|
||||
never trust session memory:
|
||||
|
||||
```bash
|
||||
# GNU grep:
|
||||
grep -oP '### Observation \K\d+' log.md | sort -n | tail -1
|
||||
# macOS / POSIX:
|
||||
grep -o '### Observation [0-9]*' log.md | grep -o '[0-9]*' | sort -n | tail -1
|
||||
```
|
||||
|
||||
2. *Pre-write assertion:* immediately before appending, confirm the proposed
|
||||
number doesn't already exist:
|
||||
|
||||
```bash
|
||||
PROPOSED=$(( $(grep -oP '### Observation \K\d+' log.md | sort -n | tail -1) + 1 ))
|
||||
grep -qE "^### Observation ${PROPOSED}:" log.md && {
|
||||
echo "COLLISION on #${PROPOSED}"; exit 1; }
|
||||
```
|
||||
|
||||
If it fires, increment past all existing numbers and re-check (and log a
|
||||
meta-observation — it signals a parallel-session collision).
|
||||
|
||||
3. *Post-write verification:* after appending, count occurrences of the
|
||||
number; if >1, a parallel writer collided between check and write —
|
||||
renumber YOUR entry to max+1. Identify your entry from your own append
|
||||
operation (capture the file's line count immediately before and after
|
||||
your `>>`; your entry starts at the old line count + 1) — do NOT
|
||||
re-grep and take the last occurrence, which may be a colliding writer's
|
||||
entry appended after yours. After any `sed` renumber, re-read the
|
||||
affected line to confirm the substitution actually took effect — a
|
||||
line-addressed `s///` whose target shifted finds no match and still
|
||||
exits 0. Pre-write catches stale reads; only a post-write check catches
|
||||
the race. The pattern for shared logs written by parallel agents is
|
||||
check-then-act-then-verify.
|
||||
|
||||
**Log-write safety — never let a mutation span entry boundaries:** When
|
||||
mutating the log programmatically (marking entries ACTIONED/DECLINED,
|
||||
archiving, renumbering), a greedy or DOTALL pattern over the whole file can
|
||||
silently swallow everything from one match to EOF. This has happened: a
|
||||
`.*$` under `re.S` over the multi-entry file captured from one entry's
|
||||
Status line to end-of-file and overwrote 16 later entries in a single
|
||||
substitution. The log is shared state across many entries; mutate it one
|
||||
bounded entry at a time and verify every mutation.
|
||||
|
||||
1. **Re-read and merge immediately before any write-back.** Any full-file
|
||||
rewrite (archival, renumbering, reassembly from chunks) built from a
|
||||
snapshot destroys whatever concurrent sessions appended after that
|
||||
snapshot — the write-back succeeds, the victim gets no error, and the
|
||||
loss is invisible. This has happened in production: a parallel session's
|
||||
write-back erased two entries appended minutes earlier, hours after the
|
||||
exact failure mode had been documented. So: take the snapshot, prepare
|
||||
the mutation, then — immediately before writing — re-read the live log
|
||||
and diff against the snapshot. If new entries appeared, merge them into
|
||||
the write-back (or rebuild from the fresh read). Never write back a
|
||||
stale snapshot.
|
||||
|
||||
2. **Isolate the target entry, or anchor to a single line.** Either split
|
||||
the log on `### Observation N:` headers, edit the TARGET entry's chunk in
|
||||
isolation, and reassemble — OR, for a status-only edit, use a strictly
|
||||
line-anchored multiline substitution that cannot cross a newline, e.g.
|
||||
`re.sub(r'(?m)^(\s*-?\s*)\*\*Status:\*\*.*$', ...)` (multiline `^...$`
|
||||
bounds the match to one line). NEVER use a DOTALL/greedy pattern across
|
||||
the multi-entry file.
|
||||
|
||||
3. **Assert a structural invariant against the LIVE pre-write file.** Count
|
||||
`### Observation` headers in the live file immediately before writing and
|
||||
again after. For a status-only edit the count MUST be unchanged; for
|
||||
archival or append it must change by exactly the expected number. The
|
||||
baseline must be the live file at write time, NOT your session's earlier
|
||||
snapshot — an invariant computed against a stale snapshot validates that
|
||||
you wrote what you intended while still destroying what others wrote in
|
||||
between. Fail loudly if the count is off.
|
||||
|
||||
4. **Keep the pre-write backup.** Copy `log.md` before any programmatic
|
||||
mutation. This is what made full recovery trivial when the truncation
|
||||
above occurred — it turned a destructive bug into a non-event.
|
||||
|
||||
5. **Verify your entries SURVIVED, not just that they were written.** A
|
||||
successful append proves nothing an hour later — a concurrent session's
|
||||
write-back can silently delete it, and only the destroying session gets
|
||||
any signal (none). Before surfacing observations at session end, grep
|
||||
the log for every entry number this session wrote and confirm each still
|
||||
exists exactly once; re-append any that are missing (with fresh numbers)
|
||||
and log a meta-observation about the collision.
|
||||
|
||||
Principle: a log shared across many entries must be mutated one bounded
|
||||
entry at a time; every rewrite must be based on a fresh read, verified by a
|
||||
structural invariant against the live pre-write file, and backed up. Writers
|
||||
must verify survival, not just successful writes — in a concurrent erase,
|
||||
the victim gets no error.
|
||||
|
||||
**Format and insertion:** always `### Observation NNN:`, always appended to
|
||||
the END of the log, never mid-file, never alternative ID formats. One
|
||||
format, one insertion point. **Every new observation MUST include
|
||||
`**Status:** OPEN` as its first field — this is mandatory at write time, not
|
||||
optional.** Reviews classify entries by their Status line; an observation
|
||||
written without one is invisible to any status-filtered pass and risks being
|
||||
silently skipped instead of triaged.
|
||||
|
||||
```markdown
|
||||
### Observation [N]: [Short descriptive title]
|
||||
|
||||
**Status:** OPEN
|
||||
**Date:** [date]
|
||||
**Session context:** [what task was being worked on]
|
||||
**Skill:** [existing skill name, or "New skill candidate: [working name]"]
|
||||
**Type:** [open-source | internal]
|
||||
**Phase/Area:** [which part of the skill or workflow]
|
||||
|
||||
**Issue:** [What happened — specific enough to understand weeks later
|
||||
without the original conversation.]
|
||||
|
||||
**Suggested improvement:** [Concrete change. For existing skills, name the
|
||||
section or rule; for new skills, scope and key components.]
|
||||
|
||||
**Principle:** [The generalisable takeaway — the most important field.]
|
||||
```
|
||||
|
||||
**Context preservation:** if an observation depends on session-local data
|
||||
(uploads, API output), save that context into the workspace first and add a
|
||||
`**Reference file:**` line — an observation whose evidence dies with the
|
||||
session is incomplete.
|
||||
|
||||
**Confidentiality at logging time:** for `type: open-source` observations,
|
||||
the Issue/Improvement fields may reference specifics for context, but the
|
||||
Principle must be fully generalised — no client names, domains, or details
|
||||
traceable to a real project. Full confidentiality layers for skill
|
||||
authoring: `references/skill-authoring.md`.
|
||||
|
||||
## Referencing Observations
|
||||
|
||||
When citing an observation by number — in conversation, in a review report,
|
||||
or from within another observation — the number must come from the entry's
|
||||
literal `### Observation N:` header line. Never cite an observation number
|
||||
that wasn't read from that header.
|
||||
|
||||
- **Search-tool line numbers are positional metadata, not IDs.** `grep -n`
|
||||
prefixes every match with a line number; when a match lands mid-entry
|
||||
(e.g., on a Session context or Principle line rather than the header),
|
||||
that line number is NOT the observation number. Resolve to the owning
|
||||
header first — scan backwards from the matched line to the nearest
|
||||
preceding `### Observation N:` header and take the number from there
|
||||
(e.g., an awk backwards-scan, or re-grep for `^### Observation` and pick
|
||||
the last header line before the match).
|
||||
- **Plausibility check (cheap second layer):** before quoting any
|
||||
observation number, compare it against the known counter range — the
|
||||
highest `### Observation N:` header in the log. A number outside that
|
||||
range (e.g., citing #1365 when the log's counter is at #766) is almost
|
||||
certainly a line number or other positional artefact misread as an ID.
|
||||
|
||||
The general rule: IDs must come from the record's own identifier field,
|
||||
never from the positional metadata of the search tool that found it.
|
||||
|
||||
## Taxonomy (quick version)
|
||||
|
||||
**Open-source** — client-agnostic, methodology-driven, useful to other
|
||||
practitioners. **Internal** — contains user/client/project specifics or
|
||||
personal preferences. Default to open-source when it could go either way,
|
||||
stripping specifics. The boundary is also a confidentiality boundary. Full
|
||||
requirements (attribution, licensing, structure): `references/skill-authoring.md`.
|
||||
|
||||
## Archival on Write
|
||||
|
||||
On every log write, first move already-resolved entries to
|
||||
`skill-observations/archive/log-[YYYY-MM-DD].md` (preserving the log header
|
||||
in the archive). "Already resolved" is decided by date, read from the file:
|
||||
a resolved status MUST record its date — `ACTIONED (YYYY-MM-DD) — [what was
|
||||
done]` / `DECLINED (YYYY-MM-DD) — [reason]` — and archival moves only
|
||||
entries whose recorded date is before today. Entries resolved today stay in
|
||||
the active log until the next day, no matter which session resolved them:
|
||||
the grace period lives in the file, never in session memory, so it holds
|
||||
across parallel and subsequent sessions. A resolved entry with no readable
|
||||
date gets today's date added instead of being archived. The active log
|
||||
keeps its header, status key, all OPEN entries, and the same-day-resolved
|
||||
ones.
|
||||
|
||||
Archival is a read-filter-rewrite — the highest-risk mutation the log
|
||||
undergoes, and the one that has destroyed concurrent appends in production.
|
||||
It MUST follow the full Log-write safety sequence above: backup, re-read
|
||||
the live log immediately before writing back and merge any entries that
|
||||
appeared since the snapshot, then verify the post-write header count equals
|
||||
the live pre-write count minus exactly the number of archived entries.
|
||||
|
||||
## Log Structure
|
||||
|
||||
```markdown
|
||||
# Skill Observation Log
|
||||
|
||||
Observations captured during task-oriented work.
|
||||
|
||||
**Status key:** OPEN = not yet actioned | ACTIONED (YYYY-MM-DD) = skill
|
||||
updated/created | DECLINED (YYYY-MM-DD) = user decided not to pursue —
|
||||
resolved statuses always carry their resolution date
|
||||
|
||||
---
|
||||
|
||||
## [Date]
|
||||
|
||||
### Observation 1: [Title]
|
||||
**Status:** OPEN
|
||||
[... full format ...]
|
||||
```
|
||||
|
||||
## Surfacing Protocol
|
||||
|
||||
Default: at end of session, as a grouped summary — improvements grouped by
|
||||
skill, new-skill candidates listed separately; for each, one sentence plus
|
||||
suggested type; ask which to act on. Surface earlier when an observation
|
||||
needs user input to be complete, when a skill is actively producing wrong
|
||||
output, or when observations cluster on one skill.
|
||||
|
||||
**Default to log-and-defer.** Surfacing an observation is not an invitation
|
||||
to act on it. The default is log-and-defer: state that the observation is
|
||||
logged for the next review, and stop. Reserve in-session application
|
||||
strictly for the two triggers already defined under "Acting on
|
||||
Observations" — an explicit user request that names the action, or
|
||||
correcting a skill that is producing wrong output in the current session.
|
||||
|
||||
Do NOT routinely offer a binary "apply now vs leave for next review" choice
|
||||
when surfacing observations. For users who run regular reviews, that offer is
|
||||
unwanted friction repeated every session. If a user has expressed a standing
|
||||
preference to always defer to the next review, suppress the in-session
|
||||
"act now?" offer entirely rather than asking each time.
|
||||
|
||||
**Self-check before surfacing:** observations were logged throughout the
|
||||
whole session (including discussion phases); logged silently; each follows
|
||||
Issue → Improvement → Principle; each is typed; existing-skill items name
|
||||
the section; no open-source Principle contains client-identifying info;
|
||||
every appended observation carries a Status line (`**Status:** OPEN` at
|
||||
write time) — a statusless entry is invisible to any status-filtered review
|
||||
pass, so if any observation lacks one, add it now. Finally, run the
|
||||
survival check (Log-write safety rule 5): grep the log for every entry
|
||||
number this session wrote and confirm each still exists exactly once — a
|
||||
concurrent session's write-back deletes silently. Fix failures before
|
||||
surfacing.
|
||||
|
||||
## Acting on Observations
|
||||
|
||||
Act only in three contexts: (1) the comprehensive review (load
|
||||
`references/weekly-review.md`); (2) an explicit user request ("update X
|
||||
skill", "act on observation #N"); (3) in-session correction when a skill is
|
||||
producing wrong output the user should know about. Otherwise: log, don't
|
||||
act.
|
||||
|
||||
When acting: small, clearly-additive, low-risk changes (a new rule, a
|
||||
clarification, a factual fix) may be applied directly. Substantial changes
|
||||
(restructuring, new capabilities, changed methodology) and all new-skill
|
||||
creation: load `references/skill-authoring.md` first and follow its editing
|
||||
and staging rules. If an observation reveals a principle that applies to
|
||||
skills generally, propose it for the cross-cutting principles file (see the
|
||||
same reference).
|
||||
|
||||
## Quick Reference
|
||||
|
||||
| Question | Answer |
|
||||
|----------|--------|
|
||||
| When do I observe? | The whole session, including feedback and reflection phases |
|
||||
| How do I log? | Silently, immediately, appended to the end, with the 3-step numbering discipline |
|
||||
| When do I surface? | End of session, or earlier if needed |
|
||||
| Status line? | Mandatory `**Status:** OPEN` as the first field of every new observation; reviews treat statusless entries as OPEN, never as nonexistent |
|
||||
| Citing an observation number? | Only from its literal `### Observation N:` header — `grep -n` line numbers are positional metadata, not IDs; sanity-check against the known counter range |
|
||||
| Open-source or internal? | Default open-source; the boundary is confidential |
|
||||
| Small fix or substantial? | Additive → apply directly; restructuring/new skill → `references/skill-authoring.md` |
|
||||
| Rewriting the log (archival/renumber/status)? | Backup → re-read live and merge → bounded mutation → verify count against live pre-write file → confirm own entries survived |
|
||||
| Weekly review? | Trigger check at session start; procedure in `references/weekly-review.md` |
|
||||
| No filesystem? | Handoff-doc mode — `references/environments.md` |
|
||||
@@ -0,0 +1,113 @@
|
||||
# Environments, Activation Setup, and Handoff-Doc Mode
|
||||
|
||||
Load this for setup questions, compaction/resume behaviour, or when running
|
||||
in an environment without filesystem access.
|
||||
|
||||
## Recommended activation setup
|
||||
|
||||
Description-level matching alone can miss invocation when the agent is
|
||||
focused on the task, so pair the skill with a configuration-level
|
||||
instruction (CLAUDE.md, project instructions, or equivalent):
|
||||
|
||||
```
|
||||
At the start of any task-oriented session — any interaction where you will
|
||||
use tools and produce deliverables — invoke the task-observer skill before
|
||||
beginning work. This ensures skill improvement opportunities are captured
|
||||
throughout the session.
|
||||
|
||||
When loading any skill, check the observation log for OPEN observations
|
||||
tagged to that skill. Apply their insights to the current work, even if
|
||||
the skill file hasn't been updated yet. This enables immediate application
|
||||
of observations before they're permanently integrated during the weekly
|
||||
review.
|
||||
```
|
||||
|
||||
**Config detection (once per session):** with filesystem access, check the
|
||||
workspace root's CLAUDE.md (or equivalent) for a task-observer activation
|
||||
instruction — suggest adding it if absent, creating the file if none
|
||||
exists. Without filesystem access, check the system prompt / project
|
||||
instructions and suggest the user add the instruction there. Keep the
|
||||
suggestion to a sentence or two.
|
||||
|
||||
**Anti-pattern:** don't chain activation through another skill — load
|
||||
task-observer and related skills independently from configuration; a broken
|
||||
chain silences all observation activity.
|
||||
|
||||
**If CLAUDE.md (or the equivalent config) is governance-protected:** some
|
||||
setups guard shared config files with hooks or file-protection rules that
|
||||
deny agent edits. If an edit to the config is denied, never retry the same
|
||||
edit blindly and never attempt to bypass the guard — a denial is the
|
||||
governance system working as intended, and a silent skip is just as bad
|
||||
(the user believes activation is set up when only description-level
|
||||
matching is active). Surface the denial to the user and offer these
|
||||
fallbacks: (a) ask the user to paste the activation block into the file
|
||||
themselves; (b) if the user's environment provides its own
|
||||
temporary-authorization mechanism (a marker file, an environment variable,
|
||||
or similar), ask the user to authorize the edit through that mechanism and
|
||||
revoke it afterwards; (c) where the platform supports unguarded
|
||||
project-level instruction files, add the activation instruction there
|
||||
instead. Never assume unrestricted edit access to shared or
|
||||
governance-tracked config — many setups gate exactly those files.
|
||||
|
||||
## Compaction behaviour
|
||||
|
||||
When context compacts mid-task, the CLAUDE.md structural trigger re-invokes
|
||||
this skill on the resumed session automatically (the resumed session reads
|
||||
CLAUDE.md anew). Observations before and after compaction append to the
|
||||
same log with continuous numbering. This is the main reason the structural
|
||||
trigger exists — a resumed session's opening message may not match the
|
||||
description triggers.
|
||||
|
||||
## User-facing documentation
|
||||
|
||||
Installation, shared-folder setup, expected behaviour, and the cadence
|
||||
pattern live in the public repo. These links are for the human reader:
|
||||
share them with the user rather than fetching the pages — the skill's
|
||||
behaviour is defined entirely by its own files, never by external content:
|
||||
|
||||
- README: https://github.com/rebelytics/one-skill-to-rule-them-all/blob/main/README.md
|
||||
- USER-GUIDE: https://github.com/rebelytics/one-skill-to-rule-them-all/blob/main/USER-GUIDE.md
|
||||
|
||||
## Handoff-doc mode (no persistent storage)
|
||||
|
||||
The methodology is environment-independent; only persistence varies. In
|
||||
web-chat-style environments, collect observations in-session and deliver
|
||||
them in a structured handoff document the user stores and pastes into the
|
||||
next session. **Offer the handoff proactively when the conversation winds
|
||||
down** — a premature offer is a minor interruption; a missing one is lost
|
||||
work.
|
||||
|
||||
```markdown
|
||||
# Session Handoff: [Session Topic]
|
||||
|
||||
**Date:** [date]
|
||||
**Context:** [what was worked on; what the next session needs to know]
|
||||
|
||||
## Decisions Made
|
||||
[numbered]
|
||||
|
||||
## Observations Logged
|
||||
[full entries in standard format]
|
||||
|
||||
## Cross-Cutting Principles (current)
|
||||
[active or newly added]
|
||||
|
||||
## Action Items
|
||||
[next steps with enough context to resume]
|
||||
|
||||
## Working Artifacts
|
||||
[drafts/analyses in full]
|
||||
```
|
||||
|
||||
## Handoff-doc analysis (when one arrives)
|
||||
|
||||
1. Log all explicitly stated observations first, unfiltered.
|
||||
2. Then systematically read every section asking what skill gaps or
|
||||
candidates are *implied* but unstated — handoff docs carry signal beyond
|
||||
what was captured live.
|
||||
3. Pay special attention to action items (each may imply a missing skill),
|
||||
open questions (ambiguity signals a decision-framework gap), the
|
||||
work-completed narrative (patterns may reveal meta-skills), and session
|
||||
notes.
|
||||
4. Attribute derived observations as coming from handoff-doc analysis, not
|
||||
the original session.
|
||||
@@ -0,0 +1,235 @@
|
||||
# Skill Authoring — taxonomy, licensing, confidentiality, editing rules
|
||||
|
||||
Load this before creating any skill or making substantial changes to one.
|
||||
|
||||
## Taxonomy in full
|
||||
|
||||
**Open-source skills** are client-agnostic and methodology-driven.
|
||||
Recognise one: the methodology works across clients and contexts; no
|
||||
proprietary information is needed; other practitioners would find it
|
||||
valuable; it captures a process, not personal preferences. Required
|
||||
elements: the body identifies itself as open-source; author attribution
|
||||
block (template below); a licence statement; a feedback/support section
|
||||
routing methodology feedback to the creator; tool-agnostic language
|
||||
(capabilities like "browser access", not product names); built-in
|
||||
enforcement (see Pre-Flight Principle). Default to open-source when a skill
|
||||
could go either way — strip specifics and generalise.
|
||||
|
||||
**Internal skills** contain user/client/project specifics, personal
|
||||
preferences, or context only the user has. They identify themselves as
|
||||
internal, need no attribution or licence, and can be shorter and less
|
||||
formal. They're working documents — keep them current, don't over-engineer.
|
||||
|
||||
## The Pre-Flight Principle
|
||||
|
||||
Rules documented in a skill are not reliably followed during creative flow.
|
||||
Every skill with explicit rules needs a verification step where the agent
|
||||
re-reads the rules and checks its output against them before delivery. When
|
||||
creating or improving any skill ask: "Does it have rules? Does it have a
|
||||
mechanism to enforce them?" If not, add one.
|
||||
|
||||
**Embedded commands are pre-flight items too — execute before you ship.**
|
||||
Prose rules and command snippets fail differently: a prose rule is
|
||||
re-interpreted in context on every run, so ambiguity can be caught at
|
||||
execution time; an embedded command runs verbatim, unattended, forever —
|
||||
and a subtly wrong command can read as correct on every re-read
|
||||
(`git log -1 --format=%cI --reverse` returns the NEWEST commit, because
|
||||
`-1` applies before `--reverse`, while the plausible reading is "oldest").
|
||||
Any command embedded in a skill must be executed once against real data,
|
||||
with its output inspected for plausibility, before the skill file is
|
||||
saved. An unverified snippet is among the highest-risk lines in a skill:
|
||||
it ships bugs that no re-read can catch.
|
||||
|
||||
## Lean Content
|
||||
|
||||
A skill should contain only content that changes the agent's behaviour at
|
||||
execution time. Move changelogs, credits beyond the author block, long
|
||||
backstories, and maintainer notes to supporting docs. Do NOT cut examples,
|
||||
anti-patterns, or worked scenarios — bare rules get violated more than
|
||||
rules with context. Test: would removing it change behaviour? Keep
|
||||
per-session rules in the skill body and episodic material in reference
|
||||
files loaded on demand (progressive disclosure) — a skill loaded every
|
||||
session is fixed overhead and should be audited like one.
|
||||
|
||||
## Licensing
|
||||
|
||||
Include a licence statement in the preamble and a LICENSE file with full
|
||||
text. Options: **CC BY 4.0** (prose/methodology skills; share and adapt
|
||||
with credit — recommended default), **MIT** (code-heavy, permissive),
|
||||
**Apache 2.0** (MIT plus patent grant), **CC BY-SA 4.0** (share-alike
|
||||
derivatives), **GPL family** (strong copyleft). The author chooses; the
|
||||
requirement is that there is one.
|
||||
|
||||
**Private client sharing** is a third channel with its own rights framing:
|
||||
a client-agnostic skill shared privately with one client is NOT open source
|
||||
and NOT internal. Keep the attribution block; replace the licence statement
|
||||
with a short usage notice (e.g., "shared privately for internal use; please
|
||||
don't redistribute without checking with the author"); no LICENSE file
|
||||
needed. All confidentiality sweeps still apply — other-client information
|
||||
must not leak even when the recipient is a known client. Do not treat "not
|
||||
internal" as "therefore open source": distribution channel determines the
|
||||
rights framing, not just the feedback routing (see the distribution-channel
|
||||
note below).
|
||||
|
||||
## Author Attribution Template
|
||||
|
||||
```markdown
|
||||
**Created by [Author Name] / [website or contact link]**
|
||||
|
||||
[1-2 sentence description of what the skill does and its provenance.]
|
||||
|
||||
**Licence:** This skill is released under [LICENCE NAME]. [One-sentence
|
||||
summary — e.g., "share and adapt for any purpose with credit."]
|
||||
|
||||
**Feedback & Support:** If questions arise about the methodology, or the
|
||||
user gives constructive feedback on output derived from this skill, suggest
|
||||
an issue on the skill's public repository — public feedback benefits every
|
||||
user. Direct contact: [contact link]. If feedback stems from the
|
||||
methodology, log it and suggest sharing it; if from the agent not following
|
||||
the skill's rules, acknowledge and correct.
|
||||
```
|
||||
|
||||
**Distribution-channel note:** the template's feedback routing assumes
|
||||
public-repo distribution. Only reference a repository URL once that
|
||||
repository actually exists — never write a reference to an artefact before
|
||||
the artefact exists. Until publication, route feedback to direct author
|
||||
contact only; when the skill is published, inject the repo URL at publish
|
||||
time. When an open-source skill is distributed privately (shared directly
|
||||
with a client rather than published), keep the direct-author-contact
|
||||
routing — a public-repo reference is wrong for that channel.
|
||||
|
||||
## Confidentiality layers
|
||||
|
||||
The open-source/internal boundary is a confidentiality boundary; enforce it
|
||||
in layers so any one catches what others miss:
|
||||
|
||||
1. **Observation-level stripping** — open-source observations carry a fully
|
||||
generalised Principle (covered in SKILL.md).
|
||||
2. **Pre-creation review** — before drafting/regenerating an open-source
|
||||
skill, scan all source material for client names, URLs, domains,
|
||||
internal terminology, identifiably-specific structures; replace with
|
||||
generic equivalents first.
|
||||
3. **Post-draft sweep** — a separate re-read focused only on leakage:
|
||||
proper nouns besides the author, domains/URLs/project identifiers,
|
||||
vertical details that narrow the client, examples traceable to a real
|
||||
project.
|
||||
4. **Structural principle** — when in doubt, remove. Slightly more generic
|
||||
beats slightly leaky.
|
||||
5. **Cross-product re-identifiability sweep** — the final pass before any
|
||||
public release. Individually-sanitised examples can combine to identify
|
||||
a client (enumerated counts matching a public client list; specific
|
||||
numbers in a thin vertical; thinly-disguised placeholder names in the
|
||||
same vertical as a real client). List every example and its fields
|
||||
(vertical, geography, numbers, timing, counts); ask whether a reader
|
||||
with the author's public client list could map them; mitigate by
|
||||
blurring counts, widening verticals, using illustrative ranges, or
|
||||
consolidating into composites. Run this mechanically — the author is the
|
||||
least reliable judge because they know the ground truth.
|
||||
|
||||
## Editing skills — always start from the live file
|
||||
|
||||
1. The live file is the authoritative source: in Claude Code,
|
||||
`~/.claude/skills/{skill}/SKILL.md`; in Cowork, a read-only mount at
|
||||
`.claude/skills/{skill}/SKILL.md` (writes fail with EROFS by design).
|
||||
Do not edit skill files in place, in any environment — staging-only is
|
||||
what keeps the autonomous review safe.
|
||||
2. Always base edits on a fresh read of the live file — never a workspace
|
||||
copy, prior draft, or memory.
|
||||
3. Before overwriting any staged/workspace copy, diff it against the live
|
||||
file; if they differ, rebase your edits on the live version. (Observed
|
||||
failure: an update built on a stale snapshot silently dropped two
|
||||
sections added to the live skill the same day; only a pre-merge diff
|
||||
caught it.)
|
||||
4. Stage every update to
|
||||
`[workspace folder]/skill-updates/[date]/[skill-name]/` — the FULL
|
||||
skill directory (SKILL.md plus references/, scripts/, assets/ where
|
||||
present), never SKILL.md alone — and present it for review and
|
||||
installation; nothing goes live until the user installs it. Where no
|
||||
presentation/upload tool exists (e.g. Claude Code CLI), present the
|
||||
staged path and a change summary in chat instead; staging-only applies
|
||||
in every environment — it's the review loop's safety property, not a
|
||||
filesystem constraint. For any
|
||||
skill with supporting files, zip the staged directory into a `.skill`
|
||||
bundle and present the bundle, never the bare SKILL.md: a single-file
|
||||
delivery convention applied to a multi-file skill truncates it
|
||||
silently (the install succeeds, the skill loads, and the missing
|
||||
pieces only surface when a reference load or script call fails
|
||||
mid-task). **Pre-delivery gate — two items, checked at the moment of
|
||||
delivery, not just at drafting time:** (1) every `references/`,
|
||||
`scripts/`, `assets/` path in the staged SKILL.md body has its file in
|
||||
the staged set; (2) if the skill is multi-file, the delivery artefact
|
||||
is the `.skill` bundle — bare file links fail this gate even when all
|
||||
files are staged. (Reading this rule while drafting does not enforce
|
||||
it at delivery; run the gate as the last step before presenting.)
|
||||
Packaging hygiene: before zipping, sweep the staged tree for build
|
||||
artefacts (`__pycache__/`, `*.pyc`, `.DS_Store`, `.~lock.*`) left by
|
||||
in-session checks, and read the archive listing back after zipping —
|
||||
the listing is the cheap verification that catches leaked artefacts.
|
||||
5. When seeding a staged copy by copying from the read-only mount, reset
|
||||
write permissions immediately (`chmod -R u+w` on the staged path, or
|
||||
`cp --no-preserve=mode`) — the mount's read-only mode travels with
|
||||
the copy, for directories as well as files, and the follow-up edit
|
||||
otherwise fails with a permission error.
|
||||
6. Match process rigour to the change: complex/open-source/uncertain design
|
||||
→ use the skill-creator if available; internal skills with requirements
|
||||
already established in conversation → write directly, flagging
|
||||
substantial changes for review.
|
||||
|
||||
## Verifying relocations and restructures
|
||||
|
||||
When content is relocated verbatim (splits into core + references, merges,
|
||||
restructures), "nothing was lost" is checkable mechanically — but only with
|
||||
a two-tier check:
|
||||
|
||||
1. Enumerate every added/moved line via `diff` of the old base vs the new
|
||||
base.
|
||||
2. Exact-match each non-empty line against the restructured file set
|
||||
(`grep -F`).
|
||||
3. For misses, substance-check via a distinctive mid-line substring before
|
||||
concluding loss — most misses are container artifacts (heading-level
|
||||
changes, list-to-prose adaptation, re-wrapped lines splitting a phrase
|
||||
across newlines), not real losses.
|
||||
4. Word-count sanity check per file.
|
||||
|
||||
One tier alone either misses losses (substance-only) or cries wolf
|
||||
(exact-only). Additionally, inventory the original's enforcement
|
||||
mechanisms (checkpoints, assertions, invariants, mandatory-write rules,
|
||||
defaults) as an explicit checklist — compression preferentially destroys
|
||||
enforcement machinery because it reads as redundancy — and sweep any "pure
|
||||
restructuring" change for net-new behaviour, which hides well in a large
|
||||
rewording diff.
|
||||
|
||||
## New skills
|
||||
|
||||
Use the skill-creator when available, passing the observation(s) as the
|
||||
brief. Determine type early: open-source → strip and generalise; internal →
|
||||
include specifics freely; uncertain → default open-source and let the user
|
||||
add internal detail afterwards.
|
||||
|
||||
## Principle Propagation
|
||||
|
||||
When an observation's Principle applies to skills in general, log it with
|
||||
`Skill: All skills` and surface it; if the user approves, add it to
|
||||
`[workspace folder]/skill-observations/cross-cutting-principles.md`. That
|
||||
file is a mandatory checklist during any skill creation or regeneration.
|
||||
The user chooses propagation timing: immediate (update all skills now — for
|
||||
things like confidentiality rules) or opportunistic (apply at each skill's
|
||||
next update).
|
||||
|
||||
```markdown
|
||||
# Cross-Cutting Principles
|
||||
|
||||
Principles that apply to all skills. Read as a mandatory checklist during
|
||||
any skill creation or regeneration.
|
||||
|
||||
---
|
||||
|
||||
## Active Principles
|
||||
|
||||
### 1. [Principle title]
|
||||
**Added:** [date]
|
||||
**Applies to:** [all skills | all open-source skills | all skills with rules]
|
||||
**Requirement:** [what it requires]
|
||||
**Propagation:** [immediate | opportunistic]
|
||||
**Status:** [active]
|
||||
```
|
||||
@@ -0,0 +1,201 @@
|
||||
# Comprehensive Review (scheduled or fallback)
|
||||
|
||||
Cross-checks all OPEN observations against all skills, propagates
|
||||
cross-cutting principles, and applies improvements that don't need user
|
||||
input. Two modes:
|
||||
|
||||
- **Scheduled autonomous review (preferred):** a recurring task (e.g.
|
||||
Mon/Wed/Fri mornings) via the platform's scheduler. Runs without the user
|
||||
present and applies non-escalated observations autonomously.
|
||||
- **In-session 7-day fallback:** pending at session start when BOTH are
|
||||
true: no scheduled review is registered (or none succeeded in 7+ days),
|
||||
AND `skill-observations/last-review-date.txt` contains `never` or a date
|
||||
more than 7 days old (a missing file is recreated with `never` — see
|
||||
Session Start steps 1 and 3; the file's value is authoritative, a date
|
||||
means a review actually ran). In an interactive session a pending
|
||||
fallback surfaces as a one-line offer and runs only if the user opts in
|
||||
(SKILL.md, Session Start step 3) — it never gates the user's task.
|
||||
|
||||
**Reachability — where does scheduled work actually run?** Scheduled mode
|
||||
requires the scheduling agent's execution environment to read and write
|
||||
the workspace folder. Persistence and execution context are independent
|
||||
axes: knowing where the state lives is not enough — check whether the
|
||||
scheduler runs somewhere that can reach it. Three regimes:
|
||||
|
||||
1. **Shared filesystem** (e.g. Cowork's mounted folder): scheduled mode
|
||||
works as described.
|
||||
2. **Local-only filesystem with a cloud scheduler** (e.g. remote routines
|
||||
that run on hosted infrastructure): scheduled mode is physically broken
|
||||
— the remote agent cannot read `skill-observations/` or stage updates
|
||||
to `skill-updates/`. Do not register a routine. Recommend a recurring
|
||||
calendar reminder plus a manual "run the skill review" trigger in a
|
||||
local session, or syncing the observation log to storage the scheduler
|
||||
can reach (e.g. a git repository it can clone).
|
||||
3. **Local-only filesystem with a local scheduler** (cron, Task Scheduler,
|
||||
a terminal-resident loop): works, but the user must keep the local
|
||||
agent runnable.
|
||||
|
||||
## Approval policy
|
||||
|
||||
**Interactive (user present):** always present observations grouped by
|
||||
skill (number, title, one-sentence summary), flag judgment calls as "needs
|
||||
your input", and wait for blanket or selective approval before applying.
|
||||
|
||||
**Scheduled autonomous (user absent):** apply non-escalated observations by
|
||||
default — safety comes from the staging-plus-review pattern (nothing is
|
||||
live until the user installs it). **Escalate without applying** when: (1)
|
||||
the observation proposes a NEW skill (naming/scope/type/licence need the
|
||||
user); (2) it removes or substantially restructures existing content; (3)
|
||||
it self-flags uncertainty ("not sure if…", "worth discussing…"); (4) two
|
||||
observations conflict. A scheduled run should still apply every
|
||||
non-escalated item — a review that applies nothing is just a report
|
||||
generator.
|
||||
|
||||
## Steps
|
||||
|
||||
**Step 0 — recommend scheduled setup (fallback mode only).** Ordering
|
||||
guard: run Step 1's no-observations short-circuit FIRST — if there are no
|
||||
OPEN observations and no outstanding principles, skip Step 0 entirely and
|
||||
just update the timestamp. A brand-new install must never get a setup
|
||||
prompt before it has done any work. Otherwise: check
|
||||
`skill-observations/scheduled-review-decline.txt`: if under 30 days old and
|
||||
the fallback isn't firing repeatedly, skip. Check for a registered
|
||||
scheduled task (scheduler presence or
|
||||
`skill-observations/scheduler-registered.txt`); if found, skip. Before
|
||||
offering, check reachability (see the regimes above): if the platform's
|
||||
scheduler runs where it cannot reach the workspace folder (regime 2), do
|
||||
NOT offer registration — recommend the calendar-reminder-plus-manual-
|
||||
trigger pattern instead, and skip the rest of this step. Otherwise
|
||||
offer to set one up. Yes → register via the platform scheduler (Cowork:
|
||||
`create-shortcut` / `set_scheduled_task`; terminal: cron), name it
|
||||
`weekly-skill-review`, use the draft prompt at
|
||||
`skill-observations/scheduled-task-draft.md` if present, then verify the
|
||||
registration actually succeeded (the scheduler lists the task, or the
|
||||
platform confirmed creation) BEFORE writing today's date to
|
||||
`scheduler-registered.txt`. If registration fails or can't be verified, do
|
||||
NOT write the marker — the marker would permanently suppress the fallback
|
||||
while no review ever runs. Tell the user registration failed and leave the
|
||||
fallback active. No → write today's date to
|
||||
`scheduled-review-decline.txt` (suppresses for 30 days; repeated fallback
|
||||
firings within the window re-surface the offer). No scheduler available in
|
||||
this environment → skip silently.
|
||||
|
||||
**Step 1 — load.** Archive entries resolved in *previous* sessions (see
|
||||
Archival on Write in SKILL.md). Read the observation log.
|
||||
|
||||
Build the work queue from the structural identifiers, not from a status
|
||||
filter. The OPEN set is defined as: **status is literally OPEN, OR the
|
||||
observation has no Status line at all.** Concretely:
|
||||
|
||||
1. Enumerate all `### Observation N:` headers first — this is the
|
||||
authoritative list of entries in the log.
|
||||
2. For each header, classify the entry's status by looking for a
|
||||
`**Status:**` line within its body. Treat a missing, blank, or any
|
||||
non-ACTIONED / non-DECLINED status as OPEN.
|
||||
3. Never derive the work queue from a `grep '**Status:** OPEN'` alone.
|
||||
Derive it from the header list minus the resolved (ACTIONED /
|
||||
DECLINED) entries. A grep on an optional field silently drops every
|
||||
entry missing that field — the review then confidently reports a
|
||||
clean log while a backlog of untriaged observations is skipped.
|
||||
|
||||
**Reconciliation guard:** before proceeding, assert that
|
||||
`count(### Observation headers) == count(status-classified entries)`.
|
||||
If the counts differ, the delta is statusless entries — surface and
|
||||
triage them (as OPEN) rather than proceeding as if the log were clean.
|
||||
|
||||
Also read all active cross-cutting principles. If there are no OPEN
|
||||
observations and no outstanding principles: report "no open observations
|
||||
or outstanding principles", update the timestamp, and stop.
|
||||
|
||||
**Step 2 — inventory skills.** List all skills (system prompt
|
||||
`<available_skills>` or the skills directory). Only user-owned custom
|
||||
skills can be updated. Known read-only system skills: docx, pdf, xlsx,
|
||||
pptx, skill-creator, schedule (grow this list when an update fails for
|
||||
permissions). Observations targeting a system skill are NOT skipped — route
|
||||
them to a complementary user-owned `{system-skill}-extras` skill containing
|
||||
only the delta, creating it if needed and noting the pairing in
|
||||
configuration.
|
||||
|
||||
**Step 3 — cross-check observations.** Evaluate every OPEN observation
|
||||
against every skill — not just the skill named in its header; Principles
|
||||
often generalise. Build skill → [relevant observations]. Interactive:
|
||||
present all of it and await approval. Autonomous: apply the approval policy
|
||||
above and continue.
|
||||
|
||||
**Step 4 — cross-check principles.** Flag every skill that doesn't yet
|
||||
comply with each active cross-cutting principle.
|
||||
|
||||
**Step 5 — apply.** For each skill with approved/non-escalated items,
|
||||
produce an updated SKILL.md: integrate insights into the sections where
|
||||
they belong (never append an observations list at the bottom); preserve
|
||||
structure, voice, and attribution; place new rules where they logically
|
||||
live. Follow the editing rules in `references/skill-authoring.md` (live
|
||||
file as base, staging, diff-before-overwrite).
|
||||
|
||||
**Step 6 — mark ACTIONED.** Update each applied observation's status:
|
||||
`ACTIONED (YYYY-MM-DD) — Applied to [skill-name] (weekly review)`. The
|
||||
date immediately after the status word is load-bearing: archival is gated
|
||||
on it (entries archive only when it's before today), so a dateless mark
|
||||
breaks the cross-session grace period. Do NOT archive same-session — the
|
||||
next log write on a later day archives them.
|
||||
|
||||
**Step 7 — timestamp.** Write today's date to
|
||||
`skill-observations/last-review-date.txt`.
|
||||
|
||||
**Step 8 — deliver and summarise.** Stage updated skills (see Delivery
|
||||
below), then present:
|
||||
|
||||
```
|
||||
## Weekly Skill Review Complete — [date]
|
||||
|
||||
Updated skills ([N] observations, [N] principles applied):
|
||||
|
||||
**[skill-name]** — [1-sentence change summary]; observations #[N], #[N]
|
||||
|
||||
### Observations Actioned
|
||||
[numbers and titles]
|
||||
|
||||
### Skipped (needs manual review)
|
||||
[items with reasons]
|
||||
```
|
||||
|
||||
Wait for the user to acknowledge before other work.
|
||||
|
||||
## Constraints
|
||||
|
||||
- Don't modify observation entries beyond their status field.
|
||||
- Don't create new skills in a review — note candidates for the user to
|
||||
action via the skill-creator.
|
||||
- Unsure how to integrate an observation → skip it and say so in the
|
||||
summary.
|
||||
- Treat internal observations with the same rigour as open-source.
|
||||
|
||||
## Delivering updated skills
|
||||
|
||||
Save each updated skill to
|
||||
`[workspace folder]/skill-updates/[date]/[skill-name]/` — the FULL skill
|
||||
directory (SKILL.md plus references/, scripts/, assets/ where present),
|
||||
never SKILL.md alone — and present it for review and installation. In
|
||||
Cowork: via `present_files` and its upload button. In environments without
|
||||
a presentation tool (e.g. Claude Code CLI): report the staged path and a
|
||||
change summary in chat and let the user review and install from there.
|
||||
Never write to the live skill directly, even where the skills directory is
|
||||
writable — staging-only is a deliberate safety property of the review loop
|
||||
(nothing goes live without the user's sign-off), not a filesystem
|
||||
constraint. For any skill with
|
||||
supporting files, zip the staged directory into a `.skill` bundle and
|
||||
present the bundle; a bare SKILL.md install silently truncates a
|
||||
multi-file skill. Pre-delivery gate (two items, run as the last step
|
||||
before presenting): (1) grep the staged SKILL.md body for `references/`,
|
||||
`scripts/`, `assets/` paths and fail the delivery if any referenced file
|
||||
is missing from the staged set; (2) for multi-file skills, fail the
|
||||
delivery if the artefact being presented is bare file links rather than
|
||||
the `.skill` bundle. Sweep build artefacts (`__pycache__/`, `*.pyc`,
|
||||
`.DS_Store`, `.~lock.*`) before zipping and read the archive listing back
|
||||
after. When seeding staged
|
||||
copies from the read-only mount, `chmod -R u+w` the staged path first —
|
||||
the mount's read-only mode travels with the copy, for directories as
|
||||
well as files. Do not edit skill files in place — nothing goes live
|
||||
until the user installs it. **Keep-two rule:** for any skill, keep only
|
||||
the two most recent date directories under `skill-updates/`; delete
|
||||
older ones.
|
||||
@@ -253,7 +253,7 @@ scenarios.f6_prove_is_serial = async () => {
|
||||
const proveCalls = calls.filter((c) => String(c.opts.label).startsWith('prove:'))
|
||||
assert.equal(proveCalls.length, 2)
|
||||
for (const c of proveCalls) {
|
||||
assert.equal(c.opts.model, 'sonnet')
|
||||
assert.equal(c.opts.model, 'opus')
|
||||
assert.equal(c.opts.effort, 'medium')
|
||||
}
|
||||
assert.equal(result.commits.length, 2)
|
||||
|
||||
@@ -133,7 +133,9 @@ function fixPrompt(cluster) {
|
||||
` 4. You may edit a shared file outside your own cluster (${cluster.file}) when that is where the ` +
|
||||
`root cause lives. If you do, you MUST list every SOURCE file you modified - including this cluster's ` +
|
||||
`own file - in touchedPaths, repo-relative with forward slashes. Test files belong in testPath, not ` +
|
||||
`touchedPaths.\n` +
|
||||
`touchedPaths. If you regenerate shared generated output (sqlc, protocol), list EVERY generated ` +
|
||||
`file the regeneration changed - check with git status --porcelain (read-only, allowed despite ` +
|
||||
`rule 6), do not guess: an unlisted generated file defeats the cross-cluster overlap guard.\n` +
|
||||
` 5. Because several findings share this file, look for one change that closes more than one of them ` +
|
||||
`before writing separate patches.\n` +
|
||||
` 6. DO NOT run any git command. No add, no commit, no stash, no checkout. Other agents are working ` +
|
||||
@@ -313,12 +315,17 @@ function provePrompt(cluster, fixedIds, testPaths, sourcePaths) {
|
||||
` 6. Run the tests again. They MUST pass. Set greenObserved accordingly. Capture the ACTUAL output ` +
|
||||
`of this run, including the command you ran, and return it verbatim in greenOutput - not a summary, ` +
|
||||
`not a paraphrase. If they do not pass, set committed=false, explain in note, and STOP.\n` +
|
||||
` 7. Stage ALL source file(s) listed above (git add ${sourcePaths.join(' ')}) AND the test files, ` +
|
||||
` 7. Before staging, diff every file you are about to commit and check the content belongs to ` +
|
||||
`THIS cluster: other agents' uncommitted work shares this tree, and shared test files or ` +
|
||||
`regenerated output can carry their hunks. A test function or comment citing a finding id not ` +
|
||||
`listed above, or a hunk in a generated/shared file unrelated to your findings, must NOT be ` +
|
||||
`committed - set committed=false, name the foreign content in note, and STOP.\n` +
|
||||
` 8. Stage ALL source file(s) listed above (git add ${sourcePaths.join(' ')}) AND the test files, ` +
|
||||
`then commit with subject:\n` +
|
||||
` fix(<area>): ${fixedIds.length} defect(s) (${fixedIds.join(', ')})\n` +
|
||||
` Use a conventional-commit area matching the file (voice, ws, client, identity...). Do not add a ` +
|
||||
`Co-Authored-By trailer.\n` +
|
||||
` 8. Return the short sha.\n\n` +
|
||||
` 9. Return the short sha.\n\n` +
|
||||
`Client tests run from Client/tauri-client with:\n` +
|
||||
` NODE_OPTIONS=--no-experimental-webstorage npx vitest run <testfile>\n` +
|
||||
`Server tests run from Server with:\n` +
|
||||
@@ -353,10 +360,13 @@ for (const { cluster, results, union } of fixed) {
|
||||
// A dead/thrown prove agent must not take down the sibling clusters still waiting in this
|
||||
// serial loop - same "one blocked cluster does not poison the rest" rule Phase 2 gets from
|
||||
// parallel()'s catch. Fold it into a null result so the ok/why logic below handles it uniformly.
|
||||
// Opus on purpose: prove is the last eyes before content reaches a public commit. The
|
||||
// 2026-08-14 run's sonnet prove agents staged a sibling cluster's test function and a
|
||||
// regenerated-file hunk from another cluster's uncommitted work without noticing either.
|
||||
const p = await agent(provePrompt(cluster, ids, testPaths, union), {
|
||||
label: `prove:${cluster.file}`,
|
||||
phase: 'Prove',
|
||||
model: 'sonnet',
|
||||
model: 'opus',
|
||||
effort: 'medium',
|
||||
schema: PROVE_RESULT,
|
||||
}).catch(() => null)
|
||||
|
||||
@@ -299,6 +299,11 @@ export function createConnectPage(
|
||||
// failure rather than whatever set it during this session.
|
||||
setTransientError(null);
|
||||
|
||||
// Mirror MainPage.destroy(): clear settingsOpen so the next page to
|
||||
// mount an (initially hidden) SettingsOverlay off that flag — MainPage,
|
||||
// after a successful login — doesn't show it over the freshly loaded app.
|
||||
closeSettings();
|
||||
|
||||
if (container && root) {
|
||||
container.removeChild(root);
|
||||
}
|
||||
|
||||
@@ -577,7 +577,15 @@ export function createMainPage(options: MainPageOptions): MountableComponent {
|
||||
);
|
||||
unsubscribers.push(
|
||||
ws.on("call_declined", (payload) => {
|
||||
ringCtrl?.cancel(payload.channel_id);
|
||||
// Addressed to every other DM participant, not just the caller — the
|
||||
// server holds no call state to target with (see handlers_call.go).
|
||||
// In a group DM that includes fellow callees who are also ringing;
|
||||
// only the actual ringer declining should silence this client's ring.
|
||||
const ringing = ringCtrl?.current();
|
||||
if (ringing === null || ringing === undefined) return;
|
||||
if (payload.from_user === ringing.fromUserId) {
|
||||
ringCtrl?.cancel(payload.channel_id);
|
||||
}
|
||||
}),
|
||||
);
|
||||
// The ringer hanging up before anyone answered: their voice_leave is the
|
||||
|
||||
@@ -169,6 +169,12 @@ export function createInviteManagerController(opts: {
|
||||
opening = true;
|
||||
try {
|
||||
const raw = await opts.api.getInvites();
|
||||
// Re-derive liveness: a page teardown during the fetch nulls the root
|
||||
// MainPage handed out, but the pre-await `root` const above still
|
||||
// points at the now-detached node. Mounting on it anyway would create
|
||||
// an instance whose document-level listeners nothing ever tears down.
|
||||
const liveRoot = opts.getRoot();
|
||||
if (liveRoot === null) return;
|
||||
const invites = raw.filter((r) => !isInviteRevoked(r)).map(mapInviteResponse);
|
||||
instance = createInviteManager({
|
||||
invites,
|
||||
@@ -198,9 +204,7 @@ export function createInviteManagerController(opts: {
|
||||
showToast(message, "error");
|
||||
},
|
||||
});
|
||||
if (root !== null) {
|
||||
instance.mount(root);
|
||||
}
|
||||
instance.mount(liveRoot);
|
||||
} catch (err) {
|
||||
log.error("Failed to open invite manager", { error: String(err) });
|
||||
showToast("Failed to load invites", "error");
|
||||
@@ -261,6 +265,11 @@ export function createPinnedPanelController(opts: {
|
||||
opening = true;
|
||||
try {
|
||||
const resp = await opts.api.getPins(channelId);
|
||||
// Re-derive liveness: a page teardown during the fetch nulls the root
|
||||
// MainPage handed out, but the pre-await `root` const above still
|
||||
// points at the now-detached node — see InviteManagerController.open.
|
||||
const liveRoot = opts.getRoot();
|
||||
if (liveRoot === null) return;
|
||||
const pins = resp.messages.map(mapToPinnedMessage);
|
||||
instance = createPinnedMessages({
|
||||
channelId,
|
||||
@@ -286,9 +295,7 @@ export function createPinnedPanelController(opts: {
|
||||
},
|
||||
onClose: close,
|
||||
});
|
||||
if (root !== null) {
|
||||
instance.mount(root);
|
||||
}
|
||||
instance.mount(liveRoot);
|
||||
} catch (err) {
|
||||
log.error("Failed to load pinned messages", { error: String(err) });
|
||||
showToast("Failed to load pinned messages", "error");
|
||||
|
||||
@@ -6,6 +6,7 @@
|
||||
import { createElement } from "@lib/dom";
|
||||
import { createEmojiPicker } from "@components/EmojiPicker";
|
||||
import { addOptimisticReaction, getChannelMessages } from "@stores/messages.store";
|
||||
import { listCustomEmoji } from "@stores/emoji.store";
|
||||
import type { WsClient } from "@lib/ws";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -82,6 +83,10 @@ export function createReactionController(opts: ReactionControllerOptions): React
|
||||
});
|
||||
|
||||
const picker = createEmojiPicker({
|
||||
// Read the set at open time, not at controller construction: an
|
||||
// emoji_update while the app is alive must be in the next picker the
|
||||
// user opens (matches MessageInput.ts's composer picker).
|
||||
customEmoji: listCustomEmoji(),
|
||||
onSelect: (selectedEmoji: string) => {
|
||||
closePicker();
|
||||
sendReaction(msgId, selectedEmoji);
|
||||
|
||||
@@ -112,6 +112,13 @@ export function createSidebarArea(opts: SidebarAreaOptions): SidebarAreaResult {
|
||||
// InviteManagerController / PinnedPanelController in OverlayManagers.ts.
|
||||
let openingQuickSwitch = false;
|
||||
|
||||
// Set once this SidebarArea's own teardown unsubscriber has run.
|
||||
// `sidebarWrapper.parentElement` can NOT be used for this: MainPage.destroy()
|
||||
// removes an ancestor (`root`) and never detaches sidebarWrapper from its
|
||||
// own parent, so that check is never true and a profile load that resolves
|
||||
// after teardown mounts an orphaned overlay onto document.body.
|
||||
let tornDown = false;
|
||||
|
||||
// Track the rename-group prompt so page teardown removes it — every other
|
||||
// modal in this file assigns `activeModal` for the same reason.
|
||||
let activePrompt: ModalInstance | null = null;
|
||||
@@ -728,7 +735,7 @@ export function createSidebarArea(opts: SidebarAreaOptions): SidebarAreaResult {
|
||||
}
|
||||
|
||||
// Ensure we haven't been cleaned up while awaiting
|
||||
if (sidebarWrapper.parentElement === null) return;
|
||||
if (tornDown) return;
|
||||
|
||||
quickSwitchInstance = createQuickSwitchOverlay({
|
||||
profiles,
|
||||
@@ -809,6 +816,7 @@ export function createSidebarArea(opts: SidebarAreaOptions): SidebarAreaResult {
|
||||
});
|
||||
|
||||
unsubscribers.push(() => {
|
||||
tornDown = true;
|
||||
closeQuickSwitch();
|
||||
});
|
||||
|
||||
|
||||
@@ -989,6 +989,21 @@ describe("ConnectPage", () => {
|
||||
page.destroy?.();
|
||||
});
|
||||
|
||||
// OC-0083: destroy() must clear settingsOpen — otherwise MainPage, which
|
||||
// mounts its own SettingsOverlay eagerly and shows it off a stale
|
||||
// settingsOpen === true, pops the settings panel open over the freshly
|
||||
// loaded app right after login.
|
||||
it("closes settings on destroy so the flag doesn't leak into the next page", async () => {
|
||||
const { closeSettings } = await import("../../src/stores/ui.store");
|
||||
vi.mocked(closeSettings).mockClear();
|
||||
const page = createConnectPage(makeCallbacks(), testProfiles);
|
||||
page.mount(container);
|
||||
|
||||
page.destroy?.();
|
||||
|
||||
expect(closeSettings).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
// --- setCredentials with password sets remember checkbox ---
|
||||
|
||||
it("setCredentials with password checks the remember password checkbox", async () => {
|
||||
|
||||
@@ -462,6 +462,35 @@ describe("MainPage — video grid, DM profile panel, calls, settings", () => {
|
||||
expect(banner.style.display).not.toBe("none");
|
||||
});
|
||||
|
||||
it("does not cancel an incoming ring when a fellow group-DM callee declines, only when the actual ringer does (OC-0114)", () => {
|
||||
const ws = fakeWs();
|
||||
uiStore.setState((prev) => ({ ...prev, connectionStatus: "connected" }));
|
||||
|
||||
page = createMainPage({ ws, api: fakeApi() });
|
||||
page.mount(container);
|
||||
|
||||
// Alice (10) rings a group DM; this client is a third participant.
|
||||
ws.emit("call_incoming", { channel_id: 50, from_user: 10, username: "alice" });
|
||||
|
||||
const banner = document.querySelector('[data-testid="incoming-call-banner"]') as HTMLElement;
|
||||
expect(banner.style.display).not.toBe("none");
|
||||
|
||||
// Bob (11), a different callee in the same group DM, declines. The
|
||||
// server addresses call_declined to every other participant (not just
|
||||
// the caller — it holds no call state to target with), so this client
|
||||
// receives it too, but it must not silence a ring it is still deciding
|
||||
// on: Bob declining is not Alice hanging up.
|
||||
ws.emit("call_declined", { channel_id: 50, from_user: 11, username: "bob" });
|
||||
|
||||
expect(banner.style.display).not.toBe("none");
|
||||
|
||||
// The actual ringer's own call_declined (e.g. a glare decline) still
|
||||
// cancels it.
|
||||
ws.emit("call_declined", { channel_id: 50, from_user: 10, username: "alice" });
|
||||
|
||||
expect(banner.style.display).toBe("none");
|
||||
});
|
||||
|
||||
it("clears settingsOpen on destroy so the next page (e.g. ConnectPage after logout) doesn't inherit a stale open overlay", () => {
|
||||
page = createMainPage({ ws: fakeWs(), api: fakeApi() });
|
||||
page.mount(container);
|
||||
|
||||
@@ -513,6 +513,29 @@ describe("createPinnedPanelController", () => {
|
||||
expect(api.getPins).toHaveBeenCalledOnce();
|
||||
});
|
||||
|
||||
it("re-checks getRoot after the getPins() await and does not mount on a torn-down page", async () => {
|
||||
// Same teardown-during-fetch race as InviteManagerController.open (OC-0055):
|
||||
// toggle() must not mount on the pre-await root once the page has torn
|
||||
// down while getPins() was in flight.
|
||||
const api = makeMockApi();
|
||||
let calls = 0;
|
||||
const getRoot = vi.fn(() => {
|
||||
calls++;
|
||||
return calls === 1 ? root : null;
|
||||
});
|
||||
|
||||
const controller = createPinnedPanelController({
|
||||
api: api as never,
|
||||
getRoot,
|
||||
getCurrentChannelId: () => 42,
|
||||
});
|
||||
|
||||
await controller.toggle();
|
||||
|
||||
expect(createPinnedMessages).not.toHaveBeenCalled();
|
||||
expect(mockPinnedMessagesMount).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("cleanup is safe when no panel is open", () => {
|
||||
const api = makeMockApi();
|
||||
|
||||
@@ -1049,6 +1072,30 @@ describe("createInviteManagerController (additional)", () => {
|
||||
expect(mockShowToast).toHaveBeenCalledWith("Something went wrong", "error");
|
||||
});
|
||||
|
||||
it("re-checks getRoot after the getInvites() await and does not mount on a torn-down page", async () => {
|
||||
// Root is live when open() starts but the page tears down (MainPage.destroy
|
||||
// nulls its root) while getInvites() is still in flight. open() must not
|
||||
// resurrect the overlay on the stale, detached root it captured before the
|
||||
// await — that leaves a document-level keydown listener with no teardown
|
||||
// path (OC-0055).
|
||||
const api = makeMockApi();
|
||||
let calls = 0;
|
||||
const getRoot = vi.fn(() => {
|
||||
calls++;
|
||||
return calls === 1 ? root : null;
|
||||
});
|
||||
|
||||
const controller = createInviteManagerController({
|
||||
api: api as never,
|
||||
getRoot,
|
||||
});
|
||||
|
||||
await controller.open();
|
||||
|
||||
expect(createInviteManager).not.toHaveBeenCalled();
|
||||
expect(mockInviteManagerMount).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("onRevokeInvite passes invite code directly to API", async () => {
|
||||
const api = makeMockApi();
|
||||
|
||||
|
||||
@@ -7,16 +7,41 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||
const {
|
||||
mockGetChannelMessages,
|
||||
mockAddOptimisticReaction,
|
||||
createMockEmojiPickerElement,
|
||||
mockEmojiPickerDestroy,
|
||||
} = vi.hoisted(() => ({
|
||||
mockGetChannelMessages: vi.fn(
|
||||
(): Array<{ id: number; reactions: Array<{ emoji: string; me: boolean }> }> => [],
|
||||
),
|
||||
mockAddOptimisticReaction: vi.fn(),
|
||||
createMockEmojiPickerElement: () => document.createElement("div"),
|
||||
mockEmojiPickerDestroy: vi.fn(),
|
||||
}));
|
||||
mockListCustomEmoji,
|
||||
mockCreateEmojiPicker,
|
||||
captured,
|
||||
} = vi.hoisted(() => {
|
||||
const captured: {
|
||||
onSelect: ((emoji: string) => void) | null;
|
||||
onClose: (() => void) | null;
|
||||
} = { onSelect: null, onClose: null };
|
||||
const createMockEmojiPickerElement = () => document.createElement("div");
|
||||
const mockEmojiPickerDestroy = vi.fn();
|
||||
return {
|
||||
mockGetChannelMessages: vi.fn(
|
||||
(): Array<{ id: number; reactions: Array<{ emoji: string; me: boolean }> }> => [],
|
||||
),
|
||||
mockAddOptimisticReaction: vi.fn(),
|
||||
mockEmojiPickerDestroy,
|
||||
mockListCustomEmoji: vi.fn((): Array<{ id: number; shortcode: string; url: string }> => []),
|
||||
mockCreateEmojiPicker: vi.fn(
|
||||
(opts: {
|
||||
onSelect: (e: string) => void;
|
||||
onClose: () => void;
|
||||
customEmoji?: readonly { id: number; shortcode: string; url: string }[];
|
||||
}) => {
|
||||
captured.onSelect = opts.onSelect;
|
||||
captured.onClose = opts.onClose;
|
||||
return {
|
||||
element: createMockEmojiPickerElement(),
|
||||
destroy: mockEmojiPickerDestroy,
|
||||
};
|
||||
},
|
||||
),
|
||||
captured,
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("@lib/dom", () => ({
|
||||
createElement: vi.fn((tag: string, attrs?: Record<string, string>) => {
|
||||
@@ -32,18 +57,8 @@ vi.mock("@lib/dom", () => ({
|
||||
}),
|
||||
}));
|
||||
|
||||
let capturedOnSelect: ((emoji: string) => void) | null = null;
|
||||
let capturedOnClose: (() => void) | null = null;
|
||||
|
||||
vi.mock("@components/EmojiPicker", () => ({
|
||||
createEmojiPicker: vi.fn((opts: { onSelect: (e: string) => void; onClose: () => void }) => {
|
||||
capturedOnSelect = opts.onSelect;
|
||||
capturedOnClose = opts.onClose;
|
||||
return {
|
||||
element: createMockEmojiPickerElement(),
|
||||
destroy: mockEmojiPickerDestroy,
|
||||
};
|
||||
}),
|
||||
createEmojiPicker: mockCreateEmojiPicker,
|
||||
}));
|
||||
|
||||
vi.mock("@stores/messages.store", () => ({
|
||||
@@ -51,6 +66,10 @@ vi.mock("@stores/messages.store", () => ({
|
||||
addOptimisticReaction: mockAddOptimisticReaction,
|
||||
}));
|
||||
|
||||
vi.mock("@stores/emoji.store", () => ({
|
||||
listCustomEmoji: mockListCustomEmoji,
|
||||
}));
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Imports (after mocks)
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -87,8 +106,9 @@ function makeOpts(overrides: Partial<ReactionControllerOptions> = {}): ReactionC
|
||||
describe("createReactionController", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
capturedOnSelect = null;
|
||||
capturedOnClose = null;
|
||||
mockListCustomEmoji.mockReturnValue([]);
|
||||
captured.onSelect = null;
|
||||
captured.onClose = null;
|
||||
// Clean up any leftover DOM elements
|
||||
document.querySelectorAll(".reaction-picker-wrap").forEach((el) => el.remove());
|
||||
document.querySelectorAll("[data-testid]").forEach((el) => el.remove());
|
||||
@@ -176,6 +196,32 @@ describe("createReactionController", () => {
|
||||
expect(document.querySelector(".reaction-picker-wrap")).toBeNull();
|
||||
});
|
||||
|
||||
it("passes the server's custom emoji to the picker (OC-0149)", () => {
|
||||
const customEmoji = [{ id: 1, shortcode: "partyparrot", url: "/api/v1/emoji/1/image" }];
|
||||
mockListCustomEmoji.mockReturnValue(customEmoji);
|
||||
|
||||
const btn = document.createElement("button");
|
||||
btn.setAttribute("data-testid", "msg-react-1");
|
||||
btn.getBoundingClientRect = vi.fn(() => ({
|
||||
left: 500,
|
||||
right: 530,
|
||||
top: 100,
|
||||
bottom: 130,
|
||||
width: 30,
|
||||
height: 30,
|
||||
x: 500,
|
||||
y: 100,
|
||||
toJSON: () => {},
|
||||
}));
|
||||
document.body.appendChild(btn);
|
||||
|
||||
const opts = makeOpts();
|
||||
const ctrl = createReactionController(opts);
|
||||
ctrl.handleReaction(1, "");
|
||||
|
||||
expect(mockCreateEmojiPicker).toHaveBeenCalledWith(expect.objectContaining({ customEmoji }));
|
||||
});
|
||||
|
||||
it("opens picker when react button exists", () => {
|
||||
const btn = document.createElement("button");
|
||||
btn.setAttribute("data-testid", "msg-react-1");
|
||||
@@ -249,8 +295,8 @@ describe("createReactionController", () => {
|
||||
ctrl.handleReaction(1, "");
|
||||
|
||||
// Simulate emoji selection from picker
|
||||
expect(capturedOnSelect).not.toBeNull();
|
||||
capturedOnSelect!("🎉");
|
||||
expect(captured.onSelect).not.toBeNull();
|
||||
captured.onSelect!("🎉");
|
||||
|
||||
expect(opts.ws.send).toHaveBeenCalledWith({
|
||||
type: "reaction_add",
|
||||
@@ -281,7 +327,7 @@ describe("createReactionController", () => {
|
||||
const ctrl = createReactionController(opts);
|
||||
ctrl.handleReaction(1, "");
|
||||
|
||||
capturedOnSelect!("🎉");
|
||||
captured.onSelect!("🎉");
|
||||
|
||||
expect(mockEmojiPickerDestroy).toHaveBeenCalledOnce();
|
||||
});
|
||||
@@ -306,7 +352,7 @@ describe("createReactionController", () => {
|
||||
const ctrl = createReactionController(opts);
|
||||
ctrl.handleReaction(1, "");
|
||||
|
||||
capturedOnClose!();
|
||||
captured.onClose!();
|
||||
|
||||
expect(mockEmojiPickerDestroy).toHaveBeenCalledOnce();
|
||||
expect(document.querySelector(".reaction-picker-wrap")).toBeNull();
|
||||
|
||||
@@ -2087,6 +2087,45 @@ describe("SidebarArea", () => {
|
||||
|
||||
cleanup(result);
|
||||
});
|
||||
|
||||
it("does not mount an orphaned overlay if torn down while profiles are still loading", async () => {
|
||||
// MainPage.destroy() removes an ANCESTOR of sidebarWrapper (`root`) and
|
||||
// never detaches sidebarWrapper from its own parent, so
|
||||
// `sidebarWrapper.parentElement` stays non-null forever — it cannot be
|
||||
// used as the "were we torn down while awaiting?" check. Reproduce that
|
||||
// exactly: sidebarWrapper stays attached to `container` for the whole
|
||||
// test, teardown runs (the real unsubscriber list, same as
|
||||
// MainPage.destroy()'s loop) while loadProfiles() is still pending, and
|
||||
// only then does the profile load resolve.
|
||||
let resolveLoad!: () => void;
|
||||
const pending = new Promise<void>((resolve) => {
|
||||
resolveLoad = resolve;
|
||||
});
|
||||
(createProfileManager as MockedFn).mockReturnValueOnce({
|
||||
loadProfiles: vi.fn().mockReturnValue(pending),
|
||||
getAll: vi.fn().mockReturnValue([]),
|
||||
store: { getState: () => ({ profiles: [], healthStatuses: new Map() }) },
|
||||
});
|
||||
|
||||
const callsBefore = (createQuickSwitchOverlay as MockedFn).mock.calls.length;
|
||||
|
||||
const result = createSidebarArea(defaultOpts());
|
||||
container.appendChild(result.sidebarWrapper);
|
||||
|
||||
result.openQuickSwitch();
|
||||
|
||||
// Teardown, as MainPage.destroy() runs it: every unsubscriber fires
|
||||
// (including the one that calls closeQuickSwitch()), but sidebarWrapper
|
||||
// is never removed from its parent.
|
||||
for (const unsub of result.unsubscribers) unsub();
|
||||
expect(result.sidebarWrapper.parentElement).not.toBeNull();
|
||||
|
||||
resolveLoad();
|
||||
await pending;
|
||||
|
||||
// No overlay should have been created for a page that no longer exists.
|
||||
expect((createQuickSwitchOverlay as MockedFn).mock.calls.length).toBe(callsBefore);
|
||||
});
|
||||
});
|
||||
|
||||
// -------------------------------------------------------------------------
|
||||
|
||||
@@ -279,6 +279,43 @@ func (rh *ringHandler) Enabled(level slog.Level) bool {
|
||||
return level >= rh.level.Level()
|
||||
}
|
||||
|
||||
// logAttrValue converts an slog.Value to the value stored in the ring
|
||||
// buffer's JSON attrs, matching what slog's own JSONHandler does for stdout
|
||||
// (see appendJSONValue in log/slog/json_handler.go):
|
||||
//
|
||||
// - Resolve() first, so an slog.LogValuer (db.User, db.Session,
|
||||
// config.Config and its secret-bearing sections — see
|
||||
// Server/db/logvalue.go and Server/config/logvalue.go) is redacted before
|
||||
// it reaches JSON. Record.Attrs does not resolve on its own, so without
|
||||
// this the ring buffer bypasses that redaction even though stdout honors it.
|
||||
// - error values marshal to "{}" (errors.errorString / fmt.wrapError have
|
||||
// only unexported fields), so an error not otherwise handled by
|
||||
// json.Marshal is rendered as its Error() string instead.
|
||||
// - a resolved group (LogValue returning slog.GroupValue, as every type
|
||||
// above does) is walked into a map instead of json.Marshal-ed as-is: a
|
||||
// bare []slog.Attr marshals to "{}" per element, since slog.Value's
|
||||
// fields are unexported. Resolve() only resolves the outer LogValuer, not
|
||||
// nested ones (see its doc comment), so nested group members are resolved
|
||||
// by this same recursive call.
|
||||
func logAttrValue(v slog.Value) any {
|
||||
v = v.Resolve()
|
||||
if v.Kind() == slog.KindGroup {
|
||||
group := v.Group()
|
||||
m := make(map[string]any, len(group))
|
||||
for _, a := range group {
|
||||
m[a.Key] = logAttrValue(a.Value)
|
||||
}
|
||||
return m
|
||||
}
|
||||
a := v.Any()
|
||||
if err, ok := a.(error); ok {
|
||||
if _, isJSONMarshaler := a.(json.Marshaler); !isJSONMarshaler {
|
||||
return err.Error()
|
||||
}
|
||||
}
|
||||
return a
|
||||
}
|
||||
|
||||
func (rh *ringHandler) Handle(r slog.Record) {
|
||||
// Build source from file path.
|
||||
source := categorizeSource(r)
|
||||
@@ -287,7 +324,7 @@ func (rh *ringHandler) Handle(r slog.Record) {
|
||||
attrs := make(map[string]any)
|
||||
// Add pre-set attrs from WithAttrs.
|
||||
for _, a := range rh.attrs {
|
||||
attrs[a.Key] = a.Value.Any()
|
||||
attrs[a.Key] = logAttrValue(a.Value)
|
||||
}
|
||||
// Add record attrs.
|
||||
r.Attrs(func(a slog.Attr) bool {
|
||||
@@ -295,7 +332,7 @@ func (rh *ringHandler) Handle(r slog.Record) {
|
||||
if len(rh.groups) > 0 {
|
||||
key = strings.Join(rh.groups, ".") + "." + key
|
||||
}
|
||||
attrs[key] = a.Value.Any()
|
||||
attrs[key] = logAttrValue(a.Value)
|
||||
return true
|
||||
})
|
||||
|
||||
|
||||
@@ -4,9 +4,13 @@ import (
|
||||
"bytes"
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/owncord/server/config"
|
||||
)
|
||||
|
||||
// The multiHandler tees every server log record into the admin panel's live
|
||||
@@ -316,6 +320,96 @@ func TestCategorizeSource_AttributesAdminPackage(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// ─── error attrs (OC-0110) ──────────────────────────────────────────────────
|
||||
//
|
||||
// Go error values (*errors.errorString, *fmt.wrapError) have only unexported
|
||||
// fields, so a bare json.Marshal of one produces "{}". slog's own JSONHandler
|
||||
// special-cases error attrs and emits err.Error() instead (see appendJSONValue
|
||||
// in log/slog/json_handler.go); the ring handler must match, or the admin log
|
||||
// viewer shows every error as an empty object while stdout shows the real
|
||||
// message.
|
||||
|
||||
func TestMultiHandler_ErrorAttr_RecordLevel(t *testing.T) {
|
||||
logger, buf, _ := newTeeLogger(t, slog.LevelDebug)
|
||||
|
||||
logger.Error("failed to issue log stream ticket",
|
||||
"err", fmt.Errorf("generating ticket: %w", errors.New("boom")))
|
||||
|
||||
entries := buf.Snapshot()
|
||||
if len(entries) != 1 {
|
||||
t.Fatalf("ring buffer has %d entries, want 1", len(entries))
|
||||
}
|
||||
|
||||
var attrs map[string]any
|
||||
if err := json.Unmarshal([]byte(entries[0].Attrs), &attrs); err != nil {
|
||||
t.Fatalf("unmarshal attrs %q: %v", entries[0].Attrs, err)
|
||||
}
|
||||
got, _ := attrs["err"].(string)
|
||||
if got != "generating ticket: boom" {
|
||||
t.Errorf("attrs[err] = %#v, want the error string %q (got attrs %q)",
|
||||
attrs["err"], "generating ticket: boom", entries[0].Attrs)
|
||||
}
|
||||
}
|
||||
|
||||
func TestMultiHandler_ErrorAttr_WithAttrsLevel(t *testing.T) {
|
||||
logger, buf, _ := newTeeLogger(t, slog.LevelDebug)
|
||||
|
||||
logger.With("err", errors.New("boom")).Error("failed")
|
||||
|
||||
entries := buf.Snapshot()
|
||||
if len(entries) != 1 {
|
||||
t.Fatalf("ring buffer has %d entries, want 1", len(entries))
|
||||
}
|
||||
|
||||
var attrs map[string]any
|
||||
if err := json.Unmarshal([]byte(entries[0].Attrs), &attrs); err != nil {
|
||||
t.Fatalf("unmarshal attrs %q: %v", entries[0].Attrs, err)
|
||||
}
|
||||
got, _ := attrs["err"].(string)
|
||||
if got != "boom" {
|
||||
t.Errorf("attrs[err] = %#v, want the error string %q (got attrs %q)",
|
||||
attrs["err"], "boom", entries[0].Attrs)
|
||||
}
|
||||
}
|
||||
|
||||
// TestMultiHandler_LogValuerResolved pins the sibling half of OC-0110's fix:
|
||||
// Record.Attrs (and a bare a.Value.Any()) does not call slog.Value.Resolve(),
|
||||
// so an slog.LogValuer attr — e.g. config.VoiceConfig, whose LogValue redacts
|
||||
// the LiveKit secret (Server/config/logvalue.go) — reaches the ring buffer as
|
||||
// the raw, unresolved struct instead of the redacted one. stdout's JSONHandler
|
||||
// resolves it correctly; the admin log viewer must match, or a secret redacted
|
||||
// everywhere else leaks into the searchable/exportable admin log stream.
|
||||
func TestMultiHandler_LogValuerResolved(t *testing.T) {
|
||||
logger, buf, _ := newTeeLogger(t, slog.LevelDebug)
|
||||
|
||||
logger.Info("voice config", "voice", config.VoiceConfig{
|
||||
LiveKitAPIKey: "AKIA-plaintext-key",
|
||||
LiveKitAPISecret: "supersecretvalue",
|
||||
LiveKitURL: "wss://example.invalid",
|
||||
})
|
||||
|
||||
entries := buf.Snapshot()
|
||||
if len(entries) != 1 {
|
||||
t.Fatalf("ring buffer has %d entries, want 1", len(entries))
|
||||
}
|
||||
|
||||
if strings.Contains(entries[0].Attrs, "supersecretvalue") {
|
||||
t.Fatalf("ring buffer attrs leaked the unredacted LiveKit secret: %q", entries[0].Attrs)
|
||||
}
|
||||
|
||||
var attrs map[string]any
|
||||
if err := json.Unmarshal([]byte(entries[0].Attrs), &attrs); err != nil {
|
||||
t.Fatalf("unmarshal attrs %q: %v", entries[0].Attrs, err)
|
||||
}
|
||||
voice, ok := attrs["voice"].(map[string]any)
|
||||
if !ok {
|
||||
t.Fatalf("attrs[voice] = %#v (%T), want a resolved LogValue group", attrs["voice"], attrs["voice"])
|
||||
}
|
||||
if voice["livekit_api_secret"] != "[REDACTED]" {
|
||||
t.Errorf("voice[livekit_api_secret] = %#v, want the redacted marker", voice["livekit_api_secret"])
|
||||
}
|
||||
}
|
||||
|
||||
func TestMultiHandler_HandleReturnsNil(t *testing.T) {
|
||||
var stdout bytes.Buffer
|
||||
buf := NewRingBuffer(4)
|
||||
|
||||
@@ -24,16 +24,20 @@ type DMBroadcaster interface {
|
||||
// warm-reconnects across the gap takes the full-ready path instead of a
|
||||
// sequenced-only replay that can never redeliver it. Reached by type
|
||||
// assertion rather than being added to DMBroadcaster directly so the
|
||||
// SendToUser-only test doubles in this package keep working.
|
||||
//
|
||||
// NOTE: *ws.Hub does not export this yet — its watermark bump
|
||||
// (ws/hub.go bumpVisibilityWatermark, the same one ws/emit.go forces for the
|
||||
// WS-side dm_channel_open) is unexported, so until a one-line exported
|
||||
// wrapper lands the assertion below misses and this is a no-op.
|
||||
// SendToUser-only test doubles in this package keep working. Satisfied in
|
||||
// production by ws.Hub.MarkVisibilityChanged, which forwards to the same
|
||||
// bumpVisibilityWatermark the WS-side dm_channel_open emitter uses
|
||||
// (ws/emit.go).
|
||||
type dmVisibilityMarker interface {
|
||||
MarkVisibilityChanged()
|
||||
}
|
||||
|
||||
// The production broadcaster must keep satisfying it: a type assertion that
|
||||
// silently stops matching would turn the watermark bump back into the no-op
|
||||
// this fixed, with nothing failing to say so — mirrors the dmVoiceEvictor
|
||||
// assertion below for its sibling capability.
|
||||
var _ dmVisibilityMarker = (*ws.Hub)(nil)
|
||||
|
||||
// markDMVisibilityChanged bumps the visibility watermark if broadcaster
|
||||
// supports it. dm_channel_open/close are unsequenced and targeted, so a
|
||||
// client that misses one via a dropped connection and then warm-reconnects
|
||||
@@ -253,6 +257,12 @@ func broadcastDMOpen(ctx context.Context, svc *service.Services, broadcaster DMB
|
||||
if broadcaster == nil || len(targetIDs) == 0 {
|
||||
return
|
||||
}
|
||||
// The mutation that led here has already committed, so this fan-out must
|
||||
// survive the caller's request context being cancelled after that point
|
||||
// (client disconnect mid-handler) — otherwise every DMSummaryFor lookup
|
||||
// below fails with context.Canceled and no participant, including ones
|
||||
// otherwise unaffected by the cancellation, ever receives the open.
|
||||
ctx = context.WithoutCancel(ctx)
|
||||
// dm_channel_open is unsequenced and targeted — a recipient who is
|
||||
// offline or drops the connection right now can never have it replayed
|
||||
// to them by the ordinary seq-based resume path, so a warm reconnect must
|
||||
|
||||
@@ -0,0 +1,46 @@
|
||||
package api_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"testing"
|
||||
|
||||
"github.com/owncord/server/api"
|
||||
"github.com/owncord/server/auth"
|
||||
"github.com/owncord/server/service"
|
||||
)
|
||||
|
||||
// OC-0140: broadcastDMOpen is called with r.Context() by CreateGroupDM (and
|
||||
// the rename/group-leave refreshes). The channel row and its participants
|
||||
// have already committed by the time broadcastDMOpen runs, so if the caller's
|
||||
// request context is cancelled in the gap (client disconnect), every
|
||||
// DMSummaryFor lookup inside the fan-out loop fails with context.Canceled and
|
||||
// is silently skipped — nobody gets the dm_channel_open, and (because it is
|
||||
// unsequenced) nothing ever redelivers it.
|
||||
func TestBroadcastDMOpen_SurvivesCancelledRequestContext(t *testing.T) {
|
||||
database := newDMTestDB(t)
|
||||
svc := service.New(database, auth.NewRateLimiter())
|
||||
broadcaster := &mockBroadcaster{}
|
||||
|
||||
// Fresh per-test DB, so creation order fixes the IDs: alice=1, bob=2, carol=3.
|
||||
_ = dmCreateToken(t, database, "ctx_alice", 4)
|
||||
_ = dmCreateToken(t, database, "ctx_bob", 4)
|
||||
_ = dmCreateToken(t, database, "ctx_carol", 4)
|
||||
|
||||
result, err := svc.DMs.CreateGroupDM(context.Background(), 1, []int64{2, 3}, "")
|
||||
if err != nil {
|
||||
t.Fatalf("CreateGroupDM: %v", err)
|
||||
}
|
||||
|
||||
// Simulate the request context being cancelled after the mutation has
|
||||
// committed but before the fan-out loop runs — e.g. the caller's
|
||||
// connection dropped mid-handler.
|
||||
cancelledCtx, cancel := context.WithCancel(context.Background())
|
||||
cancel()
|
||||
|
||||
api.BroadcastDMOpenForTest(cancelledCtx, svc, broadcaster, result.Channel.ID, result.ParticipantIDs)
|
||||
|
||||
if len(broadcaster.sent) != len(result.ParticipantIDs) {
|
||||
t.Fatalf("SendToUser calls = %d, want %d (one per participant): a cancelled request context must not stop the DM-open fan-out from reaching participants who are otherwise unaffected by the cancellation",
|
||||
len(broadcaster.sent), len(result.ParticipantIDs))
|
||||
}
|
||||
}
|
||||
@@ -255,11 +255,16 @@ func handleDeleteEmoji(svc *service.Services, store *storage.Storage, broadcaste
|
||||
// broadcastEmojiSet re-reads the set and pushes it to every client. A failure
|
||||
// here is logged and swallowed: the mutation itself already succeeded, and the
|
||||
// caller's own response carries the change.
|
||||
//
|
||||
// Called after the mutation has already committed, so the caller's request
|
||||
// context may be canceled by the time this runs (client aborted, deadline
|
||||
// fired) -- context.WithoutCancel detaches the re-read from that, matching
|
||||
// the pattern service/emoji.go already uses for its post-commit audit write.
|
||||
func broadcastEmojiSet(ctx context.Context, svc *service.Services, broadcaster EmojiBroadcaster) {
|
||||
if broadcaster == nil {
|
||||
return
|
||||
}
|
||||
list, err := svc.Emoji.List(ctx)
|
||||
list, err := svc.Emoji.List(context.WithoutCancel(ctx))
|
||||
if err != nil {
|
||||
slog.Error("failed to load emoji for broadcast", "error", err)
|
||||
return
|
||||
|
||||
@@ -537,6 +537,37 @@ func TestEmojiDelete_BadIDIs400(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// ─── broadcast fan-out must survive request cancellation ────────────────────
|
||||
|
||||
// TestBroadcastEmojiSet_SurvivesCanceledRequestContext pins OC-0139: both
|
||||
// handleCreateEmoji and handleDeleteEmoji call broadcastEmojiSet with
|
||||
// r.Context() AFTER the mutation has already committed. If the client aborts
|
||||
// (or the deadline fires) in that window, the re-read inside
|
||||
// broadcastEmojiSet must not ride the same now-canceled context, or the
|
||||
// fan-out silently never happens and every connected client keeps the stale
|
||||
// set even though the row is already gone (or already created).
|
||||
func TestBroadcastEmojiSet_SurvivesCanceledRequestContext(t *testing.T) {
|
||||
h := newEmojiHarness(t)
|
||||
// Seed a row directly, bypassing the upload handler (which broadcasts on
|
||||
// its own success), so this isolates broadcastEmojiSet itself.
|
||||
if _, err := h.database.CreateEmoji(context.Background(), "wave", "stored-1", "image/png", 1); err != nil {
|
||||
t.Fatalf("CreateEmoji: %v", err)
|
||||
}
|
||||
|
||||
svc := service.New(h.database, auth.NewRateLimiter())
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
cancel() // the request was already aborted by the time the commit lands
|
||||
|
||||
api.BroadcastEmojiSetForTest(ctx, svc, h.broadcaster)
|
||||
|
||||
if len(h.broadcaster.calls) != 1 {
|
||||
t.Fatalf("broadcast calls = %d, want 1 (fan-out must survive a canceled request context)", len(h.broadcaster.calls))
|
||||
}
|
||||
if got := h.broadcaster.calls[0]; len(got) != 1 || got[0].Shortcode != "wave" {
|
||||
t.Errorf("broadcast payload = %+v, want one :wave:", got)
|
||||
}
|
||||
}
|
||||
|
||||
// ─── WebP header parsing ─────────────────────────────────────────────────────
|
||||
|
||||
func TestWebPDimensions_AllChunkFlavours(t *testing.T) {
|
||||
|
||||
@@ -4,9 +4,15 @@ import (
|
||||
"context"
|
||||
"net/http"
|
||||
|
||||
"github.com/owncord/server/service"
|
||||
"github.com/owncord/server/ws"
|
||||
)
|
||||
|
||||
// BroadcastDMOpenForTest exposes broadcastDMOpen for external tests.
|
||||
func BroadcastDMOpenForTest(ctx context.Context, svc *service.Services, broadcaster DMBroadcaster, channelID int64, targetIDs []int64) {
|
||||
broadcastDMOpen(ctx, svc, broadcaster, channelID, targetIDs)
|
||||
}
|
||||
|
||||
// HandleMetricsForTest exposes handleMetrics for use in external tests.
|
||||
var HandleMetricsForTest = handleMetrics
|
||||
|
||||
@@ -54,6 +60,11 @@ var IsPrivateIPForTest = isPrivateIP
|
||||
// cases are tested directly rather than only through the upload handler.
|
||||
var WebPDimensionsForTest = webpDimensions
|
||||
|
||||
// BroadcastEmojiSetForTest exposes broadcastEmojiSet for external tests.
|
||||
func BroadcastEmojiSetForTest(ctx context.Context, svc *service.Services, broadcaster EmojiBroadcaster) {
|
||||
broadcastEmojiSet(ctx, svc, broadcaster)
|
||||
}
|
||||
|
||||
// SetGIFUpstreamForTest points the GIF proxy at a stub upstream and returns a
|
||||
// restore func. The production transport uses the SSRF-guarded dialer, which
|
||||
// refuses loopback addresses, so tests must supply their own client too.
|
||||
|
||||
@@ -289,7 +289,7 @@ func (q *Queries) IsGroupDM(ctx context.Context, id int64) (int64, error) {
|
||||
return is_group, err
|
||||
}
|
||||
|
||||
const openDM = `-- name: OpenDM :exec
|
||||
const openDM = `-- name: OpenDM :execrows
|
||||
INSERT OR IGNORE INTO dm_open_state (user_id, channel_id) VALUES (?, ?)
|
||||
`
|
||||
|
||||
@@ -298,9 +298,12 @@ type OpenDMParams struct {
|
||||
ChannelID int64 `json:"channelId"`
|
||||
}
|
||||
|
||||
func (q *Queries) OpenDM(ctx context.Context, arg OpenDMParams) error {
|
||||
_, err := q.db.ExecContext(ctx, openDM, arg.UserID, arg.ChannelID)
|
||||
return err
|
||||
func (q *Queries) OpenDM(ctx context.Context, arg OpenDMParams) (int64, error) {
|
||||
result, err := q.db.ExecContext(ctx, openDM, arg.UserID, arg.ChannelID)
|
||||
if err != nil {
|
||||
return 0, err
|
||||
}
|
||||
return result.RowsAffected()
|
||||
}
|
||||
|
||||
const removeDMParticipant = `-- name: RemoveDMParticipant :exec
|
||||
|
||||
@@ -187,7 +187,7 @@ type Querier interface {
|
||||
// (db.ConnectStatus). A stale choice never renders as "present" because the
|
||||
// read path treats a member with no live connection as offline regardless.
|
||||
MarkUserDisconnected(ctx context.Context, id int64) error
|
||||
OpenDM(ctx context.Context, arg OpenDMParams) error
|
||||
OpenDM(ctx context.Context, arg OpenDMParams) (int64, error)
|
||||
// seq is supplied by the hub so the row seq matches the wrapped-payload seq.
|
||||
PersistEvent(ctx context.Context, arg PersistEventParams) error
|
||||
PluginKVDelete(ctx context.Context, arg PluginKVDeleteParams) error
|
||||
|
||||
+10
-6
@@ -423,15 +423,19 @@ func (d *DB) GetDMParticipants(ctx context.Context, channelID, viewerID int64) (
|
||||
|
||||
// ─── OpenDM / CloseDM ──────────────────────────────────────────────────────
|
||||
|
||||
// OpenDM adds a DM channel to a user's open list (idempotent).
|
||||
func (d *DB) OpenDM(ctx context.Context, userID, channelID int64) error {
|
||||
if err := d.q.OpenDM(ctx, dbgen.OpenDMParams{
|
||||
// OpenDM adds a DM channel to a user's open list (idempotent). The bool
|
||||
// reports whether the DM was actually (re)opened by this call — false when it
|
||||
// was already open, via the INSERT OR IGNORE's affected-row count — so a
|
||||
// caller can distinguish a genuine open from a no-op on an already-open DM.
|
||||
func (d *DB) OpenDM(ctx context.Context, userID, channelID int64) (bool, error) {
|
||||
rows, err := d.q.OpenDM(ctx, dbgen.OpenDMParams{
|
||||
UserID: userID,
|
||||
ChannelID: channelID,
|
||||
}); err != nil {
|
||||
return fmt.Errorf("OpenDM: %w", err)
|
||||
})
|
||||
if err != nil {
|
||||
return false, fmt.Errorf("OpenDM: %w", err)
|
||||
}
|
||||
return nil
|
||||
return rows > 0, nil
|
||||
}
|
||||
|
||||
// CloseDM removes a DM channel from a user's open list.
|
||||
|
||||
@@ -442,9 +442,12 @@ func TestOpenDM_Idempotent(t *testing.T) {
|
||||
t.Fatalf("GetOrCreateDMChannel: %v", err)
|
||||
}
|
||||
|
||||
// Already open from creation — opening again should not error.
|
||||
if err := database.OpenDM(context.Background(), user1, ch.ID); err != nil {
|
||||
// Already open from creation — opening again should not error, and must
|
||||
// report false: the row already existed, so this call is a no-op.
|
||||
if opened, err := database.OpenDM(context.Background(), user1, ch.ID); err != nil {
|
||||
t.Errorf("OpenDM (idempotent) error: %v", err)
|
||||
} else if opened {
|
||||
t.Error("OpenDM (idempotent) reported opened=true for an already-open DM")
|
||||
}
|
||||
|
||||
// Should still have exactly 1 DM.
|
||||
@@ -513,8 +516,10 @@ func TestOpenDM_AfterClose(t *testing.T) {
|
||||
t.Fatalf("CloseDM: %v", err)
|
||||
}
|
||||
|
||||
if err := database.OpenDM(context.Background(), user1, ch.ID); err != nil {
|
||||
if opened, err := database.OpenDM(context.Background(), user1, ch.ID); err != nil {
|
||||
t.Fatalf("OpenDM after close: %v", err)
|
||||
} else if !opened {
|
||||
t.Error("OpenDM after close reported opened=false — a genuine reopen must report true")
|
||||
}
|
||||
|
||||
dms, err := database.GetUserDMChannels(context.Background(), user1)
|
||||
|
||||
@@ -184,13 +184,22 @@ const mentionCountChunkSize = 500
|
||||
// last_message_id stays 0 for a created row: the user has read nothing, and the
|
||||
// mention they were just given is unread by definition.
|
||||
//
|
||||
// msgID is the id of the message that triggered this fan-out. The increment
|
||||
// is skipped for a recipient whose read state already covers msgID
|
||||
// (last_message_id >= msgID): a mark_read that lands between the message
|
||||
// commit and this deferred call already zeroed their mention_count, and an
|
||||
// unconditional increment would leave a permanent phantom badge on a channel
|
||||
// with zero unread — nothing else ever zeroes it again. The guard is atomic
|
||||
// with the increment itself (part of the same ON CONFLICT ... DO UPDATE ...
|
||||
// WHERE), so there is no separate check-then-write race window.
|
||||
//
|
||||
// Batched into one multi-row INSERT per chunk of mentionCountChunkSize
|
||||
// recipients instead of one exec per recipient: an @everyone mention fans out
|
||||
// to every reader of a channel, and the writer txn used to pay one round trip
|
||||
// per reader for that. The caller has already excluded the author, so
|
||||
// semantics are unchanged — each listed user id still gets exactly one
|
||||
// increment (or a fresh row seeded at 1).
|
||||
func (d *DB) IncrementMentionCounts(ctx context.Context, channelID int64, userIDs []int64) error {
|
||||
// increment (or a fresh row seeded at 1), unless the guard above skips it.
|
||||
func (d *DB) IncrementMentionCounts(ctx context.Context, channelID, msgID int64, userIDs []int64) error {
|
||||
if len(userIDs) == 0 {
|
||||
return nil
|
||||
}
|
||||
@@ -209,12 +218,17 @@ func (d *DB) IncrementMentionCounts(ctx context.Context, channelID int64, userID
|
||||
rowPlaceholders[i] = "(?, ?, 0, 1)"
|
||||
args = append(args, uid, channelID)
|
||||
}
|
||||
// msgID is a bound parameter of the WHERE clause, not a VALUES column:
|
||||
// every conflicting row in this chunk shares the one message that
|
||||
// triggered the fan-out, so one trailing arg covers the whole batch.
|
||||
args = append(args, msgID)
|
||||
|
||||
query := fmt.Sprintf( //nolint:gosec // G201: placeholder interpolation, not user input
|
||||
`INSERT INTO read_states (user_id, channel_id, last_message_id, mention_count)
|
||||
VALUES %s
|
||||
ON CONFLICT(user_id, channel_id) DO UPDATE SET
|
||||
mention_count = mention_count + 1`,
|
||||
mention_count = mention_count + 1
|
||||
WHERE read_states.last_message_id < ?`,
|
||||
strings.Join(rowPlaceholders, ","),
|
||||
)
|
||||
if _, err := tx.ExecContext(ctx, query, args...); err != nil {
|
||||
|
||||
@@ -120,10 +120,10 @@ func TestIncrementMentionCounts_AndReadStateClear(t *testing.T) {
|
||||
seedMentionFixture(t, database)
|
||||
ctx := context.Background()
|
||||
|
||||
if err := database.IncrementMentionCounts(ctx, 1, []int64{2, 3}); err != nil {
|
||||
if err := database.IncrementMentionCounts(ctx, 1, 100, []int64{2, 3}); err != nil {
|
||||
t.Fatalf("IncrementMentionCounts: %v", err)
|
||||
}
|
||||
if err := database.IncrementMentionCounts(ctx, 1, []int64{2}); err != nil {
|
||||
if err := database.IncrementMentionCounts(ctx, 1, 100, []int64{2}); err != nil {
|
||||
t.Fatalf("IncrementMentionCounts: %v", err)
|
||||
}
|
||||
|
||||
@@ -169,7 +169,7 @@ func TestIncrementMentionCounts_BatchesAcrossChunkBoundary(t *testing.T) {
|
||||
ids = append(ids, uid)
|
||||
}
|
||||
|
||||
if err := database.IncrementMentionCounts(ctx, 1, ids); err != nil {
|
||||
if err := database.IncrementMentionCounts(ctx, 1, 1, ids); err != nil {
|
||||
t.Fatalf("IncrementMentionCounts: %v", err)
|
||||
}
|
||||
|
||||
@@ -180,7 +180,9 @@ func TestIncrementMentionCounts_BatchesAcrossChunkBoundary(t *testing.T) {
|
||||
}
|
||||
|
||||
// A second pass bumps every one of them again, chunk boundary included.
|
||||
if err := database.IncrementMentionCounts(ctx, 1, ids); err != nil {
|
||||
// msgID is higher than the first pass's so the read-state guard does not
|
||||
// treat this as the same already-seen message.
|
||||
if err := database.IncrementMentionCounts(ctx, 1, 2, ids); err != nil {
|
||||
t.Fatalf("IncrementMentionCounts (second pass): %v", err)
|
||||
}
|
||||
for _, uid := range []int64{ids[0], ids[499], ids[500], ids[n-1]} {
|
||||
@@ -193,11 +195,74 @@ func TestIncrementMentionCounts_BatchesAcrossChunkBoundary(t *testing.T) {
|
||||
func TestIncrementMentionCounts_EmptyIsNoop(t *testing.T) {
|
||||
database := newMigratedTestDB(t)
|
||||
seedMentionFixture(t, database)
|
||||
if err := database.IncrementMentionCounts(context.Background(), 1, nil); err != nil {
|
||||
if err := database.IncrementMentionCounts(context.Background(), 1, 1, nil); err != nil {
|
||||
t.Fatalf("IncrementMentionCounts(nil): %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// TestIncrementMentionCounts_NoOpWhenReaderAlreadyPastMessage locks OC-0066:
|
||||
// a mark_read that lands between the message commit and the deferred badge
|
||||
// increment must not leave a phantom mention_count behind. If the recipient's
|
||||
// read state already covers the mentioning message (last_message_id >=
|
||||
// msgID), the increment for that message must be a no-op — otherwise the
|
||||
// badge is stuck at 1 forever on a channel with zero unread, since nothing
|
||||
// else ever zeroes it again.
|
||||
func TestIncrementMentionCounts_NoOpWhenReaderAlreadyPastMessage(t *testing.T) {
|
||||
database := newMigratedTestDB(t)
|
||||
seedMentionFixture(t, database)
|
||||
ctx := context.Background()
|
||||
|
||||
msgID, err := database.CreateMessage(ctx, 1, 1, "hey @bob", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("CreateMessage: %v", err)
|
||||
}
|
||||
|
||||
// The reader (user 2 = bob) marks the channel read before the deferred
|
||||
// badge increment for this same message lands.
|
||||
if err := database.UpdateReadState(ctx, 2, 1, msgID); err != nil {
|
||||
t.Fatalf("UpdateReadState: %v", err)
|
||||
}
|
||||
|
||||
if err := database.IncrementMentionCounts(ctx, 1, msgID, []int64{2}); err != nil {
|
||||
t.Fatalf("IncrementMentionCounts: %v", err)
|
||||
}
|
||||
|
||||
if n, _ := database.GetMentionCount(ctx, 2, 1); n != 0 {
|
||||
t.Errorf("mention_count = %d after a read state that already covers the mentioning message, want 0 (phantom badge)", n)
|
||||
}
|
||||
}
|
||||
|
||||
// TestIncrementMentionCounts_StillAppliesWhenReaderIsBehind is the control:
|
||||
// the guard must not turn every increment into a no-op — a reader who has NOT
|
||||
// read up to msgID still gets the badge.
|
||||
func TestIncrementMentionCounts_StillAppliesWhenReaderIsBehind(t *testing.T) {
|
||||
database := newMigratedTestDB(t)
|
||||
seedMentionFixture(t, database)
|
||||
ctx := context.Background()
|
||||
|
||||
// Bob's read state is behind: he has read message 1's predecessor, not the
|
||||
// mention itself.
|
||||
older, err := database.CreateMessage(ctx, 1, 1, "hi", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("CreateMessage(older): %v", err)
|
||||
}
|
||||
if err := database.UpdateReadState(ctx, 2, 1, older); err != nil {
|
||||
t.Fatalf("UpdateReadState: %v", err)
|
||||
}
|
||||
|
||||
msgID, err := database.CreateMessage(ctx, 1, 1, "hey @bob", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("CreateMessage: %v", err)
|
||||
}
|
||||
if err := database.IncrementMentionCounts(ctx, 1, msgID, []int64{2}); err != nil {
|
||||
t.Fatalf("IncrementMentionCounts: %v", err)
|
||||
}
|
||||
|
||||
if n, _ := database.GetMentionCount(ctx, 2, 1); n != 1 {
|
||||
t.Errorf("mention_count = %d for a reader behind the mentioning message, want 1", n)
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetUserIDsByUsernames_CaseInsensitive(t *testing.T) {
|
||||
database := newMigratedTestDB(t)
|
||||
seedMentionFixture(t, database)
|
||||
|
||||
@@ -1258,7 +1258,7 @@ func TestGetChannelUnreadCounts_IncludesParticipatingDMs(t *testing.T) {
|
||||
t.Fatalf("GetOrCreateDMChannel: %v", err)
|
||||
}
|
||||
msgID, _ := database.CreateMessage(context.Background(), ch.ID, bob, "hey", nil)
|
||||
if err := database.IncrementMentionCounts(context.Background(), ch.ID, []int64{alice}); err != nil {
|
||||
if err := database.IncrementMentionCounts(context.Background(), ch.ID, msgID, []int64{alice}); err != nil {
|
||||
t.Fatalf("IncrementMentionCounts: %v", err)
|
||||
}
|
||||
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
-- name: OpenDM :exec
|
||||
-- name: OpenDM :execrows
|
||||
INSERT OR IGNORE INTO dm_open_state (user_id, channel_id) VALUES (?, ?);
|
||||
|
||||
-- name: CloseDM :exec
|
||||
|
||||
@@ -2,6 +2,8 @@ module github.com/owncord/server
|
||||
|
||||
go 1.26
|
||||
|
||||
toolchain go1.26.6
|
||||
|
||||
require (
|
||||
aead.dev/minisign v0.3.0
|
||||
github.com/BurntSushi/toml v1.6.0
|
||||
|
||||
+8
-1
@@ -197,6 +197,13 @@ func run(log *slog.Logger, logBuf *admin.RingBuffer, levelVar *slog.LevelVar) er
|
||||
// ── 5b. Build HTTP router ──────────────────────────────────────────────
|
||||
router, hub, routerCleanup := api.NewRouter(cfg, database, version, logBuf, pluginRegistry)
|
||||
defer routerCleanup()
|
||||
// Backstop for every early return below (serve error, ACME shutdown
|
||||
// failure, etc.): hub.GracefulStop is the only caller of
|
||||
// LiveKitProcess.Stop(), so skipping it orphans the companion
|
||||
// livekit-server process and leaves the hub's dispatch goroutine
|
||||
// running. gracefulOnce makes it idempotent alongside the explicit call
|
||||
// on the normal shutdown path below.
|
||||
defer hub.GracefulStop()
|
||||
|
||||
// ── 5c. Wire event persistence (Phase B Step 7) ────────────────────────
|
||||
if cfg.EventPersistence.Enabled && hub != nil {
|
||||
@@ -287,6 +294,7 @@ func run(log *slog.Logger, logBuf *admin.RingBuffer, levelVar *slog.LevelVar) er
|
||||
}
|
||||
|
||||
stopMaintenance := make(chan struct{})
|
||||
defer close(stopMaintenance) // backstop for early returns below; see hub.GracefulStop defer above
|
||||
go func() {
|
||||
ticker := time.NewTicker(15 * time.Minute)
|
||||
defer ticker.Stop()
|
||||
@@ -404,7 +412,6 @@ func run(log *slog.Logger, logBuf *admin.RingBuffer, levelVar *slog.LevelVar) er
|
||||
return fmt.Errorf("graceful shutdown: %w", err)
|
||||
}
|
||||
|
||||
close(stopMaintenance)
|
||||
log.Info("server stopped cleanly")
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -0,0 +1,48 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"io"
|
||||
"log/slog"
|
||||
"testing"
|
||||
|
||||
"go.uber.org/goleak"
|
||||
|
||||
"github.com/owncord/server/admin"
|
||||
)
|
||||
|
||||
// TestRun_ServeErrorReturn_StopsHubDispatchGoroutine pins OC-0027:
|
||||
// hub.GracefulStop() (the only caller of LiveKitProcess.Stop(), and what
|
||||
// closes the hub's dispatch goroutine) is a plain statement reached only on
|
||||
// the graceful-shutdown path. The serve-error branch — `case err :=
|
||||
// <-serveErr: ... return fmt.Errorf(...)` — returns from run() before ever
|
||||
// reaching it, so the hub's `go hub.Run()` dispatch goroutine (started by
|
||||
// api.NewRouter) is left running, and in production the companion
|
||||
// livekit-server process it owns is left running with it.
|
||||
//
|
||||
// An out-of-range port fails the first listen attempt with an error that
|
||||
// isAddrInUse does not recognize, so run() takes the servErr branch
|
||||
// immediately instead of retrying for ~10s.
|
||||
func TestRun_ServeErrorReturn_StopsHubDispatchGoroutine(t *testing.T) {
|
||||
t.Chdir(t.TempDir())
|
||||
|
||||
t.Setenv("OWNCORD_SERVER_PORT", "99999") // out of range: immediate, non-retryable listen error
|
||||
t.Setenv("OWNCORD_TLS_MODE", "off") // skip self-signed cert generation
|
||||
t.Setenv("OWNCORD_VOICE_AUTO_DOWNLOAD_LIVEKIT", "false") // the generated default config.yaml turns this on; keep the test offline
|
||||
|
||||
logBuf := admin.NewRingBuffer(64)
|
||||
levelVar := new(slog.LevelVar)
|
||||
log := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{Level: levelVar}))
|
||||
|
||||
leakOpt := goleak.IgnoreCurrent()
|
||||
|
||||
if err := run(log, logBuf, levelVar); err == nil {
|
||||
t.Fatal("expected run() to return an error for an out-of-range port")
|
||||
}
|
||||
|
||||
// hub.Run's dispatch goroutine only exits once hub.stop is closed, which
|
||||
// only happens inside hub.GracefulStop(). If run() returned without
|
||||
// calling it, this goroutine is still alive here.
|
||||
if err := goleak.Find(leakOpt); err != nil {
|
||||
t.Fatalf("hub dispatch goroutine (and, in production, its LiveKit process) leaked after run() returned early: %v", err)
|
||||
}
|
||||
}
|
||||
@@ -64,11 +64,12 @@ func (r *Registry) DispatchCommand(ctx context.Context, userID int64, channelID
|
||||
cmd = strings.ToLower(strings.TrimPrefix(cmd, "/"))
|
||||
r.mu.RLock()
|
||||
inst, ok := r.commands[cmd]
|
||||
platform := r.runtimePlatform
|
||||
r.mu.RUnlock()
|
||||
if !ok {
|
||||
return nil, false
|
||||
}
|
||||
if r.runtimePlatform == nil {
|
||||
if platform == nil {
|
||||
return &CommandResult{
|
||||
Reply: fmt.Sprintf("plugin %q owns /%s but the wazero runtime is not built (run with -tags wazero)", inst.Manifest.Name, cmd),
|
||||
}, true
|
||||
|
||||
@@ -0,0 +1,54 @@
|
||||
// OC-0088: DispatchCommand read r.runtimePlatform without the registry lock
|
||||
// that Close() writes it under, unlike the sibling reader activate() which
|
||||
// takes r.mu.RLock() first specifically to avoid observing a concurrent
|
||||
// Close mid-use. This file pins the fix with a concurrent -race test that
|
||||
// runs under every build variant (no wazero dependency: the default build's
|
||||
// runtimePlatform is always nil, but Close() still writes it under lock,
|
||||
// which is enough for the race detector to flag the unguarded read).
|
||||
package plugin
|
||||
|
||||
import (
|
||||
"context"
|
||||
"sync"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestDispatchCommandRuntimePlatformRace(t *testing.T) {
|
||||
mem := openPluginTestDB(t)
|
||||
reg, err := NewRegistry(Config{Directory: t.TempDir(), Store: mem})
|
||||
if err != nil {
|
||||
t.Fatalf("NewRegistry: %v", err)
|
||||
}
|
||||
|
||||
ctx := context.Background()
|
||||
manifest, err := ParseManifest([]byte(`{"name":"racer","version":"0.1.0","entrypoint":"p.wasm","permissions":["commands"],"commands":[{"name":"roll"}]}`))
|
||||
if err != nil {
|
||||
t.Fatalf("ParseManifest: %v", err)
|
||||
}
|
||||
if err := reg.installFromDisk(ctx, foundPlugin{Manifest: manifest, WASMPath: "p.wasm"}); err != nil {
|
||||
t.Fatalf("installFromDisk: %v", err)
|
||||
}
|
||||
reg.mu.RLock()
|
||||
inst := reg.byName["racer"]
|
||||
reg.mu.RUnlock()
|
||||
if err := reg.RegisterCommand("roll", inst); err != nil {
|
||||
t.Fatalf("RegisterCommand: %v", err)
|
||||
}
|
||||
|
||||
// Race DispatchCommand's unsynchronised read of r.runtimePlatform (line
|
||||
// 71 in host_commands.go) against Close's r.mu.Lock()-guarded write of
|
||||
// the same field. Before the fix, `go test -race` reports:
|
||||
// DATA RACE ... Registry.Close() ... Registry.DispatchCommand()
|
||||
var wg sync.WaitGroup
|
||||
for range 8 {
|
||||
wg.Go(func() {
|
||||
for range 500 {
|
||||
reg.DispatchCommand(ctx, 1, 2, "roll", nil)
|
||||
}
|
||||
})
|
||||
}
|
||||
wg.Go(func() {
|
||||
_ = reg.Close(ctx)
|
||||
})
|
||||
wg.Wait()
|
||||
}
|
||||
@@ -15,6 +15,7 @@ package plugin
|
||||
import (
|
||||
"archive/zip"
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"log/slog"
|
||||
@@ -59,6 +60,7 @@ type Registry struct {
|
||||
type Instance struct {
|
||||
ID int64
|
||||
Manifest *Manifest
|
||||
Dir string // on-disk plugin directory, from foundPlugin.Dir — NOT derived from Manifest.Name
|
||||
WASMPath string
|
||||
Enabled bool
|
||||
|
||||
@@ -210,6 +212,7 @@ func (r *Registry) installFromDisk(ctx context.Context, found foundPlugin) error
|
||||
inst := &Instance{
|
||||
ID: id,
|
||||
Manifest: found.Manifest,
|
||||
Dir: found.Dir,
|
||||
WASMPath: found.WASMPath,
|
||||
Enabled: false,
|
||||
}
|
||||
@@ -383,6 +386,42 @@ func (r *Registry) InstallFromZip(ctx context.Context, zipBytes []byte) (string,
|
||||
}); err != nil {
|
||||
return manifest.Name, fmt.Errorf("installFromDisk: %w", err)
|
||||
}
|
||||
|
||||
// installFromDisk always registers the fresh instance as disabled and
|
||||
// InstallPlugin's upsert never touches the `enabled` column, so a plugin
|
||||
// that was enabled before this upgrade would otherwise come out the
|
||||
// other side with the store row still saying enabled while the runtime
|
||||
// instance sits inactive. LoadAll's startup path avoids this because it
|
||||
// always runs activateAll afterward; this is the one caller of
|
||||
// installFromDisk that doesn't, so it has to reactivate for itself.
|
||||
// EnablePlugin already rolls the DB flag back if activation fails, so
|
||||
// the two can no longer disagree.
|
||||
if row, err := r.cfg.Store.GetPluginByName(ctx, manifest.Name); err == nil && row != nil && row.Enabled {
|
||||
if err := r.EnablePlugin(ctx, row.ID); err != nil {
|
||||
if errors.Is(err, ErrRuntimeUnavailable) {
|
||||
// Default (non-wazero) build: nothing can activate here, and
|
||||
// leaving EnablePlugin's rollback in place would persistently
|
||||
// disable a plugin the admin left enabled — after a rebuild
|
||||
// with -tags wazero it would silently stay off. Preserve the
|
||||
// enabled intent instead; the next wazero-tagged start's
|
||||
// activateAll does the real activation.
|
||||
if reErr := r.cfg.Store.EnablePlugin(ctx, row.ID); reErr != nil {
|
||||
slog.Warn("plugin: could not preserve enabled flag across runtime-less upgrade",
|
||||
"name", manifest.Name, "err", reErr)
|
||||
} else {
|
||||
r.mu.Lock()
|
||||
if inst, ok := r.byName[manifest.Name]; ok {
|
||||
inst.Enabled = true
|
||||
}
|
||||
r.mu.Unlock()
|
||||
slog.Info("plugin: runtime unavailable, enabled flag preserved across upgrade",
|
||||
"name", manifest.Name)
|
||||
}
|
||||
} else {
|
||||
slog.Warn("plugin: reactivate after upgrade failed", "name", manifest.Name, "err", err)
|
||||
}
|
||||
}
|
||||
}
|
||||
return manifest.Name, nil
|
||||
}
|
||||
|
||||
@@ -463,6 +502,10 @@ func (r *Registry) EnablePlugin(ctx context.Context, id int64) error {
|
||||
inst, ok := r.plugins[id]
|
||||
r.mu.RUnlock()
|
||||
if !ok {
|
||||
// No in-memory instance to activate — roll the DB flag back so it
|
||||
// doesn't stay stuck at enabled=1 with nothing backing it, the same
|
||||
// way the activation-failure path below rolls back.
|
||||
_ = r.cfg.Store.DisablePlugin(ctx, id)
|
||||
return ErrPluginNotFound
|
||||
}
|
||||
r.mu.Lock()
|
||||
@@ -505,6 +548,13 @@ func (r *Registry) DisablePlugin(ctx context.Context, id int64) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// removeAll is os.RemoveAll indirected so tests can force a directory-removal
|
||||
// failure deterministically. Windows silently succeeds at deleting read-only
|
||||
// files (there's no portable, privilege-free way to make a real RemoveAll
|
||||
// fail from a test), so this is the seam that lets the error-return path in
|
||||
// UninstallPlugin be pinned.
|
||||
var removeAll = os.RemoveAll
|
||||
|
||||
// UninstallPlugin removes a plugin entirely.
|
||||
func (r *Registry) UninstallPlugin(ctx context.Context, id int64) error {
|
||||
if err := r.DisablePlugin(ctx, id); err != nil {
|
||||
@@ -517,7 +567,7 @@ func (r *Registry) UninstallPlugin(ctx context.Context, id int64) error {
|
||||
inst, instOK := r.plugins[id]
|
||||
var pluginDir string
|
||||
if instOK {
|
||||
pluginDir = filepath.Join(r.cfg.Directory, inst.Manifest.Name)
|
||||
pluginDir = inst.Dir
|
||||
}
|
||||
r.mu.RUnlock()
|
||||
|
||||
@@ -533,10 +583,15 @@ func (r *Registry) UninstallPlugin(ctx context.Context, id int64) error {
|
||||
r.mu.Unlock()
|
||||
|
||||
// Remove on-disk files so the plugin isn't resurrected on the next
|
||||
// startup by scanPluginDirectory.
|
||||
// startup by scanPluginDirectory. The DB row and in-memory record are
|
||||
// already gone at this point, so a removal failure is reported rather
|
||||
// than swallowed — the caller needs to know the directory still has to
|
||||
// be cleaned up by hand before the next restart, or scanPluginDirectory
|
||||
// will bring the "uninstalled" plugin right back.
|
||||
if pluginDir != "" {
|
||||
if err := os.RemoveAll(pluginDir); err != nil {
|
||||
if err := removeAll(pluginDir); err != nil {
|
||||
slog.Warn("plugin: failed to remove plugin directory after uninstall", "dir", pluginDir, "err", err)
|
||||
return fmt.Errorf("remove plugin dir: %w", err)
|
||||
}
|
||||
}
|
||||
return nil
|
||||
|
||||
@@ -23,6 +23,7 @@ import (
|
||||
"bytes"
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
@@ -230,6 +231,44 @@ func TestRegistry_EnablePlugin_UnknownID(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0126: EnablePlugin sets the DB flag before it looks the in-memory
|
||||
// instance up. When the store row survives but the instance does not (the
|
||||
// on-disk manifest vanished without going through UninstallPlugin), the
|
||||
// early `!ok` return must roll the DB flag back the same way the
|
||||
// activation-failure path below it already does — otherwise the row is
|
||||
// permanently stuck at enabled=1 with no runtime instance to match it.
|
||||
func TestRegistry_EnablePlugin_RollsBackWhenInstanceMissing(t *testing.T) {
|
||||
r, store, dir := newRegistryWithDir(t)
|
||||
ctx := context.Background()
|
||||
writePluginDir(t, dir, "alpha", simpleManifest("alpha"))
|
||||
if err := r.LoadAll(ctx); err != nil {
|
||||
t.Fatalf("LoadAll: %v", err)
|
||||
}
|
||||
inst := r.List()[0]
|
||||
id := inst.ID
|
||||
|
||||
// Simulate the plugin's on-disk manifest disappearing independently of
|
||||
// UninstallPlugin (e.g. a manual directory delete): the store row
|
||||
// survives but the in-memory registration does not.
|
||||
r.mu.Lock()
|
||||
delete(r.plugins, id)
|
||||
delete(r.byName, inst.Manifest.Name)
|
||||
r.mu.Unlock()
|
||||
|
||||
err := r.EnablePlugin(ctx, id)
|
||||
if !errors.Is(err, ErrPluginNotFound) {
|
||||
t.Fatalf("EnablePlugin on a store-only row = %v, want ErrPluginNotFound", err)
|
||||
}
|
||||
|
||||
row, getErr := store.GetPlugin(ctx, id)
|
||||
if getErr != nil {
|
||||
t.Fatalf("GetPlugin: %v", getErr)
|
||||
}
|
||||
if row.Enabled {
|
||||
t.Error("store row stayed enabled after EnablePlugin found no in-memory instance; the rollback did not run")
|
||||
}
|
||||
}
|
||||
|
||||
// ─── DisablePlugin ──────────────────────────────────────────────────────────
|
||||
|
||||
func TestRegistry_DisablePlugin_ClearsFlagAndCommands(t *testing.T) {
|
||||
@@ -318,6 +357,102 @@ func TestRegistry_UninstallPlugin_RemovesRowAndDirectory(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0127: when the on-disk directory can't be removed, UninstallPlugin must
|
||||
// report that instead of returning nil — the comment right above the removal
|
||||
// says the removal is what stops scanPluginDirectory resurrecting the plugin
|
||||
// on the next startup, so silently swallowing the error produces exactly the
|
||||
// outcome the removal exists to prevent.
|
||||
func TestRegistry_UninstallPlugin_ReturnsErrorWhenDirRemovalFails(t *testing.T) {
|
||||
r, store, dir := newRegistryWithDir(t)
|
||||
ctx := context.Background()
|
||||
pluginDir := writePluginDir(t, dir, "alpha", simpleManifest("alpha"))
|
||||
if err := r.LoadAll(ctx); err != nil {
|
||||
t.Fatalf("LoadAll: %v", err)
|
||||
}
|
||||
inst := r.List()[0]
|
||||
|
||||
// Force the directory removal to fail. There's no portable,
|
||||
// privilege-free way to make a real os.RemoveAll fail on Windows (it
|
||||
// happily deletes read-only files), so the removeAll seam is overridden
|
||||
// directly.
|
||||
origRemoveAll := removeAll
|
||||
removeAll = func(string) error { return fmt.Errorf("simulated removal failure") }
|
||||
t.Cleanup(func() { removeAll = origRemoveAll })
|
||||
|
||||
err := r.UninstallPlugin(ctx, inst.ID)
|
||||
if err == nil {
|
||||
t.Fatal("UninstallPlugin succeeded despite a directory-removal failure; want an error")
|
||||
}
|
||||
|
||||
if _, statErr := os.Stat(pluginDir); statErr != nil {
|
||||
t.Errorf("plugin directory was actually removed despite the reported error (stat err = %v)", statErr)
|
||||
}
|
||||
|
||||
// The DB row and in-memory record are gone regardless — only the on-disk
|
||||
// cleanup failed, and that failure must be visible to the caller.
|
||||
rows, lErr := store.ListPlugins(ctx)
|
||||
if lErr != nil {
|
||||
t.Fatalf("ListPlugins: %v", lErr)
|
||||
}
|
||||
if len(rows) != 0 {
|
||||
t.Errorf("store row survived a failed uninstall; want it removed regardless of directory cleanup")
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0133: UninstallPlugin must remove the plugin's real on-disk directory,
|
||||
// not one rebuilt from manifest.Name. scanPluginDirectory keys the directory
|
||||
// off the filesystem entry name, so a plugin dropped in a folder whose name
|
||||
// differs from its manifest name (e.g. a hand-installed "hello-v2" holding
|
||||
// manifest name "hello") is otherwise resurrected by the next LoadAll: the
|
||||
// wrong path 404s os.RemoveAll into a silent no-op.
|
||||
func TestRegistry_UninstallPlugin_DirNameDiffersFromManifestName(t *testing.T) {
|
||||
r, store, dir := newRegistryWithDir(t)
|
||||
ctx := context.Background()
|
||||
|
||||
actualDir := filepath.Join(dir, "hello-v2")
|
||||
if err := os.MkdirAll(actualDir, 0o750); err != nil {
|
||||
t.Fatalf("mkdir: %v", err)
|
||||
}
|
||||
manifestJSON := `{"name":"hello","version":"1.0.0","entrypoint":"hello.wasm","permissions":["storage"]}`
|
||||
if err := os.WriteFile(filepath.Join(actualDir, "plugin.json"), []byte(manifestJSON), 0o600); err != nil {
|
||||
t.Fatalf("write manifest: %v", err)
|
||||
}
|
||||
if err := os.WriteFile(filepath.Join(actualDir, "hello.wasm"), []byte("\x00asm\x01\x00\x00\x00"), 0o600); err != nil {
|
||||
t.Fatalf("write wasm: %v", err)
|
||||
}
|
||||
|
||||
if err := r.LoadAll(ctx); err != nil {
|
||||
t.Fatalf("LoadAll: %v", err)
|
||||
}
|
||||
inst := r.List()[0]
|
||||
if inst.Manifest.Name != "hello" {
|
||||
t.Fatalf("test setup: manifest name = %q, want hello", inst.Manifest.Name)
|
||||
}
|
||||
|
||||
if err := r.UninstallPlugin(ctx, inst.ID); err != nil {
|
||||
t.Fatalf("UninstallPlugin: %v", err)
|
||||
}
|
||||
|
||||
if _, statErr := os.Stat(actualDir); !os.IsNotExist(statErr) {
|
||||
t.Errorf("actual plugin directory %q survived uninstall (stat err = %v)", actualDir, statErr)
|
||||
}
|
||||
|
||||
// The real bug symptom: a stale directory makes the plugin come back.
|
||||
if err := r.LoadAll(ctx); err != nil {
|
||||
t.Fatalf("LoadAll after uninstall: %v", err)
|
||||
}
|
||||
if got := r.List(); len(got) != 0 {
|
||||
t.Errorf("uninstalled plugin was resurrected by LoadAll: %d entries", len(got))
|
||||
}
|
||||
rows, err := store.ListPlugins(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("ListPlugins: %v", err)
|
||||
}
|
||||
if len(rows) != 0 {
|
||||
t.Errorf("store still has %d rows after uninstall+reload, want 0", len(rows))
|
||||
}
|
||||
}
|
||||
|
||||
func TestRegistry_UninstallPlugin_UnknownID(t *testing.T) {
|
||||
r, _, _ := newRegistryWithDir(t)
|
||||
|
||||
@@ -419,6 +554,51 @@ func TestRegistry_InstallFromZip_Success(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0104: InstallFromZip is the hot-upgrade path (unlike LoadAll, nothing
|
||||
// downstream of it calls activateAll). installFromDisk always registers the
|
||||
// fresh Instance with Enabled: false and InstallPlugin's upsert never touches
|
||||
// the `enabled` column, so a plugin that was enabled before the upgrade must
|
||||
// come out the other side with its store row and its runtime instance still
|
||||
// agreeing — otherwise the DB says enabled while the module is actually
|
||||
// unloaded, and the admin panel has no reason to prompt a re-enable.
|
||||
func TestRegistry_InstallFromZip_ReactivatesPreviouslyEnabledPlugin(t *testing.T) {
|
||||
r, store, dir := newRegistryWithDir(t)
|
||||
ctx := context.Background()
|
||||
writePluginDir(t, dir, "alpha", simpleManifest("alpha"))
|
||||
if err := r.LoadAll(ctx); err != nil {
|
||||
t.Fatalf("LoadAll: %v", err)
|
||||
}
|
||||
inst := r.List()[0]
|
||||
|
||||
// Simulate a previously-activated, enabled plugin — what activateAll
|
||||
// leaves behind under a real (wazero-tagged) runtime: DB row enabled and
|
||||
// the in-memory flag set to match.
|
||||
if err := store.EnablePlugin(ctx, inst.ID); err != nil {
|
||||
t.Fatalf("EnablePlugin (setup): %v", err)
|
||||
}
|
||||
r.mu.Lock()
|
||||
inst.Enabled = true
|
||||
r.mu.Unlock()
|
||||
|
||||
// Upload a new version of the same plugin — the runtime-upgrade path.
|
||||
zipBytes := buildZip(t, map[string]string{
|
||||
"plugin.json": simpleManifest("alpha"),
|
||||
"alpha.wasm": "\x00asm\x01\x00\x00\x00",
|
||||
})
|
||||
if _, err := r.InstallFromZip(ctx, zipBytes); err != nil {
|
||||
t.Fatalf("InstallFromZip: %v", err)
|
||||
}
|
||||
|
||||
newInst := r.List()[0]
|
||||
row, err := store.GetPlugin(ctx, newInst.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetPlugin: %v", err)
|
||||
}
|
||||
if row.Enabled != newInst.Enabled {
|
||||
t.Errorf("store row Enabled=%v but runtime instance Enabled=%v after InstallFromZip; a hot upgrade must never leave these disagreeing", row.Enabled, newInst.Enabled)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRegistry_InstallFromZip_Rejections(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
|
||||
@@ -0,0 +1,52 @@
|
||||
//go:build !wazero
|
||||
|
||||
package plugin
|
||||
|
||||
import (
|
||||
"context"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// On a default (non-wazero) build nothing can ever activate, so the
|
||||
// reactivate-after-upgrade path must not let EnablePlugin's activation-failure
|
||||
// rollback persistently flip an enabled plugin's store row to disabled: the
|
||||
// admin's enabled intent has to survive the upgrade so a later restart under a
|
||||
// wazero-tagged build brings the plugin back up, exactly as activateAll would
|
||||
// have at startup.
|
||||
func TestRegistry_InstallFromZip_RuntimeUnavailable_PreservesEnabledFlag(t *testing.T) {
|
||||
r, store, dir := newRegistryWithDir(t)
|
||||
ctx := context.Background()
|
||||
writePluginDir(t, dir, "alpha", simpleManifest("alpha"))
|
||||
if err := r.LoadAll(ctx); err != nil {
|
||||
t.Fatalf("LoadAll: %v", err)
|
||||
}
|
||||
inst := r.List()[0]
|
||||
|
||||
if err := store.EnablePlugin(ctx, inst.ID); err != nil {
|
||||
t.Fatalf("EnablePlugin (setup): %v", err)
|
||||
}
|
||||
r.mu.Lock()
|
||||
inst.Enabled = true
|
||||
r.mu.Unlock()
|
||||
|
||||
zipBytes := buildZip(t, map[string]string{
|
||||
"plugin.json": simpleManifest("alpha"),
|
||||
"alpha.wasm": "\x00asm\x01\x00\x00\x00",
|
||||
})
|
||||
if _, err := r.InstallFromZip(ctx, zipBytes); err != nil {
|
||||
t.Fatalf("InstallFromZip: %v", err)
|
||||
}
|
||||
|
||||
newInst := r.List()[0]
|
||||
row, err := store.GetPlugin(ctx, newInst.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetPlugin: %v", err)
|
||||
}
|
||||
if !row.Enabled {
|
||||
t.Error("store row flipped to disabled by an upgrade on a runtime-less build — " +
|
||||
"the enabled intent must survive until a wazero-tagged start can actually activate")
|
||||
}
|
||||
if !newInst.Enabled {
|
||||
t.Error("in-memory instance must carry the preserved enabled flag so admin listings stay truthful")
|
||||
}
|
||||
}
|
||||
@@ -42,7 +42,7 @@ type Store interface {
|
||||
// ── Mentions ──
|
||||
ReplaceMessageMentions(ctx context.Context, messageID int64, mentionedUserIDs []int64, mentionsEveryone bool) error
|
||||
GetMentionsByMessageIDs(ctx context.Context, msgIDs []int64) (map[int64][]int64, error)
|
||||
IncrementMentionCounts(ctx context.Context, channelID int64, userIDs []int64) error
|
||||
IncrementMentionCounts(ctx context.Context, channelID, msgID int64, userIDs []int64) error
|
||||
GetUserIDsByUsernames(ctx context.Context, usernames []string) (map[string]int64, error)
|
||||
ListMentionTargetsByRoles(ctx context.Context, roleIDs []int64) ([]db.MentionTarget, error)
|
||||
ListBlockersOf(ctx context.Context, blockedID int64) ([]int64, error)
|
||||
@@ -154,7 +154,7 @@ type Store interface {
|
||||
GetOrCreateDMChannel(ctx context.Context, user1ID, user2ID int64) (*db.Channel, bool, error)
|
||||
GetUserDMChannels(ctx context.Context, userID int64) ([]db.DMChannelInfo, error)
|
||||
GetUserDMChannelIDs(ctx context.Context, userID int64) ([]int64, error)
|
||||
OpenDM(ctx context.Context, userID, channelID int64) error
|
||||
OpenDM(ctx context.Context, userID, channelID int64) (bool, error)
|
||||
CloseDM(ctx context.Context, userID, channelID int64) error
|
||||
IsDMParticipant(ctx context.Context, userID, channelID int64) (bool, error)
|
||||
GetDMParticipantIDs(ctx context.Context, channelID int64) ([]int64, error)
|
||||
|
||||
@@ -104,7 +104,7 @@ func TestHandleChannelFocus_ClearsMentionBadgeWhenAllMessagesDeleted(t *testing.
|
||||
if err != nil {
|
||||
t.Fatalf("CreateMessage: %v", err)
|
||||
}
|
||||
if err := database.IncrementMentionCounts(ctx, 10, []int64{1}); err != nil {
|
||||
if err := database.IncrementMentionCounts(ctx, 10, msgID, []int64{1}); err != nil {
|
||||
t.Fatalf("IncrementMentionCounts: %v", err)
|
||||
}
|
||||
if err := database.DeleteMessage(ctx, msgID, 2, false); err != nil {
|
||||
|
||||
@@ -152,7 +152,7 @@ func (s *MessageService) resolveMentions(ctx context.Context, content string, au
|
||||
//
|
||||
// The message is already committed by the time this runs, so failures are
|
||||
// logged rather than surfaced — a lost badge must not fail a delivered send.
|
||||
func (s *MessageService) applyMentionCounts(ctx context.Context, channelID, authorID int64, set mentionSet, isDM bool, participantIDs []int64) {
|
||||
func (s *MessageService) applyMentionCounts(ctx context.Context, channelID, msgID, authorID int64, set mentionSet, isDM bool, participantIDs []int64) {
|
||||
if len(set.UserIDs) == 0 && !set.Everyone {
|
||||
return
|
||||
}
|
||||
@@ -212,7 +212,7 @@ func (s *MessageService) applyMentionCounts(ctx context.Context, channelID, auth
|
||||
for id := range recipients {
|
||||
ids = append(ids, id)
|
||||
}
|
||||
if err := s.st.IncrementMentionCounts(ctx, channelID, ids); err != nil {
|
||||
if err := s.st.IncrementMentionCounts(ctx, channelID, msgID, ids); err != nil {
|
||||
slog.Error("MessageService.applyMentionCounts IncrementMentionCounts", "err", err, "channel_id", channelID)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -171,26 +171,36 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) (
|
||||
|
||||
// DM path: open DM for recipients.
|
||||
if isDM {
|
||||
participantIDs, pErr := s.st.GetDMParticipantIDs(ctx, p.ChannelID)
|
||||
// The message is already committed, so everything below must survive
|
||||
// the sender's connection dropping the instant the write commits — the
|
||||
// same reason the compensating deletes and applyMentionCounts below
|
||||
// detach from ctx. Without WithoutCancel, a canceled request ctx here
|
||||
// silently drops every recipient from the fan-out (ParticipantIDs
|
||||
// stays nil), skips re-opening the recipient's dm_open_state, and
|
||||
// degrades the payload shape — with no error surfaced to anyone: the
|
||||
// sender sees chat_send_ok and the other participant never gets the
|
||||
// message live.
|
||||
bgCtx := context.WithoutCancel(ctx)
|
||||
participantIDs, pErr := s.st.GetDMParticipantIDs(bgCtx, p.ChannelID)
|
||||
if pErr != nil {
|
||||
slog.Error("MessageService.SendMessage GetDMParticipantIDs", "err", pErr, "channel_id", p.ChannelID)
|
||||
return result, nil // Message saved, skip DM side effects.
|
||||
}
|
||||
result.ParticipantIDs = participantIDs
|
||||
|
||||
sender, _ := s.st.GetUserByID(ctx, p.UserID)
|
||||
sender, _ := s.st.GetUserByID(bgCtx, p.UserID)
|
||||
result.SenderUser = sender
|
||||
|
||||
// Viewer-neutral (viewerID 0 matches nobody, so every status is
|
||||
// broadcast-collapsed); the ws layer re-derives "who is the recipient"
|
||||
// per addressee. A read failure is non-fatal — the message is already
|
||||
// committed, and the caller falls back to the 1:1 shape.
|
||||
if participants, partErr := s.st.GetDMParticipants(ctx, p.ChannelID, 0); partErr == nil {
|
||||
if participants, partErr := s.st.GetDMParticipants(bgCtx, p.ChannelID, 0); partErr == nil {
|
||||
result.DMParticipants = participants
|
||||
} else {
|
||||
slog.Warn("MessageService.SendMessage GetDMParticipants", "err", partErr, "channel_id", p.ChannelID)
|
||||
}
|
||||
if isGroup, gErr := s.st.IsGroupDM(ctx, p.ChannelID); gErr == nil {
|
||||
if isGroup, gErr := s.st.IsGroupDM(bgCtx, p.ChannelID); gErr == nil {
|
||||
result.DMIsGroup = isGroup
|
||||
}
|
||||
|
||||
@@ -198,11 +208,21 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) (
|
||||
if pid == p.UserID {
|
||||
continue
|
||||
}
|
||||
if openErr := s.st.OpenDM(ctx, pid, p.ChannelID); openErr != nil {
|
||||
// OpenDM is INSERT OR IGNORE and idempotent: opened reports whether
|
||||
// this call actually inserted the row. Only a genuine (re)open goes
|
||||
// into OpenedDMFor — the ws layer emits a dm_channel_open per id in
|
||||
// that slice, and each one bumps the hub's global visibility
|
||||
// watermark, forcing every other connected client's next reconnect
|
||||
// onto a full resync. An already-open DM must not pay that cost on
|
||||
// every single message.
|
||||
opened, openErr := s.st.OpenDM(bgCtx, pid, p.ChannelID)
|
||||
if openErr != nil {
|
||||
slog.Error("MessageService.SendMessage OpenDM", "err", openErr, "recipient_id", pid, "channel_id", p.ChannelID)
|
||||
continue
|
||||
}
|
||||
result.OpenedDMFor = append(result.OpenedDMFor, pid)
|
||||
if opened {
|
||||
result.OpenedDMFor = append(result.OpenedDMFor, pid)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -211,12 +231,13 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) (
|
||||
// the batched increment) must not delay delivering the message to the rest
|
||||
// of the channel. The ctx is detached from cancellation — for the same
|
||||
// reason audit writes are — so a client hanging up mid-request cannot drop
|
||||
// the badges. The count is advisory: if a reader's channel_focus clears it
|
||||
// in the tiny window before the increment lands, the badge simply does not
|
||||
// reappear, which matches Discord's eventual-consistency behaviour.
|
||||
// the badges. If a reader's channel_focus clears the badge in the tiny
|
||||
// window before the increment lands, IncrementMentionCounts' own read-state
|
||||
// guard (msgID vs. last_message_id) makes the increment a no-op instead of
|
||||
// resurrecting it — the badge does not reappear.
|
||||
channelID, authorID, participantIDs := p.ChannelID, p.UserID, result.ParticipantIDs
|
||||
s.bg(func() {
|
||||
s.applyMentionCounts(context.WithoutCancel(ctx), channelID, authorID, mentions, isDM, participantIDs)
|
||||
s.applyMentionCounts(context.WithoutCancel(ctx), channelID, msgID, authorID, mentions, isDM, participantIDs)
|
||||
})
|
||||
|
||||
slog.Debug("message sent", "user", p.Username, "channel_id", p.ChannelID, "msg_id", msgID)
|
||||
@@ -249,12 +270,16 @@ func (s *MessageService) EditMessage(ctx context.Context, userID, msgID int64, r
|
||||
return nil, fmt.Errorf("%w: cannot edit this message", ErrDeletedMessage)
|
||||
}
|
||||
|
||||
// Channel type for DM-aware permissions.
|
||||
// Channel type for DM-aware permissions. Fail closed: a lookup failure must
|
||||
// not fall through with chanType="", which routes into the non-DM
|
||||
// permission branch below. That branch passes on the base role mask alone
|
||||
// (SEND_MESSAGES|READ_MESSAGES, no per-channel override exists for a DM),
|
||||
// skipping both the DM-participant check and requireDMNotBlocked entirely.
|
||||
ch, chErr := s.st.GetChannel(ctx, msg.ChannelID)
|
||||
chanType := ""
|
||||
if chErr == nil && ch != nil {
|
||||
chanType = ch.Type
|
||||
if chErr != nil || ch == nil {
|
||||
return nil, fmt.Errorf("%w: cannot edit this message", ErrForbidden)
|
||||
}
|
||||
chanType := ch.Type
|
||||
isDM := chanType == "dm"
|
||||
|
||||
if isDM {
|
||||
@@ -313,7 +338,10 @@ func (s *MessageService) EditMessage(ctx context.Context, userID, msgID int64, r
|
||||
}
|
||||
|
||||
if isDM {
|
||||
participantIDs, pErr := s.st.GetDMParticipantIDs(ctx, msg.ChannelID)
|
||||
// Detached from ctx for the same reason as the SendMessage post-commit
|
||||
// lookup: the edit already committed, so an editor whose connection
|
||||
// drops right after must not silently drop the chat_edited fan-out.
|
||||
participantIDs, pErr := s.st.GetDMParticipantIDs(context.WithoutCancel(ctx), msg.ChannelID)
|
||||
if pErr != nil {
|
||||
slog.Error("MessageService.EditMessage GetDMParticipantIDs", "err", pErr, "channel_id", msg.ChannelID)
|
||||
} else {
|
||||
@@ -342,8 +370,21 @@ func (s *MessageService) DeleteMessage(ctx context.Context, userID, msgID int64)
|
||||
return nil, fmt.Errorf("%w: cannot delete this message", ErrForbidden)
|
||||
}
|
||||
|
||||
// Fail closed, mirroring EditMessage: a lookup failure must not fall
|
||||
// through to the non-DM permission branch (skipping the DM-participant
|
||||
// check) or past the archived gate below.
|
||||
ch, chErr := s.st.GetChannel(ctx, msg.ChannelID)
|
||||
isDM := chErr == nil && ch != nil && ch.Type == "dm"
|
||||
if chErr != nil || ch == nil {
|
||||
return nil, fmt.Errorf("%w: cannot delete this message", ErrForbidden)
|
||||
}
|
||||
isDM := ch.Type == "dm"
|
||||
|
||||
// Archived channels are read-only, mirroring SendMessage's gate
|
||||
// (message_crud.go:54): history stays visible, but a member or moderator
|
||||
// must not be able to mutate it by deleting a message out of the archive.
|
||||
if !isDM && ch.Archived {
|
||||
return nil, fmt.Errorf("%w: channel is archived", ErrForbidden)
|
||||
}
|
||||
|
||||
var isMod bool
|
||||
if isDM {
|
||||
@@ -385,7 +426,10 @@ func (s *MessageService) DeleteMessage(ctx context.Context, userID, msgID int64)
|
||||
}
|
||||
|
||||
if isDM {
|
||||
participantIDs, pErr := s.st.GetDMParticipantIDs(ctx, msg.ChannelID)
|
||||
// Detached from ctx for the same reason as the send/edit paths: the
|
||||
// soft-delete already committed, so a deleter whose connection drops
|
||||
// right after must not silently drop the chat_deleted fan-out.
|
||||
participantIDs, pErr := s.st.GetDMParticipantIDs(context.WithoutCancel(ctx), msg.ChannelID)
|
||||
if pErr != nil {
|
||||
slog.Error("MessageService.DeleteMessage GetDMParticipantIDs", "err", pErr, "channel_id", msg.ChannelID)
|
||||
} else {
|
||||
|
||||
@@ -0,0 +1,308 @@
|
||||
package service
|
||||
|
||||
// Tests for the 2026-08-12/13 message_crud.go findings: the post-commit
|
||||
// GetDMParticipantIDs fan-out gap on send/edit/delete (OC-0033, OC-0067,
|
||||
// OC-0068), EditMessage's fail-open channel lookup (OC-0074), DeleteMessage's
|
||||
// missing archived-channel gate (OC-0077), and SendMessage's redundant
|
||||
// dm_channel_open re-opens (OC-0106).
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"testing"
|
||||
|
||||
"github.com/owncord/server/db"
|
||||
"github.com/owncord/server/permissions"
|
||||
)
|
||||
|
||||
// disconnectAfterWriteStore models a client whose connection drops the instant
|
||||
// its write commits — the request ctx gets canceled right after the wrapped
|
||||
// write call returns, before the post-commit DM participant lookup runs.
|
||||
// GetDMParticipantIDs is overridden to fail whenever it is handed an
|
||||
// already-canceled context, so a test can tell whether the caller used the
|
||||
// (canceled) request ctx or a detached one for that lookup.
|
||||
type disconnectAfterWriteStore struct {
|
||||
Store
|
||||
cancel context.CancelFunc
|
||||
}
|
||||
|
||||
func (s disconnectAfterWriteStore) CreateMessageWithMentions(ctx context.Context, channelID, userID int64, content string, replyTo *int64, mentionedUserIDs []int64, mentionsEveryone bool) (*db.Message, error) {
|
||||
msg, err := s.Store.CreateMessageWithMentions(ctx, channelID, userID, content, replyTo, mentionedUserIDs, mentionsEveryone)
|
||||
s.cancel()
|
||||
return msg, err
|
||||
}
|
||||
|
||||
func (s disconnectAfterWriteStore) EditMessage(ctx context.Context, id, userID int64, content string) (*db.Message, error) {
|
||||
msg, err := s.Store.EditMessage(ctx, id, userID, content)
|
||||
s.cancel()
|
||||
return msg, err
|
||||
}
|
||||
|
||||
func (s disconnectAfterWriteStore) DeleteMessage(ctx context.Context, id, userID int64, isMod bool) error {
|
||||
err := s.Store.DeleteMessage(ctx, id, userID, isMod)
|
||||
s.cancel()
|
||||
return err
|
||||
}
|
||||
|
||||
func (s disconnectAfterWriteStore) GetDMParticipantIDs(ctx context.Context, channelID int64) ([]int64, error) {
|
||||
if ctx.Err() != nil {
|
||||
return nil, ctx.Err()
|
||||
}
|
||||
return s.Store.GetDMParticipantIDs(ctx, channelID)
|
||||
}
|
||||
|
||||
// newDMFixture seeds a two-person DM channel (alice=1, bob=2) and returns the
|
||||
// permission service so callers can build plain and wrapped MessageServices
|
||||
// against the same underlying database.
|
||||
func newDMFixture(t *testing.T) (*db.DB, *PermissionService) {
|
||||
t.Helper()
|
||||
database := newTestDB(t)
|
||||
seedRole(t, database, &db.Role{
|
||||
ID: permissions.MemberRoleID,
|
||||
Name: "member",
|
||||
Permissions: permissions.SendMessages | permissions.ReadMessages,
|
||||
Position: 1,
|
||||
})
|
||||
seedUser(t, database, &db.User{ID: 1, Username: "alice"})
|
||||
seedUser(t, database, &db.User{ID: 2, Username: "bob"})
|
||||
seedUserRole(t, database, 1, permissions.MemberRoleID)
|
||||
seedUserRole(t, database, 2, permissions.MemberRoleID)
|
||||
seedChannel(t, database, &db.Channel{ID: 50, Name: "dm-1-2", Type: "dm"})
|
||||
seedDMParticipant(t, database, 50, 1)
|
||||
seedDMParticipant(t, database, 50, 2)
|
||||
return database, NewPermissionService(database, permissions.NewChecker(database))
|
||||
}
|
||||
|
||||
// OC-0033: a sender whose connection drops right after their DM message
|
||||
// commits must not silently drop live fan-out to the other participant(s).
|
||||
func TestSendMessage_DMFanoutSurvivesSenderDisconnectAfterCommit(t *testing.T) {
|
||||
database, permSvc := newDMFixture(t)
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
svc := NewMessageService(disconnectAfterWriteStore{Store: database, cancel: cancel}, permSvc, nil)
|
||||
|
||||
result, err := svc.SendMessage(ctx, SendMessageParams{
|
||||
ChannelID: 50, UserID: 1, Username: "alice", Content: "hi bob",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("SendMessage: %v", err)
|
||||
}
|
||||
if len(result.ParticipantIDs) == 0 {
|
||||
t.Fatal("ParticipantIDs empty after a sender disconnect that raced the post-commit DM lookup — " +
|
||||
"the recipient gets no live chat_message fan-out until their next reconnect")
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0067: same gap on the edit path — a chat_edited must still reach the
|
||||
// other participant even when the editor's connection drops after the DB
|
||||
// write commits.
|
||||
func TestEditMessage_DMFanoutSurvivesEditorDisconnectAfterCommit(t *testing.T) {
|
||||
database, permSvc := newDMFixture(t)
|
||||
plainSvc := NewMessageService(database, permSvc, nil)
|
||||
|
||||
sent, err := plainSvc.SendMessage(context.Background(), SendMessageParams{
|
||||
ChannelID: 50, UserID: 1, Username: "alice", Content: "original",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("send: %v", err)
|
||||
}
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
svc := NewMessageService(disconnectAfterWriteStore{Store: database, cancel: cancel}, permSvc, nil)
|
||||
|
||||
editResult, err := svc.EditMessage(ctx, 1, sent.MessageID, "edited content")
|
||||
if err != nil {
|
||||
t.Fatalf("EditMessage: %v", err)
|
||||
}
|
||||
if len(editResult.ParticipantIDs) == 0 {
|
||||
t.Fatal("ParticipantIDs empty after an editor disconnect that raced the post-commit DM lookup — " +
|
||||
"the other participant never sees the edit live")
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0068: same gap on the delete path — a chat_deleted must still reach the
|
||||
// other participant even when the deleter's connection drops after the
|
||||
// soft-delete commits.
|
||||
func TestDeleteMessage_DMFanoutSurvivesDeleterDisconnectAfterCommit(t *testing.T) {
|
||||
database, permSvc := newDMFixture(t)
|
||||
plainSvc := NewMessageService(database, permSvc, nil)
|
||||
|
||||
sent, err := plainSvc.SendMessage(context.Background(), SendMessageParams{
|
||||
ChannelID: 50, UserID: 1, Username: "alice", Content: "delete me",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("send: %v", err)
|
||||
}
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
svc := NewMessageService(disconnectAfterWriteStore{Store: database, cancel: cancel}, permSvc, nil)
|
||||
|
||||
delResult, err := svc.DeleteMessage(ctx, 1, sent.MessageID)
|
||||
if err != nil {
|
||||
t.Fatalf("DeleteMessage: %v", err)
|
||||
}
|
||||
if len(delResult.ParticipantIDs) == 0 {
|
||||
t.Fatal("ParticipantIDs empty after a deleter disconnect that raced the post-commit DM lookup — " +
|
||||
"the other participant never sees the delete live")
|
||||
}
|
||||
}
|
||||
|
||||
// erroringGetChannelStore fails GetChannel for exactly one channel id,
|
||||
// modeling a transient DB hiccup on that lookup alone.
|
||||
type erroringGetChannelStore struct {
|
||||
Store
|
||||
failFor int64
|
||||
}
|
||||
|
||||
func (s erroringGetChannelStore) GetChannel(ctx context.Context, id int64) (*db.Channel, error) {
|
||||
if id == s.failFor {
|
||||
return nil, errors.New("simulated transient GetChannel failure")
|
||||
}
|
||||
return s.Store.GetChannel(ctx, id)
|
||||
}
|
||||
|
||||
// OC-0074: a GetChannel failure inside EditMessage must not fail OPEN into
|
||||
// the non-DM permission branch — that branch passes on the base role mask
|
||||
// (SEND_MESSAGES|READ_MESSAGES) with no per-channel override to stop it,
|
||||
// skipping both the DM-participant check and the block gate entirely.
|
||||
func TestEditMessage_FailsClosedWhenChannelLookupErrors(t *testing.T) {
|
||||
database, permSvc := newDMFixture(t)
|
||||
plainSvc := NewMessageService(database, permSvc, nil)
|
||||
|
||||
sent, err := plainSvc.SendMessage(context.Background(), SendMessageParams{
|
||||
ChannelID: 50, UserID: 1, Username: "alice", Content: "hi bob",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("send: %v", err)
|
||||
}
|
||||
seedBlock(t, database, 2, 1) // bob blocks alice
|
||||
|
||||
svc := NewMessageService(erroringGetChannelStore{Store: database, failFor: 50}, permSvc, nil)
|
||||
|
||||
if _, err := svc.EditMessage(context.Background(), 1, sent.MessageID, "slipped past the block"); err == nil {
|
||||
t.Fatal("EditMessage succeeded despite a failed channel lookup and an active block — " +
|
||||
"a GetChannel error must fail closed, not fall through to the non-DM permission path")
|
||||
}
|
||||
msg, err := database.GetMessage(context.Background(), sent.MessageID)
|
||||
if err != nil || msg == nil {
|
||||
t.Fatalf("GetMessage: %v", err)
|
||||
}
|
||||
if msg.Content != "hi bob" {
|
||||
t.Fatalf("content must survive the refused edit, got %q", msg.Content)
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0077: DeleteMessage must refuse to mutate a message in an archived
|
||||
// channel, mirroring the SendMessage gate (message_crud.go:54).
|
||||
func TestDeleteMessage_RefusedInArchivedChannel(t *testing.T) {
|
||||
svc, database := newTestMessageService(t)
|
||||
ctx := context.Background()
|
||||
|
||||
sent, err := svc.SendMessage(ctx, SendMessageParams{
|
||||
ChannelID: 10, UserID: 1, Username: "alice", RoleName: "member", Content: "delete me",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("send: %v", err)
|
||||
}
|
||||
|
||||
if _, err := database.ExecContext(ctx, `UPDATE channels SET archived = 1 WHERE id = 10`); err != nil {
|
||||
t.Fatalf("archive channel: %v", err)
|
||||
}
|
||||
|
||||
if _, err := svc.DeleteMessage(ctx, 1, sent.MessageID); !errors.Is(err, ErrForbidden) {
|
||||
t.Fatalf("DeleteMessage in an archived channel: err = %v, want ErrForbidden", err)
|
||||
}
|
||||
|
||||
msg, err := database.GetMessage(ctx, sent.MessageID)
|
||||
if err != nil || msg == nil {
|
||||
t.Fatalf("GetMessage: %v", err)
|
||||
}
|
||||
if msg.Deleted {
|
||||
t.Fatal("message must survive a delete attempt against an archived channel")
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0106: OpenDM is INSERT OR IGNORE and SendMessage used to append every
|
||||
// recipient to OpenedDMFor unconditionally, so a DM that is already open
|
||||
// still re-emits dm_channel_open (and bumps the hub's global visibility
|
||||
// watermark) on every single message. Once a DM is open for a recipient, a
|
||||
// second send must not report it as freshly opened.
|
||||
func TestSendMessage_DoesNotReopenAlreadyOpenDM(t *testing.T) {
|
||||
database, permSvc := newDMFixture(t)
|
||||
svc := NewMessageService(database, permSvc, nil)
|
||||
ctx := context.Background()
|
||||
|
||||
first, err := svc.SendMessage(ctx, SendMessageParams{
|
||||
ChannelID: 50, UserID: 1, Username: "alice", Content: "first",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("first send: %v", err)
|
||||
}
|
||||
if len(first.OpenedDMFor) != 1 || first.OpenedDMFor[0] != 2 {
|
||||
t.Fatalf("first send OpenedDMFor = %v, want [2] (bob's first open)", first.OpenedDMFor)
|
||||
}
|
||||
|
||||
second, err := svc.SendMessage(ctx, SendMessageParams{
|
||||
ChannelID: 50, UserID: 1, Username: "alice", Content: "second",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("second send: %v", err)
|
||||
}
|
||||
if len(second.OpenedDMFor) != 0 {
|
||||
t.Fatalf("second send OpenedDMFor = %v, want [] — the DM was already open for bob, "+
|
||||
"so re-reporting it forces a redundant dm_channel_open and a global visibility-watermark bump "+
|
||||
"for every other connected client's next reconnect", second.OpenedDMFor)
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0033 residual: the recipient's dm_open_state re-open must survive the
|
||||
// sender's disconnect too — GetDMParticipantIDs was detached from ctx but the
|
||||
// OpenDM loop was not, so a canceled request ctx silently skipped the re-open
|
||||
// and the recipient's sidebar never learned the DM existed.
|
||||
func TestSendMessage_DMReopenSurvivesSenderDisconnectAfterCommit(t *testing.T) {
|
||||
database, permSvc := newDMFixture(t)
|
||||
// Bob has closed the DM; this send must genuinely re-open it for him.
|
||||
if err := database.CloseDM(context.Background(), 2, 50); err != nil {
|
||||
t.Fatalf("CloseDM: %v", err)
|
||||
}
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
svc := NewMessageService(disconnectAfterWriteStore{Store: database, cancel: cancel}, permSvc, nil)
|
||||
|
||||
result, err := svc.SendMessage(ctx, SendMessageParams{
|
||||
ChannelID: 50, UserID: 1, Username: "alice", Content: "hi bob",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("SendMessage: %v", err)
|
||||
}
|
||||
if len(result.OpenedDMFor) != 1 || result.OpenedDMFor[0] != 2 {
|
||||
t.Fatalf("OpenedDMFor = %v, want [2] — OpenDM must run detached from the request ctx, "+
|
||||
"or a sender disconnect after commit leaves the DM invisible in the recipient's sidebar",
|
||||
result.OpenedDMFor)
|
||||
}
|
||||
}
|
||||
|
||||
// Same fail-closed rule the edit path got (OC-0074): DeleteMessage must not
|
||||
// fall open into the non-DM permission branch — and past the new archived
|
||||
// gate (OC-0077) — when the channel lookup errors.
|
||||
func TestDeleteMessage_FailsClosedWhenChannelLookupErrors(t *testing.T) {
|
||||
database, permSvc := newDMFixture(t)
|
||||
|
||||
sendSvc := NewMessageService(database, permSvc, nil)
|
||||
sent, err := sendSvc.SendMessage(context.Background(), SendMessageParams{
|
||||
ChannelID: 50, UserID: 1, Username: "alice", Content: "to be deleted",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("SendMessage: %v", err)
|
||||
}
|
||||
|
||||
svc := NewMessageService(erroringGetChannelStore{Store: database, failFor: 50}, permSvc, nil)
|
||||
if _, err := svc.DeleteMessage(context.Background(), 1, sent.MessageID); err == nil {
|
||||
t.Fatal("DeleteMessage succeeded despite a failed channel lookup — " +
|
||||
"a GetChannel error must fail closed, not fall through to the non-DM permission path")
|
||||
}
|
||||
msg, err := database.GetMessage(context.Background(), sent.MessageID)
|
||||
if err != nil || msg == nil {
|
||||
t.Fatalf("message must survive the refused delete; GetMessage: msg=%v err=%v", msg, err)
|
||||
}
|
||||
}
|
||||
@@ -2,6 +2,7 @@ package service
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"slices"
|
||||
@@ -224,5 +225,17 @@ func (s *MessageService) SetMessagePinned(ctx context.Context, userID, channelID
|
||||
if err != nil || msg == nil || msg.ChannelID != channelID {
|
||||
return fmt.Errorf("%w: message not found in this channel", ErrNotFound)
|
||||
}
|
||||
return s.st.SetMessagePinned(ctx, msgID, pinned)
|
||||
if err := s.st.SetMessagePinned(ctx, msgID, pinned); err != nil {
|
||||
// The pin SQL excludes soft-deleted rows, so a message deleted between
|
||||
// the GetMessage check above and this UPDATE (or one whose Deleted flag
|
||||
// we didn't re-check) surfaces here as db.ErrNotFound. Map it to the
|
||||
// service taxonomy so writeServiceError answers 404, not a 500 — same
|
||||
// class of guard as EditMessage's ErrDeletedMessage and handleReaction's
|
||||
// ErrBadRequest on their own deleted-message paths.
|
||||
if errors.Is(err, db.ErrNotFound) {
|
||||
return fmt.Errorf("%w: message not found in this channel", ErrNotFound)
|
||||
}
|
||||
return fmt.Errorf("%w: %v", ErrInternal, err)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -105,6 +105,7 @@ func (s *MessageService) handleReaction(ctx context.Context, userID, msgID int64
|
||||
ch, chErr := s.st.GetChannel(ctx, msg.ChannelID)
|
||||
isDM := chErr == nil && ch != nil && ch.Type == "dm"
|
||||
|
||||
var participantIDs []int64
|
||||
if isDM {
|
||||
ok, dmErr := s.st.IsDMParticipant(ctx, userID, msg.ChannelID)
|
||||
if dmErr != nil || !ok {
|
||||
@@ -113,6 +114,18 @@ func (s *MessageService) handleReaction(ctx context.Context, userID, msgID int64
|
||||
if blkErr := requireDMNotBlocked(ctx, s.st, userID, msg.ChannelID); blkErr != nil {
|
||||
return nil, blkErr
|
||||
}
|
||||
// Resolve the fan-out audience before mutating anything. Participants
|
||||
// are unaffected by the reaction itself, so failing here is cheap;
|
||||
// fetching this after AddReaction/RemoveReaction commits (as this
|
||||
// used to) risked a reaction persisted with no participant list to
|
||||
// broadcast it to, which reactionV2Handler would then fan out to
|
||||
// nobody while reporting success to the caller.
|
||||
ids, pErr := s.st.GetDMParticipantIDs(ctx, msg.ChannelID)
|
||||
if pErr != nil {
|
||||
slog.Error("MessageService.handleReaction GetDMParticipantIDs", "err", pErr, "channel_id", msg.ChannelID)
|
||||
return nil, fmt.Errorf("%w: failed to resolve DM participants", ErrInternal)
|
||||
}
|
||||
participantIDs = ids
|
||||
} else if !s.perms.HasChannelPerm(ctx, userID, msg.ChannelID, permissions.ReadMessages|permissions.AddReactions) {
|
||||
// Require READ_MESSAGES in addition to ADD_REACTIONS so a user cannot
|
||||
// react in a channel they cannot read. Mirrors checkSendPermission,
|
||||
@@ -144,12 +157,7 @@ func (s *MessageService) handleReaction(ctx context.Context, userID, msgID int64
|
||||
}
|
||||
|
||||
if isDM {
|
||||
participantIDs, pErr := s.st.GetDMParticipantIDs(ctx, msg.ChannelID)
|
||||
if pErr != nil {
|
||||
slog.Error("MessageService.handleReaction GetDMParticipantIDs", "err", pErr, "channel_id", msg.ChannelID)
|
||||
} else {
|
||||
result.ParticipantIDs = participantIDs
|
||||
}
|
||||
result.ParticipantIDs = participantIDs
|
||||
}
|
||||
|
||||
return result, nil
|
||||
|
||||
@@ -0,0 +1,72 @@
|
||||
package service
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"testing"
|
||||
|
||||
"github.com/owncord/server/db"
|
||||
"github.com/owncord/server/permissions"
|
||||
)
|
||||
|
||||
// errDMParticipantsStore wraps a real *db.DB but always fails
|
||||
// GetDMParticipantIDs, so the fail-closed contract for OC-0069 is testable.
|
||||
// Embedding *db.DB satisfies the service Store interface; only the one
|
||||
// overridden method diverges, every other call still hits the real database.
|
||||
type errDMParticipantsStore struct {
|
||||
*db.DB
|
||||
}
|
||||
|
||||
func (errDMParticipantsStore) GetDMParticipantIDs(context.Context, int64) ([]int64, error) {
|
||||
return nil, errors.New("boom")
|
||||
}
|
||||
|
||||
// TestHandleReaction_DMParticipantFetchErrorFailsClosed locks OC-0069: a DM
|
||||
// reaction must not be persisted with no way to notify anyone. Before the
|
||||
// fix, handleReaction committed AddReaction/RemoveReaction first and only
|
||||
// then fetched GetDMParticipantIDs; a failure there was logged and
|
||||
// swallowed, leaving the reaction row committed but result.ParticipantIDs
|
||||
// nil, so reactionV2Handler fanned the reaction_update out to nobody while
|
||||
// returning success to the caller. The fix must resolve participants before
|
||||
// mutating and fail the whole request on error.
|
||||
func TestHandleReaction_DMParticipantFetchErrorFailsClosed(t *testing.T) {
|
||||
database := newTestDB(t)
|
||||
seedRole(t, database, &db.Role{
|
||||
ID: permissions.MemberRoleID,
|
||||
Name: "member",
|
||||
Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.AddReactions,
|
||||
Position: 1,
|
||||
})
|
||||
seedUser(t, database, &db.User{ID: 1, Username: "alice"})
|
||||
seedUser(t, database, &db.User{ID: 2, Username: "bob"})
|
||||
seedUserRole(t, database, 1, permissions.MemberRoleID)
|
||||
seedUserRole(t, database, 2, permissions.MemberRoleID)
|
||||
seedChannel(t, database, &db.Channel{ID: 50, Name: "dm-1-2", Type: "dm"})
|
||||
seedDMParticipant(t, database, 50, 1)
|
||||
seedDMParticipant(t, database, 50, 2)
|
||||
|
||||
permSvc := NewPermissionService(database, permissions.NewChecker(database))
|
||||
realSvc := NewMessageService(database, permSvc, nil)
|
||||
|
||||
sent, err := realSvc.SendMessage(context.Background(), SendMessageParams{
|
||||
ChannelID: 50, UserID: 1, Username: "alice", Content: "hi bob",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("send: %v", err)
|
||||
}
|
||||
|
||||
failingStore := errDMParticipantsStore{DB: database}
|
||||
svc := NewMessageService(failingStore, permSvc, nil)
|
||||
|
||||
if _, err := svc.AddReaction(context.Background(), 1, sent.MessageID, "👋"); err == nil {
|
||||
t.Fatal("AddReaction must fail when GetDMParticipantIDs errors, not silently drop the fan-out")
|
||||
}
|
||||
|
||||
counts, err := database.GetReactions(context.Background(), sent.MessageID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetReactions: %v", err)
|
||||
}
|
||||
if len(counts) != 0 {
|
||||
t.Fatalf("reaction must not be committed when its fan-out cannot be resolved, got %d reaction rows", len(counts))
|
||||
}
|
||||
}
|
||||
@@ -829,3 +829,50 @@ func TestDMBlock_EnforcedOnEveryInteractionSink(t *testing.T) {
|
||||
t.Fatalf("blocker is equally refused, matching IsEitherBlocked: got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// TestSetMessagePinned_DeletedMessageReturnsNotFound covers the same
|
||||
// deleted-message guard EditMessage (ErrDeletedMessage) and handleReaction
|
||||
// (ErrBadRequest) already have on their sinks. GetMessage deliberately still
|
||||
// returns a soft-deleted row (history queries rely on that), so the pin SQL's
|
||||
// `AND deleted = 0` clause matches zero rows and the store returns
|
||||
// db.ErrNotFound — a distinct sentinel from service.ErrNotFound that
|
||||
// writeServiceError does not recognize. Unwrapped, that misclassifies a
|
||||
// perfectly ordinary not-found as a 500 INTERNAL_ERROR.
|
||||
func TestSetMessagePinned_DeletedMessageReturnsNotFound(t *testing.T) {
|
||||
database := newTestDB(t)
|
||||
seedRole(t, database, &db.Role{
|
||||
ID: permissions.MemberRoleID,
|
||||
Name: "member",
|
||||
Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.ManageMessages,
|
||||
Position: 1,
|
||||
})
|
||||
seedUser(t, database, &db.User{ID: 1, Username: "alice"})
|
||||
seedUserRole(t, database, 1, permissions.MemberRoleID)
|
||||
seedChannel(t, database, &db.Channel{ID: 10, Name: "general", Type: "text"})
|
||||
|
||||
permSvc := NewPermissionService(database, permissions.NewChecker(database))
|
||||
svc := NewMessageService(database, permSvc, nil)
|
||||
|
||||
sent, err := svc.SendMessage(context.Background(), SendMessageParams{
|
||||
ChannelID: 10, UserID: 1, Username: "alice", Content: "will be deleted",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("send: %v", err)
|
||||
}
|
||||
|
||||
if err := svc.SetMessagePinned(context.Background(), 1, 10, sent.MessageID, true); err != nil {
|
||||
t.Fatalf("pin: %v", err)
|
||||
}
|
||||
|
||||
if _, err := svc.DeleteMessage(context.Background(), 1, sent.MessageID); err != nil {
|
||||
t.Fatalf("delete: %v", err)
|
||||
}
|
||||
|
||||
err = svc.SetMessagePinned(context.Background(), 1, 10, sent.MessageID, false)
|
||||
if !errors.Is(err, ErrNotFound) {
|
||||
t.Fatalf("expected service.ErrNotFound for a soft-deleted message, got %v", err)
|
||||
}
|
||||
if errors.Is(err, ErrInternal) {
|
||||
t.Fatalf("must not be classified as an internal error: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -145,6 +145,93 @@ func TestRollbackVoiceJoin_NoDBState_DoesNotPanic(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0044: rollbackVoiceJoin must not let a stale/failed join's compensating
|
||||
// delete destroy a newer voice_states row a second connection for the same
|
||||
// user has since legitimately established. A delayed rollback (the dying
|
||||
// connection that triggered it is the most common case) firing
|
||||
// "DELETE ... WHERE user_id = ?" with no channel/token condition wipes
|
||||
// whatever the user is currently in, not just the failed join.
|
||||
//
|
||||
// This covers the token-generation-failure call site (voice_join.go:300),
|
||||
// which already holds the failed join's own JoinedAt by the time it rolls
|
||||
// back — that value must scope the delete instead of being discarded.
|
||||
func TestRollbackVoiceJoin_StaleTokenDoesNotDeleteNewerJoin(t *testing.T) {
|
||||
hub, database := newCoverageHub(t)
|
||||
user := seedCoverageOwner(t, database, "rb-stale-token")
|
||||
staleChID := seedVoiceChannel(t, database, "rb-stale-token-old")
|
||||
newChID := seedVoiceChannel(t, database, "rb-stale-token-new")
|
||||
send := make(chan []byte, 64)
|
||||
c := ws.NewTestClientWithUser(hub, user, 0, send)
|
||||
hub.Register(c)
|
||||
waitRegistered(t, hub, c)
|
||||
|
||||
// A second connection for the same user has already joined a different
|
||||
// channel and committed its own voice_states row by the time the stale
|
||||
// rollback below runs.
|
||||
if err := database.JoinVoiceChannel(context.Background(), user.ID, newChID); err != nil {
|
||||
t.Fatalf("JoinVoiceChannel (newer): %v", err)
|
||||
}
|
||||
newState, err := database.GetVoiceState(context.Background(), user.ID)
|
||||
if err != nil || newState == nil {
|
||||
t.Fatalf("GetVoiceState (newer): state=%v err=%v", newState, err)
|
||||
}
|
||||
|
||||
// Roll back the stale, now-superseded join to staleChID using a join
|
||||
// token that does not match the row currently in the DB (newChID's).
|
||||
hub.RollbackVoiceJoinWithTokenForTest(c, staleChID, "stale-token-does-not-match-anything")
|
||||
|
||||
got, err := database.GetVoiceState(context.Background(), user.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetVoiceState after rollback: %v", err)
|
||||
}
|
||||
if got == nil {
|
||||
t.Fatal("rollbackVoiceJoin deleted the newer voice membership it did not own")
|
||||
}
|
||||
if got.ChannelID != newChID || got.JoinedAt != newState.JoinedAt {
|
||||
t.Fatalf("voice state after rollback = %+v, want unchanged channel=%d joined_at=%q",
|
||||
got, newChID, newState.JoinedAt)
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0044: mirrors the GetVoiceState-failure call site (voice_join.go:208),
|
||||
// which never learns the failed join's own JoinedAt and so rolls back with
|
||||
// an empty token. That must not degrade to the old unconditional
|
||||
// "DELETE ... WHERE user_id = ?" — it must re-read the row and refuse to
|
||||
// touch it unless the row still names the channel being rolled back.
|
||||
func TestRollbackVoiceJoin_EmptyTokenDoesNotDeleteDifferentChannel(t *testing.T) {
|
||||
hub, database := newCoverageHub(t)
|
||||
user := seedCoverageOwner(t, database, "rb-empty-token")
|
||||
staleChID := seedVoiceChannel(t, database, "rb-empty-token-old")
|
||||
newChID := seedVoiceChannel(t, database, "rb-empty-token-new")
|
||||
send := make(chan []byte, 64)
|
||||
c := ws.NewTestClientWithUser(hub, user, 0, send)
|
||||
hub.Register(c)
|
||||
waitRegistered(t, hub, c)
|
||||
|
||||
if err := database.JoinVoiceChannel(context.Background(), user.ID, newChID); err != nil {
|
||||
t.Fatalf("JoinVoiceChannel (newer): %v", err)
|
||||
}
|
||||
newState, err := database.GetVoiceState(context.Background(), user.ID)
|
||||
if err != nil || newState == nil {
|
||||
t.Fatalf("GetVoiceState (newer): state=%v err=%v", newState, err)
|
||||
}
|
||||
|
||||
// RollbackVoiceJoinForTest exercises the empty-joinedAt path.
|
||||
hub.RollbackVoiceJoinForTest(c, staleChID)
|
||||
|
||||
got, err := database.GetVoiceState(context.Background(), user.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetVoiceState after rollback: %v", err)
|
||||
}
|
||||
if got == nil {
|
||||
t.Fatal("rollbackVoiceJoin deleted the newer voice membership it did not own")
|
||||
}
|
||||
if got.ChannelID != newChID || got.JoinedAt != newState.JoinedAt {
|
||||
t.Fatalf("voice state after rollback = %+v, want unchanged channel=%d joined_at=%q",
|
||||
got, newChID, newState.JoinedAt)
|
||||
}
|
||||
}
|
||||
|
||||
// ─── leaveVoiceChannelWithRetry (voice_leave.go:57) ─────────────────────────
|
||||
|
||||
func TestLeaveVoiceChannelWithRetry_SuccessOnFirstAttempt(t *testing.T) {
|
||||
|
||||
@@ -81,6 +81,21 @@ func (h *Hub) SubscribeVoiceTopicForTest(c *Client, channelID int64) {
|
||||
h.pubsub.Subscribe(c, VoiceTopic(channelID))
|
||||
}
|
||||
|
||||
// SubscribedToChannelTopicForTest reports whether c itself (identity compare,
|
||||
// not just its userID) holds the subscription to channelID's channel topic.
|
||||
func (h *Hub) SubscribedToChannelTopicForTest(c *Client, channelID int64) bool {
|
||||
h.pubsub.mu.RLock()
|
||||
defer h.pubsub.mu.RUnlock()
|
||||
return h.pubsub.topics[ChannelTopic(channelID)][c.userID] == c
|
||||
}
|
||||
|
||||
// ApplySetChannelIDForTest exposes Hub.applySetChannelID for external tests —
|
||||
// the SetChannelID applier that channel_focus's handleMessage result runs
|
||||
// through (subscribe + live re-validate, OC-0024).
|
||||
func (h *Hub) ApplySetChannelIDForTest(c *Client, newChID int64) {
|
||||
h.applySetChannelID(c, newChID)
|
||||
}
|
||||
|
||||
// SetClientVoiceStateForTest sets both the voice channel and join token.
|
||||
func SetClientVoiceStateForTest(c *Client, channelID int64, joinToken string) {
|
||||
c.voiceMu.Lock()
|
||||
@@ -177,9 +192,17 @@ func TouchForTest(c *Client) {
|
||||
c.touch()
|
||||
}
|
||||
|
||||
// RollbackVoiceJoinForTest exposes Hub.rollbackVoiceJoin for external tests.
|
||||
// RollbackVoiceJoinForTest exposes Hub.rollbackVoiceJoin for external tests,
|
||||
// exercising the empty-joinedAt (re-read) path.
|
||||
func (h *Hub) RollbackVoiceJoinForTest(c *Client, channelID int64) {
|
||||
h.rollbackVoiceJoin(context.Background(), c, channelID, true)
|
||||
h.rollbackVoiceJoin(context.Background(), c, channelID, "", true)
|
||||
}
|
||||
|
||||
// RollbackVoiceJoinWithTokenForTest exposes Hub.rollbackVoiceJoin with an
|
||||
// explicit join token, for external tests exercising the join-instance-scoped
|
||||
// delete (OC-0044).
|
||||
func (h *Hub) RollbackVoiceJoinWithTokenForTest(c *Client, channelID int64, joinedAt string) {
|
||||
h.rollbackVoiceJoin(context.Background(), c, channelID, joinedAt, true)
|
||||
}
|
||||
|
||||
// LeaveVoiceChannelWithRetryForTest exposes leaveVoiceChannelWithRetry for external tests.
|
||||
|
||||
@@ -0,0 +1,171 @@
|
||||
package ws_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"testing"
|
||||
|
||||
"github.com/owncord/server/permissions"
|
||||
"github.com/owncord/server/ws"
|
||||
)
|
||||
|
||||
// TestApplySetChannelID_RevalidatesAfterConcurrentRevoke pins OC-0024: the
|
||||
// channel_focus applier (Hub.applySetChannelID, run from handleMessage after
|
||||
// the service-layer READ_MESSAGES check already passed) must not leave a
|
||||
// socket subscribed to a channel's pub/sub topic once access is gone.
|
||||
//
|
||||
// service.HandleChannelFocus's permission check and the applier's Subscribe
|
||||
// call are separated by two SQLite round trips (GetLatestMessageID,
|
||||
// UpdateReadState). A revoke (channel_overrides deny + admin's
|
||||
// RefreshChannelVisibility sweep) that commits inside that window finds the
|
||||
// client not yet subscribed — Unsubscribe is a no-op and c.channelID doesn't
|
||||
// match yet — so the sweep does nothing, and the applier's Subscribe lands
|
||||
// right after with no further sweep ever revisiting it. This test reproduces
|
||||
// that ordering directly: the deny override is already committed by the time
|
||||
// the applier runs, exactly as it would be had the revoke landed first.
|
||||
func TestApplySetChannelID_RevalidatesAfterConcurrentRevoke(t *testing.T) {
|
||||
hub, database := newHandlerHub(t)
|
||||
user := seedMemberUser(t, database, "focus-race-user")
|
||||
chOld := seedTestChannel(t, database, "focus-race-old")
|
||||
chNew := seedTestChannel(t, database, "focus-race-new")
|
||||
|
||||
// Simulate the admin's revoke having already committed before the
|
||||
// applier runs: deny READ_MESSAGES for Member on the channel the focus
|
||||
// handler already cleared the user for.
|
||||
denyReadOnChannel(t, database, chNew, permissions.MemberRoleID)
|
||||
|
||||
send := make(chan []byte, 16)
|
||||
c := ws.NewTestClientWithUser(hub, user, chOld, send)
|
||||
hub.Register(c)
|
||||
waitRegistered(t, hub, c)
|
||||
|
||||
// This is exactly what handleMessage's applier runs for a channel_focus
|
||||
// result carrying SetChannelID: &chNew — the READ check already happened
|
||||
// (and passed, before the revoke) inside the handler; only the
|
||||
// subscribe-and-focus side effect is left to apply.
|
||||
hub.ApplySetChannelIDForTest(c, chNew)
|
||||
|
||||
if hub.SubscribedToChannelTopicForTest(c, chNew) {
|
||||
t.Error("client is subscribed to a channel topic it no longer has READ_MESSAGES on")
|
||||
}
|
||||
if got := ws.ClientChannelIDForTest(c); got != 0 {
|
||||
t.Errorf("client channelID = %d, want 0 (focus must not stick to an unreadable channel)", got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestApplySetChannelID_AllowedChannel_SubscribesAndFocuses is the control:
|
||||
// when access is still valid at apply time, the applier must subscribe and
|
||||
// focus normally (the re-validation must not be a blanket deny).
|
||||
func TestApplySetChannelID_AllowedChannel_SubscribesAndFocuses(t *testing.T) {
|
||||
hub, database := newHandlerHub(t)
|
||||
user := seedMemberUser(t, database, "focus-ok-user")
|
||||
chOld := seedTestChannel(t, database, "focus-ok-old")
|
||||
chNew := seedTestChannel(t, database, "focus-ok-new")
|
||||
|
||||
send := make(chan []byte, 16)
|
||||
c := ws.NewTestClientWithUser(hub, user, chOld, send)
|
||||
hub.Register(c)
|
||||
waitRegistered(t, hub, c)
|
||||
|
||||
hub.ApplySetChannelIDForTest(c, chNew)
|
||||
|
||||
if !hub.SubscribedToChannelTopicForTest(c, chNew) {
|
||||
t.Error("client should be subscribed to the newly focused, readable channel")
|
||||
}
|
||||
if got := ws.ClientChannelIDForTest(c); got != chNew {
|
||||
t.Errorf("client channelID = %d, want %d", got, chNew)
|
||||
}
|
||||
}
|
||||
|
||||
// TestApplySetChannelID_DMParticipantWithoutRead_KeepsFocus: the admission
|
||||
// gate (service.HandleChannelFocus) deliberately waives the READ_MESSAGES
|
||||
// role bit for DM channels — a DM is participant-gated, not role-gated. The
|
||||
// applier's re-validation must mirror that, or a DM participant whose role
|
||||
// lacks READ on the channel gets every focus silently unwound and their DM
|
||||
// message stream never arrives.
|
||||
func TestApplySetChannelID_DMParticipantWithoutRead_KeepsFocus(t *testing.T) {
|
||||
hub, database := newHandlerHub(t)
|
||||
user := seedMemberUser(t, database, "focus-dm-user")
|
||||
other := seedMemberUser(t, database, "focus-dm-other")
|
||||
dm, _, err := database.GetOrCreateDMChannel(context.Background(), user.ID, other.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetOrCreateDMChannel: %v", err)
|
||||
}
|
||||
|
||||
// A deny-READ override on the DM channel: the focus admission gate never
|
||||
// consults it for DMs, so the applier's recheck must not either.
|
||||
denyReadOnChannel(t, database, dm.ID, permissions.MemberRoleID)
|
||||
|
||||
send := make(chan []byte, 16)
|
||||
c := ws.NewTestClientWithUser(hub, user, 0, send)
|
||||
hub.Register(c)
|
||||
waitRegistered(t, hub, c)
|
||||
|
||||
hub.ApplySetChannelIDForTest(c, dm.ID)
|
||||
|
||||
if !hub.SubscribedToChannelTopicForTest(c, dm.ID) {
|
||||
t.Error("DM participant must stay subscribed to their own DM regardless of the READ role bit")
|
||||
}
|
||||
if got := ws.ClientChannelIDForTest(c); got != dm.ID {
|
||||
t.Errorf("client channelID = %d, want %d (DM focus must stick)", got, dm.ID)
|
||||
}
|
||||
}
|
||||
|
||||
// TestApplySetChannelID_DeletedChannel_Unwinds: a channel deleted between the
|
||||
// admission gate and the applier mirrors the admission gate's own answer for
|
||||
// a missing row (NotFound): the subscription is unwound, matching the delete
|
||||
// sweep the client will also receive.
|
||||
func TestApplySetChannelID_DeletedChannel_Unwinds(t *testing.T) {
|
||||
hub, database := newHandlerHub(t)
|
||||
user := seedMemberUser(t, database, "focus-del-user")
|
||||
ch := seedTestChannel(t, database, "focus-del-chan")
|
||||
|
||||
send := make(chan []byte, 16)
|
||||
c := ws.NewTestClientWithUser(hub, user, 0, send)
|
||||
hub.Register(c)
|
||||
waitRegistered(t, hub, c)
|
||||
|
||||
if err := database.DeleteChannel(context.Background(), ch); err != nil {
|
||||
t.Fatalf("DeleteChannel: %v", err)
|
||||
}
|
||||
|
||||
hub.ApplySetChannelIDForTest(c, ch)
|
||||
|
||||
if hub.SubscribedToChannelTopicForTest(c, ch) {
|
||||
t.Error("client must not stay subscribed to a deleted channel's topic")
|
||||
}
|
||||
if got := ws.ClientChannelIDForTest(c); got != 0 {
|
||||
t.Errorf("client channelID = %d, want 0 after focusing a deleted channel", got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestApplySetChannelID_TransientLookupError_KeepsFocus: the re-validation
|
||||
// exists to catch a concrete revoke that landed inside the Subscribe race
|
||||
// window. On a transient lookup failure there is no positive denial — the
|
||||
// sweeps stay authoritative — so the freshly admitted focus must be kept:
|
||||
// unwinding would turn any DB hiccup into a silently dead message stream
|
||||
// with no error frame sent to the client.
|
||||
func TestApplySetChannelID_TransientLookupError_KeepsFocus(t *testing.T) {
|
||||
hub, database := newHandlerHub(t)
|
||||
user := seedMemberUser(t, database, "focus-err-user")
|
||||
ch := seedTestChannel(t, database, "focus-err-chan")
|
||||
|
||||
send := make(chan []byte, 16)
|
||||
c := ws.NewTestClientWithUser(hub, user, 0, send)
|
||||
hub.Register(c)
|
||||
waitRegistered(t, hub, c)
|
||||
|
||||
// Closing the DB makes every lookup in the recheck error — the closest
|
||||
// deterministic stand-in for a transient failure on those queries.
|
||||
if err := database.Close(); err != nil {
|
||||
t.Fatalf("Close: %v", err)
|
||||
}
|
||||
|
||||
hub.ApplySetChannelIDForTest(c, ch)
|
||||
|
||||
if !hub.SubscribedToChannelTopicForTest(c, ch) {
|
||||
t.Error("a transient lookup failure must not unwind a just-admitted focus")
|
||||
}
|
||||
if got := ws.ClientChannelIDForTest(c); got != ch {
|
||||
t.Errorf("client channelID = %d, want %d (focus must survive a transient lookup error)", got, ch)
|
||||
}
|
||||
}
|
||||
@@ -70,10 +70,11 @@ func TestMarkRead_ClearsMentionCount(t *testing.T) {
|
||||
hub, database := newCoverageHub(t)
|
||||
user := seedCoverageOwner(t, database, "markread-mention-user")
|
||||
chID := seedTestChannel(t, database, "markread-mention-chan")
|
||||
if _, err := database.CreateMessage(context.Background(), chID, user.ID, "@you", nil); err != nil {
|
||||
msgID, err := database.CreateMessage(context.Background(), chID, user.ID, "@you", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("CreateMessage: %v", err)
|
||||
}
|
||||
if err := database.IncrementMentionCounts(context.Background(), chID, []int64{user.ID}); err != nil {
|
||||
if err := database.IncrementMentionCounts(context.Background(), chID, msgID, []int64{user.ID}); err != nil {
|
||||
t.Fatalf("IncrementMentionCounts: %v", err)
|
||||
}
|
||||
|
||||
|
||||
+57
-14
@@ -8,6 +8,7 @@ import (
|
||||
|
||||
"github.com/owncord/server/auth"
|
||||
"github.com/owncord/server/db"
|
||||
"github.com/owncord/server/permissions"
|
||||
)
|
||||
|
||||
// HandleMessageForTest dispatches a raw WebSocket message from client c.
|
||||
@@ -156,20 +157,7 @@ func (h *Hub) handleMessage(c *Client, raw []byte) {
|
||||
|
||||
// Apply client state mutations and side effects.
|
||||
if result.SetChannelID != nil {
|
||||
oldChID := c.getChannelID()
|
||||
c.mu.Lock()
|
||||
c.channelID = *result.SetChannelID
|
||||
c.mu.Unlock()
|
||||
// Update pub/sub channel topic subscriptions.
|
||||
newChID := *result.SetChannelID
|
||||
if oldChID != newChID {
|
||||
if oldChID > 0 {
|
||||
c.hub.pubsub.Unsubscribe(c, ChannelTopic(oldChID))
|
||||
}
|
||||
if newChID > 0 {
|
||||
c.hub.pubsub.Subscribe(c, ChannelTopic(newChID))
|
||||
}
|
||||
}
|
||||
h.applySetChannelID(c, *result.SetChannelID)
|
||||
}
|
||||
if result.SetE2EEPubKey != nil {
|
||||
sig := ""
|
||||
@@ -201,6 +189,61 @@ func (h *Hub) handleMessage(c *Client, raw []byte) {
|
||||
}
|
||||
}
|
||||
|
||||
// applySetChannelID moves c's focused channel to newChID and updates its
|
||||
// pub/sub topic subscription to match.
|
||||
//
|
||||
// OC-0024: channel_focus's handler (service/channel.go) checks READ_MESSAGES
|
||||
// before returning, but that check and the Subscribe below are separated by
|
||||
// two SQLite round trips (GetLatestMessageID/UpdateReadState), and the hub's
|
||||
// visibility-revoke sweeps (RefreshChannelVisibility, revokeUnreadableChannels)
|
||||
// only ever Unsubscribe a topic the socket already holds at the instant they
|
||||
// run — a revoke that commits inside that window finds nothing to undo, and
|
||||
// the Subscribe that lands once the window closes is never revisited. Once
|
||||
// Subscribe is called we re-validate READ_MESSAGES live and unwind it on
|
||||
// failure, which closes the race from both directions: a revoke that already
|
||||
// committed is caught right here, and a revoke that commits afterward still
|
||||
// finds the subscription the sweeps have always expected to see.
|
||||
func (h *Hub) applySetChannelID(c *Client, newChID int64) {
|
||||
oldChID := c.getChannelID()
|
||||
c.mu.Lock()
|
||||
c.channelID = newChID
|
||||
c.mu.Unlock()
|
||||
if oldChID == newChID {
|
||||
return
|
||||
}
|
||||
if oldChID > 0 {
|
||||
h.pubsub.Unsubscribe(c, ChannelTopic(oldChID))
|
||||
}
|
||||
if newChID <= 0 {
|
||||
return
|
||||
}
|
||||
h.pubsub.Subscribe(c, ChannelTopic(newChID))
|
||||
// The re-validation mirrors HandleChannelFocus's admission gate: DMs are
|
||||
// participant-gated (the READ role bit is deliberately waived), non-DMs
|
||||
// need READ_MESSAGES, and a deleted channel is a denial. A transient
|
||||
// lookup error is NOT a denial — the recheck exists to catch a concrete
|
||||
// revoke in the Subscribe race window, the sweeps stay authoritative, and
|
||||
// unwinding on error would turn any DB hiccup into a silently dead
|
||||
// message stream with no error frame sent to the client.
|
||||
ch, chErr := h.db.GetChannel(c.ctx, newChID)
|
||||
if chErr != nil {
|
||||
return
|
||||
}
|
||||
if ch != nil && ch.Type == "dm" {
|
||||
if ok, dmErr := h.db.IsDMParticipant(c.ctx, c.userID, newChID); dmErr != nil || ok {
|
||||
return
|
||||
}
|
||||
} else if ch != nil && hasChannelAccess(c.ctx, h.db, h.permChecker, h.perms, c.userID, newChID, permissions.ReadMessages) {
|
||||
return
|
||||
}
|
||||
h.pubsub.Unsubscribe(c, ChannelTopic(newChID))
|
||||
c.mu.Lock()
|
||||
if c.channelID == newChID {
|
||||
c.channelID = 0
|
||||
}
|
||||
c.mu.Unlock()
|
||||
}
|
||||
|
||||
// hasChannelPerm reports whether the client's role has all the given permission bits.
|
||||
// Delegates to the unified permissions.Checker.
|
||||
//
|
||||
|
||||
+24
-15
@@ -98,11 +98,10 @@ func handleChatSendV2(ctx context.Context, cmd Command, info ClientInfo, deps an
|
||||
}
|
||||
}
|
||||
|
||||
events = append(events, MessageSentDMEvent{
|
||||
channelID: sendCmd.ChannelID(),
|
||||
participantIDs: result.ParticipantIDs,
|
||||
payload: broadcast,
|
||||
})
|
||||
events = append(events, dmEventOrFallback(
|
||||
MessageSentDMEvent{channelID: sendCmd.ChannelID(), participantIDs: result.ParticipantIDs, payload: broadcast},
|
||||
MessageSentChannelEvent{channelID: sendCmd.ChannelID(), payload: broadcast},
|
||||
result.ParticipantIDs))
|
||||
|
||||
return Result{Reply: reply, Events: events}
|
||||
}
|
||||
@@ -120,11 +119,10 @@ func handleChatEditV2(ctx context.Context, cmd Command, info ClientInfo, deps an
|
||||
editedPayload := buildChatEdited(result.MessageID, result.ChannelID, result.Content, result.EditedAt,
|
||||
result.Mentions, result.MentionsEveryone)
|
||||
if result.IsDM {
|
||||
return Result{Events: []Event{MessageEditedDMEvent{
|
||||
channelID: result.ChannelID,
|
||||
participantIDs: result.ParticipantIDs,
|
||||
payload: editedPayload,
|
||||
}}}
|
||||
return Result{Events: []Event{dmEventOrFallback(
|
||||
MessageEditedDMEvent{channelID: result.ChannelID, participantIDs: result.ParticipantIDs, payload: editedPayload},
|
||||
MessageEditedChannelEvent{channelID: result.ChannelID, payload: editedPayload},
|
||||
result.ParticipantIDs)}}
|
||||
}
|
||||
return Result{Events: []Event{MessageEditedChannelEvent{
|
||||
channelID: result.ChannelID,
|
||||
@@ -144,11 +142,10 @@ func handleChatDeleteV2(ctx context.Context, cmd Command, info ClientInfo, deps
|
||||
|
||||
deletedPayload := buildChatDeleted(result.MessageID, result.ChannelID)
|
||||
if result.IsDM {
|
||||
return Result{Events: []Event{MessageDeletedDMEvent{
|
||||
channelID: result.ChannelID,
|
||||
participantIDs: result.ParticipantIDs,
|
||||
payload: deletedPayload,
|
||||
}}}
|
||||
return Result{Events: []Event{dmEventOrFallback(
|
||||
MessageDeletedDMEvent{channelID: result.ChannelID, participantIDs: result.ParticipantIDs, payload: deletedPayload},
|
||||
MessageDeletedChannelEvent{channelID: result.ChannelID, payload: deletedPayload},
|
||||
result.ParticipantIDs)}}
|
||||
}
|
||||
return Result{Events: []Event{MessageDeletedChannelEvent{
|
||||
channelID: result.ChannelID,
|
||||
@@ -156,6 +153,18 @@ func handleChatDeleteV2(ctx context.Context, cmd Command, info ClientInfo, deps
|
||||
}}}
|
||||
}
|
||||
|
||||
// dmEventOrFallback returns the participant-targeted DM event, falling back
|
||||
// to the channel-topic broadcast when the participant list is empty — the
|
||||
// degraded shape a failed post-commit GetDMParticipantIDs leaves behind. A
|
||||
// sequenced frame addressed to nobody would consume a seq and reach no one;
|
||||
// the topic fallback still reaches whoever has the DM focused.
|
||||
func dmEventOrFallback(dmEvent, fallback Event, participantIDs []int64) Event {
|
||||
if len(participantIDs) == 0 {
|
||||
return fallback
|
||||
}
|
||||
return dmEvent
|
||||
}
|
||||
|
||||
// serviceErrorToResult converts a service-layer error to a WS Result.
|
||||
func serviceErrorToResult(err error) Result {
|
||||
switch {
|
||||
|
||||
@@ -0,0 +1,19 @@
|
||||
package ws
|
||||
|
||||
import "testing"
|
||||
|
||||
// dmEventOrFallback guards the degraded-fan-out shape: a DM event whose
|
||||
// participant list is empty (a failed post-commit participant lookup) must
|
||||
// fall back to the channel-topic broadcast instead of consuming a seq for a
|
||||
// frame addressed to nobody — the topic fallback still reaches whoever has
|
||||
// the DM focused.
|
||||
func TestDMEventOrFallback(t *testing.T) {
|
||||
dm := MessageSentDMEvent{channelID: 5}
|
||||
fb := MessageSentChannelEvent{channelID: 5}
|
||||
if _, ok := dmEventOrFallback(dm, fb, nil).(MessageSentChannelEvent); !ok {
|
||||
t.Error("empty participant list must fall back to the channel broadcast")
|
||||
}
|
||||
if _, ok := dmEventOrFallback(dm, fb, []int64{1, 2}).(MessageSentDMEvent); !ok {
|
||||
t.Error("a populated participant list must keep the DM-targeted event")
|
||||
}
|
||||
}
|
||||
@@ -346,6 +346,16 @@ func (h *Hub) bumpVisibilityWatermark() {
|
||||
}
|
||||
}
|
||||
|
||||
// MarkVisibilityChanged bumps the visibility watermark. It is the exported
|
||||
// entry point REST handlers (api.markDMVisibilityChanged, reached via a
|
||||
// dmVisibilityMarker type assertion) use to force the same full-resync
|
||||
// guarantee for an unsequenced, targeted DM event that the WS-side emitter of
|
||||
// the same event (emit.go DMChannelOpenEvent) already gets via
|
||||
// bumpVisibilityWatermark directly.
|
||||
func (h *Hub) MarkVisibilityChanged() {
|
||||
h.bumpVisibilityWatermark()
|
||||
}
|
||||
|
||||
// IsUserConnected returns true if a client with the given userID is already
|
||||
// registered in the hub. Safe to call from any goroutine.
|
||||
func (h *Hub) IsUserConnected(userID int64) bool {
|
||||
|
||||
@@ -66,6 +66,33 @@ func TestBumpVisibilityWatermark_ConcurrentCallsNeverRegress(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestMarkVisibilityChanged_BumpsWatermarkLikeInternalWriters pins OC-0013:
|
||||
// api.markDMVisibilityChanged reaches this bump through a type assertion on
|
||||
// an exported MarkVisibilityChanged() method — the same watermark the three
|
||||
// internal writers above ratchet via bumpVisibilityWatermark. Without an
|
||||
// exported wrapper, the assertion in api/dm_handler.go always misses against
|
||||
// the real *ws.Hub, so a REST-originated DM event (group create, rename,
|
||||
// close, group-leave) never bumps the watermark and a client that warm-
|
||||
// reconnects across it is wrongly admitted onto the replay path — which
|
||||
// cannot carry the unsequenced, targeted dm_channel_open/close those REST
|
||||
// handlers send.
|
||||
func TestMarkVisibilityChanged_BumpsWatermarkLikeInternalWriters(t *testing.T) {
|
||||
h := &Hub{}
|
||||
atomic.StoreUint64(&h.seq, 42)
|
||||
|
||||
h.MarkVisibilityChanged()
|
||||
|
||||
if got := h.visibilityChangeSeq.Load(); got != 42 {
|
||||
t.Fatalf("visibilityChangeSeq = %d after MarkVisibilityChanged, want 42", got)
|
||||
}
|
||||
if !h.mustFullResync(42) {
|
||||
t.Error("a client resuming from a seq at or before the REST DM event must be forced onto the full-ready path")
|
||||
}
|
||||
if h.mustFullResync(43) {
|
||||
t.Error("clients past the change must keep replaying normally")
|
||||
}
|
||||
}
|
||||
|
||||
// TestRevokeUnreadableChannels_WatermarkNeverRegresses targets the specific
|
||||
// defect: revokeUnreadableChannels used `defer h.visibilityChangeSeq.Store(atomic.LoadUint64(&h.seq))`,
|
||||
// whose argument Go evaluates at the DEFER STATEMENT (function entry), not at
|
||||
|
||||
@@ -86,7 +86,7 @@ func TestBuildReady_CarriesMentionCount(t *testing.T) {
|
||||
if err != nil {
|
||||
t.Fatalf("CreateChannel: %v", err)
|
||||
}
|
||||
if err := database.IncrementMentionCounts(ctx, chID, []int64{user.ID}); err != nil {
|
||||
if err := database.IncrementMentionCounts(ctx, chID, 1, []int64{user.ID}); err != nil {
|
||||
t.Fatalf("IncrementMentionCounts: %v", err)
|
||||
}
|
||||
|
||||
|
||||
+36
-15
@@ -67,8 +67,10 @@ func ServeWS(hub *Hub, database *db.DB, allowedOrigins []string) http.HandlerFun
|
||||
// try to replay missed events from the ring buffer instead of
|
||||
// sending a full ready payload.
|
||||
if lastSeq > 0 {
|
||||
if hub.handleReconnect(ctx, conn, c, database, lastSeq) {
|
||||
startPumps()
|
||||
if handled, shouldStartPumps := hub.handleReconnect(ctx, conn, c, database, lastSeq); handled {
|
||||
if shouldStartPumps {
|
||||
startPumps()
|
||||
}
|
||||
return
|
||||
}
|
||||
// Replay failed (seq too old) — fall through to full ready payload.
|
||||
@@ -116,9 +118,22 @@ func (h *Hub) upgradeAndAuth(
|
||||
return c, lastSeq, nil
|
||||
}
|
||||
|
||||
// handleReconnect attempts to resume a client via replay. Its two return
|
||||
// values are independent signals for ServeWS:
|
||||
// - handled reports whether this function owns the outcome of the
|
||||
// connection attempt. false means "replay isn't possible, fall through
|
||||
// to handleFreshConnect for a full ready."
|
||||
// - startPumps reports whether ServeWS should start readPump/writePump.
|
||||
// It is only meaningful when handled is true, and is false on the
|
||||
// handshake-write-failure paths below: those paths already ran the full
|
||||
// unregisterFailedHandshake teardown and closed conn themselves, so no
|
||||
// pump may start — readPump's defer would find the client already gone
|
||||
// (unregisterNow reporting replaced=false) and run that same teardown a
|
||||
// second time (OC-0051): a duplicate MarkUserDisconnected, a duplicate
|
||||
// offline presence broadcast, and a duplicate hub seq for it.
|
||||
func (h *Hub) handleReconnect(
|
||||
ctx context.Context, conn *websocket.Conn, c *Client, database *db.DB, lastSeq uint64,
|
||||
) bool {
|
||||
) (handled, startPumps bool) {
|
||||
// Channel-visibility changes are delivered as targeted, unsequenced
|
||||
// messages, so replay cannot bring a client that missed one back into a
|
||||
// coherent state — force the full-ready path instead.
|
||||
@@ -127,7 +142,7 @@ func (h *Hub) handleReconnect(
|
||||
"user_id", c.userID, "last_seq", lastSeq)
|
||||
h.reconnectTierFull.Add(1)
|
||||
telemetry.NewAppMetrics().WSReconnectTierTotal.Add(ctx, 1, telemetry.String("tier", "full"))
|
||||
return false
|
||||
return false, false
|
||||
}
|
||||
// Compute the set of channel IDs the reconnecting user can access so that
|
||||
// channel-scoped replay events are filtered by current permissions (M3).
|
||||
@@ -135,7 +150,7 @@ func (h *Hub) handleReconnect(
|
||||
if err != nil {
|
||||
slog.Warn("ws handleReconnect: computeAllowedChannels failed, falling back to full ready",
|
||||
"user_id", c.userID, "err", err)
|
||||
return false
|
||||
return false, false
|
||||
}
|
||||
|
||||
// Voice membership needs only CONNECT_VOICE, not READ_MESSAGES
|
||||
@@ -241,7 +256,7 @@ func (h *Hub) handleReconnect(
|
||||
if events == nil {
|
||||
h.reconnectTierFull.Add(1)
|
||||
telemetry.NewAppMetrics().WSReconnectTierTotal.Add(ctx, 1, telemetry.String("tier", "full"))
|
||||
return false
|
||||
return false, false
|
||||
}
|
||||
}
|
||||
|
||||
@@ -303,7 +318,7 @@ func (h *Hub) handleReconnect(
|
||||
"user_id", c.userID, "last_seq", lastSeq)
|
||||
h.reconnectTierFull.Add(1)
|
||||
telemetry.NewAppMetrics().WSReconnectTierTotal.Add(ctx, 1, telemetry.String("tier", "full"))
|
||||
return false
|
||||
return false, false
|
||||
}
|
||||
events = fresh
|
||||
case "db":
|
||||
@@ -318,7 +333,7 @@ func (h *Hub) handleReconnect(
|
||||
"user_id", c.userID, "max_persisted_seq", maxPersistedSeq)
|
||||
h.reconnectTierFull.Add(1)
|
||||
telemetry.NewAppMetrics().WSReconnectTierTotal.Add(ctx, 1, telemetry.String("tier", "full"))
|
||||
return false
|
||||
return false, false
|
||||
}
|
||||
}
|
||||
h.registerNow(c, allowedChannelIDs)
|
||||
@@ -350,14 +365,17 @@ func (h *Hub) handleReconnect(
|
||||
slog.Warn("ws: failed to send auth_ok (reconnect)", "user_id", c.userID, "err", err)
|
||||
h.unregisterFailedHandshake(ctx, c)
|
||||
_ = conn.Close(websocket.StatusInternalError, "handshake failed")
|
||||
return true
|
||||
// startPumps=false: the teardown above already ran in full. Starting
|
||||
// readPump on this closed conn would hit an immediate Read error and
|
||||
// its defer would run the identical teardown a second time (OC-0051).
|
||||
return true, false
|
||||
}
|
||||
for _, evt := range events {
|
||||
if err := conn.Write(ctx, websocket.MessageText, evt); err != nil {
|
||||
slog.Warn("ws: failed to send replay event", "user_id", c.userID, "err", err)
|
||||
h.unregisterFailedHandshake(ctx, c)
|
||||
_ = conn.Close(websocket.StatusInternalError, "handshake failed")
|
||||
return true
|
||||
return true, false
|
||||
}
|
||||
}
|
||||
slog.Info("ws replay completed", "user_id", c.userID, "events_replayed", len(events), "from_seq", lastSeq, "source", replaySource)
|
||||
@@ -366,7 +384,7 @@ func (h *Hub) handleReconnect(
|
||||
applyConnectStatus(ctx, database, c)
|
||||
h.announceConnectPresence(c)
|
||||
|
||||
return true
|
||||
return true, true
|
||||
}
|
||||
|
||||
// liveVoiceEventsSince returns voice_state/voice_leave events for chID at or
|
||||
@@ -410,10 +428,13 @@ func (h *Hub) liveVoiceEventsSince(ctx context.Context, afterSeq uint64, chID in
|
||||
}
|
||||
|
||||
// unregisterFailedHandshake removes c after a post-registerNow handshake
|
||||
// write failure. No readPump ever starts for this connection, and the old
|
||||
// connection this one replaced already ran its defer (skipping teardown
|
||||
// because this client held the slot) — so when no replacement remains, the
|
||||
// standard disconnect teardown must run here or the user stays online forever.
|
||||
// write failure. No readPump ever starts for this connection — the
|
||||
// fresh-connect callers return an error that stops ServeWS before it starts
|
||||
// the pumps, and handleReconnect's callers report startPumps=false for the
|
||||
// same reason (OC-0051) — and the old connection this one replaced already
|
||||
// ran its defer (skipping teardown because this client held the slot) — so
|
||||
// when no replacement remains, the standard disconnect teardown must run
|
||||
// here or the user stays online forever.
|
||||
func (h *Hub) unregisterFailedHandshake(ctx context.Context, c *Client) {
|
||||
// Snapshot voice state BEFORE unregister, mirroring readPump's defer
|
||||
// (serve_pumps.go): once unregisterNow removes c, there is no way to tell
|
||||
|
||||
@@ -265,13 +265,24 @@ func (h *Hub) buildReady(ctx context.Context, database *db.DB, userID int64, rol
|
||||
slog.Warn("buildReady collectAllVoiceStates", "err", err)
|
||||
allVoiceStates = []db.VoiceState{}
|
||||
}
|
||||
visibleSet := make(map[int64]struct{}, len(visibleChannels)+len(dmChannels))
|
||||
visibleSet := make(map[int64]struct{}, len(visibleChannels)+len(dmChannels)+1)
|
||||
for i := range visibleChannels {
|
||||
visibleSet[visibleChannels[i].ID] = struct{}{}
|
||||
}
|
||||
for i := range dmChannels {
|
||||
visibleSet[dmChannels[i].ChannelID] = struct{}{}
|
||||
}
|
||||
// The caller's own live voice room can never leak by definition -- seed it
|
||||
// even if it fell outside both sets above (e.g. CONNECT_VOICE granted
|
||||
// without READ_MESSAGES, or a DM voice call after the DM was closed:
|
||||
// CloseDM removes dm_open_state but performs no voice eviction). This
|
||||
// mirrors liveVoiceEventsSince's rationale on the reconnect-replay tier
|
||||
// (serve.go), which this full-ready tier had no equivalent for (OC-0028).
|
||||
for i := range allVoiceStates {
|
||||
if allVoiceStates[i].UserID == userID {
|
||||
visibleSet[allVoiceStates[i].ChannelID] = struct{}{}
|
||||
}
|
||||
}
|
||||
voiceStates := make([]db.VoiceState, 0, len(allVoiceStates))
|
||||
for i := range allVoiceStates {
|
||||
if _, ok := visibleSet[allVoiceStates[i].ChannelID]; ok {
|
||||
|
||||
@@ -0,0 +1,77 @@
|
||||
package ws_test
|
||||
|
||||
// serve_ready_own_voice_test.go — regression test for finding OC-0028:
|
||||
// buildReady filters voice_states through visibleSet, which is seeded only
|
||||
// from READ-visible non-DM channels plus the caller's currently-*open* DM
|
||||
// channels (dm_open_state). Voice membership needs only CONNECT_VOICE
|
||||
// (voice_join.go), not READ_MESSAGES nor an open DM row, so a user's own
|
||||
// live voice room can be absent from visibleSet -- the exact hole
|
||||
// liveVoiceEventsSince patches on the reconnect-replay tier (serve.go) but
|
||||
// which buildReady, the full-ready tier, never covered. Concretely: a user
|
||||
// closes a DM from the sidebar while still in that DM's voice call (CloseDM
|
||||
// only removes dm_open_state; it does not evict from voice), and a
|
||||
// subsequent full ready wipes their own voice_state row.
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// TestBuildReady_IncludesOwnVoiceStateAfterDMClosed locks that a user's own
|
||||
// live voice_state row survives buildReady's visibility filter even after
|
||||
// they close the DM the call lives in. Before the fix, closing the DM
|
||||
// dropped the channel from both visibleChannels (DM channels are always
|
||||
// skipped there) and dmChannels (no longer open), so visibleSet had no entry
|
||||
// for the channel and the user's own voice_state was silently filtered out
|
||||
// of their own ready payload.
|
||||
func TestBuildReady_IncludesOwnVoiceStateAfterDMClosed(t *testing.T) {
|
||||
hub, database := newServeHub(t)
|
||||
|
||||
viewer := seedServeUser(t, database, "own-voice-viewer")
|
||||
other := seedServeUser(t, database, "own-voice-other")
|
||||
viewerRole, err := database.GetRoleByID(context.Background(), viewer.RoleID)
|
||||
if err != nil || viewerRole == nil {
|
||||
t.Fatalf("GetRoleByID: %v", err)
|
||||
}
|
||||
|
||||
dmChannelID := seedDMChannel(t, database, viewer.ID, other.ID)
|
||||
|
||||
// Viewer joins the DM's voice channel, then closes the DM from the
|
||||
// sidebar -- mirroring CloseDM's non-group branch, which only removes
|
||||
// the caller's dm_open_state row and performs no voice eviction.
|
||||
if err := database.JoinVoiceChannel(context.Background(), viewer.ID, dmChannelID); err != nil {
|
||||
t.Fatalf("JoinVoiceChannel: %v", err)
|
||||
}
|
||||
if err := database.CloseDM(context.Background(), viewer.ID, dmChannelID); err != nil {
|
||||
t.Fatalf("CloseDM: %v", err)
|
||||
}
|
||||
|
||||
msg, err := hub.BuildReadyWithRoleForTest(database, viewer.ID, viewerRole)
|
||||
if err != nil {
|
||||
t.Fatalf("BuildReadyWithRoleForTest: %v", err)
|
||||
}
|
||||
|
||||
var env struct {
|
||||
Payload struct {
|
||||
VoiceStates []struct {
|
||||
ChannelID int64 `json:"channel_id"`
|
||||
UserID int64 `json:"user_id"`
|
||||
} `json:"voice_states"`
|
||||
} `json:"payload"`
|
||||
}
|
||||
if err := json.Unmarshal(msg, &env); err != nil {
|
||||
t.Fatalf("unmarshal: %v", err)
|
||||
}
|
||||
|
||||
found := false
|
||||
for _, vs := range env.Payload.VoiceStates {
|
||||
if vs.ChannelID == dmChannelID && vs.UserID == viewer.ID {
|
||||
found = true
|
||||
}
|
||||
}
|
||||
if !found {
|
||||
t.Fatalf("ready payload's voice_states is missing the viewer's own live call (channel_id=%d, user_id=%d) after closing the DM; got %+v",
|
||||
dmChannelID, viewer.ID, env.Payload.VoiceStates)
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,130 @@
|
||||
package ws
|
||||
|
||||
// serve_reconnect_double_teardown_test.go — regression test for OC-0051.
|
||||
//
|
||||
// unregisterFailedHandshake's doc comment (serve.go:412-414) claims "No
|
||||
// readPump ever starts for this connection" — true on the fresh-connect
|
||||
// branch (handleFreshConnect returns an error and ServeWS returns without
|
||||
// starting pumps) but false on the reconnect branch: handleReconnect ran the
|
||||
// full unregisterFailedHandshake teardown itself on a handshake-write
|
||||
// failure and then returned true, which ServeWS reads as "success, start the
|
||||
// pumps" (serve.go:69-72). readPump then runs against the already-closed
|
||||
// conn, fails its first Read, and its defer runs the SAME disconnect
|
||||
// teardown a second time — a second MarkUserDisconnected and a second
|
||||
// offline presence broadcast for a connection that was already torn down.
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/coder/websocket"
|
||||
|
||||
"github.com/owncord/server/auth"
|
||||
"github.com/owncord/server/db"
|
||||
)
|
||||
|
||||
// TestHandleReconnect_HandshakeWriteFailure_TearsDownOnlyOnce locks OC-0051:
|
||||
// a failed auth_ok write on the reconnect path must result in exactly one
|
||||
// offline-presence broadcast, however ServeWS reacts to handleReconnect's
|
||||
// return value.
|
||||
func TestHandleReconnect_HandshakeWriteFailure_TearsDownOnlyOnce(t *testing.T) {
|
||||
database, err := db.Open(":memory:")
|
||||
if err != nil {
|
||||
t.Fatalf("db.Open: %v", err)
|
||||
}
|
||||
if err := db.Migrate(database); err != nil {
|
||||
t.Fatalf("Migrate: %v", err)
|
||||
}
|
||||
t.Cleanup(func() { database.Close() })
|
||||
|
||||
ctx := context.Background()
|
||||
userID, err := database.CreateUser(ctx, "reconnect-write-fail", "hash", 1)
|
||||
if err != nil {
|
||||
t.Fatalf("CreateUser: %v", err)
|
||||
}
|
||||
if err := database.UpdateUserStatus(ctx, userID, "online"); err != nil {
|
||||
t.Fatalf("UpdateUserStatus: %v", err)
|
||||
}
|
||||
|
||||
// Run is deliberately not started so h.broadcast can be drained directly,
|
||||
// matching the convention in serve_failed_handshake_teardown_test.go.
|
||||
h := NewHub(database, auth.NewRateLimiter(), nil)
|
||||
|
||||
// Seed the ring buffer so the reconnect replay succeeds from the buffer
|
||||
// tier: lastSeq=2 sits strictly between oldestSeq(1) and newestSeq(3).
|
||||
rb := h.ReplayBuffer()
|
||||
rb.Push(1, 0, []byte(`{"seq":1,"type":"broadcast"}`))
|
||||
rb.Push(2, 0, []byte(`{"seq":2,"type":"broadcast"}`))
|
||||
rb.Push(3, 0, []byte(`{"seq":3,"type":"broadcast"}`))
|
||||
|
||||
c := NewTestClient(h, userID, make(chan []byte, 8))
|
||||
c.user = &db.User{ID: userID, Status: "online"}
|
||||
c.lastSeq = 2
|
||||
|
||||
// A real server-side *websocket.Conn, closed before handleReconnect ever
|
||||
// writes to it — reproducing "peer already gone / write timeout" from the
|
||||
// finding's repro without relying on network timing.
|
||||
connCh := make(chan *websocket.Conn, 1)
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
conn, acceptErr := websocket.Accept(w, r, nil)
|
||||
if acceptErr != nil {
|
||||
return
|
||||
}
|
||||
// CloseNow (not Close) — Close performs a close handshake that blocks
|
||||
// up to 5s waiting for a reply the unread client connection below
|
||||
// never sends. CloseNow just tears down the underlying net.Conn, which
|
||||
// is all this test needs to make the next Write on conn fail.
|
||||
_ = conn.CloseNow()
|
||||
connCh <- conn
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
dialCtx, cancel := context.WithTimeout(ctx, 5*time.Second)
|
||||
defer cancel()
|
||||
clientConn, resp, dialErr := websocket.Dial(dialCtx, "ws"+strings.TrimPrefix(srv.URL, "http"), nil)
|
||||
if resp != nil && resp.Body != nil {
|
||||
_ = resp.Body.Close()
|
||||
}
|
||||
if dialErr != nil {
|
||||
t.Fatalf("dial: %v", dialErr)
|
||||
}
|
||||
defer func() { _ = clientConn.Close(websocket.StatusNormalClosure, "") }()
|
||||
|
||||
var conn *websocket.Conn
|
||||
select {
|
||||
case conn = <-connCh:
|
||||
case <-time.After(5 * time.Second):
|
||||
t.Fatal("server never accepted the connection")
|
||||
}
|
||||
|
||||
// handleReconnect: replay succeeds (buffer tier), but the auth_ok write
|
||||
// fails because conn is already closed — the handshake-write-failure
|
||||
// branch runs and performs the full teardown itself, then reports
|
||||
// startPumps=false so ServeWS (serve.go:69-74) does not start readPump on
|
||||
// top of it. Mirror that gating here: readPump must run only when
|
||||
// startPumps says so.
|
||||
handled, startPumps := h.handleReconnect(ctx, conn, c, database, c.lastSeq)
|
||||
if !handled {
|
||||
t.Fatal("precondition: replay must succeed so the write-failure branch (not the fall-through-to-full-ready branch) runs")
|
||||
}
|
||||
if startPumps {
|
||||
t.Error("handleReconnect reported startPumps=true after a handshake write failure — its own teardown already ran, so ServeWS starting the pumps would run it again")
|
||||
readPump(ctx, conn, h, c)
|
||||
}
|
||||
|
||||
var offlineBroadcasts int
|
||||
for len(h.broadcast) > 0 {
|
||||
bm := <-h.broadcast
|
||||
if bytes.Contains(bm.msg, []byte(`"status":"offline"`)) {
|
||||
offlineBroadcasts++
|
||||
}
|
||||
}
|
||||
if offlineBroadcasts != 1 {
|
||||
t.Errorf("got %d offline presence broadcasts after a failed reconnect handshake write, want exactly 1", offlineBroadcasts)
|
||||
}
|
||||
}
|
||||
+25
-6
@@ -205,7 +205,7 @@ func (h *Hub) handleVoiceJoin(ctx context.Context, c *Client, payload json.RawMe
|
||||
state, err := h.db.GetVoiceState(ctx, c.userID)
|
||||
if err != nil || state == nil {
|
||||
slog.Error("ws handleVoiceJoin GetVoiceState", "err", err, "user_id", c.userID)
|
||||
h.rollbackVoiceJoin(ctx, c, channelID, false)
|
||||
h.rollbackVoiceJoin(ctx, c, channelID, "", false)
|
||||
c.sendMsg(buildErrorMsg(ErrCodeInternal, "failed to join voice channel"))
|
||||
return
|
||||
}
|
||||
@@ -297,7 +297,7 @@ func (h *Hub) handleVoiceJoin(ctx context.Context, c *Client, payload json.RawMe
|
||||
token, tokenErr := h.livekit.GenerateToken(c.userID, c.user.Username, channelID, state.JoinedAt, canPublish, canSubscribe, canVideo, canScreenShare)
|
||||
if tokenErr != nil {
|
||||
slog.Error("ws handleVoiceJoin GenerateToken", "err", tokenErr, "user_id", c.userID)
|
||||
h.rollbackVoiceJoin(ctx, c, channelID, false)
|
||||
h.rollbackVoiceJoin(ctx, c, channelID, state.JoinedAt, false)
|
||||
c.sendMsg(buildErrorMsg(ErrCodeInternal, "failed to generate voice token"))
|
||||
return
|
||||
}
|
||||
@@ -461,7 +461,18 @@ func handleVoiceTokenRefreshV2(ctx context.Context, cmd Command, info ClientInfo
|
||||
// rollbackVoiceJoin undoes a partially-completed voice join: clears the
|
||||
// client's voice channel ID, removes the DB voice state row, and broadcasts
|
||||
// voice_leave so other clients don't see a ghost participant.
|
||||
func (h *Hub) rollbackVoiceJoin(ctx context.Context, c *Client, channelID int64, broadcast bool) {
|
||||
//
|
||||
// joinedAt scopes the compensating delete to the join instance being undone
|
||||
// (mirrors LeaveVoiceChannelIfMatch, used for the same reason by every
|
||||
// sibling leave path). A rollback fires most often because the connection
|
||||
// that started the join just died, and that same cancellation is exactly
|
||||
// what lets a second connection for this user race ahead and establish a
|
||||
// newer, legitimate voice_states row before this rollback runs — an
|
||||
// unconditional "DELETE ... WHERE user_id = ?" would destroy that newer row
|
||||
// instead of the failed one. When joinedAt is empty (the caller never read
|
||||
// the row back far enough to learn it), the row is re-read here and the
|
||||
// delete is skipped unless it still names channelID.
|
||||
func (h *Hub) rollbackVoiceJoin(ctx context.Context, c *Client, channelID int64, joinedAt string, broadcast bool) {
|
||||
c.clearVoiceChID()
|
||||
// The client's voice state is now set before token generation (BUG-088),
|
||||
// so a concurrent join/leave in the same channel can have elected this
|
||||
@@ -472,9 +483,17 @@ func (h *Hub) rollbackVoiceJoin(ctx context.Context, c *Client, channelID int64,
|
||||
h.updateKeyHolder(channelID)
|
||||
// The compensating delete must run even when the join failed BECAUSE the
|
||||
// connection died — that cancellation is the most common rollback trigger.
|
||||
if err := h.db.LeaveVoiceChannel(context.WithoutCancel(ctx), c.userID); err != nil {
|
||||
slog.Error("ws rollbackVoiceJoin LeaveVoiceChannel", "err", err,
|
||||
"user_id", c.userID, "channel_id", channelID)
|
||||
rbCtx := context.WithoutCancel(ctx)
|
||||
if joinedAt == "" {
|
||||
if state, err := h.db.GetVoiceState(rbCtx, c.userID); err == nil && state != nil && state.ChannelID == channelID {
|
||||
joinedAt = state.JoinedAt
|
||||
}
|
||||
}
|
||||
if joinedAt != "" {
|
||||
if _, err := h.db.LeaveVoiceChannelIfMatch(rbCtx, c.userID, channelID, joinedAt); err != nil {
|
||||
slog.Error("ws rollbackVoiceJoin LeaveVoiceChannelIfMatch", "err", err,
|
||||
"user_id", c.userID, "channel_id", channelID)
|
||||
}
|
||||
}
|
||||
if broadcast {
|
||||
h.broadcastVoiceEvent(ctx, channelID, buildVoiceLeave(channelID, c.userID))
|
||||
|
||||
@@ -295,6 +295,12 @@ func handleVoiceModMoveV2(ctx context.Context, cmd Command, info ClientInfo, dep
|
||||
if dest.Type != "voice" {
|
||||
return Result{Error: ClientError{Code: ErrCodeBadRequest, Message: "destination is not a voice channel"}}
|
||||
}
|
||||
// The re-join this move hands off to (handleVoiceJoin) refuses an
|
||||
// archived channel outright; check it here too, or the pre-flight commits
|
||||
// the destructive half of the move for a re-join guaranteed to bounce.
|
||||
if dest.Archived {
|
||||
return Result{Error: ClientError{Code: ErrCodeBadRequest, Message: "channel is archived"}}
|
||||
}
|
||||
// The destination is gated on the TARGET's access, not the moderator's:
|
||||
// a move must not become a way to place someone in a channel they could
|
||||
// not join themselves.
|
||||
|
||||
@@ -530,6 +530,49 @@ func TestVoiceMod_Move_TextChannelDestination_BadRequest(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestVoiceMod_Move_ArchivedDestination_BadRequest locks OC-0072: the move
|
||||
// pre-flight validates destination type, target access, and capacity, but
|
||||
// skipped dest.Archived even though the re-join it hands off to
|
||||
// (handleVoiceJoin) refuses an archived channel outright. Without the gate,
|
||||
// the pre-flight commits the destructive half of the move — the target is
|
||||
// dropped from their current channel — for a re-join guaranteed to bounce,
|
||||
// leaving the target stranded out of voice entirely.
|
||||
func TestVoiceMod_Move_ArchivedDestination_BadRequest(t *testing.T) {
|
||||
hub, database := newVoiceModHub(t)
|
||||
fromID := seedVoiceChan(t, database, "vc-move-archived-from")
|
||||
toID := seedVoiceChan(t, database, "vc-move-archived-to")
|
||||
if err := database.AdminUpdateChannel(context.Background(), toID, db.ChannelUpdate{
|
||||
Name: "vc-move-archived-to",
|
||||
Archived: true,
|
||||
}); err != nil {
|
||||
t.Fatalf("AdminUpdateChannel: %v", err)
|
||||
}
|
||||
actor := seedVoiceUserWithRole(t, database, "admin-move-archived", 2)
|
||||
target := seedVoiceUserWithRole(t, database, "member-move-archived", 4)
|
||||
|
||||
joinVoice(t, hub, target, fromID)
|
||||
|
||||
send := make(chan []byte, 16)
|
||||
c := ws.NewTestClientWithUser(hub, actor, fromID, send)
|
||||
hub.Register(c)
|
||||
waitRegistered(t, hub, c)
|
||||
|
||||
hub.HandleMessageForTest(c, voiceModMoveMsg(target.ID, toID))
|
||||
|
||||
if code := receiveErrorCode(send, waitTimeout); code != "BAD_REQUEST" {
|
||||
t.Fatalf("error code = %q, want BAD_REQUEST", code)
|
||||
}
|
||||
state, err := database.GetVoiceState(context.Background(), target.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetVoiceState: %v", err)
|
||||
}
|
||||
if state == nil {
|
||||
t.Error("a refused move must leave the target in voice")
|
||||
} else if state.ChannelID != fromID {
|
||||
t.Errorf("target channel = %d, want %d (unchanged)", state.ChannelID, fromID)
|
||||
}
|
||||
}
|
||||
|
||||
// TestVoiceMod_Kick_EvictionIsScopedToAuthorizedChannel locks the fix for
|
||||
// v024: voiceModTarget authorizes against a DB snapshot, but the eviction ran
|
||||
// through the unscoped VoiceModerator.DisconnectFromVoice, which drops the
|
||||
|
||||
Reference in New Issue
Block a user