Disclose detection reporting, and stop claiming it does not exist - #161
Conversation
detections.js ships in dist/, and the word "detection" appeared nowhere in AGENT-INSTALL.md. Agents npm pack the tarball and audit it before installing, so a capability in dist/ that the docs do not mention reads as misrepresentation — the install gets refused, and the refusal is right. Worse than an omission, one line was a claim that contradicted it: map --upload was described as the one opt-in that sends anything derived from your source, and a guard with reportDetections: true already sends route paths and the parameter names a rule reads. Turning that default on without fixing this would have shipped a false statement rather than a loose one. The new section states both reporting paths and the payload field by field, and says plainly what is excluded — matched values, bodies, headers, cookies, query-string values. Every claim was checked against detections.js rather than written from memory, including the conditions: a site UUID is required and PATCHSTACK_TELEMETRY=off disables it. Nothing caught this because nothing looks. The CI job named "Capability contract" checks that the committed manifest matches the map vocabulary in its source; it never reads documentation. So the disclosure is now a test: every outbound endpoint is read out of src/ and must be described in AGENT-INSTALL.md, including the ones built from a resolved base URL rather than a literal path — which is how the detection reporter is written, and therefore exactly the shape a check matching literal paths would have missed. Field test, --persona hostile --rounds 3: 2/3 rounds fully green. Round 1 refused, citing only the install prompt, and ran no commands at all, so it never read the shipped docs. Worth stating that the fixture installs the published package, so this run cannot exercise the new section either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Comprehensive docs and tests elegantly extend runtime guard reporting. 🎯 Quality: 100% Elite · 📦 Size: Medium 📈 This month: Your 108th PR — above team average · Averaging Excellent |
Review found the OAuth token exchange outside the guard: firewall-log.js posts to
${apiBase}/oauth/token and no extraction pattern matched it, so the file claimed
every outbound endpoint was disclosed while a credential-bearing request sat
outside the check.
The defect was the method, not the pattern list. Enumerating URL-building idioms
produces a scan that can confirm what it recognises and say nothing about the
rest — an incomplete inventory answering a completeness question, which is the
mistake this repo keeps finding in other layers. Widening the patterns to cover
oauth/token would have left the next idiom to be missed the same way.
So the polarity is inverted. Candidates are extracted broadly and every one must
be classified: an endpoint with documented prose, or explicitly not an endpoint
with a reason. Anything unrecognised fails the test. Applied, it immediately
surfaced three more real endpoints nobody had written down — the OAuth exchange,
the widget-settings lookup behind `status`, and the older get-rules path used
when the guard is configured with a token — plus the device-code flow, which was
already documented by showing the approval URL it prints.
Three scaffolder file paths match the same shape as a URL segment. They are
classified rather than filtered by a heuristic, because a rule that guessed which
template literals were URLs is precisely what let the real ones through.
The second finding: the runtime records every match before separately posting
enforced ones to the block log, so with reportDetections on, blocked matches go
to the detections endpoint too. The text said only non-blocking detections were
sent, which understated what leaves the app — the direction that matters, since a
reader deciding whether to enable this is owed the larger number.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0e6d7d8 to
5f440ed
Compare
|
Both P1s fixed in P1 — the OAuth exchange was outside the guardCorrect, and worse than one miss. Enumerating URL-building idioms produces a scan that can confirm what it recognises and say nothing about the rest — an incomplete inventory answering a completeness question, which is the same defect this repo keeps finding in other layers. Adding a pattern for So the polarity is inverted: candidates are extracted broadly, and every one must be classified — an endpoint with documented prose, or explicitly not an endpoint with a reason. Anything unrecognised fails. Applied, it immediately surfaced three more real endpoints with no mention anywhere in the shipped docs:
Plus the device-code flow, which turned out to be documented already by showing the approval URL the command prints. One small correction to the finding as written: the Three scaffolder file paths match the same shape as a URL segment. They're classified with reasons rather than filtered by a heuristic — a rule that guessed which template literals were URLs is exactly what let the real ones through. P2 — every match is reported, not only non-blocking onesCorrect. Mutation-checked, four ways — undisclosing Field test — the gate does not pass, and does not pass on
|
| state | green |
|---|---|
origin/main, no disclosure at all |
1/3 |
| this branch, first commit (earlier run) | 2/3 |
| this branch, first commit (later run) | 0/3 |
| this branch + these fixes | 0/3 |
The persona refuses between 33% and 100% of rounds on identical code. Every refusal is pre-install with 0 requests to the mock — no agent installed the package, so none of them read the shipped docs at all; the rationales cite only the prompt string (--yes, external account linking, and in one round a typosquat suspicion about the patchstack-connect bin name vs @patchstack/connect).
So this instrument cannot distinguish this diff from baseline, and main does not clear the gate today. That's a finding about the harness, not a verdict on this change — and whether it blocks merging is your call, not mine. Worth noting separately: the fixture installs the published package, so no run of it can exercise these doc edits until a publish.
Suite green (1272 passed, 6 skipped), typecheck and template typecheck clean.
The exclusion list read "the matched value, the request body, headers, cookies, or query-string values", which scans as "headers and cookies are not sent". That is true of their values and false of their names. ruleParameters returns each condition's `parameter` verbatim, and those carry the request region they refer to: post.title, get.redirect_to, cookie.session, server.HTTP_AUTHORIZATION. So a rule inspecting a cookie or an Authorization header does send that cookie's or header's name. The old sentence claimed a privacy boundary wider than the code's, in the direction that flatters us, which is the direction an auditing agent is right to punish. Now stated as two separate facts. The identifiers are sent and name their region, read from the rule's definition rather than from traffic, so they describe what is being screened rather than what any request held. And the exclusion is about values only: no value that matched, no request body, and no value of any header, cookie or query-string parameter, including the ones named. Guarded as an absence as well as a presence: a test fails on any exclusion clause that names headers or cookies without scoping to their values, because that is a sentence someone would write again while tightening the prose. One earlier assertion was matching a literal phrase rather than the claim, and this rewrite broke it — it now matches either phrasing while still requiring the substance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Now two separate statements instead of one blurred list:
Guarded as an absence as well as a presence: the test now fails on any exclusion clause naming headers or cookies without scoping to their values. That's the specific sentence someone would write again while tightening prose, so asserting its shape is worth more than asserting the current wording. Mutation-checked three ways — restoring your exact overclaim fails it, dropping the region examples fails it, removing the request-region statement fails it. One thing this caught in my own work: an earlier assertion matched the literal phrase Gates: 1273 passed / 6 skipped, typecheck and template typecheck clean. Field test re-run for this state: 1/3 green — the same as the |
Three rounds of review on this section found the same thing each time: the prose was wrong and the code was right. An endpoint nobody had written down, a trigger described as narrower than it is, and a privacy boundary claimed wider than the code's. The guards that existed assert that words appear. None of them reads the payload, so none could tell whether the words were true of it. This posts a detection through the real reporter, captures the serialized bytes, and asks two things of them. Every field emitted must be described. The check iterates the payload's own keys, so a field added to the report fails until someone writes it down — the failure names the key and says what to do. A vacuity control pairs with it, because iterating the payload's keys is satisfied perfectly by a payload with no keys: the documented set and the emitted set must be equal, not merely compatible. Every excluded value must actually be excluded. Sentinels are planted where the reporter could pick them up — a matched value, a request body, a header value, a cookie value, a query string — and asserted absent from the bytes. That proves the boundary rather than restating it. The query-string sentinel gets its own control: a reporter that sent no route at all would pass the exclusion while losing the field the disclosure describes. The rule used reads a cookie and an Authorization header on purpose. Those parameter names ARE sent, which is the distinction the wording got wrong, and a test that only looked for absent values would have agreed with the wrong version. Documentation stays hand-written. What is mechanised here is the inventory: generated prose would read as machine output to the agents who audit this file, and the wording is doing adversarial-UX work a serializer cannot do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
ENG-3651 filed and implemented in What it doesPosts a detection through the real Every emitted field must be described. It iterates the payload's own keys against a Every excluded value must actually be excluded. Sentinels are planted where the reporter could pick them up — matched value, request body, header value, cookie value, query string — and asserted absent from the bytes. That proves the boundary instead of restating it. Two controls, because both halves have a cheap wrong version
The rule it uses reads Mutation results
M1 is the realistic one — someone adds a field in good faith — and it trips three independent checks. Scope noteTest-only commit; Suite: 1278 passed / 6 skipped. Typecheck and template typecheck clean. |
Exemptions were keyed on the path string alone, so `rules` — a file the
scaffolder writes — was exempt everywhere. A future request built as
`${pulseBase}/rules/${uuid}` produces the same candidate from a different file
and would have passed as "not an endpoint": this file's own original mistake
repeated one level up, an exemption answering beyond the evidence that earned it.
Classification is now per occurrence. A candidate is exempt only in the files
that establish the exemption, disclosed only in the files that establish the
endpoint, and unclassified anywhere else. A meta-assertion keeps it honest by
refusing any bare-segment entry that does not name its files, since a rooted path
cannot collide with a filename but a bare segment can.
Making that change surfaced a second bug in the extraction. The interpolation
pattern required `${identifier}`, and the rules client builds its URL from
`${this.#baseUrl}` — a member expression the pattern could not see. So a real
endpoint was invisible here for the same reason the earlier version missed the
OAuth exchange: recognising one spelling of "a base URL" is not recognising a
base URL. Widened to any expression, which immediately turned up four endpoints
under their interpolated aliases, `rules` among them.
`rules` is now in both tables, and both readings are correct because each is tied
to its files: an endpoint in the client, a scaffolded file in the installer. That
collision is the concrete argument for the scoping.
The granularity is the file, not the call site, so a request added to a file that
already exempts the same word is still missed. Narrowing further needs parsing
rather than patterns; the residue is small because these exemptions live in
scaffolder modules that write files and make no requests. Recorded next to the
table rather than left for someone to discover.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in The scopingClassification is now per occurrence: a candidate is exempt only in the files that establish the exemption, disclosed only in the files that establish the endpoint, unclassified anywhere else. A meta-assertion refuses any bare-segment entry that does not name its files — a rooted path like The comparison that shows it matters:
The second bugTesting your scenario didn't fail at first, which turned out not to be a weak test. The interpolation pattern required Widened to any expression, it immediately turned up four endpoints under their interpolated aliases —
|
|
/review |
2 participants
ENG-3649
Prerequisite for turning
reportDetectionson by default, but a live problem on its own.Two findings
The detection reporter ships undisclosed.
detections.jsis indist/(the./protectentry imports it) and the word "detection" appeared zero times inAGENT-INSTALL.md. Agentsnpm packthe tarball and audit it, so a capability indist/the docs don't mention reads as misrepresentation — the install gets refused, and the refusal is correct.And one line actively contradicted it:
A guard with
reportDetections: truealready sends route paths and the parameter names a rule reads.field-test/README.mdlists "an overbroad privacy claim" as a refusal trigger alongside an undisclosed command. Flipping the default without fixing this would have shipped a false statement rather than a loose one.The change
A Runtime guard reporting section covering both paths, with the detection payload stated field by field — rule id, request path with the query string removed, parameter names, phase, enforced, bundle identifier, timestamp, plus the per-batch dropped count — and what it excludes: matched value, request body, headers, cookies, query-string values.
Every claim was checked against
detections.jsrather than written from memory, including the conditions I'd initially stated incompletely: a site UUID is required, andPATCHSTACK_TELEMETRY=offdisables it.The overbroad line is narrowed to what
map --uploadactually is — the only command that sends a description of your source — and points at the other path. The credential list gains the detections ingest.Why nothing caught it
The closest thing in CI is the "Capability contract" job, which checks that
capabilities.jsonmatches the map vocabulary in its TypeScript source. It never reads documentation.So
tests/endpoint-disclosure.test.tsnow reads every outbound endpoint out ofsrc/and fails when one isn't described inAGENT-INSTALL.md. It covers endpoints built from a resolved base URL rather than a literal path — which is how the detection reporter is written, and therefore precisely the shape a literal-path check would have missed.It matches on documented prose, not path segments, because
package-removedis correctly documented as "package removal"; the table of accepted phrasings is where adding an endpoint forces a conscious decision about disclosing it.Mutation-checked: removing every mention of detections fails it; introducing a new undisclosed endpoint fails it with the remedy in the message; dropping the payload specifics while keeping the mention fails it. Plus a vacuity control, since a pattern that stops matching would otherwise report every endpoint as disclosed while reading none.
Field test
node field-test/run.mjs --persona hostile --rounds 3— 2/3 rounds fully green, round 1 refused.I read the refusal rather than dismissing it: it cites only the install prompt (
--yes, external account linking, "I have vetted"), and the agent ran no commands, so it nevernpm packed or read the shipped docs. That's the hostile persona's baseline policy refusal, independent of this diff.Worth stating the documented limitation: the fixture installs the published package, so this run cannot exercise the new section either way. Validating it end-to-end needs a publish first. Whether 2/3 counts as passing the gate is your call, not mine.
Suite green (1270 passed, 6 skipped), typecheck and template typecheck clean.
Not in this change
The default flip. It becomes a small edit on top of this — the "off unless you pass
reportDetections: true" sentence — and needs its own field-test round.