feat(skills): add add-workload-type authoring skill - #187
Open
rogirun wants to merge 12 commits into
Open
Conversation
Add an Agent Skill that guides authoring and validating a Karta definition for a new Kubernetes workload type. Bundles the authoring workflow, a technical-guide cheatsheet, a sample index, a troubleshooting catalog, and three eval prompts. Content is grounded in the API types and validator. Signed-off-by: Roee Gil <roee.gil@run.ai>
Replace the prose eval-prompts.md with evals/evals.json in the skill-creator schema (id, prompt, expected_output, expectations) so the add-workload-type skill is consumable by the skill-creator eval and benchmark tooling. Each eval carries verifiable expectation assertions rather than a golden output. Signed-off-by: Roee Gil <roee.gil@run.ai>
Close the gaps surfaced by testing the skill against all committed samples: role-label keys are operator-specific (replica-type vs job-role), disambiguate roles that share a label via key existence, use byExpression when a controller has no phase/conditions, note a single matcher may AND its kinds, and allow intentional additionalChildKinds duplication for RBAC/owner traversal. Signed-off-by: Roee Gil <roee.gil@run.ai>
rogirun
marked this pull request as ready for review
July 27, 2026 13:24
rogirun
requested review from
AviadHayumi,
Isan-Rivkin,
ronlv10,
shaked-bouktus and
yuval-gr
as code owners
July 27, 2026 13:24
Contributor
|
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 the packaged ChangesAdd workload type authoring skill
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Author
participant Skill
participant Verifier
participant WorkloadCR
Author->>Skill: Gather facts and select a matching sample
Skill->>Author: Define jq paths and status mappings
Author->>Verifier: Submit definition, workload, and predictions
Verifier->>WorkloadCR: Parse real workload data
Verifier->>Author: Return observations, mismatches, and warnings
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
skills/add-workload-type/SKILL.md (1)
134-137: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope selector exclusivity to the selector kind.
This says all pod selectors must be mutually exclusive, while
technical-guide.mdlimits the rule to selectors of the same kind. Align the wording with the validator semantics so agents do not over-constrain valid instance or replica configurations.🤖 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 `@skills/add-workload-type/SKILL.md` around lines 134 - 137, Update the pod-selector guidance in the workload-type instructions so mutual exclusivity is required only among selectors of the same selector kind, matching the validator semantics. Preserve the existing pod-field and role-label key-existence guidance, and explicitly allow selectors of different kinds to coexist when valid.
🤖 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 `@skills/add-workload-type/reference/sample-index.md`:
- Around line 11-13: Update the “How to choose” guidance to select the
most-specific matching sample rather than simply taking the first matching row,
ensuring specialized patterns such as Ray workloads with instanceIdPath are
preferred over generic role-based samples.
In `@skills/add-workload-type/reference/technical-guide.md`:
- Around line 236-239: Qualify the additionalChildKinds guidance to prohibit
duplicate kinds only when they are not intentionally included for RBAC or owner
traversal. Apply this aligned wording in
skills/add-workload-type/reference/technical-guide.md lines 236-239 and
skills/add-workload-type/reference/troubleshooting.md lines 74-79.
---
Nitpick comments:
In `@skills/add-workload-type/SKILL.md`:
- Around line 134-137: Update the pod-selector guidance in the workload-type
instructions so mutual exclusivity is required only among selectors of the same
selector kind, matching the validator semantics. Preserve the existing pod-field
and role-label key-existence guidance, and explicitly allow selectors of
different kinds to coexist when valid.
🪄 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: Enterprise
Run ID: 68155ce7-748f-4c43-9273-343f9b38e104
📒 Files selected for processing (5)
skills/add-workload-type/SKILL.mdskills/add-workload-type/evals/evals.jsonskills/add-workload-type/reference/sample-index.mdskills/add-workload-type/reference/technical-guide.mdskills/add-workload-type/reference/troubleshooting.md
5 tasks
…d kinds Address CodeRabbit review on #187: - Scope pod-selector mutual exclusivity to the same selector kind (different kinds may coexist on a component); note it is an authoring guideline, not a validator check. Aligned in SKILL.md and the technical-guide checklist. - sample-index: choose the most specific matching row rather than the first, so multi-instance patterns (e.g. Ray worker groups needing instanceIdPath) win over a generic role-based row. - additionalChildKinds: allow intentional duplicates for RBAC or owner traversal, aligning the technical-guide prose and checklist with troubleshooting.md. Signed-off-by: Roee Gil <roee.gil@run.ai>
AviadHayumi
requested changes
Jul 29, 2026
Contributor
There was a problem hiding this comment.
nice skill , the technical parts all match the code
my main thing , it never tests the definition on a real CR.
you write the yaml and check it by eye , but nothing catches a jq path that passes validation and still points at a field the CR doesnt have.
so id add:
- step 1 : asks the user for the CRD and a real example CR
- step 2 : runs the definition on that CR and prints what it pulled out , so you see the fields match
docs/examples/quickstart/main.go already does this , its just hardcoded to jobset and lws. give it a --karta and --workload flag and it would be awesome
btw :
I think the best way to test the skill before the E2E flow is merged is to start a fresh session, remove the known Karta definition from both the catalog and code, and provide only the CRD and a real example CR (e.g., PyTorchJob).
Then let the skill generate the Karta definition and compare it to our known-good version. This will help ensure no fields are missed and reveal if the skill instructions need updates.
The skill validated structure but never exercised a definition, so a jq path could pass every check and still resolve to null against a real object. Step 1 now asks the user for the CRD and at least one real example CR, step 6 is scoped explicitly to structure, and a new step 7 runs the definition through the offline quickstart and gates completion on the extracted values (status not Undefined, every spec-bearing component yields a pod template, instance keys and replica counts match the CR). Addresses review feedback on #187. Signed-off-by: Roee Gil <roee.gil@run.ai>
Blind-authoring six definitions with the skill and diffing them against docs/catalog surfaced guidance the guide was missing. All six passed KartaValidator and still contained defects. - Scale: state the rule as the count at the component's level of the tree, not objects of its kind (LWS leader has kind StatefulSet and resolves to the group count). Add nested multiplication, two self-checks, and a prompt to look for autoscaling bounds. - Virtual components: document the kind-less grouping node that carries a scaleDefinition and replicaSelector, using the LWS group level. - fragmentedPodSpecDefinition: a CRD can embed a real podSpec and still need fragmented paths when labels and annotations sit a level up (Grove). Document read-only projections and permit `as $x` bindings. - additionalChildKinds: give a rule for component vs additional kind. - Gang scheduling: document the deprecated podGroups format with groupByKeyPaths and filters, which every catalog definition uses and the guide did not mention. - Checklist: five new entries, including declared-but-unreferenced conditionsDefinition. Relates to #186. Signed-off-by: Roee Gil <roee.gil@run.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/add-workload-type/SKILL.md (1)
88-95: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign fragmented-path guidance across both references.
The two files use different rules for non-assignable fragmented paths. Use one contract: require assignable paths when pod-spec mutation is supported, and permit read-only paths only when the mutation limitation is documented.
skills/add-workload-type/SKILL.md#L88-L95: replace the absolute assignability requirement with the mutable/read-only rule and link to the technical guide.skills/add-workload-type/reference/technical-guide.md#L105-L123: retain the read-only exception and define the required mutation limitation.🤖 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 `@skills/add-workload-type/SKILL.md` around lines 88 - 95, The fragmentedPodSpecDefinition guidance is inconsistent about non-assignable paths. In skills/add-workload-type/SKILL.md lines 88-95, replace the absolute assignability requirement with the rule that mutation-capable pod specs require assignable paths, while read-only paths are allowed only when their mutation limitation is documented, and link to the technical guide. In skills/add-workload-type/reference/technical-guide.md lines 105-123, retain the read-only exception and explicitly define the required mutation limitation so both references share the same contract.
🤖 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 `@skills/add-workload-type/reference/technical-guide.md`:
- Around line 220-223: Update the technical guide’s scale examples to document
positional pairing between scale results and instanceIdPath: replicasPath,
minReplicasPath, and maxReplicasPath must emit one value for each instance. In
the JobSet example, add the corresponding instanceIdPath, or explicitly use an
aggregate when the count is scalar.
In `@skills/add-workload-type/SKILL.md`:
- Around line 50-59: Update the add-workload-type workflow to make at least one
real custom resource a required input before producing a completed definition.
Use the existing CRD and CR inputs and quickstart validation step to verify the
definition against that resource; if draft-only continuation remains supported,
clearly label the output as unverified rather than presenting it as complete.
---
Outside diff comments:
In `@skills/add-workload-type/SKILL.md`:
- Around line 88-95: The fragmentedPodSpecDefinition guidance is inconsistent
about non-assignable paths. In skills/add-workload-type/SKILL.md lines 88-95,
replace the absolute assignability requirement with the rule that
mutation-capable pod specs require assignable paths, while read-only paths are
allowed only when their mutation limitation is documented, and link to the
technical guide. In skills/add-workload-type/reference/technical-guide.md lines
105-123, retain the read-only exception and explicitly define the required
mutation limitation so both references share the same contract.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: dfef5d8f-71cc-419b-9656-5dbb2dc0b776
📒 Files selected for processing (2)
skills/add-workload-type/SKILL.mdskills/add-workload-type/reference/technical-guide.md
Comment on lines
+220
to
+223
| `(.spec.replicas // 1) * ((.spec.leaderWorkerTemplate.size // 1) - 1)`. A nested | ||
| level multiplies by its parent's count the same way: JobSet's `replicatedjob` | ||
| uses `.spec.replicatedJobs[] | .replicas * .template.spec.parallelism`. | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 10 'type ScaleDefinition|ReplicasPath|replicasPath|minReplicasPath|maxReplicasPath' pkg
rg -n -C 8 'replicatedJobs|autoScalingConfig|instanceIdPath|replicatedjob' docs pkgRepository: run-ai/karta
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- resource accessor scale extraction ---'
sed -n '1,180p' pkg/resource/accessor.go
printf '%s\n' '--- extraction helpers and instance construction ---'
rg -n -C 12 'func extract|safeGetByIndex|InstanceIdPath|instanceId|Scale\{' pkg/resource pkg/tree pkg/instructions
printf '%s\n' '--- array-scale tests ---'
sed -n '500,575p' pkg/instructions/gang_scheduling_test.go
printf '%s\n' '--- technical guide target lines ---'
sed -n '205,240p' skills/add-workload-type/reference/technical-guide.mdRepository: run-ai/karta
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- technical guide examples ---'
sed -n '200,240p' skills/add-workload-type/reference/technical-guide.md
printf '%s\n' '--- scale aggregation by definition ---'
sed -n '100,175p' pkg/instructions/gang_scheduling.go
printf '%s\n' '--- component factory instance and scale methods ---'
rg -n -C 8 'func .*GetScale|ExtractScale|GetInstanceIds|HasInstanceIdDefinition|type Component' pkg/resourceRepository: run-ai/karta
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- scale-to-instance mapping ---'
sed -n '200,250p' pkg/resource/component.go
sed -n '250,330p' pkg/resource/component.go
printf '%s\n' '--- guide instance and scale contract ---'
rg -n -C 12 'InstanceIdPath|instanceIdPath|ScaleDefinition|replicasPath|replicatedJobs' skills/add-workload-type/reference/technical-guide.mdRepository: run-ai/karta
Length of output: 15311
Document positional scale pairing for array components. The runtime pairs each scale result by index with instanceIdPath. State that replicasPath, minReplicasPath, and maxReplicasPath must emit one value per instance. Add the matching instanceIdPath to the JobSet example, or use an explicit aggregate for a scalar count.
🤖 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 `@skills/add-workload-type/reference/technical-guide.md` around lines 220 -
223, Update the technical guide’s scale examples to document positional pairing
between scale results and instanceIdPath: replicasPath, minReplicasPath, and
maxReplicasPath must emit one value for each instance. In the JobSet example,
add the corresponding instanceIdPath, or explicitly use an aggregate when the
count is scalar.
Contributor
Author
There was a problem hiding this comment.
those question and more are addressed on a future PR
Contributor
There was a problem hiding this comment.
@rogirun, acknowledged. This documentation detail can be addressed in the future PR.
Would you like me to create a follow-up GitHub issue for the positional pairing requirement between scale paths and instanceIdPath?
(_/)
You are interacting with an AI system.
AviadHayumi
requested changes
Aug 6, 2026
AviadHayumi
left a comment
AviadHayumi
left a comment
Contributor
There was a problem hiding this comment.
looks amazing.
only one thing missing , if the user gave a real CR we should actually use it, not just eyeball it. take the CR + the karta and check:
the karta is verified , runs clean against that CR
add a small main (can live under the skill) that:
- gets the karta and just validate
- has the llm predict the result first
- runs it and dumps the actual result
- checks predicted == dump
that way the definition is proven against a real object, not just structurally valid.
Structural validation cannot see a jq path that resolves to null against a real object, so a definition could pass every check and still extract nothing. Step 7 told the agent to run the definition but gave it no way to, beyond copying the quickstart to a scratch directory and repointing the embedded manifest. Add scripts/verify/, bundled with the skill: it validates the Karta, builds the tree from a real manifest, prints the extracted status, replica counts and containers per component instance, and checks them against values predicted before the run. Predictions are partial, so one may cover a single component or field. Warnings name the valid-but-empty cases the validator cannot see: unresolved status, a specDefinition that extracted no pod spec, a pod spec with no containers, an instanceIdPath that matched nothing. Exit codes are 0 clean, 1 load or validation failure, 2 prediction mismatch, 3 warnings under --strict. Predicting first is the point. Reading the output afterwards invites accepting whatever appears; a prediction that disagrees with the extraction is a defect that cannot be talked away. Step 7 now requires deriving expected values from the CR rather than from an existing definition, and forbids editing the prediction to make a run pass. Verified against definitions already in the repository. The LeaderWorkerSet catalog definition run against docs/examples/quickstart/lws.yaml reports group=4 while its sibling leader=3, from a manifest declaring replicas 3 and size 4, which is the sibling-consistency failure described in the technical guide. Addresses review feedback on #187. Signed-off-by: Roee Gil <roee.gil@run.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@skills/add-workload-type/scripts/verify/main.go`:
- Around line 52-69: Rename the type declarations observation and component to
Observation and Component, and update every reference to these types throughout
the verification code, including fields such as observation.Components and any
parsing or comparison logic. Preserve the existing JSON tags and behavior.
- Around line 213-217: Extend the warning traversal around the wt.Children loop
to recursively inspect every node’s Children, including nested components under
each instance, and append the same no-instances warning whenever a nested node
has empty Instances. Ensure this complete traversal runs before the strict-mode
exit, and add a test covering a nested instanceIdPath that matches nothing.
- Around line 252-253: Update the mismatch guidance in the verification output
near the mismatch-reporting logic to direct users to fix the definition or
independently re-derive expected values from the manifest before rerunning,
rather than suggesting they edit the prediction based on extracted output.
Preserve the existing mismatch count and verification behavior.
In `@skills/add-workload-type/scripts/verify/README.md`:
- Around line 75-77: Update the technical guide reference in the README's
scale-section example from reference/technical-guide.md to
../../reference/technical-guide.md, matching the guide's location relative to
the verify directory.
In `@skills/add-workload-type/SKILL.md`:
- Around line 172-175: Fix the final sentence in the verification instructions
to be grammatical and explicitly identify scripts/verify/README.md as the
reference for flags and predictions format. Keep the surrounding description of
scripts/verify unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 4c119b9a-b0d7-49fe-a0f1-0c3dce80bf94
⛔ Files ignored due to path filters (1)
skills/add-workload-type/scripts/verify/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
skills/add-workload-type/SKILL.mdskills/add-workload-type/scripts/verify/README.mdskills/add-workload-type/scripts/verify/go.modskills/add-workload-type/scripts/verify/main.go
Comment on lines
+213
to
+217
| for _, node := range wt.Children { | ||
| if len(node.Instances) == 0 { | ||
| warnings = append(warnings, fmt.Sprintf("%s produced no instances: instanceIdPath matched nothing", node.Name)) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check nested components for empty instance extraction.
This loop checks only top-level nodes in wt.Children. Nested nodes exist below inst.Children, but the warning walk never evaluates them. If a nested instanceIdPath matches nothing, the command emits no warning and --strict exits successfully.
Traverse each child level before the strict-mode check. Add a nested-component test for this case.
🤖 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 `@skills/add-workload-type/scripts/verify/main.go` around lines 213 - 217,
Extend the warning traversal around the wt.Children loop to recursively inspect
every node’s Children, including nested components under each instance, and
append the same no-instances warning whenever a nested node has empty Instances.
Ensure this complete traversal runs before the strict-mode exit, and add a test
covering a nested instanceIdPath that matches nothing.
Comment on lines
+252
to
+253
| fmt.Printf("\n%d mismatch(es). The definition does not do what it was expected to do;\n"+ | ||
| "fix the path or the prediction, then run again.\n", len(mismatches)) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Preserve the prediction-before-execution rule.
The mismatch message tells users to “fix the path or the prediction” after extraction. This permits output-driven prediction edits that make a failed check pass. Direct users to fix the definition, or independently re-derive the expected values from the manifest before rerunning.
Proposed fix
- "fix the path or the prediction, then run again.\n", len(mismatches))
+ "fix the definition, or independently re-derive the prediction from the manifest before rerunning.\n", len(mismatches))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fmt.Printf("\n%d mismatch(es). The definition does not do what it was expected to do;\n"+ | |
| "fix the path or the prediction, then run again.\n", len(mismatches)) | |
| fmt.Printf("\n%d mismatch(es). The definition does not do what it was expected to do;\n"+ | |
| "fix the definition, or independently re-derive the prediction from the manifest before rerunning.\n", len(mismatches)) |
🤖 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 `@skills/add-workload-type/scripts/verify/main.go` around lines 252 - 253,
Update the mismatch guidance in the verification output near the
mismatch-reporting logic to direct users to fix the definition or independently
re-derive expected values from the manifest before rerunning, rather than
suggesting they edit the prediction based on extracted output. Preserve the
existing mismatch count and verification behavior.
Comment on lines
+75
to
+77
| The manifest declares `replicas: 3` and `size: 4`. The `group` component reports | ||
| 4 while its sibling `leader` reports 3, which is the sibling-consistency failure | ||
| described in the scale section of `reference/technical-guide.md`. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the technical guide path from this README.
reference/technical-guide.md resolves below skills/add-workload-type/scripts/verify/. The guide is at skills/add-workload-type/reference/technical-guide.md. Replace the displayed path with ../../reference/technical-guide.md.
🤖 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 `@skills/add-workload-type/scripts/verify/README.md` around lines 75 - 77,
Update the technical guide reference in the README's scale-section example from
reference/technical-guide.md to ../../reference/technical-guide.md, matching the
guide's location relative to the verify directory.
Step 7 gated completion on running the definition against a real CR with a predictions file. That is too strong: a real CR is often unavailable, and the step it displaced was the one every definition should pass. The harness already validated before extracting, but that path could not be reached without also passing --workload, so validation was only available as a side effect of the full run. Make --workload optional. With --karta alone the command runs KartaValidator, reports the result, and says plainly that the paths remain unproven. The extraction flags now require --workload rather than being ignored without it, so a dropped flag cannot exit 0 having checked nothing it was asked to check. Step 6 becomes "Validate the definition" and always runs the validator, with the checklist split into what the validator enforces and what only the author can judge. It notes that gang-scheduling componentName references are checked only in the deprecated podGroups format, leaving podGroup.subGroups to a manual look. Step 7 keeps the predict-first protocol but applies only when a CR was supplied; without one, the skill must state that the definition is valid and unexercised. Step 1 asks for the CR as optional rather than as a precondition. Relates to #186. Signed-off-by: Roee Gil <roee.gil@run.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/add-workload-type/scripts/verify/README.md`:
- Around line 17-18: Update the validation result description in the README to
use the hyphenated compound adjective “well-formed” when describing a valid
definition; leave the surrounding behavior and wording unchanged.
In `@skills/add-workload-type/SKILL.md`:
- Around line 149-150: Update the validation prose in SKILL.md to use
“well-formed” at the referenced sentence, and replace “These it cannot check.”
with “The validator cannot check these.” Preserve the surrounding Markdown
content and meaning.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 70feea72-fbe0-46b5-ab65-0427b6720d51
📒 Files selected for processing (3)
skills/add-workload-type/SKILL.mdskills/add-workload-type/scripts/verify/README.mdskills/add-workload-type/scripts/verify/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/add-workload-type/scripts/verify/main.go
Comment on lines
+17
to
+18
| It runs `KartaValidator` and exits 0 when the definition is well formed, or 1 | ||
| with the validator's message otherwise. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use well-formed in the validation result description.
Hyphenate the compound adjective in well-formed.
🧰 Tools
🪛 LanguageTool
[grammar] ~17-~17: Use a hyphen to join words.
Context: ... and exits 0 when the definition is well formed, or 1 with the validator's messag...
(QB_NEW_EN_HYPHEN)
🤖 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 `@skills/add-workload-type/scripts/verify/README.md` around lines 17 - 18,
Update the validation result description in the README to use the hyphenated
compound adjective “well-formed” when describing a valid definition; leave the
surrounding behavior and wording unchanged.
Source: Linters/SAST tools
AviadHayumi
reviewed
Aug 6, 2026
| | `... failed validation: del function is not allowed` | Uses `del`. | Read, do not modify. | | ||
| | `... failed validation: recursive descent operator '..' is not allowed` | Uses `..`. | Spell out the absolute path instead. | | ||
| | `... failed validation: function '<name>' may produce excessive output and is not allowed` | Uses `range`, `paths`, `recurse`, `walk`, or `repeat`. | Address the fields directly with a bounded expression. | | ||
|
|
Contributor
There was a problem hiding this comment.
i dont think we need here dedicated go.mod go.sum
Contributor
Author
There was a problem hiding this comment.
Removed. Script moved to the hack folder
AviadHayumi
reviewed
Aug 6, 2026
| @@ -0,0 +1,384 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
| // Copyright (c) 2026 NVIDIA Corporation | |||
Contributor
There was a problem hiding this comment.
i would put it under /hack folder
and I think when we will have karta cli it would be much better and we will delete this file
karta validate ./my-karta.yaml
karta describe -f ./job.yaml # describe an unsubmitted manifest, no cluster
AviadHayumi
requested changes
Aug 6, 2026
AviadHayumi
reviewed
Aug 6, 2026
| # Sample index | ||
|
|
||
| Pick the sample whose shape is closest to the target workload, copy it, and | ||
| adapt the GVK, paths, and status mapping. Samples live in `docs/samples/`. |
Contributor
There was a problem hiding this comment.
i think we can update this part it lives under catalog
same for line 39 and 80
AviadHayumi
reviewed
Aug 6, 2026
| | Several worker groups under one role that need per-group identity | `docs/samples/raycluster.yaml`, `docs/samples/rayjob.yaml`, `docs/samples/rayservice.yaml` | Worker component uses `instanceIdPath` on `workerGroupSpecs[].groupName` paired with a `componentInstanceSelector`. | | ||
| | Replicated jobs, each a named template instance | `docs/samples/jobset.yaml` | `replicatedjob` child with `instanceIdPath: .spec.replicatedJobs[].name` and a `componentInstanceSelector`. | | ||
| | Nested ownership with identical replicated sub-structures (a group that owns a leader and workers) | `docs/samples/lws.yaml` | `group` child owns `leader` and `worker`; `replicaSelector` on the group; `componentTypeSelector` distinguishes leader from worker. | | ||
| | Fields scattered across the spec, status via a phase string | `docs/samples/dynamo.yaml`, `docs/samples/dynamo-v1beta1.yaml`, `docs/samples/nimservice.yaml`, `docs/samples/nimcache.yaml` | `fragmentedPodSpecDefinition` with per-field paths; `phaseDefinition` plus `byPhase` mappings. | |
Contributor
There was a problem hiding this comment.
33-39 need to be updated
AviadHayumi
requested changes
Aug 6, 2026
Review feedback on #187. Move scripts/verify to hack/karta-verify and drop its dedicated go.mod and go.sum. The script now belongs to the root module, so go build ./..., go vet ./... and make check cover it; a nested module only made sense for docs/examples/quickstart, which demonstrates external consumption through a replace directive. Invocations become go run ./hack/karta-verify from the repository root. The script is expected to be deleted once the Karta CLI can run karta validate. sample-index and SKILL.md still pointed at docs/samples, which no longer exists. Repoint every row at the generated docs/catalog definition, mapping each name to its real filename (pytorch.yaml becomes kubeflow-org-pytorchjob-v1.yaml, and so on). All 18 referenced files were checked to exist. Also from review: - Collect the no-instances warning during the tree walk instead of looping only wt.Children, so a nested component whose instanceIdPath matches nothing is reported rather than passing under --strict. Reachable only for a nested component with neither a spec nor a scale definition; the builder hard-errors on the other shapes. - Reword the mismatch output so it directs the reader to fix the definition or re-derive the prediction from the manifest, never to copy extracted values back into the prediction. - Hyphenate well-formed and reword a checklist sentence. The PascalCase suggestion was skipped: the types are internal to the script and stay unexported. Relates to #186. Signed-off-by: Roee Gil <roee.gil@run.ai>
The package doc repeated the usage, flag list, and rationale already carried by README.md and flag.PrintDefaults, and several inline comments restated the code below them. Keep only the ones explaining a why the code does not show: the refusal of extraction flags without --workload, validating before building, collecting no-instance nodes during the walk, the resolver reporting Undefined explicitly, and the Key format callers need to write a prediction. Comments only where the why is non-obvious, per AGENTS.md. No behavior change. Signed-off-by: Roee Gil <roee.gil@run.ai>
staticcheck ST1005: error strings should not be capitalized. Surfaced once the script joined the root module in 4ae8cd5; while it had its own go.mod, golangci-lint never saw it. Signed-off-by: Roee Gil <roee.gil@run.ai>
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.
What does this PR do?
Adds an Anthropic Agent Skill,
add-workload-type, underskills/, that guidesan agent through authoring and validating a Karta definition for a new Kubernetes
workload type. It bundles a 6-step authoring workflow (
SKILL.md), atechnical-guide cheatsheet, a sample index that maps a workload shape to the
closest sample in
docs/samples/, a troubleshooting catalog, and three evalprompts. All content is grounded in the API types (
pkg/api/runai/v1alpha1/) andthe validator, so it does not teach fields or rules that do not exist. A
dogfooded Argo Workflow definition passes
KartaValidator.Related issue(s)
Fixes #186
Checklist
git commit -s)make check)Summary by CodeRabbit
add-workload-typeskill to guide authoring and validation of Karta definitions for Kubernetes workload types.