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 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, especially line 342; the bypassed retrieval condition is skills.py:703. Original source: routes/skills_routes.py:1039. 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 and skills.py:59. Original source: routes/skills_routes.py:710. 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 and job_coordinator.py:49. Original: routes/skills_routes.py:635. 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, teacher retry at line 280 and demotion at line 303. Original: routes/skills_routes.py:1448 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, including loser writes at line 228. Original: routes/skills_routes.py:914. 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. Original: routes/skills_routes.py:1017. 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.