Repository navigation
[docs] - Align the loop and persona skills with the commit hook and verification policy - #1642
Conversation
…erification policy The four *-refactor loops, add-comment, karpathy-guidelines, CyClaw-Optimize and babysit-github-pr told agents to commit with `refactor:`/`perf:`/ `test(fix):`/`log(...)`/`docs(...)`/`ci:` subjects, all of which the tracked commit-msg hook refuses, and to run the full `pytest tests/` (or `--cov`) suite as a routine step, which CLAUDE.md §5 reserves for CI and the owner. Both are now stated once per skill, with the hook-compliant subject form and the §5 verification order (run the code, lint, let Actions run the suites, one targeted file at most). tests-refactor and logging-refactor keep their whole-suite measurement loop but say plainly that an explicit invocation is the owner's authorization. Also removed or corrected: the "feature freeze (CLAUDE.md §1)" references (no such section), the fictional conftest fixture checklist (`utils.rate_limit`, `RateLimiter._store`, a real-index session fixture), `grep -E "\d"` metric extraction (GNU grep has no \d) and the doubled `-q` that hides pytest's summary line, the unauthenticated `/soul` timing in speed-refactor, the retired `harness/` sweep target and the old two-header template-check description in CyClaw-Optimize, the `agent/` branch prefix where `claude/` is the documented one, and the babysit claims that its scripts fix comments, query run history, or prompt for a test command. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LbrgG4NUbR3sUTgNMfKXpU
|
PR is a complete fill of |
…d-refactor tests/test_gate.py::TestProxyHeaderTrust requires every copy-paste start recipe in the skill docs to name uvicorn gate:app --host with --no-proxy-headers; the driver.sh serve replacement dropped that line and failed the ci (3.12) and ubuntu-22.04 legs on 855fee4. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LbrgG4NUbR3sUTgNMfKXpU
…c strategies and restated rules Collapses the four refactor loops' identical tracker-init heredocs and decision trees into one-liners, drops the fictional tracker and report examples, removes the Mobile/DB-migration/terraform strategies from verification-specialist in favour of CyClaw's own surfaces, and cuts the CyClaw-Optimize, babysit and add-comment sections that restated Step 3.5, CLAUDE.md §3/§4 or an earlier step of the same file. 59 insertions, 248 deletions; no guidance removed that is not stated once elsewhere in the file or in CLAUDE.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LbrgG4NUbR3sUTgNMfKXpU
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3646a2ad2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| GROK_API_KEY=dummy python3 -m cProfile -s cumulative -c \ | ||
| "from fastapi.testclient import TestClient; import gate; TestClient(gate.app).get('/health')" 2>&1 | head -40 |
There was a problem hiding this comment.
When the speed-refactor loop reaches per-request profiling, this command exits before importing gate: python3 -m cProfile --help accepts [-m module | scriptfile], and executing the documented command on Python 3.12 returns exit 2 with no such option: -c. Use a temporary script, cProfile.runctx, or another supported invocation so the prescribed profiling step can actually run.
Useful? React with 👍 / 👎.
| new session-scoped ones: `client` (a function-scoped `TestClient` over the | ||
| mocked gateway, one loopback IP per test file so rate-limit state cannot | ||
| leak), `test_config` (a deepcopy — `test_conftest_fixtures` guards that), and | ||
| the autouse mocks for the embedder, Ollama, Grok and Claude. Nothing builds a | ||
| real index and nothing starts a live service (CLAUDE.md §6 "Test"). |
There was a problem hiding this comment.
When an agent adds another test file following this guidance, client does not allocate one peer per file: tests/conftest.py:517-526 defaults every direct use to ("127.0.0.1", 51234), and its own docstring requires consumers to choose distinct peers because the rate limiter is process-global. The service patches are also scoped to _mocked_gateway/client, not autouse mocks. Promising this isolation can therefore create order-dependent 429s or tests that unexpectedly reach real dependencies; document the required peer parametrization and explicit mocks instead.
Useful? React with 👍 / 👎.
| Follow CLAUDE.md §5's verification order: run the code you moved (import the | ||
| new module, call the extracted function, or hit the endpoint with | ||
| `driver.sh serve` + `probe`), then lint, and let GitHub Actions run the full | ||
| suite on the pushed draft PR. When a step cannot be exercised that way, run the | ||
| one test file that covers it: |
There was a problem hiding this comment.
On an architecture-refactor run with only local commits, this workflow has no push or draft-PR creation step: after verification it commits and loops back to assessment. It nevertheless defers the suite to Actions here and later requires green CI on the pushed head before stopping, so the loop cannot satisfy its own completion condition. Add an explicit push/draft/CI phase, or defer the CI criterion until a separate publication phase.
AGENTS.md reference: AGENTS.md:L49-L49
Useful? React with 👍 / 👎.
| # Find except blocks with no logging | ||
| grep -n "except" **/*.py | grep -v "log\|logger\|print\|LOG" | ||
| # Find except blocks with no logging (rg recurses; bash's ** needs globstar) | ||
| rg -n "^\s*except\b" --type py -A2 . | rg -v "log|logger|LOG|raise" |
There was a problem hiding this comment.
When the logging audit runs this pipeline, the second rg filters individual output lines rather than whole exception blocks: every except line survives because it lacks log, while a following logger.* line is merely removed. For example, the already-logged handler at gate.py:464-465 is still reported, so this scan identifies most exception handlers as gaps and can prompt duplicate logging. Group each context block before testing it for logging, or use a small AST-aware script.
Useful? React with 👍 / 👎.
| - **Data / ML:** Run a sample input through the pipeline. Verify output shape, schema, and value ranges. Test with empty inputs, null values, NaN, and confirm row or record counts. | ||
| - **Database migrations:** Run the migration up. Verify the resulting schema matches expectations. Run the migration down. Test against existing seed or fixture data. | ||
| - **Refactoring:** The existing test suite must pass without modification. Diff the public API surface to confirm nothing was unintentionally changed. Spot-check key behaviors end-to-end. | ||
| - **Backend / API (gate.py, the route modules):** Start the server (`driver.sh serve`). Curl each relevant endpoint. Inspect response status codes, headers, and body shapes. Deliberately send bad input to exercise error-handling paths. |
There was a problem hiding this comment.
For a backend verification task, driver.sh serve conflicts with this skill's strict prohibition on modifying project files: cmd_serve runs mkdir -p logs, starts gate.py with the repository config, and the subsequent probe can write application and audit logs in the checkout. Use disposable sink/personality/index paths or an isolated checkout before prescribing this live flow.
AGENTS.md reference: AGENTS.md:L49-L49
Useful? React with 👍 / 👎.
| | Pass rate | 62% | 100% | | ||
| | Coverage | 41% | 100% | | ||
| | Tests total | 18 | 47 | | ||
| | Pass rate | 98.1% | 100% | |
There was a problem hiding this comment.
When this loop reaches its stated stopping condition, coverage must equal 100%, but the newly updated Final State example records only 91% while presenting the run as completed. That contradictory template can lead an agent to stop and report success before the loop's mandatory target is met; either retain 100% as the final example or label sub-100% output as an incomplete/exhausted run.
Useful? React with 👍 / 👎.
| `--cov=.`, and keep `-o addopts=""` so the doubled `-q` does not hide the | ||
| summary line (see `tests-refactor`'s Measurement Protocol for both). Extract | ||
| the metrics with the same Python snippet `tests-refactor` uses; GNU `grep -E` | ||
| does not understand `\d`. |
There was a problem hiding this comment.
When /logging-refactor reaches its required measurement step, this section no longer contains any command that extracts PASSED, FAILED, ERRORS, PASS_RATE, or COVERAGE; it only tells the agent to find a Python snippet in another skill. That makes this loop non-standalone and leaves its decision tree without the metrics it requires if the other skill is unavailable or changes. Keep the corrected extraction snippet here, or move it into a shared executable both skills invoke.
Useful? React with 👍 / 👎.
| 4. Run linters and type-checkers if the project has them configured. | ||
| 5. Check for regressions in areas adjacent to the change. | ||
|
|
||
| In CyClaw specifically (CLAUDE.md §5 and §8): there is no build step; "the |
There was a problem hiding this comment.
For packaging or manifest changes, this CyClaw-specific override causes the specialist to skip a real build even though the project has one: .github/workflows/ci.yml:341 runs python -m build --wheel, and .github/workflows/python-publish.yml:55 runs python -m build. This contradicts the universal build check immediately above and can miss broken wheel metadata or package contents; remove the blanket “no build step” exception or scope it only to changes that cannot affect packaging.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
| ruff check --select E,F,I,B,C4,UP,S --ignore E501 <touched files> | ||
| python3 -c "import <touched.module>" # a stray quote breaks here, not in CI | ||
| python3 .claude/skills/invariant-guard/check_invariants.py |
There was a problem hiding this comment.
When this skill comments a permitted non-Python target such as config.yaml, a shell launcher, PowerShell file, or frontend asset, import <touched.module> is not a runnable verification command and cannot detect malformed syntax in that file. Even for Python scripts that are not importable modules, the prescribed check fails for the wrong reason. Select validation by file type—such as py_compile, YAML parsing, bash -n, or the appropriate frontend checker—instead of requiring every touched file to be importable.
Useful? React with 👍 / 👎.
2 participants
Proposed changes
Skill-catalog resync, part 1 of 3 (parts 2 and 3, #1643 and #1645, are merged; all three touch disjoint files). This PR reconciles the four
*-refactorloops,add-comment,karpathy-guidelines,verification-specialist,CyClaw-Optimizeandbabysit-github-prwith what the repo enforces today.Every SKILL.md was read end to end and each concrete claim checked against the code,
config.yaml, the hooks and the workflows. What changed:refactor:,perf:,test(fix):,log(coverage):,docs(retrieval):andci:. The tracked.githooks/commit-msgaccepts only[prefix] - Short descriptive sentence. Each skill now shows the hook-compliant form once and says why.--covruns are replaced by the §5 order: run the changed code, lint, let GitHub Actions run the suites, one targeted test file at most.tests-refactorandlogging-refactorkeep their whole-suite measurement loop but state that an explicit/tests-refactoris the owner's authorization. Their metric extraction is fixed (grep -E "\d"matches nothing in GNU grep; the doubled-qon top ofpyproject'saddoptsbecomes-qqand hides theN passedline).CLAUDE.md §4cited for rules that live in §5/§6; the fictional conftest checklist intests-refactor(utils.rate_limit,RateLimiter._store, a session fixture that builds a real index);RateLimiter.check()(the method isallow());logging.getLogger(__name__)+extra={}presented as the house pattern (CLAUDE.md §5 sayscyclaw.<module>+ lazy%s);hash(query)as a query identifier (the repo uses the keyed HMACquery_fingerprint); theharness/sweep target removed by [infra] - Remove the Python coding-harness console; CG-agent-harness owns that role #1367;agent/as the branch prefix for Claude Code; the two-header description ofpr-template-check.yml(it now requires all eight sections, the stamp line and the title format)./soul, no longer profiles by runninggate.py's__main__, and states that the 50 ms gate cannot apply to a live 27B generation path. The hand-runuvicorn gate:app --host … --no-proxy-headersrecipe thattests/test_gate.py::TestProxyHeaderTrustpins stays (the first push dropped it and failed theci (3.12)andubuntu-22.04legs; 019bce7 restored it).ALLOW_FORCE_WITH_LEASEis the operator's hook override and never set by an agent, that a human merges in this repo, and records the two helper-script gaps found (gh pr checks --json name,statusis rejected by currentgh;--repo .is not a valid repo). Those script fixes are left for a follow-up becausegh pr checkscannot be exercised from this sandbox (GraphQL is blocked).architecture-refactor.mdno longer names a nonexistent "autoreview" tool;verification-specialist.mdno longer calls the full suite "the lighter-weight check").verification-specialistlists CyClaw's own surfaces instead of Mobile/DB-migration/terraform strategies;CyClaw-Optimize,babysitandadd-commentlose sections that restated Step 3.5, CLAUDE.md §3/§4 or an earlier step of the same file.Net: 416 insertions, 622 deletions across 12 files.
Invariant / Governance Impact: none of the six invariants or I6 is touched; no code outside
.claude/changes. The agent instruction surface (.claude/skills,.claude/commands) is what changed, which the pre-commit security gate flagged as a reminder; every edit is reproduced in the diff for line-by-line review.Types of changes
Scope note: Docs + audits (agent skill files only).
Benefits / why
tests-refactorconftest checklist alone described three fixtures that have never existed.Risks to monitor
driver.sh serve/probefor live checks; ifcyclaw-gotchas/driver.shchanges its subcommands, three skills drift together (doc-sync does not parse skill prose).babysit-github-pr's script gaps are documented, not fixed; the scripts remain scaffolding until a follow-up corrects theghcalls.tests-refactor/logging-refactorexemption from §5 rests on "user-invoked only"; if either ever losesdisable-model-invocation: true, the exemption text must go with it.Checklist
docs/THREAT_MODEL.md,INVARIANTS.mdandSECURITY.md(and any relevant Phase docs)doc_sync.py(0 drift),babysit-github-pr/verify.sh(pass),bash -non the edited bootstrap script, the new metric/median shell snippets and the tracker one-liner executed against sample input, and thetest_gaterecipe rule replicated against both skill files.Further comments
Verification against the repo was done by five read-only subagents (one per skill group), each finding re-checked by hand before editing. Findings that were out of scope for a docs PR and are not addressed here: the
run-babysit.sh/pr-state.sh/wait-for-checks.shghinvocation bugs;tests-refactor's 100% coverage aspiration (left as the loop's stated goal, with the realfail_under = 80gate named beside it).Suggested merge order of open PRs
Trial merge verified (
git merge --no-commit --no-ff+ abort) against main @ 6902d39 on 2026-10-13 (after #1637, #1638, #1640, #1643 and #1645 merged). This PR touches only loop/persona skill files under.claude/and overlaps none of the other open PRs.#1641 · Safe ✅
Impact: CI bwrap/AppArmor workflow; independent.
Action: merge anytime.
#1642 (this PR) · Safe ✅
Impact: independent; loop/persona skill files only.
Action: merge anytime; re-run the trial merge if main moves.
#1644 · Safe ✅
Impact: CI bwrap/AppArmor selection; independent of this PR.
Action: merge anytime.
#1646 · Safe ✅
Impact: otel-hardening inventory; independent of this PR.
Action: merge anytime.
ELI5
The repo's "how to work here" cheat-sheets for the refactor loops and a few helper personas had gone out of date: they told an agent to write commit messages the repo's own gatekeeper script rejects, to run the whole 5,000-test suite after every small step even though the owner decided the CI robots should do that, and to look for fixtures and files that don't exist. This PR rewrites those sentences to match what the repo actually enforces today, deletes the made-up bits, and then trims the four loop sheets' copy-pasted setup boilerplate to one line each. Nothing in the running server changes. What could still go wrong: the sheets now point at the
driver.shhelper for live checks, so if that helper is renamed, these files go stale again; the signal isdoc-syncstaying green while a skill quietly names a command that no longer exists, so keep the manual pass in the loop.🤖 Generated with Claude Code
https://claude.ai/code/session_01LbrgG4NUbR3sUTgNMfKXpU
Generated by Claude Code
Last updated: 2026-10-13 05:56 ET