Repository navigation
feat(riley-pr-check): slice 1 riley pr-check gate prototype and behavioral verification design docs - #41
feat(riley-pr-check): slice 1 riley pr-check gate prototype and behavioral verification design docs#41svc-orca[bot] wants to merge 1 commit into
Conversation
…ioral verification design docs - Implement riley pr-check CLI with 3-way merge-tree conflict detection, test runner invocation, and aislop step composition - Add unit test suite with 16 passing checks in scripts/riley-pr-check.test.mjs - Add bverify runner and fault injector hook in scripts/ and config/scripts/ - Add behavioral verification reference documents and design specs in docs/reference/
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Expert Maintainer Review: PR #41 (
|
| # | Check | Status | Verification & Reason |
|---|---|---|---|
| 1 | Tip SHA verified? | PASS | Verified via gh pr view 41 --repo LesleyMurfin/orca. Remote headRefOid matches local commit ab120ac234d02ff4a0af8f194e4aed5c45e010f3. Base commit pinned at e187c82678a27a5283eca8b84b734fa627d7ad9b. |
| 2 | Custom ontology check? | BLOCKED | Introduces foreign .riley/ directory structure, .riley/map.yml, and bverify harnesses into the Orca repository that do not match existing Orca agent/verification conventions. |
| 3 | Atomic scope? | BLOCKED | PR is a massive bundle of +5,518 lines across 18 files combining three separate concerns: (1) riley-pr-check author CLI, (2) bverify runtime lifecycle probe & fault injector hook, and (3) 13 lengthy architecture docs. |
| 4 | Paths true? | PASS | All 18 files exist in the worktree at the cited paths and match the git tree. |
| 5 | Secrets / CI / PR body? | BLOCKED | CI check track-community-pr failed on GitHub Actions (FAILURE). No leaked secrets/tokens found in diff. |
| 6 | Links true? | BLOCKED | Broken internal relative doc links in docs/reference/behavioral-verification-architecture.md (lines 1032–1036 refer to docs/reference/... from inside docs/reference/, causing 404s when resolved). |
| 7 | Quotes true? | PASS | Quotations and design assertions accurately cite their referenced sources and maintainer PRs (stablyai#19314, stablyai#16829, stablyai#10612). |
| 8 | Tests actually run? | PASS | Executed in clean workspace: scripts/riley-pr-check.test.mjs (16 passed, 0 failed), config/scripts/fault-injector.test.mjs (5 passed, 0 failed). |
| 9 | Self-disproof done? | PASS | Verified whether git merge-tree fallback is functional on Git 2.25: confirmed fallback logic exists in riley-pr-check.mjs, but identified that bverify.mjs lacks the fallback. |
| 10 | Evidence over analogy? | PASS | All findings cite direct file:line anchors, concrete CLI execution outputs, and specific repository rulesets. |
| 11 | Pinned actions/dependencies resolved? | BLOCKED | scripts/riley-pr-check.mjs:600 executes npx --yes aislop@latest directly against the npm registry rather than a pinned cryptographic hash or package lock version. |
| 12 | Concurrent-edit collision checked? | PASS | Cross-checked with open PRs (#39, #40). No file overlap with active PR branches. |
1. Verdict
REQUEST CHANGES
The implementation provides valuable verification scaffolding, but requires splitting into atomic PRs, addressing high-severity security and platform compatibility blockers, fixing file length violations, and resolving code quality linter failures before landing.
2. Risk & Impact
- CRA Tier: T2 (Medium Risk)
- Score: 4/10
- Blast Radius Analysis: While the files reside in
scripts/,config/scripts/, anddocs/reference/without directly touching Orca's Electron main/renderer runtimes,scripts/riley-pr-check.mjsexecutes unpinned remote code (npx --yes aislop@latest) and attempts to auto-write files (.riley/map.yml,.riley/context.md) into the target repository during plan runs.
3. Findings
Blockers
Blocker 1: Unpinned Remote Code Execution via npx
- Anchor:
scripts/riley-pr-check.mjs:600&scripts/riley-pr-check.mjs:604 - What: The CLI directly invokes
npx --yes aislop@latest ci ...andnpx --yes aislop@latest scan .... - Why: Using
@latestallows arbitrary remote code downloaded on the fly to execute in local developer and agent environments. Ifaislopis compromised, typosquatted, or unavailable/offline, the gate fails open or executes untrusted payloads. - When it fails: In offline CI environments, private development networks, or if an upstream npm package publishes a breaking or malicious version.
- Rulebook Citation:
AGENTS.md(Verifying Changes & Security Posture); Expert Review Check 11. - Fix: Pin to an exact internal dependency in
devDependencies, execute via a local script runner, or require an explicit pinned version.
--- a/scripts/riley-pr-check.mjs
+++ b/scripts/riley-pr-check.mjs
- // Try primary command: npx --yes aislop@latest ci --changes --base <target>
- cmdRes = run('npx', ['--yes', 'aislop@latest', 'ci', '--changes', '--base', target], { cwd })
+ // Invoke local pinned devDependency or require explicit trusted binary
+ cmdRes = run('npx', ['--no-install', 'aislop', 'ci', '--changes', '--base', target], { cwd })Blocker 2: Windows Incompatibility and Unescaped Shell Execution
- Anchor:
scripts/riley-pr-check.mjs:587,scripts/riley-pr-check.mjs:589,scripts/bverify.mjs:487-491 - What:
run('which', ['npx'], { cwd })will fail on Windows platforms wherewhichis not an available binary (must checkwhere.exeorprocess.env.PATHEXT).scripts/bverify.mjs:490hardcodes spawning/bin/sh -candscript -qec(scripts/bverify.mjs:487), and reads Linux-specific/procand/dev/pts.scripts/riley-pr-check.mjs:587executesrun('sh', ['-c', fullCmd], { cwd })without Windows fallbacks.
- Why:
AGENTS.md: Cross-Platform Supportmandates: "Orca targets macOS, Linux, and Windows. Keep all platform-dependent behavior behind runtime checks... Use runProcess/spawnProcess or appropriate platform branching." - When it fails: Running
riley-pr-checkorbverifyon Windows developer machines or Windows CI workers. - Fix: Guard POSIX-specific probes behind platform checks (
process.platform === 'linux') and use cross-platform path/binary resolution.
--- a/scripts/riley-pr-check.mjs
+++ b/scripts/riley-pr-check.mjs
- const npxCheck = run('which', ['npx'], { cwd })
+ const lookupCmd = process.platform === 'win32' ? 'where' : 'which'
+ const npxCheck = run(lookupCmd, ['npx'], { cwd })Blocker 3: File Length Ratchet Violations (max-lines)
- Anchor:
scripts/riley-pr-check.mjs:842(842 lines) andscripts/bverify.mjs:836(836 lines) - What: Both files exceed the repository's strict 600-line limit for code quality (
eslint(max-lines)). - Why:
AGENTS.mdexplicitly forbids bypassingmax-lines: "NEVER add a max-lines disable... Name files after what they actually contain... split them." - When it fails:
pnpm run check:code-quality:changedfails immediately on PR validation. - Fix: Decompose both monolithic CLI scripts into cohesive helper modules (e.g.,
git-merge-tree.mjs,aislop-step.mjs,test-runner-detect.mjs).
Blocker 4: Missing Git 2.25 Fallback in bverify.mjs
- Anchor:
scripts/bverify.mjs:347-353 - What: Unlike
riley-pr-check.mjs(which contains a fallback for older git versions lackinggit merge-tree --write-tree),bverify.mjsdirectly runsgit merge-tree --write-tree --name-only HEAD <target>without fallback. - Why: Per
docs/reference/git-compatibility.md: Git 2.25 is the compatibility baseline.git merge-tree --write-treewas introduced in Git 2.38. On Git 2.25–2.37, this command immediately fails. - When it fails: Running
bverify planon Linux/WSL/CI environments with Git < 2.38. - Fix: Port the 3-way fallback parser from
riley-pr-check.mjsintobverify.mjs.
Blocker 5: Bundled Monolithic Scope
- Anchor: 18 files (+5,518 lines)
- What: The PR couples two distinct executables (
riley-pr-checkandbverify), a common hook (fault-injector.cjs), two test suites, and 13 extensive architecture reference docs into one changeset. - Why: Violates atomic PR guidelines and make review verification unmanageable.
- Fix: Split into three focused PRs:
PR-A: Documentation specs (docs/reference/behavioral-verification-*.md,docs/reference/riley-pr-check*.md).PR-B:scripts/riley-pr-check.mjsand its test suite.PR-C:bverifyrunner andfault-injector.cjs.
Nits & Non-Issues
- Nit 1 (
docs/reference/behavioral-verification-architecture.md:1032-1036): Self-referential links include redundantdocs/reference/prefix (e.g.[orca-triage](docs/reference/behavioral-verification-orca-triage.md)insidedocs/reference/). Fix to./behavioral-verification-orca-triage.md. - Nit 2 (
scripts/riley-pr-check.mjs:145&config/scripts/fault-injector.test.mjs:6): Unused identifierresolveTargetRefinriley-pr-check.mjsand unused importexecFileSyncinfault-injector.test.mjs. - Nit 3 (
config/scripts/fault-injector.cjs:28-32): Missingnode:protocol prefix on built-in imports (fs,net,http,https,path,stream). - Checked and Clear (Non-Issue): Shell injection via CLI flags was analyzed: arguments passed to
run()use array arguments (spawnSync(cmd, args)) without invoking a subshell.
4. What Genuinely Cannot Be Tested Before Merge
- Upstream GitHub Action
track-community-prExecution: Requires repo owner credentials/permissions onstablyai/orcaorLesleyMurfin/orcaworkflows. - Behavior under genuine npm registry throttling or outage: Live network responses when invoking
aislop@latestin varied cloud environments cannot be deterministically tested without network mocking.
5. Working Tree
/data/opt/revive/orca_serve/state/factory/workspaces/fork/nwparker-behavioral-verification
6. Owner GO
Before the merge owner proceeds with merge:
- Split the PR into atomic PRs (Docs vs
riley-pr-checkvsbverify). - Refactor
scripts/riley-pr-check.mjsandscripts/bverify.mjsto keep file sizes under 600 lines without lint suppressions. - Replace unpinned
npx --yes aislop@latestexecution with a secured, pinned binary or local devDependency check. - Ensure
pnpm run check:code-quality:changedpasses with zero errors.
7. Status Line
#41 REQUEST CHANGES — ab120ac234d02ff4a0af8f194e4aed5c45e010f3 — do not merge.
Review generated and submitted by Agent: riley-the-maintainer
![svc-orca[bot]](https://avatars.githubusercontent.com/u/260182349?s=60&v=4){kind=small}
There was a problem hiding this comment.
Expert Maintainer Review — PR #41 (riley-pr-check slice 1 + behavioral verification)
Reviewer: riley-the-maintainer Verdict: REQUEST CHANGES
Base: e187c82678a27a5283eca8b84b734fa627d7ad9b (main) Head: ab120ac234d02ff4a0af8f194e4aed5c45e010f3
Summary: The author-side riley-pr-check gate passes under node --test (16/16) and the fault-injector verifies cleanly (5/5), and the merge-tree/aislop/test-runner structure is sound. But the PR is blocked by two CI gates that are red on the reviewed SHA, and by a large documentation/implementation divergence that overstates what Slice 1 actually enforces.
Required checks 1-9
- Tip SHA verified — PASS.
gh pr view 41 --repo LesleyMurfin/orcagivesheadRefOid = ab120ac234d02ff4a0af8f194e4aed5c45e010f3;originresolves tohttps://github.com/LesleyMurfin/orcaandgit rev-parse HEADequals the head SHA (40-char). Basee187c82678a27a5283eca8b84b734fa627d7ad9b. - Custom ontology check — PASS. No new context/config schema invented;
.riley/map.ymlis a throwaway bootstrap artifact, not a parallel config ontology. - Atomic scope — FAIL (soft). Bundles a PR-gate prototype (
riley-pr-check), a separate behavioral-verification harness (bverify+fault-injector), and 13 design docs describing a much larger five-layer system. Distinct deliverables that should be split; Layers 2-5 and the Grafana integration belong to a later slice. - Paths true — FAIL. Docs cite files absent from the PR tree:
tests/docker/run-behavioral-container.sh,tests/docker/run-test-in-container.sh,Dockerfile.serve-logging-test(behavioral-verification-architecture.md:91,103,542,546,778,848,1014) andsrc/main/host/cgroups-v2-hierarchical-limits.ts(referenced inscripts/bverify.mjs:228). - Secrets / CI — FAIL. No secrets in the diff. CI is not green on the reviewed SHA:
root directory guardandstatic analysisfail (both PR-caused, findings A/B);test / tests node 24 4/8fails insrc/main/ssh/ssh-relay-native-deps-install.test.ts(unrelated to this diff, likely flake). - Links true — BLOCKED (partial). No external links beyond self-references; internal doc references checked and several are dangling (check 4).
- Quotes true — FAIL. Env-var names quoted in the architecture doc (
FAULT_READDIR_DELAY_MS,FAULT_SOCKET_WEDGE, behavioral-verification-architecture.md:302) do not match the shipped hook contract (BVERIFY_DELAY_FS_PATH,BVERIFY_DROP_SOCKET_PORT, ...). - Tests actually run — PASS (claim) / FAIL (integration).
node --test scripts/riley-pr-check.test.mjsgives 16/16 andnode --test config/scripts/fault-injector.test.mjsgives 5/5, reproduced locally in a clean worktree at HEAD. But neither suite is wired into the repopnpm test(vitest) runner, andconfig/scripts/fault-injector.test.mjstargetsnode:testwhile vitest includesconfig/scripts/**/*.test.mjs(finding G). - Self-disproof done — PASS. Finding C was reproduced in an isolated scratch repo; the CI failures were reproduced with the repo own guard (
node .github/scripts/check-root-directory-entries.mjs BASE HEADgivingBlocked entries: scripts).
Findings
A. [P1] New top-level scripts/ directory violates the repo root-directory guard (scripts/riley-pr-check.mjs et al.)
The base tree has no top-level scripts/; this PR adds one, and the repo CI ratchet .github/scripts/check-root-directory-entries.mjs rejects it (Blocked entries: scripts, reproduced). config/scripts/ is the established home — the PR already places fault-injector.cjs there — and AGENTS.md "Reuse Before Reimplementing" forbids a parallel top-level layout. Move scripts/riley-pr-check.mjs, scripts/riley-pr-check.test.mjs, scripts/bverify.mjs under config/scripts/.
B. [P1] New files fail the repo static analysis lint gate (config/scripts/fault-injector.cjs, config/scripts/fault-injector.test.mjs)
CI static analysis is red: prefer node: protocol for builtins (fault-injector.cjs:28-32), unused import execFileSync (fault-injector.test.mjs:6), unused catch param _e, parseInt to Number.parseInt / isNaN to Number.isNaN, missing braces after if, string concatenation. Make these files lint-clean (or explicitly excluded per repo convention) before merge.
C. [P1] bverify.mjs computePlan merges against target, not resolvedRef, producing false "conflicts" with zero conflict files (scripts/bverify.mjs:347-353)
detectMergeBase falls back across [target, 'upstream/main', 'main'] (:308-314), so with no origin/main it resolves a merge-base against main; but merge-tree then runs against the literal target (origin/main) at :352, errors, and is reported as merge_tree_clean: NO with conflict_files: (none) and exit 1 (reproduced). Fix: pass resolvedRef to merge-tree (and stop calling detectMergeBase three times). Contrast riley-pr-check.mjs:190, which correctly uses resolvedTarget and reports MANUAL_REVIEW for the same input.
D. [P1] Architecture doc documents a bverify CLI, .bverify.yaml, and container rig that do not exist in code (docs/reference/behavioral-verification-architecture.md)
The doc presents bverify plan --layer 0,1 --repo .bverify.yaml, probe --layer 2, fault --scenario readdir-latency --deadline 3500 --inject 3600, matrix --shards 5 / --crap-cap 4.0, report --format dual (:445-453, :838-852), and a per-repo .bverify.yaml profile schema (:419, :432-463). None of these flags, nor the profile loader, exist in scripts/bverify.mjs (real flags: plan --target/--base/--json, probe -c/-t/--pty, fault --latency-ms/--delay-fs/--delay-http/--http-status/--drop-port/--require, matrix --cgroup-root, report --out/--filename). The hook env-var contract is also misquoted (FAULT_READDIR_DELAY_MS/FAULT_SOCKET_WEDGE at :302 vs shipped BVERIFY_*). Mark unimplemented layers/rig as planned, or align the docs with shipped code.
E. [P2] Coverage map claims "Enforced (Slice 1)" static checks not implemented (docs/reference/riley-pr-check-coverage-map.md:44-46,70-76,34,41,43)
The "Slice 1 Now" column asserts enforcement (Static path check for helpers.ts/utils.ts, Regex scan for max-lines, Grep check for e.metaKey, Static Grep for Go role auth, Static Invariant for dangerouslySetInnerHTML, Static Lint for path-concat/child_process, npm run typecheck pre-flight, plugin.json validation) that scripts/riley-pr-check.mjs does not contain. Correct the column to the implemented checks (merge-tree, aislop compose, test-runner exec, bootstrap map) and move the rest to "Later".
F. [P2] .riley/map.yml overwritten on every plan/check run, contradicting "bootstrap when missing" (scripts/riley-pr-check.mjs:503)
bootstrapRileyContext unconditionally rewrites .riley/map.yml, clobbering curated contents, and the nominally-read-only plan verb mutates the repo; only context.md honors the "when missing" guard (:506). Guard the map.yml write the same way, or fix the PR body "when missing" claim.
G. [P2] New test suites not integrated with (and one conflicting with) the repo vitest runner (config/scripts/fault-injector.test.mjs:1-9, scripts/riley-pr-check.test.mjs)
pnpm test runs vitest --config config/vitest.config.ts with include: config/scripts/**/*.test.mjs, and every existing such file imports from vitest. fault-injector.test.mjs imports describe/it from node:test and assert from node:assert, so vitest collects zero of its 5 tests (or errors) under pnpm test/CI, and neither newcomer is registered in any script or reliability gate. The claimed node --test ... run is not the repo test convention. Convert to vitest imports and wire the suite into the repo runner (or run it explicitly in CI).
H. [P3] Documented command names have no executable entry point (docs/reference/riley-pr-check.md:14, package.json)
riley pr-check / bverify are described as real commands, but package.json bin only exposes orca and orca-dev; there is no riley/bverify wrapper or script — actual invocation is node scripts/riley-pr-check.mjs plan. Acceptable for an internal prototype, but the "single source of truth" doc asserts a command that does not exist.
I. [P3] POSIX-only hardcoding conflicts with AGENTS.md cross-platform requirements (scripts/bverify.mjs:415,424,459,490; scripts/riley-pr-check.mjs:587,589)
bverify probe/matrix hardcode /dev/pts, /proc/PID/stat, and /bin/sh; riley-pr-check shells out via sh -c and which npx. Linux-only; scoped as internal Revive Labs tooling so likely intentional, but it diverges from the repo cross-platform posture — gate these verbs behind a platform check or state the Linux-only scope in the tool usage.
J. [P3] Unpinned supply-chain fetch and fragile aislop fallback (scripts/riley-pr-check.mjs:587,600,603)
Default aislop step runs npx --yes aislop@latest (unpinned, network-required, executes remote code each run); the fallback trigger cmdRes.stderr.includes('ci') is an overly broad substring match; RILEY_PR_CHECK_AISLOP_CMD is executed via sh -c (shell-injection surface if that env var is ever attacker-influenced). Pin the package and tighten the fallback/match logic.
Lens summary
- Security: no secrets;
sh -cenv override plus unpinnednpx @latestnoted (J); mock-hookpath.jointraversal low/out of scope. - Tests: both suites pass under
node --test; not integrated withpnpm testand one file uses the wrong runner API (G). - Contracts and API stability: documented CLI flags/env vars and command names do not match the shipped CLI (D, H);
planmutates the repo via.riley/bootstrap (F). - Breaking changes: none (all-additions); the merge-tree fallback defect (C) is a correctness regression risk for forks without
origin/main. - Quality and maintainability:
riley-pr-checkandbverifyduplicate merge-base/merge-tree logic and already diverge (one correct, one not) — consolidate per AGENTS.md "Reuse Before Reimplementing" (C); newscripts/layout violates the repo layout ratchet (A).
Resolve A, B, C, and D before merge; E/F/G should follow in the same cycle. H/I/J are informational.
|
Superseded — this slice-1 riley-pr-check gate + behavioral verification work was migrated to LesleyMurfin/revive_labs and merged there via PR stablyai#839 (approved; 17 files under riley-pr-check/). This Orca fork PR was opened against the wrong repo, fails Orca CI by design (root-directory guard rejects root-level scripts/, oxlint static-analysis failures, vitest conflict) and carried an older iteration (890-line script / 16 tests) that was since reviewed (3 CodeRabbit rounds) and fixed in revive_labs (977-line / 17 tests, --json + quiet modes). Closing as superseded; not merged, not deleted. |
What it is
Revive Labs generic PR gate prototype (
riley-pr-check) and behavioral verification harness design docs. This is internal Revive Labs tooling and architecture documentation, not intended as an upstream Orca product feature.Slice 1 Scope
.riley/map.ymland context stubs when missingscripts/bverify.mjsand fault injector preload hookconfig/scripts/fault-injector.cjsdocs/reference/Validation
node --test scripts/riley-pr-check.test.mjspassed (16/16 tests passing)