Improving engineering excellence in Canasta’s contributions

[jeffw16]

The recent change which occurred in CanastaWiki/Canasta-DockerCompose#63 is highly troubling. There were no tests written, there was no proof on the PR page of the patch working without causing regressions, and the review was a rubber stamp.

The quality of the codebase has, to put it mildly, deteriorated to an embarrassing state. The CLI has become completely unusable, especially after the mailing feature was implemented.

Here is what I propose:

1. Revisit who has approver status based on the quality of contributions made and the number of times they have approved broken code. It is okay for someone to not have approver rights anymore; this is why code reviews exist.
2. Undo all contributions made to the CLI since September 2023, because it seems it’s all broken code.
3. Enforce unit and integration tests to be added with each PR.
4. Increase the number of reviewers per PR to 2 until these code quality issues are resolved.
5. Require the burden of proof that the PR is indeed working to be on the PR author, written on the PR page.
6. PR authors must reply to comments on GitHub and cannot use back channels of communication to respond using a system of “telephone” with another person with approval rights to avoid responding to the author directly.
7. PR authors who have approval rights but have a consistent track record of not replying to comments have their approval rights stripped.

I am saddened that the situation has come to this, but when broken PRs are being merged without any code reviews, it’s a question of whether Canasta is still usable or not by anyone.

[yaronkoren]

Thanks for these spirited suggestions. It's true that the CLI needs some work; there's actually a plan in place to improve it relatively soon. Of course, you should free to implement any fixes/improvements yourself!

[jeffw16]

Thanks for replying. Would you be able to share the plan, @yaronkoren?

As for implementing the fixes and improvements, my proposal is to roll everything back to September 2023 and restart from there. I can make a PR for that, but I figured that would not be a quick and easy PR to merge in without discussion.

[yaronkoren]

I'm looking forward to your patch.

[tosfos]

I agree with @jeffw16 that the code quality is quite poor and needs to be addressed ASAP. It seems like much of the activity in the repos has been on adding new features but that will only exacerbate the bigger issue, which is quality and bug-free code. A good chunk of that, of course, is a lack of maintainers. WikiTeq will be making a go at adopting Canasta to some degree, which should help alleviate some code quality problems (and already has), but also means that code quality is a bigger problem. We can only adopt Canasta more fully if we are confident that the quality is good and will remain good. To that end, we should start with @jeffw16 's more "no-brainer" suggestions immediately and then debate the rest. To my mind, that includes:

Revisit who has approver status based on the quality of contributions made and the number of times they have approved broken code

We can debate who should get this status and how difficult it should be to get, but the policy is wise.

Enforce unit and integration tests to be added with each PR.

Even if we only require minimal tests, something is infinitely better than nothing. And besides the tests themselves, this will help create a culture of quality and "document" expected behavior.

Require the burden of proof that the PR is indeed working to be on the PR author, written on the PR page.

Again, something is better than nothing. Even "I ran this and my hair didn't catch fire."

PR authors must reply to comments on GitHub and cannot use back channels of communication to respond using a system of “telephone” with another person with approval rights to avoid responding to the author directly.

Much better for everybody not on the private line.

[yaronkoren]

@tosfos - thanks for your insights. I don't think any of these make sense as policy per se in that none of them can be enforced. In the absence of sufficient manpower for enforcement, they look like rules for the sake of rule-making, which obviously some people enjoy, but I don't. I do agree, though, that adding a testing framework/apparatus to some/all of the repositories would be a huge improvement - we have talked about having that for years now, but arguably it should be the highest development priority.

[tosfos]

@yaronkoren I appreciate your concerns about enforcement and manpower. However, I think we can address these in a way that actually reduces maintenance burden rather than adding to it.

On enforceability: Many of these can be automated through GitHub's built-in features, requiring zero ongoing manual enforcement:

• Tests: CI can be configured to block merges if tests fail or if test coverage decreases. This is a one-time setup, not ongoing enforcement.
• PR descriptions/proof of working: GitHub branch protection rules can require PR templates that prompt authors to describe what they tested. Again, one-time setup.
• Multiple reviewers: Branch protection can require 2 approvals automatically. No manual oversight needed.
• Approver status: This is just managing GitHub permissions - something that should be reviewed periodically anyway.

On "rules for the sake of rules": I understand the sentiment, but these aren't arbitrary rules. They're direct responses to the problem @jeffw16 identified: broken PRs being merged without review. The current process isn't working, and these suggestions are standard practices that successful open source projects use to maintain quality with limited maintainer time.

On testing framework: I completely agree that a testing framework should be the highest priority. But I'd argue that requiring tests with each PR should be part of that framework from day one. Otherwise, we'll build a framework that nobody uses, which is what we've seen happen "for years now" as you mentioned.

The beauty of starting with the no-brainers is that they can be implemented incrementally. We don't need to solve everything at once. But if we don't start somewhere, we'll be having this same conversation in another year.

Would you be open to starting with just the CI-based test enforcement? That's the most impactful and requires the least ongoing effort.

[yaronkoren]

Okay, I didn't realize that there were tests in place that are currently (a) failing and (b) non-binding. Is that really the case?

[tosfos]

@yaronkoren I may have been unclear. I wasn't saying there are currently failing tests that aren't being enforced. I was suggesting we set up CI to enforce tests going forward.

[yaronkoren]

Oh, I see. But that's the easy part, no? I would think creating the tests is the harder part.

[jeffw16]

A few years ago, I would have agreed that writing tests can be quite the onerous burden. Nowadays, creating comprehensive unit tests are quite straightforward with LLMs and iterative agentic modes. That's one of the more mature aspects of AI-assisted coding, actually. It's as simple as going on GitHub Copilot's agent mode/Cursor's equivalent and typing "write unit tests for this code and make sure they all pass" into the chat window.

To be totally honest, I was not a fan of writing unit tests before LLMs. It was tedious and not fun, and I didn't find it easy. However, there is no excuse not to include tests whenever possible in 2025.

[vedmaka]

The fun part with writing tests with LLMs shifted from writing the tests to verifying that the tests guessed by LLM actually test anything. LLMs are very inclined on writing code that look like a test but when you look closer it tests nothing

[vedmaka]

On a serious note, just making GitHub workflow run PHPUnit would be very beneficial already, as this requires no new tests to be written. And later, we could look into adding some e2e, where the most fun begins (running Docker in Docker on CI)

[jeffw16]

I don't think any of these make sense as policy per se in that none of them can be enforced.

Au contraire, this can easily be enforced. There are contributors who have written generally good quality code which has stood the test of time and made a positive impact on the project. This includes our GSoC contributors in 2022 and 2023. Contributors who have written good code and abide by strict code review standards should get the ability to approve PRs. The enforcement comes from GitHub, which gives the owners of the CanastaWiki organization the ability to add and remove approval rights for users. On the other hand, this is an opportunity to revisit approval rights for individuals who, with all due respect, would benefit from heightened code review standards. I'm sure they won't like their privileges taken away, but it can (and, in my opinion, should) be done. I thank them for keeping the cogs turning while some of us stepped away from the project, but simultaneously, I do not think it is appropriate for them to continue at this time.

In the absence of sufficient manpower for enforcement, they look like rules for the sake of rule-making, which obviously some people enjoy, but I don't.

As @tosfos said, these proposed rules are in direct response to the issues which have occurred. Trust me, it brings me no pleasure to propose these rules. In fact, I hope they can be relaxed in the future, and I would support relaxing them ASAP. However, given the dire state of the codebase, I encourage the adoptions of these temporary measures.

---

I am aware that laissez-faire governance has generally been the preference in Canasta's development history and, to be honest, I generally support this. For instance, I get pretty upset when people force overly subjective coding standards down my throat. If I had a job where a coworker did that to me in every code review, it would make me want to quit my job. So please believe me when I say I absolutely hate that kind of behavior.

But we've arrived at the opposite end of the spectrum. By embracing a laissez-faire approach without any guarantee of functionality, the fundamental trust of the open-source project to deliver working code has evaporated. Things in Canasta are broken. Really broken. Laissez-faire governance (or, more accurately, the lack thereof) only works effectively when the results are positive. Here, the results have led to dysfunction since September 2023, when our last GSoC contributor completed his contributions.

These grave concerns are why I have proposed these rules, and if you need help with someone enforcing them, I propose myself, WikiTeq staff, and anyone who has had experience with giving comprehensive code reviews in their careers.

[jeffw16]

A tangent about governance: only a small fraction of the contributors to the Canasta Project have spent a signfiicant chunk of their professional careers working for larger organizations which assign titles and levels to folks that convey seniority and expertise. The same rules thankfully doesn't have to apply to an open-source project. That sort of bureaucracy may be necessary for a larger organization and companies, but doesn't belong here. Nonetheless, some of the principles of that do apply even here. For example, the level of experience one has in writing good quality code. Everyone has different backgrounds and everyone learned to code in different ways. I think more junior developers contributing to Canasta should be appreciated for the code they are writing, but would benefit from gaining additional experience with proper engineering excellence practices. In the meantime, I think their approval rights should be revoked until they have demonstrated that their code is of high quality. Basically, this would be the equivalent of reaching L61 at Microsoft, L4 at Google, L5 at Amazon, and E4 at Meta. In FAANGMULA+ parlance, this is usually known as SWE 2 or SDE 2. Folks who are able to approve PRs should have skillsets at a SWE II level at the very least.

[yaronkoren]

@jeffw16 - thanks for you feedback. Saying "I do not think it is appropriate for them to continue at this time" seems rather harsh - I'm actually not entirely sure who you're talking about, but I can't think of anyone working on the Canasta code that I think should be removed - or even have their approval rights taken away, for that matter.

Interestingly, one set of rules you proposed before that did get approved was this rather long set of rules for accepting extensions into Canasta. You then broke most of those rules when you merged in this PR, to add the ArrayFunctions extension, right after it was submitted, and without any discussion. Does that violation mean you should be removed from this project? That seems extreme to me, but I'm willing to be persuaded.