Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
27 changes: 26 additions & 1 deletion scripts/prose_lint.py
Original file line number Diff line number Diff line change
Expand Up @@ -1106,11 +1106,36 @@ def main(argv: list[str] | None = None) -> int:

rules = set(a.checks or DEFAULT_RULES)

# The rule set is decided by the repository being scanned, never by the directory the caller stands in.
# Reading it from `.` discarded home-path on a release repository whenever the caller happened to stand in an operational one.
# That silenced the rule that exists because real paths reached a public comment, announcing it on stderr and exiting 0.
# A run spanning two repositories refuses instead of picking one, since the two can declare different models and one rule set cannot be right for both.
# Only a repository declares a workflow model, so only distinct repositories are ambiguous.
# Two loose paths under no repository are not a conflict.
# Refusing on them would break the ordinary multi-file invocation outside a checkout, where every argument resolves somewhere different.
scan_paths = a.paths or ['.']
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
# With one repository in play, the model is that repository's own.
# With none, a file anchors on its own parent rather than on `.`, which is where the caller stands.
# Anchoring a loose file on `.` reintroduces this defect in miniature, exempting a file the caller's repository does not contain.
# A bare filename still anchors on `.`, correctly, since that is the directory holding it.
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
120 changes: 120 additions & 0 deletions scripts/test_prose_lint.py
Original file line number Diff line number Diff line change
Expand Up @@ -1794,6 +1794,126 @@ 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_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