Conversation
|
@codex review |
|
@claude review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @itsHabib's task in 5m 19s —— View job Code Review: Gate Grant Discovery
Overall this is a well-structured addition. The boundary is clean (read-only, no state writes, no new authority), the TOCTOU protections are solid, and the test suite covers the important invariants. A few things worth calling out: P2 — Silent suppression of authentication errors when a valid grant coexists
if len(available) == 0 && unreadable != nil {
return d.failed(unreadable)
}If a grant has a bad signature but another valid grant exists, P2 — Dual
|
![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: 13a34c456e
ℹ️ 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".
| return gateResult{PR: fmt.Sprintf("%s#%d", repo, pr), HeadSHA: d.Subject.HeadSHA, | ||
| Outcome: "capability_refused", Code: "grant_" + d.Status, Why: d.Why, Discovery: &d}, d.exitCode(), nil |
There was a problem hiding this comment.
When omitted-grant discovery encounters an unreadable head, diff, floor, key, or ledger, d.exitCode() returns 4, but this branch still serializes outcome: "capability_refused". That contradicts Gate's contract—3 is refusal and 4 is a hard error—so callers that validate both fields receive an internally inconsistent terminal and may route an infrastructure or integrity failure as an authority refusal. Return an error/no decision outcome for assessment_required rather than labeling it a capability refusal.
AGENTS.md reference: cmd/gate/AGENTS.md:L8-L11
Useful? React with 👍 / 👎.
| func SubjectDiff(pr PRRef, head string) (string, error) { | ||
| result, err := primaryDiff(pr, head) | ||
| if err != nil { | ||
| return "", err |
There was a problem hiding this comment.
For a PR whose compare diff exceeds GitHub's line limit, primaryDiff returns the recognized HTTP 406 error and this new discovery path returns it directly. The normal evidence collector handles the same condition with tooLarge(err) and fallbackDiff, so an explicit-grant Gate run can evaluate such a PR while the new default omitted-grant path and discover-grant cannot get past assessment_required. Reuse the existing pinned local-diff fallback when assessing the discovery floor.
Useful? React with 👍 / 👎.
|
Reviewer: claude |
|
Consolidated review disposition for
Copilot was requested but no start/completion receipt was observed. CI passed the initial head. The forthcoming fix will receive fresh tests, independent review and configured review requests; this PR remains a draft with no merge or installation action. |
|
Addressed the consolidated findings in 8f89444: hard-error/no-outcome assessment failures, the existing pinned oversized-diff fallback, absolute custody/filesystem-floor paths, and visible malformed-grant diagnostics alongside valid selection. Full module race tests, vet, lint (zero issues), build and diff checks passed. Relative-path, terminal JSON, fallback and diagnostic regressions are included. |
|
@codex review |
|
@claude review |
|
Claude finished @itsHabib's task in 4m 14s —— View job Code Review: Gate Grant Discovery — fix commit
|
|
Reviewer: claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f894443ec
ℹ️ 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".
| if d.Status == "assessment_required" { | ||
| return gateResult{Discovery: &d}, codeError, fmt.Errorf("grant_assessment_required: %s", d.Why) |
There was a problem hiding this comment.
When an omitted-grant assessment fails because the floor, diff, key, or ledger is unreadable, this returns both a populated Discovery result and an error, but cmdGate discards res whenever err != nil. The top-level error renderer therefore emits the generic gate next escape instead of the discover-grant retry constructed by discoveryRoute, losing the exact -state, -key, and -floor configuration needed to repair and rerun the failed assessment. Preserve the discovery-specific terminal data while still exiting 4 with no decision outcome.
Useful? React with 👍 / 👎.
|
Addressed Codex P2 4001208984 in f3ddb66. Assessment errors now carry discovery through the terminal error renderer: stdout retains the exact subject, candidate diagnostics, state/key/floor configuration and read-only repair/retry route, while exit 4 still has an error and no decision outcome. CLI regressions cover unread floor/diff and invalid signing-key authentication. Full-module vet/race/build passed the runtime change; final Gate lint, focused CLI tests and whitespace checks passed. The unrelated Claude cosmetic suggestions remain recorded as non-blocking; no live Gate evaluation or installation occurred. |
|
@codex review |
|
@claude review |
|
Claude finished @itsHabib's task in 3m 23s —— View job Code Review: Gate Grant Discovery — fix commit
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3ddb66a25
ℹ️ 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".
| res, code, err := runGateBound(e, repo, pr, d.Subject.HeadSHA, d.GrantID, live, modelBackend, reviewsOptional) | ||
| res.Discovery = &d | ||
| return res, code, err |
There was a problem hiding this comment.
When the PR is pushed between discovery's final refresh and Gate's first gh pr view, matchBoundView returns grant_assessment_required, but this plain error is returned alongside res.Discovery; cmdGate then discards res on its error path and terminalErrorFor emits the generic gate next escape. Fresh evidence beyond the earlier pre-selection assessment-error case is that TestGateStopsWhenDiscoveredHeadMovesBeforeView exercises this post-selection branch only through runGateSelected, not the CLI renderer, so the new recovery wrapper still does not cover it. Preserve or wrap the discovery context here so the terminal directs the caller to reassess the new head with the original state, key, and floor configuration.
Useful? React with 👍 / 👎.
|
Reviewer: claude |
|
Addressed the post-selection recovery finding4001264942 in45c263bb585fcb6b88921520fff19d855c758dc8. The error wrapper now sits at cmdGate where results would otherwise be discarded, preserving discovery context for both initial assessment failures and later Gate failures such as a head moving before the first view. The actual CLI regression now covers that transition. Uncached targeted race tests, Gate vet/lint and diff checks pass; no authority or live-state changes. The prior Claude review completed with only the already recorded cosmetic suggestions. |
|
@codex review |
|
@claude review |
|
Claude finished @itsHabib's task in 2m 49s —— View job Code Review: Gate Grant Discovery — fix commit
|
|
Reviewer: claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45c263bb58
ℹ️ 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".
| if len(available) == 0 { | ||
| d.Status = "uncovered" | ||
| d.Why = fmt.Sprintf("no authenticated existing grant covers %s#%d at %s for merge, minimum %s and cycle %d; candidate gaps are listed; full Gate assessment may raise the tier", d.Subject.Repo, d.Subject.Number, d.Subject.HeadSHA, d.MinimumTier, d.NextCycle) | ||
| d.MintRequest = shellJoin([]string{"gate", "grant", "-repo", d.Subject.Repo, "-action", d.Action, "-max-tier", d.MinimumTier, "-max-cycles", fmt.Sprint(d.NextCycle), "-ttl", "24h", "-state", d.StateDir, "-key", d.KeyDir}) |
There was a problem hiding this comment.
When the ledger already records three consumed cycles and no candidate fits, NextCycle becomes 4 and this mint command explicitly asks the operator for -max-cycles 4. That bypasses the repository's terminating review cap: after three cycles, the required recovery is to stop the loop rather than request wider authority. Suppress the mint route once the cycle budget has been exhausted.
AGENTS.md reference: AGENTS.md:L102-L107
Useful? React with 👍 / 👎.
| if res.Discovery != nil { | ||
| return &grantAssessmentError{discovery: res.Discovery.failed(err)} |
There was a problem hiding this comment.
When omitted-grant discovery succeeds but a later normal Gate step fails—for example model construction or review-evidence fetching—res.Discovery remains non-nil, so this branch relabels the failure as grant_assessment_required and directs the operator to discover-grant. That command will merely report the same grant as available and cannot repair the actual failure, replacing the normal error classification and recovery route with a dead end. Restrict this wrapper to discovery failures and the intentional selected-head mismatch.
Useful? React with 👍 / 👎.
|
Latest review disposition at45c263b: the normal-evaluation error-routing P2 is valid and will be fixed with an explicit selected-head-mismatch error identity, retaining the original route for ordinary model/evidence failures. The P1 request for an unconditional three-cycle stop is not adopted: Michael explicitly superseded the procedural stop-fixing cap for this work, and the reusable resolver reports grant coverage rather than owning a universal reviewer-cycle policy (existing tier-aware policy even permits different caps). A proposed operator mint command grants no authority or review continuation; actual review-plan/request/spend limits and operator-only minting remain binding. No live grant or limit was changed. Claude completed at this head with only the already recorded cosmetic suggestions. |
|
Normal evaluation error routing is fixed in271bab1a7c81b7f0e249258f59e86f2a0c83cfde: discoveryTerminalError now wraps only initial assessment failures or the explicit selected-head-mismatch error identity. Backend/evidence errors retain their original classification and recovery route. The CLI regression proves an invalid model backend is not relabeled, alongside all earlier discovery and moved-head cases. Uncached targeted race tests, Gate vet/lint and diff checks pass. The procedural-cap finding is dispositioned separately above; no unconditional review policy or grant authority is added here. |
|
@codex review |
|
@claude review |
|
Claude finished @itsHabib's task in 2m 51s —— View job Code Review: Gate Grant Discovery — fix commit
|
d |
d.Status |
error | result |
|---|---|---|---|
| nil | — | any | plain error (explicit grant path) ✓ |
| non-nil | "assessment_required" |
any | wrapped ✓ |
| non-nil | "available" |
errGateHeadChanged |
wrapped (post-selection head move) ✓ |
| non-nil | "available" |
backend/evidence err | plain error (the fix) ✓ |
cmdGate (main.go:531)
The inline guard is replaced with the single call discoveryTerminalError(err, res.Discovery). Clean.
Fixture change (grant_discovery_test.go:354-361) ✓
pr view response now switches on a status variable that defaults to "MERGED" and becomes "OPEN" for mode "model failure". This is correct: runGateBound → runGateWithSynthesis refuses a merged PR before reaching model invocation. A "MERGED" fixture would cause the test to fail at the live-PR check rather than at the model backend — the fixture must return "OPEN" to exercise the actual model failure path. The removed mergedAt field is not read by matchBoundView or the model-failure path.
TestDiscoveryDoesNotRelabelNormalGateFailure ✓
The test creates a valid T2 grant (so discovery succeeds), then runs gate -model-backend fixture-invalid. Assertions:
terminal.Discovery == nil— confirmsterminalErrorForreceived a plain backend error, not a*grantAssessmentError, so noDiscoveryfield is set in the output. This is the tightest possible invariant.!strings.Contains(terminal.Error, "grant_assessment_required")— confirms the error string is not relabeled.!strings.Contains(terminal.Escape.Next, "discover-grant")— confirms the escape route is the normal gate recovery path, not the discovery retry.strings.Contains(terminal.Error, "fixture-invalid")— confirms the actual backend error is preserved and surfaced to the operator.
The three-condition OR means all four assertions must pass simultaneously. That's the right shape.
Verified correct
Residual TestGateDiscoveryAssessmentFailureIsHardError sub-cases unchanged. The "moved before view" case still works: matchBoundView returns fmt.Errorf("%w: %s to %s", errGateHeadChanged, ...), errors.Is(err, errGateHeadChanged) is true in discoveryTerminalError, and d.Status == "available" at that point (discovery succeeded). d.failed(err) sets Status = "assessment_required" and sets Why = err.Error(). grantAssessmentError.Error() calls TrimPrefix to deduplicate the prefix, which is also correct for the backend path where d.Why already carries the prefix from the previous commit's matchBoundView. ✓
errors.Is chain. matchBoundView returns fmt.Errorf("%w: ...") wrapping errGateHeadChanged. That error propagates through readGateView → runGateBound → runGateSelected without additional wrapping. errors.Is unwraps correctly. ✓
No regression on explicit-grant path. runGate (explicit grant) never calls discoverGrant, so res.Discovery is nil. discoveryTerminalError(err, nil) returns the plain error. ✓
Residual nits (carried, non-blocking)
-
unreadablelast-writer-wins — when multiple grants fail authentication in one sweep, only the last error appears ind.failed(unreadable). StructuredCandidatesgaps capture all failures; the hard-error text surfaces only one. Recorded and accepted. -
FloorBinpath-resolution comment — theContainsAnyguard (bare PATH-lookup names stay unresolved; path-relative references are absolutized) is non-obvious. A brief inline comment would help future readers. Still absent.
All five commits are clean. The fix narrows discoveryTerminalError to exactly two cases — initial assessment failure (d.Status == "assessment_required") and post-selection head movement (errors.Is(err, errGateHeadChanged)) — and the new test validates the key invariant that a backend failure after a successful discovery does not become a discovery error.
|
Reviewer: claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 271bab1a7c
ℹ️ 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".
| if fields.HeadSHA != head { | ||
| return fmt.Errorf("%w: %s to %s", errGateHeadChanged, head, fields.HeadSHA) |
There was a problem hiding this comment.
When gate -slack obtains an exact-head grant and the PR moves before the first gh pr view, this shared check returns errGateHeadChanged. Because the Slack path has no Discovery result, discoveryTerminalError does not translate it, so cmdGate emits exit 4 and the generic error recovery path. This is an expired subject binding rather than an infrastructure failure; return an exit-3 capability refusal or route the operator to request a fresh Slack grant so callers do not misclassify the terminal.
AGENTS.md reference: cmd/gate/AGENTS.md:L8-L11
Useful? React with 👍 / 👎.
|
Final evidence at c5a77e9:
Earlier verified findings are fixed. The unconditional procedural-cycle suggestion was dispositioned in the existing comment under Michael's explicit direction; actual grant/request/spend ceilings remain unchanged. Claude's two cosmetic observations remain recorded: all authentication failures are present in Candidates even though the terminal headline carries the last error, and the PATH-name guard is covered by tests/documented behavior. Neither requires another broad panel. Companion cc-skills #75 is independently verified with final Codex/CI evidence at 180d3d7. The change removes three redundant operator interaction steps for already covered work, or one inventory command plus manual selection from a careful reuse path; these are workflow counts, not measured time savings. No live grant mint, Gate evaluation, custody edit, merge or installed runtime/skill change occurred in this grant-discovery work. The source and stamped local candidate are reviewable; ordinary live delivery through the resolver is not yet demonstrated. |
|
Claude finished @itsHabib's task in 7m 58s —— View job Code Review: Gate Grant Discovery + Slack Head-Change RoutesReviewed at
Authorization correctness — which grants discovery may selectSignature verification is mandatory and upstream of every other check. In The repo filter runs pre-verification, but cannot be forged past signature verification. Action is hardcoded to "merge". Head and PR binding are correctly conditional. Cycle accounting uses a single audited snapshot.
Selection is widest-eligible. Fail-closed behaviorInfrastructure failures produce
Empty or non-hex view head SHA remains a hard error on the Slack path. A joined state-write failure is not converted to a capability refusal. When Closed PR → Normal gate failures after a successful discovery are not relabeled. Exit-code / JSON outcome agreementVerified correct across all new terminal states:
Slack head-move path: Discovery assessment path:
Cycle accounting: zero cycles consumed on head-change. The run-abort artifact (
|
…y -grant An independent exact-head review found that discovery could spend grants an explicit -grant refuses. CheckArtifactSubject accepts a head-bound grant whose head and PR match, so an omitted -grant could reuse a Slack exact-head approval for later runs or prefer a protected-executor T3 grant over repository authority. DESIGN.md reserves those grants for their own flows. Discovery now reports them as grant_bound candidates and never selects them. An explicitly empty -grant (for example an unset variable in a script) also fell through to discovery and ran under the widest grant. It is now an error. The remaining review findings fail closed or are advisory text; they are recorded in FOLLOWUPS.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
Gate currently requires a pasted grant ID, while advisory inventory and shared delivery guidance can request a new grant even when suitable authority already exists.
gate gatenow discovers and reuses existing operator-minted authority when-grantis omitted. Explicit IDs remain binding.What this adds
discover-grant -repo R -pr N -json. It checks the current open subject, immutable diff and existing deterministic floor, then authenticates all relevant grants against audited cycle accounting before selecting a covering candidate.available,uncovered,assessment_requiredandnot_applicableresults. Unknown assessment and closed PRs do not produce mint requests. Commands preserve selected custody and floor configuration; a moved head stops evaluation before model invocation.Discovery establishes eligibility for assessment. Its deterministic tier is a minimum, not the final reduced tier or permission to merge. Judgment remains attached to its existing run and grant lineage. This adds no mint authority, MCP, key creation, grant modification, required-check change or installation.
Validation
c5a77e9539c9aacb751faab379cf307b863c9678.8f89444: Workbench docs: make Fleet a focused chapter in the Workbench teaching path #335 found existing coverage with no mint prompt; RoxIQ chore: drop codex from the required panel while its quota is out #250 reported the actual T3 gap; Workbench fix(fleet): keep provider preparation failures retryable #334's T1 minimum remained an assessment floor, not proof of its separately identified T2 requirement. The probes are retained snapshots, not current PR-state claims.271bab1; CI passed at that exact head.722c3e4: an actual temporary signed-approval CLI regression fails at the prior head and now returns exit 3 with a fresh Slack route, preserves configured flags and limits, and proves no action/stamp or consumed cycle. Joined audit failures remain hard errors.c5a77e9539c9aacb751faab379cf307b863c9678, including the subsequently reproduced malformed-success view response. Empty/nonhex observed heads remain hard errors; the actual signed-callback regressions prove no consumed cycles or authority effects. Independent overlays and the targeted race suite (44.482s), Gate vet/lint and diff checks passed.c5a77e9539c9aacb751faab379cf307b863c9678. Claude's completed review remains correctly attributed to271bab1; no sixth broad panel request is added beyond the configured five. Copilot completion remains unverified.Companion source guidance: cc-skills #75, independently verified with final Codex/CI evidence at
180d3d7. Seecmd/gate/docs/grant-discovery-poc.mdfor reproduction and exact probe subjects. Fixture signing is disposable. No live Gate evaluation, merge, install, custody change or live Fleet change was performed. Draft for operator review.