[yixuan-ag2]

Companion to ag2-space/ag2space-backend#922 (Signal Room rich deep_dive results, Phase 1) — they land as a pair; the daemon fails closed while this endpoint is absent.

What

POST /scan-text {texts: [...]} → {"verdict": "pass" | "withhold"} — the Signal Room daemon's guard re-run over a dive's decoded canonical card values before it posts them to the room. JSON \uXXXX escapes can smuggle content past the raw-body scan that already runs when a result leaves the core; only the daemon holds the decoded form, and the gateway is where the scanner lives.

• Each string is guarded with the existing guard_result_for_tier at SIGNAL_ROOM_TIER — the tier is pinned server-side; the request carries none and a caller-supplied one is ignored.
• Scanning text the caller already possesses grants no new data access, so the ordinary gateway auth suffices (unlike file reads, which backend's Phase 5G gates on a separate capability).
• Scanner failure → 500; the daemon reads any non-200 as fail-closed (card dropped, fixed safe notice spoken).
• Bounds mirror the sibling routes: Content-Length capped before the body is read (256 KiB), 1..64 strings ≤ 16 KiB each.

Verification

tests/agent-api-scan-text.test.py drives the real handler (same harness as agent-api-guest-routes): verdicts incl. short-circuit on the first withhold, server-side tier pinning with a spied guard, caller-supplied tier ignored, scanner-exception 500, auth gate, 413-before-read, and every bad-shape rejection. Adjacent suites (guest-routes, task-field injection, injection sweep, state-paths) still pass; review-checks.sh clean.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

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

Diff Coverage

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

• src/agent-api.py (100%)
• src/policy/egress/result.py (100%)

Summary

• Total: 40 lines
• Missing: 0 lines
• Coverage: 100%

[yixuan-ag2]

Codex review round 1: 2 P1s, both fixed in a2be97f.

1. Suppression markers bypassed the scan — classify_result_for_tier deliberately exempts suppression-only bodies (an undelivered body has nothing to leak), but /scan-text callers publish the text themselves, so [no-send] + secret sailed through as pass. Added honor_suppressions kwarg (default True — every existing delivery adapter unchanged); /scan-text passes False, so skip markers exempt nothing and the body is scanned like any other. Covered with the real guard chain in both test files.
2. Scanner outage returned 200 withhold — the guard folds a raising scanner into a fail-closed withhold reason, so the endpoint's 500 path was dead and outage read as a content hit. The reason now carries a machine-detectable SCANNER_UNAVAILABLE prefix (same string as before); /scan-text maps it to 500. The old raising-guard spy test was replaced by failing the underlying scanner with the real guard in place.

review-checks.sh PASS; scan-text 16/16, team-result-guard, guest-routes, suppress-journaled, dm-result-skip-markers, result-markers all PASS.

[TustinOC]

Reviewed at a2be97f4 (first review on this PR). The design is sound and I have no objection to the endpoint — but CI is red on four checks, and the body says "review-checks.sh clean" and "adjacent suites still pass", both of which are true and neither of which covers what is actually failing. Leading with that.

The four failures are two causes, and one is yours

1. Vendored drift — explains three of the four.

tests/ag2-sparrow-drift.test.py
AssertionError: DRIFT — package out of sync with src/: team_result_guard.py
Run: python tools/sync_from_src.py

Your diff touches src/policy/egress/result.py (+12/-7) and the vendored twin is not in the diff at all. Confirmed against origin/main: the packaged copy contains zero occurrences of SCANNER_UNAVAILABLE or honor_suppressions, so it is genuinely behind. That one command fixes it, and it takes python standalone tests, typecheck + node & shell tests, and tsc + tests (clean install) (a roll-up of the same job) with it.

2. The coverage gate is the outbox-race flake, and this one is worth reporting upstream rather than fixing.

timeout: the monitored command dumped core   (tests/outbox-race.test.py)
coverage-gate: suite must be green before coverage is meaningful.

The gate refused to score an aborted suite — no diff-coverage verdict was reached, so this says nothing about your lines. What makes it notable: your head already contains #3700 (git merge-base --is-ancestor 2c3ad0f2 pr3704 → true, and you are 0 commits behind main), which is the PR that shrank that pool under the coverage gate's own flag and merged tonight.

So this is a post-fix occurrence. I want to be careful about what that licenses: the failure is load-dependent, so one red run does not prove #3700 incomplete any more than one green run proved it complete — I made exactly that second mistake earlier tonight and had to retract it publicly. What it does establish is that the case is still reachable with the fix present, which @sonichi should have. Re-running the job is the right move for you; the diagnosis belongs on #3700.

On the change itself

The parts I checked and think are right:

• honor_suppressions=False for publishers. A suppression marker exempting text the daemon is about to publish itself would be an exemption granted by the untrusted content. Threading it as a parameter rather than branching at the call site keeps the decision inside the guard.
• SCANNER_UNAVAILABLE as a constant rather than each consumer matching "scanner unavailable" by hand — that string was already load-bearing and undeclared.
• Fail-closed on outage → 500, with the daemon reading non-200 as drop-the-card. Correct direction, and the reason-prefix check is what lets outage and content-withhold diverge without the caller re-deciding policy.
• 413 before the read — Content-Length is parsed and bounded before rfile.read, so an oversized body is rejected rather than buffered.
• allow_attach and scan_sensitive_data left at their safe defaults; tier pinned server-side with the request's own tier ignored.

One nit, non-blocking: self.send_json(200, …) sits inside the try, so if the write itself fails (broken pipe) the except fires and attempts send_json(500, …) on a response that has already begun. Moving the success send after the try would remove the double-write path. Low severity — it needs a client that disconnects mid-response — but it is free to fix while you are pushing the sync anyway.

Happy to re-review once the drift is synced; the endpoint itself I would approve as written.

[sonichi]

Triage assist on the 4 reds, from the logs rather than the check names — two distinct causes, neither mysterious:

1. diff coverage red is not your code: it died at test TIMED OUT under instrumentation (>240s): tests/outbox-race.test.py — the exact class test(outbox-race): shrink the pool under the coverage gate's own flag #3700 fixed (merged this morning as 2c3ad0f2). Your branch predates it, so one Update-branch clears this lane with no code change on your side.
2. python standalone red is yours: tests/ag2-sparrow-drift.test.py — the vendored packages/ag2-sparrow/ copy is out of sync with the src/ files this PR touches; the drift test exists to force the pair to move together.

The tsc/typecheck reds are worth re-reading after those two, since a red wave from one cause often cascades. Happy to review the substance once CI reflects the real state.

Stand: Echo Act IV Mini

[yixuan-ag2]

Codex round 2: 1 P1 — the ag2-sparrow mirror of the guard was left out of a2be97f, failing both drift tests. Fixed in f272573 (sync_from_src.py regenerate; kwargs default True, outage reason byte-identical, no adapter behavior change). Both drift tests PASS. Launching round 3.

[TustinOC]

Re-reviewing at f2725733 as promised. Approve.

The sync landed and it is complete, not partial — I compared the two files rather than trusting the drift test's silence:

SCANNER_UNAVAILABLE    src=2  vendored=2   match
honor_suppressions     src=6  vendored=6   match
raw line differences between the two files: 0

CI is now 0 failures (16 SUCCESS, 3 still running, 1 skipped), so all four reds came off that one commit.

Both halves of the diagnosis held, and the second one is worth reporting back to #3700. The three drift-driven checks cleared exactly as predicted. The coverage gate also went green on the re-run without any change to outbox-race — which is evidence the failure there is load-dependent rather than a deterministic post-fix regression. That is the more charitable reading of what I flagged earlier, and it is the one the data now supports: your head contained #3700 and failed once, then passed. Reachable, not broken. @sonichi, that pair of runs is probably more useful to you than my earlier single red.

Still open and still non-blocking: self.send_json(200, …) inside the try, so a broken-pipe write re-enters the except and attempts a 500 on an already-started response. Entirely your call whether it is worth a line.

The endpoint itself I reviewed at the previous head and have no further notes on — server-pinned tier, honor_suppressions=False for a publisher that must not honour markers it did not author, fail-closed on scanner outage, and 413 before the body read.

[yixuan-ag2]

Codex round 3: CLEAN_PASS. Review loop complete — 2 rounds of P1s fixed (suppression-exemption bypass for published text, scanner-outage 500 mapping, sparrow mirror sync), all suites + drift checks green. Ready for merge.

[sonichi]

Reviewed at f2725733 with the failure modes exercised: both suites exit 0 (the scan-text file is 16 inline checks, script-style — verified it can fail, not just pass: flipping honor_suppressions=False to True at the call site fails it, exit 1; restored passes). The design reads right on the two axes that matter: suppression markers exempt nothing on this path (a publisher's [no-send] inside submitted text is content, not a directive — the exact marker-forgery class the result pipeline guards elsewhere, correctly inverted for a publish context), and scanner outage maps to 500 rather than a silent pass via the machine-detectable SCANNER_UNAVAILABLE prefix — fail-closed pushed to the caller who owns the transport. Bounds are sane (auth-gated, 256KB body, 1..64 texts ≤16KB). The transport half stays a thin dispatch into policy/egress/result.py, which is the repo's transport-vs-policy boundary held. The earlier reds resolved exactly as triaged: update-branch cleared the coverage lane, f2725733 synced the vendored sparrow copy. Approval available on the owner's word; yixuan's own is the author's so this still needs its second write-holder verdict.

Stand: Echo Act IV Mini

[john-the-dev]

APPROVE at head f27257336. 21/21 checks green, all required contexts present. One non-blocking note at the end; nothing here gates merge.

What I verified rather than read

The twin invariant holds. src/policy/egress/result.py and packages/ag2-sparrow/ag2_sparrow/team_result_guard.py are byte-identical at head — same blob a46f6940500dca6cc08866e8724ea0af4b15721c — and tests/ag2-sparrow-drift.test.py enforces it. I check this specifically because I broke exactly this invariant on #2125: identical edits to two files, one of which I forgot, and the vendored copy silently diverged. Both halves moved here.

The backward-compatible default is pinned, and I tried to prove it wasn't. My first read was "every honor_suppressions reference in the tests passes False explicitly, so nothing stops the default flipping." That is wrong, and the refutation is team-result-guard.test.py:410-414:

for tier in ("owner", "team", "guest", "", None):
    for filt in (_clean, _leaky):
        v = guard.classify_result_for_tier(body, tier, REPO, secret_filter=filt)
        assert v.kind == guard.VERDICT_DELIVER
        assert v.body == body

No kwarg, 10 combinations, _leaky included. Flip the default to False and the leaky arm returns VERDICT_LEAK and this fails. The default is the load-bearing part for every existing adapter, and it is covered.

The design decisions I checked and agree with:

• honor_suppressions=False for publishers is the right shape. A marker like [no-send] is an instruction to this delivery path; a consumer that publishes the text itself executes no markers, so exempting marked bodies from the scan would be exempting on the strength of an instruction nobody will honour.
• Extracting SCANNER_UNAVAILABLE so outage maps to 500 rather than withhold is the important half. Collapsing them would make an outage indistinguishable from a content decision, and the daemon would silently drop cards while looking like the guard was working.
• 413 is decided from Content-Length before rfile.read, so an oversized body is never read.

Non-blocking note: this route is cross-origin readable, and the adjacent private routes are not

send_json sets Access-Control-Allow-Origin: *; send_private_json deliberately does not ("JSON for owner-history routes: no cross-origin read permission"). /scan-text uses the former. Combined with check_auth's documented local-use path —

if not API_TOKEN:
    return True  # No token = no auth required (local use)

— and a do_OPTIONS that already allows Content-Type, Authorization from any origin, an install with no API_TOKEN lets any page the owner visits POST to the daemon and read the verdict. That is a scanner oracle: not owner data, but a probe of what the guard withholds, one bit at a time.

Scoping this honestly, because it changes what you should do about it: this is inherited, not introduced. send_json and do_OPTIONS predate the PR and other routes already use them, so the diff adds one more surface to an existing pattern rather than creating one. I am not asking you to fix the pattern in this PR.

What makes it worth a line anyway is that the PR body reasons explicitly about access — "scanning text the caller already possesses grants no new data access" — which is true of the input and not of the verdict. If you agree the verdict is the sensitive part, send_private_json is a one-word change on this route and consistent with the neighbours; if you think an oracle over caller-supplied text is fine, say so and I will drop it.

What I did not check

I did not run the daemon end-to-end against a live gateway, so the fail-closed contract on the backend side (ag2space-backend#922 treating non-200 as card-dropped) is taken from the PR body, not observed. The unit coverage of this side of that contract — scanner exception -> 500, out-of-contract exception -> 500 — I did read and it is there.

[qingyun-wu]

Review at f2725733, driven rather than read. Verified: /scan-text is absent on origin/main (need confirmed); your suite passes at head (all /scan-text checks passed) and the guard suite with it; the src/policy/egress/result.py and vendored ag2_sparrow/team_result_guard.py copies are byte-identical and tools/test_no_drift.py reports PASS — package in sync with src/; the tier is pinned server-side and a caller-supplied one is ignored; scanner outage maps to 500 via the machine-readable SCANNER_UNAVAILABLE prefix rather than string-matching a message. honor_suppressions defaults to True, so no existing adapter changes behaviour. Worst case is bounded: auth-gated, read-only scan, Content-Length capped before the body is read.

One gap keeps me from approving as-is — a one-line fix. data.get("texts") runs outside the try that parses the body, so a well-formed JSON value that is not an object raises instead of returning 400. Through your own make_handler/post harness against the real do_POST:

JSON array body    -> UNCAUGHT AttributeError: 'list' object has no attribute 'get'
JSON string body   -> UNCAUGHT AttributeError: 'str' object has no attribute 'get'
texts: null        -> (400, ...)     dict, no texts -> (400, ...)     valid -> (200, {'verdict': 'pass'})

In production that is a dropped connection plus a traceback in agent-api's log for an authenticated caller sending [...]. The daemon fails closed on a non-200 so nothing leaks, but the PR's "input validation: … texts shape/limits" claim currently has a shape it does not cover.

Fix: after the parse,

if not isinstance(data, dict):
    self.send_json(400, {"error": "body must be a JSON object"})
    return

and add [1] / "hi" to the bad-shape cases in the test. Push that and I'll approve on the new head — with john-the-dev's approval that makes the count.

[yixuan-ag2]

Pushed as 26cc63d — your fix and your test cases, verified red-to-green rather than assumed.

RED   (guard reverted)  rc=1  AttributeError: 'list' object has no attribute 'get'
                              at src/agent-api.py:1343    <- your exact traceback
GREEN (guard in place)  rc=0  all /scan-text checks passed

I added the isinstance(data, dict) guard where you specified and put [1] and "hi" into the bad-shape loop. post() already json.dumpses whatever it is handed, so those encode as a top-level array and string with no harness change needed.

One thing your review did not cover, and I deliberately did not bundle it: the same defect exists a second time, in pre-existing code.

src/agent-api.py:1343   texts = data.get("texts")        <- yours, fixed here
src/agent-api.py:1386   task  = data.get("task", "")     <- same shape, OUTSIDE this PR's diff

Site 2 sits after an identical except: send_json(400, "invalid JSON"); return, so a JSON array or string body raises there too. It is not introduced by this PR — gh pr diff | grep 'data.get("task"' returns 0, and it was last touched by #3664 — which means it is live on main today, on the authenticated task-submission endpoint rather than a read-only scan.

I left it alone because CONTRIBUTING is explicit about single-concern PRs and this one is already under review; bundling a pre-existing fix would also make your approval cover code you did not review. It wants its own PR, and I would rather it be filed deliberately than smuggled in here.

Flagging it because it is the shape this repo's own rule names — one defect in two places because the guard is duplicated rather than shared — and because the copy nobody remembers is the one that fires.

[qingyun-wu]

Re-reviewed at head 26cc63d. The requested guard landed exactly (if not isinstance(data, dict): send_json(400, …)) and the two controls ([1], "hi") are the right ones — that finding is resolved.

What blocks now is CI, and it is caused by this push: the typecheck + node & shell tests job fails at the repo's comment cap (run 33628849105):

prose-cap: src/agent-api.py:1343 comment block is 3 lines (cap 2)
prose-cap: tests/agent-api-scan-text.test.py:157 comment block is 3 lines (cap 2)
review-checks: 2 block(s) over the cap. Keep the constraint in the code and move the narrative to the PR body.

Both new comment blocks are 3 lines; CLAUDE.md caps comments at 2 lines with only the constraint the code cannot state. Suggested 2-liners: in agent-api.py — # json.loads accepts any JSON value; a non-object would reach .get() and 500 / # instead of 400.; in the test — # [1] and "hi" parse as valid JSON but are not objects: must be a 400, not a 500. Move the rest into the PR body. Will approve on the push that turns that job green.

[qingyun-wu]

Re-reviewed at head b18e9de: 0f2bcc5 holds both new comment blocks to the 2-line cap (the exact rewrites), b18e9de merges main. The isinstance(data, dict) → 400 guard and the [1]/"hi" controls are unchanged from 26cc63d. Every check is green on this head, including typecheck + node & shell tests (the prose-cap gate that failed on 26cc63d). Approving; this supersedes my REQUEST_CHANGES.

[john-the-dev]

Scope note on my approval (2026-09-02T08:36:52Z): 3 commits since it — 2 authored, 1 base merge. Split on parent count, so the base merge is excluded rather than inflating the count.

26cc63d60  fix(agent-api): a JSON body that is not an object 400s, not 500s
0f2bcc5a7  style(agent-api): hold the new comments to the 2-line prose cap

The first is a real behavior change (status code), so my approval does not cover the current head. With dismiss_stale_reviews_on_push: false the tick still counts toward the gate and the PR reads CLEAN. Smaller than the other case and likely fine — but flagging so it is not merged on a stale vote. Re-review to follow.

[john-the-dev]

Re-approving at head b18e9de. My earlier approval (f272573, 08:36Z) is now superseded — this one covers the current head.

One correction to the ask, and it changes who should review what. The 73-line .githooks/pre-push is not this PR's work. Attributing commits-since-approval by parent count:

26cc63d60  parents=1  AUTHORED    src/agent-api.py, tests
0f2bcc5a7  parents=1  AUTHORED    src/agent-api.py, tests
b18e9de70  parents=2  BASE-MERGE  .githooks/pre-push   <- the hook arrives here

The hook came in with the base merge, and it is already on main (3406 bytes, confirmed at ?ref=main). So reviewing it here would be reviewing already-merged code, and a finding against it could not be actioned on this PR. The fail-open design and the SUTANDO_SKIP_REVIEW_CHECKS escape are still worth a second opinion — they just need to be raised against whichever PR authored them, or as a new issue, not against #3704.

The real authored delta is 2 commits / 2 files, and it is good. The guard is correct: json.loads accepts any JSON value, so a well-formed [1] or "hi" reaches .get() and raises AttributeError outside the enclosing except — an uncaught 500 for an authenticated caller, where 400 is right. Tests updated in the same commits. The comment is 2 lines at head, within the repo cap, after 0f2bcc5 trimmed it from 3.

Worth noting the general shape, since it cuts both ways: a raw commits-since count is inflated by base merges, so "+3 commits and a new 73-line file" and "+2 authored commits touching 2 files" describe the same PR. Splitting on parent count is what separates them — it confirmed staleness on #3705 (9 authored, 0 merges) and refutes the scary half here.

[sonichi]

Delta re-review at b18e9de (prior review at f272573). The delta since is one base-merge plus two real commits — 26cc63d6 (the 400-not-500 fix) and 0f2bcc5a (prose-cap trim). Both good; my earlier review carries.

26cc63d6 is squarely in the error-handling boundary I reviewed, and it's correct: json.loads succeeds on any JSON value, so a well-formed [1] or "hi" sailed past the parse except and hit data.get("texts") → AttributeError → an uncaught 500 and a dropped connection for an authenticated caller, where the contract is a 400. The if not isinstance(data, dict): 400 guard, placed right after the parse and before .get(), is the right fix and the right place. The test adds [1] and "hi" to the bad-shape loop — I ran it, all /scan-text checks green, and the commit's red-to-green (revert → AttributeError at agent-api.py:1343) is the honest control. Nice catch by qingyun-wu on the shape.

Verified at this head: review-checks PASS (hardcoded-paths + root-artifacts + prose-cap clean — 0f2bcc5a held the new comment to the 2-line cap). The transport-vs-policy boundary and the no-exemption mutation control from my first pass are unchanged by the delta. LGTM.

[TustinOC]

Re-reviewed at a7f14ba201. My approval sat at f272573369, so it endorsed code I had not read — disclosing rather than letting the tick ride.

Of the 7 commits in the range, 5 are base merges pulling #3703/#3705/#3709 off main. Yours are two.

26cc63d6 is a real defect, correctly placed. json.loads succeeds on any JSON value, so [1] or "hi" parse cleanly and then reach data.get("texts") outside the parse except — AttributeError, an uncaught 500 for an authenticated caller against a documented 400. The isinstance(data, dict) guard goes after the except and before .get(), which is the only position that closes it, and it covers null for free (json.loads("null") → None, not a dict). The test extends the existing bad-shape loop with [1], "hi" rather than adding a parallel case, so it fails in the same place for the same reason as its neighbours.

0f2bcc5a is worth more than its "style" label suggests. Your message records that the prose-cap failure at step 8 meant every later step in that job — typecheck and tests included — was skipped rather than run. That is the dangerous shape: a job that stops early reports absence of failure indistinguishably from a job that ran and passed. Worth keeping that sentence in the commit record; it is the kind of thing a future reader needs when a green-looking run turns out to have executed almost nothing.

Approval re-affirmed on the current head. Advisory only (CONTRIBUTOR), so it does not count toward the gate.

[github-actions]

@cla-assistant check