Skip to content

fix(brownfield): scan repos with any origin remote#486

Open
andrew-adamson wants to merge 1 commit intoQ00:mainfrom
andrew-adamson:fix/brownfield-origin-scan
Open

fix(brownfield): scan repos with any origin remote#486
andrew-adamson wants to merge 1 commit intoQ00:mainfrom
andrew-adamson:fix/brownfield-origin-scan

Conversation

@andrew-adamson
Copy link
Copy Markdown

Summary

  • broaden brownfield repository scanning so repos with any configured origin remote are considered
  • align setup and brownfield MCP handling with the updated scan behavior
  • add focused coverage for the scan and handler paths

Testing

  • uv run pytest tests/unit/bigbang/test_brownfield.py tests/unit/mcp/tools/test_brownfield_handler.py

Copy link
Copy Markdown
Contributor

@ouroboros-agent ouroboros-agent Bot left a comment

Choose a reason for hiding this comment

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

Review — ouroboros-agent[bot]

Verdict: REQUEST_CHANGES

Reviewing commit ed34eaf for PR #486

Review record: 952b8291-886c-4fcd-b158-cf0c1e1456a5

Blocking Findings

| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.

---|-----------|----------|---------|
| 1 | src/ouroboros/mcp/tools/brownfield_handler.py:6 | BLOCKING | The new MCP contract says scan discovers repos/worktrees, but scan_home_for_repos() still only recognizes directories that literally contain a .git entry (if ".git" in dirnames at src/ouroboros/bigbang/brownfield.py). Git worktrees use a .git file, not a .git directory, so managed worktrees under ~/.ouroboros/worktrees will be silently skipped even though the API now advertises support for them. This is a user-visible behavior bug, not just a doc mismatch. |
| 2 | src/ouroboros/bigbang/brownfield.py:100 | BLOCKING | _has_github_origin() now returns True for any non-empty origin URL, including GitLab/Azure, and the updated tests explicitly lock that in. Keeping the old name while changing the semantics is misleading enough to cause future callers to get the wrong result. If backward compatibility is required, the old helper should preserve GitHub-only behavior and callers should migrate to _has_origin_remote() instead of reusing a now-misnamed predicate. |

Non-blocking Suggestions

None.

Design Notes

The direction is reasonable: separating “has any origin remote” from “scan and register” makes the scan policy more flexible. The PR needs the implementation and compatibility surface to match that new policy precisely; right now the advertised worktree support and the legacy helper name both drift from the actual behavior.


Reviewed by ouroboros-agent[bot] via Codex deep analysis

@shaun0927
Copy link
Copy Markdown
Collaborator

I understand the motivation here. Broadening brownfield scan from "GitHub origin only" to "any repo with an origin remote" is a reasonable direction for local-first brownfield UX.

That said, I don't think this is merge-ready yet because the contract is still blurry in two places:

  1. _has_github_origin() no longer matches its own name/meaning.
    If the new policy is "any configured origin remote," then please keep the legacy helper GitHub-specific and migrate callers to a new _has_origin_remote() predicate instead of changing the semantics under the old name.

  2. The MCP/tool copy now says scan discovers repos/worktrees, but the actual scan path still centers on directories that literally contain a .git entry. If we want to claim worktree support here, the implementation/tests need to cover that explicitly; otherwise the contract text should stay narrower.

The direction is fine. I just want the naming, docs, and real behavior to line up cleanly.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants