From 9536928054c7aa06a23eb77f0edd68b3977afd2c Mon Sep 17 00:00:00 2001 From: JSLMPR Date: Sat, 11 Jul 2026 09:36:56 +0200 Subject: [PATCH] Add cinematic edit approval gate --- docs/cinematic-editing-runbook.md | 14 +++++ ...uction-cinematic-highlight-editing-plan.md | 3 +- .../config/VideoClippingProperties.java | 20 +++++++ .../editing/EditPlanInboxScanner.java | 53 +++++++++++++++++-- .../application-cinematic-editing-local.yml | 2 + src/main/resources/application.yml | 2 + .../config/VideoClippingPropertiesTest.java | 8 ++- .../CinematicEditingLocalProfileTest.java | 4 ++ .../editing/EditPlanInboxScannerTest.java | 34 ++++++++++++ 9 files changed, 133 insertions(+), 7 deletions(-) diff --git a/docs/cinematic-editing-runbook.md b/docs/cinematic-editing-runbook.md index 406f1cc..2c7e441 100644 --- a/docs/cinematic-editing-runbook.md +++ b/docs/cinematic-editing-runbook.md @@ -60,6 +60,8 @@ video-clipping: source-directory: ./input/editing/source poll-interval-ms: 5000 auto-render-when-plan-appears: false + require-approval-before-render: true + approval-file-name: approved.flag ``` ## 3. Prepare A Source Project Folder @@ -286,6 +288,18 @@ If you set `VIDEO_EDITING_LOCAL_DIRECTOR_AUTO_RENDER=true`, the service renders output/edit-projects/porsche-session-001/inbox/edit-plan.json ``` +By default, automatic rendering is still gated by human review. To approve a validated plan for automatic rendering, create: + +```text +output/edit-projects/porsche-session-001/inbox/approved.flag +``` + +On the next inbox scan, the service renders the already imported `edit-plan.json`. To disable the approval gate for local experiments, set: + +```bash +VIDEO_EDITING_LOCAL_DIRECTOR_REQUIRE_APPROVAL=false +``` + ## 9. Find The Edited Video After rendering completes, the final output is: diff --git a/docs/production-cinematic-highlight-editing-plan.md b/docs/production-cinematic-highlight-editing-plan.md index 10d1b2b..9ca9637 100644 --- a/docs/production-cinematic-highlight-editing-plan.md +++ b/docs/production-cinematic-highlight-editing-plan.md @@ -317,7 +317,8 @@ The plan should be strict JSON so a cheaper model or deterministic renderer can - [x] Milestone 16 progress: Added FFmpeg-backed local director integration coverage for car, food, family, and generic fixture projects, verifying category artifacts, highlight candidates, and director prompts. - [x] Milestone 17: Add benchmark metrics for analysis time, render time, token usage, asset generation cost, and final output size. - [x] Milestone 17 progress: Added Micrometer benchmark summaries for analysis clip/error counts, render output duration, render output size, AI director prompt token estimate, and asset generation cost baseline. -- [ ] Milestone 18: Add a human review mode where the user can approve or edit the AI director plan before rendering. +- [x] Milestone 18: Add a human review mode where the user can approve or edit the AI director plan before rendering. +- [x] Milestone 18 progress: Added a configurable local director approval gate so imported plans wait for `inbox/approved.flag` before automatic rendering, with profile defaults, runbook guidance, and scanner tests. ## Testing Strategy diff --git a/src/main/java/org/example/videoclips/config/VideoClippingProperties.java b/src/main/java/org/example/videoclips/config/VideoClippingProperties.java index 8118068..288ce81 100644 --- a/src/main/java/org/example/videoclips/config/VideoClippingProperties.java +++ b/src/main/java/org/example/videoclips/config/VideoClippingProperties.java @@ -806,6 +806,10 @@ public class VideoClippingProperties { private boolean autoRenderWhenPlanAppears = false; + private boolean requireApprovalBeforeRender = true; + + private String approvalFileName = "approved.flag"; + public boolean isEnabled() { return enabled; } @@ -877,6 +881,22 @@ public class VideoClippingProperties { public void setAutoRenderWhenPlanAppears(boolean autoRenderWhenPlanAppears) { this.autoRenderWhenPlanAppears = autoRenderWhenPlanAppears; } + + public boolean isRequireApprovalBeforeRender() { + return requireApprovalBeforeRender; + } + + public void setRequireApprovalBeforeRender(boolean requireApprovalBeforeRender) { + this.requireApprovalBeforeRender = requireApprovalBeforeRender; + } + + public String getApprovalFileName() { + return approvalFileName; + } + + public void setApprovalFileName(String approvalFileName) { + this.approvalFileName = approvalFileName; + } } } diff --git a/src/main/java/org/example/videoclips/editing/EditPlanInboxScanner.java b/src/main/java/org/example/videoclips/editing/EditPlanInboxScanner.java index d19e7fb..4aaf7b3 100644 --- a/src/main/java/org/example/videoclips/editing/EditPlanInboxScanner.java +++ b/src/main/java/org/example/videoclips/editing/EditPlanInboxScanner.java @@ -31,6 +31,8 @@ public class EditPlanInboxScanner { private final Path projectRoot; private final String expectedPlanFileName; private final boolean autoRender; + private final boolean requireApprovalBeforeRender; + private final String approvalFileName; private final ObjectMapper objectMapper; private final EditProjectStore store; private final EditProjectService projectService; @@ -72,6 +74,8 @@ public class EditPlanInboxScanner { this.projectRoot = Path.of(properties.getEditing().getProjectDirectory()); this.expectedPlanFileName = properties.getEditing().getLocalDirector().getExpectedPlanFileName(); this.autoRender = properties.getEditing().getLocalDirector().isAutoRenderWhenPlanAppears(); + this.requireApprovalBeforeRender = properties.getEditing().getLocalDirector().isRequireApprovalBeforeRender(); + this.approvalFileName = safeFileName(properties.getEditing().getLocalDirector().getApprovalFileName()); this.objectMapper = objectMapper; this.store = store; this.projectService = projectService; @@ -89,7 +93,13 @@ public class EditPlanInboxScanner { return; } try { - findNextPlan().ifPresent(this::importPlan); + Optional nextPlan = findNextPlan(); + if (nextPlan.isPresent()) { + importPlan(nextPlan.get()); + } else { + findNextApprovedProject().ifPresent(project -> renderIfApproved(project.getFileName().toString(), + project.resolve("inbox"))); + } } finally { scanning.set(false); } @@ -111,6 +121,22 @@ public class EditPlanInboxScanner { } } + Optional findNextApprovedProject() { + if (!autoRender || !requireApprovalBeforeRender || !Files.isDirectory(projectRoot)) { + return Optional.empty(); + } + try (Stream projects = Files.list(projectRoot)) { + return projects.filter(Files::isDirectory) + .sorted(Comparator.comparing(path -> path.getFileName().toString())) + .filter(path -> Files.isRegularFile(path.resolve("edit-plan.json"))) + .filter(path -> Files.isRegularFile(path.resolve("inbox").resolve(approvalFileName))) + .filter(path -> !Files.exists(path.resolve("render-manifest.json"))) + .findFirst(); + } catch (IOException ex) { + throw new IllegalStateException("Unable to scan approved edit projects: " + projectRoot, ex); + } + } + private void importPlan(Path inboxPlan) { String projectId = inboxPlan.getParent().getParent().getFileName().toString(); log.info("event=edit_plan_inbox_detected project_id={} file={}", projectId, inboxPlan); @@ -134,16 +160,25 @@ public class EditPlanInboxScanner { move(inboxPlan, inboxPlan.resolveSibling(expectedPlanFileName + ".accepted")); EditProjectResponse project = projectService.getProject(projectId); projectService.updateProject(projectId, EditProjectStatus.PLANNED, Path.of(project.inputDirectory()), null); - log.info("event=edit_plan_imported project_id={} decision_count={} auto_render={}", - projectId, plan.decisions().size(), autoRender); + log.info("event=edit_plan_imported project_id={} decision_count={} auto_render={} approval_required={}", + projectId, plan.decisions().size(), autoRender, requireApprovalBeforeRender); log.info("event=edit_plan_saved project_id={} decision_count={} source=inbox", projectId, plan.decisions().size()); if (autoRender) { - renderer.orElseThrow(() -> new IllegalStateException( - "Automatic rendering is enabled but no EditRenderer is available")).render(projectId); + renderIfApproved(projectId, store.inboxDirectory(projectId)); } } + private void renderIfApproved(String projectId, Path inbox) { + if (requireApprovalBeforeRender && !Files.isRegularFile(inbox.resolve(approvalFileName))) { + log.info("event=edit_render_waiting_for_approval project_id={} approval_file={}", + projectId, inbox.resolve(approvalFileName)); + return; + } + renderer.orElseThrow(() -> new IllegalStateException( + "Automatic rendering is enabled but no EditRenderer is available")).render(projectId); + } + private EditPlan normalized(EditPlan plan) { return new EditPlan(plan.projectId(), plan.style(), plan.targetDurationSeconds(), List.copyOf(plan.decisions()), List.copyOf(plan.audioCues()), List.copyOf(plan.voiceover()), @@ -157,4 +192,12 @@ public class EditPlanInboxScanner { throw new IllegalStateException("Unable to archive edit plan inbox file: " + source, ex); } } + + private static String safeFileName(String fileName) { + if (fileName == null || fileName.isBlank() || !Path.of(fileName).getFileName().toString().equals(fileName) + || fileName.contains("..")) { + throw new IllegalArgumentException("Approval file name must be a plain file name: " + fileName); + } + return fileName; + } } diff --git a/src/main/resources/application-cinematic-editing-local.yml b/src/main/resources/application-cinematic-editing-local.yml index 5bf4be4..8d85f96 100644 --- a/src/main/resources/application-cinematic-editing-local.yml +++ b/src/main/resources/application-cinematic-editing-local.yml @@ -6,6 +6,8 @@ video-clipping: local-director: enabled: true auto-render-when-plan-appears: false + require-approval-before-render: true + approval-file-name: approved.flag logging: level: diff --git a/src/main/resources/application.yml b/src/main/resources/application.yml index 446478d..6434688 100644 --- a/src/main/resources/application.yml +++ b/src/main/resources/application.yml @@ -53,6 +53,8 @@ video-clipping: director-prompt-file-name: ${VIDEO_EDITING_LOCAL_DIRECTOR_PROMPT_FILE_NAME:ai-director-prompt.md} expected-plan-file-name: ${VIDEO_EDITING_LOCAL_DIRECTOR_EXPECTED_PLAN_FILE_NAME:edit-plan.json} auto-render-when-plan-appears: ${VIDEO_EDITING_LOCAL_DIRECTOR_AUTO_RENDER:false} + require-approval-before-render: ${VIDEO_EDITING_LOCAL_DIRECTOR_REQUIRE_APPROVAL:true} + approval-file-name: ${VIDEO_EDITING_LOCAL_DIRECTOR_APPROVAL_FILE_NAME:approved.flag} logging: level: diff --git a/src/test/java/org/example/videoclips/config/VideoClippingPropertiesTest.java b/src/test/java/org/example/videoclips/config/VideoClippingPropertiesTest.java index 4bc0eec..3c1c91e 100644 --- a/src/test/java/org/example/videoclips/config/VideoClippingPropertiesTest.java +++ b/src/test/java/org/example/videoclips/config/VideoClippingPropertiesTest.java @@ -53,6 +53,8 @@ class VideoClippingPropertiesTest { assertThat(properties.getEditing().getLocalDirector().getSourceDirectory()) .isEqualTo("./input/editing/source"); assertThat(properties.getEditing().getLocalDirector().isAutoRenderWhenPlanAppears()).isFalse(); + assertThat(properties.getEditing().getLocalDirector().isRequireApprovalBeforeRender()).isTrue(); + assertThat(properties.getEditing().getLocalDirector().getApprovalFileName()).isEqualTo("approved.flag"); }); } @@ -79,7 +81,9 @@ class VideoClippingPropertiesTest { "video-clipping.editing.local-director.enabled=false", "video-clipping.editing.local-director.source-directory=/tmp/source", "video-clipping.editing.local-director.poll-interval-ms=9000", - "video-clipping.editing.local-director.auto-render-when-plan-appears=true" + "video-clipping.editing.local-director.auto-render-when-plan-appears=true", + "video-clipping.editing.local-director.require-approval-before-render=false", + "video-clipping.editing.local-director.approval-file-name=go.flag" ) .run(context -> { VideoClippingProperties properties = context.getBean(VideoClippingProperties.class); @@ -104,6 +108,8 @@ class VideoClippingPropertiesTest { assertThat(properties.getEditing().getLocalDirector().getSourceDirectory()).isEqualTo("/tmp/source"); assertThat(properties.getEditing().getLocalDirector().getPollIntervalMs()).isEqualTo(9000); assertThat(properties.getEditing().getLocalDirector().isAutoRenderWhenPlanAppears()).isTrue(); + assertThat(properties.getEditing().getLocalDirector().isRequireApprovalBeforeRender()).isFalse(); + assertThat(properties.getEditing().getLocalDirector().getApprovalFileName()).isEqualTo("go.flag"); }); } diff --git a/src/test/java/org/example/videoclips/editing/CinematicEditingLocalProfileTest.java b/src/test/java/org/example/videoclips/editing/CinematicEditingLocalProfileTest.java index 8435ed4..958307d 100644 --- a/src/test/java/org/example/videoclips/editing/CinematicEditingLocalProfileTest.java +++ b/src/test/java/org/example/videoclips/editing/CinematicEditingLocalProfileTest.java @@ -22,5 +22,9 @@ class CinematicEditingLocalProfileTest { assertThat(properties.getProperty("video-clipping.editing.local-director.enabled")).isEqualTo("true"); assertThat(properties.getProperty("video-clipping.editing.local-director.auto-render-when-plan-appears")) .isEqualTo("false"); + assertThat(properties.getProperty("video-clipping.editing.local-director.require-approval-before-render")) + .isEqualTo("true"); + assertThat(properties.getProperty("video-clipping.editing.local-director.approval-file-name")) + .isEqualTo("approved.flag"); } } diff --git a/src/test/java/org/example/videoclips/editing/EditPlanInboxScannerTest.java b/src/test/java/org/example/videoclips/editing/EditPlanInboxScannerTest.java index ce1354e..0569ca3 100644 --- a/src/test/java/org/example/videoclips/editing/EditPlanInboxScannerTest.java +++ b/src/test/java/org/example/videoclips/editing/EditPlanInboxScannerTest.java @@ -89,12 +89,46 @@ class EditPlanInboxScannerTest { void invokesRendererWhenAutoRenderIsEnabled() throws Exception { writeValidPlan(); EditRenderer renderer = mock(EditRenderer.class); + properties.getEditing().getLocalDirector().setRequireApprovalBeforeRender(false); scanner(true, renderer).scan(); verify(renderer).render("porsche-edit"); } + @Test + void waitsForApprovalBeforeAutoRendering() throws Exception { + writeValidPlan(); + EditRenderer renderer = mock(EditRenderer.class); + + scanner(true, renderer).scan(); + + assertThat(store.editPlanFile("porsche-edit")).exists(); + assertThat(store.inboxDirectory("porsche-edit").resolve("edit-plan.json.accepted")).exists(); + verify(renderer, never()).render("porsche-edit"); + } + + @Test + void rendersApprovedPlanOnLaterScan() throws Exception { + writeValidPlan(); + EditRenderer renderer = mock(EditRenderer.class); + EditPlanInboxScanner scanner = scanner(true, renderer); + + scanner.scan(); + Files.writeString(store.inboxDirectory("porsche-edit").resolve("approved.flag"), "approved"); + scanner.scan(); + + verify(renderer).render("porsche-edit"); + } + + @Test + void rejectsUnsafeApprovalFileName() { + properties.getEditing().getLocalDirector().setApprovalFileName("../approved.flag"); + + org.assertj.core.api.Assertions.assertThatIllegalArgumentException().isThrownBy(() -> + scanner(true, mock(EditRenderer.class))); + } + private void writeValidPlan() throws Exception { EditPlan plan = new EditPlan("porsche-edit", "cinematic-porsche-promo", 60, List.of(new EditDecision("clip_00001", 0, 2.5, 0, 2.5,