fix(foreman): accept workspace-internal absolute paths in resolveInside

[Defilan]

What

Widens resolveInside (the path-containment boundary for every file tool) to accept absolute paths that resolve inside the workspace, instead of rejecting all absolute paths outright. External absolute paths are still rejected, now with a clearer message.

Why

Fixes #945

Local coder models constantly quote absolute paths from their own bash output (find, ls, git status against $WORKSPACE_ROOT) and paste them into edit tools. The blanket rejection cost a full turn on each — fatal under the 3-turn restricted-edit budget of an aggressive stuck-loop profile. Observed live twice on the fleet (the #942/#943 validation runs).

How

When the input is absolute, compute filepath.Rel(wsResolved, filepath.Clean(p)); reject if the result escapes (..), otherwise strip the prefix and fall through to the unchanged relative-path containment path (Join → Rel check → terminal Lstat+EvalSymlinks symlink-escape check). Containment is computed with filepath.Rel (a real path relationship), never a naive string prefix, so a sibling like <ws>-evil is correctly rejected.

Adversarially verified: prefix-collision, absolute traversal, non-existent-path lexical escape, and absolute-path-through-an-escaping-symlink (with real on-disk symlinks) all reject; only the accepted spelling of already-inside paths widens. The two highest-value cases (prefix-collision, absolute+symlink escape) are pinned as regression tests so a future refactor can't reintroduce a strings.HasPrefix bug.

Authored by Foreman, LLMKube's agentic coder harness (local model on the lab fleet); reviewed adversarially and hardened with the two extra containment tests by the maintainer, who takes responsibility for the change per the AI-assisted contribution policy.

Checklist

• Tests added/updated - TestResolveInside_Cases gains absolute-inside/outside, prefix-collision, and absolute-symlink-escape subtests
• make test passes locally
• make lint passes locally (+ GOOS=linux golangci-lint, 0 issues)
• Commit messages follow conventional commits
• All commits are signed off (git commit -s) per DCO
• AI assistance (if any) is disclosed above, per CONTRIBUTING.md
• Documentation updated (if user-facing change) - not user-facing (internal tool boundary)

[codecov]

Codecov Report

❌ Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/foreman/agent/tools/workspace.go	77.77%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[joryirving]

Two issues to address before merge:

1. Misleading error message on relative-path escape (workspace.go:65)

The new error "paths must be workspace-relative" fires when a relative path like ../etc/passwd escapes the workspace. The path IS already relative; the problem is that it escapes containment. The model will read this and be confused — exactly the failure mode this PR exists to fix. The pre-PR "%w: %q" was clearer because it just showed the offending path.

Suggested: "%w: %q resolves outside workspace" (or similar that names the actual problem).

The sibling error in the absolute branch — "absolute paths must be inside the workspace" — is well-formed; the issue is only the relative-branch message.

2. Stale doc comment (workspace.go:36)

//  1. p is not absolute.

Rule 1 no longer applies: absolute paths resolving inside the workspace are now accepted. The comment should reflect the new contract — e.g. split into "absolute paths inside the workspace are accepted; absolute paths outside are rejected" alongside the existing rules 2 and 3.

Both are small; happy to re-review once addressed.

[Defilan]

Thanks Jory — both fixed in 1db7e3f:

1. The relative-branch escape error now reads path escapes workspace: "../etc/passwd" resolves outside workspace — names the actual problem and shows the offending path, matching the pre-PR clarity. Left the absolute-branch message as-is since you noted it was well-formed.
2. Refreshed doc rule 1 to state the new contract: absolute paths must resolve inside the workspace (outside is rejected) and are then treated as the equivalent relative path; relative paths as-is.

Tests + GOOS=linux lint green. Ready for re-review.

[joryirving]

Re-reviewing after the latest push. First, my CHANGES_REQUESTED findings — both addressed:

• Misleading error message — now "%w: %q resolves outside workspace". Fixed.
• Stale doc comment (rule 1) — now reflects that absolute paths inside the workspace are accepted. Fixed.

Mea culpa: I posted this re-review without re-reading the diff after the author's update; the framing "Three additional findings" wrongly implied the original two were still open.

Three new findings against the current diff:

1. Symlinked-workspace case still broken (medium, config-dependent)

The new absolute branch compares filepath.Rel(wsResolved, cleaned) where wsResolved = EvalSymlinks(wsAbs). When Workspace itself is reached via a symlink (e.g. ws = /tmp/.../link → /tmp/.../real), an absolute input using the link spelling is rejected even though it resolves inside:

wsResolved=/tmp/.../real
abs=/tmp/.../link/file.txt    # what $WORKSPACE_ROOT literally echoes
rel → "../link/file.txt"      # REJECTED

This is exactly the path the PR describes ("model quotes find/ls/git status output against $WORKSPACE_ROOT and pastes it") and the one the fix doesn't cover. Workspace is constructed as filepath.Join(workspaceRoot, ns, name) (internal/controller/..., pkg/foreman/agent/executor_native.go:249) and is not EvalSymlinks'd before being passed to BashTool; bash then echoes the un-resolved spelling back into $WORKSPACE_ROOT (pkg/foreman/agent/tools/bash.go:275).

Severity depends on whether your deployment has a symlinked workspace root. Typical $HOME/foreman-workspaces/... Linux layout won't trigger, but if the workspace is ever created through a symlink (or any parent component is) the fix regresses to the pre-PR behavior for that path.

Cheap fix: when the first Rel escapes, retry against wsAbs (un-resolved spelling); if it doesn't escape there, fall through to the existing EvalSymlinks(p) check for the symlink-escape case. A regression test mirroring the prefix-collision / absolute-symlink-escape shape, but with a symlinked workspace, would pin this.

2. Minor doc inaccuracy (workspace.go)

Strip the workspace prefix and continue with the same containment checks as for a relative path.

The code computes filepath.Rel, not a literal prefix strip. Since the new prefix-collision test is explicitly about why filepath.Rel is not strings.HasPrefix, calling the operation "strip" reads as the simpler mechanism the test is pinning against. Suggest: "Compute the workspace-relative path…".

3. Test coverage gap

The "absolute inside workspace accepted" subtest uses ws from makeWorkspace, which is already EvalSymlinks'd (tools_test.go:37) — so wsResolved == wsAbs and the new branch never exercises the case it was added for. The hard spelling (Workspace ≠ EvalSymlinks(Workspace)) is the one the absolute-path branch is supposed to make work, and it's untested. A test in the same shape as the prefix-collision / absolute-symlink-escape ones — workspace through a symlink, input using the link spelling — would pin this.

Non-issues confirmed

• All four call sites (read_file.go:116, str_replace.go:84, write_file.go:70, grep.go:100) just wrap with errors.Is(err, ErrPathEscapesWorkspace) — error-message text change doesn't break consumers.
• Removed "absolute rejected" subtest cleanly subsumed by absolute inside accepted + absolute outside rejected.
• TOCTOU between EvalSymlinks(wsAbs) and the terminal EvalSymlinks(joined) is pre-existing, not introduced here.
• Bash cd-guard (bash.go:84-86) is a separate mechanism, unaffected.

Verdict

Ship-able. Address (1) only if a symlinked workspace root is realistic in your deployment matrix; otherwise add a one-liner near the new branch documenting the limitation so the next person debugging the same $WORKSPACE_ROOT paste failure has a pointer.