Skip to content

fix(harness): fence off retired project graph routes - #892

Open
ynadge wants to merge 5 commits into
mainfrom
yashnadge/sap-3089-retire-server-graph-authority
Open

fix(harness): fence off retired project graph routes#892
ynadge wants to merge 5 commits into
mainfrom
yashnadge/sap-3089-retire-server-graph-authority

Conversation

@ynadge

@ynadge ynadge commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Primary change type

  • Bug fix
  • Documentation
  • Feature
  • Tests
  • Dependency update
  • Maintenance or refactor

Problem and motivation

Current Studio already uses durable Agent Map state, but stale System Graph requests could still resolve scopes and retain graph watcher/inventory work. A second topology authority remained reachable during identity failures.

Summary and scope

Return an authenticated 410 legacy_graph_retired before old graph reads, refreshes or navigation resolve a scope. Retain no current-server legacy graph owners and avoid their inventory invalidation when no graph consumer exists. Shared discovery and ordinary session watchers continue to operate.

This is the server layer of SAP-3089. The graph implementation stays unreachable until SAP-3091; the next layer (#893) handles browser identity recovery and retirement evidence.

Release dependency: #892 must not be released ahead of #893. The server-only diff still contains the older browser fallback during catalog failure; #893 supplies its unavailable-map recovery. A checked-in .release-blocked file makes pnpm version-packages and pnpm release fail before changing versions, building or publishing. The Release PR, npm Publish and Desktop Release workflows also run the check immediately after checkout. #893 removes the blocker with the client recovery. The breaking minor changeset stays in this server layer, so unrelated pending patches cannot turn the endpoint retirement into an unannounced patch.

Related work

Related issue or discussion:
https://linear.app/sapiom/issue/SAP-3089

Validation

Current head: 5870d0f5, including main b460c9aa.

  • GitHub Test: full workspace build, typecheck, lint and tests passed on Node 20 and Node 22. Each Harness run passed 4,039 tests (two existing skips) plus all 10 performance cases. Examples, repository content checks and CI security tests passed.
  • GitHub Harness: 643 Studio browser tests + 15 canvas tests passed, with no failed or flaky cases. Packaged Linux desktop smoke passed 16 checks, with one Windows-only skip.
  • CodeQL and PR classification passed. All 11 reported checks are green; this is CI status, not GitHub reviewer approval.

The CI repair reproduces and fixes four startup failures also present on current main: the definition-list fixture omitted credentialsFilePath from its auth mock. The mock now points the credential observer at a fixture-owned temporary path. All 32 related auth, observer, discovery and wiring tests passed locally after the fix. The combined stack also passed Harness build/typecheck and 19 authority/session-tab browser tests, including the newer MCP restart flow.

The release check passed all 178 script tests and nine desktop release-tag tests. Actual server-only pnpm release:check, pnpm version-packages and pnpm release each exit 1 with the required-client explanation before downstream work. All three release workflows check immediately after checkout; desktop publication depends on the guarded prepare job. The full Changesets release-plan API confirms the breaking minor wins over pending patches. No versions or published changelogs were changed.

Tests and documentation

The wiring test proves protected 410 responses, zero legacy reads/refreshes/watchers, and unchanged ordinary sessions for root and descendant scopes. Existing discovery tests now use their shared scan/session consumers and retain cold inventory, renames/deletions, latest-scan budgets, repository boundaries, final-lease retirement and symlink coverage. The shipped README and HTTP contract mark all three endpoints as retired and document migration to durable project IDs, map reads and exact node navigation. This layer includes the breaking minor changeset and an executable release check. The next stack layer supplies client recovery, removes the release blocker and records the complete authority matrix and packaged evidence.

Compatibility and release impact

  • Breaking or externally visible changes: The documented GET /api/workspaces/:workspaceKey/system-graph, POST /api/workspaces/:workspaceKey/system-graph/refresh, and GET /api/workspaces/:workspaceKey/system-graph/navigation routes now return authenticated 410 with error: "legacy_graph_retired". Migrate to GET /api/projects/:projectId/agent-map/workspace and exact node implementation navigation using server-issued project IDs. The matching browser recovery in fix(harness): recover Agent Map identity without legacy fallback #893 must ship in the same release.
  • Changeset: Added .changeset/quiet-project-map-authority.md as a breaking Harness minor with all three retired routes, legacy_graph_retired and migration guidance. The complete pending changeset queue, including the pending Harness patches, resolves to 0.17.0. .release-blocked prevents the configured versioning and release entrypoints from proceeding until fix(harness): recover Agent Map identity without legacy fallback #893 is included.

Security

  • I have not included secrets, credentials, private data, or unsanitized logs.
  • This pull request does not publicly disclose a suspected vulnerability. I
    will follow the
    Security Policy for
    private reporting.

AI assistance

  • I did not use AI assistance for this change.
  • I used AI assistance and have described it below.

Codex researched and implemented the change. An independent agent reviewed the authority and session boundaries; build, typecheck, lint, integration/browser tests and packaged smoke checks verified the combined stack.

Checklist

  • I read CONTRIBUTING.md, and this contribution follows the direct-PR or issue-first policy.
  • This pull request addresses one focused problem and contains no unrelated cleanup.
  • I added or updated tests, or explained above why tests are not applicable.
  • I ran the relevant build, typecheck, lint, and test commands, or explained
    any N/A checks above.
  • I updated documentation for user-facing changes, or marked it N/A above.
  • I added a Changeset for a published-package change, or explained why it is not applicable.
  • I can explain and maintain every submitted change, including any AI-assisted work.

Refs SAP-3089. Reject protected legacy requests before resolving scopes or retaining graph owners, while preserving shared discovery and session watcher coverage.
@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 15514496-c2d7-49d0-913a-f91f9835c3fa

📥 Commits

Reviewing files that changed from the base of the PR and between 452a9f8 and d3c9135.

📒 Files selected for processing (4)
  • .changeset/quiet-project-map-authority.md
  • packages/harness/src/server/index.ts
  • packages/harness/src/server/studio-workspace-wiring.test.ts
  • packages/harness/src/server/system-graph-freshness.test.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The harness retires legacy system-graph routes, limits graph invalidation to active scopes, and preserves workspace discovery through workflow scans and inventory. Tests now validate explicit workflow discovery, session lifecycle behavior, project identity, and symlink handling.

Changes

Legacy graph retirement

Layer / File(s) Summary
Retire graph routes and inactive scope handling
.changeset/..., packages/harness/src/server/index.ts
Legacy graph requests return HTTP 410. Graph invalidation runs only for active scopes. Failed catalog access no longer retains legacy scopes.
Validate workspace wiring after retirement
packages/harness/src/server/studio-workspace-wiring.test.ts
Tests verify authentication responses, retirement payloads, no graph operations, empty graph scopes, preserved project identity, and recent-directory cleanup.
Move freshness tests to workflow discovery
packages/harness/src/server/system-graph-freshness.test.ts
Tests use explicit workflow scans, /api/workflows, persisted workflow data, session-only reconciliation, and real-path checks instead of legacy graph authority.
Retire leases through session lifecycle
packages/harness/src/server/system-graph-freshness.test.ts
Lease tests use bootstrap-session termination, agent definition removal, retained inventory, and recovery scans.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to d3c91

The legacy graph endpoints are retired without disrupting replacement discovery or ordinary sessions, and no merge-blocking risk is evident.

Suggested reviewers: gwitwer

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main change: retiring and isolating legacy project graph routes in the Harness server.
Description check ✅ Passed The description includes all required sections, explains the motivation and scope, identifies related work, documents validation, describes compatibility and release impact, records security and AI-us…
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch yashnadge/sap-3089-retire-server-graph-authority

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ynadge
ynadge added this pull request to stack #894 September 9, 2026 20:25

ynadge commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Review — PR #892 (round 1)

1. The bundled UI still routes users into the retired endpoint — same version, no recovery

packages/harness/src/server/index.ts:3967 returns 410 for all three graph routes, but the
web bundle shipped in this same @sapiom/harness build still has a live code path to them:

  • packages/harness/web/src/App.tsx:1714 — when a rail-selected workspace scope has no
    projectId, the handler sets selectedProject instead of a Studio selection.
  • App.tsx:1446studioView is then null, so App.tsx:3627 renders
    WorkspaceGraphView.
  • WorkspaceGraphView.tsx:110 calls getSystemGraph, gets 410, and setError(true).

That scope-without-projectId state is not hypothetical: listWorkspaceScopesAndRetain
preserves un-reconciled scopes verbatim and, in the catch, returns every scope with no
identity — the exact case this PR's new comment calls out ("Keep folders/sessions reachable
when identity storage is unavailable"). So the user-visible result of an identity/catalog
failure changes from "legacy map renders" to "error card". The 410 body says "Reload Studio
to use Agent Map"
, but reloading serves the same bundle and re-enters the same fallback the
moment the user clicks that project again; there is no Agent Map to reach without a
projectId.

The PR body defers this to #893, but #892 is independently publishable — if it releases
alone, that regression ships. Either land the client half in this PR, or state in the PR
that #892 must not be released ahead of #893 and hold the changeset until they merge.

2. Changeset is a plain patch for the removal of documented HTTP endpoints

.changeset/quiet-project-map-authority.md is a bare patch with no Breaking marker and no
migration note. This repo's own precedent for exactly this class of change is explicit —
packages/harness/CHANGELOG.md:133 ("Breaking changes for embedders and HTTP clients") and
:103/:125 ("Breaking for embedders (minor while @sapiom/harness is pre-1.0)"). A
consumer on ~0.16 who reads only the changelog gets "background work is rejected" and no
signal that GET /api/workspaces/:workspaceKey/system-graph, /refresh, and /navigation
now return 410. Make it a minor, mark it Breaking, name the three routes and the
legacy_graph_retired code, and point at the Agent Map replacement.

3. Shipped README still advertises the retired endpoints as a live contract

packages/harness/README.md:335-337 lists docs/workspace-system-graph.md under "HTTP
contracts that need more than a type to use" — "the Project dependency-graph endpoints,
lifecycle states, cache signal, warnings, and system-graph.changed event". README is in
package.json files, so that text ships in the tarball describing routes that now 410.
packages/harness/docs/workspace-system-graph.md:33-34,83 documents the request/response
shapes as current. Both need a retirement note in this PR, not in #893.

Nits

  • activeSystemGraphScopes is written only by onScopeAccess (index.ts:3987), which is
    now unreachable behind the 410, so the new if (activeSystemGraphScopes.size > 0) guards
    at :1947 and :2275 are permanently false and retained = new Set<string>() at :2892
    makes the delete-loop below it dead. Harmless, but it reads as if a graph consumer could
    still exist; a comment saying the map is now always empty would save the next reader.
  • The symlink test dropped the node_modules symlink setup along with the graph assertions
    (system-graph-freshness.test.ts); if scanner exclusion of a symlinked node_modules was
    only covered there, that coverage is gone.
  • No confidentiality issues: the changeset prose is generic, and SAP-#### refs in source
    comments match existing repo practice (cli/consent.ts:4, profiles/introspection, etc.).

Verdict: Request changes — the server change is sound and well tested, but it ships a
bundled UI path that dead-ends on the new 410, under a patch changeset and a README that
still document the removed endpoints.

Hold the breaking release note for the client recovery layer and document the retired HTTP contract. Restore linked dependency exclusion coverage.

Refs: SAP-3089
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Review — PR #892 (round 2, delta since d3c9135)

New finding — deleting the changeset made the release gate weaker, not safer

Round 1 offered "hold the changeset until both layers merge" as an option, and the push took
it. But .changeset/ already holds four pending @sapiom/harness: patch entries
(archive-before-retention, mcp-alias-role-names, order-studio-auth-transitions,
template-harness-selection). A release PR cut from main with #892 merged and #893 not
publishes a patch of @sapiom/harness in which:

  • GET/POST /api/workspaces/:workspaceKey/system-graph{,/refresh,/navigation} return 410
    with zero changelog mention — a pinned consumer sees only unrelated patch notes;
  • the tarball ships docs/workspace-system-graph.md telling users the recovery is Studio's
    Reload projects action, which exists only in fix(harness): recover Agent Map identity without legacy fallback #893 (grep "Reload projects" on this
    branch: no hits) — so the doc's own migration path is unreachable in that bundle.

The ordering is enforced only by a paragraph in the PR body. Either keep a minor
changeset here marked Breaking (naming the three routes, legacy_graph_retired, and the
Agent Map replacement) and gate the version PR on #893, or merge both layers together.

Round-1 findings, status

  • cleanup HTTP SDKs #1 bundled UI dead-ends on 410 — not fixed in code; addressed only by the PR body's
    release-dependency paragraph. Acceptable per round 1's stated option, but see above.
  • update SDKs to use payment protocol data #2 patch changeset — "fixed" by deletion; see the new finding.
  • langchain SDK config cleanup #3 README/docs advertise live endpoints — fixed. README:335 and the doc's new
    retirement header + migration section are accurate against agent-map.ts:318 and
    api.ts:678, and the 410 mount at index.ts:3970 matches the documented ordering.
  • Nits — both fixed: the always-empty comments are in, and the symlink test now
    scaffolds a discoverable agent under a linked node_modules and asserts exclusion through
    both the canonical and symlinked scans.

Nothing round 1 got wrong. No confidentiality issues in the new prose.

Verdict: One blocker left — restore a Breaking minor changeset on this layer (or merge
the two layers as one), since the pending patch queue can publish the 410 unannounced.

Restore the breaking minor retirement changeset on the server layer. Check the pending client blocker before versioning, npm publishing and desktop releases so unrelated patch changesets cannot ship it alone.

Refs: SAP-3089

ynadge commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Review — PR #892 (round 3, delta since 551837e)

New — the gate turns every push to main into a failed Publish and Release PR run

.release-blocked is enforced by a hard exit 1 at the top of publish.yml,
release-pr.yml and the desktop prepare job — all three of which run on every push
to main
, not only on a version-PR merge. publish.yml's own comment states the run
"is a safe no-op on pushes that aren't a version-PR merge"; from #892 merging until #893
merges, that no-op becomes a red X on every commit. Two consequences: an intentional
block is indistinguishable from a genuinely broken publish pipeline, and every other
package (cli, mcp, agent-core, harness-desktop) is frozen too, including a
hotfix.

The hard failure is right for local pnpm version-packages / pnpm release. In CI, have
the step set an output and gate the downstream steps with if:, so a blocked release
reports green-with-skipped rather than failed.

Nit — the changeset describes #893's UI in an unretractable surface

.changeset/quiet-project-map-authority.md's last paragraph ("Agent Map unavailable"
/ "Reload projects") is client behavior that ships in #893, which already carries its
own .changeset/calm-map-identity-recovery.md for it. The published 0.17.0 CHANGELOG
will state the same recovery twice, and this copy cannot be corrected if #893's wording
moves before merge. Drop the paragraph or reduce it to a one-line pointer.

Earlier findings, status

  • R2 blocker (Breaking minor changeset) — fixed. minor, "Breaking for HTTP
    clients", all three routes, legacy_graph_retired, and migration guidance that checks
    out: /api/state does return studioProjects (packages/harness/src/server/rest.ts:365).
  • R1 cleanup HTTP SDKs #1 (bundled UI dead-ends on 410) — still unfixed in code, now backed by a
    machine-checked gate instead of PR prose. Acceptable per R1's stated option.
  • R1 update SDKs to use payment protocol data #2, langchain SDK config cleanup #3 and both nits — remain fixed.
  • Nothing earlier rounds got wrong.

No confidentiality issues: .release-blocked carries only a SAP- id and bare PR
numbers, and the root package is private, so it never enters a tarball.

Verdict: Approve with one CI change — make the blocked-release path skip rather than
fail on ordinary main pushes.

Provide the credentialsFilePath export used by the new shared credential observer, pointing it inside each fixture's temporary directory. This repairs the four CI startup failures after the main-branch auth integration.

Refs: SAP-3089
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant