fix(server): accept namespaced workflow names on HTTP launch and read routes#2047
Open
truffle-dev wants to merge 4 commits into
Open
fix(server): accept namespaced workflow names on HTTP launch and read routes#2047truffle-dev wants to merge 4 commits into
truffle-dev wants to merge 4 commits into
Conversation
… routes Workflows can be namespaced one subfolder deep on disk (e.g. triage/foo), discovered at MAX_DISCOVERY_DEPTH = 1, and the CLI launches them by that name. The HTTP /run and GET routes validated the name with isValidCommandName, which rejects any '/', so namespaced workflows could only be run from the CLI (coleam00#2007). Add isValidWorkflowName: allows a single '/' by requiring every slash-separated segment to be a valid command name, so '..', '\', leading dots, and empty segments (leading/trailing/double slash) stay rejected and path traversal is unchanged. Wire it into the non-mutating /run and GET routes only; PUT and DELETE keep isValidCommandName since creating a subfolder on save is separate.
📝 WalkthroughWalkthroughAdds Namespaced workflow name validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/server/src/routes/api.workflows.test.ts (1)
333-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
/runnamespaced-route regression test.This change updates both
GET /api/workflows/:nameandPOST /api/workflows/:name/run, but the new integration coverage only locks down the GET path. The issue being fixed is launchability over HTTP, so a percent-encoded namespaced name should be exercised on the run endpoint too.🤖 Prompt for 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. In `@packages/server/src/routes/api.workflows.test.ts` around lines 333 - 360, Add a regression test for the namespaced run path in api.workflows.test.ts so the percent-encoded workflow name is covered for POST /api/workflows/:name/run as well as GET. Reuse the existing triage/review setup in the same test area, then call the run endpoint with triage%2Freview and assert the request succeeds and targets the expected workflow. Keep the coverage aligned with registerApiRoutes and the workflow lookup behavior used by the existing namespaced GET test.packages/workflows/src/command-validation.ts (1)
33-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the namespace-depth rule with workflow discovery.
segments.length > 2duplicates the one-subfolder contract frompackages/workflows/src/workflow-discovery.ts(MAX_DISCOVERY_DEPTH = 1). If that depth ever changes, the API validator can drift and start rejecting discoverable workflows or accepting names the loader never finds.🤖 Prompt for 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. In `@packages/workflows/src/command-validation.ts` around lines 33 - 35, The namespace-depth check in the command validator is duplicating the workflow discovery contract and can drift from the loader. Update the validation logic in command-validation.ts to reuse the same depth rule/source of truth as workflow-discovery.ts, ideally by sharing MAX_DISCOVERY_DEPTH or a common helper, so name validation stays aligned with workflow discovery behavior.
🤖 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.
Nitpick comments:
In `@packages/server/src/routes/api.workflows.test.ts`:
- Around line 333-360: Add a regression test for the namespaced run path in
api.workflows.test.ts so the percent-encoded workflow name is covered for POST
/api/workflows/:name/run as well as GET. Reuse the existing triage/review setup
in the same test area, then call the run endpoint with triage%2Freview and
assert the request succeeds and targets the expected workflow. Keep the coverage
aligned with registerApiRoutes and the workflow lookup behavior used by the
existing namespaced GET test.
In `@packages/workflows/src/command-validation.ts`:
- Around line 33-35: The namespace-depth check in the command validator is
duplicating the workflow discovery contract and can drift from the loader.
Update the validation logic in command-validation.ts to reuse the same depth
rule/source of truth as workflow-discovery.ts, ideally by sharing
MAX_DISCOVERY_DEPTH or a common helper, so name validation stays aligned with
workflow discovery behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4f6fa13-44c2-420c-b9ad-c7b50922c542
📒 Files selected for processing (5)
packages/server/src/routes/api.tspackages/server/src/routes/api.workflows.test.tspackages/server/src/test/workflow-mock-factories.tspackages/workflows/src/command-validation.test.tspackages/workflows/src/command-validation.ts
The POST /api/workflows/:name/run route now validates with isValidWorkflowName, but this test still mocked isValidCommandName to reject, so validation passed and the route returned 200 instead of 400. Mock the validator the route actually calls.
…loader The workflow-name validator hard-coded a two-segment cap to mirror the loader's discovery depth, duplicating the one-subfolder contract. If the loader's MAX_DISCOVERY_DEPTH ever changed, the validator would silently drift and start rejecting discoverable names or accepting names the loader never finds. Move MAX_DISCOVERY_DEPTH into command-validation (the dependency-free leaf module) as the single source of truth, derive the segment cap from it, and import it back into workflow-discovery. The import direction keeps the leaf import-free, so the executor/dag-executor cycle this module breaks stays broken.
The namespaced-name fix updated both GET /api/workflows/:name and POST /api/workflows/:name/run, but the new coverage only locked down the GET path. Since launchability over HTTP is the point of the fix, exercise a percent-encoded namespaced name on the run endpoint too: assert it is accepted and that the decoded `triage/review` reaches the orchestrator as `/workflow run triage/review ...`. The test installs the real validator logic so it goes red if the run route validates with isValidCommandName instead of isValidWorkflowName.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/server/src/routes/api.workflow-runs.test.ts (1)
381-389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit return types to the new test callbacks.
Line 381, Line 384, and Line 393 introduce new functions without explicit return annotations, which breaks the repo’s strict TypeScript rule for complete function typings.
Suggested diff
- const segmentOk = (seg: string) => + const segmentOk = (seg: string): boolean => !!seg && !seg.startsWith('.') && !seg.includes('\\') && !seg.includes('..'); @@ - (isValidWorkflowName as ReturnType<typeof mock>).mockImplementationOnce((name: string) => { + (isValidWorkflowName as ReturnType<typeof mock>).mockImplementationOnce((name: string): boolean => { if (!name) return false; const segments = name.split('/'); if (segments.length > 2) return false; return segments.every(segmentOk); }); @@ - (name: string) => segmentOk(name) && !name.includes('/') + (name: string): boolean => segmentOk(name) && !name.includes('/') );As per coding guidelines,
**/*.{ts,tsx}requires that “all functions must have complete type annotations; noanytypes without explicit justification”.Also applies to: 392-393
🤖 Prompt for 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. In `@packages/server/src/routes/api.workflow-runs.test.ts` around lines 381 - 389, The new test callbacks in api.workflow-runs.test.ts are missing explicit return type annotations, which violates the repo’s strict TypeScript typing rule. Update the local helpers and mock implementations around segmentOk and isValidWorkflowName so every newly introduced function has a complete signature, including explicit return types for the arrow functions and mockImplementationOnce callback.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/server/src/routes/api.workflow-runs.test.ts`:
- Around line 381-389: The new test callbacks in api.workflow-runs.test.ts are
missing explicit return type annotations, which violates the repo’s strict
TypeScript typing rule. Update the local helpers and mock implementations around
segmentOk and isValidWorkflowName so every newly introduced function has a
complete signature, including explicit return types for the arrow functions and
mockImplementationOnce callback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62946b2d-7874-4d86-a517-d84af4c7bc5a
📒 Files selected for processing (3)
packages/server/src/routes/api.workflow-runs.test.tspackages/workflows/src/command-validation.tspackages/workflows/src/workflow-discovery.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/workflows/src/command-validation.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
triage/foo), are discovered atMAX_DISCOVERY_DEPTH = 1, and the CLI launches them by that name — but the HTTPPOST /api/workflows/{name}/runandGET /api/workflows/{name}routes validate the name withisValidCommandName, which rejects any/. So namespaced workflows can only be run from the CLI, never over the API (HTTP API cannot launch namespaced workflows: POST /api/workflows/{name}/run rejects names containing '/' #2007).~/.archon/workflows/triage/foo.yaml) is silently unreachable from the API surface, with two failure modes: an unencoded slash 404s (Hono:namematches one segment), and a percent-encoded%2Fdecodes and hits the validator, returning400 Invalid workflow name.isValidWorkflowNameto@archon/workflows/command-validation. It allows a single/by splitting on it and requiring every segment to itself be a valid command name — so..,\, leading dots, and empty segments (leading / trailing / double slash) all stay rejected and the path-traversal protection is unchanged. Wired it into the/runandGETroutes.PUTandDELETE /api/workflows/{name}keepisValidCommandName. Those write/remove a file, and creating (or cleaning up) a subfolder on save is a separate concern from resolving an existing one.isValidCommandNameis untouched, so DAG step-command validation is unaffected.Label Snapshot
risk: lowsize: Sserverserver:api-workflowsChange Metadata
bugserverLinked Issue
Validation Evidence
Guards proven red, not just green:
isValidWorkflowNameto delegate toisValidCommandName(reject all/) turns the two namespace-accepting unit tests red.GETroute back toisValidCommandNameturns the newtriage%2Freviewintegration test (writes.archon/workflows/triage/review.yaml, expects200 source:project) red.Security Impact
isValidWorkflowNameis strictly narrower than "allow any name": it permits exactly one extra character class (a single interior/) and still rejects..,\, leading dots, and empty segments, so no new path-traversal surface is opened. The resolved paths are the same ones the CLI already reads.Compatibility / Migration
Side Effects / Blast Radius
/runandGETworkflow-name routes only. A name that resolves to a namespaced workflow now reaches discovery/readFileinstead of 400ing; everything else behaves identically.Rollback Plan
isValidCommandNameand namespaced workflows are CLI-only again. No flags/config.Risks and Mitigations
/that does not correspond to an on-disk namespaced workflow./runroute resolves throughresolveWorkflowName, which already handles misses.Summary by CodeRabbit
New Features
triage/review).Bug Fixes
400“Invalid workflow name” response.Tests