Skip to content

refactor(ai): route Four Pillars through orchestrator/free - #39

Draft
seonghobae wants to merge 73 commits into
mainfrom
refactor/orchestrator-free-runtime
Draft

seonghobae wants to merge 73 commits into
mainfrom
refactor/orchestrator-free-runtime

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Route every repository-owned LLM path through the Contextual Orchestrator bounded context and make orchestrator/free the fail-closed runtime contract.

Runtime and DDD boundary

  • remove the provider-native NimReportInterpreter, NIM runtime settings, public export, and src/four_pillars/nim.py
  • move OpenAI-compatible transport mechanics and the Contextual Orchestrator client under four_pillars.infrastructure.orchestration
  • keep four_pillars.contextual_orchestrator only as a compatibility re-export
  • reject INTERPRETATION_BACKEND=nvidia_nim, unknown backends, and non-orchestrator/free virtual models
  • preserve ReportInterpreter as the application port for caller-owned MSA composition
  • add an architecture regression test preventing the retired direct-provider path from reappearing

Live and autonomous workflows

  • replace direct NVIDIA NIM smoke/evaluation with a manual Contextual Orchestrator orchestrator/free lane
  • give that live lane only the gateway token and URL, never provider-native credentials
  • retire the repository-specific minute-47 NVIDIA NIM/OpenCode autonomous writer and its self-modifying publication helper
  • keep the minute-17 repository workflow model-free; organization-wide model-backed development is owned by the shared CWL hourly maintainer

Verification contracts

  • add provider-neutral OpenAI-compatible transport tests
  • prove outbound product requests keep model=orchestrator/free across retry, repair, and route/conduct modes
  • prove provider passthrough fields are absent, direct-provider configuration is rejected, and failures do not change routes
  • rename the opt-in marker to orchestrator_live

Documentation

  • accept ADR 0004 and supersede the runtime portions of ADR 0002/0003
  • align PRD, TRD, API, architecture, UML, security, verification, runbooks, agent instructions, standards traceability, and CHANGELOG with the bounded-context decision

Public repository surface

  • add the exact-cased Ask DeepWiki badge linking to https://deepwiki.com/ContextualWisdomLab/four-pillars
  • add docs/index.md as a bounded GitHub Pages source covering product scope, architecture, onboarding, releases, security, and documentation entry points
  • keep live Pages unclaimed until this source is integrated to protected main and the organization-owned metadata reconciler publishes and verifies the site

Merge gate

This PR is intentionally Draft until the exact current head passes the full repository verification/security gates and actionable review findings are resolved. The live orchestrator/free lane remains opt-in because it needs the independently deployed gateway and is supplementary to deterministic CI.

Current exact head after public-surface coordination: 647ee6623c630ad69a54ccb6b01c1ed6587d4b18.

  • Security Scan run 33541708409 and SAST Semgrep run 33541708419 are terminal GREEN.
  • CI run 33541708425 is terminal failure; Draft and the merge block remain correct.
  • The only review thread was verified against both transport_module.asyncio.sleep monkeypatch sites, documented as a false positive, and resolved without a source change.
  • Submitted APPROVED reviews: 0; unresolved threads: 0.
  • The README owns one exact linked DeepWiki badge and docs/index.md is present, but protected-main integration and live Pages publication are not claimed.
  • Predecessor-head checks are not accepted as merge evidence.

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread tests/test_orchestration_transport.py
@seonghobae seonghobae added the enhancement New feature or request label Sep 1, 2026 — with ChatGPT Codex Connector
@seonghobae

Copy link
Copy Markdown
Contributor Author

Local verification of head 647ee66 (read-only, isolated worktree, Python 3.12)

1. Ruff failure (quality 3.11/3.12) — reproduced and fixable.
ruff check . → 1 error in tests/test_orchestration_architecture.py (import block ordering, I001). ruff check --fix resolves it; no semantic change.

2. Hidden behind the Ruff failure: 12 tests fail once pytest runs.

tests/test_backend_url_security.py  (9 cases)  Failed: DID NOT RAISE ValidationError
tests/test_nim_credential_name.py   (2 cases)
tests/test_complete_coverage_support.py::test_report_service_generate_uses_the_nim_boundary

Root cause: this branch removes nim_base_url and nvidia_nim_api_key from Settings (and the nim_base_url validator) but the tests above were not updated. Note CLAUDE.md still states "Direct hosted model work uses NVIDIA_NIM_API_KEY", so the settings removal needs an explicit decision (keep NIM direct path alongside orchestrator/free, or update the boundary + tests together).

Coverage on the branch was 100% for the statements that ran. Ruff fix is held locally (not pushed) pending the settings decision, to avoid a CI run that would fail at pytest anyway.

@seonghobae

Copy link
Copy Markdown
Contributor Author

The two failing quality jobs are one import ordering

Both quality (3.11) and quality (3.12) fail before any test runs. The job installs dependencies, runs ruff check ., and stops at Found 1 error.

The error is in tests/test_orchestration_architecture.py, lines 7 through 12:

from four_pillars.infrastructure.orchestration.contextual_orchestrator import (
    ContextualOrchestratorClient as InfrastructureClient,
)
from four_pillars.contextual_orchestrator import (
    ContextualOrchestratorClient as CompatibilityClient,
)

I001, import block un-sorted. four_pillars.contextual_orchestrator sorts before four_pillars.infrastructure.orchestration.contextual_orchestrator, so the two statements need to swap. Ruff marks it [*], so:

ruff check --fix tests/test_orchestration_architecture.py

This is bound to the current head. The failing job ran on 647ee66, which is still this PR's head, so the failure is live rather than stale. The PR's later updatedAt reflects metadata, not a new commit.

Nothing else in the quality job failed, because nothing else ran. The test suite and the coverage gate have not been evaluated on this head at all, so clearing the lint is what makes the real verdict visible.

strix also fails here, but that is the organisation reviewer agent that is failing across this repository's open PRs and is not a finding about this branch.

I am not pushing to this branch. It is a draft with its own writer and the change belongs to whoever holds it.

@seonghobae

Copy link
Copy Markdown
Contributor Author

This branch moves nim.py, and two fixes that live there are in flight

src/four_pillars/nim.py is deleted here and its transport becomes src/four_pillars/infrastructure/orchestration/openai_compatible.py. Two open pull requests repair defects in exactly that code, and both travel with the rename. Checking the moved file on this branch:

The untrusted-input delimiter is unsealed. Line 167 is verbatim the construction #52 fixes:

f"<input>{json.dumps(user_payload, ensure_ascii=False, default=str)}</input>"

json.dumps escapes quotes and backslashes but not angle brackets, so customer text can emit a literal </input> and close the boundary the line above it declares.

_content has no finish_reason check. That is the defect #53 fixes: a reply the gateway cut short at the token ceiling is validated like a complete one, and because report-section models have optional tail fields a truncated prefix validates and reaches the customer's PDF.

Neither is caused by this branch. Both are inherited from nim.py and would arrive at the new path intact.

The merge is safe but not automatic

I merged this branch with both locally, without pushing anything.

The good news first: the <input> call site merges cleanly. Git follows the rename and line 167 ends up calling _sealed_payload(user_payload). So a resolution that drops the helper produces an immediate NameError rather than a silent reintroduction of the vulnerability. It fails loudly, which is the behaviour you want.

The conflict is one region, and it is about names rather than logic. My side carries NimSchemaError and the NimClient class, which this branch deliberately removes and renames. The resolution is:

  • keep this branch's OrchestrationSchemaError;
  • keep _sealed_payload as a module-level function;
  • drop the NimClient remnant entirely, since this branch removes that class on purpose.

I applied exactly that locally. The file then imports cleanly, ruff check src/ passes, and the seal is present at the new path. #53's change adapts the same way: its NimTruncationError becomes a subclass of OrchestrationTransportError, and self._provider_label becomes self._service_label.

Why I am telling you rather than doing it

This branch is yours and I have not pushed to it. Merge order decides who resolves: if this lands first, #52 and #53 need the rename applied to them; if they land first, this branch inherits both fixes and only needs the two name adaptations above.

Either order works. What should not happen is this landing while #52 and #53 are resolved by taking one side wholesale, because the delimiter seal is a security fix and the truncation check guards what reaches a customer's report.

Separately, on the routing contract

build_report_interpreter now returns ContextualOrchestratorReportInterpreter unconditionally. That satisfies the standing boundary that a selected interpretation backend never silently falls back to another provider, in the strongest possible way: there is no second provider to fall back to. nim.py's client survives only for the evaluation harness, which matches the guidance that new runtime LLM behaviour goes through Contextual Orchestrator.

Copy link
Copy Markdown
Contributor Author

Public-surface and review reconciliation — 2026-09-20

  • source exact head remains 647ee6623c630ad69a54ccb6b01c1ed6587d4b18; no source commit was added
  • README exact linked DeepWiki badge count: 1
  • docs/index.md exists and documents product, architecture, onboarding, security, operations, and release entry points
  • the import-style review finding was checked against concrete uses at lines 255 and 285: the module alias is required to monkeypatch transport_module.asyncio.sleep; the false-positive thread is now documented and resolved
  • exact-head Security Scan and SAST Semgrep are GREEN; CI run 33541708425 is terminal failure
  • lifecycle remains Draft; mergeable true; unresolved threads 0; approvals 0
  • canonical labels normalized to enhancement / priority: medium / status: draft; conflicting type: feature alias removed
  • central desired-state TDD: RED 4df681e2888003350078247775e7c937cd566d09, implementation c0a35c9eddbd29d542ef2978dfa227e3344842c6 in chore(metadata): isolate public-surface desired state .github#2110

No CI bypass, merge, repository-settings mutation, or GitHub Pages publication is claimed.

Copy link
Copy Markdown
Contributor Author

Exact-head CI RCA and ordinary repair — 2026-09-20

The only failing source step on predecessor 647ee6623c630ad69a54ccb6b01c1ed6587d4b18 was not Contextual Orchestrator transport behavior. CI run 33541708425 failed identically on Python 3.11 and 3.12 at ruff check .: I001 in tests/test_orchestration_architecture.py. The compatibility import appeared after the infrastructure import, contrary to Ruff's deterministic first-party-module ordering. Container, dependency installation and pip check had already passed; Security and Semgrep were terminal GREEN on that predecessor only.

Ordinary commit 8233e3e2 applies exactly Ruff's prescribed import order. No runtime code, provider boundary, assertions, workflow, dependency, gate or PR ancestry was changed. The PR remains Draft and mergeable. Current-head hosted runs have not materialized yet, so predecessor GREEN does not transfer and this head is not merge-ready.

Next admission is fresh exact-head CI/Security/SAST plus qualifying review. No manual rerun, source-neutral wake commit, Force Push, bypass, self-approval or gate weakening was used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant