feat(background-file) Add the background-file CLI option

[Victor-D]

feat(background-file) Add the background-file CLI option to read a local business context file

Description

Following discussion in #176 add the new -B flag :

  -b, --background string       optional requirement/business context for the review
  -B, --background-file string  path to a Markdown file used as review background (combined with --background; inline value appears first when both are set)

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

How Has This Been Tested?

• make test passes locally
• Manual testing (describe below)

soft limit

% ./dist/opencodereview review --repo /home/USER/bitbucket/REPO_NAME --commit <commit-hash> -B /tmp/context.md
[ocr] --background-file content is 4623 characters, exceeding the recommended 2000 (continuing but review quality might be impacted)
[ocr] 2 file(s) changed, reviewing 1 in /home/USER/bitbucket/REPO_NAME
[ocr] Skipping hieradata/roles/web.eyaml — filtered by path/extension rules
[ocr] Filtered 1 file(s) by include/exclude rules

hard limit

user@HOSTNAME open-code-review % ./dist/opencodereview review --repo /home/USER/bitbucket/REPO_NAME --commit <commit-hash> -B /tmp/context.md
Error: background content is 14631 characters, exceeding the hard limit of 8000 (aborting)

I also checked I see the two prompts in the session file when using -b and -B flags.

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly (if applicable)
• I have signed the CLA

Related Issues

[CLAassistant]

All committers have signed the CLA.

[github-actions]

🔍 OpenCodeReview found 2 issue(s) in this PR.

• ✅ 2 posted as inline comment(s)
• 📝 0 posted as summary

[lizhengfeng101]

Code Review

Overall: Good quality PR with thorough test coverage. A few issues to address before merging.

---

Issue 1 (Bug): mergeBackground unconditionally changes existing --background behavior

In review_cmd.go, mergeBackground is called even when --background-file is not used:

opts.background = mergeBackground(opts.background, fileBackground)

Since mergeBackground internally calls sanitizeMarkdown(inline), users who only use --background (without -B) will now have their content sanitized (control chars stripped, excess newlines collapsed, whitespace trimmed). This is an implicit behavioral change for existing users.

Suggestion: Only call mergeBackground when backgroundFile is actually set:

if opts.backgroundFile != "" {
    fileBackground, err = loadBackgroundFile(opts.backgroundFile)
    if err != nil {
        return err
    }
    opts.background = mergeBackground(opts.background, fileBackground)
}

---

Issue 2 (Bug): Localized README files not updated

The project requires all localized READMEs to be kept in sync when README.md is modified (README.zh-CN.md, README.ja-JP.md, README.ko-KR.md, README.ru-RU.md). The PR adds the --background-file row and usage examples to README.md but none of the four localized versions were updated.

---

Issue 3 (Design): Inconsistent wrapping between inline and file content

loadBackgroundFile wraps file content in <ocr_user_background> tags, but mergeBackground does not apply the same wrapping to inline content. After merging, opts.background looks like:

inline context here

<ocr_user_background>
file content here
</ocr_user_background>

If this asymmetry is intentional (e.g. inline content is short and trusted), it would be worth a brief code comment explaining why.

---

Issue 4 (Minor): Hard limit error message includes wrapper overhead

The character count check in loadBackgroundFile runs on the wrapped string (after adding <ocr_user_background> tags, ~40+ chars of overhead):

if n := len([]rune(wrapped)); n > backgroundHardLimit {

The error message says "background content is X characters, exceeding the hard limit of 8000" but X includes wrapper overhead that the user cannot control. Consider either checking cleaned instead of wrapped, or adjusting the limit to account for the overhead internally.

---

Issue 5 (Minor): Redundant checks in isForbiddenChar

All characters listed in the switch (​, ‌, ‍, ‎, ‏, ⁠, ­, ) belong to Unicode category Cf, which is already caught by the final unicode.Is(unicode.Cf, r) call. The explicit entries serve as documentation, so this is not a correctness issue — just noting the redundancy.

---

Issue 6 (Minor): Performance test has no assertion

TestLoadBackgroundFilePerformance runs 1000 iterations of sanitizeMarkdown but doesn't assert on timing. To actually guard against quadratic regressions, consider converting it to a func BenchmarkSanitizeMarkdown(b *testing.B) so go test -bench produces meaningful data.

---

Highlights

• Excellent test coverage: edge cases for empty files, directories, oversized files, multi-byte runes, delimiter injection, CRLF normalization, soft/hard limits
• TestBackgroundFromCommitThenFile is a well-designed end-to-end test for the commit message + file merge path
• Good security considerations: reserved delimiter injection check, file size limits, control character sanitization
• Help text alignment cleanup is a nice touch

Category	Count
Bug / Must fix	2 (behavioral change + README localization)
Design discussion	1 (wrapping inconsistency)
Minor suggestions	3