anthropics / anthropics/claude-code-action

getFileMode uses stat(), making its isSymbolicLink() branch unreachable dead code

未关闭
#1,768 0 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看
bug p3
主要语言
TypeScript
星标
8.9k
派生
2.1k
PR 合并指标
PR 指标待抓取

描述

**Type:** bug / dead code
**Severity:** low
**Area:** `src/mcp/github-file-ops-server.ts`
**Effort:** small (the safe fix is trivial; full symlink support is not)

## Summary

`getFileMode` branches on `fileStat.isSymbolicLink()` to return git's symlink
mode `120000`. It calls `stat()`, which **follows** symlinks, so the returned
`Stats` never describes a link - `isSymbolicLink()` is always `false` and the
branch can never be taken.

## Affected code

`src/mcp/github-file-ops-server.ts:172-190`

```ts
const fileStat = await stat(filePath); // follows symlinks
if (fileStat.isFile()) {
...
} else if (fileStat.isDirectory()) {
return "040000";
} else if (fileStat.isSymbolicLink()) { // unreachable
return "120000";
} else {
return "100644";
}
```

## Actual behaviour today

A symlink passed to `commit_files` is committed as a **regular file** whose
content is the link target's content:

- `getFileMode` stats through the link and returns `100644` (or `100755`).
- `readFile(fullPath)` (line 261) also follows the link and reads the target's
bytes.
- The tree entry is a normal blob.

So the link is silently dereferenced. That is a defensible behaviour, but it is
not the behaviour the code claims to implement, and there is no test covering it.

## Why the obvious fix is wrong

Swapping `stat` for `lstat` in isolation makes things worse, not better. Git
stores a symlink as a blob whose *content is the target path*. With `lstat` alone
the entry would get mode `120000` while `readFile` still supplies the **target's
file content** as the blob - producing a committed "symlink" pointing at a path
made of the target file's bytes. That is a corrupt tree.

Correct symlink support requires changing both halves together:

```ts
const fileStat = await lstat(filePath);
if (fileStat.isSymbolicLink()) {
const target = await readlink(filePath);
// blob content must be `target`, mode 120000
}
```

...plus a decision about whether links escaping the repo should be rejected,
which interacts with `validatePathWithinRepo` (`src/mcp/path-validation.ts`).
`validatePathWithinRepo` already resolves symlinks and rejects ones landing
outside the repo root, so an in-repo link is the only case that reaches here.

## Suggested fix

Two options, in order of preference:

1. **Minimal and honest** - drop the unreachable branch and document that
symlinks are intentionally dereferenced:

```ts
// stat() follows symlinks, and readFile() below does too, so a symlinked
// path is committed as a regular blob holding the target's content. That is
// deliberate: committing mode 120000 would require the blob content to be
// the link target path instead.
const fileStat = await stat(filePath);
if (fileStat.isDirectory()) return "040000";
if (fileStat.isFile() && fileStat.mode & constants.S_IXUSR) return "100755";
return "100644";
```

2. **Full support** - `lstat` + `readlink`, with the blob content changed in
`commit_files` at the same time, and a test asserting the committed tree entry
for a symlink. Larger change; only worth it if preserving links in commits is
actually wanted.

Either way the current state - a branch that documents behaviour the code does
not have - should not stay.

贡献指南

打开贡献指南

评估

这个 Issue 还没有评估数据。

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。