Skip to content

feat(artifacts): add shared planning artifacts - #109

Merged
tyler-barton-horizon merged 8 commits into
j5/mainfrom
t3code/repo-review
Sep 11, 2026
Merged

tyler-barton-horizon merged 8 commits into
j5/mainfrom
t3code/repo-review

Conversation

@tyler-barton-horizon

@tyler-barton-horizon tyler-barton-horizon commented Sep 5, 2026 •

Copy link
Copy Markdown
Collaborator

What Changed

  • Stores project artifacts in server-owned application data outside the repository, partitioned by project rather than thread.
  • Exports completed structured plans to the logical path artifacts/plan.md.
  • Gives every provider-scoped agent list_artifacts, read_artifact, and write_artifact tools, including read access in Claude read-only sessions.
  • Adds authenticated, bounded list/read endpoints and a project-scoped filesystem watcher so open artifact lists and previews refresh without navigation.
  • Adds Artifacts to both the sidebar and the thread right panel, with Markdown, text, image, and sandboxed HTML previews.
  • Styles logical artifacts/... references in chat as distinct Artifact chips and opens them directly in the Artifacts pane.
  • Documents the durable planning-output criteria and storage behavior. Mobile support is intentionally deferred from this focused first pass.

Why

Planning output was easy to lose in chat history or scatter around a repository. Thread-scoped storage also prevented agents from sharing it, while a repository-local directory required Git exclusion and could collide with tracked files. Server-owned project storage makes artifacts durable across threads and agents without touching repository contents or Git configuration.

UI Changes

Before, generated planning documents had no dedicated navigation or preview surface. After, Artifacts can be opened from the sidebar or alongside a thread in the right panel; artifact references in chat are visually distinct and open the selected document there; selected previews update as files change; and HTML renders in a transparent empty-sandbox iframe.

Integrated verification was completed against the desktop-backed web client for the full-page artifacts flow.

Verification

  • Focused server and web test suites passed
  • Targeted lint passed with pre-existing advisory warnings only
  • Contracts, server, and web typechecks passed
  • Desktop-backed UI pass covered project selection, file listing, and sandboxed HTML rendering

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • No animation or timing behavior requires a video

Built with GPT-5.6 Sol in the Codex harness through T3 Code.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XXL 1,000+ effective changed lines (test files excluded in mixed PRs). labels Sep 5, 2026
@tyler-barton-horizon tyler-barton-horizon changed the title feat(artifacts): add shared planning workspace feat(artifacts): add shared planning artifacts Sep 10, 2026

@bryantderosier bryantderosier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read-only code and security review of commit c2aa3d90d4cd82fdc0f091fc095b73c46124f845 against 28f26f6c670bedae06f93a31fe83a595936b521f.

Three medium-priority findings are detailed inline:

  1. [P2] Automatic plan export can select an obsolete revision. The projection orders plans by random IDs, while finalization uses findLast() without excluding superseded plans. A completed run can overwrite shared artifacts/plan.md with an old draft.
  2. [P2] Artifact previews remain stale after reconnection. A fresh watcher subscription emits revision 0, which the page ignores. Changes made while disconnected are not reloaded until another filesystem event or manual refresh.
  3. [P2, security] Rejected artifact writes can create directories outside storage. Parent directories are created before resolved-path containment is checked. An existing escaping directory symlink allows directory creation at its external destination before the write is rejected. This establishes directory creation, not arbitrary file overwrite.

Validation: 121 existing tests passed across 10 focused suites in an isolated snapshot of the reviewed commit. Additional focused reproductions against the finalization service and artifact page confirmed findings 1 and 2. An isolated reproduction of the filesystem sequence confirmed finding 3. Browser and live-provider behavior were not exercised. No high or critical security issue was confirmed.

Reviewed with GPT-6 in the Codex harness, with parallel correctness, security, and fork-boundary reviewers.

@bryantderosier

Copy link
Copy Markdown
Collaborator

Fork-boundary refactor recommendations for reviewed commit c2aa3d90d4cd82fdc0f091fc095b73c46124f845.

These recommendations are separate from the three correctness/security findings. The aim is to keep artifact behavior in J5-owned modules and leave small, explicit integration points in upstream T3 files, consistent with FORK.md.

  1. Remove the redundant turn-start preparation hook. ArtifactWorkspace.write, list, watch, and exportPlan already prepare storage. Unless there is an additional requirement, remove ArtifactProviderTurnStartObserver and its integration in ProviderTurnStartService and server.ts. This avoids adding a lifecycle hook and per-turn project/filesystem work to upstream orchestration for storage that prepares itself when used.

  2. Move plan selection into the J5 finalization observer. Inject ProjectionStore into j5/artifacts/ArtifactRunFinalizationObserver and resolve the project and final proposal there using the existing threadId and runId. This should allow restoring the upstream RunFinalizationObserver contract without the new projectId and planMarkdown arguments. It also puts the plan-selection correction and its tests beside the J5 feature instead of in upstream finalization logic.

  3. Extract artifact runtime assembly and WebSocket subscription behavior. Move the artifact dependency graph currently assembled in server.ts into a J5 runtime-layer helper. Move project validation, watching, revision generation, and error conversion from the roughly 50-line handler in ws.ts into a J5 subscription helper. Keep a thin composition call and RPC handler delegate upstream. This reduces overlapping edits in two central files that upstream is likely to change frequently.

  4. Use the existing J5 MCP registration boundary. Move mcp/ArtifactMcpService.ts and mcp/toolkits/artifacts/* under j5/artifacts/mcp, and compose their registration through the existing J5McpIntegrationLive seam. Avoid a separate artifact registration and dependency assembly in McpHttpServer.ts. Later artifact-tool changes can then remain within J5-owned files instead of repeatedly modifying upstream MCP infrastructure.

  5. Extract artifact Markdown-link behavior. ChatMarkdown.tsx currently owns artifact detection, logical-path rewriting, action restrictions, icon/label rendering, and styling across several locations. Put these in a J5 ArtifactMarkdownLink component/helper, leaving one dispatch after file metadata resolution and the existing upstream file-link path as the fallback. Reuse shared primitives where needed instead of duplicating context-menu logic. This concentrates artifact changes and reduces conflicts in a large, frequently edited upstream renderer.

  6. Consolidate artifact navigation and panel behavior under J5. Move sidebar route parsing and thread/project lookup into an ArtifactSidebarItem or useArtifactNavigation hook. Put the duplicated launcher metadata and panel wrapper in a small J5 module. Extract artifact-specific surface construction, migration normalization, and selection transitions from rightPanelStore into pure J5 helpers, while retaining the shared store and thin discriminator/rendering cases. This keeps feature policy out of upstream sidebar, tab, and persistence implementations without introducing a second store or a general plugin framework.

I would prioritize items 1, 2, and 5 for the largest reduction in upstream implementation changes. Keep necessary shared RPC/auth contracts, route registration, and generated route-tree entries in their integration layers. The goal is fewer and smaller upstream diff hunks, with future artifact work and its tests concentrated in J5-owned files; that should make upstream merges easier to review and resolve.

Reviewed with GPT-6 in the Codex harness.

@tyler-barton-horizon

Copy link
Copy Markdown
Collaborator Author

Fork-boundary follow-up: 2b8fa93 adopts recommendations 1 and 2. The redundant turn-start preparation observer and its upstream orchestration integration were removed, and plan lookup moved into the J5 artifact finalization observer while restoring the upstream observer contract. I am deferring recommendations 3–6 in this PR: they are broader file/module relocations with no behavior change, and would add substantial review churn after the correctness fixes. They remain reasonable candidates for a separate boundary-focused cleanup.

@bryantderosier
bryantderosier self-requested a review September 10, 2026 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL 1,000+ effective changed lines (test files excluded in mixed PRs). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants