fix(harness): expose session workspace in prompt - #3020
Conversation
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| AbstractFilesystem filesystem = workspaceManager.getFilesystem(); | ||
| Path effectiveWorkspace = | ||
| detectLocalUpper(filesystem) != null | ||
| ? workspaceManager.resolveRuntimeDataPath(rc, "") |
There was a problem hiding this comment.
This advertises /workspace/session-1 under SESSION isolation, but HarnessAgent still wires WorkspacePathNormalizer to the base /workspace. So if the model follows the prompt and calls write_file with /workspace/session-1/artifact.txt, the normalizer strips that down to session-1/artifact.txt, then LocalFilesystem prefixes the session again — and you land on /workspace/session-1/session-1/artifact.txt. Shell using the advertised absolute path won't find that file (same story when reading a shell-written artifact by absolute path).
Could you make absolute and relative paths hit the same place, and add a FilesystemTool regression with HarnessAgent's normalizer so we don't get a duplicated session directory? The new prompt-string checks don't cover this. Also noting: this gap was already there; the new prompt just makes it easier to hit.
There was a problem hiding this comment.
Thanks for catching this. The issue was valid: under SESSION isolation, the prompt could advertise a session-scoped absolute workspace path, while the filesystem namespace was applied again after path normalization.
I updated the existing PR to normalize paths with the current RuntimeContext namespace, so the advertised absolute path and the relative path now resolve to the same file without producing a duplicated session directory such as session-1/session-1.
|
This PR currently conflicts with the git fetch origin
git checkout fix/2941-session-workspace-context
git rebase origin/main
# resolve conflicts, then:
git push --force-with-leaseThis is a one-time reminder. Feel free to @mention me for a re-review after the conflicts are resolved. Automated notification by github-manager-bot |
|
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review after the merge conflict was resolved (thanks for the quick rebase, and for the @mention). Conflicts are gone (mergeable: MERGEABLE), CLA is signed, and all checks pass on the current head — including build (windows-latest), which matters for a path-normalisation change.
The fix is the right shape: rather than patching the prompt string in isolation, it derives the advertised root from the same NamespaceFactory the filesystem already applies at operation time, and threads the operation's RuntimeContext through norm(...) so absolute and relative paths converge on one file. Reusing resolveRuntimeDataPath keeps a single source of truth, and the end-to-end sessionIsolation_absoluteAndRelativeWorkspacePathsResolveToSameFile test is the right way to pin that contract — asserting the negative (session-1/session-1/... must not exist) is especially good.
Verdict: no blocking issues; left as COMMENT so the three questions below can be answered or explicitly deferred. Two are about how safely this new dual-mode normaliser resists future misuse, one is a scope question about an adjacent prompt section that appears to have the same bug class as #2941.
Findings
- [Warning]
WorkspacePathNormalizer.java:100— the 1-argnormalize(String)overload now silently resolves the namespace from an empty context, which reproduces the double-nesting bug for any caller that does not pass aRuntimeContext. - [Warning]
WorkspacePathNormalizer.java:123— namespace segments are treated as trusted path components; worth pinning the contract with id sanitisation (#2952) via a hostile-id test. - [Info]
WorkspaceContextMiddleware.java:219—buildKnowledgeBlockstill receives the base workspace, soKnowledge files:stays relative to the un-namespaced root: same bug class as #2941, or intentional? - [Info]
WorkspaceContextMiddleware.java:243— two adjacentPathparams distinguished only by name; a small holder type would prevent future mix-ups. - [Info]
WorkspacePathNormalizer.java:118— multi-segment namespaces and the empty-namespace overlay case are untested.
Suggestions
- For the overload: prefer making the safe path the only path — either
@Deprecatedthe 1-arg method with a note, or have it throw (or at leastlog.warn) whennamespaceFactory != null. A compile-time/deprecation warning is much cheaper than a misplaced artifact at runtime. - For the knowledge block: if base-scoped is deliberate, a one-line comment saying so is enough; otherwise it likely wants
effectiveWorkspacetoo. - Consider a test asserting the rendered prompt is unchanged when isolation is disabled, so the refactor of
buildWorkspaceParagraphis provably behaviour-preserving for the common single-tenant configuration.
Automated review by github-manager-bot
| * @return workspace-relative path, or the original path if no registered prefix matched | ||
| */ | ||
| public String normalize(String path) { | ||
| return normalize(path, RuntimeContext.empty()); |
There was a problem hiding this comment.
The retained 1-arg overload now silently resolves the namespace from RuntimeContext.empty(). For a normalizer built with a namespaceFactory, an empty context yields an empty namespace, so the namespaced branch is skipped and the plain base prefix is stripped instead: /workspace/session-1/artifact.txt -> session-1/artifact.txt -> the filesystem re-applies the namespace -> /workspace/session-1/session-1/artifact.txt. That is precisely the double-nesting failure the new test asserts must not happen, but it becomes reachable through any caller that keeps (or later adds) a 1-arg call.
Both production callers were migrated here, so nothing is broken today — could the overload be deprecated, or made to throw / log when namespaceFactory != null, so the context is always explicit? Otherwise the invariant "always normalise with the operation's RuntimeContext" is held only by convention, and it is a very quiet failure mode when it slips.
Minor related point: the Javadoc on normalize(String) still describes only prefix stripping, without mentioning that the result now depends on a namespace resolved from an empty context.
| namespaceFactory.getNamespace( | ||
| runtimeContext != null ? runtimeContext : RuntimeContext.empty()); | ||
| if (namespace != null && !namespace.isEmpty()) { | ||
| String namespacePrefix = String.join("/", namespace); |
There was a problem hiding this comment.
Composing the prefix as prefix + "/" + String.join("/", namespace) treats namespace segments as trusted path components. If a session id (or any other namespace segment) can contain /, .., or an empty segment, the composed prefix can match a path outside the intended session — e.g. a .. segment makes prefix + "/.." match /workspace/../etc, which then strips down to a bare relative name and escapes the session scoping rather than containing it.
Is session-id sanitisation guaranteed upstream? #2952 (fix(harness): sanitize session ids used in workspace file names) looks like it addresses the same surface for file names, so this may already be covered — but the two changes compose right here. Worth adding an explicit test with a hostile id (a/../../b, absolute-looking or empty segments) asserting the path is not stripped, to pin the contract between id sanitisation and prefix composition.
| Path workspace = workspaceManager.getWorkspace(); | ||
| String sessionContext = buildSessionContextSection(workspace, rc); | ||
| AbstractFilesystem filesystem = workspaceManager.getFilesystem(); | ||
| Path effectiveWorkspace = |
There was a problem hiding this comment.
Scope question rather than a defect: effectiveWorkspace is substituted into the session-context section and the Workspace paragraph, but buildKnowledgeBlock(...) two lines below is still passed the base workspace, and WorkspaceManager.listKnowledgeFiles relativises its local-disk half against the base workspace while getKnowledgeDir() returns the un-namespaced workspace.resolve(KNOWLEDGE_DIR).
So under session isolation the model sees "Workspace (your home base): /workspace/session-1" while Knowledge files: entries remain relative to /workspace, and session-scoped knowledge files may not appear in the listing at all (the local walk looks in the base dir; only the filesystem.glob(rc, ...) half is context-aware). Since the symptom reported in #2941 is exactly models resolving advertised paths against the wrong root, this looks like the same bug class in an adjacent section.
If keeping knowledge base-scoped is deliberate (shared read-only knowledge across sessions), could you note that in a short comment so the next reader does not "fix" it? Otherwise passing effectiveWorkspace (and namespacing getKnowledgeDir()) would complete the correction. Happy to see that as a follow-up PR rather than blocking this one.
| String workspaceParagraph = | ||
| buildWorkspaceParagraph( | ||
| workspace, workspaceManager.getFilesystem(), artifactDeliveryEnabled); | ||
| workspace, effectiveWorkspace, filesystem, artifactDeliveryEnabled); |
There was a problem hiding this comment.
Nit: threading workspace and effectiveWorkspace side by side through the private helpers makes it easy to pick the wrong one at a call site later, and the two are only distinguishable by name. Since extraRootsOf(localUpper, project, workspace) and the path-policy roots genuinely want the base while the prompt text wants the effective root, consider a small value holder (e.g. WorkspaceRoots(Path base, Path effective)) or resolving both once inside the method. Not blocking.
| if (path == null || path.isBlank()) { | ||
| return path; | ||
| } | ||
| if (namespaceFactory != null) { |
There was a problem hiding this comment.
Suggestion for completeness: String.join("/", namespace) supports multi-segment namespaces, but the new tests only exercise a single sessionId segment. If any isolation scope composes two dimensions (e.g. user + session) the prefix becomes two levels deep, which is exactly where a mistake would hide — one test with a 2-segment namespace would cover it cheaply.
Also worth confirming the overlay-without-namespace case (namespaceFactory != null but an empty namespace) stays byte-identical to the pre-PR prompt, since HarnessAgent now always passes nsFactory for local overlays.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review after 443c3502 (the only author commit since my last review — the other new commits are upstream resync merges). Thanks for the follow-up: conflict resolution, @Deprecated on the unscoped overload, and the mention ping.
What I verified this round:
- Scope of the new commit:
git diff 1f5b9b7f..443c3502touches onlyWorkspacePathNormalizerjavadoc +@Deprecated. No behaviour change, so the previously reviewed logic (single source of truth viaresolveRuntimeDataPath,RuntimeContextthreaded throughnorm(...)) still stands. - Migration is complete in main:
FilesystemTool.normandArtifactDeliveryTool.normare the only production call sites and both pass theRuntimeContext, so the deprecated overload has no in-repo users. That is what makes the finding below a hardening note rather than a defect. - Gate correctness: the namespaced strip is only wired in for the
OverlayFilesystem+LocalFilesystemWithShellbranch (HarnessAgent.java:2692), and the new prompt test asserts both the positive (session-1advertised) and the negative (Additional rootsdoes not absorb the workspace) — good. - CI / CLA / conflicts:
check,check windows build,check codeql,check license/clagreen on head443c3502;mergeable: MERGEABLE,mergeStateStatus: BLOCKED(blocked on review/merge settings, not on conflicts).
Verdict: no new blocking issues; nothing here changes my previous assessment. Not approving because the five threads from my previous review are still open and unresolved — I have not restated them; the three notes below are the new ones only, all non-blocking. If you consider the open questions addressed or deferred, resolve the threads (or reply "deferred: …") and @mention me for an approval pass.
Automated review by github-manager-bot
| * <p>Callers using a normalizer with a namespace factory should use | ||
| * {@link #normalize(String, RuntimeContext)} so the operation context is available. This | ||
| * overload is retained for compatibility and uses an empty context. | ||
| * | ||
| * @param path the raw path (absolute or relative) | ||
| * @return workspace-relative path, or the original path if no registered prefix matched | ||
| */ | ||
| @Deprecated | ||
| public String normalize(String path) { | ||
| return normalize(path, RuntimeContext.empty()); |
There was a problem hiding this comment.
[Warning] Good step — but the deprecation is currently descriptive rather than structural. The javadoc only tells callers with a namespaceFactory to prefer normalize(String, RuntimeContext); nothing prevents a future call site from taking the 1-arg overload and silently falling back to the base prefix (the exact session-1/session-1/... mis-write this PR fixes).
Since there are no remaining unscoped call sites in main today (FilesystemTool / ArtifactDeliveryTool are both migrated), consider making the safe path the only path:
// option A — no unscoped escape hatch on a namespaced normalizer
public static WorkspacePathNormalizer unscoped(String... prefixes) { ... }
public static WorkspacePathNormalizer namespaced(String prefix, NamespaceFactory nsf) { ... }
// option B — keep the overload, but make the silent case observable
if (namespaceFactory != null) {
log.debug("normalizing without RuntimeContext; namespace prefix not applied to {}", path);
}Option A also lets the deprecation be retired later without a second API break.
| Path effectiveWorkspace = | ||
| detectLocalUpper(filesystem) != null | ||
| ? workspaceManager.resolveRuntimeDataPath(rc, "") | ||
| : workspace; |
There was a problem hiding this comment.
[Info] The effectiveWorkspace gate is detectLocalUpper(filesystem) != null, i.e. local-overlay only. A plain (non-shell, non-overlay) LocalFilesystem built with the same nsFactory still gets namespaced relative paths at operation time, but falls into the generic "Your working directory is: " branch and keeps advertising the unscoped root — the same mismatch as #2941 for that topology, and a regression test for it would not catch the drift.
If that is deliberate (only the overlay topology can advertise a host path), a one-line comment here stating why the gate is detectLocalUpper would be enough. Otherwise it may be worth a follow-up issue so remote / composite topologies are tracked explicitly.
| @Test | ||
| void sessionIsolation_absoluteAndRelativeWorkspacePathsResolveToSameFile( | ||
| @TempDir Path workspace) { | ||
| NamespaceFactory namespaceFactory = IsolationScope.SESSION.toNamespaceFactory(); | ||
| AbstractFilesystem filesystem = | ||
| new LocalFilesystemSpec() | ||
| .isolationScope(IsolationScope.SESSION) | ||
| .toFilesystem(workspace, namespaceFactory); | ||
| WorkspacePathNormalizer normalizer = | ||
| WorkspacePathNormalizer.of( | ||
| workspace.toAbsolutePath().normalize().toString(), namespaceFactory); | ||
| tool = new FilesystemTool(filesystem, normalizer); | ||
| RuntimeContext runtimeContext = RuntimeContext.builder().sessionId("session-1").build(); | ||
| Path absolutePath = workspace.resolve("session-1/artifact.txt").toAbsolutePath(); | ||
|
|
||
| assertTrue( | ||
| tool.writeFile(runtimeContext, absolutePath.toString(), "artifact") | ||
| .startsWith("Written to ")); | ||
| assertTrue(Files.exists(absolutePath)); | ||
| assertFalse(Files.exists(workspace.resolve("session-1/session-1/artifact.txt"))); | ||
| assertEquals("artifact", tool.readFile(runtimeContext, "artifact.txt", null, null)); | ||
| assertEquals( | ||
| "artifact", tool.readFile(runtimeContext, absolutePath.toString(), null, null)); | ||
| } |
There was a problem hiding this comment.
[Info] Nice that the negative (session-1/session-1/artifact.txt must not exist) is asserted. One adjacent case is still unpinned: a model that echoes a foreign namespace, e.g. /workspace/session-2/x.txt while the active namespace is session-1. With the current ordering that path does not match the namespaced prefix, so it strips to session-2/x.txt, which the filesystem then re-namespaces to session-1/session-2/x.txt — a new directory inside the current session rather than an error.
Not introduced here (it is pre-existing relative-path semantics), and the absolute-path bounds check may already reject it — if so, a one-assert test would make that guarantee explicit and cheap to keep.
4 participants
Summary
Problem
With session isolation enabled, relative file-tool paths resolve under the session namespace (for example,
/workspace/session-1), while the prompt currently advertises the base workspace (/workspace). Models can therefore run skill scripts against the wrong path, write artifacts outside the session namespace, and then fail to find those artifacts through file tools.This change only corrects the path shown to the model. It reuses
WorkspaceManager.resolveRuntimeDataPathso the advertised path follows the same namespace factory already used by runtime data.Testing
mvn -pl agentscope-harness spotless:checkmvn -pl agentscope-harness -Dtest="WorkspaceContextMiddlewarePathBoundsTest,WorkspaceContextMiddlewareMemoryPromptTest,WorkspaceContextMiddlewareSandboxPromptTest,LocalFilesystemModeTest,LocalFilesystemWithShellTest" test(38 tests passed)Fixes #2941