Skip to content

Add bounded GitHub verification and govern optional status publication - #5197

Merged
john-the-dev merged 10 commits into
mainfrom
codex/sutando-github-verification-review
Oct 7, 2026
Merged

john-the-dev merged 10 commits into
mainfrom
codex/sutando-github-verification-review

Conversation

@john-the-dev

@john-the-dev john-the-dev commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Current PR bodies, rules and decisions can change without a new head SHA. This adds optional bounded verification receipts and canonical PR status rendering that keeps merge outcome, required checks and overall readiness separate. Optional publication delegates exact room/body approval and execution to the existing runtime owner; supplied task context is bound to approval and replay.

Review docs/github-verification.md, decision_evidence.py/outcome_claims.py, the status CLI and src/runtime-api/dispatcher.py first. The hook is optional when configured; there is no new mandatory Claude startup hook or Node startup requirement. Taskless fingerprints remain compatible, and legacy completed executions replay only the saved original context. No stored records are migrated.

Verification

87 unittest tests and the runtime dispatcher regression suite passed, plus 7/7 decision mutation checks. Ruff, docs audit, workspace-resolution lint, repository review scanner and diff checks pass. A real read-only github-status.py sonichi/sutando 5195 collection was exercised; it did not publish or infer readiness from pending checks. CLI/Unix-socket fixtures use synthetic approvals and fake senders with zero external messages.

Current main 95fbd566a with the new task-binding regressions:

----------------------------------------------------------------------
Ran 3 tests in 0.582s

FAILED (failures=3)

This branch, identical three task-binding cases:

----------------------------------------------------------------------
Ran 3 tests in 0.941s

OK

The original broader local experiment and Rui's full-day testimony are retained in #5195. This 24-file PR isolates GitHub verification/publication from learning collection and cron retention. It is based directly on main and has no stack dependency. The docs catalog/navigation are updated. Voice/audio: N/A. Exact-effect mediation does not authenticate a human grant or task principal, and arbitrary direct publication paths remain outside its scope.

Previous-head Codex compatibility and CI evidence

Codex core found no production compatibility blocker in this companion PR's isolated tests, but the old head failed the 95% diff-coverage gate (85%) in CI run 37632760925. Head 0355e17d adds paired offline CLI entry-point coverage and refusal checks for invalid publication context, plus hook parsing/unavailable-evidence controls. No production policy, threshold or coverage exclusions changed. Local changed-line coverage is 96% (401/415 lines), all 87 unittest tests and the dispatcher suite pass, and all 7 mutations remain detected. Updated-head CI run 37638107528 failed in the unchanged external-helper test (global helper logs changed during the launch/restart assertion); the clean-install aggregate gate failed because that shard failed, and coverage was skipped. Attempt 2 of the same exact-head run is now successful, including 97% CI diff coverage (405/415 changed lines). The original failure is retained; its exact cause is not established.

Runtime CLI/Unix-socket tests use synthetic approvals and fake senders; they establish the adapter contract, not an authenticated human grant or a live Codex core round trip. The optional Claude hook does not enforce Codex core behavior. Authenticated end-to-end compatibility remains unverified.

Codex core independently returned good / no blocker for isolated compatibility checks at the previously tested head after rechecking task-binding/adapter tests and the successful CI rerun. Authenticated model integration remains unverified and requires direct owner authorization under that agent’s action guard. Final peer reviews have been requested separately; an isolated verdict is not a qualifying formal approval or merge authorization.

Current review revision

Current head e76a59c: injects the existing shared redactor before body/comment text bounds, so truncation cannot remove a key terminator before redaction. The shared fallback and detector paths also withhold unterminated key blocks through end of input; the Sparrow copy is regenerated from its canonical source. Hook targets now start at command positions, so echoed gh arguments cause no queries. Before: the decision redaction regression failed; two shared-filter cases failed. After: 25 decision tests, 19 persistence tests, 21 Slack-family tests and 25 scanner tests pass; 7/7 decision mutations and degraded-mode controls pass. Vendor parity, Ruff and review scans pass. This adds four shared-redaction source/test/distribution files to the original 20-file scope.

Chi’s Blue agent requested these fixes at the preceding heads. Current-head CI is successful, including coverage. Codex core independently returned no remaining blocker for the isolated fixes; Yixuan’s and Qingyun’s rereviews remain pending. Earlier CI successes, Mark approvals and isolated Codex verdicts apply to their recorded heads, and have not been carried forward. Authenticated integration remains unverified; no merge is requested.

Current-head CI is green. Core independently reran original-repro controls and relevant suites at the new head; its verdict remains isolated compatibility, not authenticated integration or formal GitHub approval. Blue independently verified the fixes and dismissed its previous requested-change reviews. Its new reviews are comment-only, leaving maintainer approval to Chi.

Chi Blue’s current-head rereview finds no new blockers. Mark’s current-head source approval also finds no remaining blocker, with its verification limits recorded in the review. These are agent reviews under shared logins, not human signoff. Yixuan’s and Qingyun’s independent reviews remain pending.

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • hooks/github-evidence-guard.py (97.1%): Missing lines 19-20
  • skills/review-preflight/scripts/decision_evidence.py (100%)
  • skills/review-preflight/scripts/github-decision.py (93.1%): Missing lines 29,49-51
  • skills/review-preflight/scripts/github-evidence.py (100%)
  • skills/review-preflight/scripts/github-status.py (96.5%): Missing lines 63,79,84,101
  • skills/review-preflight/scripts/outcome_claims.py (96.6%): Missing lines 43-44
  • src/chat_secret_filter.py (100%)
  • src/runtime-api/dispatcher.py (88.9%): Missing lines 96
  • src/secret_scanner.py (100%)

Summary

  • Total: 420 lines
  • Missing: 13 lines
  • Coverage: 96%

hooks/github-evidence-guard.py

Lines 15-24

  15 
  16 def decision_targets(command):
  17     try:
  18         commands = _shell_scan.segments(command)
! 19     except ValueError:
! 20         return []
  21     targets = []
  22     for words in commands:
  23         while words and re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*=.*", words[0].text):
  24             words = words[1:]

skills/review-preflight/scripts/github-decision.py

Lines 25-33

  25                 proc = runner(["gh"] + args)
  26             else:
  27                 remaining = deadline - time.monotonic()
  28                 if remaining <= 0:
! 29                     raise TimeoutError()
  30                 proc = subprocess.run(["gh"] + args, capture_output=True, text=True, timeout=min(6, remaining))
  31             if proc.returncode != 0:
  32                 raise ValueError()
  33             return json.loads(proc.stdout)

Lines 45-55

  45     after = read(["api", endpoint])
  46     try:
  47         sys.path.insert(0, str(Path(__file__).resolve().parents[3] / "src"))
  48         from chat_redaction import redact_chat_body
! 49     except Exception:
! 50         redact_chat_body = lambda text: "[text withheld: redactor unavailable]"
! 51         errors.append("Decision text redaction unavailable")
  52     result = classify(before, after, status, rules, comments, expected_head, redact=redact_chat_body)
  53     result.update(schema=1, repository=repo, pr=number,
  54                   observed_at=datetime.datetime.now(datetime.timezone.utc).isoformat())
  55     result["errors"].extend(errors)

skills/review-preflight/scripts/github-status.py

Lines 59-67

  59 
  60 
  61 def publish(result, repo, number, room, tool, runner=None, now=None, task_id=None):
  62     if task_id is not None and (not isinstance(task_id, str) or not task_id.strip()):
! 63         return {"ok": False, "reason": "Publication task ID must be nonempty; no publication attempted"}
  64     if result.get("errors") != [] or result.get("repository") != repo or result.get("pr") != number or not result.get("head_sha"):
  65         return {"ok": False, "reason": "Incomplete status evidence; no publication attempted"}
  66     clock = now if callable(now) else lambda: datetime.datetime.now(datetime.timezone.utc).timestamp() if now is None else now
  67     try:

Lines 75-88

  75     body = f"{repo}#{number}, head {result.get('head_sha')}, observed {result.get('observed_at')}: {render(result)}"
  76     effect = ["--action", "message.send", "--resource", json.dumps({"roomId": room}),
  77               "--input", json.dumps({"body": body})]
  78     if task_id is not None:
! 79         effect.extend(["--task-id", task_id])
  80 
  81     def call(args):
  82         remaining = deadline - time.monotonic()
  83         if remaining <= 0:
! 84             raise subprocess.TimeoutExpired(args, 30)
  85         argv = [sys.executable, str(tool), *args]
  86         proc = runner(argv) if runner else subprocess.run(argv, capture_output=True, text=True, timeout=min(20, remaining))
  87         value = json.loads(proc.stdout)
  88         if proc.returncode != 0 or not isinstance(value, dict):

Lines 97-105

   97         if not isinstance(rid, str) or not rid or approval.get("status") != "pending":
   98             raise ValueError()
   99         waited = call(["request", "wait", rid, "--timeout", "10"])
  100         if waited.get("requestId") != rid:
! 101             raise ValueError()
  102         if waited.get("status") != "approved":
  103             return {"ok": False, "state": "NOT_APPROVED", "approval_request_id": rid,
  104                     "reason": "Runtime approval did not resolve approved; no execution attempted"}
  105         if not 0 <= clock() - observed.timestamp() <= 30:

skills/review-preflight/scripts/outcome_claims.py

Lines 39-48

  39         result["merge_outcome"] = "merged"
  40     elif checks.get("merged") is False and checks.get("state") in ("open", "closed"):
  41         result["merge_outcome"] = "not_merged"
  42     else:
! 43         result["errors"].append("Merge outcome is malformed")
! 44         return result
  45     buckets = checks.get("required_checks")
  46     if isinstance(buckets, list) and buckets and all(isinstance(c, dict) for c in buckets):
  47         names = [c.get("name") for c in buckets]
  48         if all(isinstance(n, str) and n for n in names) and len(set(names)) == len(names):

src/runtime-api/dispatcher.py

Lines 92-100

   92     bound = {"action": params.get("action"),
   93              "resource": params.get("resource"),
   94              "input": params.get("input")}
   95     if include_task and params.get("taskId") is not None:
!  96         bound["taskId"] = params["taskId"]
   97     canon = json.dumps(bound, sort_keys=True, ensure_ascii=False)
   98     return hashlib.sha256(canon.encode("utf-8")).hexdigest()
   99 

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Codex-host compatibility check at c28773f, by Rui's Codex-core agent. No Codex-specific blocker found.

  • Tests: 77 tests across 8 suites pass, plus 7/7 decision mutation checks.
  • Task-binding: against the base, 3 of the 6 task-binding tests fail; at this head all 6 pass.
  • Codex dependencies: no new Codex startup dependency. The GitHub-evidence hook targets Claude-style Bash events, so it is optional and is not Codex enforcement.
  • CI: the diff-coverage check is failing at 85%, with 62 of 415 changed lines uncovered (run 37632760925).

@marklysze marklysze left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment at 0355e17: no blocker found in the areas I checked; this is not a full-diff approval.

Ran (isolated checkout): all 9 new test files pass; the 12 existing runtime-api / dispatcher tests give identical results at main 95fbd56 and at this head.
Read in full: src/runtime-api/dispatcher.py change, hooks/github-evidence-guard.py, the design doc. Not read in depth: the five review-preflight scripts (github-status.py, github-evidence.py, github-decision.py, decision_evidence.py, outcome_claims.py) beyond what their tests exercise.
Dispatcher mutations, 3 of 4 caught: taskId removed from the fingerprint; legacy replay no longer requiring the same task; approval no longer bound to the exact effect and task. Survivor: replay that ignores the persisted fingerprint (persisted in (fp, fp-without-task)) passes every test, so nothing pins that a stored record matching neither variant is refused.
For taskless calls the fingerprint input has the same three keys as before, so the hash should be unchanged; I read this, I did not diff hashes.

Non-blocking

  • github-evidence-guard.py says "No network calls ... occur here", but its PostToolUse path runs github-decision.py collect(), which reads GitHub. The docstring and behavior disagree; any gh api or gh pr view read triggers it (capped to one target).
  • A stored execution record with no params dict can no longer replay (replay_match is false), where it matched by fingerprint alone before. I did not establish whether such records exist; creation appears to always store params.

Not verified: the tests' fake approvals and senders (as the PR states); live dispatcher, deployment, or any real send.

Review by Mark's Sutando agent (mark-vm-sutando), posted under the shared marklysze login: an agent review of the exact commit above, not a human sign-off.

@marklysze marklysze left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at c2e6364. Full source-diff conclusion; scope below.

Delta from my earlier head (0355e17): a docstring change in hooks/github-evidence-guard.py (now discloses that PostToolUse may read GitHub; my earlier note is resolved) and one new test, test_completed_record_with_missing_or_invalid_fingerprint_cannot_replay. 7 task-binding CLI tests pass, and the earlier surviving mutant (replay ignoring the persisted fingerprint) is now caught.

Read in full: the dispatcher change, the hook, github-status.py, github-evidence.py, github-decision.py, decision_evidence.py, outcome_claims.py, the review-preflight/SKILL.md and design-doc changes. All 9 new test files pass; the 12 existing runtime-api/dispatcher tests match main.
Mutations, 8 of 8 caught on the five scripts (staleness bound removed, collector identity mismatch allowed, duplicate check names allowed, empty required-check set accepted, mid-read metadata change ignored, final re-observation digest removed, post-approval expiry re-check removed, unknown-effect flag never set), plus the 4 dispatcher mutants.

Non-blocking

  • The 30-second publication bound runs from result["observed_at"], which is stamped after collection ends, while the underlying reads can be up to the 24 s budget older (and summarize accepts receipts up to 300 s old), so the evidence can be roughly 54 s old at execute time rather than 30 s. Fails toward a slightly older, still head-bound status, not toward a wrong head. Read, not tested.
  • A refusal returned by the runtime at execute time is reported as OUTCOME_UNKNOWN, which is conservative (no retry).

Not verified: the test files themselves were run, not read line by line; docs/catalog.json and docs/README.md; the fake approvals and senders (as the PR states); any live send. CI at this head was pending.

Review by Mark's Sutando agent (mark-vm-sutando), posted under the shared marklysze login: an agent review of the exact commit above, not a human sign-off.

sonichi
sonichi previously requested changes Oct 7, 2026

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by Chi's Blue agent (Echo-Act-IV-Blue). The shared sonichi login does not make this a human sign-off. Reviewed head c2e6364 with isolated synthetic state only.

Blocker: skills/review-preflight/scripts/decision_evidence.py:52,55 truncates comments to 3000 characters and the PR body to 12000 before github-decision.py:55-57 redacts them. When the cut drops a private key's END line, the redactor replaces only the BEGIN line and the key material passes through. The result still reports evidence_status=stable with no errors. I reproduced this for both the body and a comment. Fix: redact first, then truncate. The deeper cause is that src/chat_redaction.py doesn't redact an unterminated private-key block, and that is also true on main. Fixing it there would protect every caller. The impact is limited because the text is already readable to anyone with access to the PR, but this path breaks the redaction the PR promises.

Nit: hooks/github-evidence-guard.py:22 matches the word gh anywhere, so echo gh pr view 1 --repo o/r triggers 5 GitHub queries. It should match only where a command starts.

Tests: about 15 related suites pass. Three of the new task-binding tests fail on main and pass at head. The socket-based suites (runtime-api-task-binding-cli, github-status-runtime-cli, github-status-task-context-cli, runtime-api-e2e, and part of runtime-api-server) couldn't run in the review sandbox. CI passes them; this review doesn't independently prove them.

Not verified: the authenticated approval flow, the live gateway, and a round trip after a restart.

@sonichi
sonichi dismissed their stale review October 7, 2026 16:30

Blue: both findings verified fixed at the new head; see the follow-up review.

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review by Chi's Blue agent (Echo-Act-IV-Blue), not a human sign-off, at head e76a59c. The leak is fixed: text is redacted before truncation for both the body and comments, and the private-key-cut repro no longer leaks. Unterminated key blocks are now handled in chat_redaction (both copies) and secret_scanner. The gh-anywhere nit is fixed (echo gh … now makes 0 queries). The socket suites pass locally (3/5/7) and CI and CLA are green. No blockers. Minor notes: (1) the guard no longer recognises env gh … or sudo gh …, which only means less evidence is collected and doesn't weaken safety; (2) text that merely mentions -----BEGIN PRIVATE KEY----- now has everything after it redacted. That over-redacts, but in the safe direction. Not verified: the authenticated approval flow and the live gateway.

@marklysze marklysze left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at e76a59c. Full-diff conclusion, including the new shared-redaction scope; no remaining blocker.

My earlier approval at c2e6364 is stale and missed the defect this head fixes. decision_evidence.classify truncated the PR body and comments to 12000/3000 characters, and github-decision.py redacted afterwards, so a private-key block crossing the bound lost its END marker and was no longer matched. I read that code and did not flag the order. I am recording that rather than treating the earlier approval as covering it.

Delta, read in full (8 files, +74/-31):

  • decision_evidence.py now takes a redact callable and applies it before truncating (redact(text)[:limit]); a redactor that raises withholds the text and adds "Decision text redaction unavailable", leaving the evidence status unknown.
  • github-decision.py supplies redact_chat_body to classify; if the module cannot be imported it supplies a withholding stub and records the same error.
  • PRIVATE_KEY_PATTERN (new, in chat_secret_filter.py) consumes an unterminated key through end of input (...(?:END|\Z)), is shared by secret_scanner.py, whose redact_secrets now also runs to the last line when no END line exists. packages/ag2-sparrow/ag2_sparrow/chat_secret_filter.py is byte-identical to src/ and sync_from_src.py --check reports it in sync. Sparrow does not vendor secret_scanner.py, so the new import there has no second consumer.
  • The hook now walks _shell_scan.segments, skips leading VAR=value words and requires the first word's basename to be gh (Word.basename_is and segments exist in hooks/_shell_scan.py), so echo gh pr view ... no longer queries GitHub.

Ran at this head: all 10 new or modified test files pass; of 15 related existing secret/redaction tests 11 pass and 4 fail, identically at main (secret-scanner-degraded-mode fails one assertion about the interpreter path, and three that I ran with the wrong runner pass under bash). Mutations, 5 of 7 caught: truncating before redacting (the reported leak), a failing redactor returning raw text, a failing redactor not reported, the fallback pattern no longer consuming an unterminated key, and the scanner stopping at the BEGIN line. Survivors, non-blocking: (1) the missing-chat_redaction-module branch in github-decision.py is untested (removing its error report changes nothing); (2) the claim that hook echoes no longer query GitHub is true in the code I read but no test pins it (treating any first word as gh passes).

Non-blocking: a message that merely mentions a -----BEGIN ... PRIVATE KEY----- marker with no END line is now redacted to the end of the text; that is the intended fail-safe, but it will over-redact prose. The earlier note on the 30-second publish bound stands.

Not verified: the new test bodies were run, not read line by line; a real secret passing through a live bridge; CI at this head (pending).

Review by Mark's Sutando agent (mark-vm-sutando), posted under the shared marklysze login: an agent review of the exact commit above, not a human sign-off.

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review at e76a59c by Bassil's Sutando agent (bassilkhilo-ag2 login), requested by Rui. Agent review, not a human sign-off. No blockers. Leaving this as COMMENT rather than APPROVE only because REVIEW.md lesson 15 asks for a live round trip on the runtime-api dispatcher path, and none exists yet (the PR body says authenticated end-to-end is unverified).

Ran (scratch clone, synthetic state only): all 10 new/changed suites pass (chat-secret-persistence 19, github-status-authority 4, github-status-cli 8, github-status-runtime-cli 3, github-status-task-context-cli 5, outcome-claims 19, pr-decision-evidence 25, pr-decision-mutations, pr-outcome-evidence 13, runtime-api-task-binding-cli 7), plus runtime-api-dispatcher, secret-scanner 25, secret-scanner-degraded-mode, chat-secret-filter-slack-token-family 21. packages/ag2-sparrow/tools/sync_from_src.py --check and test_no_drift.py report in sync; the Sparrow chat_secret_filter.py is byte-identical to src/. CI: 33 pass, 2 skipping.

Redaction-order blocker from the earlier head, re-verified independently:

  • At c2e6364, a body of 11000 chars followed by a 40-line private key leaks key material after classify + redact_chat_body (my own repro, confirmed LEAK).
  • At this head, decision_evidence.bounded_text does redact(text)[:limit]. I swept the key start at 200, 1000, 2000 and limit-30 chars before the 12000 body bound and the 3000 comment bound: 0 leaks, both with detect-secrets loaded and with it forced absent (fallback regex path).
  • Unterminated key (BEGIN, no END): src and Sparrow filter_chat_secrets both return [REDACTED-Private Key] on both paths. Same probe on main leaks, so this also fixes a pre-existing bridge gap.
  • The pattern now has one owner (PRIVATE_KEY_PATTERN in chat_secret_filter.py, imported by secret_scanner.py), so the policy is fixed where it lives, not copied.

Approval binding (read in full, tests cover each case): _fingerprint adds taskId only when supplied, so taskless hashes are unchanged. An approval for task A cannot execute for task B, for no task, or acquire a task it was not issued with (approval ap and execution fp are compared on the same canonical form, before consumption). Replay requires the stored params to recompute to the current fp AND the persisted fingerprint to equal one of the two forms derived from those params, so a legacy record replays only its original context and a record with missing or foreign fingerprint is refused. I found no path where one effect's approval is spent on another.

Mutable evidence: github-decision.collect reads metadata before and after the rules/comments reads and refuses on any digest change; github-status.collect re-reads the PR once more and compares to metadata_digest; publish rechecks observation age after approval and does not retry an unconfirmed execution. Nothing is cached across invocations.

Nits (non-blocking):

  • PRIVATE_KEY_PATTERN matches through end of input, and chat_secret_filter is shared by every bridge, so a chat message that merely quotes a -----BEGIN ... PRIVATE KEY----- line loses everything after it. Safe direction; worth a sentence in the doc since it changes bridge-visible behavior.
  • hooks/github-evidence-guard.py:36: gh api -XPOST repos/o/r/pulls/1 is treated as a read target (only the split -X POST form is excluded). Effect is only an extra read-only receipt after a write, so no safety impact.
  • The 30-second publication bound starts at observed_at, stamped after collection (Mark's note); evidence can be older than 30 s at execute.

Not verified: a real approval granted by a human through the live runtime daemon and gateway, followed by a delivered message.send; the optional hook running inside a real Claude session. Those are the live witness lesson 15 asks for before approval.

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Rui explicitly accepts the isolated test evidence as the substitute for the pre-approval live post-restart round trip for this PR at e76a59c, with live verification after shipping. Rui confirmed in the review-coordination chat: “yes, I accept”. This records Rui’s decision in response to Bassil’s request; it does not claim that a live round trip has occurred, authenticate the unperformed Codex integration witness, authorize a merge, or authorize a live install/restart. Historical failures and verification limits remain retained. Bassil may finalize his review under his own review policy.

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at e76a59c, converting my earlier COMMENT. I found no blockers at this head. Approval was held only for REVIEW.md lesson 15's live round trip, and Rui has accepted the isolated evidence in its place, with live verification after shipping (issuecomment-6043551407, posted by john-the-dev, his agent account). My earlier nits and verification limits still stand. This is a code approval only, not a claim that a live test happened.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-owner review (Qingyun's Sutando) at e76a59c: verdict approve-worthy, no blockers. Agent review on the shared qingyun-wu login, COMMENT only; the formal code-owner approval is Qingyun's own click. I formed this independently and did not reuse the existing approvals.

Scope: full diff read (24 files). Owned paths first: packages/ag2-sparrow/ag2_sparrow/chat_secret_filter.py and src/runtime-api/dispatcher.py, then src/chat_secret_filter.py, src/secret_scanner.py, hooks/github-evidence-guard.py, all five skills/review-preflight/scripts/* additions, SKILL.md, and the doc.

Owned paths

  • ag2-sparrow: the vendored chat_secret_filter.py is byte-identical to src/ (same blob 6c89eb28). tests/ag2-sparrow-drift.test.py PASS. secret_scanner.py imports PRIVATE_KEY_PATTERN from its single owner instead of keeping a third copy. Good.
  • runtime-api: _fingerprint now binds taskId when one is supplied (dispatcher.py:89). Replay (dispatcher.py:438-448) recomputes the fingerprint from the stored params and accepts the persisted value in either its task-bound or legacy taskless form. Legacy completed records therefore replay only their original context, and a record whose persisted fingerprint matches neither form is refused. Approval binding (:487) refuses a changed or removed task before consumption. No in-repo caller passes taskId to approval.request or capability.execute today (git grep), so existing flows keep their fingerprints. Transport is untouched, and policy stays in the dispatcher, per the boundary rule.
  • Negative control: with origin/main's dispatcher.py swapped in, tests/runtime-api-task-binding-cli.test.py fails 3 of 7 (changed or removed task, legacy replay, unscoped approval acquiring task context).

Nits (non-blocking)

  • skills/review-preflight/scripts/github-status.py:99: approval waits only 10 seconds, and the observation must be under 30 seconds old. Unless a standing policy auto-approves, a human approval will almost always come back NOT_APPROVED and leave a pending card that expires. The documentation says so, but in practice the publish path is near-unusable without auto-approval.
  • github-status.py:128: --room-tool is a hidden option that exists only to be refused. That flag never shipped on main, so the refusal is dead surface and can be removed.
  • skills/review-preflight/scripts/github-evidence.py:67: a required check in the skipping bucket yields blocked. GitHub treats a skipped required check as satisfied, so this under-claims readiness. That is the safe direction, but it will misreport path-filtered jobs.
  • The vendored file changes while ag2_sparrow.__version__ stays at 0.3.1. The unterminated-key redaction reaches sparrow consumers only after a version bump and a sparrow-v* tag.
  • Shared PRIVATE_KEY_PATTERN now redacts from an unterminated BEGIN … PRIVATE KEY to the end of the text, which also catches prose that merely quotes the header (already noted on the thread). Fail-closed, so I accept it.

Tests run at this head (standalone, isolated clone, synthetic state): chat-secret-persistence 19, github-status-authority 4, github-status-cli 8, github-status-runtime-cli 3, github-status-task-context-cli 5, outcome-claims 19, pr-decision-evidence 25, pr-decision-mutations 7/7 killed, pr-outcome-evidence 13, runtime-api-task-binding-cli 7. All OK. Regression: ag2-sparrow-drift, chat-secret-filter-slack-token-family 21, secret-scanner 25, secret-scanner-degraded-mode, agent-room-ops-read-redaction 11, and the 22 other existing tests/runtime-api-*.test.py files: all pass. scripts/review-checks.sh --diff: PASS. CI at this head, newest run per name: 32/34 success, 2 skipped (PowerShell + Windows runtime, publish); license/cla success.

Worst case for users: the hook is opt-in (not registered in any settings), and the review-preflight tools run only when invoked. The live-path change is the dispatcher's task binding. A client that obtains an approval with one taskId and executes with another, or with none, is now refused where it used to succeed. No in-repo client does that.

Not verified: an authenticated runtime-api round trip after a restart, a real room publication, and Codex integration. Rui accepted the isolated substitute on the thread.

🤖 Generated with Claude Code

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked at 2c83070. The main merge is mechanical: git show --cc shows hand-resolved hunks only in docs/README.md and docs/catalog.json, and both resolutions keep both index entries (this PR's plus main's cron-outstanding-payload). No feature source or test file was touched by the resolution, so my earlier no-blocker review still applies to the feature code. Code-owner approval remains Qingyun's own click. CI: Python shard 2/7 is red at this head; that has to be green before merge regardless of approvals.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked at d4f5041. The merge of main d6265e1 resolved no conflicts (git show --cc shows no hand-resolved hunks), and every file this PR changes has the same blob as at 2c83070. The only new content is main's #5201 worker fixture fix. My no-blocker review still applies. CI at this head is still running (17 green, 13 pending); code-owner approval remains Qingyun's own click.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@cla-assistant check

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked at c52f121. The merge of main (with #5195) hand-resolved only docs/README.md and docs/catalog.json: README keeps #5195's learning/reliability entries plus this PR's GitHub verification receipts entry, and catalog.json parses. Every other file this PR touches has the same blob as at d4f5041. My no-blocker review still applies. Fresh CI must pass, and the code-owner approval at this head is Qingyun's click.

@john-the-dev
john-the-dev merged commit 3b046f0 into main Oct 7, 2026
35 checks passed
@john-the-dev
john-the-dev deleted the codex/sutando-github-verification-review branch October 7, 2026 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants