Skip to content

feat: add canonical Relay agent skills - #32

Merged
krishna916 merged 8 commits into
mainfrom
feature/issue-23-canonical-relay-skills
Jul 28, 2026
Merged

feat: add canonical Relay agent skills#32
krishna916 merged 8 commits into
mainfrom
feature/issue-23-canonical-relay-skills

Conversation

@krishna916

@krishna916 krishna916 commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Summary

  • add canonical Relay Capture and Session Review skills with deterministic fixtures
  • validate policy frontmatter, canonical-source boundaries, contract links, and required fixture coverage
  • format repository sources so the full verification gate is clean

Validation

  • pnpm verify

Vendor packaging and marketplace assets remain deferred to #24/#25.

Summary by CodeRabbit

  • Documentation

    • Added guidance for Relay capture and session-review workflows.
    • Documented MCP preference and CLI JSON fallback behavior.
    • Clarified session ownership, isolation, review requirements, and agent autonomy boundaries.
    • Added canonical skill guides and positive/negative scenario examples.
  • Validation

    • Added checks to ensure required skill documentation, fixtures, links, and policy content remain consistent.
  • Tests

    • Added coverage for skill validation, repository asset validation, session review requirements, and prohibited policy patterns.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@krishna916, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3662e57c-700b-4f2f-9b25-55fd0a1ff6f2

📥 Commits

Reviewing files that changed from the base of the PR and between 24db71b and 6c66400.

📒 Files selected for processing (5)
  • scripts/validate-repository-assets.ts
  • scripts/validate-skill-assets.ts
  • skills/fixtures/session-review-negative.md
  • tests/unit/scripts/validate-repository-assets.test.ts
  • tests/unit/scripts/validate-skill-assets.test.ts
📝 Walkthrough

Walkthrough

Adds canonical Relay agent skill documentation and fixtures, introduces comprehensive skill-asset validation, integrates it into repository validation, and adds unit tests covering policy, fixture, provenance, session-review, and canonical-source rules.

Changes

Canonical agent skills

Layer / File(s) Summary
Skill policy and fixture assets
README.md, docs/agent-skills.md, skills/relay-*/SKILL.md, skills/fixtures/*, docs/superpowers/plans/...
Documents canonical capture and session-review behavior, MCP/CLI selection, session semantics, policy constraints, remediation tasks, and deterministic ACCEPT/REJECT fixture scenarios.
Skill asset validator
scripts/validate-skill-assets.ts
Validates canonical skill structure, frontmatter, contract links, required policy wording, forbidden patterns, canonical-source reuse, fixture parsing, coverage, expected outcomes, and unique case IDs.
Repository validation wiring and tests
scripts/validate-repository-assets.ts, tests/unit/scripts/validate-repository-assets.test.ts, tests/unit/scripts/validate-skill-assets.test.ts
Requires the new assets, validates README links and skill content during repository checks, and adds unit coverage for missing assets, malformed fixtures, unsafe policies, provenance, session review, vendor copies, and legacy paths.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related issues

  • krishna916/relay#2 — Covers the canonical MCP/CLI agent skills, session semantics, fixtures, and repository asset enforcement implemented here.
  • krishna916/relay#23 — Directly corresponds to the canonical Relay Capture and Session Review skills, fixtures, and deterministic validation.

Possibly related PRs

  • krishna916/relay#3 — Introduced the repository-validation script that this PR extends for skill and fixture assets.
  • krishna916/relay#27 — Also extends the repository validator’s required asset paths, although for a different contract domain.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: introducing canonical Relay agent skills and related docs/validators.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/issue-23-canonical-relay-skills

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.

❤️ Share

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

Copy link
Copy Markdown
Owner Author

Luna: step-by-step remediation plan for PR #32

Address the review findings below in order. Keep the work strictly within issue #23 scope: canonical skills, deterministic fixtures, validators/tests, and supporting docs only. Do not add MCP/CLI implementation, lifecycle logic, vendor packaging, marketplace work, live-LLM tests, or a policy engine.


Task 1 — Fix the Relay Capture provenance wording

Problem

skills/relay-capture/SKILL.md currently says both:

  • provide agent name and session ID; and
  • "do not supply status or provenance fields"

This is ambiguous and conflicts with the actual task_capture contract. The caller must provide createdByName and sessionId, while Relay owns createdByType: AGENT and status: INBOX.

Required changes

Edit:

  • skills/relay-capture/SKILL.md

In the Capture procedure section, replace the broad instruction not to supply provenance fields with precise adapter-owned/caller-owned guidance.

The resulting policy must explicitly state:

  1. The agent supplies:
    • title
    • createdByName
    • sessionId
    • optional concise workspace
    • optional concise sourceContext
    • other optional fields only when allowed by the authoritative contract
  2. The agent must not supply:
    • createdByType
    • status
    • lifecycle timestamps
    • any other adapter-owned or forbidden fields
  3. Relay sets autonomous captures to INBOX.
  4. The skill must link to docs/mcp-tools.md rather than duplicating the complete schema.

Suggested wording:

Use task_capture, or the documented source-checkout CLI invocation for task capture --output json. Supply the required title, createdByName, and exact active sessionId, plus concise optional workspace and source context when available. Do not supply adapter-owned createdByType, status, lifecycle timestamps, or other fields forbidden by the authoritative contract; Relay sets autonomous captures to INBOX.

Tests

Update/add focused tests in:

  • tests/unit/scripts/validate-skill-assets.test.ts

Add a regression test proving validation fails when the capture skill contains the old ambiguous wording or omits required caller-owned provenance guidance.

Prefer validating policy concepts rather than one entire exact sentence. The test should require all of these concepts:

  • createdByName
  • exact/active sessionId
  • adapter-owned createdByType
  • Relay-owned/default INBOX or status

Acceptance criteria

  • An agent reading the skill can tell exactly which provenance fields it must provide.
  • No wording can be reasonably interpreted as forbidding createdByName or sessionId.
  • The skill still avoids duplicating the complete MCP/CLI schema.

Task 2 — Make session review unconditional before final completion

Problem

The prohibited-behaviour wording currently says review must not be omitted "when captures may exist". That allows an agent to guess that no captures exist and skip the authoritative exact-session lookup.

Issue #23 requires the agent to call the session-captures capability with the exact active session ID before final completion. An empty result is valid and authoritative.

Required changes

Edit:

  • skills/relay-session-review/SKILL.md

Make the policy unambiguous:

  1. Before final completion, always perform the exact-session lookup for the active Relay session.
  2. Do not rely on memory, cached IDs, timestamps, or an assumption that nothing was captured.
  3. If the result is empty, do not invent or reconstruct captures.
  4. User-triggered wrap-up/review remains an additional trigger, not a replacement for final-completion review.
  5. Continue to prohibit timer, inactivity, process-exit, or guessed-session inference.

Suggested wording for Prohibited behaviour:

Never omit the exact-session lookup before final completion, even when the agent believes no captures exist. Treat an empty authoritative result as valid. Never query a guessed session, reconstruct captures from memory or timestamps, silently apply dispositions, use a generic status mutation, hide completed or archived captures, or infer completion from a timer, inactivity, or process exit.

Fixture updates

Review and, where useful, strengthen:

  • skills/fixtures/session-review-positive.md
  • skills/fixtures/session-review-negative.md

Add or update a negative case covering:

  • agent believes it captured nothing;
  • agent skips session_captures_list before final completion;
  • expected result is REJECT because the lookup itself is mandatory.

Use a stable issue-specific fixture ID such as:

  • REVIEW-SKIP-EMPTY-006

Do not replace existing required fixture IDs.

Tests

Update/add tests in:

  • tests/unit/scripts/validate-skill-assets.test.ts

Add a regression test proving validation fails when the session-review skill makes the pre-completion lookup conditional, for example through phrases such as:

  • when captures may exist
  • if captures exist
  • if the agent remembers capturing tasks

Keep the forbidden checks narrow enough to avoid false positives in explanatory examples.

Acceptance criteria

  • The exact active-session lookup is mandatory before final completion.
  • Empty lookup results are explicitly supported.
  • No agent judgement is allowed to replace the authoritative query.

Task 3 — Remove the fixture-coverage validation bypass

Problem

validateFixtureCoverage() currently returns early when every fixture ID starts with CASE-:

if (cases.every((fixtureCase) => fixtureCase.id.startsWith('CASE-'))) return;

This allows all required issue-specific fixtures to be replaced by arbitrary CASE-* fixtures while production validation still passes.

Required changes

Edit:

  • scripts/validate-skill-assets.ts

Remove the CASE-* bypass entirely.

Production validation must always enforce the required fixture IDs for each canonical fixture file.

Do not add another content-based test-mode escape hatch.

Test-fixture strategy

If unit tests currently depend on generic CASE-* fixture IDs, update the test helper to create the real minimum required IDs for each fixture file.

Recommended helper behaviour:

  • capture-positive.md includes at minimum:
    • CAPTURE-ACTIONABLE-001
    • CAPTURE-DUPLICATE-002
    • CAPTURE-CLI-FALLBACK-003
  • capture-negative.md includes at minimum:
    • CAPTURE-SENSITIVE-002
    • CAPTURE-MUTATION-003
    • CAPTURE-SESSION-005
    • CAPTURE-ADAPTER-006
  • session-review-positive.md includes at minimum:
    • REVIEW-ACTIVE-SESSION-001
    • REVIEW-EXPLICIT-ACTIONS-002
    • REVIEW-UNRESOLVED-003
  • session-review-negative.md includes at minimum:
    • REVIEW-OMITTED-001
    • REVIEW-WRONG-SESSION-002
    • REVIEW-SILENT-MUTATION-003
    • REVIEW-TIMER-005

The helper may generate minimal valid case bodies, but it must not disable production invariants.

Tests

Add a regression test proving:

  1. all fixture files contain syntactically valid CASE-* entries;
  2. required issue-specific IDs are absent;
  3. validateSkillAssets() fails.

Also retain or add a positive test proving the canonical required fixture set passes.

Acceptance criteria

  • There is no production bypass based on fixture naming.
  • Required coverage cannot be removed without failing validation.
  • Tests use valid canonical fixture IDs instead of weakening production code.

Task 4 — Add deterministic forbidden-policy checks

Problem

The validator currently checks that required safe phrases exist, but it does not reject contradictory unsafe permissions elsewhere in the same skill.

Example of a currently possible false pass:

  • one paragraph says agents must not archive autonomously;
  • another says agents may archive low-priority tasks autonomously.

Issue #23 requires forbidden vague or unsafe instructions to be absent.

Required changes

Edit:

  • scripts/validate-skill-assets.ts

Add a small, explicit validation layer for unsafe policy grants. Keep it deterministic and narrow. Do not attempt natural-language understanding or build a general policy engine.

Suggested structure:

function validateForbiddenPolicies(content: string, rules: readonly ForbiddenPolicyRule[]): void

Each rule should contain:

  • a narrow regular expression;
  • a human-readable label;
  • the skill(s) it applies to, if needed.

Minimum forbidden concepts to reject

For relay-capture:

  • autonomous permission to edit an existing task;
  • autonomous permission to triage/move/start/complete/archive/delete/merge an existing task;
  • permission to move a new autonomous capture out of INBOX without explicit user direction;
  • permission to store prompts, transcripts, source files, secrets, credentials, tokens, large stack traces, logs, or oversized copied context;
  • permission to reuse one session ID across unrelated concurrent sessions;
  • permission to switch MCP and CLI during one workflow without failure, unavailability, debugging need, or explicit reason;
  • permission to parse decorative/human CLI output instead of JSON.

For relay-session-review:

  • permission to mutate without explicit user direction;
  • permission to use a generic/unrestricted status mutation;
  • permission to skip the exact-session lookup before final completion;
  • permission to use a guessed/different session ID;
  • permission to omit completed or archived captures returned by Relay;
  • permission to infer completion from timer, inactivity, or process exit.

Important implementation constraint

Avoid naive patterns that reject valid negative statements such as:

Never archive autonomously.

The forbidden patterns should target affirmative permission wording, for example combinations such as:

  • may autonomously ... archive
  • can silently ... complete
  • without user direction ... move
  • skip ... session lookup

Keep the patterns inspectable and covered by focused tests.

Tests

Update:

  • tests/unit/scripts/validate-skill-assets.test.ts

Add table-driven tests for at least these contradictions:

  1. Required safe autonomy wording exists, but another paragraph permits autonomous archive.
  2. Required exact-session wording exists, but another paragraph permits skipping review when the agent believes there are no captures.
  3. Required JSON wording exists, but another paragraph permits parsing decorative CLI output.
  4. Required context-safety wording exists, but another paragraph permits storing full source files or secrets.

Each test must fail for the unsafe contradiction, not merely because a required phrase was removed.

Also include positive controls showing the canonical wording passes.

Acceptance criteria

  • Contradictory affirmative permissions fail deterministic validation.
  • Valid prohibition wording does not produce false positives.
  • The implementation remains a small explicit guardrail, not a policy engine.

Task 5 — Review and strengthen fixture coverage

Review all four canonical fixture files:

  • skills/fixtures/capture-positive.md
  • skills/fixtures/capture-negative.md
  • skills/fixtures/session-review-positive.md
  • skills/fixtures/session-review-negative.md

Ensure fixtures still cover every issue #23 requirement after the policy fixes.

Capture positive coverage

Must include:

  • actionable follow-up captured without derailing current work;
  • duplicate warning treated as advisory;
  • CLI JSON fallback when MCP is unavailable.

Capture negative coverage

Must include:

  • speculative/every-thought capture;
  • transcript/source-code/secret/oversized context;
  • silent edit/triage/start/complete/archive of existing tasks;
  • moving an autonomous capture out of INBOX;
  • session ID reuse across unrelated concurrent sessions;
  • unjustified MCP/CLI switching.

Session-review positive coverage

Must include:

  • exact active-session lookup before final completion;
  • completed and archived captures included;
  • explicit user-selected triage/complete/archive actions;
  • unresolved captures left unchanged in INBOX;
  • CLI JSON fallback.

Session-review negative coverage

Must include:

  • omitted pre-completion lookup, including the "I think there were no captures" case;
  • wrong/guessed session ID;
  • silent mutation;
  • mixing another session’s tasks;
  • timer/inactivity/process-exit inference;
  • hiding completed or archived captures.

Keep fixtures concise and deterministic. They are policy examples, not executable end-to-end tests and not live-LLM evaluations.


Task 6 — Update documentation only where policy changed

Review:

  • docs/agent-skills.md
  • README.md

Only change docs where needed to keep them consistent with the corrected canonical skills.

The docs should state:

  • MCP preferred, CLI JSON fallback;
  • one adapter retained through one workflow unless unavailable;
  • caller supplies the active session ID and agent name where required;
  • Relay owns createdByType and autonomous capture status;
  • exact-session lookup always occurs before final completion;
  • empty lookup results are authoritative;
  • canonical policy remains in the two SKILL.md files.

Do not duplicate detailed schemas or lifecycle rules into these docs.


Task 7 — Diagnose and fix the current CI failure

The current PR workflow fails in the Run verification gate step. Do not merely rerun CI before identifying the deterministic cause.

Required sequence

Run locally from a clean checkout of the PR branch:

corepack enable
pnpm install --frozen-lockfile
pnpm verify

If pnpm verify fails, isolate the failing command by running the repository scripts individually in the same order used by verify, likely including:

pnpm format:check
pnpm lint
pnpm typecheck
pnpm test
pnpm test:coverage
pnpm build

Use the actual package.json script definition as authoritative if the order differs.

Constraints

  • Fix the underlying failure; do not weaken quality gates.
  • Do not reduce coverage thresholds.
  • Do not add || true, ignored warnings, broad excludes, or CI-only bypasses.
  • Do not make pnpm verify mutate tracked files.
  • If formatting is the failure, format only the files touched by this PR and confirm format:check is clean afterwards.

Required evidence in the PR

Update the PR description or add a completion comment with:

  • the exact command run;
  • confirmation that pnpm verify passes locally;
  • the resulting green GitHub Actions run.

Task 8 — Final self-review against issue #23

Before requesting re-review, perform these human-readable checks:

  1. Read skills/relay-capture/SKILL.md as an agent.
    • Confirm there is no accidental permission to mutate existing tasks.
    • Confirm required caller-owned provenance is clear.
    • Confirm adapter-owned fields are clear.
  2. Read skills/relay-session-review/SKILL.md as an agent.
    • Confirm exact-session lookup is unconditional before final completion.
    • Confirm empty results are authoritative.
    • Confirm no timer, inactivity, memory, or process-exit inference remains.
  3. Confirm context guidance is privacy-preserving and concrete.
  4. Confirm MCP/CLI selection is deterministic.
  5. Confirm no command schema or business logic has been duplicated into skills.
  6. Confirm no vendor-specific directory became a canonical policy source.
  7. Confirm no issue Create canonical Relay Capture and Session Review skills #23 forbidden scope was introduced.
  8. Run:
pnpm verify
  1. Push changes and confirm GitHub Actions is green.

Completion checklist

Before marking this review addressed, all boxes must be true:

  • Capture skill distinguishes caller-owned and adapter-owned provenance/status fields.
  • Capture skill cannot be interpreted as forbidding required createdByName or sessionId.
  • Session review always performs exact-session lookup before final completion.
  • Empty session lookup results are explicitly authoritative.
  • CASE-* fixture coverage bypass is removed.
  • Tests prove required fixture IDs cannot be bypassed.
  • Narrow forbidden-policy validation rejects contradictory unsafe permissions.
  • Positive controls prove valid canonical wording passes.
  • Fixtures cover the new regression cases.
  • Documentation remains consistent without duplicating contracts.
  • pnpm verify passes locally from a clean install.
  • GitHub Actions is green.
  • No out-of-scope implementation or packaging work was added.

After completing the above, add a PR comment summarising each fix by task number and include the final verification result.

@krishna916
krishna916 marked this pull request as ready for review July 28, 2026 17:24

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🤖 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 `@scripts/validate-repository-assets.ts`:
- Around line 201-209: Update the README validation loop in
scripts/validate-repository-assets.ts to parse Markdown link targets rather than
using readme.includes(requiredLink). Normalize each extracted target and compare
it against the required paths, so plain-text or code-block mentions do not
satisfy the check while valid links do.

In `@scripts/validate-skill-assets.ts`:
- Around line 295-302: Update the validation around canonicalSkillPaths and the
relay policy check to identify the applicable canonical skill from the asset’s
content or metadata, not its file path. Replace byte-for-byte comparison with
validation that the content contains an explicit link/reference to the selected
canonical source, allowing vendor skills to reference either Capture or Session
Review without requiring duplicated content.
- Around line 162-165: Update the exact-session lookup validation rule near the
pattern definition and its evaluation logic to recognize prohibition wording
such as “Do not skip…” and “must not omit…” as valid directives rather than
rejecting them. Require an affirmative permission or directive before reporting
this violation, and add regression cases covering both prohibition forms.

In `@skills/fixtures/session-review-negative.md`:
- Around line 97-111: Rename the duplicate fixture ID in
skills/fixtures/session-review-negative.md:97-111 to a unique canonical ID such
as REVIEW-GENERIC-MUTATION-007. Add the same ID to the required
session-review-negative coverage in scripts/validate-skill-assets.ts:91-99 and
to the generated valid fixture root in
tests/unit/scripts/validate-skill-assets.test.ts:47-53.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f0918de7-4373-4fa7-a86f-e8397b433a59

📥 Commits

Reviewing files that changed from the base of the PR and between 477e613 and 24db71b.

📒 Files selected for processing (14)
  • README.md
  • docs/agent-skills.md
  • docs/superpowers/plans/2026-07-28-issue-23-canonical-relay-skills.md
  • docs/superpowers/plans/2026-07-28-pr-32-review-remediation.md
  • scripts/validate-repository-assets.ts
  • scripts/validate-skill-assets.ts
  • skills/fixtures/capture-negative.md
  • skills/fixtures/capture-positive.md
  • skills/fixtures/session-review-negative.md
  • skills/fixtures/session-review-positive.md
  • skills/relay-capture/SKILL.md
  • skills/relay-session-review/SKILL.md
  • tests/unit/scripts/validate-repository-assets.test.ts
  • tests/unit/scripts/validate-skill-assets.test.ts

Comment thread scripts/validate-repository-assets.ts
Comment thread scripts/validate-skill-assets.ts
Comment thread scripts/validate-skill-assets.ts Outdated
Comment thread skills/fixtures/session-review-negative.md Outdated
@krishna916
krishna916 merged commit c54b3df into main Jul 28, 2026
2 checks passed
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.

1 participant