fix(persona-kit): preserve trigger paths#276
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughTrigger definitions now support optional path scopes. Parsing and schema validation enforce non-empty absolute paths, while runtime and deployment tests verify those scopes remain available through agent definition and persistence projection. ChangesTrigger path scoping
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PersonaDefinition
participant parseIntegrationTrigger
participant CompiledAgent
participant PersistenceProjection
PersonaDefinition->>parseIntegrationTrigger: provide trigger paths
parseIntegrationTrigger->>CompiledAgent: return validated paths
CompiledAgent->>PersistenceProjection: pass compiled triggers
PersistenceProjection-->>CompiledAgent: retain trigger paths
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 10 files
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a paths property to PersonaIntegrationTrigger to enable scoping wake routing for agents. This change includes updates to the JSON schema, TypeScript interfaces, parser validation, and corresponding tests. A review comment correctly identifies an inconsistency in the path validation logic regarding whitespace handling and provides a robust implementation suggestion that should be addressed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (typeof path !== 'string' || !path.trim()) { | ||
| throw new Error(`${pathContext} must be a non-empty string`); | ||
| } | ||
| if (!path.startsWith('/')) { | ||
| throw new Error(`${pathContext} must start with /`); | ||
| } | ||
| return path; |
There was a problem hiding this comment.
The current validation logic for paths has an inconsistency. It uses path.trim() to check for emptiness but then uses the original path for the startsWith('/') check and for the return value. This can lead to unexpected behavior with whitespace. For example, a path like " /foo" would be incorrectly rejected, and a path like "/foo " would be accepted and stored with trailing whitespace.
It would be more robust to trim the path once and use the trimmed value for all subsequent validation and for the return value. This is also more consistent with how other values are parsed in this file.
if (typeof path !== 'string') {
throw new Error(`${pathContext} must be a string`);
}
const trimmed = path.trim();
if (!trimmed) {
throw new Error(`${pathContext} must be a non-empty string`);
}
if (!trimmed.startsWith('/')) {
throw new Error(`${pathContext} must start with /`);
}
return trimmed;There was a problem hiding this comment.
Fixed in da2b05a. The shared path parser now rejects leading or trailing whitespace instead of silently normalizing an exact routing glob, and the generated schema enforces the same contract. Trigger and watch regressions cover padded paths; persona-kit 290/290 and deploy 226/226 pass.
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/persona-kit/src/parse.ts">
<violation number="1" location="packages/persona-kit/src/parse.ts:556">
P2: Agent schema validation still accepts padded `watch[].paths`, but `parseWatch` now rejects them through this helper. Apply the same non-empty, absolute, no-trailing-whitespace constraints to `WatchRule.paths` in the schema emitter so authoring validation matches parsing.</violation>
</file>
<file name="packages/persona-kit/schemas/agent.schema.json">
<violation number="1" location="packages/persona-kit/schemas/agent.schema.json:57">
P2: Schema consumers can accept trigger paths that `parseIntegrationTrigger` then rejects: `'/ github/**'` matches this pattern because only trailing whitespace is excluded. Constrain the first character after `/` as well, and update `scripts/emit-schema.mjs` with the same expression so regeneration preserves the contract.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| if (typeof path !== 'string' || !path.trim()) { | ||
| throw new Error(`${pathContext} must be a non-empty string`); | ||
| } | ||
| if (path !== path.trim()) { |
There was a problem hiding this comment.
P2: Agent schema validation still accepts padded watch[].paths, but parseWatch now rejects them through this helper. Apply the same non-empty, absolute, no-trailing-whitespace constraints to WatchRule.paths in the schema emitter so authoring validation matches parsing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/persona-kit/src/parse.ts, line 556:
<comment>Agent schema validation still accepts padded `watch[].paths`, but `parseWatch` now rejects them through this helper. Apply the same non-empty, absolute, no-trailing-whitespace constraints to `WatchRule.paths` in the schema emitter so authoring validation matches parsing.</comment>
<file context>
@@ -553,6 +553,9 @@ function parseAbsolutePathList(value: unknown, context: string): string[] {
if (typeof path !== 'string' || !path.trim()) {
throw new Error(`${pathContext} must be a non-empty string`);
}
+ if (path !== path.trim()) {
+ throw new Error(`${pathContext} must not have leading or trailing whitespace`);
+ }
</file context>
| "items": { | ||
| "type": "string", | ||
| "minLength": 1, | ||
| "pattern": "^/(?:.*\\S)?$" |
There was a problem hiding this comment.
P2: Schema consumers can accept trigger paths that parseIntegrationTrigger then rejects: '/ github/**' matches this pattern because only trailing whitespace is excluded. Constrain the first character after / as well, and update scripts/emit-schema.mjs with the same expression so regeneration preserves the contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/persona-kit/schemas/agent.schema.json, line 57:
<comment>Schema consumers can accept trigger paths that `parseIntegrationTrigger` then rejects: `'/ github/**'` matches this pattern because only trailing whitespace is excluded. Constrain the first character after `/` as well, and update `scripts/emit-schema.mjs` with the same expression so regeneration preserves the contract.</comment>
<file context>
@@ -54,7 +54,7 @@
"type": "string",
"minLength": 1,
- "pattern": "^/"
+ "pattern": "^/(?:.*\\S)?$"
},
"description": "Optional Relayfile path globs used to scope wake routing before an agent is provisioned. Paths are absolute (leading `/`) and provider-specific.",
</file context>
* feat(review-kit): strip the model's preamble from the review defineReviewAgent posted `run.output.trim()` — the harness's raw stdout, which carries whatever the model said on its way to an answer: "Evidence confirmed.", "I have all evidence needed.", a draft findings list, "Writing the review now." Every kit-built reviewer publishes the search alongside the findings. This cannot be fixed in a charter, and #281's trap list did not include it because I only learned it after filing. Measured on a real reviewer against a 150-word cap: 537 words, then 256 once the ban was sharpened, then 191, then 187 after moving the entire output contract to the top of the charter where it is read first. Four rewrites of increasing severity bought 4 words. Stripping in code took the same reviewer to 153 on the next run. The reason is simple in hindsight: you cannot instruct a model not to think, only decline to publish the thinking. So the charter owns the SHAPE — which it gets right reliably, every review nailed the finding format — and the kit owns the BOUNDARY. Same reason agents/review strips its own trailing READY sentinel instead of trusting the model to omit it. reviewBody() cuts at the LAST verdict line: a model that drafts before writing emits two, and the real review is the final one. No verdict line means the model ignored the format — pass it through rather than post nothing, since for an advisory agent silence reads as approval. Exported so a lens with a bespoke handler gets the same boundary. Tests cover the three cases: preamble stripped, last-verdict-wins, and passthrough/empty. Note: `pnpm --filter @agentworkforce/review-kit typecheck` and test `traps 2-4` already fail on pristine origin/main in a local checkout — both assert trigger `paths`, which needs persona-kit >= 4.1.26 (#276). Unrelated to this change; verified by stashing it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * review-kit: tolerate how the model bolds the verdict line From @gemini-code-assist's review: /^\*\*Verdict:/ assumed one exact format. Verified the concern is real — the old pattern misses three variants models actually produce: **Verdict**: Blocker (colon outside the bold) -> MISS **Verdict** : Blocker (space before colon) -> MISS **verdict:** Blocker (casing) -> MISS **Verdict: Blocker.** (what production emits) -> match A miss is not a graceful degradation here: an unmatched line publishes the entire preamble, which is the one thing this function exists to prevent. Being lax costs nothing; being strict costs the whole feature on a formatting wobble. The charter asks for one form and the production reviewer emits it — but the point of stripping in code was precisely that the model's output is not something you can legislate. The ^ anchor still does the real work: a verdict mentioned mid-sentence in prose is not matched. Covered by a test. Tests for all five variants plus the prose case, per the review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Ricky Schema Cascade <ricky@agent-relay.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Closes #275.
Summary
pathsthroughparseIntegrationTrigger, agent compilation, and persistencepaths?: string[]on both runtime and authoring trigger interfacesFailure-class regression
The deploy persistence regression was added and run before the implementation. On pristine
origin/mainplus the test-only diff:With this change, the same test is green and the persisted agent block contains both:
Validation
pnpm --filter @agentworkforce/persona-kit test— 290/290 passedpnpm --filter @agentworkforce/deploy test— 226/226 passednode --test packages/runtime/dist/define-agent.test.js— 6/6 passedgit diff --check— passedThe full runtime suite compiles, but this local host uses Node 25.8.1; 13 existing local-preview permission-boundary tests require the repository's patched Node >=26.3.1. The changed runtime authoring test is run directly and passes.
Compatibility / scope
Triggers without
pathsare unchanged. Invalid declared paths previously vanished silently and now fail with a precise field path. Unknown trigger-key rejection is intentionally not included: oncepathsis supported, that temporary mitigation is no longer needed, and rejecting extension keys would conflict with the repository's established forward-compatible parsing policy.