ADR-0030: Review ingestion spawns gh, over public allowlisted repositories only
- Status: proposed
- Date: 2026-09-05
- Deciders: Theurian maintainers
- Requirements: FR-V1, FR-V5, FR-V6, SEC-9, SEC-10, SEC-11, SEC-15, SEC-19, T-3, T-7
- Decision recorded in #479, the Milestone 8 design-first step for the FR-V GitHub-API arm
- Situates against ADR-0004 — whose Never Git-tracked (derived) list named "raw GitHub review caches", and which this ADR amends in place: decision 3 withdraws that entry and adds a fourth category, ingested content that is authored upstream and not rebuildable — ADR-0013 (nothing ingested here becomes approved knowledge), ADR-0019 (ingested labels are data, not governance), ADR-0026 (a served review thread is evidence; it gates nothing), ADR-0027 (the pre-landing secret scan this one mirrors), and ADR-0029 (the git-native floor of FR-V, which named this arm as explicitly not its own)
This ADR records a decision and ships no code. The adapter, the environment
construction, the caps, the version floor, the evidence files, the scan gate, the
serving store, review.search, the capability flag, the T-7 rewrite and the
prose-population sweep are all deferred to the slices named in Slicing plan and
Compliance. Nothing here modifies SCHEMA_VERSION, INDEX_SCHEMA_VERSION,
FINDINGS_SCHEMA_VERSION, any *.schema.json, or any *.py; the diff is
confined to docs/.
Two measurement frames, kept apart on purpose.
- The measurements that needed the network are quoted, never re-run here —
this document's own change performs no network call at all. Two sources, each
named where it is used, both on 2026-09-05 against
gh2.86.0:
| Source | Members |
|---|---|
| The design consult | runs A, B, C (decision 1); the PullRequestReviewThread schema introspection (decision 5); the configuration-key observation, http_unix_socket versus api_host (decision 1); the last-40-merged thread count (Dispositions) |
| Round one's adversarial review | runs D, E, F (decision 1), of which D was re-run by the orchestrator and reproduced; the by-PR-number thread population (Dispositions), also re-run by the orchestrator |
They say what was true of that binary and that repository on that date. Slice 1
re-runs A–F as driving tests, which is where they stop being quotations and
become controls.
2. Every repository measurement was taken on 2026-09-05 against origin/main
@ 1fe3302b, and each names the command that produced it. Where a
measurement is a population, the key is stated beside the number so a reader
can attack the key and not only the count.
Context
ADR-0029 built the git-native
floor of FR-V: Review-Finding: trailers parsed out of local history, landed in
their own store, served as review.findings. It bounded itself explicitly — "the
GitHub-API arm (threads, inline comments, resolution state, CI results, LLM
candidate generation) is broader, needs credentials, and is not this ADR's to
design". This ADR is that arm's design.
What exists today, measured. The domain model is built and nothing fills it:
| Fact | Command (2026-09-05, origin/main @ 1fe3302b) |
Result |
|---|---|---|
The ReviewProvider GitHub adapter |
cat packages/theurian-core/src/theurian/infrastructure/github/__init__.py |
a package docstring that says "Not implemented. This package holds no adapter and no HTTP client" |
| The capability the server publishes | grep -n 'reviewIngestion' packages/theurian-core/src/theurian/mcp/tools.py |
"reviewIngestion": False (line 1905) |
| The allowlist key | git grep -n 'providers\.review\.repositories' -- packages/theurian-core/src |
2 hits, both in security/project_config.py's own docstring prose (:5, :11) — no reader anywhere |
| The tool names already published as planned | the planned-tools table in docs/protocol/mcp-tools.md, lines 352–358 (command and output in Compliance) |
7 rows — review.findings shipped, six planned, review.search among them |
| Places in the shipped package that can start another program | the pinned set in tests/unit/test_network_call_sites.py (PROCESS_SPAWN_SITES) |
3: cli/context.py, infrastructure/git/trailer_source.py, infrastructure/services/runner.py — none of them reaches a network |
T-7 has had the wrong owner twice, and the change that ends that is slice 1 —
not this document, which ships nothing and can only name the owner.
docs/security/threat-model.md records three controls for SSRF — a scheme
allowlist, private-network rejection, and the repository allowlist in
.theurian/config.yaml — and none of the three is built.
#129 was closed having
corrected the entry's wording; the audit then repointed the controls at
#368, and review found that
368 builds no fetch path either. The threat model states the lesson in its own
words: "an owner has to be the change that would implement the control, and an
epic in the right milestone is not automatically that."
#429 now holds the three
against whatever first performs an external fetch. Slice 1 is that change for
the gh activation context, where one of the three is discharged and one is
reduced with a recorded residual; Dispositions states which is which, in
which context, and what stays #429's.
What stands in for the controls today is an absence, and the first spawn site
deletes it. tests/unit/test_network_call_sites.py pins, by equality against
the whole set, that nothing in the shipped package outside three fixed-argument
spawn sites and the daemon's own loopback health probe can open a connection.
Its process-spawn arm was added precisely because a gh api adapter "would
contain no client module at all" — a mutation that replaced a fetch with
subprocess.run(["curl", ...]) survived the entire suite with the network
enumeration green. That file states the admission checklist this ADR has to
satisfy, and it is quoted here rather than paraphrased — the source is a
concatenated Python string, joined here into prose with its -- set as an em
dash and nothing else changed (test_network_call_sites.py:653-661):
If you added a site, establish before listing it: the argument vector is fixed by the adapter rather than taken from a document or a configuration file; the command cannot be handed a URL or a remote; there is a timeout; and a test goes red when any of those stops holding. If the command is meant to reach the network — the Milestone 7
gh apishape — then the repository allowlist is due in the same change, along with the documents that currently promise nothing fetches: docs/security/threat-model.md (T-7) and the infrastructure/github/ package docstring.
So the design problem is not "how do we call GitHub". It is: the moment a spawn site is admitted, an absence that many documents rest on stops being true, and something has to hold what the absence was holding. Decision 1 is the replacement, stated in the positive.
Decision
1. GitHub is reached by spawning gh api graphql, and the absence control is replaced by a positive invariant
The adapter reaches GitHub by spawning the operator's gh binary as an argument
vector. The absence control retires at the commit that adds the site, and the
ten clauses below take its place. Each clause is a property, and each owes a
test that goes RED when it stops holding — an invariant with no test is what
the absence was already better than.
| # | The rule | What it holds | Owed test, and its slice |
|---|---|---|---|
| 1 | Exactly one module may reach GitHub. The spawn site lives in infrastructure/github/, and the pinned spawn-site set grows by exactly that one entry. |
A second fetch path added later on a page nobody re-reads; the equality pin catches an addition and a removal. | The existing PROCESS_SPAWN_SITES equality assertion, extended by one entry, plus a test that no other module in the shipped package names the adapter's spawn helper. Slice 1. |
| 2 | The endpoint is the literal graphql. Repository identity travels as typed GraphQL variables, never in the URL position. |
The gh api <path> form interpolates caller data into a path; the GraphQL form has no path segment an owner or repo name can escape into. It also means no raw URL exists for T-7's scheme allowlist to be needed on. |
A test that the argument vector's endpoint element equals graphql byte-for-byte, and that no element is derived by string-formatting a repository name. Slice 1. |
| 3 | The destination host is pinned by an explicit --hostname github.com. |
An inherited GH_HOST silently moving the request to another host — measured to move it (run B). |
A test asserting the flag and its value are present in every spawned vector. Slice 1. |
| 4 | The child environment is CONSTRUCTED from a closed enumerated constant (env={...} passed to the spawn), never inherited and never merely scrubbed. The membership is the table below, derived from its own Value column; the one open platform question — a Linux credential-store member — is reconciled there by a slice-1 CI measurement, not left to an implementer's judgement. |
The measured attack class below: destination and identity taken from inherited environment. A scrub is a blocklist, and a blocklist has to be right about every variable gh and its transport stack read; a constructed environment has to be right about the few Theurian deliberately passes. |
(i) an equality test — the child's environment mapping equals the expected mapping written out test-side, not imported. Membership is by key and value, and an empty string is a present key, not an absent one: gh treats an empty config-locating variable as absent and falls through (decision 1, member (d)), so the two are not interchangeable and the test pins which one the constant carries. It is a strong assertion: it goes RED for every wrong mapping, including a passed-through parent variable, a missing member and a wrong value, and round two's adversarial review drove it RED five ways. The test-side restatement is what makes it killable, and this is the correction of an inverted claim: a test that reads the production constant moves with it, so mutating the constant changes both sides and the test SURVIVES — measured by round three's adversarial review with toy models, 2026-09-05. The killable forms are a test-side expectation or a source scan, which is the shape PROCESS_SPAWN_SITES already uses in this repository (clause 1). (ii) the run-D-shaped fixture — a config directory carrying http_unix_socket, reached through a forwarded config-locating variable — split in two, because its halves have different requirements. (ii-a) the refusal driver: with the fixture in place the adapter refuses before spawning, so it needs no gh at all and is never skipped — a pre-spawn refusal that never reaches the binary is testable on any machine. (ii-b) the residual demonstration: with the refusal bypassed through a test seam — an injection point the test supplies, never a shipped flag or configuration key (a runtime switch that disables a security check is a second way to reach the exposure) — the request leaves through the socket. That half needs a real gh, so it is skipped where the binary is absent and the skip is reported rather than counted as a pass. Only (ii-b) is skippable, and the driver for the control is not. Slice 1. |
| 5 | The gh binary is resolved to an absolute path, and the vector is passed with shell=False. |
SEC-9 verbatim: "Never build a shell command by string concatenation. git and gh are invoked as argument vectors with shell=False" (requirements-analysis.md:236). An unresolved name would also let the child's PATH choose the executable; clause 4 means whatever PATH the child sees is one this project constructed, and clause 5 means it is not consulted for the executable at all. |
A test that the first vector element is an absolute path and that shell=True appears nowhere in the module. Slice 1. |
| 6 | No --paginate. Every page after the first is requested by handing back a GraphQL cursor in a typed variable, with the vector otherwise unchanged. |
--paginate exists to follow a next-page reference the response supplies. Exactly what it follows, and how, is behaviour of a binary this design does not pin (clause 8 bounds only its version) — and that is the reason the flag is excluded rather than characterised: a destination the response chooses is the shape T-7 names, and a cursor in a typed variable cannot become one. |
A test that --paginate is absent from every spawned vector, and a cursor-pagination test over a recorded fixture. Slice 1. |
| 7 | A request timeout (SEC-19) and recorded ingest cost bounds: a page cap and a PR-count cap, each a named constant. Exceeding a cap is a reported, graded stop, never a silent truncation and never an unbounded loop. | A caller — or a large repository — making the system spend work no recorded limit bounds. The severity table grades exactly that as HIGH, and #26's T-6 concurrency cap is the precedent for how such a bound is recorded: a constant, a test, and prose that names the number. | A test per cap that the constant is the value the adapter uses, and a test that a fixture exceeding the cap stops with a report. Slice 1. |
| 8 | A gh version floor, expressed as a constant with a test, not as prose. |
gh is not a Python dependency, so ADR-0014's exact pinning does not reach it; the behaviours clauses 2–6 rely on are flag and config behaviours of a binary the operator upgrades independently. Prose asking for "a recent gh" is not a control. |
A test that the adapter refuses to spawn below the floor, and that the floor is the constant the refusal message names. Slice 1. Measured against 2.86.0 — the floor is chosen at implementation, not asserted here. |
| 9 | gh absent, or present and unauthenticated, is a graded refusal envelope with a remedy — never a traceback. The child's stderr surfaces only inside that envelope, never straight to a log or a caller. |
The failure the product already has a shape for: requirements-analysis.md:328-329 records Degraded as "a success-with-warnings terminal state, not a failure: a missing gh token must not prevent local knowledge from working." Ingestion is the optional capability; the rest of the product keeps working. The stderr half is the GH_DEBUG=api shape — a debug-verbose child can print request detail, and the environment constant of clause 4 excludes GH_DEBUG, so the two halves close it together. |
A test for each of the two states asserting a refusal envelope carrying a remedy and no traceback, and a test that child stderr reaches no sink outside the envelope. Slice 1. |
| 10 | A per-response byte cap, as a named constant, with a typed refusal when a response exceeds it — and the read shape that makes the cap real: the child's output is read incrementally against the cap, not accumulated first and measured after. | An unbounded capture_output of third-party bodies is the same unbounded-work class clause 7 covers for request count, one layer down: a repository's comment bodies are content Theurian does not control. The product already records a bar for a bounded read — MAX_SOURCE_FILE_BYTES (8 MiB, security/paths.py:45), used by ingestion_service.py:180 — so the cap is set beside a recorded number rather than invented. |
A test that a response past the constant is refused with the typed refusal and not truncated silently, and a test that the read does not buffer the whole response before deciding, spelled so it can fail and so a wrong implementation reddens rather than hangs: a child that emits the cap and then blocks, with the assertion under a bounded wait — an implementation that accumulates first exceeds the deadline and fails; one that reads incrementally refuses at the cap and returns inside it. Slice 1. |
The attack class clause 4 exists for, measured
Three runs, quoted from the design consult of 2026-09-05, gh 2.86.0. Runs B
and C are the positive controls: without them, a clean run of A would be
consistent with the threat not existing at all.
| Run | Command shape | Observed |
|---|---|---|
| A | GH_HOST=evil.test gh api --hostname github.com graphql … |
request went to Host: api.github.com — the --hostname pin holds against GH_HOST |
| B | GH_HOST=evil.test gh api graphql … (no pin) |
error connecting to evil.test — the threat is real, and A's result is a pin doing work rather than a variable being ignored |
| C | HTTPS_PROXY=http://127.0.0.1:9 gh api --hostname github.com graphql … |
proxyconnect tcp: dial tcp 127.0.0.1:9 — the pin does not cover this class: the request was routed to loopback with the hostname pin in place |
Three further runs, quoted from the adversarial review of 2026-09-05 (same
binary, gh 2.86.0); run D was re-run by the orchestrator before this text was
written and reproduced. They are recorded at the fidelity the round reported them
— each run's shape and its result — and slice 1 re-runs them as clause 4's
positive control:
| Run | Shape | Observed |
|---|---|---|
| D | --hostname github.com pinned, only allowlisted variables set, GH_CONFIG_DIR pointing at an isolated config directory whose config sets http_unix_socket |
the request went to the unix socket |
| E | the same config reached through HOME instead |
the same |
| F | the same config reached through XDG_CONFIG_HOME instead |
the same |
Runs B, C and D–F are each a positive control for a different half of clause 4: B
shows the GH_HOST threat is real rather than ignored, C shows the --hostname
pin does not reach the proxy family, and D–F show it does not reach the config
file either. Together they are why clause 4 is construction and not scrubbing
of GH_HOST: the destination is decided by at least three independent inputs,
and a defence that enumerates what an attacker may not set has to be complete
about all of them.
The admission rule, derived from the table's Value column
A rule that contradicts its own table is not a rule — :177 below states that
standard, and three drafts have now died by it. The first two split the
environment by what a variable carries: credentials may pass, destination-movers
may not, which admits the GH_TOKEN family — identity from the caller's
environment, the second half of the class clause 4 exists to close — and then
passed the config-locating variables anyway, which runs D–F show to be
destination-bearing. It excluded exactly the variables it admitted. The third
replaced it with a single necessity test ("admitted only if gh cannot locate the
operator's persisted authentication without it") and then admitted five rows that
fail it: NO_COLOR, the three notifier and prompt variables, and PATH.
So the rule is not written over the table and then checked against it. It is
derived from the table's Value column, and that column has three forms —
which is what the table shows, not two:
Value form |
Admission ground | Rows |
|---|---|---|
| forwarded by value — the parent's value crosses into the child unchanged | necessity: admitted only if gh cannot locate the operator's persisted authentication, or its platform credential store, without it |
HOME, GH_CONFIG_DIR, XDG_CONFIG_HOME |
set to the literal 1 — a constant carrying no parent data and no value choice |
a named operational property, stated per row, with its transport reach enumerated beside it | NO_COLOR, GH_NO_UPDATE_NOTIFIER, GH_PROMPT_DISABLED, GH_NO_EXTENSION_UPDATE_NOTIFIER |
| set by Theurian to a fixed value that is not forwarded — a value this project chooses, where the choice itself is the decision | the operational property plus a pinned value: the value is fixed in the constant and never derived from the parent's, because a derived value would re-admit the parent's input through a row that reads as a literal | PATH |
PATH's ground is the third form, and the load-bearing half is the
never-derived. An implementer may satisfy "a Theurian-constructed value" by
building it from the parent's PATH — filtering it, prepending to it — and every
word of the earlier wording would still be honoured while the parent chose the
helper binaries again. So this ADR fixes the rule (a fixed literal, never
derived from the parent's PATH) and leaves the exact literal to slice 1, which
is also what makes clause 4(i)'s test-side expected mapping writable: an unpinned
value has no expectation to write.
The grounds are asymmetric because the risks are: a forwarded value is an input
the parent controls, so it has to be necessary; a literal carries no input at
all, so it has to be useful and bounded; a chosen value is neither, so it needs
both the property and the pin. Reach is enumerated for every row either way, in
the open — reduced where a check exists, recorded as a residual where none does.
The derivation cannot contradict the table, because every row's ground is read
off that row's own Value.
The closed enumerated constant. The child environment is exactly the rows below, and the table is the constant — there is no "and similar":
| Variable | Value | Why it is admitted | Transport reach |
|---|---|---|---|
HOME |
forwarded by value | gh locates its config directory and the operator's persisted login through it |
reaches the config file — reduced by slice 1's pre-spawn refusal for the accidental single-well-formed-file case only, with the divergence class derived below |
GH_CONFIG_DIR |
forwarded by value | the same, when the operator sets it explicitly | the same |
XDG_CONFIG_HOME |
forwarded by value | the same, on the XDG path | the same |
NO_COLOR |
1 |
machine-readable output | none |
GH_NO_UPDATE_NOTIFIER |
1 |
set, not merely absent: without it gh performs its own 24-hour release check — an outbound request no argument vector of ours chose (gh 2.86.0's documented environment, read in round two) |
removes an outbound request |
GH_PROMPT_DISABLED |
1 |
a spawned gh must never block on an interactive prompt |
none |
GH_NO_EXTENSION_UPDATE_NOTIFIER |
1 |
same class as the update notifier: a check nobody asked for | removes an outbound request |
PATH |
a fixed literal chosen by Theurian, never derived from the parent's (the literal is slice 1's; the never-derived is this ADR's) | gh shells out (git, credential helpers); an inherited PATH would let the parent environment choose those binaries |
it selects which helper binaries gh runs — enumerated here as reach, not claimed to be harmless: a helper this project's own PATH value resolves is one this project chose |
Nothing else. GH_TOKEN, GITHUB_TOKEN, GH_ENTERPRISE_TOKEN and
GITHUB_ENTERPRISE_TOKEN would have to enter as forwarded by value, and they
fail that form's ground: gh finds the operator's persisted login without them.
So identity never comes from a caller's environment. Headless environment-token authentication is a recorded non-goal
until somebody needs it, at which point it is a decision with its own reasoning
rather than a variable quietly added to this table.
One platform question is open, and it is answered by measurement, not by
guessing. On Linux the credential store is reached through a session bus
(DBUS_SESSION_BUS_ADDRESS, XDG_RUNTIME_DIR), and whether gh can find a
stored credential without them is not measured here — this machine is macOS.
Slice 1 owes that measurement in CI, on Linux, and the constant gains a
platform member only if the measurement says the credential is otherwise
unreachable. The rule stays fixed either way: the equality test pins whatever
the constant records on that platform, so "fixed membership" is a property of the
rule, not a claim that today's eight rows are the final list.
The residual, on the measured storage facts
The premise an earlier draft rested on — the same directory holds hosts.yml,
and so the operator's token — is false in gh 2.86.0's default
configuration. Two measurements, both from round two and both re-verified:
- The default credential store is the OS keychain.
hosts.ymlon this machine carries nooauth_token; a plaintext token in that file is the--insecure-storagefallback, not the default. - Redirecting and writing are different acts. Pointing a config-locating
variable at a new directory loses the credential too —
ghreports itself unauthenticated. Writing a transport override into the operator's real located directory does not: the keychain handsghthe token as usual, andghhands the authenticated request to the attacker's socket. The credential is captured without ever being read.
So the exposure is not "an attacker who could already read the token". It is: an
actor able to write the operator's own gh config directory can redirect an
authenticated request and capture what it carries, without holding the credential
themselves. Slice 1's pre-spawn refusal of known transport-override keys reduces
that exposure — but only some of it. The residual is a class, not a list, and
it is derived from one fact: the check's read cannot be gh's read. Every way
those two reads can diverge is a member, and four are measured:
| The two reads diverge on | Member | Why the check cannot reach it |
|---|---|---|
when — the check reads at check time, gh at spawn time |
(a) the race — a writer landing a known key in that window | Two reads of a mutable file with nothing holding it still between them |
which keys — the check knows a fixed set, gh honours what its version understands |
(b) an unknown or future key, needing no race at all | Clause 8's floor bounds the binary only below: a newer gh may honour a setting this check has never heard of |
| which bytes parse, and how — both read the same file and disagree about it | (c) parser divergence. Measured, adversarial round 4, gh 2.86.0: with http_unix_socket present twice, PyYAML safe_load takes the last occurrence (the check sees no override) while gh dials the first occurrence's socket. A parse error is the same class in the other direction: a naive best-effort check fails open |
A YAML reader is not gh's reader; agreeing on the bytes is not agreeing on the value |
which file — the check reads what os.environ.get suggests, gh resolves one directory by precedence |
(d) resolution divergence. Measured: gh reads exactly one directory — GH_CONFIG_DIR > $XDG_CONFIG_HOME/gh > $HOME/.config/gh — an empty-string variable falls through to the next, and a relative value resolves against the child's cwd |
Runs D–F moved one variable at a time and never measured the precedence, so a check that reads all three, or reads them in the wrong order, checks a file gh will not open |
Members (c) and (d) need neither timing nor an unknown key: the same file, the same moment, and the check still answers a different question.
So the check is priced honestly, in one sentence: it reduces the accidental,
pre-existing, single-well-formed-file case, and nothing above it. That is worth
having — a typo'd or inherited http_unix_socket is the case an operator actually
meets — and it is not a control against an adversary. The class above is the
recorded residual.
The parse-error arm is a decision, not an oversight: the check fails open. An unparseable config is refused by nothing here. Refusing to spawn on any config the check cannot parse would deny the ingest to precisely the operator this check exists to help — someone whose config has a typo — and it would make a YAML reader's strictness a gate on an unrelated capability. The exposure that decision accepts is member (c), recorded above.
What the check's own spec must therefore say, all of it owed to slice 1: it
resolves the same precedence chain gh does, treating an empty-string variable
as absent and falling through exactly as gh does (measured); and it runs
before any binary probe — ahead of clause 8's version read and clause 9's
authentication probe — because a probe is itself a spawn, and a check that runs
after one has already handed the config a request.
The precondition, named because it is the operator's trust and not Theurian's
check. The directory is the operator's own, user-owned by construction; Theurian
does not inspect its permissions or ownership, and does not propose to. Which
directory it is, is decided by precedence and not by union: exactly one is read,
by the order in member (d) — an earlier draft of this paragraph said the reach was
"the union of what they resolve", which is not what gh does. (Measured on 2.86.0,
http_unix_socket is a real configuration key while api_host is not — which is
why both the refusal and member (b) name the class of transport-override settings
and not one key.) Clause 4's tests keep member (a) demonstrated rather than argued,
in the split clause 4 specifies.
Theurian borrows the operator's ambient GitHub identity, and holds no token.
Ingestion runs with whatever scopes the operator's gh login already has,
which may include private repositories. That is accepted, because the alternative
is Theurian taking custody of a token (rejected in Alternatives considered), and
because the control that keeps ingestion off private repositories is decision 2's
allowlist and its refusal — not the credential's reach.
2. Scope: public allowlisted repositories only, and the argument is an audience argument
The adapter reads providers.review.repositories before any spawn. The key
is already schema-ready: schemas/config/project-config.schema.json types it as
an array of strings matching ^[\w.-]+/[\w.-]+$, beside a
providers.review.adapter enum of github | none defaulting to none. Its
description says "Not in force… Nothing reads it today", and slice 1 is the change
that makes both sentences false and rewrites them.
Three refusals, each with a synthetic-input driving test owed to slice 1:
- A repository not in the allowlist is refused before the process is spawned. Not filtered after the fetch — the spawn does not happen.
- A repository that resolves as private is refused at ingestion, even if it is in the allowlist, and nothing about it is written.
- The repository the response describes is checked back against the allowlisted
entry. GitHub redirects a renamed
owner/repo, so an allowlisted name can resolve to a repository nobody allowlisted. The adapter compares the response's resolvednameWithOwneragainst the entry it asked for, case-folded — GitHub treats owner and repository names case-insensitively, so a byte-comparison would refuse a correct answer. The repository id is checked only against a previously recorded one: on a first ingest there is nothing to compare it to, and an id read out of the same response it is meant to validate proves nothing. Owed a slice-1 test with a renamed-repository fixture and a case-difference fixture.
The argument for public-only v1 is an audience argument, and it must be written narrowly. The tempting sentence — "a public repository cannot carry embargoed content" — is false as a universal, and this ADR does not make it: a public repository's review threads can discuss anything, including an unpublished vulnerability, and this project's own embargo discipline exists precisely because people are capable of putting such a thing where it does not belong. The true and narrower claim is:
Public-only v1 ingests no advisory-private GitHub surface (private repositories, security advisories, private forks), and every record it ingests was visible to the public repository's audience at the moment it was ingested.
That is the same structural shape ADR-0029 decision 6 records for public main:
the protection is structural — the source has no access to the withheld
surface — and it is not a claim that the public surface is guaranteed clean. What
guards the second half is unchanged from the rest of the product: the secret scan
at ingestion (decision 4) and the untrusted-content triple at serve (decision 6).
The subject is theurian review ingest, not .theurian/review/, and the
correction is to the sentence rather than to the decision. This quote read
"every record it holds" until 2026-09-11, which a reader could take as an
inventory of the evidence directory — and that is false, because decision 3 makes
those files source rather than derived state and theurian init deliberately
does not git-ignore them, so a clone can carry evidence a repository author wrote
and theurian review build projects it like any other. The scope this decision
sets is unchanged: what an ingestion run may fetch, and from where. What a
corpus may contain is threat-model T-24, an accepted residual recorded there
with its grade and its upgrade triggers. Every surface that publishes this scope
— the capabilities schema, mcp/tools.py, docs/protocol/mcp-tools.md — names
the same subject.
The tense is load-bearing, and the residual it names is retention. Decision 3
makes the evidence files durable precisely so an upstream delete does not erase
the record — which means "visible to the public audience" is true at ingestion
time and can stop being true afterwards, when an author edits or deletes a
comment upstream. This ADR does not pretend otherwise. The two cases part
there, and only one of them is the residual. An upstream edit does reach
Theurian's copy: decision 3's writer rewrites a record whose content changed and
ReviewIngestReport counts it updated, so the next theurian review ingest
run whose window covers the record brings the edit with it. An upstream delete
does not — that is the retention decision itself, and no mechanism here would
notice one. The remediation path exists and is manual, and it is the delete
case's: delete the evidence file and rebuild the derived store, which is
exactly the operation decision 3's files-as-source shape already supports (the
store is rebuilt from the files, so removing a file removes the record from every
surface). A capability that would make deletion automatic is not designed here
and is listed in What this does not close.
The sentence above read "it does not build a propagation path either" until
2026-09-11, and that was wrong for edits. Its colon-clause named only delete,
but the sentence it followed had enumerated "edits or deletes", so the generic
half read as covering both — and the surfaces that publish this scope spelled it
out that way, as "an upstream edit or delete does not reach Theurian's copy". The
capabilities schema, mcp/tools.py, docs/protocol/mcp-tools.md and the flag's
own pin in tests/integration/test_mcp_tools.py are corrected in the same
commit, and they were corrected against the measured behaviour:
tests/unit/test_review_evidence_store.py::test_a_refetch_rewrites_a_record_whose_content_changed_upstream
has pinned the update since slice 2. As with the it holds correction above, the
decision is unchanged — retention is still what decision 3 chooses — and what was
wrong was a sentence about how the corpus behaves.
The private-repository arm stays owed, and it keeps a named owner. ADR-0029
assigned it to this arm: a finding marked securityRelated at ingestion time,
where advisory state is available, then refused uniformly at serve — the
refusal must not distinguish "an embargoed item exists and is withheld" from "no
such item exists". Milestone 8 does not build it, and this ADR does not hand it to
an unnamed follow-up either: it is owned by
#575, the change that adds
private-repository ingestion. ADR-0029's owed table and the threat model's
embargo-arm sentence are repointed to #575 in the same commit as this ADR;
nothing pins them to each other thereafter — a later edit to either can drift,
and no test would notice. An owed item whose owner is "a follow-up" is the
owner-position defect this document diagnoses twice elsewhere, and it is not
repeated here.
The scope is machine-visible, not only prose. system.capabilities today
publishes reviewIngestion: false beside reviewFindings: true, and
docs/roadmap.md records why those two are separate flags: "the change that
reaches GitHub is the one that owes SEC-10's repository allowlist … and an
offline trailer read owes none." The same reasoning applies one level down, so
the flag alone is not enough: a client that reads reviewIngestion: true and
nothing else would conclude that review history is ingested wherever the operator
points it. The decision is therefore reviewIngestion: true plus a scope
field — proposed shape reviewIngestionScope: "public-allowlisted", a string
beside the booleans, which the capability block already does elsewhere (the
"knowledgeSearch": "hybrid" entry in mcp/tools.py) — flipped together at the
serve slice, with the wire schema change that publishes them landing in slice 3.
(That cite read mcp/tools.py:1868 until slice 3 moved the line; it is re-cited
by the quoted entry under this document's own Demoted populations rule, which
is that a quote survives renumbering and a line number does not.)
The flag's published meaning narrows, and that redefinition is stated here
rather than performed quietly. Today reviewIngestion: false is read as this
build cannot reach GitHub. From slice 3 the flag means an ingestion call surface
exists that a client may call — a narrower promise, because the fetch path will
have shipped two slices earlier while the flag was still false. Redefining a
published flag without moving what cites it is how a security statement becomes
false, so slice 1 moves the sites that record the old meaning.
The population is a key, and what lives here is the key, its frame and the rule that judges a line — not the annotated population. Four rounds tried to keep a member table inside this ADR in bijection with a moving tree, and each round found a mispointed anchor, an unjudged line or a stale frame beside a correct run. The table is therefore demoted (see Demoted populations in Dispositions); what a design record owes is the key and the judgment rule, both of which are stable.
The key, at origin/main @ 1fe3302b:
$ git grep -n "reviewIngestion" 1fe3302b -- . ':!.claude' ':!.theurian' ':!docs/work-logs' | wc -l
31
$ git grep -l "reviewIngestion" 1fe3302b -- . ':!.claude' ':!.theurian' ':!docs/work-logs' | wc -l
14
The exclusions, measured rather than asserted — git grep -n "reviewIngestion"
1fe3302b -- <path> for each: .claude/ drops 0, .theurian/ drops 0,
docs/adr/0030-* drops 0 at that frame. Only docs/work-logs/ drops
anything — 1 line — and it stays excluded because a work log is a dated record
of what was believed then. CHANGELOGs are not excluded by path, because that
dropped a live member: claim_surfaces.dated_lines — the audit tooling's own
answer to "is this line inside a dated section?" — puts five reviewIngestion
lines inside dated sections of packages/theurian-core/CHANGELOG.md and
plugins/claude-code/CHANGELOG.md:281 outside one. The rule is
owner_position_cites's, quoted from its _RELEASE_RECORDS docstring:
[Unreleased]is not one of those entries … It describes the tree a reader has checked out, it is rewritten on every merge, and a dead owner written into it is a live claim in a governed file.
The judgment rule, which is what a slice-1 brief applies to each line. Every line in the key's output is exactly one of:
| Class | Rule | Moves in slice 1 |
|---|---|---|
| Mover | the line states what the flag means — that nothing reaches GitHub, that this is the flag that would, or that no adapter exists | yes |
| Non-mover | the line states the flag's value, or names it as a key in a list, or is another record's own recorded prose about its own decision | no — the value flips at slice 3 |
| Dated record | the line is inside a dated release section | no — correcting a release record would falsify it |
Two frames, because they differ. Line anchors are 1fe3302b; this branch
edits three of those fourteen files, so its own frame differs:
$ comm -12 <(git grep -l "reviewIngestion" 1fe3302b -- . ':!.claude' ':!.theurian' ':!docs/work-logs' \
| sed 's/^1fe3302b://' | sort) \
<(git diff --name-only origin/main...HEAD | sort)
docs/architecture/review-knowledge.md
docs/roadmap.md
docs/security/threat-model.md
Slice 1 is not a prose pass: movers include production-source docstrings, two
assertion messages, and INGEST_CONFIG_BULLET in
tests/unit/test_config_key_call_sites.py, which pins the plugin's ingest.md
sentence byte for byte — the page and the constant move together or the test
goes RED.
The flag itself does not flip until slice 3, because until then there is nothing for a client to call; the two facts are separated in the documents rather than papered over by moving the flip.
The window that leaves is a bounded residual, recorded rather than argued
away. Through slices 1 and 2 the machine-readable answer is reviewIngestion:
false while a fetch path ships — the documents will say what is true, but a
client that reads only the flag gets the retiring meaning. The alternative was
considered: publish the scope field early, in slice 1, so the machine-readable
surface moves with the code. It was not taken because the field's honest value in
slice 1 is "a fetch path exists that no tool exposes", which is a third meaning
for a flag already being redefined once, and it would ship a wire-schema change
two slices before the surface it describes. The residual's reach is bounded by what
the flag can be used for and by nothing else: no tool is callable, so a client
acting on the false loses no capability it could have had — but for those two
slices the machine-readable security statement reads false while a fetch path
ships, which is a wrong answer to a security question even when no feature is
lost by it. That is the cost this residual carries, and it is why the documents
move in slice 1 rather than waiting for the flag.
3. Evidence files are the source; SQLite is derived
Normalized evidence records land as structured JSON files under
.theurian/review/ — durable, git-trackable, and deliberately not under
.theurian/cache/. The SQLite serving store (slice 3) is built from those files
and is deletable.
This withdraws a position that was live in the documentation, and the closure is a key on the position's own words. Three rounds enumerated the records carrying it by hand — one, then four, then five — and each missed one; a fourth round found that a key on the path token could not see the served corpus at all. So the key is the position string itself:
$ git grep -n "raw GitHub review caches" -- . ':!docs/adr/0030-*'
.theurian/knowledge/architecture/sqlite-is-a-derived-artifact.01M0D5GTZN1YPYAPJ18TAP7NAD.md:41:- raw GitHub review caches, normalized temporary documents
.theurian/knowledge/architecture/sqlite-is-a-derived-artifact.01M1EVBH96EE1X6Z9KTA5EZ95K.md:41:- raw GitHub review caches, normalized temporary documents
.theurian/knowledge/architecture/sqlite-is-a-derived-artifact.01M1QMNC4YSFVPST9RA2FNHKDJ.md:41:- raw GitHub review caches, normalized temporary documents
docs/adr/0004-sqlite-is-a-derived-artifact.md:41:- raw GitHub review caches, normalized temporary documents
docs/adr/0004-sqlite-is-a-derived-artifact.md:86:> "raw GitHub review caches" entry in the *Never Git-tracked (derived)* list
This ADR is excluded from the key by pathspec, for the reason round three recorded and round four upheld: a document that withdraws a position must quote it, so its own lines match any key about that position and an unfiltered paste can never be current in the file it sits in.
Corpus membership, stated because this is a repo-wide key (the standing rule
for any walker here): the served corpus is IN, and it is where three of the five
hits are. Those three are sqlite-is-a-derived-artifact twins — one current, two
superseded revisions — and they carry the withdrawn line because a governed
snapshot moves by re-seed and never by edit. They are
#579's: the same re-seed that
clears the drift this PR causes brings the amendment across.
So the claim is qualified, not universal. No record outside this ADR and
outside the frozen corpus twins #579 re-seeds asserts the position: ADR-0004:41
is the as-authored line the amendment at :86 withdraws in place — the repository
retracts by amending, never by rewriting.
Two measured limits of this key, recorded rather than discovered later. It
matches one physical line, so a straddled form — the phrase split across a wrap —
is invisible to it; and it is invariant under deletion of the amendment: delete
ADR-0004:86 and this key still returns the corpus twins and :41. What guards the amendment's
presence is not this key but #579's re-seed, which byte-pins the amended body into
the corpus, plus test_every_pinned_body_is_byte_identical_to_its_source_anchor_commit
comparing that body against its anchor.
The withdrawn wording is quoted above because the reasoning, not the corrected sentence, is what a later reader needs. It is safe for an artifact whose source outlives it. It is not safe here: GitHub review comments are editable and deletable upstream, so a deleted local copy of a comment that has since been deleted upstream is data loss, not a cache miss, and no refetch recovers it. "Rebuildable" would be asserting a property the upstream does not provide.
With files as the source, ADR-0004's real
property comes back: deleting the SQLite store is a cache miss, because the
store is rebuilt from the evidence files. The contrast with the findings store
is worth stating honestly, because the two look alike and are grounded
differently — infrastructure/sqlite/findings_schema.py says so in its own
docstring: the findings store is safe to delete because "the source of truth is
git history … and this file is reconstructed wholesale by replaying the git
source." Review evidence has no such replayable source. So:
| Artifact | Why deleting it is safe | Layer (ADR-0010) |
|---|---|---|
| Review-finding store (ADR-0029) | it is safe: git history is replayable | Canonical, projected from git |
| Review evidence files (this ADR) | it is not safe — deleting them is data loss | Canonical, carrying a SourceAnchor — but with no replayable source |
| Review serving store (slice 3) | it is safe: rebuilt from the evidence files | Index / derived |
The middle row is a case ADR-0010's three layers do not have, and it is named
rather than forced into one of them. The bytes GitHub returned are Source; what
lands on disk is a normalized record Theurian wrote, which is the same operation
ADR-0029 places in Canonical — so putting the evidence files in Source would
contradict that precedent for an identical act. What makes them unlike every other
Canonical artifact is the property in the middle column: findings_schema.py says
the findings store is safe to delete because "the source of truth is git
history … and this file is reconstructed wholesale by replaying the git source",
and review evidence has no such replay. So this is a Canonical record whose
own source is not re-readable — the fourth-category case ADR-0004's Milestone 8
amendment records — and it carries a SourceAnchor (FR-S3) naming the upstream
object it came from, precisely because that anchor is the only remaining pointer
to material Theurian can no longer re-fetch.
Paths are built from ids, never from allowlist strings. An evidence file's
path is derived from provider-generated identifiers, or a hash of the repository
identity — never by joining the configured owner/repo string into a
filesystem path. The reason is measurable today: the schema pattern
^[\w.-]+/[\w.-]+$ accepts ../.., so a joined path escapes the directory while
satisfying the config schema. Two controls, both owed:
- Every write resolves through
security/paths.py's containment (resolve_within_root,assert_no_symlink_escape) — the same SEC-7 containment the rest of the product uses, applied to the write path and not only to a servedfilePathfield. - The schema pattern is tightened to reject
.and..segments, and that tightening lands in slice 1's allowlist-reader commit — the commit that first makes the key load-bearing is the commit that makes its values safe.
Refetch is a best-effort refresh, never the durability story. A later
ingestion run updates what upstream still returns and does not delete what it no
longer does; a record that vanished upstream stays in the evidence files, marked
with the run that last saw it. Whether a project commits .theurian/review/ is
the project's decision — the directory is not written into the ignore block. What
lands in it is also subject to the configurable ingestion-time redaction
review-knowledge.md's Privacy section describes, which is design and not shipped
behaviour: no review ingestion path exists to perform it — the GitHub package
holds no adapter, measured in Context — and it is slice 2's, applied at
landing, beside the scan gate, since both are decisions about what a file may
contain before it is written.
The evidence record's field set, enumerated once
The scan gate (decision 4) and the serving trust table (decision 6) need the same population, so it is written here once. Every field FR-V1 names is listed with who controls its value — which decides both whether the scan reads it and which side of the trust boundary it is served on:
| Field | Controlled by | Trust class | Read by the ingestion scan |
|---|---|---|---|
repository owner/name, PR number, event key, review / thread / comment ids |
the provider | structural | no — no free text |
thread state, resolution state, isResolved / isOutdated, timestamps, diff side, line numbers |
the provider | structural | no |
| head, fix and merge commit shas; linked issue numbers; CI rollup outcome | the provider | structural | no |
participant external_id |
the provider | structural | no — see the correction below |
| comment body, review body, PR title, PR description | the author | untrusted | yes |
participant display_name |
the author | untrusted | yes |
| file path as received | the author (whoever named the file in the PR) | untrusted | yes |
| labels, head branch name, milestone name | the author | untrusted | yes |
SourceAnchor (provider, source URI, upstream object id, path) |
Theurian, at ingestion | structural | no — it is written here, not received |
| last-seen-run stamp (which run last observed the record upstream) | Theurian, at ingestion | structural | no |
Correction, 2026-09-08: the participant external_id row holds only while a
node id exists. The row above is left as it was written; this note is what
amends it. infrastructure/github/response.optional_participant reads
external_id as node id or login, so an actor GitHub answers with no id — an
Actor implementation that is not a Node, or a partly-errored response
carrying id: null — arrives with the author's own login in that field. A
login is chosen by its owner and can be changed, which is the untrusted side of
this very table, and a GitHub login is up to 39 characters of [A-Za-z0-9-],
so AKIAIOSFODNN7EXAMPLE is a login somebody can register. Two consequences,
both shipped:
- The scan reads
external_idin both redaction states since PR #596 round 1 (the round record, adversarial H-D). Which of the two a given id is cannot be decided from the record — a rule recognising node-id shapes would restate the adapter's mapping in a second place, and would be wrong in the direction that skips a login — so the field is read unconditionally rather than only where it can be author-chosen. - Redaction pseudonymises a login-fallback id before the record is written
(R-12, decision 3's redaction half). Under
providers.review.redactParticipantNames, a participant whoseexternal_idequals its pre-redactiondisplay_name— the adapter's own signature for the fallback — lands underredacted~<sha256 prefix>instead, deterministically, so the record keeps a stable identity and the login never becomes a file. A node-id-shaped id is kept verbatim, which is what makes the identity graph survive the redaction.
What the round measured is why both were needed rather than either: with the
field unscanned and unredacted, turning R-12 on turned a block refusal into
a landing that published the login under externalId while replacing the
displayName R-12 promises to remove.
Three values in the Controlled by column, not two. A record carries fields
Theurian itself writes: the SourceAnchor (FR-S3) that names the upstream object,
and the stamp decision 3 relies on to say a record survived a refetch. They are
neither provider structure nor author content, and calling them either would put
Theurian's own writes on the wrong side of a trust boundary — so they are their
own row, scanned by nothing because nothing outside this process authored them.
Author-controlled structural-looking metadata is on the untrusted side, and that is the row most easily got wrong. A label, a branch name and a milestone name look like provider structure and are not: anyone who can open a pull request chooses them, so they carry text a person wrote and ride under the same safety triple as a comment body.
This is where ADR-0019 is
discharged rather than merely cited: an ingested label is data, and it governs
nothing. A record labelled security upstream is not thereby security-related
to Theurian; no serving decision, no refusal and no ranking reads a label's value.
The marking that would carry weight — securityRelated — is
#575's, and it is computed at
ingestion from advisory state, never read off an author-chosen label. A design
that let a label decide what is withheld would hand the withholding decision to
whoever opened the pull request.
4. The secret scan runs at ingestion, per record, like propose accept
security.secretScan (SEC-11) applies at ingestion, per record, before a
record becomes a file:
| Policy | Behaviour at review ingestion |
|---|---|
block (default) |
the flagged record is refused and never written; the run reports the refusal by record identity and does not exit as if it were clean |
warn |
the record lands and every finding is reported |
off |
no scan |
This mirrors propose accept, not index build, and the reason is the source's
own premise. application/index_secret_scan.py states why the build records
rather than refuses:
A landed secret is readable through
knowledge.searchandknowledge.getthe momenttheurian migrate applywrites it, before any index exists at all … So a build that refused to publish would deny ranking without un-disclosing anything, and on a project that has never built one it would deny ranking for ever.
That premise is false pre-landing. Review ingestion runs before the content exists anywhere in Theurian, so refusing genuinely un-discloses relative to Theurian's own surfaces: nothing is written, so nothing is served, indexed, ranked or reachable through any tool. The gate that matches is ADR-0027's — validate before you move.
What block costs, stated rather than assumed. Refusal is not free, and the
cost is not symmetric with the build's. Upstream may already have edited or
deleted the comment by the next run (decision 2's retention residual, in the other
direction), so a record refused at ingestion — including on a false positive,
and the detector is best-effort entropy heuristics by its own admission — may be
evidence Theurian never gets another chance to hold. That is accepted, with the
reason named: a landed credential is worse than a gap in the evidence. A gap
is visible in the ingest report and recoverable by re-running with warn after a
human has looked; a landed secret is a disclosure through every serving surface
the moment it is written, and no later policy change un-writes it. Operators who
weigh those differently have warn, which is why the policy is a setting and not
a constant.
The report names the record (repository, PR number, thread and comment id), not
the matched bytes, for the reason index_secret_scan.py gives about pasted
reports: a report that quotes the secret is a second copy of it.
The consult disagreed here, and the disagreement is recorded rather than
smoothed over. The Codex reader recommended the index build signal-mode shape
(land and report, never refuse), reasoning by analogy with the existing
scan-at-build control. The watchdog reader checked the premise of that control
rather than its shape and found it did not transfer — the "already disclosed"
condition is exactly what ingestion does not have. The premise check won, and the
analogy lost. It is noted here so a future reader who rediscovers the analogy
finds the answer instead of re-deciding it.
5. The domain model bends to what GitHub can answer
The ReviewResolution model as built requires two fields GitHub does not
guarantee. Four rows below, of which two change — the fourth,
PromotionGate.ci_successful, is listed precisely because Milestone 8 does not
touch it. Slice 1 work:
| Field | Today | Becomes | Why |
|---|---|---|---|
resolved_at |
datetime (required) |
datetime \| None |
No resolution timestamp exists on the API object. |
resolved_by |
ReviewParticipant (required) |
ReviewParticipant \| None |
resolvedBy is nullable — a thread can be resolved with no participant recorded. |
ReviewEvent.ci_successful |
bool \| None (domain/review.py:61) |
unchanged | None already means unknown, and that is the honest value for a PR with no status rollup. |
PromotionGate.ci_successful |
required bool (domain/review.py:152) |
not touched by Milestone 8 | The gate is candidate-generation machinery, which is out of scope (below). None is unrepresentable there today, so how the gate should treat unknown is a real open question — assigned to the candidate-generation design, not answered here. |
Amended in Phase B slice B5, by the tri-state widening (2026-09-18, the branch commit
ca6246aeof PR #744). The fourth row's "requiredbool" no longer holds, and the open question it hands over has been answered.PromotionGate.ci_successfulisbool | None, andNonedoes not satisfy the gate — ADR-0033 decision 4. What the row records about Milestone 8 stands: Milestone 8 did not touch the field, and the candidate-generation design settled it exactly where this row assigned it.The table's two line numbers resolved when this ADR was authored and no longer do.
domain/review.py:61and:152were both correct at46d0e869, this ADR's own commit; the module has grown since and moved both fields. They are left as authored rather than corrected into history — what to follow is the field onReviewEventand onPromotionGate, not the line.
The measurement, quoted from the design consult (2026-09-05, GraphQL schema
introspection). PullRequestReviewThread's fields are:
comments diffSide id isCollapsed isOutdated isResolved line
originalLine originalStartLine path pullRequest repository resolvedBy
startDiffSide startLine subjectType viewerCanReply viewerCanResolve
viewerCanUnresolve
There is no resolution-timestamp field in that list, and resolvedBy is
nullable. A required field the provider cannot fill leaves the adapter only bad
options: fabricate a value — the ingestion time, or the last comment's time — and
every consumer downstream reads as a measurement something nobody measured; or
drop the whole resolution record, losing the resolution state the model exists to
carry. Making the field optional is the change that keeps unknown expressible,
which is the thing neither bad option can express.
This is a breaking change to the domain model, and it costs nothing today.
Measured 2026-09-05 against origin/main @ 1fe3302b, with the population key
being every occurrence of the symbol anywhere in the shipped package:
$ git grep -n "ReviewResolution" -- packages/theurian-core/src
packages/theurian-core/src/theurian/domain/review.py:98:class ReviewResolution:
packages/theurian-core/src/theurian/domain/review.py:121: resolution: ReviewResolution | None = None
Two hits, both inside the defining module — the class statement and its own field
on ReviewThread. There is no consumer to migrate. The CHANGELOG entry for slice
1 still names it as a breaking change with the old shape and the new one, because
"no consumer exists" answers the migration cost and not the question of whether
the record is honest.
ReviewEvent.ci_successful's mapping is a rule, not a measured enum. The
adapter maps the PR's status rollup to the tri-state as: a definite success →
True; a definite failure or error → False; anything else — pending,
expected, absent, or a value this version of the adapter does not recognise — →
None. The mapping is stated by semantics rather than by enumerating the API's
enum members, because this ADR did not measure that enum; slice 1 pins the member
list against the schema at implementation time. The load-bearing half is the
default: on the ingested record, an unrecognised value becomes unknown, never
failed.
That is a statement about the ingested record and not about the gate.
PromotionGate.ci_successful is a required bool, and is_satisfied / unmet()
read it as a verdict — so an unknown handed to that type has already been
flattened into True or False by whoever constructed it. Milestone 8 constructs
no gate (candidate generation is out of scope), which is why this ADR does not
change the type and does not claim the gate is safe from a shrug. Deciding whether
the gate gains a tri-state, refuses construction on unknown, or treats unknown as
unmet is the candidate-generation design's, and it is listed in What this does
not close.
Amended in Phase B slice B5 (2026-09-18, the branch commit
ca6246aeof PR #744). The first of those three was chosen: the gate gains the tri-state, soPromotionGate.ci_successfulisbool | Noneand an unknown is no longer flattened by whoever constructs the gate — it arrives asNoneand does not satisfy it (ADR-0033 decision 4). This ADR's own position is unchanged: it did not change the type, and the deciding was the candidate-generation design's.
Candidate generation is out of Milestone 8. FR-V2 classification and FR-V3
KnowledgeCandidate generation are the write-path half of Phase B: they are
SEC-12-gated, and they raise the model question (ADR-0009)
that ingestion does not. They are filed and sequenced, not folded in. FR-V5 is
satisfied structurally rather than by a fallback path: no model exists anywhere in
the ingest path, so raw ingestion cannot be broken by candidate generation
failing.
That sentence is a universal, and its authority is a test that does not exist yet,
so it is named here as owed rather than left standing on a reading: slice 2 owes
a walk of the ingest path's modules asserting that none reaches an embedding,
summarization or reranking provider. The shape to follow is
test_no_registered_tool_can_reach_a_canonical_write
(tests/integration/test_mcp_tools.py:2357), which walks the built object
graph — the registered callables, their __wrapped__ chains and nested code
objects — rather than a directory of source files, for ADR-0013's equivalent
claim. What that buys, stated honestly: it sees what the running system
actually holds, including a path no test exercises. What it does not buy is
factory resolution — co_names is a name scan one level down, so a provider
obtained through a factory or getattr is invisible to it, as round two
demonstrated. The owed test is therefore scoped to what it can hold: no ingest
module names a provider module or constructor, plus a runtime assertion over the
built ingest pipeline's object graph. Until it lands, "no model exists anywhere in
the ingest path" is design intent with a named owner, not a measured property —
and even once it lands, a factory-resolved provider is outside its reach.
6. Serving is review.search, under the SEC-15 triple, with its own disclosure round
The serving surface is review.search, taken from the already-published
planned table in docs/protocol/mcp-tools.md (7 review.* rows — the grep and
its output are in Compliance). No eighth name is invented: a tool name is a wire
contract, and the table has been publishing this one as planned.
Every body-derived field is untrusted content (T-3, SEC-15); only structural fields are validated and normalized.
The population is decision 3's field table, not a second list. Its
Controlled by column decides the side: every author-controlled field —
comment and review bodies, PR title and description, display name, file path as
received, and the structural-looking metadata (labels, head branch name, milestone
name) — is served under contentClassification: untrusted-knowledge,
mayContainInstructions: true, executable: false. Every provider-controlled
field — states, timestamps, line numbers, repository, PR number, ids, commit shas,
CI outcome — is validated and normalized. Enumerating the fields in one place is
what keeps a field from being scanned as content at ingestion and then served as
structure.
The file path is deliberately on the untrusted side: it is chosen by whoever authored the pull request. It is served as data and SHALL NOT be used to build a filesystem path — the containment SEC-7 requires is not weakened by a string that arrived over the network, and decision 3 states the same rule for the write path.
The disclosure closure is its own round, and its test is built with the serving
change. ADR-0029's closure argument says it in five words — "new surface owes
its own" — so review.search inherits nothing from review.findings' round. The closure form this project uses is one query against two
corpora: an index that held withheld rows and an index that never did must
return identical responses. Because public-only v1 makes real withheld rows
absent, the fixture is synthetic — that is not a weaker test, it is the only
way to have a withheld row at all in a corpus whose scope excludes them, and a
sweep with no withheld row reports its own answer.
Two inherited controls are named so the serve slice does not rediscover them:
- If review evidence is ever served through a ranked surface, the withheld-row
exclusion is a physical purge (threat-model T-17a), not a result-set filter:
a
fusedScoreis priced over FTS5/BM25 collection statistics computed at index-build time, which a filter does not clean and a tombstone does not move. This is ADR-0029's family-4 third instance, and it applies here unchanged. reviewIngestion: true(with its scope field, decision 2) flips at the serve slice, not at ingest, because from that slice the flag means an ingestion call surface exists that a client may call — the narrowed meaning decision 2 records, whose six citing sites are rewritten in slice 1, two slices before the flip.- The triple is bound by import, not by spelling. A
review.searchpayload carriestheurian.mcp.results.SAFETY(mcp/results.py:44) splatted into the result — imported, never re-typed — so a future edit to the shared constant reaches this surface too. A test that asserts three literal key/value pairs passes on a payload that re-spells them locally and then drifts; the owed test therefore asserts the payload against the imported object, with a companion that the assertion can fail.
Consequences
Positive
- T-7's repository allowlist stops being owed and starts running. The control
is promised in many places and enforced in none. Measured 2026-09-05 at
1fe3302b, key = the literal phrase, population = the repository minus.claude/,docs/work-logs/and this ADR:git grep -l "repository allowlist" -- . ':!.claude' ':!docs/work-logs' ':!docs/adr/0030-*'→ 8 files, 20 lines. The key's recorded limit: it misses the places that spell it differently — the schema description ("Not in force. Allowlist ofowner/repovalues…"), theinfrastructure/github/docstring ("Repositories must be allowlisted… owed with the adapter rather than in force") andreview-knowledge.md("the design obligation on the adapter, not current behaviour") — so 8 is a floor, not a census. And T-7 is two entries, not one:docs/security/threat-model.md:6454anddocs/architecture/requirements-analysis.md:1352each carry their own T-7 row naming the allowlist as owed to #429, so "the T-7 entry" is always plural here and slice 1 rewrites both. Slice 1 is the first change that can carry the control, and both entries are rewritten per control rather than repointed at another epic. - No new production dependency and no token custody. The core's runtime
dependencies are six (
jsonschema,pydantic,python-ulid,pyyaml,referencing,typer—packages/theurian-core/pyproject.toml, measured 2026-09-05). Spawningghkeeps that number, and keeps GitHub credentials in the operator's own credential store. - The absence control is replaced by something that keeps working after the first fetch lands. "Nothing can reach out" is a sentence with no successor: the first time it is false it is retired, and whatever it was protecting is unprotected. Ten clauses with their own tests still say something on the day after.
- Evidence outlives the upstream. Files-as-source means a deleted upstream comment is still in the record, which is the whole point of ingesting review history rather than querying it live.
- The scope is machine-readable. A client learns "public allowlisted
repositories only" from
system.capabilities, not from a paragraph.
Negative
- A spawn site is a hole no name-based scan can fully watch.
test_network_call_sites.pyrecords this itself: a program started under a name assembled at runtime survives every structural arm, and a socket watch cannot see into another process. After slice 1, "what doesghdo with the vector we hand it" is outside every instrument this suite has. - Ingestion inherits the operator's identity and scopes. The credential that runs is not one Theurian issued, bounded, or can revoke.
ghis an unpinned external binary. ADR-0014's exact pinning does not reach it; a version floor with a test is a weaker instrument than a lockfile, and the behaviours clauses 2–6 rely on are that binary's.- The evidence files are a new durability obligation. They are the source, so losing them is data loss — which is exactly the property that makes them the right place for the data, and exactly the property that means a project must treat the directory as content rather than as scratch.
- A second serving surface means a second disclosure round.
review.searchinherits nothing fromreview.findings' round, and its two-corpora fixture has to be built rather than borrowed.
Neutral
- The git-native arm (ADR-0029) is untouched.
review.findingskeeps serving trailers from local history and keeps needing no allowlist; the two arms share the safety triple and the FR-V family, not a source. .theurian/review/is not automatically committed. The ADR decides only that it is not a cache; whether a given project tracks it is that project's decision.- The five planned
review.*tools other thanreview.searchstay planned (seven rows: one shipped, six planned, of whichreview.searchis one). This ADR neither builds nor retires them.
What this does not close
Amended in Phase B slice B5 (2026-09-18, the branch commit
ca6246aeof PR #744): item 2's unknown-CI clause is closed.PromotionGate.ci_successfulisbool | None, unknown is unmet, and ADR-0033 decision 4 carries the reasoning. The rest of item 2 stands — FR-V2 classification and FR-V3 generation are designed and not built, and slice B5 is where they land.It sits above the list rather than under item 2 because
tools/audit/owner_position_cites.pyreads an amendment block two blocks below a sentence as retracting it, and under item 2 this block retracted item 1's live#575ownership cite instead.
- Private-repository ingestion, and with it the
securityRelatedingestion-time marking plus uniform serve refusal that ADR-0029 assigned to this arm. Owned by #575; not built in Milestone 8, and ADR-0029's owed table names #575 rather than this ADR. - FR-V2 classification and FR-V3
KnowledgeCandidategeneration. The write-path half of Phase B, SEC-12-gated, deliberately sequenced after ingestion — and with it howPromotionGateshould treat an unknown CI outcome, since itsci_successfulis a requiredbooltoday (decision 5). - The other two T-7 controls in their raw-URL form — the scheme allowlist and private-network rejection — stay #429's (see Dispositions).
- The remaining planned
review.*tools:review.getThread,review.findSimilar,review.getDecisions,review.generateKnowledgeCandidate,review.listUnresolved. - GitLab and other providers. The
ReviewProviderport exists so that a second provider is an adapter and not a domain change; this ADR designs one adapter and does not claim the port is provider-neutral until a second one exists to test that claim. - FR-V6 Markdown views over review evidence. The requirement scopes them as derived artifacts; this ADR neither designs nor forbids them.
- What
ghdoes after the vector is handed over. Clause 8's version floor bounds which binary, not its behaviour; the residual is recorded in decision 1 and in Consequences → Negative. - Propagating an upstream delete into the evidence files. Decision 2
records the retention residual and the manual remediation — delete the file,
rebuild the derived store; a capability that notices an upstream deletion and
acts on it is not designed here. An upstream edit is not in this item and
never was: a refetch rewrites the record and reports it
updated. The heading read "an upstream edit or delete" until 2026-09-11, matching the widening decision 2 records above and describing as unbuilt something the write path already does. - Headless environment-token authentication. Excluded from clause 4's
constant by decision, not by omission: identity comes from the operator's
persisted
ghlogin. Admitting a token variable is a decision with its own reasoning, owed to whoever needs it.
Dispositions this ADR records
Each disposition is a decision with a reason, recorded here so a later session finds an answer rather than an open question.
#429 is narrowed, not closed, and exactly one control is discharged. Slice 1
discharges one of T-7's three controls for the gh activation context,
reduces a second with a residual that stays recorded, and leaves all three
owed in the raw-URL context that is #429's:
| T-7 control | On the gh path (slice 1) |
In the raw-URL context |
|---|---|---|
| Repository allowlist | Discharged — a live check consulted before the spawn, with a pinning test. The only discharged control on this path | n/a |
| Private-network rejection | Split by family, and neither half is discharged as a whole. The proxy family is closed by construction: it is absent from the child by the equality of clause 4's constant, and run C is what shows it would otherwise move the request. The config family is reduced, not closed: the config file is reached by the child through the three forwarded variables (runs D–F, re-run by round two under exactly the enumerated constant — the request still dialled the unix socket), and slice 1 adds a pre-spawn best-effort refusal of known transport-override keys in the file those variables resolve to. What survives is decision 1's divergence class — four measured members, derived there from the fact that the check's read cannot be gh's read (when, which keys, which bytes-parse, which file). The check reduces the accidental, pre-existing, single-well-formed-file case and nothing above it |
still #429's |
| Scheme allowlist | not applicable — there is no URL in the argument vector to check a scheme on (clause 2) | still #429's |
What holds this row is clause 4's tests, in the split clause 4 specifies — (ii-a) drives the refusal, (ii-b) demonstrates a residual. Neither is "the control that keeps this discharged", and the rationale is not restated here: it lives in clause 4, and round two's fix drifted precisely because the same reasoning sat in two places.
429's activation context is wider than this ADR: it includes the OpenAPI $ref
fetcher, where a URL taken from an ingested document is the input, and there both the scheme allowlist and a destination check are live checks with something to check. T-7 is therefore rewritten per control in slice 1 — the absence control retired at a named commit, the repository allowlist recorded as discharged with the test that pins it, private-network rejection recorded as reduced with its residual class named (decision 1's four-member divergence class), never as discharged, and the rest still owed with #429 named. The threat model's own lesson is the reason this is spelled out — "an owner has to be the change that would implement the control" — and this entry has had the wrong owner twice.
The fetch-absence prose population moves in slice 1, in the same PR as the first
spawn site. Not a later slice, and not a follow-up: on-main claims must never
call the fetch absent while a fetch path ships. Measured 2026-09-05 against
origin/main @ 1fe3302b, with the key stated so it can be attacked:
$ git grep -l -i -E "reviewIngestion|nothing (here )?can reach out|no external fetch|contacts no repository|never fetche[sd]|first external fetch" -- . ':!.claude' | wc -l
30
The key is a deliberately wide phrase heuristic over six spellings, and it
returns tests, tools and work logs alongside the prose that has to change — the
same discipline tools/audit/owner_position_cites.py uses: narrow the population
to what a person must read, then have the person read it. The number is a
dispatch input, re-measured when slice 1 is briefed, not a claim about how many
sentences are wrong. This sequencing is binding.
A second population moves in the same slice, and it needs its own key: the
sentences that attribute this unbuilt work to Milestone 7. Milestone 7 is closed
and built no fetch path, so every one of them is false today. The two sentences
this PR corrected in review-knowledge.md are not members of the key below —
that key's pathspec is source, tests and plugin commands, and cannot select
anything under docs/. An earlier draft of this paragraph claimed them as
members, which was a claim about a population its own key excludes. Round three's
reviewers re-ran a widened key over docs/ as well and reported the docs side
clean after this PR's corrections; the key below stays narrow because the
docs-side movers are already dispatched by the fetch-absence population above.
Two subject-filtered keys were tried and both leaked, in opposite directions. Round one's key is recorded in that round's own comment (issuecomment-5551278216), which reports it as returning 8 lines across 4 files; an earlier draft of this paragraph returned 10 across 7, and neither is a superset of the other, so their counts do not reconcile (8 + 3 ≠ 10 was the arithmetic that gave it away). A subject filter reads one physical line, and these attributions routinely straddle two. So the population is taken unfiltered and classified by hand — the union of both keys and then some:
$ git grep -n "Milestone 7" -- packages/theurian-core/src packages/theurian-core/tests \
plugins/claude-code/commands | wc -l
27
$ git grep -l "Milestone 7" -- packages/theurian-core/src packages/theurian-core/tests \
plugins/claude-code/commands | wc -l
18
27 lines across 18 files at origin/main @ 1fe3302b (same on this branch —
it edits none of them). A person classifies each line into one of three, and the
rule is what lives here; the annotated member list is demoted to the slice-1
brief (Demoted populations, below):
| Class | Rule |
|---|---|
| Mover | the line attributes review collection or a T-7 fetch control to Milestone 7 — false, since that milestone closed and built no fetch path |
| History | the line describes something Milestone 7 actually did — for example tests/unit/test_config_key_call_sites.py:42, "the Milestone 7 diff that added the first reader of .theurian/config.yaml", which is true |
| Unrelated | Milestone 7 named about the write path, the ports register or the corpus seed |
The three classes partition the key's output, and the reconciliation identity a re-measurement has to satisfy is: movers + history + unrelated = the key's line count. The counts themselves are the demoted population's, not this record's — the last annotation and its numbers live in the round-5 record on this PR, which also found two members the previous annotation had misjudged (a mover filed as unrelated, a second history line), so the re-measurement judges every line against the rule rather than trusting a prior split.
The unrelated class is Milestone 7 references to the write path, the ports register and the corpus seed — different subjects entirely. Slice 1 dispatches on the union above, re-measured at brief time; the unfiltered key is what makes that re-measurement reproducible, at the cost of a hand pass over 27 lines.
The #368 / #479 boundary, stated so a third owner-position defect cannot
happen. #368 (open, phase-b)
keeps the six git-native findings-store items ADR-0029's owed table assigns it —
including a ranked-search surface over findings.
#479 (Milestone 8) owns the
GitHub-API arm: events, threads, comments, resolutions. review.search serves
review evidence, not findings; review.findings keeps serving findings. Two
issues, two sources, two tools, no overlap.
Milestone 8 is a planned epic, so the class-expansion brake does not bind it. Four top-row PRs at full synchronous review weight — this ADR, ingest, land, serve — is the plan, not a class that kept producing siblings. A later session counting PRs in this milestone should read them against the plan, not against the three-siblings budget.
Corpus membership: this PR drifts one anchored twin, and the sentence below is
the checker's output rather than an assertion about it. An earlier draft asked
the wrong question — its key looked for ADR-0025–0029, the threat model and
review-knowledge.md among the anchors, and never asked about the file this PR
edits. The right key is the anchored filePath list intersected with this PR's
own file set:
$ comm -12 <(git grep -h "filePath:" -- '.theurian/migrations/*.yaml' | awk '{print $2}' | sort -u) \
<(git diff --name-only origin/main...HEAD | sort)
docs/adr/0004-sqlite-is-a-derived-artifact.md
One member — the ADR this PR amends. The checker agrees, and its output is the claim:
$ uv run --frozen python tools/corpus_drift.py
Corpus drift: drifted -- 1 drifted -- compared 26 anchor(s) across 48 committed migration(s); 0 uncheckable; 22 superseded.
DRIFT architecture.sqlite-is-a-derived-artifact: docs/adr/0004-sqlite-is-a-derived-artifact.md now hashes to <digest>, and the corpus pins <digest>
(The two contentSha256 prefixes the checker prints are elided as <digest>
above, and only there: they are content hashes, not commits, and
tools/audit/sha_anchors.py reads any 7–40 hex characters in governed prose as a
commit anchor it must resolve. Everything else in that block is the run's exact
output.)
The re-seed follows this PR's merge, and cannot precede it. A twin's
sourceAnchors[].commitSha must name a commit that holds the body verbatim, and
test_every_pinned_body_is_byte_identical_to_its_source_anchor_commit raises on
an anchor "this complete clone does not contain". Measured: all 7 distinct anchor
commits in the corpus today are ancestors of origin/main
(git merge-base --is-ancestor <sha> origin/main for each, 7/7). The amended
ADR-0004 exists only on this branch, so the only commit that could be anchored is
one a squash-merge discards — which would turn a working test RED on main the
moment this lands. So the re-seed is owed to the change that runs after the
squash commit exists, by the standing pattern recorded when the seven twins were
re-seeded on 2026-09-05, and it is owned by
#579 (open; read on
2026-09-05), sequenced as the first work item after this PR merges. Naming the
owner is the point: an owed item described without a number is the stated-absence
shape this ADR corrects twice elsewhere. CI runs the checker --advisory, which
is why this drift does not block, and why saying so here is the only thing that
records it.
tools/corpus_drift.py walks the committed migrations and compares each anchor to
its live source, so a new docs/ file with no twin — ADR-0030 itself — is outside
its population by construction, the same way ADR-0025 through ADR-0029 already
are. Stated because every repo-wide claim in this repository declares which side of
the frozen corpus it stands on.
Dogfood ground: thin, bot-authored and mutable — not absent. Two measurements, each with a bounded population, because the first one's window moves and a moving window cannot be re-checked later:
| Measurement | Population, bounded | Result |
|---|---|---|
Design consult, 2026-09-05 (gh api graphql, pullRequests(last: 40, states: MERGED)) |
a moving window, bounded by naming the merge list it covered rather than a range: the last 40 commits on origin/main at 1fe3302b carry 38 trailing pull-request refs — 440, 446, 448, 460, 466, 467, 470, 471, 474, 475, 478, 482, 486, 487, 488, 489, 490, 492, 498, 500, 501, 504, 513, 514, 518, 519, 524, 525, 534, 536, 541, 545, 552, 554, 556, 557, 560, 563 (git log origin/main --format='%s' -40 \| sed -n 's/.*(\(#[0-9 #]*\))$/\1/p' \| awk '{print $NF}', taking the trailing ref per ADR-0029's rule, so (#520 #525) contributes 525 and not 520; two of the forty commits carry no ref) |
0 inline review threads, 0 top-level reviews |
Adversarial review, 2026-09-05 (REST pulls / comments), re-run by the orchestrator |
keyed by PR number, not by a window: #12, #132, #224, #352, #569 | 11 inline review threads (#352 ×5, #12 ×2, #224 ×2, #132, #569 ×1 — open at measurement, so a floor rather than a pin; 2 as of 2026-09-07) and 5 COMMENTED top-level reviews — every root comment authored by github-advanced-security[bot], one dated 2026-09-05 on the still-open #569 |
Both figures are dated snapshots, and the thread count moved while this PR was under review: a further bot thread landed on the open #569 six minutes after this PR's round-one fix commit, taking 11 to 12. The number is therefore written as 11 at the round-two measurement, 12 shortly after rather than as a property of the repository — and the movement is not a nuisance, it is the evidence for the fixture decision below.
Re-measured 2026-09-07, and the row's #569 ×1 is annotated rather than
corrected. Mend's harness re-ran the by-PR-number population and found #569
carrying 2 inline threads — both authored by github-advanced-security, both
dated 2026-09-05 — where the member list reads it as one. The 2026-09-05 figures
stay exactly as they were taken: #569 was open when they were, so its member
count was a floor from the moment it was written, and a snapshot that a later
snapshot exceeds is not a snapshot that was wrong. The second thread is
consistent with the one this paragraph already records as arriving mid-round; no
comment id was compared across the two runs, so they are recorded as agreeing
rather than as identified. Discovering record: PR #595's
body. What this adds to the argument below is a second independent observation of
the same property — the population moves, on an open PR, between one round and
the next — rather than a new number for it.
Re-measured a third time on 2026-09-11, by comment identity, and #569's count
is now frozen. gh api graphql against
repository(owner:"theurian", name:"theurian").pullRequest(number:569) answers
state: MERGED, mergedAt: 2026-09-05T15:55:12Z, and
reviewThreads.totalCount: 2 — thread ids PRRT_kwDOTqMAc86fiBod and
PRRT_kwDOTqMAc86fiqjB, each rooted by github-advanced-security, created
2026-09-05T09:35:33Z and 2026-09-05T11:13:26Z. Both root comments predate the
merge, so this is the same pair the 2026-09-07 run counted, now identified
rather than merely agreed with, and the pull request is closed so the count can no
longer move. It does not move the 2026-09-05 row, which was a floor taken while
the PR was open; it retires the uncertainty in the paragraph above.
The second measurement falsifies the universal an earlier draft of this section drew from the first, which said this repository "cannot exercise thread ingestion at all". None of those five PRs is in the merge list above: #12, #132,
224 and #352 are older than every member, and #569 merged on 2026-09-05
after the window that list covers — it is not among the 38 refs enumerated there. (This sentence read "#569 is not merged at all", which was true when it was written and stopped being true the same day: the merge landed at 15:55Z. The claim it carries is unchanged, because what the argument needs is that #569 is outside the merge list, not that it is unmerged.) The narrow 0/0 reading was true; the universal was not, and it is withdrawn here rather than softened.
What remains true is the ground the fixture decision actually rests on: this project's review rounds happen in agent transcripts and land as commit trailers — which is why ADR-0029's arm exists — so the native GitHub-side population is thin (11–12 threads across the repository's whole history), bot-authored (every root comment from one scanner), and mutable (it grew by one during a single review round, on an open PR). Dogfooding on this repository is worth doing and is not a substitute for a controlled corpus. Recorded so that no completion claim for Milestone 8 overstates what was exercised.
So the serve slice's real-run verification data source is decided now, not at
slice 3. Verification runs against a named disposable public fixture
repository under the theurian org — for example review-ingestion-fixture —
carrying a planted, frozen PR and thread set; it is created when slice 3's
verification needs it, allowlisted only in the verification configuration, and
never in a shipped default. The ground is the population above, and its movement
during this very review is the argument: assertions need data that does not
change under them, and the native threads are thin, bot-authored, and demonstrably
live — one arrived mid-round. The rejected alternative is recorded: an external third-party
public repository, rejected for the same reason one step further out, its content
being owned by someone else entirely.
Demoted populations
A design record states decisions; it cannot stay in bijection with a moving tree. Four review rounds each found a correct pasted key standing beside a hand-maintained member list that had gone stale — a mispointed anchor, an unjudged line, a count under two different units. The lists are therefore relocated, not deleted: each one below keeps its key, its frame and its judgment rule here, and its annotated membership lives in the slice-1 assignment brief, where the standing dispatch rule re-measures it at assignment time rather than trusting a figure written weeks earlier.
| Demoted population | What stays in this ADR | New home | Re-measured by |
|---|---|---|---|
| Flag-sites annotated population (M/N/D per line, and its composition) | the key, both frames, the exclusions with measured drops, the three-class judgment rule | slice-1 assignment brief | the assignment-brief counting rule (CLAUDE.md, The orchestrator has no reviewer: a "how many places" claim is answered by a search command with its output pasted into the brief); last full annotation: PR #570 comment 5552854865 |
| Fetch-absence prose population | the key and its stated width, and the binding sequencing (it moves in the same PR as the first spawn site) | slice-1 assignment brief | the same rule |
| Milestone-7 attribution union | the unfiltered key, the three-class rule, and the partition identity (movers + history + unrelated = the key's line count) | slice-1 assignment brief | the same rule |
providers.review.repositories promise population |
the key, its recorded limit (it misses the differently-spelled promises), and the commit-split reason | slice-1 assignment brief | the same rule |
| Line-anchored citations converted to quoted sentences | the quoted sentence and its file | — (it travels with the quote) | a quote survives renumbering; a line number does not |
Rows that quote their anchor sentence carry no line number, deliberately: this PR's own owner repoint moved two cited line numbers inside its own branch, which is the whole argument.
Slicing plan
| Slice | Contents | Notable ordering |
|---|---|---|
| 1 — ingest | the allowlist reader as its own commit (with the schema-pattern tightening, decision 3); then domain fidelity (decision 5); then the adapter, the ten clause tests, and the document corrections | The split is justified by a pinned test, not by disjointness — see the note below, which measures the overlap the earlier draft assumed away |
| 2 — land | evidence files under .theurian/review/, the ingestion-time secret-scan gate, the CLI entry point |
The CLI verb is slice 2's to choose; naming one here would assert a command that does not exist |
| 3 — serve | the SQLite serving store, review.search, its wire schema, the capability flag and scope field, and the T-7 / roadmap / mcp-tools.md updates that follow from serving |
The disclosure round and the two-corpora fixture belong to this slice |
Why the allowlist reader is its own commit — the real reason, measured. An
earlier draft said the two commits touch different file sets. They do not: of
the 15 files naming providers.review.repositories, 11 are also in the
fetch-absence population at 1fe3302b (12 on this branch, the twelfth being this
ADR, which is a member of both populations it measures):
$ comm -12 <(git grep -l -i -E "reviewIngestion|nothing (here )?can reach out|no external fetch|contacts no repository|never fetche[sd]|first external fetch" -- . ':!.claude' | sort) \
<(git grep -l 'providers\.review\.repositories' -- . ':!.claude' | sort) | wc -l
12
The justification is a pinned test, not disjointness:
tests/unit/test_config_key_call_sites.py asserts by equality that nothing
reads the key, so the reader's commit is the one that must flip that pin and
rewrite the sentences resting on it. Bundling it with the adapter would put two
independently-revertable claim flips in one commit. Because the file sets overlap,
the two commits are sequenced serially in one worktree, never fanned out.
Alternatives considered
| Alternative | Why rejected |
|---|---|
An in-process HTTP client (httpx plus a token from the environment) |
Three costs at once. It adds a production dependency to a core whose runtime dependency list is six packages, none of them an HTTP client. It puts token custody inside Theurian — reading, holding and possibly logging a credential the operator currently keeps in their own credential store. And it creates a raw-URL surface, which makes all three of SEC-10's controls live checks that must be built and kept correct. The gh api graphql form does not make the SSRF class disappear — run C and runs D–F show the destination moving under a pinned hostname — it removes the input those two checks read: there is no URL in the argument vector to apply a scheme allowlist to, and the private-network reach is reduced by constructing the environment (clause 4) rather than by inspecting a destination, with the config-family residuals recorded (decision 1 derives the four-member divergence class that survives slice 1's pre-spawn refusal). The gh spawn trades a process boundary for that, and buys a smaller reduction than an earlier draft of this row claimed. |
| Inherit the parent environment and scrub the dangerous variables | A scrub is a blocklist over a set the adapter does not control. Run C measured that HTTPS_PROXY moves the request even with --hostname pinned, so the blocklist would have to be complete over destination variables, proxy variables, and whatever the next gh release reads. Constructing the environment inverts the burden onto a set this project chooses and pins. |
gh api --paginate |
Whatever the flag follows, the response chooses it rather than the adapter — and this design pins no version-specific semantics for it (clause 8 bounds the binary's version, not its behaviour). Excluding the flag is cheaper than characterising it: a GraphQL cursor is an opaque string in a typed variable, so the adapter still decides every destination it reaches. |
gh api repos/{owner}/{repo}/pulls/... (the REST path form) |
Repository identity would travel in the URL position, where it is string-interpolated into a path. The GraphQL form carries the same identity as typed variables with no path to escape into, which is what makes clause 2 a checkable property rather than a promise about quoting. |
Keep the review data under .theurian/cache/, as review-knowledge.md said before this change |
Upstream review comments are editable and deletable, so a discarded cache entry for a deleted comment is unrecoverable. Calling that a cache asserts a rebuild property GitHub does not provide. |
Scan for secrets at index-build time only, in index build's signal mode (land and report, never refuse) |
This was the Codex reader's recommendation, and it was rejected on the source's own premise: index_secret_scan.py records rather than refuses because its content is already in the canonical store and already served, so refusal "would deny ranking without un-disclosing anything". Pre-landing, refusal genuinely un-discloses — nothing has been written and nothing served — so the propose accept gate is the matching one. |
Make resolved_at required and fill it with the ingestion time or the last comment's time |
The API object carries no resolution timestamp (decision 5's introspection). A required field filled by the adapter is a fabricated measurement that every downstream consumer reads as real. None is the honest value for a quantity the provider does not record. |
| Invent a new tool name for review-evidence search | review.search has been published as planned in docs/protocol/mcp-tools.md since before this design. A tool name is a wire contract; adding an eighth name would leave a published one orphaned and make clients choose between them. |
| Verify the serve slice against an external third-party public repository | Its content can change or vanish under the assertions, so the harness's ground truth would be owned by someone else. A disposable fixture repository under this project's own org is frozen by construction. |
| Ingest private repositories in v1, gated by the allowlist alone | The allowlist decides which repository is contacted; it says nothing about whether the material is safe to serve. Private ingestion needs the securityRelated marking and the uniform serve refusal ADR-0029 specifies, and building that is a disclosure-class design in its own right — folding it into the first fetch path would ship the transport and the disclosure boundary in one review round. |
Compliance
This ADR ships no behaviour, so it has no shipped test to name. Its enforcement at design time is the measurements it cites; its enforcement at implementation time is the tests the slices owe. The names below are the properties an implementation must pin, not files that exist today — the same honest split ADR-0029 states for the same reason.
Measured now, and reproducible from this ADR (2026-09-05, origin/main @
1fe3302b):
- The published
review.*tool table has 7 rows, one shipped and six planned, which is why serving needs no new name:
console
$ grep -n '^| `review\.' docs/protocol/mcp-tools.md
352:| `review.findings` | Shipped | Landed `Review-Finding:` trailers, filtered by reviewer, severity, commit or text |
353:| `review.search` | Planned | Search review history |
354:| `review.getThread` | Planned | One thread with comments and resolution |
355:| `review.findSimilar` | Planned | Threads resembling a described situation |
356:| `review.getDecisions` | Planned | Decisions reached in review |
357:| `review.generateKnowledgeCandidate` | Planned write-intent | Emit a proposal; no approved-state write |
358:| `review.listUnresolved` | Planned | Open threads |
Slice 3 did what this measurement predicted: the review.search row moved from
Planned to Shipped in place, and no row was added. The block above is left
at its own commit, because a measurement re-taken every time the tree moves
stops being a measurement.
ReviewResolutionhas no consumer outside its defining module: two hits, both indomain/review.py(git grep -n "ReviewResolution" -- packages/theurian-core/src).- The core's runtime dependency list is 6 packages, none an HTTP client
(
packages/theurian-core/pyproject.toml). - Three populations slice 1 must move, each with its key in Dispositions and
each a dispatch input to be re-measured then: the fetch-absence prose
(30 files, wide phrase key), the
providers.review.repositorieskey (15 files), and the Milestone-7 attribution (the demoted union above — its numbers live with the annotation, per Demoted populations). The first two overlap in 12 files on this branch (11 at1fe3302b), which is why the allowlist reader's commit is sequenced serially before the adapter's rather than fanned out — the split is justified bytest_config_key_call_sites.py's pinned absence, not by disjointness. - This ADR is a member of both prose populations it measures, which is why every one of those counts is stated against a named commit rather than as a property of the repository.
- The scope claim is a stable property, not a snapshot. "Only paths under
docs/" was true when it was written and false by the next fix commit — a branch-scope measurement is invalidated by the commits that answer the review, which is what made it wrong for four rounds. What holds for the whole branch and keeps holding:
console
$ git diff origin/main...HEAD --name-only -- ':(glob)packages/*/src/**' 'plugins' 'schemas' | wc -l
0
No shipped source, no plugin, no schema. The branch does touch four non-docs/
files, each non-behavioural and each named here rather than counted:
SECURITY.md (a governed record, decision 3);
tests/unit/test_phase0_exit_records.py (one docstring word, 29→30);
tests/unit/test_project_and_traceability.py (a parametrised illustration
string, target unchanged — is_derived keys on the first path segment); and
tools/audit/owner_position_cites.py (ledger rows and their reasoning, which
the audit reads as data).
Quoted, not re-run here — design consult, 2026-09-05, gh 2.86.0: runs A, B
and C; the PullRequestReviewThread field list; the observation that
http_unix_socket is a configuration key on that version while api_host is not;
and the 0/0 thread count over the last-40-merged window. Round one's adversarial
review, same date and binary: runs D, E and F, and the by-PR-number thread
population (11 threads across #12, #132, #224, #352, #569, plus 5 top-level
reviews) — run D and the thread population were re-run by the orchestrator and
reproduced. Slice 1 re-runs D–F as driving tests — that is where they become controls
instead of quotations. A–C cannot be re-run in a test: each one needs a real
outbound request to api.github.com or a hostile host, which no suite here makes.
They stay quoted, and they are re-taken by hand when clause 8's version floor
moves, since what they measure is a property of the binary.
Owed at implementation, each tied to the slice that discharges it. A slice that lands moves its own items from owed to landed here and names the test that discharges each; an item a slice did not reach stays on the list with the slice that will, and is never quietly dropped.
Slice 1 — ingest (shipped at 7c486588. The discharges below were filled in
during slice 3 (#603), which
is late: the list stood as owed for two slices while the tests existed, and a
list that says "owed" about work that shipped is as wrong as one that says
"landed" about work that did not. Paths are packages/theurian-core/tests/, and
every name here was resolved against the tree rather than written from the
slice's own brief. Three items are not discharged by a test and say so by
name.)
- Landed in slice 1 — ten clause tests, one per row of decision 1's table.
Clause 1, the single spawn site:
unit/test_network_call_sites.py::test_no_module_outside_the_recorded_spawn_sites_can_start_another_program(the equality pin, grown by one) with::test_the_process_scan_sees_each_spawning_form_and_no_otheras its positive control, andunit/test_gh_argument_vector.py::test_only_the_recorded_modules_name_the_spawn_helper_or_the_class_that_calls_itfor the second half of the clause. Clause 2, the literal endpoint with identity in variables:::test_the_endpoint_element_is_the_literal_graphqland::test_no_vector_element_is_built_by_formatting_a_repository_name. Clause 3, the host pin:::test_the_hostname_is_pinned_in_every_vector_that_makes_a_request. Clause 5, the absolute binary and no shell:::test_the_first_vector_element_is_an_absolute_pathand::test_the_spawn_module_reaches_no_shell. Clause 6, no--paginate:::test_paginate_is_absent_from_every_vectorand::test_a_second_page_changes_only_the_cursor, with the cursor read itself atintegration/test_gh_review_provider.py::test_a_second_page_is_asked_for_with_a_cursor. Clause 7, the timeout and the two caps as named constants:unit/test_gh_argument_vector.py::test_each_recorded_bound_is_the_value_the_prose_namesand::test_every_recorded_limit_has_a_test_side_home, with the graded stops atintegration/test_gh_review_provider.py::test_a_limit_past_the_recorded_cap_stops_before_spawning,::test_a_response_that_never_stops_paging_is_stopped_by_the_page_capand::test_a_thread_past_the_comment_cap_is_reported_not_truncated, and the timeout atintegration/test_gh_bounded_read.py::test_a_child_that_never_answers_is_stopped_at_the_recorded_timeout. Clause 8, the version floor:integration/test_gh_review_provider.py::test_a_gh_below_the_floor_is_refused_and_the_message_names_the_floor. Clause 9, the two graded envelopes with the child's stderr contained:::test_no_gh_on_the_path_is_a_graded_refusal_not_a_traceback,::test_an_unauthenticated_gh_is_a_graded_envelope_carrying_its_own_stderrand::test_a_failed_query_reports_the_childs_own_words_inside_the_envelope. Clause 10, the per-response byte cap and the read shape that makes it real:integration/test_gh_bounded_read.py::test_a_child_that_overruns_the_cap_is_refused_without_waiting_for_it_to_finish— the bounded wait is what separates an incremental read from an accumulating one — with::test_a_child_at_the_cap_exactly_is_read_rather_than_refusedand::test_a_cap_refusal_leaves_no_child_behind. - Landed in slice 1 — clause 4's tests (i), (ii-a) and (ii-b), exactly as
clause 4 specifies them. (i) is
unit/test_gh_child_environment.py::test_the_child_environment_is_exactly_the_recorded_mapping, written test-side rather than read from the production constant — which is what makes it killable — with::test_the_expected_mapping_can_failas its control andintegration/test_gh_review_provider.py::test_the_child_receives_the_constructed_environment_and_nothing_elseobserving the same mapping on the far side of the process boundary. (ii-a), the refusal driver that needs nogh, is::test_a_planted_transport_override_refuses_before_any_binary_probe. (ii-b), the residual demonstration through a test seam, isintegration/test_gh_transport_residual.py::test_with_the_refusal_bypassed_the_request_leaves_through_the_socketwith::test_without_the_seam_the_same_fixture_reaches_the_socket_zero_timesas the control that makes the zero mean something; only (ii-b) skips, and it reports the skip. The rationale lives in clause 4 and is not restated here: round two corrected it in the clause and left this bullet carrying the superseded version byte-unchanged, which is the two-places class this ADR keeps meeting. (An earlier version of this bullet cited "(i)'s companion" as a fourth item; the companion is a property of (i)'s form, not a separate test, so the citation resolved to nothing.) - Landed in slice 1 — the allowlist is consulted before the spawn.
integration/test_gh_review_provider.py::test_an_unallowlisted_repository_starts_no_processasserts the spawn recorder is empty rather than that the result was filtered; the refusal itself isunit/test_review_allowlist.py::test_an_unlisted_repository_is_refused_with_its_grade, and::test_an_empty_allowlist_allows_nothingholds the direction an absent list fails in. - Landed in slice 1 — a private repository is refused at ingestion.
integration/test_gh_review_provider.py::test_a_private_repository_is_refused_at_ingestion, with::test_a_repository_that_does_not_resolve_as_public_is_refusedfor the answer that is neither public nor private and::test_a_private_repository_is_refused_on_the_reviews_read_toofor the second read, which would otherwise be a seam the first test never reaches. - Landed in slice 1 — a renamed repository is refused.
integration/test_gh_review_provider.py::test_a_rename_redirect_is_refused_rather_than_followed, held from the other side by::test_a_resolved_name_contained_in_the_entry_is_still_a_different_repositoryand::test_a_case_difference_is_not_a_rename— without the second, a check that refused every case difference would pass the first while breaking ingestion of a correctly allowlisted repository. - Landed in slice 1 — evidence paths are containment-safe before the key is
load-bearing. The schema-pattern half is
unit/test_review_allowlist.py::test_the_pattern_this_module_enforces_is_the_one_the_schema_publishesand::test_a_traversal_or_malformed_name_is_refused; the path half isunit/test_project_paths_containment.py::test_every_path_helper_refuses_when_a_committed_symlink_escapes_under_it, whose population is derived rather than listed by::test_the_containment_sweep_covers_every_path_returning_helper, soProjectPaths.reviewjoined that sweep by existing. - Landed in slice 1 —
test_network_call_sites.py's absence claim is retired in the same commit that admits the site. Same pin as clause 1:unit/test_network_call_sites.py::test_no_module_outside_the_recorded_spawn_sites_can_start_another_program, an equality that fails on an addition and a removal, with::test_the_process_scan_sees_each_spawning_form_and_no_otherproving the scan can see one. The file's own admission checklist is prose beside the pinned set and is not separately held. - Landed in slice 1 — a pre-spawn refusal of transport-override keys in the
precedence-resolved gh config — best effort, reducing only the accidental
single-well-formed-file case and leaving decision 1's four-member divergence
class, with clause 4's tests as its drivers. Its spec: the same precedence chain
ghresolves (empty-string variable treated as absent), and the check runs before any binary probe.unit/test_gh_transport_guard.py::test_a_planted_transport_override_is_refused_before_anything_is_spawnedis the refusal; the precedence chain is::test_gh_config_dir_wins_over_both_others,::test_an_empty_gh_config_dir_falls_through_exactly_as_gh_doesand::test_the_override_is_seen_through_every_locator_gh_resolves; and::test_a_configuration_that_moves_nothing_refuses_nothingis what stops the check from being satisfied by refusing everything. - Still owed, and not a test obligation — the known-key set is re-taken
whenever clause 8's version floor moves. Member (b) is bounded only by what
that version of
ghunderstands, so nothing in this suite can discharge it: a test can only hold the set this build looks for, whichunit/test_gh_transport_guard.py::test_the_known_key_set_is_what_the_check_looks_fordoes. The obligation is a hand re-measurement against the new binary, owed by whoever raises the floor constant, and it is recorded here rather than dressed as discharged. - Landed in slice 1, prose with one pinned half — every T-7 sentence is
rewritten per control, recording the repository allowlist as discharged
on the
ghpath and private-network rejection as reduced with its residual class named (decision 1's four-member divergence class), never as discharged, with #429 still owning the raw-URL context. The only half a test holds is the spawn-site enumeration:unit/test_threat_model_t7_claims.py::test_the_t7_spawn_bullet_names_every_pinned_spawn_site_and_spells_how_many. That the per-control rewrite is faithful is a reading, and no mechanical check reaches it — said here rather than left to be inferred from a name beside it. (The three line-anchored cites this bullet carried —threat-model.md:6454,requirements-analysis.md:1352,roadmap.md:272— are dropped under this document's own Demoted populations rule: a line number does not survive an edit to the file above it, and the sentences are findable by the control each names.) - Landed in slice 1, and discharged by a measurement rather than a test — the
three populations move in this PR: the fetch-absence prose, the
providers.review.repositoriessentences, and the Milestone-7 attributions. Each key was re-run on the branch and its output pasted into PR #583's Populations, re-measured section, which is the record; nothing in the suite re-runs them, and a later drift is caught by the next assignment's re-measurement rather than by a RED. - Landed in slice 1, pinned in slice 3 — the sites recording the old
reviewIngestionmeaning are rewritten — the key, its exclusions and its output are in decision 2 — two slices before the flag flips. Test sites are among the movers, one of them a byte-pinned constant, so the sweep is not a prose pass. The annotated population and its composition live where the brief does (Demoted populations), not in a count here that would go stale beside a correct key. The sweep left nothing holding the result, which is what slice 3 added:unit/test_review_ingestion_flag_claims.py::test_the_four_sites_spell_the_never_meant_sentences_one_wayholds the four sites' shared sentences against each other, and::test_each_record_narrates_the_flag_the_capability_dict_publishesholds each narrating record to the era the live flag value selects, with::test_the_docstring_checker_demands_the_other_era_when_the_flag_movesas its control. Its population is enumerated, not derived: a ninth narrating record would not redden it, and that bound is recorded in the module's own docstring. - Half landed in slice 1 — the domain-model break is recorded. The CHANGELOG
half landed:
packages/theurian-core/CHANGELOG.md's### Changedsection (the level the file actually uses; this item said#### Changed) carries twoBREAKINGentries naming the old shape and the new one, forReviewResolutionand forReviewEvent. The commit half did not. The merged commit7c486588records the break as prose inside its body — "(BREAKING: ReviewResolution's field order and optionality — no consumer existed to migrate)" — and carries noBREAKING CHANGE:footer;git log origin/main --format='%B' | grep -cE '^BREAKING CHANGE:'answers 1 across the whole history, and it is not this commit. History is not rewritten for it, so what is owed is the next break on this path taking the trailer, and this item is left here saying so rather than marked landed. No test holds either half.
Slice 2 — land
Four of the five landed in slice 2 and each names the test that discharges it;
the fifth was half discharged there and its second half landed in slice 3,
named in place below. Paths are packages/theurian-core/tests/.
- Landed in slice 2 — a flagged record under
blocknever becomes a file.unit/test_review_landing_gate.py::test_under_block_the_flagged_record_never_becomes_a_fileplants a synthetic secret and asserts the file is absent;::test_no_report_line_carries_the_matched_bytesholds the report to identities and a bounded family prefix; and::test_under_block_a_flagged_record_does_not_stop_another_pull_request_landingholds the refusal to one record. That the run does not read as clean is the CLI's half:integration/test_review_ingest_cli.py::test_block_exits_one_and_the_flagged_unit_is_absent_on_disk. - Landed in slice 2 —
warnlands and reports;offscans nothing.unit/test_review_landing_gate.py::test_under_warn_the_record_lands_and_every_finding_is_reportedand::test_under_off_the_detector_is_never_called, the second with its own positive control (::test_the_counter_would_have_seen_a_scan_under_the_default_policy) so that "never called" is not satisfied by a detector nothing reaches. The CLI's exit codes areintegration/test_review_ingest_cli.py::test_warn_exits_zero_and_still_reports_the_findingand::test_off_scans_nothing_and_lands_everything. - Landed in slice 2 — the scan reads exactly the author-controlled fields of
decision 3's table.
unit/test_review_landing_gate.py::test_a_secret_planted_in_any_author_controlled_field_refuses_the_recordis parametrised one case per field, so a label and a head branch name are driving inputs rather than an argument; the boundary is held from the other side by::test_a_secret_shaped_value_in_a_structural_field_does_not_refuseand::test_the_url_is_structural_even_though_a_person_can_choose_a_branch_in_it, without which a gate that scanned every string on the object would pass every positive case. - Half landed in slice 2 — evidence files are the source. The write→read-back
half is
unit/test_review_evidence_store.py::test_every_record_kind_reads_back_as_the_object_that_was_written, and the survival half is::test_a_record_upstream_no_longer_returns_survives_the_refetchwith::test_a_refetch_rewrites_a_record_whose_content_changed_upstreamas the positive control that keeps a store writing nothing at all from passing it. The second half landed in slice 3, which is when it first became expressible — there was no derived store to delete until slice 3 built one:integration/test_review_search_rebuild.py::test_a_deleted_store_rebuilds_to_the_same_answers_from_the_same_evidencethrows the store away, rebuilds from the files alone and compares the served answers, with::test_two_builds_agree_whatever_order_the_evidence_records_arrived_inand::test_the_served_order_does_not_depend_on_the_order_records_were_writtenholding the reproduction against the one input a filesystem is free to vary. - Landed in slice 2 — FR-V5, made checkable.
integration/test_review_ingest_is_model_free.py::test_no_callable_in_the_built_pipeline_reaches_a_modelwalks the built ingest pipeline's object graph in the shape oftest_no_registered_tool_can_reach_a_canonical_write, with::test_the_walk_reaches_the_pipeline_it_claims_to_inspectproving the walk is not vacuous and three planted-model cases (::test_a_model_planted_on_a_service_method_reddens_the_walkand its two siblings) proving it can go RED. Decision 5's "no model exists anywhere in the ingest path" is therefore a measured property rather than design intent — with the bound that module's docstring states: a provider resolved through a factory one level down is invisible to it.
Slice 3 — serve
Five of the six landed and each names the test that discharges it; the sixth is
conditional and its condition is unmet, which is a different state from owed
and says so. Paths are packages/theurian-core/tests/.
The slice closed at round five under the fixed-round-budget ruling, with one HIGH open and release-gating (2026-09-11). CRITICAL was zero throughout; #636 — the empty-publish guard keying on read-time facts while the build held the publish-time listing that decides them — was filed rather than fixed and was the first post-merge item. It touched none of the discharges below: no arm of the two-corpora equality, the triple, the flag, SEC-13 or the bound-input battery rests on that guard.
That HIGH is discharged, so the 0.2.0 cut is no longer waiting on it
(PR #637). The guard is keyed on
at_the_publish — the listing taken inside the write section — together with the
withholding outcome, and on nothing the read alone saw, which is what separates a
stale build from an operator's deletion: a build that read nothing while the
corpus is there at the publish refuses instead of emptying a store a landing had
just filled, and a corpus emptied whole inside another build's window publishes
the empty store instead of being refused while the store goes on serving records
whose files are gone. Both faces are driven through the shipped CLI by
integration/test_review_build_empty_publish.py::test_a_build_that_read_nothing_refuses_rather_than_emptying_a_store_a_landing_just_filled
and
::test_a_corpus_an_operator_emptied_inside_the_window_is_published_rather_than_refused,
which hold the read-to-publish window open with a barrier at the read seam and
assert what the read returned rather than relying on a race; the two-process
reproduction that filed the issue is #636's own and is not re-run there, which
that module states as its cost. The rows nothing else reached are
integration/test_review_search_builder.py::test_a_build_that_read_nothing_while_a_landing_filled_the_corpus_refuses
and
::test_a_build_that_kept_none_of_what_it_read_publishes_when_the_corpus_is_gone,
the withholding arm is
::test_a_build_that_withheld_everything_publishes_whether_the_files_are_there_or_not
with ::test_an_all_withheld_build_and_a_purpose_emptied_corpus_are_one_observable.
That pairing's "one observable" is three named ones and not every observable,
so this record states the bound rather than the phrase: what an all-withheld
build and a build whose corpus is gone at the publish may not differ in is
whether the build raises, what the store holds, and what the report publishes —
every key of it but withheld. The axis is the listing and not an operator's
intent, which is the axis the guard reads: a corpus still there while the read
found nothing is the refusing input, and whether it never held the records or
somebody had just emptied it is not something entries can say. The case
standing on that row is
integration/test_review_build_empty_publish.py::test_a_build_over_an_emptied_corpus_a_writer_landed_into_refuses_and_leaves_the_store.
Duration is outside that claim and was measured, not argued away: the
all-withheld build parses every evidence file and a corpus that is gone has
nothing to parse, so the two differ in duration. Two figures were taken in
round one, and they are two instruments rather than two points on one curve.
1.75× at 60 records is wall-clock over the shipped CLI, where a
process-startup floor of roughly 27 ms sits in both numerator and denominator and
dilutes the ratio (security review's instrument); 29.8× at 200 records is the
builder method driven directly, with no process to start (adversarial review's
harness). Neither number extrapolates to the other's instrument, and no curve is
claimed here. It is not a channel: the only parties that can time this build are
the ones that ran theurian review build or theurian review ingest in their
own working tree — both reach it through review_commands.rebuild_search_store —
and either can read .theurian/review/ directly, while no MCP tool reaches the
build at all (review.search reads the store a build produced). What would
re-grade it is a caller able to provoke a build without reading the corpus, and
none exists.
The key itself is pinned from both sides by
unit/test_review_search_builder_claims.py::test_the_empty_publish_guard_is_keyed_on_the_publish_time_capture
and ::test_the_build_records_the_decision_its_guard_makes.
- Landed in slice 3 — every author-controlled field carries the SEC-15 triple,
bound by import.
integration/test_review_search_tool.py::test_every_served_record_carries_the_imported_safety_triplecompares a served row againsttheurian.mcp.results.SAFETYitself rather than against three re-spelled literals, with::test_the_triple_assertion_reddens_on_a_re_spelled_drifted_tripleas the companion that proves the check can fail and::test_the_shaper_follows_the_imported_object_and_not_three_typed_literalsholding the shaper to the imported object. The population is decision 3's field table:::test_every_author_controlled_field_is_either_served_under_the_triple_or_not_servedis what reddens for a field added there without a disposition, with::test_the_corpus_really_plants_each_unserved_fieldkeeping it from passing over fields no fixture carries, andunit/test_schemas.py::test_the_published_review_record_fields_are_the_ones_the_shaper_classifiesand::test_the_published_review_record_safety_triple_is_the_one_the_code_attacheshold the published schema to the same classification. - Landed in slice 3 — the two-corpora equality, at both layers. At the store:
integration/test_review_search_absence_proof.py::test_every_query_in_the_battery_answers_identically_over_the_two_corpora, with::test_no_generated_query_separates_a_withholding_store_from_one_that_never_held_the_recordsas the generated-input arm. At the tool, which is the layer a caller actually reaches:integration/test_review_search_tool_absence_proof.py::test_every_query_in_the_battery_answers_identically_over_the_two_corpora, with::test_the_battery_really_reaches_the_withheld_recordsand::test_the_control_serves_the_withheld_records_through_the_very_same_toolkeeping the equality from being satisfied by a battery that reaches nothing, and::test_no_generated_request_separates_the_two_corpora_at_the_tool. Three channels beyond the field values are held separately, because "every published field" does not cover them:::test_a_withheld_record_never_costs_a_visible_one_its_slot_in_the_response(displacement),::test_the_page_boundary_bit_does_not_move_with_a_withheld_record(truncated), and::test_both_published_copies_of_one_response_carry_the_same_payload— which exists because the SDK sends the payload twice and a comparison that read only one copy was comparingNonetoNone. - Conditional, and the condition is unmet — a ranked surface over review
evidence ranks the T-17a-purged population, if one is built. None is built:
this slice's read has no score, no term weight and no collection statistic, so
there is no build-time statistic for a withheld row to price, and that absence
is structural rather than asserted —
integration/test_review_search_absence_proof.py::test_the_store_schema_carries_no_full_text_index_for_a_withheld_row_to_pricerecomputes it from the store's own schema, with::test_no_trace_of_a_withheld_record_survives_in_any_table_or_any_byte_of_the_storeholding the physical absence the argument rests on. The item stays on this list for whoever builds a ranked surface; it is not discharged, because a filter does not clean FTS5 collection statistics and nothing here proves it would. - Landed in slice 3 — the capability flag and its scope field.
integration/test_mcp_tools.py::test_capabilities_report_what_is_and_is_not_builtholds both values together,::test_the_capability_block_holds_exactly_the_flags_that_are_pinnedand::test_the_system_capabilities_response_holds_exactly_the_keys_that_are_pinnedare the two equality pins the slice had to move to land the flag, and the wire half isintegration/test_wire_contract.py::test_a_real_capabilities_response_validates_against_its_published_schemawith::test_the_capabilities_conformance_check_can_fail. The response schema it validates against is new in this slice. (This bullet cited the flag-value assertion by line number,:1980; the line has moved, and the pins are named here instead — see Demoted populations.) - Landed in slice 3 — SEC-13 cross-project isolation over review evidence.
integration/test_review_search_tool.py::test_a_review_search_for_one_project_cannot_observe_the_other. The refusals are held to the same standard, since a message that varied with a project's contents would be the same leak one layer over:::test_a_bad_filter_is_refused_the_same_way_whether_or_not_the_project_resolves,integration/test_review_search_tool_absence_proof.py::test_an_empty_corpus_refuses_like_a_full_one_and_serves_a_well_formed_query, and the provenance armintegration/test_review_search_tool.py::test_a_store_this_installation_did_not_build_is_refused_in_the_words_a_missing_one_gets. - Landed in slice 3 —
review.searchquery input is bound, not interpolated.integration/test_review_search_bound_input.py::test_a_sql_statement_in_a_filter_is_a_value_and_alters_nothingand::test_a_wildcard_in_a_structural_filter_is_a_character_and_not_a_patternhold the binding;::test_operator_shaped_search_text_selects_only_the_record_that_spells_itholds operator syntax as text rather than as query structure — the ADR wrote that as FTS5 syntax, and the surface that shipped is aLIKEwith no query language at all, so what is driven is the operator vocabulary a caller would reach for against either; and::test_every_text_field_of_a_query_refuses_a_value_sqlite_cannot_be_handedholds the two byte shapes that cannot cross the boundary. At the tool:integration/test_review_search_tool.py::test_operator_syntax_in_the_query_arrives_at_the_store_as_text.