✨ Symlink skills dir for native goose discovery

[savitharaghunathan]

Summary by CodeRabbit

• New Features

  • Skills are now discovered through the native skills platform at runtime.
  • The harness automatically links the home skills directory to the configured skills location, defaulting to /opt/skills, and updates the link when needed.

• Bug Fixes

  • Conflicting existing directories are detected and reported instead of overwritten.
  • Prompts no longer include injected skill content, while working and commit guidelines remain available.

[coderabbitai]

Warning

Review limit reached

@savitharaghunathan, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 55 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 594123d6-36d0-4a14-9638-fc8eb0a15b42

📥 Commits

Reviewing files that changed from the base of the PR and between f1469d0 and 350d5e2.

📒 Files selected for processing (1)

• harness/cmd/migration-harness/main_test.go

📝 Walkthrough

Walkthrough

The migration harness now exposes discovered skills through ~/.agents/skills, linked to the configurable skills directory. Prompt construction no longer embeds skill content and always includes working guidelines with the commit instruction. Tests cover discovery, symlink behavior, and prompt output.

Changes

Skill discovery and prompt integration

Layer / File(s)	Summary
Skill path discovery and symlink management harness/cmd/migration-harness/main.go, harness/cmd/migration-harness/main_test.go, changes/unreleased/70-skill-symlink-for-goose-discovery.yaml	Skill discovery returns paths without reading content. The harness creates, preserves, replaces, and validates the ~/.agents/skills symlink. Tests cover empty results, conflicting directories, repeated calls, changed targets, and relative paths.
Prompt layer and guideline updates harness/internal/prompt/prompt.go, harness/internal/prompt/prompt_test.go	Layers no longer contains Skill. Prompt construction omits skill instructions and always includes working guidelines and the commit instruction. Tests validate layer ordering, required guidelines, and newline behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MigrationHarness
  participant SkillSource
  participant AgentsSkills
  participant PromptBuilder
  MigrationHarness->>SkillSource: discover skill paths
  MigrationHarness->>AgentsSkills: create or update ~/.agents/skills
  AgentsSkills-->>MigrationHarness: expose configured skills
  MigrationHarness->>PromptBuilder: build prompt without skill content
  PromptBuilder-->>MigrationHarness: return prompt with working guidelines

Loading

Possibly related PRs

• konveyor/agentic-controller#134: Modifies the same skill discovery, symlink management, and prompt-layer code paths.
• konveyor/agentic-controller#138: Implements related symlink-based discovery and prompt-assembly changes.
• konveyor/agentic-controller#83: Introduced the skill discovery and prompt-injection behavior revised here.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description contains only template guidance and omits the change summary, implementation details, testing, and related context.	Replace the template comments with a concise summary of the symlink behavior, affected components, testing performed, and any known limitations.
Docstring Coverage	⚠️ Warning	Docstring coverage is 4.76% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the primary change and uses the required ✨ feature prefix.
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.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[pranavgaikwad]

what happens with skills that are marked as rules?

[savitharaghunathan]

Rules are not in phase1 iirc. I could totally be wrong and if that's the case, harness needs to account for the types either a rule or a skill. @djzager do we support rules for the initial release?

[pranavgaikwad]

We could just append the contents of the rules to .goosehints file right?

[savitharaghunathan]

we could. .goosehints is goose-specific though and adding opencode support in the harness is in my pipeline, so want to make sure we pick something that works across runtimes. If this is not needed in phase 1, can we use #135 to talk about the approach?

[fabianvf]

I'd point $HOME/.agents/skills at the skills dir rather than putting the link in the clone, then the .agents/ gitignore entry and the watcher exclusion both go away with it. goose scans the home root as well as the project root (all_skill_dirs in crates/goose/src/skills/mod.rs, checked against v1.45.0 since that's what agent-base pins) so what it lists is the same either way, the difference is nothing about skill delivery touches the tree we push.

The gitignore write is best-effort:

if err := git.EnsureGitignore(cloneDir, []string{
    ".agents/",
    ...
}); err != nil {
    logging.Warn("gitignore: %v", err)
}

and the symlink gets made regardless. Current skills stage specific paths so I don't think it bites today, but the only thing keeping a link to /opt/skills off the user's branch is a write we don't check. We're also editing their .gitignore and committing it in harness: add grounding data just to make room for the link.

os.Symlink also fails if the repo already has .agents/skills, which TestSymlinkSkillsDir_AlreadyExists pins, so a repo that ships its own skills dir fails the run at setup. Probably rare now, less so in a year.

Tradeoff of the home root is a repo with its own .agents/skills shadows a mounted skill silently instead of failing loudly. I think that's the better failure mode but it is a real change.

Answering @pranavgaikwad's question above: rules work today, but only by accident. The controller mounts a type: rule SkillCard at /opt/skills/<name> exactly like any other skill (resolveSkillVolumes never looks at spec.type), and the harness concatenates everything it finds there, so rule content reaches the prompt because all content does. Nothing in the controller or the harness actually reads the field.

This PR removes the accident. goose lists name and description in its system instructions and exposes load_skill, and the model decides whether to call it, so after this a rule is a suggestion. no-javax-imports that the agent may or may not read isn't a constraint.

What we could do instead is have the controller mount rule-typed cards at /opt/rules/<name>, and the harness concatenate only those, after the staging rules and before the stage task. Keeps the harness dumb about it, no frontmatter parsing and no cluster access, the mount path carries the CRD field. It also gives the Agent controller's context budget check something real to measure, since rules become the only always-loaded content.

Doesn't need to be this PR, I picked up #135 for it, better place to hash it out than here.

[fabianvf]

I don't think my /opt/rules idea above actually works. Nothing links /opt/rules into goose's discovery roots, so a rule-typed skill would lose load_skill and couldn't reach its own references/, which is the thing we're trying to fix. plan and javaee-to-quarkus both ship supporting files so it'd bite immediately. Probably better to leave rules mounted at /opt/skills with everything else and have the controller name them, KONVEYOR_RULES or whatever, so the harness knows which ones to concatenate. Then they're in the prompt and still discoverable. If it's unset the harness can just inject everything like it does now, so an old controller with a new harness lands on current behaviour instead of quietly dropping rules.

@savitharaghunathan on phase 1, I think rules already do something even if they're not a feature yet, a type: rule card gets mounted like any other skill and the glob picks it up, so it's in the prompt. This PR would take that away. Might be the right call for phase 1, I'd just want it to be a decision we make and record.

Also the context budget thing I said last comment was wrong, nothing reads contextWindow, it's just a doc comment on the Gateway field.

[savitharaghunathan]

@fabianvf [c7ad6e1](https://github.com/konveyor/agentic-controller/pull/136/commits/c7ad6e10b285c1931f49bc0d940f5a5ba135b157) addresses the symlinking to home directory instead of repo dir. PTAL

[fabianvf]

Wrote it up as #138. It lands where this PR already ended up, so nothing here needs to change on my account. The KONVEYOR_RULES part is the only piece still open, and that's waiting on the phase 1 answer.

[ibolton336]

Traced the mechanics beyond the diff; they hold together:

• Ordering is right — the link lands after clone, before goose.StartServe, and only when skills are actually mounted, so no dangling link on skill-less runs.
• agent-base pins HOME=/home/harness group-writable, so os.UserHomeDir() + MkdirAll survive OpenShift's random UID.
• Skills mount as ImageVolumes (resolveSkillVolumes → /opt/skills/{name}), so there's no ConfigMap ..data indirection under the link — goose sees real files one symlink hop away.
• Harness unit tests pass locally at this head.

One question: has c7ad6e1 had a live run where goose actually lists/loads a skill through the ~/.agents/skills → /opt/skills hop? The unit tests prove the link exists, not that goose traverses it. If the hack/harness-test rig makes that cheap, an assertion there would pin it against future goose version bumps.

[savitharaghunathan]

Tested the symlink in Kind with the full 5-stage workflow.

The $HOME/.agents/skills symlink works — goose discovers all mounted skills. But goose's native skill loading is opt-in: it lists skills by name/description in the system prompt and the model decides whether to call load_skill. With generic stage instructions like "Scan the project and gather migration decisions", the agent skipped loading the questionnaire skill entirely — it produced a free-form questionnaire.json that ignored the skill's schema and templates.

Fixed by making stage instructions explicit: "Load the 'questionnaire' skill and follow its instructions exactly. Use its templates for the output schema." With that, the agent immediately loaded the skill, read the template and example files, then followed the prescribed phases.

The plan stage loaded the skill and all its references. But it wrote PLAN.md at the repo root instead of .konveyor/spec.md and .konveyor/implementation.md — ignoring the skill's explicit output paths even though Phase 4 says "MUST" and "stage is NOT complete until both files are written."

Workflow run: savitharaghunathan/coolstore#17

[fabianvf]

LGTM

[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 `@harness/cmd/migration-harness/main_test.go`:
- Line 182: Close the preceding TestShouldRevokeToken function before declaring
TestParseHubTokenID, ensuring its assertions and body are fully terminated so
the two test functions are separate top-level declarations and the package
compiles.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 958c0879-106b-4be3-bd9d-2e97bda2c83f

📥 Commits

Reviewing files that changed from the base of the PR and between c7ad6e1 and f1469d0.

📒 Files selected for processing (2)

• harness/cmd/migration-harness/main.go
• harness/cmd/migration-harness/main_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• harness/cmd/migration-harness/main.go