feat: scope file-event triggers to a specific oCIS space - #31
Conversation
6b07942 to
e9ec23f
Compare
dj4oC
left a comment
There was a problem hiding this comment.
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
idmatches the SSE event'sspaceidin 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 viaGET /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: personalagainst 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 ''forspace_id) follows the exact same pattern already established forpath_prefix/extension, so existing rows correctly default to "any space" (no accidental over-restriction of pre-existing triggers on upgrade). - Traced the frontend's
eventSpaceIdcomputed setter — spreading...filtersbefore overwritingspaceIdcorrectly preservespathPrefix/extensionwhen 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/DriveListerfollows the same thin-interface, dependency-injected-for-testing pattern already used byWorkflowsHandler/TriggerIndexer— consistent, no new pattern introduced.- Explicitly calling out in the PR body that pre-existing workflows with a raw-API-set
spaceIdwill 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
e9ec23f to
39079e5
Compare
39079e5 to
f0e1b11
Compare
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>
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>
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>
f0e1b11 to
ee5c31e
Compare
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>
#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>
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.spaceIdwas already declared in both the frontend type and the backend'sEventFiltersmodel, but was silently dropped before persistence and never matched — setting it via the raw API looked like it worked while doing nothing.ocisclient.ListDrivesproxies oCIS's Graph API (GET /graph/v1.0/me/drives), exposed through a newGET /me/spacesendpoint that filters outdriveType == "virtual"(the aggregate "Shares" pseudo-space).SpaceIDnow correctly persists through the trigger-index storage and is matched in the SSE handler alongside the existing path-prefix/extension filters.Full design rationale:
docs/superpowers/specs/2026-07-24-event-trigger-space-scope-design.mdImplementation plan:
docs/superpowers/plans/2026-07-24-event-trigger-space-scope.mdNote for anyone with an existing workflow that already had
spaceIdset 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
go build ./... && go vet ./... && go test ./...(frombackend/) — all green, including new unit tests for the trigger-index persistence fix and the SSE space-matching logic.go test -tags=e2e ./tests/e2e/..., real oCIS instance):GET /me/spacesreturns 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'sspaceidare the same value in practice, not just in theory.pnpm test:unit && pnpm check:types && pnpm lint— all green.pnpm test:e2e) — all 4 specs green, including an extendedevent-trigger.spec.tsthat picks a space, saves, reloads, and confirms it persisted.🤖 Generated with Claude Code