Skip to content

refactor(context): system prompt section 化重构#115

Merged
minorcell merged 2 commits into
1024XEngineer:mainfrom
wynxing:feat
Apr 2, 2026
Merged

refactor(context): system prompt section 化重构#115
minorcell merged 2 commits into
1024XEngineer:mainfrom
wynxing:feat

Conversation

@wynxing

@wynxing wynxing commented Apr 2, 2026

Copy link
Copy Markdown
Collaborator

概要

  • internal/context 中的 system prompt 从单一硬编码字符串重构为可组合的 section 结构。
  • 保持 runtime -> provider 接口不变,仍由 builder 输出最终 SystemPrompt 字符串。
  • 同步补充 section 顺序、rules 截断、system state 摘要等测试,并更新相关文档描述。

验证

  • go test ./...

关闭 #104

@wynxing wynxing marked this pull request as ready for review April 2, 2026 01:31
@codecov

codecov Bot commented Apr 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@minorcell

Copy link
Copy Markdown
Member

/review

- Base claims on the current workspace state instead of generic advice.`),
},
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance: Repeated Allocations

This function allocates a new 6-element slice and calls strings.TrimSpace() 6 times on every Build() call (happens on every ReAct loop iteration, potentially 8+ times per request). The content is static and never changes.

Suggestion: Cache the result as a package-level variable:

var defaultSections = []promptSection{
    {id: "agent-identity", title: "Agent Identity", content: "..."},
    // ... other sections (already trimmed)
}

func defaultSystemPromptSections() []promptSection {
    return defaultSections
}

This eliminates 6 string allocations and 6 TrimSpace operations per Build() call.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已处理。这次把默认 section 提升为包级缓存,defaultSystemPromptSections() 现在直接返回缓存结果,避免每轮 Build() 重建切片和重复 TrimSpace

Comment thread internal/context/prompt.go Outdated
case content == "":
return ""
default:
return "## " + title + "\n\n" + content

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance: Inefficient String Concatenation

String concatenation with + operator creates multiple intermediate string allocations. For large content sections (project rules up to 12000 runes), this is inefficient.

Suggestion: Use strings.Builder:

var builder strings.Builder
builder.Grow(len(section.title) + len(section.content) + 6)
builder.WriteString("## ")
builder.WriteString(section.title)
builder.WriteString("\n\n")
builder.WriteString(section.content)
return builder.String()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已处理。renderPromptSection() 已改为使用 strings.Builder 渲染,减少额外字符串分配。

return `You are NeoCode, a local coding agent.
type promptSection struct {
id string
title string

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Quality: Unused Field

The id field is set throughout the codebase but never used anywhere. This adds cognitive overhead without providing value.

Suggestion: Either remove the unused field, or add documentation explaining its reserved purpose for future use (e.g., section deduplication, lookup, or ordering).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已处理。考虑到当前实现还没有 section 去重或查找场景,这里直接删除了未使用的 id 字段,先保持结构最小化。

Comment thread docs/runtime-provider-event-flow.md Outdated
- 规则文件只支持大写文件名 `AGENTS.md`
- 多份命中结果按“从全局到局部”的顺序注入
- git 只注入摘要,不注入完整 `git status`
- 各 section 统一由 `internal/context` 内部 composer 渲染,`runtime` 仍只消费最终字符串

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation: Non-existent Term

The term "composer" doesn't exist in the code. The actual functions are renderPromptSection and composeSystemPrompt.

Suggestion: Change to: "各 section 统一由 internal/context 内部的 renderPromptSectioncomposeSystemPrompt 函数渲染"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已处理。文档里的 composer 表述已经改成当前代码中的实际函数:renderPromptSectioncomposeSystemPrompt

@minorcell

Copy link
Copy Markdown
Member

Code Review Summary

This PR introduces a well-structured refactoring to make system prompts composable. The changes maintain backward compatibility while improving code organization. However, several noteworthy issues were identified across code quality, performance, security, and documentation areas.

Critical Security Issues (Pre-existing, not introduced by this PR)

While reviewing this PR, I discovered several critical security vulnerabilities in existing code that should be addressed:

  1. Prompt Injection via Backticks (source_system.go): The promptValue() function doesn't escape backticks in metadata fields. A malicious git branch name like main` ## FAKE\n\nInjection could inject arbitrary sections into the system prompt.

  2. Path Traversal (source_rules.go:62-92): discoverRuleFilesWithFinder() walks to filesystem root without boundary validation. Setting workdir to /etc or ../../etc could expose sensitive directories.

  3. Symlink Following (source_rules.go:110-115): findExactRuleFile() doesn't check for symlinks. An attacker could create AGENTS.md symlinks pointing to /etc/passwd, AWS credentials, or SSH keys.

  4. Resource Exhaustion (source_rules.go:42): loadRuleDocuments() uses os.ReadFile() which loads entire files before truncation. A multi-GB AGENTS.md file could cause DoS.

Recommendation: These vulnerabilities should be addressed in a follow-up security PR before deploying to production.

Performance Optimizations

Two performance issues were identified in the new code that compound because Build() is called on every ReAct loop iteration (8+ times per request):

  • Addressed inline: Repeated allocations in defaultSystemPromptSections() - caching would eliminate 6 string allocations per call
  • Addressed inline: Inefficient concatenation in renderPromptSection() - using strings.Builder would reduce allocations

Code Quality & Documentation

  • Addressed inline: Unused id field in promptSection struct adds cognitive overhead
  • Addressed inline: Documentation uses non-existent "composer" term

Test Coverage

The PR includes excellent test coverage with new tests for section ordering, blank line prevention, and truncation behavior. Well done!

@minorcell minorcell merged commit d6ea040 into 1024XEngineer:main Apr 2, 2026
2 checks passed
@wynxing

wynxing commented Apr 2, 2026

Copy link
Copy Markdown
Collaborator Author

已根据本轮 review 收敛了当前 PR 范围内的问题:缓存默认 sections、将 renderPromptSection() 改为 strings.Builder、删除未使用的 id 字段、补充 prompt.go 的分支测试,并修正文档中的失真表述。

review 总结里提到的几项安全问题我认同需要跟进,但它们都属于本次重构前就已存在的问题,而且范围已经超出 issue #104,因此这次没有混入当前 PR,后续更适合单独开 issue 或 PR 处理。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants