Skip to content

fix(runtime): close general library OCR findings (AP-ISS-0201) - #139

Open
khanhkit wants to merge 10 commits into
mainfrom
kit/ap-iss-0201-library-ocr
Open

khanhkit wants to merge 10 commits into
mainfrom
kit/ap-iss-0201-library-ocr

Conversation

@khanhkit

@khanhkit khanhkit commented Oct 13, 2026 •

Copy link
Copy Markdown
Owner

AP-ISS-0201 — general library-runtime OCR findings

Closes the 15 findings authorized by the live AP-ISS-0201 claim (1 HIGH + 14 MEDIUM) with bounded RED→GREEN fixes/dispositions against current code. This supersedes older routing/count assumptions.

Exact candidate

  • head: 5d6fe9bf935d20d35ac845869df2be6484444e80
  • branch: kit/ap-iss-0201-library-ocr
  • worktree clean after publish
  • current origin/main observed before publish: 166b4cbb52595005522d2aed5d47282a0942918e

Current finding/review disposition

  • All 15 live-claim findings have terminal source-level dispositions with reproduction/current-code evidence in the KitControl handoff history.
  • Prior independent KitAuditor review SA-20261013175832-63767dde3297: PASS, 15/15 acceptance criteria, 0 missing.
  • Exact-head OCR follow-up fixed two real post-review findings: xAI orphan/mismatched role:function handling and OpenWA startup-state tolerance. API-bridge import and reasoning-carrier comments were dispositioned as false positives against current code/contract; all prior inline threads were resolved before this head.
  • 5d6fe9bf… fixes the hosted TS2367 regression exposed by the OpenWA startup-state remediation by re-reading post-await service state instead of comparing the stale control-flow-narrowed field. Runtime behavior is unchanged.

Fresh local verification for 5d6fe9bf…

  • API Route Typecheck: PASS (276 frozen-baseline errors, no regression)
  • Dashboard Typecheck: PASS (196 frozen-baseline errors, no regression)
  • OpenWA startup regression: 2/2 PASS
  • service unit boundary: 17/17 PASS
  • npm run typecheck:core: PASS
  • npm run quality:scan:fast: 20/20 PASS
  • npm run test:security: 23/23 PASS
  • touched ESLint: PASS with repository-supported --pass-on-unpruned-suppressions
  • touched Prettier: PASS
  • git diff --check: PASS
  • Full npm test was attempted locally but the KitMCP connector aborted mid-run before a terminal test summary; no product failure was recorded. Exact-head protected hosted CI is authoritative for the full-suite result.

Hosted exact-head oracle

Fresh CI, API Route Typecheck, OpenCodeReview, CodeQL, semgrep, and DAST runs were triggered on exact head 5d6fe9bf…. They must be terminal-successful before final acceptance; all new review threads must be dispositioned/resolved against this same head.

Integration sequencing / blocker

AP-ISS-0149 / PR #123 remains open and unmerged and is the API Bridge lifecycle predecessor. This PR must not be integrated yet. After AP-ISS-0149 reaches accepted main, fetch/rebase this branch, revalidate the API Bridge trust-boundary behavior against that accepted lifecycle implementation, and obtain fresh exact-head hosted evidence. AP-ISS-0136 remains blocked until this issue is terminal.

@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

🔍 OpenCodeReview found 6 issue(s) in this PR.

  • ✅ Successfully posted inline: 4 comment(s)
  • ⏭️ Skipped (overlap with history): 1 comment(s)
  • ❌ Failed to post inline: 1 comment(s)

security · high

📄 src/lib/memory/embedding/index.ts (L284-L287)

⚠️ GitHub could not post this as an inline comment: Lines 284-287 could not be resolved (outside PR diff hunks)

EmbeddingError.message promises sanitized output, but this new branch returns error.message verbatim. A custom endpoint validation error can contain credentials, headers, or internal URLs and is exposed to callers/UI. Apply sanitizeErrorMessage(...) here, matching remote.ts.

💡 Suggested Change

Before:

        message:
          error instanceof Error
            ? error.message
            : "Custom embedding endpoint is invalid or blocked",

After:

        message: sanitizeErrorMessage(
          error instanceof Error
            ? error.message
            : "Custom embedding endpoint is invalid or blocked"
        ),

Comment thread src/app/api/models/test-all/route.ts
Comment thread src/lib/providers/xai/translators/openai-chat.ts Outdated
Comment thread src/lib/reasoningRouting/policy.ts
@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

CI Coverage Report

  • Coverage job: success
  • PR test policy: success

Coverage artifact was not available for this run.

Comment thread src/lib/apiBridgeServer.ts
Comment thread src/lib/services/ServiceSupervisor.ts
@khanhkit

Copy link
Copy Markdown
Owner Author

AP-ISS-0201 review disposition for the non-inline embedding HIGH: current resolveMemoryCustomEmbeddingProvider() catches endpoint validation/to-URL failures and rethrows only MemoryCustomEmbeddingConfigError, whose message is the constant sanitized string Custom embedding endpoint is invalid or blocked. The embed() catch therefore cannot expose the raw endpoint/validator error on this path. No source change: the suggested extra sanitization would be redundant and would add churn without changing observable behavior.

Copy link
Copy Markdown
Owner Author

KitGoalKeeper dependency reconciliation (2026-10-20 14:32 ICT, read-only GitHub + KitControl check): The integration-sequencing claim in this PR body is stale. AgentProxy PR #123 (AP-ISS-0149) is MERGED, accepted merge SHA cf9a1a04e1d57c32d9b0d74d4a8b68ecfc6f921a (merged by 2026-10-17). The current KitDev33 / AP-ISS-0201 canonical handoff still lists #123 as OPEN, while this PR #139 remains OPEN. This does NOT imply #139 is ready to merge: owner must revalidate the accepted lifecycle base, check branch/head ancestry, rerun exact-head CI/reviews and resolve any remaining threads. GoalKeeper is NOT claiming a worker running from recovered lease, did NOT mutate code/lease/claim, and has not verified implementation readiness. Please update the task handoff and PR integration sequence via the real current owner using supported KitControl/Git workflow. Preserve all WIP.

…OCR fixes

Non-destructive integration of accepted main cf9a1a0 into AP-ISS-0201 head 5d6fe9b. Tree sourced from GitHub's conflict-free PR #139 merge preview 9c7b4b4, preserving both changes. Re-run exact-head gates before merge.

Copy link
Copy Markdown
Owner Author

KitDev33 Gen19 — post-AP-ISS-0149 integration checkpoint (2026-10-21 ICT)

  • Durable KitControl owner: KitDev33 / AP-ISS-0201 / task-run tr_8d17668f826b4ac3abc24e54b606f345; scoped GoalKeeper recovery to Gen19 was verified before writing. This PR comment is evidence only, not control-plane authority or a handoff replacement.
  • Accepted main contains AP-ISS-0149 PR fix(lifecycle): establish process-global shutdown authority #123 at cf9a1a04e1d57c32d9b0d74d4a8b68ecfc6f921a.
  • New PR head 7e0433672135407a892e099f08a11636afc514f0 is a non-force, fast-forward two-parent merge commit (5d6fe9bf935d20d35ac845869df2be6484444e80 + cf9a1a04e1d57c32d9b0d74d4a8b68ecfc6f921a) with the exact tree from GitHub's conflict-free merge preview. No branch history rewritten, no local WIP reset/stash/clean.
  • The combined src/lib/apiBridgeServer.ts preserves both explicit-trusted-proxy hardening and AP-ISS-0149 relay/drain lifecycle; src/lib/arenaEloSync.ts preserves complete-refresh freshness and background-lifecycle tracking. GitHub comparison shows 40 issue-scoped files, zero behind accepted main.
  • NOT MERGE READY pending terminal exact-head CI / security / reviewer gates. OpenCodeReview's first run 39558511594 failed because worker 39558563428 attempt 1 had ocr: command not found (exit 127) after its install step reported success. Worker attempt 2 was triggered and remains under observation. Other CI/security checks for this head are still in progress.
  • Protected KitControl heartbeat attempts via Dev and GoalKeeper shared lifecycle currently fail with lifecycle bridge rejected request (2) despite assess succeeding; do not claim handoff/checkpoint or ACTIVE state was persisted by those rejected operations.

NEXT: finish exact-head CI and review; if all required gates pass, merge via expected-head SHA and verify accepted-main provenance; persist canonical KitControl handoff/close using supported privileged lifecycle only. If gates cannot be repaired within issue scope or lifecycle writes remain denied, keep ownership/WIP and escalate via official KitControl support. DO NOT force-update refs, bypass failed checks, or hand-edit protected runtime.

Comment thread src/lib/arenaEloSync.ts
Comment thread src/lib/arenaEloSync.ts
Comment thread src/lib/providers/xai/translators/openai-chat.ts
Comment on lines +77 to +78
net.isIP(candidate) !== 0 &&
candidate.replace(/^::ffff:/, "").toLowerCase() === peer.toLowerCase()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security · medium
IPv6 peers are compared as raw strings after only stripping ::ffff:. Equivalent IPv6 spellings (for example fd00::1 versus fd00:0:0:0:0:0:0:1) will not match, so a configured trusted proxy can be classified as remote; forwarding headers are then stripped and the proxy address is recorded as the client. Normalize both addresses via parsed IP/binary representation before comparison, or require canonical configuration and validate it explicitly.

Suggestion:

Suggested change
net.isIP(candidate) !== 0 &&
candidate.replace(/^::ffff:/, "").toLowerCase() === peer.toLowerCase()
net.isIP(candidate) !== 0 &&
normalizeIp(candidate) === normalizeIp(peer)

Comment thread src/lib/arenaEloSync.ts
Comment on lines +517 to 521
const latestCompleteRefresh = readLastCompleteRefreshAt();
if (latestCompleteRefresh !== null) {
const age = Date.now() - latestCompleteRefresh;
if (age >= 0 && age < SYNC_INTERVAL_MS) {
return {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · high
lastCompleteRefreshAt survives clearSyncedIntelligence(). After a manual DELETE/clear, this guard can return success within SYNC_INTERVAL_MS while the Arena rows are empty, preventing repopulation until the interval expires. Clear or invalidate this marker when clearing the dataset, or require stored rows before taking this fast path.

Suggestion:

Suggested change
const latestCompleteRefresh = readLastCompleteRefreshAt();
if (latestCompleteRefresh !== null) {
const age = Date.now() - latestCompleteRefresh;
if (age >= 0 && age < SYNC_INTERVAL_MS) {
return {
const latestCompleteRefresh = readLastCompleteRefreshAt();
if (latestCompleteRefresh !== null && hasArenaEloEntries()) {
const age = Date.now() - latestCompleteRefresh;
if (age >= 0 && age < SYNC_INTERVAL_MS) {
return {

Comment thread src/lib/arenaEloSync.ts
Comment on lines 564 to 566
applyArenaEloRefresh(entries);
writeLastCompleteRefreshAt(Date.now());
clearLastFailedAt();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
applyArenaEloRefresh commits before this KV write. If writeLastCompleteRefreshAt fails (disk/DB error), the catch reports the sync as failed even though the dataset was replaced; the freshness marker remains stale, so every retry re-fetches/re-applies the same data and can repeatedly report failure. Make the marker update part of the same transaction, or treat marker persistence as best-effort after a successful apply.

Suggestion:

Suggested change
applyArenaEloRefresh(entries);
writeLastCompleteRefreshAt(Date.now());
clearLastFailedAt();
applyArenaEloRefresh(entries);
try {
writeLastCompleteRefreshAt(Date.now());
} catch (err) {
console.warn(`[ARENA_ELO_SYNC] Failed to persist refresh timestamp: ${getErrorMessage(err)}`);
}
clearLastFailedAt();

Comment on lines +578 to +579
if (targetFormat === "claude") dropIncompatibleCarrier(body, "reasoning");
if (targetFormat === "openai-responses") dropIncompatibleCarrier(body, "output_config");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
targetEffort === "none" exits through clearReasoning before these carrier guards. For targetFormat === "openai-responses", an existing output_config therefore survives, even though this format treats reasoning as the carrier and output_config as incompatible. A force-disable rule can still send the stale carrier and be rejected/misinterpreted. Remove the incompatible carrier in the none path too.

Suggestion:

Suggested change
if (targetFormat === "claude") dropIncompatibleCarrier(body, "reasoning");
if (targetFormat === "openai-responses") dropIncompatibleCarrier(body, "output_config");
if (targetFormat === "claude") dropIncompatibleCarrier(body, "reasoning");
if (targetFormat === "openai-responses") dropIncompatibleCarrier(body, "output_config");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant