Add an optional en-GB-oxendict spelling gate (default on) - #55
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
Validation
WalkthroughChangesOxford spelling gate
Sequence Diagram(s)sequenceDiagram
participant Copier
participant Makefile
participant Generator
participant Typos
participant CI
Copier->>Makefile: Render spellcheck target when enabled
CI->>Makefile: Run make spellcheck
Makefile->>Generator: Refresh dictionary and write typos.toml
Makefile->>Typos: Scan Markdown with generated configuration
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 4 warnings, 2 inconclusive)
✅ Passed checks (12 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21fa0e0aef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
31e24b8 to
52292bd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@template/`{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py:
- Around line 34-50: Document all specified public interfaces with NumPy-style
docstrings: in template/{% if en_gb_oxendict %}scripts{% endif
%}/typos_rollout.py lines 34-50, add Attributes and invariants for Dictionary
and RefreshResult; lines 106-108 document load_dictionary parameters and parse
failures; lines 111-155 document merge and generated-mapping conflict behavior;
lines 171-192 document rendering output and TOML validation failures; lines
209-211 document write behavior and filesystem failures; and lines 383-398
document source selection, offline behavior, statuses, and exceptions. In
template/{% if en_gb_oxendict %}scripts{% endif %}/generate_typos_config.py
lines 25-58, document cache merging, generated output, and refresh options. Use
structured Parameters, Returns, Raises, and relevant Attributes sections for
public functions, classes, and methods; retain only single-line summaries for
private helpers.
- Around line 354-364: Update the refresh flow around _read_metadata,
_conditional_headers, and _remote_is_not_newer to detect when saved["source"]
differs from source, discard the prior metadata and cache validators for that
request, and prevent returning the old cache as current. Add a regression test
that refreshes two different URLs using matching ETags or timestamps and
verifies the second URL is fetched and its cache is retained.
- Around line 317-325: Update the HTTP request flow around _https_request and
urllib.request.urlopen so every redirect target is validated before it is
opened, rejecting any target whose URL scheme is not HTTPS. Preserve the
existing HTTPS-only validation for the initial source and ensure redirects
cannot downgrade to HTTP.
In `@tests/test_template/test_spelling_gate.py`:
- Around line 72-89: Update test_spelling_gate_config_matches_generator to run
scripts/generate_typos_config.py in the rendered project with
subprocess.run(check=True, cwd=project) before reading typos.toml; add the
required subprocess import and keep the existing assertions against the
regenerated file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ee87e1f6-4519-4245-a9bc-e55dd5440784
📒 Files selected for processing (18)
copier.yamldocs/users-guide.mdtemplate/.github/workflows/ci.yml.jinjatemplate/AGENTS.md.jinjatemplate/Makefile.jinjatemplate/docs/developers-guide.md.jinjatemplate/docs/documentation-style-guide.mdtemplate/docs/repository-layout.md.jinjatemplate/docs/users-guide.md.jinjatemplate/{% if en_gb_oxendict %}scripts{% endif %}/generate_typos_config.pytemplate/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.pytemplate/{% if en_gb_oxendict %}typos.local.toml{% endif %}.jinjatemplate/{% if en_gb_oxendict %}typos.toml{% endif %}.jinjatests/helpers/rendering.pytests/helpers/tooling_contracts/makefile.pytests/helpers/tooling_contracts/workflows.pytests/test_template/__snapshots__/test_snapshots.ambrtests/test_template/test_spelling_gate.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/whitaker(auto-detected)
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@template/`{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py:
- Around line 34-50: Document all specified public interfaces with NumPy-style
docstrings: in template/{% if en_gb_oxendict %}scripts{% endif
%}/typos_rollout.py lines 34-50, add Attributes and invariants for Dictionary
and RefreshResult; lines 106-108 document load_dictionary parameters and parse
failures; lines 111-155 document merge and generated-mapping conflict behavior;
lines 171-192 document rendering output and TOML validation failures; lines
209-211 document write behavior and filesystem failures; and lines 383-398
document source selection, offline behavior, statuses, and exceptions. In
template/{% if en_gb_oxendict %}scripts{% endif %}/generate_typos_config.py
lines 25-58, document cache merging, generated output, and refresh options. Use
structured Parameters, Returns, Raises, and relevant Attributes sections for
public functions, classes, and methods; retain only single-line summaries for
private helpers.
- Around line 354-364: Update the refresh flow around _read_metadata,
_conditional_headers, and _remote_is_not_newer to detect when saved["source"]
differs from source, discard the prior metadata and cache validators for that
request, and prevent returning the old cache as current. Add a regression test
that refreshes two different URLs using matching ETags or timestamps and
verifies the second URL is fetched and its cache is retained.
- Around line 317-325: Update the HTTP request flow around _https_request and
urllib.request.urlopen so every redirect target is validated before it is
opened, rejecting any target whose URL scheme is not HTTPS. Preserve the
existing HTTPS-only validation for the initial source and ensure redirects
cannot downgrade to HTTP.
In `@tests/test_template/test_spelling_gate.py`:
- Around line 72-89: Update test_spelling_gate_config_matches_generator to run
scripts/generate_typos_config.py in the rendered project with
subprocess.run(check=True, cwd=project) before reading typos.toml; add the
required subprocess import and keep the existing assertions against the
regenerated file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ee87e1f6-4519-4245-a9bc-e55dd5440784
📒 Files selected for processing (18)
copier.yamldocs/users-guide.mdtemplate/.github/workflows/ci.yml.jinjatemplate/AGENTS.md.jinjatemplate/Makefile.jinjatemplate/docs/developers-guide.md.jinjatemplate/docs/documentation-style-guide.mdtemplate/docs/repository-layout.md.jinjatemplate/docs/users-guide.md.jinjatemplate/{% if en_gb_oxendict %}scripts{% endif %}/generate_typos_config.pytemplate/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.pytemplate/{% if en_gb_oxendict %}typos.local.toml{% endif %}.jinjatemplate/{% if en_gb_oxendict %}typos.toml{% endif %}.jinjatests/helpers/rendering.pytests/helpers/tooling_contracts/makefile.pytests/helpers/tooling_contracts/workflows.pytests/test_template/__snapshots__/test_snapshots.ambrtests/test_template/test_spelling_gate.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/whitaker(auto-detected)
🛑 Comments failed to post (3)
template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py (3)
34-50: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document all public interfaces in NumPy style.
Add structured
Parameters,Returns,Raises, and relevantAttributessections. Keep single-line summaries only for private helpers.
template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L34-L50: DocumentDictionaryandRefreshResultattributes and invariants.template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L106-L108: Documentload_dictionaryinputs and parse failures.template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L111-L155: Document merge and generated-mapping conflict behaviour.template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L171-L192: Document rendering output and TOML validation failures.template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L209-L211: Document write behaviour and filesystem failures.template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L383-L398: Document source selection, offline behaviour, statuses, and exceptions.template/{% if en_gb_oxendict %}scripts{% endif %}/generate_typos_config.py#L25-L58: Document cache merging, generated output, and refresh options.As per path instructions, “Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.”📍 Affects 2 files
template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L34-L50(this comment)template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L106-L108template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L111-L155template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L171-L192template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L209-L211template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L383-L398template/{% if en_gb_oxendict %}scripts{% endif %}/generate_typos_config.py#L25-L58🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@template/`{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py around lines 34 - 50, Document all specified public interfaces with NumPy-style docstrings: in template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py lines 34-50, add Attributes and invariants for Dictionary and RefreshResult; lines 106-108 document load_dictionary parameters and parse failures; lines 111-155 document merge and generated-mapping conflict behavior; lines 171-192 document rendering output and TOML validation failures; lines 209-211 document write behavior and filesystem failures; and lines 383-398 document source selection, offline behavior, statuses, and exceptions. In template/{% if en_gb_oxendict %}scripts{% endif %}/generate_typos_config.py lines 25-58, document cache merging, generated output, and refresh options. Use structured Parameters, Returns, Raises, and relevant Attributes sections for public functions, classes, and methods; retain only single-line summaries for private helpers.Source: Path instructions
317-325: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Python 3.13 official documentation urllib.request HTTPRedirectHandler HTTPS to HTTP redirect default💡 Result:
In Python 3.13, the
urllib.request.HTTPRedirectHandlerdoes not inherently block or restrict redirects from HTTPS to HTTP by default based on the protocol switch itself [1][2]. TheHTTPRedirectHandlerperforms a security check on the scheme of the redirected URL, but it permits transitions between 'http', 'https', and 'ftp' [1][3]. Specifically, the internalredirect_requestmethod verifies the scheme of the new URL: if urlparts.scheme not in ('http', 'https', 'ftp', ''): raise HTTPError(...) As long as the target URL uses one of these allowed schemes, the redirect is processed [1][2]. There is no default logic in the standard library'sHTTPRedirectHandlerthat inspects the original request's protocol (HTTPS) and prevents it from being redirected to an insecure protocol (HTTP) [1][4]. If you require stricter behavior—such as disallowing "downgrade" redirects from HTTPS to HTTP—you must subclassHTTPRedirectHandlerand override theredirect_requestmethod to implement custom validation logic that checks the scheme of both the current request and the target URL [2][5].Citations:
- 1: https://github.com/python/cpython/blob/3.13/Lib/urllib/request.py
- 2: https://docs.python.org/3/library/urllib.request.html
- 3: https://github.com/python/cpython/blob/36e4ffc1/Doc/library/urllib.request.rst
- 4: https://github.com/python/cpython/blob/69851a64/Lib/urllib/request.py
- 5: https://www.pythonlore.com/managing-http-redirects-with-http-client-httpredirecthandler/
🏁 Script executed:
#!/bin/bash set -e file=$(find template -path '*typos_rollout.py' -print -quit) printf '%s\n' "FILE=$file" sed -n '280,345p' "$file" rg -n "_https_request|urlopen|HTTPRedirectHandler|source|_refresh_http" template -g '*.py' || trueRepository: leynos/agent-template-rust
Length of output: 6324
🏁 Script executed:
#!/bin/bash set -e file=$(find template -path '*typos_rollout.py' -print -quit) sed -n '345,380p' "$file" python3 - <<'PY' import sys import urllib.request handler = urllib.request.HTTPRedirectHandler() request = urllib.request.Request("https://example.test/dictionary") redirected = handler.redirect_request( request, None, 302, "Found", {"location": "http://attacker.test/dictionary"}, ) print(f"python={sys.version.split()[0]}") print(f"redirect_request_result={redirected.full_url if redirected else None}") PYRepository: leynos/agent-template-rust
Length of output: 1707
🏁 Script executed:
#!/bin/bash set -e python3 - <<'PY' import sys import urllib.request handler = urllib.request.HTTPRedirectHandler() request = urllib.request.Request("https://example.test/dictionary") redirected = handler.redirect_request( request, None, 302, "Found", {"location": "http://attacker.test/dictionary"}, "http://attacker.test/dictionary", ) print(f"python={sys.version.split()[0]}") print(f"redirect_request_result={redirected.full_url if redirected else None}") PYRepository: leynos/agent-template-rust
Length of output: 234
Reject non-HTTPS redirect targets before opening them.
urllib.request.urlopen()follows HTTPS-to-HTTP redirects. Validate every redirect target to prevent an on-path attacker from altering the spelling configuration or bypassing the gate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@template/`{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py around lines 317 - 325, Update the HTTP request flow around _https_request and urllib.request.urlopen so every redirect target is validated before it is opened, rejecting any target whose URL scheme is not HTTPS. Preserve the existing HTTPS-only validation for the initial source and ensure redirects cannot downgrade to HTTP.
354-364: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reset validators when the remote source changes.
Check
saved["source"]againstsourcebefore building conditional headers or comparing response validators. A caller can changemain(..., source=...), but this path reuses metadata from the previous URL. Matching ETags or timestamps can then returncurrentand retain a valid cache from the wrong source.Add a regression test that refreshes two different URLs with matching validators.
Proposed fix
saved = _read_metadata(metadata) + if saved.get("source") != source: + saved = {} request = _https_request(source, _conditional_headers(saved))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.saved = _read_metadata(metadata) if saved.get("source") != source: saved = {} request = _https_request(source, _conditional_headers(saved)) try: with urllib.request.urlopen( # noqa: S310 - _https_request rejects non-HTTPS URLs. request, timeout=30.0, ) as response: if response.status == HTTP_NOT_MODIFIED and _valid_cache(cache): return RefreshResult("current", cache) if _valid_cache(cache) and _remote_is_not_newer(saved, response.headers): return RefreshResult("current", cache)🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 356-359: Request-controlled URL passed to urlopen; validate against an allowlist to prevent SSRF.
Context: urllib.request.urlopen( # noqa: S310 - _https_request rejects non-HTTPS URLs.
request,
timeout=30.0,
)
Note: [CWE-918] Server-Side Request Forgery (SSRF).(urlopen-unsanitized-data)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@template/`{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py around lines 354 - 364, Update the refresh flow around _read_metadata, _conditional_headers, and _remote_is_not_newer to detect when saved["source"] differs from source, discard the prior metadata and cache validators for that request, and prevent returning the old cache as current. Add a regression test that refreshes two different URLs using matching ETags or timestamps and verifies the second URL is fetched and its cache is retained.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/users-guide.md`:
- Around line 80-84: Update the make spellcheck entry in the users guide to
state that it is available only when en_gb_oxendict is enabled, while preserving
the existing description of its behavior. Ensure the documentation does not
present this target as unconditional.
In `@scripts/generate_typos_config.py`:
- Around line 94-129: Update the Raises section of the refresh function’s
docstring to explicitly document rollout.DictionaryTooLargeError for local or
remote dictionaries exceeding the fixed input limit, while preserving the
existing ValueError description for other validation failures.
In `@scripts/typos_rollout.py`:
- Around line 627-671: Update both scripts/typos_rollout.py (lines 627-671 and
166-176) and template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py
(lines 627-671 and 166-176): extract the repeated stale-cache handling from
_refresh_http into _fallback_or_raise and call it from all three exception
handlers, and change _cache_lock’s return annotation from typ.Iterator to
cabc.Iterator[None]. Add a contract test or checksum assertion ensuring both
modules remain identical, then run make test.
In `@template/`{% if en_gb_oxendict %}scripts{% endif %}/generate_typos_config.py:
- Around line 18-22: Annotate the module-level constants in
generate_typos_config.py explicitly: declare DEFAULT_BASE_URL as str and
REPOSITORY_ROOT as Path, preserving their existing values and initialization.
In `@template/AGENTS.md.jinja`:
- Around line 24-27: Update the conditional closing tags associated with the
en_gb_oxendict sections, including the block near the spelling guidance and the
corresponding block later in the template, to use whitespace-trimming endif
syntax so Copier removes the standalone tag newline and does not insert blank
lines into generated lists.
In `@template/docs/developers-guide.md.jinja`:
- Around line 56-63: Change the conditional “Spelling policy” heading in the
template from level 2 to level 3 so it remains nested under “Tooling” and does
not capture “Security audit ignores”; leave the surrounding prose and
conditional behavior unchanged.
In `@tests/test_template/test_spelling_gate.py`:
- Around line 207-227: Add descriptive assertion messages to every bare assert
in the property-test block, including the typos configuration, generator script,
spellcheck target, counterpart path existence, byte equality, and Polonius
Makefile-line comparisons. Update the assertions around the generated Makefile
and selected/counterpart paths so Hypothesis failures identify the violated
contract directly.
- Around line 161-162: Add descriptive assertion messages to both bare asserts
in the relevant spelling-gate test, preserving their existing conditions and
clearly identifying which forbidden string was found in users_guide.
- Around line 90-94: Update the subprocess invocation in the test to address
Ruff S607 on the argument-list line: preferably resolve the uv executable with
shutil.which("uv") and invoke that path; otherwise add a narrowly scoped # noqa:
S607 on the diagnostic line with an explicit justification, while preserving the
existing S603 suppression and trusted-generator behavior.
In `@tests/test_typos_rollout.py`:
- Around line 368-409: Strengthen test_cache_lock_serializes_generator_processes
by having the child emit a readiness marker immediately before generator.main
attempts lock acquisition, then wait for and assert that marker before releasing
the parent-held lock. Retain the existing post-release success assertions, but
remove reliance on process.wait(timeout=0.2) as proof of lock contention.
- Around line 183-409: Update the new tests in the shown test functions to add
descriptive failure messages to every bare assert, including cache-file checks,
response/request validation, metadata cleanup, retry results, and
subprocess/cache assertions. Preserve each assertion’s existing condition and
follow the file’s established `assert expression, "message"` style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c805b8e7-eda2-49e3-a3b9-431d3ff3429c
📒 Files selected for processing (27)
.gitignorecopier.yamldocs/adr-003-shared-oxford-spelling-base.mddocs/developers-guide.mddocs/users-guide.mdscripts/generate_typos_config.pyscripts/typos_rollout.pytemplate/.github/workflows/ci.yml.jinjatemplate/.gitignoretemplate/.gitignore.jinjatemplate/AGENTS.md.jinjatemplate/Makefile.jinjatemplate/docs/developers-guide.md.jinjatemplate/docs/documentation-style-guide.mdtemplate/docs/repository-layout.md.jinjatemplate/docs/users-guide.md.jinjatemplate/scripts/generate_typos_config.pytemplate/{% if en_gb_oxendict %}scripts{% endif %}/generate_typos_config.pytemplate/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.pytemplate/{% if en_gb_oxendict %}typos.local.toml{% endif %}.jinjatemplate/{% if en_gb_oxendict %}typos.toml{% endif %}.jinjatests/helpers/rendering.pytests/helpers/tooling_contracts/makefile.pytests/helpers/tooling_contracts/workflows.pytests/test_template/__snapshots__/test_snapshots.ambrtests/test_template/test_spelling_gate.pytests/test_typos_rollout.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/whitaker(auto-detected)
💤 Files with no reviewable changes (2)
- template/.gitignore
- template/scripts/generate_typos_config.py
| """Refresh shared policy and write the merged generated configuration. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| output : Path | None | ||
| Generated configuration destination. Defaults to ``typos.toml`` in | ||
| ``repository``. | ||
| repository : Path | ||
| Repository that owns the cache, metadata, overlay, and output. | ||
| source : str | Path | ||
| Authoritative local dictionary path or HTTPS URL. | ||
| offline : bool | ||
| Reuse a valid cache without consulting ``source`` when true. | ||
|
|
||
| Returns | ||
| ------- | ||
| rollout.RefreshResult | ||
| Refresh status and the validated cache used to generate the output. | ||
|
|
||
| Raises | ||
| ------ | ||
| FileNotFoundError | ||
| Offline mode has no valid cache or a local source is absent. | ||
| OSError | ||
| Refresh, locking, or output filesystem operations fail. | ||
| TypeError | ||
| A dictionary value has the wrong TOML shape. | ||
| ValueError | ||
| A source, dictionary, merge, or generated mapping is invalid. | ||
| tomllib.TOMLDecodeError | ||
| Input or generated output is not valid TOML. | ||
| urllib.error.HTTPError | ||
| A remote refresh fails and no valid cache is available. | ||
| urllib.error.URLError | ||
| A network refresh fails and no valid cache is available. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document rollout.DictionaryTooLargeError.
The contract tests expect main to raise rollout.DictionaryTooLargeError for oversized local and HTTP dictionaries, but the Raises section lists only ValueError for validation failures. Name the custom exception explicitly so callers can identify the fixed input-limit failure.
Based on learnings, the refresh must bound dictionary inputs and validate them before atomic replacement.
Proposed docstring update
ValueError
A source, dictionary, merge, or generated mapping is invalid.
+ rollout.DictionaryTooLargeError
+ A local or remote dictionary exceeds the fixed input limit.
tomllib.TOMLDecodeError🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/generate_typos_config.py` around lines 94 - 129, Update the Raises
section of the refresh function’s docstring to explicitly document
rollout.DictionaryTooLargeError for local or remote dictionaries exceeding the
fixed input limit, while preserving the existing ValueError description for
other validation failures.
Source: Learnings
| def _refresh_http( | ||
| source: str, | ||
| cache: pathlib.Path, | ||
| metadata: pathlib.Path, | ||
| ) -> RefreshResult: | ||
| """Refresh a cache from a validated HTTPS source with stale fallback.""" | ||
| saved = _read_metadata(metadata) | ||
| same_source = saved.get("source") == source | ||
| if not same_source: | ||
| saved = {} | ||
| request = _https_request(source, _conditional_headers(saved)) | ||
| try: | ||
| with urllib.request.urlopen( # noqa: S310 - _https_request rejects non-HTTPS URLs. | ||
| request, | ||
| timeout=30.0, | ||
| ) as response: | ||
| if response.status == HTTP_NOT_MODIFIED and _valid_cache(cache): | ||
| with _open_https(request) as response: | ||
| if ( | ||
| response.status == HTTP_NOT_MODIFIED | ||
| and same_source | ||
| and _valid_cache(cache) | ||
| ): | ||
| return RefreshResult("current", cache) | ||
| if _valid_cache(cache) and _remote_is_not_newer(saved, response.headers): | ||
| return RefreshResult("current", cache) | ||
| content = _read_bounded_stream( | ||
| response, | ||
| declared_size=_content_length(response.headers), | ||
| ) | ||
| return _write_remote_cache( | ||
| source, | ||
| _CacheTargets(cache, metadata), | ||
| response.read(), | ||
| content, | ||
| response.headers, | ||
| ) | ||
| except urllib.error.HTTPError as error: | ||
| if error.code == HTTP_NOT_MODIFIED and _valid_cache(cache): | ||
| if error.code == HTTP_NOT_MODIFIED and same_source and _valid_cache(cache): | ||
| return RefreshResult("current", cache) | ||
| if _valid_cache(cache): | ||
| return RefreshResult("stale-cache", cache) | ||
| return _stale_cache_result(cache, category="http", status=error.code) | ||
| raise | ||
| except urllib.error.URLError: | ||
| if _valid_cache(cache): | ||
| return _stale_cache_result(cache, category="network") | ||
| raise | ||
| except (OSError, urllib.error.URLError): | ||
| except OSError: | ||
| if _valid_cache(cache): | ||
| return RefreshResult("stale-cache", cache) | ||
| return _stale_cache_result(cache, category="os") | ||
| raise |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Apply both rollout-module fixes to the parent copy and the template copy. scripts/typos_rollout.py and the template module are identical duplicates, so each correction must land twice or the two copies drift apart. Two corrections apply: extract the repeated stale-cache fallback from _refresh_http to clear Ruff PLR0911, and replace the deprecated typ.Iterator alias with cabc.Iterator.
scripts/typos_rollout.py#L627-L671: extract a_fallback_or_raisehelper and call it from the threeexceptbodies.scripts/typos_rollout.py#L166-L176: change the_cache_lockreturn type tocabc.Iterator[None].template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L627-L671: apply the identical_fallback_or_raiseextraction.template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L166-L176: change the_cache_lockreturn type tocabc.Iterator[None].
Add a contract test or a checksum assertion that proves the two modules stay identical. Run make test after the change, as the guidelines require for files under template/.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 627-627: Too many return statements (7 > 6)
(PLR0911)
📍 Affects 2 files
scripts/typos_rollout.py#L627-L671(this comment)scripts/typos_rollout.py#L166-L176template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L627-L671template/{% if en_gb_oxendict %}scripts{% endif %}/typos_rollout.py#L166-L176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/typos_rollout.py` around lines 627 - 671, Update both
scripts/typos_rollout.py (lines 627-671 and 166-176) and template/{% if
en_gb_oxendict %}scripts{% endif %}/typos_rollout.py (lines 627-671 and
166-176): extract the repeated stale-cache handling from _refresh_http into
_fallback_or_raise and call it from all three exception handlers, and change
_cache_lock’s return annotation from typ.Iterator to cabc.Iterator[None]. Add a
contract test or checksum assertion ensuring both modules remain identical, then
run make test.
Sources: Coding guidelines, Linters/SAST tools
| DEFAULT_BASE_URL = ( | ||
| "https://raw.githubusercontent.com/leynos/agent-helper-scripts/" | ||
| "refs/heads/main/data/typos-oxendict-base.toml" | ||
| ) | ||
| REPOSITORY_ROOT = Path(__file__).resolve().parent.parent |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add explicit annotations to module constants.
Add : str to DEFAULT_BASE_URL and : Path to REPOSITORY_ROOT. Keep module-level values explicitly typed.
As per path instructions, “All code must have clear type hints using modern style”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@template/`{% if en_gb_oxendict %}scripts{% endif %}/generate_typos_config.py
around lines 18 - 22, Annotate the module-level constants in
generate_typos_config.py explicitly: declare DEFAULT_BASE_URL as str and
REPOSITORY_ROOT as Path, preserving their existing values and initialization.
Source: Path instructions
| {% if en_gb_oxendict %}- **Use consistent spelling and grammar.** Comments must use en-GB-oxendict | ||
| ("-ize" / "-yse" / "-our") spelling and grammar, with the exception of | ||
| references to external APIs. | ||
| {% endif %} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect Copier Jinja whitespace settings and the AGENTS.md conditional blocks.
fd -t f 'copier.yaml' -x rg -n '_envops|trim_blocks|lstrip_blocks|keep_trailing_newline' {}
fd -t f 'AGENTS.md.jinja' -x sed -n '20,30p;333,345p' {}
rg -n 'AGENTS.md' tests -C3Repository: leynos/agent-template-rust
Length of output: 3869
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- template configuration files ---'
fd -t f -a | rg '(^|/)(copier|pyproject|Makefile|AGENTS\.md\.jinja|test_.*\.py)$' | head -200
printf '%s\n' '--- Jinja environment settings ---'
rg -n -S 'envops|trim_blocks|lstrip_blocks|keep_trailing_newline|copier' . --glob '!**/.git/**' --glob '!**/target/**' | head -240
printf '%s\n' '--- template block context ---'
sed -n '1,60p' template/AGENTS.md.jinja
sed -n '320,350p' template/AGENTS.md.jinja
printf '%s\n' '--- relevant test implementations ---'
sed -n '1,230p' tests/test_template/test_spelling_gate.py
sed -n '1,230p' tests/test_template/test_tooling_contracts.py
printf '%s\n' '--- test and render commands ---'
rg -n 'copier|render|make test|markdownlint|AGENTS' Makefile pyproject.toml tests --glob '*.py' --glob 'Makefile' --glob 'pyproject.toml' | head -260Repository: leynos/agent-template-rust
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
for path in [Path("copier.yaml"), Path("copier.yml"), Path(".copier-answers.yml"), Path("pyproject.toml")]:
if path.exists():
print(f"--- {path} ---")
print(path.read_text())
PY
printf '%s\n' '--- exact whitespace around conditional tags ---'
python3 - <<'PY'
from pathlib import Path
p = Path("template/AGENTS.md.jinja")
s = p.read_text()
for needle in ["{% if en_gb_oxendict %}", "{% endif %}"]:
print(f"{needle}: {[i+1 for i,l in enumerate(s.splitlines()) if needle in l]}")
for start, end in [(20, 30), (333, 345)]:
print(f"--- lines {start}-{end} ---")
for n, line in enumerate(s.splitlines(), 1):
if start <= n <= end:
print(f"{n:4}: {line!r}")
PY
printf '%s\n' '--- repository status and test targets ---'
git status --short
rg -n '^(test|markdownlint|fmt|render)|copier run|copier copy|pytest' Makefile justfile pyproject.toml tox.ini noxfile.py 2>/dev/null || trueRepository: leynos/agent-template-rust
Length of output: 5806
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import importlib.util
print("jinja2_available", importlib.util.find_spec("jinja2") is not None)
print("copier_available", importlib.util.find_spec("copier") is not None)
PY
printf '%s\n' '--- focused render-related test helpers ---'
sed -n '1,110p' tests/helpers/rendering.py
sed -n '1,90p' tests/test_template/test_basic_rendering.py
printf '%s\n' '--- full Copier settings and dependencies ---'
sed -n '1,35p' requirements.txt
sed -n '1,45p' Makefile
printf '%s\n' '--- template whitespace control markers ---'
rg -n '\{%-|-%\}|\{%\s*(if|endif)' template/AGENTS.md.jinjaRepository: leynos/agent-template-rust
Length of output: 8493
🌐 Web query:
Copier documentation default _envops trim_blocks lstrip_blocks keep_trailing_newline
💡 Result:
In Copier, _envops is a configuration key used to pass options directly to the underlying Jinja2 environment [1][2]. While Copier provides specific defaults, it allows you to override these or pass additional supported Jinja2 settings by including them under _envops in your copier.yml file [1][3]. Regarding the specific settings you mentioned: 1. keep_trailing_newline: This is the only Jinja2 environment option that Copier explicitly overrides by default. Copier sets keep_trailing_newline to true [1][2]. If you prefer the standard Jinja2 behavior (where trailing newlines are stripped), you must explicitly set keep_trailing_newline to false in your _envops configuration [1][2]. 2. trim_blocks and lstrip_blocks: These are standard Jinja2 environment options. They are not explicitly configured by Copier by default, meaning they inherit the default values provided by Jinja2 itself (typically false unless otherwise specified by the user). You can enable them by adding them to your _envops block in copier.yml, for example: _envops: trim_blocks: true lstrip_blocks: true Copier does not maintain its own custom implementation for these specific flags; it passes them directly to the Jinja2 Environment constructor [4]. You can refer to the official Jinja2 API documentation for the full list of available options that can be controlled via _envops [1][2].
Citations:
- 1: https://copier.readthedocs.io/en/stable/configuring/
- 2: https://copier.readthedocs.io/en/v9.3.0/configuring/
- 3: https://github.com/copier-org/copier/blob/master/tests/test_normal_jinja2.py
- 4: https://copier.readthedocs.io/en/v5.1.0/api/reference/tools/
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- snapshot and Markdown lint configuration ---'
fd -a -t f | rg '(__snapshots__|snapshot|markdownlint|\.markdownlint|AGENTS)' | head -160
rg -n -S 'MD032|markdownlint|AGENTS' . --glob '!**/.git/**' --glob '!**/target/**' | head -220
printf '%s\n' '--- standalone newline-behaviour verifier ---'
python3 - <<'PY'
from pathlib import Path
source = Path("template/AGENTS.md.jinja").read_text()
lines = source.splitlines(keepends=True)
def render(flag: bool, trim_blocks: bool) -> str:
out = []
active = True
for line in lines:
if "{% if en_gb_oxendict %}" in line:
active = flag
line = line.replace("{% if en_gb_oxendict %}", "")
if not active:
line = ""
if "{% endif %}" in line:
line = line.replace("{% endif %}", "")
active = True
if active and line:
out.append(line)
elif active and not line and trim_blocks:
# Jinja's trim_blocks removes the newline on a block-tag-only line.
pass
elif not active and line.endswith("\n") and not trim_blocks:
# A standalone block tag still contributes its source newline.
out.append("\n")
text = "".join(out)
return text
for trim in (False, True):
print(f"trim_blocks={trim}")
for flag in (False, True):
rendered = render(flag, trim)
print(f" en_gb_oxendict={flag}")
for n, line in enumerate(rendered.splitlines(), 1):
if line == "" and 1 < n < len(rendered.splitlines()):
print(f" blank line {n}; previous={rendered.splitlines()[n-2]!r}; next={rendered.splitlines()[n]!r}")
PYRepository: leynos/agent-template-rust
Length of output: 25400
Use {% endif -%} at lines 27 and 341. With the default en_gb_oxendict value, Copier preserves the standalone tag newlines and adds blank lines to the generated lists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@template/AGENTS.md.jinja` around lines 24 - 27, Update the conditional
closing tags associated with the en_gb_oxendict sections, including the block
near the spelling guidance and the corresponding block later in the template, to
use whitespace-trimming endif syntax so Copier removes the standalone tag
newline and does not insert blank lines into generated lists.
| subprocess.run( # noqa: S603 - runs the rendered, trusted generator. | ||
| ["uv", "run", "scripts/generate_typos_config.py"], | ||
| check=True, | ||
| cwd=project.path, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Suppress the S607 finding on the "uv" invocation, not just S603.
Ruff reports S607 ("start-process-with-partial-path") on the argument-list line because "uv" is resolved through PATH. The # noqa: S603 comment on line 90 does not cover this, because the diagnostic is attached to line 91. Add a targeted, justified suppression for S607, or resolve the full path with shutil.which("uv").
As per path instructions, "Only narrow in-line disables (# noqa: XYZ) are permitted, must be accompanied by justification and used only as a last resort."
🔧 Proposed fix
subprocess.run( # noqa: S603 - runs the rendered, trusted generator.
- ["uv", "run", "scripts/generate_typos_config.py"],
+ ["uv", "run", "scripts/generate_typos_config.py"], # noqa: S607 - "uv" is resolved via PATH by design.
check=True,
cwd=project.path,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| subprocess.run( # noqa: S603 - runs the rendered, trusted generator. | |
| ["uv", "run", "scripts/generate_typos_config.py"], | |
| check=True, | |
| cwd=project.path, | |
| ) | |
| subprocess.run( # noqa: S603 - runs the rendered, trusted generator. | |
| ["uv", "run", "scripts/generate_typos_config.py"], # noqa: S607 - "uv" is resolved via PATH by design. | |
| check=True, | |
| cwd=project.path, | |
| ) |
🧰 Tools
🪛 Ruff (0.16.0)
[error] 91-91: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_template/test_spelling_gate.py` around lines 90 - 94, Update the
subprocess invocation in the test to address Ruff S607 on the argument-list
line: preferably resolve the uv executable with shutil.which("uv") and invoke
that path; otherwise add a narrowly scoped # noqa: S607 on the diagnostic line
with an explicit justification, while preserving the existing S603 suppression
and trusted-generator behavior.
Sources: Path instructions, Linters/SAST tools
| assert "spellcheck" not in users_guide | ||
| assert "en-GB-oxendict" not in users_guide |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add assertion messages.
Lines 161-162 use bare assert statements. Every other assertion in this file carries a descriptive message. Add one here too, for consistent failure diagnostics.
As per path instructions, "Use assert …, "message" over bare asserts."
♻️ Proposed fix
- assert "spellcheck" not in users_guide
- assert "en-GB-oxendict" not in users_guide
+ assert "spellcheck" not in users_guide, (
+ "expected disabled render users guide to omit the spellcheck mention"
+ )
+ assert "en-GB-oxendict" not in users_guide, (
+ "expected disabled render users guide to omit the en-GB-oxendict mention"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert "spellcheck" not in users_guide | |
| assert "en-GB-oxendict" not in users_guide | |
| assert "spellcheck" not in users_guide, ( | |
| "expected disabled render users guide to omit the spellcheck mention" | |
| ) | |
| assert "en-GB-oxendict" not in users_guide, ( | |
| "expected disabled render users guide to omit the en-GB-oxendict mention" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_template/test_spelling_gate.py` around lines 161 - 162, Add
descriptive assertion messages to both bare asserts in the relevant
spelling-gate test, preserving their existing conditions and clearly identifying
which forbidden string was found in users_guide.
Source: Path instructions
| assert (project / "typos.toml").exists() is en_gb_oxendict | ||
| assert (project / "scripts" / "generate_typos_config.py").exists() is ( | ||
| en_gb_oxendict | ||
| ) | ||
| makefile = read_generated_text(project / "Makefile") | ||
| assert ("spellcheck" in makefile) is en_gb_oxendict | ||
|
|
||
| selected_path = project / polonius_path | ||
| counterpart_path = counterpart / polonius_path | ||
| assert selected_path.exists() is counterpart_path.exists() | ||
| if selected_path.exists(): | ||
| assert selected_path.read_bytes() == counterpart_path.read_bytes() | ||
| selected_polonius_lines = [ | ||
| line for line in makefile.splitlines() if "POLONIUS" in line | ||
| ] | ||
| counterpart_polonius_lines = [ | ||
| line | ||
| for line in read_generated_text(counterpart / "Makefile").splitlines() | ||
| if "POLONIUS" in line | ||
| ] | ||
| assert selected_polonius_lines == counterpart_polonius_lines |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add assertion messages to the property test.
Lines 207, 208-210, 212, 216, 218, and 227 use bare assert statements. Add descriptive messages so a Hypothesis-shrunk failure identifies which contract broke without inspecting the generated example by hand.
As per path instructions, "Use assert …, "message" over bare asserts."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_template/test_spelling_gate.py` around lines 207 - 227, Add
descriptive assertion messages to every bare assert in the property-test block,
including the typos configuration, generator script, spellcheck target,
counterpart path existence, byte equality, and Polonius Makefile-line
comparisons. Update the assertions around the generated Makefile and
selected/counterpart paths so Hypothesis failures identify the violated contract
directly.
Source: Path instructions
| def test_oversized_local_dictionary_is_rejected_without_cache_mutation( | ||
| generator: types.ModuleType, | ||
| tmp_path: pathlib.Path, | ||
| ) -> None: | ||
| """Local sources over the fixed input limit never reach the cache.""" | ||
| repository, source = prepare_repository(tmp_path) | ||
| source.write_bytes(b"x" * (generator.rollout.MAX_DICTIONARY_BYTES + 1)) | ||
|
|
||
| with pytest.raises( | ||
| generator.rollout.DictionaryTooLargeError, | ||
| match="dictionary declares", | ||
| ): | ||
| generator.main(repository=repository, source=source) | ||
|
|
||
| assert not (repository / ".typos-oxendict-base.toml").exists() | ||
| assert not (repository / ".typos-oxendict-base.json").exists() | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("headers", "content", "message"), | ||
| [ | ||
| ({"Content-Length": "1048577"}, b"", "dictionary declares"), | ||
| ({}, b"x" * 1_048_577, "dictionary exceeds"), | ||
| ], | ||
| ids=["declared-size", "streamed-size"], | ||
| ) | ||
| def test_oversized_http_dictionary_is_rejected_without_cache_mutation( | ||
| generator: types.ModuleType, | ||
| tmp_path: pathlib.Path, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| headers: dict[str, str], | ||
| content: bytes, | ||
| message: str, | ||
| ) -> None: | ||
| """HTTP sources are bounded with and without Content-Length.""" | ||
| repository, _ = prepare_repository(tmp_path) | ||
| response = FakeHttpResponse(content, headers) | ||
| monkeypatch.setattr( | ||
| generator.rollout, | ||
| "_open_https", | ||
| lambda *args, **kwargs: response, | ||
| ) | ||
|
|
||
| with pytest.raises(generator.rollout.DictionaryTooLargeError, match=message): | ||
| generator.main(repository=repository, source="https://example.invalid/base") | ||
|
|
||
| assert not (repository / ".typos-oxendict-base.toml").exists() | ||
| assert not (repository / ".typos-oxendict-base.json").exists() | ||
|
|
||
|
|
||
| def test_http_failure_reports_bounded_stale_cache_diagnostic( | ||
| generator: types.ModuleType, | ||
| tmp_path: pathlib.Path, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| caplog: pytest.LogCaptureFixture, | ||
| ) -> None: | ||
| """A degraded HTTP refresh identifies status and cache age without its URL.""" | ||
| repository, source = prepare_repository(tmp_path) | ||
| generator.main(repository=repository, source=source) | ||
| monkeypatch.setattr( | ||
| generator.rollout, | ||
| "_open_https", | ||
| lambda *args, **kwargs: (_ for _ in ()).throw( | ||
| urllib.error.HTTPError( | ||
| "https://secret.invalid/dictionary", | ||
| 503, | ||
| "unavailable", | ||
| email.message.Message(), | ||
| None, | ||
| ) | ||
| ), | ||
| ) | ||
|
|
||
| with caplog.at_level("WARNING", logger=generator.rollout.__name__): | ||
| result = generator.main( | ||
| repository=repository, | ||
| source="https://example.invalid/base", | ||
| ) | ||
|
|
||
| assert result.status == "stale-cache" | ||
| record = caplog.records[-1] | ||
| assert record.levelname == "WARNING" | ||
| assert "operation=https-refresh" in record.message | ||
| assert "category=http" in record.message | ||
| assert "status=503" in record.message | ||
| assert "cache_age_seconds=" in record.message | ||
| assert "secret.invalid" not in record.message | ||
| assert "example.invalid" not in record.message | ||
|
|
||
|
|
||
| def test_changed_http_source_discards_matching_validators( | ||
| generator: types.ModuleType, | ||
| tmp_path: pathlib.Path, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| """A new HTTP authority is fetched even when its validators match.""" | ||
| repository, _ = prepare_repository(tmp_path) | ||
| headers = { | ||
| "ETag": '"shared-validator"', | ||
| "Last-Modified": "Wed, 21 Oct 2015 07:28:00 GMT", | ||
| } | ||
| responses = iter( | ||
| [ | ||
| FakeHttpResponse(dictionary_text(stem="first").encode(), headers), | ||
| FakeHttpResponse(dictionary_text(stem="second").encode(), headers), | ||
| ] | ||
| ) | ||
| requests: list[urllib.request.Request] = [] | ||
|
|
||
| def open_https(request: urllib.request.Request) -> FakeHttpResponse: | ||
| requests.append(request) | ||
| return next(responses) | ||
|
|
||
| monkeypatch.setattr(generator.rollout, "_open_https", open_https) | ||
|
|
||
| first_url = "https://first.example.invalid/dictionary.toml" | ||
| second_url = "https://second.example.invalid/dictionary.toml" | ||
| generator.main(repository=repository, source=first_url) | ||
| result = generator.main(repository=repository, source=second_url) | ||
|
|
||
| second_request = requests[1] | ||
| assert second_request.full_url == second_url | ||
| assert second_request.get_header("If-none-match") is None | ||
| assert second_request.get_header("If-modified-since") is None | ||
| assert result.status == "refreshed" | ||
| assert '"secondise" = "secondize"' in generator.render_config(repository) | ||
|
|
||
|
|
||
| def test_http_redirect_cannot_downgrade_to_plain_http( | ||
| generator: types.ModuleType, | ||
| ) -> None: | ||
| """Every redirect target must retain HTTPS transport.""" | ||
| handler = generator.rollout._HttpsRedirectHandler() | ||
| request = generator.rollout._https_request( | ||
| "https://example.invalid/dictionary.toml", | ||
| {}, | ||
| ) | ||
|
|
||
| with pytest.raises(ValueError, match="must use HTTPS"): | ||
| handler.redirect_request( | ||
| request, | ||
| io.BytesIO(), | ||
| 302, | ||
| "Found", | ||
| email.message.Message(), | ||
| "http://example.invalid/dictionary.toml", | ||
| ) | ||
|
|
||
|
|
||
| def test_metadata_failure_cleans_up_and_retries_changed_source( | ||
| generator: types.ModuleType, | ||
| tmp_path: pathlib.Path, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| """An interrupted metadata write leaves valid cache state for a clean retry.""" | ||
| repository, source = prepare_repository(tmp_path) | ||
| cache = repository / ".typos-oxendict-base.toml" | ||
| metadata = repository / ".typos-oxendict-base.json" | ||
| original_write_metadata = generator.rollout._write_metadata | ||
|
|
||
| def fail_metadata(*args: object, **kwargs: object) -> None: | ||
| raise OSError("simulated metadata failure") | ||
|
|
||
| monkeypatch.setattr(generator.rollout, "_write_metadata", fail_metadata) | ||
| with pytest.raises(OSError, match="simulated metadata failure"): | ||
| generator.main(repository=repository, source=source) | ||
|
|
||
| assert generator.rollout.load_dictionary(cache) | ||
| assert not metadata.exists() | ||
| temporary_files = [ | ||
| path | ||
| for path in repository.glob(f".{cache.name}.*") | ||
| if path.name != f"{cache.name}.lock" | ||
| ] | ||
| assert temporary_files == [] | ||
|
|
||
| monkeypatch.setattr(generator.rollout, "_write_metadata", original_write_metadata) | ||
| source.write_text(dictionary_text(stem="retried"), encoding="utf-8") | ||
| result = generator.main(repository=repository, source=source) | ||
|
|
||
| assert result.status == "refreshed" | ||
| assert metadata.exists() | ||
| assert '"retriedise" = "retriedize"' in generator.render_config(repository) | ||
|
|
||
|
|
||
| def test_cache_lock_serializes_generator_processes( | ||
| generator: types.ModuleType, | ||
| tmp_path: pathlib.Path, | ||
| ) -> None: | ||
| """A second process waits for the cache owner and then refreshes cleanly.""" | ||
| repository, source = prepare_repository(tmp_path) | ||
| cache = repository / ".typos-oxendict-base.toml" | ||
| lock_path = cache.with_name(f"{cache.name}.lock") | ||
| lock_path.touch() | ||
| child_code = ( | ||
| "import pathlib, sys; " | ||
| "sys.path.insert(0, sys.argv[1]); " | ||
| "import generate_typos_config as generator; " | ||
| "generator.main(repository=pathlib.Path(sys.argv[2]), " | ||
| "source=pathlib.Path(sys.argv[3]))" | ||
| ) | ||
|
|
||
| with lock_path.open("a+b") as lock: | ||
| fcntl.flock(lock.fileno(), fcntl.LOCK_EX) | ||
| process = subprocess.Popen( # noqa: S603 - argv contains trusted test paths. | ||
| [ | ||
| sys.executable, | ||
| "-c", | ||
| child_code, | ||
| str(SCRIPTS_ROOT), | ||
| str(repository), | ||
| str(source), | ||
| ], | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| text=True, | ||
| ) | ||
| with pytest.raises(subprocess.TimeoutExpired): | ||
| process.wait(timeout=0.2) | ||
| fcntl.flock(lock.fileno(), fcntl.LOCK_UN) | ||
|
|
||
| stdout, stderr = process.communicate(timeout=10) | ||
| assert process.returncode == 0, f"stdout={stdout}\nstderr={stderr}" | ||
| assert generator.rollout.load_dictionary(cache) | ||
| assert (repository / ".typos-oxendict-base.json").exists() | ||
|
|
||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add assertion messages throughout the new tests.
Several new assertions are bare, for example lines 197-198, 229-230, 350-351, 357, 363-365, and 406-407. The rest of this file, and most of the newer stale-cache and validator tests (lines 233-309), consistently carry descriptive messages. Bring the remaining bare asserts in line for consistent failure diagnostics.
As per path instructions, "Use assert …, "message" over bare asserts."
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 327-327: Do not make http calls without encryption
Context: "http://example.invalid/dictionary.toml"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[error] 386-398: Command coming from incoming request
Context: subprocess.Popen( # noqa: S603 - argv contains trusted test paths.
[
sys.executable,
"-c",
child_code,
str(SCRIPTS_ROOT),
str(repository),
str(source),
],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.0)
[warning] 223-223: Unused lambda argument: args
(ARG005)
[warning] 223-223: Unused lambda argument: kwargs
(ARG005)
[warning] 245-245: Unused lambda argument: args
(ARG005)
[warning] 245-245: Unused lambda argument: kwargs
(ARG005)
[warning] 343-343: Unused function argument: args
(ARG001)
[warning] 343-343: Unused function argument: kwargs
(ARG001)
[warning] 344-344: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_typos_rollout.py` around lines 183 - 409, Update the new tests in
the shown test functions to add descriptive failure messages to every bare
assert, including cache-file checks, response/request validation, metadata
cleanup, retry results, and subprocess/cache assertions. Preserve each
assertion’s existing condition and follow the file’s established `assert
expression, "message"` style.
Source: Path instructions
| def test_cache_lock_serializes_generator_processes( | ||
| generator: types.ModuleType, | ||
| tmp_path: pathlib.Path, | ||
| ) -> None: | ||
| """A second process waits for the cache owner and then refreshes cleanly.""" | ||
| repository, source = prepare_repository(tmp_path) | ||
| cache = repository / ".typos-oxendict-base.toml" | ||
| lock_path = cache.with_name(f"{cache.name}.lock") | ||
| lock_path.touch() | ||
| child_code = ( | ||
| "import pathlib, sys; " | ||
| "sys.path.insert(0, sys.argv[1]); " | ||
| "import generate_typos_config as generator; " | ||
| "generator.main(repository=pathlib.Path(sys.argv[2]), " | ||
| "source=pathlib.Path(sys.argv[3]))" | ||
| ) | ||
|
|
||
| with lock_path.open("a+b") as lock: | ||
| fcntl.flock(lock.fileno(), fcntl.LOCK_EX) | ||
| process = subprocess.Popen( # noqa: S603 - argv contains trusted test paths. | ||
| [ | ||
| sys.executable, | ||
| "-c", | ||
| child_code, | ||
| str(SCRIPTS_ROOT), | ||
| str(repository), | ||
| str(source), | ||
| ], | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| text=True, | ||
| ) | ||
| with pytest.raises(subprocess.TimeoutExpired): | ||
| process.wait(timeout=0.2) | ||
| fcntl.flock(lock.fileno(), fcntl.LOCK_UN) | ||
|
|
||
| stdout, stderr = process.communicate(timeout=10) | ||
| assert process.returncode == 0, f"stdout={stdout}\nstderr={stderr}" | ||
| assert generator.rollout.load_dictionary(cache) | ||
| assert (repository / ".typos-oxendict-base.json").exists() | ||
|
|
||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Strengthen the lock-contention test against false confidence.
process.wait(timeout=0.2) proves the child has not finished while the parent holds the lock, but it does not prove the child was actually blocked on the lock. If the wrapped refresh operation happens to take longer than 0.2 s on its own (Python startup, module import, TOML parsing), the test passes even with a broken or missing lock. Have the child process emit a readiness marker (for example, a line on stdout) right before it attempts to acquire the lock, and assert that marker was observed before releasing the parent's lock. This turns the timing assumption into a deterministic synchronization point.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 386-398: Command coming from incoming request
Context: subprocess.Popen( # noqa: S603 - argv contains trusted test paths.
[
sys.executable,
"-c",
child_code,
str(SCRIPTS_ROOT),
str(repository),
str(source),
],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_typos_rollout.py` around lines 368 - 409, Strengthen
test_cache_lock_serializes_generator_processes by having the child emit a
readiness marker immediately before generator.main attempts lock acquisition,
then wait for and assert that marker before releasing the parent-held lock.
Retain the existing post-release success assertions, but remove reliance on
process.wait(timeout=0.2) as proof of lock contention.
Generated repositories now enforce en-GB Oxford ("-ize") spelling in
Markdown prose with typos, using the two-layer mechanism stilyagi
introduced in ee77d08a19bc9c1107f30fc2b5bb63ced979fbc4: a generated
typos.toml restores Oxford -ize forms over the en-gb locale, and
scripts/generate_typos_config.py is the single source of truth for its
stems, accepted words, and ignore patterns.
The gate is a new boolean copier option, en_gb_oxendict, defaulting to
true. When enabled the template renders the generator script and the
committed typos.toml, pins the typos version through a Makefile
TYPOS_VERSION variable, and adds a spellcheck target that markdownlint
depends on. Because the generated repository has no Python test
runner, the generator carries the netsuke parse tests' intent in a
--check mode: it tomllib-parses the rendered document (failing fast on
duplicate extend-words keys), asserts the exact entry count, and diffs
the committed file against the generator output. CI runs the gate as a
dedicated step after uv is installed, which required converting ci.yml
to a raw-wrapped Jinja template following the release workflow's
escaping convention. Disabled renders carry no trace of the gate.
Running the gate over rendered output surfaced five genuine spelling
issues in the template documentation (Flavored, signaling, afterward,
parameterisation, and an American citation title), which are corrected
or, for the external citation title, ignored by pattern.
Parent contract tests cover both toggle states, and the rendered
Makefile and CI snapshots are updated accordingly.
Trim Copier control-block whitespace without stripping Make recipe tabs. Keep default and disabled renders syntactically valid while avoiding blank lines that make generated snapshots fail Git whitespace checks.
Exercise the Copier default and prove that spelling and Polonius options remain independent across generated project states. Bound dictionary inputs, serialize cache refreshes across processes, and report actionable stale-cache diagnostics. Document the optional generated gate and its distinction from the parent repository spelling target.
Reject transport-downgrading redirects and scope HTTP validators to their recorded source so an old authority cannot be reported as current. Document the generator's public contracts and prove regeneration, source changes, and redirect handling in the parent template tests.
0342b3e to
b70c296
Compare
2 participants
Summary
Adds an en-GB-oxendict (Oxford "-ize") spelling gate to generated repositories as a new boolean Copier option,
en_gb_oxendict, default enabled.Implementation
The rebased branch builds on the shared-dictionary spelling implementation now present on
main:typos.toml,typos.local.toml, and thescripts/generate_typos_config.py/scripts/typos_rollout.pygenerator pair;typosis pinned at 1.48.0 throughTYPOS_VERSIONand run withuv tool run;make spellcheckinvokes the generator withuv run, so its declared Python version is honoured, then checks Markdown withtypos;make markdownlintdepends onmake spellcheckand CI runs the gate afterSetup uv.Conditional root files retain the
.jinjatemplate suffix, allowing Copier to evaluate their conditional path names correctly.Cache safety
Content-Lengthvalues and stream through a capped reader;Main integration
The latest
mainadds the independentenable_poloniusCopier option. The shared render helper omits either option when its value isNone, so Copier's real defaults are exercised. Property tests hold flavour and Polonius state constant while toggling spelling, proving that spelling traces vary only withen_gb_oxendictand Polonius output is unchanged.Toggle semantics
When
en_gb_oxendictis true (the default), generated repositories gain the spelling configuration, generator scripts, Makefile/CI wiring, and spelling documentation.When false, renders carry no trace of the gate: no spelling files or
scripts/directory, cache-ignore entries, Makefile wiring, CI step, or generated spelling guidance.Validation
mbakeand the structured snapshots are reviewed.make check-fmt,make test,make typecheck,make lint, andmake spellingpass.References