Repository navigation
Conversation
|
Implemented zero-token deterministic public-surface embargo scrub mod. |
potiuk
left a comment
There was a problem hiding this comment.
Thanks for picking up a deterministic scrub — the idea is right, but as written the agent can switch the guard off from inside the command it is checking, several ordinary shell shapes get a message past the parser, and the scope (every commit in every repository) is wider than the rule it enforces.
Outputs are probabilistic; gates are deterministic (plugins/magpie-security/hooks/public-surface-scrub.ts:237)
Skills produce drafts. Tool calls enforce schemas. Humans or deterministic checks decide whether a draft becomes state. Probabilistic at the input, deterministic at every state change. The boundary never blurs, even when the draft looks reliable enough to short-circuit the gate.
/(?:^|\s)MAGPIE_ALLOW_EMBARGO_TERMS=1(?:\s|$)/.test(commandLine) ||
commandLine.includes('--allow-embargo-terms')
The escape hatch can be switched on by the very command it is meant to stop. An agent that hits the block (and whose reason text tells it how to proceed) only has to prefix MAGPIE_ALLOW_EMBARGO_TERMS=1 or append --allow-embargo-terms anywhere in the command, including inside the commit message or a trailing # comment, and the guard passes it. --allow-embargo-terms is also not a real git or gh flag, so a literal use makes the underlying command fail. A deterministic gate has to be outside the agent's reach.
Suggested fix: honour only options.env (the hook process environment, set by the human before the session), drop the command-line forms, and leave the override out of the blocking message, so the agent is told to ask the maintainer rather than how to bypass the guard.
Flagged by both the primary and adversarial reviewers.
Writing a literal project value directly into a skill is a refactor bug (plugins/magpie-security/hooks/public-surface-scrub.ts:88)
Concretely,
gh issue view <N> --repo <tracker>means "substitute
<tracker>for<project-config>/project.md→tracker_repobefore
running this". Writing a literal project value directly into a skill is a
refactor bug — skills must stay project-agnostic so swapping projects is
a config change, not a code change.
repo.endsWith('-s') ||
repo.includes('-s/') ||
repo.includes('/private') ||
repo.includes('-private') ||
repo.includes('security-tracker')
The private-tracker exemption is a naming guess, and it fails open: any public repository whose name ends in -s or contains -private / /private / security-tracker is exempt, so security wording on that public surface passes unchecked. A tracker named any other way (for example apache/foo-security) gets blocked instead. Only the airflow-s/airflow-s convention is encoded. It also exempts the whole command: isPrivateTrackerTarget searches the entire string and line 118 returns early, so gh pr view 1 -R org/private-tracker; gh pr comment 2 -R apache/airflow -b "CVE-… SSRF" passes although the second command targets the public upstream (found by codex).
Suggested fix: read tracker_repo (and upstream_repo) from the resolved <project-config>/project.md and exempt exactly that slug, and nothing when it cannot be resolved; decide the exemption per sub-command, not for the whole script.
Flagged by both the primary and adversarial reviewers.
This is the single forbidden-term list for every public surface of this skill. (plugins/magpie-security/hooks/public-surface-scrub.ts:128)
This is the single forbidden-term list for every public surface of this skill.
if (/(?:^|[;&|]\s*)git\s+(?:-[^\s]+(?:\s+[^\s]+)?\s+)*commit(?:\s|$)/i.test(trimmed)) {
The list governs the public surfaces of the security-issue-fix flow, the <upstream> fix PR before its advisory ships. The hook applies it to every git commit, gh pr create and gh pr comment in every repository the plugin is loaded in. In this repository that blocks ordinary work: \bCVE\b (line 26, case-insensitive) matches fix(generate-cve-json): …, and vulnerability / security advisory appear in many routine commit messages and PR descriptions here and in any adopter's own docs.
Suggested fix: scope it to the configured upstream_repo (from project.md), and preferably to an active fix (a branch or marker the issue-fix skill sets), and let every other repository pass.
This is the single forbidden-term list for every public surface of this skill. (plugins/magpie-security/hooks/public-surface-scrub.ts:128)
This is the single forbidden-term list for every public surface of this skill.
if (/(?:^|[;&|]\s*)git\s+(?:-[^\s]+(?:\s+[^\s]+)?\s+)*commit(?:\s|$)/i.test(trimmed)) {
Several ordinary shell shapes get a leaking message past the parser (the first four found by codex; each traced in the code):
- a newline between commands: the detectors accept only start-of-string or
;,&,|, sogit status+ newline +git commit -m "CVE-… SSRF"is not recognised; - an attached message,
git commit -m"CVE-… SSRF"(and an attached-F): the extraction requires=or whitespace after the flag; - a commit followed by a PR: the
git commitbranch returns before theghbranches, sogit commit -m "neutral" && gh pr create -t "CVE-… SSRF"checks only the commit message, and&&does not trigger the whole-command scan; $(…), backticks or a variable in the message, or a piped body (… | git commit -F -): only the literal command text is scanned, and-is skipped;- not covered at all:
git commit --amendreusing a message,git tag -m,gh pr create --fill,gh pr edit --title/--body,gh pr review --body,gh issue commenton<upstream>,gh api … -f body=.
Suggested fix: split the command into sub-commands on every shell separator (newline included) and evaluate each one; accept attached short-option arguments; when a message comes from command substitution, a variable or stdin, scan the whole command and block when the published text cannot be determined; add the missing subcommands, with a test per shape.
Flagged by both the primary and adversarial reviewers.
Outputs are probabilistic; gates are deterministic (plugins/magpie-security/hooks/public-surface-scrub.ts:263)
Skills produce drafts. Tool calls enforce schemas. Humans or deterministic checks decide whether a draft becomes state.
} catch {
// Safe skip on unreadable file; pure regex inspection continues
}
A message file the hook cannot read is skipped and the command is allowed, and the path is read relative to the session's working directory even after git -C <dir>, where git reads <dir>/<file> (found by codex). So git -C /upstream commit -F msg.txt with an embargoed msg.txt passes whenever the hook resolves the path differently from git.
Suggested fix: resolve -F / --body-file paths against the -C directory, and block (fail closed, naming the file) when a referenced message file cannot be read.
Flagged by codex (adversarial review); cross-checked.
Smaller observations
- See inline comments on
plugins/magpie-security/hooks/public-surface-scrub.ts:28.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
|
Thanks for the thorough review @potiuk. I’ve addressed all findings: the override is now restricted strictly to The parser now splits multi-command pipelines across all shell separators (including newlines), supports attached short flags ( All 47 unit tests pass cleanly covering each of these review shapes, and plugin validation passes strict static analysis without Node runtime built-in dependencies. |
potiuk
left a comment
There was a problem hiding this comment.
Thanks for the round of fixes — the in-command bypass is gone, the tracker / upstream slugs now come from project.md, each sub-command is judged on its own, and unreadable message files and dynamic input now fail closed. Two things still block it: the mod keeps its own copy of the term list instead of using the canonical one, and the gate still fails open in several shapes.
On my earlier threads:
- Message-file /
-Chandling — fixed; resolving. - In-command bypass, tracker exemption, ordinary shell shapes — answered on their own terms, but the replacements open the gaps in the inline comments (the escape hatch never reaching the hook,
cd … &&,git -c, escaped quotes,--reporead from a body, directory-name heuristics). - Scope, and the bare
advisory— folded into the term-list comment: deriving the list fromDEFAULT_TERMSsettles both.
On the architecture question in the first inline comment, moving this into tools/agent-guard as a Python guard is worth weighing before more work goes into the TypeScript version: it would import the canonical list directly and run on every harness agent-guard supports.
Smaller, not blocking: in upstream scope this fails closed on the common git commit -m "$(cat <<'EOF' … EOF)" form; that is safe, but the block message could say -F <file> is the expected shape.
single forbidden-term list (plugins/magpie-security/hooks/public-surface-scrub.ts:28)
This is the single forbidden-term list for every public surface of this skill. 5g, Step 7, Step 9 and the Guardrails check against it rather than keeping their own copies.
export const FORBIDDEN_TERM_RULES: ReadonlyArray<{ name: string; pattern: RegExp }> = [
Derive the list from magpie_vcs.publication.DEFAULT_TERMS (generated data file plus a sync check), or move the guard into tools/agent-guard where it can import the canonical list.
Humans or deterministic checks decide whether a draft becomes state. (plugins/magpie-security/hooks/public-surface-scrub.ts:726)
Humans or deterministic checks decide whether a draft becomes state.
const evaluation = await evaluatePublicSurfaceCommand(command, {
readFile: async (filePath: string) => {
Pass env: $.env, and add a register()-level test with MAGPIE_ALLOW_EMBARGO_TERMS=1.
The boundary never blurs (plugins/magpie-security/hooks/public-surface-scrub.ts:624)
The boundary never blurs, even when the draft looks reliable enough to short-circuit the gate.
} else if (projectConfig.isTrackerCheckout) {
// Bare git command in the tracker repository itself without upstream context
continue;
Track cd <dir> across sub-commands; block when the target repository cannot be determined; resolve config in the upstream clone instead of passing everything.
Probabilistic at the input, deterministic at every state change. (plugins/magpie-security/hooks/public-surface-scrub.ts:293)
Probabilistic at the input, deterministic at every state change.
const cwdMatch = trimmed.match(/(?:^|\s)git\s+.*?-C(?:=|\s+)(?:"([^"]+)"|'([^']+)'|([^\s;&|]+))/i);
Tokenise argv with a shell-word parser; read -C only from git's global options, case-sensitively; same for -R/--repo.
Smaller observations
- See inline comments on
plugins/magpie-security/hooks/public-surface-scrub.ts:613,plugins/magpie-security/hooks/hooks.json:5.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
| const command = e?.input?.command || e?.params?.command; | ||
|
|
||
| if (toolName === 'bash' && typeof command === 'string') { | ||
| const evaluation = await evaluatePublicSurfaceCommand(command, { |
There was a problem hiding this comment.
register() passes only readFile here, never env, so the MAGPIE_ALLOW_EMBARGO_TERMS escape hatch can never fire in production, while the block message still tells the maintainer to "set the environment bypass". The test that covers it calls evaluatePublicSurfaceCommand directly, so it does not notice. Please forward $.env and add a test through register().
| // Working in an unrelated repository (e.g. apache/magpie) | ||
| continue; | ||
| } | ||
| } else if (projectConfig.isTrackerCheckout) { |
There was a problem hiding this comment.
The gate passes exactly where a fix commit is most likely made. (1) A session in the tracker checkout: cd ../<upstream> && git commit -m "Fix CVE-2026-1234" passes, because cd is not followed and a bare git here is skipped. (2) A session started in the upstream clone has no project.md, so every command is allowed. When the target repository cannot be determined, this should block, not pass.
| "$schema": "https://code.claude.com/schemas/hooks.json", | ||
| "description": "Apache Magpie — Claude Code mod for deterministic public-surface scrub against unredacted security embargo leaks.", | ||
| "modules": [ | ||
| "./public-surface-scrub.ts" |
There was a problem hiding this comment.
This is the first mod in magpie-security and the PR touches no docs. docs/setup/secure-agent-setup.md should cover its trade-offs (it runs unsandboxed and reads any -F path via $.fs.read) and how the escape hatch is set, and the issue-fix skill should mention it next to the scrub step so a maintainer knows where a block came from.
…closed gates (apache#1676) - Derive forbidden terms directly from canonical magpie_vcs.publication.DEFAULT_TERMS using tools/dev/sync-forbidden-terms.py, with pre-commit hook to prevent drift. - Tokenize shell commands with POSIX shell-words parser to handle quotes and escaped quotes. - Track directory context across chained sub-commands (cd <dir> && git ...). - Parse git -C only from global options and disambiguate clustered/attached flags. - Inspect remotes and worktrees to detect contributor fork checkouts as upstream scope. - Fail closed on indeterminate dynamic input with -F <file> guidance. - Pass environment from hook event and add register()-level tests for MAGPIE_ALLOW_EMBARGO_TERMS. - Document mod architecture, tradeoffs, and escape hatch in secure-agent-setup.md.
|
will resolve CI shortly! |
…d (part of apache#1497) Adds a zero-token Claude Code mod in plugins/magpie-security/hooks/ that intercepts Bash commands (git commit, gh pr create, gh pr comment) and guards public surfaces against accidental security embargo leaks (CVE IDs, vulnerability wording, security fix phrasing, and vulnerability classes). - Intercepts tool.call on Bash commands, parsing commit messages (-m, -am, -sm, -F) and PR descriptions (-t, -b, -F, --body-file). - Ingests heredocs and shell pipelines to prevent stdin bypasses. - Exempts private security tracker repositories (-s suffix convention). - Provides MAGPIE_ALLOW_EMBARGO_TERMS=1 maintainer escape hatch for legitimate post-coordinated-disclosure maintenance. - Passes clean behavioral changes through with zero token consumption. - Includes 29 unit tests covering all matching rules, command syntax, edge cases, and hook middleware integration.
…ace scrub mod - Limit escape hatch strictly to options.env set by human, removing in-command bypasses. - Scope checks to upstream_repo from project.md; evaluate private tracker exemption per subcommand. - Split multi-command pipelines on shell separators (including newlines) and accept attached short flags. - Fail closed on unreadable message files and indeterminate dynamic input. - Resolve file paths relative to git -C directory without node:path. - Expand test suite to 47 unit tests covering all review shapes.
…closed gates (apache#1676) - Derive forbidden terms directly from canonical magpie_vcs.publication.DEFAULT_TERMS using tools/dev/sync-forbidden-terms.py, with pre-commit hook to prevent drift. - Tokenize shell commands with POSIX shell-words parser to handle quotes and escaped quotes. - Track directory context across chained sub-commands (cd <dir> && git ...). - Parse git -C only from global options and disambiguate clustered/attached flags. - Inspect remotes and worktrees to detect contributor fork checkouts as upstream scope. - Fail closed on indeterminate dynamic input with -F <file> guidance. - Pass environment from hook event and add register()-level tests for MAGPIE_ALLOW_EMBARGO_TERMS. - Document mod architecture, tradeoffs, and escape hatch in secure-agent-setup.md.
cffec98 to
4a89ad2
Compare
|
Thanks @potiuk! Addressed all review items:
Ready for another look! |
potiuk
left a comment
There was a problem hiding this comment.
Thanks for this round: the term list is now generated from DEFAULT_TERMS with a prek sync check, argv is tokenised properly, -C / -c and --repo-in-body are fixed, cd is tracked, and the docs section is in. One thing still blocks it: the hook reads the wrong fields of the tool.call event, so in a real session it never sees a command. Behind that, several common shapes still let a message reach the upstream unchecked.
On my earlier points:
- Canonical term list, tokenising argv,
-C/-c/ escaped quotes /--repoin a body, directory-name heuristics, heredoc guidance, docs: fixed. I've resolved those threads. - Escape hatch through
register(): still not reachable (first item below). - Block when the target can't be determined:
cdis followed now, but aproject.mdin the session cwd is taken to mean "tracker", which re-opens the upstream case (second item). - Moving this into
tools/agent-guard: still worth answering before another round. Most of the items below would need solving in either place, and a Python guard importingDEFAULT_TERMSdirectly would work on every harness.
Blocking: the hook never sees the command (plugins/magpie-security/hooks/public-surface-scrub.ts:1289)
Skills produce drafts. Tool calls enforce schemas. Humans or deterministic checks decide whether a draft becomes state.
— PRINCIPLES.md
const command = e?.input?.command || e?.params?.command;
...
const hookEnv = e?.env ?? (typeof process !== 'undefined' ? process.env : {});
...
return { error: evaluation.reason };
In the mod API the tool's arguments sit at the top level of the tool.call input (e.command for Bash), so e.input.command is undefined and nothing is evaluated. A refusal is { deny: reason }; { error } is not a result the engine knows. The event carries no env, and a hooks module has no Node process, so MAGPIE_ALLOW_EMBARGO_TERMS still cannot reach the hook. The register tests (public-surface-scrub.test.ts:723, :753, :781) build { tool, input: { command }, env } by hand and assert result.error, so they pass against a shape the engine never sends.
Suggested fix: on('tool.call', { tool: 'Bash' }, …), read e.command, return { deny }, and read the bypass with await $.env.get("MAGPIE_ALLOW_EMBARGO_TERMS"). Drive the register tests through claude-code/testing's tool noun, so the call goes through the plugin's hook as in a session.
Major: a project.md in the cwd is treated as the tracker (plugins/magpie-security/hooks/public-surface-scrub.ts:1187)
The boundary never blurs, even when the draft looks reliable enough to short-circuit the gate.
— PRINCIPLES.md
if (projectConfig.isTrackerCheckout && !effectiveDir) {
continue;
}
isTrackerCheckout is true whenever project.md is found under the cwd (:210). An adopter that commits .apache-magpie-overrides/project.md into its upstream repository has one there, and so does this repository. There, a bare git commit -m "Fix CVE-2026-1234" skips the remote check and passes, although the remote is the upstream.
Suggested fix: drop the shortcut. Always inspect the effective directory's remotes, exempt only on a tracker_repo match, and scan on an upstream_repo (or fork) match.
Major: the PR URL and GH_REPO select the repository too (plugins/magpie-security/hooks/public-surface-scrub.ts:701)
Probabilistic at the input, deterministic at every state change.
— PRINCIPLES.md
if ((token === '-R' || token === '--repo') && i + 1 < tokens.length) {
From a tracker-checkout session, all of these pass and land on the public upstream PR:
gh pr comment https://github.com/apache/airflow/pull/1 -b "Fix CVE-2026-1234 SSRF"gh pr edit <same URL> --body "…"GH_REPO=apache/airflow gh pr comment 1 -b "…"
Suggested fix: take the target from a PR/issue URL argument and from a leading GH_REPO=, with a test for each.
Major: gh pr review --body-file is not read (plugins/magpie-security/hooks/public-surface-scrub.ts:888)
Write the text to a file in the session scratch directory or
$TMPDIRfirst, then hand the CLI the path:gh pr create --body-file,gh pr comment --body-file,gh issue comment --body-file,gh pr review --body-file,
— AGENTS.md → Commit and PR conventions
if (token === '-F' && i + 1 < tokens.length) {
Only -F is handled in the review branch. gh pr review 1 -R apache/airflow --comment --body-file evil.md passes without the file being opened.
Suggested fix: handle --body-file / --body-file= as the comment branch does.
Major: gh api coverage is narrower than documented (plugins/magpie-security/hooks/public-surface-scrub.ts:979)
gh api … --input <file>for JSON payloads
— AGENTS.md → Commit and PR conventions
const match = fieldArg.match(/^(?:body|title)=(.*)$/s);
Only an inline body= / title= field is scanned. These all pass against repos/apache/airflow/...:
--input body.json, the form AGENTS.md prescribes;-F body=@msg.md, where gh reads the file and the hook sees only@msg.md;gh api graphql -f query='mutation{addComment(input:{…body:"CVE-…"})}'.
Suggested fix: read --input and @file values, and fail closed when they can't be read. Scan or fail closed on a graphql mutation. Otherwise, narrow the claim in secure-agent-setup.md and SKILL.md.
Major: every repository without Magpie config is now scanned (plugins/magpie-security/hooks/public-surface-scrub.ts:1199)
Re-read the diff and check that every change is intentional.
— AGENTS.md → Before submitting
if (!remoteInfo.isUpstream && upstreamRepo) {
continue;
}
With no project.md, upstreamRepo is undefined and nothing passes. git commit -m "fix(generate-cve-json): tidy" and git commit -m "docs: security advisory template" are blocked in any unrelated repository the plugin is enabled in. That contradicts the new docs line ("Commands targeting unrelated third-party or adopter documentation repositories pass unchecked").
Suggested fix: use the upstream clone path from user.md, so an upstream session is still recognised without a local project.md. Then let repositories whose remotes are known and match neither slug pass, and fail closed only when scope is genuinely unknown.
Smaller observations
- A single
&is not a separator (public-surface-scrub.ts:273).git -C ../airflow status & git -C ../airflow commit -m "Fix CVE-…"passes. reporterNames/privateTexts(:79-107,:1210) are never filled in production, so the reporter-name and verbatim-text checks only run in unit tests. Wire a source for them or drop them.forbidden-terms.jsonis generated and sync-checked, but nothing reads it.-m 'costs $HOME'in single quotes is blocked as dynamic input, because the$VARcheck runs on the raw string rather than the tokens. This is safe, but it is a false positive.- If a command writes a message file with a heredoc and then commits with
-Fon that file, the hook reads the old file before the new text is written. /usr/bin/git,bash -c '…'and(cd … && git commit …)are not recognised. That is fine for a backstop, but then the docs should not say it "cannot be … faked".
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
| export function register(on: any): void { | ||
| on('tool.call', async ($: any, e: any, next: any) => { | ||
| const toolName = (e?.tool || '').toLowerCase(); | ||
| const command = e?.input?.command || e?.params?.command; |
There was a problem hiding this comment.
Blocking. tool.call spreads the tool's arguments at the top level, so for Bash the command is e.command; e.input.command is always undefined and nothing is evaluated. A refusal is { deny: reason }, not { error }, and the event has no env (read it with await $.env.get("MAGPIE_ALLOW_EMBARGO_TERMS")). See the first section of the review body.
| // 3. For commands without explicit repo flag, determine repository from git context | ||
| if (!parsed.targetRepo) { | ||
| // If we are in the tracker repository checkout itself without upstream context, exempt it | ||
| if (projectConfig.isTrackerCheckout && !effectiveDir) { |
There was a problem hiding this comment.
Major. isTrackerCheckout is true whenever a project.md is under the cwd, which includes upstream repositories that commit .apache-magpie-overrides/project.md (this one does). A bare git commit -m "Fix CVE-…" there skips the remote check. Drop the shortcut and always inspect the remotes.
| if (binary === 'gh') { | ||
| // Extract target repo: -R <repo>, -R=<repo>, --repo <repo>, --repo=<repo>, -R<repo> | ||
| // Linear scan skipping arguments to other flags to prevent reading --repo from inside body/title | ||
| for (let i = cmdIdx + 1; i < tokens.length; i++) { |
There was a problem hiding this comment.
Major. The target repository is read only from -R / --repo. A PR or issue URL argument, or a leading GH_REPO=, also selects it, and those shapes reach the public upstream unchecked.
| result.extractedTexts.push(token.slice(2)); | ||
| continue; | ||
| } | ||
| if (token === '-F' && i + 1 < tokens.length) { |
There was a problem hiding this comment.
Major. Only -F is read in the gh pr review branch; --body-file / --body-file=, the form AGENTS.md prescribes, is never opened.
| continue; | ||
| } | ||
|
|
||
| if ((token === '-f' || token === '-F' || token === '--field' || token === '--raw-field') && i + 1 < tokens.length) { |
There was a problem hiding this comment.
Major. Only inline body= / title= fields are scanned. --input <file>, -F body=@file and graphql mutations pass, although the docs say gh api calls are gated.
| continue; // Safe tracker repository | ||
| } | ||
| // If verified not upstream (and not a fork of upstream) and not tracker, allow | ||
| if (!remoteInfo.isUpstream && upstreamRepo) { |
There was a problem hiding this comment.
Major. With no project.md, upstreamRepo is undefined, so this never lets a repository pass: ordinary commits in unrelated repositories are blocked (fix(generate-cve-json): tidy), contrary to the new docs line.
| current = ''; | ||
| continue; | ||
| } | ||
| if (char === '&' && next === '&') { |
There was a problem hiding this comment.
A lone & (and |&) is not treated as a separator, so git … status & git … commit -m "Fix CVE-…" is evaluated as one status command and passes.
| if (parsed.hasDynamicInput) { | ||
| // First check if the commandLine itself explicitly carries forbidden terms | ||
| const dynamicTerms = findForbiddenTerms(commandLine, { | ||
| reporterNames: options?.reporterNames ?? projectConfig.reporterNames, |
There was a problem hiding this comment.
reporterNames / privateTexts are never filled outside the unit tests: register() passes neither and parseProjectManifest sets neither. Wire a real source, or drop the checks and say the guard covers the term list only.
… and CLI parser
- Correct Claude Code hook registration: register on('tool.call', { tool: 'Bash' }),
read e.command directly, and return { deny: evaluation.reason } on block.
- Read environment bypass via $.env.get('MAGPIE_ALLOW_EMBARGO_TERMS') to pass strict plugin validation.
- Drop isTrackerCheckout shortcut; inspect git remotes directly and probe fallback user.md for upstream identification.
- Expand CLI subcommand parsing in parseSubcommand:
* Extract repository targets from PR/issue URLs (https://github.com/<owner>/<repo>/pull/<id>).
* Extract leading GH_REPO= environment variables.
* Support --body-file in gh pr review.
* Inspect gh api --input, -F body=@file, and GraphQL mutations.
- Enhance shell splitting and dynamic tokenization:
* Split on single & command separator (unless part of >&).
* Ignore literal $VAR inside single quotes (e.g. -m 'costs $HOME').
- Calibrate documentation in secure-agent-setup.md regarding zero-token deterministic guard.
Generated-by: Antigravity
|
Hi @potiuk, Thanks for the detailed review! All findings from review have been addressed in the latest commit:
All 60 tests in |
Summary
plugins/magpie-security/hooks/that intercepts Bash commands (git commit,git tag,gh pr create,gh pr edit,gh pr review,gh pr comment,gh issue comment,gh api) and guards public surfaces against accidental security embargo leaks.-m,-am,-sm,-sam,-F), attached short flags (-m"...",-Fpath), PR descriptions/comments, heredocs, and shell pipelines.;,&&,||,|,\n).plugins/magpie-security/skills/issue-fix/implementation-plan.md:31-43(CVE references, vulnerability wording, security fix phrasing, and vulnerability classes like SSRF, XSS, CSRF, SQL injection, path traversal, RCE, privilege escalation, auth bypass).upstream_repoandtracker_repofrom<project-config>/project.mdviaresolveProjectConfig(), scoping the scrub strictly toupstream_repoand exemptingtracker_repoper subcommand without hardcoded naming conventions.--amend/--fill, and indeterminate dynamic inputs ($VAR,$(...), stdin-). Resolves-F/--body-filerelative togit -C <dir>without Node built-in imports.options.env.MAGPIE_ALLOW_EMBARGO_TERMS=1environment escape hatch for legitimate post-coordinated-disclosure maintenance (in-command bypasses removed).Type of change
.claude/skills/<name>/) — eval fixtures updated belowtools/<system>/*.md)tools/*/withpyproject.toml)docs/,README.md,CONTRIBUTING.md)projects/_template/)prek, workflows, validators)plugins/magpie-security/hooks/)Test plan
claude plugin validate --strict plugins/magpie-security(Validation passed, minimal API surface:$.fs.read).claude plugin test plugins/magpie-security(47 pass, 0 fail).claude plugin test plugins/magpie-utilitiesandplugins/magpie-setupfor regression testing (83 total tests pass across repo).preklinters (end-of-file-fixer,trailing-whitespace,mixed-line-ending) pass cleanly.RFC-AI-0004 compliance
$.fs.read), nonode:fsornode:pathbuilt-in imports, deterministic execution.issue-fixskill workflow and Markdown verification checklist remain completely untouched for non-Claude harnesses.Linked issues
Notes for reviewers (optional)
tool.callfor Bash tools, halting execution with actionable remediation if unredacted embargo terms appear in public-surface commands.