Repository navigation
feat: install reliability contract owners - #52
Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_db46f02d-8e95-4a19-b030-a374f0416323) |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughThe PR reorganizes Codex Graph reliability guidance, adds authoritative decision and evidence references, strengthens structural bundle validation, adds acceptance tests for invalid bundles, and runs those tests in Unix and Windows regression suites. ChangesReliability specification and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@skills/codex-graph/references/authority-and-decisions.md`:
- Around line 48-50: Update the “Protected-domain mutation gate” ordering to
require acceptance-capable action-path proof before mutation, covering canonical
identity, complete pre-state, authoritative action evidence, and independent
post-state capability; either list this explicit check between authorization and
mutation or define transport completeness to include it.
In `@skills/codex-graph/references/evidence-and-acceptance.md`:
- Around line 120-135: Revise the set definitions in the evidence and acceptance
guidance to distinguish overlapping lifecycle sets—authorized, inspected,
intended, attempted, receipt-resolved, and post-verified—from pairwise-disjoint
terminal outcome sets—accepted, failed, unknown, skipped, unauthorized, and
duplicate. Apply identity and count agreement requirements to terminal sets
without requiring lifecycle sets to be disjoint, preserving the rule that
mismatches cannot be accepted.
- Line 137: Update the zero-mutation acceptance rule so its second valid path
requires transport-complete discovery of the entire candidate universe, then
evidenced exclusion of every discovered candidate. Do not accept incomplete or
uncertain discovery, point reads, sampling, or aggregate totals; preserve the
transport-complete empty-set proof as the other valid path.
In `@skills/codex-graph/scripts/validate_skill.py`:
- Around line 156-160: Update require_headings to parse Markdown headings only
outside fenced code blocks, so matching lines inside literal blocks do not
satisfy requirements. Track fenced-block state while scanning the document,
validate headings from non-fenced content, and add a regression case covering a
required heading that appears only inside a code fence.
- Around line 190-205: Update the link-target handling in the validation loop to
remove angle brackets and normalize the fragment before classification; then
skip only empty anchors and actual URI schemes, while treating root-relative
paths as local and validating them against the bundle root. Add regression cases
covering a root-relative missing link and an angle-bracketed HTTPS URI.
🪄 Autofix
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: d3a6cda5-51e2-4b32-a494-43094067b285
📒 Files selected for processing (13)
scripts/run_regressions.shscripts/run_windows_regressions.ps1skills/codex-graph/SKILL.mdskills/codex-graph/references/authority-and-decisions.mdskills/codex-graph/references/code-mode-script-patterns.mdskills/codex-graph/references/evidence-and-acceptance.mdskills/codex-graph/references/progressive-complexity.mdskills/codex-graph/references/self-testing.mdskills/codex-graph/references/task-lifecycle.mdskills/codex-graph/references/topology-library.mdskills/codex-graph/scripts/task_collection_harness.test.jsskills/codex-graph/scripts/test_validate_skill.pyskills/codex-graph/scripts/validate_skill.py
| Compare these explicit sets exactly: | ||
|
|
||
| - authorized; | ||
| - inspected; | ||
| - intended; | ||
| - attempted; | ||
| - receipt-resolved; | ||
| - post-verified; | ||
| - accepted; | ||
| - failed; | ||
| - unknown; | ||
| - skipped; | ||
| - unauthorized; and | ||
| - duplicate. | ||
|
|
||
| Outcome sets are explicit and disjoint. Derive displayed counts from the sets; count agreement never substitutes for identity agreement. A mismatch cannot be accepted. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Separate overlapping lifecycle sets from disjoint terminal sets.
authorized, inspected, intended, attempted, receipt-resolved, and post-verified are lifecycle sets. A target normally belongs to several of them. Only terminal outcome sets such as accepted, failed, unknown, skipped, unauthorized, and duplicate should be pairwise disjoint. State this split explicitly; otherwise enforcing Line 135 across the full list makes successful reconciliation impossible.
🤖 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 `@skills/codex-graph/references/evidence-and-acceptance.md` around lines 120 -
135, Revise the set definitions in the evidence and acceptance guidance to
distinguish overlapping lifecycle sets—authorized, inspected, intended,
attempted, receipt-resolved, and post-verified—from pairwise-disjoint terminal
outcome sets—accepted, failed, unknown, skipped, unauthorized, and duplicate.
Apply identity and count agreement requirements to terminal sets without
requiring lifecycle sets to be disjoint, preserving the rule that mismatches
cannot be accepted.
|
|
||
| Outcome sets are explicit and disjoint. Derive displayed counts from the sets; count agreement never substitutes for identity agreement. A mismatch cannot be accepted. | ||
|
|
||
| A zero-mutation outcome is accepted only after either a transport-complete empty-set proof or evidenced exclusion of every transport-completely inspected candidate. Point reads, uncertain discovery, incomplete lists, sampling, and aggregate totals do not prove a set-wide negative. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require a complete candidate universe for zero-mutation acceptance.
The second alternative excludes every “transport-completely inspected candidate.” That condition can pass with an incomplete discovery result while undiscovered candidates remain. Require transport-complete discovery of the candidate set before accepting zero mutation. Keep the transport-complete empty-set proof as the other valid path.
🤖 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 `@skills/codex-graph/references/evidence-and-acceptance.md` at line 137, Update
the zero-mutation acceptance rule so its second valid path requires
transport-complete discovery of the entire candidate universe, then evidenced
exclusion of every discovered candidate. Do not accept incomplete or uncertain
discovery, point reads, sampling, or aggregate totals; preserve the
transport-complete empty-set proof as the other valid path.
| for raw_target in link_pattern.findall(text): | ||
| target = raw_target.strip() | ||
| if ( | ||
| not target | ||
| or target.startswith(("#", "/", "http://", "https://", "mailto:")) | ||
| ): | ||
| continue | ||
| if target.startswith("<") and target.endswith(">"): | ||
| target = target[1:-1] | ||
| target = unquote(target.split("#", 1)[0]) | ||
| resolved = (path.parent / target).resolve() | ||
| try: | ||
| resolved.relative_to(root) | ||
| except ValueError: | ||
| fail(f"{path.relative_to(root)} links outside the bundle: {raw_target}") | ||
| if not resolved.exists(): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject root-relative links after destination normalization.
At Lines 192-200, a target that starts with / is skipped. Therefore, /references/missing.md passes although the bundle requires relative links and paths outside the bundle must fail. Also, <https://example.com> is classified before its angle brackets are removed, so it is treated as a local path. Normalize the destination first, then skip only anchors and actual URI schemes. Add regression cases for both forms.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 193-193: Do not make http calls without encryption
Context: "http://"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🤖 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 `@skills/codex-graph/scripts/validate_skill.py` around lines 190 - 205, Update
the link-target handling in the validation loop to remove angle brackets and
normalize the fragment before classification; then skip only empty anchors and
actual URI schemes, while treating root-relative paths as local and validating
them against the bundle root. Add regression cases covering a root-relative
missing link and an angle-bracketed HTTPS URI.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_3907b778-ca8a-4832-ac29-e439785c8f2e) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_eafa8f56-502b-4619-b59c-418b29c3f45d) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_1a75e7d5-bd1e-4622-bdde-c10d5ca22164) |
| | Node kind | Contract purpose | | ||
| |---|---| | ||
| | work | Produce one scoped handoff or artifact | | ||
| | join | Combine declared upstream results without adding authority | | ||
| | gate | Decide one declared branch from machine-readable evidence | | ||
| | repair | Apply the one bounded correction path | | ||
| | terminal | End one graph path with its scoped result | |
There was a problem hiding this comment.
🟡 Workflow guide lists node categories the test-case checker does not accept
The topology guide now defines its own five node categories (| Node kind | table at skills/codex-graph/references/topology-library.md:18-24) that do not match the eight categories the test-case checker accepts, so a workflow authored from this table is rejected whenever an explicit test case is selected.
Impact: Generated workflows that follow the topology guide can fail conformance checking with an "unknown kind" error.
Conflicting role-kind vocabularies between the topology module and the conformance harness
SKILL.md:50 routes "Graph patterns and semantic-role to graph-local-node mapping" to the topology library, and the new table there enumerates work, join, gate, repair, terminal.
The authoritative kind set used by the offline matcher is ROLE_KINDS in skills/codex-graph/testcases/harness/conformance.js:35-42 (contract, discovery, writer, validator, integration, gate, repair, terminal), documented in skills/codex-graph/testcases/README.md:54-63. conformance.js:168-170 fails a contract whose role kind is outside that set, so work and join are rejected, while discovery, writer, validator, integration, and contract are missing from the topology table.
The prose immediately above the table (topology-library.md:13-16) uses the harness vocabulary (discovery, integration, validation, repair, terminal), which makes the added table the inconsistent element.
Prompt for agents
The new "Role and node kinds" table in skills/codex-graph/references/topology-library.md introduces node kinds (work, join, gate, repair, terminal) that conflict with the authoritative semantic-role kinds owned by the test-case harness (contract, discovery, writer, validator, integration, gate, repair, terminal) defined in skills/codex-graph/testcases/harness/conformance.js (ROLE_KINDS) and documented in skills/codex-graph/testcases/README.md. Because SKILL.md's route map points at the topology library for semantic-role to graph-local-node mapping, a model authoring a contract from that table can emit kinds the matcher rejects with 'unknown kind'. Either align the table with the harness kind set, or make the table explicitly non-normative and state that the testcase README/harness owns the kind vocabulary.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
SKILL.mdthe navigational entry point and install exactly two reliability ownerspassedvocabulary without changing collector behaviorCloses #41
Validation
.\scripts\run_windows_regressions.ps1from Codex Desktop on Windows.Test notes
Documentation-only exception: #41 is a documentation and contract migration. The added regression controls cover only the existing skill-bundle acceptance seam. Merged PRs #50 and #51 supply root-workflow behavior and structural testcase conformance through
main; this PR preserves but does not redefine those contracts.origin/mainat8e8e3ae34f4855eeaeb6377a10e6cb2a8f410b89; retained owner navigation, explicitcase_idbinding, Acorn dependency setup, structural testcase deletions, and both WF-01/WF-03 runner entriesnpm ci && python3 skills/codex-graph/scripts/validate_skill.py && python3 -m unittest -v skills/codex-graph/scripts/test_validate_skill.py skills/codex-graph/scripts/test_root_workflow.py && node --test skills/codex-graph/testcases/harness/testcases.test.js— dependency install and bundle acceptance passed; 7 Python tests and 14 structural-conformance tests passed./scripts/run_regressions.sh— passed on the merged tree committed as8e8e3ae34f4855eeaeb6377a10e6cb2a8f410b89: bundle acceptance, 1 validator test, 6 root-workflow tests, graph self-check, 43 task-harness tests, 14 testcase-conformance tests, and 11 release testsNote
Medium Risk
Large documentation and contract migration for authority, mutation gates, and evidence chains affects how generated workflows are designed and validated, though runtime collector behavior is unchanged and regressions target structure only.
Overview
Installs two reliability contract owners —
authority-and-decisions.mdandevidence-and-acceptance.md— and turnsSKILL.mdinto a short entry point with precedence, a route map, and links instead of inlined normative rules.Shrinks and reroutes the skill bundle. Long orchestration, lifecycle, and evidence prose moves out of
SKILL.mdinto the owners and slimmer reference modules (code-mode-script-patterns,task-lifecycle,progressive-complexity,topology-library,self-testing). Consumers must link to a single owner per contract; authority preflight runs before generic L0–L4 complexity, and workflow outcomes stay distinct from task/process verdicts.Replaces
validate_skill.pyphrase checks with structural acceptance: required owner links and headings, JSON/YAML integrity, relative links, balanced fences, and Mermaid coherence.test_validate_skill.pyadds negative controls (missing owners, broken links, bad graphs, etc.), and bothrun_regressions.shandrun_windows_regressions.ps1run that suite after the validator. The task-collection harness test now asserts consumers link totask-lifecycle.mdas the read-bounds owner.Reviewed by Cursor Bugbot for commit 8e8e3ae. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Improvements
Bug Fixes
Tests