feat(docs): animated architecture overview video for contributors#2365
feat(docs): animated architecture overview video for contributors#2365ilblackdragon wants to merge 2 commits intostagingfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a Remotion-based documentation video project (animated architecture overview) plus supporting tooling so contributors can preview and re-render the video from the repo.
Changes:
- Introduces
docs/architecture-video/Remotion project with 12 animated scenes and shared styling/code-block components. - Adds a root-level render helper script for one-command MP4 generation.
- Adds a Claude skill doc (
.claude/skills/architecture-video/) describing how to maintain/regenerate the video as architecture evolves.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/render-architecture-video.sh | One-command render script that installs deps (if needed) and runs remotion render. |
| docs/architecture-video/tsconfig.json | TypeScript configuration for the Remotion project. |
| docs/architecture-video/src/theme.ts | Shared color palette + font constants for all scenes. |
| docs/architecture-video/src/scenes/TitleScene.tsx | Intro/title animation scene. |
| docs/architecture-video/src/scenes/PrimitivesScene.tsx | Scene describing the engine v2 primitives. |
| docs/architecture-video/src/scenes/ExecutionLoopScene.tsx | Scene visualizing ExecutionLoop::run() steps. |
| docs/architecture-video/src/scenes/CodeActScene.tsx | Scene illustrating CodeAct/Monty flow and host functions. |
| docs/architecture-video/src/scenes/ThreadStateScene.tsx | Thread state machine diagram scene (SVG + animated edges). |
| docs/architecture-video/src/scenes/SkillsPipelineScene.tsx | Scene explaining skill selection pipeline stages. |
| docs/architecture-video/src/scenes/ToolDispatchScene.tsx | Scene explaining the ToolDispatcher dispatch pipeline. |
| docs/architecture-video/src/scenes/ChannelsRoutingScene.tsx | Scene describing Channel trait + stream merging + routing metadata. |
| docs/architecture-video/src/scenes/ChannelImplsScene.tsx | Scene listing channel implementations and IO characteristics. |
| docs/architecture-video/src/scenes/TraitsScene.tsx | Scene listing key extensibility traits and implementers. |
| docs/architecture-video/src/scenes/LlmDecoratorScene.tsx | Scene describing the LLM provider decorator chain. |
| docs/architecture-video/src/scenes/OutroScene.tsx | Outro scene with contribution steps. |
| docs/architecture-video/src/Root.tsx | Remotion root composition registration. |
| docs/architecture-video/src/IronClawArchitecture.tsx | Scene sequencing, durations, and transitions; total duration calc. |
| docs/architecture-video/src/index.ts | Remotion registerRoot entry point. |
| docs/architecture-video/src/index.css | Tailwind import for styling baseline. |
| docs/architecture-video/src/components/Code.tsx | Minimal syntax highlighting + CodeBlock component used by scenes. |
| docs/architecture-video/remotion.config.ts | Remotion CLI configuration (image format + overwrite). |
| docs/architecture-video/README.md | Remotion project README (commands/help). |
| docs/architecture-video/package.json | Remotion/React/TS tooling dependencies and scripts. |
| docs/architecture-video/package-lock.json | Dependency lockfile for reproducible installs. |
| docs/architecture-video/eslint.config.mjs | Remotion-provided ESLint flat config. |
| docs/architecture-video/.prettierrc | Prettier settings for the video project. |
| docs/architecture-video/.gitignore | Ignores node artifacts and render outputs for the video project. |
| .claude/skills/architecture-video/SKILL.md | Internal skill guide describing how to update/regenerate the video. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" | ||
| VIDEO_DIR="$PROJECT_ROOT/docs/architecture-video" | ||
| OUTPUT="${1:-$PROJECT_ROOT/ironclaw-architecture.mp4}" |
There was a problem hiding this comment.
If the caller passes a relative output path (e.g. ./scripts/render-architecture-video.sh output.mp4), OUTPUT will be interpreted relative to docs/architecture-video because the render runs after cd "$VIDEO_DIR". Consider normalizing OUTPUT to an absolute path (or to $PROJECT_ROOT) before the cd so the output location is predictable.
| OUTPUT="${1:-$PROJECT_ROOT/ironclaw-architecture.mp4}" | |
| OUTPUT="${1:-$PROJECT_ROOT/ironclaw-architecture.mp4}" | |
| case "$OUTPUT" in | |
| /*) ;; | |
| *) OUTPUT="$PROJECT_ROOT/${OUTPUT#./}" ;; | |
| esac |
There was a problem hiding this comment.
Fixed in ba472bf — OUTPUT is now normalized to an absolute path before the cd to $VIDEO_DIR.
scripts/render-architecture-video.sh
Outdated
|
|
||
| if [ ! -d "$VIDEO_DIR/node_modules" ]; then | ||
| echo "Installing dependencies..." | ||
| (cd "$VIDEO_DIR" && npm install --no-fund --no-audit) |
There was a problem hiding this comment.
This script uses npm install even though a package-lock.json is checked in (in docs/architecture-video/). To make renders reproducible and faster in CI/local, prefer npm ci when the lockfile exists (and fall back to npm install only if needed).
| (cd "$VIDEO_DIR" && npm install --no-fund --no-audit) | |
| if [ -f "$VIDEO_DIR/package-lock.json" ]; then | |
| (cd "$VIDEO_DIR" && npm ci --no-fund --no-audit) | |
| else | |
| (cd "$VIDEO_DIR" && npm install --no-fund --no-audit) | |
| fi |
There was a problem hiding this comment.
Fixed in ba472bf — now uses npm ci when package-lock.json exists, falls back to npm install otherwise.
| }, | ||
| { | ||
| name: "EmbeddingProvider", | ||
| file: "workspace/embeddings.rs", |
There was a problem hiding this comment.
The EmbeddingProvider entry points to workspace/embeddings.rs, but the file in this repo is src/workspace/embeddings.rs. Since this video is aimed at contributors, it would be less confusing to update the displayed path to the real one.
| file: "workspace/embeddings.rs", | |
| file: "src/workspace/embeddings.rs", |
There was a problem hiding this comment.
Fixed in ba472bf — updated to src/workspace/embeddings.rs.
| }, | ||
| { | ||
| name: "NetworkPolicyDecider", | ||
| file: "sandbox/proxy/policy.rs", |
There was a problem hiding this comment.
The NetworkPolicyDecider entry points to sandbox/proxy/policy.rs, but the real path is src/sandbox/proxy/policy.rs. Please update the displayed path so contributors can jump to the correct file.
| file: "sandbox/proxy/policy.rs", | |
| file: "src/sandbox/proxy/policy.rs", |
There was a problem hiding this comment.
Fixed in ba472bf — updated to src/sandbox/proxy/policy.rs.
| const CHANNELS = [ | ||
| { | ||
| icon: "⌨", | ||
| name: "REPL", | ||
| file: "channels/repl.rs", | ||
| input: "stdin via rustyline", | ||
| output: "stdout + termimad markdown", |
There was a problem hiding this comment.
The file paths in this scene omit the src/ prefix (e.g. channels/repl.rs), and the TUI entry points at channels/cli/ even though the repo uses src/channels/tui.rs (and a separate crates/ironclaw_tui crate). Consider updating these strings to match the actual paths to avoid sending contributors on a file hunt.
There was a problem hiding this comment.
Fixed in ba472bf — all 6 channel file paths now include the src/ prefix.
| const TRAIT_CODE = `pub trait Channel: Send + Sync { | ||
| fn name(&self) -> &str; | ||
| async fn start(&self) -> Result<MessageStream>; | ||
| async fn respond(&self, msg: &IncomingMessage, | ||
| response: OutgoingResponse); | ||
| async fn send_status(&self, s: StatusUpdate, meta: &Value); | ||
| async fn broadcast(&self, user_id: &str, | ||
| response: OutgoingResponse); | ||
| async fn health_check(&self); | ||
| fn conversation_context(&self, meta: &Value) | ||
| -> HashMap<String, String>; | ||
| async fn shutdown(&self); | ||
| }`; |
There was a problem hiding this comment.
This code block is presented as the Channel trait, but the signatures don’t match the actual trait in src/channels/channel.rs (e.g. methods return Result<_, ChannelError>, some have default impls, and send_status/broadcast signatures differ). Either make the snippet match the real trait or explicitly label it as simplified pseudocode so new contributors don’t copy the wrong API shape.
There was a problem hiding this comment.
Fixed in ba472bf — added a // Simplified label at the top of the code block, and updated signatures to include Result<> return types to better match the real trait. Full accuracy isn't the goal for a visual overview, but the shapes are now closer.
| prompts/codeact_preamble.md • | ||
| executor/orchestrator.rs |
There was a problem hiding this comment.
The heading references prompts/codeact_preamble.md and executor/orchestrator.rs, but in this repo these live under crates/ironclaw_engine/prompts/codeact_preamble.md and crates/ironclaw_engine/src/executor/orchestrator.rs. Updating the displayed paths would make it easier for contributors to locate the referenced files.
| prompts/codeact_preamble.md • | |
| executor/orchestrator.rs | |
| crates/ironclaw_engine/prompts/codeact_preamble.md • | |
| crates/ironclaw_engine/src/executor/orchestrator.rs |
There was a problem hiding this comment.
Fixed in ba472bf — updated to crates/ironclaw_engine/prompts/codeact_preamble.md and crates/ironclaw_engine/src/executor/orchestrator.rs.
docs/architecture-video/package.json
Outdated
| "version": "1.0.0", | ||
| "description": "My Remotion video", | ||
| "repository": {}, | ||
| "license": "UNLICENSED", |
There was a problem hiding this comment.
license is set to UNLICENSED, but this repository is dual-licensed (MIT/Apache-2.0 at the root). Using UNLICENSED here can confuse license scanners and downstream redistribution; consider aligning it with the repo’s license (e.g. MIT OR Apache-2.0).
| "license": "UNLICENSED", | |
| "license": "MIT OR Apache-2.0", |
There was a problem hiding this comment.
Fixed in ba472bf — changed to MIT OR Apache-2.0 to match the repo license.
There was a problem hiding this comment.
Code Review
This pull request introduces a Remotion-based project for generating an animated architecture overview video, featuring twelve React scenes, a custom syntax highlighter, and a shell script for rendering. The feedback identifies a path resolution issue in the rendering script, a typo in the TypeScript version, and missing library definitions in the TypeScript configuration. Additionally, a logic error in the video duration calculation was highlighted to ensure accurate rendering when transitions are omitted.
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" | ||
| VIDEO_DIR="$PROJECT_ROOT/docs/architecture-video" | ||
| OUTPUT="${1:-$PROJECT_ROOT/ironclaw-architecture.mp4}" |
There was a problem hiding this comment.
The script does not resolve relative output paths before changing the working directory to $VIDEO_DIR. If a user provides a relative path as an argument (e.g., ./output.mp4), the video will be rendered inside the docs/architecture-video/ directory instead of the user's current working directory. It is safer to resolve the output path to an absolute one before the cd command.
| OUTPUT="${1:-$PROJECT_ROOT/ironclaw-architecture.mp4}" | |
| OUTPUT="${1:-$PROJECT_ROOT/ironclaw-architecture.mp4}" | |
| if [[ "$OUTPUT" != /* ]]; then | |
| OUTPUT="$(pwd)/$OUTPUT" | |
| fi |
There was a problem hiding this comment.
Fixed in ba472bf — same fix as above, OUTPUT resolved to absolute path before cd.
docs/architecture-video/package.json
Outdated
| "@types/web": "0.0.166", | ||
| "eslint": "9.19.0", | ||
| "prettier": "3.8.1", | ||
| "typescript": "5.9.3" |
There was a problem hiding this comment.
Fixed in ba472bf — changed to 5.7.3 and regenerated package-lock.json.
| "jsx": "react-jsx", | ||
| "strict": true, | ||
| "noEmit": true, | ||
| "lib": ["es2015"], |
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed in ba472bf — updated lib to ["dom", "dom.iterable", "esnext"].
| export const TOTAL_DURATION = | ||
| SCENES.reduce((acc, sc) => acc + sc.dur, 0) - | ||
| (SCENES.length - 1) * TRANSITION_DUR; |
There was a problem hiding this comment.
The TOTAL_DURATION calculation assumes that every scene except the last one has a transition. However, the rendering logic below only applies a transition if sc.transition is truthy. If a scene is added to the SCENES array without a transition (as suggested by the guide in SKILL.md), the calculated duration will be longer than the actual video duration, leading to trailing empty frames. It is better to calculate the duration based on the actual presence of transitions.
export const TOTAL_DURATION =
SCENES.reduce((acc, sc, i) => {
const hasTransition = i < SCENES.length - 1 && sc.transition;
return acc + sc.dur - (hasTransition ? TRANSITION_DUR : 0);
}, 0);
There was a problem hiding this comment.
Fixed in ba472bf — TOTAL_DURATION now counts only scenes that have a transition property set, instead of assuming all scenes except the last one have transitions.
|
Merged Local regression check passed:
All prior review comments from Copilot and Gemini were already addressed in ba472bf. Ready for re-review. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 61 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Resolve the effective user_id for a settings operation. | ||
| /// | ||
| /// When `scope=admin`, the operation targets the shared admin-default scope | ||
| /// (`__admin__`). Only admin users may use this scope; non-admins get 403. | ||
| /// Without the scope parameter (or any other value), operations target the | ||
| /// calling user's own settings. | ||
| fn resolve_settings_scope( | ||
| user: &crate::channels::web::auth::UserIdentity, | ||
| query: &SettingScopeQuery, | ||
| ) -> Result<String, StatusCode> { | ||
| if query.scope.as_deref() == Some("admin") { | ||
| if user.role != "admin" { | ||
| tracing::warn!( | ||
| user_id = %user.user_id, | ||
| role = %user.role, | ||
| "Non-admin attempted to use scope=admin on settings endpoint" | ||
| ); | ||
| return Err(StatusCode::FORBIDDEN); | ||
| } | ||
| Ok(crate::tools::permissions::ADMIN_SETTINGS_USER_ID.to_string()) | ||
| } else { | ||
| Ok(user.user_id.clone()) | ||
| } | ||
| } |
There was a problem hiding this comment.
SettingScopeQuery introduces ?scope=admin semantics for settings endpoints, but settings_list_handler (GET /api/settings) does not accept/query this parameter and always lists the caller’s per-user settings. This makes it impossible for admins to list the admin-default scope via the API/UI while get/set/delete support it. Consider adding Query(query): Query<SettingScopeQuery> to the list handler and applying resolve_settings_scope() so /api/settings?scope=admin returns the admin-default settings (and keeps masking/secret-annotation behavior consistent).
| [package] | ||
| name = "ironclaw" | ||
| version = "0.25.0" | ||
| edition = "2024" | ||
| rust-version = "1.92" | ||
| description = "Secure personal AI assistant that protects your data and expands its capabilities on the fly" | ||
| authors = ["NEAR AI <support@near.ai>"] | ||
| license = "MIT OR Apache-2.0" | ||
| publish = false | ||
| homepage = "https://github.com/nearai/ironclaw" | ||
| repository = "https://github.com/nearai/ironclaw" |
There was a problem hiding this comment.
The PR title/description are scoped to adding a Remotion architecture video + render script, but this change set also bumps the workspace version to 0.25.0 and includes substantial runtime/security/LLM/config changes across Rust, JS, and WASM channel code. Please either update the PR title/description to reflect the full scope (and link the motivating issues), or split the release/version + behavior changes into separate PR(s) to keep review risk manageable.
henrypark133
left a comment
There was a problem hiding this comment.
Review: Animated architecture overview video (Risk: Medium)
The video itself (Remotion React project under docs/architecture-video/) looks like a great contributor resource. However, this PR has structural issues that need to be resolved before review of the actual content is meaningful.
Concerning: PR bundles ~9 unrelated commits with the video feature
This PR has 13 commits across 61 files, but most are unrelated merged features/fixes:
- fix(gateway): scope chat approvals to active thread (#2267)
- Fix paired Telegram owner scope (#2258)
- fix(engine): always append ActionResult (#2322)
- feat(engine): LLM council via per-call model override (#2320)
- feat(config): default CLI_MODE to TUI
- ci: build docker image in release process (#2321)
- fix: re-apply Telegram UTF-16 splitting
- feat: user-facing temperature setting (#2275)
- chore: sync staging and main (#2337)
- fix: resolve cargo-deny failures
- fix web chat refresh active thread (#2330)
Only commits 10 and 12 are the actual video feature.
Concerning: CI failures
5 jobs failing: E2E (features), E2E (routines), E2E Tests rollup, Run Tests rollup, Version Bump Check. These are likely caused by the bundled unrelated commits creating conflicts.
Concerning: Base branch is main not staging
This repo's workflow goes through staging first. Targeting main directly is unusual.
Suggested fix: Rebase this branch to contain only the video-specific commits (10 and 12), target staging, and resolve CI failures. The video content itself is low-risk (self-contained React/TypeScript project under docs/) and should be easy to approve once isolated.
Adds a Remotion-based animated video (82s, 12 scenes at 30fps) that visualizes the IronClaw architecture for new contributors. Covers engine v2 primitives, CodeAct execution, thread state machine, skills pipeline, tool dispatch, channel routing, trait implementations, and LLM decorator chain. - docs/architecture-video/ — Remotion project with 12 animated scenes - scripts/render-architecture-video.sh — render script - .claude/skills/architecture-video/ — Claude Code skill to update the video when architecture changes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Resolve relative output paths in render script before cd - Use npm ci when package-lock.json exists for reproducible builds - Fix file paths in TraitsScene, ChannelImplsScene, CodeActScene - Label Channel trait code as simplified in ChannelsRoutingScene - Fix TypeScript version (5.9.3 → 5.7.3) and update lockfile - Add dom/esnext to tsconfig lib for React 19 compatibility - Fix license to MIT OR Apache-2.0 to match repo - Fix TOTAL_DURATION to count only scenes with transitions - Fix cargo-deny: add publish = false, ignore RUSTSEC-2026-0097 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
c0957a0 to
0a8d93c
Compare
| description = "Secure personal AI assistant that protects your data and expands its capabilities on the fly" | ||
| authors = ["NEAR AI <support@near.ai>"] | ||
| license = "MIT OR Apache-2.0" | ||
| publish = false |
There was a problem hiding this comment.
High Severity — Unrelated build policy change hidden in docs PR
The publish = false addition to Cargo.toml is unrelated to the stated purpose of this PR ("animated architecture overview video"). This silently prevents the crate from being published to crates.io. Whether intentional or not, it should not be bundled into a "docs" PR — it's a build/release policy change that deserves its own PR with explicit team review.
Suggested fix: Remove this line from this PR. If publish = false is desired, submit it as a separate, clearly-titled PR (e.g., chore: prevent accidental crates.io publish).
| "license": "MIT OR Apache-2.0", | ||
| "private": true, | ||
| "dependencies": { | ||
| "@remotion/cli": "4.0.447", |
There was a problem hiding this comment.
Medium Severity — Remotion licensing not addressed
Remotion has a custom license requiring a company license for organizations with 3+ employees making revenue. The PR introduces Remotion as a dependency without any licensing discussion.
Suggested fix: Add a note in the PR description confirming Remotion licensing has been reviewed and is either covered by the free tier or a license has been obtained. Alternatively, consider a fully open-source alternative.
| @@ -0,0 +1,4610 @@ | |||
| { | |||
There was a problem hiding this comment.
Medium Severity — 378 npm packages added with --no-audit suppressing vulnerability scanner
The lock file adds 378 npm packages. The --no-audit flag in the render script suppresses npm's vulnerability scanner. This is a large attack surface added to a Rust project.
Suggested fix: (1) Remove --no-audit from scripts/render-architecture-video.sh. (2) Consider running npm audit in CI for this sub-project. (3) Document that this is an optional docs tool, not required for building IronClaw.
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" | ||
| VIDEO_DIR="$PROJECT_ROOT/docs/architecture-video" | ||
| OUTPUT="${1:-$PROJECT_ROOT/ironclaw-architecture.mp4}" |
There was a problem hiding this comment.
Medium Severity — Argument injection in render script
The $OUTPUT variable is derived from $1 and passed to npx remotion render. A user passing --some-flag as $1 would have it interpreted as a flag to remotion render, not a filename (argument injection).
Suggested fix: Add -- before "$OUTPUT" in the npx command, or validate that $1 doesn't start with -.
| {!isLast && presentation && ( | ||
| <TransitionSeries.Transition | ||
| presentation={presentation} | ||
| timing={linearTiming({ durationInFrames: TRANSITION_DUR })} |
There was a problem hiding this comment.
Medium Severity — Fragile duration calculation
TOTAL_DURATION is computed as sum(durations) - count(scenes_with_transition) * TRANSITION_DUR. This assumes transitions only exist between non-last scenes AND that a scene has a transition property iff it actually gets a rendered transition. Currently this works by coincidence (Outro is both last and the only scene without transition).
If someone reorders scenes or adds a scene with transition at the end, the duration calculation silently breaks and the video will be truncated or padded.
Suggested fix: Change to: SCENES.reduce((acc, sc) => acc + sc.dur, 0) - (SCENES.length - 1) * TRANSITION_DUR or explicitly count rendered transitions. Add a comment explaining the coupling.
Summary
.claude/skills/architecture-video/) so the video can be regenerated when architecture changesscripts/render-architecture-video.sh) for one-command video generationScenes
ExecutionLoop::run()pipelinedispatch()pipelinestream::select_allmergingUsage
Test plan
npx tsc --noEmitpasses with zero errors🤖 Generated with Claude Code