Attempts to split gito into multiple personas.#54
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🔒 Security Reviewer4 issues found Arbitrary Code Execution via post_process Configuration📍 .github/gito-agents/defender.toml:5 The 'post_process' field in .github/gito-agents/defender.toml contains a Python code string that is executed by the gito-agent tool. If an attacker can modify this configuration file (e.g., via a malicious pull request in a CI/CD environment), they can execute arbitrary Python code, leading to potential secret theft, malware deployment, or system compromise. Arbitrary code execution via post_process script📍 .github/gito-agents/linter.toml:5 The Arbitrary code execution via malicious module import📍 .github/gito-agents/security.toml:5 The post_process script inserts the repository's .github/scripts directory into the Python path and imports gito_filter. This allows an attacker to execute arbitrary code by adding a malicious gito_filter.py file in that directory in a pull request. The code will run with the privileges of the CI runner, potentially leading to secret exfiltration or repository modification. Untrusted code execution in non-fork pull requests📍 .github/workflows/gito-mentions.yml:30 The GitHub Actions workflow conditionally restores trusted versions of the .github/scripts/ directory only for pull requests from forks. For pull requests from branches within the same repository, the workflow executes the handle_mentions.py script from the PR branch without restoration. This allows arbitrary code execution with access to sensitive secrets (LLM_API_KEY, GITHUB_TOKEN) if a malicious or compromised collaborator creates a PR with a modified script and triggers the workflow by commenting. |
🛡️ Defender Against the Dark Arts9 issues found Security reviewer explicitly excludes malicious code detection📍 .github/gito-agents/security.toml:45 The prompt_vars.requirements for the AI security reviewer lists 'Malicious code or intentional backdoors' under OUT OF SCOPE, meaning the reviewer will intentionally not report such issues. This configuration directly undermines the repository's defense against backdoors, supply chain compromises, and insider threats, allowing malicious code to evade detection. Suspicious package name with dot: gito.botThe package name 'gito.bot' contains a dot, which is highly unusual for standard PyPI packages unless it belongs to a well-known namespace (e.g., 'zope.interface'). 'gito' is not a recognized namespace, and the name closely resembles a misspelling of 'git' combined with 'bot', suggesting potential typosquatting or a malformed package intended to deceive. This poses a significant supply chain risk as it could lead to the installation of malicious code. Path Traversal via Unsanitized AGENT_NAME📍 .github/scripts/collapse_agent_reviews.py:183 The AGENT_NAME environment variable is used directly in constructing the report file path without sanitization. An attacker who can control this variable (e.g., via a compromised CI configuration) could read arbitrary files on the runner's filesystem by injecting path traversal sequences such as '../../'. This may lead to disclosure of sensitive data including tokens, source code, or other repository contents. Potential Path Traversal in Agent Name Handling📍 .github/scripts/handle_mentions.py:39 The agent name derived from TOML filename stems is used in directory path construction (e.g., Unintended Logging of Potentially Sensitive Comment Content📍 .github/scripts/handle_mentions.py:222 In Potential supply chain risk: usage of external AI service API📍 .github/workflows/gito-code-review.yml:40 The workflow relies on an external AI service (api.ppq.ai) for code reviews. The hardcoded API endpoint and model (stepfun/step-3.5-flash) are not from a widely recognized, audited provider. If the service or model is compromised, malicious code could be injected into reviews, or code could be exfiltrated via the LLM API. The secret PPQ is used for authentication, but the service provider itself is opaque. Insider threat risk: matrix agents with elevated trust📍 .github/workflows/gito-code-review.yml:33 The matrix includes an agent named 'defender', which implies high-privilege review capabilities. If an insider compromises the configuration file .github/gito-agents/defender.toml (not shown in diff), they could manipulate review outcomes. The workflow trusts these configs without validation, and they are used for both forked and internal PRs (with restoration only for forks). An internal malicious contributor could modify the config to hide vulnerabilities or approve malicious code. Potential backdoor: unrestricted Python script execution📍 .github/workflows/gito-code-review.yml:48 The step 'Collapse previous ${{ matrix.agent }} reviews' runs python .github/scripts/collapse_agent_reviews.py with GITHUB_TOKEN and PR_NUMBER. The script is not part of the diff but is executed in the workflow. If an insider or compromised account modifies this script (e.g., to exfiltrate PR data or tamper with reviews), it would run with write permissions to the PR. The script is only executed on 'success' of the review step, but that does not mitigate the risk if the script itself is malicious. Fork PR trusted files restore uses incorrect remote leading to bypass of security control📍 .github/workflows/gito-mentions.yml:29 The step 'Restore trusted repository files' attempts to revert critical files (.github/gito-agents, .github/requirements.txt, .github/scripts/) to the base repository's version using 'git checkout origin/main'. However, when triggered by a PR from a fork, the 'origin' remote points to the forked repository, not the base repository. As a result, the command either fails (if the fork lacks a main branch) or restores the fork's own version of these files. This completely bypasses the intended security control that aims to prevent untrusted code from forks from being executed. Subsequent steps install and run code from these files with write permissions via GITHUB_TOKEN, potentially allowing an attacker to execute arbitrary code in the repository context. |
🧠 Wise Man6 issues found Avoid embedding executable code in configuration📍 .github/gito-agents/bugs.toml:5 The Incorrect error recovery in collapse_comment leads to partial updates and stale state📍 .github/scripts/collapse_agent_reviews.py:114 When the comment body update succeeds (HTTP 200) but the subsequent minimize_comment GraphQL operation fails, collapse_comment returns False while the comment body has already been permanently modified to include the collapsed header. This creates a partially updated state where the comment appears collapsed (due to HTML) but is not actually minimized by GitHub, and the script will never retry minimization because is_already_collapsed() will detect the header and skip the comment in future runs. The function should return True if the body update succeeds, treating minimization as a best-effort operation with proper warning logging. Hardcoded relative path to collapse script📍 .github/scripts/handle_mentions.py:178 The path to the collapse_agent_reviews.py script is hardcoded as a string literal relative to the current working directory. This makes the script brittle if the working directory differs from the repository root or if the script is moved. Compute the path relative to the current file using pathlib to ensure robustness: str(pathlib.Path(file).parent / 'collapse_agent_reviews.py'). Duplication of 'github-comment' command string📍 .github/scripts/handle_mentions.py:158 The string literal 'github-comment' appears in two separate function calls to run_gito_command. This violates the DRY principle; any change to the command name would require updating multiple locations, increasing the risk of inconsistency. Extract the command name into a module-level constant (e.g., GITHUB_COMMENT_CMD) and use it in both places. Matrix agent list hardcoded and duplicated with file system📍 .github/workflows/gito-code-review.yml:18 The matrix strategy defines a hardcoded list of agents, while the agent configuration files are stored in .github/gito-agents/. This violates the DRY principle and risks configuration drift: if a new agent file is added without updating the matrix, or an agent is removed from the matrix but the file remains, the workflow may fail or skip expected reviews. Inconsistent step ID name📍 .github/workflows/gito-mentions.yml:21 The step ID 'determine_if_fork_or_branch' on line 21 inaccurately implies the step checks for both fork and branch conditions, but the logic exclusively determines if the PR is from a fork. This misnaming can lead to confusion during maintenance and does not accurately reflect the step's functionality. The corresponding reference on line 30 also uses the incorrect ID, requiring updates for consistency. |
🏛️ Architect8 issues found Hardcoded file system path in configuration📍 .github/gito-agents/defender.toml:7 The post_process configuration hardcodes the path '.github/scripts' for sys.path.insert. This creates a strong coupling to a specific directory structure, making the configuration non-portable and fragile to changes in the project layout. It violates the principle of separating configuration from environment-specific details. Executable code embedded in configuration📍 .github/gito-agents/defender.toml:5 The post_process field contains a multi-line string of Python code that is executed. This mixes configuration with logic, violating separation of concerns. It reduces testability, as the code is not in a module, and increases maintenance complexity. Configuration should be declarative data, not include executable code. Executable code embedded in configuration violates separation of concerns📍 .github/gito-agents/wise.toml:5 The 'post_process' field contains a multi-line Python script that modifies sys.path and imports modules. Configuration should be declarative data, not executable code. This anti-pattern leads to tight coupling between the configuration and the implementation language, makes the configuration harder to test and validate, introduces portability issues (hard-coded relative path), and poses security risks if the configuration is user-editable. A better approach is to have the configuration specify a separate script file path that the system can execute, keeping configuration and code separate. Shared working directory leads to state leakage between agents📍 .github/scripts/handle_mentions.py:27 The script uses a single global working directory ('.gito') for all agents processed in a single execution. Each agent's configuration is copied to this directory, but any additional state (caches, temporary files) written by the 'gito' commands persists across agents. This creates hidden coupling and can cause unpredictable behavior, violating the principle of isolation between independent agents. To ensure reliability, each agent should operate in a clean, isolated environment. A minimal fix is to clean the working directory before setting up each agent. Duplicate environment configuration across review steps📍 .github/workflows/gito-code-review.yml:39 The environment variables for the AI code review tool (LLM_API_KEY, LLM_API_TYPE, LLM_API_BASE, MODEL, GITHUB_TOKEN, PR_NUMBER) are duplicated in the 'Run ${{ matrix.agent }} review' and 'Post ${{ matrix.agent }} review' steps. This violates the DRY principle, creates a maintenance burden, and risks configuration drift if one copy is updated and the other is not. Hard-coded file paths scattered across workflow steps📍 .github/workflows/gito-code-review.yml:36 The workflow hard-codes repository paths for agent configuration (.github/gito-agents/), collapse script (.github/scripts/collapse_agent_reviews.py), output directories (review-${{ matrix.agent }}/), and artifact paths. These repeated literals tightly couple the workflow to a specific directory structure, making refactoring difficult and increasing the risk of errors when paths change. Implicit coupling between matrix agents and filesystem📍 .github/workflows/gito-code-review.yml:18 The matrix defines which agents to run (defender, security, etc.) but there is no validation or single source of truth ensuring that corresponding configuration files exist in .github/gito-agents/. This implicit contract can cause runtime failures if an agent name is added without a matching TOML file, or if a file is removed without updating the matrix. The architecture does not enforce consistency between the matrix and the filesystem. Incorrect Git Remote Reference for Forked Pull Requests📍 .github/workflows/gito-mentions.yml:31 The workflow assumes that 'origin/main' points to the base repository's main branch after checkout, but for forked pull requests, 'origin' points to the forked repository, not the base. This causes the 'Restore trusted repository files' step to potentially use untrusted files from the fork instead of the base repository, defeating the security intent. |
🔧 Expensive Linter3 issues found Missing blank lines between top-level function definitions📍 .github/scripts/collapse_agent_reviews.py PEP 8 requires two blank lines between top-level function definitions to improve readability. This file has no blank lines between consecutive functions (e.g., between lines 31 and 32, 50 and 51, etc.), violating the standard Python style guide. Typographical error in filename📍 .github/scripts/gito_filter.py The filename 'gito_filter.py' likely contains a typo; it should probably be 'git_filter.py' to align with standard Git-related terminology and avoid confusion. Use of tab characters for indentation📍 .github/scripts/handle_mentions.py:20 The code uses tab characters for indentation in multiple blocks, including function bodies (e.g., process_review_request), class definitions (AgentConfig), and multi-line function calls (logging.basicConfig). This violates PEP 8, which recommends using spaces for indentation to ensure consistency across editors and environments. All leading tabs should be replaced with 4 spaces. |
🐛 Bug Hunter12 issues found Post-process filtering excludes major severity functional errors📍 .github/gito-agents/bugs.toml:9 The post_process script calls filter_issues with Severity.MINOR, which filters out issues with severity higher than minor. According to the requirements in prompt_vars, all functional errors (incorrect calculations, failures, broken functionality) should be reported regardless of severity, as severity only indicates impact, not validity as a bug. This filtering causes major functional errors to be omitted from the report, resulting in incomplete bug detection and violating the scope to report all incorrect behavior. Indentation in post_process string causes Python IndentationError📍 .github/gito-agents/defender.toml:6 The multi-line string assigned to post_process contains indented Python code on lines 6-9. When the agent executes this string with exec(), the indentation will cause an IndentationError because Python does not allow indented statements at the top level. This results in a crash and prevents the post_process from filtering issues, leading to incorrect behavior where issues may not be processed as intended. Body update precedes minimization in collapse_comment📍 .github/scripts/collapse_agent_reviews.py:108 In collapse_comment, the comment body is updated to include the collapsed header before attempting to minimize the comment via minimize_comment. If the minimize step fails after the body update, the comment will have the collapsed header but remain unminimized. Since is_already_collapsed only checks the body for the collapsed header, the script will not retry minimization in subsequent runs, leaving the comment in a partially collapsed state where it appears collapsed but is not actually minimized by GitHub. Incorrect API endpoint for PR review comments fetch📍 .github/scripts/collapse_agent_reviews.py:137 The function fetch_all_comments uses the GitHub API endpoint for issue comments ('/issues/{pr_number}/comments') to fetch comments on a pull request. For pull request reviews, which are typically review comments, the correct endpoint is '/pulls/{pr_number}/comments'. This causes the script to miss review comments from agents, failing to collapse them as intended. Mention trigger pattern vulnerable to email address false positives📍 .github/scripts/handle_mentions.py:126 The regex pattern used to detect agent mentions in find_triggered_agents is r'@trigger\b'. This matches any occurrence of '@' followed by the trigger string, including within email addresses like 'user@trigger.com'. This causes agents to be triggered unintentionally when an email containing the trigger string is present in the comment. Review trigger pattern matches within email addresses📍 .github/scripts/handle_mentions.py:28 REVIEW_TRIGGER_PATTERN includes a plain word alternative that matches 'review' or 'code-review' as a word. This can trigger a review request when these words appear in an email address (e.g., 'my@review.com'), even though the user did not intend to request a review. Fix request pattern susceptible to email false positives📍 .github/scripts/handle_mentions.py:29 FIX_REQUEST_PATTERN as r'fix\b' will match the word 'fix' in email addresses like 'fix@example.com', causing the bot to incorrectly treat the request as a fix request (which is not supported) and skip further processing. Invalid ref in checkout step for pull_request events📍 .github/workflows/gito-code-review.yml:21 The checkout action sets the 'ref' parameter to an empty string for pull_request events, which is not a valid git reference. This will cause the checkout to fail, preventing the workflow from executing the code review for pull requests. Restore trusted files uses incorrect remote for fork PRs📍 .github/workflows/gito-code-review.yml:25 In the 'Restore trusted repository files' step, for fork PRs, it checks out 'origin/main', but 'origin' may point to the fork's repository instead of the base repository, leading to incorrect or untrusted files being restored. Incorrect permission key for pull requests📍 .github/workflows/gito-mentions.yml:7 The permissions block uses 'pull-requests' instead of the correct 'pull_requests', causing the job to lack write permissions for pull requests, leading to failures when attempting to modify PRs. gh CLI not installed before use📍 .github/workflows/gito-mentions.yml:22 The step 'Determine if PR is from fork' uses the gh command without installing it first, which will cause the step to fail with 'command not found' error. Hardcoded default branch name in git checkout📍 .github/workflows/gito-mentions.yml:31 The step 'Restore trusted repository files' uses 'origin/main' explicitly, which assumes the default branch is named 'main'. If the repository uses a different default branch name, this command will fail. |
| @@ -0,0 +1,119 @@ | |||
| ai-microcore==5.0.1 --hash=sha256:d3d48ae087f918f360fee51c20ec8dadd5fb23f969a444405e25cb2cc723ccab --hash=sha256:38d7ca5ade43c9db8bd8d70c6c72e1d0d786192b1a1e3444575bb521cc53bd32 | |||
There was a problem hiding this comment.
do we really need all these requirements?
There was a problem hiding this comment.
Annoyingly, yes. It is basically all of the gito stuff. Longer term the solution is to roll our own PR review bot that doesn't have a bazillion transitive dependencies, but that is going to take at least a week.
There was a problem hiding this comment.
is it possible to run this from some other git repository and it just have access to comment on prs?
There was a problem hiding this comment.
The right solution is to build our own system I think. The more I dig into this stuff, the more I realize how simple it actually all is under the hood. You are just cloning the repository and asking an AI to review it. You seed the AI with a file list of everything in the repository and you give it the tools to read whatever file it wants and use grep. After it is done it gives you its analysis.
The changes here were me trying to modify Gito to do what I wanted, and it ended up requiring a non-trivial amount of code be added. It was "quick" because AI vibe coded it, but then I spent 2 days iterating on that vibe coded hack to make it not as terrible.
If we were to build our own, I would pick a better language (Rust or TypeScript probably). It would not be a hard project with AI dev tools, and I'm not opposed to us setting aside some time to doing it properly.
There was a problem hiding this comment.
so should we abandon this pr and work on a better one, or do you feel this is usefl?
There was a problem hiding this comment.
I feel this is useful as it is, and I am OK with letting it take over my PR reviews. That being said, if we want to pivot to writing AI tooling I don't mind shelving this and building a better replacement instead of merging, but it means Zoltar PRs will likely get blocked until we finish that.
| trim_trailing_whitespace = true | ||
|
|
||
| [*.yml,*.yaml] | ||
| indent_style = space |
There was a problem hiding this comment.
YAML breaks with tabs, it has a hard requirement on spaces.
There was a problem hiding this comment.
and we cannot use json instead?
There was a problem hiding this comment.
I don't believe that GitHub workflows can be defined in JSON. Also, I suspect JSON would be significantly harder to read because there are a lot of inline multi-line things and JSON is very bad at handling multiline stuff.
No idea if this will work. Whole thing vibe coded by a new agent I am trying out. I'll be impressed if it works on the first try.