Skip to content

feat(mt#5159): Validate recorded repository identity against origin; one backend-precedence - #3766

Merged
edobry merged 2 commits into
mainfrom
task/mt-5159
Sep 15, 2026
Merged

edobry merged 2 commits into
mainfrom
task/mt-5159

Conversation

@minsky-ai

@minsky-ai minsky-ai Bot commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Closes mt#5159. .minsky/config.yaml's repository.{backend,url,github} and project.slug are derived from git remote get-url origin by init once (mt#2414) and never re-read — so a value frozen at the wrong moment is permanent. flowtato's init ran before its remote existed, froze repository.backend: local, and nothing re-derived it (mt#5152, 2026-09-14). Two defects: nothing validates the recorded identity against the live origin, and two readers resolved the backend from two different keys (session start read repository.default_repo_backend; getRepositoryBackendFromConfig read repository.backend) with nothing reporting a disagreement.

This is ADR-003 extended, not replaced: the backend stays a project-level property init writes, and this adds (1) validation against origin with 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): resolveRepositoryIdentity composes the recorded config with origin via mt#5154's compareRepositoryIdentity and returns a drift result; resolveBackend states the precedence in ONE place — project repository.backend → user repository.default_repo_backend → the backend origin derives to (SC3); describeRepositoryDrift renders the one-line operator message. Pure over injected reads; never throws.
  • packages/domain/src/workspace/info.ts: getWorkspaceInfo validates the recorded identity against origin (injected reader, defaults to deriveRemoteUrl) and adds repositoryDrift when 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 at resolveBackend, so session start and workspace_info cannot read different keys in disagreement (SC2). Behavior unchanged (it already prefers repository.backend, falling back to the detection that reads default_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 compareRepositoryIdentity and 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_info result gains an optional repositoryDrift string; getWorkspaceInfoDeps gains an optional readOriginUrl. No renamed or newly-required field; repository.backend / repository.default_repo_backend keep 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:

  • SC1/SC1b (validate + report drift): 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_info reports repositoryDrift on a backend: local-vs-github mismatch (names the fix minsky init --overwrite), is absent on a match, and reports the no-origin case.
  • SC2 (single helper / cannot disagree): resolveRepositoryIdentity + resolveBackend are the one resolver; workspace_info routes through it and getRepositoryBackendFromConfig's docblock binds session start / PR creation to the same resolveBackend precedence.
  • SC3 (precedence documented + tested): resolveBackend tests assert project repository.backend wins over default_repo_backend, user-default is used only when the project set none, origin-derived is the fallback, and local when nothing is recorded.
  • AT3 (existing correct repos unchanged): the Minsky repo and any project whose config matches origin get repositoryDrift absent (a match renders null); 253 tests across the 19 directly-affected files pass, including the existing repository-backend-* and workspace/info suites.

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_info gains an additive field; no behavior change for a project whose identity matches origin. After merge I will run deployment_wait-for-latest for minsky-mcp (notBefore = merge time, expectCommitSha = merge SHA), require SUCCESS, correlate the Deploy MCP workflow run's head_sha, confirm /health (service: minsky-mcp, status: ok), and — since this repo's own identity matches origin — assert the hosted workspace_info does 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.

  • [BLOCKING] SC2 — single helper not adopted by session start. getRepositoryBackendFromConfig now resolves the backend through the shared resolveBackend (reading repository.backend AND repository.default_repo_backend), so session start / PR creation and workspace_info resolve 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.
  • [BLOCKING] SC1 — session start does not validate against origin. It now reads origin (argv git, best-effort, in the config branch only) and surfaces drift on RepositoryBackendInfo.repositoryDrift + a log.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. repoUrl also falls back to origin (repo.url || originUrl || "").
  • [non-blocking] shell-string exec. workspace_info's default origin reader now uses execFileSync("git", […argv], { timeout: 5000, stdio pipe }), not deriveRemoteUrl's shell string.
  • [non-blocking] GitHub-specific wording. 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.

…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

minsky-reviewer Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 477K prompt, 7K completion | Duration: 87s
Mode: normal

Commands

  • /review — request a fresh review

@minsky-ai minsky-ai Bot added the authorship/co-authored Co-authored by human and AI agent label Sep 15, 2026

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 against origin and report drift. Only workspace_info wires the new resolver: see packages/domain/src/workspace/info.ts:151-189, which imports resolveRepositoryIdentity and describeRepositoryDrift and adds repositoryDrift to the result. In contrast, packages/domain/src/session/repository-backend-detection.ts retains its prior logic and never calls the new helper — there is no import or invocation of repository-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 introduces packages/domain/src/project/repository-identity.ts as the single resolver, and workspace_info now uses it. However, getRepositoryBackendFromConfig still composes precedence locally and falls back to resolveRepositoryAndBackend without consulting resolveBackend/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 deriveRemoteUrl uses shell-string exec; consider argv-based execFile and timeouts
    getWorkspaceInfo now, by default, calls deriveRemoteUrl (packages/domain/src/project/slug.ts) which shells git remote get-url origin via execSync with a string command. While errors are caught and suppressed, this introduces a new spawned-process path for the workspace_info command. Elsewhere in the codebase (e.g., repository-backend-detection.ts) git execution was moved to argv-based execFileSync specifically to avoid shell-string risks and to shape stderr handling. Consider aligning deriveRemoteUrl with that pattern (argv, no shell) and/or ensuring a bounded timeout, or keep readOriginUrl injected 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 — describeRepositoryDrift messages are GitHub-specific; clarify wording for non-GitHub origins
    describeRepositoryDrift renders: "origin (...) is not a GitHub owner/name URL" and always recommends minsky init --overwrite to re-derive from origin. Since backendFromOriginUrl and compareRepositoryIdentity treat 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.repositoryDrift field; 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

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@edobry
edobry merged commit 4636c05 into main Sep 15, 2026
21 checks passed
@edobry
edobry deleted the task/mt-5159 branch September 15, 2026 02:00

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses execFileSync directly
    In WorkspaceInfoDeps.readOriginUrl's docblock (packages/domain/src/workspace/info.ts:55-63) the comment says production defaults to deriveRemoteUrl, but the actual fallback implementation below uses execFileSync("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

@edobry

edobry commented Sep 15, 2026

Copy link
Copy Markdown
Owner

Deploy verification — outcome (post-merge, 2026-09-15T02:0xZ): deployed, verified-1b. Posted as a comment (merge cleanup removed the session session_pr_edit needs — mt#5177).

  • Merge 4636c05be1a0ee9b3ece58cab51655f6b23d4b6f at 02:00:26Z.
  • deployment_wait-for-latest minsky-mcp (notBefore 02:00:26Z, expectCommitSha 4636c05…) → deployment 3de2a1e4 SUCCESS, 02:01:46Z, buildIdentity: indeterminate (image-source, expected).
  • Correlated: Deploy MCP run 34919495809, head_sha 4636c05b…, success, 02:00:28Z–02:01:51Z.
  • Runtime started: GET /health → {"service":"minsky-mcp","status":"ok"}.
  • Change-produced: the hosted workspace_info now returns repoBackend: github plus a repositoryDrift field — the drift check runs live (the field didn't exist pre-merge).
  • Benign observation: the hosted no-cwd call reports repositoryDrift: "no origin remote…" because the deployed container has no git checkout, so there's genuinely no origin to validate against there. Warning-level info, not an error; an explicit cwd (a session workspace) validates normally. Noted in the spec closeout.

Had Claude run the verification; same record in the mt#5159 spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

authorship/co-authored Co-authored by human and AI agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant