chore: import upstream snapshot with attribution
OSV-Scanner (Scheduled) / scan-scheduled (push) Failing after 0s
Create Release / test-gate (push) Has been cancelled
Create Release / release-gate (push) Has been cancelled
Create Release / ci-gate (push) Has been cancelled
Create Release / version-check (push) Has been cancelled
Create Release / e2e-test-gate (push) Has been cancelled
Create Release / responsive-test-gate (push) Has been cancelled
Create Release / compat-test-gate (push) Has been cancelled
Create Release / compose-integration-gate (push) Has been cancelled
Create Release / vulture-gate (push) Has been cancelled
Create Release / build (push) Has been cancelled
Create Release / provenance (push) Has been cancelled
Create Release / prerelease-docker (push) Has been cancelled
Create Release / publish-docker (push) Has been cancelled
Create Release / create-release (push) Has been cancelled
Create Release / cleanup-changelog (push) Has been cancelled
Create Release / trigger-pypi (push) Has been cancelled
Create Release / monitor-pypi (push) Has been cancelled
Create Release / Clean up orphan prerelease tags and signatures (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [research-form] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [research-metrics] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [research-workflow] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [settings-core] (push) Has been cancelled
CodeQL Advanced / Analyze (javascript-typescript) (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [history-news] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [library] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [link-analytics] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [chat-core] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [chat-lifecycle] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [error-benchmark] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [settings-pages] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) (push) Has been cancelled
Docker Tests (Consolidated) / Accessibility Tests (push) Has been cancelled
Docker Tests (Consolidated) / LLM Unit Tests (push) Has been cancelled
Docker Tests (Consolidated) / LLM Example Tests (push) Has been cancelled
Docker Tests (Consolidated) / Production Image Smoke Test (push) Has been cancelled
Docker Tests (Consolidated) / Infrastructure Tests (push) Has been cancelled
OSSF Scorecard / OSSF Security Scorecard Analysis (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [mobile] (push) Has been cancelled
Backwards Compatibility / Verify Encryption Constants (push) Has been cancelled
Backwards Compatibility / PyPI Version Compatibility (push) Has been cancelled
Backwards Compatibility / Database Migration Tests (push) Has been cancelled
CodeQL Advanced / Analyze (python) (push) Has been cancelled
Docker Tests (Consolidated) / detect-changes (push) Has been cancelled
Docker Tests (Consolidated) / Build Test Image (push) Has been cancelled
Docker Tests (Consolidated) / All Pytest Tests + Coverage (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [accessibility] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [api-crud] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [auth-login] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [auth-pages] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [auth-register] (push) Has been cancelled
OSV-Scanner (Scheduled) / scan-scheduled (push) Failing after 0s
Create Release / test-gate (push) Has been cancelled
Create Release / release-gate (push) Has been cancelled
Create Release / ci-gate (push) Has been cancelled
Create Release / version-check (push) Has been cancelled
Create Release / e2e-test-gate (push) Has been cancelled
Create Release / responsive-test-gate (push) Has been cancelled
Create Release / compat-test-gate (push) Has been cancelled
Create Release / compose-integration-gate (push) Has been cancelled
Create Release / vulture-gate (push) Has been cancelled
Create Release / build (push) Has been cancelled
Create Release / provenance (push) Has been cancelled
Create Release / prerelease-docker (push) Has been cancelled
Create Release / publish-docker (push) Has been cancelled
Create Release / create-release (push) Has been cancelled
Create Release / cleanup-changelog (push) Has been cancelled
Create Release / trigger-pypi (push) Has been cancelled
Create Release / monitor-pypi (push) Has been cancelled
Create Release / Clean up orphan prerelease tags and signatures (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [research-form] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [research-metrics] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [research-workflow] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [settings-core] (push) Has been cancelled
CodeQL Advanced / Analyze (javascript-typescript) (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [history-news] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [library] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [link-analytics] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [chat-core] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [chat-lifecycle] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [error-benchmark] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [settings-pages] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) (push) Has been cancelled
Docker Tests (Consolidated) / Accessibility Tests (push) Has been cancelled
Docker Tests (Consolidated) / LLM Unit Tests (push) Has been cancelled
Docker Tests (Consolidated) / LLM Example Tests (push) Has been cancelled
Docker Tests (Consolidated) / Production Image Smoke Test (push) Has been cancelled
Docker Tests (Consolidated) / Infrastructure Tests (push) Has been cancelled
OSSF Scorecard / OSSF Security Scorecard Analysis (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [mobile] (push) Has been cancelled
Backwards Compatibility / Verify Encryption Constants (push) Has been cancelled
Backwards Compatibility / PyPI Version Compatibility (push) Has been cancelled
Backwards Compatibility / Database Migration Tests (push) Has been cancelled
CodeQL Advanced / Analyze (python) (push) Has been cancelled
Docker Tests (Consolidated) / detect-changes (push) Has been cancelled
Docker Tests (Consolidated) / Build Test Image (push) Has been cancelled
Docker Tests (Consolidated) / All Pytest Tests + Coverage (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [accessibility] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [api-crud] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [auth-login] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [auth-pages] (push) Has been cancelled
Docker Tests (Consolidated) / UI Tests (Puppeteer) [auth-register] (push) Has been cancelled
This commit is contained in:
@@ -0,0 +1,60 @@
|
||||
# ADR-0001: Remove detect-secrets in favor of gitleaks
|
||||
|
||||
**Date:** 2026-02-28
|
||||
**Status:** Accepted
|
||||
|
||||
## Context
|
||||
|
||||
The repository used two pre-commit secret scanners:
|
||||
|
||||
1. **detect-secrets** (Yelp) — entropy + pattern analysis, tracking false positives
|
||||
in a `.secrets.baseline` JSON file (173 KB, ~5200 lines)
|
||||
2. **gitleaks** — pattern-based detection with path/regex allowlists in
|
||||
`.gitleaks.toml`
|
||||
|
||||
### Problems with detect-secrets
|
||||
|
||||
detect-secrets tracks known false positives using **line numbers** in
|
||||
`.secrets.baseline`. When any file changes above a flagged line, the baseline
|
||||
shifts and must be regenerated. This caused:
|
||||
|
||||
- **Constant merge conflicts** on `.secrets.baseline` across branches
|
||||
- **Manual re-staging** of the baseline on every commit touching flagged files
|
||||
- **173 KB of PR churn** unrelated to actual secret scanning
|
||||
|
||||
All 34 entropy-only detections in the baseline were verified as false positives
|
||||
(git SHAs, GPG fingerprints, base64 character sets, test fixtures).
|
||||
|
||||
### Why gitleaks is sufficient
|
||||
|
||||
- gitleaks uses **path-based and regex-based** allowlists (`.gitleaks.toml`)
|
||||
that are stable across line changes — no baseline regeneration needed
|
||||
- gitleaks covers every service-specific detector that detect-secrets provides
|
||||
(AWS, GitHub, Stripe, Slack, etc.)
|
||||
- gitleaks runs as both a **pre-commit hook** and in **CI workflows**
|
||||
(`gitleaks.yml` on PRs, `gitleaks-main.yml` as release gate)
|
||||
- Additional CI coverage from **Semgrep** (`p/secrets`) and **Bearer**
|
||||
(`scanner: sast,secrets`) in the release gate
|
||||
|
||||
## Decision
|
||||
|
||||
Remove detect-secrets entirely:
|
||||
|
||||
- Delete the `Yelp/detect-secrets` hook from `.pre-commit-config.yaml`
|
||||
- Delete `.secrets.baseline`
|
||||
- Clean up references in `.gitleaks.toml`, `.gitleaksignore`, `CODEOWNERS`,
|
||||
`danger-zone-alert.yml`, `SECURITY_REVIEW_PROCESS.md`, `SECURITY.md`,
|
||||
and `.file-whitelist.txt`
|
||||
|
||||
**Do not re-add detect-secrets.** Use `.gitleaks.toml` allowlists for managing
|
||||
false positives instead.
|
||||
|
||||
## Consequences
|
||||
|
||||
- Eliminates merge conflicts and churn from `.secrets.baseline`
|
||||
- Simplifies the pre-commit pipeline (one secret scanner instead of two)
|
||||
- False positives are managed via `.gitleaks.toml` path/regex allowlists and
|
||||
`.gitleaksignore` commit fingerprints, both of which are stable across
|
||||
line-number changes
|
||||
- No loss of detection coverage — gitleaks + Semgrep + Bearer provide
|
||||
equivalent or better coverage
|
||||
@@ -0,0 +1,102 @@
|
||||
# ADR-0002: Pre-commit hook batch review
|
||||
|
||||
**Date:** 2026-03-28
|
||||
**Status:** Accepted
|
||||
|
||||
## Context
|
||||
|
||||
Nine PRs proposing new or modified pre-commit hooks were evaluated
|
||||
simultaneously. The repository already has 43 pre-commit hooks; new hooks must
|
||||
clear a high bar for signal-to-noise ratio, speed, and non-duplication with CI.
|
||||
|
||||
Review was conducted using 86 automated review agents across 4 rounds,
|
||||
cross-validating findings and verifying correctness of each recommendation.
|
||||
|
||||
## Decision
|
||||
|
||||
### Accepted (4 PRs)
|
||||
|
||||
| PR | Hook | Fix Applied |
|
||||
|----|------|-------------|
|
||||
| #3220 | fix(security): escape server data in innerHTML | None — merged as-is |
|
||||
| #3221 | fix(config): sync ruff version (v0.14.10 → v0.15.8) | Full `ruff format .` pass to reformat all old-style lambdas |
|
||||
| #3225 | chore(hooks): `raise ... from` enforcement in except blocks | None — merged as-is |
|
||||
| #3219 | chore(hooks): layer-import boundary enforcement | Added `exclude: ^tests/` to prevent false positives on test files |
|
||||
|
||||
### Rejected (5 PRs)
|
||||
|
||||
**#3218 — utcnow callable check:** Directly contradicts the existing
|
||||
`check-utcnow-parens` hook. `sqlalchemy_utc.utcnow` is a `FunctionElement`
|
||||
class, so `utcnow()` creates a SQL expression rendered per-INSERT as
|
||||
`CURRENT_TIMESTAMP`. The existing hook correctly requires parentheses. Having
|
||||
both hooks would block every commit.
|
||||
|
||||
**#3222 — codespell (typo detection):** False-positives on camelCase identifiers
|
||||
are a known, unsolved problem
|
||||
([codespell-project/codespell#196](https://github.com/codespell-project/codespell/issues/196),
|
||||
open since 2017). The tool treats `hasTable`, `doesnt`, `crasher`, and similar
|
||||
code identifiers as misspellings, requiring an ever-growing ignore-words-list.
|
||||
|
||||
**#3227 — innerHTML regex scanner:** The core regex (`INTERPOLATION_RE` using
|
||||
`[^}]+`) truncates at the first `}`, failing on nested braces in template
|
||||
literals. Multiple real JS files are affected: `settings.js:80`,
|
||||
`collection_details.js:210`, `history.js:421,501,503`,
|
||||
`semantic_search.js:197-205`. This causes both false positives and false
|
||||
negatives — unacceptable for a security tool. The project already has ESLint and
|
||||
Bearer for JavaScript security analysis, both of which use proper AST-based
|
||||
parsing. Use ESLint `no-unsanitized` rule instead.
|
||||
|
||||
**#3230 — vulture (dead code detection):** The hook used `language: system`,
|
||||
which requires vulture on the system PATH and breaks in CI (the pre-commit
|
||||
workflow does not install project dev dependencies). The larger issue is policy:
|
||||
vulture is advisory-only in CI (release gate, not PR gate). Promoting it to a
|
||||
blocking pre-commit hook is a policy escalation that should be a deliberate team
|
||||
decision. Use the existing `vulture-dead-code.yml` CI workflow instead.
|
||||
|
||||
**#3231 — mypy (type checking):** Most production modules have
|
||||
`ignore_errors = true` in `pyproject.toml` (`web.*`, `database.*`, `settings.*`,
|
||||
`utilities.*`, `security.*`, `api.*`, and 8+ more). mypy already blocks PRs via
|
||||
`mypy-type-check.yml` + `ci-gate.yml`. Adding it as a pre-commit hook would add
|
||||
10-30 seconds per commit for near-zero detection value on the majority of the
|
||||
codebase.
|
||||
|
||||
## Consequences
|
||||
|
||||
- Four new hooks strengthen the pre-commit pipeline (XSS prevention, ruff
|
||||
consistency, exception chaining, architectural boundaries)
|
||||
- Five proposals are documented as rejected with specific technical reasons,
|
||||
preventing re-proposals without new information
|
||||
- Rejected hooks are listed in `.pre-commit-config.yaml` comments with a link
|
||||
to this document
|
||||
- The total hook count increases from 43 to 45, with negligible performance
|
||||
impact (~100-200ms per commit for the new hooks)
|
||||
|
||||
## Principles for future hook proposals
|
||||
|
||||
1. **Fast**: target under 3 seconds for a typical staged-file set.
|
||||
2. **Scoped appropriately**: file-scoped hooks are preferred. Hooks requiring
|
||||
full-repo analysis need a narrow `files:` trigger and team consensus.
|
||||
3. **High local value**: don't add slow CI checks to pre-commit when the local
|
||||
feedback value is low (e.g., mypy with most modules ignored).
|
||||
4. **Validated**: run the hook against the existing codebase before proposing.
|
||||
5. **Low false-positive rate**: if suppressions are needed from day one,
|
||||
reconsider whether the hook belongs here.
|
||||
6. **Low false-positive on identifiers**: tools designed for natural language
|
||||
may over-fire on camelCase identifiers, abbreviations, and domain terms.
|
||||
7. **Self-contained**: hooks must work in pre-commit's CI workflow without
|
||||
requiring system-level dependencies (`language: python` with
|
||||
`additional_dependencies`, not `language: system`).
|
||||
|
||||
## Related PRs
|
||||
|
||||
| PR | Title | Decision |
|
||||
|----|-------|----------|
|
||||
| [#3218](https://github.com/LearningCircuit/local-deep-research/pull/3218) | chore(hooks): prevent frozen utcnow() | Rejected — conflicts with existing hook |
|
||||
| [#3219](https://github.com/LearningCircuit/local-deep-research/pull/3219) | chore(hooks): layer-import boundary enforcement | Accepted (fix: `exclude: ^tests/`) |
|
||||
| [#3220](https://github.com/LearningCircuit/local-deep-research/pull/3220) | fix(security): escape server data in innerHTML | Accepted |
|
||||
| [#3221](https://github.com/LearningCircuit/local-deep-research/pull/3221) | fix(config): sync ruff version | Accepted (fix: full `ruff format .`) |
|
||||
| [#3222](https://github.com/LearningCircuit/local-deep-research/pull/3222) | chore(hooks): add codespell typo detection | Rejected — CamelCase false positives |
|
||||
| [#3225](https://github.com/LearningCircuit/local-deep-research/pull/3225) | chore(hooks): `raise ... from` in except blocks | Accepted |
|
||||
| [#3227](https://github.com/LearningCircuit/local-deep-research/pull/3227) | chore(hooks): detect unescaped innerHTML | Rejected — regex bug |
|
||||
| [#3230](https://github.com/LearningCircuit/local-deep-research/pull/3230) | chore(hooks): vulture dead code detection | Rejected — policy escalation |
|
||||
| [#3231](https://github.com/LearningCircuit/local-deep-research/pull/3231) | chore(hooks): mypy type checking | Rejected — already in CI |
|
||||
@@ -0,0 +1,88 @@
|
||||
# ADR-0003: Reject universal raise-without-from enforcement
|
||||
|
||||
**Date:** 2026-03-28
|
||||
**Status:** Accepted
|
||||
|
||||
## Context
|
||||
|
||||
PR #3225 proposed a pre-commit hook (`check-raise-without-from`) to enforce
|
||||
`raise X(...) from e` inside all `except` blocks, citing PEP 3134 (explicit
|
||||
exception chaining). The hook would flag any `raise NewException(...)` inside
|
||||
an except handler that omits the `from` clause.
|
||||
|
||||
### The conflict with existing security hooks
|
||||
|
||||
The codebase already enforces **exception sanitization** to prevent PII
|
||||
leakage through three mechanisms:
|
||||
|
||||
1. **`check-sensitive-logging.py`** blocks exception variables in
|
||||
`logger.warning/error/critical()` calls and forbids `exc_info=True` on
|
||||
production log levels. Only `logger.exception()` and
|
||||
`logger.debug(..., exc_info=True)` are permitted.
|
||||
|
||||
2. **`fix-exception-logging.py`** auto-removes exception variable references
|
||||
from f-strings and `exc_info=True` from non-debug logs.
|
||||
|
||||
3. **Intentional chain-breaking pattern** used across the codebase: catch a
|
||||
broad exception, log full details with `logger.exception()`, then raise a
|
||||
sanitized application exception *without* `from e`:
|
||||
|
||||
```python
|
||||
except Exception as e:
|
||||
logger.exception("Error getting news feed") # full details logged
|
||||
raise NewsFeedGenerationException(str(e), ...) # no "from e"
|
||||
```
|
||||
|
||||
This pattern appears in `news/api.py`, `notifications/service.py`,
|
||||
`utilities/es_utils.py`, and others. Flask error handlers then return only
|
||||
`error_code` and `message` to the client — never stack traces.
|
||||
|
||||
### Why `raise X from e` is harmful here
|
||||
|
||||
`raise X from e` preserves the full original exception chain at the Python
|
||||
runtime level. Even if logs are sanitized, the chain propagates to:
|
||||
|
||||
- Error tracking services (Sentry, DataDog) that capture `__cause__`
|
||||
- Any downstream handler that logs with `exc_info=True`
|
||||
- Test output and development tooling that prints full tracebacks
|
||||
|
||||
If the original exception contains PII (SQL errors with user data, API
|
||||
responses with auth tokens, file paths with usernames), preserving the chain
|
||||
defeats the sanitization strategy.
|
||||
|
||||
### When `from e` IS appropriate
|
||||
|
||||
Explicit chaining is valuable in infrastructure code where:
|
||||
|
||||
- The original exception contains no user data (e.g., config parsing errors)
|
||||
- Both exceptions will be caught and handled before reaching any boundary
|
||||
- Debugging requires understanding the root cause chain
|
||||
|
||||
The seven existing `from e` usages in the codebase (`mcp/client.py`,
|
||||
`config/llm_config.py`, `security/path_validator.py`, `exporters/`) follow
|
||||
this pattern correctly.
|
||||
|
||||
## Decision
|
||||
|
||||
Do not add a universal `raise-without-from` enforcement hook. The existing
|
||||
pattern of intentional chain-breaking for PII protection takes precedence
|
||||
over PEP 3134 compliance.
|
||||
|
||||
Developers should use their judgment:
|
||||
|
||||
- **Use `raise X from e`** when the original exception is safe to propagate
|
||||
(no user data, internal infrastructure errors)
|
||||
- **Use `raise X from None`** when you want to explicitly suppress the chain
|
||||
and document the intent
|
||||
- **Omit `from`** when wrapping user-facing exceptions to sanitize error
|
||||
details (the current default pattern)
|
||||
|
||||
## Consequences
|
||||
|
||||
- The allowlisted files in the rejected PR do not need remediation
|
||||
- Exception chain decisions remain a code review concern, not an automated
|
||||
enforcement
|
||||
- The existing `check-sensitive-logging` and `fix-exception-logging` hooks
|
||||
remain the primary defense against PII leakage through exceptions
|
||||
- `raise X from None` is available for cases where developers want to
|
||||
explicitly document chain suppression, but is not required
|
||||
@@ -0,0 +1,103 @@
|
||||
# ADR-0004: QueuePool (not NullPool) for SQLCipher databases
|
||||
|
||||
**Date:** 2026-04-13
|
||||
**Status:** Accepted
|
||||
|
||||
## Context
|
||||
|
||||
Each user has their own SQLCipher-encrypted SQLite database, opened at
|
||||
login and closed at logout. Background threads (research workers,
|
||||
metric writers, news scheduler jobs) need database sessions for the
|
||||
same user concurrently with Flask request handlers.
|
||||
|
||||
### Why QueuePool
|
||||
|
||||
SQLCipher's `PRAGMA key` adds ~0.2 ms per connection open. With 20–30
|
||||
queries per page load, NullPool (new connection per checkout) adds a
|
||||
noticeable 4–6 ms overhead vs QueuePool's ~1.5 ms for pool-resident
|
||||
connections.
|
||||
|
||||
### Why not per-thread NullPool engines
|
||||
|
||||
An earlier design maintained a second engine system: one NullPool
|
||||
engine per `(username, thread_id)` in `_thread_engines`, used by
|
||||
background threads for metric writes. This was removed in PR #3441
|
||||
because:
|
||||
|
||||
1. **FD leak.** Each SQLCipher + WAL connection holds 3 file
|
||||
descriptors (main db + WAL + SHM). Orphaned thread engines — left
|
||||
behind when `@thread_cleanup` did not fire — accumulated FDs
|
||||
unboundedly, eventually exhausting the 1024 soft limit and crashing
|
||||
the server with `OSError: [Errno 24] Too many open files`.
|
||||
|
||||
2. **Architectural redundancy.** The per-user QueuePool engine is
|
||||
already created with `check_same_thread=False`, making it safe for
|
||||
background threads. Routing all work through one bounded pool per
|
||||
user keeps FD usage at `pool_size + max_overflow` (currently 60)
|
||||
instead of scaling with background-thread count.
|
||||
|
||||
### SQLCipher + WAL handle leak workaround
|
||||
|
||||
SQLCipher in WAL mode leaks file handles when pooled connections close
|
||||
out of open-order (a known issue with WAL-mode SQLite engines under
|
||||
connection pooling). The cleanup scheduler in `connection_cleanup.py`
|
||||
calls `engine.dispose()` on all per-user engines every 30 minutes,
|
||||
closing all idle pooled connections and resetting handle state. This is
|
||||
a workaround, not a root fix — it limits accumulation to a 30-minute
|
||||
window.
|
||||
|
||||
### Current pool sizing
|
||||
|
||||
```
|
||||
pool_size = 20
|
||||
max_overflow = 40
|
||||
pool_timeout = 10 # seconds; fail fast rather than queue
|
||||
pool_recycle = 3600 # seconds; recycle stale connections
|
||||
pool_pre_ping = True
|
||||
```
|
||||
|
||||
Peak FD usage per user: `(20 + 40) × 2 + 1 = 121` (WAL mode).
|
||||
|
||||
## Decision
|
||||
|
||||
Use a single shared QueuePool engine per user for all threads (request
|
||||
handlers and background workers). Do not maintain per-thread engines.
|
||||
Periodic `dispose()` mitigates the SQLCipher+WAL handle leak.
|
||||
|
||||
## Consequences
|
||||
|
||||
- FD usage is bounded and predictable.
|
||||
- `pool_timeout=10` makes pool exhaustion a loud error rather than a
|
||||
silent deadlock.
|
||||
- The `ParallelConstrainedStrategy` (`max_workers=100`) could
|
||||
theoretically spike past 60 simultaneous checkouts. Sessions are
|
||||
short-lived (millisecond metric writes), so sustained contention is
|
||||
unlikely. Flagged as a known follow-up.
|
||||
|
||||
## Addendum — PR #3487 investigation (2026-04-16)
|
||||
|
||||
PR #3487 proposed skipping the 30-min `engine.dispose()` for any engine
|
||||
with `pool.checkedout() > 0`, citing a claim that dispose orphans
|
||||
checked-out connections and causes torn writes in the post-login bulk
|
||||
settings import. Investigation found:
|
||||
|
||||
- **SA 2.0 source disagrees.** `QueuePool.dispose` only drains idle
|
||||
queue entries (`_pool.get(False)`); `Engine.dispose` replaces the
|
||||
pool via `pool.recreate()`. SA docs: *"Connections that are still
|
||||
checked out will not be closed."* A thread holding a checked-out
|
||||
connection keeps using it until return.
|
||||
- **Real root cause was elsewhere.** The sticky-loop symptom came
|
||||
from the post-login path committing twice
|
||||
(`load_from_defaults_file(commit=True)` then `update_db_version()`
|
||||
with its own commit), which left `app.version` unwritten on any
|
||||
inter-commit failure. Fix: one session, one terminal commit, both
|
||||
calls use `commit=False`. See `web/auth/routes.py` ATOMICITY
|
||||
INVARIANT comment.
|
||||
- **Regression guard:** `tests/database/test_post_login_settings_atomicity.py`
|
||||
locks in both properties — the atomic all-or-nothing write and the
|
||||
SA 2.0 checked-out-survives-dispose contract — so neither can
|
||||
silently regress.
|
||||
|
||||
Future refactors of the cleanup cycle should preserve the
|
||||
checked-out-survival property; do not add a `checkedout()` skip guard
|
||||
without a real reproducer.
|
||||
@@ -0,0 +1,83 @@
|
||||
# ADR-0005: Reject in-app HTTP response compression
|
||||
|
||||
**Date:** 2026-06-28
|
||||
**Status:** Accepted
|
||||
|
||||
## Context
|
||||
|
||||
LDR serves its front-end as Vite-built static bundles (~2 MB JS + ~435 KB CSS)
|
||||
through the custom `app_serve_static` route in `web/app_factory.py`. In a
|
||||
single-process deployment with no reverse proxy these assets are sent
|
||||
**uncompressed**, so a proposal (PR #3158, then a re-scoped PR #4832) added
|
||||
[`flask-compress`](https://github.com/colour-science/flask-compress) to compress
|
||||
them in-process with zstd/brotli/deflate, restricted to static MIME types.
|
||||
|
||||
The change was implemented, adversarially reviewed, and ultimately **rejected**.
|
||||
Both PRs are closed; nothing was merged.
|
||||
|
||||
### Why the benefit is marginal
|
||||
|
||||
- **Localhost is the default and the point of the tool.** Over loopback,
|
||||
transfer is effectively free; compression only spends CPU. Content-hashed
|
||||
bundles are already `immutable`-cached, so a browser fetches them once.
|
||||
- The win only materialises for users self-hosting LDR **remotely, for multiple
|
||||
users, with no reverse proxy** — a small slice of a "Local" research tool.
|
||||
- Those same users should run a reverse proxy anyway (for TLS), and **nginx /
|
||||
Caddy then provide compression *and* a disk-cached compressed artifact for
|
||||
free** — strictly better than re-compressing on every cold request in-process
|
||||
(`flask-compress`'s `COMPRESS_CACHE_BACKEND` defaults to `None`, so there is no
|
||||
compressed-output cache).
|
||||
|
||||
### Why the cost/risk is real
|
||||
|
||||
In-app compression bolts an HTTP-correctness surface onto the app. Adversarial
|
||||
review found, and empirically reproduced:
|
||||
|
||||
- **Malformed `Range`/`206` responses.** `flask-compress` has no range guard:
|
||||
a `Range` request with a compressible `Accept-Encoding` yields a `206` whose
|
||||
`Content-Range` describes the *identity* (uncompressed) coordinates while the
|
||||
body is compressed (e.g. `Content-Range: bytes 0-1023/14000` with a 23-byte
|
||||
zstd body). This is malformed per RFC 9110 and **corrupts conformant slicing
|
||||
intermediaries**: nginx's `slice` module resets the connection
|
||||
("unexpected range in slice response"), CloudFront caches corrupt byte math,
|
||||
and `curl -C -` / download-manager resumes break.
|
||||
- **Varnish mis-serve.** Varnish (default `http_gzip_support=on`) deliberately
|
||||
ignores `Vary: Accept-Encoding`; with the standard "brotli through Varnish"
|
||||
VCL it can cache a brotli body and serve it to a gzip-only client.
|
||||
- **CDN re-opens BREACH.** Cloudflare and Fastly compress `text/html` themselves
|
||||
by default, so behind them the CSRF-token HTML is compressed regardless of an
|
||||
in-app MIME allow-list. (LDR's CSRF token is already per-render masked by
|
||||
Flask-WTF, which is the durable BREACH mitigation — not declining to compress
|
||||
in-process.)
|
||||
- **CPU amplification.** `app_serve_static` is public and `@limiter.exempt`; with
|
||||
no compressed-output cache, a client can force repeated compression of the
|
||||
~2 MB bundle. Pre-change, static serving was a near-zero-CPU file send.
|
||||
|
||||
The benefit accrues to a minority; several of the risks affect *more* setups
|
||||
(anyone behind a slicing CDN, Varnish, or a TLS-terminating CDN). That is a poor
|
||||
trade for a local-first tool.
|
||||
|
||||
## Decision
|
||||
|
||||
**Do not add in-app HTTP response compression to the Flask application.** Do not
|
||||
re-introduce `flask-compress` or an equivalent runtime compressor.
|
||||
|
||||
For remote / multi-user deployments, the guidance is to **front LDR with a
|
||||
reverse proxy** (nginx / Caddy), which handles TLS, compression, and caching —
|
||||
see [Deploying behind a reverse proxy](../deployment/reverse-proxy.md).
|
||||
|
||||
If asset compression is ever genuinely wanted *without* a proxy, use
|
||||
**build-time pre-compression** instead: have Vite emit `.br` / `.gz` artifacts
|
||||
served as plain static files. That has zero per-request CPU cost, adds no runtime
|
||||
dependency, and — because the files are served plainly — avoids the
|
||||
content-negotiation, conditional-request, and byte-range edge cases above.
|
||||
|
||||
## Consequences
|
||||
|
||||
- The codebase keeps a near-zero-CPU static file path and no new dependency
|
||||
(`flask-compress` + transitive `backports-zstd`).
|
||||
- Remote operators get compression from their reverse proxy/CDN, which is where
|
||||
it belongs.
|
||||
- Two of PR #3158's original items were unrelated and **already shipped**:
|
||||
static `Cache-Control` headers (#3185 / #3207) and removal of the duplicate
|
||||
`styles.css` link (#3207). Only the compression idea is rejected here.
|
||||
@@ -0,0 +1,336 @@
|
||||
# ADR-0006: Keep `focused-iteration` minimal — accept the state-leak/citation fixes, defer the `final_max_results` truncation
|
||||
|
||||
**Date:** 2026-06-28 (updated 2026-07-03)
|
||||
**Status:** Accepted
|
||||
**Relates to:** PR #4850 (merged 2026-06-28), issue #4851, #2001 (closed), #2067 (closed), #2716/#2724/#2799 (open), #4420 (merged)
|
||||
|
||||
> **Outcome:** this decision was enacted the same day it was drafted. The
|
||||
> contributor dropped the truncation and added tests for both fixes; #4850
|
||||
> merged on 2026-06-28 as "fix(focused-iteration): reset per-call state and
|
||||
> offset citations across subsections". See the Decision section at the end.
|
||||
|
||||
## Context
|
||||
|
||||
PR #4850 (originally "fix(focused-iteration): resolve state leak, add
|
||||
truncation ceiling and correct citation offset") bundled three independent
|
||||
changes to `advanced_search_system/strategies/focused_iteration_strategy.py`.
|
||||
Two are genuine bug fixes; the third re-introduces a change we have already
|
||||
rejected twice. This ADR records the per-change verdict and the surrounding
|
||||
history so we do not re-litigate it a fourth time.
|
||||
|
||||
`focused-iteration` is one of the five production strategies that remain after
|
||||
#4420 removed ~22 experimental ones (and #4548 later removed `mcp`). It is the documented 96.51%-SimpleQA strategy
|
||||
(see the module docstring), but it is **not** the primary direction anymore: the
|
||||
default strategy is now **`langgraph-agent`**. langgraph-agent is considered the
|
||||
more flexible and stronger-performing option — it can drive multiple tools and
|
||||
adapt its approach per query — and it is the recommended path when there is
|
||||
enough VRAM to run a capable model (or a hosted model is used). `focused-iteration`
|
||||
remains supported and strong for SimpleQA-style factual lookups, but it is a
|
||||
secondary path. So the bar for changing it is "fix real bugs, change behaviour as
|
||||
little as possible" — not "make it converge with `source-based`."
|
||||
|
||||
## The three changes in PR #4850
|
||||
|
||||
### 1. State-leak reset — **ACCEPT**
|
||||
|
||||
```python
|
||||
# top of analyze_topic()
|
||||
self.all_search_results = []
|
||||
self.results_by_iteration = {}
|
||||
```
|
||||
|
||||
In report ("detailed") mode the **same** strategy instance handles every
|
||||
subsection sequentially (`report_generator.py:476-493`, which also pins
|
||||
`max_iterations=1` per subsection). On `main`, `all_search_results` was never
|
||||
reset, so subsection *N* synthesised over subsections 1..*N* combined →
|
||||
unbounded growth → context-window overflow (issue #4851).
|
||||
|
||||
Why we want it: it is the actual root cause. Three independent reviewers
|
||||
confirmed the reset fixes the leak and introduces no new cross-subsection
|
||||
corruption. Residual un-reset state (`explorer.progress`,
|
||||
`findings_repository.documents`) is pre-existing and inert under report mode's
|
||||
1-iteration pin — a possible follow-up, not a blocker. The sibling
|
||||
`source_based_strategy` avoids the class of bug structurally by using a *local*
|
||||
accumulator instead of a `self.` attribute; that is the cleaner long-term shape,
|
||||
but the reset is correct as-is.
|
||||
|
||||
**Completeness check (does the reset drop sources?).** A natural worry is that
|
||||
this change — which also moves the bibliography collection from a per-iteration
|
||||
`all_links_of_system.extend(iteration_results)` *inside* the loop to a single
|
||||
post-loop `all_links_of_system.extend(self.all_search_results)` — could leave the
|
||||
source list incomplete. It does not, because `all_links_of_system` is **not**
|
||||
reset: it is the cumulative, shared list (the *same* object as
|
||||
`search_system.all_links_of_system`, search_system.py:229; the `id()` guard at
|
||||
search_system.py:472-475 prevents the back-compat re-extend from doubling it).
|
||||
Each `analyze_topic` call appends only *its own* accumulated results to that
|
||||
shared list, so across report subsections it accumulates correctly:
|
||||
|
||||
| Step | `all_search_results` | `all_links_of_system` (shared) | offset |
|
||||
|------|----------------------|--------------------------------|--------|
|
||||
| Sub1 | reset→`[A,B,C]` | `[A,B,C]` | 0 → [1,2,3] |
|
||||
| Sub2 | reset→`[D,E]` | `[A,B,C,D,E]` | 3 → [4,5] |
|
||||
| Sub3 | reset→`[F]` | `[A,B,C,D,E,F]` | 5 → [6] |
|
||||
|
||||
On `main` the same scenario also ends with `[A…F]` in `all_links` — the `main`
|
||||
bug was that Sub3's *synthesis* re-ingested A–E (overflow), not that sources went
|
||||
missing. So **on the success path**, with the truncation ceiling (change #2) held
|
||||
high enough never to fire, the PR's `all_links_of_system` contains the same
|
||||
sources as `main` (verified for single-research mode, report mode, and the
|
||||
follow-up delegate path, which propagates the delegate's sources via
|
||||
`result["all_links_of_system"]` in `enhanced_contextual_followup.py:208-244`).
|
||||
Nothing reads `all_links_of_system` mid-loop, so removing the per-iteration extend
|
||||
breaks no incremental reader.
|
||||
|
||||
**One error-path caveat (adversarially verified).** Because the PR's *only*
|
||||
`all_links_of_system.extend(...)` now lives *after* the loop, inside the same
|
||||
`try` that wraps the loop (the catch-all `except Exception` in
|
||||
`analyze_topic`; line 382 as of the #4850 merge), an **uncaught exception mid-loop** — e.g.
|
||||
the unguarded `model.invoke` in question generation (`browsecomp_question.py:96`
|
||||
and `:282`), which fails routinely on LLM rate-limit/timeout — skips that extend.
|
||||
On `main`, the per-iteration extend had already recorded the completed iterations'
|
||||
sources, so a degraded/failed single-research run still surfaces them in its
|
||||
Sources list; on the PR, that failed run surfaces none. This is a real divergence,
|
||||
but only on the *error* path (the run already returns `_create_error_response`),
|
||||
and the PR's all-or-nothing behaviour is arguably *preferable* (no half-populated
|
||||
bibliography from a run that errored out). In report mode it does not arise
|
||||
(`max_iterations=1`: a mid-loop throw means iteration 1 never completed, so
|
||||
neither side accumulated anything). It is **not** the "source list suddenly
|
||||
incomplete" symptom on successful research — that one is change #2 alone (below).
|
||||
|
||||
### 2. `final_max_results` truncation ceiling — **DROP FROM THIS PR (cap is open for separate, benchmark-gated debate)**
|
||||
|
||||
The objection is to the *blind insertion-order slice as shipped in #4850*, not to
|
||||
the idea of capping results in principle. A relevance-aware cap may have merit;
|
||||
the maintainer is open to it as its own PR, decided with a benchmark (see the
|
||||
"standing position" note below).
|
||||
|
||||
```python
|
||||
max_results = int(self.get_setting("search.final_max_results", 100))
|
||||
if len(self.all_search_results) > max_results:
|
||||
self.all_search_results = self.all_search_results[:max_results] # blind, insertion-order
|
||||
```
|
||||
|
||||
Why we do **not** want this:
|
||||
|
||||
- **It keeps the *first* 100, not the *best* 100.** Results accumulate
|
||||
iteration 1→8; the strategy's highest-value work (verification searches gated
|
||||
on `iteration > 3`) lands at the *tail* and is the first thing dropped. The
|
||||
proven sibling applies the *same* setting via
|
||||
`cross_engine_filter.filter_results(..., reorder=True, reindex=True)` —
|
||||
relevance-rank + dedup, then top-N (`source_based_strategy.py:421-431`). A raw
|
||||
`[:100]` slice does neither; the setting's own description even promises
|
||||
"deduplicating and filtering."
|
||||
- **It runs unconditionally on the protected SimpleQA path** (8 iterations),
|
||||
which the docstring asks us to preserve. `main` applies **no** truncation
|
||||
here, so the 96.51% number was measured without it. At default
|
||||
`search.max_results=50` × 5 questions, the 100 ceiling is hit by ~iteration 2,
|
||||
discarding iterations 3–8 from synthesis — an unbenchmarked accuracy risk.
|
||||
- **It is mostly unnecessary for the bug it targets.** Report mode already
|
||||
forces `max_iterations=1`, and change #1 (the reset) already fixes the
|
||||
overflow. The truncation's cost falls almost entirely on the *non-report* path
|
||||
it was not meant to touch.
|
||||
- **It is the one change that actually shrinks the source list.** Per the
|
||||
completeness check under #1, `all_links_of_system` is otherwise complete. But
|
||||
in single-research mode (8 iterations producing, say, 250 results) the slice
|
||||
caps `all_search_results` at 100 *before* the `all_links` extend, so the
|
||||
returned `sources` / Sources section silently drops the other 150 (and keeps
|
||||
the *first* 100 by insertion order, not the best). On `main` all 250 are
|
||||
returned. This is exactly the "source list suddenly incomplete" failure mode —
|
||||
and it is caused by #2, not #1. Dropping #2 restores a complete, correctly
|
||||
numbered source list.
|
||||
- **Minor robustness smell:** `int(get_setting(..., 100))` raises on a
|
||||
present-but-null/non-numeric value, and the surrounding `except Exception`
|
||||
converts that into a generic "Search iteration failed" *after* all search work
|
||||
(reachable via direct DB / programmatic `settings_snapshot`, not the validated
|
||||
UI).
|
||||
|
||||
This is **the third time** this exact change has been proposed:
|
||||
|
||||
- **#2001** — "apply final_max_results filtering in focused_iteration_strategy"
|
||||
(CrossEngineFilter, matching source-based). **Closed.** Maintainer:
|
||||
*"I am not so sure about this change. It is not what the strategy is supposed
|
||||
to be. Not every strategy should do the same thing."* and *"Outdated due to
|
||||
langraph strategy."*
|
||||
- **#2067** — same idea, simpler. **Closed.**
|
||||
- **#4850 (#2 of 3)** — same idea, blunter (`[:N]` instead of the filter).
|
||||
|
||||
**Standing position (open, not a flat no).** The decision for *this PR* is to drop
|
||||
#2; the broader question of whether `focused-iteration` should cap results at all
|
||||
is open for separate debate. Guardrails for that debate: (a) not every strategy
|
||||
needs the same final-cap behaviour; (b) any cap must preserve the *best* sources
|
||||
(relevance-rank/dedup, e.g. harvested from the V2/V3 work below), not drop them by
|
||||
position; and (c) it should be gated on a SimpleQA benchmark proving no
|
||||
regression. Note also that an earlier informal evaluation (maintainer's
|
||||
recollection, details unverified) did not show a clear performance gain from a
|
||||
results cap — a reason to benchmark before committing, not a settled conclusion.
|
||||
|
||||
**Post-merge evidence (2026-06-29).** After #4850 merged (with the truncation
|
||||
dropped), the contributor reported still hitting the context-length error on
|
||||
report mode: the reset fixes the *cross-subsection* leak, but nothing in the
|
||||
strategy accounts for the model's context window within a single call, so a
|
||||
sufficiently large single run can still overflow. That confirms #4851 had two
|
||||
components — the state leak (fixed) and missing context-length awareness (not
|
||||
fixed) — and it strengthens the case for having the capping debate. It does
|
||||
*not* change the verdict on the blind head-slice: a context-aware or
|
||||
relevance-aware mechanism under guardrails (a)–(c) above is the follow-up to
|
||||
propose, not `[:100]`.
|
||||
|
||||
### 3. Citation offset — **ACCEPT**
|
||||
|
||||
```python
|
||||
nr_of_links = total_citation_count_before_this_search # = len(all_links_of_system), was hardcoded 0
|
||||
```
|
||||
|
||||
In report mode each subsection previously re-numbered citations from `[1]`
|
||||
(`nr_of_links=0`), so `[1]` meant different sources in different subsections and
|
||||
desynced from the single shared bibliography that `format_links_to_markdown`
|
||||
builds from `all_links_of_system`. Offsetting by the running link count makes
|
||||
inline citations contiguous and collision-free across subsections, matching the
|
||||
proven `source_based_strategy` invariant (verified by walking subsections 1→3:
|
||||
no off-by-one, gap, or orphan). For single (non-report) research the offset is 0,
|
||||
so citations still start at `[1]`.
|
||||
|
||||
Test coverage: an early revision of #4850 only tested change #1; the merged
|
||||
version covers everything we asked for —
|
||||
`test_does_not_accumulate_results_across_calls` (#1),
|
||||
`test_citation_offset_uses_existing_all_links_count` and
|
||||
`test_report_mode_indices_stay_sequential_across_sections` (#3).
|
||||
|
||||
## Cross-strategy comparison — #1 and #3 were focused-only oversights
|
||||
|
||||
The two fixes are not a cross-cutting change; they bring `focused-iteration` in
|
||||
line with what the other production strategies already do. The contested
|
||||
truncation (#2) matches *neither* sibling.
|
||||
|
||||
| Concern | focused (pre-#4850) | focused (post-#4850) | `langgraph-agent` (default) | `source-based` |
|
||||
|---|---|---|---|---|
|
||||
| **#1** Reset per-subsection working set | ❌ never reset (the leak) | ✅ resets `all_search_results` | ✅ `self.collector.reset()` (`langgraph_agent_strategy.py:788`) | ✅ *local* accumulator — no shared state to leak |
|
||||
| **#3** Citation offset | ❌ `nr_of_links=0` | ✅ `len(all_links_of_system)` | ✅ `nr_of_links = len(self.all_links_of_system)` (`:789`) | ✅ `:178` / `:481` |
|
||||
| **#2** `final_max_results` cap | none | blind `[:100]` | **none** (agent manages its own context) | proper: cross-engine filter `reorder=True, reindex=True` |
|
||||
|
||||
Takeaways:
|
||||
|
||||
- **#1 and #3 fix bugs unique to `focused-iteration`.** `langgraph-agent` and
|
||||
`source-based` already reset per subsection and already offset citations, so the
|
||||
fixes match two independent, already-shipping implementations — strong evidence
|
||||
they are correct.
|
||||
- **#2 is an outlier in both directions.** Only `source-based` caps, and it does so
|
||||
*properly* (relevance reorder + reindex); `langgraph-agent` deliberately doesn't
|
||||
cap. A blind head-slice matches neither — extra evidence to drop it.
|
||||
- Because `langgraph-agent` is the **default**, the #4851 overflow this PR fixes
|
||||
mainly bites users who switched to `focused-iteration` for report mode — which
|
||||
fits the "not the primary investment area" framing.
|
||||
|
||||
## Cross-section context & citation resolution (report mode)
|
||||
|
||||
Resetting the per-subsection working set raises a fair question: does a later
|
||||
subsection still "know" what earlier ones used, and do in-text citations still
|
||||
resolve? Both hold, because the relevant state is **not** what gets reset:
|
||||
|
||||
- **Coherence channel is independent of the reset.** Report generation feeds each
|
||||
subsection a `=== CONTENT ALREADY WRITTEN (DO NOT REPEAT) ===` block built from
|
||||
the previous sections' *generated content* (`report_generator.py:261-297`,
|
||||
`_build_previous_context`; default last 3 sections, char-capped), injected into
|
||||
the subsection query that drives both question generation and synthesis. This
|
||||
survives change #1 untouched. The `main` leak added *raw search results* on top
|
||||
of that — which is the overflow bug, not the coherence mechanism.
|
||||
- **In-text citations still resolve.** Inline `[N]` markers are hyperlinked by
|
||||
index-matching against the structured source list
|
||||
(`text_optimization/citation_formatter.py` `apply_inline_hyperlinks`:
|
||||
`{str(s["index"]): (title, url)}`), and that list is the cumulative, un-reset
|
||||
`all_links_of_system`. With #3 giving each subsection a distinct index range,
|
||||
every `[N]` maps to exactly one bibliography entry. The reset removes sources
|
||||
from the per-subsection *synthesis input*, not from the *source list citations
|
||||
resolve against* — only #2's truncation removes anything from the latter.
|
||||
|
||||
**Known soft nuance (separate follow-up, not a blocker for #4850).**
|
||||
`_build_previous_context` injects prior prose *with its old `[N]` markers intact*
|
||||
(no stripping). After the reset, the current subsection's synthesis LLM sees those
|
||||
numbers but no longer holds their source content (only its own offset-numbered
|
||||
sources). They still resolve globally in the assembled report, and the "do not
|
||||
repeat" instruction discourages echoing them, so this is a quality wrinkle rather
|
||||
than broken links — and it is strictly *less* bad than `main`, where `nr_of_links=0`
|
||||
made the numbers collide across sections. The clean fix (strip inline markers from
|
||||
the context block, or wire a real `previous_knowledge` in place of the hardcoded
|
||||
`""` in `focused_iteration_strategy.py`'s `analyze_followup` call, line 325 as of
|
||||
the #4850 merge) belongs in its own PR.
|
||||
|
||||
## History of focused-iteration (for future readers)
|
||||
|
||||
- **#917** — introduced the strategy (standard + adaptive search).
|
||||
- **#1277** — configurable options (the A/B knobs in `__init__`).
|
||||
- **#1258 / #1282** — robustness (empty-questions ThreadPool crash; instruction
|
||||
injection when adaptive questions disabled).
|
||||
- **#2293** — docs: suggest 10–20 iterations.
|
||||
- **#2001 / #2067** — `final_max_results` truncation. **Both closed** (see above).
|
||||
- **V2/V3/V4 A/B era (#2716 = V2, #2724 = V2+V3 combined, #2799 = V4 — all
|
||||
open):** separate, selectable strategies for benchmarking, leaving V1
|
||||
untouched. V2 adds **URL dedup**; V3 adds **semantic dedup + relevance
|
||||
sorting** (places most-relevant results last for context-window use); V4 swaps
|
||||
in a task-oriented question generator. Merged review-fix PRs from this era:
|
||||
#2797, #2980, #2996.
|
||||
- **#4420 (merged):** removed ~22 experimental strategies + the
|
||||
`show_all_strategies` toggle. Survivors: `source-based`, `focused-iteration`,
|
||||
`focused-iteration-standard`, `topic-organization`, `mcp`, `langgraph-agent`
|
||||
(default). `mcp` was subsequently removed by #4548 (the factory now redirects
|
||||
it to `langgraph-agent`). The V2/V3/V4 strategy files are **not** on `main`.
|
||||
- **#4711 / #4717 / #4729** — DRY refactors (base error-response + citation
|
||||
helpers, `run_parallel_searches`, post-merge cleanups).
|
||||
|
||||
## Open V2/V3/V4 PRs (#2716, #2724, #2799) — keep open, revisit later
|
||||
|
||||
**Status: undecided — left open on purpose.** This ADR records the context for a
|
||||
future revisit rather than recommending closure now.
|
||||
|
||||
Factors to weigh when we come back to them:
|
||||
|
||||
- **Against merging as-is:** exploratory A/B artifacts (created March 2026;
|
||||
785 / 2,406 / 3,687 lines; `mergeable` UNKNOWN — likely heavy conflicts after
|
||||
#4711/#4717/#4729 and #4420); **never benchmarked** against V1 (#2716's
|
||||
SimpleQA checkbox is unchecked; #2724/#2799 don't mention a benchmark at
|
||||
all); and as standalone strategies they would re-add
|
||||
experimental options that #4420 deliberately removed, against the
|
||||
langgraph-first direction.
|
||||
- **Worth keeping for:** the *ideas*, not the parallel strategies. V2's URL dedup
|
||||
and V3's relevance-sorting-before-cap are exactly the "keep the best, not the
|
||||
first" mechanism a future `focused-iteration` cap would need (see change #2).
|
||||
If a cap is ever justified, the cleanest path is to port that — small, targeted,
|
||||
into the single strategy — and benchmark it, rather than reviving three
|
||||
separate strategies.
|
||||
|
||||
No action on these PRs for now; this ADR just makes the trade-offs explicit so
|
||||
the eventual decision is a quick one.
|
||||
|
||||
## Decision
|
||||
|
||||
We asked the contributor to **split**: keep changes #1 (reset) and #3 (citation
|
||||
offset) — both correct, valuable bug fixes — and **drop #2 (truncation)**, plus
|
||||
add the missing tests for #3. The contributor did exactly that, and #4850
|
||||
merged on 2026-06-28 with #1 + #3 and tests for both. That shipped the
|
||||
state-leak half of #4851 without reshaping the strategy's synthesis behaviour
|
||||
or re-opening a question we had already closed twice.
|
||||
|
||||
## Consequences
|
||||
|
||||
- `focused-iteration` now resets its per-call working set and numbers citations
|
||||
contiguously across report subsections, matching `langgraph-agent` and
|
||||
`source-based`.
|
||||
- No truncation was added; the 96.51%-SimpleQA synthesis path is
|
||||
behaviourally unchanged.
|
||||
- The context-length half of #4851 remains open (see the post-merge evidence
|
||||
under change #2): any future cap must follow guardrails (a)–(c) —
|
||||
per-strategy justification, keep-the-best (not head-slice), SimpleQA
|
||||
benchmark gate.
|
||||
- Follow-ups (separate PRs):
|
||||
1. **Cross-section context quality** — strip inline `[N]` markers from
|
||||
`_build_previous_context`, and/or wire a real `previous_knowledge` in
|
||||
place of the hardcoded `""` in `focused_iteration_strategy.py`'s
|
||||
`analyze_followup` call; verify with an LLM-judged before/after on a
|
||||
report-mode query (not unit-testable).
|
||||
2. **Optional refactor** — move `focused-iteration` to a *local* accumulator
|
||||
like `source-based`, so the per-call working set cannot leak by
|
||||
construction (removes the need to remember the reset).
|
||||
3. **If a result cap is ever wanted on `focused-iteration`** — port V2/V3's
|
||||
relevance-rank + dedup (not a blind slice), consider context-length
|
||||
awareness per the post-merge evidence, and gate it behind a SimpleQA
|
||||
benchmark.
|
||||
@@ -0,0 +1,302 @@
|
||||
# ADR-0007: Restructure egress around a two-axis data-classification model
|
||||
|
||||
**Date:** 2026-06-28
|
||||
**Status:** Accepted (incremental rollout)
|
||||
|
||||
## The model at a glance
|
||||
|
||||
Every model a run touches (search engine, collection / store, LLM, embeddings)
|
||||
is labelled on two **orthogonal** axes — **Sensitivity** (of the data it
|
||||
*sources*) and **Exposure** (of the *sink* it is). The matrix reads: *a
|
||||
component with this Sensitivity (row) and this Exposure (column) is treated as
|
||||
follows.*
|
||||
|
||||
| | **Contained** sink | **Exposing** sink |
|
||||
|---|---|---|
|
||||
| **Non-sensitive** source | Combines with **anything** — *public collection, local Ollama* | Only alongside **non-sensitive** sources — *public web engine, cloud LLM* |
|
||||
| **Sensitive** source | Only with other **contained** sources; never an exposing sink — *private collection + local LLM* | **By itself only**, with contained (local) inference — *remote monitored store of private data* |
|
||||
|
||||
**The rule in one line:** a run may never let a *sensitive* source reach an
|
||||
*exposing* sink — unless it is explicitly switched to **Permissive** mode (which
|
||||
warns but never blocks). The rest of this document explains why this is the
|
||||
right model, how each component is classified, and how we roll it out.
|
||||
|
||||
## Out of scope: the query text itself
|
||||
|
||||
This model classifies **sources** and **sinks** — not the **question the user
|
||||
types**. A run's query is sent verbatim to whichever search sinks the run uses,
|
||||
so a sensitive question typed against an exposing engine leaves the machine
|
||||
*regardless* of how the sources are labelled. The guardrail cannot inspect or
|
||||
sanitise query intent; that stays the **user's responsibility**. The UI must say
|
||||
so plainly — *we can't protect the content of your questions, so choose sources
|
||||
appropriate to how sensitive your question is.*
|
||||
|
||||
## Context
|
||||
|
||||
The egress guardrail (`src/local_deep_research/security/egress/`, see its
|
||||
`README.md`) currently expresses policy on a **single axis**: a per-run
|
||||
`policy.egress_scope` enum (`adaptive` / `both` / `public_only` /
|
||||
`private_only` / `strict`), refined by a per-collection `is_public` flag and
|
||||
two `require_local` inference toggles.
|
||||
|
||||
This works for the common cases, but it collapses two genuinely independent
|
||||
properties into one word, and that conflation is now blocking us.
|
||||
|
||||
### Problem 1 — sensitivity and exposure are different things, fused into one word
|
||||
|
||||
"public ↔ private" is treated as a single spectrum. Two distinct questions hide
|
||||
inside it:
|
||||
|
||||
- **Is the *data* sensitive?** — a property of a **source** (a collection, a
|
||||
document store).
|
||||
- **Does the *destination* expose data outward?** — a property of a **sink** (a
|
||||
web search engine you send a query to; a cloud LLM you send chunks to).
|
||||
|
||||
A collection marked `is_public` is the clearest symptom. The word implies the
|
||||
data is *published*, but a collection is **always a local store** — searching it
|
||||
never pushes its contents to a search engine. What `is_public` actually
|
||||
authorizes is *cloud inference* on that data. "Public" overstates the exposure
|
||||
axis to describe the sensitivity axis.
|
||||
|
||||
### Problem 2 — dual-risk sources can't be expressed
|
||||
|
||||
Elasticsearch and Paperless break the single axis outright. An Elasticsearch
|
||||
instance can be:
|
||||
|
||||
- a **sensitive data store** (your private corpus) → must not be combined with
|
||||
exposing sinks, **and/or**
|
||||
- a **monitored / external service** (a sink that itself exposes the queries you
|
||||
send it).
|
||||
|
||||
These are opposite risks, and a single `is_local` / `is_public` flag can only
|
||||
pick one. Today the only safety for them is the asymmetric URL fail-up. We
|
||||
genuinely "cannot guarantee anything" for these engines under the current model,
|
||||
so for now they should not be auto-combined with other sources.
|
||||
|
||||
### Problem 3 — `both` is a silent blanket
|
||||
|
||||
`both` overrides every per-source classification at once, with no visual or
|
||||
conceptual signal that protection has been dropped. It is the muddy middle
|
||||
between "classify each source" and "turn the policy off."
|
||||
|
||||
### This is a known model
|
||||
|
||||
The two-axis framing is the standard **Data Loss Prevention (DLP) /
|
||||
information-flow-control** model: classify data by **sensitivity**, classify
|
||||
destinations by **trust / exposure**, forbid sensitive→exposing flows. The
|
||||
enforcement-mode vocabulary is **SELinux's** (`enforcing` / `permissive` /
|
||||
`disabled`). The egress package already borrows XACML / zero-trust PDP–PEP
|
||||
vocabulary, so this is a continuation, not a foreign import.
|
||||
|
||||
## Decision
|
||||
|
||||
Re-base the egress model on **two orthogonal labels**:
|
||||
|
||||
| Axis | Applies to | Values | Question |
|
||||
|---|---|---|---|
|
||||
| **Sensitivity** | sources | `sensitive` / `non-sensitive` | Must this data never leave the machine? |
|
||||
| **Exposure** | sinks | `exposing` / `contained` | Does sending data here put it off-machine? |
|
||||
|
||||
Some components play **both roles** — a search engine is a *sink* for your query
|
||||
and a *source* of results; Elasticsearch is a *source* of data and a *sink* for
|
||||
your query. Each gets a label on every axis it participates in. This is exactly
|
||||
what the single-axis model cannot represent and is the crux of the change.
|
||||
|
||||
**Core invariant:** *sensitive* data must not flow to an *exposing* sink —
|
||||
unless the run is explicitly switched to the escape-hatch mode.
|
||||
|
||||
### Every model carries both labels — classification is per-component
|
||||
|
||||
The classification is computed **per component**, and "component" means *every*
|
||||
model the run touches: each search engine, each collection / store, the LLM, and
|
||||
the embeddings model. A single function —
|
||||
`classify(component, settings) -> (Sensitivity, Exposure)` — becomes the one
|
||||
home for logic scattered today across `_engine_bucket`, the
|
||||
`_CLOUD_LLM_PROVIDERS` set, the per-collection `is_public` lookup, and the URL
|
||||
fail-up.
|
||||
|
||||
| Component (example) | Sensitivity | Exposure |
|
||||
|---|---|---|
|
||||
| Public collection / public web results | non-sensitive | contained |
|
||||
| Private collection (default) | **sensitive** | contained |
|
||||
| Public web / academic engine (Google, arXiv) | non-sensitive | **exposing** (query egress) |
|
||||
| Cloud LLM / embeddings (Anthropic, OpenAI) | non-sensitive | **exposing** |
|
||||
| Local LLM / embeddings (Ollama, sentence-transformers) | non-sensitive | contained |
|
||||
| Paperless / Elasticsearch — local, private | **sensitive** | contained |
|
||||
| Elasticsearch — remote / monitored, private | **sensitive** | **exposing** |
|
||||
|
||||
### The four quadrants — the combination rule
|
||||
|
||||
A run touches a *set* of components. Whether the set is permitted follows
|
||||
directly from the two labels:
|
||||
|
||||
| # | A component that is… | may be combined with | Example |
|
||||
|---|---|---|---|
|
||||
| 1 | non-sensitive, contained | **anything** | public collection; local Ollama |
|
||||
| 2 | **sensitive**, contained | other **contained** sources only (sensitive or not) — **no exposing sink** | private collection + local LLM |
|
||||
| 3 | non-sensitive, **exposing** | **non-sensitive** sources only | cloud LLM; public web engine |
|
||||
| 4 | **sensitive + exposing** | the **only sensitive source** (non-sensitive contained companions are fine), with **contained (local) inference** | remote monitored store holding private data |
|
||||
|
||||
Underlying invariant: **a run may not let a sensitive source reach an exposing
|
||||
sink.** Because an agentic run can turn one engine's results into another
|
||||
engine's query (the LangGraph silent-expansion class already in our threat
|
||||
model), this is enforced over the whole run's component set, not per call.
|
||||
Equivalent phrasing: a run must be **either** all-non-sensitive **or** free of
|
||||
exposing sinks — with a lone quadrant-4 component allowed to run as the **only
|
||||
sensitive source** (non-sensitive contained companions are still fine), since
|
||||
returning its own data to itself is not a *new* leak and its inference is forced
|
||||
contained.
|
||||
|
||||
The escape hatch (**Permissive / "Unprotected"**) means "suspend this invariant
|
||||
for this run, keep warning."
|
||||
|
||||
### Enforcement modes (SELinux naming)
|
||||
|
||||
- **Enforcing** — current behaviour: the invariant is enforced; violating
|
||||
engines/sinks are blocked.
|
||||
- **Permissive ("Unprotected")** — the escape hatch: the policy is still
|
||||
*evaluated* so the warning banners fire, but nothing is blocked. Replaces
|
||||
`both`, with an honest, loud, light-red UI. (User-facing label is an open
|
||||
question — "Unprotected" / "Unrestricted" / "Permissive".)
|
||||
|
||||
### Mapping the current model onto the two axes (migration, not rewrite)
|
||||
|
||||
- collection `is_public=False` → Sensitivity **sensitive**; `is_public=True` →
|
||||
**non-sensitive** (relabel away from "public" — it never *publishes*).
|
||||
- public web / academic engines → Exposure **exposing**.
|
||||
- cloud LLM / cloud embeddings → Exposure **exposing** sinks;
|
||||
`require_local_*` = "forbid exposing inference sinks."
|
||||
- local collections / local LLM (Ollama) → Exposure **contained**.
|
||||
- scopes re-expressed: `private_only` ≈ "no exposing sinks"; `public_only` ≈
|
||||
"exposing sinks allowed, sensitive sources excluded"; `strict` ≈ single
|
||||
source; `adaptive` ≈ infer the run's posture from the primary.
|
||||
- `both` → removed, superseded by per-source classification + Permissive mode.
|
||||
- Elasticsearch / Paperless → declared **sensitive + contained** by default
|
||||
(usable with other contained / local sources — quadrant 2), with the URL
|
||||
fail-up flipping exposure to **exposing** (quadrant 4) when the configured
|
||||
endpoint resolves to a public host. A per-destination trust entry
|
||||
(`policy.trusted_search_engines`) can re-contain a self-hosted instance that
|
||||
happens to sit on a public hostname.
|
||||
|
||||
### Per-destination trust (the "I trust Anthropic" case)
|
||||
|
||||
A user-managed override that re-labels a specific sink **contained** (e.g. a
|
||||
zero-retention Anthropic endpoint, a self-hosted Paperless on a public
|
||||
hostname). This is the Exposure-axis counterpart of promoting a collection on
|
||||
the Sensitivity axis. Out of scope for the first steps; recorded here as its
|
||||
principled home.
|
||||
|
||||
## Consequences
|
||||
|
||||
**Positive**
|
||||
- One coherent model that matches industry DLP / IFC vocabulary.
|
||||
- Dual-risk sources (Elasticsearch / Paperless) become expressible.
|
||||
- Honest labels: promoting a collection is clearly about *inference
|
||||
authorization*, not *publication*.
|
||||
- The escape hatch is explicit and visible, not a silent scope.
|
||||
|
||||
**Negative / risks**
|
||||
- The egress guardrail is security-critical and has already been through two
|
||||
adversarial review rounds. A big-bang rewrite is high blast radius.
|
||||
- Every engine / source / sink needs labels on both axes; some are genuinely
|
||||
ambiguous.
|
||||
|
||||
**Therefore: rewrite incrementally, core-first, behind a test net.** The model
|
||||
is the chosen direction, but the guardrail is security-critical and has already
|
||||
survived two adversarial review rounds — so it lands in stages that each keep
|
||||
the egress test suite green:
|
||||
|
||||
A. **Classification core** (`security/egress/classification.py`): the
|
||||
`Sensitivity` / `Exposure` types, `classify(component, settings)`, and
|
||||
`evaluate_run(components)` implementing the four-quadrant rule — pure, fully
|
||||
unit-tested against the truth table above, **not yet wired in** (zero
|
||||
behaviour change).
|
||||
B. **Wire the PEPs** to the core; re-express the existing scopes in its terms;
|
||||
keep behaviour parity (existing egress tests stay green) while the new
|
||||
capabilities (quadrant 4, dual-risk sources) become reachable.
|
||||
C. **UI**: surface the two axes, add **Permissive / "Unprotected"** mode, remove
|
||||
`both`, make collection labels honest.
|
||||
D. **Per-destination trust** (the Exposure override — the "trust Anthropic"
|
||||
case) and explicit classification for Elasticsearch / Paperless.
|
||||
|
||||
**Status (PR #4882).** All four stages are implemented.
|
||||
|
||||
- **A / B** — classification core (`classification.py`) + resolver
|
||||
(`run_classification.py`) + explicit per-element labels on every engine and
|
||||
provider.
|
||||
- **C** — the `UNPROTECTED` escape hatch (with the SSRF / cloud-metadata
|
||||
invariant preserved *after* the hatch gate), `both` **fully retired** — removed
|
||||
from the selector, existing rows migrated to `adaptive` by migration 0019, and
|
||||
any residual value (env var / queued snapshot / un-migrated DB) coerced to
|
||||
`adaptive` at read time; `EgressScope.BOTH` survives only as the internal
|
||||
resolution result for an unclassifiable ADAPTIVE primary — a
|
||||
non-dismissible "protection disabled" banner, and the **enforcement flip**:
|
||||
the run-start precheck rejects a two-axis denial as defense-in-depth over the
|
||||
scope PEPs (`UNPROTECTED` evaluates permissive; an uncomputable decision
|
||||
**fails closed** — a denying `audit_error` — not open). This runs at the web
|
||||
`/api/start_research` precheck **and** at the shared worker chokepoint in
|
||||
`run_research_process`, so follow-up / chat / queue runs are covered too; only
|
||||
CLI / programmatic callers that bypass both get the scope PEPs alone (which
|
||||
still force local inference under `private_only` / adaptive-private). The
|
||||
precheck classifies the **primary** engine plus the inference providers;
|
||||
widening to the full resolved engine set is a follow-up (see residuals).
|
||||
- **D** — per-destination trust (`policy.trusted_inference_providers` /
|
||||
`policy.trusted_search_engines`) relaxing a trusted off-machine sink to
|
||||
contained, a trust banner, and honest collection-label copy.
|
||||
|
||||
**Before it enforces in a shipped release it must pass a fresh adversarial
|
||||
review** — the enforcement flip is a behaviour change to a security boundary.
|
||||
|
||||
### Known residuals (tracked in issue #4951)
|
||||
|
||||
Four rounds of adversarial review established that the run-start audit is
|
||||
best-effort defense-in-depth, not a completeness guarantee: it re-derives each
|
||||
sink's classification from settings and can diverge from what the run actually
|
||||
connects to. The **default `adaptive` config is protected by the scope PEPs**
|
||||
(a private primary forces local inference), and retiring `both` removes the
|
||||
scope where the audit was the sole guard. These edges remain and are tracked as
|
||||
issues rather than blockers:
|
||||
|
||||
- **Search axis — Elasticsearch `cloud_id`.** The exposure fail-up inspects
|
||||
`hosts`, not `cloud_id`; a sensitive ES store reachable via `cloud_id` on a
|
||||
permissive scope is classified contained. The engine's own
|
||||
`_cloud_id_forbidden_by_scope` covers `private_only` / `strict`.
|
||||
- **Full engine set / agentic expansion.** Enforcement (audit and the scope
|
||||
PEPs' local-inference coupling) keys on the run's *primary* engine; a
|
||||
non-primary sensitive engine pulled in mid-run by expansion is not reflected
|
||||
back into `require_local`.
|
||||
- **Dual-risk store on a public host.** A self-hosted store whose URL resolves
|
||||
public classifies PUBLIC_ONLY, so the scope-level local-inference coupling
|
||||
does not fire (the run-start audit still flags it on web paths).
|
||||
- **Name-keyed trust drift.** `trusted_inference_providers` is keyed by provider
|
||||
name, not the vetted endpoint URL, and is honored only in the audit, not the
|
||||
boundary PEP.
|
||||
- **`LibraryRAGService` direct construction** resolves its primary from the
|
||||
global `search.tool`, so a direct (non-factory) construction can miss the
|
||||
local-embeddings coupling.
|
||||
- **Query text** is out of scope by design (see above).
|
||||
|
||||
## Open questions — resolution
|
||||
|
||||
1. **Deferred.** User-facing axis vocabulary. Internally settled as
|
||||
`Sensitivity{sensitive, non_sensitive}` / `Exposure{contained, exposing}`.
|
||||
The two axes are *not* surfaced as independent UI controls yet — they are
|
||||
expressed through the existing egress-scope selector, the per-collection
|
||||
public flag, and the trust lists. A dedicated two-axis UI is a follow-up.
|
||||
2. **Resolved — Unprotected.** Setting value / label / `EgressScope.UNPROTECTED`
|
||||
(the internal enforcement *mode* is separately `Mode.PERMISSIVE`).
|
||||
3. **Resolved.** Elasticsearch / Paperless default to **sensitive + contained**;
|
||||
the URL fail-up flips exposure to exposing (quadrant 4) on a public endpoint;
|
||||
`policy.trusted_search_engines` is the explicit override.
|
||||
4. **Resolved — no.** The aggregate `library` is always sensitive
|
||||
(`_resolve_collection_is_public` hard-returns private for it).
|
||||
5. **Resolved — settings lists.** `policy.trusted_inference_providers` /
|
||||
`policy.trusted_search_engines` (JSON-list settings, mirroring
|
||||
`allowed_local_hostnames`); no dedicated table.
|
||||
|
||||
## References
|
||||
|
||||
- Egress package: `src/local_deep_research/security/egress/README.md`
|
||||
- SELinux modes — enforcing / permissive / disabled
|
||||
- DLP: data-classification (sensitivity labels) + destination-trust / egress
|
||||
controls (information-flow control)
|
||||
Reference in New Issue
Block a user