Audit workflow - resolved concern in next commit


as stated in, lets consider this scenario:

  • Bailey notices a few minor problems with Alice’s commit. She leaves comments describing improvements and uses “Raise Concern” to send the commit back into Alice’s queue.
  • Later, Alice logs into Phabricator and sees that Bailey has raised a concern (usually, Alice will also get an email). She resolves the issue somehow, maybe by making a followup commit with fixes.
  • After the issues have been dealt with, she uses “Request Verification” to return the change to Bailey so Bailey can verify that the concerns have been addressed.

In other words, this is typical situation: developer makes commit -> someone spots problem and inform developer -> developer makes fix -> problem solved.

We have commit A and corresponding audit with raised concern. Developer addressed that concern and made next commit B. The fix was OK, however this commit is not linked to original audit in any way. When we mention the commit B in the audit comment, it is indeed linked, but only as a reference. We expected that the commit B will be somehow joined to the audit, or overtakes it, or something like that, so the audit can be resolved by accepting commit B.

Clearly, we miss some information how to “link” commits to audit or we do not understand the recommended audit workflow - specifically what is meant by “followup commit”.

Can anybody give some hints or explanation?

There is currently no way to make commit B automatically close an audit raised in commit A.

One concern I have with this workflow is that it can leave commits in an ambiguous state when audit feedback isn’t trivial.

If audit feedback is simple (“Missed one typo in the docs here.”), this workflow makes sense and would likely work well.

However, I suspect this workflow might not work as well when feedback is more complex. For example, if Bailey (the auditor) raises several different concerns/questions on commit A and Alice (the author) later marks commit B as “resolving” the issues without explanation, who should act next?

  • Did Alice overlook the other questions in the audit? Should Bailey remind her?
  • Or, is Alice going to follow up on commit A and explain things in more detail? Should Bailey just wait?

With the current workflow, Alice must explicitly return to commit A and leave a comment (“I fixed this in commit B, …”). This makes it clear who should act next: either she also addresses the other audit feedback (and this workflow reminds her about other questions) or she overlooks it, which gives Bailey a pretty good idea that she should act next (“That fix looks good to me, thanks! What about the question I had about performance?”).

Another slightly different case is when Bailey raises several trivial pieces of feedback (on commit W) and Alice fixes them across multiple different commits (commits X, Y, Z). This (small, focused commits) is a pattern which I believe is good and something we want to encourage, but means the “X fixes W” syntax either needs to be more complex (and thus harder to learn/understand) or that W will be left in an ambiguous state between when X is published and when Z is published.

These ambiguities wouldn’t necessarily be a problem in practice. I’m sure disciplined engineers could make this workflow successful (for example, often Bailey will review the change that Alice makes in Differential and they might discuss other issues there – but not all installs use review). But I believe disciplined engineers can already make the “mention” workflow successful, too.

Some version of this is a feature we might implement in the future (although I believe there are no outstanding customer requests for it, today). But it’s a hard sell because I think it tends to encourage two bad patterns (ambiguous “next action” audit states, commits doing multiple things) over two good patterns (discussion between authors and auditors, commits doing just one thing).

A version of this that I’d be comfortable implementing today might be just be like a stronger version of a mention, without any automatic state changes. The UI could show these commits more prominently, the list of audits waiting on you could show which audits have followup commits, etc. This might make things easier without creating any peril around states or commit arrangement.

Many thanks for thorough explanation. Now we are in pretty good picture about the workflow and I agree with points you presented. There is no need for auto state changes. I would personally welcome “stronger version of a mention” as you suggested, maybe somehow link commits together based on referring pending audit (“starting” commit) by mentioning, or something like that. In practice you need to see what raised an audit issue and which of following commits resolves which point of the issue, so any kind of list showing this will be sufficient.