fix: skip repeated workspace file injection after first turn; include agentDir bootstrap files#52916
Conversation
|
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. |
Greptile SummaryThis PR addresses two orthogonal issues: it eliminates repeated workspace bootstrap-file injection on every message turn (
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/pi-embedded-runner/run/attempt.ts
Line: 1688-1691
Comment:
**Duplicate `stat` on session file**
`isSubsequentTurn` is derived from `stat(params.sessionFile)` here, but an identical check is repeated ~340 lines later as `hadSessionFile` (line ~2029). Since `repairSessionFileIfNeeded` rewrites rather than deletes the file, both checks will always agree in practice.
Consider deriving `isSubsequentTurn` from `hadSessionFile` to avoid the redundant syscall and keep the source-of-truth in one place. This would require moving bootstrap resolution to after the session lock block, or threading `hadSessionFile` back up — but even a short comment linking the two would help future readers.
```suggestion
// isSubsequentTurn is determined from the session-file stat that also drives hadSessionFile
// (see ~line 2029). Pre-check here so bootstrap resolution (below) can skip token injection
// on subsequent turns. repair does not delete the file, so both checks agree.
const isSubsequentTurn = await fs
.stat(params.sessionFile)
.then(() => true)
.catch(() => false);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/bootstrap-files.ts
Line: 116-121
Comment:
**Path comparison may miss semantically-identical paths**
`resolveUserPath` only expands `~` prefixes and trims whitespace — it does not call `path.resolve` or `path.normalize`. Two paths that refer to the same directory but are expressed differently (e.g. trailing slash, embedded `..`, or symlinks) would compare as unequal, causing `mergeBootstrapFilesFromAgentDir` to run unnecessarily and potentially produce duplicate entries.
Consider normalising both paths with `path.resolve` before comparing:
```suggestion
const resolvedAgentDir = params.agentDir ? path.resolve(resolveUserPath(params.agentDir)) : undefined;
const resolvedWorkspaceDir = path.resolve(resolveUserPath(params.workspaceDir));
```
(`path` is already imported in this file's peer code; add `import path from "node:path"` at the top if not already present.)
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: skip repeated workspace file inject..." | Re-trigger Greptile |
| const isSubsequentTurn = await fs | ||
| .stat(params.sessionFile) | ||
| .then(() => true) | ||
| .catch(() => false); |
There was a problem hiding this comment.
stat on session file
isSubsequentTurn is derived from stat(params.sessionFile) here, but an identical check is repeated ~340 lines later as hadSessionFile (line ~2029). Since repairSessionFileIfNeeded rewrites rather than deletes the file, both checks will always agree in practice.
Consider deriving isSubsequentTurn from hadSessionFile to avoid the redundant syscall and keep the source-of-truth in one place. This would require moving bootstrap resolution to after the session lock block, or threading hadSessionFile back up — but even a short comment linking the two would help future readers.
| const isSubsequentTurn = await fs | |
| .stat(params.sessionFile) | |
| .then(() => true) | |
| .catch(() => false); | |
| // isSubsequentTurn is determined from the session-file stat that also drives hadSessionFile | |
| // (see ~line 2029). Pre-check here so bootstrap resolution (below) can skip token injection | |
| // on subsequent turns. repair does not delete the file, so both checks agree. | |
| const isSubsequentTurn = await fs | |
| .stat(params.sessionFile) | |
| .then(() => true) | |
| .catch(() => false); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/run/attempt.ts
Line: 1688-1691
Comment:
**Duplicate `stat` on session file**
`isSubsequentTurn` is derived from `stat(params.sessionFile)` here, but an identical check is repeated ~340 lines later as `hadSessionFile` (line ~2029). Since `repairSessionFileIfNeeded` rewrites rather than deletes the file, both checks will always agree in practice.
Consider deriving `isSubsequentTurn` from `hadSessionFile` to avoid the redundant syscall and keep the source-of-truth in one place. This would require moving bootstrap resolution to after the session lock block, or threading `hadSessionFile` back up — but even a short comment linking the two would help future readers.
```suggestion
// isSubsequentTurn is determined from the session-file stat that also drives hadSessionFile
// (see ~line 2029). Pre-check here so bootstrap resolution (below) can skip token injection
// on subsequent turns. repair does not delete the file, so both checks agree.
const isSubsequentTurn = await fs
.stat(params.sessionFile)
.then(() => true)
.catch(() => false);
```
How can I resolve this? If you propose a fix, please make it concise.| const resolvedAgentDir = params.agentDir ? resolveUserPath(params.agentDir) : undefined; | ||
| const resolvedWorkspaceDir = resolveUserPath(params.workspaceDir); | ||
| const mergedRawFiles = | ||
| resolvedAgentDir && resolvedAgentDir !== resolvedWorkspaceDir | ||
| ? await mergeBootstrapFilesFromAgentDir(rawFiles, resolvedAgentDir) | ||
| : rawFiles; |
There was a problem hiding this comment.
resolveUserPath only expands ~ prefixes and trims whitespace — it does not call path.resolve or path.normalize. Two paths that refer to the same directory but are expressed differently (e.g. trailing slash, embedded .., or symlinks) would compare as unequal, causing mergeBootstrapFilesFromAgentDir to run unnecessarily and potentially produce duplicate entries.
Consider normalising both paths with path.resolve before comparing:
| const resolvedAgentDir = params.agentDir ? resolveUserPath(params.agentDir) : undefined; | |
| const resolvedWorkspaceDir = resolveUserPath(params.workspaceDir); | |
| const mergedRawFiles = | |
| resolvedAgentDir && resolvedAgentDir !== resolvedWorkspaceDir | |
| ? await mergeBootstrapFilesFromAgentDir(rawFiles, resolvedAgentDir) | |
| : rawFiles; | |
| const resolvedAgentDir = params.agentDir ? path.resolve(resolveUserPath(params.agentDir)) : undefined; | |
| const resolvedWorkspaceDir = path.resolve(resolveUserPath(params.workspaceDir)); |
(path is already imported in this file's peer code; add import path from "node:path" at the top if not already present.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/bootstrap-files.ts
Line: 116-121
Comment:
**Path comparison may miss semantically-identical paths**
`resolveUserPath` only expands `~` prefixes and trims whitespace — it does not call `path.resolve` or `path.normalize`. Two paths that refer to the same directory but are expressed differently (e.g. trailing slash, embedded `..`, or symlinks) would compare as unequal, causing `mergeBootstrapFilesFromAgentDir` to run unnecessarily and potentially produce duplicate entries.
Consider normalising both paths with `path.resolve` before comparing:
```suggestion
const resolvedAgentDir = params.agentDir ? path.resolve(resolveUserPath(params.agentDir)) : undefined;
const resolvedWorkspaceDir = path.resolve(resolveUserPath(params.workspaceDir));
```
(`path` is already imported in this file's peer code; add `import path from "node:path"` at the top if not already present.)
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 890afe5d13
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const isSubsequentTurn = await fs | ||
| .stat(params.sessionFile) | ||
| .then(() => true) |
There was a problem hiding this comment.
Base turn detection on transcript state, not file existence
isSubsequentTurn is derived from fs.stat(sessionFile), which marks any existing file as a later turn even when the transcript is empty/header-only (for example pre-created files or a failed first attempt). In those cases this path skips contextFiles injection, so bootstrap instructions (AGENTS/BOOTSTRAP, etc.) are omitted on what is effectively the first usable turn. This is reachable because the runner already handles hadSessionFile && !hasAssistant as an initial-state repair path; turn detection should similarly check transcript content (at least one persisted turn) rather than file existence alone.
Useful? React with 👍 / 👎.
Summary
Fixes #9157 and #29387.
#9157 — Token waste from repeated workspace file injection
resolveBootstrapContextForRun()was called on every message turn, re-injecting workspace files (AGENTS.md, CLAUDE.md, etc.) each time#29387 — agentDir bootstrap files silently ignored
workspacedirectory, ignoring files inagentDiragentDirTest plan
pnpm test -- src/agents/bootstrap-files.test.tspasses🤖 Generated with Claude Code