mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-10 17:57:54 -10:00
* Tools: arg-tolerance, ask_user, secret scrubbing, structured results, hardening First module of the tools->agent->tui chain that leads to the TUI shell (off main). Scope: internal/tools (+ a 1-line specialist vocabulary update). 3-way merged onto main's specialist drift (RunOptions tool-context fields, Safety.AdvertiseInAuto, optionsAwareTool dispatch, PropertySchema, base structured Result) and grafted the runtime-core additions: - Shared arg-tolerance layer (argtolerance.go: alias-aware/type-strict arg resolution + slice coercion) adopted across all tools so weak models' non-canonical arg shapes are accepted; bool/int coercion in args.go. - ask_user tool (interactive clarifying questions) wired into CoreReadOnlyTools; nested PropertySchema (Properties/Required) for its object questions + update_plan. - Secret scrubbing at the registry boundary (scrubResultSecrets via internal/redaction) applied to every tool-execution path, incl. the optionsAwareTool dispatch; structured Result (Display/Redacted/ChangedFiles). - mutation_targets (paths a mutating tool will write, for session checkpointing) + hardened grep (symlink confinement), apply_patch, bash, edit/write. EXCLUDED per the module split: the blueprint task tool (subagents — another dev's area; the merged specialist Task is separate) and the skill tool (ships with the later skills module); plus their subagent-only registry helpers (Without/isolateForChild). ask_user is now a core tool, so internal/specialist knownToolNames adds "ask_user" to keep the TestKnownToolNamesMatchCoreRegistry cross-package invariant. No new deps; build/vet/-race/windows/full-suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * Address #119 review (CodeRabbit): scrub Display, validate apply_patch mutation paths, narrow test skips - registry.go: scrubResultSecrets now redacts Result.Display.Summary too (not just Output), so a caller preferring Display can't bypass the boundary redaction. - mutation_targets.go: apply_patch targets now run through validatePatchPaths(applyRoot, ...) before being returned, so a traversal patch (../x) can't yield an out-of-workspace checkpoint target. Regression case added to TestMutationTargetsRejectsEscapingPaths. - argtolerance_test/write_tools_test: the apply_patch tests no longer Skipf on ANY non-OK result (which masked real diff/apply regressions); they skip ONLY when the git binary is unavailable (gitApplyUnavailable helper checks exec's 'executable file not found') and otherwise Fatalf. build/vet/-race/full-suite + GOOS=windows build green. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * Address #119 re-review (CodeRabbit): best-effort multiSelect + Display.Summary scrub coverage - ask_user: an uncoercible multiSelect no longer fails the whole call — it's a UI hint, so it defaults to false (best-effort, mirroring the lenient options path). Test: TestParseAskUserQuestionsLenientOptions extended. - registry_test: TestRunWithOptionsScrubsSecretsForAllCallers now also leaks a secret via Result.Display.Summary and asserts it is redacted, locking in the boundary scrub for the structured-result path (secretTool gained a display field). build/vet/-race/full-suite + GOOS=windows build green. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * Address #119 review (@Vasanthdev2004): scrub ALL registry return paths + harden intArg [blocker] RunWithOptions now scrubs every return path, not just tool-execution: a named return + a single 'defer scrubResultSecrets(result)' covers the unknown-tool, sandbox-deny, sandbox-approval-required, permission-required and permission-denied early returns too. Previously a sandbox denial that echoed a secret-bearing path/arg returned it raw with Redacted=false. Regression: TestRunWithOptionsScrubsSecretsOnDenialPaths (permission-denied reason carrying a token -> scrubbed + Redacted). [non-blocking] intArg fails closed on NaN/Inf/out-of-range floats (and their string forms) via a floatToInt helper, before any implementation-defined cast. Tests added for NaN/Inf/NaN-string/non-integral. build/vet/-race/full-suite + GOOS=windows build green. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * Address #119 review (CodeRabbit grep TOCTOU + anandh8x dependency blockers) - grep: close the confinement TOCTOU — confineGrepFile now returns the symlink-RESOLVED path and collectGrepMatches reads THAT (not the raw candidate), so a symlink swapped in between check and read can't escape the workspace. - ask_user: removed from CoreReadOnlyTools. It needs the agent loop's interactive intercept (OnAskUser), which is in the (unmerged) agent module; without it the tool only returns the non-interactive fallback. The tool still ships (NewAskUserTool); the agent module registers it in core + wires the intercept together. Reverted the specialist knownToolNames 'ask_user' entry + the ask_user-in-core test accordingly. - update_plan: reverted the plan schema to a flat array (item structure documented in the description). The structured nested-object Items schema depends on the agent's PropertySchema serializer (propertyToRuntimeMap) passing nested Properties/Required through to providers, which is in the agent module; a nested schema here would be silently dropped. PropertySchema.Properties/Required fields are kept (used by ask_user.go's own schema + the later MCP module). build/vet/-race/full-suite + GOOS=windows build green; no new deps. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * Harden #119 from adversarial self-review: sandbox alias-gate, Meta scrub, intArg 2^63 A multi-dimension adversarial review of the tools diff surfaced 3 real issues beyond the reviewer rounds: - [major/security] write_file/edit_file path aliases file_path/filename/filepath bypassed the sandbox workspace+symlink gate: sandbox.requestPaths only inspected path/cwd/file/dir, so an aliased target skipped enforcement (same class as audit H4). Aligned requestPaths with the tools' path-alias lists (internal/sandbox/risk.go). The tools' own resolveWorkspaceTargetPath still confined, but the sandbox LAYER/audit/symlink-check was silently skipped. Regression: TestRegistrySandboxGatesPathAliasKeys. - [minor] scrubResultSecrets skipped Result.Meta, which is forwarded to the transcript and carries model-controlled strings (glob pattern, bash cwd). Now scrubs Meta values too. Covered in TestRunWithOptionsScrubsSecretsForAllCallers. - [minor] floatToInt used f > float64(MaxInt); that constant rounds up to 2^63 so exactly 2^63 slipped to an out-of-range cast. Changed to >=. Test added for 2^63 float+string. build/vet/-race/full-suite + GOOS=windows build green; no new deps. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> --------- Co-authored-by: KRATOS <[email protected]> Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
132 lines
4.5 KiB
Go
132 lines
4.5 KiB
Go
package tools
|
|
|
|
import (
|
|
"fmt"
|
|
"strconv"
|
|
"strings"
|
|
)
|
|
|
|
// argtolerance.go is the shared, model-agnostic argument-tolerance layer for the
|
|
// tools package. Different models (weak ones like minimax-m3 as well as strong
|
|
// ones) emit the same conceptual argument under different key spellings. These
|
|
// helpers let every tool accept the natural variants instead of hard-erroring,
|
|
// while preserving the existing TYPE-strictness contract (a present-but-non-string
|
|
// value still errors, scalars are never silently coerced to strings).
|
|
|
|
// aliasedStringArg reads a string argument, trying each key in order (the primary
|
|
// key first, then aliases) and returning the first present non-nil value. It
|
|
// preserves the exact semantics of stringArg/stringArgWithEmpty:
|
|
//
|
|
// - A present-but-non-string value errors "<primaryKey> must be a string"
|
|
// (type-strictness is preserved; scalars are NOT coerced to strings). The
|
|
// error always names the PRIMARY key so messages stay stable regardless of
|
|
// which alias the model happened to use.
|
|
// - Missing + required errors "<primaryKey> is required".
|
|
// - Missing + optional returns the fallback.
|
|
// - allowEmpty controls whether an empty string is accepted (when false, a
|
|
// present empty string errors "<primaryKey> must be a non-empty string").
|
|
//
|
|
// When allowEmpty=true, an empty-string value under one key does NOT mask a
|
|
// populated value under a later alias: the scan skips empty values so a populated
|
|
// alias wins (e.g. {"content":"","text":"hi"} -> "hi"). Only when EVERY present
|
|
// key is empty does it return "" (empty preserved), and only when every key is
|
|
// absent does it fall back / error required. Type errors (present-but-non-string)
|
|
// still fire eagerly regardless of emptiness.
|
|
//
|
|
// keys must be non-empty; keys[0] is the canonical/primary key used in errors.
|
|
func aliasedStringArg(args map[string]any, keys []string, fallback string, required bool, allowEmpty bool) (string, error) {
|
|
primary := ""
|
|
if len(keys) > 0 {
|
|
primary = keys[0]
|
|
}
|
|
sawPresentKey := false
|
|
for _, key := range keys {
|
|
value, ok := args[key]
|
|
if !ok || value == nil {
|
|
continue
|
|
}
|
|
text, ok := value.(string)
|
|
if !ok {
|
|
return "", fmt.Errorf("%s must be a string", primary)
|
|
}
|
|
if text == "" {
|
|
if !allowEmpty {
|
|
return "", fmt.Errorf("%s must be a non-empty string", primary)
|
|
}
|
|
// allowEmpty: don't let an empty value under one key mask a populated
|
|
// alias under a later key. Remember it was present and keep scanning.
|
|
sawPresentKey = true
|
|
continue
|
|
}
|
|
return text, nil
|
|
}
|
|
// No populated value found. If a key was present-but-empty (allowEmpty path),
|
|
// preserve the empty string rather than falling back / erroring required.
|
|
if sawPresentKey {
|
|
return "", nil
|
|
}
|
|
if required {
|
|
return "", fmt.Errorf("%s is required", primary)
|
|
}
|
|
return fallback, nil
|
|
}
|
|
|
|
// coerceStringSlice turns whatever a model put in an array-shaped argument into a
|
|
// string slice without ever failing. It accepts a []string, a []any of
|
|
// strings/scalars/objects (label/value/text/name/title/option), or a single
|
|
// newline-delimited string. Presentation-hint style arguments (e.g. ask_user
|
|
// options) use this so a malformed shape never breaks the whole call.
|
|
func coerceStringSlice(value any) []string {
|
|
switch v := value.(type) {
|
|
case nil:
|
|
return nil
|
|
case []string:
|
|
return v
|
|
case []any:
|
|
out := make([]string, 0, len(v))
|
|
for _, item := range v {
|
|
if s := scalarOrLabelString(item); s != "" {
|
|
out = append(out, s)
|
|
}
|
|
}
|
|
return out
|
|
case string:
|
|
lines := strings.Split(strings.ReplaceAll(v, "\r\n", "\n"), "\n")
|
|
out := make([]string, 0, len(lines))
|
|
for _, line := range lines {
|
|
if t := strings.TrimSpace(line); t != "" {
|
|
out = append(out, t)
|
|
}
|
|
}
|
|
return out
|
|
default:
|
|
if s := scalarOrLabelString(value); s != "" {
|
|
return []string{s}
|
|
}
|
|
return nil
|
|
}
|
|
}
|
|
|
|
// scalarOrLabelString best-effort renders a single list element to a string:
|
|
// trimmed strings, scalars (bool/number), or an object's common label keys.
|
|
// Returns "" when nothing usable is present.
|
|
func scalarOrLabelString(item any) string {
|
|
switch v := item.(type) {
|
|
case string:
|
|
return strings.TrimSpace(v)
|
|
case bool:
|
|
return strconv.FormatBool(v)
|
|
case float64:
|
|
return strconv.FormatFloat(v, 'f', -1, 64)
|
|
case int:
|
|
return strconv.Itoa(v)
|
|
case map[string]any:
|
|
for _, key := range []string{"label", "value", "text", "name", "title", "option"} {
|
|
if s, ok := v[key].(string); ok && strings.TrimSpace(s) != "" {
|
|
return strings.TrimSpace(s)
|
|
}
|
|
}
|
|
}
|
|
return ""
|
|
}
|