feat(llm): support configurable output language - #425
Conversation
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Requesting changes because the new prompt suffix accepts an unbounded, multi-line environment value verbatim. That permits accidental context-window exhaustion and lets configuration text inject instructions after the analyzer's security contract. Please constrain the setting to a short single-line language label (or a documented allowlist/tag format) and add rejection tests for CR/LF and oversized values.
8d13fcf to
1d8cda6
Compare
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
1d8cda6 to
5326ce9
Compare
|
Thanks @deepujain for the quick fix and your contribution to SkillSpector. I am approving and merging your PR now! |
| return ( | ||
| f"{prompt}\n\n## Output language\n\n" | ||
| "Write human-readable finding text (including message, finding, explanation, " | ||
| f"remediation, and intent fields when present) in {language}. Keep rule IDs, " |
There was a problem hiding this comment.
This line asks the model to write intent in the target language. But the only place intent comes from an LLM is MetaAnalyzerFinding (meta_analyzer.py:101), where it must be exactly "malicious", "negligent", or "benign" — Pydantic rejects anything else. So if the model actually follows this instruction and writes, say, "悪意のある", validation fails, the call retries, and after max attempts the whole meta-analyzer batch fails. OpenAI's structured output would block this server-side, but claude_cli (JSON parsed from plain text) and Ollama via OPENAI_BASE_URL won't — and those are supported setups. Fix is one word: remove intent from this list (or move it to the "keep unchanged" list). Maybe mention impact there too, since it's also a fixed-choice field.
| return prompt | ||
| return ( | ||
| f"{prompt}\n\n## Output language\n\n" | ||
| "Write human-readable finding text (including message, finding, explanation, " |
There was a problem hiding this comment.
Small one: finding shouldn't be in this list. In this codebase Finding.finding is the "short matched snippet" (models.py:121) — actual text copied from the scanned file, i.e. evidence, not something to translate. No LLM schema returns a finding field today so nothing breaks, but if one ever does, this line would tell the model to translate evidence. Safer to just drop the word.
| def build_prompt(self, batch: Batch, **_kwargs: object) -> str: | ||
| """Use TP4's purpose-built prompt without the generic file wrapper.""" | ||
| return batch.content | ||
| return append_output_language_instruction(batch.content) |
There was a problem hiding this comment.
Adding the language instruction here only translates what the LLM returns — but TP4's final message is built in Python from an English template (_tp4_finding, around line 1047): f"Description-behavior mismatch: declared purpose is '{declared}' but code also performs: {mismatched_text}.", and remediation a few lines below is a hardcoded English string the LLM never touches. So with SKILLSPECTOR_OUTPUT_LANGUAGE=Japanese the user gets an English sentence with Japanese pieces stuffed inside, plus an always-English remediation. Either translate those wrapper templates too, or note in the docs that TP4 messages stay in English.
| analyzer_prompt=self.base_prompt, | ||
| file_label=batch.file_label, | ||
| numbered_content=numbered, | ||
| return append_output_language_instruction( |
There was a problem hiding this comment.
Suggestion, not a blocker: instead of wrapping the prompt in all 3 build_prompt methods, wrap it once where the prompt is actually used — the two prompt = self.build_prompt(batch, **kwargs) lines in the run loops (around lines 826 and 925 after this PR). That covers all current analyzers plus any future subclass automatically. Right now, anyone who writes a new build_prompt override has to remember to add this call, and nothing reminds them — TP4 shows overrides do happen. Only cost is adjusting the tests that call build_prompt directly.
| return os.environ.get("SKILLSPECTOR_OUTPUT_LANGUAGE", "").strip() or None | ||
|
|
||
|
|
||
| def append_output_language_instruction(prompt: str) -> str: |
There was a problem hiding this comment.
Nit: this adds ~60 tokens to every prompt, but get_batches budgets file content using only estimate_tokens(self.base_prompt), so the extra text isn't counted. It's noise in practice (the base wrapper isn't fully counted either) — a short comment here would be enough.
| file_label=batch.file_label, | ||
| file_content=batch.content, | ||
| static_findings=findings_text, | ||
| return append_output_language_instruction( |
There was a problem hiding this comment.
Nit: The meta-analyzer prompt ends with "Analyze the findings now:", and this appends the language section after that. Models cope fine, but it reads oddly — a final instruction after the "go" line. Fine to leave (good to fix as well 🙃) ; just flagging.
3 participants
Summary
SKILLSPECTOR_OUTPUT_LANGUAGEfor localizing human-readable LLM finding text..env.example, the README, and the development guide.Validation
.venv/bin/pytest tests/nodes/test_llm_analyzer_base.py tests/test_mcp_tool_poisoning.py -q -m 'not integration and not provider'— 210 passed, 6 deselected; covers unset, blank, trimmed, discovery, meta-analyzer, and TP4 prompt behavior without live provider calls..venv/bin/ruff check src tests— passed..venv/bin/ruff format --check src tests— 192 files already formatted..venv/bin/pytest -m 'not integration and not provider' tests/ -q— 2,804 passed, 13 skipped, 38 deselected, 4 xfailed.git diff --check— passed.Risk
Fixes #345