feat(FAR-795) [partial]: org-scoped persistent config flag substrate (work-item agent minting) - #453
Conversation
…te + work_item_agent_minting_enabled flag Slice A substrate for the agent-mint safety layer: - core/runtime_config/org_flags.py: org-scoped boolean flags persisted in the existing Organisation.settings_json surface (no schema change), readable cheaply on a hot path via a 30s-TTL per-process cache with write invalidation, and fail-closed on ANY read error (DB outage -> flag OFF) via is_org_flag_enabled. Only a JSON bool True enables; unknown/malformed values never enable. - admin API: GET/PUT /api/v1/admin/org/work-item-agent-minting mirroring the sandbox/run-concurrency sibling endpoints (admin-role-gated, org-scoped, StrictBool to reject truthiness coercion, SELECT...FOR UPDATE on the settings read-modify-write, AuditEvent org.work_item_agent_minting_updated on change). No mint-path wiring in this slice. No migration (reuses settings_json).
…ting endpoints The schema-freshness CI job failed because this branch added GET/PUT /api/v1/admin/org/work-item-agent-minting endpoints but the generated frontend/src/lib/api/schema.ts was stale. Regenerated via openapi-typescript@7.13.0 to re-sync the committed schema. Co-Authored-By: Branch Fixer Bot <bot@farnalabs.com>
Automated fix: schema freshnessRoot cause: The branch added admin API endpoints Fix: Regenerated Commit: |
farnalabs
left a comment
There was a problem hiding this comment.
Review feedback (FAR-795 Slice A substrate) — informational only, formal decision posted separately.
Overall this looks solid:
- org_flags.py substrate is fail-closed by design (is_org_flag_enabled returns False on any read error and evicts the cache entry so the next call retries the DB), strictly honours only JSON bool true, and documents the ≤30s cross-process staleness contract. The SELECT ... FOR UPDATE read-modify-write in set_org_flag correctly prevents dropping concurrent settings_json writers, and the merge preserves sibling keys (covered by tests).
- Admin routes correctly mirror the sandbox/run-concurrency siblings: admin-role-gated, org-scoped from the principal (never from path/body), StrictBool rejects 1/"true" coercion with 422, error mapping (LookupError→404, ProgrammingError→501, SQLAlchemyError→503) matches house style, and the audit event is best-effort in a second transaction after the flag commit — flag update is never lost to an audit failure.
- schema.ts regeneration (7d43605) re-syncs the OpenAPI types and resolves the Schema freshness failure from bd0e038; field names match backend Pydantic snake_case exactly. The _fresh?: boolean query param in the generated operations comes from the global dependency, same as sibling endpoints.
- No frontend UI consumes the endpoints yet — acceptable for a Slice A substrate PR.
Minor notes (non-blocking):
- backend/src/modulo/api/routes/admin.py (audit block in PUT): 'except HTTPException: raise' is dead code inside the audit transaction — nothing in that block raises HTTPException. Harmless (mirrors sibling patterns) but could be dropped for clarity.
- backend/src/modulo/core/runtime_config/org_flags.py: read_org_flag caches a False default for a missing org (org=None) for the full TTL; fail-closed so safe, just noting the 30s window before a newly created org's flag read retries the DB.
- Tests are unit-level with mocked sessions; an integration/BDD round-trip through the real endpoint/table will be worth adding when Slice B wires minting to this flag.
modulo-reviewbot
left a comment
There was a problem hiding this comment.
Approval (PR Review post-decision)
Approving PR #453 (FAR-795 Slice A: org-scoped persistent config flag substrate).
Key findings from the review node (verdict: APPROVE, low-risk):
- New org-flag substrate in org_flags.py is fail-closed (any read error -> False + cache eviction) and strict (only JSON bool true enables).
- set_org_flag uses SELECT...FOR UPDATE with dict-merge preserving sibling settings_json keys — correct against concurrent writers.
- GET/PUT /org/work-item-agent-minting mirror the sandbox/run-concurrency siblings: admin-only, org from principal, RLS set in-transaction, correct error mapping (404/501/503).
- Audit event is best-effort post-commit in a second transaction — a failing audit never rolls back the flag update.
- Tests cover default-off, non-bool rejection, TTL expiry, fail-closed eviction, FOR UPDATE lock, merge preservation, and RBAC.
- Regenerated frontend schema.ts matches backend snake_case fields and resolves the Schema freshness CI failure.
Three non-blocking minor notes were carried in the review-node comments (dead 'except HTTPException: raise' in the audit block, 30s cached-False window for missing orgs, and unit-mocked tests without an integration round-trip yet). No high-risk path, test-deletion, or skip/xfail additions detected.
Add unit tests covering every previously-uncovered branch of the new work-item agent-minting admin routes (admin.py) and the org_flags substrate: - GET/PUT asyncio.CancelledError propagation (fail-open, never swallowed) - GET unexpected Exception -> 500 - PUT LookupError -> 404, IntegrityError -> 409, ProgrammingError -> 501 - PUT HTTPException re-raise verbatim, unexpected Exception -> 500 - PUT audit-event fire-and-forget branches (IntegrityError/ProgrammingError/ SQLAlchemyError/Exception swallowed, CancelledError propagated) - org_flags.read_org_flag org-not-found -> fail-closed default OFF Brings new-code coverage to 100% so the SonarCloud new_coverage gate re-runs green.
Automated fix: SonarCloud new-code coverage gate (FAR-795)The CI failure was the SonarCloud
Commit
|
farnalabs
left a comment
There was a problem hiding this comment.
Re-review of new head d7db5ad (commit 3: test coverage raise for FAR-795 org-flag routes) — no issues found, prior approval stands.
Findings from the full review of the three-dot diff vs origin/main:
- org_flags.py substrate: fail-closed semantics are correct (
raw is Truestrictness, error → False + cache eviction). TTL ≤30s documented, cross-process staleness bounded by design. - admin.py routes faithfully mirror the sandbox-concurrency pattern (inline admin role check, RLS, FOR UPDATE read-modify-write, identical error-surface mapping 404/409/501/503/500, best-effort audit in a second transaction). Introspection EXEMPT entry for the PUT is consistent with the sibling allowlist.
- Prove-the-fix: tests genuinely exercise the real routes (httpx ASGITransport + dependency overrides); 422 for non-bool StrictBool, string "true" reads OFF, FOR UPDATE asserted — each would fail without the implementation.
- Contract round-trip: snake_case field names match on both sides; schema.ts regenerated (Schema freshness CI green). No frontend UI consumers yet (Slice A substrate).
- No test deletions, no skip/xfail markers.
Minor notes (non-blocking): is_org_flag_enabled has no production callers yet (expected — Slice A substrate for the later SAQ-side safety layer); read_org_flag's default param is only ever invoked with False today, and a default=True caller would cache the default for the TTL — worth remembering when new flags are added.
modulo-reviewbot
left a comment
There was a problem hiding this comment.
Approve: org-scoped TTL-cached fail-closed runtime flag substrate (org_flags.py backed by Organisation.settings_json), admin GET/PUT work_item_agent_minting_enabled routes mirroring the sandbox-concurrency pattern, regenerated frontend OpenAPI schema, and 609 lines of tests.
- Fail-closed substrate sound: only JSON bool true enables; read errors clear cache + return False; 30s TTL with documented cross-process staleness.
- Prove-the-fix holds: tests assert FOR UPDATE lock, settings-merge preservation, StrictBool 422 rejection, string-true reads OFF.
- Contract round-trip holds: snake_case Pydantic fields match schema.ts (regenerated).
- No test deletions and no skip/xfail markers anywhere in the diff.
Verdict APPROVE (low-risk, authorized auto-approve). Human HITL gate not applicable.
…Cloud gate Extract _run_admin_rls_txn and _record_org_audit shared helpers and route the sandbox-concurrency, work-item-agent-minting, and run-concurrency endpoints through them. This collapses the near-identical admin-role gate, RLS setup, DB->HTTP error mapping, and best-effort audit blocks into one copy each, cutting new-code duplication below SonarCloud's 3% threshold and lifting new-code coverage above 80% (added 404/409/500/audit-failure tests for the minting endpoint). Fixes SonarCloud Code Analysis failure on head 7d43605 (PR #453).
Branch Fixer — SonarCloud quality gate fixThe SonarCloud Code Analysis check on
Both are attributable to the org-config admin routes in Fix (commit
|
|
Branch Fixer stood down on this PR. The SonarCloud quality-gate failure (new_duplicated_lines_density 9.9% > 3%) on commit d7db5ad was already resolved on-branch by commit 1b68792 ("fix(admin): dedupe org-config route error/audit blocks to clear SonarCloud gate") — a comprehensive refactor that extracts the shared I did not push a competing commit to avoid racing the existing fix. Verified locally that 1b68792 passes ruff, ruff-format and |
farnalabs
left a comment
There was a problem hiding this comment.
Review feedback (FAR-795 Slice A, head 1b68792) — APPROVE recommendation, feedback only:
- org_flags.py: clean fail-closed substrate (only strict JSON
trueenables; DB errors evict cache and resolve OFF; ≤30s TTL documented as worst-case staleness). FOR UPDATE row-lock on the settings_json read-modify-write matches the FAR-589 D3b pattern. - admin.py: the _run_admin_rls_txn / _record_org_audit dedupe preserves the previous error contract (403 admin gate, 404, 409, 501, 503, 500, CancelledError passthrough) in one place; the new minting routes mirror sandbox-concurrency exactly, and the route_introspection exemption entry is consistent.
- Contract round-trip: UpdateWorkItemAgentMintingRequest/WorkItemAgentMintingResponse use snake_case work_item_agent_minting_enabled matching the Pydantic fields; StrictBool rejects 1/'true' coercion (covered by test_put_rejects_non_bool). schema.ts changes match the generated OpenAPI shape.
- Tests cover the real route via dependency overrides (not just idealized payloads): default-off, string-'true' reads off, role gates, 404/409/501/503/500 surfaces, audit swallow branches, cache TTL/staleness/fail-closed-eviction. Note: could not execute the suite in the review sandbox (repo requires Python ≥3.12 for PEP 695 generics; sandbox has 3.11 only) — CI on this SHA is green so far (only the parallel SonarCloud scan still running).
- Minor (non-blocking): read_org_flag caches a missing-org read as the default for the full TTL; acceptable since it fails closed, just be aware a newly-created org can read stale OFF for up to 30s.
modulo-reviewbot
left a comment
There was a problem hiding this comment.
APPROVED - PR #453 (g-far-795-org-config, head 1b68792).
Low-risk change: no high-risk path matched. Decision artifacts (policy-router + review node) both APPROVE.
Key findings from review:
- New fail-closed org-flag substrate (core.runtime_config.org_flags) persisted in Organisation.settings_json with FOR UPDATE row-locking; only strict JSON true enables; DB errors evict cache and resolve OFF; 30s TTL documented worst-case staleness (fails closed).
- Admin self-service GET/PUT /api/v1/admin/org/work-item-agent-minting routes; _run_admin_rls_txn/_record_org_audit dedupe preserves prior error contract (403/404/409/501/503/500, CancelledError passthrough).
- Wire contract round-trips: snake_case request/response fields match Pydantic models; regenerated schema.ts consistent.
- Comprehensive tests: default-off, strict bool semantics, role gates, error surfaces, audit swallow branches, cache TTL/staleness/fail-closed eviction, introspection exemption.
- CI: 12/13 checks green on this SHA, 0 failures; PR mergeable. Minor non-blocking note: missing-org read caches fail-closed default for full TTL.
|
|
Branch Fixer — no action taken (branch already green) This Branch Fixer run was triggered by the SonarCloud quality-gate failure on run No competing fix was pushed to avoid racing the author's commit. If the re-run's SonarCloud gate still fails, that would be a new signal and the Branch Fixer can act then. |



What
FAR-795 slice A � org-scoped, persistent config-flag substrate, plus the first flag
work_item_agent_minting_enabled(default OFF) with an admin GET/PUT and an audit event.Why
FAR-794 (PR #427) ships work-item refs with agent-sourced minting deliberately OFF. The safety layer's kill-switch must be readable by EVERY mint path across API and SAQ worker processes. The existing
RuntimeConfigStoreis a process-global in-memory singleton keyed by env vars � a flag set via the API never reaches the workers, so it cannot serve as the control.How
Reuses the existing org-scoped persistent surface
Organisation.settings_json(already backing sandbox/run concurrency + retention) � no migration needed.read_org_flagwith a 30s TTL in-process cache;is_org_flag_enabledis fail-CLOSED (any read error ? OFF, logs, evicts cache so the next call retries). Only a JSON boolTrueenables. Admin endpoints mirror the sandbox/run-concurrency siblings (admin-gated, org-scoped from principal, StrictBool, standard error mapping, AuditEvent).Not in this PR
The flag is deliberately NOT wired into any mint path � that is the next FAR-795 slice.
[partial]so FAR-795 stays open.Tests
50 new tests (org_flags + admin route), architecture test-style suite, ruff/mypy clean.