Compare commits

..
Author SHA1 Message Date
taodengandClaude Opus 5 79656b485f ci: drop the alignment.yml paths change — it would have been a green check for a check that never ran
Reverting my own half of this PR. The reviewer proved the alignment.yml change
was theatre, and my issue #198 was filed on a wrong premise.

What I verified myself before reverting:
  - the alignment job BODY references models.json zero times; only the paths
    filter I added mentioned it
  - test.yml is `pull_request:` with NO paths filter, so it already runs on
    every PR, models.json-only ones included
  - on a deliberately corrupted models.json (contextWindow 1000000, a typo'd
    openclawName), `npm test` catches it: 455 passed, 2 failed

So the real guard on models.json already existed and always ran. Adding the
path would only have made a job execute that inspects nothing about
models.json, and then reported a green "Alignment Guardrail" — implying a check
that did not happen. That is strictly worse than the job not running, which is
precisely the reviewer's point.

#198's premise was also wrong. The blacklist greps server.mjs for known
hallucinated tokens. A models.json-only PR cannot introduce a token into
server.mjs, so CLAUDE.md hard-requirement #2 is INAPPLICABLE on such a PR, not
vacuous. I read "the guardrail didn't run" as "coverage gap" without checking
what the guardrail actually inspects.

The release.yml half is unaffected and stays: that one is a real, reproduced
failure (the no-CHANGELOG branch never wrote the file the create step consumes).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8
2026-07-27 09:05:33 +10:00
taodengandClaude Opus 5 66096cdcab ci: cover models.json in the alignment guardrail; fix release.yml's no-CHANGELOG path
Two CI-layer fixes found during the v3.25.0 review. Same layer, both small, so
they land together (Iron Rule 11).

#198 — alignment.yml did not run on a models.json-only PR.
The `paths:` filter listed server.mjs / setup.mjs / scripts / lib / ocp /
ocp-connect, but not models.json. That made CLAUDE.md hard-requirement #2 ("CI
blacklist pass") vacuous for exactly the PRs that change model routing:
confirmed on #192, where only gitleaks and test-features ran.

models.json is not inert data. It drives MODEL_MAP and VALID_MODELS, the
default request model, and — since ADR 0009 — the global MAX_PROMPT_CHARS
truncation budget. A models.json-only PR can therefore change server.mjs's
runtime behavior with the guardrail never running. models.schema.json is
included for the same reason one level up: loosening the schema is what would
let a malformed models.json through.

Adding paths only widens coverage; the blacklist grep still scans server.mjs,
so this costs nothing and closes the gap. Per CLAUDE.md this is a PR amendment
to alignment.yml, which is the sanctioned way to change it (removing entries
would need an ALIGNMENT.md amendment; nothing is removed here).

#202 — release.yml's no-CHANGELOG fallback would have failed the release job.
The branch wrote an output named `notes` and exited WITHOUT creating
/tmp/release-notes.md, while the create step hard-codes
`--notes-file /tmp/release-notes.md`. So the one path that exists to "degrade
to minimal notes" instead produced a failed release.

Verified both directions by extracting the step's actual script from the YAML
and running it, rather than reading it:

  PRE-FIX,  no CHANGELOG -> exit 0, GITHUB_OUTPUT="notes=Release v3.25.0",
                            /tmp/release-notes.md MISSING
                            -> gh release create --notes-file WOULD FAIL
  POST-FIX, no CHANGELOG -> exit 0, notes_file set, file present ("Release v3.25.0")
  POST-FIX, with CHANGELOG -> file present, first line "## v3.25.0 — 2026-07-27"

Fixed by writing the file in that branch, and by removing the hard-coded-path
coupling entirely: both branches now set `notes_file` and the create step
consumes `steps.notes.outputs.notes_file`. That output existed already and was
never read — the two only agreed by accident.

Also added `set -euo pipefail` (the step previously ran unset-tolerant) and an
echo of the resolved notes before creating the release, so a wrong-looking body
is visible in the job log instead of only on the published release.

NOT changed: the empty-extraction guard. My issue text suggested adding one,
but `if [ ! -s "$NOTES" ]` was already there and already handles a heading
mismatch. Correcting that claim on #202 rather than taking credit for it.

Both workflows re-validated as YAML.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8
2026-07-27 08:51:09 +10:00
3 changed files with 13 additions and 34 deletions
+13 -6
View File
@@ -21,24 +21,31 @@ jobs:
- name: Extract CHANGELOG section
id: notes
run: |
set -euo pipefail
VERSION="${{ steps.ver.outputs.version }}"
NOTES=/tmp/release-notes.md
# Extract section for this version from CHANGELOG.md
# Pattern: "## v${VERSION}" through the next "## " or EOF
if [ ! -f CHANGELOG.md ]; then
echo "No CHANGELOG.md found; using minimal release notes"
echo "notes=Release v${VERSION}" >> $GITHUB_OUTPUT
# MUST write the file, not just an output: the create step consumes a FILE, so an
# early exit here used to leave --notes-file pointing at a path that never existed,
# turning "degrade to minimal notes" into a failed release job (#202).
echo "Release v${VERSION}" > "$NOTES"
echo "notes_file=$NOTES" >> $GITHUB_OUTPUT
exit 0
fi
awk -v ver="v${VERSION}" '
$0 ~ "^## " ver { found=1; print; next }
found && /^## v/ { exit }
found { print }
' CHANGELOG.md > /tmp/release-notes.md
if [ ! -s /tmp/release-notes.md ]; then
' CHANGELOG.md > "$NOTES"
if [ ! -s "$NOTES" ]; then
echo "No matching section in CHANGELOG for v${VERSION}; using minimal notes"
echo "Release v${VERSION}" > /tmp/release-notes.md
echo "Release v${VERSION}" > "$NOTES"
fi
echo "notes_file=/tmp/release-notes.md" >> $GITHUB_OUTPUT
echo "--- release notes (${VERSION}) ---"; cat "$NOTES"
echo "notes_file=$NOTES" >> $GITHUB_OUTPUT
- name: Create GitHub Release
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
@@ -49,5 +56,5 @@ jobs:
fi
gh release create "v${{ steps.ver.outputs.version }}" \
--title "v${{ steps.ver.outputs.version }}" \
--notes-file /tmp/release-notes.md \
--notes-file "${{ steps.notes.outputs.notes_file }}" \
--latest
-16
View File
@@ -52,22 +52,6 @@ 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.
-12
View File
@@ -2923,16 +2923,6 @@ 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 {
@@ -2952,8 +2942,6 @@ 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;