fix: validate and correct AI agent skill examples - #73
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesSkill documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SkillDocs
participant SkillsTest
participant A7CLI
SkillDocs->>SkillsTest: provide fenced shell examples
SkillsTest->>SkillsTest: tokenize and resolve commands
SkillsTest->>A7CLI: request command help
A7CLI-->>SkillsTest: return commands and flags
SkillsTest-->>SkillDocs: report validation errors
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04835b138d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/a7-plugin-jwt-auth/SKILL.md`:
- Around line 148-153: Update the RS256 setup instructions around the credential
creation command to ensure consumer bob exists first: either add the consumer
creation step before a7 credential create or associate the credential with an
already existing consumer. Preserve the --consumer bob association when using
the newly created consumer.
In `@test/e2e/skills/skills_test.go`:
- Around line 221-227: Update the skill-file validation setup before the loop
over matches to fail the test when filepath.Glob returns an empty slice. Keep
the existing filepath.Glob error handling and per-file validation unchanged.
- Around line 285-303: Update resolveCommand to reject an unknown non-flag,
non-argument token when availableCommands(help) advertises nested subcommands,
instead of silently treating it as positional. Preserve handling for flags and
valid positional arguments, and add a regression case covering a misspelled
nested command such as a7 global-rule crte.
- Around line 240-245: Update the flag validation loop using longFlagPattern and
validHelp so it parses complete flag tokens from the help output, builds exact
flag-name membership, and checks each discovered flag against that set instead
of using strings.Contains. Preserve the existing --help exclusion and
unsupported-flag failure behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 74c924ec-e595-4f58-b4f3-62c819a70253
📒 Files selected for processing (13)
docs/skills.mdskills/a7-persona-developer/SKILL.mdskills/a7-persona-operator/SKILL.mdskills/a7-plugin-basic-auth/SKILL.mdskills/a7-plugin-hmac-auth/SKILL.mdskills/a7-plugin-http-logger/SKILL.mdskills/a7-plugin-jwt-auth/SKILL.mdskills/a7-plugin-kafka-logger/SKILL.mdskills/a7-plugin-key-auth/SKILL.mdskills/a7-plugin-prometheus/SKILL.mdskills/a7-plugin-skywalking/SKILL.mdskills/a7-plugin-zipkin/SKILL.mdtest/e2e/skills/skills_test.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60dc7c1a32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1f0ebb004
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/skills/skills_test.go (1)
18-19: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck only real CLI flags for shell blocks.
longFlagPatternruns over the joined shell line, soa7 init --name="text --nested"and comment text containing--commentare reported as unsupported flags. Parse flags from fields/subfields first, then handle=valueassignments, before comparing against valid flags.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/skills/skills_test.go` around lines 18 - 19, Update the shell-block flag validation around longFlagPattern to tokenize each parsed shell field and its subfields before matching long options, rather than scanning the joined command text. Strip or separately handle =value assignments before comparing flags against the valid-flag set, so quoted values and comments containing --text are not reported as CLI flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/a7-plugin-jwt-auth/SKILL.md`:
- Around line 157-168: Clarify the `bob-rs256-credential.yaml` instructions so
the `public_key` value is unambiguous: either instruct readers to paste only the
base64 text between the `public.pem` delimiters while retaining the template
delimiters, or remove the template delimiters and instruct them to paste the
complete PEM block. Ensure the final guidance cannot produce duplicated PEM
headers or footers.
---
Outside diff comments:
In `@test/e2e/skills/skills_test.go`:
- Around line 18-19: Update the shell-block flag validation around
longFlagPattern to tokenize each parsed shell field and its subfields before
matching long options, rather than scanning the joined command text. Strip or
separately handle =value assignments before comparing flags against the
valid-flag set, so quoted values and comments containing --text are not reported
as CLI flags.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ded68dc-6061-4d06-b698-f217a2173616
📒 Files selected for processing (6)
docs/user-guide/gateway-group.mdskills/a7-persona-developer/SKILL.mdskills/a7-persona-operator/SKILL.mdskills/a7-plugin-jwt-auth/SKILL.mdskills/a7-recipe-multi-tenant/SKILL.mdtest/e2e/skills/skills_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/a7-persona-developer/SKILL.md
- skills/a7-persona-operator/SKILL.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be9c0b3036
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69a08a2ffd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/e2e/skills/skills_test.go (3)
409-452: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache
commandHelpresults.
resolveCommandcallscommandHelpfor the first token and again for every resolved subcommand. The test runs this for everya7invocation in everySKILL.md. Each call starts the builta7binary, so the same help output is produced many times. Add a memo keyed by the joined command path.♻️ Proposed refactor
+var ( + helpCacheMu sync.Mutex + helpCache = map[string]string{} +) + +func cachedCommandHelp(t *testing.T, path []string) string { + t.Helper() + key := strings.Join(path, " ") + helpCacheMu.Lock() + defer helpCacheMu.Unlock() + if help, ok := helpCache[key]; ok { + return help + } + help := commandHelp(t, path) + helpCache[key] = help + return help +}Then replace the
commandHelp(t, path)calls inresolveCommandwithcachedCommandHelp(t, path).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/skills/skills_test.go` around lines 409 - 452, Cache command help output by joined command path: add a memoized cachedCommandHelp helper and replace both commandHelp calls in resolveCommand, including the initial path and each resolved subcommand, while preserving existing help lookup behavior.
302-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the global value flags from the command tree.
a7GlobalValueFlagshardcodes the persistent flags thatrootcmd.NewCmddeclares.commandFieldsalso builds the map again on every call. If a persistent flag is added or renamed inpkg/cmd/root/root.go, this map goes stale and the test silently mis-parses examples.Build the set once from
newA7CommandTree().PersistentFlags()and pass it tocommandFields.♻️ Proposed refactor
-func commandFields(fields []string) []string { - valueFlags := a7GlobalValueFlags() +func commandFields(fields []string, valueFlags map[string]bool) []string { for len(fields) > 0 && strings.HasPrefix(fields[0], "-") { flag := strings.SplitN(fields[0], "=", 2)[0] hasInlineValue := strings.Contains(fields[0], "=") fields = fields[1:] if valueFlags[flag] && !hasInlineValue && len(fields) > 0 { fields = fields[1:] } } return fields } -func a7GlobalValueFlags() map[string]bool { - return map[string]bool{ - "--gateway-group": true, - "--output": true, - "--server": true, - "--token": true, - "-g": true, - "-o": true, - } -} +func a7GlobalValueFlags(root *cobra.Command) map[string]bool { + valueFlags := map[string]bool{} + root.PersistentFlags().VisitAll(func(flag *pflag.Flag) { + if flag.NoOptDefVal != "" { + return + } + valueFlags["--"+flag.Name] = true + if flag.Shorthand != "" { + valueFlags["-"+flag.Shorthand] = true + } + }) + return valueFlags +}Update the two call sites at Line 230 and Line 280 to pass
commandTree.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/skills/skills_test.go` around lines 302 - 324, Replace the hardcoded a7GlobalValueFlags map with a value-flag set derived once from newA7CommandTree().PersistentFlags(), and update commandFields to accept that set instead of constructing it internally. Adjust both commandFields call sites around the existing test flows to pass the command tree-derived flags, keeping argument parsing behavior unchanged.
284-292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping the help-text flag scan.
longFlagPattern.FindAllString(invocation, -1)scans the raw invocation string, including quoted values and text after a--separator. It then validates against a set parsed from help output, where a flag name inside a description also becomes "valid".positionalArgsalready resolves each flag throughlookupFlagandlookupShorthandFlagon the exact Cobra command, which is stricter and token-aware.Remove this block and rely on
validatePositionalArgs, or keep it only for the shorthand cases thatpositionalArgsdoes not reach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/skills/skills_test.go` around lines 284 - 292, Remove the longFlagPattern-based validation loop that scans raw invocation text and relies on help-derived validFlags. Use validatePositionalArgs and its token-aware lookupFlag/lookupShorthandFlag validation as the sole long-flag validation path, preserving shorthand validation only if those cases are not covered there.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/e2e/skills/skills_test.go`:
- Around line 409-452: Cache command help output by joined command path: add a
memoized cachedCommandHelp helper and replace both commandHelp calls in
resolveCommand, including the initial path and each resolved subcommand, while
preserving existing help lookup behavior.
- Around line 302-324: Replace the hardcoded a7GlobalValueFlags map with a
value-flag set derived once from newA7CommandTree().PersistentFlags(), and
update commandFields to accept that set instead of constructing it internally.
Adjust both commandFields call sites around the existing test flows to pass the
command tree-derived flags, keeping argument parsing behavior unchanged.
- Around line 284-292: Remove the longFlagPattern-based validation loop that
scans raw invocation text and relies on help-derived validFlags. Use
validatePositionalArgs and its token-aware lookupFlag/lookupShorthandFlag
validation as the sole long-flag validation path, preserving shorthand
validation only if those cases are not covered there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2aa63aab-fdd6-44ec-84f5-17bd2afd5d66
📒 Files selected for processing (8)
skills/a7-plugin-http-logger/SKILL.mdskills/a7-plugin-jwt-auth/SKILL.mdskills/a7-plugin-kafka-logger/SKILL.mdskills/a7-plugin-prometheus/SKILL.mdskills/a7-plugin-skywalking/SKILL.mdskills/a7-plugin-zipkin/SKILL.mdskills/a7-recipe-multi-tenant/SKILL.mdtest/e2e/skills/skills_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- skills/a7-plugin-skywalking/SKILL.md
- skills/a7-plugin-kafka-logger/SKILL.md
- skills/a7-plugin-prometheus/SKILL.md
- skills/a7-plugin-http-logger/SKILL.md
1 participant
Summary
Why
The Skills Hub audit found that the existing validation could pass while skill bodies still referenced removed commands, flags, malformed credential requests, and outdated resource relationships. Agents may execute these examples directly, so command examples need validation against the released CLI surface.
Impact
This is a documentation and test change. It does not alter a7 runtime behavior. The extended test fails when a shell example uses an unknown command or long flag.
Validation
make validate-skillsmake test-skillsFollow-up
After merge, publish a reviewed skill-bearing release so
api7/docscan pin the redesigned Skills Hub catalog to this corrected artifact.Summary by CodeRabbit
Documentation
Tests