Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7e6a42210
ℹ️ 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".
| positive_examples=( | ||
| examples/rdf/irreversible_task.ttl | ||
| examples/rdf/low_confidence_escalation.ttl | ||
| examples/rdf/multi_agent_handoff.ttl | ||
| ) |
There was a problem hiding this comment.
The workflow now validates only a hardcoded subset of positive fixtures, so any new .ttl example added under examples/rdf will be skipped unless someone also edits this list. That regresses the previous wildcard coverage and can let broken example artifacts merge with a green CI run, which undermines the repository’s SHACL-based compliance checks.
Useful? React with 👍 / 👎.
| if python scripts/validate_rdf.py "${negative_examples[@]}" --shapes shacl/hacp-core.ttl; then | ||
| echo "Expected SHACL failure for negative fixtures, but validation passed." | ||
| exit 1 | ||
| else | ||
| echo "Negative fixtures failed core SHACL as expected." |
There was a problem hiding this comment.
This if treats any non-zero exit from validate_rdf.py as an expected negative-fixture result, but that script also exits non-zero on parse/path errors (not just SHACL non-conformance). As a result, a missing negative fixture file or other runtime failure will still produce a passing CI message ("failed as expected"), masking real breakage in the validation pipeline.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| echo "Expected SHACL failure for negative fixtures, but validation passed." | ||
| exit 1 | ||
| else | ||
| echo "Negative fixtures failed core SHACL as expected." |
There was a problem hiding this comment.
Negative test masks script errors as expected failures
Medium Severity
The negative fixture test treats any non-zero exit code from validate_rdf.py as an expected SHACL validation failure. If the script crashes due to a missing file (e.g., failed_validation.ttl gets deleted), an import error, or any other runtime exception, the else branch still runs and the CI reports success with "Negative fixtures failed core SHACL as expected." Since positive examples use different files, their success doesn't guard against this masking. The test needs to distinguish a SHACL validation rejection from a script crash.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: Preview (646625f576)diff --git a/.github/workflows/validate-shacl.yml b/.github/workflows/validate-shacl.yml
--- a/.github/workflows/validate-shacl.yml
+++ b/.github/workflows/validate-shacl.yml
@@ -44,9 +44,17 @@
python scripts/validate_rdf.py "${positive_examples[@]}" --shapes shacl/hacp-core.ttl
python scripts/validate_rdf.py "${positive_culture_examples[@]}" --shapes shacl/culture-profile.shacl.ttl
- if python scripts/validate_rdf.py "${negative_examples[@]}" --shapes shacl/hacp-core.ttl; then
+ set +e
+ python scripts/validate_rdf.py "${negative_examples[@]}" --shapes shacl/hacp-core.ttl
+ status=$?
+ set -e
+
+ if [ "$status" -eq 0 ]; then
echo "Expected SHACL failure for negative fixtures, but validation passed."
exit 1
+ elif [ "$status" -eq 1 ]; then
+ echo "Negative fixtures failed core SHACL as expected."
else
- echo "Negative fixtures failed core SHACL as expected."
+ echo "Negative fixtures validation errored (exit $status)."
+ exit "$status"
fi
diff --git a/scripts/validate_rdf.py b/scripts/validate_rdf.py
--- a/scripts/validate_rdf.py
+++ b/scripts/validate_rdf.py
@@ -12,14 +12,14 @@
data_graph.parse(data_file, format="turtle")
except Exception as e:
print(f"Error parsing data file {data_file}: {e}")
- return False
+ return None
shacl_graph = Graph()
try:
shacl_graph.parse(shape_file, format="turtle")
except Exception as e:
print(f"Error parsing shape file {shape_file}: {e}")
- return False
+ return None
conforms, results_graph, results_text = validate(
data_graph,
@@ -46,10 +46,16 @@
args = parser.parse_args()
failures = 0
+ errors = 0
for data_file in args.data_files:
- if not validate_file(data_file, args.shapes):
+ result = validate_file(data_file, args.shapes)
+ if result is None:
+ errors += 1
+ elif result is False:
failures += 1
+ if errors > 0:
+ sys.exit(2)
if failures > 0:
sys.exit(1)
else: |
There was a problem hiding this comment.
Pull request overview
This pull request introduces a Culture Profile layer to the HACP (Human-AI Collaboration Protocol) specification, adding schemas, SHACL validation rules, and examples for team-level decision-making governance. The PR also refines the existing SHACL validation logic for authority boundaries to use a more precise SPARQL-based constraint instead of the previous property-based approach.
Changes:
- Adds Culture Profile schema and validation layer with team decision-rights, dissent channels, and risk policy tiers
- Updates task manifest schema to include optional
team_idandculture_profile_reffields - Refactors SHACL authority boundary constraint to use SPARQL for conditional validation
- Enhances CI workflow to validate positive and negative test fixtures separately
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
shacl/hacp-core.ttl |
Refactors AuthorityBoundaryShape to use SPARQL constraint for conditional humanApproval requirement on irreversible executions |
shacl/culture-profile.shacl.ttl |
Adds new SHACL shapes for validating CultureProfile, RiskPolicy, RiskTier, and DissentChannel entities |
schemas/task-manifest.yaml |
Adds optional team_id and culture_profile_ref fields for culture context binding |
schemas/culture-profile.yaml |
Defines new YAML schema for Culture Profile with team governance, risk tiers, norms, incentives, rituals, and metrics |
examples/rdf/culture_profile.ttl |
Provides RDF example demonstrating valid Culture Profile with two risk tiers |
examples/culture_profile.yaml |
Provides YAML example showing complete Culture Profile structure |
README.md |
Updates quick start guide to reference culture profile schemas and fixtures |
HACP.md |
Adds section 3.2 defining Culture Policy Binding requirements for high-importance/irreversible tasks |
COMPLIANCE.md |
Documents culture-profile.shacl.ttl as part of SHACL validation suite |
CHANGELOG.md |
Records all changes including culture layer additions and SHACL updates |
.github/workflows/validate-shacl.yml |
Restructures validation to use explicit file arrays and separate positive/negative test suites |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ### 3.2 Culture Policy Binding | ||
|
|
||
| Teams MAY publish a Culture Profile artifact that defines decision-rights model, dissent channels, and risk policy tiers. | ||
| For tasks where `importance=High` OR `reversibility=Irreversible`, collaboration **MUST** apply the matching Culture Profile `risk_policy` tier rules, including `requires_divergence` when that flag is set. |
There was a problem hiding this comment.
The specification states that teams MAY publish a Culture Profile, but then requires that tasks with importance=High OR reversibility=Irreversible MUST apply the matching Culture Profile risk_policy tier rules. This creates ambiguity: what should happen when such a task is executed but no Culture Profile exists? Consider clarifying whether: (1) a Culture Profile is required for high-importance/irreversible tasks, (2) there should be a default fallback behavior, or (3) the MUST only applies when a Culture Profile is present.
| For tasks where `importance=High` OR `reversibility=Irreversible`, collaboration **MUST** apply the matching Culture Profile `risk_policy` tier rules, including `requires_divergence` when that flag is set. | |
| When a Culture Profile is present, tasks where `importance=High` OR `reversibility=Irreversible` **MUST** apply the matching Culture Profile `risk_policy` tier rules, including `requires_divergence` when that flag is set. If no Culture Profile exists, such tasks MUST still follow a documented, human-governed risk policy and escalation path consistent with this protocol. |
| @@ -36,6 +36,12 @@ properties: | |||
| owner: | |||
| type: string | |||
| description: Human ID responsible for the task.il or org id) | |||
There was a problem hiding this comment.
The description contains a typo: ".il or org id)" should likely be "(email or org id)" or similar. The text appears garbled.
| description: Human ID responsible for the task.il or org id) | |
| description: Human ID (email or org id) responsible for the task. |
1 participant
Note
Medium Risk
Changes validation rules and CI gating around compliance fixtures, which may cause previously passing RDF artifacts to fail or vice versa. Adds new schema/shape surface area that needs alignment with downstream producers/consumers.
Overview
Introduces a Culture Profile layer by adding
schemas/culture-profile.yaml,shacl/culture-profile.shacl.ttl, and matching YAML/Turtle examples, and updates the spec/docs to require culture-policy tier binding for high-importance or irreversible tasks.Extends
schemas/task-manifest.yamlwith optionalteam_idandculture_profile_ref, refines core SHACL authority-boundary enforcement to requirehumanApprovalonly whenreversibilityisIrreversiblevia a SPARQL constraint, and updates the GitHub Action to run separate positive suites (core + culture) while asserting the negative fixture fails validation.Written by Cursor Bugbot for commit a7e6a42. This will update automatically on new commits. Configure here.