Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
45 changes: 37 additions & 8 deletions .claude/CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,53 @@ Consult the justfile to learn how to run tests and checks:

@../justfile

## Comments
## Git

Don't add comments or docstrings that justify or narrate a change; comments describe how the code behaves, not why an edit was made. Trivial changes get no comment.
I create the branches, commit, and push. Never do any of that, and never offer to: finishing a piece of work means reporting it with the tests and checks green, not proposing a commit.

## Code review

When reviewing code, whether standalone or as part of the TDD refactor step, pay attention to the following:

- **Comments**: don't add comments or docstrings that justify or narrate a change; comments describe how the code behaves, not why an edit was made. Trivial changes get no comment. A docstring describing an approach the code has moved on from is worse than none, so re-read the docstrings around a change and check they still describe what is there.
- **Duplication**: look for the same decision taken in more than one place, rather than for repeated lines. A rule every module has to remember to apply is duplication too: prefer stating it once, somewhere it cannot be forgotten. In tests, repeated setup or a repeated assertion is worth naming as a helper.
- **Complexity**: a function should hold one decision. Watch for nesting, for flag parameters that make one function do two things, and for long parameter lists. When a docstring needs several sentences to describe the control flow, the code is doing too much, rather than the docstring being too short.
- **Missing abstractions**: values that always travel together want a type, and a sequence of calls that callers must make in the right order wants a name. Raw strings, tuples, and dicts standing in for domain concepts are the usual smell; look for a type that already models the concept before adding one.
- **Readability**: use the domain vocabulary the README establishes, consistently across code, docstrings, log messages, and tests. Prefer an early return to a nested conditional, say what a test asserts in its method name, and keep implementation terms out of anything the user reads.

## TDD

Develop test-first, in small steps, and stop after each step so I can steer:
Develop test-first, in small steps, and hand back after each step so I can steer. Steps 3, 4, and 5 each end a turn: report the red run and stop, report the green run and stop, report the refactor and stop. Running the cycle end to end and reporting it afterwards leaves me nothing to steer.

1. **Pick a starting point.** For a new feature, propose at most five candidate tests and let me choose which to start with. Build a behaviour before its off-switch: don't test an `ignore` scope, a flag, or any other suppression until the thing it suppresses exists.
1. **Pick a starting point.** For a new feature, propose at most five candidate tests and let me choose which to start with. When the test to start with is already prescribed — by the issue, or by me — say that is why you are skipping the proposal, rather than skipping it silently. Build a behaviour before its off-switch: don't test an opt-out, a flag, or any other suppression until the thing it suppresses exists.
2. **Write the test.** Only the test — don't design or write the implementation yet. Follow the conventions of the nearest existing test for the same kind of behaviour.
3. **Predict the failure, then run `just test`.** Say how the test will fail, naming the exact error, before running it. Verify that only the new test fails, and that it fails as predicted. When anything else happens, analyse why before writing any code.
3. **Predict the failure, then run `just test`.** Say how the test will fail, naming the exact error, before running it. Run the whole suite, not just the new test's module, so that "only the new test fails" covers the whole suite. Verify that only the new test fails, and that it fails as predicted. When anything else happens, analyse why before writing any code.
4. **Implement** the smallest change that makes the test pass, then run `just test` again.
5. **Refactor** production and test code, then run `just test` and `just check`.
5. **Refactor** production and test code, then run `just test` and `just check`. The cycle ends here, not at step 4: don't report the work finished with this step still outstanding, and don't wait to be asked for it.

A few rules that keep the cycle honest:

- Park test cases you are not writing yet in an explicit list instead of writing them all at once, and work through the list once the main path works.
- Park test cases you are not writing yet in an explicit list instead of writing them all at once, and work through the list once the main path works. One test starts a cycle even when two look like one step, say the same defect in two modules: fixing both on one branch is not a reason to write both tests at once.
- A test that pins down existing behaviour drives no code, so predict it passes and say so; it closes a gap in intent, not in behaviour.
- Coverage must stay at 100%. A gap after implementing points at a test case worth adding, not at a line worth excluding.
- A refactor that moves the mechanism a test relies on can leave the test green but vacuous. Stub the mechanism out, confirm the test fails, and restore it.
- Coverage must stay at 100%, which `just test` already enforces, so a passing run needs no separate coverage command. A gap after implementing points at a test case worth adding, not at a line worth excluding.
- Treat a failing existing test as a signal: work out whether its premise legitimately changed and say why, rather than patching the assertion to match the new output.
- Assert the actual value first, as in `assertEqual(actual, expected)`. Both orders pass locally, but the reverse is flagged in CI, so it costs a round-trip to find out.
- `just check` prints `NOK` for a failing check, so don't count `OK` occurrences to conclude it passed: `NOK` contains `OK`.

## Documentation

- README.md is generated: edit `docs/README.md.in` and regenerate with `just readme`. An edit to README.md itself is silently lost on the next run.
- A user-visible change gets a changelog entry under `[Unreleased]`, one line naming the behaviour and linking the issue. The detail belongs in the README, not in the changelog.

## Self-improvement

At the end of a session, review how the session went against this file and fold what you learn back into it.

- Initiate this yourself when the work looks finished, with the tests and checks green and nothing left to steer, instead of waiting to be asked. When I take the session somewhere else instead, let it go.
- Review before editing, and report the review: which guidelines were followed, which were deviated from, and what actually happened in each case. Be factual and brief; state the deviation and move on.
- Separate the two causes, because they call for different edits. A guideline that was clear and simply not followed needs sharpening, so it cannot be read past a second time; a guideline that turned out to be silent on a case needs a line of its own.
- Ground every edit in something that happened this session. Don't add best practice that nothing in the session called for.
- Keep this file short: rewrite an existing line rather than adding one, and delete lines that events have overtaken. It is read in full at the start of every session, so its length is a cost paid every time.
- Make the edits, then report each one with the moment in the session that prompted it, so I can drop the ones I disagree with.
- This file holds how we work. What the code does belongs in the code, its docstrings, the README, and the changelog, so don't move project facts in here.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
### Fixed

- Log at log-level DEBUG when an `# update-time: ignore[stale]` or `# update-time: ignore[yanked]` marker holds a warning back. Closes [#169](https://github.com/ICTU/update-time/issues/169).
- Attribute the staleness, yank, redundant-bound, and GitHub pin log records to the updater that triggered them, so the source position in the log names the updater like every other record. Closes [#171](https://github.com/ICTU/update-time/issues/171).

## 0.0.21 - 2026-07-22

Expand Down
33 changes: 21 additions & 12 deletions src/update_time/io/log.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,34 +120,43 @@ def _restyle_delimited(text: Text, pattern: re.Pattern[str], style: str, *, keep
LOG_MESSAGE_FORMAT = "%(message)s"


# Files that wrap or dispatch logging on behalf of the updaters. Frames in these files are skipped when
# determining a log record's origin, so the reported origin is the updater that triggered the log rather than this
# wrapper or a registered engine (see `attribute_logs_to_caller`).
_helper_files = {str(Path(__file__).resolve())}
# This wrapper, and the packages that log on behalf of the updaters (see `attribute_logs_to_caller`). Frames in
# these are skipped when determining a log record's origin, so the reported origin is the updater that triggered the
# log rather than this wrapper or the shared machinery in between.
_wrapper_file = Path(__file__).resolve()
_helper_packages: set[Path] = set()


def attribute_logs_to_caller(module_file: str) -> None:
"""Register a module whose frames should be skipped when determining a log record's origin.
def attribute_logs_to_caller(package_file: str) -> None:
"""Register a package whose frames should be skipped when determining a log record's origin.

A module that logs on behalf of the updaters registers itself, so its frames are walked past and a log record
is attributed to the updater that triggered it rather than to the shared machinery in between.
A package whose modules log on behalf of the updaters registers itself, passing its `__init__.py`'s `__file__`,
so the frames of every module in it are walked past and a log record is attributed to the updater that triggered
it rather than to the shared machinery in between. Registration covers the whole package, so a module added to it
needs none of its own.
"""
_helper_files.add(str(Path(module_file).resolve()))
_helper_packages.add(Path(package_file).resolve().parent)


def _is_helper_frame(filename: str) -> bool:
"""Return whether the frame's file is this wrapper or a module in a registered package."""
path = Path(filename).resolve()
return path == _wrapper_file or any(path.is_relative_to(package) for package in _helper_packages)


def _caller_stacklevel() -> int:
"""Return the stacklevel of the first frame outside the logging and rewriting helpers.
"""Return the stacklevel of the first frame outside this wrapper and the registered packages.

A fixed stacklevel can't work because some log methods are called directly by an updater while others
are dispatched through the registered helper modules (with extra comprehension frames in between), so walk
are dispatched through a registered package (with extra comprehension frames in between), so walk
the stack to find the originating updater frame instead.
"""
level = 1 # Start at the frame that emits the record (Logger._log) and skip helper frames from there.
try:
frame: FrameType | None = sys._getframe(level) # noqa: SLF001
except ValueError: # pragma: no cover
return level
while frame is not None and str(Path(frame.f_code.co_filename).resolve()) in _helper_files:
while frame is not None and _is_helper_frame(frame.f_code.co_filename):
level += 1
frame = frame.f_back
return level
Expand Down
5 changes: 5 additions & 0 deletions src/update_time/references/__init__.py
Original file line number Diff line number Diff line change
@@ -1 +1,6 @@
"""Decide which version each pinned reference should update to, honouring its marker, and rewrite it in place."""

from update_time.io.log import attribute_logs_to_caller

# Every module in this package logs on behalf of the updaters, so records point at the updater.
attribute_logs_to_caller(__file__)
4 changes: 0 additions & 4 deletions src/update_time/references/file.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from typing import TYPE_CHECKING

from update_time.io.filesystem import glob
from update_time.io.log import attribute_logs_to_caller
from update_time.references.rewrite import update_references_in_lines

if TYPE_CHECKING:
Expand All @@ -18,9 +17,6 @@
from update_time.domain.bound import NewVersionGetter
from update_time.io.log import Logger

# This orchestration logs on behalf of the updaters, so records point at the updater.
attribute_logs_to_caller(__file__)


def rewrite_file(path: Path, transform: Callable[[list[str]], list[str]], logger: Logger) -> None:
"""Read the file, apply `transform` to its lines, and write it back only if the lines changed.
Expand Down
4 changes: 0 additions & 4 deletions src/update_time/references/rewrite.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
from update_time.domain.location import Location
from update_time.domain.marker import parse_marker
from update_time.domain.version import Reference
from update_time.io.log import attribute_logs_to_caller
from update_time.primitives.environment import EnvVar
from update_time.references.resolve import latest_version

Expand All @@ -29,9 +28,6 @@
from update_time.domain.marker import Marker
from update_time.domain.version import DependencyVersion
from update_time.io.log import Logger

attribute_logs_to_caller(__file__) # This engine logs on behalf of the updaters, so records point at the updater.

# Private channel that passes --allow-image-digest-drift from the CLI to the updater subprocesses: whether a
# re-pushed image digest should be adopted repo-wide.
ALLOW_IMAGE_DIGEST_DRIFT = EnvVar(
Expand Down
42 changes: 34 additions & 8 deletions tests/update_time/io/test_log.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@
from update_time.domain.bound import Redundancy, Verb
from update_time.domain.location import Location
from update_time.domain.marker import Marker
from update_time.domain.version import DependencyVersion, Yank
from update_time.domain.version import DependencyVersion, Reference, Yank
from update_time.io.log import DEPENDENCY_DELIMITER, LOCATION_DELIMITER, Logger, LogHighlighter, get_logger
from update_time.references import file
from update_time.references.github import latest_pin
from update_time.references.resolve import latest_version

from tests.update_time.helpers import bound, new_version_getter

Expand Down Expand Up @@ -535,18 +537,22 @@ def test_dependency_style_is_bold_white(self):


class LogOriginTests(TestCase):
"""Tests that log records are attributed to the originating updater, not the logging or rewriting helpers."""
"""Tests that log records are attributed to the originating updater, not to the shared machinery in between."""

def assert_origin_is_this_test(self, records: list[logging.LogRecord]) -> None:
"""Assert that every record names this test file, the caller of the shared machinery, as its origin."""
self.assertEqual({Path(record.pathname).name for record in records}, {"test_log.py"})

def test_direct_call_is_attributed_to_the_caller(self):
"""Test that a log method called directly reports the calling line as its origin."""
logger = Logger("origin direct")
with self.assertLogs(logger.log, level="DEBUG") as captured:
logger.path(Path.cwd())
self.assertEqual(Path(captured.records[0].pathname).name, "test_log.py")
self.assert_origin_is_this_test(captured.records)

def test_rewrite_helper_call_is_attributed_to_the_caller(self):
"""Test that logs emitted via the file-rewrite helpers report the helpers' caller, not a helper, as origin."""
logger = Logger("origin helper")
def test_file_rewrite_is_attributed_to_the_caller(self):
"""Test that logs emitted while rewriting a file report the rewriting's caller as their origin."""
logger = Logger("origin rewrite")
with TemporaryDirectory() as directory:
(Path(directory) / "config.yml").write_text("dependency: 1.0\n")
with (
Expand All @@ -560,5 +566,25 @@ def test_rewrite_helper_call_is_attributed_to_the_caller(self):
logger=logger,
start=Path(directory),
)
origins = {Path(record.pathname).name for record in captured.records}
self.assertEqual(origins, {"test_log.py"})
self.assert_origin_is_this_test(captured.records)

def test_version_decision_is_attributed_to_the_caller(self):
"""Test that a warning from the shared version decision reports its caller, not the decision, as origin."""
logger = Logger("origin decision")
stale = DependencyVersion("2.0", newest_published=datetime.now(UTC) - timedelta(days=1000))
reference = Reference("dependency", "1.0")
with self.assertLogs(logger.log, level="DEBUG") as captured:
latest_version(reference, lambda *_args: stale, Marker(), Location(Path.cwd()), logger)
self.assert_origin_is_this_test(captured.records)

def test_github_pin_decision_is_attributed_to_the_caller(self):
"""Test that a pin from the shared GitHub decision reports its caller, not the decision, as origin."""
logger = Logger("origin github")
latest = DependencyVersion("2.0", sha="a" * 40)
reference = Reference("owner/action", "1.0")
with (
patch("update_time.references.github.get_latest_version", Mock(return_value=latest)),
self.assertLogs(logger.log, level="DEBUG") as captured,
):
latest_pin(reference, Marker(), Location(Path.cwd()), logger)
self.assert_origin_is_this_test(captured.records)