feat: add canonical AGH bundled skill

[pedronauck]

Summary

• Replaces the split AGH bundled skills with one canonical root-level agh skill installable from skills/agh.
• Adds reference-driven AGH guidance, including native tool usage for agents running inside AGH.
• Rewires daemon prompt sections, native/CLI skill view, review routing, docs, tests, and the active skills RFC to use the single bundled skill without old-name aliases.

Changes

• Bundled skill source: Adds skills/agh/SKILL.md, reference documents, and the root skills Go package that embeds bundled skill content/resources.
• Runtime wiring: Loads contextual AGH references through daemon startup prompt sections and review overlays; removes internal/skills/bundled and old agh-* bundled skill names.
• Native tools and CLI: Extends skill resource loading so agh__skill_view and CLI skill viewing can read bundled reference files such as references/native-tools.md.
• Task review authority: Keeps submit_run_review availability tied to runtime task review binding instead of skill metadata.
• Docs, RFC, and tests: Updates runtime skill/review docs, site assertions, and docs/rfcs/001_agent-md-with-skills-memory.md to document agh as the sole bundled skill.

Release Notes

🎉 Features

• Add a canonical agh bundled skill with reference documents for runtime operations, native tools, network work, tasks, memory, QA, docs, and repository contribution.

♻️ Refactoring

• Replace the old split agh-* bundled skills with one root skills/agh bundle and reference-based contextual loading.

📚 Documentation

• Update runtime skills, review-gate docs, and the skills/memory RFC for the new bundled skill, skill_view --file, and native tool guidance.

Test plan

• python3 .agents/skills/agh/agh-test-conventions/scripts/check-test-conventions.py skills/bundled_test.go
• go test ./skills ./internal/skills ./internal/daemon ./internal/cli ./internal/session ./internal/api/core ./internal/config ./internal/task ./internal/store/globaldb ./internal/testutil/acpmock
• bunx turbo run test --filter=./packages/site
• make verify before the feature commit
• make verify after the feature commit
• make verify before the RFC follow-up commit
• make verify after the RFC follow-up commit
• Peer review round 2 returned SHIP

Notes

• skeeper sync --dry-run reported it would push 5547 specs, so it was not applied in this PR to avoid mixing unrelated spec churn with the skill migration.

Summary by CodeRabbit

• New Features
  • Skills can load individual resource files by path and the skill view supports an optional file parameter to show specific resource content.
• Chores
  • Bundled skill packaging consolidated under "agh"; bundled references and startup prompt fragments updated.
• Tests
  • Many tests adjusted or added to reflect the new bundled layout and resource-loading behavior.
• Chores
  • Registry and resource handling expanded to support resource reads and canonical skill resource representation.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agh-site	Ready	Preview, Comment	May 12, 2026 11:42pm

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Consolidates bundled skills into a single "agh" skill and adds resource-level loading. Introduces a top-level skills package (LoadContent, LoadResource), a SkillsRegistry.LoadResource method, canonical skill resource spec, filesystem/resource helpers, and updates consumers/tests to use resource-based APIs. (49 words)

Changes

Bundled skills consolidation and resource-based loading

Layer / File(s)	Summary
Embedded bundled FS and public loader skills/embed.go, skills/content.go, skills/bundled_test.go	Adds top-level skills package embedding agh and implements LoadContent(name) and LoadResource(name, relativePath) with validation, path sanitization, frontmatter stripping, sentinel errors, and unit tests for content and resource loading.
Skills registry surface and test stub internal/api/core/interfaces.go, internal/api/testutil/skills_stub.go, internal/skills/registry.go	Adds SkillsRegistry.LoadResource(ctx, skill, relativePath) to the API, implements (*Registry).LoadResource dispatching by Skill.Source (bundled vs FS) and validating inputs, and extends the stub registry with LoadResourceFn.
Filesystem-backed resource reading and sanitization internal/skills/resource.go	Introduces ReadSkillResourceContent, readBundledSkillResource, and cleanSkillResourcePath to normalize/validate relative paths, prevent traversal/backslashes/absolute paths, enforce containment within skill directories, and wrap read errors. Removes prior resource-spec codec logic from this file.
Canonical persisted skill resource spec and conversions internal/skills/resource_spec.go	Adds SkillResourceSpec type and SkillResourceKind, NewResourceCodec() with validation, SkillToResourceSpec/SkillFromResourceSpec conversions, and validation/normalization for MCP server decls, hooks, and provenance.
Consumer wiring and prompt assembly changes internal/daemon/prompt_sections.go, internal/daemon/composed_assembler_test.go, internal/daemon/harness_context_integration_test.go, internal/daemon/boot.go	Replaces single-skill content loading with bundledReferencesPromptSectionProvider that calls skillbundled.LoadResource for multiple reference files (tools/native-tools/network), trims/concatenates fragments, and updates daemon config to use skillbundled.FS(). Tests updated to load and assert trimmed resources.
Native tools / CLI tool changes internal/daemon/native_tools.go, internal/tools/builtin/skills.go, internal/cli/skill_commands.go, internal/cli/skill_workspace.go, internal/cli/skill_test.go, internal/daemon/native_tools_test.go	Extends skillView input with optional file field and uses Skills.LoadResource when present; updates tool descriptor schema and CLI/tests to reference the consolidated agh skill and resource paths (e.g., references/memory.md).
Review routing and review tooling policy changes internal/daemon/review_router.go, internal/daemon/review_router_test.go, internal/daemon/native_review_tools.go	Removes skill-metadata-based gating for review availability, updates review-router prompt template to reference agh and references/tasks-and-orchestration.md, and simplifies availability logic to depend only on task/deps presence.
Session, manager, and integration tests migration internal/session/manager_test.go, internal/session/manager_integration_test.go, internal/daemon/daemon_test.go, internal/daemon/daemon_integration_test.go, internal/daemon/harness_context_integration_test.go	Updates many tests and constants to load bundled network/tools/native fragments via skillbundled.LoadResource(bundledAghSkillName, <reference>), trim results, and expect "agh" as the bundled skill identifier in prompts and registries.
Removed internal bundled package artifacts internal/skills/bundled/content.go, internal/skills/bundled/bundled_test.go	Removes the internal bundled package's LoadContent implementation, sentinel errors, and its test suite; functionality migrated to the top-level skills package and new tests in skills/*.
Tests, config, docs and small updates multiple test files (e.g., internal/config/*, internal/testutil/*, internal/store/*, internal/task/*, packages/site/lib/__tests__/*, internal/e2elane/command_wiring_test.go)	Adjusts disabled-skills fixtures, prompt expectations, test imports to github.com/pedronauck/agh/skills, updates reviewer agent names in some tests, adds turbo JSON helpers/tests, and aligns docs/tests to new agh + reference-file structure.

sequenceDiagram
  participant CLI as Client/CLI
  participant Daemon as Daemon
  participant Registry as SkillsRegistry
  participant FS as Bundled FS / Embedded FS

  CLI->>Daemon: skill view {name:"agh", file:"references/memory.md"}
  Daemon->>Registry: LoadResource(ctx, skill:"agh", "references/memory.md")
  Registry->>FS: Open "agh/references/memory.md"
  FS-->>Registry: file contents
  Registry-->>Daemon: trimmed contents
  Daemon-->>CLI: HTTP/JSON response with file contents

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• compozy/agh#12: Related refactor of internal/skills/registry.go and registry loading internals that overlap with this PR's addition of SkillsRegistry.LoadResource.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 9.23% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat: add canonical AGH bundled skill' clearly and concisely summarizes the primary change: introducing a single canonical bundled skill named 'agh' to replace multiple previous bundled skills.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/skills/catalog_test.go (1)

183-207: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add subtest parallelization for consistency with test guidelines.

Line 183 introduces a t.Run(...) subtest, but the subtest body does not call t.Parallel(). Add it unless this case is intentionally environment-sensitive.

Suggested patch

 	t.Run("Should exclude disabled skills", func(t *testing.T) {
+		t.Parallel()
+
 		got := BuildCatalog([]*Skill{
 			{

As per coding guidelines: In Go tests, use t.Run("Should ...") subtest pattern and t.Parallel by default (with t.Setenv opt-out for environment-sensitive tests).

🤖 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 `@internal/skills/catalog_test.go` around lines 183 - 207, The "Should exclude
disabled skills" subtest in catalog_test.go is missing t.Parallel(); update the
t.Run subtest body to call t.Parallel() as the first line inside the anonymous
func used by t.Run to enable parallel execution (unless this test is
intentionally environment-sensitive, in which case add a comment and use
t.Setenv to opt out). Ensure the change is applied within the t.Run("Should
exclude disabled skills", func(t *testing.T) { ... }) block surrounding the
BuildCatalog assertions.

🧹 Nitpick comments (3)

internal/skills/resource_spec.go (1)

169-207: ⚡ Quick win

Consider extracting env map cleaning logic.

Lines 180-191 and 193-204 contain nearly identical logic for cleaning Env and SecretEnv maps (trimming keys/values, removing empty keys, nil-ing empty maps). Extracting a helper function like cleanEnvMap(m map[string]string) map[string]string would reduce duplication and improve maintainability.

♻️ Suggested helper function

+func cleanEnvMap(env map[string]string) map[string]string {
+	if len(env) == 0 {
+		return nil
+	}
+	cleaned := make(map[string]string, len(env))
+	for key, value := range env {
+		trimmedKey := strings.TrimSpace(key)
+		if trimmedKey == "" {
+			continue
+		}
+		cleaned[trimmedKey] = strings.TrimSpace(value)
+	}
+	if len(cleaned) == 0 {
+		return nil
+	}
+	return cleaned
+}
+
 func normalizeMCPServerDecl(decl MCPServerDecl) MCPServerDecl {
 	normalized := MCPServerDecl{
 		Name:      strings.TrimSpace(decl.Name),
 		Command:   strings.TrimSpace(decl.Command),
 		Args:      append([]string(nil), decl.Args...),
-		Env:       cloneStringMap(decl.Env),
-		SecretEnv: cloneStringMap(decl.SecretEnv),
+		Env:       cleanEnvMap(decl.Env),
+		SecretEnv: cleanEnvMap(decl.SecretEnv),
 	}
 	for idx := range normalized.Args {
 		normalized.Args[idx] = strings.TrimSpace(normalized.Args[idx])
 	}
-	if len(normalized.Env) > 0 {
-		for key, value := range normalized.Env {
-			trimmedKey := strings.TrimSpace(key)
-			delete(normalized.Env, key)
-			if trimmedKey == "" {
-				continue
-			}
-			normalized.Env[trimmedKey] = strings.TrimSpace(value)
-		}
-		if len(normalized.Env) == 0 {
-			normalized.Env = nil
-		}
-	}
-	if len(normalized.SecretEnv) > 0 {
-		for key, value := range normalized.SecretEnv {
-			trimmedKey := strings.TrimSpace(key)
-			delete(normalized.SecretEnv, key)
-			if trimmedKey == "" {
-				continue
-			}
-			normalized.SecretEnv[trimmedKey] = strings.TrimSpace(value)
-		}
-		if len(normalized.SecretEnv) == 0 {
-			normalized.SecretEnv = nil
-		}
-	}
 	return normalized
 }

🤖 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 `@internal/skills/resource_spec.go` around lines 169 - 207,
normalizeMCPServerDecl contains duplicated logic for cleaning Env and SecretEnv
maps; extract that into a helper like cleanEnvMap(m map[string]string)
map[string]string that: iterates entries, trims keys and values, skips entries
with empty trimmed keys, deletes original keys, and returns nil if the resulting
map is empty (preserving the current nil vs empty behavior). Replace the two
nearly identical blocks in normalizeMCPServerDecl with normalized.Env =
cleanEnvMap(normalized.Env) and normalized.SecretEnv =
cleanEnvMap(normalized.SecretEnv), ensuring behavior and semantics remain
unchanged.

internal/daemon/daemon_test.go (1)

3775-3777: ⚡ Quick win

Make the skill-name assertion more specific.

Line 3775 currently checks plain "agh", which can match unrelated prompt text and mask a skills-rendering regression. Prefer an exact, block-scoped fragment for the listed skill entry.

As per coding guidelines: "MUST test meaningful business logic" and "Ensure tests verify behavior outcomes, not just function calls."

🤖 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 `@internal/daemon/daemon_test.go` around lines 3775 - 3777, The test currently
asserts the plain string "agh" via the fragments slice (variable fragments in
daemon_test.go), which can match unrelated text; change the expectation to a
specific, block-scoped fragment such as
"<available-skills>agh</available-skills>" (or update the appended fragments to
append that exact block instead of separate "<available-skills>" and "agh") so
the test verifies the skill entry as a distinct element rather than a loose
substring.

internal/session/manager_test.go (1)

3204-3209: ⚡ Quick win

Extract a shared helper for bundled resource loading in tests.

Line 3204 repeats the same LoadResource + TrimSpace sequence used again at Line 3281, Line 3310, and Line 3646. A small helper keeps assertions consistent and avoids drift in failure messages.

♻️ Suggested refactor

+func loadTrimmedBundledResource(t *testing.T, relativePath string) string {
+	t.Helper()
+	content, err := skillbundled.LoadResource(testBundledAghSkillName, relativePath)
+	if err != nil {
+		t.Fatalf("LoadResource(%q, %q) error = %v", testBundledAghSkillName, relativePath, err)
+	}
+	return strings.TrimSpace(content)
+}

-	networkSkill, err := skillbundled.LoadResource(testBundledAghSkillName, testBundledNetworkReference)
-	if err != nil {
-		t.Fatalf("LoadResource(%q, %q) error = %v", testBundledAghSkillName, testBundledNetworkReference, err)
-	}
-	networkSkill = strings.TrimSpace(networkSkill)
+	networkSkill := loadTrimmedBundledResource(t, testBundledNetworkReference)

🤖 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 `@internal/session/manager_test.go` around lines 3204 - 3209, Tests repeatedly
call skillbundled.LoadResource(... ) followed by strings.TrimSpace(...) (e.g.,
the calls using LoadResource with testBundledAghSkillName and
testBundledNetworkReference at lines where networkSkill is loaded), causing
duplicated logic and inconsistent assertions; extract a small helper function
(e.g., loadBundledResourceOrFatal(t, name, ref) used in manager_test.go tests)
that wraps skillbundled.LoadResource, checks err and calls t.Fatalf with a
consistent message, trims the result with strings.TrimSpace, and return the
string, then replace all repeated LoadResource+TrimSpace sequences (including
the occurrences referring to
testBundledAghSkillName/testBundledNetworkReference) with calls to that helper
to keep failure messages and behavior consistent.

🤖 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.

Outside diff comments:
In `@internal/skills/catalog_test.go`:
- Around line 183-207: The "Should exclude disabled skills" subtest in
catalog_test.go is missing t.Parallel(); update the t.Run subtest body to call
t.Parallel() as the first line inside the anonymous func used by t.Run to enable
parallel execution (unless this test is intentionally environment-sensitive, in
which case add a comment and use t.Setenv to opt out). Ensure the change is
applied within the t.Run("Should exclude disabled skills", func(t *testing.T) {
... }) block surrounding the BuildCatalog assertions.

---

Nitpick comments:
In `@internal/daemon/daemon_test.go`:
- Around line 3775-3777: The test currently asserts the plain string "agh" via
the fragments slice (variable fragments in daemon_test.go), which can match
unrelated text; change the expectation to a specific, block-scoped fragment such
as "<available-skills>agh</available-skills>" (or update the appended fragments
to append that exact block instead of separate "<available-skills>" and "agh")
so the test verifies the skill entry as a distinct element rather than a loose
substring.

In `@internal/session/manager_test.go`:
- Around line 3204-3209: Tests repeatedly call skillbundled.LoadResource(... )
followed by strings.TrimSpace(...) (e.g., the calls using LoadResource with
testBundledAghSkillName and testBundledNetworkReference at lines where
networkSkill is loaded), causing duplicated logic and inconsistent assertions;
extract a small helper function (e.g., loadBundledResourceOrFatal(t, name, ref)
used in manager_test.go tests) that wraps skillbundled.LoadResource, checks err
and calls t.Fatalf with a consistent message, trims the result with
strings.TrimSpace, and return the string, then replace all repeated
LoadResource+TrimSpace sequences (including the occurrences referring to
testBundledAghSkillName/testBundledNetworkReference) with calls to that helper
to keep failure messages and behavior consistent.

In `@internal/skills/resource_spec.go`:
- Around line 169-207: normalizeMCPServerDecl contains duplicated logic for
cleaning Env and SecretEnv maps; extract that into a helper like cleanEnvMap(m
map[string]string) map[string]string that: iterates entries, trims keys and
values, skips entries with empty trimmed keys, deletes original keys, and
returns nil if the resulting map is empty (preserving the current nil vs empty
behavior). Replace the two nearly identical blocks in normalizeMCPServerDecl
with normalized.Env = cleanEnvMap(normalized.Env) and normalized.SecretEnv =
cleanEnvMap(normalized.SecretEnv), ensuring behavior and semantics remain
unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a81836bd-6b21-4a8b-a622-e79fed57710d

📥 Commits

Reviewing files that changed from the base of the PR and between fb991d8 and a7cbdcb.

⛔ Files ignored due to path filters (25)

• internal/CLAUDE.md is excluded by !**/*.md
• internal/skills/bundled/skills/agh-agent-setup/SKILL.md is excluded by !**/*.md
• internal/skills/bundled/skills/agh-memory-guide/SKILL.md is excluded by !**/*.md
• internal/skills/bundled/skills/agh-network/SKILL.md is excluded by !**/*.md
• internal/skills/bundled/skills/agh-orchestrator/SKILL.md is excluded by !**/*.md
• internal/skills/bundled/skills/agh-session-guide/SKILL.md is excluded by !**/*.md
• internal/skills/bundled/skills/agh-task-reviewer/SKILL.md is excluded by !**/*.md
• internal/skills/bundled/skills/agh-task-worker/SKILL.md is excluded by !**/*.md
• internal/skills/bundled/skills/agh-tools-guide/SKILL.md is excluded by !**/*.md
• packages/site/content/runtime/core/autonomy/review-gate.mdx is excluded by !**/*.mdx
• packages/site/content/runtime/core/skills/bundled.mdx is excluded by !**/*.mdx
• packages/site/content/runtime/core/skills/index.mdx is excluded by !**/*.mdx
• packages/site/content/runtime/core/skills/marketplace.mdx is excluded by !**/*.mdx
• skills/agh/SKILL.md is excluded by !**/*.md
• skills/agh/references/agent-definitions.md is excluded by !**/*.md
• skills/agh/references/capabilities-and-bundles.md is excluded by !**/*.md
• skills/agh/references/contributing-to-agh.md is excluded by !**/*.md
• skills/agh/references/docs-design-and-copy.md is excluded by !**/*.md
• skills/agh/references/memory.md is excluded by !**/*.md
• skills/agh/references/native-tools.md is excluded by !**/*.md
• skills/agh/references/network.md is excluded by !**/*.md
• skills/agh/references/qa-and-verification.md is excluded by !**/*.md
• skills/agh/references/runtime-operations.md is excluded by !**/*.md
• skills/agh/references/tasks-and-orchestration.md is excluded by !**/*.md
• skills/agh/references/tools-and-skills.md is excluded by !**/*.md

📒 Files selected for processing (36)

• internal/api/core/interfaces.go
• internal/api/testutil/skills_stub.go
• internal/cli/skill_commands.go
• internal/cli/skill_marketplace_integration_test.go
• internal/cli/skill_test.go
• internal/cli/skill_workspace.go
• internal/config/config_test.go
• internal/config/persistence_test.go
• internal/daemon/boot.go
• internal/daemon/composed_assembler_test.go
• internal/daemon/daemon_integration_test.go
• internal/daemon/daemon_test.go
• internal/daemon/harness_context_integration_test.go
• internal/daemon/native_review_tools.go
• internal/daemon/native_tools.go
• internal/daemon/native_tools_test.go
• internal/daemon/prompt_sections.go
• internal/daemon/review_router.go
• internal/daemon/review_router_test.go
• internal/session/manager_integration_test.go
• internal/session/manager_test.go
• internal/skills/bundled/bundled_test.go
• internal/skills/bundled/content.go
• internal/skills/catalog_test.go
• internal/skills/registry.go
• internal/skills/resource.go
• internal/skills/resource_spec.go
• internal/store/globaldb/global_db_task_review_test.go
• internal/task/manager_review_test.go
• internal/testutil/acpmock/fixture_test.go
• internal/tools/builtin/skills.go
• packages/site/lib/__tests__/runtime-autonomy-docs.test.ts
• packages/site/lib/__tests__/runtime-tools-canonical-docs.test.ts
• skills/bundled_test.go
• skills/content.go
• skills/embed.go

💤 Files with no reviewable changes (2)

• internal/skills/bundled/content.go
• internal/skills/bundled/bundled_test.go

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@sdk/create-extension/src/__tests__/index.test.ts`:
- Around line 11-12: The outer test timeout goTemplateTestTimeoutMs is too small
relative to the two Go subprocess budgets (goCommandTimeoutMs); update
goTemplateTestTimeoutMs to at least the sum of both goCommandTimeoutMs values
plus a small buffer (e.g., goTemplateTestTimeoutMs = 2 * goCommandTimeoutMs +
5_000) to avoid flakes, and apply the same change to the other occurrences of
goTemplateTestTimeoutMs in this test file (the top-level constants and the later
uses referenced around the two Go command invocations).

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9ac42e92-b000-4281-b749-dd7c8622c8ce

📥 Commits

Reviewing files that changed from the base of the PR and between 08bbfc6 and 83e9f04.

📒 Files selected for processing (1)

• sdk/create-extension/src/__tests__/index.test.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Align the test timeout with combined subprocess budgets.

goTemplateTestTimeoutMs is 30s, but the test runs two Go commands that can each take up to 20s. The outer timeout can fire first and cause avoidable flakes on slower CI.

Suggested fix

 const goCommandTimeoutMs = 20_000;
-const goTemplateTestTimeoutMs = 30_000;
+const goTemplateTestTimeoutMs = goCommandTimeoutMs * 2 + 15_000;

Also applies to: 126-127, 131-135

🤖 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 `@sdk/create-extension/src/__tests__/index.test.ts` around lines 11 - 12, The
outer test timeout goTemplateTestTimeoutMs is too small relative to the two Go
subprocess budgets (goCommandTimeoutMs); update goTemplateTestTimeoutMs to at
least the sum of both goCommandTimeoutMs values plus a small buffer (e.g.,
goTemplateTestTimeoutMs = 2 * goCommandTimeoutMs + 5_000) to avoid flakes, and
apply the same change to the other occurrences of goTemplateTestTimeoutMs in
this test file (the top-level constants and the later uses referenced around the
two Go command invocations).

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@internal/e2elane/command_wiring_test.go`:
- Around line 286-297: The test currently asserts that the Turbo task
"@agh/site#build" has "^build" in its DependsOn but misses the same assertion
for "@agh/ui#build"; update the test to fetch cfg.Tasks["@agh/ui#build"]
(similar to siteBuild) and assert using containsString(uiBuild.DependsOn,
"^build") (or the existing containsString helper) so both Vercel build tasks are
validated for build propagation alongside the existing check for
siteBuild.DependsOn.
- Around line 271-285: The loop over Vercel build tasks should be converted into
subtests to provide isolation and allow parallel execution: replace the for _,
taskName := range ... loop with t.Run(fmt.Sprintf("Should not depend on codegen
for %s", taskName), func(t *testing.T) { t.Parallel(); task, ok :=
cfg.Tasks[taskName]; if !ok { t.Fatalf(...) } ; if
containsString(task.DependsOn, codegenCheckTask) { t.Fatalf(...) } }) so each
case uses t.Run and t.Parallel and still references cfg.Tasks, containsString,
and codegenCheckTask.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b5885b55-3ee1-4cf0-87d7-8b1bfdf9530f

📥 Commits

Reviewing files that changed from the base of the PR and between c6eb38f and 699999e.

⛔ Files ignored due to path filters (1)

• turbo.json is excluded by !**/*.json

📒 Files selected for processing (1)

• internal/e2elane/command_wiring_test.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use subtests for each Vercel build task case.

Line 271 iterates test cases without t.Run("Should ..."), so case-level parallelism and failure isolation are missing.

Suggested patch

-	for _, taskName := range []string{"@agh/site#build", "@agh/ui#build"} {
-		task, ok := cfg.Tasks[taskName]
-		if !ok {
-			t.Fatalf("turbo task %q missing from %#v", taskName, cfg.Tasks)
-		}
-		if containsString(task.DependsOn, codegenCheckTask) {
-			t.Fatalf(
-				"turbo task %q dependsOn = %#v, want no %q because Vercel site deploys do not install Go",
-				taskName,
-				task.DependsOn,
-				codegenCheckTask,
-			)
-		}
-	}
+	for _, taskName := range []string{"@agh/site#build", "@agh/ui#build"} {
+		taskName := taskName
+		t.Run("Should keep "+taskName+" deployable without Go toolchain", func(t *testing.T) {
+			t.Parallel()
+			task, ok := cfg.Tasks[taskName]
+			if !ok {
+				t.Fatalf("turbo task %q missing from %#v", taskName, cfg.Tasks)
+			}
+			if containsString(task.DependsOn, codegenCheckTask) {
+				t.Fatalf(
+					"turbo task %q dependsOn = %#v, want no %q because Vercel site deploys do not install Go",
+					taskName,
+					task.DependsOn,
+					codegenCheckTask,
+				)
+			}
+		})
+	}

As per coding guidelines, "**/*_test.go: MUST use t.Run(\"Should ...\") pattern for ALL test cases" and use t.Parallel by default.

🤖 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 `@internal/e2elane/command_wiring_test.go` around lines 271 - 285, The loop
over Vercel build tasks should be converted into subtests to provide isolation
and allow parallel execution: replace the for _, taskName := range ... loop with
t.Run(fmt.Sprintf("Should not depend on codegen for %s", taskName), func(t
*testing.T) { t.Parallel(); task, ok := cfg.Tasks[taskName]; if !ok {
t.Fatalf(...) } ; if containsString(task.DependsOn, codegenCheckTask) {
t.Fatalf(...) } }) so each case uses t.Run and t.Parallel and still references
cfg.Tasks, containsString, and codegenCheckTask.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert ^build propagation for both Vercel build tasks, not just site.

Line 286 validates ^build only for @agh/site#build; @agh/ui#build can regress silently.

Suggested patch

-	siteBuild, ok := cfg.Tasks["@agh/site#build"]
-	if !ok {
-		t.Fatalf("turbo task %q missing from %#v", "@agh/site#build", cfg.Tasks)
-	}
-	if !containsString(siteBuild.DependsOn, "^build") {
-		t.Fatalf(
-			"turbo task %q dependsOn = %#v, want dependency build propagation",
-			"@agh/site#build",
-			siteBuild.DependsOn,
-		)
-	}
+	for _, taskName := range []string{"@agh/site#build", "@agh/ui#build"} {
+		taskName := taskName
+		t.Run("Should keep "+taskName+" depending on ^build", func(t *testing.T) {
+			t.Parallel()
+			task, ok := cfg.Tasks[taskName]
+			if !ok {
+				t.Fatalf("turbo task %q missing from %#v", taskName, cfg.Tasks)
+			}
+			if !containsString(task.DependsOn, "^build") {
+				t.Fatalf(
+					"turbo task %q dependsOn = %#v, want dependency build propagation",
+					taskName,
+					task.DependsOn,
+				)
+			}
+		})
+	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	siteBuild, ok := cfg.Tasks["@agh/site#build"]
	if !ok {
	t.Fatalf("turbo task %q missing from %#v", "@agh/site#build", cfg.Tasks)
	}
	if !containsString(siteBuild.DependsOn, "^build") {
	t.Fatalf(
	"turbo task %q dependsOn = %#v, want dependency build propagation",
	"@agh/site#build",
	siteBuild.DependsOn,
	)
	}
	}
	for _, taskName := range []string{"@agh/site#build", "@agh/ui#build"} {
	taskName := taskName
	t.Run("Should keep "+taskName+" depending on ^build", func(t *testing.T) {
	t.Parallel()
	task, ok := cfg.Tasks[taskName]
	if !ok {
	t.Fatalf("turbo task %q missing from %#v", taskName, cfg.Tasks)
	}
	if !containsString(task.DependsOn, "^build") {
	t.Fatalf(
	"turbo task %q dependsOn = %#v, want dependency build propagation",
	taskName,
	task.DependsOn,
	)
	}
	})
	}
	}

🤖 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 `@internal/e2elane/command_wiring_test.go` around lines 286 - 297, The test
currently asserts that the Turbo task "@agh/site#build" has "^build" in its
DependsOn but misses the same assertion for "@agh/ui#build"; update the test to
fetch cfg.Tasks["@agh/ui#build"] (similar to siteBuild) and assert using
containsString(uiBuild.DependsOn, "^build") (or the existing containsString
helper) so both Vercel build tasks are validated for build propagation alongside
the existing check for siteBuild.DependsOn.