fix: address follow-up PR review findings
- NEW-001: Add sanitization to GitLab notes in buildIssueContext Apply sanitizeText() to note.author.username and note.body before writing to TASK.md, consistent with other external data sanitization. - NEW-003: Add try/finally protection to sys.modules manipulation Save original modules and sys.path before modifications, restore in finally block to prevent cascading test failures if exceptions occur. - NEW-004: Remove dead async function definition in test Removed agent_fn async function that was immediately overwritten by SystemExit(0) side_effect assignment.
This commit is contained in:
@@ -249,7 +249,9 @@ export function buildIssueContext(
|
||||
lines.push(`## Notes (${notes.length})`);
|
||||
lines.push('');
|
||||
for (const note of notes) {
|
||||
lines.push(`**${note.author.username}:** ${note.body}`);
|
||||
const safeAuthor = sanitizeText(note.author?.username || 'unknown', 100);
|
||||
const safeBody = sanitizeText(note.body, 20000, true);
|
||||
lines.push(`**${safeAuthor}:** ${safeBody}`);
|
||||
lines.push('');
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2030,9 +2030,6 @@ class TestHandleBuildInterruptEdgeCases:
|
||||
# so we can check the resume instructions
|
||||
with patch("cli.build_commands.select_menu", return_value="skip"):
|
||||
with patch("agent.run_autonomous_agent") as mock_agent:
|
||||
async def agent_fn(*args, **kwargs):
|
||||
return (True, "Success")
|
||||
mock_agent.side_effect = agent_fn
|
||||
mock_agent.side_effect = SystemExit(0)
|
||||
|
||||
# Execute - will exit after trying to resume
|
||||
@@ -2571,27 +2568,33 @@ class TestBuildCommandsModuleImport:
|
||||
module_path = build_cmd_module.__file__
|
||||
parent_dir = str(Path(module_path).parent.parent)
|
||||
|
||||
# Save original sys.path
|
||||
# Save original state
|
||||
original_path = sys.path.copy()
|
||||
original_modules = {k: sys.modules[k] for k in sys.modules.keys() if k.startswith('cli.build_commands')}
|
||||
|
||||
# Remove the parent directory from sys.path to make the condition True
|
||||
while parent_dir in sys.path:
|
||||
sys.path.remove(parent_dir)
|
||||
try:
|
||||
# Remove the parent directory from sys.path to make the condition True
|
||||
while parent_dir in sys.path:
|
||||
sys.path.remove(parent_dir)
|
||||
|
||||
# Remove module and its submodules from sys.modules to force re-import
|
||||
modules_to_remove = [k for k in sys.modules.keys() if k.startswith('cli.build_commands')]
|
||||
for mod_name in modules_to_remove:
|
||||
del sys.modules[mod_name]
|
||||
# Remove module and its submodules from sys.modules to force re-import
|
||||
modules_to_remove = [k for k in sys.modules.keys() if k.startswith('cli.build_commands')]
|
||||
for mod_name in modules_to_remove:
|
||||
del sys.modules[mod_name]
|
||||
|
||||
# Now import it fresh - this should execute line 15 under coverage
|
||||
import importlib.util
|
||||
spec = importlib.util.spec_from_file_location("cli.build_commands", module_path)
|
||||
module = importlib.util.module_from_spec(spec)
|
||||
sys.modules['cli.build_commands'] = module
|
||||
spec.loader.exec_module(module)
|
||||
# Now import it fresh - this should execute line 15 under coverage
|
||||
import importlib.util
|
||||
spec = importlib.util.spec_from_file_location("cli.build_commands", module_path)
|
||||
module = importlib.util.module_from_spec(spec)
|
||||
sys.modules['cli.build_commands'] = module
|
||||
spec.loader.exec_module(module)
|
||||
|
||||
# Verify the module loaded correctly
|
||||
assert hasattr(module, 'handle_build_command')
|
||||
|
||||
# Restore original sys.path
|
||||
sys.path[:] = original_path
|
||||
# Verify the module loaded correctly
|
||||
assert hasattr(module, 'handle_build_command')
|
||||
finally:
|
||||
# Always restore original state, even if an exception occurred
|
||||
sys.path[:] = original_path
|
||||
# Restore saved modules (if they still exist in sys.modules, skip to avoid conflicts)
|
||||
for mod_name, mod_obj in original_modules.items():
|
||||
if mod_name not in sys.modules:
|
||||
sys.modules[mod_name] = mod_obj
|
||||
|
||||
Reference in New Issue
Block a user