Document external adapter audit findings
This commit is contained in:
@@ -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`.
|
||||
|
||||
Reference in New Issue
Block a user