chore(#3d): delete the OpenCode substrate — Pydantic AI is the only agent runtime#254
Merged
galanko merged 14 commits intoJun 3, 2026
Merged
Conversation
Drop config_manager / opencode_client from the settings routes — the last
contract-bearing OpenCode dependency outside the engine package:
- GET/PUT /settings/model now route only through AIIntegrationService
(canonical app_setting(model)); no opencode.json fallback. Empty
ModelConfig when no provider is connected; PUT 503s without a vault and
400s with no active provider / prefix mismatch.
- GET /settings/providers is rebuilt from cliff.ai.catalog (static
per-provider picker rows) instead of OpenCode's models.dev dump. Wire
shape {id,name,env,models} is preserved; models keyed by the bare id so
the UI/CLI rebuild f"{provider}/{model_id}" unchanged.
- POST /settings/providers/test now runs a bounded "Say OK" through
Pydantic AI (build_model + Agent) and classifies the outcome.
- Delete the vestigial config_manager-only routes /settings/providers/
configured and /settings/api-keys/* (no live UI consumer) plus their
dead frontend wrappers/hooks/types and the MSW api-keys handler.
CLI (cliffsec model get/set/list) is unaffected — same wire shapes.
Part of PR #3d (OpenCode substrate removal), stacked on #253.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ADR-0047: the PA model factory reads the provider key straight from the resolved env (resolve_env_for_workspace) — there is no auth.json to keep in sync. Remove sync_to_opencode, _sync_opencode_auth, _clear_opencode_auth, _safe_write_opencode_config, the _OPENCODE_AUTH_PROVIDERS set, and all their call sites in save/disconnect/set_model. Canonical vault + app_setting(model) state is untouched; _fire_key_change stays as a generic env-refresh hook. Drop the now-obsolete auth.json-push tests and the dead opencode_client stub fixtures. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ADR-0047: agents run in-process via Pydantic AI, so nothing spawns or talks to an OpenCode process anymore. Remove from the lifespan: - the singleton opencode_process start/stop + set_extra_env seeding - the auth.json sync_to_opencode reconcile + config_manager reconcile_model / restore_keys_to_engine startup calls - the WorkspaceProcessPool (constructed but never read after #3c — the executor never called it), its app.state handle, the idle-cleanup loop, and pool.stop_all on shutdown - the singleton-restart half of the ai_on_key_change hook (it now only refreshes the warm env/model cache) - opencode_client.close() / opencode_process.stop() teardown AgentExecutor drops its unused ``pool`` parameter. Test construction sites updated to the new signature. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ates ADR-0047: PA agents run in-process from prompts defined in cliff.agents.runtime — they never read a workspace's opencode.json or rendered .opencode/agents/*.md. Stop writing both: - WorkspaceDirManager.create() drops the .opencode/agents tree, the opencode.json write, and its mcp_servers/model params. The dir still carries the finding context (finding.json, finding.md, CONTEXT.md, context sections) the agents read. - create_repo_workspace() becomes a thin clone-dir allocator (history/ + REPO_ACTION.md); no rendered prompt, no opencode.json. The repo-action agent carries its own prompt + permission policy. - WorkspaceContextBuilder drops the AgentTemplateEngine dependency and the per-create / per-update write_agents render passes; it still writes the workspace-integrations.json manifest (read by check_config_freshness). - Delete _build_opencode_config and the WorkspaceDir opencode_json / opencode_dir / agents_dir properties. - main.py lifespan + _engine_dep spawner drop the template engine / model plumbing. Tests updated to assert finding context (not templates); the obsolete opencode.json/template assertions and the OpenCode pool-wiring test (test_ai_phase_f_wiring) are removed. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move VersionInfo out of the doomed cliff.engine.models into cliff.models alongside HealthStatus, and add a shared substrate_version() helper. The /version handshake's compat ``opencode`` field and /health's ``opencode_version`` both now carry "pydantic-ai <ver>" (ADR-0047) instead of the pinned OpenCode binary version. /health stops falling back to settings.opencode_model (opencode.json is gone) — the model is the canonical ai_model_cache or empty. The config.py opencode_* settings removal rides with the engine deletion (they're read by engine/ at runtime until it's deleted). Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Every OpenCode caller is gone (#3b/#3c + steps 1-5), so remove the engine itself and everything that fed it: Backend - delete backend/cliff/engine/ (client, process, pool, config_manager, models) and backend/cliff/agents/template_engine.py + templates/*.j2 - config.py: drop the opencode_* connection/model/version settings and properties; repo-root sentinel moves from .opencode-version to VERSION - agents/__init__.py stops re-exporting the template engine - delete the engine/pool/template unit tests + the OpenCode e2e suite; strip the opencode fixtures from the conftests; retarget test_config / test_docker / the openrouter route test; regenerate the OpenAPI snapshot Packaging / CLI - Dockerfile: drop the OpenCode binary install step, the opencode.json / .opencode COPYs, and the CLIFF_OPENCODE_* env - build-tarball / install-local / install smoke / install-smoke.yml stop bundling + installing + asserting the opencode binary - CLI updater stops re-running install-opencode.sh; daemon doctor drops the opencode binary check (it would report "not found" post-migration) Root - delete .opencode-version, opencode.json, .opencode/, scripts/install-opencode.sh Follow-up (noted, not blocking): the CLI's process_sweep still classifies a ``$CLIFF_HOME/bin/opencode`` process kind and cli.py keeps the (now always "ok") opencode health field — dead but harmless; a separate cleanup PR. Backend 1432 pass, CLI 97 pass, frontend tsc clean, ruff clean. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Architecture table, repo layout, "How It Runs", the workspace-runtime layers, the AI-provider section, and the testing guidance now describe the in-process Pydantic AI substrate (ADR-0047) instead of the OpenCode subprocess / process pool / auth.json / drift model. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR eliminates the OpenCode subprocess engine and refactors Cliff to run agents in-process via Pydantic AI (ADR-0047). Configuration is simplified by removing OpenCode-specific settings and switching repo root detection to ChangesRefactor from OpenCode subprocess to in-process Pydantic AI agents (ADR-0047)
🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Comment on lines
19
to
24
| # ``ai_model_cache`` is the canonical active model resolved at boot / on | ||
| # provider change; fall back to the configured default. | ||
| model = ( | ||
| getattr(request.app.state, "ai_model_cache", None) | ||
| or settings.opencode_model | ||
| ) | ||
| # provider change (ADR-0047); empty string when no provider is connected. | ||
| model = getattr(request.app.state, "ai_model_cache", None) | ||
|
|
||
| # ``ai_env_cache`` is the resolved provider env. A non-empty cache means a | ||
| # provider credential is present *and* resolved (vault decrypt succeeded); |
There was a problem hiding this comment.
/health reads model only from ai_model_cache, so on the vault-missing path in backend/cliff/main.py:213-216 where the cache is cleared/unset, HealthStatus.model returns "" even when a canonical model exists — should we fall back to settings.opencode_model (or DB) when ai_model_cache is None?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
In backend/cliff/api/routes/health.py around lines 19-36, update the model resolution to
fall back to the canonical configured model (e.g., `settings.opencode_model` or DB
source) when `ai_model_cache` is None, so `HealthStatus.model` is never empty when a
model is configured. Also verify backend/cliff/main.py around lines 213-216: the
vault-missing path should either initialize `ai_model_cache` to the canonical model or
avoid clearing it when only provider credentials are missing. Ensure `model=model or ""`
uses the resolved fallback, and keep the existing `opencode_version=substrate_version()`
change intact.
Collaborator
Author
There was a problem hiding this comment.
The settings.opencode_model fallback can't be restored — it read opencode.json, which this PR deletes. ai_model_cache is now the single authoritative resolved model.
On the vault-missing path it's empty by construction, and that's correct: a provider can only be connected via BYOK/OAuth, both of which require the vault to store the credential. No vault ⇒ no ai_integration row ⇒ no resolvable model ⇒ model="" and ai_provider_ready=False, which is the honest "AI not configured" state. There's no scenario where a usable canonical model exists but /health.model shows "".
cliffsec status also doesn't depend on /health.model: it reads /api/integrations/ai/status first (DB-backed app_setting(model), no vault needed for the model read) and only falls back to health["model"]. So no code change here.
There was a problem hiding this comment.
Thanks, that makes sense — ai_model_cache is the authoritative source here, and model="" is the correct unconfigured state in the no-vault path. I'll save this to memory once the PR is merged.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Comment/docstring-only. The PA code was already correct; the prose still described OpenCode subprocesses, an auth.json/singleton restart, a process pool, and opencode.json MCP config that no longer exist. Reword the load-bearing-wrong comments to describe the in-process Pydantic AI behaviour (executor dispatch, errors, repo runner, tools, provider/catalog, ai/service + models, gateway), and drop already-happened "deleted in PR2.E" / "PR #3 finalizes" future-tense references. Kept: accurate "used to run on OpenCode / pre-migration" provenance that explains why code is shaped the way it is, the intentional ``opencode`` HealthStatus/VersionInfo wire-compat field, and the legacy_migration notes about legacy opencode-keyed app_settings. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
backend/tests/test_executor.py (1)
385-399:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis test no longer exercises the parse-failure path.
AgentExecutor(mock_context_builder)now hits_resolve_active_model()before any remediation-executor output is parsed, so this setup fails on missing AI resolvers rather than on malformed agent output. The assertions still pass, but they no longer protect the regression described in the docstring. Stub_run_pa_executor()here or construct the executor through_executor_with_pa(...)so the failure is driven by bad executor output instead of configuration wiring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_executor.py` around lines 385 - 399, The test currently exercises configuration wiring because AgentExecutor(mock_context_builder) triggers _resolve_active_model() before parsing executor output; to restore coverage of the parse-failure path, prevent _resolve_active_model() from running by stubbing the executor output instead: either patch/stub AgentExecutor._run_pa_executor() to return the malformed output that triggers the parse-failure, or create the executor via the helper _executor_with_pa(...) which sets up a preconfigured PA executor; update the test setup to patch "cliff.agents.executor.AgentExecutor._run_pa_executor" (or call _executor_with_pa) so the failure is driven by bad executor output rather than missing AI resolvers, keeping the rest of the assertions intact.backend/cliff/main.py (1)
239-244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't flip AI readiness to true before the boot probe completes.
_refresh_ai_env_cache(verify=False)currently marksai_provider_credential_ok = Trueas soon as a credential resolves, and the boot path calls that before the background verification probe starts. That makes startup report ready during the exact window where a revoked or wrong key has not been authenticated yet, which contradicts the readiness contract documented in this function. The boot warm-cache path needs to keep readiness false until the async probe succeeds, while the post-save hook can still trust its already-validated credential.Also applies to: 268-277
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cliff/main.py` around lines 239 - 244, The boot warm-cache path currently sets app.state.ai_provider_credential_ok = True inside _refresh_ai_env_cache(verify=False) which flips readiness before the async verification probe runs; change the logic so _refresh_ai_env_cache(verify=False) does not set app.state.ai_provider_credential_ok when invoked from the boot/warm-cache path—leave readiness false until the background probe (the async verify path invoked later) sets app.state.ai_provider_credential_ok = True on success; keep the existing behavior for the post-save hook (which calls _refresh_ai_env_cache with a context that can trust the credential) so that saving still marks readiness immediate only in that trusted code path.backend/tests/api/openapi_snapshot.json (1)
4955-4980:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
VersionInfo.opencoderequired while claiming backward compatibility.The new description says
opencodeis retained for a backward-compatible wire shape, but Line 4978 drops it fromrequired. That weakens the/api/versioncontract exactly where oldercliffsec statuscallers are supposed to keep working.Suggested contract fix
"required": [ - "cliff" + "cliff", + "opencode" ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/api/openapi_snapshot.json` around lines 4955 - 4980, The VersionInfo schema has dropped "opencode" from its required list which breaks backward-compatible wire shape for /api/version; update the VersionInfo definition (schema named VersionInfo in the OpenAPI snapshot) to include "opencode" in the "required" array (i.e., add "opencode" alongside "cliff"), retain its default/title/type entries, and ensure any code that generates this openapi snapshot (or the source model used to produce it) marks VersionInfo.opencode as required so older cliffsec status clients continue to receive that field.backend/cliff/workspace/workspace_dir_manager.py (1)
64-85:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix misleading
GIT_CEILING_DIRECTORIESwarning (stale safety claim)
backend/cliff/workspace/workspace_dir_manager.pystill warns that agent git operations are “confined viaGIT_CEILING_DIRECTORIES” (and referencesengine/pool.py), butGIT_CEILING_DIRECTORIESis only referenced in comments (no backend runtime code sets/injects it). Meanwhile backend runtime code does spawngitsubprocesses (e.g.,backend/cliff/assessment/clone.py/backend/cliff/assessment/scope.py/backend/cliff/integrations/github_app/client.py). Update the warning to match the current containment mechanism (workspace isolation/tool gating), or reintroduce explicitGIT_CEILING_DIRECTORIESinjection into the in-process git execution path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cliff/workspace/workspace_dir_manager.py` around lines 64 - 85, The warning in workspace_dir_manager.py incorrectly claims git operations are "confined via GIT_CEILING_DIRECTORIES" even though no runtime code sets that env var; update the logger call (the block using _git_ancestor(base_dir) and the logger.warning invocation) to either (A) remove/reference GIT_CEILING_DIRECTORIES as deprecated and instead mention the actual containment mechanism ("workspace isolation/tool gating") or (B) implement real protection by injecting GIT_CEILING_DIRECTORIES into the environment for any in-process git subprocesses (update the subprocess/git invocation sites in backend/cliff/assessment/clone.py, backend/cliff/assessment/scope.py, and backend/cliff/integrations/github_app/client.py to add GIT_CEILING_DIRECTORIES pointing at the intended ceiling directories). Choose one approach and make the logger message and code consistent with it.scripts/install-local.sh (1)
205-217:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreflight
bashbefore invoking the scanner helper.This block now depends on
bash, but the prerequisite scan still only validatescurl,tar,git, andgh. On hosts withoutbash, the install now fails here after the tarball is unpacked and the backend venv is already synced. Addbashto the earlier preflight list so the failure is immediate and actionable.🔧 Suggested fix
-missing="" -for cmd in curl tar git; do +missing="" +for cmd in curl tar git bash; doBased on learnings: keep every
install-local.shPOSIX-compliant and invoke Bash-specific helpers directly so their shebang takes effect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install-local.sh` around lines 205 - 217, Add 'bash' to the preflight prerequisite checks in scripts/install-local.sh so the script fails early on systems without bash before unpacking the tarball; update the same list/array that currently validates curl, tar, git, and gh (e.g., REQUIRED_CMDS or PRE_REQS) to include bash (and ensure the check uses command -v or which), leaving the scanner helper invocation (using APP_DIR, BIN_DIR, and calling "${APP_DIR}/scripts/install-scanners.sh" with CLIFF_SCANNER_VERIFY) unchanged so its shebang can run when bash is present.backend/tests/conftest.py (1)
35-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the shared FastAPI app after each
clientfixture.
cliff.main.appis a singleton. This fixture overwritesapp.router.lifespan_contextand yields without restoring it or clearing mutatedapp.state, so tests that touch the same app object can leak state into laterclientcases and become order-dependent.💡 Minimal containment fix
`@pytest.fixture` def client(): """FastAPI test client with a no-op lifespan (ADR-0047 — in-process substrate, nothing to spawn).""" from cliff.main import app - app.router.lifespan_context = _noop_lifespan - with TestClient(app) as c: - yield c + original_lifespan = app.router.lifespan_context + original_state = { + "vault": getattr(app.state, "vault", None), + "audit_logger": getattr(app.state, "audit_logger", None), + "context_builder": getattr(app.state, "context_builder", None), + "process_pool": getattr(app.state, "process_pool", None), + "assessment_tasks": getattr(app.state, "assessment_tasks", None), + } + app.router.lifespan_context = _noop_lifespan + try: + with TestClient(app) as c: + yield c + finally: + app.router.lifespan_context = original_lifespan + for key, value in original_state.items(): + setattr(app.state, key, value)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/conftest.py` around lines 35 - 42, The client fixture mutates the singleton cliff.main.app (overwrites app.router.lifespan_context and yields without cleanup), causing test state leakage; modify the client fixture to save the original app.router.lifespan_context and a shallow copy of app.state before assigning _noop_lifespan, then after the TestClient context (or in a finally block) restore the original lifespan_context and reset app.state to the saved copy (clearing any keys added during the test) so that the cliff.main.app singleton is returned to its prior state for subsequent tests; reference the client fixture, cliff.main.app, _noop_lifespan, app.router.lifespan_context, and app.state when making the change.backend/cliff/workspace/context_builder.py (1)
99-128:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRollback the DB/finding mutation if workspace setup fails.
create_workspace()inserts the row and flips the finding toin_progressbefore any filesystem/path persistence. If_dir_manager.create(), the manifest write, orupdate_workspace_dir()fails, you leave behind a workspace row and finding state with no usable directory/path. This needs a compensating cleanup path, or the status flip needs to move after the directory + DB path are durable.As per coding guidelines, "The
WorkspaceContextBuilderorchestrates the directory, finding context, and DB metadata."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cliff/workspace/context_builder.py` around lines 99 - 128, The workspace creation currently updates the DB and flips the finding to in_progress before filesystem and path persistence, so failures in WorkspaceContextBuilder (e.g., self._dir_manager.create, writing workspace-integrations.json, or update_workspace_dir) leave a dangling DB row and bad finding state; modify create_workspace()/WorkspaceContextBuilder so either (A) perform filesystem creation (self._dir_manager.create) and update_workspace_dir(db, workspace.id, ...) first and only then flip the finding status to in_progress, or (B) add a compensating rollback path on any exception that deletes the inserted workspace row and restores the finding state; ensure the rollback targets the same identifiers used currently (workspace.id, update_workspace_dir, and the workspace-integrations.json write) and that exceptions from self._mcp_resolver resolution are still handled without preventing cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/cliff/api/routes/settings.py`:
- Around line 136-164: Update the list_providers endpoint to return typed
Pydantic models: add response_model=list[ProviderInfo] to the `@router.get`
decorator and change the payload construction to build and return ProviderInfo
instances (not raw dicts). Inside list_providers (which iterates
catalog.all_providers(), catalog.get(provider), and
catalog.picker_options(provider)), convert each provider entry to
ProviderInfo(id=..., name=..., env=[...] or [], models=...) where models is a
mapping from bare model id to the appropriate model schema expected by
ProviderInfo; ensure you import ProviderInfo and any nested model types and
update the payload typing to list[ProviderInfo] before returning it.
In `@backend/cliff/workspace/context_builder.py`:
- Around line 170-182: The update_context path currently writes the filesystem
section (self._dir_manager.write_context_section), appends to agent-runs.jsonl
(AgentRunLog.append) and bumps the DB version (increment_context_version) but
does not persist the structured_output into the SidebarState mirror; add a call
after run_log.append and before increment_context_version to persist the same
structured_output (and section/workspace_id) into the sidebar store so sidebar
reads are fresh — e.g., invoke your SidebarState/SidebarStore persistence method
(via the existing sidebar manager instance, e.g.
self._sidebar_state_manager.persist or similar) with workspace_id, section, and
structured_output to mirror the agent result into SidebarState.
In `@backend/tests/agents/conftest.py`:
- Around line 23-28: The pytest_collection_modifyitems hook currently adds
_skip_no_key to every test under backend/tests/agents and should instead only
skip tests that require a real LLM; update pytest_collection_modifyitems so it
still marks items in the agents directory with pytest.mark.agent but only adds
_skip_no_key when the test item is explicitly marked for live LLMs (e.g., has
marker "live_llm" or "live") or matches the dedicated live-eval test filename
pattern (e.g., contains "eval_" in str(item.fspath)), leaving
FunctionModel/TestModel unit tests unskipped; modify the logic around
pytest_collection_modifyitems and the use of _skip_no_key to check
item.get_closest_marker("live_llm") (or inspect the filename) before calling
item.add_marker(_skip_no_key).
In `@backend/tests/integration/test_workspace_repo_url_overrides_global.py`:
- Around line 24-34: The fixture real_builder currently replaces
app.state.context_builder and never restores it; capture the original builder
(orig = app.state.context_builder) before assigning the new
WorkspaceContextBuilder (created with WorkspaceDirManager and
mcp_resolver=None), yield the new builder as now, and after the yield restore
app.state.context_builder = orig so tests don’t leak the mutated singleton;
update the fixture body to perform this save/restore around the yield while
keeping the WorkspaceContextBuilder and WorkspaceDirManager usage intact.
In `@backend/tests/test_workspace_repo_creation.py`:
- Around line 34-46: The test for create_repo_workspace() verifies the workspace
directory and history folder but misses asserting the presence of the history
log file; update the test around ws_dir (derived from manager.base_dir /
workspace_id) to also assert that (ws_dir / "history" /
"agent-runs.jsonl").exists() so regressions that drop the touched file are
caught; locate the assertions block that checks ws_dir and add this single
assertion referencing agent-runs.jsonl.
In `@CLAUDE.md`:
- Line 35: Update the private cliff-os ADR/IMPL documents to match the new
in-process substrate and canonical AI-state flow described in this PR: modify
ADR 0047-pydantic-ai-substrate.md and ADR 0014-workspace-runtime-architecture.md
(and any related IMPL plans) to reflect the substrate/runtime rewrite and
workspace isolation changes, include the new state flow diagrams and behavioral
details, and add a brief changelog entry so the public CLAUDE.md statements are
consistent with the private source-of-truth before merging.
In `@cli/cliff_cli/daemon.py`:
- Line 505: The doctor/stop checks still treat OpenCode/workspace ports (4096
and 4100-4102) as Cliff-owned; update _cliff_ports() and any hard-coded port
loops (the loop that iterates over ("trivy","semgrep") and the doctor port list)
to remove 4096 and 4100-4102 so those ports are no longer considered
Cliff-owned, and eliminate any remaining references to WorkspaceProcessPool or
the old subprocess substrate in daemon.py that caused those port assumptions.
In `@cli/tests/test_daemon.py`:
- Around line 489-491: The test currently only checks that expected checks are
present via expected.issubset(names) but does not ensure a stale "opencode"
check is absent; update the assertion logic in the test around payload["checks"]
(the names set and expected variables) to explicitly assert that "opencode" is
not in names (e.g., add a negative assertion like "opencode" not in names) or
replace the subset check with an exact-match assertion against the full expected
set to lock the migration contract.
In `@scripts/build-tarball.sh`:
- Around line 14-15: The generated README-LOCAL-INSTALL.md contains stale text
claiming the installer lays down `opencode` binaries; update the README
generation in scripts/build-tarball.sh to remove any references to `opencode`
and change the installer description to reference only the shipped
`install-scanners.sh` (and any shipped files like `scripts/` or
`.scanner-versions`) so the produced README accurately reflects the tarball
contents; locate the README template/emission code in build-tarball.sh (search
for README-LOCAL-INSTALL.md, `opencode`, and `install-scanners.sh`) and edit the
template text to remove the outdated installer wording and replace it with a
concise statement that the tarball includes `install-scanners.sh` for
installation.
---
Outside diff comments:
In `@backend/cliff/main.py`:
- Around line 239-244: The boot warm-cache path currently sets
app.state.ai_provider_credential_ok = True inside
_refresh_ai_env_cache(verify=False) which flips readiness before the async
verification probe runs; change the logic so _refresh_ai_env_cache(verify=False)
does not set app.state.ai_provider_credential_ok when invoked from the
boot/warm-cache path—leave readiness false until the background probe (the async
verify path invoked later) sets app.state.ai_provider_credential_ok = True on
success; keep the existing behavior for the post-save hook (which calls
_refresh_ai_env_cache with a context that can trust the credential) so that
saving still marks readiness immediate only in that trusted code path.
In `@backend/cliff/workspace/context_builder.py`:
- Around line 99-128: The workspace creation currently updates the DB and flips
the finding to in_progress before filesystem and path persistence, so failures
in WorkspaceContextBuilder (e.g., self._dir_manager.create, writing
workspace-integrations.json, or update_workspace_dir) leave a dangling DB row
and bad finding state; modify create_workspace()/WorkspaceContextBuilder so
either (A) perform filesystem creation (self._dir_manager.create) and
update_workspace_dir(db, workspace.id, ...) first and only then flip the finding
status to in_progress, or (B) add a compensating rollback path on any exception
that deletes the inserted workspace row and restores the finding state; ensure
the rollback targets the same identifiers used currently (workspace.id,
update_workspace_dir, and the workspace-integrations.json write) and that
exceptions from self._mcp_resolver resolution are still handled without
preventing cleanup.
In `@backend/cliff/workspace/workspace_dir_manager.py`:
- Around line 64-85: The warning in workspace_dir_manager.py incorrectly claims
git operations are "confined via GIT_CEILING_DIRECTORIES" even though no runtime
code sets that env var; update the logger call (the block using
_git_ancestor(base_dir) and the logger.warning invocation) to either (A)
remove/reference GIT_CEILING_DIRECTORIES as deprecated and instead mention the
actual containment mechanism ("workspace isolation/tool gating") or (B)
implement real protection by injecting GIT_CEILING_DIRECTORIES into the
environment for any in-process git subprocesses (update the subprocess/git
invocation sites in backend/cliff/assessment/clone.py,
backend/cliff/assessment/scope.py, and
backend/cliff/integrations/github_app/client.py to add GIT_CEILING_DIRECTORIES
pointing at the intended ceiling directories). Choose one approach and make the
logger message and code consistent with it.
In `@backend/tests/api/openapi_snapshot.json`:
- Around line 4955-4980: The VersionInfo schema has dropped "opencode" from its
required list which breaks backward-compatible wire shape for /api/version;
update the VersionInfo definition (schema named VersionInfo in the OpenAPI
snapshot) to include "opencode" in the "required" array (i.e., add "opencode"
alongside "cliff"), retain its default/title/type entries, and ensure any code
that generates this openapi snapshot (or the source model used to produce it)
marks VersionInfo.opencode as required so older cliffsec status clients continue
to receive that field.
In `@backend/tests/conftest.py`:
- Around line 35-42: The client fixture mutates the singleton cliff.main.app
(overwrites app.router.lifespan_context and yields without cleanup), causing
test state leakage; modify the client fixture to save the original
app.router.lifespan_context and a shallow copy of app.state before assigning
_noop_lifespan, then after the TestClient context (or in a finally block)
restore the original lifespan_context and reset app.state to the saved copy
(clearing any keys added during the test) so that the cliff.main.app singleton
is returned to its prior state for subsequent tests; reference the client
fixture, cliff.main.app, _noop_lifespan, app.router.lifespan_context, and
app.state when making the change.
In `@backend/tests/test_executor.py`:
- Around line 385-399: The test currently exercises configuration wiring because
AgentExecutor(mock_context_builder) triggers _resolve_active_model() before
parsing executor output; to restore coverage of the parse-failure path, prevent
_resolve_active_model() from running by stubbing the executor output instead:
either patch/stub AgentExecutor._run_pa_executor() to return the malformed
output that triggers the parse-failure, or create the executor via the helper
_executor_with_pa(...) which sets up a preconfigured PA executor; update the
test setup to patch "cliff.agents.executor.AgentExecutor._run_pa_executor" (or
call _executor_with_pa) so the failure is driven by bad executor output rather
than missing AI resolvers, keeping the rest of the assertions intact.
In `@scripts/install-local.sh`:
- Around line 205-217: Add 'bash' to the preflight prerequisite checks in
scripts/install-local.sh so the script fails early on systems without bash
before unpacking the tarball; update the same list/array that currently
validates curl, tar, git, and gh (e.g., REQUIRED_CMDS or PRE_REQS) to include
bash (and ensure the check uses command -v or which), leaving the scanner helper
invocation (using APP_DIR, BIN_DIR, and calling
"${APP_DIR}/scripts/install-scanners.sh" with CLIFF_SCANNER_VERIFY) unchanged so
its shebang can run when bash is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ee62b455-cade-4cbc-a7ec-3bfd95e50146
📒 Files selected for processing (86)
.github/workflows/install-smoke.yml.opencode-version.opencode/agents/security-orchestrator.mdCLAUDE.mdbackend/cliff/agents/__init__.pybackend/cliff/agents/executor.pybackend/cliff/agents/template_engine.pybackend/cliff/agents/templates/dependabot_config_generator.md.j2backend/cliff/agents/templates/enricher.md.j2backend/cliff/agents/templates/evidence_collector.md.j2backend/cliff/agents/templates/exposure_analyzer.md.j2backend/cliff/agents/templates/orchestrator.md.j2backend/cliff/agents/templates/owner_resolver.md.j2backend/cliff/agents/templates/remediation_executor.md.j2backend/cliff/agents/templates/remediation_planner.md.j2backend/cliff/agents/templates/security_md_generator.md.j2backend/cliff/agents/templates/validation_checker.md.j2backend/cliff/ai/service.pybackend/cliff/api/_engine_dep.pybackend/cliff/api/routes/health.pybackend/cliff/api/routes/settings.pybackend/cliff/api/routes/version.pybackend/cliff/config.pybackend/cliff/engine/__init__.pybackend/cliff/engine/client.pybackend/cliff/engine/config_manager.pybackend/cliff/engine/models.pybackend/cliff/engine/pool.pybackend/cliff/engine/process.pybackend/cliff/main.pybackend/cliff/models/__init__.pybackend/cliff/workspace/context_builder.pybackend/cliff/workspace/workspace_dir.pybackend/cliff/workspace/workspace_dir_manager.pybackend/tests/agents/conftest.pybackend/tests/agents/fixtures/plain_description_evals.jsonbackend/tests/agents/test_deferred_tools_persist.pybackend/tests/agents/test_executor_provider_chain.pybackend/tests/api/openapi_snapshot.jsonbackend/tests/conftest.pybackend/tests/e2e/__init__.pybackend/tests/e2e/conftest.pybackend/tests/e2e/test_agent_pipeline_e2e.pybackend/tests/e2e/test_health_e2e.pybackend/tests/e2e/test_process_pool_e2e.pybackend/tests/e2e/test_repo_workspace_agents.pybackend/tests/e2e/test_settings_e2e.pybackend/tests/integration/test_permission_flow.pybackend/tests/integration/test_rate_limit_backoff.pybackend/tests/integration/test_workspace_npm_cache_isolation.pybackend/tests/integration/test_workspace_repo_url_overrides_global.pybackend/tests/test_agent_template_engine.pybackend/tests/test_ai_phase_f_wiring.pybackend/tests/test_ai_service.pybackend/tests/test_config.pybackend/tests/test_config_manager.pybackend/tests/test_context_builder.pybackend/tests/test_docker.pybackend/tests/test_engine_client.pybackend/tests/test_engine_process.pybackend/tests/test_executor.pybackend/tests/test_gateway.pybackend/tests/test_mcp_gateway_integration.pybackend/tests/test_permission_client.pybackend/tests/test_process_pool.pybackend/tests/test_repo_action_templates.pybackend/tests/test_routes_ai_integrations.pybackend/tests/test_routes_ai_openrouter.pybackend/tests/test_routes_settings.pybackend/tests/test_routes_version.pybackend/tests/test_workspace_dir.pybackend/tests/test_workspace_integrations.pybackend/tests/test_workspace_repo_creation.pycli/cliff_cli/daemon.pycli/cliff_cli/updater.pycli/tests/test_daemon.pycli/tests/test_updater.pydocker/Dockerfilefrontend/src/api/client.tsfrontend/src/api/hooks.tsfrontend/src/test/msw/sessionHandlers.tsopencode.jsonscripts/build-tarball.shscripts/install-local.shscripts/install-opencode.shtests/install/smoke.sh
💤 Files with no reviewable changes (51)
- .opencode/agents/security-orchestrator.md
- backend/cliff/agents/templates/evidence_collector.md.j2
- backend/cliff/agents/templates/remediation_planner.md.j2
- scripts/install-opencode.sh
- opencode.json
- .opencode-version
- backend/cliff/agents/templates/exposure_analyzer.md.j2
- backend/cliff/agents/templates/dependabot_config_generator.md.j2
- .github/workflows/install-smoke.yml
- backend/tests/test_engine_client.py
- backend/cliff/workspace/workspace_dir.py
- backend/tests/e2e/test_health_e2e.py
- backend/cliff/engine/init.py
- backend/cliff/engine/models.py
- tests/install/smoke.sh
- backend/cliff/engine/client.py
- backend/tests/test_agent_template_engine.py
- backend/cliff/agents/templates/security_md_generator.md.j2
- backend/cliff/agents/templates/owner_resolver.md.j2
- backend/tests/test_config_manager.py
- backend/cliff/agents/templates/validation_checker.md.j2
- backend/cliff/engine/process.py
- frontend/src/test/msw/sessionHandlers.ts
- backend/tests/e2e/test_agent_pipeline_e2e.py
- backend/tests/e2e/test_settings_e2e.py
- backend/tests/test_engine_process.py
- backend/cliff/agents/templates/orchestrator.md.j2
- backend/cliff/agents/templates/enricher.md.j2
- backend/cliff/api/_engine_dep.py
- backend/tests/test_repo_action_templates.py
- backend/tests/test_routes_ai_openrouter.py
- backend/tests/test_permission_client.py
- backend/tests/e2e/test_process_pool_e2e.py
- backend/tests/integration/test_rate_limit_backoff.py
- backend/cliff/agents/template_engine.py
- backend/tests/agents/test_deferred_tools_persist.py
- backend/cliff/engine/config_manager.py
- backend/tests/test_process_pool.py
- backend/cliff/agents/templates/remediation_executor.md.j2
- frontend/src/api/client.ts
- frontend/src/api/hooks.ts
- backend/tests/integration/test_workspace_npm_cache_isolation.py
- backend/tests/e2e/test_repo_workspace_agents.py
- backend/tests/test_ai_phase_f_wiring.py
- backend/tests/test_workspace_dir.py
- backend/tests/agents/test_executor_provider_chain.py
- backend/tests/e2e/conftest.py
- backend/cliff/engine/pool.py
- backend/tests/test_routes_ai_integrations.py
- cli/tests/test_updater.py
- backend/tests/test_gateway.py
Comment on lines
+136
to
+164
| @router.get("/settings/providers") | ||
| async def list_providers(): | ||
| try: | ||
| return await config_manager.list_available_providers() | ||
| except Exception as exc: | ||
| raise HTTPException(status_code=502, detail=f"OpenCode unavailable: {exc}") from exc | ||
|
|
||
| """Return the supported-provider catalog (ADR-0037). | ||
|
|
||
| @router.get("/settings/providers/configured") | ||
| async def get_configured_providers(): | ||
| try: | ||
| providers = await config_manager.get_configured_providers() | ||
| auth = await config_manager.get_auth_status() | ||
| return {"providers": providers, "auth": auth} | ||
| except Exception as exc: | ||
| raise HTTPException(status_code=502, detail=f"OpenCode unavailable: {exc}") from exc | ||
| Built from the static :mod:`cliff.ai.catalog` — one entry per | ||
| provider with its key env var and the curated model picker rows. The | ||
| wire shape (``{id, name, env, models}`` with ``models`` keyed by the | ||
| bare model id) is what the Settings model picker and ``cliffsec model | ||
| list`` consume. | ||
| """ | ||
| payload: list[dict] = [] | ||
| for provider in catalog.all_providers(): | ||
| info = catalog.get(provider) | ||
| models: dict[str, dict] = {} | ||
| for opt in catalog.picker_options(provider): | ||
| # The picker id is the full ``<provider>/<model>`` id; key by | ||
| # the bare model id so ``f"{provider}/{model_id}"`` round-trips | ||
| # (the UI and CLI both rebuild the full id that way). | ||
| bare = opt.id.split("/", 1)[1] if "/" in opt.id else opt.id | ||
| models[bare] = {"id": bare, "name": opt.label} | ||
| payload.append( | ||
| { | ||
| "id": provider, | ||
| "name": info.docs_label, | ||
| "env": [info.env_var_name] if info.env_var_name else [], | ||
| "models": models, | ||
| } | ||
| ) | ||
| return payload |
There was a problem hiding this comment.
Restore a typed API contract for /settings/providers.
Returning raw dicts here drops FastAPI response validation and weakens the generated OpenAPI schema for a user-facing endpoint, even though ProviderInfo already models this shape. Please add response_model=list[ProviderInfo] and return ProviderInfo(...) instances so the CLI/frontend keep a stable contract.
🧩 Suggested fix
from cliff.models import (
CredentialCreate,
CredentialInfo,
IntegrationConfig,
IntegrationConfigCreate,
IntegrationConfigUpdate,
IntegrationHealthStatus,
ModelConfig,
ModelUpdateRequest,
+ ProviderInfo,
TestConnectionResult,
)
@@
-@router.get("/settings/providers")
-async def list_providers():
+@router.get("/settings/providers", response_model=list[ProviderInfo])
+async def list_providers() -> list[ProviderInfo]:
@@
- payload: list[dict] = []
+ payload: list[ProviderInfo] = []
@@
- payload.append(
- {
- "id": provider,
- "name": info.docs_label,
- "env": [info.env_var_name] if info.env_var_name else [],
- "models": models,
- }
- )
+ payload.append(
+ ProviderInfo(
+ id=provider,
+ name=info.docs_label,
+ env=[info.env_var_name] if info.env_var_name else [],
+ models=models,
+ )
+ )As per coding guidelines: **/*.py: Use Pydantic models for data structures and API schemas in the backend.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/cliff/api/routes/settings.py` around lines 136 - 164, Update the
list_providers endpoint to return typed Pydantic models: add
response_model=list[ProviderInfo] to the `@router.get` decorator and change the
payload construction to build and return ProviderInfo instances (not raw dicts).
Inside list_providers (which iterates catalog.all_providers(),
catalog.get(provider), and catalog.picker_options(provider)), convert each
provider entry to ProviderInfo(id=..., name=..., env=[...] or [], models=...)
where models is a mapping from bare model id to the appropriate model schema
expected by ProviderInfo; ensure you import ProviderInfo and any nested model
types and update the payload typing to list[ProviderInfo] before returning it.
There was a problem hiding this comment.
Commit 8b29957 addressed this comment by importing ProviderInfo, adding response_model=list[ProviderInfo], typing the payload as list[ProviderInfo], and populating it with ProviderInfo instances so the endpoint returns typed Pydantic models.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
Comment on lines
170
to
182
| # 1. Write context section (auto-regenerates CONTEXT.md) | ||
| self._dir_manager.write_context_section(workspace_id, section, structured_output) | ||
|
|
||
| # 2. Re-render agent templates with full updated context | ||
| ws_dir = self._dir_manager.get(workspace_id) | ||
| if ws_dir is None: | ||
| raise FileNotFoundError(f"Workspace directory not found: {workspace_id}") | ||
|
|
||
| finding_data = json.loads(ws_dir.finding_json.read_text()) | ||
| all_context = self._dir_manager.read_all_context(workspace_id) | ||
| self._template_engine.write_agents( | ||
| ws_dir.agents_dir, finding=finding_data, **all_context | ||
| ) | ||
|
|
||
| # 3. Log to agent-runs.jsonl | ||
| # 2. Log to agent-runs.jsonl | ||
| run_log = AgentRunLog(ws_dir.agent_runs_log) | ||
| run_log.append(agent_type=agent_type, status="completed", summary=summary) | ||
|
|
||
| # 4. Bump version in DB | ||
| # 3. Bump version in DB | ||
| new_version = await increment_context_version(db, workspace_id) |
There was a problem hiding this comment.
Persist agent output into SidebarState in this write path.
update_context() now only writes the filesystem section, appends agent-runs.jsonl, and bumps context_version. That drops the SidebarState mirror, so sidebar/Issues-page flows can read stale data even though the workspace files were updated. The structured output needs to be persisted to the sidebar store here as part of the same agent-result path.
As per coding guidelines, "Every agent result must persist into both the chat timeline AND the SidebarState. Never only save to chat." Also, "Every user-facing action follows the interaction grammar: ask -> run -> summarize -> persist -> decide next."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/cliff/workspace/context_builder.py` around lines 170 - 182, The
update_context path currently writes the filesystem section
(self._dir_manager.write_context_section), appends to agent-runs.jsonl
(AgentRunLog.append) and bumps the DB version (increment_context_version) but
does not persist the structured_output into the SidebarState mirror; add a call
after run_log.append and before increment_context_version to persist the same
structured_output (and section/workspace_id) into the sidebar store so sidebar
reads are fresh — e.g., invoke your SidebarState/SidebarStore persistence method
(via the existing sidebar manager instance, e.g.
self._sidebar_state_manager.persist or similar) with workspace_id, section, and
structured_output to mirror the agent result into SidebarState.
Collaborator
Author
There was a problem hiding this comment.
update_context() never wrote SidebarState — and this PR didn't change that. The dual-persist (context + sidebar) happens in the executor: AgentExecutor.execute() calls map_and_upsert(...) (backend/cliff/agents/executor.py:1015) to mirror the structured output into SidebarState, then calls context_builder.update_context() for the filesystem context + version bump. The only thing this PR removed from update_context was the dead template_engine.write_agents() re-render. So the dual-persist invariant is intact; no change needed here.
There was a problem hiding this comment.
@galanko, you're right — I misread the responsibility boundary. update_context() is only a filesystem + version-bump helper; the dual-persist (sidebar mirror via map_and_upsert + filesystem via context_builder.update_context()) is orchestrated one level up in AgentExecutor.execute(). Removing template_engine.write_agents() didn't touch that invariant at all. Apologies for the noise.
✏️ Learnings added
Learnt from: galanko
Repo: cliff-security/cliff PR: 254
File: backend/cliff/workspace/context_builder.py:170-182
Timestamp: 2026-06-02T21:40:07.231Z
Learning: In cliff-security/cliff, the dual-persist invariant (agent result → SidebarState + filesystem context) is orchestrated at the `AgentExecutor.execute()` level: it calls `map_and_upsert(...)` (around backend/cliff/agents/executor.py:1015) to mirror structured output into SidebarState, and then calls `context_builder.update_context()` for the filesystem section write + context_version DB bump. `WorkspaceContextBuilder.update_context()` is only responsible for filesystem I/O and version tracking — it never has been responsible for SidebarState persistence. Do not flag `update_context()` for missing SidebarState writes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: galanko
Repo: cliff-security/cliff PR: 251
File: backend/cliff/api/routes/agent_execution.py:518-518
Timestamp: 2026-06-01T14:31:02.717Z
Learning: In this repo, Ruff is configured without the `ANN` ruleset (it only selects `E,F,I,N,UP,B,SIM,TCH`). During review, do not flag missing type annotations when they occur as part of the existing convention: (1) FastAPI/DI route handler parameters representing a database dependency (e.g., `db=Depends(get_db)`), and (2) test helpers/fixtures. Treat adding `ANN`-style annotations for these elements as out of scope for individual feature PRs; only flag missing/incorrect typing where the repo already enforces it via other rules or where the code explicitly violates existing typing conventions.
Comment on lines
23
to
28
| def pytest_collection_modifyitems(items): | ||
| """Mark all items in this directory as agent tests and apply skip conditions.""" | ||
| """Mark all items in this directory as agent tests and skip when no key.""" | ||
| for item in items: | ||
| if "/agents/" in str(item.fspath): | ||
| item.add_marker(pytest.mark.agent) | ||
| item.add_marker(_skip_no_binary) | ||
| item.add_marker(_skip_no_key) |
There was a problem hiding this comment.
Don't skip the entire agent test directory when no API key is present.
This hook adds _skip_no_key to every test under backend/tests/agents, even though the module docstring says most of these tests run against FunctionModel/TestModel and need no network. In keyless CI/dev runs, that masks regressions in the in-process agent runtime instead of only skipping the live eval(s) that actually call a real LLM. Gate the skip behind an explicit live-LLM marker or the specific eval test file instead of the whole directory. Based on learnings: "Every phase must have tests passing before it is considered complete. Backend agent tests should use FunctionModel/TestModel."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/agents/conftest.py` around lines 23 - 28, The
pytest_collection_modifyitems hook currently adds _skip_no_key to every test
under backend/tests/agents and should instead only skip tests that require a
real LLM; update pytest_collection_modifyitems so it still marks items in the
agents directory with pytest.mark.agent but only adds _skip_no_key when the test
item is explicitly marked for live LLMs (e.g., has marker "live_llm" or "live")
or matches the dedicated live-eval test filename pattern (e.g., contains "eval_"
in str(item.fspath)), leaving FunctionModel/TestModel unit tests unskipped;
modify the logic around pytest_collection_modifyitems and the use of
_skip_no_key to check item.get_closest_marker("live_llm") (or inspect the
filename) before calling item.add_marker(_skip_no_key).
There was a problem hiding this comment.
Commit 8b29957 addressed this comment by limiting _skip_no_key to the specific live-LLM files (e.g., the known live evals) while still marking all agent tests, ensuring FunctionModel/TestModel tests remain runnable without a key.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
Comment on lines
24
to
34
| @pytest.fixture | ||
| async def real_builder(db_client, tmp_path): | ||
| """Swap the conftest mock for a REAL WorkspaceContextBuilder rooted at | ||
| ``tmp_path`` so ``create_workspace`` actually persists ``repo_url`` and | ||
| writes agent templates we can grep. | ||
| ``tmp_path`` so ``create_workspace`` actually persists ``repo_url``. | ||
| """ | ||
| from cliff.main import app | ||
|
|
||
| dir_manager = WorkspaceDirManager(base_dir=tmp_path) | ||
| template_engine = AgentTemplateEngine() | ||
| real = WorkspaceContextBuilder(dir_manager, template_engine, mcp_resolver=None) | ||
| real = WorkspaceContextBuilder(dir_manager, mcp_resolver=None) | ||
| app.state.context_builder = real | ||
| yield real |
There was a problem hiding this comment.
Restore the original app.state.context_builder after the fixture.
This fixture mutates the singleton FastAPI app and never resets it, so later tests can inherit a builder rooted at a previous tmp_path and become order-dependent.
💡 Suggested fix
`@pytest.fixture`
async def real_builder(db_client, tmp_path):
"""Swap the conftest mock for a REAL WorkspaceContextBuilder rooted at
``tmp_path`` so ``create_workspace`` actually persists ``repo_url``.
"""
from cliff.main import app
dir_manager = WorkspaceDirManager(base_dir=tmp_path)
real = WorkspaceContextBuilder(dir_manager, mcp_resolver=None)
- app.state.context_builder = real
- yield real
+ previous = app.state.context_builder
+ app.state.context_builder = real
+ try:
+ yield real
+ finally:
+ app.state.context_builder = previous🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/integration/test_workspace_repo_url_overrides_global.py` around
lines 24 - 34, The fixture real_builder currently replaces
app.state.context_builder and never restores it; capture the original builder
(orig = app.state.context_builder) before assigning the new
WorkspaceContextBuilder (created with WorkspaceDirManager and
mcp_resolver=None), yield the new builder as now, and after the yield restore
app.state.context_builder = orig so tests don’t leak the mutated singleton;
update the fixture body to perform this save/restore around the yield while
keeping the WorkspaceContextBuilder and WorkspaceDirManager usage intact.
There was a problem hiding this comment.
Commit 8b29957 addressed this comment by saving the prior app.state.context_builder, yielding the temporary builder, and restoring the original builder in a finally block so the singleton no longer leaks between tests.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
Comment on lines
34
to
+46
| ws_dir = manager.base_dir / workspace_id | ||
| assert ws_dir.is_dir() | ||
| assert (ws_dir / ".opencode" / "agents").is_dir() | ||
| assert (ws_dir / "history").is_dir() | ||
| assert (ws_dir / "opencode.json").is_file() | ||
|
|
||
| # No finding-scoped files on a repo workspace. | ||
| assert not (ws_dir / "finding.json").exists() | ||
| assert not (ws_dir / "finding.md").exists() | ||
| assert not (ws_dir / "CONTEXT.md").exists() | ||
|
|
||
| # No OpenCode scaffolding — the PA repo-action agent carries its own | ||
| # prompt + permission policy (ADR-0047); nothing is rendered to disk. | ||
| assert not (ws_dir / ".opencode").exists() | ||
| assert not (ws_dir / "opencode.json").exists() |
There was a problem hiding this comment.
Assert history/agent-runs.jsonl exists too.
create_repo_workspace() now explicitly touches history/agent-runs.jsonl so repo and finding workspaces share the same log contract. This test only checks the directory, so a regression dropping the file would go unnoticed.
💡 Suggested assertion
ws_dir = manager.base_dir / workspace_id
assert ws_dir.is_dir()
assert (ws_dir / "history").is_dir()
+ assert (ws_dir / "history" / "agent-runs.jsonl").is_file()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_workspace_repo_creation.py` around lines 34 - 46, The test
for create_repo_workspace() verifies the workspace directory and history folder
but misses asserting the presence of the history log file; update the test
around ws_dir (derived from manager.base_dir / workspace_id) to also assert that
(ws_dir / "history" / "agent-runs.jsonl").exists() so regressions that drop the
touched file are caught; locate the assertions block that checks ws_dir and add
this single assertion referencing agent-runs.jsonl.
There was a problem hiding this comment.
Commit 8b29957 addressed this comment by adding an assertion that history/agent-runs.jsonl exists, ensuring the test now verifies the file presence as requested.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| | Deployment | Single Docker container, port 8000 | `docker/` | | ||
|
|
||
| See `cliff-os/docs/architecture/overview.md` (private) for the full system diagram and `cliff-os/docs/adr/0014-workspace-runtime-architecture.md` (private) for the workspace isolation architecture. | ||
| See `cliff-os/docs/architecture/overview.md` (private) for the full system diagram, `cliff-os/docs/adr/0047-pydantic-ai-substrate.md` (private) for the substrate, and `cliff-os/docs/adr/0014-workspace-runtime-architecture.md` (private) for the workspace isolation architecture. |
There was a problem hiding this comment.
Update the private cliff-os ADR/IMPL docs in this change.
This file now documents the new in-process substrate and canonical AI-state flow, but the PR notes say the matching cliff-os docs are deferred. For a substrate/runtime rewrite of this size, leaving the private architecture source of truth stale should block merge.
As per coding guidelines, "All architectural changes must be documented via Architecture Decision Records (ADRs) in the private cliff-os/docs/adr/ directory. When behavior documented in an ADR or IMPL plan changes, update that documentation in the same change."
Also applies to: 210-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` at line 35, Update the private cliff-os ADR/IMPL documents to
match the new in-process substrate and canonical AI-state flow described in this
PR: modify ADR 0047-pydantic-ai-substrate.md and ADR
0014-workspace-runtime-architecture.md (and any related IMPL plans) to reflect
the substrate/runtime rewrite and workspace isolation changes, include the new
state flow diagrams and behavioral details, and add a brief changelog entry so
the public CLAUDE.md statements are consistent with the private source-of-truth
before merging.
Collaborator
Author
There was a problem hiding this comment.
Agreed the private cliff-os ADR/IMPL-0022 docs need the same update — but that's the architect's surface in the private umbrella repo, not this public code PR (per the repo workflow, cliff-os docs are updated via an architect pass, and this PR is build-mode). It's tracked as a follow-up alongside the cliff-os IMPL-0022 status update. The public CLAUDE.md in this PR is the user-facing source and is consistent with the new substrate.
There was a problem hiding this comment.
@galanko, that makes sense — the build/architect role separation is clear, and tracking it against IMPL-0022 is the right place for it. I'll defer to that workflow. No further action needed on this comment.
Comment on lines
+14
to
15
| # scripts/ install-scanners.sh | ||
| # .scanner-versions |
There was a problem hiding this comment.
Remove the stale OpenCode install text from the shipped README.
Line 116 still says the installer lays down opencode binaries even though this tarball now ships only install-scanners.sh. That makes the generated README-LOCAL-INSTALL.md false for every archive built from this script.
Suggested cleanup
-# ---- scripts (just the two install helpers — install-local.sh isn't needed
+# ---- scripts (just the scanner install helper — install-local.sh isn't needed
# at runtime, it's how the user got here in the first place) -----------------
@@
-sets up a uv-managed Python venv, installs the opencode/trivy/semgrep
-binaries via the bundled scripts, and drops `cliffsec` into ~/.local/bin/.
+sets up a uv-managed Python venv, installs the trivy/semgrep
+binaries via the bundled script, and drops `cliffsec` into ~/.local/bin/.Also applies to: 96-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build-tarball.sh` around lines 14 - 15, The generated
README-LOCAL-INSTALL.md contains stale text claiming the installer lays down
`opencode` binaries; update the README generation in scripts/build-tarball.sh to
remove any references to `opencode` and change the installer description to
reference only the shipped `install-scanners.sh` (and any shipped files like
`scripts/` or `.scanner-versions`) so the produced README accurately reflects
the tarball contents; locate the README template/emission code in
build-tarball.sh (search for README-LOCAL-INSTALL.md, `opencode`, and
`install-scanners.sh`) and edit the template text to remove the outdated
installer wording and replace it with a concise statement that the tarball
includes `install-scanners.sh` for installation.
There was a problem hiding this comment.
Commit 8b29957 addressed this comment by updating the build script’s README template to describe only the scanner installer and by removing the outdated reference to installing the opencode binaries, ensuring the generated README now matches the shipped tarball contents.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
- settings.py: GET /settings/providers gets response_model=list[ProviderInfo] and returns typed instances — restores FastAPI validation + OpenAPI schema for a user-facing endpoint (CodeRabbit). - tests/agents/conftest.py: skip-no-key now applies ONLY to the two live-LLM files (test_normalizer_agent, test_plain_description_eval); the FunctionModel/ TestModel runtime tests run in keyless CI so a regression isn't masked (CodeRabbit). Verified: keyless run → FunctionModel tests run, live skip. - test_workspace_repo_url_overrides_global.py: save/restore app.state.context_builder so the fixture can't leak a tmp_path-rooted builder into later tests (CodeRabbit). - test_workspace_repo_creation.py: also assert history/agent-runs.jsonl exists (CodeRabbit). - build-tarball.sh: drop the stale "opencode binaries" text from the generated README-LOCAL-INSTALL.md (CodeRabbit). - test_ai_catalog.py: docstring no longer references "spawning OpenCode". Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The agent substrate runs in-process via Pydantic AI (ADR-0047), so the CLI no longer manages any child agent process or its ports: - process_sweep.py: drop the ``opencode`` process kind, the OPENCODE_SINGLETON_PORT (4096) + WORKSPACE_PORT_RANGE (4100-4199) constants, and the bin/opencode classification arm. find_cliff_processes() is uvicorn-only and no longer needs cliff_home. - daemon.py: _cliff_ports() + the doctor port loop only consider the app port now; stop/uninstall stop scanning the dead 4096/4100-4199 range; layout docstring drops .opencode-version + bin/opencode. - cli.py: drop the now-unreachable ``opencode_engine_unavailable`` status blocker (/health.opencode is hard-wired "ok"). - updater.py docstring no longer claims install-opencode.sh. Tests updated: test_process_sweep (uvicorn-only), test_daemon (app-port sweep, opencode→uvicorn fixtures), test_cli (unconfigured blockers). CLI suite 94 passed, ruff clean. Zero OpenCode references remain in cli/cliff_cli. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CodeRabbit: lock the migration contract — the doctor payload must no longer emit an ``opencode`` check (the binary version check was removed with the substrate). Adds the explicit negative assertions alongside the subset check. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment on lines
116
to
119
| has_uvicorn = any("uvicorn" in arg for arg in cmdline) | ||
| has_app = any("cliff.main:app" in arg for arg in cmdline) | ||
| if has_uvicorn and has_app: | ||
| return "uvicorn" |
There was a problem hiding this comment.
_classify() only recognizes uvicorn + cliff.main:app, so find_cliff_processes() misses legacy opencode children and stop_cmd()/uninstall_cmd() can think Cliff is gone while it still runs — should we keep the opencode match or add a migration cleanup path for old installs?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
cli/cliff_cli/process_sweep.py around lines 106-121 in `_classify()`, you removed the
legacy OpenCode `opencode` ownership matching and now only return "uvicorn" when the
cmdline contains both `uvicorn` and `cliff.main:app`. Update `_classify()` to also
recognize the legacy `opencode` child processes from pre-ADR-0047 installs using the
same safety contract (match a distinctive cmdline token or the resolved
`cliff_home/bin/opencode` path), and return kind="opencode" for those processes. Then in
`find_cliff_processes()` around lines 137-167, ensure the function computes whatever is
needed (e.g., pass `cliff_home`/`opencode_bin` or otherwise derive the legacy match
input) and that the downstream callers like `stop_cmd()`/`uninstall_cmd()` can see and
act on `FoundProcess(kind="opencode")` for legacy cleanup during in-place
upgrade/uninstall.
Collaborator
Author
There was a problem hiding this comment.
Intentional — this is a clean break, not a regression. The OpenCode binary is no longer installed or spawned post-ADR-0047, so there are no opencode children for a healthy install to reap. The only way to have a leftover orphan is to force-kill the parent of a pre-upgrade install and then upgrade the CLI without stopping first — a rare one-time edge where the orphan sits idle on a port the new app never binds (no functional impact; pkill opencode clears it). Re-adding permanent opencode awareness to carry that one-time edge forever runs against the whole point of the migration, so I'm leaving the sweep uvicorn-only.
There was a problem hiding this comment.
Got it — thanks for the context. I’ll save this to memory once the PR is merged.
Comment on lines
+548
to
552
| # Port — only the app port matters now (the substrate is in-process). | ||
| for port in (DEFAULT_PORT,): | ||
| free = _port_free(port) | ||
| warn_only = port != DEFAULT_PORT | ||
| checks.append( |
There was a problem hiding this comment.
doctor only probes DEFAULT_PORT here, but _configured_app_port() is the bind-port source of truth — should we iterate over that instead so non-default CLIFF_APP_PORT values don't miss port-in-use failures?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
cli/cliff_cli/daemon.py around lines 548-554 inside _gather_doctor_checks(), update the
“# Port — only the app port matters now” logic to probe the real configured app
port instead of hardcoding DEFAULT_PORT. Refactor the loop to iterate over
_cliff_ports() (or directly over _configured_app_port()) so doctor reports in-use
correctly when CLIFF_APP_PORT overrides the default. Preserve warn_only behavior if
needed, but ensure the checked port set matches what start_cmd/stop_cmd/_cliff_ports()
actually bind.
Collaborator
Author
There was a problem hiding this comment.
Good catch — fixed in 6c-latest: the doctor port loop now iterates _cliff_ports() (the configured CLIFF_APP_PORT) instead of the hard-coded DEFAULT_PORT, so a non-default deployment no longer misses a port-in-use failure. Added a regression test (test_doctor_probes_configured_app_port).
There was a problem hiding this comment.
Commit 398bdf8 addressed this comment by refactoring the doctor port loop to iterate over _cliff_ports() so the configured CLIFF_APP_PORT is probed, and by adding test_doctor_probes_configured_app_port to confirm non-default ports are checked instead of the hard-coded default.
Adding response_model=list[ProviderInfo] to GET /settings/providers (PR review fix) changes the schema's response component. Re-snapshot to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Baz: the doctor port check hard-coded DEFAULT_PORT, so a deployment with a non-default CLIFF_APP_PORT would miss a real port-in-use failure (and report 8000 free while 9123 is wedged). Iterate over _cliff_ports() (the configured app port) instead. Adds a regression test. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
After the e2e verification passed, clean the user-facing and operational references the substrate removal left behind: - README: installer no longer fetches an OpenCode binary; bundled subprocesses are now just Trivy + Semgrep (PA is a Python dep). - NOTICE + THIRD-PARTY-LICENSES.md: drop the OpenCode (MIT) bundled-binary entry — we no longer download or redistribute it. - .dockerignore / .gitignore / CONTRIBUTING.md / scripts/dev.sh / scripts/install-local.sh: drop the .opencode-version + install-opencode.sh references. - docker/Dockerfile + entrypoint.sh: drop the "OpenCode Go binary" stage comment and the CLIFF_OPENCODE_PORT/BIN runtime banner lines. - KNOWN_ISSUES.md: remove the 9 now-resolved OpenCode-era issues (SSE/chat sessions, opencode.json drift, keys-lost-on-restart, 75+-provider catalog) and reword the orphan-PR issue's failure modes. - plugins/cliff-security knowledge: install/doctor no longer mention an opencode binary or the retired 4096/4100-4102 port checks. - frontend: PostureCard no longer shows "spinning up OpenCode…"; regenerate api/types.ts from the current OpenAPI snapshot (drops the deleted session/chat endpoints); reword the AIProviderStatus / ModelPicker comments + a test fixture. Left intact: CHANGELOG.md + ROADMAP.md completed-phase milestones (accurate history — Phase 1 genuinely was an OpenCode spike), the CLAUDE.md historical -names note, and the intentional ``opencode``/``opencode_version`` HealthStatus wire-compat fields. Verified: frontend tsc + lint + build clean. (The pre-existing frontend vitest failures live on #253 too and are uncaught by CI, which runs lint+build only — out of scope for this PR.) Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
galanko
added a commit
that referenced
this pull request
Jun 3, 2026
… substrate removal (PR #3b–#3d) (#253) * feat(agents): migrate the finding normalizer to a Pydantic AI app-level agent IMPL-0022 PR #3b — the second OpenCode consumer off the substrate. The finding normalizer is a single LLM call (raw scanner JSON → normalized findings); it now runs in-process through Pydantic AI instead of the singleton OpenCode process. - add `cliff/agents/runtime/normalizer_agent.py` — `NormalizedFinding` (a deliberately lenient output schema) + `build_normalizer_agent`, with `output_type=list[NormalizedFinding]`, no deps/tools. App-level, so the prompt is passed in by the caller. - rewrite `integrations/normalizer.py`: `normalize_findings(source, raw, *, env, model)` builds the model via the runtime provider factory and calls `agent.run()`. Pydantic AI's structured output + internal retries replace the hand-rolled `_call_llm_with_retry` / `_extract_json_array` / regex cleanup and the `opencode_client` model-override dance. The per-item `FindingCreate` validation + coercion is unchanged, so the partial-success `(valid, errors)` contract is preserved. - `ingest_worker.py`: inject `env_resolver` / `model_resolver` (mirroring the executor's pattern); resolve the provider env per job and fall back to the canonical active model when a job didn't pin one. - `main.py`: wire the worker with the app-level AI resolvers. Tests: - rewrite `test_normalizer.py` against a `FunctionModel` (success, partial failure, source_type injection, null-status default, list-wrapped raw_payload coercion, LLM-error → error string, model-not-configured). - update the `_process_job` call sites + mock signatures for the new resolver params; pass `env`/`model` in the real-LLM eval tests and pick a capable eval model (a nano-tier default under-extracts on batches). The singleton OpenCode process stays (repo-action agents still use the pool; #3c migrates them, #3d deletes the substrate). Full unit suite: 1552 passed; real-LLM normalizer evals pass with a capable model. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(agents): migrate repo-action agents to Pydantic AI (#3c) IMPL-0022 PR #3c — the last per-workspace-pool consumer. The two posture generators (security_md_generator, dependabot_config_generator, ADR-0024) clone → write → commit → push → open a draft PR; they now run in-process through Pydantic AI, reusing the executor's bash/edit/read/gh tools, instead of a per-workspace OpenCode process driven by a Jinja template. - add cliff/agents/runtime/repo_actions.py — RepoActionOutput + build_repo_action_agent(model, kind). Both system prompts are adapted for the PA tool API: self-contained `git -C repo` commands (bash does not persist cwd), the `edit` tool for file writes, and `output_type` in place of the JSON-code-block contract. - WorkspaceDeps.auto_approve + gate_tool_call honour it: repo-action runs pre-approve the `ask` tier (no HITL surface for a one-shot background run); the `deny` tier still hard-denies catastrophic commands. - rewrite repo_workspace_runner.RepoAgentRunner: pool.get_or_start() → agent.run(); drop the SSE collect/stall machinery, template render, and output_parser usage; keep the B16 PR-URL verification. - rewire the _engine_dep spawner: pool → app-level AI env/model resolvers. Also fixes a latent bug in the merged PR #2 executor: the `bash` tool passed `env=env_vars`, which REPLACED the process environment (env_vars carries only GH_TOKEN/CLIFF_REPO_URL) and would strip PATH/HOME so git/gh couldn't run. Now merges over os.environ. The executor's live clone→PR smoke was deferred, so this had never surfaced. Tests: runner (FunctionModel + the B16 verification branches), spawner (autouse no-op for the now-always-launched background run), and auto_approve gating. The deterministic suite stays green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(#3d): remove the vestigial workspace-scoped chat/sessions First slice of the substrate removal. The workspace-scoped chat/sessions routes were the last per-workspace-pool consumer, but they're dead weight post-migration: the PA pipeline runs agents in-process (it never used the OpenCode session), the frontend client defined the calls but no component invoked them, and `cliffsec fix` created a session as a leftover no-op. - delete the 4 routes from api/routes/workspaces.py (sessions, chat/send, chat/stream, chat/permission) + the pool-status debug route + _get_pool; drop the pool-stop from delete_workspace - drop the no-op `POST /sessions` step from `cliffsec fix` - remove createWorkspaceSession / sendWorkspaceMessage / streamWorkspaceEvents / respondToChatPermission from the frontend client; tidy the stale IssueSidePanel comment Nothing else drives the pool now — the singleton + pool come out next, once the remaining singleton consumers (health, settings, ai/service, lifespan) are rewired. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(#3d): rewire /health off the OpenCode singleton Move HealthStatus into cliff/models (it lived in engine/models, deleted next) and drop the live OpenCode probe. The substrate runs in-process via Pydantic AI, so the response shape is preserved for the frontend health card + cliffsec status: `opencode` is always "ok" and `opencode_version` now carries the PA version string ("pydantic-ai <ver>"). Model comes from the canonical ai_model_cache; ai_provider_ready is unchanged. - cliff/models: add HealthStatus (kept-shape, repurposed fields) - api/routes/health.py: drop opencode_process/opencode_client; report PA - tests: retarget test_models + test_routes_health; reduce the now-unused mock_opencode_process fixture to a no-op Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(repo-actions): gh has no -C flag — cd into the clone for `gh pr create` Code-review catch: the security.md + dependabot prompts told the agent to run `gh -C repo pr create`, but unlike git, the GitHub CLI has no directory flag, so the PR-create step would fail with "unknown flag: -C" and every repo-action run would end status=failed. Route the PR-create through bash with `cd repo && gh pr create ...` (the bash tool runs at the workspace root; gh reads the repo from the clone's remote and the token from $GH_TOKEN). Also regenerates the OpenAPI snapshot for the workspace-chat/sessions + pool-status routes removed earlier in this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: address PR #253 review — status coercion, no-provider job, clone auth Three confirmed review findings: - normalizer: force `status="new"` on every item instead of only filling null. A stray model value (e.g. "open") no longer drops an otherwise-valid finding into `errors` — normalized findings are always brand-new. - ingest_worker: short-circuit a job to terminal `failed` when no AI provider is configured (empty env / null model), instead of looping fail → pending → re-poll every ~1s forever (a new dependency the PA migration introduced; build_model can't run without a provider). - repo-action prompts: restore the conditional clone URL — embed the token only when $GH_TOKEN is set, else clone the plain URL (matches the old Jinja behaviour; a token-less private clone then fails cleanly). Tests: stray-status coercion; no-provider → failed (not retry). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(cli): drop the POST /sessions mock from `cliffsec fix` tests The fix command no longer pre-creates a workspace session (the PA pipeline runs agents in-process), so the three `fix` tests registered a `POST /workspaces/ws-1/sessions` httpx mock that's never requested — pytest_httpx fails the run on an unused mock. Remove the stale registrations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(#3d): delete the OpenCode substrate — Pydantic AI is the only agent runtime (#254) * refactor(settings): derive model + providers from canonical AI state Drop config_manager / opencode_client from the settings routes — the last contract-bearing OpenCode dependency outside the engine package: - GET/PUT /settings/model now route only through AIIntegrationService (canonical app_setting(model)); no opencode.json fallback. Empty ModelConfig when no provider is connected; PUT 503s without a vault and 400s with no active provider / prefix mismatch. - GET /settings/providers is rebuilt from cliff.ai.catalog (static per-provider picker rows) instead of OpenCode's models.dev dump. Wire shape {id,name,env,models} is preserved; models keyed by the bare id so the UI/CLI rebuild f"{provider}/{model_id}" unchanged. - POST /settings/providers/test now runs a bounded "Say OK" through Pydantic AI (build_model + Agent) and classifies the outcome. - Delete the vestigial config_manager-only routes /settings/providers/ configured and /settings/api-keys/* (no live UI consumer) plus their dead frontend wrappers/hooks/types and the MSW api-keys handler. CLI (cliffsec model get/set/list) is unaffected — same wire shapes. Part of PR #3d (OpenCode substrate removal), stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(ai): drop the OpenCode auth.json dual-path ADR-0047: the PA model factory reads the provider key straight from the resolved env (resolve_env_for_workspace) — there is no auth.json to keep in sync. Remove sync_to_opencode, _sync_opencode_auth, _clear_opencode_auth, _safe_write_opencode_config, the _OPENCODE_AUTH_PROVIDERS set, and all their call sites in save/disconnect/set_model. Canonical vault + app_setting(model) state is untouched; _fire_key_change stays as a generic env-refresh hook. Drop the now-obsolete auth.json-push tests and the dead opencode_client stub fixtures. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(main): drop the singleton OpenCode lifespan + dead process pool ADR-0047: agents run in-process via Pydantic AI, so nothing spawns or talks to an OpenCode process anymore. Remove from the lifespan: - the singleton opencode_process start/stop + set_extra_env seeding - the auth.json sync_to_opencode reconcile + config_manager reconcile_model / restore_keys_to_engine startup calls - the WorkspaceProcessPool (constructed but never read after #3c — the executor never called it), its app.state handle, the idle-cleanup loop, and pool.stop_all on shutdown - the singleton-restart half of the ai_on_key_change hook (it now only refreshes the warm env/model cache) - opencode_client.close() / opencode_process.stop() teardown AgentExecutor drops its unused ``pool`` parameter. Test construction sites updated to the new signature. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(workspace): stop scaffolding opencode.json + .j2 agent templates ADR-0047: PA agents run in-process from prompts defined in cliff.agents.runtime — they never read a workspace's opencode.json or rendered .opencode/agents/*.md. Stop writing both: - WorkspaceDirManager.create() drops the .opencode/agents tree, the opencode.json write, and its mcp_servers/model params. The dir still carries the finding context (finding.json, finding.md, CONTEXT.md, context sections) the agents read. - create_repo_workspace() becomes a thin clone-dir allocator (history/ + REPO_ACTION.md); no rendered prompt, no opencode.json. The repo-action agent carries its own prompt + permission policy. - WorkspaceContextBuilder drops the AgentTemplateEngine dependency and the per-create / per-update write_agents render passes; it still writes the workspace-integrations.json manifest (read by check_config_freshness). - Delete _build_opencode_config and the WorkspaceDir opencode_json / opencode_dir / agents_dir properties. - main.py lifespan + _engine_dep spawner drop the template engine / model plumbing. Tests updated to assert finding context (not templates); the obsolete opencode.json/template assertions and the OpenCode pool-wiring test (test_ai_phase_f_wiring) are removed. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(version): report the in-process substrate version, not OpenCode Move VersionInfo out of the doomed cliff.engine.models into cliff.models alongside HealthStatus, and add a shared substrate_version() helper. The /version handshake's compat ``opencode`` field and /health's ``opencode_version`` both now carry "pydantic-ai <ver>" (ADR-0047) instead of the pinned OpenCode binary version. /health stops falling back to settings.opencode_model (opencode.json is gone) — the model is the canonical ai_model_cache or empty. The config.py opencode_* settings removal rides with the engine deletion (they're read by engine/ at runtime until it's deleted). Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: delete the OpenCode substrate Every OpenCode caller is gone (#3b/#3c + steps 1-5), so remove the engine itself and everything that fed it: Backend - delete backend/cliff/engine/ (client, process, pool, config_manager, models) and backend/cliff/agents/template_engine.py + templates/*.j2 - config.py: drop the opencode_* connection/model/version settings and properties; repo-root sentinel moves from .opencode-version to VERSION - agents/__init__.py stops re-exporting the template engine - delete the engine/pool/template unit tests + the OpenCode e2e suite; strip the opencode fixtures from the conftests; retarget test_config / test_docker / the openrouter route test; regenerate the OpenAPI snapshot Packaging / CLI - Dockerfile: drop the OpenCode binary install step, the opencode.json / .opencode COPYs, and the CLIFF_OPENCODE_* env - build-tarball / install-local / install smoke / install-smoke.yml stop bundling + installing + asserting the opencode binary - CLI updater stops re-running install-opencode.sh; daemon doctor drops the opencode binary check (it would report "not found" post-migration) Root - delete .opencode-version, opencode.json, .opencode/, scripts/install-opencode.sh Follow-up (noted, not blocking): the CLI's process_sweep still classifies a ``$CLIFF_HOME/bin/opencode`` process kind and cli.py keeps the (now always "ok") opencode health field — dead but harmless; a separate cleanup PR. Backend 1432 pass, CLI 97 pass, frontend tsc clean, ruff clean. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: update CLAUDE.md for the Pydantic AI substrate Architecture table, repo layout, "How It Runs", the workspace-runtime layers, the AI-provider section, and the testing guidance now describe the in-process Pydantic AI substrate (ADR-0047) instead of the OpenCode subprocess / process pool / auth.json / drift model. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: scrub stale OpenCode comments from the live code Comment/docstring-only. The PA code was already correct; the prose still described OpenCode subprocesses, an auth.json/singleton restart, a process pool, and opencode.json MCP config that no longer exist. Reword the load-bearing-wrong comments to describe the in-process Pydantic AI behaviour (executor dispatch, errors, repo runner, tools, provider/catalog, ai/service + models, gateway), and drop already-happened "deleted in PR2.E" / "PR #3 finalizes" future-tense references. Kept: accurate "used to run on OpenCode / pre-migration" provenance that explains why code is shaped the way it is, the intentional ``opencode`` HealthStatus/VersionInfo wire-compat field, and the legacy_migration notes about legacy opencode-keyed app_settings. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: address PR #254 review comments - settings.py: GET /settings/providers gets response_model=list[ProviderInfo] and returns typed instances — restores FastAPI validation + OpenAPI schema for a user-facing endpoint (CodeRabbit). - tests/agents/conftest.py: skip-no-key now applies ONLY to the two live-LLM files (test_normalizer_agent, test_plain_description_eval); the FunctionModel/ TestModel runtime tests run in keyless CI so a regression isn't masked (CodeRabbit). Verified: keyless run → FunctionModel tests run, live skip. - test_workspace_repo_url_overrides_global.py: save/restore app.state.context_builder so the fixture can't leak a tmp_path-rooted builder into later tests (CodeRabbit). - test_workspace_repo_creation.py: also assert history/agent-runs.jsonl exists (CodeRabbit). - build-tarball.sh: drop the stale "opencode binaries" text from the generated README-LOCAL-INSTALL.md (CodeRabbit). - test_ai_catalog.py: docstring no longer references "spawning OpenCode". Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(cli): finish de-OpenCode'ing the CLI process management The agent substrate runs in-process via Pydantic AI (ADR-0047), so the CLI no longer manages any child agent process or its ports: - process_sweep.py: drop the ``opencode`` process kind, the OPENCODE_SINGLETON_PORT (4096) + WORKSPACE_PORT_RANGE (4100-4199) constants, and the bin/opencode classification arm. find_cliff_processes() is uvicorn-only and no longer needs cliff_home. - daemon.py: _cliff_ports() + the doctor port loop only consider the app port now; stop/uninstall stop scanning the dead 4096/4100-4199 range; layout docstring drops .opencode-version + bin/opencode. - cli.py: drop the now-unreachable ``opencode_engine_unavailable`` status blocker (/health.opencode is hard-wired "ok"). - updater.py docstring no longer claims install-opencode.sh. Tests updated: test_process_sweep (uvicorn-only), test_daemon (app-port sweep, opencode→uvicorn fixtures), test_cli (unconfigured blockers). CLI suite 94 passed, ruff clean. Zero OpenCode references remain in cli/cliff_cli. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(cli): assert the opencode doctor check is absent CodeRabbit: lock the migration contract — the doctor payload must no longer emit an ``opencode`` check (the binary version check was removed with the substrate). Adds the explicit negative assertions alongside the subset check. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: regenerate OpenAPI snapshot for typed /settings/providers Adding response_model=list[ProviderInfo] to GET /settings/providers (PR review fix) changes the schema's response component. Re-snapshot to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(cli): doctor probes the configured app port, not the default Baz: the doctor port check hard-coded DEFAULT_PORT, so a deployment with a non-default CLIFF_APP_PORT would miss a real port-in-use failure (and report 8000 free while 9123 is wedged). Iterate over _cliff_ports() (the configured app port) instead. Adds a regression test. Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: scrub remaining OpenCode references from README, license + docs After the e2e verification passed, clean the user-facing and operational references the substrate removal left behind: - README: installer no longer fetches an OpenCode binary; bundled subprocesses are now just Trivy + Semgrep (PA is a Python dep). - NOTICE + THIRD-PARTY-LICENSES.md: drop the OpenCode (MIT) bundled-binary entry — we no longer download or redistribute it. - .dockerignore / .gitignore / CONTRIBUTING.md / scripts/dev.sh / scripts/install-local.sh: drop the .opencode-version + install-opencode.sh references. - docker/Dockerfile + entrypoint.sh: drop the "OpenCode Go binary" stage comment and the CLIFF_OPENCODE_PORT/BIN runtime banner lines. - KNOWN_ISSUES.md: remove the 9 now-resolved OpenCode-era issues (SSE/chat sessions, opencode.json drift, keys-lost-on-restart, 75+-provider catalog) and reword the orphan-PR issue's failure modes. - plugins/cliff-security knowledge: install/doctor no longer mention an opencode binary or the retired 4096/4100-4102 port checks. - frontend: PostureCard no longer shows "spinning up OpenCode…"; regenerate api/types.ts from the current OpenAPI snapshot (drops the deleted session/chat endpoints); reword the AIProviderStatus / ModelPicker comments + a test fixture. Left intact: CHANGELOG.md + ROADMAP.md completed-phase milestones (accurate history — Phase 1 genuinely was an OpenCode spike), the CLAUDE.md historical -names note, and the intentional ``opencode``/``opencode_version`` HealthStatus wire-compat fields. Verified: frontend tsc + lint + build clean. (The pre-existing frontend vitest failures live on #253 too and are uncaught by CI, which runs lint+build only — out of scope for this PR.) Part of PR #3d, stacked on #253. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(pa): address pre-merge review findings on #253 Full code review of the OpenCode→Pydantic AI migration before merge to main, plus the open Baz/CodeRabbit threads. Real bugs + targeted cleanups. Correctness: - permissions: narrow `_auto_approved` so repo-action runs pre-approve ONLY the gated-bash ask bucket (rm, git reset, …). It no longer swallows the classifier's safe-default ask buckets (external_directory, edit-escape, mcp/unknown tools, empty bash) — those stay approval-gated so a confused repo-action fails closed instead of silently executing review-routed calls (CodeRabbit major). New lock-down tests cover the four buckets. - ingest_worker: reconcile a job's pinned model with the active provider's env (cross-provider pins now fall back to the active model instead of failing auth every chunk), validate the model is `<provider>/<model>` before processing (a bare override no longer loops fail→pending), and stop counting error-only chunks as completed (Baz high ×2). - executor: resume with a missing message-history now fails inside the try (marker already cleared) instead of raising before it — the run is marked failed rather than wedged at running/permission_pending forever. - provider: strip a trailing `/v1` from OLLAMA_BASE_URL before appending it, avoiding a doubled `/v1/v1` → 404 when an operator includes it. - normalizer: cap the model's output array at the input count so a hallucinated overflow can't inject more findings than were submitted. Cleanup: - Delete the dead duplicate bash classifier in executor.py (the live gate is permissions.classify_tool_request); its lock-down tests were asserting against the dead copy — drop them (coverage lives in test_permissions.py). - Delete orphaned catalog.provider_env_var_names() (subprocess-env scrubbing; no subprocess remains) + its test. - Extract the token-aware clone snippet shared by both repo-action prompts into `_clone_block(branch)` so the auth logic can't drift. - Hoist the duplicated eval env/model setup into tests/agents/eval_utils.py. - Fix bash.py docstring (deny raises ModelRetry, not ValueError) and remove stale OpenCode references in test docstrings. - Update the cliff-security onboarding knowledge doc off the removed /api/settings/api-keys* routes onto /api/integrations/ai/byok + /status. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(pa): second review pass — gateway env precedence + ProviderInfo contract From CodeRabbit's full re-scan of the PR: - gateway: use explicit-key precedence for the MCP env block. An empty `environment: {}` was falsy under `or`, so it silently fell back to the legacy `env` (and `_find_unresolved_placeholders` repeated the mistake). An explicit (even empty) `environment` now wins in both spots. - models: make `env` and `models` required on `ProviderInfo` — `GET /api/settings/providers` always emits both and the UI/CLI consume them, so generated clients must treat them as present, not optional. Regenerated the OpenAPI snapshot + frontend types. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Stacked on #253 (
feat/pa-migrate-normalizer). Final PR of the OpenCode → Pydantic AI migration (ADR-0047 / IMPL-0022): every OpenCode consumer is gone, so this removes the engine itself and everything that fed it.What this does (7 commits, each independently green)
refactor(settings)—/settings/model+/settings/providers+/providers/testnow derive from canonical AI state (cliff.ai.catalog+AIIntegrationService+ a PA "Say OK" probe) instead ofconfig_manager/opencode_client. Deleted the vestigial/providers/configured+/api-keys/*routes (no live UI consumer) and their dead frontend wrappers/hooks/types. CLIcliffsec model get/set/listwire shapes unchanged.refactor(ai)— dropped theauth.jsondual-path (sync_to_opencode,_sync/_clear_opencode_auth,_safe_write_opencode_config).refactor(main)— removed the singleton OpenCode lifespan, the spawned-but-unusedWorkspaceProcessPool(the executor never called it after #3c), the idle-cleanup loop, and the singleton-restart hook.AgentExecutordrops its unusedpoolarg.refactor(workspace)— workspace dirs stop scaffoldingopencode.json+.opencode/agents/*.md;create_repo_workspaceis now a thin clone-dir allocator. The dir still carries the finding context the PA agents read.refactor(version)—/version+/healthreport the in-process substrate version (pydantic-ai <ver>);VersionInfomoved tocliff.models.chore: delete the OpenCode substrate— deletesbackend/cliff/engine/,template_engine.py+templates/*.j2,config.pyopencode settings, the engine/pool/template tests + OpenCode e2e suite,.opencode-version/opencode.json/.opencode//scripts/install-opencode.sh, the Dockerfile binary step, and the install-tooling + CLI bits that bundled/installed/doctored the binary.docs— CLAUDE.md updated to the PA substrate.Verification
pytest -m 'not e2e': 1432 passed, 14 skipped (ruff clean)tsc --noEmitcleanDeliberately deferred (noted for follow-up, not blocking)
process_sweep.pystill classifies a$CLIFF_HOME/bin/opencodeprocess kind andcli.pykeeps the (now always"ok")opencodehealth field — dead but harmless (they only match a binary that no longer exists). A focused CLI-cleanup PR.frontend/src/api/types.tsresync (pre-existing ~2000-line drift) — its own PR.Not for merge yet — this is the next layer of the stack over #253.
🤖 Generated with Claude Code
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Replace the legacy OpenCode engine + template-driven agent renderer with an in-process Pydantic AI substrate (ADR-0047), rewire all workspace/context layers, provider settings, tooling, fixtures, docs, and install artifacts to the new architecture, and remove the obsolete engine, templates, OpenCode scaffolding, and CI/CLI references that depended on it.
Modified files (15)
Latest Contributors(1)
Modified files (17)
Latest Contributors(1)
Modified files (22)
Latest Contributors(1)
Modified files (49)
Latest Contributors(1)