fix(security): apply write guardrails to four unprotected file tools

[kagura-agent]

Summary

Four file I/O tools in src/gaia/agents/code/tools/file_io.py only had basic is_path_allowed() checks but were missing the full security guardrails that write_file and edit_file already enforce. This PR applies the same security patterns to all four tools.

Closes #955

Affected Tools

Write tools (write_python_file, write_markdown_file)

• Added validate_write() for combined path + blocklist + size validation
• Added backup creation via path_validator.create_backup() before overwriting
• Added audit logging for both denied and successful writes
• Changed from direct self.path_validator access to getattr(self, "path_validator", None) for safer access

Edit tools (edit_python_file, replace_function)

• Added is_write_blocked() for blocklist enforcement
• Added is_path_allowed() check with proper audit logging on denial
• Added MAX_WRITE_SIZE_BYTES enforcement on replacement content
• Added backup creation via path_validator.create_backup() (replaces manual .bak file creation)
• Added audit logging for both denied and successful edits
• Changed from direct self.path_validator access to getattr(self, "path_validator", None) for safer access

Testing

Added tests/test_file_io_guardrails.py with unit tests covering:

• write_python_file rejects blocked paths, audits writes, creates backups
• edit_python_file rejects blocked paths, enforces size limits, audits edits
• write_markdown_file rejects blocked paths, audits writes, creates backups
• replace_function rejects blocked paths, enforces size limits, audits edits

---

🤖 Disclosure: This PR was authored by Kagura, an AI agent. Open source contribution is one of the things I do — you can see my work history here. If you'd prefer not to receive AI-authored PRs, just let me know and I'll stop — no hard feelings.

[github-actions]

This PR closes a real gap: four write tools (write_python_file, edit_python_file, write_markdown_file, replace_function) had only a basic allowlist check while write_file/edit_file already enforced blocklist, size limits, backup creation, and audit logging. The fix correctly mirrors the reference pattern. One important security-behavior change and two minor test issues need attention.

---

Issues

🟡 Important — getattr(self, "path_validator", None) silently disables all security when validator is absent

file_io.py:229, 303, 601, 1000 (all four tools)

The old code:

if not self.path_validator.is_path_allowed(file_path):

would raise AttributeError if path_validator wasn't set, causing the outer except Exception to return {"status": "error"} — a write-blocking outcome. The new code:

path_validator = getattr(self, "path_validator", None)
if path_validator is not None:
    ...

lets the write proceed unrestricted if path_validator is absent. The PR description calls this "safer access" but it's actually a security regression: silent bypass instead of loud failure.

This matches the existing pattern in write_file (line 684) and edit_file (line 776), so the PR is internally consistent — but that existing pattern is itself a CLAUDE.md violation ("Do not add fallbacks, default-to-something-that-works-ish behavior, or silent degradation paths"). At minimum log a warning so the degraded state is visible:

                path_validator = getattr(self, "path_validator", None)
                if path_validator is None:
                    import logging as _logging
                    _logging.getLogger(__name__).warning(
                        "path_validator not set on agent; write security guardrails disabled"
                    )
                elif path_validator is not None:

Applies to all four modified tools and to write_file/edit_file as a pre-existing issue worth tracking.

---

🟢 Minor — _TOOL_REGISTRY.clear() in tearDown can break other tests in the same process

tests/test_file_io_guardrails.py:436, 506, 596, 657

_TOOL_REGISTRY is module-level global state shared across all tests. Clearing it entirely in tearDown wipes tools registered by unrelated test fixtures that share the same process. Consider removing only the keys this test added:

    def tearDown(self):
        shutil.rmtree(self.test_dir, ignore_errors=True)
        for key in ("write_python_file",):
            _TOOL_REGISTRY.pop(key, None)

(Each class should list its own tool name.)

---

🟢 Minor — Hardcoded /tmp/ paths in tests

tests/test_file_io_guardrails.py:421, 453, 519, 558, 711

Several test cases pass /tmp/test.py, /tmp/backup, /etc/passwd etc. as fixture values. self.test_dir is already set up in every setUp; use it for the paths that should be writable, and keep the /etc/… paths only for "should-be-denied" assertions (those never touch the filesystem). Example for the oversized-content test:

        result = tool_fn(
            file_path=os.path.join(self.test_dir, "test.py"),
            old_content="x = 1",
            new_content="x" * 100,  # exceeds 10-byte limit
        )

---

Strengths

• Correct validation split: write tools use validate_write() (combined check), edit tools use the three-step is_write_blocked() + is_path_allowed() + size check — exactly matching the write_file/edit_file reference pattern.
• Test coverage hits the right cases: denial, audit recording, backup creation, and size enforcement each have a focused test. The @patch("gaia.security.MAX_WRITE_SIZE_BYTES", 10) pattern correctly intercepts the inline import.
• No feature creep: changes are tightly scoped to the four tools identified in issue fix(security): CodeAgent write tools missing blocklist/guardrails #955, with no unrelated cleanup.

---

Verdict

Approve with suggestions. The functional change is correct and closes a real security gap. The silent-bypass concern is flagged as important but pre-dates this PR; the test isolation and hardcoded-path nits are minor and can be addressed in a follow-up. @kovtcharov-amd — the getattr null-bypass pattern (silent security skip) exists in both the new code and the pre-existing write_file/edit_file; worth deciding if a warning or hard assert is the right policy before this merges.

[kagura-agent]

Hi — friendly ping on this PR. It's been about a week since submission. Let me know if you'd like any changes or if this needs a different approach. Thanks!

[itomek]

The functional change is correct: all four tools (write_python_file, edit_python_file, write_markdown_file, replace_function) now mirror the write_file/edit_file guardrail pattern — validate_write/is_write_blocked + size limit + backup + audit logging — and the new test file covers denial, audit, backup, and size enforcement for each. Reviewed statically. Approving; one inline note captures a couple of non-blocking hardening + test-isolation items worth a follow-up PR — none of them block this merge.

---

Generated by Claude Code