From 0a772e03b44056b4807c2317bcac6859be4092b6 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 10 Aug 2026 13:53:49 +0000 Subject: [PATCH] Document external adapter audit findings --- docs/roadmap/audit-findings.md | 532 +++++++++++++++++++++++++++++++-- 1 file changed, 504 insertions(+), 28 deletions(-) diff --git a/docs/roadmap/audit-findings.md b/docs/roadmap/audit-findings.md index d40d126..08d1862 100644 --- a/docs/roadmap/audit-findings.md +++ b/docs/roadmap/audit-findings.md @@ -104,7 +104,7 @@ These are prioritization signals, not findings: | 4: publish and cleanup | complete | Publish prerequisites/source families, deterministic upload order, every partial remote outcome, retry semantics, effective locks, status/restore interpretation, and automatic/manual cleanup gates reviewed. Focused stage/app/artifacts/storage tests passed. Confirmed `COR-004` through `COR-007` and `RSK-005`; added `ARC-003`, `COM-002`, and `TST-004`. | | 5: restore and previous state | complete | Restore discovery/planning/execution/reporting, remote-current identity and scope, every local failure boundary, audio cache/spool identity, previous-cache planning/consumption, and status/validate policy reviewed. Focused app/previouscache/audio/artifacts/storage tests passed. Confirmed `COR-008` through `COR-011`, `RSK-006` through `RSK-008`, and `EFF-001`; added `DUP-003` and `TST-005`. | | 6: configuration and composition | complete | Discovery/precedence, strict loading, defaults/normalization, templates, cross-field validation, CLI selection, filesystem secrets, conditional adapter composition, and maintained examples reviewed. Exact focused tests and repository vet passed. Confirmed `COR-012` through `COR-015`, `RSK-009`, and `RSK-010`; added `ARC-004`, `DUP-004`, and `TST-006`. | -| 7: adapters and shared support | not_started | Assigned external-boundary and cancellation areas below. | +| 7: adapters and shared support | complete | All HTTP, subprocess, notification, storage, audio, shared-model, and diagnostic boundaries were compared with their contracts and production callers. Focused normal and race commands passed. Confirmed `COR-016`, `RSK-011` through `RSK-014`, `EFF-002`, and `ARC-004`; added `ARC-005`, `DUP-005`, `COM-003`, and `TST-007`, and refined `TST-001`. | | 8: ordinary stages | not_started | Assigned prepare/transcript behavior and disabled-outcome questions below. | | 9: extraction | not_started | Assigned extraction promotion, provenance, and resume scenario below. | | 10: analyze and dependencies | not_started | Assigned artifact dependency/source and selection scenario below. | @@ -131,8 +131,8 @@ has been mapped but not behaviorally audited. | Artifacts, paths, and policy | `internal/artifacts`, `internal/artifactpolicy`, `internal/pathsafe` | 3 (resolution consumption revisited in 10) | reviewed | | Restore | `internal/app`, `internal/artifacts`, `internal/previouscache`, `internal/audio` | 5 | reviewed | | File operations | `internal/fileops`, `internal/pathsafe`, local artifact store | 3 (promotion vertical slice in 9) | reviewed | -| External adapters and storage | `internal/adapters`, `internal/audio` | 7 | assigned | -| Shared models and diagnostics | `internal/artifactmodel`, `internal/contracts`, `internal/logging` | 7 (maintainability revisited in 11) | assigned | +| External adapters and storage | `internal/adapters`, `internal/audio` | 7 | reviewed | +| Shared models and diagnostics | `internal/artifactmodel`, `internal/contracts`, `internal/logging` | 7 (maintainability revisited in 11) | reviewed | | Tests, examples, and automation | package test owners, `examples/`, `.woodpecker/` | 12 | assigned | ## Package And Interface Ownership Map @@ -326,6 +326,16 @@ terminalization/reconciliation harder to test. | 9. Disabled/unselected/reused/generated/extraction/previous source then publish filtering | 10 | Analyze, artifact catalog/resolver/policy, publish tests; config ownership in 6 and publish result in 4 | | 10. Concurrent same-session invocation and lock cleanup failures | 3 | Runner lock lifetime in 2; local artifact store, path/file cleanup and lock tests in 3 | +Scenario 8 is resolved at the adapter boundary. Parent cancellation reaches +HTTP attempts, retry waits, every AWS call, audio downloads, and each direct +child process. HTTP/S3/local-file resources are released on error, and a +started direct child is waited. Cancellation is not complete for multipart +body construction (`EFF-002`) or subprocess descendants (`RSK-011`), and a +malformed repeated S3 continuation token needs cancellation to escape its +otherwise non-progressing loop (`RSK-014`). Runner reporting remains the Stage +2 dual-ledger behavior; Stage 8 owns worker aggregation and partial transcript +results rather than reopening these transport conclusions. + ## Preliminary Risk-To-Test Matrix This matrix identifies intended owners only. It makes no sufficiency judgment. @@ -361,6 +371,16 @@ Stage 2 test observations for this matrix: | Interruption and restart | Non-succeeded action logic and retry tests indirectly prove `running` is rerunnable. | No kill/reload normalization, reconciliation, or abandoned-run status test; confirmed `RSK-001`. | | Same-session concurrency | Artifact store has focused lock tests. | No assembled concurrent runner or release-failure test; Stage 3 owns the mechanism and sufficiency decision. | +Stage 7 test observations for this matrix: + +| Risk | Existing focused protection | Gap or disposition | +| --- | --- | --- | +| HTTP retry, cancellation, and response installation | WhisperX tests cover success, retryable/non-retryable statuses, attempt timeout, parent cancellation, malformed JSON, and absence of failed output. | No supported-scheme table, streaming/body-production cancellation, response close observation, or oversized response case; `COR-016`, `EFF-002`, and `TST-007`. | +| Process launch, wait, and diagnostics | Shared tests cover successful separated/shared logs, start/exit context, a direct-child timeout, environment inheritance/override, bounded tail use, and override-secret error redaction. Each protocol adapter checks exact invocations and normal failures. | No process-descendant, explicit cancellation, inherited-secret, raw-log redaction, symlink, non-regular, or oversized-output case; `RSK-011` through `RSK-013` and `TST-007`. | +| S3 resources and pagination | Focused tests cover one-page normalization, streamed download/upload, not-found adaptation, and credential option construction. Temporary-download tests cover failure cleanup and wrapped context causes. | No valid multi-page, repeated/empty-token, later-page failure, cancellation, or body-close probe; `RSK-014` and `TST-007`. | +| Adapter fakes under production concurrency | No-op/fake tests cover cancellation/error and deterministic placeholder materialization. The focused adapter race command passes. | The WhisperX fake's unsynchronized request slice fails the full race suite when the transcribe stage uses it concurrently; `TST-001`. Other fakes currently have sequential production callers, so no blanket race finding. | +| Shared models and logging | Artifact-model JSON/conversion tests and bounds success/error tables protect current serialized shapes; logger tests protect output/nil-writer construction. | Output acquisition bounds/type remain `RSK-013`; suite-wide fake/model/logging value and redundancy remain Stage 12 work. | + ## Path, Artifact, Filesystem, And Lock Conclusions ### Canonical ownership and normalization @@ -855,6 +875,122 @@ as a production defect. Mutable package-level constructor seams and small single-stage command wrappers are likewise test/dispatch mechanics, not new architectural findings. +## External Adapter And Shared-Support Conclusions + +### Boundary and resource matrix + +Stages depend only on Narratio request/result interfaces. HTTP, multipart, AWS +SDK, Smithy, `exec.Cmd`, and process-state types remain private to their adapter +packages; no transport type or retry policy leaks into stage code. Arguments, +working directories, environment changes, generated invocation configuration, +stream routing, exit adaptation, and first-pass output validation are likewise +adapter-owned. Production callers choose run-local paths and materialize +validated results, which is the intended division of policy. + +| Boundary | Acquisition | Cancellation and waiting | Release and conclusion | +| --- | --- | --- | --- | +| WhisperX HTTP | Opens the audio file for each attempt, constructs multipart content, and obtains one response. | Each attempt has its own timeout; parent cancellation suppresses retries and interrupts retry timers and HTTP I/O. Building the multipart body itself is not cancellation-aware. | Audio files are closed and response bodies are closed on every response path. Responses are bounded to 10 MiB and installed only after successful JSON validation. Request prebuffering remains `EFF-002`; retry/status behavior otherwise matches the integration contract. | +| Shared subprocess | Opens zero, one shared, or two separate log files, then starts one direct child and waits synchronously. All subprocess adapters use this owner. | Parent cancellation and optional timeout reach `exec.CommandContext`; `Wait` always runs for a successfully started direct child. Only that process is killed, not its descendants (`RSK-011`). | Open-failure cleanup is correct and log descriptors are closed after `Wait`, although close errors are intentionally discarded. Raw stream and diagnostic redaction is incomplete under `RSK-012`. | +| S3 object storage | AWS client construction is lazy with respect to network I/O. Each download obtains a response body and destination file; upload opens and streams a source file. | Every SDK call receives the caller context. List pagination has no deadline or progress guard (`RSK-014`). | Response bodies and local files are closed; uploads stream rather than prebuffer. Missing-object errors are adapted to `(false, nil)`. Successful system-temp ownership remains the caller issue in `RSK-009`, not an adapter leak. | +| Audio materialization | Creates a sibling temporary file and delegates one download on cache miss. | Context is checked before work and passed to storage. | Failure removes the temporary file and success renames it into place. Cache identity remains `RSK-007`; repeated install mechanics remain `DUP-003`. | +| Notification | The no-op/fake sender acquires no external resource. | Both honor a canceled context before returning. | There is no production delivery resource to release because no production transport exists; the accepted operator configuration is therefore confirmed as `ARC-004`. | + +No adapter creates an internal goroutine or channel. WhisperX concurrency is +owned by the transcribe stage, while every subprocess call is synchronous. The +focused adapter race command consequently passes; the full baseline race still +fails only when the unsynchronized WhisperX fake is exercised concurrently by +the stage. `TST-001` therefore represents a fake/consumer contract defect, not +an HTTP-client race, and Stage 8 must inspect the worker behavior before Stage +12 chooses the test-double repair. + +### HTTP, storage, retry, and malformed-response behavior + +WhisperX uses stable multipart field names, per-attempt timeouts, a bounded +response reader, context-aware retry delay, and the documented retry classes: +429, 5xx, attempt timeout, and network errors retry; other 4xx, malformed +successful JSON, and explicit cancellation do not. Failed attempts never +install the output. The constructor does, however, accept any absolute URL with +a host, including `ftp://`, although the concrete HTTP transport cannot execute +that request; this is `COR-016`. The full audio file is copied into a +`bytes.Buffer` before the HTTP request begins on every attempt. This both scales +memory with concurrent input size and delays cancellation until after local +copying, as recorded in `EFF-002`. + +S3 operations normalize bucket-relative keys and leave ordering policy to +callers. The callers that need deterministic order sort their resulting object +sets. Downloads and uploads stream, response bodies are closed, provider +not-found shapes are adapted, and system-temporary download failure removes the +partial file. Pagination continues only while the response is truncated and a +next token exists, but a repeated non-empty token is accepted forever. A faulty +or S3-compatible provider can therefore make one list call spin and append +duplicate pages until cancellation or exhaustion (`RSK-014`). No focused test +models multiple pages, token progress, or a malformed pagination response. + +### Subprocess protocol and output validation + +Audita, Seriatim, Scriptorium, and Notarius all delegate executable launch, +timeout, environment merging, stream capture, wait, exit metadata, and bounded +diagnostic-tail mechanics to `internal/adapters/subprocess`. Their flag order is +deterministic and their generated configuration stores credential environment +names/presence rather than values. Audita deliberately maps its configured +credential into `AUDITA_LLM_API_KEY`; Notarius and Scriptorium use the inherited +environment documented by their protocols. No current adapter places a raw +credential in arguments or generated configuration. + +Notarius has the strongest external-output boundary: stdout is not parsed after +a process failure; receipt, index, warnings, and rejection payloads are size +bounded; inputs and bundle/lane outputs must be regular, non-symlinked files; +bundle roots cannot escape; and declared lane media/schema/module contracts are +matched before results are exposed. The other subprocess adapters use +unbounded `os.ReadFile` for known JSON/text results or `os.Stat` for presence +and non-empty checks. Those calls follow symlinks and do not establish a +regular-file handle before parsing. Shared bounds parsing has the same +unbounded, link-following shape. `RSK-013` records this one external-output +trust-boundary cause rather than separate findings for every adapter. + +The shared launcher correctly reports executable, argument, working-directory, +timeout, exit, and log-path context, and limits the returned stderr tail to 2 +KiB. It only redacts sensitive values supplied in `EnvOverrides`, while the +actual stdout/stderr files are always raw and inherited sensitive environment +values are unknown to the redactor. A downstream tool echoing either kind of +credential can therefore place it in persisted logs, and an inherited value can +also enter the returned error and both manifests. This contradicts the +repository's explicit no-secrets-in-logs/manifests invariant (`RSK-012`). + +Request and result contracts otherwise match their external documents. +Scriptorium's validation-failure exit is adapted distinctly; Audita and +Seriatim validate their documented JSON shapes; Notarius preserves structured, +bounded diagnostics; and failure results retain non-secret paths and process +metadata. Audita's request object redundantly carries most static constructor +settings even though the production runner reads only `Modules` from the +request. Because production supplies equal values, this is not a current +correctness defect; `ARC-005` assigns the contract/fake-fidelity decision to the +later maintainability and test passes. + +### Shared models, diagnostics, and repeated mechanics + +`internal/artifactmodel` and `internal/contracts` contain stable JSON-tagged +Narratio models rather than provider objects. Artifact conversion clones slice +state at the boundary, and no lossy transport conversion was found. Bounds +parsing accepts compatible unknown fields and normalizes integer-like IDs, then +validates ordering and membership against the transcript. Its unbounded +external reads are included in `RSK-013`; no separate serialization defect was +established. + +`internal/logging` is a deliberately small `slog` text-handler constructor and +owns no resource. It does not offer redaction, so callers must not submit secret +values; the concrete violation comes from subprocess error/log construction in +`RSK-012`, not from logger construction itself. + +The two adapter-local atomic byte writers and the shared `fileops` writer repeat +the same same-directory temp/write/sync/chmod/rename mechanism. This is a real +mechanical duplication (`DUP-005`), and all variants inherit `RSK-002`'s missing +parent-directory sync. Protocol-specific argument builders, output schemas, and +error adaptations are meaningfully different and should remain local. Repeated +fake placeholder materialization is test support whose suite-wide value and +fidelity remain assigned to Stage 12; it is not a production abstraction +candidate from this pass. + ## Confirmed Findings ### `COR-001`: session identity initialization preserves stale invocation paths and accepts conflicting identity @@ -1380,6 +1516,34 @@ architectural findings. - Dependencies: `COR-011` owns remote artifact source mapping after an identity is selected; this finding owns selection of that identity. +### `COR-016`: the WhisperX HTTP client accepts schemes its transport cannot execute + +- Category: confirmed correctness defect. +- Locations/invariant: `internal/adapters/whisperx/http.go` in `NewHTTPClient` + and the WhisperX integration contract. A successfully constructed HTTP + adapter should accept only endpoint schemes its transport supports. +- Evidence: construction requires only a parsed URL with a non-empty scheme and + host. Values such as `ftp://example.com/transcribe` pass, while the standard + HTTP client later rejects the request as an unsupported protocol scheme. + Constructor tests cover missing and syntactically malformed values but no + non-HTTP absolute scheme. +- Realistic scenario: a copied or templated endpoint uses `ftp` or another + absolute scheme. Configuration and adapter construction succeed, but every + speaker transcription fails only after pipeline work reaches the first + request. +- Impact/likelihood/confidence: medium operability impact; low-to-moderate + configuration-error likelihood; high confidence from the constructor and + concrete transport contract. +- Estimated remediation scope and owner: small WhisperX adapter validation + change. Admit only `http` and `https` after normalization and retain the + existing absolute-host requirement. +- Test changes: table-test supported HTTP/HTTPS endpoints and reject FTP, + scheme-relative, hostless, and malformed values at construction. Add a + config-to-constructor case only if config validation also chooses to own the + scheme rule. +- Dependencies: `COR-013` separately owns duration validation disagreement; + neither finding requires changing retry behavior. + ### `RSK-001`: invocation audit records can remain indefinitely `running` - Category: confirmed correctness/operational risk. @@ -1726,6 +1890,127 @@ architectural findings. must not duplicate filesystem policy. `COR-003` owns mutation confinement, whereas this finding owns reads from the secret directory. +### `RSK-011`: subprocess cancellation terminates only the direct child + +- Category: confirmed correctness/operational risk. +- Locations/invariant: `internal/adapters/subprocess.Run`, used by every Audita, + Seriatim, Scriptorium, and Notarius invocation; integration contracts state + that parent cancellation and timeouts bound invocations. Cancellation must + terminate all work started for one tool invocation, not merely return from + waiting on its first process. +- Evidence: the launcher uses `exec.CommandContext` without changing process + attributes or `Cmd.Cancel`. In the audited Go toolchain that cancel function + calls `Kill` on `cmd.Process`; no process group/job ownership or descendant + cleanup exists. The timeout test starts a sleeping direct helper only. +- Realistic scenario: an external CLI starts a worker subprocess and then the + stage times out. Narratio kills and waits for the CLI, returns a timeout, and + closes its descriptors, while the worker continues consuming CPU, writing + run-local files, or making paid API requests after the manifest records + failure. +- Impact/likelihood/confidence: high resource/integrity impact; moderate + likelihood for Python/worker-based external tools; high confidence in the + launcher semantics, with exact descendant behavior dependent on each tool. +- Estimated remediation scope and owner: medium, platform-aware subprocess + mechanism. Establish an invocation-owned process group on Unix and equivalent + job/process-tree behavior on supported Windows, terminate the group on + cancellation, wait/reap deterministically, and document any unavoidable + platform limit. +- Test changes: add a helper that spawns a descendant, records its identity, + times out, and proves the descendant cannot write a delayed sentinel. Keep + the direct-child timeout test and add explicit parent cancellation. +- Dependencies: keep process mechanics in the shared launcher; adapter packages + should not each implement their own kill policy. + +### `RSK-012`: subprocess diagnostics can persist raw credentials + +- Category: confirmed confidentiality risk. +- Locations/invariant: `internal/adapters/subprocess.Run`, `openLogWriters`, + `readRedactedTail`, and inherited-environment use by Notarius and Scriptorium. + Architecture policy forbids raw secrets in logs and manifests. +- Evidence: stdout and stderr are written directly to persisted log files with + no filtering. The 2 KiB error tail replaces only values whose keys look + sensitive in `RunRequest.EnvOverrides`; values inherited through + `os.Environ()` are not considered. Audita's mapped key is protected only in + the returned tail, not in its raw log. Scriptorium and Notarius intentionally + inherit their environment, so a child that echoes an inherited credential + can also place it in the wrapped stage error persisted to both manifests. +- Realistic scenario: a downstream tool includes its API key in a debug/error + dump. Narratio retains the value in a run log, and for an inherited key also + embeds it in the durable failure text. Run archives or troubleshooting access + then disclose the credential beyond its intended environment boundary. +- Impact/likelihood/confidence: high confidentiality impact; low-to-moderate + faulty/debug-tool likelihood; high confidence in the diagnostic flow. +- Estimated remediation scope and owner: medium shared subprocess/adapter + correction. Define the sensitive environment names supplied to each child, + redact their values in streaming log writers and returned tails, minimize the + inherited environment where protocol-compatible, and preserve useful bounded + diagnostics without copying raw secret material into manifests. +- Test changes: cover override and inherited sensitive values in stdout, + stderr, on-disk logs, and returned errors; assert non-sensitive diagnostics + remain readable. Avoid real credentials in fixtures. +- Dependencies: `RSK-010` owns safe secret-file acquisition; this finding owns + propagation after values enter the process. `RSK-004` owns broad file modes, + not the prohibited content itself. + +### `RSK-013`: ordinary subprocess output validation is unbounded and follows links + +- Category: confirmed correctness/availability risk. +- Locations/invariant: Audita `validateProcessedOutput`/`validateJSONFile`, + Seriatim `validateJSONFile`/`validateTranscriptFile`/render validation, + Scriptorium output checks, and `internal/contracts` bounds/transcript reads. + External output must be a bounded regular result at the requested path before + a stage trusts or materializes it. +- Evidence: these validators use unbounded `os.ReadFile` or link-following + `os.Stat`. They do not reject a symlink before parsing, and most do not first + establish a regular-file handle. A malformed or oversized result can allocate + until memory exhaustion; a symlink to an existing valid JSON/text file can + satisfy validation and be copied as the stage output. Notarius already uses + `Lstat`, same-file checks, regular-file enforcement, and explicit size limits, + while WhisperX bounds responses to 10 MiB, demonstrating a compatible local + boundary pattern. +- Realistic scenario: a faulty external CLI writes a multi-gigabyte JSON result + or leaves the requested output as a symlink to stale data. It exits zero; + Narratio then exhausts memory or records unrelated bytes as a successful + canonical transcript/artifact. +- Impact/likelihood/confidence: high availability/integrity impact; low-to- + moderate faulty-tool or filesystem-reuse likelihood; high confidence in the + validators, with appropriate limits requiring contract decisions. +- Estimated remediation scope and owner: medium shared read-mechanism plus + adapter-specific schema policy. Open without following links where supported, + require a regular file, enforce documented per-contract limits, parse from + the established handle, and leave semantic schema checks in each adapter. +- Test changes: add one shared table for symlink/non-regular/oversized reads and + focused adapter cases proving semantic errors retain their context. Avoid + duplicating the same large fixture for every JSON validator. +- Dependencies: coordinate no-follow mechanics with `COR-003` and size policy + with Stage 8/10 consumers. Do not weaken Notarius's stricter bundle boundary. + +### `RSK-014`: S3 listing has no continuation-token progress guard + +- Category: confirmed availability/resource risk. +- Locations/invariant: `internal/adapters/storage.S3Backend.List`. Pagination + must either make observable progress, finish, fail, or honor cancellation + without unbounded duplicate accumulation. +- Evidence: the loop repeats while `IsTruncated` is true and a next token is + non-nil, assigning that token without comparing it with the prior token. A + provider that repeats one token returns the same page indefinitely. The + production callers generally have no operation-specific deadline, and the + storage fake/test returns only one page and ignores continuation behavior. +- Realistic scenario: an S3-compatible endpoint emits a malformed truncated + response with a repeated token. Session discovery, prepare, restore, status, + or cleanup loops at full request rate and appends duplicate objects until an + operator cancels or the process exhausts memory. +- Impact/likelihood/confidence: high availability and possible request-cost + impact; low provider-fault likelihood; high confidence in loop behavior. +- Estimated remediation scope and owner: small storage-adapter correction. + Reject an empty or repeated next token on a truncated response with contextual + provider/protocol error; retain caller-context cancellation and caller-owned + ordering. +- Test changes: use a stateful S3 fake for two valid pages, repeated/empty token, + later-page error, cancellation, and deterministic normalized aggregation. +- Dependencies: snapshot/generation authority remains `COR-008`; this finding + concerns completion of one list operation only. + ### `EFF-001`: restore repeatedly downloads the same objects during planning and execution - Category: confirmed efficiency and clarity issue. @@ -1761,6 +2046,69 @@ architectural findings. - Dependencies: solve with `COR-008` so caching a plan download cannot install a stale generation. Stage 7 owns adapter metadata/cost tradeoffs. +### `EFF-002`: WhisperX buffers each complete multipart upload in memory + +- Category: confirmed efficiency/resource-use issue. +- Locations/invariant: `internal/adapters/whisperx.doTranscribeAttempt` and the + transcribe stage's configured concurrent use of `Client.Transcribe`. Upload + memory should remain bounded independently of aggregate audio size. +- Evidence: every attempt copies the complete audio file into a `bytes.Buffer`, + adds the remaining multipart fields, and only then constructs and sends the + request. The copy does not observe context cancellation. Concurrent speakers + therefore retain roughly one full audio file each in memory, and every retry + repeats the allocation/copy before network I/O. Response memory is separately + and correctly capped at 10 MiB. +- Realistic scenario: several long speaker tracks are transcribed with the + configured worker concurrency. Narratio allocates their aggregate size at + once and can be killed for memory pressure before the HTTP server receives a + byte; canceling during a large local copy does not stop that work promptly. +- Impact/likelihood/confidence: medium-to-high memory/operability impact that + scales with ordinary media size and concurrency; high likelihood on long + sessions; high confidence from the request construction path. +- Estimated remediation scope and owner: medium WhisperX adapter change. Stream + multipart content from a per-attempt reopenable audio source, propagate copy + errors and cancellation through the request body, and preserve replay across + retries without sharing a consumed reader. +- Test changes: use a blocking/counting reader or transport to prove the request + begins before the complete source is buffered, cancellation interrupts body + production, retries reopen cleanly, and response/output bounds remain intact. +- Dependencies: Stage 8 owns the worker concurrency limit and stable result + ordering; streaming should make each worker bounded rather than changing that + stage policy. + +### `ARC-004`: notification configuration has no production transport consumer + +- Category: confirmed architectural/operator-boundary defect. +- Locations/invariant: `internal/config.NotificationConfig`, pipeline examples + and configuration reference, `internal/app/runner.go` notifier composition, + and `internal/adapters/notify`. Accepted operator settings must either select + implemented behavior or be rejected/described as reserved. +- Evidence: backend, recipient, and timeout are accepted and the timeout is + parsed, but composition always installs `notify.NoopSender` when no test + collaborator is injected. The adapter package contains only no-op and fake + senders and has no mapping for those three fields. The notify stage can thus + succeed with placeholder metadata regardless of configured recipient. Only + the internal overview calls it a placeholder; the public config table and + annotated example present ordinary optional settings. +- Realistic scenario: an operator configures a backend and recipient, observes + a succeeded final stage, and assumes a completion or failure notice was + delivered when no external call occurred. +- Impact/likelihood/confidence: high operator-expectation impact; moderate + likelihood because the fields are publicly surfaced; high confidence from + complete composition and adapter inventory. +- Estimated remediation scope and owner: small config/documentation change if + delivery remains deferred, or medium integration/composition work if delivery + is required. The smallest safe correction is to reject non-empty + backend/recipient values and label/remove reserved settings until a canonical + notification integration contract exists; do not silently map them in the + stage. +- Test changes: assert non-placeholder settings cannot validate while no + transport exists, or, after a transport is specified, add adapter contract + tests for timeout/cancellation/error adaptation and an assembled composition + test proving selection. Preserve the no-op path only when explicitly chosen. +- Dependencies: Stage 8 owns the notify stage's ordinary-success lifecycle and + `ARC-002` vocabulary, not transport selection. + ## Candidate Register The remaining candidates require inspection by their named owners. Completed @@ -1823,22 +2171,26 @@ register above. or derivation from loaded pointer identity. Any change must preserve the protocol correction selected for `COR-004`; do not add a post-pointer upload. -### `ARC-004`: notification configuration has no production transport consumer +### `ARC-005`: Audita request and constructor both advertise ownership of static settings - Category: architectural boundary/ownership candidate. -- Evidence: pipeline config and the annotated example expose notification - backend, recipient, and timeout settings, and validation parses the timeout. - Production composition always supplies `notify.NoopSender`; the adapter package - contains no configured production transport. The internal overview calls the - stage a placeholder/default no-op, while public config surfaces do not make - that limitation equally explicit. -- Realistic risk: an operator configures a recipient and sees a successful notify - stage without any delivery, or future code adds a transport while duplicating - validation/composition policy in the stage. -- Confirmation owner: Stage 7 should establish the notifier adapter contract and - decide whether to construct a configured transport, reject non-placeholder - values, or explicitly document/remove inert fields. Stage 8 owns ordinary - notify-stage outcome semantics, not transport construction. +- Evidence: `PolishRequest` and its integration document carry base URL, model, + transcript description, config path, output schema, work-dir retention, and + concurrency values. `SubprocessRunner.Run` ignores those request fields and + builds arguments, generated config, and metadata from constructor state; + only request `Modules` can override the configured list. The polish stage + currently copies the same config values into both places, so production + behavior agrees by convention. The fake captures the request and does not + reveal that the real runner ignores most of it. +- Realistic risk: a caller or focused stage test supplies a per-request setting + and observes it in the fake, while the production runner silently uses its + older constructor value. Future changes may update one representation only + and make provenance disagree with the apparent request contract. +- Confirmation owners: Stage 8 should decide whether polish needs per-invocation + overrides; Stage 11 should then remove the redundant request fields or make + them authoritative. Stage 12 should align the fake with the chosen contract. + Do not merge genuinely request-specific paths/modules into static runner + construction. ### `TST-001`: full race baseline fails in the concurrent transcribe test @@ -1849,9 +2201,14 @@ register above. - Observed impact: the canonical full race command exits nonzero, weakening its signal for other packages. The report currently points to a test fake, not a production data race. -- Confirmation owners: Stage 8 should inspect the worker/fake contract; Stage - 12 should classify suite impact and the smallest durable fix. Do not change - the fake during this investigative stage. +- Stage 7 refinement: the production HTTP client is stateless during requests + and passes its focused race suite. The fake appends to `Requests` without + synchronization, while the transcribe stage is contractually allowed to call + the client concurrently. The defect is therefore in fake fidelity at the + adapter/consumer seam, not evidence of a production HTTP-client race. +- Confirmation owners: Stage 8 should verify worker cancellation/result + behavior; Stage 12 should classify suite impact and the smallest durable fix. + Do not change the fake during this investigative stage. ### `TST-002`: runner tests cannot exercise invocation-manifest save failures @@ -1937,6 +2294,24 @@ register above. narrow shared predicate. Do not collapse identifier, filesystem-root, or artifact relationship policy into one generic validator. +### `DUP-005`: adapter packages repeat atomic byte-write mechanics + +- Category: duplicated mechanism candidate. +- Evidence: `internal/adapters/subprocess.WriteFileAtomic` and WhisperX's + unexported `writeFileAtomic` independently implement same-directory temp-file + creation, write, file sync, close, chmod, rename, and failure cleanup. Fakes + and generated-YAML writers use the subprocess copy. `internal/fileops` owns a + third equivalent mechanism for application files. All three omit the + directory sync identified by `RSK-002`. +- Realistic risk: the durability or symlink-safe destination repair reaches the + canonical fileops path but leaves adapter outputs/configuration with weaker + guarantees, or adapter copies drift in permissions and cleanup behavior. +- Confirmation owner: Stage 11 should compare exact context, error, permission, + and dependency constraints and select one narrow mechanics owner. Keep YAML + serialization and protocol output validation local; coordinate with + `DUP-001`, `DUP-003`, and `RSK-002` rather than creating another utilities + package. + ### `TST-003`: filesystem safety tests omit destination and lock-recovery boundaries - Category: test-suite sufficiency candidate. @@ -2013,6 +2388,29 @@ register above. and either reject differing configs, always rebind to the resolved config, or replace the dual input with a constructor that makes ownership singular. +### `TST-007`: adapter tests omit adversarial liveness and output-trust boundaries + +- Category: test-suite sufficiency candidate. +- Evidence: focused adapter tests cover successful invocations, direct-child + timeout, override-value tail redaction, HTTP retry/status/cancellation and + malformed JSON, ordinary S3 not-found, and normal/invalid subprocess outputs. + They do not cover descendant termination, inherited-secret or on-disk log + redaction, non-HTTP URL schemes, streaming/cancelable request production, + symlink/non-regular/oversized outputs, response-body close observation, or + valid and non-progressing multi-page S3 responses. Fakes generally append + requests without synchronization and materialize valid placeholder outputs; + only the WhisperX fake is currently called concurrently in production-shaped + tests. +- Realistic risk: fixes for `COR-016`, `RSK-011` through `RSK-014`, and + `EFF-002` can be incomplete while broad adapter coverage remains green, and a + fake can make a stage appear to validate output that the real adapter would + reject or race under the real concurrency contract. +- Confirmation owner: Stage 12 should add the smallest boundary test per root + risk, share process/file/HTTP fixtures where the behavior is mechanical, and + retain protocol-specific argument/schema assertions. It should classify + automatic fake materialization by the stage risks it protects rather than + mechanically rewriting every fake. + ### `SIM-001`: runner terminalization and persistence ordering lack a narrow owner - Category: simplification candidate. @@ -2060,6 +2458,19 @@ register above. behavior precisely; change code only if a later lifecycle decision establishes different intended semantics. +### `COM-003`: Audita's adapter contract still says its real adapter is unimplemented + +- Category: stale comment candidate. +- Evidence: `internal/adapters/audita/runner.go` carries a TODO to implement a + real subprocess/service adapter, while `SubprocessRunner` is production + composed, documented, and covered by extensive invocation tests. +- Realistic risk: a maintainer treats the adapter as placeholder-only, bypasses + the existing implementation, or leaves genuinely missing notification work + confused with already completed Audita work. +- Confirmation owner: Stage 11 should remove the stale TODO and, only if useful, + replace it with rationale about static constructor settings versus request + settings after `ARC-005` is resolved. No production behavior change is needed. + ## Candidate Classification Log | Candidate signal | Classification | Reason | @@ -2093,9 +2504,16 @@ register above. | All adapter constructors are unconditional expensive/external work | rejected at Stage 6 | Default HTTP/subprocess wrappers do no connection or process work at construction. Notarius, object storage, and remote locks are conditional on selected behavior that needs them. | | Repeated adapter constructors and single-stage commands require immediate consolidation | rejected as standalone Stage 6 findings | Constructors translate distinct protocols, and single-stage dispatch already funnels through `runSingleStageCommand`. Similar shape does not establish duplicated policy. | | Repeated filesystem secret scans are an efficiency defect | rejected as material at Stage 6 | Object-store construction can repeat a deterministic bounded directory scan, but the scan is small, preserves secret-before-adapter ordering, and no material latency or external cost was established. | -| Notification settings are consumed because notify succeeds | retained as `ARC-004` | Production success is supplied by a no-op sender regardless of backend/recipient. Stage 7 must decide whether this is an intentional placeholder contract or an inert operator surface. | +| Notification settings are consumed because notify succeeds | confirmed as `ARC-004` at Stage 7 | Production success is supplied by a no-op sender regardless of backend/recipient; no transport contract or composition path consumes the accepted public fields. Non-placeholder values must be rejected/reserved until a real integration exists, or a transport must be specified and composed. | | Injectable runner `Env` always represents the resolved config | retained as `TST-006` | Production composition does, but a non-nil injected `Env.Config` is retained and can differ from the explicit config used by other runner setup. | | Configuration relative-path validation is distinct from shared lexical safety | retained as `DUP-004` | Config adds field context, but its cleaned relative-path language overlaps `pathsafe`; Stage 11 must compare semantics before centralizing mechanics. | +| `exec.CommandContext` bounds a whole external-tool process tree | confirmed as `RSK-011` | The audited toolchain kills only `cmd.Process`; Narratio establishes no process group/job or descendant cleanup. | +| Sensitive override-tail redaction makes subprocess diagnostics secret-safe | confirmed as `RSK-012` | Raw logs are unfiltered and inherited sensitive values are absent from the tail redaction set, so the architecture invariant does not hold. | +| Successful exit plus JSON/non-empty validation establishes a safe subprocess result | confirmed as `RSK-013` | Ordinary adapters use unbounded, link-following reads/stats; Notarius's bounded regular-file checks show the stronger boundary is both necessary and locally expressible. | +| A non-nil S3 continuation token guarantees pagination progress | confirmed as `RSK-014` | The adapter never compares tokens, so a malformed repeated token produces unbounded requests and duplicate accumulation. | +| Similar subprocess adapter argument builders should be consolidated | rejected at Stage 7 | The shared launcher already owns common resource/process mechanics; protocol flags, schemas, exit mapping, and validation differ materially and remain clearer in their adapters. Only atomic byte writing is retained as `DUP-005`. | +| Audita request fields are authoritative per invocation | retained as `ARC-005` | The real runner uses constructor values for most duplicated fields while the stage passes equal values by convention and the fake captures the request; Stages 8, 11, and 12 own the contract decision. | +| Adapter fake request slices all require synchronization now | rejected as a blanket Stage 7 finding | Only WhisperX is currently used concurrently by production-shaped stage behavior and is refined under `TST-001`; other fakes are sequential today. Stage 12 should assess future fidelity from actual consumers. | ## Unresolved Questions And Follow-Up @@ -2145,9 +2563,16 @@ register above. no-follow regular-file reads? - Should remote session loading retain bytes until canonical preparation or expose an owned temporary handle whose cleanup spans every command consumer? -- Is notification configuration intentionally reserved while delivery stays a - no-op, or should non-placeholder values fail validation until Stage 7 defines - a production transport? +- Is notification delivery a product requirement, and if so which provider + protocol should become the canonical integration contract? Until that is + answered, `ARC-004` requires non-placeholder settings to fail explicitly. +- Which supported-platform process-tree primitive can give `RSK-011` equivalent + termination and wait semantics on Linux, macOS, and Windows? +- Which per-contract maximum sizes are operationally valid for Seriatim, + Audita, Scriptorium, and bounds outputs under `RSK-013`? +- Can subprocess environments be reduced to explicit allowlists, or must + `RSK-012` retain broad inheritance and stream-redact every known sensitive + value? - Which native CI runner limitations explain the absence of validation jobs in the tag-only release workflow? Stage 12 owns the automation conclusion. @@ -2364,11 +2789,62 @@ No accepted risks or final audit conclusions are recorded yet. false/zero; explicit empty modules remain distinct from omission; Notarius paths are anchored to the pipeline; external work is conditionally composed; default client/runner construction opens no closeable resource; secret values - are not persisted or logged; maintained examples use non-secret placeholders - and are executable under representative sessions. Repeated bounded secret - scans and protocol-specific constructors do not establish material efficiency - or duplication defects. + are not directly persisted or logged by config/composition code; Stage 7 + later found that child-produced logs and inherited-secret error tails violate + the broader invariant under `RSK-012`. Maintained examples use non-secret + placeholders and are executable under representative sessions. Repeated + bounded secret scans and protocol-specific constructors do not establish + material efficiency or duplication defects. - Documentation/example disposition: config defaults and maintained example structure match implementation. Notification backend/recipient settings are the material drift because production always uses a no-op sender; `ARC-004` assigns the transport/documentation decision to Stage 7. + +### Stage 7 + +- Entry revision: `0920062` (`Document configuration and composition audit + findings`). Commits since the pinned audit revision modify audit + documentation only, so implementation/test evidence remains pinned to the + identity recorded above. +- Contracts and code reviewed: development guide, audit sequence, all repository + policy, adapter/internal/storage/audio documentation, and every production + file under `internal/adapters`, `internal/audio`, `internal/logging`, + `internal/contracts`, and `internal/artifactmodel`; production stage/app + callers were traced for each boundary. +- Graph/source evidence: scoped architectures, symbol inventory, caller/callee + and code searches for every adapter interface and external operation, then + exact source inspection of HTTP request/retry handling, shared subprocess + launch and diagnostics, all subprocess argument/config/result adapters, S3 + pagination/body/file handling, audio temporary installation, notification + composition, fakes, shared models, and focused tests. Local Go 1.26.5 + `CommandContext` documentation/source confirmed direct-process kill behavior. +- Validation: `go test -count=1 ./internal/adapters/... ./internal/audio + ./internal/logging ./internal/contracts ./internal/artifactmodel` passed (all + 13 package results passed; 1.62 s command wall time). + `go test -race -count=1 ./internal/adapters/... ./internal/audio` passed (all + 10 package results passed; 21.61 s command wall time). No live service, + credential, destructive, or paid operation was exercised. +- Resource conclusions: every HTTP response, S3 body, opened local file, retry + timer, direct subprocess, log descriptor, and temporary download has an + explicit normal/error release path. Context reaches HTTP/S3 operations and + direct child processes; gaps are multipart pre-copy cancellation + (`EFF-002`), descendant termination (`RSK-011`), and pagination progress + (`RSK-014`). No adapter-owned goroutine/channel or process-level shutdown + resource exists. +- Findings: confirmed `COR-016`, `RSK-011`, `RSK-012`, `RSK-013`, `RSK-014`, + `EFF-002`, and prior candidate `ARC-004`; added `ARC-005`, `DUP-005`, + `COM-003`, and `TST-007`, and refined `TST-001` as a concurrent-fake defect + rather than a production HTTP-client race. +- Explicit no-finding conclusions: transport/SDK/process types and protocol + retry policy do not leak into stages; ordinary response bodies/files/timers + are closed; retry status classes and output install ordering match WhisperX; + Notarius performs bounded regular-file/root/lane validation; arguments and + generated configs are deterministic and contain credential names/presence, + not values; S3 callers own sorting/policy and not-found adaptation is correct; + shared artifact models have stable tags and non-lossy slice conversion; the + logger constructor owns no resource. Protocol-specific adapter builders are + justified rather than a consolidation target. +- Later-stage assignments: Stage 8 owns transcribe worker/fake behavior, + ordinary stage output consumption, notify lifecycle, and Audita override + intent. Stage 11 owns `ARC-005`, `DUP-005`, and `COM-003`; Stage 12 owns the + risk-based additions/consolidation in `TST-001` and `TST-007`.