Repository navigation
Edfial 630 - #136
Open
rtavernaea wants to merge 19 commits into
Open
Edfial 630#136rtavernaea wants to merge 19 commits into
rtavernaea wants to merge 19 commits into
Conversation
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
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. |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
rtavernaea
marked this pull request as ready for review
September 25, 2026 13:41
* 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>
edandylytics
reviewed
Oct 1, 2026
edandylytics
left a comment
edandylytics
left a comment
Collaborator
There was a problem hiding this comment.
Looking good overall -- still need to spend time with the tests, but wanted to get some thoughts your way
Collaborator
There was a problem hiding this comment.
The first fuzzy matching migrations use 34 -- merged to dev since you started this PR. Will need to bump to 35
| ecsTaskArn: string | null; | ||
|
|
||
| @Expose() | ||
| taskSize: EcsTaskSize | null; |
Collaborator
There was a problem hiding this comment.
Do we need the arn and task size in the DTO? I'd rather not expose it unless there's a compelling reason.
Collaborator
Author
There was a problem hiding this comment.
Good point, I don't think we do
|
|
||
| ALTER TABLE public.run | ||
| ADD COLUMN ecs_task_arn text, | ||
| ADD COLUMN task_size ecs_task_size; |
Collaborator
There was a problem hiding this comment.
I'm not sure ecs_task_size as an enum buys us much here, and it would mean that we'd have to update it before adding new task sizes. Perhaps use text?
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>
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
2 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.
ticket
task_sizeandesc_task_arn