# Coding Rules

Rules that hold for every file this repository authors. They exist because both failure modes below
have already shipped here, and both were invisible to the test suite at the time.

Two of these are enforced mechanically. The rest are review criteria and say so. A rule with no named
enforcement point is a wish, not a rule.

| Rule | Enforcement |
|---|---|
| R1 — No foreign source inside a string literal | `tests-js/coding-rules.test.mjs` (runs in `npm run check`) |
| R2 — No exception swallowed without a signal | `tests-js/coding-rules.test.mjs` |
| R3 — No untracked `TODO` / `FIXME` / `HACK` / `XXX` | `tests-js/coding-rules.test.mjs` |
| G1–G4 — Comment honesty | Code review. Not mechanical |

Scanner: [`tools/coding-rules.mjs`](../tools/coding-rules.mjs). Allowance ledger:
[`config/coding-rules-baseline.json`](../config/coding-rules-baseline.json).

---

## How the enforcement works: a ratchet, not a wall

At the time these rules landed the repository already violated R1 in 138 places and R2 in 21. R3 stood at zero.
All three are at zero today, but the ratchet stays — it is what holds them there.
A rule that turns the whole repository red on day one never gets switched on. So the check is a ratchet.

- `config/coding-rules-baseline.json` records the per-file violation count at landing time.
- The test fails when a file's count **exceeds** its baseline entry, or when a file with no entry
  gains a violation. New code is held to zero.
- The test also fails when a file's count **drops below** its baseline entry and the baseline was not
  updated. Fixing a violation must tighten the ledger in the same commit. Without this half the
  ratchet does not ratchet.

After removing violations:

```bash
node tools/coding-rules.mjs --write-baseline
```

Commit the regenerated baseline together with the fix. Never regenerate it to make a new violation pass —
that is the one use of the flag that defeats the rule.

---

## R1 — No foreign source inside a string literal

**A program written in language A must not carry the source of a program in language B inside a string
literal, heredoc, or template.**

### What it looks like

```js
// forbidden
const SCRIPT = `
import json, sys
from okstra_project import list_project_tasks
print(json.dumps(list_project_tasks(sys.argv[1])))
`;
await runPythonSnippet({ script: SCRIPT, args: [root] });
```

```bash
# forbidden
python3 - "$PYTHONPATH" "$explicit" <<'PY'
from okstra_project import resolve_project_root
print(resolve_project_root(explicit_root=sys.argv[2]))
PY
```

### Why it is banned

Embedded source is invisible to every tool that would otherwise catch a defect in it. `tsc` sees a
string. `ruff` and `mypy` never open it. `pytest` cannot import it. Coverage reports it as one line.
The defect is not that the code is ugly — it is that **the code is unreachable by the checks the rest
of the repository relies on**, so a bug there survives a green `npm run check`.

Escapes are the second cost: `\\n` inside a template that becomes `\n` in the emitted program is a
class of bug that exists only because of the embedding.

### What to do instead

Put the code in a real file with the right extension, so its toolchain sees it, and call it by path or
module name.

```js
// allowed — the python lives in scripts/okstra_ctl/task_list_cli.py and is importable, testable, lintable
await runPythonModule({ module: "okstra_ctl.task_list_cli", args: [root] });
```

### Detected forms

| Pattern | Example |
|---|---|
| `py-heredoc` | `python3 - <<'PY'` |
| `py-dash-c` | `python3 -c "..."` |
| `py-arg-c` | `spawn("python3", ["-c", ...])` |
| `run-py-snippet` | `runPythonSnippet({ script: ... })` |
| `node-dash-e` | `node -e "..."` |

### Not covered by this rule

- Invoking a real command with arguments — `git rev-parse HEAD`, `python3 -m okstra_ctl.run --flag v`.
  A module or file reference is not embedded source.
- Prompt and report templates under `prompts/` and `templates/`. They are read by humans and models,
  never executed, and are not scanned.
- A fixture file whose *content* is foreign source, when the fixture is a real file on disk. Inline
  fixture strings are still violations — write the fixture to a file.

---

## R2 — No exception swallowed without a signal

**A handler whose body neither re-raises, nor records the failure, nor returns a value the caller can
distinguish from success, must declare itself with an `expected-miss:` tag.**

### What it looks like

```python
# forbidden — the row vanishes and nothing anywhere knows
try:
    entry = json.loads(line)
except Exception:
    pass
```

```js
// forbidden — same shape, and the comment makes it look considered
try {
  entries.push(JSON.parse(line));
} catch {
  // Drop the unparseable row.
}
```

### Why it is banned

A swallowed exception converts a defect into missing data. The program keeps running, the output is
wrong, and there is no line anywhere that says so. This is the most direct form of hiding a bug: the
evidence is destroyed at the moment it is produced.

The comment does not help. A comment is read by whoever opens the file; it is not read by the person
staring at output that is short by three rows.

### What to do instead

Pick one, in order of preference:

1. Let it propagate. If the caller cannot continue without the value, that is the correct behaviour.
2. Record it — a counter in the return value, a `stderr` line, an entry in the run error log
   ([`okstra error-log`](cli.md)). The caller can then report "3 rows skipped".
3. If the exception genuinely marks a non-error branch, say so with the tag:

```python
try:
    (src_root / rel).stat()
except FileNotFoundError:
    # expected-miss: 페이로드에 없는 파일 = orphan. 존재하지 않는 것이 정상 분기다.
    orphans.append(rel)
```

The tag is what makes the check pass. Its purpose is to force the author to answer one question in
writing: *is this an error I am ignoring, or a branch that is not an error?* Those are different, and
a bare comment does not distinguish them.

An `expected-miss:` tag whose text does not name why the absence is normal is an R2 violation that
happened to get past the scanner. Reviewers reject it under G1.

---

## R3 — No untracked marker

**`TODO`, `FIXME`, `HACK`, `XXX` must carry a task reference or a URL on the same line, or be removed.**

```js
// forbidden
// TODO: handle the multi-stage case

// allowed
// TODO(dev-10174): handle the multi-stage case
```

A marker without a reference is a defect that has been noticed, recorded where no tracker will find it,
and left. Either it is worth tracking — then track it — or it is not, and the line should go.

---

## G1–G4 — Comment honesty (review criteria)

These are not mechanically checkable. They are rejection criteria in code review.

### G1 — A comment must not assert a property the code does not enforce

```python
# forbidden — nothing in this function makes that true
def load(path: str):
    # path 는 항상 절대경로다
    return json.loads(open(path).read())
```

If the property matters, enforce it (assert, validate, or type it). If it does not matter, delete the
sentence. A comment that states an unenforced invariant is worse than no comment: the next reader
writes code that depends on it.

### G2 — A comment must not justify a duplicate instead of removing it

A real example from this repository, since removed. `src/commands/lifecycle/config.mts` carried:

> "Mirror the python resolver order without depending on it … Python remains the source of truth at run
> time; this is informational."

The comment was accurate and the code was still wrong. Two implementations of one resolution order
existed, one of them declared non-authoritative, and nothing detected the day they disagreed. The
correct change is to call the authority, not to annotate the copy — which is what
[`project_setup_cli.py`](../scripts/okstra_ctl/project_setup_cli.py) now is: one resolution, called
from every entry point that used to keep its own copy.

If a duplicate is genuinely unavoidable, the comment must name the drift detector — the test that fails
when the two diverge. No detector, no duplicate.

### G3 — A comment must not stand in for a missing test

"이 경로는 테스트하기 어려워서 수동 확인함" records that a gap exists and closes the discussion. Write the
test, or record the gap where it will be triaged (R3 applies), but do not settle it in a comment.

### G4 — A comment must not argue that a known defect is acceptable

The pattern: a comment explains why the wrong behaviour is fine here. It is a review conversation
compressed into a line no reviewer will see again, and it converts a bug into a documented feature
without anyone deciding to.

Say what the code does and why it is built that way. Do not argue for it. If the defect is acceptable,
that is a decision, and decisions belong in
[`<PROJECT_ROOT>/.okstra/decisions/`](architecture/storage-model.md) or an ADR — somewhere with a date
and an owner.

---

## What a good comment does

The rules above are all prohibitions, so state the positive form once. A comment earns its line when it
carries something the code cannot:

- **Why this and not the obvious alternative.** [`src/lib/python-helper.mts:99`](../src/lib/python-helper.mts:99)
  explains that chunks are joined once because `s += chunk` is quadratic for large rendered prompts.
  The code shows the how; only the comment shows the why.
- **A constraint that lives outside this file** — an upstream bug with a link, a host contract, a
  protocol requirement.
- **A non-obvious consequence of an ordering.** Why this call must precede that one.

Repeating what the next line already says is filler. Delete it.

---

## Applying these to existing code

The baseline is not a target to preserve. Its entries are the work list.

**All three rules are at zero.** R1 started at 138, R2 at 21, R3 at zero and stayed there. The baseline
file holds no entries; the ratchet now holds every file to zero.

R1 was cleared by moving the code into files, not by loosening the rule. 44 assertion blocks came out of
`tests-e2e/*.sh` and `validators/lib/*.sh` heredocs into `tests-e2e/checks/` and `validators/checks/`,
byte-for-byte — each carries a header naming the shell line that calls it. The recurring one-liners
collapsed into `tests-e2e/lib/jsonq.py`, which replaced the same three lines of JSON-field extraction in
eight places and, unlike the inline form, exits non-zero on a missing key instead of handing the shell an
empty string. The rest were one-offs given their own file: a branch name computed through
`compute_branch_name` rather than assembled by hand, an N+1 fixture, a provider stub's recorder.

Four sites were not embedded code at all — a docstring in `forbidden_actions.py` describing the very
forms the rule bans, a markdown census fixture, and this file's own examples. Those were reworded or
split so the source no longer carries the literal token while the runtime value stays the same. The
scanner reads source text and has no waiver syntax, so quoting a banned form costs a rewording. That is
the price of a rule with no exceptions, and it is cheap.

Two extractions surfaced a fragility the heredocs had been hiding: `tests-e2e/scenario-13` and `-14` name
the repo root rather than their own directory, and `validators/lib/*.sh` took its path from whichever
caller had sourced it. Both produced an unbound variable the moment the code moved out of the string.
The e2e completion marker caught the first pair; `validators/lib` now resolves `checks/` from its own
`BASH_SOURCE` so a lib file works whether the whole validator or one contract test sourced it.

**R2 was cleared the same week.** Every handler was one of two things, and the split is the point of the
rule:

- A real swallow, fixed by recording it. The malformed `LEAD_ASSIGNMENT_JSON` / `WORKER_ASSIGNMENTS_JSON`
  fallbacks in `render.py` produced a *different* roster with no way for the caller to tell; `memory.mts`
  dropped truncated index rows so a Memory Book came back short and looked complete; `report.js` swallowed
  a refused clipboard copy, so the button read as broken. Each now names what it lost.
- A branch that is not an error, declared with `expected-miss:`. `worker_runner`'s `ProcessLookupError`
  means the group it was about to kill already exited — that is success. `install.mts` uses `fs.access`
  as an existence probe, so the throw *is* the answer.

Two handlers turned out to be both, and were split: reading a previous run's team-state
(`okstra_token_usage/collect.py`) treats a missing file as normal and a corrupt one as worth saying, and
the session-jsonl reader in `claude.py` does the same with `FileNotFoundError` against every other
`OSError`. Writing the tag forces that question, which is what the rule is for.