feat(#6681): inject harness skills into agent frontmatter for always-on activation - #6859
feat(#6681): inject harness skills into agent frontmatter for always-on activation#6859fullsend-ai-coder[bot] wants to merge 15 commits into
Conversation
|
🤖 Finished Review · ✅ Success · Started 3:54 PM UTC · Completed 4:11 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.84 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Risk Assessment: moderate (2/5) DetailsAnchored at prior score 2/moderate: Tier1 metadata remains materially unchanged (6 files, 880 vs 856 lines, large blast radius, 0.33 test ratio, bot authorship, no protected/security/CI/dependency changes), Tier2 churn/fix-revert history on the pre-existing files (claude.go, pi_bootstrap.go) remains elevated but matches the previously observed pattern, and Tier3 issue-linkage signals show a scoped, well-matched feature request with no risk labels or unresolved discussion; the weighted composite confirms no specific new signal has appeared since the prior review to justify moving off 2/moderate. Previous runRisk Assessment: moderate (2/5) DetailsAnchored at prior score 2/moderate: Tier1 metadata is unchanged in kind (same 6 files, 856 vs 851 lines, large blast radius, 0.33 test ratio, bot/human-mixed authorship, no protected/security/CI/dependency changes) giving weighted composite (1.630.5 + 2.50.3 + 2.5*0.2 ~= 2.06 -> 2); the single new commit since the prior review (80ffc8a) is a human-authored internal rewrite of frontmatter-skills parsing with added test coverage rather than a scope or risk-profile change, and while claude.go/pi_bootstrap.go continue to show elevated churn and fix/revert history, this is the same pattern already reflected in the prior assessment, so no specific new signal justifies moving off 2/moderate. Previous run (2)Risk Assessment: moderate (2/5) DetailsWeighted composite (Tier1 1.630.5 + Tier2 2.290.3 + Tier3 2.5*0.2 ~= 2.0) matches the prior anchored score of 2/moderate: Tier 1 metadata is essentially unchanged (863 vs 851 lines, same large blast radius, 0.33 test ratio, bot author, no protected/security/CI/dependency changes); Tier 2 still shows elevated churn and fix/revert history on claude.go and pi_bootstrap.go but the bulk of the diff (823 lines) remains two net-new, well-tested files; Tier 3s well-scoped, closely-matching issue is offset slightly by the lack of a feature flag guarding the new always-on skill-injection behavior, keeping overall risk at 2/moderate. Previous run (3)Risk Assessment: moderate (2/5) DetailsTier 1 metadata is materially unchanged from the prior assessment (851 vs 808 lines, same large blast radius, 0.33 test ratio, bot author, no protected/security/CI/dependency changes); Tier 2 shows the same elevated churn on claude.go and pi_bootstrap.go with fix/revert history, but the bulk of the diff remains two net-new, well-tested files, and Tier 3's well-scoped, matching issue continues to offset risk, so the score is anchored to the prior 2/moderate. Previous run (4)Risk Assessment: moderate (2/5) DetailsTier 1 metadata shows line count at 808 and large blast radius elevating the change-size dimension, but all other Tier 1 dimensions remain low (33% test ratio, bot author, no protected/security/CI/dependency changes). Tier 2 git history on claude.go remains elevated (high churn, 8 authors, fix commits) but the actual modification is small with the bulk being two net-new files. Tier 3 linked issue #6681 is well-scoped and PR scope matches. Anchored to prior assessment of 2/moderate. Previous run (5)Risk Assessment: moderate (2/5) DetailsTier 1 metadata shows elevated line count (763) and large blast radius raising the change-size dimension to 5, but all other Tier 1 dimensions score 1 (33% test ratio, bot author, no protected/security/CI/dependency changes). Tier 2 git history on claude.go remains elevated (high churn, 8 authors, fix commits) but the actual modification is small with the bulk being two net-new files. Tier 3 linked issue is well-scoped and PR scope matches. Anchored to prior assessment. Composite holds at 2/moderate. Previous run (6)Risk Assessment: moderate (2/5) DetailsTier 1 metadata shows elevated line count (659) and large blast radius raising the change-size dimension, but all other Tier 1 dimensions score 1 (50% test ratio, bot author, no protected/security/CI/dependency changes). Tier 2 git history on claude.go remains elevated (high churn, 8 authors, fix commits) but the actual modification is small with the bulk being two net-new files. Tier 3 linked issue is well-scoped. Composite holds at 2/moderate, consistent with prior assessment. Previous run (7)Risk Assessment: moderate (2/5) DetailsTier 1 metadata shows elevated line count (659) and large blast radius raising the change-size dimension, but all other Tier 1 dimensions score 1 (50% test ratio, bot author, no protected/security/CI/dependency changes). Tier 2 git history on claude.go remains elevated (high churn, 8 authors, fix commits) but the actual modification is small with the bulk being two net-new files. Tier 3 linked issue is well-scoped. Composite holds at 2/moderate, consistent with prior assessment. Previous run (8)Risk Assessment: moderate (2/5) DetailsTier 1 metadata shows increased line count (640) and large blast radius raising the change-size dimension, but all other Tier 1 dimensions score 1 (50% test ratio, bot author, no protected/security/CI/dependency changes). Tier 2 git history on claude.go remains elevated (high churn, 8 authors, fix commits) but the actual change is small with the bulk being two net-new files. Tier 3 linked issue is well-scoped. Composite holds at 2/moderate, consistent with prior assessment. Previous run (9)Risk Assessment: moderate (2/5) DetailsUnchanged from prior assessment: well-scoped additive feature (2 new files, minor edits to 2 existing) with strong test coverage (50% test ratio) from a known bot author; elevated git history signals on claude.go (high churn, 8 authors, fix commits in 90d) are offset by low security/dependency/CI impact and mostly-new-file composition, holding the composite at moderate risk. Previous run (10)Risk Assessment: moderate (2/5) DetailsUnchanged from prior assessment: well-scoped additive feature (2 new files, minor edits to 2 existing) with strong test coverage (50% test ratio) from a known bot author; elevated git history signals on claude.go (high churn, 8 authors, 16 fix commits in 90d) are offset by low security/dependency/CI impact and mostly-new-file composition, holding the composite at moderate risk. Previous run (11)Risk Assessment: moderate (2/5) DetailsModerate risk: well-scoped additive feature with strong test coverage (50% test ratio) from a known bot author, but modifies claude.go which is a high-churn hotspot with significant multi-author contention and regression history. |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Next steps:
Next steps:
Previous run (4)ReviewFindingsLow
Next steps:
Previous run (5)ReviewFindingsLow
Previous run (6)ReviewFindingsHigh
Low
Next steps:
Previous run (7)ReviewFindingsLow
Next steps:
Previous run (8)ReviewFindingsLow
Next steps:
Previous run (9)ReviewFindingsLow
Previous run (10)ReviewFindingsLow
Next steps:
Previous run (11)ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 4:13 PM UTC · Completed 4:22 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.22 |
- Handle YAML flow-style skills values (e.g. `skills: [a, b]`) by detecting `skills:` prefix and converting to block form in output - Skip YAML comments inside the skills block to avoid premature block-end detection - Unexport InjectFrontmatterSkills → injectFrontmatterSkills to match package convention (only called within runtime package) - Replace custom indexOf/countOccurrences test helpers with strings.Index/strings.Count from stdlib - Add test coverage for flow-style skills, empty flow-style, and comments within skills blocks Addresses #6859
🔧 Fix agent — iteration 1 (bot-triggered)Addressed all four review findings: fixed flow-style YAML skills detection (medium), fixed comment handling in skills block (low), unexported the function per package convention (low), and replaced custom test helpers with stdlib equivalents (low). Added three new test cases covering flow-style skills, empty flow-style, and comments within skills blocks. All 13 tests pass. Fixed (4):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 4:24 PM UTC · Completed 4:42 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.60 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 4:44 PM UTC · Completed 4:51 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.53 |
Deduplicate and sort newNames before the no-frontmatter early-return path so duplicate skill basenames are not emitted. Rename test functions from Test_injectFrontmatterSkills_* to TestInjectFrontmatterSkills_* to match the package's PascalCase convention. Drop the redundant "inject frontmatter skills: " prefix from inner error messages to avoid stuttering when wrapped by the call site. Add a test for the no-frontmatter dedup case. Addresses #6859
🔧 Fix agent — iteration 2 (bot-triggered)Addressed all three review findings: deduplicate and sort skill names before the no-frontmatter path, rename test functions to match PascalCase convention, and remove stuttered error message prefixes. Fixed (3):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 4:53 PM UTC · Completed 5:11 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.02 |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 11:28 AM UTC · Completed 11:44 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.25 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
ready-for-merge
|
🤖 Finished Fix · ✅ Success · Started 11:46 AM UTC · Completed 11:53 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.77 |
…on activation Skills listed in a harness skills: configuration are uploaded into the sandbox but may not reliably activate because the agent definition's frontmatter does not list them. This causes team behavior-modifier skills to silently fail to apply. Add InjectFrontmatterSkills to dynamically inject harness-listed skill names into the agent prompt's YAML frontmatter skills: section during ClaudeRuntime.Bootstrap. Existing frontmatter entries are preserved, new names are appended with deduplication by basename, and a skills: section is created if absent. The injection happens before the agent file is uploaded to the sandbox, ensuring Claude Code loads the skills without requiring an explicit Skill tool call in the prompt body. Files changed: - internal/runtime/frontmatter_skills.go: new InjectFrontmatterSkills function with YAML-aware frontmatter parsing and injection - internal/runtime/claude.go: call InjectFrontmatterSkills in Bootstrap before uploading the agent definition; read agent file once and use uploadBytes instead of UploadFile - internal/runtime/claude_test.go: fix test that passed a directory as agent path (now correctly uses a file) - internal/runtime/frontmatter_skills_test.go: comprehensive tests covering dedup, no-frontmatter, no-skills-section, BOM, ordering Note: pre-commit could not run in sandbox (network-restricted); local hooks (gofmt, go vet) passed. go test and go build passed. Closes #6681 Signed-off-by: Adam Scerra <ascerra@redhat.com>
- Handle YAML flow-style skills values (e.g. `skills: [a, b]`) by detecting `skills:` prefix and converting to block form in output - Skip YAML comments inside the skills block to avoid premature block-end detection - Unexport InjectFrontmatterSkills → injectFrontmatterSkills to match package convention (only called within runtime package) - Replace custom indexOf/countOccurrences test helpers with strings.Index/strings.Count from stdlib - Add test coverage for flow-style skills, empty flow-style, and comments within skills blocks Addresses #6859 Signed-off-by: Adam Scerra <ascerra@redhat.com>
Deduplicate and sort newNames before the no-frontmatter early-return path so duplicate skill basenames are not emitted. Rename test functions from Test_injectFrontmatterSkills_* to TestInjectFrontmatterSkills_* to match the package's PascalCase convention. Drop the redundant "inject frontmatter skills: " prefix from inner error messages to avoid stuttering when wrapped by the call site. Add a test for the no-frontmatter dedup case. Addresses #6859 Signed-off-by: Adam Scerra <ascerra@redhat.com>
… validation - Only match top-level (unindented) skills: keys to prevent false matches inside YAML block scalar continuations (e.g., description: >-\n skills: ...) - Track and skip continuation lines of multi-line flow-style arrays to prevent invalid YAML output from leaked remnants - Validate skill basenames against [a-zA-Z0-9._-]+ to reject YAML-unsafe characters before injection - Consistently strip UTF-8 BOM on the no-op path (all skills already present) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
Reword the invalid-skill-name error to use noun-phrase style consistent with other errors in the file. Add a targeted check for scalar skills values (e.g., `skills: my-single-skill` instead of a list) that produces a specific "must be a YAML list" message rather than a generic parse error. Addresses #6859 Signed-off-by: Adam Scerra <ascerra@redhat.com>
Change BOM byte sequence from lowercase \xef\xbb\xbf to uppercase \xEF\xBB\xBF in frontmatter_skills.go and its tests, matching the casing convention established in pi_agent.go. Addresses #6859 Signed-off-by: Adam Scerra <ascerra@redhat.com>
…p prefix Address all review findings on PR #6859: - Fix logic error where skills: lines with trailing comments or null values were misclassified as flow-style arrays, silently skipping subsequent frontmatter keys. Now checks whether the value after "skills:" starts with "[" before entering flow-style handling. - Extract isFence closure as package-level isFrontmatterFence function shared by parsePiAgent and injectFrontmatterSkills. - Rename uploadBytes temp file prefix from "fullsend-pi-*" to "fullsend-runtime-*" since both Claude and pi runtimes use it. - Detect CRLF line endings and use matching endings for injected skill lines to avoid mixed line endings. Strategy change (iteration 5): instead of patching individual detection cases, classify the skills: value by inspecting what follows the colon (empty/comment → block, "[" → flow, other → rewrite as block). Addresses #6859 Signed-off-by: Adam Scerra <ascerra@redhat.com>
Replace the fragile ]-based multi-line flow scan with a unified indentation-based approach that skips all continuation content after the skills: line. This fixes two bugs: 1. A ] inside a YAML comment on a flow-array continuation line (e.g., "code-review, # see ] for details") terminated the scan early, leaking remaining lines into the output as invalid YAML. 2. A YAML anchor on the skills value (e.g., "skills: &defaults") hit the else branch which replaced the skills: line but did not skip subsequent block list items, producing duplicate entries. Also: remove redundant sort.Strings(added) since added is built from already-sorted newNames, and fix "got scalar" error message that incorrectly described YAML mappings as scalars. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Adam Scerra <ascerra@redhat.com>
Signed-off-by: Adam Scerra <ascerra@redhat.com>
Signed-off-by: Adam Scerra <ascerra@redhat.com>
e0f8248 to
7419758
Compare
|
🤖 Finished Review · ✅ Success · Started 7:13 PM UTC · Completed 7:36 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $6.42 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ❌ Failure (running pre-script: exit status 1) · Started 7:38 PM UTC · Completed 7:38 PM UTC Commit: Effort: high |
Signed-off-by: Adam Scerra <ascerra@redhat.com>
…ntmatter-skills Signed-off-by: Adam Scerra <ascerra@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 5:23 PM UTC · Completed 5:43 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.63 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ❌ Failure (running pre-script: exit status 1) · Started 5:45 PM UTC · Completed 5:45 PM UTC Commit: Effort: high |
Signed-off-by: Adam Scerra <ascerra@redhat.com>
…ntmatter-skills Signed-off-by: Adam Scerra <ascerra@redhat.com>
|
🤖 Review · Commit: |
Signed-off-by: Adam Scerra <ascerra@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 8:14 PM UTC · Completed 8:35 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $7.48 |
| fmt.Fprintf(&buf, "---%s", eol) | ||
| fmt.Fprintf(&buf, "skills:%s", eol) | ||
| for _, name := range newNames { | ||
| fmt.Fprintf(&buf, " - %s%s", name, eol) |
There was a problem hiding this comment.
[low] logic-error
In the no-frontmatter branch of injectFrontmatterSkills, newly injected skill names are written as bare YAML scalars via fmt.Fprintf(" - %s%s", name, eol) with no type tagging, while rewriteFrontmatterSkills explicitly tags injected names as !!str. isValidSkillName only restricts characters to [a-zA-Z0-9._-], which still permits YAML 1.1 scalars that parse as non-string types when unquoted (e.g. "true", "yes", "off", "1.0", ".nan"). A harness skill directory basename colliding with one of these tokens would produce a boolean/float/null node in a newly created skills: list instead of the intended string.
Suggested fix: Build the created frontmatter using the same yaml.Node/!!str-tagged encoding path used in rewriteFrontmatterSkills (or explicitly quote injected names) instead of raw fmt.Fprintf, and add a test for a skill directory basename that is a YAML-special token (e.g. "true" or "1.0") on the no-frontmatter path.
| if err := yaml.Unmarshal(frontBytes, &doc); err != nil { | ||
| return nil, fmt.Errorf("parsing frontmatter: %w", err) | ||
| } | ||
| if len(doc.Content) != 1 || doc.Content[0].Kind != yaml.MappingNode { |
There was a problem hiding this comment.
[low] edge-case
An agent file with present-but-empty frontmatter fences (e.g. "---\n---\nBody\n") is accepted by parsePiAgent, but injectFrontmatterSkills fails on it: frontBytes is empty, and rewriteFrontmatterSkills's guard len(doc.Content) != 1 || doc.Content[0].Kind != yaml.MappingNode rejects it with "frontmatter must be a YAML mapping". ClaudeRuntime.Bootstrap propagates this as a hard error once harness skills are configured, so an agent definition that previously bootstrapped successfully with empty frontmatter now fails outright.
Suggested fix: Treat a missing or empty parsed document as an empty mapping before locating/creating the skills sequence (e.g. synthesize an empty MappingNode when doc.Content is empty), and add a regression test for "---\n---\nBody\n" with a non-empty skillDirs argument.
|
🤖 Finished Fix · ❌ Failure (running pre-script: exit status 1) · Started 8:37 PM UTC · Completed 8:37 PM UTC Commit: Effort: high |
1 participant
Summary
Injects harness-listed skill names into the agent definition's YAML frontmatter
skills:section duringClaudeRuntime.Bootstrap, ensuring skills reliably activate without requiring an explicit Skill tool call in the prompt body.Changes
internal/runtime/frontmatter_skills.go— NewInjectFrontmatterSkillsfunction that parses agent definition frontmatter, deduplicates skill names by basename, and injects missing harness skills. Handles edge cases: no frontmatter (creates one), noskills:section (appends one), BOM-prefixed files, and all-skills-already-present (returns unchanged).internal/runtime/claude.go— ModifiedClaudeRuntime.Bootstrapto read the agent file once, inject frontmatter skills frominput.SkillDirs(), then upload the modified content viauploadBytesinstead ofUploadFile.internal/runtime/claude_test.go— FixedTestClaudeRuntime_Bootstrap_OpenshellNotInPathto pass a file path (not a directory) asagentPath, matching the updated read-then-upload flow.internal/runtime/frontmatter_skills_test.go— Comprehensive test coverage (97.5%) for deduplication, no-frontmatter creation, no-skills-section injection, BOM handling, deterministic ordering, empty inputs, and frontmatter preservation.Testing
internal/runtime/...tests passgo build ./...succeedsgo vet ./internal/runtime/...passesgofmtapplied — no formatting issuesCloses #6681
Post-script verification
agent/6681-inject-frontmatter-skills)74aebe0ffe9e1a6bb6e1e8a6c7a4b97917457be7..HEAD)