feat(settings): PROOF9 defaults + workspace config tabs (#556)#589
Conversation
Surfaces the last CLI-only onboarding knobs in the web UI: which of the 9 gates are enabled by default and the workspace's root/branch/tech-stack detection. Closes #556 (Phase 5.1). Backend ------- * GET/PUT /api/v2/proof/config — persists enabled_gates + strictness to .codeframe/proof_config.json; validates gates against the Gate enum * GET/PUT /api/v2/workspaces/config — persists workspace_root, default_branch, auto_detect_tech_stack, tech_stack_override to .codeframe/workspace_config.json * core/proof/runner.py reads proof_config.json, filters obligations by enabled_gates, and in 'warn' strictness keeps overall_passed=True on gate failures (still records the failure evidence) Frontend -------- * New Proof9DefaultsTab and WorkspaceConfigTab components with the same save/discard pattern as the Agent tab * Settings page wires both into the existing tabs * proofConfigApi + workspaceConfigApi added to lib/api.ts * PROOF9_GATES constant + types Diverges from Traycer's plan in one place: workspace config uses the plural prefix /api/v2/workspaces/config (consistent with the existing workspaces router) instead of a new singular /api/v2/workspace router.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds per-workspace PROOF9 defaults and workspace configuration: typed schemas and constants, v2 GET/PUT endpoints with atomic JSON persistence, proof-runner integration to apply enabled_gates and strictness, SWR-backed React settings components and API clients, and tests covering backend and UI behavior. ChangesConfiguration Settings Feature
Sequence Diagram(s)sequenceDiagram
participant UI as Settings UI (Proof9/Workspace)
participant Frontend as web-ui `proofConfigApi`/`workspaceConfigApi`
participant Server as FastAPI `/api/v2/*` routers
participant FS as Workspace state dir (`.codeframe/*`)
participant Runner as core.proof.runner
UI->>Frontend: GET /config?workspace_path=...
Frontend->>Server: GET /api/v2/.../config
Server->>FS: read `.codeframe/*_config.json` (or return defaults)
FS-->>Server: file contents / not found
Server-->>Frontend: 200 ProofConfigResponse / WorkspaceConfigResponse
Frontend-->>UI: data (SWR)
UI->>Frontend: PUT /config (workspace_path, body)
Frontend->>Server: PUT /api/v2/.../config with JSON
Server->>FS: atomic write temp -> os.replace `.codeframe/*_config.json`
FS-->>Server: write complete
Server-->>Frontend: 200 saved config
Note over Runner,FS: Later run_proof reads config
Runner->>FS: read `.codeframe/proof_config.json`
FS-->>Runner: enabled_gates, strictness
Runner->>Runner: filter obligations by enabled_gates, execute gates
Runner->>Runner: compute overall_passed per strictness (warn may preserve True)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Review: feat(settings): PROOF9 defaults + workspace config tabs (#556)Clean, well-structured implementation. The layering (core → router → UI) follows project conventions, test coverage is solid, and the plan deviation (plural Issues1.
Move the constant to 2. Missing
# proof_v2.py – get_proof_config
except (json.JSONDecodeError, ValueError) as e:
...A permission-denied or mid-write corrupt read would still raise 3. Non-atomic file writes Both PUT endpoints write directly with import tempfile, os
tmp = path.with_suffix('.tmp')
tmp.write_text(json.dumps(payload, indent=2))
os.replace(tmp, path)4.
Minor5. Default gate order: alphabetical vs. semantic
6. Strictness description in <strong>strict</strong> fails proof runs on any open non-waived REQ.The actual behavior (from 7. assert data["enabled_gates"] == ["unit", "sec"]This implicitly relies on JSON preserving insertion order. It works in CPython ≥ 3.7+, but using 8. Duplicate
What looks good
The atomic-write (#3) and OSError (#2) items are the most important to fix before merge. The rest can be follow-ons if preferred. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/ui/routers/proof_v2.py (1)
417-433:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign cached
passedwith strictness-awareoverall_passed.Line 417 recomputes
passedfrom raw gate results, which ignores warn-mode semantics fromrun_proof()and can incorrectly report failure via cached run status.💡 Proposed fix
- passed = all( - satisfied - for gate_results in results.values() - for _, satisfied in gate_results - ) + persisted_run = get_run(workspace, run_id) + if persisted_run is not None: + passed = persisted_run.overall_passed + else: + # Fallback only if persistence unexpectedly failed + passed = all( + satisfied + for gate_results in results.values() + for _, satisfied in gate_results + )🤖 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 `@codeframe/ui/routers/proof_v2.py` around lines 417 - 433, The cached "passed" is being recomputed from raw gate results (variable passed) which ignores warn-mode semantics from run_proof(); instead use the strictness-aware overall_passed produced by run_proof() when building the RunProofResponse and when storing into _run_cache. Replace the recomputation of passed with overall_passed (and ensure RunProofResponse.success uses overall_passed) and store that value into _run_cache[(str(workspace.repo_path), run_id)]["passed"] so cached status matches run_proof() semantics.
🤖 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 `@web-ui/src/components/settings/Proof9DefaultsTab.tsx`:
- Around line 58-67: The component Proof9DefaultsTab treats absence of draft as
a load failure causing a transient "Failed to load" flash; change the error
condition so missing draft is not considered an error during hydration—keep the
loading branch as `if (isLoading && !draft)` but update the failure branch
(currently `if (error || !draft || !data)`) to `if (error || !data)` so only
real errors or missing server data show the failure message; locate and update
the conditional in Proof9DefaultsTab where isLoading, draft, error, and data are
checked.
---
Outside diff comments:
In `@codeframe/ui/routers/proof_v2.py`:
- Around line 417-433: The cached "passed" is being recomputed from raw gate
results (variable passed) which ignores warn-mode semantics from run_proof();
instead use the strictness-aware overall_passed produced by run_proof() when
building the RunProofResponse and when storing into _run_cache. Replace the
recomputation of passed with overall_passed (and ensure RunProofResponse.success
uses overall_passed) and store that value into
_run_cache[(str(workspace.repo_path), run_id)]["passed"] so cached status
matches run_proof() semantics.
🪄 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: CHILL
Plan: Pro
Run ID: 2c066910-0a37-4352-bb7e-9c9a5b4729af
📒 Files selected for processing (14)
codeframe/core/proof/runner.pycodeframe/ui/routers/proof_v2.pycodeframe/ui/routers/workspace_v2.pytests/core/test_proof_runner_config.pytests/ui/test_proof_config.pytests/ui/test_workspace_config.pyweb-ui/src/__tests__/components/settings/Proof9DefaultsTab.test.tsxweb-ui/src/__tests__/components/settings/WorkspaceConfigTab.test.tsxweb-ui/src/app/settings/page.tsxweb-ui/src/components/settings/Proof9DefaultsTab.tsxweb-ui/src/components/settings/WorkspaceConfigTab.tsxweb-ui/src/lib/api.tsweb-ui/src/lib/proof.tsweb-ui/src/types/index.ts
- Move proof_config.json filename + canonical 9-gate order into codeframe/core/proof/models.py and import from both runner and router - proof/workspace config PUT endpoints now write via temp-file + os.replace so a crash mid-write cannot leave a truncated file - Add OSError to the config GET exception handlers - Default proof config returns gates in canonical PROOF9 order (matches the frontend) instead of alphabetical - Tighten the strictness description in Proof9DefaultsTab - Document that workspace_root in WorkspaceConfig is display-only - Make TestGetProofConfig.test_returns_existing_config order-insensitive
|
Addressed claude review in f823f33:
All 58 backend + 30 frontend settings tests pass; |
Code Review — PR #589: PROOF9 Defaults + Workspace Config tabsOverall this is a well-structured addition. The core logic is clean, the test coverage is solid across all four layers (core runner, FastAPI routers, React components), and the separation between the config-file format and the Pydantic API contract is properly handled. Agreeing with CodeRabbit's two flagsBoth issues raised by coderabbitai are real and worth fixing before merge:
Additional observations
Frontend gate list needs a sync comment — Missing fallback test —
What's solid
The two CodeRabbit bugs (#1 especially) should be addressed before merge. The rest are minor polish items. |
High-priority (both reviewers): - proof_v2.py run endpoint: read the strictness-aware overall_passed from the persisted run record instead of recomputing 'passed' from raw gate results. The recomputation incorrectly reported failure for warn-mode runs via the cached run status. Regression test added. Frontend UX: - Proof9DefaultsTab + WorkspaceConfigTab: stop showing a transient "Failed to load" flash between data arrival and the useEffect that initializes the draft. Error guard now only fires on actual errors. DRY: - Extract _atomic_write_json into codeframe/ui/routers/_helpers.py and use it from both proof_v2.py and workspace_v2.py. Polish: - workspace_root marked display-only at the Pydantic field level so OpenAPI consumers see the intent. - web-ui/src/lib/proof.ts gained a SYNC comment pointing to codeframe/core/proof/models.py as the source of truth. - New test: GET endpoints fall back to defaults on corrupted JSON.
|
Round 2: addressed claude + CodeRabbit feedback in ae32da7. High priority (both reviewers flagged)
Frontend UX (claude #2)
DRY (claude observation)
Polish
All 107 backend tests + 30 frontend settings tests pass; ruff clean; build clean. |
Follow-up ReviewAll five substantive items from the prior review have been addressed in this iteration — nice work:
One open item from the previous reviewDuplicate One new minor observation
SummaryThis is in good shape. The atomic-write and OSError fixes address the two most important correctness issues from the prior review. The remaining items are low-priority cleanup. Ready to merge once CI is green. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@codeframe/ui/routers/_helpers.py`:
- Around line 8-15: The atomic_write_json function currently reuses a single tmp
name (path.suffix + ".tmp") which can cause concurrent writes to clobber each
other; change atomic_write_json to create a unique per-write temp file in the
same directory (e.g., use tempfile.NamedTemporaryFile or generate a suffix with
uuid4()/pid/timestamp) write the JSON to that unique temp file and then
os.replace the unique temp path to path, and ensure the temp file is created in
path.parent so os.replace stays atomic and any temp file is removed/left only on
failure.
In `@codeframe/ui/routers/proof_v2.py`:
- Around line 648-717: The router currently contains domain logic for defaults,
validation, file layout, and corrupted-file fallback (symbols: _VALID_GATES,
_proof_config_path, _default_proof_config,
UpdateProofConfigRequest._validate_gates, get_proof_config, update_proof_config,
atomic_write_json); move that logic into a core API (e.g., core.proof_config) by
implementing functions like default_proof_config(), validate_gates(gates) or
exceptions for invalid gates, load_proof_config(workspace) which reads the file,
returns defaults on missing/corrupt files and logs or surfaces errors, and
save_proof_config(workspace, payload) which does atomic write; then simplify the
router handlers to only call core.load_proof_config and core.save_proof_config
and map request/response models (keep ProofConfigResponse and
UpdateProofConfigRequest as DTOs) so all file/validation behavior lives in the
core module instead of codeframe/ui/routers/proof_v2.py.
In `@tests/ui/test_proof_config.py`:
- Around line 38-54: The tests currently bypass the router's workspace_path
query parameter by overriding get_v2_workspace to return the workspace object
directly; instead add a small request helper on the TestClient that always
includes workspace_path (derived from test_workspace, e.g. test_workspace.path
or test_workspace.workspace_path) and use that helper across the file.
Concretely: in the test_client fixture (which references get_v2_workspace,
proof_v2.router, TestClient and test_workspace) keep the dependency override if
needed but attach a helper like client.request_with_workspace(method, path, *,
params=None, **kwargs) that merges params with {'workspace_path':
<test_workspace path>} and calls client.request(method, path, params=merged,
**kwargs); replace direct client.get/post calls in this test module with that
helper so every request includes workspace_path.
🪄 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: CHILL
Plan: Pro
Run ID: 4c6d3674-2cb4-4908-a150-e27f306da678
📒 Files selected for processing (8)
codeframe/ui/routers/_helpers.pycodeframe/ui/routers/proof_v2.pycodeframe/ui/routers/workspace_v2.pytests/ui/test_proof_config.pytests/ui/test_workspace_config.pyweb-ui/src/components/settings/Proof9DefaultsTab.tsxweb-ui/src/components/settings/WorkspaceConfigTab.tsxweb-ui/src/lib/proof.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- web-ui/src/components/settings/Proof9DefaultsTab.tsx
- web-ui/src/components/settings/WorkspaceConfigTab.tsx
- codeframe/ui/routers/workspace_v2.py
- tests/ui/test_workspace_config.py
- web-ui/src/lib/proof.ts
| # ============================================================================ | ||
| # PROOF9 Config (issue #556) | ||
| # | ||
| # Persists which gates are enabled by default and the strictness setting | ||
| # (strict vs warn) to .codeframe/proof_config.json. | ||
| # ============================================================================ | ||
|
|
||
|
|
||
| _VALID_GATES = {g.value for g in Gate} | ||
|
|
||
|
|
||
| def _proof_config_path(workspace: Workspace): | ||
| return workspace.state_dir / PROOF_CONFIG_FILENAME | ||
|
|
||
|
|
||
| def _default_proof_config() -> dict: | ||
| return {"enabled_gates": list(PROOF9_GATE_ORDER), "strictness": "strict"} | ||
|
|
||
|
|
||
| class ProofConfigResponse(BaseModel): | ||
| enabled_gates: list[str] | ||
| strictness: Literal["strict", "warn"] | ||
|
|
||
|
|
||
| class UpdateProofConfigRequest(BaseModel): | ||
| enabled_gates: list[str] | ||
| strictness: Literal["strict", "warn"] | ||
|
|
||
| @field_validator("enabled_gates") | ||
| @classmethod | ||
| def _validate_gates(cls, v: list[str]) -> list[str]: | ||
| unknown = [g for g in v if g not in _VALID_GATES] | ||
| if unknown: | ||
| raise ValueError( | ||
| f"Unknown gate(s): {unknown}. Valid: {sorted(_VALID_GATES)}" | ||
| ) | ||
| return v | ||
|
|
||
|
|
||
| @router.get("/config", response_model=ProofConfigResponse) | ||
| @rate_limit_standard() | ||
| async def get_proof_config( | ||
| request: Request, | ||
| workspace: Workspace = Depends(get_v2_workspace), | ||
| ) -> ProofConfigResponse: | ||
| """Load PROOF9 defaults for this workspace. | ||
|
|
||
| Returns the all-gates-enabled + strict defaults if no config file exists. | ||
| """ | ||
| path = _proof_config_path(workspace) | ||
| if path.exists(): | ||
| try: | ||
| data = json.loads(path.read_text()) | ||
| return ProofConfigResponse(**data) | ||
| except (OSError, json.JSONDecodeError, ValueError) as e: | ||
| logger.warning("Invalid proof_config.json — falling back to defaults: %s", e) | ||
| return ProofConfigResponse(**_default_proof_config()) | ||
|
|
||
|
|
||
| @router.put("/config", response_model=ProofConfigResponse) | ||
| @rate_limit_standard() | ||
| async def update_proof_config( | ||
| request: Request, | ||
| body: UpdateProofConfigRequest, | ||
| workspace: Workspace = Depends(get_v2_workspace), | ||
| ) -> ProofConfigResponse: | ||
| """Persist PROOF9 defaults to .codeframe/proof_config.json.""" | ||
| payload = {"enabled_gates": body.enabled_gates, "strictness": body.strictness} | ||
| atomic_write_json(_proof_config_path(workspace), payload) | ||
| return ProofConfigResponse(**payload) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move PROOF config policy out of the router.
This block now owns default values, gate validation, file layout, and corrupted-file fallback behavior. That is domain logic, and keeping it in codeframe/ui/routers/proof_v2.py creates another place that can drift from the core runner’s interpretation of the same config. Please move load/save/default/validate behavior into a core API and keep the router to request/response translation.
As per coding guidelines, codeframe/ui/**/*.py: "FastAPI server and web UI are thin adapters over core — do NOT implement domain logic in codeframe/ui/ routers".
🤖 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 `@codeframe/ui/routers/proof_v2.py` around lines 648 - 717, The router
currently contains domain logic for defaults, validation, file layout, and
corrupted-file fallback (symbols: _VALID_GATES, _proof_config_path,
_default_proof_config, UpdateProofConfigRequest._validate_gates,
get_proof_config, update_proof_config, atomic_write_json); move that logic into
a core API (e.g., core.proof_config) by implementing functions like
default_proof_config(), validate_gates(gates) or exceptions for invalid gates,
load_proof_config(workspace) which reads the file, returns defaults on
missing/corrupt files and logs or surfaces errors, and
save_proof_config(workspace, payload) which does atomic write; then simplify the
router handlers to only call core.load_proof_config and core.save_proof_config
and map request/response models (keep ProofConfigResponse and
UpdateProofConfigRequest as DTOs) so all file/validation behavior lives in the
core module instead of codeframe/ui/routers/proof_v2.py.
| @pytest.fixture | ||
| def test_client(test_workspace): | ||
| """Create a FastAPI TestClient with proof_v2 router and workspace override.""" | ||
| from codeframe.ui.dependencies import get_v2_workspace | ||
| from codeframe.ui.routers import proof_v2 | ||
|
|
||
| app = FastAPI() | ||
| app.include_router(proof_v2.router) | ||
|
|
||
| def get_test_workspace(): | ||
| return test_workspace | ||
|
|
||
| app.dependency_overrides[get_v2_workspace] = get_test_workspace | ||
|
|
||
| client = TestClient(app) | ||
| client.workspace = test_workspace | ||
| return client |
There was a problem hiding this comment.
Make these tests send workspace_path.
Because the dependency override hands the router a workspace directly, every request in this module bypasses the required query-parameter contract. That leaves this suite unable to catch regressions in the v2 endpoint shape. Please wrap requests in a helper that always includes workspace_path and use that throughout the file.
As per coding guidelines, codeframe/ui/routers/**/*.py: "Web UI API client must require workspace_path query parameter for all endpoints".
🤖 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 `@tests/ui/test_proof_config.py` around lines 38 - 54, The tests currently
bypass the router's workspace_path query parameter by overriding
get_v2_workspace to return the workspace object directly; instead add a small
request helper on the TestClient that always includes workspace_path (derived
from test_workspace, e.g. test_workspace.path or test_workspace.workspace_path)
and use that helper across the file. Concretely: in the test_client fixture
(which references get_v2_workspace, proof_v2.router, TestClient and
test_workspace) keep the dependency override if needed but attach a helper like
client.request_with_workspace(method, path, *, params=None, **kwargs) that
merges params with {'workspace_path': <test_workspace path>} and calls
client.request(method, path, params=merged, **kwargs); replace direct
client.get/post calls in this test module with that helper so every request
includes workspace_path.
- PRODUCT_ROADMAP.md: Phase 5.1 → ✅ Complete - CLAUDE.md: add Phase 5.1 to the recent-progress note, bump date, point next-up to 5.2–5.5 - Proof9DefaultsTab/WorkspaceConfigTab: stop destructuring isLoading (unused after the load/error guard reorder in the previous commit)
Code Review — PR #589: PROOF9 Defaults + Workspace Config tabsGood implementation overall. All acceptance criteria appear met and test coverage is solid. Prior CodeRabbit reviews flagged a few things; here is where things stand plus new observations. What has been resolvedThe first CodeRabbit review's critical issue — cached Still open from previous CodeRabbit review
tmp = path.with_suffix(path.suffix + ".tmp") # same name for every callerTwo simultaneous saves to the same config file collide on the temp path. Low probability for a settings endpoint, but easy to fix with New observations1. Empty When all gates are disabled ( Suggestion: log a warning when all obligations are filtered out, or add a test that explicitly documents vacuous-pass as intentional. 2. The Suggestion: either make the field read-only in the UI ( 3. Config filename constant is duplicated
4. Missing blank line before The new Test coverageGood: defaults, round-trip, corrupt-file fallback, gate validation, strictness semantics, and the warn-mode cache regression are all exercised. Two gaps:
SummaryThe feature is solid. Two items worth addressing before merge: the temp-file race in |
- _helpers.atomic_write_json now uses tempfile.mkstemp for a unique per-call temp filename so concurrent writers do not collide on a shared ".tmp" suffix. - workspace_root input is now readOnly + disabled in the UI with a clear "Display only" note; the field served no functional purpose and the previous label invited the user to expect a relocation. - proof runner logs a warning when all gates are disabled, so the vacuous-pass case (zero obligations executed, run reports pass) is not silent. Test in tests/core/test_proof_runner_config.py locks in the documented behavior.
|
Round 3: addressed claude review in 13f18e4.
Deferred (cleanup follow-ups, not blockers):
All 109 backend + 30 frontend settings tests pass; ruff clean; build clean. |
Code Review — PR #589: feat(settings): PROOF9 defaults + workspace config tabsThis is a clean, well-scoped implementation of Phase 5.1 (#556). The layering is correct (headless core constants, thin router adapters, SWR-backed UI), tests are comprehensive, and the atomic-write helper solves the right problem. A few items worth addressing before merge: Issues1. Missing blank line in After return enabled, strictness
← only one blank line here
# Map PROOF9 gates to existing core/gates.py gate names
_GATE_TO_CORE: dict[Gate, str] = {2.
3. Error message in raise ValueError(
f"Unknown gate(s): {unknown}. Valid: {sorted(_VALID_GATES)}"
)
raise ValueError(
f"Unknown gate(s): {unknown}. Valid: {list(PROOF9_GATE_ORDER)}"
)This keeps the error consistent with the order users see everywhere else in the UI. 4. Untested fallback path in persisted_run = get_run(workspace, run_id)
if persisted_run is not None:
passed = persisted_run.overall_passed
else:
# Fallback only if persistence unexpectedly failed
passed = all(...)The Observations (no action required)
SummaryThe core logic (runner integration, atomic write, endpoint design) is sound and the test coverage is good. The three actionable items above are all minor — the missing blank line and constant placement are cosmetic/structural, and the error message sort is a small UX fix. None are blocking. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/core/test_proof_runner_config.py (1)
62-64: ⚡ Quick winUse the shared config filename constant in tests
These tests hardcode
"proof_config.json"in multiple places. Import and usePROOF_CONFIG_FILENAMEto keep tests aligned with the runner/model source of truth and avoid drift if the filename changes.Suggested refactor
from codeframe.core.proof.models import ( Gate, Obligation, + PROOF_CONFIG_FILENAME, Requirement, RequirementScope, ReqStatus, Severity, Source, ) @@ - (workspace.state_dir / "proof_config.json").write_text( + (workspace.state_dir / PROOF_CONFIG_FILENAME).write_text( json.dumps({"enabled_gates": ["unit"], "strictness": "strict"}) ) @@ - (workspace.state_dir / "proof_config.json").write_text( + (workspace.state_dir / PROOF_CONFIG_FILENAME).write_text( json.dumps({"enabled_gates": [], "strictness": "strict"}) ) @@ - (workspace.state_dir / "proof_config.json").write_text( + (workspace.state_dir / PROOF_CONFIG_FILENAME).write_text( json.dumps({"enabled_gates": ["unit"], "strictness": "strict"}) ) @@ - (workspace.state_dir / "proof_config.json").write_text( + (workspace.state_dir / PROOF_CONFIG_FILENAME).write_text( json.dumps({"enabled_gates": ["unit"], "strictness": "warn"}) )Also applies to: 98-100, 130-132, 150-152
🤖 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 `@tests/core/test_proof_runner_config.py` around lines 62 - 64, Replace hardcoded "proof_config.json" usages in tests with the shared PROOF_CONFIG_FILENAME constant: import PROOF_CONFIG_FILENAME from the module that defines it (the proof runner/model module) and use (workspace.state_dir / PROOF_CONFIG_FILENAME).write_text(...) and any path constructions or reads instead of string literals; update all occurrences (the ones around the current blocks and the ones noted at 98-100, 130-132, 150-152) so tests use the single source-of-truth constant.
🤖 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 `@codeframe/core/proof/runner.py`:
- Around line 39-47: The code assumes parsed JSON "data" is a dict and calls
data.get(...), which will crash if the JSON root is not an object; modify the
logic in runner.py (around the variables data, gates_raw, enabled_gates and
PROOF_CONFIG_FILENAME) to first verify isinstance(data, dict) before using
data.get, and if not a dict treat the config as invalid (log a warning similar
to the existing one) and fall back to the defaults (return None, "strict" or the
existing default path) so enabled remains None and downstream code won’t raise
AttributeError.
In `@web-ui/src/components/settings/WorkspaceConfigTab.tsx`:
- Around line 67-76: The code validates trimmed strings in canSave but passes
raw draft values to workspaceConfigApi.updateConfig, risking persisting
whitespace; update handleSave to normalize inputs before sending by trimming
draft.workspace_root, draft.default_branch, and draft.tech_stack_override (use
the trimmed values or default to '' for override) so the payload to
workspaceConfigApi.updateConfig contains trimmed/normalized strings; ensure
canSave still uses the same trimmed checks so validation and persisted data
match.
---
Nitpick comments:
In `@tests/core/test_proof_runner_config.py`:
- Around line 62-64: Replace hardcoded "proof_config.json" usages in tests with
the shared PROOF_CONFIG_FILENAME constant: import PROOF_CONFIG_FILENAME from the
module that defines it (the proof runner/model module) and use
(workspace.state_dir / PROOF_CONFIG_FILENAME).write_text(...) and any path
constructions or reads instead of string literals; update all occurrences (the
ones around the current blocks and the ones noted at 98-100, 130-132, 150-152)
so tests use the single source-of-truth constant.
🪄 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: CHILL
Plan: Pro
Run ID: 7461eda8-e8cc-4076-bc8e-b1342797cb3e
📒 Files selected for processing (4)
codeframe/core/proof/runner.pycodeframe/ui/routers/_helpers.pytests/core/test_proof_runner_config.pyweb-ui/src/components/settings/WorkspaceConfigTab.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- codeframe/ui/routers/_helpers.py
| try: | ||
| data = json.loads(path.read_text()) | ||
| except (OSError, json.JSONDecodeError) as exc: | ||
| logger.warning("Invalid %s — using defaults: %s", PROOF_CONFIG_FILENAME, exc) | ||
| return None, "strict" | ||
|
|
||
| enabled: Optional[set[Gate]] = None | ||
| gates_raw = data.get("enabled_gates") | ||
| if isinstance(gates_raw, list): |
There was a problem hiding this comment.
Handle non-object JSON configs to avoid runtime crashes
At Line 46, data.get(...) assumes the parsed JSON is a dict. If proof_config.json is valid JSON but not an object (e.g., []), this raises AttributeError and aborts the run instead of falling back to defaults.
Suggested fix
try:
data = json.loads(path.read_text())
except (OSError, json.JSONDecodeError) as exc:
logger.warning("Invalid %s — using defaults: %s", PROOF_CONFIG_FILENAME, exc)
return None, "strict"
+ if not isinstance(data, dict):
+ logger.warning(
+ "Invalid %s shape (expected JSON object) — using defaults",
+ PROOF_CONFIG_FILENAME,
+ )
+ return None, "strict"
enabled: Optional[set[Gate]] = None
gates_raw = data.get("enabled_gates")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| data = json.loads(path.read_text()) | |
| except (OSError, json.JSONDecodeError) as exc: | |
| logger.warning("Invalid %s — using defaults: %s", PROOF_CONFIG_FILENAME, exc) | |
| return None, "strict" | |
| enabled: Optional[set[Gate]] = None | |
| gates_raw = data.get("enabled_gates") | |
| if isinstance(gates_raw, list): | |
| try: | |
| data = json.loads(path.read_text()) | |
| except (OSError, json.JSONDecodeError) as exc: | |
| logger.warning("Invalid %s — using defaults: %s", PROOF_CONFIG_FILENAME, exc) | |
| return None, "strict" | |
| if not isinstance(data, dict): | |
| logger.warning( | |
| "Invalid %s shape (expected JSON object) — using defaults", | |
| PROOF_CONFIG_FILENAME, | |
| ) | |
| return None, "strict" | |
| enabled: Optional[set[Gate]] = None | |
| gates_raw = data.get("enabled_gates") | |
| if isinstance(gates_raw, list): |
🤖 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 `@codeframe/core/proof/runner.py` around lines 39 - 47, The code assumes parsed
JSON "data" is a dict and calls data.get(...), which will crash if the JSON root
is not an object; modify the logic in runner.py (around the variables data,
gates_raw, enabled_gates and PROOF_CONFIG_FILENAME) to first verify
isinstance(data, dict) before using data.get, and if not a dict treat the config
as invalid (log a warning similar to the existing one) and fall back to the
defaults (return None, "strict" or the existing default path) so enabled remains
None and downstream code won’t raise AttributeError.
| const canSave = dirty && !saving && draft.workspace_root.trim() !== '' && draft.default_branch.trim() !== ''; | ||
|
|
||
| const handleSave = async () => { | ||
| setSaving(true); | ||
| try { | ||
| const saved = await workspaceConfigApi.updateConfig(workspacePath, { | ||
| workspace_root: draft.workspace_root, | ||
| default_branch: draft.default_branch, | ||
| auto_detect_tech_stack: draft.auto_detect_tech_stack, | ||
| tech_stack_override: draft.tech_stack_override, |
There was a problem hiding this comment.
Persist normalized text values, not raw draft strings.
Line 67 validates default_branch with trim(), but Line 74 sends the untrimmed value; similarly Line 76 can persist whitespace-only override text. This can store invalid config and cause downstream branch resolution issues.
Suggested fix
const handleSave = async () => {
setSaving(true);
try {
+ const normalizedDefaultBranch = draft.default_branch.trim();
+ const normalizedTechStackOverride =
+ draft.tech_stack_override?.trim() || null;
+
const saved = await workspaceConfigApi.updateConfig(workspacePath, {
workspace_root: draft.workspace_root,
- default_branch: draft.default_branch,
+ default_branch: normalizedDefaultBranch,
auto_detect_tech_stack: draft.auto_detect_tech_stack,
- tech_stack_override: draft.tech_stack_override,
+ tech_stack_override: normalizedTechStackOverride,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const canSave = dirty && !saving && draft.workspace_root.trim() !== '' && draft.default_branch.trim() !== ''; | |
| const handleSave = async () => { | |
| setSaving(true); | |
| try { | |
| const saved = await workspaceConfigApi.updateConfig(workspacePath, { | |
| workspace_root: draft.workspace_root, | |
| default_branch: draft.default_branch, | |
| auto_detect_tech_stack: draft.auto_detect_tech_stack, | |
| tech_stack_override: draft.tech_stack_override, | |
| const canSave = dirty && !saving && draft.workspace_root.trim() !== '' && draft.default_branch.trim() !== ''; | |
| const handleSave = async () => { | |
| setSaving(true); | |
| try { | |
| const normalizedDefaultBranch = draft.default_branch.trim(); | |
| const normalizedTechStackOverride = | |
| draft.tech_stack_override?.trim() || null; | |
| const saved = await workspaceConfigApi.updateConfig(workspacePath, { | |
| workspace_root: draft.workspace_root, | |
| default_branch: normalizedDefaultBranch, | |
| auto_detect_tech_stack: draft.auto_detect_tech_stack, | |
| tech_stack_override: normalizedTechStackOverride, |
🤖 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 `@web-ui/src/components/settings/WorkspaceConfigTab.tsx` around lines 67 - 76,
The code validates trimmed strings in canSave but passes raw draft values to
workspaceConfigApi.updateConfig, risking persisting whitespace; update
handleSave to normalize inputs before sending by trimming draft.workspace_root,
draft.default_branch, and draft.tech_stack_override (use the trimmed values or
default to '' for override) so the payload to workspaceConfigApi.updateConfig
contains trimmed/normalized strings; ensure canSave still uses the same trimmed
checks so validation and persisted data match.
- Add the missing PEP 8 blank line between _load_proof_config and the _GATE_TO_CORE module-level mapping. - _validate_gates error message now lists gates in PROOF9_GATE_ORDER (the same order the UI shows) instead of sorted alphabetically. - Comment on the fallback branch in run_proof_endpoint documenting why the raw recompute is acceptable there.
|
Round 4: addressed the three minor items in 1e77321.
Deferred (per reviewer: not blocking): move |
Code Review — PR #589: PROOF9 Defaults + Workspace Config tabs (Phase 5.1)Summary: Solid implementation. Atomic writes, clean strictness semantics, good test coverage across all three layers. A few targeted items below. Architecture:
|
- Move WORKSPACE_CONFIG_FILENAME from workspace_v2.py (UI layer) to codeframe/core/workspace.py alongside CODEFRAME_DIR. Symmetric with PROOF_CONFIG_FILENAME in core/proof/models.py and keeps the door open for a core consumer without violating the headless-core rule. - _load_proof_config logs a targeted warning for each unknown gate name in proof_config.json, so a typo like "unittest" no longer masquerades as "all 9 gates are disabled".
|
Round 5 in 2f81b20:
Deferred (low priority): This is the last round of feedback addressed. Merging once CI is green. |
Code Review — PR #589: PROOF9 Defaults + Workspace Config TabsOverall this is a clean, well-scoped implementation. The architecture is sound: atomic writes, graceful fallbacks, proper separation between core runner logic and the API layer, and solid test coverage across both Python and TypeScript. Previous CodeRabbit issues (concurrent Remaining item from prior reviewsHardcoded
# Before
(workspace.state_dir / "proof_config.json").write_text(...)
# After
from codeframe.core.proof.models import PROOF_CONFIG_FILENAME
(workspace.state_dir / PROOF_CONFIG_FILENAME).write_text(...)New observations1.
Consider always overriding config = WorkspaceConfigResponse(**data)
# Ensure workspace_root always reflects reality
config.workspace_root = str(workspace.repo_path)
return configOr at minimum, ignore the stored 2. Parallel JSON parsing between Both read and validate 3. Dead validation in const canSave = dirty && !saving && draft.workspace_root.trim() !== '' && draft.default_branch.trim() !== '';
const canSave = dirty && !saving && draft.default_branch.trim() !== '';4.
Minor / non-blocking
Summary: The core implementation is solid and CI-clean. The main actionable items are (1) using |
- workspace_root in GET /workspaces/config is now sourced from the live workspace.repo_path on every read; any value previously stored in workspace_config.json is ignored. This closes a subtle drift bug: if the workspace ever moves, the persisted value would be silently stale, but the UI's readOnly input never lets the user correct it. Regression test test_workspace_root_always_reflects_live_path locks this in. - tests/core/test_proof_runner_config.py now uses PROOF_CONFIG_FILENAME instead of the hardcoded "proof_config.json" string, removing the silent test-vs-implementation drift risk flagged in the previous review.
|
Round 6 in e794150 — addressing the two highest-value items from the latest review:
Skipped (low priority / cosmetic):
This should be the final round. Merging when CI lands green. |
|
PR 589 Review - feat(settings): PROOF9 defaults + workspace config tabs Overall: solid implementation. The core mechanics are all well-executed. A few items worth addressing before merge. ISSUES
UpdateProofConfigRequest._validate_gates only rejects unknown gate names, not duplicates. A payload with enabled_gates containing duplicate values (e.g., unit twice) passes validation, gets persisted verbatim, and is returned by GET as-is. The UI de-dupes via enabledSet so visually fine, but the stored file is inconsistent. Fix: add a dedup step in proof_v2.py after the unknown-gates check - use dict.fromkeys(v) to preserve order while removing dupes, then return list(seen).
import logging and from codeframe.core.proof.ledger import get_run calls inside test methods should be at the module top-level. Not a correctness bug, but unusual and ruff will flag it once import-ordering rules are enabled.
When all 9 gates are unchecked the runner records overall_passed=True with zero evidence; the warning goes to server logs only. Consider an inline warning banner in Proof9DefaultsTab when draft.enabled_gates.length === 0. Usability concern, not a blocker, but mirrors the backend warning. MINOR OBSERVATIONS (non-blocking)
WHAT IS DONE WELL
|
- PUT /api/v2/proof/config now de-dupes enabled_gates while preserving submission order, so the persisted file never carries the same gate twice. Test: test_put_dedupes_gates. - Move in-function imports (logging, get_run, save_requirement, model classes) to module top-level in tests/core/test_proof_runner_config.py and tests/ui/test_proof_config.py — consistent with the rest of the test suite and friendly to future import-ordering lint rules. - Proof9DefaultsTab now shows an inline yellow warning banner when no gates are enabled, mirroring the runner's server-side warning so the vacuous-pass case is visible to the user. Tests added.
|
Round 7 in 62d51f3:
|
Code Review — PR #589: PROOF9 Defaults + Workspace Config TabsOverall: This is a well-structured, complete feature addition. The layering (shared constants in Findings1. In The simplest fix is to strip the field before persisting: payload = body.model_dump(exclude={'workspace_root'})
payload['workspace_root'] = str(workspace.repo_path) # keep it authoritative
atomic_write_json(_workspace_config_path(workspace), payload)The UI already disables the field and the docstring explains the semantics — but the fix makes storage consistent with the read behaviour. 2. Vacuous-pass (all gates disabled) is silent at the API level When Consider adding a 3. Test fixtures: prefer Both 4. Correct for the common case. For crash-safe durability you'd add Positives worth highlighting
Checklist summary
Items #1 and #2 are the ones I'd want resolved before merge; #3 and #4 are suggestions. |
- PUT /api/v2/workspaces/config now strips workspace_root from the client request and stores str(workspace.repo_path) instead, so PUT and GET return the same value and a client cannot persist a path that differs from the live workspace. Regression test: test_put_ignores_client_workspace_root. - Document the vacuous-pass API surface gap as a known follow-up under Phase 4A in PRODUCT_ROADMAP.md (the merge gate is the right place to consume a vacuous_pass flag; adding the field here would be speculative API surface for #556).
|
Round 8 in 46f072a — addressing the two pre-merge items from the latest review:
Skipping (explicitly flagged "suggestions" by reviewer):
23 backend + 16 frontend settings tests pass; ruff clean; build clean. |
Follow-up Review: feat(settings): PROOF9 defaults + workspace config tabs (#556)All items from my previous review have been addressed — thank you for the thorough follow-through. Notes below. Resolved (from prior review)
Still open#8 Duplicate
New: Pydantic
|
- Add ValidationError explicitly to both GET handlers' except clause. In pydantic 2.12.2 ValidationError is still a subclass of ValueError (so the prior code worked), but the explicit catch makes the intent self-documenting and survives any future pydantic-internals change. - Lock in the behavior with two new tests: test_invalid_field_value_falls_back_to_defaults (proof config) and test_invalid_field_type_falls_back_to_defaults (workspace config).
|
Round 9 in 45c085e — the lone pre-merge item:
Optional observation deferred: #8 (DRY All 25 backend + 16 frontend settings tests pass; ruff clean; build clean. Will merge when CI lands green. |
Code Review — PR #589: PROOF9 Defaults + Workspace Config tabsSolid implementation. The feature is well-scoped, architecture rules are respected (constants in core, persistence in UI layer, no FastAPI in core), and test coverage is thorough. A few items worth addressing before merge. CorrectnessVacuous-pass is a merge-gate risk (noted but under-specified)
# runner.py line ~132
enabled_gates, strictness = _load_proof_config(workspace)This is fine for the current file-based config. Worth a note: if the runner is ever called in a tight loop (batch mode), each call hits the filesystem. A per-workspace cache invalidated by mtime would be a cheap win later. Not urgent. Design
The PUT request model requires payload = body.model_dump(exclude={"workspace_root"})
payload["workspace_root"] = str(workspace.repo_path)This is safe, but it's confusing API design — the client must supply a field the server ignores. Consider making it Pydantic models for config responses are defined inline in routers
SecurityGate name injection is correctly blocked — the
If Test CoverageCoverage is comprehensive. A few gaps:
Frontend
function isDirty(a: ProofConfigResponse, b: ProofConfigResponse): boolean {
if (a.strictness !== b.strictness) return true;
if (a.enabled_gates.length !== b.enabled_gates.length) return true;
const setA = new Set(a.enabled_gates);
return b.enabled_gates.some((g) => !setA.has(g));
}This is logically correct. The length check + set-membership check covers symmetric difference. No change needed, just confirming the logic is sound.
useEffect(() => {
if (data) {
setDraft({ ...data, enabled_gates: [...data.enabled_gates] });
}
}, [data]);If SWR revalidates in the background while the user has unsaved changes, the draft will be silently reset. This is acceptable for a settings page (low-frequency edits, no concurrent writers), but a user who leaves the tab open long enough might lose in-progress changes. A guard like
The gate list is duplicated from Summary
The two |
|
Round 10: reviewer's items are all explicitly non-blocking — "Not a blocker for this PR", "Not urgent", "Low-priority but worth a follow-up". The vacuous-pass API surface is already documented in the roadmap under Phase 4A. The other items ( Proceeding with merge. |
1 participant
Summary
/api/v2/proof/config,/api/v2/workspaces/config) that persist to.codeframe/proof_config.jsonand.codeframe/workspace_config.json.proof_config.jsoninto the runner: gates not inenabled_gatesare skipped;strictness="warn"keepsoverall_passed=Trueon gate failures (failure evidence is still recorded).Closes #556.
Acceptance criteria
npm test(813) andnpm run buildpassPlan adaptation
Traycer's plan called for
/api/v2/workspace/config(singular) via a new router. Implemented under the existing/api/v2/workspaces(plural) router for consistency — same behavior, one fewer router registration.Test plan
uv run pytest -m v2— 2949 passed, 9 skipped (e2b cloud)uv run ruff check .— cleancd web-ui && npm test— 813 passedcd web-ui && npm run build— cleanNew tests
tests/ui/test_proof_config.py(7) — defaults, persistence, validation, round-triptests/ui/test_workspace_config.py(5) — defaults, persistence, round-trip, validationtests/core/test_proof_runner_config.py(4) — gate filtering, strictness behaviorweb-ui/src/__tests__/components/settings/Proof9DefaultsTab.test.tsx(6)web-ui/src/__tests__/components/settings/WorkspaceConfigTab.test.tsx(8)Demo (run locally)
cf serveandcd web-ui && npm run dev/settings→ PROOF9 tab → toggle gates, change strictness, Save → reload → values persist.codeframe/proof_config.jsonwithenabled_gates: ["unit"]andstrictness: "warn"→cf proof run: onlyunitgate runs;overall_passed=Trueeven if it fails.Summary by CodeRabbit
New Features
Chores
Tests