Feat/splash engine rebuild#1
Open
AzizX-coder wants to merge 3 commits into
Open
Conversation
added 2 commits
June 11, 2026 14:41
Major multi-session work delivered as one cohesive patch: # Rebrand - Full @alpclaw/* -> @splash/* rename across 11 packages, AlpClaw class -> Splash, AlpClawError/AlpClawConfig -> SplashError/SplashConfig, ALPCLAW_* -> SPLASH_*, packages/core/src/alpclaw.ts -> splash.ts; removed legacy �lpclaw bin alias and the dead ~/.alpclaw config migration. Branding scan clean. # Engine — all 10 unique algorithms (spec sec 1) - Contract-first execution + DAG executor with retry/fallback/timeout/checkpoint - Multi-strategy verifier (exact/contains/semantic/schema/function, weighted) - Self-correction loop bounded by maxRetries - Contract evolution (per-template trial/verified/review grading) - Semantic response cache (cosine >= 0.92, signature-invalidated) - Hierarchical token budget (10pct contract / 5pct verify reserves; cheap-tier downgrade under pressure) - Adversarial classification -> trash memory (no auto-promote) - Reflector learning loop with quality scoring - Circuit-breaker provider router with cost accounting + per-task RoutingPolicy - Deterministic replay log (.splashlog artifact, provider-free reconstruction) # Gateway (new package @splash/gateway, zero new deps) - HTTP API (POST/GET runs, stop/retry, /health, /metrics) - SSE log streaming + native RFC 6455 WebSocket (/v1/ws/:id) - Auth: SHA-256-hashed API keys (constant-time) + HS256 JWT with tenant claim - Token-bucket rate limiting, multi-tenant run isolation - Prometheus metrics registry; security gate refuses 0.0.0.0+auth:none - InProcessExecutor + startGateway() wires Splash core # Tools — 12 total (5 new this arc) - New: CacheTool, TimerTool, AiTool, MonitorTool, QueueTool # Skills — 43 total (4 new this arc) - New: JsonSchemaValidatorSkill, EmailDrafterSkill, TranslatorSkill, CodeFormatSkill # Providers (14) - Real Nous Portal provider (replaces stub) with documented free + paid models # CLI - Fast-path resolver: skills/connectors/providers list+test, memory search, cache stats, config doctor, replay list, version, help — all sub-200ms, no LLM - New commands: splash replay, splash gateway, splash migrate (--dry-run), splash stats, splash doctor (wired) - New "ANSI Shadow" SPLASH banner with aqua wave motif # Migration (new @splash/migrate package) - Pure migrateHermes / migrateOpenClaw mappers (config / keys / safety / channels -> bots / mcp servers); minimal YAML reader; --dry-run support; applyPatch deep-merge # Deployment - Multi-stage distroless Dockerfile (non-root, /data/.splash volume, healthcheck script) - Docs: ARCHITECTURE, ENGINE-INTERNALS, EXTENSION-POINTS, DEPLOYMENT, ENGINE-COMPARISON, MIGRATION-HERMES, MIGRATION-OPENCLAW, IMPLEMENTATION-PLAN # Verification - 288 unit + integration tests passing across 37 test files - Zero TypeScript errors repo-wide - pnpm build (tsup) succeeds - CLI commands verified live: version, stats, doctor, skills list (52 tools/skills)
|
Warning Review limit reached
More reviews will be available in 37 minutes and 46 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe entire monorepo is renamed from AlpClaw to Splash ( ChangesSplash rename and runtime feature additions
Sequence Diagram(s)sequenceDiagram
participant CLI as examples/cli.ts
participant Splash as Splash.create()
participant Router as ProviderRouter
participant CircuitBreaker
participant AgentLoop
participant SemanticCache
participant Safety as SafetyEngine
CLI->>Splash: buildAgent()
Splash-->>CLI: splash instance
CLI->>AgentLoop: splash.createAgent(callbacks)
AgentLoop->>SemanticCache: semantic lookup(task)
alt semantic hit
SemanticCache-->>AgentLoop: cached result
else miss
AgentLoop->>Router: route(request, preferProvider)
Router->>CircuitBreaker: breakerAllows(provider)?
CircuitBreaker-->>Router: allowed
Router-->>AgentLoop: CompletionResponse(providerUsed, costUsd)
AgentLoop->>AgentLoop: self-correction loop (verify → correct → merge)
AgentLoop->>Safety: redactCredentials(finalSummary)
AgentLoop->>SemanticCache: write entry
end
AgentLoop-->>CLI: ChatRunResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
examples/cli.ts (1)
2069-2074:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRedundant path checks:
splashCharandglobalCharare identical.Both variables resolve to
~/.splash/character.md, making the secondexistsSynccheck unreachable.🛠️ Suggested fix
const splashChar = path.resolve(home, ".splash", "character.md"); - const globalChar = path.resolve(home, ".splash", "character.md"); if (fs.existsSync(localChar)) return fs.readFileSync(localChar, "utf-8"); if (fs.existsSync(splashChar)) return fs.readFileSync(splashChar, "utf-8"); - if (fs.existsSync(globalChar)) return fs.readFileSync(globalChar, "utf-8"); return undefined;🤖 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 `@examples/cli.ts` around lines 2069 - 2074, The variables splashChar and globalChar both resolve to the identical path path.resolve(home, ".splash", "character.md"), making the second existsSync check for globalChar unreachable. Update the globalChar variable to resolve to a different path location, likely in the home directory directly (such as path.resolve(home, "character.md")) to provide the intended fallback hierarchy of checking local, then splash subdirectory, then global home directory locations.packages/skills/src/built-in/web-search.ts (1)
36-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout for the external search request.
Line 36 issues an external HTTP request without a timeout, so slow upstream responses can block the skill indefinitely.
⏱️ Suggested fix
- const res = await fetch(url); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 10_000); + let res: Response; + try { + res = await fetch(url, { signal: controller.signal }); + } finally { + clearTimeout(timeout); + } if (!res.ok) { return err(createError("skill", `Search execution failed: HTTP ${res.status}`)); }🤖 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 `@packages/skills/src/built-in/web-search.ts` around lines 36 - 39, The fetch request to the external search URL lacks a timeout mechanism, which could cause the skill to hang indefinitely if the upstream response is slow. Create an AbortController with a timeout duration, pass the signal to the fetch call, and ensure any AbortError is caught and converted to an appropriate error response. This will prevent the external HTTP request from blocking the skill indefinitely.packages/skills/src/built-in/web-scraper.ts (1)
28-40:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftBlock SSRF targets before fetching user-provided URLs.
Line 28 accepts untrusted URLs and Line 34 fetches them directly. This permits requests to localhost/private/link-local/metadata endpoints from the runtime. Add protocol + hostname/IP validation before
fetch.🔒 Suggested hardening
async execute(params: Record<string, unknown>, ctx: SkillContext): Promise<Result<SkillResult>> { - const url = String(params.url || ""); + const url = String(params.url || "").trim(); const question = params.question ? String(params.question) : null; if (!url) return err(createError("validation", "web-scraper requires url")); try { + const target = new URL(url); + if (!["http:", "https:"].includes(target.protocol)) { + return err(createError("validation", "Only http/https URLs are allowed")); + } + await assertPublicHost(target.hostname); // block localhost/private/link-local/metadata + ctx.log(`Fetching URL: ${url}`); - const res = await fetch(url, { + const res = await fetch(target.toString(), {🤖 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 `@packages/skills/src/built-in/web-scraper.ts` around lines 28 - 40, The web-scraper skill accepts untrusted user-provided URLs via params.url and directly fetches them without validation, creating a Server-Side Request Forgery (SSRF) vulnerability that allows requests to localhost, private IP ranges, link-local addresses, and metadata endpoints. Before the fetch call, add URL validation that checks the protocol (whitelist http/https only), parses the hostname/IP, and blocks requests to localhost, 127.0.0.1, private IP ranges (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16), link-local addresses (169.254.0.0/16), and metadata endpoints. Return a validation error if any of these checks fail before attempting to fetch the URL.packages/connectors/src/webhook.ts (1)
101-120:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
replayDlqaction always fails due to endpoint validation.The endpoint lookup at lines 102-108 runs unconditionally before the switch. Since
replayDlqdoesn't require anendpointparameter,aliasisundefined, causing immediate failure with"Unknown webhook endpoint: undefined".Move the endpoint validation inside the
sendandfetchcases, or handlereplayDlqbefore the validation block.Proposed fix
async execute(action: string, args: Record<string, unknown>): Promise<Result<unknown>> { + if (action === "replayDlq") { + return this.replayDlq(); + } + const alias = args["endpoint"] as string; const endpoint = this.endpoints.get(alias); if (!endpoint) { return err( createError("connector", `Unknown webhook endpoint: ${alias}. Register it first.`), ); } switch (action) { case "send": return this.sendPost(alias, endpoint, args["payload"] as Record<string, unknown>); case "fetch": return this.sendGet(endpoint, args["queryParams"] as Record<string, string> | undefined); - case "replayDlq": - return this.replayDlq(); default: return err(createError("connector", `Unknown webhook action: ${action}`)); } }🤖 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 `@packages/connectors/src/webhook.ts` around lines 101 - 120, The endpoint validation block (the if statement checking if !endpoint exists) runs unconditionally before the switch statement in the execute method, but the replayDlq action does not require an endpoint parameter. This causes the validation to fail when alias is undefined. Move the endpoint validation logic inside only the send and fetch cases within the switch statement where endpoints are actually needed, allowing replayDlq to execute without requiring endpoint lookup. The sendPost and sendGet calls should perform their own endpoint validation after being moved inside their respective cases.
🟠 Major comments (18)
packages/core/src/tui/app.tsx-314-319 (1)
314-319:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInput handler conflict: MemoryPane's
useInputfires alongside TuiApp's.Both
MemoryPaneandTuiAppregisteruseInputhandlers for the same keys (↑/↓, j/k). In Ink, all registered handlers receive every keystroke, so navigating runs in live mode will also move the MemoryPane cursor (and vice versa when the Memory tab is active). Consider guarding the handler with a check for the active tab or lifting cursor state to the parent.🤖 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 `@packages/core/src/tui/app.tsx` around lines 314 - 319, The useInput handler in the TuiApp component (which handles the upArrow, downArrow, j, and k keys to move the cursor via setCursor) is firing even when the MemoryPane tab is active, causing both components to respond to the same keypresses simultaneously. Guard the handler logic by checking if the current active tab is not the MemoryPane tab before executing the setCursor calls, so that keyboard navigation only works for the currently active tab. This will prevent input conflicts between TuiApp's cursor navigation and MemoryPane's own input handling.packages/safety/src/engine.ts-175-176 (1)
175-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSlack token regex only redacts/detects a token prefix, not full canonical tokens.
The current pattern stops at the first hyphenated segment. For typical Slack tokens (
xoxb-...-...-...), trailing segments can remain unredacted/unreported.🛡️ Proposed fix
- /xox[baprs]-[a-zA-Z0-9]{10,}/, // Slack + /\bxox[baprs]-[A-Za-z0-9]{8,}(?:-[A-Za-z0-9]{4,})+\b/, // Slack (multi-segment) - { re: /xox[baprs]-[a-zA-Z0-9]{10,}/g, label: "slack-token" }, + { re: /\bxox[baprs]-[A-Za-z0-9]{8,}(?:-[A-Za-z0-9]{4,})+\b/g, label: "slack-token" },Also applies to: 200-201
🤖 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 `@packages/safety/src/engine.ts` around lines 175 - 176, The Slack token regex pattern at the specified location only matches partial tokens by stopping after the first hyphenated segment. Update the regex pattern to match the complete Slack token format which includes multiple hyphenated segments (xoxb-...-...-...). The pattern should be modified to account for the full token structure with additional hyphen-separated alphanumeric components. Also apply the same fix to the similar pattern mentioned at lines 200-201.package.json-3-3 (1)
3-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBreaking CLI contract needs a major version bump (or compatibility alias).
Line 37 removes the
alpclawbin, which is a breaking CLI change for existing users, but Line 3 only bumps patch version. Please either restore a compatibility alias for one transition release or bump to a major version.Suggested fix (major bump path)
- "version": "2.5.1", + "version": "3.0.0",Also applies to: 37-37
🤖 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 `@package.json` at line 3, The removal of the alpclaw bin entry represents a breaking change to the CLI contract, which requires a major version bump rather than the current patch version bump. Update the version field in package.json from the current patch version (2.5.1) to a major version (3.0.0) to properly communicate this breaking change to users and follow semantic versioning conventions.packages/config/src/global-store.ts-9-9 (1)
9-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore real legacy path + first-read migration to avoid losing existing config.
Line 45 points
legacyConfigDir()to.splash, so it can never target prior~/.alpclawdata. Combined with Lines 60-67 (no migration/fallback read), existing users’ legacy config (including API keys) is silently ignored.Suggested fix
export function legacyConfigDir(): string { - return path.join(os.homedir(), ".splash"); + return path.join(os.homedir(), ".alpclaw"); } export function readGlobalConfig(): GlobalConfigShape { - const p = globalConfigPath(); - if (!fs.existsSync(p)) return {}; + const p = globalConfigPath(); + if (!fs.existsSync(p)) { + const legacyPath = path.join(legacyConfigDir(), "config.json"); + if (fs.existsSync(legacyPath)) { + ensureGlobalConfigDir(); + fs.copyFileSync(legacyPath, p); + } else { + return {}; + } + } try { return JSON.parse(fs.readFileSync(p, "utf-8")) as GlobalConfigShape; } catch { return {}; } }Also applies to: 45-45, 60-67
🤖 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 `@packages/config/src/global-store.ts` at line 9, The legacyConfigDir() function at line 45 is pointing to the wrong directory path and the config reading logic at lines 60-67 lacks fallback/migration logic. To fix this, update the legacyConfigDir() function to return the actual prior legacy path (the real ~/.alpclaw directory) instead of .splash, and then add migration/fallback logic in the config reading section that attempts to read from the legacy config directory first, migrating it to the new location if found, before defaulting to the new location. This ensures existing users' configurations including API keys are properly migrated and not silently lost.bots/lib/chat-agent.ts-12-13 (1)
12-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake Splash singleton initialization atomic.
The current lazy init can race under concurrent calls and create multiple
Splash.create()instances before_splashis assigned.🔧 Suggested change
let _splash: Splash | null = null; +let _splashInit: Promise<Splash> | null = null; let _personaCache: string | undefined = undefined; @@ export async function getSplash(): Promise<Splash> { - if (!_splash) _splash = await Splash.create(); - return _splash; + if (_splash) return _splash; + if (!_splashInit) { + _splashInit = Splash.create() + .then((instance) => { + _splash = instance; + return instance; + }) + .finally(() => { + _splashInit = null; + }); + } + return _splashInit; }Also applies to: 30-32
🤖 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 `@bots/lib/chat-agent.ts` around lines 12 - 13, The lazy initialization of the _splash singleton variable is not atomic and can race under concurrent calls, causing multiple Splash.create() instances to be created. To fix this, implement a Promise-based initialization pattern for both _splash and _personaCache (as mentioned in lines 30-32) where you store the initialization promise alongside the value, and ensure that subsequent concurrent calls await the same initialization promise rather than triggering new create() calls. This ensures only one instance is ever created regardless of concurrent access patterns.bots/slack.ts-79-80 (1)
79-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait Splash bootstrap in startup path.
Fire-and-forget initialization can log readiness too early and mask bootstrap failure handling.
🔧 Suggested change
- getSplash(); // warm up the agent platform + await getSplash(); // warm up the agent platform console.log(pc.green("✓ Framework initialized."));🤖 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 `@bots/slack.ts` around lines 79 - 80, The getSplash() function call is not being awaited, which causes the console.log statement to execute before the agent platform bootstrap is complete. Add the await keyword before getSplash() to ensure the initialization completes before logging the success message and continuing with the startup sequence, allowing proper error handling if the bootstrap fails.bots/messenger.ts-66-67 (1)
66-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait framework warm-up before announcing initialization.
getSplash()should be awaited here to prevent false-ready logs and startup rejection escaping.🔧 Suggested change
- getSplash(); + await getSplash(); console.log(pc.green("✓ Framework initialized."));🤖 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 `@bots/messenger.ts` around lines 66 - 67, The getSplash() function call is not being awaited, which allows the framework initialization success message to be logged before the framework warm-up completes. Add the await keyword before the getSplash() call to ensure the asynchronous operation completes before the success log message is printed, preventing false-ready logs and ensuring startup rejections are properly handled.bots/whatsapp.ts-87-88 (1)
87-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait Splash initialization in main startup flow.
Without awaiting, startup can claim initialization success before framework creation completes.
🔧 Suggested change
- getSplash(); + await getSplash(); console.log(pc.green("✓ Framework initialized."));🤖 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 `@bots/whatsapp.ts` around lines 87 - 88, The getSplash() function call in the main startup flow is not being awaited, which allows the initialization success message to be logged before the splash initialization actually completes. Add the await keyword before the getSplash() call to ensure it finishes before the console.log statement executes and reports that the framework is initialized.bots/telegram.ts-39-40 (1)
39-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait connector bootstrap before creating the bot runtime.
Initialization is currently fire-and-forget; awaiting avoids false-ready startup and unhandled bootstrap rejection paths.
🔧 Suggested change
- getSplash(); + await getSplash(); const bot = new Telegraf(token);🤖 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 `@bots/telegram.ts` around lines 39 - 40, Add an await keyword to the getSplash() function call to ensure the connector bootstrap completes before the Telegraf bot instance is created. The getSplash() call should be awaited on the line before the new Telegraf(token) instantiation to guarantee proper initialization sequencing and rejection handling.bots/discord.ts-80-81 (1)
80-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait Splash bootstrap before logging framework readiness.
getSplash()is async; fire-and-forget startup can report success before initialization finishes and can drop bootstrap failures into unhandled rejection behavior.🔧 Suggested change
- getSplash(); + await getSplash(); console.log(pc.green("✓ Framework initialized."));🤖 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 `@bots/discord.ts` around lines 80 - 81, The getSplash() function is asynchronous but is being called without awaiting it, causing the framework readiness log message to print before the splash initialization actually completes. This creates a race condition where the success message can be logged before startup finishes, and any errors in getSplash() become unhandled rejections. Add the await keyword before the getSplash() call to ensure it completes before logging the success message with pc.green().packages/core/src/verifier.ts-175-177 (1)
175-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle non-serializable outputs safely in strategy evaluation.
JSON.stringify(output)can throw on circular structures, which turns verification into a runtime failure.Suggested fix
- const text = typeof output === "string" ? output : JSON.stringify(output ?? ""); + let text: string; + if (typeof output === "string") { + text = output; + } else { + try { + text = JSON.stringify(output ?? ""); + } catch { + text = String(output); + } + }🤖 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 `@packages/core/src/verifier.ts` around lines 175 - 177, The evaluateStrategy method calls JSON.stringify on the output parameter without error handling, which can throw when the output contains circular structures or non-serializable objects, causing the verification to fail with a runtime error. Wrap the JSON.stringify call in a try-catch block to safely handle serialization failures, and provide a fallback string representation (such as "output") when serialization throws an error instead of propagating the exception.packages/core/src/verifier.ts-160-168 (1)
160-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-positive weights to preserve the
0..1score contract.With
weight <= 0, scoring can become inconsistent (including impossible ratios) whilepassedis computed independently.Suggested fix
for (const criterion of criteria) { const weight = criterion.weight ?? 1; + if (!Number.isFinite(weight) || weight <= 0) { + results.push({ + name: criterion.name, + passed: false, + weight: 0, + detail: "invalid weight: must be a finite number > 0", + }); + continue; + } totalWeight += weight; const { passed, detail } = this.evaluateStrategy(criterion.strategy, output); if (passed) passedWeight += weight; results.push({ name: criterion.name, passed, weight, detail }); } - const score = totalWeight > 0 ? passedWeight / totalWeight : 1; + const score = totalWeight > 0 ? passedWeight / totalWeight : 0;🤖 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 `@packages/core/src/verifier.ts` around lines 160 - 168, Add validation in the results loop to reject non-positive weights and preserve the 0..1 score contract. When processing each criterion, check that criterion.weight is positive (greater than 0) before including it in the weight calculations for totalWeight and passedWeight. Skip or throw an error for any criterion with weight <= 0 to ensure the score calculation remains consistent and produces values within the 0..1 range.packages/core/src/verifier.ts-92-97 (1)
92-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
schemastrategy currently passes any object without enforcing the schema.The branch only checks “is object” and ignores
strategyOptions.schema, which can incorrectly mark invalid outputs as verified.Suggested fix
- } else if (strategy === "schema") { - if (typeof output !== "object" || output === null) { - issues.push("Schema verification failed: output is not an object"); - } - // Basic schema check could be done here based on strategyOptions.schema + } else if (strategy === "schema") { + const schema = strategyOptions?.schema as Record<string, unknown> | undefined; + if (!schema) { + issues.push("Schema verification requires strategyOptions.schema"); + } else { + const r = this.validateSchema(schema, output); + if (!r.passed) issues.push(`Schema verification failed: ${r.detail}`); + }🤖 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 `@packages/core/src/verifier.ts` around lines 92 - 97, In the schema strategy branch where strategy === "schema", the current implementation only checks if the output is an object but completely ignores the actual schema validation against strategyOptions.schema. You need to implement proper schema validation by checking if the output structure matches the schema requirements defined in strategyOptions.schema, and if validation fails, push descriptive error messages to the issues array that indicate which parts of the schema validation failed. Replace the placeholder comment about basic schema check with actual validation logic that uses the schema from strategyOptions.packages/core/src/task-manager.ts-51-58 (1)
51-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate supplied step IDs and fix empty-batch return behavior.
Line 57 / Line 74 accept caller IDs without duplicate checks, and Line 78 uses
slice(-steps.length)which returns the full step list whensteps.length === 0. This can breakupdateStep(...)ID targeting and return incorrect results to callers.Suggested patch
addStep(taskId: string, description: string, toolName?: string, stepId?: string): TaskStep { const task = this.tasks.get(taskId); if (!task) throw new Error(`Task not found: ${taskId}`); + if (stepId && task.steps.some((s) => s.id === stepId)) { + throw new Error(`Duplicate step id: ${stepId}`); + } const step: TaskStep = { id: stepId || generateId("step"), description, status: "pending", toolName, }; @@ addSteps(taskId: string, steps: Array<{ id: string; description: string; toolName?: string }>): TaskStep[] { const task = this.tasks.get(taskId); if (!task) throw new Error(`Task not found: ${taskId}`); + if (steps.length === 0) return []; + + const existing = new Set(task.steps.map((s) => s.id)); + const incoming = new Set<string>(); + for (const s of steps) { + if (!s.id) throw new Error("Step id is required"); + if (existing.has(s.id) || incoming.has(s.id)) { + throw new Error(`Duplicate step id: ${s.id}`); + } + incoming.add(s.id); + } for (const s of steps) { const step: TaskStep = { id: s.id, description: s.description, status: "pending", toolName: s.toolName }; task.steps.push(step); } task.updatedAt = Date.now(); return task.steps.slice(-steps.length); }Also applies to: 69-79
🤖 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 `@packages/core/src/task-manager.ts` around lines 51 - 58, The addStep method accepts an optional stepId parameter without validating that it is unique within the task, which can cause duplicate IDs and break updateStep targeting. Add a duplicate check in the addStep method to verify that any supplied stepId does not already exist in the task's steps array before creating the new step. Additionally, there is a bug in the step return logic around line 78 where slice(-steps.length) fails to handle the edge case when steps.length equals zero, causing incorrect results to be returned to callers. Fix this by adding proper handling for the empty steps case before applying the slice operation.packages/core/src/runs/run-manager.ts-198-203 (1)
198-203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBackground launcher resolution is tied to home/storage path, which can disable true detached runs.
Line 200 uses
SPLASH_HOME(duplicated in the OR chain) to discoverbin/splash.mjs.SPLASH_HOMEis typically a data directory, so this can miss the launcher and silently fall back to inline execution.Suggested patch
private spawnBackground(id: string, task: string): void { // Find bin/splash.mjs launcher so we can re-exec ourselves with a detached child. - const splashHome = process.env.SPLASH_HOME || process.env.SPLASH_HOME || process.cwd(); - const launcher = path.join(splashHome, "bin", "splash.mjs"); + const launcher = + process.argv[1] && fs.existsSync(process.argv[1]) + ? process.argv[1] + : path.join(process.cwd(), "bin", "splash.mjs"); if (!fs.existsSync(launcher)) { // Fall back: run inline synchronously but don't block caller — fire and forget. void this.runInline(id, task); return;🤖 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 `@packages/core/src/runs/run-manager.ts` around lines 198 - 203, The spawnBackground method has a duplicate SPLASH_HOME check in the environment variable fallback chain and incorrectly relies on SPLASH_HOME (a data directory) to locate the bin/splash.mjs launcher, which causes the launcher to be missed and falls back to inline execution. Remove the duplicate SPLASH_HOME reference in the OR chain and instead resolve the launcher path relative to the application installation location using a reliable path like __dirname or the package installation directory, ensuring the launcher is always found for true detached background execution.packages/core/src/agent-loop.ts-334-352 (1)
334-352:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact replay-log payloads before disk persistence.
Line 345 and Line 346 write raw step output/error into replay logs. That bypasses the summary redaction path and can persist credentials/PII to local storage.
Suggested patch
this.replayLog.record({ runId: task.id, task: taskDescription, objective: contract.objective, contract, steps: execResult.stepResults.map((s) => ({ id: s.stepId, tool: s.tool, success: s.success, - output: String(s.output ?? ""), - error: s.error, + output: this.safety.redactCredentials(String(s.output ?? "")), + error: s.error ? this.safety.redactCredentials(s.error) : undefined, durationMs: s.durationMs, })),🤖 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 `@packages/core/src/agent-loop.ts` around lines 334 - 352, The replayLog.record() call writes raw, unredacted step output and error data to disk persistence, which can expose credentials and PII. In the steps array mapping within replayLog.record(), replace the raw values for the output and error fields with their redacted/summarized equivalents. Apply the same redaction logic that is used in the summary redaction path to both the output field (currently String(s.output ?? "")) and the error field (currently s.error) to ensure sensitive data is not persisted to local storage.packages/connectors/src/webhook.ts-180-197 (1)
180-197:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure
.splashdirectory exists before writing DLQ file.If
~/.splashdoesn't exist (e.g., fresh install),fs.writeFilewill fail withENOENT. Create the directory first.Proposed fix
private async addToDlq(alias: string, payload: Record<string, unknown>) { try { const fs = await import("node:fs/promises"); const os = await import("node:os"); const path = await import("node:path"); - const dlqPath = path.join(os.homedir(), ".splash", "webhook_dlq.json"); + const splashDir = path.join(os.homedir(), ".splash"); + await fs.mkdir(splashDir, { recursive: true }); + const dlqPath = path.join(splashDir, "webhook_dlq.json"); let dlq: any[] = [];🤖 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 `@packages/connectors/src/webhook.ts` around lines 180 - 197, The addToDlq method writes to a file in ~/.splash/webhook_dlq.json without ensuring the directory exists first, which causes fs.writeFile to fail with ENOENT on fresh installations. Before calling fs.writeFile on dlqPath, create the directory using fs.mkdir with the recursive option set to true to create any parent directories needed. This ensures the ~/.splash directory exists before attempting to write the webhook_dlq.json file.packages/connectors/src/webhook.ts-155-177 (1)
155-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick win4xx client errors are incorrectly retried.
When
res.okis false with status < 500 (e.g., 400, 401, 403, 404):
- Line 155 condition fails (not 5xx)
- Line 165 throws an error
- Caught at line 167, which retries if
attempt < maxRetriesClient errors won't resolve with retries; this wastes time and delays the failure path. Also, line 177 is unreachable dead code since all loop paths exit via
return.Proposed fix
if (!res.ok && res.status >= 500 && attempt < maxRetries) { attempt++; await new Promise((r) => setTimeout(r, 1000 * Math.pow(2, attempt))); // Exponential backoff continue; } const text = await res.text(); let body: unknown; try { body = JSON.parse(text); } catch { body = text; } - if (!res.ok) throw new Error(`HTTP ${res.status}: ${text.slice(0, 100)}`); + if (!res.ok) { + await this.addToDlq(alias, payload); + return err(createError("connector", `Webhook POST failed: HTTP ${res.status}`, { retryable: res.status >= 500 })); + } return ok({ status: res.status, body }); } catch (cause) { if (attempt < maxRetries) { attempt++; await new Promise((r) => setTimeout(r, 1000 * Math.pow(2, attempt))); continue; } await this.addToDlq(alias, payload); return err(createError("connector", "Webhook POST failed, added to DLQ", { cause, retryable: false })); } } - return err(createError("connector", "Webhook POST exhausted retries")); }🤖 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 `@packages/connectors/src/webhook.ts` around lines 155 - 177, The catch block starting at line 167 retries all errors when attempt < maxRetries, but 4xx client errors should not be retried since they won't resolve with retries. Modify the error handling in the catch block to distinguish between retryable errors (5xx or transient network errors) and non-retryable errors (4xx client errors). For 4xx errors, immediately add to DLQ without retrying; for other errors, apply the existing retry logic. Additionally, line 177 containing the final error return is unreachable dead code since all code paths through the loop exit via return statements, so this line can be removed.
🟡 Minor comments (9)
examples/cli.ts-1936-1937 (1)
1936-1937:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSuspicious path resolution:
promptsDirgoes two levels above~/.splash.
path.resolve(splashDir, "..", "..", "prompts")resolves to~/prompts(two levels up from~/.splash). This is likely unintended and may have been meant to bepath.join(splashDir, "prompts")or left over from a different directory structure.🤖 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 `@examples/cli.ts` around lines 1936 - 1937, The path resolution for the promptsDir variable is using an unintended two-level directory traversal with `path.resolve(splashDir, "..", "..", "prompts")` which results in going up two levels from the splash directory instead of staying within it. Fix this by replacing the path.resolve call with `path.join(splashDir, "prompts")` to correctly resolve to the prompts subdirectory within the splash directory structure.examples/cli.ts-2018-2018 (1)
2018-2018:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate fallback:
SPLASH_HOME || SPLASH_HOME.The fallback chain reads the same env var twice. This was likely a copy-paste issue from
ALPCLAW_HOME || SPLASH_HOMEmigration.🛠️ Suggested fix
- const splashHome = process.env.SPLASH_HOME || process.env.SPLASH_HOME || process.cwd(); + const splashHome = process.env.SPLASH_HOME || process.cwd();🤖 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 `@examples/cli.ts` at line 2018, The splashHome variable assignment contains a duplicate fallback chain where process.env.SPLASH_HOME is checked twice in a row before falling back to process.cwd(). Remove the redundant second process.env.SPLASH_HOME check so the fallback chain only evaluates the environment variable once before defaulting to process.cwd().examples/cli.ts-821-823 (1)
821-823:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnused variable:
afrombuildAgent()is never used.The agent is created but never referenced. This wastes initialization time and resources. If not needed, remove it.
🛠️ Suggested fix
const { TuiApp, RunManager } = await import("`@splash/core`"); - const a = await buildAgent(); const manager = new RunManager();🤖 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 `@examples/cli.ts` around lines 821 - 823, The variable a assigned from the buildAgent() function call is never used in the subsequent code, wasting initialization resources. Remove the line const a = await buildAgent(); entirely since the agent instance is not referenced anywhere after creation.examples/cli.ts-228-230 (1)
228-230:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnreachable code:
doctorcase is handled before the switch.Lines 137-140 already handle
cmd === "doctor"and return early, so this switch case at lines 228-230 will never execute.🛠️ Suggested fix
- case "doctor": - await runDoctor(args.slice(1)); - return;🤖 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 `@examples/cli.ts` around lines 228 - 230, The switch statement contains a "doctor" case that calls runDoctor, but this case is unreachable because the same command is already handled with an early return before the switch statement. Remove the "doctor" case from the switch block entirely since the early check at lines 137-140 (which handles cmd === "doctor" and returns) makes this switch case dead code.packages/safety/src/engine.test.ts-132-133 (1)
132-133:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid embedding secret-shaped literals directly in test source.
This literal is flagged by Betterleaks as a generic API key. Even when fake, it can create CI/security-noise and triage overhead.
🧹 Proposed tweak
- it("redacts a Slack token", () => { - const out = engine.redactCredentials("token=xoxb-1234567890-abcdef"); + it("redacts a Slack token", () => { + const token = ["xoxb", "1234567890", "abcdef"].join("-"); + const out = engine.redactCredentials(`token=${token}`); expect(out).toContain("[REDACTED:slack-token]"); });🤖 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 `@packages/safety/src/engine.test.ts` around lines 132 - 133, The test in the redactCredentials test case embeds a secret-shaped literal that resembles a real Slack token format and is being flagged by security scanning tools. Replace the fake token string in the engine.redactCredentials call with a generic placeholder that does not match real API key patterns (such as common prefixes like xoxb-, xoxp-, etc.), while ensuring the test still validates that the redaction mechanism works correctly for the test input.Source: Linters/SAST tools
packages/safety/src/engine.ts-179-184 (1)
179-184:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
scanCredentialsreturns at most one match per pattern.Using
text.match(...)once per regex drops additional credentials of the same type, so thematchesarray is incomplete.✅ Proposed fix
const matches: string[] = []; for (const pattern of credentialPatterns) { - const match = text.match(pattern); - if (match) { - matches.push(match[0]); - } + const flags = pattern.flags.includes("g") ? pattern.flags : `${pattern.flags}g`; + const globalPattern = new RegExp(pattern.source, flags); + for (const m of text.matchAll(globalPattern)) { + matches.push(m[0]); + } }🤖 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 `@packages/safety/src/engine.ts` around lines 179 - 184, The `text.match(pattern)` call in the scanCredentials function only returns the first match for each pattern, causing the matches array to be incomplete when multiple credentials of the same type exist. Replace the single match call with text.matchAll(pattern) to capture all matches for each pattern, then iterate through all matched results (not just the first one at index [0]) to add all credentials to the matches array.packages/core/src/index.ts-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorAdd
CriterionResultto the public API exports inindex.ts.
CriterionResultis exported frompackages/core/src/verifier.tsbut not re-exported frompackages/core/src/index.ts, creating an inconsistency. Since related types likeWeightedCriterionandWeightedVerificationare already re-exported,CriterionResultshould be included for API completeness.Update line 5 of
packages/core/src/index.ts:export { Verifier, type VerificationResult, type VerifierStrategy, type CriterionResult, type WeightedCriterion, type WeightedVerification } from "./verifier.js";🤖 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 `@packages/core/src/index.ts` at line 5, The CriterionResult type is missing from the public API exports in index.ts. Add type CriterionResult to the export statement on line 5 where Verifier, VerificationResult, VerifierStrategy, WeightedCriterion, and WeightedVerification are being exported from ./verifier.js. This ensures all related types from the verifier module are consistently available in the public API.packages/connectors/src/webhook.ts-131-131 (1)
131-131:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame nullish coalescing issue.
Consistent with line 37, use
??instead of||to allow explicitmaxRetries: 0.Proposed fix
- const maxRetries = endpoint.maxRetries || 3; + const maxRetries = endpoint.maxRetries ?? 3;🤖 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 `@packages/connectors/src/webhook.ts` at line 131, The maxRetries assignment in the webhook.ts file uses the OR operator (`||`) instead of the nullish coalescing operator (`??`). Replace `||` with `??` in the line `const maxRetries = endpoint.maxRetries || 3;` to properly handle the case where maxRetries is explicitly set to 0, which should be allowed as a valid value but would currently be overridden by the default value of 3.packages/connectors/src/webhook.ts-37-37 (1)
37-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse nullish coalescing to preserve explicit zero.
maxRetries || 3treats0as falsy, so an explicitmaxRetries: 0(no retries) becomes3. Use??to only default onnull/undefined.Proposed fix
- this.endpoints.set(alias, { url, headers, hmacSecret, maxRetries: maxRetries || 3 }); + this.endpoints.set(alias, { url, headers, hmacSecret, maxRetries: maxRetries ?? 3 });🤖 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 `@packages/connectors/src/webhook.ts` at line 37, The maxRetries parameter in the endpoints.set call uses the logical OR operator which treats 0 as a falsy value, causing an explicit maxRetries of 0 to default to 3 instead. Replace the || operator with the nullish coalescing operator ?? for the maxRetries defaulting logic so that only null or undefined values default to 3, while preserving an explicit 0 value for no retries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7211fd8b-e70f-45c7-9dcf-95cbeddc11b4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (110)
.env.example.gitignorebin/alpclaw.mjsbin/splash.mjsbots/discord.tsbots/lib/chat-agent.tsbots/messenger.tsbots/slack.tsbots/telegram.tsbots/whatsapp.tsexamples/cli.tsexamples/demo-standalone.tsexamples/package.jsonpackage.jsonpackages/config/package.jsonpackages/config/src/global-store.tspackages/config/src/index.tspackages/config/src/loader.tspackages/config/src/schema.tspackages/connectors/package.jsonpackages/connectors/src/browser.tspackages/connectors/src/connector.tspackages/connectors/src/database.tspackages/connectors/src/filesystem.test.tspackages/connectors/src/filesystem.tspackages/connectors/src/git.tspackages/connectors/src/github.tspackages/connectors/src/http.tspackages/connectors/src/messaging.tspackages/connectors/src/registry.tspackages/connectors/src/terminal.tspackages/connectors/src/webhook.tspackages/core/package.jsonpackages/core/src/agent-loop.tspackages/core/src/alpclaw.tspackages/core/src/index.tspackages/core/src/planner.tspackages/core/src/runs/index.tspackages/core/src/runs/run-manager.tspackages/core/src/runs/run-store.tspackages/core/src/self-corrector.tspackages/core/src/task-manager.tspackages/core/src/tui/app.tsxpackages/core/src/verifier.test.tspackages/core/src/verifier.tspackages/memory/package.jsonpackages/memory/src/episodic.tspackages/memory/src/expanded-memory.test.tspackages/memory/src/file-store.tspackages/memory/src/manager.tspackages/memory/src/memory.test.tspackages/memory/src/profile.tspackages/memory/src/skill-memory.tspackages/memory/src/store.tspackages/providers/package.jsonpackages/providers/src/cerebras.tspackages/providers/src/claude.tspackages/providers/src/cohere.tspackages/providers/src/gemini.tspackages/providers/src/groq.tspackages/providers/src/index.tspackages/providers/src/mistral.tspackages/providers/src/nous.tspackages/providers/src/nvidia.tspackages/providers/src/ollama.tspackages/providers/src/openai.tspackages/providers/src/provider.tspackages/providers/src/router.test.tspackages/providers/src/router.tspackages/safety/package.jsonpackages/safety/src/engine.test.tspackages/safety/src/engine.tspackages/safety/src/policies.tspackages/safety/src/validator.tspackages/skills/package.jsonpackages/skills/src/built-in/api-integrator.tspackages/skills/src/built-in/code-edit.tspackages/skills/src/built-in/code-reviewer.tspackages/skills/src/built-in/config-editor.tspackages/skills/src/built-in/data-analyst.tspackages/skills/src/built-in/database-admin.tspackages/skills/src/built-in/debugger.tspackages/skills/src/built-in/deployer.tspackages/skills/src/built-in/docs-generator.tspackages/skills/src/built-in/git-helper.tspackages/skills/src/built-in/linear-triage.tspackages/skills/src/built-in/message-drafter.tspackages/skills/src/built-in/notion-sync.tspackages/skills/src/built-in/pr-creator.tspackages/skills/src/built-in/python-runner.tspackages/skills/src/built-in/repo-analysis.tspackages/skills/src/built-in/shell-runner.tspackages/skills/src/built-in/sql-builder.tspackages/skills/src/built-in/subagent-runner.tspackages/skills/src/built-in/task-queue.tspackages/skills/src/built-in/task-summarizer.tspackages/skills/src/built-in/test-runner.tspackages/skills/src/built-in/web-scraper.tspackages/skills/src/built-in/web-search.tspackages/skills/src/index.tspackages/skills/src/registry.tspackages/skills/src/skill.tspackages/utils/package.jsonpackages/utils/src/logger.tspackages/utils/src/theme.tspackages/utils/src/types.tstests/integration.test.tstsconfig.jsontsup.config.tsvitest.config.ts
💤 Files with no reviewable changes (3)
- bin/alpclaw.mjs
- bin/splash.mjs
- packages/core/src/alpclaw.ts
Comment on lines
+1432
to
+1435
| console.log(pc.dim(`Installing ${pkg} into ${skillsDir}...`)); | ||
| const child_process = await import("node:child_process"); | ||
| child_process.execSync(`npm install ${pkg}`, { stdio: "inherit", cwd: skillsDir }); | ||
| console.log(pc.green(`\n[OK] Installed ${pkg}. It will be loaded automatically.`)); |
There was a problem hiding this comment.
Command injection risk: user-supplied package name passed to shell.
pkg comes directly from CLI args and is interpolated into a shell command. A malicious input like foo; rm -rf ~ would execute arbitrary commands. Use execFileSync with an argument array instead.
🔐 Secure alternative
- child_process.execSync(`npm install ${pkg}`, { stdio: "inherit", cwd: skillsDir });
+ child_process.execFileSync("npm", ["install", pkg], { stdio: "inherit", cwd: skillsDir });📝 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
| console.log(pc.dim(`Installing ${pkg} into ${skillsDir}...`)); | |
| const child_process = await import("node:child_process"); | |
| child_process.execSync(`npm install ${pkg}`, { stdio: "inherit", cwd: skillsDir }); | |
| console.log(pc.green(`\n[OK] Installed ${pkg}. It will be loaded automatically.`)); | |
| console.log(pc.dim(`Installing ${pkg} into ${skillsDir}...`)); | |
| const child_process = await import("node:child_process"); | |
| child_process.execFileSync("npm", ["install", pkg], { stdio: "inherit", cwd: skillsDir }); | |
| console.log(pc.green(`\n[OK] Installed ${pkg}. It will be loaded automatically.`)); |
🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 1434-1434: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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 `@examples/cli.ts` around lines 1432 - 1435, The `execSync` call in the code is
vulnerable to command injection because the user-supplied `pkg` variable is
interpolated directly into the shell command string. Replace the `execSync` call
with `execFileSync` instead, and pass the npm command and package as separate
array elements in the arguments array format rather than as a single
interpolated string. This ensures the package name is treated as a literal
argument and cannot be interpreted as additional shell commands. Maintain the
same options object with stdio and cwd properties.
Source: Linters/SAST tools
Comment on lines
+88
to
+91
| if (issues.length === 0 && strategy) { | ||
| if (strategy === "exact" && output !== strategyOptions?.expected) { | ||
| issues.push(`Exact match failed. Expected: ${strategyOptions.expected}, Got: ${output}`); | ||
| suggestions.push("Check formatting or exact character sequence"); |
There was a problem hiding this comment.
Prevent runtime crash when exact strategy is used without options.
This path dereferences strategyOptions.expected without a guard, so strategy: "exact" with missing options throws and aborts verification.
Suggested fix
- if (strategy === "exact" && output !== strategyOptions?.expected) {
- issues.push(`Exact match failed. Expected: ${strategyOptions.expected}, Got: ${output}`);
- suggestions.push("Check formatting or exact character sequence");
- } else if (strategy === "schema") {
+ if (strategy === "exact") {
+ const expected = strategyOptions?.expected;
+ if (typeof expected !== "string") {
+ issues.push("Exact verification requires strategyOptions.expected (string)");
+ } else if (String(output) !== expected) {
+ issues.push(`Exact match failed. Expected: ${expected}, Got: ${String(output)}`);
+ suggestions.push("Check formatting or exact character sequence");
+ }
+ } else if (strategy === "schema") {🤖 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 `@packages/core/src/verifier.ts` around lines 88 - 91, The code in the exact
strategy validation block references strategyOptions.expected in the error
message without guarding against strategyOptions being undefined or null. The
comparison uses optional chaining but the string template does not, causing a
runtime crash when the exact strategy is used without options. Add a
null/undefined check for strategyOptions to ensure it exists before entering the
exact strategy block, either by adding strategyOptions to the condition that
checks strategy === "exact" or by wrapping the error message construction with a
proper guard.
Comment on lines
+219
to
+223
| // Check if resolved path is within any of the allowed directories | ||
| const isAllowed = allowedDirs.some((dir) => { | ||
| const allowedResolved = path.resolve(dir); | ||
| return resolved.startsWith(allowedResolved); | ||
| }); |
There was a problem hiding this comment.
Fix sandbox boundary validation to prevent path traversal bypass.
The current prefix check can incorrectly allow sibling paths (for example, an allowedDir of /sandbox also allows /sandbox-escape/...). This breaks the sandbox boundary.
🔒 Proposed fix
const isAllowed = allowedDirs.some((dir) => {
const allowedResolved = path.resolve(dir);
- return resolved.startsWith(allowedResolved);
+ const rel = path.relative(allowedResolved, resolved);
+ return rel === "" || (!rel.startsWith("..") && !path.isAbsolute(rel));
});Also applies to: 225-227
🤖 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 `@packages/safety/src/engine.ts` around lines 219 - 223, The current path
traversal check using startsWith() in the allowedDirs.some() block is vulnerable
because it allows sibling paths (for example, /sandbox allows /sandbox-escape).
Fix this by ensuring the resolved path either equals the allowedResolved
directory exactly or is a child of it by checking that the next character after
the allowedResolved prefix is a path separator. Add proper boundary validation
logic that checks both that the resolved path starts with allowedResolved AND
that it's followed by a path separator character (or is an exact match). Apply
the same fix to the similar validation logic mentioned in lines 225-227.
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
1 participant
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.
Summary by CodeRabbit
Release Notes
New Features
splashCLI instead ofalpclaw.Configuration Changes
ALPCLAW_*toSPLASH_*.~/.alpclaw/to~/.splash/.