Repository navigation
docs: redact home paths from 10 tracked files (public repo + live Pages) - #161
Merged
Merged
Conversation
…, correct the docs Follow-ups from the review-standard work (#157, #158). Each item below was raised as a concern and is now closed with a measurement, not a read. ## 1. The async acceptance path dropped `parse_failed` (fail-open) `director.py:792` gates the SYNC path on the marker: accepted = cto PASS and coo PASS and ... and not parse_failed The teacher is the ASYNC path. It wrote `{role}_verdict` into the bookbag but never the marker, so a judge that returned an unparseable verdict was recorded as a clean PASS and every bookbag consumer (`school_grader.py:284-286`, `teacher_feedback.py:84-85`) saw an approving vote for work nobody reviewed. Same class as the bug #157 fixed, on the path nobody patched. - `teacher.py`: always write `{role}_parse_failed` (absent is never ambiguous) - `teacher_feedback.py`: carry `parse_failed` through the feedback record RED first: `KeyError: 'cto_parse_failed'` (2 failed). Then GREEN: 53 passed. ## 2. The circuit breaker's outcome never reached the caller (dead code) The breaker branch built a replacement `ReviewResult` and mutated `result.lens_used`, but `result` was never read again — `final_result` is built fresh, and `lens_used` was `",".join(...)` over the lens names. So the `(circuit_breaker)` / `(double_pass)` tags were dead strings and a reader could not tell a breaker-confirmed verdict from an ordinary one. - Carry a `breaker_tag` into `final_result.lens_used`; plain case unchanged - Remove the dead rebuild; update the docstring in `test_lens_parse_fail_open.py` that documented the old limitation RED first: `'correctness,security'` (no tag, 2 failed). Then GREEN. ## 3. The outer-loop gate did not exist (it was documented as if it did) `.beads/hooks/pre-commit` was 33 lines of pure beads shim — all inside the BEADS-managed markers — with no `bd lint`. `bd config get hooks` -> not set, so `bd hooks run pre-commit` ran nothing. `tests/test_outer_loop_gate.py` was an untracked spec for this unbuilt gate. - Append the `bd lint` gate BELOW the managed markers (inside them is silently replaced by `bd hooks install`) - Lints every gated type (`task bug feature`); `decision`/`epic` are containers - Explicit `BEADS_SKIP_LINT=1` bypass, so the bypass is visible, never silent - Track the test; it is green Measured: $ sh .beads/hooks/pre-commit; echo rc=$? rc=0 # bd lint clean, 18 issues checked $ PATH=<stub failing bd lint>:$PATH sh .beads/hooks/pre-commit; echo rc=$? outer-loop gate: bd lint failed for work type 'task': rc=1 # a real failure is a real refusal $ BEADS_SKIP_LINT=1 sh .beads/hooks/pre-commit; echo rc=$? rc=0 Syntax verified under `dash -n`, `bash -n`, `sh -n`. 6/6 gate tests pass. ## 4. Documentation corrections (four false claims shipped on main) - `mechanical-gate.md`: claimed the hook carried "the `bd lint` gate" and that a commit ran it. It did not. Now documents the gate as restored, with the transcript above. - `quality-gates-checklist.md`: claimed `SOUL.md` "referenced" this file. `SOUL.md:6` says "checklist-driven review" — conceptual, not a citation. Also: the `code-quality-gates` skill lives OUTSIDE this repo (`~/.hermes/profiles/lucas/skills/school-core/code-quality-gates/SKILL.md:48`), so that citation is not resolvable from a clone. Now named in full. - `quality-gates-checklist.md` §2: the `NO_TEST_NEEDED` enumeration omitted `.gitattributes`, `LICENSE*`, `CHANGELOG*`, `CODEOWNERS` — all present in `review_gate.py:47-53`. ## 5. A bot approval satisfied branch protection Measured: PRs #157 and #158 both merged on `sourcery-ai | APPROVED`, with no human review. `require_code_owner_reviews` was `false`, so `required_approving_review_count: 1` was satisfied by an app. - `.github/CODEOWNERS`: `* @branben @luna-whymage` (GitHub does not count an author's own approval, so for @branben's PRs the required reviewer is @luna-whymage) - Branch protection updated out-of-band: `require_code_owner_reviews: true` ## NOT fixed by this change - `standard_loaded` is read at `adversarial_reviewer.py:364` (it gates a warning), but the RESULT does not carry whether the standard was applied. A consumer still cannot tell a standard-applied review from a degraded one. - The gate covers `bd lint` only. No `compileall`, no `mypy --strict`, no secret scan on the commit path. - `.beads/school-core-*.json`, `.tessl/`, `.worktrees/`, `test_a2a_bridge.py` remain untracked and untouched.
…ot_* tools The agent-school MCP server (config.yaml:95) points at KnowledgeCore/01-Projects/School/school-core/mcp_server.py -- a SEPARATE directory from this repo (different inode, not a symlink), at be8438e on feat/campus-framework (2026-06-24). be8438e is NOT an ancestor of origin/main and is NOT in this repo's object store (`git cat-file` fails). The four `school_bot_*` tools (execute/mention/list/info) exist ONLY there, as thin wrappers over a root-level bot_adapter.py. This repo had no bot_adapter.py, no `def bot_execute`, and zero `school_bot_*` (grep-confirmed). So the naive "just repoint the config" fix would ImportError the server and DELETE the dispatch surface. The real fix is to port the surface into the real tree. What changed - Add src/tools/bot_adapter.py (stdlib-only): wraps `hermes -p <bot> chat` as a transport. bot_execute / bot_mention / bot_list / bot_info, plus bot_exists and _hermes_binary. Stdlib-only by design -- the MCP server must start even when the repo's optional deps are absent. - src/tools/mcp_server.py: guarded import (a missing adapter degrades to a per-tool error, not a dead server), 4 TOOL_DEFINITIONS, 4 handlers, and the Tools docstring list. - tests/test_bot_adapter.py: 17 tests. Why the parity guard is the load-bearing one The server publishes TOOL_DEFINITIONS to clients and dispatches through TOOL_HANDLERS. Nothing asserted those two sets agree, so a tool could be advertised and then answered with -32601 Unknown tool. Mutation-verified: with `school_bot_info` removed from TOOL_HANDLERS, 3 tests fail; restored, 17 pass. Verification - `python3 -m src.tools.mcp_server` from repo root, no PYTHONPATH: 12 tools, cold start 0.47s, Engram connected. `src/__init__.py` exists, so -m resolves `src` from cwd -- the MCP config needs no PYTHONPATH, only a cwd. - End-to-end tools/call: school_bot_list returns the real profile roster (lucas, mentor, principal, phymora, scribe, ...); school_bot_info returns lucas's model and SOUL.md preview. - pytest tests/test_bot_adapter.py: 17 passed in 0.19s. - ruff check src/tools/mcp_server.py src/tools/bot_adapter.py: All checks passed. - ruff check --select F821,F811,F822 tests/test_bot_adapter.py: All checks passed. - Regression batch (test_bot_adapter, test_teacher, test_teacher_feedback, test_school_grader, test_role_loader, test_parallel_fleet, test_pipeline_metrics, test_worktree_gc): 146 passed in 6.41s. - Nothing else imports mcp_server (grep over tests/: only conftest.py and this new file), so the regression surface is nil by construction. Not fixed here - The running agent-school MCP server still points at the KnowledgeCore clone until config.yaml is repointed. That is a separate profile change and needs an MCP restart to take effect. - `bot_execute`/`bot_mention` are not yet wired into the crew dispatch path -- they are callable over MCP, but School Core does not route to them yet. - The stub in tests/test_bot_adapter.py for `bot_info`'s model parse is cosmetic only. Refs #8162 (CI flake), #8163 (dead signal), #8164 (unchecked teacher daemon) are separate; this commit does not touch them.
This repo is PUBLIC and GitHub Pages is live at https://branben.github.io/school-core/ , so docs/incidents/index.html is a public web page. 17 occurrences of the local home path were tracked. Redacted to `~` (not `<home>`, which HTML would parse as a tag): - docs/incidents/index.html (3) -- LIVE on Pages - scripts/strings_and_literals_pending.md (6) - README.md (1) - docs/research/sch{9,11,13}-adversarial-review.md (3) - docs/plans/2026-08-11-001-...md (1) - .openhands/memory/2026-10-14.md (1) - .github/workflows/ci.yml (1, comment) - scripts/sanitize_data.py (1, comment) Deliberately NOT changed - tests/ (40 occurrences): the literal IS the fixture. test_sanitize_data.py asserts HOME_RE.sub("~", "...") ; test_issue_bridge.py and test_persistence_contract.py assert the path is ABSENT from output. Redacting these would break the guards they test. - scripts/com.*.plist (8): launchd does not expand `~` in ProgramArguments or StandardOutPath, so redacting breaks the services. Flagged, not fixed. - .github/workflows/school-loop.yml (1): a literal PATH entry. GitHub Actions does not shell-expand job-level env values, so `$HOME` would not work. Flagged, not fixed. Verification - `python scripts/sanitize_data.py --verify` -> OK - HOME_RE (scripts/sanitize_data.py:43) untouched: /Users/[A-Za-z0-9_.-]+ - pytest test_sanitize_data + test_verify_gate + test_pipeline_metrics + test_bot_adapter -> 81 passed, 1 skipped - docs/incidents/index.html renders `~/tmp` as text inside <code>, not a tag Not fixed here - History: 297 of 1008 commits carry a leaking author email. The global git user.email has been switched to the GitHub noreply address, which stops new leaks; existing history is unchanged (would need a force-push rewrite). - The PII guard (scripts/sanitize_data.py --verify) scans data/ ONLY. That is why none of these 17 occurrences was ever caught. Widening its scope is a separate decision. Baseline check (proves the orca failure is NOT from this diff) - tests/test_orca_execution.py WITH this diff: 1 failed, 23 passed, 15 skipped. The failure is test_coder_produces_executable_code with "OmniRoute HTTP 401: No active credentials for provider: antigravity". - Same file WITHOUT this diff (git stash): ALSO red -- times out on a threading _condition.wait. - Red both ways => pre-existing environmental failure, not a regression. - This diff changes exactly one .py file, and only a comment (scripts/sanitize_data.py:320). Zero behaviour change.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Reviewer's GuideThis PR combines the stated home-path redaction with several functional changes: it adds Hermes Bot Mode dispatch and MCP exposure, makes circuit-breaker and parse-failure outcomes observable across review pipelines, hardens the Beads pre-commit gate, and updates corresponding tests and documentation. Sequence diagram for MCP bot executionsequenceDiagram
participant Client
participant MCP as mcp_server
participant Adapter as bot_adapter
participant Hermes as hermes CLI
participant Bot as Bot Mode bot
Client->>MCP: school_bot_execute
MCP->>Adapter: bot_execute
Adapter->>Adapter: bot_exists
Adapter->>Hermes: subprocess.run
Hermes->>Bot: Execute prompt
Bot-->>Hermes: Response
Hermes-->>Adapter: stdout and return code
Adapter-->>MCP: result
MCP-->>Client: success or error
Flow diagram for observable review outcomesflowchart TD
Review[review]
Lens[Review lenses]
Breaker{Circuit breaker fires}
Tag[breaker_tag]
Result[ReviewResult]
Teacher[review_cycle]
Evidence[build_teacher_evidence]
Parse[parse_failed]
Review --> Lens
Lens --> Breaker
Breaker -->|yes| Tag
Breaker -->|no| Result
Tag --> Result
Result --> Teacher
Teacher --> Parse
Parse --> Evidence
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+642
to
+643
| result["has_error"] = not result.get("success", False) | ||
| return result |
There was a problem hiding this comment.
Successful calls to school_bot_execute and school_bot_mention return error: None, but _handle_request treats any result containing an error key as failed. This overrides has_error: false, so clients may reject the reply or retry work that already ran. Check the error value instead of key presence so successful replies get isError: false.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/mcp_server.py
Line: 642-643
Comment:
**Successful bot calls look failed**
Successful calls to `school_bot_execute` and `school_bot_mention` return `error: None`, but `_handle_request` treats any result containing an `error` key as failed. This overrides `has_error: false`, so clients may reject the reply or retry work that already ran. Check the error value instead of key presence so successful replies get `isError: false`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+117
to
+120
| create = subprocess.run( | ||
| ["bd", "create", title, "-t", "task"], | ||
| cwd=REPO_ROOT, capture_output=True, text=True, | ||
| ) |
There was a problem hiding this comment.
If bd is installed, test_bd_lint_exits_nonzero_on_missing_section creates its scratch task in the real repository's issue database. Cleanup only closes the task, so each run leaves another record. The deliberately incomplete task can also briefly block commits through the new hook. Run this probe against a temporary, isolated database.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_outer_loop_gate.py
Line: 117-120
Comment:
**Tests change real issue records**
If `bd` is installed, `test_bd_lint_exits_nonzero_on_missing_section` creates its scratch task in the real repository's issue database. Cleanup only closes the task, so each run leaves another record. The deliberately incomplete task can also briefly block commits through the new hook. Run this probe against a temporary, isolated database.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+87
to
+89
| "parse_failed": bool( | ||
| review.get("cto_parse_failed") or review.get("coo_parse_failed") | ||
| ), |
There was a problem hiding this comment.
The new parse_failed field reads only cto_parse_failed and coo_parse_failed. However, two_judge_review computes a combined parse_failed flag and omits it from its returned review. _attach_teacher_evidence uses that return value, so a crashed judge is recorded with parse_failed: false. Acceptance stays blocked, but the saved evidence loses the reason. Return the combined flag and read it here.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/teacher_feedback.py
Line: 87-89
Comment:
**Judge failures look readable**
The new `parse_failed` field reads only `cto_parse_failed` and `coo_parse_failed`. However, `two_judge_review` computes a combined `parse_failed` flag and omits it from its returned review. `_attach_teacher_evidence` uses that return value, so a crashed judge is recorded with `parse_failed: false`. Acceptance stays blocked, but the saved evidence loses the reason. Return the combined flag and read it here.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| "duration_seconds": 0.0, | ||
| } | ||
|
|
||
| hermes = _hermes_binary() |
There was a problem hiding this comment.
If a profile exists but Hermes is missing, _hermes_binary() raises BotAdapterError before the error handler in both bot_execute and bot_mention. Python callers get an exception instead of the documented failure dictionary, and MCP returns an internal error. Catch BotAdapterError and return the same failure shape used for other transport errors.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/bot_adapter.py
Line: 144
Comment:
**Missing Hermes raises unexpectedly**
If a profile exists but Hermes is missing, `_hermes_binary()` raises `BotAdapterError` before the error handler in both `bot_execute` and `bot_mention`. Python callers get an exception instead of the documented failure dictionary, and MCP returns an internal error. Catch `BotAdapterError` and return the same failure shape used for other transport errors.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
luna-whymage
previously approved these changes
Oct 25, 2026
luna-whymage
left a comment
luna-whymage
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: APPROVE — luna-whymage
Correct, careful PII hygiene — not a blind find/re-place.
- Redacts 17x
/Users/brandonbennett→~across 10 files. Target~(not<home>):docs/incidents/index.htmlis live on Pages and<home>would parse as a tag. Correct. - Deliberately NOT redacted, and correctly so:
tests/(40 occ — the literal is the fixture:test_sanitize_data.pyassertsHOME_RE.sub("~","…"),test_issue_bridge.py/test_persistence_contract.pyassert the path is ABSENT — redacting breaks the guards);scripts/com.*.plist(8 — launchd does not expand~inProgramArguments/StandardOutPath, so redacting breaks the services);.github/workflows/school-loop.yml(1 — GH Actions does not shell-expand job-level env). - Verification (run this turn as luna): pytest
test_sanitize_data test_verify_gate test_pipeline_metrics test_bot_adapter→ green ($TESTS_GREEN);python scripts/sanitize_data.py --verify→ OK;HOME_RE(sanitize_data.py:43) untouched. - Regression guard included:
tests/test_orca_execution.pyis red BOTH with this diff ("OmniRoute HTTP 401: …antigravity") and aftergit stash("_condition.wait" timeout) → pre-existing environmental failure, not a regression. - Only one .py touched, and only a comment (
scripts/sanitize_data.py:320); zero behavior change.
Not-fixed-here notes (acceptable to defer to a separate ticket): 297/1008 commits still carry the author email leak (history would need a force rewrite); the PII guard still scans data/ only. Worth one follow-up.
Clean to merge.
All three were found by review, confirmed against the code, reproduced with a
failing test first, and fixed. Each test fails on the pre-fix code.
P1 -- a successful bot call was returned as an MCP error (double execution)
src/tools/mcp_server.py:798 read `is_error = has_error or "error" in result`.
The bot tools mirror bot_adapter's return shape, which always carries `error`
(null on success), so the presence of the key marked every SUCCESSFUL call as
isError. A client that retries on isError re-runs the bot's work.
Fix: `_is_error_result()` -- an explicit `has_error` wins; `error` is only
consulted when `has_error` is absent, and `error: None` is not a failure.
Verified: test_a_successful_bot_call_is_not_an_mcp_error asserted
`isError is False` and failed with `assert True is False` before the fix.
20 passed after (18 before + 2 new wire-level). The failure-path test still
passes, so the fix does not over-correct.
P2 -- the parse_failed evidence gate was inert
teacher_feedback.py reads `cto_parse_failed`/`coo_parse_failed` off the review
dict, but _run_two_judge_review computed parse_failed only to gate `accepted`
and never returned it. So the evidence record always said parse_failed:false,
even when a judge crashed and the work was rejected for exactly that reason --
a rejection whose stated cause was absent from the record.
Fix: return parse_failed, cto_parse_failed and coo_parse_failed.
Verified: test_a_crashed_judge_is_recorded_in_the_review_dict failed with
`KeyError: 'cto_parse_failed'`; test_the_feedback_record_carries_the_judge_crash
failed with `assert False is True`. 75 passed after.
P3 -- a test left scratch issues in the developer's real database
tests/test_outer_loop_gate.py closed its probe instead of deleting it, while
its docstring claimed "auto-removed". 11 SCRATCH gate-signal probes had
accumulated in the real beads DB.
Fix: `bd delete --force` in the finally block, and correct the docstring.
Verified: 6 passed; probe count 11 before and 11 after the run, 0 open.
What this does NOT fix:
- 11 historical probe issues already in the DB (bd delete rewrites references
in connected issues to [deleted:ID]; left for an explicit decision).
- bot_execute/bot_mention are MCP-callable but not wired into school-core's
dispatch path (pre-existing, non-blocking).
- The running agent-school MCP server needs a restart to pick up its repoint.
Check 4 failed only when NO test file changed anywhere in the PR. Three real
defects passed it:
- a successful bot call returned as an MCP error (mcp_server.py:798)
- a parse_failed flag that was permanently false (teacher_feedback.py)
- a test that left 11 scratch issues in the real database
Each PR did change a test file -- just not one that exercised the broken
path. "A test changed" is not "this code is tested", and a gate that cannot
tell the difference is a checklist, not a verification.
The check now resolves each changed source file to its module name and fails
unless some test in the repo names that module, by filename convention
(tests/test_module.py) or in its text. Fails closed if tests/ is absent
rather than reading as clean.
Verified RED: committed src/core/zzz_orphan.py ->
[untested] src/core/zzz_orphan.py: no test names module 'zzz_orphan':
changed source with no covering test
Verified GREEN: removing that file -> "PASS - all mechanical checks clean"
No false positives: the 27-file real diff still passes.
tests/test_review_gate.py: 15 passed.
Two defects in my own first attempt, both caught by tests/test_review_gate.py
before this commit:
1. _test_source_blob resolved tests/ from the SCRIPT's location, so the
fixture repos in tests/ were scanned against school-core's real test
tree -- a fixture diff passed that should have failed
(test_source_change_without_test_fails).
2. The scan read only file contents, so a test named test_module.py whose
body never says "module" was not counted (test_source_change_with_test_passes).
Filenames are now included; both tests pass.
Separately, required_status_checks on main now includes pytest (py3.9),
py3.11 and py3.12 alongside review-gate. Previously review-gate was the ONLY
required check, so a PR could merge with the entire test suite red.
What this does NOT fix:
- The check is a substring scan, not a coverage measurement. It proves a
test NAMES the module, not that it exercises the changed path. The three
defects above would still pass if the naming test did not assert on the
broken behaviour. The real fix is boundary-level tests (JSON-RPC
response, not the handler's return dict).
- CircleCI Pipeline fails on PR #161 and is not required.
- integration (live Orca + OmniRoute) is push-only, so it cannot gate a PR.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Comment on lines
+153
to
+160
| result = subprocess.run( | ||
| cmd, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=timeout, | ||
| # Inherit minimal env; hermes reads profile config itself | ||
| env={**os.environ, "HERMES_QUIET": "1"}, | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
Comment on lines
+228
to
+234
| result = subprocess.run( | ||
| cmd, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=timeout, | ||
| env={**os.environ, "HERMES_QUIET": "1"}, | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
The template and the evidence generator disagreed: the template was a
checkbox list ("pytest passes", "ruff check passes") and the generator emitted
a differently-shaped checkbox comment. Both are assertions. Neither could be
wrong in a way a reader would notice.
The shape is now shared, in this order:
What -- BLUF, 2-3 sentences, human-written
Ticket -- ticket link, one-line summary, reviewed by
[inner loop] Review -- generated: which tests name each changed module
Feedback -- generated: the validate phase, what actually executed
Gotchas -- what might happen next, human-written
No checkboxes anywhere. "PASS" means a command ran and returned 0 at
generation time and the line names it; a stale box cannot stay ticked.
Two real bugs fixed in scripts/pr_evidence.py, both found by running it:
1. "no test summary produced" hid pytest's exit code behind a phrase that
read like a formatting problem. Exit 4 (usage error / nothing collected)
and exit 5 (no tests collected) both mean nothing was verified, and both
reported identically. The fallback now names the exit code and quotes the
first error line.
2. One combined pytest run over 77 files stamped a single verdict onto every
changed module, so one broken file failed all six and the summary was
byte-identical on every line -- the report could not say which module's
tests failed. It also took long enough to time out.
Now two claims are kept separate:
- per module: "named by <tests>" -- a static coverage CLAIM
- "changed tests": the test files this PR changed, actually run
Neither claim pretends to be the other.
Verified: python scripts/pr_evidence.py 161 -> exit 0,
"changed tests -- PASS -- 202 passed"
"every changed module is named by a test -- PASS -- 6/6 module(s)"
"behaviour change is pinned by a test change -- PASS -- 6 src, 6 test"
Before the fix the same command reported FAIL with "no test summary produced"
on all seven inner-loop lines.
What this does NOT fix:
- The per-module line is a NAME match, not coverage. It says a test mentions
the module, not that the test exercises the changed path.
- --merge-scout does not exist in the plannotator CLI (verified: zero
matches for "scout"). The Gotchas section is human-written until a real
blast-radius tool is wired in.
Owner
Author
[inner loop] ReviewHead
Guards
FeedbackValidate phase — what actually executed against this commit:
Generated by |
This is the root cause behind "no test summary produced" in the PR evidence
report, and behind the local suite stalling. It was not a formatting bug in
the parser.
executor.OMNIROUTE_BASE is hardcoded to http://localhost:20128/v1. Three
tests never patched the model seam, so they opened a real socket:
tests/test_blocking_finding_veto.py (all 12 tests, via
_run_two_judge_review -> _synthesize_judge_narratives -> _call_model)
tests/test_issue_bridge.py:2582 test_spawn_failure_falls_back_direct
tests/test_orca_execution.py:507 (legitimately live, see below)
A node process listens on :20128 on this machine. It accepts the connection
and never answers the POST, so the test blocks in socket.recv_into until
pytest-timeout kills the *whole session*. A killed session prints no summary
line, so pytest exits 4 with "no tests ran" -- and every consumer that parses
"N passed" concludes nothing was verified. One unpatched seam made 1303
passing tests look like zero.
Measured, 77-file changed-file set:
before: exit 4, hung at 21%, "no tests ran", no summary
after: exit 0, 1303 passed, 10 skipped, 16 deselected in 251s
Two fixes, both needed:
1. tests/conftest.py -- new autouse fixture block_live_model_calls. Patches
socket.connect/connect_ex to raise a named ConnectionRefusedError for port
20128 only. Turns a silent hang into an instant failure that says which
seam to patch. This is the fourth fixture of its kind in that file, after
hermetic_git_identity, isolate_data_dirs and disable_live_vault -- the
network one was simply missing.
Exempts @pytest.mark.live, because CI runs those in a dedicated job
(ci.yml:229, pytest -m live tests/test_orca_execution.py) whose purpose
is to touch the real service.
2. The two genuinely-hermetic tests get the patches their siblings already
had. test_timeout_falls_back_direct, immediately below
test_spawn_failure_falls_back_direct, already patched both
issue_bridge.call_model and src.crew.executor.call_model. The sibling above
it did not.
Verified both directions, not just the happy path:
live-marked probe -> 1 passed (guard correctly exempt)
unmarked probe -> exit 1, "test opened a live model connection to
('127.0.0.1', 20128). OMNIROUTE_BASE is ..."
both probe files deleted after the run
What this does NOT fix:
- The guard covers port 20128 only. A test reaching any other live service
still hangs. It is a net for the one dependency this repo hardcodes.
- --timeout killing the session still destroys the summary. The guard stops
the known cause; it does not make pytest survive an arbitrary hang.
- test_orca_execution.py's live tests are still unverified locally unless a
gateway and Orca are up; they are deselected by -m "not live".
The template content was written to .github/PULL_REQUEST_TEMPLATE.md, but git tracks .github/pull_request_template.md (lowercase). On a case-insensitive filesystem the write landed in the tracked file while 'git add' of the uppercase path staged nothing, so the template was never actually committed -- the previous commit carried only scripts/pr_evidence.py. Committing under the tracked name. No second file is created: a case-variant duplicate would be invisible on macOS and a real conflict on Linux CI.
…l rejects
The Review phase is supposed to emit a plannotator guide (docs/sdlc/loops.md:37,
docs/templates/phase-map.md:19). This file was the contract for it -- and it was
wrong in both directions.
MEASURED, not read: `plannotator guide export` on the repo's own example:
Guide is not valid:
- `intent` must be a non-empty string.
- `sections` must be a non-empty array.
The schema said {schema,title,summary,chapters[{id,title,summary,files[{path,why}]}]}.
The tool wants {title,intent,sections[{title,overview,diffs[{file,summary}]}],...},
read from `plannotator guide export --help`.
WHY NOBODY CAUGHT IT: tests/test_guide_schema.py passed 11/11 against the
hand-written schema. A schema that only agrees with itself proves nothing.
CHANGES
- schema: rewritten to the real shape, with the old shape named in $comment as
a warning, plus the real provenance/source/generator keys.
- example: rewritten to the real shape; same story, same three files.
- tests: structural checker for the real shape; `test_schema_required_matches_
the_real_tool_shape` asserts BOTH directions (new keys required, stale keys
absent) so a silent revert fails; plus two tests that RUN THE TOOL.
- test_real_plannotator_export_produces_html -- synthetic patch built from
the example's own files; asserts an HTML artifact containing the
plannotator-guided-review marker and every guide file.
- test_real_plannotator_rejects_a_file_absent_from_the_patch -- negative.
Measured: the tool exits 0 EVEN WHEN IT REJECTS, printing "Guide is not
valid" and writing no file. The assertion is therefore on the artifact and
the message, not the exit code -- asserting on exit code alone would pass
while the guide was silently refused.
VERIFICATION
12 passed in 16.44s (tests/test_guide_schema.py)
$ plannotator guide export --guide docs/templates/review_guide_example.json \
--patch /tmp/final.patch --out /tmp/guide_final.html
Exported Guided review: README onboarding win + build-cache config (7 KB)
-rw-r--r-- 6704 /tmp/guide_final.html
<!doctype html> ... <meta name="plannotator-guided-review" content="v1">
all 3 guide files present in the HTML
NOT FIXED: nothing in the repo generates guide.json or invokes plannotator --
docs/sdlc/loops.md:37 promises an artifact that is still produced by hand.
…example Two defects in my own 406f4da, both found by measuring rather than assuming. 1. FALSE CLAIM ABOUT THE EXIT CODE. I wrote that `plannotator guide export` "exits 0 even when it rejects a guide". It exits 1. Measured with subprocess.run: RETURN CODE: 1 stderr: Guide is not valid: - These files are not in the patch: scripts/bootstrap_cache.sh. My earlier "EXIT=0" came from `cmd | tail -8; echo EXIT=$?` -- that reads tail's status, not the tool's. The skill documents exit 1 on failure; the measurement now agrees with the skill. The test now asserts BOTH the non-zero exit and the absent artifact, so a future reader cannot repeat the pipeline mistake. 2. MISSING unplacedFiles IN THE EXAMPLE. The authoring skill says to always include unplacedFiles (empty array when every changed file is placed). The example omitted it. Added. VERIFICATION 12 passed in 3.46s (tests/test_guide_schema.py) export of the updated example: exit 0, Exported ... (7 KB), 6692 bytes of HTML
Three defects, all of the same shape: a signal that could only ever say one
thing, so the report was wrong in a way nothing could detect.
1. scripts/pr_evidence.py — a queued CI run rendered as FAIL.
`Report.ok()` was `all(c.ok ...)`; `all()` treats None as falsy, so a PR
whose tests had not started yet read as broken. `outer_loop()` did the same
thing one layer down: `ok = conclusion == "success"`, and `gh` reports an
unfinished run with `conclusion: null` plus a status. Both now map
queued/in_progress/pending/waiting/requested to None, which renders
"PENDING". This is the same class as teacher_feedback.py's inert
`parse_failed` gate: a field that can only hold one value reports a fact
that is not true.
PENDING is NOT success. main() exits 1 on it (`report.ok() is True`), so an
unfinished run can never gate a merge open. Honest text, fail-closed code.
Evidence:
RED .venv/bin/python -m pytest tests/test_pr_evidence_tri_state.py
-> 7 failed, 14 passed (the 7 are exactly the two unfinished paths)
GREEN -> 21 passed, rc=0
no regressions: tests/test_review_gate.py + tests/test_guide_schema.py
-> 27 passed, rc=0
2. .github/workflows/ci.yml — the "Gate report" step never rendered.
It was a multi-line PLAIN scalar, not a block scalar. YAML folds a plain
scalar's newlines into spaces, so each trailing backslash became a literal
backslash-plus-space and review_gate.py received one argv token. Measured
from the live run log (40241535803):
error: unrecognized arguments: --base origin/main --head HEAD --json
Measured from the parsed YAML before the fix:
repr(run) = 'python scripts/review_gate.py \\ --base "..." \\ --head
"HEAD" \\ --json || true' # one line
and after:
repr(run) contains real newlines, no literal '\\ --base'.
`|| true` alone is what hid it for the life of the step: a green step with
no artifact is worse than a red one, because nothing looks wrong. The step
now writes review-gate.json and fails if the file is empty, so a broken
invocation cannot pass silently again.
3. This change was itself caught by the gate it was meant to strengthen.
review_gate.py check 4 rejected the branch:
[untested] scripts/pr_evidence.py: no test names module 'pr_evidence':
changed source with no covering test
That is a true positive: 8c793d0 changed the script with no test naming it.
tests/test_pr_evidence_tri_state.py supplies what was missing. Local gate
after: "PASS - all mechanical checks clean", rc=0.
What this does NOT fix:
- The plannotator guide cannot satisfy the one-file build contract.
scripts/check_html_artifact.py returns 1 on the real export (5 external
guides.show URLs, no <main>, no tier-3 <meta name="source">), and TIER.md
:135 forbids external URLs. The plan mandates that artifact (U1) and its
own contract rejects it. That is a contract decision, not a code bug, and
it is unresolved. The verifier is not wired into any workflow, so this is
latent rather than red.
- director.py:759 still joins ThreadPoolExecutor futures with no timeout
(the 4/10 integration flake).
- CircleCI still fails and is still not a required check.
luna-whymage
previously approved these changes
Oct 26, 2026
luna-whymage
left a comment
luna-whymage
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: APPROVE — luna-whymage
Items 1–5 verified at HEAD 2f42e08 (evidence below). This is the stack tip and now carries b31fa44 (the three defects) + the gate/tri-state/CI/hermeticity work, so it subsumes #159/#160.
1. Three defects (b31fa44) — real and fixed
- P1
src/tools/mcp_server.py_is_error_result(): on the un-fixedis_error = has_error or "error" in result,test_a_successful_bot_call_is_not_an_mcp_errorfailsassert True is False(a success reported as an MCP error); with the fix restored ->20 passed. - P2
src/core/director.pynow returnsparse_failed/cto_parse_failed/coo_parse_failed: on the un-fixed return dict,test_a_crashed_judge_is_recorded_in_the_review_dictfailsKeyError: 'cto_parse_failed'; restored ->3 passed. - P3
tests/test_outer_loop_gate.pynow usesbd delete --force(wasbd close);bdpresent at ~/.local/bin/bd v1.3.1-dev;1 passed.
(These three are committed as b31fa44 on this branch. Thefix/review-followups/fix/mcp-bot-dispatch-portbranches still sit at the defective d2f9215/21924c4; REQUEST CHANGES posted there earlier still stands -- they are subsumed when this stack merges top-first.)
2. Gate change (9883d94) — fail-closed, no false positives
python3 scripts/review_gate.py --base origin/main --head HEAD --json -> {"pass": true, "failures": [], "changedFiles": 36, "addedLines": 2471}. Every changed src module is named by a test (incl. review_gate and pr_evidence); an empty/missing tests/ tree also hard-fails (review_gate.py:222-230). Your "[untested] scripts/pr_evidence.py" true-positive is in the pre-fix commit; at HEAD pr_evidence is covered by test_pr_evidence_tri_state.py. Minor: you cited 999237f, but the gate-change commit is 9883d94 (999237f is not in branch history -- likely a typo).
3. Tri-state (2f42e08) — PENDING is not success, fail-closed
pr_evidence.py:402 return 0 if report.ok() is True else 1; Report.ok() returns None for queued/in_progress (PENDING_CONCLUSIONS = {"","queued","in_progress","pending","waiting","requested"}, pr_evidence.py:56-58). tests/test_pr_evidence_tri_state.py -> 21 passed (incl. test_pending_is_none_not_false, test_in_progress_is_pending_not_failure, test_pending_in_any_section_is_pending). Pending renders PENDING and exits 1 -- never counts as success. Did not accidentally make PENDING green.
4. CI fix (2f42e08) — block scalar + artifact assert
.github/workflows/ci.yml:286 run: | (block scalar; fixes the plain-scalar newline-fold that fed review_gate a mangled argv and produced error: unrecognized arguments: --base origin/main --head HEAD --json). :292 test -s review-gate.json || { ::error::review-gate.json is empty -- the gate report did not render; exit 1 }. An empty report now fails the step instead of being hidden by || true.
5. Hermeticity (71be57a) — guard fires, no 300s hang
tests/conftest.py:199-200 exempts @pytest.mark.live; :222-232 raises ConnectionRefusedError on socket.connect to port 20128. .github/workflows/ci.yml:229 runs live tests separately (-m live tests/test_orca_execution.py). tests/test_blocking_finding_veto.py -m "not live" --timeout=30 -> 12 passed, 0.16s (no hang); the previously-unmocked :20128 seam no longer kills the session.
This PR (#161 redaction) — clean
Only scripts/sanitize_data.py:320 (.py) is touched, and only a comment; zero behavior change. Redaction target ~ is correct for live Pages HTML (<home> would parse as a tag). Deliberate non-redactions (tests/, scripts/com.*.plist, .github/workflows/school-loop.yml) are correct (~ is not expanded by launchd or by GH Actions job-level env). python scripts/sanitize_data.py --verify -> OK; pytest test_sanitize_data test_verify_gate test_pipeline_metrics test_bot_adapter -> 84 passed, 1 skipped; test_orca_execution red both with and without this diff (pre-existing env: OmniRoute 401 / threading wait), not a regression.
6. Out of scope, flagged (latent, not blocking) — plannotator guide vs TIER contract
See final report for the read on which side should change. Not wired to CI; this is latent, not red, per your note. Do not let it pass silently: tracked here.
Approving on items 1–5 holding at HEAD 2f42e08. Item 6 is the one unresolved call; my read is in the summary.
…d wrong
scripts/guide_gen.py turns a unified diff into a plannotator guide.json that
cannot violate the tool's hard rules, so a CI job can gate on
`plannotator guide export` and fail closed.
Measured contract, probed against the installed binary BOTH WAYS (not read from
help text):
* title/intent non-empty; sections 1..6 (0 rejected as "must be a non-empty
array", 7 rejected)
* diffs[].file is the EXACT path with b/ stripped (b/src/a.py is rejected and
the tool names the real paths)
* a file in AT MOST one section (duplicate placement rejected)
* unplacedFiles is OPTIONAL; a changed file may be in zero sections
* every path in unplacedFiles must still appear in the patch -- the tool
rejects "unplacedFiles not in the patch: X"
FIVE corrections to what I had recorded earlier. My previous notes said 2-6
sections (it is 1-6), that unplacedFiles was mandatory (it is optional), that a
file could not appear in a section AND unplacedFiles (it may), that a dropped
file was an error (it is not), and that unplacedFiles could name anything (it
may only name patched files). The schema's $comment and tests/test_guide_schema.py
now record the measured rules, and the anti-rot tests run the real binary so the
contract cannot drift again.
End-to-end on this branch's real diff: 36 changed files -> 6 sections -> 173064
bytes of HTML, with every changed file present in the artifact (0 missing).
Tests: tests/test_guide_gen.py + tests/test_guide_schema.py -> 30 passed.
The real-binary test earned its keep: it caught the unplacedFiles rule, which no
probe of mine had covered.
Known and NOT fixed here: scripts/check_html_artifact.py still returns 1 on the
export (external guides.show URLs, no <main> landmark) while TIER.md:135 forbids
external URLs. That is a policy call on the design doc, not a defect in the
generator, and it is latent -- the verifier is referenced by no workflow.
Collaborator
Non-blocking review notes (luna-whymage) — APPROVE holds; tracking nits(1-5 and the guide-gen gate all green at
|
luna-whymage
approved these changes
Oct 26, 2026
luna-whymage
left a comment
luna-whymage
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: APPROVE — luna-whymage (re-approval at head 0793a09)
Earlier APPROVE on 2f42e08 was dismissed by your push to 0793a09. Re-verified at the new tip (evidence this turn, identity confirmed luna-whymage):
- Fresh CI @ 0793a09: review-gate + pytest (py3.9/3.11/3.12) = all
success(mergeStateStatus=BLOCKED is only the missing review). - New tests green:
tests/test_guide_gen.py + tests/test_guide_schema.py=30 passed(incl. the 2 real-plannotator-binary acceptance tests and the schema's cross-section-rule assertions). - Generator cannot emit a tool-rejected guide:
test_generated_guide_is_accepted_by_the_real_toolruns the realplannotator guide exportagainst the generator's output and asserts rc==0 + no missing-file-in-HTML (tests/test_guide_gen.py:213). That is the "cannot emit something the tool rejects" bar — met.
The three defects (b31fa44) remain fixed: confirmed at 2f42e09's parents (director.py returns parse_failed keys; mcp_server uses _is_error_result(); outer-loop uses bd delete --force).
Item-by-item
- Defect fixes — b31fa44 is on the stack tip; re-confirmed via the green 30 (P2/P3 tests + P1 wire tests) and the stash-fail/fail/pass proof from the prior turn on the same code.
- Gate check 4 — name-based, fail-closed;
review_gate.py --jsonPASS on the full 36-file diff (no false positive onpr_evidence,guide_gen, etc.). - Tri-state —
pr_evidence.py:402exits 1 unlessok() is True; None(PENDING) → exit 1;test_pr_evidence_tri_state.py:21 passed. - CI block scalar —
.github/workflows/ci.yml:286 run: |+:292 test -s review-gate.json || { ::error::… ; exit 1 }. - Hermeticity —
conftest.py:199-200exempts@live; port-20128 connect raises (:222-232); live tests run separately (ci.yml:229), andtest_blocking_finding_veto.py -m "not live" --timeout=30= 12 passed, 0.16s. - #161 redaction — only
sanitize_data.py:320(comment) changed;sanitize --verify OK;84 passed, 1 skippedon the redaction's own test set;test_orca_executionred both with and without the diff (pre-existing env).
Non-blocking notes (posted as a PR comment, not gating)
See PR comment — (a) the 6-section merge loop is structurally unreachable (only 6 bucket keys); (c) the "no critique" test is a weak word filter; (d) "file placed twice" is stdlib-checked, not run through the real tool; item 6 (plannotator HTML vs TIER) — agree with Lucas: gate the JSON guide, keep check_html_artifact on self-authored HTML only.
Clean to merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Redacts the local home path from 17 occurrences across 10 tracked files. Redacted to
~(not<home>, which HTML would parse as a tag).Why it matters
https://branben.github.io/school-core/, sodocs/incidents/index.htmlis a public web page carrying the path 3 times.Files
docs/incidents/index.html(3) — live on Pagesscripts/strings_and_literals_pending.md(6)README.md(1)docs/research/sch{9,11,13}-adversarial-review.md(3)docs/plans/2026-08-11-001-...md(1).openhands/memory/2026-10-14.md(1).github/workflows/ci.yml(1, comment)scripts/sanitize_data.py(1, comment)Deliberately NOT changed
tests/(40 occurrences) — the literal IS the fixture.test_sanitize_data.pyassertsHOME_RE.sub("~", ...);test_issue_bridge.pyandtest_persistence_contract.pyassert the path is absent from output. Redacting these would break the guards they test.scripts/com.*.plist(8) — launchd does not expand~inProgramArguments/StandardOutPath; redacting breaks the services. Flagged, not fixed..github/workflows/school-loop.yml(1) — a literalPATHentry. GitHub Actions does not shell-expand job-level env values, so$HOMEwould not work. Flagged, not fixed.Evidence
python scripts/sanitize_data.py --verify→ OKHOME_RE(scripts/sanitize_data.py:43) untouched:/Users/[A-Za-z0-9_.-]+pytest test_sanitize_data + test_verify_gate + test_pipeline_metrics + test_bot_adapter→ 81 passed, 1 skippeddocs/incidents/index.htmlrenders~/tmpas text inside<code>, not a tagtests/test_orca_execution.pyis red with and without this diff (401No active credentials for provider: antigravity/ a threading timeout). Pre-existing environmental failure, not a regression. This diff changes exactly one.pyfile and only a comment.Not fixed here
git user.emailhas been switched to the GitHub noreply address, which stops new leaks; existing history is unchanged and would need a force-push rewrite.scripts/sanitize_data.py --verifyscansdata/only — which is why none of these 17 occurrences was ever caught. Widening its scope is a separate decision.Summary by Sourcery
Harden review and execution workflows with bot integrations, fail-closed evidence gates, stronger test isolation, preserved review failure signals, and public-path redaction.
New Features:
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests:
Chores:
Fix the successful bot calls being marked as errors before merging.
Findings
Fix with agent prompt
Summary
Redacts home paths from public documentation and comments. The full diff also:
bd lint.~instead of the local home path.Successful bot calls are currently marked as errors. The review also found a test that changes real issue records, a missing judge-failure signal, and an uncaught missing-Hermes error.
branben explicitly deferred launchd paths because
~would break the services, the school-loop PATH because job-level values are not shell-expanded, historical emails because removing them requires rewriting history, and widening the sanitizer's scope as a separate decision.Reviews (1) · Last reviewed commit: "docs: redact home paths from 10 tracked ..." · Reviewed by Greptile