Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Pi‑AI direct API adapter with OAuth and streaming optimization, extends model types and model-picker UI, updates agent detection/install/linking and instruction-file handling (MDC), introduces semverDiff and async embedding DB lifecycle, and broad CLI/wizard/config UX changes. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI
participant Detect as AgentDetection
participant Wizard as SetupWizard
participant Picker as ModelPicker
participant OAuth as OAuthProvider
participant PiAI as Pi‑AI Adapter
participant Runner as LocalLLMRunner
participant Config as ConfigStore
User->>CLI: run setup / sync / enhance
CLI->>Detect: detectTargetAgent() / detectProjectAgents()
Detect-->>CLI: agent or ambiguous/null
alt no agent or ambiguous
CLI->>Wizard: runWizard(opts)
Wizard->>Picker: pickModel()
Picker->>OAuth: list/connect providers
OAuth-->>Picker: provider status & tokens
Picker-->>Wizard: model selected
Wizard->>Config: update config
end
CLI->>Picker: is model "pi:*"?
alt pi:* model
CLI->>PiAI: optimizeSectionPiAi(opts)
PiAI->>OAuth: resolve API key / refresh
OAuth-->>PiAI: credentials (env or oauth)
PiAI->>PiAI: inline refs & stream request
PiAI-->>CLI: progress events + final result (usage/cost)
else non-pi model
CLI->>Runner: spawn CLI LLM process
Runner-->>CLI: output
end
CLI-->>User: show results/status
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/retriv/embedding-cache.ts (1)
66-68:⚠️ Potential issue | 🟡 MinorPotential file-locking issue when clearing cache.
If
clearEmbeddingCache()is called while a database connection fromcachedEmbeddings()is still in use, this may fail on Windows due to file locking, or leave the application in an inconsistent state. If a singleton pattern is adopted (per earlier suggestion), this function should also close the singleton instance before deleting.🛡️ Proposed fix (if singleton pattern is adopted)
+export function clearEmbeddingCache(): void { + if (dbInstance) { + dbInstance.close() + dbInstance = null + } rmSync(EMBEDDINGS_DB_PATH, { force: true }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/retriv/embedding-cache.ts` around lines 66 - 68, clearEmbeddingCache currently unconditionally deletes EMBEDDINGS_DB_PATH which can fail or corrupt state if a database instance from cachedEmbeddings() is open; update clearEmbeddingCache to first check for and close the singleton DB instance returned by cachedEmbeddings() (or the module-level singleton) by invoking its close/cleanup method, await/handle that close if asynchronous, swallow/propagate errors appropriately, and only then remove EMBEDDINGS_DB_PATH (rmSync) to avoid Windows file-locking and inconsistent state; reference the cachedEmbeddings() accessor, the singleton DB instance and EMBEDDINGS_DB_PATH when making the change.
🧹 Nitpick comments (7)
src/commands/assemble.ts (1)
176-176: Keep terminology consistent across command metadata and file docs.
meta.descriptionnow says “enhancement output files”, but the file header (Line 2) still refers to “LLM output”. Please align both strings to avoid mixed terminology.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/assemble.ts` at line 176, Update the command metadata to use consistent terminology with the file header: change the meta.description string in the assemble command (meta: { name: 'assemble', description: ... }) to match the phrase used in the file header ("LLM output") or vice versa—whichever term the repository standard uses—so both the meta.description and the file header use the same wording (either "LLM output" or "enhancement output files") for consistency.src/retriv/embedding-cache.ts (1)
13-21: Database connection is never closed and no connection reuse.Each call to
openDb()creates a newDatabaseSyncconnection that is never closed. IfcachedEmbeddings()is called multiple times, this accumulates open connections. Consider using a singleton pattern or explicitly closing connections.♻️ Proposed singleton pattern
+let dbInstance: DatabaseSync | null = null + async function openDb(): Promise<DatabaseSync> { + if (dbInstance) { + return dbInstance + } const { DatabaseSync: DB } = await import('node:sqlite') - const db = new DB(EMBEDDINGS_DB_PATH) - db.exec('PRAGMA journal_mode=WAL') - db.exec('PRAGMA busy_timeout=5000') - db.exec(`CREATE TABLE IF NOT EXISTS embeddings (text_hash TEXT PRIMARY KEY, embedding BLOB NOT NULL)`) - db.exec(`CREATE TABLE IF NOT EXISTS meta (key TEXT PRIMARY KEY, value TEXT NOT NULL)`) - return db + dbInstance = new DB(EMBEDDINGS_DB_PATH) + dbInstance.exec('PRAGMA journal_mode=WAL') + dbInstance.exec('PRAGMA busy_timeout=5000') + dbInstance.exec(`CREATE TABLE IF NOT EXISTS embeddings (text_hash TEXT PRIMARY KEY, embedding BLOB NOT NULL)`) + dbInstance.exec(`CREATE TABLE IF NOT EXISTS meta (key TEXT PRIMARY KEY, value TEXT NOT NULL)`) + return dbInstance }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/retriv/embedding-cache.ts` around lines 13 - 21, The openDb() function currently creates a new DatabaseSync for EMBEDDINGS_DB_PATH on every call and never closes it; change it to reuse a single shared DatabaseSync instance (singleton) and expose it to cachedEmbeddings() so multiple calls reuse the same connection, and ensure you provide a safe close path (e.g., a closeDb() that calls db.close() and/or process exit handler) to release the connection when the app shuts down; update openDb() to return the cached db instance when present, reference DatabaseSync/DB and EMBEDDINGS_DB_PATH to locate the code, and modify cachedEmbeddings() to call openDb() once and avoid creating new DatabaseSync objects per invocation.src/commands/cache.ts (1)
31-31: Consider aligning internal variable naming with the new "enhancement" terminology.The user-facing messages now reference "enhancement cache" (lines 68, 83), but internal variables still use "LLM" terminology (
expiredLlmon line 31). While the directory nameLLM_CACHE_DIRmay need to remain "llm-cache" for backward compatibility, consider renaming the internal counter variable for consistency:♻️ Proposed refactor for naming consistency
- let expiredLlm = 0 + let expiredEnhancements = 0 let freedBytes = 0 // Clean expired LLM cache entries @@ -40,7 +40,7 @@ const { timestamp } = JSON.parse(readFileSync(path, 'utf-8')) if (now - timestamp > LLM_CACHE_MAX_AGE) { freedBytes += safeRemove(path) - expiredLlm++ + expiredEnhancements++ } } catch { // Corrupt cache entry — remove it freedBytes += safeRemove(path) - expiredLlm++ + expiredEnhancements++ } } } const freedKB = Math.round(freedBytes / 1024) - if (expiredLlm > 0 || embeddingCleared) { + if (expiredEnhancements > 0 || embeddingCleared) { const parts: string[] = [] - if (expiredLlm > 0) + if (expiredEnhancements > 0) parts.push(`${expiredLlm} expired enhancement cache entries`)Also applies to: 68-68, 83-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/cache.ts` at line 31, Rename the internal counter expiredLlm to a name aligned with the new "enhancement" terminology (e.g., expiredEnhancements or expiredEnhancementCount) and update every usage site in this file to the new identifier; leave the LLM_CACHE_DIR constant (and any on-disk directory name) unchanged for backward compatibility, but update any user-facing messages that currently reference expiredLlm to use the new variable name so internal naming matches the "enhancement cache" messages.src/commands/uninstall.ts (1)
28-36: Guard.cursorrulescleanup to be cursor-specific.Line 35 unconditionally cleans up
.cursorrulesfor any agent with an MDC instruction file. Currently, only cursor uses the.mdcformat (.cursor/rules/skilld-activation.mdc), so the code is functionally correct. However, the cleanup logic should be explicit about being cursor-specific to prevent future issues if another agent adopts.mdcformat.Suggested fix
if (agentConfig.instructionFile.endsWith('.mdc')) { // MDC files are dedicated skilld files - just delete if (existsSync(filePath)) { rmSync(filePath) removed = true } - // Also clean up legacy .cursorrules markers - removed = removeMarkerBlock(join(projectPath, '.cursorrules')) || removed + // Also clean up legacy .cursorrules markers (cursor-specific) + if (agent === 'cursor') { + removed = removeMarkerBlock(join(projectPath, '.cursorrules')) || removed + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/uninstall.ts` around lines 28 - 36, The cleanup of .cursorrules is unconditional whenever agentConfig.instructionFile endsWith('.mdc'), which is fragile; update the conditional around removeMarkerBlock so it only runs for the cursor agent (e.g., check agentConfig.agentType === 'cursor' or test that instructionFile path includes '.cursor' as appropriate) before calling removeMarkerBlock(join(projectPath, '.cursorrules')), leaving the existing MDC file deletion (existsSync/rmSync) intact; reference agentConfig.instructionFile, agentConfig.agentType (or path check), removeMarkerBlock, projectPath and ensure removed is updated only when the cursor-specific condition passes.src/commands/remove.ts (1)
128-129: Prefer runtime-selected agent in intro context.Line 128 currently prioritizes
config.agentover the resolved runtimeagent, so--agentcan display stale context in the intro line.Proposed fix
- const intro = { state, generators, modelId: config.model, agentId: config.agent || agent || undefined } + const intro = { state, generators, modelId: config.model, agentId: agent || config.agent || undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/remove.ts` around lines 128 - 129, The intro object is using config.agent before the resolved runtime agent, causing the intro to show stale agent values; update the construction of intro (used in p.intro and introLine) to prefer the runtime-resolved agent variable first (agent), falling back to config.agent, e.g., set the agentId field to use agent || config.agent || undefined so the intro reflects the runtime selection.src/agent/targets/github-copilot.ts (1)
8-25: Consider logging or narrowing the caught exception.The empty
catch {}block silently swallows all errors fromreaddirSync. WhileexistsSyncguards against missing directories, permission errors or other I/O issues would be silently ignored, potentially masking debugging information.💡 Optional: narrow exception handling
try { if (readdirSync(dir).some(e => e.startsWith('github.copilot-'))) return true } - catch {} + catch { + // Ignore errors (permission denied, etc.) — best-effort detection + }Alternatively, if you want stricter handling:
- catch {} + catch (err: unknown) { + // Only ignore expected filesystem errors + if (err instanceof Error && 'code' in err && err.code !== 'ENOENT' && err.code !== 'EACCES') { + throw err + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/targets/github-copilot.ts` around lines 8 - 25, The empty catch in hasCopilotExtension silently swallows errors from readdirSync; change it to either log the error via your logger (including context like dir and error) or narrow the catch to only handle expected errors (e.g., ENOENT/EPERM) and rethrow or log others; update the try/catch around readdirSync in hasCopilotExtension to capture the caught error (e) and call a logger or handle non-expected errors instead of ignoring them entirely.test/unit/agent-detect.test.ts (1)
68-72: Consider asserting the exact return value for clarity.The test correctly verifies that
CODEX_HOMEis not a session signal, but using.not.toBe('codex')leaves ambiguity about the actual return value. Since no other env vars are set,detectTargetAgent()should returnnull.💡 Suggested improvement
it('does not detect codex from CODEX_HOME env (config dir, not session)', () => { process.env.CODEX_HOME = '/home/user/.codex' // CODEX_HOME is a config dir override, not a "running inside codex" signal - expect(detectTargetAgent()).not.toBe('codex') + expect(detectTargetAgent()).toBeNull() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/agent-detect.test.ts` around lines 68 - 72, Update the test "does not detect codex from CODEX_HOME env (config dir, not session)" to assert the exact return value from detectTargetAgent() instead of a negated string check; replace expect(detectTargetAgent()).not.toBe('codex') with an explicit null assertion (e.g., expect(detectTargetAgent()).toBeNull()) so the test verifies detectTargetAgent() returns null when only CODEX_HOME is set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/clis/index.ts`:
- Around line 303-311: The pi-ai execution branch in optimizeSectionViaPiAi (the
isPiAiModel() path) currently ignores the CLI subprocess timeout and can hang if
the stream stalls; update that branch to honor the same timeout behavior used
elsewhere by wiring in an AbortController/timeout (or reuse the existing timeout
logic) so the pi-ai subprocess is aborted and its promise rejected after the
configured duration, ensure any onProgress/stream handlers are cleaned up and
the subprocess killed on timeout, and surface a proper timeout error from
optimizeSectionViaPiAi so callers (e.g., skilld add/update) do not hang.
In `@src/agent/clis/pi-ai.ts`:
- Around line 295-313: The README/_INDEX additions bypass the
MAX_REFERENCE_CHARS limit because files.push and totalChars += content.length
happen before any size guard; update the logic around readmePath and indexPath
handling (the blocks that define readmePath/indexPath, readFileSync, files, and
totalChars) to check MAX_REFERENCE_CHARS before appending—either skip or
truncate the file content so adding it cannot make totalChars exceed
MAX_REFERENCE_CHARS, and after each append ensure you stop processing further
REFERENCE_SUBDIRS once totalChars >= MAX_REFERENCE_CHARS.
- Around line 287-363: collectReferenceContent currently inlines raw file
contents into files and the final blocks without sanitization; import and use
the markdown sanitization function from src/core/sanitize.ts to clean every
content blob before pushing it into files and before building blocks (i.e.,
sanitize the README.md read into content, each _INDEX.md and other .md entries
read in the loop, and the .d.ts content) so prompt-injection vectors are
stripped; update the function to call the sanitizer (from src/core/sanitize.ts)
on each content value prior to files.push and ensure the final blocks use the
sanitized f.content.
- Around line 63-75: The merged auth object returned by loadAuth() (which
overlays PI_AGENT_AUTH_PATH onto SKILLD_AUTH_PATH) must never be written back to
disk; change saveAuth() and any callers like resolveApiKey(), the login/logout
helpers to only read, modify, and persist the local SKILLD_AUTH content (not the
merged view) — e.g., always call readAuthFile(SKILLD_AUTH_PATH) to get the local
store, apply only local-provider updates/removals, then write that local object
with saveAuth(); ensure no code persists tokens originating from
PI_AGENT_AUTH_PATH or writes the merged object back to disk.
In `@src/agent/install.ts`:
- Line 116: The install logic short-circuits (via continue) before inspecting
and cleaning existing symlinks, and linkedDirs only tracks agentSkillsDir so
stale links for agents now covered by another dir remain; update the loop that
handles agentSkillsDir/target to always inspect the current target path and
remove any stale symlinks before any continue, and change linkedDirs to record
the actual symlinked directory(s) or targets for each agent (not just
agentSkillsDir) so overlap detection is order-independent; ensure cleanup code
runs for the cases referenced around the existing target/agentSkillsDir checks
so covered agents have their old links removed even when installation is
skipped.
- Around line 61-63: The global-install dedupe is asymmetric because writtenDirs
only records baseDir and the install check only looks at additionalSkillsDirs,
so agent ordering can cause duplicates; update the logic in installAgent (or the
loop that uses writtenDirs, baseDir and additionalSkillsDirs) to: when deciding
to skip installing an agent, check both its baseDir and all entries in
additionalSkillsDirs against writtenDirs, and when marking an agent as installed
add its baseDir plus each additionalSkillsDirs entry into writtenDirs; reference
writtenDirs, baseDir, additionalSkillsDirs and detectInstalledAgents to locate
the relevant code paths and apply the symmetric add/check change so overlaps are
detected regardless of agent processing order.
In `@src/agent/targets/codex.ts`:
- Around line 25-29: detectProject currently only checks for a .codex directory
and misses valid Codex projects; update the detectProject function to also check
for AGENTS.md, AGENTS.override.md, and the .agents/skills directory (i.e.,
return true if any of these exist) while leaving detectEnv unchanged; modify the
existing detectProject arrow function to perform these three existence checks
(referencing detectProject and detectEnv in src/agent/targets/codex.ts) and
return the combined boolean result.
In `@src/cli-helpers.ts`:
- Around line 68-70: Replace the class export MenuCancel with a functional
tagging approach: remove the class MenuCancel and instead export a unique symbol
constant (e.g., MENU_CANCEL_TAG) and a factory function (e.g.,
createMenuCancel()) that returns a plain Error whose name is 'MenuCancel' and
which has the MENU_CANCEL_TAG property set to true; update any throw sites that
do new MenuCancel() to throw createMenuCancel(), and replace any instanceof
MenuCancel checks (including inside menuLoop handlers) with checks for
error[MENU_CANCEL_TAG] === true (or error.name === 'MenuCancel') to detect the
cancellation.
In `@src/cli.ts`:
- Around line 510-525: The menu options are built from a stale snapshot of state
and the resolved agent; after actions that mutate configuration (specifically
removeCommand() and configCommand()) refresh the global state and re-resolve the
agent before the next menu iteration so the options/hints and install targets
reflect current config. Locate the menuLoop invocation and update the flow so
that after calling removeCommand() or configCommand() you call the existing
state-refresh function (e.g., whatever updates `state`) and the agent-resolution
function (the code that produces the resolved agent used by install/update
paths), then continue to the next loop so the options() closure sees the latest
`state` and agent. Ensure you reference the same symbols (`menuLoop`, `state`,
`removeCommand`, `configCommand`, and the agent resolution function) when making
the change.
In `@src/commands/config.ts`:
- Around line 142-166: The spinner is never stopped on the successful OAuth path
so the terminal may keep spinning; after awaiting loginOAuthProvider (the const
success = await loginOAuthProvider(...) call) ensure you call spinner.stop()
before calling p.log.success(`Connected to ${pr.name}`) (and also ensure any
early returns from the promise rejection handler still stop the spinner). Locate
the spinner variable and the success check (the success boolean and the
p.log.success call) and insert spinner.stop() in the success branch (and confirm
the .catch branch already stops the spinner).
In `@src/commands/install.ts`:
- Around line 617-619: The log at the skip branch currently calls
llmLog.error('Enhancement skipped') which is misleading for a non-failure path;
change this to a non-error level (e.g., llmLog.info or llmLog.debug) in the same
branch that checks wasOptimized so the message reflects a normal skip rather
than an error.
In `@src/commands/sync-shared.ts`:
- Around line 1160-1163: The early return in selectLlmConfig when presetModel is
set bypasses availability checks; instead, verify the preset model is currently
available before returning (e.g., call the project’s model-availability
validator or a helper like isModelAvailable/validateModel with the presetModel),
and only return { model: presetModel, sections: DEFAULT_SECTIONS } when that
validation succeeds; if validation fails, fall through to the normal selection
flow (or return null) so the stale model is not used. Ensure you reference
selectLlmConfig, presetModel, and DEFAULT_SECTIONS when making the change.
- Around line 259-262: The global install base dir is inconsistent: get-global
path uses agents[agent].globalSkillsDir but handleShippedSkills still writes to
join(CACHE_DIR, 'skills'); update handleShippedSkills to compute the
shipped-skills destination the same way as the global branch (use
agents[agent].globalSkillsDir when global is requested, falling back to
join(CACHE_DIR, 'skills') if unset) so shipped skills and global installs share
the same base; ensure you reference the same resolution logic (e.g., reuse the
code that reads agentConfig = agents[agent] and agentConfig.globalSkillsDir) so
both code paths write to the identical directory.
In `@src/commands/wizard.ts`:
- Around line 29-30: runWizard currently logs "Setup cancelled" and returns
void, which prevents callers from stopping execution; change the cancellation
behavior to throw a specific error (e.g., new SetupCancelledError) instead of
returning—replace any early returns that log cancellation in runWizard (and the
other cancellation sites referenced) with throwing that error, add a small
SetupCancelledError class or reuse an existing CancellationError, and update
callers that await runWizard to catch this error (or let it propagate) so the
enclosing flow actually aborts instead of continuing.
- Around line 181-186: The wizard currently calls updateConfig with a
conditional that omits the model field when the user selects "Auto", but
updateConfig in src/core/config.ts merges partial updates so the prior pinned
model remains; change the call in the wizard so that when modelId is falsy you
explicitly set the model field to undefined (or null as your config expects)
along with skipLlm: skippedEnhancement, i.e. always include model in the object
passed to updateConfig so the saved selection is cleared; refer to updateConfig,
modelId, skipLlm, and skippedEnhancement to locate and update this behavior.
---
Outside diff comments:
In `@src/retriv/embedding-cache.ts`:
- Around line 66-68: clearEmbeddingCache currently unconditionally deletes
EMBEDDINGS_DB_PATH which can fail or corrupt state if a database instance from
cachedEmbeddings() is open; update clearEmbeddingCache to first check for and
close the singleton DB instance returned by cachedEmbeddings() (or the
module-level singleton) by invoking its close/cleanup method, await/handle that
close if asynchronous, swallow/propagate errors appropriately, and only then
remove EMBEDDINGS_DB_PATH (rmSync) to avoid Windows file-locking and
inconsistent state; reference the cachedEmbeddings() accessor, the singleton DB
instance and EMBEDDINGS_DB_PATH when making the change.
---
Nitpick comments:
In `@src/agent/targets/github-copilot.ts`:
- Around line 8-25: The empty catch in hasCopilotExtension silently swallows
errors from readdirSync; change it to either log the error via your logger
(including context like dir and error) or narrow the catch to only handle
expected errors (e.g., ENOENT/EPERM) and rethrow or log others; update the
try/catch around readdirSync in hasCopilotExtension to capture the caught error
(e) and call a logger or handle non-expected errors instead of ignoring them
entirely.
In `@src/commands/assemble.ts`:
- Line 176: Update the command metadata to use consistent terminology with the
file header: change the meta.description string in the assemble command (meta: {
name: 'assemble', description: ... }) to match the phrase used in the file
header ("LLM output") or vice versa—whichever term the repository standard
uses—so both the meta.description and the file header use the same wording
(either "LLM output" or "enhancement output files") for consistency.
In `@src/commands/cache.ts`:
- Line 31: Rename the internal counter expiredLlm to a name aligned with the new
"enhancement" terminology (e.g., expiredEnhancements or expiredEnhancementCount)
and update every usage site in this file to the new identifier; leave the
LLM_CACHE_DIR constant (and any on-disk directory name) unchanged for backward
compatibility, but update any user-facing messages that currently reference
expiredLlm to use the new variable name so internal naming matches the
"enhancement cache" messages.
In `@src/commands/remove.ts`:
- Around line 128-129: The intro object is using config.agent before the
resolved runtime agent, causing the intro to show stale agent values; update the
construction of intro (used in p.intro and introLine) to prefer the
runtime-resolved agent variable first (agent), falling back to config.agent,
e.g., set the agentId field to use agent || config.agent || undefined so the
intro reflects the runtime selection.
In `@src/commands/uninstall.ts`:
- Around line 28-36: The cleanup of .cursorrules is unconditional whenever
agentConfig.instructionFile endsWith('.mdc'), which is fragile; update the
conditional around removeMarkerBlock so it only runs for the cursor agent (e.g.,
check agentConfig.agentType === 'cursor' or test that instructionFile path
includes '.cursor' as appropriate) before calling
removeMarkerBlock(join(projectPath, '.cursorrules')), leaving the existing MDC
file deletion (existsSync/rmSync) intact; reference agentConfig.instructionFile,
agentConfig.agentType (or path check), removeMarkerBlock, projectPath and ensure
removed is updated only when the cursor-specific condition passes.
In `@src/retriv/embedding-cache.ts`:
- Around line 13-21: The openDb() function currently creates a new DatabaseSync
for EMBEDDINGS_DB_PATH on every call and never closes it; change it to reuse a
single shared DatabaseSync instance (singleton) and expose it to
cachedEmbeddings() so multiple calls reuse the same connection, and ensure you
provide a safe close path (e.g., a closeDb() that calls db.close() and/or
process exit handler) to release the connection when the app shuts down; update
openDb() to return the cached db instance when present, reference
DatabaseSync/DB and EMBEDDINGS_DB_PATH to locate the code, and modify
cachedEmbeddings() to call openDb() once and avoid creating new DatabaseSync
objects per invocation.
In `@test/unit/agent-detect.test.ts`:
- Around line 68-72: Update the test "does not detect codex from CODEX_HOME env
(config dir, not session)" to assert the exact return value from
detectTargetAgent() instead of a negated string check; replace
expect(detectTargetAgent()).not.toBe('codex') with an explicit null assertion
(e.g., expect(detectTargetAgent()).toBeNull()) so the test verifies
detectTargetAgent() returns null when only CODEX_HOME is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa6899ae-a239-4520-8a73-02020d02905c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
package.jsonpnpm-workspace.yamlsrc/agent/clis/index.tssrc/agent/clis/pi-ai.tssrc/agent/clis/types.tssrc/agent/detect.tssrc/agent/index.tssrc/agent/install.tssrc/agent/targets/claude-code.tssrc/agent/targets/codex.tssrc/agent/targets/cursor.tssrc/agent/targets/gemini-cli.tssrc/agent/targets/github-copilot.tssrc/agent/targets/types.tssrc/cli-helpers.tssrc/cli.tssrc/commands/assemble.tssrc/commands/cache.tssrc/commands/config.tssrc/commands/install.tssrc/commands/remove.tssrc/commands/setup.tssrc/commands/sync-parallel.tssrc/commands/sync-shared.tssrc/commands/sync.tssrc/commands/uninstall.tssrc/commands/wizard.tssrc/core/shared.tssrc/retriv/embedding-cache.tstest/unit/agent-detect.test.ts
Comment on lines
+61
to
+63
| // Track directories already written to, so agents that also scan those dirs | ||
| // (via additionalSkillsDirs) don't get duplicate skills | ||
| const writtenDirs = new Set<string>() |
There was a problem hiding this comment.
Make the global-install dedupe symmetric.
writtenDirs only stores prior baseDir values, and Line 84 only checks the current agent’s additionalSkillsDirs. If agent A reads agent B’s dir and A is processed first, B still gets installed and A sees the skill twice. Because detectInstalledAgents() preserves registry order in src/agent/detect.ts:12-16, overlap resolution now depends on agent ordering.
♻️ Suggested fix
- const writtenDirs = new Set<string>()
+ const coveredDirs = new Set<string>()
for (const agentType of targetAgents) {
const agent = agents[agentType]
@@
- if (isGlobal && agent.additionalSkillsDirs.some(d => writtenDirs.has(d))) {
+ const readDirs = isGlobal ? [baseDir, ...agent.additionalSkillsDirs] : [baseDir]
+ if (isGlobal && readDirs.some(d => coveredDirs.has(d))) {
skipped.push({ agent: agentType, reason: 'already covered by another agent dir' })
continue
}
@@
- writtenDirs.add(baseDir)
+ for (const dir of readDirs)
+ coveredDirs.add(dir)
}Also applies to: 84-85, 102-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/install.ts` around lines 61 - 63, The global-install dedupe is
asymmetric because writtenDirs only records baseDir and the install check only
looks at additionalSkillsDirs, so agent ordering can cause duplicates; update
the logic in installAgent (or the loop that uses writtenDirs, baseDir and
additionalSkillsDirs) to: when deciding to skip installing an agent, check both
its baseDir and all entries in additionalSkillsDirs against writtenDirs, and
when marking an agent as installed add its baseDir plus each
additionalSkillsDirs entry into writtenDirs; reference writtenDirs, baseDir,
additionalSkillsDirs and detectInstalledAgents to locate the relevant code paths
and apply the symmetric add/check change so overlaps are detected regardless of
agent processing order.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/cli.ts (1)
223-224:⚠️ Potential issue | 🟠 MajorRe-resolve the target agent after returning from Configure.
agentis fixed before the menu loop and never refreshed here. If the user changes the target agent inconfigCommand(), the next install/update/remove in the same session still operates on the old agent until the CLI is restarted.Also applies to: 686-688
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 223 - 224, The code captures a fixed AgentType in the variable `agent` from `currentAgent` before entering the menu loop, so changes made by `configCommand()` are ignored; update the menu loop to re-resolve the target agent from `currentAgent` immediately before handling actions that operate on the agent (e.g., the install/update/remove branches) instead of using the stale `agent` variable, or replace uses of `agent` inside the loop with a fresh `const agent = currentAgent` (or a call to the resolver used elsewhere) so the CLI respects agent changes made during `configCommand()`.src/agent/clis/pi-ai.ts (2)
408-426:⚠️ Potential issue | 🟠 Major
signalonly aborts between streamed events.If
streamSimple()stalls and stops yielding, thefor awaitloop never reachesopts.signal?.aborted, so the caller’s timeout can still hang indefinitely. This needs either a library-supported abort wired intostreamSimple()or an independent timeout race around the iterator.Does `@mariozechner/pi-ai` `streamSimple()` support `AbortSignal` or another cancellation API for aborting a stalled stream? If yes, how should it be passed into the stream call?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/clis/pi-ai.ts` around lines 408 - 426, The for-await loop over streamSimple can hang if the iterator stalls because opts.signal is only checked between events; either pass an AbortSignal into streamSimple (if `@mariozechner/pi-ai` supports it) by adding the signal to the options object passed to streamSimple, or add an external cancellation race: create an abort/timeout promise that, when triggered, calls stream.return() on the async iterator and throws an error so the loop exits; update the caller to wire opts.signal (or a derived timeout AbortSignal) into the race and ensure any cleanup (calling stream.return()) happens when aborting.
64-70:⚠️ Potential issue | 🟠 MajorKeep the merged auth view read-only.
loadAuth()overlaysPI_AGENT_AUTH_PATHon top of the local store, but these paths then persist refreshed/login/logout state back into~/.skilld/pi-ai-auth.json. That copies external OAuth tokens into a second file and makes “Disconnect” ineffective whenever the provider still exists in the pi-agent store. Only mutatereadAuthFile(SKILLD_AUTH_PATH)when saving local auth.Also applies to: 113-124, 162-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/clis/pi-ai.ts` around lines 64 - 70, loadAuth currently merges PI_AGENT_AUTH_PATH creds over SKILLD_AUTH_PATH and the merged object is later persisted back to SKILLD_AUTH_PATH, which copies external tokens into the local store; fix by treating the merged view as read-only: in loadAuth() return a frozen/shallow-copied object (e.g., Object.freeze({...skilldAuth, ...piAuth})) and ensure any persistence function (e.g., save/write functions that call readAuthFile or write to SKILLD_AUTH_PATH) only mutates the original SKILLD auth object (the one returned from readAuthFile(SKILLD_AUTH_PATH)) rather than the merged result; apply the same change for the other merge sites so merged views from PI_AGENT_AUTH_PATH are never written back to SKILLD_AUTH_PATH.src/commands/sync-shared.ts (1)
1065-1068:⚠️ Potential issue | 🟠 MajorClear
skipLlmwhen saving a picked model.
updateConfig()merges partial updates insrc/core/config.ts, so persisting onlymodelleaves an oldskipLlm: truein place. The current run works, but later runs can still auto-skip even though the user just re-enabled enhancement.🩹 Minimal fix
- updateConfig({ model: choice as OptimizeModel }) + updateConfig({ model: choice as OptimizeModel, skipLlm: false })- updateConfig({ model: picked as OptimizeModel }) + updateConfig({ model: picked as OptimizeModel, skipLlm: false })Also applies to: 1258-1264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/sync-shared.ts` around lines 1065 - 1068, When persisting a selected model via updateConfig({ model: choice as OptimizeModel }) you must also clear any previous skipLlm flag so future runs won't auto-skip enhancements; change the updateConfig call (in the block using choice as OptimizeModel and the similar block at the other location) to include skipLlm: false (e.g., updateConfig({ model: choice as OptimizeModel, skipLlm: false })) so the old skipLlm: true is overwritten.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/clis/pi-ai.ts`:
- Around line 369-370: The reference file contents are interpolated raw into
XML-like <file> blocks and must be sanitized and XML-escaped to prevent prompt
injection; update the construction around files.map (the blocks variable and its
template return) to first run each f.content through the project's markdown
sanitizer (from src/core/sanitize.ts, e.g., sanitizeMarkdown or sanitize) and
then XML-escape special chars (at minimum &, <, > and the sequence "</") before
embedding so sequences like </file> or </reference-files> cannot break out of
the wrapper.
In `@src/commands/sync-shared.ts`:
- Around line 234-236: The code sets baseDir to agentConfig.globalSkillsDir
unconditionally when global is true, causing undefined paths for agents that
don't expose that field; update the logic around baseDir (and the similar block
at the later 258-261 region) to first check that agentConfig.globalSkillsDir
exists before using it—if global is true but agentConfig.globalSkillsDir is
falsy, fall back to the non-global path (shared || join(cwd,
agentConfig.skillsDir)) or throw a clear error; specifically modify the
assignment that references baseDir, the global flag and
agentConfig.globalSkillsDir so you only return agentConfig.globalSkillsDir when
it is defined, and ensure any subsequent code that assumes baseDir is a string
handles the fallback case.
In `@src/commands/wizard.ts`:
- Around line 52-53: The example path in the template string in wizard.ts
incorrectly points to "vue-skilld" but real generated skill directories use the
package name; update the example (the template that uses skillsDir) to show
.claude/skills/<pkg>/SKILL.md (e.g., `${skillsDir}/vue/SKILL.md`) and, if
helpful, mention the accompanying gitignored symlink folder `.skilld` (i.e.,
`.claude/skills/<pkg>/.skilld/`) so users look at the actual generated
project-level skill file rather than a non-existent "vue-skilld" path.
---
Duplicate comments:
In `@src/agent/clis/pi-ai.ts`:
- Around line 408-426: The for-await loop over streamSimple can hang if the
iterator stalls because opts.signal is only checked between events; either pass
an AbortSignal into streamSimple (if `@mariozechner/pi-ai` supports it) by adding
the signal to the options object passed to streamSimple, or add an external
cancellation race: create an abort/timeout promise that, when triggered, calls
stream.return() on the async iterator and throws an error so the loop exits;
update the caller to wire opts.signal (or a derived timeout AbortSignal) into
the race and ensure any cleanup (calling stream.return()) happens when aborting.
- Around line 64-70: loadAuth currently merges PI_AGENT_AUTH_PATH creds over
SKILLD_AUTH_PATH and the merged object is later persisted back to
SKILLD_AUTH_PATH, which copies external tokens into the local store; fix by
treating the merged view as read-only: in loadAuth() return a
frozen/shallow-copied object (e.g., Object.freeze({...skilldAuth, ...piAuth}))
and ensure any persistence function (e.g., save/write functions that call
readAuthFile or write to SKILLD_AUTH_PATH) only mutates the original SKILLD auth
object (the one returned from readAuthFile(SKILLD_AUTH_PATH)) rather than the
merged result; apply the same change for the other merge sites so merged views
from PI_AGENT_AUTH_PATH are never written back to SKILLD_AUTH_PATH.
In `@src/cli.ts`:
- Around line 223-224: The code captures a fixed AgentType in the variable
`agent` from `currentAgent` before entering the menu loop, so changes made by
`configCommand()` are ignored; update the menu loop to re-resolve the target
agent from `currentAgent` immediately before handling actions that operate on
the agent (e.g., the install/update/remove branches) instead of using the stale
`agent` variable, or replace uses of `agent` inside the loop with a fresh `const
agent = currentAgent` (or a call to the resolver used elsewhere) so the CLI
respects agent changes made during `configCommand()`.
In `@src/commands/sync-shared.ts`:
- Around line 1065-1068: When persisting a selected model via updateConfig({
model: choice as OptimizeModel }) you must also clear any previous skipLlm flag
so future runs won't auto-skip enhancements; change the updateConfig call (in
the block using choice as OptimizeModel and the similar block at the other
location) to include skipLlm: false (e.g., updateConfig({ model: choice as
OptimizeModel, skipLlm: false })) so the old skipLlm: true is overwritten.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e97a796e-be41-49ab-b539-5c13aaf680bc
📒 Files selected for processing (13)
src/agent/clis/index.tssrc/agent/clis/pi-ai.tssrc/agent/install.tssrc/agent/targets/codex.tssrc/cli.tssrc/commands/config.tssrc/commands/install.tssrc/commands/remove.tssrc/commands/sync-shared.tssrc/commands/uninstall.tssrc/commands/wizard.tssrc/retriv/embedding-cache.tstest/unit/sync-shared.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/commands/uninstall.ts
- src/commands/install.ts
- src/agent/targets/codex.ts
- src/agent/install.ts
- src/commands/remove.ts
- src/retriv/embedding-cache.ts
src/commands/sync-shared.ts
Outdated
Comment on lines
234
to
236
| const baseDir = global | ||
| ? join(CACHE_DIR, 'skills') | ||
| ? agentConfig.globalSkillsDir | ||
| : shared || join(cwd, agentConfig.skillsDir) |
There was a problem hiding this comment.
Guard agents that do not expose a global skills dir.
src/core/skills.ts only treats global installs as valid when agent.globalSkillsDir exists, but both of these branches return it unconditionally. For any agent without that field, baseDir becomes undefined and the first global install path that touches it will throw.
Also applies to: 258-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/sync-shared.ts` around lines 234 - 236, The code sets baseDir to
agentConfig.globalSkillsDir unconditionally when global is true, causing
undefined paths for agents that don't expose that field; update the logic around
baseDir (and the similar block at the later 258-261 region) to first check that
agentConfig.globalSkillsDir exists before using it—if global is true but
agentConfig.globalSkillsDir is falsy, fall back to the non-global path (shared
|| join(cwd, agentConfig.skillsDir)) or throw a clear error; specifically modify
the assignment that references baseDir, the global flag and
agentConfig.globalSkillsDir so you only return agentConfig.globalSkillsDir when
it is defined, and ensure any subsequent code that assumes baseDir is a string
handles the fallback case.
Comment on lines
+52
to
+53
| + `\x1B[90mExample: \`skilld add vue\` creates ${skillsDir}/vue-skilld/SKILL.md\n` | ||
| + `Your agent then knows the right APIs, gotchas, and patterns\n` |
There was a problem hiding this comment.
Fix the example skill path.
This copy points users at .../vue-skilld/SKILL.md, but the generated skill directory is the package name itself. New users will end up checking a path that does not exist.
🩹 Minimal fix
- + `\x1B[90mExample: \`skilld add vue\` creates ${skillsDir}/vue-skilld/SKILL.md\n`
+ + `\x1B[90mExample: \`skilld add vue\` creates ${skillsDir}/vue/SKILL.md\n`📝 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
| + `\x1B[90mExample: \`skilld add vue\` creates ${skillsDir}/vue-skilld/SKILL.md\n` | |
| + `Your agent then knows the right APIs, gotchas, and patterns\n` | |
| `\x1B[90mExample: \`skilld add vue\` creates ${skillsDir}/vue/SKILL.md\n` | |
| `Your agent then knows the right APIs, gotchas, and patterns\n` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/wizard.ts` around lines 52 - 53, The example path in the
template string in wizard.ts incorrectly points to "vue-skilld" but real
generated skill directories use the package name; update the example (the
template that uses skillsDir) to show .claude/skills/<pkg>/SKILL.md (e.g.,
`${skillsDir}/vue/SKILL.md`) and, if helpful, mention the accompanying
gitignored symlink folder `.skilld` (i.e., `.claude/skills/<pkg>/.skilld/`) so
users look at the actual generated project-level skill file rather than a
non-existent "vue-skilld" path.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/cli.ts (1)
224-224:⚠️ Potential issue | 🟠 MajorRefresh resolved
agentafter config changes before next menu actions.Line 224 captures
agentonce, but Line 687-689 only refreshesstate. AfterconfigCommand(), follow-up install/update/remove actions can still write to the previously selected agent path.💡 Suggested fix
- const agent: AgentType = currentAgent + let agent: AgentType = currentAgent ... - const refreshState = async () => { + const refreshContext = async () => { state = await getProjectState(cwd) + const resolved = resolveAgent(args.agent) + if (resolved && resolved !== 'none') + agent = resolved } ... - await refreshState() + await refreshContext() return true ... - await refreshState() + await refreshContext() return true ... - await refreshState() + await refreshContext() break ... case 'config': await configCommand() - await refreshState() + await refreshContext() breakAlso applies to: 686-689
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` at line 224, The variable agent is captured from currentAgent once (const agent: AgentType = currentAgent) but not refreshed after running configCommand(), so subsequent install/update/remove actions use a stale path; after calling configCommand() (the block that currently refreshes state around the existing state refresh logic), re-read currentAgent and reassign agent (or avoid the stale local capture) so that agent reflects the updated config before any follow-up menu actions; update the same spots where state is refreshed (the code invoking configCommand()) to recompute currentAgent/agent.src/agent/clis/pi-ai.ts (1)
162-164:⚠️ Potential issue | 🟠 MajorMerged auth view still persisted in login/logout helpers.
While
resolveApiKey()was correctly fixed to read only fromSKILLD_AUTH_PATH(line 122),loginOAuthProviderstill callsloadAuth()which returns the merged view (pi-agent + skilld). Saving this back copies any credentials from~/.pi/agent/auth.jsoninto~/.skilld/pi-ai-auth.json.The same issue exists in
logoutOAuthProviderat line 170.🔧 Proposed fix: Read skilld auth only
export async function loginOAuthProvider(providerId: string, callbacks: LoginCallbacks): Promise<boolean> { const provider = getOAuthProvider(providerId) if (!provider) return false const credentials = await provider.login({ onAuth: (info: any) => callbacks.onAuth(info.url, info.instructions), onPrompt: async (prompt: any) => callbacks.onPrompt(prompt.message, prompt.placeholder), onProgress: (msg: string) => callbacks.onProgress?.(msg), }) - const auth = loadAuth() + const auth = readAuthFile(SKILLD_AUTH_PATH) auth[providerId] = { type: 'oauth', ...credentials } saveAuth(auth) return true } /** Remove OAuth credentials for a provider */ export function logoutOAuthProvider(providerId: string): void { - const auth = loadAuth() + const auth = readAuthFile(SKILLD_AUTH_PATH) delete auth[providerId] saveAuth(auth) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/clis/pi-ai.ts` around lines 162 - 164, loginOAuthProvider and logoutOAuthProvider are calling loadAuth() which returns the merged pi-agent+skilld view and thus writes pi-agent credentials into SKILLD_AUTH_PATH; change those calls to read only the skilld auth store (i.e., replace loadAuth() with a loader that reads SKILLD_AUTH_PATH only—e.g., loadSkilldAuth or a loadAuth({skilldOnly:true}) variant) so that auth[providerId] = { type: 'oauth', ...credentials } and the subsequent saveAuth(auth) only persist the skilld-only map; keep resolveApiKey behavior unchanged and ensure the same adjustment is applied in both loginOAuthProvider and logoutOAuthProvider.src/commands/sync-shared.ts (1)
254-257:⚠️ Potential issue | 🟠 MajorGuard
globalSkillsDirbefore returning it.Line 256 can return
undefinedfor agents without a global dir, and Line 233 then passes that intomkdirSync, causing a runtime failure.Proposed fix
export function resolveBaseDir(cwd: string, agent: AgentType, global: boolean): string { - if (global) { - const agentConfig = agents[agent] - return agentConfig.globalSkillsDir - } + const agentConfig = agents[agent] + if (global) { + if (agentConfig.globalSkillsDir) + return agentConfig.globalSkillsDir + throw new Error(`Global installs are not supported for agent "${agent}"`) + } const shared = getSharedSkillsDir(cwd) if (shared) return shared - const agentConfig = agents[agent] return join(cwd, agentConfig.skillsDir) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/sync-shared.ts` around lines 254 - 257, The current branch returns agentConfig.globalSkillsDir directly which can be undefined and later passed into mkdirSync; change the block in the global branch to guard the value (e.g. const agentConfig = agents[agent]; if (agentConfig?.globalSkillsDir) return agentConfig.globalSkillsDir; else return undefined) so callers won't receive undefined; also ensure the caller that invokes mkdirSync only does so when the returned path is truthy.
🧹 Nitpick comments (1)
src/agent/clis/pi-ai.ts (1)
396-398: Consider validating that auth is available before streaming.If
resolveApiKeyreturnsnull, the code proceeds without an API key (line 417). WhilegetAvailablePiAiModels()filters out unauthenticated providers, callers could pass a model ID directly that lacks credentials, leading to a less descriptive runtime error from the pi-ai library.💡 Optional: Add early validation
const model = getModel(parsed.provider as any, parsed.modelId as any) const apiKey = await resolveApiKey(parsed.provider) + + if (!apiKey) { + throw new Error(`No API key or OAuth credentials found for provider: ${parsed.provider}. Run 'skilld config' to set up authentication.`) + } // Build non-agentic prompt with inlined references🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/clis/pi-ai.ts` around lines 396 - 398, The code calls getModel(...) and then await resolveApiKey(...); add an early check after resolving apiKey to ensure credentials exist before initiating any streaming or pi-ai calls: if apiKey is null/undefined, throw or log a clear error mentioning the provider and modelId (from the model variable or parsed inputs) and abort the operation (process.exit or throw) so callers get a descriptive authentication error instead of a downstream pi-ai runtime error; update any places that call getAvailablePiAiModels() to keep filtering but still enforce this runtime validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli.ts`:
- Around line 446-451: The preview is printed raw (previewContent/previewLines)
and may contain terminal control sequences; call the markdown sanitization
utility from src/core/sanitize.ts (the exported sanitize function) on the
content before computing previewLines and fileSize and before passing to p.note
so the output is stripped of control chars, zero-width characters, HTML
comments, agent directive tags, external images/links/base64 blobs and directive
patterns; update the import to bring in sanitize from src/core/sanitize.ts and
replace uses of previewContent/previewLines with the sanitized result when
rendering the SKILL.md preview.
In `@src/commands/sync-shared.ts`:
- Around line 1208-1211: The code computes days using updateCtx.syncedAt without
validating it, which can produce "NaNd ago"; before using updateCtx.syncedAt in
sync-shared.ts (the block that pushes into ageParts), validate that
updateCtx.syncedAt parses to a valid Date (e.g., Date.parse or new Date(...) and
isNaN check), only compute and push the relative age when the date is valid, and
otherwise push a safe fallback (e.g., omit the entry or use 'unknown') so
ageParts never receives an "NaNd ago" string.
---
Duplicate comments:
In `@src/agent/clis/pi-ai.ts`:
- Around line 162-164: loginOAuthProvider and logoutOAuthProvider are calling
loadAuth() which returns the merged pi-agent+skilld view and thus writes
pi-agent credentials into SKILLD_AUTH_PATH; change those calls to read only the
skilld auth store (i.e., replace loadAuth() with a loader that reads
SKILLD_AUTH_PATH only—e.g., loadSkilldAuth or a loadAuth({skilldOnly:true})
variant) so that auth[providerId] = { type: 'oauth', ...credentials } and the
subsequent saveAuth(auth) only persist the skilld-only map; keep resolveApiKey
behavior unchanged and ensure the same adjustment is applied in both
loginOAuthProvider and logoutOAuthProvider.
In `@src/cli.ts`:
- Line 224: The variable agent is captured from currentAgent once (const agent:
AgentType = currentAgent) but not refreshed after running configCommand(), so
subsequent install/update/remove actions use a stale path; after calling
configCommand() (the block that currently refreshes state around the existing
state refresh logic), re-read currentAgent and reassign agent (or avoid the
stale local capture) so that agent reflects the updated config before any
follow-up menu actions; update the same spots where state is refreshed (the code
invoking configCommand()) to recompute currentAgent/agent.
In `@src/commands/sync-shared.ts`:
- Around line 254-257: The current branch returns agentConfig.globalSkillsDir
directly which can be undefined and later passed into mkdirSync; change the
block in the global branch to guard the value (e.g. const agentConfig =
agents[agent]; if (agentConfig?.globalSkillsDir) return
agentConfig.globalSkillsDir; else return undefined) so callers won't receive
undefined; also ensure the caller that invokes mkdirSync only does so when the
returned path is truthy.
---
Nitpick comments:
In `@src/agent/clis/pi-ai.ts`:
- Around line 396-398: The code calls getModel(...) and then await
resolveApiKey(...); add an early check after resolving apiKey to ensure
credentials exist before initiating any streaming or pi-ai calls: if apiKey is
null/undefined, throw or log a clear error mentioning the provider and modelId
(from the model variable or parsed inputs) and abort the operation (process.exit
or throw) so callers get a descriptive authentication error instead of a
downstream pi-ai runtime error; update any places that call
getAvailablePiAiModels() to keep filtering but still enforce this runtime
validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25d21609-b10a-4864-84cc-c66e5b31ab89
📒 Files selected for processing (4)
README.mdsrc/agent/clis/pi-ai.tssrc/cli.tssrc/commands/sync-shared.ts
Comment on lines
+446
to
+451
| const previewContent = readFileSync(previewPath, 'utf-8') | ||
| const previewLines = previewContent.split('\n').slice(0, 20).join('\n') | ||
| const fileSize = (Buffer.byteLength(previewContent) / 1024).toFixed(1) | ||
| p.note( | ||
| `\x1B[90m${previewLines}\n...\x1B[0m`, | ||
| `${agents[agent].skillsDir}/${previewSkill.name}/SKILL.md (${fileSize} KB)`, |
There was a problem hiding this comment.
Sanitize SKILL.md preview before terminal rendering.
The preview is printed raw from generated markdown. If upstream content contains control sequences, this can inject terminal escapes into the user session.
🛡️ Suggested fix
if (existsSync(previewPath)) {
const previewContent = readFileSync(previewPath, 'utf-8')
- const previewLines = previewContent.split('\n').slice(0, 20).join('\n')
+ const previewLines = previewContent.split('\n').slice(0, 20).join('\n')
+ const safePreview = previewLines
+ .replace(/\x1B\[[0-?]*[ -/]*[`@-`~]/g, '')
+ .replace(/\x1B\].*?(?:\x07|\x1B\\)/g, '')
const fileSize = (Buffer.byteLength(previewContent) / 1024).toFixed(1)
p.note(
- `\x1B[90m${previewLines}\n...\x1B[0m`,
+ `\x1B[90m${safePreview}\n...\x1B[0m`,
`${agents[agent].skillsDir}/${previewSkill.name}/SKILL.md (${fileSize} KB)`,
)
}As per coding guidelines, “Use markdown sanitization from src/core/sanitize.ts to strip prompt injection vectors including zero-width chars, HTML comments, agent directive tags, external images/links, base64 blobs, and directive patterns with code-fence-aware state machine”.
📝 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
| const previewContent = readFileSync(previewPath, 'utf-8') | |
| const previewLines = previewContent.split('\n').slice(0, 20).join('\n') | |
| const fileSize = (Buffer.byteLength(previewContent) / 1024).toFixed(1) | |
| p.note( | |
| `\x1B[90m${previewLines}\n...\x1B[0m`, | |
| `${agents[agent].skillsDir}/${previewSkill.name}/SKILL.md (${fileSize} KB)`, | |
| import { sanitizeMarkdown } from '../core/sanitize.ts' | |
| // ... later in the code ... | |
| const previewContent = readFileSync(previewPath, 'utf-8') | |
| const previewLines = previewContent.split('\n').slice(0, 20).join('\n') | |
| const safePreview = sanitizeMarkdown(previewLines) | |
| const fileSize = (Buffer.byteLength(previewContent) / 1024).toFixed(1) | |
| p.note( | |
| `\x1B[90m${safePreview}\n...\x1B[0m`, | |
| `${agents[agent].skillsDir}/${previewSkill.name}/SKILL.md (${fileSize} KB)`, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli.ts` around lines 446 - 451, The preview is printed raw
(previewContent/previewLines) and may contain terminal control sequences; call
the markdown sanitization utility from src/core/sanitize.ts (the exported
sanitize function) on the content before computing previewLines and fileSize and
before passing to p.note so the output is stripped of control chars, zero-width
characters, HTML comments, agent directive tags, external images/links/base64
blobs and directive patterns; update the import to bring in sanitize from
src/core/sanitize.ts and replace uses of previewContent/previewLines with the
sanitized result when rendering the SKILL.md preview.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/cli.ts (2)
224-224:⚠️ Potential issue | 🟠 MajorRefresh resolved agent after config changes to avoid stale install/update/remove targets.
refreshState()updates onlystate; afterconfigCommand(), subsequent actions still use the oldagentcaptured earlier.🔧 Proposed fix
- const agent: AgentType = currentAgent + let agent: AgentType = currentAgent ... const refreshState = async () => { state = await getProjectState(cwd) + const nextAgent = resolveAgent(args.agent) ?? resolveAgent(readConfig().agent) + if (nextAgent && nextAgent !== 'none') + agent = nextAgent }Also applies to: 512-514, 686-689
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` at line 224, The code captures a stale agent into a const (e.g., "const agent: AgentType = currentAgent") before calling configCommand() / refreshState(), so subsequent install/update/remove operations use the old agent; change the binding to a mutable variable (let) or re-read currentAgent after refreshState() / configCommand() and reassign agent (i.e., call refreshState() then set agent = currentAgent) in each spot where you currently create a const agent prior to configCommand() (including the other similar occurrences), ensuring actions use the refreshed agent value.
446-448:⚠️ Potential issue | 🔴 CriticalReplace inline ANSI regex sanitization with
sanitize()to fix lint blocker and harden preview output.The current regex at Line 447 introduces control characters that fail lint/CI, and it bypasses the project’s required markdown sanitization path.
🔧 Proposed fix
+import { sanitize } from './core/sanitize.ts' ... if (existsSync(previewPath)) { - const previewContent = readFileSync(previewPath, 'utf-8') - const previewLines = previewContent.split('\n').slice(0, 20).join('\n').replace(/\x1B\[[0-?]*[ -/]*[`@-`~]/g, '').replace(/\x1B\].*?(?:\x07|\x1B\\)/g, '') - const fileSize = (Buffer.byteLength(previewContent) / 1024).toFixed(1) + const previewContent = sanitize(readFileSync(previewPath, 'utf-8')) + const previewLines = previewContent.split('\n').slice(0, 20).join('\n') + const fileSize = (Buffer.byteLength(previewContent) / 1024).toFixed(1) p.note( `\x1B[90m${previewLines}\n...\x1B[0m`, `${agents[agent].skillsDir}/${previewSkill.name}/SKILL.md (${fileSize} KB)`, ) }As per coding guidelines, “Use markdown sanitization from src/core/sanitize.ts to strip prompt injection vectors including zero-width chars, HTML comments, agent directive tags, external images/links, base64 blobs, and directive patterns with code-fence-aware state machine”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 446 - 448, Replace the inline ANSI-regex sanitization with the project's sanitize function: import sanitize from src/core/sanitize.ts and call it on previewContent (e.g., const sanitizedPreview = sanitize(previewContent)); then compute previewLines from sanitizedPreview (sanitizedPreview.split('\n').slice(0,20).join('\n')) and use sanitizedPreview when computing fileSize (Buffer.byteLength(sanitizedPreview)). Update references to previewLines and fileSize accordingly and remove the regex call.
🧹 Nitpick comments (2)
src/commands/sync-shared.ts (2)
957-958: Consider usingdirnamefor clarity.Using
join(filePath, '..')to get the parent directory works, butdirname(filePath)frompatheis more idiomatic and explicit.Proposed fix
Add
dirnameto the import:-import { join, relative, resolve } from 'pathe' +import { dirname, join, relative, resolve } from 'pathe'Then replace:
- mkdirSync(join(filePath, '..'), { recursive: true }) + mkdirSync(dirname(filePath), { recursive: true })Apply the same change at line 979.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/sync-shared.ts` around lines 957 - 958, Replace the parent-directory computation that uses join(filePath, '..') with the clearer dirname(filePath) from the pathe module: add dirname to the existing pathe import and update the two calls that currently call mkdirSync(join(filePath, '..'), { recursive: true }) so they instead call mkdirSync(dirname(filePath), { recursive: true }) while leaving writeFileSync(filePath, content) unchanged; reference symbols: mkdirSync, writeFileSync, join(filePath, '..'), dirname(filePath), and filePath.
1156-1173: Consider reusingavailablearray when falling through from preset validation.When
presetModelis provided but unavailable in interactive mode,getAvailableModels()is called twice (lines 1158 and 1173). This could be optimized by hoisting the call or storing the result.Proposed fix
export async function selectLlmConfig(presetModel?: OptimizeModel, message?: string, updateCtx?: UpdateContext): Promise<LlmConfig | null> { + // Get available models once, reuse throughout + const available = await getAvailableModels() + if (presetModel) { - // Validate preset model is still available (env/OAuth may have changed) - const available = await getAvailableModels() if (available.some(m => m.id === presetModel)) return { model: presetModel, sections: DEFAULT_SECTIONS } - // Fall through to interactive selection if preset unavailable if (!isInteractive()) return null } - // Non-interactive (CI, agent, no TTY): skip generation unless model explicitly provided if (!isInteractive()) { return null } - // Resolve default model (configured or recommended) without prompting const config = readConfig() - const available = await getAvailableModels() if (available.length === 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/sync-shared.ts` around lines 1156 - 1173, The code calls getAvailableModels() twice when presetModel is provided but not available; hoist that single call and reuse its result to avoid duplicate work: move the initial await getAvailableModels() result into one available variable used both for the presetModel availability check and later when resolving the default model, ensuring you don't shadow the variable (rename if needed) and keep the same control flow around presetModel, isInteractive(), readConfig(), and DEFAULT_SECTIONS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/cli.ts`:
- Line 224: The code captures a stale agent into a const (e.g., "const agent:
AgentType = currentAgent") before calling configCommand() / refreshState(), so
subsequent install/update/remove operations use the old agent; change the
binding to a mutable variable (let) or re-read currentAgent after refreshState()
/ configCommand() and reassign agent (i.e., call refreshState() then set agent =
currentAgent) in each spot where you currently create a const agent prior to
configCommand() (including the other similar occurrences), ensuring actions use
the refreshed agent value.
- Around line 446-448: Replace the inline ANSI-regex sanitization with the
project's sanitize function: import sanitize from src/core/sanitize.ts and call
it on previewContent (e.g., const sanitizedPreview = sanitize(previewContent));
then compute previewLines from sanitizedPreview
(sanitizedPreview.split('\n').slice(0,20).join('\n')) and use sanitizedPreview
when computing fileSize (Buffer.byteLength(sanitizedPreview)). Update references
to previewLines and fileSize accordingly and remove the regex call.
---
Nitpick comments:
In `@src/commands/sync-shared.ts`:
- Around line 957-958: Replace the parent-directory computation that uses
join(filePath, '..') with the clearer dirname(filePath) from the pathe module:
add dirname to the existing pathe import and update the two calls that currently
call mkdirSync(join(filePath, '..'), { recursive: true }) so they instead call
mkdirSync(dirname(filePath), { recursive: true }) while leaving
writeFileSync(filePath, content) unchanged; reference symbols: mkdirSync,
writeFileSync, join(filePath, '..'), dirname(filePath), and filePath.
- Around line 1156-1173: The code calls getAvailableModels() twice when
presetModel is provided but not available; hoist that single call and reuse its
result to avoid duplicate work: move the initial await getAvailableModels()
result into one available variable used both for the presetModel availability
check and later when resolving the default model, ensuring you don't shadow the
variable (rename if needed) and keep the same control flow around presetModel,
isInteractive(), readConfig(), and DEFAULT_SECTIONS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00ec8609-029a-42ab-9a61-174dd8e15692
📒 Files selected for processing (2)
src/cli.tssrc/commands/sync-shared.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/commands/sync-shared.ts (1)
254-257:⚠️ Potential issue | 🟠 MajorAdd defensive check for missing
globalSkillsDirto match patterns used elsewhere.The function unconditionally returns
agentConfig.globalSkillsDirwithout validation. While all registered agents currently define this field (verified by tests), similar code insrc/core/skills.ts,src/commands/uninstall.ts, andsrc/agent/install.tsincludes explicit guards against missingglobalSkillsDir. This inconsistency creates a maintenance risk and violates defensive programming practices.Proposed fix
export function resolveBaseDir(cwd: string, agent: AgentType, global: boolean): string { if (global) { const agentConfig = agents[agent] + if (!agentConfig.globalSkillsDir) { + throw new Error(`Agent "${agent}" does not support global skill installation`) + } return agentConfig.globalSkillsDir }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/sync-shared.ts` around lines 254 - 257, The snippet returns agentConfig.globalSkillsDir without validation; update the code in the block that checks `global` to ensure `agentConfig` exists and that `agentConfig.globalSkillsDir` is present (e.g., `const agentConfig = agents[agent]; if (!agentConfig?.globalSkillsDir) return undefined; return agentConfig.globalSkillsDir;`) so it matches the defensive checks used in `src/core/skills.ts`, `src/commands/uninstall.ts`, and `src/agent/install.ts`.
🧹 Nitpick comments (2)
src/agent/clis/index.ts (1)
331-331: Hoist regex to module scope to resolve lint warning and avoid recompilation.Line 331 compiles
/-/gon each invocation. Move to a module constant/helper (also reusable at Line 505).♻️ Suggested refactor
+const DASH_GLOBAL_RE = /-/g + ... - const logName = section.toUpperCase().replace(/-/g, '_') + const logName = section.toUpperCase().replace(DASH_GLOBAL_RE, '_') ... - const logName = section.toUpperCase().replace(/-/g, '_') + const logName = section.toUpperCase().replace(DASH_GLOBAL_RE, '_')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/clis/index.ts` at line 331, The code compiles /-/g each time when computing logName; extract that regex into a module-level constant (e.g., DASH_REGEX = /-/g) and reuse it in the logName computation and the other occurrence (around line with similar replace at or near Line 505) so both places call section.toUpperCase().replace(DASH_REGEX, '_') instead of compiling the literal regex repeatedly; update references in this module to use the new constant.src/commands/sync.ts (1)
601-613: Minor inefficiency: model resolution before cache check.
getAvailableModels()(line 607) may be called even whenallSectionsCachedis true, making the call unnecessary. Consider moving the auto-resolution inside the!allSectionsCachedcheck to avoid the redundant API/detection call.Proposed optimization
const globalConfig = readConfig() let resolvedModel = config.model || (config.yes && !globalConfig.skipLlm ? globalConfig.model as OptimizeModel | undefined : undefined) - // Auto-resolve when -y, not skipping LLM, but no explicit model (e.g. user picked "Auto" in wizard) - if (!resolvedModel && config.yes && !globalConfig.skipLlm) { - const available = await getAvailableModels() - const auto = available.find(m => m.recommended)?.id ?? available[0]?.id - if (auto) - resolvedModel = auto as OptimizeModel - } - if (!allSectionsCached && !globalConfig.skipLlm && !(config.yes && !resolvedModel)) { + if (!allSectionsCached && !globalConfig.skipLlm) { + // Auto-resolve when -y, not skipping LLM, but no explicit model + if (!resolvedModel && config.yes) { + const available = await getAvailableModels() + const auto = available.find(m => m.recommended)?.id ?? available[0]?.id + if (auto) + resolvedModel = auto as OptimizeModel + } + if (!(config.yes && !resolvedModel)) { const llmConfig = await selectLlmConfig(resolvedModel, undefined, updateCtx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/sync.ts` around lines 601 - 613, The call to getAvailableModels and the auto-resolution of resolvedModel happens before checking allSectionsCached, causing unnecessary work when allSectionsCached is true; move the auto-resolution logic (the block that calls getAvailableModels and sets resolvedModel using available.find(...)) inside the existing if that guards llm usage (the if using !allSectionsCached && !globalConfig.skipLlm && !(config.yes && !resolvedModel)) so that getAvailableModels is only invoked when you actually need to call selectLlmConfig; keep the initial resolvedModel assignment from config.model/readConfig as-is and ensure the later if still sets resolvedModel before calling selectLlmConfig (referencing resolvedModel, getAvailableModels, allSectionsCached, selectLlmConfig, readConfig, config.yes, globalConfig.skipLlm).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/clis/index.ts`:
- Around line 581-583: The current check lets any pi: model bypass validation
and causes repeated errors downstream; update the logic around
isPiAiModel(model) to first call getAvailablePiAiModels(), verify the requested
pi: model is present (and thus available/authenticated), and if not return the
single upfront error { optimized: '', wasOptimized: false, error: `No available
Pi model: ${model}` } instead of proceeding to section fan-out; keep the
existing fallback for non-pi models using CLI_MODELS unchanged and reference
isPiAiModel, getAvailablePiAiModels, and CLI_MODELS to locate where to add this
early-return validation.
---
Duplicate comments:
In `@src/commands/sync-shared.ts`:
- Around line 254-257: The snippet returns agentConfig.globalSkillsDir without
validation; update the code in the block that checks `global` to ensure
`agentConfig` exists and that `agentConfig.globalSkillsDir` is present (e.g.,
`const agentConfig = agents[agent]; if (!agentConfig?.globalSkillsDir) return
undefined; return agentConfig.globalSkillsDir;`) so it matches the defensive
checks used in `src/core/skills.ts`, `src/commands/uninstall.ts`, and
`src/agent/install.ts`.
---
Nitpick comments:
In `@src/agent/clis/index.ts`:
- Line 331: The code compiles /-/g each time when computing logName; extract
that regex into a module-level constant (e.g., DASH_REGEX = /-/g) and reuse it
in the logName computation and the other occurrence (around line with similar
replace at or near Line 505) so both places call
section.toUpperCase().replace(DASH_REGEX, '_') instead of compiling the literal
regex repeatedly; update references in this module to use the new constant.
In `@src/commands/sync.ts`:
- Around line 601-613: The call to getAvailableModels and the auto-resolution of
resolvedModel happens before checking allSectionsCached, causing unnecessary
work when allSectionsCached is true; move the auto-resolution logic (the block
that calls getAvailableModels and sets resolvedModel using available.find(...))
inside the existing if that guards llm usage (the if using !allSectionsCached &&
!globalConfig.skipLlm && !(config.yes && !resolvedModel)) so that
getAvailableModels is only invoked when you actually need to call
selectLlmConfig; keep the initial resolvedModel assignment from
config.model/readConfig as-is and ensure the later if still sets resolvedModel
before calling selectLlmConfig (referencing resolvedModel, getAvailableModels,
allSectionsCached, selectLlmConfig, readConfig, config.yes,
globalConfig.skipLlm).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c9073c5-51a9-4aec-b4ba-a423ba868fc3
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
package.jsonpnpm-workspace.yamlsrc/agent/clis/index.tssrc/commands/install.tssrc/commands/sync-parallel.tssrc/commands/sync-shared.tssrc/commands/sync.tstest/unit/sync-shared.test.ts
✅ Files skipped from review due to trivial changes (2)
- package.json
- test/unit/sync-shared.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/commands/install.ts
- pnpm-workspace.yaml
- src/commands/sync-parallel.ts
This was referenced Mar 24, 2026
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.
🔗 Linked issue
Related to pi-mono AI provider integration
❓ Type of change
📚 Description
Adds Pi AI as an OAuth provider via
@mariozechner/pi-ai, enabling non-CLI users to access LLM enhancement through browser-based OAuth flows. Reworks the CLI onboarding UX with a provider-first model picker, interactive setup command, enhanced wizard flow, and clearer sync output. Also improves agent detection when multiple agents match, prevents duplicate skill installs, and handles MDC instruction files.