Skip to content

feat(api): PR action routes — merge + resolve-comments#88

Merged
AgentWrapper merged 11 commits into
mainfrom
feat/issue-21
Jun 2, 2026
Merged

feat(api): PR action routes — merge + resolve-comments#88
AgentWrapper merged 11 commits into
mainfrom
feat/issue-21

Conversation

@neversettle17-101
Copy link
Copy Markdown
Collaborator

@neversettle17-101 neversettle17-101 commented Jun 2, 2026

Summary

  • Implements POST /api/v1/prs/{id}/merge and POST /api/v1/prs/{id}/resolve-comments
  • Service logic lives in internal/service/pr/ as ActionManager interface + ActionService struct — following the projects controller pattern (import service package directly, no ports indirection)
  • Drops internal/ports/scm.go and internal/scm/ intermediary package from earlier iterations
  • Fixes P1 bug: resolveComments was guarding JSON decode with r.ContentLength > 0, which silently dropped bodies sent with chunked transfer encoding — replaced with unconditional decode + io.EOF check
  • *github.Provider satisfies PRProvider via duck typing; no extra adapter layer

Test plan

  • go test ./... passes (all packages green, drift guard passes)
  • POST /api/v1/prs/1/merge → 501 when service is nil
  • POST /api/v1/prs/1/resolve-comments → 501 when service is nil
  • Unit tests in service/pr/action_service_test.go cover happy paths and all error mappings
  • Unit tests in controllers/prs_test.go cover 200/404/409/422/501 for both routes

Closes #21.

🤖 Generated with Claude Code

Adds two 501 Not Implemented route shells for the SCM/PR action lane
as specified in issue #21. No business logic — the routes are stubs
that return a structured planned body with the embedded OpenAPI spec
slice, consistent with the existing route-shell pattern.

Routes registered:
  POST /api/v1/prs/{id}/merge
  POST /api/v1/prs/{id}/resolve-comments

Closes part of #18.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 2, 2026

Greptile Summary

Adds POST /api/v1/prs/{id}/merge and POST /api/v1/prs/{id}/resolve-comments action routes, wiring a new ActionManager interface and stub ActionService in internal/service/pr/. The PR addresses several issues from prior iterations — sentinel errors now live in service/pr, the nil-service guard correctly returns 501, and the body-decode guard correctly excludes io.ErrUnexpectedEOF.

  • controllers/prs.go + service/pr/errors.go: new controller with nil-guard 501s, error sentinel package corrected from controllers to service/pr, and isEmptyBody narrowed to io.EOF only.
  • service/pr/action_service.go: stub ActionService with TODO placeholders for both operations; correctly parses prID via strconv.Atoi for the merge response.
  • specgen/build.go + openapi.yaml: OpenAPI spec extended with both operations, schema components, and a prs tag.

Confidence Score: 5/5

Safe to merge; the core routing, error mapping, nil-service guard, and body-decode logic are all correct.

The major issues from prior review rounds have all been addressed: sentinel errors now live in service/pr (no circular-import risk), the nil-service guard correctly returns 501 via apispec.NotImplemented, and isEmptyBody is correctly narrowed to io.EOF only. The remaining findings are documentation or dead-code nits that do not affect runtime behavior. The stub nature of ActionService is clearly called out with TODOs and poses no runtime hazard since real callers must supply a non-nil ActionManager.

No files require special attention; specgen/build.go has a dead schemaNames entry and a missing 400 response unit for resolveComments, but neither affects the running server.

Important Files Changed

Filename Overview
backend/internal/httpd/controllers/prs.go New controller for PR action routes; nil-service guard correctly emits 501, error mapping reads from service/pr sentinels, and isEmptyBody is correctly narrowed to io.EOF only.
backend/internal/service/pr/action_service.go Stub ActionService with TODO placeholders; Merge correctly parses prID with strconv.Atoi (parse error silently ignored, returning PRNumber 0 for non-numeric input), ResolveComments always returns Resolved: 0.
backend/internal/service/pr/errors.go Sentinel errors correctly placed in service/pr package, eliminating the circular-import risk from the prior iteration.
backend/internal/httpd/api.go PRsController wired with deps.PRs directly (nil when SCM not configured); no stub injection, so the nil guard in the controller fires correctly.
backend/internal/httpd/apispec/specgen/build.go prOperations() added with correct reqBody: nil for resolveComments; ControllersResolveCommentsRequest entry in schemaNames is dead code and 400 response is undocumented.
backend/internal/httpd/controllers/prs_test.go Good coverage of 200/404/409/422/501 for both routes; section comment at line 39 is stale (says 503 SCM_NOT_CONFIGURED but tests correctly assert 501 NOT_IMPLEMENTED).
backend/internal/httpd/controllers/dto.go Four new DTO types added; json tags are consistent with the OpenAPI spec.
backend/internal/httpd/apispec/openapi.yaml Both PR routes added with correct path params, response schemas, and status codes; no 400 response documented for resolveComments despite the handler returning 400 on invalid JSON.

Sequence Diagram

sequenceDiagram
    participant Client
    participant PRsController
    participant ActionManager
    participant SCMProvider

    Client->>PRsController: "POST /api/v1/prs/{id}/merge"
    alt "Svc == nil"
        PRsController-->>Client: 501 NOT_IMPLEMENTED
    else
        PRsController->>ActionManager: Merge(ctx, prID)
        ActionManager->>SCMProvider: squash-merge (TODO)
        SCMProvider-->>ActionManager: MergeResult / error
        alt success
            ActionManager-->>PRsController: "MergeResult{PRNumber, Method}"
            PRsController-->>Client: 200 MergePRResponse
        else ErrPRNotFound
            PRsController-->>Client: 404 PR_NOT_FOUND
        else ErrPRNotMergeable
            PRsController-->>Client: 409 PR_NOT_MERGEABLE
        else ErrPRPreconditions
            PRsController-->>Client: 422 PR_PRECONDITIONS_UNMET
        end
    end

    Client->>PRsController: "POST /api/v1/prs/{id}/resolve-comments"
    alt "Svc == nil"
        PRsController-->>Client: 501 NOT_IMPLEMENTED
    else
        PRsController->>PRsController: decode optional JSON body
        alt malformed JSON
            PRsController-->>Client: 400 INVALID_JSON
        else
            PRsController->>ActionManager: ResolveComments(ctx, prID, commentIDs)
            ActionManager->>SCMProvider: resolve threads (TODO)
            SCMProvider-->>ActionManager: ResolveResult / error
            alt success
                ActionManager-->>PRsController: "ResolveResult{Resolved}"
                PRsController-->>Client: 200 ResolveCommentsResponse
            else ErrNothingToResolve
                PRsController-->>Client: 422 NOTHING_TO_RESOLVE
            end
        end
    end
Loading

Reviews (9): Last reviewed commit: "fix(prs): align nil-service guard with s..." | Re-trigger Greptile

Comment thread backend/internal/httpd/apispec/specgen/build.go Outdated
Comment thread backend/internal/httpd/apispec/specgen/build.go
Builds the two SCM/PR action routes end-to-end per issue #21:

  POST /api/v1/prs/{id}/merge
  POST /api/v1/prs/{id}/resolve-comments

**ports/scm.go** — new PRService interface, MergeResult, ResolveResult.

**adapters/scm/github** — adds ErrNotMergeable/ErrUnprocessable sentinels
to the client (405/409/422 classification) and MergePR / ListUnresolvedThreadIDs /
ResolveThread methods to the Provider.

**internal/scm/pr_service.go** — concrete PRService over PRProvider. Parses
the path ID as a PR number, calls the provider, maps github sentinel errors to
domain errors (ErrPRNotFound / ErrPRNotMergeable / ErrPRPreconditions /
ErrNothingToResolve). Nil PRService keeps routes registered but returns
OpenAPI-backed 501s.

**httpd/controllers/prs.go** — real handlers; writePRError maps the four domain
errors to 404 / 409 / 422 / 500.

**prs_test.go** — httptest coverage: 501 (nil service), 200/404/409/422 for
both routes, spec-slice present in 501 body.

**scm/pr_service_test.go** — table-driven unit tests with a fake PRProvider.

Closes part of #18. Closes #21.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@neversettle17-101 neversettle17-101 changed the title feat(api): PR action route shells (merge + resolve-comments) feat(api): PR action routes — full impl (merge + resolve-comments) Jun 2, 2026
Comment thread backend/internal/httpd/controllers/prs.go
Implements POST /api/v1/prs/{id}/merge and POST /api/v1/prs/{id}/resolve-comments.

Service logic lives in internal/service/pr (ActionManager interface + ActionService
struct). Controllers use the projects pattern — import the service package directly
rather than going through a ports interface. Drops the internal/scm intermediary
package and the ports/scm.go file added in earlier iterations.

Also fixes the ContentLength-based body-decode guard in resolveComments, which
silently dropped JSON bodies sent with chunked transfer encoding; now decodes
unconditionally and treats io.EOF as an absent body.

Closes #21.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@neversettle17-101 neversettle17-101 changed the title feat(api): PR action routes — full impl (merge + resolve-comments) feat(api): PR action routes — merge + resolve-comments Jun 2, 2026
ControllersProjectIDParam, ControllersSessionIDParam, and ControllersPRIDParam
are never matched by the schemaName interceptor — swaggest reflects path-param
structs inline rather than as $ref component schemas, so the hook is never
called for these types.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@neversettle17-101
Copy link
Copy Markdown
Collaborator Author

Removed all three dead path-param entries (ControllersProjectIDParam, ControllersSessionIDParam, ControllersPRIDParam) from schemaNames — they were never matched by the interceptor since swaggest reflects path-param structs inline rather than as $ref component schemas.

Comment thread backend/internal/service/pr/action_service.go Outdated
When commentIDs were provided, the parsed PR number was never used — any
thread ID could be resolved regardless of which PR was in the URL path.
Add a ListUnresolvedThreadIDs existence probe in the else branch so the PR
must be reachable before iterating the caller-supplied IDs.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread backend/internal/httpd/controllers/prs.go
neversettle17-101 and others added 2 commits June 2, 2026 22:20
A truncated body (e.g. {"commentIds":["T_1") returns io.ErrUnexpectedEOF,
not io.EOF. Treating it as an absent body caused the handler to fall through
to "resolve all unresolved threads" instead of returning 400. Only io.EOF
(reader returned no bytes) is a genuine empty-body signal.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove adapter changes (ErrNotMergeable/ErrUnprocessable from client.go,
  MergePR/ListUnresolvedThreadIDs/ResolveThread from provider.go)
- ActionService returns dummy values with TODO; no business logic
- Errors (ErrPRNotFound etc.) moved to controllers/errors.go
- PR DTOs moved to controllers/dto.go
- Remove 501 guards — stub service always wired via NewAPI default

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread backend/internal/httpd/api.go Outdated
Comment thread backend/internal/httpd/controllers/errors.go Outdated
- Restore original client.go alignment (no functional change)
- Move PR sentinel errors to service/pr/errors.go
- controllers/errors.go now only contains writePRError, referencing prsvc sentinels
- Fix schemaNames alignment in specgen/build.go (goimports lint)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread backend/internal/httpd/apispec/specgen/build.go
neversettle17-101 and others added 2 commits June 2, 2026 22:44
The nil-service fallback was silently injecting a stub that returned
fake merge/resolve success, misleading callers when no SCM is wired.
Remove the injection; nil Svc now returns 503 SCM_NOT_CONFIGURED.
Also inline writePRError into prs.go and delete controllers/errors.go.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
reqBody: nil removes the requestBody.required: true annotation so
generated SDK clients can omit the body (which resolves all threads).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread backend/internal/httpd/controllers/prs.go
Comment thread backend/internal/service/pr/action_service.go Outdated
Use apispec.NotImplemented (501) instead of 503 so nil-service responses
match the OpenAPI spec and generated clients hit the documented 501 branch.

Echo prID as PRNumber in the stub Merge to avoid claiming the wrong PR
was merged if NewActionService is wired by accident before real impl lands.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@AgentWrapper AgentWrapper merged commit fab5451 into main Jun 2, 2026
6 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(api): 1b — PR action route shell (merge + resolve-comments)

2 participants