Repository navigation
ci(holmes): detect schema and wire outputs - #323
flyingrobots wants to merge 1 commit into
Conversation
Summary by CodeRabbit
PR ReviewWalkthroughA new CI workflow step "🕵️ Detect Schema" added to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related issues
Listen, you've added a shell script fragment to your workflow that tries to be clever about finding schema files. Fine. The logic looks straightforward enough—check env var, walk the repo, sort by preference. But I have questions:
The change is minimal and incremental, which is good. No needless refactoring. No breaking changes. But minimal doesn't mean bulletproof. Test this properly. Edge cases everywhere. BunBun Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚢 SHIPME[REALM] PASS — sha 678af21 |
🔍 The Case of Pull Request #323🕵️ SHA-lock HOLMES's Investigation🕵️ SHA-lock HOLMES Investigation
🔍 Executive Deduction"Watson, after careful examination of the evidence, I deduce..." Weighted Completion: ██████░░░░ 60.0% 🧮 Score BreakdownSchema Coverage (SCS)
Test Confidence (TCI)
Migration Risk (MRI)
📊 The Weight of Evidence"Observe, Watson, how not all features carry equal importance..."
🚪 Security & Performance Gates"Elementary security measures, Watson..."
📋 The Verdict
Signed and sealed,
[END OF INVESTIGATION FOR COMMIT 678af21] 🩺 Dr. WATSON's Verification🩺 Dr. Watson's Independent Verification ReportMedical Examination of Evidence
🔬 Citation Verification"Let me examine each piece of evidence independently..."
Verification Rate: 0.0% 📊 Mathematical Verification"I shall recalculate Holmes's arithmetic..." Holmes claimed SCS: 60.0% 🔍 Consistency Analysis"Checking for contradictions in Holmes's deductions..." ✅ No logical inconsistencies detected 🩺 Dr. Watson's Medical OpinionVERIFICATION: CONCERNS NOTED "While Holmes's methods are generally sound, I have noted some" Respectfully submitted,
🔮 Professor MORIARTY's Predictions🧠 Professor Moriarty's Temporal PredictionsThe Mathematics of Inevitability
🔮 Current StateSCS: ██████░░░░ 60.0% 📈 Velocity AnalysisSCS Velocity: +0.00%/day ⏰ Completion PredictionsETA: Cannot predict (insufficient velocity) "At current velocity, completion is... improbable." 📊 Historical Trajectory10-27: ██████░░░░ 60.0% "Every problem becomes elementary when reduced to mathematics" Machine-readable reports: holmes-report.json · watson-report.json · moriarty-report.json (see workflow artifacts). Filed at 221B Repository Street |
|
@coderabbitai review please |
|
From: Linus Torvalds torvalds@linux-foundation.org Fine. I'll review your schema detection plumbing. This better be clean extraction work with no hidden behavioral landmines, or we're going to have words about scope creep. Initiating review now. BunBun ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/wesley-holmes.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/wesley-holmes.yml (1)
43-50: ---The detect step outputs are never consumed; the concern is based on a false premise.
All downstream jobs (
holmes-setupactions on lines 53, 90, 133, 181) hardcodeschema: ${{ env.HOLMES_SCHEMA }}andbundle-dir: ${{ env.HOLMES_BUNDLE_DIR }}. Theschemaandbundle_diroutputs from the detect step at lines 48–49 are never referenced anywhere in the workflow—they're dead code.Since
HOLMES_SCHEMAis defined at workflow scope (line 11) astest/fixtures/examples/ecommerce.graphql(a committed test fixture), there is no silent failure risk to downstream jobs. The detect step runs, computes an unused path, and exits cleanly; nothing propagates a bogus schema.Adding validation to the detect step is harmless defensive programming and makes sense if the step is intended for future use or visibility. But the stated risk—that invalid schema will corrupt downstream execution—does not actually occur.
From: Linus Torvalds torvalds@linux-foundation.org
To: {PR_AUTHOR_NAME} <{PR_AUTHOR_EMAIL}>
Cc: code-rabbit@vger.kernel.orgThe reviewer is chasing ghosts. The detect step outputs are orphaned—never consumed by any downstream job. They might as well be /dev/null. The actual workflow uses hardcoded env vars that point to a committed test fixture. There's no pathway for invalid schema to propagate downstream.
If you want to add validation anyway as defensive belt-and-suspenders, fine—that's sensible practice. But don't mistake good hygiene for a critical bug. They're different things.
BunBun
Likely an incorrect or invalid review comment.
| - name: "🕵️ Detect Schema" | ||
| id: detect | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| if [ -n "${WESLEY_SCHEMA:-}" ] && [ -f "$WESLEY_SCHEMA" ]; then | ||
| schema="$WESLEY_SCHEMA" | ||
| else | ||
| mapfile -t files < <(git ls-files "**/*.graphql" 2>/dev/null || true) | ||
| picked="" | ||
| if [ ${#files[@]} -gt 0 ]; then | ||
| for f in "${files[@]}"; do | ||
| if [ "${f##*/}" = "schema.graphql" ]; then picked="$f"; break; fi | ||
| done | ||
| if [ -z "$picked" ]; then picked="${files[0]}"; fi | ||
| fi | ||
| schema="${picked:-$HOLMES_SCHEMA}" | ||
| fi | ||
| base_dir="$(dirname "$schema")" | ||
| bundle_dir="$base_dir/.wesley" | ||
| echo "schema=$schema" >> "$GITHUB_OUTPUT" | ||
| echo "bundle_dir=$bundle_dir" >> "$GITHUB_OUTPUT" | ||
| echo "Detected schema: $schema" | ||
| echo "Bundle dir: $bundle_dir" |
There was a problem hiding this comment.
🧩 Analysis chain
Step outputs not exposed at job level—job-level outputs: section missing.
To make outputs available to downstream jobs via needs.<job_id>.outputs, you must define job-level outputs that map to step outputs. Currently, the detect step writes to GITHUB_OUTPUT, but the wesley-generate job (lines 17–20) lacks an outputs: block to expose them.
Without job-level outputs:, downstream jobs like holmes-investigate (line 96) cannot access the detected schema/bundle_dir via needs.wesley-generate.outputs.*—they remain hardcoded to the env vars (lines 105–109). Per the PR description, this is intended as scaffolding for a follow-up, but to fulfill the stated goal of "expose bundle_dir/schema via step outputs for later wiring," you need to add:
wesley-generate:
name: "🚀 Wesley Generation"
runs-on: ubuntu-latest
+ outputs:
+ schema: ${{ steps.detect.outputs.schema }}
+ bundle_dir: ${{ steps.detect.outputs.bundle_dir }}
steps:Then downstream jobs can reference ${{ needs.wesley-generate.outputs.schema }} etc. Verify this aligns with the extraction scope: is this deferred to the follow-up PR intentionally, or should it be included now?
🏁 Script executed:
#!/bin/bash
# Verify the structure of wesley-holmes.yml workflow
echo "=== Workflow file structure ==="
cat .github/workflows/wesley-holmes.ymlLength of output: 19381
🏁 Script executed:
#!/bin/bash
# Check for job-level outputs in wesley-generate job
echo "=== Checking wesley-generate job for outputs section ==="
grep -n -A 20 "wesley-generate:" .github/workflows/wesley-holmes.yml | head -30Length of output: 716
🏁 Script executed:
#!/bin/bash
# Check downstream job references (holmes-investigate)
echo "=== Checking holmes-investigate job (line ~96) ==="
grep -n -A 15 "holmes-investigate:" .github/workflows/wesley-holmes.ymlLength of output: 603
🏁 Script executed:
#!/bin/bash
# Search for env var usage in downstream jobs
echo "=== Checking for hardcoded env vars ==="
grep -n "WESLEY_SCHEMA\|HOLMES_SCHEMA" .github/workflows/wesley-holmes.ymlLength of output: 656
🏁 Script executed:
#!/bin/bash
# Check if any downstream jobs reference needs.wesley-generate.outputs
echo "=== Checking for needs.wesley-generate.outputs references ==="
grep -n "needs\.wesley-generate\.outputs" .github/workflows/wesley-holmes.yml || echo "No references found"Length of output: 81
Job-level outputs: block missing—wiring is incomplete.
The detect step correctly outputs schema and bundle_dir to GITHUB_OUTPUT (lines 51–52), but these outputs are not exposed at the job level. Without a job-level outputs: block on wesley-generate, downstream jobs cannot access these values via the needs context—they're stuck using hardcoded ${{ env.HOLMES_SCHEMA }} and ${{ env.HOLMES_BUNDLE_DIR }} (confirmed across holmes-investigate, watson-verify, moriarty-predict, test-pgtap).
This breaks the stated extraction goal. Add this to wesley-generate (after line 19):
wesley-generate:
name: "🚀 Wesley Generation"
runs-on: ubuntu-latest
+ outputs:
+ schema: ${{ steps.detect.outputs.schema }}
+ bundle_dir: ${{ steps.detect.outputs.bundle_dir }}
steps:The follow-up PR can then switch downstream jobs to ${{ needs.wesley-generate.outputs.schema }} instead of hardcoded env vars. This needs to be in this PR to avoid dangling scaffolding.
BunBun
Committable suggestion skipped: line range outside the PR's diff.
|
Consolidated into integration PR #334 (integration/holmes-2025-10-27 → main). Keeping all changes via merge commits; original PR closed to reduce noise. |
1 participant
Extracted from #214: add a schema detection step and expose bundle_dir/schema via step outputs for later wiring. No behavior change yet; a follow-up PR can switch downstream steps to use needs.wesley-generate.outputs. Partial extraction to reduce scope and unblock CI refinement.