Conversation
📝 WalkthroughWalkthroughThe PR introduces a skills validation framework via a bash script and Go test suite, updates CI workflows with concurrency policies, modifies skill documentation to reflect architectural changes toward service-backed routes and route-centric health checks, and applies JSON parsing improvements and error handling consistency across test and command files. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
Makefile (1)
34-35: Optional hardening: invoke script viabashfor portability.Calling
bash ./scripts/validate-skills.shavoids failures if executable permissions drift.Suggested tweak
validate-skills: - ./scripts/validate-skills.sh + bash ./scripts/validate-skills.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 34 - 35, The Makefile target validate-skills calls the script directly which can fail if executable bits change; update the target so it invokes the script through the shell (e.g., use bash to run ./scripts/validate-skills.sh) to improve portability and avoid relying on file execute permissions—locate the validate-skills target and replace the direct execution with an explicit bash invocation referencing ./scripts/validate-skills.sh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/testing-strategy.md`:
- Around line 71-74: Update the sentence that claims CI provides
A7_GATEWAY_GROUP via repository secrets to reflect the actual workflow: state
that A7_GATEWAY_GROUP is set to the fixed value "default" in the CI workflow
(rather than supplied as a secret), and mention that A7_ADMIN_URL and A7_TOKEN
continue to come from repository secrets; reference the A7_GATEWAY_GROUP symbol
so reviewers can find and correct the wording in testing-strategy.md.
In `@scripts/validate-skills.sh`:
- Around line 63-66: The script validate-skills.sh currently only checks for the
presence of the "description:" key in the frontmatter (variable frontmatter) but
not that it contains non-empty text; update the validation to extract the
description value(s) after the "description:" key (including multi-line/folded
YAML blocks) and verify they contain non-whitespace characters, and if empty set
status=1 and print the existing error `${skill_name}: missing required
frontmatter field: description` (or a slightly adjusted message indicating empty
description). Locate the logic that greps for '^description:' and replace or
augment it with a check that reads the description block from frontmatter and
fails when it is blank, using the same variables frontmatter, skill_name, and
status to report errors.
In `@skills/a7-recipe-api-versioning/SKILL.md`:
- Around line 16-17: The frontmatter command examples currently list "a7 service
create" and "a7 service update" but the recipe steps use "a7 upstream create"
(and "a7 upstream update"); make them consistent by either changing the
frontmatter entries to "a7 upstream create" and "a7 upstream update" or by
updating the recipe steps to use "a7 service create" and "a7 service update"
everywhere; update any examples, flags, and usage text so the command names (a7
service create, a7 service update, a7 upstream create, a7 upstream update) match
across the document and run a quick search to remove any remaining mismatches.
In `@skills/a7-recipe-blue-green/SKILL.md`:
- Around line 15-16: Update the frontmatter command list to match the actual
commands used in the recipe: replace or add the documented "a7 upstream create"
and any other upstream-related commands so the frontmatter isn't limited to "a7
service create" / "a7 service update"; search the SKILL.md content for
occurrences of "a7 upstream create" (and the related upstream commands
referenced in the body) and ensure those exact command strings appear in the
frontmatter commands block to keep documentation consistent.
In `@skills/a7-recipe-canary/SKILL.md`:
- Line 15: The frontmatter command 'a7 service create' is inconsistent with the
recipe steps that call 'a7 upstream create' and 'a7 upstream update'; update the
metadata to match the documented flow by replacing 'a7 service create' with 'a7
upstream create' (and ensure any related metadata reflects 'a7 upstream update'
where applicable), and verify the occurrences referenced by the reviewer (the
recipe steps that call 'a7 upstream create' / 'a7 upstream update') are
consistent with the updated frontmatter.
In `@skills/a7-recipe-health-check/SKILL.md`:
- Around line 195-202: The verification step incorrectly queries a service that
was never created; replace the service-level command(s) "a7 service get backend"
with the upstream equivalent "a7 upstream get backend" (and any other
occurrences of "a7 service get backend") so the flow checks the upstreams
actually created earlier (backend, backend-passive, production-backend), and
ensure the paired route check ("a7 route list --gateway-group default --output
json") remains unchanged; update the duplicate occurrence later in the file as
well.
In `@skills/a7-recipe-mtls/SKILL.md`:
- Around line 20-21: The frontmatter metadata.a7_commands lists "a7 service ..."
but the recipe body still uses "a7 upstream ..." — update the recipe body to
match the metadata by replacing all occurrences of the legacy "a7 upstream"
command (e.g., the steps that run "a7 upstream create" / "a7 upstream update")
with the corresponding "a7 service create" / "a7 service update" invocations so
the instructions and metadata.a7_commands stay in sync; ensure any example flags
or positional arguments remain identical after the rename.
In `@test/e2e/skills/skills_test.go`:
- Around line 55-63: The current frontmatter parsing stores block-scalar markers
like ">-" or "|" as the description value (via the strings.Cut handling that
populates the fields map), causing false positives; implement a helper
hasNonEmptyDescription(lines []string, startIdx int, fields map[string]string)
that recognizes when fields["description"] begins with '>' or '|' and then scans
the subsequent lines until the next key (or end) to ensure at least one
non-whitespace content line exists, and replace the simple non-empty check (used
around lines checking fields["description"]) with a call to
hasNonEmptyDescription; keep existing behavior for single-line descriptions and
ensure you reference the same fields map and parsing flow that uses strings.Cut.
---
Nitpick comments:
In `@Makefile`:
- Around line 34-35: The Makefile target validate-skills calls the script
directly which can fail if executable bits change; update the target so it
invokes the script through the shell (e.g., use bash to run
./scripts/validate-skills.sh) to improve portability and avoid relying on file
execute permissions—locate the validate-skills target and replace the direct
execution with an explicit bash invocation referencing
./scripts/validate-skills.sh.
🪄 Autofix (Beta)
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
Run ID: 756b61f4-f4a3-490e-afa5-2f7cee8176f2
📒 Files selected for processing (14)
.github/workflows/e2e.ymlMakefiledocs/skills.mddocs/testing-strategy.mdscripts/validate-skills.shskills/a7-persona-developer/SKILL.mdskills/a7-persona-operator/SKILL.mdskills/a7-recipe-api-versioning/SKILL.mdskills/a7-recipe-blue-green/SKILL.mdskills/a7-recipe-canary/SKILL.mdskills/a7-recipe-graphql-proxy/SKILL.mdskills/a7-recipe-health-check/SKILL.mdskills/a7-recipe-mtls/SKILL.mdtest/e2e/skills/skills_test.go
| - a7 service create | ||
| - a7 service update |
There was a problem hiding this comment.
Frontmatter command inventory is out of sync with the recipe body.
metadata.a7_commands now lists a7 service ..., but the recipe still instructs a7 upstream ... (e.g., Line 114 and Line 172). Please align these so the skill metadata reflects the actual workflow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/a7-recipe-mtls/SKILL.md` around lines 20 - 21, The frontmatter
metadata.a7_commands lists "a7 service ..." but the recipe body still uses "a7
upstream ..." — update the recipe body to match the metadata by replacing all
occurrences of the legacy "a7 upstream" command (e.g., the steps that run "a7
upstream create" / "a7 upstream update") with the corresponding "a7 service
create" / "a7 service update" invocations so the instructions and
metadata.a7_commands stay in sync; ensure any example flags or positional
arguments remain identical after the rename.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/e2e/skills/skills_test.go (1)
92-117:⚠️ Potential issue | 🟠 MajorHandle block-scalar
descriptionvalues explicitly.
description: >-/description: |is recorded here as a non-empty literal marker, so the check at Lines 146-147 passes even when the actual description body is blank. This still produces false positives for valid YAML frontmatter.
🧹 Nitpick comments (2)
test/e2e/skills/skills_test.go (1)
222-241: Make the docs inventory check data-driven.This only guards against the six names hard-coded today. The next skill rename or removal under
skills/*will slip through until someone updates this list by hand, which is weaker than the “current inventory” validation this PR is trying to add.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/skills/skills_test.go` around lines 222 - 241, Replace the hard-coded staleSkillNames in TestSkillsDocumentationReferencesExistingSkills with a data-driven inventory: read the current skills directory (e.g., using os.ReadDir or filepath.Glob on filepath.Join(root, "skills")) to build a set of existing skill directory names, then scan the docs/skills.md content (variable doc) and fail if it references any skill name not present in that set; update the test logic that currently iterates over staleSkillNames to instead iterate over the derived inventory or compute the difference between referenced names and existing names and call t.Fatalf with those missing entries..github/workflows/ci.yml (1)
46-48: Pingolangci-lintto a specific version for reproducible CI.Using
version: latestintroduces non-determinism; a new upstream release can cause lint failures without any change to your code. Replace with a pinned version (e.g.,version: v1.60.3) and upgrade intentionally when needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 46 - 48, Replace the non-deterministic `version: latest` for the GitHub Action `golangci/golangci-lint-action` with a pinned release tag (for example `version: v1.60.3`) so CI uses a fixed linter version; update the `version` field in the workflow entry that references `golangci/golangci-lint-action` to the chosen tag and bump it intentionally in future changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/skills/skills_test.go`:
- Around line 39-53: The temp directory cleanup is skipped because os.Exit
bypasses deferred calls; replace the current pattern by capturing exit codes and
performing explicit cleanup of tmpDir before calling os.Exit. Concretely: remove
reliance on defer os.RemoveAll(tmpDir), after creating tmpDir set a7Binary and
run the build (using cmd and cmd.Run()), track a non-zero exitCode on errors
(e.g., build error -> exitCode = 1), call os.RemoveAll(tmpDir) unconditionally
before calling os.Exit(exitCode), and for the test run assign exitCode = m.Run()
then cleanup and os.Exit(exitCode).
- Around line 164-173: The test is over-parsing a7_commands entries with
strings.Fields (in the loop over metadata.A7Commands) which breaks
quoted/escaped args; instead only validate the entry starts with the subcommand
prefix and avoid tokenizing with strings.Fields: check that the trimmed command
equals "a7" or has the prefix "a7 " (use strings.HasPrefix/strings.TrimSpace
against the raw command string), then build the exec invocation without relying
on Fields (e.g., run a7Binary with "--help" or invoke the command string via a
shell if you need full parsing) so the test no longer mis-parses quoted/escaped
arguments when calling exec.Command(a7Binary, ...).
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 46-48: Replace the non-deterministic `version: latest` for the
GitHub Action `golangci/golangci-lint-action` with a pinned release tag (for
example `version: v1.60.3`) so CI uses a fixed linter version; update the
`version` field in the workflow entry that references
`golangci/golangci-lint-action` to the chosen tag and bump it intentionally in
future changes.
In `@test/e2e/skills/skills_test.go`:
- Around line 222-241: Replace the hard-coded staleSkillNames in
TestSkillsDocumentationReferencesExistingSkills with a data-driven inventory:
read the current skills directory (e.g., using os.ReadDir or filepath.Glob on
filepath.Join(root, "skills")) to build a set of existing skill directory names,
then scan the docs/skills.md content (variable doc) and fail if it references
any skill name not present in that set; update the test logic that currently
iterates over staleSkillNames to instead iterate over the derived inventory or
compute the difference between referenced names and existing names and call
t.Fatalf with those missing entries.
🪄 Autofix (Beta)
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
Run ID: 126446cc-65f2-4fdf-9f12-c211b4fb7291
📒 Files selected for processing (52)
.github/workflows/ci.yml.github/workflows/e2e.yml.github/workflows/skills.ymlMakefilepkg/api/client_test.gopkg/cmd/consumer-group/create/create_test.gopkg/cmd/consumer-group/get/get_test.gopkg/cmd/consumer-group/list/list_test.gopkg/cmd/consumer-group/update/update_test.gopkg/cmd/consumer/create/create.gopkg/cmd/consumer/delete/delete.gopkg/cmd/consumer/get/get.gopkg/cmd/consumer/list/list.gopkg/cmd/consumer/update/update.gopkg/cmd/credential/create/create_test.gopkg/cmd/credential/get/get_test.gopkg/cmd/credential/list/list_test.gopkg/cmd/credential/update/update_test.gopkg/cmd/global-rule/get/get_test.gopkg/cmd/global-rule/list/list_test.gopkg/cmd/plugin-config/create/create_test.gopkg/cmd/plugin-config/get/get_test.gopkg/cmd/plugin-config/list/list_test.gopkg/cmd/plugin-config/update/update_test.gopkg/cmd/plugin-metadata/create/create_test.gopkg/cmd/plugin-metadata/get/get_test.gopkg/cmd/plugin-metadata/update/update_test.gopkg/cmd/plugin/get/get.gopkg/cmd/plugin/get/get_test.gopkg/cmd/plugin/list/list.gopkg/cmd/proto/create/create_test.gopkg/cmd/proto/get/get_test.gopkg/cmd/proto/list/list_test.gopkg/cmd/proto/update/update_test.gopkg/cmd/secret/create/create_test.gopkg/cmd/secret/get/get_test.gopkg/cmd/secret/list/list_test.gopkg/cmd/secret/update/update_test.gopkg/cmd/service/create/create_test.gopkg/cmd/service/get/get_test.gopkg/cmd/service/list/list_test.gopkg/cmd/service/update/update_test.gopkg/cmd/ssl/create/create.gopkg/cmd/ssl/delete/delete.gopkg/cmd/ssl/get/get.gopkg/cmd/ssl/list/list.gopkg/cmd/ssl/update/update.gopkg/cmd/stream-route/create/create_test.gopkg/cmd/stream-route/get/get_test.gopkg/cmd/stream-route/list/list_test.gopkg/cmd/stream-route/update/update_test.gotest/e2e/skills/skills_test.go
✅ Files skipped from review due to trivial changes (42)
- pkg/cmd/proto/update/update_test.go
- pkg/cmd/plugin-metadata/create/create_test.go
- pkg/cmd/consumer-group/get/get_test.go
- pkg/cmd/consumer-group/create/create_test.go
- pkg/cmd/stream-route/get/get_test.go
- pkg/cmd/consumer-group/list/list_test.go
- pkg/cmd/credential/get/get_test.go
- pkg/cmd/secret/list/list_test.go
- .github/workflows/e2e.yml
- pkg/cmd/proto/get/get_test.go
- pkg/cmd/proto/create/create_test.go
- pkg/cmd/service/list/list_test.go
- pkg/cmd/ssl/get/get.go
- pkg/cmd/global-rule/list/list_test.go
- pkg/cmd/stream-route/list/list_test.go
- pkg/cmd/secret/get/get_test.go
- pkg/cmd/plugin-config/list/list_test.go
- pkg/cmd/consumer/update/update.go
- pkg/cmd/plugin-metadata/get/get_test.go
- pkg/cmd/proto/list/list_test.go
- pkg/cmd/plugin/get/get_test.go
- pkg/cmd/credential/create/create_test.go
- pkg/cmd/plugin-config/get/get_test.go
- pkg/cmd/service/update/update_test.go
- pkg/cmd/plugin-config/create/create_test.go
- pkg/cmd/service/create/create_test.go
- pkg/cmd/consumer-group/update/update_test.go
- pkg/cmd/credential/list/list_test.go
- pkg/cmd/secret/update/update_test.go
- pkg/cmd/global-rule/get/get_test.go
- pkg/cmd/plugin-metadata/update/update_test.go
- pkg/cmd/stream-route/update/update_test.go
- pkg/cmd/consumer/get/get.go
- pkg/cmd/consumer/create/create.go
- pkg/cmd/plugin-config/update/update_test.go
- pkg/cmd/stream-route/create/create_test.go
- pkg/cmd/credential/update/update_test.go
- pkg/cmd/ssl/create/create.go
- pkg/cmd/plugin/list/list.go
- pkg/cmd/service/get/get_test.go
- pkg/cmd/consumer/delete/delete.go
- pkg/cmd/secret/create/create_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
There was a problem hiding this comment.
♻️ Duplicate comments (1)
skills/a7-recipe-health-check/SKILL.md (1)
325-325:⚠️ Potential issue | 🟡 MinorInconsistency: Troubleshooting still references
a7 service get.The verification step (line 199) was correctly updated to use
a7 upstream get, but this troubleshooting entry still suggests usinga7 service get. This should be aligned with the rest of the document.Suggested fix
-| No standalone health command | Current a7 does not expose upstream health status | Verify service/route config with `a7 service get` and use gateway observability | +| No standalone health command | Current a7 does not expose upstream health status | Verify upstream/route config with `a7 upstream get` and use gateway observability |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/a7-recipe-health-check/SKILL.md` at line 325, Update the troubleshooting table entry that currently reads "Verify service/route config with `a7 service get` and use gateway observability" so it references the upstream command used elsewhere; replace `a7 service get` with `a7 upstream get` to match the verification step (the table cell containing "No standalone health command | Current a7 does not expose upstream health status | Verify service/route config with `a7 service get` and use gateway observability").
🧹 Nitpick comments (2)
test/e2e/skills/skills_test.go (1)
239-253: Minor: Consider refactoring nested loop to glob once.The current implementation calls
filepath.Globinside the loop for each disallowed pattern, re-reading the same file list. This is a minor inefficiency.Suggested refactor
func TestSkillsDoNotReferenceRemovedA7Commands(t *testing.T) { root := repoRoot(t) disallowed := []string{ "a7 health", "a7 portal", "a7 upstream health", "a7 consumer-restriction create", } - for _, pattern := range disallowed { - matches, err := filepath.Glob(filepath.Join(root, "skills", "*", "SKILL.md")) - if err != nil { - t.Fatal(err) - } - for _, file := range matches { - data, err := os.ReadFile(file) - if err != nil { - t.Fatal(err) - } + matches, err := filepath.Glob(filepath.Join(root, "skills", "*", "SKILL.md")) + if err != nil { + t.Fatal(err) + } + for _, file := range matches { + data, err := os.ReadFile(file) + if err != nil { + t.Fatal(err) + } + content := string(data) + for _, pattern := range disallowed { - if strings.Contains(string(data), pattern) { + if strings.Contains(content, pattern) { t.Fatalf("%s: references removed or unsupported command %q", file, pattern) } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/skills/skills_test.go` around lines 239 - 253, Refactor the nested loops so you call filepath.Glob once and reuse the resulting matches instead of calling it inside the disallowed-pattern loop: move the call to filepath.Glob(filepath.Join(root, "skills", "*", "SKILL.md")) (currently inside the for _, pattern := range disallowed loop) to run once before iterating disallowed, store the result in matches, then iterate over disallowed and for each pattern iterate over matches, reading files with os.ReadFile and checking strings.Contains as before (preserve existing t.Fatal/t.Fatalf error handling).scripts/validate-skills.sh (1)
42-42: Consider edge case:namefield with inline comment.The awk extraction doesn't strip inline YAML comments. A frontmatter line like
name: my-skill # commentwould include# commentin the extracted value.Suggested fix
- name="$(printf '%s\n' "${frontmatter}" | awk -F': *' '$1 == "name" { print $2; exit }' | tr -d '"'"'"'')" + name="$(printf '%s\n' "${frontmatter}" | awk -F': *' '$1 == "name" { sub(/ *#.*$/, "", $2); print $2; exit }' | tr -d '"'"'"'')"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate-skills.sh` at line 42, The current extraction of the frontmatter "name" into the variable name can include inline YAML comments (e.g., "name: foo # comment"); update the pipeline that sets name (the line assigning name from frontmatter) to strip any inline comments and trailing/leading whitespace after awk extracts the value — for example, pipe the awk output through a command that removes everything from a "#" onward and then trim whitespace (or use an awk expression that splits on "#" or uses gsub to remove comments) so name contains only the actual value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@skills/a7-recipe-health-check/SKILL.md`:
- Line 325: Update the troubleshooting table entry that currently reads "Verify
service/route config with `a7 service get` and use gateway observability" so it
references the upstream command used elsewhere; replace `a7 service get` with
`a7 upstream get` to match the verification step (the table cell containing "No
standalone health command | Current a7 does not expose upstream health status |
Verify service/route config with `a7 service get` and use gateway
observability").
---
Nitpick comments:
In `@scripts/validate-skills.sh`:
- Line 42: The current extraction of the frontmatter "name" into the variable
name can include inline YAML comments (e.g., "name: foo # comment"); update the
pipeline that sets name (the line assigning name from frontmatter) to strip any
inline comments and trailing/leading whitespace after awk extracts the value —
for example, pipe the awk output through a command that removes everything from
a "#" onward and then trim whitespace (or use an awk expression that splits on
"#" or uses gsub to remove comments) so name contains only the actual value.
In `@test/e2e/skills/skills_test.go`:
- Around line 239-253: Refactor the nested loops so you call filepath.Glob once
and reuse the resulting matches instead of calling it inside the
disallowed-pattern loop: move the call to filepath.Glob(filepath.Join(root,
"skills", "*", "SKILL.md")) (currently inside the for _, pattern := range
disallowed loop) to run once before iterating disallowed, store the result in
matches, then iterate over disallowed and for each pattern iterate over matches,
reading files with os.ReadFile and checking strings.Contains as before (preserve
existing t.Fatal/t.Fatalf error handling).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29d613f9-6f33-4976-91f0-b5a690a8b830
📒 Files selected for processing (9)
.github/workflows/ci.ymlMakefiledocs/testing-strategy.mdscripts/validate-skills.shskills/a7-recipe-blue-green/SKILL.mdskills/a7-recipe-canary/SKILL.mdskills/a7-recipe-health-check/SKILL.mdskills/a7-recipe-mtls/SKILL.mdtest/e2e/skills/skills_test.go
💤 Files with no reviewable changes (2)
- skills/a7-recipe-mtls/SKILL.md
- skills/a7-recipe-blue-green/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- skills/a7-recipe-canary/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- .github/workflows/ci.yml
Summary
Validation
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores