fix: e2e hardening for v2.1.3 (first-run gitignore, no-throw contracts, panic concurrency, large-output + federation conformance) - #182
Merged
Conversation
A fresh `git init` repo has no .gitignore, and `openlore init` previously only appended to an existing one — leaving .openlore/ analysis artifacts (multi-MB lance binaries, config) untracked. These then leaked into `git status` and polluted diff-based tools: the impact-certificate and blast-radius counted internal artifacts as "changed files" (observed 27 files / 2 symbols for a 1-file change). Now both the CLI and API init paths create .gitignore when absent. Tests updated to assert creation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
docs/agent-setup.md cited "61 tools" / "all 61 tools" while the live surface is 62 (TOOL_DEFINITIONS.length). The existing count-drift guard did not cover this file because it also documents the 6-tool `openlore-core` preset, which the strict exact-match check would reject. Extend the guard with a per-file preset-count allowlist so agent-setup.md is protected for its full-surface claims without choking on the legitimate preset size, plus assert the full surface is actually stated. Bumped the adjacent ~15k→~16k tokens figure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The same gitignore bug fixed in `openlore init` also lived in the two `run` pipeline init paths (the CLI command's inline init and the api/run.ts init): both only appended to an existing .gitignore, so `openlore run` on a fresh `git init` repo left .openlore/ untracked. Fixed both; added a run-level regression test asserting .gitignore creation when absent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hrough it The "create-or-append .openlore/ to .gitignore" logic was copy-pasted across four sites (cli init, api init, cli run, api run) — and three of the four silently guarded the write with `if (hasGitignore)`, which is exactly why the fresh-repo bug recurred everywhere. Extract a single `ensureGitignored(root, entry, comment)` helper that creates the file when absent and returns 'present' | 'appended' | 'created', and route every site through it. The per-case decision now lives (and is unit-tested) in one place; callers keep only their own messaging/prompt. No behavior change beyond the already-fixed create-when-absent semantics. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ntract) surfacesFromConfig guarded Array.isArray(members) but passed elements through unchecked. A `null` element (common in raw JSON.parse'd config) passes the array check, then resolveSurfaces' `m.symbol` / `m.file` access throws — and that call site in computeImpactCertificate is not wrapped, so the throw escapes the handler, violating the no-throw contract for change_impact_certificate. Filter members to objects at the existing coercion chokepoint. Regression test covers null/string/number members. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…xpired
panic-state.json is hand-editable, so readPanicState must treat every field as
untrusted. Two fail-open gaps:
1. A non-parseable `updatedAt` made `age = NaN`, and `NaN > EXPIRY` is false, so
a corrupt/zombie state survived forever instead of resetting. Now a non-finite
age is treated as expired.
2. Numeric fields (panicScore, panicLevel, counts, localityConfidence) flowed in
verbatim — a string/NaN/out-of-range value could poison scoring or index off
the end of SEVERITY_MAP/DIRECTIVE_MESSAGES. Now each is coerced and clamped,
and triggers is filtered to strings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… parity) buildLeaseAnchors is called from computeImpactCertificate with no try/catch at the call site, and handleChangeImpactCertificate has no outer catch — so a throw from AnchorContext.open or a corrupt/locked EdgeStore mid-read would escape the handler. The sibling recheckCertificate path already guards AnchorContext.open for exactly this reason. Harden the function itself to degrade to no anchors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…counter The MCP server wrote panic-state via an unlocked read-then-write, while the panic-check hook and gryph daemon serialize through a cross-process lock + CAS. A concurrent MCP write could clobber a hook's interventionCountSinceStable increment (last-writer-wins), under-counting the advisory→directive escalation gate. The MCP path also treated its in-memory tracker counter as authoritative, overwriting the shared on-disk counter. Fix: add mutatePanicStateLocked — a locked read-modify-write that reads the freshest on-disk state under the same lock all writers use, so none clobbers another. Route both MCP write sites (per-call score/level update and signal injection) through it. The disk file is now the single source of truth for the cross-process counter: - per-call update preserves the disk counter while panicLevel > 0 (so a concurrent hook increment survives) and resets to 0 at level 0 — matching the tracker's own invariant (level 0 ⟺ count 0; updatePanic/resetPanicOnOrient); - signal injection increments the disk counter so it composes with hook increments rather than overwriting them; - the in-memory tracker counter/revision are synced from each write. Fails open: if the lock can't be acquired, applies the mutation to a fresh read and writes best-effort (prior behavior). Never throws. Closes the known issue flagged in the PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…artifact
handleGetFileDependencies parsed dependency-graph.json inside a try/catch but
then called graph.nodes.find() / .map() OUTSIDE it, dereferencing n.file in the
predicate. A valid-but-partial artifact (e.g. {} or an interrupted analyze, a
node missing its file field) parses fine, so the catch doesn't fire, and the
access throws a TypeError out of the handler. Guard the nodes/edges array shape
(returning the existing friendly "run analyze" message) and access node.file
defensively. Regression tests cover {} and a node missing file.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…b390) The get_decisions MCP tool was removed from the surface but docs/mcp-tools.md still listed it in the tools table and the parameter reference, claiming a tool that no longer exists. The tool-count guard only checks the integer, not row identity, so it didn't catch this. Removed both references. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…runcation) Tool results are JSON-serialized, then capped to MCP_TOOL_MAX_BYTES (256 KB) by capOutput — which byte-truncates the serialized string. Cutting mid-string-literal yields UNPARSEABLE JSON, so a tool whose output exceeds the budget returns garbage the agent can't use. get_spec on the 313 KB analyzer spec hit exactly this (the e2e harness caught "Bad control character in string literal" — the truncation note's raw newlines landed inside a broken JSON string). Add capStructuredResult, which truncates at the STRUCTURED level: object results are re-serialized with their single largest top-level string field truncated to fit (shape preserved, valid JSON, marked truncated:true); raw-string results still use capOutput; the fallback wraps the partial in a valid JSON envelope. Binary search keeps the output within the byte budget despite JSON-escaping overhead. Route the MCP dispatch through it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The federation tools emitted the caller's absolute directory (including the home-dir prefix, e.g. /Users/user/...) as a repo-self identifier: spec_store_status / working_set_context / change_impact_certificate used it as the no-binding/no-surfaces finding `subject`, and federation_status as `homeRepo`. The spec-09/10 live-data harness flags this as an absolute-path leak (tool output should be portable/repo-relative). Use basename(absDir) — a stable, portable identifier — for these repo-self fields. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… edges The golden snapshots recorded totalEdges:0 for c-sds and the 110-file repo, predating the merged feature that synthesizes call edges into dependency-graph.json. Current analysis correctly reports those edges (c-sds: sds.c→sds.h impl/header call edge = 1; the larger repo = 443). The goldens went stale because the integration suite is CI-excluded; refresh them to the current correct values. Verified the edges are legitimate, not spurious. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The MCP e2e client matched responses to requests by FIFO waiter order. When the server completes concurrent tool calls out of order (it correctly may), waiters received mismatched lines, re-queued, and deadlocked → 30s timeout on the "concurrent tool calls" test. Correlate responses by JSON-RPC id via a pending map instead (the standard client pattern), so out-of-order delivery is handled. Also decode stdout via StringDecoder so a multibyte UTF-8 char split across a chunk boundary isn't corrupted. The server itself was verified correct (id-keyed client returns both concurrent calls in ~230ms with valid JSON). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
changed the title
handleGetSpec joined the untrusted `domain` tool arg straight into a path: pjoin(absDir, 'openspec', 'specs', domain, 'spec.md'). A traversing domain (e.g. "../../../../etc") escaped the repo root, letting get_spec read any spec.md on disk and probe directory existence. Its sibling tools (get_function_body/skeleton) already use safeJoin; route get_spec through it too (and return the normal "no spec found" error on a blocked escape). Regression test covers ../ and deep-escape domains. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…--sync Two commands reported failure but exited 0, silently defeating CI/gate automation: - `verify --json` skipped the exit-code block (it lived in the text-output else), so a "regenerate" verdict still exited 0. Moved the exit decision out of the display branch so it applies to both json and text. - `decisions --sync` printed per-decision errors but never set a non-zero exit, so the documented "sync then retry git commit" workflow would proceed on a partial sync. Now exits 1 when result.errors is non-empty (json + text). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ction `openlore view` called server.listen() with strictPort:true and no catch, so a busy port surfaced a raw EADDRINUSE stack trace. Catch it: print an actionable "port N in use, try --port" message and exit 1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The get_decisions MCP tool was removed from the surface in 785b390, but the handler and its 3 tests were left behind — dead code reachable from nothing (verified: no dispatch/registry/import references). Its purpose (browsing generated ADR .md files) is now covered by search_specs, which indexes ADRs under the `decisions` domain (1f2680c). Remove the handler and its test block; typecheck + lint stay green (no orphaned imports). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… stack trace) The decisions command had no top-level catch, so a throw from the LLM consolidate/verify path, spec-map build, git, or a spec write surfaced as a raw unhandled-rejection stack trace. Wrap the action body in try/catch — print a friendly error + exit 1 — matching drift/generate/verify. The nested consolidate try/finally (lock release) is unaffected; early returns still work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two real crashers in the non-streaming provider parsers: - Anthropic read data.content.filter(...) and data.usage.* unguarded — a 200 response missing content (proxy/overload/error-shaped body) threw "Cannot read properties of undefined", and a missing usage object threw too. - OpenAI / Copilot / Gemini read data.usage.* (data.usageMetadata.*) directly; many OpenAI-compatible gateways (Ollama, LM Studio, some proxies) omit usage, which either threw or poisoned cost tracking with NaN (every later += NaN stays NaN → "$NaN" in the report). Add a tokenCount() coercion helper (finite, non-negative, 0 default), guard Anthropic's content with Array.isArray, and route every provider's usage fields through it (total falls back to input+output when absent). Regression tests feed a content-less Anthropic response and a usage-less OpenAI response. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ate) The opt-in panic hooks (setup --hooks <format>) had two lifecycle gaps: - No uninstall path — once installed, panic-check/gryph-watch lingered in .claude/settings.json forever. Added uninstallPanicHooks and wired `--hooks none` as the inverse of `--hooks <format>`; it strips only openlore-marked entries (user hooks preserved) and is idempotent. - Re-running with a different --format silently kept the stale command (the idempotency guard matched the format-independent marker and returned early). Now replaces the entry in place when the command differs. New setup-hooks.test.ts covers install, idempotency, format-update, user-hook preservation, and no-op-when-absent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The three federation query loops (findCrossRepoConsumersBatch, locateSymbolProducers, findCrossRepoTests) called edgeStore.getExternalConsumers / getNode / searchNodes (and the call-graph walk) with no per-repo try/catch. readCachedContext swallows OPEN failures, but a store that opens fine then throws mid-query (SQLite corruption on an untouched page, disk error, a DB locked by a concurrent `analyze`) propagated out and aborted the WHOLE fleet query — violating the documented "one bad repo must not break the query" invariant. Worst for find_dead_code: a thrown lookup drops the cross-repo liveness check and risks a confidently-wrong "safe to delete". Wrap each repo's load+query in try/catch: on throw, push the repo to reposSkipped with a reason and continue; reposConsulted is recorded only after a full success. Regression test spies getExternalConsumers to throw and asserts the repo is skipped (not the query aborted). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dropped) detectLanguage had no case for the Node ESM/CommonJS (.mjs/.cjs) or TS module (.mts/.cts) extensions, so it returned 'unknown' and those files were excluded from BOTH the call graph and the signature index (CALL_GRAPH_LANGS has no 'unknown'). Real Node ESM packages and TS projects using these extensions had whole files invisible to analysis. The inconsistency confirmed the oversight: dependency-graph.ts already treats .mjs/.cjs as JS for import edges, so those files had dependency edges but zero call-graph nodes. Map .mjs/.cjs→JavaScript and .mts/.cts→TypeScript in detectLanguage, and add the same to repository-mapper's language-stats display map. Verified e2e: a repo with lib.mjs + mod.mts now yields their functions in the call graph. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…arity)
readCachedContext shape-guards the top-level artifact and (separately) the
inventory artifacts, but a present-but-malformed callGraph (a truncated or
hand-edited llm-context.json, e.g. `{"callGraph": {}}`) slipped through: it
passes every graph handler's `!ctx.callGraph` guard and then throws on
`cg.nodes.map(...)` — across orient, get_subgraph, analyze_impact, health-map,
find_path, etc. Normalize a callGraph whose nodes/edges aren't arrays to
undefined, so those handlers return their friendly "re-run analyze_codebase"
while signature-only tools keep working. Parity with the inventory shape guards.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three issues in the local viewer's API middlewares: - /api/chat/models logged the raw fetch error BEFORE sanitizing — the gemini path puts the API key in the request URL (?key=...) and fetch errors embed the URL, so the key leaked into the server console/log. Sanitize before logging. - /api/chat/models with no provider configured fired an unauthenticated request at api.openai.com (401 → misleading empty-200). Short-circuit the unconfigured fallback (no key + default OpenAI base) with a clear "no provider configured" message; a genuine local provider (custom baseUrl) still lists models keylessly. - /api/dependency-graph returned 500 on a missing artifact while its sibling endpoints 404 — added the same fileExists guard for a friendly "run analyze". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…he graph Follow-up to d129a32: dropping a callGraph whose nodes/edges weren't arrays was too aggressive — a valid minimal callGraph can carry only entryPoints/hubFunctions (architecture_overview reads those without touching nodes), so dropping it regressed those handlers. Instead, coerce a missing/invalid nodes or edges to [] (preserving every other field) so `cg.nodes.map(...)` can't throw while entryPoints-only consumers keep working. A callGraph that isn't an object at all is still dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bump package.json 2.1.2 → 2.1.3 (the CLI and MCP server both read the version from package.json, so this is the single source of truth) and sync the lockfile. Add CHANGELOG.md documenting everything since v2.1.2: the new capabilities (panic behavioral governance, spec-store binding, working-set context, change-impact certificate, live watch-mode dependency graph, Pi updates) and the end-to-end hardening pass (PR #182). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Refresh stale tests badge (3900+ → 4400+). - Note watch mode reconciles file creates/deletes and keeps dependency-graph import edges live (#173). - Add the spec-store arc to the Federation section: spec_store_status, working_set_context, and change_impact_certificate (all under the federation preset) were undocumented in the README. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
End-to-end hardening + full dogfooding over everything merged into
mainsince v2.1.2 (panic governance, spec-store binding, working-set-context, change-impact-certificate, watcher incremental, multi-repo federation), preparing v2.1.3. Across several rounds I dogfooded the real built CLI on clean repos and a live cross-repo federation, exercised the MCP server over stdio (presets, concurrent calls, large payloads, 5 real-language corpora: C/Go/Java/Python/Ruby), audited every subsystem with parallel agents, and ran the CI-excluded integration suite + per-provider failure injection.Every fix ships with a regression test. Unit: 4433 passing · integration: 161 passing (one fs-event watcher test is timing-flaky under load, passes in isolation) · typecheck + lint clean. 27 commits.
Fixes by area
Analyzer core
.mjs/.cjs/.mts/.ctsfiles were silently dropped (detectLanguage→ 'unknown' → excluded from call graph and signatures). Now mapped to JS/TS; verified e2e that their functions appear in the graph.MCP robustness / no-throw
readCachedContext: a present-but-malformedcallGraph({}, or a minimal entryPoints-only graph) threw oncg.nodes.map(...)across graph handlers — now normalizes missingnodes/edgesto[](preserving other fields; a non-object callGraph is dropped).get_specpath traversal;get_file_dependenciespartial artifact;change_impact_certificatenull member + unguardedbuildLeaseAnchors; large results truncated into invalid JSON (capStructuredResult); federation absolute-path leak.view HTTP server
/api/chat/modelslogged the raw API key (gemini key in URL) before sanitizing → sanitize first; fired an unauthenticated request at api.openai.com when no provider configured → clear message instead;/api/dependency-graph500 → 404 on a missing artifact (sibling-endpoint parity).LLM generation
$NaNon a malformed/usage-less response →tokenCount()coercion + content guards.Panic concurrency + lifecycle
mutatePanicStateLocked, disk as single source of truth; NaN/sanitize fail-open;setup --hooks noneuninstall + in-place format update.Multi-repo federation
First-run, CLI exit codes, cleanup
.gitignorenever created on a fresh repo (4 sites → oneensureGitignored());verify --json/decisions --syncreported failure but exited 0;decisionstop-level error boundary;viewEADDRINUSE; removed orphanedhandleGetDecisions;mcp-e2eclient id-correlation (was deadlocking on out-of-order responses); refreshed stale golden edge counts;agent-setup.mdtool count + danglingget_decisionsdoc refs.Verified healthy
MCP presets; all core tools across C/Go/Java/Python/Ruby corpora (no crashes, correct counts);
panic-checkfail-open; watcher incremental reconciliation (e2e);preflightexits 1 stale / 0 fresh; graceful no-LLM-key degradation; spec-store/working-set/federation config coercion + path-traversal guards; federation registry atomic-write/dedup/corrupt-manifest; cross-repofind_dead_codeliveness (dogfooded).Deferred (documented, low-severity)
Concurrent
federation addlost-update; all-unknownfederationReposempty-but-caveated no-op; cursor/continue MCP writers reformat on re-merge (cosmetic);uninstallClaudeHook's broad legacyopenlore analyzemarker (deliberate Spec 26 B9 cleanup).🤖 Generated with Claude Code