From bd7a65ac567a4363c940b6f575f7a8a4c8d64543 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Harboe?= Date: Tue, 27 Oct 2026 11:57:43 +0100 Subject: [PATCH] docs: gemini: add a review style guide with hard stops on silenced errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ORFS has no Gemini review guidance. The most common advice reviewers turn down is also the most harmful when followed: guarding a variable that variables.yaml defaults, or a fallback that turns a missing file into an empty result, so an hours-long run finishes green and measures nothing. Make that a hard stop, point at the Tcl and Makefile contracts instead of repeating them, and list the other false positives that recur in the review threads. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Øyvind Harboe --- .gemini/styleguide.md | 57 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 .gemini/styleguide.md diff --git a/.gemini/styleguide.md b/.gemini/styleguide.md new file mode 100644 index 0000000000..6e08cf4f9e --- /dev/null +++ b/.gemini/styleguide.md @@ -0,0 +1,57 @@ +# OpenROAD-flow-scripts code-review guidance + +A change to `flow/scripts/*.tcl` is reviewed against every rule in +[TclContract.md](../docs/contrib/TclContract.md); a change to `flow/Makefile`, +`flow/scripts/*.mk`, `flow/scripts/variables.yaml` or a `config.mk` against +every rule in [MakefileContract.md](../docs/contrib/MakefileContract.md). This +guide does not repeat them. + +A wrong comment costs the author time to refute it, and a wrong comment that +gets followed costs more: it can turn a loud failure into a silent wrong result, +which in ORFS can mean an hours-long run that finishes green and measures +nothing. The rules below are hard stops; a comment that breaks one is not posted. + +## Hard stops: never suggest silencing an error + +ORFS wants loud, early failures. A missing file, a malformed setting or an +unexpected tool output stops the flow where it is found, with a message that +names it. + +- Never suggest guarding a variable that has a `default:` in + `flow/scripts/variables.yaml`. ORFS sets it; a bare `$::env(VAR)` is correct, + and `env_var_exists_and_non_empty` or `env_var_or_empty` around it hides a + broken setup. +- Never suggest a fallback that runs without saying so: `or 0.0` or `.get(key, + default)` on a required setting, `try`/`except: pass`, `catch` without + rethrowing, a tolerant parser for a file ORFS writes itself, `mkdir -p` or + `$(wildcard ...)` that makes a misspelt path vanish, or "if this fails, skip + the step". +- A missing input is reported as a stop with a clearer message, never turned + into an empty result. + +## Ground findings in the current code + +- Before claiming a tool option, PDK layer or via, Python package version or + repository file does not exist, find it: the yosys `help` text, the OpenROAD + docs, the LEF, the package index, the tree. Before saying a helper such as + `write_lec_verilog` gets a path wrong, read the helper. +- Check shell, make, Tcl and yosys semantics before calling something a bug. + An empty `for` word list is valid shell and runs zero times. In + `$(eval $(call ...))`, `$$$$` passes a literal `$` through to the shell, so + work out which expansion the code needs before suggesting it. In make, `target: prerequisite` means the target depends on the + prerequisite. A yosys `select` with several arguments is a union. +- ORFS runs on Linux. RHEL 7 and paths with spaces are not supported anywhere; + do not ask for quoting, arrays or compiler probes for them. + +## Keep comments in scope + +- Do not comment on code moved verbatim, vendored PDK files or files copied + from upstream projects; they are kept identical to their source. +- `rules-base.json` limits, QoR trade-offs and constraint choices are the + maintainers' decision. Do not lecture on a degradation the PR description + explains. +- Do not suggest one-off cleanups (encoding arguments, wording, extra file + extensions) that belong in a sweep of their own. +- Do not repeat resolved findings, or a finding the author answered in an + earlier round. Read earlier rounds; never ask to undo a change an earlier + round asked for. Keep comments concise and omit unsolicited PR summaries.