Skip to content

feat: scope file-event triggers to a specific oCIS space - #31

Merged
LukasHirt merged 11 commits into
mainfrom
feature/event-trigger-space-scope
Aug 25, 2026
Merged

feat: scope file-event triggers to a specific oCIS space#31
LukasHirt merged 11 commits into
mainfrom
feature/event-trigger-space-scope

Conversation

@LukasHirt

Copy link
Copy Markdown
Collaborator

Summary

Lets a user optionally restrict a file-event trigger (upload/move/share/lock) to fire only for events originating in one specific oCIS space, instead of matching across every space they have access to.

This closes a real pre-existing bug: event.filters.spaceId was already declared in both the frontend type and the backend's EventFilters model, but was silently dropped before persistence and never matched — setting it via the raw API looked like it worked while doing nothing.

  • Backend: ocisclient.ListDrives proxies oCIS's Graph API (GET /graph/v1.0/me/drives), exposed through a new GET /me/spaces endpoint that filters out driveType == "virtual" (the aggregate "Shares" pseudo-space). SpaceID now correctly persists through the trigger-index storage and is matched in the SSE handler alongside the existing path-prefix/extension filters.
  • Frontend: the event-trigger config panel gets a "Space (optional)" dropdown, populated from the real endpoint, defaulting to "Any space."

Full design rationale: docs/superpowers/specs/2026-07-24-event-trigger-space-scope-design.md
Implementation plan: docs/superpowers/plans/2026-07-24-event-trigger-space-scope.md

Note for anyone with an existing workflow that already had spaceId set via the raw API: it was previously silently ignored (matching all spaces); after this merges, it will start being enforced the next time that workflow is saved. This is the intended fix, not a regression.

Test plan

  • Backend: go build ./... && go vet ./... && go test ./... (from backend/) — all green, including new unit tests for the trigger-index persistence fix and the SSE space-matching logic.
  • Backend e2e (go test -tags=e2e ./tests/e2e/..., real oCIS instance): GET /me/spaces returns real space data, and a real upload correctly fires a workflow scoped to the exact space it landed in — proving the Graph API's drive id and the SSE event's spaceid are the same value in practice, not just in theory.
  • Frontend: pnpm test:unit && pnpm check:types && pnpm lint — all green.
  • Frontend e2e (pnpm test:e2e) — all 4 specs green, including an extended event-trigger.spec.ts that picks a space, saves, reloads, and confirms it persisted.

🤖 Generated with Claude Code

@LukasHirt
LukasHirt requested a review from a team as a code owner July 24, 2026 16:50
@LukasHirt
LukasHirt force-pushed the feature/event-trigger-space-scope branch 2 times, most recently from 6b07942 to e9ec23f Compare July 27, 2026 14:13

@dj4oC dj4oC left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat: scope file-event triggers to a specific oCIS space

Stats: +1938/-17 across 20 files (most of the line count is two docs/plan files; the actual code diff is modest)

Overview

Closes a real, verifiable pre-existing bug: EventFilters.SpaceID/spaceId and the SSE payload's spaceid both already existed in the model before this PR, but syncTriggerIndex never copied SpaceID into the trigger index, and sse/manager.go's filter loop never checked it — so setting it via the raw API silently did nothing. I confirmed this directly against main: model.EventFilters already declares SpaceID string \json:"spaceId,omitempty"`andsse.eventPayloadalready parsesSpaceIDfrom the SSE JSON (and already uses it for path resolution viaItemPath`) — so the "declared but dropped" framing in the PR body checks out, this isn't just the PR's own characterization of its own bug.

What I verified

  • The core claim that Graph's drive id matches the SSE event's spaceid in practice is the one thing that could quietly not work despite compiling fine (format/casing mismatch between the two). The PR backs this with a real e2e test (TestEventTriggeredWorkflowRespectsMatchingSpaceScope) that fetches a real space id via GET /me/spaces, scopes a trigger to it, uploads a real file, and confirms the workflow fires — against a live oCIS instance, not a mock. That's exactly the right way to de-risk this specific claim, and it's a meaningfully more rigorous verification step than some other recent PRs in this batch have done for comparable "does this actually match the real API" questions.
  • Cross-checked driveType: personal against the public libre-graph OpenAPI spec — consistent with what this PR assumes; I couldn't independently corroborate the "virtual" value the same way (it doesn't appear in that spec's static examples), but combined with the PR's own live-instance verification and its dedicated e2e test, I'm not flagging this as a real risk.
  • The SQLite migration (ALTER TABLE ... DEFAULT '' for space_id) follows the exact same pattern already established for path_prefix/extension, so existing rows correctly default to "any space" (no accidental over-restriction of pre-existing triggers on upgrade).
  • Traced the frontend's eventSpaceId computed setter — spreading ...filters before overwriting spaceId correctly preserves pathPrefix/extension when a space is selected/cleared, and setting "" for "Any space" matches the backend's "empty means unfiltered" convention exactly.

Minor observation

WorkflowBuilder.vue's loadSpaces() fails completely silently (empty catch {}, deliberately, per its own comment) if GET /me/spaces errors. I traced through what actually happens to an existing workflow whose trigger already has a specific spaceId set, opened while spaces fail to load: the <select> won't have an <option> for that id, so it'll display as if "Any space" were selected — but since eventSpaceId is a computed with an explicit getter/setter, the underlying data.event.filters.spaceId value isn't actually touched unless the user interacts with the control (Vue doesn't fire the setter just because the visual selection couldn't match). So this isn't silent data loss, just a confusing "your setting looks reset when it isn't" moment if oCIS hiccups at exactly the wrong time — low severity, but a one-line inline error would be more honest than looking identical to "you have no space filter."

Cross-PR note — this is the one worth planning around

NodeDetailsPanel.vue is now being modified by six different currently-open PRs in this batch: #22 (adds required nodes/edges props + a warning banner), #23 (output-hint section), #26 (share/role fields), #27 (condition fields, plus hides the legacy condition field on condition nodes), #29 (extractText output-variable field), and now #31 (the space dropdown). That's a meaningfully higher collision surface than the usual two-PRs-touch-the-same-file case I've flagged elsewhere in this batch — six independent sets of new template blocks and new computeds all landing in roughly the same few dozen lines. Worth specifically noting: #22 adds nodes/edges as required (non-optional) props, which means every other PR's test file that mounts NodeDetailsPanel with just { node } (or { node, spaces: [] } as this PR's own test does) will need updating once #22 merges — so merging #22 first, then rebasing the rest, would avoid the most disruptive part of an eventual six-way reconciliation.

Code quality & style

  • SpacesHandler/DriveLister follows the same thin-interface, dependency-injected-for-testing pattern already used by WorkflowsHandler/TriggerIndexer — consistent, no new pattern introduced.
  • Explicitly calling out in the PR body that pre-existing workflows with a raw-API-set spaceId will go from "silently ignored" to "enforced" on next save is exactly the right thing to surface before merge — a real behavior change for anyone who used the undocumented field, called out rather than left for someone to discover the hard way.
  • The large embedded plan/design docs (docs/superpowers/plans/..., docs/superpowers/specs/...) account for the bulk of this diff's line count but aren't code — didn't review them as implementation, just confirmed the actual code matches what they describe.

Test coverage

Strong and appropriately layered: unit tests for the new SSE space-matching (TestHandleEventSkipsNonMatchingSpace/TestHandleEventMatchesSpecificSpace) cover the matching logic in isolation with fakes, while the e2e suite (both backend and frontend) proves the real id-format compatibility question and the UI's persist-reload round-trip. The one gap — no e2e case uploading to a different space and confirming the trigger does not fire — is reasonably explained by the dev stack only having one real space per test user; the unit test already covers that negative case with fakes, so this isn't a meaningful hole.

Summary

Solid bug fix, well-verified against the one claim that mattered most (Graph drive id ≡ SSE spaceid), with good migration hygiene and an honest callout of the behavior change for existing raw-API users. Nothing here blocks merge; the NodeDetailsPanel.vue six-way collision is worth a quick look at merge order across this batch, not a fix to this PR itself.


🤖 Generated with Claude Code

@LukasHirt
LukasHirt force-pushed the feature/event-trigger-space-scope branch from e9ec23f to 39079e5 Compare July 29, 2026 08:33
@LukasHirt LukasHirt self-assigned this Jul 29, 2026
@dj4oC
dj4oC self-requested a review July 29, 2026 08:50
@LukasHirt
LukasHirt force-pushed the feature/event-trigger-space-scope branch from 39079e5 to f0e1b11 Compare July 29, 2026 09:21
LukasHirt and others added 11 commits August 20, 2026 17:24
Wires up the existing but dead event.filters.spaceId field end to
end: a new /me/spaces endpoint backed by oCIS's Graph API, persisting
SpaceID through the trigger index, matching it in the SSE handler,
and a space picker in the trigger config UI.

Signed-off-by: Lukas Hirt <info@hirt.cz>
Eight-task plan: oCIS client + API model, the /me/spaces endpoint,
persisting SpaceID through the trigger index (closing an existing
bug where it's silently dropped), SSE-handler matching, backend and
frontend e2e coverage, and the space picker UI, per the approved
design spec.

Signed-off-by: Lukas Hirt <info@hirt.cz>
Signed-off-by: Lukas Hirt <info@hirt.cz>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Lukas Hirt <info@hirt.cz>
Signed-off-by: Lukas Hirt <info@hirt.cz>
… triggers

Signed-off-by: Lukas Hirt <info@hirt.cz>
Signed-off-by: Lukas Hirt <info@hirt.cz>
The output-hint test helper for NodeDetailsPanel was introduced on
main after this branch added the required spaces prop to the
component. Rebasing merged both changes textually without conflict,
leaving the helper's mount call out of sync and failing type-check.

Signed-off-by: Lukas Hirt <info@hirt.cz>
@LukasHirt
LukasHirt force-pushed the feature/event-trigger-space-scope branch from f0e1b11 to ee5c31e Compare August 20, 2026 15:27
@mzner
mzner self-requested a review August 25, 2026 14:35
@LukasHirt
LukasHirt merged commit c915943 into main Aug 25, 2026
5 checks passed
@LukasHirt
LukasHirt deleted the feature/event-trigger-space-scope branch August 25, 2026 14:35
LukasHirt added a commit that referenced this pull request Aug 25, 2026
Derives drive scope from the trigger index (reusing PR #31's SpaceID),
queries activitylog per drive since a persisted cursor, maps messages
to trigger types, and dispatches matching workflows using the same
MatchesFilters/IsInternalPath logic sse.Manager already applies to
live events. Bounded by a bare-metal semaphore so a fleet-wide SSE
reconnect can't fire unbounded concurrent requests.

Deviates from the task brief's sample splitResourceID/dispatch in two
spots, both required to satisfy the brief's own test expectations:
splitResourceID returns the opaque id after the last "!" as itemID
(not the whole compound string), and dispatch uses the already-known
driveID parameter as the space scope for ItemPath/MatchesFilters
rather than re-deriving a differently-formatted value (with a storage
provider prefix) from the activity's own resourceId.

Signed-off-by: Lukas Hirt <info@hirt.cz>
LukasHirt added a commit that referenced this pull request Aug 25, 2026
#44)

* docs: add design spec for event-trigger reliability backstop

Event triggers currently rely solely on a live SSE connection, which
oCIS deliberately makes non-durable — any event that fires while the
connection is down is silently lost forever, with no error or retry.
This spec proposes an activitylog-based reconciliation backstop,
triggered on SSE reconnect and scoped via the trigger index, after
investigating and ruling out an oCIS Graph delta query (doesn't
exist), hardening SSE itself (upstream has explicitly declined to
support replay), and direct NATS consumption (requires exposing an
unauthenticated bus or a much larger deployment commitment).

Signed-off-by: Lukas Hirt <info@hirt.cz>

* docs: add implementation plan for event-trigger reliability backstop

Nine-task plan implementing the design in
docs/superpowers/specs/2026-08-20-event-trigger-reliability-design.md:
shared filter-matching helper, event-cursor storage, an activitylog
client, message-to-trigger mapping, the Reconciler core, wiring it
into sse.Manager's reconnect path, surfacing reliability on
GET /me/automation, server wiring, and an e2e test that deliberately
races the original bug's exact failure window.

Signed-off-by: Lukas Hirt <info@hirt.cz>

* refactor(backend): extract TriggerIndexEntry.MatchesFilters

Shared by sse.Manager today and reconcile.Reconciler in a later commit,
so the two event paths can't silently drift apart on filter semantics.

Signed-off-by: Lukas Hirt <info@hirt.cz>

* feat(backend): add event_cursors table for reconciliation state

Per-(user, drive) cursor tracking the last time this backend checked
oCIS's activitylog for events the SSE connection may have missed, plus
whether that check itself succeeded — the basis for the reliability
field added to GET /me/automation in a later commit.

Signed-off-by: Lukas Hirt <info@hirt.cz>

* feat(backend): add ListActivities oCIS activitylog client

Parsing pinned against a real captured response (upload + rename)
from a live oCIS instance, including the empty resource.id case
observed on a freshly-added file.

Signed-off-by: Lukas Hirt <info@hirt.cz>

* fix(backend): use proper percent-encoding for ListActivities query

Changed from hand-rolled space-only encoding to url.Values.Encode() for
proper RFC 3986 compliance, matching the package's established convention
(driveitem.go uses url.PathEscape for compound storageid$spaceid IDs).

Updated test to verify decoded query parameters instead of raw RawQuery
string, since percent-encoded characters are decoded when parsed.

Added TestListActivitiesWithCompoundDriveID to verify the encode/decode
round-trip preserves compound driveIDs containing $ and ! characters.

Signed-off-by: Lukas Hirt <info@hirt.cz>

* feat(backend): add activitylog message-to-trigger-type mapping

Signed-off-by: Lukas Hirt <info@hirt.cz>

* feat(backend): add Reconciler core (drive scope, query, dispatch)

Derives drive scope from the trigger index (reusing PR #31's SpaceID),
queries activitylog per drive since a persisted cursor, maps messages
to trigger types, and dispatches matching workflows using the same
MatchesFilters/IsInternalPath logic sse.Manager already applies to
live events. Bounded by a bare-metal semaphore so a fleet-wide SSE
reconnect can't fire unbounded concurrent requests.

Deviates from the task brief's sample splitResourceID/dispatch in two
spots, both required to satisfy the brief's own test expectations:
splitResourceID returns the opaque id after the last "!" as itemID
(not the whole compound string), and dispatch uses the already-known
driveID parameter as the space scope for ItemPath/MatchesFilters
rather than re-deriving a differently-formatted value (with a storage
provider prefix) from the activity's own resourceId.

Signed-off-by: Lukas Hirt <info@hirt.cz>

* fix(backend): pass full compound resourceID as ItemPath's itemID

splitResourceID previously truncated itemID to the bare opaque suffix
after the last "!". Verified live against a running oCIS dev stack:
Graph's /drives/{spaceID}/items/{itemID} route 404s (itemNotFound) on
the bare opaque suffix and only resolves with the full compound
"storageid$spaceid!opaqueid" id. Restore the full string as itemID and
update the tests (TestSplitResourceID and the fakePathResolver-keyed
dispatch tests) to match that verified behavior instead of the
previously-assumed truncated shape.

Signed-off-by: Lukas Hirt <info@hirt.cz>

* fix(backend): stop permanent repeat-dispatch, honor ctx on semaphore wait

Two Important findings from review, both traced to the plan's own Step 3
reference code:

1. reconcileDrive advanced the cursor to the latest dispatched activity's
   own RecordedTime instead of wall-clock now. On a drive that goes quiet
   after one upload, the cursor got stuck at that upload's timestamp
   forever: since = cursor - overlap never moved, so the same activity
   fell inside the query window (and got redispatched) on every single
   future reconnect, unboundedly. Advance to now (captured at the start
   of the pass) instead, so each successful pass moves the cursor forward
   regardless of what it found, and drop the now-unneeded `latest`
   tracking. TestReconcileDoesNotRepeatDispatchOnSubsequentPass proves
   this: verified it fails against the reverted (latest-based) cursor
   logic and passes against the fix. It also gives fakeActivityLister an
   opt-in since-filter (off by default, so existing tests are
   unaffected) modeling oCIS's real server-side "timestamp>since"
   filtering — without it no test can distinguish the two cursor
   strategies, since since is a pure function of the stored cursor value,
   independent of elapsed wall-clock time.

2. Reconcile's semaphore acquisition (`r.sem <- struct{}{}`) ignored ctx
   cancellation. A pass queued behind the semaphore whose ctx got
   cancelled before a slot freed (e.g. caller's SSE consumer shut down)
   would still run later against a dead context, fail ListActivities,
   and get its cursor wrongly marked "sse-only" even though nothing was
   actually wrong with activitylog. Now a select on ctx.Done() lets a
   cancelled-before-acquiring call bail out without acquiring a slot or
   touching any cursor, and the ListActivities error path checks
   ctx.Err() before writing the degraded status, skipping it when the
   failure is just our own cancellation.

Deferred (out of scope for this fix, per review triage): dead spaceID
return from splitResourceID, errors.Is vs direct ErrNotFound comparison,
N+1 query in dispatch, synchronous workflow execution inside the
semaphore, minor test cleanup.

Signed-off-by: Lukas Hirt <info@hirt.cz>

* feat(backend): trigger reconciliation on SSE (re)connect

streamOnce now calls an onConnected hook once a connection is
established (200 OK, before the read loop), which consumeForUser
wires to reconcile.Reconciler.Reconcile - closing the exact race
from the original bug report, where an upload landing in the gap
before a fresh SSE consumer finishes connecting was silently lost.

sse.Reconciler is a small local interface (matching this package's
existing style) rather than an import of pkg/reconcile, to avoid any
future import-cycle risk between the two packages.

Note: pkg/command/server.go still calls sse.New with the old
signature and pkg/command no longer builds until that call site is
updated - that wiring is Task 8's job per the plan, not this one.

Signed-off-by: Lukas Hirt <info@hirt.cz>

* feat(backend): surface event-trigger reliability on GET /me/automation

Backend contract only — frontend surfacing of the new field is a
follow-up, not part of this change (see spec's Risks/open questions).

Signed-off-by: Lukas Hirt <info@hirt.cz>

* feat(backend): wire the reconciliation backstop into the running server

Signed-off-by: Lukas Hirt <info@hirt.cz>

* test(backend): prove the reconciliation backstop recovers a missed upload

Uploads immediately after creating an event trigger, deliberately not
waiting for the SSE manager to open its connection first — the exact
race from the original bug report — and asserts the workflow still
fires once reconciliation runs on the eventual (re)connect.

Signed-off-by: Lukas Hirt <info@hirt.cz>

* fix(backend): recover missed events on a drive's very first reconciliation

reconcileDrive previously seeded a brand-new (user, drive) pair's cursor
at "now" and returned without ever querying activitylog — but that's
precisely the scenario this backstop exists for: a just-created event
trigger whose upload races the very first SSE connection. Caught by
Task 9's e2e work: TestEventTriggeredWorkflowRecoversFromMissedSSEEvent
only passed because an earlier test in the same suite happened to warm
admin's drive cursor as a side effect; run alone against a fresh
database it failed.

On ErrNotFound, synthesize a starting cursor firstConnectLookback (5m
default) in the past and fall through to the existing query/dispatch/
advance path, instead of a separate skip-the-query branch — bounded so
a first-ever check on an old, busy drive doesn't flood-dispatch its
whole history, just the genuine just-created-trigger window.

Replaces TestReconcileFirstEverCallSeedsCursorWithoutBackfill (which
asserted the bug: 0 activitylog calls on first-ever) with two tests:
one proving a first-ever pass dispatches an activity inside the
lookback window, one proving it excludes one recorded before the
lookback boundary (using a short lookback + the fakeActivityLister's
opt-in since-filter to prove the bound is real, not unbounded).
Verified both fail against the reverted skip-the-query branch and pass
against the fix.

New Reconciler tunable firstConnectLookback (New(...)'s new parameter,
inserted after overlap); server.go wires it to a 5-minute default,
generous relative to realistic SSE reconnect delays but far short of
activitylog's retention.

Signed-off-by: Lukas Hirt <info@hirt.cz>

* docs: correct first-ever-connect reasoning in the design spec

The original text treated a brand-new (user, drive) pair as "nothing
to backfill" — but that's exactly the original bug's scenario, and
the implementation now looks back a bounded window instead. Update
the spec to match the fix (commit 748f407), found via an e2e test
during Task 9.

Signed-off-by: Lukas Hirt <info@hirt.cz>

* docs: correct spec's cursor-advancement and dedup reasoning

Section 4 still described the pre-fix "advance to latest recordedTime"
cursor semantics (the permanent-repeat-dispatch bug) as intended
behavior, and its "rare double-fire" cross-path dedup reasoning didn't
account for the cursor only ever advancing during reconciliation, never
while SSE is healthy — meaning a long-lived connection's eventual
reconnect could otherwise replay its entire uptime. Documents the
lookback clamp fix (final whole-branch review finding) and the
periodic-refresh-while-healthy gap it leaves as a follow-up.

Signed-off-by: Lukas Hirt <info@hirt.cz>

* fix(backend): bound unbounded lookback, harden reconciliation edge cases

Final whole-branch review found one Critical and several Important
issues across pkg/reconcile, pkg/sse, pkg/automation, and pkg/localdb.
All bundled into this one fix wave per the review process (one dispatch,
one scoped re-review):

1. CRITICAL: clamp the activitylog query floor (since) to never look
   back further than firstConnectLookback from now. UpsertEventCursor
   is only ever called from reconcileDrive, so a long-lived healthy SSE
   connection (normal operation — nothing in pkg/sse ever touches the
   cursor while a connection stays up), a backend restart after
   extended downtime, or recovery after an extended activitylog outage
   would each compute an unbounded since and redispatch everything SSE
   already delivered live across that whole window, with no cross-path
   dedup by design. Reuses firstConnectLookback rather than adding a
   new tunable. Doesn't close the gap for a connection healthy longer
   than that window — the proper fix (periodic cursor refresh while
   healthy) is a bigger sse read-loop change, deferred per the spec.

2. Track first-ever-ness explicitly (firstEver bool) instead of
   implicitly relying on firstConnectLookback exceeding gracePeriod for
   the debounce check to behave correctly — an unreachable-today but
   silent-forever failure mode if that relationship were ever inverted
   by a config change.

3. Add localdb.DeleteEventCursors (mirrors DeleteAutomation), called
   from automation.Service.Disconnect — a cursor marked "sse-only" that
   then falls out of scope (disconnect, workflow deleted, trigger
   narrowed) used to report degraded reliability forever with no way
   to clear it, and rows just accumulated indefinitely.

4. Add a process-local per-user pass-start debounce at the top of
   Reconcile, ahead of drivesForUser — a flapping SSE connection with
   an unscoped trigger was firing a real GET /me/drives on every single
   reconnect attempt, regardless of the per-drive grace period one
   level down. Rate-limiting only; the durable per-drive cursor is
   still what prevents duplicate dispatch.

5. Wrap the reconciliation goroutine in sse.Manager.consumeForUser's
   onConnected closure with a deferred recover — a panic anywhere
   inside Reconcile used to crash the whole process instead of failing
   one background pass.

6. Fetch ListEventTriggers once per Reconcile pass and thread it
   through drivesForUser/reconcileDrive/dispatch instead of dispatch
   independently re-querying it once per activity (N+1).

7. Minor: drop a duplicated doc-comment block in localdb_test.go;
   correct splitResourceID's doc comment (the parsed spaceID isn't
   wrong-format, driveID is just already-authoritative and redundant
   with it); use errors.Is instead of direct ErrNotFound comparison in
   reconcileDrive, matching this file's own convention elsewhere.

Every new/changed assertion was verified RED against a temporarily
reverted version of its fix and GREEN against the fix, including the
process-crashing case for the panic-recovery test (run in isolation,
confirmed it takes down the whole test binary without the recover).

Explicitly out of scope per the controller's ruling: synchronous
workflow execution inside the semaphore stays as-is (bounds total
concurrent work, cost is bounded not unbounded); no cross-path
SSE/reconciliation dedup; no periodic cursor-refresh-while-healthy; no
change to SSE-side resolvedPath empty-string handling.

Signed-off-by: Lukas Hirt <info@hirt.cz>

---------

Signed-off-by: Lukas Hirt <info@hirt.cz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants