feat(skill): bcli skill install — slash command projection (AIP Phase 6/7)

[igor-ctrl]

Implements tasks/todo.md Phase 6 / agent-cli-contract-v0.1.md §Phase 7 — slash-command projection from saved queries and batch templates.

Summary

• New bcli skill install [--target PATH] [--dry-run] projects the active profile's saved queries and batch templates as Claude Code slash commands.
• One .claude/commands/bcli-<name>.md per saved query; one .claude/commands/bcli-batch-<name>.md per batch template at ~/.config/bcli/batches/<profile>/*.yaml.
• Top-level .claude/skills/bcli/SKILL.md index grouped by categories: (matches Claude Code's documented ~/.claude/skills/<name>/SKILL.md convention).
• Idempotent via SHA-256 content hash embedded in the provenance comment; re-runs on unchanged sources don't rewrite files.
• manual: true in a file's YAML frontmatter protects it from regeneration — even if a saved query by the same name exists.
• bcli skill is registered as a Typer group so Worker B's Phase 7 wizard (bcli skill init) can land alongside without restructuring.

Saved-query YAML schema extension (additive)

Three new optional fields, all backward-compatible:

Field	Type	Default	Used for
description	string	(existing — also flows into the slash command frontmatter)	Slash command description:
categories	list[str]	["unsorted"]	Grouping in SKILL.md
args	list[{name, type, required, example}]	derived from params: keys	Slash command positional ordering

Key migration trap addressed: when args: is omitted, the installer derives it from params: keys — required first, optional (with default:) second, both in YAML insertion order. Existing saved-query bundles get usable slash commands without hand-editing.

docs/saved-queries.md updated with the schema extension + a new "Slash-command projection" section covering idempotency, manual: true, dry-run, and --target resolution.

Coordination with Worker B (Phase 7)

• Typer group from the start (app.add_typer(skill_cmd.app, name="skill")); Worker B adds skill init inside without restructuring.
• Schema is documented; Worker B consumes the args: shape unchanged.

Decisions worth flagging

• Provenance has no generated_at timestamp — would invalidate the content hash on every run and cause infinite churn. The file's mtime tells you when it was generated.
• manual: true is YAML frontmatter only, not a comment — single parse path, one canonical convention.
• CWD-relative batch discovery deferred to v0.5; today we read ~/.config/bcli/batches/<profile>/*.yaml only. Open an issue if needed for project-checked-in batch workflows.
• No new runtime deps — stdlib templating (f-strings + string.Template-equivalent inline), no jinja2.

Test plan

• uv run pytest tests/test_skill_install/ -v — 12 tests, all green
• uv run pytest tests/ -v — 811 passed, 5 skipped (was 799 baseline; +12 new)
• HOME=/tmp/empty uv run pytest tests/test_skill_install/ tests/test_describe/ tests/test_idempotency/ — hermetic 47 passed
• uv run ruff check src/ tests/ — clean
• Manual: bcli skill --help shows the group; bcli skill install --help shows the subcommand
• Manual: bcli describe skill install --format json auto-picks up the new command (effects: ["other"], options: ["--target", "--dry-run"])

[igor-ctrl]

Lead review — REQUEST_CHANGES (integration refactor only; substantive content is solid)

The skill-install machinery is excellent — args inference from params: is the right backward-compat call, the no-generated_at idempotency is exactly the right defensive choice (avoids the same churn pitfall I flagged on PR #19's SKILL.md frontmatter), and the test suite covers every contract clause. 811 passed / 5 skipped / ruff clean / CI green on 3.11/3.12/3.13 / Mergeable=CLEAN.

But there's a real skill Typer group ownership collision with PR #19 that needs resolving before merge.

The collision

Both PR #19 (already APPROVED) and this PR register name="skill" via app.add_typer(...):

PR #19 src/bcli_cli/app.py:

app.add_typer(skill_init_cmd.app, name="skill", help="Generate a per-user bcli skill bundle (AIP Phase 7)")

PR #20 src/bcli_cli/app.py:

app.add_typer(
    skill_cmd.app,
    name="skill",
    help="Project saved queries + batch templates as Claude Code slash commands (AIP §Phase 7)",
)

Whichever lands second will text-conflict on this region, AND Typer's add_typer errors on duplicate group names — only one app.add_typer(..., name="skill", ...) call can exist.

Both PRs assumed the other would integrate around their group. PR #20's body even says "bcli skill is registered as a Typer GROUP so Worker B's Phase 7 bcli skill init wizard can land alongside without restructuring" — but PR #19 made the same assumption in reverse.

Resolution: Option A — PR #19 keeps the skill group; this PR adapts

Rationale:

• PR #19 was reviewed/approved first in this session; revising it would reopen a closed review loop.
• The init/update wizard is the broader Phase 7/8 mechanism — natural home for the group label.
• install is one operation that fits naturally inside that group.
• The required change in this PR is small.

Asked change (~5 lines):

1. src/bcli_cli/commands/skill_cmd.py:

   • Remove app = typer.Typer(...) constructor call.
   • At the top, add: from bcli_cli.commands import skill_init_cmd.
   • Replace app = typer.Typer(...) with: app = skill_init_cmd.app # share the ``skill`` Typer group with the wizard (Phase 7 mechanism).
   • Keep the @app.command("install") decorator on install_command — it now lands inside the shared group.
   • The __all__ = ["app", "install_command"] export stays; downstream tests can still runner.invoke(skill_cmd.app, ["install", ...]) against the same Typer instance.

2. src/bcli_cli/app.py:

   • Remove the new app.add_typer(skill_cmd.app, name="skill", ...) block. PR #19 already registers the group; importing skill_cmd in this module is enough — its @skill_init_cmd.app.command("install") decorator runs at import time and attaches install to the existing group.
   • Add skill_cmd to the import list at line 174 (the multi-line from bcli_cli.commands import (...) block) so the decorator is triggered.

After the refactor, bcli skill --help lists init, update, AND install — one group, three commands, no collision.

The substantive skill_cmd.py content (everything below the Typer constructor) doesn't need to change.

Substantive review (would be APPROVE absent the collision)

Args inference from params: (review focus #1) ✓
_coerce_args correctly preserves explicit args: when present, otherwise walks params: keys with required-first then optional-second, both in YAML insertion order (Python 3.7+ dict iteration is insertion-ordered). test_skill_install_args_inferred_from_params_when_omitted seeds a query with required: true first, two default: params after, and asserts the resulting argument-hint reflects that ordering. Existing saved-query bundles that don't carry args: get usable slash commands with zero hand-migration. Strong backward-compat win.

No-timestamp idempotency (review focus #2) ✓
test_skill_install_idempotent_no_changes is rigorous: captures mtime_ns after first run, sleeps 50ms for filesystem granularity, runs again, asserts mtime_2 == mtime_1. If the second run rewrote — even atomically — mtime would advance. Per-file content hash (content_hash: sha256:... in the provenance comment) gates rewrites in install_command via _existing_hash() == _content_hash_of(rendered). Combined with the no-generated_at design, re-runs on unchanged sources are byte-stable no-ops. test_skill_install_embedded_content_hash_is_stable independently pins that the embedded hash matches the body's hash, so external sha256sum'ing the file post-install would match.

manual: true YAML frontmatter (review focus #3) ✓
_is_manual parses the YAML frontmatter between --- markers via regex + yaml.safe_load, then checks loaded.get("manual", False). test_skill_install_preserves_manual_files writes a manual file, runs install, asserts the file is untouched and the install summary reports it. The switch from comment-based to YAML-frontmatter detection is the right call — frontmatter is the conventional Claude Code metadata location and survives a bcli skill update re-run cleanly.

Target resolution (review focus #4) ✓
_resolve_target_dir: explicit --target always wins → CWD with .claude/ → $HOME. test_skill_install_target_dir_defaults_to_cwd_when_dot_claude_present pins the second clause; the third clause is exercised implicitly via the other tests (which use --target to escape $HOME pollution). Worth one more test pinning the third clause explicitly, but non-blocking.

Batch template discovery (review focus #5) ✓
_discover_batches walks ~/.config/bcli/batches/<profile>/*.yaml. test_skill_install_creates_command_per_batch_yaml confirms a discovered batch becomes a slash command with the right body (bcli batch run <path> --result-out ... invocation). CWD-relative discovery deferral correctly documented in PR body.

docs/saved-queries.md updates (review focus #6) ✓
New ### Slash-command projection (Phase 6) section added describing the schema extension (description, categories, args with params: fallback documented) and command output shape. Cross-references both _coerce_args semantics and the manual-frontmatter override.

Additional notes (non-blocking)

• _render_command_md builds the file in chunks via string concatenation. Stdlib-only, no jinja2 dependency — clean.
• _summarise prints to Rich stdout. For pipe-redirected callers, the summary still goes to stdout — could be moved to stderr like the batch ledger metadata (Phase 3) for consistency, but acceptable for a one-shot wizard.
• Atomic writes use tempfile.mkstemp + os.replace (same pattern as _envelope_wrap and _atomic_write in PR #19) — consistent module-level idiom across the codebase.
• The <!-- content_hash: sha256:... --> HTML comment appended to SKILL.md and the file-level provenance comment is the same pattern PR #19 uses (YAML frontmatter for SKILL.md, inline provenance: block per query). Two different formats per file type but each appropriate to the consumer (Claude Code reads YAML frontmatter from SKILL.md; commands files use HTML comments for arbitrary metadata).

What to push next

1. skill_cmd.py: replace app = typer.Typer(...) with from bcli_cli.commands import skill_init_cmd; app = skill_init_cmd.app.
2. app.py: remove the app.add_typer(skill_cmd.app, name="skill", ...) block; ensure skill_cmd is in the import list so the @app.command("install") decorator runs.
3. Verify locally: uv run bcli skill --help lists init, update, install; uv run bcli skill install --help works; all 12 test_skill_install tests still green.
4. Re-push. I'll re-review on next ping.

Verdict: REQUEST_CHANGES on the integration refactor only. Substantive work is solid. Worker A — apologies the heads-up about PR #19's group registration didn't reach you in time.

[igor-ctrl]

Refactor applied per the review — chose rebase on top of PR #19 for a clean fast-forward post-merge.

Two-file delta (squashed into the existing commit via rebase, new SHA 310e9ca):

1. src/bcli_cli/commands/skill_cmd.py — replaced the local app = typer.Typer(...) with from bcli_cli.commands import skill_init_cmd; app = skill_init_cmd.app. All @app.command("install") decorators now attach to PR feat(skill): bcli skill init — interactive wizard mechanism (AIP Phase 7) #19's group.
2. src/bcli_cli/app.py — dropped my app.add_typer(skill_cmd.app, name="skill", ...). Kept the skill_cmd import (marked noqa: F401) so the @app.command("install") decorator still fires at module-load time. PR feat(skill): bcli skill init — interactive wizard mechanism (AIP Phase 7) #19's app.add_typer(skill_init_cmd.app, name="skill", ...) is the only registration.

Verified locally:

• uv run pytest tests/test_skill_install/ tests/test_skill_init/ -v → 32 passed (12 mine + 20 from PR feat(skill): bcli skill init — interactive wizard mechanism (AIP Phase 7) #19)
• uv run pytest tests/ -v → 831 passed, 5 skipped
• uv run ruff check src/ tests/ → clean
• uv run bcli skill --help lists init, update, and install together under one group:

╭─ Commands ───────────────────────────────────────────────────────╮
│ init     Interview the user and generate a per-user bcli skill bundle. │
│ update   Re-run the wizard against the current describe surface. │
│ install  Project saved queries + batch templates as Claude Code  │
│          slash commands.                                         │
╰──────────────────────────────────────────────────────────────────╯

Branch was force-pushed (82357b4 → 310e9ca) since the rebase replayed my commit on top of 6258aa3 (PR #19). After PR #19 lands on main, this branch is a clean fast-forward.

Ready for re-review.

[igor-ctrl]

Rebased on origin/main (now at 3d1e8da after #19's merge). Worker B's pre-merge commit 6258aa3 skipped as already-applied — no conflicts, clean replay of just my Phase 6 commit on top.

New branch shape:

7883a33 feat(skill): bcli skill install — slash command projection (AIP §Phase 7)
3d1e8da feat(skill): bcli skill init — interactive wizard mechanism (AIP Phase 7) (#19)   ← main HEAD

One commit on top of main; git log --oneline origin/main..HEAD shows exactly 7883a33.

Verified locally:

• uv run pytest tests/ → 831 passed, 5 skipped
• uv run ruff check src/ tests/ → clean
• uv run bcli skill --help → lists init, update, install together under one group

PR status: mergeable: MERGEABLE, headRefOid: 7883a33. Ready for re-review.

[igor-ctrl]

Lead re-review of 310e9ca — APPROVE

Refactor delta is exactly what I asked for. The substantive 592-line bcli skill install implementation is preserved unchanged; only the Typer-group integration is rewired.

Verification

Alias pattern in skill_cmd.py:

# `bcli skill` is a single Typer group shared by ``skill_init_cmd``
# (``init`` / ``update`` wizards — PR #19) and this module
# (``install``). PR #19 was approved first and owns the group's
# top-level registration in ``app.py``; we alias its Typer instance
# so ``@app.command("install")`` below attaches to the *same* group.
# This avoids a duplicate ``add_typer(..., name="skill", ...)`` (which
# Typer raises on) and keeps ``bcli skill --help`` listing init,
# update, and install side by side.
from bcli_cli.commands import skill_init_cmd  # noqa: E402

app = skill_init_cmd.app

Comment cites PR #19, explains the rationale (Typer raises on duplicate group names), and the alias keeps the rest of the module unchanged. The 592 lines below this stay-as-they-were.

app.py:

• skill_cmd added to the multi-line from bcli_cli.commands import (...) block with # noqa: F401 — import-time side effect registers \skill install`` — exactly the import-only-for-decorator-side-effect pattern.
• Explanatory comment introducing the skill_init_cmd.app registration block ("skill is a single Typer group shared by skill_init_cmd (init / update) and skill_cmd (install). PR #19 owns the group registration; the skill_cmd module aliases skill_init_cmd.app ...").
• Exactly one app.add_typer(... name="skill" ...) call (PR #19's). No app.add_typer(skill_cmd.app, name="skill", ...).

bcli skill --help (in /tmp/aip-review-20-refactor):

Commands
  init     Interview the user and generate a per-user bcli skill bundle.
  update   Re-run the wizard against the current describe surface.
  install  Project saved queries + batch templates as Claude Code slash
           commands.

Three commands, one group. No duplicate-group error at import. Clean.

Test suite + lint:

• uv run pytest tests/ → 831 passed, 5 skipped, 3 warnings in 21.31s (matches worker self-report: 811 Phase 6 baseline + 20 absorbed from PR #19 via rebase).
• uv run ruff check src/ tests/ → All checks passed!

GitHub state:

• headRefOid: 310e9caa67fc8b20f979aa0af1113be0a4eb57c2 matches Worker A's reported tip.
• CI: SUCCESS on 3.11/3.12/3.13.
• mergeable: CONFLICTING / mergeStateStatus: DIRTY is the expected state: this branch is rebased onto origin/feat/skill-init-mechanism (PR #19), so GitHub sees a dependency on commits not yet in main. Once Igor merges PR #19 (already APPROVED), this branch fast-forwards cleanly. Worker A flagged this in the self-report.

Rebase-vs-import-from-future choice

Worker A chose rebase over the "import from a future state" pattern. Both are valid; rebase has the advantage of clean fast-forward semantics post-merge of #19, with the trade-off of force-push-induced CI re-runs. Reasonable engineering call.

Substantive content (from prior review, unchanged)

All my prior APPROVE-grade observations on the substantive Phase 6 work stand:

• args: derivation from params: (required-first then optional, YAML insertion order) — strong backward-compat win.
• No-generated_at idempotency with SHA-256 content hash; mtime_ns invariance test confirms byte-stable re-runs.
• manual: true YAML frontmatter protects hand-edits.
• --target resolution: explicit > CWD-with-.claude > $HOME.
• Stdlib only — no jinja2.
• 12 tests pinning every contract clause.
• docs/saved-queries.md updated with the schema extension.

After this merges

bcli 0.4.0 is ready to tag. PR #19 merges first → PR #20 fast-forwards → version bump + CHANGELOG + tag. The release plan in tasks/todo.md is being extended with Phase 6 + Phase 7 (skills mechanism) inline now.

Verdict: APPROVE. Igor merges; I do not.

[igor-ctrl]

Post-merge-of-#19 follow-up — APPROVE still stands at 7883a33

Saw the orchestrator's heads-up that #19 has merged to main and you rebased feat/skill-install onto origin/main (new tip 7883a33, drops 6258aa3 as "already applied"). Re-verified on the new tip:

• git log origin/main..origin/feat/skill-install --oneline shows exactly one commit (7883a33) — Phase 6 work as a clean delta on merged-#19 main.
• Diff stat unchanged: 5 files, +1163 / -1.
• Local on 7883a33: 831 passed, 5 skipped, ruff clean in 21.65s.
• bcli skill --help still shows init / update / install together under one group.
• GitHub: mergeable: MERGEABLE, mergeStateStatus: CLEAN, CI SUCCESS on 3.11 / 3.12 / 3.13.

The substantive refactor delta is byte-equivalent to what I just APPROVE'd at 310e9ca — rebase only changed commit hashes. Verdict still: APPROVE at 7883a33. Igor merges.

After this lands, 0.4.0 is ready to tag.