feat(effectors): the outbound guard is blind to every sender that matters - #1729
Conversation
`github.comment` declared `scripts/preflight-thread-state.py` as its ground-truth
predicate. The file did not exist. The guard fails CLOSED inside its match, so the
effector was not merely unproven — arming it would have denied every `gh pr comment`
in the estate. check-runner-coverage reported exactly that as finding C.
Writing the predicate surfaced the sharper defect. `outbound-preflight-test` — the
deny matrix that is the ENTIRE proof the outbound gate denies anything — is
`scoped: false`, carries no `ci_job`, and was named by verify-whole.sh nowhere. It
had therefore never executed, from the day it landed. Its three siblings
(armed-valve-test, ship-gate-test, worktree-guard-test) are all correctly wired,
which is precisely why reading the registry did not reveal it.
· scripts/preflight-thread-state.py — reads the thread from the API (conversation
comments, review bodies, inline review comments), never from a cached PR body or
a previous turn's memory. Exit contract mirrors preflight-sent-state.py: 0 pass,
1 deny-with-the-thread-printed, 77 could-not-look. 77 STILL DENIES, because "I
could not look" is not "there is nothing there". Acknowledgement binds to comment
ids the API had to tell you, so a new reply re-closes the gate; PENDING reviews
are excluded (invisible to others); bots are labelled, never exempted; pagination
truncation is REPORTED, since a gate that passes on what it did not read is worse
than no gate.
· outbound-effectors.yaml — the target pattern captured a bare number and dropped
`--repo`, so `gh pr comment 5 -R other/repo` would have been checked against the
AMBIENT repo's #5. `{target}` is a single substitution and cannot carry a repo, so
both flag orders and both aliases now yield NO extractable target and the guard
denies. `-R` is the form this estate actually uses
(.github/workflows/pr-review-fanout.yml:38); omitting it was not hypothetical.
· check-runner-coverage.py class F — a gate must have a runner. Scoped gates are
selected by changed paths; a `ci_job` is a runner; a whole-only gate must be named
by verify-whole.sh. Also catches a scoped gate declaring no paths, which
verify.py's select() can never match (zero gates today — the ratchet keeps it so).
Baseline ratchets 3 -> 2: finding C no longer reproduces, and F starts at zero.
Verified live, read-only, against real threads: #1720 (merged PR, 10 comments) and
#1717 exit 1 with the thread printed, issue #320 (18 comments) proves the issue path,
ambient repo resolution works without --repo, a nonexistent number exits 77, and no
acknowledgement file was minted for any real thread.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WMdtji1ydxXmsyARdvmCkd
`scripts/hooks/outbound-preflight-guard.py` is a PreToolUse(Bash) hook. It
inspects a COMMAND STRING an agent is about to run — so it cannot see, and by
construction can never see, `subprocess.run(["gh", "pr", "comment", url, ...])`
executed inside a Python module. No shell, no command string, no tool call.
Every genuinely dangerous outward action in this estate takes that in-process
form and runs unattended on the beat:
scripts/apply-visibility.py gh repo edit --visibility
scripts/autonomy-governor.py gh pr merge (merge to main deploys)
scripts/contributions-organ.py gh pr comment (third-party repos)
scripts/creds-hydrate.py gh secret set (Actions secrets)
scripts/reap-remote-branches.py gh api -X DELETE (remote refs)
organs/.../representation_substrate.py smtplib.SMTP_SSL
The hook covers the one surface where a charter-reading model is already in the
loop, and misses all the sharp ones. 24 sites total.
scripts/check-effectors.py is the OUTBOUND-EFFECTORS panel's parity predicate —
the fifth VIGILIA-shaped registry to get one, beside GATES, PARAMETERS, SENSORS
and MAIL-TIERS. Three classes:
A declaration↔disk. A `predicate:` naming a file that does not exist would,
because the guard fails CLOSED inside its match, deny EVERY matching action
the moment the hook is armed. That was github.comment's live state this
morning until #1727.
B the single-capture-group bound. The guard reads
`match.group(1) if match.groups() else match.group(0)`, so a two-group
target pattern silently keeps the first and discards the second — running
the predicate against the AMBIENT repo's #N while the command addresses a
different repo's #N. The registry states this in a comment; class B is what
makes the comment true.
C in-process coverage. Enumerated by AST walk and ratcheted against
institutio/governance/ungated-effectors-baseline.txt. The 24 known sites are
pinned; a NEW ungated sender is a red check.
The scan is an AST walk over every list literal beginning with "gh", not a
pattern, because a hand grep for `subprocess.run(["gh"` found only 17 of the 24:
autonomy-governor.py binds the argv to a NAME and runs it later, and
sync-hishand-issues.py routes through a local sh() wrapper. Walking list
literals is invariant to how the argv reaches the process.
Deliberately NOT flagged: `osascript -e 'display notification'`
(conducting-report.py, notify-events.py, _notify.py) is a local desktop toast,
not an outward send, and `gh pr view`/`list`/`checks`/`api` without a mutating
method are how the preflight predicates do their job — a gate that fired on
those would make "stop checking reality" the cheapest way to go green, which
inverts the entire point of this registry.
44 tests, mutation-checked: disabling the list-literal walk fails 10, relaxing
the group bound fails exactly 1, and treating read verbs as writes fails 16.
A guard test that passes on the buggy code proves nothing.
This PR makes the surface visible and non-growing. It does not close it —
routing the 24 senders through the receipt check is the follow-on, and the
baseline is the worklist.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WMdtji1ydxXmsyARdvmCkd
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Multi-agent review roll call (CodeRabbit and Claude review automatically. Reviewers: post substantive findings only. Authors/agents: address every thread, push fixes to this branch, reply and resolve, then re-request review.) |
|
To use Codex here, create a Codex account and connect to github. |
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request adds ChangesEffector governance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PRGate
participant Checker
participant SourceFiles
participant Baseline
PRGate->>Checker: Run check-effectors.py
Checker->>SourceFiles: Parse Python files
SourceFiles-->>Checker: Return outbound-action findings
Checker->>Baseline: Compare findings
Baseline-->>Checker: Return known findings
Checker-->>PRGate: Return pass or new-findings failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pr-gate caught what my local run did not: verify-resolver-test failed 6 cases. check-gates.py only asks whether a gate is DECLARED coherently; the resolver fixtures assert what a change actually SELECTS. The new row passed the first and failed the second — the same declaration-vs-behaviour split class F exists to catch, now applied to me. Five of the six were correct selections needing fixture updates: cli/src/limen/ io.py, mcp/src/limen_mcp/server.py, scripts/enactment-audit.py, scripts/ verify.py, and the mixed case are all Python, and a new sender can appear in any of them. The sixth was a real defect. organs-change is organs/consulting/FUNNEL-ENGINE.md — a MARKDOWN file pulling in an AST scan, because the paths said `organs/**`. Narrowing to `organs/**/*.py` fixed that and immediately opened a worse hole: cli-change and mixed-change flipped to PASSING, because in this glob dialect `**` requires at least one intervening segment, so `cli/src/limen/**/*.py` does NOT match cli/src/limen/io.py. Every direct child of a scanned root would have stopped selecting the gate — including cli/src/limen/dispatch.py, which holds a live `gh pr merge`. A coverage gate that skips the file containing the merge effector is worse than no gate. Both forms are now listed per root (`dir/*.py` and `dir/**/*.py`), which is why gates.yaml's own file_sets already spell out `cli/src/limen/*.py` separately. The cli-change fixture carries a comment naming the hazard, since it is the only thing standing between a future paths edit and silently reintroducing it, and organs-change is its negative control. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMdtji1ydxXmsyARdvmCkd
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
cli/tests/test_effector_coverage.py (1)
335-355: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
--updatepath.
write_baselineandmain(["--update"])are the only mutating code in the script, and no test exercises them. A test can monkeypatchce.BASELINEto atmp_pathfile and assert two properties: the written file round-trips throughread_baseline, and a second--updaterun produces identical bytes.💚 Proposed test
def test_update_writes_a_sorted_idempotent_baseline(ce, monkeypatch, tmp_path): """--update must be idempotent: re-pinning twice changes nothing.""" target = tmp_path / "baseline.txt" monkeypatch.setattr(ce, "BASELINE", target) monkeypatch.setattr(ce, "collect", lambda: ["C ungated-effector: b.py ...", "C ungated-effector: a.py ..."]) assert ce.main(["--update"]) == 0 first = target.read_text(encoding="utf-8") assert ce.main(["--update"]) == 0 assert target.read_text(encoding="utf-8") == first assert ce.read_baseline() == set(ce.collect())As per coding guidelines: "predicates must be runnable, self-verifying, and idempotent".
🤖 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 `@cli/tests/test_effector_coverage.py` around lines 335 - 355, Add a test near the existing effector coverage tests for the --update path, using a tmp_path file patched into ce.BASELINE and a deterministic collect result containing unsorted entries. Assert ce.main(["--update"]) succeeds, the written file round-trips through ce.read_baseline() to match ce.collect(), and a second update produces identical bytes.Source: Coding guidelines
scripts/check-effectors.py (1)
65-65: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the yaml import fail-open.
scripts/check-effectors.pyimportsyamlbefore applying thescripts/**fail-open rule for missing dependencies. If PyYAML is absent, the predicate exits with a traceback. CatchImportError/ModuleNotFoundErrorand print a readable skip beforesys.exit(0).🤖 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/check-effectors.py` at line 65, Update the top-level yaml import in scripts/check-effectors.py to catch ImportError or ModuleNotFoundError, print a readable skip message, and exit successfully with sys.exit(0) so the scripts/** predicate remains fail-open when PyYAML is unavailable.Source: Path instructions
🤖 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/check-effectors.py`:
- Around line 313-329: Make all remaining text I/O in read_baseline and
write_baseline use UTF-8 by passing encoding="utf-8" to BASELINE.read_text() and
BASELINE.write_text(); also update collect at scripts/check-effectors.py lines
336-337 to pass encoding="utf-8" to REGISTRY.read_text().
- Around line 38-41: Update the docstring in the class that describes the
ratcheted baseline to state 24 known class C sites instead of 17; leave the
surrounding explanation and implementation unchanged.
- Around line 191-194: Update the AST walk around _gh_write_verb and
_leading_constants to process both ast.List and ast.Tuple nodes, preserving the
existing verb detection for each node’s .elts. Add a negative-control test in
cli/tests/test_effector_coverage.py covering tuple-form subprocess argv so the
scanner’s CANNOT GROW coverage includes that syntax.
- Around line 355-362: Filter findings to only include class C before passing
them to write_baseline in the `if args.update:` block, since the docstring and
test_the_live_registry_has_no_declaration_or_pattern_findings assert that only
class C should be baselined. Additionally, add validation to fail the script
when class A or B findings are encountered, regardless of whether update mode is
active, to prevent those findings from being pinned to the baseline or masked by
a corrupted baseline file.
---
Nitpick comments:
In `@cli/tests/test_effector_coverage.py`:
- Around line 335-355: Add a test near the existing effector coverage tests for
the --update path, using a tmp_path file patched into ce.BASELINE and a
deterministic collect result containing unsorted entries. Assert
ce.main(["--update"]) succeeds, the written file round-trips through
ce.read_baseline() to match ce.collect(), and a second update produces identical
bytes.
In `@scripts/check-effectors.py`:
- Line 65: Update the top-level yaml import in scripts/check-effectors.py to
catch ImportError or ModuleNotFoundError, print a readable skip message, and
exit successfully with sys.exit(0) so the scripts/** predicate remains fail-open
when PyYAML is unavailable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e15e2a3f-8a8a-41d5-aaec-f6afa25b7362
📒 Files selected for processing (5)
.github/workflows/pr-gate.ymlcli/tests/test_effector_coverage.pyinstitutio/governance/gates.yamlinstitutio/governance/ungated-effectors-baseline.txtscripts/check-effectors.py
| This class enumerates those sites mechanically and ratchets them. It does not pretend the | ||
| 17 known sites can be fixed at once; it pins them in a baseline so the surface is VISIBLE | ||
| and CANNOT GROW. A newly-added ungated sender is a red check, not an archaeology session | ||
| six months later. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the site count in the docstring.
The baseline pins 24 class C findings. 17 is the grep under-count described in cli/tests/test_effector_coverage.py, not the number of known sites.
📝 Proposed fix
This class enumerates those sites mechanically and ratchets them. It does not pretend the
- 17 known sites can be fixed at once; it pins them in a baseline so the surface is VISIBLE
+ 24 known sites can be fixed at once; it pins them in a baseline so the surface is VISIBLE
and CANNOT GROW. A newly-added ungated sender is a red check, not an archaeology sessionAs per coding guidelines: "Before drawing conclusions from datasets, state the exact scope, date/window boundaries, record direction, filters, and total record count".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| This class enumerates those sites mechanically and ratchets them. It does not pretend the | |
| 17 known sites can be fixed at once; it pins them in a baseline so the surface is VISIBLE | |
| and CANNOT GROW. A newly-added ungated sender is a red check, not an archaeology session | |
| six months later. | |
| This class enumerates those sites mechanically and ratchets them. It does not pretend the | |
| 24 known sites can be fixed at once; it pins them in a baseline so the surface is VISIBLE | |
| and CANNOT GROW. A newly-added ungated sender is a red check, not an archaeology session |
🤖 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/check-effectors.py` around lines 38 - 41, Update the docstring in the
class that describes the ratcheted baseline to state 24 known class C sites
instead of 17; leave the surrounding explanation and implementation unchanged.
Source: Coding guidelines
| if isinstance(node, ast.List): | ||
| verb = _gh_write_verb(_leading_constants(node)) | ||
| if verb: | ||
| found.add(verb) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A tuple argv escapes the class C scanner.
subprocess.run(("gh", "pr", "merge", pr, "--squash")) is accepted by subprocess, but the walk matches ast.List only. A sender written in tuple form is therefore invisible to the ratchet, which defeats the "CANNOT GROW" guarantee for that shape. ast.Tuple exposes the same .elts, so widening the match keeps the check syntactic.
🐛 Proposed fix
- if isinstance(node, ast.List):
+ if isinstance(node, (ast.List, ast.Tuple)):
verb = _gh_write_verb(_leading_constants(node))
if verb:
found.add(verb)Widen the annotation as well:
-def _leading_constants(node: ast.List) -> list[str]:
+def _leading_constants(node: ast.List | ast.Tuple) -> list[str]:Add a matching negative control to cli/tests/test_effector_coverage.py.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isinstance(node, ast.List): | |
| verb = _gh_write_verb(_leading_constants(node)) | |
| if verb: | |
| found.add(verb) | |
| if isinstance(node, (ast.List, ast.Tuple)): | |
| verb = _gh_write_verb(_leading_constants(node)) | |
| if verb: | |
| found.add(verb) |
🤖 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/check-effectors.py` around lines 191 - 194, Update the AST walk
around _gh_write_verb and _leading_constants to process both ast.List and
ast.Tuple nodes, preserving the existing verb detection for each node’s .elts.
Add a negative-control test in cli/tests/test_effector_coverage.py covering
tuple-form subprocess argv so the scanner’s CANNOT GROW coverage includes that
syntax.
| def read_baseline() -> set[str]: | ||
| if not BASELINE.is_file(): | ||
| return set() | ||
| return { | ||
| line.strip() for line in BASELINE.read_text().splitlines() if line.strip() and not line.lstrip().startswith("#") | ||
| } | ||
|
|
||
|
|
||
| def write_baseline(findings: list[str]) -> None: | ||
| header = ( | ||
| "# ungated-effectors-baseline — outward actions performed in-process, where a\n" | ||
| "# PreToolUse(Bash) hook cannot reach them. Known and owned rather than silently\n" | ||
| "# tolerated. The gate fails on any NEW finding; this list may only shrink.\n" | ||
| "# Route the sender through the receipt check, then run:\n" | ||
| "# python3 scripts/check-effectors.py --update\n" | ||
| ) | ||
| BASELINE.write_text(header + "\n".join(sorted(findings)) + ("\n" if findings else "")) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Locale-dependent text I/O in scripts/check-effectors.py. Three Path text calls omit encoding, so they use the locale preferred encoding. The finding strings, the baseline header, and the registry comments all contain non-ASCII characters, so a C/POSIX locale or a non-UTF-8 Windows code page makes the predicate raise instead of report. scan_file at line 182 already pins UTF-8; apply the same treatment to the remaining call sites.
scripts/check-effectors.py#L313-L329: passencoding="utf-8"toBASELINE.read_text()inread_baselineand toBASELINE.write_text()inwrite_baseline.scripts/check-effectors.py#L336-L337: passencoding="utf-8"toREGISTRY.read_text()incollect.
As per path instructions: "Fleet scripts must be fail-open, idempotent, and offline-safe".
📍 Affects 1 file
scripts/check-effectors.py#L313-L329(this comment)scripts/check-effectors.py#L336-L337
🤖 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/check-effectors.py` around lines 313 - 329, Make all remaining text
I/O in read_baseline and write_baseline use UTF-8 by passing encoding="utf-8" to
BASELINE.read_text() and BASELINE.write_text(); also update collect at
scripts/check-effectors.py lines 336-337 to pass encoding="utf-8" to
REGISTRY.read_text().
Source: Path instructions
| if args.update: | ||
| write_baseline(findings) | ||
| print(f"effectors: baseline updated with {len(findings)} finding(s) -> {BASELINE.relative_to(ROOT)}") | ||
| return 0 | ||
|
|
||
| baselined = read_baseline() | ||
| fresh = sorted(f for f in findings if f not in baselined) | ||
| stale = sorted(baselined - set(findings)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--update can baseline class A and class B findings, and can erase the ratchet.
The docstring at lines 48-49 states that only class C is baselined, and test_the_live_registry_has_no_declaration_or_pattern_findings asserts that A and B fail on sight. The code does not enforce that split. Two consequences:
--updatepins any A or B finding into the baseline. The next run finds it inbaselined, so a missing predicate file or an over-grouped target pattern exits 0.- If the registry is missing or unparseable,
collect()returns only the single A finding.--updatethen rewrites the baseline with that one line and drops all 24 class C entries.
Restrict the baseline to class C and always fail on A and B.
🐛 Proposed fix
findings = collect()
if args.update:
- write_baseline(findings)
- print(f"effectors: baseline updated with {len(findings)} finding(s) -> {BASELINE.relative_to(ROOT)}")
- return 0
+ blocking = [f for f in findings if not f.startswith("C ")]
+ if blocking:
+ for finding in blocking:
+ print(f"FAIL {finding}")
+ print("\neffectors: refusing to re-pin the baseline while declaration/pattern findings stand")
+ return 1
+ write_baseline(findings)
+ print(f"effectors: baseline updated with {len(findings)} finding(s) -> {BASELINE.relative_to(ROOT)}")
+ return 0
baselined = read_baseline()
- fresh = sorted(f for f in findings if f not in baselined)
+ fresh = sorted(f for f in findings if not f.startswith("C ") or f not in baselined)
stale = sorted(baselined - set(findings))Add a test that an A finding is never accepted from the baseline.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if args.update: | |
| write_baseline(findings) | |
| print(f"effectors: baseline updated with {len(findings)} finding(s) -> {BASELINE.relative_to(ROOT)}") | |
| return 0 | |
| baselined = read_baseline() | |
| fresh = sorted(f for f in findings if f not in baselined) | |
| stale = sorted(baselined - set(findings)) | |
| findings = collect() | |
| if args.update: | |
| blocking = [f for f in findings if not f.startswith("C ")] | |
| if blocking: | |
| for finding in blocking: | |
| print(f"FAIL {finding}") | |
| print("\neffectors: refusing to re-pin the baseline while declaration/pattern findings stand") | |
| return 1 | |
| write_baseline(findings) | |
| print(f"effectors: baseline updated with {len(findings)} finding(s) -> {BASELINE.relative_to(ROOT)}") | |
| return 0 | |
| baselined = read_baseline() | |
| fresh = sorted(f for f in findings if not f.startswith("C ") or f not in baselined) | |
| stale = sorted(baselined - set(findings)) |
🤖 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/check-effectors.py` around lines 355 - 362, Filter findings to only
include class C before passing them to write_baseline in the `if args.update:`
block, since the docstring and
test_the_live_registry_has_no_declaration_or_pattern_findings assert that only
class C should be baselined. Additionally, add validation to fail the script
when class A or B findings are encountered, regardless of whether update mode is
active, to prevent those findings from being pinned to the baseline or masked by
a corrupted baseline file.
![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: dfe1f265b9
ℹ️ 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".
| predicate = str(row.get("predicate") or "").strip() | ||
| if predicate: | ||
| if "{target}" not in predicate: |
There was a problem hiding this comment.
When an effector declares predicate: "", the required-key check succeeds, but this truthiness guard skips both the {target} and predicate-file validations. The gate therefore reports the registry as coherent even though preflight-receipt.py will attempt to execute an empty predicate and cannot mint a PASS receipt, effectively denying every matching outbound action. Treat an empty predicate as an A-class failure before entering this block.
Useful? React with 👍 / 👎.
| if args.update: | ||
| write_baseline(findings) |
There was a problem hiding this comment.
When --update is run while the registry has an A/B integrity failure, collect() includes that failure and this writes it into the baseline; subsequent normal runs subtract it as known debt and return success. This contradicts test_the_live_registry_has_no_declaration_or_pattern_findings, which explicitly says only C findings are baselined, and can hide defects such as a missing predicate script or unsafe target regex. Refuse updates while A/B findings exist and write only C findings.
Useful? React with 👍 / 👎.
| for index, token in enumerate(parts): | ||
| if token in GH_API_WRITE_FLAGS and index + 1 < len(parts): | ||
| method = parts[index + 1].upper() | ||
| if method in GH_API_WRITE_METHODS: | ||
| return f"gh api -X {method}" |
There was a problem hiding this comment.
When the HTTP method is supplied through a variable, _leading_constants() stops before it, so this loop sees ['gh', 'api', '-X'] and returns no finding. The existing scripts/setup-rulesets.py:80 uses exactly ['gh', 'api', '-X', method, ...] for its documented mutating endpoint, meaning those permission/ruleset writes are absent from the baseline and future senders using the same common helper shape can grow unchecked. Conservatively classify a nonconstant method after -X/--method, or resolve constant arguments at call sites.
Useful? React with 👍 / 👎.
|
|
||
| check-effectors: | ||
| command: "python3 scripts/check-effectors.py" | ||
| paths: ["institutio/governance/outbound-effectors.yaml", "institutio/governance/ungated-effectors-baseline.txt", "scripts/check-effectors.py", "scripts/hooks/outbound-preflight-guard.py", "scripts/*.py", "cli/src/limen/**", "mcp/**", "ianva/**", "organs/**"] |
There was a problem hiding this comment.
When a sender is added under a nested production directory such as scripts/lib/, this path set does not select check-effectors: the resolver documents that * does not cross /, while _iter_python_files() recursively scans all of scripts/. Such a PR can therefore merge through scoped verification without the promised new-sender check; use scripts/** or enumerate every recursively scanned production path.
AGENTS.md reference: AGENTS.md:L142-L149
Useful? React with 👍 / 👎.
| for pattern in matches: | ||
| try: | ||
| re.compile(str(pattern)) |
There was a problem hiding this comment.
When YAML supplies a numeric or boolean match value, coercing it with str() lets this check pass, but the guard later calls re.search(pattern, ...) with the original non-string object. That raises TypeError, and the guard's top-level exception handler exits 0 without emitting a deny decision, so a malformed registry row disables enforcement instead of failing closed; the same mismatch exists for target.pattern, which is coerced here but passed directly to re.finditer() by the guard. Validate that both regex fields are strings rather than compiling coerced values.
Useful? React with 👍 / 👎.
| if isinstance(node, ast.List): | ||
| verb = _gh_write_verb(_leading_constants(node)) | ||
| if verb: | ||
| found.add(verb) |
There was a problem hiding this comment.
When a baselined sender is repaired by adding the required receipt check before its existing gh call, this AST branch still reports exactly the same file-and-verb finding because it records only the command literal and never whether a gate dominates its execution. Running the documented --update therefore keeps the entry, while manually removing it makes the unchanged finding fresh and fails CI, so the advertised shrink-only worklist cannot record an in-place repair. Add a mechanically verifiable gated-call shape or an explicit reviewed disposition that lets a routed sender stop reproducing.
Useful? React with 👍 / 👎.
| receipts_dir = str(registry.get("receipts_dir") or "") | ||
| if not receipts_dir: | ||
| findings.append("A receipts-dir-missing: outbound-effectors.yaml declares no receipts_dir") | ||
| elif not receipts_dir.startswith("logs/"): |
There was a problem hiding this comment.
When receipts_dir is set to a path such as logs/../institutio/receipts, this prefix check accepts it even though filesystem resolution places the receipts in a tracked directory; additional .. components can escape the repository entirely. That defeats the stated invariant that runtime receipts remain under logs/ and can leave generated receipt state available for accidental commits. Resolve the candidate against ROOT and verify it is contained by the resolved ROOT / 'logs' directory.
Useful? React with 👍 / 👎.
| for path in sorted(base.rglob("*.py")): | ||
| parts = set(path.parts) | ||
| if "tests" in parts or "test" in parts or path.name.startswith("test_"): | ||
| continue | ||
| if "node_modules" in parts or ".venv" in parts or "__pycache__" in parts: | ||
| continue | ||
| files.append(path) |
There was a problem hiding this comment.
When an agent or unattended runner invokes a shell script, the preflight hook sees only the script invocation, not the gh commands executed inside it, but this scanner considers only *.py. Existing production examples include scripts/ship-docs.sh:70-82, which creates, reviews, and merges a PR, and scripts/await-pr.sh:191-199, which performs the final merge; none appear in the baseline despite having the same structural blind spot as Python subprocess calls. Include production shell sources in Class C coverage, with the same test and fixture exclusions.
Useful? React with 👍 / 👎.
1 participant
The hole
scripts/hooks/outbound-preflight-guard.pyis aPreToolUse(Bash)hook. It inspects a command string an agent is about to run — so it cannot see, and by construction can never see:No shell, no command string, no tool call. Every genuinely dangerous outward action in this estate takes that in-process form and runs unattended on the beat:
scripts/apply-visibility.pygh repo edit --visibilityscripts/autonomy-governor.pygh pr merge— a merge to main auto-deploysscripts/contributions-organ.pygh pr commenton third-party reposscripts/creds-hydrate.pygh secret set— writes Actions secretsscripts/reap-remote-branches.pygh api -X DELETE— deletes remote refsorgans/representation/representation_substrate.pysmtplib.SMTP_SSL24 sites total. The hook covers the one surface where a charter-reading model is already in the loop, and misses all the sharp ones.
The predicate
scripts/check-effectors.py— the OUTBOUND-EFFECTORS panel's parity predicate, the fifth VIGILIA-shaped registry to get one beside GATES, PARAMETERS, SENSORS and MAIL-TIERS.Class A — declaration↔disk. A
predicate:naming a file that does not exist would, because the guard fails closed inside its match, deny every matching action the moment the hook is armed. That wasgithub.comment's live state this morning until #1727.Class B — the single-capture-group bound. The guard reads
match.group(1) if match.groups() else match.group(0), so a two-group target pattern silently keeps the first and discards the second — running the predicate against the ambient repo's #N while the command addresses a different repo's #N. The registry states this in a comment; class B is what makes the comment true.Class C — in-process coverage. Enumerated by AST walk, ratcheted against
institutio/governance/ungated-effectors-baseline.txt. The 24 known sites are pinned; a new ungated sender is a red check.Why an AST walk and not a pattern
A hand grep for
subprocess.run(["gh"found 17 of the 24 — a 40% undercount.autonomy-governor.pybinds the argv to a name and runs it later;sync-hishand-issues.pyroutes through a localsh()wrapper. Walking every list literal beginning with"gh"is invariant to how the argv reaches the process.Deliberately not flagged
osascript -e 'display notification'(conducting-report.py,notify-events.py,_notify.py) — a local desktop toast, not an outward send.gh pr view/list/checks, andgh apiwithout a mutating method — these are how the preflight predicates do their job. A gate that fired on them would make "stop checking reality" the cheapest way to go green, inverting the entire point of this registry.Verification
check-effectors: no new findings (24 baselined)— and--updateis idempotent (identical SHA on re-run).check-gates: OK — 88 gates·check-sensors: OK — 66 sensors·runner-coverage: no new findings (2 baselined)gates.yaml-sensitive test file.Scope
This makes the surface visible and non-growing. It does not close it — routing the 24 senders through the receipt check is the follow-on, and the baseline is the worklist.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WMdtji1ydxXmsyARdvmCkd
Summary by CodeRabbit