Skip to content

chore(harness): complete unified agent cutover [SAP-3152] - #811

Closed
ynadge wants to merge 6 commits into
conductor/sap-3151-writable-idempotent-project-subsessionsfrom
conductor/sap-3152-unified-agent-cutover
Closed

chore(harness): complete unified agent cutover [SAP-3152]#811
ynadge wants to merge 6 commits into
conductor/sap-3151-writable-idempotent-project-subsessionsfrom
conductor/sap-3152-unified-agent-cutover

Conversation

@ynadge

@ynadge ynadge commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Primary change type

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

Problem and motivation

Complete the SAP-3147 checkpoint by removing retired project-agent compatibility surfaces, making the dead-model audit executable, and recording the final journey, disposition, Linear, rollout, and rollback evidence without adding product behavior.

Summary and scope

  • Removes retired role-bearing public types, HTTP aliases, browser mocks, and obsolete prompt/session compatibility paths.
  • Isolates deployed E2 actor and session metadata decoders to explicit migration-only modules.
  • Renames the bootstrap and map-navigation sources to their role-neutral product terminology.
  • Extends the terminology gate across production, tests, e2e, docs, and changesets with exact migration allowlist counts.
  • Adds the current design/interface/roadmap source, frozen replacement and legacy ledgers, ready-to-apply Linear reconciliation, journey evidence, and beta-first roll-forward recovery guidance.
  • Does not modify SAP-3151, add product features, merge branches, or perform unavailable Linear mutations or legacy PR closure.

Related work

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

Validation

pnpm build — passed
pnpm typecheck — passed, including web TypeScript
pnpm lint — passed with the existing repository lint boundary
pnpm --filter @sapiom/harness test — 223 files / 3,577 tests and 3 perf files / 10 tests passed
focused bootstrap/map/plan/brief/delegation Vitest matrix — 9 files / 117 tests passed
explicit E1/E2-to-neutral migration matrix — 2 files / 20 tests passed
privacy/redaction matrix — 2 files / 103 tests passed
pnpm --filter @sapiom/harness test:ui — 501 Chromium tests passed
pnpm --filter @sapiom/harness test:canvas — 11 Chromium tests passed
examples, terminology, provider-copy gates — passed; terminology audited 913 files
Harness production build — passed
Desktop dist — AppImage produced; deb packaging blocked by missing libcrypt.so.1 in this VM
Packaged smoke — blocked in this VM by missing FUSE and X server
Live opt-in simulation — complete phase one passed; unchanged cached-api-key fixture assertion failed in phase two
Analytics simulation — unchanged ingest fixture lacks the now-required authenticate dependency

Tests and documentation

Tests cover isolated legacy migration reachability, neutral context helpers, redaction needles, role-free terminology, migration, and renamed project navigation. The checked-in journey evidence explicitly preserves manual/hosted gaps; it does not report them as passes.

Compatibility and release impact

  • Breaking or externally visible changes: Retired planner-named compatibility types and HTTP/client aliases are removed. Persisted deployed metadata still migrates through isolated decoders. The existing unified-project-agents changeset documents the migration.
  • Changeset: Updated the existing unified-project-agents changeset and removed two obsolete pre-cutover pending entries.

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 implemented the bounded compatibility cleanup, terminology checks, tests, and reconciliation documentation. The exact commands and results above, the complete Harness and Playwright suites, diff hygiene, and content-free telemetry assertions were used to verify the result.

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.

Open workspace in Conductor

Mechanical CI workflow sync

Commit f6cada4 copies only .github/workflows/claude-code-review.yml byte-for-byte from freshly fetched origin/main. This already-landed workflow sync exists solely to satisfy GitHub workflow validation on the stale stacked base; its blob equals origin/main and it disappears from the eventual main-relative diff. No SAP-3151 file or behavior changed.

ynadge commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

/review

ynadge commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Exact-head review automation status: both the automatic run and the single /review fallback produced no review because the Claude action refused the stale stacked checkout. The runner reports that .github/workflows/claude-code-review.yml must be identical to the current default-branch version. SAP-3152 does not modify that workflow; origin/main advanced after the frozen SAP-3151 base. I am not merging unrelated main changes, rewriting the predecessor, or changing this PR base merely to manufacture a check. No autonomous verdict exists, so the legacy-PR closure gate remains closed.

ynadge commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

CI sync proof for exact head: commit f6cada4 changes only .github/workflows/claude-code-review.yml; its blob is 2bf016cb8d23bf7a2731f94e0bb62fb784ec2465, exactly equal to origin/main. No other default-branch file entered the worktree. This mechanical sync is solely for GitHub workflow validation and disappears from the eventual main-relative diff. Waiting for the automatic review; no duplicate /review was posted.

ynadge commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

/review

@ynadge
ynadge changed the base branch from conductor/sap-3151-writable-idempotent-project-subsessions to conductor/sap-3152-review-base September 4, 2026 23:13
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Review — PR #811 (SAP-3152 unified agent cutover)

Confidentiality: clean. The new docs/plans/** files, the rewritten changeset, and the
README edits name only Sapiom, in-repo PR numbers, and SAP-* ticket ids — no third-party
company, business arrangement, private host, or employee name. Provider vocabulary is
untouched.


1. Dropping plannerOrigin silently corrupts transcripts recorded by v0.4.3/v0.4.4

packages/harness/src/core/session-record.ts:174,180

foldSessionRecord now recognizes only payload.projectBootstrapOrigin === "infrastructure".
But plannerOrigin: "infrastructure" was written into the durable event log by released
code: planner-greeting.ts:1410 at ac9a35f3 (#765), tagged v0.4.3 and v0.4.4. Those
payloads sit in ~/.sapiom/harness/events.ndjson (collector/store.ts:394, append-only,
shared by the CLI and desktop hosts) and nothing in this PR rewrites them.

Failure scenario: a user on v0.4.3/v0.4.4 who created a Studio project upgrades. Reopening
that session's history now folds the bootstrap event with prompt: payload.prompt and
promptAt: event.ts — the private bootstrap instruction is rendered as a user message and
counted as a human turn. That is precisely what the surviving comment two lines above forbids
("its private instruction must never appear as a user message or inflate the human turn
count"), and it contradicts the changeset's own promise that valid legacy state "keeps its
… transcript".

plannerOrigin is a persisted-record key, so it belongs in the ledger's "retained legacy
strings" list alongside the E2 actor decoder — not in the deletion set. Fix: keep the
|| payload.plannerOrigin === "infrastructure" arm (allowlisted with an exact-count
terminology entry), or add a real ndjson rewrite. This is the one item in the diff that
changes behavior for already-shipped data, and no changeset line covers it.

2. Security-boundary coverage lost in the planning-session.test.tsproject-session.test.ts rewrite

packages/harness/src/core/project-session.test.ts:63

The 885-line suite was replaced with 119 lines. Most of the delta is the deleted
ProjectSessionService and is fine. But isWithinCurrentProject survives and is still the
containment half of canDispatch — the gate that decides whether ProjectBootstrapCoordinator
may inject an infrastructure prompt into a session (server/index.ts:3325). Three assertions
on it disappeared with no replacement anywhere (grep isWithinCurrentProject finds only the
new four-line test):

  • a binding with status: "missing" must not authorize its descendants;
  • C:\Users\... project roots must match C:/Users/.../agents after separator normalization;
  • a Windows root must not match a POSIX candidate (mixed path families).

Failure scenario: a future change to canonicalGraphPath or the status === "active" filter
re-admits an inactive or Windows-normalized root, and dispatch authorization silently widens
with a green suite. Restore those three cases — they are pure-function assertions and cost
nothing.

Nits

  • packages/harness/src/core/project-session-legacy-migration.ts:196import { join } sits
    at the bottom of the file, below the code that uses it (line 18). It hoists, so it works,
    but it reads as a mistake; move it to the top with the type imports.
  • packages/harness/web/src/lib/api.ts:2024 — the /** Live planner records are mutable mock state … */ doc block outlived the plannerSessionRecords field it described and now dangles
    over workflowsStore. Delete it.

Verdict: Request changes — finding 1 is a data-compatibility regression against a released
version and should be fixed before merge; finding 2 is a coverage hole on an authorization
helper. The rest of the cutover (route/type removal, isolated migration decoders with the
reachability test, the second terminology rule with exact-count allowlisting, the changeset
rewrite) is sound and correctly scoped.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — PR #811 (delta since aa28764)

One commit moved (1a23163a). Both earlier findings are fixed; no new confidentiality,
semver, dependency, or tarball issues in the delta.

Earlier findings — resolved

  • 1 (dropped plannerOrigin) — fixed. session-record.ts:171 now ORs
    isPreUnifiedInfrastructureBootstrapPayload(payload), the released key is isolated in
    project-session-legacy-migration.ts:24, session-record.test.ts:242 asserts
    turnCount === 0 and that the private instruction never appears in the folded record,
    and the changeset + ledger both name the retained compatibility. Confirmed the released
    writer (ac9a35f3:planner-greeting.ts:1410) emitted only plannerOrigin on the path
    foldSessionRecord reads; the sibling plannerAttemptId/plannerInputId keys are
    ingest-time telemetry only, so the fix is complete rather than partial.
  • 2 (lost containment coverage) — fixed. All three assertions are back in
    project-session.test.ts:63,88: status: "missing" denies its descendants, a
    C:\… root matches its C:/… descendant, and a Windows root rejects a POSIX candidate.
  • Both nits fixed (join hoisted to the top; the dangling MockApi doc block removed).

Terminology gate additions check out: scripts/ is not in STATIC_TARGETS, so the new
regex arm cannot self-trip, and the three exact-count allowlist entries match the three
real occurrences.

New — 1 finding

  • Merge target. The PR base is conductor/sap-3152-review-base, the temporary shim
    branch; the journey doc itself says that branch is "retained and never merged as a pull
    request." Merging chore(harness): complete unified agent cutover [SAP-3152] #811 as-is lands the cutover on the throwaway branch, not the SAP-3151
    base. Retarget before merge. (The workflow-sync commits are harmless: HEAD's
    claude-code-review.yml blob is 2bf016cb, byte-identical to origin/main, so it
    vanishes from the main-relative diff as claimed.)

Verdict: Approve on content — retarget the base branch before merging.

ynadge commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by the maintainer-authorized 15-part Agent Map review stack. This PR is closed without merging; its branch and review history remain preserved.

Replacement increments:

The complete testing branch remains fix/studio-onboarding-followups. Full stack: #819 through #834. Final hosted-review corrections and Mac journey validation are continuing on those same replacement PRs.

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