Repository navigation
Experimental - #8
Merged
Merged
Conversation
The acting user was a self-asserted free-text string (ProposeBody.user / localStorage) threaded into a Co-authored-by trailer — anyone could claim to be anyone, the classic enterprise blocker. Add a thin identity layer (spec §17): - mdl_server/identity.py resolves who a request is from: a trusted reverse-proxy header (enterprise SSO via oauth2-proxy/Pomerium/cloudflared/nginx/Azure), the model dir's git config (solo — zero config), or anonymous. - Safe by default: a raw header is NEVER trusted unless the operator names it via MDL_AUTH_TRUSTED_USER_HEADER; an optional shared-secret gate (fail-closed when the value is missing) proves the request came through the proxy. - /api/model now reports an identity block so the Studio app can greet the user. - propose attributes the commit to the SERVER-resolved identity, overriding the self-asserted string when trusted; the branch prefix and Co-authored-by trailer both derive from the effective author. Falls back to the client string only when anonymous, so nothing regresses for a solo user with no git config. No OAuth/OIDC/SAML flow, token vault, user store, or sessions — the proxy already did the IdP dance; authorization stays in git (branch protection + CODEOWNERS). Also records the Principal-Architect/PM enterprise-adoption review as Part II of modelith-spec.md (competitive positioning, ranked gaps, identity spec, M7-M9). Tests: 19 new identity unit tests (resolution, safe defaults, secret gate, author precedence) + propose integration tests proving a spoofed body.user cannot override a proxy-authenticated author. 525 passed, 1 skipped; ruff clean.
Consumes the /api/model identity block (spec §17): when the server established a trusted identity (proxy or git config), the propose dialog shows "Signed in as X — this proposal is attributed to you" instead of the free-text "Your name" input, and no longer requires or sends a self-asserted name (the server overrides it anyway). Falls back to the name input only for an anonymous deployment, so nothing changes for a solo user with no git identity. Rebuilds the static SME bundle.
A shared/enterprise deployment can now hard-fail writes by an unestablished
(anonymous) identity instead of silently accepting a self-asserted author. When
MDL_AUTH_REQUIRE is truthy, every write endpoint — /propose, /commit, /discard,
/api/command, /api/decisions/{k}/verdict — returns 403 with a clear reason; a
trusted proxy or git identity always passes, and the gate is OFF by default so a
solo user with no git identity is never blocked.
write_denied_reason() in identity.py centralizes the decision; the routes just
translate it to 403. Verified anonymous→403 / proxy→200 in unit + integration tests
and against the running server. 532 passed, 1 skipped; ruff clean.
The steward flow was broken end to end: staged edits collect in the client tray and are never written to the working tree until propose, but the Review screen diffed the working tree against HEAD and the route panel classified the working tree too. So a purely staged proposal showed "0 changes / no route" and Submit stayed disabled — the steward could not actually propose anything. - SmeApp previews the COMBINED tray (glossary edits + model edits) via /api/preview and feeds that diff to the Review screen; the two trays are one proposal. - New POST /api/git/classify previews the change set to learn which model files it touches (preview_changed_paths) and routes those, so reviewers/gates are correct before anything is written. A `model/` sentinel prefix normalizes every layout (model at repo root, under model/, or a nested dir) to what classify_paths expects. - ReviewScreen takes stagedDiff + stagedRoute props and uses them over the working-tree fallbacks. Verified end to end against a standalone git repo: a definition edit shows a real before/after diff, routes to A · Meaning · data-stewards, Submit enables, and it lands on sme/<identity>/… authored by the resolved identity. 535 passed, 1 skipped; ruff clean; canvas builds.
The proposal branch slug was auto-derived from the title with no way to change it, so a modeler could not prepend a ticket id (e.g. PROJ-123). Add a "Branch name" field that defaults to the title-derived slug and follows the title until the user edits it, then stops. The "Goes to" preview now shows the full branch (sme/<identity>/<slug>) instead of an ellipsis, slugified the same way the server does so the preview matches the branch actually created. Client-only change (the server already accepted a slug); a contract test locks in that a ticket-prefixed slug becomes sme/<identity>/proj-123-… . 536 tests pass, ruff clean, canvas builds.
Git records model changes but not administrative ones: who was let in, what a
request tried and whether it was ALLOWED or REFUSED (a denied write never becomes a
commit, so git can't show it). Add an audit trail that captures exactly those, and
make it OTel-native so it lands in whatever collector/SIEM an enterprise already
runs — no bespoke integration.
- mdl_server/audit.py: an AuditSink resolved once from env. Emits each write event
as an OpenTelemetry log record over OTLP when OTEL_EXPORTER_OTLP_ENDPOINT is set,
and/or appends a local JSONL file when MDL_AUDIT_LOG is set. Off when neither is
set — solo users configure nothing and get no stray file. OTel is an optional
extra (modelith-dbt[audit]); absent, it degrades to JSONL only, never an import
error. Records carry trace context when a span is active.
- Every write endpoint emits {ts, identity, source, op, target, outcome}: /api/command
and /api/decisions/{k}/verdict (app.py), /commit /discard /propose (git_api.py) —
including denied/errored outcomes, which are the events a reviewer most wants.
- GET /api/audit tails the local file, admin-gated (404 when auditing off, 403 for an
anonymous viewer under MDL_AUTH_REQUIRE) so a shared server doesn't leak its own
trail. The record names the action and target, never the model's contents.
Verified end-to-end against the running tool: an authenticated command is recorded
ok, an anonymous one is recorded denied, /api/audit returns both to an admin and 403s
an anonymous viewer, and OTLP records build + export (retrying against a live
collector). 546 passed, 1 skipped; ruff clean. README + spec §20 document the config.
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness and operational issues in the new audit + staged-classify implementations (timestamp generation, request validation, and audit attribution/identity selection) that should be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces an opt-in “enterprise adoption” layer for Modelith Studio: server-resolved identity (proxy header or git config), an optional anonymous-write gate, and an audit log, plus UX/server fixes so the Review/propose flow uses the staged change set and supports an editable branch slug.
Changes:
- Add identity resolution + write gating (
MDL_AUTH_*) and propagate server-resolved identity into propose attribution and API responses. - Add audit logging (JSONL + optional OpenTelemetry OTLP export) and an admin-gated
/api/auditendpoint. - Fix Review/propose routing and diffs to operate on the staged change set; add branch-name (slug) editing in the propose dialog.
File summaries
| File | Description |
|---|---|
| README.md | Documents shared-server identity configuration and audit logging setup/behavior. |
| pyproject.toml | Adds optional audit extra for OpenTelemetry dependencies (root project). |
| packages/server/pyproject.toml | Adds optional audit extra for server package. |
| modelith-spec.md | Adds Enterprise Adoption spec section (identity, require gate, staged classify, audit). |
| packages/server/src/mdl_server/identity.py | New identity policy + resolution and write-gate logic. |
| packages/server/src/mdl_server/audit.py | New audit sink (JSONL + optional OTLP export). |
| packages/server/src/mdl_server/app.py | Wires identity into /api/model, gates write endpoints, adds /api/audit, emits audit events. |
| packages/server/src/mdl_server/git_api.py | Identity-aware branch prefix/authoring, staged classify endpoint, and audit emission for git writes. |
| packages/server/src/mdl_server/preview.py | Adds preview_changed_paths() to classify staged changes by affected files. |
| packages/server/tests/test_identity.py | New unit tests for identity resolution, author precedence, and require gate. |
| packages/server/tests/test_audit.py | New tests for audit sink behavior and server audit endpoint gating. |
| packages/server/tests/test_propose.py | Updates propose tests for identity precedence, require gate behavior, and edited slug behavior. |
| packages/server/tests/test_preview.py | Adds tests for staged classify + changed-path detection for staged edits. |
| packages/server/tests/test_diff_api.py | Updates git context/proposals tests for identity-driven SME branch prefix. |
| canvas/src/types.ts | Extends ModelDoc to include server-resolved identity. |
| canvas/src/api.ts | Adds classifyStaged() API call. |
| canvas/src/sme/SmeApp.tsx | Uses combined staged changes for Review diff + staged classify; captures server identity and passes to dialog. |
| canvas/src/sme/ReviewScreen.tsx | Prefers staged classification for routing/reviewers on staged proposals. |
| canvas/src/sme/ProposeDialog.tsx | Adds signed-in UX, branch slug edit field, and slugify helper matching server behavior. |
| canvas/src/sme/sme.css | Adds styling for “Signed in as …” block in propose dialog. |
| packages/server/src/mdl_server/static/index.html | Updates hashed asset references for Studio build. |
| packages/server/src/mdl_server/static/sme.html | Updates hashed asset references for SME build. |
| packages/server/src/mdl_server/static/mocks.html | Updates hashed asset references for mocks build. |
| packages/server/src/mdl_server/static/assets/main-DKFrx98H.js | Updated built Studio JS bundle. |
| packages/server/src/mdl_server/static/assets/sme-CnDkXpN4.js | New built SME JS bundle (hashed). |
| packages/server/src/mdl_server/static/assets/sme-M2gtckV0.js | Removes old built SME JS bundle (hashed). |
| packages/server/src/mdl_server/static/assets/sme-BFiNU1gH.css | New built SME CSS bundle (hashed). |
| packages/server/src/mdl_server/static/assets/sme-C7XDneCF.css | Removes old built SME CSS bundle (hashed). |
| packages/server/src/mdl_server/static/assets/ModelDiagram-DmGsnZ_X.js | Updated built diagram chunk to reference new styles bundle. |
| packages/server/src/mdl_server/static/assets/mocks-CG2pmWEb.js | New built mocks JS bundle (hashed). |
Review details
Files not reviewed (2)
- packages/server/src/mdl_server/static/assets/sme-BFiNU1gH.css: Generated file
- packages/server/src/mdl_server/static/assets/sme-CnDkXpN4.js: Generated file
Suppressed comments (1)
packages/server/src/mdl_server/app.py:263
- Audit records for decision verdicts prefer ident.name over ident.email. For consistency with other audit events and better uniqueness in shared deployments, prefer email when present.
audit.record(
"verdict", identity=ident.name or ident.email, source=ident.source,
target=signal_key, outcome="ok", detail=verdict.value,
)
- Files reviewed: 28/33 changed files
- Comments generated: 7
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+71
to
+74
| def _now_iso() -> str: | ||
| return datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S.") + ( | ||
| f"{datetime.now(UTC).microsecond // 1000:03d}Z" | ||
| ) |
Comment on lines
+845
to
+848
| changes = body.get("changes") or [] | ||
| if not isinstance(changes, list): | ||
| return JSONResponse({"ok": False, "error": "changes must be a list"}, status_code=422) | ||
| return JSONResponse(_classify_staged_response(model_dir, changes)) |
Comment on lines
+1023
to
+1026
| _audit( | ||
| "propose", ident, branch_name, "ok", | ||
| f"{applied} change(s), pushed={payload.get('pushed')}", | ||
| ) |
Comment on lines
+57
to
+70
| def _yaml_bytes(root: Path) -> dict[str, bytes]: | ||
| out: dict[str, bytes] = {} | ||
| for p in root.rglob("*.yaml"): | ||
| out[str(p.relative_to(root))] = p.read_bytes() | ||
| return out | ||
|
|
||
| before = _yaml_bytes(disk_root) | ||
| after = _yaml_bytes(tmp) | ||
| changed = { | ||
| rel | ||
| for rel in before.keys() | after.keys() | ||
| if before.get(rel) != after.get(rel) | ||
| } | ||
| return sorted(changed) |
Comment on lines
+244
to
+247
| audit.record( | ||
| "verdict", identity=ident.name or ident.email, source=ident.source, | ||
| target=signal_key, outcome="denied", detail=denied, | ||
| ) |
Comment on lines
+268
to
+272
| denied = write_denied_reason(ident, policy) | ||
| op = body.get("op", "") | ||
| _who = dict(identity=ident.name or ident.email, source=ident.source) | ||
| if denied: | ||
| audit.record("command", target=op, outcome="denied", detail=denied, **_who) |
Comment on lines
+780
to
+784
| def _audit(op: str, ident: Identity, target: str, outcome: str, detail: str | None = None): | ||
| audit.record( | ||
| op, identity=ident.name or ident.email, source=ident.source, | ||
| target=target, outcome=outcome, detail=detail, | ||
| ) |
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
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.
What this changes
Adds the enterprise-adoption layer to Modelith Studio without disturbing the solo, git-native experience: an IdP-agnostic identity seam (trusted reverse-proxy header → SSO, or git config for solo), a write gate (MDL_AUTH_REQUIRE) for shared servers, an OpenTelemetry-native audit log, a fix so the Review screen and reviewer/route panel read the staged change set instead of the working tree (the steward couldn't propose before), and an editable branch name on the propose dialog. All configuration is opt-in; with nothing set, behavior is unchanged for a solo user.
How it was tested
Plus end-to-end verification against a running mdl studio on a standalone git repo: identity resolves git/proxy/anonymous correctly (spoofed header ignored when auth is off); an anonymous write under MDL_AUTH_REQUIRE returns 403 and is recorded; a staged glossary edit shows a real diff, routes to its reviewers, submits to sme// authored by the resolved identity; and the audit log captures both allowed and denied writes, with GET /api/audit admin-gated. Browser screenshots captured for the Review/propose flow.
Checklist