Scope the extract gate to subqueries and correct the suppression doc

Suppression matches a record's own `name` field, so a projection that
filters on `name` without returning it carries an upstream value through.
The README claimed the record was always dropped on every query shape.
State the rule the code implements and pin the shape with a test.

`hasExtract` exempted only `in`. openvoxdb's `valid-operator?`
(src/puppetlabs/puppetdb/query_eng/engine.clj:2779-2784) lists `subquery`
separately, and the AST-rewrite stage (:2111-2123) expands
["subquery" entity expr] into ["in" cols ["extract" cols ["select_x" expr]]]
before any plan node is built, so its operand is projected into a subquery
exactly like `in`'s (:2705-2712). Exempt `subquery` and the explicit
`select_<entity>` forms (:1889-1911).

Signed-off-by: unkin-agent <unkin-agent@unkin.net>

README.md

@@ -114,22 +114,26 @@ attributes a shared node to the first backend in configured order, while
 `/nodes` always attributes it to the backend holding the newer
 `report_timestamp`. Each answer describes the record it is attached to.
 
-While the fact is enabled the configured name is `pdbmux`'s alone. A `/facts`
-record of that name coming from a backend is **always dropped**, on every query
-shape — including the shapes below, where nothing is injected in its place — so
-the fact means exactly one thing and a node never carries two of it. Each request
-that drops one logs it once. Only `source_fact_enabled: false` restores upstream
+While the fact is enabled, any `/facts` record whose own `name` field equals the
+configured name is dropped, on every query shape — including the shapes below,
+where nothing is injected in its place. The rule reads the record, not the query,
+so a projection that filters on `name` without returning it — say
+`["extract",["certname","value"],["=","name","pdbmux_source"]]` — produces rows
+that no longer identify themselves, and an upstream value of that name comes
+through. Ask for the `name` column and the guarantee holds. Each request that
+drops a record logs it once. Only `source_fact_enabled: false` restores upstream
 records of that name; rename the synthetic fact via `source_fact` if the real one
 matters more.
 
 **Injection is skipped**, and no synthetic record is added, when:
 
-- the query contains an `extract` anywhere outside an `in` subquery — it projects
-  a column subset, and with a `["function", ...]` column it aggregates. Injecting
-  there would break the row shape or silently inflate a `count()`, so **aggregate
+- the query contains an `extract` outside a subquery — it projects a column
+  subset, and with a `["function", ...]` column it aggregates. Injecting there
+  would break the row shape or silently inflate a `count()`, so **aggregate
   results are never changed**. The whole query is walked, so an `extract` nested
-  under `and`/`or`/`not`/`from` skips injection too; an `extract` inside an `in`
-  operand projects the subquery rather than the response, so it does not;
+  under `and`/`or`/`not`/`from` skips injection too; an `extract` under `in`,
+  `subquery`, or `select_<entity>` projects that subquery rather than the
+  response, so it does not;
 - the query is not an AST array — every **PQL-syntax** query (`facts { certname
   = "web1" }`) lands here. `pdbmux` cannot tell what such a query projects, so it
   never injects into a PQL response. Use the AST form to get the fact;

source.go

@@ -3,6 +3,7 @@ package main
 import (
 	"encoding/json"
 	"log"
+	"strings"
 )
 
 // sourceInjector owns the configured fact name for one request. A nil
@@ -25,9 +26,10 @@ func (s *Server) newSourceInjector(query string, factEntity bool) *sourceInjecto
 	return &sourceInjector{name: s.cfg.SourceFact, inject: injectable(query, factEntity)}
 }
 
-// claims reports whether an upstream record carries the name pdbmux owns. While
-// the feature is enabled the name means one thing on every query shape, so such
-// a record is dropped even when the query gate has ruled synthesis out.
+// claims reports whether an upstream record carries the name pdbmux owns, and is
+// keyed on the record's own name field: a projection that omits the name column
+// yields records that cannot be identified, so they pass through. Suppression
+// does not depend on the query gate.
 func (si *sourceInjector) claims(factName string) bool {
 	return si != nil && factName != "" && factName == si.name
 }
@@ -95,7 +97,8 @@ func (si *sourceInjector) stamp(raw json.RawMessage, backend string) json.RawMes
 //     pdbmux cannot tell what it projects, so it changes nothing;
 //   - an `extract` anywhere in the query's own projection scope, which projects a
 //     column subset and, with a `["function", ...]` column, aggregates — injecting
-//     there would corrupt the row shape or silently inflate a count();
+//     there would corrupt the row shape or silently inflate a count(). Operands
+//     scoped to a subquery are excluded; see hasExtract;
 //   - on the facts entity, any outer constraint on `name`, which selects
 //     specific facts. Subquery operands are not descended into: they choose which
 //     nodes match, not which facts come back.
@@ -125,11 +128,18 @@ func injectable(query string, factEntity bool) bool {
 // projection scope. openvoxdb accepts an extract as an operand of a boolean
 // operator — engine.clj's user-node->plan-node sends every and/or/not operand
 // back through itself (src/puppetlabs/puppetdb/query_eng/engine.clj:2697-2733)
-// and valid-operator? lists "extract" (:2780-2784) — so the row shape can be
+// and valid-operator? lists "extract" (:2779-2784) — so the row shape can be
 // rewritten below the top level, and the whole tree is walked to fail closed.
-// `in` is the one operator not descended into: its operand becomes
-// InExpression's :subquery (:2705-2712), projecting the subquery rather than
-// the response.
+//
+// Operators whose operand is scoped to a subquery are not descended into,
+// because an extract there projects the subquery rather than the response:
+//
+//   - `in`, whose operand becomes InExpression's :subquery (:2705-2712);
+//   - `subquery`, which the AST-rewrite stage expands into
+//     ["in" cols ["extract" cols ["select_<entity>" expr]]] (:2111-2123)
+//     before any plan node is built;
+//   - `select_<entity>`, the explicit subquery form (:1889-1911), reachable
+//     only under one of the two above in a query openvoxdb accepts.
 func hasExtract(parts []json.RawMessage) bool {
 	if len(parts) == 0 {
 		return false
@@ -138,10 +148,10 @@ func hasExtract(parts []json.RawMessage) bool {
 	if json.Unmarshal(parts[0], &op) != nil {
 		return false
 	}
-	switch op {
-	case "extract":
+	switch {
+	case op == "extract":
 		return true
-	case "in":
+	case op == "in", op == "subquery", strings.HasPrefix(op, "select_"):
 		return false
 	}
 	for _, p := range parts[1:] {

source_test.go

@@ -302,6 +302,32 @@ func TestHandler_SourceDisabledKeepsUpstreamFact(t *testing.T) {
 	}
 }
 
+// Suppression matches a record's own name field, so rows from a projection that
+// filters on name without returning it are not self-identifying and pass
+// through. Pinned as a known limit of the guarantee, and documented as one.
+func TestHandler_ProjectionWithoutNameColumnCarriesUpstreamValue(t *testing.T) {
+	a := newFakeBackend(t, `[`+node("h1", "2026-07-20T00:00:00Z")+`]`, `[]`)
+	b := newFakeBackend(t, `[]`, `[]`)
+	a.bodies[factsPath] = `[{"certname":"h1","value":"upstream-value"}]`
+	var buf bytes.Buffer
+	srv := NewServer(testConfig(a.srv.URL, b.srv.URL, mergeStatic), log.New(&buf, "", 0))
+
+	rec := doGet(t, srv.Handler(), factsPath,
+		`["extract",["certname","value"],["=","name","`+defaultSourceFact+`"]]`)
+	if rec.Code != http.StatusOK {
+		t.Fatalf("status %d: %s", rec.Code, rec.Body.String())
+	}
+	if got := rec.Body.String(); !strings.Contains(got, "upstream-value") {
+		t.Errorf("unidentifiable row was dropped: %s", got)
+	}
+	if strings.Contains(buf.String(), "dropped") {
+		t.Errorf("a row with no name field was counted as suppressed: %s", buf.String())
+	}
+	if _, n := sourceValues(t, rec.Body.Bytes(), defaultSourceFact); n != 0 {
+		t.Errorf("gated projection gained %d synthetic records: %s", n, rec.Body.String())
+	}
+}
+
 // Covers only that an aggregate /facts response gains no synthetic record and no
 // rewritten row. It does not cover whether the aggregate rows are correct:
 // /facts has no parseAggregate branch, so its rows take the certname merge
@@ -427,6 +453,11 @@ func TestInjectable(t *testing.T) {
 		{"nested extract on nodes", `["and",["extract",["certname"]]]`, false, false},
 		// An extract inside an `in` operand projects the subquery, not the response.
 		{"extract under in stays injectable", `["in","certname",["extract",["certname"],["select_facts",["=","name","os"]]]]`, true, true},
+		// `subquery` is rewritten to ["in" ... ["extract" ... ["select_x" ...]]]
+		// before any plan node is built, so its operand is subquery-scoped too.
+		{"subquery operand stays injectable", `["and",["=","certname","h1"],["subquery","facts",["extract",["certname"],["=","name","os"]]]]`, true, true},
+		{"bare subquery stays injectable", `["subquery","facts",["extract",["certname"]]]`, true, true},
+		{"extract under select_facts stays injectable", `["and",["select_facts",["extract",["certname"]]]]`, true, true},
 		{"nodes name filter is not a fact filter", `["=","name","os"]`, false, true},
 		{"nodes extract", `["extract",["certname"]]`, false, false},
 		{"unparseable query", `not json`, true, false},