feat(plugin): a directory says which project it belongs to, git repo or not (#4085)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 10s
CI & Build / integration (push) Successful in 47s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m1s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 10s
CI & Build / integration (push) Successful in 47s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / Python tests (push) Failing after 1m1s
CI & Build / Build & push image (push) Skipped
All six hooks scoped their requests one way: `git remote get-url origin`,
resolved server-side through the repo bindings. That key does not exist
outside a git repo, so a session in a plain directory was unscoped in every
hook at once — no project context, no prior-art scoping, no project rules —
and silently, because a missing remote is indistinguishable from a remote
nobody bound.
A `.scribe` file is the second key, read by the shared scribe_scope_query so a
directory scopes the same way everywhere:
{"instance": "https://scribe.example.com", "project_id": 2, "project": "…"}
`instance` is why the file is not just a number: an id is a different project
on every Scribe, so a marker that travels — a copied directory, a shared
machine, a repo someone else clones — would otherwise scope the session to the
wrong project without a word. Compared host-only, and a mismatch drops the id:
no project beats the wrong project. A bare integer is accepted too, since it
is what a person writes by hand. The marker beats a git remote — someone put
the file there on purpose — which is also how a directory overrides its
binding.
Two things it found on the way:
* An explicit project_id that did not resolve rendered NO message at all —
the branch hung off `if project_id` as an `elif`, so a caller holding a
pointer it believed in got a context that silently omitted the project it
had asked for. Now reported.
* The refusal reason was a global set inside a function every caller reads
through `$( )`. The assignment died with the subshell, leaving the caller
to read an unset variable under `set -u` — which aborts the hook and costs
the whole session's SessionStart context, to fetch a warning about a file.
It comes back through stdout with the id instead, and a test pins it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -0,0 +1,150 @@
|
||||
"""`.scribe` — the second key a directory can be scoped by (#4085).
|
||||
|
||||
Every hook in `plugin/hooks/` scopes its request to a project, and until this
|
||||
existed all six turned on one key: `git remote get-url origin`. That key does
|
||||
not exist outside a git repo, so a session in a plain directory was unscoped
|
||||
everywhere at once — silently, because a missing remote is indistinguishable
|
||||
from a remote nobody bound.
|
||||
|
||||
`scribe_scope_query` is the shared answer, so these run the real shell rather
|
||||
than a reimplementation of it. What they pin:
|
||||
|
||||
* the marker is found from a subdirectory, the way git finds its root;
|
||||
* a bare integer works, because that is what a person writes by hand;
|
||||
* the marker BEATS a git remote, since someone put the file there on purpose;
|
||||
* an `instance` naming a different Scribe is refused — the case the field
|
||||
exists for. A project id means nothing on its own: id 2 is a different
|
||||
project on every instance, so a marker that travels would otherwise scope
|
||||
the session to the wrong project, confidently and without a word.
|
||||
|
||||
That last one is the reason the file is JSON and not a number, so it is tested
|
||||
from both directions: the matching host is honoured, the mismatched one is not.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import os
|
||||
import shutil
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
ROOT = Path(__file__).resolve().parents[1]
|
||||
DEFS = ROOT / "plugin" / "hooks" / "scribe_defs.sh"
|
||||
INSTANCE = "https://scribe.example.com"
|
||||
|
||||
|
||||
def _sh(script: str, cwd: Path, env_extra: dict | None = None) -> str:
|
||||
for tool in ("bash", "jq"):
|
||||
if shutil.which(tool) is None:
|
||||
pytest.skip(f"hook runtime tool {tool!r} not installed")
|
||||
env = {"PATH": os.environ["PATH"], "HOME": str(cwd)}
|
||||
env.update(env_extra or {})
|
||||
body = f'set -uo pipefail\n. "{DEFS}"\nurl="{INSTANCE}"\n{script}\n'
|
||||
out = subprocess.run(["bash", "-c", body], cwd=cwd, capture_output=True,
|
||||
text=True, env=env, timeout=30)
|
||||
assert out.returncode == 0, out.stderr
|
||||
return out.stdout.strip()
|
||||
|
||||
|
||||
def _marker(d: Path, **fields) -> None:
|
||||
(d / ".scribe").write_text(json.dumps(fields))
|
||||
|
||||
|
||||
def _git_repo(d: Path, remote: str) -> None:
|
||||
env = {"PATH": os.environ["PATH"], "HOME": str(d)}
|
||||
subprocess.run(["git", "init", "-q"], cwd=d, check=True, env=env)
|
||||
subprocess.run(["git", "remote", "add", "origin", remote], cwd=d, check=True, env=env)
|
||||
|
||||
|
||||
def test_a_bare_integer_is_a_valid_marker(tmp_path):
|
||||
"""What someone writes by hand. It parses as JSON already, so one filter
|
||||
reads it and the written form alike."""
|
||||
(tmp_path / ".scribe").write_text("7\n")
|
||||
assert _sh(f'scribe_scope_query "{tmp_path}"', tmp_path) == "project_id=7"
|
||||
|
||||
|
||||
def test_the_marker_is_found_from_a_subdirectory(tmp_path):
|
||||
_marker(tmp_path, instance=INSTANCE, project_id=2, project="FabledScribe")
|
||||
deep = tmp_path / "src" / "scribe" / "services"
|
||||
deep.mkdir(parents=True)
|
||||
assert _sh(f'scribe_scope_query "{deep}"', tmp_path) == "project_id=2"
|
||||
|
||||
|
||||
def test_a_matching_instance_is_honoured(tmp_path):
|
||||
"""Host-only comparison, so http/https and a trailing slash are not a
|
||||
mismatch — only a genuinely different Scribe is."""
|
||||
_marker(tmp_path, instance="http://scribe.example.com/", project_id=2)
|
||||
assert _sh(f'scribe_scope_query "{tmp_path}"', tmp_path) == "project_id=2"
|
||||
|
||||
|
||||
def test_a_marker_for_another_instance_is_refused_with_a_reason(tmp_path):
|
||||
"""The case the `instance` field exists for. No project beats the wrong
|
||||
project, and the reason is carried so the session can say which it was."""
|
||||
_marker(tmp_path, instance="https://someone-elses.example.org", project_id=2)
|
||||
assert _sh(f'scribe_scope_query "{tmp_path}"', tmp_path) == ""
|
||||
said = _sh(f'scribe_marker_read "{tmp_path}/.scribe"', tmp_path)
|
||||
assert "someone-elses.example.org" in said and "scribe.example.com" in said
|
||||
|
||||
|
||||
def test_the_marker_beats_a_git_remote(tmp_path):
|
||||
"""Someone put the file there deliberately; a remote is only where the
|
||||
code happens to be pushed. This is also how a directory overrides its
|
||||
binding."""
|
||||
_git_repo(tmp_path, "git@git.example.com:someone/thing.git")
|
||||
assert _sh(f'scribe_scope_query "{tmp_path}"', tmp_path).startswith("repo=")
|
||||
_marker(tmp_path, instance=INSTANCE, project_id=2)
|
||||
assert _sh(f'scribe_scope_query "{tmp_path}"', tmp_path) == "project_id=2"
|
||||
|
||||
|
||||
def test_without_a_marker_the_git_remote_still_answers(tmp_path):
|
||||
"""The path every existing install is on — it must not have moved."""
|
||||
_git_repo(tmp_path, "git@git.example.com:someone/thing.git")
|
||||
got = _sh(f'scribe_scope_query "{tmp_path}"', tmp_path)
|
||||
assert got.startswith("repo=") and "git.example.com" in got.replace("%2F", "/")
|
||||
|
||||
|
||||
def test_a_plain_directory_with_no_marker_scopes_to_nothing(tmp_path):
|
||||
"""Not an error — the caller sends no scope and the server says so."""
|
||||
assert _sh(f'scribe_scope_query "{tmp_path}"', tmp_path) == ""
|
||||
|
||||
|
||||
@pytest.mark.parametrize("body", ["", "not json at all", "{}", '{"project_id": 0}',
|
||||
'{"project_id": "../../etc"}', "[1,2,3]"])
|
||||
def test_an_unusable_marker_is_refused_rather_than_sent(tmp_path, body):
|
||||
"""A marker is operator-written and can say anything. Nothing that is not
|
||||
a positive integer may reach the query string."""
|
||||
(tmp_path / ".scribe").write_text(body)
|
||||
assert _sh(f'scribe_scope_query "{tmp_path}"', tmp_path) == ""
|
||||
|
||||
|
||||
def test_the_reason_survives_the_command_substitution_that_fetches_it(tmp_path):
|
||||
"""Regression on a bug this nearly shipped with.
|
||||
|
||||
The reason used to be a global the function set, which every caller reads
|
||||
through `$( )` — a subshell, so the assignment died with it and the caller
|
||||
saw an UNSET variable. The hooks all run `set -u`, where reading one aborts
|
||||
the script: a directory with a misaddressed marker would have cost the
|
||||
session its entire SessionStart context, to fetch a warning about a file.
|
||||
|
||||
So the reason comes back through stdout with the id, and this asserts on
|
||||
the shape the caller actually uses: read in a subshell, split, and used
|
||||
under `set -u` without the shell dying.
|
||||
"""
|
||||
_marker(tmp_path, instance="https://elsewhere.example.org", project_id=2)
|
||||
got = _sh(
|
||||
f'read_out=$(scribe_marker_read "$(scribe_marker_file "{tmp_path}")")\n'
|
||||
'printf "id=[%s] why=[%s]" "${read_out%%$\'\\t\'*}" "${read_out#*$\'\\t\'}"',
|
||||
tmp_path,
|
||||
)
|
||||
assert got.startswith("id=[]")
|
||||
assert "elsewhere.example.org" in got
|
||||
|
||||
|
||||
def test_the_walk_up_terminates_at_the_root(tmp_path):
|
||||
"""No marker anywhere above: the loop must end rather than spin. tmp_path
|
||||
is several levels down from /, so this really does walk."""
|
||||
deep = tmp_path / "a" / "b" / "c"
|
||||
deep.mkdir(parents=True)
|
||||
assert _sh(f'scribe_scope_query "{deep}"', tmp_path) == ""
|
||||
@@ -215,6 +215,31 @@ async def test_build_session_context_unbound_repo_emits_bind_hint():
|
||||
assert "## Active project" not in ctx
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_project_id_that_does_not_resolve_is_reported_not_dropped():
|
||||
"""A `.scribe` marker names a project directly (#4085), so for the first
|
||||
time a caller arrives holding a pointer it BELIEVES in. If the id is for
|
||||
another instance, or names a deleted project, the session has to be told —
|
||||
it cannot infer it from an absence.
|
||||
|
||||
This used to render nothing whatsoever: the branch hung off `if project_id`
|
||||
as an `elif`, so an id that was sent and failed took the outer arm, found
|
||||
no project, and fell out of the block having said nothing at all.
|
||||
"""
|
||||
from scribe.services.plugin_context import build_session_context
|
||||
with patch("scribe.services.plugin_context.projects_svc.get_project",
|
||||
AsyncMock(return_value=None)):
|
||||
out = await build_session_context(user_id=7, project_id=41)
|
||||
|
||||
ctx = out["context"]
|
||||
assert out["project"] is None
|
||||
assert "## Project 41 could not be loaded" in ctx
|
||||
assert "list_projects" in ctx
|
||||
# Not mistaken for the repo case, which has a different remedy.
|
||||
assert "## Repository not yet bound" not in ctx
|
||||
assert "No Scribe project is bound" not in ctx
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_build_process_manifest_renders_stub_specs():
|
||||
items = [
|
||||
|
||||
Reference in New Issue
Block a user