Add workingDir folder selection to Ripple API topic creation

[mumez]

Summary

• Add a /workingDirs/list endpoint to AbTopicManagerRipple and extend /topics/create with optional workingDir/isNewFolder parameters.

Changes

• AbTopicManagerRipple: new /workingDirs/list request handler listing immediate subfolders of the agentic-browser root directory (excluding reserved topic-template/screenshots), each with name, createdAt, modifiedAt.
• AbTopicManagerRipple>>handleCreateTopic:: accepts optional workingDir (relative folder name) and isNewFolder (default false) body keys, validated against path traversal, reserved names, and root-boundary escape; reuses an existing folder or creates+seeds a new one from the topic template; raises RpError code 10009 (folder-name collision when isNewFolder: true) or 10010 (invalid/reserved name).
• New private helpers: resolveWorkingDirPath:isNewFolder:, validateWorkingDirName:, isReservedWorkingDirName:, workingDirJson:, newTopicFrom:workingDirPath:, workingDirAlreadyExistsError:, invalidWorkingDirNameError:, and workingDirsRootDirectory/workingDirsRootDirectory: accessors.
• AbTopicManagerRippleTest: new tests covering working-dir listing (empty, populated, reserved-folder exclusion) and topic creation with new/existing/colliding working directories and invalid names, plus a withTempWorkingDirsRoot: test helper.
• Adds docs/scripting-features/feature-ripple-api-topic-working-dir.scripting.md, the orchestration script used to generate this change.

Commits Included

5ea23f4 Add workingDir folder selection to Ripple API topic creation

Files Changed

docs/scripting-features/feature-ripple-api-topic-working-dir.scripting.md
src/AgenticBrowser-WebUI-Tests/AbTopicManagerRippleTest.class.st
src/AgenticBrowser-WebUI/AbTopicManagerRipple.class.st

[mumez]

Required - create path ignores the injected root, so the create tests pollute the real filesystem.

handleListWorkingDirs: lists from self workingDirsRootDirectory (the overridable ivar), but resolveWorkingDirPath: builds the target against AbSettings defaultAgenticBrowserRootDirectory directly. The accessor comment says the ivar is overridable for test isolation - yet the create path bypasses it entirely, so:

1. The list and create endpoints can resolve to different roots.
2. withTempWorkingDirsRoot: has no effect on the create-path tests. testCreateTopicWithNewWorkingDirCreatesAndSeedsFromTemplate, the reuse test, and the collision test therefore write real folders (seeded from the template, or holding a marker file) directly under the live defaultAgenticBrowserRootDirectory and never delete them on teardown - genuine test pollution of the repo/image filesystem.

Suggested: resolve against self workingDirsRootDirectory here (consistent with the list handler) and wrap the three create-path tests in withTempWorkingDirsRoot: so the helper actually isolates and cleans up.

[mumez]

Fixed in 1ff5dac: resolveWorkingDirPath:isNewFolder: now resolves against self workingDirsRootDirectory (same accessor handleListWorkingDirs: uses) instead of AbSettings defaultAgenticBrowserRootDirectory directly, so both endpoints agree on the root and tests can override it. The three affected create-path tests are now wrapped in withTempWorkingDirsRoot: (which nests the temp root under the real agentic-browser root so AbTopicTemplateDirectory>>copyTo:'s own guard still allows seeding) and clean up via deleteAll in the block's ensure:.

[mumez]

Required - empty/whitespace-only and "." workingDir names are not rejected and collapse to the agentic-browser root.

validateWorkingDirName: rejects /, backslash, .., absolute paths, and reserved names - but not an empty string or a single dot. With workingDir: \"\" (or "."), root / \"\" / root / \".\" resolves to the root directory itself, which trivially passes the beginsWith: re-check since it equals the root. A topic then receives the entire agentic-browser root as its working directory (containing topic-template, screenshots, and every other topic folder) - the opposite of the per-topic isolation this feature exists to provide.

Add a reject for empty/whitespace-only and "."/".." names (e.g. aDirName trimmed isEmpty), plus a regression test for the empty-string case.

[mumez]

Fixed in 1ff5dac: validateWorkingDirName: now rejects an empty/whitespace-only name and a bare "." (via aDirName trimmed isEmpty and aDirName trimmed = '.'), in addition to the existing checks. Added testCreateTopicSignalsErrorForEmptyWorkingDirName and testCreateTopicSignalsErrorForDotWorkingDirName regression tests.

[mumez]

Optional - reserved-name set duplicates knowledge owned elsewhere.

#( topic-template screenshots ) is hardcoded here. These names are derived by AbTopicTemplateDirectory and AbScreenshotAttachment (their root folders under the agentic-browser dir). If a third reserved folder is later added there, this list silently goes stale and future topics could target it. Consider a single source of truth (e.g. those classes exposing their reserved folder basename) and consume it here and in the list handler.

[mumez]

Addressed in 1ff5dac: added AbTopicTemplateDirectory class >> reservedFolderName and AbScreenshotAttachment class >> reservedFolderName, each returning their own literal, and initialize/screenshotDirectory now derive their path from that method instead of an inline string literal. AbTopicManagerRipple>>isReservedWorkingDirName: now checks against { AbTopicTemplateDirectory reservedFolderName. AbScreenshotAttachment reservedFolderName } instead of a hardcoded array, so a rename in either owning class can't silently drift out of sync.

[mumez]

Nit - newTopicFrom: reads like a pure constructor but has side effects.

It registers the topic (addTopic:) and, for non-nil workingDirPath, triggers working-directory creation/seeding. createTopicFrom:workingDirPath: would reveal the side effects better; alternatively split construction from the addTopic:/ensureExists step.

[mumez]

Renamed to createTopicFrom:workingDirPath: in 1ff5dac to name the side effects (addTopic:, directory creation/seeding) honestly.

[mumez]

FYI - validation errors take a different path than creation errors.

resolveWorkingDirPath: (raising the 10009/10010 RpError) runs outside the on: Error handler that wraps newTopicFrom: (which rewraps failures as 10006). That is deliberate so the specific codes propagate - good - but request-validation and creation-failure now flow through two error paths, and correctness depends on resolveWorkingDirPath: staying outside the handler. Consider a comment noting that, so a future move inside the block does not silently collapse 10009/10010 into 10006.

[mumez]

Good catch - added a comment at the call site in 1ff5dac explaining that resolveWorkingDirPath:isNewFolder: must stay outside the on: Error handler so its 10009/10010 codes aren't rewrapped as 10006, so a future edit doesn't move it in without noticing.

[mumez]

Reviewing this change from first principles:

• 2 Required — (1) the create path ignores the injected workingDirsRootDirectory, so the three create-path tests write real folders into the live defaultAgenticBrowserRootDirectory and never clean up (list and create also disagree about the root); (2) empty/"." workingDir names aren't rejected and collapse to the whole agentic-browser root.
• 1 Optional — reserved-name list hardcodes names owned by AbTopicTemplateDirectory/AbScreenshotAttachment.
• 2 nits/FYI — newTopicFrom: name vs. its side effects; validation vs. creation check errors flow through two paths.

Verification story: the PR reports all SUnit tests pass, but I could not independently run the suite in this environment, and the two Required issues concern tests that pass while polluting the real filesystem (they never assert on teardown state, only on exists). So a green suite here is not strong evidence of isolation. I'd like to see the create-path tests routed through withTempWorkingDirsRoot: with a teardown assertion, and an empty-string/"." regression test.

Verdict: request changes on the two Required items (bright-side: the correctness design — workingDirPath: set before ensureExists seeding, default-path behavior preserved when workingDir is omitted, and the collision flag semantics — is sound and well-tested otherwise).

[mumez]

Pushed 1ff5dac addressing both Required items from the review plus the optional and nit suggestions:

• Create-path workingDir resolution now goes through the same overridable workingDirsRootDirectory accessor the list endpoint uses, and the three affected tests are wrapped in withTempWorkingDirsRoot: so they no longer write into the live agentic-browser root.
• Empty/whitespace-only and "." names are now rejected (previously they collapsed to the root itself), with new regression tests.
• Reserved folder names are now sourced from AbTopicTemplateDirectory reservedFolderName / AbScreenshotAttachment reservedFolderName instead of a hardcoded literal array.
• newTopicFrom:workingDirPath: renamed to createTopicFrom:workingDirPath:.
• Added a comment documenting why validation must stay outside the on: Error handler.

All 76 AgenticBrowser-WebUI-Tests and 485 AgenticBrowser-Tests pass after the fix.

[mumez]

String >> trimmed comment says that its obsolete. trimBoth is better.

[mumez]

Good catch, fixed in b591d04: replaced both trimmed calls with trimBoth (trimmed is deprecated in Pharo 14, see pharo-project/pharo#18307). All 76 AgenticBrowser-WebUI-Tests still pass.

[mumez]

In AbTopicManagerRipple >> validateWorkingDirName: aDirName,

#('..' '/' '\') anySatisfy: [ :each | aDirName includesSubstring: each]

would be better for avoiding deep nesting

[mumez]

Fixed in 1008d33: flattened the three includesSubstring: checks into #('..' '/' '\\') anySatisfy: [ :each | aDirName includesSubstring: each ] as suggested, reducing the nesting depth from 6 to 4. All 76 AgenticBrowser-WebUI-Tests still pass.

[mumez]

Pushed e901508: updated docs/web-ui-api.md with the new request /workingDirs/list endpoint, the workingDir/isNewFolder fields on /topics/create, and error codes 10009/10010.