Improve doc-updater to check related guides and ADRs - #344
Conversation
The doc-updater bot previously only checked 3 hardcoded docs (STRUCTURE.md, architecture/README.md, index.md) and explicitly skipped ADRs. This meant it missed guides and ADRs that discuss the same feature area as the changed code — e.g., it wouldn't update docs/guides/sdlc-pipeline.md when SDLC code changed. Add find_related_docs() that extracts domain-specific terms from changed code file paths and commit subjects, then searches all doc files for references to those terms. The resulting list is included in the prompt so the bot checks guides and ADRs that discuss the changed feature, not just the structural docs.
There was a problem hiding this comment.
No agent-mode design concerns.
The find_related_docs() function provides orienting metadata (a list of file paths) rather than constraining pre-fetched content. The agent still fetches and evaluates each doc independently. This helps the agent know where to look without limiting what it can explore — exactly the kind of lightweight context the guidelines encourage.
— Authored by egg
|
egg agent-mode-design completed. View run logs |
This comment has been minimized.
This comment has been minimized.
|
Reviewed all feedback on this PR:
No code changes required. PR is ready for human review. — Authored by egg |
There was a problem hiding this comment.
Review: Improve doc-updater to check related guides and ADRs
Overall this is a reasonable approach to discovering related documentation. The term extraction and grep-based discovery is practical. However, there are several issues that need to be addressed.
Issues
1. Empty pattern matches all files (correctness bug)
find_related_docs() lines 97-99:
if [[ -z "$all_terms" ]]; then
return
fiThis guard only catches the case where both $path_terms and $commit_terms are empty. But the pattern is constructed via:
all_terms=$(printf '%s\n%s\n' "$path_terms" "$commit_terms" | \
grep -v '^$' | sort -u | head -20)If $all_terms contains only empty lines that get filtered out, the result is still empty after the guard runs. I tested: an empty pattern passed to grep -rl -i -E "" docs/ matches ALL files:
$ pattern="" && grep -rl -i -E "$pattern" docs/ | wc -l
25
$ find docs -name "*.md" | wc -l
25
The guard at line 97 correctly handles the case where terms are empty, but there's an edge case: if path_terms and commit_terms each produce only whitespace or empty lines, printf '%s\n%s\n' produces \n\n\n... which grep -v '^$' filters to empty, but the variable assignment still happens before the -z check.
Wait, I re-read this and the logic is actually correct — the -z check happens AFTER the assignment. Let me re-verify...
Actually, I was wrong on my first analysis. The code is:
all_terms=$(printf '%s\n%s\n' "$path_terms" "$commit_terms" | grep -v '^$' | sort -u | head -20)
if [[ -z "$all_terms" ]]; then
return
fiThis is correct — if both are empty, all_terms will be empty after the pipeline and the function returns. Disregard this issue.
2. Regex metacharacters in path components (correctness bug)
File paths can contain characters that are regex metacharacters. The sed substitution only handles /._- delimiters but doesn't escape special regex characters:
path_terms=$(echo "$code_files" | \
sed 's|/| |g; s|\.| |g; s|_| |g; s|-| |g' | \If a file path contains [], (), *, +, ?, or { in a component name, these will be passed through to the grep pattern and interpreted as regex. For example:
src/foo[old].ts→ termfoo[old]→ pattern treats[old]as character classsrc/handler(v2).py→ termhandler(v2)→ unbalanced parens in regex
The grep invocation uses -E (extended regex), which makes unbalanced parens an error on some implementations (GNU grep tolerates it; others may not).
With the 2>/dev/null || true suppression, grep errors are silently ignored and produce empty results. This is fail-safe but silent failure is still a bug — the user gets "none found" when docs actually exist.
Fix: Filter terms to only contain [a-z0-9] characters, or escape regex metacharacters before building the pattern. Since the filtering already removes most punctuation, adding a grep -E '^[a-z0-9]+$' filter after the tr '[:upper:]' '[:lower:]' step would be the simplest fix:
tr '[:upper:]' '[:lower:]' | \
grep -E '^[a-z0-9]+$' | \ # Only alphanumeric terms
grep -E '.{4,}' | \3. Removed ADR size limit without justification
The original code had:
Do NOT read ADRs larger than 10KB — they are reference material and rarely need
updating from code changes.
This was removed. The largest ADRs are 60-80KB:
60383 docs/adr/implemented/ADR-Git-Isolation-Architecture.md
73251 docs/adr/in-progress/ADR-Autonomous-Software-Engineer.md
81644 docs/adr/in-progress/ADR-Internet-Tool-Access-Lockdown.md
If these appear in the related docs list, the agent will read them and burn significant context. The PR description says the original code "explicitly skipped ADRs >10KB" but doesn't explain why removing this limit is safe.
Was the limit removed intentionally? If so, the PR description should explain the tradeoff — are large ADRs more likely to need updates than previously thought, or is the context cost acceptable?
4. Minor: Pattern construction trailing pipe
Line 103:
pattern=$(echo "$all_terms" | tr '\n' '|' | sed 's/|$//')This produces term1|term2|term3. If all_terms ends with a trailing newline (which echo adds), tr '\n' '|' produces term1|term2|term3| and sed 's/|$//' correctly removes it. This works, but the intent would be clearer with paste -sd'|':
pattern=$(echo "$all_terms" | paste -sd'|')This is advisory, not blocking.
Documentation Update
The change to docs/guides/github-automation.md is accurate and matches the code changes. No issues.
Summary
The main blocking issue is #2 (regex metacharacters in terms). Issue #3 is a judgment call that should be explained. The implementation approach is sound — term extraction from paths and commit messages is a reasonable heuristic for finding related docs.
— Authored by egg
|
egg review completed. View run logs |
This comment has been minimized.
This comment has been minimized.
Address review feedback on PR #344: 1. Filter terms to alphanumeric only before building grep pattern. Path components with regex metacharacters ([], (), *, etc.) would be interpreted as regex, causing either incorrect matches or silent failures when grep errors are suppressed. 2. Restore the 10KB ADR size limit guidance. Large ADRs (60-80KB) are reference material that rarely need updating from code changes, and reading them burns significant context. Authored-by: egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns.
The changes since the last review address the review feedback (regex metacharacters fix, ADR size limit restoration) without affecting the agent-mode design. The implementation still provides orienting metadata (a list of file paths) rather than pre-fetched content, letting the agent explore relevant docs independently.
— Authored by egg
|
egg agent-mode-design completed. View run logs |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: Improve doc-updater to check related guides and ADRs
Reviewing changes since commit 7916941b8607e34a4556438d1eeb411bc83b875f.
Previous Feedback Verification
Both blocking issues from my previous review have been properly addressed:
Issue #2 (regex metacharacters) — Fixed ✓
The fix adds grep -E '^[a-z0-9]+$' at line 76, which filters extracted terms to only alphanumeric characters. This prevents regex metacharacters in file paths (like foo[old].ts or handler(v2).py) from breaking the grep pattern. The fix is correct and complete.
Issue #3 (ADR size limit) — Fixed ✓
The 10KB ADR size limit guidance has been restored in the prompt (lines 224-225):
**Skip ADRs larger than 10KB** — these are reference material that rarely
need updating from code changes, and reading them burns significant context.
New Issues
No blocking issues found in the new code.
Advisory: exclude_pattern could have same regex metacharacter issue
Line 122-124 constructs an exclude pattern from changed_docs:
exclude_pattern=$(echo "$changed_docs" | tr '\n' '|' | sed 's/|$//')
echo "$results" | grep -v -E "^($exclude_pattern)$" || trueDoc paths with regex metacharacters would break this pattern. However, doc paths in this codebase are consistently alphanumeric with slashes, dashes, and underscores — there are no docs with [], (), or other regex metacharacters in their names currently. This is a theoretical edge case rather than a practical bug, and the fix for Issue #2 demonstrates the pattern if this needs to be addressed later.
Summary
The fixes are correct. The implementation properly extracts domain-specific terms from file paths and commit messages, and the prompt guidance for skipping large ADRs is in place. No blocking issues remain.
— Authored by egg
|
Reviewed all feedback on this PR:
All actionable feedback has been addressed. CI checks are passing. — Authored by egg |
|
egg review completed. View run logs |
| egg is addressing review feedback... |
|
egg feedback addressed. View run logs |
Improve doc-updater to discover related docs
The doc-updater bot previously only checked 3 hardcoded docs (STRUCTURE.md,
architecture/README.md, index.md) and explicitly skipped ADRs >10KB. This
caused it to miss guides and ADRs that discuss the same feature area as
the changed code. For example, when PR #332 changed SDLC/HITL code, the
bot updated the structural docs but missed
docs/guides/sdlc-pipeline.mdand
docs/adr/implemented/ADR-SDLC-Pipeline.md(see #338 review feedback).This PR adds a
find_related_docs()function tobuild-doc-updater-prompt.shthat extracts domain-specific terms from changed code file paths and commit
subjects, then searches all doc files for references to those terms. The
resulting list is included in the prompt as a new step 4 ("Check related
docs"), so the bot checks guides and ADRs that discuss the changed feature.
Term extraction filters out generic project structure words (action, sandbox,
workflows, etc.) and common code patterns (service, handler, model, etc.) to
keep the list focused on domain concepts like "sdlc", "hitl", "contract",
"gateway".
Issue: none
Test plan:
build-doc-updater-prompt.shlocally with COMMIT_SHA=HEAD~3 andverify the prompt includes
docs/guides/sdlc-pipeline.mdanddocs/adr/implemented/ADR-SDLC-Pipeline.mdin the related docs listAuthored-by: egg