diff --git a/.opencode/scripts/pipeline-status.py b/.opencode/scripts/pipeline-status.py index 0cf6eca..515a0b9 100644 --- a/.opencode/scripts/pipeline-status.py +++ b/.opencode/scripts/pipeline-status.py @@ -168,7 +168,11 @@ def parse_remote_url(url: str) -> tuple[str, str, str]: ssh_match = re.match(r"git@([^:]+):([^/]+)/(.+?)(?:\.git)?$", url) if ssh_match: 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: return https_match.group(1), https_match.group(2), https_match.group(3) raise ValueError(f"Cannot parse remote URL: {url}") diff --git a/.opencode/scripts/spec-status.py b/.opencode/scripts/spec-status.py index 90c61af..879a75a 100644 --- a/.opencode/scripts/spec-status.py +++ b/.opencode/scripts/spec-status.py @@ -126,7 +126,11 @@ def parse_remote_url(url: str) -> tuple[str, str, str]: ssh_match = re.match(r"git@([^:]+):([^/]+)/(.+?)(?:\.git)?$", url) if ssh_match: 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: return https_match.group(1), https_match.group(2), https_match.group(3) raise ValueError(f"Cannot parse remote URL: {url}") diff --git a/docs/decisions/023-pr-55-parse-remote-url.md b/docs/decisions/023-pr-55-parse-remote-url.md new file mode 100644 index 0000000..6d7f900 --- /dev/null +++ b/docs/decisions/023-pr-55-parse-remote-url.md @@ -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'а. \ No newline at end of file diff --git a/docs/handoff/pr-55-parse-remote-url.md b/docs/handoff/pr-55-parse-remote-url.md new file mode 100644 index 0000000..41acac5 --- /dev/null +++ b/docs/handoff/pr-55-parse-remote-url.md @@ -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, по конвенции репо). \ No newline at end of file diff --git a/tests/test_pipeline_status.py b/tests/test_pipeline_status.py index 369c6ea..fe4b870 100644 --- a/tests/test_pipeline_status.py +++ b/tests/test_pipeline_status.py @@ -141,6 +141,40 @@ def test_parse_remote_url_invalid(): 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 ────────────────────────────────────────────────────────────── @@ -704,6 +738,37 @@ def test_get_memory_file_path_ssh(monkeypatch): 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 ─────────────────────────────────────────────────────── diff --git a/tests/test_spec_status.py b/tests/test_spec_status.py index be5ce33..ba98b9c 100644 --- a/tests/test_spec_status.py +++ b/tests/test_spec_status.py @@ -152,6 +152,29 @@ def test_parse_remote_url_invalid(): 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 ─────────────────────────────────────────────────────── @@ -195,6 +218,32 @@ def test_get_repo_full_name_remote_error(monkeypatch): 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 ───────────────────────────────────────────────────────