Skip to content

Refactor/docs#60

Closed
thealphacubicle wants to merge 21 commits intomainfrom
refactor/docs
Closed

Refactor/docs#60
thealphacubicle wants to merge 21 commits intomainfrom
refactor/docs

Conversation

@thealphacubicle
Copy link
Copy Markdown
Owner

[Copilot is generating a summary...]

Srihari Raman and others added 21 commits March 3, 2026 11:08
* WIP: Socrata support (formatting etc)

* Updated README file with right config

* Updated Socrata isntructions to be more LLM friendly

* Fixed smoke test bug

* Updated smoke test bug

---------

Co-authored-by: Srihari Raman <[email protected]>
* Added SoSQL query support

* Smoke test bug fix

* Removed smoke test

---------

Co-authored-by: Srihari Raman <[email protected]>
* Bug fix

* Ruff fix

---------

Co-authored-by: Srihari Raman <[email protected]>
* Updated config files and deployment scripts (#33)

* Deployed prod MCP

* Added ACM SSL cert

* Updated shell script and staging vars

* Staging tf vars files changed

---------

Co-authored-by: Srihari Raman <[email protected]>

* Generalized boston specific values

---------

Co-authored-by: Srihari Raman <[email protected]>
* Lint fix

* Lint fix

* bug fix

---------

Co-authored-by: Srihari Raman <[email protected]>
* Lint fix

* Security update: removed Lambda entrypoint

* Added DLQ SQS + tagging

* Updated CLI tools for new service additions

* Added transparency CLI commands + tests

---------

Co-authored-by: Srihari Raman <[email protected]>
@thealphacubicle thealphacubicle self-assigned this Apr 7, 2026
Copilot AI review requested due to automatic review settings April 7, 2026 15:06
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR replaces the legacy shell-script workflow with a Typer-based opencontext CLI, expands Terraform infrastructure (tags, X-Ray, DLQ, optional custom domain), and updates docs/tests accordingly to reflect the new developer and deployment experience.

Changes:

  • Add a full-featured opencontext CLI (authenticate/configure/deploy/status/logs/domain/destroy/test/validate/etc.) and corresponding test coverage.
  • Enhance AWS Terraform: add common resource tags, enable X-Ray tracing, add an SQS DLQ, and support optional API Gateway custom domains with ACM outputs.
  • Tighten plugin security: CKAN SQL aggregation input validation, ArcGIS SSRF protection tests, and Socrata discovery scoping adjustments; update documentation and examples to match the new CLI workflow.

Reviewed changes

Copilot reviewed 92 out of 99 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
tests/test_socrata_plugin.py Replace city-specific constants with generic example values in tests.
tests/test_cli_plugin.py Add tests for opencontext plugin list behavior and constants.
tests/test_cli_main_and_utils.py Add tests for CLI app assembly and cli/utils.py helpers.
tests/test_cli_logs.py Add tests for opencontext logs command behavior and fallbacks.
tests/test_cli_domain.py Add tests for custom-domain helper functions and status flows.
tests/test_cli_destroy.py Add tests for interactive destruction guardrails and signatures.
tests/test_cli_deploy.py Add unit tests for deploy validation, plan parsing, packaging, TTY requirement.
tests/test_cli_authenticate.py Add tests for prerequisite checking and auto-install flows.
tests/test_cli_architecture.py Add tests for architecture command output and registration.
tests/test_ckan_plugin.py Add aggregation validation/security test cases.
tests/conftest.py Add boto3/botocore import stubs for test collection.
tests/ckan/test_aggregate_data_security.py New focused SQL-injection guard test suite for CKAN aggregation.
tests/ckan/init.py Package marker for CKAN tests.
tests/arcgis/test_ssrf_protection.py New SSRF guard tests for ArcGIS plugin.
tests/arcgis/init.py Package marker for ArcGIS tests.
terraform/README.md Update Terraform docs to prefer the CLI over scripts.
terraform/bootstrap/variables.tf Remove DynamoDB lock table variable from bootstrap module.
terraform/bootstrap/README.md Update bootstrap docs for S3-only state + CLI configure guidance.
terraform/bootstrap/outputs.tf Remove DynamoDB lock table output.
terraform/bootstrap/main.tf Remove DynamoDB lock table resource; keep S3 bootstrap.
terraform/aws/variables.tf Add custom_domain variable; formatting tweaks.
terraform/aws/outputs.tf Remove lambda URL output; add custom-domain outputs.
terraform/aws/main.tf Add common tags, X-Ray policy/tracing, SQS DLQ, and remove Lambda Function URL.
terraform/aws/custom_domain.tf Add optional ACM + API Gateway custom-domain resources.
terraform/aws/api_gateway.tf Add resource tags for API Gateway and stage.
server/lambda_handler.py Align python-json-logger import style with core/logging_utils.py.
scripts/test_streamable_http.sh Remove legacy shell test script in favor of CLI test command.
scripts/setup-backend.sh Remove legacy backend setup script (superseded by CLI configure).
scripts/README.md Remove scripts documentation (scripts removed/superseded).
scripts/local_server.py Remove legacy local server script (superseded by opencontext serve).
scripts/deploy.sh Remove legacy deploy script (superseded by opencontext deploy).
requirements.txt Update/add runtime deps (aiohttp bump; add requests/pygments/pre-commit).
requirements-dev.txt Add richer test/lint/CLI dev dependencies.
README.md Rewrite Quick Start for CLI-first workflow; add doc links; generalize examples.
pyproject.toml Add CLI entrypoint + packaging; adjust dependencies and author email.
plugins/socrata/plugin.py Add search_context scoping to discovery API calls.
plugins/socrata/config_schema.py Update example URLs in schema descriptions.
plugins/ckan/plugin.py Add identifier/metric validation for aggregate_data inputs.
plugins/arcgis/where_validator.py Add a basic forbidden-keyword WHERE validator (new file).
plugins/arcgis/config_schema.py Add Pydantic config schema for ArcGIS plugin (new file).
plugins/arcgis/init.py Add module docstring (new file).
local_server.py Remove old top-level local server script.
LICENSE Update copyright attribution.
docs/TESTING.md Update local/deployed testing instructions to use CLI commands.
docs/QUICKSTART.md Rewrite quickstart to CLI-first flow (authenticate/configure/deploy/test).
docs/GETTING_STARTED.md Add CLI install + CLI reference; update setup/deploy flows.
docs/FAQ.md Update FAQ to reference CLI usage and broaden plugin list.
docs/DEPLOYMENT.md Update deployment guidance for CLI, new infra components, and endpoints.
docs/CUSTOM_PLUGINS.md Update deploy instructions; clarify DataPlugin method requirements and skeleton.
docs/BUILT_IN_PLUGINS.md Expand to include ArcGIS; improve tool reference tables and SoQL notes.
docs/ARCHITECTURE.md Update architecture overview for CLI + new AWS resources.
custom_plugins/template/plugin_template.py Fix tool prefix example and reduce unused-variable noise in template.
core/validators.py Update “deploy” guidance text to reference opencontext deploy.
config-example.yaml Add ArcGIS + Socrata config templates; generalize examples.
cli/utils.py Add shared CLI utilities (config/tfvars/terraform helpers, friendly_exit, etc.).
cli/main.py Register Typer app and command groups; install opencontext entrypoint.
cli/commands/validate.py Add pre-deploy validation checks and rich output table.
cli/commands/upgrade.py Add upstream merge helper with protected-file handling.
cli/commands/test.py Add HTTP smoke tests for MCP endpoints (ping/init/tools/list/tool call).
cli/commands/status.py Add status display using Terraform outputs and AWS CLI calls.
cli/commands/serve.py Add local aiohttp dev server command (opencontext serve).
cli/commands/plugin.py Add opencontext plugin list command.
cli/commands/logs.py Add CloudWatch log tailing with optional structured “verbose” view.
cli/commands/domain.py Add custom domain status + DNS instructions/email template output.
cli/commands/destroy.py Add interactive, TTY-only terraform destroy wrapper.
cli/commands/cost.py Add CloudWatch-based cost estimation command.
cli/commands/authenticate.py Add prerequisite checker with optional auto-install for uv/awscli.
cli/commands/architecture.py Add rich “architecture diagram” output in terminal.
cli/commands/init.py Package marker for CLI commands.
cli/init.py Package marker for CLI.
CLAUDE.md Add contributor/automation guide including uv/ruff/pytest commands.
.vscode/tasks.json Add workspace tasks for uv sync, tests, linting, and local server.
.gitignore Ignore terraform tfvars (secrets) and claude artifacts; clarify intent.
.github/workflows/release.yml Add pre-release validation job; adjust lambda packaging input config.
.github/workflows/infra.yml Refactor Terraform CI to validate/lint all terraform/* directories.
.github/workflows/ci.yml Split CI into lint/security/test jobs and adjust dependencies.
.devcontainer/devcontainer.json Add devcontainer config with Python/Go/uv + postCreate setup.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +44 to +48
# Install stubs only when the real packages are absent so that environments
# that have boto3 installed (e.g. CI with full deps) continue to use the real
# library.
if "boto3" not in sys.modules:
sys.modules["boto3"] = _make_boto3_stub()
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

The boto3/botocore stubs are installed based on "boto3" not in sys.modules, which will be true even when boto3 is installed but hasn’t been imported yet (pytest loads conftest early). This can unintentionally shadow the real boto3/botocore in environments that have them installed. Prefer checking importability (e.g., importlib.util.find_spec or a try: import boto3 guard) before stubbing.

Copilot uses AI. Check for mistakes.
Comment on lines +31 to +35
command_names = {c.name for c in app.registered_commands}
# Sub-typers are registered separately via add_typer
assert "authenticate" in command_names or True # typer groups differ
# At minimum verify the app object has callable structure
assert callable(app)
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

assert "authenticate" in command_names or True will always pass, so this test doesn’t validate anything. Consider asserting on a stable property instead (e.g., that specific callbacks are present, or that expected command names are registered) without the unconditional or True.

Copilot uses AI. Check for mistakes.
Comment on lines +40 to +41
with open(config_path) as f:
return yaml.safe_load(f)
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

load_config() returns yaml.safe_load(f) directly, which can be None for an empty file. Callers like get_city_name() assume a dict and will crash. Consider returning yaml.safe_load(f) or {} and narrowing the return type accordingly.

Copilot uses AI. Check for mistakes.
body = response.json()
except Exception:
body = response.text
return response.status_code < 500, elapsed_ms, body
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

This treats any HTTP status < 500 as a passing test (including 404/403/401), which can make opencontext test report false positives. Consider requiring a 2xx status for ping/initialize/tools/list, and only allowing expected 4xx for the tool-call smoke test if desired.

Copilot uses AI. Check for mistakes.
domain_text = Text.assemble(
"Custom domain resources are only created when ",
("custom_domain", "italic"),
" is set in the Terraform variables (", ("configure[/italic]", "dim"),
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

The Text.assemble segment ("configure[/italic]", "dim") looks like an unbalanced Rich markup tag and will likely render literally as configure[/italic]. Consider replacing this with a properly styled literal (e.g., ("opencontext configure", "bold")) rather than embedding a closing tag in the text.

Suggested change
" is set in the Terraform variables (", ("configure[/italic]", "dim"),
" is set in the Terraform variables (",
("configure", "dim"),

Copilot uses AI. Check for mistakes.
Comment on lines +186 to +190
["aws", "acm", "list-certificates", "--output", "json"],
capture_output=True, text=True, timeout=15,
)
if result.returncode == 0:
certs = json.loads(result.stdout).get("CertificateSummaryList", [])
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

This validation check uses aws acm list-certificates and then reads cert.get("Status"), but the list API typically doesn’t return Status. Consider switching to: find matching CertificateArn via list-certificates, then call aws acm describe-certificate to retrieve Status (or just validate existence and omit Status).

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +19
tags = {
Name = "${local.lambda_name}-cert"
Environment = var.stage_name
}
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

Tagging here is inconsistent with the local.common_tags pattern used elsewhere in terraform/aws/*.tf. Consider using tags = merge(local.common_tags, { Name = "${local.lambda_name}-cert" }) (or equivalent) so resources are uniformly tagged for cost allocation/filtering.

Copilot uses AI. Check for mistakes.
Comment on lines 1 to 11
# API Gateway REST API
resource "aws_api_gateway_rest_api" "mcp_api" {
name = "${local.lambda_name}-api"
description = "API Gateway for OpenContext MCP Server"

endpoint_configuration {
types = ["REGIONAL"]
}

tags = local.common_tags
}
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

API Gateway usage-plan quotas/throttles only apply to requests made with an API key, but the methods in this module set api_key_required = false and there are no aws_api_gateway_api_key / usage-plan-key resources. This means the configured quota_settings likely won’t be enforced. Consider either (a) requiring an API key and provisioning it (plus documenting how to set it in clients) or (b) removing the usage plan + quota variables/docs and relying solely on stage-level throttling.

Copilot uses AI. Check for mistakes.
Comment on lines 223 to 227
| Endpoint | Auth | Use |
|----------|------|-----|
| API Gateway | Rate limit, quota | Production |
| Lambda Function URL | None | Testing |
| API Gateway | Throttling + usage plan quota | Production |
| Lambda Function URL | None | Testing only |

Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

This doc still lists “Lambda Function URL” as a testing endpoint, but the Terraform changes remove the Lambda Function URL resource/output. Either reintroduce the Lambda Function URL, or update the documentation to reflect that only API Gateway is supported.

Copilot uses AI. Check for mistakes.
Comment on lines +18 to 25
"aiohttp>=3.13.4",
"pygments>=2.20.0",
"requests>=2.33.0",
"pre-commit>=4.5.1",
"boto3>=1.42.83",
"botocore>=1.42.83",
"click>=8.3.1",
]
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

pre-commit is included in the project’s main install dependencies, which makes it a runtime dependency for all installs (including Lambda packaging if using pyproject deps). Since it’s a development tool, consider moving it under the dev optional extra instead of project.dependencies.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants