Complete the D&D collection assembly
This commit is contained in:
@@ -1,149 +1,173 @@
|
||||
# Feature 0011 Remediation Review
|
||||
# Feature 0012 Remediation Review
|
||||
|
||||
Status: resolved
|
||||
|
||||
Reviewed implementation commits:
|
||||
|
||||
- `292d866 Upgrade the Narratio role to version 2.1.0`
|
||||
- `28a4d27 Add managed credentials to the D&D pipeline`
|
||||
- `8ff0761 Document and test pipeline credential delivery`
|
||||
- `600eb21 Document standalone and collection pipeline usage`
|
||||
- `3f7f82e Establish the D&D collection foundation`
|
||||
- `c84aa0d Merge commit '5dbae8a' as 'roles/pipeline'`
|
||||
- `10ad31c Test the assembled D&D collection`
|
||||
|
||||
## Review Summary
|
||||
|
||||
Feature 0011 implements the principal target architecture. The standalone
|
||||
Narratio role now installs v2.1.0 and generically exposes its three native
|
||||
Notarius controls. The pipeline pins that role revision, manages a restricted
|
||||
conventional secrets directory, coordinates the Audita and Notarius credential
|
||||
references, and enables ordinary Notarius checkpoint reuse without duplicating
|
||||
the root-owned chunk-cache policy. Both complete integration entry points pass,
|
||||
including native application validation and immediate clean repeats.
|
||||
Feature 0012 implements the principal target architecture. The standalone
|
||||
pipeline repository remains authoritative, and collection history records its
|
||||
exact `600eb21` snapshot as a squashed subtree at `roles/pipeline`. The
|
||||
collection has valid Galaxy and runtime metadata, exact native collection
|
||||
dependencies, one unified bootstrap manifest for the five Git-hosted roles,
|
||||
focused build exclusions, and an isolated artifact-level integration entry
|
||||
point. The collection builds as `rakestraw.dnd` 0.1.0 and exposes
|
||||
`rakestraw.dnd.pipeline`.
|
||||
|
||||
Two issues remain. First, the feature roadmap and implementation plan
|
||||
strengthened the requested credential-delivery interface with deletion and
|
||||
empty-value policies that were not required. The implementation consequently
|
||||
contains an extra production task and substantial test and documentation
|
||||
machinery for those invented semantics. Second, the pipeline README does not
|
||||
explain the Scriptorium credential flow that the feature roadmap expressly
|
||||
requires it to document.
|
||||
Two low-severity issues were found and are now resolved. The collection's
|
||||
subtree maintenance examples derived a commit from the local sibling
|
||||
repository but told Git to fetch that commit from a remote which did not
|
||||
contain it at review time. Separately, the collection integration driver
|
||||
repeated secret-redaction assertions already owned by the standalone pipeline
|
||||
suite; those assertions did not validate collection assembly and strengthened
|
||||
the collection test contract beyond the selected packaging boundary.
|
||||
|
||||
## Finding 1: The Credential Mapping Adds Unrequested Lifecycle Semantics
|
||||
|
||||
Severity: moderate
|
||||
|
||||
The user requested an inventory-friendly way to deliver credential values to
|
||||
Narratio's conventional secrets directory. The roadmap subsequently added two
|
||||
stronger behaviors without an express requirement: null means delete an exact
|
||||
file, and an empty string must be rejected by the role. Those choices were
|
||||
then made mandatory by the implementation plan.
|
||||
|
||||
The resulting production implementation appears in
|
||||
`ansible-role-dnd-pipeline/tasks/main.yml:10-53`. It validates the stronger
|
||||
value policy, conditionally copies non-null entries, and uses a separate file
|
||||
loop to delete null entries. The public contract and example are repeated in
|
||||
`ansible-role-dnd-pipeline/README.md:275-291`, while
|
||||
`ansible-role-dnd-pipeline/tests/integration:167-194` adds an empty-value
|
||||
failure case and a multi-convergence deletion lifecycle.
|
||||
|
||||
The environment-variable-name validation is justified because caller input
|
||||
becomes a destination path. The two value semantics are different: they add
|
||||
credential lifecycle and application-validity guarantees that the role was
|
||||
not asked to own. ConfigKit accepts an empty immediate credential file and
|
||||
performs its own nonempty check only when a configured typed reference resolves
|
||||
to that value. A simple delivery role should write the caller-supplied string
|
||||
and trust the native application boundary. Credential deletion can be added
|
||||
later if an operator-facing removal contract is expressly requested.
|
||||
|
||||
### Required Remediation
|
||||
|
||||
- Narrow `dnd_pipeline_secrets` to an empty-by-default mapping from valid
|
||||
environment-variable names to string file contents.
|
||||
- Retain complete key validation before path construction and retain
|
||||
`no_log: true` for every task that evaluates or writes credential values.
|
||||
- Validate that values are strings, but do not add a nonempty policy that is
|
||||
stricter than the native bootstrap and typed-reference boundary.
|
||||
- Converge every supplied entry directly with one `ansible.builtin.copy` loop.
|
||||
Remove the null condition and the separate deletion task.
|
||||
- Treat omitted keys as unmanaged. Do not invent a replacement deletion,
|
||||
purge, directory-mirroring, or credential-validation mechanism.
|
||||
- Remove the empty-value and explicit-null lifecycle cases from the integration
|
||||
suite. Retain the valid-name boundary case, representative credential
|
||||
delivery, redaction checks, authorized native loading, and immediate clean
|
||||
repeat.
|
||||
- Correct the Feature 0011 roadmap, the settled MVP text, and the pipeline
|
||||
README so they describe the narrower interface and no longer present the
|
||||
strengthened semantics as user decisions.
|
||||
|
||||
## Finding 2: Scriptorium Credential Delivery Is Not Documented for Consumers
|
||||
## Finding 1: Subtree Maintenance Commands Mix Local and Remote Sources
|
||||
|
||||
Severity: low
|
||||
|
||||
The Feature 0011 roadmap requires documentation of all application credential
|
||||
flows, including Scriptorium's existing profile-owned `api_key_env` selection.
|
||||
`ansible-role-dnd-pipeline/README.md:275-291` explains the generic mapping and
|
||||
the earlier Notarius section explains its typed reference, but the README never
|
||||
tells an operator how a selected Scriptorium profile obtains its credential.
|
||||
`ansible-collection-dnd/README.md:51-63` obtains `pipeline_commit` from
|
||||
`../ansible-role-dnd-pipeline`, but passes the Gitea URL as the repository
|
||||
argument to both `git subtree add` and `git subtree pull`. The reviewed
|
||||
pipeline commit was seven commits ahead of `origin/main` at review time; the
|
||||
remote advertised `b15fe8d`, not imported commit `600eb21`. The documented
|
||||
command therefore could not reproduce the current import.
|
||||
|
||||
Production behavior is already correct: Narratio bootstraps the named file into
|
||||
its environment and its existing Scriptorium adapter forwards the selected
|
||||
profile credential. This is a documentation gap, not a request for another
|
||||
variable, task, or integration test.
|
||||
The collection artifact already contains the imported role, so consumers do
|
||||
not run these commands or need a sibling checkout. The commands are the
|
||||
maintainer-facing assembly interface and should also be portable outside this
|
||||
workspace. The durable contract should therefore use Gitea exclusively: first
|
||||
push the reviewed pipeline commit or tag to the authoritative repository, then
|
||||
identify that remote revision explicitly and import it from the same URL.
|
||||
|
||||
### Required Remediation
|
||||
|
||||
- Add a concise consumer-facing explanation that a Scriptorium profile remains
|
||||
the owner of its `api_key_env` name.
|
||||
- Explain that an operator can add a matching filename/value entry to
|
||||
`dnd_pipeline_secrets`; Narratio's bootstrap supplies it and the existing
|
||||
adapter forwards only the profile-selected credential.
|
||||
- Do not add a suite-specific Scriptorium credential variable, inspect profile
|
||||
files in Ansible, duplicate the application adapter, or add tests of
|
||||
Narratio/Scriptorium internals.
|
||||
- Remove the sibling-repository `git rev-parse` commands from both subtree
|
||||
examples.
|
||||
- Define an explicit `pipeline_revision` placeholder representing a reviewed
|
||||
commit or immutable tag that has already been pushed to Gitea, and pass that
|
||||
revision with the authoritative Gitea URL to both `git subtree add` and
|
||||
`git subtree pull`.
|
||||
- State concisely that a pipeline revision must be committed, reviewed, and
|
||||
pushed to the authoritative repository before collection assembly imports
|
||||
it. Do not prescribe a branch name or require a release tag during active
|
||||
development; a pushed full commit ID remains valid.
|
||||
- Do not add a configured remote, sibling-path assumption, synchronization
|
||||
script, lock file, or private provenance state.
|
||||
- Do not rewrite subtree history or reimport the current role. This is a
|
||||
collection-owned documentation correction only.
|
||||
- Confirm that Gitea contains the selected pipeline revision, then validate
|
||||
the corrected initial command in a disposable Git repository without using
|
||||
the local sibling repository as the subtree source.
|
||||
|
||||
## Finding 2: The Collection Test Duplicates Secret-Redaction Coverage
|
||||
|
||||
Severity: low
|
||||
|
||||
`ansible-collection-dnd/tests/integration:19-20` defines two synthetic values
|
||||
solely so lines 74-75 can search the first convergence log for them. Secret-safe
|
||||
task output is pipeline behavior established by Feature 0011 and already
|
||||
covered by `ansible-role-dnd-pipeline/tests/integration`. The collection owns
|
||||
artifact construction, bootstrap installation, FQCN resolution, representative
|
||||
assembled behavior, and clean repeated convergence; it does not own a second
|
||||
credential-redaction contract.
|
||||
|
||||
The implementation plan required synthetic credentials so native
|
||||
configuration could be exercised without real provider values, but it did not
|
||||
require the collection to retest Feature 0011 redaction. These two assertions
|
||||
are therefore an inadvertent strengthening by the implementing agent and
|
||||
conflict with the roadmap's instruction to keep detailed pipeline feature
|
||||
coverage in the standalone role.
|
||||
|
||||
The native Seriatim, Audita, Notarius, Scriptorium, and Narratio checks are not
|
||||
part of this finding. They are state-based, inexpensive, and implement the
|
||||
roadmap's express requirement for representative native validation of the
|
||||
assembled suite.
|
||||
|
||||
### Required Remediation
|
||||
|
||||
- Retain distinctive synthetic credential contents in the collection
|
||||
convergence variables; no real credential or live provider call may enter
|
||||
the suite.
|
||||
- Remove the two shell variables used only for log searching and remove the two
|
||||
secret-content grep assertions.
|
||||
- Retain isolated dependency installation, archive boundary checks, FQCN
|
||||
syntax and lint validation, the selected native application checks, and the
|
||||
immediate `changed=0 failed=0` repeat.
|
||||
- Do not replace the removed assertions with another collection-level
|
||||
credential, log, file-content, or implementation-trace test.
|
||||
|
||||
## Resolution
|
||||
|
||||
Resolved in pipeline-role commit `de1b191`. The credential mapping now accepts
|
||||
valid environment-variable names with string contents, validates the complete
|
||||
mapping before path construction, and converges every supplied entry through
|
||||
one secret-safe `ansible.builtin.copy` loop. The unrequested empty-value policy,
|
||||
null deletion task, and associated lifecycle tests were removed. Omitted names
|
||||
remain unmanaged.
|
||||
Resolved in collection commits `556b6ed` and `da67b34`.
|
||||
|
||||
The pipeline README now explains how a Scriptorium profile's application-owned
|
||||
`api_key_env` name selects a matching `dnd_pipeline_secrets` entry without
|
||||
adding another suite variable or duplicating adapter behavior. The durable
|
||||
Feature 0011 roadmap and MVP policy were corrected to record the narrower
|
||||
operator-facing contract.
|
||||
The collection README now uses an explicit `PUSHED_COMMIT_OR_TAG` revision and
|
||||
the authoritative Gitea URL for both initial and subsequent subtree imports.
|
||||
It states that the selected revision must be committed, reviewed, and pushed
|
||||
before collection assembly. The examples no longer inspect or depend on a
|
||||
sibling checkout. A disposable repository successfully imported the exact
|
||||
commit advertised by Gitea through the documented remote-only command shape.
|
||||
|
||||
The collection integration driver retains its distinctive synthetic
|
||||
credential inputs but no longer defines separate values solely for log
|
||||
searching or duplicates the standalone pipeline suite's two redaction
|
||||
assertions. Its artifact, dependency, FQCN, native application, and clean
|
||||
repeat coverage remain unchanged.
|
||||
|
||||
## Required Validation
|
||||
|
||||
Remediation validation:
|
||||
After remediation, run:
|
||||
|
||||
```text
|
||||
cd ansible-role-dnd-pipeline && tests/integration
|
||||
cd ansible-collection-dnd
|
||||
bash -n tests/integration
|
||||
tests/integration
|
||||
git diff --check
|
||||
```
|
||||
|
||||
The result was successful syntax and lint validation; synthetic
|
||||
credential delivery through the conventional directory; native Narratio
|
||||
configuration loading for authorized identities; absence of synthetic values
|
||||
from captured output; preservation of Notarius resume and root-owned `auto`
|
||||
chunk caching; and immediate repeats with `changed=0` and `failed=0`.
|
||||
The result must demonstrate isolated dependency bootstrap, collection build
|
||||
and artifact installation, FQCN resolution, the retained native application
|
||||
checks, and immediate clean convergence. The built artifact must still include
|
||||
`roles/pipeline/tasks/main.yml` and exclude the selected development-only
|
||||
subtree content.
|
||||
|
||||
Validate the corrected subtree example against Gitea in a disposable Git
|
||||
repository without modifying the real collection history or relying on the
|
||||
local sibling repository as the source. The standalone pipeline integration
|
||||
suite does not need to be rerun for these collection-only documentation and
|
||||
test-scope changes.
|
||||
|
||||
## Review Validation Evidence
|
||||
|
||||
The review inspected the exact implementation diffs and confirmed that the
|
||||
Narratio dependency pin resolves to the reviewed `292d866` revision. The
|
||||
standalone Narratio `tests/integration` entry point passed with v2.1.0 native
|
||||
configuration validation and clean repeats. The complete pipeline
|
||||
`tests/integration` entry point also passed, including isolated dependency
|
||||
installation, syntax checking, production-profile lint, authorized and denied
|
||||
native loading, redaction checks, profile and version selections, and clean
|
||||
repeats. No live model, transcription, storage, or publishing endpoint was
|
||||
contacted.
|
||||
The review compared the complete standalone pipeline repository with
|
||||
`roles/pipeline`; the trees are byte-for-byte identical outside Git metadata.
|
||||
The root bootstrap manifest's role and collection sections exactly match the
|
||||
two imported pipeline manifests, and `galaxy.yml` declares the same native
|
||||
collection versions.
|
||||
|
||||
After remediation, the complete pipeline integration entry point passed again.
|
||||
It exercised isolated dependency installation, the invalid-name boundary,
|
||||
synthetic credential delivery and redaction, authorized native loading,
|
||||
Notarius resume and root-owned `auto` chunk caching, the retained profile and
|
||||
version selections, and immediate clean repeats. `git diff --check` also
|
||||
passed before the remediation commit.
|
||||
A fresh collection build produced
|
||||
`rakestraw-dnd-0.1.0.tar.gz`. The archive contains the pipeline task entry
|
||||
point and omits the selected nested tests, Ansible configuration, dependency
|
||||
manifests, and Git ignore file. Installing that artifact into a temporary
|
||||
collection path made `rakestraw.dnd.pipeline` discoverable. A separate
|
||||
isolated bootstrap installed the unified requirements, after which syntax
|
||||
checking and production-profile ansible-lint passed.
|
||||
|
||||
`bash -n ansible-collection-dnd/tests/integration`, manifest consistency
|
||||
checks, `git diff --check`, and collection repository integrity checks also
|
||||
passed during review.
|
||||
|
||||
After remediation, the remote-only subtree command successfully imported the
|
||||
revision advertised by Gitea into a disposable repository, and a fresh
|
||||
collection build succeeded. The complete collection integration entry point
|
||||
then passed isolated dependency installation, artifact construction and
|
||||
installation, syntax checking, production-profile lint, all retained native
|
||||
application checks, and two recaps with `changed=0` and `failed=0`. The
|
||||
standalone pipeline suite was not rerun because neither remediation changed
|
||||
the authoritative pipeline role.
|
||||
|
||||
Reference in New Issue
Block a user