Repository navigation
Conversation
|
Thanks for the pull request, @adeotek. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Formal verification. PR-changed functions: 2/5 verified (0 proven, 2 may-equivalent, 0 distinguished) · 3 not verified (3 vacuous).
Not verified on this run: \_platform\_skill\_destination (vacuous: never exercised), \_project\_install (vacuous: never exercised), \_project\_uninstall (vacuous: never exercised).
Graphify review — findings
Adds moca as a skill-only install target: graphify moca install (or install --platform moca) copies the shared claude skill bundle, including the references/ sidecar, to $XDG_CONFIG_HOME/moca/skills/graphify/SKILL.md, falling back to ~/.config/moca/... when the variable is unset. --project installs to ./.moca/skills/graphify/ instead. Neither scope writes a rules file like CLAUDE.md or AGENTS.md, and uninstall removes the skill or prints "nothing to remove".
Worth a look
- CLI test installs into and deletes from the real user home directory —
tests/test_moca.py:183· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- User-scope tests don't clear XDG_CONFIG_HOME, leaking writes outside tmp_path —
tests/test_moca.py:19· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 553 functions depend on the 207 functions this change touches.
Health — this change adds coupling hotspots:
- new:
main()— 105 callers, 3 callees - new:
dispatch_command()— 2 callers, 128 callees - new:
install()— 14 callers, 11 callees - new:
_copy_skill_file()— 17 callers, 7 callees - new:
codebuddy_install()— 21 callers, 5 callees - new:
_refresh_stale_skills()— 25 callers, 4 callees - new:
_project_uninstall()— 7 callers, 13 callees - new:
_project_install()— 6 callers, 13 callees - …and 21 more — each is listed as a finding
Verification — 553 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 553 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
35 of 335 test file(s) selected (10%) via static blast radius.
tests/test_affected_cli.py— impacttests/test_agents_platform.py— impacttests/test_atomic_version_stamp.py— impacttests/test_claude_md.py— impacttests/test_codebuddy.py— impacttests/test_dedup_shrink_refuses_force_write.py— impacttests/test_detect.py— impacttests/test_devin.py— impacttests/test_explain_cli.py— impacttests/test_extract_cli.py— impacttests/test_gemini_hook.py— impacttests/test_global_add_tag_inference.py— impacttests/test_god_nodes_cli.py— impacttests/test_hollow_chunks_arm_shrink_guard.py— impacttests/test_home_sandbox.py— impacttests/test_hook_strict.py— impacttests/test_incomplete_build_guard.py— impacttests/test_install.py— impacttests/test_install_references.py— impacttests/test_install_strings.py— impacttests/test_install_version_warning.py— impacttests/test_merge_chunks_validation.py— impacttests/test_moca.py— impact, changed-testtests/test_multigraph_diagnostics.py— impacttests/test_no_dedup_flag.py— impacttests/test_path_cli.py— impacttests/test_query_cli.py— impacttests/test_query_induced_edges.py— impacttests/test_read_hook.py— impacttests/test_replace_or_append_section.py— impacttests/test_search_hook.py— impacttests/test_skill_auto_refresh.py— impacttests/test_skill_version_warning.py— impacttests/test_uninstall_scope.py— impacttests/test_unverified_semantic_shrink.py— impact
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
No difference found (not proven): No behavior difference found in \_run\_cli (not a proof).
The verifier ran both versions of \_run\_cli on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify \_platform\_skill\_destination.
The verifier did not have enough to check \_platform\_skill\_destination, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: not verifiable: all 41 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly KeyError — names the real obstacle, not a sampling gap)
Could not verify: Could not verify \_project\_install.
The verifier did not have enough to check \_project\_install, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: not verifiable: all 218 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
Could not verify: Could not verify \_project\_uninstall.
The verifier did not have enough to check \_project\_uninstall, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: not verifiable: all 218 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly KeyError — names the real obstacle, not a sampling gap)
No difference found (not proven): No behavior difference found in dispatch\_install\_cli (not a proof).
The verifier ran both versions of dispatch\_install\_cli on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 29 more finding(s) on lines outside this diff (see the check run).
….1 (#9) * docs: OAuth policy + endpoint verification record (anthropic api_key only; openai SIWC token sharing) * feat(provider): 0600 token store with cross-process locked refresh; config rejects anthropic oauth * feat(provider): PKCE login with loopback callback, headless paste, ID token validation * feat: moca login/logout for subscription providers permitted by policy; registry + responses adapter OAuth route * docs: graphify install --platform moca (upstream PR Graphify-Labs/graphify#4174) * chore: version stamping, release build, README for v0.1, CHANGELOG * test: automated §14 ship gate (fixture + checker + runner) * docs: SPECS.md phase-7 update (OAuth, token store, CLI, ship gate) * docs: phase-7 plan checkboxes + implementation notes * docs: phase-7 review passes 1 & 2 (0 High; 6 Medium, 10 Low combined) * fix(provider): store writes force 0600; entropy failure aborts login; first-wins login channel; scheme-less paste recovery; JWKS kid/exponent pins (review 1-1, 1-2, 2-1, 2-3, 2-8, 2-9, 2-10) * fix(config): oauth requires an OAuth-capable provider; SetString last-wins on duplicates + refuses non-string values (review 1-4, 2-5, 2-6) * test(cmd): pin the OAuth sentinel exit mapping (review 1-3, 2-2) * chore: shipgate temp-dir cleanup (keep on failure); README status wording (review 1-5, 1-6) * docs: SPECS phase-7 review-fix updates; plan review-fixes section; store symlink-policy comment (review 2-7) * test(shipgate): checker runs go test ./... in the final repo (review 3-1) Gate 4 trusted the transcript's "[exit 0]", which is the exit status of the whole shell command: `go test ./... | tail` or `|| true` reports 0 with the suite red. The checker now also runs the suite in -repo and fails gate 4 if it is red. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(provider,cmd): login keeps the switch-auth answer; CheckCredential never refreshes; ctx-aware lock wait, bounded + retried refresh; callback page reports failure (review 3-2..3-5, 3-8, 3-9) - CheckCredential (the TUI /model and resume check) refreshed an expired OAuth token over the network on the update goroutine with no ctx. For OAuth it now only confirms a stored login. - The paste prompt's blocked stdin read outlived Login and swallowed the "Switch it now? [y/N]" answer. New provider.LineReader lets an abandoned read keep its line for the next consumer; runLogin shares one reader. oauthLookup seam lets the CLI be tested against a fake authorization server. - The store-lock wait ignored ctx (blocking flock): it now polls non-blocking (LOCK_NB / LOCKFILE_FAIL_IMMEDIATELY) and gives up with ctx. Refresh is bounded at 30s, logout's revoke at 15s. - A 429/5xx from the token endpoint is now an *HTTPError (retried, Retry-After honoured), and CredentialFor falls back to a still-valid access token when the pre-expiry refresh fails non-terminally. - The loopback page answers a refused login with 400 "Login failed". - Tests for the open-failure message, wait-then-paste, and the full CLI login path; each new behaviour test was shown to fail against the pre-fix code. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * docs: phase-7 review pass 3 + fixes (SPECS, plan, CHANGELOG) (review 3-6, 3-7) SPECS: invalid_grant keeps the entry, unknown-provider wording, CheckCredential never touches the network, 388 tests across 13 packages. Plan: review-fixes section, corrected implementation note, recorded Task 4 deviation. CHANGELOG v0.1.0 is "unreleased" until the tag. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * chore: refresh graphify-out after phase-7 review fixes Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(cmd): wire the revoke timeout; bound the login HTTP client (review 3-4 follow-up) The pass-3 fix declared revokeTimeout but never used it: runLogout still called provider.Revoke with the unbounded context, so a stalled revoke endpoint could hang the command while the note and plan claimed a 15s bound. Wired via context.WithTimeout; TestLogoutRevokeIsBounded (stalled endpoint, shrunk timeout) fails against the un-wired code. The login path's HTTP client was still unbounded (pass 3 asked for the token/revoke/JWKS client bound): moca login now uses a 30s client timeout; TestLoginTokenEndpointIsBounded (stalling token endpoint) fails against the unbounded client. Both new tests are mutation-proven load-bearing (go test -overlay scratch copies); full suite race-green at 390 tests; docs updated (SPECS 6.5, plan re-review section). * feat(agent,shipgate): hide rtk when the CLI is absent; gate 6 adapts to the host The first live ship-gate run on a host without rtk failed gate 1: the prompt hard-coded the rtk push, so the model's first action was a dead `rtk` command and it never read the failing test. moca must not advertise a tool the shell cannot run. agent.filterSkills drops builtin skills whose external CLI is missing from PATH (project/global skills are never filtered); PromptInput.RTK gates the rtk-preference line on exec.LookPath. The ship-gate checker's gate 6 adapts: rtk present -> prompt must advertise it and the session must use it; rtk absent -> the prompt must hide it. run.sh prints the mode. Verified: stripped-PATH end-to-end simulation passes with an rtk-free prompt; an rtk-built session under the stripped PATH fails exactly '6: rtk is not installed on this host but the prompt still advertises it'; the rtk-present rehearsal is unchanged; full suite race-green (392 tests). * feat(agent): working-style guidance — read the failing test before changing code; commit when asked The second live ship-gate run (rtk host) failed gate 1: the model diagnosed from the sources and fixed the bug without ever opening the failing test, which DESIGN §14 step 1 requires. The gate stands; the core prompt now carries the missing guidance. Also adds 'commit when the task asks' to the end-to-end bullet (gate 5 needs the commit). Both pinned by TestBuildSystemPrompt; the scripted rehearsals (rtk-present, rtk-absent, negative controls) are unchanged; full suite race-green (392 tests). * feat(agent,skills): steer file access to the tools; make the failing-test step tool-explicit The third live ship-gate run failed gate 1 again: the kept session shows the model read the failure text, shell-catted the sources (the edit guard then forced a tracked read of calc.go), fixed the bug and committed — but never opened the failing test and never used search. The rtk skill itself was reinforcing the drift: it taught rtk read/ls/grep/find for files. Fixes: a tools bullet (use read/search/ls for files, not shell cat/grep/find/ls — tracked reads are what edit accepts); the working-style line is tool-explicit (read the failing test file with the read tool, locate the cause with search, before changing code); the token-discipline rtk line is scoped to shell command output; the builtin rtk skill drops its file rows and defers to the tools. Checker and the frozen task prompt untouched. Pinned by TestBuildSystemPrompt; rehearsals (rtk-present/absent/negative) unchanged; full suite race-green (392 tests). * feat(tools): failing test runs carry a read-the-test hint The fourth live ship-gate run used search (the tool-explicit line landed) but still never opened the failing test: the model read the failure text, searched, hit the edit guard, read calc.go, fixed and committed. The prompt lever is exhausted for this model class, so the step is repeated where the attention is. A shell result whose command mentions a known test runner and whose run failed (exit != 0, or 'fail' in the output — piped runs mask the exit code) gets a trailing hint before the status line: read the failing test file with the read tool and locate the cause with search before changing code. Checker, frozen task prompt and tool schemas untouched. Pinned by TestTestFailureHint (pure) and TestShellToolHintsFailingTestRun (a real red go test through the tool); rehearsals (rtk-present/absent/negative) stay green; full suite race-green (394 tests). * feat(agent,tools): numbered investigation protocol for failing tests The fifth live run read calc_test.go and stats_test.go right after the failing suite run (gate 1 green, the hint worked) but gate 2 surfaced next: no search tool call at all (shell ls + reading all four files). Both remaining misses share a shape: a compound sentence loses its second half with this model. The failing-run hint is now a numbered protocol — (1) read the failing test file with the read tool, (2) locate the cause with the search tool, (3) only then edit — and the working-style line is split into two bullets (read the test; locate with search) so neither step rides on the other's coattails. Rehearsals (rtk-present/absent/negative) green; full suite race-green (394 tests). * feat(tools,agent,provider): enforce the failing-test investigation loop; bound provider stalls The live ship-gate runs showed the target model class treats tool-specific steps as satisfied by equivalents (a test run, cat, ls) and ignores hints. The §14 loop's investigation steps are now structural: while a failing test run is unresolved (Env.TestFailed/FailingTest/TestSeen/Searched), every tool result carries a registry-level banner naming the next step, and the edit tool refuses until the failing test has been read with the read tool and the cause located with the search tool. Also from the live iterations: the default HTTP client bounds the response-header wait at 120 s (a silent provider hung a run for 6+ minutes — the 90 s stall guard only covers the body), and HTTP 400s led by "type":"server_error" are retried (the opencode-go [1210] thinking-config flake). Live legs after these fixes: four consecutive green checker passes (2026-10-07, real glm-5.3-flash; evidence in docs/specs/SPECS.md §15 and the phase-7 plan). Tests 402, race-green; docs updated. --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
[moca](https://github.com/adeotek/moca) is a terminal coding agent in one static Go binary that loads Agent-Skills-standard SKILL.md files from ~/.config/moca/skills/<name>/ (or $XDG_CONFIG_HOME/moca/skills) and <project>/.moca/skills/<name>/. It reads its own AGENTS.md, but there is no graphify rules file and no plugin/hook surface, so the skill is the whole integration. Register it like the other skill-only platforms, reusing claude's split bundle (skill.md, byte-identical to pi's) plus the references/ sidecar, and honor XDG_CONFIG_HOME the way moca itself resolves its config dir. moca seeds a create-only copy of this skill (0.9.93) and never refreshes it; writing the current skill plus its .graphify_version stamp here is what lets graphify's own stale-skill check and auto-refresh keep that directory current. Tests: tests/test_moca.py covers the registration, both scope destinations, the XDG override, the installed layout, uninstall, CLI dispatch and the help text. README's platform table gains the row. Drafted with an AI coding assistant (Hermes Agent).
5ebf3cb to
d64b72d
Compare
|
Thanks for the review. Both findings are covered by the repo-wide sandbox, so no code change was needed there:
Also rebased onto current |
What
Adds a
mocaplatform tographify install --platform moca/graphify moca install|uninstall.moca (MIT) — Mo Coding Agent — is a terminal coding agent shipped as one static Go binary that loads Agent-Skills-standard
SKILL.mdfiles unchanged (verified against graphify's own skill in its ecosystem-compatibility corpus). It reads:~/.config/moca/skills/<name>/SKILL.md(or$XDG_CONFIG_HOME/moca/skills/<name>/SKILL.md)<project>/.moca/skills/<name>/SKILL.md(loaded for a trusted workdir)It has its own instructions file (
~/.config/moca/AGENTS.md, plus a trusted workdir'sAGENTS.md/CLAUDE.md) which graphify does not write into, and no plugin/hook surface — a skill-only registration likepi/agents.Why
moca ships a create-only seeded copy of this skill (0.9.93:
SKILL.md+references/) at~/.config/moca/skills/graphify/and never refreshes it. That copy carries no.graphify_version, so graphify cannot even warn that it is stale. This platform entry writes the current skill to the directory moca actually reads, plus the version stamp graphify's own stale-skill check and auto-refresh key on.moca's docs already name it as the refresh route: docs/specs/external-tools.md.
How
_PLATFORM_CONFIG["moca"]reuses claude's split bundle (skill.md, byte-identical to pi's) plus thereferences/sidecar._platform_skill_destination: user scope honorsXDG_CONFIG_HOME(moca resolves its config dir that way, falling back to~/.config); project scope is.moca/skills/graphify/SKILL.md._project_install/_project_uninstall: added to the skill-only tuple.graphify moca install|uninstall), both help-text lists, and the README platform table.tests/test_moca.py: platform registration, both scope destinations (incl. the XDG override), installed layout (SKILL.md + references), uninstall, CLI dispatch, help text.Verification
Rebased onto current
v8(the branch was 407 commits behind; the only conflict was the hand-maintained--helpplatform list that #4655 replaced with a derived expression — that line is dropped andmocais picked up from_PLATFORM_CONFIG).tests/test_moca.py(15 tests) plus the install / references / round-trip / help / wheel suites pass;ruff checkclean.Notes