feat: Add AI-assisted GitHub issue agent POC#492
Conversation
|
Warning Review limit reached
More reviews will be available in 45 minutes and 20 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an end-to-end AI Access Control (AIAC) demo: an LLM-driven PolicyBuilder (LangGraph) that generates validated Keycloak YAML, Keycloak export/apply/delete tooling, provisioning scripts, a CLI pipeline, Kubernetes manifests, and full demo documentation. ChangesAI Access Control Agent + Keycloak Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
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 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
omerboehm
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (16)
authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py-27-37 (1)
27-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace Optional[X] with Python 3.12+ union syntax X | None.
The dataclass fields use
Optional[str]which is legacy syntax. Per coding guidelines, usestr | Noneinstead.♻️ Proposed fix
`@dataclass` class LLMConfig: """Configuration for LLM.""" model: str - endpoint: Optional[str] - api_key: Optional[str] + endpoint: str | None + api_key: str | None temperature: float max_tokens: int timeout: int retries: intAs per coding guidelines: "Python code must use Python 3.12+ syntax with type hints using modern union syntax (e.g.,
str | Noneinstead ofOptional[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 `@authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py` around lines 27 - 37, Update the LLMConfig dataclass to use Python 3.12+ union syntax for nullable fields: replace any occurrences of Optional[str] on the endpoint and api_key fields with str | None (leave model as str), and remove or adjust any unnecessary Optional imports; ensure the class name LLMConfig and field names endpoint and api_key are updated accordingly so type hints use the modern union form.Source: Coding guidelines
authbridge/demos/aiac-github-issue/aiac_agent/config/config_utils.py-15-15 (1)
15-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace legacy typing generic with Python 3.12+ built-in type.
The return type uses
Dict[str, Any]from the typing module. Per coding guidelines, usedict[str, Any]instead.♻️ Proposed fix
-def load_config(config_path: Path) -> Dict[str, Any]: +def load_config(config_path: Path) -> dict[str, Any]:As per coding guidelines: "Python code must use Python 3.12+ syntax with type hints using modern union syntax" — this includes using
dictinstead ofDict.🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/config/config_utils.py` at line 15, The function signature for load_config currently uses the legacy typing generic Dict[str, Any]; update its return annotation to the modern built-in form dict[str, Any] (and similarly replace any other occurrences of typing.Dict in the same file if present) so the signature becomes load_config(config_path: Path) -> dict[str, Any]; keep imports unchanged unless typing.Dict is no longer used anywhere in the module, in which case remove that import.Source: Coding guidelines
authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py-171-176 (1)
171-176:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace Optional with Python 3.12+ union syntax.
The function signature uses
Optional[str]andOptional[Path]which are legacy syntax. Per coding guidelines, usestr | NoneandPath | None.♻️ Proposed fix
def create_llm( - model_name: Optional[str] = None, - env_path: Optional[Path] = None, - yaml_path: Optional[Path] = None, + model_name: str | None = None, + env_path: Path | None = None, + yaml_path: Path | None = None, verbose: bool = True, ) -> BaseChatModel:As per coding guidelines: "Python code must use Python 3.12+ syntax with type hints using modern union syntax (e.g.,
str | Noneinstead ofOptional[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 `@authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py` around lines 171 - 176, The function signature for create_llm uses legacy typing Optional; update the annotations to Python 3.12+ union syntax by replacing Optional[str] with str | None and Optional[Path] with Path | None (and any other Optional usages in the same function signature or nearby vars), keeping the default values and return type BaseChatModel unchanged; ensure imports remain valid (remove unused Optional if present).Source: Coding guidelines
authbridge/demos/aiac-github-issue/aiac_agent/agent/state.py-37-43 (1)
37-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace legacy typing generics with Python 3.12+ built-in types.
The field type hints use
List[...]andDict[...]from the typing module. Per coding guidelines, Python 3.12+ code should use the lowercase built-in generic syntax:list[...]anddict[...].♻️ Proposed fix
- parsed_scopes: List[Dict[str, Any]] - policy_structure: Dict[str, Any] + parsed_scopes: list[dict[str, Any]] + policy_structure: dict[str, Any] yaml_output: str - messages: Annotated[List, add] # Annotated with 'add' for accumulation - errors: List[str] # NOT accumulated - replaced on each validation attempt + messages: Annotated[list, add] # Annotated with 'add' for accumulation + errors: list[str] # NOT accumulated - replaced on each validation attempt retry_count: int validation_passed: bool # Boolean flag for retry decision, not accumulatedAs per coding guidelines: "Python code must use Python 3.12+ syntax with type hints using modern union syntax (e.g.,
str | Noneinstead ofOptional[str])" — this includes usinglistanddictinstead ofListandDict.🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/agent/state.py` around lines 37 - 43, Replace typing module generics with Python 3.12+ builtins for the annotated fields in the state dataclass: change List[Dict[str, Any]] for parsed_scopes to list[dict[str, Any]], change Dict[str, Any] for policy_structure to dict[str, Any], change List for messages and errors to list, and any other List/Dict occurrences in this file to the lowercase builtins; keep Annotated[..., add], Any, and other types unchanged and ensure imports remove unused typing names if applicable.Source: Coding guidelines
authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py-70-70 (1)
70-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace Optional with Python 3.12+ union syntax.
The function signature uses
Optional[Path]which is legacy syntax. Per coding guidelines, usePath | None.♻️ Proposed fix
-def load_llm_config_from_yaml(model_name: str, yaml_path: Optional[Path] = None) -> LLMConfig: +def load_llm_config_from_yaml(model_name: str, yaml_path: Path | None = None) -> LLMConfig:As per coding guidelines: "Python code must use Python 3.12+ syntax with type hints using modern union syntax (e.g.,
str | Noneinstead ofOptional[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 `@authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py` at line 70, The function signature for load_llm_config_from_yaml uses legacy Optional[Path]; update the type hint to Python 3.12+ union syntax by replacing Optional[Path] with Path | None in the parameter list (i.e., change the yaml_path annotation in load_llm_config_from_yaml accordingly) and run a quick type-check to ensure no other references expect typing.Optional import.Source: Coding guidelines
authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py-40-40 (1)
40-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace Optional and Dict with Python 3.12+ syntax.
The function signature uses
Optional[Path]andDict[str, Any]which are legacy typing forms. Per coding guidelines, usePath | Noneanddict[str, Any].♻️ Proposed fix
-def load_llm_models_yaml(yaml_path: Optional[Path] = None) -> Dict[str, Any]: +def load_llm_models_yaml(yaml_path: Path | None = None) -> dict[str, Any]:As per coding guidelines: "Python code must use Python 3.12+ syntax with type hints using modern union syntax (e.g.,
str | Noneinstead ofOptional[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 `@authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py` at line 40, Update the type hints on the load_llm_models_yaml function signature to use Python 3.12+ native syntax: replace Optional[Path] with Path | None and replace Dict[str, Any] with dict[str, Any]; modify the function definition (load_llm_models_yaml) and any corresponding imports or annotations that reference Optional or Dict so they’re no longer required.Source: Coding guidelines
authbridge/demos/aiac-github-issue/aiac_agent/config/config_utils.py-34-36 (1)
34-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace legacy typing generics with Python 3.12+ built-in types.
The return type annotation mixes lowercase
tuple(correct) with uppercaseListandDict(legacy). Per coding guidelines, uselistanddictconsistently.♻️ Proposed fix
def extract_realm_roles_and_clients( config: Dict[str, Any], -) -> tuple[List[Dict[str, str]], Dict[str, List[Dict[str, str]]], Dict[str, List[str]]]: +) -> tuple[list[dict[str, str]], dict[str, list[dict[str, str]]], dict[str, list[str]]]:Also update line 35:
- config: Dict[str, Any], + config: dict[str, Any],As per coding guidelines: "Python code must use Python 3.12+ syntax with type hints using modern union syntax" — this includes using
listanddictinstead ofListandDict.🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/config/config_utils.py` around lines 34 - 36, The return type annotation on the function extract_realm_roles_and_clients uses legacy typing generics (List, Dict); update the signature to use built-in generics (list, dict) and modern typing where needed so the return type becomes tuple[list[dict[str, str]], dict[str, list[dict[str, str]]], dict[str, list[str]]]; keep the same ordering and semantics of the three returned structures and only change the type names (and import usage if present) to the Python 3.12+ built-ins.Source: Coding guidelines
authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py-104-104 (1)
104-104:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace Optional with Python 3.12+ union syntax.
The function signature uses
Optional[Path]which is legacy syntax. Per coding guidelines, usePath | None.♻️ Proposed fix
-def load_llm_config_from_env(env_path: Optional[Path] = None) -> LLMConfig: +def load_llm_config_from_env(env_path: Path | None = None) -> LLMConfig:As per coding guidelines: "Python code must use Python 3.12+ syntax with type hints using modern union syntax (e.g.,
str | Noneinstead ofOptional[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 `@authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py` at line 104, Update the function signature for load_llm_config_from_env to use Python 3.12+ union syntax by replacing Optional[Path] with Path | None for the env_path parameter, and remove the now-unused Optional import from typing (or update any other annotations that rely on Optional) so imports remain accurate; keep the function name and parameter name unchanged.Source: Coding guidelines
authbridge/demos/aiac-github-issue/aiac_agent/agent/state.py-9-10 (1)
9-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse Python 3.12+ built-in generic types instead of typing module classes.
The imports include
DictandListfrom typing, but per coding guidelines, Python 3.12+ code should use lowercase built-in types (dict,list) directly. Thetyping.Dictandtyping.Listforms are legacy syntax.♻️ Proposed fix
-from typing import Annotated, Any, Dict, List, TypedDict +from typing import Annotated, Any, TypedDictAs per coding guidelines: "Python code must use Python 3.12+ syntax with type hints using modern union syntax" — this includes using lowercase built-in generic types.
🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/agent/state.py` around lines 9 - 10, Replace legacy typing generics with built-in generics: remove Dict and List from the typing import and use the lowercase built-ins in annotations; update the import line in state.py (currently importing Dict and List) to omit them and change any uses of Dict[...] and List[...] in functions/classes in this module to dict[...] and list[...], keeping other typing names (Annotated, Any, TypedDict) as-is.Source: Coding guidelines
authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py-12-12 (1)
12-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse Python 3.12+ built-in types instead of typing module classes.
The imports include
DictandOptionalfrom typing. Per coding guidelines, Python 3.12+ code should usedictdirectly andX | Noneinstead ofOptional[X].♻️ Proposed fix
-from typing import Any, Dict, Optional +from typing import AnyAs per coding guidelines: "Python code must use Python 3.12+ syntax with type hints using modern union syntax (e.g.,
str | Noneinstead ofOptional[str])" — this includes usingdictinstead ofDict.🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py` at line 12, Replace typing imports with built-in types and modern union syntax: remove Dict and Optional from the import line in llm_config.py and use lowercase dict in type annotations and X | None for optional types throughout the module (search for any uses of Dict[...] and Optional[...] and update them accordingly); keep Any if still needed or replace with builtins when applicable.Source: Coding guidelines
authbridge/demos/aiac-github-issue/aiac_agent/config/config_utils.py-9-10 (1)
9-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse Python 3.12+ built-in generic types instead of typing module classes.
The imports include
DictandListfrom typing. Per coding guidelines, Python 3.12+ code should use lowercase built-in types (dict,list) directly instead of the legacy typing module classes.♻️ Proposed fix
-from typing import Any, Dict, List +from typing import AnyAs per coding guidelines: "Python code must use Python 3.12+ syntax with type hints using modern union syntax" — this includes using lowercase built-in generic types.
🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/config/config_utils.py` around lines 9 - 10, Replace usages of typing.Dict and typing.List in config_utils.py with the built-in generic types dict and list and remove Dict and List from the import list; update the import line to only import Any if still needed (or remove typing entirely if not), and update any type annotations in functions/classes that reference Dict or List (e.g., signatures or variable annotations) to use dict[...] and list[...] with the same type parameters.Source: Coding guidelines
authbridge/demos/aiac-github-issue/demo.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix user-facing grammar/spelling typos in setup instructions.
There are visible wording issues here (e.g., “AI based” and “githb”) that should be corrected for clarity in the walkthrough.
Also applies to: 396-396
🤖 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 `@authbridge/demos/aiac-github-issue/demo.md` at line 3, Fix the typos and wording in the demo text: change "AI based" to "AI-based" and correct "githb" to "GitHub" (and any other similar misspellings). Update the occurrences in authbridge/demos/aiac-github-issue/demo.md, including the instance around the shown line with the phrase "AI based" and the other occurrence around line 396, to ensure consistent, user-facing copy (use "AI-based" and "GitHub" throughout).Source: Linters/SAST tools
authbridge/demos/aiac-github-issue/demo.md-22-22 (1)
22-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize code block formatting for markdown rendering consistency.
Please add explicit language tags to fenced code blocks and convert the indented code block at Line 84 to fenced style so snippets render consistently and are easier to copy/run.
Also applies to: 84-84, 92-92, 211-211, 216-216, 229-229, 254-254, 268-268, 326-326, 341-341, 512-512, 530-530
🤖 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 `@authbridge/demos/aiac-github-issue/demo.md` at line 22, Convert all code snippets to fenced triple-backtick blocks and add explicit language tags (e.g., ```bash, ```js, ```python) for each fenced block; locate any indented code block (4-space or tab-prefixed) and replace it with a fenced block using the correct language tag, and then audit the file for other fenced blocks missing language tags and add the appropriate tag so all examples render consistently and are copy/paste-ready.Source: Linters/SAST tools
authbridge/demos/aiac-github-issue/aiac_agent/agent/graph.py-36-37 (1)
36-37:⚠️ Potential issue | 🟡 MinorUse Python 3.12 typing syntax in
authbridge/demos/aiac-github-issue/aiac_agent/agent/graph.pyThis file still uses
Optional/Dicttyping forms instead of the project’s modern Python 3.12 style.from typing import Any, Dict, OptionalUpdate:
llm: Optional[BaseChatModel] = None→llm: BaseChatModel | None = None(around 433-439)generate_policy(...) -> Dict[str, Any]→-> dict[str, Any](around 502-503)🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/agent/graph.py` around lines 36 - 37, Replace legacy typing imports and annotations in graph.py with Python 3.12 union and builtin generic syntax: remove Optional and Dict uses and update the annotation for the llm variable (llm: Optional[BaseChatModel] = None → llm: BaseChatModel | None = None) and the generate_policy signature (generate_policy(...) -> Dict[str, Any] → -> dict[str, Any]); search for other Optional[...] and Dict[...] occurrences (and any from typing import Optional, Dict) and convert them similarly to | unions and builtin dict/list generics, keeping the same types (e.g., Optional[X] → X | None, Dict[K, V] → dict[K, V]) so all annotations in the file use Python 3.12 style.Source: Coding guidelines
authbridge/demos/aiac-github-issue/keycloak_ops/delete_policy.py-9-10 (1)
9-10:⚠️ Potential issue | 🟡 MinorModernize typing generics in this module to the Python 3.12 style.
delete_policy.pystill usestyping.Dict/List/Set(from typing import Any, Dict, List, Set, plus-> Dict[...]/-> List[...]/Set[str]at lines 15, 22, and 75). Use built-in generics (dict[...],list[...],set[...]) instead to match modern typing conventions.🤖 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 `@authbridge/demos/aiac-github-issue/keycloak_ops/delete_policy.py` around lines 9 - 10, The module currently imports Dict/List/Set from typing and uses those in annotations; change the import to only "from typing import Any" and replace all uses of Dict[...] with dict[...], List[...] with list[...], and Set[...] (e.g. Set[str]) with set[str] in the function signatures and variable annotations in this file so annotations use the native generics (update the return types and parameter types where Dict/List/Set appear).Source: Coding guidelines
authbridge/demos/aiac-github-issue/policies/regular_policy.txt-1-2 (1)
1-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse official product capitalization in policy text.
Line 1 and Line 2 should use
GitHub(notgithub) for user-facing documentation quality.🤖 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 `@authbridge/demos/aiac-github-issue/policies/regular_policy.txt` around lines 1 - 2, Update the policy text in regular_policy.txt to use the official product capitalization "GitHub" instead of "github" in both bullet points (the line "* Members of the R&D can access both private and public github repositories" and the line "* Other technical personnel can access public github repositories only"); replace each occurrence of "github" with "GitHub" so user-facing documentation uses correct branding.Source: Linters/SAST tools
🧹 Nitpick comments (7)
authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py (1)
61-62: ⚡ Quick winSpecify encoding when opening text files.
The YAML file is opened without an explicit encoding parameter. This can cause cross-platform issues, as the default encoding varies by system.
♻️ Proposed fix
- with open(yaml_path, "r") as f: + with open(yaml_path, "r", encoding="utf-8") as f: config = yaml.safe_load(f)🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py` around lines 61 - 62, Open the YAML file using an explicit encoding to avoid cross-platform defaults: update the file-open call that currently uses open(yaml_path, "r") in llm_config.py (the block referencing yaml_path and yaml.safe_load) to pass encoding="utf-8" (i.e., open(yaml_path, "r", encoding="utf-8")). Ensure the rest of the logic (reading into config and yaml.safe_load) remains unchanged.authbridge/demos/aiac-github-issue/aiac_agent/config/config_utils.py (2)
72-77: ⚡ Quick winAdd defensive check for required 'name' key in role dictionaries.
When role is a dict, the code directly accesses
role["name"]without verifying the key exists. If the configuration contains a malformed role dict missing the "name" field, this will raise aKeyError.🛡️ Proposed fix
if isinstance(role, dict): # New format with name and description - realm_roles.append({"name": role["name"], "description": role.get("description", "")}) + if "name" not in role: + raise ValueError(f"Role dict missing required 'name' field: {role}") + realm_roles.append({"name": role["name"], "description": role.get("description", "")}) else:The same pattern should be applied to the client roles parsing at lines 91-93.
🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/config/config_utils.py` around lines 72 - 77, When iterating roles, don't access role["name"] blindly; add a defensive check that when role is a dict it contains a non-empty "name" key (and is a string), otherwise skip the entry or raise a clear ValueError/log a warning and continue; update the realm_roles append logic to use role.get("name") after validation and preserve role.get("description", "") for description. Apply the identical defensive validation to the client_roles parsing (the same pattern around the client role handling) so malformed dicts won't raise KeyError and will produce an explicit, actionable error or log entry.
30-31: ⚡ Quick winSpecify encoding when opening text files.
The file is opened without an explicit encoding parameter. This can cause cross-platform issues, as the default encoding varies by system (e.g., cp1252 on Windows, UTF-8 on Linux).
♻️ Proposed fix
- with open(config_path, "r") as f: + with open(config_path, "r", encoding="utf-8") as f: return yaml.safe_load(f)🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/config/config_utils.py` around lines 30 - 31, The file is opening the config file without specifying text encoding which can cause cross-platform issues; update the open call that reads config_path (the block returning yaml.safe_load(f)) to pass an explicit encoding (e.g., encoding="utf-8") when opening for reading, so change the open(...) in the function that reads config_path to open(config_path, "r", encoding="utf-8") and then call yaml.safe_load on the file handle as before.authbridge/demos/aiac-github-issue/aiac_agent/utils/validators.py (1)
10-23: ⚡ Quick winAlign validator type hints with Python 3.12 conventions.
Please replace
typing.Dict/List/Tupleusages with built-in generics in signatures and annotations.As per coding guidelines,
authbridge/**/*.pymust use Python 3.12+ type hint syntax.Also applies to: 99-105
🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/utils/validators.py` around lines 10 - 23, Update type hints to use Python 3.12 built-in generics instead of typing.Dict/List/Tuple: change the signature of validate_policy_structure to use dict[str, Any], list[dict[str, str]], list[str], and dict[str, list[dict[str, str]]], and replace other occurrences of typing.Dict/typing.List/typing.Tuple in this module (including the functions around the region reported) with the corresponding built-in generics; keep imports only for names you still need (e.g., Any) and remove unused typing imports.Source: Coding guidelines
authbridge/demos/aiac-github-issue/aiac_agent/prompts/prompt_builder.py (1)
9-16: ⚡ Quick winAdopt Python 3.12 typing syntax in these function signatures.
Please replace
typing.List/Dictstyle with built-in generics (list[...],dict[...]) to match repository typing standards.As per coding guidelines,
authbridge/**/*.pymust use Python 3.12+ type hint syntax (e.g., modern union syntax and contemporary annotations).Also applies to: 248-248
🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/prompts/prompt_builder.py` around lines 9 - 16, Update the type hints in build_system_prompt to use Python 3.12 built-in generics: replace typing.List and typing.Dict with list[...] and dict[...] for the parameters realm_roles, client_roles_map, and client_audience_targets; ensure any other signatures in this module using typing.List/Dict (e.g., the other function noted in the file) are updated similarly to match the repository's Python 3.12 typing style while keeping parameter names and return type unchanged.Source: Coding guidelines
authbridge/demos/aiac-github-issue/aiac_agent/utils/parsers.py (1)
11-15: ⚡ Quick winUpdate parser annotations to Python 3.12 style.
Use built-in generics (
list,tuple) and add an explicit-> Nonereturn annotation forprint_explanationto keep typing consistent with project standards.As per coding guidelines,
authbridge/**/*.pymust use Python 3.12+ type hint syntax.Also applies to: 100-100
🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/utils/parsers.py` around lines 11 - 15, The type annotations in extract_explanation_and_json and print_explanation should use Python 3.12 built-in generics and include an explicit return annotation: change Tuple[str, List] to tuple[str, list] in extract_explanation_and_json, and add -> None to the signature of print_explanation; update any matching import usage (remove typing.Tuple/List if now unused) and ensure the function signatures (extract_explanation_and_json and print_explanation) follow the new syntax consistently across the file.Source: Coding guidelines
authbridge/demos/aiac-github-issue/keycloak_ops/apply_policy.py (1)
73-73: ⚡ Quick winUse Python 3.12 union syntax in type hints.
Line 73 should use
dict[str, str] | Noneinstead ofOptional[Dict[str, str]]for consistency with the repository typing rule.As per coding guidelines,
authbridge/**/*.pymust use Python 3.12+ type hints with modern union syntax (str | Nonestyle).🤖 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 `@authbridge/demos/aiac-github-issue/keycloak_ops/apply_policy.py` at line 73, Update the type hint for the parameter scope_ids in apply_policy.py from Optional[Dict[str, str]] to the Python 3.12 union form dict[str, str] | None; also remove or adjust any now-unused typing imports (Optional, Dict) and ensure any other occurrences in the function signature or annotations use the modern union syntax (reference: the scope_ids parameter in the relevant function definition).Source: Coding guidelines
🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/agent/graph.py`:
- Around line 266-267: The retry logic is off-by-one: don't increment
retry_count before testing the budget; update the conditional to check whether
retry_count + 1 <= max_retries before returning a state with retry_count + 1
(i.e., replace the current "if structural_errors and retry_count < max_retries"
with a check that ensures the incremented count is still within budget), and
make the same change for the analogous check around retry_count usage at the
later block (the code around retry_count and max_retries at the other
occurrence).
In `@authbridge/demos/aiac-github-issue/aiac_agent/config/llm_conf.yaml.TEMPLATE`:
- Around line 10-12: Replace literal template tokens with env-expandable
placeholders: change the endpoint, api_key and temperature values in this LLM
config template from the current "<...>" text to ${ENV_VAR} style placeholders
(e.g. use ${LLM_ENDPOINT}, ${LLM_API_KEY}, ${LLM_TEMPERATURE} or other
project-standard names) so runtime/env substitution works; apply the same
replacement for the repeated blocks referenced (the occurrences for
endpoint/api_key/temperature at the other ranges) and ensure the YAML values
remain unquoted or appropriately quoted per YAML rules to allow env expansion.
In `@authbridge/demos/aiac-github-issue/aiac_agent/utils/validators.py`:
- Around line 283-288: The except block in validators.py currently returns
(True, True, "Verification skipped due to API rate limiting") for API errors
which incorrectly signals a successful semantic verification; change that return
so the validator does NOT mark the policy as valid — e.g., return (False, False,
"Verification skipped due to API rate limiting") — keeping the error message but
ensuring the boolean pass/fail flags reflect a non-successful verification
(update the tuple returned in the except Exception as e handler where "Already
borrowed" or "BadRequestError" is checked).
In `@authbridge/demos/aiac-github-issue/aiac.env`:
- Around line 3-4: The committed file contains hardcoded admin credentials
(KEYCLOAK_ADMIN_USERNAME and KEYCLOAK_ADMIN_PASSWORD); replace these literal
values with non-sensitive placeholders (e.g., empty values or template tokens)
and require callers to supply real secrets at runtime or via a secrets manager,
add a .env.template (or document in README) showing the placeholder keys, and
ensure the actual aiac.env is excluded from VCS (gitignored) so real credentials
are never committed.
In `@authbridge/demos/aiac-github-issue/config.yaml`:
- Line 6: Replace the hardcoded value for the YAML key "secret" (currently
"demo-ui-secret") with an env-expanded placeholder like ${AUTHBRIDGE_UI_SECRET}
in the config.yaml, and update the authbridge config-loading/startup parsing
logic that reads these YAML files to perform environment variable expansion so
the placeholder is resolved at runtime; ensure the loader treats ${VAR} syntax
(for keys such as "secret") and falls back or errors if the env var is missing
per existing validation rules.
In `@authbridge/demos/aiac-github-issue/k8s/git-issue-agent-deployment.yaml`:
- Around line 53-57: The container spec for the container named "agent" is
missing a hardened securityContext; update the Pod/Container spec (the container
with name "agent" in the Deployment manifest) to enforce non-root and disable
privilege escalation by adding a securityContext that sets runAsNonRoot: true,
specify a non-zero runAsUser (e.g. 1000) at pod or container level, set
allowPrivilegeEscalation: false, drop all Linux capabilities and enable
readOnlyRootFilesystem where appropriate; apply the same changes to the second
container block referenced (the other "agent" container at the later block).
In `@authbridge/demos/aiac-github-issue/k8s/github-tool-deployment.yaml`:
- Around line 43-49: Add a pod/container securityContext to enforce least
privilege for the github-tool container: inside the container spec for the
container named "github-tool" (the entry with image
ghcr.io/kagenti/agent-examples/github-tool:latest) add a securityContext that
sets runAsNonRoot: true, runAsUser (e.g., 1000) or runAsGroup as appropriate,
disallows privilege escalation with allowPrivilegeEscalation: false, drops
capabilities (capabilities: drop: ["ALL"]), and enables readOnlyRootFilesystem:
true (plus seccomp/apparmor profile if available); apply the same
securityContext to the other github-tool container block referenced (lines
78-85) so both containers enforce non-root, no privilege escalation, and
readonly root filesystem.
In `@authbridge/demos/aiac-github-issue/keycloak_ops/apply_policy.py`:
- Around line 93-94: The current broad except around adding a client role (the
block that prints "Client role '{client_name}.{role_name}' already in composite
or error: {e}") swallows all errors; change it to only handle the
idempotent/already-exists case (e.g., detect the API/HTTP 409 or the specific
Keycloak SDK exception for an existing composite) and log/continue, but re-raise
or propagate any other exceptions (network, auth, permission, etc.) so failures
don't get hidden; locate the try/except surrounding the composite role add
(references to client_name and role_name) and replace the generic except
Exception with a conditional catch/inspection of the error type/status and
re-raise unexpected errors.
- Around line 22-26: Validate the result of yaml.safe_load before using it:
after loading into policy_config in apply_policy.py, check that policy_config is
a mapping (e.g., isinstance(policy_config, dict)) and raise a clear ValueError
(including access_control_policy_file) if it's None or not a dict, then safely
read policy = policy_config.get("policy", {}) or {}; this ensures the code in
apply_policy.py fails fast with a clear validation error for empty or
non-mapping YAML documents.
In `@authbridge/demos/aiac-github-issue/keycloak_ops/delete_policy.py`:
- Around line 43-64: The try/except in remove_all_composites_from_realm_role is
swallowing errors (printing them and continuing) so composite-removal failures
become non-fatal; change the handler to surface failures instead of silently
continuing by either removing the try/except so exceptions propagate or by
re-raising after logging (e.g., log the error with context for
realm/realm_role_name and then raise), ensuring the call sites see the failure;
apply the same change to the other composite-removal block that uses
get_realm_role_composites and admin.connection.raw_delete so cleanup errors are
not hidden.
In `@authbridge/demos/aiac-github-issue/keycloak_ops/export_config.py`:
- Around line 156-157: The except block that currently swallows all errors (the
bare "except Exception: pass") during role discovery/audience inference must be
replaced so failures are visible and handled safely: catch the exception as e,
log it (using the module logger or processLogger) with context mentioning role
discovery/audience inference and the affected client_audience_targets, and then
either set client_audience_targets to a safe default (e.g., empty list) or
re-raise the exception depending on caller expectations; locate the try/except
around the role fetch/audience inference and update that handler to use "except
Exception as e: logger.exception(...)" and apply the safe default to
client_audience_targets instead of silently passing.
- Around line 199-209: The code loads the wrong env file causing missing creds;
update the load call in export_config.py so load_dotenv uses the demo env
filename ("aiac.env") instead of ".env" (locate the load_dotenv(script_dir /
".env") call), keep using script_dir and ensure KEYCLOAK_URL,
KEYCLOAK_ADMIN_USERNAME, and KEYCLOAK_ADMIN_PASSWORD checks remain unchanged so
export_config() will validate presence of credentials from aiac.env.
In `@authbridge/demos/aiac-github-issue/requirements.txt`:
- Line 2: Replace all non-exact dependency specs in the requirements file with
exact pins using == for the listed packages: change "pre-commit",
"langgraph>=0.2", "langchain-core>=0.2", "langchain-openai>=1.2", "pydantic>=2",
"PyYAML>=6", and "python-dotenv>=1" to concrete versions (e.g.
pre-commit==x.y.z, langgraph==x.y.z, langchain-core==x.y.z,
langchain-openai==x.y.z, pydantic==x.y.z, PyYAML==x.y.z, python-dotenv==x.y.z)
consistent with your project's lockfile or the latest tested compatible release;
ensure the exact == pins are committed in place of the current unpinned or >=
specs.
In `@authbridge/demos/aiac-github-issue/setup_keycloak.py`:
- Around line 309-313: The code currently prints plaintext secrets using the
client_secret variable in the conditional block around
client_config.get("secret"); remove any logging of client_secret (no f-strings
or prints that include the secret) and instead print a non-secret placeholder
such as " Secret: (set)" or " Secret: (preserved from existing client)"
depending on client_config.get("secret"); update the logic in the same
function/block in setup_keycloak.py to ensure client_secret is never written to
logs or stdout.
- Around line 446-465: The scope_ids map in create_client_scopes is keyed only
by scope_name so identical role names across clients collide; change the key to
a unique composite (e.g., "{client_name}:{scope_name}" or similar) when
assigning scope_ids and return that map, and then update any lookup sites that
currently expect plain scope_name (calls that use create_client_scopes results
and later bind scopes/audience) to construct the same composite key when
retrieving the scope id; locate create_client_scopes and
create_single_client_scope plus downstream bind/assignment code that consumes
scope_ids and ensure both creation and lookup use the same composite-key
convention so entries no longer overwrite each other.
- Around line 559-560: Replace the silent "except Exception: pass" in the
scope-assignment try/except blocks with proper error handling: catch Exception
as e and call the module logger (or processLogger) with logger.exception or
logger.error including contextual text (e.g., "failed assigning scope XYZ") so
the stack/exception is recorded, and re-raise or return a failure result for
non-idempotent provisioning errors; for idempotent/expected conflicts you may
suppress after logging. Update both occurrences that currently use "except
Exception: pass".
- Around line 49-55: load_main_config currently opens and parses YAML raw;
change it to read the file as text, expand ${ENV_VAR} (use os.path.expandvars or
equivalent) before calling yaml.safe_load, then validate a required "mode" field
in the resulting dict against the process startup mode (e.g.
os.getenv("STARTUP_MODE") or provided expected mode) and raise a descriptive
ValueError if missing or mismatched; keep the existing FileNotFoundError
behavior and ensure these checks live inside load_main_config so configs under
authbridge/**/*.{yaml,yml} support env expansion and startup mode validation.
- Around line 248-258: delete_realm_roles currently assumes realm_roles_config
contains strings, but the demo uses dicts like {"name", "description"} so
deletions fail; update delete_realm_roles to accept either strings or dicts and
extract the role name before calling KeycloakAdmin.delete_realm_role (e.g.,
check if each item is a dict and use item.get("name") or fallback to the item
itself), handle missing names by skipping/logging, and keep the existing
print/error messages referencing the resolved role_name.
---
Minor comments:
In `@authbridge/demos/aiac-github-issue/aiac_agent/agent/graph.py`:
- Around line 36-37: Replace legacy typing imports and annotations in graph.py
with Python 3.12 union and builtin generic syntax: remove Optional and Dict uses
and update the annotation for the llm variable (llm: Optional[BaseChatModel] =
None → llm: BaseChatModel | None = None) and the generate_policy signature
(generate_policy(...) -> Dict[str, Any] → -> dict[str, Any]); search for other
Optional[...] and Dict[...] occurrences (and any from typing import Optional,
Dict) and convert them similarly to | unions and builtin dict/list generics,
keeping the same types (e.g., Optional[X] → X | None, Dict[K, V] → dict[K, V])
so all annotations in the file use Python 3.12 style.
In `@authbridge/demos/aiac-github-issue/aiac_agent/agent/state.py`:
- Around line 37-43: Replace typing module generics with Python 3.12+ builtins
for the annotated fields in the state dataclass: change List[Dict[str, Any]] for
parsed_scopes to list[dict[str, Any]], change Dict[str, Any] for
policy_structure to dict[str, Any], change List for messages and errors to list,
and any other List/Dict occurrences in this file to the lowercase builtins; keep
Annotated[..., add], Any, and other types unchanged and ensure imports remove
unused typing names if applicable.
- Around line 9-10: Replace legacy typing generics with built-in generics:
remove Dict and List from the typing import and use the lowercase built-ins in
annotations; update the import line in state.py (currently importing Dict and
List) to omit them and change any uses of Dict[...] and List[...] in
functions/classes in this module to dict[...] and list[...], keeping other
typing names (Annotated, Any, TypedDict) as-is.
In `@authbridge/demos/aiac-github-issue/aiac_agent/config/config_utils.py`:
- Line 15: The function signature for load_config currently uses the legacy
typing generic Dict[str, Any]; update its return annotation to the modern
built-in form dict[str, Any] (and similarly replace any other occurrences of
typing.Dict in the same file if present) so the signature becomes
load_config(config_path: Path) -> dict[str, Any]; keep imports unchanged unless
typing.Dict is no longer used anywhere in the module, in which case remove that
import.
- Around line 34-36: The return type annotation on the function
extract_realm_roles_and_clients uses legacy typing generics (List, Dict); update
the signature to use built-in generics (list, dict) and modern typing where
needed so the return type becomes tuple[list[dict[str, str]], dict[str,
list[dict[str, str]]], dict[str, list[str]]]; keep the same ordering and
semantics of the three returned structures and only change the type names (and
import usage if present) to the Python 3.12+ built-ins.
- Around line 9-10: Replace usages of typing.Dict and typing.List in
config_utils.py with the built-in generic types dict and list and remove Dict
and List from the import list; update the import line to only import Any if
still needed (or remove typing entirely if not), and update any type annotations
in functions/classes that reference Dict or List (e.g., signatures or variable
annotations) to use dict[...] and list[...] with the same type parameters.
In `@authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py`:
- Around line 27-37: Update the LLMConfig dataclass to use Python 3.12+ union
syntax for nullable fields: replace any occurrences of Optional[str] on the
endpoint and api_key fields with str | None (leave model as str), and remove or
adjust any unnecessary Optional imports; ensure the class name LLMConfig and
field names endpoint and api_key are updated accordingly so type hints use the
modern union form.
- Around line 171-176: The function signature for create_llm uses legacy typing
Optional; update the annotations to Python 3.12+ union syntax by replacing
Optional[str] with str | None and Optional[Path] with Path | None (and any other
Optional usages in the same function signature or nearby vars), keeping the
default values and return type BaseChatModel unchanged; ensure imports remain
valid (remove unused Optional if present).
- Line 70: The function signature for load_llm_config_from_yaml uses legacy
Optional[Path]; update the type hint to Python 3.12+ union syntax by replacing
Optional[Path] with Path | None in the parameter list (i.e., change the
yaml_path annotation in load_llm_config_from_yaml accordingly) and run a quick
type-check to ensure no other references expect typing.Optional import.
- Line 40: Update the type hints on the load_llm_models_yaml function signature
to use Python 3.12+ native syntax: replace Optional[Path] with Path | None and
replace Dict[str, Any] with dict[str, Any]; modify the function definition
(load_llm_models_yaml) and any corresponding imports or annotations that
reference Optional or Dict so they’re no longer required.
- Line 104: Update the function signature for load_llm_config_from_env to use
Python 3.12+ union syntax by replacing Optional[Path] with Path | None for the
env_path parameter, and remove the now-unused Optional import from typing (or
update any other annotations that rely on Optional) so imports remain accurate;
keep the function name and parameter name unchanged.
- Line 12: Replace typing imports with built-in types and modern union syntax:
remove Dict and Optional from the import line in llm_config.py and use lowercase
dict in type annotations and X | None for optional types throughout the module
(search for any uses of Dict[...] and Optional[...] and update them
accordingly); keep Any if still needed or replace with builtins when applicable.
In `@authbridge/demos/aiac-github-issue/demo.md`:
- Line 3: Fix the typos and wording in the demo text: change "AI based" to
"AI-based" and correct "githb" to "GitHub" (and any other similar misspellings).
Update the occurrences in authbridge/demos/aiac-github-issue/demo.md, including
the instance around the shown line with the phrase "AI based" and the other
occurrence around line 396, to ensure consistent, user-facing copy (use
"AI-based" and "GitHub" throughout).
- Line 22: Convert all code snippets to fenced triple-backtick blocks and add
explicit language tags (e.g., ```bash, ```js, ```python) for each fenced block;
locate any indented code block (4-space or tab-prefixed) and replace it with a
fenced block using the correct language tag, and then audit the file for other
fenced blocks missing language tags and add the appropriate tag so all examples
render consistently and are copy/paste-ready.
In `@authbridge/demos/aiac-github-issue/keycloak_ops/delete_policy.py`:
- Around line 9-10: The module currently imports Dict/List/Set from typing and
uses those in annotations; change the import to only "from typing import Any"
and replace all uses of Dict[...] with dict[...], List[...] with list[...], and
Set[...] (e.g. Set[str]) with set[str] in the function signatures and variable
annotations in this file so annotations use the native generics (update the
return types and parameter types where Dict/List/Set appear).
In `@authbridge/demos/aiac-github-issue/policies/regular_policy.txt`:
- Around line 1-2: Update the policy text in regular_policy.txt to use the
official product capitalization "GitHub" instead of "github" in both bullet
points (the line "* Members of the R&D can access both private and public github
repositories" and the line "* Other technical personnel can access public github
repositories only"); replace each occurrence of "github" with "GitHub" so
user-facing documentation uses correct branding.
---
Nitpick comments:
In `@authbridge/demos/aiac-github-issue/aiac_agent/config/config_utils.py`:
- Around line 72-77: When iterating roles, don't access role["name"] blindly;
add a defensive check that when role is a dict it contains a non-empty "name"
key (and is a string), otherwise skip the entry or raise a clear ValueError/log
a warning and continue; update the realm_roles append logic to use
role.get("name") after validation and preserve role.get("description", "") for
description. Apply the identical defensive validation to the client_roles
parsing (the same pattern around the client role handling) so malformed dicts
won't raise KeyError and will produce an explicit, actionable error or log
entry.
- Around line 30-31: The file is opening the config file without specifying text
encoding which can cause cross-platform issues; update the open call that reads
config_path (the block returning yaml.safe_load(f)) to pass an explicit encoding
(e.g., encoding="utf-8") when opening for reading, so change the open(...) in
the function that reads config_path to open(config_path, "r", encoding="utf-8")
and then call yaml.safe_load on the file handle as before.
In `@authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py`:
- Around line 61-62: Open the YAML file using an explicit encoding to avoid
cross-platform defaults: update the file-open call that currently uses
open(yaml_path, "r") in llm_config.py (the block referencing yaml_path and
yaml.safe_load) to pass encoding="utf-8" (i.e., open(yaml_path, "r",
encoding="utf-8")). Ensure the rest of the logic (reading into config and
yaml.safe_load) remains unchanged.
In `@authbridge/demos/aiac-github-issue/aiac_agent/prompts/prompt_builder.py`:
- Around line 9-16: Update the type hints in build_system_prompt to use Python
3.12 built-in generics: replace typing.List and typing.Dict with list[...] and
dict[...] for the parameters realm_roles, client_roles_map, and
client_audience_targets; ensure any other signatures in this module using
typing.List/Dict (e.g., the other function noted in the file) are updated
similarly to match the repository's Python 3.12 typing style while keeping
parameter names and return type unchanged.
In `@authbridge/demos/aiac-github-issue/aiac_agent/utils/parsers.py`:
- Around line 11-15: The type annotations in extract_explanation_and_json and
print_explanation should use Python 3.12 built-in generics and include an
explicit return annotation: change Tuple[str, List] to tuple[str, list] in
extract_explanation_and_json, and add -> None to the signature of
print_explanation; update any matching import usage (remove typing.Tuple/List if
now unused) and ensure the function signatures (extract_explanation_and_json and
print_explanation) follow the new syntax consistently across the file.
In `@authbridge/demos/aiac-github-issue/aiac_agent/utils/validators.py`:
- Around line 10-23: Update type hints to use Python 3.12 built-in generics
instead of typing.Dict/List/Tuple: change the signature of
validate_policy_structure to use dict[str, Any], list[dict[str, str]],
list[str], and dict[str, list[dict[str, str]]], and replace other occurrences of
typing.Dict/typing.List/typing.Tuple in this module (including the functions
around the region reported) with the corresponding built-in generics; keep
imports only for names you still need (e.g., Any) and remove unused typing
imports.
In `@authbridge/demos/aiac-github-issue/keycloak_ops/apply_policy.py`:
- Line 73: Update the type hint for the parameter scope_ids in apply_policy.py
from Optional[Dict[str, str]] to the Python 3.12 union form dict[str, str] |
None; also remove or adjust any now-unused typing imports (Optional, Dict) and
ensure any other occurrences in the function signature or annotations use the
modern union syntax (reference: the scope_ids parameter in the relevant function
definition).
🪄 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 Plus
Run ID: 225c549e-d705-4820-baa4-8f6096aa5754
📒 Files selected for processing (29)
authbridge/demos/aiac-github-issue/aiac.envauthbridge/demos/aiac-github-issue/aiac_agent/__init__.pyauthbridge/demos/aiac-github-issue/aiac_agent/agent/__init__.pyauthbridge/demos/aiac-github-issue/aiac_agent/agent/graph.pyauthbridge/demos/aiac-github-issue/aiac_agent/agent/state.pyauthbridge/demos/aiac-github-issue/aiac_agent/config/__init__.pyauthbridge/demos/aiac-github-issue/aiac_agent/config/config_utils.pyauthbridge/demos/aiac-github-issue/aiac_agent/config/constants.pyauthbridge/demos/aiac-github-issue/aiac_agent/config/llm_conf.yaml.TEMPLATEauthbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.pyauthbridge/demos/aiac-github-issue/aiac_agent/prompts/__init__.pyauthbridge/demos/aiac-github-issue/aiac_agent/prompts/prompt_builder.pyauthbridge/demos/aiac-github-issue/aiac_agent/utils/__init__.pyauthbridge/demos/aiac-github-issue/aiac_agent/utils/parsers.pyauthbridge/demos/aiac-github-issue/aiac_agent/utils/validators.pyauthbridge/demos/aiac-github-issue/aiac_cli.pyauthbridge/demos/aiac-github-issue/config.yamlauthbridge/demos/aiac-github-issue/demo.mdauthbridge/demos/aiac-github-issue/k8s/configmaps.yamlauthbridge/demos/aiac-github-issue/k8s/git-issue-agent-deployment.yamlauthbridge/demos/aiac-github-issue/k8s/github-tool-deployment.yamlauthbridge/demos/aiac-github-issue/keycloak_ops/__init__.pyauthbridge/demos/aiac-github-issue/keycloak_ops/apply_policy.pyauthbridge/demos/aiac-github-issue/keycloak_ops/delete_policy.pyauthbridge/demos/aiac-github-issue/keycloak_ops/export_config.pyauthbridge/demos/aiac-github-issue/policies/permissive_policy.txtauthbridge/demos/aiac-github-issue/policies/regular_policy.txtauthbridge/demos/aiac-github-issue/requirements.txtauthbridge/demos/aiac-github-issue/setup_keycloak.py
| if structural_errors and retry_count < max_retries: | ||
| return {**state, "errors": structural_errors, "validation_passed": False, "retry_count": retry_count + 1} |
There was a problem hiding this comment.
Retry budget is off by one and can skip the final allowed retry.
Line 267 increments retry_count before routing, but Line 313 checks < max_retries. With max_retries=1, the first failure sets retry_count=1 and routing stops immediately (no retry).
Suggested fix
- if not validation_passed and retry_count < max_retries:
+ if not validation_passed and retry_count <= max_retries:
print(f"\n⚠️ Validation failed (attempt {retry_count}/{max_retries}). Retrying from parse_and_extract...")Also applies to: 313-314
🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/agent/graph.py` around lines
266 - 267, The retry logic is off-by-one: don't increment retry_count before
testing the budget; update the conditional to check whether retry_count + 1 <=
max_retries before returning a state with retry_count + 1 (i.e., replace the
current "if structural_errors and retry_count < max_retries" with a check that
ensures the incremented count is still within budget), and make the same change
for the analogous check around retry_count usage at the later block (the code
around retry_count and max_retries at the other occurrence).
| endpoint: <YOUR END POINT HERE> | ||
| api_key: <YOUR API KEY HERE> | ||
| temperature: 0.0 |
There was a problem hiding this comment.
Use ${ENV_VAR} placeholders instead of literal <...> tokens.
These fields are currently template text, not env-expandable values. Please switch to ${ENV_VAR} format so runtime/env substitution works consistently with other AuthBridge YAML configs.
As per coding guidelines, authbridge/**/*.{yaml,yml} files must support ${ENV_VAR} expansion.
Also applies to: 21-23, 32-34, 43-45, 54-56
🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/config/llm_conf.yaml.TEMPLATE`
around lines 10 - 12, Replace literal template tokens with env-expandable
placeholders: change the endpoint, api_key and temperature values in this LLM
config template from the current "<...>" text to ${ENV_VAR} style placeholders
(e.g. use ${LLM_ENDPOINT}, ${LLM_API_KEY}, ${LLM_TEMPERATURE} or other
project-standard names) so runtime/env substitution works; apply the same
replacement for the repeated blocks referenced (the occurrences for
endpoint/api_key/temperature at the other ranges) and ensure the YAML values
remain unquoted or appropriately quoted per YAML rules to allow env expansion.
Source: Coding guidelines
| except Exception as e: | ||
| error_msg = str(e) | ||
| # Handle "Already borrowed" errors gracefully | ||
| if "Already borrowed" in error_msg or "BadRequestError" in error_msg: | ||
| return (True, True, "Verification skipped due to API rate limiting") | ||
| return (False, False, f"Could not verify policy: {error_msg}") |
There was a problem hiding this comment.
Do not mark semantic verification failures as successful validation.
Line 287 currently returns (True, True, ...) on certain API errors, which can let invalid policies pass final validation. In agent/graph.py this short-circuits retry/failure behavior because the validator booleans are treated as authoritative pass/fail signals.
Suggested fix
- if "Already borrowed" in error_msg or "BadRequestError" in error_msg:
- return (True, True, "Verification skipped due to API rate limiting")
+ if "Already borrowed" in error_msg or "BadRequestError" in error_msg:
+ return (False, False, "Verification skipped due to transient API error; retry required")🧰 Tools
🪛 Ruff (0.15.15)
[warning] 283-283: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/utils/validators.py` around
lines 283 - 288, The except block in validators.py currently returns (True,
True, "Verification skipped due to API rate limiting") for API errors which
incorrectly signals a successful semantic verification; change that return so
the validator does NOT mark the policy as valid — e.g., return (False, False,
"Verification skipped due to API rate limiting") — keeping the error message but
ensuring the boolean pass/fail flags reflect a non-successful verification
(update the tuple returned in the except Exception as e handler where "Already
borrowed" or "BadRequestError" is checked).
| KEYCLOAK_ADMIN_USERNAME=admin | ||
| KEYCLOAK_ADMIN_PASSWORD=admin |
There was a problem hiding this comment.
Avoid committed default admin credentials.
Lines 3-4 commit live-looking credentials (admin/admin). Even for demos, this is high-risk when manifests/scripts are reused in shared environments. Use placeholders and require runtime-provided secrets.
🤖 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 `@authbridge/demos/aiac-github-issue/aiac.env` around lines 3 - 4, The
committed file contains hardcoded admin credentials (KEYCLOAK_ADMIN_USERNAME and
KEYCLOAK_ADMIN_PASSWORD); replace these literal values with non-sensitive
placeholders (e.g., empty values or template tokens) and require callers to
supply real secrets at runtime or via a secrets manager, add a .env.template (or
document in README) showing the placeholder keys, and ensure the actual aiac.env
is excluded from VCS (gitignored) so real credentials are never committed.
| # Optional 'secret' parameter: if not provided, Keycloak will auto-generate the client secret | ||
| clients: | ||
| - client_id: "kagenti" | ||
| secret: "demo-ui-secret" |
There was a problem hiding this comment.
Replace hardcoded client secret with env-expanded value.
Line 6 stores a plaintext secret in repo config. Switch to ${...} placeholder (and resolve it during startup parsing) to avoid committing secrets.
As per coding guidelines, authbridge/**/*.{yaml,yml} files must support ${ENV_VAR} expansion.
🤖 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 `@authbridge/demos/aiac-github-issue/config.yaml` at line 6, Replace the
hardcoded value for the YAML key "secret" (currently "demo-ui-secret") with an
env-expanded placeholder like ${AUTHBRIDGE_UI_SECRET} in the config.yaml, and
update the authbridge config-loading/startup parsing logic that reads these YAML
files to perform environment variable expansion so the placeholder is resolved
at runtime; ensure the loader treats ${VAR} syntax (for keys such as "secret")
and falls back or errors if the env var is missing per existing validation
rules.
Source: Coding guidelines
| def load_main_config(config_file: Path) -> Dict[str, Any]: | ||
| """Load main configuration from YAML file.""" | ||
| if not config_file.exists(): | ||
| raise FileNotFoundError(f"Configuration file not found: {config_file}") | ||
|
|
||
| with open(config_file, "r") as f: | ||
| return yaml.safe_load(f) |
There was a problem hiding this comment.
Add ${ENV_VAR} expansion and startup mode validation when loading YAML.
load_main_config currently loads raw YAML only. This misses required env expansion support and startup validation for config mode matching.
As per coding guidelines, authbridge/**/*.{yaml,yml} configs must support ${ENV_VAR} expansion and include startup validation for mode matching.
🤖 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 `@authbridge/demos/aiac-github-issue/setup_keycloak.py` around lines 49 - 55,
load_main_config currently opens and parses YAML raw; change it to read the file
as text, expand ${ENV_VAR} (use os.path.expandvars or equivalent) before calling
yaml.safe_load, then validate a required "mode" field in the resulting dict
against the process startup mode (e.g. os.getenv("STARTUP_MODE") or provided
expected mode) and raise a descriptive ValueError if missing or mismatched; keep
the existing FileNotFoundError behavior and ensure these checks live inside
load_main_config so configs under authbridge/**/*.{yaml,yml} support env
expansion and startup mode validation.
Source: Coding guidelines
| def delete_realm_roles(admin: KeycloakAdmin, realm_roles_config: List[str]) -> None: | ||
| print("\nDeleting realm roles:") | ||
| if not realm_roles_config: | ||
| print(" No realm roles in configuration") | ||
| return | ||
| for role_name in realm_roles_config: | ||
| try: | ||
| admin.delete_realm_role(role_name) | ||
| print(f" ✓ Deleted realm role: {role_name}") | ||
| except Exception as e: | ||
| print(f" - Realm role not found or error: {role_name} ({e})") |
There was a problem hiding this comment.
Realm-role cleanup breaks with dict-shaped role config.
delete_realm_roles assumes List[str], but this demo config uses objects ({name, description}). That causes failed deletions and undermines reset idempotency.
Suggested fix
-def delete_realm_roles(admin: KeycloakAdmin, realm_roles_config: List[str]) -> None:
+def delete_realm_roles(admin: KeycloakAdmin, realm_roles_config: List[dict[str, Any] | str]) -> None:
@@
- for role_name in realm_roles_config:
+ for role in realm_roles_config:
+ role_name = role["name"] if isinstance(role, dict) else role
try:
admin.delete_realm_role(role_name)🧰 Tools
🪛 Ruff (0.15.15)
[warning] 257-257: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@authbridge/demos/aiac-github-issue/setup_keycloak.py` around lines 248 - 258,
delete_realm_roles currently assumes realm_roles_config contains strings, but
the demo uses dicts like {"name", "description"} so deletions fail; update
delete_realm_roles to accept either strings or dicts and extract the role name
before calling KeycloakAdmin.delete_realm_role (e.g., check if each item is a
dict and use item.get("name") or fallback to the item itself), handle missing
names by skipping/logging, and keep the existing print/error messages
referencing the resolved role_name.
| def create_client_scopes( | ||
| admin: KeycloakAdmin, | ||
| client_ids: Dict[str, Dict[str, Any]], | ||
| ) -> Dict[str, str]: | ||
| """Create one scope per role per client. Returns {scope_name: scope_id}.""" | ||
| print("\n=== Creating client scopes ===") | ||
| scope_ids: Dict[str, str] = {} | ||
| for client_name, client_info in client_ids.items(): | ||
| for role in client_info["roles"]: | ||
| # Extract role name - support both dict and string formats | ||
| scope_name = role["name"] if isinstance(role, dict) else role | ||
| scope_id = create_single_client_scope( | ||
| admin, | ||
| scope_name, | ||
| client_name, | ||
| DEFAULT_SCOPE_ATTRIBUTES, | ||
| DEFAULT_MAPPER_CONFIG, | ||
| ) | ||
| scope_ids[scope_name] = scope_id | ||
| return scope_ids |
There was a problem hiding this comment.
Scope ID map keyed only by role name can collide across clients.
scope_ids[scope_name] = scope_id overwrites entries when different clients share a role name (common with access). Downstream assignments (Lines 553/621) can bind wrong scopes and break audience isolation.
🤖 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 `@authbridge/demos/aiac-github-issue/setup_keycloak.py` around lines 446 - 465,
The scope_ids map in create_client_scopes is keyed only by scope_name so
identical role names across clients collide; change the key to a unique
composite (e.g., "{client_name}:{scope_name}" or similar) when assigning
scope_ids and return that map, and then update any lookup sites that currently
expect plain scope_name (calls that use create_client_scopes results and later
bind scopes/audience) to construct the same composite key when retrieving the
scope id; locate create_client_scopes and create_single_client_scope plus
downstream bind/assignment code that consumes scope_ids and ensure both creation
and lookup use the same composite-key convention so entries no longer overwrite
each other.
| except Exception: | ||
| pass # Already added |
There was a problem hiding this comment.
Avoid silent except: pass in scope assignment paths.
These blocks suppress real provisioning failures (timeouts/auth/permission) and still report success. At minimum, log the exception; ideally re-raise non-idempotent cases.
Also applies to: 627-628
🧰 Tools
🪛 Ruff (0.15.15)
[error] 559-560: try-except-pass detected, consider logging the exception
(S110)
[warning] 559-559: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@authbridge/demos/aiac-github-issue/setup_keycloak.py` around lines 559 - 560,
Replace the silent "except Exception: pass" in the scope-assignment try/except
blocks with proper error handling: catch Exception as e and call the module
logger (or processLogger) with logger.exception or logger.error including
contextual text (e.g., "failed assigning scope XYZ") so the stack/exception is
recorded, and re-raise or return a failure result for non-idempotent
provisioning errors; for idempotent/expected conflicts you may suppress after
logging. Update both occurrences that currently use "except Exception: pass".
Source: Linters/SAST tools
Signed-off-by: Omer Boehm <omerboehm@gmail.com>
omerboehm
force-pushed
the
feat/aiac-github-issue-agent-poc
branch
from
fe62c8f to
bfb4fb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
authbridge/demos/aiac-github-issue/aiac_agent/utils/validators.py (1)
286-288:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not mark skipped semantic verification as a successful validation.
When transient LLM/API errors occur, returning
(True, True, ...)lets the graph treat validation as passed, which can bypass retries and accept invalid policy mappings.Suggested fix
- if "Already borrowed" in error_msg or "BadRequestError" in error_msg: - return (True, True, "Verification skipped due to API rate limiting") + if "Already borrowed" in error_msg or "BadRequestError" in error_msg: + return (False, False, "Verification skipped due to API rate limiting")🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/utils/validators.py` around lines 286 - 288, The current branch that handles transient LLM/API errors returns (True, True, ...) which incorrectly signals a successful validation; update the conditional that checks for "Already borrowed" or "BadRequestError" in validators.py so it does not mark the check as passed—return (False, True, "Verification skipped due to API rate limiting") (i.e., success=False, skipped=True) instead, keeping the same descriptive message so the system knows the verification was skipped but not accepted as valid.
🧹 Nitpick comments (4)
authbridge/demos/aiac-github-issue/demo.md (1)
400-400: 💤 Low valueUse consistent capitalization for "GitHub tool".
For consistency with the rest of the documentation and the official product name, use "GitHub tool" with a capital H.
📝 Proposed fix
-Authbridge outbound check will exchange the token, then deny the request since the exchanged token will not include the github tool in the 'aud' claim. +Authbridge outbound check will exchange the token, then deny the request since the exchanged token will not include the GitHub tool in the 'aud' claim.🤖 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 `@authbridge/demos/aiac-github-issue/demo.md` at line 400, Change the lowercase instance of "github tool" in the sentence that currently reads "Authbridge outbound check will exchange the token, then deny the request since the exchanged token will not include the github tool in the 'aud' claim." to "GitHub tool" so it matches the rest of the docs and the official product naming; update that exact string wherever it appears in this demo text.authbridge/demos/aiac-github-issue/keycloak_ops/export_config.py (1)
211-228: ⚡ Quick winRemove redundant KeycloakAdmin creation.
The first
KeycloakAdmininstance (lines 212-218) is immediately discarded and replaced (lines 222-228). Remove the first creation.Suggested fix
print(f"\nConnecting to Keycloak at {KEYCLOAK_URL} ...") - admin = KeycloakAdmin( - server_url=KEYCLOAK_URL, - username=KEYCLOAK_ADMIN_USERNAME, - password=KEYCLOAK_ADMIN_PASSWORD, - realm_name="master", - user_realm_name="master", - ) - - # Switch to target realm - print(f"Switching to realm: {realm_name}") admin = KeycloakAdmin( server_url=KEYCLOAK_URL, username=KEYCLOAK_ADMIN_USERNAME, password=KEYCLOAK_ADMIN_PASSWORD, realm_name=realm_name, user_realm_name="master", ) + print(f"Connected to realm: {realm_name}")🤖 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 `@authbridge/demos/aiac-github-issue/keycloak_ops/export_config.py` around lines 211 - 228, The first KeycloakAdmin instance is created and immediately overwritten; remove the redundant creation block (the admin = KeycloakAdmin(...) initialized with realm_name="master") so only the intended admin for the target realm is created. Keep the connection and realm switching prints, and ensure the remaining KeycloakAdmin instantiation uses server_url/username/password and realm_name=realm_name (the existing admin variable and KeycloakAdmin symbol) so there is a single, correct client for subsequent operations.authbridge/demos/aiac-github-issue/keycloak_ops/apply_policy.py (1)
9-9: ⚡ Quick winUse modern Python 3.12+ union syntax for type hints.
Replace
Optional[Dict[str, str]]withDict[str, str] | Noneper coding guidelines.Suggested fix
-from typing import Dict, List, Optional +from typing import Dict, ListAnd at line 73:
- scope_ids: Optional[Dict[str, str]] = None, + scope_ids: Dict[str, str] | None = 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 `@authbridge/demos/aiac-github-issue/keycloak_ops/apply_policy.py` at line 9, Replace legacy typing Optional usage with Python 3.12+ union syntax: find occurrences of Optional[Dict[str, str]] in apply_policy.py (e.g., the type hint used around line 73) and change them to Dict[str, str] | None; ensure you remove Optional from the typing imports if no longer needed and keep the Dict and List imports as required.Source: Coding guidelines
authbridge/demos/aiac-github-issue/setup_keycloak.py (1)
350-355: ⚡ Quick winChain exception properly with
raise ... from.Line 353 raises a new exception but loses the original exception context. Use exception chaining.
Suggested fix
except KeycloakPostError: internal_id = admin.get_client_id(client_id) if internal_id is None: - raise ValueError(f"Client '{client_id}' not found and could not be created") + raise ValueError(f"Client '{client_id}' not found and could not be created") from None print(f" Using existing client: {client_id}") return internal_id🤖 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 `@authbridge/demos/aiac-github-issue/setup_keycloak.py` around lines 350 - 355, The except block catching KeycloakPostError in setup_keycloak.py loses the original exception context when raising ValueError; modify the except KeycloakPostError handler to capture the original exception (e.g., except KeycloakPostError as err) and re-raise the ValueError using exception chaining (raise ValueError(f"Client '{client_id}' not found and could not be created") from err) while keeping the existing check using admin.get_client_id and the print/return of internal_id.
🤖 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 `@authbridge/demos/aiac-github-issue/aiac_agent/utils/validators.py`:
- Line 10: Update the type annotations in validate_policy_structure and
verify_policy_semantics to use Python 3.12 built-in generics (e.g., dict[...,
...], list[...], tuple[...]) instead of typing.Dict/typing.List/typing.Tuple,
and remove the now-unused Dict/List/Tuple imports from the module; ensure
function signatures and any internal variable annotations referencing
Dict/List/Tuple are replaced with the corresponding built-in generic forms to
keep types modern and PEP-compliant.
In `@authbridge/demos/aiac-github-issue/aiac_cli.py`:
- Around line 94-115: The code prints errors when result["success"] is False but
then continues and prints parsed mappings and allows run_full_pipeline to
proceed; change the failure branch in aiac_cli.py so that after printing the
error messages (from result["errors"]) it stops further processing by raising an
exception (e.g., RuntimeError or SystemExit) or returning a failure indicator
that run_full_pipeline can detect, and ensure the parsed-scopes printing block
and builder.save_policy call only run when result["success"] is True; reference
the result dict, the error loop, and run_full_pipeline to locate where to stop
execution.
In `@authbridge/demos/aiac-github-issue/demo.md`:
- Line 396: Update the typo in the sentence "The user 'alice' is allowed to send
requests to the git issue agent, how ever the agent is not allowed to invoke the
githb tool." by replacing "githb tool" with "github tool" so it reads "...invoke
the github tool."; locate the exact string in demo.md to edit the word "githb"
to "github" (preserve surrounding punctuation and casing).
- Around line 108-123: The markdown demo has an unclosed bash code block in the
section starting with the ```bash fence in the "Step 1: Environment Setup" (the
block containing the python venv creation and pip install commands); close that
code fence by adding the missing triple backticks immediately after the last
command (`pip install -r requirements.txt`) and before the "### Step 2:" heading
so the markdown renders correctly.
- Line 835: The markdown expected-output code block is missing its closing
triple backticks; add a line containing only triple backticks (```) immediately
before the "### Step 17: Reset Realm (Optional)" heading to close the code block
and restore proper markdown rendering.
---
Duplicate comments:
In `@authbridge/demos/aiac-github-issue/aiac_agent/utils/validators.py`:
- Around line 286-288: The current branch that handles transient LLM/API errors
returns (True, True, ...) which incorrectly signals a successful validation;
update the conditional that checks for "Already borrowed" or "BadRequestError"
in validators.py so it does not mark the check as passed—return (False, True,
"Verification skipped due to API rate limiting") (i.e., success=False,
skipped=True) instead, keeping the same descriptive message so the system knows
the verification was skipped but not accepted as valid.
---
Nitpick comments:
In `@authbridge/demos/aiac-github-issue/demo.md`:
- Line 400: Change the lowercase instance of "github tool" in the sentence that
currently reads "Authbridge outbound check will exchange the token, then deny
the request since the exchanged token will not include the github tool in the
'aud' claim." to "GitHub tool" so it matches the rest of the docs and the
official product naming; update that exact string wherever it appears in this
demo text.
In `@authbridge/demos/aiac-github-issue/keycloak_ops/apply_policy.py`:
- Line 9: Replace legacy typing Optional usage with Python 3.12+ union syntax:
find occurrences of Optional[Dict[str, str]] in apply_policy.py (e.g., the type
hint used around line 73) and change them to Dict[str, str] | None; ensure you
remove Optional from the typing imports if no longer needed and keep the Dict
and List imports as required.
In `@authbridge/demos/aiac-github-issue/keycloak_ops/export_config.py`:
- Around line 211-228: The first KeycloakAdmin instance is created and
immediately overwritten; remove the redundant creation block (the admin =
KeycloakAdmin(...) initialized with realm_name="master") so only the intended
admin for the target realm is created. Keep the connection and realm switching
prints, and ensure the remaining KeycloakAdmin instantiation uses
server_url/username/password and realm_name=realm_name (the existing admin
variable and KeycloakAdmin symbol) so there is a single, correct client for
subsequent operations.
In `@authbridge/demos/aiac-github-issue/setup_keycloak.py`:
- Around line 350-355: The except block catching KeycloakPostError in
setup_keycloak.py loses the original exception context when raising ValueError;
modify the except KeycloakPostError handler to capture the original exception
(e.g., except KeycloakPostError as err) and re-raise the ValueError using
exception chaining (raise ValueError(f"Client '{client_id}' not found and could
not be created") from err) while keeping the existing check using
admin.get_client_id and the print/return of internal_id.
🪄 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 Plus
Run ID: 50631262-b2b6-4d98-9622-811ba122c3bc
📒 Files selected for processing (29)
authbridge/demos/aiac-github-issue/aiac.envauthbridge/demos/aiac-github-issue/aiac_agent/__init__.pyauthbridge/demos/aiac-github-issue/aiac_agent/agent/__init__.pyauthbridge/demos/aiac-github-issue/aiac_agent/agent/graph.pyauthbridge/demos/aiac-github-issue/aiac_agent/agent/state.pyauthbridge/demos/aiac-github-issue/aiac_agent/config/__init__.pyauthbridge/demos/aiac-github-issue/aiac_agent/config/config_utils.pyauthbridge/demos/aiac-github-issue/aiac_agent/config/constants.pyauthbridge/demos/aiac-github-issue/aiac_agent/config/llm_conf.yaml.TEMPLATEauthbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.pyauthbridge/demos/aiac-github-issue/aiac_agent/prompts/__init__.pyauthbridge/demos/aiac-github-issue/aiac_agent/prompts/prompt_builder.pyauthbridge/demos/aiac-github-issue/aiac_agent/utils/__init__.pyauthbridge/demos/aiac-github-issue/aiac_agent/utils/parsers.pyauthbridge/demos/aiac-github-issue/aiac_agent/utils/validators.pyauthbridge/demos/aiac-github-issue/aiac_cli.pyauthbridge/demos/aiac-github-issue/config.yamlauthbridge/demos/aiac-github-issue/demo.mdauthbridge/demos/aiac-github-issue/k8s/configmaps.yamlauthbridge/demos/aiac-github-issue/k8s/git-issue-agent-deployment.yamlauthbridge/demos/aiac-github-issue/k8s/github-tool-deployment.yamlauthbridge/demos/aiac-github-issue/keycloak_ops/__init__.pyauthbridge/demos/aiac-github-issue/keycloak_ops/apply_policy.pyauthbridge/demos/aiac-github-issue/keycloak_ops/delete_policy.pyauthbridge/demos/aiac-github-issue/keycloak_ops/export_config.pyauthbridge/demos/aiac-github-issue/policies/permissive_policy.txtauthbridge/demos/aiac-github-issue/policies/regular_policy.txtauthbridge/demos/aiac-github-issue/requirements.txtauthbridge/demos/aiac-github-issue/setup_keycloak.py
✅ Files skipped from review due to trivial changes (4)
- authbridge/demos/aiac-github-issue/aiac_agent/utils/init.py
- authbridge/demos/aiac-github-issue/aiac_agent/init.py
- authbridge/demos/aiac-github-issue/policies/regular_policy.txt
- authbridge/demos/aiac-github-issue/aiac.env
🚧 Files skipped from review as they are similar to previous changes (11)
- authbridge/demos/aiac-github-issue/aiac_agent/config/constants.py
- authbridge/demos/aiac-github-issue/policies/permissive_policy.txt
- authbridge/demos/aiac-github-issue/aiac_agent/config/llm_conf.yaml.TEMPLATE
- authbridge/demos/aiac-github-issue/aiac_agent/prompts/prompt_builder.py
- authbridge/demos/aiac-github-issue/k8s/configmaps.yaml
- authbridge/demos/aiac-github-issue/aiac_agent/config/config_utils.py
- authbridge/demos/aiac-github-issue/aiac_agent/agent/state.py
- authbridge/demos/aiac-github-issue/config.yaml
- authbridge/demos/aiac-github-issue/aiac_agent/utils/parsers.py
- authbridge/demos/aiac-github-issue/aiac_agent/config/llm_config.py
- authbridge/demos/aiac-github-issue/aiac_agent/agent/graph.py
| """ | ||
|
|
||
| import re | ||
| from typing import Any, Dict, List, Tuple |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="authbridge/demos/aiac-github-issue/aiac_agent/utils/validators.py"
# Basic sanity: ensure file exists and show key regions with line numbers
ls -l "$FILE"
echo "---- imports + around line 10 ----"
sed -n '1,40p' "$FILE" | cat -n
echo "---- around lines 80-120 (covers 99-105) ----"
sed -n '80,130p' "$FILE" | cat -n
echo "---- find typing usage in this file ----"
rg -n "from typing import|typing\.Dict|typing\.List|typing\.Tuple|\bDict\[|\bList\[|\bTuple\[" "$FILE" || true
echo "---- show function signatures that contain Dict/List/Tuple ----"
rg -n "def [a-zA-Z0-9_]+\([^)]*(Dict|List|Tuple)\b" "$FILE" || trueRepository: kagenti/kagenti-extensions
Length of output: 4788
Switch this module to Python 3.12 built-in generics (dict/list/tuple)
authbridge/demos/aiac-github-issue/aiac_agent/utils/validators.py still uses typing.Dict/List/Tuple in validate_policy_structure (lines 19-23) and verify_policy_semantics (lines 102-105); update signatures to dict[...]/list[...]/tuple[...] and remove the unused Dict/List/Tuple imports.
Suggested update
-from typing import Any, Dict, List, Tuple
+from typing import Any
@@
def validate_policy_structure(
- policy: Dict[str, Any],
- realm_roles: List[Dict[str, str]],
- client_names: List[str],
- client_roles_map: Dict[str, List[Dict[str, str]]],
-) -> List[str]:
+ policy: dict[str, Any],
+ realm_roles: list[dict[str, str]],
+ client_names: list[str],
+ client_roles_map: dict[str, list[dict[str, str]]],
+) -> list[str]:
@@
def verify_policy_semantics(
state: PolicyState,
llm: BaseChatModel,
- client_roles_map: Dict[str, List[Dict[str, str]]],
- client_audience_targets: Dict[str, List[str]],
+ client_roles_map: dict[str, list[dict[str, str]]],
+ client_audience_targets: dict[str, list[str]],
verbose: bool = True,
-) -> Tuple[bool, bool, str]:
+) -> tuple[bool, bool, 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 `@authbridge/demos/aiac-github-issue/aiac_agent/utils/validators.py` at line
10, Update the type annotations in validate_policy_structure and
verify_policy_semantics to use Python 3.12 built-in generics (e.g., dict[...,
...], list[...], tuple[...]) instead of typing.Dict/typing.List/typing.Tuple,
and remove the now-unused Dict/List/Tuple imports from the module; ensure
function signatures and any internal variable annotations referencing
Dict/List/Tuple are replaced with the corresponding built-in generic forms to
keep types modern and PEP-compliant.
Source: Coding guidelines
| if result["success"]: | ||
| print("✓ Access rules generated successfully!\n") | ||
| print("Generated YAML:") | ||
| print("-" * 80) | ||
| print(result["yaml_output"]) | ||
| print("-" * 80) | ||
| builder.save_policy(result["yaml_output"], output_file) | ||
| else: | ||
| print("✗ Policy generation failed with errors:") | ||
| for error in result["errors"]: | ||
| print(f" - {error}") | ||
|
|
||
| print("\n" + "=" * 80) | ||
| print("Parsed Role-to-Client-Role Mappings:") | ||
| print("=" * 80) | ||
| for role_mapping in result["parsed_scopes"]: | ||
| realm_role = role_mapping["role"] | ||
| client_roles = role_mapping.get("client_roles", []) | ||
| print(f" {realm_role}:") | ||
| for cr in client_roles: | ||
| print(f" - {cr['client']}: {cr['role']}") | ||
|
|
There was a problem hiding this comment.
Silent failure allows run_full_pipeline to proceed with invalid policy.
When result["success"] is False, the function prints errors but does not raise an exception or return a failure indicator. The caller run_full_pipeline (line 167-172) expects to catch exceptions to detect failure, so it will incorrectly print "Policy generated successfully" and proceed to delete old policies and apply the (failed) new one.
Proposed fix
if result["success"]:
print("✓ Access rules generated successfully!\n")
print("Generated YAML:")
print("-" * 80)
print(result["yaml_output"])
print("-" * 80)
builder.save_policy(result["yaml_output"], output_file)
else:
print("✗ Policy generation failed with errors:")
for error in result["errors"]:
print(f" - {error}")
+ raise RuntimeError(f"Policy generation failed: {result['errors']}")
print("\n" + "=" * 80)
print("Parsed Role-to-Client-Role Mappings:")🤖 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 `@authbridge/demos/aiac-github-issue/aiac_cli.py` around lines 94 - 115, The
code prints errors when result["success"] is False but then continues and prints
parsed mappings and allows run_full_pipeline to proceed; change the failure
branch in aiac_cli.py so that after printing the error messages (from
result["errors"]) it stops further processing by raising an exception (e.g.,
RuntimeError or SystemExit) or returning a failure indicator that
run_full_pipeline can detect, and ensure the parsed-scopes printing block and
builder.save_policy call only run when result["success"] is True; reference the
result dict, the error loop, and run_full_pipeline to locate where to stop
execution.
| ### Step 1: Environment Setup | ||
|
|
||
| Create and activate a Python virtual environment: | ||
|
|
||
| ```bash | ||
| cd kagenti-extensions/authbridge/demos/aiac-github-issue | ||
|
|
||
| # Create virtual environment | ||
| python -m venv venv | ||
| source venv/bin/activate # On Windows: venv\Scripts\activate | ||
|
|
||
| # Install dependencies | ||
| pip install --upgrade pip | ||
| pip install -r requirements.txt | ||
|
|
||
|
|
There was a problem hiding this comment.
Critical: Missing closing backticks breaks markdown rendering.
The bash code block starting at line 112 is never closed. Add closing triple backticks after line 121 (after pip install -r requirements.txt) and before the ### Step 2: heading on line 124.
📝 Proposed fix
# Install dependencies
pip install --upgrade pip
pip install -r requirements.txt
+```
### Step 2: Apply Demo ConfigMaps🤖 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 `@authbridge/demos/aiac-github-issue/demo.md` around lines 108 - 123, The
markdown demo has an unclosed bash code block in the section starting with the
```bash fence in the "Step 1: Environment Setup" (the block containing the
python venv creation and pip install commands); close that code fence by adding
the missing triple backticks immediately after the last command (`pip install -r
requirements.txt`) and before the "### Step 2:" heading so the markdown renders
correctly.
|
|
||
| ⚠️ NOTE ⚠️ - make sure alice|alice123 (bob/bob123) appear in keycloak users under 'kagenti' realm (reset password if needed and make sure temporary password is set) | ||
|
|
||
| The user 'alice' is allowed to send requests to the git issue agent, how ever the agent is not allowed to invoke the githb tool. |
There was a problem hiding this comment.
Fix typo: "githb tool" → "github tool".
📝 Proposed fix
-The user 'alice' is allowed to send requests to the git issue agent, how ever the agent is not allowed to invoke the githb tool.
+The user 'alice' is allowed to send requests to the git issue agent, however the agent is not allowed to invoke the github tool.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The user 'alice' is allowed to send requests to the git issue agent, how ever the agent is not allowed to invoke the githb tool. | |
| The user 'alice' is allowed to send requests to the git issue agent, however the agent is not allowed to invoke the github tool. |
🧰 Tools
🪛 LanguageTool
[grammar] ~396-~396: Ensure spelling is correct
Context: ... the agent is not allowed to invoke the githb tool. First, get a valid token from Key...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_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 `@authbridge/demos/aiac-github-issue/demo.md` at line 396, Update the typo in
the sentence "The user 'alice' is allowed to send requests to the git issue
agent, how ever the agent is not allowed to invoke the githb tool." by replacing
"githb tool" with "github tool" so it reads "...invoke the github tool."; locate
the exact string in demo.md to edit the word "githb" to "github" (preserve
surrounding punctuation and casing).
| 5. Sales is now just like Tech-support ("Other personnel"): | ||
| client: github-tool | ||
| role: github-tool-aud | ||
|
|
There was a problem hiding this comment.
Add missing closing backticks after the expected output.
The expected output code block starting at line 817 should be closed before the heading on line 838.
📝 Proposed fix
role: github-tool-aud
-
-
+```
### Step 17: Reset Realm (Optional)🤖 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 `@authbridge/demos/aiac-github-issue/demo.md` at line 835, The markdown
expected-output code block is missing its closing triple backticks; add a line
containing only triple backticks (```) immediately before the "### Step 17:
Reset Realm (Optional)" heading to close the code block and restore proper
markdown rendering.
…oak.py Signed-off-by: Omer Boehm <omerboehm@gmail.com>
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Review Summary
Well-structured POC — YAML loads are all safe_load, no shell injection, TLS verification never disabled, resource limits set on both deployments, and there's a real structural allowlist (validate_policy_structure with bounded retry). Nicely done, and good catch self-fixing the credential-logging in commit 2.
The crux of this kind of demo is the LLM → IdP trust path, and that's where the suggestions cluster: the structural allowlist is the right idea, but it's only enforced in the agent graph — not re-checked at apply time — and two spots fail open (the semantic verifier on exception, and the swallowed apply error). The raw NL description is also interpolated straight into both the generation and verification prompts, so treat it as untrusted input and keep the structural allowlist authoritative over the LLM's verdict. None of this blocks a demo, but these become must-fix if AIAC ever graduates past POC.
Smaller notes (not inlined): no tests for validate_policy_structure/load_access_control_policy (high-value given they gate IdP writes); no HTTP timeout on KeycloakAdmin; and a # Made with Bob trailer left in ~12 source files — strip those.
Areas reviewed: Python (agent, keycloak ops, CLI), k8s, env/config, docs · Commits: 2, signed-off ✅ · CI: all green
No code blockers for a POC demo — approving, with the LLM-to-IdP guardrails flagged for follow-up.
Assisted-By: Claude Code
| error_msg = str(e) | ||
| # Handle "Already borrowed" errors gracefully | ||
| if "Already borrowed" in error_msg or "BadRequestError" in error_msg: | ||
| return (True, True, "Verification skipped due to API rate limiting") |
There was a problem hiding this comment.
suggestion (security) — The semantic verifier fails open here: on Already borrowed/BadRequestError it returns (True, True, "Verification skipped…"), i.e. "policy is correct," silently skipping validation. A validator that gates privileged Keycloak role grants should fail closed — treat an unverifiable result as not-validated rather than assuming success.
| add_client_role_to_realm_role_composite(admin, realm, user_role, client_id, role_name) | ||
| print(f" ✓ Added client role '{client_name}.{role_name}' to realm role '{user_role}'") | ||
| except Exception as e: | ||
| print(f" ℹ Client role '{client_name}.{role_name}' already in composite or error: {e}") |
There was a problem hiding this comment.
suggestion (security) — The role allowlist is enforced only in the agent graph (validators.validate_policy_structure), not here at write time — load_access_control_policy validates shape only and trusts whatever YAML it's handed. And this except swallows a failed get_client_role as "…already in composite or error", so an invalid/over-broad mapping is logged as benign instead of aborting. Re-check roles against the allowlist / live client roles at apply time and fail (non-zero) on error, so a regenerated or hand-edited policy can't bypass the agent-side gate.
| realm_name=realm_name, | ||
| user_realm_name="master", | ||
| ) | ||
| delete_access_control_policy(admin, realm_name, script_dir / main_config) |
There was a problem hiding this comment.
suggestion — run_full_pipeline runs delete old policy → apply new policy against a live realm with no confirmation gate. For a demo whose thesis is "an LLM infers IdP policy," a "show the generated diff → confirm before apply" step (or a --yes flag defaulting off) between generation and the destructive delete/apply is the expected guardrail.
| # Keycloak connection settings | ||
| KEYCLOAK_URL=http://keycloak.localtest.me:8080 | ||
| KEYCLOAK_ADMIN_USERNAME=admin | ||
| KEYCLOAK_ADMIN_PASSWORD=admin |
There was a problem hiding this comment.
suggestion — Committing a populated aiac.env with KEYCLOAK_ADMIN_PASSWORD=admin breaks the repo's .TEMPLATE convention (cf. llm_conf.yaml.TEMPLATE using <YOUR API KEY HERE>). These are demo defaults rather than a real secret, but ship aiac.env.example/.TEMPLATE and .gitignore the real file so committed creds don't become a habit in this directory.
| serviceAccountName: git-issue-agent | ||
| containers: | ||
| - name: agent | ||
| image: ghcr.io/kagenti/agent-examples/git-issue-agent:latest |
There was a problem hiding this comment.
suggestion — :latest is non-reproducible (pin a digest/version), and this container has no securityContext (runAsNonRoot, allowPrivilegeEscalation: false, dropped capabilities, ideally readOnlyRootFilesystem). Resource requests/limits are present, which is good. Same applies to github-tool-deployment.yaml.
|
@omerboehm we can merge now or would you like to make some changes based on the reviews? Will wait until next week in case you want to make some changes. |
4 participants
Summary
Adds a proof-of-concept demo for AI-driven Access Control (AIAC) under
kagenti-extensions/authbridge/demos/aiac-github-issue.The demo showcases how an AI agent can automatically infer and configure Keycloak access policies from natural language descriptions — eliminating the need for manually written permission rules.
Key components:
aiac_agent/) — a LangGraph-based state machine that parses a plain-English policy description and generates structured YAML role mappings, with LLM-based semantic validation and automatic retry logickeycloak_ops/) — exports realm config, applies/deletes composite role mappings via the Keycloak admin APIaiac_cli.py) — runs the full pipeline: export → generate → delete old policy → apply new policysetup_keycloak.py) — idempotently provisions the demo realm with clients, roles (developer, tech-support, sales), and users (alice, bob, charlie)k8s/) — deployments and AuthBridge configmaps for cluster-based testingpolicies/) — two example natural language policy files demonstrating different access levelsThe AIAC agent integrates with the existing AuthBridge token exchange flow: once policies are applied, Keycloak composite roles ensure that exchanged tokens automatically carry only the scopes allowed by the user's realm role — enforcing access control at the token-issuing layer.
Related issue(s)
Fixes #931
Testing Instructions
aiac.envwith Keycloak connection detailspython setup_keycloak.py config.yamlto provision the demo realmpython aiac_cli.py policies/regular_policy.txtto generate and apply a policySee demo.md for a full walkthrough.
Summary by CodeRabbit
New Features
Documentation