mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-13 10:15:21 +00:00
fix(workflows): redact CEL error values by default; tokenize attachments once
Two follow-ups from PR review.
_fit_attachment_text tokenized the extraction twice on the oversized
path — once for the budget check and once for chars_per_token. That is a
full BPE pass over the whole document (~12ms per 250k chars) on the hot
path of every attachment turn. Compute it once.
The CEL error summarizer kept quoted fragments that looked like
identifiers, on the theory that those are variable names rather than
data. They are not distinguishable: celpy interpolates state values into
messages (`StringType('SECRET')`), and a one-word user query such as
`invoice` is a valid identifier, so it survived redaction and reached
both the log line and — since config errors now bypass
sanitize_api_error — the user. The earlier test passed only because its
sample value contained hyphens.
Quoted fragments are now redacted by default, allowing only the two
positions known to hold names rather than data: the variable in
"undeclared reference to 'x'" and the class in "<class 'ValueError'>".
Tests cover identifier-shaped values, that the exception class survives,
the truncation backstop, and the empty-expression guard.
This commit is contained in:
1 parent
23e249fca1
commit
e196248058
3 files changed
+51
-9
No files matched your search
@@ -52,12 +52,19 @@ def _summarize_cel_error(exc: Exception) -> str:
|
||||
if marker in text:
|
||||
text = text.split(marker, 1)[0]
|
||||
text = text.strip().lstrip("(").strip()
|
||||
# Quoted fragments in celpy messages are either the offending identifier
|
||||
# (safe, and the useful part) or a state value (not ours to echo). Keep
|
||||
# bare identifiers, redact anything else.
|
||||
|
||||
# Redact quoted fragments by DEFAULT. celpy interpolates state values into
|
||||
# messages (``StringType('SECRET')``), and a value that happens to look
|
||||
# like an identifier is indistinguishable from a variable name — so allow
|
||||
# only the two positions that are known to hold names, not data: the
|
||||
# offending variable in "undeclared reference to 'x'", and the exception
|
||||
# class in "<class 'ValueError'>".
|
||||
def _redact(match: "re.Match[str]") -> str:
|
||||
inner = match.group(1)
|
||||
return f"'{inner}'" if inner.isidentifier() else "'…'"
|
||||
prefix = text[max(0, match.start() - 14):match.start()]
|
||||
if prefix.endswith("reference to ") or prefix.endswith("<class "):
|
||||
return f"'{inner}'"
|
||||
return "'…'"
|
||||
|
||||
text = re.sub(r"'([^']*)'", _redact, text)
|
||||
text = text.strip().strip('"').strip()
|
||||
|
||||
@@ -1742,9 +1742,12 @@ class OpenAILLM(BaseLLM):
|
||||
# Half the window: the prompt, history and the answer share the rest.
|
||||
budget = int(limit * 0.5) if limit else 24000
|
||||
try:
|
||||
if num_tokens_from_string(content) <= budget:
|
||||
# Tokenize once: this is a full BPE pass over the whole extraction
|
||||
# (~12ms per 250k chars), on the hot path of every attachment turn.
|
||||
token_count = num_tokens_from_string(content)
|
||||
if token_count <= budget:
|
||||
return content
|
||||
chars_per_token = len(content) / max(num_tokens_from_string(content), 1)
|
||||
chars_per_token = len(content) / max(token_count, 1)
|
||||
keep = max(int(budget * chars_per_token * 0.95), 0)
|
||||
except Exception:
|
||||
keep = budget * 4
|
||||
|
||||
@@ -204,12 +204,44 @@ class TestCelErrorMessages:
|
||||
assert "NameContainer" not in message
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_error_does_not_echo_state_values(self):
|
||||
@pytest.mark.parametrize(
|
||||
"secret",
|
||||
[
|
||||
"SUPER-SECRET-VALUE",
|
||||
# An identifier-shaped value is indistinguishable from a variable
|
||||
# name, so it must be redacted too — this is the realistic case
|
||||
# (a one-word user query) and the earlier redaction let it through.
|
||||
"invoice",
|
||||
"SECRET",
|
||||
],
|
||||
)
|
||||
def test_error_does_not_echo_state_values(self, secret):
|
||||
"""Config errors skip sanitize_api_error now, so they must not carry
|
||||
state contents — a shared agent's runner is not its owner."""
|
||||
with pytest.raises(CelEvaluationError) as exc:
|
||||
evaluate_cel("int(query)", {"query": "SUPER-SECRET-VALUE"})
|
||||
assert "SUPER-SECRET-VALUE" not in str(exc.value)
|
||||
evaluate_cel("int(query)", {"query": secret})
|
||||
assert secret not in str(exc.value)
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_error_keeps_the_exception_class(self):
|
||||
"""Redaction must not swallow the one safe diagnostic."""
|
||||
with pytest.raises(CelEvaluationError) as exc:
|
||||
evaluate_cel("int(query)", {"query": "nope"})
|
||||
assert "ValueError" in str(exc.value)
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_long_error_is_truncated(self):
|
||||
"""Backstop for celpy messages that survive activation-stripping."""
|
||||
from application.agents.workflows.cel_evaluator import _summarize_cel_error
|
||||
|
||||
summary = _summarize_cel_error(Exception("word " * 200))
|
||||
assert len(summary) <= 200
|
||||
assert summary.endswith("…")
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_validate_rejects_empty_expression(self):
|
||||
with pytest.raises(CelEvaluationError, match="Empty expression"):
|
||||
validate_cel_expression(" ")
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.parametrize(
|
||||
|
||||
Reference in new issue
Block a user