Repository navigation
harness: autonomous improvement loop and decisions.yaml lifecycle (design, off until founder approval on #43) - #70
BotshareAI wants to merge 2 commits into
Conversation
Harness: policy (tiers A to D, protected paths, blocked commands, budgets, schedule, missions, escalation, enrolment of 15 repositories, 4 excluded by owner decision, audits private), baseline with the register-documents sync gate, 13 always-loaded core rules (SessionStart hook in CI and in the plugin), 16 skills (also a Claude Code plugin), 12 missions, 14 behaviour evaluations for `claude plugin eval` (14 of 14 at 3 of 3 runs), Claude Code settings and a fail-closed PreToolUse hook, deterministic selection, inputs, guard (recomputed in the publish job from the bundle), output allowlist with secret scrubbing, publish, review queue and stage 4 merge gate; design workflows under rollout/. Decisions lifecycle: Decisions impact section in the PR template and in check_pr_evidence.py (--decisions, --files-json; warning until --require-decisions-impact), decision_impact.py, decision_followup.py, register-sync mission (platform-lead-approved requests only), weekly register audit in the private audits repository. REG audit prefix owned by the platform lead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZnPEN4YoUKzMSoLmaZCAS Claude-Session: https://claude.ai/code/session_01DZnPEPJrZ6MioobL2aYbRm Signed-off-by: Alex Reznichenko <info@botshare.ai>
|
Tracking issue for the "Decisions impact rule warns until each repository requires it" item under Not verified: #71 Generated by Claude Code |
Review of #70 (adapt): - Dry run: with HARNESS_PUBLISH unset, a run started by hand runs any mission whatever HARNESS_STAGE is and publishes nothing; scheduled AI runs are skipped. Stage gates apply only when publishing. Policy gains stage 0. Publish steps need HARNESS_PUBLISH=true and the route's dry_run=false. The route step is now executed by tests. - Core rule 5: one approved contract change request drives one PR per affected repository, merged in order (interface, consumers, removal). - Core rule 12 and the digest: excluded repositories are named as excluded (out of scope), never as clean or unscanned. - Core rule 13: the revert test applies to code changes; read-only work has its own evidence. - README: federation credentials are short-lived; an API key is spend-limited but long-lived. - Evaluations rerun with claude-opus-5-5: 14 of 14 at 3 of 3 runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZnPEN4YoUKzMSoLmaZCAS Signed-off-by: Alex Reznichenko <info@botshare.ai>
panthera-momagdii
left a comment
panthera-momagdii
left a comment
There was a problem hiding this comment.
Reviewed head 50ff826, focusing on policy.yaml, CORE_RULES.md, guard_tool.py, harness_guard.py, harness_publish.py and the publish job. Also checked the merge gate, relevant test coverage and the check_pr_evidence.py diff.
The design-only rollout and separate founder approval on #43 must stay unchanged. This review does not authorize activation.
Requesting changes for three reproducible gaps in the final guard/publish boundary:
- Git-quoted filenames bypass the protected-path classification.
- Removing an existing assertion still passes as tier A, despite the tier-A no-test-line-removal condition.
- A secret-like value committed and then removed is missed by the net-diff scan, although the original commits remain in the bundle and branch that get published.
Inline comments include reproduction details and suggested regression coverage. Please fix these before approval and rerun the guard/publisher tests, including negative cases.
Validation performed: isolated local Git fixtures using byte-verified copies of harness_guard.py and harness_lib.py from this head, with the corresponding path-tier, dependency, mission-exception and safety-path policy entries copied into a test fixture. A safe new test passed and an ordinary protected harness file failed, as expected. All three unsafe fixtures unexpectedly returned PASS. Synthetic secret-pattern text only; no real credentials and no GitHub pushes were used in these tests.
The Repository quality workflow is green. The full upstream 353-test suite and live workflow/agent execution were NOT RUN in this review. No activation, variable changes or merges performed. AI-assisted review.
| changes = [l.split("\t") for l in git(root, "diff", "--name-status", "--no-renames", | ||
| f"{args.base}..HEAD").splitlines() if l] |
There was a problem hiding this comment.
[P1] Parse changed paths with NUL delimiters before applying policy
git diff --name-status quotes paths containing tabs, newlines and, by default, non-ASCII bytes. Splitting the display output leaves the surrounding quotes and escapes in path, so tier_of() is not checking the real pathname. Reproduced with a signed commit adding harness/probe\t.md (a literal tab in the filename): the actual path is tier D, but the guard returns PASS / tier B with files containing the quoted representation. This defeats the publish job's protected-path check even though an ordinary harness/probe.md is correctly rejected.
Use --name-status -z and parse the NUL-delimited status/path records without display quoting. Make diff/stat parsing consistent, or reject unsupported path characters explicitly. Add regression cases for tab, newline and non-ASCII names under protected directories.
| removed_tests |= test_names(removed) | ||
| added_tests |= test_names(added) | ||
| lost = sorted(removed_tests - added_tests) | ||
| if lost: | ||
| failures.append(f"test functions removed: {', '.join(lost[:10])}") |
There was a problem hiding this comment.
[P1] Enforce the tier-A restriction on removing existing test lines
Only removed test function names are rejected here. Removing the sole assert from an existing test while keeping its def test_... and a harmless assignment returns PASS / tier A; supplying unchanged before/after executed-test counts also passes. I reproduced that exact case. This violates policy.yaml tier A's condition that no line is removed from an existing test except a rename/move. harness_merge_gate.decide() rechecks file tiers, not this diff condition, so the deterministic gate still treats the weakened test as eligible for stage-4 automatic merge if the other gates pass.
Reject assertion/test weakening and enforce the stated removal rule. At minimum, changes that cannot be demonstrated to be pure renames/moves must not retain tier-A automatic-merge eligibility. Add a regression test that removes an assertion without removing or renaming the test function.
| for line in added: | ||
| if SKIP.search(line) and not ISSUE_REF.search(line): | ||
| failures.append(f"{path}: skip or xfail without a tracking issue on the same line: {line.strip()[:120]}") | ||
| if SECRET.search(line): | ||
| failures.append(f"{path}: secret-like string added") |
There was a problem hiding this comment.
[P1] Validate all history that will be published, not only the final tree diff
per_file comes from the net base..HEAD diff, while the later per-commit loop only checks DCO sign-off. The workflow bundles the original commit range and Publisher.push_bundle() pushes the candidate branch without squashing. Reproduced: commit synthetic text that matches this SECRET regex, then remove it in a second signed commit and add a harmless test. The guard returns PASS / tier A, but the synthetic value is still reachable in the bundle's commit history. The same gap applies to protected-path edits that are reverted before HEAD.
Before publishing, scan every newly reachable commit/blob that will be transferred for secrets and forbidden changes, or reconstruct a fresh commit from the validated net change on the trusted base and publish only that sanitized history. Include an add-then-remove secret fixture and an edit-then-revert protected-path fixture. No real credential was used in this reproduction.
panthera-momagdii
left a comment
panthera-momagdii
left a comment
There was a problem hiding this comment.
Focused security/governance review of #70 at head 50ff826, covering harness/policy.yaml, CORE_RULES.md, the PreToolUse hook, tools/harness_guard.py, tools/harness_publish.py, the publish job in rollout/workflows/harness-run.yml, and the stage-4 merge gate.
[P2] Enforce the selected task's tier ceiling in the deterministic publish guard. harness_select.py sets a selected issue's tier to A or B and harness/missions/build.md calls it a ceiling. The publish job passes it as --max-tier. However, in harness_guard.py, if TIER_ORDER[tier] > TIER_ORDER[args.max_tier] only appends a note, not a failure: a selected tier-A issue can produce a tier-B source change, get guard PASS, and be published as a draft PR. The stage-4 merge gate independently rechecks file tiers, which prevents this from automatically merging, but it does not enforce the earlier one-issue/one-scope authoring boundary. Please add a guard failure when the actual tier exceeds --max-tier, and regression tests for A->B rejection and permitted A->A/B->B. Keep the human escalation path instead of silently upgrading the task scope.
Other boundaries inspected: tier C/D path matching, separate publish-job guard and bundle-base check, bot-author review-marker checks, and stage-4 file-tier recheck. No workflows were activated. I did NOT run the 353-test suite, hook evaluations, supervised dry run, GitHub publish, or hardware; this is source-derived and needs verification before activation. Keep #70 draft, and preserve founder approval on .github issue #43 plus a distinct activation PR. AI-assisted review.
|
Review fixes pushed in 54bdd2b. All five points accepted.
Behaviour evaluations rerun after the rule changes, model claude-opus-5-5: 14 of 14 at 3 of 3 runs ( |
panthera-momagdii
left a comment
panthera-momagdii
left a comment
There was a problem hiding this comment.
Follow-up review of the new head 54bdd2bb253df881901c3daf626acdf1034043a4 against previously reviewed 50ff82690ea3a0987bd00712177c8fb9a554acd9. COMMENT only, not approval or authorization to activate.
The incremental commit addresses the separate governance/dry-run feedback: the manual route now reports dry_run, allows supervised triage/build/review independently of publishing stage, and both publish paths require HARNESS_PUBLISH=true and dry_run=false before --apply. I inspected the changed workflow, policy.yaml, core rules, publish code, route tests, the current hook/guard/merge gate, the shared AGENTS.md rules, and prior review threads. The hosted Repository quality quality/test job 124097061558 passed 362 tests. NOT RUN locally: harness tests, live dry runs, publishing, or activation. The described behaviour is source/CI evidence, not a live deployment test.
The prior security change requests are not resolved by this commit. tools/harness_guard.py is unchanged: the earlier three reproduced P1 findings (Git-quoted protected paths; deletion of an existing assertion still qualifying as tier A; secret/protected edits hidden in earlier commits of the published history) and the P2 tier-ceiling note-versus-failure finding still apply. I am not duplicating their inline reproductions. Please address those checks and add regression coverage before security acceptance.
The AGENTS.md v2 contract wording versus the new one-PR-per-repository core rule is explicitly deferred by the author to shared-rules v3; keep that owner-led reconciliation visible. This is still a design-only draft. No workflow activation without a separate activation PR and Alex's explicit approval on #43, and no automatic-merge or safety acceptance implied.
wikki26
left a comment
wikki26
left a comment
There was a problem hiding this comment.
[P1] Require executed successful verification before stage-4 automatic merge.
Reviewed head 54bdd2b. tools/harness_merge_gate.py accepts success, neutral and skipped for every check, and its only nonempty requirement is that check_runs contains an object. I ran decide() with an otherwise eligible tier-A PR, a current-head bot PASS marker, an expired veto window, and exactly one completed/skipped check. It returns [] (no rejection reasons), so the candidate is eligible even though no check succeeded or verified anything.
Please require the repository's authoritative verification checks to execute and succeed, while permitting genuinely optional skipped jobs separately. Add all-skipped, all-neutral, missing-required-verifier, and mixed required-success/optional-skip regression cases. The merge path should also inspect the applicable commit statuses/required contexts; its current check-runs-only read is not independent evidence of CLA or other status-based gates. GitHub rulesets remain another boundary, not a substitute for the stated deterministic gate.
Validation: python3 -m pytest -q tests/test_harness.py tests/test_harness_publish.py tests/test_harness_decisions.py: 84 passed on this head. Standalone decide() reproduction returned []. No merges, publishing, dry-run activation or robot execution. The prior quoted-path, weakened-assertion, hidden-history and tier-ceiling requests remain unresolved in the unchanged guard. Keep design-only/draft and preserve separate founder authorization on #43. OpenAI ChatGPT/Codex-assisted review.
wikki26
left a comment
wikki26
left a comment
There was a problem hiding this comment.
CI/security review for head 54bdd2b (requested by Alex for 2 Nov).
Changes requested — do not activate or treat the green 362-test run as security sign-off. The staged design and separate founder approval on #43 are appropriate, but deterministic guard/publish enforcement still has four release-blocking gaps at this exact head:
- Protect actual Git filenames, not quoted display strings (
tools/harness_guard.py:56-69,82-93).git diff --name-status(and patch headers) quote tabs/newlines. The currentsplit("\\t")/header parsing can tier a protectedharness/filename as B. Use NUL-delimited raw filenames and path-scoped diff/stat parsing; add tab/newline/non-ASCII protected-path regressions. - Never auto-publish a weakened existing test as tier A (
harness_guard.py:117-132). Existing code checks whether thetest_*name disappears, not whether its assertions or test lines are removed. Make deletion of a line from an existing test at least tier B / non-auto-merge (or reject it), and add a fixture removing only the sole assertion. - Scan all commits actually pushed (
harness_guard.py:117-147,harness_publish.py:201-212). The netbase..HEADscan misses secrets or protected-path edits introduced and then reverted in separate signed commits; publishing the original bundle preserves those objects. Reject unsafe intermediate commits/blobs, or publish only a newly constructed sanitized commit from the independently validated final tree. Test both add/remove synthetic secret and protected-edit/revert fixtures. - Enforce the selected issue tier ceiling (
harness_guard.py:106-115). Exceeding--max-tiermerely appends a note while returning PASS; the publisher subsequently creates the draft PR. Fail the guard when the final actual tier (including dependency-manifest promotion) exceeds the ceiling. Test A→B rejection, A→A and B→B acceptance.
Please rerun the guard/publisher regression suite and the entire harness verifier on the corrected head, retaining separate HARNESS_PUBLISH=true + dry_run=false authorization and the independent founder/activation PR gate. quality/test currently passes, but the four negative security cases above are not established by its existing tests.
Reviewed: policy tiers/blocked commands, 13 core rules, tool hook, publish workflow and credential separation, guard/publisher and existing unit cases at this SHA. NOT RUN: local Git bypass fixtures, model evaluations, supervised dry run, live publish or hardware. This review is not an activation approval.
Summary
Adds the harness: 13 always-loaded core rules, 16 skills, 12 missions, 14 behaviour evaluations and deterministic tools that let Claude Code agents improve OpenAMRobot every day, while people keep safety, decisions and merging. The agents' work covers tests, CI preparation, cleanup, PR reviews, and audits of dependencies, contracts, impact, performance, maintainability and the register. It also adds the decisions.yaml lifecycle:
Nothing runs after this PR merges: the workflows are design files under rollout/workflows/. Activation needs the founder's explicit approval comment on #43 (D-02), given after this PR is reviewed and merged, the behaviour evaluations pass, and a supervised dry run has been read. Closing #43 is not authorization by itself. harness/README.md is the operating manual.
Work package
#43 (AI workflow activation). Related: #59, #69.
Integration Gate
Builds on #69 (daily collector; triage reads its watchdog-daily artifact); no file overlap with #69. Reuses tools/check_decisions.py (loader, applies_to matching, JSON findings), tools/sync_audit_issues.py (audit issues, one ID range per audit) and agent-prompts/push-from-bundle.md. Inherits the 29 Oct external audit and instruction matrix:
AGENTS.md and CLAUDE.md are unchanged (shared block v2,
@AGENTS.md). Rejected:Tests
Ran 362 tests, 0 skipped with
python3 -m pytest -q tests(278 on the parent). New: tests/test_harness.py, tests/test_harness_publish.py, tests/test_harness_decisions.py. They cover policy, tiers, coverage, the hook, guard, selection, publish validation, artifact boundaries, the review queue, the merge gate, inputs, output scrubbing, decisions impact, the follow-up, register-sync approval, core-rule loading and the eval suite shape. Behaviour evaluations: 14 of 14 cases passed 3 of 3 runs withclaude plugin evalon Claude Code 2.1.312 (harness/evals/BASELINE.md; rerun after merge with the pinned model).Evidence
Base SHA: 71c439e
Head SHA: 54bdd2b
Tree: 016c61c80e3e49f558690b89fa08c328bce79fab
changed.txt is
git diff --name-only 71c439e3b6dc823589ba2020a7063cf4dce72c13 54bdd2bb253df881901c3daf626acdf1034043a4.Dependencies
None added to the repository. The design workflows pin by full commit SHA, each verified with
git ls-remoteagainst the official repository:Decisions impact
Safety impact
The changed files are governance tooling, tests and design workflows in this repository. No robot source, launch file, URDF or collision geometry, motion limit, firmware configuration or interface definition changes, and no changed path matches maintainers.yaml safety_paths (tests/test_harness.py checks this with the evidence check's matching). The harness decides what agents may change on the robot later. So before any activation, two parts need human review: the protected paths and tiers in harness/policy.yaml, and the publish job with its guard.
STATE.md
no change: this repository has no STATE.md (rollout/STATE.md.example only).
Not verified
AI disclosure
Claude Code drafted and tested every file in a session directed by the platform lead. It ran two independent code review passes and the behaviour evaluations, and fixed every reported defect. This session applied and pushed the prepared change set. A person must read first:
Contribution terms
Generated by Claude Code