Skip to content

feat(FAR-795) [partial]: org-scoped persistent config flag substrate (work-item agent minting) - #453

Merged
github-actions[bot] merged 4 commits into
mainfrom
g-far-795-org-config
Sep 13, 2026
Merged

feat(FAR-795) [partial]: org-scoped persistent config flag substrate (work-item agent minting)#453
github-actions[bot] merged 4 commits into
mainfrom
g-far-795-org-config

Conversation

@farnalabs

Copy link
Copy Markdown
Owner

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 RuntimeConfigStore is 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_flag with a 30s TTL in-process cache; is_org_flag_enabled is fail-CLOSED (any read error ? OFF, logs, evicts cache so the next call retries). Only a JSON bool True enables. 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.

…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).
@farnalabs farnalabs added the agent-generated PR created by an autonomous agent label Sep 13, 2026
…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>
@farnalabs

Copy link
Copy Markdown
Owner Author

Automated fix: schema freshness

Root cause: The branch added admin API endpoints GET/PUT /api/v1/admin/org/work-item-agent-minting (org-scoped agent-minting kill-switch flag, FAR-795), but the committed generated frontend type schema frontend/src/lib/api/schema.ts was stale. CI's schema-freshness job regenerates the schema from the backend OpenAPI and fails when the committed file differs.

Fix: Regenerated frontend/src/lib/api/schema.ts from the backend OpenAPI via openapi-typescript@7.13.0 (the exact version pinned in CI and frontend/package.json), adding the new /api/v1/admin/org/work-item-agent-minting path plus the UpdateWorkItemAgentMintingRequest and WorkItemAgentMintingResponse components. No other code changed.

Commit: 7d4360574 — fast-forwarded onto the branch head, no history rewrite. The schema freshness gate should now pass.

@farnalabs farnalabs left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

  1. 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.
  2. 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.
  3. 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 modulo-reviewbot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@farnalabs

Copy link
Copy Markdown
Owner Author

Automated fix: SonarCloud new-code coverage gate (FAR-795)

The CI failure was the SonarCloud new_coverage quality gate:\n> Coverage on New Code 79.1% is below the 80% threshold.

**/schema.ts and backend/tests/** are excluded from the SonarCloud new-code scope (sonar-project.properties), so the only in-scope new production code is api/routes/admin.py and core/runtime_config/org_flags.py. The gap was the error-surface branches of the new work-item agent-minting routes that had no tests.

Commit d7db5adc92729f773e355107e4984eb79e9b2e3b

Added unit tests covering every previously-uncovered branch:

  • GET/PUT asyncio.CancelledError propagation — the safety control must never swallow cancellation (the minting chokepoint must be able to abort).
  • GET unexpected Exception → 500.
  • PUT LookupError → 404, IntegrityError → 409, ProgrammingError → 501, HTTPException re-raised verbatim, unexpected Exception → 500.
  • PUT audit-event fire-and-forget block: IntegrityError / ProgrammingError / SQLAlchemyError / generic Exception are logged and swallowed (the flag write already committed), while asyncio.CancelledError still propagates.
  • org_flags.read_org_flag org-not-found → fail-closed default OFF.

These mirror the existing fail-closed contract of the substrate (DB outage / unknown state ⇒ minting disabled). New-code coverage is now 100%, so the SonarCloud analysis re-runs green.

All backend unit tests for this feature pass and ruff / ruff-format / bandit / gitleaks are clean.

@farnalabs farnalabs left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 True strictness, 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 modulo-reviewbot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
@farnalabs

Copy link
Copy Markdown
Owner Author

Branch Fixer — SonarCloud quality gate fix

The SonarCloud Code Analysis check on 7d43605 failed two new-code gates:

  • Coverage on New Code: 79.1% (required ≥ 80%)
  • Duplication on New Code: 9.9% (required ≤ 3%)

Both are attributable to the org-config admin routes in backend/src/modulo/api/routes/admin.py — the work-item-agent-minting GET/PUT routes repeated the same RLS/error-mapping/audit boilerplate that the sandbox- and run-concurrency routes already had, and the error branches were untested.

Fix (commit 1b68792b5)

  • Extracted two shared helpers: _run_admin_rls_txn(session, current_user, body, *, detail_admin) (RLS set + branded 403/501/503/500/DuplicateKey→409 mapping, CancelledError re-raised) and _record_org_audit(...) (best-effort, non-fatal audit block).
  • Routed the minting GET/PUT (and reused the pattern for the sandbox- and run-concurrency GET/PUT) through these helpers, removing the duplicated error/audit blocks.
  • Kept each route opening its own async with session.begin(): + set_rls_org(...) (satisfies the missing-session-begin semgrep rule) and referenced append_audit_event via the modulo.core.audit_logger module object (no new inline-import finding), so CI semgrep stays green.
  • The branch already carried comprehensive error-surface tests for these routes (404/409/500/CancelledError/HTTPException-reraise/audit-fire-and-forget), which now cover the previously-uncovered branches, lifting new-code coverage above 80% and dropping duplication below 3%.

All checks pass locally: ruff, ruff format, mypy --strict, semgrep --error, and the affected unit tests. Pushed to g-far-795-org-config for CI re-run.

@farnalabs

Copy link
Copy Markdown
Owner Author

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 _run_admin_rls_txn error contract and _record_org_audit audit block used by all org-config routes, consolidating the duplicated error handling into single functions. This supersedes the minimal route-level helper substitution I had prepared.

I did not push a competing commit to avoid racing the existing fix. Verified locally that 1b68792 passes ruff, ruff-format and mypy --strict; its in-progress CI run (34762257524) will re-evaluate the SonarCloud gate. No changes pushed by the Branch Fixer.

@farnalabs farnalabs left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review feedback (FAR-795 Slice A, head 1b68792) — APPROVE recommendation, feedback only:

  • org_flags.py: clean fail-closed substrate (only strict JSON true enables; 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 modulo-reviewbot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sonarqubecloud

Copy link
Copy Markdown

@farnalabs

Copy link
Copy Markdown
Owner Author

Branch Fixer — no action taken (branch already green)

This Branch Fixer run was triggered by the SonarCloud quality-gate failure on run 34760849380 (head d7db5adc9). That failure has already been resolved by the author's follow-up commit 1b68792b5"fix(admin): dedupe org-config route error/audit blocks to clear SonarCloud gate" — which is now the branch HEAD and is currently being re-validated by run 34762257524 (all substantive checks green; SonarCloud scan still running on the fix commit).

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.

@github-actions
github-actions Bot merged commit 77eb141 into main Sep 13, 2026
17 checks passed
@github-actions
github-actions Bot deleted the g-far-795-org-config branch September 13, 2026 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-generated PR created by an autonomous agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants