Add orchestration governance drift check - #18
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a governance check script, check_orchestration_governance.py, and integrates it into the unit test suite via package.json. The script validates the presence of required repository files and the structure of the orchestration configuration. Feedback from the review highlights two potential issues: the validation logic could be bypassed if the configuration file is an empty dictionary, and the script lacks a type check for the JSON root, which could lead to crashes if the configuration is not an object.
|
|
||
| config, config_errors = load_config(root) | ||
| failures.extend(config_errors) | ||
| if config: |
There was a problem hiding this comment.
The check if config: will evaluate to False if the configuration file is an empty dictionary ({}). This causes the script to skip all validation logic for labels and gates, effectively allowing an empty or minimal configuration to pass the governance check. Changing this to check for the absence of config_errors ensures that the content is validated even if the dictionary is currently empty.
| if config: | |
| if not config_errors: |
| return json.loads(path.read_text(encoding="utf-8-sig")), [] | ||
| except FileNotFoundError: | ||
| return {}, [f"missing_file:{REQUIRED_FILES['orchestration_config']}"] | ||
| except json.JSONDecodeError as error: | ||
| return {}, [f"invalid_json:{path}:{error}"] |
There was a problem hiding this comment.
The load_config function does not verify that the parsed JSON root is a dictionary. If orchestration.json contains a JSON list or a scalar value at the root, subsequent calls to .get() in list_value and dict_value will raise an AttributeError, causing the script to crash. It is safer to validate the type immediately after loading.
| return json.loads(path.read_text(encoding="utf-8-sig")), [] | |
| except FileNotFoundError: | |
| return {}, [f"missing_file:{REQUIRED_FILES['orchestration_config']}"] | |
| except json.JSONDecodeError as error: | |
| return {}, [f"invalid_json:{path}:{error}"] | |
| data = json.loads(path.read_text(encoding="utf-8-sig")) | |
| if not isinstance(data, dict): | |
| return {}, [f"invalid_json_root:{path}:expected_object"] | |
| return data, [] | |
| except FileNotFoundError: | |
| return {}, [f"missing_file:{REQUIRED_FILES['orchestration_config']}"] | |
| except json.JSONDecodeError as error: | |
| return {}, [f"invalid_json:{path}:{error}"] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 459f41b9e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for snippet in ("agent:ready", "Cel / Goal", "Kryteria akceptacji / Acceptance Criteria"): | ||
| if snippet not in issue_text: |
There was a problem hiding this comment.
Validate issue labels structurally, not by text snippet
The governance check only asserts that the string agent:ready appears anywhere in .github/ISSUE_TEMPLATE/agent_task.yml, so it will still pass if the actual labels: list drops agent:ready but the term remains in descriptive text. In that scenario new issues stop being pre-labeled correctly while this drift check reports success, undermining the purpose of the guard. Parse the YAML and verify agent:ready is present under the top-level labels field (or with a stricter pattern tied to that field) to avoid this false negative.
Useful? React with 👍 / 👎.
Summary
scripts/check_orchestration_governance.pyto detect GitHub Issue orchestration drift.agent_task.ymlis missing oragent:readyis removed.npm run test:unit.Linked Issue
Closes #5
Evidence
python scripts/check_orchestration_governance.pypassed.python C:\Users\user\.codex\skills\github-issue-orchestrator\scripts\codex_orchestrate.py doctor --repo-root .passed.npm run test:unitpassed afternpm ciin the clean worktree.area:governance,gate:quick.Agent Checklist
main.Risks / Rollback
npm cireported existing dependency audit findings: 2 moderate and 2 high. This PR does not change dependencies.