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.
1 participant
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.