fix: deal with injectMetadataSave properly#286
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the injectMetadataSave function to more robustly handle various ways /bin/sh -c can be defined in container specifications, including normalization of command and argument slices. It also refactors the command wrapping logic to reuse a centralized metadata command. The review feedback identifies a missing case where the shell script might reside in the third element of the Command slice rather than Args, and points out a potential shell syntax error if the original command is empty, which would result in a leading '&&'.
I am having trouble creating individual review comments. Click here to see my feedback.
cmd/cli/cmd/llm/pull.go (394-400)
The current logic assumes the shell script is always in container.Args[0]. However, if the container is defined with the script directly in the Command slice (e.g., Command: ["/bin/sh", "-c", "my-script"]), container.Args will be empty. In this case, the code incorrectly appends metadataCmd to container.Args, where it will be treated as a positional parameter ($0) rather than being executed as part of the command. The logic should check if the script is in container.Command[2] first.
if len(container.Command) > 2 {
container.Command[2] = container.Command[2] + " && " + metadataCmd
} else if len(container.Args) == 0 {
container.Args = []string{metadataCmd}
} else {
// Args should typically have exactly one element for /bin/sh -c;
// additional elements become $0, $1... and are not executed as commands.
container.Args[0] = container.Args[0] + " && " + metadataCmd
}cmd/cli/cmd/llm/pull.go (421)
If both container.Command and container.Args are empty, fullCmd.String() will be empty, resulting in a wrappedCmd starting with &&, which is a shell syntax error. While plugins should typically provide a command, handling this case gracefully by omitting the leading && ensures the Job doesn't fail with a syntax error if the plugin relies on the image's default entrypoint.
wrappedCmd := metadataCmd
if fullCmd.Len() > 0 {
wrappedCmd = fmt.Sprintf("%s && %s", fullCmd.String(), metadataCmd)
}
2 participants
Summary
Fixes two issues in the
injectMetadataSavehelper withinrbgctl llm pull:/bin/sh -cdetection — the previous code only recognizedCommand: ["/bin/sh", "-c"], Args: [...]. It now also handles the variantCommand: ["/bin/sh"], Args: ["-c", ...]and normalizes it to the standard form so downstream logic stays uniform.printf '%s\n'without generating thedownloadedAttimestamp viadateandsed. It now reuses the samemetadataCmdas Case 1, ensuring the.rbg-metadata.jsonfile always contains the correctdownloadedAtvalue.Changes
cmd/cli/cmd/llm/pull.goisShellCdetection to cover/bin/shwith-cinArgsCommand: ["/bin/sh", "-c"]before appending the metadata commandcontainer.Argsis emptymetadataCmd(withdate+sed) instead of a shorter variantcmd/cli/cmd/llm/pull_test.goprintf '%s\n'withdateandsed