Repository navigation
fix(github): accept PR-tab URLs (e.g. /files, /commits) in PR URL parser - #68
detail-app[bot] wants to merge 2 commits into
Conversation
🤖 Augment PR SummarySummary: Broadens GitHub URL parsing to recognize PR and issue URLs with trailing tab or deep-link paths. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| "prShortDescription" = NULL | ||
| WHERE "prDescriptionFetchedAt" IS NOT NULL | ||
| AND "prShortDescription" IS NULL | ||
| AND "prUrl" ~ 'github\.com/[^/]+/[^/]+/pull/[0-9]+/'; |
There was a problem hiding this comment.
prisma/migrations/20260925120000_reset_blacklisted_pr_tab_urls/migration.sql:6 also matches https://github.com/o/r/pull/42/, which the prior regex already parsed; if summarization returned no text, this migration resets it and causes another GitHub/OpenAI attempt despite it not being affected by this regression. The unanchored match likewise includes hosts such as notgithub.com, outside the intended GitHub-only recovery scope.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Addressed in 6973637. Anchored the match to ^https?://(www.)?github.com/ and required a path segment after the PR number (pull/[0-9]+/[^/?#]), so plain pull// URLs (already parsed by the prior regex) and non-github hosts like notgithub.com are no longer reset.
… segment Anchor the migration's prUrl match to github.com and require at least one path segment after the PR number, so plain pull/<n>/ URLs (already parsed by the prior regex) and non-github hosts are not reset unnecessarily.
|
Closing this pull request because it has had no human activity in 30 days (since 2026-09-25) and was opened 30 days ago. Detail closes its pull requests once they are more than 30 days old and have had no human activity for 14 days. If this change is still useful, reopen the pull request and Detail won't close it again. To keep a Detail pull request open, add the |
Detail bug report: View on Detail
Bug
parseGithubPullRequestUrlinsrc/github/index.tsrejected valid GitHub PR URLs that carry a trailing path segment after the PR number — any PR-tab URL such as…/pull/42/files,/commits,/diffs,/conflicts,/patches, or a deep link like…/pull/42/files/diff/<sha>. TheGITHUB_URL_REregex only allowed an optional trailing/plus a?#suffix after/pull/<n>, so any additional segment failed to match$and the parser returnednull.Its sole consumer,
generatePrDescriptions, selects projects whoseprUrlcontains/pull/(so tab URLs are selected), then onnullsetsprDescriptionFetchedAtwithoutprShortDescriptionandcontinues — permanently blacklisting the row. Affected PRs never appear in thecontributionsquery, which filtersprShortDescription: { not: null }. Students paste their own PR links viaaddProjectPrwith no normalization, so a URL copied from GitHub's Files-changed or Commits tab is stored verbatim and silently dropped.Fix
GITHUB_URL_REto allow zero or more trailing path segments after the PR (or issue) number via(?:\/[^/?#]+)*?inside the existing(issues|pull)/(\d+)group. Capture-group indices are unchanged, soparseGithubPullRequestUrlandparseGithubRepoUrlare unchanged beyond accepting the trailing segments. I used*?(multi-segment) rather than a single-segment form so deep PR links (e.g./pull/42/files/diff/<sha>,/pull/42/commits/<sha>) also parse.20260925120000_reset_blacklisted_pr_tab_urls) that resetsprDescriptionFetchedAtandprShortDescriptiontoNULLonly for rows the over-narrow regex wrongly blacklisted — fetched-but-no-description, with aprUrlmatching a PR-tab pattern. This re-queues them for the next cron run; already-summarized PRs, never-attempted rows, genuinely malformed URLs, and non-github URLs are untouched.Testing
tsc --skipLibCheck --noEmit, the offline parser unit tests, a Docker build (CI parity), andnpm run prisma(noschema.prismachurn).src/github/index.test.ts: a trailing-tab URL (/pull/42/files), a multi-segment deep link (/pull/42/files/diff/abc123), and an issue URL with a trailing segment (/issues/42/files→null, guarding against over-acceptance since the new clause attaches to the shared(issues|pull)group). Reverting only the regex makes these fail and the suite exit 1, confirming they guard the fix.www.,http://,/pull/with no number,/blob/..., non-github hosts) — all parse as expected.generatePrDescriptions(typediContainerinjection): a/pull/42/filesrow now gets the success update signature (bothprDescriptionFetchedAtand a non-emptyprShortDescription, with the ref{codeday, labs-gql, 42}extracted), while genuinely malformed URLs still get the blacklist-only signature.Projectrows and ran the migration SQL — exactly the 3 PR-tab blacklisted rows were reset; already-summarized, never-attempted, malformed, and non-github rows were untouched. Re-running updated 0 rows.…/pull/42/filesrow, applied the reset SQL, ran the realgeneratePrDescriptionstask (realPrismaClient, external GitHub/OpenAI calls stubbed), and confirmed the row appears in thecontributions-equivalent query withprUrlpreserved verbatim and a non-emptyprShortDescription.Could not run:
prisma migrate deployagainst a fresh database: it fails at a pre-existing 2022 migration (DROP INDEX "Student.username_unique"references an index no migration in the chain creates), which cascades to later migrations. Unrelated to this fix; the migration's SQL validity was instead verified by building the schema fromschema.prisma(prisma db push) and executing the migration SQL against it./graphqlendpoint smoke test: the dev environment has no Elasticsearch (the server fails to boot with a connection refused on the configuredELASTIC_URL) and the configuredGITHUB_TOKEN/OPENROUTER_API_KEYare placeholder values GitHub rejects with401 Bad credentials. The data-path test above exercises everything except the two external API calls.Note:
npx eslint .reports 285 parse errors across the repo from a pre-existingeslint/@typescript-eslint/parserincompatibility with the installed TypeScript (present on untouched files); not introduced or worsened by this change, and not run in CI.Automatic Fixes PRs can be configured here.