Feat/agnosticism

[mosesgameli]

Summary

Describe the problem and the change.

Validation

• uv run ruff check .
• uv run ty check src tests examples scripts
• uv run pytest

Docs

• Docs updated for user-facing behavior changes
• Examples updated if relevant
• API reference regenerated if public docstrings changed

Release Notes

• Commit titles follow Conventional Commits
• Breaking changes are called out explicitly

[Copilot]

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

[mosesgameli]

@copilot review

[Copilot]

@copilot review

Here's a review of the PR changes. All checks pass locally (ruff, ty, pytest — 365 tests green). The overall direction is solid and the architecture is well-structured. Below are the findings, grouped by severity:

---

🔴 Issues (should fix)

1. Unsafe getattr without defaults in observability.py (src/bustan/logger/observability.py:102–106)

controller_cls = getattr(route_contract, "controller_cls")          # raises AttributeError if missing
"route": f"{getattr(route_contract, 'method')} {getattr(route_contract, 'path')}",  # same
"operation": f"{controller_cls.__name__}.{getattr(route_contract, 'handler_name')}",  # same

Line 116 already uses a safe getattr(route_contract, "versions", ()) pattern — lines 102–106 should do the same. An unexpected route_contract type will produce an unhandled AttributeError instead of a graceful fallback.

2. Unsafe getattr without default in adapter.py (src/bustan/platform/http/adapter.py:141)

binding_plan = getattr(route_contract, "binding_plan")  # AttributeError if not present

route_contract.binding_plan is always set on RouteContract dataclass instances, but since the parameter type is RouteContract (not object), using getattr without a default here is inconsistent and masks what should be direct attribute access: route_contract.binding_plan.

---

🟡 Warnings (worth addressing)

3. Breaking lifecycle hook renames not backward-compatible
on_app_startup → on_application_bootstrap and on_app_shutdown → on_application_shutdown are breaking changes for any existing code implementing these hooks. There are no compatibility shims or deprecation warnings. The CHANGELOG doesn't reflect this. Recommend either adding a deprecation fallback in run_lifecycle_stage (trying both old and new names with a warning) or calling this out explicitly in the release notes.

4. compile_adapter_routes is not truly adapter-agnostic (src/bustan/platform/http/adapter.py:89–104)
The function performs isinstance(adapter, StarletteAdapter) and raises TypeError for any other adapter. This defeats the purpose of AbstractHttpAdapter as an extension point. Consider using an abstract method on the adapter (e.g., adapter.compile_routes(...)) instead.

5. ModuleRef.for_module() bypasses DI (src/bustan/addons/module_ref.py:29–32)

scoped = ModuleRef(self._application)  # passes Application to __init__ expecting Annotated[object, Inject(APPLICATION)]
scoped._module_key = ...               # mutates private state after construction

This works in practice because _resolve_application() accepts an Application directly, but it's fragile — it bypasses the DI contract. Consider a @classmethod factory or an internal __init__ overload.

6. ObservabilityHooks._override is mutable class-level state
This acts as a process-level global and can leak between tests. The reset_global() classmethod helps, but it relies on test teardown discipline. Consider using a contextvars.ContextVar or an explicit registry instead.

---

🔵 Nitpicks

7. Optional naming conflicts with typing.Optional
Exporting Optional as a public name (from bustan import Optional) shadows the built-in typing.Optional. The function is correct and follows NestJS conventions, but users doing from bustan import Optional will silently shadow typing.Optional in that scope. Consider OptionalDep or document this explicitly.

8. HttpRequest Protocol imports Starlette types
url: URL, query_params: QueryParams, and form: FormData reference starlette.datastructures types, meaning any adapter implementing the HttpRequest protocol still depends on Starlette. This may be intentional for now, but it's worth noting as it limits true agnosticism.

9. Missing trailing newlines
Several new files are missing a trailing newline: addons/__init__.py, addons/context.py, addons/module_ref.py, platform/http/abstractions.py, logger/observability.py. Standard Python convention (and PEP 8) requires files to end with a newline.

---

Overall: the framework-agnostic refactor is well-designed and the new abstractions (ArgumentsHost, ExecutionContext, AdapterCapabilities, DiscoveryService, ModuleRef) are clean and follow established patterns. The items above are the main blockers/concerns before merging.

[mosesgameli]

@copilot do a thorough review again, be explicit about

• security checks
• input sanitisation and handling
• code smells
• possible request/response pipeline leaks
• unexpected escape hatches
• dirty api surface
• e.t.c

[Copilot]

@copilot do a thorough review again, be explicit about

• security checks
• input sanitisation and handling
  ...

Thorough security and quality review below.

---

🔴 Security Issues

1. Internal error messages leak to HTTP 500 responses

src/bustan/platform/http/execution.py:182

response = HttpResponse.json({"detail": str(exc)}, status_code=500)

This is the fallback path reached when context or resolved_pipeline is None (e.g., an exception during controller instantiation or pipeline setup). Internal stack-trace strings, DI resolution paths, class names, and provider configuration details are sent verbatim in the 500 response body. This is an information disclosure vulnerability — attackers can learn your internal class structure and module topology. The message should be a generic string like "Internal server error" and the exception should be logged server-side.

2. Internal exception detail exposed in ProblemDetails

src/bustan/pipeline/filters.py:140

detail=str(exc) or title,

ProblemDetailsExceptionFilter (the framework's catch-all filter) unconditionally forwards str(exc) as detail in the 500 response for every unhandled exception. Stack trace strings, database connection errors, file path disclosures, secret extraction failures — all propagated. For 500-class errors this must default to a generic message. User-facing 400-class exceptions can reasonably include detail.

3. Throttler bypass via spoofed client IP

src/bustan/security/throttler.py:83

key = f"throttle:{client.host if client is not None else 'unknown'}"

The throttle key is derived from request.client.host, which reflects the TCP connection IP, not the actual client IP behind a proxy/load-balancer. In most cloud deployments the connecting IP is always the LB. Clients behind proxies can trivially bypass the throttler by using different connections. No X-Forwarded-For / X-Real-IP header is respected. This must be documented clearly ("only suitable behind a trusted reverse proxy" or support a configurable IP extractor). A misconfigured deployment could also mean unknown becomes the throttle key, collapsing all requests into a single bucket.

4. _resolve_container uses unguarded getattr on an untrusted state object

src/bustan/testing/overrides.py:48

return cast(Container, getattr(target.state, "bustan_container"))

getattr returns None silently when the attribute is absent; the cast to Container is unsound. If this function is called against an app not bootstrapped with Bustan (e.g., a plain Starlette TestClient), downstream code receives None typed as Container — silent misbehavior. Should raise a descriptive error.

---

🟠 Pipeline / Request-Response Leaks

5. Request-scope provider cache grows unbounded per request

src/bustan/core/ioc/scopes.py:118–124
The per-request provider cache is stored in request.state as a plain dict keyed by (ModuleKey, token). There is no eviction, size cap, or cleanup after a request completes. For long-lived or ASGI-replayed requests this dict stays in memory until the request object is garbage-collected. This is a memory leak when providers hold references back to the container, creating reference cycles.

6. Rate-limit headers set unconditionally after exception path

src/bustan/platform/http/execution.py:184

_apply_rate_limit_headers(request, response)

_apply_rate_limit_headers in the exception path checks hasattr(request.state, "rate_limit_limit") — fine for the success path. But in the failure path (lines 174–191), if the guard raised an exception before the throttler ran (e.g., a prior guard), rate_limit_limit is still absent and headers are silently skipped. This inconsistency isn't harmful but means clients see rate-limit headers only on allowed requests.

7. Application.get() wraps scope_manager.push_application but scope_manager.push_response is never restored

src/bustan/app/application.py:57–66
ApplicationContext.get() correctly pushes/pops the application scope token. However, push_response is called on the ScopeManager in the Starlette adapter request path (execute_http_route) but RESPONSE injection is documented as a public token. There is no path in the adapter code that actually calls push_response, meaning Inject(RESPONSE) would always fail or return None (since active_response defaults to None). This is a silent, broken feature that could mislead users.

8. execute_http_exception doesn't restore application context on exception

src/bustan/platform/http/execution.py:208–252

application_token = container.scope_manager.push_application(application_runtime)
# ...
try:
    ...
finally:
    container.scope_manager.pop_application(application_token)

This finally block appears on line 252, but it only wraps the inner try block starting at line 214. If factory.instantiate() or factory.resolve_pipeline() raises before entering try, the application_token is never popped. Trace the indentation carefully — container.scope_manager.pop_application(application_token) in execute_http_exception is correctly in finally but the scope for execute_http_route starts the application_token before try. In asyncio this leaks context to subsequent coroutines in the same task.

---

🟡 Code Smells and Dirty API Surface

9. co_flags & 0x80 — brittle async detection

src/bustan/platform/http/execution.py:86

is_async_handler=route_contract.route_definition.handler.__code__.co_flags & 0x80 != 0,

CO_COROUTINE = 0x100 in CPython 3.5+; 0x80 is CO_GENERATOR. This check detects generator functions, not async functions. inspect.iscoroutinefunction() — already used correctly elsewhere in the codebase — is the correct and stable approach. The current check will give wrong results for async generator handlers and normal async def in future Python versions.

10. eval() on handler annotation strings with module globals

src/bustan/platform/http/params.py:1039, src/bustan/platform/http/compiler.py:253

resolved_annotations[parameter_name] = eval(annotation, module_globals, local_namespace)

eval() is called with module_globals = sys.modules[cls.__module__].__dict__ — the module's full global namespace including all its imports. While these annotations are developer-written code compiled at startup (not user input), this is still a footgun: any annotation string that happens to reference a callable in the module globals will be called. Using get_type_hints() with include_extras=True (already used in resolver.py) or typing.get_type_hints() would be safer and avoids raw eval. There's also no depth limit on recursive annotation strings.

11. annotation(raw_value) calls arbitrary constructors on user input

src/bustan/platform/http/params.py:840, 890

return annotation(**raw_value_mapping)   # line 840
return annotation(raw_value)             # line 890

When annotation is any isinstance(annotation, type), arbitrary class constructors are called with user-controlled arguments. For UUID, datetime, Decimal, Path etc. this is fine. But if a developer mistakenly annotates a parameter with a class whose constructor has side effects (e.g., opens a file, connects to a database), user-controlled input can trigger it. This is low risk in practice but worth documenting.

12. _no_lifespan escape hatch is a public-facing parameter

src/bustan/app/bootstrap.py:33

def create_app(..., _no_lifespan: bool = False) -> Application:

Leading-underscore convention in Python signals "private," but this parameter is on the main public factory. It disables all startup lifecycle hooks including on_module_init, on_application_bootstrap, and async singleton warming — silently producing an uninitialized application. This should be moved out of the public signature (e.g., use a _testing_options dataclass or remove it from create_app entirely).

13. compile_adapter_routes hardcodes Starlette dispatch

src/bustan/platform/http/adapter.py:89–104 (pre-existing, confirmed still present)

if isinstance(adapter, StarletteAdapter):
    return StarletteAdapterCompiler(...).compile(...)
raise TypeError(f"Unsupported HTTP adapter: {type(adapter).__name__}")

Despite AbstractHttpAdapter being the extension point, route compilation is gated on isinstance checks for StarletteAdapter. Any third-party adapter subclassing AbstractHttpAdapter will hit TypeError at startup. This makes the abstraction a facade with no real openness.

14. ObservabilityHooks.override_global is not reentrant-safe

src/bustan/logger/observability.py:66–67

@classmethod
def override_global(cls, hooks: "ObservabilityHooks") -> None:
    cls._override.set(hooks)

override_global calls ContextVar.set() without saving the token — this means reset_global() sets to None rather than restoring the previous value. In tests that nest override_global calls (e.g., fixture + test), the outer context is lost. The existing scoped_override context manager handles this correctly; override_global should either be removed or document that it is destructive.

15. assert in RequestContext.request and ThrottlerGuard

src/bustan/pipeline/context.py:326, src/bustan/security/throttler.py:73–74

assert request is not None  # context.py
assert route is not None    # throttler.py

Asserts are disabled when Python runs with -O / -OO flags (which some production environments use). These should be replaced with raise RuntimeError(...) or raise GuardRejectedError(...) to guarantee the invariant is checked at runtime regardless of optimization flags.

---

🔵 Nitpicks

16. _resolve_module_node uses untyped getattr

src/bustan/addons/discovery.py:86–87

for node in nodes:
    if getattr(node, "key") == module or getattr(node, "module") is module:

nodes is typed as tuple[object, ...] but all elements are ModuleNode. Direct attribute access with proper typing is cleaner and statically checkable.

17. Snapshot _snapshot_identity loses route specificity

src/bustan/platform/http/registry.py:178–183

def _snapshot_identity(entry: Mapping[str, object]) -> tuple[str, str]:
    return (str(entry["controller"]), str(entry["handler"]))

Two routes on the same controller with the same handler name (one normal, one versioned) collapse to the same identity key in the diff. Versions or path should be included in the identity.

18. Missing trailing newlines on new files (original feedback confirmed still applicable)

addons/__init__.py, addons/context.py, addons/module_ref.py, platform/http/abstractions.py, logger/observability.py — all still missing trailing newlines.

---

Summary: The two highest-priority security items to address before merging are #1 (internal error detail in 500s) and #2 (ProblemDetails leaking str(exc) for server errors). Items #3 (throttler IP bypass) and #9 (co_flags async detection) are also urgent bugs. The rest are hardening or API cleanup items.