Repository navigation
server: memorySkillFiles.list() and read() do not normalize or validate non-canonical relative paths #3102
Description
Quick question on memorySkillFiles in packages/server/src/ext/skills/files.ts (lines 53-67 on main commit 46d9c99). indexRecord normalizes keys via splitRelativePath(key, 'file path').join('/') when building the dirs and files maps, but memorySkillFiles.list(dir) and memorySkillFiles.read(path) look up dir and path directly without normalizing them through splitRelativePath first (unlike nodeSkillFiles in node.ts).
Because of that, calling list('sub/') or read('./SKILL.md') throws SkillFileNotFoundError, and in read(path) the Object.hasOwn(files, path) check runs before index ?? reindex(), so calling read() before list() bypasses splitRelativePath validation when path matches a raw key on files. Should list() and read() normalize dir and path via splitRelativePath? Happy to open a quick PR with unit tests if you want!
Activity
xubenqiang commentedon Oct 26, 2026
I went ahead with this: #3104 normalises
dir/paththroughsplitRelativePathin bothlist()andread()(with''still meaning the record root), sosub/,./SKILL.mdandsub//other.txtresolve, and a..segment is rejected before thehasOwnfast path. Six tests cover those shapes plus the unchanged missing-path errors, and there is a patch changeset.Ben-Home commentedon Oct 28, 2026
Yes to normalising in
list()andread(), and the detail in your second paragraph is worth more than the normalisation request itself.Object.hasOwn(files, path)running beforeindex ?? reindex()means aread()that happens before anylist()can match a raw key onfilesthatsplitRelativePathwould have rejected or canonicalised. That is a validation bypass, not a convenience gap, so it wants its own test rather than being folded into the normalisation change.Three behaviours I would pin in units:
list('sub/'),list('./sub')andlist('sub//')return the same set aslist('sub').read('./SKILL.md')andread('SKILL.md')return the same record, and a path that escapes the skill root throws instead of resolving.read(<a raw key that is not canonical>)on a fresh instance throws the same error thatlist()thenread()would, which is the assertion that closes the bypass.Point 3 is the one most likely to fail against
maintoday, and it is also the regression test that stops the ordering from being reintroduced later.nodeSkillFilesalready routing throughsplitRelativePathis a good precedent, and mirroring it infiles.tsrather than adding a second normalisation path keeps the two implementations comparable for whoever touches the third one.