Skip to content

Commit c64a89e

Browse files
Merge branch 'main' into fix/critical-issue-anchorable-8352
2 parents 7bf9534 + f1b5cc1 commit c64a89e

29 files changed

Lines changed: 1148 additions & 82 deletions
Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
import { afterEach, describe, expect, it, vi } from "vitest";
2+
import { handleAnalyticsProxy } from "./analytics-proxy";
3+
4+
// #8387: the analytics proxy is the cookieless-beacon relay to the Umami-compatible upstream. Its four
5+
// security behaviors (strict allowlist, cookie strip, cf-connecting-ip-only x-forwarded-for, set-cookie
6+
// strip) had zero coverage. These pin each one against a stubbed upstream fetch.
7+
8+
const UPSTREAM = "https://tasty.aethereal.dev";
9+
10+
type ForwardedCall = { url: string; method: string; headers: Headers };
11+
12+
/** Stub global fetch to return `response`, recording each forwarded request in a typed, inspectable list. */
13+
function stubUpstream(response: Response) {
14+
const calls: ForwardedCall[] = [];
15+
const fetchMock = vi.fn(
16+
async (url: string | URL, init?: { method?: string; headers?: HeadersInit }) => {
17+
calls.push({
18+
url: String(url),
19+
method: init?.method ?? "GET",
20+
headers: new Headers(init?.headers),
21+
});
22+
return response;
23+
},
24+
);
25+
vi.stubGlobal("fetch", fetchMock);
26+
return { fetchMock, calls };
27+
}
28+
29+
function send(init: RequestInit & { path?: string; query?: string } = {}) {
30+
const { path = "/stats/api/send", query = "", ...rest } = init;
31+
return new Request(`https://loopover.ai${path}${query}`, { method: "POST", ...rest });
32+
}
33+
34+
afterEach(() => {
35+
vi.unstubAllGlobals();
36+
});
37+
38+
describe("handleAnalyticsProxy", () => {
39+
it("forwards an allowed POST to the upstream collect endpoint, preserving the query and relaying the response", async () => {
40+
const { fetchMock, calls } = stubUpstream(
41+
new Response("ok-body", { status: 202, statusText: "Accepted" }),
42+
);
43+
44+
const response = await handleAnalyticsProxy(
45+
send({ query: "?v=2&cache=abc", body: "beacon-payload" }),
46+
);
47+
48+
expect(fetchMock).toHaveBeenCalledTimes(1);
49+
// /stats prefix stripped, path + query preserved onto the real upstream host.
50+
expect(calls[0]!.url).toBe(`${UPSTREAM}/api/send?v=2&cache=abc`);
51+
expect(calls[0]!.method).toBe("POST");
52+
// Upstream status/statusText/body are relayed back untouched.
53+
expect(response).toBeInstanceOf(Response);
54+
expect(response!.status).toBe(202);
55+
expect(response!.statusText).toBe("Accepted");
56+
expect(await response!.text()).toBe("ok-body");
57+
});
58+
59+
it("rejects a disallowed method with 405 + an allow header, without ever calling fetch (method gate)", async () => {
60+
const { fetchMock } = stubUpstream(new Response("should not be used"));
61+
62+
const response = await handleAnalyticsProxy(send({ method: "GET" }));
63+
64+
expect(response!.status).toBe(405);
65+
expect(response!.headers.get("allow")).toBe("POST");
66+
expect(fetchMock).not.toHaveBeenCalled();
67+
});
68+
69+
it("returns undefined (falls through to SSR) and never fetches for a path outside the allowlist", async () => {
70+
const { fetchMock } = stubUpstream(new Response("should not be used"));
71+
72+
// The admin/auth API lives on the same upstream origin as the collect endpoint -- must NOT be proxied.
73+
expect(await handleAnalyticsProxy(send({ path: "/stats/admin" }))).toBeUndefined();
74+
expect(await handleAnalyticsProxy(send({ path: "/stats/api/collect" }))).toBeUndefined();
75+
expect(fetchMock).not.toHaveBeenCalled();
76+
});
77+
78+
it("strips the visitor's first-party cookie before forwarding (cookieless guarantee, #597)", async () => {
79+
const { calls } = stubUpstream(new Response(null, { status: 200 }));
80+
81+
await handleAnalyticsProxy(
82+
send({ headers: { cookie: "session=secret; theme=dark", "x-keep": "yes" } }),
83+
);
84+
85+
expect(calls[0]!.headers.get("cookie")).toBeNull();
86+
// Non-stripped headers still pass through, so this isn't just dropping everything.
87+
expect(calls[0]!.headers.get("x-keep")).toBe("yes");
88+
});
89+
90+
it("re-derives x-forwarded-for from the trusted cf-connecting-ip and drops any client-supplied value (no geo spoofing)", async () => {
91+
const { calls } = stubUpstream(new Response(null, { status: 200 }));
92+
93+
await handleAnalyticsProxy(
94+
send({ headers: { "cf-connecting-ip": "203.0.113.7", "x-forwarded-for": "66.66.66.66" } }),
95+
);
96+
97+
expect(calls[0]!.headers.get("x-forwarded-for")).toBe("203.0.113.7");
98+
// The trusted-IP header itself is not leaked upstream.
99+
expect(calls[0]!.headers.get("cf-connecting-ip")).toBeNull();
100+
});
101+
102+
it("does not set x-forwarded-for when there is no cf-connecting-ip", async () => {
103+
const { calls } = stubUpstream(new Response(null, { status: 200 }));
104+
105+
await handleAnalyticsProxy(send({ headers: { "x-forwarded-for": "66.66.66.66" } }));
106+
107+
expect(calls[0]!.headers.get("x-forwarded-for")).toBeNull();
108+
});
109+
110+
it("strips set-cookie from the upstream response before relaying it to the browser", async () => {
111+
stubUpstream(
112+
new Response("ok", {
113+
status: 200,
114+
headers: { "set-cookie": "umami=1; Path=/", "x-app": "v1" },
115+
}),
116+
);
117+
118+
const response = await handleAnalyticsProxy(send());
119+
120+
expect(response!.headers.get("set-cookie")).toBeNull();
121+
expect(response!.headers.get("x-app")).toBe("v1"); // unrelated response headers are still relayed
122+
});
123+
124+
it("fails quietly with 502 when the upstream fetch throws (analytics must never take the page down)", async () => {
125+
vi.stubGlobal(
126+
"fetch",
127+
vi.fn(async () => {
128+
throw new Error("network down");
129+
}),
130+
);
131+
132+
const response = await handleAnalyticsProxy(send({ body: "beacon" }));
133+
134+
expect(response!.status).toBe(502);
135+
});
136+
});

packages/loopover-engine/README.md

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,9 @@ flags, so it works on the whole declared `engines` range.
3636
Codecov (`codecov/patch`) only reads the root vitest suite, so modules that need a Codecov-visible mirror also
3737
have a root test under `test/unit/` (e.g. `test/unit/reviewer-consensus-calibration.test.ts` for
3838
`src/reviewer-consensus-calibration.ts`#8349, or `test/unit/signal-tracking.test.ts` for
39-
`src/calibration/signal-tracking.ts`#8343). The package-local `node:test` suite remains the package's own
40-
gate; the root mirror is what makes the same scenarios gradeable by Codecov.
39+
`src/calibration/signal-tracking.ts`#8343, or `test/unit/harness-submission-trigger.test.ts` for
40+
`src/miner/harness-submission-trigger.ts`#8346). The package-local `node:test` suite remains the package's
41+
own gate; the root mirror is what makes the same scenarios gradeable by Codecov.
4142

4243
## `opportunity-ranker`
4344

@@ -681,6 +682,16 @@ These modules compute the *decisions*; the append-only record of what was decide
681682
in [Governor ledger](#governor-ledger) below (`allowed` / `denied` / `throttled` / `kill_switch`), which the
682683
chokepoint's returned ledger event feeds.
683684

685+
`action-mode.ts`'s dry-run-by-default precedence (`resolveMinerActionMode` and siblings) has a Codecov-visible
686+
root mirror at `test/unit/miner-governor-action-mode.test.ts` (#8345) — `codecov/patch` only reads the root
687+
vitest suite (see [Test](#test)), so this safety-adjacent write-execution gate is gradeable there as well as by
688+
the package's own `node:test` suite, alongside the existing `test/unit/miner-governor-kill-switch.test.ts` mirror.
689+
690+
Similarly, the miner self-review adapter (`src/miner/self-review-adapter.ts`, which builds the predicted-gate +
691+
slop inputs the miner's self-review pass runs) has a Codecov-visible root mirror at
692+
`test/unit/self-review-adapter.test.ts` (#8348) — again because `codecov/patch` only reads the root vitest suite
693+
(see [Test](#test)), not the package's own `node:test` suite.
694+
684695
## Governor ledger
685696

686697
`normalizeGovernorLedgerEvent` validates append-only governor decision rows before the local miner persists them.

packages/loopover-engine/src/governor-ledger.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,25 @@ function normalizeRequiredString(value: unknown, code: string): string {
3636
return trimmed;
3737
}
3838

39+
// #5831/#7525's path-safety guard, restated locally. The miner package's parsers share
40+
// repo-clone.ts's isValidRepoSegment, but this engine package must not import from the miner package
41+
// (miner depends on engine, not the reverse), so the semantics are duplicated here deliberately:
42+
// a segment must be entirely [A-Za-z0-9._-] and must not be a bare "." or ".." traversal segment.
43+
const REPO_SEGMENT_PATTERN = /^[A-Za-z0-9._-]+$/;
44+
45+
function isValidRepoSegment(segment: string): boolean {
46+
return REPO_SEGMENT_PATTERN.test(segment) && segment !== "." && segment !== "..";
47+
}
48+
3949
function normalizeOptionalRepoFullName(repoFullName: unknown): string | null {
4050
if (repoFullName === undefined || repoFullName === null) return null;
4151
if (typeof repoFullName !== "string") throw new Error("invalid_repo_full_name");
4252
const [owner, repo, extra] = repoFullName.trim().split("/");
4353
if (!owner || !repo || extra !== undefined) throw new Error("invalid_repo_full_name");
54+
// This is the WRITE path (normalizeGovernorLedgerEvent -> appendGovernorEvent's SQLite INSERT). Without
55+
// this, "../evilrepo" normalized unchanged -- owner ".." and repo "evilrepo" both pass the
56+
// non-empty/one-slash check -- and reached persistence, the exact value class #7525 exists to stop.
57+
if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name");
4458
return `${owner}/${repo}`;
4559
}
4660

1.83 KB
Binary file not shown.

scripts/deploy-selfhost-image.sh

Lines changed: 1 addition & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -61,31 +61,6 @@ validate_inputs() {
6161
fi
6262
}
6363

64-
wait_for_healthy() {
65-
local deadline container_id status
66-
67-
deadline=$((SECONDS + HEALTH_TIMEOUT_SECONDS))
68-
while [ "$SECONDS" -le "$deadline" ]; do
69-
container_id="$(docker compose "${compose_args[@]}" ps -q "$SERVICE" 2>/dev/null || true)"
70-
if [ -n "$container_id" ]; then
71-
status="$(docker inspect --format '{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}' "$container_id" 2>/dev/null || true)"
72-
if [ "$status" = "healthy" ]; then
73-
echo "selfhost image deploy: $SERVICE is healthy"
74-
return 0
75-
fi
76-
fi
77-
if [ "$SECONDS" -ge "$deadline" ]; then
78-
break
79-
fi
80-
sleep 2
81-
done
82-
83-
echo "error: $SERVICE did not become healthy within ${HEALTH_TIMEOUT_SECONDS}s" >&2
84-
docker compose "${compose_args[@]}" ps "$SERVICE" >&2 || true
85-
docker compose "${compose_args[@]}" logs --tail=80 "$SERVICE" >&2 || true
86-
exit 1
87-
}
88-
8964
require_cmd docker
9065
docker compose version >/dev/null
9166

@@ -126,7 +101,7 @@ docker compose "${compose_args[@]}" pull --policy always "$SERVICE"
126101
echo "selfhost image deploy: restarting $SERVICE"
127102
maybe_infisical_run docker compose "${compose_args[@]}" up -d --no-build --no-deps "$SERVICE"
128103

129-
wait_for_healthy
104+
wait_for_healthy "$SERVICE" "$HEALTH_TIMEOUT_SECONDS" "selfhost image deploy" "${compose_args[@]}"
130105
env_put LOOPOVER_IMAGE "$IMAGE"
131106

132107
echo "selfhost image deploy: complete ($IMAGE)"

scripts/deploy-selfhost-prebuilt.sh

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@ set -euo pipefail
1616
ENV_FILE="${SELFHOST_ENV_FILE:-.env}"
1717
NODE_IMAGE="${SELFHOST_NODE_IMAGE:-public.ecr.aws/docker/library/node:24-slim}"
1818
SERVICE="${SELFHOST_SERVICE:-loopover}"
19+
# #8395: same override + default deploy-selfhost-image.sh already uses, so both deploy paths honour one
20+
# health-check budget.
21+
HEALTH_TIMEOUT_SECONDS="${SELFHOST_HEALTH_TIMEOUT_SECONDS:-180}"
1922
SKIP_SENTRY_UPLOAD="${SELFHOST_SKIP_SENTRY_UPLOAD:-0}"
2023
SENTRY_CLI_PACKAGE="${SENTRY_CLI_PACKAGE:-@sentry/cli@3.6.0}"
2124

@@ -118,6 +121,13 @@ YAML
118121

119122
echo "selfhost deploy: restarting $SERVICE"
120123
maybe_infisical_run docker compose "${compose_args[@]}" up -d --no-deps "$SERVICE"
124+
125+
# #8395: `up -d` only confirms the container was CREATED and STARTED -- without this, a crash-looping
126+
# or never-healthy image still reported "selfhost deploy: complete". Called here (not at the top level)
127+
# because compose_args is function-local, and it includes the generated override file. Exits non-zero
128+
# with the same ps/logs diagnostics deploy-selfhost-image.sh produces; the EXIT trap above still cleans
129+
# up the temp override file on that path.
130+
wait_for_healthy "$SERVICE" "$HEALTH_TIMEOUT_SECONDS" "selfhost deploy" "${compose_args[@]}"
121131
}
122132

123133
require_cmd docker

scripts/lib/selfhost-deploy-common.sh

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,3 +134,38 @@ compose_file_args() {
134134
printf '%s\n' -f "$file"
135135
done
136136
}
137+
138+
# Block until $service reports healthy, or fail the deploy (#8395). Moved here from
139+
# deploy-selfhost-image.sh so BOTH deploy scripts share one implementation instead of one hand-copying
140+
# the other -- deploy-selfhost-prebuilt.sh previously printed "complete" straight after
141+
# `docker compose up -d`, which only confirms the container STARTED, so a crash-looping image reported
142+
# success. Parameterized rather than reading caller globals: prebuilt's compose_args is a function-local
143+
# array. $log_prefix keeps each script's existing message wording byte-identical.
144+
# Usage: wait_for_healthy <service> <timeout_seconds> <log_prefix> <compose_args...>
145+
wait_for_healthy() {
146+
local service="$1" timeout_seconds="$2" log_prefix="$3"
147+
shift 3
148+
local -a compose_args=("$@")
149+
local deadline container_id status
150+
151+
deadline=$((SECONDS + timeout_seconds))
152+
while [ "$SECONDS" -le "$deadline" ]; do
153+
container_id="$(docker compose "${compose_args[@]}" ps -q "$service" 2>/dev/null || true)"
154+
if [ -n "$container_id" ]; then
155+
status="$(docker inspect --format '{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}' "$container_id" 2>/dev/null || true)"
156+
if [ "$status" = "healthy" ]; then
157+
echo "$log_prefix: $service is healthy"
158+
return 0
159+
fi
160+
fi
161+
if [ "$SECONDS" -ge "$deadline" ]; then
162+
break
163+
fi
164+
sleep 2
165+
done
166+
167+
echo "error: $service did not become healthy within ${timeout_seconds}s" >&2
168+
docker compose "${compose_args[@]}" ps "$service" >&2 || true
169+
docker compose "${compose_args[@]}" logs --tail=80 "$service" >&2 || true
170+
exit 1
171+
}

src/db/migration-column-extraction.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ export function extractSchemaEvents(rawStatement: string): SchemaEvent[] {
163163
const dropTableMatch = /^\s*DROP\s+TABLE\s+(?:IF\s+EXISTS\s+)?(\w+)/i.exec(statement);
164164
if (dropTableMatch) return [{ type: "drop_table", table: dropTableMatch[1]!.toLowerCase() }];
165165

166-
const renameColumnMatch = /\bALTER\s+TABLE\s+(\w+)\s+RENAME\s+COLUMN\s+(\w+)\s+TO\s+(\w+)/i.exec(statement);
166+
const renameColumnMatch = /\bALTER\s+TABLE\s+(\w+)\s+RENAME\s+(?:COLUMN\s+)?(\w+)\s+TO\s+(\w+)/i.exec(statement);
167167
if (renameColumnMatch) {
168168
const table = renameColumnMatch[1]!.toLowerCase();
169169
return [
@@ -172,10 +172,10 @@ export function extractSchemaEvents(rawStatement: string): SchemaEvent[] {
172172
];
173173
}
174174

175-
const dropColumnMatch = /\bALTER\s+TABLE\s+(\w+)\s+DROP\s+COLUMN\s+(\w+)/i.exec(statement);
175+
const dropColumnMatch = /\bALTER\s+TABLE\s+(\w+)\s+DROP\s+(?:COLUMN\s+)?(\w+)/i.exec(statement);
176176
if (dropColumnMatch) return [{ type: "remove_column", table: dropColumnMatch[1]!.toLowerCase(), column: dropColumnMatch[2]!.toLowerCase() }];
177177

178-
const addColumnMatch = /\bALTER\s+TABLE\s+(\w+)\s+ADD\s+COLUMN\s+(\w+)/i.exec(statement);
178+
const addColumnMatch = /\bALTER\s+TABLE\s+(\w+)\s+ADD\s+(?:COLUMN\s+)?(\w+)/i.exec(statement);
179179
if (addColumnMatch) return [{ type: "define_column", table: addColumnMatch[1]!.toLowerCase(), column: addColumnMatch[2]!.toLowerCase() }];
180180

181181
const createTableMatch = /\bCREATE\s+TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?(\w+)\s*\(([\s\S]*)\)[^)]*$/i.exec(statement);

src/db/retention.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,10 @@ import { nowIso } from "../utils/json";
22

33
/**
44
* Data-retention policy for the high-volume, append-only / log / superseded-snapshot tables. These hold
5-
* pure history (logs, usage metrics, ephemeral observations) or snapshots where only the latest matters,
6-
* so rows older than the window can be safely deleted. Current-state and reference tables (repositories,
7-
* repository_settings, pull_requests, issues, contributors, registry/scoring snapshots, repository_ai_keys,
8-
* focus manifests, webhook delivery idempotency records, etc.) are intentionally EXCLUDED — they are not append-only logs.
5+
* pure history (logs, usage metrics, ephemeral observations, webhook delivery traces) or snapshots where
6+
* only the latest matters, so rows older than the window can be safely deleted. Current-state and reference
7+
* tables (repositories, repository_settings, pull_requests, issues, contributors, registry/scoring snapshots,
8+
* repository_ai_keys, focus manifests, etc.) are intentionally EXCLUDED — they are not append-only logs.
99
*
1010
* `column` is the row's primary timestamp (ISO-8601). Windows are deliberately conservative.
1111
*/
@@ -24,6 +24,9 @@ export const RETENTION_POLICY: readonly RetentionRule[] = [
2424
// One payloadJson blob per agent run (#3896); a per-run diagnostic snapshot with no cross-run rollup
2525
// depending on it, so a shorter window than the audit/usage-log tables above is appropriate.
2626
{ table: "agent_context_snapshots", column: "created_at", days: 30 },
27+
// One row per inbound webhook delivery (#8381 / unfinished #3896); short-lived idempotency lookups,
28+
// not durable history — same 90d window as audit/ai_usage logs.
29+
{ table: "webhook_events", column: "received_at", days: 90 },
2730
];
2831

2932
export type PruneResult = { table: string; column: string; cutoff: string; deleted: number };

src/orb/relay.ts

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -106,14 +106,30 @@ async function finalizeRelayFailureRetryRow(
106106
row: { delivery_id: string; event_name: string; installation_id: number },
107107
outcome: RelayForwardOutcome,
108108
): Promise<void> {
109-
if (isRelayFailureRetryTerminal(outcome, row.event_name)) {
110-
await env.DB.prepare("DELETE FROM orb_relay_failures WHERE delivery_id = ?").bind(row.delivery_id).run();
109+
try {
110+
if (isRelayFailureRetryTerminal(outcome, row.event_name)) {
111+
await env.DB.prepare("DELETE FROM orb_relay_failures WHERE delivery_id = ?").bind(row.delivery_id).run();
112+
return;
113+
}
114+
await env.DB
115+
.prepare("UPDATE orb_relay_failures SET attempts = attempts + 1, last_attempt_at = datetime('now') WHERE delivery_id = ?")
116+
.bind(row.delivery_id)
117+
.run();
118+
} catch (error) {
119+
// The forward already succeeded (outcome is known) before this write failed -- the row stays pending retry, so
120+
// the SAME event risks redelivery to the container on the next retry tick. Never throw (retryFailedRelays's
121+
// "Never throws" contract): log with enough context to spot the duplicate-forward risk from the log alone.
122+
console.error(JSON.stringify({
123+
level: "error",
124+
event: "orb_relay_failure_finalize_write_failed",
125+
message: `finalizeRelayFailureRetryRow DB write failed after a successful forward -- duplicate redelivery risk for ${row.delivery_id}`,
126+
deliveryId: row.delivery_id,
127+
eventName: row.event_name,
128+
outcome,
129+
error: error instanceof Error ? error.message : String(error),
130+
}));
111131
return;
112132
}
113-
await env.DB
114-
.prepare("UPDATE orb_relay_failures SET attempts = attempts + 1, last_attempt_at = datetime('now') WHERE delivery_id = ?")
115-
.bind(row.delivery_id)
116-
.run();
117133
if (outcome === "skipped") {
118134
logRelayTransientSkip({
119135
deliveryId: row.delivery_id,

0 commit comments

Comments
 (0)