Skip to content

refactor(skills): move fork-skill validation into SkillsToolset#2524

Merged
dgageot merged 1 commit intodocker:mainfrom
dgageot:board/extracting-skills-code-from-runtime-e329c176
Apr 27, 2026
Merged

refactor(skills): move fork-skill validation into SkillsToolset#2524
dgageot merged 1 commit intodocker:mainfrom
dgageot:board/extracting-skills-code-from-runtime-e329c176

Conversation

@dgageot
Copy link
Copy Markdown
Member

@dgageot dgageot commented Apr 27, 2026

Extract skill-specific business rules (lookup, fork-mode validation, content
expansion) from the runtime's handleRunSkill into a new
SkillsToolset.PrepareForkSubSession method. The runtime handler now keeps
only the parts that genuinely need its private internals: sub-session
creation, OpenTelemetry tracing, and event forwarding.

What changed

  • Add PreparedSkillFork struct + PrepareForkSubSession method to
    pkg/tools/builtin/skills.go. It returns either a populated bundle or a
    *tools.ToolCallResult describing the failure (skill missing, not
    configured for fork, content read error).
  • Shrink handleRunSkill from ~80 to ~50 lines; it now delegates
    validation/expansion to the toolset and focuses on runtime-private
    orchestration.
  • Add unit tests for PrepareForkSubSession covering the happy path,
    not-found, not-fork, and read-failure cases. These previously had no
    equivalent at the toolset layer.

Why stop here

Going further — moving handleRunSkill itself out of pkg/runtime (like
the bgAgents pattern in pkg/tools/builtin/agent) — runs into a real
obstacle: run_skill needs live event forwarding to the user's stream
so the skill execution streams in the TUI, and the events channel is typed
as chan runtime.Event. The bgAgent pattern sidesteps this by collecting
output via a plain OnContent func(string) callback, which run_skill
can't use without changing UX. Fully extracting would require either
pushing Event out into a shared package (a much larger refactor) or
loose-typing the channel as chan any. Neither felt worth it for the ~50
lines that remain — those genuinely are runtime concerns (sub-session
creation, OTEL span, event channel).

Validation

  • + "mise lint" + — 0 issues
  • + "mise test" + — all packages pass

Assisted-By: docker-agent

@dgageot
refactor(skills): move fork-skill validation into SkillsToolset
d194cee 
Extract skill-specific business rules (lookup, fork-mode validation,
content expansion) from the runtime's handleRunSkill into a new
SkillsToolset.PrepareForkSubSession method. The runtime handler now
keeps only the parts that genuinely need its private internals:
sub-session creation, OpenTelemetry tracing, and event forwarding.

- Add PreparedSkillFork struct + PrepareForkSubSession method to
  pkg/tools/builtin/skills.go. It returns either a populated bundle
  or a *tools.ToolCallResult describing the failure (skill missing,
  not configured for fork, content read error).
- Shrink handleRunSkill from ~80 to ~50 lines; it now delegates
  validation/expansion to the toolset and focuses on runtime-private
  orchestration.
- Add unit tests for PrepareForkSubSession covering the happy path,
  not-found, not-fork, and read-failure cases. These previously had
  no equivalent at the toolset layer.

Assisted-By: docker-agent
@dgageot dgageot requested a review from a team as a code owner April 27, 2026 11:15
@dgageot dgageot merged commit 9c7270d into docker:main Apr 27, 2026
9 checks passed
@dgageot dgageot mentioned this pull request Apr 27, 2026
Merged
dgageot added a commit that referenced this pull request Apr 27, 2026
@dgageot
fix(skills): unbreak main after fork-skill refactor merge
70a5c7e 
The merge of #2524 (move fork-skill validation into SkillsToolset) on
top of #2525 (skill model-override) left handleRunSkill referencing
identifiers that no longer exist in its scope: skill.Model and
params.Name. The refactor renamed params -> args and dropped the
local skill variable in favour of a PreparedSkillFork bundle, but
the model-override block introduced by #2525 was not updated, so
package github.com/docker/docker-agent/pkg/runtime failed to build.

Restore the build by surfacing the model override through the
toolset, in line with the refactor's intent of keeping skill-specific
business rules out of the runtime.

- Add Model to PreparedSkillFork and populate it from skill.Model
  inside (*SkillsToolset).PrepareForkSubSession, so the runtime no
  longer needs a direct skills.Skill reference.
- Switch handleRunSkill's override block to prepared.Model and
  prepared.SkillName; the WithAgentModel call, defer restore(), and
  warning log are otherwise unchanged.
- Extend the PrepareForkSubSession happy-path test to assert the
  Model field is propagated, and add a _NoModelOverride case that
  pins down the empty-Model contract the runtime relies on to skip
  the override path.

Validated with go build, go vet, golangci-lint (0 issues), and
go test on pkg/runtime, pkg/tools/builtin, pkg/skills, and pkg/agent.

Assisted-By: docker-agent
dgageot added a commit to dgageot/cagent that referenced this pull request Apr 27, 2026
@dgageot
feat(hooks): add before_compaction and after_compaction events
7335ad0 
Session compaction is now first-class in the hooks mechanism. Two new
events let users observe, veto, or replace a compaction without
touching the runtime:

- before_compaction fires immediately before a compaction. The hook
  receives input_tokens, output_tokens, context_limit and a
  compaction_reason of "threshold" (proactive 90% trigger),
  "overflow" (post-overflow recovery) or "manual" (user-invoked
  /compact). It can veto the compaction (Decision: "block") or
  supply a custom summary string via HookSpecificOutput.summary,
  in which case the runtime applies that summary verbatim and skips
  the LLM-based summarization.
- after_compaction fires once a summary has been applied to the
  session. Receives the produced summary text in Input.summary
  alongside the same token / reason fields. Purely observational.

The runtime also now re-emits session_start with Source="compact"
right after a successful compaction so hooks like add_environment_info
can re-inject ambient context that may have been lost in the summary.
This honors the previously dead "compact" Source value documented on
hooks.Input.Source.

When no compaction-related hooks are configured, control flow through
doCompact is bit-for-bit identical to the previous implementation —
guarded by a new TestDoCompactNoHooksMatchesPriorBehavior regression
test. dispatchHook short-circuits when no executor is configured, so
the hook dispatch sites are essentially free in the common case.

Implementation notes:

- pkg/hooks/types.go gets the two new EventType constants, two new
  Config fields, six new Input fields (input_tokens, output_tokens,
  context_limit, compaction_reason, summary; the existing source
  field is documented to include "compact"), and a Summary field on
  HookSpecificOutput plus Result. aggregate() surfaces the first
  non-empty Summary into Result.Summary; concurrent execution makes
  "first wins" deterministic at the value level (no concatenation,
  no clobbering).
- pkg/runtime/hooks.go adds executeBeforeCompactionHooks,
  executeAfterCompactionHooks, and a parameterised
  executeSessionStartHooksWithSource (the existing
  executeSessionStartHooks is now a thin wrapper).
- pkg/runtime/session_compaction.go threads a reason parameter into
  doCompact and adds a computeFirstKeptEntry helper so the
  hook-supplied-summary path keeps the same maxKeepTokens tail-keep
  policy as the LLM path. A new internal compactWithReason method
  on LocalRuntime forwards the reason; the public Summarize stays
  unchanged and reports "manual". loop.go callsites tag "threshold"
  and "overflow".

Schema and example:

- agent-schema.json adds the two new properties on HooksConfig with
  the full contract documented (TestSchemaMatchesGoTypes covers
  drift between schema and Go types).
- examples/hooks.yaml adds two well-commented blocks demonstrating
  observational logging and veto/replace semantics, with a warning
  about denying on compaction_reason="overflow".

Tests:

- pkg/hooks: 5 new tests covering allow-by-default, exit-code-2 veto,
  HookSpecificOutput.summary surfacing into Result.Summary,
  first-summary-wins under concurrent dispatch, after_compaction's
  observational contract, and the JSON wire format.
- pkg/runtime: 5 new tests covering deny-skips-everything,
  hook-summary-skips-LLM, after_compaction fires with the produced
  summary, session_start re-emit with Source="compact", and the
  no-hooks regression guard.

Drive-by fix: a merge between docker#2524 (PrepareForkSubSession
extraction) and the earlier model-override series left
pkg/runtime/skill_runner.go referencing two no-longer-defined
symbols (skill, params). PreparedSkillFork now carries the Model
override and the runner reads from prepared.Model / prepared.SkillName.

Assisted-By: docker-agent
dgageot added a commit to dgageot/cagent that referenced this pull request Apr 28, 2026
@dgageot
feat(hooks): add before_compaction and after_compaction events
6e4748e 
Session compaction is now first-class in the hooks mechanism. Two new
events let users observe, veto, or replace a compaction without
touching the runtime:

- before_compaction fires immediately before a compaction. The hook
  receives input_tokens, output_tokens, context_limit and a
  compaction_reason of "threshold" (proactive 90% trigger),
  "overflow" (post-overflow recovery) or "manual" (user-invoked
  /compact). It can veto the compaction (Decision: "block") or
  supply a custom summary string via HookSpecificOutput.summary,
  in which case the runtime applies that summary verbatim and skips
  the LLM-based summarization.
- after_compaction fires once a summary has been applied to the
  session. Receives the produced summary text in Input.summary
  alongside the same token / reason fields. Purely observational.

The runtime also now re-emits session_start with Source="compact"
right after a successful compaction so hooks like add_environment_info
can re-inject ambient context that may have been lost in the summary.
This honors the previously dead "compact" Source value documented on
hooks.Input.Source.

When no compaction-related hooks are configured, control flow through
doCompact is bit-for-bit identical to the previous implementation —
guarded by a new TestDoCompactNoHooksMatchesPriorBehavior regression
test. dispatchHook short-circuits when no executor is configured, so
the hook dispatch sites are essentially free in the common case.

Implementation notes:

- pkg/hooks/types.go gets the two new EventType constants, two new
  Config fields, six new Input fields (input_tokens, output_tokens,
  context_limit, compaction_reason, summary; the existing source
  field is documented to include "compact"), and a Summary field on
  HookSpecificOutput plus Result. aggregate() surfaces the first
  non-empty Summary into Result.Summary; concurrent execution makes
  "first wins" deterministic at the value level (no concatenation,
  no clobbering).
- pkg/runtime/hooks.go adds executeBeforeCompactionHooks,
  executeAfterCompactionHooks, and a parameterised
  executeSessionStartHooksWithSource (the existing
  executeSessionStartHooks is now a thin wrapper).
- pkg/runtime/session_compaction.go threads a reason parameter into
  doCompact and adds a computeFirstKeptEntry helper so the
  hook-supplied-summary path keeps the same maxKeepTokens tail-keep
  policy as the LLM path. A new internal compactWithReason method
  on LocalRuntime forwards the reason; the public Summarize stays
  unchanged and reports "manual". loop.go callsites tag "threshold"
  and "overflow".

Schema and example:

- agent-schema.json adds the two new properties on HooksConfig with
  the full contract documented (TestSchemaMatchesGoTypes covers
  drift between schema and Go types).
- examples/hooks.yaml adds two well-commented blocks demonstrating
  observational logging and veto/replace semantics, with a warning
  about denying on compaction_reason="overflow".

Tests:

- pkg/hooks: 5 new tests covering allow-by-default, exit-code-2 veto,
  HookSpecificOutput.summary surfacing into Result.Summary,
  first-summary-wins under concurrent dispatch, after_compaction's
  observational contract, and the JSON wire format.
- pkg/runtime: 5 new tests covering deny-skips-everything,
  hook-summary-skips-LLM, after_compaction fires with the produced
  summary, session_start re-emit with Source="compact", and the
  no-hooks regression guard.

Drive-by fix: a merge between docker#2524 (PrepareForkSubSession
extraction) and the earlier model-override series left
pkg/runtime/skill_runner.go referencing two no-longer-defined
symbols (skill, params). PreparedSkillFork now carries the Model
override and the runner reads from prepared.Model / prepared.SkillName.

Assisted-By: docker-agent
@BrewTestBot BrewTestBot mentioned this pull request Apr 28, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gtardif gtardif gtardif approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dgageot @gtardif