Skip to content

feat: expand agent skill management - #35

Merged
chaim0m merged 1 commit into
mainfrom
codex/dci-skill-management
Aug 3, 2026
Merged

feat: expand agent skill management#35
chaim0m merged 1 commit into
mainfrom
codex/dci-skill-management

Conversation

@chaim0m

@chaim0m chaim0m commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add dci skill list with byte sizes and estimated token counts
  • add dci skill update [agent] with local-edit protection and --force
  • record installed managed-file checksums so a legitimate newer embedded version is not mistaken for a local edit
  • leave unmanaged files such as .DS_Store in place without letting them block future updates
  • add --dir for non-standard agent configuration locations
  • add dci skill --all for every detected supported agent
  • document skill inspection and update behavior in the README and embedded skill

Part of #13. Ambient customer, anomaly, and budget context is intentionally deferred because it needs a bounded API aggregate rather than multiple hidden client calls.

Test methods

Automated validation run on this branch:

go test ./...
go vet ./...

Manual validation in a disposable directory:

go build -o /tmp/dci-pr35 .
skill_dir=$(mktemp -d)
/tmp/dci-pr35 skill list --json | jq
/tmp/dci-pr35 skill codex --dir "$skill_dir"
touch "$skill_dir/skills/dci-cli/.DS_Store"
/tmp/dci-pr35 skill update codex --dir "$skill_dir"
echo 'local edit' >> "$skill_dir/skills/dci-cli/SKILL.md"
/tmp/dci-pr35 skill update codex --dir "$skill_dir"
/tmp/dci-pr35 skill update codex --dir "$skill_dir" --force

The update with .DS_Store should succeed and leave that file in place. The next update should protect the edited managed file and refuse to overwrite it. The explicit --force update should restore the embedded skill. Unit tests also simulate updating an unchanged skill installed by a previous CLI version.

Could this break things?

Risk: low-medium. This only writes inside a selected agent skill directory and does not call DCI APIs or touch customer data. A new .dci-skill-manifest.json file records managed-file checksums. Normal updates refuse to overwrite managed files changed since installation, while unmanaged files are preserved. The main overwrite risk is an operator deliberately using --force or selecting the wrong --dir.

@chaim0m
chaim0m marked this pull request as ready for review August 2, 2026 13:38
@chaim0m
chaim0m requested a review from apgiorgi as a code owner August 2, 2026 13:38
Comment thread skill_management.go Outdated
@chaim0m chaim0m self-assigned this Aug 3, 2026
@apgiorgi

apgiorgi commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Re-reviewed at 671bc1c. Built the branch and tested against throwaway dirs (never the real $HOME).

Both blockers are fixed, and one of them was my mistake.

The .DS_Store deadlock is genuinely resolved — dropping Extra from HasLocalChanges() (skill_management.go:223) does it. Verified: with a stray .DS_Store in the skill dir, updateupdate --forceupdate all succeed now, where before the first and third both failed permanently with no remedy.

Correction on my earlier "missing file blocks updates" note — that was a bad test on my part. I deleted an already-installed file, which is a deliberate user deletion, and blocking that is correct behavior. The manifest change (18fd222) makes baseline the recorded manifest rather than the embedded payload (skill_management.go:164-174), so a file that's new in a release isn't in the manifest and doesn't trigger the block. That's the right design and it resolves the concern. Sorry for the noise.

One narrow residue: when there's no manifest (installs predating this change), the fallback baseline is the embedded payload, so the old false positive fires once on the first update after upgrading. Probably acceptable — worth a line in the error message pointing at --force for that transition, which it already does.

Two things still open.

1. --force destroys edits with no backup and no diff

The error message says "inspect them and re-run with --force to overwrite," but nothing ever shows what changed, and --force is a plain overwrite. Verified:

$ echo 'MY LOCAL EDIT' >> $D/skills/dci-cli/SKILL.md
$ dci skill update codex --dir $D
Error: installed codex skill has local changes to SKILL.md; inspect them and re-run with --force to overwrite   # exit 1
$ dci skill update codex --dir $D --force
$ grep -c 'MY LOCAL EDIT' $D/skills/dci-cli/SKILL.md
0
$ find $D -name '*.bak' -o -name '*.orig' -o -name '*~' | wc -l
0

Issue #13 asks to "surface a diff if local edits exist." Right now we block, then irrecoverably clobber — the worst of both. Either write <file>.bak before overwriting, or print the diff, or both. Now that there's a checksum manifest, a diff against the recorded digest is cheap.

2. The path README documents is the unprotected one

dci skill update <agent> protects local edits. Plain dci skill <agent> — which is what README.md:169 tells users to run — still overwrites silently:

$ echo 'MY EDIT' >> $G/skills/dci-cli/SKILL.md
$ dci skill codex --dir $G
Skill installed to .../skills/dci-cli
$ grep -c 'MY EDIT' $G/skills/dci-cli/SKILL.md
0

Issue #13's framing was "today re-running dci skill <agent> would overwrite; make this intent obvious." That footgun is untouched — a second, protected path was added beside it, and --all now clobbers up to five agent dirs in one invocation. I'd either share the check or make skill <agent> an alias of the protected flow.

Smaller notes

  • --dir is filepath.Clean-ed and otherwise unvalidated. --dir "$T/sub/../../escape-target" installs outside the given subtree, and --dir '~/.codex' (unexpanded tilde, e.g. from a config value or CI var) creates a literal ~ directory in cwd and prints Skill installed to ~/.codex/... as if it worked. Perms are fine (0o755/0o644). Low severity, but the tilde case will confuse someone.
  • Batch update aborts mid-loop. With .claude dirty and .kiro clean, the run errors on claude and kiro is never refreshed — whether an agent gets updated depends on its position in skillAgents. Same for --all. Collect per-target results and print a summary instead of returning on first error.
  • Stale files are never pruned. Since installSkill only writes, a file removed from a future payload stays on disk forever and keeps loading into agent context — the opposite of the token-hygiene motivation in Skill-management gaps and ambient context for agents #13. The manifest now makes pruning straightforward: anything in the old manifest but not in the new payload is safe to delete.
  • skill_management.go has no comments (383 lines, zero //), and the split dropped an existing one — main.go had {"opencode", ".config/opencode"}, // OpenCode uses XDG config dir (~/.config), not a dotfile, and the new skillAgents carries nothing. The (len(data)+3)/4 token heuristic and the shared-backing-array sort.Strings loop both want a line of why.
  • The token number is ceil(bytes/4) presented as "ESTIMATED TOKENS" with no stated method. Column says ESTIMATED so it isn't dishonest, but nothing says it's a byte heuristic rather than a tokenizer (understates markdown by roughly 5–10%). list also prints no total, and Skill-management gaps and ambient context for agents #13's per-file budget + CI growth check aren't delivered.
  • skill list emits JSON whenever agentMode is on, which diverges from the CLI's own agent-mode format (TOON everywhere else) — and since agent mode turns on for any non-TTY stdout, dci skill list > out.txt silently switches format. Also --json is a new per-command output flag no other local command has. Either use the existing output plumbing or adopt --json as a deliberate local-command convention and apply it to status too.
  • Help output lost the destination path. dci skill codex --help went from Install skill into ~/.codex/skills/dci-cli/ to Install skill for codex, and README.md:175 points users at dci skill --help for exactly that.
  • Test coverage gaps. Nothing covers --force, --all, detectedSkillTargets, list rendering, or the update happy path on a clean install. main_test.go:1704-1712 still hard-codes its own copy of the agent/dir table instead of using skillAgents, and the installSkill tests stayed in main_test.go while the function moved.
  • errorsForNoDetectedAgents renders as Error: invalid argument: no supported agent directories were detected — that isn't an argument problem.

Test hygiene is good: every test uses t.TempDir() or an explicit override, nothing touches the real $HOME, and no real names/emails/internal URLs in fixtures. AGENTS.md "Key Files" still says "Single-file CLI: main.go (all logic)" and lists a skills-embed chapter that has now moved out — worth updating in this PR since it's the one that moved it.

@chaim0m
chaim0m force-pushed the codex/dci-skill-management branch from 671bc1c to c735c50 Compare August 3, 2026 16:38
@chaim0m
chaim0m merged commit 32859b5 into main Aug 3, 2026
6 checks passed
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.

3 participants