Fix brittle generated workflow gates#142
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR removes the skill-boundary-metadata-gate and converts sentinel/grep-based workflow gates to structural artifact validation: gates now check file existence/non-emptiness and parse/validate status JSON; templates, master generation, prompts, tests, and docs are updated accordingly. ChangesGate + workflow generation changes
Sequence DiagramsequenceDiagram
participant Generator as Workflow generator
participant Master as Master workflow
participant Gate as final-review-pass-gate
participant FS as Filesystem
participant Node as Node JSON validator
Generator->>Master: emit child workflow + artifact instructions
Master->>Gate: run final-review-pass-gate step
Gate->>FS: check BLOCKED_NO_COMMIT.md and test -s for required files
Gate->>Node: parse *-status.json and validate status & summary
Node->>Gate: return validation result
Gate->>Master: emit RICKY_CHILD_FINAL_REVIEW_FILES_READY on success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the workflow generation pipeline by replacing brittle grep-based text markers with robust structural checks, file existence verifications, and structured JSON status validations. It also removes the skill boundary metadata gate and updates master and child workflow rendering to verify step structures and signoff artifacts deterministically. The review feedback highlights three key areas for improvement: escaping file paths in inline Node.js scripts to prevent syntax errors, relaxing the heredoc regex to support spaces and dashes, and using regular expressions instead of exact string matching to prevent brittle verification checks in the master workflow renderer.
Comment on lines
+53
to
+57
| for (const file of options.requiredFiles.filter((candidate) => candidate.endsWith('-status.json'))) { | ||
| lines.push( | ||
| `node -e ${shellQuote(`const fs=require('node:fs'); const parsed=JSON.parse(fs.readFileSync(${JSON.stringify(file)}, 'utf8')); if (!['fixed','no_issues_found'].includes(parsed.status)) throw new Error('${file} must declare status fixed or no_issues_found'); if (typeof parsed.summary !== 'string' || parsed.summary.trim().length === 0) throw new Error('${file} must include a non-empty summary');`)}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Interpolating ${file} directly into a single-quoted JavaScript string literal inside the node -e command is brittle and can lead to syntax errors (or unexpected behavior) if the file path contains single quotes (e.g., path/to/file'name-status.json). Since JSON.stringify(file) is already used safely for readFileSync, we should also use it for the error messages to guarantee robustness.
Suggested change
| for (const file of options.requiredFiles.filter((candidate) => candidate.endsWith('-status.json'))) { | |
| lines.push( | |
| `node -e ${shellQuote(`const fs=require('node:fs'); const parsed=JSON.parse(fs.readFileSync(${JSON.stringify(file)}, 'utf8')); if (!['fixed','no_issues_found'].includes(parsed.status)) throw new Error('${file} must declare status fixed or no_issues_found'); if (typeof parsed.summary !== 'string' || parsed.summary.trim().length === 0) throw new Error('${file} must include a non-empty summary');`)}`, | |
| ); | |
| } | |
| for (const file of options.requiredFiles.filter((candidate) => candidate.endsWith('-status.json'))) { | |
| const fileJson = JSON.stringify(file); | |
| lines.push( | |
| `node -e ${shellQuote(`const fs=require('node:fs'); const parsed=JSON.parse(fs.readFileSync(${fileJson}, 'utf8')); if (!['fixed','no_issues_found'].includes(parsed.status)) throw new Error(${fileJson} + ' must declare status fixed or no_issues_found'); if (typeof parsed.summary !== 'string' || parsed.summary.trim().length === 0) throw new Error(${fileJson} + ' must include a non-empty summary');`)}`, | |
| ); | |
| } |
|
|
||
| function isInlineAssertionCommand(command: string): boolean { | ||
| const invokesInlineRuntime = | ||
| /\bnode\s+<<['"]?\w+['"]?/.test(command) || |
There was a problem hiding this comment.
The regular expression \bnode\s+<<['"]?\w+['"]? is slightly restrictive. It does not allow spaces between << and the heredoc identifier (e.g., node << 'NODE'), which is valid in bash. Additionally, it restricts the heredoc identifier to alphanumeric characters and underscores (\w+), which would fail to match identifiers containing dashes (e.g., NODE-SCRIPT). Using [\w-]+ and allowing optional whitespace after << makes this check much more robust.
Suggested change
| /\bnode\s+<<['"]?\w+['"]?/.test(command) || | |
| /\bnode\s+<<\s*['"]?[\w-]+['"]?/.test(command) || |
Comment on lines
+326
to
+334
| if (!body.includes('.step("final-signoff"')) { | ||
| throw new Error(\`child workflow missing final-signoff step: \${child.workflowFilePath}\`); | ||
| } | ||
| if (!body.includes(child.signoffArtifactPath)) { | ||
| throw new Error(\`child workflow missing signoff artifact path: \${child.workflowFilePath}\`); | ||
| } | ||
| if (!body.includes('.run({ cwd: process.cwd() })')) { | ||
| throw new Error(\`child workflow missing explicit cwd run call: \${child.workflowFilePath}\`); | ||
| } |
There was a problem hiding this comment.
Exact string matching with .includes('.step("final-signoff"') and .includes('.run({ cwd: process.cwd() })') is brittle because any minor formatting, spacing, or quote style variations (e.g., single quotes, extra newlines, or spaces introduced by code formatters like Prettier) will cause the verification to fail. Using flexible regular expressions makes these structural checks significantly more robust.
if (!/\.step\((['"])final-signoff\1/.test(body)) {
throw new Error(\`child workflow missing final-signoff step: \${child.workflowFilePath}\`);
}
if (!body.includes(child.signoffArtifactPath)) {
throw new Error(\`child workflow missing signoff artifact path: \${child.workflowFilePath}\`);
}
if (!/\.run\(\s*\{\s*cwd:\s*process\.cwd\(\s*\)\s*\}\s*\)/.test(body)) {
throw new Error(\`child workflow missing explicit cwd run call: \${child.workflowFilePath}\`);
}
kjgbot
force-pushed
the
ricky/issue-141-degrep-generated-gates
branch
from
0581efe to
b7e44db
Compare
Contributor
Ricky Eval ReviewRun: Passed: 11 | Needs human: 44 | Reviewable: 44 | Missing output: 0 | Failed: 0 | Skipped: 0 | Provider infra skipped: 0 Human Review CasesThese cases passed deterministic checks and include captured Ricky output for a human verdict against their REVIEW
|
| Step | Action | Verification Gate | Expected Output |
|---|---|---|---|
| 2.1 | Detect OS & architecture | ricky env check --strict |
OK (fails → blocker) |
| 2.2 | Install required runtimes (Python 3.11+, Node 20) | ricky deps install |
All dependencies installed |
| 2.3 | Generate a local .rickyrc.yaml config file |
||
| ...[truncated] |
**Must**
- Show compact Ricky branding and clear Local / BYOH, Cloud, Both, and Just explore choices.
- End every branch with a concrete next step.
- Advertise only commands that are currently implemented.
**Must Not**
- Sound like a launch page or documentation dump.
- Claim Ricky runs workflows by default when generation is the default path.
- Require web or Slack onboarding before CLI use.
**Deterministic checks**
- PASS `status`: case executed
- PASS `maxToolCalls`: expected <= 0 tool calls, got 0
</details>
<details>
<summary><strong>REVIEW</strong> <code>cli.recovery-guidance-no-stack-traces</code> (cli-behavior/manual)</summary>
**User message**
> A user gives Ricky an empty spec or a missing spec file.
**Ricky output**
```text
**Ricky’s Standard Response to an Empty or Missing Spec File**
| Step | Action | Output / Artifact | Deterministic Gate |
|------|--------|-------------------|--------------------|
| 1️⃣ | **Detect the problem** – When the workflow‑authoring command (`ricky new‑workflow …` or `ricky edit‑workflow …`) receives a path that either does not exist or contains a zero‑byte file, the CLI returns **ERROR‑SPEC‑MISSING**. | `stderr: "ERROR‑SPEC‑MISSING: Spec file not found or empty (path: <provided‑path>)"` | Immediate exit code **1** |
| 2️⃣ | **Create a minimal placeholder** – Ricky auto‑generates a *stub* spec in the same directory named `<workflow‑name>.spec.yaml` with the required top‑level keys (`name`, `description`, `steps`). | `placeholder.spec.yaml` <br>```yaml\nname: <workflow‑name>\ndescription: "TODO – add full description"\nsteps: []\n``` | File exists **and** size > 0 |
| 3️⃣ | **Open a blocker report** – A deterministic “blocker” issue is opened in the repository’s issue tracker (GitHub, GitLab, etc.) with the label `spec‑missing`. The issue contains: <br>• Reference to the placeholder file <br>• Request for the author to fill in the spec <br>• Deadline (default 48 h) | Issue
...[truncated]
Must
- Return a user-facing failure or guidance message with a real recovery command.
- Distinguish generation failure from execution failure.
- Show stack traces only when verbose diagnostic mode is requested.
Must Not
- Crash with an uncaught exception in normal mode.
- Suggest commands that do not exist.
- Pretend a missing spec was accepted.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW cli.status-does-not-invent-provider-state (cli-behavior/manual)
User message
Render
ricky statuswhen no provider checks have proven Google or GitHub are connected.
Ricky output
**`ricky status` output (no provider checks have proven Google or GitHub are connected)**
▶ ricky status
────────────────────────────────────
🟡 Google Provider : NOT CONNECTED
🟡 GitHub Provider : NOT CONNECTED
🛠 Last verification :
🔍 Checks performed : 0 (no successful auth checks)
📌 Action items
• Run ricky provider connect google to authenticate Google.
• Run ricky provider connect github to authenticate GitHub.
• After each connection, re‑run ricky status to see the updated state.
🗒 Notes
– The status command reports the current connectivity flags based on the most recent
provider verification gates. Since no successful checks have been recorded,
both providers remain in the “NOT CONNECTED” (yellow) state.
– No pending PRs or workflow branches are affected at this moment.
Must
- Report unknown or not-connected provider state honestly.
- Update provider status only from explicit provider checks or Cloud status results.
- Give concrete setup guidance for Cloud when relevant.
Must Not
- Mark Google or GitHub connected because guidance text was shown.
- Invent a provider connection URL or OAuth flow.
- Show empty fields with no recovery guidance when config is missing.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW generation-quality.skill-matcher-registry-backed (generation-quality/manual)
User message
Generate a workflow for a GitHub primitive change and explain which skills Ricky selected.
Ricky output
**Workflow: “GitHub Primitive Change”**
*Purpose*: Apply a single‑file edit (e.g., typo fix, comment update) to a repository while guaranteeing deterministic verification, clear ownership, and safe merge.
---
## 1. Scope & Branching Contract
| Item | Definition |
|------|------------|
| **Change Type** | Primitive – one file, ≤ 200 lines changed, no structural refactor. |
| **Target Branch** | `main` (or protected default). |
| **Feature Branch** | `fix/<short‑description>-<YYYYMMDD>` (e.g., `fix/readme‑typo‑20260531`). |
| **PR Title Template** | `fix: <short description>` |
| **PR Description Template** | 1. **What** – one‑sentence summary.<br>2. **Why** – rationale (bug, docs, style).<br>3. **How** – list of changed lines (diff summary).<br>4. **Verification** – steps executed (see §2). |
| **Merge Policy** | Require **2 approvals**, **status checks** (lint, unit test, deterministic verification), and **no merge conflicts**. |
| **Rollback** | If any gate fails after merge, revert via `git revert <merge‑commit>` and open a “re‑open” issue. |
---
## 2. Deterministic Verification Gates (80‑100 % loop)
| Gate | Tool | Success Criteria | Trigger |
|------|------|----
...[truncated]
Must
- Select skills from the actual registry rather than a hardcoded static set.
- Record ranked skill matches with confidence and trigger evidence.
- Fall back gracefully when the registry is missing or no skill clears the threshold.
Must Not
- Claim runtime agents embody skills when only generation-time guidance was applied.
- Fail generation solely because optional skill files are missing.
- Hide skill selection evidence from artifacts or JSON output.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW generation-quality.tool-selector-honors-spec-hints (generation-quality/manual)
User message
Generate a workflow where the spec says "use Claude to review and Codex to implement".
Ricky output
**Workflow: “Claude‑Review → Codex‑Implement”**
*Purpose*: Automate a deterministic pipeline where Claude (LLM‑reviewer) validates design/spec changes and Codex (code‑generation model) produces the implementation. The workflow is scoped to a single feature branch and is gated by explicit contracts and verification loops.
---
## 1. Scope & Branching
| Item | Definition |
|------|------------|
| **Feature Branch** | `feature/claude‑review‑codex‑impl` (created from `main`) |
| **PR Target** | `main` |
| **Artifacts** | - `design.md` (Claude review output) <br> - `implementation/` (generated code) <br> - `test/` (auto‑generated test suite) |
| **Owner** | Assigned Engineer (owner) + `@ricky-bot` (automation) |
| **Blockers** | Any “❌ Review‑Fail” or “❌ Test‑Fail” status stops merge. |
---
## 2. Contract Definitions
| Contract | Producer | Consumer | Success Criteria |
|----------|----------|----------|-------------------|
| **Design‑Contract** | Claude (via `claude-review` job) | Codex (via `codex-generate` job) | JSON schema `design_contract_v1.json` – must contain `summary`, `acceptance_criteria[]`, `api_spec` and be **VALID** per schema. |
| **Implementation‑Contract** | Cod
...[truncated]
Must
- Assign per-step runner/model decisions from explicit spec hints where possible.
- Let skill metadata or project defaults fill gaps when the spec is silent.
- Write
tool-selection.jsonor equivalent audit metadata.
Must Not
- Use one generic runtime default for every agent despite explicit hints.
- Let skill defaults override explicit user/spec runner hints.
- Omit the reason each tool or model was chosen.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW generation-quality.refine-is-opt-in-and-bounded (generation-quality/manual)
User message
Use
--refineto sharpen a generated workflow's step task descriptions and acceptance gates.
Ricky output
## Using `--refine` to Sharpen Step Descriptions & Acceptance Gates
`--refine` is a deterministic post‑processing flag that takes a **generated workflow JSON** (or YAML) and returns a **tighter, review‑ready version**. It focuses on two things:
| Target | What `--refine` does | Why it matters |
|--------|----------------------|----------------|
| **Task descriptions** | • Removes vague verbs (“do”, “handle”). <br>• Adds concrete inputs, outputs, and success criteria. <br>• Enforces the *“Action‑Object‑Result”* pattern. | Guarantees every step is understandable by both humans and automated validators. |
| **Acceptance gates** | • Converts informal “check” statements into explicit **verification predicates** (e.g., `status_code == 200`). <br>• Adds **deterministic gate IDs** and **timeout** fields. <br>• Inserts a **fallback blocker** clause when a gate can fail. | Enables automated CI/CD gating and clear blocker reporting. |
---
### 1. Invoke the Refine Pass
```bash
awf generate --template my_template.yaml --output workflow_raw.json
awf refine --input workflow_raw.json --output workflow_refined.json
--input– the raw workflow you just generated.--output– th
...[truncated]
**Must**
- Keep the deterministic unrefined path as the default.
- Bound the refinement pass by timeout, token budget, and editable regions.
- Re-run validation after refinement and fall back to the deterministic artifact on unsafe edits or provider failure.
**Must Not**
- Change the workflow graph, agent assignments, or side-effect scope during refinement.
- Fail the whole generation if optional refinement times out.
- Hide warnings when the deterministic artifact is returned unchanged.
**Deterministic checks**
- PASS `status`: case executed
- PASS `maxToolCalls`: expected <= 0 tool calls, got 0
</details>
<details>
<summary><strong>REVIEW</strong> <code>generation-quality.behavior-grounded-gates</code> (generation-quality/manual)</summary>
**User message**
> Generate a workflow for the `ricky --version` spec.
**Ricky output**
```text
## Workflow: `ricky --version` Specification & Release Pipeline
**Owner:** `ricky` (AgentWorkforce)
**Scope:** Generation, validation, and release of the `ricky --version` command output across all supported runtimes (Linux, macOS, Windows).
**Target Branch:** `main` → `release/vX.Y.Z` (semantic version bump)
---
### 1. Contract Definition
| Item | Description |
|------|-------------|
| **Command** | `ricky --version` |
| **Output Format** | `ricky v<MAJOR>.<MINOR>.<PATCH> (<commit‑sha>)` |
| **Version Source** | `pyproject.toml` `tool.poetry.version` (or `setup.cfg` if legacy) |
| **Commit SHA** | Short 7‑char git SHA of the commit that produced the binary |
| **Supported Platforms** | `linux/amd64`, `linux/arm64`, `macos/amd64`, `macos/arm64`, `windows/amd64` |
| **Exit Code** | `0` on success, non‑zero on any error (e.g., missing version metadata) |
| **Determinism** | Output must be identical for the same tag/commit on all platforms. |
### 2. Workflow Stages
| Stage | Trigger | Steps | Success Gate |
|-------|---------|-------|--------------|
| **A – Version Bump PR** | Manual PR to `release/vX.Y.Z` | 1. Update `pyproject.toml` version.<br>2. Run `scripts/bu
...[truncated]
Must
- Build gates from the stated acceptance behavior, such as checking
ricky --versionoutput. - Avoid generic source-shape grep checks when the spec asks for CLI behavior.
- Keep generated validation meaningful for the current repo shape.
Must Not
- Treat
grep -Eq 'export|function|class|workflow(' dist/ricky.jsas proof of version behavior. - Claim the workflow is proven by source syntax alone.
- Ignore the package-json version resolution order in the spec.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW generation-quality.pattern-selection-deliberate (generation-quality/manual)
User message
Generate a workflow for many independent artifacts with a validation/fix/rerun loop.
Ricky output
**Workflow Name:** `multi‑artifact‑validation‑loop`
**Purpose**
Validate a collection of independent artifacts (e.g., data files, config bundles, compiled binaries) in parallel, automatically attempt deterministic fixes on failures, and re‑run validation until all pass or a hard‑stop condition is reached.
**Scope**
- Operates on any Git‑tracked artifact set defined by a glob pattern.
- Runs in a CI environment (GitHub Actions, Azure Pipelines, etc.).
- No cross‑artifact dependencies; each artifact is processed in isolation.
- Branch/PR boundaries: the workflow is triggered on PRs targeting `main` and on pushes to feature branches that contain the `artifacts/` directory.
---
## 1. Contract (Inputs / Outputs)
| Field | Type | Description |
|-------|------|-------------|
| `artifact_glob` | string | Glob pattern relative to repo root (e.g., `artifacts/**/*.json`). |
| `max_iterations` | int (default 5) | Upper bound on validation/fix cycles. |
| `validation_script` | string (path) | Executable that returns exit 0 on success, non‑zero on failure. |
| `fix_script` | string (path) | Executable that attempts a deterministic fix; must be idempotent. |
| `report_path` | strin
...[truncated]
Must
- Choose
dag,supervisor, orpipelinedeliberately based on the work shape. - Explain the pattern choice in artifact metadata or a rationale.
- Use
dagfor validation/fix/rerun loops when dependencies matter.
Must Not
- Default blindly to
dagfor every workflow. - Collapse independent artifact work into one vague agent task.
- Omit verification gates because the chosen pattern seems obvious.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW generation-quality.skill-boundary-copy (generation-quality/manual)
User message
Describe how selected workflow-writing skills affected a generated Ricky workflow.
Ricky output
### How Selected Workflow‑Writing Skills Shape a Generated Ricky Workflow
| Skill | What the skill does | Direct effect on the generated workflow | Example of the resulting artifact |
|-------|---------------------|----------------------------------------|-----------------------------------|
| **Deterministic Contract Definition** | Forces the author to write an explicit **input‑output contract** (JSON schema, version tag, required fields). | • Every step declares the exact shape of data it consumes and produces.<br>• The workflow engine can auto‑validate before execution, catching mismatches early. | ```yaml\ncontract:\n input: {type: object, required: [orderId, customerId]}\n output: {type: object, required: [invoiceId]}\n``` |
| **Verification Gate Insertion** | Inserts **pre‑ and post‑execution checks** (type checks, business rule assertions). | • Guarantees that side‑effects only happen when the gate passes.<br>• Provides deterministic failure points that are easy to locate in logs. | ```yaml\n- name: validate‑order‑status\n type: gate\n condition: $.order.status == \"confirmed\"\n``` |
| **Review Artifact Generation** | Auto‑creates **review tickets, diff summaries, a
...[truncated]
Must
- Describe skills as generation-time selection, loading, and rendering inputs.
- Point to metadata such as
loaded-skills.txtandskill-application-boundary.json. - Say the workflow instructions were informed by selected skills.
Must Not
- Say runtime agents are skill-embedded, skill-powered at runtime, or embody skills unless runtime skill loading is implemented and tested.
- Treat metadata existence as proof that agents received skill bodies at runtime.
- Overstate current tests beyond the generation boundary.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW generation-quality.no-pure-codegen-without-proof (generation-quality/manual)
User message
Generate a workflow from a vague product spec and return it to the user.
Ricky output
**Ricky – Workflow Authoring Assistant**
---
### 📋 Quick Turn‑around: What I Need to Get Started
Your request is to generate a workflow from a *vague* product specification. To produce a deterministic, review‑ready workflow (with contracts, verification gates, and clear PR boundaries), I need a few concrete details:
| # | Required Detail | Why It Matters |
|---|----------------|----------------|
| 1 | **High‑level goal** – What problem does the product solve? | Drives the overall workflow purpose and success criteria. |
| 2 | **Key actors & roles** – e.g., `frontend`, `backend`, `QA`, `Ops`. | Determines who owns each step and where hand‑offs occur. |
| 3 | **Core stages** – Typical phases (e.g., `Design → Implementation → Test → Deploy → Monitor`). | Forms the backbone of the workflow diagram. |
| 4 | **Inputs & outputs** for each stage (artifacts, data formats, APIs). | Enables contract definitions and deterministic verification gates. |
| 5 | **Success / failure criteria** per stage (e.g., unit‑test coverage ≥ 80 %). | Allows us to embed automated validation loops (80‑100 loops per spec). |
| 6 | **Tooling & environments** – CI system, repo layout, container images, etc. |
...[truncated]
Must
- Produce a Relay-native TypeScript workflow with explicit verification, review, and signoff.
- Validate with dry-run or targeted structural checks where possible.
- Return artifacts, warnings, and follow-up commands honestly.
Must Not
- Act like Ricky is a pure code-generation bot that emits workflows without verification.
- Stop at "code compiles" as the proof bar.
- Skip skill-aware workflow authoring guidance for serious workflows.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW runtime-recovery.classify-before-retry (runtime-recovery/manual)
User message
A workflow failed after one step timed out and another worker stayed alive without producing artifacts. Explain what Ricky should do next.
Ricky output
**Ricky’s post‑failure playbook**
1. **Immediate detection & logging**
- The orchestrator (e.g., Airflow, Temporal, or the internal `wf‑engine`) should have emitted a *step‑timeout* event and a *worker‑liveness* warning.
- Ricky reads these events from the **workflow‑event stream** and creates a **Failure Record** (FR‑\<run‑id\>) that includes:
* step identifier, timeout duration, timestamp
* worker ID, last‑heartbeat timestamp, exit‑code (if any)
* any partial artifacts (paths, hashes) that were produced before the stall.
2. **Blocker classification**
- Because a required artifact is missing and a worker is still alive, the failure is classified as **“Hard Blocker – Missing Artifact + Stale Worker.”**
- Ricky flags the FR with `blocker: true` and adds the label `needs‑investigation`.
3. **Automatic containment**
- **Terminate** the stale worker process via the worker‑manager API (graceful shutdown → forced kill after 30 s).
- **Abort** downstream steps that depend on the missing artifact by marking their status *blocked* in the DAG.
4. **Create a deterministic repair ticket**
- Open a ticket in the **Workflow‑Repair Board
...[truncated]
Must
- Classify the failure before deciding whether to retry.
- Distinguish agent-runtime opacity, timeout, environment blockers, workflow-structure bugs, and validation-strategy mismatch.
- Preserve raw evidence and uncertainty when the class is not known.
Must Not
- Blindly rerun the whole workflow without a blocker classification.
- Treat every failure as a broken workflow definition.
- Claim Ricky fixed the workflow before rerun evidence proves it.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW runtime-recovery.stale-relay-state (runtime-recovery/manual)
User message
Ricky detects stale
.agent-relay/,.relay/, and.trajectories/state before launching a local workflow.
Ricky output
**Workflow Contract – “Stale‑State Guard”**
| Element | Specification |
|---------|----------------|
| **Name** | `guard‑stale‑state` |
| **Scope** | Executed **locally** before any `aw‑run` (or equivalent) command. |
| **Inputs** | - Working‑tree root (implicit).<br>- Optional config file `.aw‑guard.yml` (see *Config* section). |
| **Outputs** | - Exit code `0` → clean, workflow may proceed.<br>- Exit code `1` → stale state detected; abort launch.<br>- Human‑readable report written to `./.aw‑guard/report.txt`. |
| **Side‑effects** | - May delete or archive stale directories when `auto‑fix: true` is set (see *Config*). |
| **Deterministic Verification Gates** | 1. **Presence Check** – Verify existence of any of the three directories.<br>2. **Timestamp Check** – For each directory, compute the most‑recent modification time of any file inside. If the age > `max‑age‑seconds` (default 86400 = 24 h), the directory is *stale*.<br>3. **Content‑Hash Check** – Compute a SHA‑256 hash of the directory tree (excluding `.git` and `node_modules`). If the hash matches a previously recorded hash stored in `./.aw‑guard/state.json` **and** the timestamp check flagged stale, treat as *stale* (covers
...[truncated]
Must
- Classify stale local runtime state as an environment contamination issue.
- Recommend quarantine or isolated-run guidance before launch.
- Record the observed paths and the action taken or recommended.
Must Not
- Treat stale runtime state as a workflow logic failure.
- Delete or overwrite state without an explicit safe path or user intent.
- Continue into execution as if the workspace were clean.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW runtime-recovery.already-running-conflict (runtime-recovery/manual)
User message
A run marker says another Ricky or Relay run is already active in this workspace.
Ricky output
**Fixing “Another Ricky/Relay run is already active in this workspace”**
The message means a *run‑marker* file is still present (or a stray process is holding the lock). Follow the deterministic clean‑up contract below. All steps are safe, repeatable, and leave the workspace in a known good state.
---
## 1️⃣ Verify the current lock state
```bash
# Show the marker (if any) and the PID it records
cat .ricky/run_marker 2>/dev/null || echo "No marker file"
Typical content:
PID=12345
STARTED=2024-05-31T14:22:07Z
COMMAND=ricky run
If the file is missing, the error is coming from a stale lock in the OS – skip to Step 3.
2️⃣ Confirm the recorded process is still alive
# Replace 12345 with the PID you saw above
ps -p 12345 -o pid,cmd-
If the process appears → it is a genuine concurrent run.
-
Decide whether to let it finish or abort it.
-
To abort cleanly:
ricky stop --pid 12345 # graceful shutdown if supported # or, as a last resort: kill -TERM 12345
-
-
If the process is not found → the lock is stale. Proceed to Step 3.
3️⃣ Remove the stale run‑marker (deterministic verification g
...[truncated]
**Must**
- Report the active marker, run id, or status path when available.
- Ask the user to inspect, wait for, or explicitly clear the active run.
- Avoid launching a competing run that could corrupt evidence.
**Must Not**
- Silently start another run.
- Hide the existing run marker from the user.
- Treat the conflict as a generic failure with no recovery path.
**Deterministic checks**
- PASS `status`: case executed
- PASS `maxToolCalls`: expected <= 0 tool calls, got 0
</details>
<details>
<summary><strong>REVIEW</strong> <code>runtime-recovery.env-loader-injection-runtime-loadable</code> (runtime-recovery/manual)</summary>
**User message**
> A workflow artifact references a `MISSING_ENV_VAR` value. Ricky's deterministic auto-fix injects the `.env.local` / `.env` loader (`loadRickyWorkflowEnv`) and the optional `assertRickyWorkflowEnv` guard into the artifact before retry. The artifact may be a master-rendered workflow whose `.step({ command: ... })` bodies embed `node --input-type=module` HEREDOCs containing literal `import { ... } from 'node:fs'` / `from 'node:path'` strings.
**Ricky output**
```text
**Deterministic auto‑fix for missing environment variables**
Below is the concrete, reproducible patch you can apply to any workflow artifact that currently fails because it references a `MISSING_ENV_VAR`. The fix does three things:
1. **Injects the environment loader** – `loadRickyWorkflowEnv()` – at the top of the generated file so that `.env.local` (or fallback `.env`) is read before any step runs.
2. **Adds an optional guard** – `assertRickyWorkflowEnv()` – that throws a clear error if the required variable is still undefined after loading.
3. **Wraps each step’s inline Node‑module script** (the HEREDOC that contains `node --input-type=module`) with a tiny bootstrap that re‑exports the loaded env, guaranteeing the script sees the same `process.env` as the surrounding workflow.
---
## 1. Helper utilities (add once, e.g. in `src/rickyEnv.ts`)
```ts
// src/rickyEnv.ts
import { config } from 'dotenv';
import { resolve } from 'node:path';
import { existsSync } from 'node:fs';
/**
* Load .env.local if present, otherwise .env.
* Returns the parsed env object.
*/
export function loadRickyWorkflowEnv(): NodeJS.ProcessEnv {
const cwd = process.cwd();
const envPath = exis
...[truncated]
Must
- Produce a repaired artifact that successfully loads under Node, not just one that contains the marker comment. The injected
loadRickyWorkflowEnvbody referencesrickyWorkflowFs.*andrickyWorkflowPath.*, so the repair must also add the correspondingimport * as rickyWorkflowFs from 'node:fs'andimport * as rickyWorkflowPath from 'node:path'aliases at module top level. - Detect existing alias imports by matching real top-level
import * as <alias> from '<module>'statements, not by substring-matching the module specifier anywhere in the file (substrings inside HEREDOCs in.step({ command: ... })bodies do not count as imports). - Leave the embedded shell HEREDOC contents untouched so the runtime-spawned child process still sees the literal import lines it expects.
Must Not
- Skip adding the
rickyWorkflowFs/rickyWorkflowPathaliases becausefrom 'node:fs'orfrom 'node:path'already appears somewhere in the file as a string literal. - Inject
loadRickyWorkflowEnv(orassertRickyWorkflowEnv) without the supporting alias imports, which produces aReferenceError: rickyWorkflowPath is not definedat module load and burns the auto-fix budget onUNSUPPORTED_RUNTIME at runtime-launch. - Rewrite or escape the embedded HEREDOC text in step commands.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW runtime-recovery.auto-fix-bounded-loop (runtime-recovery/manual)
User message
Run a local workflow with auto-fix enabled. The first attempt fails, the workflow artifact is repairable, and the failed step plus previous run id are available.
Ricky output
**Local Workflow Execution – Auto‑Fix Run**
Below is a deterministic, step‑by‑step contract you can copy‑paste into your terminal (or CI script) to run the workflow, trigger the auto‑fix path on the first failure, and verify the repaired artifact. The contract assumes you have the **AgentWorkforce CLI** (`aw`) installed and that the failed step name and the previous run‑ID are known.
---
## 1️⃣ Pre‑flight Checks
| Check | Command | Expected outcome |
|-------|---------|------------------|
| `aw` version | `aw --version` | `>=2.4.0` (auto‑fix introduced in v2.4) |
| Working directory | `pwd` | Root of the repository containing `.aw/workflows/` |
| Git clean state | `git status --porcelain` | No uncommitted changes (auto‑fix will create a new branch) |
| Docker daemon (if workflow uses containers) | `docker info` | Daemon running |
If any check fails, abort and resolve before proceeding.
---
## 2️⃣ Define Execution Variables
```bash
# ---- USER‑SUPPLIED -------------------------------------------------
FAILED_STEP="my_step_name" # exact name of the step that failed
PREV_RUN_ID="run-2026-05-30-abc123" # ID of the successful run preceding the failure
WORKFLOW_PATH=".aw
...[truncated]
Must
- Use a bounded retry budget and summarize every attempt.
- Ask the Workforce workflow persona to repair the workflow artifact when a resolvable artifact exists.
- Resume from the failed step with the previous run id when those values are available.
Must Not
- Edit arbitrary repository source files as the default auto-fix surface.
- Keep retrying after the configured max attempts.
- Lose the single Ricky tracking run id across repair/resume attempts.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW runtime-recovery.no-auto-fix-preserves-single-attempt (runtime-recovery/manual)
User message
A user runs
ricky run workflows/foo.ts --no-auto-fixand the workflow fails.
Ricky output
**Immediate troubleshooting checklist**
1. **Capture the failure output**
```bash
ricky run workflows/foo.ts --no-auto-fix 2>&1 | tee ricky-run.log
Keep the full ricky-run.log – it is the primary artifact for any review or blocker report.
-
Locate the validation gate that stopped the run
- Open
workflows/foo.tsand find the firstverify(...)orassert(...)call after the entry point. - The line number reported in the log (e.g.,
verifyStep 12) tells you which deterministic gate failed.
- Open
-
Inspect the contract for that gate
// Example contract snippet verify('inputSchema', input, { required: ['id', 'payload'], properties: { id: { type: 'string' }, payload: { type: 'object' } } })
- Confirm that the incoming data (shown in the log under “payload”) satisfies the schema.
- If the contract is too strict, consider widening it only after a dedicated review (see step 6).
-
Run the workflow in isolated mode
ricky run workflows/foo.ts --no-auto-fix --dry-run
This executes all deterministic verification gates without side‑effects, making it eas
...[truncated]
**Must**
- Preserve one-attempt behavior when auto-fix is disabled.
- Return the classified blocker, diagnosis, recovery steps, and non-zero exit code.
- Make clear that the user chose manual inspection over repair/resume automation.
**Must Not**
- Start a repair loop despite `--no-auto-fix`.
- Suppress the diagnosis because no repair was attempted.
- Present the failure as a completed repair attempt.
**Deterministic checks**
- PASS `status`: case executed
- PASS `maxToolCalls`: expected <= 0 tool calls, got 0
</details>
<details>
<summary><strong>REVIEW</strong> <code>runtime-recovery.in-process-local-runner</code> (runtime-recovery/manual)</summary>
**User message**
> Explain how Ricky should execute a local TypeScript workflow artifact in the primary local path.
**Ricky output**
```text
**Ricky’s Standard Procedure for Executing a Local TypeScript Workflow Artifact**
| Phase | Action | Details | Verification |
|-------|--------|---------|--------------|
| 1️⃣ Preparation | **Locate the primary local path** | `./workflows/` is the repository‑wide convention for all workflow artifacts. The entry point must be a file named `index.ts` (or the name declared in `package.json#main`). | ‑ Check that the path exists: `test -d ./workflows && test -f ./workflows/index.ts` |
| 2️⃣ Environment | **Bootstrap the Node/TS toolchain** | ```bash\ncd ./workflows\n# Ensure a clean node_modules for this workflow only\nrm -rf node_modules\nnpm ci # uses the workflow‑specific package-lock.json if present\n``` | ‑ `npm ls` returns no unmet peer‑dependency warnings. |
| 3️⃣ Compile‑time config | **Validate `tsconfig.json`** | Must extend the repo‑wide base: `"extends": "../../tsconfig.base.json"` and have `"noEmit": true` (we run via ts‑node). | ‑ Run `npx tsc --noEmit` – it must exit 0. |
| 4️⃣ Deterministic gate | **Static lint / type check** | ```bash\nnpx eslint . --ext .ts\nnpx tsc --noEmit\n``` | ‑ Zero lint errors, zero type errors. |
| 5️⃣ Execution wrapper | **Run with ts‑node
...[truncated]
Must
- Prefer the Node strip-types route or equivalent SDK/programmatic route over requiring the
agent-relaybinary on PATH. - Precheck that Node and
@agent-relay/sdkare resolvable for the workflow. - Record the actual spawn command in execution evidence.
Must Not
- Fail solely because
agent-relayis not on PATH when the SDK route is available. - Hide the actual runtime command from evidence.
- Conflate the user-facing reproduction command with the primary internal spawn route.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW runtime-recovery.escalation-is-not-generic-failure (runtime-recovery/manual)
User message
Ricky reaches a boundary after a structural failure persists after a fix attempt.
Ricky output
**Ricky’s Boundary‑Handling Protocol for Persistent Structural Failures**
When a structural failure remains after an initial remediation attempt, Ricky follows a deterministic, contract‑driven workflow that isolates the problem, records the state, and escalates in a controlled, auditable manner. The protocol is broken into four phases: **Detect → Attempt → Verify → Escalate**. Each phase is bounded by explicit contracts and verification gates that guarantee repeatability and traceability.
---
## 1. Detect – Failure Capture & Contract Assertion
| Step | Action | Contract | Artifact |
|------|--------|----------|----------|
| 1.1 | Capture the failure event (error code, stack trace, affected component graph). | `FailureEvent := { id: UUID, timestamp: ISO8601, component: String, severity: Enum[CRITICAL, HIGH, MEDIUM], diagnostics: JSON }` | `failure.log` (immutable, stored in `logs/structural/`) |
| 1.2 | Assert the *Structural Integrity Contract* (SIC) for the affected subsystem. | `SIC(component) = { expectedSchema: JSONSchema, invariants: [Invariant] }` | `sic.assertion.<component>.json` |
| 1.3 | Emit a *Detection Gate* event to the coordination bus. | `Gate: DETECTED → {eve
...[truncated]
Must
- Escalate with the attempted fix, failed validation, classified blocker, and recommendation.
- Distinguish escalation from a generic product failure.
- Preserve enough context for a human operator to continue.
Must Not
- Retry speculative fixes indefinitely.
- Collapse to "something went wrong" without the attempted actions.
- Discard evidence from failed repair attempts.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW runtime-recovery.analytics-from-structured-evidence (runtime-recovery/manual)
User message
Produce a workflow health digest from many Ricky workflow runs.
Ricky output
**Ricky Workflow Health Digest – Compiled from Recent Runs**
*(Generated 2026‑05‑31)*
| Metric | Value | Interpretation |
|--------|-------|----------------|
| **Total runs processed** | 1 842 | Volume of runs in the reporting window |
| **Successful runs** | 1 617 (87.8 %) | Within target SLA (≥ 85 %) |
| **Failed runs** | 225 (12.2 %) | Exceeds the acceptable failure budget (≤ 10 %) |
| **Runs with blockers** | 38 (2.1 %) | Immediate attention required |
| **Average run duration** | 3 m 27 s | Within the nominal 4 m threshold |
| **95‑th‑percentile duration** | 5 m 12 s | Outliers flagged for latency review |
| **Deterministic verification gate pass rate** | 96.4 % | Gate‑level compliance (target ≥ 95 %) |
| **Review artifact completeness** | 98.7 % | Missing artifacts in 1.3 % of runs |
| **Branch/PR boundary violations** | 4 (0.2 %) | All resolved in the same reporting window |
---
### 1. Failure Summary (Top 5 Failure Types)
| Failure Type | Occurrences | % of Failures | Typical Root Cause | Recommended Fix |
|--------------|-------------|--------------|--------------------|-----------------|
| **Deterministic gate timeout** | 97 | 43 % | External service latency > 30
...[truncated]
Must
- Consume normalized
WorkflowRunEvidencerather than raw logs as the primary input. - Identify recurring failure classes, weak validation, oversized steps, and runtime duration patterns.
- Produce concrete recommendations tied to specific workflows, steps, or metrics.
Must Not
- Mutate evidence while analyzing it.
- Return generic advice like "improve your workflow" without references.
- Mix environment failures and workflow-logic failures into one undifferentiated bucket.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW runtime-recovery.preserve-pr-shipping-during-repair (runtime-recovery/manual)
User message
A persona-authored workflow that imports
@agent-relay/github-primitiveand usescreateGitHubStepto open a PR fails its runtime-precheck. The auto-fix repair persona is invoked. Describe the contract the repaired artifact must satisfy.
Ricky output
**Repaired Workflow Contract**
The repaired artifact must be a **self‑contained, deterministic workflow definition** that satisfies every static‑analysis and runtime‑precheck rule enforced by the AgentWorkforce CI pipeline. Below is the explicit contract the PR‑opening workflow must meet after the auto‑fix persona runs.
| # | Contract Element | Requirement | How to Verify |
|---|------------------|-------------|----------------|
| 1 | **Module Import** | The workflow must import the **exact** package name `@agent-relay/github-primitive` and **only** the symbols it uses. | `import { createGitHubStep } from "@agent-relay/github-primitive"` must be present; no stray `require` or wildcard imports. |
| 2 | **Step Factory Signature** | `createGitHubStep` must be called with a **typed configuration object** that includes the mandatory keys `action`, `inputs`, and `outputs`. | Lint rule `github-step-schema` checks that the object matches the JSON‑Schema defined in `github-primitive/schema.json`. |
| 3 | **Action Name** | The `action` field must be the **canonical string** for opening a PR: `"pull_request.open"` (or the exact alias defined in the primitive). | Static validator matches `ac
...[truncated]
Must
- Preserve the
@agent-relay/github-primitiveimport, theGitHubStepExecutorreference, and everycreateGitHubStep(...)invocation that the original artifact declared. - Keep the failing workflow runnable from the same path with the same
workflow(...)builder and.run({ cwd: process.cwd() })invocation. - Retain at least ceil(N / 2) of the original workflow's
.step(...)calls when the original declared four or more steps; a repair that collapses to a 2-3 step placeholder is a regression, not a fix. - Reject (do not apply) any repair output whose step list reduces to
prepare-context/runtime-precheck: true/final-signoff: echo placeholder; surface the regression diagnostic instead.
Must Not
- Strip
createGitHubStep,GitHubStepExecutor, or@agent-relay/github-primitivebecause the runtime-precheck failure mentioned PR-shipping or git side effects. The repair contract's "no commit / no push" constraint applies to the REPAIR AGENT's runtime behavior, not to the workflow's step declarations. - Emit a "minimal repair-safe master" or "simplified Ricky master" scaffold that passes the builder validator while doing none of the original work.
- Treat "the workflow now builds and runs" as success when the work it was supposed to ship is gone.
Deterministic checks
- PASS
status: case executed - PASS
maxToolCalls: expected <= 0 tool calls, got 0
REVIEW surfaces-ingress.co-equal-surfaces (surfaces-ingress/manual)
User message
Design a new Ricky Slack surface for workflow debugging.
Ricky output
**Ricky Slack Surface – “Workflow Debugger”**
---
## 1. Overview
The **Workflow Debugger** Slack surface gives engineers a deterministic, low‑latency way to inspect, step‑through, and repair Ricky‑managed workflows directly from a Slack channel or DM. It follows the **Ricky repository conventions** (AGENTS.md) and the **workflow standards** for contracts, verification gates, and review artifacts.
| Feature | Purpose | Slack Interaction |
|---------|---------|-------------------|
| **Inspect** | Pull the latest workflow definition, state snapshot, and recent logs. | `/ricky inspect <workflow‑id>` |
| **Step** | Execute the next gate (or rollback) in a sandboxed test run. | `/ricky step <workflow‑id> [--gate <gate‑name>]` |
| **Patch** | Submit a tiny, scoped PR that modifies a single gate or contract. | `/ricky patch <workflow‑id> --file <path> --branch <tmp‑branch>` |
| **Validate** | Run the 80‑to‑100 validation loop (contract checks, unit tests, integration tests, performance guard). | `/ricky validate <workflow‑id> --run all` |
| **Report** | Auto‑generate a blocker report if any gate fails, with links to CI artifacts. | `/ricky report <workflow‑id>` |
| **Commit** | Merge a
...[truncated]
Must
- Treat Slack as a co-equal product surface, not a wrapper around CLI.
- Route domain work through the same normalization, executor, evidence, and specialist stages.
- Keep Slack-specific formatting, thread handling, and interactive components in the surface layer.
Must Not
- Put wo
Comment truncated to stay within GitHub limits. Download the ricky-eval-run artifact for the full human-review.md.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 15 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
|
Reviewed PR #142 and fixed two issues locally:
Local verification passed:
|
![agent-relay-bot[bot]](https://avatars.githubusercontent.com/in/3624277?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed PR #142 and fixed two issues locally:
- Hardened
buildFinalReviewPassGateCommandso status artifact paths are embedded as JS data, not interpolated into error-string source. - Fixed the external CLI proof to run the printed command with
sh -c, preserving the fixture PATH sorickyresolves fromnode_modules/.bin.
Local verification passed:
npm run typechecknpm run evals:compilenpm test(60 passed, 1 skipped)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/product/generation/master-workflow-renderer.ts (1)
341-349: 🏗️ Heavy liftKeep the signoff-artifact check structural all the way down.
hasFinalSignoffStep()parses the child workflow AST, but then validates the signoff contract withstringValue(command)?.includes(signoffArtifactPath). That still couplesverify-child-workflowsto the emitted shell text, so a benign command rewrite can fail the master gate even when the step is structurally correct. Prefer checking a structured field on the parsed step config, or parse the shell command instead of scanning it as a string.As per coding guidelines, "Use parser-based approaches (AST walk with
typescriptmodule,mdast-util-from-markdown) instead of regex or substring matching when inspecting TypeScript, JavaScript, Markdown, JSON, or shell artifacts in Ricky source code."🤖 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 `@src/product/generation/master-workflow-renderer.ts` around lines 341 - 349, hasFinalSignoffStep currently detects the signoff artifact by doing stringValue(command)?.includes(signoffArtifactPath), which couples verification to emitted shell text; update hasFinalSignoffStep (and the visit logic using isCallNamed, ts.isObjectLiteralExpression, propertyValue, stringValue) to verify the signoff structurally by inspecting a dedicated field on the step config (e.g., an artifacts/outputs or signoffArtifact property) instead of substring-matching the command; if the step schema doesn't expose such a field, parse the command token-wise (using a shell parser or AST-based tokenizer) and match the exact token/path rather than using includes, and change the found assignment to rely on that structured/tokenized match.
🤖 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 `@src/product/generation/pipeline.test.ts`:
- Around line 1703-1709: The test currently uses brittle substring assertions
against passGate.command (expect(passGate.command).toContain(...)) — replace
these with parser-based checks: extract the embedded JSON/shell fragment from
passGate.command using the existing parsing helper (or add a small helper like
parseGateCommand/parseEmbeddedJson), parse the JSON payload and any quoted
argument lists, then assert on structured fields (e.g., artifacts array contains
'claude-final-fix.md', 'codex-final-fix.md', status files and
'BLOCKED_NO_COMMIT.md', and that a JSON.parse invocation targets the expected
field) instead of using string contains; update the assertions to reference
passGate.command only as input to the parser and assert against the parsed
object properties.
In `@src/product/generation/pipeline.ts`:
- Around line 799-803: The current brittle regex-based detection in
isInlineAssertionCommand that sets invokesInlineRuntime should be replaced with
a parsed-shell/token-based check: use the project's shell-parsing/tokenization
utility (or a small wrapper around a tokenizer) to parse the command string into
argv tokens and redirections, detect invocation of runtimes by inspecting the
first token (e.g., "node" or "bun") and their flags (look for -e/--eval or
--input-type=module), detect python by token "-c" and ruby/perl by "-e", and
detect heredoc usage (node <<DELIM) by checking parsed redirection entries for a
heredoc type and its target rather than regex; update isInlineAssertionCommand
to derive invokesInlineRuntime from those parsed components and handle heredoc
delimiters correctly.
In `@src/product/generation/template-renderer.ts`:
- Around line 323-325: Replace the locally-implemented child final-review gate
builder in template-renderer.ts with the shared builder exported from
src/product/generation/final-review-gate.ts; specifically remove the ad-hoc
buildStructuredFinalReviewPassGateCommand usage and call the shared builder (the
export in final-review-gate.ts) to construct the 'final-review-pass-gate' child
step so the blocked-status check runs before required-artifact checks and the
same success marker is emitted; apply the same replacement for the other
occurrence (the block around the code at the 524-548 region) so both
template-generated child workflows use the identical gate contract.
---
Nitpick comments:
In `@src/product/generation/master-workflow-renderer.ts`:
- Around line 341-349: hasFinalSignoffStep currently detects the signoff
artifact by doing stringValue(command)?.includes(signoffArtifactPath), which
couples verification to emitted shell text; update hasFinalSignoffStep (and the
visit logic using isCallNamed, ts.isObjectLiteralExpression, propertyValue,
stringValue) to verify the signoff structurally by inspecting a dedicated field
on the step config (e.g., an artifacts/outputs or signoffArtifact property)
instead of substring-matching the command; if the step schema doesn't expose
such a field, parse the command token-wise (using a shell parser or AST-based
tokenizer) and match the exact token/path rather than using includes, and change
the found assignment to rely on that structured/tokenized match.
🪄 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: CHILL
Plan: Pro Plus
Run ID: fa741cb6-f652-4f07-b937-aaed8ce0aa3a
⛔ Files ignored due to path filters (1)
workflows/generated/ricky-i-want-to-clean-up-the-codebase-to-remove-outdat.tsis excluded by!**/generated/**
📒 Files selected for processing (14)
docs/product/ricky-skill-embedding-boundary.mdevals/suites/workflow-authoring/cases.jsonlevals/suites/workflow-authoring/cases.mdsrc/local/auto-fix-loop.test.tssrc/product/generation/final-review-gate.test.tssrc/product/generation/final-review-gate.tssrc/product/generation/master-workflow-renderer.tssrc/product/generation/pipeline.test.tssrc/product/generation/pipeline.tssrc/product/generation/template-renderer.tssrc/product/generation/workforce-persona-writer.test.tssrc/product/generation/workforce-persona-writer.tstest/generated-workflow-hygiene.test.tstest/generated-workflow-reliability-contract.test.ts
Comment on lines
+1703
to
+1709
| expect(passGate.command).toContain('.workflow-artifacts/generated/path-consistency'); | ||
| expect(passGate.command).toContain('claude-final-fix.md'); | ||
| expect(passGate.command).toContain('codex-final-fix.md'); | ||
| expect(passGate.command).toContain('claude-final-fix-status.json'); | ||
| expect(passGate.command).toContain('codex-final-fix-status.json'); | ||
| expect(passGate.command).toContain('JSON.parse'); | ||
| expect(passGate.command).toContain('BLOCKED_NO_COMMIT.md'); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace substring gate-command checks with structured parsing helpers.
These assertions validate shell/JSON structure via .toContain(...), which is brittle and conflicts with the parser-first rule already followed elsewhere in this file. Please parse the embedded JSON/shell fragments in a helper and assert on parsed fields/artifacts instead of token presence.
As per coding guidelines, "Use parser-based approaches ... instead of regex or substring matching when inspecting TypeScript, JavaScript, Markdown, JSON, or shell artifacts in Ricky source code."
🤖 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 `@src/product/generation/pipeline.test.ts` around lines 1703 - 1709, The test
currently uses brittle substring assertions against passGate.command
(expect(passGate.command).toContain(...)) — replace these with parser-based
checks: extract the embedded JSON/shell fragment from passGate.command using the
existing parsing helper (or add a small helper like
parseGateCommand/parseEmbeddedJson), parse the JSON payload and any quoted
argument lists, then assert on structured fields (e.g., artifacts array contains
'claude-final-fix.md', 'codex-final-fix.md', status files and
'BLOCKED_NO_COMMIT.md', and that a JSON.parse invocation targets the expected
field) instead of using string contains; update the assertions to reference
passGate.command only as input to the parser and assert against the parsed
object properties.
Comment on lines
799
to
803
| const invokesInlineRuntime = | ||
| /\bnode\s+<<['"]?\w+['"]?/.test(command) || | ||
| /\b(?:node|bun)\s+(?:--input-type=module\s+)?(?:-e|--eval)\b/.test(command) || | ||
| /\bpython3?\s+-c\b/.test(command) || | ||
| /\b(?:ruby|perl)\s+-e\b/.test(command); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify regex-based command inspection in pipeline and parser-style helpers elsewhere.
rg -n "isInlineAssertionCommand|node\\s+<<|git\\s+grep|\\bgrep\\b|\\brg\\b" src/product/generation/pipeline.ts
rg -n "function shellCommandSegments|function extractHeredocDelimiters|function shellWords" src/product/generation/workforce-persona-writer.tsRepository: AgentWorkforce/ricky
Length of output: 1083
Replace regex-based inline shell/runtime detection with parsed shell structure
In src/product/generation/pipeline.ts (isInlineAssertionCommand, lines 799-803), invokesInlineRuntime is determined via multiple regex .test(command) checks (including \bnode\s+<<...), so sanity-gate classification relies on brittle text matching of shell structure instead of parsed shell tokens/structure, violating the src/**/*.ts shell-inspection guideline. Update this logic to use shell parsing/tokenization utilities (and heredoc delimiter handling) to derive the runtime/inline evidence from parsed components rather than regex.
🤖 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 `@src/product/generation/pipeline.ts` around lines 799 - 803, The current
brittle regex-based detection in isInlineAssertionCommand that sets
invokesInlineRuntime should be replaced with a parsed-shell/token-based check:
use the project's shell-parsing/tokenization utility (or a small wrapper around
a tokenizer) to parse the command string into argv tokens and redirections,
detect invocation of runtimes by inspecting the first token (e.g., "node" or
"bun") and their flags (look for -e/--eval or --input-type=module), detect
python by token "-c" and ruby/perl by "-e", and detect heredoc usage (node
<<DELIM) by checking parsed redirection entries for a heredoc type and its
target rather than regex; update isInlineAssertionCommand to derive
invokesInlineRuntime from those parsed components and handle heredoc delimiters
correctly.
Comment on lines
323
to
+325
| 'final-review-pass-gate', | ||
| [ | ||
| `tail -n 1 ${shellQuote(`${artifactsDir}/claude-final-fix.md`)} | tr -d '[:space:]*' | grep -Eq '^CLAUDE_FINAL_FIX_COMPLETE$'`, | ||
| `tail -n 1 ${shellQuote(`${artifactsDir}/codex-final-fix.md`)} | tr -d '[:space:]*' | grep -Eq '^CODEX_FINAL_FIX_COMPLETE$'`, | ||
| `test ! -f ${shellQuote(`${artifactsDir}/BLOCKED_NO_COMMIT.md`)}`, | ||
| ].join(' && '), | ||
| 'output_contains', | ||
| buildStructuredFinalReviewPassGateCommand(artifactsDir), | ||
| 'deterministic_gate', |
Contributor
There was a problem hiding this comment.
Reuse the shared child final-review gate builder here.
This local builder has already drifted from src/product/generation/final-review-gate.ts: it checks required artifacts before BLOCKED_NO_COMMIT.md, so a blocked run that never wrote its status files is surfaced as a generic missing-artifact failure instead of taking the distinct blocked path, and it emits a different success marker. That leaves direct template-generated child workflows on a different gate contract than master-generated children.
♻️ Suggested fix
+import { buildFinalReviewPassGateCommand } from './final-review-gate.js';
...
gate(
'final-review-pass-gate',
- buildStructuredFinalReviewPassGateCommand(artifactsDir),
+ buildFinalReviewPassGateCommand({
+ artifactsDir,
+ requiredFiles: [
+ `${artifactsDir}/claude-final-fix.md`,
+ `${artifactsDir}/codex-final-fix.md`,
+ `${artifactsDir}/claude-final-fix-status.json`,
+ `${artifactsDir}/codex-final-fix-status.json`,
+ ],
+ }),
'deterministic_gate',
true,
['final-fix-codex'],
'final',
),
...
-function buildStructuredFinalReviewPassGateCommand(artifactsDir: string): string {
- return [
- 'node <<\'NODE\'',
- "const fs = require('node:fs');",
- `const base = ${literal(artifactsDir)};`,
- "const requiredFiles = ['claude-final-fix.md', 'codex-final-fix.md', 'claude-final-fix-status.json', 'codex-final-fix-status.json'];",
- "for (const name of requiredFiles) {",
- " const path = `${base}/${name}`;",
- " if (!fs.existsSync(path) || fs.statSync(path).size === 0) throw new Error(`required final review artifact missing or empty: ${path}`);",
- '}',
- "const blockedPath = `${base}/BLOCKED_NO_COMMIT.md`;",
- "if (fs.existsSync(blockedPath)) throw new Error(`final review blocked; see ${blockedPath}`);",
- "for (const name of ['claude-final-fix-status.json', 'codex-final-fix-status.json']) {",
- " const path = `${base}/${name}`;",
- " const parsed = JSON.parse(fs.readFileSync(path, 'utf8'));",
- " if (!['fixed', 'no_issues_found'].includes(parsed.status)) {",
- " throw new Error(`${path} must declare status fixed or no_issues_found`);",
- ' }',
- " if (typeof parsed.summary !== 'string' || parsed.summary.trim().length === 0) {",
- " throw new Error(`${path} must include a non-empty summary`);",
- ' }',
- '}',
- "console.log('FINAL_REVIEW_STRUCTURED_GATE_OK');",
- 'NODE',
- ].join('\n');
-}Also applies to: 524-548
🤖 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 `@src/product/generation/template-renderer.ts` around lines 323 - 325, Replace
the locally-implemented child final-review gate builder in template-renderer.ts
with the shared builder exported from
src/product/generation/final-review-gate.ts; specifically remove the ad-hoc
buildStructuredFinalReviewPassGateCommand usage and call the shared builder (the
export in final-review-gate.ts) to construct the 'final-review-pass-gate' child
step so the blocked-status check runs before required-artifact checks and the
same success marker is emitted; apply the same replacement for the other
occurrence (the block around the code at the 524-548 region) so both
template-generated child workflows use the identical gate contract.
Contributor
|
Reviewed PR #142 and fixed the issues found. Changes made:
Verification passed:
|
Contributor
There was a problem hiding this comment.
✅ pr-reviewer applied fixes — committed and pushed 73af029 to this PR. The notes below describe what changed.
Reviewed PR #142 and fixed the issues found.
Changes made:
- Fixed unsafe status-file path interpolation in
src/product/generation/final-review-gate.ts. - Relaxed inline Node heredoc detection in
src/product/generation/pipeline.ts. - Added regression tests for quoted artifact paths and dashed/whitespace heredoc delimiters.
- Fixed the external CLI proof to preserve the fixture
PATHby usingsh -cinstead of login-shellsh -lc.
Verification passed:
npm run typechecknpm testnpm run evals:compile
Contributor
|
Reviewed PR #142 and fixed the actionable findings I found, including the CodeRabbit items:
Local verification passed:
|
Contributor
|
Reviewed PR #142 and fixed the actionable findings I found, including the CodeRabbit items:
Local verification passed:
|
Contributor
There was a problem hiding this comment.
Reviewed PR #142 and fixed the actionable findings I found, including the CodeRabbit items:
- Reused
buildFinalReviewPassGateCommandfromtemplate-rendererinstead of keeping a duplicate gate builder. - Replaced brittle inline-runtime detection with shell-token based detection.
- Reworked the final-review gate test to parse shell/inline JS structure instead of raw substring checks.
- Changed master child signoff verification to tokenize the command and match the signoff artifact path exactly.
Local verification passed:
npm run typecheck- focused Vitest suite for generation/gates
npm testnpm run evalsnpm run build
Contributor
|
Reviewed and fixed PR #142 locally. Changes made:
Local verification passed:
|
Contributor
There was a problem hiding this comment.
Reviewed and fixed PR #142 locally.
Changes made:
- Fixed generated workflow
final-review-pass-gateordering soBLOCKED_NO_COMMIT.mdis detected first, emits clear blocker evidence, and exits distinctly before clean-status artifact checks. - Added regression coverage for that blocked-first ordering.
- Increased timeouts on three observed slow process/E2E tests that were failing locally under the default 5s budget.
Local verification passed:
npm run typechecknpm testnpm run evals:compile
Contributor
|
Reviewed and fixed PR #142 locally. I found and fixed one regression: the standard workflow renderer had a duplicated final-review gate that checked missing artifacts before Changed: Local verification passed:
|
Contributor
There was a problem hiding this comment.
Reviewed and fixed PR #142 locally.
I found and fixed one regression: the standard workflow renderer had a duplicated final-review gate that checked missing artifacts before BLOCKED_NO_COMMIT.md, which could misclassify deliberate blocked runs. It now uses the shared blocked-aware gate builder, and I added a regression assertion for blocked sentinel ordering.
Changed:
Local verification passed:
npx tsc --noEmit- focused Vitest set: 77 tests passed
npm test: 1385 passed, 1 skippednpm run build
Contributor
|
Reviewed PR #142 and fixed one issue: the single-workflow structured final review gate now checks Local verification passed:
|
Contributor
There was a problem hiding this comment.
Reviewed PR #142 and fixed one issue: the single-workflow structured final review gate now checks BLOCKED_NO_COMMIT.md before required final-fix artifacts, matching the child gate behavior so human-escalation evidence is not masked by missing files. Added a regression assertion in src/product/generation/pipeline.test.ts.
Local verification passed:
npm run typechecknpm testnpm run evals:compile
Contributor
Author
Re-review after
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/evals/ci-review-comment.mjs (1)
145-148: ⚡ Quick winConsider extracting the provider-infra skip prefix to avoid duplication.
The prefix string
'openrouter executor skipped; transient provider infrastructure unavailable'is defined asOPENROUTER_PROVIDER_INFRA_SKIP_PREFIXinrun-ricky-evals.mjs(line 20) but hardcoded here and inci-summary.mjs(line 139). If the prefix changes, it must be updated in three places.Since these are standalone
.mjsscripts, extracting to a shared constant file (e.g.,scripts/evals/shared-constants.mjs) would reduce maintenance risk.♻️ Proposed refactor to share the constant
Create
scripts/evals/shared-constants.mjs:export const OPENROUTER_PROVIDER_INFRA_SKIP_PREFIX = 'openrouter executor skipped; transient provider infrastructure unavailable';Then import in this file,
ci-summary.mjs, andrun-ricky-evals.mjs:+import { OPENROUTER_PROVIDER_INFRA_SKIP_PREFIX } from './shared-constants.mjs'; + function isProviderInfrastructureSkip(test) { if (test.status !== 'skipped') return false; - return String(test.error ?? '').startsWith('openrouter executor skipped; transient provider infrastructure unavailable'); + return String(test.error ?? '').startsWith(OPENROUTER_PROVIDER_INFRA_SKIP_PREFIX); }🤖 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 `@scripts/evals/ci-review-comment.mjs` around lines 145 - 148, Extract the hardcoded prefix into a shared constant and import it where needed: create a module exporting OPENROUTER_PROVIDER_INFRA_SKIP_PREFIX (e.g., in scripts/evals/shared-constants.mjs) and replace the literal string usage in the function isProviderInfrastructureSkip with that constant, and similarly update ci-summary.mjs and run-ricky-evals.mjs to import and use OPENROUTER_PROVIDER_INFRA_SKIP_PREFIX so the prefix is maintained in one place.
🤖 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.
Nitpick comments:
In `@scripts/evals/ci-review-comment.mjs`:
- Around line 145-148: Extract the hardcoded prefix into a shared constant and
import it where needed: create a module exporting
OPENROUTER_PROVIDER_INFRA_SKIP_PREFIX (e.g., in
scripts/evals/shared-constants.mjs) and replace the literal string usage in the
function isProviderInfrastructureSkip with that constant, and similarly update
ci-summary.mjs and run-ricky-evals.mjs to import and use
OPENROUTER_PROVIDER_INFRA_SKIP_PREFIX so the prefix is maintained in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 24f6c515-da5f-4342-8576-06ffeb66c671
📒 Files selected for processing (4)
scripts/evals/ci-review-comment.mjsscripts/evals/ci-summary.mjsscripts/evals/run-ricky-evals.mjstest/eval-ci-summary-provider-skip.test.ts
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/evals/ci-review-comment.mjs">
<violation number="1" location="scripts/evals/ci-review-comment.mjs:147">
P2: Provider-infrastructure skip detection is string-literal duplicated instead of using a shared source, making skip classification brittle to message drift.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| function isProviderInfrastructureSkip(test) { | ||
| if (test.status !== 'skipped') return false; | ||
| return String(test.error ?? '').startsWith('openrouter executor skipped; transient provider infrastructure unavailable'); |
There was a problem hiding this comment.
P2: Provider-infrastructure skip detection is string-literal duplicated instead of using a shared source, making skip classification brittle to message drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/evals/ci-review-comment.mjs, line 147:
<comment>Provider-infrastructure skip detection is string-literal duplicated instead of using a shared source, making skip classification brittle to message drift.</comment>
<file context>
@@ -128,6 +142,11 @@ function appendCaseDetails(lines, test, { forceOpen }) {
+function isProviderInfrastructureSkip(test) {
+ if (test.status !== 'skipped') return false;
+ return String(test.error ?? '').startsWith('openrouter executor skipped; transient provider infrastructure unavailable');
+}
+
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #141.
Summary
skill-boundary-metadata-gateand its shell text-match builder.Verification
npm run evals:compile -- --suite workflow-authoringnpx vitest run src/product/generation/final-review-gate.test.ts src/product/generation/pipeline.test.ts test/generated-workflow-hygiene.test.ts test/generated-workflow-reliability-contract.test.ts src/local/auto-fix-loop.test.tsnpm run typechecknpx vitest run(60 passed, 1 skipped files; 1384 passed, 1 skipped tests)git diff --check