Skip to content

feat(skill-memory): per-skill cross-session recall + historian auto-extraction#181

Open
iceteaSA wants to merge 12 commits into
cortexkit:masterfrom
iceteaSA:skill-memory-pr
Open

feat(skill-memory): per-skill cross-session recall + historian auto-extraction#181
iceteaSA wants to merge 12 commits into
cortexkit:masterfrom
iceteaSA:skill-memory-pr

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds skill-memory — per-skill "motor memory" that gives a skill cross-session recall of its own hard-won lessons (gotchas, discoveries, fixes, workflow steps). When a skill's SKILL.md declares skill-memory: { enabled: true }, accumulated notes for that skill surface automatically in a <skill-memory> block appended to the skill tool's result on every load — and, with the historian extension, the historian writes those notes automatically during compaction (no agent action required).

It's fully opt-in per skill and cache-safe by construction: the block rides the tool-result tail (conversation), never the cached system/m[0] prefix, so it can't bust the prompt cache.

How it works

  • Transparent recall — three-hook augmentation: tool.definition advertises an intent param; tool.execute.before stashes the intent (bounded TTL); the after-hook parses the skill's Base directory, reads its SKILL.md frontmatter, and formats the recall block. Lands in the tool RESULT (cache-safe).
  • Write pathsctx_skill_note (agent-authored) and the historian (auto-extracted). Both dedup on a normalized hash.
  • Intent-scoped ranking — a recall cascade: model-matched embeddings → cosine blend over intent_embedding + delta_embedding (relevance/recency/hit weights tunable per skill via ranking_* frontmatter); FTS5 fallback over a content-linked skill_memory_fts vtable; flat recency×hit fallback.
  • Historian auto-extraction — the historian sees TC: skill(<name>) markers in its chunk and emits a <skill_observations> block; both the OpenCode and Pi runners promote those post-commit as global notes (project_identity='*', source_type='historian'), recallable from any project.

Review units (4 commits)

The branch is organized into four coherent, reviewable phases:

  1. P1 — transparent per-skill recall: skill_memory table migration, the three-hook augmentation, flat recall + storage, ctx_skill_note/ctx_skill_recall tools, opt-in distill-skill-memory dreamer task, TUI/ctx-status stats, docs.
  2. P2 — embeddings + intent-scoped recall: delta_embedding + recall_count columns + skill_memory_fts FTS5 vtable; embed-on-write; the cosine/FTS recall cascade; a programmatic (no-LLM) reembed pre-step.
  3. P3a — historian-extraction foundation: the TC: skill(<name>) marker keystone; origin_project + source_type columns; global-tier notes unified under project_identity='*' (collision-merge); partitionKey routing.
  4. P3b — historian auto-extraction pipeline: prompt emits <skill_observations>, parser, validated-result threading, both runners promote via the shared promoteSkillObservations helper, plus an initializeDatabase self-heal net (re-creates skill_memory + ensureColumn so an upgraded DB recovers even if a migration row is lost).

Schema / migrations

Three migrations (numbered after the current master ceiling): skill_memory table, then delta_embedding/recall_count/FTS, then origin_project/source_type/* unification. LATEST_SUPPORTED_VERSION bumped in lockstep (the schema-version-fence test enforces it). Migration bodies are ensureColumn/IF NOT EXISTS idempotent.

Testing

Full plugin + Pi suites pass (one unrelated pre-existing full-suite ordering flake in tui-config.test.ts that passes in isolation), tsc clean both packages, lint clean, build produces all bundles. Dedicated coverage for the migrations (coexistence + fence), recall rungs, the tools, FTS triggers/backfill, the '*' collision-merge, and the historian promotion path on both runners. Verified working live: the historian auto-extraction writes genuine source_type='historian' notes, embeddings populate, and the read-side recall_count increments on surfacing.

Notes for reviewers

  • Migration version numbers are placeholders relative to this fork's base — happy to renumber to whatever slots are free at merge time.
  • The four commits are independently meaningful; if you'd prefer this as stacked PRs (P1 / P2 / historian), I can split it.

View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.


Summary by cubic

Adds per-skill cross-session memory with embeddings + FTS ranking and historian auto-extraction. A <skill-memory> block is appended to each skill tool result; ctx_skill_note writes notes and ctx_skill_recall recalls them on demand.

  • New Features

    • Transparent per-skill recall: the skill tool accepts optional intent; before-hook stashes it and after-hook injects recall; ctx_skill_note writes and ctx_skill_recall reads; Status/TUI show per-project totals and pinned counts.
    • Intent-scoped ranking: cosine blend over intent_embedding + delta_embedding with FTS5 fallback; read-side recall_count; re-embed pre-step backfills NULL/stale vectors before the distill run.
    • Historian auto-extraction: TC: skill(<name>) markers → <skill_observations>; both runners promote via promoteSkillObservations as global '*' notes (source_type='historian').
    • Distill task: opt-in distill-skill-memory dreamer task added to config/CLI/dashboard/schema; runs a programmatic re-embed pre-step and emits a read-only corpus health report; off by default.
    • Schema/self-heal: skill_memory table + FTS vtable/columns with migrations v50–v52 (LATEST_SUPPORTED_VERSION=52); initializeDatabase re-creates missing objects and rebuilds skill_memory_fts; supports opencode’s plain-path skill provenance.
  • Bug Fixes

    • Cross-partition recall: union a skill’s partition with global '*' so project-local skills surface historian-written notes; writes/dedup remain exact-partition.
    • Frontmatter gating + parser: reject ctx_skill_note when skill-memory.enabled is false or missing; parser accepts inline { enabled: true }, is start-of-file anchored, tolerates BOM/whitespace, and strips inline # from unquoted scalars.
    • Provenance + tiering: accept plain filesystem paths and file://; correctly classify singular ~/.config/opencode/skill/** as global; derive tier via dirname(resolvedPath); normalize Windows paths.
    • Guidance + observability: drop ctx_memory cross-reference in skill guidance when memory is off; log recall errors; count per-note XML framing and clamp pinned budget.
    • Disabled-path safety: avoid init crash when the plugin is disabled; escape apostrophes in SQL.

Written for commit 3825d5f. Summary will update on new commits.

Review in cubic

Greptile Summary

Adds skill-memory — per-skill cross-session recall that appends a <skill-memory> block to each skill tool result when a skill opts in via skill-memory: { enabled: true } frontmatter. The feature is fully cache-safe (recall block lands in the tool result tail, never the cached prefix) and includes historian auto-extraction so accumulated lessons are written globally without any agent action.

  • Schema + migrations: Three new migrations (v50–v52) add the skill_memory table, FTS5 vtable with content-linked triggers, delta_embedding/recall_count/origin_project/source_type columns, and a v52 global-tier collision-merge. initializeDatabase gains a self-heal FTS backfill and ensureColumn calls so an upgraded DB recovers even if a migration row is lost.
  • Three-rung recall cascade: cosine blend over intent_embedding + delta_embedding (rung 1, model-matched) → FTS5 MATCH on (intent, delta) (rung 2) → flat recency×hit (rung 3); pinned notes prepended ahead of any ranked result, all subject to a token-budget fill with a clamped pinned sub-budget.
  • Historian auto-extraction: TC: skill(<name>) markers in historian chunks trigger <skill_observations> output, parsed by compartment-parser and promoted as global '*'-partition notes by both the opencode and Pi runners via a shared promoteSkillObservations helper.

Confidence Score: 5/5

Safe to merge — all four issues identified in the previous review round have been addressed with regression tests, and no new defects were found in the core recall, storage, or historian-promotion paths.

The recall cascade, budget-fill logic, dedup paths, FTS triggers, migration idempotency, historian promotion gating, and intent-stash lifecycle were all reviewed in depth. The only findings are a duplicate call to recallPartitionPredicate inside searchSkillMemoryFts (which is pure and functionally harmless) and an API ambiguity in the write-path functions where callers pass a pre-computed partition key to functions that apply partitionKey internally (idempotent for all valid inputs). Both are style-level concerns with no current failure path.

storage.ts has the minor searchSkillMemoryFts double-call and write-function API ambiguity noted in comments; all other files are straightforward.

Important Files Changed

Filename Overview
packages/plugin/src/features/magic-context/skill-memory/storage.ts New file: core storage layer for skill_memory table — insert, dedup, recall (flat/FTS/cosine), bump counters, stats. Logic is correct; minor API ambiguity around whether callers should pass raw projectIdentity or pre-computed partitionKey result (both happen to be idempotent today). searchSkillMemoryFts calls recallPartitionPredicate twice instead of destructuring once.
packages/plugin/src/features/magic-context/skill-memory/recall.ts New file: shared recall core used by both the transparent after-hook path and ctx_skill_recall tool. Recall cascade (cosine → FTS5 → flat recency×hit), budget fill with pinned-note sub-budget clamping, XML escaping, and error logging are all correctly implemented.
packages/plugin/src/features/magic-context/skill-memory/promote.ts New file: historian-extraction promotion — writes global-tier '*'-partition notes with source_type='historian', hash-dedup with hit_count bump, best-effort per-item error handling. Correct.
packages/plugin/src/features/magic-context/skill-memory/provenance.ts New file: parses 'Base directory for this skill:' from tool output (both legacy file:// URL and current plain-path forms), derives tier and skill_source. Session-scoped registry with composite sessionId:skillId keys prevents cross-session bleed. Correct.
packages/plugin/src/features/magic-context/skill-memory/frontmatter.ts New file: custom YAML frontmatter parser for skill-memory: block. Handles inline flow-mapping, block form, inline comments, BOM, CRLF, quoted strings, and comment stripping for unquoted scalars. Start-anchored regex correctly prevents mid-document --- from being misparsed. Correct.
packages/plugin/src/hooks/magic-context/hook-handlers.ts Adds createToolExecuteBeforeHook (intent stash) and extends createToolExecuteAfterHook with skill-load registry population and maybeInjectSkillMemory injection. Intent map keyed sessionID:callID with TTL+cap backstop; single hoisted await import shared by registry-populate and injection blocks. All previous review thread issues addressed.
packages/plugin/src/features/magic-context/migrations.ts Adds migrations v50 (skill_memory table + indexes), v51 (delta_embedding + recall_count + FTS5 vtable/triggers + backfill rebuild), and v52 (origin_project + source_type + global-tier collision-merge). All use ensureColumn/IF NOT EXISTS idempotency. v52 merge logic correctly picks the earliest survivor and aggregates counters.
packages/plugin/src/features/magic-context/storage-db.ts Adds skill_memory table, FTS vtable/triggers, and ensureColumn calls to initializeDatabase. Self-heal FTS backfill is guarded (ftsCount === 0 && rowCount > 0) so it fires only on the gap and degrades gracefully on failure.
packages/plugin/src/tools/ctx-skill-note/tools.ts New write tool: validates kind, checks registry for skill load, guards on frontmatterConfig.enabled before any DB work, exact-hash dedup → semantic cosine dedup → insert, handles concurrent-insert race via NULL return. All previous review issues addressed.
packages/plugin/src/tools/ctx-skill-recall/tools.ts New read tool: registry-first resolution falling back to ordered disk search matching opencode's discoverSkills() order. Guards on frontmatterConfig.enabled before recall. Delegates to shared recallSkillMemoryBlock. Correct.
packages/plugin/src/hooks/magic-context/compartment-parser.ts Adds skill_observations block parsing via SKILL_OBS_ITEM_REGEX. unescapeXml applied to skillId and lesson; SKILL_OBS_KINDS set validates kind; items missing any required field are silently skipped. Correct.
packages/plugin/src/hooks/magic-context/compartment-runner-incremental.ts Promotes historian-extracted skill observations via the same promotionActive && !discardedLast gate used for facts/primers, correctly preventing double-emit on provisional tails.
packages/pi-plugin/src/pi-historian-runner.ts Pi-runner mirror of the opencode historian skill-observation promotion path. Same promotionActive && !discardedLast gate; skillObservations threaded through ValidationOutcome and validateHistorianResult. Correct.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Agent
    participant BeforeHook as tool.execute.before
    participant SkillTool as skill tool
    participant AfterHook as tool.execute.after
    participant Registry as SkillLoadRegistry
    participant Recall as recallSkillMemoryBlock
    participant DB as SQLite (skill_memory)
    participant Historian as Historian runner
    participant Promote as promoteSkillObservations

    Agent->>BeforeHook: "skill(name, intent=...)"
    BeforeHook->>BeforeHook: stashIntent(sessionID:callID, intent)
    BeforeHook->>SkillTool: forward call
    SkillTool-->>AfterHook: output with Base directory line
    AfterHook->>Registry: parseSkillProvenance → set(sessionID:skillId, entry)
    AfterHook->>AfterHook: getAndDeleteIntent(sessionID:callID)
    AfterHook->>Recall: recallSkillMemoryBlock(skill, intent, scope, projectIdentity)
    Recall->>DB: embed intent → cosine rank (rung 1)
    alt embeddings matched
        DB-->>Recall: ranked notes
    else FTS fallback (rung 2)
        DB-->>Recall: FTS MATCH notes
    else flat fallback (rung 3)
        DB-->>Recall: recency x hit notes
    end
    Recall->>DB: bumpRecallCountByIds
    Recall-->>AfterHook: skill-memory block
    AfterHook->>AfterHook: "output.output += block"
    AfterHook-->>Agent: tool result with appended skill-memory

    Agent->>DB: ctx_skill_note → insertSkillMemoryNote or bumpHitCount

    Historian->>Historian: parse skill_observations from chunk
    Historian->>Promote: promoteSkillObservations(db, originProject, obs)
    Promote->>DB: "insertSkillMemoryNote(tier=global, project_identity=*, source_type=historian)"
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Agent
    participant BeforeHook as tool.execute.before
    participant SkillTool as skill tool
    participant AfterHook as tool.execute.after
    participant Registry as SkillLoadRegistry
    participant Recall as recallSkillMemoryBlock
    participant DB as SQLite (skill_memory)
    participant Historian as Historian runner
    participant Promote as promoteSkillObservations

    Agent->>BeforeHook: "skill(name, intent=...)"
    BeforeHook->>BeforeHook: stashIntent(sessionID:callID, intent)
    BeforeHook->>SkillTool: forward call
    SkillTool-->>AfterHook: output with Base directory line
    AfterHook->>Registry: parseSkillProvenance → set(sessionID:skillId, entry)
    AfterHook->>AfterHook: getAndDeleteIntent(sessionID:callID)
    AfterHook->>Recall: recallSkillMemoryBlock(skill, intent, scope, projectIdentity)
    Recall->>DB: embed intent → cosine rank (rung 1)
    alt embeddings matched
        DB-->>Recall: ranked notes
    else FTS fallback (rung 2)
        DB-->>Recall: FTS MATCH notes
    else flat fallback (rung 3)
        DB-->>Recall: recency x hit notes
    end
    Recall->>DB: bumpRecallCountByIds
    Recall-->>AfterHook: skill-memory block
    AfterHook->>AfterHook: "output.output += block"
    AfterHook-->>Agent: tool result with appended skill-memory

    Agent->>DB: ctx_skill_note → insertSkillMemoryNote or bumpHitCount

    Historian->>Historian: parse skill_observations from chunk
    Historian->>Promote: promoteSkillObservations(db, originProject, obs)
    Promote->>DB: "insertSkillMemoryNote(tier=global, project_identity=*, source_type=historian)"
Loading

Comments Outside Diff (1)

  1. packages/plugin/src/features/magic-context/skill-memory/recall.ts, line 481-484 (link)

    P2 Bare catch {} makes recall errors invisible

    Any exception thrown inside recallSkillMemoryBlock — a SQLite error, embedding provider crash, or a coding mistake — is silently swallowed and returns an empty string. From the caller's perspective this is indistinguishable from "no notes exist" or "skill-memory disabled". At minimum a sessionLog or console.warn call here would let a developer diagnose why the recall block is absent without attaching a debugger.

Reviews (10): Last reviewed commit: "fix(skill-memory): accept plain filesyst..." | Re-trigger Greptile

@iceteaSA iceteaSA force-pushed the skill-memory-pr branch 4 times, most recently from dc83db6 to 4034019 Compare June 25, 2026 11:37
@iceteaSA iceteaSA marked this pull request as ready for review June 25, 2026 13:18

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

11 issues found across 78 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/plugin/src/features/magic-context/dreamer/task-executor.ts">

<violation number="1" location="packages/plugin/src/features/magic-context/dreamer/task-executor.ts:419">
P2: Re-embed pre-step errors are suppressed, allowing successful task completion reporting despite a failed prerequisite data maintenance step.</violation>
</file>

Note: This PR contains a large number of files. cubic only reviews up to 40 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.

Re-trigger cubic

Comment thread CONFIGURATION.md Outdated
Comment thread packages/plugin/src/features/magic-context/skill-memory/promote.ts Outdated
Comment thread packages/plugin/src/index.ts Outdated
Comment thread packages/plugin/src/tools/ctx-skill-recall/types.ts
Comment thread packages/plugin/src/features/magic-context/dreamer/task-executor.ts
Comment thread packages/plugin/src/hooks/magic-context/read-session-formatting.ts Outdated
Comment thread packages/plugin/src/hooks/magic-context/read-session-formatting.ts
Comment thread packages/plugin/src/features/magic-context/skill-memory/frontmatter.ts Outdated
Comment thread packages/plugin/src/features/magic-context/skill-memory/frontmatter.ts Outdated
Comment thread packages/plugin/src/hooks/magic-context/hook.ts Outdated
Comment thread packages/plugin/src/hooks/magic-context/hook-handlers.ts
Comment thread packages/plugin/src/features/magic-context/storage-db.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 issues found across 15 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/plugin/src/features/magic-context/dreamer/task-executor.ts">

<violation number="1" location="packages/plugin/src/features/magic-context/dreamer/task-executor.ts:419">
P2: Re-embed pre-step errors are suppressed, allowing successful task completion reporting despite a failed prerequisite data maintenance step.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/plugin/src/features/magic-context/dreamer/task-prompts.ts Outdated
Comment thread packages/plugin/src/features/magic-context/skill-memory/frontmatter.ts Outdated
Comment thread packages/plugin/src/hooks/magic-context/read-session-formatting.ts Outdated
Comment thread packages/plugin/src/features/magic-context/skill-memory/frontmatter.test.ts Outdated
Comment thread packages/plugin/src/features/magic-context/skill-memory/promote.ts
@iceteaSA iceteaSA marked this pull request as draft June 25, 2026 14:55
iceteaSA pushed a commit to iceteaSA/magic-context that referenced this pull request Jun 25, 2026
…routing/parser gaps)

Council review (deepseek/sonnet/gpt-5.5) of PR cortexkit#181:

- Must (consensus rev-2+rev-3): the ctx_skill_note fail-loud guard threw during
  plugin init when the plugin is disabled (enabled:false OR conflict-disabled) —
  createSessionHooks returns {magicContext:null} by design, so the unconditional
  guard crashed the entry module on the disabled path. Gate it on
  pluginConfig.enabled; pass a throwaway Map to createToolRegistry (which
  early-returns {} when disabled and never reads it).
- Must (rev-3): singular ~/.config/opencode/skill/ global path was misclassified
  as project tier (opencode's pattern is {skill,skills}/**/SKILL.md) — fixed in
  deriveSkillTier/deriveSkillSource + the ctx_skill_recall cold-start search list.
- Must (rev-3): the frontmatter parser rejected the inline flow-mapping form
  'skill-memory: { enabled: true }' — the EXACT form the ctx_skill_recall
  remediation message and ARCHITECTURE/CONFIGURATION/README advertise. Added
  inline-mapping parsing so guidance and parser agree.
- Should (consensus rev-1+rev-3): recallSkillMemoryBlock swallowed all errors
  silently — added a log() so FTS/blob corruption is diagnosable (still no-throw).

Regression tests: inline frontmatter form (3 cases), singular skill/ global path
(2 cases).
iceteaSA pushed a commit to iceteaSA/magic-context that referenced this pull request Jun 25, 2026
…routing/parser gaps)

Council review (deepseek/sonnet/gpt-5.5) of PR cortexkit#181:

- Must (consensus rev-2+rev-3): the ctx_skill_note fail-loud guard threw during
  plugin init when the plugin is disabled (enabled:false OR conflict-disabled) —
  createSessionHooks returns {magicContext:null} by design, so the unconditional
  guard crashed the entry module on the disabled path. Gate it on
  pluginConfig.enabled; pass a throwaway Map to createToolRegistry (which
  early-returns {} when disabled and never reads it).
- Must (rev-3): singular ~/.config/opencode/skill/ global path was misclassified
  as project tier (opencode's pattern is {skill,skills}/**/SKILL.md) — fixed in
  deriveSkillTier/deriveSkillSource + the ctx_skill_recall cold-start search list.
- Must (rev-3): the frontmatter parser rejected the inline flow-mapping form
  'skill-memory: { enabled: true }' — the EXACT form the ctx_skill_recall
  remediation message and ARCHITECTURE/CONFIGURATION/README advertise. Added
  inline-mapping parsing so guidance and parser agree.
- Should (consensus rev-1+rev-3): recallSkillMemoryBlock swallowed all errors
  silently — added a log() so FTS/blob corruption is diagnosable (still no-throw).

Regression tests: inline frontmatter form (3 cases), singular skill/ global path
(2 cases).
iceteaSA pushed a commit to iceteaSA/magic-context that referenced this pull request Jun 25, 2026
…routing/parser gaps)

Council review (deepseek/sonnet/gpt-5.5) of PR cortexkit#181:

- Must (consensus rev-2+rev-3): the ctx_skill_note fail-loud guard threw during
  plugin init when the plugin is disabled (enabled:false OR conflict-disabled) —
  createSessionHooks returns {magicContext:null} by design, so the unconditional
  guard crashed the entry module on the disabled path. Gate it on
  pluginConfig.enabled; pass a throwaway Map to createToolRegistry (which
  early-returns {} when disabled and never reads it).
- Must (rev-3): singular ~/.config/opencode/skill/ global path was misclassified
  as project tier (opencode's pattern is {skill,skills}/**/SKILL.md) — fixed in
  deriveSkillTier/deriveSkillSource + the ctx_skill_recall cold-start search list.
- Must (rev-3): the frontmatter parser rejected the inline flow-mapping form
  'skill-memory: { enabled: true }' — the EXACT form the ctx_skill_recall
  remediation message and ARCHITECTURE/CONFIGURATION/README advertise. Added
  inline-mapping parsing so guidance and parser agree.
- Should (consensus rev-1+rev-3): recallSkillMemoryBlock swallowed all errors
  silently — added a log() so FTS/blob corruption is diagnosable (still no-throw).

Regression tests: inline frontmatter form (3 cases), singular skill/ global path
(2 cases).
@iceteaSA iceteaSA marked this pull request as ready for review June 25, 2026 16:09

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 issues found across 78 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/plugin/src/features/magic-context/dreamer/task-executor.ts">

<violation number="1" location="packages/plugin/src/features/magic-context/dreamer/task-executor.ts:419">
P2: Re-embed pre-step errors are suppressed, allowing successful task completion reporting despite a failed prerequisite data maintenance step.</violation>
</file>

Note: This PR contains a large number of files. cubic only reviews up to 40 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.

Re-trigger cubic

Comment thread ARCHITECTURE.md Outdated
Comment thread ARCHITECTURE.md
Comment thread packages/plugin/src/hooks/magic-context/read-session-formatting.ts
Comment thread packages/plugin/src/features/magic-context/skill-memory/provenance.ts Outdated
iceteaSA pushed a commit to iceteaSA/magic-context that referenced this pull request Jun 25, 2026
…routing/parser gaps)

Council review (deepseek/sonnet/gpt-5.5) of PR cortexkit#181:

- Must (consensus rev-2+rev-3): the ctx_skill_note fail-loud guard threw during
  plugin init when the plugin is disabled (enabled:false OR conflict-disabled) —
  createSessionHooks returns {magicContext:null} by design, so the unconditional
  guard crashed the entry module on the disabled path. Gate it on
  pluginConfig.enabled; pass a throwaway Map to createToolRegistry (which
  early-returns {} when disabled and never reads it).
- Must (rev-3): singular ~/.config/opencode/skill/ global path was misclassified
  as project tier (opencode's pattern is {skill,skills}/**/SKILL.md) — fixed in
  deriveSkillTier/deriveSkillSource + the ctx_skill_recall cold-start search list.
- Must (rev-3): the frontmatter parser rejected the inline flow-mapping form
  'skill-memory: { enabled: true }' — the EXACT form the ctx_skill_recall
  remediation message and ARCHITECTURE/CONFIGURATION/README advertise. Added
  inline-mapping parsing so guidance and parser agree.
- Should (consensus rev-1+rev-3): recallSkillMemoryBlock swallowed all errors
  silently — added a log() so FTS/blob corruption is diagnosable (still no-throw).

Regression tests: inline frontmatter form (3 cases), singular skill/ global path
(2 cases).
iceteaSA pushed a commit to iceteaSA/magic-context that referenced this pull request Jun 26, 2026
…routing/parser gaps)

Council review (deepseek/sonnet/gpt-5.5) of PR cortexkit#181:

- Must (consensus rev-2+rev-3): the ctx_skill_note fail-loud guard threw during
  plugin init when the plugin is disabled (enabled:false OR conflict-disabled) —
  createSessionHooks returns {magicContext:null} by design, so the unconditional
  guard crashed the entry module on the disabled path. Gate it on
  pluginConfig.enabled; pass a throwaway Map to createToolRegistry (which
  early-returns {} when disabled and never reads it).
- Must (rev-3): singular ~/.config/opencode/skill/ global path was misclassified
  as project tier (opencode's pattern is {skill,skills}/**/SKILL.md) — fixed in
  deriveSkillTier/deriveSkillSource + the ctx_skill_recall cold-start search list.
- Must (rev-3): the frontmatter parser rejected the inline flow-mapping form
  'skill-memory: { enabled: true }' — the EXACT form the ctx_skill_recall
  remediation message and ARCHITECTURE/CONFIGURATION/README advertise. Added
  inline-mapping parsing so guidance and parser agree.
- Should (consensus rev-1+rev-3): recallSkillMemoryBlock swallowed all errors
  silently — added a log() so FTS/blob corruption is diagnosable (still no-throw).

Regression tests: inline frontmatter form (3 cases), singular skill/ global path
(2 cases).
@iceteaSA iceteaSA force-pushed the skill-memory-pr branch 2 times, most recently from 9c62039 to 3e28417 Compare July 2, 2026 15:22
iceteaSA pushed a commit to iceteaSA/magic-context that referenced this pull request Jul 2, 2026
…routing/parser gaps)

Council review (deepseek/sonnet/gpt-5.5) of PR cortexkit#181:

- Must (consensus rev-2+rev-3): the ctx_skill_note fail-loud guard threw during
  plugin init when the plugin is disabled (enabled:false OR conflict-disabled) —
  createSessionHooks returns {magicContext:null} by design, so the unconditional
  guard crashed the entry module on the disabled path. Gate it on
  pluginConfig.enabled; pass a throwaway Map to createToolRegistry (which
  early-returns {} when disabled and never reads it).
- Must (rev-3): singular ~/.config/opencode/skill/ global path was misclassified
  as project tier (opencode's pattern is {skill,skills}/**/SKILL.md) — fixed in
  deriveSkillTier/deriveSkillSource + the ctx_skill_recall cold-start search list.
- Must (rev-3): the frontmatter parser rejected the inline flow-mapping form
  'skill-memory: { enabled: true }' — the EXACT form the ctx_skill_recall
  remediation message and ARCHITECTURE/CONFIGURATION/README advertise. Added
  inline-mapping parsing so guidance and parser agree.
- Should (consensus rev-1+rev-3): recallSkillMemoryBlock swallowed all errors
  silently — added a log() so FTS/blob corruption is diagnosable (still no-throw).

Regression tests: inline frontmatter form (3 cases), singular skill/ global path
(2 cases).
Comment thread packages/plugin/src/tools/ctx-skill-note/tools.ts
iceteaSA pushed a commit to iceteaSA/magic-context that referenced this pull request Jul 2, 2026
…d in frontmatter

Counterpart to ctx_skill_recall's enabled-guard (greptile review, PR cortexkit#181):
without it, notes for skills that never opted in inserted successfully but
were permanently orphaned — recallSkillMemoryBlock returns "" when
frontmatter is disabled, while the agent saw a convincing 'Skill note
saved' response. Now returns an actionable error before any insert.

Red-checked: new regression test fails without the guard (orphan row
inserted + 'saved' response), passes with it (no row, 'not enabled').
iceteaSA pushed a commit to iceteaSA/magic-context that referenced this pull request Jul 2, 2026
…d in frontmatter

Counterpart to ctx_skill_recall's enabled-guard (greptile review, PR cortexkit#181):
without it, notes for skills that never opted in inserted successfully but
were permanently orphaned — recallSkillMemoryBlock returns "" when
frontmatter is disabled, while the agent saw a convincing 'Skill note
saved' response. Now returns an actionable error before any insert.

Red-checked: new regression test fails without the guard (orphan row
inserted + 'saved' response), passes with it (no row, 'not enabled').

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/plugin/src/features/magic-context/skill-memory/provenance.test.ts">

<violation number="1" location="packages/plugin/src/features/magic-context/skill-memory/provenance.test.ts:107">
P2: The test "parses a PLAIN filesystem path for a PROJECT skill" verifies that the function returns the correct `tier` and `skillSource`, but it doesn't verify the `resolvedPath` output. Since this is the only test exercising plain-path resolution for a project-level skill, a regression in the path-joining logic — like a missing `/SKILL.md` suffix, incorrect directory concatenation, or broken normalization — would go undetected. The adjacent test for global plain paths (and six other tests covering file:// paths) all assert `resolvedPath`. Consider adding an assertion like `expect(result!.resolvedPath).toBe("${HOME}/projects/foo/.opencode/skills/my-skill/SKILL.md")` to match the coverage pattern of the rest of the test suite.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

test("parses a PLAIN filesystem path for a PROJECT skill", () => {
const output = `Base directory for this skill: ${HOME}/projects/foo/.opencode/skills/my-skill`;
const result = parseSkillProvenance(output, "my-skill");
expect(result!.tier).toBe("project");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The test "parses a PLAIN filesystem path for a PROJECT skill" verifies that the function returns the correct tier and skillSource, but it doesn't verify the resolvedPath output. Since this is the only test exercising plain-path resolution for a project-level skill, a regression in the path-joining logic — like a missing /SKILL.md suffix, incorrect directory concatenation, or broken normalization — would go undetected. The adjacent test for global plain paths (and six other tests covering file:// paths) all assert resolvedPath. Consider adding an assertion like expect(result!.resolvedPath).toBe("${HOME}/projects/foo/.opencode/skills/my-skill/SKILL.md") to match the coverage pattern of the rest of the test suite.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/skill-memory/provenance.test.ts, line 107:

<comment>The test "parses a PLAIN filesystem path for a PROJECT skill" verifies that the function returns the correct `tier` and `skillSource`, but it doesn't verify the `resolvedPath` output. Since this is the only test exercising plain-path resolution for a project-level skill, a regression in the path-joining logic — like a missing `/SKILL.md` suffix, incorrect directory concatenation, or broken normalization — would go undetected. The adjacent test for global plain paths (and six other tests covering file:// paths) all assert `resolvedPath`. Consider adding an assertion like `expect(result!.resolvedPath).toBe("${HOME}/projects/foo/.opencode/skills/my-skill/SKILL.md")` to match the coverage pattern of the rest of the test suite.</comment>

<file context>
@@ -91,4 +91,36 @@ describe("parseSkillProvenance", () => {
+    test("parses a PLAIN filesystem path for a PROJECT skill", () => {
+        const output = `Base directory for this skill: ${HOME}/projects/foo/.opencode/skills/my-skill`;
+        const result = parseSkillProvenance(output, "my-skill");
+        expect(result!.tier).toBe("project");
+        expect(result!.skillSource).toBe("opencode-project");
+    });
</file context>

Tehan added 3 commits July 6, 2026 16:24
Per-skill "motor memory": when a skill's SKILL.md declares
`skill-memory: { enabled: true }`, accumulated gotchas/discoveries/fixes/
workflow-steps surface in a <skill-memory> block appended to the skill
tool's RESULT on every load (cache-safe — rides the tool-result tail).
Agents write back via ctx_skill_note; ctx_skill_recall is the explicit
companion to the transparent after-hook.

- migration: skill_memory table (per-skill; tier project/global; UNIQUE on
  skill_id/tier/project_identity/normalized_hash) + lookup indexes.
- three-hook augmentation: tool.definition advertises an `intent` param;
  tool.execute.before stashes intent (bounded TTL); after-hook parses the
  skill's Base directory, reads SKILL.md frontmatter, formats the block.
- flat recency×hit recall + storage layer; ctx_skill_note / ctx_skill_recall.
- opt-in distill-skill-memory dreamer task; agent-prompt guidance; TUI/ctx-status stats.
- docs: ARCHITECTURE / STRUCTURE / CONFIGURATION / README.
Upgrade recall from flat recency×hit to a multi-rung cascade: intent +
model-matched embeddings → cosine blend across intent_embedding +
delta_embedding (relevance/recency/hit weights tunable per skill via
ranking_* frontmatter); intent + no model match → FTS5 fallback over the
content-linked skill_memory_fts vtable; empty → flat fallback.

- migration: delta_embedding + recall_count columns + skill_memory_fts FTS5 vtable.
- embed-on-write in insertSkillMemoryNote; delta-only semantic dedup.
- programmatic, no-LLM reembed pre-step for the distill-skill-memory dreamer task.
- read-side recall_count (distinct from write-side hit_count).
- canonical vector serde + dedup/ranking/FTS query helpers.
…ication (P3a)

Foundation for the historian to auto-capture skill notes cross-project.

- surface the skill name in the historian chunk as a `TC: skill(<name>)`
  marker (the keystone — the tool input name was previously dropped).
- migration: origin_project + source_type columns; unify global-tier notes
  under project_identity='*' (collision-merge) so a global note is one row
  recallable from any repo.
- partitionKey helper routes global write/recall/reembed/stats through '*';
  recall reads global-tier from '*' (cross-project); reembed sweeps '*'.
Tehan added 9 commits July 6, 2026 16:27
Close the loop so the historian writes skill notes during compaction
without an agent volunteering ctx_skill_note.

- historian prompt emits a <skill_observations> block; parser extracts it;
  threaded through the validated historian result.
- both runners (OpenCode + Pi) promote skill observations post-commit via
  the shared promoteSkillObservations helper, gated by
  promotionActive && !discardedLast, writing global '*' notes with
  source_type='historian'.
- self-heal net: initializeDatabase re-creates skill_memory + ensureColumn
  so an upgraded DB recovers even if a migration row is lost.
- Remove committed <<<<<<< HEAD conflict marker in CONFIGURATION.md (P1)
- Move injectSkillIntentParam before the lastChatContext guard so the intent
  param is advertised even on tool.definition flights before first chat.message
- Key intentByCallId by sessionID:callID + prefix-prune on session delete so a
  concurrent session's delete can't evict another session's in-flight intents
- Log silent catch in promoteSkillObservations (observability for dropped writes)
- Anchor frontmatter regex to start-of-file (drop m flag) so a later --- rule
  can't be misparsed; strip inline # comments from unquoted YAML scalars + block header
- Scope distill report SQL to ('<identity>','*') instead of non-deterministic LIMIT 1
- Don't truncate skill name in TC: skill(<name>) marker (identity key); sanitize
  newlines/control chars
- Normalize backslash->slash after fileURLToPath for Windows provenance checks
- FTS self-heal rebuild in initializeDatabase when skill_memory_fts is empty but
  skill_memory has rows
- Move ctx_skill_recall _test* DI fields to a separate test-only deps type
- Hoist the shared registryKey dynamic import (one import, both blocks)

Pushback: reembed pre-step errors are already logged (task-executor.ts) — the
non-blocking try/catch is by design (failure leaves notes on the FTS rung).
…2/P3)

- P1: recall now unions the skill's own partition with the global '*' partition
  (recallPartitionPredicate helper) so a PROJECT-LOCAL skill surfaces
  historian-written global notes — previously orphaned (tier='project' query
  never matched tier='global'/'*'). Write/dedup paths stay exact-partition.
- Escape apostrophes in projectPath before SQL string interpolation in the
  distill prompt template.
- Frontmatter regex tolerates a leading UTF-8 BOM / whitespace (still start-anchored).
- TC: skill(<name>) marker emits the name VERBATIM when marker-safe, else drops
  it — never mutates the identity key (recall keys on raw input.name).
- Fix misleading frontmatter test: now actually exercises a '#' inside a quoted
  scalar (preserved) vs unquoted (comment-stripped).

Regression tests: project-local skill recalls a global historian note; agent
project note + historian global note both surface for the same skill.
…routing/parser gaps)

Council review (deepseek/sonnet/gpt-5.5) of PR cortexkit#181:

- Must (consensus rev-2+rev-3): the ctx_skill_note fail-loud guard threw during
  plugin init when the plugin is disabled (enabled:false OR conflict-disabled) —
  createSessionHooks returns {magicContext:null} by design, so the unconditional
  guard crashed the entry module on the disabled path. Gate it on
  pluginConfig.enabled; pass a throwaway Map to createToolRegistry (which
  early-returns {} when disabled and never reads it).
- Must (rev-3): singular ~/.config/opencode/skill/ global path was misclassified
  as project tier (opencode's pattern is {skill,skills}/**/SKILL.md) — fixed in
  deriveSkillTier/deriveSkillSource + the ctx_skill_recall cold-start search list.
- Must (rev-3): the frontmatter parser rejected the inline flow-mapping form
  'skill-memory: { enabled: true }' — the EXACT form the ctx_skill_recall
  remediation message and ARCHITECTURE/CONFIGURATION/README advertise. Added
  inline-mapping parsing so guidance and parser agree.
- Should (consensus rev-1+rev-3): recallSkillMemoryBlock swallowed all errors
  silently — added a log() so FTS/blob corruption is diagnosable (still no-throw).

Regression tests: inline frontmatter form (3 cases), singular skill/ global path
(2 cases).
- budgetFill now counts per-note XML framing (~20 tokens) so the rendered
  <skill-memory> block stays within max_tokens instead of ~13% overshoot (rev-1).
- clamp effective pinned budget to min(max_pinned_tokens, max_tokens) so the
  default 4000>1500 can't imply pinned gets more room than the whole block (rev-2).
- ctx_skill_recall: derive tier via dirname(resolvedPath) instead of a fragile
  .replace('/SKILL.md','') (rev-2).
Updated the budget-truncation test for the framing-inclusive math.
- provenance.ts: anchor the Base-directory regex to line-start (^…/gm) and take
  the LAST match — opencode appends the provenance line at the END of tool
  output, so a skill whose CONTENT echoes 'Base directory for this skill:' (e.g.
  a skill documenting skill-memory) would otherwise shadow the real line and
  misdirect recall to a bogus identity.
- read-session-formatting.ts: narrow the marker-safe exclusion to CR/LF/tab only
  — a ')' does not break the single-line TC: skill(<name>) marker and the
  historian reads it as natural language, so a ')'-containing name is preserved
  verbatim (identity key) instead of dropped.
- ARCHITECTURE.md: update the 'Skill-memory (motor memory)' Key Abstraction to
  the shipped reality (v50/51/52, multi-rung embedding+FTS recall, global-'*'
  union) — was stale (v37, 'P2 TODO'). Remove the PR-added duplicate
  'Tag Identity (v3.3.1+)' section (upstream owns the lean '## Tag identity';
  Tag Identity is unrelated to skill-memory — rebase scope-creep).

Regression tests: provenance last-match + mid-line rejection; ')' name preserved
+ CR/LF/tab still dropped.
…emory off

Rebase-onto-v0.29.0 resolution completion. Upstream ab4f01c added a
memory.enabled gate that drops ALL ctx_memory mentions from the system
prompt when memory is off (ctx_memory is then unregistered). The skill-memory
guidance carried a 'those belong in ctx_memory' cross-reference that violated
the new contract (buildMagicContextSection memory-gating tests). Parameterized
ctxSkillMemoryGuidance(memoryEnabled) so the cross-ref drops when memory is off;
skill-memory itself stays ungated (independent store).
…d in frontmatter

Counterpart to ctx_skill_recall's enabled-guard (greptile review, PR cortexkit#181):
without it, notes for skills that never opted in inserted successfully but
were permanently orphaned — recallSkillMemoryBlock returns "" when
frontmatter is disabled, while the agent saw a convincing 'Skill note
saved' response. Now returns an actionable error before any insert.

Red-checked: new regression test fails without the guard (orphan row
inserted + 'saved' response), passes with it (no row, 'not enabled').
…opencode #33580)

opencode's skill tool changed the 'Base directory for this skill:' line from a
file:// URL to a plain filesystem path (upstream #33580). Our parser hard-required
file:/// so parseSkillProvenance returned null on current opencode -> skill-load
registry never populated -> every agent ctx_skill_note failed with a provenance
parse error. Agent-written notes silently stopped 2026-07-02 (only historian-path
notes, which bypass this parser, continued).

Widen BASE_DIR_REGEX to capture the rest of the line and branch on the value:
file:// -> fileURLToPath (legacy/back-compat); otherwise treat as a plain path.
Keeps the line-anchor + last-match decoy-rejection invariant. +4 regression tests
(plain global, plain project, plain decoy last-match, plain mid-line-ignore); all
existing file:// tests unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant