Repository navigation
Release/10 6 26 - #148
Merged
Merged
Release/10 6 26#148
Conversation
Issuer.discover and getKeySet stand in for external systems for the life of a test file, but were installed with jest.spyOn, which puts them in the registry jest.restoreAllMocks() clears. Two integration specs call that in an afterEach, so from their second test onward those two mocks were gone. getKeySet survives on a technicality — onModuleInit caches the key set before any test runs. Issuer.discover does not: registration is not a bootstrap-only event, since a pg notification re-runs refreshRegistrations, so a test that causes an IdP to be registered reaches the real implementation and tries to resolve the fixture hostname. Assigning a jest.fn() instead leaves both out of the registry entirely, which is already how the FileService stubs survive. Verified with a probe spec that calls restoreAllMocks and then Issuer.discover: it fails with getaddrinfo ENOTFOUND idp-a.example.com before this change and passes after. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* add id_matching_mode to partner and job
Runway will select how student identities are resolved per partner, and the
executor needs a value that can't change mid-flight. A partner-level setting
alone would let an edit between job creation and a re-run silently change how
an existing job behaves, so the job carries a snapshot taken at creation.
The column defaults are migration safety only — existing rows stay id_based
without a backfill. Job creation will write the snapshot explicitly.
idMatchingMode joins GetJobDto's intentionally-not-exposed block: the type
requires every Job field, but there is no matching-mode UI in this rollout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* hand IDRS credentials to the executor just in time
The executor needs a partner-scoped IDRS bearer token, but the job payload is
fetched once at container start and can outlive any token we could put in it —
and a token sitting in that payload is one more copy of a credential in flight.
So the payload carries only a callback URL, and the executor exchanges its
run-scoped bearer token for credentials immediately before it needs them.
That run-scoped token is the whole trust boundary: it names the run, which
names the partner whose connection info may be returned. The callback adds no
matching-mode, run-status, or one-call restriction on top — background fuzzy
work legitimately runs after the authoritative run reports done, and an
id_based executor calling the unadvertised URL just gets a 500 and carries on.
Token minting is bounded so it fits inside the executor's 20s request timeout:
5s for the secret lookup (SDK retries included, via a per-request abort signal
so no other secret getter inherits the bound) and 5s per OAuth attempt with one
jittered sub-second retry for transient failures only. Tokens are cached per
partner per app instance, with concurrent misses coalesced onto one lookup so a
cold start doesn't stampede Secrets Manager.
The ten-minute buffer is a reuse threshold, not a minimum lifetime — a token
too short to clear it is still returned, just not cached, since caching it
would only make the next caller mint another one. Errors collapse to three
broad responses; diagnosis comes from the structured logs, which carry cause
categories and safe upstream metadata but never credentials, tokens, OAuth
bodies, or the callback response.
Pure fuzzy does no roster matching, so it gets neither appUrls.roster nor
rosterFilePath. Background mode still runs the authoritative ID-based pass and
keeps both.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test the matching-mode snapshot and the identity-service callback
Integration tests cover what the executor actually sees: the mode on the
payload, whether the callback URL is advertised, which roster sources survive
each mode, that a run follows its job's snapshot after the partner setting
moves, and that the callback resolves the right partner, stays open after done,
and maps failures to the three-way contract. The callback tests stub only the
AWS and OAuth boundaries so routing, auth, partner resolution and error
mapping all run for real.
Unit tests carry the parts integration can't reach: the exact token request,
the cache reuse window, the short-lived and missing-expiration branches,
per-partner isolation, miss coalescing and its failure cleanup, and which
OAuth failures retry. Both layers assert the logs don't leak credentials or
tokens, using distinctive values so a leak can't hide in ordinary log words.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* classify malformed secret and OAuth responses
typeof null === 'object', so a secret or token body of JSON null reached the
destructuring and threw a raw TypeError. IdentityServiceTokenService only
translates its own typed errors, so that surfaced as an unclassified 500 with
no cause-category log — the two things the error contract exists to prevent.
Arrays slipped through the same check. Reject both at each boundary.
Secret field values are validated as non-empty strings rather than merely
truthy. Secrets Manager content is untrusted at runtime whatever the type
says, and a numeric clientID would be coerced by URLSearchParams and come back
as an OAuth rejection — pointing diagnosis at the authorization server for
what is actually a provisioning error.
Tests cover both null bodies (verified red against the previous code), arrays,
and wrongly-typed secret fields, plus the two latency bounds the callback
depends on: a five-second abort signal per OAuth attempt and a retry backoff
capped at one second under maximum jitter.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* document IDRS config as owned outside this repo
A reader comparing IDRS_OAUTH_TOKEN_URL against OAUTH2_ISSUER or
UM_CONFIG_SECRET would reasonably conclude the CloudFormation wiring was
simply forgotten, since cloudformation/ lives in this repo. It wasn't — the
cloud engineering team owns that stack and provisions both the token endpoint
and the per-partner secrets out of band.
Say so where the rollout steps are described, so the next person doesn't
"fix" it by adding a stack parameter.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* cut the IDRS error taxonomy, retry, and coalescing
The error handling had outgrown what anyone acts on. Two cause unions, two AWS
error-name allowlists and a translation layer between services existed to pick
between 500, 502 and 503 — but the executor only distinguishes 200 from
non-200, so none of it changed a decision. Collapse to one response and one
log line. The AWS error name we were bucketing is more informative raw than
mapped, and the allowlists were guesses about SDK internals that would have
silently mis-bucketed anything unanticipated.
getIdrsConnectionInfo now follows getEduConnectionInfo right above it: null for
missing or malformed config, real AWS failures thrown. That convention has run
in prod for two years.
The OAuth retry is gone. One request, bounded at 5s. A transient blip now fails
the run, which is a fine thing to learn about from a real failure rather than
pre-solve — and it takes the worst-case dependency budget from ~16s to ~10s
inside the executor's 20s timeout, so the jitter arithmetic goes away too.
In-flight coalescing is gone. One callback per IDRS phase backed by a 24-hour
token means the concurrency it guarded against is rare and harmless when it
happens: two tokens instead of one.
Token-lifetime handling is one rule — cache it if expires_in leaves more than
the buffer, otherwise hand it back uncached with a warning — replacing separate
missing/short/zero/negative/non-numeric branches. An odd expires_in no longer
fails a run over a token that works.
Tests follow the requirements down: 67 → 39 unit, and the integration matrix
collapses to one parameterized case per distinct upstream failure. What's left
asserts observable behavior.
Net: app-config +201 → +94 lines against development, token service 260 → 145.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* guard the token endpoint and keep untrusted values out of the log
Two narrow holes from review, both about credential material rather than
correctness.
IDRS_OAUTH_TOKEN_URL was returned unvalidated and handed to fetch with the
client secret in the body, so a misconfigured http endpoint would transmit the
credential in plaintext. That is worse than a failed job, and it is the one
IDRS URL that had no scheme check — the per-partner base url already had one.
Refuse non-https outside local development, in the getter, alongside the
existing check.
The not-cacheable warning interpolated the raw expires_in, which is whatever
the OAuth server put in the response body. Log the lifetime we derived
instead: 0 already distinguishes unusable from short-but-valid, so nothing
diagnostic is lost.
The partner-isolation test started A and B concurrently against an empty
cache, so both read it before either populated it — returning any cached entry
to any partner would have passed. Sequential A/B/A/B over the reuse path
fails that mutation at partner B. Refresh is driven by controlled time rather
than reading the private cache, fetch is a restorable spy instead of a direct
assignment, the timeout tests assert the signal reaches the fetch/SDK boundary
rather than only that it was constructed, and the snapshot test now changes
the partner setting to something it wasn't already.
Also: drop the redundant database resets the shared beforeEach already does,
collapse one duplicate integration failure case that the unit layer covers,
and trim comments that retold history or narrated obvious checks.
AGENTS.md now makes an EDFIAL-481 executor a hard prerequisite for enabling
either fuzzy mode, not just an ordering preference. An older executor ignores
both new payload fields, and pure fuzzy omits the roster sources it reads
unconditionally.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test what the token service actually strips
The logging test claimed to prove a foreign upstream message stays out of the
log, but requestToken already replaces a transport error's message with one of
ours before it can reach any logger, and the test only captured this service's
logger anyway. The assertion could not fail. Review caught it.
Split it in two. The logging test keeps the assertions that can fail — the
token and the raw expires_in — and says in its name and a comment that its
scope is this service's logger, with the callback boundary covered in the
integration spec.
The stripping itself moves to the network-error case, asserted where it is
enforced: our message, the error name as upstream, and the transport message
absent from the error. Inverting that behavior now fails the test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* return the full IDRS search route, not the base url
The executor uses the url from the identity-service callback as-is, so the
app has to hand over the route it will actually call rather than the base
recorded in the partner secret.
The base is still stored and sent verbatim as the OAuth audience, which has
to match what IDRS was configured with — the route is only appended at the
callback boundary, where the run's partner and tenant are both in hand.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* deduplicate identity service failure tests
Keep shared failure coverage at the integration boundary and fold warning redaction into the existing lifetime cases. Preserve no-retry assertions without repeating the unit setup. No production behavior or nearby documentation changes are needed.
* remove tests pinning absent identity callback restrictions
Mode and run-status restrictions would reflect a deliberate scope change. Remove the two tests that only preserve their absence; production behavior and its documentation are unchanged.
* remove test pinning absent job mode updates
The creation tests already verify that the partner mode is snapshotted. Retroactively changing existing jobs would require an explicit product decision, so do not preserve its absence with another integration case.
* trim redundant IDRS test coverage
Remove tests that pin deliberate non-behavior, duplicate coverage at another boundary, or repeat equivalent invalid-input cases. Keep coverage for auth, roster modes, caching, timeouts, secret rotation, malformed responses, and credential redaction.
* use clientId in IDRS connection secrets
Define the per-partner secret with the same clientId spelling used by the application. This removes an unnecessary boundary mapping from clientID.
* assert the seeded matching mode instead of resetting it
The afterEach reset was dead: per-test-file-setup runs refreshSeed() in a
global beforeEach, which deletes and recreates the partner row from the
fixture, so the mutation never outlived the test. The sibling cross-year
test at the bottom of this describe already relies on that.
Asserting the seeded id_based value up front says the same thing the reset
was trying to say, and says it where it matters — it is what makes the
later fuzzy assertion evidence that creation copied the setting.
With the reset gone the wrapping describe held a single test, so it folds
into the parent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* name the payload gate after the mode, not the roster
usesIdBasedMatching describes what is being tested — the job runs the
authoritative ID-based pass — rather than one of the two things that
follow from it, and reads correctly for id_based_fuzzy_background, which
does run that pass.
Trims the comments alongside. The dropped detail about the callback
having no mode check lives in the controller and in AGENTS.md, where it
belongs; repeating it here was the third copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* drop the no-token assertions from the payload test
The spy asserted that building a payload doesn't call getCredentials,
which nothing in the payload path could do — the callback URL is a
string. The res.body.token check was the same idea from the other side,
and the response DTO has no token field to expose.
What the test is for is that the two fuzzy modes advertise the callback,
which the remaining assertions cover.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fold upstream into the error message
The field existed so the controller could append a sanitized fragment to
its log line, which meant the controller had to know the error type to
read a value it only concatenated into a string. Nothing consumed it
programmatically.
Sanitizing at construction is what actually made it safe, and that has
not moved: the transport error contributes its name, the rejected request
its status, and neither carries foreign text. Saying so in the message
puts it where every other reason already lives, and the controller goes
back to logging any error the same way.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* throw plain errors from the token service
Once the controller stopped reading the type, IdentityServiceTokenError
was a marker with no production consumer — only the tests distinguished
it, and asserting the message says more than asserting the class.
The failure tests now name the reason they expect, so a throw that moves
to a different branch fails them instead of passing on a shared type.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* name the service after what it returns
The field said tokens and the method handed back credentials — a token
plus the partner's base url, which the return type already called
IdentityServiceCredentials. The class was named after one of the two
things it produces, and the field pluralized that into a collection.
Renamed to IdrsCredentialsService, matching the IDRS_* wording the config
layer already uses, and injected as `idrs` so the call site reads as what
it does: get credentials for this partner.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* say what the identity-service tests mean
Three readability bumps, no behavior change.
The expected url repeated the partner id because the stubbed IDRS base
embeds it; naming that base separately makes the two occurrences distinct
things, and searchUrl now takes the tenant fixture rather than two fields
pulled off it.
`arrange` came from the AAA vocabulary and said nothing about what the
case does — it induces the failure the row is named for.
The cross-run token test was really a cross-partner one: tenantX sits
under a different partner, which is the leak the run-scoped token exists
to prevent. Title says so, and it now asserts we never reached for the
other partner's connection info.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* give each test partner its own IDRS host
Putting the partner id in the host rather than a path segment leaves the
one in the search route reading as the separate thing it is, so the
expected url no longer looks like it says the same thing twice.
Name the parameterized row's setup for the mock it installs. induceFailure
described the intent and left the reader to work out the mechanics, which
is the wrong way round in an it.each — the rows differ only by what they
do to getIdrsConnectionInfo.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* inline the one-use IDRS lookup timeout
The constant was referenced once, and its test asserted it equalled the
literal it was named for. The bound is clearer stated where it applies,
with the reason beside it.
Also notes the local-development fallback in the getter's doc comment, and
has the missing-local-values test assert we never reached for a secret.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* state the secret's validity positively
Four negated predicates joined by or is a hard way to say "all three are
non-empty strings and the url is https". Asserting that directly and
returning from inside the guard leaves the malformed case falling through
to the warning, which is where it belongs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* drop the abort-signal wiring tests
Both asserted that AbortSignal.timeout was called with 5000 and that the
resulting signal was passed along. Neither could show an abort cancelling
anything, because the thing that would be cancelled is mocked — whether a
signal actually stops an in-flight request belongs to the SDK and to fetch.
That leaves OAUTH_TIMEOUT_MS used once, so it is inlined beside the call
like the secret-lookup bound.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* say why the token request is form-encoded
The shape reads as an odd choice next to every other request in the app,
which sends JSON. It is what RFC 6749 requires of a token endpoint, so
cite it rather than leave the next reader to wonder.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* name the refresh condition instead of a window
"Enters the reuse window" reads as the opposite of what happens: a reuse
window would be when the token is reused, not when it is replaced. The
condition is a comparison, so state it — the token is served while more
life remains than the buffer.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test what the parse-failure catch is for
The case read as though it checked that a non-JSON body throws, which is
fetch's behavior and not something a mocked fetch can show. What is ours
is replacing that error: a real SyntaxError quotes the text it choked on,
so without the catch a piece of the token endpoint's response body ends up
in our logs. The mock's message now looks like one and the test asserts it
does not survive.
The sibling network-error case made the same claim through
JSON.stringify(err), which is "{}" for an Error — message and stack are not
enumerable, so it would have passed while carrying the sentinel. String(err)
actually reads the message.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* read the IDRS base url from config, not the partner secret
There is one IDRS service per deployment and each partner is addressed
within it by request path — that is why the search route carries
/partners/{partnerId} at all. So the base url does not vary per partner,
and keeping it in {ENVLABEL}-idrs-connection-info-{partnerId} meant
provisioning N copies of one value, each an opportunity to typo a
partner into a failure that only shows up when they first run a fuzzy
job.
The split was also backwards against the rollout we expect: the token
endpoint, the one url we already anticipate varying (a future TX-specific
endpoint), was deployment-level, while the base url, with no identified
reason to vary, was per-partner.
IDRS_URL now applies in every environment rather than standing in for
the secret only in local development, behind an idrsUrl() getter that
mirrors idrsOauthTokenUrl(): lazy, null when unset, https required
outside dev. It is still the OAuth audience and is still returned
verbatim. The secret holds { clientId, clientSecret } and nothing else,
so cloud engineering provisions two fields per partner.
The token cache drops its url field as a consequence. It only earned a
place there when the url arrived alongside rotating per-partner
credentials; a single env-var read cannot go stale against itself.
The per-partner IDRS host in the integration test was what proved the
callback minted a token for the right partner — wrong credentials would
have surfaced in the returned url. One shared base removes that
evidence, and the path segments come from the run lookup rather than the
credential path, so they do not substitute for it. The success case now
asserts the OAuth request carries that partner's client_id, scope and
the configured audience.
Nothing is provisioned yet, so this costs no out-of-band rework.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* cover the unparseable secret body and the malformed warn
Two gaps Rachel found in the config spec. A secret body that isn't JSON
takes a path the existing matrix misses: fetchAWSSecret treats it as a
plain-text secret and returns the raw string, so there are no fields to
read. And the malformed branch is the only one that logs, which makes it
the one place a future edit would be tempted to quote the body to explain
what was wrong -- so pin that only the secret name appears.
Capturing logger.warn instead of discarding it is what lets the second
test see the line at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* say which log the unprovisioned partner actually produces
The ResourceNotFoundException catch returns before the "missing or
malformed" warn, so that is never the line an id_based executor sees when
it calls the unadvertised callback. It gets the error the credentials
service throws instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* add tables for fuzzy-matching review records The Executor will report students it could not auto-match so they can be reviewed later. That needs input observables, the outcome of each matching attempt, and the ordered evidence behind it, all preserved across runs rather than overwritten. Input identity is natural — the job plus the Executor's opaque correlation id — so retries land on the same row without a lookup. A matching result is per (input group, run), and exists even with zero suggestions, so "searched and found nothing" stays distinguishable from "never searched". Suggestions key on (result_id, ordinal): an immutable position within one result, not a student identity, which future user decisions can reference. run gains UNIQUE (id, job_id) so the composite (run, job) foreign keys can enforce that a referenced run really belongs to the referenced job — ownership is derived from the authenticated run, never from body fields. student_match_result.id is BIGINT rather than INTEGER: if this later holds successfully-loaded records too, not just unmatched ones, int4 is only a few years of headroom. Prisma maps it to a JS bigint, which JSON.stringify rejects; nothing serializes it today, and a read API should expose a uid column like job.uid rather than the primary key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * red: specify unmatched-student-record ingestion and validation Nineteen tests covering the shape of the callback before it exists: a batch that mixes a multi-suggestion group with a zero-suggestion one, an empty batch, unknown-key projection, the invalid-container matrix, the correlation id length boundary, and the two authentication paths. All nineteen currently fail with 404 because the route is unimplemented. Two assertions are easy to lose later and are deliberate. A birth_date of '1815-13-45' must round-trip verbatim: malformed input may be exactly why a record needs review, so the callback preserves details rather than validating them. And a sentinel student name must not appear in any error body, which is the data-protection rule expressed as a test rather than a comment. The length boundary uses a multibyte character so that 128 means code points, matching PostgreSQL's length(), rather than UTF-16 units or bytes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * green: ingest unmatched student records atomically Implements the callback the previous commit specified: a projection pipe, a service that logs safely, and a repository holding the whole write in one transaction. The pipe validates structure and nothing else. Correlation ids, canonical student ids and scores are checked because the schema and the retry comparison depend on them; names and dates are copied verbatim, since malformed details may be the reason a record needs review. Candidate fields keep missing distinct from null, while roster fields normalize missing to null, because IDRS gives roster absence no separate meaning. Nest's global ValidationPipe does not validate an array of DTOs, so the top-level array is checked explicitly rather than assumed. The repository locks the run's job row first, so concurrent batches for one job serialize while other jobs proceed. It then inserts input details, compares them against what is stored, inserts one result per group for this run, and gives children only to results this request actually created. ON CONFLICT DO NOTHING is how rows avoid being written twice; it is not permission to accept differing content, which the two comparisons reject by throwing a private sentinel so the whole request rolls back. Returning an error value from inside the callback would instead commit the preceding inserts. Both comparisons run in SQL. Input is compared as lowercased JSON text read back as jsonb, matching how the Executor derives correlation ids, which makes a case-variant retry a no-op while the first accepted spelling survives. Suggestions are compared as an ordered aggregate coalesced to an empty array, so an empty-to-nonempty retry is caught rather than ignored. Scores travel to numeric as text, avoiding a float or Decimal round trip. Logs and error bodies carry run ids, counts and outcomes only. Run state is never touched: acting on a failed callback is the Executor's job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * cover retries, run history, concurrency, and ownership constraints Thirteen more tests for the behavior the transaction exists to provide: a rebatched, key-reordered, case-variant retry is a no-op that preserves ids, timestamps and the first accepted spelling; six kinds of disagreement roll the whole batch back with a 409, including a new group that travelled with the conflicting one; a later run on the same input is new history rather than a conflict; concurrent identical requests converge on one dataset while concurrent conflicting ones produce exactly one winner; and another tenant's identical correlation and canonical ids stay a separate graph. Two tests insert directly against the composite foreign keys, bypassing the endpoint, because those constraints are the last defence if ownership is ever derived from something other than the authenticated run. These were green when written, unlike the previous pair: the plan called for the transactional algorithm from the outset rather than an interim overwrite-based implementation, so there was no honest red state to commit. To check they are not passing vacuously, two mutations were run against them — disabling the suggestion comparison fails six, and replacing the lowercased input comparison with raw jsonb equality fails the case-variant retry as well. Both were reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * red: expect the unmatched-student-records url in the job payload The executor learns where to post review records from the job payload, the same way it learns every other callback. Advertisement follows the job's snapshotted matching mode: present for both fuzzy modes, absent for id_based, which has no fuzzy step to report from. Extends the existing mode tests rather than adding parallel ones, so the absent and present cases stay in the same place as the identity-service assertions they sit beside. Both fuzzy cases currently fail on undefined. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * green: advertise the unmatched-student-records callback Adds the URL builder beside the existing ones and emits it from the branch that already decides whether the executor needs IDRS. The two travel together for the same reason: a mode that uses IDRS is a mode that can leave students unresolved, so an executor that gets the identity service also needs somewhere to report what it could not match. All 46 earthbeam-api tests pass, so the other callback URLs and the serializer are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * document the unmatched-student-records callback Describes the request shape, the four response statuses, what retries are treated as no-ops, the three tables behind them, and the failure contract: the app never changes run state, so acting on a non-success response is the Executor's job — failing the run in fuzzy, stopping only background processing in id_based_fuzzy_background. The mode table now covers both fuzzy-only callbacks in one column, since they are advertised together and for the same reason. Records the outstanding request-size limit in the docs rather than only in the plan, so the next person to touch this endpoint sees that it runs under the default parser and is not deployment-ready. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * simplify ingestion now that executor batching is pinned down Two facts about the Executor narrow what this transaction has to defend against: a run's batches arrive sequentially, and input details are extracted only on a job's first run. Together they mean no two requests ever insert the same input or result row concurrently — a later run adds only its own result, against inputs that already exist. So the FOR UPDATE lock on the job row is gone. It was guarding concurrent same-job ingestion that does not happen, and the argument that it prevented a spurious 409 was simply wrong: the suggestion comparison filters on run id, so another run's uncommitted rows were never visible to it. The SELECT stays, since it is how the owning job is derived and how a missing run becomes a 404. Removing it leaves both concurrency tests passing, repeated three times. The suggestion comparison now skips groups whose result this request just created, which were only ever compared against what we wrote one statement earlier. Every reprocessing run inserts fresh results, so this skips the check entirely for them; what remains is the case the check exists for, a sequential retry re-sending a group already accepted within its run. Atomicity is untouched and remains the real reason for the transaction: the three bulk inserts are individually atomic but a failure between them would leave a result with no suggestions, which is indistinguishable from a genuine no-match and permanent, since the retry inserts nothing. The concurrency tests now say in comments that they are defence rather than modelled behavior, so they are not read later as documenting the Executor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * correct the callback's concurrency documentation The previous text said requests commit under a lock on the job row so concurrent batches serialize. That lock is gone, and the reasoning behind it did not hold: a run's batches arrive sequentially and input details are extracted only on a job's first run, so no two requests contend. Says instead what the transaction is actually for — a failure between the three bulk inserts would leave a result with no suggestions, which reads as a genuine no-match and cannot be repaired by a retry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * drop the concurrency tests They exercised two requests racing within one run, which the Executor does not do: it sends a run's batches sequentially. Comments saying so were not enough — a test is read as a statement about how the system is used, and these implied a concurrency story that does not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * drop conflict detection in favor of first-report-wins The callback compared every retry against stored content and raised a 409 on any difference. Both comparisons are gone, with the conflict sentinel, the rollback-by-throw and the 409 response; the repository drops from 216 lines to 121. The input comparison was redundant with the correlation id by construction: the id is an MD5 of the lowercased input details, so the same id cannot carry different input without a hash collision. It was also the only reason the lowercased-jsonb comparison existed, which was the hardest thing in the file to explain. The suggestion comparison covered the half the id says nothing about, but matches is passthrough IDRS output — there is no bug in which the executor corrupts it, only a choice to re-query IDRS on retry instead of re-sending its buffer. That same scenario is where the check misfires: if IDRS breaks scoring ties differently, identical content arrives reordered, and an order-sensitive check would 409 and fail the run in fuzzy mode over nothing. The risk it created was more concrete than the corruption it caught. What remains is a rule rather than a gap: within a run the first report of a group wins, and new evidence arrives as a new run with its own result row, which is how the schema already records history. A test asserts it, and AGENTS.md records the debugging consequence — a re-sent group carrying different matches is silently discarded. The transaction stays, for atomicity alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * address round 1 review Test fixes, all of which were real gaps: The 'top-level string' validation case never tested a string. Superagent defaults a string body to x-www-form-urlencoded, so Express parsed it into an object and the pipe's array check was reached by the wrong path. Sent explicitly as JSON now. Atomicity had no test, despite being the transaction's only remaining justification. Nothing in a request can trigger a mid-transaction failure — the pipe and the schema constraints agree, which is the point — so the failure is injected at the database client after the input and result rows are written. Mutation-checked: swallowing the error inside the transaction fails it, which is the regression the test exists for. The 404 branch was reachable but untested; a signed token can name a run that no longer exists. snapshot() compared row arrays elementwise without ordering them, so two assertions passed on incidental order. IngestResult's error arm allowed 'SUCCESS' and the controller mapped every error to 404 regardless of code. Both narrowed. AGENTS.md gains the 401/403/500 rows the spec already asserts. The kickoff Progress section described a transaction that no longer exists: it claimed the suggestion comparison was narrowed when it was deleted, and that the concurrency tests were kept when they were removed. Replaced with a single accurate record naming the two unmet acceptance criteria, and the Executor facts the design rests on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * guard single-run extraction; fix round 2 review findings Replaces the removed content comparison with a structural check Andy proposed: a job's input details are established by exactly one run. The Executor extracts them on a job's first run, so a later run creating an input row means a fresh extraction, which belongs to a new job, or a bug that would attach one run's matches to another run's observables. One count(DISTINCT source_run_id) inside the transaction, 409 and full rollback if it exceeds one. This is the better guard for the reason the comparison was dropped. The brief's "no executor-normalization dependency" means the app must not care how the executor normalized, and must stay correct when that changes. The lowercase comparison failed that test — its mechanism was lowercasing to approximate the executor's own lowercasing — while a provenance check depends on no normalization at all. It also costs one statement rather than eighteen lines. The atomicity test was passing for the wrong reason. Every assertion was a zero count, which a failure at the first statement satisfies just as well, so the positional injection was unverified; re-aiming it from the second statement to the first left the suite green. The proxy now reads through the same transaction before throwing, and the test asserts both rows existed at that moment. It also now asserts the 500 path logs the run id and neither the candidate name nor the correlation id. The round-1 controller fix had introduced a worse failure than it removed: a guard clause with no else, so a second ingestion outcome would compile and fall through to an empty 200 — the worst answer for a callback whose non-success response is what fails the run. Now an exhaustive switch with a never default. AGENTS.md no longer states the executor's hashing as a property the app relies on, and documents the new 409. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * revert the single-extraction-run guard The guard counted distinct source_run_id values per job and returned 409 above one. It was wrong in both directions. It rejected a legitimate case. A run that fails partway leaves groups undelivered, and restarting the job creates a new run on that same job — startJob refuses only while a run is running or new. That run has to insert the undelivered groups, which tripped the guard and returned 409 for the job permanently, discarding the legitimate results batched alongside them. The 413 path makes this reachable now rather than hypothetically, since the endpoint still runs under the default parser while step 7 is unfinished. More decisively, it missed the case it was written for. A re-sent correlation id whose details differ hits an existing row, so ON CONFLICT DO NOTHING skips the insert and the distinct count stays at one. The guard could never see the anomaly it was added to catch, so the previous commit message had it backwards: a colliding id is precisely what it cannot detect. Input rows keep per-row provenance, as migration 034 already describes. A test now covers the recovery path the guard was breaking. Also restores a Logger spy that leaked into every later test in the file, and documents the 413 the default parser can already return. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * stop run deletion cascading through input details student_input_details referenced its establishing run with ON DELETE CASCADE. Input details are established once per job, so deleting that one run removed the job's input rows and, through them, every other run's results and suggestions for the job — confirmed against a dev database before this change. source_run_id is provenance, not ownership; the job FK already owns the row. It is now ON DELETE NO ACTION. Deleting the establishing run is refused, deleting any other run still removes only that run's results, and deleting the job still cascades through everything. NO ACTION rather than RESTRICT is what keeps that last case working: its check runs at the end of the statement, by which point the job's cascade has removed both the run rows and the input rows. Amended in place, since 034 has only been applied to local dev. The Prisma schema diff is the one relation's onDelete. A test pins both halves of the behavior. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fold the ingestion repository into its service The repository was the only one in the codebase. Every other service injects Prisma directly, including those that run transactions and raw SQL, and this one had become a thin logging wrapper around the repository's single method. Neither follow-on feature — recording decisions, kicking off reprocessing — would call it, so the extra layer had no demonstrated value. The transaction moves unchanged into a private persist method, and the error arm of IngestResult is now the literal 'NOT_FOUND', so the controller's exhaustive switch still catches a new code at compile time. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * describe the api layering as it actually is AGENTS.md said controller → service → repository, but the codebase has no repository layer: services inject Prisma directly, including for transactions and raw SQL. The line led the plan for the unmatched student records callback to add the only repository in the repo, which was then folded back into its service. Saying what the code does keeps the next change from repeating that. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * type the stored review json and stop renaming it The pipe emitted a camelCase envelope — correlationId, inputDetails, rosterDetails, studentUniqueId — which the service's toSqlRecord then converted straight back to snake_case for the SQL. The JSON content itself was never renamed, so the round trip bought nothing. The normalized batch now keeps the wire and column names and is handed to SQL as-is. Keeping snake_case also means stored details can be sent back out unchanged, for example to re-query IDRS; casing for the frontend belongs on a read DTO. Scores now reach numeric through JSON.stringify of the number rather than String(). Both produce the same shortest round-trip text, so stored values do not change. The two JSONB columns are typed with prisma-json-types-generator, using types in models so the frontend can share them. Every field is JsonValue: details are preserved rather than validated, since malformed input may be why a record needs review. That needs a /// annotation in schema.prisma, which AGENTS.md said never to edit. Re-introspection preserves annotations on existing fields — checked on a scratch copy and then through the real pull-and-generate — and #42 set the precedent with RunOutputFileSetFiles, so AGENTS.md now names this as the one exception. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * pin callback projection behavior before the dto refactor Adds the one projection rule that had no test — roster student ids that are not objects are kept verbatim, and objects keep only id_type and id_value — plus a primitive record and a null record in the validation matrix. These pass against the current procedural pipe, so the declarative version that replaces it has to preserve them. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * validate the callback body with declarative dtos The pipe validated and projected the body procedurally. The rules now live in class-validator and class-transformer DTOs in models, as request DTOs do elsewhere, and the pipe keeps only what a DTO cannot express: the top-level array check, duplicate correlation ids across items, and the safe rejection log. ParseArrayPipe looked like the obvious fit for the array but is built for query strings: it splits a string body on commas and JSON-parses each piece, so a string holding a single record would be accepted. Projection is declarative. The pipe's own ValidationPipe transforms with excludeExtraneousValues, so unknown keys are dropped rather than stored, and exposeDefaultValues, so null defaults on the roster fields stand in for missing ones. Roster student_ids use @type without @ValidateNested, which keeps non-object entries verbatim instead of rejecting them. Correlation ids use a u-flag @matches rather than @Length: validator.js counts a character plus a variation selector as one, PostgreSQL counts two, so @Length would pass ids the database CHECK then rejects with a 500. A new test pins it. The SQL now reads the validated wire shape directly — candidate as the input details, and for each match an ordinal from WITH ORDINALITY and roster details as the match minus its two column fields — so no JS reshaping remains. Behavior is unchanged; the previous commit pinned the one rule that had no test. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * let the database validate the callback and store it as sent The Executor is trusted and only distinguishes success from failure, so validating the body in the app mostly repeated the database. Every field that lands in a column is already guarded by that column: correlation ids by NOT NULL and the length CHECK, candidates by NOT NULL and the object CHECK, each match's student id and score by theirs, and jsonb_to_recordset rejects anything that isn't an array of objects. The pipe and the request DTOs are gone, and the controller hands the raw body to the service. The one field that lands in no column is the matches array. A missing or null matches expanded to zero rows and would have been stored as "IDRS found nothing"; coalescing it to JSON null makes jsonb_array_elements fail instead, as a non-array already did. Duplicate correlation ids need no check: two non-empty match lists both claim ordinal 0 under the one result and violate its primary key, so they can never mix. Payloads are stored exactly as sent. Input details are the candidate; roster details are the match minus its two column fields. Keys the app does not read yet are kept, so fields IDRS adds later are already stored when the app wants them, and nothing is normalized. Malformed payloads are now a 500 rather than a 400. Failure logs gain PostgreSQL's SQLSTATE as the diagnosis but never its message, since a constraint violation's detail quotes the failing row, and the rejection tests check that no student value reaches the response or the log. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * describe the callback payload with dtos again The previous commit removed the request DTOs in favor of database constraints. DTOs are how the rest of the app describes an expected payload, and they are legible in a way constraints are not, so they are back: UnmatchedStudentRecordDto and UnmatchedStudentMatchDto describe the shape and validate it as far as class-validator can. Payloads are still stored as sent. The DTO validates without projecting — no excludeExtraneousValues, no defaults — so unknown keys survive and missing fields stay missing. ParseArrayPipe now handles the top-level array. It had been ruled out because it splits string input on commas, but a body can never be a string here: the JSON body parser's strict mode accepts only objects and arrays. The pipe subclasses it only to log rejections safely. Each rule now has a home. Correlation id, candidate, student id and score are validated by the DTO and also guarded by their columns. That matches is an array lives only in the DTO, because the array lands in no column; the SQL COALESCE that stood in for it is gone. Duplicate correlation ids live only in the database, because a per-record DTO cannot see across records, and none is needed: two non-empty match lists violate the suggestion primary key and roll back. Correlation ids use @Length(1, 128) for legibility. It differs from PostgreSQL's count only for characters with variation selectors, where the database CHECK still rejects the id, as a 500 rather than a 400. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * name the callback dtos after the app's concepts UnmatchedStudentRecordDto and UnmatchedStudentMatchDto named things the way the Executor sees them. The app stores each item as a match result holding suggestions, in student_match_result and student_match_suggestion, so the DTOs are now EarthbeamApiStudentMatchResultDto and EarthbeamApiStudentMatchSuggestionDto. "Unmatched" in particular was the Executor's framing, and would be wrong if these tables later hold auto-matched records too. EarthbeamApi follows the file's prefix for Executor-facing DTOs. Payload is dropped: each DTO describes one item of the body, not the body itself. Property names stay as the Executor sends them, candidate and matches, because the payload is stored as sent; the doc comments now say they hold the input details and the suggestions. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * generalize the ingestion names to match results Nothing in the storage or the ingestion is specific to unmatched students: a result is one search for one group of input details, with its suggestions, and an auto-matched student would take the same path. The service, pipe, controller method, files and log prefix are renamed to match results accordingly. The route and appUrls.unmatchedStudentRecords keep their names for now. They are the contract with the Executor, which implements its side separately, so renaming them is a coordinated change rather than a local one. AGENTS.md now says the storage is general and records the one change reporting auto-matched students would need: a way to record the automatic resolution, without which those students would look the same as ones awaiting review. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * rename the match results route and payload key POST .../unmatched-student-records becomes .../student-match-results, and appUrls.unmatchedStudentRecords becomes appUrls.studentMatchResults, to match the internal names: the callback carries match results, and only the Executor's current use of it is limited to students it could not auto-match. This is the contract with the Executor, but its side has not been written yet, so the rename costs nothing now and would need a coordinated deploy later. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * map ingestion errors with an if rather than a switch A switch invites fall-through bugs and nothing here needs one. The if keeps the same guarantee: after the NOT_FOUND branch, the remaining code is narrowed to never, so adding an IngestResult error code without handling it is still a compile error rather than a fall-through to an empty 200. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * return 201 from the match results callback, like its siblings The kickoff specified 200 for every success, on the grounds that a no-op retry creates nothing. Every other Executor callback returns 201, though, including summary, which is an update rather than a create, so here 201 means processed and stored rather than literally created. The consistency matters because the Executor already checks one callback for exactly 201 (update_failure retries on status_code != 201). An implementation of this callback copying that pattern would have read 200 as a failure and failed every fuzzy run. The Executor side is not written yet, so this is the moment to settle it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * use ParseArrayPipe directly for the match results body The subclass existed only to log rejected payloads. The route now uses the built-in pipe inline, and diagnosing a rejection is left to the Executor: the 400 body names what failed without quoting values, so it is safe for the Executor to log. That body needed one fix to be useful. By default ParseArrayPipe rethrows the first failing item's error unchanged, so the body said a field was wrong but not which record. stopAtFirstError: false makes it index every failing record, and matches how main.ts configures the global ValidationPipe. A test pins the indexed message. The note that the body parser must stay strict, since ParseArrayPipe would otherwise split a string body on commas, moves to the controller. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * drop the strict body parsing caveat from the match results route The comment said the JSON body parser must stay strict because ParseArrayPipe splits string input on commas. It doesn't need to: a valid record has at least three keys separated by commas, so splitting a string always breaks it into fragments that fail validation, strict or not. Checked by calling the pipe directly with string bodies; every one was rejected. What remains is the note specific to this route: rejections are not logged, and the indexed 400 body is the diagnosis. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * tighten the match results spec to tests that earn their place A review for load-bearing, non-duplicative, forward-looking tests took it from 34 tests to 21. Several assertions could never fail. Nothing caught the service logging an error's message: 400s are not logged at all, and the errors the 500 tests raised never quoted student data. The atomicity test now injects an error that does, and fails if the log repeats it. Its suggestion count was always zero, since the injection replaces that insert. The schema tests accepted any error, including a typo in their own SQL; they now assert the foreign key violation codes. The case-variant retry test was written for the input comparison that no longer exists; it becomes one first-report-wins test that also covers an empty result gaining a match. The rejection matrix keeps one case per DTO rule, the two later-run tests merge, and tests of Express's strict parser and of the guard's 401, already covered elsewhere, are gone. Mutation-checked: logging err.message, dropping @isarray, dropping stopAtFirstError and dropping the transaction each fail the intended test. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * prune match results docs and comments to the current design The AGENTS.md section and the comments had accumulated the history of how the design got here: a guard that was tried and reverted, checks judged not worth adding, the reasoning behind class names. They now describe what the code does and why, and say each thing once. The AGENTS.md section drops from 1,186 words to 744, and the controller defers to it rather than restating the service's behavior. One statement was wrong, not just long. The docs and the service said no row lock was needed because input details come only from a job's first run, which the recovery path, a restarted job's run delivering groups the failed run never reported, contradicts. The real reason is that a run's batches arrive in sequence and the unique constraints keep overlapping runs consistent. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * drop the run id indexes from the match results tables The indexes on student_input_details.source_run_id and student_match_result.run_id served only lookups by run without its job. The known access path, a job page showing a job's data across runs, uses the job-first primary and unique keys. The foreign keys to run are composite, (run_id, job_id), so their lookups on run deletion use those keys too. Checked with EXPLAIN on a 900,000-row copy of student_match_result. With the run_id index gone, the foreign key lookup becomes an index-only scan on (job_id, correlation_id, run_id). Only a lookup by run alone gets worse, falling back to a sequential scan, and the app always knows a run's job. Each dropped index was an extra write on every inserted row. Migration 034 has only been applied locally, so it is amended in place; the Prisma schema loses the two matching @@index lines. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * describe retries and later runs as they actually happen AGENTS.md and the service described two things that don't happen. The Executor never re-queries IDRS for a group it has already reported, so a retry re-sends identical content; the paragraph speculating about silently dropped re-queried matches, and the reasoning for not comparing them, go. Nor is there a restarted job whose new run fills in a failed run's gaps: the first run extracts input details and calls IDRS, and later runs happen only after it succeeds. The no-lock reason is restated to match: only a job's first run reports match results, and its batches arrive in sequence. Results stay keyed by run so that a later search, such as rematching, adds history rather than overwriting. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test retries and later runs as the executor produces them The retry test re-sent groups with different details and suggestions, but the Executor never re-queries IDRS for a group it has reported; a retry re-sends identical content, possibly rebatched. The test now does exactly that. It still guards the step that matters: if a retry's existing results were given suggestions again, the primary key would reject them and the retry would fail. The later-run test also delivered a group the first run had never reported, modelling a restarted job filling in a failed run's gaps. Later runs happen only after the first succeeds, so that half is gone; what remains shows a later run's results added as history alongside the first run's. Mutation-checked: returning existing results from the retry's insert fails the retry test, and filing results under the job's first run fails the later-run test. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * look up the run's job with prisma instead of raw sql The lookup was raw SQL only to take a FOR UPDATE lock on the job, which is gone. What remains just reads the authenticated run's job_id, so it no longer joins job, whose row the run's foreign key already guarantees, and uses Prisma. The variable is named for what it holds, a run, rather than "owner". The no-row-lock note goes with the query: with nothing nearby that locks, it explained an absence, and AGENTS.md covers it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * drop the redundant integer casts from the ingestion inserts Prisma sends a JS number as a typed parameter, and PostgreSQL's assignment cast converts it to the target integer column on insert, so ::int on the job and run ids changed nothing. The same holds for the bigint ordinality going into the integer ordinal column. The casts that remain are needed: the payload arrives as text and must become jsonb, a score extracted with ->> is text and has no implicit cast to numeric, and jsonb_to_recordset needs its column types. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * center the ingestion inserts on the normal flow The names and comments were framed around a retry re-sending groups that are already stored: results were "inserted" as opposed to existing, their ids "new", and each step's comment led with what a retry does. They now describe what each step stores, with the retry's behavior kept to one clause at step 3, where step 4's correctness depends on it. The guard skipping the suggestions insert when no results came back only saved a round trip on retries. Without it, the insert finds no results to join and adds nothing, so it goes. The note on duplicate correlation ids also goes; AGENTS.md and its test cover it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * correct the duplicate id and locking docs; widen the retry test AGENTS.md said duplicate correlation ids in one request can never mix, presenting the database as the rule that catches them. It isn't: the suggestion primary key only happens to refuse duplicates that both carry matches. The docs now say what actually holds. Duplicates go unchecked because the Executor sends one entry per correlation id, and if it did send them, a shared id means the same input and so the same matches. Identical duplicates without matches collapse harmlessly, and those with matches fail the request with nothing written. Only a correlation id attached to the wrong input or matches at the source could store wrong data. The duplicates test is renamed so it no longer claims more than it checks. The stated reason for taking no row lock was the Executor's batching. The real one is that every insert is ON CONFLICT DO NOTHING against a unique key, which keeps overlapping requests consistent even when a timed-out retry races its original. The retry test only ever re-sent groups already stored, so it could not catch the skip-if-nothing-new shortcut combined with attaching suggestions to the run's existing results. It now re-sends a stored group alongside one not yet delivered, which fails under that regression. AGENTS.md also notes that a NUL character anywhere in a payload always fails the request, since jsonb cannot store one. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * put the match results auth test first Auth tests come first in each suite, so the check that a token for another run is refused now opens the spec in its own authentication block. The 404 and atomicity tests, which sat outside any describe and so were reported ahead of every block, are grouped under failures, leaving the report in file order. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test that the match results route refuses a request without a token The guard's own missing-token handling is covered in earthbeam-api.spec.ts, but checking it on this route too costs little and confirms directly that an unauthenticated request can't write review data. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * make the main ingestion test readable at a glance It built its request from shared helpers, sent records in the opposite order to the one it asserted, and compared mapped tuples whose order came from the snapshot helper, so checking it meant reconstructing what the helpers did. It now sends a literal body, queries each table itself with the ordering in view, and compares whole rows, so each expected value can be read straight off the request. Database-generated ids and timestamps match with expect.any. Scores compare as Prisma Decimals, which toEqual checks by value; replacing an expected score or ordinal with a wrong one fails the test. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test atomicity with a real failure instead of an injected one The atomicity test wrapped the transaction client in a Proxy that threw on the second raw statement, because the DTO stops any request that could fail partway through. The integration suite can call the service directly instead, skipping the DTO: an empty student id passes the input and result inserts and then violates the suggestion table's CHECK. That is a real failure partway through and a real rollback, with no mock and no reliance on statement order. PostgreSQL's error quotes the failing row, student data included, and the test asserts it does. So its check that the log never repeats an error's message is now genuine; the Proxy test had to inject a message to make it so. Calling the service skips the controller, so a second test keeps the response covered through HTTP. @Length counts a character plus a variation selector as one where PostgreSQL counts two, so 65 hearts pass the DTO and fail the correlation id CHECK, whose error quotes the row; the response must not. Mutation-checked: returning err.message from the controller, logging it from the service, and dropping the transaction each fail the intended test. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test the controller's error response with a stubbed failure The check that an error's message never reaches the response used a correlation id of 65 hearts, the one payload that passes the DTO and makes the database return an error quoting student data. The quirk pulled attention from what is being tested, which is simpler: the controller answers any service failure with a fixed 500. The test now stubs the service to reject with an error quoting the sentinel. That covers any error, not one database quirk, and the service level test already shows real errors do quote student data. Returning err.message from the controller fails it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * name run B's result in the establishing run deletion test Run B exists to show what the refused delete protects: with a cascade from the input row, deleting run A would also have removed run B's result. The test only implied that through a count of two. It now checks both runs' results by run id. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
- axios 1.19.0 -> 1.20.0 - @nestjs/platform-express 11.2.3 -> 11.2.6 (pulls multer 2.2.0 -> 2.4.0) - proxy-addr 2.0.7 -> 2.0.8 - qs -> 6.16.0 everywhere (express 4.22.3, body-parser 1.20.8 for storybook) - moment 2.30.1 -> 2.31.0 toml (via snowflake-sdk) is deferred until snowflake-sdk 3.4.0 clears the package age window. Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
* Restrict ODS connection requests to public addresses The ODS connection test requests the user-supplied host and the auth endpoint named in its response. Outside of dev, these requests now go only to http(s) destinations that resolve to public addresses, checked at connect time and on each redirect. Address classification uses ipaddr.js (IANA special-purpose ranges). Requests also get a 10s overall deadline, and refused requests are logged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * Log only the origin of the ODS host The host field is user-entered and may contain userinfo, query parameters, or text pasted into the wrong field, so the refused-request warning now records just the URL's origin. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * Fix EdfiModule config injection and make guard tests independent of module load order EdfiService now uses the globally provided AppConfigService rather than importing AppConfigModule, which had changed which instance the app resolves. The guard tests spy on ipaddr.process instead of mocking the module, so they pass when the app is loaded first, as in the integration run. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * Require https for ODS connection requests outside dev The connection test sends the ODS client credentials, so outside of dev the host, the auth endpoint and any redirects must use https. With http no longer reachable there, the http agent is removed. Tests now run the local ODS over https using a generated self-signed certificate (selfsigned, dev only). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The integration Jest config had no path filter, so the unit specs under api/src ran in both the unit and integration suites. Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- urllib3 2.7.0 -> 2.8.0: improper certificate validation (high), unbounded chunk-size buffering (high), deflate infinite loop (medium) - aiohttp 3.14.1 -> 3.14.3: use after free (high), resource exhaustion and request smuggling (medium) urllib3 backs the executor's requests session to the app and boto3's S3 calls; aiohttp backs lightbeam's ODS fetch/send. 2.8.0's breaking change is limited to HTTPS proxy TLS config, and the executor uses no proxy. The aiohttp releases are bug fixes only. Both versions are past the CI safe-chain age gate. Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
* idrs client module * add IDRS_MATCHES artifact * add IDRSQueryError class * Add IDRS Client orchestration * Set IDRS config fallbacks * rename idrs orchestration method * fix import * move idrs orchestration back to finally * rm outdated idrs_url state * Add batch size config * add batching logic to post to the IDRS * cleanup * Raise if empty array returned * Collapse if statements * remove needs_upload * add type hinting * update comment
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
* adding new IDRS env vars * bump template version * template version
rtavernaea
approved these changes
Oct 6, 2026
zabdykadyrova
approved these changes
Oct 6, 2026
theokaufman
approved these changes
Oct 6, 2026
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
4 participants
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.
This PR is for the 10/6 deploy. It includes: