fix(branches): seal GraphQL node-id lookups at execution so check mode never issues them - #75
Merged
Merged
Conversation
…e never issues them Since 752b190 the branches plan() resolved the repository's and the bypass actors' GraphQL node ids while planning, to build the create/update rule payload. Those lookups tolerate no NOT_FOUND, so a fine-grained token denied Administration failed the section on a READ in check mode, against the section's declared "absent" posture (a denied token surfaces at the first write). The nightly fuzz caught it on two seeds. The lookups now seal at execution: ruleVariables() builds a Late thunk when actor or late ids are needed and a literal value otherwise, and a literal entry's first operation carries a `before` hook (a new PlannedOp facet the executor runs ahead of the request) that resolves the actors before the PUT replaces the live protection, so a misspelled actor still fails with live state untouched. The class is closed at the contract: a read declared `phase: "execution"` (REST or GraphQL) exposes its bound helpers Gated behind the ExecTools token only a thunk holds, so a plan() body cannot call it; planningReads() excludes such reads from readGating(), denialPosture(), the write-only check note, and the oracle's no-read set, and denialPosture() rejects a primaryRead on one. The e2e mock treats an execution-phase read in check mode as a contract violation. Harness: an oracle mismatch on a run the runner passed now dumps a report.md too (failureArtifacts), merged into the runner's own report when both failed, so the nightly's fuzz-issue action has a report to file instead of "no failure report". The curated scenario branches-administration-denied-check-drift locks the semantics; it fails on the pre-fix code. Fixes #73
There was a problem hiding this comment.
🟡 Changes recommended
Per-entry actor validation allows earlier branch mutations before a later invalid actor aborts the section.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes issue #73 by deferring branch-protection node-ID lookups until apply execution, preventing check mode from issuing them.
Changes:
- Adds execution-gated reads and pre-request hooks.
- Defers branch actor and repository ID resolution.
- Improves mock enforcement and fuzz failure artifacts.
File summaries
| File | Description |
|---|---|
src/engine/execute.ts |
Executes pre-request hooks. |
src/sections/contract/endpoints.ts |
Adds REST execution phases. |
src/sections/contract/graphql.ts |
Adds GraphQL execution phases. |
src/sections/contract/module.ts |
Distinguishes planning reads. |
src/sections/contract/plan.ts |
Adds gated reads and hooks. |
src/sections/branches/index.ts |
Defers node-ID resolution. |
src/sections/branches/branches.test.ts |
Tests deferred branch lookups. |
src/sections/branches/scenarios/branches-administration-denied-check-drift.yml |
Adds denial regression scenario. |
test/engine/execute.test.ts |
Tests hook execution and failure. |
test/sections/contract.test.ts |
Tests execution-read contracts. |
test/sections/registry.test.ts |
Validates planning-read metadata. |
test/sections/plan-idempotence.ts |
Includes hooks in plan identity. |
test/sections/plan-idempotence.test.ts |
Tests hook identity behavior. |
test/e2e/mock/routes.ts |
Enforces check-mode read barriers. |
test/e2e/mock/server.test.ts |
Tests REST barrier behavior. |
test/e2e/mock/graphql-pipeline.test.ts |
Tests GraphQL barrier behavior. |
test/e2e/oracle.ts |
Excludes execution-only reads. |
test/e2e/fuzz.ts |
Produces artifacts for oracle failures. |
test/e2e/runner.ts |
Adds merged failure reports. |
test/e2e/runner.test.ts |
Tests failure artifact generation. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…n's first write The before hook resolved actors per entry, so with an earlier entry drifting and a later one naming a misspelled actor, the earlier PUT landed before the later hook threw. ruleVariables() now records every actor list it seals into a mutation on the run state, and plan() attaches one prelude to the plan's first operation that resolves them all, whichever entry they belong to; the mutations' thunks find the ids cached. The generated coverage prose says actors resolve when apply executes, not at plan time.
There was a problem hiding this comment.
🟢 Approval recommended
The execution-phase contract, branch ordering guarantees, mock barriers, and artifact reporting are consistently implemented and covered by focused regression tests.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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.
Before
The check-mode step summary under a fine-grained token denied Administration:
After
How
Regression from 752b190 (branches onto the plan contract):
plan()resolved the repository's and the bypass actors' GraphQL node ids at plan time for the create/update payload. Those lookups tolerate no NOT_FOUND, so a denied token failed the section on a read in check mode, against its declared "absent" posture (denial surfaces at the first write).before: newPlannedOpfacet the executor runs ahead of the request. Keeps the pre-752b190 guarantee that a misspelled actor fails while live protection is untouched.phase: "execution"on a REST or GraphQL read: its bound helpers areGatedbehind theExecToolstoken only a thunk holds, so aplan()body cannot compile a call to it.planningReads()drops such reads fromreadGating(),denialPosture(), the write-only note, and the oracle's no-read set; aprimaryReadon one is a BUG throw.failureArtifacts()dumps areport.mdfor an oracle mismatch on a run the runner passed (why issue Nightly fuzz failures #73 said "no failure report"), and merges caller failures into the runner's own report.Proof
beforehook, gated read port, execution-phase derivations, mock check-mode barriers, report merging, branches plan/execute paths).bun test/e2e/run.ts --sections branches16/16; the new scenario fails on the pre-fix commit.build:checkgreen.ExecToolsis a deliberate act the type gate does not target, and the mock's check-mode barrier catches it in CI.Fixes #73