Pass file-based script metadata to the script runner#530
Open
PratikDhanave wants to merge 3 commits into
Open
Conversation
newScript's Run closure handed the user-provided ScriptRunner a bare
&Script{Name: name}, discarding the ParametersSchema and the
AdditionalProperties (notably fsskills.scriptFS, the script's backing
fs.FS) that the discovered Script exposes. A runner that inspects those
fields to locate or execute the file received empty values; only runners
that rely solely on the name and the skill's root FS were unaffected,
which is why it went unnoticed.
Populate the runner-facing Script with the same metadata as the
discovered Script. Adds a black-box test asserting the runner receives
the schema and the scriptFS entry.
There was a problem hiding this comment.
Pull request overview
This PR fixes a contract issue in the fsskills file-based script implementation where the ScriptRunner was being invoked with a stripped *skills.Script (name only), preventing runners from accessing discovered script metadata (notably ParametersSchema and AdditionalProperties["fsskills.scriptFS"]) needed to locate/execute the underlying file.
Changes:
- Populate the runner-facing
*skills.Scriptwith the discovered script’sParametersSchemaandAdditionalProperties(includingfsskills.scriptFS). - Share a single
additionalPropertiesmap between the discovered script and the runner-facing script to keep metadata consistent. - Add a black-box regression test to verify the runner receives non-empty schema and the backing FS metadata.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| agent/skills/fsskills/source.go | Ensures the ScriptRunner receives a *Script populated with the discovered script’s schema and FS metadata via AdditionalProperties. |
| agent/skills/fsskills/source_script_test.go | Adds a regression test asserting the runner receives ParametersSchema and fsskills.scriptFS in AdditionalProperties. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Problem
fsskillsfile-based scripts hand the user-providedScriptRunnera stripped*Script. InnewScript(source.go):But the discovered
Scriptreturned bynewScriptcarries more:So a
ScriptRunnerthat inspectsscript.ParametersSchemaorscript.AdditionalProperties["fsskills.scriptFS"](the script's backingfs.FS, e.g. to open and execute the file) receives empty values. TheScriptRunnersignature deliberately passes a*Scriptrather than just a name, so runners are entitled to that metadata.The in-tree subprocess runner only uses
script.Name+ the skill's root FS, so nothing broke — which is exactly why the contract violation went unnoticed.Fix
Populate the runner-facing
Scriptwith the sameParametersSchemaandAdditionalProperties(sharing the singleadditionalPropertiesmap) as the discoveredScript.Test
TestFileSource_Runner_ReceivesScriptMetadata(black-box, insource_script_test.go) uses a runner that captures the received script and assertsParametersSchemais non-empty and thefsskills.scriptFSentry is present. Fails onmain(empty schema, missing key), passes with the fix.