fix(delivery): read the setting a user wrote, and resolve a worktree's origin (#1258)

changelog.d/1258-delivery-repo.fixed.md

@@ -0,0 +1 @@
+The delivery board reads `delivery.repo` however the instance wrote it — nested (`delivery: { repo: … }`), as a literal dotted key, or flat (`deliveryRepo`) — instead of only the flat one, so the setting the page and the hint name really names the repository ([#1258](https://github.com/ThinkFlowLab/FlyBot/issues/1258)). A workspace that is a linked `git worktree` now resolves its `origin` through the worktree's `commondir` rather than falling back to the package default, and a repository name with a `.` or `..` segment is refused instead of being turned into `/repos/../../issues`.

core/src/host/delivery-board.js

@@ -11,7 +11,7 @@
 // `{ message, endpoint, status, kind, scope, at }` so the view can say what failed, where, and what
 // to do about it. "No GitHub connection yet" is reserved for a host that has never tried.
 import { repoSlug } from './update.js'
-import { NO_REPO_MESSAGE, resolveRepo } from './delivery-repo.js'
+import { NO_REPO_MESSAGE, instanceRepoSetting, resolveRepo } from './delivery-repo.js'
 
 export const DELIVERY_COLUMNS = ['raw', 'ready', 'working', 'review', 'done-today']
 
@@ -761,7 +761,7 @@ export function install(rt) {
     try { return rt.ctx?.workspaceRegistry?.get?.(rt.state?.workspaceId)?.path ?? null } catch { return null }
   }
   const setting = () => {
-    try { return rt.config?.deliveryRepo ?? null } catch { return null }
+    try { return instanceRepoSetting(rt.config) } catch { return null }
   }
   // Level 3 is the package's own repository, not the instance's choice. The shipped package carries
   // it; a host whose package.json has none (or a test) hands in '' and the board refuses rather than

core/src/host/delivery-repo.js

@@ -5,12 +5,14 @@
 // were. One repository is resolved here, for the host and for `scripts/delivery-probe.mjs` alike,
 // in three steps, first answer wins:
 //
-//   1. the instance setting, `delivery.repo` on the plugin entry (`config.deliveryRepo`);
+//   1. the instance setting, `delivery.repo` on the plugin entry — the host hands the entry through
+//      verbatim, so all three spellings people write are read (`instanceRepoSetting`);
 //   2. the workspace's own git remote — `<teamFolder>/.git/config`, `[remote "origin"] url`;
 //   3. the package's `repository.url`, which is a default and is labelled as one.
 //
-// Nothing is spawned: the host may run where `git` is not on PATH. A `.git` that is a file (a
-// worktree or a submodule) is followed through its `gitdir:` pointer instead. A name is normalised
+// Nothing is spawned: the host may run where `git` is not on PATH. A `.git` that is a file is
+// followed through its `gitdir:` pointer: a submodule keeps `config` there, and a linked worktree
+// keeps only a `commondir` pointer to the shared directory that does (#1258). A name is normalised
 // to `owner/name`; a GitHub URL or the scp form `git@github.com:owner/name.git` is accepted where a
 // name is, because that is what people paste.
 import { readFileSync, statSync } from 'node:fs'
@@ -19,7 +21,10 @@ import { join, resolve } from 'node:path'
 // The wording acceptance criterion 4 asks for, in one place so the view, the host and the CLI agree.
 export const NO_REPO_MESSAGE = 'no repository — set delivery.repo'
 
-const OWNER_NAME = /^[\w.-]+\/[\w.-]+$/
+// A segment is a plain repository name part: word characters, dots and dashes, but never a leading
+// dot. `.` and `..` are path steps, not names, and a board that accepted them would build
+// `/repos/../../issues` and normalise it to the API root (#1258).
+const OWNER_NAME = /^(?!\.)[\w.-]+\/(?!\.)[\w.-]+$/
 const SCP_GITHUB = /^(?:[\w.-]+@)?github\.com:([\w.-]+\/[\w.-]+?)(?:\.git)?\/?$/i
 
 const cleanName = text => {
@@ -61,18 +66,51 @@ export function originUrlIn(text) {
   return null
 }
 
-// Where git keeps the config for this folder: `.git/config`, or the `gitdir:` a `.git` file names.
-// Null when the folder is not a git checkout at all.
+// Where git keeps the config for this folder: `.git/config`, or whatever a `.git` file's `gitdir:`
+// pointer leads to. A submodule keeps `config` in that directory; a linked worktree keeps only a
+// `commondir` file naming the shared directory that has it (#1258). Null when the folder is not a
+// git checkout at all.
 export function gitConfigPath(folder) {
   if (typeof folder !== 'string' || folder === '') return null
   const dot = join(folder, '.git')
   try {
     if (statSync(dot).isDirectory()) return join(dot, 'config')
     const pointer = /^gitdir:\s*(.+)$/m.exec(readFileSync(dot, 'utf8'))
-    return pointer ? join(resolve(folder, pointer[1].trim()), 'config') : null
+    if (!pointer) return null
+    const gitdir = resolve(folder, pointer[1].trim())
+    const own = join(gitdir, 'config')
+    if (isFile(own)) return own
+    const common = commonDirOf(gitdir)
+    return common === null ? null : join(common, 'config')
   } catch { return null }
 }
 
+const isFile = path => {
+  try { return statSync(path).isFile() } catch { return false }
+}
+
+// A linked worktree's gitdir holds no config: `commondir` names the common directory, relative to
+// the gitdir, where the shared `config` lives.
+const commonDirOf = gitdir => {
+  try {
+    const pointer = readFileSync(join(gitdir, 'commondir'), 'utf8').trim()
+    return pointer === '' ? null : resolve(gitdir, pointer)
+  } catch { return null }
+}
+
+// The instance setting as the profile actually wrote it. The host passes the plugin entry through
+// verbatim and expands no dotted key, so one setting reaches us as a flat `deliveryRepo`, a literal
+// `'delivery.repo'`, or a nested `delivery.repo` — the hint, the page and the docs all say the last
+// spelling, and reading only the first would ignore what a user just wrote (#1258). First defined
+// answer wins; validation stays in `parseRepoName`.
+export function instanceRepoSetting(config) {
+  if (config === null || typeof config !== 'object') return null
+  for (const value of [config.delivery?.repo, config['delivery.repo'], config.deliveryRepo]) {
+    if (value !== undefined && value !== null && value !== '') return value
+  }
+  return null
+}
+
 // The `owner/name` the folder's `origin` names, read from disk without running git.
 export function gitOriginRepo(folder) {
   const path = gitConfigPath(folder)

core/test/delivery-board.test.js

@@ -852,6 +852,20 @@ test('the instance setting names the repository for the board and the probe, and
   assert.ok(probe.rows.every(row => row.ok))
 })
 
+test('a dotted or nested delivery.repo names the repository too, as a user writes it (#1258)', async () => {
+  // The hint says `delivery.repo`, so that is what a profile carries — as a literal dotted key or
+  // nested; a literal key wins nowhere, it is simply read. The flat `deliveryRepo` is the third.
+  for (const config of [{ 'delivery.repo': 'acme/widget' }, { delivery: { repo: 'acme/widget' } }]) {
+    const github = fakeGitHub({ slug: 'acme/widget' })
+    const installed = hostWith(github, { config })
+    const settled = await installed.deliveryBoard({ wait: true })
+    assert.equal(settled.repo, 'acme/widget', JSON.stringify(config))
+    assert.equal(settled.repoSource, 'config', JSON.stringify(config))
+    assert.equal(settled.state, 'fresh')
+    for (const call of github.calls) assert.match(call.url, /^https:\/\/api\.github\.com\/repos\/acme\/widget(\/|$|\?)/, call.url)
+  }
+})
+
 test('the workspace git remote names the repository when there is no setting (#1250)', async () => {
   const dir = mkdtempSync(join(tmpdir(), 'delivery-install-'))
   try {

core/test/delivery-repo.test.js

@@ -7,7 +7,7 @@ import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'nod
 import { tmpdir } from 'node:os'
 import { join } from 'node:path'
 import { test } from 'node:test'
-import { gitConfigPath, gitOriginRepo, NO_REPO_MESSAGE, originUrlIn, parseRepoName, resolveRepo } from '../src/host/delivery-repo.js'
+import { gitConfigPath, gitOriginRepo, instanceRepoSetting, NO_REPO_MESSAGE, originUrlIn, parseRepoName, resolveRepo } from '../src/host/delivery-repo.js'
 
 const scratch = () => mkdtempSync(join(tmpdir(), 'delivery-repo-'))
 const cleanup = dir => rmSync(dir, { recursive: true, force: true })
@@ -56,18 +56,43 @@ test('an origin written as a URL, an scp path or a name with no .git reads the s
   }
 })
 
-test('a .git file is followed through its gitdir, and git is never spawned (#1250)', () => {
-  // A worktree (or a submodule) keeps its config elsewhere and `.git` is a pointer file.
+test('a submodule keeps its config in the gitdir the `.git` file names (#1250)', () => {
+  // A submodule's `.git` is a pointer file and its own gitdir really does hold `config`.
   const root = scratch()
   try {
-    const store = join(root, 'main.git', 'worktrees', 'wt')
+    const store = join(root, 'super', '.git', 'modules', 'sub')
     mkdirSync(store, { recursive: true })
-    writeFileSync(join(store, 'config'), '[remote "origin"]\n\turl = git@github.com:acme/worktree.git\n')
+    writeFileSync(join(store, 'config'), '[remote "origin"]\n\turl = git@github.com:acme/submodule.git\n')
+    const dir = join(root, 'super', 'sub')
+    mkdirSync(dir, { recursive: true })
+    writeFileSync(join(dir, '.git'), `gitdir: ${join('..', '.git', 'modules', 'sub')}\n`)
+    assert.equal(gitConfigPath(dir), join(store, 'config'))
+    assert.deepEqual(resolveRepo({ workspace: dir }), { repo: 'acme/submodule', source: 'remote' })
+  } finally { cleanup(root) }
+})
+
+test('a linked worktree follows its `commondir` to the config the shared directory holds (#1258)', () => {
+  // `git worktree add` writes `.git` -> `<main>/.git/worktrees/<name>`, and that directory holds no
+  // config of its own: only `commondir`, naming the shared directory where git keeps it. Reading
+  // `config` beside the pointer finds nothing, so a worktree workspace fell to the package default.
+  const root = scratch()
+  try {
+    const shared = join(root, 'main', '.git')
+    mkdirSync(join(shared, 'worktrees', 'wt'), { recursive: true })
+    writeFileSync(join(shared, 'config'), '[remote "origin"]\n\turl = git@github.com:acme/worktree.git\n')
+    writeFileSync(join(shared, 'worktrees', 'wt', 'commondir'), '../..\n')
     const dir = join(root, 'checkout')
     mkdirSync(dir)
-    writeFileSync(join(dir, '.git'), `gitdir: ${join('..', 'main.git', 'worktrees', 'wt')}\n`)
-    assert.equal(gitConfigPath(dir), join(store, 'config'))
+    writeFileSync(join(dir, '.git'), `gitdir: ${join('..', 'main', '.git', 'worktrees', 'wt')}\n`)
+    assert.equal(gitConfigPath(dir), join(shared, 'config'))
     assert.deepEqual(resolveRepo({ workspace: dir }), { repo: 'acme/worktree', source: 'remote' })
+    // A pointer with neither a config nor a commondir answers nothing rather than a wrong path.
+    const bare = join(root, 'bare')
+    mkdirSync(join(bare, 'gitdir'), { recursive: true })
+    mkdirSync(join(root, 'elsewhere'))
+    writeFileSync(join(root, 'elsewhere', '.git'), 'gitdir: ../bare/gitdir\n')
+    assert.equal(gitConfigPath(join(root, 'elsewhere')), null)
+    assert.deepEqual(resolveRepo({ workspace: join(root, 'elsewhere') }), { repo: null, source: null })
   } finally { cleanup(root) }
 })
 
@@ -108,6 +133,39 @@ test('a value that is not a GitHub repository is refused, not mined for its last
   assert.equal(originUrlIn('[remote "origin"]\n\tfetch = +refs/heads/*:refs/remotes/origin/*\n'), null)
 })
 
+test('a path step is not a repository name, so `.` and `..` segments are refused (#1258)', () => {
+  // `OWNER_NAME` allowed a dot anywhere, so `../..` answered a "repository": the board then built
+  // `/repos/../../issues`, which normalises to the API root. Same host, so no token left, but every
+  // request it made was meaningless and the "this is a repository name" invariant was gone.
+  for (const value of ['../..', './widget', '.a/b', 'acme/.b', 'acme/..']) {
+    assert.equal(parseRepoName(value), null, value)
+  }
+  // A dot inside a segment is a real repository name and stays allowed.
+  for (const value of ['acme/widget.js', 'a.b/c-d', 'a1-2_3/b.4']) {
+    assert.equal(parseRepoName(value), value, value)
+  }
+})
+
+test('the instance setting is read in all three spellings a profile can write it (#1258)', () => {
+  // The host passes the plugin entry through verbatim and expands no dotted key, so the hint's
+  // `delivery.repo` reaches the board as one of three shapes. Reading only the flat one ignored
+  // what a user had just written and fell through to the labelled package default.
+  assert.equal(instanceRepoSetting({ delivery: { repo: 'acme/widget' } }), 'acme/widget')
+  assert.equal(instanceRepoSetting({ 'delivery.repo': 'acme/widget' }), 'acme/widget')
+  assert.equal(instanceRepoSetting({ deliveryRepo: 'acme/widget' }), 'acme/widget')
+  // The documented spelling wins over the runtime shape when a profile happens to carry both.
+  assert.equal(instanceRepoSetting({ delivery: { repo: 'acme/nested' }, deliveryRepo: 'acme/flat' }), 'acme/nested')
+  // Nothing written, or an empty value, is "no setting", not the empty string as a repository.
+  assert.equal(instanceRepoSetting({}), null)
+  assert.equal(instanceRepoSetting({ delivery: {} }), null)
+  assert.equal(instanceRepoSetting({ deliveryRepo: '' }), null)
+  assert.equal(instanceRepoSetting(null), null)
+  assert.equal(instanceRepoSetting(undefined), null)
+  // A nested setting is a real setting end to end, not only in the helper.
+  assert.deepEqual(resolveRepo({ setting: instanceRepoSetting({ delivery: { repo: 'acme/widget' } }), packageSlug: 'x/y' }),
+    { repo: 'acme/widget', source: 'config' })
+})
+
 test('the probe is a shell over this resolver, and refuses before it asks (#1250)', () => {
   const source = readFileSync(new URL('../scripts/delivery-probe.mjs', import.meta.url), 'utf8')
   // One resolver, imported — a second copy in the CLI is how the two drift.