342 fixes - #394
Conversation
Signed-off-by: José Luis Castro <jlcastrog99@gmail.com>
Signed-off-by: José Luis Castro <jlcastrog99@gmail.com>
oracle-contributor-agreement
Bot
added
the
OCA Verified
|
Hi @dustin-sale, any update? |
|
@JoseLuisC99 my apologies, I have not had a chance to get to this. Here are some initial changes that are needed:
|
Signed-off-by: José Luis Castro García <jose.lu.castro@oracle.com>
Signed-off-by: José Luis Castro García <jose.lu.castro@oracle.com>
Signed-off-by: José Luis Castro García <jose.lu.castro@oracle.com>
|
@dustin-sale everything is ready. We have modified our approach to align this MCP with our current agent engine and have significantly reduced the number of tools. |
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
This implementation has a promising unified discovery surface and passes its unit-test/coverage gates, but it is not ready to merge.
- PR description needs to be updated with the single oci-db-observability-mcp-server
- Pagination is unusable for most catalog operations
- Unit tests coverage gaps:
- page-token preservation
- strict schema parity
- mutation isolation
- The generic invocation tool also exposes state-changing DBM operations without a separate destructive boundary.
- Most advertised “exact” schemas accept arbitrary or incorrectly typed arguments.
- Documented uvx command does not match the published distribution
Please resolve those contract and safety issues and populate the changelog before another review.
|
Hi @dustin-sale, I have addressed your comments. |
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
Requested changes
src/oci-oracle-db-observability-mcp-server/oracle/oci_oracle_db_observability_mcp_server/metadata/tools.json:247— [P1] Remove or gate state-changing operations from the read-only catalog.- Dependency updates in
pyproject.toml.
Additional review notes
src/oci-oracle-db-observability-mcp-server/oracle/oci_oracle_db_observability_mcp_server/mcp.py:12— [P2] Use the registered MCP tool names in discovery guidance.
Validation
make lintandmake test project=oci-oracle-db-observability-mcp-serverpassed; 31 tests passed with 91.17% coverage.- The wheel builds and current CI is green, but a locked-SDK audit found five state-changing catalog operations marked
mutable: false.
See the inline comments in this review for evidence, impact, and suggested remediation.
|
Hi @dustin-sale, I have addressed your comments. |
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
Requested changes
No blocking changes requested.
Additional review notes
src/oci-oracle-db-observability-mcp-server/oracle/oci_oracle_db_observability_mcp_server/mcp.py:1— [P2] Add the repository-required license headers.src/oci-oracle-db-observability-mcp-server/oracle/oci_oracle_db_observability_mcp_server/metadata/skills.json:5— [P2] Align skill descriptions with the remaining read-only tools.src/oci-oracle-db-observability-mcp-server/pyproject.toml:12— [P3] Refresh the PR description with the updated package dependencys inpyproject.toml.
Validation
- Lint, lock validation, packaging, current CI, and all 39 tests pass with 91.48% coverage.
- An independent locked-SDK audit confirms all 229 catalog operations are GET-only.
See the inline comments in this review for evidence, impact, and suggested remediation.
Signed-off-by: José Luis Castro García <jose.lu.castro@oracle.com>
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
@JoseLuisC99 can you update the PR description to the single entry point and update the file directories to keep it consistent with the package name:
EX: src/oci-db-observability-mcp-server/oracle/oci_db_observability_mcp_server
- removes 'oracle'
|
@dustin-sale fixed |
Signed-off-by: José Luis Castro García <jose.lu.castro@oracle.com>
Add OCI Monitoring metrics and alarm catalog
Add metric catalog coverage tests
|
Hi @dustin-sale, could you review the latest changes again? @jafetcc02 added some new tools required by a few clients. Btw, we’d like to get the remaining approvals, as there are some clients waiting for this MCP 😅 |
dustin-sale
left a comment
dustin-sale
left a comment
There was a problem hiding this comment.
Review by @dustin-sale via Codex.
Requested changes
src/oci-db-observability-mcp-server/oracle/oci_db_observability_mcp_server/metadata/tools.json:15499— [P1] Align the job-status scope with the pinned SDK.src/oci-db-observability-mcp-server/oracle/oci_db_observability_mcp_server/metadata/tools.json:16403— [P1] Enforce valid Monitoring interval and resolution values.
Additional review notes
tests/test_registry.py:150— [P2] Restore SDK schema-parity coverage.runtime.py:234— [P2] Makemax_resultscap what it advertises.metadata/database-and-infra-observability-metrics.json:39— [P3] Correct the collection-interval field name.
Validation
make lintandmake test project=oci-db-observability-mcp-serverpassed; 57 tests passed with 91.92% coverage.- Lock and package builds passed, but a corrected SDK-parity audit found one contract mismatch and
git diff --checkfound an extra blank line at EOF.
See the inline comments in this review for evidence, impact, and suggested remediation.
There was a problem hiding this comment.
Comment from @dustin-sale via Codex.
[P1] Align the job-status scope with the pinned SDK
Evidence: OCI SDK 2.182.1's summarize_job_executions_statuses accepts id, managed_database_id, and managed_database_group_id, but not job_id; the generated method explicitly rejects unknown kwargs. The packaged schema currently accepts both job_id and calls with no resource scope, while invoke_registered_tool forwards those arguments directly to the SDK.
Impact: An agent following the advertised “exact” schema can submit a schema-valid call that fails locally instead of reaching OCI.
Requested change: Remove job_id, describe id as the SDK's job identifier, enforce exactly one of id, managed_database_id, or managed_database_group_id, and add regression coverage for accepted and rejected scope combinations.
There was a problem hiding this comment.
Comment from @dustin-sale via Codex.
[P1] Enforce OCI Monitoring interval constraints
Evidence: OCI Monitoring supports 1m–60m, 1h–24h, or 1d, and requires resolution to be no greater than the query interval. This pattern rejects valid 60m, accepts unsupported 2d–9d, and runtime validation does not check the interval/resolution relationship.
Impact: Valid requests can be blocked by the MCP schema, while invalid schema-approved requests proceed to an avoidable OCI service failure.
Requested change: Accept the documented boundaries, reject durations above 1d, validate resolution <= interval before client creation, and test 60m, 1d, 2d, and inverted interval/resolution pairs.
There was a problem hiding this comment.
Comment from @dustin-sale via Codex.
[P2] Restore schema-parity coverage
Evidence: Existing OCI SDK entries intentionally omit kind and are treated as oci_sdk by the registry, but this condition treats a missing value as non-SDK and skips all 229 of them. The subsequent skill-name exclusion also skips the three standard Monitoring alarm bindings. A corrected audit immediately exposes the unsupported job_id contract.
Impact: This core regression test can pass without checking any of the established OCI SDK schemas.
Requested change: Use the registry default (tool.get("kind", "oci_sdk")), skip only metadata handlers and custom adapters, and either include standard alarm bindings in parity checks or add explicit assertions for any intentionally narrowed contracts.
There was a problem hiding this comment.
Comment from @dustin-sale via Codex.
[P2] Make max_results cap the advertised result
Evidence: response.data is a list of metric streams, and each stream contains its own aggregated_datapoints. Slicing only this outer list means max_results=1 may still return tens of thousands of nested datapoints. The full OCI response is also serialized before the slice occurs.
Impact: The parameter cannot reliably bound MCP response size or protect model context and local memory.
Requested change: Either rename this to max_streams and document its limitations, or cap nested datapoints as well and add a regression test using one stream with multiple datapoints.
There was a problem hiding this comment.
Comment from @dustin-sale via Codex.
[P3] Correct the collection-interval field name
Evidence: Every one of the 245 catalog records uses collectionInternal; no record exposes collectionInterval. These records are returned directly by the search, get, and list handlers.
Impact: Consumers receive a misleading field name for the metric collection cadence.
Requested change: Rename the field to collectionInterval consistently and add a catalog-shape test that locks the output contract.
| [tool.hatch.build.targets.wheel] | ||
| packages = ["oracle"] | ||
| exclude = ["/oracle/**/tests/**"] | ||
|
|
There was a problem hiding this comment.
My Codex review did not highlight this, but we recently added a [tool.hatch.build.targets.sdist] section that excludes a number of files not needed in the package source distribution. Please see oci-cloud-mcp-server/pyproject.toml#L38.
Address monitoring catalog review findings
3 participants
Description
Adds one unified, read-only OCI Database Observability MCP server for Operations Insights (OPSI) and Database Management (DBM).
oracle.oci-db-observability-mcp-serveroracle.oci_db_observability_mcp_serversrc/oci-db-observability-mcp-serverThe server supports STDIO and HTTP streaming transports, OCI authentication through
oracle-mcp-common, and OCI IAM/IDCS request-token authentication for HTTP deployments.Its MCP surface provides:
get_oci_compartmentandlist_oci_compartmentslist_dbo_skills,list_dbo_tools, anddescribe_dbo_toolinvoke_dbo_toolThe catalog includes 34 workflow skills and 229 GET-backed OPSI/DBM operations. Operation schemas are aligned with the locked OCI Python SDK, validate arguments before client construction, and support pagination using
nextPage.Dependencies are declared in
pyproject.toml:fastmcp==3.4.5oci==2.182.1pydantic==2.13.4jsonschema>=4.25.0,<5.0.0oracle-mcp-common>=0.1.2,<0.2.0Fixes #342
Type of change
How Has This Been Tested?
uv run python -m compileall oracleuv run python -m pytest --cov=oracle.oci_db_observability_mcp_server --cov-branch --cov-report=term-missing oracle/oci_db_observability_mcp_server/testsmake lintgit diff --checkTest Configuration:
Checklist