Repository navigation
Handle scalar agent paths when packaging - #229
Conversation
Claude manifests accept one agent path or an array of paths. Normalize the scalar form before validating bundled references so list() does not split a path into individual characters. Cover scalar and array output contents, manifest preservation, and rejection of missing or escaping scalar references before output creation. Default runtime package bytes remain unchanged. Assisted-by: OpenAI Codex Signed-off-by: Amit Vijapur <[email protected]>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
n3wth
left a comment
n3wth
left a comment
There was a problem hiding this comment.
Reviewed 38f925d. The fix is correct: Claude accepts agents as a string or an array, and list("./x.md") splits the string into characters, which is the Unbundled manifest reference: . failure described. Wrapping the string before the existing validation keeps the containment and bundle-membership checks on the same path for both forms, and array and missing-field behavior are unchanged.
One non-blocking item inline on the new branch: other non-string, non-list values still don't get a clear error. One test nit inline.
I ran tests/unit at this head. The new cases pass; the one failure is test_published_claude_runtime_is_current, the README.md drift that #209 addresses. I did not run Windows or a live host. cla/google and the scope/template check pass.
Nothing blocking.
Reject non-string, non-list agents values before packaging output is created. Preserve scalar normalization, list copying and reference checks. Add invalid-type regressions for both hosts and remove a redundant parsed-manifest assertion covered by exact-byte comparisons. Assisted-by: OpenAI Codex Signed-off-by: Amit Vijapur <[email protected]>
n3wth
left a comment
n3wth
left a comment
There was a problem hiding this comment.
Re-reviewed at 59efdb4. The only change since 38f925d is the follow-up to my last round: agents must now be a string or a list, anything else raises ValueError: Invalid manifest agents: ... before the output directory is created, and the redundant json.loads comparison is gone from the scalar test. Keeping list(agents) is fine, since it copies before the icon and logo references are appended. Containment and bundle-membership checks are unchanged for both forms.
The new tests cover None, 5, True, {}, a non-empty dict and 1.5 on both hosts, and assert the exact message and that the output is not created. The scalar tests also check missing.md, ../escape.md and an absolute path against the allowlist.
I ran tests/unit locally at this head. The one failure is test_published_claude_runtime_is_current, the README.md drift that #209 fixes. I did not run Windows or a live host. cla/google and the scope/template check pass.
No remaining finding.
Contribution Type
skills/oragents/; no design doc required)Refs #___withdesign-ready| Addsdocs/designs/<name>.mdand updatesdocs/designs/README.md)Closes #___withpr-ready| Implements mergeddocs/designs/<name>.md)Summary of Key Changes
Claude accepts
agentsas one path or an array of paths. The builder applieslist()to the field, splitting a single path into characters. An existing adapter supplied as a string therefore fails withValueError: Unbundled manifest reference: ., while the equivalent one-element array succeeds.agentstypes with a clearValueErrorbefore output creation, following review feedback. Retain the list copy before appending other references.Default Claude and OpenAI ZIPs are byte-identical before and after the change. Scope is offline packaging only; rollback is a normal commit revert, with no migration.
Human Accountability Affirmation
Prepared with Codex assistance. The review follow-up in 59efdb4 was also implemented and tested with Codex assistance and published after the contributor's approval of the presented diff.
Verification
pytest tests/unitexecuted; results below.Python 3.12.13, pytest 8.3.5, PyYAML 6.0.2 on macOS:
38f925d: twelve new invalid-type cases failed, with five existing controls passing. With the guard in59efdb4: all seventeen selected cases pass.59efdb4: 154 passed, one failed. The failure is the existingtest_published_claude_runtime_is_currentREADME mismatch, also reproduced on mainf0ba151and unchanged PR head38f925d, and tracked by fix(packaging): sync the packaged readme #209. This PR does not include that unrelated fix.git diff --checkpass. The review follow-up also leaves both default ZIPs byte-identical to the original PR head.Windows and native host/model execution were not tested. The absolute-style invalid-path case still checks allowlist rejection on Windows, not drive-absolute-path detection. The new guard validates the top-level field type, not array-element types.