Skip to content

Commit 4b09004

Browse files
committed
Fix CodeRabbit review issues and switch to domain changelog headings
- Remove dead `if not LOG_FILE` guard in redirect-builtin-agents.py - Fix git global flag handling in stash/config index resolution - Add multi-target extraction for rm/touch/mkdir/chmod/chown commands - Switch CLAUDE.md changelog guidelines to domain headings - Restructure [Unreleased] changelog to domain headings, fix test counts - Add 12 new tests (289 total): global-flag stash tests, multi-target tests
1 parent dc0521f commit 4b09004

8 files changed

Lines changed: 161 additions & 278 deletions

File tree

.devcontainer/CHANGELOG.md

Lines changed: 14 additions & 261 deletions
Large diffs are not rendered by default.

.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/guard-readonly-bash.py

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -513,8 +513,9 @@ def check_git_readonly(command: str) -> str | None:
513513
# Resolve git global flags to find the real subcommand
514514
# e.g. git -C /path --no-pager log -> subcommand is "log"
515515
sub = None
516+
sub_idx = 0
516517
skip_next = False
517-
for w in words[1:]:
518+
for idx, w in enumerate(words[1:], start=1):
518519
if skip_next:
519520
skip_next = False
520521
continue
@@ -524,6 +525,7 @@ def check_git_readonly(command: str) -> str | None:
524525
if w.startswith("-"):
525526
continue
526527
sub = w
528+
sub_idx = idx
527529
break
528530

529531
if sub is None:
@@ -545,18 +547,18 @@ def check_git_readonly(command: str) -> str | None:
545547
"-l",
546548
"--get-regexp",
547549
}
548-
if not (set(words[2:]) & safe_flags):
550+
if not (set(words[sub_idx + 1 :]) & safe_flags):
549551
return "Blocked: 'git config' is only allowed with --get or --list"
550552

551553
elif sub == "stash":
552554
# Only allow "stash list" and "stash show"
553-
if len(words) <= 2:
555+
if len(words) <= sub_idx + 1:
554556
return "Blocked: bare 'git stash' (equivalent to push) is not allowed in read-only mode"
555-
if words[2] not in ("list", "show"):
556-
return f"Blocked: 'git stash {words[2]}' is not allowed in read-only mode"
557+
if words[sub_idx + 1] not in ("list", "show"):
558+
return f"Blocked: 'git stash {words[sub_idx + 1]}' is not allowed in read-only mode"
557559

558560
else:
559-
for w in words[2:]:
561+
for w in words[sub_idx + 1 :]:
560562
if w in restricted:
561563
return f"Blocked: 'git {sub} {w}' is not allowed in read-only mode"
562564

.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/redirect-builtin-agents.py

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,7 @@
4242

4343

4444
def log(message: str) -> None:
45-
"""Append a timestamped log entry if logging is enabled."""
46-
if not LOG_FILE:
47-
return
45+
"""Append a timestamped log entry."""
4846
try:
4947
with open(LOG_FILE, "a") as f:
5048
ts = datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%S")

.devcontainer/plugins/devs-marketplace/plugins/protected-files-guard/scripts/guard-protected-bash.py

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
import json
1212
import re
13+
import shlex
1314
import sys
1415

1516
# Same patterns as guard-protected.py
@@ -86,6 +87,59 @@
8687
]
8788

8889

90+
# Commands where all trailing non-flag arguments are file targets
91+
_MULTI_TARGET_CMDS = frozenset({"rm", "touch", "mkdir"})
92+
# Commands where the first non-flag arg is NOT a file (mode/owner), rest are
93+
_SKIP_FIRST_ARG_CMDS = frozenset({"chmod", "chown"})
94+
95+
96+
def _extract_multi_targets(command: str) -> list[str]:
97+
"""Extract all file targets from commands that accept multiple operands."""
98+
try:
99+
tokens = shlex.split(command)
100+
except ValueError:
101+
return []
102+
if not tokens:
103+
return []
104+
105+
# Handle prefixes like sudo, env, etc.
106+
prefixes = {"sudo", "env", "nohup", "nice", "command"}
107+
i = 0
108+
while i < len(tokens) and tokens[i] in prefixes:
109+
i += 1
110+
# Skip sudo flags like -u root
111+
if i > 0 and tokens[i - 1] == "sudo":
112+
while i < len(tokens) and tokens[i].startswith("-"):
113+
i += 1
114+
if i < len(tokens) and not tokens[i].startswith("-"):
115+
i += 1 # skip flag argument
116+
# Skip env VAR=val
117+
if i > 0 and tokens[i - 1] == "env":
118+
while i < len(tokens) and "=" in tokens[i]:
119+
i += 1
120+
if i >= len(tokens):
121+
return []
122+
cmd = tokens[i]
123+
124+
if cmd not in _MULTI_TARGET_CMDS and cmd not in _SKIP_FIRST_ARG_CMDS:
125+
return []
126+
127+
# Collect non-flag arguments
128+
args = []
129+
j = i + 1
130+
while j < len(tokens):
131+
if tokens[j].startswith("-"):
132+
j += 1
133+
continue
134+
args.append(tokens[j])
135+
j += 1
136+
137+
if cmd in _SKIP_FIRST_ARG_CMDS and args:
138+
args = args[1:] # First arg is mode/owner, not a file
139+
140+
return args
141+
142+
89143
def extract_write_targets(command: str) -> list[str]:
90144
"""Extract file paths that the command writes to."""
91145
targets = []
@@ -94,6 +148,10 @@ def extract_write_targets(command: str) -> list[str]:
94148
target = match.group(1).strip("'\"")
95149
if target:
96150
targets.append(target)
151+
# Supplement with multi-target extraction for commands like rm, touch, chmod
152+
for target in _extract_multi_targets(command):
153+
if target not in targets:
154+
targets.append(target)
97155
return targets
98156

99157

CLAUDE.md

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,7 @@ See `.devcontainer/CLAUDE.md` for full devcontainer documentation.
1818
Every change MUST have a corresponding entry in `.devcontainer/CHANGELOG.md`.
1919

2020
- New features, enhancements, fixes, and removals each get their own bullet
21-
- Group related changes under the appropriate `### Added`, `### Changed`, `### Fixed`, or `### Removed` heading
22-
- Use sub-headings (`####`) to organize by area (e.g., Workspace Scope Guard, Features, Configuration)
21+
- Group related changes under domain headings (`###`) by area (e.g., `### Security`, `### Agent System`, `### Documentation`, `### Configuration`)
2322
- If an unreleased version section doesn't exist, add changes to the current version's section
2423
- Write entries from the user's perspective — what changed, not how it was implemented
2524

tests/plugins/test_guard_protected.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,5 @@ def test_exception_causes_exit_code_2(self):
262262
text=True,
263263
)
264264
assert result.returncode == 2, (
265-
f"Expected exit code 2, got {result.returncode}. "
266-
f"stderr: {result.stderr}"
265+
f"Expected exit code 2, got {result.returncode}. stderr: {result.stderr}"
267266
)

tests/plugins/test_guard_protected_bash.py

Lines changed: 53 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -284,9 +284,59 @@ def test_extended_pattern_blocks_protected_file(self, command, expected_target):
284284
targets = guard_protected_bash.extract_write_targets(command)
285285
assert expected_target in targets
286286
is_protected, message = guard_protected_bash.check_path(expected_target)
287-
assert is_protected is True, (
288-
f"Expected '{expected_target}' to be protected"
287+
assert is_protected is True, f"Expected '{expected_target}' to be protected"
288+
assert message != ""
289+
290+
291+
# ---------------------------------------------------------------------------
292+
# Multi-target extraction
293+
# ---------------------------------------------------------------------------
294+
295+
296+
class TestMultiTargetExtraction:
297+
"""Commands with multiple file operands should check all targets."""
298+
299+
@pytest.mark.parametrize(
300+
"command, expected_target",
301+
[
302+
("rm safe.txt .env", ".env"),
303+
("touch a.txt .secrets", ".secrets"),
304+
("chmod 644 safe.txt .env", ".env"),
305+
("rm -rf safe/ .env", ".env"),
306+
("mkdir safe_dir .ssh/keys", ".ssh/keys"),
307+
],
308+
ids=[
309+
"rm-multi-catches-env",
310+
"touch-multi-catches-secrets",
311+
"chmod-multi-catches-env",
312+
"rm-rf-multi-catches-env",
313+
"mkdir-multi-catches-ssh",
314+
],
315+
)
316+
def test_multi_target_extracts_protected(self, command, expected_target):
317+
targets = guard_protected_bash.extract_write_targets(command)
318+
assert expected_target in targets, (
319+
f"Expected '{expected_target}' in extracted targets {targets}"
289320
)
321+
322+
@pytest.mark.parametrize(
323+
"command, expected_target",
324+
[
325+
("rm safe.txt .env", ".env"),
326+
("touch a.txt .secrets", ".secrets"),
327+
("chmod 644 safe.txt .env", ".env"),
328+
],
329+
ids=[
330+
"rm-blocks-env",
331+
"touch-blocks-secrets",
332+
"chmod-blocks-env",
333+
],
334+
)
335+
def test_multi_target_blocks_protected(self, command, expected_target):
336+
targets = guard_protected_bash.extract_write_targets(command)
337+
assert expected_target in targets
338+
is_protected, message = guard_protected_bash.check_path(expected_target)
339+
assert is_protected is True
290340
assert message != ""
291341

292342

@@ -324,6 +374,5 @@ def test_exception_causes_exit_code_2(self):
324374
text=True,
325375
)
326376
assert result.returncode == 2, (
327-
f"Expected exit code 2, got {result.returncode}. "
328-
f"stderr: {result.stderr}"
377+
f"Expected exit code 2, got {result.returncode}. stderr: {result.stderr}"
329378
)

tests/plugins/test_guard_readonly_bash.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,3 +324,28 @@ class TestGitReadonlyAllowed:
324324
)
325325
def test_readonly_commands_allowed(self, cmd: str) -> None:
326326
assert_allowed(guard_readonly_bash.check_git_readonly(cmd), cmd)
327+
328+
329+
# ---------------------------------------------------------------------------
330+
# 10. check_git_readonly - global flags with stash subcommand
331+
# ---------------------------------------------------------------------------
332+
333+
334+
class TestGitReadonlyGlobalFlagsStash:
335+
"""Ensure git global flags (-C, etc.) don't break stash sub-action detection."""
336+
337+
def test_stash_list_with_global_flag_allowed(self) -> None:
338+
cmd = "git -C /some/path stash list"
339+
assert_allowed(guard_readonly_bash.check_git_readonly(cmd), cmd)
340+
341+
def test_stash_show_with_global_flag_allowed(self) -> None:
342+
cmd = "git -C /some/path stash show"
343+
assert_allowed(guard_readonly_bash.check_git_readonly(cmd), cmd)
344+
345+
def test_stash_push_with_global_flag_blocked(self) -> None:
346+
cmd = "git -C /some/path stash push"
347+
assert_blocked(guard_readonly_bash.check_git_readonly(cmd), cmd)
348+
349+
def test_stash_drop_with_global_flag_blocked(self) -> None:
350+
cmd = "git -C /some/path stash drop"
351+
assert_blocked(guard_readonly_bash.check_git_readonly(cmd), cmd)

0 commit comments

Comments
 (0)