fix(reward): derive what the receipt asserts instead of believing the bundle and the trainer - #463
Merged
Merged
Conversation
… bundle and the trainer The reward worker reads a store and signs what it read. Five paths let the signature say something the read did not support. 1. The tier was a string the bundle author wrote. `json_file` and `screen_dump` build the same `JsonDocumentOracle` over the same kind of bytes, and one document earned tier 0 with `development_only` under one kind and tier 2 with `certified` under the other. The channel is now a class attribute of the adapter, both kinds read through `ocr`, and `build_oracle` refuses any adapter whose channel differs from the recipe table's. `sqlite` keeps tier 2 and now checks the file header, so a screen dump renamed `store.db` is refused. The seeded tier-2 bundle moves to a real SQLite store. For `rest` and `fhir` the worker verifies that it made a network read and not that the endpoint is authoritative; that claim is narrowed in the docs rather than enforced. 2. The graded subject was chosen after the rollout. `declared_identity or registered_identity` let a descriptor name a different patient from the one `begin_episode` registered, and the receipt carried the trainer's. Registration now wins and a conflicting descriptor gets 422 `identity_conflict`. Re-registering an episode under another subject, or after it is scored, is refused too. 3. A rollout that did nothing could earn 1.0. The seeded required effect was a plain `record_written`, a statement about the store's current contents, so a subject whose row already existed scored full reward with no episode having run. `RewardBundle.load` now refuses a contract whose required effects include no claim about change, and the seeded effect sets `count_new_only`. 4. Certificate expiry counted a number the counterparty reported. 0, 999, 10^9, then 0 again returned `current` for the last one. The worker keeps a per-contract high-water mark under `<data-dir>/policy_updates/` and refuses a descriptor below it with 422 `policy_update_regressed`. The mark is per contract, not per checkpoint, because per checkpoint a trainer could rename its checkpoint and reset the count. 5. The calibration corpus was hard-coded. `faulted_store` always emitted `type: Triage`, so a contract for any other record type matched nothing, recorded zero false accepts, and received the best epsilon the method can produce. The corpus is now read off the contract's own required and forbidden effects, `extradup_trials` refuses to report a bound unless a clean store built that way earns VERIFIED, and the corpus digest is derived and checked at load time against both the policy and the certificate. `wrong_subject` joins `FAULT_CLASSES`, so the bound now samples the write that landed on somebody else. Adds `POST /v1/episodes` so the HTTP path can register an episode before the rollout, which a mandatory baseline now requires. Also corrects two published sentences: the README advertised a `reward` extra and a `serve-reward` command that no published release carries, and `docs/REWARD_WORKER.md` described certificate revocation, which nothing implements. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An adversarial review found five ways to make the reward receipt assert
something the oracle read did not support. Each one is reproduced below with
its real output before and after. Every fix adds a refusal; none removes one.
The shape is the same in all five: the worker trusted a label or a number its
counterparty supplied. The bundle author chose the oracle tier by naming a
recipe kind. The trainer chose the graded subject after the rollout, and chose
the policy update that decides whether the certificate has expired. The
seeded contract asked about the store's current contents instead of about the
episode. The calibration corpus was a constant, so the bound did not depend on
the contract it was issued for.
1. The oracle tier was a string the bundle author wrote
json_filemapped to channelfile(tier 2) andscreen_dumptoocr(tier0), and
build_oraclehanded both the sameJsonDocumentOracle. Onedocument, two tiers, on the same bytes:
The channel is now a class attribute of the adapter, and no caller sets it.
Both kinds read through
ocrat tier 0, andbuild_oraclerefuses to returnan adapter whose channel differs from the recipe table's, so the table in
models.pyand the adapters inoracles.pycannot drift apart quietly.Tier 0 is the floor a local JSON document can defend. Whoever writes the file
writes the answer, and nothing in the bytes tells a system-of-record dump from
a screen scrape.
sqlitekeeps tier 2 because the worker opens a real database read-only andruns one SELECT through the engine, and it now checks the file header, so a
screen dump renamed
store.dbis refused before the worker starts. The seededtier-2 bundle moves from
mockmed/records.jsonto a real SQLite store atmockmed/records.db.Narrowed rather than enforced:
restandfhirstill reach tier 2 bypointing at any HTTP server that answers with JSON. The worker can verify that
it made a network call. It cannot verify that the endpoint is the customer's
system of record rather than a server the trainer stood up, and a self-signed
synthetic-scope certificate does not attest to it.
docs/REWARD_WORKER.mdnow says so under "What the tier rests on" and names whoever admits the bundle
as the owner of that check. Adding a hostname or scheme rule would have looked
like enforcement without being any.
2. The graded subject was chosen after the rollout
worker.py:331readidentity = declared_identity or registered_identity, sothe descriptor won over the registration and nothing compared them:
The registration is made by the environment before the rollout, when nobody
knows how the episode will end. The descriptor arrives after, when the trainer
does. So the registration decides, and a descriptor that names a different
subject is refused rather than silently overridden, because a trainer that
believes it is grading one subject while the worker grades another has a bug
worth stopping for.
Re-registering the same episode under the same subject is still allowed and
only re-reads the baseline. Re-registering an already scored episode is
refused.
3. A rollout that did nothing could earn reward 1
The seeded required effect was a plain
record_written, a statement about thestore's current contents. No episode had to run:
RewardBundle.loadnow refuses a contract whose required effects contain noclaim about change.
count_new_onlyandexact_new_setare the two kinds thejudge settles against the pre-episode baseline; everything else describes the
store as it stands. The required effects are judged as a conjunction, so one
change claim is enough for
verifiedto mean the episode added a record, and afield_equalsread-back can still ride alongside it.Narrowed rather than enforced: the task allowed an escape hatch for a
contract that says explicitly it is state-only. I did not add one. The bundle
is written by the same party the rule constrains, and an opt-out that party
writes is defect 1 in a different costume. The claim is narrower instead: this
worker serves contracts whose required effects assert a change, because
verifiedis a statement about an episode and a state-only contract cannotmake one.
This is also why
POST /v1/episodesis new. A baseline is now needed for ascored episode,
begin_episodewas in process only, and without a route theHTTP path could never produce one.
4. Certificate expiry counted a number the counterparty reported
episode.policy_updatewent from the wire straight tocertificate_statewith no comparison to anything seen before:
The worker now keeps its own high-water mark under
<data-dir>/policy_updates/, beside the episode index, and refuses anythingbelow it:
Stronger than asked, and here is the sixth defect. The task said to key the
mark per contract and per policy checkpoint. Per checkpoint alone, a trainer
resets the counter by calling its checkpoint something else: register a fresh
policy_checkpoint_id, sendpolicy_update: 0, and an expired certificatereads
currentagain. The mark is therefore per contract, andtest_renaming_the_checkpoint_does_not_reset_expirydrives exactly that. Theledger still records which checkpoint set the mark, for the error message.
5. The calibration corpus was hard-coded
calibration.py:132 faulted_storealways emitted{"id", "patient_id", "type": "Triage", "status": "saved"}, whatever thecontract asked for:
The contract matched nothing, so every trial refuted for a reason the planted
fault did not cause, the false-accept count was zero because the checker
rejected everything, and the certificate got the best epsilon 300 trials can
produce. The same 0.009936 a correct contract gets.
corpus_from_effectsnow reads the records off the contract's own requiredand forbidden effects. A
field_equalsread-back on the same selector mergesinto the record a
record_writteneffect already describes, so a pairedcontract plants one row rather than two. Each trial gets a real pre-state, so
count_new_onlyhas a baseline to work against.Three refusals now stand behind that number.
extradup_trialsruns a controltrial first and refuses to report a bound unless a clean store built from the
contract's own effects earns
VERIFIED, so a zero count that only means "thechecker rejects everything" cannot become a certificate. The corpus digest is
derived from the contract, and
RewardBundle.loadrefuses a certificate or acertificate policy that names a different one. And a contract no fault class
applies to cannot be calibrated at all.
calibration.jsonbeside the certificate now records the corpus digest andwhich fault classes were sampled, so a reader can see what the bound covers
rather than assume.
FAULT_CLASSES: I added the class rather than correcting the docstring.wrong_subjectis now sampled, planting the required record correct in everyfield under another subject's identity.
RewardOutcomeV1.WRONG_EFFECTcallsitself "a terminal effect that differs from the required one", the judge
already catches that mode, and a bound that never planted it was quiet about
the failure this reward exists to price. The docstring is in
openadapt-typesand I did not touch it; it is now accurate as written.
wrong_typeis renamedwrong_field, because on a derived corpus it spoilswhichever declared literal the contract has rather than a hardcoded
typecolumn.
What the seeded MockMed run looks like now
The demo got sharper rather than weaker. Because a required effect asserts a
change, an episode has to write something, and the two channels can be pointed
at the same episode:
The OCR dump agrees with the banner while the database holds nothing. That is
the whole argument for the tier ladder, and the fixture now shows it on one
episode instead of two.
Two published sentences corrected
README.mdadvertisedpip install 'openadapt-flow[reward]'andopenadapt-flow serve-reward --seed-mockmedon the GitHub landing page.Published 1.34.0 declares no
rewardextra, ships noopenadapt_flow/reward/,and registers no
serve-rewardentry point, so a reader got a failed installand then an unknown command. Both commands stay as what will work when the
release lands, with no version and no date, and a repository-head install is
added for today.
docs/REWARD_WORKER.mdgets the same treatment, matching theshape openadapt-ops#212 used for the docs page.
docs/REWARD_WORKER.mdline 29 said the control service "issues and revokesreward certificates". Nothing implements revocation. The sentence now says
expiry is the only thing that ends a certificate.
Tests
tests/test_reward_trust_boundary.pyis new: 21 tests, each driving areproduction above.
tests/test_reward_worker.pyis updated so its outcometests register an episode and write what the episode would have written,
rather than scoring a store that was already correct. 53 tests pass; ruff,
ruff format and mypy are clean on
openadapt_flow.Opened by an agent session, not the founder.
🤖 Generated with Claude Code