Add synthesis spec: superior assistant architecture from OpenClaw + Hermes comparison#93
Open
khaliqgant wants to merge 6 commits into
Open
Add synthesis spec: superior assistant architecture from OpenClaw + Hermes comparison#93khaliqgant wants to merge 6 commits into
khaliqgant wants to merge 6 commits into
Conversation
…ermes comparison Deep comparison of openclaw/openclaw and NousResearch/hermes-agent against the agent-assistant SDK. Identifies what each does well, where each falls short, and proposes a concrete architecture that combines all three strengths. Key additions specified: - @agent-assistant/skills — policy-gated trajectory-to-skill promotion pipeline - DynamicTraitOverlay — Honcho-pattern user modeling composing at assembly time - Platform surface adapters — Telegram, Slack, Discord as surfaces implementations (not a daemon) - Multi-backend execution adapter matrix — maps Hermes 7-backend model to ExecutionAdapter - Migration paths from both OpenClaw (SKILL.md) and Hermes (~/.hermes/skills/) Governing principles extended: "Learning is policy-gated. Platform reach is surface adapters." https://claude.ai/code/session_0161bPaoc8gKK3JgZZDEdt2V
Full architecture spec for OpenKaren ($75/month hosted), covering: Token consciousness stack (the primary differentiator vs OpenClaw + Hermes): - burn: spend attribution, budget gates, model downgrade at 75% budget - rtk: command output compression 60-90% via Rust bash hook - tilth: structural code navigation -40% tokens via tree-sitter MCP - tokensave: semantic graph queries -93% on codebase retrieval - workshop (raindrop): local trace dashboard + eval framework Proactive architecture: - relaycron: daily standup, weekly spend review, workflow health checks - relayfile watches: Linear/GitHub/Notion triggers Karen proactively - relaycast: agent messaging for delegation to Sage (planning) and Ricky (workflows) Local vs hosted breakdown: - relayfile, relaycron, relaycast (agent-to-agent), burn, rtk, tilth, tokensave, workshop, n8n: fully local on Mac mini - relaycast webhooks: Cloudflare Tunnel recommended for external reception - Nango, Composio, Pipedream: cloud-hosted; no local option Workforce persona definition, BYOK subscription model, n8n integration, 6-phase implementation roadmap, port map, token savings projection. https://claude.ai/code/session_0161bPaoc8gKK3JgZZDEdt2V
Two additional repos complete the n8n integration picture: n8n-nodes-relayfile (AgentWorkforce/n8n-nodes-relayfile): - Relayfile Trigger Node: polls VFS workspace for create/update/delete events and fires n8n workflows directly — no Karen involvement needed for routine event routing (issue assigned → Slack, not Karen's budget) - Relayfile Action Node: 7 operations (Read, Write, Query, List, Bulk Write, Get Events, Export) for n8n flows that need to write back to Linear/GitHub/Notion - Relayfile Ops Node: monitors writeback pipeline health; replay failed ops relaycast-n8n-bridge (AgentWorkforce/relaycast-n8n-bridge): - Subscribes to relaycast channels via glob patterns - Forwards agent findings (Ricky bugs, Sage plans) to n8n webhook URLs - 100ms batching + 3-attempt exponential backoff - Message metadata: severity, prNumber, filePaths, agentRole, agentId - /health endpoint for operational visibility - Enables: Ricky finding → PagerDuty + Jira + Telegram without Karen needing to orchestrate each downstream system Updated: component stack diagram, section 9 (full rewrite with 4 flow scenarios), local/hosted table, pre-wiring list, Phase 4 roadmap (now 4 integration tests), port map, comparison table. https://claude.ai/code/session_0161bPaoc8gKK3JgZZDEdt2V
…n bridging Karen now supports Telegram + Slack simultaneously via agent-assistant/surfaces. Conversations bridge seamlessly across both — same session context regardless of which surface the user messages from. Surfaces: - Slack via createSlackSurface() with thread-reply mode; Cloudflare Tunnel for Slack event delivery to local Mac mini - Telegram unchanged (polling mode, fully local) Nango as single OAuth foundation: - One Slack OAuth flow covers three consumers: Karen's Slack surface (bot token), relayfile's Slack VFS provider (user token), and relaycast-n8n-bridge (posting token). No duplicate auth flows. Cross-surface session bridging (sessions package): - crossSurfaceAffinity: 'user-identity' — same user on different surfaces attaches to the same session - surfaceAffinityTtl: 4h — surfaces lose affinity after inactivity - proactiveSurfacePreference: 'most-recent-active' - urgentDelivery: 'all-surfaces' — critical findings go to both Surface routing rules: - Replies go to the surface that received the message - Urgent/spend reports go to all surfaces - Structured Sage plans prefer Slack (better for long-form) - Daily standups prefer Telegram (personal, not team-facing) Slack thread context enrichment: thread history (up to 20 messages) fed into turn-context as enrichment when user invokes Karen inside a thread. Updated: section 1 (three properties), component stack, new section 5 (multi-surface + bridging), persona JSON surfaceBridging field, integration onboarding (Nango Slack as step 1), comparison table, Phase 0 roadmap, local/hosted table. https://claude.ai/code/session_0161bPaoc8gKK3JgZZDEdt2V
Adds new §5 State Layer covering KarenUserDO as the authoritative per-user
database. Motivated by state concerns that cannot live on local disk:
- Nango OAuth tokens: security risk on disk; inaccessible if compute moves
- Cross-surface session bridge: Telegram + Slack handlers may be separate
processes; need shared strongly-consistent session state
- Budget gate: two concurrent turns must not both pass a $75 ceiling —
requires atomic read-modify-write, not two local SQLite reads
- Proactive workflow state: must survive Mac mini restarts
- Memory: must be queryable, not injected wholesale
KarenUserDO SQLite schema (one DO per user):
nango_connections — OAuth tokens, refresh tokens, scopes, metadata
sessions — cross-surface session rows with bridge_session_id
messages + FTS5 — full transcript with unicode61 FTS for cross-session recall
budget — monthly token spend with atomic check-and-record
workflow_state — proactive watches, scheduled jobs, continuations
memory + FTS5 — persistent facts, preferences, workflow patterns
DO Alarms replace relaycron in hosted path: workflow_state rows drive
per-user scheduling; alarm fires, executes due items, reschedules to next.
Budget gate: DO single-writer model serializes concurrent turns — no race
condition possible. Hermes and OpenClaw have no equivalent.
Cross-surface bridge: getOrCreateBridgeSession() uses bridge_session_id
to link Telegram and Slack sessions in the DO — strongly consistent.
Nango webhook fires → DO upsert_nango_connection. Runtime fetches token
from DO per turn. Token never held in memory across turns.
Mac mini uses KarenDOClient (thin HTTP SDK) to talk to DO proxy Worker.
burn.sqlite remains for local rtk/tilth/workshop attribution only; DO
is authoritative for all budget, session, and memory state.
Also: §5.8 maps each agent-assistant package to its DO implementation;
§5.9 comparison table vs Hermes SQLite and OpenClaw files.
Updated: section 1 (4th property), component stack (DO layer), §7
local/hosted table split into Compute vs State, comparison table (DO rows),
Phase 0 roadmap (new DO bootstrap phase before Mac mini runtime).
https://claude.ai/code/session_0161bPaoc8gKK3JgZZDEdt2V
📝 WalkthroughWalkthroughThis PR adds two architecture specification documents: OpenKaren (hosted personal agent with token-conscious runtime, Durable Object persistence, proactive triggers, multi-surface bridging, and Relay↔n8n automation) and Synthesis Superior Assistant (SDK design with skills, dynamic trait overlays, surface/execution adapters, negotiation matrix, and migration paths). ChangesOpenKaren Hosted Product Architecture
Superior Assistant Synthesis Architecture
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+573
to
+577
| ON CONFLICT(integration_id) DO UPDATE SET | ||
| connection_id = excluded.connection_id, | ||
| expires_at = excluded.expires_at, | ||
| metadata = excluded.metadata, | ||
| updated_at = excluded.updated_at |
Contributor
There was a problem hiding this comment.
🔴 upsertNangoConnection ON CONFLICT clause silently drops scopes update
The upsertNangoConnection spec code includes scopes in the INSERT column list (line 571) but omits scopes = excluded.scopes from the ON CONFLICT(integration_id) DO UPDATE SET clause (lines 573–577). If implemented as written, when a Nango OAuth connection is re-authorized with different scopes (e.g., user grants additional permissions), the stored scopes value won't be updated — it will remain stale from the original insert. This would cause the runtime to believe a connection has narrower permissions than actually granted (or vice versa), leading to incorrect authorization decisions downstream.
Suggested change
| ON CONFLICT(integration_id) DO UPDATE SET | |
| connection_id = excluded.connection_id, | |
| expires_at = excluded.expires_at, | |
| metadata = excluded.metadata, | |
| updated_at = excluded.updated_at | |
| ON CONFLICT(integration_id) DO UPDATE SET | |
| connection_id = excluded.connection_id, | |
| expires_at = excluded.expires_at, | |
| scopes = excluded.scopes, | |
| metadata = excluded.metadata, | |
| updated_at = excluded.updated_at |
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+151
to
+152
| if (remainingRatio < 0.1) { | ||
| return { outcome: 'deny', reason: 'monthly-budget-exhausted' }; |
Contributor
There was a problem hiding this comment.
🟡 Budget deny threshold in §3.1 contradicts allow-economy tier in §5.4, making 90-100% economy tier unreachable
The spec defines contradictory behavior for the 90–100% budget range. In §3.1 (line 151), the pre-turn policy check denies all turns when remainingRatio < 0.1 (i.e., ≥90% spent): return { outcome: 'deny', reason: 'monthly-budget-exhausted' }. But in §5.4 (line 551), checkAndRecordSpend allows turns with economy tier when ratio > 0.90 (i.e., ≥90% spent), only denying at 100% (projected > limit). If both layers are implemented as written, the pre-turn policy gate would deny all requests at 90%+, making the DO's economy tier for 90–100% dead code. The persona JSON at line 880 (budget.remainingRatio <= 0.25 → economy) introduces yet a third threshold. Implementers will not know which threshold is authoritative.
Prompt for agents
The spec defines three different budget threshold behaviors for the same feature across §3.1 (lines 145-157), §5.4 (lines 536-553), and the persona JSON (lines 864-883). In §3.1, remainingRatio < 0.1 (90%+ spent) triggers deny. In §5.4, only projected > limit (100%) triggers deny, while 90-100% gets economy tier. In the persona JSON, economy kicks in at remainingRatio <= 0.25 (75%+ spent). These need to be reconciled to a single authoritative set of thresholds. The recommended fix is to pick one set of thresholds and update all three code examples to be consistent. If the intent is a two-layer check (pre-turn quick check + atomic DO gate), document which thresholds belong to which layer.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 `@docs/architecture/openkaren-spec.md`:
- Line 677: The subsection headings have drifted (e.g. "### 5.1 Surfaces:
Telegram + Slack" and "### 9.1") that do not match their parent section numbers;
update each subsection header to the correct numeric prefix to match its
enclosing section (e.g. change "### 5.1 Surfaces: Telegram + Slack" to the
proper subsection number under Section 6 and correct "### 9.1" under Section 11
accordingly), scanning for other mismatched "###" headings and adjusting their
numeric prefixes so in-file references and navigation remain consistent.
- Line 26: Replace untitled fenced code blocks (``` ) in the markdown with
language-labeled fences to satisfy MD040: e.g., use ```text for plain output,
```bash for shell snippets, and ```mermaid for diagrams; update each occurrence
noted in the review (the multiple anonymous fenced blocks in the openkaren spec)
by adding the appropriate language identifier to the opening fence so
markdownlint no longer reports MD040.
- Around line 573-577: The ON CONFLICT(integration_id) DO UPDATE SET clause
currently updates connection_id, expires_at, metadata, and updated_at but omits
provider_config_key and scopes; update the conflict-handling clause in the SQL
(the ON CONFLICT(integration_id) DO UPDATE SET block) to also set
provider_config_key = excluded.provider_config_key and scopes = excluded.scopes
so re-auth or provider config changes replace stale values (keep updated_at
as-is).
- Around line 541-548: The upsert SQL only inserts and increments
input_tokens/output_tokens but omits cache_read_tokens; update the INSERT and ON
CONFLICT clause that builds the budget row (the INSERT INTO budget (...) VALUES
(...) ON CONFLICT(period) DO UPDATE SET ...) to include cache_read_tokens in the
column list and VALUES, add "cache_read_tokens = cache_read_tokens +
excluded.cache_read_tokens" to the DO UPDATE SET list, and pass
tokens.cache_read (or the appropriate variable) as an additional parameter in
the parameter array so the new value is inserted and correctly aggregated on
conflict.
- Around line 426-431: Update the spec to explicitly state where FTS
synchronization is implemented: note that the virtual tables messages_fts and
memory_fts are created without built-in sync (no content= clause or triggers)
and that FTS population is handled by the consumer package
(`@agent-assistant/memory`) rather than embedded in the DO schema; either add a
brief migration example showing triggers or a content= relationship, or add a
clear comment in the docs near the CREATE VIRTUAL TABLE statements explaining
that implementers must implement sync (via triggers, content=, or insert hooks)
in their consumer code to avoid empty FTS tables.
In `@docs/architecture/synthesis-superior-assistant-spec.md`:
- Around line 338-348: Define a formal CapabilityDescriptor union type and use
it as the return type for adapter capability descriptions: add an interface
named CapabilityDescriptor with the specified unions for toolUse
('native-iterative' | 'adapter-mediated' | 'none'), continuationSupport
('structured' | 'opaque-resume' | 'none'), approvalInterrupts ('native' |
'adapter-mediated' | 'none'), traceDepth ('detailed' | 'standard' | 'minimal'),
attachments: boolean, and maxContextStrategy ('large' | 'medium' | 'small'),
then update the describeCapabilities() signature to return CapabilityDescriptor
so all adapters (e.g., BuiltIn, ClaudeAPI, DockerSandbox, SSH, Modal) must
conform to the typed capability shape.
- Around line 126-175: The document mentions agentskills.io but never defines or
links its schema; update the spec by adding a clear reference or embedded format
definition and point imports/exports to it: add a canonical agentskills.io URL
or an appendix section describing the agentskills schema and mapping to our
types (Skill, SkillTrigger, SkillBody) and then update SkillRegistry.import()
and SkillRegistry.export() descriptions to reference that URL/appendix and
specify expected format/version and validation rules for migration paths.
- Around line 182-191: The code block showing the process flow (starting with
"trajectory completed" and the following arrows) lacks a language specifier for
the fenced code block; update the triple-backtick fence around that flow to
include a language like `text` or `plaintext` (e.g., change ``` to ```text) so
the snippet renders correctly in
docs/architecture/synthesis-superior-assistant-spec.md and the process flow
(SkillExtractor, SkillCandidate, policy.evaluate, registry.query, turn-context
assembler) displays with proper formatting.
- Around line 245-249: The fenced code block showing the composition flow should
include a language specifier for proper rendering; update the triple-backtick
fence around the snippet that contains "AssistantTraits", "DynamicTraitOverlay",
and "TurnContextAssembler" to use a language like `text` or `plaintext` (e.g.,
change ``` to ```text) so the diagram renders correctly.
- Around line 116-122: Update the fenced block containing the principle
statements to include a language specifier for proper rendering: replace the
opening triple-backtick fence with one that includes "text" (i.e., use ```text)
so the block that lists "Product identity is canonical.", "Execution harnesses
are replaceable.", "Learning is policy-gated.", "Platform reach is surface
adapters.", and "Skills are turn-context enrichment inputs." is marked as
plaintext for correct syntax highlighting.
- Around line 469-472: There is an extra blank line inside the blockquote
separating the two quoted principles ("Learning is policy-gated. No skill or
trait overlay becomes active without an explicit approval decision." and
"Platform reach is surface adapters. No platform integration should require a
separate daemon or runtime."); fix by removing the blank line so both lines
remain in the same blockquote (or, if you intend two distinct blockquotes,
ensure each line is prefixed with '>' and insert a blank line between the two
blockquote blocks), updating the lines in the block containing those two quoted
principles.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e208e9f5-e50e-4d0a-a60a-fdf32d74baad
📒 Files selected for processing (2)
docs/architecture/openkaren-spec.mddocs/architecture/synthesis-superior-assistant-spec.md
|
|
||
| ## 2. The Full Component Stack | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks to satisfy MD040.
These fences are currently untyped and will keep markdownlint warning/failing. Use text, bash, or mermaid (where applicable).
Also applies to: 171-171, 317-317, 711-711, 776-776, 938-938, 963-963, 988-988, 1014-1014, 1038-1038, 1054-1054, 1088-1088, 1138-1138, 1146-1146, 1155-1155, 1164-1164
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/architecture/openkaren-spec.md` at line 26, Replace untitled fenced code
blocks (``` ) in the markdown with language-labeled fences to satisfy MD040:
e.g., use ```text for plain output, ```bash for shell snippets, and ```mermaid
for diagrams; update each occurrence noted in the review (the multiple anonymous
fenced blocks in the openkaren spec) by adding the appropriate language
identifier to the opening fence so markdownlint no longer reports MD040.
Comment on lines
+426
to
+431
| CREATE VIRTUAL TABLE IF NOT EXISTS messages_fts USING fts5( | ||
| content, | ||
| session_id UNINDEXED, | ||
| timestamp UNINDEXED, | ||
| tokenize = 'unicode61' | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether any sync strategy exists for FTS tables in docs/specs
rg -n -C3 "CREATE VIRTUAL TABLE IF NOT EXISTS (messages_fts|memory_fts)|CREATE TRIGGER|INSERT INTO (messages_fts|memory_fts)|content=" docs/architecture/openkaren-spec.mdRepository: AgentWorkforce/agent-assistant
Length of output: 517
🏁 Script executed:
#!/bin/bash
# Get full FTS table definitions and check for content= clause
sed -n '425,435p' docs/architecture/openkaren-spec.md
echo "---"
sed -n '470,480p' docs/architecture/openkaren-spec.md
echo "---"
# Search entire file for sync-related keywords around FTS
rg -i -n "trigger|sync|update.*fts|insert.*fts" docs/architecture/openkaren-spec.mdRepository: AgentWorkforce/agent-assistant
Length of output: 3114
🏁 Script executed:
#!/bin/bash
# Check how data is inserted into messages and memory tables
rg -n "INSERT INTO (messages|memory)" docs/architecture/openkaren-spec.md
echo "---"
# Check migration method and surrounding context
sed -n '385,395p' docs/architecture/openkaren-spec.mdRepository: AgentWorkforce/agent-assistant
Length of output: 548
🏁 Script executed:
#!/bin/bash
# Search for methods that reference messages or memory tables
rg -n -i "messages|memory" docs/architecture/openkaren-spec.md | head -30Repository: AgentWorkforce/agent-assistant
Length of output: 3298
Document where FTS table synchronization is handled.
Lines 426–430 and 471–475 create messages_fts and memory_fts FTS tables but do not show any sync mechanism (triggers, content= clause, or insert hooks). While line 651 suggests @agent-assistant/memory handles the sync, the spec should explicitly document that FTS population is the responsibility of the consumer package and is not embedded in the DO schema. Otherwise, implementers copying the schema snippet may create empty FTS tables.
Either add triggers to the migration or add a note clarifying that FTS sync is handled outside the DO.
🤖 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 `@docs/architecture/openkaren-spec.md` around lines 426 - 431, Update the spec
to explicitly state where FTS synchronization is implemented: note that the
virtual tables messages_fts and memory_fts are created without built-in sync (no
content= clause or triggers) and that FTS population is handled by the consumer
package (`@agent-assistant/memory`) rather than embedded in the DO schema; either
add a brief migration example showing triggers or a content= relationship, or
add a clear comment in the docs near the CREATE VIRTUAL TABLE statements
explaining that implementers must implement sync (via triggers, content=, or
insert hooks) in their consumer code to avoid empty FTS tables.
Comment on lines
+541
to
+548
| INSERT INTO budget (period, estimated_cost_usd, input_tokens, output_tokens, updated_at) | ||
| VALUES (?, ?, ?, ?, ?) | ||
| ON CONFLICT(period) DO UPDATE SET | ||
| estimated_cost_usd = estimated_cost_usd + excluded.estimated_cost_usd, | ||
| input_tokens = input_tokens + excluded.input_tokens, | ||
| output_tokens = output_tokens + excluded.output_tokens, | ||
| updated_at = excluded.updated_at | ||
| `, period, cost, tokens.input, tokens.output, Date.now()); |
There was a problem hiding this comment.
Track cache_read_tokens in budget write path.
Line 541 upsert math only increments input_tokens/output_tokens, even though budget schema includes cache_read_tokens. This undercounts spend attribution and can skew gating decisions.
Suggested spec patch
- INSERT INTO budget (period, estimated_cost_usd, input_tokens, output_tokens, updated_at)
- VALUES (?, ?, ?, ?, ?)
+ INSERT INTO budget (period, estimated_cost_usd, input_tokens, output_tokens, cache_read_tokens, updated_at)
+ VALUES (?, ?, ?, ?, ?, ?)
ON CONFLICT(period) DO UPDATE SET
estimated_cost_usd = estimated_cost_usd + excluded.estimated_cost_usd,
input_tokens = input_tokens + excluded.input_tokens,
output_tokens = output_tokens + excluded.output_tokens,
+ cache_read_tokens = cache_read_tokens + excluded.cache_read_tokens,
updated_at = excluded.updated_at
- `, period, cost, tokens.input, tokens.output, Date.now());
+ `, period, cost, tokens.input, tokens.output, tokens.cacheRead ?? 0, Date.now());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| INSERT INTO budget (period, estimated_cost_usd, input_tokens, output_tokens, updated_at) | |
| VALUES (?, ?, ?, ?, ?) | |
| ON CONFLICT(period) DO UPDATE SET | |
| estimated_cost_usd = estimated_cost_usd + excluded.estimated_cost_usd, | |
| input_tokens = input_tokens + excluded.input_tokens, | |
| output_tokens = output_tokens + excluded.output_tokens, | |
| updated_at = excluded.updated_at | |
| `, period, cost, tokens.input, tokens.output, Date.now()); | |
| INSERT INTO budget (period, estimated_cost_usd, input_tokens, output_tokens, cache_read_tokens, updated_at) | |
| VALUES (?, ?, ?, ?, ?, ?) | |
| ON CONFLICT(period) DO UPDATE SET | |
| estimated_cost_usd = estimated_cost_usd + excluded.estimated_cost_usd, | |
| input_tokens = input_tokens + excluded.input_tokens, | |
| output_tokens = output_tokens + excluded.output_tokens, | |
| cache_read_tokens = cache_read_tokens + excluded.cache_read_tokens, | |
| updated_at = excluded.updated_at | |
| `, period, cost, tokens.input, tokens.output, tokens.cacheRead ?? 0, Date.now()); |
🤖 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 `@docs/architecture/openkaren-spec.md` around lines 541 - 548, The upsert SQL
only inserts and increments input_tokens/output_tokens but omits
cache_read_tokens; update the INSERT and ON CONFLICT clause that builds the
budget row (the INSERT INTO budget (...) VALUES (...) ON CONFLICT(period) DO
UPDATE SET ...) to include cache_read_tokens in the column list and VALUES, add
"cache_read_tokens = cache_read_tokens + excluded.cache_read_tokens" to the DO
UPDATE SET list, and pass tokens.cache_read (or the appropriate variable) as an
additional parameter in the parameter array so the new value is inserted and
correctly aggregated on conflict.
Comment on lines
+573
to
+577
| ON CONFLICT(integration_id) DO UPDATE SET | ||
| connection_id = excluded.connection_id, | ||
| expires_at = excluded.expires_at, | ||
| metadata = excluded.metadata, | ||
| updated_at = excluded.updated_at |
There was a problem hiding this comment.
Update all mutable Nango fields on conflict.
Line 573 conflict handling skips provider_config_key and scopes. Re-auth/provider changes can leave stale connection metadata.
Suggested spec patch
ON CONFLICT(integration_id) DO UPDATE SET
connection_id = excluded.connection_id,
+ provider_config_key = excluded.provider_config_key,
expires_at = excluded.expires_at,
+ scopes = excluded.scopes,
metadata = excluded.metadata,
updated_at = excluded.updated_at📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ON CONFLICT(integration_id) DO UPDATE SET | |
| connection_id = excluded.connection_id, | |
| expires_at = excluded.expires_at, | |
| metadata = excluded.metadata, | |
| updated_at = excluded.updated_at | |
| ON CONFLICT(integration_id) DO UPDATE SET | |
| connection_id = excluded.connection_id, | |
| provider_config_key = excluded.provider_config_key, | |
| expires_at = excluded.expires_at, | |
| scopes = excluded.scopes, | |
| metadata = excluded.metadata, | |
| updated_at = excluded.updated_at |
🤖 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 `@docs/architecture/openkaren-spec.md` around lines 573 - 577, The ON
CONFLICT(integration_id) DO UPDATE SET clause currently updates connection_id,
expires_at, metadata, and updated_at but omits provider_config_key and scopes;
update the conflict-handling clause in the SQL (the ON CONFLICT(integration_id)
DO UPDATE SET block) to also set provider_config_key =
excluded.provider_config_key and scopes = excluded.scopes so re-auth or provider
config changes replace stale values (keep updated_at as-is).
Comment on lines
+126
to
+175
| ## 4. New Package: `@agent-assistant/skills` | ||
|
|
||
| This is the primary new package. It closes the gap that neither OpenClaw nor Hermes solves cleanly. | ||
|
|
||
| ### 4.1 Design Principles | ||
|
|
||
| - Skills are **turn-context enrichment inputs**, not filesystem prompt injections | ||
| - Skills are **policy-gated**: no skill promotes to active without an approval decision | ||
| - Skills are **versioned**: each promotion creates a new version, old versions are retained | ||
| - Skills are **prunable**: deprecated skills are marked, not silently deleted | ||
| - Skills are **provenance-tracked**: `'trajectory' | 'authored' | 'imported'` | ||
| - Skills are **agentskills.io compatible** for interop with Hermes and ClawHub | ||
|
|
||
| ### 4.2 Core Types | ||
|
|
||
| ```typescript | ||
| interface Skill { | ||
| id: string; | ||
| version: string; | ||
| name: string; | ||
| description: string; | ||
| trigger: SkillTrigger; | ||
| body: SkillBody; | ||
| provenance: 'trajectory' | 'authored' | 'imported'; | ||
| approvalStatus: 'pending' | 'approved' | 'deprecated'; | ||
| usageStats: SkillUsageRecord[]; | ||
| createdAt: string; | ||
| lastUsedAt?: string; | ||
| } | ||
|
|
||
| interface SkillTrigger { | ||
| type: 'semantic' | 'keyword' | 'domain' | 'explicit'; | ||
| pattern: string; | ||
| confidence?: number; // minimum match confidence for semantic triggers | ||
| } | ||
|
|
||
| interface SkillBody { | ||
| enrichmentText: string; // injected into turn-context as enrichment | ||
| toolHints?: string[]; // suggested tools to surface | ||
| voiceOverride?: Partial<AssistantTraits['voice']>; // optional turn-scoped identity shaping | ||
| } | ||
|
|
||
| interface SkillRegistry { | ||
| query(input: TurnContextInput): Promise<Skill[]>; | ||
| promote(trajectoryId: string, candidate: SkillCandidate): Promise<Skill>; | ||
| approve(skillId: string, version: string): Promise<Skill>; | ||
| deprecate(skillId: string, reason: string): Promise<void>; | ||
| import(source: 'openclaw' | 'hermes' | 'agentskills', path: string): Promise<Skill[]>; | ||
| export(skillId: string, format: 'agentskills' | 'openclaw'): Promise<SkillExport>; | ||
| } |
There was a problem hiding this comment.
Specify agentskills.io format or provide reference link.
The skills package references the agentskills.io standard multiple times (lines 137, 173, 373, 489) for interoperability with OpenClaw and Hermes, but the format specification is never defined or linked. This creates ambiguity for:
- The
SkillRegistry.import()implementation (line 173) - The
SkillRegistry.export()format (line 174) - Migration path validation (line 489)
Add a reference link to the agentskills.io specification, or include a format definition in an appendix.
🤖 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 `@docs/architecture/synthesis-superior-assistant-spec.md` around lines 126 -
175, The document mentions agentskills.io but never defines or links its schema;
update the spec by adding a clear reference or embedded format definition and
point imports/exports to it: add a canonical agentskills.io URL or an appendix
section describing the agentskills schema and mapping to our types (Skill,
SkillTrigger, SkillBody) and then update SkillRegistry.import() and
SkillRegistry.export() descriptions to reference that URL/appendix and specify
expected format/version and validation rules for migration paths.
Comment on lines
+182
to
+191
| ``` | ||
| trajectory completed | ||
| → proactive engine fires post-task analysis | ||
| → SkillExtractor analyzes trajectory for reusable patterns | ||
| → SkillCandidate created with provenance: 'trajectory' | ||
| → policy.evaluate({ type: 'skill-promotion', candidate }) | ||
| → if approved: skill.approvalStatus = 'approved', added to registry | ||
| → on next relevant turn: registry.query() returns skill as enrichment input | ||
| → turn-context assembler includes skill.body.enrichmentText | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to code fence.
The code fence should specify a language for proper rendering. Use text or plaintext for the process flow diagram.
📝 Proposed fix
-```
+```text
trajectory completed
→ proactive engine fires post-task analysis
→ SkillExtractor analyzes trajectory for reusable patterns
→ SkillCandidate created with provenance: 'trajectory'
→ policy.evaluate({ type: 'skill-promotion', candidate })
→ if approved: skill.approvalStatus = 'approved', added to registry
→ on next relevant turn: registry.query() returns skill as enrichment input
→ turn-context assembler includes skill.body.enrichmentText</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 182-182: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/architecture/synthesis-superior-assistant-spec.md around lines 182 -
191, The code block showing the process flow (starting with "trajectory
completed" and the following arrows) lacks a language specifier for the fenced
code block; update the triple-backtick fence around that flow to include a
language like text or plaintext (e.g., change totext) so the snippet
renders correctly in docs/architecture/synthesis-superior-assistant-spec.md and
the process flow (SkillExtractor, SkillCandidate, policy.evaluate,
registry.query, turn-context assembler) displays with proper formatting.
</details>
<!-- fingerprinting:phantom:triton:puma -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+245
to
+249
| ``` | ||
| stable AssistantTraits (traits package — unchanged) | ||
| + approved DynamicTraitOverlay (per user, per context) | ||
| = effectiveTraits fed into TurnContextAssembler | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to code fence.
The code fence should specify a language for proper rendering. Use text or plaintext for the composition flow diagram.
📝 Proposed fix
-```
+```text
stable AssistantTraits (traits package — unchanged)
+ approved DynamicTraitOverlay (per user, per context)
= effectiveTraits fed into TurnContextAssembler</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 245-245: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/architecture/synthesis-superior-assistant-spec.md around lines 245 -
249, The fenced code block showing the composition flow should include a
language specifier for proper rendering; update the triple-backtick fence around
the snippet that contains "AssistantTraits", "DynamicTraitOverlay", and
"TurnContextAssembler" to use a language like text or plaintext (e.g.,
change totext) so the diagram renders correctly.
</details>
<!-- fingerprinting:phantom:triton:puma -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+338
to
+348
| Each adapter's `describeCapabilities()` should return honest values: | ||
|
|
||
| | Capability | BuiltIn | ClaudeAPI | DockerSandbox | SSH | Modal | | ||
| |---|---|---|---|---|---| | ||
| | `toolUse` | `native-iterative` | `native-iterative` | `adapter-mediated` | `adapter-mediated` | `adapter-mediated` | | ||
| | `continuationSupport` | `structured` | `opaque-resume` | `structured` | `structured` | `opaque-resume` | | ||
| | `approvalInterrupts` | `native` | `none` | `adapter-mediated` | `none` | `none` | | ||
| | `traceDepth` | `detailed` | `standard` | `detailed` | `standard` | `minimal` | | ||
| | `attachments` | `false` | `true` | `false` | `false` | `false` | | ||
| | `maxContextStrategy` | `large` | `large` | `large` | `medium` | `medium` | | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Formalize capability value types.
The capability negotiation matrix uses string literal values ('native-iterative', 'adapter-mediated', 'structured', 'opaque-resume', 'native', 'none', etc.) without formal type definitions. These should be specified as union types to ensure type safety during implementation and prevent runtime errors from typos.
♻️ Proposed type definitions
Add to the execution adapter types section:
interface CapabilityDescriptor {
toolUse: 'native-iterative' | 'adapter-mediated' | 'none';
continuationSupport: 'structured' | 'opaque-resume' | 'none';
approvalInterrupts: 'native' | 'adapter-mediated' | 'none';
traceDepth: 'detailed' | 'standard' | 'minimal';
attachments: boolean;
maxContextStrategy: 'large' | 'medium' | 'small';
}Reference this type in the describeCapabilities() return signature mentioned at line 338.
🤖 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 `@docs/architecture/synthesis-superior-assistant-spec.md` around lines 338 -
348, Define a formal CapabilityDescriptor union type and use it as the return
type for adapter capability descriptions: add an interface named
CapabilityDescriptor with the specified unions for toolUse ('native-iterative' |
'adapter-mediated' | 'none'), continuationSupport ('structured' |
'opaque-resume' | 'none'), approvalInterrupts ('native' | 'adapter-mediated' |
'none'), traceDepth ('detailed' | 'standard' | 'minimal'), attachments: boolean,
and maxContextStrategy ('large' | 'medium' | 'small'), then update the
describeCapabilities() signature to return CapabilityDescriptor so all adapters
(e.g., BuiltIn, ClaudeAPI, DockerSandbox, SSH, Modal) must conform to the typed
capability shape.
Comment on lines
+469
to
+472
| Two new principles added by this synthesis: | ||
| > Learning is policy-gated. No skill or trait overlay becomes active without an explicit approval decision. | ||
|
|
||
| > Platform reach is surface adapters. No platform integration should require a separate daemon or runtime. |
There was a problem hiding this comment.
Remove blank line inside blockquote.
The blank line at line 471 between the two blockquote paragraphs may cause rendering issues in some Markdown processors. Merge into a single blockquote or separate into two distinct blockquotes.
📝 Proposed fix (option 1: single blockquote)
> Learning is policy-gated. No skill or trait overlay becomes active without an explicit approval decision.
-
+>
> Platform reach is surface adapters. No platform integration should require a separate daemon or runtime.📝 Proposed fix (option 2: separate blockquotes)
Remove the blockquote markers to make these regular paragraphs:
-> Learning is policy-gated. No skill or trait overlay becomes active without an explicit approval decision.
+Learning is policy-gated. No skill or trait overlay becomes active without an explicit approval decision.
-> Platform reach is surface adapters. No platform integration should require a separate daemon or runtime.
+Platform reach is surface adapters. No platform integration should require a separate daemon or runtime.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Two new principles added by this synthesis: | |
| > Learning is policy-gated. No skill or trait overlay becomes active without an explicit approval decision. | |
| > Platform reach is surface adapters. No platform integration should require a separate daemon or runtime. | |
| Two new principles added by this synthesis: | |
| > Learning is policy-gated. No skill or trait overlay becomes active without an explicit approval decision. | |
| > | |
| > Platform reach is surface adapters. No platform integration should require a separate daemon or runtime. |
Suggested change
| Two new principles added by this synthesis: | |
| > Learning is policy-gated. No skill or trait overlay becomes active without an explicit approval decision. | |
| > Platform reach is surface adapters. No platform integration should require a separate daemon or runtime. | |
| Two new principles added by this synthesis: | |
| Learning is policy-gated. No skill or trait overlay becomes active without an explicit approval decision. | |
| Platform reach is surface adapters. No platform integration should require a separate daemon or runtime. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 471-471: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 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 `@docs/architecture/synthesis-superior-assistant-spec.md` around lines 469 -
472, There is an extra blank line inside the blockquote separating the two
quoted principles ("Learning is policy-gated. No skill or trait overlay becomes
active without an explicit approval decision." and "Platform reach is surface
adapters. No platform integration should require a separate daemon or
runtime."); fix by removing the blank line so both lines remain in the same
blockquote (or, if you intend two distinct blockquotes, ensure each line is
prefixed with '>' and insert a blank line between the two blockquote blocks),
updating the lines in the block containing those two quoted principles.
Documents the fully cloud-hosted path (Cloudflare Workers + DO) as a first-class deployment option alongside Mac mini mode. Key changes: - §1: surface both deployment modes and the shared state layer invariant - §7: add cloud-only breakdown table showing what changes vs Mac mini, what stays identical, token consciousness tradeoffs without local tools, Karen Worker skeleton, and a step-by-step graduation path - §12: add "Fully cloud option" row to feature matrix - §14: add Phase 6 checklist for cloud-only graduation (no data migration) The core insight: because KarenUserDO was always authoritative from day one, graduating from Mac mini to cloud-only is a compute swap — the DO state layer, Nango connections, budget history, sessions, and memory are all already there. https://claude.ai/code/session_0161bPaoc8gKK3JgZZDEdt2V
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/architecture/openkaren-spec.md">
<violation number="1" location="docs/architecture/openkaren-spec.md:154">
P2: Budget-tier thresholds are inconsistent between §3.1 and §5.4. Section 3.1 maps 75%+ spent → economy, while §5.4's `checkAndRecordSpend` maps 75%+ → standard and only 90%+ → economy. One of these needs to be corrected to avoid implementing conflicting routing logic.</violation>
<violation number="2" location="docs/architecture/openkaren-spec.md:541">
P2: The `cache_read_tokens` column is defined in the `budget` table schema but is never written in this upsert. This will undercount spend attribution and can skew budget gating decisions. Add `cache_read_tokens` to both the INSERT column list and the ON CONFLICT UPDATE SET clause.</violation>
<violation number="3" location="docs/architecture/openkaren-spec.md:573">
P1: `provider_config_key` and `scopes` are included in the INSERT column list but omitted from the ON CONFLICT UPDATE SET clause. When a connection is re-authorized with different scopes (e.g., user grants additional permissions), the stored values won't be updated — the runtime will believe the connection has narrower permissions than actually granted, causing incorrect authorization decisions.</violation>
<violation number="4" location="docs/architecture/openkaren-spec.md:677">
P2: Section numbering is out of sync — subsections under §6 are labeled 5.x and subsections under §11 are labeled 9.x, making cross-references within the spec ambiguous.</violation>
<violation number="5" location="docs/architecture/openkaren-spec.md:1284">
P2: Duplicate 'Phase 1' heading in the roadmap — there are two distinct `### Phase 1:` sections (Mac Mini Runtime and Karen Persona + Token Consciousness), making the sequencing ambiguous.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
Comment on lines
+573
to
+577
| ON CONFLICT(integration_id) DO UPDATE SET | ||
| connection_id = excluded.connection_id, | ||
| expires_at = excluded.expires_at, | ||
| metadata = excluded.metadata, | ||
| updated_at = excluded.updated_at |
There was a problem hiding this comment.
P1: provider_config_key and scopes are included in the INSERT column list but omitted from the ON CONFLICT UPDATE SET clause. When a connection is re-authorized with different scopes (e.g., user grants additional permissions), the stored values won't be updated — the runtime will believe the connection has narrower permissions than actually granted, causing incorrect authorization decisions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/architecture/openkaren-spec.md, line 573:
<comment>`provider_config_key` and `scopes` are included in the INSERT column list but omitted from the ON CONFLICT UPDATE SET clause. When a connection is re-authorized with different scopes (e.g., user grants additional permissions), the stored values won't be updated — the runtime will believe the connection has narrower permissions than actually granted, causing incorrect authorization decisions.</comment>
<file context>
@@ -0,0 +1,1372 @@
+ INSERT INTO nango_connections
+ (integration_id, connection_id, provider_config_key, expires_at, scopes, metadata, updated_at, created_at)
+ VALUES (?, ?, ?, ?, ?, ?, ?, ?)
+ ON CONFLICT(integration_id) DO UPDATE SET
+ connection_id = excluded.connection_id,
+ expires_at = excluded.expires_at,
</file context>
Suggested change
| ON CONFLICT(integration_id) DO UPDATE SET | |
| connection_id = excluded.connection_id, | |
| expires_at = excluded.expires_at, | |
| metadata = excluded.metadata, | |
| updated_at = excluded.updated_at | |
| ON CONFLICT(integration_id) DO UPDATE SET | |
| connection_id = excluded.connection_id, | |
| provider_config_key = excluded.provider_config_key, | |
| expires_at = excluded.expires_at, | |
| scopes = excluded.scopes, | |
| metadata = excluded.metadata, | |
| updated_at = excluded.updated_at |
| } | ||
|
|
||
| this.sql.exec(` | ||
| INSERT INTO budget (period, estimated_cost_usd, input_tokens, output_tokens, updated_at) |
There was a problem hiding this comment.
P2: The cache_read_tokens column is defined in the budget table schema but is never written in this upsert. This will undercount spend attribution and can skew budget gating decisions. Add cache_read_tokens to both the INSERT column list and the ON CONFLICT UPDATE SET clause.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/architecture/openkaren-spec.md, line 541:
<comment>The `cache_read_tokens` column is defined in the `budget` table schema but is never written in this upsert. This will undercount spend attribution and can skew budget gating decisions. Add `cache_read_tokens` to both the INSERT column list and the ON CONFLICT UPDATE SET clause.</comment>
<file context>
@@ -0,0 +1,1372 @@
+ }
+
+ this.sql.exec(`
+ INSERT INTO budget (period, estimated_cost_usd, input_tokens, output_tokens, updated_at)
+ VALUES (?, ?, ?, ?, ?)
+ ON CONFLICT(period) DO UPDATE SET
</file context>
| - [ ] Configure Slack surface with thread-reply mode and channel allowlist | ||
| - [ ] Verify cross-surface session bridging: message on Telegram, continue on Slack | ||
|
|
||
| ### Phase 1: Karen Persona + Token Consciousness (Week 2–3) |
There was a problem hiding this comment.
P2: Duplicate 'Phase 1' heading in the roadmap — there are two distinct ### Phase 1: sections (Mac Mini Runtime and Karen Persona + Token Consciousness), making the sequencing ambiguous.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/architecture/openkaren-spec.md, line 1284:
<comment>Duplicate 'Phase 1' heading in the roadmap — there are two distinct `### Phase 1:` sections (Mac Mini Runtime and Karen Persona + Token Consciousness), making the sequencing ambiguous.</comment>
<file context>
@@ -0,0 +1,1372 @@
+- [ ] Configure Slack surface with thread-reply mode and channel allowlist
+- [ ] Verify cross-surface session bridging: message on Telegram, continue on Slack
+
+### Phase 1: Karen Persona + Token Consciousness (Week 2–3)
+
+- [ ] Define `karen.persona.json` in workforce repo
</file context>
Suggested change
| ### Phase 1: Karen Persona + Token Consciousness (Week 2–3) | |
| ### Phase 1b: Karen Persona + Token Consciousness (Week 2–3) |
|
|
||
| > *Session bridging state lives in KarenUserDO (see §5.6). The surfaces layer reads and writes session rows through the DO client; it does not hold session state in memory.* | ||
|
|
||
| ### 5.1 Surfaces: Telegram + Slack |
There was a problem hiding this comment.
P2: Section numbering is out of sync — subsections under §6 are labeled 5.x and subsections under §11 are labeled 9.x, making cross-references within the spec ambiguous.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/architecture/openkaren-spec.md, line 677:
<comment>Section numbering is out of sync — subsections under §6 are labeled 5.x and subsections under §11 are labeled 9.x, making cross-references within the spec ambiguous.</comment>
<file context>
@@ -0,0 +1,1372 @@
+
+> *Session bridging state lives in KarenUserDO (see §5.6). The surfaces layer reads and writes session rows through the DO client; it does not hold session state in memory.*
+
+### 5.1 Surfaces: Telegram + Slack
+
+Karen runs two surfaces simultaneously via `@agent-assistant/surfaces`. Slack is already partially implemented in the SDK (Slack progress stream helpers, Slack thread gate tests pass in CI). Nango manages the Slack OAuth token — no manual bot configuration required during onboarding.
</file context>
Suggested change
| ### 5.1 Surfaces: Telegram + Slack | |
| ### 6.1 Surfaces: Telegram + Slack |
| if (remainingRatio < 0.1) { | ||
| return { outcome: 'deny', reason: 'monthly-budget-exhausted' }; | ||
| } | ||
| if (remainingRatio < 0.25) { |
There was a problem hiding this comment.
P2: Budget-tier thresholds are inconsistent between §3.1 and §5.4. Section 3.1 maps 75%+ spent → economy, while §5.4's checkAndRecordSpend maps 75%+ → standard and only 90%+ → economy. One of these needs to be corrected to avoid implementing conflicting routing logic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/architecture/openkaren-spec.md, line 154:
<comment>Budget-tier thresholds are inconsistent between §3.1 and §5.4. Section 3.1 maps 75%+ spent → economy, while §5.4's `checkAndRecordSpend` maps 75%+ → standard and only 90%+ → economy. One of these needs to be corrected to avoid implementing conflicting routing logic.</comment>
<file context>
@@ -0,0 +1,1372 @@
+if (remainingRatio < 0.1) {
+ return { outcome: 'deny', reason: 'monthly-budget-exhausted' };
+}
+if (remainingRatio < 0.25) {
+ return { outcome: 'route-downgrade', targetTier: 'economy' };
+}
</file context>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
docs/architecture/openkaren-spec.md (4)
980-984: ⚡ Quick winDocument local burn attribution handling in migration.
The graduation path states "State after graduation: unchanged" but doesn't clarify what happens to local
burn.sqliteattribution data from the Mac mini. Line 846 describes burn as "local attribution only" while the DO budget table is "already authoritative," but users may wonder whether historical burn metrics (per-command rtk savings, workshop traces) are lost or should be archived before decommissioning.📋 Suggested addition
Add a note after step 7:
7. Decommission Mac mini processes (relaycron, relaycast daemon, workshop, rtk hook, n8n local instance) + + Note: Local burn.sqlite contains attribution details (per-command rtk savings, + workshop traces) for debugging and analysis. The DO budget table is the + authoritative spend record. Archive burn.sqlite if you want to preserve + historical per-tool metrics; otherwise it can be discarded.State after graduation: unchanged. KarenUserDO has the complete history...
</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/architecture/openkaren-spec.mdaround lines 980 - 984, Add a short
migration note after step 7 clarifying what happens to the Mac mini local burn
attribution (burn.sqlite) data: state whether burn.sqlite is archived, exported,
or merged into KarenUserDO and how it reconciles with the authoritative DO
budget table; mention preservation of per-command rtk savings and workshop
traces and give a minimal action (export/attach/ignore) for operators to follow
during decommissioning so historical burn metrics are not accidentally lost.</details> --- `1008-1027`: _⚡ Quick win_ **Reconcile tier condition logic with code examples.** The persona JSON execution tiers specify `task.complexity == 'high'` as a condition for premium tier (line 1013), but the budget-aware routing code example (lines 165-174) only checks `budget.remainingRatio` and doesn't evaluate task complexity. This creates an inconsistency between the declarative persona config and the implementation example. <details> <summary>♻️ Suggested fix</summary> Either update the code example to show task complexity evaluation: ```diff // If >75% of budget spent, route to cheaper model +const taskComplexity = assessComplexity(turn); // 'low' | 'medium' | 'high' const tier = policy.getRoutingTier(userId); // 'premium' | 'standard' | 'economy' const model = { - premium: 'claude-opus-4-7', // deep planning, complex reasoning + premium: taskComplexity === 'high' ? 'claude-opus-4-7' : 'claude-sonnet-4-6', standard: 'claude-sonnet-4-6', // most turns economy: 'claude-haiku-4-5', // budget-near-exhausted turns }[tier];Or simplify the persona JSON to match the current implementation:
{ "name": "premium", "harness": "built-in", "model": "claude-opus-4-7", - "condition": "budget.remainingRatio > 0.75 && task.complexity == 'high'" + "condition": "budget.remainingRatio > 0.75" },🤖 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 `@docs/architecture/openkaren-spec.md` around lines 1008 - 1027, The executionTiers entry for the "premium" tier includes a condition using task.complexity but the budget-aware routing example only checks budget.remainingRatio, creating a mismatch; fix by either (A) updating the routing example to evaluate task.complexity along with budget.remainingRatio (mirror the persona JSON condition used in the "executionTiers" object) so selection logic checks both budget.remainingRatio and task.complexity, or (B) simplify the persona JSON "condition" for "premium" to remove task.complexity so it matches the current routing example; locate the "executionTiers" JSON block (the "premium" object and its "condition" field) and the budget-aware routing code sample (the function that references budget.remainingRatio) and make the chosen change consistently across both places.
913-913: ⚡ Quick winClarify multi-user routing in Worker example.
env.USER_IDsuggests a single-user deployment, but a hosted service needs to extractuserIdfrom the incoming request and route to the correct DO per user. The current example doesn't show how user identity is determined from Telegram/Slack webhooks.📝 Suggested clarification
Add a comment showing user extraction:
function createKarenRouter(env: Env) { - // KarenDOClient still the same — Worker calls DO directly, no HTTP proxy needed - const doId = env.KAREN_DO.idFromName(env.USER_ID); - const doStub = env.KAREN_DO.get(doId); + return { + async handleTelegram(request: Request) { + const update = await request.json(); + const userId = extractUserIdFromTelegram(update); // Map Telegram chat ID to canonical userId + const doId = env.KAREN_DO.idFromName(userId); + const doStub = env.KAREN_DO.get(doId); + // ... continue with assistant creation + }, + async handleSlack(request: Request) { + const event = await request.json(); + const userId = extractUserIdFromSlack(event); // Map Slack user ID to canonical userId + const doId = env.KAREN_DO.idFromName(userId); + const doStub = env.KAREN_DO.get(doId); + // ... continue with assistant creation + }, + };🤖 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 `@docs/architecture/openkaren-spec.md` at line 913, The example currently uses a hardcoded env.USER_ID when computing doId (const doId = env.KAREN_DO.idFromName(env.USER_ID)); update the Worker example to show extracting the user identity from the incoming webhook/request (e.g., parse request body/headers for Telegram/Slack userId or team_id), assign it to a local userId variable, and then call env.KAREN_DO.idFromName(userId) so each request is routed to the correct per-user DO; reference env.KAREN_DO, idFromName, and replace env.USER_ID with the parsed userId in the snippet and add a short comment describing where the userId comes from (Telegram/Slack webhook payload or auth header).
1414-1429: ⚡ Quick winFix duplicate Phase 1 heading.
Both line 1414 ("Phase 1: Mac Mini Runtime") and line 1429 ("Phase 1: Karen Persona + Token Consciousness") use "Phase 1" with the same week range (Week 2-3). These should be disambiguated as Phase 1A/1B or combined into a single phase with subsections.
🔧 Suggested fix
-### Phase 1: Mac Mini Runtime (Week 2–3) +### Phase 1A: Mac Mini Runtime (Week 2–3) - [ ] Bootstrap `agent-assistant` SDK on Mac mini ... -### Phase 1: Karen Persona + Token Consciousness (Week 2–3) +### Phase 1B: Karen Persona + Token Consciousness (Week 2–3) - [ ] Define `karen.persona.json` in workforce repo ...🤖 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 `@docs/architecture/openkaren-spec.md` around lines 1414 - 1429, The document has duplicate "Phase 1" headings: "Phase 1: Mac Mini Runtime (Week 2–3)" and "Phase 1: Karen Persona + Token Consciousness (Week 2–3)"; disambiguate them by renaming one to "Phase 1A: Mac Mini Runtime (Week 2–3)" and the other to "Phase 1B: Karen Persona + Token Consciousness (Week 2–3)" or merge the two sections under a single "Phase 1" heading with subsections titled "Mac Mini Runtime" and "Karen Persona + Token Consciousness" so the headings are unique and the week range remains consistent.
🤖 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.
Nitpick comments:
In `@docs/architecture/openkaren-spec.md`:
- Around line 980-984: Add a short migration note after step 7 clarifying what
happens to the Mac mini local burn attribution (burn.sqlite) data: state whether
burn.sqlite is archived, exported, or merged into KarenUserDO and how it
reconciles with the authoritative DO budget table; mention preservation of
per-command rtk savings and workshop traces and give a minimal action
(export/attach/ignore) for operators to follow during decommissioning so
historical burn metrics are not accidentally lost.
- Around line 1008-1027: The executionTiers entry for the "premium" tier
includes a condition using task.complexity but the budget-aware routing example
only checks budget.remainingRatio, creating a mismatch; fix by either (A)
updating the routing example to evaluate task.complexity along with
budget.remainingRatio (mirror the persona JSON condition used in the
"executionTiers" object) so selection logic checks both budget.remainingRatio
and task.complexity, or (B) simplify the persona JSON "condition" for "premium"
to remove task.complexity so it matches the current routing example; locate the
"executionTiers" JSON block (the "premium" object and its "condition" field) and
the budget-aware routing code sample (the function that references
budget.remainingRatio) and make the chosen change consistently across both
places.
- Line 913: The example currently uses a hardcoded env.USER_ID when computing
doId (const doId = env.KAREN_DO.idFromName(env.USER_ID)); update the Worker
example to show extracting the user identity from the incoming webhook/request
(e.g., parse request body/headers for Telegram/Slack userId or team_id), assign
it to a local userId variable, and then call env.KAREN_DO.idFromName(userId) so
each request is routed to the correct per-user DO; reference env.KAREN_DO,
idFromName, and replace env.USER_ID with the parsed userId in the snippet and
add a short comment describing where the userId comes from (Telegram/Slack
webhook payload or auth header).
- Around line 1414-1429: The document has duplicate "Phase 1" headings: "Phase
1: Mac Mini Runtime (Week 2–3)" and "Phase 1: Karen Persona + Token
Consciousness (Week 2–3)"; disambiguate them by renaming one to "Phase 1A: Mac
Mini Runtime (Week 2–3)" and the other to "Phase 1B: Karen Persona + Token
Consciousness (Week 2–3)" or merge the two sections under a single "Phase 1"
heading with subsections titled "Mac Mini Runtime" and "Karen Persona + Token
Consciousness" so the headings are unique and the week range remains consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0cf978e5-9e65-4e08-bc08-ba30173dd3af
📒 Files selected for processing (1)
docs/architecture/openkaren-spec.md
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/architecture/openkaren-spec.md">
<violation number="1" location="docs/architecture/openkaren-spec.md:911">
P2: `createKarenRouter` uses `await` but is not declared `async`. This code example would fail to parse.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Fix all with cubic | Re-trigger cubic
| }, | ||
| }; | ||
|
|
||
| function createKarenRouter(env: Env) { |
There was a problem hiding this comment.
P2: createKarenRouter uses await but is not declared async. This code example would fail to parse.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/architecture/openkaren-spec.md, line 911:
<comment>`createKarenRouter` uses `await` but is not declared `async`. This code example would fail to parse.</comment>
<file context>
@@ -821,6 +827,113 @@ Single Slack connection → surfaces for conversation + VFS for data + bridge fo
+ },
+};
+
+function createKarenRouter(env: Env) {
+ // KarenDOClient still the same — Worker calls DO directly, no HTTP proxy needed
+ const doId = env.KAREN_DO.idFromName(env.USER_ID);
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deep comparison of openclaw/openclaw and NousResearch/hermes-agent against the
agent-assistant SDK. Identifies what each does well, where each falls short, and
proposes a concrete architecture that combines all three strengths.
Key additions specified:
Governing principles extended:
"Learning is policy-gated. Platform reach is surface adapters."
https://claude.ai/code/session_0161bPaoc8gKK3JgZZDEdt2V