feat(skills): detect installed AI agents and install the DeployHQ skill#26
Merged
Conversation
Add a Wrangler-style post-login hook and a standalone `dhq skills`
command that detect AI coding agents on this machine and install the
DeployHQ skill (skills/deployhq/) into each agent's native format.
Twelve targets supported:
- Aider (user-scope, owned file + wire-up note)
- Antigravity (project-scope, repo-root AGENTS.md)
- Claude Code (user-scope, ~/.claude/skills/ tree)
- Cline (project-scope, .clinerules/deployhq.md)
- Codex CLI (user-scope, ~/.codex/AGENTS.md sentinel section)
- Continue.dev (user-scope, ~/.continue/rules/deployhq.md)
- Cursor (user-scope, ~/.cursor/rules/deployhq.mdc)
- Gemini CLI (user-scope, ~/.gemini/GEMINI.md sentinel section)
- GitHub Copilot (project-scope, .github/copilot-instructions.md)
- Kiro CLI (project-scope, .kiro/steering/deployhq.md)
- OpenCode (user-scope, $XDG_CONFIG_HOME/opencode/AGENTS.md)
- Windsurf (user-scope, ~/.codeium/windsurf/memories/global_rules.md)
Key design choices:
- Scope (User vs Project) is first-class on the Target interface; the
hello prompt only auto-offers ScopeUser targets so we never mutate
a user's repo as a side effect of logging in.
- Targets that share an instructions file with the user (Windsurf,
Codex, Gemini, OpenCode, Copilot, Antigravity) own only a
sentinel-bounded section; user content outside the markers is
preserved byte-for-byte across (re)installs.
- Targets that own their file entirely (Aider, Cline, Continue) use
a top-of-file `<!-- deployhq-skill v1 -->` HTML comment for
versioning.
- All installs are idempotent: install -> re-install produces
byte-identical files. Outdated installs upgrade silently.
- An optional Noter interface lets Aider surface the one-line
`~/.aider.conf.yml` wire-up step we explicitly refuse to automate.
The runtime agent (per harness.Detect) is auto-installed during
`dhq hello` without prompting — if the user is running dhq from
inside Claude Code right now, they want the skill. Other user-scope
agents detected on disk are batched into a single Y/n prompt.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a complete ChangesAgent Skills Subsystem
Sequence Diagram(s)sequenceDiagram
actor User
participant hello as dhq hello
participant offer as offerSkillInstall
participant detect as skillinstaller.DetectInstalled()
participant harness as harness.Detect()
participant prompt as Y/n Prompt
participant install as t.Install()
rect rgba(70, 130, 180, 0.5)
Note over hello,install: Step 2 — non-fatal skill install
User->>hello: dhq hello
hello->>offer: offerSkillInstall(env)
offer->>detect: DetectInstalled()
detect-->>offer: []DetectResult
offer->>harness: Detect() for runtime agent
harness-->>offer: runtime agent or nil
offer->>install: auto-install runtime (no prompt, even NonInteractive)
install-->>offer: path or error (warning only)
offer->>prompt: Confirm install for N other user-scope agents? [Y/n]
prompt-->>offer: yes / no / skip
offer->>install: installOne() per remaining if confirmed
install-->>offer: path + optional PostInstallNote
offer-->>hello: nil (non-fatal)
end
rect rgba(60, 179, 113, 0.5)
Note over User,install: dhq skills install command
User->>hello: dhq skills install --agent cursor
hello->>detect: Find("cursor")
detect-->>hello: cursor Target
hello->>install: cursor.Install()
install-->>hello: ~/.cursor/rules/deployhq.mdc
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: d55b132992
ℹ️ 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".
…epo root Codex review (PR #26) flagged two bugs in the skill-installer: 1. `dhq skills install` (no --agent) iterated every detected target, silently mutating .github/, AGENTS.md, .clinerules/, and .kiro/ in the user's current repo. That contradicts the PR's own contract that project-scope targets are opt-in. Filter bulk install to ScopeUser; print a hint listing skipped project-scope agents and the --agent invocation to install each. 2. Copilot's repoRoot() returned cwd, not the .git-bearing ancestor. Running `dhq` from a subdirectory wrote to `subdir/.github/copilot-instructions.md`, where Copilot doesn't look. Same pattern existed in Cline, Kiro, and Antigravity. Extract a shared findRepoRoot() helper and route all four project-scope targets through it for both Detect() and Install(). Adds regression tests in repo_test.go: each project-scope target, when invoked from a nested subdirectory, must write at the repo root and must NOT create anything in the subdirectory. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
internal/skillinstaller/section_test.go (1)
58-65: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueOptional: simplify contains() helper with strings.Contains.
The
contains()helper implements substring search manually (lines 58-65). Go's standard library providesstrings.Contains()which is more idiomatic and equally efficient here:func contains(s, sub string) bool { return strings.Contains(s, sub) }Or inline it in the test calls. This is purely a style preference.
🤖 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/skillinstaller/section_test.go` around lines 58 - 65, The `contains()` helper function manually implements substring search which is unnecessary since Go's standard library provides this functionality. Replace the manual implementation of the `contains()` function with a simple call to `strings.Contains()`, which is more idiomatic and efficient. Alternatively, you can inline `strings.Contains()` directly at the call sites in the test instead of keeping a separate helper function. Make sure to import the strings package if you choose this approach.internal/commands/skills.go (1)
37-39: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAlign
skills listwording with actual output.Line 38 says “detected AI agents,” but Lines 43-49 enumerate all registered targets (including
not-installed). Either switch to detected-only rows or update command/help text to “supported AI agents and status”.Also applies to: 43-49
🤖 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/commands/skills.go` around lines 37 - 39, The Short description for the list subcommand states "detected AI agents" but the actual output enumeration includes all registered targets including not-installed ones. Update the Short field text to say "supported AI agents and status" instead to accurately reflect the actual output displayed in the command implementation, ensuring the help text matches what users will see when running the command.internal/skillinstaller/copilot_test.go (1)
180-191: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winStrengthen ancestor test to verify install location, not just detect status.
This case proves nested repo detection, but it does not assert that
Install()writes to<repo>/.github/copilot-instructions.md. Adding that assertion will prevent regressions where nested cwd writes to the wrong directory.Suggested test extension
func TestCopilot_InRepo_FindsAncestor(t *testing.T) { @@ withCwd(t, sub) if got := (copilot{}).Detect(); got != StatusAvailable { t.Fatalf("Detect() nested in repo = %v, want StatusAvailable", got) } + + gotPath, err := (copilot{}).Install() + if err != nil { + t.Fatalf("Install() nested in repo = %v", err) + } + wantPath := filepath.Join(root, copilotInstructionsFile) + if gotPath != wantPath { + t.Fatalf("Install() path = %q, want %q", gotPath, wantPath) + } }🤖 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/skillinstaller/copilot_test.go` around lines 180 - 191, The TestCopilot_InRepo_FindsAncestor test function currently only verifies that Detect() returns StatusAvailable when called from a nested directory, but does not assert that Install() writes the copilot-instructions.md file to the correct repository root location. After the existing Detect() assertion, add a call to Install() on the copilot struct and then verify that the copilot-instructions.md file exists at the expected path within the repository root's .github directory (constructed from the root variable). This ensures that when installing from a nested working directory, the file is written to the correct ancestor repository location and not to the nested cwd.
🤖 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/commands/skills.go`:
- Around line 78-80: The code in the DetectInstalled loop unconditionally
includes all detected targets without filtering by scope, allowing
project-scoped installers to run without explicit opt-in when dhq skills install
is called without the --agent flag. Filter the targets returned by
skillinstaller.DetectInstalled() to only include those appropriate for the
current scope context. Specifically, exclude project-scoped targets (check the
d.Target scope property) unless the command was invoked with explicit opt-in
such as the --agent flag, ensuring project-scope modifications require
intentional user action.
In `@internal/skillinstaller/aider_test.go`:
- Around line 181-183: The test case with the label "v42 mid file" on line 182
is validating that a marker found in the middle of a file (with content "x\n"
before the <!-- deployhq-skill v42 --> marker) is detected as a valid version.
This conflicts with the contract that owned-file markers must be at the top of
the file. Remove this test case entirely, as it codifies invalid behavior that
can cause false positives in the Detect() function when processing non-owned
files.
In `@internal/skillinstaller/aider.go`:
- Around line 124-127: The path variable p is being interpolated directly into
YAML and CLI syntax within the fmt.Sprintf call without proper quoting, which
will break on Windows paths containing backslashes or paths with spaces. Add
appropriate quote characters around the %s placeholders in the format string to
properly escape the path values in both the YAML snippet (read: [%s]) and the
CLI argument (--read %s) portions of the returned string.
In `@internal/skillinstaller/antigravity.go`:
- Around line 61-70: The Detect method uses the current working directory (cwd)
for file operations instead of the repository root, which causes files to be
installed in the wrong location when running from a subdirectory. Modify the
antigravity Detect method to resolve and use the repository root path (obtained
via the same mechanism that inRepo() uses to detect .git) instead of the raw cwd
when reading files like antigravityInstructionsFile at line 69. Apply this same
fix to the other file operations at lines 94 and 102 that also use cwd to ensure
all read/write operations are consistently resolved from the repository root
rather than the current working directory.
In `@internal/skillinstaller/cline.go`:
- Around line 56-74: The skillFile() and rulesPathStat() methods construct paths
to .clinerules using getCwd() instead of the repository root, causing them to
target incorrect locations from nested directories. Replace getCwd() calls in
both the skillFile() method (lines 57-63) and rulesPathStat() method (lines
69-73) with logic that finds the repository root by walking up the directory
tree to locate the .git directory. This ensures the methods consistently resolve
.clinerules paths relative to the actual repository root rather than the current
working directory.
In `@internal/skillinstaller/copilot.go`:
- Around line 49-55: The repoRoot() method is returning the current working
directory directly without resolving to the actual repository root, which causes
Copilot files to be placed in incorrect subdirectories when invoked from
repo/subdir. Modify the repoRoot() function to traverse up the directory tree
from the current working directory to find and return the ancestor directory
containing .git, ensuring that methods at lines 89, 113, and 121 place files
correctly at the repository root rather than in subdirectories.
In `@internal/skillinstaller/installer.go`:
- Around line 133-138: The DetectInstalled() docstring incorrectly describes
special handling for the runtime agent, claiming it is included with special
logic even if Status would be StatusInstalled. However, the actual
implementation simply filters out StatusNotInstalled and returns all other
statuses without any runtime agent-specific logic. Update the docstring to
accurately state that DetectInstalled() returns all agents that are installed
(all statuses except StatusNotInstalled), and clarify that the decision to
handle the runtime agent differently (e.g., for silent upgrades) is delegated to
the caller.
- Around line 149-158: The Find() function has conflicting behavior between its
docstring and implementation. The docstring on line 150 claims "Matching is
case-insensitive on the canonical name," but the comparison at lines 152-156
uses case-sensitive equality check (t.Name() == name). Verify the intended
behavior by checking how Find() is used throughout the codebase, then either
update the docstring to remove the "case-insensitive" claim if case-sensitive
matching is correct, or implement case-insensitive comparison by converting both
t.Name() and name to lowercase before comparing them in the Find() function.
In `@internal/skillinstaller/kiro.go`:
- Around line 55-61: The skillPath() method in the kiro type uses getCwd() to
build the path to the skill file, but this returns the current working directory
rather than anchoring to the repository root. When kiro tools are executed from
nested directories within a repo, this causes the skill file to be detected or
installed in the wrong location. Replace the getCwd() call in skillPath() with a
function that determines and returns the repository root directory, then build
the path from that root. Also review and apply the same fix to other methods in
the kiro type that are mentioned to be affected (lines 63-106) to ensure
consistent behavior across all file path operations.
In `@internal/skillinstaller/opencode.go`:
- Around line 33-36: The xdgConfigDir() function accepts any non-empty
XDG_CONFIG_HOME environment variable value without validating that it is an
absolute path. Add a check after retrieving the XDG_CONFIG_HOME environment
variable to ensure the path is absolute before returning it; if the path is
relative, skip it and fall through to the default behavior. Apply the same
validation to the other similar functions mentioned in the comment that handle
environment variable paths (those around lines 44-50 and 75-104) to ensure all
config and data directory lookups reject relative paths and prevent unintended
writes to the current directory.
In `@internal/skillinstaller/section.go`:
- Around line 45-58: The mergeSection function calls joinAround on lines that
construct the return value, but the joinAround function is not defined in this
file and no import provides it, causing a compilation error. Either define the
joinAround helper function directly in section.go (it should accept three string
parameters: pre, section, and post, and return a joined string) or verify that
joinAround exists elsewhere in the skillinstaller package and is being called
with the correct qualified name. Ensure the function is accessible to
mergeSection when it makes the two joinAround calls.
---
Nitpick comments:
In `@internal/commands/skills.go`:
- Around line 37-39: The Short description for the list subcommand states
"detected AI agents" but the actual output enumeration includes all registered
targets including not-installed ones. Update the Short field text to say
"supported AI agents and status" instead to accurately reflect the actual output
displayed in the command implementation, ensuring the help text matches what
users will see when running the command.
In `@internal/skillinstaller/copilot_test.go`:
- Around line 180-191: The TestCopilot_InRepo_FindsAncestor test function
currently only verifies that Detect() returns StatusAvailable when called from a
nested directory, but does not assert that Install() writes the
copilot-instructions.md file to the correct repository root location. After the
existing Detect() assertion, add a call to Install() on the copilot struct and
then verify that the copilot-instructions.md file exists at the expected path
within the repository root's .github directory (constructed from the root
variable). This ensures that when installing from a nested working directory,
the file is written to the correct ancestor repository location and not to the
nested cwd.
In `@internal/skillinstaller/section_test.go`:
- Around line 58-65: The `contains()` helper function manually implements
substring search which is unnecessary since Go's standard library provides this
functionality. Replace the manual implementation of the `contains()` function
with a simple call to `strings.Contains()`, which is more idiomatic and
efficient. Alternatively, you can inline `strings.Contains()` directly at the
call sites in the test instead of keeping a separate helper function. Make sure
to import the strings package if you choose this approach.
🪄 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: 3837e22c-155a-48c5-ba7d-8145e7509dc3
📒 Files selected for processing (34)
internal/commands/hello.gointernal/commands/hello_skills.gointernal/commands/root.gointernal/commands/skills.gointernal/skillinstaller/aider.gointernal/skillinstaller/aider_test.gointernal/skillinstaller/antigravity.gointernal/skillinstaller/antigravity_test.gointernal/skillinstaller/claude.gointernal/skillinstaller/cline.gointernal/skillinstaller/cline_test.gointernal/skillinstaller/codex.gointernal/skillinstaller/codex_test.gointernal/skillinstaller/continue.gointernal/skillinstaller/continue_test.gointernal/skillinstaller/copilot.gointernal/skillinstaller/copilot_test.gointernal/skillinstaller/cursor.gointernal/skillinstaller/cursor_test.gointernal/skillinstaller/flatten.gointernal/skillinstaller/frontmatter.gointernal/skillinstaller/gemini.gointernal/skillinstaller/gemini_test.gointernal/skillinstaller/installer.gointernal/skillinstaller/installer_test.gointernal/skillinstaller/kiro.gointernal/skillinstaller/kiro_test.gointernal/skillinstaller/opencode.gointernal/skillinstaller/opencode_test.gointernal/skillinstaller/section.gointernal/skillinstaller/section_test.gointernal/skillinstaller/windsurf.gointernal/skillinstaller/windsurf_test.goskills/embed.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/skillinstaller/repo.go (1)
18-22: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd test coverage for
getCwd()error case.The error path when
getCwd()fails (line 20-21) is not covered by tests. While this is straightforward error handling, testing it would improve coverage and prevent regressions.🧪 Suggested test case
func TestFindRepoRoot_GetCwdError(t *testing.T) { oldGetCwd := getCwd defer func() { getCwd = oldGetCwd }() getCwd = func() (string, error) { return "", os.ErrPermission } got, ok := findRepoRoot() if ok { t.Error("findRepoRoot() returned ok when getCwd failed") } if got != "" { t.Errorf("findRepoRoot() = %q, want empty string on error", got) } }🤖 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/skillinstaller/repo.go` around lines 18 - 22, Add a test case for the error path in the findRepoRoot() function when getCwd() fails. Create a new test function that temporarily replaces the getCwd function with a mock implementation that returns an error, then verify that findRepoRoot() returns an empty string and false (the ok boolean should be false). Use a defer statement to restore the original getCwd function after the test completes, ensuring the test properly isolates the getCwd() error scenario that is currently not covered by existing tests.internal/commands/skills.go (1)
93-98: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winError message could be misleading when only project-scope agents are detected.
If all detected agents are project-scope (e.g., only Copilot and Cline are installed), they're filtered into
skippedProjectandtargetsremains empty. The error "No supported AI agents detected on this machine" is then factually incorrect—agents were detected but excluded from bulk install.Consider a more accurate message when
len(skippedProject) > 0:if len(targets) == 0 { if len(skippedProject) > 0 { return &output.UserError{ Message: "No user-scope AI agents available for bulk install", Hint: "Install a user-scope agent (e.g. Claude Code) or pass --agent <name> to install for a project-scope agent.", } } return &output.UserError{ Message: "No supported AI agents detected on this machine", Hint: "Install one (e.g. Claude Code) and re-run, or pass --agent <name>.", } }🤖 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/commands/skills.go` around lines 93 - 98, The error message returned when len(targets) == 0 is misleading when project-scope agents are detected but filtered into skippedProject. Modify the condition block starting at the len(targets) == 0 check to first verify if len(skippedProject) > 0. If true, return a different UserError with Message "No user-scope AI agents available for bulk install" and an appropriate Hint directing users to install user-scope agents or use the --agent flag. If skippedProject is empty, return the existing error about no supported agents being detected. This way, the error message accurately reflects whether agents were found but excluded versus no agents being detected at all.
🤖 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 `@internal/commands/skills.go`:
- Around line 93-98: The error message returned when len(targets) == 0 is
misleading when project-scope agents are detected but filtered into
skippedProject. Modify the condition block starting at the len(targets) == 0
check to first verify if len(skippedProject) > 0. If true, return a different
UserError with Message "No user-scope AI agents available for bulk install" and
an appropriate Hint directing users to install user-scope agents or use the
--agent flag. If skippedProject is empty, return the existing error about no
supported agents being detected. This way, the error message accurately reflects
whether agents were found but excluded versus no agents being detected at all.
In `@internal/skillinstaller/repo.go`:
- Around line 18-22: Add a test case for the error path in the findRepoRoot()
function when getCwd() fails. Create a new test function that temporarily
replaces the getCwd function with a mock implementation that returns an error,
then verify that findRepoRoot() returns an empty string and false (the ok
boolean should be false). Use a defer statement to restore the original getCwd
function after the test completes, ensuring the test properly isolates the
getCwd() error scenario that is currently not covered by existing tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 740957bf-9e70-4777-84e6-991a6f88d286
📒 Files selected for processing (7)
internal/commands/skills.gointernal/skillinstaller/antigravity.gointernal/skillinstaller/cline.gointernal/skillinstaller/copilot.gointernal/skillinstaller/kiro.gointernal/skillinstaller/repo.gointernal/skillinstaller/repo_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/skillinstaller/kiro.go
- internal/skillinstaller/copilot.go
- internal/skillinstaller/cline.go
- internal/skillinstaller/antigravity.go
Addresses CodeRabbit findings on PR #26 that survived the first fixup: - aider: PostInstallNote double-quotes the path so the suggested `read: ["..."]` snippet and `--read "..."` invocation are safe to paste verbatim when the user's home contains spaces or other characters that would otherwise need shell/YAML escaping. - opencode: xdgConfigDir requires XDG_CONFIG_HOME to be absolute, per the XDG Base Directory spec. Falls through to ~/.config when the variable holds a relative path, so a misconfigured env never causes writes into the dev's cwd. - installer: tighten two docstrings that lied about behavior. DetectInstalled no longer claims runtime-agent special handling (that lives in offerSkillInstall, not here); Find documents its actual case-sensitive matching and explains why canonical names are always lowercase. - flatten: parseOwnedFileVersion now requires the marker to be the first thing in the body. Owned-file targets (Aider/Cline/Continue/ Kiro) write the marker at the top via buildOwnedSkillFile, so any content above it means the file isn't ours — Detect now reports Available so the next install rewrites cleanly. aider_test flips the corresponding case from "v42 mid file → 42" to "rejected". - skills list: Short text reads "List supported AI agents and their skill install status" — the output enumerates every registered target, not just those detected on disk. - section_test: drops the hand-rolled `contains` helper in favour of strings.Contains. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- repo_test: cover the getCwd-error path in findRepoRoot so a future refactor that silently turns the error into "no repo found" is caught. - skills.go: when bulk install is invoked in a context where every detected agent is project-scope (e.g. inside a git repo with only Copilot/Cline/etc. installed), the previous "No supported AI agents detected on this machine" message was a lie. Distinguish the case and surface the exact `dhq skills install --agent <name>` line for each filtered agent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
internal/skillinstaller/cursor.go (1)
124-124: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winQuote the frontmatter description scalar for YAML safety.
Line 124 writes an unquoted YAML scalar; descriptions containing characters like
:or#can break parsing or truncate content.Proposed fix
- fmt.Fprintf(&buf, "description: %s\n", oneLine(description)) + fmt.Fprintf(&buf, "description: %q\n", oneLine(description))🤖 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/skillinstaller/cursor.go` at line 124, The fmt.Fprintf call writing the YAML description field is outputting an unquoted scalar value, which can break YAML parsing if the description contains special characters like colons or hashes. Fix this by modifying the format string in the fprintf call to wrap the description value in quotes, ensuring it is treated as a safe YAML scalar string regardless of its content.internal/skillinstaller/opencode_test.go (1)
125-146: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression case for relative
XDG_CONFIG_HOME.
TestXDGConfigDircurrently covers empty and absolute values only. A relative-value assertion would lock in the safety behavior and prevent regression to cwd-relative writes.Suggested patch
func TestXDGConfigDir(t *testing.T) { home := t.TempDir() withHomeDir(t, home) @@ if got != "/some/path" { t.Errorf("XDG override = %q, want %q", got, "/some/path") } + + t.Setenv("XDG_CONFIG_HOME", "relative/path") + got, err = xdgConfigDir() + if err != nil { + t.Fatal(err) + } + if got != filepath.Join(home, ".config") { + t.Errorf("relative XDG should fall back = %q, want %q", got, filepath.Join(home, ".config")) + } }🤖 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/skillinstaller/opencode_test.go` around lines 125 - 146, In the TestXDGConfigDir function, add a third test case after the existing absolute path check to verify the behavior when XDG_CONFIG_HOME is set to a relative path value (e.g., "relative/path"). Use t.Setenv to set XDG_CONFIG_HOME to a relative path, call xdgConfigDir(), and assert the expected behavior to prevent regression and ensure the function safely handles relative paths rather than writing to cwd-relative locations.
🤖 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/commands/skills.go`:
- Around line 18-21: The example text for the bulk "dhq skills install" command
(without the --agent flag) currently claims it "Install for every detected
agent," but the actual implementation intentionally skips project-scope targets.
Update this example text in the Examples section to accurately reflect what bulk
install actually does, making it clear to users which agent scopes are included
or excluded to avoid confusion about the command's repo mutation behavior.
In `@internal/skillinstaller/aider_test.go`:
- Around line 148-151: The test assertion around line 149 is comparing the note
against a raw filesystem path in the `want` variable, but the actual
post-install note contains a quoted or escaped version of the path. Fix this by
modifying the `want` variable to include the proper quoting/escaping that
matches the format the note actually outputs, so the test correctly validates
the path on all platforms including Windows. Reference the string comparison in
the if block where strings.Contains checks the note against want.
In `@internal/skillinstaller/aider.go`:
- Around line 130-145: The function quotePathForYAMLAndShell escapes the path
for both YAML and shell contexts using the same logic, but shell double-quoted
strings still evaluate $() and backticks, making the note's "safe to paste"
guarantee incorrect for edge-case paths. Create two separate functions: one for
YAML escaping (quotePathForYAML) and one for shell escaping (quotePathForShell).
The shell version should additionally escape dollar signs and backticks beyond
what the YAML version does. Then in the return statement of the parent function,
use quotePathForYAML for the YAML config example and quotePathForShell for the
shell command example.
In `@internal/skillinstaller/codex.go`:
- Around line 58-62: The Detect() function in codex.go currently treats all
errors from os.ReadFile() as StatusAvailable, but permission and I/O errors
should be handled separately from missing files. Modify the error handling in
the Detect() function to use os.IsNotExist() to check if the error is
specifically a file-not-found error, and only return StatusAvailable in that
case. For other error types (permission denied, I/O errors, etc.), handle them
appropriately by either returning the error or a different status that indicates
failure rather than availability.
In `@internal/skillinstaller/flatten.go`:
- Around line 102-113: The loop iterating through entries in the references
directory currently processes all files, but the function contract specifies
only markdown files should be ingested. Add a check after the directory skip
condition to ensure only files with the .md extension are processed, filtering
out any non-markdown files before reading them with fs.ReadFile and adding them
to the output buffer.
In `@internal/skillinstaller/gemini.go`:
- Around line 51-54: The Detect() function currently returns StatusAvailable for
any error encountered when reading the geminiInstructionsFile, but this masks
actual read failures like permission errors. Modify the error handling in the
os.ReadFile() call to check if the error is specifically a file-not-found error
using os.IsNotExist(err), and only return StatusAvailable in that case. Other
read errors should be handled separately to properly reflect the actual
availability status rather than treating all errors the same way.
In `@internal/skillinstaller/opencode.go`:
- Around line 66-69: The Detect() function in the opencode.go file currently
treats all errors from os.ReadFile when reading the opencodeInstructionsFile as
StatusAvailable, which masks configuration problems. Instead, differentiate
between missing file errors and other read failures: check if the error returned
from os.ReadFile is specifically a "file not found" error using os.IsNotExist(),
and only return StatusAvailable for that case; for all other errors (permission
denied, broken config, etc.), return StatusUnavailable or a different
appropriate status to indicate the configuration state is inaccessible or
broken.
In `@internal/skillinstaller/windsurf.go`:
- Around line 73-77: The current code in the Detect() function treats all
os.ReadFile errors as StatusAvailable, but only file-not-found cases should map
there. Use os.IsNotExist(err) to distinguish between a missing rules file (which
should return StatusAvailable) and other types of errors like permission denied
or I/O errors (which should be handled differently). Modify the error handling
after the os.ReadFile(rulesPath) call to check the error type more precisely and
handle non-existence cases separately from other error conditions.
---
Nitpick comments:
In `@internal/skillinstaller/cursor.go`:
- Line 124: The fmt.Fprintf call writing the YAML description field is
outputting an unquoted scalar value, which can break YAML parsing if the
description contains special characters like colons or hashes. Fix this by
modifying the format string in the fprintf call to wrap the description value in
quotes, ensuring it is treated as a safe YAML scalar string regardless of its
content.
In `@internal/skillinstaller/opencode_test.go`:
- Around line 125-146: In the TestXDGConfigDir function, add a third test case
after the existing absolute path check to verify the behavior when
XDG_CONFIG_HOME is set to a relative path value (e.g., "relative/path"). Use
t.Setenv to set XDG_CONFIG_HOME to a relative path, call xdgConfigDir(), and
assert the expected behavior to prevent regression and ensure the function
safely handles relative paths rather than writing to cwd-relative locations.
🪄 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: 4be1bc79-3db9-4367-a419-1224c94d8d31
📒 Files selected for processing (36)
internal/commands/hello.gointernal/commands/hello_skills.gointernal/commands/root.gointernal/commands/skills.gointernal/skillinstaller/aider.gointernal/skillinstaller/aider_test.gointernal/skillinstaller/antigravity.gointernal/skillinstaller/antigravity_test.gointernal/skillinstaller/claude.gointernal/skillinstaller/cline.gointernal/skillinstaller/cline_test.gointernal/skillinstaller/codex.gointernal/skillinstaller/codex_test.gointernal/skillinstaller/continue.gointernal/skillinstaller/continue_test.gointernal/skillinstaller/copilot.gointernal/skillinstaller/copilot_test.gointernal/skillinstaller/cursor.gointernal/skillinstaller/cursor_test.gointernal/skillinstaller/flatten.gointernal/skillinstaller/frontmatter.gointernal/skillinstaller/gemini.gointernal/skillinstaller/gemini_test.gointernal/skillinstaller/installer.gointernal/skillinstaller/installer_test.gointernal/skillinstaller/kiro.gointernal/skillinstaller/kiro_test.gointernal/skillinstaller/opencode.gointernal/skillinstaller/opencode_test.gointernal/skillinstaller/repo.gointernal/skillinstaller/repo_test.gointernal/skillinstaller/section.gointernal/skillinstaller/section_test.gointernal/skillinstaller/windsurf.gointernal/skillinstaller/windsurf_test.goskills/embed.go
Five clear wins from the second-round full review on PR #26: - skills.go: command Examples docstring claimed `dhq skills install` installs for "every detected agent" — out of date since the earlier fixup taught the bulk path to skip project-scope targets. Replace with examples that show the user-scope default plus explicit --agent invocations for project-scope agents. - flatten.go: references-tree loop now skips non-*.md entries. The embedded skill is markdown by contract, but a stray editor backup or future binary asset shouldn't get inlined verbatim into a single-file target's output. - cursor.go: write the .mdc description as a YAML-quoted scalar (`description: %q` instead of `%s`). Unquoted scalars containing `:` or `#` would corrupt the frontmatter; %q's Go-quoted form is also valid YAML double-quoted syntax. Test expectation updated to match the new quoted output. - opencode_test: add a regression case for relative XDG_CONFIG_HOME to lock in the spec-mandated fallback to ~/.config and prevent a future refactor from accidentally honouring relative paths and writing into the dev's cwd. - aider_test: the path-mention assertion now compares against the output of quotePathForYAMLAndShell rather than the raw path. Passed on Linux/macOS because the quoting is a no-op without metacharacters, but would have broken on Windows where backslashes in the path get escaped — making the substring search miss. Skipping by design: - Separating quotePathForYAML and quotePathForShell — paths with $/backtick in $HOME aren't realistic and the note is informational. - Replacing `if err != nil → StatusAvailable` with IsNotExist checks across Detect functions — without a StatusError/Unknown enum, the alternatives misreport non-existence harder than the status quo; Install will still surface the real error if Detect misclassifies. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…hook Council review (security + tests) on PR #26 surfaced three items worth fixing before merge: 1. Symlink-redirect on writes (MEDIUM, security council). All 12 targets plus writeEmbeddedTree used plain os.WriteFile, which follows symlinks on the final path component. A local actor pre-planting a symlink at any of dhq's predictable install paths (~/.codex/AGENTS.md, .github/copilot-instructions.md, ~/.aider/deployhq-skill.md, …) pointing at a victim file (e.g. ~/.bashrc) would have caused dhq to corrupt that file during install. Self-LPE only — never network reachable — but cheap to close. Add internal/skillinstaller/safewrite.go with safeWriteFile (Lstat- refuse symlinks before WriteFile) and ensureNotSymlinkDir (the same check for refs-tree roots). Route every production write through safeWriteFile, and gate writeEmbeddedTree on ensureNotSymlinkDir before walking. Add safewrite_test.go covering: refuses symlink to victim (asserts victim bytes are not corrupted), writes missing path, overwrites a regular file, refuses symlinked refs root in writeEmbeddedTree. Not race-free against an attacker who swaps in a symlink between the Lstat and the WriteFile. That window is irrelevant under the local-only threat model and could be tightened with O_NOFOLLOW in a future per-OS pass. 2. Zero coverage on offerSkillInstall (P1, test council). The runtime entry point for the entire feature has real branching — runtime auto-install, project-scope filter, NonInteractive suppression, the batched prompt — and none of it was tested. Extract three test seams as package vars (detectInstalledFn, detectRuntimeAgentFn, confirmInstallFn) and add hello_skills_test.go with 8 tests covering: empty detection no-op, runtime auto-install, project- scope skip, non-interactive prompt suppression, prompt-yes, prompt-no, already-installed not re-installed, install-error non-fatal. Production code does not reassign the seams. 3. Mutable-globals + future t.Parallel footgun (all three reviewers flagged this independently). homeDir, getCwd, findAider are all package-level vars overridden by tests. Today's tests are serial so it works; the next contributor to add t.Parallel produces flaky races across the whole package. Add doc comments on each of the three vars explicitly forbidding t.Parallel in this package and pointing at homeDir's note as the rationale. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address CodeRabbit review on PR #26: - Detect() in codex/gemini/opencode/windsurf treated every ReadFile error as StatusAvailable. Only os.IsNotExist now maps to Available; permission/IO errors fall back to StatusNotInstalled so they aren't misreported as a ready-to-install state. - Aider PostInstallNote reused one escaped string for both YAML and shell snippets, but a double-quoted shell arg still evaluates $(...), backticks, and $VAR. Split into quotePathForYAML (double-quoted scalar) and quotePathForShell (single-quoted literal); update test to assert both forms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address Review Council findings on PR #26: 1. Unify section detection and merge (section.go). parseSectionVersion spotted a bare BEGIN prefix while mergeSection required a full BEGIN...END pair, so a truncated section (or user prose mentioning the marker) was reported StatusInstalled while a reinstall appended a duplicate. Both now key off one regex requiring a complete pair, and mergeSection strips ALL owned blocks before appending one — collapsing any accidental duplicates. 2. Refuse to clobber unmarked owned files (flatten.go + aider/cline/ continue/kiro). An existing file without our version marker is treated as user content: writeOwnedFile returns an actionable error instead of overwriting it. Re-installs of our own marked files are unaffected. 3. Extend symlink hardening to the immediate parent (safewrite.go). A planted symlink at the predictable install *directory* (~/.codex, .github, ...) redirected the leaf write even when the leaf wasn't a symlink. safeWriteFile now also refuses a symlinked parent. Deeper ancestors stay out of scope per the documented local-only model (walking to root would false-positive on symlinked homes / macOS /var). 4. Make reference-tree writes atomic (claude.go writeEmbeddedTree). The tree is staged in a sibling temp dir and swapped in via rename only after it's fully written, so a mid-write failure leaves the existing install intact instead of deleting it. Drops the now-redundant pre-RemoveAll in the six shared-file installers. New tests cover each: BEGIN-without-END detection, duplicate-section collapse, owned-file refusal, symlinked-parent refusal, and atomic stale-file replacement. go test ./... -race, go vet, golangci-lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@claude please double check if this change is backward compatible |
|
Claude finished @facundofarias's task in 1m 51s —— View job Backward Compatibility Check
Result: ✅ Backward Compatible (with one behavioral note)Here's a breakdown by surface area:
|
| Scenario | Impact |
|---|---|
Automated test parsing exact dhq hello output |
|
dhq hello run inside Claude Code interactively |
Silent write to ~/.claude/skills/deployhq/ (new, non-fatal) |
dhq hello in CI / non-interactive |
No change — already blocked before this step |
All existing dhq <command> other than hello |
No change |
pkg/sdk consumers / importers |
No change |
Summary: No breaking changes to CLI interfaces, public APIs, config, or env vars. dhq hello gains a new interactive step and a new file-system side effect, but both are gated on TTY/interactive mode and are non-fatal. Existing automation is unaffected.
facundofarias
added a commit
that referenced
this pull request
Jun 30, 2026
* docs(skills): document the skills installer and dhq skills command PRs #25 (dhq launch) and #26 (skills installer) merged, but only launch made it into the docs. Bring the skills installer up to date: - Quick Start: note that dhq hello now offers to install the DeployHQ skill into detected agents, and add a standalone 'dhq skills install'. - Command list: add 'dhq skills list | install'. - Agent Integration: lead with 'dhq skills install' (auto-detects 12 agents, native formats, user vs project scope), and frame 'dhq setup' as the narrower per-agent alternative. - CLAUDE.md: add internal/skillinstaller/ to the architecture map. - hello.go: extend the command's long description to mention the skill install step so --help matches the new behaviour. Docs-only (plus the one help string); no behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(skills): fix scope overpromise and agent display names Address Review Council findings on PR #29: - Quick Start + hello.go no longer imply bare 'dhq skills install' / 'dhq hello' cover project-scope agents. Copilot is ScopeProject and is skipped by the bare command (opt-in via --agent), which the prose now reflects instead of contradicting the Agent Integration section. - Align the supported-agent list with the CLI display names: Continue -> Continue.dev, Kiro -> Kiro CLI (as shown by 'dhq skills list'). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wrangler-style onboarding for
dhq. Afterdhq hellosucceeds, detect AI coding agents on this machine and install the DeployHQ skill (already inskills/deployhq/) into each agent's native format. Also adds a standalonedhq skillscommand for non-interactive / re-install flows.Twelve agents supported:
~/.aider/deployhq-skill.md(+ wire-up note)<repo>/AGENTS.mdsentinel section~/.claude/skills/deployhq/tree.clinerules/deployhq.md~/.codex/AGENTS.mdsentinel section~/.continue/rules/deployhq.md~/.cursor/rules/deployhq.mdc~/.gemini/GEMINI.mdsentinel section.github/copilot-instructions.mdsentinel section.kiro/steering/deployhq.md$XDG_CONFIG_HOME/opencode/AGENTS.mdsentinel section~/.codeium/windsurf/memories/global_rules.mdsentinel sectionDesign highlights
Target.Scope()returnsScopeUserorScopeProject. Thedhq helloprompt only auto-offers ScopeUser targets — we never mutate a user's repo as a side effect of logging in. Project-scope targets are opt-in viadhq skills install --agent <name>.writeEmbeddedTree+ dotfile version marker.buildOwnedSkillFile+<!-- deployhq-skill v1 -->HTML comment for versioning.mergeSectionkeeps user content outside the sentinel markers byte-for-byte intact across (re)installs.install -> installproduces identical files; outdated installs upgrade silently.Noterinterface lets Aider surface the one wire-up step we explicitly refuse to automate (editing the user's~/.aider.conf.ymlis too risky for the scaffold).harness.Detectsays we're running inside (say) Claude Code, the skill installs without prompting — the user is using it right now, they want it.Test plan
go build ./...cleango vet ./...cleango test ./...— all packages green;internal/skillinstaller/has 60+ tests covering detect / install / idempotence / outdated / coexistence per targetgolangci-lint run— 0 issuesdhq skills listsmoke-tested locally — all 12 targets visible, statuses match the on-disk reality of the dev boxdhq helloafter the merge, verify the prompt fires and installs cleanly for at least Claude Code + Cursordhq skills install --agent copilotinside a test repo, verify.github/copilot-instructions.md+.github/copilot/deployhq/tree🤖 Generated with Claude Code
Summary by CodeRabbit
dhq skillscommand withlistandinstallto manage DeployHQ skill integrations across supported tools.dhq hellointeractive setup with a new step to offer local skill installation (errors won’t stop setup).dhq skills installinstalls detected user-scope skills, skips project-scope ones unless explicitly selected, and reports available/installed/outdated status while preserving existing user content.