From 9c72db56e9eadaed35ee4dd9e494483a1638ca52 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 10 Aug 2026 12:42:49 +0000 Subject: [PATCH] Audit artifact paths and filesystem safety --- docs/roadmap/audit-findings.md | 411 ++++++++++++++++++++++++++++++++- 1 file changed, 402 insertions(+), 9 deletions(-) diff --git a/docs/roadmap/audit-findings.md b/docs/roadmap/audit-findings.md index ddfba7f..67448a9 100644 --- a/docs/roadmap/audit-findings.md +++ b/docs/roadmap/audit-findings.md @@ -100,7 +100,7 @@ These are prioritization signals, not findings: | 0: baseline | complete | Revision/environment pinned; graph refreshed; inventories and every prescribed baseline command recorded. `TST-001` owns the non-blocking race limitation. | | 1: contract and boundary map | complete | Canonical contracts and focused internal docs read; ownership, stage-contract, lifecycle, scenario, area, and preliminary risk-to-test matrices recorded below. | | 2: runner and manifest | complete | Full/single-stage entry paths, every lifecycle outcome, both manifest models/transitions, save disagreement states, canonical invalidation boundaries, and runner lock lifetime reviewed. Focused app/manifest test and race commands passed. Confirmed `COR-001` and `RSK-001`; assigned `DUP-001`, `SIM-001`, `COM-001`, `TST-002`, and lock-release details to later stages. | -| 3: paths and filesystem | not_started | Assigned path, lock, artifact, and mutation questions below. | +| 3: paths and filesystem | complete | Canonical local/remote paths, every artifact source family, filesystem mutations, directory promotion, cleanup confinement, permissions, and lock mechanics reviewed. Focused normal and race commands passed. Confirmed `COR-002`, `COR-003`, `RSK-002`, `RSK-003`, and `RSK-004`; added `DUP-002` and `TST-003`, and refined `DUP-001`. | | 4: publish and cleanup | not_started | Assigned remote commit and cleanup scenarios below. | | 5: restore and previous state | not_started | Assigned restore and previous-cache scenarios below. | | 6: configuration and composition | not_started | Assigned configuration, CLI composition, and process-boundary areas below. | @@ -128,9 +128,9 @@ has been mapped but not behaviorally audited. | Analyze and artifact dependencies | `internal/stage`, `internal/artifacts`, `internal/artifactpolicy` | 10 | assigned | | Publish and cleanup | `internal/stage`, `internal/app` | 4 | assigned | | Manifest state | `internal/manifest`, transition policy in `internal/app` | 2 | reviewed | -| Artifacts, paths, and policy | `internal/artifacts`, `internal/artifactpolicy`, `internal/pathsafe` | 3 (resolution consumption revisited in 10) | assigned | +| 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 | assigned | -| File operations | `internal/fileops`, `internal/pathsafe`, local artifact store | 3 (promotion vertical slice in 9) | assigned | +| 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 | | Tests, examples, and automation | package test owners, `examples/`, `.woodpecker/` | 12 | assigned | @@ -361,6 +361,127 @@ 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. | +## Path, Artifact, Filesystem, And Lock Conclusions + +### Canonical ownership and normalization + +`internal/artifacts/paths.go` owns the campaign/session layout, run-local +layout, previous cache, Notarius bundle, spool, and audio-cache constructors. +`internal/artifacts/s3_keys.go` owns session/run/current and published-output +keys. `internal/pathsafe` is the shared lexical boundary for slash-normalized +relative destinations: it rejects empty, absolute, drive-qualified, traversal, +and leading-backslash forms, normalizes mixed separators, and verifies a +joined destination remains lexically under its root. `internal/artifactpolicy` +adds configured source/destination policy without performing filesystem I/O. + +Most production callers consume those owners directly. Previous-cache planning +normalizes configured and manifest-derived relative paths before calling the +path constructor, and restore derives a normalized relative path before +installing it. One ad hoc reconstruction remains in publish: +`resolvePublishRunManifestSource` joins the literal `manifest.json` to an +already-derived run root instead of using the canonical run-manifest helper; +`DUP-002` assigns that maintainability decision to Stage 11. + +The constructors themselves do not enforce that campaign, session, run, or +artifact-relative components are safe opaque segments. Configuration currently +checks session identifiers and campaign identity only for presence. As a +result, traversal-bearing operator identity reaches both local `filepath.Join` +and remote `path.Join`; `COR-002` records the concrete namespace escape. The +exported previous-artifact helper also promises a path under +`previous/artifacts` while accepting a traversal value, although its current +production callers validate first. That helper-level contract should be fixed +with the same boundary rather than treated as a separate defect. + +### Artifact resolution + +The complete resolution chain is deterministic and matches the documented +source-family distinctions: + +- built-ins normalize against a fixed registry, prefer matching producer + outputs from the manifest, validate content shape, then use a canonical + session fallback; missing state returns `SessionArtifactNotFoundError`; +- configured sources resolve only through explicit runtime-catalog + availability, preserve generated-versus-disabled-file provenance, validate + non-empty text, and return the same typed missing condition when planned but + unavailable; +- extraction hydration requires a successful current extract record, exact + configured definitions, producer/receipt identity, compatible contract and + external provenance, confined regular files, a complete index/lane set, and + matching checksums before making any source available; +- previous-session planning sorts requirements and records, validates relative + cache destinations, loads current remote state with expected campaign, + session, and run identity, and hydrates only explicit remote objects into the + local cache. Analyze-time resolution is then local-only, preferring a + manifest input path before the documented filesystem fallback and returning + a typed missing error; and +- current-state loading has distinct typed missing-pointer and missing-manifest + errors, rejects empty or malformed state, and validates expected campaign, + session, and run identity or pointer/manifest run consistency. + +Only extraction claims checksum/contract/external-provenance enforcement at +resolution time, and its implementation enforces all three. Built-in, +configured, and previous-cache resolvers enforce their documented content and +availability rules but do not re-hash ordinary manifest records. That is not a +Stage 3 contract mismatch; Stages 5 and 10 remain responsible for deciding +whether restore or analyze threat models require stronger trust than the +documented previous-cache fallback and manifest-aware built-in behavior. + +### Filesystem mutation mechanics + +The mutation inventory separates lexical policy from explicit-path mechanics: + +| Mechanism | Normal-process atomicity and cleanup | Confinement and durability conclusion | +| --- | --- | --- | +| `WriteFileAtomic` | Same-directory temporary file; write, file sync, close, chmod, rename; removes a failed temporary file; replaces an existing destination where the platform rename permits it. | Accepts an explicit destination as intended, but follows symlinked destination ancestors and does not sync the parent after rename. | +| `CopyFileAtomicWithChecksum` / `CopyFileAtomic` | Opens the explicit source, copies and hashes through a same-directory temporary file, syncs/closes/chmods, then renames; prior destination survives failures before rename. | Does not require a regular source, follows source and destination symlinks, and does not sync the destination parent. Current canonical materialization callers validate content but inherit destination confinement and crash-durability findings. | +| `InstallDownloadedTempFile` | Chmods and renames a caller-supplied sibling temporary file; same-filesystem placement is established by current audio, previous-cache, and restore callers. | Does not sync the downloaded file or destination directory. The object-store downloads close their path-based files but expose no completed-data sync guarantee. | +| `PromoteDirectory` | Copies to a temporary sibling, permits only regular files/directories, uses confined source handles and identity checks, syncs files/directories, atomically installs without replacement, syncs the destination parent, cleans failures, and reports unsupported platforms before creating the promotion tree. | Source replacement, source symlinks/non-regular entries, existing or concurrently-created destinations, same-filesystem placement, and platform behavior are strongly handled. Destination ancestors remain path-based and can be symlinked or replaced. | +| Cleanup removal | Refuses empty roots/targets, lexical root deletion and traversal, wrong final-node type, and a final target that is a symlink. Missing targets are idempotent. | It does not inspect root or ancestor components and performs `RemoveAll` after a path-based check, so a symlinked ancestor can redirect deletion outside the root. | + +Low-level file and storage helpers correctly accept explicit destinations and +do not infer stage, workspace, or cleanup policy. Policy belongs in their +callers. The current caller-side checks are lexical and final-node checks, +however, so they cannot prevent destination-ancestor symlink traversal or +replacement. `COR-003` consolidates the write, promotion-destination, and +destructive-cleanup variants under that single root cause; later stage and +adapter audits should reference it rather than duplicate it. + +Single-file writers preserve the prior visible file on failures before rename, +but unlike directory promotion they do not sync the containing directory after +rename. Download installs additionally do not establish a file-sync boundary. +`RSK-002` records the resulting crash/power-loss durability gap for canonical +files and manifests. Stage 3 also confirmed that `DUP-001` duplicates this +same incomplete persistence sequence across both manifest save paths. + +Runtime layout and copied/generated files use fixed requested modes `0755` and +`0644` subject to process umask, including transcripts, artifacts, logs, +manifests, and the lock. No operating contract requires a restrictive umask or +documents an ownership/mode boundary. `RSK-004` records the private-data +exposure risk rather than assuming a deployment-specific parent directory is +always restrictive. + +### Lock scenario and focused tests + +The local lock is an atomically created `O_CREATE|O_EXCL` sentinel. This is +sufficient to serialize two live contenders before either reads a manifest, +and the existing focused conflict test exercises that mechanism. It is not an +OS-owned advisory lock: any existing sentinel conflicts without interpreting +the recorded PID/time, process death cannot remove it, and recovery is manual. +Release closes then unlinks, but the runner discards its error. An unlink +failure can therefore let a command report success while every later invocation +is blocked. `RSK-003` resolves Scenario 10 and the Stage 2 release question. + +Existing focused tests strongly cover lexical mixed-slash/traversal handling, +artifact-source ordering and typed missing states, extraction trust checks, +current-state identity, ordinary atomic replacement/temp cleanup, source-side +promotion races and special files, no-replace installation, permissions, and +basic lock acquire/conflict/release. They do not cover unsafe identity segments, +destination-ancestor symlinks or replacement, cleanup through a symlinked +ancestor, directory-sync/file-sync guarantees, stale-lock ownership/recovery, +release failure, or assembled concurrent runners. `TST-003` assigns a narrow +cross-package regression set to Stage 12; each confirmed finding below also +identifies its smallest behavior-level test. + ## Confirmed Findings ### `COR-001`: session identity initialization preserves stale invocation paths and accepts conflicting identity @@ -402,6 +523,88 @@ Stage 2 test observations for this matrix: 6 owns configuration identity validation. They should reference this root finding rather than create duplicates. +### `COR-002`: unsafe identity components escape canonical local and remote namespaces + +- Category: confirmed correctness/security defect. +- Locations/invariant: `internal/artifacts/paths.go` canonical constructors, + `internal/artifacts/s3_keys.go` in `S3SessionPrefix`, and + `internal/config/validate.go` in `validateCampaign`, `validateSession`, and + `validateSessionIdentifier`. Canonical identities must be opaque safe + segments, and traversal must not broaden local or remote operation scope. +- Evidence: campaign, session, and previous-session identifiers are checked + only for non-emptiness. Path/key constructors clean and join the raw values + without rejecting `.`/`..`, separators, drive forms, or traversal. A value + such as `../../outside` therefore changes the cleaned local workspace/spool + destination and the S3 namespace. `EnsureLayoutFor` also validates only + presence before creating the derived directories. The exported + `SessionPreviousArtifactPath*` helpers similarly claim an under-root result + but accept traversal; current production callers happen to validate their + relative artifact values first. +- Realistic scenario: an operator typo, generated session config, or untrusted + restored identity containing traversal makes prepare create/copy files + outside the intended workspace or spool root. Publish can read or write a + different campaign/session prefix, potentially colliding with unrelated + remote state. +- Impact/likelihood/confidence: high local and remote integrity/confidentiality + impact; low-to-moderate likelihood because ordinary date-like IDs are safe + but configuration is operator-controlled; high confidence from direct data + flow into canonical joins. +- Estimated remediation scope and owner: small-to-medium shared + config/artifacts change. Define one strict opaque-segment contract, validate + identities before layout/key construction, and make canonical helpers fail + closed rather than return an escaped path. Decide compatibility for existing + non-segment IDs explicitly in Stage 6. +- Test changes: table-test separator, traversal, absolute/drive, dot, and mixed- + slash identities at the configuration boundary; add artifacts tests proving + no local directory or S3 key can escape its expected namespace. Add a direct + unsafe-relative test for the exported previous-artifact constructor if it + remains public. +- Dependencies: Stage 6 owns configuration compatibility and validation. + `COR-001` separately owns conflicts among otherwise valid identities; + `COR-003` owns symlink-based escape after lexical identities are safe. + +### `COR-003`: filesystem mutation confinement follows symlinked destination ancestors + +- Category: confirmed correctness/security defect. +- Locations/invariant: lexical joins in `internal/pathsafe`, path-based writers + in `internal/fileops/fileops.go`, destination setup in + `internal/fileops/directory.go`, local layout/copy operations in + `internal/artifacts/local.go`, and cleanup validation/removal in + `internal/app/cleanup_targets.go`, `clean.go`, and + `post_publish_cleanup.go`. Writes, replacements, promotions, and deletions + must remain beneath an explicit root despite symlinks or replacement races. +- Evidence: lexical `Rel` checks cannot observe filesystem links. File writers + call `MkdirAll`/`CreateTemp`/`Rename` through destination paths. Promotion + strongly confines and identity-checks its source, but only path-checks the + destination parent before creating/installing the sibling tree. Cleanup + `Lstat`s only the final target and then calls `RemoveAll`; it neither rejects + a symlinked root/ancestor nor holds a confined directory handle. A target + such as `root/campaign/session`, where `campaign` is a symlink to an outside + tree, passes the lexical and final-node checks and deletes the outside + session directory. The analogous ancestor redirects writes and promotion. +- Realistic scenario: a stale, user-created, restored, or concurrently replaced + workspace component redirects prepare/materialization into another tree; a + later `clean session` or post-publish cleanup recursively removes data there. + The same gap permits a time-of-check/time-of-use replacement of a destination + ancestor. +- Impact/likelihood/confidence: critical destructive and confidentiality impact; + low-to-moderate likelihood depending on workspace ownership and multi-user + exposure; high confidence from the path-based checks and standard symlink + resolution semantics. +- Estimated remediation scope and owner: medium shared filesystem change. Use + root-relative directory handles/no-follow component traversal (with explicit + platform behavior) for mutations and deletion, and carry validated handles + through install/remove where feasible. Keep `fileops` policy-neutral by + passing an explicit root/destination capability rather than inferring stage + policy. +- Test changes: real-filesystem tests for a symlinked root, intermediate + component, destination parent replacement, and cleanup ancestor; assert an + outside sentinel survives and no outside temporary/output is created. Retain + the existing promotion source-race suite as the model for narrow hooks. +- Dependencies: Stages 4, 5, 7-9 must reference this finding for their concrete + cleanup, restore/download, adapter, stage-materialization, and extraction + consequences. `COR-002` owns lexical identity traversal separately. + ### `RSK-001`: invocation audit records can remain indefinitely `running` - Category: confirmed correctness/operational risk. @@ -438,10 +641,113 @@ Stage 2 test observations for this matrix: status/restore presents abandoned runs; Stage 13 should decide whether true process interruption is accepted risk after handled errors are fixed. +### `RSK-002`: single-file atomic replacement is not crash-durable + +- Category: confirmed data-durability risk. +- Locations/invariant: `internal/fileops.WriteFileAtomic`, + `CopyFileAtomicWithChecksum`, `InstallDownloadedTempFile`, and both atomic + manifest-save sequences in `internal/manifest/store.go`. Successful canonical + files and durable ledgers must survive a crash/power-loss boundary consistent + with reported success. +- Evidence: writers that create their own temporary file sync its data before + rename, but none syncs the containing directory after rename. Download + callers close an initially empty sibling temp before object-store download, + and neither the download interface nor `InstallDownloadedTempFile` syncs the + completed file before rename. Directory promotion already demonstrates the + stronger sequence by syncing copied files, temporary directories, and the + destination parent after no-replace rename. +- Realistic scenario: a command reports a saved session/run manifest or + materialized canonical output, then the host loses power. The directory entry + rename is not durable and can disappear or expose filesystem-dependent state; + a downloaded restore/previous/audio file has an additional unsynced-data + window. +- Impact/likelihood/confidence: high integrity/recovery impact; low likelihood + per invocation but cumulative operational exposure; high confidence that the + sync calls are absent, with exact failure manifestation filesystem-dependent. +- Estimated remediation scope and owner: small-to-medium shared fileops and + manifest change. Centralize the durable temp-file install sequence, sync + completed downloads before install, then sync the parent directory with the + same explicit platform policy used by directory promotion. +- Test changes: introduce a narrow injectable sync/rename seam or ordered + filesystem-operation fake to assert file-sync-before-rename and directory- + sync-after-rename for write, copy, download install, and both manifest types; + retain real-filesystem overwrite/temp-cleanup tests for visible atomicity. +- Dependencies: `DUP-001` is the maintainability multiplier. Stages 5 and 7 + should reference this root for restore/audio/storage download behavior rather + than create new durability findings. + +### `RSK-003`: stale sentinel locks can block a session indefinitely and release failures are hidden + +- Category: confirmed availability/operational risk. +- Locations/invariant: `internal/artifacts/local.go` lock acquisition/release, + the ignored deferred release in `internal/app/runner.go`, and the manual stale + lock procedure in `docs/troubleshooting.md`. A live same-session invocation + must exclude competitors, while completed or dead ownership must have a safe, + observable recovery path. +- Evidence: `O_CREATE|O_EXCL` correctly serializes live contenders, but any + existing `.lock` conflicts without checking whether its recorded PID/time is + live. Process death leaves the file forever. `ReleaseSessionLock` can report + close or unlink failure, but the runner defers it as `_ = ...`; an unlink + failure can therefore be reported as command success while the next run is + blocked. Recovery requires the operator to inspect process state and manually + delete the file. +- Realistic scenario: the process is killed or the filesystem rejects unlink + after a successful long run. Every later invocation for the session fails at + acquisition until an operator notices and safely removes the sentinel. +- Impact/likelihood/confidence: medium-to-high availability impact; moderate + lifetime likelihood for interruption and low likelihood for unlink failure; + high confidence. Mutual exclusion itself is sound in the ordinary live- + process case. +- Estimated remediation scope and owner: medium artifact-store/application + change. Prefer an OS-released lock while retaining useful metadata, or define + a conservative ownership/lease protocol; surface release failures without + obscuring an earlier command error and document automated versus manual + recovery. +- Test changes: add process/concurrency coverage for live exclusion and death + recovery, an injectable close/unlink failure proving the command cannot + silently succeed, and a subsequent-acquisition assertion. Do not encode + unsafe PID reuse assumptions in a unit test. +- Dependencies: `RSK-001` covers abandoned invocation audit state, not + exclusion. Stage 5 should check operator status presentation; Stage 12 owns + the smallest durable assembled-runner case. + +### `RSK-004`: default runtime modes can expose private campaign material to other local users + +- Category: confirmed security/operational risk. +- Locations/invariant: layout and mutation modes across + `internal/artifacts/local.go`, `internal/fileops`, stage/download writers, and + manifest persistence; security contract in `docs/policy/architecture.md`. + Transcripts, prompts, artifacts, reports, logs, and manifests are private + campaign material. +- Evidence: runtime directories request `0755` and files request `0644`, subject + only to ambient process umask. The default workspace is `/var/lib/narratio`; + Narratio can create its layout with those modes, and no operations contract + requires a restrictive umask, private parent, service-user ownership, or + configurable mode policy. Fixed-mode promotion also normalizes bundle files + to `0644` and directories to `0755`. +- Realistic scenario: a service or operator runs with the common `0022` umask + on a multi-user host. Other local accounts can traverse the workspace and + read transcripts, prompts, generated artifacts, diagnostics, and manifests. +- Impact/likelihood/confidence: high confidentiality impact; environment- + dependent but realistic likelihood; high confidence in requested modes and + documentation absence, moderate confidence in exposure on any particular + deployment because parent ACLs can mitigate it. +- Estimated remediation scope and owner: medium operations/configuration and + shared-writer change. Establish secure directory/file defaults, preserve + deliberate executability where needed, define ownership/ACL/umask + expectations, and provide an explicit compatibility/migration story. +- Test changes: assert privacy-oriented effective modes under a controlled + permissive umask for representative layout, manifest, artifact, log, and + promoted-bundle paths; document platform/ACL limitations instead of assuming + POSIX bits are universal. +- Dependencies: Stage 6 owns configuration/default compatibility, Stage 7 owns + adapter diagnostics, and Stage 13 should rank the risk against deployment + assumptions. + ## Candidate Register -The remaining candidates require inspection by their named owners. Stage 2 -promoted `COR-001` and `RSK-001` into the confirmed register above. +The remaining candidates require inspection by their named owners. Stages 2 +and 3 have promoted their confirmed defects and risks into the register above. ### `ARC-001`: `IODecl` is not a complete or consistently classified stage contract @@ -513,12 +819,48 @@ promoted `COR-001` and `RSK-001` into the confirmed register above. - Evidence: `LocalStore.Save` contains its own temp/create/write/sync/close/ context-check/rename sequence, while `SaveRun` delegates the same mechanism to `writeJSONAtomically`. Error prefixes differ, but the durability mechanism - is otherwise repeated. + is otherwise repeated. Stage 3 confirmed that both copies omit the same + parent-directory sync required by `RSK-002`. - Realistic risk: a future durability, cleanup, permission, or platform fix may reach only one manifest type, creating different guarantees for the two ledgers. - Confirmation owners: Stage 3 should compare filesystem guarantees and Stage 11 should decide whether sharing the helper preserves useful error context. + The filesystem comparison is complete; the simplification decision remains + assigned to Stage 11. + +### `DUP-002`: publish reconstructs the canonical run-manifest path + +- Category: duplicated path-ownership candidate. +- Evidence: `internal/stage/publish.go` in + `resolvePublishRunManifestSource` joins the literal `manifest.json` to an + already-derived run root, while `internal/artifacts` owns + `SessionRunManifestPath*` and the run-manifest path model. The same literal is + repeated when constructing the upload-relative record. +- Realistic risk: a layout/name change can update canonical path construction + without updating publish discovery, causing a completed run to fail publish + or upload the wrong record. Current values agree, so this is not a correctness + defect at the audited revision. +- Confirmation owner: Stage 11 should decide whether publish should receive the + canonical run-manifest path from its caller/model or use an artifacts helper; + preserve the explicit remote relative name separately if it is a protocol + constant. + +### `TST-003`: filesystem safety tests omit destination and lock-recovery boundaries + +- Category: test-suite sufficiency candidate. +- Evidence: focused path/file tests cover lexical traversal, mixed slashes, + ordinary atomic overwrite/cleanup, promotion source symlinks and replacement, + no-replace installation, and basic lock conflict/release. No test covers an + unsafe identity component, symlinked destination ancestor, destination-parent + replacement, cleanup through an ancestor symlink, file/directory sync order, + stale lock recovery, lock release failure, or concurrent assembled runners. +- Realistic risk: the confirmed `COR-002`, `COR-003`, `RSK-002`, and `RSK-003` + mechanisms can regress or be only partially repaired while a broad focused + suite remains green. +- Confirmation owner: Stage 12 should select one narrow behavior-level case per + distinct invariant and reuse shared low-level tests across callers. Avoid + duplicating every path spelling or persistence call sequence. ### `SIM-001`: runner terminalization and persistence ordering lack a narrow owner @@ -557,8 +899,11 @@ promoted `COR-001` and `RSK-001` into the confirmed register above. | Coverage percentages | deferred diagnostic signals, not findings | Stage 12 must reason from risk and test ownership, not a numeric target. | | Session `last_error` survives a later stage success | rejected as a current-state defect at Stage 2 | No production reader was found; current status and per-stage error are authoritative, so the field can serve as historical context. Reopen only if an operator surface treats it as the active error. | | Minimal loaded-manifest status/timestamp validation | rejected as a standalone Stage 2 finding | Unknown/non-succeeded statuses fail conservatively into execution, nil maps/records are normalized, and no realistic unsafe caller was established. Configured-versus-persisted identity conflict is separately confirmed in `COR-001`. | -| Ignored runner lock-release error | assigned to Stage 3 | The application discards the error, but whether close/unlink failure preserves an OS lock or merely a harmless lock file depends on the local-store implementation and platform semantics. | +| Ignored runner lock-release error | confirmed as `RSK-003` at Stage 3 | The lock is an exclusive-create sentinel, not an OS-released lock. An unlink failure leaves the conflicting file while the runner suppresses the error; process death does the same without a release attempt. | | Two durable meanings of “skip” | runner portion confirmed, stage-contract conclusion deferred | Run action/status distinguishes idempotent skip from executed self-skip. Stages 4 and 8 must determine whether successful disabled outcomes and their wording are intentional. | +| Exported previous-artifact helper accepts traversal | consolidated into `COR-002` | Current production callers normalize first, but the helper's under-root contract is false in isolation. The identity/relative-segment boundary should be repaired once rather than as separate caller bugs. | +| Built-in and previous resolvers do not re-hash manifest records | documented trust distinction; deferred to Stages 5 and 10 | Extraction explicitly requires checksum/contract/provenance validation and enforces it. Other source families explicitly use content validation and a previous-cache filesystem fallback; consumer/restore threat models must establish a stronger requirement before this becomes a finding. | +| Promotion destination is path-based while source is handle-confined | consolidated into `COR-003` | Source hardening is strong, but destination ancestors share the same symlink/replacement root cause as ordinary writers and cleanup. | ## Unresolved Questions And Follow-Up @@ -566,8 +911,17 @@ promoted `COR-001` and `RSK-001` into the confirmed register above. them interrupted, or retain them as an explicitly accepted audit limitation? - Do Stage 3 path mechanics and Stage 4 publish/cleanup behavior confirm every concrete consequence of the stale run-derived fields in `COR-001`? -- Can any `ReleaseSessionLock` error retain exclusion or make a subsequent - acquisition unsafe, and how should the runner surface such a deferred error? +- Which cross-platform root-relative filesystem primitive should own the + symlink-safe mutation capability required by `COR-003` without moving stage + policy into low-level `fileops`? +- Should sentinel locking be replaced by an OS-released lock, or can a + conservative ownership/recovery protocol satisfy `RSK-003` across supported + platforms? +- Which existing identifier spellings constrain the strict segment validation + required by `COR-002`? +- Do deployment packaging and service definitions already enforce an umask, + ownership, or ACL that reduces `RSK-004`, and where should that contract be + documented and tested? - Are disabled render/analyze/publish outcomes intentionally successful so pipeline continuation and optional outputs work, and do all operator views describe that distinction accurately? @@ -640,3 +994,42 @@ No accepted risks or final audit conclusions are recorded yet. intentionally retain diagnosis data while running/failure/skip clear it; session authority makes all enumerated disagreement states retry or reuse conservatively; the runner's two skip forms are durably distinguishable. + +### Stage 3 + +- Contracts and code reviewed: architecture/path/security policy; internal + artifacts, workspace, manifest, operations, and troubleshooting contracts; + all canonical local/S3/cache constructors; pathsafe and artifactpolicy; + built-in, configured, extraction, previous, and current-state resolution; + local-store layout/copy/lock code; atomic write/copy/download installation; + directory promotion and platform-specific no-replace/directory-sync support; + restore/audio/previous download callers; manual and post-publish cleanup. +- Graph/source evidence: canonical-helper and direct-mutation inventories; + callers of path/key, artifact-resolution, fileops, current-state, previous- + cache, and lock helpers; focused test-function inventories; fallback text + search for non-code policy and direct OS mutation sites where graph results + were insufficient. +- Validation: `go test -count=1 ./internal/artifacts + ./internal/artifactpolicy ./internal/pathsafe ./internal/fileops` passed (0.98 + s wall time). `go test -race -count=1 ./internal/artifacts + ./internal/fileops` passed (2.25 s wall time). +- Conclusions: canonical owners and artifact resolution order are recorded + above; lexical normalization handles mixed separators, traversal, absolute, + and drive forms when callers invoke it; extraction source trust and source- + side promotion are strong; low-level helpers correctly consume explicit + destinations. Scenario 10 is resolved: live contenders are excluded, while + stale sentinel/release behavior is unsafe operationally. +- Findings: confirmed `COR-002`, `COR-003`, `RSK-002`, `RSK-003`, and + `RSK-004`; added `DUP-002` and `TST-003`; refined `DUP-001` with the shared + durability gap. +- Explicit no-finding conclusions: current-state helpers have typed missing + cases and strict identity checks; artifact resolution is deterministic and + matches its documented source-specific validation; promotion preserves an + existing/concurrent destination and rejects unsafe source trees; temporary + files/trees are cleaned on ordinary failures; unsupported promotion + platforms fail before creating a durable bundle; fileops does not infer + higher-level policy. +- Follow-up: Stages 4-10 should cite the shared confinement/durability roots for + concrete callers. Stages 5 and 10 must decide whether ordinary manifest and + previous-cache checksum trust is sufficient. Stages 6, 11, and 12 own + compatibility, simplification, and durable regression coverage respectively.