fix(annotator): completing a job flips the workspace to read-only in place - #447
Merged
Conversation
…place A completed job went on declaring `annotate` on every one of its frames, and went on accepting the labels that declaration promised. Completing a job does not complete its batch — `BatchService` derives that separately — so the batch gate had nothing to say, and the annotation workspace stayed a live editor over work it had just been told was finished. The gate is the kernel's, not the page's. `OPEN_JOB_STATES` is the set both sides read: `asset_actions` takes the job's state as a third dimension, and `JobService.require_open_job` refuses the three annotation writes and `mark` with `JobFinished` (409 `JOB_FINISHED`), so the declaration and the refusal cannot disagree. The workspace then flips through the invalidation the Finish mutation already performed — in place, on every frame, with nothing in the browser computing legality. Beside it, the frame's own verbs leave the bar once the **job** is closed: `Skip` / `Un-skip` and the flow verb are absent rather than disabled on every frame of a completed batch or a finished job. The gate is the job and not the frame, because a settled frame inside a working job still keeps its `Un-skip` and still owes the cluster its measured width. `Finish job` stays either way — `complete` is the job's declaration, not the frame's.
This was referenced Aug 8, 2026
JArmandoAnaya
added a commit
that referenced
this pull request
Aug 8, 2026
…439) The skill encoded the pre-#447 world: annotation writes gated on asset progress alone, with no job dimension. Left as it was, the next design session consulting it re-derives the hole that #439 closed. Decision 2 now states both dimensions and names OPEN_JOB_STATES as the one set the declaration (asset_actions) and the refusal (require_open_job) both read, so they cannot disagree; it also states why the batch gate never covered this — a job completing does not complete its batch. Decision 9 records the two rulings #447 shipped and a later reader would tidy away: frame-verb gating is job-level, and Finish job stays reachable on a settled last frame. Prose sweep for the same false sentence: docs/annotations.md, docs/jobs.md, docs/ui.md, docs/mcp.md, docs/mcp-walkthrough.md, mcp/_autostart.py, AnnotationPage.tsx and GalleryScreen.tsx all described one or two gates where there are now three. ui-capabilities gains the read-only transition, which is what a mid-session Finish now performs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #439
The diagnosis, corrected before the fix
The issue was filed as a stale-declaration bug: the mutation completes, the workspace keeps the
allowed_actionsit already held, and the mode never re-derives. That reproduction was run firstand did not hold. A
TestClientwalk over the real kernel — approve, start, annotate everyasset,
POST /jobs/{job_id}/complete— answers:batch.statein_annotationjob.state/job.allowed_actionscompleted/[]allowed_actions["annotate", "skip", "submit_for_review"]POST /jobs/{job_id}/annotationsThree facts follow, and together they move the fix a layer down:
JobService.completesays so in its owndocstring;
BatchServicederives batch completion from the jobs when asked, and the workspacenever asks. So the batch gate — the only dimension
asset_actionshad past progress — cannotsee a finished job at all.
stale one.
useJobTransition'sonSuccessalready invalidatedjobKeys.job(jobId),["projects"]and["batches"], and the last of those prefix-covers the workspace's own assetquery key. The refetch happened; the refetched answer was identical. Adding invalidation would
have been a no-op.
AnnotationServicegated onrequire_open_batchplusWRITABLE_PROGRESS, andJobService.markonrequire_open_batchalone — job state was nowherein either path. Labels written after Finish were stored, and an asset in a finished job could be
un-skipped back to
unannotated, unsettling a job whose own record already said every asset wasdealt with.
So
Finisheddescribed nothing, and the layer holding the wrong answer was the wire. Ruled onbefore any code was written (2026-08-08): a completed job is closed for writes, and the gate
belongs in the kernel, because the alternative — reading
jobStatein the page — is the frontenddeciding legality, which
ui-capabilitiesbans and which would have left the CLI, MCP and RESTstill writing into finished work.
What changed
The kernel, and one set read by both sides.
OPEN_JOB_STATES({pending, in_progress}) joinsWRITABLE_PROGRESSandSETTLED_PROGRESSindomain/task.py, stated outright rather than derivedas "the states with a move left" —
PROMOTABLE_PROGRESS's argument, since the two agree today bycoincidence.
asset_actions(progress, *, batch_state, job_state)— three dimensions, none of them optionaland none defaulted, so a caller cannot drop one the way the browser's old mirror dropped the
batch dimension.
JobService.require_open_jobraisesJobFinished(409JOB_FINISHED). Called by the threeannotation writes and by
mark, beforemark's documented no-op check — writing into workthat is over is a bug whether or not the value would have changed. Reads are untouched:
for_assetpasses membership alone, because a viewer over finished work has to be able to showit.
server/models.pyandvisionset/wire/) and bothlistings that build them, so REST and MCP answer identically. No schema change — the field is
allowed_actionsand it simply comes back empty, soopenapi.jsonand the generated client arebyte-identical.
The frontend does not compute anything new. The mode flips because
readOnlyre-derives from adeclaration that finally moved. What the page gains is copy and one absence:
finished, so its frames can no longer be edited." It names no route onward, deliberately —
JOB_TRANSITIONShas no way back and the batch is still open, so there is no correction tooffer. The three causes are held as one value (
workflowBecause) because three places have toagree on them: the banner renders for either workflow cause, the skipped notice yields to either,
and the sentence is whichever spoke. Without that, a skipped frame in a finished job would have
reproduced Completed batch opens editable: edit functions remain active in the read-only workspace #423 one level down.
Skip/Un-skipand the flow verb(
Save and next/Next) are absent rather than disabled on every frame of a completed batch ora finished job — the tool strip's rule applied to the bar, since all three only ever move this
frame and there is no move behind any of them there. The
browse | resolvedivider goes withthem.
toast.success, the add-a-class chain's idiom — becauseeverything else the press does is a subtraction, and a screen with less on it is not an
explanation.
Two deviations from the task, both named, both found by a test
The task's §B3 says the flow verbs are absent "in read-only". Taken literally — gating on the
frame's
readOnly— it breaks two shipped rules, and the browser suite said so rather than review:Finish jobis kept.completeis the job's declaration, not the frame's. A job whoselast frame is
accepted— read-only, while other frames are still outstanding — would have hadno way to be finished from the workspace at all. The cycle walk is the proof rather than the
argument: it accepts its last frame and then presses Finish, so removing the control breaks
the real-kernel suite. Where it is withheld it already explains itself (Finish job on the last frame: disabled state must explain itself #427); once pressed it
reads
Finished.The gate is the job, not the frame. A per-frame gate cost six e2e tests on the first run,
and two of them were load-bearing.
Un-skipis the one way back out of a skipped frame, and askipped frame is read-only — gating on the mode strands it under a notice promising a control
that is not there, which is Completed batch opens editable: edit functions remain active in the read-only workspace #423 reproduced one level down. And Annotation top bar: consolidate image navigation into a centered cluster #416 measured the navigation
cluster to a constant width so that walking a job does not move the arrows under a cursor; a
slot that emptied and refilled frame by frame through a mixed job breaks that.
frameVerbsis thereforeworkflowBecause === null— a closed batch or a finished job, both ofwhich withhold every move on every frame alike, so the cluster is uniformly narrower and nothing
jitters. That is also exactly the state §B3 is about.
The behaviour was written down as correct
tests/mcp/test_job_tools.py::test_a_write_into_a_completed_job_starts_nothingasserted the writelands, and said why in its own docstring: "Writing here is legal — the gate is the batch, and its
batch is still
in_annotation." That sentence is the bug, stated as a rule. The claim the testactually exists to protect is #109's — that
add_annotationsauto-starting apendingjob mustnever drag a completed one backwards — and that claim survives intact; only the "and the write
still lands" half is gone. It now asserts the refusal and the unchanged job state together, because
a refusal that had nevertheless started the job would be the same backwards move by another road.
tests/server/test_errors.py's exact-correspondence table also went red on its own, which is thetable doing its job: an unmapped kernel error answers 500 the day a route reaches it.
Already true at HEAD, asserted rather than implemented
Of the three surfaces the task asked to verify, two needed no work and are now pinned on the far
side of the transition instead of only on a completed batch: the tool strip has been absent
rather than disabled since #306, and the classes panel absent with the objects region at full
height is #431's, re-measured post-flip. Navigation was already ungated and is asserted
working —
‹›, the counter and the gallery — with no save-first guard engaging.Found, not fixed
BatchAssetOutpublishesjob_idbut notjob_state, so a client that wanted to render why aframe is frozen would have to fetch the job. Nothing needs it today — the workspace already holds
the job — and adding a field to a published model is a wire change that should be asked for.
allowed_actionsand therefore go quiet on afinished job's frames too, which is correct; whether they should say why is a separate design
question and not smuggled in here.
Skipon anacceptedorreview_pendingframe is still a disabledcontrol with no reason attached to it — the
titlespread it carried wasclosedBecause, whichis null in exactly the case the control now renders in, so the spread was dead and is gone. The
banner above says why the frame is a viewer, which is the one-explanation-surface rule; whether
the control should carry it too is Finish job on the last frame: disabled state must explain itself #427's question applied to a second control.
Test plan
tests/kernel/test_capabilities.py— the asset matrix gains its third dimension.(batch, job, progress)scenarios now includein_progressthroughout and the row where the bug lived: acompleted job inside an
in_annotationbatch. Plus three sentence-tests, one of which(
test_the_asset_gate_reads_the_same_set_the_services_refuse_by) makes the stronger claim — thatwhat an asset declares is computed from the very set the service raises against, so a third job
state cannot be admitted by one and refused by the other.
tests/kernel/test_job_service.py— a finished job refuses a progress move inside an open batch;the gate fires before the no-op; a second job of the same batch carries on, which is the
difference between this gate and the batch's.
tests/kernel/test_annotation_service.py— all three writes refused, nothing moved; reads stillwork.
tests/server/test_annotations.py— 409JOB_FINISHEDover HTTP with the batch deliberately leftopen, the declarations asserted empty in the same test (declaration and refusal agreeing is the
contract), the progress route refused by the same code, and an open-job control.
frontend/app/e2e/annotate.spec.ts— the deterministic reproduction, in chromium: drive to thelast frame with everything resolved, press Finish, and assert the viewer with no reload. A
sentinel written on
windowbefore the press is read back after it, so a reload fails the testrather than passing it. Then: banner and its sentence, no palette / classes region / Skip /
Save and next,
Finished, the objects region measured at full panel height, Read-only workspace: no class panel, no edit affordances, selection syncs to the panel #426's handle andselection-sync rules on the far side, the flip on a middle frame after navigating, and the
gallery still opening.
frontend/app/cycle/cycle.spec.ts— the same claim against a real server and a real kernel, atthe point the walk already presses Finish, with the batch still
in_annotation.frontend/ui-core/src/annotator/topBar.test.tsx— the filled-slot sweep becomes "at most one",which is the rule change stated where it was pinned.
Mutation verification, each applied by its exact diff with the anchor asserted before and after,
reverted by
git apply -R, tree byte-identical to this commit at the end:job_stategate removed fromasset_actionstest_a_finished_job_offers_its_assets_nothing_even_in_an_open_batch; the asset matrixrequire_open_jobremoved from the three annotation writestest_nothing_is_written_after_the_job_finishes_even_with_the_batch_open;test_no_annotation_is_written_into_a_finished_jobrequire_open_jobremoved fromJobService.marktest_a_finished_job_refuses_a_progress_move_inside_an_open_batch;test_the_job_gate_fires_before_the_no_opframeVerbsforced true (the verbs render on a closed job)finishedBecausecause removedGate stages, run in the worktree (staged, because a full
pytesthere runs past the harness'sceiling; exit codes verbatim):
uv run pytest -q -rf tests/kerneluv run pytest -q -rf --ignore=tests/kerneluv run ruff check ./uv run ruff format --check .uv run mypy src/visionsetuv run lint-importspnpm -r build/pnpm test/pnpm -r lint(all three annotator boundary gates)CI=1 playwright test, 231 tests)CI=1 playwright test -c playwright.cycle.config.ts, real server + real kernel)The two kernel failures are pre-existing on
mainand tracked as #444 — merged under thebaseline-proof exception (
refactor-protocol, added by #446), steppython tests, cf. #444.Branch run,
uv run pytest -q -rf tests/kernel(exit 1):Baseline run, the same two node ids on unmodified
mainat the merge-base1a59521, workingtree clean (
git status --shortempty), same machine:Identical, and the cause is #444's rather than anything here: this diff touches capability
declarations, the job and annotation service gates, two wire projections and the annotation
workspace, while those two tests exercise ffmpeg frame extraction from a truncated clip —
ffmpeg_video_processor.py, which this branch does not touch at all. ThepythonCI job is thearbiter and is green on this PR.