WIP: #570 GitHub PR/commit automation slice#707
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65edf4b4c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| accountLogin: gitHubAccountLogin, | ||
| repositories: [ | ||
| { | ||
| id: gitHubRepositoryFullName.trim(), |
There was a problem hiding this comment.
When a user connects GitHub through this form, the saved repository_id is the owner/repo full name, but webhook ingestion checks the mapping against payload.Repository.ID from GitHub, which is the numeric repository id (gitHubRepositoryIsMapped(..., gitHubAnyID(payload.Repository.ID))). As a result, PR/push webhooks for repositories configured by the UI are treated as unmapped and return processedIssueCount: 0, so the new automation never links issues unless the repo was configured out-of-band with the numeric id.
Useful? React with 👍 / 👎.
| StateID string | ||
| } | ||
|
|
||
| var gitHubIdentifierPattern = regexp.MustCompile(`(?i)\b[A-Z][A-Z0-9]{1,9}-[0-9]+\b`) |
There was a problem hiding this comment.
Team keys can be a single character (validateKey only rejects empty keys, keys over 10 chars, or invalid characters), but this pattern requires at least two characters before the dash. In workspaces with identifiers like A-1, a PR title, branch, or commit message containing that issue id produces no matches, so GitHub webhooks won't link commits/PRs or run merged-PR workflow automation for those teams.
Useful? React with 👍 / 👎.
|
Controller disposition for current head Current blockers:
Evidence checked:
Required before merge: either land/repair #569 first and rebase this slice to consume its substrate, or explicitly retarget/narrow this PR as the #569 substrate replacement with the #569 contract and acceptance checks. Then rerun full Go/web/OpenAPI/SDK/Playwright gates in a dependency-complete environment. |
jaeyunha
changed the title
|
Controller update: PR #707 has been re-scoped in title/body to child #570 and marked WIP/blocked. It still must not merge as-is. Current path:
Merge gate remains: rerun full Go/web/OpenAPI/SDK/Playwright validation in a dependency-complete environment after the re-scope/rebase. |
|
Controller update after #696/#569 merged to #569 substrate is now available on
Required path now: rebase #707 onto current |
Union-merged all conflicts from origin/staging: - apps/api/internal/http/router.go: union both webhook routes (global + legacy per-integrationID) - apps/api/internal/integrations/github.go: union both architectures (legacy manual setup + new GitHub App connect/register flow + PR/commit processing logic); renamed legacy webhook handler to GitHubWebhookLegacy - apps/api/internal/integrations/handler.go: union all GitHub routes; take staging's setupRequirement checks - apps/api/internal/integrations/handler_test.go: take staging's figma/intercom/github env checks; remove duplicate TestGitHubSignatureVerification and duplicate githubTestSignature helper - packages/proto/openapi.yaml: union all GitHub endpoints and schemas from both sides - packages/sdk/src/generated.ts: regenerated from resolved openapi.yaml - apps/web/src/app/(app)/settings/integrations/page.tsx: take staging (newer GitHub App flow supersedes WIP manual setup UI) - apps/web/tests/integrations-view.test.tsx: take staging Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r actor identity - Add canManage role gate to GitHubStatus handler (was ungated, any workspace member could call it) - Remove WebhookSecret from gitHubStatusResponse struct and GitHubStatus response literal (secret only surfaces at setup time via GitHubSetup) - Remove gitHubActorIDByEmail call from handleGitHubCommitEvents (commit author email is untrusted git metadata; raw author name/email still stored as informational metadata) - Delete now-unused gitHubActorIDByEmail function Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e328d6c13
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| actorName := firstNonEmpty(commit.Author.Name, commit.Author.Username, commit.Committer.Name, commit.Committer.Username, payload.Sender.Login) | ||
| actorEmail := firstNonEmpty(commit.Author.Email, commit.Committer.Email) | ||
| actorLogin := firstNonEmpty(commit.Author.Username, commit.Committer.Username, payload.Sender.Login) | ||
| out = append(out, gitHubCommitEvent{RepositoryID: repositoryID, RepositoryFullName: payload.Repository.FullName, RepositoryURL: payload.Repository.HTMLURL, SHA: sha, Message: commit.Message, URL: firstNonEmpty(commit.HTMLURL, commit.URL), Identifiers: identifiers, ActorName: actorName, ActorEmail: actorEmail, ActorLogin: actorLogin, DeliveryID: deliveryID}) |
There was a problem hiding this comment.
For push webhooks, commit.Author.Name and commit.Author.Email are git metadata controlled by whoever authored the commit, not a verified GitHub identity. Since these values are later inserted into issue_history as actor_name/actor_email, a commit mentioning an issue can spoof the issue timeline by using an arbitrary author like CEO <ceo@company.com>. Use the GitHub sender/login as the actor and keep commit author details only as source metadata.
Useful? React with 👍 / 👎.
| GitHubStatusResponse: | ||
| type: object | ||
| required: [connected, integrationId, installationId, displayName, webhookUrl, webhookSecret, repositories, workflows] |
There was a problem hiding this comment.
The new GET /integrations/github schema marks webhookSecret as a required response field, but the GitHubStatus handler serializes gitHubStatusResponse, which has no WebhookSecret member and never returns it. Generated SDK consumers will therefore treat a field as guaranteed even though the API omits it on both connected and disconnected responses; either return the secret intentionally or remove it from the status response schema.
Useful? React with 👍 / 👎.
1 participant
Refs #570 under parent #558. Does not close #558.
This PR is currently blocked before merge until the GitHub App install/repository mapping/signed webhook substrate from #569 is landed or this branch is explicitly narrowed to replace #569's substrate contract.
Scoped changes currently present:
Validation evidence:
cd apps/api && go test ./internal/integrations ./internal/http.node scripts/check-migrations.mjs,node scripts/check-go-openapi-generated.mjs,node scripts/check-openapi-coverage.mjs,node scripts/check-web-api-empty.mjs.pnpm exec vitest run tests/integrations-view.test.tsx, SDK test/build,pnpm lint,make openapi-coverage,make migrations,make openapi-strict.Current blockers:
Required before merge: