mirror of
https://github.com/wshobson/agents
synced 2026-06-21 14:13:58 +00:00
review: address code-audit findings — .omo/ artifact, docs drift, YAML quoting, validator helper
This commit is contained in:
@@ -42,6 +42,9 @@ node_modules
|
|||||||
.opencode/
|
.opencode/
|
||||||
opencode.json
|
opencode.json
|
||||||
|
|
||||||
|
# OpenCode session continuation artifacts
|
||||||
|
.omo/
|
||||||
|
|
||||||
# Copilot
|
# Copilot
|
||||||
.copilot/
|
.copilot/
|
||||||
|
|
||||||
|
|||||||
+1
-1
@@ -4,7 +4,7 @@ Top-level architectural map for the claude-agents marketplace. Detail lives in [
|
|||||||
|
|
||||||
## Invariants
|
## Invariants
|
||||||
|
|
||||||
1. **Single source of truth.** All agent / skill / command authoring happens under `plugins/<name>/`. Generated harness-specific artifacts (`.codex/`, `.cursor-plugin/`, `.opencode/`, `.github/agents/`, `.github/skills/`, `commands/`, `agents/`, `skills/` at extension root for Gemini) are produced by adapters and gitignored. Never hand-edit generated files.
|
1. **Single source of truth.** All agent / skill / command authoring happens under `plugins/<name>/`. Generated harness-specific artifacts (`.codex/`, `.cursor-plugin/`, `.opencode/`, `.copilot/`, `commands/`, `agents/`, `skills/` at extension root for Gemini) are produced by adapters and gitignored. Never hand-edit generated files.
|
||||||
|
|
||||||
2. **One canonical context file.** `AGENTS.md` at repo root is the only context file authored directly. `CLAUDE.md` imports it via `@AGENTS.md`. Gemini CLI reads it via `.gemini/settings.json` `context.fileName`. Codex / Cursor / OpenCode read `AGENTS.md` natively.
|
2. **One canonical context file.** `AGENTS.md` at repo root is the only context file authored directly. `CLAUDE.md` imports it via `@AGENTS.md`. Gemini CLI reads it via `.gemini/settings.json` `context.fileName`. Codex / Cursor / OpenCode read `AGENTS.md` natively.
|
||||||
|
|
||||||
|
|||||||
@@ -161,7 +161,7 @@ clean:
|
|||||||
# make generate-all
|
# make generate-all
|
||||||
# make clean-generated HARNESS=opencode
|
# make clean-generated HARNESS=opencode
|
||||||
|
|
||||||
HARNESSES := codex copilot cursor opencode gemini
|
HARNESSES := codex copilot cursor gemini opencode
|
||||||
|
|
||||||
generate:
|
generate:
|
||||||
ifndef HARNESS
|
ifndef HARNESS
|
||||||
@@ -195,7 +195,7 @@ garden:
|
|||||||
|
|
||||||
# Full pytest suite — plugin-eval framework + tools/ adapters/validators/gardener.
|
# Full pytest suite — plugin-eval framework + tools/ adapters/validators/gardener.
|
||||||
test:
|
test:
|
||||||
uv run $(EVAL_PROJECT) --extra dev pytest -q plugins/plugin-eval/ tools/tests/
|
uv run $(EVAL_PROJECT) pytest -q plugins/plugin-eval/ tools/tests/
|
||||||
|
|
||||||
# Real-CLI smoke test. Generates artifacts (if not present), then invokes whichever
|
# Real-CLI smoke test. Generates artifacts (if not present), then invokes whichever
|
||||||
# of opencode / gemini / codex / claude are on PATH. Per-CLI tests skip gracefully
|
# of opencode / gemini / codex / claude are on PATH. Per-CLI tests skip gracefully
|
||||||
@@ -206,7 +206,7 @@ smoke-test:
|
|||||||
echo "Generating harness artifacts first..."; \
|
echo "Generating harness artifacts first..."; \
|
||||||
$(MAKE) generate-all; \
|
$(MAKE) generate-all; \
|
||||||
fi
|
fi
|
||||||
uv run $(EVAL_PROJECT) --extra dev pytest -v tools/tests/test_cli_smoke.py
|
uv run $(EVAL_PROJECT) pytest -v tools/tests/test_cli_smoke.py
|
||||||
|
|
||||||
clean-generated:
|
clean-generated:
|
||||||
ifdef HARNESS
|
ifdef HARNESS
|
||||||
|
|||||||
+7
-8
@@ -1,7 +1,6 @@
|
|||||||
# Authoring portable plugin content
|
# Authoring portable plugin content
|
||||||
|
|
||||||
Plugin content in this repo ships to **five** harnesses: Claude Code (source of truth),
|
Plugin content in this repo ships to **five** harnesses: OpenAI Codex CLI, Cursor, OpenCode, Gemini CLI, and GitHub Copilot. Claude Code is the source-of-truth. The adapter framework handles per-harness
|
||||||
OpenAI Codex CLI, Cursor, OpenCode, and Gemini CLI. The adapter framework handles per-harness
|
|
||||||
mechanics (frontmatter rewrites, format transforms, output paths) so you author one set of
|
mechanics (frontmatter rewrites, format transforms, output paths) so you author one set of
|
||||||
markdown files. But content choices still affect portability — this guide tells you what to
|
markdown files. But content choices still affect portability — this guide tells you what to
|
||||||
do, and what to avoid, so the work you do for Claude Code translates cleanly everywhere.
|
do, and what to avoid, so the work you do for Claude Code translates cleanly everywhere.
|
||||||
@@ -106,12 +105,12 @@ clean naming — pick distinct names for skill/command pairs within a plugin.
|
|||||||
|
|
||||||
### Model aliases
|
### Model aliases
|
||||||
|
|
||||||
| Source field | Codex | Cursor | OpenCode | Gemini |
|
| Source field | Codex | Cursor | OpenCode | Gemini | Copilot |
|
||||||
|---|---|---|---|---|
|
|---|---|---|---|---|---|---|
|
||||||
| `model: opus` | `gpt-5` | `inherit` | `anthropic/claude-opus-4-7` | `gemini-2.5-pro` |
|
| `model: opus` | `gpt-5` | `inherit` | `anthropic/claude-opus-4-7` | `gemini-2.5-pro` | `gpt-5` |
|
||||||
| `model: sonnet` | `gpt-5-mini` | `inherit` | `anthropic/claude-sonnet-4-6` | `gemini-2.5-pro` |
|
| `model: sonnet` | `gpt-5-mini` | `inherit` | `anthropic/claude-sonnet-4-6` | `gemini-2.5-pro` | `gpt-5-mini` |
|
||||||
| `model: haiku` | `gpt-5-nano` | `inherit` | `anthropic/claude-haiku-4-5-20251001` | `gemini-2.5-flash` |
|
| `model: haiku` | `gpt-5-nano` | `inherit` | `anthropic/claude-haiku-4-5-20251001` | `gemini-2.5-flash` | `gpt-5-nano` |
|
||||||
| `model: inherit` | `gpt-5` | `inherit` | `anthropic/claude-sonnet-4-6` | `gemini-2.5-pro` |
|
| `model: inherit` | `gpt-5` | `inherit` | `anthropic/claude-sonnet-4-6` | `gemini-2.5-pro` | `gpt-5` |
|
||||||
|
|
||||||
The adapter handles mapping. The `BARE_MODEL_ALIAS` lint is informational — it just notes
|
The adapter handles mapping. The `BARE_MODEL_ALIAS` lint is informational — it just notes
|
||||||
that the mapping is implicit. If you want explicit, use `inherit`.
|
that the mapping is implicit. If you want explicit, use `inherit`.
|
||||||
|
|||||||
@@ -24,6 +24,8 @@ def _needs_yaml_quoting(value: str) -> bool:
|
|||||||
"false",
|
"false",
|
||||||
"yes",
|
"yes",
|
||||||
"no",
|
"no",
|
||||||
|
"on",
|
||||||
|
"off",
|
||||||
"null",
|
"null",
|
||||||
"~",
|
"~",
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -1050,7 +1050,12 @@ class TestCopilotAdapter:
|
|||||||
assert _needs_yaml_quoting("3.14")
|
assert _needs_yaml_quoting("3.14")
|
||||||
assert _needs_yaml_quoting("true")
|
assert _needs_yaml_quoting("true")
|
||||||
assert _needs_yaml_quoting("false")
|
assert _needs_yaml_quoting("false")
|
||||||
|
assert _needs_yaml_quoting("yes")
|
||||||
|
assert _needs_yaml_quoting("no")
|
||||||
|
assert _needs_yaml_quoting("on")
|
||||||
|
assert _needs_yaml_quoting("off")
|
||||||
assert _needs_yaml_quoting("null")
|
assert _needs_yaml_quoting("null")
|
||||||
|
assert _needs_yaml_quoting("~")
|
||||||
assert not _needs_yaml_quoting("hello world")
|
assert not _needs_yaml_quoting("hello world")
|
||||||
assert not _needs_yaml_quoting("Use when testing.")
|
assert not _needs_yaml_quoting("Use when testing.")
|
||||||
|
|
||||||
@@ -1161,7 +1166,7 @@ class TestCapabilities:
|
|||||||
def test_every_adapter_id_has_capabilities_entry(self):
|
def test_every_adapter_id_has_capabilities_entry(self):
|
||||||
from tools.adapters.capabilities import CAPABILITIES
|
from tools.adapters.capabilities import CAPABILITIES
|
||||||
|
|
||||||
for adapter_cls in (CodexAdapter, CopilotAdapter, CursorAdapter, OpenCodeAdapter, GeminiAdapter):
|
for adapter_cls in (CodexAdapter, CopilotAdapter, CursorAdapter, GeminiAdapter, OpenCodeAdapter):
|
||||||
assert adapter_cls.harness_id in CAPABILITIES
|
assert adapter_cls.harness_id in CAPABILITIES
|
||||||
|
|
||||||
def test_model_aliases_complete(self):
|
def test_model_aliases_complete(self):
|
||||||
|
|||||||
@@ -178,7 +178,7 @@ class TestCopilotValidator:
|
|||||||
|
|
||||||
report = Report()
|
report = Report()
|
||||||
validate_copilot(report)
|
validate_copilot(report)
|
||||||
assert any("must not be empty" in f.message for f in report.errors())
|
assert any("is empty" in f.message for f in report.errors())
|
||||||
|
|
||||||
def test_missing_description_errors(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch):
|
def test_missing_description_errors(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch):
|
||||||
_patch_worktree(monkeypatch, tmp_path)
|
_patch_worktree(monkeypatch, tmp_path)
|
||||||
|
|||||||
+54
-83
@@ -61,6 +61,53 @@ class Report:
|
|||||||
return [f for f in self.findings if f.severity == "info"]
|
return [f for f in self.findings if f.severity == "info"]
|
||||||
|
|
||||||
|
|
||||||
|
# ── Helpers ──────────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
|
||||||
|
def _check_nonempty_str_field(
|
||||||
|
report: Report,
|
||||||
|
fm: dict,
|
||||||
|
field: str,
|
||||||
|
harness: str,
|
||||||
|
path: Path,
|
||||||
|
label: str = "",
|
||||||
|
) -> bool:
|
||||||
|
"""Validate that `field` exists in `fm`, is a str, and is non-empty.
|
||||||
|
|
||||||
|
Adds one Finding per violation. Returns True if the field passes all checks.
|
||||||
|
``label`` is only used in remediation hints (defaults to ``field``).
|
||||||
|
"""
|
||||||
|
label = label or field
|
||||||
|
if field not in fm:
|
||||||
|
report.add(
|
||||||
|
severity="error",
|
||||||
|
harness=harness,
|
||||||
|
path=path,
|
||||||
|
message=f"missing required `{field}` field in frontmatter",
|
||||||
|
remediation=f"Each {harness} {label} needs a `{field}`.",
|
||||||
|
)
|
||||||
|
return False
|
||||||
|
if not isinstance(fm[field], str):
|
||||||
|
report.add(
|
||||||
|
severity="error",
|
||||||
|
harness=harness,
|
||||||
|
path=path,
|
||||||
|
message=f"`{field}` field must be a string",
|
||||||
|
remediation=f"Set `{field}` to a plain string in the {label} frontmatter.",
|
||||||
|
)
|
||||||
|
return False
|
||||||
|
if not fm[field].strip():
|
||||||
|
report.add(
|
||||||
|
severity="error",
|
||||||
|
harness=harness,
|
||||||
|
path=path,
|
||||||
|
message=f"`{field}` field is empty",
|
||||||
|
remediation=f"Provide a non-empty `{field}` in the {label} frontmatter.",
|
||||||
|
)
|
||||||
|
return False
|
||||||
|
return True
|
||||||
|
|
||||||
|
|
||||||
# ── Codex validators ─────────────────────────────────────────────────────────
|
# ── Codex validators ─────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
|
||||||
@@ -583,54 +630,8 @@ def validate_copilot(report: Report) -> None:
|
|||||||
remediation="Regenerate via `make generate HARNESS=copilot`.",
|
remediation="Regenerate via `make generate HARNESS=copilot`.",
|
||||||
)
|
)
|
||||||
continue
|
continue
|
||||||
if "name" not in fm:
|
_check_nonempty_str_field(report, fm, "name", "copilot", agent_md, label="agent")
|
||||||
report.add(
|
_check_nonempty_str_field(report, fm, "description", "copilot", agent_md, label="agent")
|
||||||
severity="error",
|
|
||||||
harness="copilot",
|
|
||||||
path=agent_md,
|
|
||||||
message="missing required `name` field in frontmatter",
|
|
||||||
remediation="Each Copilot agent needs a name.",
|
|
||||||
)
|
|
||||||
elif not isinstance(fm["name"], str):
|
|
||||||
report.add(
|
|
||||||
severity="error",
|
|
||||||
harness="copilot",
|
|
||||||
path=agent_md,
|
|
||||||
message="`name` field must be a string",
|
|
||||||
remediation="Set `name` to a plain string in the agent frontmatter.",
|
|
||||||
)
|
|
||||||
elif not fm["name"].strip():
|
|
||||||
report.add(
|
|
||||||
severity="error",
|
|
||||||
harness="copilot",
|
|
||||||
path=agent_md,
|
|
||||||
message="`name` field must not be empty",
|
|
||||||
remediation="Provide a non-empty `name` in the agent frontmatter.",
|
|
||||||
)
|
|
||||||
if "description" not in fm:
|
|
||||||
report.add(
|
|
||||||
severity="error",
|
|
||||||
harness="copilot",
|
|
||||||
path=agent_md,
|
|
||||||
message="missing required `description` field in frontmatter",
|
|
||||||
remediation="Copilot requires `description` in agent frontmatter. Check the source agent file.",
|
|
||||||
)
|
|
||||||
elif not isinstance(fm.get("description"), str):
|
|
||||||
report.add(
|
|
||||||
severity="error",
|
|
||||||
harness="copilot",
|
|
||||||
path=agent_md,
|
|
||||||
message="`description` field must be a string",
|
|
||||||
remediation="Set `description` to a plain string; Copilot agent frontmatter does not accept other types.",
|
|
||||||
)
|
|
||||||
elif not fm["description"].strip():
|
|
||||||
report.add(
|
|
||||||
severity="error",
|
|
||||||
harness="copilot",
|
|
||||||
path=agent_md,
|
|
||||||
message="`description` field is empty",
|
|
||||||
remediation="Copilot requires a non-empty `description` in agent frontmatter.",
|
|
||||||
)
|
|
||||||
# 3. Skills: validate .copilot/skills/*/SKILL.md exists and has valid frontmatter.
|
# 3. Skills: validate .copilot/skills/*/SKILL.md exists and has valid frontmatter.
|
||||||
skills_dir = (WORKTREE / ".copilot" / "skills")
|
skills_dir = (WORKTREE / ".copilot" / "skills")
|
||||||
if skills_dir.is_dir():
|
if skills_dir.is_dir():
|
||||||
@@ -647,38 +648,8 @@ def validate_copilot(report: Report) -> None:
|
|||||||
remediation="Regenerate via `make generate HARNESS=copilot`.",
|
remediation="Regenerate via `make generate HARNESS=copilot`.",
|
||||||
)
|
)
|
||||||
continue
|
continue
|
||||||
if "name" not in fm:
|
_check_nonempty_str_field(report, fm, "name", "copilot", skill_md, label="skill")
|
||||||
report.add(
|
_check_nonempty_str_field(report, fm, "description", "copilot", skill_md, label="skill")
|
||||||
severity="error",
|
|
||||||
harness="copilot",
|
|
||||||
path=skill_md,
|
|
||||||
message="missing required `name` field in frontmatter",
|
|
||||||
remediation="Each Copilot skill needs a name.",
|
|
||||||
)
|
|
||||||
elif not isinstance(fm["name"], str) or not fm["name"].strip():
|
|
||||||
report.add(
|
|
||||||
severity="error",
|
|
||||||
harness="copilot",
|
|
||||||
path=skill_md,
|
|
||||||
message="`name` must be a non-empty string",
|
|
||||||
remediation="Set `name` to a non-empty string in the skill frontmatter.",
|
|
||||||
)
|
|
||||||
if "description" not in fm:
|
|
||||||
report.add(
|
|
||||||
severity="error",
|
|
||||||
harness="copilot",
|
|
||||||
path=skill_md,
|
|
||||||
message="missing required `description` field in frontmatter",
|
|
||||||
remediation="Each Copilot skill needs a description.",
|
|
||||||
)
|
|
||||||
elif not isinstance(fm.get("description"), str) or not fm["description"].strip():
|
|
||||||
report.add(
|
|
||||||
severity="error",
|
|
||||||
harness="copilot",
|
|
||||||
path=skill_md,
|
|
||||||
message="`description` must be a non-empty string",
|
|
||||||
remediation="Set `description` to a non-empty string in the skill frontmatter.",
|
|
||||||
)
|
|
||||||
|
|
||||||
commands_dir = WORKTREE / ".copilot" / "commands"
|
commands_dir = WORKTREE / ".copilot" / "commands"
|
||||||
if commands_dir.is_dir():
|
if commands_dir.is_dir():
|
||||||
@@ -719,10 +690,10 @@ def validate_copilot(report: Report) -> None:
|
|||||||
|
|
||||||
_VALIDATORS = {
|
_VALIDATORS = {
|
||||||
"codex": validate_codex,
|
"codex": validate_codex,
|
||||||
"cursor": validate_cursor,
|
|
||||||
"opencode": validate_opencode,
|
|
||||||
"gemini": validate_gemini,
|
|
||||||
"copilot": validate_copilot,
|
"copilot": validate_copilot,
|
||||||
|
"cursor": validate_cursor,
|
||||||
|
"gemini": validate_gemini,
|
||||||
|
"opencode": validate_opencode,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user