diff --git a/docs/bugs/fixed/bug-slug-extractor-truncated-repo-names.md b/docs/bugs/fixed/bug-slug-extractor-truncated-repo-names.md new file mode 100644 index 0000000..31c14e2 --- /dev/null +++ b/docs/bugs/fixed/bug-slug-extractor-truncated-repo-names.md @@ -0,0 +1,92 @@ +--- +id: bug-slug-extractor-truncated-repo-names +title: Duplicate slug extractor truncated repo names ending in . g i t +type: bug +status: fixed +commits: ["34b863a"] +repos: [epic-code-gen] +decisions: [ADR-0030, ADR-0035] +--- + +# Bug: Duplicate slug extractor truncated repo names ending in `.` `g` `i` `t` + +## Summary + +`clone_target.py` carried its own `_extract_slug()`, a near-copy of +`github_utils.extract_slug()`. The copy stripped the `.git` suffix with +`url.rstrip(".git")`. + +`str.rstrip` takes a **character set**, not a suffix. It removes every trailing +character that appears in `{'.', 'g', 'i', 't'}`, so it does not stop once +`.git` is gone: + +``` +https://github.com/rh-forge/rh-forge-ui.git + → strip 't' 'i' 'g' '.' → rh-forge-ui (correct so far) + → strip 'i' → rh-forge-u (wrong; 'i' is in the set) +``` + +Every fork API call then addressed a repository that does not exist. + +## Reproduction + +```bash +python3 scripts/clone_target.py rh-forge/rh-forge-ui RHAI-760 --clean \ + --dest /tmp/tr --fork-owner ederign --gh-token-var RH_FORGE_GITHUB_TOKEN +``` + +## Expected + +Clone, add a `fork` remote for `ederign/rh-forge-ui`, create `epic/RHAI-760`. + +## Actual + +``` +HTTP 404: {"message":"Not Found", ... #get-a-repository} ← GET /repos/ederign/rh-forge-u +HTTP 404: {"message":"Not Found", ... #create-a-fork} ← POST /repos/rh-forge/rh-forge-u/forks +Error: HTTP Error 404: Not Found +``` + +`git clone` itself succeeded — the failure is entirely inside +`_setup_fork_remote()` → `github_utils.ensure_fork()`, which is why nothing in +the trace mentions git. GitHub answers 404 rather than 403 for a repository a +token cannot see, so the symptom reads exactly like a permissions problem: the +first hour of diagnosis went to the PAT (classic vs fine-grained, SSO +authorisation, org opt-in) and found nothing wrong with it. + +## Impact + +High, and latent since the extractor was duplicated. It fires only for repo +names whose last character is in `{'.', 'g', 'i', 't'}`, which no previous +target had — `odh-dashboard`, `kale`, `mlflow`, `codeflare-sdk` all end outside +the set. `rh-forge-ui` is the first target to end in `i`. Any future +`…-config`, `…-training` or `…-widget` target would have hit it too. + +Compounding it: `setup_target_repo()` records a clone failure as terminal +`Failed`, so both epics of RHAISTRAT-2671 had to be reset by hand in the data +repo before they could retry — see [[bug-clone-fault-marks-epic-failed]]. + +## Fix + +Deleted `clone_target._extract_slug` outright and pointed both call sites +(`_url_matches`, `_setup_fork_remote`) at `github_utils.extract_slug`, which +already used the correct `.removesuffix(".git")`. Deleting rather than patching +is the point: two copies of one function are what allowed the fix in +`github_utils` to never reach the copy that ran. + +Regression coverage: + +- `TestExtractSlug::test_repo_name_ending_in_git_suffix_chars` in + `tests/test_github_utils.py` — the character-set case directly. +- `TestSetupForkRemote::test_repo_name_ending_in_i_is_not_truncated` in + `tests/test_clone_target.py` — asserts the exact arguments reaching + `ensure_fork` for `rh-forge-ui`, pinning the call site that actually broke. + +The duplicated suite in `tests/test_clone_target.py` was removed along with the +function it covered. + +## Related + +- [[task-per-repo-github-identity]] — the change that first pointed a target at + `rh-forge/rh-forge-ui` and exposed this. +- [[bug-clone-fault-marks-epic-failed]] diff --git a/docs/bugs/open/bug-clone-fault-marks-epic-failed.md b/docs/bugs/open/bug-clone-fault-marks-epic-failed.md new file mode 100644 index 0000000..c677171 --- /dev/null +++ b/docs/bugs/open/bug-clone-fault-marks-epic-failed.md @@ -0,0 +1,70 @@ +--- +id: bug-clone-fault-marks-epic-failed +title: A clone or credential fault marks the epic terminally Failed +type: bug +status: open +repos: [epic-code-gen] +decisions: [ADR-0025] +--- + +# Bug: A clone or credential fault marks the epic terminally Failed + +## Summary + +`setup_target_repo()` treats any non-zero exit from `clone_target.py` as an +epic failure and writes `status: Failed`. `Failed` is in `CI_TERMINAL_STATES`, +so the epic is skipped on every subsequent run and only a hand-edit of +`run-metadata.yaml` in the data repo brings it back. + +But a clone failure is an *environment* fault, not a property of the epic: +an expired or unauthorised token, a private repo, a transient GitHub 5xx, a +network blip. Nothing about the epic changed, and the next run — after the +variable is fixed — would succeed. + +This is the same distinction [ADR-0025] draws for missing tools, where the +toolchain preflight deliberately leaves status at `Ready` so the epic retries +once the image is fixed. Clone faults were never brought into line with it. + +## Reproduction + +Point an epic at a private repo whose token is wrong or absent, then run the +pipeline twice. + +## Expected + +Run 1 flags the epic and generates nothing; status stays `Ready`. Run 2, after +the credential is corrected, picks it up and proceeds. + +## Actual + +Run 1 sets `status: Failed` with a `failure_reason`. Run 2 skips the epic +because `Failed` is terminal. The epic is stuck until someone edits the data +repo by hand. + +Observed live on RHAISTRAT-2671: `RHAI-760` went `Ready → Failed` on a 404 +caused by [[bug-slug-extractor-truncated-repo-names]], and had to be reset with +a manual commit to the data repo before the fix could even be tested. + +## Impact + +Medium. It does not corrupt anything, but it converts every transient +infrastructure fault into manual data-repo surgery, and it does so silently — +the dashboard shows a red epic that looks like a codegen failure. + +## Proposed Fix + +Classify the clone failure the way preflight already classifies a missing tool: + +- Credential / not-found / network faults → leave `status: Ready`, record the + reason, generate nothing. Retryable by construction. +- Genuinely epic-caused faults (a `target_repo` that is malformed or absent + from `config/repo_mapping.json`) → `Failed`, since a re-run cannot help. + +Worth extracting the retryable-vs-terminal judgement into one helper shared +with the preflight gate, rather than a second ad-hoc copy of the rule. + +## Related + +- [[bug-slug-extractor-truncated-repo-names]] +- [[task-toolchain-preflight]] +- [[task-per-repo-github-identity]] diff --git a/scripts/clone_target.py b/scripts/clone_target.py index bb06c81..1fb416d 100644 --- a/scripts/clone_target.py +++ b/scripts/clone_target.py @@ -128,22 +128,12 @@ def clone(repo_url, epic_id, dest=None, fork_owner=None, clean=False, def _url_matches(url, remote_output): """Check if url matches any remote URL (handles https vs git@ variants).""" - slug = _extract_slug(url) + slug = github_utils.extract_slug(url) if slug: return slug in remote_output return False -def _extract_slug(url): - """Extract org/repo from a GitHub URL.""" - url = url.rstrip("/").rstrip(".git") - if "github.com" in url: - parts = url.split("github.com")[-1].strip("/:").split("/") - if len(parts) >= 2: - return f"{parts[0]}/{parts[1]}" - return None - - def _configure_git_identity(dest, token): """Set git user.name and user.email from the GitHub token owner.""" user = github_utils.get_authenticated_user(token) @@ -263,7 +253,7 @@ def _setup_fork_remote(dest, upstream_url, fork_owner, token=None): Returns dict with: fork_url (display URL), fork_created. """ - slug = _extract_slug(upstream_url) + slug = github_utils.extract_slug(upstream_url) if not slug: return {"fork_url": None, "fork_created": False} diff --git a/tests/test_clone_target.py b/tests/test_clone_target.py index 70fc0aa..4c4f55b 100644 --- a/tests/test_clone_target.py +++ b/tests/test_clone_target.py @@ -12,7 +12,6 @@ from clone_target import ( clone, - _extract_slug, _url_matches, _ensure_branch, _setup_fork_remote, @@ -50,24 +49,6 @@ def _init_repo(path): # ─── URL Utilities ─────────────────────────────────────────────────────────── -class TestExtractSlug: - - def test_https_url(self): - assert _extract_slug("https://github.com/org/repo") == "org/repo" - - def test_https_with_git_suffix(self): - assert _extract_slug("https://github.com/org/repo.git") == "org/repo" - - def test_ssh_url(self): - assert _extract_slug("git@github.com:org/repo.git") == "org/repo" - - def test_trailing_slash(self): - assert _extract_slug("https://github.com/org/repo/") == "org/repo" - - def test_non_github(self): - assert _extract_slug("https://gitlab.com/org/repo") is None - - class TestUrlMatches: def test_matches_https(self): @@ -291,6 +272,23 @@ def test_with_token_uses_authenticated_url(self, tmp_path): ) assert "x-access-token" in out.stdout + def test_repo_name_ending_in_i_is_not_truncated(self, tmp_path): + """Regression: rh-forge-ui.git must not become rh-forge-u. + + The old module-local slug extractor used rstrip(".git"), so every + fork API call for this repo hit a name that doesn't exist and 404'd. + """ + repo = _init_repo(tmp_path / "repo") + with patch("github_utils.ensure_fork", + return_value=("ederign/rh-forge-ui", False)) as ensure_fork: + result = _setup_fork_remote( + repo, "https://github.com/rh-forge/rh-forge-ui.git", "ederign", + token="ghp_test123") + + ensure_fork.assert_called_once_with( + "rh-forge", "rh-forge-ui", "ederign", "ghp_test123") + assert result["fork_url"] == "https://github.com/ederign/rh-forge-ui.git" + def test_with_token_creates_fork(self, tmp_path): repo = _init_repo(tmp_path / "repo") with patch("github_utils.ensure_fork", return_value=("newuser/myrepo", True)): diff --git a/tests/test_github_utils.py b/tests/test_github_utils.py index 64dd0dc..fbc5cba 100644 --- a/tests/test_github_utils.py +++ b/tests/test_github_utils.py @@ -41,6 +41,16 @@ def test_non_github(self): def test_no_repo(self): assert extract_slug("https://github.com/org") is None + def test_repo_name_ending_in_git_suffix_chars(self): + """`.git` must be stripped as a suffix, not as a character set. + + rstrip(".git") eats any trailing '.', 'g', 'i' or 't', which turned + rh-forge-ui into rh-forge-u and 404'd every fork API call. + """ + for name in ("rh-forge-ui", "some-widget", "config", "kubeflow-training"): + url = f"https://github.com/org/{name}.git" + assert extract_slug(url) == f"org/{name}" + def test_empty_string(self): assert extract_slug("") is None