ci: wire Lychee link-checker (workflow + installer + pre-push hook)#2084
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request completes the integration of the Lychee link-checker into the repository's CI and local development toolchain. By providing a reliable, platform-aware installation script and a unified configuration, it ensures consistent link validation across documentation. The changes also address existing link rot and formalize the tool's status in the project's architecture decision records. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Dependency ReviewThe following issues were found:
License Issues.github/workflows/lychee.yml
OpenSSF Scorecard
Scanned Files
|
WalkthroughAdds Lychee link-checking: refactors scripts/install_cli_tools.sh to manage golangci-lint and lychee installs; adds .github/workflows/lychee.yml and lychee.toml (strict HTTP-200 checks, cache, excludes); ignores .lycheecache; updates .pre-commit-config.yaml to run lychee at pre-push and skip it in pre-commit.ci; updates Renovate rules; applies multiple documentation cross-reference updates; and adjusts test lock/poll behavior and git-backend rate-limit detection with a regression test. Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates the lychee Markdown link-checker into the project's development workflow. Key changes include the addition of a lychee.toml configuration file, the implementation of a new pre-push hook in .pre-commit-config.yaml, and a refactor of scripts/install_cli_tools.sh to automate the installation and SHA256 verification of the lychee binary. Documentation across the repository was also updated to fix broken links and reflect the landing of this tool. Review feedback correctly identified a regex mismatch in the Renovate configuration that would prevent version tracking in shell scripts, a potential failure when handling multi-entry GOPATH strings, and a cleanup issue where temporary directories might persist after a script failure.
| "managerFilePatterns": ["/\\.github/.*\\.ya?ml$/", "/scripts/install_.*\\.sh$/"], | ||
| "matchStrings": [ | ||
| "# renovate: datasource=github-releases depName=(?<depName>[^\\s]+)\\n\\s+[A-Z0-9_]+_VERSION:\\s*[\"']?(?<currentValue>[^\"'\\s]+)[\"']?" | ||
| "# renovate: datasource=github-releases depName=(?<depName>[^\\s]+)\\n\\s+[A-Z0-9_]+_VERSION[:=]\\s*[\"']?(?<currentValue>[^\"'\\s]+)[\"']?" |
There was a problem hiding this comment.
The regex for matching tool versions in shell scripts uses \\s+ after the newline. In scripts/install_cli_tools.sh, the LYCHEE_VERSION variable is defined at the start of the line with no leading whitespace. This will cause Renovate to fail to match the dependency in the script. Changing this to \\s* will allow it to match both indented YAML and unindented shell variables.
"# renovate: datasource=github-releases depName=(?<depName>[^\\s]+)\\n\\s*[A-Z0-9_]+_VERSION[:=]\\s*["']?(?<currentValue>[^"'\\s]+)["']?"| local gobin gopath install_dir installed_binary | ||
| gobin=$(go env GOBIN 2>/dev/null || true) | ||
| gopath=$(go env GOPATH 2>/dev/null || true) | ||
| install_dir="${gobin:-${gopath}/bin}" |
There was a problem hiding this comment.
If GOPATH contains multiple entries (a colon-separated list), appending /bin to the raw string will result in an invalid path, causing mkdir -p and subsequent checks to fail. It is safer to extract the first entry from GOPATH as that is where go install places binaries by default.
| install_dir="${gobin:-${gopath}/bin}" | |
| install_dir="${gobin:-$(echo "$gopath" | cut -d: -f1)/bin}" |
|
|
||
| local tmpdir | ||
| tmpdir="$(mktemp -d -t lychee-install.XXXXXX)" | ||
| trap 'rm -rf "${tmpdir}"' RETURN |
There was a problem hiding this comment.
The RETURN trap only executes when the function returns normally. Since the script uses set -e, any command failure inside install_lychee (like a failed curl or sha256sum check) will cause the script to exit immediately without triggering the RETURN trap, leaving the temporary directory behind. Using an EXIT trap is more robust for ensuring cleanup on both success and failure.
| trap 'rm -rf "${tmpdir}"' RETURN | |
| trap 'rm -rf "${tmpdir}"' EXIT |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Aureliolo
force-pushed
the
refactor/wire-lychee-ci
branch
from
6de9802 to
1f11e93
Compare
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2084 +/- ##
==========================================
- Coverage 87.11% 87.11% -0.01%
==========================================
Files 2251 2251
Lines 130269 130272 +3
==========================================
Hits 113484 113484
- Misses 16770 16773 +3
Partials 15 15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/guides/dynamic-scoring.md`:
- Line 130: The cross-reference in docs/guides/dynamic-scoring.md that claims to
point readers to the “broader scoring architecture and the existing strategies”
is incorrect because docs/reference/scoring-hyperparameters.md only covers
tunable weights/thresholds; update the sentence (the line containing “For the
broader scoring architecture and the existing strategies...”) to link to the
correct documentation page that actually describes the architecture and the
composite/weighted/ranked/multi-objective strategies (e.g., replace the link
with docs/reference/scoring-strategies.md or
docs/architecture/scoring-architecture.md), and if no such page exists, create a
short reference page that documents those strategies and link to it from
docs/guides/dynamic-scoring.md.
In `@scripts/install_cli_tools.sh`:
- Around line 280-282: The current check only verifies lychee exists on PATH
(lychee_on_path) but doesn't detect a stale version; update the check to compare
the resolved PATH entry to the installer target and fail/notify if they differ:
use the resolved path from lychee_on_path (or command -v/which) and compare it
against "${install_dir}/lychee" (or the exact installed binary name) and, if not
identical, print an error advising the user to remove the old binary or update
PATH (or prepend "${install_dir}" to PATH) so the pre-push hook will invoke the
newly installed lychee; keep the existing message when lychee is entirely
missing but replace the pass when a different executable is found with a failing
message.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19e53507-4974-4baa-896c-6b8622631453
📒 Files selected for processing (22)
.github/workflows/lychee.yml.gitignore.markdownlint.json.pre-commit-config.yamlCLAUDE.mdREADME.mddocs/api/layer.mddocs/decisions/0006-tiered-module-size-policy.mddocs/design/a2a-protocol.mddocs/getting_started.mddocs/guides/a2a-federation.mddocs/guides/approval-workflow.mddocs/guides/cost-attribution.mddocs/guides/custom-rules-and-meta-loop.mddocs/guides/dynamic-scoring.mddocs/guides/monitoring.mddocs/guides/rest-api-examples.mddocs/reference/comparison.mddocs/research/llm-provider-auth-survey.mdlychee.tomlrenovate.jsonscripts/install_cli_tools.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Deploy Preview
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Site
- GitHub Check: Test Unit (shard 4)
- GitHub Check: Test Unit (shard 2)
- GitHub Check: Test Unit (shard 1)
- GitHub Check: Test Unit (shard 3)
- GitHub Check: Test Integration (shard 1)
- GitHub Check: Test Integration (shard 2)
- GitHub Check: Test Integration (shard 4)
- GitHub Check: Test Integration (shard 3)
- GitHub Check: Test Conformance (SQLite)
- GitHub Check: Test E2E
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Numerics in README and public docs must be sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markers (MANDATORY Doc Numeric Claims)
Files:
docs/design/a2a-protocol.mddocs/guides/a2a-federation.mddocs/guides/cost-attribution.mdREADME.mddocs/guides/dynamic-scoring.mddocs/guides/approval-workflow.mddocs/api/layer.mddocs/guides/custom-rules-and-meta-loop.mddocs/guides/rest-api-examples.mdCLAUDE.mddocs/decisions/0006-tiered-module-size-policy.mddocs/research/llm-provider-auth-survey.mddocs/guides/monitoring.mddocs/getting_started.mddocs/reference/comparison.md
**/*.{d2,mmd,md}
📄 CodeRabbit inference engine (CLAUDE.md)
D2 for architecture/nested containers (theme 200, Dark Mauve, CLI pinned to v0.7.1 in CI), mermaid for flowcharts/sequence/pipelines, Markdown tables for tabular data
Files:
docs/design/a2a-protocol.mddocs/guides/a2a-federation.mddocs/guides/cost-attribution.mdREADME.mddocs/guides/dynamic-scoring.mddocs/guides/approval-workflow.mddocs/api/layer.mddocs/guides/custom-rules-and-meta-loop.mddocs/guides/rest-api-examples.mdCLAUDE.mddocs/decisions/0006-tiered-module-size-policy.mddocs/research/llm-provider-auth-survey.mddocs/guides/monitoring.mddocs/getting_started.mddocs/reference/comparison.md
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:18:32.698Z
Learning: Read `docs/design/` page before implementing; deviations need approval per DESIGN_SPEC.md
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:18:32.698Z
Learning: Present every plan for accept/deny before coding (MANDATORY Planning)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:18:32.698Z
Learning: Use metric units and British English; no region/currency/locale privilege per `docs/reference/regional-defaults.md` (MANDATORY Regional Defaults)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:18:32.698Z
Learning: Never edit `tests/baselines/unit_timing.json`, any `scripts/*_baseline.{txt,json}`, or `scripts/_*_baseline.py`; timeout/slow failures indicate source-code regression. Per-invocation bypass: `ALLOW_BASELINE_GROWTH=1 git commit ...` (MANDATORY Test Regression)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:18:32.698Z
Learning: After issue: branch + commit + push (no auto-PR); use `/pre-pr-review`. Use `/aurelio-review-pr` for external feedback after PR. Fix EVERYTHING valid; no deferring (MANDATORY Post-Implementation + Pre-PR Review)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:18:32.698Z
Learning: Commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced. Signed commits required on protected refs. Branches: `<type>/<slug>` from main. Squash merge. PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) in PR body
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:18:32.698Z
Learning: Pre-commit/pre-push hooks: `.pre-commit-config.yaml`. Tool-call gates: `.claude/settings.json` PreToolUse (`scripts/check_*.sh`/`.py`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:18:32.698Z
Learning: After every squash merge → `/post-merge-cleanup`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:18:32.698Z
Learning: CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:18:32.698Z
Learning: Web: see `web/CLAUDE.md`. CLI: see `cli/CLAUDE.md` (use `go -C cli`, never `cd cli`). Shell: see `~/.claude/rules/common/bash.md` (canonical for `cd` / `git -C` / Bash file-write rules)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:18:32.698Z
Learning: GitHub queries: use `gh issue list` via Bash, NOT MCP `list_issues`
📚 Learning: 2026-05-16T18:36:31.446Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/reference/conventions.md:787-789
Timestamp: 2026-05-16T18:36:31.446Z
Learning: In Aureliolo/synthorg, do not require adding `<!--RS:...-->` “Doc Numeric Claims (MANDATORY)” numeric macros for Python version numbers mentioned in documentation prose (e.g., “Python 3.14”, “Python 3.15”). The `scripts/check_doc_numeric_macros.py` gate only applies to `README.md`, `docs/index.md`, `docs/roadmap/index.md`, `docs/architecture/decisions.md`, and `docs/reference/convention-gates.md`, and it only flags digits adjacent to specific stat nouns (tests/providers/agents/stars/releases), not language version mentions like “Python 3.14”.
Applied to files:
docs/design/a2a-protocol.mddocs/guides/a2a-federation.mddocs/guides/cost-attribution.mdREADME.mddocs/guides/dynamic-scoring.mddocs/guides/approval-workflow.mddocs/api/layer.mddocs/guides/custom-rules-and-meta-loop.mddocs/guides/rest-api-examples.mdCLAUDE.mddocs/decisions/0006-tiered-module-size-policy.mddocs/research/llm-provider-auth-survey.mddocs/guides/monitoring.mddocs/getting_started.mddocs/reference/comparison.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing Markdown in the synthorg repo, account for the CI gate `check_doc_numeric_macros.py`: it skips fenced code blocks entirely, and it only flags digits that are adjacent to these stat nouns: `tests`, `providers`, `agents`, `stars`, `releases`. Therefore, numeric examples such as CLI flag values (e.g., `--num-workers=4` in fenced bash blocks) and prose version numbers (e.g., `3.14`/`3.15`) are not expected to trigger this check; prioritize changes only when digits appear next to one of the listed nouns (e.g., “5 tests”, “10 stars”, etc.).
Applied to files:
docs/design/a2a-protocol.mddocs/guides/a2a-federation.mddocs/guides/cost-attribution.mdREADME.mddocs/guides/dynamic-scoring.mddocs/guides/approval-workflow.mddocs/api/layer.mddocs/guides/custom-rules-and-meta-loop.mddocs/guides/rest-api-examples.mdCLAUDE.mddocs/decisions/0006-tiered-module-size-policy.mddocs/research/llm-provider-auth-survey.mddocs/guides/monitoring.mddocs/getting_started.mddocs/reference/comparison.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing markdown files for the "Doc Numeric Claims (MANDATORY)" RS-marker rule, only require/flag missing RS markers in the files that are actually in-scope for the rule. The scope is enforced via an identical _SCOPED_FILES allowlist in scripts/check_doc_numeric_macros.py and scripts/inject_runtime_stats.py, and currently includes: README.md; docs/index.md; docs/roadmap/index.md; docs/architecture/decisions.md; docs/reference/convention-gates.md. For any other markdown files (e.g., docs/getting_started.md, docs/guides/*), missing RS markers for numeric claims are no-ops and should NOT be flagged.
Applied to files:
docs/design/a2a-protocol.mddocs/guides/a2a-federation.mddocs/guides/cost-attribution.mdREADME.mddocs/guides/dynamic-scoring.mddocs/guides/approval-workflow.mddocs/api/layer.mddocs/guides/custom-rules-and-meta-loop.mddocs/guides/rest-api-examples.mddocs/decisions/0006-tiered-module-size-policy.mddocs/research/llm-provider-auth-survey.mddocs/guides/monitoring.mddocs/getting_started.mddocs/reference/comparison.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing Markdown in the synthorg repo against the `check_doc_numeric_macros.py` gate, account for its documented behavior: it skips fenced code blocks entirely, and it only flags digits that are adjacent to specific stat nouns (`tests`, `providers`, `agents`, `stars`, `releases`). As a result, CLI-style numbers (e.g., `--num-workers=4`) inside fenced bash code blocks should never be treated as violations of this gate; only non-fenced text needs checking, and only around those specific nouns.
Applied to files:
docs/design/a2a-protocol.mddocs/guides/a2a-federation.mddocs/guides/cost-attribution.mddocs/guides/dynamic-scoring.mddocs/guides/approval-workflow.mddocs/api/layer.mddocs/guides/custom-rules-and-meta-loop.mddocs/guides/rest-api-examples.mddocs/decisions/0006-tiered-module-size-policy.mddocs/research/llm-provider-auth-survey.mddocs/guides/monitoring.mddocs/getting_started.mddocs/reference/comparison.md
📚 Learning: 2026-05-16T18:36:19.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/guides/contributing.md:95-95
Timestamp: 2026-05-16T18:36:19.195Z
Learning: In the SynthOrg repo, the “Doc Numeric Claims (MANDATORY)” RS-marker rule should be applied only to these docs: README.md; docs/index.md; docs/roadmap/index.md; docs/architecture/decisions.md; docs/reference/convention-gates.md. This rule is enforced by scripts/check_doc_numeric_macros.py (with runtime substitution by scripts/inject_runtime_stats.py), so reviewers should not flag similar numeric-claim issues in other paths (e.g., anything under docs/guides/). When checking those scoped files, the rule skips fenced code blocks and only flags digits that are adjacent to stat nouns (tests/providers/agents/stars/releases). Numeric CLI flags like “--num-workers=4” inside fenced bash code blocks are not subject to this rule.
Applied to files:
README.md
📚 Learning: 2026-05-16T18:36:31.446Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/reference/conventions.md:787-789
Timestamp: 2026-05-16T18:36:31.446Z
Learning: In Aureliolo/synthorg, follow the `Doc Numeric Claims (MANDATORY)` rule enforced by `scripts/check_doc_numeric_macros.py` only for these markdown files: `README.md`, `docs/index.md`, `docs/roadmap/index.md`, `docs/architecture/decisions.md`, and `docs/reference/convention-gates.md`. The gate flags digits that appear adjacent to the stat nouns `tests`, `providers`, `agents`, `stars`, and `releases`—those numeric claims must use the required `<!--RS:...-->` macro format. Do not apply this rule to prose that mentions Python version numbers (e.g., “Python 3.14” / “Python 3.15”); those should not be flagged as requiring `<!--RS:...-->`.
Applied to files:
README.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: In the synthorg repo, the “Doc Numeric Claims (MANDATORY)” RS-marker rule is enforced only for this exact set of Markdown files: README.md, docs/index.md, docs/roadmap/index.md, docs/architecture/decisions.md, and docs/reference/convention-gates.md. During code reviews, do not raise RS-marker/numeric-claims findings for numeric values in any other files (e.g., docs/getting_started.md, docs/guides/*, docs/reference/conventions.md), since they are not checked or injected by scripts/check_doc_numeric_macros.py or scripts/inject_runtime_stats.py.
Applied to files:
README.md
🪛 LanguageTool
docs/getting_started.md
[uncategorized] ~45-~45: The official name of this software platform is spelled with a capital “H”.
Context: ...golangci-lint version that matches CI (.github/workflows/cli.yml) and the pinned lyc...
(GITHUB)
[uncategorized] ~45-~45: The official name of this software platform is spelled with a capital “H”.
Context: ...inned lychee version that matches CI (.github/workflows/lychee.yml). Re-run only aft...
(GITHUB)
🪛 Shellcheck (0.11.0)
scripts/install_cli_tools.sh
[warning] 222-222: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[info] 236-236: Use '[:upper:]' to support accents and foreign alphabets.
(SC2019)
[info] 236-236: Use '[:lower:]' to support accents and foreign alphabets.
(SC2018)
[info] 237-237: Use '[:upper:]' to support accents and foreign alphabets.
(SC2019)
[info] 237-237: Use '[:lower:]' to support accents and foreign alphabets.
(SC2018)
🔇 Additional comments (21)
scripts/install_cli_tools.sh (1)
2-279: LGTM!Also applies to: 283-287
renovate.json (1)
68-72: LGTM!Also applies to: 107-110, 127-130
.github/workflows/lychee.yml (1)
1-76: LGTM!lychee.toml (1)
1-68: LGTM!.gitignore (1)
30-30: LGTM!Also applies to: 97-98
.markdownlint.json (1)
9-9: LGTM!Also applies to: 13-13
.pre-commit-config.yaml (1)
17-22: LGTM!Also applies to: 44-44, 415-427
CLAUDE.md (1)
27-27: LGTM!Also applies to: 40-40
docs/getting_started.md (1)
37-51: LGTM!docs/decisions/0006-tiered-module-size-policy.md (1)
194-196: LGTM!docs/api/layer.md (1)
25-25: Confirm updatedsynthorg.corelink targets a validcore.md
docs/api/core.mdexists;docs/core/index.mdis not present, and there are no remainingcore/index\.mdreferences in*.md.docs/guides/a2a-federation.md (1)
112-112: Confirm cross-reference targeta2a-protocol.mdis valid
docs/design/a2a-protocol.mdexists, and there are no remaining markdown references to the old pathdesign/a2a.md(the old filedocs/design/a2a.mdalso does not appear to be present).docs/guides/cost-attribution.md (1)
101-101: Cross-reference looks consistent:budget.mdexists and oldcost-control.mdreferences are gone.
docs/design/budget.mdis present. The repo search forcost-control.mdin*.mdreturned no matches (anddocs/design/cost-control.mddoes not exist), so the updated link target should resolve correctly.README.md (1)
137-137: Confirm the README OpenAPI docs link is live and correct
https://synthorg.io/docs/openapi/returns 200 and the page content matches the Scalar/OpenAPI reference (includes OpenAPI schema download / Scalar UI references). The oldhttps://synthorg.io/docs/rest-api/URL returns 404.docs/guides/approval-workflow.md (1)
101-101: Check the documentation link after narrowing the design reference
docs/reference/sec-prompt-safety.mdexists, and the guide currently links only to it.docs/design/approval.mdis not present anywhere in the repo’s markdown files, so keeping the narrower reference is consistent—but confirm the original intent was to drop the missing design doc rather than an accidental omission.docs/design/a2a-protocol.md (1)
8-8: A2A protocol URL is reachable and old domain is no longer referenced.
https://a2a-protocol.org/returnsHTTP 200.- No remaining
agent-protocol.aireferences found in the repo for*.md,*.py,*.ts, and*.yml.docs/reference/comparison.md (1)
69-69: ⚡ Quick winOGX documentation URL looks correct
https://ogx-ai.github.io/docs returns HTTP 200 and serves an OGX-branded page (e.g., “Welcome to OGX | OGX” present in the content). The referenced repository URL https://github.com/meta-llama/llama-stack also returns HTTP 200.docs/research/llm-provider-auth-survey.md (1)
17-17: ⚡ Quick winVerify the Alibaba DashScope/Model Studio documentation URL content
https://www.alibabacloud.com/help/en/model-studioreturns HTTP 200.- The page content includes “Qwen” references, but the check doesn’t confirm it contains the actual DashScope/Model Studio API documentation (endpoints/auth/SDK usage). Ensure the linked page specifically documents DashScope/Qwen API usage, or update the link to the correct API-reference subpage if needed.
docs/guides/rest-api-examples.md (1)
213-213: ⚡ Quick winFix the internal WebSocket docs fragment link:
../api/layer.md#websocket-modelsis valid
docs/api/layer.mdexists and contains the## WebSocket Modelssection, which generates the#websocket-modelsfragment; the previousdocs/reference/websocket-protocol.mdtarget is removed.docs/guides/custom-rules-and-meta-loop.md (1)
118-118: ⚡ Quick winCross-reference
self-improvementnow resolves and stalemeta-loop.mdis removed.
docs/design/self-improvement.mdexists,docs/design/meta-loop.mdis not present, and there are no remainingmeta-loop.mdreferences in markdown.docs/guides/monitoring.md (1)
341-341: ⚡ Quick winFix Logfire docs link to point to Prometheus/
/metricssetup (current URL is only the docs landing page).https://pydantic.dev/docs/logfire/ is reachable (HTTP 200), but the fetched Logfire docs pages around it (get-started, integrations index, system-metrics, add-metrics) contain no Prometheus-specific setup text and don’t clearly document scraping/configuring the
/metricsendpoint; update the link to the specific Prometheus/metrics setup page that matches the claim.
Aureliolo
force-pushed
the
refactor/wire-lychee-ci
branch
from
1f11e93 to
eac9b2b
Compare
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/install_cli_tools.sh (1)
280-282:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail when PATH resolves a stale
lycheebinary.Line 280 only checks for presence on PATH. A stale
lycheeearlier in PATH can still be used by hooks even after installing the pinned version.Proposed fix
- if [ -z "$(lychee_on_path)" ]; then + local path_binary path_version + path_binary="$(lychee_on_path)" + if [ -z "${path_binary}" ]; then echo "warning: ${install_dir} is not on PATH; add it (e.g. 'export PATH=\"${install_dir}:\$PATH\"' in ~/.bashrc / ~/.zshrc) to use lychee directly" >&2 + else + path_version="$(extract_lychee_version "${path_binary}")" + if [ "${path_version:-}" != "${LYCHEE_VERSION}" ]; then + echo "error: lychee version mismatch on PATH -- expected ${LYCHEE_VERSION}, got '${path_version:-unknown}' from ${path_binary}" >&2 + echo "hint: ensure ${install_dir} precedes other lychee locations on PATH, or remove the stale binary" >&2 + return 1 + fi fi🤖 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/install_cli_tools.sh` around lines 280 - 282, Currently the script only checks lychee_on_path presence, which misses stale executables earlier in PATH; update the check so that when lychee_on_path returns a path (from command -v or which), resolve that path and compare it to the expected "${install_dir}/lychee" (or "${install_dir}/bin/lychee" if that’s the install layout) and if they differ, print an error to stderr and exit non-zero instructing the user to fix their PATH or remove the old lychee; reference the lychee_on_path function/command and the install_dir variable so you can locate and replace the simple presence check with a path-equality check and fail fast when a stale binary would be used.
🤖 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 `@scripts/install_cli_tools.sh`:
- Around line 84-103: The script currently only checks that golangci-lint exists
on PATH and separately verifies the installed_binary in ${install_dir}, which
can leave PATH pointing to an older binary; change the validation to resolve and
check the PATH-resolved binary version too: use command -v golangci-lint to set
path_binary, call extract_version on path_binary and compare that against
golangci_lint_version, and if it mismatches emit the existing error/hint
(mentioning ${install_dir} should precede other locations or remove stale
binary); keep the existing fallback that prefers installed_binary for
verification only if install_dir is unreadable, but ensure the PATH-resolved
binary is validated first so pre-push hooks will run the expected version.
---
Duplicate comments:
In `@scripts/install_cli_tools.sh`:
- Around line 280-282: Currently the script only checks lychee_on_path presence,
which misses stale executables earlier in PATH; update the check so that when
lychee_on_path returns a path (from command -v or which), resolve that path and
compare it to the expected "${install_dir}/lychee" (or
"${install_dir}/bin/lychee" if that’s the install layout) and if they differ,
print an error to stderr and exit non-zero instructing the user to fix their
PATH or remove the old lychee; reference the lychee_on_path function/command and
the install_dir variable so you can locate and replace the simple presence check
with a path-equality check and fail fast when a stale binary would be used.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a3eedf5-e3f0-4a96-b0b5-cd3b63e98e4c
📒 Files selected for processing (24)
.github/workflows/lychee.yml.gitignore.markdownlint.json.pre-commit-config.yamlCLAUDE.mdREADME.mddocs/api/layer.mddocs/decisions/0006-tiered-module-size-policy.mddocs/design/a2a-protocol.mddocs/getting_started.mddocs/guides/a2a-federation.mddocs/guides/approval-workflow.mddocs/guides/cost-attribution.mddocs/guides/custom-rules-and-meta-loop.mddocs/guides/dynamic-scoring.mddocs/guides/monitoring.mddocs/guides/rest-api-examples.mddocs/reference/comparison.mddocs/research/llm-provider-auth-survey.mdlychee.tomlrenovate.jsonscripts/install_cli_tools.shtests/conformance/persistence/conftest.pytests/conftest.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Site
- GitHub Check: Test Integration (shard 3)
- GitHub Check: Test Integration (shard 1)
- GitHub Check: Test Unit (shard 1)
- GitHub Check: Test Integration (shard 4)
- GitHub Check: Test Integration (shard 2)
- GitHub Check: Test Unit (shard 4)
- GitHub Check: Test Conformance (SQLite)
- GitHub Check: Test E2E
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
{README.md,docs/**/*.md}
📄 CodeRabbit inference engine (CLAUDE.md)
Numerics in README and public docs must be sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markers
Files:
docs/guides/cost-attribution.mddocs/guides/a2a-federation.mddocs/guides/dynamic-scoring.mddocs/guides/rest-api-examples.mddocs/guides/custom-rules-and-meta-loop.mddocs/getting_started.mddocs/design/a2a-protocol.mddocs/guides/approval-workflow.mddocs/api/layer.mddocs/reference/comparison.mddocs/decisions/0006-tiered-module-size-policy.mddocs/research/llm-provider-auth-survey.mdREADME.mddocs/guides/monitoring.md
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use D2 for architecture/nested containers (theme 200, v0.7.1); use mermaid for flowcharts/sequence/pipelines; use Markdown tables for tabular data
Files:
docs/guides/cost-attribution.mddocs/guides/a2a-federation.mddocs/guides/dynamic-scoring.mddocs/guides/rest-api-examples.mddocs/guides/custom-rules-and-meta-loop.mddocs/getting_started.mddocs/design/a2a-protocol.mddocs/guides/approval-workflow.mddocs/api/layer.mddocs/reference/comparison.mddocs/decisions/0006-tiered-module-size-policy.mddocs/research/llm-provider-auth-survey.mddocs/guides/monitoring.md
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test markers:
@pytest.mark.{unit,integration,e2e,slow}; async auto; global timeout 30s; coverage 80% minWindows unit tests use
WindowsSelectorEventLoopPolicy(3.14 IOCP teardown race); subprocess tests override backTest doubles: use
FakeClockfor Clock seam,mock_of[T](**overrides)for typed-boundary substitutions,SimpleNamespacefor attribute-bags; bareMagicMockat typed boundary blocked by check_mock_spec.pyFakeClock and
mock_ofimport fromtests._shared; inject viaclock=and helper's spec subscriptHypothesis: 10 deterministic CI examples; failures are real bugs (fix + add
@example(...))Flaky tests: never skip/xfail; fix fundamentally; use
asyncio.Event().wait()notsleep(large)
Files:
tests/conftest.pytests/conformance/persistence/conftest.py
⚙️ CodeRabbit configuration file
Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare
@settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which@given() honors automatically.
Files:
tests/conftest.pytests/conformance/persistence/conftest.py
tests/conformance/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Dual-backend conformance:
tests/conformance/persistence/consumesbackendfixture (SQLite + Postgres); enforced by check_dual_backend_test_parity.py
Files:
tests/conformance/persistence/conftest.py
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: Read design specification in `docs/design/` before implementing; deviations require approval per DESIGN_SPEC.md
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: Present every plan for accept/deny before coding
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: Use no region/currency/locale privileged defaults; use metric units; use British English
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: Every convention PR must ship its enforcement gate per docs/reference/convention-gates.md
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: Timeout/slow test failures indicate source-code regression; never edit baselines in `tests/baselines/unit_timing.json` or `scripts/*_baseline.{txt,json}` or `scripts/_*_baseline.py`; bypass only with explicit `ALLOW_BASELINE_GROWTH=1` commit flag
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: After issue completion: create branch + commit + push (no auto-PR); use `/pre-pr-review`; after PR creation use `/aurelio-review-pr` for external feedback; fix all valid feedback before merging
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: xdist applied auto via pyproject addopts: `-n 8 --dist=loadfile` (loadfile prevents 3.14+ Windows ProactorEventLoop leak)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: Postgres fixture defaults to testcontainers-managed `postgres:18-alpine`; bypass via `SYNTHORG_TEST_POSTGRES_*` env vars
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: Git commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: Branches: `<type>/<slug>` from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: Pre-commit/pre-push hooks via `.pre-commit-config.yaml`; tool-call gates via `.claude/settings.json` PreToolUse
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: Squash merge; PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) must be in PR body
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: Use GitHub Bash queries (`gh issue list`) instead of MCP `list_issues`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: After every squash merge → `/post-merge-cleanup`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T05:50:20.389Z
Learning: CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API
📚 Learning: 2026-05-16T18:36:31.446Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/reference/conventions.md:787-789
Timestamp: 2026-05-16T18:36:31.446Z
Learning: In Aureliolo/synthorg, do not require adding `<!--RS:...-->` “Doc Numeric Claims (MANDATORY)” numeric macros for Python version numbers mentioned in documentation prose (e.g., “Python 3.14”, “Python 3.15”). The `scripts/check_doc_numeric_macros.py` gate only applies to `README.md`, `docs/index.md`, `docs/roadmap/index.md`, `docs/architecture/decisions.md`, and `docs/reference/convention-gates.md`, and it only flags digits adjacent to specific stat nouns (tests/providers/agents/stars/releases), not language version mentions like “Python 3.14”.
Applied to files:
docs/guides/cost-attribution.mddocs/guides/a2a-federation.mddocs/guides/dynamic-scoring.mddocs/guides/rest-api-examples.mddocs/guides/custom-rules-and-meta-loop.mdCLAUDE.mddocs/getting_started.mddocs/design/a2a-protocol.mddocs/guides/approval-workflow.mddocs/api/layer.mddocs/reference/comparison.mddocs/decisions/0006-tiered-module-size-policy.mddocs/research/llm-provider-auth-survey.mdREADME.mddocs/guides/monitoring.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing Markdown in the synthorg repo, account for the CI gate `check_doc_numeric_macros.py`: it skips fenced code blocks entirely, and it only flags digits that are adjacent to these stat nouns: `tests`, `providers`, `agents`, `stars`, `releases`. Therefore, numeric examples such as CLI flag values (e.g., `--num-workers=4` in fenced bash blocks) and prose version numbers (e.g., `3.14`/`3.15`) are not expected to trigger this check; prioritize changes only when digits appear next to one of the listed nouns (e.g., “5 tests”, “10 stars”, etc.).
Applied to files:
docs/guides/cost-attribution.mddocs/guides/a2a-federation.mddocs/guides/dynamic-scoring.mddocs/guides/rest-api-examples.mddocs/guides/custom-rules-and-meta-loop.mdCLAUDE.mddocs/getting_started.mddocs/design/a2a-protocol.mddocs/guides/approval-workflow.mddocs/api/layer.mddocs/reference/comparison.mddocs/decisions/0006-tiered-module-size-policy.mddocs/research/llm-provider-auth-survey.mdREADME.mddocs/guides/monitoring.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing markdown files for the "Doc Numeric Claims (MANDATORY)" RS-marker rule, only require/flag missing RS markers in the files that are actually in-scope for the rule. The scope is enforced via an identical _SCOPED_FILES allowlist in scripts/check_doc_numeric_macros.py and scripts/inject_runtime_stats.py, and currently includes: README.md; docs/index.md; docs/roadmap/index.md; docs/architecture/decisions.md; docs/reference/convention-gates.md. For any other markdown files (e.g., docs/getting_started.md, docs/guides/*), missing RS markers for numeric claims are no-ops and should NOT be flagged.
Applied to files:
docs/guides/cost-attribution.mddocs/guides/a2a-federation.mddocs/guides/dynamic-scoring.mddocs/guides/rest-api-examples.mddocs/guides/custom-rules-and-meta-loop.mddocs/getting_started.mddocs/design/a2a-protocol.mddocs/guides/approval-workflow.mddocs/api/layer.mddocs/reference/comparison.mddocs/decisions/0006-tiered-module-size-policy.mddocs/research/llm-provider-auth-survey.mdREADME.mddocs/guides/monitoring.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing Markdown in the synthorg repo against the `check_doc_numeric_macros.py` gate, account for its documented behavior: it skips fenced code blocks entirely, and it only flags digits that are adjacent to specific stat nouns (`tests`, `providers`, `agents`, `stars`, `releases`). As a result, CLI-style numbers (e.g., `--num-workers=4`) inside fenced bash code blocks should never be treated as violations of this gate; only non-fenced text needs checking, and only around those specific nouns.
Applied to files:
docs/guides/cost-attribution.mddocs/guides/a2a-federation.mddocs/guides/dynamic-scoring.mddocs/guides/rest-api-examples.mddocs/guides/custom-rules-and-meta-loop.mddocs/getting_started.mddocs/design/a2a-protocol.mddocs/guides/approval-workflow.mddocs/api/layer.mddocs/reference/comparison.mddocs/decisions/0006-tiered-module-size-policy.mddocs/research/llm-provider-auth-survey.mddocs/guides/monitoring.md
📚 Learning: 2026-05-16T18:36:19.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/guides/contributing.md:95-95
Timestamp: 2026-05-16T18:36:19.195Z
Learning: In the SynthOrg repo, the “Doc Numeric Claims (MANDATORY)” RS-marker rule should be applied only to these docs: README.md; docs/index.md; docs/roadmap/index.md; docs/architecture/decisions.md; docs/reference/convention-gates.md. This rule is enforced by scripts/check_doc_numeric_macros.py (with runtime substitution by scripts/inject_runtime_stats.py), so reviewers should not flag similar numeric-claim issues in other paths (e.g., anything under docs/guides/). When checking those scoped files, the rule skips fenced code blocks and only flags digits that are adjacent to stat nouns (tests/providers/agents/stars/releases). Numeric CLI flags like “--num-workers=4” inside fenced bash code blocks are not subject to this rule.
Applied to files:
README.md
📚 Learning: 2026-05-16T18:36:31.446Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/reference/conventions.md:787-789
Timestamp: 2026-05-16T18:36:31.446Z
Learning: In Aureliolo/synthorg, follow the `Doc Numeric Claims (MANDATORY)` rule enforced by `scripts/check_doc_numeric_macros.py` only for these markdown files: `README.md`, `docs/index.md`, `docs/roadmap/index.md`, `docs/architecture/decisions.md`, and `docs/reference/convention-gates.md`. The gate flags digits that appear adjacent to the stat nouns `tests`, `providers`, `agents`, `stars`, and `releases`—those numeric claims must use the required `<!--RS:...-->` macro format. Do not apply this rule to prose that mentions Python version numbers (e.g., “Python 3.14” / “Python 3.15”); those should not be flagged as requiring `<!--RS:...-->`.
Applied to files:
README.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: In the synthorg repo, the “Doc Numeric Claims (MANDATORY)” RS-marker rule is enforced only for this exact set of Markdown files: README.md, docs/index.md, docs/roadmap/index.md, docs/architecture/decisions.md, and docs/reference/convention-gates.md. During code reviews, do not raise RS-marker/numeric-claims findings for numeric values in any other files (e.g., docs/getting_started.md, docs/guides/*, docs/reference/conventions.md), since they are not checked or injected by scripts/check_doc_numeric_macros.py or scripts/inject_runtime_stats.py.
Applied to files:
README.md
📚 Learning: 2026-05-05T09:04:46.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.
Applied to files:
tests/conftest.pytests/conformance/persistence/conftest.py
📚 Learning: 2026-05-21T22:55:20.496Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2035
File: src/synthorg/meta/toolsmith/models.py:114-114
Timestamp: 2026-05-21T22:55:20.496Z
Learning: In this repo’s “magic number” review standard, the existing gate in `scripts/check_no_magic_numbers.py` intentionally does NOT flag numeric literals used as raw call-site arguments. So, do not flag numeric literals passed as keyword arguments to Pydantic `Field()` (e.g., `Field(ge=0, le=100)` / `Field(ge=1, le=50)`)—this is an established idiom. Only treat numeric literals as “magic numbers” when they occur in the locations the gate checks (module-level assignments and function/method parameter defaults).
Applied to files:
tests/conftest.pytests/conformance/persistence/conftest.py
📚 Learning: 2026-05-23T12:24:00.128Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2080
File: tests/_shared/test_postgres_proxy.py:19-48
Timestamp: 2026-05-23T12:24:00.128Z
Learning: When creating test doubles for Python typing.Protocols in tests, prefer a hand-written Protocol fake (a concrete class that explicitly implements the Protocol) over `mock_of[T]` if the Protocol only defines annotation-only attributes (e.g., `username: str`, `password: str`, `dbname: str`) with no class-level values/assignments. This is because `mock_of[T]` relies on `create_autospec(..., spec_set=True)`, which enumerates members via `dir(spec)`; annotation-only attributes are not included, so `mock_of`’s kwarg-based attribute setting can raise `AttributeError: attribute not present on spec type`. In that annotation-only case, don’t recommend `mock_of[T]`—use an explicit fake class instead.
Applied to files:
tests/conftest.pytests/conformance/persistence/conftest.py
🪛 LanguageTool
docs/getting_started.md
[uncategorized] ~45-~45: The official name of this software platform is spelled with a capital “H”.
Context: ...golangci-lint version that matches CI (.github/workflows/cli.yml) and the pinned lyc...
(GITHUB)
[uncategorized] ~45-~45: The official name of this software platform is spelled with a capital “H”.
Context: ...inned lychee version that matches CI (.github/workflows/lychee.yml). Re-run only aft...
(GITHUB)
🪛 Shellcheck (0.11.0)
scripts/install_cli_tools.sh
[warning] 222-222: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[info] 236-236: Use '[:upper:]' to support accents and foreign alphabets.
(SC2019)
[info] 236-236: Use '[:lower:]' to support accents and foreign alphabets.
(SC2018)
[info] 237-237: Use '[:upper:]' to support accents and foreign alphabets.
(SC2019)
[info] 237-237: Use '[:lower:]' to support accents and foreign alphabets.
(SC2018)
🔇 Additional comments (24)
scripts/install_cli_tools.sh (1)
2-83: LGTM!Also applies to: 104-279, 284-287
renovate.json (1)
68-72: LGTM!Also applies to: 107-110, 127-130
.github/workflows/lychee.yml (1)
1-76: LGTM!lychee.toml (1)
1-68: LGTM!.gitignore (1)
30-30: LGTM!Also applies to: 97-98
.markdownlint.json (1)
9-9: LGTM!Also applies to: 13-13
.pre-commit-config.yaml (1)
17-21: LGTM!Also applies to: 44-44, 415-427
CLAUDE.md (1)
27-27: LGTM!Also applies to: 40-40
docs/getting_started.md (1)
37-51: LGTM!README.md (1)
137-137: LGTM!docs/api/layer.md (1)
25-25: LGTM!docs/decisions/0006-tiered-module-size-policy.md (1)
194-196: LGTM!docs/design/a2a-protocol.md (1)
8-8: LGTM!docs/guides/a2a-federation.md (1)
112-112: LGTM!docs/guides/approval-workflow.md (1)
101-101: LGTM!docs/guides/cost-attribution.md (1)
101-101: LGTM!docs/guides/dynamic-scoring.md (1)
130-130: Previously flagged cross-reference concern.This line matches a concern already raised in prior review feedback.
docs/guides/custom-rules-and-meta-loop.md (1)
118-118: LGTM!docs/guides/monitoring.md (1)
341-341: LGTM!docs/guides/rest-api-examples.md (1)
213-213: LGTM!docs/reference/comparison.md (1)
69-69: LGTM!Also applies to: 119-119, 169-169, 219-219, 267-267
docs/research/llm-provider-auth-survey.md (1)
17-17: LGTM!Also applies to: 313-313
tests/conformance/persistence/conftest.py (1)
324-332: LGTM!tests/conftest.py (1)
932-944: LGTM!Also applies to: 960-972, 982-1007, 1009-1028, 1032-1047
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
|
@coderabbitai fullreview |
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll pull the key files for a thorough review before writing up findings. 🏁 Script executed: Length of output: 180461
Review SummaryThe overall implementation is well-structured and thorough. One critical bug must be fixed before merge; a handful of medium/low findings are noted below. 🔴 Critical —
|
…installer refactor) Pre-reviewed by 5 agents (infra-reviewer, docs-consistency, comment-quality-rot, issue-resolution-verifier, code-reviewer); 6 findings addressed.
Rebased on origin/main to pick up PR #2082's workflow SHA refresh, then folded fixes for the six failing checks (zizmor, lychee, 3x test flake symptoms, CI Pass) plus the three gemini-code-assist inline comments. lychee.yml: bump local action SHA 31a45a7 → 2592118 to match the rest of the workflows on main (the old SHA is no longer reachable in the repo, which is why zizmor's impostor-commit check fired across every workflow); rename version: to lycheeVersion: (the input name in v2 of lycheeverse/lychee-action — the old name was silently ignored, so the action ran default v0.23.0 instead of the pinned v0.24.2). renovate.json (gemini #1): \s+ → \s* so the binary-tool matcher catches unindented shell variables (LYCHEE_VERSION sits at column 0, not after YAML indent). Also add lycheeVersion: alternation to the action-input matcher so renovate keeps tracking the lychee bump path after the rename above. scripts/install_cli_tools.sh (gemini #2): handle GOPATH multi-entry strings — take the first colon/semicolon-separated entry, since go install writes to the first entry's bin/ and the raw joined string breaks mkdir -p. scripts/install_cli_tools.sh (gemini #3): switch tmpdir cleanup trap from RETURN to EXIT so set -e failures (failed curl / sha256sum / tar) still trigger cleanup; expand ${tmpdir} at trap-install time since the local goes out of scope before EXIT fires. lychee.toml: exclude dash.cloudflare.com (always 403 to unauthenticated probes — referenced from docs/guides/fork-setup.md as a navigation pointer) and docs.sigstore.dev (intermittent timeouts under the strict 10s ceiling — referenced from docs/guides/deployment.md). Test failures (Test Unit shard 4, Test E2E, Test Conformance SQLite) are pre-existing pytest_sessionstart cross-worker flakes — the os.abort() in tests/conftest.py is the documented forensic mechanism for capturing stacks when the FileLock races. The same flake hit main's prior CI run at 22:29 and self-healed on the next run at 23:51; re-pushing should re-trigger and likely clear them. CI Pass is the aggregator; it will go green once the above clear.
RCA for the test_unit/e2e/conformance pytest_sessionstart timeouts: the SQLite template DB build in tests/conftest.py serialised every xdist worker behind a single 180s FileLock. When the leader's yoyo migration chain on a cold-cache CI runner crossed 180s (PR #2080's 4-way sharding + COVERAGE_CORE=sysmon overhead is what pushed it past the line), followers (gw1/gw2) timed out at exactly the ceiling and os.abort()ed. Same pattern flaked on origin/main at 22:29 (sha 2592118) and self-cleared at 23:51 (sha 72c6648) on luck, not a fix. Three changes in tests/conftest.py: 1. Poll-acquire loop for followers: instead of one blocking lock.acquire(timeout=600), try the lock in 5s slices and re-check db_path.exists() between attempts. Follower wait now tracks the leader's actual build time + one poll slice, not the catastrophe ceiling. The leader path is unchanged. 2. Atomic rename: builder writes to template.db.building then Path.replace()s to template.db. Closes a latent partial-write race -- yoyo's migrate_apply opens the SQLite file at start of migration, so a follower racing the fast-path exists() check during a direct-to-template.db build could read a half-migrated file. Comment-rationale updated (the previous comment claimed migrate_apply used an implicit temp path, which is incorrect). 3. _FILE_LOCK_TIMEOUT_SECONDS 180 -> 600. Catastrophe ceiling for a wedged lock, not the expected wait. Mirrored to tests/conformance/persistence/conftest.py's postgres-container coordinator (refcount semantics there require every worker to acquire so polling doesn't help, but the ceiling is aligned). Holistic sweep of FileLock + cross-worker shared-dir patterns confirms only the SQLite template path has the unlocked fast-path-read shape; postgres state is fully lock-serialised (no fast-path), and tests/integration/persistence/test_wp1_restart_safety.py only references the pattern in a comment.
…indings Root cause of round-2 lychee CI failure: lycheeverse/lychee-action v2.8.0's install step does not handle lychee v0.24.x's new nested archive layout. The action's #330 fix (merged 2026-04-24) has not shipped a v2.9 release yet (issue #332 open). Our local install_cli_tools.sh already handles the nested archive correctly, so the cleanest fix is to drop the action entirely and use the script in CI. Removes drift: the script (local pre-push hook) and CI now install the byte-identical binary from one Renovate-tracked version pin. Eliminates upstream-action release-cadence coupling. Changes: - scripts/install_cli_tools.sh: positional subcommand dispatcher (all | lychee | golangci-lint, default all). PATH-staleness check after both installs (CodeRabbit major actionable + AI-agent inline prompt): if the PATH-resolved binary differs from the install_dir binary AND its --version disagrees with the pin, fail fast with a 'fix your PATH or remove the stale binary' hint. SC2064 disabled on the EXIT trap with rationale comment (early expansion is intentional since the tmpdir is function-local). - .github/workflows/lychee.yml: replaced lycheeverse/lychee-action step with three steps: install via bash scripts/install_cli_tools.sh lychee (LYCHEE_INSTALL_DIR at step-level env per actionlint), echo to GITHUB_PATH, then run lychee directly. --format markdown teed into GITHUB_STEP_SUMMARY preserves the action's job-summary feature. Path-filter widened to include scripts/install_cli_tools.sh so the workflow re-runs when the install changes. - renovate.json: reverted the version|lycheeVersion regex alternation (no workflow uses lycheeVersion: now that the action is gone). - docs/guides/dynamic-scoring.md: fixed misleading cross-reference (CodeRabbit minor). scoring-hyperparameters.md is operator-tunable weights / thresholds, not an architecture overview; the sentence now says exactly that instead of pretending it documents composite/weighted/ranked/multi-objective strategies.
…mbers Root cause: _RATE_LIMIT_MARKERS held the bare substring "429", which substring-matched any text containing those three digits in sequence. Python's http.server picks ephemeral ports in the ~32k-60k range, frequently producing port strings like 42919 (literal CI log: 'fatal: repository https://localhost:42919/acme/proj-new.git/ not found'). The 'not found' stderr from a clone of a missing remote — exactly the path test_lazy_create_on_missing_remote exercises — got misclassified as a forge rate-limit, GitBackendRateLimitError was raised instead of GitBackendRemoteMissingError, the lazy-create branch never fired, and the retry budget burned through 3 attempts before re-raising. Fix: split rate-limit detection into substring markers (rate limit, too many requests; both genuinely unique phrases) plus a regex with digit-boundary lookaround for the HTTP-429 status code: (?<!\d)429(?!\d). The regex still matches every realistic '429 Too Many Requests', 'HTTP 429', 'status: 429', '429:' shape while rejecting '42919', '4290', '1429'. Wrapped in a new _is_rate_limit() helper; the two call sites (_do_push, _do_fetch) swap from _matches(_, _RATE_LIMIT_MARKERS) to _is_rate_limit(_). Regression test: parametrized over three port-substring URL shapes (42919, 14290, 34291), asserts the classifier reaches the remote-missing branch (forge.exists=False), triggers the lazy-create path, and the second push attempt succeeds — i.e. exactly the integration-test contract that was broken. Surfaced by the integration shard 4 failure on PR #2084 after the merge of main pulled in PR #2085 (codespell vocab + identifier renames). The test itself was not new and the failing code path is unrelated to either PR; the merge widened the affected-tests set so the latent flake re-fired.
The file is a classic adapter — translates the internal GitBackendProtocol to external git forges (GitHub/GitLab/Gitea/Forgejo) via the git CLI (subprocess) and a per-forge REST client. The default 'code' tier cap of 500 LOC is inappropriate for an integration boundary; 'adapter' tier (cap 700) is the correct classification and matches similar boundary files in the repo. Surfaced by the module-size budget pre-push gate after the rate-limit classifier fix in the previous commit pushed the file from 523 to 528 LOC, over the existing baseline. Reclassification leaves comfortable headroom (528 of 700) without bumping any baseline.
Aureliolo
force-pushed
the
refactor/wire-lychee-ci
branch
from
c14bf52 to
b15ddf9
Compare
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Merging this PR will not alter performance
Comparing 
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@scripts/install_cli_tools.sh`:
- Around line 245-248: The two single-attempt curl calls that download
"${download_url}" and "${sha_url}" are flaky; replace them with a retry/backoff
mechanism (either add curl's --retry and --retry-delay/--retry-max-time flags or
wrap the curl call in a short retry loop) so the downloads for
"${tmpdir}/${archive}" and "${tmpdir}/${archive}.sha256" are retried on
transient failures; update both curl invocations found in
scripts/install_cli_tools.sh to use the same retry/backoff logic (referencing
the variables download_url, sha_url, tmpdir, and archive) and ensure the loop
exits with a non-zero status after exhausting attempts.
- Around line 241-242: The ShellCheck directive is malformed; replace the inline
comment marker so the directive parses correctly: change the line with the
directive above the trap from "# shellcheck disable=SC2064 -- early expansion
intentional: ${tmpdir} is function-local." to use a proper directive plus a
normal comment, e.g. "# shellcheck disable=SC2064 # early expansion intentional:
${tmpdir} is function-local." so that the SC2064 suppression applies to the trap
"rm -rf '${tmpdir}'" while the explanatory text remains a valid comment.
In `@tests/unit/engine/workspace/git_backend/test_external_remote_backend.py`:
- Line 286: Replace the direct await_count assertion on the mock with the
idiomatic unittest.mock helper: change the check using
forge.create_repo.await_count == 1 to call
forge.create_repo.assert_awaited_once() so the test uses consistent, clearer
mock assertion semantics (referencing the forge.create_repo mock used in the
test).
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 55a62872-a584-4d28-a941-494f8cd8947b
📒 Files selected for processing (26)
.github/workflows/lychee.yml.gitignore.markdownlint.json.pre-commit-config.yamlCLAUDE.mdREADME.mddocs/api/layer.mddocs/decisions/0006-tiered-module-size-policy.mddocs/design/a2a-protocol.mddocs/getting_started.mddocs/guides/a2a-federation.mddocs/guides/approval-workflow.mddocs/guides/cost-attribution.mddocs/guides/custom-rules-and-meta-loop.mddocs/guides/dynamic-scoring.mddocs/guides/monitoring.mddocs/guides/rest-api-examples.mddocs/reference/comparison.mddocs/research/llm-provider-auth-survey.mdlychee.tomlrenovate.jsonscripts/install_cli_tools.shsrc/synthorg/engine/workspace/git_backend/external_remote.pytests/conformance/persistence/conftest.pytests/conftest.pytests/unit/engine/workspace/git_backend/test_external_remote_backend.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Build Backend
- GitHub Check: Test Unit (shard 1)
- GitHub Check: Test Conformance (SQLite)
- GitHub Check: Test Integration (shard 1)
- GitHub Check: Test Integration (shard 2)
- GitHub Check: Test Integration (shard 4)
- GitHub Check: Test E2E
- GitHub Check: Test Integration (shard 3)
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Site
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{md,markdown}
📄 CodeRabbit inference engine (CLAUDE.md)
Numerics in README and public documentation must be sourced from data/runtime_stats.yaml via markers
Documentation: follow D2 and Mermaid diagram guidelines for architecture and flowchart illustrations
Commits: use : format (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced; signed commits required on protected refs (GPG/SSH or GitHub App); branches: / from main; squash merge with PR body as commit message; trailers (Release-As, Closes
#N) must be in PR body
Files:
docs/api/layer.mddocs/design/a2a-protocol.mddocs/guides/cost-attribution.mddocs/guides/custom-rules-and-meta-loop.mdREADME.mddocs/guides/approval-workflow.mddocs/guides/a2a-federation.mddocs/guides/dynamic-scoring.mddocs/research/llm-provider-auth-survey.mddocs/guides/rest-api-examples.mddocs/getting_started.mdCLAUDE.mddocs/reference/comparison.mddocs/decisions/0006-tiered-module-size-policy.mddocs/guides/monitoring.md
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
After issue resolution: create branch, commit, and push without auto-PR; use /pre-pr-review command (gh pr create is blocked by scripts/check_no_pr_create.sh); after PR created: use /aurelio-review-pr for external feedback; fix all valid feedback before merging
Module-size budget: tiered LOC caps per # module-kind: header on first non-blank/non-shebang/non-encoding line: controller 400, service/orchestrator 600, repository 500, adapter/integration 700, feature 100, code 500 (default), tests 800, declarative exempt, generated glob-exempt; existing offenders baselined in scripts/_module_size_baseline.json; no file may grow past its baseline; allowlisted god-modules (api/app.py, api/state.py, api/auto_wire.py, api/lifecycle.py, api/lifecycle_builder.py, core/enums.py, observability/events/persistence.py) must net-shrink
Files:
tests/unit/engine/workspace/git_backend/test_external_remote_backend.pytests/conformance/persistence/conftest.pysrc/synthorg/engine/workspace/git_backend/external_remote.pytests/conftest.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test markers: use
@pytest.mark.{unit,integration,e2e,slow}; async tests use auto; global timeout 30s; coverage minimum 80%; xdist -n 8 --dist=loadfile auto-applied via pyproject addopts; Windows unit tests use WindowsSelectorEventLoopPolicy (3.14 IOCP teardown race); subprocess tests override backTest doubles: ladder detailed in conventions.md section 12.1; use FakeClock for Clock seam, mock_ofT for typed-boundary substitutions, SimpleNamespace for attribute-bags; bare MagicMock at typed boundary blocked by scripts/check_mock_spec.py (zero-tolerance); FakeClock and mock_of imported from tests._shared
Vendor-agnostic testing: NEVER use real vendor names in project code/tests; use example-provider, test-provider, example-{large,medium,small}-001; real names allowed only in .claude/, third-party imports, providers/presets.py, web/public/provider-logos/
Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add
@example(...)); never skip/xfail flaky tests; fix fundamentally using asyncio.Event().wait() not sleep(large)
Files:
tests/unit/engine/workspace/git_backend/test_external_remote_backend.pytests/conformance/persistence/conftest.pytests/conftest.py
⚙️ CodeRabbit configuration file
Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare
@settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which@given() honors automatically.
Files:
tests/unit/engine/workspace/git_backend/test_external_remote_backend.pytests/conformance/persistence/conftest.pytests/conftest.py
docs/reference/**
📄 CodeRabbit inference engine (CLAUDE.md)
Reference documentation: maintain comprehensive docs in docs/reference/ covering claude-reference.md, conventions.md, convention-gates.md, regional-defaults.md, persistence-boundary.md, configuration-precedence.md, errors.md, sec-prompt-safety.md, lifecycle-sync.md, mcp-handler-contract.md, typed-boundaries.md, retry-patterns.md, scaffolding.md, audit-category-gate-coverage.md, dead-api-endpoints.md, pluggable-subsystems.md, protocols-audit.md, telemetry.md; load on demand as reference
Files:
docs/reference/comparison.md
tests/conformance/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Dual-backend conformance: persistence tests consume backend fixture (SQLite + Postgres); enforced by check_dual_backend_test_parity.py
Files:
tests/conformance/persistence/conftest.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Configuration precedence via SettingsService/ConfigResolver: DB > env > code default (Cat-1), or env > code default with read_only_post_init (Cat-2); Cat-3 bootstrap secrets are pure env at the boot site; no os.environ.get outside startup; use settings.bootstrap_resolver.resolve_init_value for pre-init Cat-2 reads
Comments must explain WHY only; no reviewer citations, issue back-references, or migration framing
No 'from future import annotations' (Python 3.14 has PEP 649); use PEP 758 except: except A, B: without parens unless binding
Type hints required on public functions; strict mypy checking; use Google-style docstrings; maintain line length of 88 characters; keep functions under 50 lines
Error classes must follow Error naming and inherit from DomainError, never directly from Exception/RuntimeError/etc; enforced by check_domain_error_hierarchy.py
Pydantic v2: use frozen + extra="forbid" on every frozen model project-wide (gate check_frozen_model_extra_forbid.py);
@computed_fieldauto-exempt; use # lint-allow: frozen-extra-forbid -- for extra="allow"/"ignore" boundaries;@computed_fieldfor derived fields; use NotBlankStr for identifiersArgs models required at every system boundary; use parse_typed() for every external dict ingestion; enforced by check_boundary_typed.py
Immutability: use model_copy(update=...) or copy.deepcopy(); apply deepcopy at system boundaries
Async code: use asyncio.TaskGroup for fan-out/fan-in; async helpers catch Exception (re-raise MemoryError/RecursionError)
Clock seam: use clock: Clock | None = None parameter; tests inject FakeClock; services own _lifecycle_lock; timed-out stops mark unrestartable
Untrusted content (SEC-1): use wrap_untrusted() from engine.prompt_safety; use HTMLParseGuard for HTML processing
Datetime in persistence: use parse_iso_utc / format_iso_utc from persistence._shared (reject naive datetimes); use normalize_utc for already-typed datetimes
Logging: use 'from synthorg....
Files:
src/synthorg/engine/workspace/git_backend/external_remote.py
src/**/*.py
⚙️ CodeRabbit configuration file
This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.
Files:
src/synthorg/engine/workspace/git_backend/external_remote.py
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:22:22.492Z
Learning: Read the design specification (docs/design/ page) before implementing; deviations need approval per DESIGN_SPEC.md
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:22:22.492Z
Learning: Present every plan for accept/deny before coding
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:22:22.492Z
Learning: No region/currency/locale privileged; use metric units and British English for all user-facing content
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:22:22.492Z
Learning: Every convention PR must ship its enforcement gate per docs/reference/convention-gates.md
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:22:22.492Z
Learning: Post-implementation and pre-PR review workflow: use /pre-pr-review command and /aurelio-review-pr; fix everything valid without deferring
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:22:22.492Z
Learning: Bash/shell rules: see ~/.claude/rules/common/bash.md (canonical for cd / git -C / Bash file-write rules)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:22:22.492Z
Learning: Documentation diagrams: use d2 for architecture/nested containers (theme 200 Dark Mauve, CLI pinned v0.7.1); use mermaid for flowcharts/sequence/pipelines; use Markdown tables for tabular data
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:22:22.492Z
Learning: Git workflow: commits follow <type>: <description> (feat/fix/refactor/docs/test/chore/perf/ci) via commitizen; signed commits required on protected refs (GPG/SSH or GitHub App); branches: <type>/<slug> from main; squash merge with PR body as commit; trailers (Release-As, Closes `#N`) in PR body; after merge use /post-merge-cleanup
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:22:22.492Z
Learning: After every squash merge, run /post-merge-cleanup; CLI is Docker-only (init/start/stop/status); new features go in dashboard + REST API, not CLI
📚 Learning: 2026-05-16T18:36:31.446Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/reference/conventions.md:787-789
Timestamp: 2026-05-16T18:36:31.446Z
Learning: In Aureliolo/synthorg, do not require adding `<!--RS:...-->` “Doc Numeric Claims (MANDATORY)” numeric macros for Python version numbers mentioned in documentation prose (e.g., “Python 3.14”, “Python 3.15”). The `scripts/check_doc_numeric_macros.py` gate only applies to `README.md`, `docs/index.md`, `docs/roadmap/index.md`, `docs/architecture/decisions.md`, and `docs/reference/convention-gates.md`, and it only flags digits adjacent to specific stat nouns (tests/providers/agents/stars/releases), not language version mentions like “Python 3.14”.
Applied to files:
docs/api/layer.mddocs/design/a2a-protocol.mddocs/guides/cost-attribution.mddocs/guides/custom-rules-and-meta-loop.mdREADME.mddocs/guides/approval-workflow.mddocs/guides/a2a-federation.mddocs/guides/dynamic-scoring.mddocs/research/llm-provider-auth-survey.mddocs/guides/rest-api-examples.mddocs/getting_started.mdCLAUDE.mddocs/reference/comparison.mddocs/decisions/0006-tiered-module-size-policy.mddocs/guides/monitoring.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing Markdown in the synthorg repo, account for the CI gate `check_doc_numeric_macros.py`: it skips fenced code blocks entirely, and it only flags digits that are adjacent to these stat nouns: `tests`, `providers`, `agents`, `stars`, `releases`. Therefore, numeric examples such as CLI flag values (e.g., `--num-workers=4` in fenced bash blocks) and prose version numbers (e.g., `3.14`/`3.15`) are not expected to trigger this check; prioritize changes only when digits appear next to one of the listed nouns (e.g., “5 tests”, “10 stars”, etc.).
Applied to files:
docs/api/layer.mddocs/design/a2a-protocol.mddocs/guides/cost-attribution.mddocs/guides/custom-rules-and-meta-loop.mdREADME.mddocs/guides/approval-workflow.mddocs/guides/a2a-federation.mddocs/guides/dynamic-scoring.mddocs/research/llm-provider-auth-survey.mddocs/guides/rest-api-examples.mddocs/getting_started.mdCLAUDE.mddocs/reference/comparison.mddocs/decisions/0006-tiered-module-size-policy.mddocs/guides/monitoring.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing markdown files for the "Doc Numeric Claims (MANDATORY)" RS-marker rule, only require/flag missing RS markers in the files that are actually in-scope for the rule. The scope is enforced via an identical _SCOPED_FILES allowlist in scripts/check_doc_numeric_macros.py and scripts/inject_runtime_stats.py, and currently includes: README.md; docs/index.md; docs/roadmap/index.md; docs/architecture/decisions.md; docs/reference/convention-gates.md. For any other markdown files (e.g., docs/getting_started.md, docs/guides/*), missing RS markers for numeric claims are no-ops and should NOT be flagged.
Applied to files:
docs/api/layer.mddocs/design/a2a-protocol.mddocs/guides/cost-attribution.mddocs/guides/custom-rules-and-meta-loop.mdREADME.mddocs/guides/approval-workflow.mddocs/guides/a2a-federation.mddocs/guides/dynamic-scoring.mddocs/research/llm-provider-auth-survey.mddocs/guides/rest-api-examples.mddocs/getting_started.mddocs/reference/comparison.mddocs/decisions/0006-tiered-module-size-policy.mddocs/guides/monitoring.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing Markdown in the synthorg repo against the `check_doc_numeric_macros.py` gate, account for its documented behavior: it skips fenced code blocks entirely, and it only flags digits that are adjacent to specific stat nouns (`tests`, `providers`, `agents`, `stars`, `releases`). As a result, CLI-style numbers (e.g., `--num-workers=4`) inside fenced bash code blocks should never be treated as violations of this gate; only non-fenced text needs checking, and only around those specific nouns.
Applied to files:
docs/api/layer.mddocs/design/a2a-protocol.mddocs/guides/cost-attribution.mddocs/guides/custom-rules-and-meta-loop.mddocs/guides/approval-workflow.mddocs/guides/a2a-federation.mddocs/guides/dynamic-scoring.mddocs/research/llm-provider-auth-survey.mddocs/guides/rest-api-examples.mddocs/getting_started.mddocs/reference/comparison.mddocs/decisions/0006-tiered-module-size-policy.mddocs/guides/monitoring.md
📚 Learning: 2026-05-16T18:36:19.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/guides/contributing.md:95-95
Timestamp: 2026-05-16T18:36:19.195Z
Learning: In the SynthOrg repo, the “Doc Numeric Claims (MANDATORY)” RS-marker rule should be applied only to these docs: README.md; docs/index.md; docs/roadmap/index.md; docs/architecture/decisions.md; docs/reference/convention-gates.md. This rule is enforced by scripts/check_doc_numeric_macros.py (with runtime substitution by scripts/inject_runtime_stats.py), so reviewers should not flag similar numeric-claim issues in other paths (e.g., anything under docs/guides/). When checking those scoped files, the rule skips fenced code blocks and only flags digits that are adjacent to stat nouns (tests/providers/agents/stars/releases). Numeric CLI flags like “--num-workers=4” inside fenced bash code blocks are not subject to this rule.
Applied to files:
README.md
📚 Learning: 2026-05-16T18:36:31.446Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/reference/conventions.md:787-789
Timestamp: 2026-05-16T18:36:31.446Z
Learning: In Aureliolo/synthorg, follow the `Doc Numeric Claims (MANDATORY)` rule enforced by `scripts/check_doc_numeric_macros.py` only for these markdown files: `README.md`, `docs/index.md`, `docs/roadmap/index.md`, `docs/architecture/decisions.md`, and `docs/reference/convention-gates.md`. The gate flags digits that appear adjacent to the stat nouns `tests`, `providers`, `agents`, `stars`, and `releases`—those numeric claims must use the required `<!--RS:...-->` macro format. Do not apply this rule to prose that mentions Python version numbers (e.g., “Python 3.14” / “Python 3.15”); those should not be flagged as requiring `<!--RS:...-->`.
Applied to files:
README.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: In the synthorg repo, the “Doc Numeric Claims (MANDATORY)” RS-marker rule is enforced only for this exact set of Markdown files: README.md, docs/index.md, docs/roadmap/index.md, docs/architecture/decisions.md, and docs/reference/convention-gates.md. During code reviews, do not raise RS-marker/numeric-claims findings for numeric values in any other files (e.g., docs/getting_started.md, docs/guides/*, docs/reference/conventions.md), since they are not checked or injected by scripts/check_doc_numeric_macros.py or scripts/inject_runtime_stats.py.
Applied to files:
README.md
📚 Learning: 2026-05-05T09:04:46.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.
Applied to files:
tests/unit/engine/workspace/git_backend/test_external_remote_backend.pytests/conformance/persistence/conftest.pysrc/synthorg/engine/workspace/git_backend/external_remote.pytests/conftest.py
📚 Learning: 2026-05-21T22:55:20.496Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2035
File: src/synthorg/meta/toolsmith/models.py:114-114
Timestamp: 2026-05-21T22:55:20.496Z
Learning: In this repo’s “magic number” review standard, the existing gate in `scripts/check_no_magic_numbers.py` intentionally does NOT flag numeric literals used as raw call-site arguments. So, do not flag numeric literals passed as keyword arguments to Pydantic `Field()` (e.g., `Field(ge=0, le=100)` / `Field(ge=1, le=50)`)—this is an established idiom. Only treat numeric literals as “magic numbers” when they occur in the locations the gate checks (module-level assignments and function/method parameter defaults).
Applied to files:
tests/unit/engine/workspace/git_backend/test_external_remote_backend.pytests/conformance/persistence/conftest.pysrc/synthorg/engine/workspace/git_backend/external_remote.pytests/conftest.py
📚 Learning: 2026-05-23T12:24:00.128Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2080
File: tests/_shared/test_postgres_proxy.py:19-48
Timestamp: 2026-05-23T12:24:00.128Z
Learning: When creating test doubles for Python typing.Protocols in tests, prefer a hand-written Protocol fake (a concrete class that explicitly implements the Protocol) over `mock_of[T]` if the Protocol only defines annotation-only attributes (e.g., `username: str`, `password: str`, `dbname: str`) with no class-level values/assignments. This is because `mock_of[T]` relies on `create_autospec(..., spec_set=True)`, which enumerates members via `dir(spec)`; annotation-only attributes are not included, so `mock_of`’s kwarg-based attribute setting can raise `AttributeError: attribute not present on spec type`. In that annotation-only case, don’t recommend `mock_of[T]`—use an explicit fake class instead.
Applied to files:
tests/unit/engine/workspace/git_backend/test_external_remote_backend.pytests/conformance/persistence/conftest.pytests/conftest.py
📚 Learning: 2026-05-21T22:55:09.289Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2035
File: src/synthorg/meta/toolsmith/config.py:29-30
Timestamp: 2026-05-21T22:55:09.289Z
Learning: For this repo’s Pydantic configuration idiom, do not treat numeric literals passed directly as arguments to `pydantic.Field(...)` as “magic numbers” during review. This includes call-site usages like `Field(default=0.2, ge=0.0, le=1.0)` (e.g., in config models such as `ToolAuthoringConfig`, `ToolValidationConfig`, `ToolsmithConfig`). Do not request extracting those `Field(...)` numeric arguments into named constants, since the repo’s `scripts/check_no_magic_numbers.py` intentionally excludes call-site `Field(...)` numerics and relies on `Field(...)` as the canonical way to express these constraints/defaults.
Applied to files:
src/synthorg/engine/workspace/git_backend/external_remote.py
🪛 GitHub Actions: Link Check (lychee) / 0_lychee.txt
scripts/install_cli_tools.sh
[error] 1-1: Failed to install lychee v0.24.2. curl returned error (22): The requested URL returned error: 502.
🪛 GitHub Actions: Link Check (lychee) / lychee
scripts/install_cli_tools.sh
[error] 1-1: Command failed while installing lychee v0.24.2: curl returned error 502 (requested URL returned error: 502). Process completed with exit code 22.
🪛 LanguageTool
docs/getting_started.md
[uncategorized] ~45-~45: The official name of this software platform is spelled with a capital “H”.
Context: ...golangci-lint version that matches CI (.github/workflows/cli.yml) and the pinned lyc...
(GITHUB)
[uncategorized] ~45-~45: The official name of this software platform is spelled with a capital “H”.
Context: ...inned lychee version that matches CI (.github/workflows/lychee.yml). Re-run only aft...
(GITHUB)
🪛 Shellcheck (0.11.0)
scripts/install_cli_tools.sh
[info] 154-154: The mentioned syntax error was in this brace group.
(SC1009)
[error] 241-241: Couldn't parse this shellcheck directive. Fix to allow more checks.
(SC1073)
[error] 241-241: Expected '=' after directive key. Fix any mentioned problems and try again.
(SC1072)
🔇 Additional comments (24)
src/synthorg/engine/workspace/git_backend/external_remote.py (1)
100-115: LGTM!Also applies to: 450-450, 509-509
.github/workflows/lychee.yml (1)
19-42: LGTM!Also applies to: 44-48, 50-108
lychee.toml (1)
1-68: LGTM!.gitignore (1)
30-30: LGTM!Also applies to: 97-99
.markdownlint.json (1)
9-9: LGTM!Also applies to: 13-13
.pre-commit-config.yaml (1)
17-22: LGTM!Also applies to: 44-44, 415-427
renovate.json (1)
68-72: LGTM!Also applies to: 107-110
CLAUDE.md (1)
27-27: LGTM!Also applies to: 40-40
docs/getting_started.md (1)
37-51: LGTM!README.md (1)
137-137: LGTM!docs/api/layer.md (1)
25-25: LGTM!docs/decisions/0006-tiered-module-size-policy.md (1)
194-196: LGTM!docs/design/a2a-protocol.md (1)
8-8: LGTM!docs/guides/a2a-federation.md (1)
112-112: LGTM!docs/guides/approval-workflow.md (1)
101-101: LGTM!docs/guides/cost-attribution.md (1)
101-101: LGTM!docs/guides/custom-rules-and-meta-loop.md (1)
118-118: LGTM!docs/guides/dynamic-scoring.md (1)
130-130: LGTM!docs/guides/monitoring.md (1)
341-341: LGTM!docs/guides/rest-api-examples.md (1)
213-213: LGTM!docs/reference/comparison.md (1)
69-69: LGTM!Also applies to: 119-119, 169-169, 219-219, 267-267
docs/research/llm-provider-auth-survey.md (1)
17-17: LGTM!Also applies to: 313-313
tests/conformance/persistence/conftest.py (1)
324-332: LGTM!tests/conftest.py (1)
932-944: LGTM!Also applies to: 960-972, 982-985, 995-1027, 1032-1047
…syntax + assert idiom) Round-3 CodeRabbit review left 3 actionable items, all addressed: 1. (MAJOR) scripts/install_cli_tools.sh:245-248 -- single-attempt curl downloads are flaky. Confirmed by the lychee CI failure right after the round-3 push: 'curl: (22) The requested URL returned error: 502' from github.com's releases CDN. Adding --retry 3 --retry-delay 2 --retry-all-errors covers transient 5xx + curl-level errors (DNS, connection reset). Applied to both the archive and .sha256 downloads since both are subject to the same CDN flakiness. 2. (MINOR) scripts/install_cli_tools.sh:241 -- ShellCheck SC1072/SC1073: my disable directive used '--' as the comment separator, which ShellCheck's directive parser rejects (it expects '#' for the trailing comment, or no trailing text at all). Switched to '# shellcheck disable=SC2064 # early expansion intentional: ...' so the suppression parses correctly and the rationale is preserved. 3. (NIT) tests/unit/engine/workspace/git_backend/test_external_remote_backend.py:286 -- switched assert forge.create_repo.await_count == 1 to forge.create_repo.assert_awaited_once() to match the existing test at line 309 and give clearer failure messages from unittest.mock. CodeRabbit's previous 'critical' Python 2 except-syntax claim was not re-raised in this review (PEP 758 false positive correctly dropped after the model recalled its own learnings from PR #1760).
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai rate limit |
|
Your plan currently allows 10 reviews/hour. Refill in 3 minutes and 9 seconds. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/install_cli_tools.sh`:
- Around line 251-255: The curl calls unconditionally use --retry-all-errors
which breaks on older curl; add feature detection and a reusable variable (e.g.,
retry_args or retry_all) and use it in both curl invocations instead of the
literal flag. Detect support by checking curl help/version output for the string
"--retry-all-errors" (e.g., run curl --help 2>&1 | grep -q --
'--retry-all-errors') and set retry_args to "--retry-all-errors" when present or
to an empty string when absent; then update the two downloads/sha256 curl
command lines that reference "${tmpdir}/${archive}" and "${download_url}" to
include the variable (e.g., curl ... ${retry_args} ...). Ensure the detection
runs before the curl calls and remains POSIX-compatible.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fc97c5bf-281f-423e-87fc-05d23ce874f4
📒 Files selected for processing (2)
scripts/install_cli_tools.shtests/unit/engine/workspace/git_backend/test_external_remote_backend.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test markers: use
@pytest.mark.{unit,integration,e2e,slow}. Async tests useautomode. Global timeout 30s. Minimum coverage 80%. xdist:-n 8 --dist=loadfile(auto-applied via pyprojectaddopts; loadfile prevents 3.14+ Windows ProactorEventLoop leak). Windows: unit tests useWindowsSelectorEventLoopPolicy(3.14 IOCP teardown race; subprocess tests override back). See docs/reference/conventions.mdTest doubles: use ladder in docs/reference/conventions.md section 12.1.
FakeClockfor Clock seam fromtests._shared,mock_of[T](**overrides)for typed-boundary substitutions,SimpleNamespacefor attribute-bags. BareMagicMockat a typed boundary (constructor arg, function param, annotated local, typed fixture return) is blocked byscripts/check_mock_spec.py(zero-tolerance, no baseline)Vendor-agnostic test naming: NEVER use real vendor names in project code/tests. Use
example-provider,test-provider,example-{large,medium,small}-001. Real vendor names allowed only in.claude/, third-party imports,providers/presets.py,web/public/provider-logos/Hypothesis testing: 10 deterministic CI examples; failures are real bugs (fix + add
@example(...)). Flaky tests: NEVER skip/xfail; fix fundamentally. Useasyncio.Event().wait()instead ofsleep(large)
Files:
tests/unit/engine/workspace/git_backend/test_external_remote_backend.py
⚙️ CodeRabbit configuration file
Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare
@settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which@given() honors automatically.
Files:
tests/unit/engine/workspace/git_backend/test_external_remote_backend.py
scripts/**/*.sh
📄 CodeRabbit inference engine (CLAUDE.md)
Shell scripts: follow canonical rules in
~/.claude/rules/common/bash.mdforcd/git -C/ Bash file-write patterns
Files:
scripts/install_cli_tools.sh
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:46:20.835Z
Learning: Planning is MANDATORY: present every plan for accept/deny before coding
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:46:20.835Z
Learning: Convention PR rollout is MANDATORY: every convention PR must ship its enforcement gate (see [docs/reference/convention-gates.md](docs/reference/convention-gates.md))
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:46:20.835Z
Learning: Configuration precedence (MANDATORY): DB > env > code default via `SettingsService`/`ConfigResolver` (Cat-1) or env > code default (Cat-2, `read_only_post_init`); Cat-3 bootstrap secrets are pure env at boot site. YAML is a company-template ingestion format, not a precedence tier. No `os.environ.get` outside startup; pre-init Cat-2 reads use `settings.bootstrap_resolver.resolve_init_value`. See [docs/reference/configuration-precedence.md](docs/reference/configuration-precedence.md)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:46:20.835Z
Learning: Post-implementation + pre-PR review workflow (MANDATORY): after issue, create branch + commit + push (no auto-PR). Use `/pre-pr-review` (`gh pr create` is blocked by `scripts/check_no_pr_create.sh`). After PR approval, use `/aurelio-review-pr` for external feedback. Fix EVERYTHING valid; no deferring
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:46:20.835Z
Learning: Comments in code must explain WHY only; no reviewer citations, issue back-refs, or migration framing. Enforced by `check_no_review_origin_in_code.py` + `check_no_migration_framing.py`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:46:20.835Z
Learning: API startup lifecycle has two phases: construction (synchronous services in `create_app` body) and on_startup (persistence-dependent services in `_build_lifecycle.on_startup`). Construction-phase ordering: `agent_registry` before `auto_wire_meetings`; `tunnel_provider` unconditionally. On-startup ordering: `SettingsService` before `WorkflowExecutionObserver`; `OntologyService` after `persistence.connect()`; cost-dial services via `_try_wire_cost_dial` (best-effort, idempotent); knowledge substrate via `_wire_knowledge_engine` (best-effort, gated on persistence + memory backend); `EnvironmentService` behind persistence (threaded via `build_runtime_services`). See API startup section in CLAUDE.md
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T07:46:20.835Z
Learning: Git workflow: commits use `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci), enforced by Commitizen. Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`). Branches: `<type>/<slug>` from main. Squash merge with PR body as commit message; trailers (`Release-As`, `Closes `#N``) in PR body. After every squash merge → `/post-merge-cleanup`
📚 Learning: 2026-05-05T09:04:46.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.
Applied to files:
tests/unit/engine/workspace/git_backend/test_external_remote_backend.py
📚 Learning: 2026-05-21T22:55:20.496Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2035
File: src/synthorg/meta/toolsmith/models.py:114-114
Timestamp: 2026-05-21T22:55:20.496Z
Learning: In this repo’s “magic number” review standard, the existing gate in `scripts/check_no_magic_numbers.py` intentionally does NOT flag numeric literals used as raw call-site arguments. So, do not flag numeric literals passed as keyword arguments to Pydantic `Field()` (e.g., `Field(ge=0, le=100)` / `Field(ge=1, le=50)`)—this is an established idiom. Only treat numeric literals as “magic numbers” when they occur in the locations the gate checks (module-level assignments and function/method parameter defaults).
Applied to files:
tests/unit/engine/workspace/git_backend/test_external_remote_backend.py
📚 Learning: 2026-05-23T12:24:00.128Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2080
File: tests/_shared/test_postgres_proxy.py:19-48
Timestamp: 2026-05-23T12:24:00.128Z
Learning: When creating test doubles for Python typing.Protocols in tests, prefer a hand-written Protocol fake (a concrete class that explicitly implements the Protocol) over `mock_of[T]` if the Protocol only defines annotation-only attributes (e.g., `username: str`, `password: str`, `dbname: str`) with no class-level values/assignments. This is because `mock_of[T]` relies on `create_autospec(..., spec_set=True)`, which enumerates members via `dir(spec)`; annotation-only attributes are not included, so `mock_of`’s kwarg-based attribute setting can raise `AttributeError: attribute not present on spec type`. In that annotation-only case, don’t recommend `mock_of[T]`—use an explicit fake class instead.
Applied to files:
tests/unit/engine/workspace/git_backend/test_external_remote_backend.py
🔇 Additional comments (2)
tests/unit/engine/workspace/git_backend/test_external_remote_backend.py (1)
286-286: LGTM!scripts/install_cli_tools.sh (1)
241-242: LGTM!
Aureliolo
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
Stale: review was on commit 1f11e93 (round 1 first push). All inline findings (3 gemini-code-assist comments on renovate.json regex / GOPATH multi-entry / RETURN trap) were addressed in commit cd1ba00 (round 1) and validated by the subsequent CodeRabbit re-review on the new head. Dismissing to clear the reviewDecision noise; PR history is the source of truth for the actual fix commits.
…ss for CodeRabbit) Encodes the policy learned this round: CodeRabbit auto-clears its own stale CHANGES_REQUESTED on each push by submitting a new review on the new head, so manual dismissal of stale CR reviews is wasted work AND erases reviewer-thread context that humans use to trace the conversation. The carve-out is Gemini, which only reviews on PR open (no auto re-review without an explicit /gemini review trigger), so its CHANGES_REQUESTED is frozen on the head it first saw and IS the right thing to dismiss when the cited findings have been addressed in subsequent commits. Added a per-reviewer table covering CodeRabbit / Gemini / Other bots / Human reviewers with the re-review behaviour, auto-clear behaviour, and the correct action for each. Default rule for unknown reviewers: reply, don't dismiss. The dismissal-when-needed shell snippet now requires a message body that names the original head SHA, the findings, and the commits that addressed each one, plus a round-history entry. Surfaced during PR #2084 round 4 babysit: I dismissed three stale CodeRabbit CHANGES_REQUESTED reviews before realizing CR was going to clear them itself; the dismissal was inert (CR's auto-clear still works on the next review) but burned the audit trail.
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.claude/skills/babysit-pr/SKILL.md:
- Around line 490-491: Add a short "Verified as of <YYYY-MM-DD>" note with a
citation next to the Gemini row (the line containing "Gemini
(gemini-code-assist[bot])") stating that Gemini does not auto re-review and
requires an explicit "/gemini review" command; attach the two provided URLs as
the sources (developers.google.com/gemini-code-assist/docs/review-github-code
and developers.google.com/gemini-code-assist/docs/customize-repo-review) and
include the verification date to make the guidance time-boxed.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0bd26ede-3916-4c94-8d4e-5f8a8ab9d019
📒 Files selected for processing (1)
.claude/skills/babysit-pr/SKILL.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build Backend
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Site
- GitHub Check: Test Unit (shard 1)
- GitHub Check: Test Integration (shard 2)
- GitHub Check: Test Integration (shard 1)
- GitHub Check: Test Integration (shard 3)
- GitHub Check: Test Integration (shard 4)
- GitHub Check: Analyze (python)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: Read `docs/design/` page before implementing; deviations need approval (consult DESIGN_SPEC.md)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: Present every implementation plan for accept/deny before coding
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: No region/currency/locale privileged; use metric units; use British English (see docs/reference/regional-defaults.md)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: Every convention PR must ship its enforcement gate (see docs/reference/convention-gates.md)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: Test regression rule: timeout/slow failures indicate source-code regression; never edit `tests/baselines/unit_timing.json` or any `scripts/*_baseline.{txt,json}` / `scripts/_*_baseline.py`. Both families are PreToolUse-blocked. Bypass per-invocation with `ALLOW_BASELINE_GROWTH=1 git commit ...` (requires explicit user approval)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: After issue: branch + commit + push (no auto-PR); use `/pre-pr-review` (`gh pr create` blocked by `scripts/check_no_pr_create.sh`). After PR: use `/aurelio-review-pr` for external feedback. Fix EVERYTHING valid; no deferring
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: xdist: `-n 8 --dist=loadfile` auto-applied via pyproject `addopts` (`loadfile` prevents 3.14+Windows ProactorEventLoop leak)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: Postgres fixture (local): defaults to testcontainers-managed `postgres:18-alpine`. Bypass by exporting `SYNTHORG_TEST_POSTGRES_HOST` / `PORT` / `USER` / `PASSWORD` / `DB`. Fixture detects and yields connection-info proxy without testcontainers
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: Git commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: Branches: `<type>/<slug>` from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: Pre-commit/pre-push hooks: use `.pre-commit-config.yaml`. Tool-call gates: `.claude/settings.json` PreToolUse (`scripts/check_*.sh`/`.py`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: Squash merge. PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) must be in PR body
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: GitHub queries: use `gh issue list` via Bash, NOT MCP `list_issues`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: After every squash merge → run `/post-merge-cleanup`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-24T08:07:37.479Z
Learning: CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API. See `cli/CLAUDE.md` (use `go -C cli`, never `cd cli`)
<!-- HIGHLIGHTS_START --> ## Highlights > _AI-generated summary (model: `openai/gpt-4.1-mini` via GitHub Models). Commit-based changelog below._ ### What you'll notice - New brownfield codebase intake mode supports merger and acquisition scenarios. - Added deep CEO interview feature to improve project charter creation. - Introduced mission control and flight recorder operator cockpit for better operational oversight. - Research mode added for enhanced exploratory work. - Runtime services now log safety-spine state at boot for clearer diagnostics. ### What's new - Research mode feature enables deeper data exploration. - CEO interview integration helps shape project charters. - Mission control and flight recorder cockpit introduced for operational tracking. ### Under the hood - Improved codebase modularity with module-size gates and lint tightening. - Added __init__.py to 21 test directories for better test discovery. - Promoted six transitive dependencies to direct dependencies for clarity. - Split codespell ignore list into vocabulary and source renames. - Decomposed oversized web utilities, hooks, and libraries for maintainability. - Enhanced CI with Lychee link checker integration and retry logic for cosign signing. - Sharded unit and integration tests and added Postgres service container in CI. - Updated infrastructure and web dependencies; maintained lock files. <!-- HIGHLIGHTS_END --> :robot: I have created a release *beep* *boop* --- ## [0.8.8](v0.8.7...v0.8.8) (2026-05-24) ### Features * brownfield codebase intake (merger/acquisition entry mode) ([#2042](#2042)) ([e287621](e287621)), closes [#1975](#1975) * deep CEO interview to project charter ([#2045](#2045)) ([904f2fb](904f2fb)) * mission control + flight recorder operator cockpit ([#2044](#2044)) ([1c2660b](1c2660b)) * research mode ([#2041](#2041)) ([f81a5ac](f81a5ac)), closes [#1989](#1989) * surface safety-spine state in runtime-services boot log (closes [#2096](#2096)) ([#2097](#2097)) ([f187b31](f187b31)) ### Refactoring * add __init__.py to 21 leaf test directories (INP001) ([#2081](#2081)) ([2592118](2592118)), closes [#2064](#2064) * codebase modularity (1/4) - module-size gates + lint tightening + tools ([#2078](#2078)) ([556fbd9](556fbd9)), closes [#2047](#2047) [#2040](#2040) * promote 6 transitive deps to direct deps ([#2083](#2083)) ([adedc6a](adedc6a)) * split codespell ignore-words-list into vocab + source renames ([#2085](#2085)) ([917d98a](917d98a)), closes [#2074](#2074) * **web:** PR A foundation, decompose oversized utils/hooks/lib ([#2092](#2092)) ([#2098](#2098)) ([aedbba5](aedbba5)) ### CI/CD * exclude slsa.dev from lychee (transient timeout on canonical badge) ([#2090](#2090)) ([346c51d](346c51d)) * fix paths-filter shallow-clone race and scorecard allowlist ([#2089](#2089)) ([7cd7ce8](7cd7ce8)) * refresh .test_durations.{unit,integration} ([#2087](#2087)) ([ddf2d86](ddf2d86)) * retry cosign sign on transient GHCR/Rekor failures ([#2100](#2100)) ([da9422a](da9422a)) * shard test-unit + test-integration, sysmon coverage, Postgres service container ([#2080](#2080)) ([0768787](0768787)) * wire Lychee link-checker (workflow + installer + pre-push hook) ([#2084](#2084)) ([1c0694a](1c0694a)) ### Maintenance * Lock file maintenance ([#2086](#2086)) ([a78810a](a78810a)) * Update Infrastructure dependencies ([#2055](#2055)) ([041ad8b](041ad8b)) * Update Web dependencies ([#2054](#2054)) ([4d57b9a](4d57b9a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: synthorg-repo-bot[bot] <279117679+synthorg-repo-bot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
Closes #2070.
Summary
PR #2047 attempted to wire Lychee via the upstream
lycheeverse/lycheepre-commit-hook repo, but pre-commit-uv does not bundle the Rust binary; the hook landed commented-out and no CI workflow shipped. This PR closes that exemption (ADR-0006 §F) with three complementary pieces:scripts/install_cli_tools.shnow installs the lychee binary alongsidegolangci-lint. Platform-aware (Linux x86_64/aarch64, macOS x86_64/arm64, Windows x86_64 via Git Bash / MSYS / Cygwin), downloads the prebuilt binary from GitHub Releases, verifies SHA256 against the upstream.sha256file (handles both GNU and WindowsCertUtillayouts), installs to$HOME/.local/bin. Wrapped in symmetricinstall_golangci_lint+install_lycheefunctions so future tools can be appended cleanly..github/workflows/lychee.ymlrunslycheeverse/lychee-action@v2.8.0(SHA-pinned) againstREADME.md,CLAUDE.md,cli/CLAUDE.md,web/CLAUDE.md, anddocs/**/*.mdon PRs tomainand pushes tomain.permissions: {}top-level +contents: readjob-level. Cache viaactions/cache@v5.0.5. Strict-preset config (accept = ["200"])..pre-commit-config.yamlis replaced with alocallycheehook at the pre-push stage that shells out to the installed binary. Added toci.skip(cloud runner doesn't have the binary).Shared
lychee.tomlat the repo root carries the config for both surfaces (cache, network tuning, exclusions for loopback hosts, anti-bot 403s, 429 rate-limit hosts, and build-timefile://artefacts underdocs/openapi/).Renovate gains a packageRules entry for
lycheeverse/lychee(strips the upstreamlychee-v...tag prefix), and the "Binary tool version env vars" custom regex manager now scansscripts/install_*.shso both pin sites stay in sync.13 markdown files updated to fix actual link rot surfaced by the new check: design-doc reference repairs (
a2a.md→a2a-protocol.md,cost-control.md→budget.md, etc.), URL drift (agent-protocol.ai→a2a-protocol.org, Llama Stack→OGX rebrand, Logfire docs move, DashScope→Alibaba Model Studio, README's REST API path), plus removal of pointers to docs that never existed.ADR-0006 ledger updated to remove the lychee exemption (Section F, row 334) and to move lychee from "Deferred" to "Landed" in the New docs/SQL/YAML tools section.
Test plan
bash scripts/install_cli_tools.shinstalls both binaries clean on a fresh machine and is idempotent on re-run (verified locally on Windows / Git Bash).lychee --config lychee.toml README.md CLAUDE.md cli/CLAUDE.md web/CLAUDE.md "docs/**/*.md"exits 0.uv run pre-commit run lychee --hook-stage pre-push --all-filespasses.uv run pre-commit run actionlint --all-files,... zizmor --all-files,... yamllint --all-filesall clean..pyfiles changed; ruff/markdownlint/codespell/yamllint all green on the diff).Review coverage
Pre-reviewed by 5 agents in parallel (infra-reviewer, docs-consistency, comment-quality-rot, issue-resolution-verifier, code-reviewer); 6 findings addressed in the follow-up commit:
CLAUDE.md:27"Quick Commands" comment said "golangci-lint only".gitignore:98carried# (hand-off artefact for issue #1417...)lycheeQuick Commands entryuv run pre-commit run lychee --hook-stage pre-push --all-filesscripts/install_cli_tools.shlychee block used top-levelexit(broke symmetry with golangci-lint function-wrap)install_lychee(); switchedexittoreturn; trap onRETURNissue-resolution-verifier: 6/6 acceptance criteria RESOLVED.infra-reviewer: PASS, 0 findings.Notes
lycheeverse/lychee-action@*viagh api -X PUT repos/Aureliolo/synthorg/actions/permissions/selected-actions..lycheecacheadded to.gitignore(runtime cache file)..markdownlint.jsonMD025 set withfront_matter_title: ""so markdownlint stops misreading the project's pre-existing convention (YAML front-mattertitle:+ in-body# Heading) as two top-level headings; the convention itself is older than this PR.