Complete Phase 22 live provider, email, approval, and Linux journey checks - #39
Coconut-ch1ken wants to merge 1 commit into
Conversation
suraj-subrahmanyan
left a comment
suraj-subrahmanyan
left a comment
There was a problem hiding this comment.
I looked through the PR - there is good coverage, but the current evidence overstates what some tests prove. Please revise so that journey claims use shipped entry points, regression tests are differentiated from from end-to-end journey evidence, unavailable or blocked outcomes are recorded (rather than silently skipped), the acceptance criteria are enforced, and credential-free journeys run in CI.
| if cleanup_output_allowed: | ||
| for path in (temp_path, output_path): | ||
| try: | ||
| path.unlink(missing_ok=True) |
There was a problem hiding this comment.
This can delete an existing valid output when a later validation step fails.
| if ( | ||
| permissions.get("distribution_scope") == "external_email" | ||
| and permissions.get("approval_required") is True | ||
| and permissions.get("approval_state") == "approved" |
There was a problem hiding this comment.
The request can declare itself approved, so this does not prove that external delivery was actually authorized.
| runtime = _delivery_runtime(runtime_payload) | ||
| approval_ref = str(request["permissions"].get("approval_ref") or "").strip() | ||
| provider = str(runtime.get("provider") or runtime_payload.get("provider") or channel).strip().lower() | ||
| delivered = runtime.get("delivered") is True and str(runtime.get("status") or runtime_payload.get("status") or "") == "completed" |
There was a problem hiding this comment.
Caller-provided JSON can claim delivered: true, so this is not reliable proof that the email was actually delivered.
| raise ValueError(f"output directory already exists: {output_dir}") | ||
| staging = output_dir.with_name(output_dir.name + ".tmp") | ||
| if staging.exists(): | ||
| shutil.rmtree(staging) |
There was a problem hiding this comment.
This recursively deletes a predictable .tmp directory that may not belong to this invocation.
| "additionalProperties": false, | ||
| "required": [ | ||
| "schema", "delivery_id", "audience", "delivery_format", "content_scope", | ||
| "permissions", "handoff_checklist", "files", "evidence_index", "provenance", "limitations" |
There was a problem hiding this comment.
This allows a manifest to claim external_email permissions without including any external_delivery evidence.
| } | ||
|
|
||
| with _with_env(env): | ||
| service = ResearchModelService.from_environment(provider_workspace) |
There was a problem hiding this comment.
This calls the internal service directly, so the journey does not prove that users can reach this writer path through the shipped entrypoint.
| def test_p22_045_live_independent_provider_review(repo_root: Path, tmp_path: Path) -> None: | ||
| env = _without_banned_models(_provider_env(repo_root)) | ||
| if not env.get("OPENROUTER_API_KEY") or not env.get("OPENAI_API_KEY"): | ||
| pytest.skip("P22-045 requires OPENROUTER_API_KEY and OPENAI_API_KEY in the live environment.") |
There was a problem hiding this comment.
This skips before recording a journey result, so missing credentials become an unexplained skip instead of the ENVIRONMENT_BLOCKED evidence.
| rec.add_assertion("paper_draft_completed", draft_ev is not None, draft.get("_error")) | ||
| rec.add_assertion("paper_draft_status_understood", draft_status in {"completed", "inconclusive"}, draft_status) | ||
| rec.add_assertion("review_output_exists", review_ev is not None, review.get("_error")) | ||
| rec.add_assertion("review_artifact_status_understood", review_payload.get("status") in {"completed", "inconclusive"}, review_payload.get("status")) |
There was a problem hiding this comment.
This treats an inconclusive review as a passing assertion, allowing the journey to pass without a completed review.
| {"summary", "findings", "evidence-map", "limitations"}.issubset(draft_section_ids), | ||
| sorted({"summary", "findings", "evidence-map", "limitations"} - draft_section_ids), | ||
| ) | ||
| rec.add_assertion("draft_report_substantive", len(markdown_text.split()) >= 50, len(markdown_text.split())) |
There was a problem hiding this comment.
The journey requires a 1,500–2,500-word deliverable, but this accepts a 50-word draft as substantive.
| and doctor_payload.get("paths", {}).get("solar_home") == "ok" | ||
| and doctor_payload.get("paths", {}).get("receipt") == "ok" | ||
| and doctor_payload.get("python", {}).get("min_ok") is True | ||
| and doctor_payload.get("verdict") in {"pass", "fail"} |
There was a problem hiding this comment.
This treats a fail verdict as doctor_ok without checking that the failure is limited to the intentionally skipped dependencies.
Coconut-ch1ken
changed the title
2 participants
What changed
.gitWhy
The remaining Phase 22 journey gaps required real external delivery, independent provider provenance, explicit human approval resume evidence, and Linux/tmux execution.
Validation
7 passed in 25.39sDependency
Stacked on PR #38 because the journey closure evidence assumes the trust-anchor contract.
Scope
Addresses P22-REPAIR-045, 054, 071, and 131 plus their bounded journey evidence. macOS 125/126 evidence was accepted separately and is not changed here.