• We need to put this behind locked config that default-off, because it's dramatically more dangerous than anything else in Phabricator. Particularly, it allows anyone with build plan access to take over the host, which isn't normally possible.
• Is the #{...} syntax common/conventional?
• I think the parsing needs to be smarter, so if we add a name variable and then I title a revision ; rm -rf /, it doesn't wipe the disk.
• It will also fail if any variable contains #{...}, I think.
• Maybe we just leave this out of the upstream for the moment until we're in a better position to deal with all the caveats?

I'd be tempted to go so far as to define a configuration option (editable only with "bin/config") that defines the commands that can be executed. LocalCommandBuildStepImplementation would then become a dropdown where the user selects the command they want to run.

On default installs, there'd be no commands defined, so we wouldn't need to put the actual implementation behind a config option, since it's entirely harmless without any commands.

We can change #{...} to ${...} if you like; I don't mind either way.

I'm not quite sure how we can replace variables with values in a safe way that won't affect the command structure, without requiring explicit seperations of arguments (where an argument can either be text or a variable, but not

If the replacement algorithm is like:

1. Replace all % with %%.
2. Replace all #{...} with %s.
3. Call csprintf() on the string to actually do the replacement.

...would that work? You would not be able to build compound variables by concatenating text as easily, but it should be safe in all cases where the user has built a reasonable command string. Even if I don't care about a machine being wiped, it seems bad if I can't build an object named ";" or whatever, no matter how carefully construct my command.

I don't feel strongly about the #{...} syntax, but I'd slightly prefer to use an existing one to inventing one, if it's reasonable. For example, if Buildbot or Jenkins or Bamboo use #{...} (and don't have a rich DSL around it) I'd say that's a good choice. I'm not sure what they do offhand, though.

I don't think the config thing is too bad, but I'm worried it's not a great long-term solution and that we'd eventually want to obsolete and remove it. If holding this out of the upstream for the moment is OK, I think getting the variables in but holding the "run arbitrary commands" code makes sense. We could use the variable stuff immediately for, e.g., "Request a URL", too, which would make sure it was getting some use.