Project identity: an id that survives the founder's other laptop - #111
Open
TruongGiang2000 wants to merge 12 commits into
Open
Project identity: an id that survives the founder's other laptop#111TruongGiang2000 wants to merge 12 commits into
TruongGiang2000 wants to merge 12 commits into
Conversation
Two errors in the first draft of the spec, both found by reading the code to write the plan rather than by reasoning about it, both corrected in place with a note saying where they were wrong: The prerequisite was already built. The spec named ProjectStore.activeProjectPath as the thing to wire into the chat lane. But CompanyStore.activeProjectLink already exists, already persists as a security-scoped bookmark, and already carries path/isGitRepo/hasClaudeMd — and it is the better anchor of the two, because a linked folder is the founder's statement where an inferred cwd is a guess about what they are doing. The scope key was machine-specific. The spec had scope holding a project path and the cloud document keyed on a hash of one. The same repo sits at a different path on a second machine, so every repo-scoped fact would have orphaned there — silently, which is the exact failure class this design exists to remove. It also defeated its own §4.2: syncing the project tier is worthless if the key that finds it only exists on one machine. Scope now holds an opaque minted id, and §4.3 defines how a folder resolves to one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
… that matches it A project's identity has to survive the founder opening their other laptop. An absolute path does not, and neither does a hash of one — the same repo lands under two keys and every fact scoped to it is silently orphaned. So the id is minted once and carries nothing about the machine. normalizeRemote reduces a remote to host/owner/name so an ssh clone on one machine and an https clone on another compare equal. Without it, protocol choice alone would split one project in two.
Nil on every failure rather than throwing — not a repo, no remote, git missing, path gone. A folder with no remote is an ordinary case, not an error, and every caller handles it the same way: ask the founder instead of guessing.
The original tests could not distinguish the guard on result.ok from the empty stdout check — both failure cases write nothing to stdout. Injecting the runner function allows tests to construct GitResult values that fail with non-zero exit and present stdout, proving the guard is essential. Added three new tests: - test_remoteURL_nilWhenTheCommandFailsEvenWithStdout (catches guard deletion) - test_remoteURL_returnsTheTrimmedURLOnSuccess (happy path with whitespace) - test_remoteURL_nilOnSuccessWithBlankStdout (successful command, empty output)
… orphan facts
The path names a location on one machine; the id is what DecisionEntry.scope will store
and what has to mean the same thing everywhere. Many paths mapping to one id is the
supported case — that is the whole reason for the indirection rather than scoping facts
by path directly.
Two levels, keyed by account, because a flat map has no correct behaviour on sign-out. A
project id only means something inside one founder's companies/{uid}: clear the map and
the same founder coming back re-links their folder, mints a new id, and silently orphans
every fact scoped to the old one; keep it across a switch and the next founder inherits
bindings pointing at projects they cannot see. With two levels, signing out is
account = nil and destroys nothing.
Injectable defaults for the same reason PetMemoryStore has them: a test writing to the
real key could have its cleanup clobbered by a running app and eat real bindings.
A matching git remote returns .propose, not .bound. Adopting it silently would attach one repo's memory to another with nothing on screen — and a duplicate project is the cheaper mistake of the two: it is visible and it can be merged, where wrongly merged memory is neither. Two cloud projects sharing a remote mints instead of picking one. Choosing either would be a coin toss with the founder's facts. folderName never decides anything: two unrelated checkouts called api are ordinary, so it is shown to the founder rather than acted on.
…sure activeProjectId stays nil while a proposal is unanswered. Everything downstream reads that property, so an unconfirmed guess cannot scope a fact by accident — the guard is the nil, not a flag somebody has to remember to check. pendingProjectMatch carries the normalised remote that produced it, so the founder is shown why the match was proposed instead of being asked to trust it. Rejecting mints a fresh id rather than leaving the folder unresolved, because an unresolved folder scopes nothing and reads as the feature quietly not working. hydrate points the identity map at the incoming account and reset clears only that pointer, never the bindings. Clearing them would mean the same founder signing back in re-links their folder, mints a second id, and silently orphans every fact scoped to the first — which is the failure this whole design exists to remove.
@mainactor gave this class an isolated deinit, and on Xcode 26.2 that stops its whole XCTest suite from executing rather than merely crashing at teardown: the xcresult recorded all 11 ProjectIdentityMapTests as Failed with "Executed 0 tests" (the same pattern the pre-existing EngineeringRunStoreTests shows, so not a regression, but not something a NEW type has to inherit either). ProjectIdentityMap is a plain UserDefaults wrapper with no actual need for actor isolation, so dropping @mainactor costs nothing and buys back the whole suite: 11 executed, 0 failures. CompanyStore.swift and both test files needed no adjustment — verified via a full build and build-for-testing before running anything.
ProjectProbe checks for .git in that exact folder and does not walk up; git remote get-url does walk up. Those are two different definitions of "is a git repo", and reading the remote unconditionally conflated them: a folder nested inside a tracked ancestor reported the ANCESTOR's remote as its own, and could be proposed as the ancestor's project. The founder still had to confirm, so nothing mis-scoped silently — but it put a wrong question in front of them with plausible-looking evidence attached, which is the same failure wearing a human in the loop. repoRoot compared against the linked path is what tells the two definitions apart. A folder that is not its own root now mints instead, which is the safe direction. Also: a proposal now carries the path it was made for. confirmProjectMatch and rejectProjectMatch previously resolved against activeProjectLink?.path, so a founder who linked a second folder before answering the first proposal had their answer applied to the wrong folder.
…anyStore-backed suite executing Final-review fix wave over feat/two-tier-memory PR 1 (base 67084a3). CompanyStoreChatTests hydrates a real account before calling linkProject, so its store was binding into the app's real cp_project_ids_v1 on .standard on every run — the "these suites never hydrate so nothing lands" comment in CompanyStoreProjectIdentityTests was false for exactly that reason. Fixed by injecting a ProjectIdentityMap on its own test suite there and, defensively, in ChatModeEngineeringTests and CompanyStoreCodeRunTests too, since neither should depend on staying account-less by luck. Deleted the ~140 stray cp.tests.*.plist files and the (already-absent) cp_project_ids_v1 key those runs left behind, and rewrote the false comment to describe what the suites actually do now. ProjectIdentityMapTests' `defer { UserDefaults().removePersistentDomain(forName: name) }` never worked: it called the method on a fresh UserDefaults() instance instead of the one that actually held the data, so the domain was never removed. Fixing that alone still left files behind, because removePersistentDomain only empties a plist's content — it does not delete the file — so an explicit FileManager.removeItem was added too. Even with both fixes, cfprefsd flushes a suite's plist on its own schedule, and a late flush can recreate the file after we deleted it (measured: 0, 0, 8, 8, 0 leftover files across 5 consecutive runs). There is no synchronous fix for that from inside the test process, so instead of chasing a per-run guarantee this class now sweeps every cp.tests.*.plist at the START of its run, bounding growth to at most one run's stragglers instead of the unbounded accumulation that produced 140 files. CompanyStoreProjectIdentityTests compiled and reported "Executed 0 tests" / exit 65 on every run, with codepet-*.ips crash reports showing abort -> ___BUG_IN_CLIENT_OF_LIBMALLOC_POINTER_BEING_FREED_WAS_NOT_ALLOCATED -> swift_task_deinitOnExecutorImpl -> CompanyStore.__deallocating_deinit — CompanyStore is a @mainactor ObservableObject with an isolated deinit, and letting one deallocate mid-test aborts the host on this toolchain. Retaining every CompanyStore the class creates in a never-cleared `retained` array fixes it: 10 executed, 0 failures, confirmed stable across 3 runs with zero new crash reports. Applied the same fix to CompanyStoreCodeRunTests, which showed the identical pattern pre-existing on the base branch: now 2/2. Could not find a clean reason ChatModeEngineeringTests and CompanyStoreChatTests run fine without this — both also construct and deallocate CompanyStore — so that gap is reported as unexplained rather than papered over with an invented rule. Also: extended test_linkingWritesNothingWhileNoAccountIsSet to assert activeProjectId stays nil (companion to the CompanyStore.swift fix in the next commit), and added test_symlinkedPathStillMatchesTheResolvedRoot, which links via an unresolved symlink path while injecting the RESOLVED path as forcedRoot — the case every prior test in this file structurally could not see, since they all inject the same raw string on both sides. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…ded bind, and tidy dead code Continuing the PR 1 fix wave (base 67084a3). linkProject compared a raw incoming path against `git rev-parse --show-toplevel`, which canonicalises symlinks — so a folder reached through a symlinked ancestor could genuinely BE the repo root and still fail the isRoot check, silently dropping a legitimate remote hint. Fixed at the source: linkProject now canonicalises the path once with URL(fileURLWithPath:).resolvingSymlinksInPath().path and uses that single canonical value for the probe, the CLAUDE.md write, the bookmark, the isRoot comparison, and every identityMap call downstream. Otherwise the same repo reached via a symlinked path and via its real path would bind to two different map keys — two ids for one repo, the exact failure this opaque id exists to prevent, and PR 1 is the cheapest point to fix it because nothing has bound yet. adopt(id:for:) set activeProjectId unconditionally, so a link before hydrate (or one that outlives a sign-out) produced a live id backed by nothing on disk. It now only sets activeProjectId when identityMap.id(forPath:) confirms the bind actually landed. ProjectIdentityMap.reload() had no caller anywhere in the repo (verified before deleting) and no test — it existed only because the plan's Task 5 instruction pointed at the wrong file (PetMemoryStore.shared.reload(), which CompanyStore never calls). resetAll() stays: also uncalled in PR 1, but it's tested and documented as the deliberate-full-reset escape hatch a later PR is expected to wire up. CompanyStore.knownCloudProjects becomes `let` — nothing mutates it before PR 2's cloud sync gives it a writer, and `var` was sending the next reader hunting for one. The `nonisolated` comment on ProjectIdentityMap now says plainly that it is main-actor-by-convention only and NOT thread-safe, so the annotation can't be misread as a safety claim. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…ecord what shipped differently Final commit of the PR 1 fix wave (base 67084a3). Spec §5.2 claimed the linked project "survives relaunch as a security-scoped bookmark under cp_active_project_bookmark." It does not: linkProject writes that bookmark and nothing in the repo ever resolves it (grep -rn resolvingBookmarkData codepet/ returns nothing), so activeProjectLink — and therefore activeProjectId — is nil at every cold start until the founder re-links by hand. Corrected in place, with the line's stale references (:42, :558, :2324) replaced by symbol names, which don't rot the way line numbers do. Added a "PR 2 preconditions" subsection stating two hard requirements the next PR must satisfy: a founder-facing affordance for pendingProjectMatch has to exist before knownCloudProjects is ever populated (a proposal nobody can answer leaves activeProjectId permanently nil), and the bookmark has to be resolved on launch or the repo tier this whole design exists to add is empty every cold start. Appended "Corrections applied during implementation" to the plan doc, which is committed while the SDD ledger it was executed against is not: Task 3's flat map became account-keyed to survive sign-out/sign-in without orphaning bindings; Task 5's two conflicting makeStore signatures became one; the async cloud-project reader the plan suggested stayed synchronous, since knownCloudProjects is empty in PR 1 by construction; reload() — which the plan pointed at the wrong file for — was dropped in favour of the account pointer CompanyStore already sets; the isGitRepo gate was removed and then replaced by repoRoot/isRoot once a review caught the ancestor-remote hole that removal opened; and the isRoot canonicalisation fix from this wave, which nothing had written down before now. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
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
1 participant
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.
PR 1 of the two-tier memory feature. It gives a linked project folder a stable, opaque identity. It does not scope any memory yet —
DecisionEntryandChatContext.composeare untouched. Identity has to be right first, because everything later hangs off it.Design:
docs/superpowers/specs/2026-08-21-two-tier-memory-design.md· Plan:docs/superpowers/plans/2026-08-22-two-tier-memory.mdWhy an opaque id and not a path
The first draft of the design had
scopeholding a project path, and the cloud document keyed on a hash of one. Both are machine-specific: the same repo sits at a different path on a second machine, so every repo-scoped fact would have orphaned there — silently, which is the exact failure the feature exists to remove. It also defeated its own purpose, since syncing the project tier is worthless if the key that finds it only exists on one machine.So a project gets an id minted once, carrying nothing about the machine. Each machine keeps its own path→id map locally; the id is what travels.
The rules this establishes
folderNamenever decides anything: two unrelated checkouts calledapiare ordinary.activeProjectIdis nil while a proposal is unanswered, so an unconfirmed guess cannot scope anything. The guard is the nil, not a flag somebody has to remember to check.ProjectProbechecks.gitin that exact folder;git remote get-urlwalks up. Conflating those made a folder nested inside a tracked ancestor borrow the ancestor's remote and get proposed as the ancestor's project.Tests
88 across seven suites, 0 failures, each run per-suite (
-only-testing:) because a whole-target run exits 65 on this toolchain.ProjectIdentityTestsProjectIdentityMapTestsGitRemoteTestsCompanyStoreProjectIdentityTestsCompanyStoreChatTestsChatModeEngineeringTestsCompanyStoreCodeRunTestsTwo suites were reporting
Executed 0 testsbefore this branch fixed them —CompanyStoreCodeRunTestswas doing so onmain, unrelated to this work. The cause was an isolated-deinit abort when a@MainActorstore deallocated inside a test body; retaining the stores fixes it.ProjectIdentityMapisnonisolatedfor the same reason, and it needed no isolation anyway.Also fixed:
CompanyStoreChatTestswas hydrating and then linking a project against the realUserDefaults, writing intocp_project_ids_v1on every run. Four suites now inject their own suite.Known and deliberately left
knownCloudProjectsis empty in production until PR 2 lands the cloud write side, so only the.mintbranch runs today. An id minted now is the id forever, so nothing is lost.normalizeRemoteleaves a port inssh://host:2222/o/r, and bare/file://remotes yield a path-shaped hint. All fail toward minting a duplicate — visible and mergeable.resolvingSymlinksInPath()deliberately does not resolve/var,/tmpor/etcthemselves, so a repo living directly under one of those could still mismatch.UserDefaultssuite without removing it; growth is bounded only by a sweep inProjectIdentityMapTests. Local dev hygiene, not correctness.CompanyStore-constructing suites never needed the retain fix. Recorded rather than papered over.Two hard preconditions for PR 2
knownCloudProjectsis ever populated.confirmProjectMatch/rejectProjectMatchhave no caller outside tests. The moment that list fills, a proposal leavesactiveProjectIdnil with no way to answer — memory silently unscoped, permanently, for that folder.linkProjectwrites one and nothing in the repo reads it, soactiveProjectLink— and thereforeactiveProjectId— is nil on every cold start until the founder re-links. The spec claimed otherwise; that claim is corrected in this PR.🤖 Generated with Claude Code