From 8a7656b857c11777311024ec87ff5f5fd19e5f4c Mon Sep 17 00:00:00 2001 From: Shangjie Chen Date: Mon, 29 Jun 2026 15:59:04 -0700 Subject: [PATCH] refactor(ci): Consolidate compliance checks into pre-commit hook Move custom file compliance checks (logger pattern, future annotations, cli imports, mTLS endpoints) from GHA inline bash scripts to a unified python script (compliance_checks.py) and expose it as a local pre-commit hook. Remove the compliance-check job from CI workflow. Co-authored-by: Shangjie Chen PiperOrigin-RevId: 940081615 --- .github/workflows/continuous-integration.yml | 124 ------------ .pre-commit-config.yaml | 5 + .../mcp/mcp_toolset_auth/oauth_mcp_server.py | 2 +- scripts/compliance_checks.py | 178 ++++++++++++++++++ 4 files changed, 184 insertions(+), 125 deletions(-) create mode 100755 scripts/compliance_checks.py diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index 1ae99891..1a13c4c1 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -149,128 +149,4 @@ jobs: --ignore=tests/unittests/artifacts/test_artifact_service.py \ --ignore=tests/unittests/tools/google_api_tool/test_googleapi_to_openapi_converter.py - # 4. Custom file content compliance checks (PR only) - compliance-check: - name: File Content Compliance - runs-on: ubuntu-latest - if: github.event_name == 'pull_request' - steps: - - name: Checkout Code - uses: actions/checkout@v6 - with: - # Fetch full history (depth: 0) instead of shallow clone (depth: 2) to ensure - # git diff origin/${base_ref}...HEAD can reliably find the merge base, - # preventing fatal git errors on deep PRs or when the target branch has progressed. - fetch-depth: 0 - - name: Check for logger pattern in all changed Python files - run: | - git fetch origin ${GITHUB_BASE_REF} - CHANGED_FILES=$(git diff --diff-filter=ACMR --name-only origin/${GITHUB_BASE_REF}...HEAD | grep -E '\.py$' || true) - if [ -n "$CHANGED_FILES" ]; then - echo "Changed Python files to check:" - echo "$CHANGED_FILES" - echo "" - - # Check for 'logger = logging.getLogger(__name__)' in changed .py files. - set +e - FILES_WITH_FORBIDDEN_LOGGER=$(grep -lE 'logger = logging\.getLogger\(__name__\)' $CHANGED_FILES) - GREP_EXIT_CODE=$? - set -e - - if [ $GREP_EXIT_CODE -eq 0 ]; then - echo "❌ Found forbidden use of 'logger = logging.getLogger(__name__)'. Please use 'logger = logging.getLogger('google_adk.' + __name__)' instead." - echo "The following files contain the forbidden pattern:" - echo "$FILES_WITH_FORBIDDEN_LOGGER" - exit 1 - elif [ $GREP_EXIT_CODE -eq 1 ]; then - echo "✅ No instances of 'logger = logging.getLogger(__name__)' found in changed Python files." - fi - else - echo "✅ No relevant Python files found." - fi - - - name: Check for import pattern in certain changed Python files - run: | - git fetch origin ${GITHUB_BASE_REF} - CHANGED_FILES=$(git diff --diff-filter=ACMR --name-only origin/${GITHUB_BASE_REF}...HEAD | grep -E '\.py$' | grep -v -E '__init__.py$|version.py$|tests/.*|contributing/samples/' || true) - if [ -n "$CHANGED_FILES" ]; then - echo "Changed Python files to check:" - echo "$CHANGED_FILES" - echo "" - - # Use grep -L to find files that DO NOT contain the pattern. - FILES_MISSING_IMPORT=$(grep -L 'from __future__ import annotations' $CHANGED_FILES || true) - - if [ -z "$FILES_MISSING_IMPORT" ]; then - echo "✅ All modified Python files include 'from __future__ import annotations'." - exit 0 - else - echo "❌ The following files are missing 'from __future__ import annotations':" - echo "$FILES_MISSING_IMPORT" - echo "This import is required to allow forward references in type annotations without quotes." - exit 1 - fi - else - echo "✅ No relevant Python files found." - fi - - - name: Check for import from cli package in certain changed Python files - run: | - git fetch origin ${GITHUB_BASE_REF} - CHANGED_FILES=$(git diff --diff-filter=ACMR --name-only origin/${GITHUB_BASE_REF}...HEAD | grep -E '\.py$' | grep -v -E 'cli/.*|src/google/adk/tools/apihub_tool/apihub_toolset.py|tests/.*|contributing/samples/' || true) - if [ -n "$CHANGED_FILES" ]; then - echo "Changed Python files to check:" - echo "$CHANGED_FILES" - echo "" - - set +e - FILES_WITH_FORBIDDEN_IMPORT=$(grep -lE '^from.*\bcli\b.*import.*$' $CHANGED_FILES) - GREP_EXIT_CODE=$? - set -e - - if [[ $GREP_EXIT_CODE -eq 0 ]]; then - echo "❌ Do not import from the cli package outside of the cli package. If you need to reuse the code elsewhere, please move the code outside of the cli package." - echo "The following files contain the forbidden pattern:" - echo "$FILES_WITH_FORBIDDEN_IMPORT" - exit 1 - else - echo "✅ No instances of importing from the cli package found in relevant changed Python files." - fi - else - echo "✅ No relevant Python files found." - fi - - - name: Check for hardcoded googleapis.com endpoints - run: | - git fetch origin ${GITHUB_BASE_REF} - CHANGED_FILES=$(git diff --diff-filter=ACMR --name-only origin/${GITHUB_BASE_REF}...HEAD | grep -E '\.py$' || true) - if [ -n "$CHANGED_FILES" ]; then - echo "Checking for hardcoded endpoints in: $CHANGED_FILES" - - # 1. Identify files containing any googleapis.com URL. - set +e - FILES_WITH_ENDPOINTS=$(grep -lE 'https?://[a-zA-Z0-9.-]+\.googleapis\.com' $CHANGED_FILES) - - # 2. From those, identify files that are MISSING the required mTLS version. - if [ -n "$FILES_WITH_ENDPOINTS" ]; then - FILES_MISSING_MTLS=$(grep -L '.mtls.googleapis.com' $FILES_WITH_ENDPOINTS) - fi - set -e - - if [ -n "$FILES_MISSING_MTLS" ]; then - echo "❌ Found hardcoded googleapis.com endpoints without mTLS support." - echo "The following files must define both standard and mTLS (.mtls.googleapis.com) endpoints" - echo "to support dynamic endpoint selection as required by security policy:" - echo "$FILES_MISSING_MTLS" - echo "" - echo "To fix this, please follow these steps:" - echo "1. Initialize an AuthorizedSession with your credentials." - echo "2. Use 'mtls.has_default_client_cert_source() from google-auth' to check for available client certificates." - echo "3. If certificates are present, use 'session.configure_mtls_channel()'." - echo "4. Dynamically select the '.mtls.' variant of the endpoint when mTLS is active." - exit 1 - else - echo "✅ All hardcoded endpoints have corresponding mTLS definitions or no endpoints found." - fi - fi diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4dad3d9b..8241633a 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -56,6 +56,11 @@ repos: language: script files: ^src/google/adk/.*\.py$ pass_filenames: false + - id: compliance-checks + name: ADK Compliance Checks + entry: scripts/compliance_checks.py + language: script + files: \.py$ - repo: https://github.com/executablebooks/mdformat rev: 0.7.22 hooks: diff --git a/contributing/samples/mcp/mcp_toolset_auth/oauth_mcp_server.py b/contributing/samples/mcp/mcp_toolset_auth/oauth_mcp_server.py index d9d76dd0..0862fb19 100644 --- a/contributing/samples/mcp/mcp_toolset_auth/oauth_mcp_server.py +++ b/contributing/samples/mcp/mcp_toolset_auth/oauth_mcp_server.py @@ -33,7 +33,7 @@ from mcp.server.fastmcp import FastMCP import uvicorn logging.basicConfig(level=logging.INFO) -logger = logging.getLogger(__name__) +logger = logging.getLogger('google_adk.' + __name__) # Expected OAuth token for testing VALID_TOKEN = 'test_access_token_12345' diff --git a/scripts/compliance_checks.py b/scripts/compliance_checks.py new file mode 100755 index 00000000..0524ba0e --- /dev/null +++ b/scripts/compliance_checks.py @@ -0,0 +1,178 @@ +#!/usr/bin/env python3 +# Copyright 2026 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Runs compliance checks on ADK source files. + +This script is used as a pre-commit hook and in CI to enforce coding standards. +""" + +import argparse +import os +import re +import sys + +# Legacy files that are temporarily excluded from the mTLS check. +# Do not add new files to this list. All new code must support mTLS. +_EXCLUDED_FROM_MTLS = { + 'contributing/samples/environment_and_skills/e2b_environment/agent.py', + 'contributing/samples/integrations/bigquery_mcp/agent.py', + 'contributing/samples/integrations/bigtable/agent.py', + 'contributing/samples/integrations/data_agent/agent.py', + 'contributing/samples/integrations/gcp_auth/agent.py', + 'contributing/samples/integrations/gcs/agent.py', + 'contributing/samples/integrations/gcs_admin/agent.py', + 'contributing/samples/integrations/integration_connector_euc_agent/agent.py', + 'contributing/samples/integrations/oauth_calendar_agent/agent.py', + 'contributing/samples/integrations/spanner/agent.py', + 'contributing/samples/integrations/spanner_admin/agent.py', + 'contributing/samples/integrations/spanner_rag_agent/agent.py', + 'contributing/samples/mcp/mcp_service_account_agent/agent.py', + 'contributing/samples/models/interactions_api/main.py', + 'contributing/samples/multimodal/static_non_text_content/agent.py', + 'src/google/adk/auth/auth_credential.py', + 'src/google/adk/integrations/api_registry/api_registry.py', + 'src/google/adk/integrations/bigquery/bigquery_credentials.py', + 'src/google/adk/integrations/bigquery/data_insights_tool.py', + 'src/google/adk/integrations/bigquery/metadata_tool.py', + 'src/google/adk/integrations/gcs/gcs_credentials.py', + 'src/google/adk/plugins/bigquery_agent_analytics_plugin.py', + 'src/google/adk/tools/_google_credentials.py', + 'src/google/adk/tools/apihub_tool/clients/apihub_client.py', + 'src/google/adk/tools/application_integration_tool/application_integration_toolset.py', + 'src/google/adk/tools/application_integration_tool/clients/connections_client.py', + 'src/google/adk/tools/application_integration_tool/clients/integration_client.py', + 'src/google/adk/tools/bigtable/bigtable_credentials.py', + 'src/google/adk/tools/data_agent/credentials.py', + 'src/google/adk/tools/data_agent/data_agent_tool.py', + 'src/google/adk/tools/google_api_tool/google_api_toolset.py', + 'src/google/adk/tools/google_api_tool/googleapi_to_openapi_converter.py', + 'src/google/adk/tools/mcp_tool/mcp_session_manager.py', + 'src/google/adk/tools/openapi_tool/auth/auth_helpers.py', + 'src/google/adk/tools/openapi_tool/auth/credential_exchangers/service_account_exchanger.py', + 'src/google/adk/tools/pubsub/pubsub_credentials.py', + 'src/google/adk/tools/spanner/spanner_credentials.py', + 'tests/unittests/auth/test_credential_manager.py', + 'tests/unittests/cli/utils/test_gcp_utils.py', + 'tests/unittests/flows/llm_flows/test_functions_request_euc.py', + 'tests/unittests/integrations/api_registry/test_api_registry.py', + 'tests/unittests/integrations/bigquery/test_bigquery_credentials.py', + 'tests/unittests/tools/apihub_tool/clients/test_apihub_client.py', + 'tests/unittests/tools/application_integration_tool/clients/test_connections_client.py', + 'tests/unittests/tools/application_integration_tool/clients/test_integration_client.py', + 'tests/unittests/tools/application_integration_tool/test_application_integration_toolset.py', + 'tests/unittests/tools/data_agent/test_data_agent_tool.py', + 'tests/unittests/tools/google_api_tool/test_docs_batchupdate.py', + 'tests/unittests/tools/google_api_tool/test_google_api_toolset.py', + 'tests/unittests/tools/google_api_tool/test_googleapi_to_openapi_converter.py', + 'tests/unittests/tools/openapi_tool/auth/credential_exchangers/test_service_account_exchanger.py', + 'tests/unittests/tools/openapi_tool/openapi_spec_parser/test_openapi_toolset.py', + 'tests/unittests/tools/openapi_tool/openapi_spec_parser/test_rest_api_tool.py', + 'tests/unittests/tools/spanner/test_spanner_credentials.py', + 'tests/unittests/tools/test_base_google_credentials_manager.py', + 'tests/unittests/tools/test_google_tool.py', + 'tests/unittests/workflow/utils/test_workflow_hitl_utils.py', +} + + +def check_logger(content: str) -> bool: + # Forbidden: 'logger = logging.getLogger(__name__)' + pattern = re.compile(r'logger\s*=\s*logging\.getLogger\(__name__\)') + return not pattern.search(content) + + +def check_future_annotations(content: str, filename: str) -> bool: + # Exclude: __init__.py, version.py, tests/, contributing/samples/ + if ( + filename.endswith('__init__.py') + or filename.endswith('version.py') + or 'tests/' in filename + or 'contributing/samples/' in filename + ): + return True + return 'from __future__ import annotations' in content + + +def check_cli_import(content: str, filename: str) -> bool: + # Exclude: cli/, apihub_toolset.py, tests/, contributing/samples/ + if ( + 'cli/' in filename + or filename.endswith('apihub_toolset.py') + or 'tests/' in filename + or 'contributing/samples/' in filename + ): + return True + # Pattern: ^from.*\bcli\b.*import.*$ (multiline) + pattern = re.compile(r'^from.*\bcli\b.*import.*$', re.MULTILINE) + return not pattern.search(content) + + +def check_mtls(content: str, filename: str) -> bool: + if filename in _EXCLUDED_FROM_MTLS: + return True + # Pattern for googleapis: https?://[a-zA-Z0-9.-]+\.googleapis\.com + endpoint_pattern = re.compile(r'https?://[a-zA-Z0-9.-]+\.googleapis\.com') + if endpoint_pattern.search(content): + return '.mtls.googleapis.com' in content + return True + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument('files', nargs='*', help='Files to check') + args = parser.parse_args() + + failed = False + for f in args.files: + # Skip directories if they are passed accidentally + if not os.path.isfile(f): + continue + try: + with open(f, 'r', encoding='utf-8') as file: + content = file.read() + except Exception as e: # pylint: disable=broad-except + print(f"Error reading {f}: {e}") + continue + + # Run checks + if not check_logger(content): + print( + f"❌ {f}: Found forbidden use of 'logger = logging.getLogger(__name__)'. " + "Please use 'logger = logging.getLogger(\"google_adk.\" + __name__)' instead." + ) + failed = True + + if not check_future_annotations(content, f): + print(f"❌ {f}: Missing 'from __future__ import annotations'.") + failed = True + + if not check_cli_import(content, f): + print( + f"❌ {f}: Do not import from the cli package outside of the cli package." + ) + failed = True + + if not check_mtls(content, f): + print( + f"❌ {f}: Found hardcoded googleapis.com endpoints without mTLS support." + ) + failed = True + + if failed: + sys.exit(1) + sys.exit(0) + + +if __name__ == '__main__': + main()