Ops capi hardening - #46
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
Warning Review limit reached
Next review available in:119 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (13)
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 |
reprewindai-dev
commented
Aug 13, 2026
Blocking merge for concrete reasons despite green checks:
Please split the operational port/config cleanup from the Lockerphycer/R2/Outly additions, remove deployment-specific defaults, make integration tests fail when required services fail (or explicitly mark environment-gated tests as skipped), and require a successful Lockerphycer write before returning a dual-ledger evidence claim. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:1d46a54d7f
ℹ️ 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".
| import { test, expect } from '@playwright/test'; | ||
| test.describe('cAPI Backend Integrations', () => { |
There was a problem hiding this comment.
The existing npm test command runs vitest run, and vitest.config.ts does not exclude Playwright files, so Vitest discovers this new integration.spec.ts and evaluates Playwright's test.describe outside the Playwright runner. This makes the repository's required CI test step fail before these integration checks can run; add a Playwright-specific script/config and exclude its specs from Vitest (or use a non-Vitest filename/location).
Useful? React with 👍 / 👎.
| ...decision, | ||
| evidence_reference: { evidence_id: anchored.event_id, entry_hash: anchored.event_hash, ledger: "pgl" }, | ||
| evidence_reference: { | ||
| evidence_id: parsed.data.action_id, |
There was a problem hiding this comment.
For every successful intercept, this replaces the actual pglAnchored.event_id with the caller-controlled action ID. Consumers pass evidence_reference.evidence_id to /api/capi/v1/evidence/[id], which forwards that value to PGL's event lookup, so the returned reference can no longer retrieve the event that was just anchored; the outcome route still correctly returns the PGL event ID.
Useful? React with 👍 / 👎.
| evidence_reference: { | ||
| evidence_id: parsed.data.action_id, | ||
| entry_hash: pglAnchored.event_hash, | ||
| ledger: "dual-pgl-lockerphycer" |
There was a problem hiding this comment.
When Lockerphycer is unavailable or returns any non-2xx response, registerAuditRecord() returns false, but that result is ignored and the response still labels the evidence as dual-pgl-lockerphycer. This produces a successful compliance response that asserts an audit copy exists when only the PGL event was stored; either fail closed/check lockerphycerAnchored or report only the ledger that actually accepted the record.
Useful? React with 👍 / 👎.
| uses: ForAllSecure/mapi-action@v2 | ||
| with: | ||
| mayhem-token: ${{ secrets.MAYHEM_TOKEN }} | ||
| api-url: http://localhost:3000 |
There was a problem hiding this comment.
In this workflow, npm run dev expands to next dev -p 3003, while Mayhem is configured to connect to port 3000. Next's -p, --port <port> option explicitly selects the listening port, so the CLI argument takes precedence over the step's PORT=3000; after the fixed sleep, the action probes a port with no cAPI server and the new DAST job cannot exercise the API.
Useful? React with 👍 / 👎.
| if (!this.trust.has(agent.agent_id)) { | ||
| this.trust.set(agent.agent_id, { | ||
| agent_id: agent.agent_id, | ||
| score: agent.metadata.tier === "system" ? 95 : 50, |
There was a problem hiding this comment.
When an agent is first loaded from Lockerphycer, every non-system tier now starts at trust 50. The existing registerAgent() path assigns service agents 78 and user agents 52, so a remote service identity is unexpectedly denied by otherwise applicable policies with trust thresholds of 60 or 70 even though the same identity registered locally would pass; reuse registerAgent() or the same tier mapping here.
Useful? React with 👍 / 👎.
| import type { AgentIdentity, Policy, CapabilityIdentity } from "./types"; | ||
| const LOCKERPHYCER_URL = process.env.LOCKERPHYCER_URL || "http://lockerphycer-api:8092"; |
There was a problem hiding this comment.
When LOCKERPHYCER_URL is unset, this hard-coded hostname still enables the integration, so every unknown agent triggers an outbound fetch before the runtime can return its normal 401. In deployments outside that Docker network, an unresolved or blackholed lockerphycer-api host—and the absence of any fetch timeout—can turn attacker-controlled unknown-agent requests into long-running work instead of immediate rejection; use an empty default and skip the lookup unless explicitly configured.
Useful? React with 👍 / 👎.
| const identity: AgentIdentity = await res.json(); | ||
| agentCache.set(agent_id, { identity, expiresAt: Date.now() + CACHE_TTL_MS }); |
There was a problem hiding this comment.
A successful HTTP response is cast directly to AgentIdentity without runtime validation. If Lockerphycer returns a 200 error envelope, a wrapped payload, or an otherwise incomplete record, the object is cached and runtime.process() then dereferences fields such as agent.metadata.tier, producing an uncaught 500 on this and subsequent requests rather than treating the identity lookup as unavailable; validate the full identity shape and requested agent_id before returning it.
Useful? React with 👍 / 👎.
| # --- Phase 6 execution bridge (Veklom BYOS MCP gateway) --- | ||
| BYOS_MCP_GATEWAY_URL= | ||
| BYOS_MCP_GATEWAY_URL=http://n13gp1nhrcdp0hvazvbnlxru-213557155694:8088/api/v2/invoke |
There was a problem hiding this comment.
With this configuration, every mcp:// capability posts the JSON-RPC tools/call payload from src/lib/covenant/mcp-bridge.ts to /api/v2/invoke. The checked-in Interlink router binds /api/v2/invoke to an InvocationRequest requiring capability_id, arguments, and context, while its JSON-RPC handler is a different route, so the configured endpoint rejects the bridge payload before execution; configure a compatible MCP endpoint or serialize the REST invocation contract.
Useful? React with 👍 / 👎.
| # into gnomledger's append-only, hash-chained ledger. Leave empty to keep the | ||
| # local seal only. | ||
| PGL_LEDGER_URL= | ||
| PGL_LEDGER_URL=http://gnomledger-api-1:8001 |
There was a problem hiding this comment.
The file instructs developers to copy it to .env.local and explicitly says this value should be empty to retain local-only sealing, but the new default enables a Docker-network-specific hostname with no API key. A normal local setup copied from the template therefore attempts an unavailable or unauthorized external ledger instead of remaining disabled, changing Outly calls from the intended clear unconfigured response into network/integration failures and generating failed-forwarding records throughout the runtime.
Useful? React with 👍 / 👎.
| console.log("\n📡 Submitting to cAPI (Governed Connection Layer) -> /api/outly/intercept"); | ||
| try { | ||
| const response = await fetch("https://capi.veklom.com/api/outly/intercept", { |
There was a problem hiding this comment.
Running this developer test script always submits its generated demo action to the production capi.veklom.com endpoint, even though its failure message tells the user to start a localhost server. Each invocation can therefore add synthetic Outly decisions to the production immutable PGL and Lockerphycer audit trail; use an environment-provided base URL with a localhost default and require an explicit opt-in for production.
Useful? React with 👍 / 👎.
reprewindai-dev
commented
Aug 13, 2026
Merge-blocking reconciliation: this PR currently violates the canonical source/runtime and secret/topology boundaries. Exact head |
reprewindai-dev
left a comment
There was a problem hiding this comment.
Merge remains blocked. Fresh review has 12 unresolved threads, including P1 failures in evidence-reference identity, false dual-anchor claims when Lockerphycer rejects/unavailable, revoked Lockerphycer identity refresh, Mayhem targeting forbidden/wrong port 3000, BYOS bridge contract mismatch, and Vitest discovering Playwright specs. There is also no exact-head Actions run. Keep this PR draft; remove concrete internal topology/default integrations, make provider/ledger integrations opt-in, use canonical cAPI 3003 everywhere, make evidence claims reflect only confirmed durable writes, and reconcile with #44's fail-closed authority path before exact-head test/lint/build/security verification.
reprewindai-dev
left a comment
There was a problem hiding this comment.
Exact-head remediation review: keep this PR draft and unmerged. Twelve unresolved threads remain, including P1 defects for Playwright/Vitest collision, wrong returned PGL evidence ID, false dual-anchor success when Lockerphycer did not persist, Mayhem targeting forbidden/wrong port 3000, Lockerphycer identity cache surviving revocation/key rotation, and BYOS bridge endpoint/contract mismatch. P2 issues also remain around trust initialization, OpenAPI heartbeat schema drift, unconfigured Lockerphycer fallback, response validation, PGL template defaults, and production-default test harness behavior. Reconcile this branch with #44’s fail-closed authority path; remove concrete internal topology from source-controlled examples; keep cAPI canonical runtime 3003; and do not claim PGL/Lockerphycer evidence success unless the persisted identifiers are real and independently retrievable. I attempted to push the small topology/3000 corrections directly, but the connector safety layer blocked repository file mutations, so this review records the exact missing corrections without claiming they landed.
No description provided.