Repository navigation
feat: pin and unpin a commit from the signed API - #829
RaphaelFakhri wants to merge 2 commits into
Conversation
f68ccbb to
b6142a7
Compare
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Pin endpoints and response app/Http/Controllers/Api/CommitPinController.php, routes/api.php, KNOWLEDGE_BASE.md |
The controller invokes the pin or unpin action and returns a CommitResource. Authenticated, throttled routes expose both operations. The signed API capability list now includes commit pinning and unpinning. |
Endpoint behavior tests tests/Feature/Controllers/Api/CommitPinControllerTest.php |
Feature tests cover successful operations, replacement and stale unpin requests, authorization, missing commits, unsigned requests, and rate limiting. |
Priority:
Estimated code review effort: 2 (Simple) | ~15 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant APIClient
participant APIRoute
participant CommitPinController
participant UpdateCommitPin
participant CommitPageQuery
APIClient->>APIRoute: Send authenticated PUT request
APIRoute->>CommitPinController: Call update
CommitPinController->>UpdateCommitPin: Pin current user's commit
CommitPinController->>CommitPageQuery: Build commit page for user
CommitPageQuery-->>APIClient: Return CommitResource
Possibly related PRs
- dev-null-sh/dev-null.sh#1168: Changes how profile-pinned commits are retrieved, which connects to this PR’s API operations that update the same pin state.
Suggested reviewers: simonhamp
Merge Risk: 
b1955
-
The new pin and unpin endpoints have no identified issue requiring correction before merge.
Security architecture risk: b1955
-
The endpoints reuse existing signed-device authentication, ownership checks and atomic pin updates. Their effects remain limited to the caller’s own top-level commits. No material security regression was identified.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The new operation lets an approved signed device change its owner’s profile pin. The attacker-controlled commit identifier cannot select another user’s pin row: authorization requires commit ownership and a top-level commit, and persistence is keyed to the authenticated user.
Trust Boundaries and Controls
- observed — The routes sit inside mandatory signed-request authentication and the versioned API throttle group. Authentication establishes the approved device owner as the current user; the shared pins limiter permits 20 operations per minute keyed by user.
Resilience and Maintainability Implications
- inferred — Pinning replaces one authenticated user’s reference in a single update. Unpinning atomically clears it only when it still matches the requested commit, protecting a replacement from a stale DELETE. Repeated operations converge without competing writes; competing pins remain last-writer-wins. These persistence semantics predate the API caller.
-
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: adding commit pinning and unpinning to the signed API. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Lets the mobile app pin a commit to the user's profile and unpin it, the way the web's Pin does (#90). The app (NativePHP/dev-null-app) already shows the pinned commit, but the API has no route to change it.
Endpoints
PUT /api/v1/commits/{id}/pinandDELETE /api/v1/commits/{id}/pin, signed, no body.200with the commit asGET /api/v1/commits/{id}shows it, so the app gets the newis_pinned, the same way the star endpoints answer.UpdateCommitPinRequest,DeleteCommitPinRequest,UpdateCommitPinandDeleteCommitPin: only the user's own top-level commits, pending or rejected included. Pinning another commit replaces the pin, and unpinning an older commit leaves a newer pin alone.403for someone else's commit or a reply,404for a deleted or unknown commit,401without a signature.throttle:pins(20 a minute), shared with the web.The knowledge base no longer lists pinning as web-only.
Tests
tests/Feature/Controllers/Api/CommitPinControllerTest.phpcovers pin, unpin, replacing a pin, a stale unpin, the response shape, the403/404/401cases and the rate limit. All 16 tests fail without the change and pass with it.Summary by CodeRabbit