Repository navigation
Send the cloud every Evidence value apart from the reserved names - #72
Merged
Merged
Conversation
…in the all evidence tests Evidence added before the cloud's evidencekeys answer arrives passes the engine's default filter, so the test of evidence the cloud does not list passed without the fix. Waiting for ready gives the engine the filter a running service has, and the test then fails without the fix.
…gine alone The Set Headers Element accepts every evidence key and the JavaScript Builder every query and header key, so in a default pipeline nothing is filtered out and the tests passed without the fix. With both left out, the engine's own filter decides, and the tests fail without the fix. The recording client also answers the accessible properties post a license key makes.
This was referenced Nov 11, 2026
Merged
… the set aside evidence
… Engine sends addQueryData keeps the first value it meets for a name, so the order of the sendable evidence decides which of two values with one name is sent. The accepted values now come first, and a set aside value never replaces an accepted one of the same key.
This was referenced Nov 11, 2026
sendableEvidence and addQueryData tested a name with in, which also finds the members every object has, so evidence named constructor, toString or valueOf was taken as already present and never sent. Both now ask whether the object has a member of its own by that name.
# Conflicts: # pipeline/fiftyone.pipeline.core/evidence.js # pipeline/fiftyone.pipeline.core/types/evidence.d.ts
rc's addQueryData now leaves out Evidence whose name is resource, license, licence or values, so isSent drops its own copy of that check and leaves out the web request view alone. What reaches the cloud is unchanged.
jwrosewell
marked this pull request as ready for review
November 11, 2026 14:45
This was referenced Nov 11, 2026
Merged
Draft
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
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.
Problem
A Cloud Request Engine on a cloud-only pipeline never sees most of the
Evidence a caller added, because the Evidence store throws it away before
processing starts.
pipeline/fiftyone.pipeline.core/evidence.js,add, lines 137 to 160 onrc at a8f1c227,
walks the pipeline's elements, sets
keeponly where some element'sevidenceKeyFilter.filterEvidenceKey(key)is true, and writesthis.evidenceStore[key] = valueon line 153 only insideif (keep). Theelse branch on lines 154 to 159 logs "filtered out of evidence. Not added."
and the value is gone.
On a cloud-only pipeline the only element is the Cloud Request Engine, and
its filter is a
BasicListEvidenceKeyFilterbuilt from the cloud'sevidencekeysanswer, set ingetEvidenceKeysatcloudRequestEngine.js line 496.
So a key the cloud has never heard of is thrown away before the engine can
read it, and
getContentatline 522,
which reads
flowData.evidence.getAll(), cannot see it either.getContentitself was already right. The loss happens earlier.What is lost depends on the other elements in the pipeline. The Set Headers
Element has no filter of its own, so it keeps the base filter, which accepts
every key, and the JavaScript Builder's filter accepts every
query.andheader.key. So a pipeline built byPipelineBuilderwith its defaultskeeps everything and sends it. A pipeline built with
useSetHeaderProperties: falseloses thecookie.evidence and any otherprefix the cloud does not list, and one built with
addJavaScriptBuilder: falseas well loses every key the cloud does notlist. What the cloud sees therefore changes with elements that have nothing
to do with the request, which the fix below removes.
The decision
James Rosewell, 10 November 2026.
What the specification says
The Processing section of
cloud-request-engine.md at e89679e9,
the specifications
rchead that holdsspecifications#138,
merged on 11 November.
The same page also says the accepted Evidence "is what the Engine advertises
to the Pipeline, for example for the web
Varyheader, and it does not limitthe Evidence the Engine sends".
Why the Evidence key filter was not widened
The obvious change is to make the Cloud Request Engine's
evidenceKeyFilteraccept every key, soaddkeeps everything. That wasnot done, for two reasons.
First, the filter is what the Engine advertises to the Pipeline. A web
integration reads it for the
VaryandETagheaders, andweb-integration.md
lets a web integration add only the accepted values, so widening the filter
would silently widen what a web integration sends from the browser. Web
integrations are unchanged by this work.
Second, other things read the accepted Evidence. An engine's cache key is
evidenceKeyFilter.filterEvidence(flowData.evidence.getAll())(
pipeline/fiftyone.pipeline.engines/engine.jslines 91 and 128), usagesharing walks
flowData.evidence.getAll()directly(
pipeline/fiftyone.pipeline.engines.fiftyone/shareUsage.jsline 186), andthe JavaScript Builder filters
getAll()for the values it writes into thescript (
pipeline/fiftyone.pipeline.core/javascriptbuilder.jsline 335).Widening the filter, or widening
getAll(), would change all three at once.So the filter stays as it is,
getAll()stays the accepted Evidence alone,and the Engine reads the set aside Evidence as well when it builds its
request.
One consequence of keeping the cache key as it is. Two requests that differ
only in Evidence the cloud does not list now post different bodies but share
one cache key, so the second is served the first's answer, which could not
happen before because the two bodies were the same. That follows the
Caching page at e89679e9,
which builds the key from the Evidence each element advertises.
The cloud lists the Evidence it reads, and the rest is sent so the cloud can
learn from it, as the decision above says, so the key stays the advertised
Evidence.
What changed
Three files.
pipeline/fiftyone.pipeline.core/evidence.js.addnow keeps a keyno element accepts in a second store,
unacceptedStore, rather thanthrowing it away, and a new
getUnaccepted()gives it back.getAll()is unchanged and still returns the accepted Evidence alone, so the cache
key, usage sharing and the JavaScript Builder behave exactly as before.
The two stores are kept apart, so a key is in one or the other and never
both. The debug log line for a key no element accepts now says it was set
aside rather than not added.
pipeline/fiftyone.pipeline.cloudrequestengine/cloudRequestEngine.js.getContentreads a newsendableEvidence(evidence)rather thanflowData.evidence.getAll().sendableEvidencemergesgetAll()andgetUnaccepted(), with the accepted value winning, and leaves out anythingisSent(key)refuses, which isserver.http-requestin any letter case.The reserved names,
resource,license,licenceandvaluesunder anyprefix in any letter case, are left out by
addQueryData, through thecheck node#61 put
there.
sendableEvidencereadsgetUnacceptedonly where the method exists, so an olderfiftyone.pipeline.corestill works and simply sends what it has.types/*.d.tsfiles, so the published types match.Checks made
addhas two branches andboth now write, one to each store.
sendableEvidencehas one exit andreturns an empty object where there is no evidence, which is what
getContentmade of an empty store before.isSenthas one exit, falsefor the web request view and true for anything else.
getContentis unchanged below its first line.
CloudRequestEngine.processInternalcallsgetContentatline 412
on every data request, and sets
resource,licenseandvalueson thebody after
getContenthas run, so the engine's own values are the onessent.
Evidence.addis reached fromaddObject, fromaddFromRequest(line 285) and from any caller adding evidence by hand.
sharing, the engine cache key and the JavaScript Builder all read
getAll(), which is unchanged.Evidence.get(key)still reads theaccepted store alone, and every caller of it asks for a key its own
element accepts, so no caller sees a new value.
FlowData.getEvidenceDataKeyfilters
getAll()and is unchanged. The JSON Builder reads element dataand no evidence store.
whose filter is a
BasicListEvidenceKeyFilterover a cloudevidencekeysanswer of
header.user-agentandquery.user-agent, nine keys added.getAll()holdsheader.user-agentalone,getUnaccepted()holds theother eight,
sendableEvidencegives the four that may be sent, and thebody the engine would post is
51D_experiment,sec-ch-ua-platform,user-agentandmy-own-thing, with no reserved name and nohttp-request. That run uses the realEvidence,BasicListEvidenceKeyFilterandCloudRequestEngineclasses with nonetwork call, so it shows the store and the body and not the wire format.
The full test run on a runner is below.
Tests
pipeline/fiftyone.pipeline.cloudrequestengine/tests/cloudAllEvidence.test.jsis new. It builds a pipeline holding the engine alone, with
addJavaScriptBuilder: falseanduseSetHeaderProperties: false, over arequest client that keeps each post, and waits for the engine to be ready
before adding evidence, so the engine's own filter is the cloud's key list
and nothing else accepts anything. Six tests.
query.my-own-thing,header.sec-ch-ua-platformandcookie.51D_experimentare all in the body, none of which the stand-incloud lists.
header.User-Agent, which the cloud does list, is still sent.name, whether the cloud lists the name or not.
resource,license,licenceandvaluesfrom Evidence are never inthe body, in any case under any prefix, and the engine's own resource key,
licence key and property list are.
server.http-requestis never in the body.The tests came first, in
cef0322, with the fix in45e82ce. The firstred run of
cef0322,run 42980282522,
failed only the reserved name and web request tests, because the evidence
was added before the cloud's
evidencekeysanswer arrived and the defaultpipeline's other elements accepted every key, so nothing was filtered out.
58dc2d5makes the test wait for the engine to be ready, and02a11ccleaves the other elements out and answers the accessible properties post a
license key makes.
Every temporary run below sits in one workflow file shared by all the
temporary runs, so its page carries the title "Temporary red run" and the
name of the first branch that used the file. Each run's own branch is
named here.
on branch
tmp/node-cloudev-red3, the test file at02a11ccon therccode at
c32a2905. 5 of 6 fail, the firstbecause the three keys are thrown away in
addand the body carries noneof them. The one that passes, a reserved name under any prefix and case,
passes because on
rcthose keys are thrown away as well.on branch
tmp/node-cloudev-green2, the same file on02a11cc, 6 of 6pass.
addQueryDatakeeps the first valueit meets for a name, so
sendableEvidencenow takes the accepted valuesfirst, and a set aside value never replaces an accepted one of the same
key. Red, run 42994362197
on branch
tmp/node-cloudev-red4, the test commite8c048b, where thenew test fails and the other 6 pass. Green,
run 42994380869
on branch
tmp/node-cloudev-green3, the head41809f6, 7 of 7 pass.41809f6, success, run 42996681660. Configure and the Ubuntu, Ubuntu ARM and Windows jobs on Node 22 and 24 pass, 1004 of 1006 tests with 2 skipped in each, Configure printed "Left out 2 macOS entries", and all 7Setting 'PullRequestSha'lines are41809f61c66dc3fe8c8f0a9381601f54bd153315.sendableEvidenceandaddQueryDatatested a name within, which also finds the members every object has, so evidence namedconstructor,toStringorvalueOfwas taken as already present and not sent. Both now ask whether the object has a member of its own by that name. Red, on branchtmp/node-72-red5, run 43024737099, the test commitad1def7, where the new test fails and the other 7 pass. Green, on branchtmp/node-72-green4, run 43024752879, the head82df729with this file andcloudRequestEngine.test.js, 29 of 29.82df729was cancelled once Read and store evidence keys without regard to letter case #71 and Never send resource, license, licence or values from Evidence to the cloud #61 merged, as the draft was about to takerc.After node#71 and node#61 merged:
2cfe69fmergesrcatb9a4d29. The set aside store is now keyed through Read and store evidence keys without regard to letter case #71's lower-case table, so two spellings of one key no element accepts are one entry, under the first spelling.getstill reads the accepted store alone, and asks whether that store holds the key itself, because the table now names set aside keys too.a3eaf6dtests that two spellings of one set aside key are sent once. Red, on branchtmp/node-72-red6, run 43033990673, the test file on82df729's code, where the new test fails and the other 8 pass. Green, on branchtmp/node-72-green5, run 43034023473, the head with this file,cloudRequestEngine.test.jsandevidenceKeyCase.test.js, 37 of 37.e587c40drops this draft's own reserved name check fromisSentin favour of Never send resource, license, licence or values from Evidence to the cloud #61's inaddQueryData. What reaches the cloud is unchanged, and the reserved name tests in this draft still pass against Never send resource, license, licence or values from Evidence to the cloud #61's check in the green run above.e587c40, success, run 43034641588. Configure and the Ubuntu, Ubuntu ARM and Windows jobs on Node 22 and 24 pass, 1188 of 1190 tests with 2 skipped in each, Configure printed "Left out 2 macOS entries", and all 7Setting 'PullRequestSha'lines aree587c40a8dcf33a5f13885dca18574c0410b1f79.rchas since taken a README change only, and the draft still merges cleanly.What remains
which is James Rosewell's decision quoted above, that a Cloud Request
Engine sends every Evidence value apart from the reserved names.
macOS was left out of this draft's full build under the 9 November decision.
The rc head is built with macOS before it merges into main.