Skip to content

Commit cbd53d0

Browse files
author
aibuddy
committed
feat(manifest): resolve relative command paths against manifest directory and document
- Validate relative command[0] as before, then rewrite to an absolute path anchored to the manifest's folder to decouple from process CWD - Add unit test covering nested manifest resolution - Update manifest reference to state resolution rule Refs: #1
1 parent 306f949 commit cbd53d0

3 files changed

Lines changed: 93 additions & 6 deletions

File tree

docs/reference/tools-manifest.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ ToolSpec fields:
1515
- `name` (string, required): Unique tool name. Must be non-empty and unique across the manifest.
1616
- `description` (string, optional): Short human description.
1717
- `schema` (object, optional): JSON Schema for the tool parameters. This is passed through to the model as `parameters` in the OpenAI "function" tool.
18-
- `command` (array of string, required): Argv vector. First element is the program path (relative or absolute); subsequent elements are fixed args. When relative, it MUST start with `./tools/bin/NAME` (use `.exe` on Windows). The runner will execute this program and write the function call JSON arguments to stdin.
18+
- `command` (array of string, required): Argv vector. First element is the program path (relative or absolute); subsequent elements are fixed args. When relative, it MUST start with `./tools/bin/NAME` (use `.exe` on Windows). Relative paths are resolved against the directory containing this `tools.json` (not the process working directory). The runner will execute this program and write the function call JSON arguments to stdin.
1919
- `timeoutSec` (integer, optional): Per-call timeout override in seconds. If omitted, the CLI's `-timeout` applies.
2020

2121
Notes:
@@ -78,7 +78,7 @@ On Windows, use the `.exe` suffix for the tool binary:
7878
- Missing `name`: error `tool[i]: name is required`.
7979
- Duplicate `name`: error `tool[i] "<name>": duplicate name`.
8080
- Empty `command`: error `tool[i] "<name>": command must have at least program name`.
81-
- Relative `command[0]` not using the canonical bin prefix: error `tool[i] "<name>": relative command[0] must start with ./tools/bin/` (absolute paths are allowed for tests). This ensures tools are invoked from `./tools/bin/NAME`.
81+
- Relative `command[0]` not using the canonical bin prefix: error `tool[i] "<name>": relative command[0] must start with ./tools/bin/` (absolute paths are allowed for tests). This ensures tools are invoked from `./tools/bin/NAME` and are then resolved relative to the manifest directory.
8282
- Relative `command[0]` that normalizes to escape the tools bin directory (e.g., `./tools/bin/../hack`): error `tool[i] "<name>": command[0] escapes ./tools/bin after normalization (got "./tools/bin/../hack" -> "./tools/hack")`.
8383

8484
## Execution model

internal/tools/manifest.go

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ type Manifest struct {
2424
}
2525

2626
// LoadManifest reads tools.json and returns a name->spec registry and an OpenAI-compatible tools array.
27+
// Relative command paths in the manifest are validated and then resolved relative to the manifest's directory,
28+
// so they do not depend on the process working directory.
2729
func LoadManifest(manifestPath string) (map[string]ToolSpec, []oai.Tool, error) {
2830
data, err := os.ReadFile(manifestPath)
2931
if err != nil {
@@ -36,7 +38,8 @@ func LoadManifest(manifestPath string) (map[string]ToolSpec, []oai.Tool, error)
3638
registry := make(map[string]ToolSpec)
3739
var oaiTools []oai.Tool
3840
nameSeen := make(map[string]struct{})
39-
for i, t := range man.Tools {
41+
manifestDir := filepath.Dir(manifestPath)
42+
for i, t := range man.Tools {
4043
if t.Name == "" {
4144
return nil, nil, fmt.Errorf("tool[%d]: name is required", i)
4245
}
@@ -50,7 +53,7 @@ func LoadManifest(manifestPath string) (map[string]ToolSpec, []oai.Tool, error)
5053
// S52/S30: Harden command[0] validation. For any relative program path,
5154
// enforce the canonical tools bin prefix and prevent path escapes.
5255
cmd0 := t.Command[0]
53-
if !filepath.IsAbs(cmd0) {
56+
if !filepath.IsAbs(cmd0) {
5457
// Normalize separators first (convert backslashes to slashes cross‑platform),
5558
// then apply a platform-agnostic clean.
5659
raw := strings.ReplaceAll(cmd0, "\\", "/")
@@ -68,12 +71,23 @@ func LoadManifest(manifestPath string) (map[string]ToolSpec, []oai.Tool, error)
6871
if !(strings.HasPrefix(norm, "./tools/bin/")) {
6972
return nil, nil, fmt.Errorf("tool[%d] %q: command[0] escapes ./tools/bin after normalization (got %q -> %q)", i, t.Name, cmd0, norm)
7073
}
71-
} else {
74+
} else {
7275
// Enforce canonical prefix for all other relative commands
7376
if !strings.HasPrefix(norm, "./tools/bin/") {
7477
return nil, nil, fmt.Errorf("tool[%d] %q: relative command[0] must start with ./tools/bin/", i, t.Name)
7578
}
7679
}
80+
// Resolve relative program path against the manifest directory to avoid dependence on process CWD
81+
// Keep validation based on the normalized forward-slash path, but compute a concrete absolute filesystem path.
82+
// Example: manifest in /repo/sub/manifest/tools.json and command "./tools/bin/name" -> /repo/sub/manifest/tools/bin/name
83+
// Trim leading "./" for joining, then convert to OS-specific separators.
84+
trimmed := strings.TrimPrefix(norm, "./")
85+
resolved := filepath.Join(manifestDir, filepath.FromSlash(trimmed))
86+
absResolved, errAbs := filepath.Abs(resolved)
87+
if errAbs != nil {
88+
return nil, nil, fmt.Errorf("tool[%d] %q: resolve command[0]: %v", i, t.Name, errAbs)
89+
}
90+
t.Command[0] = absResolved
7791
}
7892
registry[t.Name] = t
7993
// Build OpenAI tools entry

internal/tools/manifest_test.go

Lines changed: 74 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import (
1010
// https://github.com/hyperifyio/goagent/issues/1
1111
func TestLoadManifest_OK(t *testing.T) {
1212
dir := t.TempDir()
13-
file := filepath.Join(dir, "tools.json")
13+
file := filepath.Join(dir, "tools.json")
1414
data := map[string]any{
1515
"tools": []map[string]any{
1616
{
@@ -123,3 +123,76 @@ func TestLoadManifest_CommandEscapeAndDotDot(t *testing.T) {
123123
})
124124
}
125125
}
126+
127+
// Relative command paths must resolve against the manifest directory, not process CWD.
128+
// The loader should rewrite command[0] to an absolute path rooted at the manifest's folder.
129+
func TestLoadManifest_ResolvesRelativeAgainstManifestDir(t *testing.T) {
130+
// Create nested manifest directory
131+
base := t.TempDir()
132+
nested := filepath.Join(base, "configs", "sub")
133+
if err := os.MkdirAll(nested, 0o755); err != nil {
134+
t.Fatalf("mkdir nested: %v", err)
135+
}
136+
// Create a fake tools/bin tree relative to the manifest
137+
binDir := filepath.Join(nested, "tools", "bin")
138+
if err := os.MkdirAll(binDir, 0o755); err != nil {
139+
t.Fatalf("mkdir bin: %v", err)
140+
}
141+
// Create a small executable file to represent the tool binary
142+
toolPath := filepath.Join(binDir, "hello_tool")
143+
if err := os.WriteFile(toolPath, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil {
144+
t.Fatalf("write tool bin: %v", err)
145+
}
146+
// Write manifest that references ./tools/bin/hello_tool relative to the manifest dir
147+
manPath := filepath.Join(nested, "tools.json")
148+
data := map[string]any{
149+
"tools": []map[string]any{
150+
{
151+
"name": "hello",
152+
"command": []string{"./tools/bin/hello_tool"},
153+
},
154+
},
155+
}
156+
b, err := json.Marshal(data)
157+
if err != nil {
158+
t.Fatalf("marshal: %v", err)
159+
}
160+
if err := os.WriteFile(manPath, b, 0o644); err != nil {
161+
t.Fatalf("write manifest: %v", err)
162+
}
163+
// Change working directory to a different location to ensure CWD is not used for resolution
164+
oldWD, err := os.Getwd()
165+
if err != nil {
166+
t.Fatalf("getwd: %v", err)
167+
}
168+
other := filepath.Join(base, "other")
169+
if err := os.MkdirAll(other, 0o755); err != nil {
170+
t.Fatalf("mkdir other: %v", err)
171+
}
172+
if err := os.Chdir(other); err != nil {
173+
t.Fatalf("chdir other: %v", err)
174+
}
175+
t.Cleanup(func() {
176+
_ = os.Chdir(oldWD)
177+
})
178+
179+
reg, _, err := LoadManifest(manPath)
180+
if err != nil {
181+
t.Fatalf("LoadManifest: %v", err)
182+
}
183+
spec, ok := reg["hello"]
184+
if !ok {
185+
t.Fatalf("missing tool in registry")
186+
}
187+
if len(spec.Command) == 0 {
188+
t.Fatalf("empty command")
189+
}
190+
got := spec.Command[0]
191+
if !filepath.IsAbs(got) {
192+
t.Fatalf("command[0] not absolute: %q", got)
193+
}
194+
// It should point to the tool under the manifest's directory, not under CWD
195+
if got != toolPath {
196+
t.Fatalf("resolved path mismatch:\n got: %s\nwant: %s", got, toolPath)
197+
}
198+
}

0 commit comments

Comments
 (0)