Skip to content

feat: tag daemon artifacts with semantic types#1066

Merged
thymikee merged 6 commits into
callstack:mainfrom
sjchmiela:stanley/daemon-artifact-types
Jul 4, 2026
Merged

feat: tag daemon artifacts with semantic types#1066
thymikee merged 6 commits into
callstack:mainfrom
sjchmiela:stanley/daemon-artifact-types

Conversation

@sjchmiela

@sjchmiela sjchmiela commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

When we collect and upload artifacts to EAS I think it would be nice if we could tell the user what they are not only through the file name, but also through some kind of type? Maybe we can even group them and display in different ways — screenshots as one collapsible / gallery; logs some other way, etc.


Summary

Adds semantic artifactType metadata to daemon-managed downloadable artifacts so clients can display or group screenshots, screen recordings, chunks, telemetry, and trace logs without guessing from filenames or MIME types.

Typed artifact producer APIs now require callers to provide the artifactType property. Callers may still pass undefined explicitly when an artifact is intentionally untyped, and daemon finalization keeps those artifacts registered instead of dropping them. The value is threaded through runtime output reservations, daemon finalization, downloadable artifact tracking, and daemon artifact inventory.

Validation

  • pnpm format
  • pnpm typecheck
  • pnpm check:unit

Copilot AI review requested due to automatic review settings July 3, 2026 16:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds an optional semantic artifactType to daemon-managed downloadable artifacts so clients can classify/group artifacts (e.g., screenshots, recordings, trace logs) without inferring meaning from filenames or MIME types. This is threaded through contracts, runtime output reservation, daemon finalization, artifact tracking/inventory, and surfaced in CLI output and tests while keeping artifactType optional.

Changes:

  • Introduces DaemonArtifactType (known values + open string) and adds artifactType? to daemon artifact contracts/descriptors.
  • Propagates artifactType through output reservation, daemon response finalization, downloadable artifact tracking, and artifact inventory/CLI formatting.
  • Updates integration/unit tests and domain vocabulary docs to cover the new metadata.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/integration/provider-scenarios/daemon-http-server.test.ts Tracks a downloadable screenshot artifact with artifactType.
src/kernel/contracts.ts Adds DaemonArtifactType and artifactType? to DaemonArtifact.
src/io.ts Extends artifact descriptors and output reservation options to carry artifactType.
src/daemon/request-router.ts Updates router dependency typing for trackDownloadableArtifact to accept artifactType.
src/daemon/request-finalization.ts Preserves/passes artifactType when registering downloadable artifacts; adds screenshot fallback type.
src/daemon/request-execution-scope.ts Updates request scope typing for trackDownloadableArtifact to accept artifactType.
src/daemon/handlers/record-trace-recording.ts Tags recording, chunk, and telemetry artifacts with semantic types.
src/daemon/handlers/tests/session-replay.test.ts Updates replay test fixtures to include artifactType for recordings.
src/daemon/artifact-tracking.ts Stores and exposes artifactType in the daemon artifact inventory entries.
src/daemon/tests/response-views.test.ts Ensures response views/digests preserve artifact type metadata.
src/daemon/tests/request-handler-catalog.test.ts Updates handler catalog test artifact tracking to include artifactType.
src/daemon/tests/request-finalization.test.ts Adds coverage ensuring finalization passes artifactType into tracking and response artifacts.
src/daemon/tests/http-server-artifacts.test.ts Extends inventory response typing and tests artifactType in inventory; adjusts polling helper.
src/commands/recording/runtime/recording.ts Reserves outputs with semantic types for recording and trace commands.
src/commands/management/output.ts Renders daemon artifact inventory lines including artifactType when present.
src/commands/management/output.test.ts Updates CLI output expectations to include artifactType.
src/commands/capture/runtime/screenshot.ts Reserves screenshot output with artifactType: 'screenshot'.
src/commands/capture/runtime/diff-screenshot.ts Reserves diff output as screenshot-diff and overlay output as screenshot.
src/cloud-artifacts.ts Extends daemon artifact inventory entry type to include artifactType.
src/cli/commands/tests/screenshot.test.ts Ensures digest JSON passthrough includes artifactType for artifacts.
src/tests/daemon-entrypoint.test.ts Minor formatting update around tracking runtime artifacts.
src/tests/client.test.ts Tightens digest typing and includes artifactType in artifact entries.
CONTEXT.md Documents DaemonArtifactType vocabulary and preservation rules.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/daemon/__tests__/http-server-artifacts.test.ts
Copilot AI review requested due to automatic review settings July 3, 2026 16:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.

Comment thread src/kernel/contracts.ts
Comment thread src/io.ts
Comment thread src/io.ts
Comment thread src/daemon/artifact-tracking.ts
Comment thread src/daemon/__tests__/http-server-artifacts.test.ts
Copilot AI review requested due to automatic review settings July 3, 2026 16:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.

Comment on lines +336 to 340
async function waitFor(condition: () => boolean | Promise<boolean>): Promise<void> {
for (let attempt = 0; attempt < 20; attempt++) {
if (condition()) return;
if (await condition()) return;
await new Promise((resolve) => setTimeout(resolve, 10));
}
@thymikee

thymikee commented Jul 4, 2026

Copy link
Copy Markdown
Member

Design review: this is directionally sound. I would keep the semantic tag command-owned and preserve-only, not inferred from filename/MIME/field. The open-string type also seems right for this repo: the known literals document first-party categories, while provider/command owners can add narrower values later without another daemon protocol shape change.\n\nThe one API-shape tweak I would consider before merge: split producer discipline from wire compatibility. For producer-owned APIs like reserveOutput(...) and trackDownloadableArtifact(...), requiring artifactType: DaemonArtifactType | undefined is useful because it forces the artifact owner to explicitly decide whether the artifact is classified. For public/wire/result shapes such as DaemonArtifact and daemon artifact inventory entries, I would model it as artifactType?: DaemonArtifactType instead. Missing metadata is valid, JSON serialization drops explicit undefined anyway, and remote/older daemon responses may simply omit the field. That keeps the external contract honest while still letting internal producer APIs prevent accidental silent omission.\n\nI do not think this needs an enum-only registry or filename/MIME fallback. That would make the display layer feel simpler short-term but would recreate the guessing this PR is trying to remove. The current threading through finalization, downloadable artifact tracking, response views, remote materialization, and CLI output looks coherent. Checks are mostly green; I did not mark ready while the remaining smoke jobs are still pending.

@thymikee

thymikee commented Jul 4, 2026

Copy link
Copy Markdown
Member

Final CI refresh: all required checks are green now; Bundle Size is skipped for this run. Since the design review found no actionable blockers, I added ready-for-human.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jul 4, 2026
Applies the design-review tweak: producer-owned APIs (reserveOutput,
trackDownloadableArtifact, finalization callbacks) keep the required
'DaemonArtifactType | undefined' form so artifact owners must explicitly
decide, while public/wire/result shapes (DaemonArtifact, both artifact
inventory entry types) become 'artifactType?:' — missing metadata is
valid, JSON drops undefined, and remote/older daemons may omit the
field. Construction sites now omit the key for untyped artifacts, and
the finalization test asserts key absence (toEqual cannot distinguish
absent from explicitly-undefined).
@thymikee

thymikee commented Jul 4, 2026

Copy link
Copy Markdown
Member

Reviewed the full diff plus the follow-up commits — this is a good idea, and the design landed where the earlier review pointed: the tag is command-owned and preserve-only (no filename/MIME inference anywhere in the threading), the open string union (DaemonArtifactKnownType | (string & {})) documents first-party categories without freezing the space, and untyped artifacts still register. The waitFor robustness comments are addressed by the branch's own follow-up commits (fails on timeout now). With #1073 merged on main, inventory consumption is deterministic before the download response completes, so those waits should never spin anyway.

I pushed the one outstanding design-review item directly (e1d641a, as offered): producer/wire split for the type's optionality.

  • Producer-owned APIs keep the required artifactType: DaemonArtifactType | undefinedreserveOutput, trackDownloadableArtifact, the finalization/router callbacks, ArtifactDescriptor. Artifact owners must still explicitly decide.
  • Public/wire/result shapes become artifactType?:DaemonArtifact (kernel contract), DownloadableArtifactInventoryEntry, and the client-side DaemonArtifactInventoryEntry. Missing metadata is valid; remote/older daemons may omit the field.
  • Construction sites (registerDownloadableArtifacts, listDownloadableArtifacts) now omit the key entirely for untyped artifacts instead of carrying artifactType: undefined, and the finalization test asserts key absence (toEqual can't distinguish absent from explicitly-undefined).

Verified on the branch: typecheck/lint/format clean, 1337 unit + 87 provider integration tests green. From my side this is merge-ready once CI confirms.

@thymikee

thymikee commented Jul 4, 2026

Copy link
Copy Markdown
Member

This PR changed after the earlier ready-for-human label, and GitHub currently reports no checks on the updated stanley/daemon-artifact-types head. I’m clearing ready-for-human until CI/checks are reported for this head; once checks are back and green, the existing design review can stand.

@thymikee thymikee removed the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jul 4, 2026
@thymikee

thymikee commented Jul 4, 2026

Copy link
Copy Markdown
Member

CI is fully refreshed on e1d641aa8 now: all required checks are green, with Bundle Size skipped for this run. The existing design review still applies to this head, so I restored ready-for-human.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jul 4, 2026
@thymikee thymikee merged commit 9407839 into callstack:main Jul 4, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants