Greptile Summary

This PR tightens the skills.workspace.symlink_escape audit probe to use workspace/skills/ as the containment boundary instead of the workspace root, aligning it with how resolveContainedSkillPath in the skill loader actually enforces containment. The logic change is a one-liner, the updated finding messages are accurate, and the new test case directly covers the previously-invisible gap (symlink resolving inside workspace but outside the skills dir).

Confidence Score: 5/5

Safe to merge — the boundary fix is correct and well-tested; only a minor test comment mismatch remains.

All findings are P2 (a misleading comment in the new test). The core logic change correctly aligns the audit boundary with the loader, and the new test validates the previously-missed gap scenario.

No files require special attention.

This is a comment left during a code review.
Path: src/security/audit.test.ts
Line: 1397-1401

Comment:
**Test comment describes relative symlink, code creates absolute**

The comment says `../../shared/skills/my-skill/SKILL.md` (relative), but `fs.symlink` receives an absolute path via `path.join(sharedSkillDir, "SKILL.md")`. Both types correctly exercise the escape scenario here, but the comment mismatches the actual symlink type and could mislead someone copying this test pattern.

```suggestion
          // Symlink: skills/linked-skill/SKILL.md -> <abs>/workspace/shared/skills/my-skill/SKILL.md
          await fs.symlink(
            path.join(sharedSkillDir, "SKILL.md"),
            path.join(workspaceDir, "skills", "linked-skill", "SKILL.md"),
          );
```

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

_{Reviews (1): Last reviewed commit: "fix(security): align audit symlink_escap..." | Re-trigger Greptile}

Closing this as duplicate or superseded after Codex automated review.

Close #60155 as superseded by the later open PR #60513, which tracks the same remaining security-audit boundary fix. Current main has not implemented the fix yet, so this is not an implemented-on-main close.

Best possible solution:

Close #60155 as duplicate/superseded by open #60513, then use #60513 or a maintainer branch to port the boundary fix onto the current audit-workspace-skills.ts implementation, update the dedicated audit-workspace-skill-escape test, and align the audit-check docs/finding copy with the skills-directory boundary.

What I checked:

• Current audit still uses workspace root boundary: On current main, collectWorkspaceSkillSymlinkEscapeFindings resolves workspacePath/workspaceRealPath and accepts any SKILL.md realpath inside that workspace, with finding text still saying workspace root. (src/security/audit-workspace-skills.ts:128, e29d3516bf05)
• Loader enforces skills directory root: The loader constructs workspaceSkillsDir as /skills, calls loadSkills with that root, and resolveContainedSkillPath rejects candidates whose realpath is outside the configured root. (src/agents/skills/workspace.ts:268, e29d3516bf05)
• Loader test covers the gap case: The workspace skill loader test symlinks a skill file to a target inside the workspace but outside /skills and expects the skill not to load, with the warning root shown as /skills. (src/agents/skills.loadworkspaceskillentries.test.ts:306, e29d3516bf05)
• Audit probe has moved since this PR's patch: Current main exports collectWorkspaceSkillSymlinkEscapeFindings from audit-workspace-skills.ts; fix(security): align audit symlink_escape boundary with skill loader #60155 patches the older audit-extra.async.ts/audit.test.ts locations, so the useful fix needs to be ported to the current split files. (src/security/audit.nondeep.runtime.ts:28, e29d3516bf05)
• Later canonical PR exists: GitHub API shows fix(security): align audit symlink_escape boundary with skill loader #60513 is open, by the same author, with the same title and same requested fix; it relates the work to [Bug]: Symbolic links for skill directories under ~/.openclaw/skills/ are not consistently resolved #49408 and is the better remaining tracker for porting/rebasing the fix.
• Existing review supports the logic: The Greptile review on fix(security): align audit symlink_escape boundary with skill loader #60155 agreed the boundary change is correct and well-tested, flagging only a minor test-comment mismatch, which supports preserving the idea via the canonical follow-up rather than treating it as invalid.

So I’m closing this here and keeping the remaining discussion on the canonical linked item.

Codex Review notes: model gpt-5.5, reasoning high; reviewed against e29d3516bf05.