Compare commits

..
Author SHA1 Message Date
taodengandClaude Opus 5 4759639d9b docs: reference only helpers that exist on main (AGENTS.md)
Review caught a real forward-reference: the new AGENTS.md section named
`ltDiag` and told readers to wait on `closed`, and NEITHER exists on main —
both are introduced by the still-unmerged #204. `git grep ltDiag` matched
exactly one line: the documentation itself.

That is worse than a broken link. AGENTS.md is, by its own "Handoff
expectations", the FIRST file a fresh session reads; it would have sent that
session looking for two identifiers it cannot find, in the file whose whole job
is to orient them.

It also meant this PR had an undeclared merge-order dependency on #204, which
I had not stated anywhere.

Fixed so each PR is self-consistent regardless of merge order:
  - names `ltPostStatus` (which does exist) instead of `ltDiag`
  - states the #203 rule as a PRINCIPLE — a terminated child's pipes may still
    hold unread data, so wait for stdio to close rather than for the exit —
    instead of naming the `closed` flag that #204 adds

Verified: every helper the section now names resolves to a `function <name>` in
test-features.mjs on this branch, and neither `ltDiag` nor `closed` is
referenced any more.

Tests: 457 passed, 0 failed (comments/docs only, unchanged).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8
2026-07-27 09:06:58 +10:00
taodengandClaude Opus 5 868a1953f4 docs: guard the asymmetric cache keys (#200); document the ltBoot fault lever (#197)
Two knowledge-preservation fixes. No behavior change: server.mjs gains comments
only, verified by `git diff --stat` (comment lines) and an unchanged suite.

#200 — structuredHash and dedupKey compute the same value and must NOT be
collapsed. They take identical arguments and read as obvious duplicate work,
which is how someone "optimises" them into a bug. Their GUARDS differ, verified
line by line rather than asserted:

  structuredHash: if (CACHE_TTL > 0 && !conversationId && !hasCacheControl(...))
  dedupKey:       (!conversationId && !hasCacheControl(...))          <- no CACHE_TTL

CLAUDE_CACHE_TTL defaults to 0, so in the DEFAULT configuration structuredHash
is null while dedupKey must still be computed — it drives #153's single-flight
stampede protection, which is deliberately independent of response caching.
`dedupKey = structuredHash` would silently disable stampede protection by
default, in exactly the concurrent-AI-Task case it exists to bound. The comment
records that, and says what a safe deduplication would look like if anyone
still wants one (compute under the weaker guard, derive under the stronger,
add a stampede test first).

This was first raised in the #194 review as a cosmetic nit and then RETRACTED
by that reviewer after reading the guards — the retraction is the valuable
part, so it goes in the code rather than staying in a PR thread.

#197 — new "Testing: reaching faults inside server.mjs" section in AGENTS.md.
The premise the issue was originally filed on was wrong twice over, and both
errors were mine: first that no live-server harness existed (ltBoot has been
there all along, and I had used it myself in #191), then that a synchronous
fault deep inside spawnClaudeProcess needed a production test hook (it needs
--stack-size, which ltBoot already forwards).

The section documents the fixture, the --stack-size fault lever with the
concrete numbers (~124k element spread threshold at the default stack vs ~24k
at --stack-size=200, which is what fits Linux's 131072-byte MAX_ARG_STRLEN so
the test RUNS in CI instead of skipping), and the three rules that made it hold:
discover the threshold in a child under the same flags, assert the fault
actually fired, and wait for the thing you are about to assert rather than a
proxy for it. Each rule cites the issue that taught it (#193/#199/#203).

Written to be read BEFORE someone concludes a bug class is untestable, since
that conclusion has now been reached wrongly twice in this repo.

cli.js citation: NOT APPLICABLE — comments only, no wire behavior, no code
path, no argv change. Authorized by ADR 0006 as a behaviour-preserving edit;
the field set and every request/response contract are byte-identical.

Tests: 457 passed, 0 failed (unchanged, as expected for a comments-only diff).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8
2026-07-27 08:53:30 +10:00
5 changed files with 29 additions and 128 deletions
+16 -1
View File
@@ -30,7 +30,6 @@ Runtime: Node.js (ESM, `.mjs` throughout). No build step. No bundler. `server.mj
- `server.mjs` — the proxy itself; every request path lives here. Governed by `ALIGNMENT.md`.
- `models.json` — single source of truth for model IDs, aliases, and context windows. See ADR 0003.
- `models.schema.json` — the schema `models.json` declares in its `$schema`. CI validates the SPOT against it (`test-features.mjs`) using the repo's own `validateJsonSchema`, so a malformed entry fails the build instead of surfacing downstream in OpenClaw.
- `setup.mjs` — first-time installer; reads `models.json` to derive bootstrap config.
- `scripts/sync-openclaw.mjs` — idempotent OpenClaw registry sync invoked by `ocp update`. See ADR 0004.
- `ocp` — user-facing CLI (install, update, start, stop, status, logs, etc.).
@@ -53,6 +52,22 @@ Runtime: Node.js (ESM, `.mjs` throughout). No build step. No bundler. `server.mj
---
## Testing: reaching faults inside `server.mjs`
`test-features.mjs` cannot `import` `server.mjs` (it calls `server.listen()` at top level), and that has twice led to the wrong conclusion that a class of bug is untestable. It isn't. Read this before writing "no regression test is possible here".
**There is a real live-server fixture.** `ltBoot(env, dir, nodeArgs)` (around `test-features.mjs:990`) spawns the actual `server.mjs` as a child with a **fake `claude` binary**, so integration tests cost no quota. `ltPost` / `ltPostStatus` / `ltWait` / `ltFreePort` round it out. It already covers boot gates, cache-epoch invalidation across two boots sharing one SQLite store, and system-prompt capture.
**`--stack-size` is a fault lever.** `ltBoot`'s third argument passes V8 flags to the child, which puts recursion- and argument-count-limited failures in reach at a much smaller input. `#193` needed a *synchronous throw* deep inside `spawnClaudeProcess`; `buildCliArgs` does `args.push("--allowedTools", ...ALLOWED_TOOLS)`, and under `--stack-size=200` that spread throws at ~24k elements instead of ~124k — which is what brings the trigger under Linux's `MAX_ARG_STRLEN` (131072 bytes for a single env string) so the test runs in CI rather than skipping. **No production fault hook was needed.**
Three rules that made it hold up, all learned the hard way:
- **Discover the threshold in a child under the same flags**, never in the test process — the parent's stack is not the one that matters.
- **Assert that the fault actually fired**, not just that the outcome looks right. `#193` asserts HTTP 500 *and* that the body carries `call stack size exceeded`; a control mutation (trigger neutered, bug still present) proves the test fails rather than passing vacuously.
- **Wait for the thing you are about to assert**, not a proxy for it. Waiting on `listening on` and then asserting a different line is a race (`#199`); waiting for the process to *exit* and then reading its `stderr` is another, because a terminated child's pipes may still hold unread data (`#203` — wait for the stdio to close, not for the exit).
Allocate ports with `ltFreePort()`. Fixed ports have caused at least one flake here.
## Release protocol
OCP follows the machine-readable `release_kit:` overlay in `CLAUDE.md` (Iron Rule 5.5). Before any version bump or tag push, re-read that YAML block and walk every item in `new_feature_doc_expectations` and `bootstrap_quirk_policy`. Tag push triggers `.github/workflows/release.yml`, which creates the GitHub Release automatically — do not create the release manually.
+1 -1
View File
@@ -177,7 +177,7 @@ Any tool use happens server-side, under the `--allowedTools` set configured on t
| `claude-sonnet-4-6` | Previous Sonnet, retained for pinning |
| `claude-haiku-4-5-20251001` | Fastest, lightweight (default for `haiku` alias) |
The canonical list lives in [`models.json`](./models.json) — the single source of truth as of v3.11.0, validated in CI against [`models.schema.json`](./models.schema.json). Both `server.mjs` (the `/v1/models` endpoint) and `setup.mjs` (the OpenClaw registration) derive from it. Adding a new model is now a one-file edit:
The canonical list lives in [`models.json`](./models.json) — the single source of truth as of v3.11.0. Both `server.mjs` (the `/v1/models` endpoint) and `setup.mjs` (the OpenClaw registration) derive from it. Adding a new model is now a one-file edit:
```bash
# 1. Edit models.json — add an entry
-65
View File
@@ -1,65 +0,0 @@
{
"$schema": "https://json-schema.org/draft/2020-12/schema",
"$id": "https://github.com/dtzp555-max/ocp/blob/main/models.schema.json",
"title": "OCP models.json",
"description": "Schema for models.json, the single source of truth for model metadata (ADR 0003). Enforced in CI by test-features.mjs, which validates models.json against this file using the repo's own validateJsonSchema (lib/structured-output.mjs) — no new dependency. ⚠ ONLY A SUBSET OF DRAFT 2020-12 IS ENFORCED: type, required, const, enum, additionalProperties (boolean and schema forms), items, minItems, maxItems, anyOf/allOf/oneOf, $ref, nullable. Anything else — minimum, maxLength, pattern, uniqueItems, propertyNames, not — is SILENTLY IGNORED by that validator, so adding one here buys nothing and misleads the next reader. Add such a constraint as a test in test-features.mjs instead. NOTE: referential integrity (every aliases/legacyAliases target must exist as a models[].id) is likewise not expressible here and is covered by separate tests.",
"type": "object",
"required": ["version", "models", "aliases", "legacyAliases"],
"additionalProperties": false,
"properties": {
"$schema": {
"type": "string",
"description": "Relative path to this file. Editors use it for completion; CI does not read it."
},
"version": {
"const": 1,
"description": "Schema version of this document. Bump only alongside a shape change and an ADR."
},
"models": {
"type": "array",
"minItems": 1,
"description": "Every model OCP advertises on /v1/models, newest first. Order is load-bearing: ocp-connect uses model_ids[0] as a primary fallback.",
"items": {
"type": "object",
"required": ["id", "displayName", "openclawName", "reasoning", "contextWindow", "maxTokens"],
"additionalProperties": false,
"properties": {
"id": {
"type": "string",
"description": "Canonical CLI model id, passed verbatim to `claude --model`. Must match the id in the compiled CLI registry."
},
"displayName": {
"type": "string",
"description": "Human label. Also used as the OpenClaw agent alias by scripts/sync-openclaw.mjs."
},
"openclawName": {
"type": "string",
"description": "Label written into OpenClaw's model registry."
},
"reasoning": {
"type": "boolean",
"description": "Whether OpenClaw should treat the model as reasoning-capable."
},
"contextWindow": {
"type": "integer",
"description": "Advertised context window. GLOBAL side effect: MAX_PROMPT_CHARS derives from max(contextWindow) x 3 across ALL entries (lib/prompt.mjs derivePromptCharBudget), so raising this on ONE model raises the truncation ceiling for EVERY model. See ADR 0009. Also feeds OpenClaw's compaction budget."
},
"maxTokens": {
"type": "integer",
"description": "Advertised output cap. OCP does not enforce it; it is propagated to OpenClaw (via setup.mjs / scripts/sync-openclaw.mjs) where it bounds request and compaction budgets."
}
}
}
},
"aliases": {
"type": "object",
"description": "Short name -> canonical models[].id. Client-addressable, so a repoint changes routing for every request using the alias. Cache keys resolve these before hashing (server.mjs cacheModel).",
"additionalProperties": { "type": "string" }
},
"legacyAliases": {
"type": "object",
"description": "Retired ids kept resolvable for backward compatibility -> canonical models[].id. Also client-addressable and also resolved in cache keys.",
"additionalProperties": { "type": "string" }
}
}
}
+12
View File
@@ -2923,6 +2923,16 @@ async function handleChatCompletions(req, res) {
const t0s = Date.now();
const promptCharsS = messages.reduce((a, m) => a + contentToText(m.content).length, 0);
let structuredHash = null;
// DO NOT collapse this with `dedupKey` below (#200). The two cacheHash calls take IDENTICAL
// arguments and look like obvious duplicate work — they are not interchangeable, because
// their GUARDS differ: this one additionally requires CACHE_TTL > 0. CLAUDE_CACHE_TTL
// DEFAULTS TO 0, so in the default configuration structuredHash is null while dedupKey must
// still be computed — it drives #153's single-flight stampede protection, which is
// deliberately independent of whether response caching is on. `dedupKey = structuredHash`
// would therefore silently disable stampede protection by default, in exactly the
// concurrent-AI-Task case it exists to bound. The duplicate call is the honest price of the
// asymmetry. If you do deduplicate it, compute once under the WEAKER guard and derive the
// cache lookup under the stronger one — and add a stampede test before you do.
if (CACHE_TTL > 0 && !conversationId && !hasCacheControl(messages)) {
structuredHash = cacheHash(cacheModel, messages, { keyId: req._authKeyId, temperature: parsed.temperature, max_tokens: parsed.max_tokens, top_p: parsed.top_p, structured, configEpoch: CONFIG_EPOCH });
try {
@@ -2942,6 +2952,8 @@ async function handleChatCompletions(req, res) {
// firing several AI Tasks at once) must NOT each pay N× — they share one flight. We dedup every
// one-off structured request (not stateful sessions / client-side prompt caching), independent of
// whether OCP response caching is enabled; when caching IS on, the same key gates cache read/write.
// Note the guard here is deliberately WEAKER than structuredHash's — no CACHE_TTL check. See the
// do-not-collapse comment above (#200).
const dedupKey = (!conversationId && !hasCacheControl(messages))
? cacheHash(cacheModel, messages, { keyId: req._authKeyId, temperature: parsed.temperature, max_tokens: parsed.max_tokens, top_p: parsed.top_p, structured, configEpoch: CONFIG_EPOCH })
: null;
-61
View File
@@ -4305,67 +4305,6 @@ test("models.json: every legacyAliases value resolves to a real models[].id (ref
}
});
// ── models.json validates against models.schema.json (#196) ─────────────────
// models.json carried `"$schema": "./models.schema.json"` while that file had never been
// committed, so the SPOT that ADR 0003 makes canonical had no structural validation at all —
// a missing contextWindow or a typo'd openclawName would only surface downstream, in OpenClaw
// or in a truncation budget. Validated with the repo's OWN validator (lib/structured-output.mjs,
// shipped for #153) rather than a new dependency: zero deps added, and it exercises that
// validator on a second real input.
//
// The schema deliberately does NOT try to express referential integrity (alias -> models[].id);
// that is not a JSON Schema concept and is covered by the two tests directly above.
import { validateJsonSchema as _spotValidate } from "./lib/structured-output.mjs";
const _spotSchema = JSON.parse(spotReadFileSync(spotJoin(_spotDir, "models.schema.json"), "utf8"));
test("models.json: the $schema reference resolves to a committed file", () => {
assert.equal(_spotModels.$schema, "./models.schema.json", "models.json must point at the schema");
assert.ok(_ltExists(spotJoin(_spotDir, "models.schema.json")),
"models.schema.json must exist — a dangling $schema is what #196 was filed for");
});
test("models.json validates against models.schema.json (strict)", () => {
const errors = _spotValidate(_spotModels, _spotSchema, "$", true);
assert.deepEqual(errors, [], `models.json violates its own schema:\n ${errors.join("\n ")}`);
});
// Three corruptions the SCHEMA structurally cannot catch, asserted directly instead of pretending
// the schema covers them: the validator has no uniqueItems, no minLength, and no minimum. Adding
// those keywords to the schema would be silently ignored (see its description), so they live here.
test("models.json: ids are unique, non-empty, and windows are positive (not schema-expressible)", () => {
const ids = _spotModels.models.map(m => m.id);
assert.equal(new Set(ids).size, ids.length, `duplicate models[].id: ${ids.filter((v, i) => ids.indexOf(v) !== i)}`);
for (const m of _spotModels.models) {
for (const f of ["id", "displayName", "openclawName"]) {
assert.ok(typeof m[f] === "string" && m[f].trim().length > 0, `${m.id}: ${f} must be a non-empty string`);
}
assert.ok(m.contextWindow > 0, `${m.id}: contextWindow must be positive`);
assert.ok(m.maxTokens > 0, `${m.id}: maxTokens must be positive`);
}
});
// Guards the guard: if the schema were vacuous (e.g. `{}` or a typo'd `properties`), the test
// above would pass on anything. Each corruption below must be caught.
test("models.schema.json actually rejects malformed entries (guard is not vacuous)", () => {
const clone = () => JSON.parse(JSON.stringify(_spotModels));
const cases = [
["missing required field", m => { delete m.models[0].contextWindow; }],
["wrong scalar type", m => { m.models[0].reasoning = "yes"; }],
["non-integer window", m => { m.models[0].contextWindow = 200000.5; }],
["unknown extra field", m => { m.models[0].tokensPerSecond = 42; }],
["alias mapped to non-string", m => { m.aliases.opus = { id: "x" }; }],
["empty models array", m => { m.models = []; }],
["wrong document version", m => { m.version = 2; }],
["unknown top-level key", m => { m.providers = {}; }],
];
for (const [label, corrupt] of cases) {
const bad = clone(); corrupt(bad);
const errs = _spotValidate(bad, _spotSchema, "$", true);
assert.ok(errs.length > 0, `schema failed to reject: ${label}`);
}
});
// ── escapeHtml + key-name validator (issue #114) ────────────────────────────
// Replicated verbatim from dashboard.html so tests run without a browser.
function escapeHtml(s) {