Skip to content

fix(cue): wire toggle + activity through main, drop dead preload bridges - #983

Merged
pedramamini merged 2 commits into
RunMaestro:rcfrom
chr1syy:fix/cue-remote-callbacks-cleanup
May 13, 2026
Merged

fix(cue): wire toggle + activity through main, drop dead preload bridges#983
pedramamini merged 2 commits into
RunMaestro:rcfrom
chr1syy:fix/cue-remote-callbacks-cleanup

Conversation

@chr1syy

@chr1syy chr1syy commented May 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #982 (`fix/cue-remote-subscriptions`). Same dead-bridge shape existed for two more cue IPC channels — fixing them and removing the unused preload bridges so the cue control surface is consistent.

Stacked on #982. If #982 is merged first, this PR rebases to one commit. If reviewed together, both can land in one rc.

What was broken

The web-server factory forwarded `remote:toggleCueSubscription` and `remote:getCueActivity` to the renderer and waited 10 s / 30 s for a response on IPC channels no renderer code had ever subscribed to. Net effect:

  • Per-row Cue subscription toggles in the web UI silently no-op'd after a 10 s stall.
  • The web UI activity tab always rendered empty after a 30 s stall.

Fix

Mirror the `triggerCueSubscription` / `getCueGraphData` pattern from #982. Two new deps on `WebServerFactoryDependencies`:

  • `setCueSubscriptionEnabled(subscriptionId, enabled): boolean`
  • `getCueActivityLog(): CueRunResult[]`

Wired in `main/index.ts` from `cueEngine`. The toggle needs an engine-side mutation, so this PR adds `CueEngine.setSubscriptionEnabled`, which parses the `${sessionId}::${name}` id format (same format `getCueGraphData`'s flattener emits in #982), looks up the owning session, reads → parses → mutates → writes the YAML, and calls `refreshSession`. Comments and field ordering aren't preserved on the round-trip — same trade-off as the pipeline editor's save path.

Activity mapping converts engine `CueRunResult[]` → web `CueActivityEntry[]`, applies the optional `sessionId` filter before slicing to `limit` (so the caller gets N matching entries, not N total of which some are filtered), and collapses engine-side `timeout` / `stopped` into the web-facing `failed` enum.

Dead-code cleanup

Removed the six matching preload bridges from `src/main/preload/process.ts`:

  • `onRemoteGetCueSubscriptions` / `sendRemoteGetCueSubscriptionsResponse`
  • `onRemoteToggleCueSubscription` / `sendRemoteToggleCueSubscriptionResponse`
  • `onRemoteGetCueActivity` / `sendRemoteGetCueActivityResponse`

`grep -rn` confirms zero remaining references anywhere in `src/`. The trigger bridge (`onRemoteTriggerCueSubscription`) is kept — it's still wired and used by `useRemoteIntegration`.

Test plan

  • `vitest run src/tests/main/web-server/web-server-factory.test.ts` — 58/58 pass. 7 new tests:
    • toggle: dispatches through dep, propagates false return, warns when dep missing.
    • activity: maps `CueRunResult` → `CueActivityEntry`, filters by `sessionId` before `limit`, collapses `timeout`/`stopped` to `failed`, warns when dep missing.
  • `vitest run src/tests/main/cue/cue-engine.test.ts` — 115/115 pass. 5 new tests for `setSubscriptionEnabled`: malformed ids, unresolved session, missing YAML, missing subscription, happy path (parse → mutate → write → refresh).
  • Prettier + ESLint clean on touched files.
  • Smoke test in a new rc build:

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • New Features
    • Added ability to remotely enable or disable Cue subscriptions
    • Implemented Cue activity log retrieval with session filtering and status tracking

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 11, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4df0639a-b909-4a5f-bc98-e67354f277ba

📥 Commits

Reviewing files that changed from the base of the PR and between 5324e61 and 983e5b1.

📒 Files selected for processing (6)
  • src/__tests__/main/cue/cue-engine.test.ts
  • src/__tests__/main/web-server/web-server-factory.test.ts
  • src/main/cue/cue-engine.ts
  • src/main/index.ts
  • src/main/preload/process.ts
  • src/main/web-server/web-server-factory.ts
💤 Files with no reviewable changes (1)
  • src/main/preload/process.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/main/index.ts
  • src/tests/main/cue/cue-engine.test.ts
  • src/main/cue/cue-engine.ts
  • src/tests/main/web-server/web-server-factory.test.ts
  • src/main/web-server/web-server-factory.ts

📝 Walkthrough

Walkthrough

This PR refactors Cue subscription management from renderer IPC to in-process engine delegation. It adds a new setSubscriptionEnabled API to CueEngine with per-project serialized YAML writes, updates web-server callbacks to call engine functions directly, removes obsolete preload IPC endpoints, and adds tests throughout.

Changes

Cue Subscription Remote Toggle via In-Process Engine Callbacks

Layer / File(s) Summary
CueEngine subscription toggle implementation
src/main/cue/cue-engine.ts
Add subscription-id parsing imports, per-projectRoot YAML write chains for serialization, setSubscriptionEnabled public API that parses composite ids and enqueues chained YAML updates, and runSubscriptionEnabledWrite helper that reads, locates subscription by name + pipeline key, flips enabled flag, and persists via js-yaml.
CueEngine toggle tests
src/__tests__/main/cue/cue-engine.test.ts
Mock cue-config-repository with in-memory ViTest I/O, add js-yaml import for YAML assertions, and test setSubscriptionEnabled covering malformed ids, missing session/YAML, absent subscription, cross-pipeline scoping enforcement, duplicate name handling, written YAML content, and serialized concurrent toggles.
WebServer types, helpers, and dependencies interface
src/main/web-server/web-server-factory.ts
Extend type imports for Cue contracts, add formatCueSchedule helper for CLI-friendly schedule strings, and extend WebServerFactoryDependencies interface with optional getCueGraphData, setCueSubscriptionEnabled, and getCueActivityLog accessors.
WebServer in-process Cue callbacks
src/main/web-server/web-server-factory.ts
Replace IPC-based handlers with in-process implementations: subscriptions callback calls getCueGraphData and flattens with id composition; toggle callback delegates to setCueSubscriptionEnabled; activity callback calls getCueActivityLog, filters by sessionId, applies positive limits, and maps runs to CueActivityEntry with status normalization.
WebServer callback tests
src/__tests__/main/web-server/web-server-factory.test.ts
Test toggle callback direct delegation with correct argument/return propagation; test activity callback delegation including timestamp parsing, subscriptionId derivation with fallback, sessionId filtering before limits, and engine-to-web status normalization.
Main process integration and preload cleanup
src/main/index.ts, src/main/preload/process.ts
Wire engine accessors into createWebServerFactory with fallback []/false; remove obsolete preload IPC endpoints (onRemoteGetCueSubscriptions, sendRemoteGetCueSubscriptionsResponse, onRemoteToggleCueSubscription, sendRemoteToggleCueSubscriptionResponse, onRemoteGetCueActivity, sendRemoteGetCueActivityResponse).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • RunMaestro/Maestro#982: Shared subscription-id composition and pipeline-key logic for correct subscription targeting across Cue web-server layer.
  • RunMaestro/Maestro#582: Introduces activity log and run-status behavior that the new getCueActivityLog callback maps into web schema.

Poem

🐰 With chains that serialize our YAML writes,

And callbacks speaking in the light,

No IPC hops from thread to thread—

The engine speaks, the web is fed! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(cue): wire toggle + activity through main, drop dead preload bridges' directly and accurately summarizes the main change: moving Cue toggle and activity operations from dead IPC preload bridges to the main process.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/main/web-server/web-server-factory.ts`:
- Around line 2305-2309: The subscription IDs are not globally unique because
they are composed only from session.sessionId and sub.name (which is only unique
within a pipeline), causing setCueSubscriptionEnabled(...) to match and toggle
the wrong subscription; update both the listing and toggle paths to use a truly
stable identifier (e.g., include the pipeline discriminator or a persisted
per-subscription id) instead of `${session.sessionId}::${sub.name}`: modify
where IDs are composed (look for usage of session.sessionId and sub.name in
web-server-factory.ts) to emit the new unique id and update
setCueSubscriptionEnabled(...) and any resolver that matches by name to resolve
by this new id consistently so toggles operate on the exact subscription.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7907f637-c6bf-4532-8e4f-76920cccd74b

📥 Commits

Reviewing files that changed from the base of the PR and between 56ece61 and ba16d45.

📒 Files selected for processing (6)
  • src/__tests__/main/cue/cue-engine.test.ts
  • src/__tests__/main/web-server/web-server-factory.test.ts
  • src/main/cue/cue-engine.ts
  • src/main/index.ts
  • src/main/preload/process.ts
  • src/main/web-server/web-server-factory.ts
💤 Files with no reviewable changes (1)
  • src/main/preload/process.ts

Comment thread src/main/web-server/web-server-factory.ts
@greptile-apps

greptile-apps Bot commented May 11, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes three dead IPC bridges in the Cue control surface by routing getCueSubscriptions, toggleCueSubscription, and getCueActivity directly through the main-process engine instead of bouncing them to a renderer that never registered listeners, eliminating 10–30 s stalls and silent no-ops in the web UI. It also removes the six corresponding preload bridges and adds CueEngine.setSubscriptionEnabled for YAML-based toggle persistence.

  • setSubscriptionEnabled (cue-engine.ts): parses the ${sessionId}::${name} id, reads → mutates → writes cue.yaml, then calls refreshSession. The read-modify-write is not atomic; concurrent toggles or a simultaneous pipeline-editor write can silently overwrite each other.
  • Activity mapping (web-server-factory.ts): maps CueRunResult[]CueActivityEntry[], applying the sessionId filter before limit so callers receive N matching entries. timeout and stopped engine statuses collapse to the web-facing failed enum; result for the running state surfaces stderr rather than stdout, which may appear as an empty error field in the dashboard.
  • Preload cleanup: six unused IPC bridges removed from src/main/preload/process.ts; the trigger bridge is correctly retained.

Confidence Score: 4/5

Safe to merge; the fix correctly removes dead IPC bridges and replaces them with direct engine calls. The main thing to watch is the unguarded read-modify-write in setSubscriptionEnabled, which could lose a toggle if the pipeline editor or another web request writes to the same cue.yaml concurrently.

The core change is straightforward and well-tested (12 new tests, 115+58 total pass). The only open concern is the read-modify-write in setSubscriptionEnabled: the method reads cue.yaml, mutates it in memory, and writes it back with no lock or compare-and-swap. In a desktop app this is low-frequency, but the pipeline editor shares the same write path, so the window exists. Everything else — status mapping, filter-before-limit ordering, the boundary guard for malformed ids — checks out.

src/main/cue/cue-engine.ts — the setSubscriptionEnabled read-modify-write loop; src/main/web-server/web-server-factory.ts — the result field mapping for the running status case.

Important Files Changed

Filename Overview
src/main/cue/cue-engine.ts Adds setSubscriptionEnabled: reads YAML, mutates the enabled flag in-memory, writes back, then calls refreshSession. Read-modify-write is not atomic — concurrent toggles or a simultaneous pipeline-editor save between the read and write can cause one to silently overwrite the other.
src/main/web-server/web-server-factory.ts Replaces three IPC-bounce stubs (getCueSubscriptions, toggleCueSubscription, getCueActivity) with direct engine calls. Mapping logic, sessionId filtering, and status collapsing look correct.
src/main/index.ts Wires three new deps into createWebServerFactory, all guarding against a null cueEngine. Clean and consistent with the existing triggerCueSubscription pattern.
src/main/preload/process.ts Removes six dead preload bridges; the onRemoteTriggerCueSubscription bridge is intentionally retained.
src/tests/main/cue/cue-engine.test.ts Adds 5 focused tests for setSubscriptionEnabled covering malformed IDs, missing session, missing YAML, missing subscription, and the happy path.
src/tests/main/web-server/web-server-factory.test.ts Adds 7 tests covering subscriptions, toggle, and activity callbacks including filter-before-limit and status collapsing.

Sequence Diagram

sequenceDiagram
    participant WebUI as Web UI / CLI
    participant WebServer as WebServerFactory
    participant Engine as CueEngine
    participant FS as cue.yaml (YAML)

    note over WebUI,FS: getCueSubscriptions
    WebUI->>WebServer: "GET /cue/subscriptions?sessionId=X"
    WebServer->>Engine: getCueGraphData()
    Engine-->>WebServer: CueGraphSession[]
    WebServer-->>WebUI: CueSubscriptionInfo[] (flattened)

    note over WebUI,FS: toggleCueSubscription
    WebUI->>WebServer: POST /cue/subscriptions/:id/toggle
    WebServer->>Engine: setSubscriptionEnabled(id, enabled)
    Engine->>FS: readCueConfigFile(projectRoot)
    FS-->>Engine: raw YAML
    Engine->>FS: writeCueConfigFile(projectRoot, mutated YAML)
    Engine->>Engine: refreshSession(sessionId, projectRoot)
    Engine-->>WebServer: boolean
    WebServer-->>WebUI: success / false

    note over WebUI,FS: getCueActivity
    WebUI->>WebServer: "GET /cue/activity?sessionId=X&limit=N"
    WebServer->>Engine: getCueActivityLog()
    Engine-->>WebServer: CueRunResult[]
    WebServer-->>WebUI: CueActivityEntry[] (filtered + mapped)
Loading

Comments Outside Diff (1)

  1. src/main/web-server/web-server-factory.ts, line 2844-2854 (link)

    P2 result field is undefined for running status

    The mapping r.status === 'completed' ? r.stdout || undefined : r.stderr || undefined sets result to r.stderr || undefined for every non-completed status, including 'running'. A run that is still active will typically have an empty stderr, so result silently becomes undefined in the activity dashboard. If the dashboard distinguishes between "run in progress" and "run finished with no output", this could appear as "no result" even when the run has already produced stdout. Consider returning r.stdout || undefined (or leaving result absent) for the 'running' state so the dashboard can show live progress rather than an empty error field.

Reviews (1): Last reviewed commit: "fix(cue): wire toggle + activity through..." | Re-trigger Greptile

Comment on lines +664 to +697
const file = readCueConfigFile(projectRoot);
if (!file) return false;

let parsed: unknown;
try {
parsed = yaml.load(file.raw);
} catch (err) {
captureException(err, { operation: 'setSubscriptionEnabled:yamlLoad', sessionId });
return false;
}
if (!parsed || typeof parsed !== 'object') return false;
const subs = (parsed as Record<string, unknown>).subscriptions;
if (!Array.isArray(subs)) return false;

let found = false;
for (const sub of subs) {
if (!sub || typeof sub !== 'object') continue;
if ((sub as Record<string, unknown>).name === subName) {
(sub as Record<string, unknown>).enabled = enabled;
found = true;
break;
}
}
if (!found) return false;

try {
const serialized = yaml.dump(parsed, { lineWidth: -1, noRefs: true });
writeCueConfigFile(projectRoot, serialized);
} catch (err) {
captureException(err, { operation: 'setSubscriptionEnabled:yamlWrite', sessionId });
return false;
}

this.refreshSession(sessionId, projectRoot);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Unguarded read-modify-write on cue.yaml

readCueConfigFile → in-memory mutation → writeCueConfigFile is not atomic. If two toggle requests for subscriptions in the same session arrive concurrently (e.g., rapid web-UI clicks, or a simultaneous pipeline-editor save that also calls writeCueConfigFile), whichever write lands second silently discards the first write's changes. Existing callers use writeCueConfigFile too (the pipeline editor save path), so the window is real even for a single user. A simple guard — a per-projectRoot mutex or a "read the file again immediately before writing and compare" — would close it.

@pedramamini

Copy link
Copy Markdown
Collaborator

@chr1syy thanks for the follow-up — nice surgical cleanup, and the test coverage on both new paths (setSubscriptionEnabled happy/failure modes + the activity-log mapping + sessionId-filter-before-limit) is exactly what I'd want to see.

One concern from the bot reviews I'd like your read on before I approve:

Subscription identity (CodeRabbit, major). ${sessionId}::${name} assumes names are unique within a session, but the validator contract you cite is unique-within-pipeline. Two pipelines under the same session can each legitimately have a sub named Digest Script, in which case setSubscriptionEnabled finds the first match and silently toggles the wrong row — and the web UI has no way to tell. Options I see:

  • Encode pipeline into the id (${sessionId}::${pipeline}::${name}) on both the flatten side (getCueGraphData callback in web-server-factory) and the parse side here, OR
  • Resolve by stable index from the flattener output, OR
  • Tighten the validator contract to unique-within-session and call that out.

Whichever you prefer is fine, but the current shape is foot-gun-y once someone has two pipelines.

Read-modify-write atomicity (Greptile, P2). Same trade-off as the pipeline editor's save path, as you noted, so I'd just file it as a known limitation rather than blocking on it here. If you want to preemptively add a single-flight lock per project root in a follow-up that'd be welcome, but not for this PR.

Happy to merge as soon as the identity question is resolved (either a fix or a "won't happen because X" answer).

chr1syy added a commit to chr1syy/Maestro that referenced this pull request May 11, 2026
Two review fixes for PR RunMaestro#982:

- **Subscription id disambiguation (CodeRabbit major on follow-up RunMaestro#983,
  Pedram).** The old `\${sessionId}::\${name}` shape collides when two
  pipelines under the same session each legitimately define a sub with
  the same name. A downstream toggle would resolve by name and mutate
  the wrong row. New format `\${sessionId}::\${pipeline}::\${name}`
  carries the pipeline discriminator. Extracted compose/parse/pipeline-
  key helpers into a new `src/shared/cue/subscription-id.ts` module so
  the toggle path in PR RunMaestro#983 reuses the same identity contract.
  `pipelineKeyForSubscription` prefers explicit \`pipeline_name\` and
  falls back to the same suffix-stripping as `yamlToPipeline.ts`'s
  `getBasePipelineName` for legacy YAML. Compose throws on hand-edited
  YAML where any component contains the `::` separator instead of
  silently producing an unparseable id.

- **schedule_days surfaced in CLI output (Greptile P2 + Pedram).**
  `cue list` used to render day-pinned schedules as just the times
  (or `undefined` when only days were set). New `formatCueSchedule`
  helper renders `\"07:00 (Mon, Wed, Fri)\"` when both are present and
  `\"days: Sat, Sun\"` when only days are present.

Tests:
- New `subscription-id.test.ts` with 14 cases covering compose, parse,
  pipeline-key derivation, collision avoidance, and the `::` guard.
- `web-server-factory.test.ts`: updated the flatten test to assert the
  new id format; added regressions for (a) two pipelines/same name id
  disambiguation, (b) legacy YAML fallback to base-name stripping,
  (c) schedule_days rendering alongside schedule_times and standalone.
- 68/68 tests pass in the touched suites; prettier + ESLint clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chr1syy added a commit to chr1syy/Maestro that referenced this pull request May 11, 2026
…oot mutex

Two follow-up fixes layered on top of PR RunMaestro#983's toggle/activity migration:

- **Pipeline-disambiguated subscription id (CodeRabbit major, Pedram).**
  `CueEngine.setSubscriptionEnabled` now parses
  `${sessionId}::${pipeline}::${name}` via `parseCueSubscriptionId` (the
  helper added in the parent RunMaestro#982 update) and matches the YAML row by
  BOTH pipeline AND name. Activity-log mapping in the web-server factory
  composes ids the same way via `composeCueSubscriptionId`, so a web UI
  could navigate from an activity row to the toggle callback without
  re-deriving identity. Without this, two pipelines under one session
  that each define a sub named "Foo" produced indistinguishable ids and
  the first-match heuristic would silently toggle the wrong row.

- **Per-projectRoot write serialisation (Greptile P2).** The read →
  mutate → write cycle on cue.yaml is not atomic on the filesystem. Two
  concurrent toggles for subs in the same project (rapid web-UI clicks,
  or a toggle racing the pipeline editor's save) would otherwise let
  whichever write lands second silently discard the first's flip. The
  engine now keeps a `yamlWriteChains: Map<projectRoot, Promise>` and
  appends each call to its project's chain, so writes serialise per
  file even under concurrent invocation. The map entry is dropped when
  the chain settles to keep this from leaking across the engine's
  lifetime.

`setSubscriptionEnabled` is now `async` and returns `Promise<boolean>`.
Updated the `setCueSubscriptionEnabled` dep type and the `main/index.ts`
wiring accordingly.

Tests:
- `cue-engine.test.ts`: rewrote the existing 5 tests for the new id
  format and async signature; added a "cross-pipeline no silent toggle"
  regression (the CodeRabbit case verbatim — id `session-1::Pipeline B::Foo`
  must not match a row with `pipeline_name: Pipeline A`), a happy-path
  test for two same-named subs in one session, and a serialisation
  test that asserts strict `read,write,read,write` ordering on two
  Promise.all'd toggles.
- `web-server-factory.test.ts`: bumped id format in the toggle + activity
  expectations and added a fallback test for activity entries whose
  `CueRunResult.pipelineName` is absent.

194/194 tests pass across the three touched suites; prettier + ESLint
clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@chr1syy
chr1syy force-pushed the fix/cue-remote-callbacks-cleanup branch from ba16d45 to 5324e61 Compare May 11, 2026 07:48

@coderabbitai coderabbitai Bot 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.

♻️ Duplicate comments (1)
src/shared/cue/subscription-id.ts (1)

37-45: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Legacy subscriptions without pipeline_name are still ambiguous.

This only disambiguates rows when the pipeline can actually be recovered. For legacy subs where pipeline_name is missing and the name does not encode a distinct suffix, pipelineKeyForSubscription() collapses to name, so two same-named subs in one session still serialize to the same sessionId::name::name ID. That reopens the exact wrong-row toggle bug for the legacy path this helper is trying to preserve. Please either materialize a real pipeline discriminator before exposing remote IDs, or reject unresolved legacy rows instead of composing a supposedly stable ID.

Also applies to: 55-69

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/shared/cue/subscription-id.ts` around lines 37 - 45,
pipelineKeyForSubscription currently falls back to the raw name for legacy rows,
which can produce non-unique keys; update pipelineKeyForSubscription (and its
sibling logic around lines 55-69) to detect when pipeline_name is missing and
the name normalization does not change the value (i.e., no distinct suffix) and
then either (A) refuse to produce a key by throwing a clear error or returning a
failure so callers can reject unresolved legacy subscriptions, or (B)
materialize a stable discriminator (e.g., derive/append a stored pipeline id or
a deterministic hash from subscription metadata) before composing the remote ID;
locate and change pipelineKeyForSubscription and any related functions that
build sessionId::name::name IDs to implement one of these two options.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@src/shared/cue/subscription-id.ts`:
- Around line 37-45: pipelineKeyForSubscription currently falls back to the raw
name for legacy rows, which can produce non-unique keys; update
pipelineKeyForSubscription (and its sibling logic around lines 55-69) to detect
when pipeline_name is missing and the name normalization does not change the
value (i.e., no distinct suffix) and then either (A) refuse to produce a key by
throwing a clear error or returning a failure so callers can reject unresolved
legacy subscriptions, or (B) materialize a stable discriminator (e.g.,
derive/append a stored pipeline id or a deterministic hash from subscription
metadata) before composing the remote ID; locate and change
pipelineKeyForSubscription and any related functions that build
sessionId::name::name IDs to implement one of these two options.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f8b7a04a-3945-496b-a226-ec81c22b9182

📥 Commits

Reviewing files that changed from the base of the PR and between ba16d45 and 5324e61.

📒 Files selected for processing (8)
  • src/__tests__/main/cue/cue-engine.test.ts
  • src/__tests__/main/web-server/web-server-factory.test.ts
  • src/__tests__/shared/cue/subscription-id.test.ts
  • src/main/cue/cue-engine.ts
  • src/main/index.ts
  • src/main/preload/process.ts
  • src/main/web-server/web-server-factory.ts
  • src/shared/cue/subscription-id.ts
💤 Files with no reviewable changes (1)
  • src/main/preload/process.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/index.ts
  • src/tests/main/web-server/web-server-factory.test.ts

@chr1syy chr1syy added the ready to merge This PR is ready to merge label May 11, 2026
pedramamini pushed a commit that referenced this pull request May 13, 2026
…er IPC (#982)

* fix(cue): answer get_cue_subscriptions from main, not via dead renderer IPC

The web-server factory used to fulfil `get_cue_subscriptions` (the CLI's
`maestro-cli cue list` message) by forwarding `remote:getCueSubscriptions`
to the renderer and waiting 30 s for a response — but no renderer code
ever registered a handler for that channel. The CLI's 10 s timeout fired
on every call and `cue list --json` surfaced as
`Command timed out waiting for cue_subscriptions`.

Mirror the `triggerCueSubscription` pattern: inject a `getCueGraphData`
dep that calls `cueEngine.getGraphData()` directly in main, then flatten
the result into `CueSubscriptionInfo[]` inside the callback. The engine
lives in main anyway, so the renderer round-trip was never needed.

Per-subscription `lastTriggered` / `triggerCount` aren't tracked by the
engine yet (only per-session) — leave the numeric fields zero so the CLI
renders "never triggered" rather than fabricating values.

Adds three regression tests in `web-server-factory.test.ts`:
- flattens engine graph data into CueSubscriptionInfo[] without IPC
- filters by sessionId when supplied
- returns [] and warns when the dep is missing

The dead `remote:toggleCueSubscription` and `remote:getCueActivity`
bridges have the same shape — same diagnosis, same fix path — but neither
is reachable from any current CLI command. Tracked for a follow-up.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(cue): pipeline-disambiguated sub ids + surface schedule_days

Two review fixes for PR #982:

- **Subscription id disambiguation (CodeRabbit major on follow-up #983,
  Pedram).** The old `\${sessionId}::\${name}` shape collides when two
  pipelines under the same session each legitimately define a sub with
  the same name. A downstream toggle would resolve by name and mutate
  the wrong row. New format `\${sessionId}::\${pipeline}::\${name}`
  carries the pipeline discriminator. Extracted compose/parse/pipeline-
  key helpers into a new `src/shared/cue/subscription-id.ts` module so
  the toggle path in PR #983 reuses the same identity contract.
  `pipelineKeyForSubscription` prefers explicit \`pipeline_name\` and
  falls back to the same suffix-stripping as `yamlToPipeline.ts`'s
  `getBasePipelineName` for legacy YAML. Compose throws on hand-edited
  YAML where any component contains the `::` separator instead of
  silently producing an unparseable id.

- **schedule_days surfaced in CLI output (Greptile P2 + Pedram).**
  `cue list` used to render day-pinned schedules as just the times
  (or `undefined` when only days were set). New `formatCueSchedule`
  helper renders `\"07:00 (Mon, Wed, Fri)\"` when both are present and
  `\"days: Sat, Sun\"` when only days are present.

Tests:
- New `subscription-id.test.ts` with 14 cases covering compose, parse,
  pipeline-key derivation, collision avoidance, and the `::` guard.
- `web-server-factory.test.ts`: updated the flatten test to assert the
  new id format; added regressions for (a) two pipelines/same name id
  disambiguation, (b) legacy YAML fallback to base-name stripping,
  (c) schedule_days rendering alongside schedule_times and standalone.
- 68/68 tests pass in the touched suites; prettier + ESLint clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chr1syy and others added 2 commits May 13, 2026 10:26
Follow-up to fix/cue-remote-subscriptions (PR RunMaestro#982). The same dead-bridge
shape existed for `remote:toggleCueSubscription` and `remote:getCueActivity`:
the web-server factory forwarded each request to the renderer and waited
for a response on an IPC channel no renderer code had ever subscribed to.
Web-UI toggles silently no-op'd after a 10 s stall and the activity tab
rendered empty after a 30 s stall.

Mirror the `triggerCueSubscription` / `getCueGraphData` pattern: add two
deps on `WebServerFactoryDependencies` — `setCueSubscriptionEnabled` and
`getCueActivityLog` — and wire them in `main/index.ts` from `cueEngine`.

The toggle flow needs an engine-side mutation, so add
`CueEngine.setSubscriptionEnabled(subscriptionId, enabled)`. It parses
the `${sessionId}::${name}` id format used by `getCueGraphData`'s
flattener, looks up the owning session via `deps.getSessions()`,
reads → parses → mutates → writes the YAML, and calls `refreshSession`.
Comments and field ordering aren't preserved on the round-trip (same
trade-off as the pipeline editor's save path).

Activity mapping converts `CueRunResult[]` → `CueActivityEntry[]`,
applies the optional `sessionId` filter before slicing to `limit`, and
collapses engine-side `timeout`/`stopped` statuses into the web-facing
`failed` enum so dashboards don't render `?`.

Dead-code cleanup: the matching preload bridges
(`onRemoteGetCueSubscriptions` / `sendRemoteGetCueSubscriptionsResponse`,
`onRemoteToggleCueSubscription` / `sendRemoteToggleCueSubscriptionResponse`,
`onRemoteGetCueActivity` / `sendRemoteGetCueActivityResponse`) now have
zero callers — `grep -rn` confirms no references anywhere in `src/`.
Removed from `src/main/preload/process.ts`.

Tests:
- `web-server-factory.test.ts`: 7 new tests covering both new callback
  paths — dispatch through dep, false-return propagation, missing-dep
  warning, sessionId filter before limit, status-enum collapsing.
- `cue-engine.test.ts`: 5 new tests for `setSubscriptionEnabled` covering
  malformed ids, unresolved session, missing YAML, missing subscription,
  and the happy path (parse → mutate → write → refresh).

All 173 tests in the two suites pass; prettier + ESLint clean on touched
files.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…oot mutex

Two follow-up fixes layered on top of PR RunMaestro#983's toggle/activity migration:

- **Pipeline-disambiguated subscription id (CodeRabbit major, Pedram).**
  `CueEngine.setSubscriptionEnabled` now parses
  `${sessionId}::${pipeline}::${name}` via `parseCueSubscriptionId` (the
  helper added in the parent RunMaestro#982 update) and matches the YAML row by
  BOTH pipeline AND name. Activity-log mapping in the web-server factory
  composes ids the same way via `composeCueSubscriptionId`, so a web UI
  could navigate from an activity row to the toggle callback without
  re-deriving identity. Without this, two pipelines under one session
  that each define a sub named "Foo" produced indistinguishable ids and
  the first-match heuristic would silently toggle the wrong row.

- **Per-projectRoot write serialisation (Greptile P2).** The read →
  mutate → write cycle on cue.yaml is not atomic on the filesystem. Two
  concurrent toggles for subs in the same project (rapid web-UI clicks,
  or a toggle racing the pipeline editor's save) would otherwise let
  whichever write lands second silently discard the first's flip. The
  engine now keeps a `yamlWriteChains: Map<projectRoot, Promise>` and
  appends each call to its project's chain, so writes serialise per
  file even under concurrent invocation. The map entry is dropped when
  the chain settles to keep this from leaking across the engine's
  lifetime.

`setSubscriptionEnabled` is now `async` and returns `Promise<boolean>`.
Updated the `setCueSubscriptionEnabled` dep type and the `main/index.ts`
wiring accordingly.

Tests:
- `cue-engine.test.ts`: rewrote the existing 5 tests for the new id
  format and async signature; added a "cross-pipeline no silent toggle"
  regression (the CodeRabbit case verbatim — id `session-1::Pipeline B::Foo`
  must not match a row with `pipeline_name: Pipeline A`), a happy-path
  test for two same-named subs in one session, and a serialisation
  test that asserts strict `read,write,read,write` ordering on two
  Promise.all'd toggles.
- `web-server-factory.test.ts`: bumped id format in the toggle + activity
  expectations and added a fallback test for activity entries whose
  `CueRunResult.pipelineName` is absent.

194/194 tests pass across the three touched suites; prettier + ESLint
clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@pedramamini
pedramamini force-pushed the fix/cue-remote-callbacks-cleanup branch from 5324e61 to 983e5b1 Compare May 13, 2026 15:29
@pedramamini
pedramamini merged commit 69abfc2 into RunMaestro:rc May 13, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge This PR is ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants