Skip to content

refactor(credentials): expose semantic registry identifiers - #999

Open
seonghobae wants to merge 12 commits into
mainfrom
fix/credential-semantic-identifiers
Open

refactor(credentials): expose semantic registry identifiers#999
seonghobae wants to merge 12 commits into
mainfrom
fix/credential-semantic-identifiers

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Owner and reuse boundary

ContextualWisdomLab/contextual-orchestrator owns the shared runtime credential registry used by provider discovery, model routing, CI gateway seeding, server bearer-token resolution, and downstream ecosystem consumers. Its public Python helpers exposed generic keyword names (name, value, backend) even though the domain concepts are credential name, credential value, and credential backend.

TDD / repair

  • Regression-first commit 99cb1d1af9a51f41d923dd389e4239f6bc530c74 requires public semantic signatures and semantic keyword calls while pinning legacy keyword compatibility.
  • credentials.py now uses credential_name, credential_value, credential_backend, credential_store, credential_lock, and database-specific local names internally.
  • Historical public Python keywords name=, value=, and backend= remain bounded compatibility aliases. Semantic+legacy duplicate authority fails closed; unknown compatibility kwargs are rejected.
  • inspect.signature reports the required semantic parameters through explicit __signature__ metadata installed with setattr; no typing/deprecation/security finding is suppressed.
  • Existing positional calls are unchanged.

Review-driven causal repair

Fresh Devin review found that the semantic rename had updated InMemoryCredentialBackend to _credential_lock / _credential_store but left package-internal atomic registration and rollback callers on removed _lock / _store. That was a real current-head runtime defect, not a naming-only concern: the default in-memory provider bootstrap could abort before model discovery.

The canonical branch now repairs both causal callers through ordinary non-force history:

  • provider_bootstrap.register_provider_credentials_atomically uses _credential_lock and _credential_store while preserving a single-lock atomic batch update;
  • provider_catalog_bootstrap._restore_provider_credentials_atomically uses the same renamed state while preserving single-lock atomic rollback/delete semantics;
  • existing provider-bootstrap and provider-catalog boundary tests exercise in-memory registration and rollback paths, so the semantic rename cannot silently detach those consumers again.

The corresponding review thread was resolved only after both source repairs landed. No predecessor check result is transferred.

Persistence / security invariants

The compliant Postgres schema is unchanged: provider_credentials(credential_name, encrypted_value, updated_at). UPSERT conflict ownership remains credential_name; pgcrypto encryption, transactions, provider-secret handling, KV-vs-env policy, and network authority do not change.

Documentation / research

docs/doctoring/credential-semantic-identifiers.md records the DDD bounded context, compatibility boundary, persistence invariants, verification contract, and APA 7 research traceability to Schankin et al. (2018) and Feitelson et al. (2022).

Exact identity

  • protected base: main@4d143601c2904a28e95d091b261c0a15e9a4f283
  • branch: fix/credential-semantic-identifiers
  • exact current head after review-driven repair: 5f1d7e23929378698a8a1d14d3327f0da2b0cde7

Fresh exact-head Tests, Fuzz, Security, Security Scan, SAST, coverage, dependency, OSV, Trivy, Scorecard, OpenCode, Strix, and queue evidence is required. Current repository workflow runs have materialized for this head and remain non-passing evidence until terminal. Predecessor/base/model-only evidence does not transfer.

Merge gate

Do not merge until the unchanged current head has all applicable required checks terminal-success, valid review findings/threads resolved, qualifying independent non-author approval current for the last push, and ordinary branch protection permits merge. No force-push, self-approval, bypass, or gate weakening.


Devin Review

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 6fcb074f-1087-48cf-b6b0-9a021f2bb7d0


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

opencode-agent Bot and others added 2 commits September 2, 2026 01:06
…rename

The credential-semantic-identifiers rename in this branch renamed
InMemoryCredentialBackend's private attributes (_store -> _credential_store,
_lock -> _credential_lock) and the module-level backend lock
(_backend_lock -> _credential_backend_lock), but two tests still referenced
the pre-rename names, failing CI's "Full unit and contract suite" job:

- tests/test_credentials_backends.py::test_get_backend_handles_another_thread_winning_initialization
  patched the nonexistent `credentials._backend_lock` instead of
  `credentials._credential_backend_lock`, and assigned to `_backend`
  instead of `_credential_backend`.
- tests/test_pii_protection.py::test_missing_kv_key_and_invalid_event_declarations_fail_closed
  popped from the nonexistent `InMemoryCredentialBackend._store` instead of
  `_credential_store`.

Both tests now reference the current attribute names. Verified locally:
targeted tests pass, and the full suite is green except the two
pre-existing, environment-specific failures unrelated to this change
(fast_mlsirm unavailable in sandbox; a local tokenizer usage-source
artifact in test_spend_analytics.py).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
@seonghobae seonghobae added priority: medium Normal-priority or P2 work status: needs-review Open pull request requiring current-head review or checks type: maintenance Maintenance, build, dependency, or operational upkeep labels Sep 2, 2026 — with ChatGPT Codex Connector

Copy link
Copy Markdown
Contributor Author

Brought this branch current with main (it was stuck behind against the stale base 8839081, abandoned days ago while main advanced to 212ff43+).

  • CI checks on the pre-merge head were all queued/skipped/cancelled — a review-infrastructure artifact of the stale/superseded head, not a real code failure.
  • git merge origin/main --no-edit: clean auto-merge, no conflicts.
  • Verified locally in a fresh venv (pip install --require-hashes -r requirements.lock && pip install --no-deps -e .):
    • This PR's own changed files + closely related suites (test_credential_naming_contract.py, test_credentials_backends.py, test_pii_protection.py, test_chat_model_capability_isolation.py, test_model_discovery.py, test_provider_bootstrap*.py, test_provider_catalog_bootstrap*.py, test_provider_catalog_credential_promotion.py): 276 passed
    • Merge-diff-touched suites (test_rater_observation*.py, test_review_gateway*.py, test_admin_contract.py): 65 passed, 1 pre-existing failure (test_admin_contract.py::test_model_group_mutations_refresh_audit_events, NameError: name 'json' is not defined, already present on main HEAD, unrelated to this PR, tracked/fixed separately in fix(admin): repair test_model_group_mutations_refresh_audit_events #1029)
  • Pushed the merge commit directly to fix/credential-semantic-identifiers (no force-push).

Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Autonomous loop note on exact head 06968a5bb22fad6330bd49e5c3ce93262a7fe9cc:

  • noema-review (run 33723688027) failed with HTTP Error 502: Bad Gateway; phase=connecting, duration=590.3s — transient upstream LLM-gateway infra flakiness, not a review verdict. Re-ran the failed job.
  • Full unit and contract suite (run 33723688000) failed with exactly one test: tests/test_provider_embedding_batch_backend.py::test_unknown_tokenizer_byte_bound_never_becomes_recorded_usage[한글🙂é] - KeyError: 'total_tokens'. This is the same known async embedding-batch race condition already root-caused and fixed (test-only) in fix(tests): wait for provider embedding batch completion before assertions #1044 — unrelated to this PR's own diff. Re-ran the failed job.

No source change made here; no action needed beyond the re-runs unless they fail again for a different reason.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance priority: medium Normal-priority or P2 work status: needs-review Open pull request requiring current-head review or checks type: maintenance Maintenance, build, dependency, or operational upkeep

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants