MOO-86: move ownership/blame and file-preview-fallback server-side - #75
Closed
OwenTanzer wants to merge 113 commits into
Closed
MOO-86: move ownership/blame and file-preview-fallback server-side#75OwenTanzer wants to merge 113 commits into
OwenTanzer wants to merge 113 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.
…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>
…ity, in-flight key scope - Thread retryAfterMs through to the client Retry controls in both panels and actually gate/delay Retry on it, instead of computing and ignoring it -- a 429 no longer lets all retry attempts fire inside the same rate-limit window. - Widen the repository-layer PanelErrorBoundary key to include repository identity (sourceOwner/sourceRepo), not just resolvedSha, and an analysis-generation counter (now bumped on every local/ZIP analysis entry point too, not just the GitHub-backed ones) so the boundary resets for every new analysis target, not just a new GitHub revision. - Scope the file-layer InFlightRegistry key to the pyan3-analysis inputs only, excluding depthMode -- two concurrent requests for the same file@revision at different depths now share one pyan3 run instead of each running a full duplicate subprocess. Proven with a new test asserting exactly one workspace is created for two differently-depth- requested concurrent requests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
RateLimiter.check() now exposes retryAfterMs (time remaining in the current fixed window) whenever it rejects, and server/index.js's rate-limit branch sets a real Retry-After header (rounded up to whole seconds) from it. Without this, the client-side Retry-After gating landed in the same review round never actually engaged for the one real application-generated 429 -- retryAfterMs stayed null and Retry re-enabled immediately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…n-retry MOO-72 Commit 4: layer-specific failure isolation and retry behavior
Adds Node runtime, cache storage, Python runtime, GitHub reachability/
credential validity, and Python tree-sitter grammar checks to /readyz,
alongside the existing pyan3/CodeVisualizer checks -- normalized to one
{ok, detail, version, checkedAt} status shape and periodically
re-verified (folded into the existing 5-minute metrics interval, with a
reentrancy guard) where the underlying dependency actually supports it.
readiness-gating checks (buildOutput, workspaceRoot, cacheStorage,
nodeRuntime) determine the 200/503 status; feature-specific checks
(pyan3, pythonRuntime, codeVisualizer, pythonTreeSitter, githubReachable)
are reported but never take the whole service out of rotation.
Sensitive detail/version fields are only exposed to an authenticated
caller (the existing AUTH_TOKEN), never publicly.
The cache-storage check runs against a dedicated, isolated GraphCache
instance -- never the live, request-serving cache, which a naive
round-trip self-test would otherwise perturb (LRU eviction, capacity,
key-collision risk) on every /readyz hit.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…check verifyPythonRuntime accepted any parseable "Python X.Y.Z" output, so an operator-configured Python 2 interpreter (which prints its version to stderr in the identical shape) was reported ok:true even though pyan3/the whole adapter require Python 3 -- a misleading /readyz signal (healthy runtime, broken analysis). Extracted the parse/gate logic into a pure evaluatePythonVersionOutput() helper, now checking the major version is 3, and covered it directly with real Python 2 and Python 3 --version output shapes -- avoids an unreliable fake-executable workaround (Windows' execFile rejects a .bat stand-in without shell:true, which this codebase deliberately never sets). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…h-checks MOO-72 Commit 5: dependency and runtime health checks
…cycle WorkspaceManager gains a per-process instances/<bootId>/ namespace + PID lock file, an ownership marker (.codeflow-owned-v1), and sweepStaleWorkspaces() -- process-restart cleanup that only removes other instance directories confirmed not-alive (process.kill(pid, 0)), never a live overlapping instance, and refuses to run at all against a root lacking the ownership marker (protects a misconfigured shared WORKSPACE_ROOT from unrelated-file deletion). The workspace object gains writeFile()/copyTree() as the only sanctioned way to put content into a workspace: an ancestor-walking symlink check (not just the immediate parent), an atomic `wx` exclusive-create write (closing the TOCTOU race and the final-target-is-a-symlink case a realpath-only check would miss), and private file/directory permissions. stagePythonFiles (pyan3Adapter.js) and analyze.js's local-tree copy both route through these instead of their own raw mkdir/writeFile/cp calls -- copyTree rejects (aborts) rather than dereferences or skips a symlinked source tree. No diagnostic-artifact retention mechanism is added -- documented as a deliberate, explicit policy (docs/baseline.md): every diagnostic stays in-memory/response-only, consistent with "avoid retaining private source longer than necessary." Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…I fixture
The ownership marker was written unconditionally by ensureRoot() before
sweepStaleWorkspaces() ever checked for it -- since server/index.js always
calls ensureRoot() first, the marker was always present by sweep time,
so the "refuse to sweep an unmarked root" protection never actually
triggered in the real startup path. Fixed by having ensureRoot() record
whether the marker already existed *before* this call wrote it
(_rootWasPreviouslyOwned), and gating the sweep on that instead -- a
root's first-ever startup now genuinely never sweeps, only a second and
later startup (once a prior process established continuity of ownership)
does. Added the exact regression test the review asked for: the real,
unmodified ensureRoot()->sweepStaleWorkspaces() order against a
previously-unowned root with a stray UUID-shaped directory.
Also fixed a Linux CI failure: one malicious-path fixture
('..\..\windows\system32\config\sam') isn't actually a traversal on
POSIX, where backslash is just an ordinary filename character -- made it
Windows-only in the test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…never be swept
The previous fix only delayed the destructive behavior by one restart
cycle: a genuinely foreign instances/<uuid>/ directory (no lock file
inside it at all, so nothing proves CodeFlow ever created it) survived
the first startup (root not yet marked owned) but was deleted on the
second, once the first startup's own ensureRoot() call had written the
root-level ownership marker. The root-level marker only proves "CodeFlow
has run against this root before" -- it says nothing about whether any
specific instances/<uuid>/ directory was actually created by CodeFlow.
Fixed at the source: isInstanceAlive's boolean return (missing/malformed
lock file => "not alive" => removed) is replaced with a three-way
instanceLifecycleState ('alive' | 'dead' | 'unknown'). Only 'dead' (a
lock file naming a PID confirmed via process.kill(pid, 0) to not be
running) is ever removed. 'unknown' (no lock file, or one that doesn't
parse as a PID) is now treated the same as 'alive' -- never touched,
regardless of the root-level marker or how many restarts have happened.
Added the exact regression test requested: two full startup cycles
against a root containing a pre-existing, lock-file-less UUID-shaped
directory, confirming it survives both, not just the first.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
MOO-72 Commit 6: harden secure temporary workspaces and artifact lifecycle
Adds tests/e2e-construction-smoke.mjs -- a structural, CI-runnable smoke test proving the actual repository -> file -> function chain end-to-end at the HTTP/JSON level (against psf/requests, the same fixture function-layer-smoke.mjs already uses): GraphIR schema validation via validateGraphIR, revision propagation (resolvedSha carried unchanged from the repository response through file and function requests), central caching surviving a repeated request at both the file and function layers (never proven cross-layer before -- existing cache tests only cover the repository layer alone with a stubbed fetch), that the real render-model builders (buildRepositoryRenderModel/ buildFileRenderModel/buildFunctionRenderModel) don't throw against a really-fetched graph, and error isolation (a failed file-layer request doesn't corrupt cache/shared state for the next legitimate one). Supports an optional PRIVATE_FIXTURE_REPO env var for the "controlled private fixture where feasible" checklist item, explicitly skipped with a clear reason when unset rather than fabricated. server-smoke.mjs gets the one repository-layer ref-mode gap the audit found: a full commit SHA passed as `ref` (a distinct request shape from a named branch, even though the route internally treats both the same way), asserting resolvedSha comes back unchanged. Both scripts now read GITHUB_TOKEN from the environment before falling back to `gh auth token`, so a CI runner with no gh CLI session can supply it directly. Wires both scripts into .github/workflows/test.yml using the Actions-provided automatic token -- nothing ran in CI beyond the unit suite before this. function-layer-smoke.mjs (Playwright/browser-driven) stays local-only, a deliberate scope boundary: it belongs to MOO-71's visual-smoke path, not this structural-smoke commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Resolving the mutable default branch meant any upstream rename/removal touching sessions.py or resolve_redirects could fail this required CI check with no change on our side. Verified the pinned SHA still has the file and symbol.
MOO-72 Commit 7: end-to-end construction smoke tests
…its, version provenance, rollback runbook) Adds feature flags (FILE_LAYER_ENABLED, FUNCTION_LAYER_ENABLED, DEGRADED_ANALYSIS_ENABLED, EXPERIMENTAL_INTERACTIONS_ENABLED) surfaced via GET /api/capabilities so the UI can hide/disable a disabled layer instead of leaving a still-clickable control that just 503s. Adds a server-wide concurrency limiter shared across all five analysis routes, acquired only around the actual expensive work (creator-only for the file layer's in-flight-deduplicated pyan3 runs), with a 503+Retry-After response the client now parses correctly (previously only 429 was). Makes GitHub fetch concurrency and pyan3's subprocess buffer size configurable instead of hardcoded. Adds a single shared version/commit-provenance source (scripts/generate-build-info.mjs) consumed by both the server (/healthz) and the UI (a small corner badge), since the server runs outside Vite and a define-only injection would never reach it. Documents the confirmed current state of auth/allowlist/rate-limiting, two distinct rollback controls (release vs. domain), and the DNS/Hub cutover runbook in docs/deployment.md -- the live infrastructure steps themselves are explicitly out of this commit's scope, to be proposed individually later. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MAoVCjfAo9e7jWCRrMwyvq
…che, read RAILWAY_GIT_COMMIT_SHA - mapServerJsonResponse now honors an explicit top-level `retryable` boolean (e.g. the disabled-layer 503s) before falling back to the status-based default, so a server that explicitly says "don't retry" is no longer overridden. - fetchCapabilities now expires its cache after 30s instead of caching forever by token -- the auth token isn't a configuration version, so a long-lived tab could never observe an operator's flag toggle/redeploy. - generate-build-info.mjs now reads RAILWAY_GIT_COMMIT_SHA as a fallback between the explicit BUILD_COMMIT_SHA override and git rev-parse, so a Railway build with no git metadata in the remote build context still gets real provenance instead of "unknown". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MAoVCjfAo9e7jWCRrMwyvq
…ning MOO-72 Commit 8: production hardening, feature flags, concurrency limits, version provenance
- docs/deployment.md: fix stale prospective cutover framing (the cutover actually happened -- codeviz.moopertonic.net is live), add a cutover baseline record with live-verified deployment ID/commit SHA, replace an unexecutable "point smoke tests at the live URL" step with the real curl commands used during the actual cutover, and add dependency pinning (split into exact pins / compatibility ranges / the package-lock.json reproducibility source), cache management, log operations, recovery (workspace disk fill, crash/restart survival, GITHUB_TOKEN rotation -- each grounded in this deployment's actual ephemeral-filesystem/no-persistent-volume configuration), and per-dependency upgrade procedures. - docs/baseline.md: forward cross-reference to the now-completed cutover, without rewriting its own historically-accurate narrative. - docs/garrison-handoff.md (new): a real test matrix (known baseline, open question, suggested test input, evidence to collect, decision/exit criterion, source doc) for repository density, pyan3 correctness, function-flow correctness, PR-review usefulness (the one dimension no existing doc covers -- and the project's own original stated goal, never actually tested), broader-language possibilities, and future LLM annotation. Documentation-only commit, matching this ticket's own precedent (docs/repository-layer-density.md, MOO-69 Commit 7). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MAoVCjfAo9e7jWCRrMwyvq
…ndoff MOO-72 Commit 9: finalize operational documentation and Garrison handoff
Removes the client-side GitHub PAT/App toolbar fields entirely -- the only two features that consumed them (the ownership/blame panel and the file-preview GitHub fallback) now run through two new server routes (POST /api/github/blame, POST /api/github/file-content) using the server's own GITHUB_TOKEN, same as the rest of the app. Eliminates a real exposure (a raw PAT sitting in unprotected page state) with no loss of functionality. The legacy client-side "Analyze Pull Request" flow still calls the GitHub API directly and is left unauthenticated (its existing degraded rate-limit behavior), since migrating it is a separate, larger effort.
|
@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 against the wrong repo by mistake (gh defaulted to the fork parent) -- recreating against OwenTanzer/codeflow instead. |
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
Part of MOO-86 ("Reduce friction: credentials, and file/function-layer graph usability") -- the credential half.
Removes the client-side GitHub PAT/GitHub App toolbar fields entirely. Previously, analyzing a repository required two separate credentials: the CodeFlow server's own
AUTH_TOKEN, and a second, separate client-side GitHub PAT/App used only by two legacy features (ownership/blame, file-preview-fallback) that still calledapi.github.comdirectly from the browser.POST /api/github/blameandPOST /api/github/file-content-- lightweight server routes using the server's existingGITHUB_TOKEN, same auth/allowlist/rate-limit gate as every other/api/*route. Deliberately lighter-weight than the three graph routes (no cache/concurrency-limiter/metrics needed for a single cheap GitHub call).src/state/githubMetaClient.js, mirroring the existing graph-endpoint client pattern (src/state/serverRequest.js).selectFile(ownership/blame) andopenFilePreview(GitHub fallback) inindex.htmlnow call these server routes instead ofGitHub.getBlame/GitHub.getFiledirectly.jsrsasignCDN dependency, and the now-deadGitHub.generateJWT/getRepoInstallation/getInstallationToken/authenticateApp/getBlamemethods.Scope note: the legacy client-side "Analyze Pull Request" flow (
analyzePR/analyzePRHead) also read the removed client token, but does its own real client-side fetch/parse/churn computation distinct from the server's GraphIR pipeline -- migrating it is a separate, larger effort. Per discussion, it's left as-is: it now always runs unauthenticated (GitHub's public rate limit), same degraded behavior it already had for any user without a token.Net effect: one fewer credential to set up, two real security/UX wins (no more raw PAT sitting in unprotected browser state), zero feature loss.
Test plan
architecture-diagram.test.mjs's block-count-sensitive "Testing" subgraph assertion -- confirmed viagit stashthat it fails identically on a cleanmaincheckout, before any of this PR's changes)npm run buildsucceedsserver/routes/github-meta.js,server/lib/validate-github-meta-request.js,src/state/githubMetaClient.js) import without errorCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com