Skip to content

fix: account for CWD when building git ls-tree pathspecs - #75

Merged
csoceanu merged 3 commits into
redhat-community-ai-tools:mainfrom
csoceanu:fix/ls-tree-cwd-relative-pathspec
Aug 17, 2026
Merged

fix: account for CWD when building git ls-tree pathspecs#75
csoceanu merged 3 commits into
redhat-community-ai-tools:mainfrom
csoceanu:fix/ls-tree-cwd-relative-pathspec

Conversation

@csoceanu

Copy link
Copy Markdown
Collaborator

Summary

git ls-tree pathspecs are CWD-relative. In same-repo mode, setup_docs_environment() does os.chdir(DOCS_SUBFOLDER), so CWD is inside the docs folder. The ref-based functions (get_folder_doc_hashes_from_ref, _get_docs_content_from_ref) built pathspecs like docs/commands — but from inside docs/, git interpreted this as docs/docs/commands which doesn't exist, returning empty results.

This caused [review-docs] on merged PRs to skip all index operations and return "No documentation updates needed" even when docs needed updating (seen on migtools/crane PR #790).

Fix

Added _get_effective_subfolder() that checks whether CWD is already inside the docs subfolder (same logic as get_docs_root()) and omits the prefix when it is.

Test plan

  • All 418 tests pass (1 new test for CWD-inside-subfolder scenario)
  • Ruff check and format clean
  • Verified: git ls-tree origin/main -- docs returns 0 results from inside docs/
  • Verified: git ls-tree origin/main -- . returns correct results from inside docs/

🤖 Generated with Claude Code

git ls-tree pathspecs are CWD-relative. When the tool runs in same-repo
mode, setup_docs_environment() does os.chdir(DOCS_SUBFOLDER), so CWD is
inside the docs folder. The ref-based functions then built pathspecs
like "docs/commands" — but from inside docs/, git interpreted this as
"docs/docs/commands" which doesn't exist, returning empty results.

Added _get_effective_subfolder() that checks whether CWD is already
inside the docs subfolder (same logic as get_docs_root()) and omits
the prefix when it is.

This caused [review-docs] on merged PRs to skip all index operations
and return "No documentation updates needed" (seen on crane PR #790).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 17, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:21 AM UTC · Completed 8:37 AM UTC

Commit: 4d1004b · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 17, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [edge-case] src/doc_index.py:248_get_effective_subfolder() uses exact equality (prefix == docs_subfolder) to detect when CWD is inside the docs subfolder. If CWD were deeper than the subfolder root (e.g., docs/commands/ while DOCS_SUBFOLDER is docs), the prefix would not match and the function would return the full subfolder, producing a double-prefixed pathspec. In current code, setup_docs_environment() only does os.chdir(docs_subfolder) (never deeper), so this edge case is not triggered today.
    Remediation: Consider adding a comment documenting the assumption that CWD is exactly the subfolder root, never deeper.
Previous run

Review

Findings

Low

  • [edge-case] src/doc_index.py:240 — The _get_effective_subfolder() heuristic uses a filesystem existence check to determine whether CWD is inside the docs subfolder. This can produce a false result if the subfolder contains a same-named sub-directory (e.g., docs/docs/). The scenario is unlikely and the heuristic is consistent with get_docs_root().

  • [test-coverage-gap] tests/test_doc_index.py:345 — The new test_handles_docs_subfolder_from_inside test verifies the output but does not assert the actual pathspec passed to git ls-tree. Consider adding an assertion on the constructed pathspec to verify _get_effective_subfolder() is working correctly.

  • [design-concern] src/doc_index.py:232 — The _get_effective_subfolder() function uses a CWD-dependent filesystem check. The logic is intentional but implicit — consider adding a brief inline comment explaining the CWD-detection mechanism for future maintainers.

Previous run (2)

Review

Findings

Medium

  • [logic-error] src/doc_index.py:311 — The fix correctly makes git ls-tree pathspecs CWD-aware via _get_effective_subfolder(), but git cat-file blob <ref>:<path> (line 311) and git show <ref>:<path> (line 485) still use the CWD-relative file_path from ls-tree output. These git commands require tree-root-relative paths, not CWD-relative paths. When CWD is inside the docs subfolder, ls-tree returns e.g. commands/export.md, but cat-file blob origin/main:commands/export.md fails because the tree path is docs/commands/export.md. The content-reading step silently fails (returncode != 0 is caught and skipped), producing empty hash dicts / content lists — the same behavior as the old code. The fix is incomplete: file discovery now works correctly, but content retrieval does not.
    Remediation: Prefix ./ to the path in cat-file and git show calls (e.g., f"{ref}:./{file_path}") which git resolves relative to CWD, or reconstruct the tree-root-relative path by re-adding the subfolder prefix for these commands.

Low

  • [test-inadequate] tests/test_doc_index.py:348test_handles_docs_subfolder_from_inside mocks subprocess.run (used for cat-file) to return success for any input, masking the path mismatch between ls-tree output (CWD-relative) and cat-file input (tree-root-relative). The test validates pathspec construction but cannot catch the content-reading failure described above.

Labels: PR fixes a bug in Python source code (CWD-relative pathspec handling in doc_index.py)


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the bug Something isn't working label Aug 17, 2026
git cat-file and git show require tree-root-relative paths, but after
the ls-tree pathspec fix, file paths are CWD-relative. Using the ./
prefix (e.g., origin/main:./commands/export.md) tells git to resolve
relative to CWD, matching the ls-tree output.

Without this, content retrieval silently failed (returncode != 0),
producing empty hash dicts and content lists — same broken behavior
as before the pathspec fix.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 17, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:42 AM UTC · Completed 8:55 AM UTC

Commit: d93db96 · View workflow run →

@fullsend-ai-review
fullsend-ai-review Bot dismissed their stale review August 17, 2026 08:55

Superseded by updated review

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Aug 17, 2026
Replace the Path.exists() check in _get_effective_subfolder() with
git rev-parse --show-prefix, which reliably reports CWD's position
within the repo. Avoids false results if the subfolder contains a
same-named sub-directory (e.g., docs/docs/).

Update tests to mock _get_effective_subfolder directly instead of
relying on filesystem state.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 17, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:06 AM UTC · Completed 9:19 AM UTC

Commit: d5bcc2a · View workflow run →

Comment thread src/doc_index.py
result = run_command_safe(["git", "rev-parse", "--show-prefix"], check=False)
if result.returncode == 0:
prefix = result.stdout.strip().rstrip("/")
if prefix == docs_subfolder:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] edge-case

_get_effective_subfolder() uses exact equality (prefix == docs_subfolder) to detect when CWD is inside the docs subfolder. If CWD were deeper than the subfolder root (e.g., docs/commands/ while DOCS_SUBFOLDER is docs), the prefix would not match and the function would return the full subfolder, producing a double-prefixed pathspec. In current code, setup_docs_environment() only does os.chdir(docs_subfolder) (never deeper), so this edge case is not triggered today.

Suggested fix: Consider adding a comment documenting the assumption that CWD is exactly the subfolder root, never deeper.

@csoceanu
csoceanu merged commit 0ff78bf into redhat-community-ai-tools:main Aug 17, 2026
22 checks passed
@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 17, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 9:36 AM UTC · Completed 9:56 AM UTC

Commit: d5bcc2a · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #75 — fix: account for CWD when building git ls-tree pathspecs

What happened

PR #75 was a human-authored bugfix (co-authored with Claude Opus 4.6) that fixed CWD-relative pathspec construction in doc_index.py. When running in same-repo mode, setup_docs_environment() changes CWD into the docs subfolder, causing git ls-tree pathspecs to be double-prefixed and return empty results. The review agent ran 3 iterations across 3 commits over 75 minutes:

  1. First review (run 32009900624, commit 4d1004b, 17 min): The correctness sub-agent found a genuine medium-severity bug — the fix addressed git ls-tree but missed that git cat-file and git show also needed CWD-relative path handling. Verdict: REQUEST_CHANGES.
  2. Second review (run 32011510035, commit d93db96, 14 min): After the author fixed cat-file/show paths (3-minute turnaround), re-review found only 3 low-severity findings. Verdict: APPROVE.
  3. Third review (run 32013487614, commit d5bcc2a, 14 min): After the author replaced a filesystem heuristic with git rev-parse --show-prefix, re-review found 1 low edge-case finding. Verdict: APPROVE.

The PR was merged by the author after agent-only review (no human reviewers participated).

What went well

  • High-value catch: The correctness sub-agent identified an incomplete fix (medium severity) that the human + AI authoring pair missed — a classic "did you check all call sites?" finding that demonstrates genuine review value.
  • Effective challenger filtering: The intent-coherence sub-agent flagged "missing authorization: no linked issue" as High severity on runs 2 and 3. The challenger correctly debunked it both times, recognizing this isn't a repo policy.
  • Self-trigger prevention: The review agent's own review submissions triggered pull_request_review workflow events (runs 32011238648, 32012680204, 32014653793), but routing correctly skipped all of them.
  • Fast iteration cycle: 3 commits addressing review feedback within 75 minutes total, with the author fixing the medium finding in just 3 minutes.

Known inefficiencies (all tracked by existing issues)

  • Re-review dispatched all dimensions: All 3 reviews dispatched the same 4 sub-agents (correctness, intent-coherence, style-conventions, docs-currency) + challenger. Commits 2 and 3 were targeted fixes that only needed correctness verification. Related: agents#343, agents#623.
  • PRIOR_REVIEW_PROVENANCE=none on re-reviews: Run 2 had PRIOR_REVIEW_PROVENANCE=none despite a prior REQUEST_CHANGES review existing, so it treated itself as a first review. This caused the intent-coherence sub-agent to re-flag the same false positive the challenger already debunked. Related: agents#203, agents#685.
  • 422 on inline comments: Run 1's inline comments were rejected by GitHub API (422). The harness recovered gracefully by retrying without inline placement. Related: agents#193, agents#699, agents#760.

No new proposals

This workflow executed well. The review agent provided genuine value by catching an incomplete fix before merge. All identified inefficiencies are tracked by existing open issues in the agents repo. No novel proposals are warranted.

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

Labels

bug Something isn't working ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant