Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion scripts/prose_lint.py
Original file line number Diff line number Diff line change
Expand Up @@ -1106,11 +1106,37 @@ def main(argv: list[str] | None = None) -> int:

rules = set(a.checks or DEFAULT_RULES)

# The invariant: the rule set is decided by what is scanned, and no path here reads the working directory to decide it.
# Only a repository declares a model, so two of them refuse and anything else resolves to one anchor.
# TestScanRootDecidesTheRuleSet carries the cases and the reason each one exists.
scan_paths = a.paths or ['.']
# A path that does not exist is refused rather than absorbed.
# `discover` reads a non-file, non-directory argument as `.`, so a typo scanned the caller's directory while the rule set anchored on the missing path's parent.
absent = [p for p in scan_paths if not Path(p).exists()]
if absent:
print(f"error: requested path(s) do not exist: {', '.join(sorted(absent))}. Refusing "
'rather than falling back to the current directory, which would scan one tree and '
'choose the rule set from another.', file=sys.stderr)
return 2
git_roots = {found for found in (repo_root(Path(p)) for p in scan_paths) if found}
if len(git_roots) > 1:
print('error: the requested paths span more than one repository (' +
', '.join(sorted(git_roots)) + '). Each declares its own workflow model, so no '
'single rule set is correct for all of them. Run the gate once per repository.',
file=sys.stderr)
return 2
# A file anchors on its own parent rather than on `.`, which is where the caller stands.
if git_roots:
scan_root = Path(next(iter(git_roots)))
else:
first = Path(scan_paths[0])
scan_root = first if first.is_dir() else first.parent

# An operational repository's runbook carries the literal path an operator types.
# That is the repository's own content, not an agent quoting an environment it observed.
# The skip is announced, since a rule that silently stops running reads as one that passed.
# That is the same failure the diff-scope floor below exists to prevent.
if 'home-path' in rules and operational_checkout(Path(repo_root(Path('.')) or '.')):
if 'home-path' in rules and operational_checkout(scan_root):
rules.discard('home-path')
print('note: home-path is not checked in an operational repository, where an absolute '
'path is the operator instruction rather than observed data.', file=sys.stderr)
Expand Down
150 changes: 148 additions & 2 deletions scripts/test_prose_lint.py
Original file line number Diff line number Diff line change
Expand Up @@ -1528,20 +1528,26 @@ def test_a_path_under_no_repository_is_refused_too(self) -> None:
repository-relative ones, so the scope drops every file and the run exits 0. Testing for
a *different* root missed this, because there is no root to differ from.
"""
# A real directory, since a path that does not exist is refused earlier for another reason.
# That would pass this assertion without ever exercising the diff guard.
loose = self.tmp / 'loose'
loose.mkdir()
with mock.patch.object(prose_lint, 'repo_root',
side_effect=lambda p: '/hub' if str(p) == '.' else ''), \
contextlib.redirect_stderr(io.StringIO()) as err:
self.assertEqual(2, prose_lint.main(['--diff', 'HEAD', '/tmp/loose']))
self.assertEqual(2, prose_lint.main(['--diff', 'HEAD', str(loose)]))
self.assertIn('no git repository', err.getvalue())

def test_list_files_still_reports_scope_across_repositories(self) -> None:
"""It reports the scan scope and never consults the diff, so the guard must not stop it."""
clean = self.tmp / 'clean.md'
clean.write_text('fine\n', encoding='utf-8')
other = self.tmp / 'other'
other.mkdir()
with mock.patch.object(prose_lint, 'repo_root',
side_effect=lambda p: '/hub' if str(p) == '.' else '/other'), \
mock.patch.object(prose_lint, 'discover', return_value=[clean]):
self.assertEqual(0, prose_lint.main(['--list-files', '--diff', 'HEAD', '/other/tree']))
self.assertEqual(0, prose_lint.main(['--list-files', '--diff', 'HEAD', str(other)]))

def test_a_matching_repository_is_not_refused(self) -> None:
"""The guard must not reject the ordinary case it sits in front of."""
Expand Down Expand Up @@ -1794,6 +1800,146 @@ def test_a_release_repository_still_reports_the_finding(self) -> None:
self.assertEqual(1, prose_lint.main(['--check', 'home-path']))


class TestScanRootDecidesTheRuleSet(unittest.TestCase):
"""The rule set follows the repository being scanned, never the directory the caller stands in.

Read from `.`, the exemption discarded home-path on a release repository whenever the caller
happened to stand in an operational one, announced the skip on stderr, and exited 0. That
silences the rule that exists because real paths reached a public comment.

Every case here gives the two roots *different* models. `TestOperationalExemption` above mocks
`repo_root` to one value for every argument, which cannot tell the caller's repository from the
scanned one, and is why this defect survived it.
"""

def setUp(self) -> None:
self.tmp = Path(self.enterContext(tempfile.TemporaryDirectory()))
self.enterContext(contextlib.redirect_stdout(io.StringIO()))
self.err = self.enterContext(contextlib.redirect_stderr(io.StringIO()))
self.operational = self._repo('operational', ('repo-config', 'operational', 'develop.json'))
self.release = self._repo('release', ('repo-config', 'develop.json'))
self.bait = self.release / 'runbook.md'
self.bait.write_text(f'Deploy into {NIX_HOME}/stack here.\n', encoding='utf-8')

def _repo(self, name: str, payload: tuple[str, ...]) -> Path:
root = self.tmp / name
target = root.joinpath(*payload)
target.parent.mkdir(parents=True, exist_ok=True)
target.write_text('{}\n', encoding='utf-8')
return root

def _roots(self, cwd: Path, scanned: Path):
"""Resolve `.` to one repository and every other path to another."""
return lambda p: str(cwd) if str(p) == '.' else str(scanned)

def test_standing_in_an_operational_repo_does_not_exempt_a_release_repo(self) -> None:
"""The defect: the caller's directory turned the rule off on the repository being scanned."""
with mock.patch.object(prose_lint, 'repo_root',
side_effect=self._roots(self.operational, self.release)), \
mock.patch.object(prose_lint, 'discover', return_value=[self.bait]):
self.assertEqual(1, prose_lint.main([str(self.release), '--check', 'home-path']))
self.assertNotIn('operational repository', self.err.getvalue())

def test_standing_in_a_release_repo_does_not_un_exempt_an_operational_repo(self) -> None:
"""The inverse error, which reports a finding the exemption exists to suppress."""
runbook = self.operational / 'runbook.md'
runbook.write_text(f'Deploy into {NIX_HOME}/stack here.\n', encoding='utf-8')
with mock.patch.object(prose_lint, 'repo_root',
side_effect=self._roots(self.release, self.operational)), \
mock.patch.object(prose_lint, 'discover', return_value=[runbook]):
self.assertEqual(0, prose_lint.main([str(self.operational), '--check', 'home-path']))
self.assertIn('operational repository', self.err.getvalue())

def test_paths_spanning_two_repositories_refuse_rather_than_pick_one(self) -> None:
"""Two repositories declare two models, and one rule set cannot be correct for both."""
with mock.patch.object(prose_lint, 'repo_root', side_effect=lambda p: str(Path(p))):
self.assertEqual(2, prose_lint.main(
[str(self.release), str(self.operational), '--check', 'home-path']))
self.assertIn('more than one repository', self.err.getvalue())

def test_a_path_that_does_not_exist_is_refused_rather_than_absorbed(self) -> None:
"""`discover` reads a non-file, non-directory argument as `.`.

A typo therefore scanned the caller's directory while the rule set anchored on the missing
path's parent, so the run described one tree and judged it by another.
"""
with mock.patch.object(prose_lint, 'repo_root', return_value=''):
self.assertEqual(2, prose_lint.main(
[str(self.tmp / 'no-such-dir'), '--check', 'home-path']))
self.assertIn('do not exist', self.err.getvalue())

def test_an_existing_path_is_not_refused_by_that_check(self) -> None:
"""The guard must not reject the ordinary case it sits in front of."""
good = self.tmp / 'good.md'
good.write_text('Nothing here breaks a rule.\n', encoding='utf-8')
with mock.patch.object(prose_lint, 'repo_root', return_value=''), \
mock.patch.object(prose_lint, 'discover', return_value=[good]):
self.assertEqual(0, prose_lint.main([str(good), '--check', 'home-path']))
self.assertNotIn('do not exist', self.err.getvalue())

def test_several_paths_under_no_repository_are_not_a_conflict(self) -> None:
"""Only a repository declares a model, so two loose paths are not ambiguous.

Refusing on distinct filesystem paths broke the ordinary multi-file invocation outside a
checkout, where every argument resolves somewhere different.
"""
loose = self.tmp / 'loose'
(loose / 'docs').mkdir(parents=True)
one, two = loose / 'a.md', loose / 'docs' / 'b.md'
for target in (one, two):
target.write_text('Nothing to find here.\n', encoding='utf-8')
with mock.patch.object(prose_lint, 'repo_root', return_value=''), \
mock.patch.object(prose_lint, 'discover', return_value=[one, two]):
self.assertEqual(0, prose_lint.main([str(one), str(two), '--check', 'home-path']))
self.assertNotIn('more than one repository', self.err.getvalue())

def test_one_repository_plus_a_loose_path_is_not_a_conflict(self) -> None:
"""One model is in play, so there is nothing to disambiguate."""
loose = self.tmp / 'loose'
loose.mkdir()
bait = loose / 'notes.md'
bait.write_text(f'Deploy into {NIX_HOME}/stack here.\n', encoding='utf-8')
with mock.patch.object(prose_lint, 'repo_root',
side_effect=lambda p: str(self.release) if Path(p) == self.release else ''), \
mock.patch.object(prose_lint, 'discover', return_value=[bait]):
# The one repository in play is a release repo, so home-path stays on and finds it.
self.assertEqual(1, prose_lint.main(
[str(self.release), str(loose), '--check', 'home-path']))
self.assertNotIn('more than one repository', self.err.getvalue())

def test_a_loose_file_anchors_on_its_own_parent_rather_than_on_the_caller(self) -> None:
"""The fallback must not reintroduce the dependency the fix removes.

A *file* argument is the case that matters. Anchoring it on `.` puts the caller's directory
back in charge, so scanning a loose file from inside an operational repository exempts it.
Passing a directory here would not exercise that branch at all, which is how the earlier
version of this test let the regression through.
"""
loose = self.tmp / 'loose'
loose.mkdir()
bait = loose / 'notes.md'
bait.write_text(f'Deploy into {NIX_HOME}/stack here.\n', encoding='utf-8')
# The caller stands somewhere operational; the scanned file's own directory does not.
with mock.patch.object(prose_lint, 'repo_root', return_value=''), \
mock.patch.object(prose_lint, 'operational_checkout',
side_effect=lambda root: Path(root) == Path('.')), \
mock.patch.object(prose_lint, 'discover', return_value=[bait]):
self.assertEqual(1, prose_lint.main([str(bait), '--check', 'home-path']))
self.assertNotIn('operational repository', self.err.getvalue())

def test_a_loose_directory_anchors_on_itself(self) -> None:
loose = self.tmp / 'loose'
loose.mkdir()
bait = loose / 'notes.md'
bait.write_text(f'Deploy into {NIX_HOME}/stack here.\n', encoding='utf-8')
with mock.patch.object(prose_lint, 'repo_root', return_value=''), \
mock.patch.object(prose_lint, 'operational_checkout',
side_effect=lambda root: Path(root) == Path('.')), \
mock.patch.object(prose_lint, 'discover', return_value=[bait]):
self.assertEqual(1, prose_lint.main([str(loose), '--check', 'home-path']))
self.assertNotIn('operational repository', self.err.getvalue())


class TestDiffScopeFloor(unittest.TestCase):
"""The assertion every `--diff` verdict rests on, that the run matched something it was given.

Expand Down