UN-3636 [MISC] Drop the ENVIRONMENT gate on the LLM mock#2191
Conversation
It did not defend the case it was added for. The threat was a worker env block copied out of the test overlay into a real deployment, but the gate was written into that same block, so a copy carries it. Base compose also sets ENVIRONMENT=development on both workers that run the injection, so any deployment derived from it satisfied the gate regardless. That left one real case -- the mock var set alone somewhere that sets no ENVIRONMENT at all -- which holds by accident rather than design, in exchange for depending on a variable nothing else in the codebase reads. What actually guards the hatch is unchanged: it is off unless someone sets UNSTRACT_LLM_MOCK_RESPONSE, and it warns once per process while active. Making mocked spend distinguishable downstream is the defence worth having, and it belongs on the usage record rather than in a config check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WalkthroughLLM mocking no longer depends on ChangesLLM mocking behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Nothing reads it: no service, worker, frontend or plugin looks the variable up, and the one consumer it ever had — the LLM mock gate — was removed in the previous commit. Dropping it everywhere keeps a dead knob from looking load-bearing to the next reader. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ryg9chVDJQggCybpq3YoY3
|
|
| Filename | Overview |
|---|---|
| unstract/sdk1/src/unstract/sdk1/llm.py | Removes the ENVIRONMENT allow-list before LiteLLM mock injection. |
| unstract/sdk1/tests/test_mock_response.py | Removes tests for environment-based refusal while keeping unset, empty, set, override, warning, and completion-path checks. |
| docker/docker-compose.yaml | Removes ENVIRONMENT=development from compose services. |
| tests/compose/docker-compose.test.yaml | Removes ENVIRONMENT=test from test services while keeping mock-response forwarding for the main execute-path workers. |
| tests/README.md | Updates the test documentation for the removed gate and remaining mock behavior. |
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
unstract/sdk1/src/unstract/sdk1/llm.py:55-56
**Mocked Completions Reach Production**
When a real worker has a non-empty `UNSTRACT_LLM_MOCK_RESPONSE`, this path now injects `mock_response` without any runtime gate. All SDK completion paths can then return canned LiteLLM output and persist synthetic token usage as normal usage records, with no in-band mock marker for billing or dashboards to filter.
Reviews (1): Last reviewed commit: "UN-3636 [MISC] Drop the unread ENVIRONME..." | Re-trigger Greptile
Unstract test resultsPer-group results
Critical paths
|
da2a5ce
into
feat/rig-extra-manifests
…TS (#2188) * UN-3636 [FEAT] Merge overlay manifests via UNSTRACT_RIG_EXTRA_MANIFESTS load_groups() now merges extra group manifests listed in the env var (os.pathsep-separated, REPO_ROOT-relative) onto the base tests/groups.yaml before validation, so cross-manifest depends_on and the platform-gate invariant are checked over the union. Name collisions are an error. Lets a downstream repo (the cloud build) contribute its own test groups by copying a groups.cloud.yaml into the merged tree, without editing the OSS manifest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz * UN-3636 [FIX] Skip org lookup when no org in context UserContext.get_organization() ran Organization.objects.get() even with no org id in StateStore (import time, or management commands with no request), catching only DoesNotExist/ProgrammingError — a DB-less/unmigrated setup hit an uncaught OperationalError. Short-circuit when there's no org id: no query, so serializers/managers that reference org-scoped querysets at class-def can be imported during DB-free test collection. * UN-3636 [FIX] Scope overlay manifests to the default manifest Address review feedback on the UNSTRACT_RIG_EXTRA_MANIFESTS overlay: - Overlays now apply only when loading the default manifest, so an explicit `load_groups(path)` (test fixture, ad-hoc manifest) can no longer absorb a downstream repo's ambient overlay. - `_merge_manifest` returns the merged defaults so an overlay can rename `platform_gate_group` instead of having it silently ignored. - A bad path in the env var raises a ValueError naming the variable rather than a bare FileNotFoundError/IsADirectoryError. - Extract `_load_manifest_dict` to single-source manifest parsing and its error message. - Tests: drive the real default-manifest path; cover overlay isolation, overlay defaults, malformed overlay, and a missing overlay path. - Pin the truthy branch of UserContext.get_organization so inverting the guard can't pass unnoticed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz * UN-3636 [FIX] Stop re-running cross-tier deps in every tier leg `--tier X` expanded each selected group's `depends_on` transitively, with no tier bound. `integration-workflow-execution` and `e2e-smoke` both declare `depends_on: [unit-sdk1, unit-workers]`, so those two unit groups ran again in the integration leg and a third time in the e2e leg. Tiers run as separate CI legs and the unit leg already covers them, so dep expansion is now bounded to the requested tier. Explicitly named groups are never dropped, and intra-tier deps (e2e-smoke -> e2e-login) still expand and order as before. Unrun deps do not weaken gating: `blocked_by` intersects with groups that failed in the same run. Measured on the last main run: ~88s of unit-workers and ~48s of unit-sdk1 re-executed per run. On the cloud CI runner the same duplication costs ~350s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz * UN-3636 [MISC] Drop the ENVIRONMENT gate on the LLM mock (#2191) * UN-3636 [FIX] Drop the ENVIRONMENT gate on the LLM mock It did not defend the case it was added for. The threat was a worker env block copied out of the test overlay into a real deployment, but the gate was written into that same block, so a copy carries it. Base compose also sets ENVIRONMENT=development on both workers that run the injection, so any deployment derived from it satisfied the gate regardless. That left one real case -- the mock var set alone somewhere that sets no ENVIRONMENT at all -- which holds by accident rather than design, in exchange for depending on a variable nothing else in the codebase reads. What actually guards the hatch is unchanged: it is off unless someone sets UNSTRACT_LLM_MOCK_RESPONSE, and it warns once per process while active. Making mocked spend distinguishable downstream is the defence worth having, and it belongs on the usage record rather than in a config check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3636 [MISC] Drop the unread ENVIRONMENT variable from compose Nothing reads it: no service, worker, frontend or plugin looks the variable up, and the one consumer it ever had — the LLM mock gate — was removed in the previous commit. Dropping it everywhere keeps a dead knob from looking load-bearing to the next reader. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ryg9chVDJQggCybpq3YoY3 --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * UN-3636 [MISC] Tighten code comments Drop lines that restate the code, trim session-specific detail, and merge comments that duplicated each other across a module and its test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz * UN-3636 [FIX] Gate overlay merging on an omitted path, not its value `load_groups(DEFAULT_MANIFEST)` merged overlays even though the caller named a manifest explicitly, because the check compared path values. Path equality is also spelling-sensitive, so the same file relative and absolute behaved differently. Key on `path is None` instead: an explicit path loads exactly what it names. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ryg9chVDJQggCybpq3YoY3 * UN-3636 [FIX] Stop ide_prompt_complete tests from hitting the network TestIdePromptComplete drives the full success path, which reaches client_plugin_registry.get_client_plugin("subscription_usage"). In OSS no such plugin is installed, so the lookup returns None instantly. In a tree with the cloud plugins copied in, it resolves to a real plugin that POSTs to the backend; with no backend running the call only fails after a multi-second connect timeout, and _track_subscription_usage swallows the error so the tests still pass. That accounted for ~190s of the cloud unit-workers run. The file already declared _PATCH_GET_PLUGIN but never applied it outside the dedicated subscription-usage classes. Apply it as a class-scoped fixture so the lookup is pinned to the OSS answer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* UN-3636 [FEAT] Merge overlay manifests via UNSTRACT_RIG_EXTRA_MANIFESTS load_groups() now merges extra group manifests listed in the env var (os.pathsep-separated, REPO_ROOT-relative) onto the base tests/groups.yaml before validation, so cross-manifest depends_on and the platform-gate invariant are checked over the union. Name collisions are an error. Lets a downstream repo (the cloud build) contribute its own test groups by copying a groups.cloud.yaml into the merged tree, without editing the OSS manifest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz * UN-3636 [FIX] Skip org lookup when no org in context UserContext.get_organization() ran Organization.objects.get() even with no org id in StateStore (import time, or management commands with no request), catching only DoesNotExist/ProgrammingError — a DB-less/unmigrated setup hit an uncaught OperationalError. Short-circuit when there's no org id: no query, so serializers/managers that reference org-scoped querysets at class-def can be imported during DB-free test collection. * UN-3636 [FIX] Scope overlay manifests to the default manifest Address review feedback on the UNSTRACT_RIG_EXTRA_MANIFESTS overlay: - Overlays now apply only when loading the default manifest, so an explicit `load_groups(path)` (test fixture, ad-hoc manifest) can no longer absorb a downstream repo's ambient overlay. - `_merge_manifest` returns the merged defaults so an overlay can rename `platform_gate_group` instead of having it silently ignored. - A bad path in the env var raises a ValueError naming the variable rather than a bare FileNotFoundError/IsADirectoryError. - Extract `_load_manifest_dict` to single-source manifest parsing and its error message. - Tests: drive the real default-manifest path; cover overlay isolation, overlay defaults, malformed overlay, and a missing overlay path. - Pin the truthy branch of UserContext.get_organization so inverting the guard can't pass unnoticed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz * UN-3636 [FIX] Stop re-running cross-tier deps in every tier leg `--tier X` expanded each selected group's `depends_on` transitively, with no tier bound. `integration-workflow-execution` and `e2e-smoke` both declare `depends_on: [unit-sdk1, unit-workers]`, so those two unit groups ran again in the integration leg and a third time in the e2e leg. Tiers run as separate CI legs and the unit leg already covers them, so dep expansion is now bounded to the requested tier. Explicitly named groups are never dropped, and intra-tier deps (e2e-smoke -> e2e-login) still expand and order as before. Unrun deps do not weaken gating: `blocked_by` intersects with groups that failed in the same run. Measured on the last main run: ~88s of unit-workers and ~48s of unit-sdk1 re-executed per run. On the cloud CI runner the same duplication costs ~350s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz * UN-3636 [MISC] Drop the ENVIRONMENT gate on the LLM mock (#2191) * UN-3636 [FIX] Drop the ENVIRONMENT gate on the LLM mock It did not defend the case it was added for. The threat was a worker env block copied out of the test overlay into a real deployment, but the gate was written into that same block, so a copy carries it. Base compose also sets ENVIRONMENT=development on both workers that run the injection, so any deployment derived from it satisfied the gate regardless. That left one real case -- the mock var set alone somewhere that sets no ENVIRONMENT at all -- which holds by accident rather than design, in exchange for depending on a variable nothing else in the codebase reads. What actually guards the hatch is unchanged: it is off unless someone sets UNSTRACT_LLM_MOCK_RESPONSE, and it warns once per process while active. Making mocked spend distinguishable downstream is the defence worth having, and it belongs on the usage record rather than in a config check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * UN-3636 [MISC] Drop the unread ENVIRONMENT variable from compose Nothing reads it: no service, worker, frontend or plugin looks the variable up, and the one consumer it ever had — the LLM mock gate — was removed in the previous commit. Dropping it everywhere keeps a dead knob from looking load-bearing to the next reader. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ryg9chVDJQggCybpq3YoY3 --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * UN-3636 [MISC] Tighten code comments Drop lines that restate the code, trim session-specific detail, and merge comments that duplicated each other across a module and its test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz * UN-3636 [FIX] Gate overlay merging on an omitted path, not its value `load_groups(DEFAULT_MANIFEST)` merged overlays even though the caller named a manifest explicitly, because the check compared path values. Path equality is also spelling-sensitive, so the same file relative and absolute behaved differently. Key on `path is None` instead: an explicit path loads exactly what it names. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ryg9chVDJQggCybpq3YoY3 * UN-3636 [FIX] Stop ide_prompt_complete tests from hitting the network TestIdePromptComplete drives the full success path, which reaches client_plugin_registry.get_client_plugin("subscription_usage"). In OSS no such plugin is installed, so the lookup returns None instantly. In a tree with the cloud plugins copied in, it resolves to a real plugin that POSTs to the backend; with no backend running the call only fails after a multi-second connect timeout, and _track_subscription_usage swallows the error so the tests still pass. That accounted for ~190s of the cloud unit-workers run. The file already declared _PATCH_GET_PLUGIN but never applied it outside the dedicated subscription-usage classes. Apply it as a class-scoped fixture so the lookup is pinned to the OSS answer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz * UN-3636 [PERF] Stop the test rig wasting CPU and hashing time Three independent wins measured on the cloud-merged tree: - `-n auto` collapsed to a single worker on any group shipping psutil, because xdist prefers physical cores there. Resolve the count in the rig instead, capped at 8 to avoid contending on the test database. - `--no-migrations` builds the schema from the models rather than replaying the full migration history once per xdist worker. - Test fixtures were paying 600k-iteration PBKDF2 per seeded user. integration-backend fell from 163s to ~52s with an identical result set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz * UN-3636 [TEST] Cut rig CI overhead and unblock unit-workers-cloud Second performance pass on the test rig: - unit-workers-cloud ran zero tests: its group `PYTHONPATH` overwrote the rig-injected plugin dir, so `-p rig_critical_path` failed to import. Merge the two instead of letting env.update clobber it. - Drop `-s` from backend addopts — it disabled capture and flooded the log. - Persist uv's cache across runs via setup-uv enable-cache, so per-group `uv sync` links from cache instead of refetching. - No-op the real backoff sleeps in the sdk retry tests (~7s -> ~1s); no test asserts on elapsed time. - TestCleanupTasks needs no transaction semantics; TestCase over TransactionTestCase drops the per-test truncate-and-reseed. - Reject a non-mapping `groups:` manifest instead of crashing later. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz * UN-3636 [MISC] Drop stale-prone CI comments --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>



Follow-up to #2170, removing a guard that PR added.
Why
The gate required
ENVIRONMENT ∈ {test, development}on top ofUNSTRACT_LLM_MOCK_RESPONSEbefore the SDK would mock a completion. It was added for this review comment — the hazard being a worker env block copied out of the test overlay into a real deployment, where litellm's synthetic 10/20/30 token usage would flow into the usage tables indistinguishable from real spend.It does not defend that case:
docker/docker-compose.yamlsetsENVIRONMENT=developmenton both workers that run the injection, so any deployment derived from base compose — which is how Unstract is self-hosted — satisfied the gate anyway.That leaves one case it genuinely catches: the mock var set alone somewhere that sets no
ENVIRONMENTat all. That holds by accident (nothing sets the variable in k8s) rather than by design, and it costs a dependency on a variable nothing else in the codebase reads — 16 of its 20 declarations across the compose files are dead.What still guards the hatch
Unchanged: it is off unless someone sets
UNSTRACT_LLM_MOCK_RESPONSE, and it logs a warning once per process while active.The defence worth having is making mocked spend distinguishable downstream — an
is_mockmarker on the usage record, which config checks can't be copy-pasted around. Not in this PR.Changes
unstract/sdk1/src/unstract/sdk1/llm.py— drop_ENVIRONMENT_ENV,_MOCK_ALLOWED_ENVIRONMENTS,_warn_mock_refused, and the gateunstract/sdk1/tests/test_mock_response.py— drop the 4 gate tests and the autouse env fixture (9 tests remain, all passing)tests/compose/docker-compose.test.yaml— drop the twoENVIRONMENT=testlines added for the gatetests/README.md— record why it was removed, so it doesn't get re-added🤖 Generated with Claude Code