S2 step 9: structural self-gate (git apply verification)#289
Merged
Conversation
…undle
python -m ownlang own-fix subscriptions gate --bundle <step8-bundle> \
--plan <validated-plan.json> --candidates <candidates.json> \
--root <pristine-source-root> --out <gate-evidence-dir>
Step 9 proves not that the patch came from OUR generator, but that it is structurally
admissible and that its SEMANTICS hold when an INDEPENDENT host Git applies it to a
pristine preimage in a hermetic throwaway repository. It never touches the real checkout,
index or config, runs no model, no o7, no analyzer, no target tests, and publishes ONE
byte-deterministic gate-result.json.
Authority model (locked amendment 1): plan + candidates are REQUIRED and the gate calls its
OWN pure validator, NOT the step 8 validate_apply_inputs() — that function reads the source
itself, which would make two reads and a TOCTOU against the single-snapshot contract. The
manifest is a claim, re-derived from the plan (actions, in candidate order) and the
hash-bound candidates (identity) and required to EQUAL the canonical projection byte for
byte; the patch, postimage and preimage are re-hashed over bytes read exactly once.
The strict authority validator (amendment 2) restates the FROZEN envelope with exact key
sets on EVERY object — including each candidate — the permission tiering (convert_acquire
only for inotify_property_changed), one type / one file, and decision order == candidate
order, because a hash proves only that two files agree with each other, never that they
obey policy.
The one snapshot boundary (amendment 3): the six external byte inputs are each read once —
reject symlink/reparse, open O_NOFOLLOW where available, fstat the handle, require a regular
file — and every hash / parse / materialization runs over those memory bytes. The throwaway
repo is built from the SAME preimage bytes that were hashed, so there is no second read and
no TOCTOU between the pre-SHA check and the tree.
The patch parser (amendment 7) accepts EXACTLY the step 8 language: one file header whose
three paths are byte-equal to the manifest's canonical rel (so a quoted/escaped or alternate
path never matches), LF-terminated records, hunks whose arithmetic is consistent and whose
old ranges lie in the preimage, context/`-`/`+` lines and the no-newline marker — and
nothing else. index/mode/rename/copy/binary/submodule/new/deleted/second-file records are
refused by construction, not by a blocklist.
The hermetic repo (amendments 5 & 6): git init → write the preimage → `git add` (a BASELINE
index, so `git diff-files` can prove exactly rel changed) → `git apply --check` → `git
apply`, all with a minimal ALLOWLIST env (no inherited GIT_*; GIT_CONFIG_NOSYSTEM,
GIT_ATTR_NOSYSTEM, empty HOME/XDG/global config) and `-c core.autocrlf=false -c core.eol=lf
-c core.safecrlf=false`, the patch fed via stdin from the memory snapshot. After apply: diff
== rel only, no untracked file, a raw worktree walk == {rel}, the index and config
byte-unchanged, and the applied bytes == postimage == manifest.post_sha256. There is
deliberately NO public --git override (amendment 4) — evidence of an INDEPENDENT apply
cannot come from a caller-supplied stand-in; a missing git is INFRASTRUCTURE / exit 2.
An empty patch (manual_review-only) is valid, not a refusal: pre == post == the pristine
bytes, Git is not run, and git_apply_check / git_apply / isolated_tree record
"not_applicable". Publication reuses the step 8 protocol: claimed unpredictable staging
under a physically-off-tree parent, one atomic rename; a refusal leaves no <out>, no staging,
and a byte-identical source tree.
Failure taxonomy (all exit 2, stable category for assertions): BUNDLE_LAYOUT, MANIFEST_SHAPE,
AUTHORITY_BINDING, HASH_MISMATCH, PRISTINE_SOURCE, PATCH_STRUCTURE, APPLY_CHECK,
APPLY_MISMATCH, ISOLATION, PUBLICATION, INFRASTRUCTURE.
Tests: tests/test_gate_patch.py (46 checks — the patch grammar, manifest shape, authority
validator and every byte-tampering case; dotnet-free, runs in the `tests` job).
tests/gate_regressions.sh + a CI step drive the real chain and the filesystem/git-real
cases: happy path, determinism, manual-only, and — via forged fixtures REBOUND so the
refusal reaches the branch under test — APPLY_CHECK vs APPLY_MISMATCH vs HASH_MISMATCH, a
symlink/extra bundle entry, a stale/symlink-escaping pristine source, and the publication
refusals, with the real checkout never touched.
_resolve_source ruling: Step 9 does NOT reach S0's case-sensitive _resolve_source (it does
not call validate_apply_inputs); it uses its own platform-aware physical verifier. That S0
finding stays a separate maintenance scope.
Steps 10-12 NOT started: no analyzer delta, no own-check/extractor re-run, no OWN001/OWN050
assertions, no fake target gate, no gate.toml, no 007 evidence, no STS clone.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
📝 WalkthroughWalkthroughAdds a structural self-gate for canonical subscription-fix bundles, exposing it through the CLI, validating bundles with hermetic git application, publishing deterministic evidence, and adding unit, end-to-end, harness, and CI coverage. ChangesStructural self-gate
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant run_gate
participant Bundle
participant Source
participant ThrowawayGit
participant Output
CLI->>run_gate: Provide bundle, plan, candidates, root, and out
run_gate->>Bundle: Read and validate manifest and patch
run_gate->>Source: Verify pristine source hash
run_gate->>ThrowawayGit: Check and apply patch
ThrowawayGit-->>run_gate: Return resulting file state
run_gate->>Output: Write gate-result.json
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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 |
![chatgpt-codex-connector[bot]](https://cdn.github.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: fbe88bfe98
ℹ️ 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".
| print(f"gate (S2 step 9): {checks - len(failures)}/{checks} checks pass") | ||
| for f in failures: | ||
| print(f" FAIL: {f}") | ||
| sys.exit(1 if failures else 0) |
There was a problem hiding this comment.
When python tests/run_tests.py auto-imports this module, this unconditional sys.exit(0) terminates the entire suite as soon as these checks pass. That skips every test module sorted after test_gate_patch.py and discards failures already accumulated by the core suite and earlier modules, so CI can report success despite unrelated regressions. Expose the expected run() -> int function and reserve SystemExit for an if __name__ == "__main__" entry point.
Useful? React with 👍 / 👎.
…rness defect)
1. THE TEST MODULE KILLED THE AGGREGATE RUNNER. test_gate_patch.py ran its checks at import
and ended with sys.exit(), so when run_tests.py imported it every module discovered
after it — and the aggregate return code — was silently dropped: a green job that had
"turned the lights off in the building". The SAME latent defect was already in the
merged step-8 test_patch_bundle.py (it sorts AFTER test_gate_patch, so it had never
actually run in the aggregate). Both now expose `def run() -> int` with no import-time
sys.exit and an `if __name__ == "__main__"` guard. A new tests/test_harness_contract.py
STATICALLY proves (via ast) that every sibling test_*.py exposes run() and never calls
sys.exit / raise SystemExit at module scope — so this class of bug cannot recur. With
the fix the suite now reaches its summary and both modules gate: step 8 = 79 checks,
step 9 = 63, harness = 29, all under one run_tests return code (verified: an injected
failure flips rc to 1).
2. FULL FROZEN-CANDIDATE VALIDATION. The exact key set was held but not every value. The
gate now validates every string field (diagnostic_code, enclosing_member, event_identity,
...), occurrence_ordinal as a non-negative int, and the full teardown block — exact keys,
status in {none,exact,ambiguous}, and each teardown candidate's exact keys with its 6-int
span. A re-hashed bundle with `event_identity: 123` or a malformed teardown now refuses
at AUTHORITY_BINDING (fixtures added for event_identity/occurrence_ordinal wrong type,
teardown extra key / unknown status / malformed candidate / bad span shape).
3. EXACT DIRECTORY LAYOUT + BUNDLE-ROOT lstat. The layout walk returned only files, so an
extra/hidden/nested EMPTY directory rode through, and --bundle was realpath'd first so a
symlinked bundle root was accepted. The walk now returns (dirs, files) and the postimage
subtree must equal EXACTLY rel's ancestor dirs plus rel; --bundle is lstat'd before
realpath and a symlink/reparse root is refused. Fixtures: extra/hidden/nested empty dir,
symlinked bundle root — all BUNDLE_LAYOUT.
4. A TRULY CLAIMED WORKDIR + RE-PROVEN PUBLICATION. _prepare_out only checked a name; the
dir was created later with a plain makedirs. A new _claim_workdir creates the
unpredictable dir immediately (mode 0700 on POSIX, in the mkdir itself) and PROVES it:
not a link/reparse, realpath == itself under the platform-aware comparison, empty. And
_publish re-resolves the out-dir parent, re-confirms it is off the source tree, and
re-confirms no final out exists — immediately before the atomic rename, not only at
parse time.
5. TIGHTER PATCH GRAMMAR. The no-newline marker was allowed after any seen body line and
any number of times. It is now stateful: it must sit immediately after an eligible
( / - / + line, marks the LAST line of its side(s) (a later line of a closed side is
refused), and appears at most once per side. Range bounds now cover the zero-length
(insertion) case: old_len>0 requires 1 <= old_start and old_start+old_len-1 <= preimage
lines (so a non-zero range starting at 0 is refused); old_len==0 requires
0 <= old_start <= preimage lines (so an insertion past EOF is refused rather than dying
later as APPLY_CHECK). Fixtures: marker before any line, duplicate marker, marker with
more of the same side after it, insertion past the preimage, non-zero range at 0, plus
positive cases (a well-placed marker, a top insertion).
The step-8 change is strictly the test-harness run() wrapper — no test logic, no Steps 4-8
semantics/schema touched. All other Step 9 architecture (authority model, six snapshots,
manifest schema, git environment, baseline index, evidence schema, empty-patch policy,
taxonomy) is unchanged. Steps 10-12 not started.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
|
@coderabbitai review |
1. THE HARNESS GUARD WAS TOO LOOSE. test_harness_contract.py skipped ANY top-level `if` (so `if True: sys.exit(0)` slid through) and broke traversal at the first nested FunctionDef (so a module-scope exit inside a top-level `try` was missed). The guard is rewritten as a proper recursive walk: it skips ONLY a structurally-recognised `__name__ == "__main__"` (a real ast.Compare of __name__ == "__main__"), does NOT descend into function / class / lambda scopes, and DOES descend through module-scope compound statements (if / try / with / for / while) — so an exit inside `if True:` or a top-level `try:` is caught. Added 8 guard self-tests: five that MUST be caught (bare exit, `if True:` exit, exit in a top-level try, `raise SystemExit`, exit in a for loop) and three that MUST pass (the guarded entrypoint, an exit inside a function, an exit inside a lambda). 2. NO-NEWLINE STATE WAS PER-HUNK. old_closed/new_closed reset each hunk, so a no-newline marker in the first hunk did not forbid a second hunk after it — the marker means EOF-without-newline, so nothing can follow on that side. State is now FILE-level (`saw_marker`): once any no-newline marker appears, a following hunk header is a PATCH_STRUCTURE refusal (previously it would have slipped to APPLY_CHECK — the right refusal via the wrong branch). Added a two-hunk fixture asserting PATCH_STRUCTURE. 3. CLAIM/PUBLICATION EQUALITY WAS NOT PLATFORM-AWARE, AND A POST-mkdir FAILURE LEAKED THE DIRECTORY. Both the workdir self-resolution proof and the publication parent re-proof used a raw string `realpath(x) != y`, which on case-insensitive Windows disagrees with the accepted _same_or_inside rule and could give a casing-only false refusal. A new platform-aware `_same_path` (normcase+normpath, the same rule) is now used in both. And `_claim_workdir` created the directory before the outer cleanup scope began, so a proof failure after the mkdir would strand it — the post-mkdir proofs are now wrapped so the just-created directory is removed on ANY failure before raising. Added a pure-function regression (symlink-capable platforms): a claim under a symlinked parent fails the self-resolution proof with PUBLICATION and leaves NO directory behind, plus _same_path platform cases. Nothing else in Step 9 reopened. Steps 10-12 not started. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
`if isinstance(child, ast.If) and _is_main_guard(child.test): continue` exempted the WHOLE guard, but its `else:` runs on import — so `if __name__ == '__main__': ... else: sys.exit(0)` slipped through. Only the guard's body is exempt now; its orelse is re-checked with the same per-child logic (via a synthetic Module, so a bare `raise SystemExit` there is caught as a statement). Added MUST_CATCH self-tests for sys.exit / raise SystemExit in a main-guard else and an elif. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
…a nodes Skipping a FunctionDef/AsyncFunctionDef/Lambda/ClassDef subtree missed code that runs at IMPORT: a class body, class bases/keywords, decorators, argument defaults and lambda defaults all execute while the module is imported — any of them can sys.exit and abort run_tests' importlib.import_module before the aggregate rc is collected. The scanner now prunes ONLY the genuinely deferred subtrees (function/lambda BODY, the main-guard body), checks the node itself before its children (so a decorator/default that IS sys.exit is caught), scans class bodies normally, and models 'from __future__ import annotations' so eager annotations are checked but stringified ones are not. Added MUST_CATCH for class body / base / decorator, function default + decorator, keyword-only default, lambda default and an eager annotation; MUST_PASS for a class method body and a stringified annotation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@ownlang/__main__.py`:
- Around line 649-652: Update the primary cmd_own_fix usage text to include the
newly supported gate subcommand alongside candidates, render, validate-plan, and
apply. Keep the unknown-subcommand error text and _cmd_gate dispatch unchanged.
In `@ownlang/fix_gate.py`:
- Around line 584-610: Validate new-side hunk coordinates in the hunk-processing
loop instead of discarding _new_start. Track the cumulative line delta from
prior hunks, require each new_start to match the expected position (including
rejecting invalid zero or inconsistent offsets), and update that delta from each
hunk’s old_len and new_len. Apply the same validation to the corresponding logic
around the later hunk-processing block.
In `@tests/gate_regressions.sh`:
- Around line 231-237: The pristine-source regression test currently supplies a
root that cannot reach the escaping symlink. Update the `refuse` invocation for
“source through a symlinked dir (escape)” to pass `$T/symroot` as the root, so
traversal encounters `$T/symroot/frontend` and validates the symlink escape
rather than a missing source.
In `@tests/test_harness_contract.py`:
- Around line 85-129: Update the import-termination analysis around
_is_systemexit and _is_sys_exit to track aliases introduced by import sys as s,
from sys import exit, and aliased SystemExit imports, then recognize those
aliases wherever termination expressions are checked. Extend
_SELFTEST_MUST_CATCH with representative aliased sys.exit, imported exit, and
imported SystemExit cases while preserving existing guarded and function-scope
behavior.
- Around line 145-159: Update the run() validation in the test discovery loop to
inspect the module-level FunctionDef signature and reject any required
parameters that tests/run_tests.py cannot satisfy when calling mod.run().
Preserve acceptance for zero-argument run() functions and any parameters
explicitly supported by the aggregate runner’s special-call contract, reporting
invalid signatures through failures.
🪄 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: 700bbaad-7356-4c09-b887-fa18de98cc7d
📒 Files selected for processing (7)
.github/workflows/ci.ymlownlang/__main__.pyownlang/fix_gate.pytests/gate_regressions.shtests/test_gate_patch.pytests/test_harness_contract.pytests/test_patch_bundle.py
Comment on lines
+649
to
+652
| if verb == "gate": | ||
| return _cmd_gate(args) | ||
| print(f"own-fix: unknown subcommand {verb!r} " | ||
| "(candidates | render | validate-plan | apply)", file=sys.stderr) | ||
| "(candidates | render | validate-plan | apply | gate)", file=sys.stderr) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add gate to the primary cmd_own_fix usage text.
Lines 635–638 still advertise only {candidates|render|validate-plan|apply}, so the missing-verb path incorrectly omits the new command.
Proposed fix
- """`own-fix subscriptions {candidates|render|validate-plan|apply} ...`."""
+ """`own-fix subscriptions {candidates|render|validate-plan|apply|gate} ...`."""
@@
- "{candidates|render|validate-plan|apply} ...", file=sys.stderr)
+ "{candidates|render|validate-plan|apply|gate} ...", file=sys.stderr)🤖 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 `@ownlang/__main__.py` around lines 649 - 652, Update the primary cmd_own_fix
usage text to include the newly supported gate subcommand alongside candidates,
render, validate-plan, and apply. Keep the unknown-subcommand error text and
_cmd_gate dispatch unchanged.
Comment on lines
+584
to
+610
| prev_old_end = 0 | ||
| saw_hunk = False | ||
| saw_marker = False # FILE-level: a no-newline marker means EOF-without-newline, so no | ||
| while i < len(recs): # further hunk may follow it — the marked line is the file's last. | ||
| rec = recs[i] | ||
| if not (rec.startswith(b"@@ -") and rec.endswith(b" @@")): | ||
| raise GateError(PATCH_STRUCTURE, f"patch: expected a hunk header, got {rec[:40]!r}") | ||
| if saw_marker: | ||
| raise GateError(PATCH_STRUCTURE, "patch: a hunk after a no-newline-at-EOF marker") | ||
| body = rec[len(b"@@ -"):-len(b" @@")] | ||
| try: | ||
| old_part, new_part = body.split(b" +", 1) | ||
| except ValueError as exc: | ||
| raise GateError(PATCH_STRUCTURE, "patch: malformed hunk header") from exc | ||
| old_start, old_len = _range(old_part) | ||
| _new_start, new_len = _range(new_part) | ||
| # Range bounds, including the zero-length (pure-insertion) case: a 0-length old | ||
| # range names the line BEFORE it (0..pre_lines); a non-zero range is 1-based and | ||
| # must lie within the preimage. | ||
| if old_len > 0: | ||
| if old_start < 1 or old_start + old_len - 1 > pre_lines: | ||
| raise GateError(PATCH_STRUCTURE, "patch: a hunk range is outside the preimage") | ||
| elif old_start > pre_lines: | ||
| raise GateError(PATCH_STRUCTURE, "patch: an insertion range is past the preimage") | ||
| if old_start < prev_old_end: | ||
| raise GateError(PATCH_STRUCTURE, "patch: hunks not increasing / non-overlapping") | ||
| prev_old_end = old_start + old_len |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the new-side hunk coordinates.
new_start is discarded, so ranges such as +0,N or offsets inconsistent with earlier hunks pass PATCH_STRUCTURE. This can certify a noncanonical patch or defer its refusal to the wrong gate. Derive the expected new position from the cumulative line delta.
Proposed fix
i = 3
prev_old_end = 0
+ line_delta = 0
saw_hunk = False
@@
old_start, old_len = _range(old_part)
- _new_start, new_len = _range(new_part)
+ new_start, new_len = _range(new_part)
+ expected_new_start = old_start + line_delta
+ if old_len == 0:
+ expected_new_start += 1
+ elif new_len == 0:
+ expected_new_start -= 1
+ if new_start != expected_new_start:
+ raise GateError(PATCH_STRUCTURE, "patch: inconsistent new-side hunk range")
@@
if minus == 0 and plus == 0:
raise GateError(PATCH_STRUCTURE, "patch: a hunk changes nothing")
+ line_delta += new_len - old_lenAlso applies to: 652-653
🤖 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 `@ownlang/fix_gate.py` around lines 584 - 610, Validate new-side hunk
coordinates in the hunk-processing loop instead of discarding _new_start. Track
the cumulative line delta from prior hunks, require each new_start to match the
expected position (including rejecting invalid zero or inconsistent offsets),
and update that delta from each hunk’s old_len and new_len. Apply the same
validation to the corresponding logic around the later hunk-processing block.
Comment on lines
+231
to
+237
| # a source reached through a symlinked directory that escapes the root. | ||
| rm -rf "$T/symroot"; mkdir -p "$T/symroot/outside" | ||
| cp -r "$REPO/frontend" "$T/symroot/outside/frontend" | ||
| ln -s outside/frontend "$T/symroot/frontend" | ||
| mkdir -p "$T/symroot/real" | ||
| refuse "source through a symlinked dir (escape)" "$T/g_symsrc" "$T/bundle" PRISTINE_SOURCE \ | ||
| "$T/plan.json" "$T/candidates.json" "$T/symroot/real" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the pristine-source test actually traverse the escaping symlink.
The supplied root is $T/symroot/real, but the symlink is $T/symroot/frontend. The current test therefore exercises “source not found” while still matching the broad PRISTINE_SOURCE category.
Proposed fix
-rm -rf "$T/symroot"; mkdir -p "$T/symroot/outside"
-cp -r "$REPO/frontend" "$T/symroot/outside/frontend"
-ln -s outside/frontend "$T/symroot/frontend"
-mkdir -p "$T/symroot/real"
+rm -rf "$T/symroot" "$T/symroot-outside"
+mkdir -p "$T/symroot" "$T/symroot-outside"
+cp -r "$REPO/frontend" "$T/symroot-outside/frontend"
+ln -s ../symroot-outside/frontend "$T/symroot/frontend"
refuse "source through a symlinked dir (escape)" "$T/g_symsrc" "$T/bundle" PRISTINE_SOURCE \
- "$T/plan.json" "$T/candidates.json" "$T/symroot/real"
+ "$T/plan.json" "$T/candidates.json" "$T/symroot"📝 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.
Suggested change
| # a source reached through a symlinked directory that escapes the root. | |
| rm -rf "$T/symroot"; mkdir -p "$T/symroot/outside" | |
| cp -r "$REPO/frontend" "$T/symroot/outside/frontend" | |
| ln -s outside/frontend "$T/symroot/frontend" | |
| mkdir -p "$T/symroot/real" | |
| refuse "source through a symlinked dir (escape)" "$T/g_symsrc" "$T/bundle" PRISTINE_SOURCE \ | |
| "$T/plan.json" "$T/candidates.json" "$T/symroot/real" | |
| # a source reached through a symlinked directory that escapes the root. | |
| rm -rf "$T/symroot" "$T/symroot-outside" | |
| mkdir -p "$T/symroot" "$T/symroot-outside" | |
| cp -r "$REPO/frontend" "$T/symroot-outside/frontend" | |
| ln -s ../symroot-outside/frontend "$T/symroot/frontend" | |
| refuse "source through a symlinked dir (escape)" "$T/g_symsrc" "$T/bundle" PRISTINE_SOURCE \ | |
| "$T/plan.json" "$T/candidates.json" "$T/symroot" |
🤖 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 `@tests/gate_regressions.sh` around lines 231 - 237, The pristine-source
regression test currently supplies a root that cannot reach the escaping
symlink. Update the `refuse` invocation for “source through a symlinked dir
(escape)” to pass `$T/symroot` as the root, so traversal encounters
`$T/symroot/frontend` and validates the symlink escape rather than a missing
source.
Comment on lines
+85
to
+129
| def _is_systemexit(exc: ast.expr | None) -> bool: | ||
| if isinstance(exc, ast.Call): | ||
| exc = exc.func | ||
| return isinstance(exc, ast.Name) and exc.id == "SystemExit" | ||
|
|
||
|
|
||
| def _is_sys_exit(func: ast.expr) -> bool: | ||
| return (isinstance(func, ast.Attribute) and func.attr == "exit" | ||
| and isinstance(func.value, ast.Name) and func.value.id == "sys") | ||
|
|
||
|
|
||
| _SELFTEST_MUST_CATCH = ( | ||
| ("bare sys.exit at module scope", "import sys\nsys.exit(0)\n"), | ||
| ("exit inside `if True:`", "import sys\nif True:\n sys.exit(0)\n"), | ||
| ("exit inside a top-level try", | ||
| "import sys\ntry:\n sys.exit(0)\nexcept Exception:\n pass\n"), | ||
| ("raise SystemExit at module scope", "raise SystemExit(1)\n"), | ||
| ("exit inside a for loop", "import sys\nfor _ in range(1):\n sys.exit(0)\n"), | ||
| ("sys.exit in a main-guard else", | ||
| "import sys\nif __name__ == '__main__':\n pass\nelse:\n sys.exit(0)\n"), | ||
| ("raise SystemExit in a main-guard else", | ||
| "if __name__ == '__main__':\n pass\nelse:\n raise SystemExit(1)\n"), | ||
| ("exit in a main-guard elif", | ||
| "import sys\nif __name__ == '__main__':\n pass\nelif True:\n sys.exit(0)\n"), | ||
| # Declarations that run code AT IMPORT — the trapdoors that skipping a whole | ||
| # FunctionDef / ClassDef / Lambda node would miss. | ||
| ("exit in a class body", "import sys\nclass C:\n sys.exit(7)\n"), | ||
| ("exit in a class base", "import sys\nclass C(sys.exit(7)):\n pass\n"), | ||
| ("exit in a class decorator", "import sys\n@sys.exit(7)\nclass C:\n pass\n"), | ||
| ("exit in a function default", "import sys\ndef f(value=sys.exit(7)):\n pass\n"), | ||
| ("exit in a function decorator", "import sys\n@sys.exit(7)\ndef f():\n pass\n"), | ||
| ("exit in a keyword-only default", | ||
| "import sys\ndef f(*, value=sys.exit(7)):\n pass\n"), | ||
| ("exit in a lambda default", "import sys\nf = lambda value=sys.exit(7): None\n"), | ||
| ("exit in an eager annotation", "import sys\ndef f(x: sys.exit(7)):\n pass\n"), | ||
| ) | ||
| _SELFTEST_MUST_PASS = ( | ||
| ("guarded entrypoint", "import sys\nif __name__ == '__main__':\n sys.exit(0)\n"), | ||
| ("exit inside a function", "import sys\ndef run():\n sys.exit(0)\n"), | ||
| ("exit inside a lambda", "f = lambda: __import__('sys').exit(0)\n"), | ||
| ("exit inside a class method body", | ||
| "import sys\nclass C:\n def m(self):\n sys.exit(0)\n"), | ||
| ("stringified annotation under future-annotations", | ||
| "from __future__ import annotations\nimport sys\ndef f(x: sys.exit(7)):\n pass\n"), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make import-termination detection alias-aware.
import sys as s; s.exit(0), from sys import exit; exit(0), and aliased SystemExit bypass these predicates while still terminating module import. Track relevant import aliases and add self-tests for them.
🤖 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 `@tests/test_harness_contract.py` around lines 85 - 129, Update the
import-termination analysis around _is_systemexit and _is_sys_exit to track
aliases introduced by import sys as s, from sys import exit, and aliased
SystemExit imports, then recognize those aliases wherever termination
expressions are checked. Extend _SELFTEST_MUST_CATCH with representative aliased
sys.exit, imported exit, and imported SystemExit cases while preserving existing
guarded and function-scope behavior.
Comment on lines
+145
to
+159
| for fname in sorted(os.listdir(_HERE)): | ||
| if not (fname.startswith("test_") and fname.endswith(".py")): | ||
| continue | ||
| if fname == os.path.basename(__file__): | ||
| continue | ||
| checks += 1 | ||
| src = open(os.path.join(_HERE, fname), encoding="utf-8").read() | ||
| tree = ast.parse(src) | ||
| has_run = any(isinstance(n, ast.FunctionDef) and n.name == "run" | ||
| for n in ast.iter_child_nodes(tree)) | ||
| if not has_run: | ||
| failures.append(f"{fname}: has no module-level run()") | ||
| if _module_scope_exit(tree): | ||
| failures.append(f"{fname}: calls sys.exit/raise SystemExit at import scope " | ||
| "(would short-circuit the aggregate runner)") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate that discovered run() functions are callable by the aggregate runner.
This accepts def run(required): ..., but tests/run_tests.py invokes non-special modules as mod.run(), causing a TypeError and aborting discovery. Reject required parameters or share the runner’s special-call contract.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 150-150: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(_HERE, fname), encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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 `@tests/test_harness_contract.py` around lines 145 - 159, Update the run()
validation in the test discovery loop to inspect the module-level FunctionDef
signature and reject any required parameters that tests/run_tests.py cannot
satisfy when calling mod.run(). Preserve acceptance for zero-argument run()
functions and any parameters explicitly supported by the aggregate runner’s
special-call contract, reporting invalid signatures through failures.
Source: Coding guidelines
Two unsoundnesses in the harness guard, both fixed: (1) The guard ran too late. test_harness_contract.py was itself an auto-discovered module, so an earlier-sorting test_*.py that sys.exit()s at import ended the process before the scan ran. The check is now a PREFLIGHT (tests/_preflight.py) that run_tests.run() calls FIRST, over the source, before importing any test module — so an offender cannot sort ahead of it. A violation prints PREFLIGHT FAIL and the runner returns 1. (2) Pruning function/lambda/class bodies was unsound: a body is reached at import when a module-scope call or an immediately-invoked lambda runs it. The invariant is now strict and purely LOCATION-based (no call-graph analysis): sys.exit / raise SystemExit is allowed ONLY inside a standalone guard body — nowhere else, function bodies included. Regressions: MUST_CATCH now includes a helper called at module scope, an IIFE, an uncalled function body and a class method body; the MUST_PASS cases that permitted function-body exits are removed. A load-bearing regression builds a throwaway tests dir whose earliest-sorting file exits at import and confirms the preflight reports it (plus a helper-invoked offender and a clean guarded module). Verified end to end: injecting an earlier-sorting offender into the real tests dir makes run_tests return 1, not a silent 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/run_tests.py (1)
1091-1096: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCatch exceptions during test execution to prevent discovery aborts.
If a discovered
run()function requires arguments (e.g.,def run(required): ...), invoking it raises aTypeErrorthat crashes the discovery loop. Additionally, any unhandled test exception will also abort the run and silently drop subsequent tests. Wrap the invocation in atry...except Exceptionblock to isolate module failures and continue running the suite. As per previous review comments, validate that discoveredrun()functions are callable by the aggregate runner without causing aTypeError.🛠️ Proposed fix to isolate test executions
- runner = special.get(name) - if runner is None and not hasattr(mod, "run"): - print(f"run_tests: FAIL — {name} has no run(); every test_*.py must expose one") - module_rcs.append(1) - continue - module_rcs.append(runner(mod) if runner is not None else mod.run()) + runner = special.get(name) + if runner is None and not hasattr(mod, "run"): + print(f"run_tests: FAIL — {name} has no run(); every test_*.py must expose one") + module_rcs.append(1) + continue + try: + module_rcs.append(runner(mod) if runner is not None else mod.run()) + except Exception as e: + print(f"run_tests: FAIL — {name}.run() aborted: {type(e).__name__}: {e}") + module_rcs.append(1)🤖 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 `@tests/run_tests.py` around lines 1091 - 1096, Wrap the per-module invocation in the discovery loop around runner(mod) or mod.run() with try/except Exception, record a failure in module_rcs, and continue processing subsequent tests when execution raises, including TypeError from incompatible run() signatures. Validate discovered run callables before invocation so functions requiring unsupported arguments are treated as module failures rather than aborting the suite.
🤖 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 `@tests/_preflight.py`:
- Around line 34-42: Make the import-termination detection used by
_is_systemexit and _is_sys_exit alias-aware: track aliases introduced by “import
sys as …” and “from sys import exit”, and recognize aliased SystemExit
references, while preserving existing direct-call detection. Also cover relevant
built-ins such as exit() and quit() if they are part of the preflight
termination cases, and verify each detected pattern through the existing
preflight analysis.
---
Outside diff comments:
In `@tests/run_tests.py`:
- Around line 1091-1096: Wrap the per-module invocation in the discovery loop
around runner(mod) or mod.run() with try/except Exception, record a failure in
module_rcs, and continue processing subsequent tests when execution raises,
including TypeError from incompatible run() signatures. Validate discovered run
callables before invocation so functions requiring unsupported arguments are
treated as module failures rather than aborting the suite.
🪄 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: 4dfced31-369d-4c08-8e62-296f68e11876
📒 Files selected for processing (3)
tests/_preflight.pytests/run_tests.pytests/test_harness_contract.py
Comment on lines
+34
to
+42
| def _is_systemexit(exc: ast.expr | None) -> bool: | ||
| if isinstance(exc, ast.Call): | ||
| exc = exc.func | ||
| return isinstance(exc, ast.Name) and exc.id == "SystemExit" | ||
|
|
||
|
|
||
| def _is_sys_exit(func: ast.expr) -> bool: | ||
| return (isinstance(func, ast.Attribute) and func.attr == "exit" | ||
| and isinstance(func.value, ast.Name) and func.value.id == "sys") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚖️ Poor tradeoff
Make import-termination detection alias-aware.
import sys as s; s.exit(0), from sys import exit; exit(0), and aliased SystemExit bypass these predicates while still terminating module import. Consider tracking relevant import aliases (or covering built-ins like exit() and quit()) to prevent bypasses of the preflight check. As per previous review comments, verify each finding and track relevant aliases.
🤖 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 `@tests/_preflight.py` around lines 34 - 42, Make the import-termination
detection used by _is_systemexit and _is_sys_exit alias-aware: track aliases
introduced by “import sys as …” and “from sys import exit”, and recognize
aliased SystemExit references, while preserving existing direct-call detection.
Also cover relevant built-ins such as exit() and quit() if they are part of the
preflight termination cases, and verify each detected pattern through the
existing preflight analysis.
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
1 participant
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.
See commit message. Step 9 gates a canonical step 8 bundle: strict bundle/manifest validation, self-contained authority re-validation (plan + candidates, NOT validate_apply_inputs), single-snapshot byte reads, the frozen step 8 patch grammar, and an INDEPENDENT
git apply --check→git applyin a hermetic throwaway repo that reproduces the postimage byte for byte. Publishes one byte-deterministic gate-result.json; the real checkout/index/config are never touched. Steps 10-12 not started.🤖 Generated with Claude Code
Summary by CodeRabbit
own-fix subscriptions gateCLI command to run the gate and publishgate-result.json.