Revise the Seriatim plan and workspace conventions
This commit is contained in:
@@ -1,174 +1,19 @@
|
||||
# Feature 0001 Remediation Findings
|
||||
# Feature 0001 Prior Remediation Record
|
||||
|
||||
Status: resolved
|
||||
Historical record: superseded
|
||||
|
||||
## Review Scope
|
||||
The original Feature 0001 review required increasingly elaborate recovery for
|
||||
build-state files, metadata drift, custom path shapes, and symbolic links.
|
||||
Those findings were resolved in `ansible-role-seriatim` commits `c6c1907`
|
||||
through `6fb1aa2`, but the resulting architecture was later rejected as
|
||||
disproportionate to the MVP.
|
||||
|
||||
This review compared `ansible-role-seriatim` commits `ac35c1f` through
|
||||
`efd4734` with the Feature 0001 roadmap and staged implementation plan. It also
|
||||
ran the committed static and integration validation and performed targeted
|
||||
state-corruption, permission-drift, and custom-path checks on the managed local
|
||||
installation.
|
||||
The authoritative replacement specification is
|
||||
`docs/roadmap/features/0001-seriatim-source-installation.md`. It deliberately
|
||||
removes persistent build state and treats unusual corruption, adversarial path
|
||||
redirection, exact compiler-input tracking, and transactional replacement as
|
||||
potential future hardening rather than current requirements.
|
||||
|
||||
The four planned implementation stages are structurally present and the
|
||||
committed integration suite passes. The findings below are gaps that must be
|
||||
addressed before Feature 0001 is marked complete or its patterns are copied to
|
||||
other application roles.
|
||||
|
||||
## Finding 1: Build-input state content is not part of convergence
|
||||
|
||||
Severity: high
|
||||
|
||||
`tasks/build.yml` checks whether `build-inputs.json` exists but never reads or
|
||||
compares its content. The rebuild decision trusts `fingerprint.sha256`
|
||||
independently. An empty, corrupted, stale, or replaced build-input document is
|
||||
therefore accepted as converged when the fingerprint and binary checksum files
|
||||
remain unchanged.
|
||||
|
||||
This diverges from the plan's requirement that malformed or unreadable state
|
||||
cause a rebuild and from the role's claim that readable inputs and their
|
||||
fingerprint describe the installed result. A targeted test truncated
|
||||
`build-inputs.json`; convergence returned `changed=0` and `failed=0`, while the
|
||||
subsequent verification play correctly failed its input/fingerprint assertion.
|
||||
|
||||
Required remediation:
|
||||
|
||||
- Read the stored build-input document safely during convergence.
|
||||
- Require its exact canonical content to match the desired build inputs and
|
||||
require its SHA-256 digest to match the stored and desired fingerprint.
|
||||
- Treat missing, unreadable, malformed, or inconsistent input state as a
|
||||
rebuild condition rather than an unhandled failure.
|
||||
- Extend `tests/integration` to corrupt or replace `build-inputs.json`, verify
|
||||
that convergence repairs it, and verify that the following run is
|
||||
idempotent.
|
||||
|
||||
## Finding 2: Managed ownership and modes drift without repair
|
||||
|
||||
Severity: medium
|
||||
|
||||
The binary's checksum and version participate in the rebuild decision, but its
|
||||
owner, group, and mode do not. Binary metadata is set only inside the rebuild
|
||||
block. Build-state directory and file metadata are likewise set only after a
|
||||
build. A content-identical installation can therefore remain in an unsafe or
|
||||
incorrect metadata state indefinitely.
|
||||
|
||||
A targeted test changed `/usr/local/bin/seriatim` from mode `0755` to `0777`.
|
||||
Convergence returned `changed=0` and left mode `0777` in place. This violates
|
||||
the target state requiring a root-owned, non-operator-writable binary. The same
|
||||
design leaves drifted state-file ownership and permissions uncorrected.
|
||||
|
||||
Required remediation:
|
||||
|
||||
- Converge `seriatim_binary_path` to `root:root` mode `0755` whenever a valid
|
||||
installed regular file exists, without forcing an unnecessary rebuild.
|
||||
- Converge `seriatim_build_state_dir` to `root:root` mode `0755` and its three
|
||||
managed files to `root:root` mode `0644` on every successful role run.
|
||||
- Treat unexpected file types, including a directory at the binary path, as a
|
||||
clear validation or repair condition rather than passing them into the build
|
||||
copy operation.
|
||||
- Add integration cases that drift binary and build-state metadata, require
|
||||
one repairing run, and require an idempotent repeat.
|
||||
|
||||
## Finding 3: A configurable binary path requires a pre-existing parent
|
||||
|
||||
Severity: medium
|
||||
|
||||
`seriatim_binary_path` is a documented public path override, but the role does
|
||||
not create or validate its parent directory. `ansible.builtin.copy` consequently
|
||||
fails after a successful build when the configured parent is absent. A
|
||||
targeted run using `/tmp/seriatim-review-missing-parent/seriatim` failed with
|
||||
`Destination directory ... does not exist`.
|
||||
|
||||
Required remediation:
|
||||
|
||||
- Inspect the configured binary parent before building.
|
||||
- Fail clearly if an existing parent is not a directory.
|
||||
- Create a missing parent as `root:root` mode `0755` without changing ownership
|
||||
or mode on an already existing shared directory.
|
||||
- Add an isolated integration case proving that a nondefault binary path with
|
||||
a missing parent installs, validates, and converges idempotently.
|
||||
|
||||
## Finding 4: Role metadata and privilege documentation are incomplete
|
||||
|
||||
Severity: low
|
||||
|
||||
Stage 1 required role source and issue information, but `meta/main.yml` omits
|
||||
the supported `issue_tracker_url` field. The README also does not state plainly
|
||||
that consumers must apply the role with privilege escalation, even though it
|
||||
manages packages, snaps, root-owned source, and `/usr/local/bin`.
|
||||
|
||||
Required remediation:
|
||||
|
||||
- Add the role repository's Gitea issue tracker to `meta/main.yml`.
|
||||
- Add role repository and issue links to the README where appropriate.
|
||||
- State explicitly that the consuming play must use `become: true` or provide
|
||||
equivalent root privileges.
|
||||
- Re-run metadata/schema validation and ansible-lint.
|
||||
|
||||
## Remediation Completion Criteria
|
||||
|
||||
- All four findings are addressed in `ansible-role-seriatim`.
|
||||
- Static validation and the expanded `tests/integration` suite pass.
|
||||
- Targeted corruption, metadata-drift, and alternate-path cases converge to
|
||||
the documented state and then report `changed=0` on an immediate repeat.
|
||||
- The default local installation ends at Seriatim `v1.5.0`, source commit
|
||||
`0fc92f364316c874d366b00cdc884a33e1900b25`, with a clean checkout and
|
||||
consistent build state.
|
||||
- The role and workspace repositories are clean, and remediation commits are
|
||||
ready for review before Feature 0001 is marked complete.
|
||||
|
||||
## Resolution Evidence
|
||||
|
||||
- Finding 1 was resolved by `ansible-role-seriatim` commit `c6c1907`.
|
||||
- Finding 2 was resolved by `ansible-role-seriatim` commit `f2c1b1a`.
|
||||
- Finding 3 was resolved by `ansible-role-seriatim` commit `92906eb`.
|
||||
- Finding 4 was resolved by `ansible-role-seriatim` commit `07dd736`.
|
||||
- Malformed state-object recovery was hardened by
|
||||
`ansible-role-seriatim` commit `ca49dae` during the completion audit.
|
||||
- The expanded `tests/integration` suite passed after all remediation commits
|
||||
and restored the default Seriatim installation.
|
||||
|
||||
## Completion Review
|
||||
|
||||
The four original findings are resolved, and the expanded integration suite
|
||||
passes. A focused review of the malformed-state recovery identified one
|
||||
additional edge case, which has also been resolved.
|
||||
|
||||
## Finding 5: A malformed build-state directory is not handled
|
||||
|
||||
Severity: medium
|
||||
|
||||
`tasks/build.yml` inspects the three state files beneath
|
||||
`seriatim_build_state_dir` without first inspecting the state-directory path.
|
||||
If that configured path exists as a regular file, the first
|
||||
`ansible.builtin.stat` fails with `Not a directory`. This is an unhandled
|
||||
malformed-state failure rather than the planned recovery behavior. A targeted
|
||||
run with an otherwise valid custom binary confirmed that the role exits before
|
||||
rebuilding or converging the state directory. A second targeted run using a
|
||||
symbolic link as the state-directory path succeeded and wrote all three
|
||||
root-owned state files into the link target while leaving the configured path
|
||||
as a symbolic link.
|
||||
|
||||
Required remediation:
|
||||
|
||||
- Inspect `seriatim_build_state_dir` before inspecting any child state path.
|
||||
- Define and implement deliberate behavior for an existing non-directory
|
||||
object. Because the state directory is fully managed, replacing that object
|
||||
and rebuilding is consistent with the existing malformed-state-file
|
||||
behavior; alternatively, fail with a purpose-written assertion if preserving
|
||||
an unexpected object is preferred.
|
||||
- Ensure an existing symbolic link is not silently treated as the managed
|
||||
state directory or used to redirect root-owned state writes.
|
||||
- Add integration coverage for the selected non-directory behavior and require
|
||||
an idempotent immediate repeat after recovery when recovery is supported.
|
||||
|
||||
Resolution:
|
||||
|
||||
- `ansible-role-seriatim` commit `6fb1aa2` makes the build-state directory
|
||||
authoritative, replacing regular files and symbolic links with a root-owned
|
||||
directory before inspecting child state.
|
||||
- Integration coverage confirms repair and immediate idempotence for both
|
||||
regular-file and symbolic-link drift, and confirms that the symbolic-link
|
||||
target remains untouched.
|
||||
- The complete integration suite passes and restores a verified default
|
||||
installation.
|
||||
This file is retained only to explain the historical commit sequence. It is
|
||||
not an active remediation specification and must not be used as a template for
|
||||
another role.
|
||||
|
||||
Reference in New Issue
Block a user