Remove the vestigial folder-structure (getFolderStructure) feature (Fixes #3072) - #3356
Conversation
* Harden dependencies and ZIP extraction (Fixes #3324) Replace extract-zip with staged, resource-bounded yauzl extraction that rejects symlinks, traversal, path aliases, and publication collisions. Raise vulnerable direct and transitive dependency floors in both lockfiles and preserve the reviewed CodeQL false-positive dispositions. * Make ZIP collision test portable Assert the exact preexisting directory entry instead of reopening it with alternate casing, which is not valid on case-sensitive Linux filesystems. * Verify streamed ZIP entry sizes * Record security review completion
github-actions
Bot
added
the
maintainer:e2e:ok
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe change removes folder-structure discovery, configuration, prompt variables, skill APIs, templates, exports, tests, and documentation. Environment context now includes working-directory information without folder-tree listings. ChangesFolder structure removal
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change removes an unused folder-structure feature and updates its related settings, prompts, interfaces, tests, and documentation; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation The changes are aligned with the linked issue and stated objectives. The directory-context preservation and activate-skill resource-directory update are related behavioral corrections, not unrelated scope.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
WalkthroughBefore this PR, the workspace context path included an optional folder-structure feature: when Release NotesBug Fixes
Documentation
Refactor
Chore
Changes
Sequence DiagramsequenceDiagram
participant AgentRuntime
participant PromptBuilder
participant PromptService
participant TemplateEngine
participant EnvironmentContext
participant SkillService
participant ActivateSkillTool
AgentRuntime->>PromptBuilder: buildAgentSystemPrompt(inputs, runtimeContext, template)
PromptBuilder->>PromptService: getService()
PromptService-->>PromptBuilder: PromptService
PromptBuilder->>EnvironmentContext: getDirectoryContextString(runtimeContext)
EnvironmentContext->>AgentRuntime: workspace directories
EnvironmentContext-->>PromptBuilder: working-directory preamble only, no folder tree
PromptBuilder->>PromptService: getPrompt(context, userMemory, coreMemory)
PromptService->>TemplateEngine: processTemplate(template, variables)
TemplateEngine-->>PromptService: rendered prompt without FOLDER_STRUCTURE
PromptService-->>PromptBuilder: system prompt
PromptBuilder-->>AgentRuntime: final prompt
AgentRuntime->>ActivateSkillTool: execute(name)
ActivateSkillTool->>SkillService: activateSkill(name)
SkillService-->>ActivateSkillTool: SkillActivationResult without folderStructure
ActivateSkillTool-->>AgentRuntime: llmContent with resourceDirectory
Magnitude🎯 4 (XL) Related
Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
OpenCodeReview — PR #3356
|
|
Addressed both inline comments (CI OCR) in |
|
Rejecting this one with reasoning:
If the empty-workspace case ever becomes real (e.g. a future caller using |
1 participant
TLDR
Removes the vestigial folder-structure (
getFolderStructure) feature end to end: the util and its test, the{{FOLDER_STRUCTURE}}prompt template machinery, the profile setting (include-folder-structure), theISkillService.folderStructuresurface, the 13 client-test mocks, and the docs. 41 files, +435/−1205. Reviewers should look at the two places where removal could have over-reached and did not:getDirectoryContextStringkeeps its exportedPromise<string>contract (preamble-only, no tree walk), andactivate-skillnow carries the resource directory in model-facingllmContent, not justreturnDisplay.Fixes #3072
Dive Deeper
What is gone
packages/core/src/utils/getFolderStructure.tsand its test, plus the./utils/getFolderStructure.jsentry in core's export map.prompts.tsfolder helpers (compactFolderStructureSnapshot,resolveFolderStructure,FOLDER_STRUCTUREsettings resolution) and the{{FOLDER_STRUCTURE}}var inTemplateEngine, prompt-configtypes.ts, and the three geminicore.mdtemplates.include-folder-structureacross the settings registry, profile types,ProfileManager(including itsoptionalBooleanhelper, which had no other user), and docs.ISkillService.folderStructure,CoreSkillServiceAdapter.folderStructureCache, and the per-skill folder listing inactivate-skill.client*.test.tsgetFolderStructuremocks,INITIAL_HISTORY_LENGTHre-export, and a staletest-run.logline.What was deliberately kept
environmentContext.tssurvives withgetEnvironmentContextandgetDirectoryContextString. The helper keeps its exportedasync/Promise<string>signature and returns only the working-directory preamble (one directory line or a bulleted list), soclient.tsandexecutor-prompt-builder.tscontinue to work unmodified;client.tsended up byte-identical tomain../utils/environmentContext.jssubpath export stays; onlygetFolderStructure.jswas dropped from the export map.addDirectoryContextstill pushes the preamble into history via the shared helper.Behavior fixes that fell out of review
activate-skill'sllmContentnow includes a<resource_directory>element (omitted when the skill has no resource directory) so the model sees where resources live, matching plan AC-3; previously only the UI-onlyreturnDisplaycarried it.Reviewer Test Plan
The env-context suites assert the preamble positively (date, OS, working-directory lines, environment memory) and guard against any tree listing returning.
Testing Matrix
* macOS only, via bun directly. Targeted suites green (executor x5, client core, subagent.create + ChatSessionFactory, settings profiles 139/0, prompts-async, environmentContext, core/tools, api T14b + P23.c); scoped eslint and
prettier --checkclean on all changed files; smoke test (--profile-load stepfun-37) boots cleanly to the provider call and fails only with the known stepfun 400 "no active step plan subscription".Pre-existing failures, proven on clean
main(eaae3364b), not addressed here:tscreports exactly 3 errors inpackages/cli/src/utils/zipExtract.ts(yauzl typings). Nopackages/clifile is in this diff.npm run buildtherefore stops at the same point it stops onmain; core/agents/lsp/providers all build green before that.bun test packages/core/src/configflakes 18 tests identically onmain; it is order-dependent (vi.mockleakage across files in one bun process — the executor suites' environmentContext automock exhibits the same class of leakage, which is why the env-context test should be run in its own invocation).npm run testexceeds the 900 s shell timeout on this machine.Linked issues / bugs
Fixes #3072
Summary by CodeRabbit
Changes
Documentation