Feature/index push pr by csoceanu · Pull Request #20 · redhat-community-ai-tools/code-to-docs

csoceanu · 2026-06-22T16:22:05Z

Summary

Replaces the direct-to-main push of documentation indexes with a PR-based workflow. This prevents bypassing branch protection rules on repos that require PR reviews before merging to main.

What changed

commit_indexes_to_repo() in doc_index.py previously switched to the base branch, committed indexes, and pushed directly to main. This involved complex branch-switching, stash, and temp-dir logic that was fragile and bypassed branch protection.

Now it:

Creates/checks out a persistent branch code-to-docs/update-indexes from the latest base
Commits the updated indexes
Pushes with --force-with-lease (safe for concurrent runs)
Creates a PR if none exists, or updates the existing one

Design decisions

Single persistent branch: all index updates go to code-to-docs/update-indexes. Only one open PR at a time — subsequent runs update it.
--force-with-lease: handles concurrent runs safely. If two PRs trigger index updates simultaneously, the last to push wins, the other fails gracefully.
Always returns to original branch: the action restores the working branch after pushing, so the rest of the pipeline is unaffected.

Test plan

Unit tests pass (239 total)
Tested commit_indexes_to_repo with mocked subprocess calls
Verified PR creation when no existing PR exists
Verified existing PR is reused when one is already open

Feature/style config

Replace direct-to-main push with PR-based workflow. Uses a persistent branch 'code-to-docs/update-indexes' and creates/updates a single PR. Concurrent runs are handled safely with --force-with-lease. Removes the complex branch-switching/stash/cherry-pick logic in favor of a simpler flow: backup index files, checkout index branch from base, restore files, commit, push, create/update PR, return to original branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

fullsend-ai-review · 2026-06-22T16:24:16Z

🤖 Finished Review · ✅ Success · Started 4:24 PM UTC · Completed 4:45 PM UTC
Commit: 6da2ced · View workflow run →

fullsend-ai-review · 2026-06-22T16:45:02Z

Review

Findings

Medium

[error-handling] src/doc_index.py — In the new code, git stash pop in the finally block (called with check=False) can fail silently if the stash conflicts with files on the restored branch. While the stash is not lost (it remains in git stash list), user work could be stranded without any warning. This primarily affects local development runs with a dirty working tree — CI environments typically have clean trees.
Remediation: Check the return code of git stash pop. If it fails, leave the stash intact and print a warning that includes the stash ref so the user can manually recover.

Low

[missing-test] src/doc_index.py — The commit_indexes_to_repo function has no test coverage, neither before nor after this change. The function now has significantly different behavior (creating PRs via gh, force-pushing to a shared branch, branch switching with stash/restore) and none of it is tested. This is a pre-existing gap, not a regression.
Remediation: Add tests covering: (1) GH_TOKEN not set returns False, (2) no changes returns False, (3) successful PR creation path (mocking subprocess), (4) existing PR update path.
[edge-case] src/doc_index.py — Uses a single hardcoded branch name INDEX_BRANCH = 'code-to-docs/update-indexes' for all operations. If two CI runs execute concurrently, the second --force-with-lease push will fail safely (no data corruption), but the failure is caught by the outer except CalledProcessError and returns False with a warning.
[api-contract] src/doc_index.py — The function now requires GH_TOKEN to be set, returning False with a print warning when absent. The caller in discovery.py does not check the return value. In production (GitHub Actions), GH_TOKEN is always set via action.yml. Local invocations that previously succeeded will now silently skip index commits.
[data-exposure] src/security_utils.py — run_command_safe() only sanitizes output on the error path. On the success path, raw stdout/stderr is returned unsanitized. Pre-existing issue, not practically exploitable by this PR's changes since the new code's success-path outputs (branch names, staged file lists, PR JSON) do not contain tokens.
[injection-vuln] src/doc_index.py — DOCS_BASE_BRANCH from environment is used unsanitized in git subprocess list calls. While list-based subprocess.run prevents shell injection, a value starting with -- could theoretically be interpreted as a git flag. Risk is minimal since this input is controlled by the repository owner via action.yml.

Previous run

Review

Findings

High

[stale-reference] README.md:211 — Documentation states "Indexes are automatically committed to your main branch" but this PR changes the implementation to use a PR-based workflow instead of direct push. Users reading the docs would have incorrect expectations about how indexes are persisted.
Remediation: Update to reflect PR-based workflow (e.g., "Indexes are automatically committed via a PR to your main branch").

Medium

[logic-error] src/doc_index.py:686 — The old code had merge-base checking and selective summary push logic to prevent older branches from overwriting newer indexes on main. The new code removes this entirely, always creating INDEX_BRANCH from origin/{base_branch} and copying the full .doc-index directory. In concurrent CI runs, last writer wins. The PR-based workflow partially mitigates this (human review gate), but the behavioral change is notable.
Remediation: Consider whether the PR-based workflow inherently mitigates this, or re-add concurrency protection.
[scope-creep] src/doc_index.py:645 — PR titled "Feature/index push pr" frames this as preventing branch protection bypass, but also removes the sophisticated branch-currency logic and selective summary pushing. The scope includes both the compliance fix and simplification of push logic. Removing the selective push logic may be a natural consequence of the PR-based approach, but the combined scope should be explicitly acknowledged.
Remediation: Clarify whether this is a compliance fix or an architectural refactor, and document the intentional removal of selective push logic.
[missing-doc] README.md:148 — GH_PAT documentation does not mention the token is now also used for creating index update PRs via the gh CLI (gh pr list, gh pr create).
Remediation: Update GH_PAT description to mention index PR creation usage.

Low

[stale-reference] src/doc_index.py:1386 — The function get_safe_summaries_to_push becomes dead code after this PR (its only call site is removed). Housekeeping issue.
[logic-error] src/doc_index.py:700 — git stash --include-untracked is called before the try block. If git stash saved nothing (clean working tree), git stash pop in finally will pop a previous stash entry if one exists. Pre-existing pattern from old code.
[error-handling] src/doc_index.py:693 — temp_dir is created before the inner try/finally block; if shutil.copytree fails before entering try, the temp directory leaks. Pre-existing pattern, minimal impact in CI.
[command-injection] src/doc_index.py:694 — DOCS_BASE_BRANCH env var used unsanitized in git commands. While list-based subprocess prevents shell injection, a value starting with - could be interpreted as a git flag. Pre-existing issue; env var is set by repo admin, not untrusted input.
[missing-authorization] src/doc_index.py — This PR introduces an architectural change without a linked issue. The PR description explains the rationale clearly, but linking an issue would improve traceability.
[naming-convention] src/doc_index.py:645 — INDEX_BRANCH constant defined just before its consumer function instead of with other module constants at file top (lines 39-45).

Info

[privilege-reduction] src/doc_index.py:645 — Positive security change: old code pushed directly to base branch bypassing branch protection, new code pushes to dedicated branch and creates PR. Use of --force-with-lease is appropriate. GH_TOKEN check is fail-closed.

fullsend-ai-review

See the review comment for full details.

fullsend-ai-review · 2026-06-22T16:45:03Z

@@ -689,157 +686,92 @@ def commit_indexes_to_repo(content_type="indexes"):
                print(f"No {content_type} changes to commit")


[medium] logic-error

The old code had merge-base checking and selective summary push logic to prevent older branches from overwriting newer indexes. The new code removes this entirely. In concurrent CI runs, last writer wins. The PR-based workflow partially mitigates this with a human review gate.

fullsend-ai-review · 2026-06-22T16:45:03Z

    return updated_folders


+INDEX_BRANCH = "code-to-docs/update-indexes"


[medium] scope-creep

PR frames this as preventing branch protection bypass but also removes sophisticated branch-currency logic and selective summary pushing. The combined scope should be explicitly acknowledged.

fullsend-ai-review · 2026-06-22T16:45:03Z

+                check=True
            )
+            current_branch = current_branch_result.stdout.strip()



[low] logic-error

git stash --include-untracked called before try block. If stash saved nothing, git stash pop in finally will pop a previous stash entry. Pre-existing pattern from old code.

fullsend-ai-review · 2026-06-22T16:45:03Z

+            gh_token = os.environ.get("GH_TOKEN")
+            if not gh_token:
+                print("Warning: GH_TOKEN not set, cannot create index PR")
+                return False


[low] error-handling

temp_dir created before inner try/finally block; if shutil.copytree fails before entering try, the temp directory leaks. Pre-existing pattern, minimal impact in CI.

fullsend-ai-review · 2026-06-22T16:45:03Z

+            if not gh_token:
+                print("Warning: GH_TOKEN not set, cannot create index PR")
+                return False



[low] command-injection

DOCS_BASE_BRANCH env var used unsanitized in git commands. Pre-existing issue, env var set by repo admin.

fullsend-ai-review · 2026-06-22T16:45:03Z

    return updated_folders


+INDEX_BRANCH = "code-to-docs/update-indexes"


[low] naming-convention

INDEX_BRANCH constant defined near its consumer function instead of with other module constants at file top.

- README: clarify indexes are submitted via PR, not pushed directly - README: update GH_PAT description to mention index PR creation - Move INDEX_BRANCH constant to module-level with other constants - Remove dead code: doc_matches_main, load_manifest_from_main, get_safe_summaries_to_push (no longer needed with PR-based flow) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

fullsend-ai-review · 2026-06-22T17:55:30Z

🤖 Finished Review · ✅ Success · Started 5:55 PM UTC · Completed 6:07 PM UTC
Commit: 6da2ced · View workflow run →

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

fullsend-ai-review · 2026-06-22T18:18:50Z

🤖 Finished Review · ✅ Success · Started 6:18 PM UTC · Completed 6:30 PM UTC
Commit: 6da2ced · View workflow run →

fullsend-ai-review · 2026-06-22T18:30:49Z

Review skipped — this PR is already merged.

The /fs-review command only reviews open pull requests.

_{Posted by fullsend post-review check}

csoceanu and others added 3 commits June 22, 2026 17:43

Merge pull request #5 from csoceanu/feature/style-config

04c978c

Feature/style config

Merge pull request #6 from csoceanu/feature/style-config

5fe7ce1

Feature/style config

fullsend-ai-review Bot suggested changes Jun 22, 2026

View reviewed changes

fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 22, 2026

fix: warn when git stash pop fails during index commit

3cdc75b

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

csoceanu merged commit 35e8060 into redhat-community-ai-tools:main Jun 22, 2026
8 checks passed

		@@ -689,157 +686,92 @@ def commit_indexes_to_repo(content_type="indexes"):
		print(f"No {content_type} changes to commit")

		return updated_folders


		INDEX_BRANCH = "code-to-docs/update-indexes"

Uh oh!

Conversation

csoceanu commented Jun 22, 2026

Summary

What changed

Design decisions

Test plan

Uh oh!

fullsend-ai-review Bot commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

Medium

Low

Review

Findings

High

Medium

Low

Info

Uh oh!

fullsend-ai-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jun 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fullsend-ai-review Bot commented Jun 22, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 22, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 22, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 22, 2026 •

edited

Loading