Fix an unresolved import, and declare the reporting capability - #157
Merged
Conversation
**The fix first, because it is on main.** `runtime.js` calls `createDetectionReporter` and never imported it, so `reportDetections: true` threw `ReferenceError` at boot. Being opt-in, nobody reached it — including the tests: they exercised the reporter directly, or asserted that nothing is posted when the feature is OFF, and neither path enters the branch that builds it. A feature that only fails for whoever turns it on, guarded by tests that never turn it on. Closed with the test that was missing: switch reporting on, fire a matching request, and assert a detection reaches the endpoint. Removing the import fails it, which is what the previous suite could not do. **Then the capability.** Detections are sent only when a rule fires, so silence at the server means one of three things — nothing matched, reporting is off, or reports are not arriving — and nothing distinguished them. The rules fetch now carries `X-Patchstack-Detections: enabled` when reporting is on: a request that is already periodic, already authenticated, and already identifies the site. No new outbound path and no request data. A capability, not a heartbeat. The header says "this guard reports"; the server records when IT saw that. A client-supplied "alive as of" would be a value from outside dressed as an observation, and a wrong or stale clock is exactly what it would fake. Asserted in both directions: a guard with reporting on says so, and a guard without it stays silent. The second matters as much — a declaration that is sent regardless tells the server nothing. 1252 tests, typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Well-explained addition of a reporting capability with comprehensive test coverage and clear structure. 🎯 Quality: 100% Elite · 📦 Size: Medium 📈 This month: Your 104th PR — above team average · Averaging Excellent |
The header went out whether or not a bearer token resolved. The rules endpoint still accepts a bare UUID while token auth is optional, so on that path this was an assertion anyone holding the UUID could make — and it asserts the reassuring thing: that reporting is on. A dashboard would then show a site as covered because a stranger said so, which is the worst direction for a wrong answer to point. Fetching rules must never hinge on getting a token — protection comes first, and that is unchanged. Claiming a capability may: an unauthenticated request is one whose statements about a site carry no weight, and there is nothing to lose by withholding a claim. The server will gate on a verified token as well. A client-side check only removes the ACCIDENTAL case — a guard on the legacy path telling the truth about itself — because the forgeable case is not the client's to prevent. Both halves are needed and neither is sufficient. Also fixed a test that was passing for the wrong reason: the positive case supplied no credential, so it had been asserting the header on an unauthenticated request. It now exchanges a real one and checks that the claim travels with `Bearer`, which is what the gate is about. Mutation-checked: claiming it regardless of the credential fails the unauthenticated test and nothing else. 1253 tests, typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… a guarantee The comment asserted that the server verifies a token before accepting this header. Server-side verification of the rules fetch is configurable, and nothing consumes the header yet, so the sentence claimed a present guarantee where there is a requirement on whatever eventually reads it. State the requirement instead: a client-side gate removes the accidental case only, and a consumer has to verify the token itself before believing the claim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/review |
daniloradovic
approved these changes
Aug 20, 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.
Two things, and the first is a bug on
mainright now.reportDetections: truethrows on mainruntime.jscallscreateDetectionReporterand never imports it. The feature is opt-in, so nobody reached the failure — including the tests: they exercised the reporter directly, or asserted that nothing is posted when the feature is off. Neither path enters the branch that builds the reporter.A feature that only breaks for whoever turns it on, guarded by tests that never turn it on. It would have surfaced the moment the default was flipped, which was the next planned change.
Closed with the test that was missing: switch reporting on, fire a matching request, assert a detection reaches the endpoint. Removing the import fails it — which is precisely what the previous suite could not do.
Declaring the capability
Detections are only sent when a rule fires, so silence at the server means one of three things — nothing matched, reporting is off, or reports are not arriving — and nothing distinguished them. A dashboard reading that silence has to guess, and guessing wrong means telling a correctly configured customer to enable something already on.
The rules fetch now carries
X-Patchstack-Detections: enabledwhen reporting is on. That request is already periodic, already authenticated, and already identifies the site: no new outbound path, and no request data.A capability, not a heartbeat. The header says "this guard reports"; the server records when it saw that. A client-supplied "alive as of" timestamp would be a value from outside dressed up as an observation, and a wrong or stale clock is exactly what it would fake.
Asserted in both directions — a guard with reporting on says so, and a guard without it stays silent. The second matters as much as the first: a declaration sent regardless tells the server nothing.
Verification
1252 tests, typecheck clean. Mutation-checked: removing the import fails the two tests that enter the enabled path, and nothing else.