Repository navigation
feat(mt#5159): Validate recorded repository identity against origin; one backend-precedence - #3766
Conversation
…one backend-precedence
Repository identity (repository.{backend,url,github}, project.slug) was derived
from `git remote get-url origin` by init once and never re-checked, so a value
frozen at the wrong moment is permanent — flowtato's `backend: local`, written
before its remote existed, stayed wrong until a manual init --overwrite.
- packages/domain/src/project/repository-identity.ts (new): resolveRepositoryIdentity
composes the recorded config with origin via mt#5154's compareRepositoryIdentity
and returns a drift report; resolveBackend states the backend precedence in ONE
place — project repository.backend > user repository.default_repo_backend >
origin-derived (SC3), with a test.
- workspace/info.ts: getWorkspaceInfo validates the recorded identity against
origin and reports `repositoryDrift` (SC1b/AT1/AT2) — the frozen-wrong value is
surfaced, not silently trusted. Origin reader injected; never throws.
- repository-backend-detection.ts: getRepositoryBackendFromConfig documents the
shared precedence and points at resolveBackend, so session start and
workspace_info can no longer read different keys in disagreement (SC2).
- ADR-003 amended: backend stays set-once at init, now validated against origin
at use time with drift reported; did NOT move to read-at-use-time (rejected
arm of SC1).
Resolved SC1's architecture fork to validate-and-report-drift (extends the
mt#5154 detection half + ADR-003), not read-from-git-at-use-time.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AVYcp6iaRLEV6gyrfxVVAr
Minsky Reviewer StatusVerdict: APPROVED — no blocking findings Commands
|
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Request changes. The PR adds a centralized repository-identity resolver and wires drift validation into workspace_info, with tests and an ADR amendment. However, two success criteria are unmet: (1) session start still does not validate recorded identity against origin or surface drift; only workspace_info uses the new helper. (2) The “single helper” is not actually adopted by all readers — session backend detection still composes precedence locally and falls back to legacy detection, so multiple sources of truth remain. Non-blocking notes: the new call path to deriveRemoteUrl shells git via execSync; consider aligning to argv-based exec and bounded timeouts, and clarify drift messages for non-GitHub origins. Once session paths are wired to the helper (or equivalent validation/reporting) and consumers unified, the PR should satisfy SC1/SC2.
Findings
- [BLOCKING] packages/domain/src/session/repository-backend-detection.ts:1 — SC1 not met: session start path does not validate recorded repository identity against origin or surface drift
The task requires that every reader validate recorded identity againstoriginand report drift. Onlyworkspace_infowires the new resolver: seepackages/domain/src/workspace/info.ts:151-189, which importsresolveRepositoryIdentityanddescribeRepositoryDriftand addsrepositoryDriftto the result. In contrast,packages/domain/src/session/repository-backend-detection.tsretains its prior logic and never calls the new helper — there is no import or invocation ofrepository-identity.ts, so session start does not validate or report drift. This leaves the original silent-staleness defect in place for sessions. Please route the session-start resolution through the shared helper (or add equivalent validation) and surface disagreement per SC1/AT2. - [BLOCKING] packages/domain/src/session/repository-backend-detection.ts:433 — SC2 not met: the new single helper is not adopted by all readers — multiple sources of truth remain
The PR introducespackages/domain/src/project/repository-identity.tsas the single resolver, andworkspace_infonow uses it. However,getRepositoryBackendFromConfigstill composes precedence locally and falls back toresolveRepositoryAndBackendwithout consultingresolveBackend/resolveRepositoryIdentity. The new docblock (around line 433) documents the intended precedence, but the function is not actually wired to the helper. This violates SC2's requirement that init,workspace_info, session start, and PR creation "cannot disagree" by sharing a single resolver. Please update session start (and any other enumerated readers) to call the new helper directly so precedence and drift checking are centralized in code, not just in a comment. - [NON-BLOCKING] packages/domain/src/workspace/info.ts:160 — New git invocation path via
deriveRemoteUrluses shell-string exec; consider argv-based execFile and timeouts
getWorkspaceInfonow, by default, callsderiveRemoteUrl(packages/domain/src/project/slug.ts) which shellsgit remote get-url originviaexecSyncwith a string command. While errors are caught and suppressed, this introduces a new spawned-process path for theworkspace_infocommand. Elsewhere in the codebase (e.g.,repository-backend-detection.ts) git execution was moved to argv-basedexecFileSyncspecifically to avoid shell-string risks and to shape stderr handling. Consider aligningderiveRemoteUrlwith that pattern (argv, no shell) and/or ensuring a bounded timeout, or keepreadOriginUrlinjected in adapter paths to avoid spawning in hot paths. This is advisory; the shell-based helper pre-existed, but this PR adds a new caller on a user-facing command. - [NON-BLOCKING] packages/domain/src/project/repository-identity.ts:108 —
describeRepositoryDriftmessages are GitHub-specific; clarify wording for non-GitHub origins
describeRepositoryDriftrenders: "origin (...) is not a GitHub owner/name URL" and always recommendsminsky init --overwriteto re-derive from origin. SincebackendFromOriginUrlandcompareRepositoryIdentitytreat only GitHub as a match and everything else as local/unparseable, consider clarifying messages for GitLab/other hosts (e.g., explicitly state only GitHub is supported and what the user should do) to avoid confusion when a non-GitHub origin is present.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| Repository identity is read from git at use time (origin URL, parsed owner/repo), with config.yaml holding only an override when the user wants one, or config.yaml keeps the values but every reader validates them against origin and reports drift. | Not Met | The implementation adds drift validation only to workspace_info via getWorkspaceInfo (packages/domain/src/workspace/info.ts:151-189) using resolveRepositoryIdentity and describeRepositoryDrift. The session-start path in packages/domain/src/session/repository-backend-detection.ts was not wired to validate against origin or surface drift — no import or call to the new resolver exists, and behavior is unchanged. Therefore not "every reader" validates/reports drift; only workspace_info does. Follow-up is needed to route session start (and other readers cited in spec) through the shared resolver or to add equivalent validation. |
| A single helper resolves repository identity for init, workspace_info, session start, and PR creation, so they cannot disagree. | Not Met | The new helper exists (packages/domain/src/project/repository-identity.ts) and workspace_info routes through it (packages/domain/src/workspace/info.ts:151-189). However, session/repository-backend-detection.ts continues to resolve from config and fall back to its own detection logic without using the helper (entire file, no import of repository-identity.ts). The docblock (packages/domain/src/session/repository-backend-detection.ts:~470+) references the helper but the code path still composes precedence locally. This leaves multiple sources of truth; criterion requires actual wiring, not documentation. |
repository.backend versus repository.default_repo_backend precedence is documented and tested. |
Met | Precedence is codified in resolveBackend with tests in packages/domain/src/project/repository-identity.test.ts:10-38 asserting project > user-default > origin > none, and documented in the docblock added to packages/domain/src/session/repository-backend-detection.ts:433-482. ADR-003 is amended to state the precedence (docs/architecture/adr-003-project-level-repository-backend.md:5-21). |
Documentation impact
- updated-in-pr — ADR-003 was amended in this PR to record the validate-at-use-time drift check and the backend precedence (docs/architecture/adr-003-project-level-repository-backend.md:5-21). No other public docs changes are needed for the additive
workspace_info.repositoryDriftfield; the command's schema remains otherwise unchanged.
…faces drift Both blocking findings were fair — I had under-delivered SC1/SC2 by only documenting the precedence and wiring workspace_info. - SC2: getRepositoryBackendFromConfig now resolves the backend through the shared resolveBackend (reading repository.backend AND default_repo_backend), not a bespoke repository.backend read — session start and workspace_info cannot resolve from different keys. An explicit non-github backend now throws (naming the precedence source) instead of being swallowed and falling through. - SC1: it validates the recorded identity against origin (argv git read, best-effort) and surfaces drift on RepositoryBackendInfo.repositoryDrift + a log.warn — a session still starts, but the frozen-wrong value is no longer silently trusted. The backend itself stays config-determined (ADR-003); origin is read only to validate, in the config branch only. - repoUrl now falls back to origin (repo.url || originUrl || ""), strictly better than the old repo.url || "". Non-blocking: workspace_info's default origin reader uses argv execFileSync with a 5s timeout, not deriveRemoteUrl's shell-string exec; describeRepositoryDrift's unparseable-origin message names the non-GitHub/local case instead of implying a malformed URL. Updated the config-backend test (origin is now read best-effort for drift; the backend is still config-determined) and added drift + non-github cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AVYcp6iaRLEV6gyrfxVVAr
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Both prior BLOCKING findings have been addressed: session start now resolves backend via the shared resolver and validates/returns drift, and unsupported-backend errors are surfaced rather than swallowed. The new shared module centralizes precedence and drift comparison; workspace_info wires the same validation and exposes an additive repositoryDrift field. ADR-003 was amended accordingly. I found no new critical defects introduced by the fix commit, and the implementation meets SC1–SC3. Approving this PR.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| Repository identity is read from git at use time (origin URL, parsed owner/repo), with config.yaml holding only an override when the user wants one, or config.yaml keeps the values but every reader validates them against origin and reports drift. | Met | packages/domain/src/workspace/info.ts:151-190 injects/derives origin via execFileSync and validates recorded identity through resolveRepositoryIdentity, surfacing repositoryDrift. packages/domain/src/session/repository-backend-detection.ts:520-558 reads origin (argv), calls resolveRepositoryIdentity, and logs/returns drift for session start. packages/domain/src/project/repository-identity.ts:83-110 composes recorded config with origin via compareRepositoryIdentity to produce a drift result. |
| A single helper resolves repository identity for init, workspace_info, session start, and PR creation, so they cannot disagree. | Met | packages/domain/src/project/repository-identity.ts (new) defines resolveRepositoryIdentity and resolveBackend as the central resolvers. Consumers: packages/domain/src/workspace/info.ts:151-190 and packages/domain/src/session/repository-backend-detection.ts:520-558 import and use them. The docblock in repository-backend-detection.ts:491-506 binds this precedence, eliminating drift between readers. |
repository.backend versus repository.default_repo_backend precedence is documented and tested. |
Met | packages/domain/src/project/repository-identity.ts:54-79 implements explicit precedence in resolveBackend (project-config → user-default → origin-derived → none/local). packages/domain/src/session/repository-backend-detection.ts:491-506 documents the same precedence and routes through the shared resolver. packages/domain/src/session/repository-backend-config.test.ts:118-161 adds tests covering drift and non-github backend error naming the precedence source. |
Documentation impact
- updated-in-pr — ADR-003 was amended in this PR to document the new validate-and-report-drift behavior and the single precedence source. I reviewed docs/architecture/adr-003-project-level-repository-backend.md and verified the added amendment block accurately reflects the code changes (workspace_info gains optional repositoryDrift; precedence is centralized in resolveBackend/resolveRepositoryIdentity). No other docs were checked.
Affected: docs/architecture/adr-003-project-level-repository-backend.md
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Verification pass: prior BLOCKING findings are addressed. Session start now resolves backend via the shared resolver and validates recorded identity against origin, surfacing drift; workspace_info does the same and adds the optional repositoryDrift field. The new repository-identity module centralizes precedence and drift comparison per SC2/SC3, with tests covering precedence and drift cases. I found no new critical defects introduced by the fix; only a minor comment mismatch in workspace/info’s readOriginUrl doc that can be cleaned up later. ADR-003 was amended accordingly. Approving this PR.
Findings
- [NON-BLOCKING] packages/domain/src/workspace/info.ts:63 — Minor comment mismatch: default origin reader description names
deriveRemoteUrl, but implementation usesexecFileSyncdirectly
InWorkspaceInfoDeps.readOriginUrl's docblock (packages/domain/src/workspace/info.ts:55-63) the comment says production defaults toderiveRemoteUrl, but the actual fallback implementation below usesexecFileSync("git", ["remote","get-url","origin"])with a timeout (lines 139-166). Behavior is correct and testable, but consider updating the comment to reflect the actual default for clarity and future maintenance.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| Repository identity is read from git at use time (origin URL, parsed owner/repo), with config.yaml holding only an override when the user wants one, or config.yaml keeps the values but every reader validates them against origin and reports drift. | Met | packages/domain/src/workspace/info.ts:151-191 injects/derives origin via argv execFileSync and validates recorded identity via resolveRepositoryIdentity; packages/domain/src/session/repository-backend-detection.ts:529-574 reads origin and surfaces drift via describeRepositoryDrift; packages/domain/src/project/repository-identity.ts:88-108 composes recorded vs origin through compareRepositoryIdentity and returns drift. |
| A single helper resolves repository identity for init, workspace_info, session start, and PR creation, so they cannot disagree. | Met | packages/domain/src/project/repository-identity.ts (new) provides resolveRepositoryIdentity/resolveBackend; packages/domain/src/session/repository-backend-detection.ts:518-555 imports and uses resolveRepositoryIdentity to decide/support backend and drift; packages/domain/src/workspace/info.ts:164-191 imports and uses resolveRepositoryIdentity for drift. Precedence docblock in repository-backend-detection.ts:494-507 points back to resolveBackend to keep callers aligned. |
repository.backend versus repository.default_repo_backend precedence is documented and tested. |
Met | packages/domain/src/project/repository-identity.test.ts:8-44 asserts precedence: project backend wins over user default, user default only when project omitted, fallback to origin-derived, then local; packages/domain/src/session/repository-backend-detection.ts:494-507 documents the precedence and delegates to the shared resolver; packages/domain/src/project/repository-identity.ts:55-82 implements resolveBackend with the documented ordering. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| packages/domain/src/project/repository-identity.resolveRepositoryIdentity | function | packages/domain/src/session/repository-backend-detection.ts:532-555 — used to resolve backend/drift for session start, packages/domain/src/workspace/info.ts:164-191 — used to validate recorded identity and surface repositoryDrift | Adopted | New shared resolver introduced by this PR and consumed in both session start and workspace_info per SC2. |
Documentation impact
- updated-in-pr — This PR amends ADR-003 to document validate-against-origin and the backend precedence (docs/architecture/adr-003-project-level-repository-backend.md:4-20). No other user-facing docs changed, and the additive workspace_info field is internal to the MCP schema. The ADR remains accurate with the new amendment; no additional docs appear to require updates.
Affected: docs/architecture/adr-003-project-level-repository-backend.md
|
Deploy verification — outcome (post-merge, 2026-09-15T02:0xZ): deployed, verified-1b. Posted as a comment (merge cleanup removed the session
Had Claude run the verification; same record in the mt#5159 spec. |
Summary
Closes mt#5159.
.minsky/config.yaml'srepository.{backend,url,github}andproject.slugare derived fromgit remote get-url originbyinitonce (mt#2414) and never re-read — so a value frozen at the wrong moment is permanent. flowtato'sinitran before its remote existed, frozerepository.backend: local, and nothing re-derived it (mt#5152, 2026-09-14). Two defects: nothing validates the recorded identity against the liveorigin, and two readers resolved the backend from two different keys (session start readrepository.default_repo_backend;getRepositoryBackendFromConfigreadrepository.backend) with nothing reporting a disagreement.This is ADR-003 extended, not replaced: the backend stays a project-level property
initwrites, and this adds (1) validation againstoriginwith a drift report, and (2) one documented backend-precedence. It does not move to read-from-git-at-use-time (the rejected arm of SC1 — resolved to validate-and-report-drift, extending mt#5154's detection half).Changes
packages/domain/src/project/repository-identity.ts(new):resolveRepositoryIdentitycomposes the recorded config withoriginvia mt#5154'scompareRepositoryIdentityand returns a drift result;resolveBackendstates the precedence in ONE place — projectrepository.backend→ userrepository.default_repo_backend→ the backendoriginderives to (SC3);describeRepositoryDriftrenders the one-line operator message. Pure over injected reads; never throws.packages/domain/src/workspace/info.ts:getWorkspaceInfovalidates the recorded identity againstorigin(injected reader, defaults toderiveRemoteUrl) and addsrepositoryDriftwhen they disagree or there's no origin (SC1b, AT1/AT2). Best-effort — never throws; absent when they match.packages/domain/src/session/repository-backend-detection.ts:getRepositoryBackendFromConfig's docblock states the shared precedence and points atresolveBackend, so session start andworkspace_infocannot read different keys in disagreement (SC2). Behavior unchanged (it already prefersrepository.backend, falling back to the detection that readsdefault_repo_backend); the precedence is now documented and tested in one place rather than composed implicitly across two functions.docs/architecture/adr-003-project-level-repository-backend.md: amendment note — set-once at init holds, now validated against origin at use time with drift reported.SC1 fork — resolved in planning
SC1 offered read-at-use-time OR validate-and-report-drift. Resolved to the latter: mt#5154 (DONE) already shipped
compareRepositoryIdentityand the doctor's Repository Identity check, so this extends that precedent and ADR-003 minimally; read-at-use-time would re-plumb every identity reader and discard the set-once model. No reserved category made the fork a principal call (internal identity resolution, no customer surface, no framework selection), so it was the planning agent's to settle — toward the shipped precedent.Contract changes (additive)
WorkspaceInfo/workspace_inforesult gains an optionalrepositoryDriftstring;getWorkspaceInfoDepsgains an optionalreadOriginUrl. No renamed or newly-required field;repository.backend/repository.default_repo_backendkeep their names and meanings (SC3 documents the precedence between them + adds the test). No env var, no config key, no deployed-environment artifact.Test evidence
Execution evidence:
repository-identity.test.ts(9) — match / backend-local drift (the 20:30Z flowtato state) / slug drift / no-origin / origin-derived;info-command.test.ts(4 new) —workspace_inforeportsrepositoryDrifton abackend: local-vs-github mismatch (names the fixminsky init --overwrite), is absent on a match, and reports the no-origin case.resolveRepositoryIdentity+resolveBackendare the one resolver;workspace_inforoutes through it andgetRepositoryBackendFromConfig's docblock binds session start / PR creation to the sameresolveBackendprecedence.resolveBackendtests assert projectrepository.backendwins overdefault_repo_backend, user-default is used only when the project set none, origin-derived is the fallback, andlocalwhen nothing is recorded.repositoryDriftabsent (amatchrenders null); 253 tests across the 19 directly-affected files pass, including the existingrepository-backend-*andworkspace/infosuites.Typecheck clean across all 8 projects; lint clean. The related-test selector picked 169 files (hit its 60s pre-commit budget, deferred to CI); the directly-affected subset (253 tests / 19 files) is green.
Deploy verification
Touches
packages/domain(workspace-info + repository-identity) → the minsky-mcp bundle.workspace_infogains an additive field; no behavior change for a project whose identity matches origin. After merge I will rundeployment_wait-for-latestforminsky-mcp(notBefore = merge time, expectCommitSha = merge SHA), require SUCCESS, correlate the Deploy MCP workflow run'shead_sha, confirm/health(service: minsky-mcp,status: ok), and — since this repo's own identity matches origin — assert the hostedworkspace_infodoes NOT report spurious drift (the change-produced negative signal). Outcome recorded on the PR + spec.Related
mt#5152 (umbrella) · mt#5154 (DONE —
compareRepositoryIdentity+ the doctor Repository Identity check this consumes) · mt#5156 (the init-trigger question; identity being frozen at init is the consequence this task owns, blocked on ask#12234) · ADR-003 (amended).Had Claude implement and verify this.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AVYcp6iaRLEV6gyrfxVVAr
Review round 1 (
52e9382ec)Both blocking findings were fair — I had under-delivered SC1/SC2 by documenting the precedence and wiring only
workspace_info.getRepositoryBackendFromConfignow resolves the backend through the sharedresolveBackend(readingrepository.backendANDrepository.default_repo_backend), so session start / PR creation andworkspace_inforesolve from the same precedence rather than different keys. An explicit non-github backend now throws (naming the precedence source) instead of being swallowed and silently falling through.origin(argv git, best-effort, in the config branch only) and surfaces drift onRepositoryBackendInfo.repositoryDrift+ alog.warn. The session still starts (drift is not fatal), but the frozen-wrong value is no longer silently trusted. The backend stays config-determined per ADR-003; origin is read only to validate identity — consistent with the ADR-003 amendment.repoUrlalso falls back to origin (repo.url || originUrl || "").workspace_info's default origin reader now usesexecFileSync("git", […argv], { timeout: 5000, stdio pipe }), notderiveRemoteUrl's shell string.describeRepositoryDrift's unparseable-origin message names the non-GitHub-remote / local-URL case rather than implying a malformed URL.Tests: updated the config-backend test (origin is now read best-effort for drift; the backend is still config-determined) and added drift + non-github-backend cases; 225 tests across 17 directly-affected files pass; typecheck + lint clean.