Document restore and previous-state audit findings

This commit is contained in:
2026-08-10 13:16:35 +00:00
parent e3ee3de10a
commit 39afe644eb

View File

@@ -102,7 +102,7 @@ These are prioritization signals, not findings:
| 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 | 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 | complete | Publish prerequisites/source families, deterministic upload order, every partial remote outcome, retry semantics, effective locks, status/restore interpretation, and automatic/manual cleanup gates reviewed. Focused stage/app/artifacts/storage tests passed. Confirmed `COR-004` through `COR-007` and `RSK-005`; added `ARC-003`, `COM-002`, and `TST-004`. |
| 5: restore and previous state | not_started | Assigned restore and previous-cache scenarios below. |
| 5: restore and previous state | complete | Restore discovery/planning/execution/reporting, remote-current identity and scope, every local failure boundary, audio cache/spool identity, previous-cache planning/consumption, and status/validate policy reviewed. Focused app/previouscache/audio/artifacts/storage tests passed. Confirmed `COR-008` through `COR-011`, `RSK-006` through `RSK-008`, and `EFF-001`; added `DUP-003` and `TST-005`. |
| 6: configuration and composition | not_started | Assigned configuration, CLI composition, and process-boundary areas below. |
| 7: adapters and shared support | not_started | Assigned external-boundary and cancellation areas below. |
| 8: ordinary stages | not_started | Assigned prepare/transcript behavior and disabled-outcome questions below. |
@@ -129,7 +129,7 @@ has been mapped but not behaviorally audited.
| Publish and cleanup | `internal/stage`, `internal/app` | 4 | reviewed |
| 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) | reviewed |
| Restore | `internal/app`, `internal/artifacts`, `internal/previouscache`, `internal/audio` | 5 | assigned |
| Restore | `internal/app`, `internal/artifacts`, `internal/previouscache`, `internal/audio` | 5 | reviewed |
| File operations | `internal/fileops`, `internal/pathsafe`, local artifact store | 3 (promotion vertical slice in 9) | reviewed |
| External adapters and storage | `internal/adapters`, `internal/audio` | 7 | assigned |
| Shared models and diagnostics | `internal/artifactmodel`, `internal/contracts`, `internal/logging` | 7 (maintainability revisited in 11) | assigned |
@@ -532,11 +532,14 @@ again and overwrites the same destinations. Partial run/session objects are not
removed. `COR-001` separately means a later invocation can combine a new run ID
with a stale remote run prefix.
`LoadCurrentState` reads the pointer first, then the fixed manifest, and rejects
pointer/manifest run disagreement. Status reports that error as remote publish
missing or unavailable; restore returns an error. Thus the pointer is the only
accepted commit marker, but overwriting the fixed manifest before it does not
preserve the prior readable commit. `COR-004` records that distinction.
`LoadCurrentState` reads the pointer first, then the fixed manifest, and can
reject pointer/manifest run disagreement when its caller requests run
validation. Previous-cache planning does request it; Stage 5 found that the
shared restore/status discovery path does not. Thus the pointer is the intended
commit marker, but `COR-008` confirms that two primary readers can accept a
manifest from a different run. Independently, overwriting the fixed manifest
before the pointer does not preserve the prior coherent pair; `COR-004` owns
that publish-side root cause.
| Publish boundary or durable remote state | Remote-current interpretation | Retry and cleanup consequence |
| --- | --- | --- |
@@ -544,7 +547,7 @@ preserve the prior readable commit. `COR-004` records that distinction.
| A run, published-output, or previous-cache upload fails | Earlier objects remain or overwrite existing keys; the prior current pair is still intact because the fixed manifest was not reached. With no prior pair, no current state exists. | Retry unconditionally reuploads the whole plan. Cleanup is ineligible. |
| Current-manifest construction fails | All planned data objects may exist, but the prior current pair remains intact. | Same retry behavior; cleanup is ineligible. |
| Current-manifest upload definitely fails before remote acceptance | The prior pair remains intact; no pointer attempt occurs. | Same retry behavior; cleanup is ineligible. |
| New current manifest is accepted while the old pointer remains | First publish has a manifest but no pointer and is not current. A replacement publish has old pointer/new manifest disagreement, so status and restore reject it rather than expose either run as current. | A successful retry repairs the pair; cleanup is ineligible until then. This includes the ordinary window between the final two uploads. |
| New current manifest is accepted while the old pointer remains | First publish has a manifest but no pointer and restore fails on the missing pointer. A replacement publish has old pointer/new manifest disagreement: previous-cache loading rejects it, but restore/status discovery accepts it under `COR-008`. | A successful retry repairs the pair; cleanup is ineligible until then. This includes the ordinary window between the final two uploads. |
| Pointer upload returns an error | If not accepted, the disagreement above remains. If the service accepted the write but the response was lost, remote current is committed although local publish is marked failed. The interface cannot distinguish these outcomes. | Retry is conservative and overwrites again; automatic cleanup does not run for the failed local outcome. |
| Pointer upload succeeds | Pointer and manifest identities agree and the new run is current. | The stage returns `uploaded=true` and `current_pointer_written=true`; automatic cleanup may become eligible. |
| Remote commit succeeds, then terminal session/run persistence fails | Remote current remains committed. A session-save failure leaves local publish running and causes republish; a run-save failure leaves session publish succeeded and ordinary retry skips it. | The latter boundary can permanently miss automatic cleanup under `COR-006`; remote state itself remains authoritative. |
@@ -600,6 +603,162 @@ workspace-cleanup metadata durability, retry failed cleanup, or model concurrent
remote lock writers. `TST-004` assigns the smallest stateful protocol cases to
Stage 12.
## Restore, Audio, And Previous-State Conclusions
### Discovery authority and caller policy
Restore resolves configuration and storage, then delegates pointer/manifest
loading to `artifacts.LoadCurrentState`. Campaign and session expectations are
always checked. The helper's run check is optional, however: previous-cache
planning and previous-readiness inspection set `ValidateRunID=true`, while
restore/status discovery does not. Missing pointer or manifest is fatal to
restore, displayed non-fatally by status, and is skipped only when every
previous-artifact requirement is optional. Malformed or inconsistent previous
state remains an error even for optional requirements. Those distinct missing-
state policies are appropriate; omitted restore/status run validation is part
of the broader snapshot defect in `COR-008`.
Restore then lists the entire session prefix instead of deriving its file set
from the discovered manifest. It maps the fixed current manifest to local
`manifest.json`, includes `transcripts/**` and `artifacts/**`, optionally
includes `audio/**`, and excludes current/run archives, logs, reports, generated
configuration, inputs, and the current session's archived `previous/**`.
Required previous-session cache objects are planned separately from that prior
session's current manifest. Lexical path normalization and root-relative joins
reject traversal and produce deterministic local paths; `COR-002` still owns
unsafe identity components and `COR-003` owns filesystem-link confinement.
Prefix-wide listing is not a committed snapshot. Failed publish objects, stale
destinations left by older runs, and locked/unselected leftovers can all enter a
restore even when absent from the accepted manifest. Execution later downloads
the same mutable keys again, and downloaded current/previous manifests are not
revalidated against the discovered run ID. `COR-008` consolidates these
scope/version/run-binding failures and corrects the Stage 4 reader truth table.
### Planning, force, dry-run, and local serialization
Actions sort by local relative path and then remote key. Classification is:
| Local target and remote metadata | Without force | With force |
| --- | --- | --- |
| Missing | `download` | `download` |
| Directory where a file is expected | `conflict` | Still `conflict`; command-level handling is defective under `COR-009`. |
| Audio with positive remote size and equal local size | `skip_same` without content comparison | Same `skip_same`; force does not refresh it. |
| Audio size mismatch or unavailable remote size | `conflict` | `download` |
| Non-audio positive-size mismatch | `conflict` | `download` |
| Non-audio equal/unknown size and equal downloaded checksum | `skip_same` | `skip_same` |
| Non-audio equal/unknown size and different downloaded checksum | `conflict` | `download` |
Normal differing files therefore require explicit force and identical files
remain untouched. Audio's size-only shortcut and cache validation are the
integrity risk in `RSK-007`. Final symlinks are followed by `os.Stat` and can be
classified as same; ancestor/final-link trust should be repaired with the shared
filesystem capability required by `COR-003` rather than with restore-only
lexical checks.
Dry-run returns after discovery, planning, and summary rendering. It does not
create the session layout, acquire a lock, write a report, populate audio cache
or spool, install a file, or mutate remote state. It does download pointer,
manifest, and same-size non-audio bodies to system temporary files, which are
removed. Thus it is durable-session pure, not literally free of local temporary
writes. The same checksum classification can download a differing object once
during planning and again during forced execution; current and previous
manifests are also downloaded at discovery/planning and again for installation.
`EFF-001` records this avoidable I/O and the documentation precision issue.
Executable restore acquires the local session lock only after its plan is
complete. It does not reclassify `skip_same`, `conflict`, or download decisions
under the lock. Together with incremental installation and intentional lack of
rollback, this creates the coherent-local-transition risk in `RSK-006`.
`--force` cannot bypass path joining or the local session lock, but the command
only blocks conflicts when force is false. A directory conflict therefore
survives a forced plan, is ignored by execution, and can coexist with a
successful report and newly installed manifest (`COR-009`).
### Execution, manifest-last behavior, and retry
Execution filters to `download` actions, preserves their sorted order, moves
the single current-session manifest action to the end, and rejects multiple
manifest downloads. Ordinary files download to sibling temporary files and
install by rename. The manifest temp is decoded and checked for requested and
discovered campaign/session before rename, but not for the discovered run ID.
Audio delegates to the shared spool/cache materializer. A successful execution
then writes `reports/restore-latest.json`; the report is diagnostic and is the
only intended write after manifest installation.
| Failure boundary | Durable local result and retry behavior |
| --- | --- |
| Discovery or planning | No session layout/report/restored file is written; system temporary reads are cleaned. Fix remote/config state and retry. |
| Dry-run | No durable session mutation; reported actions are recomputed on apply. |
| Layout or lock acquisition | Layout creation can precede a lock failure, but no planned file or report is installed. Retry after lock recovery. |
| Unforced conflicts | No planned file is installed; a failed conflict report is persisted under the lock. Resolve or retry with force. |
| Non-manifest download/install | Earlier installs remain; the old local manifest remains; the failing temp is removed and a failure report is attempted. Retry reclassifies completed files as same. |
| Audio download/validation/materialization | Download failure removes its temp and preserves any prior spool destination; a post-download validation failure can leave the newly installed invalid spool file. A destination copied before cache-population failure also remains. Retry overwrites/reuses those states deterministically. |
| Manifest download/decode/identity/install | All earlier files remain; failures before rename preserve the prior local manifest. Retry must complete the remaining plan; forced partial replacement can make the old manifest describe changed files (`RSK-006`). |
| Manifest installed, report write fails | Restored durable state is installed and authoritative despite command failure; report may be absent/stale. Retry normally classifies files as same and can recreate the report. |
| Report succeeds, summary write fails | Restore state and success report remain complete; only command output failed. |
There is no rollback, transaction marker, or runner check for an incomplete
restore. Manifest-last prevents a new remote manifest from being installed
before its files, but it cannot keep the old local manifest coherent after a
forced partial overwrite. Individual rename visibility is good in an ordinary
process; `RSK-002` remains the shared crash-durability gap for downloaded-file,
manifest, and report installation.
### Audio and previous-cache identity
`audio.MaterializeS3Audio` is correctly shared by prepare and restore. Its cache
namespace includes bucket and full object key. A cache miss downloads through a
sibling spool temp, validates non-empty/expected size, atomically copies to the
destination with a computed checksum, then optionally populates cache. A cache
hit avoids storage and copies directly. The stored ETag and computed checksum
are not bound to cache validity: any nonempty cache file of expected size is
accepted, and existing restore audio of equal size is skipped without reading
either body. `RSK-007` records stale/corrupt same-size reuse. Failed downloads
clean temporary files, while post-install validation/cache failures leave the
explicit partial states in the table above.
Previous requirements come only from enabled configured artifacts, deduplicate
by artifact name with required winning, and sort deterministically. With no
previous session ID, required requirements fail and optional ones skip. With an
ID, planning strictly validates campaign/session/pointer-run identity, always
maps the previous current manifest into `previous/manifest.json`, and maps
available artifact objects beneath the current session's `previous/` cache.
Missing pointer/manifest or artifact objects fail required requirements and
skip optional ones. Prepare consumes the same plan but intentionally
overwrites its managed cache and records checksums/inputs; restore applies local
conflict policy. Analyze later resolves these files locally without storage,
preferring a matching main-manifest input and otherwise using the deterministic
cache path.
The remote manifest does not retain a source-to-destination map for ordinary
published outputs. Previous planning tries the producer's local relative path,
then any `published_paths` entry with the same basename, then the current
artifact output path. A custom publish destination with a different basename is
unresolvable; duplicate basenames are ambiguous. `COR-011` owns that identity
loss. Separately, status/validate only prove that the prior current pair exists:
they neither apply `BuildPlan`'s artifact-object checks nor its optional missing-
ID policy, so they can report missing optional state as an error or missing
required objects as ready (`COR-010`).
Restored manifests preserve remote host-local absolute path fields. Top-level
run/work/spool fields trigger the already confirmed `COR-001` on the next
invocation. Output/input records are also trusted preferentially when their old
absolute path happens to exist, allowing later consumers to read outside the
new workspace instead of the restored canonical copy; `RSK-008` records this
distinct restored-reference risk.
Focused tests strongly cover typed current-state failures when run validation
is enabled, restore campaign/session mismatch, default/include-audio mapping,
traversal rejection, normal same/conflict/force actions, deterministic previous
requirements, required/optional missing remote state, cache miss/hit/refresh,
download cleanup, manifest-last validation, conflicts, lock failure, reports,
and restore-to-run/analyze workflows. They omit the exact committed-snapshot,
forced-directory, plan-under-lock race, partial forced rollback, same-size audio
mutation, custom publish destination, optional operator-readiness, and foreign
absolute-path cases above. `TST-005` assigns one stable behavior test per root
invariant to Stage 12.
## Confirmed Findings
### `COR-001`: session identity initialization preserves stale invocation paths and accepts conflicting identity
@@ -731,17 +890,19 @@ Stage 12.
be the sole commit point, and work before it must not destroy the previously
committed state.
- Evidence: publish unconditionally replaces the fixed
`current/manifest.json`, then uploads `current/run_id.txt`. Current-state
loading reads the pointer and fixed manifest and rejects unequal run IDs. A
replacement publish that uploads the new manifest but has not yet written,
or fails to write, the pointer therefore leaves old pointer/new manifest
disagreement. Existing tests prove no pointer call follows selected failures
but do not seed and preserve a prior current pair.
`current/manifest.json`, then uploads `current/run_id.txt`. A replacement
publish that uploads the new manifest but has not yet written, or fails to
write, the pointer therefore leaves old pointer/new manifest disagreement.
The shared loader can reject that pair, but only when the caller enables run
validation; restore/status do not (`COR-008`). Existing publish tests prove no
pointer call follows selected failures but do not seed and preserve a prior
current pair.
- Realistic scenario: run A is current. Publishing run B reaches the current
manifest, then the pointer upload fails or status/restore runs during the
gap. Run A is no longer readable, run B is not committed, and both operator
workflows report remote state unavailable until a later successful publish.
An upload error after server acceptance also makes local outcome ambiguous.
manifest, then the pointer upload fails or a reader runs during the gap. Run
A's coherent pair is gone and run B is not committed. Strict previous-cache
readers report unavailable state, while restore/status can incorrectly accept
run B's manifest under run A's pointer. An upload error after server
acceptance also makes local outcome ambiguous.
- Impact/likelihood/confidence: high recovery availability and integrity impact;
upload failure likelihood is low per call but the disagreement window occurs
on every replacement publish; high confidence from fixed keys, unconditional
@@ -860,6 +1021,154 @@ Stage 12.
competing cleanup state machines. `RSK-002` remains the crash-durability
guarantee of the eventual manifest save.
### `COR-008`: restore is not bound to the pointer-selected committed remote run
- Category: confirmed correctness/recovery defect.
- Locations/invariant: `internal/app/restore_discovery.go`,
`restore_plan.go`, and `restore_execute.go`; shared validation in
`internal/artifacts/current_state.go`; previous object selection in
`internal/previouscache`. Restore and status must interpret only the run
committed by the current pointer, and one restore must install one coherent
remote snapshot.
- Evidence: restore/status discovery passes campaign/session expectations but
omits `ValidateRunID`, so old-pointer/new-manifest disagreement is accepted.
The planner then lists every `transcripts/**` and `artifacts/**` object under
the mutable session prefix instead of limiting actions to manifest-declared
published/locked state. Execution downloads those keys and the fixed manifest
again; manifest validation checks campaign/session but not the discovered run
ID or bytes. Previous planning enables pointer/run validation but selects and
later downloads mutable published keys without generation binding. The unit
mismatch test exercises the helper only with `ValidateRunID=true`.
- Realistic scenario: run A is current. A failed run B publish replaces the
fixed manifest and uploads one artifact but never advances the pointer.
Restore reports run A, accepts manifest B, includes B's partial artifact plus
any stale prefix objects, and installs them locally. A concurrent successful
publish can similarly change fixed objects between discovery, planning, and
execution, producing a mixed A/B restore.
- Impact/likelihood/confidence: critical recovery integrity impact; the
disagreement window occurs on every replacement publish and partial objects
are retained by design, while concurrent/failed publish likelihood is
low-to-moderate; high confidence from validation flags, list scope, and
repeated unconditional downloads.
- Estimated remediation scope and owner: medium-to-large publish/restore/
artifacts protocol change. Pair `COR-004`'s immutable run-specific commit
representation with restore planning from explicit manifest source-to-object
records, bind object versions/digests through execution, validate run identity
everywhere, and preserve an explicit compatibility policy for old manifests.
Storage should expose only the narrow conditional/version metadata the
protocol requires, not infer current-state policy.
- Test changes: seed pointer A with manifest/object state A plus uncommitted and
stale B keys; assert restore/status reject mismatch and restore only declared
A objects. Use barriers/versioned fake objects to change pointer, manifest,
ordinary output, and previous output at every discovery/execute boundary and
prove the result is all one run or fails before manifest installation.
- Dependencies: `COR-004` is the publish-side fixed-pair root; both should share
one protocol repair. `COR-011` separately owns the missing source/destination
identity needed to construct an explicit restore set. Stage 7 owns transport-
level version/checksum feasibility.
### `COR-009`: forced restore ignores unresolved directory conflicts and can report success
- Category: confirmed correctness defect.
- Locations/invariant: `internal/app/restore_plan.go` in
`classifyRestoreAction`, `restore.go` conflict gating, and
`restore_execute.go` download filtering. Force may authorize file replacement,
but it must not turn an unexecutable conflict into silent success.
- Evidence: a local directory where a remote file is expected is always
classified `conflict`, even with force. The command blocks conflicts only
when `!force`; execution processes only `download` actions and silently
ignores the remaining conflict. It can then install the current manifest,
mark the report succeeded, and leave the directory in place. Existing force
tests cover differing regular files only.
- Realistic scenario: `artifacts/session_recap.md` is accidentally a directory.
The operator reviews the conflict and reruns with `--force`. Restore exits
successfully, its report still contains a conflict action, and the new
manifest is installed although the required artifact was never restored.
- Impact/likelihood/confidence: high local integrity/operator-trust impact;
low-to-moderate likelihood from damaged or manually edited workspaces; high
confidence from the action and command branches.
- Estimated remediation scope and owner: small application-policy change.
Require zero conflicts before execution regardless of force, or explicitly
define and safely implement directory replacement as a separate destructive
action. Successful reports must be impossible while any conflict remains.
- Test changes: cover a directory at ordinary, previous-cache, and manifest
targets with force; assert failure, old manifest preservation, and a failed
report. One table-driven application test can own all target categories.
- Dependencies: target removal would require `COR-003`'s confined deletion
capability. Do not implement ad hoc `RemoveAll` in restore.
### `COR-010`: status and validation do not verify previous-artifact readiness
- Category: confirmed correctness/operator defect.
- Locations/invariant: `internal/app/operator_inspection.go` in
`inspectPreviousArtifactReadiness`, `operator_status.go`, and
`operator_session_validate.go`; canonical planning in
`internal/previouscache.BuildPlan`. Operator readiness must match the
required/optional and object-resolution policy that prepare/restore will use.
- Evidence: inspection checks only that `previous_session_id` is nonempty and
its remote current pair validates. It reports every requirement ready without
resolving candidate keys or calling `Exists`. It also marks a missing ID as
unavailable/error whenever any previous requirement exists, although
`BuildPlan` correctly skips that state when all are optional. Focused operator
tests cover a missing pointer for a required fixture, not missing artifact
objects or optional requirements.
- Realistic scenario: a previous session has a valid committed manifest but the
required published recap was never uploaded or was removed. `session status`
says ready and `session validate` succeeds; restore/prepare then fails. In the
opposite case, an optional previous recap with no previous ID makes validation
fail even though pipeline execution would intentionally omit it.
- Impact/likelihood/confidence: medium operator and automation correctness
impact; moderate likelihood as optional/missing published outputs are normal
modeled states; high confidence from the inspection shortcut and canonical
plan branches.
- Estimated remediation scope and owner: small-to-medium application/
previouscache change. Share one read-only requirement-resolution result from
`BuildPlan` (or a narrower readiness API), then let status remain non-fatal and
validation choose finding severity without duplicating required/optional
semantics.
- Test changes: add required and optional matrices for missing ID, missing
pointer/manifest, missing candidate object, custom destination, and ready
state. Own resolution cases in `previouscache`; sample only status rendering
and validation exit classification in app tests.
- Dependencies: `COR-011` must be fixed for readiness to recognize every valid
custom destination. `COR-008` owns remote version binding, not caller severity.
### `COR-011`: previous-cache planning loses source identity for custom publish destinations
- Category: confirmed correctness defect.
- Locations/invariant: `internal/previouscache/previouscache.go` in
`artifactRelativePathCandidates`, `manifestPublishedPaths`, and candidate
selection; publish metadata in `internal/stage/publish.go`. A configured
previous-artifact source must resolve to the exact remote object that publish
committed for that source.
- Evidence: the remote manifest records only an ordered `published_paths` list,
not ordinary source-to-destination pairs. Planning starts with the analyze
output's local relative path, adds published destinations only when their
basename matches, and finally tries the current artifact output path. A custom
destination with a different basename is therefore invisible. Multiple
destinations with the same basename are candidates for the wrong source, and
the first existing key wins. Tests use identical output/published paths.
- Realistic scenario: `session_recap` is produced at
`artifacts/session_recap.md` and intentionally published as
`history/recap-v2.txt`. A later required previous-session recap reports
unavailable although the committed object exists. With two artifacts both
published as different directories' `summary.md`, one requirement can hydrate
the other's content under the expected local cache path.
- Impact/likelihood/confidence: high cross-session artifact integrity impact;
moderate likelihood because custom publish destinations are a supported
configuration feature; high confidence from candidate construction.
- Estimated remediation scope and owner: medium publish manifest/previouscache
contract change. Persist a deterministic source/destination mapping for
uploaded and intentionally locked preserved outputs, consume it by exact
source ID, and define backward-compatible fallback behavior without basename
guessing when identity is ambiguous.
- Test changes: cover changed basename, duplicate basename, locked preserved
output, absent mapping in an old manifest, and exact source mapping. Existing
default-path/fallback tests remain as compatibility cases.
- Dependencies: this mapping is also required for the committed restore scope
in `COR-008`. Stage 6 owns configuration compatibility; Stage 10 owns later
artifact-consumer semantics, not remote identity selection.
### `RSK-001`: invocation audit records can remain indefinitely `running`
- Category: confirmed correctness/operational risk.
@@ -1037,6 +1346,148 @@ Stage 12.
cannot serialize other hosts or lock commands. `COR-004` may influence the
appropriate remote compare-and-swap capability.
### `RSK-006`: restore does not protect one coherent local transition from plan through manifest install
- Category: confirmed correctness/recovery risk.
- Locations/invariant: `internal/app/restore.go`, `restore_plan.go`, and
`restore_execute.go`; runner reuse of the session manifest. Local action
decisions and the manifest-last transition must remain coherent despite
competing local work and mid-restore failure.
- Evidence: restore discovers and classifies local paths before acquiring the
session lock, then never revalidates `skip_same` or conflicts after lock
acquisition. Another completed runner/restore or local edit in that window can
make the plan stale. During execution, files install incrementally with no
rollback or incomplete marker. A forced failure before manifest installation
leaves the old successful manifest in place even though some files it governs
were overwritten with remote content. Later runners do not inspect the failed
restore report.
- Realistic scenario: planning marks a transcript `skip_same`; another runner
acquires/releases the lock and changes it; restore then acquires the lock,
skips the stale decision, and installs the remote manifest. Or forced restore
overwrites that transcript, fails on a later artifact, and releases the lock
with the old manifest still claiming success over changed content. A normal
run can then reuse that manifest instead of completing restore.
- Impact/likelihood/confidence: high pipeline integrity impact; low likelihood
per restore but realistic under operator concurrency or recovery from damaged
storage; high confidence in ordering/no-revalidation, with exact external edit
likelihood environment-dependent.
- Estimated remediation scope and owner: medium-to-large application/filesystem
change. Acquire the session lock before executable classification (dry-run can
remain unlocked/read-only), revalidate under lock, and make incomplete forced
restore observable to the runner. Consider staging a complete tree or a
durable restore transaction marker rather than attempting broad rollback.
- Test changes: deterministic barriers around plan/lock plus a failure after one
forced install; assert stale skip decisions cannot commit and ordinary runner
reuse is blocked until retry completes. Preserve the current manifest-last and
retry-idempotency tests.
- Dependencies: `RSK-003` governs stale local lock recovery; `COR-003` and
`RSK-002` govern confined and crash-durable installation. `COR-008` is the
analogous remote snapshot problem.
### `RSK-007`: audio restore and cache hits use size as content identity
- Category: confirmed correctness/data-integrity risk.
- Locations/invariant: `internal/app/restore_plan.go` audio classification and
`internal/audio/s3_audio.go` cache validation. Reused audio must correspond to
the intended remote object generation, not merely have a plausible length.
- Evidence: existing restore audio with the same positive size is
`skip_same` without a body/checksum/ETag comparison, even under force. Cache
validity accepts any nonempty non-directory path and, when available, equal
size. Although `ObjectInfo.ETag` is carried into the materializer and newly
copied files compute a checksum, neither participates in later cache identity.
With unknown remote size, any nonempty cache entry is accepted. A focused test
deliberately proves same-sized different strings skip without download.
- Realistic scenario: an S3 audio key is replaced with corrected audio of the
same byte length, or a cache file is corrupted without changing length.
Restore/prepare silently reuse the old bytes; transcription proceeds from
audio that no longer matches remote operator intent.
- Impact/likelihood/confidence: high downstream content-integrity impact;
low-to-moderate likelihood for same-size replacement/corruption; high
confidence in the comparison rules. The shortcut is an evident performance
tradeoff, so classification as risk rather than certain wrong output for every
cache hit is appropriate.
- Estimated remediation scope and owner: medium audio/storage metadata change.
Bind cache entries to a stable object generation or trustworthy checksum via
sidecar metadata, validate regular-file/no-follow status, and define multipart
ETag limitations. Force should refresh or verify when explicitly requested.
- Test changes: same-size remote replacement, corrupt same-size cache, unknown-
size cache, ETag/generation change, and force semantics. Keep the current hit/
miss/invalid-size cases as lower-cost mechanism coverage.
- Dependencies: Stage 7 owns which S3 metadata is trustworthy and portable.
`COR-003` owns symlink-based cache/path escape; do not solve identity with
path checks alone.
### `RSK-008`: restored manifests retain foreign absolute paths that later consumers prefer
- Category: confirmed correctness/security risk.
- Locations/invariant: manifest installation in
`internal/app/restore_execute.go`, path preservation in `internal/manifest`,
and manifest-first resolution in `internal/artifacts/artifact_resolver.go`.
Restored state should resolve to the selected local workspace unless a trusted
external path contract explicitly authorizes otherwise.
- Evidence: restore installs current manifest bytes unchanged. Published
manifests normally contain absolute producer-local output/input paths plus
top-level work/spool/run paths. Artifact resolution returns an absolute
manifest path unchanged and prefers it over the canonical restored fallback
whenever it exists and validates. The extraction round-trip test deliberately
preserves `/prior/workspace/...` metadata but does not run a consumer against
an existing foreign path. Top-level fields already trigger `COR-001` on the
next invocation.
- Realistic scenario: restore moves a session to a host or workspace where the
old absolute path exists with stale or attacker-controlled transcript content.
Analyze/publish reads that file instead of the restored canonical copy. Even
when it does not exist, session identity retains wrong run/work/spool metadata
and can direct later work through `COR-001`.
- Impact/likelihood/confidence: high integrity/confidentiality impact; low-to-
moderate likelihood because path existence depends on host/layout reuse; high
confidence in preservation and resolver precedence.
- Estimated remediation scope and owner: medium restore/artifact/manifest
change. Separate portable logical references from host-local diagnostics,
rebase or reject restored absolute paths outside the selected session root,
and preserve original values only as bounded provenance if useful.
- Test changes: restore a real manifest with built-in/configured input/output
records from a different root, create a conflicting outside sentinel, and
assert downstream resolution uses the restored canonical path. Extend
`COR-001` tests for top-level restored identity.
- Dependencies: `COR-001` owns stale top-level run identity; `COR-003` owns
filesystem symlink confinement. Stage 10 should reuse this result when
reviewing manifest-first artifact trust.
### `EFF-001`: restore repeatedly downloads the same objects during planning and execution
- Category: confirmed efficiency and clarity issue.
- Locations/invariant: `internal/artifacts.LoadCurrentState`,
`internal/app/restore_plan.go` checksum classification,
`internal/previouscache.BuildPlan`, and `restore_execute.go`. Recovery should
avoid redundant remote transfer while preserving conflict and snapshot
correctness.
- Evidence: discovery downloads the current manifest, planning can download it
and every same-size non-audio object again for checksum comparison, and
execution re-downloads every forced differing object plus current/previous
manifests for installation. `RestorePlanOptions.DryRun` is otherwise unused,
so dry-run performs the same temporary body downloads for classification.
Temporary files are cleaned and no durable session write occurs, but the
focused internal document's “performs no local writes” wording obscures these
system-temp writes.
- Realistic scenario: a forced restore of several large same-sized artifacts
downloads each body to decide it differs, discards it, then downloads it again
to install. High-latency or metered storage doubles transfer and lengthens the
interval exposed to `COR-008` remote changes.
- Impact/likelihood/confidence: low-to-medium cost/latency impact that scales
with artifact size and remote pricing; occurs deterministically for same-size
differing forced files and repeated manifests; high confidence.
- Estimated remediation scope and owner: medium restore/storage-contract change.
Couple snapshot/version repair with a verified downloaded candidate that can
be retained for execution, or expose trustworthy digest/version metadata.
Document dry-run as having no durable/session mutation unless truly streaming
comparison eliminates all temporary writes.
- Test changes: count bytes/downloads for missing, equal, size-different, and
same-size-different objects in dry-run and apply modes; assert relational upper
bounds rather than exact private call choreography after snapshot design is
chosen.
- Dependencies: solve with `COR-008` so caching a plan download cannot install a
stale generation. Stage 7 owns adapter metadata/cost tradeoffs.
## Candidate Register
The remaining candidates require inspection by their named owners. Stages 2
@@ -1159,6 +1610,26 @@ and 3 have promoted their confirmed defects and risks into the register above.
preserve the explicit remote relative name separately if it is a protocol
constant.
### `DUP-003`: sibling-temp download and install mechanics are repeated across restore and prepare
- Category: duplicated mechanism candidate.
- Evidence: restore's `downloadObjectToSiblingTemp` plus
`InstallDownloadedTempFile`, audio's `downloadObjectAtomic`, and prepare's
previous-cache loop each create a sibling directory/temp file, close it,
download through `ObjectStore`, clean failure, and rename-install. The callers
legitimately differ in conflict, cache, content-validation, input-record, and
report policy. `storage.DownloadObjectToTemp` is a separate system-temp
inspection primitive. All install variants inherit `RSK-002`'s durability
requirements.
- Realistic risk: a future sync, permission, cancellation-cleanup, no-follow, or
download-size fix reaches restore but not prepare/audio, creating different
guarantees for the same remote-to-canonical transition. Conversely, sharing
the entire workflows would incorrectly merge caller policy.
- Confirmation owner: Stage 11 should consider one narrow sibling-temp download
and durable-install helper/capability while leaving validation, conflict,
cache, manifest-last, and reporting in their current owners. Coordinate with
`DUP-001`/`RSK-002` rather than adding another incomplete atomic writer.
### `TST-003`: filesystem safety tests omit destination and lock-recovery boundaries
- Category: test-suite sufficiency candidate.
@@ -1195,6 +1666,29 @@ and 3 have promoted their confirmed defects and risks into the register above.
invariant. Prefer extending current order/cleanup fixtures over duplicating
all source-family tables, which already have good focused coverage.
### `TST-005`: restore tests omit committed-snapshot and partial-transition invariants
- Category: test-suite sufficiency candidate.
- Evidence: the restore suite has strong happy-path, lexical traversal,
ordinary conflict/force, typed missing state, cache, previous-cache, report,
and workflow coverage. Run mismatch is tested only on the shared helper with
validation enabled. No assembled case covers restore/status mismatch,
uncommitted/stale prefix objects, remote generation changes, forced directory
conflict, plan-before-lock changes, partial forced overwrite followed by a
runner, same-size audio replacement, foreign absolute output paths, custom
publish destinations, or optional readiness. Failure coverage centers on an
invalid second manifest and lock conflict rather than every distinct durable
boundary.
- Realistic risk: `COR-008` through `COR-011` and `RSK-006` through `RSK-008`
can remain or be partially repaired while broad restore tests stay green;
several current tests explicitly encode size-only audio and prefix-wide scope
without tying those choices to the stronger authority invariants.
- Confirmation owner: Stage 12 should add one stateful package-level behavior
case per root risk, reuse the publish fake/version barriers from `TST-004`, and
consolidate caller rendering matrices. Avoid a mock for every mechanically
identical download boundary; retain the existing manifest-invalid case as the
representative pre-rename validation failure.
### `SIM-001`: runner terminalization and persistence ordering lack a narrow owner
- Category: simplification candidate.
@@ -1256,10 +1750,16 @@ and 3 have promoted their confirmed defects and risks into the register above.
| 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. |
| Pointer is written last, so failed publish cannot advance current | rejected as sufficient atomic-commit proof at Stage 4 | The fixed current manifest is overwritten first. Pointer identity remains authoritative, but an old pointer paired with the new manifest is unreadable; confirmed `COR-004`. |
| Pointer is written last, so failed publish cannot advance current | rejected as sufficient atomic-commit proof at Stage 4; reader enforcement corrected at Stage 5 | The fixed current manifest is overwritten first (`COR-004`). Strict callers reject old-pointer/new-manifest disagreement, but restore/status omit run validation and accept it (`COR-008`). |
| Remote current manifest records `current_pointer_written=false` | safe for current readers; retained as `ARC-003` | The snapshot is necessarily precommit, current-state loaders use actual pointer identity, and cleanup uses postcommit local metadata. The shared field remains ambiguous for future consumers. |
| Post-publish cleanup is revisited by later invocations | corrected and confirmed as `COR-006` | The runner invokes the helper, but its gate requires publish in the current `executed` list. Once session publish is succeeded, ordinary retry skips publish and therefore skips cleanup. |
| Manual clean should require publish commit metadata | rejected as a policy requirement at Stage 4 | Manual clean is explicit operator authorization with session/global scope, dry-run, cache opt-in, and confined targets. Publish execution/upload/pointer gates correctly apply only to automatic cleanup. |
| Restore dry-run performs no local writes | rejected as a literal implementation guarantee; retained as documentation precision under `EFF-001` | Dry-run avoids durable workspace, spool, cache, report, layout, and lock writes, but equal-size/unknown-size classification downloads remote bodies to system temporary files for checksumming and removes them afterward. |
| Force means every conflicting restore target is replaced | confirmed as `COR-009` | File conflicts become downloads, but a directory at a planned file path remains a conflict action. The force gate permits execution, which ignores that action and can still install the manifest and report success. |
| Size equality is sufficient audio identity | confirmed as `RSK-007` | Restore skips existing audio with the same positive remote size, and the shared cache accepts same-size content without ETag or checksum validation. Focused tests lock in the same-size restore shortcut. |
| Previous-artifact readiness is equivalent to loading the prior current pair | confirmed as `COR-010` | Status/validate stop after pointer/manifest validation and do not resolve or check required artifact objects; their missing-previous-session policy also disagrees with optional planning behavior. |
| Restore should roll back files written before a later failure | rejected as the current contract; retained as `RSK-006` | The documented operation is incremental and explicitly has no transaction or rollback. The risk is that the old manifest remains authoritative over partially replaced files and planning is not revalidated under the local lock. |
| Ordinary manifest and previous-cache reads must always re-hash bytes | rejected as a universal rule at Stage 5 | Restore verifies remote/local equality when needed during classification, and prepare validates required previous artifacts before analysis. The confirmed defects concern generation binding, incomplete readiness checks, and lost source identity rather than a blanket checksum requirement. |
## Unresolved Questions And Follow-Up
@@ -1287,6 +1787,19 @@ and 3 have promoted their confirmed defects and risks into the register above.
conditional-write capability can enforce the chosen rule across hosts?
- Should committed remote manifests omit precommit-only metadata, or should
local/remote publish metadata have distinct types after `COR-004` is fixed?
- Should restore consume only objects declared by the pointer-selected manifest,
or should the committed model carry an immutable generation map that can bind
every downloaded artifact, audio object, and previous-cache source?
- Should restore's local commit point use a staging tree/snapshot swap, or can a
journal plus locked revalidation make incremental replacement and recovery
sufficiently explicit under `RSK-006`?
- What durable identity should govern audio cache and existing-file reuse:
storage version, ETag with provider-specific rules, or a published checksum?
- Should restored manifests be rewritten to canonical local paths, or should all
consumers ignore persisted absolute paths in favor of workspace-relative
identities?
- How should publish metadata retain a stable source-to-destination mapping for
previous artifacts without reintroducing mutable configuration authority?
- Which native CI runner limitations explain the absence of validation jobs in
the tag-only release workflow? Stage 12 owns the automation conclusion.
@@ -1381,8 +1894,9 @@ No accepted risks or final audit conclusions are recorded yet.
`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
cases and support strict identity checks when callers request them; 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
@@ -1421,13 +1935,54 @@ No accepted risks or final audit conclusions are recorded yet.
`RSK-005`; added `ARC-003`, `COM-002`, and `TST-004`; resolved publish's
portion of `ARC-002` and corrected Stage 2's cleanup-retry conclusion.
- Explicit no-finding conclusions: pointer is unequivocally the final upload
and only accepted current marker; failure before current-manifest publication
and intended current marker (Stage 5 later confirmed incomplete reader
enforcement as `COR-008`); failure before current-manifest publication
preserves any prior current pair; successful retry is idempotent by
unconditional replacement; static and loaded remote locks, including required
outputs, survive force; disabled extraction is safe because only explicit
extraction rules resolve it; manual clean does not need publish metadata;
storage correctly remains policy-neutral and consumes explicit paths/keys.
- Follow-up: Stage 5 should reuse the remote-current truth table when auditing
restore mechanics. Stage 11 owns metadata vocabulary/duplication decisions;
- Follow-up: Stage 5 reused and corrected the remote-current reader truth table.
Stage 11 owns metadata vocabulary/duplication decisions;
Stage 12 owns the smallest stateful commit, cleanup-retry, symlink-read, and
remote-lock concurrency tests.
### Stage 5
- Contracts and code reviewed: architecture and testing policy; restore,
workspace, storage, artifact, manifest, operations, troubleshooting, and CLI
contracts; remote-current discovery, restore planning/classification,
execution/reporting, ordinary and audio download installation, cache/spool
materialization, previous-artifact requirement collection/planning, prepare
consumption, and status/validate readiness reporting.
- Graph/source evidence: callers and exact options for current-state loading;
restore plan/action and manifest-last traces; storage/list/download and
temporary-install paths; audio cache-key and validation paths; previous-cache
candidate resolution and consumer traces; focused test-function inventories.
Direct source and text inspection covered non-code contracts and implementation
details the graph could not distinguish.
- Validation: `go test -count=1 ./internal/app ./internal/previouscache
./internal/audio ./internal/artifacts ./internal/adapters/storage` passed
(`internal/app` 0.759 s, `internal/previouscache` 0.009 s, `internal/audio`
0.012 s, `internal/artifacts` 0.025 s, `internal/adapters/storage` 0.008 s;
1.87 s command wall time).
- Conclusions: restore authority, complete remote-to-local mapping, deterministic
ordering, action/force/dry-run behavior, manifest-last execution, report and
every failure boundary are recorded above. Audio cache/spool identity and
previous-session required/optional, candidate, and fallback policies are
explicit. Shared mechanics are separated from restore, prepare, status, and
validate caller policy.
- Findings: confirmed `COR-008`, `COR-009`, `COR-010`, `COR-011`, `RSK-006`,
`RSK-007`, `RSK-008`, and `EFF-001`; added `DUP-003` and `TST-005` with named
later owners.
- Explicit no-finding conclusions: relative target construction and ordering are
deterministic and lexically confined; force does not bypass identity,
traversal, or lock validation; dry-run creates no durable restore state;
ordinary download failures remove the active temporary file; the storage
adapter remains policy-neutral; prepare intentionally overwrites its private
previous-cache destination while restore classifies existing destinations.
- Scenario disposition: scenario 4 confirms the pointer is not sufficient
authority because restore/status omit run validation and restore reads mutable
prefix objects; scenario 6 confirms partial incremental replacement, old-
manifest authority before the final install, and restored state despite a
later report failure. Rollback and automatic retry are intentionally absent.