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

[rajarshidattapy]

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

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:

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:

   // 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.