fix(review): keep notes when restating a stale approval - #411
Draft
cursor[bot] wants to merge 1 commit into
Draft
Conversation
After Approve plus an image rerun the desk re-sends tag_clip:good with notes. Rejecting that no-op dropped the whole atomic batch. Allow an already-good tag to be restated while still blocking a new approval of a missing or stale take. Co-authored-by: ignaciodelcano+dcl <ignaciodelcano+dcl@gmail.com>
PR Review — Loreframe StudioRisk: low Automated review from Findings
Changed files
CONTRIBUTING checklist
Posted by the repo PR review workflow. Re-runs on each push to the PR. |
Code healthQuality score: 63.8/100Higher is better. The score is a trend dashboard; the independent ratchet below remains the CI gate.
Change vs PR base: +0.0 points.
Markdown, JSON catalogs and tests are out of this table. Only Most complex functions
Trend vs baseline
Warnings
Ratchet passed. |
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.
Resumen ejecutivo
Qué cambia
Tras Aprobar un take y regenerar la imagen de inicio, guardar notas en Production Review ya no falla. El desk reenvía
tag_clip: good; el servidor ahora acepta restablecer una aprobación que ya existía y sigue bloqueando una aprobación nueva de un take stale o sin vídeo.Para qué sirve
#406 dejó
video_stalepersistente para que Rejoin no arme un take que ya no coincide con el start frame. El desk, sin embargo, siempre reenvía la decisión actual junto con las notas. Rechazar esegoodno-op tiraba el batch atómico y las notas se perdían.Impacto para el usuario
Puede anotar un plano aprobado después de un rerun de imagen. Rejoin/export siguen bloqueados hasta regenerar el vídeo. No se puede aprobar por primera vez un take stale.
Riesgo
Estado
Summary
After Approve +
rerun_clip_image, the clip staystag=goodandvideo_stale=True. Production Review persist always sendsselect_take+tag_clip: good+note_clip._apply_reviewrejected anygoodtag on a stale clip, so the atomic batch rolled back and notes never landed. Trigger: approve a completed take, rerun the start image, type notes and blur (or right-click compare). Workaround before this fix: Reject first.Fix: treat restating an already-good tag as a no-op that is allowed on a stale clip. Still reject a new approval when the take is missing or stale. Rejoin/export continue to read
video_staleindependently.Overview
tag=good).video_stale=Trueand keeps the tag.persistCommandsForrestatestag_clip: goodwith notes/select.video_staleset.tag_clip: goodon a stale/missing take still 400s and writes nothing.Detailed changes
Backend
_apply_reviewindirector_review.py: the stale/missing guard fortag=goodnow applies only whenclip.tag != "good".UI and Wizard
No runtime UI change. Unit test documents that notes persist restates
tag: goodon an approved+stale shot (the command shape the server must accept).Data, provenance and compatibility
No migration. Checkpoints keep
tag=good+video_stale=Trueafter notes persist, matchingtest_review_notes_keep_a_stale_selected_take_stale.Files and ownership
app/services/director_review.py— idempotent restated approvaltests/test_director_review.py— desk-shaped batch succeeds; new approval of stale still rolls backui/tests/productionReview.test.ts— desk command contract for approved+stale notesUntouched:
_select_takestill clears stale when switching to a different take (documented as intended). Rejoin stale gate from #406 unchanged.Validation
Date (UTC): 2026-09-12
Base SHA:
5da575a267fc8691c785d945d5a388fb9f43d931Head SHA:
bb11d1c3f2939c9e3ef287d30356c09f86081f7fValidation scope: focused
python scripts/verify_clean_repo.pypython -m compileall -q app/services/director_review.py— okFocused Python tests:
/workspace/.venv/bin/python -m pytest tests/test_director_review.py -q— 11 passedcd ui && npm run i18n:checkUI tests:
npx tsx --test tests/productionReview.test.ts— 11 passed;tests/productionReviewRuntime.test.tsx— 14 passedcd ui && npm run lint -- --max-warnings=0cd ui && npm run buildgit diff --check— cleanE2E/smoke checks: N/A — persist contract is covered by unit tests; no visual UI change
Code quality
main: pending CICI and review
CI of this HEAD: pending
Independent agent review of this HEAD: pending
Human merge click (operational, not code review): pending
Reviewed at current HEAD
Earlier review; HEAD has changed since (stale)
Pending
Unavailable
Coste de la tarea
Notes and limitations
Switching to a different historical take still clears
video_stale(_select_take/ persist.ts). That behavior is covered by an existing projector test and was left unchanged.Follow-up work
None.
Checklist