Conversation
Three video kinds — social-short, ugc-video, product-video — sharing one mechanical render block via include:, plus a human-gated craft review that maintains per-kind playbooks. Content generation (script, description, hashtags, search keywords, alt text) is workflow-owned so it is reproducible and inside the audit trail. Preview/presentation is deliberately NOT here. Needs PEXELS_API_KEY and CARTESIA_API_KEY in the repo-root .env plus ffmpeg and uv; no LLM key of its own. Unconfigured runs fail by name rather than degrading. No secrets are committed.
📝 WalkthroughWalkthroughThe PR adds a complete video workflow pack. Shared scripts fetch footage, generate narration and captions, compose portrait videos, run quality checks, and store artifacts. Product, social-short, and UGC workflows generate content. A human-gated workflow reviews stored videos and updates playbooks. ChangesVideo pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ContentWorkflow
participant video-render-block
participant MediaScripts
participant compose-video
participant quality-check
participant VideoLibrary
ContentWorkflow->>video-render-block: start rendering after brief and copy
video-render-block->>MediaScripts: fetch clips, narration, and captions
MediaScripts-->>video-render-block: media artifacts
video-render-block->>compose-video: compose video.mp4
compose-video-->>video-render-block: rendered video
video-render-block->>quality-check: validate video and create frames
quality-check-->>video-render-block: quality.json and frames
video-render-block->>VideoLibrary: store artifacts and catalog entry
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (4)
.archon/scripts/compose-video.py (2)
26-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilence the
randomlint or usesecrets.Ruff reports S311 as an error on line 29. The selection is a music bed, so
randomis acceptable. Add an inline suppression with the reason to keep the lint gate green.♻️ Proposed change
- return random.choice(tracks) if tracks else None + # Cosmetic variety only; no security property depends on this choice. + return random.choice(tracks) if tracks else None # noqa: S311🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.archon/scripts/compose-video.py around lines 26 - 29, Update the random.choice call in pick_music with an inline Ruff S311 suppression, including a brief reason that non-cryptographic randomness is acceptable for selecting a music bed.Source: Linters/SAST tools
101-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
cwdthroughrunAdd an optional
cwdparameter to_common.run, forward it tosubprocess.run, and replace theos.chdirblock withrun(args, cwd=wd).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.archon/scripts/compose-video.py around lines 101 - 109, Update _common.run to accept an optional cwd parameter and pass it through to subprocess.run. In the video composition flow, remove the os.chdir try/finally block and invoke run with cwd=wd while preserving the existing output reporting..archon/workflows/video/video-render-block.yaml (2)
68-81: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConfirm the concurrency behaviour of
latestandlibrary.jsonl.Two runs of different video kinds can execute in parallel. Line 81 repoints
content/latest, and line 110 appends tocontent/library.jsonl. The staged.partialdirectory protects the per-run directory, but the symlink and the ledger have no such protection. If parallel runs are supported, document the last-writer-wins behaviour oflatest, or write per-kind symlinks.Also applies to: 82-113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.archon/workflows/video/video-render-block.yaml around lines 68 - 81, Update the workflow around the `content/latest` symlink and `content/library.jsonl` append to explicitly handle concurrent runs of different video kinds: either document and preserve last-writer-wins semantics for `latest` while ensuring ledger appends are concurrency-safe, or replace the shared symlink with per-kind symlinks. Keep the existing `.partial` staging protection for each run directory.
79-79: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueBuild
meta.jsonwith a JSON serializer.
printfwith%sdoes not escape the values. IfVIDEO_KINDorRUN_IDcontains a quote or a backslash,meta.jsonbecomes invalid JSON and any consumer fails to parse it. The heredoc on line 82 already runspython3, so write the file there withjson.dumps.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.archon/workflows/video/video-render-block.yaml at line 79, Replace the printf-based meta.json creation in the video render workflow with JSON serialization in the existing Python heredoc that runs afterward. Pass VIDEO_KIND and RUN_ID into that Python block, serialize them with json.dumps as the kind and run_id fields, and preserve the destination path and output structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.archon/scripts/_common.py:
- Around line 44-47: Update load_env to include non-empty variables from
os.environ even when their keys are absent from the parsed .env mapping, while
preserving existing .env values unless a process value should override them.
Ensure process-only values such as CARTESIA_VOICE_ID are available to callers
like tts-narrate.py.
In @.archon/scripts/compose-video.py:
- Around line 34-54: Validate that clips loaded by compose-video’s
read_json("clips.json") call are a non-empty list before the total/ideal segment
math and any len(clips) or division usage. Raise SystemExit with a clear message
identifying the missing, invalid, or empty clips manifest, while preserving the
existing selection logic for valid clips.
In @.archon/scripts/quality-check.py:
- Around line 55-57: In .archon/scripts/quality-check.py lines 55-57, update the
framerate calculation to treat a zero denominator as an unknown framerate
without dividing by zero, while preserving the existing check behavior. In lines
59-67, use the video stream duration when format.duration is absent; if neither
duration is available, raise SystemExit with a clear diagnostic message so
quality.json is still produced when possible.
- Around line 27-30: Update ffmpeg_stderr to check the CompletedProcess return
code after running ffmpeg and fail immediately on non-zero status, propagating
the analysis error instead of returning stderr for downstream parsing. Preserve
the existing stderr return behavior for successful executions.
In @.archon/workflows/video/README.md:
- Line 30: Update the three fenced code blocks in the README to specify the text
language identifier, including the blocks containing the error output, workflow
directory layout, and $STATE_DIR/content/ path. Use text for each fence while
preserving their contents.
In @.archon/workflows/video/social-short.yaml:
- Around line 35-52: Update the prompt in
.archon/workflows/video/social-short.yaml at lines 35-52 to require a sourced
fact bundle before making factual public-facing claims; when sources are absent,
restrict the script to clearly framed opinion rather than presenting claims as
facts. Update .archon/workflows/video/ugc-video.yaml at lines 38-65 to require
verified first-person source material and consent for testimonials, otherwise
generate a clearly disclosed dramatization, and do not require personal details
that the input does not provide.
In @.archon/workflows/video/video-quality-review.yaml:
- Around line 29-34: Update the target-discovery assignment in the video quality
review workflow so an expected non-zero status from ls does not trigger set -e
before the existing target validation guard runs. Preserve the current
newest-directory selection, then allow the [ -z "${target:-}" ] or [ ! -d
"$target" ] check to emit the friendly guidance and exit.
- Line 53: Update the kind extraction command in the pick-video workflow to pass
the metadata path through an environment variable rather than interpolating $t
inside Python source. Have the Python code read that variable before loading
meta.json, while preserving the existing unknown fallback for failures.
- Around line 166-180: Update the approval message in the approval gate to
display the resolved playbook filename, not the raw target kind, and include the
new_strategy_markdown content or a diff-style preview that apply-strategy will
write. Ensure the gate exposes the actual destination and full pending document
so approval does not rely solely on proposed_change or its summary.
- Around line 227-231: Update the thumbnail_frame persistence block to accept
only non-boolean integers within the valid 1-based range of frames actually
present, rejecting zero, negative values, and values beyond the available frame
count before writing copy.json. Use the workflow’s existing frame collection or
metadata to determine the upper bound, and preserve the current write behavior
for valid values.
- Around line 211-223: Update the review persistence flow around the strategy
write and reviews.jsonl append: retain the existing archive copy before
replacement, append the ledger row before changing the active playbook, and
write the playbook through a temporary file followed by an atomic rename. Ensure
interrupted writes cannot truncate the playbook and a crash leaves the review
recorded but unapplied.
- Around line 199-202: Update the kind resolution and validation in the review
workflow, including the gather logic, so a missing or unresolved meta.json kind
does not default to "unknown" when review.target is "kind". Fail the node before
reading or writing a playbook in that case, while preserving the common-target
path and existing behavior for valid kinds.
---
Nitpick comments:
In @.archon/scripts/compose-video.py:
- Around line 26-29: Update the random.choice call in pick_music with an inline
Ruff S311 suppression, including a brief reason that non-cryptographic
randomness is acceptable for selecting a music bed.
- Around line 101-109: Update _common.run to accept an optional cwd parameter
and pass it through to subprocess.run. In the video composition flow, remove the
os.chdir try/finally block and invoke run with cwd=wd while preserving the
existing output reporting.
In @.archon/workflows/video/video-render-block.yaml:
- Around line 68-81: Update the workflow around the `content/latest` symlink and
`content/library.jsonl` append to explicitly handle concurrent runs of different
video kinds: either document and preserve last-writer-wins semantics for
`latest` while ensuring ledger appends are concurrency-safe, or replace the
shared symlink with per-kind symlinks. Keep the existing `.partial` staging
protection for each run directory.
- Line 79: Replace the printf-based meta.json creation in the video render
workflow with JSON serialization in the existing Python heredoc that runs
afterward. Pass VIDEO_KIND and RUN_ID into that Python block, serialize them
with json.dumps as the kind and run_id fields, and preserve the destination path
and output structure.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c07ee6c-3640-4dbc-9f2e-d84628bb1766
📒 Files selected for processing (12)
.archon/scripts/_common.py.archon/scripts/build-captions.py.archon/scripts/compose-video.py.archon/scripts/fetch-clips.py.archon/scripts/quality-check.py.archon/scripts/tts-narrate.py.archon/workflows/video/README.md.archon/workflows/video/product-video.yaml.archon/workflows/video/social-short.yaml.archon/workflows/video/ugc-video.yaml.archon/workflows/video/video-quality-review.yaml.archon/workflows/video/video-render-block.yaml
| for k in list(env): | ||
| if os.environ.get(k): | ||
| env[k] = os.environ[k] | ||
| return env |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Load process-only environment variables.
load_env only copies process values for keys already present in .env. If Archon injects only CARTESIA_VOICE_ID, .archon/scripts/tts-narrate.py line 35 ignores it and uses DEFAULT_VOICE. require_key still finds injected API keys, so missing-key tests do not detect this configuration defect.
Proposed fix
- for k in list(env):
- if os.environ.get(k):
- env[k] = os.environ[k]
+ env.update(os.environ)
return env📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for k in list(env): | |
| if os.environ.get(k): | |
| env[k] = os.environ[k] | |
| return env | |
| env.update(os.environ) | |
| return env |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.archon/scripts/_common.py around lines 44 - 47, Update load_env to include
non-empty variables from os.environ even when their keys are absent from the
parsed .env mapping, while preserving existing .env values unless a process
value should override them. Ensure process-only values such as CARTESIA_VOICE_ID
are available to callers like tts-narrate.py.
| clips = read_json("clips.json") | ||
| words = read_json("words.json") | ||
|
|
||
| narration = wd / "narration.wav" | ||
| captions = wd / "captions.ass" | ||
| for required in (narration, captions): | ||
| if not required.exists(): | ||
| raise SystemExit(f"missing {required}") | ||
|
|
||
| total = float(words["duration"]) + TAIL_PADDING | ||
| ideal = max(1, round(total / SECONDS_PER_CLIP)) | ||
| if len(clips) >= ideal: | ||
| chosen = clips[:ideal] | ||
| elif total / len(clips) <= MAX_SECONDS_PER_CLIP: | ||
| # Slightly longer shots beat showing the same footage twice. | ||
| chosen = list(clips) | ||
| else: | ||
| # Genuinely not enough footage — cycle, and say so. | ||
| chosen = [clips[i % len(clips)] for i in range(ideal)] | ||
| log(f"WARN only {len(clips)} clips for {ideal} segments — footage will repeat") | ||
| seg = total / len(chosen) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the producer contract for clips.json and the read_json return type.
fd -t f 'fetch-clips.py' -x rg -n -C4 'clips\.json|write_json|"path"'
fd -t f '_common.py' -x rg -n -C3 'def read_json|def write_json|def run'Repository: coleam00/Archon
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,200p' | rg '(^|/)compose-video\.py$|(^|/)fetch-.*clips.*\.py$|(^|*)_common\.py$|clips\.json$|words\.json$' || true
echo "== compose-video outline/slices =="
compose="$(git ls-files | rg '(^|/)compose-video\.py$' | head -n1)"
if [ -n "${compose:-}" ]; then
wc -l "$compose"
cat -n "$compose" | sed -n '1,140p'
fi
echo "== fetch scripts matching clips =="
for f in $(git ls-files | rg 'fetch-.*clips.*\.py$|fetch-clips.*\.py$'); do
echo "--- $f"
wc -l "$f"
cat -n "$f" | sed -n '1,220p'
done
echo "== _common read_json/write_json snippets =="
for f in $(git ls-files | rg '(^|)_common\.py$'); do
echo "--- $f"
rg -n -C5 'read_json|write_json|def run' "$f" || true
doneRepository: coleam00/Archon
Length of output: 10753
Guard clips.json before segment math.
.archon/scripts/fetch-clips.py writes manifest directly, so the list shape is intended. But an absent, corrupted, or non-list/empty clips.json reaches .archon/scripts/compose-video.py:47 and either TypeError: len() of unsized object or ZeroDivisionError, with no clear cause. Add a validation guard and fail with an informative message before computing segments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.archon/scripts/compose-video.py around lines 34 - 54, Validate that clips
loaded by compose-video’s read_json("clips.json") call are a non-empty list
before the total/ideal segment math and any len(clips) or division usage. Raise
SystemExit with a clear message identifying the missing, invalid, or empty clips
manifest, while preserving the existing selection logic for valid clips.
Source: Linters/SAST tools
| def ffmpeg_stderr(args: list[str]) -> str: | ||
| return subprocess.run( | ||
| ["ffmpeg", "-hide_banner", *args], capture_output=True, text=True | ||
| ).stderr |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check the ffmpeg exit status in ffmpeg_stderr.
The function ignores the return code. If ffmpeg fails, it returns error text or an empty string. Two downstream effects follow:
- The
volumedetectregexes do not match,mean_dbbecomes-99.0, and QC reports a fatalaudio_leveldefect that does not exist. - The
blackdetectoutput is empty, sono_dead_framespasses even though the analysis never ran.
Fail on a non-zero exit so an analysis error is not reported as a video defect.
🛡️ Proposed fix
def ffmpeg_stderr(args: list[str]) -> str:
- return subprocess.run(
- ["ffmpeg", "-hide_banner", *args], capture_output=True, text=True
- ).stderr
+ proc = subprocess.run(
+ ["ffmpeg", "-hide_banner", *args], capture_output=True, text=True
+ )
+ if proc.returncode != 0:
+ raise SystemExit(f"ffmpeg analysis failed ({proc.returncode}): {proc.stderr.strip()[-500:]}")
+ return proc.stderr📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def ffmpeg_stderr(args: list[str]) -> str: | |
| return subprocess.run( | |
| ["ffmpeg", "-hide_banner", *args], capture_output=True, text=True | |
| ).stderr | |
| def ffmpeg_stderr(args: list[str]) -> str: | |
| proc = subprocess.run( | |
| ["ffmpeg", "-hide_banner", *args], capture_output=True, text=True | |
| ) | |
| if proc.returncode != 0: | |
| raise SystemExit( | |
| f"ffmpeg analysis failed ({proc.returncode}): " | |
| f"{proc.stderr.strip()[-500:]}" | |
| ) | |
| return proc.stderr |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 27-29: Command coming from incoming request
Context: subprocess.run(
["ffmpeg", "-hide_banner", *args], capture_output=True, text=True
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.1)
[error] 28-28: subprocess call: check for execution of untrusted input
(S603)
[error] 29-29: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.archon/scripts/quality-check.py around lines 27 - 30, Update ffmpeg_stderr
to check the CompletedProcess return code after running ffmpeg and fail
immediately on non-zero status, propagating the analysis error instead of
returning stderr for downstream parsing. Preserve the existing stderr return
behavior for successful executions.
| num, den = (vstream.get("r_frame_rate") or "0/1").split("/") | ||
| fps = float(num) / float(den or 1) | ||
| check("framerate", 29.0 <= fps <= 31.0, f"{fps:.2f} fps") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unguarded ffprobe field reads abort QC before quality.json exists. Both sites trust the ffprobe payload shape. A malformed or missing field raises an unhandled exception, so the run produces no diagnostic report and the operator sees only a traceback.
.archon/scripts/quality-check.py#L55-L57: treat a zero denominator inr_frame_rateas an unknown framerate instead of dividing by it..archon/scripts/quality-check.py#L59-L67: fall back to the video stream duration whenformat.durationis absent, or raiseSystemExitwith a clear message.
📍 Affects 1 file
.archon/scripts/quality-check.py#L55-L57(this comment).archon/scripts/quality-check.py#L59-L67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.archon/scripts/quality-check.py around lines 55 - 57, In
.archon/scripts/quality-check.py lines 55-57, update the framerate calculation
to treat a zero denominator as an unknown framerate without dividing by zero,
while preserving the existing check behavior. In lines 59-67, use the video
stream duration when format.duration is absent; if neither duration is
available, raise SystemExit with a clear diagnostic message so quality.json is
still produced when possible.
| per-project env vars, which take precedence). They are deliberately not in this | ||
| repo's `.env`, so an unconfigured run fails immediately and by name: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to the fenced code blocks.
markdownlint-cli2 reports MD040 for these three fences. Use text because each block shows output or a directory layout.
Proposed fix
-```
+```text
PEXELS_API_KEY is not set (add it to /path/to/repo/.env)- +text
read-strategy ─> gen-brief ─┬─> write-brief ──┐
-```
+```text
$STATE_DIR/content/
Also applies to: 51-51, 77-77
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 30-30: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.archon/workflows/video/README.md at line 30, Update the three fenced code
blocks in the README to specify the text language identifier, including the
blocks containing the error output, workflow directory layout, and
$STATE_DIR/content/ path. Use text for each fence while preserving their
contents.
Source: Linters/SAST tools
| # Assign bare: the engine already shell-quotes an output ref, so adding | ||
| # our own quotes around it puts literal quote characters in the value. | ||
| t=$pick-video.output | ||
| kind="$(python3 -c "import json,sys;print(json.load(open('$t/meta.json'))['kind'])" 2>/dev/null || echo unknown)" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not interpolate the path into the Python source string.
$t derives from $ARGUMENTS through pick-video. Line 53 embeds it inside a single-quoted Python literal. A run id that contains a single quote terminates the literal and the remaining text is executed as Python. Pass the path through the environment instead. This also removes the quoting hazard entirely.
🔒️ Proposed fix
- kind="$(python3 -c "import json,sys;print(json.load(open('$t/meta.json'))['kind'])" 2>/dev/null || echo unknown)"
+ kind="$(META="$t/meta.json" python3 -c "import json,os;print(json.load(open(os.environ['META']))['kind'])" 2>/dev/null || echo unknown)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kind="$(python3 -c "import json,sys;print(json.load(open('$t/meta.json'))['kind'])" 2>/dev/null || echo unknown)" | |
| kind="$(META="$t/meta.json" python3 -c "import json,os;print(json.load(open(os.environ['META']))['kind'])" 2>/dev/null || echo unknown)" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.archon/workflows/video/video-quality-review.yaml at line 53, Update the
kind extraction command in the pick-video workflow to pass the metadata path
through an environment variable rather than interpolating $t inside Python
source. Have the Python code read that variable before loading meta.json, while
preserving the existing unknown fallback for failures.
| approval: | ||
| message: | | ||
| Quality review of $pick-video.output | ||
|
|
||
| VERDICT: $review.output.verdict | ||
|
|
||
| Writing to: $review.output.target playbook | ||
|
|
||
| Proposed single change: | ||
| $review.output.proposed_change | ||
|
|
||
| Why: $review.output.rationale | ||
| Watch next: $review.output.what_to_watch_next | ||
|
|
||
| Approve to write this into the house playbook; reject to discard it. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The approval gate does not show what will be written.
apply-strategy overwrites the playbook with new_strategy_markdown (Line 211). The approval message does not include that field. It also shows target as the literal kind or common, not the resolved filename such as strategy-social-short.md. The human therefore approves a summary of the change, not the content and not the destination file. A reviewer that returns proposed_change: "none" but a rewritten document passes this gate unnoticed.
Include the resolved playbook name and the new content, or at least a diff-style preview, in the message.
🔒️ Proposed change to the gate message
Writing to: $review.output.target playbook
Proposed single change:
$review.output.proposed_change
Why: $review.output.rationale
Watch next: $review.output.what_to_watch_next
+ New playbook content to be written:
+ ---
+ $review.output.new_strategy_markdown
+ ---
+
Approve to write this into the house playbook; reject to discard it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.archon/workflows/video/video-quality-review.yaml around lines 166 - 180,
Update the approval message in the approval gate to display the resolved
playbook filename, not the raw target kind, and include the
new_strategy_markdown content or a diff-style preview that apply-strategy will
write. Ensure the gate exposes the actual destination and full pending document
so approval does not rely solely on proposed_change or its summary.
| kind = json.loads(meta_path.read_text())["kind"] if meta_path.exists() else "unknown" | ||
| # Write back to exactly one playbook — the one the reviewer targeted. | ||
| target = review.get("target", "kind") | ||
| strategy = state / ("strategy-common.md" if target == "common" else f"strategy-{kind}.md") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
An unresolved kind writes an orphan playbook.
If meta.json is missing, Line 199 sets kind to "unknown". Line 202 then resolves the destination to strategy-unknown.md. No producer reads that file: social-short.yaml reads strategy-social-short.md, and the other producers follow the same pattern. The approved lesson is written to disk and never applied. The run reports success.
The same fallback exists in gather at Line 53, so the reviewer also judges against the wrong playbook.
Fail the node when kind cannot be resolved and target is kind.
🐛 Proposed fix
target = review.get("target", "kind")
+ if target != "common" and kind == "unknown":
+ raise SystemExit(
+ f"cannot resolve video kind from {meta_path}; refusing to write strategy-unknown.md"
+ )
strategy = state / ("strategy-common.md" if target == "common" else f"strategy-{kind}.md")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kind = json.loads(meta_path.read_text())["kind"] if meta_path.exists() else "unknown" | |
| # Write back to exactly one playbook — the one the reviewer targeted. | |
| target = review.get("target", "kind") | |
| strategy = state / ("strategy-common.md" if target == "common" else f"strategy-{kind}.md") | |
| kind = json.loads(meta_path.read_text())["kind"] if meta_path.exists() else "unknown" | |
| # Write back to exactly one playbook — the one the reviewer targeted. | |
| target = review.get("target", "kind") | |
| if target != "common" and kind == "unknown": | |
| raise SystemExit( | |
| f"cannot resolve video kind from {meta_path}; refusing to write strategy-unknown.md" | |
| ) | |
| strategy = state / ("strategy-common.md" if target == "common" else f"strategy-{kind}.md") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.archon/workflows/video/video-quality-review.yaml around lines 199 - 202,
Update the kind resolution and validation in the review workflow, including the
gather logic, so a missing or unresolved meta.json kind does not default to
"unknown" when review.target is "kind". Fail the node before reading or writing
a playbook in that case, while preserving the common-target path and existing
behavior for valid kinds.
| strategy.write_text(review["new_strategy_markdown"].rstrip() + "\n") | ||
| log = state / "reviews.jsonl" | ||
| with log.open("a") as fh: | ||
| fh.write(json.dumps({ | ||
| "reviewed_at": datetime.datetime.now().astimezone().isoformat(timespec="seconds"), | ||
| "review_run_id": os.environ["RUN_ID"], | ||
| "video_run_id": pathlib.Path(os.environ["VIDEO_DIR"]).name, | ||
| "kind": kind, | ||
| "playbook": strategy.name, | ||
| "verdict": review["verdict"], | ||
| "change": review["proposed_change"], | ||
| "watch": review["what_to_watch_next"], | ||
| }) + "\n") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A failure between the playbook write and the ledger append allows a double review.
Line 211 overwrites the playbook. Lines 212-223 then append to reviews.jsonl. The guard at Lines 38-43 uses reviews.jsonl to refuse a second review of the same video. If the process stops after Line 211 and before the append completes, the playbook already carries the change but the video is not recorded as reviewed. The next run picks the same video and stacks a second change on top. This is the exact outcome the guard exists to prevent.
write_text is also non-atomic. An interrupted write truncates the playbook. The store node in video-render-block.yaml stages into a .partial directory for this reason.
Write the playbook atomically through a temporary file and rename, and append the ledger row before the playbook write so a crash produces a recorded-but-unapplied review rather than an applied-but-unrecorded one.
🛡️ Proposed fix
- strategy.write_text(review["new_strategy_markdown"].rstrip() + "\n")
log = state / "reviews.jsonl"
with log.open("a") as fh:
fh.write(json.dumps({
...
}) + "\n")
+ fh.flush()
+ os.fsync(fh.fileno())
+ tmp = strategy.with_suffix(strategy.suffix + ".partial")
+ tmp.write_text(review["new_strategy_markdown"].rstrip() + "\n")
+ tmp.replace(strategy)Keep the archive copy at Lines 203-210 before the replace.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| strategy.write_text(review["new_strategy_markdown"].rstrip() + "\n") | |
| log = state / "reviews.jsonl" | |
| with log.open("a") as fh: | |
| fh.write(json.dumps({ | |
| "reviewed_at": datetime.datetime.now().astimezone().isoformat(timespec="seconds"), | |
| "review_run_id": os.environ["RUN_ID"], | |
| "video_run_id": pathlib.Path(os.environ["VIDEO_DIR"]).name, | |
| "kind": kind, | |
| "playbook": strategy.name, | |
| "verdict": review["verdict"], | |
| "change": review["proposed_change"], | |
| "watch": review["what_to_watch_next"], | |
| }) + "\n") | |
| log = state / "reviews.jsonl" | |
| with log.open("a") as fh: | |
| fh.write(json.dumps({ | |
| "reviewed_at": datetime.datetime.now().astimezone().isoformat(timespec="seconds"), | |
| "review_run_id": os.environ["RUN_ID"], | |
| "video_run_id": pathlib.Path(os.environ["VIDEO_DIR"]).name, | |
| "kind": kind, | |
| "playbook": strategy.name, | |
| "verdict": review["verdict"], | |
| "change": review["proposed_change"], | |
| "watch": review["what_to_watch_next"], | |
| }) + "\n") | |
| fh.flush() | |
| os.fsync(fh.fileno()) | |
| tmp = strategy.with_suffix(strategy.suffix + ".partial") | |
| tmp.write_text(review["new_strategy_markdown"].rstrip() + "\n") | |
| tmp.replace(strategy) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.archon/workflows/video/video-quality-review.yaml around lines 211 - 223,
Update the review persistence flow around the strategy write and reviews.jsonl
append: retain the existing archive copy before replacement, append the ledger
row before changing the active playbook, and write the playbook through a
temporary file followed by an atomic rename. Ensure interrupted writes cannot
truncate the playbook and a crash leaves the review recorded but unapplied.
| if cp.exists() and isinstance(review.get("thumbnail_frame"), int): | ||
| data = json.loads(cp.read_text()) | ||
| data["thumbnail_frame"] = review["thumbnail_frame"] | ||
| cp.write_text(json.dumps(data, indent=2)) | ||
| print(f"thumbnail_frame={review['thumbnail_frame']} -> {cp}") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Validate the thumbnail_frame range before writing it into copy.json.
Line 227 checks the type only. The schema at Lines 155-156 sets no bounds, and the prompt at Lines 106-107 asks for a 1-based index. The model can return 0, a negative value, or an index past the last frame. The value is then persisted into copy.json and any consumer that indexes the frame list fails or selects the wrong frame.
isinstance(x, int) also accepts True, because bool subclasses int.
Bound the value against the frames actually present.
🐛 Proposed fix
cp = vid / "copy.json"
- if cp.exists() and isinstance(review.get("thumbnail_frame"), int):
+ frames = sorted((vid / "frames").glob("*.jpg"))
+ idx = review.get("thumbnail_frame")
+ if cp.exists() and type(idx) is int and 1 <= idx <= len(frames):
data = json.loads(cp.read_text())
- data["thumbnail_frame"] = review["thumbnail_frame"]
- cp.write_text(json.dumps(data, indent=2))
- print(f"thumbnail_frame={review['thumbnail_frame']} -> {cp}")
+ data["thumbnail_frame"] = idx
+ cp.write_text(json.dumps(data, indent=2) + "\n")
+ print(f"thumbnail_frame={idx} -> {cp}")
+ elif idx is not None:
+ print(f"ignoring out-of-range thumbnail_frame={idx!r} ({len(frames)} frames)")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if cp.exists() and isinstance(review.get("thumbnail_frame"), int): | |
| data = json.loads(cp.read_text()) | |
| data["thumbnail_frame"] = review["thumbnail_frame"] | |
| cp.write_text(json.dumps(data, indent=2)) | |
| print(f"thumbnail_frame={review['thumbnail_frame']} -> {cp}") | |
| frames = sorted((vid / "frames").glob("*.jpg")) | |
| idx = review.get("thumbnail_frame") | |
| if cp.exists() and type(idx) is int and 1 <= idx <= len(frames): | |
| data = json.loads(cp.read_text()) | |
| data["thumbnail_frame"] = idx | |
| cp.write_text(json.dumps(data, indent=2) + "\n") | |
| print(f"thumbnail_frame={idx} -> {cp}") | |
| elif idx is not None: | |
| print(f"ignoring out-of-range thumbnail_frame={idx!r} ({len(frames)} frames)") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.archon/workflows/video/video-quality-review.yaml around lines 227 - 231,
Update the thumbnail_frame persistence block to accept only non-boolean integers
within the valid 1-based range of frames actually present, rejecting zero,
negative values, and values beyond the available frame count before writing
copy.json. Use the workflow’s existing frame collection or metadata to determine
the upper bound, and preserve the current write behavior for valid values.
Wirasm
added
the
needs rework
|
not ready, needs rework will go in as workflows/video-pack |
1 participant
Summary
.archon/workflows/video/(three producer workflows:social-short,ugc-video,product-video; a human-gatedvideo-quality-review; a sharedvideo-render-blockinclude) and fiveuv-runtime Python scripts under.archon/scripts/(Pexels clip fetch, Cartesia TTS with word timestamps, ASS karaoke captions, ffmpeg compose, quality gate) plus_common.py.packages/changes, no bundled defaults (workflows/defaults/untouched, nogenerate:bundledneeded), no schema, no API. Publishing/posting is explicitly out of scope; runs stop atstore.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory (list every module-to-module edge, mark changes):
include:expansion; block readsbrief.json/copy.jsonfrom$ARTIFACTS_DIR, no node refs into the parentscript:nodes,runtime: uv, explicittimeout:.envor Archon-managed env vars; fail fast by name when unsetLabel Snapshot
risk: lowsize: Lworkflowsworkflows:examplesChange Metadata
featureworkflowsLinked Issue
WORKFLOW_IDas a real env var; these workflows export it manually until then)Validation Evidence (required)
Commands and result summary:
bun run validate # full suite — passed (type-check, lint, format, tests, bundled/schema/capability checks)bun run validatepass on this branch; the diff touches no TypeScript, so the suite guards against accidental engine/bundle drift. Workflows load through normal discovery (archon workflow list).Security Impact (required)
No)Yes)No)No)Yes, describe risk and mitigation: scripts call Pexels (stock footage) and Cartesia (TTS) with user-supplied API keys read from.env/managed env vars. Keys are never logged or written to artifacts; an unset key fails immediately by name. Both calls happen only inside runs of these opt-in workflows.Compatibility / Migration
Yes)Yes— two new optional env vars,PEXELS_API_KEYandCARTESIA_API_KEY(+ optionalCARTESIA_VOICE_ID), needed only to run this pack;ffmpeganduvon PATH)No).env(or Archon per-project env vars) before first run.Human Verification (required)
What was personally validated beyond CI:
video-quality-reviewrun against a finished video including the approval gate and a per-kind playbook write.storestaging via<run-id>.partialso an interrupted run can't leave a half-populated library entry; tier/provider interaction warnings (README "Authoring gotchas").provider: claude; other providers fail loudly there), Cartesia voices other than the default.Side Effects / Blast Radius (required)
.archon/; engine, defaults bundle, and all existing workflows untouched.store(no publishing surface exists); all output lands under$STATE_DIR/$ARTIFACTS_DIR, never in the repo.Rollback Plan (required)
git revert 58196a5f) — no state, schema, or generated files to unwind.Risks and Mitigations
store.target: kind | commonand writes exactly one file per run; human approval gates every playbook change.Summary by CodeRabbit
New Features
Documentation