#19 - cli-builder Phase 4 shows wrong architecture — should use scripts/ convention - magnus/agent-skills

jasper commented

2026-05-22 16:03:39 -04:00

Contributor

Problem

The cli-builder skill's Phase 4 (Skillify) section shows a "Completed Architecture" diagram that contradicts both the Agent Skills specification and the actual convention used by every other skill in this repo.

Current diagram (SKILL.md lines 451-454)

servicex-cli/
├── servicex-cli            # The CLI binary (built with Phases 1-3)
└── SKILL.md                # The skill wrapper (built in Phase 4)

This places the CLI binary at the skill root, alongside SKILL.md.

What every other skill in the repo actually does

Every skill that ships a CLI script puts it in a scripts/ subdirectory:

ghost-cli/
├── scripts/
│   └── ghost-cli
├── SKILL.md
└── references/

The scripts/ convention is:

Format-compliant — matches the Agent Skills spec's defined directory structure
Progressive disclosure — scripts are loaded on demand, not at startup
Cleaner root — the skill root only has SKILL.md, making it immediately obvious what the skill is
Consistent — all 11+ skill+CLI pairs in this repo already do this

Additional issues in Phase 4

The "Two-Layer Architecture" description is good but doesn't address the scripts/ convention
The distinction between "CLI lives in the skill's scripts/" vs "CLI lives on global PATH" is never discussed — both are valid for different deployment models, but only one is format-compliant for distribution via this repo

Proposal

Update Phase 4 to:

Fix the architecture diagram to show scripts/servicex-cli inside the skill directory, matching the repo's actual convention
Add a brief discussion of the two deployment models:
- scripts/ inside the skill (default for distribution via this repo — portable, self-contained)
- Global PATH (~/.hermes/scripts/ or equivalent) for when the tool needs to be available to other agents or human users outside the skill's context
Move or reference the scaffold template correctly — templates/ is not a standard Agent Skills directory; if it stays as a starter scaffold, it should be documented as such
Add compatibility/requirements info for what the consuming agent needs (the script on PATH, or a relative-path invocation from the skill directory)

Trigger

Raised by neopabo on the Nous Research Discord — their observation was that the cli-builder skill teaches a pattern that doesn't match the repo's own conventions.

## Problem The cli-builder skill's **Phase 4 (Skillify)** section shows a "Completed Architecture" diagram that contradicts both the Agent Skills specification and the actual convention used by every other skill in this repo. ### Current diagram (SKILL.md lines 451-454) ``` servicex-cli/ ├── servicex-cli # The CLI binary (built with Phases 1-3) └── SKILL.md # The skill wrapper (built in Phase 4) ``` This places the CLI binary at the skill root, alongside SKILL.md. ### What every other skill in the repo actually does Every skill that ships a CLI script puts it in a `scripts/` subdirectory: ``` ghost-cli/ ├── scripts/ │ └── ghost-cli ├── SKILL.md └── references/ ``` The `scripts/` convention is: - **Format-compliant** — matches the Agent Skills spec's defined directory structure - **Progressive disclosure** — scripts are loaded on demand, not at startup - **Cleaner root** — the skill root only has SKILL.md, making it immediately obvious what the skill is - **Consistent** — all 11+ skill+CLI pairs in this repo already do this ### Additional issues in Phase 4 1. The "Two-Layer Architecture" description is good but doesn't address the `scripts/` convention 2. The distinction between "CLI lives in the skill's `scripts/`" vs "CLI lives on global PATH" is never discussed — both are valid for different deployment models, but only one is format-compliant for distribution via this repo ## Proposal Update Phase 4 to: 1. **Fix the architecture diagram** to show `scripts/servicex-cli` inside the skill directory, matching the repo's actual convention 2. **Add a brief discussion** of the two deployment models: - `scripts/` inside the skill (default for distribution via this repo — portable, self-contained) - Global PATH (`~/.hermes/scripts/` or equivalent) for when the tool needs to be available to other agents or human users outside the skill's context 3. **Move or reference the scaffold template** correctly — `templates/` is not a standard Agent Skills directory; if it stays as a starter scaffold, it should be documented as such 4. **Add compatibility/requirements info** for what the consuming agent needs (the script on PATH, or a relative-path invocation from the skill directory) ## Trigger Raised by neopabo on the Nous Research Discord — their observation was that the cli-builder skill teaches a pattern that doesn't match the repo's own conventions.

magnus was assigned by jasper

2026-05-22 16:03:56 -04:00

jasper commented

2026-05-22 16:04:09 -04:00

Author

Contributor

Triage Assessment

This is a valid documentation bug. The cli-builder skill's Phase 4 architecture diagram is inconsistent with the scripts/ convention used by every other skill+CLI pair in this repo.

Key points

Confirmed: The repo convention is scripts/ under the skill directory (see ghost-cli, forgejo-cli, etc.)
Root cause: The skill was originally authored before the scripts/ convention was standardized across the repo
Scope: Lines 451-454 (architecture diagram), plus the Phase 4 prose needs a deployment-models discussion
No labels available on this repo to classify further

Recommended action

Accept the PR that fixes the architecture diagram to show scripts/servicex-cli
Add a brief deployment-models section covering scripts/ (distribution) vs global PATH (standalone use)
Clarify the templates/ scaffold status

Assigned to @magnus for review.

Jasper (automated triage)

## Triage Assessment This is a valid documentation bug. The cli-builder skill's Phase 4 architecture diagram is inconsistent with the `scripts/` convention used by every other skill+CLI pair in this repo. ### Key points - **Confirmed**: The repo convention is `scripts/` under the skill directory (see ghost-cli, forgejo-cli, etc.) - **Root cause**: The skill was originally authored before the scripts/ convention was standardized across the repo - **Scope**: Lines 451-454 (architecture diagram), plus the Phase 4 prose needs a deployment-models discussion - **No labels available** on this repo to classify further ### Recommended action 1. Accept the PR that fixes the architecture diagram to show `scripts/servicex-cli` 2. Add a brief deployment-models section covering `scripts/` (distribution) vs global PATH (standalone use) 3. Clarify the `templates/` scaffold status Assigned to @magnus for review. --- *Jasper (automated triage)*

jasper commented

2026-05-22 16:04:18 -04:00

Author

Contributor

Triage: Accepted

This is a correct observation from neopabo. The cli-builder skill's Phase 4 architecture diagram is inconsistent with the rest of the repo.

Assessment

Valid issue — the scripts/ convention IS the standard across this repo (ghost-cli, forgejo-cli, searxng-cli, tempest-cli, etc. all follow it)
Scope: SKILL.md lines ~451-454 need the diagram fix + the two deployment models need documenting
No labels available on this repo yet — consider adding labels like bug, documentation, good-first-issue

Recommended approach

Fix the architecture diagram to show scripts/servicex-cli/
Add a brief discussion of scripts/ (portable, format-compliant) vs global PATH (~/.hermes/scripts/) deployment models
The templates/ scaffold reference should note it's a starter template, not a standard Agent Skills directory

For magnus

This is a documentation fix in the cli-builder skill, not a code change. Low effort, high correctness value — especially if this skill is meant to teach newcomers the right patterns.

Jasper (automated)

## Triage: Accepted This is a correct observation from neopabo. The cli-builder skill's Phase 4 architecture diagram is inconsistent with the rest of the repo. ### Assessment - **Valid issue** — the `scripts/` convention IS the standard across this repo (ghost-cli, forgejo-cli, searxng-cli, tempest-cli, etc. all follow it) - **Scope**: SKILL.md lines ~451-454 need the diagram fix + the two deployment models need documenting - **No labels available** on this repo yet — consider adding labels like `bug`, `documentation`, `good-first-issue` ### Recommended approach 1. Fix the architecture diagram to show `scripts/servicex-cli/` 2. Add a brief discussion of `scripts/` (portable, format-compliant) vs global PATH (~/.hermes/scripts/) deployment models 3. The `templates/` scaffold reference should note it's a starter template, not a standard Agent Skills directory ### For magnus This is a documentation fix in the cli-builder skill, not a code change. Low effort, high correctness value — especially if this skill is meant to teach newcomers the right patterns. --- _Jasper (automated)_

jasper referenced this issue from a pull request that will close it,

2026-05-22 16:11:24 -04:00

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

jasper commented

2026-05-22 16:11:28 -04:00

Author

Contributor

Addressed in PR #20 — #20

Updated the Phase 4 architecture to use scripts/ convention, added deployment model comparison, and fixed the Two-Layer Architecture intro. Raised by neopabo on Discord.

Addressed in PR #20 — https://git.brandyapple.com/magnus/agent-skills/pulls/20 Updated the Phase 4 architecture to use `scripts/` convention, added deployment model comparison, and fixed the Two-Layer Architecture intro. Raised by neopabo on Discord.

magnus closed this issue

2026-05-22 16:13:15 -04:00

Rows
Columns

cli-builder Phase 4 shows wrong architecture — should use scripts/ convention #19

Problem

Current diagram (SKILL.md lines 451-454)

What every other skill in the repo actually does

Additional issues in Phase 4

Proposal

Trigger

Triage Assessment

Key points

Recommended action

Triage: Accepted

Assessment

Recommended approach

For magnus