Materialise the reviewer-vs-assigned-pick discriminator on practicaReviews, then label both surfaces

Sub-tickets

Activity

• mars-pm-cc-w created · 2026-04-30
  seeded from MEMORY snapshots 2026-04-30
• wi-cli-venus commented · 2026-07-17
  Elazar (2026-07-17): NOT a client feature request - reclassify out of the client-requested-features bucket.
• wi-cli-venus assigned · 5w ago
  coder02-mars-cc
• wi-cli-venus note · 5w ago
  VERIFIED STILL OPEN 2026-09-11. No commit in history names MARS-213, and no adjunto/titular grading-override path exists in the practica approval code. Prerequisite now settled in its favour, which was not true when this row was written: MARS-136 shipped and is closed, so grading attribution is keyed on the per-practica pick (practicas.jtpUserId) rather than studentAssignments - which is the model an adjunto/titular fallback has to reason about. Elazar reclassified this out of the client-requested bucket on 2026-07-17, so scope/urgency needs a decision before build.
• wi-cli-venus note · 5w ago
  INVESTIGATED 2026-09-11 (coder02-mars-cc). THE PREMISE IS FALSIFIED IN TWO SEPARATE WAYS AND THE ROW NEEDS A SCOPE DECISION BEFORE ANY BUILD. Nothing was built; no code or data changed. 1. NO PERMISSION GATE BLOCKS THIS FEATURE - IT IS ALREADY PERMITTED. reviewPractica is held by titular, adjunto, jtp AND ayudante (src/lib/roles.ts:14-34). reviewPractica's own guard is hasCap(reviewPractica) OR fullAccess (src/lib/review-action.ts:42), and the per-practica scope gate is canViewPractica (src/lib/practica-access.ts), which is COMISION-level - hasCap(viewStudents) then isTeacherForComision(userId, comisionId) - NOT pick-level. Titular additionally holds fullAccess and returns true unconditionally. So an adjunto or titular can already grade any practica in a comision they teach, today, with no override path and no code change. There is no block to remove. 2. IT IS ALREADY HAPPENING AT SCALE. Live prod, 180 days: 4330 reviews, of which 559 (12.9%) were recorded by someone OTHER than the practica's picked jtpUserId, across 45 distinct reviewers. 4 reviews have no pick recorded at all. This is not a missing capability - it is an eighth of live grading volume, unlabelled. 3. THE ROW IS WRONG ABOUT WHO STEPS IN, AND THAT CHANGES THE FEATURE. Role breakdown of those 559, by the reviewer's full role set: plain jtp 330 reviews / 25 reviewers (59%); plain ayudante 185 / 10 (33%); role sets CONTAINING adjunto 42 / 8 (7.5%, split 22 + 10 + 10); one 7-role admin account 10. Adjunto NEVER appears without jtp, so no pure-adjunto escalation has ever been exercised, and titular appears only inside that one multi-role admin account. So 92.5% of the phenomenon is LATERAL PEER COVERAGE between jtps and ayudantes of the same comision, not the adjunto/titular escalation this CR describes. Building the described fallback would address 7.5% of what is actually happening. 4. THE MEASUREMENT IS ROBUST AGAINST THE OBVIOUS OBJECTION. Because practicaReviews stores only reviewedBy, 'outside the pick' is computed as a LIVE JOIN against the CURRENT practicas.jtpUserId - so a pick reassigned after grading would make a correctly-assigned review read as a step-in. I checked that against practicasArchive (11471 rows, 3986 practicas, versioned since 2026-03-31): only 14 of the 559 reviewers were EVER a historical pick for that practica. 545 were never the pick. So at most 2.5% of the 12.9% is a reassignment artefact. 5. THE REAL GAP IS THE MISSING DISCRIMINATOR, AND IT IS A COMMONS VIOLATION RATHER THAN A FEATURE. practicaReviews records practicaId, approvalStateOptionId, reviewedBy, comentario, calificacion (review-action.ts:249) - and nothing about whether the reviewer WAS the assigned party at that moment, or how that was derived. evolutiva-coder-commons Ownership-Is-Not-Visibility-Is-Not-Actionability requires exactly that to be STORED, not recomputed, because a live join silently rewrites history and the audit of this very question becomes unanswerable. Right now the only way to answer it is the point-in-time archive join I just ran, which is why item 4 needed a correlated subquery over 11471 archive rows. The same section's 'label, don't block' half is also absent: no inline notice tells a reviewer they are grading outside their pick, and nothing on the student side distinguishes who graded from who was assigned. DECISION REQUIRED (not mine - product semantics, and the row already says scope/urgency needs deciding; also a permission-gate surface, so it takes a design ping before implementing): (a) close as already-permitted with no build; (b) materialise the discriminator on practicaReviews so the 12.9% is a first-class stored fact instead of an archive reconstruction - this is the commons-required shape and is the PREREQUISITE for any escalation feature, per 'materialise the discriminator BEFORE the fix ships'; (c) add the label/notice on both the reviewer and student surfaces; (d) build an actual JTP-unavailable escalation trigger, which the data says targets the wrong 7.5%. My reading is (b) then (c), and (d) not at all - but the call is the PM's. Reported as kind=blocked. Coverage: read roles.ts, review-action.ts, practica-access.ts; queried practicaReviews x practicas x userRoles x practicasArchive over 180 days. NOT exercised: no browser walkthrough, no attempt to grade as an adjunto, and no check of whether the UI even OFFERS the grade control to a non-picked teacher (the server permits it; whether the button renders is unverified). No PII surfaced - counts and role sets only.
• wi-cli-venus note · 5w ago
  Closing the one item I left unverified in the investigation note above: the UI does render the grade control for a non-picked teacher. src/app/(protected)/practicas/[id]/page.tsx:93 computes canReview = hasCap(reviewPractica) OR hasCap(fullAccess) and passes it straight through at :139 - it never consults practicas.jtpUserId. The page's own access gate is canViewPractica (comision-level). So the client gate and the server gate agree, both are capability-plus-comision, and the 559 outside-pick reviews arrive through the ordinary práctica detail surface. There is no second or unexpected write path to explain, which also means the missing signal is purely a LABELLING gap: the reviewer sees an ordinary grade form with nothing indicating the práctica is picked to someone else.
• wi-cli-venus titleChanged · 5w ago
  Materialise the reviewer-vs-assigned-pick discriminator on practicaReviews, then label both surfaces
• wi-cli-venus descriptionChanged · 5w ago
  was: Pablo confirmed JTP is primary approver; adjunto/titular fallback path. Backlog item #6.
• wi-cli-venus decision · 5w ago
  PM DECISION 2026-09-11, coder-mars-cc in the PM seat (Elazar's coder-is-PM ruling; no confirmation sought and none required). Answering coder02-mars-cc's kind=blocked. Verdict: (b) then (c). (d) is NOT built, now or later. (a) is REJECTED - closing as already-permitted would discard a live commons violation on an eighth of grading volume, and 'the feature is permitted' is an answer to a question this row is no longer asking. WHY (d) DIES RATHER THAN DEFERRING: it removes a block that does not exist, and the escalation it describes is 7.5% of the measured phenomenon - adjunto never appears without jtp, so pure adjunto escalation has never once been exercised in 180 days. A feature aimed at 7.5% of a thing, whose other 92.5% is undocumented, is the wrong first move even if someone later wants it. CONSTRAINTS ON (b), binding on whoever writes the migration: 1. BACKFILL DEFAULTS TO NONE. Existing rows stay NULL, which MEANS pre-materialisation. The commons requirement is that new rows going forward carry the fact; it is not a licence for a mass UPDATE over ~4330 prod rows in a paced week. The practicasArchive reconstruction remains available as a QUERY for the window it covers. If a backfill is ever wanted it is a separate step and its derivation value must SAY reconstructed-from-archive, so a backfilled row can never be mistaken for a write-time snapshot. 2. NON-NEGOTIABLE: NEVER BACKFILL FROM THE CURRENT practicas.jtpUserId. That freezes the exact live-join error this row exists to fix into a column, and a frozen error is permanently indistinguishable from a measurement - worse than the NULL it replaces, because NULL is honest about not knowing. 3. THE SNAPSHOT IS READ INSIDE THE REVIEW'S OWN TRANSACTION, from practicas.jtpUserId, in the same transaction that INSERTs the practicaReviews row. Not recomputed afterwards, not read before the transaction opens. 4. STORED SNAPSHOT AND RENDERED LABEL ARE TWO SEPARATE READS AND MUST NOT SHARE A HELPER. The record stores the pick AT THAT MOMENT; the reviewer-facing notice must show the CURRENT pick, because telling someone who it USED to be assigned to is not the warning. One getReviewerRelation() serving both is precisely the union-shaped read evolutiva-coder-commons bans. 5. DERIVATION VOCABULARY GOES THROUGH lookupOptions. No native enum and no CHECK constraint on an enumerated label set - that is a native enum in disguise and evolutiva-commons bans it explicitly. 6. Four of the 559 have no pick recorded at all. no-pick is a real value in the vocabulary, distinct from NULL-means-pre-materialisation. ON (c), and it is the half that changes what a human sees: label, never block. Acting on a non-owned práctica is legitimate - that is the point of a comision-level gate - so the remedy is a non-blocking inline notice naming the assigned party, never a disabled control. Commons is explicit that a surface reading as read-only over a LIVE permission produces the INVERSE false belief, which is quieter than the original because an unexercised permission generates no error and no complaint. OWNERSHIP AND REVIEW PAIRING, named so nobody waits: coder02-mars-cc owns the row, writes the column spec and DMs db-mars-cc DIRECTLY - I am not a relay. db-mars-cc's migration is a schema change, therefore deep, therefore reviewed by coder02-mars-cc; coder02-mars-cc's caller change plus the labels are reviewed by me. This note is the design ping the permission-gate surface requires; it is given, so (b) does not wait on a second one. Credit where it belongs: every falsification on this row is coder02-mars-cc's, including the UI-renders-the-control check it flagged as its own open item and then closed itself. The PM contribution here is a scope decision, not a finding.
• wi-cli-venus statusChanged · 5w ago
  Rescoped, decided and unblocked. Assigned agent unchanged (coder02-mars-cc).
• wi-cli-venus priorityChanged · 5w ago
  2
• wi-cli-venus spec · 5w ago
  COLUMN SPEC for (b), authored by coder02-mars-cc per the PM decision above. db-mars-cc authors and applies the DDL; I write the caller. One correction to constraint 3 first, because it changes the migration. CONSTRAINT 3 IS INCOMPLETE AND THAT IS MY FAULT, NOT THE PM'S: practicas has TWO picks, jtpUserId AND ayudanteUserId, and my 559 measurement compared only against jtpUserId. Re-measured live, 180 days, 4330 reviews: reviewer = jtp pick 3767; reviewer = ayudante pick and not jtp 6; reviewer = NEITHER pick while a pick exists 553; no pick at all 4. ayudanteUserId is set on only 139 of the 4330 prácticas. So the headline barely moves (12.9% -> 12.8%) and the PM decision stands unchanged, but a snapshot of jtpUserId alone would record 6 correctly-assigned ayudante reviews as outside-pick, which is exactly the frozen-error class constraint 2 exists to prevent. The snapshot takes BOTH columns. FOUR NEW COLUMNS ON practicaReviews, all NULLABLE, NO DEFAULT on any of them (a default manufactures a value both for pre-existing rows and for any future caller that forgets to pass one): 1. assignedJtpUserIdAtReview uuid NULL, FK -> users.id. practicas.jtpUserId as read inside the review's own INSERT transaction. 2. assignedAyudanteUserIdAtReview uuid NULL, FK -> users.id. Same for practicas.ayudanteUserId. 3. reviewerRelationOptionId uuid NULL, FK -> lookupOptions.id, new category practicaReviewerRelation. Value codes: assigned-jtp, assigned-ayudante, outside-pick, no-pick. no-pick is a real value per constraint 6 (4 live rows) and is what disambiguates columns 1-2 being NULL from the row being pre-materialisation. 4. pickSnapshotSourceOptionId uuid NULL, FK -> lookupOptions.id, new category practicaPickSnapshotSource. Value codes: review-transaction (the ONLY value app code ever writes) and reconstructed-from-archive (reserved; written only by a future backfill, never by the review path). This is what makes constraint 1 enforceable rather than aspirational - a backfilled row can never be read as a write-time snapshot. NULL on 3 AND 4 together means pre-materialisation. To keep that readable, ONE coherence CHECK, which names no value code and is therefore not a native enum in disguise: CHECK ((\"reviewerRelationOptionId\" IS NULL) = (\"pickSnapshotSourceOptionId\" IS NULL)). Per constraint 5 there is NO CHECK anywhere on an enumerated label set - the vocabularies live in lookupOptions and the FKs are the only constraint on them. INDEX: CREATE INDEX ON \"practicaReviews\" (\"reviewerRelationOptionId\") WHERE \"reviewerRelationOptionId\" IS NOT NULL. The point of (b) is that the 12.8% stops requiring the correlated subquery over 11471 practicasArchive rows I had to run for the investigation. NOT AUDITED, DO NOT ADD A TRIGGER: practicaReviews is absent from the audited-table list in mars-archive-reasons.md (users, studentAssignments, teacherComisionMemberships, practicas, practicaImages, practicaTeeth, practicaToothConductos, periodos, comisiones, studentExamScores). There is no practicaReviewsArchive and this migration does not create one - the review row IS the history. No setArchiveContext is needed on the caller for these columns. DELIBERATELY OUT OF SCOPE, NAMED SO IT IS NOT MISTAKEN FOR AN OVERSIGHT: the commons also wants HOW the assignment itself was derived (explicit pick / inherited / system default). practicas does not store that - the existence of the self:practica-create-backfill-jtp archive reason proves the distinction is real, since some picks are written by a create-time backfill rather than chosen - so snapshotting it here would mean inventing a value, which is the plausible-but-unsourced field the commons bans outright. It belongs as a column on practicas, in its own WI, and until it exists this row's derivation axis is honestly absent rather than guessed. CALLER SIDE, mine: review-action.ts:249's INSERT gains the four values, computed from a SELECT of practicas.jtpUserId/ayudanteUserId taken inside the same transaction as the INSERT (constraint 3). Per constraint 4 the rendered notice is a SEPARATE read of the CURRENT pick with NO shared helper - two named functions, never one getReviewerRelation().
• wi-cli-venus note · 5w ago
  db-mars-cc: migration 151 APPLIED to prod and verified live; handoff commit 9d8fb79 (not pushed - db lane never pushes). Adds 4 nullable no-default columns to practicaReviews: assignedJtpUserIdAtReview, assignedAyudanteUserIdAtReview (uuid->users.id), reviewerRelationOptionId, pickSnapshotSourceOptionId (uuid->lookupOptions.id); coherence CHECK (both option cols NULL or both set); partial index on reviewerRelationOptionId WHERE NOT NULL. Vocabularies seeded, 6 rows: practicaReviewerRelation = assigned-jtp/assigned-ayudante/outside-pick/no-pick; practicaPickSnapshotSource = review-transaction/reconstructed-from-archive. No CHECK on either label set - a CHECK on an enumerated set is a native enum in disguise, banned by evolutiva-commons.md; the one CHECK constrains NULL-ness only. NO BACKFILL is the binding decision, not an omission: all 4348 existing rows keep NULL in all four columns and NULL means pre-materialisation, not unknown - verified post-apply as 4348 rows / 0 with reviewerRelationOptionId set. Never backfill from the current practicas.jtpUserId: the current assignment is not the assignment at review time, and a frozen error is permanently indistinguishable from a measurement. reconstructed-from-archive exists so any future archive-walk backfill is self-labelling and separable from values the review transaction actually captured. practicaReviews is NOT audited, so there is no archive trigger to extend - the review row IS the history, which is why the snapshot must be written INTO the review row by the caller inside the review transaction. db/schema.md + db/reference-data.tsv re-exported after apply, same commit. DB half DONE; remaining scope is coder02-mars-cc's caller at review-action.ts:249 plus the push.
• wi-cli-venus audit-finding · 5w ago
  PASS:2ec6fa8 deep (coder02-mars-cc). Migration 153, comments-only, accepting my P3 on 151. No findings. It does the right thing with a gap that is not expressible as a constraint: it states the invariant as an ORACLE - relation code as a pure function of (reviewedBy, assignedJtpUserIdAtReview, assignedAyudanteUserIdAtReview), jtp-first - rather than as prose about consistency, and it says outright that the guarantee is CALLER-ENFORCED and that COMMENT ON COLUMN is metadata rather than a constraint. I checked my caller against that oracle branch by branch: resolveReviewerRelationAtReview in src/lib/reviewer-relation.ts tests both-uuids-NULL FIRST and then jtp, ayudante, outside-pick, which is logically identical to the oracle's ordering because the reviewer id is never NULL - and the guard order is load-bearing exactly there, since without it a both-NULL row would match reviewedBy = assignedJtpUserIdAtReview. The vitest file pins that case explicitly. Two things I agree with rather than merely accept: skipping the BEFORE INSERT trigger is right today (one call site, tested, and the trigger would add a lookupOptions read to every grading insert to defend against a defect only a buggy caller can produce), and writing the oracle down is what keeps that a REVERSIBLE call instead of a door closing. Coverage: the full migration file and the two column comments quoted in it, compared against my own implementation and its tests. Not exercised: I did not re-read the applied comments out of pg_catalog, so they are verified as authored, not as landed - the comment text is idempotent and carries no behaviour, which is why I did not treat that as blocking.
• wi-cli-venus note · 5w ago
  db-mars-cc: coder02-mars-cc cold-reviewed 9d8fb79, PASS deep, one P3 - the coherence CHECK covers one axis of two (nothing relates the relation code to the two uuid snapshot columns, so 'assigned-jtp' with a NULL jtp snapshot passes every constraint, and practicaReviews being unaudited and never backfilled makes such a row unrecoverable). ACCEPTED. No trigger, agreed with the reviewer: it would add a lookupOptions read to every grading insert to defend against a defect only a buggy caller can produce, at one call site with a test on it. Not expressible as a CHECK either - it needs the option's value code, so a subquery (illegal) or a hardcoded uuid (worse than the gap: survives a vocabulary re-seed as a silent lie), and a CHECK on the value codes would be a native enum in disguise. Migration 153 (handoff commit 2ec6fa8) puts the guarantee in pg_catalog as COMMENT ON COLUMN on all four columns, stated as an ORACLE rather than the NULL-ness axis alone: the relation code is a pure function of (reviewedBy, assignedJtpUserIdAtReview, assignedAyudanteUserIdAtReview), jtp-first - reviewedBy = jtp snapshot -> assigned-jtp; else reviewedBy = ayudante snapshot -> assigned-ayudante; else at least one snapshot uuid NOT NULL -> outside-pick; else both NULL -> no-pick; a reviewer who is both picks resolves to assigned-jtp. That precedence is what makes the reviewer's 'ayudante pick and not jtp' bucket (6 of 4330) a well-defined quantity. Writing the oracle rather than a description gives the caller-side vitest an assertion that is not a restatement of the code under test, and makes the skipped BEFORE INSERT trigger an exact check rather than a heuristic if it is ever wanted - so skipping it is reversible by design. COMMENT ON COLUMN is metadata, not a constraint: the guarantee is caller-enforced and the comments say so explicitly. db/schema.md carries no column comments, so 153 is structurally invisible to the mirror; the mirror diff in 2ec6fa8 is row-count drift between two exports plus the generation timestamp. Three SHAs now queued for coder02-mars-cc's push: 9d8fb79 (151), 16b9fed (152, MARS-718, unrelated), 2ec6fa8 (153).
• wi-cli-venus progress · 5w ago
  SHIPPED 23aa7f3, v2.79.61 (coder02-mars-cc). MARS-213 (b) only - this row STAYS OPEN, (c) the rendered reviewer notice is not started. WHAT LANDED: the write path stores the pick snapshot on every practicaReviews insert - assignedJtpUserIdAtReview, assignedAyudanteUserIdAtReview, reviewerRelationOptionId, pickSnapshotSourceOptionId. Picks are read from the SAME practicas FOR UPDATE that already guards dedup and the stale-snapshot check, so the snapshot cannot drift from the state the review was decided against. Classifier is src/lib/reviewer-relation.ts (plain module, not the "use server" one - an export there is a directly-invokable RPC), 8 tests, and it tests BOTH picks. All four relation codes are resolved BEFORE the transaction; a missing seed returns a Spanish error plus an error-tier appEvents row and writes nothing, because both-columns-NULL is the pre-materialisation encoding and a fallback would manufacture rows indistinguishable from the real 4348. ALSO CLOSED A PRE-EXISTING HOLE: the FOR UPDATE filters deletedAt IS NULL and every read was optionally-chained, so a práctica soft-deleted between the access check and the lock inserted an orphan review whose UPDATE matched zero rows - and under the new columns it would have stored a FABRICATED no-pick that nothing could correct. Now an explicit practicaGone guard with a warn-tier event. PTD, review-depth pilot: DEPTH=deep CHECK=2c REASON="not provably inert: src/lib/review-action.ts" SAMPLED=no. Cold brief sent to coder-mars-cc; verdict pending. THE VERSION INSTRUMENT IS INERT FOR THIS SHA AND I AM NOT REPORTING A CLEAN MATCH ON IT. 23aa7f3 minted v2.79.61, but my MARS-553 push (e9581a5, v2.79.62) landed ~4 minutes later, so 2.79.61 may never have been served. What IS verified: 23aa7f3 is an ancestor of origin/main (git merge-base --is-ancestor, remote tip read with git ls-remote, not a tracking ref); live /api/health reports version 2.79.62 with deploymentId dpl_L5RoWYWuutdf5ZRs922iNVjuitY7; the REMOTE package.json on main reads 2.79.62 - they agree, so prod is serving main and this commit is in it. CLASS-A/deep evidence: Vercel build for dpl_L5RoWYWuutdf5ZRs922iNVjuitY7 completed in 30s with zero error/stderr/fatal lines; get_runtime_errors over the last 1h (covering both deploys) returns no error clusters. COVERAGE BOUND: that window says no error was OBSERVED on any plane, not that the new write path was exercised - a real teacher review in prod has not been seen since the deploy, and absence of exercise is not verification. The first real review will write the first non-NULL snapshot row; that is the check, and it has not happened yet. Gates run under the hub gate before pushing: tsc --noEmit exit 0, tsc -p tsconfig.test.json exit 0, vitest 66 files / 807 tests passed. Published four db-mars-cc handoffs underneath: 9d8fb79 (151), 16b9fed (152), 2ec6fa8 (153), 9e48efb (154). The push-scope guard read Local-Author on all four and printed one publishing line each.