88 lines
24 KiB
Markdown
88 lines
24 KiB
Markdown
## Context
|
|
|
|
Two capabilities already exist as pending, unapplied changes that this milestone sits between: `task-memory` (`apex-agent-mvp`) persists a per-task `Timeline` — `storage/timeline.py`'s `Timeline.append()` writes one `TimelineRecord{index, scene, prompt, tool_call, result, timestamp, screenshot_path}` per executed step, retrievable in order via `Timeline.read(task_id)` — and `skill-catalog-subscription` defines a `Skill` data model (`kind = knowledge | flow_template`, shared metadata `id/name/description/version/tags`, plus for `flow_template` an ordered `steps` list of tool-name + args-template and a `parameters` schema) synced *from* an external Subscription Platform into a local read-mostly catalog (`skill-catalog-subscription`'s own spec explicitly rejects local writes outside of sync: "a caller attempts to create or edit a skill directly in the local catalog... reject or ignore"). Neither change gives the agent a way to go from "I just executed a successful sequence of tool calls" to "that sequence is now a reusable skill" — `task-memory`'s `Timeline` is write-once/read-only historical record, and `skill-catalog`'s local store is deliberately sync-only, not authorable.
|
|
|
|
This creates a real tension this design must resolve explicitly: `skill-catalog-subscription`'s spec says the local catalog rejects non-sync writes, but this change's whole point is to write locally-synthesized skills into a catalog. The resolution (D6 below) is that locally-authored skills live in a **separate local store** (`skills_learning/`'s own catalog table) that composes with, rather than writes into, the `skill-catalog-subscription` store — the two are presented to a Planner/MCP-tool layer as one logical namespace in prose, but this change does not touch `skill-catalog-subscription`'s schema, sync semantics, or its "sync-only" write rule.
|
|
|
|
`runtime/task.py`'s `TaskRunner.run()` (code-complete, unapplied) is the Observe→Think→Act→Observe loop; it already knows the task's `goal`, calls `Timeline.append()` once per step (via whatever wiring `apex-agent-mvp` gives it — the loop has access to everything `Timeline` needs), and terminates with a final status. This change adds exactly one new integration point at the very end of that loop: a hook fired once, only on success.
|
|
|
|
Two stakeholders: `runtime/task.py`'s `TaskRunner` (must gain a well-defined, optional, no-op-by-default hook), and a future LLM-driven Planner (not built in this change, matching `world-model-runtime`'s and `semantic-scene-runtime`'s precedent of "expose the input, defer the smart consumer") that will eventually call `retrieve_candidate_skills()` before planning from scratch.
|
|
|
|
## Goals / Non-Goals
|
|
|
|
**Goals:**
|
|
- Synthesize a parameterized `FlowTemplateSkill` from a completed task's `Timeline` + goal, automatically, with no manual authoring step.
|
|
- Detect when a later execution that would synthesize into "the same" skill (same name/goal-family) has actually diverged in its step sequence, and version rather than silently overwrite.
|
|
- Make locally-learned skills discoverable by semantic similarity to a new goal, not just exact name/id lookup, so a Planner can find "something like this" even when the new goal's wording differs from the one that produced the stored skill.
|
|
- Compose cleanly with `skill-catalog-subscription`'s existing model and its "sync-only local writes" rule — do not require a delta against that change's spec.
|
|
- Keep the synthesis/versioning/embedding path entirely off the hot Observe→Act loop — it runs once, after a task finishes, never mid-task.
|
|
- Default to disabled, so applying this change does not silently add CPU/LLM cost to every successful task run.
|
|
|
|
**Non-Goals:**
|
|
- No change to `skill-catalog-subscription`'s sync contract, MCP tool surface (`list_skills`/`search_skills`/`get_skill`), or subscription/visibility/entitlement model.
|
|
- No Planner wiring that actually *consumes* `retrieve_candidate_skills()` results to skip planning — this change only makes retrieval available, matching `world-model-runtime`'s precedent of exposing a read-only input without teaching a Planner to use it.
|
|
- No skill *execution* engine (running a `FlowTemplateSkill`'s steps against `tools/` with resolved parameters) — `skill-catalog-subscription`'s proposal already scoped a "run_skill_flow parameter-resolution helper" as its own concern; this change produces skills, it does not add a new runner for them.
|
|
- No cross-device/cross-tenant skill sharing beyond whatever `skill-catalog`'s existing storage model already implies.
|
|
- No UI (`web-console`'s domain, not touched here).
|
|
- No multi-provider embedding abstraction — a single concrete embedding client is enough for this milestone, matching `semantic-scene-runtime`'s D-non-goal of not building a multi-provider LLM abstraction prematurely (YAGNI).
|
|
- Failed or partially-completed tasks are never synthesized into skills — only `status = succeeded` timelines are eligible input.
|
|
|
|
## Decisions
|
|
|
|
### D1: New `skills_learning/` package, sibling to `semantic/` and `world/`, not folded into `runtime/` or `skills/`
|
|
`skills_learning/` (`synthesis.py`, `versioning.py`, `embeddings.py`, `retrieval.py`, `config.py`) is its own package rather than extending `skill-catalog-subscription`'s planned `skills/` package (`skills/models.py`, `skills/catalog.py`, `skills/sync_client.py`, `skills/mcp_tools.py`) in place. Learning-from-execution has fundamentally different runtime characteristics from that package's sync-client role: it reads `task-memory`'s `Timeline`, it makes an embedding call, and it produces new skill records rather than pulling existing ones from an external platform. Keeping it separate means `skill-catalog-subscription`, when eventually implemented, does not need to anticipate a local-authoring code path it explicitly designed against (its own spec's "local catalog is not independently authored" scenario).
|
|
**Alternative considered**: add authoring/versioning/embedding logic directly inside `skills/catalog.py` behind a feature flag. Rejected — it would make the sync-client package responsible for a write path its own spec explicitly forbids for the *synced* store, forcing every future reader of `skills/catalog.py` to reason about two different write-authority rules (sync-only vs. locally-authored) inside one module; a separate package with its own store keeps each write-authority rule enforceable at the package boundary, mirroring `semantic-scene-runtime`'s D1 and `world-model-runtime`'s D1 precedent of "new sibling package over folding into the layer below."
|
|
|
|
### D2: Synthesis triggered by an optional `TaskRunner.on_task_succeeded` hook, not a background scanner over `task-memory`
|
|
`runtime/task.py`'s `TaskRunner.run()` gains an optional constructor argument `on_task_succeeded: Callable[[str, str, Timeline], None] | None = None` (task_id, goal, timeline), invoked exactly once, immediately after the loop determines final `status = succeeded`, before `run()` returns. When Skill Authoring is enabled in `skills_learning/config.py`, `TaskRunner` wires this to `synthesis.synthesize_flow_skill`; when disabled or left `None`, behavior is identical to today.
|
|
**Alternative considered**: a separate offline batch job that periodically scans `storage/task_metadata.py`'s SQLite table for newly-succeeded tasks and synthesizes skills asynchronously. Rejected for this milestone — a batch job adds a second process/scheduling concern (when does it run, how does it avoid re-processing the same task twice) for a benefit (decoupling from the task loop's latency) that doesn't matter here, since synthesis is deliberately post-completion, not mid-loop; an in-process hook fired once at completion is simpler and has no scheduling state to get wrong. A batch/backfill mode remains a reasonable follow-up once there's a concrete need to synthesize skills from tasks that ran before this change existed.
|
|
|
|
### D3: Parameter abstraction via cross-goal diffing against the same tool-call skeleton, not NLP-based slot extraction from the goal string
|
|
`synthesize_flow_skill(goal, timeline)` first extracts the ordered `(tool_name, args)` sequence from `timeline.read(task_id)`'s `tool_call` fields (dropping non-mutating/read-only calls like `describe_screen`/`screenshot`/`ui_tree` from the *template*, since those are re-derivable at replay time, while keeping `tap`/`swipe`/`input_text`/`launch_app`/similar mutating calls). It then looks up any already-stored skill whose stored steps have the same tool-name sequence (same skeleton, e.g. `launch_app → tap → input_text → tap`); if one exists, it diffs argument values position-by-position across the two executions and promotes any value that differs between the stored version and this run into a named `{param}` placeholder (e.g. a `input_text` call's literal text becomes `{search_query}`), inferring the parameter's `name` from the corresponding UI element's label/purpose when available (reusing `semantic-scene-runtime`'s `SemanticScene.widgets[].purpose` label as the naming source, when present, else falling back to a positional name like `param_2`). If no matching skeleton exists yet, the very first execution is stored as-is with zero parameters (nothing to diff against) and only gains parameters once a second, divergent-in-values execution is synthesized.
|
|
**Alternative considered**: parse the natural-language `goal` string itself (e.g. via NER/pattern matching) to identify which words are "the variable part" (a search term, a contact name) and map those directly to template parameters. Rejected — goal phrasing is free-form and unconstrained (matching `semantic-scene-runtime`'s own open-vocabulary stance on intents), so goal-string parsing is fragile and would require its own NLP pipeline; diffing the *executed steps'* concrete argument values across two runs of "the same" flow is grounded in what the agent actually did, not what it said it wanted, and needs no parameter-worthy-token classifier of its own.
|
|
|
|
### D4: Versioning triggers on tool-name-sequence divergence beyond a configured tolerance, tracked as an explicit version chain
|
|
`versioning.py`'s `diff_flow_versions(stored_steps, executed_steps) -> VersionDiff` computes: (a) whether the tool-name sequence itself differs (insertion/deletion/reorder of a step — always triggers a new version, no tolerance), and (b) for a matching skeleton, what fraction of argument positions newly qualify as "must become a parameter" per D3 (a configurable tolerance, default: any position that has ever varied is fine to keep parameterizing on the same version; only a *sequence*-level change forces a version bump). Every stored `FlowTemplateSkill` version keeps `version: int` (bumped) and `parent_version_id: str | None` (points at the version it diverged from), so history is a chain, not an overwrite; the newest version is what `skill-catalog`-style `get_skill`/list operations surface by default, but older versions remain fetchable by id.
|
|
**Alternative considered**: version by content hash of the full step list (any byte-level difference is a new version, including argument-value-only changes that D3 would otherwise absorb into a parameter). Rejected — this would create a new version on essentially every synthesis run (since argument values differ per goal by design), defeating the entire point of parameterization; divergence has to be judged at the *tool-name-sequence* level (structural), with argument-value differences intentionally absorbed as parameters rather than treated as version-worthy changes, otherwise D3's parameter abstraction and D4's versioning contradict each other.
|
|
|
|
### D5: Embedding-based retrieval via a dedicated `EmbeddingIndex`, not full-text/keyword search reused from `skill-catalog`'s existing search
|
|
`skill-embedding-retrieval` embeds `f"{name}: {description}\nOriginal goal: {goal}"` per locally-authored skill into a fixed-dimension vector using a single concrete embedding client (provider/model selected in implementation, config-overridable, mirroring `semantic-scene-runtime`'s D4 "config value, not hardcoded" stance), stores it in a new `skill_embeddings` local index (skill id → vector + model name + `updated_at`, re-embedded when a skill's description/goal changes at re-synthesis), and answers `retrieve_candidate_skills(goal, top_k)` via cosine similarity over that index, entirely in-process (no external vector-DB dependency introduced for this milestone's expected scale). `skill-catalog-subscription`'s existing `search_skills` (name/tag/description substring matching) is left untouched and is a different, complementary retrieval mode (exact/keyword vs. semantic).
|
|
**Alternative considered**: extend `skill-catalog-subscription`'s planned `search_skills` MCP tool to also do embedding similarity internally. Rejected — that tool's contract (per its own spec) is a synchronous, no-network-round-trip local query over the synced catalog; folding an embedding-model call into it would either force every `search_skills` invocation to pay embedding-call latency, or require pre-computed embeddings for *subscription-sourced* skills too, which is `skill-catalog-subscription`'s decision to make (or not) in its own future revision, not something this change should impose on it. A standalone `retrieve_candidate_skills()` in `skills_learning/retrieval.py` keeps the new retrieval mode additive and independently callable.
|
|
|
|
### D6: Locally-authored skills live in `skills_learning/`'s own catalog table, tagged `source = "local-synthesis"`, composed with `skill-catalog` only in prose
|
|
Synthesized `FlowTemplateSkill` records are persisted in a new local store owned by `skills_learning/` (same in-memory/on-disk shape as `skill-catalog-subscription`'s `Skill`/`FlowTemplateSkill` dataclasses, reused by import — not redefined — so both stores speak the same schema), with `source = "local-synthesis"` and no `subscription id`. This does not write into, nor require any schema change of, `skill-catalog-subscription`'s own store. Any future unification (one queryable namespace across "synced" and "locally-learned" skills, e.g. for a single `list_skills` MCP call to return both) is left as this change's Open Questions / a follow-on `skill-catalog-subscription` revision to make, since that store's spec today explicitly rejects non-sync writes and this change must not contradict that spec.
|
|
**Alternative considered**: propose a `MODIFIED Requirements` delta against `skill-catalog-subscription`'s `skill-catalog` spec loosening its "reject non-sync writes" rule to allow a `source = local-synthesis` exception. Rejected per this session's explicit constraint — `openspec/specs/` has no applied baseline for `skill-catalog`, so there is nothing to diff a `MODIFIED` delta against yet, and `skill-catalog-subscription` remains its own pending, unmodified change; composing in prose (shared dataclass shapes, distinct stores) achieves the same practical reuse without touching another change's files.
|
|
|
|
### D7: Reuse `semantic-scene-runtime`'s LLM-integration pattern for the embedding call: narrow client, degrade-safe, never blocks
|
|
The embedding client in `skills_learning/embeddings.py` follows the same shape `semantic-scene-runtime`'s `llm_client.py` established: a narrow, mockable client interface; `embed_skill_text(text) -> list[float] | None`, returning `None` (never raising) on timeout/rate-limit/disabled-config, exactly like `enrich_scene()`'s `SemanticScene | None` contract. Since embedding failure happens on the *post-task* synthesis path (not the live Observe→Act loop), a `None` result simply means "store the skill without an embedding, retrievable by name/id but not yet by similarity search" rather than blocking synthesis or versioning — `retrieve_candidate_skills()` skips any skill record with no stored vector.
|
|
**Alternative considered**: make embedding a hard requirement of synthesis (skill authoring fails/rolls back entirely if embedding fails). Rejected — this would let a transient embedding-provider outage silently prevent skill learning even though the flow-template synthesis and versioning (the actually load-bearing artifact) succeeded independently; degrading to "skill stored, not yet semantically searchable" is strictly better than "skill not learned at all," and matches the project's established degrade-path precedent from `semantic-scene-runtime`.
|
|
|
|
## Risks / Trade-offs
|
|
|
|
- **[Risk]** Diffing argument values across two executions to infer parameters (D3) can misfire if the *first* two executions of a goal-family happen to share a value that later varies (e.g. always searching "coffee" twice, never generalizing to `{search_query}` until a third, different execution appears) → **Mitigation**: this is an inherent, accepted limitation of any 2-sample diff approach — document it in Open Questions as "parameterization confidence improves with more samples, not guaranteed correct after exactly one divergence"; the skill is still usable (with the shared literal baked in) until a genuinely divergent execution arrives to trigger parameterization, so nothing breaks, it just under-parameterizes early.
|
|
- **[Risk]** Synthesizing a skill from a single successful task risks encoding an accidental/fragile path (e.g. steps that happened to work once but aren't a generally reliable flow) as if it were a validated reusable skill → **Mitigation**: out of scope to solve with a confidence/reliability score in this change (no repeated-execution signal exists yet for a single-run skill); `skills_learning/config.py` can gate authoring behind a "minimum N successful executions of this goal-family before first synthesis" threshold as a future config addition, but a real Planner retrieving and *trying* a skill (future work) is still expected to verify against the live `Scene`/`SemanticScene` before blindly trusting a one-shot-derived flow, matching `world-model-runtime`'s "advisory, not source-of-truth" stance on `WorldState`.
|
|
- **[Risk]** The tool-name-sequence-skeleton matching in D3/D4 (used to decide "is this the same skill as before") has no formal goal-similarity threshold defined in this design — two semantically different goals that happen to execute the identical tool-name skeleton (e.g. two different searches) could be treated as "the same skill family" prematurely → **Mitigation**: skeleton-matching is scoped as a heuristic starting point, documented as an Open Question; if this proves too coarse in practice, a future revision can require skeleton match **and** a minimum goal-embedding similarity (reusing D5's embedding index) before treating two runs as the same skill family, rather than skeleton alone.
|
|
- **[Trade-off]** Keeping locally-authored skills in a separate store from `skill-catalog-subscription`'s synced catalog (D6) means any consumer wanting "all skills, synced and learned" must query two stores and merge, not one → **Acceptable** because unifying them requires a `skill-catalog-subscription` spec change this session is explicitly scoped not to make; the shared dataclass shape (same `Skill`/`FlowTemplateSkill` types) keeps the merge trivial for whichever future change decides to do it (likely `skill-mcp-tools`' `list_skills` gaining a second source).
|
|
- **[Trade-off]** Defaulting Skill Authoring to disabled (unlike `world-model-runtime`'s default-enabled precedent) means most existing/near-term task runs will not accumulate learned skills until explicitly turned on → **Acceptable**, matching `semantic-scene-runtime`'s reasoning: synthesis + embedding is a real CPU/LLM-adjacent cost on the success path, and applying this change must not silently change existing task cost/latency profiles for callers who haven't opted in.
|
|
- **[Trade-off]** No skill-execution engine is built here (Non-Goal) — a synthesized `FlowTemplateSkill` is inert data until some future runner resolves its parameters and drives `tools/` — this is intentional scope discipline (per the proposal's Impact section) but means this milestone's value is not observable end-to-end (goal → learned skill → skill actually reused) until that follow-on work exists; acceptable since `skill-catalog-subscription`'s own proposal already scoped a "run_skill_flow parameter-resolution helper" as a distinct concern this change should not duplicate.
|
|
|
|
## Migration Plan
|
|
|
|
This is purely additive, matching the "safe default, optional hook" shape of `semantic-scene-runtime` and `world-model-runtime`:
|
|
1. Add the `skills_learning/` package: `synthesis.py`, `versioning.py`, `embeddings.py`, `retrieval.py`, `config.py` (enable flag defaulting to **disabled**, divergence tolerance, embedding model name, default `top_k`).
|
|
2. Reuse (import, do not redefine) `Skill`/`FlowTemplateSkill`/`SkillMetadata` dataclass shapes from `skill-catalog-subscription`'s planned `skills/models.py` once that change is implemented; if implemented first, `skills_learning/` depends on it for shared types only, never for its sync/storage code paths. If `skills_learning/` is implemented before `skill-catalog-subscription`, define the minimal shared dataclass shape locally and note it must be reconciled (not duplicated) once that change lands.
|
|
3. Add a new local store for synthesized skills (`skills_learning/`-owned catalog table, `source = "local-synthesis"`) plus the `skill_embeddings` index table.
|
|
4. Add the optional `on_task_succeeded: Callable[[str, str, Timeline], None] | None = None` constructor argument to `TaskRunner` (`runtime/task.py`); when Skill Authoring is enabled in config and no explicit hook is passed, `TaskRunner` wires a default one calling `synthesis.synthesize_flow_skill`; call it once, only after `status = succeeded` is determined, at the very end of `run()`.
|
|
5. Add the embedding-provider SDK dependency and `skills_learning*` to `pyproject.toml`'s `[tool.setuptools.packages.find].include` list; add embedding config with the enable flag defaulting to disabled so applying this change never silently changes existing task cost/latency (matching `semantic-scene-runtime`'s D-config precedent).
|
|
6. No changes to `storage/timeline.py`'s persisted format, `skill-catalog-subscription`'s schema/spec, or any existing tool signature.
|
|
7. Run `pytest` — full existing suite green with zero test-content edits; new tests (`tests/test_skill_synthesis.py`-style) exercise synthesis/versioning/retrieval logic against canned `Timeline` fixtures and a mocked embedding client, not live model calls.
|
|
8. Rollback: net-new package plus one additive, default-`None` constructor argument with a config-gated default wiring — reverting is `git revert` of the commit(s); no data migration of existing stores, no external system beyond the embedding provider (itself optional/degrade-safe per D7).
|
|
|
|
## Open Questions
|
|
|
|
- Whether the "same skill family" match (D3/D4's tool-name-skeleton matching) needs a goal-embedding-similarity floor in addition to skeleton equality, to avoid conflating two structurally-identical-but-semantically-different goals — left as a heuristic-first approach for this milestone, revisit once real synthesis data exists.
|
|
- Whether a minimum-successful-executions threshold should gate first-time synthesis (to avoid learning from a single lucky run) — left as a future `skills_learning/config.py` addition once there's real usage data to tune it against, per the Risks section.
|
|
- Whether/how a future `skill-catalog-subscription` revision should unify the synced and locally-authored stores behind one `list_skills`/`search_skills` surface (D6's deferred merge) — explicitly left to that change, not decided here.
|
|
- Which embedding provider/model to default to, and whether it should be the same provider as `semantic-scene-runtime`'s enrichment client (operational simplicity: one API key/provider to manage) or an independent choice optimized purely for embedding quality/cost — left as an implementation-time decision, not fixed in this design.
|
|
- Whether `WorldState.history` (from `world-model-runtime`, already flagged in that change's own Open Questions as a possible future Skill-synthesis consumer) should feed into synthesis here — e.g. using recent world-state transitions to help segment "which steps belong to this skill" versus incidental navigation — left open; this change synthesizes purely from `Timeline`'s tool-call sequence and does not yet consume `WorldState`.
|