magnus/agent-skills
1
1
Fork 0
Code Issues Pull requests Projects Releases 5 Packages Wiki Activity Actions

fix(cli-builder): align Phase 4 architecture with scripts/ convention #20

Merged
magnus merged 1 commit from fix/cli-builder-phase4-scripts-convention into main 2026-05-22 16:13:15 -04:00
Conversation 1 Commits 1 Files changed 1 +32 -12
jasper commented 2026-05-22 16:11:23 -04:00
Contributor
Copy link

Summary

Updates the cli-builder skill's Phase 4 (Skillify) to show the scripts/ convention for CLI placement, matching how every other skill in this repo is actually structured.

Changes

  • Two-Layer Architecture — Replaced the old ASCII-art diagram with a concrete servicex-cli/ directory tree showing scripts/servicex-cli. Added a Trigger/Execute layer table clarifying the role of each file.
  • What NOT to Put — Updated "Installation instructions" row to acknowledge both the scripts/ convention (for repo distribution) and global PATH deployment.
  • Completed Architecture — Fixed the directory tree to use scripts/, updated agent invocation to use scripts/servicex-cli relative path, added a deployment model comparison table (scripts/ inside skill vs global PATH), and stated the default recommendation.

Key design decisions

  1. scripts/ is the default — matches the Agent Skills spec, matches every other skill in this repo, enables progressive disclosure
  2. Global PATH is an option — acknowledged as valid when the CLI has use beyond the skill (other agents, human users, scripts). The skill body should note which model is in use.
  3. Relative-path invocation — scripts/servicex-cli list --json rather than bare servicex-cli in agent examples

Related Issues

Closes #19

Agent Disclosure

Filed by Jasper (AI agent on behalf of Magnus Hedemark)

## Summary Updates the cli-builder skill's Phase 4 (Skillify) to show the `scripts/` convention for CLI placement, matching how every other skill in this repo is actually structured. ## Changes - **Two-Layer Architecture** — Replaced the old ASCII-art diagram with a concrete `servicex-cli/` directory tree showing `scripts/servicex-cli`. Added a Trigger/Execute layer table clarifying the role of each file. - **What NOT to Put** — Updated "Installation instructions" row to acknowledge both the `scripts/` convention (for repo distribution) and global PATH deployment. - **Completed Architecture** — Fixed the directory tree to use `scripts/`, updated agent invocation to use `scripts/servicex-cli` relative path, added a deployment model comparison table (`scripts/` inside skill vs global PATH), and stated the default recommendation. ## Key design decisions 1. **`scripts/` is the default** — matches the Agent Skills spec, matches every other skill in this repo, enables progressive disclosure 2. **Global PATH is an option** — acknowledged as valid when the CLI has use beyond the skill (other agents, human users, scripts). The skill body should note which model is in use. 3. **Relative-path invocation** — `scripts/servicex-cli list --json` rather than bare `servicex-cli` in agent examples ## Related Issues Closes #19 ## Agent Disclosure Filed by Jasper (AI agent on behalf of Magnus Hedemark) 
The "Completed Architecture" diagram in Phase 4 showed the CLI binary at the
skill root, but every other skill in this repo (and the Agent Skills spec)
places executable scripts in a `scripts/` subdirectory.

Changes:
- Replaced ASCII-art two-layer diagram with concrete directory structure
  showing `scripts/servicex-cli` inside the skill directory
- Added Trigger/Execute layer table clarifying the role of each file
- Updated the "Agent opens session" flow to use `scripts/` relative paths
- Added deployment model comparison (scripts/ inside skill vs global PATH)
- Updated the "Installation instructions" entry in What NOT to Put table

Trigger: neopabo on Nous Research Discord
Signed-off-by: Magnus Hedemark <magnus919@pm.me>
magnus merged commit cecdecf6b0 into main 2026-05-22 16:13:15 -04:00
jasper reviewed 2026-05-22 16:17:57 -04:00
jasper left a comment
Copy link

First-pass code review — COMMENT

Overall this is a clean, well-structured documentation change. The scripts/ convention aligns with the Agent Skills spec and the actual structure of every other skill in this repo. The table format is more scannable than the old ASCII diagram, and the deployment model comparison gives clear guidance for both scenarios.

What's working well

  • Directory tree — concrete and accurate. Matches the actual repo layout.
  • Trigger/Execute table — clean replacement for the old ASCII side-by-side. Same content, better readability.
  • Deployment comparison — the scripts/ vs global PATH table is the right level of detail. Recommends scripts/ as default while acknowledging global PATH as valid.
  • "What NOT to Put" update — the installation instructions row now accurately reflects both distribution models.

Suggested follow-up (not blocking)

references/skill-wrapper-example.md is out of sync. The reference file shipped with the skill still shows the old directory structure:

weather-cli/
├── weather-cli              # CLI binary (built with Phases 1-3)
└── SKILL.md                 # Skill wrapper (Phase 4)

This should be updated to match the new scripts/ convention:

weather-cli/
├── scripts/
│   └── weather-cli          # CLI binary (built with Phases 1-3)
├── SKILL.md                 # Skill wrapper (Phase 4)
└── references/              # Supporting documentation

Similarly, the example invocation blocks (weather-cli current "Raleigh, NC") should note the scripts/ relative path or at minimum the compatibility frontmatter should mention both the relative-path and global-PATH deployment models.

This is a minor consistency gap — the main SKILL.md documents scripts/ as the default, but the only worked example in the skill still shows the old layout. New readers looking at the example will see a structure that contradicts the main document.

No issues found

  • No security concerns (documentation-only change)
  • No logic errors
  • Markdown is well-formed with correct table formatting
  • Design decisions are clearly stated and justified
  • Commit message is clean (conventional commit format, signed-off-by, trigger attribution)

Reviewed by Jasper (automated review bot) on behalf of Magnus Hedemark

## First-pass code review — COMMENT Overall this is a clean, well-structured documentation change. The `scripts/` convention aligns with the Agent Skills spec and the actual structure of every other skill in this repo. The table format is more scannable than the old ASCII diagram, and the deployment model comparison gives clear guidance for both scenarios. ### What's working well - **Directory tree** — concrete and accurate. Matches the actual repo layout. - **Trigger/Execute table** — clean replacement for the old ASCII side-by-side. Same content, better readability. - **Deployment comparison** — the `scripts/` vs global PATH table is the right level of detail. Recommends `scripts/` as default while acknowledging global PATH as valid. - **"What NOT to Put" update** — the installation instructions row now accurately reflects both distribution models. ### Suggested follow-up (not blocking) **`references/skill-wrapper-example.md` is out of sync.** The reference file shipped with the skill still shows the old directory structure: ``` weather-cli/ ├── weather-cli # CLI binary (built with Phases 1-3) └── SKILL.md # Skill wrapper (Phase 4) ``` This should be updated to match the new `scripts/` convention: ``` weather-cli/ ├── scripts/ │ └── weather-cli # CLI binary (built with Phases 1-3) ├── SKILL.md # Skill wrapper (Phase 4) └── references/ # Supporting documentation ``` Similarly, the example invocation blocks (`weather-cli current "Raleigh, NC"`) should note the `scripts/` relative path or at minimum the `compatibility` frontmatter should mention both the relative-path and global-PATH deployment models. This is a minor consistency gap — the main SKILL.md documents `scripts/` as the default, but the only worked example in the skill still shows the old layout. New readers looking at the example will see a structure that contradicts the main document. ### No issues found - No security concerns (documentation-only change) - No logic errors - Markdown is well-formed with correct table formatting - Design decisions are clearly stated and justified - Commit message is clean (conventional commit format, signed-off-by, trigger attribution) --- _Reviewed by Jasper (automated review bot) on behalf of Magnus Hedemark_
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
community-feedback
Originates from community review

  
enhancement
New feature or request

  
skill-upgrade
Skill enhancement or refactor

No labels
community-feedback
enhancement
skill-upgrade
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
magnus/agent-skills!20
No description provided.