feat: integration contract validation in pre-dev + delivery verification#336
feat: integration contract validation in pre-dev + delivery verification#336gandalf-at-lerian wants to merge 3 commits intomainfrom
Conversation
…ification Adds two complementary mechanisms to catch cross-product/plugin integration issues earlier in the development cycle: 1. **Integration Contracts section in task-breakdown (pre-dev)** - New mandatory section when tasks reference external products/plugins - Requires explicit declaration: endpoint, method, request/response schema, version - Gate 7 validation ensures contracts are complete before entering dev-cycle - Prevents developers from implementing against assumptions 2. **Contract Compliance check in delivery-verification (Gate 0.5)** - New automated check (Step 3.5H) validates implementation matches declared contracts - Verifies endpoint paths are referenced in code - Cross-references HTTP methods - Flags when dependent product specs are not locally available - FAIL when declared contracts are not reflected in implementation Together these ensure: the task cannot be created without formalizing the integration contract (pre-dev), and the implementation cannot advance without validating adherence to that contract (Gate 0.5).
WalkthroughAdds an "Integration Contracts" declaration to PM task templates and introduces automated Delivery Verification Check H that conditionally verifies declared integration endpoints against dependent product specs and implementation references, updates overall verdict logic to include Check H, and adds related reviewer and standards cross-reference docs. Changes
Sequence Diagram(s)mermaid Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dev-team/skills/dev-delivery-verification/SKILL.md`:
- Line 726: The grep invocations that use $files_changed (the lines computing
refs and the second grep at the other occurrence) assume files_changed is a
newline-separated list but the schema provides a comma-separated string/array;
convert the comma-separated $files_changed into newline-separated paths before
passing to grep (e.g., pipe echo "$files_changed" through tr ',' '\n' or iterate
over the converted list) so grep searches each file path correctly; update the
refs computation and the other grep usage to operate on the normalized list
instead of raw $files_changed.
- Line 771: The precedence rules for "Verdict integration" omit Check H from the
hard-fail group; update the sentence that currently reads "Any FAIL in checks D,
E, F, or G → overall verdict is FAIL" to include H (e.g., "Any FAIL in checks D,
E, F, G, or H → overall verdict is FAIL") and ensure any accompanying
explanation or list of hard-block checks (under the "Verdict integration"
section/heading) also lists H so Check H is treated with the same severity as
D/E/F/G.
- Around line 707-710: The spec path loop is using escaped variable syntax
("\\${product_lower}") which prevents shell expansion; update the path entries
in that loop (the "../\\${product_lower}/api/swagger.yaml",
"../\\${product_lower}/api/swagger.json",
"../\\${product_lower}/api/openapi.yaml",
"../\\${product_lower}/proto/\\${product_lower}.proto" entries) to use an
unescaped variable like "../${product_lower}/..." or "$product_lower" so the
product_lower value is expanded at runtime, then re-run the script to verify
spec lookup succeeds.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b1dff2a-f251-42b2-bb61-a0898c9e6a6e
📒 Files selected for processing (2)
dev-team/skills/dev-delivery-verification/SKILL.mdpm-team/skills/pre-dev-task-breakdown/SKILL.md
- Fix escaped variable syntax in spec path loop — \${product_lower} → ${product_lower}
so shell expands the variable at runtime during spec lookup
- Normalize comma-separated files_changed to space-separated list before
passing to grep (tr ',' '\n' | xargs) — fixes file path matching in
Steps H.3 and H.4
- Add Check H to the hard-fail group in verdict integration rules —
D, E, F, G, H all cause FAIL verdict (was missing H)
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dev-team/agents/performance-reviewer.md`:
- Around line 1-45: The PR includes an unrelated agent spec ("name:
ring:performance-reviewer", "type: reviewer", and its output_schema/input_schema
blocks) that doesn't match the stated objectives about SKILL.md changes; either
remove this file from the PR if it was added accidentally, or update the PR
description/objectives to explicitly list and justify adding the performance
reviewer agent (mentioning the "ring:performance-reviewer" spec and its purpose)
so reviewers understand why it's part of this change set; ensure the commit or
PR body clearly references the agent spec and why it belongs with the SKILL.md
work.
- Around line 1-525: The agent spec "ring:performance-reviewer" is missing the
required "Standards Loading" section with WebFetch instructions; add a new
"Standards Loading" section (above or below the header metadata) that declares
which standard files to fetch and the WebFetch steps to load them before
verification starts, referencing the agent name "ring:performance-reviewer" and
ensuring the added section conforms to the existing YAML/markdown structure and
input/output schema so the loader runs prior to rule checks.
- Around line 377-392: Replace the inline standards table under the "##
Standards Compliance (AUTO-TRIGGERED)" section with a single reference to
standards-coverage-table.md; remove the explicit language rows (the
Go/TypeScript/SRE table entries) and instead add a pointer sentence that directs
readers to standards-coverage-table.md for coverage mapping and cross-references
to the listed standards (e.g., mention that performance-relevant sections for
Go, TypeScript, and SRE are covered in that central table). Ensure the section
still notes that Performance Review has no dedicated standards file but now
references standards-coverage-table.md for the specific coverage instead of
embedding the table.
- Line 406: The "No infra configs available" rule must differentiate
missing-but-required configs from genuinely N/A cases: change the verdict logic
so that the rule only forces "CANNOT be PASS" when infra is required but
manifests are missing (e.g., containerized service missing deployment/manifest),
otherwise treat as N/A and allow PASS if Layer 1 is clean; update the check that
evaluates "Microservice with no DB" and "Serverless" (and the container/HPA
checks) to set an isInfraApplicable/isInfraRequired flag and use it when
computing Layer 2 verdicts so only required-missing config triggers non-PASS.
- Line 391: Fix the nested bold markers in the string "**If `**MODE: ANALYSIS
only**` is detected:**" by removing the inner bold markers around MODE and
keeping the inline code/backticks for the mode; e.g., change the fragment so it
reads either bolded whole phrase with a single inline code block like "**If
`MODE: ANALYSIS only` is detected:**" or unbold the surrounding text and keep
"`MODE: ANALYSIS only`" as inline code, ensuring no overlapping ** markers
remain; update the same occurrence of that exact token in the file.
In `@dev-team/skills/dev-delivery-verification/SKILL.md`:
- Around line 718-721: The current early "continue" when the shell variable
spec_found is false causes the script to skip the H.3/H.4 endpoint/method
compliance checks entirely for that contract; instead remove the "continue" and
set a spec_missing flag (e.g., spec_found=false or spec_missing=true) and
proceed to run the same H.3/H.4 validation code paths for the current id/product
so those checks still execute; within the validation functions or call sites
(the blocks that perform H.3/H.4) add conditional handling to treat missing spec
state as a warning/failure case (log "spec missing" with id/product) but do not
bypass endpoint/method reference checks that ensure implementation references
declared endpoints.
- Around line 692-754: The loop is running in a subshell because of "echo
\"$contracts\" | while IFS='|' read -r ...; do", so assignments to blocking are
lost; replace the pipeline with a direct here-string or redirected while loop
(e.g. while IFS='|' read -r _ id product endpoint method req_schema resp_schema
version _; do ... done <<< "$contracts") so changes to the blocking variable
persist. Also remove the broad "continue" used when spec_found=false and instead
only skip spec-specific checks (keep Steps H.3 and H.4 running regardless), i.e.
use the existing spec_found guard to only bypass Step H.5/field-level validation
while still performing endpoint refs and method checks; ensure references to
symbols blocking, contracts, spec_found, spec_path, endpoint_path, and
normalized_files remain consistent.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b77138b-5135-454f-b1a2-fdfc8cbb2db2
📒 Files selected for processing (2)
dev-team/agents/performance-reviewer.mddev-team/skills/dev-delivery-verification/SKILL.md
| --- | ||
| name: ring:performance-reviewer | ||
| description: "Performance Reviewer: Reviews code and infrastructure configurations for performance issues across Go, TypeScript, and Python. Covers code-level hotspots (allocations, goroutine leaks, N+1 queries, event loop blocking) and runtime/infra misconfigurations (GOMAXPROCS vs cgroup limits, GC tuning, CFS throttling, connection pool sizing). Usable as a PR reviewer or standalone audit." | ||
| type: reviewer | ||
| output_schema: | ||
| format: "markdown" | ||
| required_sections: | ||
| - name: "Performance Review Summary" | ||
| pattern: "^## Performance Review Summary" | ||
| required: true | ||
| - name: "Layer 1: Code-Level Findings" | ||
| pattern: "^## Layer 1: Code-Level Findings" | ||
| required: true | ||
| - name: "Layer 2: Runtime/Infra Findings" | ||
| pattern: "^## Layer 2: Runtime/Infra Findings" | ||
| required: true | ||
| - name: "Estimated Impact" | ||
| pattern: "^## Estimated Impact" | ||
| required: true | ||
| - name: "Recommended Actions" | ||
| pattern: "^## Recommended Actions" | ||
| required: true | ||
| - name: "Blockers" | ||
| pattern: "^## Blockers" | ||
| required: false | ||
| verdict_values: ["PASS", "FAIL", "NEEDS_DISCUSSION"] | ||
| input_schema: | ||
| required_context: | ||
| - name: "code_or_diff" | ||
| type: "string" | ||
| description: "Code diff (PR review mode) or codebase path (standalone audit mode)" | ||
| - name: "review_mode" | ||
| type: "enum" | ||
| description: "Either 'pr' (reviewing a diff) or 'audit' (full codebase performance review)" | ||
| optional_context: | ||
| - name: "language" | ||
| type: "string" | ||
| description: "Primary language: go, typescript, python. Auto-detected if not provided." | ||
| - name: "infrastructure_configs" | ||
| type: "file_content" | ||
| description: "Kubernetes manifests, Dockerfiles, docker-compose files, Helm values for Layer 2 analysis" | ||
| - name: "expected_load" | ||
| type: "string" | ||
| description: "Expected TPS/RPS, concurrent connections, or workload characteristics" | ||
| --- |
There was a problem hiding this comment.
File does not match PR objectives.
The PR objectives describe changes to pm-team/skills/pre-dev-task-breakdown/SKILL.md and dev-team/skills/dev-delivery-verification/SKILL.md for integration contract validation. However, this file (dev-team/agents/performance-reviewer.md) is a performance reviewer agent specification, which is completely unrelated to the stated PR objectives.
Please verify whether:
- This file was included in the PR by mistake, or
- The PR objectives are incorrect/incomplete
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev-team/agents/performance-reviewer.md` around lines 1 - 45, The PR includes
an unrelated agent spec ("name: ring:performance-reviewer", "type: reviewer",
and its output_schema/input_schema blocks) that doesn't match the stated
objectives about SKILL.md changes; either remove this file from the PR if it was
added accidentally, or update the PR description/objectives to explicitly list
and justify adding the performance reviewer agent (mentioning the
"ring:performance-reviewer" spec and its purpose) so reviewers understand why
it's part of this change set; ensure the commit or PR body clearly references
the agent spec and why it belongs with the SKILL.md work.
- performance-reviewer.md: add Standards Loading section, replace inline standards table with reference to standards-coverage-table.md, fix nested bold markers, differentiate infra missing-but-required vs N/A - dev-delivery-verification SKILL.md: fix subshell variable loss by replacing pipe+while with here-string, remove premature continue so H.3/H.4 checks run even without local spec, move normalized_files computation outside the while loop - standards-coverage-table.md: add ring:performance-reviewer entry with 10 cross-reference sections including Goroutine Leak Detection and Dependency Management Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dev-team/agents/performance-reviewer.md`:
- Around line 49-68: The Standards Loading block is missing the required
<fetch_required> WebFetch tags; add a <fetch_required> section that lists the
raw GitHub URLs for each standards file referenced in the table (e.g.,
dev-team/docs/standards/golang/architecture.md,
dev-team/docs/standards/golang/core.md,
dev-team/docs/standards/golang/bootstrap.md,
dev-team/docs/standards/typescript.md, dev-team/docs/standards/sre.md) so the
agent will fetch them before running checks, place the tag immediately after the
Standards Loading heading/table, and keep the existing fallback message
"Standards not loaded — findings based on built-in checks only" unchanged.
In `@dev-team/skills/dev-delivery-verification/SKILL.md`:
- Around line 687-689: The current check uses contracts=$(grep '^| IC-' ...) and
treats an empty result as “no Integration Contracts section” causing a SKIP;
change the logic so you first detect the presence of the Integration Contracts
section header (e.g. grep for the section title like 'Integration Contracts' or
the table header row) and only treat a true absence of the section as SKIP,
while if the section header exists but contracts (the grep '^| IC-' result) is
empty or malformed you emit a WARNING/FAIL (blocking validation) instead; update
the conditional that references the contracts variable and the grep '^| IC-'
invocation to perform these two checks and return the appropriate non-SKIP
outcome when rows are missing or malformed.
- Around line 726-737: The current grep invocations interpolate endpoint_path
directly into regexes causing metacharacter-driven false matches; update the
checks that set refs and method_refs to use fixed-string matching or escape
endpoint_path before passing to grep: for the refs check use grep -F (or escape
endpoint_path into a safe variable) when searching $normalized_files for
$endpoint_path, and for the method check change the combined search to
explicitly test for the method and the endpoint as fixed strings (e.g., use grep
-iF for $method_upper and grep -F for $endpoint_path in a piped/combined test)
so endpoint_path is treated as literal text when computing refs and method_refs.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a8eb991-6f1c-4fad-9eb0-690e7b452d92
📒 Files selected for processing (3)
dev-team/agents/performance-reviewer.mddev-team/skills/dev-delivery-verification/SKILL.mddev-team/skills/shared-patterns/standards-coverage-table.md
| ## Standards Loading (MANDATORY) | ||
|
|
||
| **MUST load performance-relevant standards before starting review:** | ||
|
|
||
| Performance Review does not have a dedicated standards file. Instead, cross-reference performance-relevant sections from existing standards. MUST load applicable standards via WebFetch based on detected language: | ||
|
|
||
| | Detected Language | Standards to Load | | ||
| |-------------------|-------------------| | ||
| | Go | `golang/architecture.md` (Performance Patterns, Concurrency Patterns, N+1 Query Detection, Goroutine Leak Detection), `golang/core.md` (Dependency Management), `golang/bootstrap.md` (Connection Management, Graceful Shutdown) | | ||
| | TypeScript | `typescript.md` (Testing, Frameworks & Libraries) | | ||
| | SRE/Infra (Layer 2) | `sre.md` (Health Checks, Observability) | | ||
|
|
||
| **Loading Steps:** | ||
| 1. Detect language(s) from project files (see Language Detection below) | ||
| 2. For each detected language, fetch the corresponding standards file(s) from `dev-team/docs/standards/` | ||
| 3. Use fetched standards as reference when evaluating findings | ||
| 4. If standards cannot be loaded, report in output: "Standards not loaded — findings based on built-in checks only" | ||
|
|
||
| **MUST NOT proceed with review without attempting to load standards.** | ||
|
|
There was a problem hiding this comment.
Standards Loading section lacks <fetch_required> WebFetch tags.
The Standards Loading section describes which standards to load but doesn't use the standard <fetch_required> tag format that other agents follow. While the table at lines 55-59 maps languages to standards files, there are no explicit <fetch_required> tags with URLs to trigger WebFetch.
Compare with the sre.md agent pattern (context snippet 2, sre.md:100-110):
<fetch_required>
https://raw.githubusercontent.com/LerianStudio/ring/main/dev-team/docs/standards/sre.md
https://raw.githubusercontent.com/LerianStudio/ring/main/dev-team/docs/standards/golang.md
</fetch_required>🔧 Proposed fix
Add <fetch_required> tags after line 53:
Performance Review does not have a dedicated standards file. Instead, cross-reference performance-relevant sections from existing standards. MUST load applicable standards via WebFetch based on detected language:
+<fetch_required>
+https://raw.githubusercontent.com/LerianStudio/ring/main/dev-team/docs/standards/golang/architecture.md
+https://raw.githubusercontent.com/LerianStudio/ring/main/dev-team/docs/standards/golang/core.md
+https://raw.githubusercontent.com/LerianStudio/ring/main/dev-team/docs/standards/golang/bootstrap.md
+https://raw.githubusercontent.com/LerianStudio/ring/main/dev-team/docs/standards/typescript.md
+https://raw.githubusercontent.com/LerianStudio/ring/main/dev-team/docs/standards/sre.md
+</fetch_required>
+
| Detected Language | Standards to Load |Based on learnings stating "Agent files MUST include 'Standards Loading' section with WebFetch instructions to load applicable standards before verification begins" and the sre.md agent pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev-team/agents/performance-reviewer.md` around lines 49 - 68, The Standards
Loading block is missing the required <fetch_required> WebFetch tags; add a
<fetch_required> section that lists the raw GitHub URLs for each standards file
referenced in the table (e.g., dev-team/docs/standards/golang/architecture.md,
dev-team/docs/standards/golang/core.md,
dev-team/docs/standards/golang/bootstrap.md,
dev-team/docs/standards/typescript.md, dev-team/docs/standards/sre.md) so the
agent will fetch them before running checks, place the tag immediately after the
Standards Loading heading/table, and keep the existing fallback message
"Standards not loaded — findings based on built-in checks only" unchanged.
| if [ -z "$contracts" ]; then | ||
| echo "CONTRACT_COMPLIANCE: ⚠️ SKIP — no Integration Contracts section in task spec" | ||
| else |
There was a problem hiding this comment.
Do not SKIP when the section exists but contract rows are missing/malformed.
contracts is derived from grep '^| IC-'; if the section exists but rows are empty or malformed, this currently emits SKIP and bypasses enforcement. That should be a blocking validation outcome (or at least WARNING), not “does not apply”.
♻️ Proposed fix
-contracts=$(sed -n '/^## Integration Contracts/,/^## /p' "$task_file" | grep '^| IC-')
+section=$(sed -n '/^## Integration Contracts/,/^## /p' "$task_file")
+contracts=$(echo "$section" | grep '^| IC-')
-if [ -z "$contracts" ]; then
- echo "CONTRACT_COMPLIANCE: ⚠️ SKIP — no Integration Contracts section in task spec"
+if [ -z "$section" ]; then
+ echo "CONTRACT_COMPLIANCE: ⚠️ SKIP — no Integration Contracts section in task spec"
+elif [ -z "$contracts" ]; then
+ echo "CONTRACT_COMPLIANCE: ⛔ FAIL — Integration Contracts section exists but has no valid IC-* rows"
else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev-team/skills/dev-delivery-verification/SKILL.md` around lines 687 - 689,
The current check uses contracts=$(grep '^| IC-' ...) and treats an empty result
as “no Integration Contracts section” causing a SKIP; change the logic so you
first detect the presence of the Integration Contracts section header (e.g. grep
for the section title like 'Integration Contracts' or the table header row) and
only treat a true absence of the section as SKIP, while if the section header
exists but contracts (the grep '^| IC-' result) is empty or malformed you emit a
WARNING/FAIL (blocking validation) instead; update the conditional that
references the contracts variable and the grep '^| IC-' invocation to perform
these two checks and return the appropriate non-SKIP outcome when rows are
missing or malformed.
| endpoint_path=$(echo "$endpoint" | sed 's|^[A-Z]* ||') # Strip HTTP method prefix | ||
| refs=$(grep -rn "$endpoint_path" $normalized_files 2>/dev/null | grep -v "_test" | wc -l) | ||
| if [ "$refs" -eq 0 ]; then | ||
| echo " ⛔ BLOCKING: $id — endpoint $endpoint_path not referenced in implementation" | ||
| blocking=1 | ||
| fi | ||
|
|
||
| # Step H.4: Verify HTTP method matches | ||
| if echo "$method" | grep -qiE "^(GET|POST|PUT|PATCH|DELETE)$"; then | ||
| method_upper=$(echo "$method" | tr '[:lower:]' '[:upper:]') | ||
| method_refs=$(grep -rn "$method_upper.*$endpoint_path\|$endpoint_path.*$method_upper" $normalized_files 2>/dev/null | grep -v "_test" | wc -l) | ||
| if [ "$method_refs" -eq 0 ]; then |
There was a problem hiding this comment.
Use fixed-string-safe matching for endpoint checks.
endpoint_path is interpolated directly into grep regex patterns. Endpoints may contain regex metacharacters, causing false matches and incorrect PASS/WARNING outcomes in H.3/H.4.
♻️ Proposed fix
endpoint_path=$(echo "$endpoint" | sed 's|^[A-Z]* ||') # Strip HTTP method prefix
-refs=$(grep -rn "$endpoint_path" $normalized_files 2>/dev/null | grep -v "_test" | wc -l)
+refs=$(grep -rnF "$endpoint_path" $normalized_files 2>/dev/null | grep -v "_test" | wc -l)
@@
if echo "$method" | grep -qiE "^(GET|POST|PUT|PATCH|DELETE)$"; then
method_upper=$(echo "$method" | tr '[:lower:]' '[:upper:]')
- method_refs=$(grep -rn "$method_upper.*$endpoint_path\|$endpoint_path.*$method_upper" $normalized_files 2>/dev/null | grep -v "_test" | wc -l)
+ method_refs=$(
+ grep -rnE "$method_upper" $normalized_files 2>/dev/null \
+ | grep -F "$endpoint_path" \
+ | grep -v "_test" \
+ | wc -l
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev-team/skills/dev-delivery-verification/SKILL.md` around lines 726 - 737,
The current grep invocations interpolate endpoint_path directly into regexes
causing metacharacter-driven false matches; update the checks that set refs and
method_refs to use fixed-string matching or escape endpoint_path before passing
to grep: for the refs check use grep -F (or escape endpoint_path into a safe
variable) when searching $normalized_files for $endpoint_path, and for the
method check change the combined search to explicitly test for the method and
the endpoint as fixed strings (e.g., use grep -iF for $method_upper and grep -F
for $endpoint_path in a piped/combined test) so endpoint_path is treated as
literal text when computing refs and method_refs.
Summary
Adds two complementary mechanisms to catch cross-product/plugin integration issues earlier in the development cycle.
Problem
When a feature in Product A depends on Product B (or a plugin), the current dev-cycle gates validate A's code in isolation. If the integration assumptions are wrong — wrong endpoint, missing fields, changed interface — the issue is only caught during testing or production.
Real example: the TED integration issue discussed in #product-launch, where implementation was done against assumed contracts instead of verified ones.
Solution
1. Integration Contracts section in pre-dev task-breakdown
2. Contract Compliance check in delivery-verification (Gate 0.5, Step 3.5H)
How they work together
Files changed
pm-team/skills/pre-dev-task-breakdown/SKILL.md— Integration Contracts section + validation rulesdev-team/skills/dev-delivery-verification/SKILL.md— Step 3.5H contract compliance checkProposed by
@jota-at-lerian + @gandalf-at-lerian — discussion in #2-men-ideas-and-sync