From 80be8be4d674e73790bd8318dd13281ffa8d4abb Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 11 Aug 2026 03:11:27 +0000 Subject: [PATCH] Confine subprocess diagnostics and retain redacted tails --- docs/roadmap/implementation.md | 1268 ++++--------------- internal/adapters/subprocess/diagnostics.go | 33 +- internal/adapters/subprocess/run.go | 36 +- internal/adapters/subprocess/run_test.go | 224 ++++ internal/fileops/confined.go | 14 +- internal/fileops/confined_open_test.go | 56 + 6 files changed, 568 insertions(+), 1063 deletions(-) create mode 100644 internal/fileops/confined_open_test.go diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index c6766ce..379d85d 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -3,51 +3,63 @@ ## Purpose and status This document is the executable roadmap for remediating the confirmed findings in -[`audit-findings.md`](audit-findings.md). It is written for a `gpt-5.6-terra` -coding agent that will implement exactly one stage per prompt, in order. +[`audit-findings.md`](audit-findings.md) and the concrete gaps found during the +post-implementation code review. It is written for a `gpt-5.6-terra` coding agent +that will implement exactly one pending stage per prompt, in order. The audit is an immutable requirements and evidence ledger. Do not edit -`audit-findings.md`, reinterpret accepted findings, or copy its detailed analysis -into this document. The line ranges below are stable references to the evidence -and acceptance criteria that the implementing agent must read for each stage. +`audit-findings.md` or reinterpret its accepted findings. Stages 1–32 completed +the original audit-remediation plan and are summarized below instead of retaining +their now-obsolete implementation instructions. The repository history and audit +remain the detailed evidence for that work. -This plan owns remediation progress; a separate `audit.md` tracker is not needed. -All stages begin pending when this plan is created. +Stages 33–35 address follow-up review findings in the completed implementation. +They are implementation gaps in the original remediation rather than new entries +in the immutable audit ledger. They do not require the implementing agent to read +the full audit or any audit line range. -| Stage | Summary | Primary findings | Status | +## Stage status and completed summaries + +| Stage | Outcome or pending work | Source | Status | | ---: | --- | --- | --- | -| 1 | Align data classification and group workspace modes | RSK-004 | Completed | -| 2 | Enforce safe identifiers and fuzz path/source contracts | COR-002, TST-013 | Completed | -| 3 | Consolidate crash-durable atomic file replacement | RSK-002, DUP-001, DUP-005 | Completed | -| 4 | Add confined destination and download/install capabilities | COR-003, DUP-003, TST-003 | Completed | -| 5 | Confine recursive cleanup and replace sentinel locks | RSK-003 | Completed | -| 6 | Harden API-key file acquisition | RSK-010 | Completed | -| 7 | Bound and verify external result acquisition | RSK-013, TST-007 | Completed | -| 8 | Terminate owned subprocess trees | RSK-011 | Completed | -| 9 | Redact and cap subprocess diagnostics | RSK-012 | Completed | -| 10 | Confine publish archive reads | COR-005 | Completed | -| 11 | Make manifest and run identity singular | COR-001, TST-006 | Completed | -| 12 | Centralize handled terminal-failure persistence | RSK-001, TST-002, SIM-001, COM-001 | Completed | -| 13 | Introduce the immutable remote-commit model and legacy boundary | ARC-003 | Completed | -| 14 | Publish through immutable commits and canonical mappings | COR-004, COR-011, DUP-002, TST-004 | Completed | -| 15 | Make remote locks generation-safe and harden pagination | RSK-005, RSK-014 | Completed | -| 16 | Persist retryable post-commit cleanup state | COR-006, COR-007 | Completed | -| 17 | Bind restore/status to a committed snapshot and reject conflicts | COR-008, COR-009, TST-005 | Completed | -| 18 | Serialize restore transitions and make restored paths portable | RSK-006, RSK-008 | Completed | -| 19 | Bind audio cache reuse to remote object identity | RSK-007 | Completed | -| 20 | Unify previous-source readiness and eliminate duplicate transfers | COR-010, EFF-001 | Completed | -| 21 | Tighten configuration parsing, values, and expectations | COR-012–COR-015, TST-011, TST-014 | Completed | -| 22 | Make product configuration truthful and own remote temp files | COR-024, RSK-009, ARC-004 | Completed | -| 23 | Stream WhisperX uploads and make the adapter race-safe | COR-016, EFF-002, TST-001 | Completed | -| 24 | Correct prepare/transcribe transition semantics | COR-017–COR-019, TST-008 | Completed | -| 25 | Enforce output-path authority and shared singleton resolution | ARC-006, DUP-006 | Completed | -| 26 | Centralize typed extraction-bundle evidence | DUP-007 | Completed | -| 27 | Bind extraction reuse to direct transcript identity | COR-020, TST-009 | Completed | -| 28 | Establish one effective artifact set and catalog bootstrap | COR-022, ARC-007, DUP-008 | Completed | -| 29 | Make analyze resolution typed, optional, actionable, and deterministic | COR-021, COR-023, RSK-015, SIM-003, TST-010 | Completed | -| 30 | Remove misleading contracts and align Audita ownership | ARC-001, ARC-005, SIM-004, COM-003, COM-006 | Completed | -| 31 | Enforce CI validation and streamline the assembled test suite | TST-012, TST-015 | Completed | -| 32 | Reconcile lifecycle/analyze documentation and close the remediation | COM-002, COM-005 | Completed | +| 1 | Aligned architecture policy and filesystem modes with collaborative, group-owned workspaces while preserving private API-key handling. | RSK-004 | Completed | +| 2 | Enforced portable safe-segment identifiers and added path/source fuzz coverage. | COR-002, TST-013 | Completed | +| 3 | Consolidated crash-durable atomic file replacement behind shared file-operation primitives. | RSK-002, DUP-001, DUP-005 | Completed | +| 4 | Added confined, no-follow destination and download/install operations. | COR-003, DUP-003, TST-003 | Completed | +| 5 | Confined recursive cleanup and replaced existence-based sentinel locks with OS-held locks. | RSK-003 | Completed | +| 6 | Made API-key acquisition private, bounded, regular-file-only, and no-follow. | RSK-010 | Completed | +| 7 | Bounded and verified externally produced adapter and stage result files. | RSK-013, TST-007 | Completed | +| 8 | Added owned subprocess-tree cancellation, timeout, and forced termination. | RSK-011 | Completed | +| 9 | Added streaming secret redaction and bounded subprocess diagnostic capture. | RSK-012 | Completed | +| 10 | Confined publish archive reads to validated workspace sources. | COR-005 | Completed | +| 11 | Established singular manifest and run identity ownership. | COR-001, TST-006 | Completed | +| 12 | Centralized handled terminal-failure persistence and removed competing failure paths. | RSK-001, TST-002, SIM-001, COM-001 | Completed | +| 13 | Introduced immutable remote commit manifests, a final current pointer, and an isolated removable legacy reader. | ARC-003 | Completed | +| 14 | Switched publishing to immutable commits, deterministic mappings, and a final pointer update. | COR-004, COR-011, DUP-002, TST-004 | Completed | +| 15 | Made remote locks generation-safe and rejected non-progressing storage pagination. | RSK-005, RSK-014 | Completed | +| 16 | Persisted retryable post-commit cleanup obligations. | COR-006, COR-007 | Completed | +| 17 | Bound restore and status to one verified committed snapshot and rejected conflicting state. | COR-008, COR-009, TST-005 | Completed | +| 18 | Serialized restore transitions, persisted interrupted-restore state, and made restored paths portable. | RSK-006, RSK-008 | Completed | +| 19 | Bound reusable audio cache entries to remote object identity. | RSK-007 | Completed | +| 20 | Unified previous-source readiness and eliminated duplicate transfers. | COR-010, EFF-001 | Completed | +| 21 | Tightened configuration parsing, validation, value domains, and test expectations. | COR-012–COR-015, TST-011, TST-014 | Completed | +| 22 | Made product configuration truthful and assigned lifecycle ownership for downloaded temporary files. | COR-024, RSK-009, ARC-004 | Completed | +| 23 | Streamed WhisperX uploads and made its transport bounded, retry-correct, and race-safe. | COR-016, EFF-002, TST-001 | Completed | +| 24 | Corrected prepare/transcribe transition, retry, and optional-output semantics. | COR-017–COR-019, TST-008 | Completed | +| 25 | Enforced authoritative output paths and shared singleton resolution. | ARC-006, DUP-006 | Completed | +| 26 | Centralized typed extraction-bundle evidence. | DUP-007 | Completed | +| 27 | Bound extraction reuse to direct transcript identity and explicit freshness evidence. | COR-020, TST-009 | Completed | +| 28 | Established one effective artifact set and deterministic catalog bootstrap behavior. | COR-022, ARC-007, DUP-008 | Completed | +| 29 | Made analyze input resolution typed, optional, actionable, and deterministic. | COR-021, COR-023, RSK-015, SIM-003, TST-010 | Completed | +| 30 | Removed misleading contracts and placed static Audita policy in adapter construction. | ARC-001, ARC-005, SIM-004, COM-003, COM-006 | Completed | +| 31 | Enforced repository-wide CI/release validation and streamlined redundant test matrices. | TST-012, TST-015 | Completed | +| 32 | Reconciled lifecycle/analyze documentation and closed the original audit traceability inventory. | COM-002, COM-005 | Completed | +| 33 | Confine diagnostic log destinations and eliminate pathname-based tail reads. | Follow-up review | Completed | +| 34 | Dispose of owned subprocess descendants after natural leader exit. | Follow-up review | Pending | +| 35 | Bound remote current-state and lock control-plane reads. | Follow-up review | Pending | + +Completed stages must not be reimplemented wholesale. A pending stage may adjust +their code only where its stated remediation requires it. ## Governing decisions @@ -66,41 +78,52 @@ implementing agent: the platform's available guarantees and explicit operational guidance. 3. Remote current-state publication uses an immutable run-scoped commit manifest selected by one final pointer update. Readers may accept coherent legacy state, - but all compatibility code must be isolated behind a clearly named boundary, - covered by dedicated tests, and carry an in-code removal comment. New writers - must never produce the legacy format. + but all compatibility code must remain isolated behind a clearly named + boundary, covered by dedicated tests, and carry an in-code removal comment. + New writers must never produce the legacy format. 4. Explicit notification no-op is supported. Configured notification backends or recipients are rejected until a real provider exists. -5. External-output limits are runaway safeguards, not normal operating limits. - Each limit must be a named, centrally discoverable package constant and at - least an order of magnitude above expected ordinary output. Every limit error - must identify the enforcing adapter/contract, the configured byte limit, and - what exceeded it. Narratio-owned stdout/stderr capture is streaming and capped; - reaching its cap terminates the owned process tree. Hard whole-disk protection - is an operations concern implemented with filesystem/service/container/volume - quotas, not output-root polling. -6. Retain every accepted-risk disposition in the audit. In particular, do not - turn abrupt process-death detection into a distributed liveness system, do not - add rollback of arbitrary external side effects, and do not add generic - speculative abstractions rejected by the audit. -7. CI must run ordinary tests, vet, build, and example checks on every change and - release path; the complete race suite runs on every change once TST-001 is - repaired; shuffled tests run on a schedule. Add native macOS/Windows jobs only - when authoritative Woodpecker runner labels and usable runners are available. - Do not invent labels. Cross-compilation is useful but does not count as native - filesystem/process evidence. +5. External-data limits are runaway safeguards, not normal operating limits. + Each limit must be a named, centrally discoverable constant in the package that + owns the contract and at least an order of magnitude above expected ordinary + data. Every limit error must identify the enforcing adapter or contract, the + configured byte limit, and what exceeded it. Hard whole-disk protection remains + an operations concern implemented with filesystem, service, container, or + volume quotas rather than output-root polling. +6. Narratio owns the complete process group or job created for every external + command. Leader completion does not transfer ownership of surviving + descendants. `Run` must not return while an owned descendant can continue + running, including when the leader exits successfully or closes its streams. +7. Diagnostic tails are derived only from content that passed through the + streaming redactor. Diagnostic files use the same confined, no-follow mutation + guarantees as other workspace destinations; a pathname must not be reopened to + obtain an error tail after execution. +8. Remote current pointers, commit manifests, selected session manifests, and + lock documents are bounded while being read. Object metadata may reject an + oversized value early but cannot replace enforcement against the actual byte + stream. Limits for these small control-plane contracts are separate from limits + for intentionally large artifact payloads. +9. Retain every accepted-risk disposition in the audit. In particular, do not + turn abrupt process-death detection into a distributed liveness system, add + rollback of arbitrary external side effects, or introduce generic speculative + abstractions rejected by the audit. +10. CI must run ordinary tests, the full race suite, vet, build, and example/docs + checks on every change and release path; shuffled tests run on a schedule. Add + native macOS/Windows jobs only when authoritative Woodpecker runner labels and + usable runners are available. Do not invent labels. Cross-compilation is + useful but does not count as native filesystem or process evidence. -## Instructions for every stage +## Instructions for every pending stage For each implementation prompt, the coding agent must: -1. Read `docs/development.md`, then the exact audit ranges and canonical documents - named by that stage. Follow the task-specific reading guide in - `docs/development.md`. Do not read all 4,000+ audit lines unless a stage - explicitly requires them. +1. Read `docs/development.md`, then the current-behavior documents and source + files named by that stage. Follow the task-specific reading guide in + `docs/development.md`. Stages 33–35 do not require reading + `audit-findings.md`; do not reread or modify the immutable audit ledger. 2. Inspect the current tree before editing. Earlier stages may have changed names and ownership boundaries. Use the repository knowledge graph first for code - discovery and call tracing, then use text search for documentation, string + discovery and call tracing, then text search for documentation, string literals, configuration, and evidence the graph cannot supply. 3. Confirm the worktree state and preserve unrelated user changes. Do not rewrite or amend earlier commits unless explicitly instructed. @@ -109,986 +132,177 @@ For each implementation prompt, the coding agent must: documentation, and removal of code made obsolete by the stage. Do not leave a second competing path or defer required tests to a later stage unless this plan explicitly says so. -5. Prefer typed, owner-specific contracts and existing package ownership. Do not - create a generic abstraction merely because two call sites look similar. Keep - policy at the owner and mechanism in `internal/fileops` or the relevant - adapter/package. -6. Treat path/race/security checks as production behavior. Validation followed by - an unconfined pathname operation is not a completed fix. Unsupported safe - behavior must fail closed with an actionable error. -7. Run focused tests while developing, then at minimum `go test ./...` for every - stage. For production-code changes also run `go vet ./...`, `go build ./...`, - and the repository's documentation/example validation command. Run targeted - `-race` tests for concurrency changes and platform build/test checks named by - the stage. Do not require live cloud or third-party services. -8. Compare the final diff with the stage's finding list and exit criteria. Update - only that stage's row in the table from `Pending` to `Completed`, adding the +5. Prefer typed, owner-specific contracts and existing package ownership. Keep + policy at the owner and reusable mechanism in `internal/fileops`, storage, or + the relevant adapter package. Do not create a generic abstraction solely + because two call sites look similar. +6. Treat path, race, process-lifecycle, and size checks as production behavior. + Validation followed by an unsafe pathname operation or unbounded read is not a + completed fix. Unsupported safe behavior must fail closed with an actionable + error. +7. Run focused tests while developing, then run at minimum: + + ```sh + go test ./... + go test -race ./... + go vet ./... + go build ./... + go test ./internal/doccheck + go test ./internal/config -run '^TestExamplesLoadAndValidate$' + ``` + + Run the platform build/test checks named by the stage. Do not require live + cloud or third-party services. +8. Compare the final diff with the stage goal and exit criteria. Update only that + stage's row in the status table from `Pending` to `Completed`, adding the implementing commit hash if the workflow supplies one. Do not mark a stage - complete while a required check is failing or a required behavior is missing. -9. Leave `docs/roadmap/audit-findings.md` unchanged. If implementation evidence - reveals that a confirmed finding is factually impossible to satisfy as written, - stop and report the contradiction instead of silently changing its meaning. + complete while a required check is failing or required behavior is missing. -## Stage 1 — Align data classification and group workspace modes +## Stage 33 — Confine diagnostic log destinations and eliminate pathname-based tail reads -**Read first:** `audit-findings.md` lines 2660–2693 (RSK-004), 4171–4224 -(resolved decisions and accepted risks); `docs/policy/architecture.md`; -`docs/operations.md`; and the workspace/artifact/file-operation package docs named -by `docs/development.md`. +**Read first:** `docs/policy/architecture.md`, `docs/policy/testing.md`, +`docs/internal/adapters.md`, `docs/internal/workspace.md`, and the current +subprocess diagnostic and confined file-operation implementations and tests. -**Goal:** Make the architecture and implementation consistently describe and -support shareable, group-owned workspaces while keeping API keys outside that -ordinary-data policy. +**Depends on:** Completed Stages 3, 4, and 9. + +**Goal:** Prevent diagnostic log setup from following a replaceable leaf symlink +or mutating a file outside the intended workspace, and ensure error diagnostics +cannot be redirected to read unrelated or secret content after a command runs. **Work:** -- Revise the architecture policy's current-behavior contract: campaign/session, - manifests, transcripts, generated configs, logs, and Notarius artifacts are - intentionally shareable; API-key material is sensitive. -- Identify every Narratio-owned ordinary workspace creation/promotion path. Give - the owning packages named, discoverable mode constants and converge final POSIX - directories/files to setgid `02775`/`0664`, without changing ownership. -- Preserve the parent workspace group through nested creation and atomic - replacement. Account for restrictive caller umasks explicitly; do not rely on a - favorable umask to add group write or setgid bits. -- Keep temporary-file exposure no broader than the intended final object and do - not weaken the separate API-key rules implemented in Stage 6. -- Document Windows ACL/ownership expectations and any mode-bit limitations; do - not claim POSIX mode semantics on Windows. - -**Tests and exit criteria:** Add focused tests under restrictive umasks for nested -directories, final files, promoted Notarius trees, and repeated replacement. -Verify that group-write/setgid behavior is correct on POSIX and platform behavior -is explicit elsewhere. The architecture, operations, and security statements must -agree. No test should assert that ordinary campaign output is private. - -## Stage 2 — Enforce safe identifiers and fuzz path/source contracts - -**Read first:** `audit-findings.md` lines 1770–1810 (COR-002) and 3837–3852 -(TST-013); configuration, manifest, workspace, artifact, and source-grammar docs. - -**Depends on:** Stage 1. - -**Goal:** Prevent externally influenced identity tokens from becoming absolute, -traversing, platform-ambiguous, or control-bearing filesystem segments. - -**Work:** - -- Define one strict portable segment grammar for campaign IDs, session IDs, run - IDs, artifact identifiers, and other opaque filesystem identity tokens: ASCII - letters, digits, `.`, `_`, and `-` only; reject empty, `.`/`..`, separators, - drive/volume syntax, absolute forms, control characters, and all other bytes. -- Enforce the contract at configuration/model construction and again at the - artifact/path boundary. Return actionable errors; never sanitize or silently - rewrite identity. -- Audit manifest/source parsing for equivalent tokens. Unsafe legacy values must - fail with migration guidance, not be accepted because an older file contains - them. -- Preserve intentional multi-segment paths only in typed path contracts that are - separately confined; do not force path-valued fields through the opaque-ID API. - -**Tests and exit criteria:** Add table and fuzz tests for Unix/Windows separators, -drive forms, Unicode lookalikes, controls, empty/dot segments, and valid boundary -values. Include source-token parsers as required by TST-013. Prove all path builders -reject unsafe identifiers before filesystem access. - -## Stage 3 — Consolidate crash-durable atomic file replacement - -**Read first:** `audit-findings.md` lines 2590–2624 (RSK-002), 3407–3427 -(DUP-001), and 3493–3514 (DUP-005); `internal/fileops` and manifest persistence -docs. - -**Depends on:** Stages 1–2. - -**Goal:** Provide one owner-neutral byte-file installation mechanism with explicit -durability semantics, while leaving serialization and policy with callers. - -**Work:** - -- Implement or complete a narrowly scoped `internal/fileops` byte-write/replace - primitive: sibling temporary file, complete write, file sync, atomic rename or - documented platform replacement, then parent-directory sync where supported. -- Accept the caller-owned final mode; make cleanup and error composition explicit. - Preserve cancellation checkpoints in callers before durable commit. -- Migrate duplicate local manifest, artifact, and adapter byte-write helpers to the - primitive. Keep manifest encoding/versioning out of `fileops`. -- Define and test the platform contract for directory sync and replacement. A - platform limitation must be explicit, not silently treated as equivalent. - -**Tests and exit criteria:** Exercise short writes, write/sync/rename/directory-sync -failures through injected seams, repeated replacement, mode preservation, and -temporary cleanup. Existing manifest and adapter behavior must remain compatible. -No duplicate sibling-temp byte writer should remain. - -## Stage 4 — Add confined destination and download/install capabilities - -**Read first:** `audit-findings.md` lines 1811–1852 (COR-003), 3448–3471 -(DUP-003), and 3596–3617 (TST-003); filesystem threat model and `internal/fileops` -docs. - -**Depends on:** Stages 2–3. - -**Goal:** Make destination mutation resistant to ancestor and leaf symlink races, -and share the safe remote-download installation mechanism without sharing caller -policy. - -**Work:** - -- Add root-relative, handle-confined creation, replacement, and promotion - capabilities that verify/traverse every ancestor without following symlinks. - Use platform-specific implementations where necessary and fail closed if the - required guarantee is unavailable. -- Replace validate-then-use pathname mutations in artifact creation, manifest - installation, Notarius tree promotion, restore/download installation, and - related destinations. -- Implement one destination-confined sibling-temp download/install mechanism. - Callers continue to own object identity, verification, force/conflict policy, - and final mode. -- Do not implement recursive deletion here; Stage 5 owns that distinct capability. - -**Tests and exit criteria:** Add adversarial tests that replace intermediate -directories and leaves with symlinks between logical phases, plus regular success, -collision, cleanup, and platform tests. Prove no external target is modified. -TST-003 is primarily owned by this stage and may receive additional cleanup cases -in Stage 5. - -## Stage 5 — Confine recursive cleanup and replace sentinel locks - -**Read first:** `audit-findings.md` lines 1811–1852 (COR-003), 2625–2659 -(RSK-003), and 3596–3617 (TST-003); local locking and workspace lifecycle docs. - -**Depends on:** Stage 4. - -**Goal:** Make deletion root-confined and make local crash recovery rely on -OS-released locks rather than stale file existence. - -**Work:** - -- Add a root-relative, no-follow cleanup/delete capability and migrate recursive - cleanup paths. Never pass an unchecked or re-resolved absolute path to broad - recursive deletion. -- Replace existence-based sentinel locking with a held OS lock whose ownership is - released by the kernel on process death. Retain useful owner/run metadata - without using metadata-file existence as the lock decision. -- Implement explicit Unix and Windows lock backends, wait/contention behavior, - cancellation, and release-error reporting. Do not auto-delete another process's - active lock. -- Ensure lock files and cleanup roots follow the group workspace policy where they - are ordinary runtime data. - -**Tests and exit criteria:** Cover contention across independent processes, -process-death recovery, cancellation, release failures, symlink swaps, malicious -cleanup inputs, and idempotent cleanup. Run native tests where available and at -least cross-build all platform-specific files. - -## Stage 6 — Harden API-key file acquisition - -**Read first:** `audit-findings.md` lines 2870–2900 (RSK-010); architecture, -configuration, and integration docs governing API keys. - -**Depends on:** Stages 1, 2, and 4. - -**Goal:** Make the sole sensitive-data path private, bounded, regular-file-only, -and resistant to path races. - -**Work:** - -- Resolve API-key directories and files through a root-confined, no-follow read - path. Reject symlinks, non-regular files, unsafe relative names, replacement - races, and oversized content. -- Define the small API-key byte limit as a named, discoverable constant in the - owning package. Errors must name the API-key loader and the limit. -- Enforce `0700` directory and `0600` file privacy for Narratio-created secret - storage on POSIX, and reject unsafe existing modes where Narratio reads key - files. Document Windows ownership/ACL requirements without pretending mode bits - are authoritative there. -- Preserve documented environment/file precedence without persisting or logging - secret values. - -**Tests and exit criteria:** Cover mode rejection, symlink/ancestor swaps, -non-regular files, over-limit reads, valid newline trimming, precedence, and error -redaction. No API-key content may appear in failures or fixtures intended for logs. - -## Stage 7 — Bound and verify external result acquisition - -**Read first:** `audit-findings.md` lines 2963–3006 (RSK-013), 3691–3718 -(TST-007), and 3744–3774 (TST-009); adapter contracts and integration docs. - -**Depends on:** Stages 4 and 6. - -**Goal:** Prevent a runaway adapter or producer from forcing unbounded memory or -post-process disk consumption while leaving abundant headroom for real sessions. - -**Work:** - -- Factor the non-secret portions of the Stage 6 read mechanism into an - owner-neutral bounded, regular-file, no-follow acquisition primitive if that - does not already exist. Keep each contract's limit in its owning package. -- Inventory every externally produced file Narratio reads after a process exits: - adapter JSON, Audita output, Scriptorium output, WhisperX result, Notarius lane - result/catalog/bundle evidence, and any equivalent stage handoff. Migrate all of - them to bounded verified acquisition. -- Set generous documented constants at least ten times larger than credible - ordinary output. Do not embed numeric byte limits in call sites or hide them in - a generic helper. -- Every over-limit error must state the responsible adapter or producer contract, - the configured limit, and the file/result category. Never return partial parsed - output. -- Document that hard disk exhaustion is prevented operationally with filesystem, - service, container, or volume quotas; do not add polling of output-root size. - -**Tests and exit criteria:** Add exact-limit and limit-plus-one tests, growth/race -cases, non-regular and symlink cases, malformed/truncated results, and clear error -assertions for each adapter family. TST-007 is primarily owned here; later adapter -stages add their specialized liveness cases. - -## Stage 8 — Terminate owned subprocess trees - -**Read first:** `audit-findings.md` lines 2901–2931 (RSK-011); adapter process and -platform-support docs. - -**Depends on:** Stage 7. - -**Goal:** Ensure cancellation, timeout, and enforced output limits stop and reap -the complete subprocess tree Narratio owns. - -**Work:** - -- Introduce a narrow platform-specific process-runner capability that starts an - owned process in a Unix process group or Windows job object and can terminate - and reap the tree. -- Migrate Notarius, Audita, Scriptorium, WhisperX, and other owned external command - execution to it without broadening environment inheritance. -- Define graceful-then-forceful cancellation behavior, bounded waits, error - composition, and already-exited handling. Ensure descendants cannot retain - pipes indefinitely after the leader exits. - -**Tests and exit criteria:** Use helper processes that spawn descendants and hold -stdout/stderr open. Prove cancellation/timeout kills descendants and reaps the -leader. Add native Unix/Windows tests where runners exist and cross-build all -platform code; record no unsupported fallback as safe. - -## Stage 9 — Redact and cap subprocess diagnostics - -**Read first:** `audit-findings.md` lines 2932–2962 (RSK-012) and 2963–3006 -(RSK-013); logging, adapter, and operations docs. - -**Depends on:** Stages 6–8. - -**Goal:** Make persisted stdout/stderr and diagnostic tails bounded and unable to -disclose known credentials. - -**Work:** - -- Define an explicit set of sensitive environment-variable names and pass their - values only to a streaming redaction layer; never persist the values or include - them in error text. -- Minimize inherited environments for external processes while preserving the - documented variables needed for execution. -- Stream stdout and stderr through redaction into capped diagnostic files using - named, centrally discoverable limits in the owning process/adapter package. - Redaction must work when a secret spans write chunks. Diagnostic tails must be - derived from already-redacted content. -- If either capture limit is reached, terminate and reap the process tree through - Stage 8 and return an unmistakable error naming stdout/stderr, the responsible - adapter, and the byte limit. Do not report successful partial execution. - -**Tests and exit criteria:** Cover exact cap, cap-plus-one, split-token secrets, -overlapping secret values, empty values, tails, cancellation, descendant cleanup, -and on-disk inspection. Assert that neither raw nor transformed error paths expose -test credentials. - -## Stage 10 — Confine publish archive reads - -**Read first:** `audit-findings.md` lines 1891–1924 (COR-005); publish, artifact, -manifest, and storage docs. - -**Depends on:** Stages 2, 4, and 7. - -**Goal:** Ensure publish uploads exactly the verified regular files inside the -selected run root, despite symlink or replacement races. - -**Work:** - -- Enumerate publish candidates through the artifact/run manifest rather than an - untrusted tree walk. -- Open each candidate root-relatively without following ancestors or the leaf; - verify regular-file type and any declared identity immediately on the opened - object, then stream from that verified handle. -- Reject undeclared files, directories, devices, sockets, symlinks, and identity - changes. Preserve deterministic ordering and actionable source-relative errors. -- Keep object-store destination policy out of the filesystem primitive. - -**Tests and exit criteria:** Add regular, leaf-symlink, ancestor-swap, -post-enumeration replacement, non-regular, and undeclared-file cases. A hostile -workspace must never cause bytes outside the run root to be uploaded. - -## Stage 11 — Make manifest and run identity singular - -**Read first:** `audit-findings.md` lines 1731–1769 (COR-001) and 3670–3690 -(TST-006); configuration, environment, run-state, and manifest docs. - -**Depends on:** Stages 2–3. - -**Goal:** Use one validated campaign/session/run identity per invocation and -recompute all run-scoped manifest fields together. - -**Work:** - -- At invocation start, resolve and validate one effective configuration and bind - the runtime environment to it; remove later reads from stale or unvalidated - configuration instances. -- On every run, recompute campaign, session, run ID, source references, and other - run-scoped manifest identity as a coherent set. Do not retain fields from the - prior invocation merely because a manifest object is reused. -- Reject disagreement among configured, CLI-selected, persisted, and manifest - campaign/session identity with an actionable error before side effects. -- Preserve intentionally session-scoped history separately from the current - run-scoped projection. - -**Tests and exit criteria:** Add consecutive-run regression tests using one -process/environment, explicit mismatch tests for every authority pair, and TST-006 -tests proving downstream operations receive the resolved configuration instance. - -## Stage 12 — Centralize handled terminal-failure persistence - -**Read first:** `audit-findings.md` lines 2553–2589 (RSK-001), 3388–3406 -(TST-002), 3886–3906 (SIM-001), and 3980–3997 (COM-001); run/session lifecycle -docs. - -**Depends on:** Stages 3 and 11. - -**Goal:** Ensure every handled command failure terminalizes the authoritative -session record and best-effort diagnostic run record with correct compounded -errors. - -**Work:** - -- Introduce the narrow terminal-failure operation described by SIM-001: persist - the session's terminal failure first, then update the run diagnostic record. - Keep the running transition separate and run-first. -- Route every handled pipeline/stage error and cancellation path through it. - Return an error that preserves the operation failure plus any session/run store - disagreement; do not mask the primary failure or report success. -- Add the short rationale comment required by COM-001 explaining the deliberately - asymmetric two-ledger ordering. -- Keep abrupt process death as the accepted residual risk. Do not add heartbeats or - pretend a stale `running` record proves liveness. - -**Tests and exit criteria:** Inject session-store and run-store failures at each -step, including cancellation and already-terminal records. Assert call order, -persisted truth, and composed errors. Remove duplicate terminalization sequences. - -## Stage 13 — Introduce the immutable remote-commit model and legacy boundary - -**Read first:** `audit-findings.md` lines 1853–1890 (COR-004), 1992–2037 -(COR-008), and 3256–3279 (ARC-003); remote manifest, storage, restore, and -compatibility docs. - -**Depends on:** Stages 2–3 and 11. - -**Goal:** Define one versioned, immutable snapshot contract selected by a small -current pointer, without mixing local metadata or temporary compatibility into the -new model. - -**Work:** - -- Add a typed, versioned run-scoped remote commit manifest whose identity and - declared object set are immutable. Include campaign/session/run identity, - enough version/checksum/generation metadata to bind readers to the selected - snapshot, and an extensible typed source-to-destination artifact mapping. -- Define the final current pointer as the sole mutable selector of a commit. The - pointer must be small, parseable, and able to select exactly one immutable - commit; it is not a second mutable manifest. -- Separate local-only metadata from remote commit state. Remove or split - `current_pointer_written` as required by ARC-003; derive remote commitment from - the selected pointer/commit relationship rather than an independently mutable - boolean. -- Isolate coherent legacy-pair reading in a clearly named compatibility file or - package boundary with dedicated tests and an in-code comment stating that it is - temporary and removable after migration. Reject torn or ambiguous legacy pairs. - Do not add legacy writing. - -**Tests and exit criteria:** Unit-test model validation, versions, unknown fields -per repository policy, pointer/commit mismatch, checksum/generation mismatch, -legacy coherent/torn cases, and the removable boundary. This stage may introduce -the model and readers without switching publish; Stage 14 switches the writer. - -## Stage 14 — Publish through immutable commits and canonical mappings - -**Read first:** `audit-findings.md` lines 1853–1890 (COR-004), 2068–2103 -(COR-011), 3428–3447 (DUP-002), and 3618–3642 (TST-004); publish, artifact, -remote-manifest, and storage docs. - -**Depends on:** Stages 10 and 13. - -**Goal:** Make new publication an immutable-object transaction whose last remote -mutation selects the complete snapshot. - -**Work:** - -- Make the artifact package's canonical run-manifest path the only source used by - publish; remove the duplicate path construction identified by DUP-002. -- Before upload, build and validate the complete deterministic source-to-remote - destination mapping using exact artifact/source identity, not basename - heuristics. Reject collisions and ambiguity. -- Upload/verify every immutable data object and the immutable commit manifest - first. Update the current pointer exactly once, last. Never upload or mutate - current-state data after pointer selection. -- New publish writes only the new protocol. Preserve the isolated legacy reader - from Stage 13 solely for existing remote state. -- Make publish tests stateful enough to observe remote ordering, generations, - object contents, failure injection, and retry behavior. - -**Tests and exit criteria:** Prove failures before the pointer leave the old -snapshot selected; successful publication selects only a complete new snapshot; -same-basename sources remain distinct; retries are deterministic; and no -post-pointer upload occurs. This is the primary TST-004 stage; Stages 15–16 extend -the same stateful fake. - -## Stage 15 — Make remote locks generation-safe and harden pagination - -**Read first:** `audit-findings.md` lines 2694–2731 (RSK-005), 3007–3032 -(RSK-014), 3618–3642 (TST-004), and 3691–3718 (TST-007); storage adapter and -publish lock docs. - -**Depends on:** Stage 14. - -**Goal:** Make remote ownership effective immediately and replacement conditional, -and prevent non-progressing object-store pagination loops. - -**Work:** - -- Extend the object-store contract with the minimum conditional write/replace - metadata needed for generation/ETag-safe lock acquisition, renewal, and release. - Implement it in the S3 adapter and stateful fake without reducing provider - preconditions to a local check-then-write. -- Commit remote lock ownership immediately, retry conditional conflicts with - bounded/cancellable policy, and never release another owner's generation. -- Revalidate lock ownership at the protected publish commit point immediately - before the current pointer changes. Loss of ownership must leave the old - snapshot selected. -- In paginated listing, reject an empty or repeated continuation token when the - provider claims more results. Include adapter/bucket/prefix context without - secrets. - -**Tests and exit criteria:** Add concurrent-owner, stale-generation, -renewal/release conflict, lock-loss-at-commit, cancellation, empty token, repeated -token, and normal multi-page tests. Run relevant tests under `-race`. - -## Stage 16 — Persist retryable post-commit cleanup state - -**Read first:** `audit-findings.md` lines 1925–1991 (COR-006 and COR-007); -publish, remote manifest, cleanup, and run/session lifecycle docs. - -**Depends on:** Stages 5 and 12–15. - -**Goal:** Make deletion after publication a durable, idempotent obligation that -does not disappear when execution state is reset or an attempt fails. - -**Work:** - -- Persist a typed cleanup obligation independently of the current executed-step - list before destructive cleanup can begin. Bind it to the committed run and - exact declared objects/paths. -- On retry/resume, detect and continue outstanding cleanup even when the current - stage list is empty or rebuilt. Keep cleanup idempotent and confined through - Stage 5. -- Persist completion evidence only after deletion has succeeded or an - already-absent target has been safely verified. A failed evidence write must - leave a retryable obligation rather than falsely completed state. -- Define command outcomes when publication committed but cleanup failed: the - committed snapshot remains authoritative, while the invocation clearly reports - incomplete cleanup and can retry it. - -**Tests and exit criteria:** In the stateful publish fake, inject failures before, -during, and after deletion/evidence persistence; restart with empty execution -state; retry multiple times; and prove no unrelated target is deleted. - -## Stage 17 — Bind restore/status to a committed snapshot and reject conflicts - -**Read first:** `audit-findings.md` lines 1992–2067 (COR-008 and COR-009) and -3643–3669 (TST-005); restore, status, remote manifest, and workspace docs. - -**Depends on:** Stages 4, 13–16. - -**Goal:** Restore and status must observe one pointer-selected immutable snapshot, -and `--force` must never overwrite a directory conflict. - -**Work:** - -- Resolve the current pointer once, load and validate the selected immutable - commit, and use only its declared object set and generation/checksum metadata - throughout one restore/status operation. -- Do not reconstruct current truth from mutable prefixes or a mixture of runs. - Route coherent legacy state through only the isolated Stage 13 compatibility - reader. -- Define conflict types before mutation. `--force` may replace eligible regular - managed files but must reject directory/non-regular conflicts, leaving no - completed local manifest and no false success. -- Make the restore fake stateful and able to mutate remote objects/pointers between - calls so snapshot-binding failures are observable. - -**Tests and exit criteria:** Cover pointer changes during restore, undeclared and -missing objects, version/generation mismatch, coherent legacy restore, file and -directory conflicts with/without force, partial download, and manifest absence on -failure. This is the primary TST-005 stage; Stages 18–20 extend it. - -## Stage 18 — Serialize restore transitions and make restored paths portable - -**Read first:** `audit-findings.md` lines 2732–2769 (RSK-006) and 2803–2838 -(RSK-008), plus TST-005 at lines 3643–3669; restore, local lock, manifest, and -runner reuse docs. - -**Depends on:** Stages 5 and 17. - -**Goal:** Prevent runner/restore races and ensure restored manifests cannot retain -unsafe producer-machine absolute paths. - -**Work:** - -- Acquire the session/workspace lock before executable-state classification and - revalidate state under the lock. All runner reuse checks must participate in the - same transition protocol. -- Persist a durable incomplete-restore marker before the first local mutation. - Keep it blocking runner reuse after a partial forced restore; clear it only after - all verified installs and the completed local manifest are durable. -- Rebase restored artifact paths from portable logical references to the selected - local root. Reject absolute paths outside that root and unsafe relative paths. - Retain an original absolute path, if useful, only as bounded non-authoritative - provenance. -- Do not attempt rollback of arbitrary external effects; the durable incomplete - marker and safe retry are the recovery contract. - -**Tests and exit criteria:** Add concurrent runner/restore process tests, -classification races, forced partial restore/restart/retry, Unix/Windows absolute -path forms, traversal, safe rebasing, and provenance bounds. - -## Stage 19 — Bind audio cache reuse to remote object identity - -**Read first:** `audit-findings.md` lines 2770–2802 (RSK-007) and TST-005 at -lines 3643–3669; previous-audio, object-store, and restore docs. - -**Depends on:** Stages 4, 15, and 17–18. - -**Goal:** Reuse restored/downloaded audio only when it is a verified regular file -for the selected remote object version. - -**Work:** - -- Persist a sidecar or equivalent typed cache record binding local audio to remote - object key plus authoritative version/generation/checksum information. -- Verify the local candidate root-relatively as a regular no-follow file and - validate size/digest as required before reuse. Do not treat an unqualified - multipart ETag as a content hash. -- Refresh or fail clearly when identity evidence is absent, stale, ambiguous, or - unsupported; `--force` must not mean trust an unrelated local file. -- Install refreshed content using Stage 4's confined/durable download path. - -**Tests and exit criteria:** Cover valid reuse, changed generation with same name, -missing/stale sidecar, multipart ETag, symlink/non-regular cache entries, corrupted -bytes, interrupted refresh, and force behavior. - -## Stage 20 — Unify previous-source readiness and eliminate duplicate transfers - -**Read first:** `audit-findings.md` lines 2038–2103 (COR-010 and COR-011) and -3064–3112 (EFF-001), plus TST-005 at lines 3643–3669; previous-cache, restore, -prepare, and dry-run docs. - -**Depends on:** Stages 14 and 17–19. - -**Goal:** Give restore, prepare, run, and dry-run one authoritative previous-source -resolver and retain verified downloads for the later restore snapshot. - -**Work:** - -- Implement one resolver that evaluates required/optional previous inputs from the - pointer-selected commit's exact source-to-destination mapping. Required absence - is actionable; optional absence is a typed non-error. -- Eliminate basename fallback from the new protocol. Keep any necessary legacy - fallback only inside the isolated Stage 13 compatibility boundary and reject - ambiguous matches. -- Reuse the generation-bound, verified candidate downloaded while constructing the - previous-source view when the restore snapshot needs the same object. Do not - reopen an unverified path or perform a second network transfer. -- Count actual transferred bytes once and make dry-run/readiness documentation - accurately describe remote checks and potential download/cache work. - -**Tests and exit criteria:** Prove identical behavior across prepare/restore/run, -required/optional cases, same basenames, legacy ambiguity, generation changes, -single-transfer accounting, cancellation, and dry-run output. - -## Stage 21 — Tighten configuration parsing, values, and expectations - -**Read first:** `audit-findings.md` lines 2104–2253 (COR-012 through COR-015), -3805–3821 (TST-011), and 3853–3868 (TST-014); configuration and CLI docs. - -**Depends on:** Stages 2 and 11. - -**Goal:** Reject ambiguous or semantically invalid configuration before side -effects and give the configuration package focused, non-duplicative tests. - -**Work:** - -- Require exactly one YAML document, including rejection of a second empty or - comment-only document if the parser exposes it as another document. -- Validate all duration values that control retries, timeouts, polling, or leases - as positive and within any existing semantic bounds. -- Select storage backends only through explicit supported configuration; remove - inference from incidental S3-shaped fields and reject incomplete/unknown modes. -- Make the previous-session flag a strict expectation: when requested, absence or - mismatch is an actionable error, not a silent downgrade. -- Centralize test environment cleanup and reorganize configuration tests into - loader, defaulting, and validation ownership as required by TST-011/TST-014. - -**Tests and exit criteria:** Cover multi-document YAML, zero/negative/overflow-like -durations, backend combinations, strict previous expectations, environment -isolation, defaults, unknown fields, and stable actionable errors. +- Replace direct diagnostic `os.Create` use with the established confined, + no-follow destination mechanism. Validate the leaf as part of the mutation, + reject symlink and non-regular destinations, preserve group-workspace modes, and + retain the platform-specific safety guarantees established by Stage 4. A prior + ancestor check followed by an ordinary pathname create is not sufficient. +- Remove pathname reopening from diagnostic-tail construction. Retain a small, + bounded per-stream tail from bytes that have already passed through streaming + redaction, preferably in the diagnostic writer itself, and obtain failure + diagnostics from that retained state after redaction has been flushed. Do not + read the log path again in the error path. +- Preserve stdout/stderr file behavior, shared-path behavior, byte caps, split- + chunk secret redaction, capture-limit signaling, and group collaboration modes. + Do not create an unbounded in-memory copy of either stream. +- Keep API-key values out of persisted logs and every returned error even if a + workspace peer renames or replaces a diagnostic pathname while the process is + running. +- Remove obsolete pathname-tail helpers and update current-behavior documentation + only if the existing documentation describes the replaced mechanism. + +**Tests and exit criteria:** Add adversarial tests proving that an existing log +leaf symlink is rejected without truncating its target; a non-regular leaf is +rejected; pathname replacement after the log is opened cannot inject foreign +content into a returned error; and a known credential remains absent from files +and errors across split writes. Cover separate and shared stdout/stderr paths, +exact diagnostic caps, and cap-plus-one behavior. Run focused subprocess and +file-operation tests under `-race`, plus Linux, macOS, and Windows cross-builds for +affected packages. Native platform tests remain conditional on available runners. **Status:** Completed. -## Stage 22 — Make product configuration truthful and own remote temp files +## Stage 34 — Dispose of owned subprocess descendants after natural leader exit -**Read first:** `audit-findings.md` lines 2521–2552 (COR-024), 3157–3197 -(ARC-004), and 2839–2869 (RSK-009); configuration schema, notifications, -Scriptorium, remote config, and integration docs. +**Read first:** `docs/policy/architecture.md`, `docs/policy/testing.md`, +`docs/internal/adapters.md`, the subprocess run/process-tree implementations for +Unix and Windows, and all process-tree test helpers. -**Depends on:** Stages 4, 7, and 21. +**Depends on:** Completed Stage 8 and Stage 33. -**Goal:** Expose only implemented configuration behavior and keep remote-config -temporary paths within a command-scoped lifecycle. +**Goal:** Close the lifecycle gap in which `cmd.Wait` can complete before a +descendant in Narratio's process group or Windows job exits, allowing that +descendant to outlive `Run` after either a successful or failed leader exit. **Work:** -- Remove inert Scriptorium `artifact` and `path` fields from schema, Go types, - examples, validation, and docs. Reject obsolete fields under the repository's - strict configuration policy with clear migration guidance where feasible. -- Support notifications only as an explicit no-op mode. Reject any configured - backend, recipient, or provider-shaped value until a provider exists; align CLI - and docs. -- Replace ad hoc remote-config temp paths with one owned command-scoped object or - lifecycle. Clean it on success, validation failure, adapter failure, - cancellation, and early returns using confined deletion. -- Never serialize ephemeral local temp paths into manifests, run records, or - diagnostic output that purports to be reusable state. +- Route the normal `cmd.Wait` completion branch through bounded owned-tree + disposal before `Run` returns. Apply the ownership rule for successful exits, + non-zero exits, and `exec.ErrWaitDelay`, not only cancellation, timeout, or + diagnostic-cap paths. +- On Unix, detect and gracefully terminate any remaining process-group members, + wait only for the existing bounded grace period, then forcefully terminate + survivors. Treat an already-empty group as success and avoid imposing the grace + delay on ordinary commands with no descendants. +- On Windows, preserve job-object kill-on-close semantics and ensure natural + leader completion closes/disposes the job before return. Keep handles and + goroutines bounded on every exit path. +- Consolidate terminal cleanup so races among context cancellation, capture-limit + signaling, and leader completion cannot skip disposal or produce unsafe double + ownership. Preserve timeout/cancellation classification and join actionable + cleanup failures without hiding the original command result. +- Treat a descendant that deliberately leaves the owned process group or job as + outside this stage; do not build a system-wide process discovery mechanism. -**Tests and exit criteria:** Cover obsolete/unknown product fields, explicit no-op, -backend/recipient rejection, every remote-config exit path, cancellation, stale -cleanup, and manifest inspection for ephemeral paths. +**Tests and exit criteria:** Extend the real subprocess helper with cases where a +leader exits zero while a descendant (a) retains inherited streams and (b) +redirects or closes them, plus a non-zero leader case. Prove `Run` completes +within bounded time and no descendant reaches a delayed sentinel after return. +Retain cancellation, timeout, and capture-limit coverage and run the subprocess +suite repeatedly, shuffled, and under `-race`. Cross-build the affected packages +for Linux, macOS, and Windows; run native platform tests only where runners are +actually available. -**Status:** Completed. +**Status:** Pending. -## Stage 23 — Stream WhisperX uploads and make the adapter race-safe +## Stage 35 — Bound remote current-state and lock control-plane reads -**Read first:** `audit-findings.md` lines 2254–2281 (COR-016), 3113–3156 -(EFF-002), 3363–3387 (TST-001), and 3691–3718 (TST-007); WhisperX integration -docs. +**Read first:** `docs/policy/architecture.md`, `docs/policy/testing.md`, +`docs/internal/storage.md`, `docs/internal/artifacts.md`, +`docs/internal/command-restore.md`, the current-state commit reader, remote lock +loader, object-store contract and implementations, and their stateful fakes and +tests. -**Depends on:** Stages 7–9 and 21. +**Depends on:** Completed Stages 13–17. -**Goal:** Make WhisperX transport bounded, retry-correct, cancellable, and safe -under the race detector. +**Goal:** Prevent corrupt, misconfigured, or hostile object storage from forcing +unbounded temporary-disk or memory consumption before small remote control-plane +objects can be parsed or verified. **Work:** -- Accept only absolute `http` or `https` WhisperX endpoints; reject file and other - schemes before execution. -- Stream multipart bodies through a pipe per attempt instead of buffering the - complete audio request. Each retry gets a fresh reader/writer and deterministic - metadata; producer failures and cancellation must reach the HTTP caller without - goroutine leaks. -- Retain bounded response/result acquisition from Stage 7 and make request/response - limits and errors adapter-specific. -- Synchronize the Whisper fake's request collection and expose snapshots through a - safe accessor. Migrate tests away from direct shared-slice reads. -- Add evidence that memory behavior is no longer proportional to complete upload - size, using deterministic backpressure/streaming tests rather than fragile heap - thresholds alone. - -**Tests and exit criteria:** Cover schemes, retries, mid-stream read failure, -server early close, cancellation, blocked producer, exact payloads, bounded -responses, and goroutine completion. Run Whisper packages and their callers under -`go test -race`; this stage unblocks the full race suite in Stage 31. - -**Status:** Completed. - -## Stage 24 — Correct prepare/transcribe transition semantics - -**Read first:** `audit-findings.md` lines 2282–2379 (COR-017 through COR-019) -and 3719–3743 (TST-008); prepare, transcribe, previous-cache, artifacts, and stage -lifecycle docs. - -**Depends on:** Stages 12 and 20–23. - -**Goal:** Make prepare/transcribe state transitions exact, cancellation-safe, and -independent of accidental basename behavior. - -**Work:** - -- Clear the managed previous-session tree when the effective requirement set is - empty as well as when it is populated. Publish only the current invocation's - verified result set. -- Track transcribe work as planned, dispatched, and completed. Cancellation or a - missing completion may not be reported as success and may not materialize a - completed artifact/manifest entry. -- Detect duplicate cleaned-audio source identities before dispatch. Allow distinct - source paths with the same basename and preserve exact identity through outputs. -- Keep state mutations inside the established lifecycle/terminalization contract. - -**Tests and exit criteria:** Add no-requirements cleanup, stale prior tree, -cancellation at each phase, partial completion, duplicate source, same-basename -distinct source, retry, and no-false-manifest tests. This is the primary TST-008 -stage; Stage 25 adds output/resolver boundary cases. - -**Status:** Completed. - -## Stage 25 — Enforce output-path authority and shared singleton resolution - -**Read first:** `audit-findings.md` lines 3306–3330 (ARC-006), 3515–3537 -(DUP-006), and 3719–3743 (TST-008); stage request/result, artifact resolver, and -transcription docs. - -**Depends on:** Stage 24. - -**Goal:** Make the stage-requested destination authoritative and resolve singleton -transcripts through one artifact-owned contract. - -**Work:** - -- Require an adapter result path to be empty (meaning the requested path) or - exactly equal to the normalized stage-requested path. Reject any other path - before materialization. -- Put the equality/empty contract in one shared stage/adapter boundary helper and - add conformance tests for all built-in adapters. -- Route singleton transcript resolution through the artifact package's canonical - resolver. Keep plural raw-source discovery separate; do not collapse different - cardinality semantics into one ambiguous helper. -- Remove duplicate singleton-selection code and stale error wording. - -**Tests and exit criteria:** Cover empty/exact/different/relative-equivalent paths, -no output, multiple transcripts, unsafe paths, adapter conformance, and plural raw -sources. No adapter-selected alternate path may become authoritative. - -**Status:** Completed. - -## Stage 26 — Centralize typed extraction-bundle evidence - -**Read first:** `audit-findings.md` lines 3538–3568 (DUP-007) and 3907–3932 -(SIM-002, merged into DUP-007), plus extraction artifact and resume/catalog docs. - -**Depends on:** Stages 2, 4, and 7. - -**Goal:** Evaluate extraction-bundle evidence once while preserving the different -policy decisions made by resume and catalog consumers. - -**Work:** - -- Add an artifact-owned typed proof/result that classifies extraction evidence as - valid, absent, obsolete, or unsafe and carries only verified metadata/paths. -- Centralize structural and filesystem proof: expected files, manifest/bundle - coherence, safe root-relative regular files, versions, and existing identity - evidence. Do not encode resume/catalog policy into the proof. -- Make resume and catalog map the same proof to their own documented outcomes. - Remove duplicate validators and the merged SIM-002 opportunity for a broad - generic resolver. - -**Tests and exit criteria:** Use one shared evidence fixture matrix for both -consumers and assert their intentionally different outcomes for every typed state. -Include unsafe root, symlink, incomplete, obsolete-version, and valid cases. - -**Status:** Completed. - -## Stage 27 — Bind extraction reuse to direct transcript identity - -**Read first:** `audit-findings.md` lines 2380–2423 (COR-020) and 3744–3774 -(TST-009); extraction fingerprint, artifact evidence, direct-input, and resume -docs. - -**Depends on:** Stages 25–26. - -**Goal:** Never reuse extraction output after the direct transcript input changes -while keeping the external producer's transitive dependencies explicit. - -**Work:** - -- Extend the extraction fingerprint with a deterministic checksum of the exact - trimmed direct transcript bytes and stable source/producer identity needed by - the contract. -- Recompute that direct-input identity on every resume decision and feed it into - the typed Stage 26 evidence proof. A mismatch is obsolete evidence, not success. -- Keep Notarius/external transitive dependency freshness under the existing - explicit `--force` contract unless stable producer evidence is available; do not - invent a false dependency graph. -- Ensure extraction failure or obsolescence does not leave newly valid-looking - catalog/manifest entries. - -**Tests and exit criteria:** Add unchanged/changed/trimmed transcript cases, -producer/source changes, resume/restart, force, unsafe root, over-limit lane -result, and failure-before-publication tests. This is the primary TST-009 stage. - -**Status:** Completed. - -## Stage 28 — Establish one effective artifact set and catalog bootstrap - -**Read first:** `audit-findings.md` lines 2456–2491 (COR-022), 3331–3362 -(ARC-007), 3569–3595 (DUP-008), and 3775–3804 (TST-010); analyze, extraction -catalog, artifact selection, configuration, and publish-filter docs. - -**Depends on:** Stages 21 and 26–27. - -**Goal:** Resolve artifact selection once into a typed effective set used by every -analyze precondition and input-composition path. - -**Work:** - -- Define one typed effective artifact set. Explicit CLI/stage selection overrides - configured defaults even when the selected artifact is disabled by default. -- Use that exact set for validation, prerequisite planning, extraction catalog - loading, prompt/input composition, and analyze execution. Keep publication - filtering a separate typed decision. -- Centralize only deterministic catalog definitions/bootstrap ordering in the - artifact owner. Keep caller-specific missing/optional/error policy outside the - shared bootstrap. -- Remove duplicate catalog lists and boolean mixtures that can diverge. - -**Tests and exit criteria:** Table-test configured defaults versus explicit -selection, disabled overrides, empty selections, ordering, catalog versions, -publication filters, and the exact composed analyze inputs. This is the primary -TST-010 stage; Stage 29 adds resolution/error cases. - -**Status:** Completed. - -## Stage 29 — Make analyze resolution typed, optional, actionable, and deterministic - -**Read first:** `audit-findings.md` lines 2424–2455 (COR-021), 2492–2520 -(COR-023), 3033–3063 (RSK-015), 3933–3961 (SIM-003), and 3775–3804 (TST-010); -analyze, stage command, artifact dependency, and troubleshooting docs. - -**Depends on:** Stage 28. - -**Goal:** Give analyze one typed resolution/execution plan with correct optional -semantics, deterministic diagnostics, and executable remediation guidance. - -**Work:** - -- Represent built-in artifact resolution as typed present/absent/error results. - Optional absence is omitted; required absence returns a stable actionable error. -- Build one analyze execution context/effective plan from the Stage 28 artifact - set and typed resolutions. Do not add a generic resolver unrelated to analyze. -- Sort aggregated missing/dependency diagnostics by stable artifact/source keys. - Never expose map iteration order. -- Correct remediation text to the real command form: - `narratio run-stage prepare --force`, with context-specific - alternatives only when they are actually valid. - -**Tests and exit criteria:** Cover optional missing built-ins, required missing, -mixed custom/built-in artifacts, repeated randomized map insertion, exact guidance, -context construction, and final prompt inputs. Run deterministic tests repeatedly. - -**Status:** Completed. - -## Stage 30 — Remove misleading contracts and align Audita ownership - -**Read first:** `audit-findings.md` lines 3198–3228 (ARC-001), 3280–3305 -(ARC-005), 3962–3979 (SIM-004), 4021–4037 (COM-003), and 4085–4101 -(COM-006); adapter, stage API, Audita, extraction source, and developer docs. - -**Depends on:** Stages 23–29. - -**Goal:** Remove dead or misleading surfaces after the behavioral contracts have -stabilized and place static Audita policy in its constructor. - -**Work:** - -- Remove the unused `IODecl`/`Declares` surface and update tests/docs rather than - preserving a compatibility shim for an internal unused contract. -- Move static Audita settings to constructor/configuration ownership. Per-run - requests carry only run-specific paths, modules, and execution data. Update the - fake and all call sites. -- Delete dead helpers made obsolete by the remediation. Do not delete a narrow - helper merely because it has one caller when it still owns a real invariant. -- Remove the stale Audita TODO and add a rationale comment only if ownership is not - self-evident. Add the extraction-source doc comment required by COM-006. - -**Tests and exit criteria:** Compile all adapters and fakes, run their focused -tests, search for removed symbols/TODOs/stale request fields, and confirm public -package documentation matches the remaining contract. - -**Status:** Completed. - -## Stage 31 — Enforce CI validation and streamline the assembled test suite - -**Read first:** `audit-findings.md` lines 3822–3836 (TST-012) and 3869–3885 -(TST-015); testing policy, development workflow, Woodpecker configuration, and -release docs. - -**Depends on:** All production stages, especially Stage 23. - -**Goal:** Make ordinary automated validation unavoidable and consolidate broad -stage matrices only after the focused regression protections exist. - -**Work:** - -- Make every change and release path run ordinary tests, `go vet`, `go build`, and - documentation/example validation. A release must consume a revision that passed - the required validation, not merely run alongside an optional test pipeline. -- Run the full `go test -race ./...` suite on every change now that TST-001 is - fixed. Add scheduled shuffled/repeated execution for order dependence. -- Preserve focused owner tests from prior stages, then consolidate redundant broad - stage matrices as directed by TST-015. Do not reduce behavior coverage or hide - platform cases in an opaque mega-test. -- Add native macOS/Windows jobs only when authoritative runner labels and - successful native runners are available. Until then, retain explicit - cross-build checks and document the native evidence gap without inventing - labels or marking cross-compilation as equivalent. - -**Tests and exit criteria:** Validate Woodpecker syntax and dependency graph, -exercise the local equivalents of every job, prove failure propagation to release -eligibility, and inspect the final suite for duplicated matrices and lost cases. - -**Status:** Completed. - -## Stage 32 — Reconcile lifecycle/analyze documentation and close the remediation - -**Read first:** `audit-findings.md` lines 3229–3255 (ARC-002, merged into -COM-002), 3998–4020 (COM-002), 4038–4055 (COM-004, merged into COM-002), and -4056–4084 (COM-005); all current-behavior policy and package documents modified by -the preceding stages. - -**Depends on:** Stages 1–31. - -**Goal:** Make the repository's normative documentation describe the implemented -architecture exactly and mechanically verify that every actionable audit finding -has a completed primary stage. - -**Work:** - -- Document the distinction between successful no-output, optional absence, and a - stage's explicit self-skip. Keep the production lifecycle semantics implemented - in earlier stages; do not add a new lifecycle abstraction for COM-002. -- Reconcile extraction catalog schema, effective selection/override semantics, - analyze consumption, optional absence, force/freshness rules, remote commit and - compatibility behavior, restore recovery, collaborative modes, secret handling, - output limits, and operations quota guidance. -- Review examples, troubleshooting commands, package docs, diagrams, and comments - against the current code. Remove superseded statements rather than layering new - caveats over them. -- Verify the traceability inventory below. Every confirmed actionable finding must - have one completed primary stage; merged/rejected findings must retain their - recorded disposition; accepted risks must not have been silently expanded. - -**Tests and exit criteria:** Run the full Stage 31 validation locally, run all -documentation/example checks, verify clean generated/example state, inspect the -final diff and repository status, and mark this stage complete only when no -required remediation remains. - -**Status:** Completed. +- Inventory the new-protocol read paths for the mutable current pointer, immutable + commit manifest, selected session manifest, and remote lock document. Include + the isolated legacy current-state reader only where it accepts equivalent small + control objects. Do not apply these limits to intentionally large published + artifact payloads. +- Define generous, documented, centrally discoverable constants in the packages + that own each control-plane contract. Use separate policy constants where the + credible sizes differ; do not embed numeric limits at call sites or replace the + owner policies with one unexplained global limit. +- Stream each object through an enforcing `limit+1` read before parsing or + retaining it. Reject oversized metadata before transfer when available, but + still enforce the limit against actual bytes because metadata can be missing, + stale, or inaccurate. Avoid downloading these small objects unboundedly to a + temporary file and then calling `os.ReadFile` or `io.ReadAll`. +- Preserve immutable commit checksum, declared-size, generation/ETag, identity, + and pointer-selection validation. Ensure the metadata used for validation + belongs to the opened object version; do not weaken coherence while removing + the temporary download path. +- Every over-limit error must identify the responsible control-plane contract, + object category, key or safe storage context, and configured byte limit without + including credentials. Maintain cancellation and reader-close behavior on all + exits. +- Factor a bounded storage-read mechanism only if it has a clear storage-layer + invariant; retain the owner-specific constants and errors in the artifact and + remote-lock owners. + +**Tests and exit criteria:** Add exact-limit and limit-plus-one cases for each +object category, including inaccurate/absent size metadata, short reads, +cancellation, malformed JSON/YAML, checksum or generation mismatch, and readers +that verify they are closed. Prove oversized current-state downloads do not create +unbounded temporary files and oversized lock documents are rejected without +unbounded allocation. Retain coherent legacy-reader tests and stateful S3/fake +ordering and conditional-write tests. Run focused storage, artifacts, app, restore, +and publish tests under `-race`, followed by the complete repository validation. + +**Status:** Pending. ## Finding traceability inventory -This inventory assigns exactly one primary implementation stage to each confirmed -actionable finding. A stage may read or test a finding owned by another stage when -the behavior crosses a boundary, but it must not create a second competing fix. +This inventory remains the closed traceability record for Stages 1–32. A pending +follow-up stage may strengthen the implementation of an original stage, but does +not change the immutable finding disposition or create a second audit finding. | Finding(s) | Primary stage | | --- | ---: | @@ -1161,9 +375,17 @@ separate implementation work: ARC-002 and COM-004 are merged into COM-002; SIM-002 is merged into DUP-007; DUP-004 is rejected. Their audit rationale remains authoritative. +## Follow-up review traceability + +| Review finding | Remediation stage | Source | +| --- | ---: | --- | +| Diagnostic leaf symlinks and pathname-reopened tails bypass confinement/redaction guarantees | 33 | Post-implementation review | +| Unix descendants survive natural leader exit | 34 | Post-implementation review | +| Remote current-state and lock control objects are read without acquisition limits | 35 | Post-implementation review | + ## Open Questions -None. The governing decisions above make this plan decision-complete. Native -macOS/Windows CI activation remains contingent on external runner availability, -but the required implementation, cross-build checks, and rule against inventing -runner labels are already specified and do not require an architectural decision. +None. The remediation behavior and ownership decisions are specified above. +Native macOS/Windows runtime validation remains contingent on external runner +availability, but cross-build requirements and the rule against inventing runner +labels make implementation decision-complete. diff --git a/internal/adapters/subprocess/diagnostics.go b/internal/adapters/subprocess/diagnostics.go index fa04f0b..f1e47b2 100644 --- a/internal/adapters/subprocess/diagnostics.go +++ b/internal/adapters/subprocess/diagnostics.go @@ -18,6 +18,7 @@ const ( MaxStdoutDiagnosticBytes int64 = 8 * 1024 * 1024 // MaxStderrDiagnosticBytes bounds persisted stderr from one external command. MaxStderrDiagnosticBytes int64 = 8 * 1024 * 1024 + diagnosticTailBytes = 2048 ) var inheritedEnvironmentNames = map[string]struct{}{ @@ -87,6 +88,7 @@ type diagnosticWriter struct { received int64 persisted int64 redactor streamRedactor + tail []byte } func openLogWriters(stdoutPath, stderrPath, owner string, sensitiveValues []string) (*logWriters, error) { @@ -169,6 +171,7 @@ func (w *diagnosticWriter) writeRedacted(data []byte) error { } written, err := w.target.Write(toWrite) w.persisted += int64(written) + w.retainTail(toWrite[:written]) w.logs.mu.Unlock() if err != nil { return err @@ -179,6 +182,34 @@ func (w *diagnosticWriter) writeRedacted(data []byte) error { return nil } +func (w *diagnosticWriter) Tail() string { + w.logs.mu.Lock() + defer w.logs.mu.Unlock() + return strings.TrimSpace(string(w.tail)) +} + +func (w *diagnosticWriter) retainTail(data []byte) { + if len(data) >= diagnosticTailBytes { + if cap(w.tail) < diagnosticTailBytes { + w.tail = make([]byte, diagnosticTailBytes) + } else { + w.tail = w.tail[:diagnosticTailBytes] + } + copy(w.tail, data[len(data)-diagnosticTailBytes:]) + return + } + if cap(w.tail) < diagnosticTailBytes { + retained := make([]byte, len(w.tail), diagnosticTailBytes) + copy(retained, w.tail) + w.tail = retained + } + if overflow := len(w.tail) + len(data) - diagnosticTailBytes; overflow > 0 { + copy(w.tail, w.tail[overflow:]) + w.tail = w.tail[:len(w.tail)-overflow] + } + w.tail = append(w.tail, data...) +} + func (w *diagnosticWriter) reachLimit() error { limit := &captureLimitError{stream: w.stream, owner: w.owner, limit: w.limit} w.logs.mu.Lock() @@ -224,7 +255,7 @@ func openDiagnosticFile(path string) (*os.File, error) { if err := fileops.EnsureWorkspaceDirectory(filepath.Dir(path)); err != nil { return nil, fmt.Errorf("create log directory for %q: %w", path, err) } - file, err := os.Create(path) + file, err := fileops.OpenFileConfined(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fileops.WorkspaceFileMode) if err != nil { return nil, fmt.Errorf("open log file %q: %w", path, err) } diff --git a/internal/adapters/subprocess/run.go b/internal/adapters/subprocess/run.go index a3294b7..038f3a8 100644 --- a/internal/adapters/subprocess/run.go +++ b/internal/adapters/subprocess/run.go @@ -3,7 +3,6 @@ package subprocess import ( "context" "fmt" - "io" "os" "os/exec" "strings" @@ -124,7 +123,7 @@ func Run(ctx context.Context, req RunRequest) (RunResult, error) { return result, nil } - stderrTail := readDiagnosticTail(req.StderrLogPath, 2048) + stderrTail := logs.stderr.Tail() diagnostics := buildDiagnostics(req, result, stderrTail) if captureLimit != nil { @@ -208,36 +207,3 @@ func fdDiagnosticsHint(exitCode int, stderrTail string) string { } return "" } - -func readDiagnosticTail(path string, maxBytes int64) string { - if strings.TrimSpace(path) == "" || maxBytes <= 0 { - return "" - } - f, err := os.Open(path) - if err != nil { - return "" - } - defer f.Close() - - info, err := f.Stat() - if err != nil { - return "" - } - size := info.Size() - start := int64(0) - if size > maxBytes { - start = size - maxBytes - } - if _, err := f.Seek(start, io.SeekStart); err != nil { - return "" - } - data, err := io.ReadAll(f) - if err != nil { - return "" - } - tail := strings.TrimSpace(string(data)) - if tail == "" { - return "" - } - return tail -} diff --git a/internal/adapters/subprocess/run_test.go b/internal/adapters/subprocess/run_test.go index e3d87ea..87675ee 100644 --- a/internal/adapters/subprocess/run_test.go +++ b/internal/adapters/subprocess/run_test.go @@ -3,9 +3,11 @@ package subprocess import ( "bytes" "context" + "errors" "os" "os/exec" "path/filepath" + "runtime" "strconv" "strings" "testing" @@ -134,6 +136,166 @@ func TestRunFailureRedactsSensitiveTail(t *testing.T) { } } +func TestRunRejectsSymlinkDiagnosticWithoutTruncatingTarget(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("creating symlinks requires privileges that are not available on every Windows runner") + } + exe, err := os.Executable() + if err != nil { + t.Fatalf("os.Executable() error = %v", err) + } + + dir := t.TempDir() + targetPath := filepath.Join(dir, "outside.log") + const original = "must remain unchanged" + if err := os.WriteFile(targetPath, []byte(original), 0o600); err != nil { + t.Fatalf("WriteFile(target) error = %v", err) + } + stdoutPath := filepath.Join(dir, "stdout.log") + if err := os.Symlink(targetPath, stdoutPath); err != nil { + t.Fatalf("Symlink() error = %v", err) + } + + _, err = Run(context.Background(), RunRequest{ + Executable: exe, + Args: []string{"-test.run=^TestSubprocessHelper$", "--", "success"}, + EnvOverrides: map[string]string{"GO_WANT_SUBPROCESS_HELPER": "1"}, + StdoutLogPath: stdoutPath, + StderrLogPath: filepath.Join(dir, "stderr.log"), + }) + if err == nil || !strings.Contains(err.Error(), "symbolic link") { + t.Fatalf("Run() error = %v, want symbolic-link rejection", err) + } + data, readErr := os.ReadFile(targetPath) + if readErr != nil { + t.Fatalf("ReadFile(target) error = %v", readErr) + } + if string(data) != original { + t.Fatalf("target content = %q, want %q", data, original) + } +} + +func TestRunFailureUsesOpenedDiagnosticAfterPathReplacement(t *testing.T) { + exe, err := os.Executable() + if err != nil { + t.Fatalf("os.Executable() error = %v", err) + } + + dir := t.TempDir() + readyPath := filepath.Join(dir, "ready") + releasePath := filepath.Join(dir, "release") + stderrPath := filepath.Join(dir, "stderr.log") + openedPath := filepath.Join(dir, "opened-stderr.log") + const secretValue = "replacement-api-key-value" + const commandContent = "trusted command failure" + req := RunRequest{ + Executable: exe, + Args: []string{"-test.run=^TestSubprocessHelper$", "--", "delayed-fail"}, + EnvOverrides: map[string]string{ + "GO_WANT_SUBPROCESS_HELPER": "1", + "API_KEY": secretValue, + "SUBPROCESS_HELPER_READY_PATH": readyPath, + "SUBPROCESS_HELPER_RELEASE_PATH": releasePath, + "SUBPROCESS_HELPER_STDERR": commandContent, + }, + StdoutLogPath: filepath.Join(dir, "stdout.log"), + StderrLogPath: stderrPath, + } + + resultCh := make(chan error, 1) + go func() { + _, runErr := Run(context.Background(), req) + resultCh <- runErr + }() + waitForHelperFile(t, readyPath) + if err := os.Rename(stderrPath, openedPath); err != nil { + t.Fatalf("Rename(stderr log) error = %v", err) + } + if err := os.WriteFile(stderrPath, []byte(secretValue), 0o600); err != nil { + t.Fatalf("WriteFile(replacement) error = %v", err) + } + if err := os.WriteFile(releasePath, []byte("continue"), 0o600); err != nil { + t.Fatalf("WriteFile(release) error = %v", err) + } + + select { + case runErr := <-resultCh: + if runErr == nil { + t.Fatal("Run() error = nil, want command failure") + } + if strings.Contains(runErr.Error(), secretValue) { + t.Fatalf("error read replacement-path content: %q", runErr) + } + if !strings.Contains(runErr.Error(), commandContent) { + t.Fatalf("error = %q, want retained command diagnostic", runErr) + } + case <-time.After(3 * time.Second): + t.Fatal("Run() did not return after helper release") + } + + openedData, err := os.ReadFile(openedPath) + if err != nil { + t.Fatalf("ReadFile(opened diagnostic) error = %v", err) + } + if !strings.Contains(string(openedData), commandContent) { + t.Fatalf("opened diagnostic = %q, want command content", openedData) + } + replacementData, err := os.ReadFile(stderrPath) + if err != nil { + t.Fatalf("ReadFile(replacement diagnostic) error = %v", err) + } + if string(replacementData) != secretValue { + t.Fatalf("replacement diagnostic = %q, want %q", replacementData, secretValue) + } +} + +func TestRunRedactsSplitCredentialInSeparateAndSharedDiagnostics(t *testing.T) { + exe, err := os.Executable() + if err != nil { + t.Fatalf("os.Executable() error = %v", err) + } + const secretValue = "split-super-secret-value" + + for _, shared := range []bool{false, true} { + t.Run(map[bool]string{false: "separate", true: "shared"}[shared], func(t *testing.T) { + dir := t.TempDir() + stdoutPath := filepath.Join(dir, "stdout.log") + stderrPath := filepath.Join(dir, "stderr.log") + if shared { + stderrPath = stdoutPath + } + req := RunRequest{ + Executable: exe, + Args: []string{"-test.run=^TestSubprocessHelper$", "--", "splitsecret"}, + EnvOverrides: map[string]string{ + "GO_WANT_SUBPROCESS_HELPER": "1", + "API_KEY": secretValue, + }, + StdoutLogPath: stdoutPath, + StderrLogPath: stderrPath, + } + + _, runErr := Run(context.Background(), req) + if runErr == nil { + t.Fatal("Run() error = nil, want command failure") + } + if strings.Contains(runErr.Error(), secretValue) || !strings.Contains(runErr.Error(), "") { + t.Fatalf("error = %q, want redacted credential", runErr) + } + paths := map[string]struct{}{stdoutPath: {}, stderrPath: {}} + for path := range paths { + data, readErr := os.ReadFile(path) + if readErr != nil { + t.Fatalf("ReadFile(%q) error = %v", path, readErr) + } + if strings.Contains(string(data), secretValue) || !strings.Contains(string(data), "") { + t.Fatalf("diagnostic %q = %q, want redacted credential", path, data) + } + } + }) + } +} + func TestRunRedactsInheritedSensitiveEnvironment(t *testing.T) { exe, err := os.Executable() if err != nil { @@ -251,6 +413,30 @@ func TestDiagnosticWriterHonorsExactLimitAndCapPlusOne(t *testing.T) { } } +func TestDiagnosticWriterRetainsBoundedRedactedTail(t *testing.T) { + logs := &logWriters{limits: make(chan *captureLimitError, 1)} + var output bytes.Buffer + secret := "credential-value" + writer := newDiagnosticWriter(logs, "stderr", "test", &output, 16*1024, []string{secret}) + prefix := strings.Repeat("x", diagnosticTailBytes+512) + if _, err := writer.Write([]byte(prefix + secret[:7])); err != nil { + t.Fatalf("first Write() error = %v", err) + } + if _, err := writer.Write([]byte(secret[7:] + "-failure")); err != nil { + t.Fatalf("second Write() error = %v", err) + } + if err := writer.Flush(); err != nil { + t.Fatalf("Flush() error = %v", err) + } + tail := writer.Tail() + if len(tail) > diagnosticTailBytes { + t.Fatalf("retained tail length = %d, want at most %d", len(tail), diagnosticTailBytes) + } + if strings.Contains(tail, secret) || !strings.Contains(tail, "-failure") { + t.Fatalf("retained tail = %q, want bounded redacted content", tail) + } +} + func TestRunFailureAddsBadDescriptorHint(t *testing.T) { exe, err := os.Executable() if err != nil { @@ -489,6 +675,30 @@ func TestSubprocessHelper(t *testing.T) { case "failbadfd": _, _ = os.Stderr.WriteString("OSError: [Errno 9] Bad file descriptor\n") os.Exit(120) + case "delayed-fail": + if err := os.WriteFile(os.Getenv("SUBPROCESS_HELPER_READY_PATH"), []byte("ready"), 0o600); err != nil { + os.Exit(4) + } + deadline := time.Now().Add(2 * time.Second) + for { + if _, err := os.Stat(os.Getenv("SUBPROCESS_HELPER_RELEASE_PATH")); err == nil { + break + } else if !errors.Is(err, os.ErrNotExist) || time.Now().After(deadline) { + os.Exit(5) + } + time.Sleep(10 * time.Millisecond) + } + _, _ = os.Stderr.WriteString(os.Getenv("SUBPROCESS_HELPER_STDERR")) + os.Exit(6) + case "splitsecret": + secret := os.Getenv("API_KEY") + split := len(secret) / 2 + for _, stream := range []*os.File{os.Stdout, os.Stderr} { + _, _ = stream.WriteString(secret[:split]) + time.Sleep(20 * time.Millisecond) + _, _ = stream.WriteString(secret[split:] + "\n") + } + os.Exit(7) case "sleep": time.Sleep(500 * time.Millisecond) os.Exit(0) @@ -546,3 +756,17 @@ func TestSubprocessHelper(t *testing.T) { os.Exit(2) } } + +func waitForHelperFile(t *testing.T, path string) { + t.Helper() + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if _, err := os.Stat(path); err == nil { + return + } else if !errors.Is(err, os.ErrNotExist) { + t.Fatalf("Stat(%q) error = %v", path, err) + } + time.Sleep(10 * time.Millisecond) + } + t.Fatalf("helper file %q was not created", path) +} diff --git a/internal/fileops/confined.go b/internal/fileops/confined.go index ebff677..c3ba75d 100644 --- a/internal/fileops/confined.go +++ b/internal/fileops/confined.go @@ -184,15 +184,21 @@ func syncOpenedDirectory(parent *os.Root) error { } // OpenFileConfined opens a file after verifying its parent hierarchy without -// following symbolic links. Existing symbolic-link leaves are rejected. +// following symbolic links. Existing symbolic-link and non-regular leaves are +// rejected. func OpenFileConfined(path string, flags int, mode os.FileMode) (*os.File, error) { parent, name, err := openConfinedParent(path, false, 0) if err != nil { return nil, err } defer func() { _ = parent.Close() }() - if info, err := parent.Lstat(name); err == nil && info.Mode()&os.ModeSymlink != 0 { - return nil, fmt.Errorf("destination file %q is a symbolic link", name) + if info, err := parent.Lstat(name); err == nil { + if info.Mode()&os.ModeSymlink != 0 { + return nil, fmt.Errorf("destination file %q is a symbolic link", name) + } + if !info.Mode().IsRegular() { + return nil, fmt.Errorf("destination file %q is not a regular file", name) + } } else if err != nil && !errors.Is(err, os.ErrNotExist) { return nil, fmt.Errorf("inspect destination file %q: %w", name, err) } @@ -206,7 +212,7 @@ func OpenFileConfined(path string, flags int, mode os.FileMode) (*os.File, error return nil, fmt.Errorf("inspect opened destination file %q: %w", name, err) } current, err := parent.Lstat(name) - if err != nil || current.Mode()&os.ModeSymlink != 0 || !os.SameFile(opened, current) { + if err != nil || !opened.Mode().IsRegular() || current.Mode()&os.ModeSymlink != 0 || !current.Mode().IsRegular() || !os.SameFile(opened, current) { _ = file.Close() if err != nil { return nil, fmt.Errorf("reinspect destination file %q: %w", name, err) diff --git a/internal/fileops/confined_open_test.go b/internal/fileops/confined_open_test.go new file mode 100644 index 0000000..c17b35d --- /dev/null +++ b/internal/fileops/confined_open_test.go @@ -0,0 +1,56 @@ +package fileops + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +func TestOpenFileConfinedRejectsNonRegularLeaf(t *testing.T) { + leaf := filepath.Join(t.TempDir(), "diagnostic.log") + if err := os.Mkdir(leaf, 0o700); err != nil { + t.Fatalf("Mkdir() error = %v", err) + } + file, err := OpenFileConfined(leaf, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, WorkspaceFileMode) + if file != nil { + _ = file.Close() + t.Fatal("OpenFileConfined() returned a file for a directory") + } + if err == nil || !strings.Contains(err.Error(), "not a regular file") { + t.Fatalf("OpenFileConfined() error = %v, want regular-file rejection", err) + } +} + +func TestOpenFileConfinedRejectsSymlinkWithoutTruncatingTarget(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("creating symlinks requires privileges that are not available on every Windows runner") + } + dir := t.TempDir() + target := filepath.Join(dir, "target.log") + const original = "outside content" + if err := os.WriteFile(target, []byte(original), 0o600); err != nil { + t.Fatalf("WriteFile(target) error = %v", err) + } + leaf := filepath.Join(dir, "diagnostic.log") + if err := os.Symlink(target, leaf); err != nil { + t.Fatalf("Symlink() error = %v", err) + } + + file, err := OpenFileConfined(leaf, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, WorkspaceFileMode) + if file != nil { + _ = file.Close() + t.Fatal("OpenFileConfined() returned a file for a symbolic link") + } + if err == nil || !strings.Contains(err.Error(), "symbolic link") { + t.Fatalf("OpenFileConfined() error = %v, want symbolic-link rejection", err) + } + data, readErr := os.ReadFile(target) + if readErr != nil { + t.Fatalf("ReadFile(target) error = %v", readErr) + } + if string(data) != original { + t.Fatalf("target content = %q, want %q", data, original) + } +}