feat: add feature catalog and proactive guardian#369
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds a Relayfile feature catalog with validation, verification procedures, CI execution, and an hourly Slack guardian. It also adds extensive guardian tests and hardens E2E, conformance, packaging, evaluation, and agent-tool test harnesses. ChangesFeature catalog and guardian
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow as verify-features workflow
participant Catalog as feature catalog validator
participant Runner as tier verification runner
participant Guardian as relayfile-feature-guardian
participant Slack as Slack transport
participant Artifact as evidence report
Workflow->>Catalog: validate manifest and procedures
Workflow->>Runner: run selected verification tiers
Runner->>Artifact: write verification totals
Guardian->>Slack: post idempotent feature check
Guardian->>Artifact: checkpoint guardian progress
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8521773957
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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 introduces a feature guardian agent for Relayfile, including its implementation, manifest validation, and end-to-end verification workflows. The changes include adding new test suites, manifest validation scripts, and CI/CD workflow definitions. I have reviewed the comments and identified several actionable improvements, including fixing cross-platform path resolution, avoiding hardcoded version strings in tests, and ensuring explicit dependency management for type definitions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/e2e.ts (1)
1-1: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTeardown code throws mid-cleanup and can mask the real test failure; Biome flags two of these directly. All four sites share one root cause: a cleanup routine that throws as soon as one step fails, instead of attempting every cleanup step and preserving the original error.
scripts/e2e.ts#L88-116: instopAll(), catch/collect the error when a child doesn't exit after SIGKILL and continue terminating the remaining tracked children instead of throwing out of the loop.scripts/e2e.ts#L719-727: don'tthrowdirectly insidefinally(BiomenoUnsafeFinally); wrap teardown sostopAll()failures don't skipcloseAuth()/fixture removal, and surface fixture-safety failures via logging rather than a throw that can supersede a pending test failure.scripts/conformance.ts#L285-309: instopServer(), guard the SIGKILL-escalation failure (e.g. via try/finally) sors256Auth.close()and the owned binary/fixture removal still run even when the child won't exit; this function is awaited frommain()'sfinally(L853-857) so an uncaught throw here also risks masking the real suite failure.scripts/packed-feature-e2e.mjs#L97-104: don'tthrowdirectly insidefinally(BiomenoUnsafeFinally); log the fixture-safety failure instead so it can't replace an in-flight error from thetryblock.As a secondary improvement,
waitForExit/waitForServerExitand the fixture-marker safety checks are duplicated verbatim betweenscripts/e2e.tsandscripts/conformance.ts— extracting them to a shared script helper would let this fix be made once instead of three times.🤖 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/e2e.ts` at line 1, Make teardown failure-tolerant: update stopAll() and stopServer() to catch or defer SIGKILL/exit failures, continue all child termination and fixture/binary cleanup, and preserve any original test error. In the e2e and packed-feature cleanup finally blocks, avoid direct throws; always run closeAuth(), fixture removal, and related cleanup, logging fixture-safety failures instead of masking pending errors. Apply the same guarded behavior to the conformance main() finally path, and extract duplicated waitForExit/waitForServerExit and fixture-marker checks into a shared helper if practical.Source: Linters/SAST tools
🧹 Nitpick comments (4)
.github/workflows/verify-features.yml (1)
27-27: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable credential persistence in checkout.
To improve security hygiene and prevent the
GITHUB_TOKENfrom being left in the local git config where untrusted build scripts could access it, explicitly setpersist-credentials: falseon the checkout steps.
.github/workflows/verify-features.yml#L27-L27: Addwith: persist-credentials: falseto thisactions/checkout@v4step..github/workflows/verify-features.yml#L56-L56: Apply the same property to this checkout step.🤖 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 @.github/workflows/verify-features.yml at line 27, Disable credential persistence on both actions/checkout@v4 steps in .github/workflows/verify-features.yml at lines 27-27 and 56-56 by adding the with configuration with persist-credentials set to false.Source: Linters/SAST tools
scripts/validate-feature-catalog.mjs (1)
227-242: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrecompile coverage regexes outside the surface loop.
globPattern(entry.pattern)is reconstructed for every(surface, coverEntry)pair. At catalog scale (~1,632 surfaces × comparable cover patterns) this is on the order of millions ofRegExpcompilations per validation run. Compile once per entry and reuse.♻️ Precompile per-entry regex
+ const compiledEntries = coverEntries.map((entry) => ({ ...entry, regex: globPattern(entry.pattern) })); const usedPatterns = new Map(coverEntries.map((entry) => [`${entry.featureId}\0${entry.pattern}`, 0])); for (const surface of publicSurfaces) { - const matches = coverEntries.filter((entry) => globPattern(entry.pattern).test(surface)); + const matches = compiledEntries.filter((entry) => entry.regex.test(surface));🤖 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/validate-feature-catalog.mjs` around lines 227 - 242, Precompile each cover entry’s glob regex once before iterating over publicSurfaces, then reuse the compiled regex when checking matches instead of calling globPattern inside the surface loop. Update the matching logic around usedPatterns and preserve the existing exclusion, omission, overlap, and usage-count behavior.scripts/feature-runbook-lib.sh (1)
14-26: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
vf_cleanupdoesn't verify the parent directory beforerm -rf.Unlike the equivalent guards in
scripts/e2e.tsandscripts/conformance.ts(which also checkdirname(resolve(path)) === resolve(tmpdir())), this only checks the basename pattern. Add the same parent-directory check for defense-in-depth consistency.🛡️ Proposed fix
vf_cleanup() { local root="${VF_CASE_ROOT:-}" if [[ -z "$root" || ! -f "$root/.relayfile-feature-fixture" ]]; then printf '%s\n' 'FAIL cleanup refused: fixture marker is absent' >&2 return 1 fi + local expected_parent="$(cd "${TMPDIR:-/tmp}" && pwd)" + if [[ "$(cd "$(dirname "$root")" && pwd)" != "$expected_parent" ]]; then + printf 'FAIL cleanup refused unexpected parent: %s\n' "$root" >&2; return 1 + fi case "$(basename "$root")" in relayfile-feature-*.??????) ;; *) printf 'FAIL cleanup refused unexpected root: %s\n' "$root" >&2; return 1 ;; esac rm -rf -- "$root" unset VF_CASE_ROOT }🤖 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/feature-runbook-lib.sh` around lines 14 - 26, Update vf_cleanup to validate that the resolved parent directory of VF_CASE_ROOT matches the resolved system temporary directory before invoking rm -rf. Preserve the existing fixture-marker and basename checks, and refuse cleanup with an error when the parent-directory validation fails.scripts/conformance.ts (1)
185-255: 📐 Maintainability & Code Quality | 🔵 Trivial
skipCloudOnlyWebhookRouteboolean return is unreachable.The function only ever returns
falseor throwsSkippedTest; it never returnstrue. Theif (skipCloudOnlyWebhookRoute(...)) return;call sites are dead code — the skip already happens via the throw. Harmless today but confusing for future maintainers.🤖 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/conformance.ts` around lines 185 - 255, Remove the boolean return contract from skipCloudOnlyWebhookRoute: keep the 404 case throwing SkippedTest, but otherwise return void without returning false. Update every call site that checks its return value, including any if (skipCloudOnlyWebhookRoute(...)) return pattern, to invoke it directly and rely on the exception for skipping.
🤖 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/e2e.ts`:
- Around line 88-116: Update stopAll() so a child that remains alive after
SIGKILL does not abort cleanup of later children: record the failure, continue
iterating through all entries in children, and report the failure only after
every child has been processed. Preserve the existing graceful SIGTERM and
SIGKILL escalation behavior in waitForExit.
In `@scripts/feature-runbook-lib.sh`:
- Around line 28-35: Update vf_result to serialize status, procedure, and detail
with jq -n --arg (or equivalent complete JSON escaping) instead of manually
escaping only quotation marks. Preserve the existing field names, values, and
one-line JSON output while correctly encoding backslashes, newlines, and other
special characters.
In `@scripts/packed-feature-e2e.mjs`:
- Around line 83-84: Replace the hardcoded version check in the packed CLI test
with the dynamically derived SDK version, reusing the existing sdkVersion()
approach from build-mount-npm-packages.mjs. Update the assertion around
cli.stdout to validate the current package version while preserving the existing
drift error behavior.
---
Outside diff comments:
In `@scripts/e2e.ts`:
- Line 1: Make teardown failure-tolerant: update stopAll() and stopServer() to
catch or defer SIGKILL/exit failures, continue all child termination and
fixture/binary cleanup, and preserve any original test error. In the e2e and
packed-feature cleanup finally blocks, avoid direct throws; always run
closeAuth(), fixture removal, and related cleanup, logging fixture-safety
failures instead of masking pending errors. Apply the same guarded behavior to
the conformance main() finally path, and extract duplicated
waitForExit/waitForServerExit and fixture-marker checks into a shared helper if
practical.
---
Nitpick comments:
In @.github/workflows/verify-features.yml:
- Line 27: Disable credential persistence on both actions/checkout@v4 steps in
.github/workflows/verify-features.yml at lines 27-27 and 56-56 by adding the
with configuration with persist-credentials set to false.
In `@scripts/conformance.ts`:
- Around line 185-255: Remove the boolean return contract from
skipCloudOnlyWebhookRoute: keep the 404 case throwing SkippedTest, but otherwise
return void without returning false. Update every call site that checks its
return value, including any if (skipCloudOnlyWebhookRoute(...)) return pattern,
to invoke it directly and rely on the exception for skipping.
In `@scripts/feature-runbook-lib.sh`:
- Around line 14-26: Update vf_cleanup to validate that the resolved parent
directory of VF_CASE_ROOT matches the resolved system temporary directory before
invoking rm -rf. Preserve the existing fixture-marker and basename checks, and
refuse cleanup with an error when the parent-directory validation fails.
In `@scripts/validate-feature-catalog.mjs`:
- Around line 227-242: Precompile each cover entry’s glob regex once before
iterating over publicSurfaces, then reuse the compiled regex when checking
matches instead of calling globPattern inside the surface loop. Update the
matching logic around usedPatterns and preserve the existing exclusion,
omission, overlap, and usage-count behavior.
🪄 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: c802b758-3c83-4eeb-8aa4-d152fe339337
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
.agentworkforce/agents/relayfile-feature-guardian/agent.test.ts.agentworkforce/agents/relayfile-feature-guardian/agent.ts.agentworkforce/agents/relayfile-feature-guardian/manifest-contract.test.ts.agentworkforce/agents/relayfile-feature-guardian/persona.json.agentworkforce/features/README.md.agentworkforce/features/critical-paths.md.agentworkforce/features/manifest.yaml.agentworkforce/features/verify/procedures.md.claude/skills/verify-features.md.claude/skills/verify-features/SKILL.md.github/workflows/verify-features.yml.gitignoreREADME.mdpackage.jsonpackages/agents/src/tools/read-roots.test.tsscripts/build-mount-npm-packages.mjsscripts/conformance.tsscripts/e2e.tsscripts/evals/run-relayfile-evals.mjsscripts/feature-catalog-surfaces.mjsscripts/feature-runbook-lib.shscripts/packed-feature-e2e.mjsscripts/validate-feature-catalog.mjsscripts/verify-features.mjsscripts/verify-features.test.mjsworkflows/verify-features.ts
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
1 participant
Summary
Validation
npm run features:validate— 18 categories / 122 featuresnpm run features:test— 59 passing testsnpm run features:verify -- --tier deterministic— 7 PASS / 1 explicit PostgreSQL SKIPnpm run features:pack-e2ebash scripts/check-contract-surface.shnpm run buildnpm run typechecknpm test— 371 passing JS/TS tests / 3 platform skipsgo test ./...Honest limits
Hosted/provider/FUSE/Slack/release operations remain SKIP or MANUAL without their named opt-ins and disposable credentials. The checked-in offline eval aggregate still exposes its pre-existing live-input and unsupported jq-filter cases; it is not counted as a passing deterministic gate.