feat(openchamber): OpenChamber-parity feature set + capability gating#1604
feat(openchamber): OpenChamber-parity feature set + capability gating#1604jms830 wants to merge 1 commit into
Conversation
Layers the OpenChamber feature set onto Paseo v0.1.96 (upstream af0437e): - RPC-backed: provider usage/quota, skills catalog, Cloudflare tunnel, per-repo git identity. Schemas/types in @getpaseo/protocol, services in @getpaseo/server, client methods in @getpaseo/client, UI sections in app. - Each RPC-backed section is gated behind a server_info.features.* flag (quota/skillsCatalog/gitIdentity/tunnel) so it degrades gracefully on older daemons. - Client-side: workspace folders, project notes, multi-run launcher, gitmoji commit toggle, typed-i18n runtime. - Dev tooling: scripts/dev-web.sh + npm run dev:web (auto-scan ports, stable PASEO_HOME). typecheck (protocol/server/app) green; oxlint 0/0; web bundle compiles.
jms830
force-pushed
the
openchamber-parity
branch
from
5026f7c to
58069b9
Compare
|
| Filename | Overview |
|---|---|
| packages/server/src/server/session.ts | Integrates four new services into Session's dispatch chain; skills scan/install handlers are missing try/catch unlike all other new handlers — unhandled throws produce client timeouts instead of error responses. |
| packages/server/src/server/tunnel/tunnel.test.ts | Uses vi.mock to replace the spawn module, which is a banned test pattern for this project; the real spawn path is never exercised. |
| packages/server/src/server/quota/quota.test.ts | Tests internal formatters and transformers rather than the QuotaService interface; implementation details are pinned while the behavior callers depend on goes untested. |
| packages/server/src/server/skills-catalog/repo.ts | Clones git repositories into temp directories and copies skill files with symlink rejection and path traversal guards; cleanup is in finally blocks and skill names are validated before installation. |
| packages/server/src/server/quota/auth.ts | Reads auth tokens from local JSON files; normalizeAuthEntry has an unreachable final branch that can be simplified; tokens are not exposed in error messages. |
| packages/server/src/server/websocket-server.ts | Instantiates the four new services as singleton fields and passes them into every new Session; TunnelService singleton is correct since cloudflared should only run once per server instance. |
| packages/protocol/src/messages.ts | Adds ten new inbound/outbound schema entries and four optional capability flags; all fields are optional, maintaining backward compatibility. |
| packages/server/src/server/tunnel/cloudflare-tunnel.ts | Manages cloudflared process lifecycle with state transitions, timeout handling, and SIGTERM cleanup; logic is correct and port is injected externally so injection risk is absent. |
| packages/app/src/stores/workspace-folders-store.ts | Zustand + AsyncStorage store for collapsible workspace folders; cascade delete, stale workspace cleanup, and persisted-state sanitization are all handled. |
| packages/server/src/server/git-identity/service.ts | Reads and writes local git config for user.name/email via execCommand with args array (no shell injection risk). |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant App as App (client)
participant DC as DaemonClient
participant WS as WebSocketServer
participant S as Session
participant Svc as Service (Quota/Skills/GitId/Tunnel)
App->>DC: quotaFetch(providerId)
DC->>WS: "quota/fetch {requestId, providerId}"
WS->>S: dispatchMessage(msg)
S->>S: dispatchQuotaMessage(msg)
S->>Svc: quotaService.fetch(providerId)
Svc-->>S: ProviderResult
S-->>WS: "emit quota/fetch/response {result}"
WS-->>DC: correlated response
DC-->>App: QuotaFetchPayload
Note over App,Svc: skills/scan and skills/install follow same path but handlers lack try/catch
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant App as App (client)
participant DC as DaemonClient
participant WS as WebSocketServer
participant S as Session
participant Svc as Service (Quota/Skills/GitId/Tunnel)
App->>DC: quotaFetch(providerId)
DC->>WS: "quota/fetch {requestId, providerId}"
WS->>S: dispatchMessage(msg)
S->>S: dispatchQuotaMessage(msg)
S->>Svc: quotaService.fetch(providerId)
Svc-->>S: ProviderResult
S-->>WS: "emit quota/fetch/response {result}"
WS-->>DC: correlated response
DC-->>App: QuotaFetchPayload
Note over App,Svc: skills/scan and skills/install follow same path but handlers lack try/catch
Reviews (1): Last reviewed commit: "feat(openchamber): OpenChamber-parity fe..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| type: "skills/install/response", | ||
| payload: { | ||
| requestId: request.requestId, | ||
| installed: result.ok ? result.installed : [], | ||
| skipped: result.ok ? result.skipped : [], | ||
| error: result.ok ? null : result.error, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| private async handleGitIdentityGetRequest( | ||
| request: Extract<SessionInboundMessage, { type: "git-identity/get" }>, | ||
| ): Promise<void> { | ||
| try { | ||
| const identity = await this.gitIdentityService.get(request.cwd); | ||
| this.emit({ | ||
| type: "git-identity/get/response", | ||
| payload: { requestId: request.requestId, identity, error: null }, | ||
| }); | ||
| } catch (error) { | ||
| this.emit({ | ||
| type: "git-identity/get/response", | ||
| payload: { | ||
| requestId: request.requestId, |
There was a problem hiding this comment.
handleSkillsScanRequest and handleSkillsInstallRequest call the service but have no try/catch, unlike every other handler added in this PR (quota, git-identity, tunnel). If scanRepository throws before returning a result — e.g., mkdtemp fails with ENOSPC or a filesystem error escapes the finally block — the exception propagates to the dispatch loop. The client receives no response and the request times out on the 120-second correlated-request timeout.
All other handlers in this file emit an error payload in their catch branch. Both skills handlers need the same treatment to match that contract.
| import { EventEmitter } from "node:events"; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||
|
|
||
| const spawnProcessMock = vi.fn(); | ||
| const execCommandMock = vi.fn(); | ||
|
|
||
| vi.mock("../../utils/spawn.js", () => ({ | ||
| spawnProcess: (...args: unknown[]) => spawnProcessMock(...args), | ||
| execCommand: (...args: unknown[]) => execCommandMock(...args), | ||
| })); | ||
|
|
||
| import { CloudflareTunnel } from "./cloudflare-tunnel.js"; |
There was a problem hiding this comment.
vi.mock is a banned test pattern for this codebase
The tunnel test hoists vi.mock("../../utils/spawn.js", ...) to replace spawnProcess and execCommand. The project's review rules explicitly ban vi.mock and vi.hoisted. The test also imports vi from vitest for vi.fn() and vi.waitFor, which means the system under test is never exercised against the real execCommand path.
The idiomatic alternative is a typed fake: pass a Spawner port into CloudflareTunnel (or TunnelService) and wire the real spawnProcess in production and an in-memory fake in tests. That way the spawn seam is named and the test verifies real behavior.
Rule Used: # Code Review Pattern Reference: Slop, Tests, Feat... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| import { describe, expect, it } from "vitest"; | ||
| import { | ||
| buildResult, | ||
| calculateResetAfterSeconds, | ||
| formatMoney, | ||
| toUsageWindow, | ||
| } from "./formatters.js"; | ||
| import { toNumber, toTimestamp } from "./transformers.js"; | ||
| import { fetchQuotaForProvider } from "./providers/index.js"; | ||
|
|
||
| describe("quota transformers", () => { | ||
| it("coerces numbers from strings and rejects junk", () => { | ||
| expect(toNumber(42)).toBe(42); | ||
| expect(toNumber("3.5")).toBe(3.5); | ||
| expect(toNumber("nope")).toBeNull(); | ||
| expect(toNumber(null)).toBeNull(); | ||
| }); | ||
|
|
||
| it("normalizes timestamps (seconds -> ms, ISO -> ms)", () => { | ||
| expect(toTimestamp(1_000)).toBe(1_000_000); | ||
| expect(toTimestamp(2_000_000_000_000)).toBe(2_000_000_000_000); | ||
| expect(toTimestamp("2025-01-01T00:00:00Z")).toBe(Date.parse("2025-01-01T00:00:00Z")); | ||
| expect(toTimestamp(null)).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("quota formatters", () => { | ||
| it("formats money to two decimals", () => { | ||
| expect(formatMoney(3.5)).toBe("3.50"); | ||
| expect(formatMoney("x")).toBeNull(); | ||
| }); | ||
|
|
||
| it("derives remaining percent from used percent", () => { | ||
| const window = toUsageWindow({ usedPercent: 30, windowSeconds: 3600, resetAt: null }); | ||
| expect(window.remainingPercent).toBe(70); | ||
| expect(window.windowSeconds).toBe(3600); | ||
| }); | ||
|
|
||
| it("leaves remaining percent null when usedPercent is null", () => { | ||
| const window = toUsageWindow({ usedPercent: null, windowSeconds: null, resetAt: null }); | ||
| expect(window.remainingPercent).toBeNull(); | ||
| }); | ||
|
|
||
| it("clamps negative reset deltas to zero", () => { | ||
| expect(calculateResetAfterSeconds(Date.now() - 10_000)).toBe(0); | ||
| expect(calculateResetAfterSeconds(null)).toBeNull(); | ||
| }); | ||
|
|
||
| it("builds a result with fetchedAt and optional error", () => { | ||
| const ok = buildResult({ providerId: "x", providerName: "X", ok: true, configured: true }); | ||
| expect(ok.usage).toBeNull(); | ||
| expect(ok.fetchedAt).toBeGreaterThan(0); | ||
| expect("error" in ok).toBe(false); | ||
|
|
||
| const failed = buildResult({ | ||
| providerId: "x", | ||
| providerName: "X", | ||
| ok: false, | ||
| configured: false, | ||
| error: "boom", | ||
| }); | ||
| expect(failed.error).toBe("boom"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("quota registry", () => { | ||
| it("returns an unsupported result for unknown providers", async () => { | ||
| const result = await fetchQuotaForProvider("does-not-exist"); | ||
| expect(result.ok).toBe(false); | ||
| expect(result.configured).toBe(false); | ||
| expect(result.error).toBe("Unsupported provider"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test imports buildResult, calculateResetAfterSeconds, formatMoney, toUsageWindow from formatters.ts and toNumber, toTimestamp from transformers.ts. These are private implementation files; no external caller imports them. The actual module interface visible to callers is QuotaService.listConfigured() and QuotaService.fetch(), and neither is tested here. The same pattern applies to skills-catalog.test.ts, which tests parseSkillRepoSource and parseSkillFrontmatter instead of SkillsCatalogService.scan() / .install().
Per the project review standard, tests should cross the same interface as callers. Tests for internal formatters lock in implementation details without verifying the behavior the session handler (and the client) actually depends on.
Rule Used: # Code Review Pattern Reference: Slop, Tests, Feat... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if (typeof entry === "string") { | ||
| return { token: entry }; | ||
| } | ||
| if (typeof entry === "object") { | ||
| return entry; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
return null — dead code after exhaustive branches
The parameter type is AuthEntry | string | null. After !entry (catches null) and typeof entry === "string", the only remaining case is AuthEntry (an object). The if (typeof entry === "object") guard is always true at that point, so return null on the last line can never execute. Remove the redundant guard to keep the logic honest and avoid confusion about when null is returned.
| if (typeof entry === "string") { | |
| return { token: entry }; | |
| } | |
| if (typeof entry === "object") { | |
| return entry; | |
| } | |
| return null; | |
| if (typeof entry === "string") { | |
| return { token: entry }; | |
| } | |
| return entry; |
1 participant
OpenChamber-parity feature set + capability gating
Ports a set of features inspired by OpenChamber (MIT) into Paseo, each gated behind
server_info.features.*capability flags so old clients/daemons degrade gracefully per the protocol contract. All ported files carry a// Ported from openchamber/openchamber (MIT)header.Features
Server-backed (WebSocket-RPC, mirrors the existing
schedulepattern):quota/list+quota/fetch; usage progress bar in host settings.~/.pi/agent/skills;skills/list+skills/scan+skills/install.git-identity/get+set.cloudflared tunnel --url;tunnel/status+start+stop.Client-only (Zustand + AsyncStorage persistence):
useI18n()/translate()with an English catalog (foundation; English-only for now).Notes
domain.op.request/.responsenamespaces; new schema fields are optional; capability flags gate each feature.typecheck(all workspaces),oxlint,oxfmtall green on0.1.97-beta.2. Smoke-tested live in the web app (folders, multi-run, notes, git identity, gitmoji toggle render and function).Opening for maintainer feedback — happy to split into smaller PRs or adjust the capability-gating approach.