Skip to content

feat(harness): define shared plan versions [Agent Map 08/15] - #827

Open
ynadge wants to merge 1 commit into
review/agent-map-07-bootstrap-activationfrom
review/agent-map-08-version-contracts
Open

feat(harness): define shared plan versions [Agent Map 08/15]#827
ynadge wants to merge 1 commit into
review/agent-map-07-bootstrap-activationfrom
review/agent-map-08-version-contracts

Conversation

@ynadge

@ynadge ynadge commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Primary change type

  • Feature

Problem and motivation

Concurrent project agents need exact immutable map, plan, and brief references with consistent semantic digests and provenance.

Summary and scope

Add canonical version identities, strict codecs, immutable ancestry and restoration helpers, and the reserved brief contracts used by subsequent storage and service increments.

Strict decoding validates canonical shape and immutable lineage even when a malformed record has a recomputed digest. Public codecs and compatibility helpers are intentionally exported and documented.

How this increment fits

Version contracts support independent offline validation. Durable writers and their consumers switch together in part 9.

Stack and review boundary

  • Part 08 of 15 in the Agent Map review stack; review this increment against its predecessor.
  • Base: review/agent-map-07-bootstrap-activation.
  • Current head: d5c26aebc152f802264863cf9b9936729486048d; 2,429 changed lines across 21 files, counting additions and deletions including tests.
  • Repackages the corresponding final behavior from #806. Original code and review history remain preserved.
  • Complete coworker testing branch: fix/studio-onboarding-followups.
  • The stack remains unmerged. Dependent PRs target their predecessor, so their diffs do not repeat earlier increments.

Related work

Agent Map checkpoint SAP-3147; relevant work SAP-3149. This packaging follows the maintainer-approved 15-PR split.

Validation

Root checks ran against a5724be7cda3d37e9ddc663604c11b1eb3e3c4a2. The final head changes only README terminology or commit ancestry; a complete tracked-file comparison confirms identical executable source and build inputs. The terminology gate was rerun on d5c26aebc152f802264863cf9b9936729486048d.

pnpm build — passed (exit 0)
pnpm typecheck — passed (exit 0)
pnpm lint — passed (exit 0)
pnpm test — passed (exit 0)

Tests and documentation

Regression coverage: Canonical digests, exact references, malformed objects, unsupported versions, immutable ancestry, restored histories, and resealed lineage corruption.

See part 15 for integrated browser, native CLI, and Mac journey validation. The checks above were run independently on this PR’s own commit.

Linux tests run with ordinary user filesystem permissions; the sandbox's extra ambient capabilities are dropped. Hosted CI and automated review are separate from these recorded local results.

Compatibility and release impact

  • Compatibility: Adds public immutable map, plan, and brief contracts, codecs, digest functions, and reference helpers in a minor release. Existing documented aliases remain available.
  • Changeset: Included: .changeset/immutable-project-version-contracts.md

Security

  • No secrets, credentials, private user data, or unsanitized logs are included.
  • This PR does not publicly disclose a suspected vulnerability.

AI assistance

  • Codex assembled the implementation, addressed reproduced defects, supplied tests and documentation, inspected the diff, and ran the checks above. Reviews are handled by hosted PR automation.

Checklist

  • Read CONTRIBUTING.md; implementation follows the requested 15-PR split.
  • Description reflects this PR's actual predecessor-relative diff.
  • Relevant tests accompany the changed behavior.
  • Root build, typecheck, lint, and test evidence matches the final implementation; any documentation-only update is identified above.
  • Release/documentation treatment is explained above.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review — PR #827 (Agent Map 08/15: shared plan versions)

No confidentiality issues found: the changeset, JSDoc and fixtures are provider-neutral, and the
SAP-#### refs in build-plan.ts JSDoc match existing repo convention (src/cli/consent.ts,
src/server/rest.ts). The frontend section does not apply — the diff touches no web/src code.

Findings

1. patch changeset for ~95 new public exports (semver)

.changeset/immutable-project-version-contracts.md is patch, but src/index.ts grows from ~50 to
~145 exported symbols: ~65 new types plus ~25 new functions/constants
(canonicalJson, computeGraphContentDigest, the eight parse* codecs, the eight
build-plan-canonicalization helpers, …). Per CONTRIBUTING/the repo's own precedent, an additive
public-surface change is minor — the comparable Agent Map slice 516f13a0
(.changeset/calm-maps-arrive.md) shipped minor for exactly this shape. As written, 0.14.0 → 0.14.1 and the published CHANGELOG entry present a ~95-symbol API commitment as a bugfix.

2. Reserved contracts are exported before anything produces them

The PR body states "No build-plan, brief-refresh or delegation tool is registered by this slice",
and build-plan.ts:247 calls AgentBriefVersion a "Reserved exact-source history seam for
SAP-3150" — yet all of it is re-exported from the package entrypoint. Once these land in a published
dist/index.d.ts they are contract commitments that a later slice cannot reshape without a breaking
change, and none of them are documented in the README. Nothing outside the harness consumes them
today; keep them internal (importable from src/ within the package) until the slice that activates
them.

Worst offenders are the aliases minted in this PR and immediately made public:
computeArchitectureGraphDigest (shared/agent-map-canonical.ts:105, "Compatibility alias for
callers introduced before the neutral vocabulary"), ArchitectureSourceRef and AgentMapRevisionId
(shared/build-plan.ts:390-391), BuildPlanId (shared/build-plan.ts:23),
AgentBriefVersionRecord (shared/build-plan.ts:275). There are no pre-existing callers, because
none of this vocabulary was ever public — these are deprecated-on-arrival exports we can never
remove.

3. shared/agent-map-version-codec.ts is unreachable and untested

parseAgentMapVersion is the strict integrity decoder for a persisted map version (exact-key check,
digest recomputation, changeKind/restoredFromVersionId consistency), and it is imported by
nothing, exported by nothing, and covered by no test — the one codec in this PR without a spec.
applyPersistedMapOperations, deterministicVersionId and EMPTY_RECORD_DIGEST
(core/agent-map-version.ts:28,85,193) are likewise dead. Either wire and test them in the slice
that needs them, or delete them here; as-is a tamper-check regression in the decoder ships silently
and CI stays green.

4. The ancestry guard has no negative test

validateAgentMapVersionHistory (core/agent-map-version.ts:173) is the only gate on lineage
integrity — sequential version, parentVersionId chaining, unique versionId, recomputed
contentDigest/recordDigest, and restore-source existence — and every test either builds a valid
history or asserts not.toThrow. The test named "rejects ancestry corruption and invalid graph
topology" (core/agent-map-version.test.ts:123) only asserts the self-referential-relationship case;
the ancestry half is never exercised. A history with a skipped version, a re-pointed
parentVersionId, a duplicate versionId, or a forged recordDigest would pass this suite. Add
one table-driven case per corruption, since PR 9 makes this function the load-bearing read gate.

5. (Low) Full-history rehash on every resolver construction

AgentMapVersionResolver's constructor calls validateAgentMapVersionHistory, which recanonicalizes
and re-hashes every graph in the history. At the limits this PR itself declares —
BUILD_PLAN_VERSION_HISTORY_LIMIT = 1_024 versions, 4,096 nodes / 16,384 relationships per graph
(shared/agent-map-codec.ts bounds) — that is a full-history SHA-256 sweep per read. Nothing calls
it yet, so it costs nothing today; decide before PR 9 whether validation belongs on write instead.

Verdict

Request changes: fix the changeset level (1) and decide the public-surface question (2) before merge;
(3) and (4) are cheap and should travel with this increment rather than a later slice.

@ynadge
ynadge force-pushed the review/agent-map-07-bootstrap-activation branch from 8b08219 to bcb30f8 Compare September 5, 2026 11:57
@ynadge
ynadge force-pushed the review/agent-map-08-version-contracts branch from c3d3ded to a5724be Compare September 5, 2026 11:57
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — PR #827 (round 2)

Delta reviewed: c3d3dedfa5724be7, restricted to this PR's own diff (the base branch also
moved). No confidentiality issues in the new copy; the changeset and README additions are
provider-neutral.

New findings

1. Published README documents AgentMapRevisionId, which is not exported

packages/harness/README.md:141 lists AgentMapRevisionId among the "supported aliases" a
consumer can import. It is declared at src/shared/build-plan.ts:391 but never re-exported from
src/index.ts, and packages/harness/package.json:27 ships only the . entrypoint — no
./shared/* subpath. import type { AgentMapRevisionId } from "@sapiom/harness" therefore fails
to compile for every consumer, and the README ships that instruction inside the tarball where it
cannot be edited. The other four names in that paragraph (BuildPlanId, ArchitectureSourceRef,
AgentBriefVersionRecord, computeArchitectureGraphDigest) do resolve. Either export it or drop
it from the list; src/public-build-plan-entrypoint.test.ts is the place to pin it.

Earlier findings not fixed

  • update SDKs to use payment protocol data #2 public surfacesrc/index.ts is unchanged; the ~95 symbols and the aliases minted here
    stay public. The author's answer is the new README section, which reclassifies them as
    "supported aliases" rather than deprecating them. That is a defensible call, but it now commits
    the package to maintaining a second vocabulary permanently, and AgentBriefVersion is published
    as a contract with no producer anywhere in the package (build-plan.ts:246 JSDoc dropped the
    "no producer" caveat rather than gaining one).
  • langchain SDK config cleanup #3 dead code (second half)applyPersistedMapOperations, deterministicVersionId and
    EMPTY_RECORD_DIGEST (src/core/agent-map-version.ts:28,85,193) are still referenced by
    nothing and covered by nothing.
  • chore: update URLs, defaults, and LangChain v1.x docs #5 (low) — full-history rehash in AgentMapVersionResolver's constructor unchanged; still a
    PR-9 decision.

Fixed

Changeset now minor (#1); parseAgentMapVersion has a strict decoder spec (#3 first half);
validateAgentMapVersionHistory has table-driven negative cases for skipped version, repointed
parent, duplicate id, forged record digest and unknown restore source (#4) — each hits the intended
guard, not an incidental digest mismatch.

Nothing the earlier round claimed was wrong.

Verdict

One blocking item: fix the README/export mismatch before publish, since that README text is
permanent once shipped.

@ynadge
ynadge force-pushed the review/agent-map-07-bootstrap-activation branch from bcb30f8 to 2168491 Compare September 5, 2026 12:17
@ynadge
ynadge force-pushed the review/agent-map-08-version-contracts branch from a5724be to d5c26ae Compare September 5, 2026 12:17
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