chore: add synapse-sentinel/gate workflow #14
Conversation
|
Warning Rate limit exceeded@jordanpartridge has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (16)
WalkthroughAdds issue comments, issue events/timeline, and milestone features (interfaces, traits, request classes, data value objects, and service wiring) with corresponding unit tests and validation updates; replaces existing CI workflows by removing three workflows and adding a single sentinel gate workflow; adds documentation EVENTS_USAGE.md. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/gate.yml (2)
9-16: Add explicitpermissions:for least-privilege GITHUB_TOKEN usage.
Start withcontents: readand add only whatsynapse-sentinel/gateactually needs (oftenchecks: write/pull-requests: writefor annotations).name: Gate +permissions: + contents: read + on: push: branches: [master] pull_request: branches: [master]
3-16: Rollout: decide whether you really want push-gating while it’s expected to fail.
Given the PR note that the syntax validation will likely fail initially, consider temporarily running only onpull_request(or making the syntax check optional) to avoid blocking direct pushes tomasterunintentionally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/gate.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/gate.yml (1)
3-8: Confirm branch targeting matches the repo’s default/protection rules (master vs main).
If the default branch ismain(or you want this on release branches too), this workflow won’t run where you expect.
| - uses: synapse-sentinel/gate@main | ||
| with: | ||
| coverage-threshold: 50 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the current state of the workflow file
cat -n .github/workflows/gate.yml | head -20Repository: conduit-ui/issue
Length of output: 432
🌐 Web query:
synapse-sentinel/gate GitHub action
💡 Result:
I couldn't find a public GitHub repo or Action named synapse-sentinel/gate. Did you mean a different name or a private repo? I can (a) search GitHub more widely for that exact repo, (b) look up related Actions (e.g., Synapse workspace deployment), or (c) help you create a GitHub Action named synapse-sentinel/gate — which would you prefer?
References I checked: GitHub Marketplace Synapse workspace deployment [1], Microsoft docs on Synapse CI/CD with GitHub Actions [2].
🌐 Web query:
GitHub Actions best practices pin version tag commit SHA
💡 Result:
Short summary of best practices for pinning Actions (tags vs SHAs):
-
Prefer full-length commit SHA for third-party actions — SHA is immutable and safest against tampered tags/branches. Example: uses: actions/checkout@f43a0e5 # v4.1.7. [1][2]
-
If you must use tags, pin to a specific release tag (vX.Y.Z), not floating tags (vX or main); add a comment with the tag version next to the SHA for clarity. Floating tags can be moved/changed. [2][6]
-
Avoid short (8-char) SHAs; use the full commit SHA. [3]
-
Automate updates: use Dependabot / Renovate / tools like pinact to safely update pinned SHAs/tags and surface PRs for review. Review changes before merging. [1][8]
-
Enforce and audit: enable organization/repository allowed-actions policies to require SHA pinning or block untrusted actions; use verified creators as a trust signal and audit action code where feasible. [1][2]
-
Tradeoffs: SHAs are most secure but require manual/automated updates (less convenient). Tags are convenient but mutable. Choose SHA for high-risk workflows (those that handle secrets or produce releases). [2][6]
Key references:
- GitHub Docs — Security hardening for GitHub Actions (pin to commit SHA; policies). [1]
- GitHub Well‑Architected — Pin versions of Actions (recommend full commit SHA, include version comment). [2]
- Examples/tools & community guidance (pinact, Ensure SHA Pinned Actions, blog posts). [4][8]
If you want, I can: (a) show exact workflow snippets for SHA pinning; (b) add Dependabot/Renovate config to update SHAs; or (c) produce a script to extract commit SHAs for a list of actions. Which would you like?
Use a pinned version for synapse-sentinel/gate instead of @main.
Using @main allows silent behavior changes and increases supply-chain risk. Pin to a specific commit SHA (preferred for third-party actions) or a release tag.
- - uses: synapse-sentinel/gate@main
+ - uses: synapse-sentinel/gate@<commit-sha-or-tag>
with:
coverage-threshold: 50Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/workflows/gate.yml around lines 14 to 16: the workflow currently uses
synapse-sentinel/gate@main which risks silent upstream changes; update the
action reference to a pinned version by replacing @main with a specific release
tag or a commit SHA (preferred) for the synapse-sentinel/gate action (e.g.,
@vX.Y.Z or @<full-commit-sha>), commit the updated YAML, and verify the workflow
run succeeds.
- Remove code-style.yml, static-analysis.yml, tests.yml - Add comprehensive gate.yml with separate jobs: - Tests & Coverage - Security Audit - Pest Syntax - Sentinel Certification (final verdict) - Each check shows as separate entry in PR checks
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/gate.yml (1)
18-18: Replace@mainwith a pinned version for allsynapse-sentinel/gateaction references.Using
@mainallows silent upstream behavior changes and increases supply-chain risk. Pin each action reference to a specific commit SHA (preferred for third-party actions) or a release tag (e.g.,@vX.Y.Z).Apply this diff to pin the action versions:
- - uses: synapse-sentinel/gate@main + - uses: synapse-sentinel/gate@<commit-sha-or-tag>Repeat this change for all four occurrences (lines 18, 32, 45, 59). Once you identify the correct version, update all references consistently.
Also applies to: 32-32, 45-45, 59-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/code-style.yml(0 hunks).github/workflows/gate.yml(1 hunks).github/workflows/static-analysis.yml(0 hunks).github/workflows/tests.yml(0 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/static-analysis.yml
- .github/workflows/code-style.yml
- .github/workflows/tests.yml
🔇 Additional comments (1)
.github/workflows/gate.yml (1)
10-22: Job structure, permissions, and orchestration look sound.The workflow correctly defines four jobs with appropriate dependencies (certify waits for tests, security, syntax), proper permissions (
contents: read,checks: write), and consistent token handling. Coverage threshold is appropriately scoped to tests and certify jobs only.Once the action version pinning is resolved, verify that the workflow executes successfully, particularly the syntax check against
test()function syntax (as noted in PR objectives).Also applies to: 24-35, 37-48, 50-62
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
.github/workflows/gate.yml (1)
17-22: Pin thesynapse-sentinel/gateaction to a specific version.Using
@mainallows silent behavior changes and increases supply-chain risk. Pin to a specific commit SHA or release tag for stability.- uses: actions/checkout@v4 - - uses: synapse-sentinel/gate@main + - uses: synapse-sentinel/gate@v1.0.0 # or commit SHA with: check: certify coverage-threshold: 50
🧹 Nitpick comments (7)
src/Traits/ValidatesInput.php (2)
269-277:strtotime()is too permissive for ISO 8601 validation.
strtotime()accepts many non-ISO 8601 formats (e.g.,"tomorrow","next week","+1 day"). For strict ISO 8601 validation, useDateTime::createFromFormat()or a regex pattern.- if (! strtotime($trimmed)) { - throw new InvalidArgumentException('Due date must be a valid ISO 8601 date string'); - } + $date = DateTime::createFromFormat(DateTime::ATOM, $trimmed); + if ($date === false) { + // Also try the basic ISO 8601 format GitHub commonly uses + $date = DateTime::createFromFormat('Y-m-d\TH:i:s\Z', $trimmed); + } + if ($date === false) { + throw new InvalidArgumentException('Due date must be a valid ISO 8601 date string'); + }
246-257: Consider consolidating duplicate state validation logic.
validateMilestoneState(lines 246-257) duplicates the logic ofvalidateState(lines 116-127). Both validate against the same['open', 'closed']set. Consider extracting a shared helper or reusingvalidateState.private function validateMilestoneState(mixed $state): void { - if (! is_string($state)) { - throw new InvalidArgumentException('State must be a string'); - } - - $validStates = ['open', 'closed']; - - if (! in_array($state, $validStates, true)) { - throw new InvalidArgumentException('State must be one of: open, closed'); - } + $this->validateState($state); }tests/Unit/Requests/Comments/DeleteCommentRequestTest.php (1)
9-14: Consider extracting reflection-based method testing to a shared helper.The reflection logic for testing protected properties appears across multiple request test files (DeleteCommentRequestTest, GetCommentRequestTest, UpdateCommentRequestTest). A shared test helper could reduce duplication:
function assertRequestMethod(Request $request, Method $expectedMethod): void { $reflection = new ReflectionClass($request); $property = $reflection->getProperty('method'); expect($property->getValue($request))->toBe($expectedMethod); }src/Data/Comment.php (1)
9-61: PreferDateTimeImmutable(or freeze the DateTime instances) to keep the VO truly immutable.
Even in areadonlyclass,DateTimecan be mutated after construction (e.g., via methods called on the property).-use DateTime; +use DateTimeImmutable; readonly class Comment { @@ - public DateTime $createdAt, - public DateTime $updatedAt, + public DateTimeImmutable $createdAt, + public DateTimeImmutable $updatedAt, @@ - createdAt: new DateTime($data['created_at']), - updatedAt: new DateTime($data['updated_at']), + createdAt: new DateTimeImmutable($data['created_at']), + updatedAt: new DateTimeImmutable($data['updated_at']),tests/Unit/Traits/ManagesMilestonesTest.php (1)
11-289: Consider asserting request shape in a few tests to catch regressions in request wiring.Saloon's MockClient supports
assertSent()with a closure to inspect the sent request (endpoint, method, query, body). For example, one or two tests could verify thatlistMilestones()builds the correct endpoint and query parameters. This is optional but useful for catching unintended changes to request construction.src/Data/IssueEvent.php (1)
34-34: Inconsistent null handling for milestone field.Line 34 uses
$data['milestone']['title'] ?? nullwithout checking if$data['milestone']is an array first, while other nullable nested objects (lines 28, 32, 33) useisset($data['field']) && $data['field'] !== nullbefore accessing nested properties. For consistency and safety, consider applying the same pattern.Apply this diff for consistent null handling:
- milestone: $data['milestone']['title'] ?? null, + milestone: isset($data['milestone']['title']) ? $data['milestone']['title'] : null,src/Data/Milestone.php (1)
75-82: Consider more explicit DateTime instantiation.Line 81 uses
new DateTime;(without parentheses) to represent "now". While valid PHP, usingnew DateTime()or explicitlynew DateTime('now')would be more conventional and clearer.- return $this->dueOn < new DateTime; + return $this->dueOn < new DateTime();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.github/workflows/gate.yml(1 hunks)EVENTS_USAGE.md(1 hunks)src/Contracts/IssuesServiceInterface.php(1 hunks)src/Contracts/ManagesIssueCommentsInterface.php(1 hunks)src/Contracts/ManagesIssueEventsInterface.php(1 hunks)src/Contracts/ManagesMilestonesInterface.php(1 hunks)src/Data/Comment.php(1 hunks)src/Data/IssueEvent.php(1 hunks)src/Data/Milestone.php(1 hunks)src/Data/TimelineEvent.php(1 hunks)src/Requests/Comments/CreateCommentRequest.php(1 hunks)src/Requests/Comments/DeleteCommentRequest.php(1 hunks)src/Requests/Comments/GetCommentRequest.php(1 hunks)src/Requests/Comments/ListCommentsRequest.php(1 hunks)src/Requests/Comments/UpdateCommentRequest.php(1 hunks)src/Requests/Events/ListIssueEventsRequest.php(1 hunks)src/Requests/Events/ListIssueTimelineRequest.php(1 hunks)src/Requests/Events/ListRepositoryIssueEventsRequest.php(1 hunks)src/Requests/Milestones/CreateMilestoneRequest.php(1 hunks)src/Requests/Milestones/DeleteMilestoneRequest.php(1 hunks)src/Requests/Milestones/GetMilestoneRequest.php(1 hunks)src/Requests/Milestones/ListMilestonesRequest.php(1 hunks)src/Requests/Milestones/UpdateMilestoneRequest.php(1 hunks)src/Services/IssuesService.php(1 hunks)src/Traits/ManagesIssueComments.php(1 hunks)src/Traits/ManagesIssueEvents.php(1 hunks)src/Traits/ManagesMilestones.php(1 hunks)src/Traits/ValidatesInput.php(2 hunks)tests/Unit/Data/CommentTest.php(1 hunks)tests/Unit/Data/IssueEventTest.php(1 hunks)tests/Unit/Data/MilestoneTest.php(1 hunks)tests/Unit/Data/TimelineEventTest.php(1 hunks)tests/Unit/Requests/Comments/CreateCommentRequestTest.php(1 hunks)tests/Unit/Requests/Comments/DeleteCommentRequestTest.php(1 hunks)tests/Unit/Requests/Comments/GetCommentRequestTest.php(1 hunks)tests/Unit/Requests/Comments/ListCommentsRequestTest.php(1 hunks)tests/Unit/Requests/Comments/UpdateCommentRequestTest.php(1 hunks)tests/Unit/Requests/EventRequestsTest.php(1 hunks)tests/Unit/Requests/MilestoneRequestsTest.php(1 hunks)tests/Unit/Traits/ManagesIssueCommentsTest.php(1 hunks)tests/Unit/Traits/ManagesIssueEventsTest.php(1 hunks)tests/Unit/Traits/ManagesMilestonesTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (27)
src/Requests/Comments/DeleteCommentRequest.php (4)
src/Requests/Comments/CreateCommentRequest.php (2)
__construct(18-23)resolveEndpoint(25-28)src/Requests/Comments/GetCommentRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Comments/ListCommentsRequest.php (2)
__construct(14-19)resolveEndpoint(21-24)src/Requests/Comments/UpdateCommentRequest.php (2)
__construct(18-23)resolveEndpoint(25-28)
src/Requests/Milestones/GetMilestoneRequest.php (4)
src/Requests/Milestones/CreateMilestoneRequest.php (2)
__construct(18-22)resolveEndpoint(24-27)src/Requests/Milestones/DeleteMilestoneRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Milestones/ListMilestonesRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Milestones/UpdateMilestoneRequest.php (2)
__construct(18-23)resolveEndpoint(25-28)
tests/Unit/Requests/EventRequestsTest.php (3)
src/Requests/Events/ListIssueEventsRequest.php (2)
ListIssueEventsRequest(10-30)resolveEndpoint(21-24)src/Requests/Events/ListIssueTimelineRequest.php (2)
ListIssueTimelineRequest(10-37)resolveEndpoint(21-24)src/Requests/Events/ListRepositoryIssueEventsRequest.php (2)
ListRepositoryIssueEventsRequest(10-29)resolveEndpoint(20-23)
src/Requests/Comments/GetCommentRequest.php (4)
src/Requests/Comments/CreateCommentRequest.php (2)
__construct(18-23)resolveEndpoint(25-28)src/Requests/Comments/DeleteCommentRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Comments/ListCommentsRequest.php (2)
__construct(14-19)resolveEndpoint(21-24)src/Requests/Comments/UpdateCommentRequest.php (2)
__construct(18-23)resolveEndpoint(25-28)
src/Contracts/ManagesMilestonesInterface.php (2)
src/Data/Milestone.php (1)
Milestone(9-94)src/Traits/ManagesMilestones.php (7)
listMilestones(23-38)getMilestone(40-52)createMilestone(54-66)updateMilestone(68-81)deleteMilestone(83-95)closeMilestone(97-100)reopenMilestone(102-105)
tests/Unit/Requests/Comments/ListCommentsRequestTest.php (2)
src/Requests/Comments/ListCommentsRequest.php (2)
ListCommentsRequest(10-30)resolveEndpoint(21-24)src/Requests/Comments/CreateCommentRequest.php (1)
resolveEndpoint(25-28)
src/Requests/Milestones/ListMilestonesRequest.php (5)
src/Data/Milestone.php (1)
__construct(11-25)src/Requests/Milestones/CreateMilestoneRequest.php (2)
__construct(18-22)resolveEndpoint(24-27)src/Requests/Milestones/DeleteMilestoneRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Milestones/GetMilestoneRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Milestones/UpdateMilestoneRequest.php (2)
__construct(18-23)resolveEndpoint(25-28)
tests/Unit/Traits/ManagesIssueCommentsTest.php (3)
src/Data/Comment.php (4)
Comment(9-61)isOwner(47-50)isMember(52-55)isContributor(57-60)src/Contracts/ManagesIssueCommentsInterface.php (5)
listComments(15-15)getComment(17-17)createComment(19-19)updateComment(21-21)deleteComment(23-23)src/Traits/ManagesIssueComments.php (5)
listComments(23-39)getComment(41-53)createComment(55-68)updateComment(70-83)deleteComment(85-97)
src/Contracts/ManagesIssueCommentsInterface.php (2)
src/Data/Comment.php (1)
Comment(9-61)src/Traits/ManagesIssueComments.php (5)
listComments(23-39)getComment(41-53)createComment(55-68)updateComment(70-83)deleteComment(85-97)
tests/Unit/Requests/Comments/CreateCommentRequestTest.php (1)
src/Requests/Comments/CreateCommentRequest.php (2)
CreateCommentRequest(12-36)resolveEndpoint(25-28)
src/Requests/Comments/ListCommentsRequest.php (6)
src/Requests/Comments/CreateCommentRequest.php (2)
__construct(18-23)resolveEndpoint(25-28)src/Requests/Comments/DeleteCommentRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Comments/GetCommentRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Comments/UpdateCommentRequest.php (2)
__construct(18-23)resolveEndpoint(25-28)src/Requests/Events/ListIssueEventsRequest.php (3)
__construct(14-19)resolveEndpoint(21-24)defaultQuery(26-29)src/Requests/Events/ListIssueTimelineRequest.php (3)
__construct(14-19)resolveEndpoint(21-24)defaultQuery(26-29)
src/Contracts/ManagesIssueEventsInterface.php (1)
src/Traits/ManagesIssueEvents.php (3)
listIssueEvents(22-38)listIssueTimeline(43-59)listRepositoryEvents(64-79)
tests/Unit/Data/IssueEventTest.php (3)
src/Data/IssueEvent.php (7)
IssueEvent(9-72)fromArray(23-36)toArray(38-51)isLabelEvent(53-56)isAssigneeEvent(58-61)isStateEvent(63-66)isMilestoneEvent(68-71)src/Data/Label.php (1)
Label(7-35)src/Data/User.php (1)
User(7-38)
src/Requests/Milestones/CreateMilestoneRequest.php (4)
src/Requests/Milestones/DeleteMilestoneRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Milestones/GetMilestoneRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Milestones/ListMilestonesRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Milestones/UpdateMilestoneRequest.php (3)
__construct(18-23)resolveEndpoint(25-28)defaultBody(30-33)
src/Data/Milestone.php (1)
src/Data/User.php (1)
User(7-38)
src/Requests/Comments/CreateCommentRequest.php (4)
src/Requests/Comments/DeleteCommentRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Comments/GetCommentRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Comments/ListCommentsRequest.php (2)
__construct(14-19)resolveEndpoint(21-24)src/Requests/Comments/UpdateCommentRequest.php (3)
__construct(18-23)resolveEndpoint(25-28)defaultBody(30-35)
tests/Unit/Data/CommentTest.php (2)
src/Data/Comment.php (6)
Comment(9-61)fromArray(21-32)toArray(34-45)isOwner(47-50)isMember(52-55)isContributor(57-60)src/Data/User.php (1)
User(7-38)
src/Requests/Events/ListRepositoryIssueEventsRequest.php (5)
src/Data/IssueEvent.php (1)
__construct(11-21)src/Requests/Comments/ListCommentsRequest.php (3)
__construct(14-19)resolveEndpoint(21-24)defaultQuery(26-29)src/Requests/Events/ListIssueEventsRequest.php (3)
__construct(14-19)resolveEndpoint(21-24)defaultQuery(26-29)src/Requests/Events/ListIssueTimelineRequest.php (3)
__construct(14-19)resolveEndpoint(21-24)defaultQuery(26-29)src/Requests/Milestones/ListMilestonesRequest.php (3)
__construct(14-18)resolveEndpoint(20-23)defaultQuery(25-28)
tests/Unit/Requests/Comments/UpdateCommentRequestTest.php (1)
src/Requests/Comments/UpdateCommentRequest.php (2)
UpdateCommentRequest(12-36)resolveEndpoint(25-28)
tests/Unit/Requests/Comments/GetCommentRequestTest.php (1)
src/Requests/Comments/GetCommentRequest.php (2)
GetCommentRequest(10-24)resolveEndpoint(20-23)
src/Data/IssueEvent.php (2)
src/Data/User.php (1)
User(7-38)src/Data/Label.php (1)
Label(7-35)
tests/Unit/Requests/Comments/DeleteCommentRequestTest.php (1)
src/Requests/Comments/DeleteCommentRequest.php (2)
DeleteCommentRequest(10-24)resolveEndpoint(20-23)
src/Data/TimelineEvent.php (2)
src/Data/User.php (1)
User(7-38)src/Data/Label.php (1)
Label(7-35)
tests/Unit/Requests/MilestoneRequestsTest.php (5)
src/Requests/Milestones/CreateMilestoneRequest.php (2)
CreateMilestoneRequest(12-33)resolveEndpoint(24-27)src/Requests/Milestones/DeleteMilestoneRequest.php (2)
DeleteMilestoneRequest(10-24)resolveEndpoint(20-23)src/Requests/Milestones/GetMilestoneRequest.php (2)
GetMilestoneRequest(10-24)resolveEndpoint(20-23)src/Requests/Milestones/ListMilestonesRequest.php (2)
ListMilestonesRequest(10-29)resolveEndpoint(20-23)src/Requests/Milestones/UpdateMilestoneRequest.php (2)
UpdateMilestoneRequest(12-34)resolveEndpoint(25-28)
tests/Unit/Data/MilestoneTest.php (2)
src/Data/Milestone.php (7)
Milestone(9-94)fromArray(27-44)toArray(46-63)isOpen(65-68)isClosed(70-73)isOverdue(75-82)completionPercentage(84-93)src/Data/User.php (1)
User(7-38)
src/Requests/Milestones/DeleteMilestoneRequest.php (5)
src/Data/Milestone.php (1)
__construct(11-25)src/Requests/Milestones/CreateMilestoneRequest.php (2)
__construct(18-22)resolveEndpoint(24-27)src/Requests/Milestones/GetMilestoneRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Milestones/ListMilestonesRequest.php (2)
__construct(14-18)resolveEndpoint(20-23)src/Requests/Milestones/UpdateMilestoneRequest.php (2)
__construct(18-23)resolveEndpoint(25-28)
src/Data/Comment.php (1)
src/Data/User.php (1)
User(7-38)
🪛 GitHub Actions: 🛡️ Sentinel Gate
tests/Unit/Data/IssueEventTest.php
[error] 1-1: Pest Syntax: Uses test() instead of describe/it blocks.
tests/Unit/Data/MilestoneTest.php
[error] 1-1: Pest Syntax: Uses test() instead of describe/it blocks.
tests/Unit/Data/TimelineEventTest.php
[error] 1-1: Pest Syntax: Uses test() instead of describe/it blocks.
🪛 LanguageTool
EVENTS_USAGE.md
[grammar] ~85-~85: Use a hyphen to join words.
Context: ...cts ### IssueEvent Represents a single issue event: ```php class IssueEvent { ...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (39)
src/Requests/Events/ListIssueTimelineRequest.php (1)
1-29: Clean implementation following established patterns.The request class correctly follows the Saloon conventions and mirrors the structure of other request classes in this codebase (e.g.,
ListIssueEventsRequest,ListCommentsRequest).src/Traits/ValidatesInput.php (1)
197-216: Validation helpers are well-structured.The new
validateMilestoneNumber,validateCommentId, andvalidateNotEmptymethods follow the established patterns and provide clear, consistent validation with meaningful error messages.src/Requests/Events/ListRepositoryIssueEventsRequest.php (1)
1-29: LGTM!The implementation is clean and consistent with the established patterns for request classes in this codebase. The endpoint correctly targets the repository-level issues events API.
src/Data/TimelineEvent.php (1)
9-26: Clean implementation of the value object pattern.The readonly class with
fromArray/toArraymethods and predicate helpers follows the established pattern fromUserandLabel. The use ofarray_filterto omit null values intoArrayis a good approach for API payloads.Also applies to: 48-66, 68-86
tests/Unit/Requests/Comments/DeleteCommentRequestTest.php (1)
8-31: Test coverage looks good.The tests appropriately validate HTTP method, endpoint resolution, and dynamic comment ID handling. The reflection-based approach is consistent with the codebase's testing patterns.
tests/Unit/Requests/Comments/UpdateCommentRequestTest.php (1)
8-46: Comprehensive test coverage.The tests validate all key aspects of the UpdateCommentRequest including HTTP method, endpoint resolution, request body structure, and edge cases like multiline comments. The reflection-based testing of protected methods is appropriate.
tests/Unit/Requests/Comments/GetCommentRequestTest.php (1)
8-31: Test coverage is solid.The tests appropriately validate the GetCommentRequest behavior including HTTP method, endpoint format, and dynamic ID handling.
src/Services/IssuesService.php (1)
10-11: Clean trait composition.The addition of
ManagesIssueComments,ManagesIssueEvents, andManagesMilestonestraits follows the existing service composition pattern and integrates well with the codebase architecture.Also applies to: 14-14, 19-20, 23-23
tests/Unit/Requests/Comments/ListCommentsRequestTest.php (1)
8-46: Thorough test coverage.The tests validate all aspects of ListCommentsRequest including method, endpoint, query parameter handling, and the edge case of empty filters. The reflection-based approach for testing protected methods is appropriate.
src/Contracts/IssuesServiceInterface.php (1)
7-7: Interface composition aligns with service implementation.The addition of
ManagesIssueEventsInterfaceandManagesMilestonesInterfaceto the extends clause correctly reflects the trait composition inIssuesService, maintaining consistency between the interface contract and its implementation.tests/Unit/Data/CommentTest.php (1)
8-157: Excellent test coverage for Comment data class.The test suite comprehensively validates the Comment data object including:
- Factory method (
fromArray) with nested User object handling- Serialization (
toArray) with proper date formatting- All author association helper methods (
isOwner,isMember,isContributor)- Edge cases (NONE association)
The test structure and assertions are well-designed.
EVENTS_USAGE.md (1)
1-273: Well-structured and comprehensive documentation.The documentation provides excellent coverage of the Issue Events and Timeline API including:
- Clear installation and setup instructions
- Complete API method documentation with practical examples
- Detailed DTO reference with helper methods
- Comprehensive event type catalog
- Error handling patterns
- Real-world usage examples
- Testing guidance and architecture overview
Note: The static analysis grammar hint about "single issue event" on line 85 is a false positive - no hyphenation is needed.
tests/Unit/Requests/Comments/CreateCommentRequestTest.php (1)
1-46: LGTM! Comprehensive test coverage with proper Pest syntax.The test suite thoroughly validates the
CreateCommentRequestclass with appropriate use of describe()/it() blocks, which aligns with the sentinel gate workflow requirements. The reflection-based approach for testing protected members is valid, and the coverage includes both standard and edge cases (multiline bodies).tests/Unit/Data/TimelineEventTest.php (1)
1-243: Intentional pipeline failure confirmed; test quality is excellent.As noted in the PR objectives, the pipeline failure for Pest syntax (test() vs. describe()/it()) is intentional to verify the sentinel gate action is working correctly. The test coverage itself is comprehensive and well-structured, covering:
- Multiple event types (commented, labeled, closed, committed, cross-referenced)
- Array serialization/deserialization with proper null filtering
- Predicate helper methods
- Edge cases (null actor, nested structures)
Once the gate validation is complete, you'll need to either convert these tests to describe()/it() syntax or configure the gate to make syntax checks optional.
src/Requests/Comments/ListCommentsRequest.php (1)
1-30: LGTM! Clean and consistent implementation.The request class follows the established pattern used throughout the codebase, with proper use of readonly properties, correct endpoint resolution, and appropriate query parameter handling. The implementation aligns with GitHub's API structure.
src/Requests/Milestones/GetMilestoneRequest.php (1)
1-24: LGTM! Straightforward and correct implementation.The request class properly implements milestone retrieval with correct HTTP method, endpoint construction, and readonly properties. The implementation is consistent with related milestone request classes.
src/Requests/Milestones/DeleteMilestoneRequest.php (1)
1-24: LGTM! Proper DELETE implementation.The request class correctly implements milestone deletion with the appropriate HTTP method and endpoint structure. The implementation follows the same clean pattern as the other milestone request classes.
src/Requests/Comments/DeleteCommentRequest.php (1)
1-24: LGTM! Clean deletion request implementation.The request class properly implements comment deletion with correct HTTP method, endpoint construction, and readonly properties. The implementation is consistent with the other comment request classes in the PR.
src/Requests/Milestones/UpdateMilestoneRequest.php (1)
1-34: LGTM! Proper PATCH implementation with body handling.The request class correctly implements milestone updates using the PATCH method with appropriate body handling via the
HasJsonBodytrait. The implementation follows the same pattern as other update operations in the codebase.src/Requests/Milestones/ListMilestonesRequest.php (1)
1-29: LGTM! Consistent list request implementation.The request class properly implements milestone listing with correct HTTP method, endpoint construction, and query parameter handling. The implementation follows the same clean pattern used for other list-type requests throughout the codebase.
tests/Unit/Requests/MilestoneRequestsTest.php (1)
12-137: LGTM! Tests are well-structured and comprehensive.The test suite thoroughly validates endpoint resolution, HTTP methods, and data handling for all milestone request classes using the describe/it syntax that passes the Pest syntax check.
src/Requests/Comments/GetCommentRequest.php (1)
10-24: LGTM! Clean implementation following Saloon patterns.The request class correctly implements the GET endpoint for fetching individual comments and is consistent with other comment-related request classes in the codebase.
tests/Unit/Data/MilestoneTest.php (1)
8-299: Test logic is solid; syntax issue is intentional.The tests comprehensively validate the Milestone data object including fromArray/toArray conversions, state predicates (isOpen, isClosed, isOverdue), and completionPercentage calculations. The use of test() syntax instead of describe/it blocks is causing the pipeline failure, which per the PR objectives is intentional to verify the gate action.
src/Requests/Milestones/CreateMilestoneRequest.php (1)
12-33: LGTM! Correct implementation for milestone creation.The request class properly implements the POST endpoint with JSON body handling using Saloon's HasJsonBody trait, consistent with other create/update request classes in the codebase.
tests/Unit/Traits/ManagesIssueCommentsTest.php (1)
30-215: LGTM! Comprehensive test coverage for comment management.The test suite thoroughly validates the ManagesIssueComments trait, including CRUD operations, validation checks, and author association predicates. The use of describe/it syntax is consistent with the project's testing standards.
src/Requests/Comments/CreateCommentRequest.php (1)
12-36: LGTM! Clean implementation for comment creation.The request class correctly implements the POST endpoint with proper JSON body structure for creating issue comments, consistent with UpdateCommentRequest and following Saloon best practices.
tests/Unit/Requests/EventRequestsTest.php (1)
10-140: LGTM! Comprehensive tests for event request classes.The test suite thoroughly validates HTTP methods, endpoint resolution, query parameters, and the special timeline preview header for ListIssueTimelineRequest. The describe/it syntax is consistent with project standards.
tests/Unit/Data/IssueEventTest.php (1)
9-258: Test logic is comprehensive; syntax issue is intentional.The tests thoroughly validate the IssueEvent data object including fromArray conversions for various event types (labeled, assigned, milestoned, referenced, closed), toArray serialization, and event type predicates. The use of test() syntax instead of describe/it blocks is causing the pipeline failure, which per the PR objectives is intentional to verify the gate action.
src/Traits/ManagesIssueComments.php (2)
41-97: CRUD methods are cohesive and consistent.
Validation + request objects +Comment::fromArrayis clean; delete returningstatus() === 204is correct for GitHub-style deletes.
20-39: No changes needed—the mapping is safe as-is.In Saloon v3.x,
Response::json()always returns an array of decoded JSON. ThehandleApiResponse()call beforejson()acts as a guard: if the response is unsuccessful (non-2xx status), it throws an exception; if successful, it returns early. Therefore, whenjson()is reached, the response is guaranteed to be successful and will always return an array, making thecollect()->map()pattern safe.tests/Unit/Traits/ManagesIssueEventsTest.php (1)
14-372: Good coverage of event/timeline mapping variants (null label/body, source/state fields, etc.).src/Contracts/ManagesIssueEventsInterface.php (1)
9-25: Interface matches the trait surface and documents DTO collection item types well.src/Contracts/ManagesMilestonesInterface.php (1)
10-28: Clean, complete milestone management contract.src/Requests/Comments/UpdateCommentRequest.php (1)
12-35: Pattern is correct and follows Saloon v3.10 documented behavior. The implementation correctly implementsHasBody, usesHasJsonBody, and overridesprotected defaultBody(): arrayto provide the JSON payload—this is the standard approach in Saloon v3.10 for PATCH requests. Saloon automatically setsContent-Type: application/jsonand allows runtime body modifications via thebody()helper if needed.src/Requests/Events/ListIssueEventsRequest.php (1)
10-29: LGTM. ThedefaultQuery()method correctly implements Saloon v3.10's query parameter hook with the proper signature and usage.src/Contracts/ManagesIssueCommentsInterface.php (1)
10-24: LGTM!The interface design is clean and follows standard CRUD patterns. Method signatures are well-typed and the DocBlock for
listCommentsclearly documents the collection type.src/Traits/ManagesIssueEvents.php (1)
14-80: LGTM!The trait implementation follows consistent patterns across all three methods with proper validation, error handling, and data mapping. The use of composed traits (
HandlesApiErrors,ValidatesInput) promotes good separation of concerns.src/Traits/ManagesMilestones.php (1)
15-106: LGTM!The trait provides comprehensive milestone management with proper validation and error handling. The
closeMilestoneandreopenMilestoneconvenience methods (lines 97-105) are particularly nice—they provide a cleaner API for common state transitions.src/Data/Milestone.php (1)
84-93: LGTM!The
completionPercentagemethod correctly handles the edge case of zero total issues and provides a useful metric with appropriate precision.
| public function toArray(): array | ||
| { | ||
| return [ | ||
| 'id' => $this->id, | ||
| 'event' => $this->event, | ||
| 'actor' => $this->actor?->toArray(), | ||
| 'created_at' => $this->createdAt->format('c'), | ||
| 'commit_id' => $this->commitId, | ||
| 'commit_url' => $this->commitUrl, | ||
| 'label' => $this->label?->toArray(), | ||
| 'assignee' => $this->assignee?->toArray(), | ||
| 'milestone' => $this->milestone, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Serialization asymmetry for milestone field.
The fromArray method expects $data['milestone'] to be an array with a 'title' key (line 34), but toArray outputs milestone as a plain string (line 49). This creates an asymmetry where calling toArray() then fromArray() on the result will fail.
If milestone should remain a string property (just the title), update fromArray to handle both array and string inputs:
- milestone: $data['milestone']['title'] ?? null,
+ milestone: is_array($data['milestone']) ? ($data['milestone']['title'] ?? null) : ($data['milestone'] ?? null),Alternatively, if the full milestone object structure should be preserved, change the constructor to accept a Milestone object instead of a string, and update both methods accordingly.
Committable suggestion skipped: line range outside the PR's diff.
| creator: User::fromArray($data['creator']), | ||
| openIssues: $data['open_issues'], | ||
| closedIssues: $data['closed_issues'], | ||
| dueOn: $data['due_on'] ? new DateTime($data['due_on']) : null, |
There was a problem hiding this comment.
Unsafe array key access for optional DateTime fields.
Lines 38 and 41 access $data['due_on'] and $data['closed_at'] without checking if the keys exist. If these keys are missing from the input array, PHP will generate undefined array key warnings.
Apply this diff to safely handle missing keys:
- dueOn: $data['due_on'] ? new DateTime($data['due_on']) : null,
+ dueOn: !empty($data['due_on']) ? new DateTime($data['due_on']) : null,- closedAt: $data['closed_at'] ? new DateTime($data['closed_at']) : null,
+ closedAt: !empty($data['closed_at']) ? new DateTime($data['closed_at']) : null,Also applies to: 41-41
🤖 Prompt for AI Agents
In src/Data/Milestone.php around lines 38 and 41, the code directly indexes
$data['due_on'] and $data['closed_at'] which can trigger undefined array key
warnings if those keys are absent; update both lines to first check for the
presence and non-null value (e.g., using isset() or the null coalescing
operator) before constructing a DateTime and otherwise assign null, so DateTime
is only created when the key exists and contains a non-null/non-empty value.
| id: $data['id'], | ||
| event: $data['event'], | ||
| actor: isset($data['actor']) && $data['actor'] !== null ? User::fromArray($data['actor']) : null, | ||
| createdAt: new DateTime($data['created_at']), |
There was a problem hiding this comment.
Uncaught exception risk from invalid created_at values.
new DateTime($data['created_at']) will throw an Exception if the date string is malformed or missing. Consider wrapping in a try-catch or validating the input first.
- createdAt: new DateTime($data['created_at']),
+ createdAt: self::parseDateTime($data['created_at']),Add a helper method:
private static function parseDateTime(string $value): DateTime
{
try {
return new DateTime($value);
} catch (\Exception $e) {
throw new \InvalidArgumentException("Invalid created_at date: {$value}", 0, $e);
}
}🤖 Prompt for AI Agents
In src/Data/TimelineEvent.php around line 34, constructing DateTime directly
from $data['created_at'] can throw on missing or malformed values; replace the
direct constructor with a safe parser: add a private static parseDateTime(string
$value): DateTime helper that catches DateTime exceptions and rethrows a clear
InvalidArgumentException, then call self::parseDateTime($data['created_at']) (or
validate presence before calling) so malformed or absent dates are handled with
a controlled exception.
| label: isset($data['label']) && $data['label'] !== null ? Label::fromArray($data['label']) : null, | ||
| assignee: isset($data['assignee']) && $data['assignee'] !== null ? User::fromArray($data['assignee']) : null, | ||
| milestone: $data['milestone']['title'] ?? null, | ||
| rename: $data['rename']['from'] ?? $data['rename']['to'] ?? null, |
There was a problem hiding this comment.
Data loss: rename field loses structure.
GitHub's rename event provides both from and to fields, but this mapping only captures one value. Consider preserving the full structure or both values.
- rename: $data['rename']['from'] ?? $data['rename']['to'] ?? null,
+ rename: isset($data['rename']) ? json_encode($data['rename']) : null,Alternatively, add separate renameFrom and renameTo properties:
public ?string $renameFrom = null,
public ?string $renameTo = null,And map:
renameFrom: $data['rename']['from'] ?? null,
renameTo: $data['rename']['to'] ?? null,🤖 Prompt for AI Agents
In src/Data/TimelineEvent.php around line 41 the current mapping flattens the
GitHub rename object into a single scalar (choosing from or to) which loses the
original structure; change the constructor/property model to preserve both
values by adding two nullable properties (renameFrom and renameTo) and map them
from $data['rename']['from'] and $data['rename']['to'] respectively, or
alternatively keep a single rename array/object property and assign the full
$data['rename'] to it so both fields are retained.
| protected function defaultHeaders(): array | ||
| { | ||
| return [ | ||
| 'Accept' => 'application/vnd.github.mockingbird-preview+json', | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub API timeline mockingbird-preview header 2025 required
💡 Result:
Short answer: No — as of current GitHub REST docs the timeline endpoint no longer requires the mockingbird preview header. Use the normal REST Accept (e.g. application/vnd.github+json) per the REST docs; the old preview (application/vnd.github.mockingbird-preview+json) was required only during the original preview period. [1][2][3]
Sources
- REST API docs — timeline endpoints (shows recommended accept header/application/vnd.github+json). [1]
- GitHub changelog: many REST previews graduated; only a couple previews still required as of 2021 (mockingbird not listed among remaining ones). [2]
- Historical developer blog announcing the Timeline API preview (mockingbird) during its preview period. [3]
🏁 Script executed:
# Search for Accept header usage in other request classes
rg "Accept.*vnd\.github" --type php -B 2 -A 2Repository: conduit-ui/issue
Length of output: 794
🏁 Script executed:
# Check the file structure and context around the ListIssueTimelineRequest
cd src/Requests/Events && ls -laRepository: conduit-ui/issue
Length of output: 406
Update the Accept header to the standard GitHub REST API format.
The mockingbird-preview media type is no longer required; GitHub graduated this preview feature years ago. Replace 'Accept' => 'application/vnd.github.mockingbird-preview+json' with 'Accept' => 'application/vnd.github+json' per current GitHub REST API documentation. Also update the corresponding test assertion in tests/Unit/Requests/EventRequestsTest.php.
🤖 Prompt for AI Agents
In src/Requests/Events/ListIssueTimelineRequest.php around lines 31 to 36,
replace the outdated Accept header
'application/vnd.github.mockingbird-preview+json' with the current standard
'application/vnd.github+json' to match the graduated API; then update the
corresponding assertion in tests/Unit/Requests/EventRequestsTest.php to expect
'application/vnd.github+json' instead of the mockingbird preview media type.
| if ($trimmed === '') { | ||
| throw new InvalidArgumentException("{$field} cannot be empty"); | ||
| } |
There was a problem hiding this comment.
Breaking change: body field can no longer be empty.
Adding the empty-string check to sanitizeString is a breaking change. The validateIssueData method at line 69 passes body through this function, which will now reject empty body strings. If empty bodies were previously valid (GitHub allows issues with empty bodies), this could break existing consumers.
Consider whether this validation should be conditional or if a separate sanitizer without the empty check is needed for nullable/optional fields.
🤖 Prompt for AI Agents
In src/Traits/ValidatesInput.php around lines 105 to 107, the added empty-string
check in sanitizeString throws on trimmed === '' which makes the change breaking
for optional/nullable fields like the issue "body" (validated at line 69);
update the code so sanitizeString does not unconditionally reject empty strings
— either add an optional parameter (e.g., $allowEmpty = false) to control the
empty check, or add a separate sanitizeOptionalString that skips the exception —
then call the appropriate variant from validateIssueData for the body field (or
adjust validateIssueData to only enforce non-empty for required fields).
| it('handles repository not found', function () { | ||
| $this->mockClient->addResponse(MockResponse::make( | ||
| ['message' => 'Not Found'], | ||
| 404 | ||
| )); | ||
|
|
||
| expect(fn () => $this->service->listIssueEvents('owner', 'repo', 123)) | ||
| ->toThrow(IssueNotFoundException::class); | ||
| }); |
There was a problem hiding this comment.
Test name/expectation mismatch on 404 (“repository not found” vs IssueNotFoundException).
Either rename the test to “handles issue not found” or update the expected exception to match the intended 404 mapping.
🤖 Prompt for AI Agents
In tests/Unit/Traits/ManagesIssueEventsTest.php around lines 106–114 the test
title says "handles repository not found" but the expectation asserts
IssueNotFoundException on a 404; either rename the test to "handles issue not
found" to match the asserted IssueNotFoundException, or change the expected
exception to the repository-not-found exception that the service maps 404 to
(e.g., RepositoryNotFoundException) so the test name and expected exception
align; update only the test name or the ->toThrow(...) to make them consistent
and run the test suite.
…fication Convert 6 data test files from test() to describe/it blocks to meet Pest syntax requirements for Sentinel Gate certification. Files updated: - IssueTest.php - LabelTest.php - UserTest.php - MilestoneTest.php - TimelineEventTest.php - IssueEventTest.php
Summary
Expected Behavior
The gate will likely fail initially because:
test()function instead ofdescribe()/it()blocksOnce validated, we can either:
describe()/it()syntaxSummary by CodeRabbit
Chores
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.