Repository navigation
Conversation
Records live as Markdown with typed YAML front matter under dokimi/research/records/<type>/<id>.md, validated by research/record.v1.json. scripts/research_register.py checks schemas, paths and cross-references, queries by field or reference, hands out the next id, and exports records, edges and a per-theme idea tree for the plans-and-research map. Negative and inconclusive outcomes are first-class; experiment outcome, decision state and implementation status are separate fields. Seeds 98 records from the project's public PRs/issues and local research notes; AGENTS.md names the register as the one exception to the no-status-reports rule. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #139 +/- ##
==========================================
- Coverage 91.29% 91.28% -0.01%
==========================================
Files 216 216
Lines 26565 26565
Branches 4337 4337
==========================================
- Hits 24252 24250 -2
- Misses 1645 1646 +1
- Partials 668 669 +1 🚀 New features to boost your workflow:
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 5dbb112. The tooling is small and does what the doc says: check passes on the real tree, the doc's example record validates, and the PR touches no package code. The problems are in the record content and in what check does not catch. Seven points are inline; I'd fix all but the superseded_by nit before merge.
What I ran, on a Linux review host at 5dbb112:
make ci-local: 20 passed, 0 failed, 4 skipped; 5322 tests passed, 1 skipped; the changed-lines coverage step passed.- Reproductions for the script findings, with a temporary register and no network.
- A hand check of the 98 records against their own result text, and of the doc's commands against the script.
Checked, no issues
- Docs:
research-register.mdis in themkdocs.ymlnav and linked fromdocs/README.md; the commands, flags and theintel//napetrov/link rule match the script and schema. - Repo rules: the Markdown records carry the SPDX header, the JSON schema is covered by the license ignore for JSON,
jsonschemaandPyYAMLare declared dependencies, and no generated file is committed. - Public content: a grep of the records for local paths, hosts and credentials found nothing.
- Counts: 21 ideas, 27 experiments (12 refuted, 12 supported, 3 inconclusive; none is
planned, unlike the description), 19 findings, 13 decisions, 18 implrefs.
…y, pin more register behaviour export, query and next read the register without checking it, so a record that check rejects crashed them with a traceback, and a nested reference crashed check itself. Every command now runs the schema and reference checks first and exits 1 with the messages. superseded_by is only valid on a superseded decision and cannot name the record itself. Tests cover --ref with --type, misplaced and misnamed files, id gaps, the broken-register path for each command, and unevaluatedProperties on every record type. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 7123967. The script fixes hold: query, next and export now refuse a broken register, a nested reference is reported as a schema error, and the new superseded_by checks and tests are pinned. The record content is where it still falls short. Four of my earlier threads are open, and I found four more points, inline.
What I ran, on a Linux review host at 7123967:
make ci-local: 20 passed, 0 failed, 4 skipped; the changed-lines coverage step reports no lines with coverage information in this diff.- Mutation run on
research_register.pyandrecord.v1.json: 26 script mutants and 5 schema mutants against the 28 register tests. The mutants that matter and survive are listed inline. - A hand check of the experiment outcomes and findings against their result text.
Still open from my earlier review:
dokimi/research/records/experiment/X006.md:8:outcome: supportedcontradicts the result text, the body does not say what the retraction covers, and F006 (high) and F010 still rest on retracted experiments.dokimi/research/records/experiment/X003.md:8: the outcome belongs to the enumeration variant, not to the hypothesis on line 7.dokimi/research/records/finding/F009.md:15: "kappa 0.576" is still not in X009, where 0.576 is an NLI AUC.dokimi/scripts/research_register.py:131: a finding that cites a retracted experiment still passescheck, andexportstill emits the retracted outcome.
Checked, no issues
- Refusal path: removing the early return, or writing the errors to stdout, fails the parametrized broken-register test for all three commands.
- New checks: the self-reference, state-mismatch, missing-target, wrong-type and non-string-reference mutants are all killed.
- Schema: flipping
unevaluatedPropertiesto true for each of the five record types fails a test. - Docs: the commands and flags in
research-register.mdmatch the script.
|
Pushed 7e152f4 for the four new points in the review at 7123967:
The four open threads (X006, X003, F009, and the retracted-experiment check in |
…field queries - X027: the hypothesis (a self-hosted tool enforces the schema) is refuted by the result text, so the outcome is refuted. - F016: drop the "~7% / lost accuracy" clause; no cited experiment has it. - query: exit 2 on a --where clause that is not FIELD=VALUE on a schema field, instead of printing nothing with exit 0. A missing field matches no value rather than the string "None". - tests: export ignores a decision that points at an idea; bad --where clauses and the missing-field case are pinned. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
bf5f52f to
7abf6f7
Compare
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…der rules - X006 refuted, partial retractions under Corrections; X012, X025 the same. F006 lowered to med. - X003 split: criterion wording only (refuted) and X028 criterion plus enumeration (supported). F002 and F003 cite both. - X029 holds the grader agreement F009 quotes; D004 rationale says where +0.007 comes from. X021 gains the compaction follow-up. - Outcome sweep: X002, X008, X021 inconclusive; X013, X016 refuted. - check fails a finding that cites a retracted experiment; export marks retracted experiments; query --ref on a missing id exits 2. - render writes the plans-and-research map (Mermaid mindmap and table) on demand; it is not committed or published. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
YAML reads retracted: true as a bool and str() spells it True, so the documented --where retracted=true matched nothing and exited 0. Query rows also showed a retracted experiment as plain supported. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
load() globbed */*.md, so a record directly under records/, one level too deep or named .markdown was never validated or exported, and check still printed OK. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
The url pattern checked only the prefix, so a valid prefix followed by a space and an internal URL, or by ../ or %2e%2e segments, passed check. Every one of the 37 links in the register still matches. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
The stamp was git rev-parse HEAD of the script's checkout, so a map rendered from uncommitted records named a commit with other contents. Read the commit from the --root tree and add a dirty flag, as the run manifest does. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Redirected stdout uses the locale encoding, so on Windows export wrote F009's ± as byte 0xB1 (not valid UTF-8 JSON), and a title outside cp1252 made export, query and render exit 1 with UnicodeEncodeError. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
…nd render No fixture experiment was retracted and no title had brackets or a pipe, so dropping the export flag, the render mark, _label or the table escape left every test green. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at ccc9fa2. All eight of my earlier threads are fixed: the outcomes now match the result text, the retracted-experiment check works and is tested, and the query typo cases exit 2. What is left is a crash in the new retracted check, a broken render, findings and decisions whose text is not in the experiments they cite, and untested reference checks; three nits. Ten points: eight inline, two below.
What I ran, on a Linux review host at ccc9fa2:
make ci-local: 26 passed, 0 failed, 4 skipped; the changed-lines coverage step passes with no measurable lines in this diff.checkon the real tree:OK: 100 records. 71 mutants onresearch_register.pyandrecord.v1.jsonagainst the 52 register tests: 22 survive, the ones that matter are inline.- Reproductions for the script findings, with temporary registers and no network.
- A hand check of every experiment outcome, finding and decision against the records it cites.
Findings that cannot be anchored inline:
- Nit:
exportandrendercrash withFileNotFoundErrorwhengitis not installed._git(research_register.py:189-191) callssubprocess.run(["git", ...]), andcheck=Falsedoes not cover a missing binary. WithPATHemptied,export --root <valid register>raisesFileNotFoundError: ... 'git'. A register outside a git repo is fine (commit: null). Suggestion: catchOSErrorin_gitand return"". F003quotes a range that its own experiments contradict.finding/F003.md:17says wrong_numbers "stayed 0.67-0.82 across every judge configuration measured". F003 cites X002, X003 and X028, and X028's baseline is 0.599. X002 has no wrong_numbers figure, so "wrong names are caught" has no cited support. The 0.47-0.72 range is in X008, which F003 does not cite. Suggestion: cite X008, and restate the range from the cited figures.
I agree with @xaleryb's points on the id clash after next and on the stale counts in the PR description.
Checked, no issues
- Outcomes: X001, X003, X006, X007, X009-X011, X013-X019, X022, X023, X027 and X029 match their hypothesis against their result text; X003/X028 and X006's
## Correctionshold. - Docs build:
mkdocs build --strictpasses; the page is in thenavand linked fromdocs/README.md; the commands and flags match the script. - Repo rules:
record.v1.jsonis outsidedokimi/schemas/, soSCHEMA_VERSIONSdoes not apply; no changelog fragment is needed; SPDX headers are on every record;gitleaksand a grep for local paths and non-GitHub hosts find nothing. - Script: the path and extension check, the UTF-8 reconfigure, the
retractedmark inquery,exportandrender, and the--refand--whereexits are all killed by a mutant.
| if record.fields.get("superseded_by") == rid: | ||
| errors.append(f"{rid}: superseded_by names the record itself") | ||
| for target in ref_values(record, "experiments"): | ||
| if target in records and records[target].fields.get("retracted"): |
There was a problem hiding this comment.
A non-string entry in experiments crashes check with a traceback.
Line 142 skips non-string targets, but this loop does target in records without that guard. With finding/F001.md set to experiments: [[X001]], check raises TypeError: unhashable type: 'list'. experiments: [{a: 1}] and experiments: {a: 1} do the same. query, export and render run check first, so they crash too. The schema error that would explain it is never printed. A scalar experiments: X001 is handled ('X001' is not of type 'array'). The nested-reference case was fixed for REFS fields in the last round; this loop is the new retracted check.
Suggestion: reuse the loop's guard, for target in ref_values(record, "experiments"): if isinstance(target, str) and target in records and ..., and add a test_bad_record_fails row with experiments: [[X001]].
| continue | ||
| lines.append(f" {theme['id']}[{_label(theme['title'])}]") | ||
| for idea in theme["ideas"]: | ||
| lines.append(f" {idea['id']}[{idea['id']} {_label(idea['title'])} - {idea['status']}]") |
There was a problem hiding this comment.
A title with a newline breaks the Mermaid mindmap and the table.
_label strips brackets but not newlines, and the schema's text type is only minLength: 1. A folded title (title: > followed by two lines keeps a trailing newline) renders as I001[I001 Folded title over lines\n - open], so the status suffix lands on its own line and the diagram does not parse. title: "multi\nline [x] - done" does the same. The table rows at :250 and :253 only replace |, so they split too.
Suggestion: collapse whitespace in _label and in the table cells with " ".join(text.split()), and add a render test with a multi-line title.
| SPDX-License-Identifier: Apache-2.0 | ||
| --> | ||
| ## Statement | ||
| When both arms pass every paired task, compare must flag no headroom. |
There was a problem hiding this comment.
F018 (confidence: high) states something its only experiment does not show.
The statement is "When both arms pass every paired task, compare must flag no headroom." The only cited record, X024, is a regrade of 1414 answers whose whole result is "0 mismatches." (experiment/X024.md:20). It says nothing about ceilings or headroom. D009 and R022 rest on F018.
Suggestion: cite the experiment that found the zero-delta ceiling case, or add it as a record, or lower the confidence and say it is a design rule.
| SPDX-License-Identifier: Apache-2.0 | ||
| --> | ||
| ## Rationale | ||
| The five dimensions behaved as one factor; averaging diluted correctness. Done in pre-public history. |
There was a problem hiding this comment.
D002 cites F002, which has nothing to do with dimension weights.
D002 is "weight correctness x2; zero-weight specificity" and its rationale is "The five dimensions behaved as one factor; averaging diluted correctness." Its only finding, F002, is about enumerating figures to catch wrong numbers (X003, X028). "Behaved as one factor" appears in no experiment in the register, so a reader following the link finds no support.
Suggestion: add the experiment and finding behind the weighting, or say in the rationale that it comes from pre-public history without a record.
| SPDX-License-Identifier: Apache-2.0 | ||
| --> | ||
| ## Rationale | ||
| Measured gains negligible or refuted: stratified probe sampling moved agreement by +0.007, inside the grader spread F009 reports, so it does not pay for its probe budget; the LLMLingua shortener was refuted (X009), and the SliCK split showed no gain worth building either. |
There was a problem hiding this comment.
The numbers in D004's rationale are in no record.
"Stratified probe sampling moved agreement by +0.007" and "the SliCK split showed no gain" appear nowhere else: grep -rn "0.007\|SliCK" research/records returns only this file. F009 cites X009 and X029, which cover the LLMLingua shortener and grader agreement. The earlier thread on F009 is fixed; this is the figure that replaced it.
Suggestion: add an experiment for the sampler and the SliCK split and cite it from F009, or drop both claims from the rationale.
| "names the record itself"), | ||
| ("idea", "I002", "title: T\ntheme: O\nquestion: Q\nstatus: open\nextra: 1\n", | ||
| "Unevaluated properties"), | ||
| ("finding", "F002", "title: T\nexperiments: [X001]\nconfidence: low\nextra: 1\n", |
There was a problem hiding this comment.
The reference checks for finding, decision and implref are not pinned.
Mutation check: emptying the finding, decision.superseded_by, decision.ideas or implref entries of REFS passes all 52 tests. Only the experiment-to-idea edge has a missing-target and wrong-type case here, and finding.experiments is covered only by the retracted check. Other mutants that survive: dropping the -- . pathspec in the dirty check, _label on the theme title, the pipe escape on an idea title, and sorted on the idea and experiment lists.
Suggestion: add test_bad_record_fails rows for finding.experiments: [X999], implref.decision: F001, decision.findings: [X001] and decision.ideas: [X001], each with a no record or want message.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 999ecf3, which fix the three nits from my review:
308d6fd:dokimi/scripts/research_register.py:155:checknow followssuperseded_byand reports{rid}: superseded_by chain loops back to {rid}; a two-decision cycle test is added.4ac9f70:dokimi/docs/research-register.md:34: the page now says optionallinks(required onimplref).999ecf3:dokimi/scripts/research_register.py:197:_gitcatchesOSErrorand returns an empty string, soexportandrenderno longer crash without git; a test is added.
make ci-local at 999ecf3: 26 passed, 0 failed, 4 skipped.
Not approving yet. Still open: 1 bug finding(s) from my review; 6 risk finding(s) from my review.
|
CI at |
This branch has not been deployed
4 participants
Summary
Adds a register in the tree for Dokimi's own research: ideas, experiments (negative ones included), findings, decisions and implementation links. Each record is plain Markdown that agents and people can both read and write. It also produces the data for the plans-and-research map.
How the record types point at each other. The three states in brackets are separate fields:
exportwrites every record, the reference edges, and ideas grouped by map theme (P/E/V/K/G/O/C/D/W) with each idea's experiment outcomes.Why plain files instead of a server: I set up XWiki, OpenProject CE, eLabFTW, GitLab CE and git+Markdown locally and ran the same 8 tests on each. Only git+Markdown passed all 8, with no server and real history. None of the server tools rejected a reference to a missing record or a wrong enum value on an API write, so a validator would be needed anyway.
Evidence
After:
tests/test_research_register.py(13 tests) runscheckon the real tree. It also covers broken records that must fail: a reference to a missing record, a reference to a record of the wrong type, a bad enum value, an unknown field, a superseded decision withoutsuperseded_by, a wrong id prefix, a non-public link, and a duplicate id.make ci-local --offline: everything passes (network and docker jobs are skipped). The license header check passes on the new files.Merge Danger
Door: two-way
Data and a standalone script only. Nothing is added to the package, so no changelog fragment is needed. Revert is a clean delete.
Blast Radius: docs
AGENTS.mdcurrently says "the tree has no ADRs or status reports". This PR adds one named exception. If maintainers would rather keep that rule strict, the register can move to a separate repo in the same format.github.com/intel|napetrovURLs (the schema enforces this).🤖 Generated with Claude Code