* fix(security): scope DELETE/UpdateIssueStatus by workspace_id
Add workspace_id to the WHERE clause of DeleteIssue, DeleteComment,
DeleteProject, DeleteSkill, DeleteChatSession, and UpdateIssueStatus
as SQL-layer defense-in-depth.
Handler loaders (loadIssueForUser / loadSkillForUser / etc.) already
enforce workspace membership today, so this is not patching a known
live vuln. But the tenant invariant is currently a handler-layer
guarantee — a future loader bypass or a new caller skipping the
loader would be silently catastrophic. Making workspace_id part of
the SQL identity collapses the trust surface to the schema itself:
forging a sibling-workspace UUID becomes ErrNoRows instead of a
cross-tenant write.
Reference: incident #1661 (util.ParseUUID silent zero UUID returning
204 on a DELETE that matched zero rows) — same class of failure,
prevented at a different layer.
Scope:
- 5 DELETE queries: issue, comment, project, skill, chat_session
- 1 simple UPDATE: UpdateIssueStatus (2 narg, no SET ordering risk)
- All callers updated (handlers, service, runtime sweeper fallback)
Multi-narg UPDATE queries (UpdateIssue, UpdateProject, UpdateSkill,
UpdateComment, UpdateChatSession*) are deferred to a follow-up to
keep this change reviewable: each needs its narg pinning shifted
and per-caller verification.
sqlc was regenerated by hand (no local sqlc toolchain); CI's
backend job is the authoritative compile check.
* test(security): add workspace_scope_guard regression test
Locks in the SQL-layer tenant guard added in this PR. For each of the 6
scoped queries (DeleteIssue, DeleteComment, DeleteProject, DeleteSkill,
DeleteChatSession, UpdateIssueStatus), creates the resource in workspace
A, invokes the query with a foreign workspace UUID, and asserts the row
is untouched (0 rows affected with no error for :exec; pgx.ErrNoRows for
:one). A future refactor that drops the workspace_id arg from any of
these queries will now fail loudly instead of silently regressing.
Includes a sanity sub-test that the in-workspace path still mutates, so
a buggy guard that returns no-op for every call would not pass.
Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
---------
Co-authored-by: Tom Qiao <tomqiaozc@users.noreply.github.com>
Co-authored-by: Claude Opus 4 <noreply@anthropic.com>
- Multi-select UI for batch importing skills from a local runtime
- Server batch-dispatches up to 10 import requests per heartbeat cycle
- WS heartbeat now reads supports_batch_import from daemon payload
instead of hardcoding true, so old daemons correctly fall back to
one-at-a-time dispatch
- Raised server pending timeout to 3min and client poll timeout to 4min
to accommodate daemons that pop only one import per 15s heartbeat
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor(server): make ParseUUID error-returning to prevent silent data loss (MUL-1410)
util.ParseUUID previously swallowed errors and returned a zero pgtype.UUID
on invalid input. When this zero UUID reached a write query (DELETE/UPDATE),
the SQL matched zero rows and the handler returned 2xx success — producing
silent data corruption. #1661 (DeleteIssue with identifier-style ID) was the
visible symptom; PR #1680 patched that one site, this commit closes the
class of bug.
Changes:
- util.ParseUUID now returns (pgtype.UUID, error). Add util.MustParseUUID
for trusted round-trips that should panic on invalid input.
- handler/handler.go: parseUUID wrapper now calls MustParseUUID — any
unguarded user-input string reaching it surfaces as a recovered panic
(chi middleware.Recoverer → 500) instead of silently corrupting data.
Add parseUUIDOrBadRequest(w, s, fieldName) for handler entry points.
- Convert every Queries.Delete*/Update* call site reachable from raw user
input (autopilot, comment, project, skill, skill_file, label, pin,
attachment, feedback, issue assignee, daemon runtime, workspace) to
validate UUIDs explicitly with parseUUIDOrBadRequest, returning 400 on
invalid input. Where a resolved entity.ID is already in scope, write
queries now use it directly instead of re-parsing the URL string.
- Update getWorkspaceMember + loadIssueForUser to handle invalid UUIDs
gracefully (404/400 instead of panic).
- Update util/middleware/cmd-level callers (subscriber_listeners,
notification_listeners, activity_listeners, scope_authorizer,
middleware/workspace) to use the error-returning API.
- Add server/internal/util/pgx_test.go covering valid/invalid input and
the MustParseUUID panic contract.
- Add TestDeleteIssueByIdentifier + TestDeleteIssueRejectsInvalidUUID
regression tests in handler_test.go (the original #1661 bug + the
invalid-input case).
- Document the handler UUID parsing convention in CLAUDE.md so the rule
is enforceable in future PR review.
* fix(server): address GPT-Boy review of #1748
P1 fixes from PR #1748 review:
1. Migrate remaining request-boundary UUIDs to parseUUIDOrBadRequest so
malformed input returns 400 instead of panic/500. Was missing on:
- issue.go: workspace_id in CreateIssue/ChildIssueProgress/ListIssues/
SearchIssues/BatchUpdateIssues/BatchDeleteIssues; project_id /
parent_issue_id / lead_id / assignee_id / assignee_ids / creator_id
filters; batch issue_ids and assignee/parent/project fields in
BatchUpdateIssues (skip on bad input via util.ParseUUID, matching
the existing per-row continue semantics).
- project.go: project id + workspace_id in GetProject/UpdateProject/
DeleteProject; lead_id in CreateProject/UpdateProject;
workspace_id in ListProjects + SearchProjects.
- handler.go: resolveActor now uses util.ParseUUID for X-Agent-ID /
X-Task-ID headers; invalid UUID falls back to "member" (matches
pre-existing semantics) instead of panicking.
- issue.go: validateAssigneePair returns 400 on invalid workspace_id
instead of panicking.
2. Fix issue:deleted WS event payloads to emit uuidToString(issue.ID)
instead of the raw URL string. After an identifier-path delete
("MUL-7"), the previous payload would have leaked the identifier to
subscribers, leaving stale entries in frontend caches that key by
UUID. Updated DeleteIssue (issue.go:1341) and BatchDeleteIssues
(issue.go:1641). The slog "issue deleted" log line also now records
the resolved UUID so logs match the WS payload.
3. Extend TestDeleteIssueByIdentifier to subscribe to the bus and
assert issue:deleted.payload.issue_id is the resolved UUID, not
the identifier.
* fix(server): validate remaining reviewed UUID inputs
* fix(server): validate remaining handler UUID inputs
* fix(server): finish request boundary UUID audit
* fix(server): validate remaining request body UUIDs
* fix(server): validate runtime path UUIDs
* fix(server): validate remaining audit UUID inputs
---------
Co-authored-by: Eve <eve@multica.ai>
Mitigates #1637 and the related model-discovery failure in MUL-1397 by bounding the /api/daemon/heartbeat hot path with an ack-safe probe/claim split, adding structured slow-log attribution, and closing the ModelListStore running-state gap. See PR description for details.
* fix(skills): shared-state runtime local-skill stores (MUL-1288)
Fixes the bug Bohan surfaced on MUL-1288: behind prod's multi-node API the
runtime-local-skill list/import flow would intermittently time out or 404.
Root cause: LocalSkillListStore and LocalSkillImportStore were per-process
sync.Mutex+map, so when the frontend POST, the daemon heartbeat and the
frontend GET landed on different API instances, each saw a different
pending set. Confirmed against production daemon logs — the failed
request_id never showed up in the daemon's "runtime local skills
requested" log, even though other requests around the same window worked.
Per Yushen's guidance (server must stay stateless; state lives in
storage), migrate both stores to Redis so every node agrees on the same
pending set.
What changed
- LocalSkillListStore / LocalSkillImportStore are now interfaces. Methods
take context.Context and return error.
- InMemoryLocalSkill{List,Import}Store — renamed from the existing types,
kept as the default for single-node dev and the in-process test suite.
- RedisLocalSkill{List,Import}Store — new. Keyed on
mul:local_skill:{list,import}:<id> (JSON record, TTL = retention), with
a per-runtime ZSET mul:local_skill:{list,import}:pending:<runtime_id>
(score = created_at UnixNano) providing cross-node ordering. PopPending
wins the claim via ZREM == 1, so concurrent pops from different nodes
never return the same request twice.
- NewRouter gets an optional *redis.Client; when non-nil it swaps in the
Redis-backed stores. main.go hoists the existing Redis client (already
used by the realtime relay) so both subsystems share one client.
- Handler fields flip to interface types; handler.New still constructs
in-memory stores by default.
- Daemon heartbeat's PopPending call sites thread r.Context() through so
Redis operations inherit request cancellation. Errors warn instead of
poisoning the heartbeat response.
Tests
- Existing in-memory tests updated for the new signatures (ctx + error).
- New runtime_local_skills_redis_store_test.go covers:
- Create/Get/Complete round trip preserves skills payload
- PopPending across two *store instances sharing one rdb (the exact
regression: node A creates, node B pops)
- N concurrent PopPending on one record => exactly one winner
- Pending-timeout threshold transitions the record and removes the zset
member so a later PopPending doesn't return a timed-out request
- Import store round-trips CreatorID (which is json:"-" on the public
struct — needs a Redis envelope so ReportLocalSkillImportResult can
still attribute the created Skill)
- Per-runtime isolation — a PopPending for runtime B does not disturb
A's pending zset
- Tests skip gracefully if REDIS_TEST_URL is unset; CI now spins up a
redis:7-alpine service and exports the URL so the suite actually runs
there.
Out of scope
PingStore / UpdateStore / ModelListStore have the same shape and the
same latent bug (they just fire rarely enough to have gone unnoticed).
Migrating them to Redis is a follow-up — MUL-1288 is specifically the
local-skills break Bohan is blocked on.
* fix(skills): atomic Redis claim + surface store write failures (PR #1557 review)
Two real gaps GPT-Boy flagged:
1. RedisLocalSkill{List,Import}Store.PopPending was doing ZREM then SET as
two separate round-trips. If the SET failed for any reason — transient
Redis error, context cancellation, pod getting SIGKILL'd mid-call — the
request was already gone from the pending zset but the stored record
still said "pending", and no subsequent PopPending would re-dispatch
it. Exactly the "request disappears" class of bug this PR is supposed
to kill.
Fix: push the claim into a Lua script so Redis runs ZREM + SET as one
atomic unit. If ZREM returns 0 (another node won the race), SET is
skipped and the caller retries.
2. ReportLocalSkill{List,Import}Result handlers were logging Complete/Fail
store failures at Warn and still returning 200 OK. That made the
daemon think the report landed when it hadn't, leaving the request
stuck in "running" until the server-side timeout and — worse for the
import flow — leaving the just-created Skill row orphaned in Postgres
so every retry collided with the unique-name constraint.
Fix: escalate to Error + return 500 so the daemon (and monitoring) can
see the write failed. For the import flow, Complete failure after the
Skill row is already committed also triggers a best-effort DeleteSkill
so a daemon retry lands on a clean slate instead of hitting
"a skill with this name already exists" forever.
Tests
- New TestRedisLocalSkillListStore_PopPendingAtomicClaim asserts the
happy-path invariant: after one PopPending the record is "running"
AND a second PopPending returns nothing. Deliberately does NOT poke
Redis internals directly so the test survives any future key-layout
refactor.
- Existing cross-instance / concurrent / timeout / per-runtime tests
continue to pass against the Lua-based claim path (verified locally
against a scratch redis-server; 8/8 Redis tests green).
