Week 16: The Editor That Learned When to Say No

With the testbench model and the runner both fixed and moving, the test case
editor finally got its own review round this week. It found the same class
of bug the model found last week, in the client side JSON this time instead
of the server side validation, and fixing it properly meant redesigning a
piece of the editor rather than patching around the symptom. A second,
unrelated bug turned up in a completely different pull request, caught only
because a setting this phase just added made an old code path reachable for
the first time.

The overwrite that would have erased a title on every save

First finding, on Assignment#testbench_data=:

(testbench || build_testbench).data = parsed

This replaces the entire data hash with whatever the editor's JSON posts.
The editor only ever authors type and groups. If an existing suite had
any other top level key, a title, say, something the simulator itself reads
at testbench.js:872, it survives right up until the next time an instructor
edits an unrelated setting and saves the form. Then it is gone, not because
anyone touched it, but because the write path never preserved what it was not
explicitly given.

tb = testbench || build_testbench
tb.data = (tb.data || {}).merge(parsed)

Merge instead of replace. I wrote a regression test that seeds a suite with a
title, saves an unrelated change through the editor, and asserts the title
survives, then deleted the merge to confirm the test catches the loss. It
does.

The same binding bug, one layer up

The second finding was the model's groups[0] binding problem again, except
this time the mismatched pins would come from the editor itself, from an
instructor adding a second test case row with different inputs than the
first:

each row becomes its own group, but the simulator binds pins from
groups[0] only. a row with a different pin set than the first makes
group.outputs.find(...).results.push return undefined and throw.

Same root cause as last week, different code path producing the malformed
suite. The fix is a shape comparison in the JavaScript controller, mirroring
what the Ruby model already does server side:

sameShape(groups) {
    const shape = (group) => JSON.stringify(['inputs', 'outputs'].map((side) => group[side].map((p) => [p.label, p.bitWidth])));
    return groups.every((group) => shape(group) === shape(groups[0]));
}

Submission blocks client side now if any row's pins do not match the first
row's, with the same pattern used for incomplete rows: preventDefault,
stopPropagation because rails-ujs still disables the submit button on a
merely prevented submit as it bubbles to its document level listener, show
the error, stop.

A third finding, a values regex that accepted any non-empty string instead
of exact-width binary digits, was the exact bug from last week's model
review, just not yet enforced in the form that produces the JSON in the
first place. Tightened the same way: [01]+, checked against the declared
bit width.

Why I did not try to merge the unsupported case in

The harder finding was about suites the editor cannot fully represent:
multi vector test cases, where one row of the JSON actually encodes several
values per pin instead of one. My first fix tried to preserve these by
tracking each group's original array position, splitting edited groups from
untouched ones, and re-merging them back into the original order on save.
It worked, but CodeRabbit found two more bugs in that merge logic within
a day: the merge could still reorder an unsupported group relative to an
edited one, and it silently rewrote the suite's type from seq to comb
on every save because the serialization always hardcoded comb.

Two fixes for a problem that a different design does not have in the first
place made me stop and reconsider the approach rather than the code. The
editor cannot safely round trip a multi vector suite. Trying to merge it
back together precisely is solving a harder problem than the feature needs:
tracking position, preserving type, reconciling two sources of truth. The
simpler rule is to never let the editor touch what it cannot fully
represent. If any group in a suite has more than one value per pin, the
whole editor goes read only for that assignment, with a note saying why,
instead of rendering an editable interface that would corrupt the suite the
moment someone saved through it.

def test_case_editable?(assignment)
  test_case_groups(assignment).all? do |group|
    (group["n"] || 1) == 1 &&
      (group["inputs"] || []).all? { |pin| Array(pin["values"]).size <= 1 } &&
      (group["outputs"] || []).all? { |pin| Array(pin["values"]).size <= 1 }
  end
end

This deleted the position tracking, the merge-back logic, and every test
written against it, and replaced all of it with one predicate and an
if/else in the view. The line count dropped by about sixty lines for a
bigger guarantee: nothing this editor cannot represent can be silently
damaged by it, because it is never editable in the first place.

Getting back under budget without cutting a fix

This phase holds pull requests to roughly two hundred hand written lines.
Chasing every one of these findings, plus the accessibility fixes an
automated check flagged separately, pushed this diff past four hundred at
its peak. Cutting it back down meant finding the difference between a line
that is load bearing and a line that is just verbose.

Consolidating five near identical validation specs into one table driven
test recovered real lines without losing a single case. Dropping a helper
spec file for a follow up PR, per this phase's own stated rule about
splitting spec edge cases rather than thinning validation, recovered more.
The read only redesign recovered the most of all, because it deleted code
that existed to solve a problem a different design does not have. What did
not move: the testbench model's shape and label validation, the values
regex, the malformed pin token check. Those stayed exactly as strict as the
reviews asked for, because a line count is a budget for how a feature is
built, not a ceiling on whether a reported bug gets fixed.

The bug that was always possible and just became reachable

A different pull request from this same run, the one adding autograde
settings like max attempts, got a review comment that had nothing to do with
any of the new columns:

when a mentor submits max_attempts: 0, the new validation rejects
@assignment.update. the action renders :edit, but
after_action :check_reopening_status still runs. if the assignment has a
forked submission, Assignment#check_reopening_status can move its
assignment association and destroy the fork despite the failed update.

after_action callbacks run regardless of whether the action they follow
succeeded. This one always had that property. It just never mattered before,
because almost nothing about updating an assignment used to fail validation.
Adding a max_attempts column with an actual numericality check gave
mentors, for the first time, an easy way to submit a rejected update, which
is the one condition this pre-existing callback was never guarded against.

def check_reopening_status
  return if @assignment.errors.present?

  @assignment.check_reopening_status
end

Two lines. I reverted them and reran the new regression test to confirm a
rejected update really does destroy a student's forked submission without
the guard, which it does, then restored the fix and confirmed it survives.
The bug was not introduced this phase. The conditions for triggering it
were.

Where the project stands

Three pull requests carry review fixes now instead of open findings: the
testbench model, the runner, and the test case editor, the last one smaller
and safer than when it went up. The autograde settings pull request gained
an unrelated but real fix along the way. Underneath all of it, the actual
grading pipeline, a result model that snapshots the suite it ran against
rather than pointing at the live one, a job that calls the runner and
respects max attempts, a mapper from raw score to whatever scale the
assignment actually uses, and a results page that redacts hidden test cases
unless an instructor has chosen to reveal them, went up this week as well.

Next week: watching whether any of these five pull requests actually merge,
and what the first one to land changes for everything still stacked behind
it.