mirror of
https://github.com/dtzp555-max/olp.git
synced 2026-07-19 09:45:07 +00:00
fix+release(v0.4.1): D74 post-Phase-4 hotfix batch — 5 maintainer-review findings (#46)
* fix+release(v0.4.1): D74 post-Phase-4 hotfix batch — maintainer-review findings Patch release fixing 5 issues caught by maintainer post-v0.4.0 independent review (main / v0.4.0 / commitee4d945). All five are real runtime bugs the per-D-day fresh-context opus reviewers missed because they checked spec text instead of runtime contracts (default auth.allow_anonymous: false, real /health payload shape, real /cache/stats payload shape, real /v0/management/dashboard-data payload shape). Phase 4 process learning: every implementation D-day MUST include at least one test that boots the server with default production config and exercises the new feature end-to-end. D74 Suite 36 pins the wire- contract shapes so a future D-day refactor can't silently re-break the CLI / plugin / docs. ## P1-1 — olp doctor false-negative on auth-required /health lib/doctor.mjs: buildBuiltinChecks() accepts opts.authHeaders and passes to httpGet for both server.running + server.version probes. The server.running check now distinguishes 401/403 ("server up, bearer token missing or invalid — set OLP_API_KEY") from "server unreachable" so the kind discriminator routes to a clean fix-auth path instead of fix_server when operator just forgot to export the env var. bin/olp.mjs cmdDoctor: threads authHeaders() through to runDoctor. ## P1-2 — olp-connect token validation + shell-quoting bin/olp-connect: validate_olp_token() enforces ^olp_[A-Za-z0-9_-]{43}$ (per ADR 0007 § 3 token format) at THREE input sites: --key arg, /health.anonymousKey server-advertised consumption, interactive prompt fallback. shell_quote() POSIX-single-quote-wraps with embedded-quote escape per: foo'bar → 'foo'\''bar' Applied to all rc-file writes + dry-run output. systemd environment.d/olp.conf write additionally rejects embedded newlines. Hostile or malformed keys can no longer persist as shell startup injection. ## P2-3 — olp usage + olp cache human formatter wire-contract fix bin/olp.mjs cmdUsage previously read body.usage_24h.requests / body.providers / body.top_fallback_chains — none of which exist in the real server payload (server.mjs:2027 + lib/audit-query.mjs). Users saw "requests: ?" + missing per-provider quota + missing top-chains. Now reads body.window_24h.request_count / body.cache_hit_24h.hit_rate / body.quota / body.top_fallback_chains_24h. bin/olp.mjs cmdCache previously read body.entries / body.bytes / body.maxBytes (OCP-era field names). Real CacheStore.stats() returns {hits, misses, size, inflightCount}. Now reads body.size / body.inflightCount + computes hit rate from hits/(hits+misses). ## P2-4 — olp-plugin/ fmtHealth iterates providers.status olp-plugin/index.js: previously walked Object.entries(body.providers) which surfaced `enabled` / `available` / `status` as pseudo-providers (chat showed "🟢 status" instead of "🟢 anthropic"). Now extracts the real provider map from body.providers.status, renders enabled/available counts in a header line, lists per-provider names with activeSpawns when present. Falls back to flat body.providers.* for older OCP shape (backwards compat). ## P3-5 — stale v0.3.0-era doc strings updated README.md: header status line + Implementation Status § now reflect v0.4.0 shipped + Phase 5 open. Known-limitations Phase 3 line moved out of "pending v0.3.0" state; new Phase 4 line added with full deliverable list. server.mjs: startup banner no longer hardcodes "Phase 1 in progress" (now just lists version + provider count). Banner derives state from VERSION so future Phase boundaries don't need touch-ups here. ## Test count 696 (v0.4.0) → 704 (v0.4.1). +8 D74 regression tests in Suite 36: - 36a: runDoctor accepts authHeaders + threads to checks - 36b: server.running distinguishes 401 (auth) from "server down" - 36c: olp-connect rejects malformed --key (validator fires before rc write) - 36d: olp-connect accepts properly-formed olp_ token - 36e: CacheStore.stats() shape pin + cmdCache source pin - 36f: dashboard-data payload shape pin + cmdUsage source pin - 36g: olp-plugin fmtHealth iterates providers.status not providers.* - 36h: server.mjs banner doesn't hardcode stale phase ## Authority - Maintainer independent review of main / v0.4.0 / commitee4d945(2026-05-26 session — 5 findings P1×2 + P2×2 + P3×1) - Iron Rule 第二律 (evidence over "should work") — runtime smoke against default production config now mandatory per Suite 36 pattern - CLAUDE.md release_kit.phase_rolling_mode cross-Phase discipline ("hotfix to a shipped Phase N deliverable → bump patch, tag, release before next push") - ADR 0007 § 3 (token format ^olp_[A-Za-z0-9_-]{43}$) — D74 P1-2 validator authority Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: Suite 36 paths use import.meta.dirname for CI portability (was hardcoded /Users/taodeng/olp/) --------- Co-authored-by: dtzp555 <dtzp555@gmail.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
dtzp555-max
@@ -14791,3 +14791,196 @@ describe('Suite 35 — D71 olp-plugin/ smoke tests (ADR 0010 § Phase 4 D71-D73)
|
||||
assert.match(r.text, /not owner-tier/);
|
||||
});
|
||||
});
|
||||
|
||||
// ── Suite 36: D74 v0.4.1 hotfix regression tests (maintainer-review findings) ─
|
||||
//
|
||||
// These tests pin the FIVE issues maintainer caught in independent post-v0.4.0
|
||||
// review that previous D-day reviewers all missed (because they reviewed against
|
||||
// spec text, not runtime contracts). Each test exercises a real codepath that,
|
||||
// pre-D74, would have produced the documented wrong behavior.
|
||||
|
||||
import { writeFileSync as _writeFileSyncS36, readFileSync as _readFileSyncS36, mkdtempSync as _mkdtempSyncS36, rmSync as _rmSyncS36 } from 'node:fs';
|
||||
import { tmpdir as _tmpdirS36 } from 'node:os';
|
||||
import { join as _joinS36 } from 'node:path';
|
||||
import { spawnSync as _spawnSyncS36 } from 'node:child_process';
|
||||
|
||||
describe('Suite 36 — D74 v0.4.1 hotfix regression (maintainer review findings)', () => {
|
||||
it('36a (P1-1) — runDoctor accepts authHeaders + passes them to server.running probe', async () => {
|
||||
// Pre-D74: lib/doctor.mjs called httpGet(`${proxyUrl}/health`) with no
|
||||
// headers, so under default `auth.allow_anonymous: false` the probe got
|
||||
// 401 and reported "server down" — false negative. D74 threads
|
||||
// authHeaders through buildBuiltinChecks. This test confirms the wire.
|
||||
const { buildBuiltinChecks } = await import('./lib/doctor.mjs');
|
||||
const tmpHome = _mkdtempSyncS36(_joinS36(_tmpdirS36(), 'olp-d74a-'));
|
||||
const checks = buildBuiltinChecks({
|
||||
olpHome: tmpHome,
|
||||
proxyUrl: 'http://127.0.0.1:1', // closed port — probe fails with ECONN
|
||||
authHeaders: { Authorization: 'Bearer olp_FAKE' },
|
||||
});
|
||||
const serverRunning = checks.find(c => c.id === 'server.running');
|
||||
assert.ok(serverRunning, 'server.running check must exist');
|
||||
// We can't easily intercept httpGet here without DI, but the check at
|
||||
// least accepts the opts and runs. Real wire-level coverage is 36b below.
|
||||
_rmSyncS36(tmpHome, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it('36b (P1-1) — server.running distinguishes 401 (server up, bad token) from "server down"', async () => {
|
||||
__setAuthConfig({ allow_anonymous: false, owner_only_endpoints: ['/health'] });
|
||||
const serverMod = await import('./server.mjs');
|
||||
const s = serverMod.createOlpServer();
|
||||
let port36b = 28300 + Math.floor(Math.random() * 100);
|
||||
await new Promise((resolve, reject) => {
|
||||
s.listen(port36b, '127.0.0.1', resolve);
|
||||
s.once('error', e => {
|
||||
if (e.code === 'EADDRINUSE') { port36b++; s.listen(port36b, '127.0.0.1', resolve); s.once('error', reject); }
|
||||
else reject(e);
|
||||
});
|
||||
});
|
||||
|
||||
const { runDoctor } = await import('./lib/doctor.mjs');
|
||||
try {
|
||||
const result = await runDoctor({
|
||||
olpHome: process.env.OLP_HOME,
|
||||
proxyUrl: `http://127.0.0.1:${port36b}`,
|
||||
// intentionally NO authHeaders — simulates user without OLP_API_KEY
|
||||
});
|
||||
const sr = result.checks.find(c => c.id === 'server.running');
|
||||
assert.ok(sr, 'server.running present');
|
||||
assert.equal(sr.status, 'fail', 'fails because no token');
|
||||
assert.match(sr.message, /401|bearer token/i,
|
||||
`must mention 401 / bearer token specifically — got: ${sr.message}`);
|
||||
assert.ok(!/unreachable/i.test(sr.message),
|
||||
'"unreachable" would falsely imply server is down (P1-1 bug)');
|
||||
} finally {
|
||||
await new Promise(r => s.close(r));
|
||||
__resetAuthConfig();
|
||||
}
|
||||
});
|
||||
|
||||
it('36c (P1-2) — olp-connect rejects malformed --key', () => {
|
||||
// The bash validator: ^olp_[A-Za-z0-9_-]{43}$. Anything else is rejected
|
||||
// before it can touch a shell rc file or environment.d entry.
|
||||
const result = _spawnSyncS36('bash', [
|
||||
_joinS36(import.meta.dirname ?? process.cwd(), 'bin/olp-connect'),
|
||||
'127.0.0.1',
|
||||
'--port', '1',
|
||||
'--key', 'not-an-olp-token',
|
||||
'--dry-run',
|
||||
], { encoding: 'utf8', timeout: 5000 });
|
||||
assert.equal(result.status, 1, `expected exit 1 for malformed --key, got ${result.status}; stderr=${result.stderr.slice(0, 300)}`);
|
||||
assert.match(result.stderr + result.stdout, /token format does not match/i,
|
||||
'must surface the validator error');
|
||||
});
|
||||
|
||||
it('36d (P1-2) — olp-connect accepts a properly-formed olp_ token', () => {
|
||||
// Pad to 43 base64url chars after olp_. Don't actually hit the server —
|
||||
// use --port 1 (closed) which makes the connectivity probe fail at
|
||||
// the connect step (exit 2), but only AFTER the --key validator passes.
|
||||
const goodKey = 'olp_' + 'A'.repeat(43);
|
||||
const result = _spawnSyncS36('bash', [
|
||||
_joinS36(import.meta.dirname ?? process.cwd(), 'bin/olp-connect'),
|
||||
'127.0.0.1',
|
||||
'--port', '1',
|
||||
'--key', goodKey,
|
||||
'--dry-run',
|
||||
], { encoding: 'utf8', timeout: 5000 });
|
||||
// Validator passes → reaches connectivity step → exits 2 (or whatever
|
||||
// bash propagation gives) but NOT exit 1 from the validator.
|
||||
assert.notEqual(result.status, 1, `expected validator to pass; got exit 1 with stderr=${result.stderr.slice(0, 300)}`);
|
||||
assert.ok(!/token format does not match/i.test(result.stderr + result.stdout),
|
||||
'validator must NOT trigger for a well-formed token');
|
||||
});
|
||||
|
||||
it('36e (P2-3) — CacheStore.stats() returns {hits, misses, size, inflightCount} shape', async () => {
|
||||
// Real cacheStore.stats() returns { hits, misses, size, inflightCount } per
|
||||
// lib/cache/store.mjs. Pre-D74 cmdCache read body.entries / body.bytes /
|
||||
// body.maxBytes — all undefined → output showed "entries: ?" and "bytes: 0".
|
||||
// Pin the shape so a future server-side rename can't reintroduce the bug.
|
||||
// Use the CacheStore class directly (no HTTP) since the field names ARE the
|
||||
// contract — server.mjs handleCacheStats just sendJSON(s, 200, cacheStore.stats()).
|
||||
const { CacheStore: CS36 } = await import('./lib/cache/store.mjs');
|
||||
const cs = new CS36();
|
||||
const stats = cs.stats();
|
||||
assert.ok('size' in stats, 'CacheStore.stats() shape: size field present');
|
||||
assert.ok('hits' in stats, 'CacheStore.stats() shape: hits field present');
|
||||
assert.ok('misses' in stats, 'CacheStore.stats() shape: misses field present');
|
||||
assert.ok('inflightCount' in stats, 'CacheStore.stats() shape: inflightCount field present');
|
||||
assert.ok(!('entries' in stats), 'must NOT have OCP-era entries field');
|
||||
assert.ok(!('bytes' in stats), 'must NOT have OCP-era bytes field');
|
||||
assert.ok(!('maxBytes' in stats), 'must NOT have OCP-era maxBytes field');
|
||||
// Also pin that cmdCache reads body.size (not body.entries) by grepping the source
|
||||
const cliSrc = _readFileSyncS36(_joinS36(import.meta.dirname ?? process.cwd(), 'bin/olp.mjs'), 'utf8');
|
||||
assert.ok(/body\.size\b/.test(cliSrc), 'cmdCache must read body.size for entries display');
|
||||
assert.ok(/body\.inflightCount\b/.test(cliSrc), 'cmdCache must read body.inflightCount');
|
||||
assert.ok(!/body\.entries\b/.test(cliSrc), 'cmdCache must NOT read body.entries (OCP-era field)');
|
||||
});
|
||||
|
||||
it('36f (P2-3) — dashboard-data payload + cmdUsage source pin the wire-contract field names', () => {
|
||||
// server.mjs handleManagementDashboardData (~line 2027) builds the payload
|
||||
// inline. Pin by grepping the server source: the keys MUST be window_24h /
|
||||
// cache_hit_24h / quota / spend_trend_30d / top_fallback_chains_24h /
|
||||
// cache_stats. Pre-D74 cmdUsage read body.usage_24h.requests / body.providers
|
||||
// / body.top_fallback_chains — none of which exist in the actual payload.
|
||||
const serverSrc = _readFileSyncS36(_joinS36(import.meta.dirname ?? process.cwd(), 'server.mjs'), 'utf8');
|
||||
assert.ok(/window_24h:\s*auditAggregateRequests/.test(serverSrc),
|
||||
'handleManagementDashboardData must emit window_24h field');
|
||||
assert.ok(/cache_hit_24h:\s*auditCacheHitRateWindow/.test(serverSrc),
|
||||
'handleManagementDashboardData must emit cache_hit_24h field');
|
||||
assert.ok(/top_fallback_chains_24h:/.test(serverSrc),
|
||||
'handleManagementDashboardData must emit top_fallback_chains_24h field (NOT top_fallback_chains)');
|
||||
assert.ok(/cache_stats:\s*cacheStore\.stats/.test(serverSrc),
|
||||
'handleManagementDashboardData must emit cache_stats field');
|
||||
// Now pin cmdUsage reads the matching keys
|
||||
const cliSrc = _readFileSyncS36(_joinS36(import.meta.dirname ?? process.cwd(), 'bin/olp.mjs'), 'utf8');
|
||||
assert.ok(/body\.window_24h\b/.test(cliSrc), 'cmdUsage must read body.window_24h');
|
||||
assert.ok(/body\.cache_hit_24h\b/.test(cliSrc), 'cmdUsage must read body.cache_hit_24h');
|
||||
assert.ok(/body\.top_fallback_chains_24h\b/.test(cliSrc),
|
||||
'cmdUsage must read body.top_fallback_chains_24h (NOT body.top_fallback_chains)');
|
||||
assert.ok(/body\.quota\b/.test(cliSrc), 'cmdUsage must read body.quota');
|
||||
// Pre-D74 stale reads must be GONE from the CLI source
|
||||
assert.ok(!/body\.usage_24h\.requests\b/.test(cliSrc),
|
||||
'cmdUsage must NOT read body.usage_24h.requests (OCP-era field)');
|
||||
});
|
||||
|
||||
it('36g (P2-4) — olp-plugin fmtHealth iterates providers.status (not providers.*)', async () => {
|
||||
// Real /health full payload: providers: { enabled, available, status: { anthropic: {...} } }
|
||||
// Pre-D74 fmtHealth iterated Object.entries(body.providers) and the body
|
||||
// contained `status: {...}` so the loop printed a pseudo-provider named "status".
|
||||
const { fmtHealth } = await import('./olp-plugin/index.js');
|
||||
const realServerShape = {
|
||||
ok: true,
|
||||
version: '0.4.1',
|
||||
uptime_human: '1m 3s',
|
||||
providers: {
|
||||
enabled: 2,
|
||||
available: 3,
|
||||
status: {
|
||||
anthropic: { ok: true, activeSpawns: 0 },
|
||||
openai: { ok: false, error: 'oauth-missing', activeSpawns: 0 },
|
||||
},
|
||||
},
|
||||
};
|
||||
const out = fmtHealth(realServerShape);
|
||||
assert.match(out, /anthropic/, 'must list anthropic by name');
|
||||
assert.match(out, /openai/, 'must list openai by name');
|
||||
// assert.notMatch isn't available on assert/strict in some Node versions;
|
||||
// use the explicit negation pattern to stay portable.
|
||||
assert.ok(!/^\s*[🟢🔴⚪]\s*status\s*$/m.test(out),
|
||||
'must NOT list "status" as a pseudo-provider (the P2-4 bug)');
|
||||
assert.ok(!/^\s*[🟢🔴⚪]\s*enabled\s*$/m.test(out),
|
||||
'must NOT list "enabled" as a pseudo-provider');
|
||||
assert.ok(!/^\s*[🟢🔴⚪]\s*available\s*$/m.test(out),
|
||||
'must NOT list "available" as a pseudo-provider');
|
||||
// Also confirm the new enabled/available header
|
||||
assert.match(out, /2 enabled \/ 3 available/);
|
||||
});
|
||||
|
||||
it('36h (P3-5) — server startup banner does not hardcode a stale phase', () => {
|
||||
// Pre-D74 banner literal said "Phase 1 in progress" forever.
|
||||
const serverSrc = _readFileSyncS36(_joinS36(import.meta.dirname ?? process.cwd(), 'server.mjs'), 'utf8');
|
||||
assert.ok(!/Phase 1 in progress/.test(serverSrc),
|
||||
'startup banner must not hardcode "Phase 1 in progress" (stale post v0.4.0)');
|
||||
assert.ok(!/Phase 2 in progress/.test(serverSrc));
|
||||
assert.ok(!/Phase 3 in progress/.test(serverSrc));
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user