[S30] Enforce ./tools/bin prefix for relative tool commands; update validator, tests, and docs

Enforces a canonical `./tools/bin/` prefix for any relative `command[0]` in the tools manifest and prevents path escapes after normalization. Updates unit tests to reflect the new rule and clarifies the reference docs’ error messages. All tests are green.

Refs: #1

Author: aibuddy

FEATURE_CHECKLIST.md

@@ -111,7 +111,7 @@
   - [x] [S51:l90-guard-no-legacy-paths] Add a `Makefile` target `check-tools-paths` that fails if any `./tools/(get_time|fs_|exec)` invocation remains outside `tools/bin` or `tools/cmd` (excluding `FEATURE_CHECKLIST.md`); implement with `rg` and negative globs; DoD: `make check-tools-paths` passes after migration, and fails before when run on current branch; no code behavior changed.
   - [x] [S52:l90-manifest-escape-check] Harden `internal/tools/manifest.go` validation to reject relative `command[0]` that normalize (via `path.Clean` + `filepath.ToSlash`) to paths containing `..` segments or escaping `./tools/bin/` (e.g., `./tools/bin/../hack`), and add focused tests in `internal/tools/manifest_test.go`; DoD: new tests fail before change and pass after, `go test ./...` green across platforms.
   - [x] [S53:l90-buildtool-windows-suffix-test] Extend `tools/testutil/buildtool.go` (after [S29:l90-test-helper-buildtool]) with a unit test asserting the returned binary path ends with `.exe` when `runtime.GOOS == "windows"` and has no suffix otherwise; migrate one representative tool test to assert the suffix via `BuildTool`; DoD: `go test ./tools/...` green on Unix and Windows runners when available.
-  - [ ] [S30:l90-manifest-enforce-bin-prefix] Enforce in `internal/tools/manifest.go` that any relative `command[0]` starts with `./tools/bin/` (absolute paths allowed for tests); adapt existing tests under `internal/tools/manifest_test.go` to cover this validation; DoD: added validation causes a failing test before change and passing after, `go test ./...` green.
+  - [x] [S30:l90-manifest-enforce-bin-prefix] Enforce in `internal/tools/manifest.go` that any relative `command[0]` starts with `./tools/bin/` (absolute paths allowed for tests); adapt existing tests under `internal/tools/manifest_test.go` to cover this validation; DoD: added validation causes a failing test before change and passing after, `go test ./...` green.
   - [ ] [S31:l90-manifest-doc-bin-prefix] Update `docs/reference/tools-manifest.md` to explicitly require relative `command` entries to use `./tools/bin/NAME`, update the JSON example to match, and note absolute paths are only for tests; DoD: doc renders on GitHub, `rg "\./tools/bin/" docs/reference/tools-manifest.md -n` finds at least one match, tests unchanged and green.
   - [ ] [S23:l90-bin-mkdir] Ensure `Makefile` creates `tools/bin` before builds by adding `mkdir -p tools/bin` (or equivalent) at the start of the `build-tools` recipe; DoD: from a clean clone `rm -rf tools/bin && make build-tools` succeeds and produces `tools/bin/{get_time,exec,fs_read_file,fs_write_file,fs_append_file,fs_rm,fs_move,fs_search,fs_mkdirp,fs_apply_patch,fs_read_lines,fs_edit_range,fs_listdir,fs_stat}`.
   - [ ] [S24:l90-clean-transitional] Update `Makefile` `clean` to remove both legacy paths (`tools/<NAME>` and `tools/*/<NAME>`) and new `tools/bin/*` during the migration window (until S20 completes) to avoid stale artifacts; DoD: after `make build-tools`, `make clean` leaves `git status` clean with no leftover tool binaries.

docs/reference/tools-manifest.md

@@ -15,7 +15,7 @@ ToolSpec fields:
 - `name` (string, required): Unique tool name. Must be non-empty and unique across the manifest.
 - `description` (string, optional): Short human description.
 - `schema` (object, optional): JSON Schema for the tool parameters. This is passed through to the model as `parameters` in the OpenAI "function" tool.
-- `command` (array of string, required): Argv vector. First element is the program path (relative or absolute); subsequent elements are fixed args. The runner will execute this program and write the function call JSON arguments to stdin.
+- `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.
 - `timeoutSec` (integer, optional): Per-call timeout override in seconds. If omitted, the CLI's `-timeout` applies.
 
 Notes:
@@ -79,7 +79,7 @@ On Windows, use the `.exe` suffix for the tool binary:
 - Duplicate `name`: error `tool[i] "<name>": duplicate name`.
 - Empty `command`: error `tool[i] "<name>": command must have at least program name`.
 - 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`.
-- Relative `command[0]` that normalizes to escape the tools bin directory (e.g., `./tools/bin/../hack`): error `tool[i] "<name>": relative command[0] escapes ./tools/bin (got "./tools/bin/../hack")`.
+- 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")`.
 
 ## Execution model
 - The assistant provides JSON arguments for the tool call. `agentcli` passes that JSON to the tool's stdin verbatim.

internal/tools/manifest.go

@@ -46,7 +46,8 @@ func LoadManifest(manifestPath string) (map[string]ToolSpec, []oai.Tool, error)
 		if len(t.Command) < 1 {
 			return nil, nil, fmt.Errorf("tool[%d] %q: command must have at least program name", i, t.Name)
 		}
-        // S52: Harden command[0] validation to prevent path escapes when relative
+        // S52/S30: Harden command[0] validation. For any relative program path,
+        // enforce the canonical tools bin prefix and prevent path escapes.
         cmd0 := t.Command[0]
         if !filepath.IsAbs(cmd0) {
             raw := filepath.ToSlash(cmd0)
@@ -61,9 +62,14 @@ func LoadManifest(manifestPath string) (map[string]ToolSpec, []oai.Tool, error)
             }
             // If original referenced ./tools/bin, ensure cleaned still stays within ./tools/bin
             if strings.HasPrefix(raw, "./tools/bin/") || raw == "./tools/bin" {
-                if !(strings.HasPrefix(norm, "./tools/bin/") || norm == "./tools/bin") {
+                if !(strings.HasPrefix(norm, "./tools/bin/")) {
                     return nil, nil, fmt.Errorf("tool[%d] %q: command[0] escapes ./tools/bin after normalization (got %q -> %q)", i, t.Name, cmd0, norm)
                 }
+            } else {
+                // Enforce canonical prefix for all other relative commands
+                if !strings.HasPrefix(norm, "./tools/bin/") {
+                    return nil, nil, fmt.Errorf("tool[%d] %q: relative command[0] must start with ./tools/bin/", i, t.Name)
+                }
             }
         }
 		registry[t.Name] = t

internal/tools/manifest_test.go

@@ -17,7 +17,8 @@ func TestLoadManifest_OK(t *testing.T) {
 				"name":        "hello",
 				"description": "says hello",
 				"schema":      map[string]any{"type": "object"},
-				"command":     []string{"echo", "{}"},
+                // absolute path allowed in tests
+                "command":     []string{"/bin/echo", "{}"},
 			},
 		},
 	}
@@ -81,7 +82,8 @@ func TestLoadManifest_CommandEscapeAndDotDot(t *testing.T) {
         wantErr  bool
     }{
         {name: "ok-absolute", command0: "/usr/bin/env", wantErr: false},
-        {name: "ok-simple-relative", command0: "echo", wantErr: false},
+        // relative simple path must now be under ./tools/bin
+        {name: "reject-simple-relative", command0: "echo", wantErr: true},
         {name: "ok-tools-bin", command0: "./tools/bin/fs_read_file", wantErr: false},
         {name: "reject-dotdot-leading", command0: "../tools/bin/get_time", wantErr: true},
         {name: "reject-escape-from-bin", command0: "./tools/bin/../hack", wantErr: true},