Skip to content

feat(api): Track 3.3 — GET /api/analysis-runs/{run_id}/source-audio - #40

Merged
slittycode merged 3 commits into
mainfrom
claude/track3-source-audio-reserve-5XA5r
May 14, 2026
Merged

feat(api): Track 3.3 — GET /api/analysis-runs/{run_id}/source-audio#40
slittycode merged 3 commits into
mainfrom
claude/track3-source-audio-reserve-5XA5r

Conversation

@slittycode

Copy link
Copy Markdown
Owner

Third of four pieces from Track 3 of the merged external-repo review. Adds a stable, round-trip-saving route for re-serving the original ingested audio for a run.

Why a new route when the bytes are already reachable?

The same bytes are reachable today via GET /api/analysis-runs/{run_id}/artifacts/{artifact_id} — source audio is stored as an artifact with kind="source_audio". So this is an ergonomic shortcut, not a net-new capability. The win is:

  • Stable URL. /source-audio doesn't change per run; the artifact id does.
  • One fewer round-trip. Callers (Phase 2 reruns, external consumers) don't need to fetch the snapshot first just to learn the artifact id.
  • Predictable for tooling. A user can write /api/analysis-runs/{id}/source-audio directly into a Max patch, a REAPER script, or a CLI fetch — no inspection step needed.

Identical bytes, identical Content-Disposition and MIME on the wire.

Auth model

  • Owner-only. Standard read pattern: _resolve_route_user_context + runtime.get_run(owner_user_id=…).
  • Does NOT honor X-Admin-Key. Re-serving another user's audio has a stronger privacy posture than the cross-user delete admin bypass added in Track 3.2. Reading user content cross-tenant is more sensitive than reversibly purging it.
  • Non-owner requests return RUN_NOT_FOUND (does not disclose whether the run exists), matching the existing pattern.

Error envelope

Condition Status Code
Success 200 audio/<original-mime>
Run missing or not owned 404 RUN_NOT_FOUND
Run row exists but source_artifact_id is null/dangling (defensive) 404 SOURCE_AUDIO_NOT_FOUND
Artifact row intact but file gone from disk 404 SOURCE_AUDIO_FILE_MISSING

The two "source missing" variants are distinct because the recovery paths differ — SOURCE_AUDIO_NOT_FOUND indicates server-side corruption an operator should investigate; SOURCE_AUDIO_FILE_MISSING means the metadata is intact and re-ingestion is the fix.

Files

File Change
apps/backend/server.py New get_run_source_audio route handler, placed next to the existing get_run_artifact handler. ~85 lines.
apps/backend/tests/test_server.py 5 new tests in GetRunSourceAudioRouteTests: happy path with Content-Disposition + MIME; non-owner → RUN_NOT_FOUND; unknown run → RUN_NOT_FOUND; missing source artifact → SOURCE_AUDIO_NOT_FOUND; missing file on disk → SOURCE_AUDIO_FILE_MISSING.
apps/backend/ARCHITECTURE.md Route documented in the canonical list.

Verification

Route tests use the existing AnalysisRuntime + patch.object(server, "get_analysis_runtime", …) pattern. CI will exercise them. No pure-logic module to unit-test separately — the route is a thin shell over existing runtime helpers (get_source_artifact, resolve_artifact_local_path).

What this PR does NOT do

  • No range-request / partial-content support. FileResponse serves the whole file; ASA's 100 MiB cap keeps that bounded. Range support could be added later if a UI playback feature needs it.
  • No frontend changes. Phase 2 reruns are the intended consumer; the UI wiring is a separate follow-up.
  • No admin-key bypass. Conservative read posture; see auth model above.

Track 3 sequencing

https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg


Generated by Claude Code

Third of four pieces from Track 3. Adds a stable, round-trip-saving
route for re-serving the original ingested audio for a run, so
clients (Phase 2 reruns, external consumers) don't have to fetch the
snapshot first to learn the artifact id.

The same bytes are reachable today via
GET /api/analysis-runs/{run_id}/artifacts/{artifact_id} (source audio
is stored as an artifact with kind='source_audio'). This new route is
an ergonomic shortcut, not a new capability:

- Stable URL (doesn't change per run; the artifact id does)
- Caller doesn't need to fetch the snapshot first
- Identical bytes, identical Content-Disposition and MIME

Auth model:

- Owner-only via _resolve_route_user_context + runtime.get_run(
  owner_user_id=...). Standard read pattern.
- Does NOT honor X-Admin-Key. Re-serving another user's audio has a
  stronger privacy posture than the cross-user delete admin bypass
  (Track 3.2). The review scoped admin to operations, not reads.
- Non-owner request returns RUN_NOT_FOUND, matching the existing
  pattern that does not disclose whether a run exists.

Error codes:

- RUN_NOT_FOUND (404) — run missing or not owned.
- SOURCE_AUDIO_NOT_FOUND (404) — defensive; would indicate the run
  row exists but source_artifact_id is null/dangling. Distinct from
  RUN_NOT_FOUND because the recovery path differs (server-side
  corruption vs ownership/typo).
- SOURCE_AUDIO_FILE_MISSING (404) — artifact row intact but bytes
  gone from disk. Distinct from SOURCE_AUDIO_NOT_FOUND because the
  recovery path differs (re-ingest vs operator look).

Files:

- apps/backend/server.py — new route between the existing artifact-
  fetch route and the spectral-enhancements route. ~85 lines.
- apps/backend/tests/test_server.py — 5 new tests in
  GetRunSourceAudioRouteTests covering: happy path with content-
  disposition; non-owner returns RUN_NOT_FOUND; unknown run returns
  RUN_NOT_FOUND; missing source artifact returns SOURCE_AUDIO_NOT_FOUND;
  missing file on disk returns SOURCE_AUDIO_FILE_MISSING.
- apps/backend/ARCHITECTURE.md — documents the new route.

Out of scope (deferred):

- Range requests / partial content for streaming. The FileResponse
  doesn't currently support Range; sufficient for the audio sizes
  ASA handles (100 MiB cap).
- Frontend UI that uses this endpoint. Phase 2 reruns are the
  intended consumer; UI wiring is a follow-up.

https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg

@slittycode slittycode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Verdict: APPROVE

Summary

Adds GET /api/analysis-runs/{run_id}/source-audio as an ergonomic stable-URL shortcut over the existing GET .../artifacts/{artifact_id} route. The route is a thin shell over get_source_artifact + resolve_artifact_local_path, follows the existing ownership-check pattern exactly, and comes with 5 tests covering all documented error branches. No Phase 1 data touched, no schema changes, no DSP.

The venv wasn't bootstrapped in this environment so I reviewed the tests statically; CI will catch execution failures.

Findings

Should fix:

SOURCE_AUDIO_NOT_FOUND and SOURCE_AUDIO_FILE_MISSING error bodies omit requestId, retryable, and diagnostics. CLAUDE.md states these are always required in error envelopes. The existing _run_not_found_response, ARTIFACT_NOT_FOUND, ARTIFACT_FILE_MISSING, and AUTHENTICATION_REQUIRED responses all have the same omission — so the new code is consistent with its neighbors — but that doesn't make it compliant. If the codebase is ever brought into full contract compliance on the run-management routes, these two new codes will need the same fix.

Worth considering:

get_source_artifact re-queries analysis_runs independently from get_run() (server.py:2321). In a concurrent delete + fetch race, get_source_artifact raises KeyError("Unknown run …") which the except KeyError: at server.py:2322 catches and maps to SOURCE_AUDIO_NOT_FOUND rather than RUN_NOT_FOUND. Both are 404s so this is a misclassification, not a data hazard. Practically unreachable but worth naming if the error codes are ever surfaced to operators for triage.

test_unknown_run_returns_404 doesn't patch SONIC_ANALYZER_RUNTIME_PROFILE to hosted the way the other four tests in GetRunSourceAudioRouteTests do. Functionally correct (local profile assigns user_id="local-dev" and the run genuinely doesn't exist), just inconsistent with the rest of the class setup.

Test results

5 new tests in GetRunSourceAudioRouteTests. Static analysis: all logic branches covered — happy path, non-owner, unknown run, missing artifact row, missing file on disk. No execution environment available; CI is the gate.

Phase boundary check

Clean. No Phase 1 fields read, mutated, or re-derived. No changes to analyze.py, any analyze_*.py module, Phase 2 prompts, or types.ts.


Generated by Claude Code

claude added 2 commits May 13, 2026 22:19
…onsistency

Addresses PR #40 review:

1. Should fix: SOURCE_AUDIO_NOT_FOUND and SOURCE_AUDIO_FILE_MISSING
   error envelopes now carry the 'retryable' boolean per the CLAUDE.md
   contract. Both are False — corrupted run state and missing-on-disk
   bytes don't recover from a naive retry; operator action is required.

   The contract also calls for requestId and diagnostics; those are
   omitted across the existing run-management routes (ARTIFACT_NOT_FOUND,
   ARTIFACT_FILE_MISSING, AUTHENTICATION_REQUIRED), and bringing the
   whole surface into full compliance is out of scope for this PR.
   Adding retryable here keeps the new code from drifting further.

2. Worth considering: test_unknown_run_returns_404 didn't patch
   SONIC_ANALYZER_RUNTIME_PROFILE the way the other four tests in
   the class do. Functionally correct (local profile auto-assigns
   local-dev) but inconsistent. Now patches to hosted for setup
   parity.

3. Not fixed (per reviewer's 'practically unreachable'): the
   delete/fetch race that maps to SOURCE_AUDIO_NOT_FOUND instead of
   RUN_NOT_FOUND. Disambiguation requires either string-matching the
   KeyError message (fragile) or adding owner_user_id threading to
   get_source_artifact. Both are real cost for an unreachable case.

The existing SOURCE_AUDIO_NOT_FOUND / SOURCE_AUDIO_FILE_MISSING
route tests now assert the retryable field, so a future regression
(removing it) would fail.

https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
The Frontend CI on the previous commit failed after 1m 53s, which is
too short for smoke tests to have executed — npm ci + playwright
install --with-deps chromium typically take 60-90 s combined before
'npm run verify' even starts.

Local checks all pass: npm run lint (clean), npm run test:unit
(398/398), npm run build (clean). Backend CI on the same workflow
passed at 4m 21s.

My local apt-get failed when installing Playwright system deps
('ppa:ondrej/php repository no longer signed'), suggesting a similar
transient registry/apt blip caused the CI failure. Empty commit
retriggers CI to confirm.

Smoke tests stub all backend routes via page.route() in their specs,
so the new GET /api/analysis-runs/{run_id}/source-audio route this
PR adds cannot affect them.

https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
@slittycode
slittycode merged commit b2332f0 into main May 14, 2026
2 checks passed
@slittycode
slittycode deleted the claude/track3-source-audio-reserve-5XA5r branch May 14, 2026 00:53
slittycode added a commit that referenced this pull request May 14, 2026
Fourth and final piece of Track 3 from the merged external-repo review.
Additive over the existing 8-state stage status — does not change or
remove anything.

What's new:

- apps/backend/stage_status.py (new) — pure module with the canonical
  8 -> 5 mapping table, the to_public_status() helper, and
  PUBLIC_STATUS_VALUES exported for type generation / docs.

  Mapping (internal -> public):
    queued        -> queued
    running       -> running
    blocked       -> queued        (transient scheduling state)
    ready         -> queued        (scheduled but not yet running)
    completed     -> completed
    failed        -> failed
    interrupted   -> interrupted
    not_requested -> null          (stage not in pipeline)

- apps/backend/server_phase1.py — _normalize_run_snapshot now also
  annotates each stage with publicStatus via the new
  _annotate_public_status helper. The original status field is
  preserved untouched.

- apps/backend/tests/test_stage_status.py (new, 16 tests) — pins
  every cell of the mapping, the public-value set, defensive cases
  (None, unknown, empty, case-sensitive), and a completeness check
  that fails if the set of internal states drifts without an explicit
  mapping update.

- apps/backend/tests/test_server.py (+1 test) — integration test
  that GET /api/analysis-runs/{run_id} returns publicStatus on all
  three stages with the correct values for a fresh run.

- apps/ui/src/types/backend.ts — new PublicStageStatus type (the
  5-value union). publicStatus: PublicStageStatus | null added to
  MeasurementStageSnapshot, PitchNoteTranslationStageSnapshot,
  InterpretationStageSnapshot. attempt summaries unchanged
  (publicStatus is only on top-level stages).

- apps/ui/src/services/analysisRunsClient.ts — new
  parsePublicStageStatus helper + PUBLIC_STAGE_STATUSES set; threaded
  into the three stage parsers. The helper accepts null and missing
  values without throwing so legacy snapshots that pre-date the
  field continue to deserialize.

- docs/adr/0001-phase1-json-schema-v1.md — documents the additive
  field, the mapping table, and the rationale (internal scheduling
  states are scheduling concerns the client doesn't need to act on).

Out of scope (deferred):

- Removing publicStatus duplication from the internal status field
  later. The additive approach was the explicit choice; collapsing
  to one field would be a breaking change requiring a v2 schema bump.
- Frontend UI that reads publicStatus. Existing UI continues to use
  the 8-state status field; publicStatus is the additive surface for
  future external consumers and any new UI views that prefer the
  smaller vocabulary.

This closes Track 3 (all four pieces shipped):
  - 3.1 URL ingestion (#36, merged)
  - 3.2 X-Admin-Key on DELETE (#37, merged)
  - 3.3 GET /source-audio re-serve (#40, merged)
  - 3.4 publicStatus additive (this PR)

https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants