MOO-72 Commit 4: layer-specific failure isolation and retry behavior - #70
Closed
OwenTanzer wants to merge 94 commits into
Closed
MOO-72 Commit 4: layer-specific failure isolation and retry behavior#70OwenTanzer wants to merge 94 commits into
OwenTanzer wants to merge 94 commits into
Conversation
Document the pinned starting revision, startup commands, and known nondeterministic fields, and extend the existing repo-smoke script with committed structural snapshots for all four fixtures so later modularization commits can prove they preserved analyzer behavior rather than assert it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Stand up dev/build/preview commands over the existing index.html unchanged, plus a minimal static server entry point serving the production build. Analyzer marker extraction and the app's inline script are untouched — this only proves the tooling works, so the existing single-file index.html stays available as a rollback path until module extraction lands in Commit 3. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… Commit 3) Move Parser, buildAnalysisData, calcBlast, calcHealth, runAnalysisData, createAnalysisWorkerSource, GitHub, and the rest of the analyzer's top-level names out of index.html's inline script into src/analyzer.js, a real ES module. index.html bridges its exports onto window for the still-classic app script; card/lib/analyzer.js and every Node test now import it directly instead of VM-extracting a marker-delimited block from index.html text. The analysis Web Worker's bootstrap (createAnalysisWorkerSource) used to fetch the page's own HTML and slice out the analyzer by string marker; it now fetches its own module URL via import.meta.url instead, with new internal CODEFLOW_CORE_START/END markers excluding runAnalysisData/createAnalysisWorkerSource themselves from what gets embedded into the worker (their source contains import.meta, which is a syntax error in the classic-script worker context). Verified end-to-end via headless Chromium against both the dev server and a production build. Also folds two previously-duplicated stub functions (getSecurityScanContent, isSanitizedPreviewRenderer) into the real module, closing a latent behavioral gap between what the browser ran and what Node-side tests exercised. Known regression, documented in docs/baseline.md rather than silently carried: opening index.html directly via file:// now crashes, since Chromium blocks the analyzer module's import under CORS for that origin. The app still works correctly served over npm run dev or npm run build + npm start. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
card/lib/analyzer.js's synchronous require(esm) of src/analyzer.js needs unflagged require(esm) support, which shipped in Node 20.19.0 and 22.12.0 specifically -- not all of card/'s previously-declared ">=20" range, and not 21.x (non-LTS, EOL before the backport). Narrow both package.json engines fields to "^20.19.0 || >=22.12.0" (matching Vite 8's own declared constraint) and add engine-strict=true via .npmrc in both locations so an incompatible local Node fails loudly at install time instead of a later ERR_REQUIRE_ESM. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Product decision: accept file:// support's removal (flagged in Commit 3) rather than restore it. MOO-67 is building toward a server-backed Railway application with server-held credentials; preserving double-click local-file execution would complicate the modular architecture for little future value. docs/baseline.md now records this as resolved rather than open. README.md's Quick Start, Architecture, Contributing, and FAQ sections no longer advertise the zero-install "just open index.html" workflow -- self-hosting now documents npm install + npm run dev (or npm run build && npm start) as the supported path. Also fixed two other now-stale README claims caught while editing this section: the Architecture diagram's "Single File" label, and the Contributing section's test command (node --test tests/ doesn't discover this repo's flat tests/*.test.mjs layout; needs the glob). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add tests/ui-smoke.mjs: six deterministic Playwright checks covering the repository-view behaviors Commit 4B-4E are about to touch -- local-folder load + graph render, node-click detail-panel update, visualization-type switching, ?repo= URL prefill without a network call, browser back/forward, and zero real console errors. Verified passing against both the dev server and a production build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move the ~150-line D3 force-graph build/update lifecycle out of App()'s inline useEffect into renderRepositoryGraph() (src/render/repositoryGraph.js), a real ES module bridged onto window the same way src/analyzer.js is. Mechanical extraction (sed-sliced, not retyped): svgRef.current -> svgEl, setTooltip -> onHover, setSelected/setBlastRadius on background click -> onBackgroundClick, everything else (data, colorMap, colorMode, theme, folderFilter, graphConfig, COLORS, LAYER_COLORS) became explicit parameters. zoomRef/simRef/linksRef/nodesRef/selectFileRef are passed through as the same ref objects App() already holds -- several other call sites (zoom controls, blast-radius reset, PDF export, "Back to Issues") read these directly and needed no changes as a result. Verified via tests/ui-smoke.mjs (6/6 against both dev and production builds) plus an ad hoc Playwright probe specifically exercising zoom in/out/reset, hover tooltip, and "Back to Issues" -- zero console errors, all three still worked. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move selected, blastRadius, rightTab, and drillDown -- the state the repository view's selection and detail panel need -- from four separate useState calls in App() into useRepositorySelection() (src/state/selection.js), bridged onto window the same way as Commits 3 and 4B. Every variable name stayed identical, so no call site elsewhere in App() needed to change. Deliberately left alone: showGraphConfig, rightPanelWidth, folderFilter, data/loading/error, theme, and all architecture/security-specific state, plus expandedPaths/ expandedCards (unrelated tree-expansion state that happened to sit textually nearby). Verified via tests/ui-smoke.mjs (6/6) plus an ad hoc Playwright probe cycling all four panel tabs -- zero console errors. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Consolidate buildAppUrl and the scattered query-param read/write logic (a mount-time useEffect reading ?repo=/?run=1, and two window.history.replaceState call sites) into src/state/route.js: buildRepoUrl, readRouteRepo, writeRepoRoute, clearRoute. Bridged onto window the same way as Commits 3, 4B, and 4C. Scoped to repository identity only, per the checklist -- not active view/panel/selection restoration, and not canonical source coordinates or breadcrumb payloads, both reserved for MOO-68. buildRepoUrl/readRouteRepo accept an optional baseHref/search parameter (defaulting to the real window.location), making this the first of the four extractions genuinely unit-testable without a DOM -- tests/route-state.test.mjs adds 8 tests covering URL construction, the run=1 gate, and the three validation guards the original inline code already had. Verified via tests/ui-smoke.mjs (6/6, its route/hash-restoration check exercises readRouteRepo directly) plus an ad hoc Playwright probe calling writeRepoRoute/clearRoute directly, since the write path only triggers on a real GitHub load the smoke suite deliberately avoids. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… 4E) Add a dblclick handler on graph nodes in repositoryGraph.js alongside the existing click handler, using the same node identity (d.id) the click handler already uses. Wired to a new activateFileRef (same pattern as selectFileRef), which App() initializes to a no-op and does not wire to anything else -- node-activate may remain unused or resolve to a no-op in MOO-67 per the checklist, with real drill-down semantics reserved for MOO-68. This is the last piece of Commit 4 -- 4A (UI smoke suite), 4B (repository graph renderer), 4C (selection/panel state), 4D (route persistence), and now 4E (interaction seam) are all landed and independently verified. Verified via tests/ui-smoke.mjs (6/6) plus an ad hoc Playwright probe double-clicking a node (no crash) then single-clicking it again (still selects correctly, proving no event-wiring corruption). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Restructure server/index.js from the Commit 2 static-file placeholder into a real server shell: config validated fail-fast at startup (server/lib/config.js), a request-scoped workspace abstraction with a single controlled root and cleanup (server/lib/workspace.js), structured per-request JSON logging with sanitized secrets (server/lib/logger.js), distinct liveness/readiness endpoints (server/lib/health.js), and a bounded POST /api/analyze endpoint (server/routes/analyze.js) that only reaches paths already on the server's own filesystem -- no GitHub credential or auth gate exists yet, that's Commit 6. analyzer-bridge.js reuses card/lib/collect.js's buildAnalyzed() for file collection rather than writing a fourth copy of that logic. Added durable automated coverage: 9 new unit tests (server-config.test.mjs, server-workspace.test.mjs) plus tests/server-smoke.mjs, which spawns the real server process and confirms /api/analyze against golden-world matches the exact files:6/functions:7/connections:6 baseline from Commit 1, path- traversal and missing-path requests are rejected, and the workspace root is completely empty after all requests (cleanup verified, not just callable). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GitHub.scanTree/scanRecursive call shouldExcludeFile/ shouldIgnoreDirectory, which were left behind in index.html during the Commit 3 extraction -- they only worked in the browser by accident (window-fallthrough), and threw "shouldExcludeFile is not defined" the first time anything called GitHub.scan() server-side, ahead of building Commit 6's GitHub-backed endpoint around it. Move IGNORE, normalizeExcludePath, matchesExcludePattern, shouldIgnoreDirectory, and shouldExcludeFile into src/analyzer.js for real, exported and re-bridged onto window so index.html's own local-folder-reading code (which also calls these) keeps working unchanged. Verified GitHub.scan/getFile against a real public repo from Node. Added tests/analyzer-module.test.mjs (4 tests, no network) so this gap can't silently reappear. Full suite 83/83, clean build, and tests/ui-smoke.mjs (6/6) all pass -- pure addition/relocation, no behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Three independent gates now sit in front of every /api/* route: - server/lib/auth.js -- a shared-secret Authorization: Bearer check, timing-safe compared. /healthz/readyz/static stay public for Railway's own health monitoring. - server/lib/rate-limit.js -- in-memory per-client-IP fixed-window limiter (RATE_LIMIT_PER_MINUTE). - server/lib/validate-repo-request.js + server/lib/allowlist.js -- owner/repo/ref/PR format validation, then an allowlist check, both before any GitHub call. AUTH_TOKEN, GITHUB_TOKEN, and at least one of ALLOWED_REPOS/ ALLOWED_OWNERS are now required, validated fail-fast at startup like everything else in config.js. New POST /api/analyze-repo (server/routes/analyze-repo.js + server/lib/github-analyzer-bridge.js) fetches a repo at a resolved ref (default branch, explicit branch/commit, or a PR's head SHA) via the GitHub REST API using the server-held token, then runs it through the same analyzer everything else uses. Reuses GitHub/Parser/ shouldExcludeFile/buildAnalysisData from src/analyzer.js rather than writing a second GitHub client -- but not GitHub.scanTree/getFile as-is, since both are hardcoded to the default branch with no ref parameter, which is exactly the gap this commit needs closed. Found and fixed two real bugs while verifying against real GitHub data (not fixtures): a PR's head commit usually lives in a fork, not the base repo, so tree/blob fetches need to follow head.repo, not the originally-requested owner/repo; and GitHub.request()'s errorMap errors are plain Errors, not GithubFetchError, so a genuinely-expected failure (a deleted PR fork) was surfacing as a generic 500 instead of a clean 502 with GitHub's own message. Added tests/server-auth.test.mjs (16 unit tests), expanded server-config.test.mjs (10 tests) and server-smoke.mjs (17 steps, including both bugs above verified against real repos/PRs via a real GitHub credential from `gh auth token`). Full suite 105/105, clean build, tests/ui-smoke.mjs still 6/6. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add railway.json pinning the build/start commands and wiring Railway's own rollout health gate to /readyz (not just "process is up") -- confirmed applied by re-checking deployment metadata before and after. Railway's Railpack builder had already auto-detected and deployed this successfully with zero config, but explicit configuration is a real checklist item, not just a nice-to-have. Created and linked the codeviz Railway project/service, set AUTH_TOKEN (generated, not committed anywhere), GITHUB_TOKEN (the same PAT already used throughout Commits 5-6), ALLOWED_OWNERS (OwenTanzer), and NODE_ENV=production via `railway variable set`. Verified against the live deployment, not just locally: healthz/ readyz, static serving, auth rejection (401 anonymous and wrong- token), the local-path analyze endpoint matching the same golden-world baseline every other environment produces, allowlist rejection (403) for a non-allowlisted owner, and a real GitHub-backed analysis (OwenTanzer/CodeVisualizer) succeeding end-to-end through the deployed instance. Documented both real rollback paths (dashboard redeploy of a specific past deployment; CLI redeploy from an earlier git commit -- confirmed there's no CLI command to target a non-latest deployment directly, rather than assuming one exists) and the exact DNS/Moopertonic Hub cutover steps for MOO-72, without performing that cutover now. MOO-67 complete: all seven commits landed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Requested after initial deployment: analyze other users' repos, not just the operator's own. server/lib/allowlist.js now recognizes a literal "*" in ALLOWED_OWNERS as an explicit "any owner" opt-in, rather than requiring individual owner names to be piled on. The auth token remains the actual gate on who can reach /api/analyze-repo; this only changes which repos a valid caller can point it at. Set ALLOWED_OWNERS=* on the live codeviz Railway deployment and redeployed. Verified octocat/Hello-World (previously blocked) now analyzes successfully, and OwenTanzer/CodeVisualizer (the original allowlist entry) still works. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Three findings from PR #1 review, all fixed: 1. MAX_REPO_FILES capped file count but not byte size -- the GitHub-backed path fetched every accepted blob into memory before analysis, and the wildcard allowlist follow-up means any authenticated caller can now point the server at any public repo. Add MAX_FILE_BYTES/MAX_REPO_BYTES; GitHub's tree API already reports each blob's size, so oversized files are rejected before any content is fetched/decoded. Individually-oversized files are skipped (like an ignored directory); the aggregate is a hard cap. Refactored the selection logic into a pure, exported selectAnalyzableFiles() specifically so it's unit-testable against synthetic tree data instead of only reachable through a real GitHub round-trip (7 new tests). 2. resolveWithinRepo()'s comment claimed it rejected symlinks; it only did lexical path-traversal checking and never called realpath(), so a symlink sitting lexically inside the repo could still point elsewhere. Fixed by resolving both the repo root and the requested target through realpath() and checking containment on the resolved paths -- what cp() actually reads from. Verified with real filesystem junctions (5 new tests) rather than just asserting the fix works: one junction escaping the repo root is rejected, one pointing elsewhere inside the root is accepted. 3. /api/analyze-repo bounded its body via MAX_REQUEST_BODY_BYTES; /api/analyze buffered the whole request unbounded, despite both being publicly addressable behind the same bearer-token gate. Extracted one shared readJsonBody() (server/lib/http-body.js, 5 new tests) both routes now use. Full suite 126/126 (up from 107). tests/server-smoke.mjs re-verified against real GitHub data (17/17) to confirm none of this broke the existing happy paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Flagged as non-blocking by review but easy to close out now: add .github/workflows/test.yml running npm ci && npm run build && node --test tests/*.test.mjs on push/PR to main. Scoped to the credential-free, no-real-network unit/integration suite -- tests/ui-smoke.mjs (needs a Playwright browser) and tests/server-smoke.mjs (needs a real GitHub token) stay manual-precondition scripts for now; wiring those into CI too is a reasonable separate follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 new alerts (2 high, 1 medium) surfaced once the diff was large enough for CodeQL to re-evaluate index.html/server/ -- pre-existing gaps, not regressions, per GitHub's own note that alerts not introduced by the PR can still appear when changes are large. Fixed regardless: - server/lib/static.js (js/path-injection, high x2): the resolved.startsWith(distDir + sep) check is a raw string-prefix comparison, not a pattern CodeQL recognizes as a proven sanitizer. Rewrote to use path.relative() + a ".."/absolute check, the same idiom server/routes/analyze.js's resolveWithinRepo already uses (which CodeQL did not flag). Deliberately did not add realpath(): dist/ is build output the operator controls, and realpath() throws for anything not on disk, which would have broken the existing SPA-style fallback for unmatched client-side routes. Added tests/server-static.test.mjs (6 tests) covering traversal rejection, a percent-encoded attempt, and the SPA-fallback case specifically. - index.html (js/functionality-from-untrusted-source, medium): every other CDN script tag already had an integrity="sha512-..." attribute; only mermaid.min.js was missing one. Computed the hash locally and cross-checked it against cdnjs's own published SRI metadata before trusting it -- they matched. Full suite 132/132 (up from 126). Clean build. tests/server-smoke.mjs re-verified (17/17). Manually re-confirmed live: traversal still rejected (400), SPA fallback for an unmatched route still works (200), a real asset still serves (200). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…deflow-and-establish-the-railway-application MOO-67: Modularize CodeFlow and establish the Railway application shell
Structured, versioned identity for any point in a specific revision of a specific repository: repository identity, resolved revision, path, scope-chain (symbolPath), symbol kind, and optional range. Serializes canonically (stable key order) and encodes to an opaque base64url token for routes/cache keys, avoiding fragile delimiter-based string splitting.
Normalizes repository/branch/commit/PR requests into one canonical AnalysisContext (owner, repo, mode, resolvedSha, optional base/head SHA, PR number), rejecting mixed or contradictory revision fields. assertContextPropagation enforces that a file/function drill-down request can never silently switch revisions relative to its parent graph.
One schema (schemaVersion, layer, context, nodes, edges, groups, analyzer provenance, confidence, warnings, rendering hints) shared by repository/file/function graphs. Layers keep distinct kind/hint vocabularies (shape, semantic colorRole, layout preference) while validation rejects cross-layer node/edge mismatches and dangling edge references with clear messages, and safely ignores unknown extra fields anywhere in the tree.
One AdapterResult envelope (graph, warnings, diagnostics, provenance, timing, cache metadata, partial flag) for every layer adapter, plus a fixed set of stable error categories (github_access, unsupported_input, parser_failure, subprocess_failure, malformed_analyzer_output, timeout, renderer_failure, internal_error). sanitizeDiagnostic strips stack traces and redacts secret-shaped keys at any depth, applied unconditionally by buildAdapterResult so a producer can't forget it. Partial-success results may omit the graph entirely, but never carry a schema-invalid one.
Shared interaction contract: single click emits selection/focus only (createSelectionEvent); double click emits drill-down intent (createDrillDownEvent) carrying the source coordinate and target layer, but only when isDrillDownEligible — an ambiguous or under-resolved coordinate throws NavigationError rather than dispatching an incorrect drill-down. createOpenSourceEvent covers the separate "view raw source" action. NavigationHistory is a back/forward breadcrumb stack keyed by graph cache key + selection + coordinate, truncating the forward branch on push like standard browser history. This is the real seam MOO-67 Commit 4E's node-activate no-op was left for.
buildCacheKey derives a stable sha256-based key from normalized revision context, analyzer name/version, GraphIR schema version, requested coordinate, and depth/options — canonicalized (deep key-sorted) so equivalent normalized requests collapse to the same key while any real difference (revision, analyzer version, schema version, depth, requested coordinate) always changes it. isCacheStale treats a schema-version mismatch as always stale and supports an optional TTL. buildProvenanceSummary reports visible provenance plus resolved/unresolved adapter-match counts.
- src/graph-ir/index.js: single barrel import surface for the whole
contract (no name collisions across the six modules).
- tests/fixtures/graph-ir/{repository,file-pyan,function-codevisualizer}.json:
representative GraphIR fixtures for all three layers, regenerable
via scripts/gen-graph-ir-fixtures.mjs, validated in CI.
- examples/minimal-graphir-adapter.mjs: a runnable example adapter
proving GraphIR can be produced and consumed (selection + drill-down
events, provenance summary) using only src/graph-ir/index.js, no
application code.
- docs/graph-ir-contract.md: the repository -> file -> function
identity flow, module-by-module ownership, and extension rules for
future languages/analyzers/renderers/LLM annotation.
- docs/baseline.md: MOO-68 completion summary alongside the existing
MOO-67 regression baseline.
Full suite: 207/207 (132 pre-existing + 75 new across all seven
commits). This closes MOO-68 -- MOO-69 can now adapt the repository
layer against these contracts instead of the MOO-67 neutral seams
directly.
…alidation (PR review) Addresses two PR #2 review findings: - The barrel was accidentally Node-only: sourceCoordinate.js used Node's Buffer for route tokens, cacheKey.js imported node:crypto for hashing, and index.js re-exports both unconditionally, despite docs/graph-ir-contract.md documenting the barrel as the one import surface for browser-side renderer/navigation code too. Replaced Buffer.from(...).toString('base64url') with a TextEncoder/TextDecoder + btoa/atob implementation, and replaced createHash('sha256') with a dependency-free FNV-1a-64 fingerprint (adequate for cache-key distribution, not a security primitive). Added scripts/verify-graph-ir-browser-import.mjs, which drives the real barrel through headless Chromium against the Vite dev server and confirms no Node-only global is reachable -- verified passing. - SourceCoordinate validation was looser than documented: revision accepted any non-empty string instead of a resolved-SHA shape, path didn't reject '..'/'.' segments, and range didn't enforce positive lines, nonnegative columns, or start <= end. Tightened all four, reusing the same SHA pattern githubContext.js already enforces on AnalysisContext (duplicated rather than imported, since the two modules are peers graphIR.js depends on, not a hierarchy). 12 new tests. Full suite: 214/214 (207 previous + 7 new... plus the browser smoke script, which isn't part of node --test since it needs a running dev server, same convention as the existing -smoke.mjs/verify-*.mjs scripts).
MOO-67 already discovered that a PR's head commit typically lives only in the contributor's fork, not the base repository (github-analyzer-bridge.js's resolveRef() resolves this via GitHub's head.repo). MOO-68's AnalysisContext dropped that distinction: it only stored the requested base owner/repo plus the resolved head SHA, so a coordinate built from a forked-PR analysis could name the wrong repository to fetch from later. Adds sourceOwner/sourceRepo, defaulting to owner/repo for every mode except pr (where a caller may supply the fork's owner/repo). Only pr mode may set them -- other modes have no fork concept. sameRevision, assertContextPropagation, and contextIdentityKey now key off the resolved source repository (where content actually lives), not the requested base repository, while assertContextPropagation still separately checks the base repository for provenance/allowlist continuity. 7 new tests covering same-repo defaulting, explicit fork resolution, cross-fork rejection, and cache-identity distinctness. Full suite: 220/220 (214 previous + 7 new, minus 1 net from a consolidated assertion -- see test diff).
validateGraphIR previously only checked that context had a resolvedSha and that each coordinate was structurally valid -- it never compared a node's coordinate against the graph's own analyzed context. A graph pinned to commit A could contain a node coordinate pinned to commit B (or a different repository entirely) and still pass validation, defeating the stated cross-revision-navigation invariant. Per the review's own refinement, blanket repository/revision equality across every coordinate was rejected as too strict -- it would make legitimate cross-repository dependency edges, cached references, and synthetic nodes needlessly hard to represent. Instead, adds an optional per-node `origin` field (default 'local'): 'local' nodes must match the graph's resolved source repository/revision exactly; 'external'/'cached' nodes may differ but must still name a concrete coordinate; 'synthetic' nodes may have no coordinate at all. rootCoordinate has no such override -- it defines what the graph is "of," so it must always be local. Regenerated tests/fixtures/graph-ir/*.json (context now includes sourceOwner/sourceRepo from the prior fork-identity fix). 10 new tests covering local mismatch rejection (revision and repository), the three origin overrides, missing-coordinate rejection for external/cached, invalid origin values, and a forked-PR context correctly accepting a coordinate naming the fork. Full suite: 230/230. Clean build. This closes out all four PR #2 review findings.
normalizeContext validated sourceOwner and sourceRepo independently, so a caller supplying only one let the other silently default to the base repository's value -- constructing a nonexistent hybrid identity (e.g. a fork owner paired with the base repo's own name, when the fork actually renamed the repo too). Now rejects any request that sets exactly one without the other. 3 new tests.
Makes the repository -> file -> function -> back path deterministic and
restorable, and adds the browser smoke coverage that proves it.
BreadcrumbEntry has carried `graphCacheKey` and `selectedNodeId` since MOO-68
Commit 5, specifically so back/forward could restore a view from cache instead
of re-running analysis -- but nothing ever wrote them, so both were always
null. NavigationHistory.updateCurrent is that missing write path. It replaces
rather than mutates the entry, so a React holder sees a changed identity
instead of silently reading a mutated object, and it deliberately does not
truncate forward history: it records what is already being shown, it is not a
navigation.
Both panels now restore before fetching. The cache is keyed by the server's own
AdapterResult.cache.key rather than a key derived client-side, so client and
server agree on what "the same analysis" means by construction -- that key
already folds in revision, analyzer version, schema version, and the requested
coordinate. It lives in a ref (writing it must never trigger a render, and
nothing renders from it) and is cleared whenever a new analysis starts, since
every key is scoped to a revision that no longer applies. A restored view also
needs no server token, so back/forward keeps working after a token is cleared.
Function-layer breadcrumbs are labelled by symbol rather than path -- several
functions in one file would otherwise produce a trail of identical-looking
segments ("sessions.py / sessions.py").
New tests/function-layer-smoke.mjs drives the whole path through the real UI
against psf/requests. Like tests/ui-smoke.mjs it is not part of the zero-setup
`node --test` suite, since it needs a running server plus both credentials.
It deliberately targets SessionRedirectMixin.resolve_redirects rather than
whichever node comes first: the first run picked Session.__enter__, whose valid
but trivial three-node graph let the loop and branch assertions pass without
ever exercising them. Latest run: 55 nodes, 3 dashed back-edges, 18 true/false
labels, 1 exception label, 0 Mermaid entities, 0 console errors.
The back-navigation check asserts on request counts rather than appearance --
after repository -> file -> function -> back, exactly one /api/graph/file
request has been issued in total, which is what actually distinguishes a
restore from a fast refetch.
This closes Commit 7's outstanding verification gap: the FunctionLayerPanel
wiring (single token prompt across the path, search highlighting, metadata
inspection, selection) is now verified in a real browser, not just by unit
tests. Earlier automation failed because the PAT input only exists once
select[aria-label="Authentication Method"] is set to 'pat'.
Verified: 425/425 from a clean install (rm -rf node_modules .vendor && npm ci
&& npm run build), plus the full browser smoke passing end to end.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Most of this commit's checklist was already satisfied: Commit 5 gave the
function endpoint per-stage error categories, sanitized diagnostics and
structured logging carrying analyzer version, coordinate, duration, node/edge
counts and cache status; Commit 7 gave the panel user-visible retry and
per-category diagnostics; Commit 8 made the layers structurally isolated, since
each is a separate panel and the upper ones restore from cache rather than
re-analyzing. What was missing was one real distinction.
@codevisualizer/core raises a single FunctionRangeNotFoundError for two
genuinely different situations, because tree-sitter is error-tolerant: it never
throws on a syntax error, it returns a tree containing ERROR nodes. So source
that does not parse does not arrive as a distinct parse error -- it arrives as
"no function definition matches that exact range", which is also what a real
offset-conversion bug on our side produces.
Every occurrence was reported as "Internal conversion error", i.e. we told users
we had a bug when their file simply wasn't valid Python, and the two were
indistinguishable in logs -- the opposite of this commit's own "logs identify
parser failure separately".
classifyFunctionRangeFailure now splits them on tree.rootNode.hasError(), which
the symbol index already computes over the same source, so it costs no extra
parse:
- parse errors present -> parser_failure, the same category pyan3's adapter
already uses for the same situation at the file layer, so both analyzers
report an unparseable file identically.
- none -> malformed_analyzer_output, keeping the precise byte range for
debugging. That case really is our bug.
The panel adapts its wording to match: retrying is useless for a parse failure
and it now says so, rather than offering a retry that cannot succeed.
Extracted and exported rather than inlined so both branches are unit-testable
without a live GitHub credential, matching resolveFunctionSymbol's precedent.
Beyond testing both branches, one test grounds the whole thing in real analyzer
behavior -- asserting that a genuine syntax-error fixture really does yield
parseErrors === true AND really does make analyzePythonFunction throw
FunctionRangeNotFoundError. If upstream ever starts throwing a distinct parse
error instead, that fails loudly instead of leaving this branch quietly
unreachable.
docs/function-layer-renderer.md now carries the per-stage category table and
the consolidated Garrison hand-off.
Verified: 430/430 from a clean install (rm -rf node_modules .vendor && npm ci &&
npm run build), plus the full browser smoke still passing end to end, confirming
the error-path change did not disturb the working path.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both workflows filtered on `pull_request: branches: ["main"]`, so a PR opened against another in-review branch reported no checks at all. That is how this project sequences dependent commits (MOO-71 Stage 1 against fix/anthropic-llm-provider, Stage 4 against the Stage 3 branch), so exactly the PRs carrying unreviewed work were the ones going unverified -- MOO-71 PR #7 had to be checked by hand instead. Dropping the base filter costs nothing here and means a stacked PR is verified before it is rebased onto main rather than after. The push trigger stays main-only; running the full suite on every branch push would be noise. Kept as its own commit rather than folded into Commit 10, since it is repository infrastructure and has nothing to do with that commit's diagnostics work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…er-function-layer
…function, disable Retry on parser_failure classifyFunctionRangeFailure previously used tree.rootNode.hasError() (whole file), so an unrelated syntax error anywhere in the file would label every function's FunctionRangeNotFoundError as parser_failure -- concealing a genuine offset-conversion bug in a function whose own range parses cleanly. indexPythonSymbols now also returns errorRanges (byte ranges of every ERROR/MISSING node, collected only when hasError() is already true), and classifyFunctionRangeFailure checks whether one of those ranges overlaps the target function's own [startByte, endByte) instead of trusting a file-wide boolean. Added a regression test against the syntax_error.py fixture's already-broken function alongside its clean solo() function, matching the exact scenario from review. Also hides the Retry button on a parser_failure: the panel already tells the user retrying won't help until the file parses, but still rendered an active button inviting the request anyway.
…e-wide hasError() docs/function-layer-renderer.md still described the pre-review-fix approach (a single file-wide tree.rootNode.hasError() boolean). Updated to match 7d16866's actual behavior: errorRanges checked for overlap with the target function's own byte range, plus a note on the Retry button now being hidden for parser_failure.
…om analyzer bugs MOO-71 Commit 10: distinguish parse failures from analyzer bugs
…ving legacy features The repository layer's client (index.html's analyze()/finishAnalysis()) never actually called the server's existing /api/graph/repository route -- it ran a full client-side GitHub-scanning pipeline using a user-entered PAT, entirely separate from the server-side route built in MOO-69. This blocked the "repository -> file -> function share one session" work MOO-72 Commit 1 needs, since there was no server request to correlate for the repository leg. Investigation before migrating found the server's GraphIR wasn't a strict superset of what the legacy analyzer produced: churn was hardcoded to 0 (never computed server-side), blast radius was deliberately excluded from GraphIR (a per-selection computation), and folder tree/exclude-pattern display had no server equivalent. Rather than silently regressing those or migrating everything the legacy analyzer ever computed, this settles each on its own merits: - Churn: bridge now reports null (not computed) instead of a fabricated 0; the adapter's own `file.churn || 0` was silently coercing that null back into a fake zero and is fixed to `file.churn` (regression test included). Real per-file churn computation (doubling GitHub API calls per scan under one shared server token) is explicitly deferred. - Blast radius: ported to a pure function operating on GraphIR nodes/edges (src/graph-ir/blastRadius.js), proven byte-for-byte equivalent to the legacy algorithm against two real fixtures. - Folder tree, exclude patterns, and full per-function call-stats (data.fnStats -- the file-detail panel's "Functions" card is a primary, constantly-used feature, not an optional enrichment) are cheap deterministic passthroughs already computed by buildAnalysisData, now carried into graph.metadata. - Exclude patterns are validated/capped server-side (server/lib/ validate-repo-request.js), compiled via the same matching implementation index.html used (moved into src/analyzer.js, matching the MOO-67 Commit 6 precedent for shouldExcludeFile/shouldIgnoreDirectory), and included in the cache key now rather than deferred to Commit 2 -- exclusions change the graph itself. A new boundary mapper (src/adapters/repositoryGraphToViewModel.js) reconstructs the legacy view-model shape from the server's GraphIR, so the dozen-plus existing UI consumers (stats sidebar, patterns/security/ duplicates/dead-functions tabs, report export, D3 visualizations, the Functions card) keep working unchanged -- proven via a full round-trip test against real fixtures. Local-folder and ZIP analysis are completely untouched: they never called the server and still don't. Also adds an app-level CodeFlow server-token gate (previously only prompted for inside the file/function panels) and makes the GitHub PAT optional for repository analysis, since the server's own GITHUB_TOKEN now owns the scan -- the PAT remains required only for the still-legacy ownership/blame, file-preview-fallback, and PR-head-analysis features, explicitly unmigrated in this commit. Verified: full test suite (459 tests, 430 passing -- the 29 failures are pre-existing pyan3-environment failures confirmed present on main before this change), a clean `vite build`, and direct diff review confirming local/ZIP code paths are untouched. Live in-browser verification wasn't possible in this sandbox (CDN dependencies blocked by network policy); verified at the source level instead.
…, and 5 bounded fixes Addresses PR #9's round-1 review (2 blockers + 5 findings): Blocker A (parser capability): loads real acorn (pure JS, zero native deps) in server/lib/github-analyzer-bridge.js instead of stubbing it to undefined alongside TreeSitter/Babel. Fixes plain JS/CommonJS function discovery and AST-based call detection; JSX/TS AST parsing and Python/JS tree-sitter call detection remain a documented, tracked gap (docs/baseline.md). Differential fixture test added (tests/parser-capability-acorn.test.mjs). Blocker B (file-count limit): raises MAX_REPO_FILES's default from 500 to 750, matching the old client-side browser path's sampling ceiling, while keeping the hard-rejection behavior (explicit over silent truncation). Boundary tests added at 500/501/750/751. Five bounded fixes: - churn: null no longer leaks back to a fabricated 0 in the render model or the D3 tooltip. - File preview now pins to the analyzed revision instead of drifting to a repository's later HEAD. - Function source code de-duplicated between functions[] and fnStats (rehydrated client-side at the view-model boundary). - Analyzer version bumped to 1.1.0 everywhere it's declared. - Round-trip test assertions strengthened: connections compared as a canonicalized, sorted multiset instead of a lossy Set of (fn, count) pairs; function summaries deep-compared by key instead of by length. Full suite: 467 tests, 438 passing, 29 pre-existing pyan3-environment failures (confirmed identical on baseline via git stash, unrelated to this change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiAY6Poz1mtVFQmqzmzFsD
…ython tree-sitter, 5 integration corrections Addresses PR #9's second-pass review (2 blockers + 5 bounded corrections): Blocker A (acorn ordering bug): server/index.js imports analyzer-bridge.js (which stubs globalThis.acorn=undefined) before github-analyzer-bridge.js, so the round-1 presence-check guard never assigned the real module. Fixed with a value-check guard, order-independent regardless of which bridge module runs first. Added a regression test reproducing the real import order. Blocker B (Python tree-sitter): wired the real web-tree-sitter runtime (already used by pythonSymbolIndex.js) into globalThis.TreeSitter via a new shim, restoring real CST-based Python call-edge detection instead of the token heuristic. This surfaced two real bugs, both fixed: - web-tree-sitter's Parser.init() reassigns module.exports as a side effect, breaking any second independent require() of the package in the same process -- extracted a shared webTreeSitterRuntime.js singleton so pythonSymbolIndex.js and the new shim consume the same captured class reference instead of each calling require()+init(). - isPyDefName (src/analyzer.js) compared tree-sitter node wrappers with `===`, which never matches since web-tree-sitter allocates a fresh wrapper object per accessor call; switched to SyntaxNode#equals(), fixing a real over-counting bug (definitions were being counted as calls to themselves). Added a differential test suite covering strings/comments, nested scopes, decorators, attribute calls, and a for-loop-target case that caught the bug directly. Five bounded integration corrections: - File preview now resolves owner/repo from repositoryGraph.context's sourceOwner/sourceRepo (the PR head's real repo) instead of always the base repo, fixing forked-PR previews. - Cache-key exclude patterns are now lowercased before hashing, matching the case-insensitive matching semantics used everywhere else. - The legacy client-side PR-head analyzer path now uses a distinct identity (codeflow-pr-head-adapter) instead of reusing the server-authoritative codeflow-repository-adapter's name/version. - Parser.extract records the real parser path taken (acorn vs acorn-babel vs heuristic-regex) at the point the decision is made, carried through every caller into parserProvenance -- Parser .getParserProvenance() remains only as a best-effort fallback for callers that bypass extract() entirely. - The repo-input-group CSS width rule now targets a dedicated .repo-url-input class instead of an order-dependent :first-child selector, fixing the layout regression from round 1's new server-token field. Full suite: 476 tests, 447 passing, 29 pre-existing pyan3-environment failures (identical set confirmed via git stash comparison against the current branch tip). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiAY6Poz1mtVFQmqzmzFsD
…on binding fixes, provenance labeling Addresses PR #9's third-pass review (1 blocker + 3 mediums): Blocker (analyzer/cache identity): bumped ANALYZER.version to 1.2.0 in all 6 places. More substantively, installNodeTreeSitter() could silently fall back to an undefined globalThis.TreeSitter on any init failure with zero effect on cache identity -- server/routes/graph-repository.js now derives the *effective* Python parser capability from each response's own real per-node parserProvenance (derivePythonParserCapability), folds it into the cache-key options whenever a repo has Python files, and raises a warning when Python files exist but show no tree-sitter provenance. Chose this over a fail-loud startup check (the pyan3 precedent) since acorn-only/regex-fallback is an intentionally accepted degraded mode, not a hard requirement. Python tree-sitter over-counting bug: isPyDefName was missing six binding contexts (assignment/augmented-assignment targets, destructuring, the walrus operator, keyword-argument labels, and global/nonlocal declarations), each causing a rebinding or label to be miscounted as a call. Confirmed real tree-sitter node types/fields by parsing each construct with the installed grammar and fixed all six, with regression tests for each (verified to fail on the pre-fix code). This bug was already live in production for browser-analyzed repos (local-folder/ZIP), since the browser has always loaded real web-tree-sitter from CDN. Parser provenance still incomplete: extract()'s embedded-script-blocks branch (HTML/Vue/Svelte) never set fns.provenance, silently falling through to the unreliable ambient-global guess; extractJSFunctions() returned nothing at all; and a TypeScript file parsed only after stripTypeScript() was mislabeled plain 'acorn', hiding that a transform happened. Both functions now report the real path taken -- acorn/acorn-babel/acorn-typescript-strip/javascript-regex/ typescript-regex -- and multi-block embedded files aggregate to the single worst-case label across all blocks. Documentation/PR description drift: updated docs/baseline.md's parser- capability section and startup table (MAX_REPO_FILES default) to describe the current state accurately, and updated the PR description to reflect current test counts and the precise sense in which local/ZIP transport is unchanged while shared analyzer semantics evolved. Full suite: 492 tests, 463 passing, 29 pre-existing pyan3-environment failures (identical set confirmed via diff against the round-2 baseline). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiAY6Poz1mtVFQmqzmzFsD
Left over from Commit 3's baseline note (132 tests); now 492. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiAY6Poz1mtVFQmqzmzFsD
Two fixes found during live handoff verification: - server/lib/github-analyzer-bridge.js: add 422 to resolveCommitSha's errorMap alongside 404 -- GitHub returns 422 (not 404) for a syntactically-valid but nonexistent branch name on the commits endpoint, causing the smoke test's ref-not-found check to get "Error 422" instead of the expected "not found" message. Smoke suite now 21/21. - vite.config.js: add server.proxy block forwarding /api, /healthz, and /readyz to http://localhost:3000 -- without this, npm run dev + node server/index.js produced 404s for all API calls in the browser since Vite had no proxy to the backend. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Commit 1A had all three graph routes computing a canonical cache key and returning it as a permanent miss. This wires a real store behind that hook so a repeat request skips the work rather than just reporting a hit. server/lib/graph-cache.js is a Map-based LRU (insertion order gives O(1) recency) bounded by BOTH item count and bytes -- a handful of large repository graphs can exhaust a Railway container well before 200 entries. Overwrites delete before reinserting so recency actually refreshes, and an entry too large to ever fit is rejected rather than flushing the cache on its way out. Cache placement is per-layer, always ahead of that layer's expensive step: - repository: github-analyzer-bridge.js gains resolveGithubRef (ref resolution only) split from fetchAndAnalyzeRepo (tree + blobs + analysis), so the key can be built from one cheap call and the whole fetch/parse phase skipped. analyzeGithubRepo remains as the composed form for /api/analyze-repo. - file: after the GitHub fetch, before workspace staging and pyan3. - function: after symbol resolution, before CodeVisualizer. Correctness details worth calling out: Cache identity now includes request mode and ref. contextIdentityKey() keys on sourceOwner/sourceRepo@sha and deliberately omits both -- correct for "is this the same content", but not sufficient for caching a whole response: a default-branch request and an explicit ref request can resolve to the same SHA while needing different graph.context values. Without this they would share an entry and one would be served the other's context. Python tree-sitter capability is now an active startup probe (a real Language.load of the grammar) rather than a runtime check that the shim loaded. A loaded runtime does not prove the grammar is usable, and the old signal would report "capable" while every Python parse silently fell back to the regex heuristic. The probe is scoped so a missing Python grammar does not disable tree-sitter for every other language. The file layer keys on request.depth ?? 'auto' rather than the resolved depth mode, since chooseDepthMode needs a built graph -- exactly the work a hit must skip. Request-derived means lookup and storage always agree. Responses are rebuilt on every hit rather than replayed: the store holds the GraphIR, not the AdapterResult, so cache.hit and timing reflect the current request and the stored graph is never mutated to carry per-request metadata. Degraded pyan3 output is never cached -- those failures are typically transient, and storing the tree-sitter-only fallback would keep serving it for the full TTL after pyan3 recovered. Tree-sitter degradation is process-permanent and already in the key, so it is cached. CACHE_ENABLED validates strictly as "true"/"false"; a typo is a startup error rather than silently leaving caching on. The cache is process-local: cleared on restart/deploy, not shared across replicas, and assumes a single Railway instance. Stated in the startup log so an operator debugging a stale result sees it. Concurrent identical misses still duplicate work; single-flight is deferred to Commit 4. Tests: +53 (492 -> 545, all passing). Store unit tests cover TTL boundaries, LRU by count and by bytes, overwrite recency, and oversized rejection. Route tests cover key identity across every dimension -- including the mode/ref collision above -- and that rejected requests never reach the cache. An end-to-end suite drives the real handler with a stubbed global fetch and asserts on GitHub call counts, so it proves a hit skips the fetching rather than merely reporting hit: true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pu8rYHKSahwRh9mu7LcdwC
…on cache hit A cache entry shared by requests differing only in exclude-pattern case/order/whitespace was serving the first request's original casing in graph.metadata.excludePatterns to all of them, since that field came from whichever request populated the entry rather than the one being served. displayExcludePatterns() rebuilds the display form fresh from the current request on every response, hit or miss.
Add in-process LRU cache for graph analysis results (MOO-72 Commit 2)
Adds log-level gating (LOG_LEVEL) and value-level secret redaction
(exact known secrets plus common token-shape patterns) to the base
logger, fixing a field-collision bug where per-call {message: err.message}
metadata silently overwrote the real event description.
Adds a small in-process Metrics store (count/sum/max per layer+resultState
bucket) and wires an explicit, closed-vocabulary resultState field through
every terminal branch of the three graph route handlers -- success,
partial_success, cache_hit, timeout, validation_error, not_allowlisted,
github_error, parser_failure, contract_violation, dependency_unavailable,
internal_error -- so Railway's log stream can distinguish them without
parsing free-text messages. Nonterminal in-flight events (e.g. a pyan3
degradation warning) are logged separately from the one terminal outcome
per request, so a single request can't be double-counted under two
result states.
Request timers moved to the top of each handler so every rejection path,
including the earliest ones, reports a real duration and records a metric.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s split, structured fatal logging, telemetry contract tests
- Every terminal log call now carries the same resultState value passed to
metrics.record(), instead of only appearing in the metric. Previously
Railway's log stream still had to infer outcomes from free-text messages
for most branches.
- 'cache_hit' removed from Metrics' resultState vocabulary. Cache
provenance and response quality are independent: graph-repository.js
deliberately caches Python-tree-sitter-degraded graphs, so a hit can
still be a partial_success. resultState now always reflects response
quality; a new orthogonal cacheStatus ('hit'|'miss') dimension is
recorded alongside it wherever a cache lookup is meaningful (never for
states that are decided before or without reaching the cache).
- server/index.js's final main().catch() now goes through the configured,
redacted structured logger instead of a raw stderr write -- by the point
anything reaches that catch, configureLogger() has always already run.
- Added tests/route-telemetry.test.mjs: drives all three route handlers
end-to-end (network/subprocess stubbed or real-but-forced-to-fail, no
mocking of the routes themselves) and asserts the integration invariant
unit tests alone can't see -- exactly one terminal metric per request,
with layer/resultState/cacheStatus identical to the one terminal log
line for that request. Covers success, cache hit, validation failure,
timeout, dependency unavailable, and a real forced pyan3 partial_success.
Full suite: 573/573 passing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…atal log writes process.exit(1) can truncate a write still sitting in a pipe's buffer; Railway's log collector reads stdout/stderr through exactly that kind of pipe. All three fatal startup paths now set process.exitCode = 1 and return/let the promise settle instead, letting the event loop drain the pending write before the process exits on its own. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s-metrics Add structured logs and metrics (MOO-72 Commit 3)
Completes the remaining scope of Commit 1 ("Commit 1A" only migrated
repository analysis onto the server): a client-owned AnalysisSession that
repository/file/function drill-down requests must go through, real
client-side cancellation of superseded requests, server-side detection of
a disconnected client, and requestId/sessionId visible in every response.
- src/state/analysisSession.js (new): a session enforces, by construction,
that every file/function request inherits the exact revision context of
the repository graph it descends from (describeFileRequest/
describeFunctionRequest read from an adopted repositoryContext, never
from caller-supplied fields). Per-layer generation counters plus a
session-wide repository epoch make "cancelled or superseded requests
cannot update the active view" hold uniformly: a new file request
invalidates any in-flight function request (hierarchical), a new
repository request invalidates every child layer at once (epoch), and
aborting always bumps the generation (not just the controller) so a
request that ignores its own abort signal can never read as current
again.
- server/lib/cancellation.js (new): disconnect detection watches `res`,
not `req` -- req's own 'close' fires on normal body-consumption
completion too, not specifically on disconnect. Three distinct
primitives for three distinct kinds of work: withTimeout (races a
timeout + abort, for GitHub-fetch phases with no timeout of their own),
raceWithAbort (abort only, for pyan3, which already owns its own
internal timeout -- racing a second one would make its internal
degrade-to-partial_success outcome timing-dependent), and
throwIfCancelled (a synchronous preflight/checkpoint for CodeVisualizer's
CPU-bound parse, which cannot be interrupted mid-flight -- Node's
single-threaded event loop can't deliver a disconnect signal until the
parse itself returns control). A new 'cancelled' resultState is recorded
exactly once per cancelled request, never double-recorded alongside a
different terminal state.
- index.html: a session is created before the repository request is even
issued (not after it succeeds), held via pendingSessionRef/
activeSessionRef -- two refs, not one, so panels (which only ever read
activeSessionRef) can never observe a graph and a session that don't
match; promotion from pending to active happens in the same synchronous
step as the graph swap. Every teardown/reset path cancels the outgoing
session before discarding it. FileLayerPanel/FunctionLayerPanel's fetch
effects now go through the session instead of ad hoc `cancelled` closure
flags that never aborted anything.
- requestId (already in every error body) now also appears in the 6
success/cache-hit AdapterResult envelopes. sessionId, optional and
diagnostic-only, is validated as UUID-shaped and normalized to null if
malformed (never a hard validation failure), echoed on every response
from the point the body is parseable, and bound into structured logs
alongside layer/requestId.
- src/analyzer.js: ARCHITECTURE_MAX_BLOCKS 64 -> 72, expected repo-growth
maintenance this test file's own comments already anticipated.
Tests: 600/600 passing, including a real node:http server + real client
abort proving the disconnect detection works end-to-end (not a stubbed
req/EventEmitter, which would encode the wrong close semantics), and a
dedicated deterministic out-of-order/cleanup-then-late-resolution/
cross-session suite for AnalysisSession's generation/epoch invariants.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Id on pre-parse errors, real cache-write assertion - A client disconnecting while readJsonBody() is still reading the body was misclassified as validation_error in all three routes -- the stream error that produces looks the same as a genuinely malformed body from the catch block's perspective, but the response-side abort signal is already set by then. Checked first now, before any other interpretation of the error. - Pre-parse error responses (body-too-large, malformed JSON) now include requestId -- sessionId genuinely cannot be known before parsing, but requestId already can be and wasn't being sent, contradicting the PR's own "every response" claim. - tests/cancellation.test.mjs: replaced a no-op cache assertion (cache.get on an arbitrary unrelated key always returns null regardless of what actually got written) with cache.size, and added a real node:http test that declares a body, sends only half of it, and destroys the socket -- proving the mid-body-read fix above, not just the already-covered post-body-parse abort path. Full suite: 601/601 passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…strator Add unified analysis-session orchestrator (MOO-72 Commit 1B)
Two spots in this smoke script predate MOO-72 Commit 1A and never got updated for it: 1. Repository analysis was moved onto the server behind the same private CodeFlow server token the file/function panels already prompted for, but this script only ever filled the panel-level prompt -- the toolbar never got a token, so pressing Enter to start analysis silently no-op'd (analyze() calls setError() and returns when serverAuthToken is empty). No console error, no network request -- the failure had no signal this script was checking for, so it went unnoticed. 2. Once the toolbar token is filled before analysis (the fix for #1), the file-layer drill-down's own ServerTokenPrompt never appears at all -- serverAuthToken is App()-level state shared by every panel. The script's second step, which waited for and filled that prompt, would now just time out waiting for something that no longer renders. Found while trying to live-verify MOO-72 Commit 1B's session/cancellation changes against a real repository -- this script was the natural tool for that and turned out to be broken independently of anything in that commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r-token Fix stale server-token steps in function-layer-smoke.mjs
Adds category-based retryability (server + client, including transport failures and unstructured 4xx/5xx), a client-side retry cap derived from session/target identity (no resetting effect, no double-fetch), a subscriber-aware InFlightRegistry that de-duplicates concurrent pyan3 subprocess work while preserving per-caller cancellation, and per-canvas React error boundaries so a render crash in one layer can't blank unrelated panels or discard already-fetched graph data. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@OwenTanzer is attempting to deploy a commit to the braedonsaunders' projects Team on Vercel. A member of the Team first needs to authorize it. |
Author
|
Opened in error (gh defaulted to the upstream fork instead of OwenTanzer/codeflow). Closing — correct PR is on OwenTanzer/codeflow. |
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
2 participants
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
RETRYABLE_BY_CATEGORYdefaults inAdapterError, forwarded throughsanitizeDiagnostic, and normalized client-side inserverRequest.jsfor structured diagnostics, 429/Retry-After, unstructured 5xx, and rawfetch()network failures (AbortError still passes through unwrapped for existing cancellation/supersession handling).session.sessionId + session.repositoryEpoch + path[+symbolKey]rather than a resetting effect — avoids the double-fetch a separate reset effect would cause and correctly treats a new revision of the same path as a new target.InFlightRegistry(server/lib/inflight-registry.js) de-duplicates concurrent pyan3 subprocess work for the same file/package@revision. The shared operation owns its own workspace lifecycle end-to-end (staging → subprocess → path-relativization → cleanup) rather than sharing only the subprocess promise, and cancels the underlying subprocess only once every waiter has detached — one caller disconnecting never affects another still waiting on the same shared work. Scoped to the file layer only; the function layer's CodeVisualizer analysis is synchronous/CPU-bound and has no real in-flight concurrency to collapse.<svg>+ render effect) remounts on crash — not the whole panel.Test plan
npm test— 623/623 passing (baseline 601 + 22 new)tests/inflight-registry.test.mjscovers the decisive concurrency cases: one waiter aborting doesn't affect a second still-subscribed waiter; all waiters aborting evicts the entry and actually kills the underlying signal-driven work; a caller arriving after full abandonment gets fresh work, not a stale one; no unhandled rejections.tests/graph-file-inflight.test.mjsdrives two concurrent real requests for the same file through the actual route handler and pyan3 subprocess, asserting both get an identical, correct graph and the completion logs showinflightStatusexecuted/coalesced.tests/server-request.test.mjscovers the client-side retryability precedence (diagnostic > 429 > 5xx > default) and the AbortError-passthrough/network-failure-wrapping split.tests/pyan-symbol-join.test.mjsto prove the pre-relativized-path join (used by the shared registry) produces identical results to the existing workspaceDir-based join.tests/function-layer-smoke.mjsconvention) — not run in this sandbox (documented network/long-running-process limitation from prior MOO-72 sessions); recommend running locally before merge.🤖 Generated with Claude Code