Skip to content

Sign bundled plugins during release with drift guard - #1298

Open
chr1syy wants to merge 10 commits into
RunMaestro:rcfrom
chr1syy:fix/a1-agent-flow-signing
Open

Sign bundled plugins during release with drift guard#1298
chr1syy wants to merge 10 commits into
RunMaestro:rcfrom
chr1syy:fix/a1-agent-flow-signing

Conversation

@chr1syy

@chr1syy chr1syy commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Finding A1 - Agent Flow ships unsigned, so its code never runs

Bundled plugins (Agent Flow) ship without a signature, so main.js never executes (isRunnable() requires signature.status === 'trusted'). The panel renders but sits on "Waiting for agent activity..." forever.

Change (code / CI / test / docs)

  • Adds a release-CI step that signs every bundled plugin with the publisher key (reuses the existing maestro-cli plugin sign), so the baked trust anchor resolves them trusted at seed time.
  • Anchor/secret drift guard that fails the release if the CI signing key and publisher-keys.ts diverge (fail-closed).
  • .gitignore entry for the release-only signature.json artifact.
  • Dev/testing docs in CLAUDE-PLUGINS.md.
  • Trust-anchor round-trip unit test.
  • publisher-keys.ts is intentionally NOT modified here (MAESTRO_PUBLISHER_KEYS stays empty).

Validation

Scoped test rpc-and-signing.test.ts -> 17/17 pass. ESLint + Prettier clean. Type-check: the only .ts change is the test file (verified type-clean in isolation); the full npm run lint is slow in this environment and is left to CI.

⚠️ Human follow-up required (by design - not in this PR)

This PR is not functionally complete on its own. A maintainer must still:

  1. Mint the real production signing keypair.
  2. Add the private key as the MAESTRO_PLUGIN_SIGNING_KEY GitHub Actions secret.
  3. Bake the matching base64 SPKI public key into MAESTRO_PUBLISHER_KEYS in src/shared/plugins/publisher-keys.ts.

Until then bundled plugins remain unsigned; the drift guard hard-fails the release while the anchor is empty, which is the intended fail-closed behavior.

Fix plan: .maestro/FIX-A1-01.md.

Summary by CodeRabbit

  • New Features
    • Bundled plugins are now signed during release builds and verified against a built-in publisher key before packaging.
    • Trusted publisher keys support plugin authenticity validation.
  • Documentation
    • Added guidance for signing plugins in release and development builds, including trust behavior, key rotation, and local testing.
  • Tests
    • Added checks to ensure publisher keys use the expected format and key type.
  • Chores
    • Updated ignore rules for generated plugin signature artifacts.
    • Added safeguards for missing signing credentials and untrusted bundled plugins.

chr1syy and others added 5 commits July 25, 2026 14:16
Add a 'Sign bundled plugins' step to the release workflow's build job,
placed after 'Build application' (which emits dist/cli/maestro-cli.js)
and before all electron-builder packaging steps. It writes the
MAESTRO_PLUGIN_SIGNING_KEY secret to a temp PEM, loops over every
examples/plugins/*/ dir signing each with maestro-cli plugin sign, then
removes the key. Because extraResources copies examples/plugins/* into
packaged resources, the freshly written signature.json ships with the
plugin so the baked MAESTRO_PUBLISHER_KEYS anchor resolves it trusted at
seed time and Agent Flow's main.js is allowed to run.

The workflow is a single matrix build job, so one shared step runs on
every platform leg; signing is deterministic given the same files+key,
so per-leg signing is safe. shell: bash keeps it identical on Windows.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
After signing bundled plugins, verify agent-flow's signature resolves
'trusted' against the baked MAESTRO_PUBLISHER_KEYS anchor. A rotated CI
secret with a stale publisher-keys.ts (or an empty anchor) now fails the
release instead of shipping a permanently seed-skipped, non-runnable
plugin. Adds set -euo pipefail and an explicit empty-anchor error.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…igning workflow

Add examples/plugins/*/signature.json to .gitignore so a locally-run
plugin sign never gets committed (it is a release-time build artifact).

Document the bundled-plugin signing flow in CLAUDE-PLUGINS.md: the release
CI path (CI secret -> plugin sign -> extraResources -> baked
MAESTRO_PUBLISHER_KEYS anchor -> drift guard), the local dev workflow
(--gen-key, add SPKI key to pluginTrustedKeys, enable + consent), and the
v1 single-key/no-revocation trust model with additive-then-prune rotation.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a resolveTrustedKeys describe block to the shared signing suite:
merge/trim/de-dup contract with user keys, resolveTrustedKeys([]) equals
the trimmed baked anchor set, and a guarded loop proving every baked
MAESTRO_PUBLISHER_KEYS entry is a non-empty, valid base64 SPKI key that
crypto.createPublicKey accepts (asserts nothing while the anchor is empty
pre-key-mint, never hard-codes the real key value).

The fixture-keypair verifyPluginSignature round trip and the CLI
sign/validate round trip were already fully covered, so no duplicate
suites were added.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Release CI signs bundled plugins and verifies their trusted status against baked publisher keys. Generated signatures are ignored, documentation describes release and development trust flows, and tests validate the baked key format.

Changes

Bundled plugin trust

Layer / File(s) Summary
Publisher-key validation tests
src/shared/plugins/publisher-keys.ts, src/__tests__/shared/plugins/publisher-keys.test.ts
The built-in publisher key is populated. Tests verify non-empty canonical base64 DER SPKI Ed25519 keys.
Release signing and trust guard
.github/workflows/release.yml, .gitignore, CLAUDE-PLUGINS.md
Release CI signs bundled plugins, validates agent-flow as trusted, ignores generated signatures, and documents release and development trust behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ReleaseCI
  participant MaestroCLI
  participant BundledPlugins
  participant PublisherKeys
  ReleaseCI->>MaestroCLI: Sign bundled plugin directories
  MaestroCLI->>BundledPlugins: Write signature.json
  ReleaseCI->>PublisherKeys: Read MAESTRO_PUBLISHER_KEYS
  ReleaseCI->>MaestroCLI: Validate agent-flow with trusted keys
  MaestroCLI-->>ReleaseCI: Return trusted status
Loading

Possibly related PRs

  • RunMaestro/Maestro#1266: Implements bundled-plugin seeding and trusted-key resolution used by this release validation.

Suggested labels: approved

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: signing bundled plugins during release with a drift guard.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 25, 2026

Copy link
Copy Markdown

Greptile Summary

The PR adds release-time signing and validation for bundled plugins.

  • Bakes an Ed25519 publisher public key and tests its canonical SPKI representation.
  • Signs bundled plugin directories before packaging and checks Agent Flow against the baked trust anchor.
  • Documents release and local-development signing behavior and ignores generated signature artifacts.

Confidence Score: 4/5

The PR is not yet safe to merge because a release can still succeed without the signing secret and ship Agent Flow in an unusable unsigned state.

The anchor is populated at current HEAD, but the workflow exits successfully whenever the private signing secret is missing; packaging then proceeds without signature.json, and the startup trust gate skips the bundled plugin.

Files Needing Attention: .github/workflows/release.yml

Important Files Changed

Filename Overview
.github/workflows/release.yml Adds release signing and drift validation, but a missing signing secret still permits packaging an unsigned Agent Flow after the anchor has been baked.
src/shared/plugins/publisher-keys.ts Populates the built-in trust anchor with one Ed25519 SPKI public key.
src/tests/shared/plugins/publisher-keys.test.ts Verifies that the baked anchor is populated, canonical base64 SPKI, and Ed25519.
CLAUDE-PLUGINS.md Documents bundled-plugin release signing, local testing, and key rotation.
.gitignore Ignores release-generated plugin signature files.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Build application] --> B[Load baked publisher anchor]
  B --> C{Anchor and signing secret available?}
  C -->|Yes| D[Sign bundled plugins]
  D --> E[Validate Agent Flow as trusted]
  E -->|Trusted| F[Package release]
  E -->|Not trusted| G[Fail release]
  C -->|No| F
  F --> H[Seed bundled plugins at startup]
  H --> I{Signature trusted?}
  I -->|Yes| J[Install and enable feature]
  I -->|No| K[Skip bundled plugin]
Loading

Reviews (2): Last reviewed commit: "fix(plugins): bake the publisher key tha..." | Re-trigger Greptile

Comment thread .github/workflows/release.yml Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/release.yml:
- Around line 256-261: Update the release workflow’s plugin-signing block around
KEYFILE to install an EXIT trap immediately after defining the key-file path,
ensuring the temporary private key is removed on both successful completion and
any signing failure; preserve the existing signing loop and cleanup behavior.

In `@src/__tests__/shared/plugins/rpc-and-signing.test.ts`:
- Around line 115-132: Strengthen the resolveTrustedKeys tests to assert exact
anchor precedence and de-duplication: verify the resolved prefix matches the
trimmed, non-blank, de-duplicated MAESTRO_PUBLISHER_KEYS in order before user
keys, and update the empty-list expectation to use the same de-duplicated anchor
sequence. Keep the existing user-key trimming and de-duplication assertions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5867bcfb-c421-4d4a-a0d9-c4158993c0cb

📥 Commits

Reviewing files that changed from the base of the PR and between 81e145f and 4074854.

📒 Files selected for processing (4)
  • .github/workflows/release.yml
  • .gitignore
  • CLAUDE-PLUGINS.md
  • src/__tests__/shared/plugins/rpc-and-signing.test.ts

Comment thread .github/workflows/release.yml Outdated
Comment thread src/__tests__/shared/plugins/rpc-and-signing.test.ts Outdated
@pedramamini

Copy link
Copy Markdown
Collaborator

Thanks for this, @chr1syy - genuinely nice work. You correctly identified that the trust gate in seedBundledPlugins() is what keeps Agent Flow's main.js from ever running, you reused the existing maestro-cli plugin sign rather than inventing a parallel signing path, and you placed the step correctly (after Build application produces dist/cli/maestro-cli.js, before any electron-builder step picks up extraResources). I verified the mechanics that matter and they hold up:

  • tsc -p tsconfig.main.json includes src/shared/**/* with module: CommonJS and outDir: dist, so require('./dist/shared/plugins/publisher-keys.js') does resolve.
  • plugin validate --json emits compact JSON, so the grep -q '"status":"trusted"' match works (it would silently break against pretty-printed output, so this is worth keeping in mind).
  • --trusted-key does accept the comma-separated list you pass, and examples/plugins/ has exactly one dir, which has a plugin.json, so the loop is safe.

CI is green and there are no merge conflicts. There are a few things I'd like addressed before we merge, one of them blocking.

1. Blocking: this breaks every release the moment it lands on rc

I confirmed MAESTRO_PLUGIN_SIGNING_KEY is not in the repo's Actions secrets, and MAESTRO_PUBLISHER_KEYS is []. So on the next release tag the new step hard-fails at two separate points, before electron-builder ever runs:

  1. .github/workflows/release.yml:256 - the unset secret renders as an empty string, printf writes an empty keyfile, plugin sign fails to load a private key, and set -euo pipefail kills the step.
  2. .github/workflows/release.yml:269 - even with the secret set, the empty ANCHOR trips the explicit exit 1.

I understand the fail-closed intent, and I agree with it as a principle. My concern is that it's guarding against something that is already safe. The runtime already fails closed, correctly and gracefully: seedBundledPlugins() only installs a plugin whose signature verifies trusted, and publisher-keys.ts documents that an empty anchor therefore means bundled plugins are simply not auto-installed. Nothing broken or non-runnable reaches the user today.

So as written, this trades a graceful runtime degradation for a total release outage, which is a net loss in release resilience rather than a gain in safety. Two ways to resolve it, your pick:

  • Preferred: make the step a loud no-op when the secret and the anchor are both absent (the pre-key-mint state), and hard-fail only on genuine drift, meaning exactly one of the two is present. That's the case you actually want to catch, and it keeps releases green until the keypair exists. Roughly:
    if [ -z "${MAESTRO_PLUGIN_SIGNING_KEY:-}" ] && [ -z "$ANCHOR" ]; then
      echo "::warning::No publisher key or anchor; shipping bundled plugins unsigned (they will not be seeded)."
      exit 0
    fi
    (You'll need to read ANCHOR before the signing loop for this ordering.)
  • Alternative: hold this PR and land it in the same change as the key mint plus the baked public key, so rc is never in a state where a release cannot be cut.

2. Clean up the private key on failure

CodeRabbit's note here is valid. If plugin sign fails, set -e exits before the rm -f on line 261 and the key file survives for the rest of the runner's lifetime. Low severity on ephemeral runners, but it's a one-line fix - add trap 'rm -f "$KEYFILE"' EXIT immediately after you define KEYFILE, and you can drop the explicit rm.

3. The new resolveTrustedKeys tests duplicate an existing test file

This is the one neither bot caught. src/__tests__/shared/plugins/publisher-keys.test.ts already exists (added in #1266, present at your merge base) and already has a describe('resolveTrustedKeys') block covering anchor merging, trimming, de-duplication, and the empty-user-list case - with stronger assertions than the new ones, since it uses exact toEqual including ordering where the new block uses toContain. Two of your three new tests are redundant against it.

This also supersedes CodeRabbit's suggestion to strengthen the new assertions: rather than hardening a duplicate, please drop the two redundant tests and move the genuinely new one - the base64 SPKI validity check, which is a good addition and has no existing equivalent - into publisher-keys.test.ts next to its siblings. Our CLAUDE.md is pretty emphatic about not growing duplicate coverage, and a signing-focused assertion is a more natural fit in the publisher-keys file than in rpc-and-signing.test.ts.

4. Docs claim contradicts the seeder's actual behavior

In the new CLAUDE-PLUGINS.md section: "seedBundledPlugins() finds the plugin unsigned and skips it - the panel renders but main.js never runs." Those two clauses contradict each other, and the second one isn't right. The trust gate continues before the copy, so the plugin is never installed at all in a dev build, which means there is no panel to render. (The PR description's Finding A1 framing has the same issue.)

The "panel renders, waits forever" symptom is real, but it belongs to a different path - a plugin manually installed unsigned into pluginsDir() and then enabled. Worth splitting those two cases apart, since this paragraph is written as dev-facing guidance and the distinction is exactly what a dev debugging it needs.


Items 2 through 4 are quick. Item 1 is the real decision, and it's mostly a sequencing question, so let me know which route you'd prefer and I'm happy to re-review. Thanks again for digging into the trust chain here.

@jSydorowicz21

Copy link
Copy Markdown
Contributor

Picked up the remaining work here. Since @pedramamini's review we now have a real release keypair, so I took his alternative path for the blocking item and landed the key instead of the no-op guard.

What's now true outside this PR:

  • MAESTRO_PLUGIN_SIGNING_KEY is set in this repo's Actions secrets — an ed25519 key minted 2026-07-23, used only for release signing.
  • Its public half is baked into MAESTRO_PUBLISHER_KEYS in the follow-up commit, so the drift guard has a real anchor to check against and the empty-anchor release outage is gone.

The follow-up commit (on branch pr-1298-followup in this repo, parented on this PR's head — one fast-forward brings it in) addresses the review:

  1. Bakes the release public key into publisher-keys.ts (item 1, resolved by key mint).
  2. trap 'rm -f "$KEYFILE"' EXIT right after the keyfile is defined, explicit rm dropped (item 2).
  3. Drops the two resolveTrustedKeys tests that duplicate publisher-keys.test.ts and moves the base64 SPKI validity check there (item 3).
  4. Rewrites the dev-testing paragraph: the seed gate skips before copy, so an unsigned bundled plugin is never installed and there is no panel — the "renders, waits forever" case is the manually-installed-unsigned path (item 4).

262/262 plugin tests pass with the baked anchor, prettier clean.

I couldn't push to the fork branch from my environment (git writes hang), hence the branch. To land it:

git pull https://github.com/RunMaestro/Maestro.git pr-1298-followup
git push

or any maintainer can push that branch onto this PR directly.

One scope note: the signing step runs in release.yml, which only fires on maintainer-pushed tags — untrusted PR code never reaches the key. I added a comment on the step making that gate explicit.

- release.yml: trap 'rm -f "$KEYFILE"' EXIT so the private key is removed
  even when a signing failure trips set -e before the explicit rm (item 2).
- Drop the two resolveTrustedKeys tests that duplicate publisher-keys.test.ts
  and move the base64 SPKI validity check there next to its siblings (item 3).
- CLAUDE-PLUGINS.md: fix the contradictory dev-testing paragraph - the seed
  gate continues before copy, so an unsigned bundled plugin is never installed
  and there is no panel; the "renders, waits forever" case is the
  manually-installed-unsigned path (item 4).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@chr1syy

chr1syy commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @pedramamini, @jSydorowicz21 - review addressed. Summary of what landed in 3dee5617e:

Item 2 (private key cleanup on failure) - added trap 'rm -f "$KEYFILE"' EXIT immediately after KEYFILE is defined and dropped the explicit trailing rm. The key is now removed on a signing failure too.

Item 3 (duplicate resolveTrustedKeys tests) - you're right, both the anchor-merge and empty-list tests duplicated the existing block in publisher-keys.test.ts (from #1266) with weaker toContain assertions. Dropped both and moved the genuinely-new base64 SPKI validity check into publisher-keys.test.ts next to its siblings. This also supersedes CodeRabbit's suggestion to harden the now-deleted assertions.

Item 4 (docs contradiction) - rewrote the dev-testing paragraph. The seed gate continues before the copy, so an unsigned bundled plugin is never installed in a dev build and there is no panel. The "panel renders but main.js never runs" symptom is now called out separately as the manually-installed-unsigned path.

Item 1 (blocking - empty anchor) - this is the maintainer-only follow-up (key mint + Actions secret + baking the public key into publisher-keys.ts), which is out of scope for me to perform here. Per @jSydorowicz21's note the keypair is minted and the secret is set, so baking the public half lands with/before this change and keeps rc releasable. I've deliberately kept the fail-closed drift guard rather than softening it to a no-op, matching the chosen "land with the key" path.

262/262 plugin tests pass, prettier clean, CI green.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/release.yml:
- Around line 256-260: Set a restrictive umask of 077 in the release signing
step before the printf command writes MAESTRO_PLUGIN_SIGNING_KEY to KEYFILE,
ensuring the temporary private key is not broadly readable while preserving the
existing cleanup trap.

In `@src/__tests__/shared/plugins/publisher-keys.test.ts`:
- Around line 5-20: Update the MAESTRO_PUBLISHER_KEYS test to validate canonical
encodings: decode each key, load it with createPublicKey, re-encode the DER
bytes with canonical base64 and compare to the original input, then assert the
loaded key’s asymmetric key type is ed25519.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c79e21c2-78de-43da-8418-3a34d6409340

📥 Commits

Reviewing files that changed from the base of the PR and between 4074854 and 3dee561.

📒 Files selected for processing (3)
  • .github/workflows/release.yml
  • CLAUDE-PLUGINS.md
  • src/__tests__/shared/plugins/publisher-keys.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • CLAUDE-PLUGINS.md

Comment thread .github/workflows/release.yml
Comment thread src/__tests__/shared/plugins/publisher-keys.test.ts Outdated
- release.yml: read the baked ANCHOR first and make signing + the drift
  guard conditional. When no anchor is baked (or the signing secret is
  absent) - the current pre-key-mint state - skip signing and the drift
  check with a warning and let the release proceed, shipping bundled
  plugins unsigned (they simply are not seeded at runtime). Only when an
  anchor is present do we sign and hard-fail on drift. Keeps the repo
  releasable today and fail-closed once the key is baked (Greptile P1).
- release.yml: umask 077 before writing the temp private key so it is not
  group/world-readable (CodeRabbit).
- publisher-keys.test.ts: tighten the anchor test to reject non-canonical
  base64 (DER round-trip) and require asymmetricKeyType === 'ed25519', not
  merely any valid SPKI key (CodeRabbit). Still empty-anchor-safe today.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/release.yml:
- Around line 271-274: Update the release guard around ANCHOR and
MAESTRO_PLUGIN_SIGNING_KEY so the pre-key-mint case still exits successfully,
but a non-empty ANCHOR with a missing signing secret exits nonzero and fails the
release. Preserve the existing unsigned-plugin warning only for the no-ANCHOR
state.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: df4a555b-d046-4770-b0a7-2561a49d485b

📥 Commits

Reviewing files that changed from the base of the PR and between 3dee561 and 27d2c27.

📒 Files selected for processing (2)
  • .github/workflows/release.yml
  • src/__tests__/shared/plugins/publisher-keys.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/tests/shared/plugins/publisher-keys.test.ts

Comment thread .github/workflows/release.yml Outdated
chr1syy added a commit to chr1syy/Maestro that referenced this pull request Aug 4, 2026
Adds a prominent header toggle to examples/plugins/agent-flow/panel.html that
renders only the lane matching the snapshot's focusedSessionId. Default OFF per
the playbook's Decision 3 sub-answer, since lazy lane seeding already removes
the idle-dot wall and an ON default would hide the fleet view.

lanesOf() now delegates to resolveLanes(), the single lane source shared by
renderGraph() and the 1s health tick. When the filter is ON but no session is
focused (or the focused session has no lane yet) it falls back to the whole
fleet and shows a small header hint rather than a blank canvas. With the toggle
OFF the all-agents view, including the focused highlight, is unchanged.

Toggle state is a panel-lifetime var, so it survives snapshot replacement with
no storage API. plugin.json is not bumped again: the 0.2.0 -> 0.3.0 bump landed
with the lazy-seeding commit.

Note: editing panel.html changes its SHA and invalidates the bundled Agent Flow
signature until the next release-time re-sign (PR RunMaestro#1298 flow). Nothing is
re-signed here.
chr1syy added a commit to chr1syy/Maestro that referenced this pull request Aug 6, 2026
seedFromSessions() no longer materializes a lane per open session, so a user
with 109 configured agents stops opening the overlay onto a wall of idle dots.
Session titles/agentIds/statuses now live in a sessionMeta side map that
getLane() reads when activity finally creates the lane, so a lane is labelled
the moment it appears.

Lane-materializing events: tool.executed, usage.updated, the terminal
agent.completed / agent.error / agent.exited / run.completed events, and
session.activated (the focused agent always needs a node for the panel's
highlight and the upcoming "current agent only" filter to land on).
session.created and session.updated are metadata only: they record into
sessionMeta and touch the lane only when one already exists.

Exception to the decision record: a busy agent.statusChanged does NOT create a
lane. That event is agentId-keyed, carries no sessionId, and fans out over
existing lanes only; synthesizing a lane there previously produced a nodeless
ghost lane that won an insertion-order lookup.

resetModel() keeps sessionMeta (the clear command forgets activity, not who the
agents are); deactivate() clears it.

Note: editing main.js and plugin.json changes their file SHAs and invalidates
the bundled Agent Flow signature until the release pipeline re-signs it
(PR RunMaestro#1298 flow). Nothing is re-signed here; that is expected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chr1syy added a commit to chr1syy/Maestro that referenced this pull request Aug 6, 2026
Adds a prominent header toggle to examples/plugins/agent-flow/panel.html that
renders only the lane matching the snapshot's focusedSessionId. Default OFF per
the playbook's Decision 3 sub-answer, since lazy lane seeding already removes
the idle-dot wall and an ON default would hide the fleet view.

lanesOf() now delegates to resolveLanes(), the single lane source shared by
renderGraph() and the 1s health tick. When the filter is ON but no session is
focused (or the focused session has no lane yet) it falls back to the whole
fleet and shows a small header hint rather than a blank canvas. With the toggle
OFF the all-agents view, including the focused highlight, is unchanged.

Toggle state is a panel-lifetime var, so it survives snapshot replacement with
no storage API. plugin.json is not bumped again: the 0.2.0 -> 0.3.0 bump landed
with the lazy-seeding commit.

Note: editing panel.html changes its SHA and invalidates the bundled Agent Flow
signature until the next release-time re-sign (PR RunMaestro#1298 flow). Nothing is
re-signed here.
@chr1syy

chr1syy commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Publisher key baked - A1 is now complete

Pushed 714b30648, which adds the missing piece to this branch's plumbing: the trust anchor itself. Diff vs rc is now 5 files (the original 4 plus src/shared/plugins/publisher-keys.ts).

What changed (only the two files the plumbing was waiting on):

  • src/shared/plugins/publisher-keys.ts - MAESTRO_PUBLISHER_KEYS now holds the real Ed25519 publisher key (base64 SPKI) instead of the empty array, with a comment naming the key's purpose, mint date, and the rotation pointer to CLAUDE-PLUGINS.md "Trust model (v1)". The header comment's "Until a real publisher key is added here this list is EMPTY" clause is rewritten to past tense so it no longer contradicts the code.
  • src/__tests__/shared/plugins/publisher-keys.test.ts - the shape loop was vacuous while the anchor was empty. It now asserts MAESTRO_PUBLISHER_KEYS.length > 0 ahead of the loop, so the canonical-base64 / SPKI-parse / ed25519 assertions actually execute against the baked key, and an accidental future emptying of the anchor fails the suite instead of silently reverting to pre-A1 behavior. The key value is deliberately not hard-coded in the test.

The key was sanity-checked before baking: exact canonical base64 round trip, parses via createPublicKey({ format: 'der', type: 'spki' }), asymmetricKeyType === 'ed25519'.

Drift guard dry run (scratch key, no production private key involved). The positive production path can only run in release CI by design, so the release step's exact command lines were exercised locally with a throwaway keypair:

  1. Anchor extraction as release.yml runs it (require('./dist/shared/plugins/publisher-keys.js').MAESTRO_PUBLISHER_KEYS.join(',')) returns non-empty and matches the baked key, so CI now takes the signing path rather than the documented empty-anchor skip branch.
  2. maestro-cli plugin sign examples/plugins/agent-flow --gen-key with the scratch key, then plugin validate --trusted-key <scratch-pub> --json reports "status":"trusted" on the real plugin dir (4 files). Sign/validate round trip works end to end.
  3. The same validate against the production anchor reports "valid":true but "status":"untrusted" - exactly the drifted-secret scenario. release.yml's grep -q '"status":"trusted"' guard returns non-zero there, so the release hard-fails. Fail-closed confirmed, nothing was weakened to reach that result.
  4. Hygiene: signature.json stayed gitignored and was deleted along with the scratch temp dir; no *.pem exists anywhere under the worktree.

Checks: scoped suite 4 passed / 0 failed; all three tsc configs clean; ESLint and Prettier clean on both touched files.

Still needs a human before the first tagged release: confirm the MAESTRO_PLUGIN_SIGNING_KEY Actions secret exists on RunMaestro/Maestro (a secret on the fork does nothing) and that the private PEM is stored offline with no copy left in a repo tree, home dir, or shell history. After the first tagged release, verify the "Sign bundled plugins" step took the signing path on every matrix leg and that the artifacts contain <resources>/plugins/agent-flow/signature.json.

@chr1syy

chr1syy commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

@pedramamini this one needs a repo secret only an admin can add. Everything you need:

Name: MAESTRO_PLUGIN_SIGNING_KEY (exact, case-sensitive)
Where: Settings -> Secrets and variables -> Actions -> New repository secret (not an environment secret - the workflow reads secrets.MAESTRO_PLUGIN_SIGNING_KEY with no environment set)
Value: the ed25519 private key in PKCS8 PEM, including the BEGIN/END lines. @chr1syy holds it - it was minted locally and deliberately never passed through an agent session; only the public half was ever shared.

The one thing that has to line up

The private key must be the counterpart of the public key already committed in src/shared/plugins/publisher-keys.ts:

MCowBQYDK2VwAyEANZcDO/IuvEyV/Xe8JbLjkA4SDCl2ZRYe50Yr3AHuT8U=

If they drift the release fails loudly rather than shipping something broken - the drift guard re-validates the fresh signature against the baked anchor and exits 1. That is deliberate: a silent mismatch would ship a plugin that can never be seeded at runtime.

There is a local verify command in the handoff note that derives the public key from the private one and compares it to the baked anchor, so the match can be confirmed before the key is handed over. It prints only the public key, so its output is safe to share.

This PR does not need the secret to merge

The signing step degrades rather than breaks. With no secret it warns and exits 0, and unsigned bundled plugins are simply not seeded - seedBundledPlugins() only installs a plugin that resolves trusted. So the failure mode is "Agent Flow does not auto-install", never "an untrusted plugin auto-installed". Merge whenever; Agent Flow just stays inert in packaged builds until the secret exists.

Full detail, including how to transfer the key safely and what rotation involves, is in .maestro/Working/round2/A1-secret-handoff-for-pedram.md on this branch's worktree (gitignored, so happy to paste it here or send it over if easier).

The anchor was set to a key minted locally on 2026-08-07, but
MAESTRO_PLUGIN_SIGNING_KEY was already configured on 2026-07-25 (confirmed via
the Actions secrets listing: created 2026-07-25, never updated) holding the
ed25519 key minted 2026-07-23 that @jSydorowicz21 landed in the pr-1298-followup
branch.

Those are different keys, so as it stood this branch would have made the drift
guard fail EVERY release: CI signs with the secret's private half, producing a
signature whose publicKey is `...gG9il...`, which does not match the baked
`...NZcDO...`. The guard is doing exactly its job - the bug was the anchor.

Baking the secret's public counterpart instead of rotating the secret keeps the
release key where it belongs (only in CI, held by no individual) and needs no
admin action. The locally minted key is discarded; it never signed anything
shipped.

Comment expanded to name the secret it pairs with and to state that rotation is
a two-part change.

Tests: 623 plugin tests pass. The key is verified to parse as an ed25519 SPKI
and round-trip to the identical base64. Three tsc configs and prettier clean.
@chr1syy

chr1syy commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Correction to my previous comment - the secret is already set, and this branch had the wrong key baked.

I checked the Actions secrets listing rather than continuing to assume:

MAESTRO_PLUGIN_SIGNING_KEY  created=2026-07-25  updated=2026-07-25

That matches @jSydorowicz21's comment on this PR to the day. So no admin action is needed and my earlier request to @pedramamini was wrong - apologies for the noise.

The real problem it uncovered

The secret holds the key minted 2026-07-23, whose public half @jSydorowicz21 baked on pr-1298-followup:

MCowBQYDK2VwAyEAgG9ilXDpkj83vdxhlOI64cehRMB2EpbW2CNQO3izPu0=

This branch was baking a different key, minted locally on 2026-08-07:

MCowBQYDK2VwAyEANZcDO/IuvEyV/Xe8JbLjkA4SDCl2ZRYe50Yr3AHuT8U=

As it stood, merging this would have failed every release at the drift guard: CI signs with the secret's private half, and the resulting signature's publicKey would not match the baked anchor. The guard was doing exactly its job - the anchor was the bug. Worth noting it would have failed loudly at release time, not shipped a broken plugin, which is the behaviour this PR was designed for.

Fix - 4cfc4df9d

Baked the secret's public counterpart instead of rotating the secret. That keeps the release key where it belongs (only in CI, held by no individual) and needs nothing from an admin. The locally minted key is discarded; it never signed anything shipped.

Verified the new anchor parses as an ed25519 SPKI and round-trips to the identical base64, so it is a real key and not just well-formed base64. 623 plugin tests pass, three tsc configs and prettier clean.

The rest of pr-1298-followup is already superseded on this branch - the trap, the duplicate-test removal and the docs rewrite landed in 3dee5617e, and the release step here is a superset (it also adds umask 077 and the graceful pre-key-mint skip instead of a hard failure). The key value was the only thing genuinely missing.

@jSydorowicz21 thank you - your comment is what surfaced this.

@chr1syy

chr1syy commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

@greptileai review please
@coderabbitai review please

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

@chr1syy: I will review the changes.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Comment thread .github/workflows/release.yml Outdated
…gone

Review of PR RunMaestro#1298 (Greptile). The pre-key-mint escape hatch treated a missing
anchor and a missing secret as the same degraded state, which was right while
MAESTRO_PUBLISHER_KEYS was empty. Now that a real key is baked, that pairing
lets a removed or renamed MAESTRO_PLUGIN_SIGNING_KEY exit 0 and ship unsigned
plugins: the anchor promises a trust guarantee the artifact cannot honour, and
Agent Flow silently stops being seeded in a shipped build.

Split into the two cases they actually are:

- No anchor (a fork, or this repo pre-mint): still skips with a warning. There
  is nothing to validate against, and breaking every fork's release would be
  worse than shipping plugins that are simply never seeded.
- Anchor baked, secret absent: now fails with an actionable error. That state is
  only reachable by removing the secret after baking a key, which is a
  misconfigured release, not a degraded one.

Same reasoning as the drift guard directly below it: a key/secret mismatch
should fail at release time rather than in users' hands.
@pedramamini

Copy link
Copy Markdown
Collaborator

Thanks @chr1syy, and thanks @jSydorowicz21 for surfacing the key mismatch. Coming back to this after my July review: items 2, 3 and 4 are all properly addressed, and item 1 (the blocking one) is resolved the way we agreed, by landing the real anchor rather than softening the guard. I re-verified the pieces that matter on f2c6b86:

  • The MAESTRO_PLUGIN_SIGNING_KEY repository secret does exist (created 2026-07-25T14:59:45Z), so your correction at 17:45 is right and no admin action is outstanding.
  • The baked anchor is a real key, not just well-formed base64: it round-trips to identical canonical base64, parses via createPublicKey({ format: 'der', type: 'spki' }), and reports asymmetricKeyType === 'ed25519'. It is also byte-identical to what landed on pr-1298-followup, so the two branches agree.
  • Step ordering holds: Build application (line 235) produces dist/, Sign bundled plugins (line 250) runs next, and the first electron-builder step is at line 339.
  • All three per-platform extraResources blocks (mac, win, linux) copy examples/plugins/agent-flow as a whole directory, so the freshly written signature.json reaches <resources>/plugins/agent-flow/ on every target, not just one.
  • pluginValidate emits compact JSON.stringify output, so the grep -q '"status":"trusted"' guard matches, and the only fields that could collide are id/name/version/tier.
  • Your item 4 docs rewrite matches the code. seedBundledPlugins() does continue on the trust gate before fs.cpSync, so an unsigned bundled plugin is never installed and there is genuinely no panel. The manually-installed-unsigned split is the right distinction.

CI is green on both matrix legs and there are no merge conflicts. Two small things I would like fixed before this goes in, then I am happy.

1. The empty-anchor branch's comment is now wrong, and it changes fork behavior

# No anchor baked: a fork, or this repo before the key was minted.
if [ -z "$ANCHOR" ]; then

ANCHOR is read out of dist/shared/plugins/publisher-keys.js, which is built from committed source. Now that the key is baked, every checkout of this tree has a non-empty anchor, including a fork. So a fork cutting its own tag no longer takes this graceful skip; it falls through to the next branch and hard-fails with an error telling the maintainer to restore a secret they never had:

MAESTRO_PUBLISHER_KEYS is baked but the MAESTRO_PLUGIN_SIGNING_KEY secret is absent... Restore the secret

This is the same class of issue as item 4 in my last review, a comment that describes behavior the code no longer has. The guard itself is still worth keeping for the deliberately-emptied-anchor case that your own error message already references, so this is just about the comment and the fork case. Either reword it to drop the fork claim and say plainly that it only fires when the anchor is deliberately emptied, or, if you would rather forks keep building, gate the hard-fail on the repository (for example github.repository == 'RunMaestro/Maestro') and warn-and-skip elsewhere. My preference is the reword, since it is one line and forks cutting Maestro releases is not a case we support today.

2. The PR description now says the opposite of what the PR does

The description still carries:

publisher-keys.ts is intentionally NOT modified here (MAESTRO_PUBLISHER_KEYS stays empty).

plus the whole "Human follow-up required" section asking a maintainer to mint the keypair, add the secret, and bake the public key. All three are done. Since this text follows the change into the merge commit and is the first thing anyone reads when they come back to this later, please bring it in line with the five-file reality.

Nits, take or leave

  • The signing loop runs over examples/plugins/*/, but pluginSign returns fail() when a directory has no plugin.json, and set -e turns that into a failed release. Only agent-flow exists today so this is fine right now, but the comment's "signing the sibling example dirs is harmless" is doing some load-bearing work. A [ -f "$dir/plugin.json" ] || continue in the loop makes the claim true permanently.
  • With pipefail set, if node dist/cli/maestro-cli.js fails to start at all, the drift guard's if ! catches it and reports a key/anchor divergence for what is really a broken build. Worth splitting the validate output into a variable first so the two failures are distinguishable in the release log.

One thing on me, not on this PR

Nothing in this PR can prove the baked public key is the counterpart of the private key sitting in the secret, since the anchor value reached this branch through a PR comment rather than being derived from the secret. If they have drifted, the guard fails all four matrix legs on the next tag. That is the designed behavior and is much better than shipping an unseedable plugin, but I would rather not discover it during a real release. I will confirm the match on my side before the first tagged release after this merges, so do not block on it.

Ping me once 1 and 2 are pushed and I will get this merged. Nice work running the drift scenario end to end with a scratch key, that write-up made this much faster to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants