fix(pipeline-status): parse_remote_url breaks with git insteadOf userinfo (#55)
* fix(pipeline-status): handle git insteadOf userinfo in parse_remote_url * test(pipeline-status): add tests for URL with userinfo * docs(handoff): set PR number 55 in handoff + ADR-023 --------- Co-authored-by: opencode-agent <agent@slaid098.dev>
This commit is contained in:
parent
02f0863a24
commit
e413a6c132
6 changed files with 202 additions and 2 deletions
|
|
@ -168,7 +168,11 @@ def parse_remote_url(url: str) -> tuple[str, str, str]:
|
||||||
ssh_match = re.match(r"git@([^:]+):([^/]+)/(.+?)(?:\.git)?$", url)
|
ssh_match = re.match(r"git@([^:]+):([^/]+)/(.+?)(?:\.git)?$", url)
|
||||||
if ssh_match:
|
if ssh_match:
|
||||||
return ssh_match.group(1), ssh_match.group(2), ssh_match.group(3)
|
return ssh_match.group(1), ssh_match.group(2), ssh_match.group(3)
|
||||||
https_match = re.match(r"https?://([^/]+)/([^/]+)/(.+?)(?:\.git)?$", url)
|
# `(?:[^/@]*@)?` optionally skips `user:password@` userinfo before host.
|
||||||
|
# Needed because `git config url.insteadOf` rewrites `https://github.com/`
|
||||||
|
# to `https://x-access-token:TOKEN@github.com/`, and `git remote get-url
|
||||||
|
# origin` returns the rewritten URL (see ADR-022 / ADR-023).
|
||||||
|
https_match = re.match(r"https?://(?:[^/@]*@)?([^/]+)/([^/]+)/(.+?)(?:\.git)?$", url)
|
||||||
if https_match:
|
if https_match:
|
||||||
return https_match.group(1), https_match.group(2), https_match.group(3)
|
return https_match.group(1), https_match.group(2), https_match.group(3)
|
||||||
raise ValueError(f"Cannot parse remote URL: {url}")
|
raise ValueError(f"Cannot parse remote URL: {url}")
|
||||||
|
|
|
||||||
|
|
@ -126,7 +126,11 @@ def parse_remote_url(url: str) -> tuple[str, str, str]:
|
||||||
ssh_match = re.match(r"git@([^:]+):([^/]+)/(.+?)(?:\.git)?$", url)
|
ssh_match = re.match(r"git@([^:]+):([^/]+)/(.+?)(?:\.git)?$", url)
|
||||||
if ssh_match:
|
if ssh_match:
|
||||||
return ssh_match.group(1), ssh_match.group(2), ssh_match.group(3)
|
return ssh_match.group(1), ssh_match.group(2), ssh_match.group(3)
|
||||||
https_match = re.match(r"https?://([^/]+)/([^/]+)/(.+?)(?:\.git)?$", url)
|
# `(?:[^/@]*@)?` optionally skips `user:password@` userinfo before host.
|
||||||
|
# Needed because `git config url.insteadOf` rewrites `https://github.com/`
|
||||||
|
# to `https://x-access-token:TOKEN@github.com/`, and `git remote get-url
|
||||||
|
# origin` returns the rewritten URL (see ADR-022 / ADR-023).
|
||||||
|
https_match = re.match(r"https?://(?:[^/@]*@)?([^/]+)/([^/]+)/(.+?)(?:\.git)?$", url)
|
||||||
if https_match:
|
if https_match:
|
||||||
return https_match.group(1), https_match.group(2), https_match.group(3)
|
return https_match.group(1), https_match.group(2), https_match.group(3)
|
||||||
raise ValueError(f"Cannot parse remote URL: {url}")
|
raise ValueError(f"Cannot parse remote URL: {url}")
|
||||||
|
|
|
||||||
39
docs/decisions/023-pr-55-parse-remote-url.md
Normal file
39
docs/decisions/023-pr-55-parse-remote-url.md
Normal file
|
|
@ -0,0 +1,39 @@
|
||||||
|
# ADR-023: parse_remote_url strips git insteadOf userinfo from HTTPS URLs
|
||||||
|
|
||||||
|
## Статус
|
||||||
|
Accepted (2026-07-24)
|
||||||
|
|
||||||
|
## Контекст
|
||||||
|
PR#53 (ADR-022) добавил `git config --global url."https://x-access-token:${GITHUB_TOKEN}@github.com/".insteadOf https://github.com/` в `setup-memory.sh` для non-interactive HTTPS auth в Docker. После этого `git remote get-url origin` возвращает rewritten URL с встроенным userinfo:
|
||||||
|
|
||||||
|
```
|
||||||
|
https://x-access-token:github_pat_...@github.com/slaid098/opencode-config.git
|
||||||
|
```
|
||||||
|
|
||||||
|
`parse_remote_url` (в `pipeline-status.py` и `spec-status.py`) использовал regex `https?://([^/]+)/([^/]+)/(.+?)(?:\.git)?$` — группа `[^/]+` жадно матчит всё между `://` и первым `/`, включая `user:password@` (символ `@` не исключён). В результате `host` = `x-access-token:github_pat_...@github.com` вместо `github.com`.
|
||||||
|
|
||||||
|
Это ломало `get_memory_file_path()` → путь `repos/x-access-token:...@github.com/slaid098/opencode-config.md` (не существует) → `check_memory` возвращал NOT_DONE для каждого PR после PR#53. `get_repo_full_name()` случайно не ломался (возвращает `org/repo`, host отбрасывается), но фикс применён в обоих файлах для консистентности.
|
||||||
|
|
||||||
|
CI на PR#53 не поймал regression — тесты `test_parse_remote_url_*` покрывали только plain URLs без userinfo.
|
||||||
|
|
||||||
|
## Решение
|
||||||
|
Изменить HTTPS-regex в `parse_remote_url` (оба файла) на:
|
||||||
|
|
||||||
|
```python
|
||||||
|
re.match(r"https?://(?:[^/@]*@)?([^/]+)/([^/]+)/(.+?)(?:\.git)?$", url)
|
||||||
|
```
|
||||||
|
|
||||||
|
`(?:[^/@]*@)?` — опциональная non-capturing группа, пропускающая `user:password@` userinfo перед host:
|
||||||
|
- Для plain `https://github.com/...` группа не матчится (нет `@`) → host = `github.com`.
|
||||||
|
- Для `https://user:token@github.com/...` группа матчит `user:token@` и отбрасывается → host = `github.com`.
|
||||||
|
- `[^/@]` внутри userinfo не матчит ни `/` ни `@`, поэтому корректно останавливается на первом `@` (поддерживает `user:pass`, `user`, `token@` формы).
|
||||||
|
|
||||||
|
7 новых тестов: 4 в `test_pipeline_status.py` (parse + get_memory_file_path), 3 в `test_spec_status.py` (parse + get_repo_full_name).
|
||||||
|
|
||||||
|
## Альтернативы
|
||||||
|
1. **Использовать `git config --get remote.origin.url`** вместо `git remote get-url origin` — bypasses insteadOf, возвращает raw URL. Отклонено: coupling с git internals, ломается если кто-то задаст remote URL с token напрямую (не через insteadOf).
|
||||||
|
2. **Strip userinfo через `url.split("@")[-1]`** — хрупкий: ломается если path содержит `@` (legitimate в GitHub URLs для определённых refs/paths).
|
||||||
|
3. **Рефакторинг `parse_remote_url` в shared module** — оба файла дублируют функцию. Отклонено для этого PR: риск для ADR-010 cwd-aware `_resolve_repo_root` logic, выходит за рамки bugfix.
|
||||||
|
4. **Не чинить, отключить insteadOf** — отклонено: insteadOf нужен для non-interactive memory clone (PR#53/ADR-022), regression в pipeline_status — меньшее из зол.
|
||||||
|
|
||||||
|
Regex fix (выбран) — минимальный, root-cause, без env coupling, покрывает оба caller'а.
|
||||||
39
docs/handoff/pr-55-parse-remote-url.md
Normal file
39
docs/handoff/pr-55-parse-remote-url.md
Normal file
|
|
@ -0,0 +1,39 @@
|
||||||
|
---
|
||||||
|
pr: 55
|
||||||
|
title: fix parse_remote_url breaking with git insteadOf userinfo
|
||||||
|
---
|
||||||
|
|
||||||
|
# PR 55: fix parse_remote_url breaking with git insteadOf userinfo
|
||||||
|
|
||||||
|
## Что сделано
|
||||||
|
- `.opencode/scripts/pipeline-status.py:171` — в `parse_remote_url` HTTPS-regex изменён с `r"https?://([^/]+)/([^/]+)/(.+?)(?:\.git)?$"` на `r"https?://(?:[^/@]*@)?([^/]+)/([^/]+)/(.+?)(?:\.git)?$"`. Опциональная non-capturing группа `(?:[^/@]*@)?` пропускает `user:password@` userinfo перед host, не ломая plain-URL случай (группа опциональна).
|
||||||
|
- `.opencode/scripts/spec-status.py:129` — тот же regex fix (функция `parse_remote_url` дублирована в spec-status.py, используется в Phase 8 `gh issue view --repo`).
|
||||||
|
- `tests/test_pipeline_status.py` — 4 новых теста:
|
||||||
|
- `test_parse_remote_url_https_with_userinfo` — URL `https://x-access-token:github_pat_TOKEN@github.com/slaid098/opencode-config.git` → `("github.com", "slaid098", "opencode-config")`.
|
||||||
|
- `test_parse_remote_url_https_without_userinfo` — plain URL работает (regression guard).
|
||||||
|
- `test_parse_remote_url_https_userinfo_no_git_suffix` — userinfo + нет `.git` suffix.
|
||||||
|
- `test_get_memory_file_path_with_userinfo` — `get_memory_file_path()` строит `repos/github.com/slaid098/opencode-config.md` (без `x-access-token`/`github_pat_TOKEN` в пути).
|
||||||
|
- `tests/test_spec_status.py` — 3 новых теста:
|
||||||
|
- `test_parse_remote_url_https_with_userinfo` — same URL → correct tuple.
|
||||||
|
- `test_parse_remote_url_https_without_userinfo` — regression guard.
|
||||||
|
- `test_get_repo_full_name_with_userinfo` — `get_repo_full_name()` → `slaid098/opencode-config` (не `x-access-token:...@github.com/...`).
|
||||||
|
- ADR-023 + этот handoff.
|
||||||
|
|
||||||
|
## Почему
|
||||||
|
PR#53 (ADR-022) добавил `git config --global url.insteadOf` в `setup-memory.sh` для non-interactive HTTPS auth при клонировании memory repo в Docker. После этого `git remote get-url origin` возвращает rewritten URL с встроенным userinfo: `https://x-access-token:TOKEN@github.com/slaid098/opencode-config.git`. Старый regex `[^/]+` жадно матчит `x-access-token:TOKEN@github.com` как host (символ `@` не входит в исключения `[^/]`), поэтому:
|
||||||
|
- `get_memory_file_path()` строил путь `repos/x-access-token:TOKEN@github.com/slaid098/opencode-config.md` (не существует) → `check_memory` в pipeline-status возвращал ❌ "memory file не существует" для ВСЕХ будущих PR.
|
||||||
|
- `get_repo_full_name()` в spec-status случайно работал (возвращает `org/repo`, host отбрасывается), но фикс всё равно применён для консистентности и на случай future callers.
|
||||||
|
|
||||||
|
Это латентная регрессия — влияет на каждый pipeline_status вызов после PR#53, пока insteadOf активен. Тесты `test_parse_remote_url_*` покрывали только plain URLs, поэтому CI на PR#53 не поймал regression.
|
||||||
|
|
||||||
|
Fix выбрал минимальный — regex-only, без env coupling (вариант "использовать `git config --get remote.origin.url` вместо `git remote get-url`" ломается если кто-то задаст remote URL с token напрямую; вариант `url.split('@')[-1]` хрупкий при `@` в path).
|
||||||
|
|
||||||
|
## Pending
|
||||||
|
— (нет)
|
||||||
|
|
||||||
|
## Watch out
|
||||||
|
- **`(?:[^/@]*@)?` опциональна** — для plain `https://github.com/...` группа не матчится (нет `@`), host = `github.com`. Для `https://user:token@github.com/...` группа матчит `user:token@` и отбрасывается (non-capturing), host = `github.com`. `[^/@]` внутри userinfo не матчит ни `/` ни `@`, поэтому корректно останавливается на первом `@`.
|
||||||
|
- **Дублирование `parse_remote_url`** — функция идентична в `pipeline-status.py` и `spec-status.py`. Fix применён в обоих. Рефакторинг в shared module выходит за рамки этого PR (риск для ADR-010 cwd-aware logic).
|
||||||
|
- **ADR number = 023** (sequential, следующий после 022), НЕ PR number.
|
||||||
|
- Существующие 362 теста не сломаны — полный suite: 369 passed (362 + 7 новых).
|
||||||
|
- **PR number в filename** — issue #54 → PR #55. Handoff/ADR первично scaffold'нуты как `pr-54-*` (по issue number), после `gh pr create` переименованы в `pr-55-*` (по PR number, по конвенции репо).
|
||||||
|
|
@ -141,6 +141,40 @@ def test_parse_remote_url_invalid():
|
||||||
ps.parse_remote_url("not-a-valid-url")
|
ps.parse_remote_url("not-a-valid-url")
|
||||||
|
|
||||||
|
|
||||||
|
def test_parse_remote_url_https_with_userinfo():
|
||||||
|
"""URL с git insteadOf userinfo (x-access-token:TOKEN@host) — host без userinfo.
|
||||||
|
|
||||||
|
Regression for PR#53/ADR-022: ``git config url.insteadOf`` rewrites
|
||||||
|
``https://github.com/`` to ``https://x-access-token:TOKEN@github.com/``,
|
||||||
|
so ``git remote get-url origin`` returns the rewritten URL. The old regex
|
||||||
|
``[^/]+`` greedily matched ``x-access-token:TOKEN@github.com`` as host,
|
||||||
|
breaking ``get_memory_file_path`` and ``check_memory``.
|
||||||
|
"""
|
||||||
|
assert ps.parse_remote_url(
|
||||||
|
"https://x-access-token:github_pat_TOKEN@github.com/slaid098/opencode-config.git"
|
||||||
|
) == ("github.com", "slaid098", "opencode-config")
|
||||||
|
|
||||||
|
|
||||||
|
def test_parse_remote_url_https_without_userinfo():
|
||||||
|
"""Обычный HTTPS URL без userinfo работает (regression guard).
|
||||||
|
|
||||||
|
The ``(?:[^/@]*@)?`` optional userinfo group must NOT break the plain-URL
|
||||||
|
case — ``host`` stays ``github.com`` with no userinfo to skip.
|
||||||
|
"""
|
||||||
|
assert ps.parse_remote_url("https://github.com/slaid098/opencode-config.git") == (
|
||||||
|
"github.com",
|
||||||
|
"slaid098",
|
||||||
|
"opencode-config",
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_parse_remote_url_https_userinfo_no_git_suffix():
|
||||||
|
"""URL с userinfo и без .git suffix — опциональный .git работает с userinfo."""
|
||||||
|
assert ps.parse_remote_url(
|
||||||
|
"https://x-access-token:TOKEN@github.com/slaid098/opencode-config"
|
||||||
|
) == ("github.com", "slaid098", "opencode-config")
|
||||||
|
|
||||||
|
|
||||||
# ── check_issue ──────────────────────────────────────────────────────────────
|
# ── check_issue ──────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -704,6 +738,37 @@ def test_get_memory_file_path_ssh(monkeypatch):
|
||||||
assert "slaid098" in str(path)
|
assert "slaid098" in str(path)
|
||||||
|
|
||||||
|
|
||||||
|
def test_get_memory_file_path_with_userinfo(monkeypatch):
|
||||||
|
"""get_memory_file_path строит правильный путь когда remote URL содержит userinfo.
|
||||||
|
|
||||||
|
Regression for PR#53/ADR-022: after ``git config url.insteadOf``, ``git
|
||||||
|
remote get-url origin`` returns
|
||||||
|
``https://x-access-token:TOKEN@github.com/slaid098/opencode-config.git``.
|
||||||
|
Without the regex fix, ``get_memory_file_path`` built
|
||||||
|
``repos/x-access-token:TOKEN@github.com/slaid098/opencode-config.md``
|
||||||
|
(nonexistent) → ``check_memory`` returned NOT_DONE. With the fix, the
|
||||||
|
path is the same as for the plain URL.
|
||||||
|
"""
|
||||||
|
monkeypatch.setattr(
|
||||||
|
ps,
|
||||||
|
"run_cmd",
|
||||||
|
mock_run_cmd(
|
||||||
|
{
|
||||||
|
("git", "remote"): (
|
||||||
|
0,
|
||||||
|
"https://x-access-token:github_pat_TOKEN@github.com/slaid098/opencode-config.git\n",
|
||||||
|
"",
|
||||||
|
),
|
||||||
|
}
|
||||||
|
),
|
||||||
|
)
|
||||||
|
path = ps.get_memory_file_path()
|
||||||
|
assert path == ps.MEMORY_DIR / "github.com" / "slaid098" / "opencode-config.md"
|
||||||
|
# userinfo must NOT leak into the path
|
||||||
|
assert "x-access-token" not in str(path)
|
||||||
|
assert "github_pat_TOKEN" not in str(path)
|
||||||
|
|
||||||
|
|
||||||
# ── get_repo_full_name ───────────────────────────────────────────────────────
|
# ── get_repo_full_name ───────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -152,6 +152,29 @@ def test_parse_remote_url_invalid():
|
||||||
ss.parse_remote_url("not-a-valid-url")
|
ss.parse_remote_url("not-a-valid-url")
|
||||||
|
|
||||||
|
|
||||||
|
def test_parse_remote_url_https_with_userinfo():
|
||||||
|
"""URL с git insteadOf userinfo (x-access-token:TOKEN@host) — host без userinfo.
|
||||||
|
|
||||||
|
Regression for PR#53/ADR-022: ``git config url.insteadOf`` rewrites
|
||||||
|
``https://github.com/`` to ``https://x-access-token:TOKEN@github.com/``,
|
||||||
|
so ``git remote get-url origin`` returns the rewritten URL. The old regex
|
||||||
|
greedily matched ``x-access-token:TOKEN@github.com`` as host, breaking
|
||||||
|
``get_repo_full_name`` (used in Phase 8 ``gh issue view --repo``).
|
||||||
|
"""
|
||||||
|
assert ss.parse_remote_url(
|
||||||
|
"https://x-access-token:github_pat_TOKEN@github.com/slaid098/opencode-config.git"
|
||||||
|
) == ("github.com", "slaid098", "opencode-config")
|
||||||
|
|
||||||
|
|
||||||
|
def test_parse_remote_url_https_without_userinfo():
|
||||||
|
"""Обычный HTTPS URL без userinfo работает (regression guard)."""
|
||||||
|
assert ss.parse_remote_url("https://github.com/slaid098/opencode-config.git") == (
|
||||||
|
"github.com",
|
||||||
|
"slaid098",
|
||||||
|
"opencode-config",
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
# ── get_repo_full_name ───────────────────────────────────────────────────────
|
# ── get_repo_full_name ───────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -195,6 +218,32 @@ def test_get_repo_full_name_remote_error(monkeypatch):
|
||||||
ss.get_repo_full_name.cache_clear()
|
ss.get_repo_full_name.cache_clear()
|
||||||
|
|
||||||
|
|
||||||
|
def test_get_repo_full_name_with_userinfo(monkeypatch):
|
||||||
|
"""get_repo_full_name работает когда remote URL содержит userinfo.
|
||||||
|
|
||||||
|
Regression for PR#53/ADR-022: ``git remote get-url origin`` returns
|
||||||
|
``https://x-access-token:TOKEN@github.com/...`` after insteadOf. The
|
||||||
|
fix strips userinfo so ``get_repo_full_name()`` returns the correct
|
||||||
|
``org/repo`` (used in Phase 8 ``gh issue view --repo``).
|
||||||
|
"""
|
||||||
|
ss.get_repo_full_name.cache_clear()
|
||||||
|
monkeypatch.setattr(
|
||||||
|
ss,
|
||||||
|
"run_cmd",
|
||||||
|
mock_run_cmd(
|
||||||
|
{
|
||||||
|
("git", "remote"): (
|
||||||
|
0,
|
||||||
|
"https://x-access-token:github_pat_TOKEN@github.com/slaid098/opencode-config.git\n",
|
||||||
|
"",
|
||||||
|
),
|
||||||
|
}
|
||||||
|
),
|
||||||
|
)
|
||||||
|
assert ss.get_repo_full_name() == "slaid098/opencode-config"
|
||||||
|
ss.get_repo_full_name.cache_clear()
|
||||||
|
|
||||||
|
|
||||||
# ── _resolve_repo_root ───────────────────────────────────────────────────────
|
# ── _resolve_repo_root ───────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue