odysseus/docs/skills-audit-second-review.md

# Skills PR #6711 second-review corrections

The correction scope is route prompt compatibility and structural constructor
contracts. PR #6711 remains draft. Durable skill ownership stays with
`SkillsManager`. No publication policy, authority mechanism, provider runtime,
frontend behavior or protected manager/index implementation was changed.

The reviewed head was `a6a33674ed4facf11f53e7cdd41f882ad0fa8482`. The independent
original baseline is `86b886c611338282af6d4a717246ea4e256f58df`. Official `dev`
integration was tested against `c3819e3cbf56c4f899eacc66b21115a77d9c570d`.

## Compatibility corrections

Service-local imports captured prompt builders outside the route namespace.
Patching the public route symbols therefore stopped affecting execution after
extraction. `AuditDependencies` now binds the current test-task, test-message and
baseline-message builders. Manual execution, exact-approval continuation, initial
audits, baseline arms, self/teacher retries and scheduled batches use those
bindings. The route `_skill_baseline_task` wrapper again delegates to the current
route `_skill_test_task`. Both execution arms still use one shared task, matching
the original behavior. No service imports Skills routes.

The signature checker treated a factory's module/qualified name as its identity
and omitted Pydantic settings outside the generated Python signature. Closures
with different captured values and default-validation changes were false
negatives. New tests additionally demonstrated hidden validation-alias path and
model-configuration changes. The checker now records public field validation
settings, rebuilt annotations, defaults and inherited model configuration. See
the [Pydantic FieldInfo reference](https://docs.pydantic.dev/latest/api/fields/)
for the distinction between factories, validation aliases and `validate_default`.

Only known builtin factories are supported by identity. Custom factories,
including factories requiring validated field data, raise `TypeError` without
being invoked during comparison. Unsupported metadata/configuration values also
fail closed. No bytecode, arbitrary object representations or factory output is
serialized. The comparator does not promise equivalence of arbitrary Python
validators or factory implementations.

The expanded fixture was generated from the original baseline in a new detached
worktree, with only the revised test comparator copied in. Python 3.11 and 3.12
produced byte-identical files. Every previous parameter/return record remains
equal. All 28 interfaces and 22 endpoint declarations remain present. The
canonical OpenAPI fixture is unchanged, and its original digest is unchanged.
Independent native `inspect.signature` snapshots also match original versus
corrected implementations on each interpreter.

## Regression evidence and validation

The new tests were run before production/comparator edits: 12 failed and four
passed, demonstrating the six signature cases and six broken prompt cases. After
correction, all 16 added cases pass. The prompt probes capture actual
`stream_agent_loop` ingress rather than replacing the runners or coordination.
They cover patched and normal execution, guard escaping and taint metadata,
owner authority, exact-approval continuation, and worker retries after both
self and teacher rewrites. The same ten prompt cases pass on the original
implementation. The six signature cases reject custom factories without calling
them and detect default validation, alias paths and model validation settings.

Environments: Python 3.11.17 and 3.12.13, FastAPI 0.143.1, Pydantic 2.14.1,
pytest 9.1.1. Python test commands set `FASTEMBED_MODEL=review-disabled` and
`HF_HUB_OFFLINE=1` to exercise unavailable optional embeddings without model
downloads. They do not validate live providers or embedding quality.

| Executed scope | Result |
| --- | --- |
| 53-file Skills, approval, authority and lifecycle selection, Python 3.11 | 757 passed |
| Same selection, Python 3.12 | 757 passed |
| Both characterization/service files, included above | 70 passed |
| Original baseline, available focused selection plus revised characterization | 724 passed |
| Original baseline, revised characterization plus prompt probes, each interpreter | 47 passed |
| Current-dev combined tree, focused selection plus eight intervening dev test files, each interpreter | 900 passed |
| Repository-wide suite, Python 3.11 | 13,991 passed, 60 skipped, two inherited environment failures, six subtests passed |
| Frontend publisher and architecture checks | Passed, zero changed modules |
| Relevant Node tests | 13 passed |
| Compilation, both Python versions | Passed |
| Import graph and fresh service-first/route-first imports, both versions | Passed; acyclic; no reverse Skills route import |
| Original normalized moved-body comparison | 26 of 26 matched after explicit dependency substitutions |
| Pinned/checksum-verified Gitleaks 8.30.1, reachable HEAD and official dev histories | No leaks |
| Same Gitleaks, tracked working files | No leaks |
| Diff/whitespace and protected-source comparisons | Passed |

The two repository-wide failures are
`tests/test_containment_enforcement.py::test_namespace_handshake_closes_model_stdin`
and `test_execution_environment_cannot_replace_probed_bwrap[False]`. Isolated
reruns on both original and corrected implementations reproduce the same extra
`Failed to create stream fd: No such file or directory` stderr. The independent
review traced it to login-profile `im-config` calls to `systemd-cat` inside
Bubblewrap without the journal stream socket. These environment failures remain;
they were not hidden by excluding tests. A full Python 3.12 suite was not run.

The combined test tree used the official merge revision `5419ed37`, whose tree
exactly matches `git merge-tree --write-tree upstream/dev a6a33674`, with the
correction diff applied in a detached scratch worktree. Published history was
not rebased, amended, reset or rewritten.

Local commands, independent fixture captures, baseline reproductions and full
logs are retained under `/tmp/skills-6711-corrections/`. The independent review
and adversarial probe source remain under `/tmp/skills-6711-second/`. These
temporary evidence paths are not portable repository fixtures. CI should read
the committed golden fixtures, not regenerate them from the implementation.

## Separate inherited P2 hardening plan

All six defects demonstrably exist on the official original baseline. The six
safety assertions in `/tmp/skills-6711-second/test_adversarial.py` fail on both
the original and corrected trees with the same outcomes. Logs are
`inherited-original.log` and `inherited-head.log` in the correction evidence
directory. The original/source body comparison also matches these paths after
dependency substitutions. There is no demonstrated material worsening from the
extraction. That finding does not make their security impact acceptable.

Priorities below concern a separate hardening change. Items 1 and 2 are release
blockers requiring maintainer resolution before deploying automatic skill
rewrite/publication workflows. This recommendation is independent of whether
the architecture PR is integrated. No tool-authority bypass or cross-owner
credential disclosure was demonstrated, and no critical extraction regression
was found that requires expanding this compatibility correction.

1. **Model-controlled lifecycle/provenance fields.** Current source:
   [publication.py:328](../services/skills/publication.py#L328), especially line
   342; the bypassed retrieval condition is
   [skills.py:703](../services/memory/skills.py#L703). Original source:
   [routes/skills_routes.py:1039](https://github.com/odysseus-dev/odysseus/blob/86b886c611338282af6d4a717246ea4e256f58df/routes/skills_routes.py#L1039).
   `test_rewrite_cannot_promote_model_text_to_builtin` uses the real manager and
   saves model markdown with `source: builtin`, `status: published` and
   `confidence: 1`. Identity/owner remain pinned, but the lifecycle fields persist
   and the skill becomes retrieval-eligible without a passing audit. Corrective
   change: allowlist procedure and approved descriptive/retrieval fields during
   model rewrites; preserve existing owner, provenance, publication, confidence,
   audit and teacher lifecycle state. Only verified policy may update those
   fields. Metadata-only fixes must preserve the procedure or trigger a retest.
   Add real-manager tests for self, teacher and metadata-only rewrites.

2. **Preference loading fails open for automatic publication.** Current source:
   [publication.py:13](../services/skills/publication.py#L13) and
   [skills.py:59](../services/memory/skills.py#L59). Original source:
   [routes/skills_routes.py:710](https://github.com/odysseus-dev/odysseus/blob/86b886c611338282af6d4a717246ea4e256f58df/routes/skills_routes.py#L710).
   `test_preferences_read_failure_does_not_enable_auto_publication` makes the
   loader raise `PermissionError`; automatic publication is enabled rather than
   preserving the owner's possible opt-out. Corrective change: distinguish a
   successfully loaded empty preference store from an exception, log the failure
   and disable automatic publication on exceptions. Align the manager retrieval
   preference adapter in the separate fix. Test unreadable/malformed existing
   preference files and the ordinary empty-store default independently.

3. **Manual cancellation retains private credentials and stale running state.**
   Current source:
   [agent_runner.py:125](../services/skills/agent_runner.py#L125) and
   [job_coordinator.py:49](../services/skills/job_coordinator.py#L49). Original:
   [routes/skills_routes.py:635](https://github.com/odysseus-dev/odysseus/blob/86b886c611338282af6d4a717246ea4e256f58df/routes/skills_routes.py#L635).
   `test_manual_cancellation_cleans_sensitive_continuation` suspends and cancels
   the stream. The stream closes in its originating task, but job status remains
   running and `_run.headers.Authorization` remains resident. Polling still hides
   these fields; this is retention, not demonstrated disclosure. Corrective
   change: handle `CancelledError`, mark cancelled, clear private continuation
   data, retire only the attached exact approval and re-raise. Preserve state on
   a normal awaiting-approval return. Retain task handles for coordinated
   replacement/cancellation and test owner/approval isolation.

4. **Approval pauses during retries lead to demotion.** Current source:
   [audit_workflow.py:244](../services/skills/audit_workflow.py#L244), teacher
   retry at line 280 and demotion at line 303. Original:
   [routes/skills_routes.py:1448](https://github.com/odysseus-dev/odysseus/blob/86b886c611338282af6d4a717246ea4e256f58df/routes/skills_routes.py#L1448)
   and lines 1484/1507. `test_retry_approval_preserves_publication_and_confidence`
   begins published at .9, fails the first grade, then pauses the rewritten test
   for approval. The result is flagged/draft/.35. The lower-level approval is
   correctly denied; state handling afterwards is wrong. Corrective change:
   immediately recognize `approval_required` after every retry, stop escalation
   and demotion, and preserve the intended pre-pause lifecycle state. Coordinate
   with item 1 so model rewrites cannot replace that saved state. Test both
   self/teacher retries, repeated pauses and batch protection.

5. **Duplicate losers are binned after keeper publication fails.** Current:
   [publication.py:217](../services/skills/publication.py#L217), including loser
   writes at line 228. Original:
   [routes/skills_routes.py:914](https://github.com/odysseus-dev/odysseus/blob/86b886c611338282af6d4a717246ea4e256f58df/routes/skills_routes.py#L914).
   `test_duplicate_losers_retained_if_keeper_write_fails` raises disk-full during
   keeper publication and still observes a loser binned. Corrective change:
   require confirmed keeper persistence before binning losers when automatic
   publication is enabled. Treat both exceptions and False returns as failure,
   retain candidates and report recovery information. Preserve existing keeper
   selection and duplicate scoring rules; add failure/retry tests.

6. **Reported publication status can disagree with canonical storage.** Current:
   [publication.py:320](../services/skills/publication.py#L320). Original:
   [routes/skills_routes.py:1017](https://github.com/odysseus-dev/odysseus/blob/86b886c611338282af6d4a717246ea4e256f58df/routes/skills_routes.py#L1017).
   `test_publication_does_not_report_unpersisted_success` returns False from the
   manager update; the helper reports published while the stored skill remains
   draft. Exceptions are also swallowed by the inspected code. Corrective change:
   check persistence success, log failures and return reloaded canonical state or
   an explicit persistence error. Test False/exception outcomes and the job/UI
   projection. Coordinate this change with item 5 without changing publication
   thresholds or scoring.

Maestrum's read-only triage delegation produced no usable result within its
usefulness window and was cancelled. Cleanup completed and the session records
an interrupted result, not a successful review. The coordinator reclaimed the
remaining report work and verified the source references and reproduction logs
above. No worker claim was used as proof. No GitHub issue, new PR, maintainer
comment or PR-description edit was made.