fix: sync outdated stub docs + integrate LSP closeAllFiles with compact#1248
Open
DeviosLang wants to merge 1 commit into
Open
fix: sync outdated stub docs + integrate LSP closeAllFiles with compact#1248DeviosLang wants to merge 1 commit into
DeviosLang wants to merge 1 commit into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (9)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR updates feature documentation across multiple modules to reflect completion of previously stubbed implementations, refactors fork command routing to eliminate explicit parameters in favor of semantic routing, and fixes an LSP file-tracking memory leak during post-compaction cleanup. ChangesLSP Memory Leak Fix and Fork Routing Refactor
Feature Implementation Status Audit
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Remove redundant fork:true and run_in_background params in /fork command - Integrate LSP closeAllFiles() into postCompactCleanup (memory leak claude-code-best#14) - Update 10 docs files: mark implemented features no longer as stubs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DeviosLang
force-pushed
the
fix/docs-status-sync-and-memory-leak-14
branch
from
5a7400e to
5a53d83
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile`:
- Line 20: The Dockerfile currently uses COPY vendor/ ./vendor/ which fails the
build if the vendor directory is missing (preventing the later mkdir fallback
from running); remove or guard this COPY and instead ensure the directory exists
in the image with a non-failing command (e.g., replace the COPY vendor/
./vendor/ with RUN mkdir -p ./vendor so the image always has ./vendor), and if
you need to include real vendor files only when present, add them to the build
context or implement an explicit build-arg/conditional multi-stage step that
copies vendor files when provided rather than unconditionally using COPY
vendor/.
- Around line 45-59: The runtime stage in the Dockerfile runs as root by
default; add an explicit non-root user in that stage by creating a low-privilege
user/group (e.g., via addgroup/adduser or useradd), chowning the application
directories (WORKDIR /app, copied files like ./dist and ./node_modules) to that
user, and then setting USER to that non-root account before the ENTRYPOINT
("bun", "run", "dist/cli.js") so the CLI runs without elevated privileges; make
these changes inside the runtime stage (after COPY steps and before ENTRYPOINT)
and reference the Dockerfile runtime stage symbols (WORKDIR /app, COPY
--from=builder ..., and ENTRYPOINT) when applying the fix.
In `@docs/features/autofix-pr.md`:
- Line 3: The README currently marks the feature as "状态：已实现" while later
checkbox lists still look like outstanding TODOs; update the header label
"状态：已实现" (the top-status line) to something that matches the later checklist
context—either "已完成记录 / 历史计划" or "部分已实现（见下方历史计划）" and convert the checklist
items in the sections currently formatted as pending TODOs into completed
entries (e.g., change unchecked boxes to checked or add a "已完成" prefix) or add a
clear "历史计划" note above those lists so readers understand they are historical
records rather than open tasks. Ensure you update the exact string "状态：已实现" and
the checklist lines that use the TODO checkbox format to keep wording
consistent.
In `@docs/features/mcp-skills.md`:
- Around line 108-110: The docs section heading is misleading because the table
describing implemented items (including fetchMcpSkillsForClient and the mcp
skill builder registration) remains under "needs completion"; update the
document heading or move the table into an "Implementation details" or
"Implemented features" section so the table aligns with content status, and
ensure the entries referencing fetchMcpSkillsForClient and the MCP skill builder
registration are placed under that corrected heading.
In `@docs/features/web-browser-tool.md`:
- Line 4: Doc states the panel is intentionally null in the summary but later
calls it a stub—unify to one authoritative state: update the phrasing in the
summary line "实现状态：核心工具已实现，面板故意为 null（结果在对话中内联显示）" and all mentions at lines
referenced (including the sections describing 数据流, 待补全表, 文件索引 and the
occurrences around lines 17, 40, 48, 66) so they consistently state that the
panel is intentionally null and results are inlined in the conversation (or
consistently state the opposite if you prefer to mark it as a stub); ensure any
TODOs, data-flow diagrams and file-index entries reflect that same state and
remove contradictory wording like "仍为 Stub".
In `@docs/features/workflow-scripts.md`:
- Line 4: 顶层“已实现”与正文里对 WorkflowTool、LocalWorkflowTask、WorkflowDetailDialog
等模块的“Stub/待补全”描述不一致；请把文档中模块状态统一为真实实现状态：检查并修改文档顶部状态标签和正文中模块表、待补全章节与文件索引，使
WorkflowTool、LocalWorkflowTask、WorkflowDetailDialog
的状态与代码库/测试结果一致（如果已实现则把对应段落从“Stub/待补全”改为“已实现”并移除占位说明；如果仍未完成则把顶部“已实现”改为“部分实现/待补全”并在文件索引中标注缺失项），同时更新测试标注以反映现有测试覆盖。
In `@docs/task/task-004-assistant-session-attach.md`:
- Line 6: 把文档中当前写为“状态: Phase 4A-4D 全部实现”的声明与随后仍列出未完成检查项的段落进行对齐：将标题“状态: Phase
4A-4D
全部实现”改为“已完成验收记录”或在该标题后追加明确说明（例如“以下检查项为历史计划/归档记录，非当前待办”），并把文中仍显示未完成的检查项段落（即列出具体验收条目的段落）标注为“历史计划/已归档验收项”或移入单独的“历史记录”小节，确保文档头部的状态声明与后续条目语义一致（查找并修改包含文本“状态:
Phase 4A-4D 全部实现”和列出未完成检查项的段落进行更新）。
In `@scripts/docker-deploy.sh`:
- Around line 21-23: The flag parsing currently overwrites a user-provided TAG
when --registry is seen because the --registry case sets TAG and TAG_LATEST
immediately; change the parser so --registry only sets REGISTRY (do not compute
TAG/TAG_LATEST there) and treat --tag as authoritative if provided; after all
flags are parsed, compute TAG and TAG_LATEST from IMAGE_NAME, VERSION and
REGISTRY only when TAG was not explicitly set (i.e., if TAG is empty) so order
doesn’t matter and pushing uses the intended repo; update the cases for
--registry, --tag, and the push logic that uses TAG_LATEST accordingly.
In `@src/services/compact/postCompactCleanup.ts`:
- Line 108: Replace the relative dynamic import path used in the expression void
import('../lsp/manager.js') with the repo alias form: use void
import('src/lsp/manager.js') so it follows the tsconfig src/* mapping; update
the import expression in postCompactCleanup.ts where the dynamic import of
manager.js occurs to use the 'src/...' alias.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 477f3651-e707-49b0-950f-5ad4c04f2ef8
📒 Files selected for processing (13)
Dockerfiledocs/features/autofix-pr.mddocs/features/bash-classifier.mddocs/features/coordinator-mode.mddocs/features/fork-subagent.mddocs/features/mcp-skills.mddocs/features/web-browser-tool.mddocs/features/workflow-scripts.mddocs/memory-leak-audit.mddocs/task/task-004-assistant-session-attach.mdscripts/docker-deploy.shsrc/commands/fork/fork.tsxsrc/services/compact/postCompactCleanup.ts
| COPY src/ ./src/ | ||
| COPY tsconfig.json biome.json ./ | ||
| COPY tests/ ./tests/ | ||
| COPY vendor/ ./vendor/ |
Contributor
There was a problem hiding this comment.
COPY vendor/ can break builds when the directory is absent
Line 20 and Line 37 copy vendor/, but Line 39 explicitly notes the binary may be missing. Docker will fail on COPY before reaching the mkdir fallback at Line 40.
Suggested fix
- COPY vendor/ ./vendor/
+ # vendor/ may be absent in some environments; do not hard-fail copy
...
- COPY vendor/ ./vendor/
- # ripgrep vendor binary may not exist in repo (platform-specific, downloaded separately)
- RUN mkdir -p src/utils/vendor/ripgrep
+ # ripgrep vendor binary may not exist in repo (platform-specific, downloaded separately)
+ RUN mkdir -p src/utils/vendor/ripgrepAlso applies to: 37-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile` at line 20, The Dockerfile currently uses COPY vendor/ ./vendor/
which fails the build if the vendor directory is missing (preventing the later
mkdir fallback from running); remove or guard this COPY and instead ensure the
directory exists in the image with a non-failing command (e.g., replace the COPY
vendor/ ./vendor/ with RUN mkdir -p ./vendor so the image always has ./vendor),
and if you need to include real vendor files only when present, add them to the
build context or implement an explicit build-arg/conditional multi-stage step
that copies vendor files when provided rather than unconditionally using COPY
vendor/.
Comment on lines
+45
to
+59
| FROM oven/bun:1-slim AS runtime | ||
|
|
||
| WORKDIR /app | ||
|
|
||
| # Copy built artifacts | ||
| COPY --from=builder /app/dist ./dist | ||
| COPY --from=builder /app/node_modules ./node_modules | ||
| COPY --from=builder /app/package.json ./ | ||
|
|
||
| # Vendor binaries (ripgrep, audio-capture) | ||
| COPY --from=builder /app/dist/vendor ./dist/vendor | ||
|
|
||
| ENV NODE_ENV=production | ||
|
|
||
| ENTRYPOINT ["bun", "run", "dist/cli.js"] |
Contributor
There was a problem hiding this comment.
Harden runtime stage with an explicit non-root user
The runtime stage has no USER instruction, so execution privileges depend on image defaults. Add an explicit non-root user in this stage to avoid running the CLI with elevated permissions.
Suggested fix
FROM oven/bun:1-slim AS runtime
...
ENV NODE_ENV=production
+USER bun
ENTRYPOINT ["bun", "run", "dist/cli.js"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| FROM oven/bun:1-slim AS runtime | |
| WORKDIR /app | |
| # Copy built artifacts | |
| COPY --from=builder /app/dist ./dist | |
| COPY --from=builder /app/node_modules ./node_modules | |
| COPY --from=builder /app/package.json ./ | |
| # Vendor binaries (ripgrep, audio-capture) | |
| COPY --from=builder /app/dist/vendor ./dist/vendor | |
| ENV NODE_ENV=production | |
| ENTRYPOINT ["bun", "run", "dist/cli.js"] | |
| FROM oven/bun:1-slim AS runtime | |
| WORKDIR /app | |
| # Copy built artifacts | |
| COPY --from=builder /app/dist ./dist | |
| COPY --from=builder /app/node_modules ./node_modules | |
| COPY --from=builder /app/package.json ./ | |
| # Vendor binaries (ripgrep, audio-capture) | |
| COPY --from=builder /app/dist/vendor ./dist/vendor | |
| ENV NODE_ENV=production | |
| USER bun | |
| ENTRYPOINT ["bun", "run", "dist/cli.js"] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile` around lines 45 - 59, The runtime stage in the Dockerfile runs as
root by default; add an explicit non-root user in that stage by creating a
low-privilege user/group (e.g., via addgroup/adduser or useradd), chowning the
application directories (WORKDIR /app, copied files like ./dist and
./node_modules) to that user, and then setting USER to that non-root account
before the ENTRYPOINT ("bun", "run", "dist/cli.js") so the CLI runs without
elevated privileges; make these changes inside the runtime stage (after COPY
steps and before ENTRYPOINT) and reference the Dockerfile runtime stage symbols
(WORKDIR /app, COPY --from=builder ..., and ENTRYPOINT) when applying the fix.
| # `/autofix-pr` 命令实现规格文档 | ||
|
|
||
| > **状态**：规划阶段（2026-04-29），等待评审通过后进入实施。 | ||
| > **状态**：已实现（598 行代码 + 5 个测试文件）。 |
Contributor
There was a problem hiding this comment.
“已实现”状态与后文待办清单冲突。
Line 3 标记为已实现，但 Line 677-695 与 Line 729-737 仍是未完成清单格式，读者会误判当前真实状态。建议将这些清单改为“已完成记录”或补充“历史计划”说明。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/features/autofix-pr.md` at line 3, The README currently marks the
feature as "状态：已实现" while later checkbox lists still look like outstanding
TODOs; update the header label "状态：已实现" (the top-status line) to something that
matches the later checklist context—either "已完成记录 / 历史计划" or "部分已实现（见下方历史计划）"
and convert the checklist items in the sections currently formatted as pending
TODOs into completed entries (e.g., change unchecked boxes to checked or add a
"已完成" prefix) or add a clear "历史计划" note above those lists so readers understand
they are historical records rather than open tasks. Ensure you update the exact
string "状态：已实现" and the checklist lines that use the TODO checkbox format to
keep wording consistent.
Comment on lines
+21
to
+23
| --registry) REGISTRY="$2"; TAG="${REGISTRY}/${IMAGE_NAME}:${VERSION}"; TAG_LATEST="${REGISTRY}/${IMAGE_NAME}:latest"; shift 2 ;; | ||
| --tag) TAG="$2"; shift 2 ;; | ||
| --no-push) NO_PUSH=true; shift ;; |
Contributor
There was a problem hiding this comment.
Flag parsing is order-dependent and can push an unintended latest tag
Line 21 rewrites TAG whenever --registry is parsed, and Line 22 only updates TAG. This makes --tag behavior order-sensitive and still pushes TAG_LATEST (Line 53) to the registry-derived path, which can publish to an unintended repo.
Suggested fix
REGISTRY="${REGISTRY:-csighub.tencentyun.com/claude-code}"
IMAGE_NAME="claude-code"
VERSION=$(grep '"version"' package.json | head -1 | sed 's/.*"\([0-9.]*\)".*/\1/')
-TAG="${REGISTRY}/${IMAGE_NAME}:${VERSION}"
-TAG_LATEST="${REGISTRY}/${IMAGE_NAME}:latest"
+TAG=""
+TAG_LATEST=""
NO_PUSH=false
RESTART_POD=false
+CUSTOM_TAG=false
while [[ $# -gt 0 ]]; do
case "$1" in
- --registry) REGISTRY="$2"; TAG="${REGISTRY}/${IMAGE_NAME}:${VERSION}"; TAG_LATEST="${REGISTRY}/${IMAGE_NAME}:latest"; shift 2 ;;
- --tag) TAG="$2"; shift 2 ;;
+ --registry)
+ [[ $# -lt 2 ]] && { echo "Missing value for --registry"; exit 1; }
+ REGISTRY="$2"; shift 2 ;;
+ --tag)
+ [[ $# -lt 2 ]] && { echo "Missing value for --tag"; exit 1; }
+ TAG="$2"; CUSTOM_TAG=true; shift 2 ;;
--no-push) NO_PUSH=true; shift ;;
--restart-pod) RESTART_POD=true; shift ;;
*) echo "Unknown option: $1"; exit 1 ;;
esac
done
+
+if [ "${CUSTOM_TAG}" = "false" ]; then
+ TAG="${REGISTRY}/${IMAGE_NAME}:${VERSION}"
+fi
+TAG_LATEST="${REGISTRY}/${IMAGE_NAME}:latest"Also applies to: 50-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/docker-deploy.sh` around lines 21 - 23, The flag parsing currently
overwrites a user-provided TAG when --registry is seen because the --registry
case sets TAG and TAG_LATEST immediately; change the parser so --registry only
sets REGISTRY (do not compute TAG/TAG_LATEST there) and treat --tag as
authoritative if provided; after all flags are parsed, compute TAG and
TAG_LATEST from IMAGE_NAME, VERSION and REGISTRY only when TAG was not
explicitly set (i.e., if TAG is empty) so order doesn’t matter and pushing uses
the intended repo; update the cases for --registry, --tag, and the push logic
that uses TAG_LATEST accordingly.
| // Next file access re-opens only what's needed, preventing unbounded growth | ||
| // in long sessions. Fire-and-forget: best-effort release, non-blocking. | ||
| if (isMainThreadCompact) { | ||
| void import('../lsp/manager.js') |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use src/* alias for the dynamic import on Line 108.
This new relative import path should follow the repo alias convention for source imports.
Suggested change
- void import('../lsp/manager.js')
+ void import('src/services/lsp/manager.js')As per coding guidelines, "Import paths must use the src/* alias (e.g., import { ... } from 'src/utils/...'); tsconfig maps src/* to ./src/*."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void import('../lsp/manager.js') | |
| void import('src/services/lsp/manager.js') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/compact/postCompactCleanup.ts` at line 108, Replace the relative
dynamic import path used in the expression void import('../lsp/manager.js') with
the repo alias form: use void import('src/lsp/manager.js') so it follows the
tsconfig src/* mapping; update the import expression in postCompactCleanup.ts
where the dynamic import of manager.js occurs to use the 'src/...' alias.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/forkcommand (fork:true, run_in_background)LSP closeAllFiles()intopostCompactCleanup— fixes memory leak 估计国内的agent 工具会迎来大更新 #14Test plan
npx tsc --noEmit— zero errorsdocker run --rm <image> --version→ 2.6.0docker run --rm <image> --help→ normal outputbun run precheck(typecheck + lint + test)🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
/autofix-prcommand, workflow scripts, MCP skills, bash classifier, and coordinator mode. Clarified/forkcommand and web browser tool implementation details.Bug Fixes