Repository navigation
Exploitation capability improvements (12 phases) + CVE/quality follow-ups - #12
Merged
Merged
Conversation
Add a per-session findings accumulator to PentestOrchestrator so discoveries flow forward across iterations: - init _session_findings (+_replan_count) in __init__ - _merge_findings_into_session classifies structured_findings after each task - inject the accumulator into target_info before command generation - rebuild the accumulator from completed PTT nodes on session resume Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…m merge - test_network_operations_agent: call real _discover_hosts (was _simulate_host_discovery, which no longer exists) and patch is_port_open to avoid real network I/O — clears 2 mypy strict attr-defined errors. - test_monitoring_components: stop the AlertCorrelationEngine daemon thread in teardown (test_correlation_performance try/finally + TestIntegrationScenarios.tearDown) so the leaked _correlation_processing_loop no longer pollutes detection_resistance/stealth tests in full-suite runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Turn forward-flowing findings into concrete follow-up tasks: - make _generate_node_id collision-proof (UUID) so a batch of follow-up nodes created before commit no longer share one id - add PTTManager.generate_typed_followup_tasks: credential -> lateral movement (T1021), ssrf/internal_service -> discovery (T1046), each carrying a chain_context payload - orchestrator._chain_findings_to_tasks spawns those typed follow-ups after each task and adds them to the PTT - command_generator routes a lateral_movement chain_context through PentestGPTPromptV3.get_lateral_movement_prompt Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When the task queue empties, attempt strategic replanning via strategy_agent.plan_next_phase to promote an unexplored MITRE tactic instead of terminating the run. Bounded by _replan_count to prevent infinite loops. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…reason Task 4.1: LearningContextManager.record read findings["raw_output"], a key the results verifier never emits (it sets raw_output_summary), so classify_failure always saw "" and returned "unknown". Read raw_output_summary first, falling back to raw_output. Task 4.2: thread the classified failure reason (and soft/hard failure type) into StrategyAgent.suggest_alternative_technique and the orchestrator's soft-failure reroute path, so alternatives are chosen with knowledge of why the technique failed instead of blindly. Interface kept in sync in StrategyAgentInterface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…thresholds Stop re-running a MITRE technique that keeps failing within the current engagement: LearningContextManager now counts failures per (technique, reason) and exposes is_session_blocked(), which the orchestrator enforces in _pre_task_retired_filtering before the learning-engine check so it fires even with no learning engine configured. Move the previously-hardcoded retirement/skip thresholds into LearningConfig (with to_dict/from_dict round-trip parity); learning_engine and command_generator now read them from config, with literal fallbacks that preserve behavior for test doubles whose config is unconfigured. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two prerequisites for the post-exploitation parser work that follows. Task 6.1 — Extend the executor allowlist. ToolExecutor._is_command_allowed matches the literal first token of a command against default_allowed_tools; AD/post-exploitation tools were absent, so their commands were blocked before execution and produced zero findings. Add 20 entries (secretsdump.py, GetUserSPNs.py, nxc/netexec, certipy, responder, ffuf, wpscan, enum4linux-ng, and case/impacket variants). New tests cover set membership and the real _is_command_allowed matching path. Task 6.2 — Fix nuclei output routing. NucleiParser.recommended_flags() returned ["-json", "-o", <tmpfile>], writing findings to a file the results verifier never reads (it parses stdout), so every nuclei finding was silently discarded. Return ["-jsonl", "-silent"] to stream JSONL on stdout; drop the now-unused os/tempfile imports. Rewrote the stale TestRecommendedFlags class to assert the new stdout-routing behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add deterministic ParsedFinding parsers for the core AD/post-exploitation tools, all registered in the parser registry: - secretsdump.py -> credential (NTLM hash dumps, pwdump format) - GetUserSPNs.py -> kerberoastable_spn (full $krb5tgs$ blob captured) - NetExec/CME -> credential ([+] lines; Pwn3d!-aware severity) - Certipy -> vulnerable_cert_template (per-template ESC detection) - Responder -> credential (NetNTLMv2 captures) Each parser routes output to stdout (recommended_flags avoids -outputfile), and each test asserts registry discovery to guard the registry's silent ImportError fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a file-ingestion fallback to ResultsVerifier so tools that emit only to files (BloodHound, ffuf default, enum4linux-ng) reach the parser layer, plus four Tier-2 parsers. - ResultsVerifier._read_output_artifact: extract -o/-of/--outputfile path and read it (bounded 5MB) when stdout is empty; finditer picks the first token that is an existing file (robust to -of/-o flag ordering) - wpscan parser: vulnerability + user_enumeration from --format json - ffuf parser: discovered_path from results[] - bloodhound parser: ad_attack_path per dangerous ACE (critical for GenericAll/WriteDacl/Owns/ForceChangePassword) - enum4linux-ng parser: user_enumeration/share/enumeration (file-based) - register all four in the parser registry Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Stop discarding the output of two existing subsystems: - _apply_technique_selection now stores the top TechniqueRecommendation on task.findings["selector_hint"] and _generate_commands injects it into the command-generation rag_context (guarded by isinstance(hint, dict)). - _trigger_exploitation now captures create_exploitation_plan's id into a new self._exploitation_plans map keyed by task id instead of dropping it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…daptation Replace the static-template exploit generator with LLM-synthesized, target-aware generation backed by a sandboxed test-and-mutate loop, and implement real-time technique adaptation that reacts to detected defenses. - Plumb model_router + tool_executor through AdvancedExploitationAgent and the orchestrator factory into ExploitDeveloper (both factory entry points). - ExploitDeveloper: _generate_exploit_code/_generate_payload now build target-aware prompts and route via the LLM, degrading to static literals when no router is wired; _test_exploit runs through the real ToolExecutor (str output) with success-marker detection instead of random(); add a bounded mutate-retry loop driven by exploit-failure classification. - AdaptiveTechniqueSelector.adjust_technique_real_time now maps detected defenses (WAF/EDR) to concrete evasion guidance while preserving its four-key return contract in every branch. - Orchestrator carries that guidance from _post_task_processing into the next task's selector_hint, surfaced into command generation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ation, CI/CD, identity-provider) Add four DomainKnowledgeModule subclasses covering the largest knowledge gaps, each auto-injected into command generation via rag.query_with_domain_context when detect_domain_applicability scores above 0.5. - api_graphql: BOLA/IDOR, GraphQL introspection/batching, mass assignment, endpoint discovery. Two-tier applicability scoring (strong API/GraphQL path markers vs. generic application/json) so a single real signal clears the gate without over-activating. - deserialization: Java (ysoserial), Python pickle, PHP (phpggc), .NET ViewState, GadgetProbe fingerprinting, PyYAML; serialized-object markers. - cicd: self-hosted runner takeover, CI secret exfiltration, poisoned pipeline execution, Jenkins script console RCE, GitHub Actions enum, GitLab runner config theft; string + port signals. - identity_provider: Golden SAML, XML signature wrapping, Pass-the-PRT, device-code phishing, rogue device CA bypass, OIDC token replay. Each module registered in both domains/__init__.py and the factory's _register_domain_modules. Techniques are impact-ordered because the RAG layer surfaces only the first five and each technique's first command. Updated two factory tests that hard-coded the previous module count (8 -> 12). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add EnhancedRAGSystem._load_corpus_documents to load *.pdf, *.md, *.txt, and *.json reference material (each format in its own isolated pass), replacing the PDF-only DirectoryLoader in _initialize_vectorstore. JSON is ingested as raw text (no jq schema) — adequate for semantic retrieval. Fix the dead `langchain.text_splitter` import (removed in langchain 1.x) to `langchain_text_splitters`. This flips LANGCHAIN_AVAILABLE on for the first time so the vector store actually initializes; verified no test regressions. Pin numpy-2-compatible spacy/thinc (spacy 3.7.5 / thinc 8.2.5 were built for numpy 1.x and crashed the langchain loaders under the installed numpy 2.x); align the numpy pin with the installed 2.4.3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…NGELOG) Update CLAUDE.md parser list with the 9 new parsers and add a CHANGELOG entry covering all 12 phases of the exploitation-capability effort. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ko for CVEs Resolve three advisories flagged by safety: langchain SQL injection (1.2.15 -> 1.3.9), langchain-core unsafe deserialization (1.2.30 -> 1.4.7), and python-dotenv arbitrary file overwrite (1.2.1 -> 1.2.2). Pin paramiko to ==4.0.0 (was >=3.0.0) so it is reproducible and scannable. Full gate (ruff/mypy/pytest 5238) green; safety now reports 0 vulnerabilities. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ge doc Add [tool.pydocstyle] with convention=google so the standalone pydocstyle run matches ruff's pydocstyle rules. Update CODE_QUALITY.md to use the non-deprecated `safety scan --policy-file` command (CI already uses it). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR expands parser coverage across nine new tools, introduces exploit execution with dependency injection and retry-based failure mutation, adds orchestrator finding chaining and bounded replanning with defense adaptation, registers four new knowledge domains, extends RAG corpus loading to multiple formats, and updates configuration, documentation, and test harness accordingly. ChangesExploitation capability expansion
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
pentestgpt/orchestrator.py (1)
641-647:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid key-collision overwrite when injecting session findings into
target_info.Line 641 flattens
self._session_findingsintotarget_info, which can overwrite existing fields (notablyopen_ports) with a different shape and break downstream consumers.Suggested fix
- target_info = {**target_info, **self._session_findings} + target_info = { + **target_info, + "session_findings": make_serializable_snapshot(self._session_findings), + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/orchestrator.py` around lines 641 - 647, The dictionary unpacking on line 641 in the generate_commands call is causing key collisions where self._session_findings can overwrite existing fields in target_info (particularly open_ports) with incompatible data shapes. Instead of directly flattening self._session_findings into target_info with the spread operator, modify the merging logic to either only add keys from self._session_findings that don't already exist in target_info, or nest the session_findings under a separate key in target_info to avoid overwriting critical fields that downstream consumers depend on.pentestgpt/utils/orchestrator_factory.py (1)
244-279:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIsolate domain-module registration failures per module.
Line 244 wraps import + all registrations in a single
try, so one failing module prevents every domain module from being registered. With the new modules added, this becomes a larger availability risk for RAG enrichment.Suggested fix
- try: - from pentestgpt.knowledge.domains import ( - ActiveDirectoryModule, - ApiGraphqlModule, - AWSModule, - AzureModule, - CicdModule, - DeserializationModule, - DockerModule, - GCPModule, - IdentityProviderModule, - KubernetesModule, - WebAdvancedModule, - WirelessModule, - ) - - for module_cls in ( - ActiveDirectoryModule, - ApiGraphqlModule, - AWSModule, - AzureModule, - CicdModule, - DeserializationModule, - GCPModule, - DockerModule, - IdentityProviderModule, - KubernetesModule, - WebAdvancedModule, - WirelessModule, - ): - rag_system.register_domain_module(module_cls()) - - logger.info("Domain knowledge modules registered with RAG system") - except Exception as e: - logger.warning(f"Domain module registration failed (continuing without): {e}") + from pentestgpt.knowledge.domains import ( + ActiveDirectoryModule, + ApiGraphqlModule, + AWSModule, + AzureModule, + CicdModule, + DeserializationModule, + DockerModule, + GCPModule, + IdentityProviderModule, + KubernetesModule, + WebAdvancedModule, + WirelessModule, + ) + + registered = 0 + for module_cls in ( + ActiveDirectoryModule, + ApiGraphqlModule, + AWSModule, + AzureModule, + CicdModule, + DeserializationModule, + GCPModule, + DockerModule, + IdentityProviderModule, + KubernetesModule, + WebAdvancedModule, + WirelessModule, + ): + try: + rag_system.register_domain_module(module_cls()) + registered += 1 + except Exception as e: + logger.warning(f"Domain module registration failed for {module_cls.__name__}: {e}") + + logger.info(f"Registered {registered} domain knowledge modules with RAG system")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/utils/orchestrator_factory.py` around lines 244 - 279, The current implementation wraps all module imports and registrations in a single try-except block, meaning if one module fails to import or register, all subsequent modules will not be registered. Instead, move the try-except to wrap only the individual module registration calls in the loop. Keep the imports outside the loop in their own try-except, and then wrap each rag_system.register_domain_module(module_cls()) call with its own try-except that logs failures per module while allowing the loop to continue. This way, failures in one module like ActiveDirectoryModule or AWSModule will not prevent other modules from being registered with the RAG system.tests/orchestrator/test_defense_adaptation_wiring.py (1)
8-42: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd strict MyPy-compatible signatures to these tests.
Annotate fixture parameters and returns (including
async def ... -> None) so the file is fully typed.As per coding guidelines, "Enforce strict MyPy typing — all new code must be fully typed."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/orchestrator/test_defense_adaptation_wiring.py` around lines 8 - 42, Add complete MyPy-compatible type annotations to all three test functions: test_defense_adaptation_queues_guidance, test_no_defenses_queues_nothing, and test_queued_guidance_flows_into_next_selector_hint. Annotate the make_orchestrator fixture parameter with its proper return type and add explicit return type hints (-> None) to all functions, including the async function. Ensure all type hints follow MyPy strict mode requirements so the file passes full type checking.Source: Coding guidelines
tests/knowledge/test_realtime_adaptation.py (1)
9-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit return types on new test functions for strict MyPy mode.
These test functions are untyped (
def ...():) while the repo requires fully typed new Python code. Add-> Noneto each test definition.Suggested patch
-def test_waf_yields_tamper_guidance(): +def test_waf_yields_tamper_guidance() -> None: @@ -def test_no_defenses_no_adjustment(): +def test_no_defenses_no_adjustment() -> None: @@ -def test_contract_keys_present_in_all_branches(): +def test_contract_keys_present_in_all_branches() -> None:As per coding guidelines: "Enforce strict MyPy typing — all new code must be fully typed".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/knowledge/test_realtime_adaptation.py` around lines 9 - 36, The three test functions test_waf_yields_tamper_guidance, test_no_defenses_no_adjustment, and test_contract_keys_present_in_all_branches are missing explicit return type annotations required for strict MyPy compliance. Add the return type annotation -> None to each function definition to satisfy the strict typing requirements for new code.Source: Coding guidelines
pentestgpt/utils/rag.py (1)
117-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winShort-circuit vectorstore initialization when corpus is empty.
When no documents are loaded, initialization proceeds and can fail later, leaving retriever unavailable. Add an early return with a warning to keep behavior explicit and stable.
Suggested patch
# Load documents (PDF + Markdown/text/JSON corpus) documents = self._load_corpus_documents(self.knowledge_base_path) + if not documents: + logger.warning(f"No knowledge-base documents found in: {self.knowledge_base_path}") + return # Split documents text_splitter = RecursiveCharacterTextSplitter(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/utils/rag.py` around lines 117 - 126, After the documents are loaded via the _load_corpus_documents method call, add an early return with a warning log if the documents list is empty. Check if documents is empty right after the assignment and before the text_splitter is created and split_documents is called. If the documents list is empty, log a warning message indicating that the corpus is empty and return early from the function to prevent further initialization that could fail downstream. This ensures the retriever behavior remains explicit and stable when no documents are available.
🟠 Major comments (21)
pentestgpt/agents/parsers/nuclei_parser.py-108-119 (1)
108-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
recommended_flags()introduces a command-mutation edge case with enhancer replacement logic.At Line 119, returning
["-jsonl", "-silent"]as a two-token sequence can interact badly with the command enhancer’s “flag + value” replacement path (frompentestgpt/agents/command_generator.py:213-257). If a command already has-jsonlalone, the enhancer may replace-jsonlplus the next positional token, corrupting the command.A safe short-term fix is to only recommend the single standalone flag here.
Suggested patch
- return ["-jsonl", "-silent"] + return ["-jsonl"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/agents/parsers/nuclei_parser.py` around lines 108 - 119, The recommended_flags() method currently returns a two-token sequence ["-jsonl", "-silent"] that can cause a command-mutation edge case when interacting with the command enhancer's "flag + value" replacement logic in pentestgpt/agents/command_generator.py:213-257. If a command already contains -jsonl, the enhancer may incorrectly replace -jsonl plus the next positional token, corrupting the command. To fix this, modify the recommended_flags() method to return only a single standalone flag instead of both flags, which avoids triggering the problematic "flag + value" replacement path in the enhancer.pentestgpt/agents/parsers/responder_parser.py-24-39 (1)
24-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
can_parse()accepts NetNTLMv2 hash-only output thatparse()currently drops.When output matches
_NTLMV2_HASHbut lacks labeledNTLMv2-SSP Hashlines, this parser is selected and then returns no findings. Add a hash-only fallback inparse()(or narrowcan_parse()).💡 Proposed fix
-_NTLMV2_HASH = re.compile( - r"^\S+::[^:]+:[a-fA-F0-9]{16}:[a-fA-F0-9]{32}:[a-fA-F0-9]+", - re.MULTILINE, -) +_NTLMV2_HASH = re.compile( + r"^(?P<user>[^:\s]+)::(?P<domain>[^:]*):(?P<challenge>[a-fA-F0-9]{16}):(?P<response>[a-fA-F0-9]{32}):(?P<blob>[a-fA-F0-9]+)$", + re.MULTILINE, +) @@ - for line in output.splitlines(): + for line in output.splitlines(): + line = line.strip() cm = _CLIENT.search(line) @@ hm = _HASH.search(line) if hm: findings.append( @@ ) domain = "" username = "" + continue + + # Fallback: hash-only NetNTLMv2 line without labeled Responder prefixes + sniff = _NTLMV2_HASH.search(line) + if sniff: + findings.append( + ParsedFinding( + tool="responder", + finding_type="credential", + severity="high", + confidence=0.9, + target=client or sniff.group("domain"), + detail={ + "protocol": "unknown", + "domain": sniff.group("domain"), + "username": sniff.group("user"), + "ntlmv2_hash": line, + "hash_format": "netntlmv2", + }, + raw_evidence=line, + ) + ) return findingsAlso applies to: 40-89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/agents/parsers/responder_parser.py` around lines 24 - 39, The can_parse() method returns True when the output matches the _NTLMV2_HASH regex pattern, but the parse() method does not have a code path to handle hash-only output and returns no findings. Add a fallback case in the parse() method that extracts and processes hashes directly when output matches _NTLMV2_HASH but does not contain the labeled "NTLMv2-SSP Hash" lines, ensuring that hash-only output is actually parsed and returned as findings instead of being dropped.pentestgpt/agents/results_verifier.py-315-342 (1)
315-342:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict artifact reads to trusted paths; current logic can read arbitrary local files.
_read_output_artifact()trusts command-provided file paths and opens any existing file. A crafted-o/--outputfilepath can make the verifier read unintended coordinator-host files.🔒 Proposed hardening
- import os - import re + import os + import shlex - for m in re.finditer(r"(?:-oJ|-of|--outputfile|-output|-o)\s+(\S+)", command): - path = m.group(1) - if path in ("-", "/dev/stdout") or not os.path.isfile(path): + output_flags = {"-oJ", "-of", "--outputfile", "-output", "-o"} + workspace_root = os.path.realpath(os.getcwd()) + tokens = shlex.split(command) + for i, token in enumerate(tokens[:-1]): + if token not in output_flags: continue + raw_path = tokens[i + 1] + if raw_path in ("-", "/dev/stdout"): + continue + path = os.path.realpath(os.path.expanduser(raw_path)) + try: + if os.path.commonpath([workspace_root, path]) != workspace_root: + continue + except ValueError: + continue + if not os.path.isfile(path): + continue try: with open(path, encoding="utf-8", errors="replace") as fh: return fh.read(5_000_000) except OSError: - return "" + continue return ""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/agents/results_verifier.py` around lines 315 - 342, The _read_output_artifact method currently reads any file path specified in the command output flags without restricting to a safe directory. Modify the method to validate that the path extracted from m.group(1) is restricted to a trusted output directory (likely a temporary or work directory where tool outputs are expected). Use os.path.realpath or pathlib.Path.resolve to normalize the path and verify it stays within the trusted directory boundary, rejecting any paths that attempt to escape via relative path traversal. Only proceed with opening and reading the file if the validation passes.pentestgpt/config/knowledge_config.py-70-73 (1)
70-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate newly added learning thresholds in
validate_config().The new
retired_*andskip_*fields are now persisted/loaded but are not range-checked, so invalid values can silently alter retirement/skip behavior.🛡️ Suggested patch
if self.learning.min_samples_for_learning < 1: issues.append("Min samples for learning must be at least 1") + if self.learning.retired_min_samples < 1: + issues.append("Retired min samples must be at least 1") + + if not 0.0 <= self.learning.retired_max_success_rate <= 1.0: + issues.append("Retired max success rate must be between 0.0 and 1.0") + + if self.learning.skip_min_samples < 1: + issues.append("Skip min samples must be at least 1") + + if not 0.0 <= self.learning.skip_max_success_rate <= 1.0: + issues.append("Skip max success rate must be between 0.0 and 1.0") + if not 0.0 <= self.learning.confidence_threshold <= 1.0: issues.append("Confidence threshold must be between 0.0 and 1.0")Also applies to: 368-371, 485-500
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/config/knowledge_config.py` around lines 70 - 73, Add validation checks for the newly added threshold fields in the validate_config() method to ensure invalid values are caught before they can affect runtime behavior. The fields retired_min_samples, retired_max_success_rate, skip_min_samples, and skip_max_success_rate need range validation (for example, ensuring min_samples is positive and success_rate values are between 0 and 1). Apply the same validation pattern used for existing configuration fields to these new fields to prevent silent failures and invalid state propagation.pentestgpt/agents/exploit_developer.py-166-173 (1)
166-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegenerated exploit candidates can collide on
exploit_idand overwrite state.The retry loop can call
_generate_exploit_candidatemultiple times in the same second, but IDs are timestamp-second based. That can reuse IDs and overwriteself.exploit_candidates.🐛 Suggested patch
+import uuid @@ - exploit_id = f"exploit_{vulnerability['vulnerability_id']}_{int(time.time())}" + exploit_id = f"exploit_{vulnerability['vulnerability_id']}_{uuid.uuid4().hex}"Also applies to: 228-228
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/agents/exploit_developer.py` around lines 166 - 173, The exploit ID generation in the `_generate_exploit_candidate` method uses only timestamp-second precision, causing collisions when multiple candidates are generated within the same second during the retry loop (lines 166-173 and also at line 228). Enhance the ID generation logic to include higher precision timing (microseconds or milliseconds) or add an incrementing counter to ensure unique exploit_ids even when multiple candidates are generated in quick succession, preventing overwrites in the self.exploit_candidates dictionary.pentestgpt/agents/exploit_developer.py-528-528 (1)
528-528:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout to sandbox execution calls.
self.tool_executor.execute(...)is currently unbounded. A hung exploit command can stall the entire session loop.🛡️ Suggested patch
- output = self.tool_executor.execute(candidate.exploit_code) # str + output = self.tool_executor.execute(candidate.exploit_code, timeout=120) # str🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/agents/exploit_developer.py` at line 528, The self.tool_executor.execute() call for candidate.exploit_code lacks a timeout parameter, which can cause the session to hang indefinitely if the exploit command becomes unresponsive. Add a timeout parameter to the execute method call to ensure it completes within a reasonable time frame, preventing the session loop from stalling. If the tool_executor.execute() method supports a timeout argument, pass an appropriate timeout value (such as a reasonable duration in seconds). If the method does not natively support timeout, consider wrapping the execute call with a timeout mechanism using Python's threading or asyncio features to interrupt long-running operations.tests/agents/test_file_ingestion.py-15-80 (1)
15-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit type annotations to new test function signatures.
These new tests currently leave fixture parameters and return types partially untyped, which breaks the strict typing requirement for newly added Python code.
♻️ Suggested patch
+from pathlib import Path +import pytest from unittest.mock import MagicMock from pentestgpt.agents.results_verifier import ResultsVerifier -def test_reads_declared_output_file(tmp_path): +def test_reads_declared_output_file(tmp_path: Path) -> None: @@ -def test_reads_file_when_flag_order_reversed(tmp_path): +def test_reads_file_when_flag_order_reversed(tmp_path: Path) -> None: @@ -def test_missing_file_returns_empty(tmp_path): +def test_missing_file_returns_empty(tmp_path: Path) -> None: @@ -def test_no_output_flag_returns_empty(): +def test_no_output_flag_returns_empty() -> None: @@ -def test_dash_stdout_returns_empty(): +def test_dash_stdout_returns_empty() -> None: @@ -def test_dispatch_loop_passes_file_content_to_parser(tmp_path, monkeypatch): +def test_dispatch_loop_passes_file_content_to_parser( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None:As per coding guidelines, “Enforce strict MyPy typing — all new code must be fully typed.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/agents/test_file_ingestion.py` around lines 15 - 80, Add explicit type annotations to all test function signatures in the file. For each test function (test_reads_declared_output_file, test_reads_file_when_flag_order_reversed, test_missing_file_returns_empty, test_no_output_flag_returns_empty, test_dash_stdout_returns_empty, and test_dispatch_loop_passes_file_content_to_parser), add type hints for all parameters and the return type. The tmp_path parameter should be typed as a Path object, the monkeypatch parameter should be typed using pytest's MonkeyPatch type, and all test functions should have a return type annotation of None.Source: Coding guidelines
pentestgpt/agents/exploit_developer.py-588-590 (1)
588-590:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden numeric mutation logic to avoid aborting the whole exploit batch.
Direct
int(...)casts on mutable vulnerability fields can raiseValueErrorand bubble to the outer handler, which drops accumulated attempt/success data.🐛 Suggested patch
def _mutate_vulnerability(self, vulnerability: dict[str, Any], reason: str) -> dict[str, Any]: """Return a copy of the vulnerability adjusted to overcome a failure reason.""" mutated = dict(vulnerability) + def _to_int(value: Any, default: int) -> int: + try: + return int(value) + except (TypeError, ValueError): + return default + if reason == "waf_blocked": mutated["waf"] = True elif reason == "filtered_char": mutated["filtered"] = True elif reason == "wrong_columns": - mutated["columns"] = int(mutated.get("columns", 1) or 1) + 1 + mutated["columns"] = _to_int(mutated.get("columns"), 1) + 1 elif reason == "wrong_gadget": - mutated["gadget_index"] = int(mutated.get("gadget_index", 0) or 0) + 1 + mutated["gadget_index"] = _to_int(mutated.get("gadget_index"), 0) + 1 return mutated🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/agents/exploit_developer.py` around lines 588 - 590, The numeric mutation logic for the "columns" and "gadget_index" fields uses direct int() casts that can raise ValueError if the stored values are not numeric, causing the entire exploit batch to abort and losing accumulated attempt/success data. Wrap the int() conversions in try-except blocks to gracefully handle ValueError exceptions, providing safe fallback values (such as 1 for "columns" and 0 for "gadget_index") when the conversion fails, ensuring the mutation process continues without interrupting the outer handler.pentestgpt/agents/command_generator.py-153-160 (1)
153-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
task.findingsbefore calling.get()to prevent runtime crashes.This branch assumes
task.findingsis always a dict. If it is present but non-dict, command generation fails withAttributeError.🐛 Suggested patch
- chain_context = getattr(task, "findings", {}).get("chain_context") - if chain_context and chain_context.get("prompt_type") == "lateral_movement": + findings_obj = getattr(task, "findings", None) + chain_context = findings_obj.get("chain_context") if isinstance(findings_obj, dict) else None + if isinstance(chain_context, dict) and chain_context.get("prompt_type") == "lateral_movement":🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/agents/command_generator.py` around lines 153 - 160, The code assumes task.findings is always a dict when calling getattr and then .get() on chain_context. If task.findings exists but is a non-dict type (None, list, string, etc.), the .get() call will fail with AttributeError. Add a type guard to verify that task.findings is actually a dict before attempting to access chain_context with .get(). Consider checking isinstance(findings, dict) or using a conditional assignment that safely handles non-dict values by falling back to an empty dict.pentestgpt/orchestrator.py-484-487 (1)
484-487:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
structured_findingsentries before.get()access.Line 485 assumes every item is a mapping; a non-dict entry will raise and abort the task cycle.
Suggested fix
- for sf in findings.get("structured_findings", []): + for sf in findings.get("structured_findings", []): + if not isinstance(sf, dict): + continue ft = sf.get("finding_type", "") detail = sf.get("detail", "") target = sf.get("target", "")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/orchestrator.py` around lines 484 - 487, In the loop that iterates over findings.get("structured_findings", []), the code assumes each item sf is a dictionary and directly calls .get() methods on it without validation. Add a guard check to verify that sf is a dictionary before attempting to access its keys with .get("finding_type"), .get("detail"), and .get("target"). If sf is not a dictionary, either skip that entry with a continue statement or handle it appropriately to prevent the AttributeError from aborting the task cycle.tests/orchestrator/test_failure_reroute.py-4-4 (1)
4-4: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winType-annotate the test function for strict MyPy mode.
Please add
-> Noneto this test definition.As per coding guidelines, "Enforce strict MyPy typing — all new code must be fully typed."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/orchestrator/test_failure_reroute.py` at line 4, The test function test_alternative_prompt_includes_failure_reason is missing a return type annotation required by strict MyPy mode. Add the return type annotation `-> None` to the function definition to comply with the coding guidelines that enforce strict MyPy typing for all new code.Source: Coding guidelines
tests/orchestrator/test_replanning.py-7-27 (1)
7-27: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd strict MyPy annotations to both test definitions.
Please annotate fixture parameters and add explicit
-> Nonereturn types.As per coding guidelines, "Enforce strict MyPy typing — all new code must be fully typed."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/orchestrator/test_replanning.py` around lines 7 - 27, Both test functions test_replanning_promotes_new_tactic and test_replanning_respects_count_guard are missing strict MyPy type annotations. Add a type annotation for the make_orchestrator fixture parameter (the type should match your fixture definition, typically something like Callable or a specific orchestrator type) and add explicit -> None return type annotations to both function definitions to comply with strict typing requirements.Source: Coding guidelines
tests/orchestrator/test_exploitation_wiring.py-6-31 (1)
6-31: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winType-annotate these test functions for strict MyPy.
Please annotate fixture parameters and add
-> Nonereturns on both test definitions.As per coding guidelines, "Enforce strict MyPy typing — all new code must be fully typed."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/orchestrator/test_exploitation_wiring.py` around lines 6 - 31, Both test functions test_exploitation_plan_stored and test_exploitation_plan_not_stored_when_id_falsy lack proper type annotations required by strict MyPy. Add type annotation for the make_orchestrator fixture parameter (use the appropriate type from your orchestrator module based on what the fixture returns) and add -> None return type annotation to both function signatures to comply with the strict MyPy typing requirements.Source: Coding guidelines
tests/orchestrator/test_finding_chaining.py-8-63 (1)
8-63: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winBring test signatures up to strict typing requirements.
Add type annotations for fixture params (
tmp_path,make_orchestrator) and explicit-> Nonereturns for all test functions in this module.As per coding guidelines, "Enforce strict MyPy typing — all new code must be fully typed."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/orchestrator/test_finding_chaining.py` around lines 8 - 63, Add strict type annotations to all test functions in this module. For test_credential_finding_spawns_lateral_movement_task, test_ssrf_finding_spawns_internal_recon_task, and test_chained_tasks_added_to_ptt, add proper type hints for the fixture parameters (tmp_path should be typed as Path from pathlib module, and make_orchestrator should be typed based on its fixture definition). Additionally, add explicit -> None return type annotation to all four test functions: test_credential_finding_spawns_lateral_movement_task, test_ssrf_finding_spawns_internal_recon_task, test_chained_tasks_added_to_ptt, and test_lateral_movement_chain_context_routes_to_lateral_prompt to comply with MyPy strict typing requirements.Source: Coding guidelines
tests/agents/test_session_failure_gate.py-6-6 (1)
6-6: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd full type annotation to the test function.
Use an explicit return type (
-> None) to satisfy strict MyPy enforcement for new Python code.As per coding guidelines, "Enforce strict MyPy typing — all new code must be fully typed."
Suggested patch
-def test_session_blocks_after_threshold(): +def test_session_blocks_after_threshold() -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/agents/test_session_failure_gate.py` at line 6, The function test_session_blocks_after_threshold() is missing an explicit return type annotation. Add the return type annotation -> None to the function definition to comply with strict MyPy typing requirements. This ensures the test function is fully typed as per the coding guidelines for new code.Source: Coding guidelines
tests/agents/test_ptt_node_id.py-6-10 (1)
6-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winType-annotate the test function signature.
test_sequential_node_ids_are_unique_before_commitis currently untyped and should include explicit parameter and return annotations under strict MyPy.As per coding guidelines, “Enforce strict MyPy typing — all new code must be fully typed.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/agents/test_ptt_node_id.py` around lines 6 - 10, Add type annotations to the test_sequential_node_ids_are_unique_before_commit function signature. The tmp_path parameter should be explicitly typed as Path (from pathlib), and the function return type should be annotated as None to comply with strict MyPy typing requirements.Source: Coding guidelines
tests/orchestrator/test_selector_wiring.py-9-16 (1)
9-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit type annotations to the new test functions.
These new tests are untyped, which conflicts with the repo’s strict MyPy rule for new Python code.
As per coding guidelines, “Enforce strict MyPy typing — all new code must be fully typed.”
Also applies to: 20-32, 35-43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/orchestrator/test_selector_wiring.py` around lines 9 - 16, The test function test_selector_recommendation_stored_on_task is missing explicit type annotations required by the repository's strict MyPy policy. Add type annotations to the make_orchestrator parameter and specify an explicit return type annotation of None for this async test function. This same issue also applies to other new test functions mentioned in the comment (lines 20-32 and 35-43), so add appropriate type annotations to all of them as well.Source: Coding guidelines
pentestgpt/knowledge/domains/cicd.py-316-320 (1)
316-320:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
detect_domain_applicabilityshould score string details too.The current parser only builds
blobfor dictdetail; string details are effectively discarded, lowering CI/CD detection confidence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/knowledge/domains/cicd.py` around lines 316 - 320, The blob construction in the detail handling section only processes dictionary-type details, which causes string details to be discarded and reduces CI/CD detection confidence. Modify the blob assignment line to handle both cases: when detail is a dictionary, join its values as currently done; when detail is a string, include that string value directly in the blob instead of leaving it empty. This ensures all detail information, whether dict or string, is included in the domain applicability scoring.pentestgpt/knowledge/domains/deserialization.py-292-296 (1)
292-296:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude non-dict
detailpayloads in matching.String
detailvalues are currently ignored, so deserialization markers in plain-text findings may never contribute to applicability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/knowledge/domains/deserialization.py` around lines 292 - 296, The current logic in the detail extraction and blob creation discards string-type detail values by converting them to empty dictionaries, which prevents deserialization markers in plain-text detail findings from contributing to applicability matching. Modify the blob construction logic to preserve and include string detail values directly in the blob variable. When detail is a string, append it to the blob; when detail is a dict, extract and join its values as currently done; ensure both string and dict detail payloads are properly included in the final blob used for matching.pentestgpt/knowledge/adaptive_technique_selector.py-711-713 (1)
711-713:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
defenses_detectedbefore iterating.If
defenses_detectedis passed as a single string, this currently iterates characters and can misclassify defenses.Suggested fix
- defenses = [str(d).lower() for d in real_time_context.get("defenses_detected", [])] + raw_defenses = real_time_context.get("defenses_detected", []) + if isinstance(raw_defenses, str): + raw_defenses = [raw_defenses] + elif not isinstance(raw_defenses, list): + raw_defenses = [] + defenses = [str(d).lower() for d in raw_defenses]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/knowledge/adaptive_technique_selector.py` around lines 711 - 713, The defenses_detected value retrieved from real_time_context may be passed as a single string instead of a list, causing character-level iteration in the list comprehension on the defenses variable. Before iterating, normalize the value by checking if it is a string and wrapping it in a list if so, otherwise use the value as-is. This ensures that whether defenses_detected is a single string, a list of strings, or empty, it is properly handled as a list before the lowercase string conversion in the list comprehension.pentestgpt/knowledge/domains/api_graphql.py-143-147 (1)
143-147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle string
detailvalues in applicability scoring.Current logic ignores non-dict
detailvalues, which can drop valid API/GraphQL indicators and under-score applicability.Suggested fix
- detail = getattr(f, "detail", None) - if not isinstance(detail, dict): - detail = f.get("detail", {}) if isinstance(f, dict) else {} - blob = " ".join(str(v) for v in detail.values()) if isinstance(detail, dict) else "" + detail = getattr(f, "detail", None) + if detail is None and isinstance(f, dict): + detail = f.get("detail") + if isinstance(detail, dict): + blob = " ".join(str(v) for v in detail.values()) + elif detail is None: + blob = "" + else: + blob = str(detail)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/knowledge/domains/api_graphql.py` around lines 143 - 147, The applicability scoring logic in the section handling the detail attribute ignores string-type detail values, which causes valid API/GraphQL indicators to be dropped. In the blob assignment following the isinstance check on detail, modify the conditional logic to handle string detail values directly by including them in the blob. Currently, only dict detail values are being processed (by joining their values), while string or other non-dict types result in an empty blob. Ensure that if detail is a string, it is included in the blob variable; if it is a dict, join its values as currently done; otherwise use an empty string. This will prevent loss of information that should contribute to applicability scoring.
🟡 Minor comments (9)
tests/agents/parsers/test_wpscan_parser.py-73-84 (1)
73-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the command-target fallback with at least one emitted finding.
Line 147 says this test validates
_target_from_command, but Line 150 only checks that parsing returns a list. BecauseSAMPLE_NO_URLcurrently yields zero findings, the fallback target path is never asserted.💡 Proposed fix
@@ SAMPLE_NO_URL = """\ { "interesting_findings": [], "version": { "number": "5.9.0", "vulnerabilities": [] }, "plugins": {}, - "users": {} + "users": { + "staging-admin": { + "id": 1, + "login": "staging-admin" + } + } } """ @@ def test_parse_target_from_command_when_no_url_in_json() -> None: """When target_url is absent, fall back to _target_from_command.""" findings = WpscanParser().parse(SAMPLE_NO_URL, command="wpscan --url http://staging.internal/ --format json") - # No vulns or users in this sample, just assert it doesn't crash - assert isinstance(findings, list) + assert len(findings) >= 1 + assert all(f.target == "http://staging.internal/" for f in findings)Also applies to: 146-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/agents/parsers/test_wpscan_parser.py` around lines 73 - 84, The SAMPLE_NO_URL test data currently yields zero findings, so the _target_from_command fallback mechanism is never actually exercised during parsing. Add at least one finding to the SAMPLE_NO_URL JSON (such as adding an entry to interesting_findings or vulnerabilities collection) so that when parsed, it generates findings that trigger the fallback target path. Then update the test assertion around line 150 to verify that the emitted findings have the correct target value populated from the command fallback instead of just checking that a list is returned.tests/agents/test_exploit_developer_deps.py-6-16 (1)
6-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explicit test function return annotations for strict MyPy.
Please annotate new test functions with
-> Noneto satisfy strict typing on new code.As per coding guidelines, "Enforce strict MyPy typing — all new code must be fully typed".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/agents/test_exploit_developer_deps.py` around lines 6 - 16, Both test functions test_exploit_developer_accepts_router_and_executor and test_exploit_developer_defaults_deps_to_none are missing return type annotations. Add `-> None` return type annotation to each function definition to comply with strict MyPy typing requirements for new code, ensuring the function signatures explicitly declare they return None.Source: Coding guidelines
tests/agents/test_learning_thresholds.py-4-9 (1)
4-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAnnotate the new test with
-> Noneto satisfy strict MyPy.Add an explicit return annotation on this test function.
As per coding guidelines, "Enforce strict MyPy typing — all new code must be fully typed".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/agents/test_learning_thresholds.py` around lines 4 - 9, The test function test_learning_config_has_threshold_fields is missing an explicit return type annotation required by strict MyPy typing. Add the return type annotation -> None to the function signature to indicate it returns nothing, ensuring compliance with the coding guidelines that require all new code to be fully typed.Source: Coding guidelines
tests/agents/test_exploit_generation.py-6-57 (1)
6-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winType-annotate new test functions to keep strict MyPy clean.
These new tests should use explicit
-> Nonereturn annotations.As per coding guidelines, "Enforce strict MyPy typing — all new code must be fully typed".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/agents/test_exploit_generation.py` around lines 6 - 57, Add explicit `-> None` return type annotations to all five test functions in this file: test_generated_exploit_varies_with_target, test_static_fallback_when_no_router, test_test_exploit_uses_executor_not_random, test_test_exploit_no_executor_returns_failure, and test_mutate_retry_eventually_succeeds. Each function definition should be updated to include `-> None` after the closing parenthesis of the function parameters and before the colon to ensure strict MyPy compliance.Source: Coding guidelines
tests/agents/test_failure_classification.py-9-33 (1)
9-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
-> Noneannotations to new tests for strict typing compliance.Please annotate these test functions explicitly to satisfy strict MyPy expectations.
As per coding guidelines, "Enforce strict MyPy typing — all new code must be fully typed".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/agents/test_failure_classification.py` around lines 9 - 33, The three test functions test_classify_failure_recognizes_known_reason, test_classify_failure_empty_is_unknown, and test_record_classifies_from_summary_key lack explicit return type annotations. Add the `-> None` return type annotation to each of these function definitions to satisfy strict MyPy typing requirements as per the coding guidelines.Source: Coding guidelines
pentestgpt/utils/ptt_manager.py-444-447 (1)
444-447:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSnapshot session arrays when building
chain_context.These fields are currently assigned by reference, so later mutations in
session_findingscan rewrite already-created task context and change downstream prompt behavior non-deterministically.Suggested patch
+from copy import deepcopy ... node.findings["chain_context"] = { "prompt_type": "lateral_movement", - "credentials": session_findings.get("credentials", []), - "discovered_hosts": session_findings.get("discovered_hosts", []), + "credentials": deepcopy(session_findings.get("credentials", [])), + "discovered_hosts": deepcopy(session_findings.get("discovered_hosts", [])), "current_access": {"task": completed_task.name, "target": sf.get("target")}, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/utils/ptt_manager.py` around lines 444 - 447, In the chain_context dictionary assignment within the node.findings["chain_context"] block, the "credentials" and "discovered_hosts" arrays are being assigned by reference from session_findings.get() calls. Create snapshots of these arrays instead by wrapping each session_findings.get() call with list() or using a similar copy mechanism to ensure that subsequent mutations to session_findings do not affect the already-created task context and cause non-deterministic prompt behavior downstream.pentestgpt/knowledge/domains/identity_provider.py-363-372 (1)
363-372:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize target host before composing enumeration URLs.
targetmay arrive as a full URL (https://idp.example.com/path), which currently produces malformed commands likehttps://https://idp.example.com/path/.... Parse/normalize to host first, then build command URLs.Suggested patch
+from urllib.parse import urlsplit from typing import Any @@ def get_enumeration_commands(self, context: dict[str, Any]) -> list[dict[str, Any]]: - host = context.get("target", "{host}") + raw_target = str(context.get("target", "{host}")).strip() + parsed = urlsplit(raw_target if "://" in raw_target else f"//{raw_target}") + host = parsed.netloc or parsed.path.split("/")[0] or "{host}" return [ { "command": f"curl -s https://{host}/.well-known/openid-configuration",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/knowledge/domains/identity_provider.py` around lines 363 - 372, The `host` variable at line 363 is directly using the target value from context without normalizing it first. If `target` arrives as a full URL like `https://idp.example.com/path`, the subsequent curl commands will have malformed URLs with duplicate protocols (e.g., `https://https://idp.example.com/...`). Before assigning to `host`, parse and extract just the hostname/domain portion from the `target` string, handling both cases where target is already just a hostname and where it's a complete URL with protocol and path components. Then use this normalized `host` value in all the command strings.docs/CHANGELOG.md-20-21 (1)
20-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse canonical repo paths for file references.
Line 20 and Line 21 use partial paths (
utils/...,agents/..., etc.) while the rest of the entry usespentestgpt/.... This makes lookup inconsistent and error-prone.Suggested fix
-- configurable retirement thresholds (`utils/learning_context_manager.py`, `agents/strategy_agent.py`, -- `config/knowledge_config.py`, `knowledge/learning_engine.py`). +- configurable retirement thresholds (`pentestgpt/utils/learning_context_manager.py`, +- `pentestgpt/agents/strategy_agent.py`, `pentestgpt/config/knowledge_config.py`, +- `pentestgpt/knowledge/learning_engine.py`).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/CHANGELOG.md` around lines 20 - 21, The file references in the CHANGELOG.md entry for configurable retirement thresholds use partial paths (utils/learning_context_manager.py, agents/strategy_agent.py, config/knowledge_config.py, knowledge/learning_engine.py) while the rest of the entry uses canonical full paths with the pentestgpt/ prefix. Update each of these four file path references by prepending pentestgpt/ to make them consistent with the canonical repository path format used elsewhere in the changelog entry.tests/monitoring/test_monitoring_components.py-1044-1044 (1)
1044-1044:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert correlation engine startup before benchmarking.
start_correlation()returns a success flag; without asserting it, this test can report performance numbers even when initialization failed.Suggested patch
- correlation_engine.start_correlation() + started = correlation_engine.start_correlation() + assert started, "Failed to start correlation engine for performance benchmark"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/monitoring/test_monitoring_components.py` at line 1044, The call to correlation_engine.start_correlation() in the test does not verify that startup was successful. Capture the return value from the start_correlation() method call and add an assertion to verify it returns True or indicates successful initialization before proceeding with the benchmarking code. This ensures the test only measures performance when the correlation engine has actually initialized correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f2de132-7a3d-4adc-8c7a-4390312f7ef4
📒 Files selected for processing (73)
CLAUDE.mddocs/CHANGELOG.mddocs/development/CODE_QUALITY.mdpentestgpt/agents/advanced_exploitation_agent.pypentestgpt/agents/agent_abstract.pypentestgpt/agents/command_generator.pypentestgpt/agents/exploit_developer.pypentestgpt/agents/parsers/bloodhound_parser.pypentestgpt/agents/parsers/certipy_parser.pypentestgpt/agents/parsers/enum4linux_ng_parser.pypentestgpt/agents/parsers/ffuf_parser.pypentestgpt/agents/parsers/getuserspns_parser.pypentestgpt/agents/parsers/netexec_parser.pypentestgpt/agents/parsers/nuclei_parser.pypentestgpt/agents/parsers/registry.pypentestgpt/agents/parsers/responder_parser.pypentestgpt/agents/parsers/secretsdump_parser.pypentestgpt/agents/parsers/wpscan_parser.pypentestgpt/agents/results_verifier.pypentestgpt/agents/strategy_agent.pypentestgpt/agents/tool_executor.pypentestgpt/config/knowledge_config.pypentestgpt/knowledge/adaptive_technique_selector.pypentestgpt/knowledge/domains/__init__.pypentestgpt/knowledge/domains/api_graphql.pypentestgpt/knowledge/domains/cicd.pypentestgpt/knowledge/domains/deserialization.pypentestgpt/knowledge/domains/identity_provider.pypentestgpt/knowledge/learning_engine.pypentestgpt/orchestrator.pypentestgpt/utils/learning_context_manager.pypentestgpt/utils/orchestrator_factory.pypentestgpt/utils/ptt_manager.pypentestgpt/utils/rag.pypyproject.tomlrequirements.txttests/agents/parsers/test_bloodhound_parser.pytests/agents/parsers/test_certipy_parser.pytests/agents/parsers/test_enum4linux_ng_parser.pytests/agents/parsers/test_ffuf_parser.pytests/agents/parsers/test_getuserspns_parser.pytests/agents/parsers/test_netexec_parser.pytests/agents/parsers/test_nuclei_parser.pytests/agents/parsers/test_responder_parser.pytests/agents/parsers/test_secretsdump_parser.pytests/agents/parsers/test_wpscan_parser.pytests/agents/test_exploit_developer_deps.pytests/agents/test_exploit_generation.pytests/agents/test_failure_classification.pytests/agents/test_file_ingestion.pytests/agents/test_learning_thresholds.pytests/agents/test_network_operations_agent.pytests/agents/test_ptt_node_id.pytests/agents/test_session_failure_gate.pytests/agents/test_tool_allowlist.pytests/knowledge/domains/test_api_graphql.pytests/knowledge/domains/test_cicd.pytests/knowledge/domains/test_deserialization.pytests/knowledge/domains/test_identity_provider.pytests/knowledge/domains/test_registration.pytests/knowledge/test_realtime_adaptation.pytests/monitoring/test_monitoring_components.pytests/orchestrator/conftest.pytests/orchestrator/test_defense_adaptation_wiring.pytests/orchestrator/test_exploitation_wiring.pytests/orchestrator/test_failure_reroute.pytests/orchestrator/test_finding_chaining.pytests/orchestrator/test_replanning.pytests/orchestrator/test_selector_wiring.pytests/orchestrator/test_session_findings_accumulator.pytests/utils/test_orchestrator_factory.pytests/utils/test_rag_domain_integration.pytests/utils/test_rag_loader.py
Replaced nested `"session_findings"` key with a spread so that `credentials`, `open_ports`, `discovered_hosts`, `vulnerabilities`, and `findings` are directly accessible in `target_info` passed to `command_generator.generate_commands`, fixing the KeyError in `test_accumulator_injected_into_target_info`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Pre-configure strategy/command-generator mocks before passing to make_orchestrator so Protocol-typed attributes are never directly reassigned; use patch.object for the internally-created ptt_manager. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…systems Namespace session findings under a dedicated key to prevent target_info key collisions, rebuild session state on explicit load, route failure classification through raw command output, deduplicate typed follow-up tasks, fix boolean-flag parsing in enum4linux-ng, narrow deserialization domain signal, and gate active exploitation tools behind an opt-in flag. Critical: - Namespace _session_findings under "session_findings" key in target_info - Call _rebuild_session_findings_from_ptt() in load_session() Major: - classify_failure now uses raw_output_text instead of LLM summary - Deduplicate credential/recon tasks in generate_typed_followup_tasks - Fix enum4linux-ng boolean flag misparsing (-A, -R, -v, etc.) - Replace overly broad "o:" deserialization signal with regex - Add logging/validation before LLM-generated exploit execution Minor: - Gate responder/evil-winrm behind active_exploitation constructor param - Cast predicted_effectiveness to float for JSON round-trip safety - Set MITRETactic.DISCOVERY on synthetic replan-anchor node - Fix netexec regex to capture passwords containing spaces - Reset stale client IP in responder parser state machine - Extract duplicated _load_json to base.load_json_with_fallback - Add logger.debug for RAG corpus loader failures - Persist _replan_count across session save/load - Use copy.deepcopy in _mutate_vulnerability Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Owner
Author
|
@coderabbitai full reivew |
✅ Action performedFull review finished. |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pentestgpt/agents/exploit_developer.py`:
- Around line 530-534: The logger.warning call in the exploit_developer.py file
is logging raw exploit code content via candidate.exploit_code[:200], which
poses a security risk by potentially leaking sensitive engagement data. Remove
the candidate.exploit_code[:200] from the log statement and replace it with safe
metadata instead, such as the length of the exploit code or a hash of it. Keep
logging the exploit_id and other non-sensitive metadata to maintain useful
debugging information while protecting sensitive payload content.
In `@pentestgpt/agents/parsers/bloodhound_parser.py`:
- Around line 94-115: The parse() method assumes meta, Properties, and RightName
are dictionaries or strings without validating their types, which can cause
crashes on malformed JSON. Add isinstance() type guards before calling .get() on
meta and the Properties dict, and before calling .lower() on right to ensure
they are the expected types. When these guards fail, skip processing that entry
(using continue) to maintain the parser's non-throwing contract and preserve
other valid findings from the same result.
In `@pentestgpt/agents/parsers/ffuf_parser.py`:
- Around line 52-57: The code on line 56 calls .get("results") on the `data`
variable without verifying it is a dictionary. If load_json_with_fallback
returns a valid JSON value that is not a dict (such as a list or string), this
will raise an AttributeError and violate the parser's contract of never raising
exceptions. Add a type check after the None check to guard against non-dict JSON
root types, returning an empty list if data is not a dictionary, similar to how
the None case is handled.
In `@pentestgpt/agents/parsers/wpscan_parser.py`:
- Around line 110-132: The parse() method lacks defensive checks for JSON shape
validation, which can cause exceptions when encountering malformed or partial
JSON data. In the version block section, add an isinstance check to ensure
version_block is actually a dict before calling .get("vulnerabilities") on it.
In the plugin loop, add an isinstance check to validate that vuln items are
dicts before passing them to _make_vuln_finding(). In the user enumeration
section starting at line 131, add an isinstance check to ensure users is
actually a dict before iterating over it with .items() or calling .keys(). Apply
the same defensive pattern to the code section mentioned at lines 177-194 to
prevent exceptions from being thrown and ensure partial findings are not
discarded when JSON structure is unexpected.
In `@pentestgpt/agents/results_verifier.py`:
- Around line 185-187: The artifact fallback logic that replaces empty `out`
with `self._read_output_artifact(cmd)` only updates the `out` variable used for
deterministic parsing, but leaves `raw_output_text` and non-technique analysis
payloads still referencing the original unmodified `results` which may be empty.
Update the code to propagate the artifact fallback replacement to all downstream
processing paths so that when `out` is populated from the artifact, the source
data feeding `raw_output_text` and failure classification is also updated
accordingly. Apply this fix at all occurrences where the artifact fallback
pattern is used (including the sections around lines 255-257 and line 277).
In `@pentestgpt/knowledge/domains/api_graphql.py`:
- Around line 125-135: The target input from context is being used directly
without normalization when building command URLs in the get_enumeration_commands
method. If the target already includes a scheme (like https://) or a path, this
results in malformed URLs such as https://https://... which breaks enumeration.
Extract and normalize the target value by removing any scheme prefix and path
components to obtain just the hostname, then use this cleaned host value when
constructing the curl command URLs.
In `@pentestgpt/knowledge/domains/cicd.py`:
- Around line 294-295: The prerequisites_met value retrieved from context needs
normalization before converting to a set because if it is explicitly None,
set(None) raises TypeError, and if it is a string, set(string) creates a set of
individual characters breaking the filtering logic. Modify the line where met is
assigned to validate and normalize prerequisites_met - ensure it is either a
list or tuple by checking its type, defaulting to an empty list if it is None or
not a valid iterable, and then convert the validated result to a set to
guarantee correct behavior in the all(p in met for p in t.prerequisites)
expression.
In `@pentestgpt/knowledge/domains/deserialization.py`:
- Around line 278-287: The get_enumeration_commands method extracts the target
value directly from context without normalizing it to remove scheme or path
components, causing malformed URLs when target includes https://, http://, or
path elements. Before using the target value to compose the command URLs,
extract and normalize it to contain only the hostname portion by removing any
scheme prefix and path components, then use this normalized host value in the
curl commands.
In `@pentestgpt/knowledge/domains/identity_provider.py`:
- Around line 383-387: The code in lines 384-387 is discarding non-dict detail
values by replacing them with empty dictionaries, which loses important signal
text needed for applicability scoring. Instead of overwriting non-dict details
with an empty dict, preserve the original detail value. Update the blob
construction logic to properly handle both dict details (extracting values) and
non-dict details (like strings) so that all detail information is retained in
the blob variable for applicability scoring.
In `@pentestgpt/orchestrator.py`:
- Around line 515-519: The _rebuild_session_findings_from_ptt method appends
findings to self._session_findings without clearing the existing findings first,
causing double-counting when the method is called during resume operations. Add
a clear operation on self._session_findings at the start of the
_rebuild_session_findings_from_ptt method before the loop that iterates through
completed nodes, ensuring a clean state before rebuilding from PTT nodes.
In `@pentestgpt/utils/ptt_manager.py`:
- Around line 432-434: The code assumes every entry in the structured_findings
list is a dictionary and directly calls .get() on it without validation. If any
parser emits a non-dict item, this will raise an AttributeError. Add a type
check using isinstance(sf, dict) before accessing sf.get("finding_type", "") to
guard against non-dictionary entries in the structured_findings list. This will
prevent the AttributeError and allow the follow-up task generation to continue
for the cycle.
In `@tests/agents/test_network_operations_agent.py`:
- Around line 567-574: The test mocks NetworkHelper.is_port_open but the
_discover_hosts method also calls NetworkHelper.reverse_dns, which performs real
DNS lookups and can cause the test to be flaky or slow. Add an additional patch
for NetworkHelper.reverse_dns alongside the existing is_port_open patch in the
patch context manager. The reverse_dns mock should return a simple hostname
string or None to prevent actual DNS resolution from occurring during the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43d82da2-6c19-4d6f-95d8-615d9f75ef45
📒 Files selected for processing (74)
CLAUDE.mddocs/CHANGELOG.mddocs/development/CODE_QUALITY.mdpentestgpt/agents/advanced_exploitation_agent.pypentestgpt/agents/agent_abstract.pypentestgpt/agents/command_generator.pypentestgpt/agents/exploit_developer.pypentestgpt/agents/parsers/base.pypentestgpt/agents/parsers/bloodhound_parser.pypentestgpt/agents/parsers/certipy_parser.pypentestgpt/agents/parsers/enum4linux_ng_parser.pypentestgpt/agents/parsers/ffuf_parser.pypentestgpt/agents/parsers/getuserspns_parser.pypentestgpt/agents/parsers/netexec_parser.pypentestgpt/agents/parsers/nuclei_parser.pypentestgpt/agents/parsers/registry.pypentestgpt/agents/parsers/responder_parser.pypentestgpt/agents/parsers/secretsdump_parser.pypentestgpt/agents/parsers/wpscan_parser.pypentestgpt/agents/results_verifier.pypentestgpt/agents/strategy_agent.pypentestgpt/agents/tool_executor.pypentestgpt/config/knowledge_config.pypentestgpt/knowledge/adaptive_technique_selector.pypentestgpt/knowledge/domains/__init__.pypentestgpt/knowledge/domains/api_graphql.pypentestgpt/knowledge/domains/cicd.pypentestgpt/knowledge/domains/deserialization.pypentestgpt/knowledge/domains/identity_provider.pypentestgpt/knowledge/learning_engine.pypentestgpt/orchestrator.pypentestgpt/utils/learning_context_manager.pypentestgpt/utils/orchestrator_factory.pypentestgpt/utils/ptt_manager.pypentestgpt/utils/rag.pypyproject.tomlrequirements.txttests/agents/parsers/test_bloodhound_parser.pytests/agents/parsers/test_certipy_parser.pytests/agents/parsers/test_enum4linux_ng_parser.pytests/agents/parsers/test_ffuf_parser.pytests/agents/parsers/test_getuserspns_parser.pytests/agents/parsers/test_netexec_parser.pytests/agents/parsers/test_nuclei_parser.pytests/agents/parsers/test_responder_parser.pytests/agents/parsers/test_secretsdump_parser.pytests/agents/parsers/test_wpscan_parser.pytests/agents/test_exploit_developer_deps.pytests/agents/test_exploit_generation.pytests/agents/test_failure_classification.pytests/agents/test_file_ingestion.pytests/agents/test_learning_thresholds.pytests/agents/test_network_operations_agent.pytests/agents/test_ptt_node_id.pytests/agents/test_session_failure_gate.pytests/agents/test_tool_allowlist.pytests/knowledge/domains/test_api_graphql.pytests/knowledge/domains/test_cicd.pytests/knowledge/domains/test_deserialization.pytests/knowledge/domains/test_identity_provider.pytests/knowledge/domains/test_registration.pytests/knowledge/test_realtime_adaptation.pytests/monitoring/test_monitoring_components.pytests/orchestrator/conftest.pytests/orchestrator/test_defense_adaptation_wiring.pytests/orchestrator/test_exploitation_wiring.pytests/orchestrator/test_failure_reroute.pytests/orchestrator/test_finding_chaining.pytests/orchestrator/test_replanning.pytests/orchestrator/test_selector_wiring.pytests/orchestrator/test_session_findings_accumulator.pytests/utils/test_orchestrator_factory.pytests/utils/test_rag_domain_integration.pytests/utils/test_rag_loader.py
Comment on lines
+185
to
187
| if not out.strip(): | ||
| out = self._read_output_artifact(cmd) | ||
| tool_name = self._extract_tool_name(cmd) |
There was a problem hiding this comment.
Propagate artifact fallback into the formatted/raw-output path.
You only replace out for deterministic parsing, but raw_output_text (and non-technique analysis payloads) still come from unmodified results. For file-only tools, downstream failure classification can still receive empty text.
🔧 Proposed fix
@@
- structured_findings: list[dict[str, Any]] = []
+ structured_findings: list[dict[str, Any]] = []
if "outputs" in results:
for output_entry in results["outputs"]:
cmd = output_entry.get("command", "")
out = str(output_entry.get("output", ""))
if not out.strip():
- out = self._read_output_artifact(cmd)
+ artifact_output = self._read_output_artifact(cmd)
+ if artifact_output:
+ out = artifact_output
+ output_entry["output"] = artifact_output
@@
+ formatted_output_text = self._format_results(results)
@@
- output_text = self._format_results(results)
+ output_text = formatted_output_text
@@
- analysis["raw_output_text"] = self._format_results(results)
+ analysis["raw_output_text"] = formatted_output_text
@@
- "raw_output_text": self._format_results(results),
+ "raw_output_text": formatted_output_text,Also applies to: 255-257, 277-277
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pentestgpt/agents/results_verifier.py` around lines 185 - 187, The artifact
fallback logic that replaces empty `out` with `self._read_output_artifact(cmd)`
only updates the `out` variable used for deterministic parsing, but leaves
`raw_output_text` and non-technique analysis payloads still referencing the
original unmodified `results` which may be empty. Update the code to propagate
the artifact fallback replacement to all downstream processing paths so that
when `out` is populated from the artifact, the source data feeding
`raw_output_text` and failure classification is also updated accordingly. Apply
this fix at all occurrences where the artifact fallback pattern is used
(including the sections around lines 255-257 and line 277).
Reconnaissance tests called NetworkHelper.is_port_open against 10.0.0.0/28, which fails in CI where no hosts respond. Mock NetworkHelper so discovery is deterministic. Lower correlation perf threshold from 100 to 50 events/sec since the 2s sleep in the timing window caps the theoretical max at ~250. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nd orchestrator - Remove raw exploit code from logs, log payload length instead - Add isinstance guards in bloodhound, ffuf, wpscan parsers for malformed JSON - Propagate artifact fallback into output_entry in results_verifier - Normalize target host in api_graphql and deserialization domain commands - Normalize prerequisites_met in all domain modules to prevent TypeError - Preserve non-dict detail values in identity_provider applicability scoring - Clear session findings before rebuild to prevent double-counting on resume - Guard structured_findings entries in ptt_manager before .get() calls - Mock reverse_dns in network operations test to prevent real DNS lookups Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… and tests Critical: wrap json.loads in verify_commands to prevent JSONDecodeError crash; replace asyncio.run_coroutine_threadsafe deadlock with ThreadPoolExecutor. High: serialize dict findings as JSON instead of repr; redact credential hashes/passwords before LLM prompt injection; remove contradictory sanitize/allowlist checks in execute_in_session; narrow blanket pipe pattern to pipe-to-shell only. Medium: fix _replan_count pop mutating live metrics dict; reset responder parser state after NTLMV2_HASH match; tighten BloodHound can_parse heuristic; add working_dir param to _read_output_artifact; correct MITRE technique IDs in CI/CD domain (T1072, T1552.001); fix learning data key-type mismatch. Low: add @pytest.mark.asyncio decorator; replace tempfile.mktemp with mkstemp; rewrite tautological factory tests to exercise real error paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pentestgpt/knowledge/adaptive_technique_selector.py (1)
705-705:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd type annotations for MyPy strict mode compliance.
The method signature lacks type annotations for both parameters and return value. As per coding guidelines, all new code must be fully typed for MyPy strict mode.
📝 Proposed type annotations
- def adjust_technique_real_time(self, real_time_context: dict[str, Any]) -> dict[str, Any]: + def adjust_technique_real_time(self, real_time_context: dict[str, Any]) -> dict[str, Any]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/knowledge/adaptive_technique_selector.py` at line 705, The adjust_technique_real_time method signature in adaptive_technique_selector.py must include proper type annotations for MyPy strict mode compliance. Ensure the method has explicit type hints for the real_time_context parameter (which should be dict[str, Any]) and the return type (which should also be dict[str, Any]). Update the method signature to include both the parameter type annotation and the return type annotation using the proper Python type hinting syntax with the arrow operator for the return type.Source: Coding guidelines
pentestgpt/agents/tool_executor.py (1)
147-166:⚠️ Potential issue | 🟡 MinorReview the threat model scope for interpreter pipe attacks.
The pattern change at line 163 from blocking all pipes to only blocking
| shand| bashcreates potential gaps for download-and-execute attacks via other interpreters. Existing patterns already blockwget | shandcurl | shspecifically (lines 157-158), but unblocked alternatives remain:
| python/| python3| perl| ruby| node| phpThese interpreters can execute similar injection attacks. If the threat model intentionally narrows scope to shell-only, document this decision. Otherwise, consider extending coverage to interpreter pipes:
re.compile(r"\|\s*(?:ba)?sh\b", re.IGNORECASE), + re.compile(r"\|\s*(?:python[23]?|perl|ruby|node|php)\b", re.IGNORECASE),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/agents/tool_executor.py` around lines 147 - 166, The regex pattern in the dangerous_patterns list that matches pipes to interpreters (the pattern with `r"\|\s*(?:ba)?sh\b"`) currently only blocks pipes to shell interpreters (sh and bash), but leaves gaps for download-and-execute attacks through other interpreters like python, perl, ruby, node, and php. Either extend this single regex pattern to include these additional interpreters in the character class or alternative group, or add separate patterns for each interpreter pipe combination (e.g., `| python`, `| python3`, `| perl`, `| ruby`, `| node`, `| php`). If narrowing scope to shell-only is intentional, document this threat model decision in a code comment.pentestgpt/agents/command_generator.py (1)
155-162:⚠️ Potential issue | 🔴 CriticalCredentials are exposed unredacted in lateral-movement prompts.
Line 161 forwards
chain_context["credentials"]directly toPentestGPTPromptV3.get_lateral_movement_prompt(), which interpolates the raw credential dictionaries into an f-string (line 387 of prompt_class_v3.py) without any masking. The resulting prompt is sent to the LLM, exposing passwords, API tokens, hashes, and private keys.The codebase has
redact_mapping()inpentestgpt/utils/learning_context_manager.pythat can redact sensitive keys. Apply redaction to the credentials before interpolation or filter out sensitive values from the credential descriptors before this call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pentestgpt/agents/command_generator.py` around lines 155 - 162, The credentials retrieved from chain_context are being passed unredacted to the get_lateral_movement_prompt method, which exposes sensitive information like passwords and API tokens to the LLM. Before passing credentials to the get_lateral_movement_prompt call within the lateral_movement prompt_type condition, apply the redact_mapping utility function from pentestgpt/utils/learning_context_manager.py to mask sensitive values in the credentials list. Replace the direct chain_context.get("credentials", []) argument with the redacted version to prevent exposure of sensitive data.
🧹 Nitpick comments (4)
tests/utils/test_orchestrator_factory.py (3)
363-372: ⚡ Quick winType annotation inconsistency.
The local variable
mock_moduleat line 363 lacks an explicit type annotation. As per coding guidelines, all new code must be fully typed for MyPy strict mode compliance.📝 Suggested type annotation
- mock_module = MagicMock() + mock_module: MagicMock = MagicMock()The test correctly validates that
_create_ml_agentreturnsNonewhen the constructor raises, matching the soft-fail contract in context snippet 2.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/test_orchestrator_factory.py` around lines 363 - 372, The variable mock_module in the test function lacks an explicit type annotation, which violates MyPy strict mode compliance requirements. Add a type annotation to the mock_module variable at its declaration point to specify that it is of type MagicMock, ensuring all new code includes complete type information as required by the coding guidelines.Source: Coding guidelines
316-329: ⚡ Quick winType annotation inconsistency.
The local variable
mock_moduleat line 318 lacks an explicit type annotation, whilecall_countat line 625 is annotated aslist[int]. As per coding guidelines, all new code must be fully typed for MyPy strict mode compliance.📝 Suggested type annotation
- mock_module = MagicMock() + mock_module: MagicMock = MagicMock()The soft-fail logic correctly validates that constructor exceptions are caught and the factory returns
(None, None, None)as specified in the upstream contract (context snippet 1).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/test_orchestrator_factory.py` around lines 316 - 329, The variable mock_module in the test_orchestrator_factory.py file lacks an explicit type annotation, which violates MyPy strict mode compliance requirements. Add a type annotation to the mock_module variable assignment (where it is initialized with MagicMock()) to specify that it is of type MagicMock. This ensures consistency with other annotated variables in the test suite and maintains full type coverage as required by the coding guidelines.Source: Coding guidelines
632-632: ⚡ Quick winType annotation inconsistency.
The local variable
mock_moduleat line 632 lacks an explicit type annotation, whilecall_countat line 625 is properly annotated. As per coding guidelines, all new code must be fully typed for MyPy strict mode compliance.📝 Suggested type annotation
- mock_module = MagicMock() + mock_module: MagicMock = MagicMock()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/test_orchestrator_factory.py` at line 632, The variable `mock_module` is missing an explicit type annotation required for MyPy strict mode compliance. Add a type annotation to the `mock_module` variable assignment to explicitly declare it as MagicMock, making it consistent with the type-annotated `call_count` variable in the same test function. This ensures the code meets the typing standards for new code in the test suite.Source: Coding guidelines
tests/monitoring/test_monitoring_components.py (1)
1035-1035: 💤 Low valueConsider moving the
dequeimport to the module level.The local import of
dequeon line 1035 works correctly but deviates from the typical Python convention of placing all imports at the top of the file. Sincedequeis a lightweight standard library data structure, moving it to the module-level imports would improve consistency.📦 Suggested import placement
Move the import to the top of the file alongside other standard library imports:
import os +from collections import dequeThen remove the local import from the test method.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/monitoring/test_monitoring_components.py` at line 1035, Move the local import statement `from collections import deque` from line 1035 to the module-level imports at the top of the test file alongside other standard library imports like collections. Remove the local import from inside the test method where it currently appears, ensuring the deque is available at the module level for use throughout the file. This follows Python convention of placing all imports at the module level rather than within function or method bodies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pentestgpt/agents/command_generator.py`:
- Around line 362-370: The ThreadPoolExecutor context manager automatically
calls shutdown(wait=True) on exit, which blocks even when the timeout is
reached, making the timeout on pool.submit().result(timeout=10) ineffective. The
TimeoutError exception is also not being caught by existing handlers. Replace
the context manager pattern with explicit executor management that calls
shutdown(wait=False) when a timeout occurs, and add a try-except block around
the pool.submit().result(timeout=10) call to catch and properly handle
TimeoutError while ensuring the executor is shut down with wait=False in the
exception handler.
---
Outside diff comments:
In `@pentestgpt/agents/command_generator.py`:
- Around line 155-162: The credentials retrieved from chain_context are being
passed unredacted to the get_lateral_movement_prompt method, which exposes
sensitive information like passwords and API tokens to the LLM. Before passing
credentials to the get_lateral_movement_prompt call within the lateral_movement
prompt_type condition, apply the redact_mapping utility function from
pentestgpt/utils/learning_context_manager.py to mask sensitive values in the
credentials list. Replace the direct chain_context.get("credentials", [])
argument with the redacted version to prevent exposure of sensitive data.
In `@pentestgpt/agents/tool_executor.py`:
- Around line 147-166: The regex pattern in the dangerous_patterns list that
matches pipes to interpreters (the pattern with `r"\|\s*(?:ba)?sh\b"`) currently
only blocks pipes to shell interpreters (sh and bash), but leaves gaps for
download-and-execute attacks through other interpreters like python, perl, ruby,
node, and php. Either extend this single regex pattern to include these
additional interpreters in the character class or alternative group, or add
separate patterns for each interpreter pipe combination (e.g., `| python`, `|
python3`, `| perl`, `| ruby`, `| node`, `| php`). If narrowing scope to
shell-only is intentional, document this threat model decision in a code
comment.
In `@pentestgpt/knowledge/adaptive_technique_selector.py`:
- Line 705: The adjust_technique_real_time method signature in
adaptive_technique_selector.py must include proper type annotations for MyPy
strict mode compliance. Ensure the method has explicit type hints for the
real_time_context parameter (which should be dict[str, Any]) and the return type
(which should also be dict[str, Any]). Update the method signature to include
both the parameter type annotation and the return type annotation using the
proper Python type hinting syntax with the arrow operator for the return type.
---
Nitpick comments:
In `@tests/monitoring/test_monitoring_components.py`:
- Line 1035: Move the local import statement `from collections import deque`
from line 1035 to the module-level imports at the top of the test file alongside
other standard library imports like collections. Remove the local import from
inside the test method where it currently appears, ensuring the deque is
available at the module level for use throughout the file. This follows Python
convention of placing all imports at the module level rather than within
function or method bodies.
In `@tests/utils/test_orchestrator_factory.py`:
- Around line 363-372: The variable mock_module in the test function lacks an
explicit type annotation, which violates MyPy strict mode compliance
requirements. Add a type annotation to the mock_module variable at its
declaration point to specify that it is of type MagicMock, ensuring all new code
includes complete type information as required by the coding guidelines.
- Around line 316-329: The variable mock_module in the
test_orchestrator_factory.py file lacks an explicit type annotation, which
violates MyPy strict mode compliance requirements. Add a type annotation to the
mock_module variable assignment (where it is initialized with MagicMock()) to
specify that it is of type MagicMock. This ensures consistency with other
annotated variables in the test suite and maintains full type coverage as
required by the coding guidelines.
- Line 632: The variable `mock_module` is missing an explicit type annotation
required for MyPy strict mode compliance. Add a type annotation to the
`mock_module` variable assignment to explicitly declare it as MagicMock, making
it consistent with the type-annotated `call_count` variable in the same test
function. This ensures the code meets the typing standards for new code in the
test suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e2f9ba5-2e12-4a26-834f-1400740026ff
📒 Files selected for processing (19)
pentestgpt/agents/command_generator.pypentestgpt/agents/exploit_developer.pypentestgpt/agents/parsers/bloodhound_parser.pypentestgpt/agents/parsers/ffuf_parser.pypentestgpt/agents/parsers/responder_parser.pypentestgpt/agents/parsers/wpscan_parser.pypentestgpt/agents/results_verifier.pypentestgpt/agents/tool_executor.pypentestgpt/knowledge/adaptive_technique_selector.pypentestgpt/knowledge/domains/api_graphql.pypentestgpt/knowledge/domains/cicd.pypentestgpt/knowledge/domains/deserialization.pypentestgpt/knowledge/domains/identity_provider.pypentestgpt/orchestrator.pypentestgpt/utils/ptt_manager.pytests/agents/test_network_operations_agent.pytests/monitoring/test_monitoring_components.pytests/orchestrator/test_session_findings_accumulator.pytests/utils/test_orchestrator_factory.py
🚧 Files skipped from review as they are similar to previous changes (13)
- pentestgpt/agents/parsers/ffuf_parser.py
- tests/orchestrator/test_session_findings_accumulator.py
- pentestgpt/agents/parsers/bloodhound_parser.py
- pentestgpt/utils/ptt_manager.py
- pentestgpt/agents/parsers/responder_parser.py
- pentestgpt/agents/results_verifier.py
- pentestgpt/knowledge/domains/cicd.py
- pentestgpt/agents/parsers/wpscan_parser.py
- pentestgpt/knowledge/domains/api_graphql.py
- pentestgpt/knowledge/domains/deserialization.py
- pentestgpt/knowledge/domains/identity_provider.py
- pentestgpt/agents/exploit_developer.py
- pentestgpt/orchestrator.py
…ool executor, and tests - Redact credentials before passing to lateral-movement LLM prompt (security) - Fix ThreadPoolExecutor timeout by using explicit shutdown(wait=False) on timeout - Extend dangerous pipe patterns to block python/perl/ruby/node/php interpreters - Add MagicMock type annotations for mypy strict compliance in test files - Move deque import to module level in test_monitoring_components Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
FastAPI's File/Form/UploadFile requires python-multipart at runtime, but it was only listed as an optional extra of FastAPI, not as a direct project dependency. This caused 3 TestPluginSystemEndpoints tests to fail in CI with "Form data requires python-multipart to be installed." Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Raises real-world exploit success in authorized engagements via surgical, additive changes to the existing 5-agent orchestration loop — making findings flow forward across iterations, failures reroute intelligently, orphaned subsystems wire into the live loop, and tool/knowledge coverage expand. Plus a security + quality follow-up pass (dependency CVE fixes, docs, lint/config alignment).
Exploitation capability — 12 phases
_generate_node_idcollision fix.langchain.text_splitterimport that kept the vectorstore from ever initializing).Follow-up pass (post-implementation)
CLAUDE.mdparser list (now 17 tools) +docs/CHANGELOG.mdentry covering all 12 phases.langchain1.2.15 → 1.3.9 (SQLi),langchain-core1.2.30 → 1.4.7 (unsafe deserialization),python-dotenv1.2.1 → 1.2.2 (arbitrary file overwrite); pinnedparamiko==4.0.0.safetynow reports 0 vulnerabilities (was 3).[tool.pydocstyle](convention=google) to match ruff; fixed a stalesafety checkdoc reference.Verification
./venv/bin/ruff check .— clean./venv/bin/mypy .(strict) — no issues in 596 source files./venv/bin/pytest— 5238 passed, 11 skipped, 0 failedsafety— 0 vulnerabilities; markdownlint over tracked files — 0 issues--help/--validate-config, core subsystems — all pass.Test plan
Notes
pentestgpt/test_connection.pyreferences a non-existentpentestgpt.utils.pentest_orchestratormodule (real module ispentestgpt.orchestrator). Not addressed here.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes