{{ message }}
fix(gitlab): populate owner/repo metadata so project-target binding lookup succeeds - #58
Merged
Merged
Conversation
Adds @opentag/gitlab: a new adapter package for GitLab issue and merge-request notes. Mirrors @opentag/github for normalize/render and ships a Hono-based local webhook receiver with constant-time token verification. CLI: wires "gitlab" into the platforms catalog as experimental_setup_pending (no setup wizard yet). Webhook port 3060 to avoid clash with 3050 (github). Scope: MR creation falls back to pr:create / pr:update until the core protocol grows MR-aware scopes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new @opentag/gitlab package, which acts as a GitLab adapter helper for OpenTag, enabling the normalization of GitLab issue and merge request notes into OpenTag events and rendering GitLab-friendly callback text. It also updates the CLI to support GitLab as an experimental platform. The review feedback suggests enhancing the webhook ingress handler by adding defensive checks on the payload to prevent runtime crashes and verifying the webhook token before reading the request body to mitigate potential DoS attacks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Contributor
Author
|
Hi! Before merging, I'd like to confirm the MVP scope is aligned with what you're willing to ship behind In scope (this PR)
Out of scope (intentionally)
Security choices worth a sanity check
Verification
Three questions before you merge:
Thanks! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/platforms/gitlab/display.ts`:
- Around line 11-12: The gitlabProjectWebhooksSettingsUrl helper is building a
malformed hooks link when projectPathWithNamespace includes leading or trailing
slashes. Normalize the input inside gitlabProjectWebhooksSettingsUrl by trimming
surrounding slashes before interpolating it into the GitLab hooks URL so copied
paths like /group/project produce a valid settings link.
In `@packages/gitlab/README.md`:
- Around line 24-31: The README example is using the wrong field name for
normalizeGitLabNote input, so update the example to use workItemUrl instead of
issueUrl. Keep the rest of the normalizeGitLabNote call the same, and make sure
the documented object shape matches the actual function parameters so copied
TypeScript examples align with the implementation.
In `@packages/gitlab/src/ingress.ts`:
- Around line 153-158: The claimed raw-body binding is not implemented because
handleNoteCreated only builds approval_gitlab_note_${id} and never takes
rawBody, so the action ID cannot include the request body hash. Update the
GitLab note handling flow in handleNoteCreated and its callers to accept
rawBody, derive the action id from both the note id and a hash of the raw body,
and make sure any downstream use of that id (including submitThreadAction and
related note-created handling paths) consistently uses the new value.
- Around line 247-249: The webhook handler in ingress.ts reads the request body
before validating the GitLab token, which allows unauthenticated requests to
force body buffering. Move the token check in the request handler so
verifyGitLabToken is called before c.req.text(), and only read rawBody after
authentication succeeds; keep the existing invalid_token response path in the
same handler flow.
- Around line 174-182: The threadKey in the GitLab ingress callback is ambiguous
because issues and merge requests can share the same IID, so the current
`${payload.project.path_with_namespace}#${issueIid}` format can collide. Update
the callback construction in ingress.ts to include the noteable kind alongside
the existing project path and IID, using the same isMergeRequest check already
used for noteableType so issue and MR threads are uniquely distinguished.
- Around line 264-270: The Note Hook branch in ingress should not cast payload
as GitLabNoteHookPayload without validation, because an empty signed body can
reach handleNoteCreated and crash on payload.object_attributes. Add a payload
shape check in the Note Hook / note path before calling handleNoteCreated, and
return a controlled 400 when the required note fields are missing; use the
existing handleNoteCreated and eventName branch as the place to gate this.
In `@packages/gitlab/src/normalize.ts`:
- Around line 150-154: The normalization logic in normalize.ts is missing
support for the legacy MergeRequestNote noteable type, so it gets filtered out
even though ingress accepts it. Update the isMergeRequest check in the
normalization flow to treat MergeRequestNote the same as MergeRequest, alongside
the existing IssueNote handling, so supported merge request notes are not
returned as null.
- Around line 18-19: The `projectPathWithNamespace` docstring is misleading
because `normalize.ts` expects the raw GitLab namespace path, not a URL-encoded
value. Update the comment on `projectPathWithNamespace` to describe the raw path
format (for example, `acme/demo`) and make sure any related usage in the
normalization logic reflects that this field should not be encoded before
building owner URIs or thread keys.
- Around line 207-210: The callback thread key in normalize() currently uses
only projectPathWithNamespace and iid, which can collide between issues and
merge requests. Update the callback object’s threadKey to include the work-item
kind as part of the key so it matches the ingress callback key shape, using the
existing input data in normalize() and the callback construction near the
threadKey assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75ac2e7c-5d67-48eb-bf50-c2e05da6ca7c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
packages/cli/src/catalogs/platforms.tspackages/cli/src/config.tspackages/cli/src/platforms/gitlab/display.tspackages/cli/src/platforms/ports.tspackages/cli/src/setup/guides.tspackages/gitlab/README.mdpackages/gitlab/package.jsonpackages/gitlab/src/index.tspackages/gitlab/src/ingress.tspackages/gitlab/src/normalize.tspackages/gitlab/src/render.tspackages/gitlab/test/ingress.test.tspackages/gitlab/test/normalize.test.tspackages/gitlab/test/render.test.tspackages/gitlab/tsconfig.jsonpackages/gitlab/tsup.config.ts
Resolves the nine actionable bot comments plus the docstring-coverage pre-merge check on PR amplifthq#58: Correctness - normalize.ts: gitlabWorkItem.externalId now encodes work-item kind (|issue|<iid> or |merge_request|<iid>) so issue #N and MR !N in the same project produce distinct externalIds. Same shape applied to callback.threadKey in normalize.ts and ingress.ts. - normalize.ts: isMergeRequest check accepts both the modern "MergeRequest" and the legacy "MergeRequestNote" noteable type (symmetric with the existing IssueNote handling). Prevents silently-dropped legacy MR notes. Hardening - ingress.ts: added shape-predicate isGitLabNoteHookPayload that enforces a Content-Length cap (1 MiB -> 413 payload_too_large) before body read, runs the token check BEFORE c.req.text() (closes the unauthenticated DoS amplifier), and rejects malformed payloads with 422 instead of silently proceeding with synthesised URLs/IDs from undefined fields. - ingress.ts: actionId now includes the first 12 hex chars of sha256(rawBody) for cross-package field-width uniformity with the dispatcher's stableHash. Reframed the comment from "replay protection" to the actual contract, which is idempotency for the apply-all decision. Surface polish - cli/display.ts: gitlabProjectWebhooksSettingsUrl now trims leading/trailing slashes from projectPathWithNamespace so inputs like "acme/demo/" produce "https://gitlab.com/acme/demo/-/hooks" rather than a double-slash. - README + normalize.ts JSDoc: corrected issueUrl to workItemUrl in the README example and replaced the URL-encoded-project-path JSDoc with the accurate "raw slash-separated identifier" framing. Docstring coverage - Added JSDoc to every exported symbol in ingress.ts, normalize.ts, and render.ts (the index.ts barrel needs no docstrings). Tests - packages/cli/test/platforms/gitlab/display.test.ts: 3 slash-trim cases for gitlabProjectWebhooksSettingsUrl. - packages/gitlab/test/ingress.test.ts: 5 new tests (token-before-body, invalid-payload 422, body-size 413, body-mutation changes actionId, same body same actionId, issue-vs-MR collision in callback.threadKey); updated 1 existing assertion to the new actionId shape. - packages/gitlab/test/normalize.test.ts: 2 new tests (MergeRequestNote happy path, MergeRequestNote without mergeRequestIid falls back to issueIid) and 1 new collision test; updated 3 existing assertions to the new threadKey/externalId shape. 31/31 gitlab tests pass. pnpm -r build / lint / typecheck / test all green.
AnandSundar
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/gitlab/src/ingress.ts`:
- Around line 195-217: The isGitLabNoteHookPayload guard is too permissive and
allows note payloads through that handleNoteCreated cannot safely use. Tighten
the checks in isGitLabNoteHookPayload to require every field consumed
downstream, including object_attributes.url, project.id, project.visibility,
user.username, and a valid supported-note IID/URL source, and reject payloads
that would otherwise fall back to an invalid iid value instead of passing them
through.
- Around line 170-177: The PROJECT_PATH_NAMESPACE_PATTERN in ingress.ts is too
restrictive because it only allows a single slash, so valid hierarchical
project.path_with_namespace values from GitLab webhook payloads are rejected.
Update the validation used by the path-with-namespace guard to accept nested
subgroup paths while still blocking pipe and whitespace characters, and keep the
check aligned with how WorkItemReference.ownerContainer.id and
callback.threadKey are derived.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9c2fc1d-b566-4bde-97a9-169ec038f817
📒 Files selected for processing (8)
packages/cli/src/platforms/gitlab/display.tspackages/cli/test/platforms/gitlab/display.test.tspackages/gitlab/README.mdpackages/gitlab/src/ingress.tspackages/gitlab/src/normalize.tspackages/gitlab/src/render.tspackages/gitlab/test/ingress.test.tspackages/gitlab/test/normalize.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/gitlab/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cli/src/platforms/gitlab/display.ts
- packages/gitlab/src/normalize.ts
- packages/gitlab/src/render.ts
Round-2 PR amplifthq#58 review comments, addressed in three local changes: Correctness - isGitLabNoteHookPayload now requires every field handleNoteCreated reads: object_attributes.url, object_attributes.noteable_type, project.id, project.visibility (private|internal|public), and user.username. Signed payloads missing any of those now return 422 invalid_payload instead of leaking undefined into the dispatched event or collapsing onto an iid=0 conversation lane. Hardening - PROJECT_PATH_NAMESPACE_PATTERN now permits one-or-more slash-separated segments so nested-subgroup projects (group/sub/p) pass shape validation. Pipe and whitespace denials are preserved. - handleNoteCreated returns 422 (not silent URL synthesis) when a supported-note payload carries iid <= 0 or a missing matching URL. Tests - 23 new assertions across 3 describe blocks: nested-subgroup paths, shape-predicate field coverage, supported-note integrity. - 51/51 gitlab package tests pass; tsc --noEmit clean.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/gitlab/test/ingress.test.ts`:
- Around line 490-505: The test is tautological because it recomputes the
expected notes callback URL instead of checking what `createGitLabWebhookApp`
actually passed into `createRun`. Update `postNote` to return or expose the
`createRun` mock, then assert the callback URL from
`createRun.mock.calls[0]![0]` in the `ingress.test.ts` note-hook cases so
regressions in `buildApiNotesUrl` are caught.
- Around line 778-787: The merge-request fixture in this test is inconsistent
because only merge_request.url is unset while object_attributes.url still points
to the issue-note URL from supportedBase. Update the ingress test case around
postRaw in ingress.test.ts so object_attributes.url matches the merge-request
note URL, keeping the fixture aligned except for the single missing field. This
keeps the 422 assertion focused on the missing merge_request.url and avoids
unrelated URL/type consistency failures in the MergeRequest note path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8866169b-ba89-40b1-b1bf-fcb5f0548d1a
📒 Files selected for processing (2)
packages/gitlab/src/ingress.tspackages/gitlab/test/ingress.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/gitlab/src/ingress.ts
…teral Two test-quality follow-ups on b062d73: - nested-subgroup 'encodes REST callback URL' test now reads the URI createRun received (event.callback.uri) instead of asserting against a locally recomputed expectedPath() helper. The previous form was tautological — it proved encodeURIComponent('acme/team/demo') matches the hard-coded literal, not that buildApiNotesUrl() produced it. - 'returns 422 when MR note is missing merge_request.url' test now aligns object_attributes.url to the MR-note URL so the only missing field is merge_request.url. The previous form left object_attributes.url as the issue-note URL, which would mask the real gap if URL/type consistency validation were tightened later.
AnandSundar
changed the title
Contributor
|
Thanks for closing the previous GitLab review threads. I pulled the latest head locally, merged it onto current main (including #52), and the local gates passed for me: pnpm typecheck, pnpm test, pnpm build, and pnpm lint. One remaining blocker before merge: the GitLab normalized event does not currently populate the repository identity fields the local daemon uses to resolve a workspace binding. Could you add a small follow-up to bridge that mapping? Suggested shape:
I think the rest of the PR is in good shape for the current experimental adapter scope; this is the main integration gap I would want fixed before merging. |
Contributor
Author
|
@mingyooagi Thank you for reviewing my PR. I'm working on the changes you suggested. I will update the PR soon. |
…lookup
normalizeGitLabNote was emitting metadata.repoProvider + projectPathWithNamespace + projectId, but projectTargetRefFromEvent (packages/core/src/project-target.ts) only resolves a ref from metadata.owner and metadata.repo. As a result, the local daemon could not match GitLab webhooks to a configured repoBindings row and routed runs through the No local workspace mapping is configured branch.
Add a private ownerRepoFromProjectPath helper in packages/gitlab/src/normalize.ts that splits path_with_namespace on /, treating the trailing segment as the project leaf and the joined leading segments as the nested namespace owner. Wire it into the metadata block in normalizeGitLabNote so the event carries owner + repo alongside repoProvider, mirroring the github normalizer shape in packages/github/src/normalize.ts.
Existing fields stay untouched: callback.threadKey and WorkItemReference.ownerContainer.id continue to carry the full path_with_namespace (the dispatcher admission gate depends on this format, and the WorkItemReference reshape is deferred to a follow-up).
Add a new describe block in packages/gitlab/test/normalize.test.ts covering single-level, nested, three-deep paths, defensive single-segment omission, and regressions on the existing metadata fields and callback.threadKey / ownerContainer.id. Add two cases to the existing describe block in packages/core/test/project-target.test.ts: a gitlab-shaped event resolves to { provider, owner, repo } with a nested owner, and a gitlab event missing owner or repo returns null.
AnandSundar
changed the title
Contributor
Author
This was referenced Jul 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Round 3 on PR #58. Closes the
mingyooagifollow-up comment:normalizeGitLabNotewas emittingmetadata.repoProviderbut notmetadata.owner/metadata.repo, soprojectTargetRefFromEventcould not resolve a ref for a GitLab event and the local daemon routed every GitLab webhook into the "No local workspace mapping is configured" branch. Source change is gitlab-only; the@opentag/coreproject-target helper is unchanged.What changed, grouped by concern
Correctness
normalizeGitLabNotenow populatesmetadata.owner(the leadingpath_with_namespacesegments joined by/) andmetadata.repo(the trailing project segment) alongside the existingmetadata.repoProvider: "gitlab". Mirrors the github normalizer shape (packages/github/src/normalize.ts:197-204, 263-270).ownerRepoFromProjectPath(pathWithNamespace)inpackages/gitlab/src/normalize.ts. Splits on/; for 2+ segments, returns{ owner: leading-joined, repo: trailing }. Single-segment paths returnundefined, so the metadata spread is a no-op — unreachable throughPROJECT_PATH_NAMESPACE_PATTERNat the ingress boundary, defensive against direct unit-call with a malformed path.Test coverage
packages/gitlab/test/normalize.test.ts— newdescribe("owner/repo projection from projectPathWithNamespace")block with 7 cases: single-level (acme/demo), nested-group (acme/team/demo, the example from the comment), three-deep (acme/team/sub/demo), defensive single-segment omission, plus regression assertions on existingcallback.threadKey(pathWithNamespace|kind|iid) andWorkItemReference.ownerContainer.id(the fullpathWithNamespace).packages/core/test/project-target.test.ts— 2 new cases inside the existingdescribe("ProjectTargetRef")block: a gitlab-shaped event resolves to a{ provider: "gitlab", owner: "acme/team", repo: "demo" }ref, and a gitlab-shaped event missing owner or repo returns null.Untouched
WorkItemReference.ownerContainer.id,callback.threadKey, andworkItem.externalIdcontinue to carry the fullpath_with_namespace. The dispatcher admission gate depends on the threadKey shape, and theWorkItemReferencereshape is still deferred.metadatais the only event-shape field touched. The metadata schema isRecord<string, unknown>, so the new keys are pure additions.Tests
packages/core/test/project-target.test.ts(8 after round 2; added 2 in this round).pnpm -r build,pnpm -r lint,pnpm -r typecheckall green.Why this is not a wider change
The diagnosis in the comment was specific: missing
owner/repoin metadata. ReshapingWorkItemReference.ownerContainer.idto carry just the project leaf, or changingcallback.threadKeytoowner|repo|kind|iid, would have been a heavier change with a seed-wipe requirement — any dispatcher state onfeat/gitlab-platformis keyed by threadKey. Adding the two metadata keys fixes the binding lookup without invalidating in-flight dispatches against the round-2 shape.Plan:
docs/plans/2026-06-30-003-fix-pr58-round-3-maintainer-repo-target-mapping-plan.mdSummary by CodeRabbit