fix: normalize DOCS_SUBFOLDER trailing slash before CWD comparison

[csoceanu]

Summary

• DOCS_SUBFOLDER set with a trailing slash (e.g. docs/) caused _get_effective_subfolder() to fail its CWD comparison: git rev-parse --show-prefix returns docs/ which gets rstrip("/") to docs, but the env var was compared as-is — "docs" != "docs/"
• The mismatch kept the subfolder prefix in pathspecs, causing the double-prefix bug (docs/docs/...) that made all ref-based reads return empty
• Fix: strip trailing slash from DOCS_SUBFOLDER at the start of _get_effective_subfolder()

Confirmed by debug logging added in #76:

CWD prefix 'docs' != DOCS_SUBFOLDER '***', keeping prefix

Test plan

• 430 tests pass
• Ruff check + format clean
• New TestGetEffectiveSubfolder class with 4 tests:
  • Trailing slash normalized and matches CWD
  • No trailing slash matches normally
  • CWD not in subfolder returns subfolder value
  • Empty subfolder returns empty string

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:02 AM UTC · Completed 7:14 AM UTC

Commit: 84b7d24 · View workflow run →

[fullsend-ai-review]

Review

Findings

Low

• [edge-case] src/doc_index.py:243 — When DOCS_SUBFOLDER is set to "." or "./", os.path.normpath(raw).strip("/") produces ".", which is truthy and passes the emptiness guard. The function then compares "." against the git rev-parse --show-prefix output (empty string at repo root), returning "." as the effective subfolder. Downstream pathspecs like "./commands" work correctly in git but the behavior is inconsistent with intent.
  Remediation: Add a guard after normpath: if docs_subfolder == ".": docs_subfolder = ""

• [defense-in-depth-gap] src/doc_index.py — Several direct reads of os.environ.get("DOCS_SUBFOLDER") outside _get_effective_subfolder() (in commit_indexes_to_repo, fetch_indexes_from_main, get_docs_root, checkout_docs_from_base_branch) still receive the raw env var value. When code runs outside Docker, the entrypoint normalization is bypassed. Practical impact is limited since Path("docs/") resolves identically to Path("docs") for filesystem operations.

• [missing-authorization] — Bug fix lacks a formally linked issue. The PR body and title clearly describe the bug and fix, and the actual logic change is ~4 lines (the rest is tests), making the PR effectively self-documenting. Consider creating a tracking issue for process completeness.

---

Labels: PR fixes a bug in Python path normalization logic within the GitHub Action entrypoint

Previous run

Review

Reason: stale-head

The review agent reviewed commit 84b7d24b9f82987203d2be32509b960f8ce1d29d but the PR HEAD is now dca4aaf312247786278a42f4b1977c7e195589f4. This review was discarded to avoid approving unreviewed code.

[fullsend-ai-review]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:15 AM UTC · Completed 7:27 AM UTC

Commit: dca4aaf · View workflow run →

[fullsend-ai-review]

[low] edge-case

When DOCS_SUBFOLDER is set to "." or "./", os.path.normpath(raw).strip("/") produces ".", which is truthy and passes the emptiness guard. The function compares "." against git rev-parse --show-prefix output (empty at repo root), returning "." as effective subfolder. Downstream pathspecs like "./commands" work in git but behavior is inconsistent with intent.

Suggested fix: Add a guard after normpath: if docs_subfolder == ".": docs_subfolder = ""

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 7:35 AM UTC · Completed 7:47 AM UTC

Commit: dca4aaf · View workflow run →

[fullsend-ai-retro]

Retro: PR #79 — fix: normalize DOCS_SUBFOLDER trailing slash

Verdict: Workflow went well. No new proposals — all improvement areas are already tracked by existing issues.

Timeline

Time (UTC)	Event
07:00:57	PR opened by csoceanu with commit 84b7d24
07:01:01	Review run 1 triggered (run 32109457135)
07:10:29	Author proactively pushes commit dca4aaf (before review 1 completes)
07:10:35	Review run 2 triggered by synchronize event (run 32110198819), queued behind run 1
07:14:07	Run 1 completes, detects stale HEAD, wraps findings in "Previous run" details block
07:14:09	Stale-head redispatch: /fs-review posted (redundant — run 2 already triggered)
07:14:33	Run 2's Review job starts (queued ~4 min at dispatch level)
07:27:34	Run 2 completes — APPROVED with 1 low-severity inline comment
07:34:45	PR merged by author

What went well

• Review quality was strong. The edge-case finding about DOCS_SUBFOLDER="." producing a truthy "." after normpath was legitimate and actionable. The defense-in-depth observation about raw os.environ.get() calls outside the normalized function was accurate. The approval was correct for a well-tested 4-line logic fix.
• Stale-head detection worked correctly. When the PR HEAD moved during review, the post-review script caught the mismatch and wrapped stale findings appropriately rather than approving unreviewed code.
• The PR itself was high quality. Clear commit messages, comprehensive tests (7 test methods including a regression test for pathspec doubling), and a well-structured description linking to the debug logging that diagnosed the bug.

Observations (evidence for existing issues)

1. ~13 min of wasted review compute from stale-head race. Run 1 reviewed commit 84b7d24 to completion even though the HEAD moved to dca4aaf 4 minutes before it finished. Root cause: the dispatch-level concurrency group uses cancel-in-progress: false, which queued Run 2 behind Run 1 instead of cancelling it. The stage-level cancel-in-progress: true on the Review job never fired because Run 1 had already completed by the time Run 2 reached the review stage. This provides concrete evidence for fullsend-ai/fullsend#1331 (reduce wasted review runs from stale-head races).

2. Stale-head redispatch was redundant. The /fs-review comment posted at 07:14:09 was unnecessary — the synchronize event had already triggered Run 2 at 07:10:35. The issue_comment runs it spawned (32110472563, 32110474861) were properly skipped, so no harm done. This supports fullsend-ai/fullsend#2399 (remove stale-head re-dispatch: redundant with concurrency group).

3. Nuance on #2399: The concurrency group does NOT currently cancel stale runs as #2399 assumes — cancel-in-progress: false at the dispatch level prevents it. The stale-head redispatch is therefore not fully redundant today; it serves as a safety net. Fixing #1331 (enabling cancellation) is a prerequisite for safely removing the redispatch per #2399.