Repository navigation
Conversation
|
🔍 OpenCodeReview found 6 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| body: JSON.stringify({ | ||
| model, | ||
| input: text, | ||
| ...(config.embeddingDimension ? { dimensions: config.embeddingDimension } : {}), |
There was a problem hiding this comment.
dimensions is only validated as positive, so fractional values (for example 1.5) reach provider APIs via dimensions; OpenAI-compatible endpoints expect an integer and will reject the probe. Require z.number().int().positive() in the route schema (and ideally validate at this boundary too).
Suggestion:
| ...(config.embeddingDimension ? { dimensions: config.embeddingDimension } : {}), | |
| ...(config.embeddingDimension !== undefined | |
| ? { dimensions: config.embeddingDimension } | |
| : {}), |
| "semanticCacheEmbeddingDimension", | ||
| "semanticCacheEmbeddingBaseUrl", | ||
| "semanticCacheEmbeddingApiKey", | ||
| "semanticCacheRedisUrl", |
There was a problem hiding this comment.
Removing the key from CACHE_CONFIG_KEYS makes the dashboard load semApiKey as empty, but saveSemanticCache() always sends semanticCacheEmbeddingApiKey: semApiKey.trim() || null; any subsequent semantic-cache save therefore clears the persisted provider credential, breaking embeddings. Keep the secret out of GET while preserving it on partial updates, or distinguish “unchanged” from explicit clear in the client/API.
Suggestion:
| "semanticCacheEmbeddingDimension", | |
| "semanticCacheEmbeddingBaseUrl", | |
| "semanticCacheEmbeddingApiKey", | |
| "semanticCacheRedisUrl", | |
| "semanticCacheEmbeddingDimension", | |
| "semanticCacheEmbeddingBaseUrl", | |
| "semanticCacheRedisUrl", |
There was a problem hiding this comment.
Fixed at 1794e1654b92a3ff79a0abe1916d7ae096e311a8: GET continues to redact semanticCacheEmbeddingApiKey, while a PUT carrying redacted null preserves the stored credential. Focused cache-config-route-8219 regression verifies GET never leaks the secret and subsequent redacted save does not clear it (6/6 suite PASS).
| if (requiresBootstrapTokenClaim && !consumeBootstrapToken(bootstrapCandidate)) { | ||
| return NextResponse.json({ error: "Local bootstrap required" }, { status: 403 }); | ||
| } | ||
|
|
||
| await updateSettings(updates); |
There was a problem hiding this comment.
Token is consumed before updateSettings succeeds. A transient DB/write failure now burns the one-shot bootstrap credential, leaving the operator unable to retry and potentially requiring manual recovery. Consume only after a successful write, or add an atomic claim/commit mechanism that can release the claim on failure while preserving one-shot concurrency semantics.
Suggestion:
| if (requiresBootstrapTokenClaim && !consumeBootstrapToken(bootstrapCandidate)) { | |
| return NextResponse.json({ error: "Local bootstrap required" }, { status: 403 }); | |
| } | |
| await updateSettings(updates); | |
| await updateSettings(updates); | |
| if (requiresBootstrapTokenClaim && !consumeBootstrapToken(bootstrapCandidate)) { | |
| return NextResponse.json({ error: "Local bootstrap required" }, { status: 403 }); | |
| } |
| if (dialect === "tei") { | ||
| return { | ||
| ...common, | ||
| texts: input.documents.map(documentText), | ||
| return_text: returnDocuments, | ||
| }; |
There was a problem hiding this comment.
TEI's /rerank schema does not accept the Cohere-only model/top_n fields (it expects query, texts, and optionally return_text). Because common is spread into this dialect, the retry still receives unknown fields and strict TEI responds 422, so the fallback remains broken. Build a TEI-specific body without those fields; apply top_n locally after normalization.
Suggestion:
| if (dialect === "tei") { | |
| return { | |
| ...common, | |
| texts: input.documents.map(documentText), | |
| return_text: returnDocuments, | |
| }; | |
| if (dialect === "tei") { | |
| return { | |
| query: input.query, | |
| texts: input.documents.map(documentText), | |
| return_text: returnDocuments, | |
| }; |
| const selfHostedConfigured = Boolean( | ||
| process.env[SELF_HOSTED_CONFIG_ENV] || process.env[SELF_HOSTED_CONFIG_FILE_ENV] | ||
| ); | ||
| if (selfHostedConfigured) { | ||
| const requestedModel = typeof parsedBody.model === "string" ? parsedBody.model : null; | ||
| const selfHostedPolicy = await enforceApiKeyPolicy(request, requestedModel); | ||
| if (selfHostedPolicy.rejection) { | ||
| return finishAdmission(selfHostedPolicy.rejection); | ||
| } | ||
| } | ||
|
|
||
| const selfHostedResponse = await handleSelfHostedCompletions(request, parsedBody); |
There was a problem hiding this comment.
Self-hosted policy runs for every request whenever either config env var exists, but handleSelfHostedCompletions can still return null for requests that are not routed to self-hosted. Cloud requests therefore receive self-hosted model/endpoint/quota checks and can be denied incorrectly. Determine routing first (or make policy conditional on the actual self-hosted route).
Suggestion:
| const selfHostedConfigured = Boolean( | |
| process.env[SELF_HOSTED_CONFIG_ENV] || process.env[SELF_HOSTED_CONFIG_FILE_ENV] | |
| ); | |
| if (selfHostedConfigured) { | |
| const requestedModel = typeof parsedBody.model === "string" ? parsedBody.model : null; | |
| const selfHostedPolicy = await enforceApiKeyPolicy(request, requestedModel); | |
| if (selfHostedPolicy.rejection) { | |
| return finishAdmission(selfHostedPolicy.rejection); | |
| } | |
| } | |
| const selfHostedResponse = await handleSelfHostedCompletions(request, parsedBody); | |
| const selfHostedResponse = await handleSelfHostedCompletions(request, parsedBody); | |
| if (selfHostedResponse) { | |
| const requestedModel = typeof parsedBody.model === "string" ? parsedBody.model : null; | |
| const selfHostedPolicy = await enforceApiKeyPolicy(request, requestedModel); | |
| if (selfHostedPolicy.rejection) { | |
| return finishAdmission(selfHostedPolicy.rejection); | |
| } | |
| return finishAdmission(selfHostedResponse); | |
| } |
There was a problem hiding this comment.
Revalidated against the actual self-hosted entry contract at new PR HEAD 1794e1654b92a3ff79a0abe1916d7ae096e311a8: handleSelfHostedCompletions returns null only when BOTH config env vars are absent; when configured, it always returns a response (including config failures). The route's selfHostedConfigured uses those same vars, so there is no cloud fallback on this configured branch to misapply policy to. Moving policy after dispatch, as suggested, would authorize after an upstream side effect and violate the original security requirement. Classified NOT_REPRODUCED/FALSE_PREMISE in the tracked disposition ledger. Independent review remains welcome.
| const selfHostedPolicy = await enforceApiKeyPolicy(request, requestedModel); | ||
| if (selfHostedPolicy.rejection) { | ||
| return finishAdmission(selfHostedPolicy.rejection); | ||
| } |
There was a problem hiding this comment.
enforceApiKeyPolicy is inside the broad prompt-injection try/catch; any policy/backend exception is logged, then execution continues into handleChat. This converts the policy's documented fail-closed behavior into a fail-open path. Catch policy errors separately and return a sanitized 5xx response (while releasing admission).
Suggestion:
| const selfHostedPolicy = await enforceApiKeyPolicy(request, requestedModel); | |
| if (selfHostedPolicy.rejection) { | |
| return finishAdmission(selfHostedPolicy.rejection); | |
| } | |
| let selfHostedPolicy; | |
| try { | |
| selfHostedPolicy = await enforceApiKeyPolicy(request, requestedModel); | |
| } catch { | |
| return finishAdmission(errorResponse(503, "API key policy unavailable")); | |
| } | |
| if (selfHostedPolicy.rejection) { | |
| return finishAdmission(selfHostedPolicy.rejection); | |
| } |
There was a problem hiding this comment.
Fixed at 1794e1654b92a3ff79a0abe1916d7ae096e311a8: enforceSelfHostedPolicyOrReject separately catches policy/backend exceptions, returning sanitized 503 before handleSelfHostedCompletions, so the outer injection catch cannot fail-open. Focused self-hosted test verifies thrown policy -> 503; restricted API-key test verifies zero upstream calls.
| if (res.status === 400 || res.status === 422) { | ||
| res = await fetch(activeUrl, makeUpstreamInit("tei")); | ||
| } |
There was a problem hiding this comment.
This retries every 400/422, not only schema-validation responses as the comment claims. Invalid query/documents, unsupported model, or provider-specific semantic errors trigger a second inference request and replace the original diagnostic; this also adds avoidable request-wide amplification. Inspect the error body/content type (or use a provider-specific schema signal) before retrying.
Suggestion:
| if (res.status === 400 || res.status === 422) { | |
| res = await fetch(activeUrl, makeUpstreamInit("tei")); | |
| } | |
| if (res.status === 400 || res.status === 422) { | |
| const errorText = await res.clone().text().catch(() => ""); | |
| const isSchemaError = /(?:unknown|extra|missing|required|field|documents|texts)/i.test(errorText); | |
| if (isSchemaError) { | |
| res = await fetch(activeUrl, makeUpstreamInit("tei")); | |
| } | |
| } |
| const allParts = [ | ||
| ...(value.contents ?? []).flatMap((content) => content.parts ?? []), | ||
| ...(value.systemInstruction?.parts ?? []), | ||
| ]; |
There was a problem hiding this comment.
z.object(...).catchall(z.unknown()) accepts system_instruction, but this refinement only traverses value.systemInstruction. A request with contents plus snake_case system instruction can carry inline media outside both per-part and 16 MiB request accounting. Either reject the unsupported alias or normalize/include it in validation and downstream conversion.
Suggestion:
| const allParts = [ | |
| ...(value.contents ?? []).flatMap((content) => content.parts ?? []), | |
| ...(value.systemInstruction?.parts ?? []), | |
| ]; | |
| const allParts = [ | |
| ...(value.contents ?? []).flatMap((content) => content.parts ?? []), | |
| ...(value.systemInstruction?.parts ?? []), | |
| ]; |
CI Coverage Report
Coverage artifact was not available for this run. |
KitDev25 / AP-ISS-0197 — BLOCKED_RECOVERY (read-only host verification)This is GitHub PR discussion evidence only, NOT an authoritative KitControl handoff, lease extension, recovery, or board-status mutation.
BLOCKER: Machine-private KitControl lifecycle recovery + durable handoff/escalation writes are unavailable in this client; stale Gen15 authority cannot safely resume owned mutation. This PR note is a noncanonical support breadcrumb, not a substitute for KitControl control-plane persistence. NEXT: GoalKeeper/Architect use a deployed approved privileged lifecycle client to validate takeover safety, perform DO-NOT: No raw actor/claim/lease/handoff edits, fabricated heartbeat/ACTIVE, anonymous/root execution, blind re-recovery, WIP reset/stash/clean, or force-push. Preserve Data-is-Gold. |
Preserve one-shot bootstrap retry semantics and strict embedding/rerank/Gemini boundaries; fail closed on self-hosted policy failure. Add direct-peer/auth rejection regressions and 8-finding review disposition evidence.
KitDev25 Gen16 — BLOCKED_EXECUTION_CONTENTION (do not merge)Current published PR #142 head: CRITICAL NEW COORDINATION BLOCKER: KitControl existing OPEN escalation Verified local tests/gates: AP-ISS-0197 cache/embedding/rerank 14/14 PASS, extended set 72/72 PASS, Codex/GLM/ownership/cache set 51/51 PASS and auth/chat/cache integration set 89/89 PASS; security 23/23 PASS; core and Open-SSE typechecks, route-guard, public-creds, Prettier, scoped ESLint with suppression bank, diff check PASS. These are independent host invocations, not hosted exact-head CI. At last check for DO NOT MERGE OR CLOSE. Next: GoalKeeper/KitArchitect must reconcile |
…ality Preserve management auth's fail-closed behavior; model a trusted loopback socket peer in the old settings regression and document red-to-green verification.
| const isSessionAuth = await isDashboardSessionAuthenticated(request); | ||
| if (isSessionAuth) { | ||
| return { |
There was a problem hiding this comment.
Session detection now returns before parsing the bearer key. A request carrying a valid dashboard cookie plus an invalid/disabled/malformed API key is accepted as session-authenticated, whereas the previous flow rejected the invalid presented key. This lets clients bypass API-key validation/policy by adding a dashboard cookie and can hide credential-confusion errors. Parse and reject a presented invalid key before taking the session-auth fallback (or explicitly define and enforce session precedence across all API routes).
Suggestion:
| const isSessionAuth = await isDashboardSessionAuthenticated(request); | |
| if (isSessionAuth) { | |
| return { | |
| const isSessionAuth = await isDashboardSessionAuthenticated(request); | |
| const apiKey = extractApiKey(request); | |
| if (apiKey) { | |
| const apiKeyMetadata = await getApiKeyMetadata(apiKey); | |
| const keyState = apiKeyMetadata && (await validateApiKey(apiKey)) ? "valid" : "invalid"; | |
| if (keyState === "invalid") { | |
| return { | |
| apiKey, | |
| apiKeyId: null, | |
| apiKeyMetadata: null, | |
| keyState, | |
| rejection: unauthorized("Invalid API key"), | |
| isSessionAuth, | |
| }; | |
| } | |
| } | |
| if (isSessionAuth) { | |
| return { |
| const systemText = | ||
| geminiBody.systemInstruction.parts?.map((p) => p.text ?? "").join("\n") || ""; | ||
| // Convert either Gemini JS camelCase or REST snake_case system instruction. | ||
| const systemInstruction = geminiBody.systemInstruction ?? geminiBody.system_instruction; |
There was a problem hiding this comment.
When both accepted fields are present, ?? chooses systemInstruction and drops system_instruction without warning. Reject the ambiguous request or define a merge policy to avoid silently changing the prompt.
Suggestion:
| const systemInstruction = geminiBody.systemInstruction ?? geminiBody.system_instruction; | |
| if (geminiBody.systemInstruction && geminiBody.system_instruction) { | |
| throw new Error("Specify only one system instruction field"); | |
| } | |
| const systemInstruction = geminiBody.systemInstruction ?? geminiBody.system_instruction; |
| for (const key of ["inlineData", "inline_data"]) { | ||
| const inline = record[key]; | ||
| if (!inline || typeof inline !== "object" || Array.isArray(inline)) continue; | ||
| const data = (inline as { data?: unknown }).data; | ||
| if (typeof data === "string") payloads.push(data); | ||
| } |
There was a problem hiding this comment.
When a part contains both spellings, validation totals both payloads, but conversion chooses only one via inlineData ?? inline_data. This makes request limits differ from actual forwarded content. Reject both keys together or use a consistent merge policy.
Suggestion:
| for (const key of ["inlineData", "inline_data"]) { | |
| const inline = record[key]; | |
| if (!inline || typeof inline !== "object" || Array.isArray(inline)) continue; | |
| const data = (inline as { data?: unknown }).data; | |
| if (typeof data === "string") payloads.push(data); | |
| } | |
| if (record.inlineData !== undefined && record.inline_data !== undefined) return []; |
| for (const data of inlineDataPayloads(part)) { | ||
| if ( | ||
| data.length > MAX_EMBEDDING_INLINE_ITEM_BASE64_LENGTH || | ||
| decodedBase64Bytes(data) > MAX_EMBEDDING_INLINE_ITEM_BYTES | ||
| ) { |
There was a problem hiding this comment.
These size checks only estimate decoded length; unlike the embedding schema, they never verify base64 syntax. Malformed inline media can pass validation and be forwarded. Reuse a shared base64 validator and reject invalid data.
Suggestion:
| for (const data of inlineDataPayloads(part)) { | |
| if ( | |
| data.length > MAX_EMBEDDING_INLINE_ITEM_BASE64_LENGTH || | |
| decodedBase64Bytes(data) > MAX_EMBEDDING_INLINE_ITEM_BYTES | |
| ) { | |
| for (const data of inlineDataPayloads(part)) { | |
| if ( | |
| !isValidBase64(data) || | |
| data.length > MAX_EMBEDDING_INLINE_ITEM_BASE64_LENGTH || | |
| decodedBase64Bytes(data) > MAX_EMBEDDING_INLINE_ITEM_BYTES | |
| ) { |
| if (isSessionAuth) { | ||
| return { | ||
| apiKey: null, | ||
| apiKeyId: null, | ||
| apiKeyMetadata: null, | ||
| keyState: "none", | ||
| rejection: null, | ||
| isSessionAuth: true, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Session auth returns before evaluating a supplied API key, so mixed-credential requests bypass that key’s ownership and scope restrictions and gain instance-wide access. Reject mixed credentials or evaluate the API key consistently.
Suggestion:
| if (isSessionAuth) { | |
| return { | |
| apiKey: null, | |
| apiKeyId: null, | |
| apiKeyMetadata: null, | |
| keyState: "none", | |
| rejection: null, | |
| isSessionAuth: true, | |
| }; | |
| } | |
| if (isSessionAuth && extractApiKey(request)) { | |
| return { | |
| apiKey: null, | |
| apiKeyId: null, | |
| apiKeyMetadata: null, | |
| keyState: "invalid", | |
| rejection: unauthorized("Conflicting credentials"), | |
| isSessionAuth: true, | |
| }; | |
| } | |
| if (isSessionAuth) { | |
| return { | |
| apiKey: null, | |
| apiKeyId: null, | |
| apiKeyMetadata: null, | |
| keyState: "none", | |
| rejection: null, | |
| isSessionAuth: true, | |
| }; | |
| } |
| if (await isLocalRerankSchemaValidationResponse(res)) { | ||
| res = await fetch(activeUrl, makeUpstreamInit("tei")); | ||
| } |
There was a problem hiding this comment.
This broad match treats any 400/422 containing validation error or field required as a dialect mismatch. Invalid client/provider input (for example an unsupported model or missing required query) therefore triggers a second authenticated POST with a different body, adding request amplification and potentially obscuring the original error. Restrict the predicate to evidence of documents/return_documents versus texts/return_text incompatibility, or only retry when the response explicitly identifies an unknown/missing dialect field.
Suggestion:
| if (await isLocalRerankSchemaValidationResponse(res)) { | |
| res = await fetch(activeUrl, makeUpstreamInit("tei")); | |
| } | |
| if (await isLocalRerankSchemaValidationResponse(res)) { | |
| res = await fetch(activeUrl, makeUpstreamInit("tei")); | |
| } |
| const systemInstruction = geminiBody.systemInstruction ?? geminiBody.system_instruction; | ||
| if (systemInstruction) { |
There was a problem hiding this comment.
Both field names are accepted, but conversion silently drops system_instruction whenever systemInstruction exists, while validation aggregates both. A request can therefore validate content that is never processed, and conflicting instructions have nondeterministic client-visible semantics. Reject the duplicate pair (including both casing variants on one part) or merge them explicitly before conversion.
Suggestion:
| const systemInstruction = geminiBody.systemInstruction ?? geminiBody.system_instruction; | |
| if (systemInstruction) { | |
| if (geminiBody.systemInstruction && geminiBody.system_instruction) { | |
| throw new Error("systemInstruction and system_instruction cannot both be provided"); | |
| } | |
| const systemInstruction = geminiBody.systemInstruction ?? geminiBody.system_instruction; | |
| if (systemInstruction) { |
| if (activeClaim || !peekBootstrapToken(candidate)) return null; | ||
| activeClaim = Object.freeze({ kind: "bootstrap-token-claim" }); | ||
| return activeClaim; |
There was a problem hiding this comment.
A stalled or abandoned updateSettings() leaves this process-wide claim active, rejecting all later bootstrap writes; cancellation may not run this handler’s catch/release path. Ensure claim cleanup is bounded or persistence has a guaranteed timeout/finally release.
Suggestion:
| if (activeClaim || !peekBootstrapToken(candidate)) return null; | |
| activeClaim = Object.freeze({ kind: "bootstrap-token-claim" }); | |
| return activeClaim; | |
| if (activeClaim || !peekBootstrapToken(candidate)) return null; | |
| activeClaim = Object.freeze({ kind: "bootstrap-token-claim" }); | |
| return activeClaim; |
| for (const key of ["inlineData", "inline_data"]) { | ||
| const inline = record[key]; |
There was a problem hiding this comment.
The aggregate/per-item byte checks count both inlineData and inline_data, but conversion uses only part.inlineData ?? part.inline_data; when both are supplied, the second payload is charged but ignored. Reject duplicate aliases or apply the same deterministic merge/rejection rule in both validation and conversion.
Suggestion:
| for (const key of ["inlineData", "inline_data"]) { | |
| const inline = record[key]; | |
| const camel = record.inlineData; | |
| const snake = record.inline_data; | |
| if (camel !== undefined && snake !== undefined) return []; | |
| const inline = camel ?? snake; | |
| if (inline && typeof inline === "object" && !Array.isArray(inline)) { | |
| const data = (inline as { data?: unknown }).data; | |
| return typeof data === "string" ? [data] : []; | |
| } | |
| return []; |
|
GoalKeeper evidence-bound CI triage (2026-10-21 ~06:38 ICT), NO SOURCE MUTATION:
GoalKeeper 60m board logger newly started at |
1 participant
AP-ISS-0197
Terminally dispositions all 21 API-route security/lifecycle OCR findings against current accepted code, with minimal RED→GREEN fixes only for reproduced defects.
Terminal dispositions
4233363247: management-authenticated GLM reset-card route with no modeled tenant/owner column on provider connections)The full per-finding evidence ledger is committed at
docs/audits/ap-iss-0197-dispositions.md.Main integration
Protected
origin/main166b4cbb52595005522d2aed5d47282a0942918ewas merged before publication and re-fetched unchanged immediately before the current head was pushed. The sole overlap,open-sse/services/cache/embeddingClient.ts, preserves both AP-ISS-0195 SSRF/cooldown hardening and AP-ISS-0197 embedding-dimension propagation.Exact-head verification
Current published PR head:
e9611720ae09c0a534756c85c213c57e2e22c3fe.Fresh local evidence on this exact head:
npm run test:security: 23 pass / 0 failGITHUB_BASE_REF=main npm run check:test-masking: PASSnpm run check:docs-all: PASSnpm run typecheck:core: PASS (RC=0)npm run check:open-sse-typecheck: PASS (RC=0,openSseTypecheckErrors=0)node scripts/check/check-api-typecheck.mjs: PASS (apiTypecheckErrors=276, all frozen baseline; 0 new)npm run check:route-guard-membership: PASSnpm run check:public-creds: PASSgit diff --check origin/main...HEAD: PASSnpm run check:secrets: environment SKIP becausegitleaksis unavailable, per repository gate behaviorA bounded full
npm testwas attempted before main integration. At the ~30-minute boundary Node reported 10,973 tests / 6,012 pass / 0 fail / 4,959 cancelled / 2 skipped. The wrapper did not persist a terminal return-code marker; this is recorded as incomplete timeout evidence and is not claimed as a green full-suite result.Hosted follow-up
Predecessor heads exposed four actionable CI findings; all were reproduced locally before repair:
batches-delete-completed-route-scope.test.tshad dropped two behavioral assertions while changing the superseded key-wins expectation to AP-ISS-0197 session authority. The replacement restores two meaningful file-deletion postcondition assertions; test-masking passes.ApiKeyRequestScope.apiKeyMetadatawas tied to the DB-only full metadata type while policy hydration supplies the policy metadata shape. The boundary now models the commonid/scopessubset; API-route typecheck is back at frozen baseline with 0 new errors.check:docs-allpasses.files-batches-ownership-2jm2-m3hp.test.tsstill encoded the superseded “API key wins over dashboard session” rule in three cases. Only those mixed-session expectations were aligned to AP-ISS-0197 instance-operator authority; key-only invalid/ownership guards remain unchanged. The file is 13/13 green and the expanded focused set is 110/110 green.No terminal merge/close claim is made until all required hosted workflows for exact head
e9611720ae09…are terminal green and current review threads are re-read.