docs: exempt maintainers from the public API issue-first rule - #245
Conversation
|
The PR should not merge until maintainer detection works for non-PostHog commit addresses and the existing-PR branch unambiguously overrides the stop instruction. Reviews (1) · Last reviewed commit: "docs: exempt maintainers from the public..." |
| - Read and follow [CONTRIBUTING.md](./CONTRIBUTING.md) before contributing to the SDK. It covers development setup, the example app, CI-aligned checks, and pull requests. | ||
| - Read and follow [RELEASING.md](./RELEASING.md) when adding change intents (`pnpm change`) or working on publishing. | ||
| - Before adding or changing public API, follow "Public API changes" in [CONTRIBUTING.md](./CONTRIBUTING.md): the API shape must be agreed on the issue first. For SDK design guidance, read https://posthog.com/handbook/engineering/sdks/guidelines.md. | ||
| - Public API changes (a diff in `api/public-api.json`): if the author is a PostHog maintainer (git email ends in `@posthog.com`), the PR is the discussion, so don't open or suggest an issue. Otherwise, follow "Public API changes" in [CONTRIBUTING.md](./CONTRIBUTING.md): if there's no agreed issue, stop and tell the user. If a PR already exists, add a public-API note to its description and draft an issue body for the user to post. Never open an issue yourself. For SDK design guidance, read https://posthog.com/handbook/engineering/sdks/guidelines.md. |
There was a problem hiding this comment.
Maintainer detection is unreliable
The @posthog.com email check misclassifies Client Libraries maintainers who commit with GitHub noreply or personal addresses. Current maintainers have authored recent commits using @users.noreply.github.com, so agents will still stop their public-API PRs and request duplicate issues—the behavior this change is intended to eliminate. Use actual team membership or another reliable maintainer signal instead of the commit-email domain.
Prompt To Fix With AI
This is a comment left during a code review.
Path: AGENTS.md
Line: 5
Comment:
**Maintainer detection is unreliable**
The `@posthog.com` email check misclassifies Client Libraries maintainers who commit with GitHub noreply or personal addresses. Current maintainers have authored recent commits using `@users.noreply.github.com`, so agents will still stop their public-API PRs and request duplicate issues—the behavior this change is intended to eliminate. Use actual team membership or another reliable maintainer signal instead of the commit-email domain.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| - Read and follow [CONTRIBUTING.md](./CONTRIBUTING.md) before contributing to the SDK. It covers development setup, the example app, CI-aligned checks, and pull requests. | ||
| - Read and follow [RELEASING.md](./RELEASING.md) when adding change intents (`pnpm change`) or working on publishing. | ||
| - Before adding or changing public API, follow "Public API changes" in [CONTRIBUTING.md](./CONTRIBUTING.md): the API shape must be agreed on the issue first. For SDK design guidance, read https://posthog.com/handbook/engineering/sdks/guidelines.md. | ||
| - Public API changes (a diff in `api/public-api.json`): if the author is a PostHog maintainer (git email ends in `@posthog.com`), the PR is the discussion, so don't open or suggest an issue. Otherwise, follow "Public API changes" in [CONTRIBUTING.md](./CONTRIBUTING.md): if there's no agreed issue, stop and tell the user. If a PR already exists, add a public-API note to its description and draft an issue body for the user to post. Never open an issue yourself. For SDK design guidance, read https://posthog.com/handbook/engineering/sdks/guidelines.md. |
There was a problem hiding this comment.
The external-contributor instructions first say to stop whenever no agreed issue exists, even when a PR is already open. The next sentence and CONTRIBUTING.md instead say an open PR should continue with a note and drafted issue. An agent handling an existing public-API PR can therefore halt before reaching the intended non-blocking path. Make the stop condition explicitly apply only when no PR exists.
Prompt To Fix With AI
This is a comment left during a code review.
Path: AGENTS.md
Line: 5
Comment:
**Open PR guidance conflicts**
The external-contributor instructions first say to stop whenever no agreed issue exists, even when a PR is already open. The next sentence and `CONTRIBUTING.md` instead say an open PR should continue with a note and drafted issue. An agent handling an existing public-API PR can therefore halt before reaching the intended non-blocking path. Make the stop condition explicitly apply only when no PR exists.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
posthog-php-fork_curl Compliance ReportDate: 2026-09-21T14:39:14.371348+00:00
|
| Test | Status | Duration |
|---|---|---|
| Format Validation.Event Has Required Fields | ✅ | 36ms |
| Format Validation.Event Has Uuid | ✅ | 532ms |
| Format Validation.Event Has Lib Properties | ✅ | 535ms |
| Format Validation.Distinct Id Is String | ✅ | 534ms |
| Format Validation.Token Is Present | ✅ | 535ms |
| Format Validation.Custom Properties Preserved | ✅ | 535ms |
| Format Validation.Event Has Timestamp | ✅ | 534ms |
| Format Validation.Non Utc Event Timestamp Is Converted To Utc | ✅ | 535ms |
| Retry Behavior.Retries On 503 | ❌ | 5539ms |
| Retry Behavior.Does Not Retry On 400 | ✅ | 2538ms |
| Retry Behavior.Does Not Retry On 401 | ✅ | 2537ms |
| Retry Behavior.Respects Retry After Header | ❌ | 5541ms |
| Retry Behavior.Implements Backoff | ❌ | 15553ms |
| Retry Behavior.Retries On 500 | ❌ | 5543ms |
| Retry Behavior.Retries On 502 | ❌ | 5541ms |
| Retry Behavior.Retries On 504 | ❌ | 5539ms |
| Retry Behavior.Max Retries Respected | ❌ | 15549ms |
| Deduplication.Generates Unique Uuids | ✅ | 542ms |
| Deduplication.Preserves Uuid On Retry | ❌ | 5536ms |
| Deduplication.Preserves Uuid And Timestamp On Retry | ❌ | 10545ms |
| Deduplication.Preserves Uuid And Timestamp On Batch Retry | ❌ | 5545ms |
| Deduplication.No Duplicate Events In Batch | ✅ | 540ms |
| Deduplication.Different Events Have Different Uuids | ✅ | 535ms |
| Compression.Sends Gzip When Enabled | ✅ | 536ms |
| Batch Format.Uses Proper Batch Structure | ✅ | 534ms |
| Batch Format.Flush With No Events Sends Nothing | ✅ | 520ms |
| Batch Format.Multiple Events Batched Together | ✅ | 523ms |
| Error Handling.Does Not Retry On 403 | ✅ | 2534ms |
| Error Handling.Does Not Retry On 413 | ✅ | 2538ms |
| Error Handling.Retries On 408 | ❌ | 5539ms |
Failures
retry_behavior.retries_on_503
Expected at least 3 requests, got 1
retry_behavior.respects_retry_after_header
Expected at least 2 requests, got 1
retry_behavior.implements_backoff
Expected at least 3 requests, got 1
retry_behavior.retries_on_500
Expected at least 2 requests, got 1
retry_behavior.retries_on_502
Expected at least 2 requests, got 1
retry_behavior.retries_on_504
Expected at least 2 requests, got 1
retry_behavior.max_retries_respected
Expected 4 requests, got 1
deduplication.preserves_uuid_on_retry
Need at least 2 requests to check retry
deduplication.preserves_uuid_and_timestamp_on_retry
Expected at least 3 requests, got 1
deduplication.preserves_uuid_and_timestamp_on_batch_retry
Expected at least 2 requests, got 1
error_handling.retries_on_408
Expected at least 2 requests, got 1
Feature_Flags Tests
✅ 17/17 tests passed
View Details
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ✅ | 524ms |
| Request Payload.Flags Request Uses V2 Query Param | ✅ | 524ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ✅ | 525ms |
| Request Payload.Flags Request Omits Authorization Header | ✅ | 525ms |
| Request Payload.Token In Flags Body Matches Init | ✅ | 524ms |
| Request Payload.Groups Round Trip | ✅ | 524ms |
| Request Payload.Groups Default To Empty Object | ✅ | 523ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ✅ | 523ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ✅ | 524ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ✅ | 523ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 518ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 518ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ✅ | 527ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ✅ | 524ms |
| Retry Behavior.Retries Flags On 502 | ✅ | 625ms |
| Retry Behavior.Retries Flags On 504 | ✅ | 625ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ✅ | 535ms |
posthog-php-lib_curl Compliance ReportDate: 2026-09-21T14:39:25.170390+00:00 ✅ All Tests Passed!47/47 tests passed Capture Tests✅ 30/30 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
posthog-php-socket Compliance ReportDate: 2026-09-21T14:39:52.167539+00:00
|
| Test | Status | Duration |
|---|---|---|
| Format Validation.Event Has Required Fields | ✅ | 25ms |
| Format Validation.Event Has Uuid | ✅ | 521ms |
| Format Validation.Event Has Lib Properties | ✅ | 522ms |
| Format Validation.Distinct Id Is String | ✅ | 522ms |
| Format Validation.Token Is Present | ✅ | 522ms |
| Format Validation.Custom Properties Preserved | ✅ | 523ms |
| Format Validation.Event Has Timestamp | ✅ | 522ms |
| Format Validation.Non Utc Event Timestamp Is Converted To Utc | ✅ | 522ms |
| Retry Behavior.Retries On 503 | ❌ | 9230ms |
| Retry Behavior.Does Not Retry On 400 | ✅ | 2527ms |
| Retry Behavior.Does Not Retry On 401 | ✅ | 2524ms |
| Retry Behavior.Respects Retry After Header | ❌ | 9234ms |
| Retry Behavior.Implements Backoff | ❌ | 19237ms |
| Retry Behavior.Retries On 500 | ❌ | 8740ms |
| Retry Behavior.Retries On 502 | ❌ | 9235ms |
| Retry Behavior.Retries On 504 | ❌ | 9234ms |
| Retry Behavior.Max Retries Respected | ❌ | 19232ms |
| Deduplication.Generates Unique Uuids | ✅ | 44ms |
| Deduplication.Preserves Uuid On Retry | ❌ | 9232ms |
| Deduplication.Preserves Uuid And Timestamp On Retry | ❌ | 14239ms |
| Deduplication.Preserves Uuid And Timestamp On Batch Retry | ❌ | 9237ms |
| Deduplication.No Duplicate Events In Batch | ✅ | 531ms |
| Deduplication.Different Events Have Different Uuids | ✅ | 524ms |
| Compression.Sends Gzip When Enabled | ✅ | 522ms |
| Batch Format.Uses Proper Batch Structure | ✅ | 522ms |
| Batch Format.Flush With No Events Sends Nothing | ✅ | 518ms |
| Batch Format.Multiple Events Batched Together | ✅ | 513ms |
| Error Handling.Does Not Retry On 403 | ✅ | 2525ms |
| Error Handling.Does Not Retry On 413 | ✅ | 2524ms |
| Error Handling.Retries On 408 | ❌ | 5529ms |
Failures
retry_behavior.retries_on_503
Expected at least 3 requests, got 1
retry_behavior.respects_retry_after_header
Expected at least 2 requests, got 1
retry_behavior.implements_backoff
Expected at least 3 requests, got 1
retry_behavior.retries_on_500
Expected at least 2 requests, got 1
retry_behavior.retries_on_502
Expected at least 2 requests, got 1
retry_behavior.retries_on_504
Expected at least 2 requests, got 1
retry_behavior.max_retries_respected
Expected 4 requests, got 1
deduplication.preserves_uuid_on_retry
Need at least 2 requests to check retry
deduplication.preserves_uuid_and_timestamp_on_retry
Expected at least 3 requests, got 1
deduplication.preserves_uuid_and_timestamp_on_batch_retry
Expected at least 2 requests, got 1
error_handling.retries_on_408
Expected at least 2 requests, got 1
Feature_Flags Tests
✅ 17/17 tests passed
View Details
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ✅ | 522ms |
| Request Payload.Flags Request Uses V2 Query Param | ✅ | 520ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ✅ | 520ms |
| Request Payload.Flags Request Omits Authorization Header | ✅ | 521ms |
| Request Payload.Token In Flags Body Matches Init | ✅ | 520ms |
| Request Payload.Groups Round Trip | ✅ | 520ms |
| Request Payload.Groups Default To Empty Object | ✅ | 520ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ✅ | 521ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ✅ | 520ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ✅ | 520ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 515ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 510ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ✅ | 525ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ✅ | 522ms |
| Retry Behavior.Retries Flags On 502 | ✅ | 623ms |
| Retry Behavior.Retries Flags On 504 | ✅ | 624ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ✅ | 523ms |
dustinbyrne
left a comment
There was a problem hiding this comment.
The maintainer exemption looks good. One non-blocking clarification about identifying maintainers.
AI-assisted review: source inspection and existing CI; no new tests executed.
| - Read and follow [CONTRIBUTING.md](./CONTRIBUTING.md) before contributing to the SDK. It covers development setup, the example app, CI-aligned checks, and pull requests. | ||
| - Read and follow [RELEASING.md](./RELEASING.md) when adding change intents (`pnpm change`) or working on publishing. | ||
| - Before adding or changing public API, follow "Public API changes" in [CONTRIBUTING.md](./CONTRIBUTING.md): the API shape must be agreed on the issue first. For SDK design guidance, read https://posthog.com/handbook/engineering/sdks/guidelines.md. | ||
| - Public API changes (a diff in `api/public-api.json`): if the author is a PostHog maintainer (git email ends in `@posthog.com`), the PR is the discussion, so don't open or suggest an issue. Otherwise, follow "Public API changes" in [CONTRIBUTING.md](./CONTRIBUTING.md): if there's no agreed issue, stop and tell the user. If a PR already exists, add a public-API note to its description and draft an issue body for the user to post. Never open an issue yourself. For SDK design guidance, read https://posthog.com/handbook/engineering/sdks/guidelines.md. |
There was a problem hiding this comment.
Minor (non-blocking): Could we treat @posthog.com as one signal of maintainer status rather than the exclusive test? A confirmed Client Libraries maintainer can use a GitHub noreply address, which sends them through the external-contributor issue-first flow despite the exemption in CONTRIBUTING.md. Consider accepting independently confirmed maintainer identity too.
Problem
Port of PostHog/posthog-js#5044.
The "Public API changes" section in
CONTRIBUTING.mdand the matching line inAGENTS.mdsay the API shape must be agreed on an issue first. Nothing in either file exempts maintainers. So when a Client Libraries maintainer's agent changes public API in their own PR, it doesn't find an agreed issue and opens one. We end up closing duplicate issues and paying for the turns and tokens. For the team, the PR is where we discuss API shape.Changes
CONTRIBUTING.md: says the issue-first rule is for external contributors, and adds a path for contributors who already have a PR open. They call out the public API change at the top of the PR description and link or open an issue to discuss it, instead of stopping.AGENTS.md: agents skip the issue when the author is a PostHog maintainer (git email ends in@posthog.com). For contributors, agents stop and ask, or draft an issue body for the user to post. Agents never open an issue on their own.CONTRIBUTING.md, becauseAGENTS.mdnow covers it.Testing
Docs only. Same wording as posthog-js#5044, which was reviewed and approved.
🤖 Generated with Claude Code