Log a workspace-containment refusal as a refusal, not an internal error - #24
Conversation
Closes #23. A URI resolving outside the workspace root raised a bare ValueError, caught by the blanket `except Exception` in server.py and logged with the same prefix at the same level as an internal fault: WARNING intentumdiff.lsp_server.server: intentumdiff/semanticDiff error: URI '...' resolves to '...' which is outside the workspace root '...' PASSED An operator could not tell "someone asked for a path outside the workspace and we blocked it" from "the diff engine threw". The refusal is a security control, so its audit trail is part of the control. Adds WorkspaceContainmentError, subclassing ValueError so every existing caller and test that catches ValueError keeps working. Raised for both the explicit traversal case and the containment case, handled separately, and the response carries code="workspace_containment" so a client can act on it without string-matching a message. The test now ASSERTS the log line, and asserts the internal-error prefix is absent. Previously nothing checked it: the warning could have been deleted and every test would still have passed. Capturing it also stops the warning printing in full beside a PASSED line, which reads like something went wrong and was ignored. pytest tests/unit/test_lsp_server.py: 24 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4998e5066
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| return json.loads(diff.model_dump_json()) | ||
|
|
||
| except WorkspaceContainmentError as exc: |
There was a problem hiding this comment.
When oldUri == newUri and the live document URI is outside the workspace (or contains explicit traversal), _do_diff catches the ValueError/WorkspaceContainmentError internally and returns None. Consequently, this new handler never runs for that documented request path: the client receives the generic Diff computation failed response without code: "workspace_containment", and the audit log still says intentumdiff: diff failed rather than recording a refusal. The containment exception must be preserved or classified before _do_diff swallows it.
Useful? React with 👍 / 👎.
1 participant
Closes #23.
A URI resolving outside the workspace root raised a bare
ValueError, caught by the blanketexcept Exceptioninserver.pyand logged with the same prefix, at the same level, as an internal fault:An operator could not tell "someone asked for a path outside the workspace and we blocked it" from "the diff engine threw". The refusal is a security control, so its audit trail is part of the control.
The fix
WorkspaceContainmentError, subclassingValueErrorso every existing caller and test that catchesValueErrorkeeps working. Raised for both the explicit-traversal and containment cases, handled separately from unexpected faults:The response now carries
code: "workspace_containment", so a client can act on a refusal without string-matching a message.The test now asserts the log line
Previously nothing checked it — the warning could have been deleted entirely and every test would still have passed. It now asserts both that the refusal is logged and that the internal-error prefix is absent, so the two cannot silently converge again.
Capturing it with
caplogalso keeps the suite output clean: this warning used to print in full beside aPASSEDline, which reads like something went wrong and was ignored. Log noise is how genuinely important warnings get skimmed past — the habit that let 0.0.1 ship printing 69 errors per run while exiting 0.Verification
pytest tests/unit/test_lsp_server.py— 24 passed.🤖 Generated with Claude Code