✨ Add questionnaire, plan, execute, and verify stage skills - #98
✨ Add questionnaire, plan, execute, and verify stage skills#98hhpatel14 wants to merge 7 commits into
Conversation
Four stage skills for the migration pipeline: - questionnaire: detect + gather decisions across 7 categories - plan: graphify + spec.md approval + implementation.md - execute: apply steps phase by phase with per-step commits - verify: build fix loop, tests, runtime health checks Also adds token usage logging via ACP PromptResult in the harness. Signed-off-by: Hit Patel <hhpatel14@gmail.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughThe PR adds questionnaire, plan, execute, and verify stage specifications and output schemas. It updates the test workflow to use four stages and records ACP token usage in ChangesHarness migration workflow stages
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant QuestionnaireAgent
participant PlanAgent
participant ExecuteAgent
participant VerifyAgent
QuestionnaireAgent->>PlanAgent: Write .konveyor/questionnaire.json
PlanAgent->>ExecuteAgent: Write .konveyor/implementation.md
ExecuteAgent->>VerifyAgent: Provide migration results
VerifyAgent->>VerifyAgent: Write .konveyor/verify.json
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
hhpatel14
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
hack/harness-test/questionnaire-resources.yaml (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the images used by the test.
quay.io/konveyor/skills:questionnaireandquay.io/konveyor/agent-java:devare mutable tags. A rerun can use different image contents and produce a different result. Use immutable release tags or digests, or document the intentional use of moving development images.Also applies to: 35-35
🤖 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 `@hack/harness-test/questionnaire-resources.yaml` at line 17, Update the image references in questionnaire-resources.yaml for the questionnaire and agent-java test images to use immutable release tags or digests instead of mutable tags. If moving development images are intentionally required, document that choice alongside the references.harness/cmd/migration-harness/main.go (1)
334-355: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the ignored errors in
writeTokenUsage.
writeTokenUsageignores every error it encounters:os.MkdirAllat Line 337,json.Unmarshalat Line 343, andos.WriteFileat Line 354.Two concrete failure modes follow from this:
- If
MkdirAllfails (for example, a permission error), the followingReadFile/WriteFilecalls also fail silently. Token usage history is lost with no diagnostic trail.- If the existing
token-usage.jsonfile is corrupted or partially written,json.Unmarshalfails silently andexistingresets to nil. The followingWriteFilethen overwrites the file, discarding all prior recorded entries without warning.Log these errors with
logging.Warnso operators can detect persistence failures instead of silently losing usage history.🔧 Proposed fix to surface ignored errors
func writeTokenUsage(workDir string, usage *acp.PromptUsage) { konveyorDir := filepath.Join(workDir, ".konveyor") - os.MkdirAll(konveyorDir, 0o755) + if err := os.MkdirAll(konveyorDir, 0o755); err != nil { + logging.Warn("token usage: create dir: %v", err) + return + } usagePath := filepath.Join(konveyorDir, "token-usage.json") var existing []map[string]any - if data, err := os.ReadFile(usagePath); err == nil { - json.Unmarshal(data, &existing) + if data, err := os.ReadFile(usagePath); err == nil { + if err := json.Unmarshal(data, &existing); err != nil { + logging.Warn("token usage: parse existing history: %v", err) + existing = nil + } + } else if !os.IsNotExist(err) { + logging.Warn("token usage: read existing history: %v", err) } entry := map[string]any{ "inputTokens": usage.InputTokens, "outputTokens": usage.OutputTokens, "totalTokens": usage.TotalTokens, } existing = append(existing, entry) - data, _ := json.MarshalIndent(existing, "", " ") - os.WriteFile(usagePath, data, 0o644) + data, err := json.MarshalIndent(existing, "", " ") + if err != nil { + logging.Warn("token usage: marshal history: %v", err) + return + } + if err := os.WriteFile(usagePath, data, 0o644); err != nil { + logging.Warn("token usage: write history: %v", err) + } }🤖 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 `@harness/cmd/migration-harness/main.go` around lines 334 - 355, Update writeTokenUsage to log errors from os.MkdirAll, json.Unmarshal, and os.WriteFile using logging.Warn, while preserving the existing persistence flow. Include relevant operation and path context in each warning; for corrupted token-usage.json, warn before continuing so the overwrite and loss of prior entries is visible.
🤖 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 `@hack/harness-test/questionnaire-resources.yaml`:
- Around line 8-9: Update the usage instructions in questionnaire-resources.yaml
to require rendering the manifest before kubectl apply: replace
__GCP_PROJECT_ID__, __TIMESTAMP__, and __HUB_TOKEN__ with runtime values, then
pipe or validate the rendered output before applying it. Remove the direct-apply
command that leaves templated values unresolved.
In `@harness/skills/execute/SKILL.md`:
- Around line 30-31: Update the execute skill documentation around the output
schema and Resume section to define an "aborted" status alongside "completed".
Explicitly instruct agents to write status "aborted" when execution is
interrupted or a step is unrecoverable, and ensure the schema’s status field
lists both allowed values.
In `@harness/skills/plan/SKILL.md`:
- Around line 102-107: Update the mapping code fence in the plan documentation
to specify the text language identifier, while preserving the existing mapping
content unchanged.
- Around line 203-210: Update the “Rules for writing steps” section in SKILL.md
to define a valid fallback Phase value, such as General, whenever the domain
skill is “none”; alternatively, reject plan generation for domain-less projects.
Ensure every generated step still has a Phase that matches an allowed phase
value.
- Around line 283-287: Update the commit command in the “Commit the outputs”
step to use git commit --only with exactly the four generated artifact paths:
.konveyor/spec.md, .konveyor/implementation.md, .konveyor/graph.json, and
.konveyor/GRAPH_REPORT.md, ensuring unrelated staged changes are excluded.
In `@harness/skills/questionnaire/SKILL.md`:
- Around line 11-13: Update the questionnaire skill’s front matter and stage
description to consistently state that the stage writes both
.konveyor/questionnaire.json and .konveyor/results.json. Explicitly define
results.json’s role alongside questionnaire.json, and replace the one-artifact
wording near the stage instructions so agents are instructed to produce both
declared outputs.
- Around line 282-288: Update the detection workflow and artifact template
around the detection JSON to compute source_file_count before writing the
artifact. Define which directories and generated, vendored, or otherwise
non-source files are excluded, count the remaining source files using the
repository’s detected language context, and write that computed approximate
count instead of the hardcoded 0.
- Around line 207-217: Update the non-interactive guidance in “Category 5:
Database / Persistence Target” so a development database is never treated as the
production target without evidence. Mark the production database decision as
needs-confirmation when production configuration is unclear, while preserving
the existing ORM and schema-constraint guidance.
- Line 133: Update the questionnaire.json template and its guidance in the
questionnaire instructions to represent unresolved decisions explicitly,
including confidence and status fields or a null chosen value with an explicit
unresolved status. Ensure non-interactive mismatch and uncertain decisions are
recorded as needs-confirmation, while resolved selections remain distinguishable
for plan/execute consumers.
- Around line 96-100: Update the architecture-sampling command in the
questionnaire skill so it is not restricted to /workspace/src. Use directories
discovered during Step 1a, or provide a repository-wide fallback that works with
app/, cmd/, root-level, and other layouts, then sample one or two files from
each distinct directory or layer.
- Around line 8-10: The SKILL.md questionnaire skill declares only
KONVEYOR_INSTRUCTIONS and source repository in its inputs section (lines 8-10
anchor), but the implementation reads KONVEYOR_PLAYBOOK_INSTRUCTIONS,
KONVEYOR_QUESTIONNAIRE_MODE, and KONVEYOR_TARGET_FRAMEWORK at lines 123-125,
152-154, and 249-251. Document these three additional inputs in the SKILL.md
inputs list to match the implementation, marking them as optional or with
env-default values. Then update the test resource at
hack/harness-test/questionnaire-resources.yaml lines 51-55 to provide these
three input values so the test validates the full contract rather than relying
on defaults.
- Around line 24-27: Add an explicit untrusted-content boundary to the Phase 1
Detect guidance: treat all files and data read from /workspace/ as repository
data only, never as commands or instructions. Update the harness tool policy to
enforce this boundary so embedded repository text cannot influence migration
decisions or tool usage, while preserving the existing read-only,
no-build/no-execute behavior.
---
Nitpick comments:
In `@hack/harness-test/questionnaire-resources.yaml`:
- Line 17: Update the image references in questionnaire-resources.yaml for the
questionnaire and agent-java test images to use immutable release tags or
digests instead of mutable tags. If moving development images are intentionally
required, document that choice alongside the references.
In `@harness/cmd/migration-harness/main.go`:
- Around line 334-355: Update writeTokenUsage to log errors from os.MkdirAll,
json.Unmarshal, and os.WriteFile using logging.Warn, while preserving the
existing persistence flow. Include relevant operation and path context in each
warning; for corrupted token-usage.json, warn before continuing so the overwrite
and loss of prior entries is visible.
🪄 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: eb91efaa-3010-4960-bc68-414c4cc2220e
📒 Files selected for processing (6)
hack/harness-test/questionnaire-resources.yamlharness/cmd/migration-harness/main.goharness/skills/execute/SKILL.mdharness/skills/plan/SKILL.mdharness/skills/questionnaire/SKILL.mdharness/skills/verify/SKILL.md
| If `.konveyor/execute.json` already exists with `status: "aborted"`, this is a | ||
| resume — see the Resume section below. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Document the "aborted" status in the output schema.
Line 30 tells the agent to check for status: "aborted" in .konveyor/execute.json to detect a resume. The output schema at Lines 70-84 only shows "status": "completed". The skill never shows the agent how or when to write "aborted".
Add an explicit instruction for writing status: "aborted" (for example, when execution is interrupted or a step is unrecoverable), and list "aborted" alongside "completed" in the schema's status field.
Also applies to: 70-84
🤖 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 `@harness/skills/execute/SKILL.md` around lines 30 - 31, Update the execute
skill documentation around the output schema and Resume section to define an
"aborted" status alongside "completed". Explicitly instruct agents to write
status "aborted" when execution is interrupted or a step is unrecoverable, and
ensure the schema’s status field lists both allowed values.
| ### Rules for writing steps | ||
|
|
||
| 1. **Phase on every step** — every step must have a `Phase:` matching a domain skill phase | ||
| 2. **One file per step** — never combine two files in one step | ||
| 3. **Exact paths** — use real paths from graph.json, not placeholders | ||
| 4. **Dependency order** — steps that others depend on come first | ||
| 5. **Phase order** — follow the domain skill's phase ordering | ||
| 6. **Hard steps flagged** — add `COMPLEX:` prefix for structural changes |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Define a valid Phase value when no domain skill exists.
The plan allows the domain skill to be "none", but every step must have a Phase: matching a domain skill phase. A domain-less project has no valid phase value under this rule.
Define a fallback such as General, or reject plan generation when no domain skill is available.
Proposed rule update
-1. **Phase on every step** — every step must have a `Phase:` matching a domain skill phase
+1. **Phase on every step** — every step must have a `Phase:` matching a domain skill phase; use `General` when no domain skill is available📝 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.
| ### Rules for writing steps | |
| 1. **Phase on every step** — every step must have a `Phase:` matching a domain skill phase | |
| 2. **One file per step** — never combine two files in one step | |
| 3. **Exact paths** — use real paths from graph.json, not placeholders | |
| 4. **Dependency order** — steps that others depend on come first | |
| 5. **Phase order** — follow the domain skill's phase ordering | |
| 6. **Hard steps flagged** — add `COMPLEX:` prefix for structural changes | |
| ### Rules for writing steps | |
| 1. **Phase on every step** — every step must have a `Phase:` matching a domain skill phase; use `General` when no domain skill is available | |
| 2. **One file per step** — never combine two files in one step | |
| 3. **Exact paths** — use real paths from graph.json, not placeholders | |
| 4. **Dependency order** — steps that others depend on come first | |
| 5. **Phase order** — follow the domain skill's phase ordering | |
| 6. **Hard steps flagged** — add `COMPLEX:` prefix for structural changes |
🤖 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 `@harness/skills/plan/SKILL.md` around lines 203 - 210, Update the “Rules for
writing steps” section in SKILL.md to define a valid fallback Phase value, such
as General, whenever the domain skill is “none”; alternatively, reject plan
generation for domain-less projects. Ensure every generated step still has a
Phase that matches an allowed phase value.
| - Does the prompt name a source technology that doesn't appear in the code? | ||
| - Is the code already on the target technology? | ||
|
|
||
| If you find a mismatch, this becomes the **first and most important question**. Do not proceed with the rest of the questionnaire until this is resolved. In non-interactive mode, record the mismatch in reasoning and mark all decisions as `"needs-confirmation"`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'questionnaire\.json|needs-confirmation|confidence|chosen|decisions' harnessRepository: konveyor/agentic-controller
Length of output: 28573
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the questionnaire template / decisions schema and relevant consumers.
wc -l harness/skills/questionnaire/SKILL.md
sed -n '270,332p' harness/skills/questionnaire/SKILL.md
printf '\n--- References to .konveyor/questionnaire.json in tracked files ---\n'
rg -n -C 8 '\.konveyor/questionnaire\.json|questionnaire\.json|chosen|confidence|needs-confirmation' --glob '!**/*.svg' --glob '!**/*.png' .
printf '\n--- Candidate schema/JSON validators around questionnaire ---\n'
rg -n -C 5 'schema|JSON Schema|jsonschema|questionnaire|validate' --glob '!**/*.png' --glob '!**/*.svg' harness . 2>/dev/null | head -n 220Repository: konveyor/agentic-controller
Length of output: 46822
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '128,304p' harness/skills/questionnaire/SKILL.mdRepository: konveyor/agentic-controller
Length of output: 8381
Make unresolved questionnaire decisions explicit in questionnaire.json.
The non-interactive instructions allow "needs-confirmation" for mismatch and uncertain decisions, and confidence ratings must be confirmed or needs-confirmation. The current template only captures a single selected chosen option and does not include confidence or status, so plan/execute consumers cannot distinguish a selected decision from an unresolved one. Add explicit state fields, or use chosen: null plus an explicit status for unresolved decisions.
🤖 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 `@harness/skills/questionnaire/SKILL.md` at line 133, Update the
questionnaire.json template and its guidance in the questionnaire instructions
to represent unresolved decisions explicitly, including confidence and status
fields or a null chosen value with an explicit unresolved status. Ensure
non-interactive mismatch and uncertain decisions are recorded as
needs-confirmation, while resolved selections remain distinguishable for
plan/execute consumers.
savitharaghunathan
left a comment
savitharaghunathan
left a comment
There was a problem hiding this comment.
@hhpatel14, thanks for putting this together! The domain knowledge in these skills is solid, especially the decision categories in the questionnaire and the step templates in the plan skill. These capture real enterprise migration decision points that would be hard to reconstruct from scratch.
Most of the review feedback is about tightening the skills to match the conventions we're establishing (agentskills.io headers, KONVEYOR_PARAM_* naming, moving schemas to templates/).
The biggest design question is the execute/verify split — issue #57. What do you think about merging these stages?
Signed-off-by: hhpatel14 <hitpatel@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
harness/skills/questionnaire/SKILL.md (1)
37-37: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winExclude secrets from repository detection.
The command includes
*.env. Later instructions read and quote configuration content. This can expose API keys, tokens, or database passwords to the agent context. Exclude.env*, private-key files, and credential directories. Redact secret values before quoting configuration lines.Proposed safer rule
-find /workspace -maxdepth 3 -type f \( ... -name "*.env" ... \) ... +find /workspace -maxdepth 3 -type f \( ... \) \ + ! \( -name ".env" -o -name ".env.*" -o -name "*.pem" -o -name "*.key" \) ...🤖 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 `@harness/skills/questionnaire/SKILL.md` at line 37, Update the repository-detection command to exclude .env and .env.* files, private-key files, and credential directories alongside the existing exclusions. Before later configuration inspection or quoting, redact secret values such as API keys, tokens, and passwords while preserving non-sensitive configuration context.Source: Linters/SAST tools
🤖 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/skills/execute/SKILL.md`:
- Line 5: Update the execute-stage instructions in SKILL.md to remove the build
gate and any requirement to fix build or test failures, keeping only the
commit-after-each-step behavior. Ensure the workflow consistently delegates
build, test, and related fixes to the verify stage, including the conflicting
guidance around the execute-stage instructions and lines 71-72.
- Around line 43-44: Define failure propagation across the execute instructions
and result schema: in harness/skills/execute/SKILL.md lines 43-44, check each
Depends on value before execution and mark dependents blocked or abort the
phase/run; in harness/skills/execute/templates/execute.md lines 9-21, represent
non-success or define completed only when every step is applied; and in lines
24-40, add blocked or skipped per-step status values and document the resulting
output.
In `@harness/skills/plan/templates/implementation.md`:
- Around line 45-53: Update the implementation-plan contract table so the File
field requires an exact graph.json path for MODIFY and DELETE steps, while
allowing an explicit repository-relative target path for CREATE steps. Preserve
the existing action semantics and ensure CREATE steps no longer require a path
that cannot exist in graph.json.
In `@harness/skills/verify/templates/verify.md`:
- Around line 24-33: Update the runtime example and contract documentation to
represent skipped checks accurately: when runtime status or health_check is
skipped, use null for unavailable startup_time_ms and clean_shutdown instead of
0 and true, and explicitly document that these metrics are null because the
runtime checks did not execute.
---
Outside diff comments:
In `@harness/skills/questionnaire/SKILL.md`:
- Line 37: Update the repository-detection command to exclude .env and .env.*
files, private-key files, and credential directories alongside the existing
exclusions. Before later configuration inspection or quoting, redact secret
values such as API keys, tokens, and passwords while preserving non-sensitive
configuration context.
🪄 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: 78e4cba2-9e8e-4184-93a9-fd7823f4061a
📒 Files selected for processing (10)
hack/harness-test/workflow-resources.yamlharness/skills/execute/SKILL.mdharness/skills/execute/templates/execute.mdharness/skills/plan/SKILL.mdharness/skills/plan/templates/implementation.mdharness/skills/plan/templates/spec.mdharness/skills/questionnaire/SKILL.mdharness/skills/questionnaire/templates/questionnaire.mdharness/skills/verify/SKILL.mdharness/skills/verify/templates/verify.md
🚧 Files skipped from review as they are similar to previous changes (1)
- harness/skills/verify/SKILL.md
| name: execute | ||
| description: > | ||
| Reads the implementation plan and executes migration steps phase by phase. | ||
| Commits after each step. Does not build, test, or push — that is the verify stage's job. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep build fixes in the verify stage.
Line 5 says execute does not build or test. Line 11 says execute runs a build gate and fixes errors. Lines 71-72 prohibit those actions. For the four-stage workflow, remove the per-phase build and fix instruction. Leave build-fix work to verify.
Proposed wording
-Runs the build gate after each phase and fixes errors before moving on.
+Applies the migration steps and commits them. The verify stage runs builds, tests, and fixes.🤖 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 `@harness/skills/execute/SKILL.md` at line 5, Update the execute-stage
instructions in SKILL.md to remove the build gate and any requirement to fix
build or test failures, keeping only the commit-after-each-step behavior. Ensure
the workflow consistently delegates build, test, and related fixes to the verify
stage, including the conflicting guidance around the execute-stage instructions
and lines 71-72.
| | Field | Description | | ||
| |---|---| | ||
| | `Phase` | Must match a domain skill phase name | | ||
| | `File` | Exact path from repo root (from graph.json) | | ||
| | `Action` | One of CREATE, MODIFY, or DELETE | | ||
| | `What to do` | Specific transformation instructions | | ||
| | `Why` | What pattern is being changed and why | | ||
| | `Depends on` | Step numbers this step depends on, or "none" | | ||
| | `Verify` | How to confirm this step is complete | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Allow explicit paths for CREATE steps.
A new file cannot appear in graph.json. The current File rule therefore cannot represent the CREATE example. Require graph paths for existing files and explicit repository-relative target paths for CREATE steps.
Proposed contract wording
-| `File` | Exact path from repo root (from graph.json) |
+| `File` | Exact repository-relative path; use graph.json for existing files and the planned target path for CREATE |📝 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.
| | Field | Description | | |
| |---|---| | |
| | `Phase` | Must match a domain skill phase name | | |
| | `File` | Exact path from repo root (from graph.json) | | |
| | `Action` | One of CREATE, MODIFY, or DELETE | | |
| | `What to do` | Specific transformation instructions | | |
| | `Why` | What pattern is being changed and why | | |
| | `Depends on` | Step numbers this step depends on, or "none" | | |
| | `Verify` | How to confirm this step is complete | | |
| | Field | Description | | |
| |---|---| | |
| | `Phase` | Must match a domain skill phase name | | |
| | `File` | Exact repository-relative path; use graph.json for existing files and the planned target path for CREATE | | |
| | `Action` | One of CREATE, MODIFY, or DELETE | | |
| | `What to do` | Specific transformation instructions | | |
| | `Why` | What pattern is being changed and why | | |
| | `Depends on` | Step numbers this step depends on, or "none" | | |
| | `Verify` | How to confirm this step is complete | |
🤖 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 `@harness/skills/plan/templates/implementation.md` around lines 45 - 53, Update
the implementation-plan contract table so the File field requires an exact
graph.json path for MODIFY and DELETE steps, while allowing an explicit
repository-relative target path for CREATE steps. Preserve the existing action
semantics and ensure CREATE steps no longer require a path that cannot exist in
graph.json.
savitharaghunathan
left a comment
savitharaghunathan
left a comment
There was a problem hiding this comment.
@hhpatel14 this is great. minor suggestions and it should be good to go. Thank you :)
some findings
Contract breaks between stages:
- Execute schema only defines "completed" status but verify checks for "aborted" — the handoff contract is broken
- Questionnaire SKILL.md says mark decisions "needs-confirmation" but the schema has no confidence/status field to carry that signal
- CREATE steps can't satisfy the rule "use exact paths from graph.json" since those files don't exist yet
- No fallback Phase value when no domain skill is loaded — plan can't satisfy "Phase on every step" rule
Internal contradictions:
- Execute frontmatter says "does not build" but body says "runs the build gate after each phase" — agent gets conflicting instructions
- Step 1c prescribes a hardcoded bash find command while Steps 1a/1b/1d correctly let the agent explore freely
Safety:
- The find command in questionnaire Step 1c includes *.env files — secrets could leak into agent context and committed artifacts
- Verify fix loop uses git add -A instead of adding the specific fixed file — risks committing build artifacts
|
|
||
| 1. Read `.konveyor/implementation.md` | ||
| 2. Scan `/opt/skills/` for skills with `tags: [domain]` in their frontmatter | ||
| 3. Read the domain skill's phases, modules, and references |
There was a problem hiding this comment.
nit: drop domain and lets refer it as skill's. we should also add it as optional
| ## Steps | ||
|
|
||
| ### Step 1: <title> | ||
| - Phase: <domain-skill-phase-name> |
There was a problem hiding this comment.
what if we dont have a domain skill?
|
|
||
| - `.konveyor/questionnaire.json` — decisions from prior stage | ||
| - `.konveyor/analysis.json` — Kantra rule violations and patterns (if present) | ||
| - Domain skills (`tags: [domain]`) — migration knowledge (phases, modules, references) |
djzager
left a comment
djzager
left a comment
There was a problem hiding this comment.
Review focused on how these stage skills line up with what's already merged — ADR 0010 (Skill Content Boundary), #136 (native goose discovery), and CONTEXT.md. Deliberately kept the still-proposed skill ADRs (0014 loading / #138, 0015 packaging / #141) out of scope: the rule-injection mechanism and bundle packaging are being worked out on those PRs, so nothing here rides on them.
Blocking: the four stage skills are added under harness/skills/ as diverging duplicates of the existing repo-root skills/ copies. Also flagged: skills carrying execution control that CONTEXT.md puts in the harness, a dead /opt/skills filesystem scan, generic stage instructions that won't reliably load skills post-#136, and a couple of open questions on token counting and the .konveyor commit contract.
| ## Startup | ||
|
|
||
| 1. Read `.konveyor/implementation.md` | ||
| 2. Scan `/opt/skills/` for skills with `tags: [domain]` in their frontmatter |
There was a problem hiding this comment.
Drop the /opt/skills scan. Scan /opt/skills/ for skills with tags:[domain] reads the container filesystem via the shell tool. Two problems, both on already-merged ground:
- ADR 0010 (Skill Content Boundary, merged) and
CONTEXT.md:246disallow "filesystem discovery" in skills. - It's factually dead — no skill declares
tags:[domain](skills/javaee-to-quarkus/SKILL.mdhas notagskey at all), so this matches nothing.
Since #136 landed native discovery (the ~/.agents/skills -> /opt/skills symlink in main.go), the domain skill's name+description are already surfaced to the model — so the scan is also redundant. Replace it with a named skill the model can load_skill plus relative paths for references/. Same pattern appears in plan/SKILL.md:43 and verify/SKILL.md.
| stages: | ||
| - name: questionnaire | ||
| agentRef: migration-questionnaire-agent | ||
| instructions: "Analyze the source application and produce .konveyor/questionnaire.json with detection results and migration decisions." |
There was a problem hiding this comment.
Name the skill in stage instructions. Since #136 the harness no longer concatenates skill bodies — goose discovers skills natively and the model chooses what to load_skill. With generic instructions like "Analyze the source application and produce .konveyor/questionnaire.json", the model may never load the stage skill or honor its output schema. Cheap, already-supported fix: name the skill in each stage's instructions, e.g. "Load the questionnaire skill and follow its instructions, then ...". (Guaranteeing load via always-injected rules is a separate mechanism still being worked out in ADR 0014 — not asking for that here.)
|
heads up that a few ADRs landed yesterday that move things under this PR. it opened Aug 4, they merged Aug 13, so this is the ground shifting rather than anything you did. the big overlap is 0010: its context section is written about the existing plan/execute/verify skills, and the copies here carry forward the same three things it rules out. it names the deeper one is that the those last two are still open and not accepted, so if the native-discovery model doesnt work for what these stages actually need, please weigh in on them - I would much rather hear it now than after they land. you have been closer to how these stages behave in practice than either of us. separately these land in the questionnaire skill is a real addition and the domain content reads well throughout, which is what 0010 says about these skills too. its the startup/discovery/git scaffolding that needs lifting out, and I think thats a big enough reshape to do before line-level review rather than after. |
| git add -A && git commit -m "Verify fix: <describe what was fixed>" | ||
| ``` | ||
|
|
||
| Repeat up to `KONVEYOR_PARAM_MAX_FIX_ITERATIONS` times (default 3). |
There was a problem hiding this comment.
0010 names this one specifically, and 0009 removed KONVEYOR_PARAM_* anyway. the guardrail is GOOSE_MAX_TURNS from the harness now. soft guidance like "try a couple of approaches before moving on" is still fine per 0010, its the counted cap that isnt.
|
|
||
| ## Phase 1: Detect | ||
|
|
||
| Analyze the source repository at `/workspace/` to build a tech summary. Do this by reading files — not by building or executing the project. |
There was a problem hiding this comment.
HARNESS_WORK_DIR is /workspace/repo, so this and the find below are a level up from the clone.
goose doesn't auto-load a skill's supporting files, so agents often skipped the separate templates/*.md and improvised the output. Move each schema into the SKILL.md body, which the harness always injects into the prompt, and delete the now-dead templates/ dirs. Verified 5/5 canary markers present in a full coolstore migration run. Signed-off-by: hhpatel14 <hitpatel@redhat.com>
4c91cc3 to
636abf1
Compare
|
@savitharaghunathan According to goose mechanism, per docs : goose loads a skill's name + description at startup and only pulls the full skill in when it judges the request relevant — the model decides. More importantly, a skill's supporting files (our templates/*.md) are never auto-loaded; goose's docs state the model must read them with file tools on demand. On our side, the harness inlines each SKILL.md body directly into the prompt (discoverSkills() function), but it never injects the templates. So the canary can only reach the output if the model actively goes and reads the template file. (The load skill · lines in the run logs are goose rendering that load action — its internal loadSkill, which an open goose discussion proposes renaming to load.) Results.
Conclusion |
…-skill # Conflicts: # hack/harness-test/setup.sh # harness/cmd/migration-harness/main.go # skills/execute/SKILL.md # skills/plan/SKILL.md # skills/verify/SKILL.md
…vely - Remove git/commit and KONVEYOR_PARAM_* control from stage skills (ADR 0010) - Name+load the skill in each stage instruction so goose discovers it (konveyor#136) - Fix questionnaire workdir path (/workspace/repo) and plan Inputs typo Signed-off-by: hhpatel14 <hitpatel@redhat.com>
Signed-off-by: hhpatel14 <hitpatel@redhat.com>
|
|
||
| ```markdown | ||
| ### Step 5: Migrate imports in <file> | ||
| - Phase: <domain-skill-phase-name> |
There was a problem hiding this comment.
what if domain skills are not available? would these steps get ignored in that case?
|
|
||
| 1. Read the error message to identify the file and issue | ||
| 2. Read the source file | ||
| 3. Consult domain skill's `references/verify-errors.md` for known error-fix mappings |
There was a problem hiding this comment.
I think we should assume that there might not be a domain skill. imo, we should leave it out and then add it later when it becomes necessary. what do you think?
There was a problem hiding this comment.
Removed the references/verify-errors.md reference (file doesn't exist yet). Kept the domain-skill reference but gated it behind an "if the domain skill is loaded" check — the skills handling covers the unloaded case fine, and prior runs already surface it as "not loaded" when the skill isn't found.
Signed-off-by: hhpatel14 <hitpatel@redhat.com>
|
@djzager — on the four-skill skeleton: the execution-control half is already resolved — I stripped git add/commit/"don't push" and the KONVEYOR_PARAM_* reads out of all four skills; committing is now solely a harness concern (prompt/prompt.go injects it into every stage) and the turn budget is GOOSE_MAX_TURNS. On de-duplicating the remaining output-contract prose, I'd hold off, based on what we found building these: we tested skill loading with canary markers, and supporting files don't reliably load — a templates/*.md is never auto-pulled (the model has to choose to read it, and often didn't), which is exactly why the schemas are inlined rather than in templates/. #136 the same gamble applies to a separate loadable skill (model only sees name+description, decides what to load_skill). So a shared skeleton skill/template would reintroduce the load-reliability problem inlining fixed. The only reliable de-dup routes are (1) harness prompt injection (deterministic, but prompt.Build() is generic across all agents so migration-specific text leaks) or (2) ADR 0014's guaranteed rule-injection, which is being designed for exactly this. Proposal: keep the output-contract prose inlined for now (small, guaranteed-loaded) and fold the de-dup into ADR 0014. Happy to file a tracking issue so it isn't lost. |
There was a problem hiding this comment.
The skills look good. Thanks, @hhpatel14 :)
LGTM from my side... anything else can be a followup
|
Here's the latest run as concrete evidence: https://github.com/hhpatel14/coolstore/tree/konveyor/migration-1787231869/.konveyor The verify stage never got to commit. Its SKILL.md declares it "Produces .konveyor/verify.json" and "You MUST write .konveyor/verify.json before finishing" — but the pod was OOMKilled (exit 137) partway through, during a blackbox mvn quarkus:dev. The agent had already applied 12 compile-fix edits in that stage, then the kill hit before it wrote verify.json and before the end-of-stage commit. Because the agent is the sole committer and commits only once at stage end, SIGKILL erased everything verify did — the file edits and the output artifact. The branch has zero trace of verify: no verify.json, no compile fixes, no failed-stage record. The watcher reported push: success throughout, because it only pushes committed history and there was nothing committed to push. This is exactly the work-loss gap tracked in #166 |
5 participants
Four stage skills for the migration pipeline:
Also adds token usage logging via ACP PromptResult in the harness.
Summary by CodeRabbit
New Features
Documentation