Skip to content

feat: integration contract validation in pre-dev + delivery verification#336

Open
gandalf-at-lerian wants to merge 3 commits intomainfrom
feat/integration-contract-validation
Open

feat: integration contract validation in pre-dev + delivery verification#336
gandalf-at-lerian wants to merge 3 commits intomainfrom
feat/integration-contract-validation

Conversation

@gandalf-at-lerian
Copy link
Copy Markdown
Contributor

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

  • New section in the per-task template, required when tasks reference external products/plugins
  • Requires explicit declaration: endpoint, method, request/response schema, API version
  • Gate 7 validation ensures contracts are complete and sourced from actual specs before entering dev-cycle
  • Anti-rationalization entries prevent deferring contract definition to implementation time

2. Contract Compliance check in delivery-verification (Gate 0.5, Step 3.5H)

  • New automated check validates that implementation references the declared contract endpoints
  • Cross-references HTTP methods against declarations
  • Flags when dependent product specs are not locally available for auto-validation
  • FAIL verdict when declared contracts are not reflected in implementation code

How they work together

Pre-dev (task-breakdown) → Developer declares: "I will call POST /api/v1/pix/payments v1.2.0"
Gate 0 (implementation)  → Developer writes the integration code
Gate 0.5 (verification)  → System validates: "Does the code actually call POST /api/v1/pix/payments?"

Files changed

  • pm-team/skills/pre-dev-task-breakdown/SKILL.md — Integration Contracts section + validation rules
  • dev-team/skills/dev-delivery-verification/SKILL.md — Step 3.5H contract compliance check

Proposed by

@jota-at-lerian + @gandalf-at-lerian — discussion in #2-men-ideas-and-sync

…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).
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 11, 2026

Walkthrough

Adds 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

Cohort / File(s) Summary
PM task-template & guidance
pm-team/skills/pre-dev-task-breakdown/SKILL.md
Adds an "Integration Contracts" task-template column and a new section defining when contracts are required, the contract-row schema (ID, Product/Plugin, Endpoint/Interface, Method, Request/Response Schema, Version), declaration rules (exact interface, typed schemas, version pinning, source-of-truth), Gate 7 validation rules, and anti-rationalization/common violations guidance.
Delivery verification & verdict rules
dev-team/skills/dev-delivery-verification/SKILL.md
Introduces automated verification Check H "Integration Contract Compliance" that runs only if ## Integration Contracts exists; extracts contract rows, normalizes files_changed, attempts to locate dependent product spec in common local paths, fails if declared endpoint path is not referenced in changed implementation files (excluding _test), emits WARNING for ambiguous HTTP-method association or missing spec, defers field-level schema validation to manual review/OAS diff, and updates overall verdict from checks A–G to A–H with adjusted blocking precedence.
New reviewer spec
dev-team/agents/performance-reviewer.md
Adds ring:performance-reviewer agent spec with input/output schemas, required Layer 1 language-specific performance checks (Go/TypeScript/Python), optional Layer 2 infra/runtime checks when infra configs provided, standards-loading and multi-language behavior, severity/blocker rules, required Markdown output format, and example outputs.
Standards cross-reference
dev-team/skills/shared-patterns/standards-coverage-table.md
Adds standards coverage entries for ring:performance-reviewer mapping performance checks to existing golang/typescript/sre standards (PERF-GO-1..6, PERF-TS-1..2, PERF-SRE-1..2).

Sequence Diagram(s)

mermaid
sequenceDiagram
actor Author
participant TaskSpec as "Task Spec (Integration Contracts)"
participant Verifier as "Delivery Verifier (Check H)"
participant DepSpec as "Dependent Product Spec (local)"
participant Impl as "Implementation Files"
Author->>TaskSpec: Add Integration Contracts section
TaskSpec->>Verifier: Trigger Check H (if section present)
Verifier->>DepSpec: Locate referenced spec in common paths
alt Spec found
Verifier->>Impl: Search changed files for declared endpoint (exclude _test)
Impl-->>Verifier: Endpoint referenced / not referenced
alt Endpoint present
Verifier->>Verifier: Assess HTTP method association
Verifier-->>TaskSpec: PASS or WARNING (method ambiguous)
else Endpoint missing
Verifier-->>TaskSpec: FAIL
end
else Spec not found
Verifier-->>TaskSpec: WARNING (spec missing; defer field-level checks)
end


Comment @coderabbitai help to get the list of available commands and usage tips.

@jgbr1el93 jgbr1el93 requested a review from fredcamaral April 11, 2026 17:25
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between ebc7a9e and 74d4b9c.

📒 Files selected for processing (2)
  • dev-team/skills/dev-delivery-verification/SKILL.md
  • pm-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)
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 74d4b9c and e09913d.

📒 Files selected for processing (2)
  • dev-team/agents/performance-reviewer.md
  • dev-team/skills/dev-delivery-verification/SKILL.md

Comment on lines +1 to +45
---
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"
---
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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:

  1. This file was included in the PR by mistake, or
  2. 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>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between e09913d and 35da201.

📒 Files selected for processing (3)
  • dev-team/agents/performance-reviewer.md
  • dev-team/skills/dev-delivery-verification/SKILL.md
  • dev-team/skills/shared-patterns/standards-coverage-table.md

Comment on lines +49 to +68
## 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.**

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Comment on lines +687 to +689
if [ -z "$contracts" ]; then
echo "CONTRACT_COMPLIANCE: ⚠️ SKIP — no Integration Contracts section in task spec"
else
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Comment on lines +726 to +737
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants