feat: add skillkit doctor command and enhance skillkit status#64
feat: add skillkit doctor command and enhance skillkit status#64
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 multiple CLI commands (doctor, activity, issue plan/list, session snapshot save/restore/list/delete, session explain), new core session features (ActivityLog, SnapshotManager, SessionExplainer), an IssuePlanner implementation with types/tests, expanded docs/README, and registers/exports the commands in the CLI surface. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "CLI\n(Command)"
participant Env as "Environment\n(Node & Tools)"
participant Config as "Config\nFiles"
participant Agents as "Agent\nDetector"
participant Skills as "Skills\nFilesystem"
participant Output as "Output\nRenderer"
User->>CLI: run `skillkit doctor` [--fix] [--json]
CLI->>Env: probe Node, npm/pnpm, and tool versions
Env-->>CLI: versions/status
CLI->>Config: read/validate project config
Config-->>CLI: config validity/agent
CLI->>Agents: detect adapters/agents
Agents-->>CLI: detected agents
CLI->>Skills: inspect skill dirs, symlinks, and contents
Skills-->>CLI: inventory & issues
alt --fix provided
CLI->>Skills: create missing dirs / remove broken symlinks
Skills-->>CLI: actions taken
end
CLI->>Output: aggregate Environment, Config, Skills
alt --json
Output->>User: JSON payload
else
Output->>User: formatted health report
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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: 1
🤖 Fix all issues with AI agents
In `@packages/cli/src/commands/doctor.ts`:
- Line 113: The code uses "as any" casts for detectedAgent when calling
getAdapter and getSearchDirs; instead, declare detectedAgent with the explicit
type AgentType (e.g., let detectedAgent: AgentType = 'universal') when assigning
from detectAgent(), then remove the two "as any" casts; update references to
detectedAgent in getAdapter(...) and getSearchDirs(...) to use the typed
variable so type safety is preserved (refer to detectedAgent, detectAgent,
AgentType, getAdapter, getSearchDirs).
🧹 Nitpick comments (5)
packages/cli/src/commands/doctor.ts (4)
286-294: Command injection risk inwhichVersion— mitigate defensively.Although
whichVersionis private and currently only called with hardcoded literals ('npm','pnpm'), passingcmddirectly intoexecSyncis a shell-injection vector if a future caller ever passes untrusted input. A minimal safeguard (e.g., allowlist orexecFileSync) eliminates this risk.🛡️ Proposed safer alternative using execFileSync
+import { execFileSync } from 'node:child_process'; ... private whichVersion(cmd: string): string | null { try { - return execSync(`${cmd} --version`, { encoding: 'utf-8', timeout: 5000 }) + return execFileSync(cmd, ['--version'], { encoding: 'utf-8', timeout: 5000 }) .trim() .replace(/^v/, ''); } catch { return null; } }
execFileSyncbypasses the shell entirely, avoiding injection even ifcmdis ever sourced dynamically.
61-64: pnpm check is silently omitted when not found.When
npmis not found, a warning is pushed (Line 58). Whenpnpmis not found, nothing is pushed — the check just disappears from output. This is probably intentional (pnpm is optional), but the asymmetry may confuse users who expect to see all checks listed. Consider pushing a'pass'result like"pnpm not installed (optional)"for visibility, or adding a brief comment explaining the intentional omission.
134-134: Hardcoded agent-to-directory mapping is fragile.The ternary
detectedAgent === 'claude-code' ? 'claude' : detectedAgentembeds knowledge about agent directory naming that likely belongs in the adapter layer (which already knows aboutskillsDir). If a new agent is added with a different directory convention, this line silently produces the wrong path.Consider querying the adapter for the global skills directory instead, or at minimum extracting this mapping into a shared helper.
42-276: Theexecute()method is quite long (~235 lines) — consider extracting check functions.Each check category (environment, configuration, agent detection, skills, symlinks) could be a private method returning
CheckResult[]. This would improve readability and testability without changing behavior.Additionally, there's notable duplication with
buildOverview()instatus.ts(agent detection, config loading, skills discovery). Extracting the shared data-gathering logic into a common helper would reduce drift between the two commands.packages/cli/src/commands/status.ts (1)
140-185: ReplaceRecord<string, unknown>with a typed interface to avoid unsafe casts.
buildOverviewreturnsRecord<string, unknown>, andshowOverviewimmediately casts fields back (as number,as Array<…>) on lines 195 and 202. Since both methods are private and co-located, introducing a small interface eliminates the casts and catches shape mismatches at compile time.♻️ Proposed interface
interface StatusOverview { agent: string; config: string | null; configAgent: string; version: string; totalSkills: number; projectSkills: number; globalSkills: number; skillsDir: string; recentHistory: Array<{ status: string; skillName: string; completedAt: string }>; }Then update signatures:
-private async buildOverview(manager: SessionManager): Promise<Record<string, unknown>> { +private async buildOverview(manager: SessionManager): Promise<StatusOverview> { ... -private showOverview(overview: Record<string, unknown>): void { +private showOverview(overview: StatusOverview): void {Also applies to: 187-215
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@packages/cli/src/__tests__/commands.test.ts`:
- Around line 23-29: Add missing assertions to the test to verify the new
exports: in packages/cli/src/__tests__/commands.test.ts update the expectation
block that checks exported commands to also assert that DoctorCommand,
IssuePlanCommand, and IssueListCommand are defined (i.e., add
expect(commands.DoctorCommand).toBeDefined(),
expect(commands.IssuePlanCommand).toBeDefined(), and
expect(commands.IssueListCommand).toBeDefined()). Ensure the symbol names match
the exports in commands/index.ts so the test fails if the exports are not
present.
In `@packages/cli/src/commands/activity.ts`:
- Line 32: The code does parseInt(this.limit) without validation so a
non-numeric --limit yields NaN and silent empty output; change parsing to use
parseInt(this.limit, 10) and validate the result (e.g., const parsed =
parseInt(this.limit, 10); if (!Number.isFinite(parsed) || Number.isNaN(parsed))
{ log a concise warning/error to the user and default limit to 10 } then use
that validated value for limit (referenced as limit and this.limit in the file);
ensure you handle negative/zero values if needed and prefer a clear
error/warning before falling back.
In `@packages/cli/src/commands/issue.ts`:
- Around line 248-253: The JSON.parse(result) call that assigns to the issues
constant can throw on malformed gh output; wrap the parse in a try/catch (or use
a safe parse helper) around JSON.parse(result) to catch SyntaxError, validate
that result is non-empty before parsing, and on failure surface a user-friendly
error (e.g., log a clear message and exit gracefully or return an empty issues
array) referencing the same symbols: result and issues; update any downstream
code that assumes issues is an array to handle the fallback case.
In `@packages/core/src/plan/issue-planner.ts`:
- Around line 202-211: The ternary spreading an empty array makes includeTests a
no-op; update the steps passed to generator.addTask so the true branch yields
actual test step objects instead of an empty array. Specifically, in the addTask
call that builds steps (currently using ...(includeTests ? [] : [])), replace
the true branch with an array of test step descriptors (e.g. { type: 'test',
description: `Write tests for ${item.name}` } or similar) so when includeTests
is true additional test steps are appended to the steps array; keep the false
branch as an empty array to preserve current behavior.
In `@packages/core/src/plan/types.ts`:
- Around line 274-280: Remove the duplicate IssuePlanMetadata interface declared
in issue-planner.ts and instead import the canonical IssuePlanMetadata from
types.ts; update issue-planner.ts by deleting its local IssuePlanMetadata
declaration and adding an import statement for IssuePlanMetadata, then ensure
any references in issue-planner.ts use the imported type.
In `@packages/core/src/session/session-explainer.ts`:
- Around line 49-59: The code mistakenly sets explanation.agent to
exec.skillSource (a repo path) in the session-explainer, which misrepresents the
AI agent; update the logic in the block using state.currentExecution (where exec
is a CurrentExecution) to populate explanation.agent from the actual agent
source (e.g., a field like exec.agent, state.agent, session.agent, or
executionContext.provider) if such metadata exists, or if no agent info is
available add/derive an agent field on CurrentExecution and populate it where
executions are created; alternatively, rename explanation.agent to
explanation.skillSource to avoid confusion and update usages including
formatText to match the chosen change (reference symbols: explanation.agent,
exec.skillSource, CurrentExecution, formatText).
🧹 Nitpick comments (8)
packages/core/src/session/activity-log.ts (2)
75-80:getByCommitprefix match could return unexpected results for short/empty SHAs.An empty string matches every commit (
"".startsWith("")is true), and very short prefixes like"a"may match unrelated commits. Since.find()returns the first match, the result is non-deterministic when multiple commits share a prefix. Consider adding a minimum-length guard:🛡️ Suggested guard
getByCommit(sha: string): SkillActivity | undefined { + if (sha.length < 4) return undefined; const data = this.load(); return data.activities.find( (a) => a.commitSha === sha || a.commitSha.startsWith(sha) ); }
45-51:save()doesn't handle write errors.If
writeFileSyncthrows (e.g., disk full, permissions), the error propagates unhandled up throughrecord(). The in-memorythis.datawill already be mutated, creating a divergence between memory and disk. Consider wrapping with a try/catch or documenting this contract.packages/cli/src/commands/session.ts (3)
360-365: Double type assertion throughunknownmasks potential type mismatches.
new ObservationStore(projectPath).getAll() as unknown as Array<Record<string, unknown>>forces the type. IfSnapshotManager.save()expects a specific type for observations, consider aligning the types properly rather than casting throughunknown.
395-404: Restore usesObject.assignto mutate internal state directly.This couples the CLI command to the internal shape of
SessionManager's state object. IfSessionManagerchanges its internal representation or adds validation on mutation, this will break silently. Consider adding a dedicatedrestoreormergemethod onSessionManagerinstead.
343-367: Remove unnecessary double type assertion on observations.
new ObservationStore(projectPath).getAll()already returnsObservation[]with proper typing. The castas unknown as Array<Record<string, unknown>>is unnecessary and loses type safety. Cast directly to the expected type or update the SnapshotManager.save() signature to acceptObservation[]if that's what's available.packages/core/src/session/snapshot-manager.ts (1)
40-58: Unsafe cast ofobservationsparameter bypasses type safety.The
observationsparameter is typed asArray<Record<string, unknown>>but is cast toSessionSnapshot['observations'](which requires specific fields likeid,timestamp,sessionId,agent,type,content,relevance). This means callers can pass any shape of objects without compile-time errors, and restored snapshots may have missing fields at runtime.Consider aligning the parameter type with
SessionSnapshot['observations']to get compile-time guarantees:Proposed fix
save( name: string, sessionState: SessionState, - observations: Array<Record<string, unknown>>, + observations: SessionSnapshot['observations'], description?: string ): void { this.ensureDir(); const snapshot: SessionSnapshot = { version: 1, name, createdAt: new Date().toISOString(), description, sessionState, - observations: observations as SessionSnapshot['observations'], + observations, }; writeFileSync(this.getPath(name), stringify(snapshot)); }packages/cli/src/__tests__/e2e-session-features.test.ts (1)
84-118: Redundantrequire()calls — use top-level imports instead.
writeFileSyncis already imported at line 3, andyamlcould be imported at the top level. The inlinerequire('yaml')(line 85) andrequire('node:fs')(line 114) are unnecessary and mix CJSrequirewith ESM-style imports.Suggested cleanup
Add
stringifyfromyamlto the top-level imports:+import { stringify } from 'yaml';Then remove the inline
requirecalls from the test bodies at lines 85, 114, and 161–162.packages/core/src/plan/issue-planner.ts (1)
108-113:JSON.parseofghoutput is unguarded.If
ghreturns non-JSON (e.g., HTML error page from a proxy),JSON.parsethrows a generic error. The CLI command catches this, but the error message won't be user-friendly. Consider wrapping with a descriptive error:Proposed fix
- const data = JSON.parse(result); + let data: Record<string, unknown>; + try { + data = JSON.parse(result); + } catch { + throw new Error(`Failed to parse GitHub CLI response for issue ${parsed.number}`); + }
| if (state.currentExecution) { | ||
| const exec = state.currentExecution; | ||
| explanation.agent = exec.skillSource || 'unknown'; | ||
|
|
||
| const startTime = new Date(exec.startedAt).getTime(); | ||
| explanation.duration = formatDuration(Date.now() - startTime); | ||
|
|
||
| explanation.skillsUsed.push({ | ||
| name: exec.skillName, | ||
| status: exec.status, | ||
| }); |
There was a problem hiding this comment.
explanation.agent is populated with skillSource (repo path), not the actual agent name.
exec.skillSource is the skill's repository (e.g., "anthropics/skills"), but the field name agent and its usage in formatText (Line 137) suggest it should represent the AI agent (e.g., "cursor", "claude"). The CurrentExecution type doesn't carry an agent field, so this may need to be sourced differently or the field renamed to avoid confusion in the output.
🤖 Prompt for AI Agents
In `@packages/core/src/session/session-explainer.ts` around lines 49 - 59, The
code mistakenly sets explanation.agent to exec.skillSource (a repo path) in the
session-explainer, which misrepresents the AI agent; update the logic in the
block using state.currentExecution (where exec is a CurrentExecution) to
populate explanation.agent from the actual agent source (e.g., a field like
exec.agent, state.agent, session.agent, or executionContext.provider) if such
metadata exists, or if no agent info is available add/derive an agent field on
CurrentExecution and populate it where executions are created; alternatively,
rename explanation.agent to explanation.skillSource to avoid confusion and
update usages including formatText to match the chosen change (reference
symbols: explanation.agent, exec.skillSource, CurrentExecution, formatText).
- New `skillkit doctor` command with 10 health checks across 3 categories (environment, configuration, skills) with --json and --fix flags - Enhanced `skillkit status` shows project overview dashboard when no active session exists instead of bare "No active session found" message - Overview includes agent, config, version, skills count, recent history
Three new session intelligence features: - Activity Log: tracks which skills were active per git commit (`skillkit activity` with --skill, --limit, --json flags) - Session Snapshots: save/restore named snapshots of session state (`skillkit session snapshot save|restore|list|delete`) - Session Explain: structured summary of current session (`skillkit session explain` with --json, --no-git flags) Includes path traversal protection on snapshot names, YAML validation on parsed activity data, and robust error handling on snapshot restore. 983 tests passing (942 core + 41 CLI including 14 E2E tests).
New `skillkit issue plan` and `skillkit issue list` commands that fetch GitHub Issues via `gh` CLI and generate StructuredPlans. Parses checkboxes, file mentions, and labels from issue body. 29 new tests, all passing.
rohitg00
force-pushed
the
feat/doctor-status-commands
branch
from
c34386d to
58ff71e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/cli/src/commands/doctor.ts`:
- Line 134: Replace the hardcoded globalSkillsDir construction with the
canonical mapping from AGENT_CONFIG: import AGENT_CONFIG from '@skillkit/core',
then set globalSkillsDir to AGENT_CONFIG[detectedAgent].globalSkillsDir with any
leading '~' replaced by homedir(); reference the existing detectedAgent and
globalSkillsDir symbols and ensure you handle a missing mapping by falling back
to the previous join(homedir(), `.${detectedAgent === 'claude-code' ? 'claude' :
detectedAgent}`, 'skills') behavior if AGENT_CONFIG[detectedAgent] is undefined.
In `@packages/cli/src/commands/status.ts`:
- Around line 140-185: Replace the untyped Record<string, unknown> return with a
concrete interface (e.g. StatusOverview or Overview) that lists the properties
returned by buildOverview (agent, config, configAgent, version, totalSkills,
projectSkills, globalSkills, skillsDir, recentHistory) and their proper types;
change buildOverview signature to return that interface and update any callers
(notably showOverview) to use the typed interface so you can remove the unsafe
casts (e.g. `as number`, `as Array<...>`). Ensure you import or declare the
interface in the same module (or a shared types module) and update
getAdapter/manager return types if needed to satisfy the new property types
(e.g. adapter.skillsDir, recentHistory).
In `@packages/core/src/session/session-explainer.ts`:
- Line 99: state.decisions can be undefined and calling .map on it will throw;
update the assignment in session-explainer.ts so that explanation.decisions is
populated from a safe default (e.g., use (state.decisions ?? []) or optional
chaining) before mapping; specifically change the expression that sets
explanation.decisions to handle undefined SessionState.decisions so mapping runs
over an empty array when absent.
- Around line 82-95: state.history may be undefined and iterating it with
for...of will throw; change the loop to iterate over a safe default (e.g.,
state.history ?? []) so the code defensively handles missing history entries.
Update the block that reads state.history and still populates
explanation.skillsUsed and explanation.filesModified (the same places that check
hist.skillName, hist.status, and hist.filesModified) to iterate the fallback
empty array.
🧹 Nitpick comments (16)
packages/core/src/session/activity-log.ts (3)
22-43: Loaded data is cached indefinitely — no version field validation.Two minor observations on
load():
- The
versionfield from the parsed YAML is not validated (Line 33 only checksparsed.activities). If the file hasversion: 2with a different schema, it will still be loaded asActivityLogData. For forward-compatibility, consider rejecting unknown versions.- Once
this.datais set, subsequent calls always return the cached value. This is fine for a short-lived CLI process, but worth noting if this class is ever reused in a long-running context.Neither is blocking for a CLI tool.
75-80: Prefix matching ingetByCommitcan return an ambiguous result.
a.commitSha.startsWith(sha)will match the first activity whose SHA shares the prefix. If multiple activities have SHAs with the same prefix, the caller gets whichever appears first (most recent) with no indication of ambiguity. This mirrors git's short-SHA behavior, so it's reasonable — just worth documenting or logging a warning if multiple matches exist.
45-51:save()could writenullif called beforeload().
this.datais initialized asnull. While currentlysave()is only reachable viarecord()(which callsload()first), a future refactor callingsave()directly would silently write"null\n"to disk. A small guard would make the class more robust.🛡️ Defensive check
private save(): void { + if (!this.data) return; const dir = join(this.projectPath, '.skillkit'); if (!existsSync(dir)) { mkdirSync(dir, { recursive: true }); } writeFileSync(this.filePath, stringify(this.data)); }packages/core/src/session/__tests__/activity-log.test.ts (2)
323-333: Consider adding a test forrecord()after encountering a corrupted file.The corrupted-file test only exercises the read path (
getRecent). It would be valuable to also verify thatrecord()gracefully recovers from a corrupted file — i.e., loads an empty log and successfully writes a new entry. This ensures the full recovery path works end-to-end.
32-109:existsSyncmock doesn't differentiate file vs. directory checks.In
recordtests,existsSyncis mocked to return a single value, but the implementation calls it for both the YAML file (inload()) and the.skillkitdirectory (insave()). The tests work because the broad mock doesn't cause failures, but more targeted mocking (e.g., usingmockImplementationkeyed on path) would make the tests more precise and less brittle.packages/cli/src/commands/doctor.ts (2)
286-294: PreferexecFileSyncover template-literalexecSyncto avoid shell injection surface.While
cmdis currently always a hardcoded literal ('npm','pnpm'), usingexecSyncwith string interpolation opens a shell injection surface if this helper is ever called with dynamic input.execFileSyncavoids spawning a shell entirely and is safer by default.Suggested fix
+ import { execFileSync } from 'node:child_process'; ... private whichVersion(cmd: string): string | null { try { - return execSync(`${cmd} --version`, { encoding: 'utf-8', timeout: 5000 }) + return execFileSync(cmd, ['--version'], { encoding: 'utf-8', timeout: 5000 }) .trim() .replace(/^v/, ''); } catch { return null; } }
66-67: SkillKit version always reportspasseven when unknown.If
this.cli.binaryVersionis nullish, the version resolves to'unknown'but is still reported as'pass'. Consider using'warn'status to surface that the version couldn't be determined.Suggested fix
const skillkitVersion = this.cli.binaryVersion ?? 'unknown'; - results.push({ name: 'SkillKit version', status: 'pass', message: `skillkit v${skillkitVersion}` }); + results.push({ + name: 'SkillKit version', + status: skillkitVersion === 'unknown' ? 'warn' : 'pass', + message: `skillkit v${skillkitVersion}`, + });packages/core/src/plan/issue-planner.ts (1)
108-127: WrapJSON.parsewith a more descriptive error forghCLI failures.If
ghreturns non-JSON output (e.g., authentication errors, stderr redirected to stdout),JSON.parse(result)will throw a genericSyntaxError. Consider catching and re-throwing with context about theghcommand failure.Suggested improvement
- const data = JSON.parse(result); + let data: Record<string, unknown>; + try { + data = JSON.parse(result); + } catch { + throw new Error( + `Failed to parse GitHub issue data. Ensure "gh" is authenticated and the issue exists.\nRaw output: ${result.slice(0, 200)}` + ); + }packages/cli/src/commands/status.ts (1)
163-163: Sameas anyissue asdoctor.ts— use an explicitAgentTypeannotation foragent.Lines 163 and 171 cast
agentwithas anyforgetSearchDirsandgetAdapter. The same fix applies: declareagentwith theAgentTypetype so both calls are type-safe without casts.Also applies to: 171-171
packages/cli/src/commands/issue.ts (1)
53-95: Plan generation / validation errors are unhandled after thefetchIssuetry/catch.If
planner.generatePlan,validator.validate,generator.toMarkdown,mkdir, orwriteFilethrow, the error propagates unhandled, producing an unfriendly stack trace. Consider wrapping lines 79–95 in a try/catch that writes a clear error message tostderrand returns 1.Proposed fix
const plan = planner.generatePlan(issue, { agent: this.agent, techStack: techStackArr, includeTests: !this.noTests, }); - const validator = createPlanValidator(); - const validation = validator.validate(plan); - - const generator = createPlanGenerator(); - const markdown = generator.toMarkdown(plan); - - const outputPath = - this.output || resolve(`.skillkit/plans/issue-${issue.number}.md`); - - await mkdir(dirname(outputPath), { recursive: true }); - await writeFile(outputPath, markdown, 'utf-8'); + let validation; + let outputPath: string; + try { + const validator = createPlanValidator(); + validation = validator.validate(plan); + + const generator = createPlanGenerator(); + const markdown = generator.toMarkdown(plan); + + outputPath = + this.output || resolve(`.skillkit/plans/issue-${issue.number}.md`); + + await mkdir(dirname(outputPath), { recursive: true }); + await writeFile(outputPath, markdown, 'utf-8'); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + this.context.stderr.write(`Error generating plan: ${message}\n`); + return 1; + }packages/core/src/session/session-explainer.ts (1)
114-124: Git commit filtering fetches up to 50 commits but only counts today's — consider passingsinceto reduce I/O.
getGitCommitssupports asinceoption. Passing today's date assincewould avoid fetching commits from previous days that are immediately discarded.Proposed fix
try { - const commits = getGitCommits(this.projectPath, { commits: 50 }); const today = new Date().toISOString().split('T')[0]; - explanation.gitCommits = commits.filter((c) => - c.date.startsWith(today) - ).length; + const commits = getGitCommits(this.projectPath, { commits: 50, since: today }); + explanation.gitCommits = commits.length; } catch {packages/cli/src/commands/session.ts (3)
395-404: Snapshot restore silently drops observations.
manager.restore()returns bothsessionStateandobservations, but onlysessionStateis applied here. If a user saved a snapshot expecting to capture observations, restoring it won't bring them back. Consider restoring observations viaObservationStoreor at minimum documenting this limitation.
399-403:Object.assignpartial overwrite may cause subtle state inconsistencies.This copies
currentExecution,history, anddecisionsfrom the snapshot but retains the current state's other fields (e.g.,lastActivity,projectPath,version). If the snapshot was taken from a different project path or version, the hybrid state could be inconsistent. Consider either doing a full replacement (preserving onlyprojectPath) or documenting which fields are restored.
360-365: Double type cast (as unknown as Array<Record<string, unknown>>) is a code smell.This suggests a type mismatch between
ObservationStore.getAll()return type and whatSnapshotManager.save()expects. Consider aligning the types at the interface level rather than casting at the call site.packages/cli/src/__tests__/e2e-session-features.test.ts (2)
85-85: Prefer top-level imports over inlinerequire()calls.
writeFileSyncis already imported at Line 3 fromnode:fs, andyamlcould be imported at the top of the file. Usingrequire()inline is non-idiomatic in an ESM/vitest project and adds unnecessary overhead per test.Proposed fix (at the top of the file)
import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { execSync } from 'node:child_process'; -import { mkdirSync, rmSync, existsSync, readFileSync } from 'node:fs'; +import { mkdirSync, rmSync, existsSync, readFileSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; +import { stringify } from 'yaml';Then replace the inline
requirecalls:- const { stringify } = require('yaml'); // ... - const { writeFileSync } = require('node:fs');Also applies to: 114-114, 161-162
9-20:run()helper swallows all errors and returns a best-effort string.The
catchblock returnserr.stdout || err.stderr || err.message. Ifstdoutis an empty string (falsy), it falls through tostderrormessage, potentially hiding the real output. Consider usingerr.stdout ?? err.stderr ?? err.message(nullish coalescing) instead of||to preserve empty-string stdout.Proposed fix
} catch (err: any) { - return err.stdout || err.stderr || err.message; + return err.stdout ?? err.stderr ?? err.message; }
packages/cli/src/commands/doctor.ts
Outdated
| } | ||
| } | ||
|
|
||
| const globalSkillsDir = join(homedir(), `.${detectedAgent === 'claude-code' ? 'claude' : detectedAgent}`, 'skills'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for globalSkillsDir, global skills dir patterns, and related patterns
rg -n --type=ts 'globalSkillsDir\|global.*skills.*dir\|globalDir' -g '!node_modules' -g '!*.test.*' -C 3Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Also search for adapter-related code and agent configuration
rg -n --type=ts 'adapter.*skills\|getGlobalDir\|globalDirectory\|skills.*dir' -g '!node_modules' -g '!*.test.*' -C 3Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check how the file packages/cli/src/commands/doctor.ts uses agent directories
cat -n packages/cli/src/commands/doctor.ts | head -150Repository: rohitg00/skillkit
Length of output: 6731
🏁 Script executed:
#!/bin/bash
# Search for agent adapter implementations to understand the architecture
fd -t f 'adapter' -path '*/src/*' --type f | head -20Repository: rohitg00/skillkit
Length of output: 229
🏁 Script executed:
#!/bin/bash
# Look for agent-related configuration or constants that map agent names to directories
rg -n --type=ts 'claude-code\|detectedAgent' -g '!node_modules' -g '!*.test.*' -C 2Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Find adapter-related files
find . -type f -name '*adapter*' | grep -v node_modules | head -20Repository: rohitg00/skillkit
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Look for the agents package and its structure
find . -path '*/agents/*' -type f -name '*.ts' | grep -v node_modules | head -30Repository: rohitg00/skillkit
Length of output: 1157
🏁 Script executed:
#!/bin/bash
# Search for the adapter interface or getAdapter implementation
rg -l 'getAdapter\|interface.*Adapter\|class.*Adapter' --type=ts -g '!node_modules' | head -10Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look for any agent name mapping or configuration
rg -n 'claude-code\|agent.*name\|agent.*mapping' --type=ts -g '!node_modules' -C 2 | head -50Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check the base adapter interface
cat -n packages/agents/src/base.ts | head -100Repository: rohitg00/skillkit
Length of output: 1105
🏁 Script executed:
#!/bin/bash
# Check claude-code agent implementation
cat -n packages/agents/src/claude-code.tsRepository: rohitg00/skillkit
Length of output: 2936
🏁 Script executed:
#!/bin/bash
# Check factory to understand getAdapter
cat -n packages/agents/src/factory.tsRepository: rohitg00/skillkit
Length of output: 2838
🏁 Script executed:
#!/bin/bash
# Check all agent adapters to see how many have the agent-name-to-dir mapping
for file in packages/agents/src/*.ts; do
if [[ "$file" == *test* ]] || [[ "$file" == *features* ]] || [[ "$file" == *index* ]] || [[ "$file" == *factory* ]] || [[ "$file" == *base* ]]; then
continue
fi
echo "=== $(basename $file) ==="
grep -n "homedir\|join.*\." "$file" | head -5
doneRepository: rohitg00/skillkit
Length of output: 3919
🏁 Script executed:
#!/bin/bash
# Get the full getAdapter/getAllAdapters implementation
rg -A 30 'export.*function.*getAdapter|export.*function.*getAllAdapters' packages/agents/src/index.tsRepository: rohitg00/skillkit
Length of output: 868
🏁 Script executed:
#!/bin/bash
# Find where AGENT_CONFIG is defined
rg -n 'AGENT_CONFIG\|export.*config' packages/core/src --type=ts | head -30Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check the core package for AGENT_CONFIG definition
find packages/core/src -name '*.ts' | xargs grep -l "AGENT_CONFIG"Repository: rohitg00/skillkit
Length of output: 204
🏁 Script executed:
#!/bin/bash
# Check the agent-config.ts to see if it has globalSkillsDir
cat -n packages/core/src/agent-config.ts | head -200Repository: rohitg00/skillkit
Length of output: 6641
Use centralized AGENT_CONFIG for global skills directory mapping.
The hardcoded mapping breaks for agents with non-standard global paths. For example, opencode should resolve to ~/.config/opencode/skills and windsurf to ~/.codeium/windsurf/skills, but the current logic constructs ~/.opencode/skills and ~/.windsurf/skills respectively. Instead of manual path construction, import AGENT_CONFIG from @skillkit/core and use AGENT_CONFIG[detectedAgent].globalSkillsDir (after replacing ~ with homedir()), which already has the correct mappings for all agents.
🤖 Prompt for AI Agents
In `@packages/cli/src/commands/doctor.ts` at line 134, Replace the hardcoded
globalSkillsDir construction with the canonical mapping from AGENT_CONFIG:
import AGENT_CONFIG from '@skillkit/core', then set globalSkillsDir to
AGENT_CONFIG[detectedAgent].globalSkillsDir with any leading '~' replaced by
homedir(); reference the existing detectedAgent and globalSkillsDir symbols and
ensure you handle a missing mapping by falling back to the previous
join(homedir(), `.${detectedAgent === 'claude-code' ? 'claude' :
detectedAgent}`, 'skills') behavior if AGENT_CONFIG[detectedAgent] is undefined.
| for (const hist of state.history) { | ||
| const alreadyListed = explanation.skillsUsed.some( | ||
| (s) => s.name === hist.skillName | ||
| ); | ||
| if (!alreadyListed) { | ||
| explanation.skillsUsed.push({ | ||
| name: hist.skillName, | ||
| status: hist.status, | ||
| }); | ||
| } | ||
| if (hist.filesModified) { | ||
| explanation.filesModified.push(...hist.filesModified); | ||
| } | ||
| } |
There was a problem hiding this comment.
state.history may also be undefined if the session YAML is incomplete.
Same concern as state.decisions — if history is missing from the deserialized state, iterating with for...of will throw. Defensively default to an empty array.
Proposed fix
- for (const hist of state.history) {
+ for (const hist of state.history ?? []) {📝 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.
| for (const hist of state.history) { | |
| const alreadyListed = explanation.skillsUsed.some( | |
| (s) => s.name === hist.skillName | |
| ); | |
| if (!alreadyListed) { | |
| explanation.skillsUsed.push({ | |
| name: hist.skillName, | |
| status: hist.status, | |
| }); | |
| } | |
| if (hist.filesModified) { | |
| explanation.filesModified.push(...hist.filesModified); | |
| } | |
| } | |
| for (const hist of state.history ?? []) { | |
| const alreadyListed = explanation.skillsUsed.some( | |
| (s) => s.name === hist.skillName | |
| ); | |
| if (!alreadyListed) { | |
| explanation.skillsUsed.push({ | |
| name: hist.skillName, | |
| status: hist.status, | |
| }); | |
| } | |
| if (hist.filesModified) { | |
| explanation.filesModified.push(...hist.filesModified); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/core/src/session/session-explainer.ts` around lines 82 - 95,
state.history may be undefined and iterating it with for...of will throw; change
the loop to iterate over a safe default (e.g., state.history ?? []) so the code
defensively handles missing history entries. Update the block that reads
state.history and still populates explanation.skillsUsed and
explanation.filesModified (the same places that check hist.skillName,
hist.status, and hist.filesModified) to iterate the fallback empty array.
|
|
||
| explanation.filesModified = [...new Set(explanation.filesModified)]; | ||
|
|
||
| explanation.decisions = state.decisions.map(({ key, value }) => ({ key, value })); |
There was a problem hiding this comment.
state.decisions may be undefined, causing a runtime crash.
If SessionState is deserialized from YAML and the decisions field is absent, state.decisions will be undefined. Calling .map() on undefined throws a TypeError.
Proposed fix
- explanation.decisions = state.decisions.map(({ key, value }) => ({ key, value }));
+ explanation.decisions = (state.decisions ?? []).map(({ key, value }) => ({ key, value }));🤖 Prompt for AI Agents
In `@packages/core/src/session/session-explainer.ts` at line 99, state.decisions
can be undefined and calling .map on it will throw; update the assignment in
session-explainer.ts so that explanation.decisions is populated from a safe
default (e.g., use (state.decisions ?? []) or optional chaining) before mapping;
specifically change the expression that sets explanation.decisions to handle
undefined SessionState.decisions so mapping runs over an empty array when
absent.
- doctor.ts: use execFileSync, typed AgentType, remove as-any casts - activity.ts: validate parseInt with radix and NaN guard - issue.ts: wrap JSON.parse in try/catch for gh CLI output - issue-planner.ts: guard JSON.parse, fix includeTests no-op ternary, remove duplicate IssuePlanMetadata (import from types.ts) - status.ts: typed StatusOverview interface, remove as-any casts - snapshot-manager.ts: align observations param with SessionSnapshot type - session.ts: proper type mapping for observations (no double cast) - activity-log.ts: min-length guard for SHA prefix match - e2e tests: use top-level imports instead of inline require() - commands.test.ts: add assertions for DoctorCommand, IssuePlanCommand, IssueListCommand
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/src/plan/issue-planner.ts`:
- Around line 220-233: The no-checklist fallback creates a single implement step
and ignores the includeTests flag; update the branch around generator.addTask
(where files: PlanTaskFiles is built from allFileMentions) to conditionally
append a { type: 'test', description: issue.title } step when includeTests is
true (same pattern used in the checklist branch), so the steps array passed to
generator.addTask(plan, issue.title, {...}) contains both implement and test
steps when includeTests is enabled.
🧹 Nitpick comments (4)
packages/core/src/session/activity-log.ts (1)
30-40: Consider validatingversionfield before casting toActivityLogData.Line 33 checks that
parsed.activitiesis an array but doesn't validateparsed.version === 1. TheActivityLogDatainterface requiresversion: 1, so a file with a different or missing version would be silently accepted with a potentially incompatible structure.Suggested tightening
const parsed = parse(content); - if (parsed && Array.isArray(parsed.activities)) { + if (parsed && parsed.version === 1 && Array.isArray(parsed.activities)) { this.data = parsed as ActivityLogData;packages/cli/src/__tests__/e2e-session-features.test.ts (1)
10-21:run()helper silently swallows all errors — consider preserving exit code.When
execSyncthrows (non-zero exit), the helper returnserr.stdout || err.stderr || err.messagewithout indicating failure. Tests that assert on error messages work fine, but a test that accidentally expects success output from a crashing command would silently pass with error output. This is acceptable for current test patterns but could be fragile if tests evolve.packages/cli/src/commands/session.ts (1)
362-375: Observation mapping uses multipleascasts that bypass type safety.Lines 368-370 cast
o.agent,o.type, ando.contentwithout runtime guards. IfObservationStore.getAll()returns records where these fields areundefinedor unexpected types, the snapshot data will silently contain incorrect values.Consider adding runtime fallbacks:
Suggested defensive mapping
observations = raw.map((o) => ({ id: o.id, timestamp: o.timestamp, sessionId: o.sessionId, - agent: o.agent as string, - type: o.type as string, - content: { ...o.content } as Record<string, unknown>, + agent: String(o.agent ?? 'unknown'), + type: String(o.type ?? 'unknown'), + content: (o.content && typeof o.content === 'object' ? { ...o.content } : {}) as Record<string, unknown>, relevance: o.relevance, }));packages/cli/src/commands/doctor.ts (1)
93-111: Nested async calls inside a sync-looking loop — verify adapter detection is quick.The
adapter.isDetected()call inside the loop (line 99) is awaited individually per adapter. If any adapter's detection is slow (e.g., network call), this will block sequentially. Consider usingPromise.allSettledfor parallel detection if latency becomes a concern.Parallel detection alternative
- try { - const allAdapters = getAllAdapters(); - for (const adapter of allAdapters) { - if (adapter.agentType === detectedAgent || adapter.agentType === 'universal') continue; - try { - if (await adapter.isDetected()) { - otherDetected.push(adapter.agentType); - } - } catch { - // skip - } - } + try { + const allAdapters = getAllAdapters() + .filter(a => a.agentType !== detectedAgent && a.agentType !== 'universal'); + const results = await Promise.allSettled(allAdapters.map(a => a.isDetected())); + for (let i = 0; i < allAdapters.length; i++) { + const result = results[i]; + if (result.status === 'fulfilled' && result.value) { + otherDetected.push(allAdapters[i].agentType); + } + }
- Include test step in no-checklist fallback when includeTests is true - Restore observations from snapshot in SessionSnapshotRestoreCommand
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/cli/src/commands/session.ts`:
- Around line 405-414: Explain in a short comment above the selective assign
where manager.restore(...) is applied that only currentExecution, history, and
decisions from the restored SessionState are intentionally copied into the
active SessionManager (via SessionManager.getOrCreate() and Object.assign(...))
while other fields such as lastActivity and projectPath are intentionally not
restored so the session is anchored to the current project context and
lastActivity is refreshed on save; mention SessionState and the specific fields
(currentExecution, history, decisions, lastActivity, projectPath) so future
maintainers understand the design choice.
🧹 Nitpick comments (3)
packages/core/src/plan/issue-planner.ts (2)
35-53: Minor: thegflag on the module-levelFILE_PATH_REGEXconstant is unused.The constant's
gflag is never leveraged becauseextractFileMentions(line 150) constructs a freshRegExpfromFILE_PATH_REGEX.source. Removinggfrom the constant makes it clear the stateful flag is only on the local copy.Also, the static analysis warning about ReDoS on line 150 is a false positive — the source is a compile-time constant, not user input.
Suggested tweak
-const FILE_PATH_REGEX = /`([a-zA-Z0-9_./\-]+\.[a-zA-Z0-9]+)`/g; +const FILE_PATH_REGEX = /`([a-zA-Z0-9_./\-]+\.[a-zA-Z0-9]+)`/;
88-130: WrapexecFileSyncto surface a friendlier error whenghis missing or fails.If the GitHub CLI isn't installed,
execFileSyncthrows anENOENTerror with no context about what went wrong. Similarly, auth failures or network errors will surface raw child-process messages. A try/catch with a user-oriented message would improve the developer experience.Proposed improvement
+ let result: string; + try { - const result = execFileSync('gh', args, { + result = execFileSync('gh', args, { encoding: 'utf-8', timeout: 15_000, }); + } catch (err: unknown) { + const code = (err as NodeJS.ErrnoException).code; + if (code === 'ENOENT') { + throw new Error( + 'GitHub CLI (gh) is not installed or not on PATH. Install it from https://cli.github.com/' + ); + } + throw new Error( + `Failed to fetch issue #${parsed.number} via GitHub CLI: ${(err as Error).message}` + ); + }packages/cli/src/commands/session.ts (1)
416-430: Observation restore uses fragileParameters<>type indexing.The positional
Parameters<ObservationStore['add']>[0],[1],[2]casts are brittle — if theaddmethod signature is reordered or extended, these will silently pass incorrect values at runtime. Consider a more explicit approach, or at minimum add a comment documenting the expectedaddsignature.
| const { sessionState, observations } = manager.restore(this.name); | ||
|
|
||
| const sessionMgr = new SessionManager(projectPath); | ||
| const currentState = sessionMgr.getOrCreate(); | ||
| Object.assign(currentState, { | ||
| currentExecution: sessionState.currentExecution, | ||
| history: sessionState.history, | ||
| decisions: sessionState.decisions, | ||
| }); | ||
| sessionMgr.save(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find SessionState interface definition
ast-grep --pattern 'interface SessionState {
$$$
}'Repository: rohitg00/skillkit
Length of output: 988
🏁 Script executed:
#!/bin/bash
# First, let's look at the broader context of the restore operation
sed -n '400,420p' packages/cli/src/commands/session.ts
# Also check what getOrCreate() does
fd -t f 'SessionManager' packages/ | head -5Repository: rohitg00/skillkit
Length of output: 749
🏁 Script executed:
#!/bin/bash
# Find SessionManager class
fd -t f 'SessionManager' packages/ --exec grep -l 'class SessionManager' {}
# Get more details about SessionManager
cat -n packages/core/src/session/SessionManager.ts | head -100Repository: rohitg00/skillkit
Length of output: 136
🏁 Script executed:
#!/bin/bash
# Search for SessionManager class definition
rg 'class SessionManager' packages/ -t ts -A 20
# Also search for the getOrCreate method
rg 'getOrCreate' packages/ -t ts -B 3 -A 10Repository: rohitg00/skillkit
Length of output: 4874
🏁 Script executed:
#!/bin/bash
# Find the init() method that creates a fresh session
rg 'init\(\).*SessionState' packages/core/src/session/manager.ts -A 20Repository: rohitg00/skillkit
Length of output: 476
🏁 Script executed:
#!/bin/bash
# Check the restore() method and what it does
rg 'restore\(' packages/ -t ts -B 5 -A 20 | head -60
# Also check if there are any comments about the snapshot/restore design
rg -i 'snapshot|restore' packages/cli/src/commands/session.ts -B 2 -A 2Repository: rohitg00/skillkit
Length of output: 9518
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or comments around the snapshot/restore feature
rg -i 'snapshot' packages/core/src/session/ -t ts -B 2 -A 2 | head -80
# Also verify the complete flow by checking what the snapshot save captures
rg 'save\(.*state.*observations' packages/core/src/session/snapshot-manager.ts -B 5 -A 15Repository: rohitg00/skillkit
Length of output: 5468
🏁 Script executed:
#!/bin/bash
# Let's check the complete context one more time to see if there are any comments
# or documentation about why selective restore is done this way
sed -n '395,425p' packages/cli/src/commands/session.tsRepository: rohitg00/skillkit
Length of output: 1128
🏁 Script executed:
#!/bin/bash
# Check the save() method to see if it updates lastActivity
rg 'save\(\)' packages/core/src/session/manager.ts -A 15Repository: rohitg00/skillkit
Length of output: 3205
🏁 Script executed:
#!/bin/bash
# Let me verify the flow one more time by checking what happens in init()
# to understand the complete picture
sed -n '20,45p' packages/core/src/session/manager.tsRepository: rohitg00/skillkit
Length of output: 649
🏁 Script executed:
#!/bin/bash
# Check for any tests related to snapshot restore to understand intended behavior
rg 'SessionSnapshotRestoreCommand|restore.*snapshot' packages/ -t ts -B 3 -A 10 | head -100
# Also check if there's any documentation or design decision about this
find packages -name "*.md" -type f | xargs grep -l "snapshot\|restore" 2>/dev/null | head -3Repository: rohitg00/skillkit
Length of output: 8253
🏁 Script executed:
#!/bin/bash
# Let me check the e2e test to understand the intended behavior better
sed -n '200,280p' packages/cli/src/__tests__/e2e-session-features.test.tsRepository: rohitg00/skillkit
Length of output: 1380
🏁 Script executed:
#!/bin/bash
# Find the actual restore test that checks the behavior
rg 'should save and restore snapshot' packages/cli/src/__tests__/e2e-session-features.test.ts -A 80Repository: rohitg00/skillkit
Length of output: 3046
Add a clarifying comment for the selective field restore.
The code intentionally restores only currentExecution, history, and decisions from the snapshot, while lastActivity is refreshed to the current time (via save()) and projectPath remains bound to the current project. This design preserves execution history while anchoring the restored session to the current context. However, without documentation, future maintainers—or developers adding new SessionState fields—may not realize this is intentional. Add a comment explaining the selective restore strategy:
const currentState = sessionMgr.getOrCreate();
+ // Restore execution history into current session, preserving session identity
+ // (version, lastActivity, projectPath stay current; only execution data is restored)
Object.assign(currentState, {
currentExecution: sessionState.currentExecution,
history: sessionState.history,
decisions: sessionState.decisions,
});
sessionMgr.save();🤖 Prompt for AI Agents
In `@packages/cli/src/commands/session.ts` around lines 405 - 414, Explain in a
short comment above the selective assign where manager.restore(...) is applied
that only currentExecution, history, and decisions from the restored
SessionState are intentionally copied into the active SessionManager (via
SessionManager.getOrCreate() and Object.assign(...)) while other fields such as
lastActivity and projectPath are intentionally not restored so the session is
anchored to the current project context and lastActivity is refreshed on save;
mention SessionState and the specific fields (currentExecution, history,
decisions, lastActivity, projectPath) so future maintainers understand the
design choice.
- doctor.ts: Use getAdapter() per AgentType instead of getAllAdapters() spread (which destroys prototype methods like isDetected) - doctor.ts: Use getGlobalSkillsDir() from AGENT_CONFIG instead of hardcoded path formula - session-explainer.ts: Defensively default state.history and state.decisions to empty arrays - session-explainer.ts: Use loadConfig().agent with skillSource fallback instead of always using skillSource as agent name - observation-store.ts: Add static readAll() that reads observations without sessionId mismatch clearing - session.ts: Use ObservationStore.readAll() for snapshot save to avoid empty observations due to sessionId mismatch
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const store = new ObservationStore(projectPath); | ||
| for (const obs of observations) { | ||
| store.add( | ||
| obs.type as Parameters<ObservationStore['add']>[0], | ||
| obs.content as unknown as Parameters<ObservationStore['add']>[1], | ||
| obs.agent as Parameters<ObservationStore['add']>[2], | ||
| obs.relevance, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 Snapshot restore silently wipes existing observations and loses original observation metadata
When restoring a snapshot via SessionSnapshotRestoreCommand, observations from the snapshot lose their original IDs, timestamps, and sessionIds because ObservationStore.add() is used instead of directly writing the snapshot data.
Root Cause and Impact
At packages/cli/src/commands/session.ts:418, a new ObservationStore(projectPath) is created without a sessionId, so a random UUID is generated (packages/core/src/memory/observation-store.ts:40). When store.add() is called for the first restored observation, the internal load() method reads the existing observations file and compares sessionIds. Since the random UUID never matches the file's sessionId, line 60-62 of observation-store.ts clears the entire observations array:
if (this.data.sessionId !== this.sessionId) {
this.data.sessionId = this.sessionId;
this.data.observations = []; // wipes all existing observations
}Then each restored observation is re-created with add(), which generates new metadata:
const observation: Observation = {
id: randomUUID(), // new ID, original lost
timestamp: new Date().toISOString(), // current time, original lost
sessionId: this.sessionId, // random UUID, original lost
agent, type, content, relevance,
};Impact:
- Any observations from the current session that weren't in the snapshot are permanently destroyed.
- Restored observations get fabricated timestamps (all set to the restore time instead of when they originally occurred), breaking any time-based analysis.
- Original observation IDs are replaced with new UUIDs, invalidating any external references.
Prompt for agents
In SessionSnapshotRestoreCommand.execute() (packages/cli/src/commands/session.ts, lines 416-430), instead of using ObservationStore.add() to restore observations (which generates new IDs, timestamps, and sessionIds for each observation), directly write the snapshot's observation data to the observations YAML file. You could either:
1. Add a new method like ObservationStore.replaceAll(observations) that writes the raw observation data without generating new metadata, OR
2. Use ObservationStore.readAll / writeFileSync to directly write the snapshot observations to the file at the known path (.skillkit/memory/observations.yaml), bypassing the add() method entirely.
This preserves the original observation IDs, timestamps, and sessionIds from the snapshot.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
skillkit doctorcommand — runs 10 health checks across environment, configuration, and skills categories with--jsonand--fixflagsskillkit status— shows project overview dashboard (agent, config, version, skills count, recent history) when no active session existsChanges
packages/cli/src/commands/doctor.tsDoctorCommandwith 10 checkspackages/cli/src/commands/status.tspackages/cli/src/commands/index.tsDoctorCommandapps/skillkit/src/cli.tsDoctorCommandTest plan
pnpm buildcompiles cleanlypnpm test— all 25 test suites passskillkit doctor— formatted output with all checksskillkit doctor --json— valid JSONskillkit doctor --fix— creates missing dirs, removes broken symlinksskillkit status— shows overview when no sessionskillkit status --json— includes overview dataSummary by CodeRabbit