Skip to content

Tests: isolate workspace skill prompt fixtures from bundled skills#32577

Closed
zwright8 wants to merge 1 commit intoopenclaw:mainfrom
zwright8:codex/pr26712-skill-prompt-fixture-isolation
Closed

Tests: isolate workspace skill prompt fixtures from bundled skills#32577
zwright8 wants to merge 1 commit intoopenclaw:mainfrom
zwright8:codex/pr26712-skill-prompt-fixture-isolation

Conversation

@zwright8
Copy link
Copy Markdown

@zwright8 zwright8 commented Mar 3, 2026

Summary

Validation

  • pnpm exec vitest run src/agents/skills.build-workspace-skills-prompt.prefers-workspace-skills-managed-skills.test.ts src/agents/skills.build-workspace-skills-prompt.syncs-merged-skills-into-target-workspace.test.ts

Context

This PR is one focused slice extracted from:

@openclaw-barnacle openclaw-barnacle bot added agents Agent runtime and tooling size: XS labels Mar 3, 2026
@zwright8 zwright8 mentioned this pull request Mar 3, 2026
Closed
18 tasks
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 3, 2026

Greptile Summary

This PR tightens test isolation by threading an explicit bundledSkillsDir (pointing to a workspace-local, typically empty .bundled subdirectory) into buildWorkspaceSkillsPrompt calls in two test files, preventing tests from accidentally reading system/global bundled skills and coupling to each other.

  • prefers-workspace-skills-managed-skills.test.ts: All three buildWorkspaceSkillsPrompt calls now supply a bundledSkillsDir — clean and consistent.
  • syncs-merged-skills-into-target-workspace.test.ts: The buildPrompt helper is updated to default both managedSkillsDir and bundledSkillsDir, which is the correct approach. However, several downstream call sites (lines 93-96, 179, 185, 207, 213) still pass managedSkillsDir explicitly with values identical to the helper's new defaults, leaving harmless but noisy redundancies worth cleaning up.

Confidence Score: 4/5

  • This PR is safe to merge — it is test-only and the isolation logic is correct.
  • The core isolation change is correct: bundledSkillsDir is now pointed to a workspace-local empty directory in all relevant test helpers, preventing cross-test contamination from real bundled skills. The only drawbacks are minor redundancies in a few call sites that pass managedSkillsDir or both dirs explicitly when the updated buildPrompt helper already provides the same values as defaults — these are noise, not bugs.
  • src/agents/skills.build-workspace-skills-prompt.syncs-merged-skills-into-target-workspace.test.ts — several call sites have redundant explicit dir overrides that could be cleaned up for consistency.

Last reviewed commit: cf8757c

greptile-apps[bot]
greptile-apps bot reviewed Mar 3, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 3, 2026

Additional Comments (2)

src/agents/skills.build-workspace-skills-prompt.syncs-merged-skills-into-target-workspace.test.ts
Redundant explicit dirs now that buildPrompt defaults them

buildPrompt's workspaceDir argument here is targetWorkspace, so the newly-added defaults in the helper already resolve to path.join(targetWorkspace, ".managed") and path.join(targetWorkspace, ".bundled"). Passing those same values explicitly via opts is now a no-op and can be dropped to keep call-sites consistent with how the other tests use buildPrompt.

    const prompt = buildPrompt(targetWorkspace);
Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/agents/skills.build-workspace-skills-prompt.syncs-merged-skills-into-target-workspace.test.ts
Line: 93-96

Comment:
**Redundant explicit dirs now that `buildPrompt` defaults them**

`buildPrompt`'s `workspaceDir` argument here is `targetWorkspace`, so the newly-added defaults in the helper already resolve to `path.join(targetWorkspace, ".managed")` and `path.join(targetWorkspace, ".bundled")`. Passing those same values explicitly via `opts` is now a no-op and can be dropped to keep call-sites consistent with how the other tests use `buildPrompt`.

```suggestion
    const prompt = buildPrompt(targetWorkspace);
```

How can I resolve this? If you propose a fix, please make it concise.

src/agents/skills.build-workspace-skills-prompt.syncs-merged-skills-into-target-workspace.test.ts
Redundant managedSkillsDir after helper update

buildPrompt now defaults managedSkillsDir to path.join(workspaceDir, ".managed"), so passing that same value explicitly is redundant. The same pattern appears at lines 185, 207, and 213 as well. Cleaning these up would make the intent of the new defaults clearer and reduce noise.

      const missingPrompt = buildPrompt(workspaceDir, {
        config: { skills: { entries: { "nano-banana-pro": { apiKey: "" } } } },
      });
Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/agents/skills.build-workspace-skills-prompt.syncs-merged-skills-into-target-workspace.test.ts
Line: 178-182

Comment:
**Redundant `managedSkillsDir` after helper update**

`buildPrompt` now defaults `managedSkillsDir` to `path.join(workspaceDir, ".managed")`, so passing that same value explicitly is redundant. The same pattern appears at lines 185, 207, and 213 as well. Cleaning these up would make the intent of the new defaults clearer and reduce noise.

```suggestion
      const missingPrompt = buildPrompt(workspaceDir, {
        config: { skills: { entries: { "nano-banana-pro": { apiKey: "" } } } },
      });
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@zwright8 zwright8 force-pushed the codex/pr26712-skill-prompt-fixture-isolation branch from cf8757c to 76c69be Compare March 3, 2026 05:07
@openclaw-barnacle
Copy link
Copy Markdown

openclaw-barnacle bot commented Mar 6, 2026

Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit.

@openclaw-barnacle openclaw-barnacle bot closed this Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

@LalithaMV LalithaMV LalithaMV approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

agents Agent runtime and tooling r: too-many-prs size: XS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zwright8 @LalithaMV @thewilloftheshadow