Skip to content

Feature/527 thread authoriser for serverless - #559

Draft
malithaYL wants to merge 5 commits into
developfrom
feature/527-thread-authoriser-for-serverless
Draft

Feature/527 thread authoriser for serverless#559
malithaYL wants to merge 5 commits into
developfrom
feature/527-thread-authoriser-for-serverless

Conversation

@malithaYL

Copy link
Copy Markdown
Contributor

Description

Implement support for thread cloud provisioning and serverless and ecs authoriser

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update
  • CI/CD update
  • Other (please describe):

Related Issues

Fixes #
Relates to #

Changes Made

  • This contains design.md explaining the design

Testing

  • Unit tests pass locally
  • Integration tests pass locally
  • Manual testing completed
  • New tests added for changes

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Screenshots (if applicable)

Additional Notes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a full spec set for #527 “Thread store deployment support and Authoriser support for serverless and ECS”, covering core (Valkey thread store), AWS Lambda native thread routes + gateway authorizer integration, ECS queue-mode authoriser mounting, and Terraform wiring across AWS/GCP/Azure.

Changes:

  • Add a new design spec (design.md) describing requirements and scope for thread-store provisioning + authorization on AWS serverless/ECS.
  • Add a detailed implementation spec (spec.md) with proposed code structure, config/env contracts, and a testing plan.
  • Add an implementation plan (plan.md) breaking the work into iterations with verification steps.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
docs/specs/527-thread-deployment-authoriser/design.md Requirements-level design for thread-store provisioning + authorization seams.
docs/specs/527-thread-deployment-authoriser/spec.md Detailed implementation spec, including builder/config changes and AWS Lambda/ECS behavior.
docs/specs/527-thread-deployment-authoriser/plan.md Iterative build plan + verification steps for implementing the spec.

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

Comment thread docs/specs/527-thread-deployment-authoriser/spec.md
Comment thread docs/specs/527-thread-deployment-authoriser/spec.md Outdated
Comment thread docs/specs/527-thread-deployment-authoriser/plan.md

@amithad amithad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Review: #527 spec set (design.md / spec.md / plan.md)

Spec-only PR, so the implementation-conformance dimension does not apply. All three documents were reviewed at their stage altitudes, and every path:line claim was checked against current develop.

Overall: a strong, unusually well-evidenced spec set. The design is point-form with one diagram, clear non-goals, and settled open questions; the spec covers the details specs routinely omit (exception scopes, concurrency contract, per-operation cost, config/data compatibility); the plan orders without restating and includes the tests and docs/skills-sync iterations. The large majority of citations verify exactly (api/thread.py mappings, rest_lambda.py dispatch, akauthorizer.py policy lines, ecs_io_handler.py, http.py auto-mount, the AWS state.tf/module lines, GCP/Azure env blocks, integration-test matrix format, #348 design.md:81).

Main issues (inline comments):

  • One real Terraform defect: the gateway_endpoints entries as specified would deploy /api/v1/api/v1/threads and silently drop the {session_id} segment (design.md:141, spec.md:204).
  • The spec set was written against the pre-#541 / pre-#364 base: _ThreadStoreConfig.type no longer has a pattern, ThreadStoreBuilder has no Types StrEnum (it now uses require_extra / _BUILTIN_THREAD_STORES / resolve_dotted), builder-dispatch tests live in test_store_builders.py, and config.py line refs shifted (AKConfig.thread moved from 393 to 580). After rebasing, re-run the ak-dev-write-spec Step 6 citation re-verification pass.
  • Questions: ECS-side route exposure through the containerized HTTP API is unaddressed (design.md:163); GCP/Azure provisioning is in scope here while issue #527 defers it to a follow-up (design.md:173).
  • Suggestions: resource templates should honor the configurable API_BASE_PATH/API_VERSION (spec.md:105); the Azure cosmos-module option 1 is a registry release, not a local edit (spec.md:291); the example's AuthValidator must set a real subject, and the test flow should exercise rename per the issue acceptance (spec.md:314).

Findings count: 3 blocker (one defect spans two documents), 6 suggestion, 2 question.

Already addressed, not re-raised: Copilot's three earlier comments (driver-module citation, memory vs in_memory naming, staged-spec note) are all fixed in the current head.

Summary-level nits:

  • Per ak-dev-write-spec PR guidance, spec-only PRs use a docs: title (e.g. docs: add spec set for #527 thread deployment and authoriser) and mark "Documentation update" rather than "Breaking change"; also fill in Relates to #527.
  • CI is green.

Comment thread docs/specs/527-thread-deployment-authoriser/design.md Outdated
Comment thread docs/specs/527-thread-deployment-authoriser/spec.md Outdated
Comment thread docs/specs/527-thread-deployment-authoriser/spec.md Outdated
Comment thread docs/specs/527-thread-deployment-authoriser/spec.md Outdated
Comment thread docs/specs/527-thread-deployment-authoriser/spec.md Outdated
Comment thread docs/specs/527-thread-deployment-authoriser/spec.md Outdated
Comment thread docs/specs/527-thread-deployment-authoriser/design.md Outdated
Comment thread docs/specs/527-thread-deployment-authoriser/design.md
Comment thread docs/specs/527-thread-deployment-authoriser/design.md
Comment thread docs/specs/527-thread-deployment-authoriser/plan.md Outdated
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.

3 participants